refactor: move docs behind enterprise module slot - #1120
Conversation
github-actions
Bot
added
size/XL
Dependency Changes DetectedThis PR modifies dependency files. Please review whether these changes are intentional. Changed files:
Maintainer checklist:
|
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Review: #1120 — refactor: move docs behind enterprise module slot
意图:把 packages/docs(整包,382 文件)从 octo-web 移除,改为经可选的企业模块插槽加载——Vite 虚拟模块 virtual:octo-enterprise-modules,由 enterpriseModulesPlugin(VITE_ENTERPRISE_MODULES_ENTRY) 提供;未配置企业入口时回落为安全 stub。锚定 head 00c2e3ecbe5a(rev-parse 确认 checkout==live head)/ mergeable=true。2 commits(重构 + 修 extension 构建),rename-aware:5 新增 / 27 修改 / 382 删除(删除即 packages/docs)。
结论:APPROVE ✅
Gate 1 — 规格符合度 ✅
docs 从核心包抽出、经插槽按需加载;默认(社区)构建无 docs、企业构建注入。与标题一致。非 docs 专属的 /d/ 登录返回路径(standaloneReturn.ts)正确留在核心。无 over-build。
Gate 2 — 代码质量 ✅
- 插槽机制正确且默认安全:
enterpriseModulesPlugin的resolveId在无VITE_ENTERPRISE_MODULES_ENTRY时解析为\0virtual:...,load返回 no-op stub(registerEnterpriseModules(){}+getEnterpriseStandaloneDocCapability(){return null})→ 默认构建无 docs、无悬空引用即可运行;有入口时校验路径存在(不存在则抛错)、扩展server.fs.allow。enterpriseHtmlHead插件按部署注入<head>。 - 消费端 null-safe:
Layout用getEnterpriseStandaloneDocCapability()?.isStandaloneDocPath(...)(可选链)——docs 缺席时/d/自然回落普通应用,不崩;renderStandaloneDocPage仅在 capability 存在时调用。 standaloneReturn.ts安全:登录返回路径持久化 +isSafeReturnPath——拒非同源(url.origin===origin)、拒控制字符、仅允/d/|/s/白名单,sessionStorage不可用 try/catch 降级。无开放重定向面。- 无悬空
@octo/docs引用(全仓 grep 0 命中);packages/docs已从 workspace 移除;extension(wxt.config.ts,第 2 commit)同装插槽插件。 - 构建代理清理一致:vite dev 的
/api/v1/docs代理随 docs 移除而删。
构建 / 测试(实跑 @ 00c2e3ecbe5a,默认无企业入口):pnpm --filter @octo/web build ✅ 2.70s(证无悬空 docs 引用);受影响 5 测试文件 47/47 全绿——standaloneReturn(3,开放重定向守卫)、enterpriseHtmlHead(7,直接断言 stub 的 resolveId/load 契约)、docsOn(10)、layoutStandaloneDocPath(11)、mainMenuReconcile(16)。
🟡 非阻断 / human-verify(大型重构,建议确认)
- 社区默认构建 docs 能力整体缺席(Docs 导航 /
/d/独立页 / 云文档搜索 tab / 文档分享卡预览等)——这是本 PR 的明确意图(docs 转企业模块),与既有docsOn远程开关方向一致;请产品/运维确认这是预期结果,并确保企业部署管线正确设置VITE_ENTERPRISE_MODULES_ENTRY。 - 跨仓契约:企业侧 docs 模块须实现
registerEnterpriseModules+EnterpriseStandaloneDocCapability(isStandaloneDocPath/parseStandaloneDocId/renderStandaloneDocPage)接口——octo-web 内无法核实,建议与企业模块仓逐条对齐(此前各轮 docs PR 的代码现移到该仓)。 - 部署管线(Dockerfile / docker-entrypoint.sh / nginx.conf.template / ci.yml)我做的是表层审阅(构建通过、方向合理),未逐行核实企业入口在生产/CI 的注入与 docs 反代路由变化;建议运维在企业构建流水线上实测一次
/d/独立页与 docs API 反代确实生效。
插槽机制正确(默认 stub 安全、有入口才加载)、消费端全 null-safe、standaloneReturn 开放重定向守卫齐、无悬空 @octo/docs 引用、extension 构建同步修复;默认构建通过、47 测试(含 stub 契约与菜单/独立页回落)全绿。可合并 —— APPROVE(建议合并前确认:社区构建 docs 缺席为预期 + 企业模块接口对齐 + 企业部署管线注入入口)。
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
This refactor cleanly moves the docs feature behind a virtual enterprise module slot. Verified the gating mechanism, dead-reference removal, type safety, and open-source/enterprise parity against the head commit.
✅ Slot-gating mechanism — correct
apps/web/vite.enterpriseHtml.tsdefines thevirtual:octo-enterprise-modulesslot. WhenVITE_ENTERPRISE_MODULES_ENTRYis unset,resolveIdreturns the internal virtual id andloadsupplies a no-opregisterEnterpriseModules()plusgetEnterpriseStandaloneDocCapability()returningnull. So open-source builds ship a no-op — docs is never registered or mounted. When the entry env var IS set, it resolves to the enterprise entry and the real module registers. Correct conditional mount for both build variants.apps/web/src/index.tsxdrops the direct@octo/docsimport andnew DocsModule()registration, replacing it withregisterEnterpriseModules({ registerModule }). No dead import remains.apps/web/src/Layout/index.tsxreplaces the former direct@octo/docsimports (StandaloneDocPage,parseStandaloneDocId,isStandaloneDocPath) withgetEnterpriseStandaloneDocCapability(); the optional-chained call (capability?.isStandaloneDocPath(...)) means non-enterprise falls through and never renders a standalone doc page.persist/consumeStandaloneReturnnow come from the new host-ownedapps/web/src/Layout/standaloneReturn.ts, so the anonymous login-return stashing survives without the docs package.- Type safety:
apps/web/src/enterprise-modules.d.tsfully types the slot (EnterpriseModulesContext,EnterpriseStandaloneDocCapability) with noany. - Host cleanup is consistent: docs-specific CI (
.github/workflows/ci.yml), Dockerfile,docker-entrypoint.sh,nginx.conf.templateproxy/CSP,apps/web/index.htmldeep-link capture, and the/api/v1/docsdev proxy inapps/web/vite.config.tsare all removed. Source-grep guard tests (docsOn.test.ts,layoutStandaloneDocPath.test.ts,enterpriseHtmlHead.test.ts) lock the wiring, and CI is green. packages/dmworkbase/*andPages/Main/{vm,menuReconcile}.tschanges are comment-only generalizations (removing@octo/docs/docs_on-specific wording); no behavior change.
💬 Non-blocking notes
- 🟡
apps/web/package.json:44—test:e2e:standalonestill runse2e-kit/tests/standalone-doc/standalone-doc.spec.ts, but this PR deletes that spec. The documented dev script will now fail with "no tests found". Recommend removing the script (and its:installsibling) or repointing it to the enterprise/private location if the standalone e2e still lives there. Dev-tooling only; not shipped and not exercised by CI. - 🟡 Anonymous
/s/share/:shareIdlogin-return is stashed atapps/web/src/Layout/index.tsxbutconsumeStandaloneReturn()rejects it, becausestandaloneReturn.tsonly allows/d/:idor single-segment/s/:taskNo(/s/share/xfails the^\/s\/([A-Za-z0-9_-]+)\/?$check). Byte-verified this is a faithful 1:1 port of the prior@octo/docsvalidator (identical regexes), so it is a pre-existing latent gap, NOT a regression introduced here — flagging as a follow-up: add a/s/share/:shareIdsafe-path branch plus a unit test. - 🟡
enterpriseModulesPlugin(apps/web/vite.enterpriseHtml.ts) returns a bare package specifier directly fromresolveId, which short-circuits normal package resolution. Fine if internal deployments only pass absolute/relative paths; otherwise considerthis.resolve(id, ..., { skipSelf: true }).
✅ Highlights
- The new host-owned
standaloneReturn.tskeeps the open-redirect guard intact (single-leading-slash, control-char rejection, same-origin check) even without the docs package. - Enterprise parity preserved: when the slot is enabled, standalone doc rendering, deep-link capture, and menu reconciliation route through the capability interface unchanged.
git diff --checkpasses; CI (build, e2e-p0, lint, scans) is green and the branch is mergeable.
No blocking issues. The two dev/pre-existing items above are worth a quick follow-up but do not affect the correctness of the gating refactor.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1120 (octo-web)
Reviewed at head SHA 00c2e3ecbe5a2275ccef7df68ab88842e5baaa46, merge-base 00c5572e.
Read in full: the 1,434-line non-packages/docs diff, plus targeted reads of the working tree at
head. The ~93k pure deletions under packages/docs/** and the 17.5k-line pnpm-lock.yaml were
verified structurally rather than line-by-line (see Coverage). Two independent automated
second-opinion passes were also run against the same diff and their findings are reconciled in §5;
where they disagreed with my reading I re-derived the answer from source rather than splitting the
difference.
1. Spec compliance
Spec: OK
All four declared deliverables are present and verified:
| Claim | Verification |
|---|---|
Remove packages/docs + @octo/docs registration |
grep -rn "@octo/docs" over the tree at head returns zero hits; apps/web/package.json drops the workspace:* dep; apps/web/src/index.tsx:15,81 replaces DocsModule with registerEnterpriseModules |
| Add virtual enterprise module slot + optional HTML head injection | apps/web/vite.enterpriseHtml.ts (new), apps/web/src/enterprise-modules.d.ts (new), wired at apps/web/vite.config.ts:67-68 |
| Remove OSS docs-specific CI / Docker / nginx proxy+CSP / deep-link wiring | .github/workflows/ci.yml (docs vitest step), Dockerfile (6 VITE_DOCS_* ARGs), nginx.conf.template (/docs-html/, /api/v1/docs, ${DOCS_ASSET_CSP_ORIGIN}, esm.sh), apps/web/index.html (inline capture script) |
| Keep host compatibility points | getEnterpriseStandaloneDocCapability() consumed at apps/web/src/Layout/index.tsx:426-439; docsOn retained at packages/dmworkbase/src/App.tsx:279; menuReconcile.ts / vm.ts changes are comment-only |
Over-build: none. The pnpm-lock.yaml diff is exactly the removal of the packages/docs
importer and the @octo/docs link from apps/web, plus resolution pruning — no unrelated
dependency version changed (filtered specifier:/version: scan over the lock diff shows no
importer-level drift outside packages/docs).
Deviation: none. The packages/dm* / whiteboard-schema / Pages/Main touches are
comment-and-docstring de-branding only; verified hunk by hunk, no logic changed.
Two pieces of removal residue survive. They are dead config with no runtime or CI effect, so they
do not fail this gate — see P2-6.
2. Code quality
Quality: Approved — no P0, no P1. Eleven P2 items follow, ordered by how likely they are to bite.
Note up front: the two second-opinion passes rated items P2-1, P2-2, P2-4 and P2-5 as blocking.
I am overruling that to non-blocking, with reasoning in §5 — briefly, none of them is reachable in
the artifact this PR actually produces, because every one requires enterprise-only build
configuration that does not exist in this repo yet.
P2-1 — enterpriseHtmlHeadPlugin corrupts head HTML containing $-replacement patterns
apps/web/vite.enterpriseHtml.ts:17-21
transformIndexHtml(html) {
const trimmed = headHtml.trim();
if (!trimmed) return html;
return html.replace(/<\/head>/i, `${trimmed}\n </head>`); // :20
}The second argument to String.prototype.replace is a replacement string, so $&, $`,
$', $$ and $<name> inside the enterprise head file are expanded rather than inserted
verbatim. Reproduced locally:
head input : <script>var s="a$&b";</script>
output : <html><head><title>t</title><script>var s="a</head>b";</script>\n </head></html>
$& expanded to the matched </head>, injecting a premature </head> inside a <script> string
literal and truncating the document head. Any enterprise head snippet containing an analytics
regex, a jQuery-style $&, or a templating token hits this silently — the build succeeds and the
HTML is malformed.
Fix — the replacer-function form never interprets $:
return html.replace(/<\/head>/i, () => `${trimmed}\n </head>`);P2-2 — the slot fails open, and the Dockerfile cannot pass the entry at all
apps/web/vite.enterpriseHtml.ts:25-34, 56-77 + Dockerfile
Two independent halves that compound into one problem.
Half one — fail-open. resolveEnterpriseEntry returns null when
VITE_ENTERPRISE_MODULES_ENTRY is unset, and resolveId/load then serve the stub (:74-75):
"export function registerEnterpriseModules(_context) {}",
"export function getEnterpriseStandaloneDocCapability() { return null }",No log line, no opt-in assertion. Contrast the sibling paths, which correctly fail closed:
readEnterpriseHtmlHead propagates readFileSync's ENOENT (:8-12) and resolveEnterpriseEntry
throws explicitly for a non-existent relative/absolute path (:30-32). The module slot is the one
that silently degrades.
Half two — the Dockerfile has no ARG at all. After this PR, grep -n "ARG" Dockerfile returns
nothing: the six VITE_DOCS_* ARG/ENV pairs were deleted and no VITE_ENTERPRISE_* pairs
were added. Docker requires an ARG declaration for --build-arg to reach the build stage, so
docker build --build-arg VITE_ENTERPRISE_MODULES_ENTRY=… is a no-op against this Dockerfile.
Together: an enterprise image build wired the obvious way produces a green build that silently
ships the open-source stub, and the failure only surfaces in production as "the Docs entry is
gone". This is the single highest-value fix in the review even though it does not block the OSS
merge.
Suggestion: declare ARG/ENV for the three VITE_ENTERPRISE_* variables, and add either a
VITE_ENTERPRISE_REQUIRED=1 guard that throws when the entry resolves to the stub, or at minimum
console.info("[enterprise-slot] using open-source no-op stub") so the build log records which
variant was produced.
P2-3 — env source diverges between the web and extension builds
apps/web/vite.config.ts:13 uses loadEnv(mode, process.cwd(), "VITE_"), which reads
.env / .env.[mode] / .env.local and process.env.
apps/extension/wxt.config.ts:11-13, 27-30 reads process.env only.
So an enterprise entry configured in apps/web/.env.production applies to the web build and is
invisible to the extension build, which then silently falls back to the stub (P2-2). This is not
hypothetical — the extension really does pull the host in:
apps/extension/entrypoints/sidepanel/main.tsx:18 imports ../../../web/src/App, which reaches
Layout/index.tsx and therefore virtual:octo-enterprise-modules. That is exactly what the second
commit (00c2e3ec, "fix: resolve enterprise slot in extension build") was fixing.
Suggestion: have the extension config call loadEnv against webRoot so both builds read one
source of truth.
P2-4 — the bare-specifier entry branch short-circuits Vite's resolver
apps/web/vite.enterpriseHtml.ts:25-34, 67-70
if (!entryPath.startsWith(".") && !path.isAbsolute(entryPath)) return entryPath; // :27
...
resolveId(id) {
if (id !== ENTERPRISE_MODULES_ID) return undefined;
return entry || RESOLVED_ENTERPRISE_MODULES_ID; // :69
}Line 27 is clearly meant to support a bare package specifier (e.g. @octo-enterprise/modules). But
returning a non-absolute string from resolveId tells Rollup/Vite "this is the final resolved
id" — subsequent resolvers, including vite:resolve, are skipped, and since the plugin's own
load only handles the \0virtual: id, the load falls through to the default filesystem loader
and fails. The plugin is enforce: "pre" (:55), so it runs ahead of vite:resolve and there is
no fallback.
Tests cover only the absolute-path and unset cases
(apps/web/src/__tests__/enterpriseHtmlHead.test.ts:52-70), so this branch is unexercised. Either
resolve properly — (await this.resolve(entryPath, undefined, { skipSelf: true }))?.id — or reject
bare specifiers with the same explicit error the path branch already uses, rather than accepting
them and failing late with an opaque Rollup message.
P2-5 — the proxy / CSP / Docker layer has no extension point, and two in-repo consumers still depend on it 🔎 human verification requested
The item most worth a human eye, and the one all three passes converged on independently. The PR
adds build-time seams for JS modules and the HTML head, but the runtime/deployment layer
was removed outright with no equivalent seam:
nginx.conf.template—location /docs-html/andlocation /api/v1/docsdeleted./api/v1/docsnow falls into the genericlocation /api/;/docs-html/*falls into the SPA handler.nginx.conf.template:37—${DOCS_ASSET_CSP_ORIGIN}dropped fromimg-src(sohttp:object-store presign images are excluded),https://esm.shdropped fromfont-src.apps/web/vite.config.ts— the/api/v1/docs→VITE_DOCS_API_URLdev proxy deleted.Dockerfile— theVITE_DOCS_*ARGs deleted (and no replacements added, see P2-2).
Meanwhile two consumers remain in the open-source host, both unconditional — not gated on docsOn
or on the capability:
- Preview fetch.
packages/dmworkbase/src/module.tsx:318-319registersDocumentShareCardCell;Messages/DocumentShareCard/preview.ts:146issuesAPIClient.shared.get("docs/{docId}/{endpoint}")againstapiURL = "/api/v1/"(apps/web/src/index.tsx:43). With the longer-prefix location gone that request reaches octo-server, which — per the very comment this PR deletes — has no/api/v1/docsroutes. - Open action.
Messages/DocumentShareCard/index.tsx:66-71—handleOpenbuilds/d/:docIdviabuildDocNavUrlandwindow.opens it. In an OSS buildgetEnterpriseStandaloneDocCapability()returnsnull, so theLayout/index.tsx:427guard is false and/d/:docIdfalls through to the app shell instead of a standalone terminal.
Why this is P2 rather than P1, despite both second-opinion passes calling it blocking: in an OSS
deployment DOCS_BACKEND_URL defaulted to blank, so the old location already returned 503,
which requestPreview's catch maps to {status:"error"} (preview.ts:160-164); it now maps 404
to {status:"unavailable"}. And the Open action already dead-ended before this PR — the standalone
preflight had no backend to talk to, so it rendered a docs error terminal. Both paths were broken
before and are broken after; what changed is which dead-end UI appears. That is a UX delta on an
already-dead path, not a newly broken feature.
Please confirm with ops before the enterprise rollout: (a) the enterprise nginx/gateway restores
/api/v1/docs and /docs-html/; (b) font-src re-allows the Excalidraw font host, or the fonts
are self-hosted, before the whiteboard ships again; (c) img-src re-allows the object-store presign
origin; (d) the enterprise image passes VITE_DOCS_* and VITE_ENTERPRISE_* some other way.
For the OSS artifact itself the CSP tightening is safe and correct — grep -rni "esm.sh" and
grep -rn excalidraw --include=package.json both return zero hits at head, so neither allowance
has a consumer left. A reasonable in-repo hardening either way: capability-gate the
DocumentShareCardCell registration, or give the card an explicit "unavailable in this deployment"
state instead of letting it fail open into a generic error.
P2-6 — incomplete-removal residue
apps/web/package.json:44-45still definestest:e2e:standaloneandtest:e2e:standalone:install, pointing ate2e-kit/tests/standalone-doc/standalone-doc.spec.ts— a path this PR deletes (lsconfirms the directory is gone). The script now fails with "no tests found".apps/web/package.jsonis already touched by this PR, so this is one-line follow-through..i18n/scan-config.json:24, 32, 36, 40still list fourpackages/docs/**files underignoredFiles. Harmless —scripts/i18n-scan.mjs:59-62only does aSet.haslookup, so absent paths never match, and the scanner does not validate that exemptions exist — but they read as live policy to the next contributor..github/workflows/e2e-nightly.yml:66referencestests/standalone-doc/in an exclusion comment.
P2-7 — newly orphaned in-repo assets carry no "kept on purpose" marker
After the extraction these have zero in-repo consumers:
packages/whiteboard-schema/**— its only consumer waspackages/docs' board collab (grep -rn "whiteboard-schema"outside the package itself returns nothing)packages/dmworkbase/src/Utils/docLink.ts,Messages/DocumentShareCard/**, and thedocsOnremote-config field are retained but unreachable from an OSS build
PR bullet 4 says this is deliberate, and the docsOn docstring was updated to say so
(App.tsx:271-278). whiteboard-schema and docLink.ts did not get the same treatment — a
one-line "consumed by the private enterprise docs module; do not delete as dead code" note in each
would stop a future cleanup PR from removing them.
P2-8 — none of the tests this PR writes are executed by CI
.github/workflows/ci.yml removes the pnpm --filter @octo/docs exec vitest run … step, which was
the only vitest invocation besides pnpm --filter @dmwork/skillmarket test (ci.yml:88-98). The PR
adds or rewrites five test files under apps/web (enterpriseHtmlHead.test.ts,
Layout/standaloneReturn.test.ts, docsOn.test.ts, layoutStandaloneDocPath.test.ts,
mainMenuReconcile.test.tsx) — CI runs none of them; per the PR body they were verified by hand.
This is a pre-existing gap rather than a regression (apps/web tests were never in CI), but the step
being deleted was the last vitest gate near this code, and a build-time plugin is exactly the kind
of logic that rots silently. Adding pnpm --filter @octo/web exec vitest run alongside the
skillmarket step would make the new coverage load-bearing.
P2-9 — extension dev server may 403 its own entrypoints once the slot is configured
apps/extension/wxt.config.ts:27-30 passes webRoot as the plugin root, and
vite.enterpriseHtml.ts:56-66 returns server.fs.allow = [rootDir, entryDir, ...extraFsAllow].
Explicitly setting server.fs.allow replaces Vite's default workspace-root allowance, and the
computed list contains apps/web but never apps/extension — so WXT entrypoint/HMR files served
from apps/extension could be rejected during enterprise extension development. Only triggers once
an entry or extra allow-path is configured; with both unset config() returns undefined (:57)
and nothing changes. Dev-only. I could not execute wxt dev to confirm the 403 (no node_modules),
so this one is reasoned, not observed.
P2-10 — no dev-server watcher for VITE_ENTERPRISE_HTML_HEAD_PATH
apps/web/vite.enterpriseHtml.ts:8-12 reads the head file once, synchronously, at config time and
never registers it via server.watcher.add. Editing it during pnpm dev produces no HMR and no
full reload; the developer must restart the dev server and will likely conclude injection is broken.
P2-11 — transformIndexHtml silently no-ops when </head> is absent
apps/web/vite.enterpriseHtml.ts:20 — if the regex does not match, replace returns the input
unchanged and the configured head HTML is dropped with no diagnostic. apps/web/index.html always
has a </head>, so this is latent, but configured-then-silently-ignored input deserves a warning.
3. Security review (PR classified security_sensitive)
What I checked and where it landed:
server.fs.allowwidening viaVITE_ENTERPRISE_FS_ALLOW—vite.enterpriseHtml.ts:36-43, 56-66. Additive torootDir, deduped, and returned only underconfig().server.fs.allow, so it affects the dev server only, not the production artifact. Operator-controlled input. Acceptable. Worth noting the test itself allowlistsnode_modules(enterpriseHtmlHead.test.ts:76-83) — please confirm this variable is never set for a network-exposedvite dev/vite preview. See also P2-9 for the flip side of replacing the default allowlist.- Unsanitized HTML injected into
<head>—readEnterpriseHtmlHeadwill read any absolute path and the plugin splices it in verbatim. The trust boundary is whoever runs the build, i.e. the same boundary asvite.config.tsitself, so this is not a privilege escalation. It does interact badly with P2-1. - CSP — strictly tightened:
${DOCS_ASSET_CSP_ORIGIN}removed fromimg-src(closing a configurablehttp:image source) andesm.shremoved fromfont-src(closing a third-party supply-chain surface). Verified no remaining consumer of either at head. Good change; see P2-5 for the enterprise-side caveat. - Open-redirect guard preserved across the move —
apps/web/src/Layout/standaloneReturn.ts:24-42is a faithful transcription of the XIN-392 hardening previously atpackages/docs/src/pages/StandaloneDocPage.tsx:91-110: reject C0/DEL control characters beforenew URL()(so a smuggled/\n/evil.example.comcannot normalize off-origin), then requireurl.origin === origin, then require the pathname to match the/d/:idor/s/:idallowlist.consumeStandaloneReturnstill clears the key unconditionally, including on the unsafe path (:44-56), so a rejected target cannot leak into a later login. The regression test covers all five bypass shapes (standaloneReturn.test.ts:33-47). No weakening.withReturnSidwas dropped, which is correct — it was already unused; the host usesremoveSidFromPath(Layout/index.tsx:187). envsubstallowlist vs template placeholders —docker-entrypoint.sh:24narrows to${API_URL} ${SUMMARY_API_URL} ${MARKET_API_URL}, andgrep -nE 'DOCS_ASSET_CSP_ORIGIN|DOC_APP_URL|DOCS_BACKEND_URL'overnginx.conf.templatereturns nothing. No placeholder is left unsubstituted, so nginx cannot boot with a literal${…}inside aproxy_pass. Consistent.- Deep-link capture script removal —
apps/web/index.htmlno longer writes theocto.docs.targetsessionStorage mirror, and nothing in the tree reads that key any more, so no half-wired storage contract is left behind.
4. Suggested changes
vite.enterpriseHtml.ts:20— switch to the replacer-function form ofreplace(P2-1). One line; removes a silent HTML-corruption class.Dockerfile— declareARG/ENVfor the threeVITE_ENTERPRISE_*variables, and log or assert which slot variant the build produced (P2-2). Highest value item here.wxt.config.ts— read env throughloadEnvlikevite.config.tsdoes (P2-3), and include the extension root in the computedfs.allow(P2-9).vite.enterpriseHtml.ts:27— either resolve bare specifiers viathis.resolve()or reject them explicitly (P2-4).apps/web/package.json:44-45— drop the twotest:e2e:standalone*scripts; prune the fourpackages/docsentries from.i18n/scan-config.json(P2-6).ci.yml— addpnpm --filter @octo/web exec vitest runso the five test files this PR ships actually gate (P2-8).- Add a "kept for the private enterprise module" note to
packages/whiteboard-schemaandUtils/docLink.ts(P2-7). - Track the proxy / CSP / Docker-ARG seam as a named follow-up issue rather than leaving it implicit in the PR description (P2-5).
None of these block merge.
5. Reconciling the second-opinion passes
Two independent automated passes reviewed the same diff. Recording where they agreed, where they
conflicted, and how I ruled — rather than flattening it into a single list.
Consensus (≥2 passes hit the same spot independently — highest confidence, fix these first)
| Finding | Passes | Ruling |
|---|---|---|
$-pattern corruption in transformIndexHtml |
A + mine | Confirmed by local repro → P2-1 |
Bare-specifier resolveId short-circuit |
A + mine | Confirmed by reading Rollup resolve semantics → P2-4 |
| No runtime counterpart for the frontend slot (nginx/CSP/Docker) | A + B + mine | Confirmed → P2-5 |
DocumentShareCard still targets the removed docs API |
A + mine | Confirmed, and A additionally caught that the Open action dead-ends → folded into P2-5 |
Extension reads process.env while web reads loadEnv |
A + mine | Confirmed → P2-3 |
Stale packages/docs exemptions in .i18n/scan-config.json |
A + mine | Confirmed → P2-6 |
Contradictions (both positions stated; ruled on evidence, not averaged)
- Pass B, rated P1:
persistStandaloneReturnwill lose?sp=because the host'sRouteManagerre-pushespathnameonly and wipes the query before React renders. Refuted.standaloneReturn.ts:12-21is a byte-for-byte move ofStandaloneDocPage.tsx:58-69at the merge-base, so this PR changes no behavior there. Independently,RouteManager.renderCurrentPath(packages/dmworkbase/src/Service/Route.tsx:121-133) no longer re-pushes the URL at all, and its own comment at:119states that legacy standalone routes including/d/:docIdcold-loads are "byte-identical to before". The wipe the deletedindex.htmlscript worked around applied to/docs, which no longer exists in this repo. Not a finding. - Pass B, rated P2: removing
esm.shfromfont-srcbreaks Excalidraw fonts now. Downgraded — true for a future enterprise build, but there is no@excalidrawdependency and noesm.shreference anywhere at head, so it is a deployment-seam concern, not a defect in this diff. Folded into P2-5, where Pass A's framing ("an extracted implementation cannot work in this image without an overlay") is the accurate one. - Pass A rated P2-1, P2-2, P2-4 and P2-5 as blocking; I rate all four non-blocking. Ruling: each requires enterprise-only build configuration (
VITE_ENTERPRISE_MODULES_ENTRYset, a head file containing$&, a bare-specifier entry, an enterprise nginx) that does not exist in this repo, and the enterprise deployment pipeline is explicitly declared an open ops item in the PR body. In the artifact this PR produces — the open-source host — the stub path is the intended behavior and all CI is green. Severity therefore follows reachability, not theoretical impact. - Pass B, P2: hash fragments dropped by
persistStandaloneReturn. True, but pre-existing and unchanged here. Nit only, not listed.
Unique to one pass (kept even though only one source raised them)
- Pass A only: Dockerfile declares no
ARGfor the newVITE_ENTERPRISE_*variables — verified (grep -n ARG Dockerfileis empty) and the strongest single catch of the review; merged into P2-2. Also the extensionfs.allow403 risk → P2-9, and the card's Open-action dead-end → P2-5. - Pass B only: no dev-server watcher for the head file → P2-10.
- Mine only: dangling
test:e2e:standalone*scripts (P2-6), orphanedwhiteboard-schema/docLink.ts(P2-7), CI executes none of the new tests (P2-8), silent no-op when</head>is absent (P2-11), plus the negative results in §3 and the two checks below.
Negative results worth recording (things that looked risky and are in fact fine)
- NavRail ordering is safe.
registerEnterpriseModulesnow runs last (index.tsx:81-83) whereDocsModulepreviously sat betweenAppBotModuleandLoopModule, butMenusManager.registertakes an explicitsort(packages/dmworkbase/src/Service/Menus.ts:12-16) and the old module passed4002, so registration order does not affect rendered order. Registration still precedesstartup(). - Lockfile is clean. No transitive or direct dependency version changed outside the removed importer.
Both passes completed. Neither leg was absent, timed out, or errored, so nothing in this report
rests on an unanswered second opinion.
6. Coverage — what this review did not establish
- Nothing was executed. No
node_modulesin the review checkout and no install was attempted, so unit tests,tsc,wxt dev, and the enterprise-slot build were not run locally. Findings come from source reading plus one isolatednode -erepro for P2-1. The PR's green CI is the substitute evidence — and per P2-8 that CI runs none of the new tests. P2-9 in particular is reasoned, not observed. - The private enterprise module was not reviewed. The contract in
enterprise-modules.d.tsis unverified againstdmwork/octo-web-enterprise/octo-docs-module: whether it exports both functions with these signatures, and whetherrenderStandaloneDocPagereproduces the deletedStandaloneDocPage's reader / writer / forbidden-with-request / not-found / archived terminals, is outside this repo. Related:registerEnterpriseModulesis typed synchronous (enterprise-modules.d.ts:18) and called withoutawait(index.tsx:81) — if the private module ever needs a dynamicimport()to register, that contract has to change. Worth confirming now rather than after the first async module. - Enterprise deployment assembly unverified (nginx / gateway / CI-CD) — explicitly open per the PR body; see P2-5.
- The ~93k deleted lines under
packages/docs/**were not read line-by-line. I verified the deletion is total and that no dangling reference survives —@octo/docs,packages/docs,VITE_DOCS_*,/api/v1/docs,esm.sh,excalidraw,octo.docs.targetall grep clean except the residue in P2-6 — but I did not audit what capability is lost. pnpm-lock.yaml(17.5k lines) was checked only for importer-level version drift, not transitive-resolution correctness;osv-scananddependency-reviewpassing is the other half of that evidence.- No runtime behavior was exercised. No browser check of
/d/:docIdfalling through to the app shell in an OSS build, and no check of howDocumentShareCardrenders against a 404 rather than the previous 503.
7. Overall verdict
APPROVED — Spec OK and Quality Approved.
The extraction is careful and unusually well-documented: the open-source host has no dangling
reference to the removed package, the security-relevant open-redirect guard survived the move
intact, the CSP moved strictly in the tightening direction, and the envsubst variable list stays
in sync with the template. All 15 checks are green and the PR is MERGEABLE.
Everything found is P2. The two that most deserve a follow-up before the enterprise side ships are
P2-2 (the Dockerfile cannot pass VITE_ENTERPRISE_* at all, and the slot fails open, so an
enterprise image can build green with the docs feature silently absent) and P2-5 (the
proxy/CSP/Docker layer has no seam matching the new JS/HTML seams). Neither affects the open-source
artifact this PR produces, which is why they do not block merge — but they will bite the first real
enterprise build, and the needs-human-review label is warranted for exactly that reason.
4 participants
Summary
Private follow-up already handled
Validation
Notes