feat: move loop and personal behind enterprise slot by menghao-webtest · Pull Request #1145 · Mininglamp-OSS/octo-web

menghao-webtest · 2026-07-29T02:38:39Z

Summary

remove open-source Loop and Personal workspace modules from the host app
add a Vite enterprise module slot for private module registration and standalone handlers
add optional enterprise HTML head injection through VITE_ENTERPRISE_HTML_HEAD_PATH
remove Loop-specific host e2e assets and keep docs registered in the host for now

Verification

git diff --check
pnpm --dir apps/web exec vitest run src/__tests__/enterpriseHtmlHead.test.ts src/__tests__/enterpriseModuleSlot.test.ts src/__tests__/layoutStandaloneDocPath.test.ts src/Layout/__tests__/standaloneReturn.test.ts
pnpm --dir apps/web build
pnpm --dir apps/extension build
VITE_ENTERPRISE_MODULES_ENTRY=/Users/houmenghao/project/octo/octo-enterprise-modules/src/loop.ts VITE_ENTERPRISE_FS_ALLOW=/Users/houmenghao/project/octo/octo-web:/Users/houmenghao/project/octo/octo-enterprise-modules:/Users/houmenghao/project/octo/octo-loop-module:/Users/houmenghao/project/octo/octo-web/node_modules pnpm --dir apps/web build
pnpm --dir apps/web build:e2e

Notes

Docs remains host-owned in this PR.
Internal CI must inject private enterprise entries through VITE_ENTERPRISE_MODULES_ENTRY.
Docker builds can pass VITE_ENTERPRISE_MODULES_ENTRY, VITE_ENTERPRISE_HTML_HEAD_PATH, and VITE_ENTERPRISE_FS_ALLOW as build args.
VITE_ENTERPRISE_FS_ALLOW only affects the Vite dev server filesystem allow-list.

github-actions · 2026-07-29T02:38:52Z

Dependency Changes Detected

This PR modifies dependency files. Please review whether these changes are intentional.

Changed files:

apps/web/package.json
packages/dmloop/package.json
packages/dmpersonal/package.json
pnpm-lock.yaml

Maintainer checklist:

Confirm dependency changes are intentional
Review package delta if lockfile changed

lml2468

Review: #1145 — feat: move loop and personal behind enterprise slot

意图:把 dmloop + dmpersonal(闭源企业模块)从 octo-web 移除,改由通用企业模块插槽按需加载;docs 仍留在 host。锚定 head dbf470f047e3(rev-parse 确认 checkout==live head)/ mergeable=true,merge-base 343f904c(#1139)。rename-aware:4 新增 / 9 修改 / 226 删除(删除即 dmloop+dmpersonal)。

结论:APPROVE ✅(架构层面;含合并协调 human-verify)

Gate 1 — 规格符合度 ✅

移除 loop/personal、经插槽加载;docs 保留 host(测试断言 index.tsx 仍静态 import @octo/docs/DocsModule 但不 import @octo/loop|personal)。与标题一致,无 over-build。

Gate 2 — 代码质量 ✅

插槽默认安全:createEnterpriseModulesVirtualModule([])(entry 未设时)返回 no-op stub:registerEnterpriseModules(){} + getEnterpriseStandaloneHandlers(){ return [] } → 默认(社区)构建无 loop/personal、无悬空即可运行。有 entries 时按 path.delimiter 多入口聚合,callEnterpriseExport(typeof fn === 'function' 守卫)+ appendEnterpriseHandlers(Array.isArray 守卫)防某入口缺导出。
消费端 null/empty-safe:Layout getEnterpriseStandaloneHandlers().find(h => h.match(pathname))(:395)→ 空数组 find 得 undefined → 回落普通应用;命中才 render(...),persistReturnOnAnonymous 时存返回路径。通用 handler 数组(id/match/render)比 #1120 的 docs 专用 capability 更泛化。
无悬空引用:@octo/loop/@octo/personal/@dmwork/loop/@dmwork/personal/dmloop/dmpersonal 全仓 import 引用 均为 0;两包目录已移除;App.tsx/index.tsx/package.json 相应去注册/去依赖。
无安全/数据面(纯模块装载重构)。

构建 / 测试(实跑 @ dbf470f047e3,默认无 entry):pnpm --filter @octo/web build ✅ 4.10s(证无悬空 loop/personal 引用);enterpriseModuleSlot.test + enterpriseHtmlHead.test 2 文件 9/9 全绿(锁定:index 不静态 import loop/personal、Layout 用 getEnterpriseStandaloneHandlers、docs 仍在 host)。

🟡 human-verify(大重构,建议合并前确认)

⚠ 与 #1120 的插槽 API 冲突/合并协调(重点):#1120(docs 抽出)引入的是 docs 专用 getEnterpriseStandaloneDocCapability,本 PR 把它泛化为 getEnterpriseStandaloneHandlers()(handler 数组)且 docs 留 host——两 PR 都改 enterprise-modules.d.ts/vite.enterpriseHtml.ts/Layout,API 分歧。两者合并会冲突,需协调合并顺序并统一到本 PR 的泛化 handler 模型(测试已断言 Layout 不再用 getEnterpriseStandaloneDocCapability)。
社区默认构建 loop/personal 能力整体缺席——本 PR 明确意图,请产品/运维确认为预期,并确保企业部署管线用 VITE_ENTERPRISE_MODULES_ENTRY(多入口 path.delimiter 分隔)注入 loop+personal。
跨仓契约:企业侧 loop/personal 模块须实现 registerEnterpriseModules + getEnterpriseStandaloneHandlers(id/match/render/persistReturnOnAnonymous),octo-web 内不可验,建议与企业模块仓对齐。

插槽默认 no-op stub 安全、多入口聚合带防御守卫、消费端空数组安全、零悬空 loop/personal 引用、docs 保留 host;默认构建通过、9 测试锁定接线。可合并 —— APPROVE(务必先协调与 #1120 的插槽 API 冲突、确认社区构建 loop/personal 缺席为预期、企业侧接口 + 部署入口注入对齐)。

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #1145 (octo-web)

Summary

This PR moves the Loop and Personal modules out of the open-source octo-web repository and behind an enterprise module slot. The entire packages/dmloop and packages/dmpersonal workspace packages are deleted (~29,600 lines removed), and replaced with a Vite virtual module (virtual:octo-enterprise-modules) that enterprise deployments can populate via the VITE_ENTERPRISE_MODULES_ENTRY environment variable.

The host app (apps/web) now has zero static imports of @octo/loop or @octo/personal. Module registration flows through registerEnterpriseModules() with a callback to WKApp.shared.registerModule. Full-page deep-links (formerly the CLI authorize path) are now dispatched generically via getEnterpriseStandaloneHandlers(), so adding a new enterprise standalone route no longer requires a host branch. An enterprise HTML head injection plugin (VITE_ENTERPRISE_HTML_HEAD_PATH) is also introduced for deployment-specific branding/scripts.

The refactoring is clean, well-tested, and the contract propagation is correct. Open-source builds degrade gracefully to no-op stubs when no enterprise entry is configured.

Verification

✅ No stale imports — grepped the entire repo for @octo/loop, @octo/personal, LoopModule, PersonalModule; zero remaining production imports (only a comment in packages/docs/types/octo-base.shim.d.ts:15 and the new guard tests).
✅ Deleted package cleanup — apps/web/package.json removes both @octo/loop and @octo/personal workspace dependencies; test:e2e:loop script removed.
✅ Auth/session lifecycle preserved — Layout/index.tsx:396-414: enterprise standalone handler branch preserves the full session recovery chain (WKApp.loginInfo.load() → recoverOctoSessionFromStorage(true) → space ID restoration) that the old CLI authorize path had. onSessionExpired callback wired to clearExpiredStandaloneSessionAndReload, matching the standalone doc pattern.
✅ Return-URL persistence correct — persistStandaloneReturn() gated by handler.persistReturnOnAnonymous, reusing the same function from @octo/docs. Consistent with the standalone doc deep-link pattern at :449.
✅ OSS fallback is no-op — createEnterpriseModulesVirtualModule([]) generates registerEnterpriseModules(_context) {} and getEnterpriseStandaloneHandlers() { return [] }. Open-source builds with no VITE_ENTERPRISE_MODULES_ENTRY will skip module registration and standalone handler matching entirely.
✅ Virtual module type contract aligned — enterprise-modules.d.ts declares registerEnterpriseModules and getEnterpriseStandaloneHandlers; the generated code in vite.enterpriseHtml.ts produces matching exports. Runtime and types agree.
✅ Test coverage adequate — enterpriseHtmlHead.test.ts covers HTML injection, no-op path, file reading, single/multi-entry virtual module, and filesystem allow-list. enterpriseModuleSlot.test.ts provides architectural guard tests preventing regression to static imports.
✅ Remote config flags preserved — dmloopOn/dmpersonalOn in App.tsx remain as remote config gate fields. Enterprise modules will register their nav items through registerModule, and the flags still control sidebar visibility. No functional regression.

Findings

No P0/P1 issues. One P2 and two nits below.

P2 — Missing `readEnterpriseHtmlHead` error handling for non-existent file (`apps/web/vite.enterpriseHtml.ts:10`)

readEnterpriseHtmlHead calls fs.readFileSync(resolved, "utf-8") without checking fs.existsSync first. When VITE_ENTERPRISE_HTML_HEAD_PATH points to a non-existent file, this throws an unhandled ENOENT during Vite config evaluation — the error message will be a raw Node.js stack trace rather than a helpful diagnostic. Compare with resolveEnterpriseEntry at :31 which does fs.existsSync and throws a clear [vite] prefixed error. Adding the same guard here would produce a consistent operator experience.

if (!fs.existsSync(resolved)) {
  throw new Error(`[vite] VITE_ENTERPRISE_HTML_HEAD_PATH does not exist: ${resolved}`);
}
return fs.readFileSync(resolved, "utf-8");

Nit — Missing semicolon (`apps/web/src/index.tsx:79`)

registerEnterpriseModules({...}) is missing a trailing semicolon. All surrounding registerModule calls use semicolons. Minor style inconsistency.

Nit — Stale comment reference to deleted file (`apps/web/e2e-kit/msw-handlers/chat-baseline.ts:12`)

Comment still references loop-empty.ts as a handler writing pattern example, but that file is deleted in this PR. The comment is in an unchanged file so it won't cause any build/test failure, but it's a stale pointer for future readers.

Things I checked that are fine

Vite plugin ordering — enterpriseModulesPlugin has enforce: "pre", ensuring virtual module resolution runs before other transforms. enterpriseHtmlHeadPlugin runs at default (post) stage for HTML transforms. Both correct.
server.fs.allow expansion — when enterprise entries point outside rootDir, the plugin correctly adds entry directories to Vite's filesystem allow list. No arbitrary filesystem exposure; paths must be explicitly configured via env vars.
Multiple enterprise entries — path.delimiter-separated entries are correctly split, resolved, and composed into the virtual module. Each entry's registerEnterpriseModules and getEnterpriseStandaloneHandlers are called independently.
Standalone handler dispatch — find() returns the first matching handler. If multiple enterprise modules register overlapping path patterns, the first wins. This matches the old hardcoded behavior (single path match).
pnpm-lock.yaml — consistent with removing two workspace packages and their unique transitive dependencies.
MswScenario type generalization — LoopScenario union replaced with string | "no-mock", decoupling the e2e-kit from loop-specific scenario names. Correct for a generic MSW harness.

Verdict: APPROVED

Clean, well-architected extraction of enterprise modules behind a build-time slot. The virtual module pattern is elegant — open-source builds get no-op stubs, enterprise deployments inject their modules via env-configured entry points. Auth lifecycle, return-URL persistence, and remote config gating are all correctly preserved. The only actionable item is a minor error-handling consistency improvement in readEnterpriseHtmlHead.

Static analysis only at head dbf470f047e38293afaf59bc6ae2011d06ef043f; build and tests not executed in this environment.

[Octo-Q] verdict: APPROVE — No P0/P1 issues. Clean enterprise module slot extraction with correct auth/session lifecycle preservation, proper OSS fallback to no-op stubs, and adequate test coverage. One P2 (error handling consistency) and two nits are non-blocking.

lml2468 · 2026-07-29T02:47:08Z

补充(head 校准):审查期间 head 从 dbf470f047e3 推进到 c334a246(=前者 + 合入 main:带入 #1111/#1142/#1141 等的 docs/chat/search 改动),我的正式 review 实际盖在 c334a246 上。已在 c334a246 复核:dmloop/dmpersonal 仍移除、@octo/loop|personal 悬空引用为 0、插槽 stub(getEnterpriseStandaloneHandlers(){return []})完好、pnpm --filter @octo/web build ✅ 3.79s——本 PR 的抽出逻辑在 c334a246 未变、APPROVE 对该 head 成立。上面 review 正文写的 dbf470 为审查起点,结论一并适用于 c334a246。其余 human-verify(与 #1120 插槽 API 协调、社区构建缺 loop/personal、企业侧接口/部署入口)不变。

Jerry-Xin

The PR is relevant to octo-web, but the new enterprise standalone return flow is currently broken for non-Docs routes.

🔴 Blocking

🔴 Critical: Enterprise anonymous deep-link return targets are persisted with the Docs-only return store, but the post-login consumer rejects them. apps/web/src/Layout/index.tsx calls persistStandaloneReturn() for matched enterprise handlers, and login later consumes that value through consumeStandaloneReturn(). However, the safety gate in packages/docs/src/pages/StandaloneDocPage.tsx only accepts /d/:docId and standalone summary paths. Any enterprise route such as /loop/cli-authorize will be cleared and ignored after SSO/OIDC login, so persistReturnOnAnonymous cannot work for the enterprise standalone handler contract. The host needs a generic return-store/validator for enterprise handlers, or the Docs consumer must be extended in a way that explicitly validates enterprise standalone paths.

💬 Non-blocking

🟡 Warning: enterpriseModulesPlugin narrows server.fs.allow to [rootDir, ...entryDirs, ...extraFsAllow] when an enterprise entry is configured. In this pnpm monorepo, the default Vite workspace root behavior is important for serving existing packages/* sources during dev. Unless every dev invocation supplies the repo root through VITE_ENTERPRISE_FS_ALLOW, enterprise dev mode can fail to serve host workspace packages. Consider including the workspace root by default or documenting/enforcing it. See apps/web/vite.enterpriseHtml.ts.

✅ Highlights

The open-source default virtual module is a no-op, so host builds no longer statically import @octo/loop or @octo/personal.
The package cleanup appears consistent: no remaining active static imports of the removed packages were found.

Superseded: re-posting with repository-relative paths.

Jerry-Xin

The PR is relevant to octo-web, but the new enterprise standalone return flow is currently broken for non-Docs routes.

🔴 Blocking

🔴 Critical: Enterprise anonymous deep-link return targets are persisted with the Docs-only return store, but the post-login consumer rejects them. apps/web/src/Layout/index.tsx:414 calls persistStandaloneReturn() for a matched enterprise handler (when persistReturnOnAnonymous is set), and the post-login flow later consumes that value through consumeStandaloneReturn(). However, the safety gate isSafeReturnPath() in packages/docs/src/pages/StandaloneDocPage.tsx:109 only accepts /d/:docId (via parseStandaloneDocId) and the standalone summary path (/s/:taskNo). Any enterprise route such as /loop/cli-authorize therefore fails validation and is cleared/ignored after SSO/OIDC login, so persistReturnOnAnonymous silently never works for the enterprise standalone handler contract. The host needs a generic return-store/validator for enterprise handlers, or the Docs consumer must be extended to explicitly validate enterprise standalone paths.

💬 Non-blocking

🟡 Warning: enterpriseModulesPlugin narrows server.fs.allow to [rootDir, ...entryDirs, ...extraFsAllow] when an enterprise entry is configured (apps/web/vite.enterpriseHtml.ts:104). In this pnpm monorepo, default Vite workspace-root behavior matters for serving existing packages/* sources during dev. Unless every dev invocation supplies the repo root through VITE_ENTERPRISE_FS_ALLOW, enterprise dev mode can fail to serve host workspace packages. Consider including the workspace root by default, or documenting/enforcing it.
🔵 Suggestion: the slot tests (apps/web/src/__tests__/enterpriseModuleSlot.test.ts) mostly assert source text. A behavioral test that executes the generated virtual module and exercises one Layout standalone-handler path would better protect the contract.

✅ Highlights

The open-source default virtual module is a no-op, so host builds no longer statically import @octo/loop or @octo/personal.
Package cleanup is consistent: no remaining active static imports of the removed packages were found; apps/web/package.json drops the @octo/loop / @octo/personal workspace deps.
apps/web/src/Layout/index.tsx:391 generalizes the previous Loop-only full-page route branch into a host-owned lifecycle hook — the right boundary for private modules.

yujiawei

Code Review — PR #1145 (octo-web)

Reviewed head SHA dbf470f047e38293afaf59bc6ae2011d06ef043f against merge-base 343f904c. 239 files, +351/−29978 — 226 of them are pure deletions (packages/dmloop, packages/dmpersonal, Loop host e2e assets). I read the complete non-deletion diff (13 files) line by line and ran the checks listed in §5.

The direction is right and the host-side hygiene is genuinely good: no dangling @octo/loop / @octo/personal references anywhere, i18n:check passes, the OSS build passes, dmloopOn / dmpersonalOn stay fail-safe false with no host code left rendering a dead entry, and WebhookIssuePreviewPanel already degrades to its unavailable state when nothing registers chatWebhookIssuePreview. Three issues in the new Vite plugin need fixing first.

1. Scope compliance

Spec: ❌

Stated scope: (a) remove the open-source Loop and Personal modules from the host, (b) add a Vite enterprise module slot for private module registration and standalone handlers, (c) remove Loop-specific host e2e assets and keep Docs host-owned.

Incomplete — (c): two visual baselines outlive the specs they belong to. Both spec files are deleted in this diff, so these baselines are now unreachable:

apps/web/e2e-kit/screenshots/chromium/C1-loop-empty-workspace.spec.ts/loop-empty-workspace.png
apps/web/e2e-kit/screenshots/chromium/C3-loop-workspace-default-view.spec.ts/loop-workspace-default-view.png

Incomplete — new build inputs are undocumented. This PR introduces three env vars — VITE_ENTERPRISE_MODULES_ENTRY, VITE_ENTERPRISE_HTML_HEAD_PATH, VITE_ENTERPRISE_FS_ALLOW — and none appear in apps/web/.env.example, nor do they get ARG/ENV plumbing in the root Dockerfile. Every other VITE_* build input in this repo follows that convention (Dockerfile:6-21). As written, an image-based build has no way to pass the enterprise entry, which is the exact integration path the PR notes describe.

Beyond stated scope: readEnterpriseHtmlHead + enterpriseHtmlHeadPlugin (apps/web/vite.enterpriseHtml.ts:8-22) add a second, independent capability — build-time injection of an arbitrary raw HTML fragment into index.html's <head>. None of the three summary bullets mention HTML head injection. It is a security-adjacent surface (arbitrary script into the app shell), it is unrelated to module registration, and it carries its own defect (P1-2). Please declare it explicitly in the PR description or split it into its own PR.

Divergence: none found. The rewritten branch that replaced isLoopCliAuthorizePath preserves the original semantics — loginInfo.load(), recoverOctoSessionFromStorage(true), cached currentSpaceId, and fall-through to the login screen when anonymous. persistStandaloneReturn is opt-in behind persistReturnOnAnonymous, so the previous Loop behavior (no return stash) remains the default. Good.

2. Code quality

Quality: Changes-Requested

P1-1 — `config()` clobbers Vite's default `fs.allow`, so the enterprise dev server 403s on every workspace-sibling import

apps/web/vite.enterpriseHtml.ts:104-112

config() {
  if (entryDirs.length === 0 && extraFsAllow.length === 0) return undefined;
  const allow = Array.from(new Set([rootDir, ...entryDirs, ...extraFsAllow]));
  return { server: { fs: { allow } } };
},

Vite only applies its default [searchForWorkspaceRoot(root)] when server.fs.allow is unset. Returning an explicit array replaces that default. rootDir here is process.cwd() — i.e. apps/web, not the monorepo root — so every sibling workspace package (@octo/base → packages/dmworkbase, @octo/docs, @octo/login, …) falls outside the allow list.

Resolved config, same vite.config.ts, only the env var differs:

[OSS default]           fs.allow = [ <repo root> ]
[enterprise entry set]  fs.allow = [ <repo root>/apps/web, /tmp/fake-ent/src, .../vite/dist/client ]

Live A/B against a real dev server, single request for packages/dmworkbase/src/Service/Module.ts via /@fs/:

[CONTROL: no enterprise env]  HTTP=200   (transformed module served)
[ENTERPRISE entry set]        HTTP=403   (Vite restricted-file page)

So pnpm dev with the enterprise entry configured cannot load the app at all. This was masked in the PR's own verification two ways: only pnpm build was exercised (server.fs.allow is inert during build), and the VITE_ENTERPRISE_FS_ALLOW value used there happens to include the repo root, which is precisely the manual workaround.

Fix: seed the list with the workspace root rather than the app dir.

import { searchForWorkspaceRoot, type Plugin } from "vite";
const allow = Array.from(new Set([
  searchForWorkspaceRoot(rootDir), ...entryDirs, ...extraFsAllow,
]));

P1-2 — `$&`, $`, `$'`, `$1` in the head fragment are interpreted as replacement patterns and corrupt the emitted `index.html`

apps/web/vite.enterpriseHtml.ts:20

return html.replace(/<\/head>/i, `${trimmed}\n  </head>`);

The second argument to String.prototype.replace is a replacement pattern, not a literal. $&, $`, $' and $n in trimmed get substituted. Since the fragment is read verbatim from an operator-supplied file, any inline analytics/monitoring snippet containing a regex or a $-sequence is silently rewritten.

Reproduced in a real vite build. Input fragment:

<script>window.__ENT__ = "a$&b$'c"</script>

build/index.html:93:

  <script>window.__ENT__ = "a</head>b

$& expanded to the matched </head>, which closes <head> early and leaves an unterminated <script> — a structurally malformed document shell, emitted with no warning. The same run confirms $` interpolates the entire preceding document and $' the entire following document.

Fix — use a replacer function so the fragment is treated literally:

return html.replace(/<\/head>/i, () => `${trimmed}\n  </head>`);

Better still, return Vite's structured form ({ html, tags: [...] } with injectTo: "head"), which also removes the dependency on a literal </head> being present in the template.

P1-3 — a misconfigured enterprise entry silently yields a build with zero enterprise modules

apps/web/vite.enterpriseHtml.ts:58-68

"function callEnterpriseExport(entry, exportName, args = []) {",
"  const fn = entry[exportName];",
"  return typeof fn === \"function\" ? fn(...args) : null;",
"}",

A configured entry whose export is renamed, misspelled, default-only, or async is silently ignored. Verified: an entry exporting registerEnterpriseModulez (one-character typo) builds clean —

✓ built in 15.19s     → build/index.html produced, zero enterprise modules registered

This inverts the failure mode the slot replaces. Before this PR, import { LoopModule } from '@octo/loop' failed the build loudly if the export moved. Now the private CI can ship a production bundle with Loop and Personal missing entirely and every gate green — and the OSS no-op path is indistinguishable from the misconfiguration.

Fix: the empty-entry case is the legitimate no-op; a configured entry exporting neither hook is not. Throw (or at minimum console.warn loudly) in appendEnterpriseHandlers / the register path when a configured entry contributes nothing. The contract is duplicated by hand across two repos with no compile-time link, so this warning is the only thing standing between a rename and a silent feature loss.

P2 findings

P2-1 — persistReturnOnAnonymous also fires on the authenticated path. apps/web/src/Layout/index.tsx:405-415: when a token exists but handler.render() returns null, control falls out of the if (WKApp.loginInfo.token) block and reaches if (enterpriseStandaloneHandler.persistReturnOnAnonymous) persistStandaloneReturn(). Every sibling branch (/d/:docId at :449, /s/share/:shareId at :463, /s/:taskNo at :481) unconditionally returns inside its token branch, so this is the only one that can persist a return target for a signed-in user. Because this sits in render(), a re-rendering authenticated user re-writes the stale target repeatedly. Either move the call inside an explicit else, or rename the flag to match what it does.

P2-2 — generic host capability sourced from a product module. apps/web/src/Layout/index.tsx:16 imports persistStandaloneReturn from @octo/docs and hands it to the enterprise slot. The extensibility mechanism whose whole purpose is decoupling now depends on the Docs module for generic post-login return persistence. Consider hoisting it to @octo/base.

P2-3 — repo-relative entry paths without ./ are misread as package specifiers. apps/web/vite.enterpriseHtml.ts:26: if (!entryPath.startsWith(".") && !path.isAbsolute(entryPath)) return entryPath;. VITE_ENTERPRISE_MODULES_ENTRY=enterprise/loop.ts becomes a bare specifier and fails resolution with a confusing message, while readEnterpriseHtmlHead (:10) resolves any relative path against rootDir. Two adjacent functions, two different rules.

P2-4 — asymmetric validation. resolveEnterpriseEntry (:28-30) raises a clear does not exist error; readEnterpriseHtmlHead (:11) calls fs.readFileSync unguarded, so a bad VITE_ENTERPRISE_HTML_HEAD_PATH surfaces as a raw ENOENT thrown from inside defineConfig.

P2-5 — declared-but-unused id, and implicit first-match-wins routing. apps/web/src/enterprise-modules.d.ts:16 declares id: string on EnterpriseStandaloneHandler; nothing in the host reads it. Meanwhile handlers.find(...) (Layout/index.tsx:396-398) resolves conflicts by the order entries appear in the env var, with no duplicate or overlap detection — a broad match in one entry can silently shadow a more specific route in another. Either use id for a duplicate check or drop it from the contract.

P2-6 — head fragment is not a watched dependency. It is read once during config evaluation (vite.config.ts:14-17), never registered via addWatchFile or a dev watcher, so edits are invisible until the Vite process restarts.

P2-7 — VITE_ENTERPRISE_FS_ALLOW is dev-only, and the dev server listens on all interfaces. It feeds server.fs.allow, which has no effect on vite build — yet the PR's verification passes it to a build. Worth documenting. Note also that server.host defaults to true (vite.config.ts:143), so a careless VITE_ENTERPRISE_FS_ALLOW=/ or =$HOME makes those paths LAN-readable through /@fs/. The allow-widening is the var's declared purpose and fs.deny still guards .env/keys, so this is documentation rather than a defect — but the note belongs next to the var.

P2-8 — the new tests don't guard the new behavior, and CI never runs them. enterpriseModuleSlot.test.ts is entirely source-text grep; enterpriseHtmlHead.test.ts asserts against generated strings rather than executing the virtual module. Neither covers registration, handler aggregation, the authenticated/anonymous fall-through, or real Vite resolution — all three P1s above sit in exactly those gaps and all three passed the suite. Separately, the deleted loopCliAuthorizeDeepLink.test.ts asserted a host invariant the replacement drops: that the full-page branch is positioned before return <Provider. Worth re-establishing against getEnterpriseStandaloneHandlers. Finally, ci.yml runs only @dmwork/skillmarket test, a fixed @octo/docs subset, build, and lint — no job runs apps/web unit tests, so both new files are inert in CI.

P2-9 — stale comment. packages/docs/types/octo-base.shim.d.ts:15 still cites "packages like @octo/loop" as an example; that package no longer exists in this repo.

3. Overall verdict

REQUEST_CHANGES — three P1s (P1-1 blocks enterprise pnpm dev outright; P1-2 silently emits a malformed index.html; P1-3 converts a build-time failure into a silent production feature loss), plus the scope items in §1.

4. Suggested changes

searchForWorkspaceRoot(rootDir) instead of rootDir in the fs.allow seed, and add a dev-server smoke check to the PR's verification list so build-only testing can't hide it again.
Replacer function (or Vite's tags form) for the head injection.
Fail loudly when a configured entry contributes no hooks; keep the silent no-op only for the unconfigured OSS path.
Document VITE_ENTERPRISE_MODULES_ENTRY / VITE_ENTERPRISE_HTML_HEAD_PATH / VITE_ENTERPRISE_FS_ALLOW in apps/web/.env.example, and add ARG/ENV to the root Dockerfile if image-based enterprise builds are intended.
Delete the two orphan baseline screenshot directories.
Either declare the HTML-head-injection capability in the PR description or split it out.
Align entry-path and head-path resolution rules, and give readEnterpriseHtmlHead the same existence check resolveEnterpriseEntry has.

5. What I verified / not covered

Ran locally at this head SHA: pnpm install --frozen-lockfile; pnpm i18n:check (pass); vitest run enterpriseHtmlHead.test.ts enterpriseModuleSlot.test.ts (9/9 pass); pnpm --dir apps/web build with no enterprise env (pass); the same build with a synthetic enterprise entry + head fragment (pass, and reproduced P1-2 in the emitted index.html); a build with a deliberately typo'd export (reproduced P1-3); resolveConfig comparison of server.fs.allow in both modes; and a live A/B dev-server /@fs/ fetch of a workspace-sibling module (reproduced P1-1). Also swept the tree for dangling @octo/loop / @octo/personal / dmloop / dmpersonal / isLoopCliAuthorizePath references (none), checked pnpm-lock.yaml contains only the two packages' removals with no unrelated version drift, and confirmed WebhookIssuePreviewPanel renders its unavailable fallback with no endpoint provider.

Not covered: the private enterprise entry module itself is not in this repo, so behavior parity for the CLI-authorize page is unverified — in particular the query-parameter preservation across the ?sid= rewrite that the deleted resolveLoopCliAuthorizeSearch guaranteed, which nothing in this repo now covers. Also unverified: internal CI/deployment wiring for the entry env var, the Playwright e2e suite (not executed), Electron/Tauri packaging paths, and runtime NavRail behavior under dmloop_on / dmpersonal_on, which is not observable without the private module. The 226 pure-deletion files were reviewed by reference sweep rather than line by line.

yujiawei · 2026-07-29T03:04:34Z

Addendum to my review above — two more nits, so the fix pass covers everything in one round. Verdict is unchanged (the three P1s stand).

Stale pointer — apps/web/e2e-kit/msw-handlers/chat-baseline.ts:12 still cites loop-empty.ts as the reference handler-writing example ("参考 loop-empty.ts 现有 handler 写法"), but this PR deletes that file. msw-handlers/ now contains only chat-baseline.ts, mcp-official.ts, and README.md, so the pointer dead-ends for the next reader. Harmless to build/test; worth repointing to mcp-official.ts or the README.
Missing semicolon — apps/web/src/index.tsx:79-81: registerEnterpriseModules({ ... }) has no trailing semicolon, while all ten surrounding WKApp.shared.registerModule(...) calls do.

For completeness on one point worth being explicit about, since it is easy to check statically and conclude the opposite: the server.fs.allow handling in enterpriseModulesPlugin.config() reads as a safe widening of the allow list, but it is a replacement. Vite applies its [searchForWorkspaceRoot(root)] default only when server.fs.allow is unset, so returning an explicit array drops the monorepo root — and the array is seeded with process.cwd() (apps/web), not the repo root. Reproduced twice on a live dev server, same URL, only the env var differing:

CONTROL    (no enterprise env)   /@fs/.../packages/dmworkbase/src/App.tsx -> HTTP=200
ENTERPRISE (entry configured)    /@fs/.../packages/dmworkbase/src/App.tsx -> HTTP=403

@octo/base is imported at src/index.tsx:7 and resolves into packages/dmworkbase, so pnpm dev with the enterprise entry set cannot load the app. vite build is unaffected, which is why a build-only verification pass shows nothing.

lml2468

Re-review: #1145 — feat: move loop and personal behind enterprise slot

新提交 0f39700f0857(在我上轮 APPROVE @ c334a246 之上)—— 修复了 Steve/Jerry-Xin 提的 enterprise 登录回跳 🔴,并处理 yujiawei 的多条 🔴/🟡。锚定 head 0f39700f0857(rev-parse 确认 checkout==live head)/ mergeable=true。

结论:APPROVE ✅(先认领我上轮的漏判)

先认领:我上轮 @ `c334a246` 的漏判

Steve/Jerry-Xin 的 🔴 属实、我漏了:泛化后的 handler 若声明 persistReturnOnAnonymous,匿名访问会 persistStandaloneReturn() 存回跳路径;但登录侧 isSafeReturnPath() 当时只放行 /d/+/s/ —— 任何 enterprise 路径(如 /loop/cli-authorize)登录后被静默丢弃,persistReturnOnAnonymous 契约形同虚设。我在 #1120 审过这个 isSafeReturnPath,本应联想到泛化后需同步放宽白名单,却没抓到。感谢 Steve/Jerry-Xin。

增量验证(`c334a246..0f39700f`)—— 🔴 已修

登录回跳白名单纳入 enterprise handler(且保留开放重定向守卫):standaloneReturn.ts 的 isSafeReturnPath(path, handlers) —— 仍先做 path[0]==='/' + 控制字符拒绝 + new URL + url.origin !== origin 同源校验,再放行 /d/、/s/、summary-share,或 handlers.some(h => h.persistReturnOnAnonymous && h.match(pathname))(:32)。enterprise 路径仅在同源 + 被已注册且 opt-in 的 handler match 时才接受,不可注入任意路径。
host 消费方接线正确:Layout/index.tsx:183 consumeStandaloneReturn(getEnterpriseStandaloneHandlers()) —— 登录回跳走 host 的 handler-aware 消费方并传入 enterprise handlers。回跳契约不再是死的。新增 standaloneReturn.test(+71)覆盖 enterprise 回跳。

yujiawei 的 🔴/🟡 — 已处理

VITE_ 环境变量声明*:apps/web/.env.example 补 VITE_ENTERPRISE_MODULES_ENTRY/_HTML_HEAD_PATH/_FS_ALLOW(注释示例 + 空默认);Dockerfile 补 ARG+ENV(MODULES_ENTRY / HTML_HEAD_PATH)。
残留视觉基线删除:loop-empty-workspace.png/loop-workspace-default-view.png 已删(loop 移除后 e2e 基线失效)。
fs.allow 合默认:vite.enterpriseHtml.ts:118 改用 searchForWorkspaceRoot(rootDir) 并入 allow —— 不再因覆盖默认 fs.allow 而挡开发访问工作区包。
enterpriseHtmlHead 声明:.env.example 记录 + enterpriseHtmlHead.test 扩充(+86);为构建期 operator 控制的 head 注入,现已文档化。

搬迁本体保持

dmloop/dmpersonal 仍移除、无悬空 loop/personal import、slot 默认 no-op stub 不变(本次未触碰抽出逻辑)。

构建 / 测试(实跑 @ 0f39700f0857):pnpm --filter @octo/web build ✅ 3.94s;standaloneReturn.test + enterpriseModuleSlot.test + enterpriseHtmlHead.test 3 文件 17/17 全绿(含 enterprise 回跳同源 + handler 匹配用例)。

🟡 非阻断(沿用)

与 #1120 的插槽 API 冲突/合并协调仍需处理(本 PR 泛化为 getEnterpriseStandaloneHandlers,#1120 为 docs 专用 capability)——合并顺序需统一到 handler 模型。
社区构建缺 loop/personal 为预期;企业侧模块须实现 registerEnterpriseModules+getEnterpriseStandaloneHandlers(跨仓)。

Steve/Jerry-Xin 的 enterprise 登录回跳 🔴(我上轮漏判)已修:isSafeReturnPath 纳入 handler 白名单且保留同源/控制字符守卫、host 消费方传入 handlers;yujiawei 的 env 声明/基线删除/fs.allow 合默认/head 文档化均处理;搬迁本体不变,构建 + 17 测试通过。从上轮(已 dismiss)的 APPROVE 修正为基于新 commit 的 APPROVE ✅(仍建议先协调与 #1120 的插槽 API)。

Jerry-Xin

Summary: The PR is relevant to octo-web and the enterprise slot approach is scoped cleanly: OSS builds get a no-op virtual module, host registration no longer imports closed Loop/Personal packages, and standalone enterprise deep links reuse the existing safe return-path lifecycle.

💬 Non-blocking

🟡 Warning: .github/workflows/e2e-baseline-update.yml:126 and .github/workflows/e2e-baseline-update.yml:135 still run Playwright with --grep "@visual" even though this PR sets expected visual coverage to zero and removes the remaining visual specs. Manual baseline-update runs may now fail with “no tests found.” Same concern applies to .github/workflows/e2e-baseline-bootstrap.yml:65 and .github/workflows/e2e-baseline-bootstrap.yml:74. Consider skipping these steps when EXPECTED_VISUAL=0 or removing the obsolete workflows.

✅ Highlights

The virtual module implementation in apps/web/vite.enterpriseHtml.ts:53 provides a no-op OSS default and composes private entries only when configured, which keeps the public build dependency graph clean.

The standalone return validation in apps/web/src/Layout/standaloneReturn.ts:13 preserves same-origin and control-character protections while allowing enterprise-owned persistent handlers.

The host registration path in apps/web/src/index.tsx:79 keeps Docs host-owned and moves private modules behind a single registration hook.

Verification note: I attempted the focused Vitest run and pnpm --dir apps/web build, but this checkout has no node_modules, so both stopped before execution because vitest and vite were not installed.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #1145 (octo-web)

Summary

This PR extracts the closed-source @octo/loop and @octo/personal packages from the OSS repository, replacing their static imports with a Vite virtual module (virtual:octo-enterprise-modules) that resolves to enterprise-specific implementations at build time. The architecture is clean: a no-op virtual module ships for OSS builds, while enterprise deployments supply real implementations via VITE_ENTERPRISE_MODULES_ENTRY. The EnterpriseStandaloneHandler interface generalizes the previously hardcoded Loop CLI authorize deep-link into a handler-based system, and standalone return-path persistence moves into a host-owned standaloneReturn.ts module. Roughly 29,000 lines of deletions (packages + e2e specs + case specs + reports + baselines) against ~600 lines of new plumbing. No P0/P1 issues found.

Verification

Static analysis only at head 0f39700f; build and tests not executed in this environment.

✅ No dangling imports — grep across the entire codebase for @octo/loop and @octo/personal finds only comment references and the architectural test asserting their absence. No production import remains.
✅ Virtual module correctness — enterpriseModulesPlugin with no entry produces a no-op (registerEnterpriseModules(_context) {} and getEnterpriseStandaloneHandlers() { return [] }). With entries, it generates indexed imports and composes handlers via handlers.push(...).
✅ Standalone return-path safety — isSafeReturnPath in standaloneReturn.ts validates: (1) string type and non-empty, (2) starts with /, (3) no control characters, (4) URL origin matches window.location.origin, (5) matches known doc/summary paths OR an enterprise handler with persistReturnOnAnonymous: true. The origin check prevents open-redirect via crafted sessionStorage values.
✅ HTML injection safety — enterpriseHtmlHeadPlugin uses a function replacer (() => \${trimmed}\n `) rather than a string replacer, preventing $&/$'` substitution bugs if enterprise head HTML contains those sequences.
✅ Extension build isolation — wxt.config.ts registers enterpriseModulesPlugin(undefined, ...) — explicitly undefined entry means the virtual module resolves to the no-op variant. Extension builds are unaffected.
✅ Docker build args — Dockerfile correctly adds ARG/ENV for all three enterprise env vars before the pnpm install && build step.
✅ CI test-count alignment — EXPECTED_P0 reduced from 29 to 1 (matching the 28 deleted loop spec files), EXPECTED_VISUAL from 2 to 0 (matching deleted baseline images).

Findings

No P0/P1 issues. Two P2 non-blocking items.

P2 — HTML head injection regex may split on embedded `</head>` (`apps/web/vite.enterpriseHtml.ts:19`)

enterpriseHtmlHeadPlugin replaces the first </head> match (case-insensitive). If the enterprise head fragment itself contains a </head> literal inside a script string or comment, the replacement splits the fragment mid-content, producing malformed HTML. Practical risk is low since enterprise authors control the head file and are unlikely to embed that tag, but validating the fragment does not contain a closing head tag (or using DOMParser) would close the edge case entirely.

P2 — Orphaned `dmloopOn` / `dmpersonalOn` feature flags (`packages/dmworkbase/src/App.tsx:291`)

dmloopOn and dmpersonalOn remain in WKRemoteConfig as parsed fields with change-listener tracking, but no code in this repo reads them after the module removal. If enterprise modules in the private repo still consume these flags from WKApp.remoteConfig, the fields should stay — but a brief comment noting the consumer lives in the enterprise module would prevent future cleanup attempts from removing them. The JSDoc was partially updated (to say "企业" instead of package names) but does not identify the downstream consumer.

Things I checked that are fine

consumeStandaloneReturn at Layout/index.tsx:183 now passes getEnterpriseStandaloneHandlers() to the host-owned function, so enterprise paths beyond /d/... and /s/... are correctly gated on handler claim.
The enterprise standalone handler branch (Layout/index.tsx:399-414) correctly mirrors the old Loop CLI authorize logic (session recovery, space-id restoration, persistReturnOnAnonymous for unauthenticated users) in a generic handler-based form.
resolveEnterpriseEntry handles both npm packages (@scope/pkg) and file paths, with proper existsSync validation for file paths.
parseEnterpriseFsAllow correctly splits on path.delimiter (platform-aware), and enterpriseModulesPlugin.config() includes searchForWorkspaceRoot(rootDir) as a baseline allow entry.
The architectural test (enterpriseModuleSlot.test.ts) asserts that index.tsx imports @octo/docs but not @octo/loop or @octo/personal, and that Layout/index.tsx uses getEnterpriseStandaloneHandlers. Good regression guard.
mswScenario.ts generalizes LoopScenario to MswScenario = string | "no-mock", and chat-baseline.ts adds MOCK_APP_CONFIG with dmloop_on: "0", dmpersonal_on: "0". Consistent with the module removal.
STANDALONE_RETURN_KEY deliberately keeps the octo.docs.standaloneReturn key name for backward compatibility with @octo/docs pages that wrote it before this refactor.

Verdict: APPROVED

Clean architectural extraction with no correctness, security, or build concerns. The two P2 items are documentation and edge-case hygiene — neither blocks merge.

[Octo-Q] verdict: APPROVE — No P0/P1 issues found. Two P2 non-blocking items (head-injection regex edge case, orphaned feature flags). Architecture is sound: virtual module correctly no-ops for OSS, standalone return paths are validated against origin and handler claims, extension builds are isolated.

yujiawei

Code Review — PR #1145 (octo-web)

Round 2, reviewed head SHA 0f39700f0857f4b1bf859c7b0a96091fc9bdb8ee against merge-base 32906690. 251 files, +613/−29988; 164 of the changed files are pure deletions of packages/dmloop / packages/dmpersonal and the Loop host e2e assets. I read the complete non-deletion diff (23 files) line by line and re-ran every check listed in §5 at this head.

All four blocking findings from round 1 are fixed, and I reproduced each fix live rather than reading it. Details in §2. Nothing P0/P1 remains, so this is an approve. Everything below the verdict is P2 — worth a follow-up pass, none of it a reason to hold the merge.

1. Scope compliance

Spec: ✅

Stated scope: (a) remove the open-source Loop and Personal modules from the host, (b) add a Vite enterprise module slot for private module registration and standalone handlers, (c) remove Loop-specific host e2e assets and keep Docs host-owned.

(a) complete. Zero remaining references to @octo/loop, @octo/personal, dmloop, dmpersonal, LoopModule, PersonalModule, isLoopCliAuthorizePath, or LOOP_CLI_AUTHORIZE_PATH anywhere in the tree outside the intentional dmloop_on / dmpersonal_on remote-config flags and the new guard tests. apps/web/package.json drops both workspace deps. pnpm-lock.yaml is effectively subtractive — I set-diffed the old and new lockfiles and the @douyinfe/semi-ui peer resolutions for every surviving importer are byte-identical, so there is no version drift hiding in the 293-line lockfile delta.
(b) complete, and the round-1 documentation gap is closed: all three new env vars are now in apps/web/.env.example with the dev-only / LAN-exposure caveat spelled out, and they have ARG/ENV plumbing at Dockerfile:18-23 matching the convention the other VITE_* inputs follow.
(c) complete. Both orphan visual baselines are gone; apps/web/e2e-kit/screenshots/ is now empty. The test:e2e:loop script and the loopEmptyHandlers import are removed, and the */common/appconfig handler that lived in the deleted loop-empty.ts is faithfully re-homed into chat-baseline.ts:42-47 with dmloop_on correctly flipped from "1" to "0".

Missing / over-built: nothing that changes what ships. Two notes:

The PR description is now stale. The three summary bullets do not mention the enterprise HTML-head-injection capability (vite.enterpriseHtml.ts:8-26), the extension plugin wiring, the e2e gate threshold changes, or the new host-owned return-target module. All of these are either declared scope or a necessary consequence of it — the extension wiring, for instance, is what keeps the root pnpm build green — so I am not treating this as scope bloat. But the Verification block still lists only the round-1 commands, and it notably still omits the dev-server check that would have caught the fs.allow regression. Please refresh both before merge.
One genuinely unstated behavior change, see P2-3: the post-login return-target allowlist silently gained /s/share/:shareId.

Divergence: none. The generalized enterprise standalone branch preserves the original Loop semantics exactly — loginInfo.load(), recoverOctoSessionFromStorage(true), cached currentSpaceId, onSessionExpired wiring, and fall-through to the login screen when anonymous.

2. Code quality

Quality: Approved

Round-1 blockers — verified fixed

P1-1 (fs.allow clobbered Vite's default workspace root) — fixed and reproduced. vite.enterpriseHtml.ts:118 now seeds the list with searchForWorkspaceRoot(rootDir). Resolved config A/B, same vite.config.ts, only the env var differing:

[OSS default]           fs.allow = [ <repo root> ]
[enterprise entry set]  fs.allow = [ <repo root>, /tmp/fake-ent/src ]

And against a live dev server, the exact request that returned 403 in round 1:

[ENTERPRISE entry set]  /@fs/.../packages/dmworkbase/src/App.tsx  -> HTTP=200
[ENTERPRISE entry set]  /@fs/tmp/fake-ent/src/loop.ts             -> HTTP=200

Workspace siblings and the out-of-tree entry are both reachable now.

P1-2 ($& / $` / $' / $n corrupting index.html) — fixed and reproduced. vite.enterpriseHtml.ts:23 now passes a replacer function. Real vite build with the adversarial fragment <script>window.__ENT__ = "a$&b$'c$1d"</script> emits it verbatim at build/index.html:93, where round 1 produced a truncated <head> and an unterminated <script>.

P1-3 (misconfigured entry silently yielding zero enterprise modules) — fixed and reproduced. vite.enterpriseHtml.ts:61-68 replaced the typeof fn === "function" soft probe with static named imports. Both misconfiguration shapes now fail loudly at build time instead of shipping a bundle with Loop missing:

entry exports registerEnterpriseModulez (1-char typo)
  → BUILD_EXIT=1  [MISSING_EXPORT] "registerEnterpriseModules" is not exported by ".../loop.ts"

entry exports only registerEnterpriseModules
  → BUILD_EXIT=1  [MISSING_EXPORT] "getEnterpriseStandaloneHandlers" is not exported by ".../loop.ts"

This makes both exports mandatory for every configured entry — a deliberate tightening, correctly documented in .env.example. The OSS no-op path is untouched and still builds clean.

Cross-reviewer blocker (enterprise return targets written to a store whose validator rejected them) — fixed. apps/web/src/Layout/standaloneReturn.ts is a new host-owned implementation whose isSafeReturnPath takes the handler list and accepts a path when a persistReturnOnAnonymous handler owns it (:32), while keeping all three original gates — rooted path, C0/DEL control-character rejection, same-origin URL resolution. Layout/index.tsx:183 threads getEnterpriseStandaloneHandlers() into the consumer. The 11 assertions in standaloneReturn.test.ts cover the accept case, the reject-when-flag-absent case, and off-origin / control-char / relative tampering; all pass.

Round-1 P2-1 also fixed correctly: Layout/index.tsx:414 is now an else if, so a signed-in user whose handler.render() returns null no longer repeatedly rewrites a stale return target on every render.

P2 findings

P2-1 — e2e-baseline-update.yml can no longer succeed: its two @visual steps now match zero tests. This PR deletes the last two @visual cases, and .github/workflows/e2e-baseline-update.yml:127 and :136 both run playwright test --grep "@visual" under set -euo pipefail. Playwright fails rather than no-ops on an empty selection:

$ playwright test --config=e2e-kit/playwright.ci.config.ts --grep "@visual" --list
Error: No tests found
Total: 0 tests in 0 files
exit 1

Not a merge blocker — the workflow is workflow_dispatch-only, and with no visual cases left nobody has a reason to trigger it. But the file was already edited in this PR (EXPECTED_P0 29→1), so finishing the job belongs here: either add --pass-with-no-tests, or retire the workflow until a visual case exists again.

P2-2 — the e2e coverage guard is now a bare total, so a future tag swap can pass it with zero blocking cases. EXPECTED_P0=1 / EXPECTED_VISUAL=0 at .github/workflows/e2e.yml:128-129 is arithmetically correct at this head — I confirmed --grep "@p0" --list reports exactly Total: 1 test in 1 file (C989), and nothing carries @visual. The residual weakness is structural: :134 compares only TOTAL_TESTS against the sum, so if someone later adds a @visual case and C989 silently loses its @p0 tag, the total stays at 1 and the gate passes having run no blocking case at all. Counting the two categories separately would restore the intent. Worth stating plainly either way: the blocking e2e gate on this repo is now a single test.

P2-3 — /s/share/:shareId was silently added to the post-login open-redirect allowlist. standaloneReturn.ts:9,31 introduces STANDALONE_SUMMARY_SHARE_PATH, which the implementation it replaces did not accept:

path: /s/share/share_abc
  previous validator (doc || summary)          -> false
  new validator     (doc || summary || share)  -> true

Layout/index.tsx:465 does call persistStandaloneReturn() on that branch, so this actually repairs a pre-existing bug where an anonymous visitor to a shared summary landed on the app root after signing in. I am not asking you to revert it — but it is a widening of a gate that was deliberately hardened, and the new file dropped the multi-paragraph rationale comment that documented the three gates and why target-shape validation exists. Please carry that reasoning over and note this widening as intentional.

P2-4 — the return-target protocol is now implemented twice against one sessionStorage key. standaloneReturn.ts:5-9 re-declares octo.docs.standaloneReturn and hardcodes the /d/:docId and /s/:taskNo regexes, while packages/docs/src/pages/StandaloneDocPage.tsx:24,29,33 keeps its own copies and :436 still writes the key. Layout/index.tsx:16 already imports parseStandaloneDocId and isStandaloneDocPath from @octo/docs, so the duplication is avoidable: if the doc-id charset ever changes, the host validator drifts and post-login return breaks silently for the new format. Consider having the host validator delegate to the exported predicates, and note that consumeStandaloneReturn is now dead in production yet still exported at packages/docs/src/index.ts:9.

P2-5 — export type MswScenario = string | "no-mock" collapses to string. apps/web/e2e-kit/_lib/mswScenario.ts:19. The union with a string literal is absorbed, so all compile-time checking of scenario names is gone: installMswScenario(page, "defualt") now typechecks and fails at runtime. (string & {}) | "no-mock" keeps the autocomplete, or just enumerate the surviving scenarios.

P2-6 — the extension typecheck gains two unresolvable-module errors. apps/extension/entrypoints/sidepanel/main.tsx:18 imports ../../../web/src/App, which reaches Layout, but apps/web/src/enterprise-modules.d.ts is outside the extension's tsconfig scope:

../web/src/Layout/index.tsx(17,49):        error TS2307: Cannot find module 'virtual:octo-enterprise-modules'
../web/src/Layout/standaloneReturn.ts(1,50): error TS2307: Cannot find module 'virtual:octo-enterprise-modules'

Low impact today — the extension's compile script already reports 3745 errors, it is not wired into turbo.json or any workflow, and wxt build is green (I ran it). But this is cheap to fix now by moving the declaration to a shared types entry or adding it to the extension's include.

P2-7 — the extension hardcodes an entry of undefined, ignoring the env var, with no comment saying why. apps/extension/wxt.config.ts:18 passes enterpriseModulesPlugin(undefined, ...), so an enterprise extension build always gets the no-op stub while apps/web reads VITE_ENTERPRISE_MODULES_ENTRY. That asymmetry looks deliberate — a side panel plausibly should not carry Loop — but the next reader cannot tell intent from accident. One line of comment settles it.

P2-8 — the Docker plumbing is necessary but not sufficient. Dockerfile:18-23 adds the ARGs, but the builder stage is a plain COPY . . (:4), so the sibling paths .env.example suggests (../../octo-enterprise-modules/src/index.ts) are outside the build context. resolveEnterpriseEntry fails fast with a clear does not exist, so this is a documentation gap rather than a silent trap — but if image-based enterprise builds are the intended path, say how the private source gets into the context.

P2-9 — resolveEnterpriseEntry's new heuristic regresses one narrow case. vite.enterpriseHtml.ts:29-35 now treats a value as a bare specifier only when it starts with @ or contains no separator. That correctly fixes round-1's repo-relative enterprise/loop.ts case, but an unscoped package subpath such as mypkg/entry is now filesystem-resolved and dies with does not exist where it previously passed through to the resolver. Unlikely in practice; worth a comment pinning the intended input shapes.

P2-10 — round-1 items still open. enterprise-modules.d.ts:16 still declares id that nothing reads, and Layout/index.tsx:396-398 still resolves overlapping routes by handlers.find(...), i.e. by env-var order, with no duplicate or overlap detection — a broad match in one entry silently shadows a more specific route in another, including the /d and /s branches that run after it. The head fragment is still read once during config evaluation with no addWatchFile, so edits need a Vite restart. And packages/docs/types/octo-base.shim.d.ts:15 still cites "packages like @octo/loop" as an example.

P2-11 — the new tests are real behavioral tests now, but CI still never runs them. Credit where due: standaloneReturn.test.ts executes the module, and enterpriseHtmlHead.test.ts now drives transformIndexHtml and plugin.config() instead of only grepping generated strings — that is exactly the right direction, and the round-1 gaps that let three P1s through are the ones these tests now cover. But ci.yml still runs only @dmwork/skillmarket test, an @octo/docs subset, build, and lint; no job runs apps/web unit tests. So the open-redirect allowlist guarding post-login navigation is protected by tests that never execute in CI. Adding apps/web to the test job is the single highest-value follow-up here — though note it currently has 36 pre-existing failures across 26 files (password-strength thresholds, voice-input, source-text-matching invite assertions), all unrelated to this PR, which would need triage first.

3. Overall verdict

APPROVE — all four blocking findings are fixed, and I reproduced each fix rather than taking the diff's word for it: the enterprise dev server now serves workspace siblings (200, was 403), $-sequences in the head fragment survive the build verbatim, and both misconfigured-entry shapes now fail the build loudly. The OSS default path is unaffected and clean. The P2s above are follow-ups, not merge conditions.

4. Suggested changes

Update the PR description: add the HTML-head-injection capability, the e2e threshold changes, and the new return-target module, and refresh Verification to include the dev-server check and standaloneReturn.test.ts.
e2e-baseline-update.yml: --pass-with-no-tests or retire the workflow (P2-1). Count @p0 and @visual separately in e2e.yml (P2-2).
Carry the return-path security rationale into standaloneReturn.ts and call out the /s/share/:shareId widening as intentional (P2-3).
Delegate the host validator to @octo/docs's exported predicates instead of re-declaring the regexes; drop the now-dead consumeStandaloneReturn export (P2-4).
(string & {}) | "no-mock" for MswScenario (P2-5).
Bring virtual:octo-enterprise-modules into the extension's type scope, and comment why the extension pins the entry to undefined (P2-6, P2-7).
Document how private enterprise source reaches the Docker build context (P2-8).
Add apps/web to the CI test job so the new guards actually gate (P2-11).

5. What I verified / not covered

Ran locally at this head: pnpm install --frozen-lockfile; pnpm i18n:check (pass); vitest run on the four affected test files (28/28 pass); vite build with no enterprise env (pass); vite build with a synthetic entry + adversarial $-laden head fragment (pass, $-sequences verbatim in build/index.html:93); vite build with a typo'd export and with a handlers-export omitted (both correctly fail MISSING_EXPORT); resolveConfig A/B of server.fs.allow in both modes; a live dev-server A/B /@fs/ fetch of packages/dmworkbase/src/App.tsx and of the out-of-tree entry (200/200); wxt build for the extension (pass); tsc --noEmit for both apps/web and apps/extension (2776 and 3745 pre-existing errors respectively — unmaintained and unenforced, but I confirmed the only new ones are the two TS2307s in P2-6); playwright --list under --grep "@p0" and --grep "@visual" (1 and 0 tests); a set-diff of pnpm-lock.yaml confirming no peer-resolution drift for surviving importers; and a full sweep for dangling @octo/loop / @octo/personal / dmloop / dmpersonal / loop-empty / test:e2e:loop references (none).

Not covered: the private enterprise entry module is not in this repo, so the handler contract and behavior parity for the CLI-authorize page remain unverified — in particular the query-parameter preservation across the ?sid= rewrite that the deleted resolveLoopCliAuthorizeSearch guaranteed, which nothing in this repo replaces. Also unverified: the browser-level login round trip for an enterprise return target (only the unit-level validator is exercised), the Playwright e2e suite (not executed), internal CI/deployment wiring for the entry env var, Electron/Tauri packaging paths, and runtime NavRail behavior under dmloop_on / dmpersonal_on, which is not observable without the private module. The 164 pure-deletion files were reviewed by reference sweep rather than line by line.

yujiawei · 2026-07-29T03:39:59Z

One more P2 to add to my review above — a documentation gap I missed on the first pass, worth a line before this merges.

dmloopOn / dmpersonalOn are now consumer-less inside this repo (packages/dmworkbase/src/App.tsx:291,297). Their only in-repo readers were packages/dmloop/src/module.tsx:105 and packages/dmpersonal/src/module.tsx:48, both of which this PR deletes. The fields are still declared, still parsed from appconfig (:418-419), and still participate in the change-detection arm that fires notifyConfigChangeListeners() (:436-437) — so a backend flip of dmloop_on now notifies a bus with no local subscriber.

They should almost certainly stay, since the enterprise Loop/Personal modules presumably still gate their NavRail entry on WKApp.remoteConfig.dmloopOn. The problem is that nothing in the file says so any more: the JSDoc was reworded from "(LoopModule)" to "企业「回路」入口" but no longer names a consumer, so a future sweep for dead remote-config fields has nothing to stop it from deleting both — silently breaking the enterprise entry gate in a repo where the breakage isn't visible.

A one-line comment naming the enterprise module as the downstream consumer would close it.

lml2468

Re-review: #1145 — feat: move loop and personal behind enterprise slot

新提交 512554d1586b(在我上轮 APPROVE @ 0f39700f 之上)—— 唯一增量是新增 CI 守卫 .github/workflows/oss-module-guard.yml(+70),无任何源码/构建改动。锚定 head 512554d1586b(rev-parse 确认 checkout==live head)/ mergeable=true。

结论:APPROVE ✅

增量验证(`0f39700f..512554d1`)—— 仅 CI 守卫

新增 OSS Module Guard workflow,对每个到 main 的 PR:

拦截重新引入 loop/personal 文件:改动落在 packages/(dmloop|dmpersonal)/ 或 loop/personal e2e specs → CI fail(removed 状态放行,即允许删除)。
拦截重新引入 import:任一改动的非删除文件(排除守卫自身)含 @octo|dmwork/(loop|personal) 引用 → CI fail。
CI 卫生良好:最小权限(contents/pull-requests: read)、pinned action SHA、persist-credentials:false、concurrency cancel-in-progress、set -euo pipefail;if grep…; then true 正确吞掉无匹配的非零退出(不误触 set -e)。
这是对搬迁边界的正确 guardrail——从 CI 上防止未来 PR 把 loop/personal 悄悄搬回 OSS,与本 PR 的抽出互补。

前轮结论保持(源码与 `0f39700` 逐字节一致)

enterprise 登录回跳 🔴(我上轮漏判、已认领)在 0f39700 修复且保留:isSafeReturnPath 保留同源/控制字符守卫 + 纳入 handler 白名单;host 消费方传入 handlers。
yujiawei 的 VITE_* 声明 / 基线删除 / fs.allow 合默认 / enterpriseHtmlHead 文档化——均已在 0f39700 处理。
搬迁本体:dmloop/dmpersonal 已移除、零悬空 loop/personal import(本 head 复核)、slot 默认 no-op stub 不变。

验证(实测 @ 512554d1586b):extraction 复核完好(两包目录不存在、四种包名 import 引用均 0);workflow YAML 解析通过(on/jobs 齐)。本次增量仅 CI yml、零源码影响,构建结果与上轮 0f39700f(build ✅ 3.94s / 17 测试)一致,不重复跑。

🟡 非阻断(沿用)

与 #1120 的插槽 API 冲突/合并协调仍需处理(本 PR 泛化为 getEnterpriseStandaloneHandlers);合并顺序需统一。
守卫的 import-grep 理论上可能命中「注释里提到 @octo/loop」的良性行(如 docs-shim 注释)——仅当该行在改动文件里才触发,属保守拦截、可接受;必要时守卫可放宽为仅匹配 import 语句。
社区构建缺 loop/personal 为预期;企业侧模块须实现 slot 接口(跨仓)。

增量仅为一条正确、least-privilege 的 CI 守卫(强制搬迁边界、防未来回搬),无源码/构建影响;前轮 enterprise 回跳 🔴 与 yujiawei 各项均保持修复,搬迁本体零悬空。可合并 —— APPROVE(仍建议先协调与 #1120 的插槽 API)。

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #1145 (octo-web)

Summary

This PR moves the Loop (@octo/loop) and Personal (@octo/personal) modules out of the open-source monorepo and behind a pluggable enterprise module slot. The mechanism is a Vite virtual module (virtual:octo-enterprise-modules) that resolves to a no-op in the OSS build and to configurable enterprise entry points in private builds. The hardcoded CLI-authorize standalone handler in Layout/index.tsx is replaced by a generic enterprise standalone handler loop. The persistStandaloneReturn / consumeStandaloneReturn utilities are extracted from @octo/docs into the host so enterprise modules can register additional standalone return paths. All loop e2e tests, MSW handlers, and related test files are removed; CI expected test counts are adjusted accordingly.

The PR is well-structured: clean separation of concerns, comprehensive test coverage for new code, and the security-sensitive return-path validation is preserved and extended correctly.

Verification

✅ Return-path security preserved — isSafeReturnPath at apps/web/src/Layout/standaloneReturn.ts:14 enforces same-origin, no control characters, leading /, and allowlist matching. Tests at standaloneReturn.test.ts:49 prove off-origin URLs, protocol-relative URLs, and control-character paths are all rejected.
✅ HTML injection safe from $ special chars — enterpriseHtmlHeadPlugin at apps/web/vite.enterpriseHtml.ts:23 uses a function replacer (() => ...) instead of a string replacer, which prevents $&, $', $\`` from being interpreted as backreferences. Test at enterpriseHtmlHead.test.ts:23` explicitly proves this.
✅ Enterprise handler ordering correct — The enterprise standalone handler at Layout/index.tsx:392 is evaluated before /d/:docId and /s/:taskNo, which is by design: enterprise modules can own their own deep-link paths without host modification.
✅ Open-source build is a no-op — createEnterpriseModulesVirtualModule([]) at vite.enterpriseHtml.ts:65 produces registerEnterpriseModules(_context) {} and getEnterpriseStandaloneHandlers() { return [] }, ensuring zero enterprise side-effects in OSS.
✅ Extension build covered — wxt.config.ts:17 passes undefined entry, correctly getting the no-op virtual module for the browser extension.
✅ CI test count aligned — EXPECTED_P0 dropped from 29→1 across e2e.yml, e2e-nightly.yml, e2e-baseline-update.yml. EXPECTED_VISUAL dropped from 2→0. Matches the removal of all loop e2e tests (28 C-tests) while keeping the standalone doc test.
✅ chat-baseline mock updated — chat-baseline.ts:17 now includes MOCK_APP_CONFIG with dmloop_on: "0" and dmpersonal_on: "0", ensuring e2e tests don't accidentally surface enterprise UI.

Findings

No P0/P1 issues. The enterprise module slot mechanism is clean, the security model is sound, and the deletion of packages/dmloop (~22k lines) and packages/dmpersonal (~600 lines) is thorough with no dangling references in production code.

P2 — `consumeStandaloneReturn` accepts query strings without validation (`apps/web/src/Layout/standaloneReturn.ts:28`)

isSafeReturnPath validates url.pathname against the allowlist but passes the full raw value (including query string) through to the caller. An enterprise handler that matches on pathname /loop/cli-authorize would accept any query string, including potentially injected parameters. In practice this is safe because window.location.assign() treats the full URL as a same-origin navigation (already proven by the origin check), and query parameters are application-specific — but documenting this contract explicitly in the function's JSDoc would help future enterprise module authors reason about what they're opting into.

Nit — Stale file reference in `webhookMessagePreview/README.md:39` (`packages/dmworkbase/src/features/webhookMessagePreview/README.md`)

The file-tree bullet now says "Enterprise Loop module" instead of listing concrete paths. This is correct for the open-source repo (those paths are deleted), but a private enterprise repo that consumes this README might want to know where to find the enterprise-side counterparts. Not a blocker — just noting for awareness.

Things I checked that are fine

Virtual module code generation: createEnterpriseModulesVirtualModule at vite.enterpriseHtml.ts:65 correctly generates imports, register calls, and handler collection with array validation. Multi-entry composition is tested.
resolveEnterpriseEntry: at vite.enterpriseHtml.ts:31 correctly distinguishes package names (@scope/pkg) from file paths, throwing a clear error when a configured file doesn't exist.
parseEnterpriseFsAllow: at vite.enterpriseHtml.ts:93 correctly splits on path.delimiter, resolves relative paths, and deduplicates via Set.
persistReturnOnAnonymous gate: the enterprise handler only calls persistStandaloneReturn() for anonymous users when the handler explicitly opts in — preventing unauthorized return-path stashing.
layoutStandaloneDocPath.test.ts updated: the test at :121 now asserts consumeStandaloneReturn(getEnterpriseStandaloneHandlers()) instead of the old bare call.
enterpriseModuleSlot.test.ts: architectural guard test at :7 proves index.tsx imports from the virtual module and does NOT statically import @octo/loop or @octo/personal.
Dockerfile build args: VITE_ENTERPRISE_MODULES_ENTRY, VITE_ENTERPRISE_HTML_HEAD_PATH, VITE_ENTERPRISE_FS_ALLOW are passed through correctly at build time.

Data-flow tracing

registerEnterpriseModules → called from apps/web/src/index.tsx:79 → delegates to virtual module → no-op in OSS build, calls each entry's registerEnterpriseModules in enterprise build. Each entry receives { registerModule: (m) => WKApp.shared.registerModule(m) }.
getEnterpriseStandaloneHandlers → called from Layout/index.tsx:394 → returns [] in OSS build → in enterprise build, collects handlers from all entries, validates each returns an array.
consumeStandaloneReturn(handlers) → called from Layout/index.tsx:183 inside goMain() → reads sessionStorage → validates against built-in paths + enterprise handlers → returns safe same-origin path or null.

Blindspot checklist

C1 (dual-path parity) — clear. Old isLoopCliAuthorizePath handler and new enterprise handler share the same auth recovery + session persistence structure. The new handler adds persistReturnOnAnonymous (opt-in), which the old handler didn't have. This is an intentional capability addition, not an omission.
C2 (control-flow ordering) — clear. Enterprise handler is evaluated before /d/:docId and /s/:taskNo handlers. No enterprise module would register a /d/... or /s/... path (those are host-owned doc/summary namespaces), so no collision risk.
C3 (authorization boundary) — N/A. No auth changes; enterprise modules plug into an existing auth flow.
C4 (authorization lifecycle) — N/A.
C5 (build/runtime path) — clear. Dockerfile, .env.example, and vite.config.ts are all aligned. The VITE_ENTERPRISE_FS_ALLOW is dev-server-only (documented in .env.example:27).
C6 (governance/docs) — N/A.

Cross-round blocker review

N/A — first review of this PR.

Verdict: APPROVED

Clean, well-tested architectural change. The enterprise module slot is implemented with proper security boundaries, the virtual module correctly no-ops for open-source builds, and the deletion of dmloop/dmpersonal is thorough. No blocking issues found.

[Octo-Q] verdict: APPROVE — no P0/P1 findings. Enterprise module slot is correctly designed with security-first return-path validation, proper no-op defaults, and comprehensive test coverage.

lml2468 · 2026-07-29T06:49:16Z

补充(head 校准):审查期间 head 由 512554d1 推进到 94913ab2,正式 review 盖在 94913ab2。核对增量 512554d1..94913ab2 纯为合入 main带进的 #1143(dragSortAction.ts 抽取 + 测试 + ConversationListGrouped 重构)——我已在 #1143 单独 APPROVE(行为保持、测试通过);#1145 自身的抽出/slot/oss-module-guard 未变。故 APPROVE 对 94913ab2 成立,前述结论(enterprise 回跳修复保持、guard 正确、零悬空 loop/personal 引用)不变。其余 human-verify(与 #1120 插槽 API 协调)照旧。

Jerry-Xin

Summary: Project relevance gate passed; this PR is in scope and cleanly moves Loop/Personal out of the OSS host behind an enterprise module slot.

💬 Non-blocking

🔵 Suggestion: Consider adding apps/extension typecheck coverage once dependencies are available. The extension imports the web App (apps/extension/entrypoints/sidepanel/main.tsx), which imports Layout, and Layout now imports virtual:octo-enterprise-modules (apps/web/src/Layout/index.tsx). WXT bundling has the plugin wired (apps/extension/wxt.config.ts), but it would be useful to ensure the ambient declaration in enterprise-modules.d.ts is visible to extension typechecking.

✅ Highlights

The virtual module defaults to a no-op OSS implementation and composes configured private entries without static Loop/Personal imports (apps/web/vite.enterpriseHtml.ts).
Standalone enterprise deep links are delegated through a narrow handler contract while keeping host-owned session recovery and return-path safety (apps/web/src/Layout/index.tsx).
Return-target validation preserves the same-origin/control-character hardening and limits enterprise returns to handlers that explicitly opt in (apps/web/src/Layout/standaloneReturn.ts).
Residual @octo/loop / @octo/personal imports were removed from the host package dependencies and app entry point (apps/web/package.json, apps/web/src/index.tsx).

Verification note: I attempted the targeted Vitest and extension compile commands, but this checkout has no installed dependencies, so vitest and tsc were unavailable.

Superseded: reviewer approved PR at head 94913ab after enterprise-slot fixes; earlier concerns are addressed. Dismissing stale review at c334a2 to leave exactly one effective review.

Jerry-Xin

Summary: Project relevance gate passed; this PR is in scope and cleanly moves Loop/Personal out of the OSS host behind an enterprise module slot.

💬 Non-blocking

🔵 Suggestion: Consider adding apps/extension typecheck coverage once dependencies are available. The extension imports the web App (apps/extension/entrypoints/sidepanel/main.tsx), which imports Layout, and Layout now imports virtual:octo-enterprise-modules (apps/web/src/Layout/index.tsx). WXT bundling has the plugin wired (apps/extension/wxt.config.ts), but it would be useful to ensure the ambient declaration in enterprise-modules.d.ts is visible to extension typechecking.

✅ Highlights

The virtual module defaults to a no-op OSS implementation and composes configured private entries without static Loop/Personal imports (apps/web/vite.enterpriseHtml.ts).
Standalone enterprise deep links are delegated through a narrow handler contract while keeping host-owned session recovery and return-path safety (apps/web/src/Layout/index.tsx).
Return-target validation preserves the same-origin/control-character hardening and limits enterprise returns to handlers that explicitly opt in (apps/web/src/Layout/standaloneReturn.ts).
Residual @octo/loop / @octo/personal imports were removed from the host package dependencies and app entry point (apps/web/package.json, apps/web/src/index.tsx).

Verification note: I attempted the targeted Vitest and extension compile commands, but this checkout has no installed dependencies, so vitest and tsc were unavailable.

lml2468

Re-review + Verdict: #1145 — feat: move loop and personal behind enterprise slot @ `ebaa93626e05`

应请求重新审核并给出结论。 锚定 head ebaa93626e05(rev-parse 确认 checkout==live head)/ mergeable=true(状态 blocked 仅因 head 变动、需一张当前 head 的通过票)。

结论:APPROVE ✅ —— 可合并。

自我上轮 APPROVE(`94913ab2`)以来的增量 = 仅 CI 守卫微调(`oss-module-guard.yml`,+4/−5)

path-block 跳过 removed 文件:新增 $2 == "removed" { next } —— 修掉一处潜在问题:原规则不看状态,会把「删除 dmloop 文件」也判为违规(而删除正是本搬迁要做的)。现在删除放行、只拦新增/修改。合理。
import-grep 去掉 pnpm-lock.yaml / package.json 扫描:减少误报(依赖图里合法出现的包名)。守卫的主防线仍在——path-block(禁 dmloop/dmpersonal 文件)+ 源码 import grep(禁 .ts/.tsx 等里的 @octo|dmwork/loop|personal import)。小幅覆盖取舍(见 🟡),可接受。

核心结论保持(源码未变)

搬迁真实、无悬空:dmloop/dmpersonal 已移除,四种包名 import 引用本 head 复核 均为 0;slot 默认 no-op stub 不变。
我上轮漏判的 enterprise 登录回跳 🔴 已修且保留:isSafeReturnPath 保同源/控制字符守卫 + 纳入 handler 白名单,host 消费方传入 handlers。
yujiawei 各项(VITE_* 声明 / 基线删除 / fs.allow 合默认 / enterpriseHtmlHead 文档化)均已在此前 commit 处理。
零依赖/构建 blast radius:本次未碰 pnpm-workspace/lock/根 package.json/vite——不同于 #1147 的全局 override 隐患。

验证(实测 @ ebaa93626e05):extraction 复核完好(dmloop 目录不存在、四包 import 0);guard YAML 解析通过(on/jobs 齐)。增量仅 CI yml、零源码/依赖影响,构建与前轮 0f39700f(build ✅ / 17 测试)一致。

🟡 非阻断

import-grep 不再扫 package.json:若某 PR 仅在 package.json 重新加 @octo/loop 依赖(无源码 import),守卫不再拦——但无源码 import 不会把 loop 打进 bundle,且 path-block 仍拦真实包文件,覆盖损失极小。如需更严可保留 package.json 扫描但仅匹配 "dependencies" 段。
与 #1120 的插槽 API 冲突/合并顺序仍需协调(本 PR 泛化为 getEnterpriseStandaloneHandlers)。
虚拟模块 ambient 声明可补 apps/extension 侧 typecheck(Jerry-Xin 🔵)。

明确判断:APPROVE(可合并)。 增量仅为一条 least-privilege CI 守卫的合理微调(删除放行 + 减误报),无源码/依赖/构建影响;搬迁本体真实无悬空、enterprise 回跳修复保持、slot 默认干净。建议合并前与 maintainer 协调 #1145 与 #1120 的插槽 API 合并顺序(统一到 handler 模型)。

Jerry-Xin

Re-review at ebaa936. Re-affirming APPROVE.

The only incremental delta since my prior APPROVE (94913ab) is two CI-guard commits on .github/workflows/oss-module-guard.yml (+4/-5, strictly ahead — no merge-of-main drift). The change is sound and actually fixes a real bug in the guard:

$2 == "removed" { next } now skips removed-status files in the blocked-path scan, so this PR's own packages/dmloop / packages/dmpersonal deletions no longer trip the guard against itself. Correct.
The case glob dropped the redundant pnpm-lock.yaml|package.json entries (already covered by *.json / *.yaml). Cosmetic, harmless.
The second loop still skips removed files and the guard file itself.

Verified at the live head (ebaa936):

The standalone-return open-redirect fix (isSafeReturnPath + enterprise handler wiring) is byte-identical / untouched since my last APPROVE — same-origin validation intact in apps/web/src/Layout/standaloneReturn.ts:13 and consumed at apps/web/src/Layout/index.tsx:183.
Still a genuine move: packages/dmloop and packages/dmpersonal are absent, and there are ZERO dangling @octo/loop|personal / @dmwork/loop|personal imports in the changed files.
No dependency / build-config changes crept in (no pnpm-workspace.yaml, pnpm-lock.yaml, root package.json, or global override touched in this delta).

💬 Non-blocking (carried forward, not introduced by this delta)

🟡 apps/extension typecheck may not see the virtual:octo-enterprise-modules declaration. The extension imports the web app at apps/extension/entrypoints/sidepanel/main.tsx:18, and the web app imports the virtual module, but the ambient declaration lives only in apps/web/src/enterprise-modules.d.ts:1. Bundling resolves via the Vite plugin, but pnpm --dir apps/extension compile may fail unless WXT's generated tsconfig includes that web .d.ts. Consider an extension-local ambient declaration or a shared one.
🟡 The manual visual baseline workflow still runs --grep "@visual" while EXPECTED_VISUAL=0 (.github/workflows/e2e-baseline-update.yml:124); with no visual tests, Playwright may report "No tests found". Does not block PR CI.
🟡 (from lml2468) The guard's ref scan can false-match @octo/loop|personal strings that appear in comments rather than real import statements. Matching only actual import/require statements would reduce false positives.
🔵 resolveEnterpriseEntry in apps/web/vite.enterpriseHtml.ts:28 treats unscoped subpaths containing / as repo-relative; fine for the documented relative-path workflow, but it would reject bare package subpaths like enterprise-modules/loop if those are ever intended.
🔵 Merge-order coordination with the #1120 slot-API change still applies — land in an order that keeps the enterprise slot contract consistent.

✅ Highlights: safe OSS no-op default for the virtual module, host no longer statically imports Loop/Personal, and the open-redirect guard on standalone returns is preserved.

yujiawei

Code Review — PR #1145 (octo-web)

Head reviewed: ebaa93626e055270fcf049ce306b353bb197cd24 against merge-base 0e8fff238f9b32882d092410afef2c01ae33e778.

Note on anchoring: I was asked to review 512554d1, but the branch has moved twice since (512554d1 → 94913ab2, a rebase onto current main, → ebaa9362). I re-anchored and this verdict applies to ebaa9362 only. I confirmed the 512554d1..94913ab2 delta is purely the main merge (dragSortAction.ts extraction from a separate PR, not this one's work), and the 94913ab2..ebaa9362 delta is 4 added / 5 removed lines in .github/workflows/oss-module-guard.yml. 252 files, +683/−29988; 226 of them are pure deletions of packages/dmloop, packages/dmpersonal and the Loop host e2e assets. I read the complete non-deletion diff (23 files) line by line.

The two blocking issues I found at 94913ab2 are both fixed at this head. For the record, since they show what the fix pass actually closed:

The guard blocked its own PR. The awk path-block had no status filter, so every one of the ~200 packages/dmloop/** / packages/dmpersonal/** deletions in this very PR matched and the job exited 1 — verified in the failing run (Block removed enterprise modules, run 30429029664: it printed the whole deleted file list, then Process completed with exit code 1). The second half of the same script did skip removed, so the asymmetry was the tell. Fixed at oss-module-guard.yml:39 with $2 == "removed" { next }; the check is now green.
actionlint was red on the same file. The case glob list had *.json|*.yaml|*.yml|pnpm-lock.yaml|package.json, which trips ShellCheck SC2221/SC2222 — *.yaml already covers pnpm-lock.yaml and *.json already covers package.json, so those two branches were unreachable. Reproduced locally against the extracted run: block. Fixed by dropping the two redundant patterns at oss-module-guard.yml:57; coverage is unchanged because the globs match across /. Also green now.

1. Spec compliance

Spec: ✅

Missing work: none. Verified against the four stated goals:
- Loop/Personal removed from the host — repo-wide sweep for @octo/loop, @octo/personal, @dmwork/loop, @dmwork/personal across .ts/.tsx/.js/.mjs/.json/.yml/.yaml/.md returns exactly one hit, a prose comment in packages/docs/types/octo-base.shim.d.ts:15. No stale tsconfig path mappings, no stale tsconfig project references, no pnpm-workspace.yaml / turbo.json residue, no orphaned test:e2e:loop script reference in any workflow. apps/web/package.json drops both workspace:* deps.
- Vite enterprise module slot — virtual:octo-enterprise-modules resolves to a return [] / no-op stub when VITE_ENTERPRISE_MODULES_ENTRY is unset, and the multi-entry composition path is exercised by tests. The green Build job (pnpm build, unfiltered turbo run build) is the empirical proof that both apps/web and apps/extension build in the OSS default.
- Optional HTML head injection — present, fails closed on a missing path (vite.enterpriseHtml.ts:11-13), no-ops on empty.
- Loop e2e assets removed, docs kept host-owned — DocsModule still registered at apps/web/src/index.tsx; @octo/docs untouched.
- The compensating changes that had to come with the deletion are all present and correct: apps/web/e2e-kit/msw-handlers/chat-baseline.ts:42-47 re-provides the common/appconfig handler that the deleted loop-empty.ts used to own (and it was first in the array, so it previously won the match); e2e.yml:128-129, e2e-nightly.yml:87-88 and e2e-baseline-update.yml:108 downgrade EXPECTED_P0 29→1 and EXPECTED_VISUAL 2→0, which matches reality exactly — one @p0 spec remains (C989-search-close-on-bot-send-message.spec.ts:14) and zero visual specs.
Out-of-scope additions: two behavior changes, both justified, neither a spec failure — but I'd name them in the PR description:
1. apps/web/src/Layout/standaloneReturn.ts:9 adds STANDALONE_SUMMARY_SHARE_PATH. The @octo/docs implementation this replaces allowed only /d/:docId and /s/:taskNo (packages/docs/src/pages/StandaloneDocPage.tsx:109), so /s/share/:shareId targets — which the host has always persisted at Layout/index.tsx:465, unchanged by this PR — were silently dropped after login. Extracting the allowlist into the host and not covering it would have preserved that bug, so I read this as correctness-of-extraction rather than scope creep. It is still a user-visible behavior change: shared-summary deep links now actually return after sign-in.
2. persistReturnOnAnonymous (enterprise-modules.d.ts:20, consumed at Layout/index.tsx:414) is a capability the old @octo/loop CLI-authorize branch never had — the old code fell through to login stashing nothing. This is not a like-for-like port, but it is the fix an earlier round explicitly asked for, so it is requested work.
Divergence: none. The generalization is faithful: the branch still runs before the app shell, still does loginInfo.load() → recoverOctoSessionFromStorage(true) → cached currentSpaceId, and handler.render() replaces WKApp.route.get(LOOP_CLI_AUTHORIZE_PATH) one-for-one.

2. Code quality

Quality: Approved — no P0/P1. Eight P2s, all follow-up-PR material.

P2-1 — The tests this PR's safety story rests on are never executed by CI

.github/workflows/ci.yml:92,95

      - name: Skill Market tests
        run: pnpm --filter @dmwork/skillmarket test
      - name: Unified import/export unit tests
        run: pnpm --filter @octo/docs exec vitest run <five explicit files>

There is no turbo run test anywhere in CI, and apps/web has neither a lint script nor a tsc --noEmit script (apps/web/package.json declares "test": "vitest run", but nothing invokes it). So all four test files this PR leans on — src/__tests__/enterpriseHtmlHead.test.ts, src/__tests__/enterpriseModuleSlot.test.ts, src/Layout/__tests__/standaloneReturn.test.ts and the updated src/__tests__/layoutStandaloneDocPath.test.ts — plus the types in vite.enterpriseHtml.ts and enterprise-modules.d.ts, are verified only on the author's machine. vite build transpiles via esbuild and reports no type errors.

To be clear: the tests themselves are well-written (they import the real symbols rather than re-declaring copies, and the $&-literal replacer test at enterpriseHtmlHead.test.ts:24-30 is a genuinely good catch). This is a pre-existing infrastructure gap, not something this PR introduced. It matters more than usual here because the open-redirect allowlist and the OSS no-op slot are exactly the kind of invariant that silently rots: widen isSafeReturnPath, or drop the () => in enterpriseHtmlHeadPlugin so $& in an enterprise fragment starts expanding, and every check on the PR stays green. Adding pnpm --filter @octo/web exec vitest run to the Build job is a two-line change.

P2-2 — Head injection fails open when `</head>` is absent

apps/web/vite.enterpriseHtml.ts:20-24

    transformIndexHtml(html) {
      const trimmed = headHtml.trim();
      if (!trimmed) return html;
      return html.replace(/<\/head>/i, () => `${trimmed}\n  </head>`);
    },

String.prototype.replace with no match returns the input unchanged, so an entry HTML without a literal </head> — or a second HTML entry added later that this plugin also transforms — produces a successful build with the enterprise head silently missing. Every other failure mode in this file is loud: readEnterpriseHtmlHead throws at :12, resolveEnterpriseEntry throws at :39, and the generated virtual module throws on a non-array. Suggest matching that: when trimmed is non-empty and the regex does not match, throw.

P2-3 — The guard drops `previous_filename` and only knows the two literal directories

.github/workflows/oss-module-guard.yml:33-42

          gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/files" \
            --paginate \
            --jq '.[] | [.filename, .status] | @tsv' > "${changed_files}"

The --jq projection discards .previous_filename, and the path rules match only packages/(dmloop|dmpersonal) plus the loop/personal e2e-kit paths, while the reference grep matches only the literal @octo|@dmwork /loop|personal specifiers. Re-adding the deleted source as packages/enterprise-loop/** or apps/web/src/loop/** with relative imports passes both gates. That is fine for a speed bump — just don't read a green check as "no enterprise code came back". Cheap hardening: project .previous_filename into the awk input too, and grep for one or two distinctive removed symbols (LOOP_CLI_AUTHORIZE_PATH, PersonalModule) alongside the package specifiers. Separately, Dockerfile* at :57 only matches a root-level Dockerfile; a nested apps/*/Dockerfile is not scanned.

P2-4 — `getEnterpriseStandaloneHandlers()` now runs on every render

apps/web/src/Layout/index.tsx:396-398

        const enterpriseStandaloneHandlers = getEnterpriseStandaloneHandlers();
        const enterpriseStandaloneHandler = enterpriseStandaloneHandlers.find((handler) =>
            handler.match(window.location.pathname)
        );

This sits in AppLayout.render(). The code it replaces was isLoopCliAuthorizePath(window.location.pathname) — a pure regex test. Now every render calls into each configured enterprise entry's getter, allocates a fresh array, and re-runs every handler.match(). Harmless against the return [] stub, but nothing in the contract says the getter must be cheap or free of side effects, and an enterprise module author has no way to know that from enterprise-modules.d.ts. Either hoist it (module-level memo, or compute once per mount) or state "must be pure and cheap; may be called on every render" in the declaration.

P2-5 — `apps/extension` hardcodes the slot to `undefined`, with no comment saying that's deliberate

apps/extension/wxt.config.ts:18

      enterpriseModulesPlugin(undefined, path.resolve(__dirname, "../web")),

Two corrections to how this reads at first glance, because I checked both: this is not dead code, and it is not a regression.

It is required. apps/extension/entrypoints/sidepanel/main.tsx:18 does import App from '../../../web/src/App', apps/web/src/App/index.tsx imports AppLayout from '../Layout', and Layout/index.tsx imports virtual:octo-enterprise-modules. Without the plugin the extension would not build.
It is not a behavior loss. The extension registers only BaseModule, DataSourceModule, LoginModule and ContactsModule (main.tsx:164-167), so the old WKApp.route.get(LOOP_CLI_AUTHORIZE_PATH) always returned undefined there and enterprise full-page routes never rendered in the side panel. @octo/loop was previously pulled into the extension bundle transitively for no benefit, so this is a small win.

The real, forward-looking point is that because the entry is a literal undefined rather than read from VITE_ENTERPRISE_MODULES_ENTRY, an enterprise extension build has no supported path to inject modules — and nothing in the file records that as intentional. One comment, or plumbing the env var through, closes it.

P2-6 — `dmloopOn` / `dmpersonalOn` now have zero readers in this repo

packages/dmworkbase/src/App.tsx:291,297

Both flags are still declared, parsed (:418-419) and change-detected (:436-437), but their only in-repo consumers were the deleted modules' menu factories. The JSDoc was updated to say "企业「回路」入口", which is right, but it doesn't say who reads them — so to a future contributor doing an OSS dead-code sweep these look deletable, and deleting them would silently break the enterprise NavRail gate. Worth one explicit line: no in-repo consumer, kept as the host↔enterprise contract, the enterprise menu factory reads it.

While I was in there, a positive worth recording: apps/web/src/Pages/Main/vm.ts:111 and menuReconcile.ts are untouched by this PR, and the flip-off teardown is keyed on menu membership rather than on any specific module — so an enterprise menu still inherits route pruning and routeRight.popToRoot() for free when its flag goes false. The extraction didn't cost that.

P2-7 — Nothing stops an enterprise `match()` from shadowing the host's own `/d` and `/s` namespaces

apps/web/src/Layout/index.tsx:397 vs :434, :457, :471

The enterprise find() runs first and unconditionally:

        const enterpriseStandaloneHandler = enterpriseStandaloneHandlers.find((handler) =>
            handler.match(window.location.pathname)
        );

isStandaloneDocPath is not reached until :434, parseStandaloneSummaryShareId until :457, and isStandaloneSummaryPath until :471. So an enterprise handler with an over-broad match() — pathname.startsWith("/"), a missing anchor in a regex, a catch-all fallback handler — silently takes over /d/:docId, /s/:taskNo and /s/share/:shareId, and the host renders the enterprise page instead of the document. Failure mode: standalone doc and summary deep links break repo-wide, from a change made entirely in a private repo, with nothing in this repo to point at.

This risk is new. The code being replaced was isLoopCliAuthorizePath(...) — a fixed regex owned by the host, where shadowing was structurally impossible. Generalizing the branch traded that guarantee away, and the host takes no compensating measure.

Two cheap options: assert in the host that no enterprise handler claims a host-owned prefix (/d/, /s/) and skip it with a console error if it does; or move the enterprise find() after the doc/summary branches so host namespaces always win. The second is a one-block move and makes the precedence explicit rather than incidental. Either way this belongs in enterprise-modules.d.ts as a stated contract — right now an enterprise author has no way to learn that /d and /s are reserved.

P2-8 — Nits (batch)

apps/web/e2e-kit/_lib/mswScenario.ts:19 — export type MswScenario = string | "no-mock" collapses to string; the literal is decorative and the type no longer constrains anything. Both remaining call sites pass 'no-mock', so a plain string (or a two-member union of what's actually supported) would be more honest.
apps/web/e2e-kit/msw-handlers/chat-baseline.ts:42-47 — msw's * spans /, so */common/appconfig already matches …/api/v1/common/appconfig; the first handler is redundant. Harmless, same payload.
apps/web/.env.example:21 documents that a relative VITE_ENTERPRISE_HTML_HEAD_PATH resolves from apps/web; the VITE_ENTERPRISE_MODULES_ENTRY (:11-18) and VITE_ENTERPRISE_FS_ALLOW (:26-32) blocks don't, although both behave the same way (vite.enterpriseHtml.ts:44,:99). Worth one line each for symmetry.
path.delimiter for multi-entry lists (vite.enterpriseHtml.ts:47,:96) makes a .env non-portable between Windows and macOS/Linux, and a Windows absolute path (C:\…) inside a :-separated list can't be expressed at all. It is documented at .env.example:12,29, so it's a stated constraint rather than a trap — non-blocking given the toolchain targets, but a fixed , would remove the caveat.
apps/web/vite.enterpriseHtml.ts is now misnamed: it owns the module slot as well (enterpriseModulesPlugin, parseEnterpriseFsAllow), and apps/extension imports the slot from a file called vite.enterpriseHtml. Splitting or renaming to vite.enterprise.ts would help the next reader.
EnterpriseStandaloneHandlerContext (enterprise-modules.d.ts:12-16) passes pathname and search but not hash, and persistStandaloneReturn (standaloneReturn.ts:38-40) stores pathname + search only. The latter is a faithful port of the @octo/docs original (StandaloneDocPage.tsx:61-64), so no regression — but a CLI-authorize-style callback that carries state in the fragment would lose it, and the handler can only recover it by reading window.location.hash itself.
consumeStandaloneReturn validates only url.pathname against the allowlist and then returns the full raw value, query string included (standaloneReturn.ts:34-44). Same-origin is already structurally guaranteed by the url.origin check at :30, so this is not exploitable as a redirect — but an enterprise handler that opts into persistReturnOnAnonymous is accepting arbitrary attacker-influenceable query parameters on its own path, since the value comes from sessionStorage. This is a faithful port of the previous @octo/docs behavior, not something this PR introduced. Worth stating in the JSDoc so enterprise authors know the query string is their problem to validate.
resolveEnterpriseEntry (vite.enterpriseHtml.ts:28-35) treats any specifier containing a / that doesn't start with @ as a filesystem path, so an unscoped package subpath (my-corp-modules/dist/entry.js) skips the bare-specifier branch, falls through to path.resolve, and dies with "does not exist" instead of letting Vite resolve it from node_modules. Scoped subpaths (@corp/mods/entry.js) are fine. Narrow case; only worth fixing if unscoped enterprise packages are on the table.

3. Overall verdict

APPROVE. Spec ✅ and Quality Approved. The extraction is clean, the OSS default genuinely degrades to a no-op, the removal is complete with no dangling references, and the two things that were actually red on CI are now green and verified so at this head. Everything above is a follow-up item; none of it is worth another push.

4. Recommendations, in priority order

Add pnpm --filter @octo/web exec vitest run to the Build job (P2-1). Without it the guard tests added here gate nothing, and that's the single highest-leverage two-line change on this PR.
Reserve /d and /s against enterprise match() shadowing (P2-7) — either move the enterprise find() below the doc/summary branches, or reject handlers that claim a host prefix. This is the one item where a mistake made in a private repo breaks open-source behavior with no local trace.
Make enterpriseHtmlHeadPlugin throw when a non-empty fragment finds no </head> (P2-2) — it's the one fail-open path in an otherwise consistently fail-closed file.
Broaden the guard with .previous_filename and a symbol-level grep (P2-3), and note in the workflow itself what it does not cover, so a green check isn't over-read.
One comment each on the dmloopOn/dmpersonalOn contract (P2-6) and the extension's hardcoded undefined (P2-5). Both are "obvious today, deletable in six months" cases.
Document the getEnterpriseStandaloneHandlers() purity/cost expectation in enterprise-modules.d.ts, or hoist the call out of render() (P2-4). While editing that file, state the query-string contract for persistReturnOnAnonymous handlers (P2-8).
Add the two behavior changes from §1 (/s/share return targets, persistReturnOnAnonymous) to the PR description — they're correct, but a reader of the description today wouldn't know redirect behavior changed.

5. Additional observations (outside the review scope, no action required on this PR)

This repository's e2e safety net is now close to nothing. @p0 goes 29 → 1 and visual 2 → 0, apps/web/e2e-kit/screenshots/ has no baselines left, and the drift guard at e2e.yml:134 now only asserts "at least one test ran" (EXPECTED_TOTAL=1). That is an unavoidable consequence of moving the modules out, not a defect in this change — but it should become a tracked follow-up, or this repo quietly loses its regression coverage along with the code.
The open-redirect allowlist is now extensible from closed source. Any enterprise handler declaring persistReturnOnAnonymous: true widens isSafeReturnPath (standaloneReturn.ts:47). Gates 1 and 2 — control-character rejection at :17 and the same-origin check at :30 — still live in the host, so this can only widen same-origin paths and can never produce an off-origin redirect. That's an acceptable trade, but this repository can no longer audit the complete allowlist, and saying so in a comment at standaloneReturn.ts would help the next security reviewer.
One invariant lost its guard in the deletion. The removed loopCliAuthorizeDeepLink.test.ts asserted that the full-page branch is evaluated before <Provider (and that recoverOctoSessionFromStorage(true) is present). The replacement enterpriseModuleSlot.test.ts asserts neither. The ordering is still correct today — handler branch at Layout/index.tsx:396, return <Provider at :505 — but it's now unguarded, and it's a cheap assertion to carry over.
Docker looks right. The three new ARG/ENV pairs (Dockerfile:18-23) sit in the builder stage, before FROM nginx:alpine at :26, so none of them leak into the runtime image. Worth noting for whoever wires internal CI that VITE_ENTERPRISE_MODULES_ENTRY and VITE_ENTERPRISE_HTML_HEAD_PATH are only usable if those files are inside the Docker build context.
Process note. This is roughly the fifth review round: 14 formal reviews across six heads (dbf470f0 → c334a246 → 0f39700f → 512554d1 → 94913ab2 → ebaa9362) in about five and a half hours, and the last two rounds fixed precisely the two things CI was already reporting red. Every finding above is a follow-up-PR item. My recommendation is to freeze the branch and land it on this head rather than keep accumulating advisory nits — the marginal value of another round is now lower than its cost.

What I could not verify

No node_modules in this checkout, so the unit suites and the OSS vite build were not reproduced locally. I relied on the green Build (unfiltered turbo run build, so both apps/web and apps/extension) and e2e-p0 jobs at this head, plus reading the test files against their implementations.
The private enterprise entry module is not visible here, so I could not check that a real registerEnterpriseModules / getEnterpriseStandaloneHandlers implementation matches the shapes declared in enterprise-modules.d.ts, nor that an enterprise build's handler.render() uses the onSessionExpired callback the host now hands it.
apps/extension has a compile script (tsc --noEmit) but no CI job invokes it, and wxt build does not typecheck — so the new cross-app TypeScript import at wxt.config.ts:5 is build-resolved but never type-checked anywhere.
The actionlint failure at 94913ab2 was reproduced locally from the extracted run: block rather than from the job log, which was no longer retrievable.

menghao-webtest requested review from a team as code owners July 29, 2026 02:38

github-actions Bot added the size/XL PR size: XL label Jul 29, 2026

github-actions Bot added the dependencies-changed This PR modifies dependency files label Jul 29, 2026

menghao-webtest force-pushed the codex/loop-module-refactor branch from dbf470f to c334a24 Compare July 29, 2026 02:41

lml2468 previously approved these changes Jul 29, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 29, 2026

View reviewed changes

Jerry-Xin previously requested changes Jul 29, 2026

View reviewed changes

menghao-webtest dismissed stale reviews from mochashanyao and lml2468 via e4a194e July 29, 2026 02:58

yujiawei requested changes Jul 29, 2026

View reviewed changes

lml2468 previously approved these changes Jul 29, 2026

View reviewed changes

Jerry-Xin previously approved these changes Jul 29, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 29, 2026

View reviewed changes

yujiawei previously approved these changes Jul 29, 2026

View reviewed changes

yujiawei mentioned this pull request Jul 29, 2026

feat(global-search): add cloud docs full-text search tab #1087

Merged

10 tasks

menghao-webtest dismissed stale reviews from yujiawei, mochashanyao, Jerry-Xin, and lml2468 via 512554d July 29, 2026 06:39

侯孟豪 added 4 commits July 29, 2026 14:41

feat: move loop and personal behind enterprise slot

70e90c6

fix: stabilize enterprise slot checks

7478a81

fix: harden enterprise module slot

feb3668

ci: block removed enterprise module changes

94913ab

menghao-webtest force-pushed the codex/loop-module-refactor branch from 512554d to 94913ab Compare July 29, 2026 06:42

lml2468 previously approved these changes Jul 29, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 29, 2026

View reviewed changes

Jerry-Xin previously approved these changes Jul 29, 2026

View reviewed changes

ci: allow enterprise module removals in guard

c3cf3be

menghao-webtest dismissed stale reviews from Jerry-Xin, mochashanyao, and lml2468 via c3cf3be July 29, 2026 07:12

ci: fix enterprise module guard awk

ebaa936

menghao-webtest requested review from Jerry-Xin and lml2468 July 29, 2026 07:31

lml2468 approved these changes Jul 29, 2026

View reviewed changes

Jerry-Xin approved these changes Jul 29, 2026

View reviewed changes

menghao-webtest merged commit 2d087f0 into Mininglamp-OSS:main Jul 29, 2026
19 checks passed

yujiawei approved these changes Jul 29, 2026

View reviewed changes

Uh oh!

Conversation

menghao-webtest commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Notes

Uh oh!

github-actions Bot commented Jul 29, 2026

Dependency Changes Detected

Uh oh!

lml2468 left a comment

Choose a reason for hiding this comment

Review: #1145 — feat: move loop and personal behind enterprise slot

Gate 1 — 规格符合度 ✅

Gate 2 — 代码质量 ✅

🟡 human-verify(大重构,建议合并前确认)

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #1145 (octo-web)

Summary

Verification

Findings

P2 — Missing readEnterpriseHtmlHead error handling for non-existent file (apps/web/vite.enterpriseHtml.ts:10)

Nit — Missing semicolon (apps/web/src/index.tsx:79)

Nit — Stale comment reference to deleted file (apps/web/e2e-kit/msw-handlers/chat-baseline.ts:12)

Things I checked that are fine

Verdict: APPROVED

Uh oh!

lml2468 commented Jul 29, 2026

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #1145 (octo-web)

1. Scope compliance

2. Code quality

P1-1 — config() clobbers Vite's default fs.allow, so the enterprise dev server 403s on every workspace-sibling import

P1-2 — $&, $`, $', $1 in the head fragment are interpreted as replacement patterns and corrupt the emitted index.html

P1-3 — a misconfigured enterprise entry silently yields a build with zero enterprise modules

P2 findings

3. Overall verdict

4. Suggested changes

5. What I verified / not covered

Uh oh!

yujiawei commented Jul 29, 2026

Uh oh!

lml2468 left a comment

Choose a reason for hiding this comment

Re-review: #1145 — feat: move loop and personal behind enterprise slot

先认领:我上轮 @ c334a246 的漏判

增量验证(c334a246..0f39700f)—— 🔴 已修

yujiawei 的 🔴/🟡 — 已处理

搬迁本体保持

🟡 非阻断(沿用)

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #1145 (octo-web)

Summary

Verification

Findings

P2 — HTML head injection regex may split on embedded </head> (apps/web/vite.enterpriseHtml.ts:19)

P2 — Orphaned dmloopOn / dmpersonalOn feature flags (packages/dmworkbase/src/App.tsx:291)

Things I checked that are fine

Verdict: APPROVED

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #1145 (octo-web)

1. Scope compliance

menghao-webtest commented Jul 29, 2026 •

edited

Loading

P2 — Missing `readEnterpriseHtmlHead` error handling for non-existent file (`apps/web/vite.enterpriseHtml.ts:10`)

Nit — Missing semicolon (`apps/web/src/index.tsx:79`)

Nit — Stale comment reference to deleted file (`apps/web/e2e-kit/msw-handlers/chat-baseline.ts:12`)

P1-1 — `config()` clobbers Vite's default `fs.allow`, so the enterprise dev server 403s on every workspace-sibling import

P1-2 — `$&`, $`, `$'`, `$1` in the head fragment are interpreted as replacement patterns and corrupt the emitted `index.html`

先认领:我上轮 @ `c334a246` 的漏判

增量验证(`c334a246..0f39700f`)—— 🔴 已修

P2 — HTML head injection regex may split on embedded `</head>` (`apps/web/vite.enterpriseHtml.ts:19`)

P2 — Orphaned `dmloopOn` / `dmpersonalOn` feature flags (`packages/dmworkbase/src/App.tsx:291`)

增量验证(`0f39700f..512554d1`)—— 仅 CI 守卫

前轮结论保持(源码与 `0f39700` 逐字节一致)

P2 — `consumeStandaloneReturn` accepts query strings without validation (`apps/web/src/Layout/standaloneReturn.ts:28`)

Nit — Stale file reference in `webhookMessagePreview/README.md:39` (`packages/dmworkbase/src/features/webhookMessagePreview/README.md`)

Re-review + Verdict: #1145 — feat: move loop and personal behind enterprise slot @ `ebaa93626e05`

自我上轮 APPROVE(`94913ab2`)以来的增量 = 仅 CI 守卫微调(`oss-module-guard.yml`,+4/−5)

P2-2 — Head injection fails open when `</head>` is absent

P2-3 — The guard drops `previous_filename` and only knows the two literal directories

P2-4 — `getEnterpriseStandaloneHandlers()` now runs on every render

P2-5 — `apps/extension` hardcodes the slot to `undefined`, with no comment saying that's deliberate

P2-6 — `dmloopOn` / `dmpersonalOn` now have zero readers in this repo

P2-7 — Nothing stops an enterprise `match()` from shadowing the host's own `/d` and `/s` namespaces