feat: config-driven activity hooks (extension publishes events to configured URLs)

[amrmelsayed]

What

The VSCode extension publishes abstract activity events (window-focus, builder-active) to URL hooks declared in the Codev config — with no knowledge of who listens. The outbound-direction sibling of the controller-agnostic command relay (#1091). The destination URL (a deep link, a companion app, a webhook launcher) lives entirely in the user's config; the extension carries zero vendor/controller references.

13 files across types / core / codev / vscode. No new dependencies.

Config + hierarchy (the important part)

Hooks are resolved through Tower's canonical loadConfig 5-layer merge, exactly like worktree-config — not by reading a file directly. So both override layers work:

defaults → <cache> → ~/.codev/config.json → .codev/config.json → .codev/config.local.json
                     (global, all repos)     (project, committed)   (per-engineer, gitignored)

// e.g. ~/.codev/config.json (personal hook that follows EVERY workspace)
{
  "activityHooks": [
    { "on": ["window-focus", "builder-active"],
      "url": "<scheme>://...?workspace={workspace}&builder={builder}",
      "background": true }
  ]
}

{workspace}/{builder} → URL-encoded event data (absent → empty). Arrays follow deepMerge semantics: a higher layer replaces a lower one's list, so define hooks in a single layer.

Layers (mirrors worktree-config end-to-end)

• types — ActivityEvent / ActivityHook / ResolvedActivityHooks wire contracts.
• codev — UserConfig.activityHooks; getActivityHooks() resolves + validates via loadConfig; GET /api/activity-hooks reuses the existing config-file watcher, so an edit to .codev/config(.local).json fans out the same worktree-config-updated SSE clients already react to (no duplicate fs.watch).
• core — TowerClient.getActivityHooks (mirror of getWorktreeConfig).
• vscode — load-activity-hooks.ts fetches from Tower; activity-hooks.ts fires from a cache refreshed on connect + the config-change SSE (wired into the existing dev-command-context subscription); the focus / diff / terminal / sidebar hook points publish events. command-relay adds the generic focus-workspace + scroll verbs.

Safety / opt-in

Inert by default — no activityHooks configured → the extension does nothing; non-users see zero behavior change. URLs are only open'd (template, never eval'd); values URL-encoded. Delivery is fire-and-forget; a url with no handler is swallowed and pauses hooks for the window after one warning.

Verification

• types + core build, codev tsc, vscode check-types + lint — all clean.
• codev: 4 new hierarchy tests prove config.local.json replaces project hooks and ~/.codev/config.json is picked up (30 config tests total).
• vscode: resolveHookUrls unit-tested (interpolation/encoding/event-filter/malformed); full suite 498 tests pass.
• Behavior verified on hardware via a Stream Deck deep-link hook.

Reworked twice from review: (1) Stream-Deck-specific → general activity hooks; (2) direct-file-read → Tower's config hierarchy. The extension now carries no controller-specific code and honors config.local.json + the global layer.

[amrmelsayed]

Updated per review: generalized the shared config-file watcher so the activity-hooks path doesn't borrow worktree-named machinery.

• worktree-config-watcher.ts → codev-config-watcher.ts
• ensureWorktreeConfigWatcher → ensureCodevConfigWatcher (+ set/stopAll notifier/watcher fns)
• SSE event worktree-config-updated → codev-config-updated

The watcher always watched .codev/config(.local).json generally (its docstring said so); the names were just historical. Pure rename — behavior unchanged. All consumers updated (tower-server, tower-routes, and the extension's dev-command + activity-hooks + Workspace-tree subscribers) plus the 3 tests that assert the event string. 498 vscode + 30 codev tests still green.

[amrmelsayed]

Reviewer Integration Review (CMAP-3)

Verdicts

• Gemini (agy): REQUEST_CHANGES (HIGH) — zero-click RCE via committed config layer; Windows cmd /c start injection; remote-dev incompatibility; focus-event spam.
• Codex: REQUEST_CHANGES (HIGH) — activation-time window-focus fires before hooks are loaded; the advertised initial-sync path is dead.
• Claude: APPROVE (HIGH) — excellent pattern-fit read, but took two surface-level reads that the diff contradicts (details below).
• Reviewer (own read): one CRITICAL finding neither lane caught — a controller can silently approve a human-only gate.

Synthesis

The architecture is genuinely clean: it mirrors worktree-config end-to-end (loader, resolver, route, client, shared watcher), the type layering is correct, the rename is behavior-preserving and complete in code, and it's inert by default. Claude is right about all of that. But the disagreement here is the signal: Gemini and Codex each found real defects Claude missed, and I verified a critical one none of them flagged. Claude's APPROVE rests on two assumptions I checked against the source and found false (approve-gate "shows confirmation modal"; the Windows opener is "the user's own config"). So this is a blocking REQUEST_CHANGES, not a 2-1 majority call — the security findings are verified against the code, not model opinion.

Findings

1. CRITICAL — a controller can silently approve a human-only gate (reviewer; verified, not caught by any lane).
command-relay.ts adds approve-gate → codev.approveGate and dispatches executeCommand(command, ...args) with args taken verbatim from the controller's /api/command POST. The registration forwards a second arg: regCli('codev.approveGate', (arg, options?: {skipConfirmation?}) => approveGate(cm, cache, extractBuilderId(arg), options)). And approve.ts has:

if (options?.skipConfirmation) {
  await runPorchApprove(...);   // → porch approve <id> <gate> --a-human-explicitly-approved-this
  return;
}

So POST /api/command {"verb":"approve-gate","args":["0042",{"skipConfirmation":true}]} runs porch approve 0042 <gate> --a-human-explicitly-approved-this with no human in the loop — passing the explicit-human-approval flag without a human. This breaches the two-human-gates invariant and directly contradicts the PR comment's "a deliberate, human-confirmed action, never a silent approve." The only barriers are the relay's focus self-gate and Tower's permissive /api/command auth (the #1091 deferred follow-up), neither of which is a real guard against the controller this is built for. Disposition: blocking. Fix: the relay must not forward arbitrary args to approve-gate (pass only the builder id, never an options object), or approveGate must ignore skipConfirmation when invoked via the relay path. Add a regression test asserting a relayed approve-gate with a second arg still shows the modal.

2. CRITICAL/HIGH — zero-click RCE via committed .codev/config.json (Gemini; verified).
getActivityHooks resolves through loadUserConfig → loadConfig, whose merge includes the committed project layer .codev/config.json. A malicious repo can commit an activityHooks entry whose url triggers execution (a registered scheme handler, a file:///path, or the Windows cmd /c start injection in finding 3), and the extension auto-activates on any workspace containing .codev/, then fires the hook on window-focus. No Workspace Trust gate and no untrustedWorkspaces declaration. Interaction with finding 4: Codex's ordering bug currently makes the activation-time fire a no-op (hooks not yet cached), which accidentally narrows this to "after first focus" — so fixing finding 4 naively makes the RCE truly zero-click. The two must be fixed together. Disposition: blocking. Fix per Gemini: exclude the committed Layer 4 from activity-hook resolution (hooks are personal: resolve from ~/.codev/config.json + .codev/config.local.json only), gate on vscode.workspace.isTrusted, and declare untrustedWorkspaces: { supported: 'limited' }.

3. MEDIUM/HIGH — openUrl uses raw execFile instead of vscode.env.openExternal (reviewer + Gemini).
Two problems with execFile('open'/'xdg-open'/'cmd /c start'):

• Remote dev (SSH/WSL/Codespaces): the extension host is the remote machine, so open/xdg-open run there (headless), not on the user's local desktop where the companion app/Stream Deck lives. The feature is misrouted or fails. vscode.env.openExternal(Uri.parse(url)) forwards to the local client.
• Windows cmd /c start "" url: an & in a URL template (e.g. app://x?workspace={workspace}&builder={builder}) is a cmd command-separator; Node won't quote a &-only arg, so cmd truncates the URL and may execute the remainder. This is also the execution primitive for finding 2 on Windows.
  Disposition: should-fix. Prefer openExternal for all platforms; keep the macOS open -g path only when background:true (its non-foregrounding semantics are the one thing openExternal can't express — worth confirming that's the actual requirement).

4. HIGH (correctness) — activation-time window-focus fires before hooks load (Codex; verified).
extension.ts:238 fires fireActivity('window-focus') on activation, but syncActivityHooks() (line 280, and async) populates cachedHooks afterward; cachedHooks starts []. So the "publish once on activation so a listener syncs on reload without a focus bounce" intent is defeated — the event fires into the void. Disposition: real bug. Fix by loading hooks first (await the initial sync, then fire if focused), and add a "window already focused on startup" lifecycle test. (See finding 2 for the coupling.)

5. LOW/MEDIUM — no dedup/throttle on builder-active (reviewer + Gemini).
builder-active fires from three subscriptions (terminal focus, sidebar selection, editor diff focus); rapid navigation within one builder re-fires the same hook, spawning a process each time. Add a last-fired-URL/event dedup or short debounce.

Non-blocking notes

• focus-workspace → vscode.openFolder(path, {forceNewWindow:true}) opens an arbitrary controller-supplied path in a new window if not already open. Minor surprise/abuse surface under the permissive relay auth; worth validating the path is a known workspace.
• deliveryDisabled is a coarse module-global latch (one bad URL pauses all hooks for the window). Acknowledged in code; fine for v1.
• Doc staleness: the PR body still references the pre-rename worktree-config-updated event string (code is consistent). Separately, arch.md (≈lines 315/319/323-324) still references worktree-config-updated / worktree-config-watcher.ts (Claude) — a MAINTAIN follow-up, not blocking.
• Test coverage of resolveHookUrls and the config hierarchy is good; the gaps are the lifecycle case (finding 4) and a relayed-approve-gate regression (finding 1).

Merge recommendation

Request changes (blocking). Two security-critical findings (controller-driven silent gate approval; committed-config RCE) plus a real activation-ordering bug. The lone APPROVE took two claims at face value that the source contradicts. The substrate and pattern-fit are strong, so once findings 1, 2, and 4 are addressed (3 and 5 should ride along), this should converge quickly.

---

Reviewer (sibling architect) CMAP-3 integration review

[amrmelsayed]

Architect Integration Review

Endorsing reviewer's REQUEST_CHANGES (blocking). I verified findings 1 and 2 against the source on PR head; both real.

Finding 1 (CRITICAL — silent gate approval) verified. Confirmed in three steps against pr-1105:

• command-relay.ts registers 'approve-gate': 'codev.approveGate' in VERB_COMMANDS.
• The dispatch is await vscode.commands.executeCommand(command, ...args) with args taken verbatim from the controller's /api/command POST.
• commands/approve.ts defines approveGate(cm, cache, builderIdArg?, options?: ApproveGateOptions) where ApproveGateOptions = { skipConfirmation?: boolean }. The docstring even names the third invocation path: "Gate-pending toast's [Approve] button → builder ID + options { skipConfirmation: true }. The toast was the context; approving from there commits directly with no second confirmation."

So POST /api/command {"verb":"approve-gate","args":["0042",{"skipConfirmation":true}]} reaches the documented "no second confirmation" path with no toast having been shown. Runs porch approve 0042 <gate> --a-human-explicitly-approved-this with no human in the loop. The PR comment's "never a silent approve" is contradicted by the registration shape. Blocking.

Finding 2 (CRITICAL — committed-config RCE) verified. getActivityHooks in the new packages/codev/.../activity-hooks.ts calls loadUserConfig(root), which is the 5-layer loadConfig chain explicitly described in the PR body: defaults → cache → ~/.codev/config.json → .codev/config.json → .codev/config.local.json. Layer 4 is the committed project layer. A malicious repo can ship an activityHooks URL that triggers execution on the first window-focus, with no Workspace Trust gate (vscode.workspace.isTrusted is not consulted; no untrustedWorkspaces declaration in package.json capabilities). Blocking.

Coupling between findings 2 and 4 confirmed. Codex's activation-ordering finding is correct against extension.ts: the activation-time fireActivity('window-focus') runs before syncActivityHooks() has populated cachedHooks, so today the activation-fire is dead code. That accidentally narrows finding 2 to "after first focus." Fixing the ordering bug naively (load hooks first, then fire the initial window-focus) makes finding 2 truly zero-click on workspace open. The two MUST be fixed in the same change, and the test added for finding 4 needs to assert the trust gate is in effect before any fire happens.

Priority order for the fix

1. Findings 1, 2, 4 are all blocking and must land together. Fix 1 and 2 independently; fix 4 only after 2's trust gate + committed-layer exclusion is in place, so the fixed ordering doesn't re-open the RCE window.
2. Finding 3 (execFile vs openExternal) should ride along in the same PR. It's the execution primitive for finding 2 on Windows and breaks remote dev today; same surface, related test coverage.
3. Finding 5 (builder-active dedup/throttle) can ride along or land as a small follow-up — non-blocking but cheap.

Suggested fixes (consistent with reviewer's directions)

• Finding 1: the relay's runVerb should special-case approve-gate to strip any second arg before dispatch — pass only the builder id, never an options object. Equivalently, approveGate could refuse skipConfirmation when invoked via the relay, but the relay-side strip is simpler and keeps the toast path's contract intact. Add a regression test that POSTing approve-gate with a second arg still shows the modal.
• Finding 2: getActivityHooks should accept a flag (or have a sibling helper) that excludes the committed Layer 4. Activity hooks are personal config, not project config — they resolve from ~/.codev/config.json + .codev/config.local.json only. Add vscode.workspace.isTrusted gate at the extension's first-fire site, and declare capabilities.untrustedWorkspaces: { supported: 'limited' } in package.json so VS Code's own trust UX is active.
• Finding 4: await syncActivityHooks() before the initial fireActivity('window-focus') on activation. Add a lifecycle test that mocks an already-focused window and asserts no fire happens until the trust gate clears AND the cache is populated.
• Finding 3: route through vscode.env.openExternal(Uri.parse(url)) for all platforms. Keep the macOS open -g path only if background: true is set AND only after confirming openExternal can't express "open without foregrounding" (worth checking the VS Code API docs — if openExternal({ allowContributedOpeners: false }) plus the URL scheme handler's own behavior covers it, drop the open -g branch entirely).

What's good (worth keeping)

Reviewer is right that the substrate is clean: the worktree-config mirror is correct end-to-end, the rename to codev-config-watcher is behavior-preserving, the type layering across types / core / codev / vscode is well-factored, and the "inert by default" stance is preserved. These findings are localized — they don't propagate into the design; they're three or four surgical changes within the architecture you already shipped. Once they land, this is mergeable.

---

Architect integration review

[amrmelsayed]

Addressed CMAP-3 review — all 5 findings fixed

I independently verified F1 and F4 against the source first (both real). Fixes in the latest push:

F1 (CRITICAL — silent human-gate approval). The relay forwarded controller args verbatim, so approve-gate could receive {skipConfirmation:true} and run porch approve --a-human-explicitly-approved-this with no human. → command-relay.ts now forwards only the builder id for approve-gate (never a second options object); other verbs that legitimately take object args (scroll) are unaffected. + regression test asserting a relayed approve-gate with a second arg never reaches codev.approveGate with the options object.

F2 (CRITICAL — zero-click RCE via committed config). getActivityHooks now resolves only from the trusted personal layers — ~/.codev/config.json + .codev/config.local.json — and never the committed .codev/config.json a cloned repo controls. + fireActivity is gated on vscode.workspace.isTrusted. + a codev test asserts the committed layer is ignored.

Note on untrustedWorkspaces: limited: I verified the VS Code docs — an undeclared extension is already disabled in Restricted Mode, so declaring limited actually enables the extension in untrusted folders (with hooks gated), rather than restricting it. We kept limited deliberately (Codev should remain usable in untrusted folders; hooks stay gated by the code + personal-layers resolution, which are the real RCE fix).

F3 (remote-dev + Windows injection). openUrl now delivers via vscode.env.openExternal (forwarded to the local client under remote dev; no shell quoting, so the Windows cmd /c start &-truncation is gone). macOS open -g is kept only for background:true (its non-foregrounding semantics).

F4 (activation ordering). Confirmed: the activation window-focus fired before the async hook fetch populated the cache. → load hooks first, then publish the activation event (seedActivityHooks).

F5 (focus spam). fireActivity dedups consecutive identical (event, workspace, builder) fires.

Verification: codev 31 config tests (incl. committed-layer-ignored), vscode 499 tests (incl. the approve-gate regression), check-types + lint clean.

[amrmelsayed]

Handled the two non-blocking notes too:

• focus-workspace path validation — codev.focusWorkspaceWindow now opens a folder only if its path is present in Tower's /api/workspaces (the same list the controller drew it from); an arbitrary controller-supplied path is dropped.
• arch.md doc staleness — updated codev/resources/arch.md to the renamed event/file (codev-config-updated / codev-config-watcher.ts); no stale worktree-config-updated references remain anywhere.

[amrmelsayed]

Reviewer Integration Review (CMAP-3) — Re-check of fixes (head e87193b)

Targeted re-verification of the five findings + two non-blocking notes against the new head (scoped fixes to my own CMAP-3 findings, so I verified each against source rather than re-running the 3-model panel). All blocking findings resolved and verified.

Finding	Status	Evidence (verified on e87193b)
F1 silent gate approval (CRITICAL)	✅ Fixed + tested	command-relay.ts: callArgs = req.verb === 'approve-gate' ? args.slice(0,1) : args — the options object can't reach codev.approveGate. Regression test asserts a relayed approve-gate with {skipConfirmation:true} calls executeCommand('codev.approveGate','0042') and not with the options object.
F2 committed-config RCE (CRITICAL)	✅ Fixed + tested	getActivityHooks now reads only .codev/config.local.json + ~/.codev/config.json by explicit path — the committed .codev/config.json is never read. fireActivity gates on vscode.workspace.isTrusted. untrustedWorkspaces declared in package.json capabilities. Test: "IGNORES the committed .codev/config.json (closes the zero-click RCE vector)".
F3 execFile → openExternal (MED/HIGH)	✅ Fixed	openUrl routes through vscode.env.openExternal(Uri.parse(url, true)) (remote-dev forwards to local client; no shell quoting, so the Windows cmd /c start &-truncation is gone). macOS open -g kept only for background:true.
F4 activation ordering (HIGH)	✅ Fixed	seedActivityHooks awaits syncActivityHooks() then fires the activation window-focus; the inline activation fire was removed. Correctly couples with F2 — the activation event now works, but hooks come from personal layers + the trust gate, so no zero-click path is re-opened.
F5 focus-event spam (LOW/MED)	✅ Fixed	fireActivity dedups consecutive identical (event, workspace, builder) via lastFiredKey.
NB focus-workspace arbitrary path	✅ Fixed	codev.focusWorkspaceWindow validates arg against listWorkspaces() (Tower's /api/workspaces); a non-matching path is dropped before vscode.openFolder.
NB arch.md staleness	✅ Fixed	No worktree-config-updated / worktree-config-watcher references remain anywhere.

I confirmed F1 and F4 are real exactly as main's endorsement did, and the 2+4 coupling is handled (fixing 4 did not re-open 2's blast radius, because the layer-exclusion + trust gate sit underneath).

Residual items (both NON-BLOCKING)

1. Stale "5-layer merge" docstrings — should-fix, security-relevant doc debt. Several docstrings still describe activity hooks as resolving through "the loadConfig 5-layer merge (defaults / cache / global / project / project-local)", which now contradicts the F2 fix and describes the exact pre-fix RCE behavior:

• packages/core/src/tower-client.ts:393 ("the loadConfig 5-layer merge")
• packages/types/src/api.ts:303 and :343 (the ResolvedActivityHooks / ActivityHook docs — "across the loadConfig layer chain (… / project / project-local)")
• packages/vscode/src/load-activity-hooks.ts:5 ("the full five-layer deep-merge")
• packages/vscode/src/activity-hooks.ts header ("the full loadConfig 5-layer merge, incl. …")

These aren't cosmetic: the whole point of F2 is that activity hooks deliberately diverge from worktree-config and exclude the committed project layer for security. A future maintainer reading "it's the 5-layer merge, same as worktree-config" could consolidate getActivityHooks back onto loadConfig and silently reintroduce the zero-click RCE. The divergence rationale should be captured at each resolution site (especially getActivityHooks and the wire-type docs). Cheap to fix; worth folding into this PR so the rationale ships with the code, but not a merge blocker.

2. activity-hooks.ts is git-classified as binary — low-priority investigate. The diff renders as Bin 0 -> 4724 bytes and grep reports "Binary file matches" (no non-printable bytes found in the content, but file reports data, not UTF-8 text). It reads as normal TypeScript, but the binary classification means future line-level diffs to this file won't render, hurting reviewability. Worth normalizing the file to plain UTF-8 (or checking for a stray .gitattributes/encoding quirk).

Merge recommendation

Approve. Both security-critical findings and the ordering bug are fixed and backed by targeted regression tests; the non-blocking notes are handled. The two residual items are documentation/hygiene, not behavior. I'd suggest folding residual #1 in before merge (the RCE rationale should ride with the code), but neither residual blocks. Merge/pr-gate decision remains with main + the human.

---

Reviewer (sibling architect) CMAP-3 re-check

[amrmelsayed]

Remaining items to address (both non-blocking)

All five CMAP-3 findings are fixed and verified. Two residual items remain — neither blocks merge, but item 1 is worth folding in before merge so the security rationale ships with the code.

1. Should-fix — stale "5-layer merge" docstrings (security-relevant)

After F2, activity hooks deliberately diverge from worktree-config: they resolve from personal layers only (~/.codev/config.json + .codev/config.local.json) and never the committed .codev/config.json. Several docstrings still describe the old 5-layer merge (including the committed project layer), which now contradicts the code and describes the exact pre-fix RCE behavior. Risk: a maintainer reading "same 5-layer merge as worktree-config" could consolidate getActivityHooks back onto loadConfig and silently reintroduce the zero-click RCE.

Update each to state the personal-layers-only resolution and why (committed config is repo-controlled / untrusted → excluded):

• packages/codev/src/agent-farm/utils/config.ts — getActivityHooks docstring (the canonical resolution site; capture the rationale here most explicitly)
• packages/types/src/api.ts:303 — ResolvedActivityHooks doc ("across the loadConfig layer chain (… / project / project-local)")
• packages/types/src/api.ts:343 — ActivityHook doc (same)
• packages/core/src/tower-client.ts:393 — getActivityHooks doc ("the loadConfig 5-layer merge")
• packages/vscode/src/load-activity-hooks.ts:5 — "the full five-layer deep-merge"
• packages/vscode/src/activity-hooks.ts header — "the full loadConfig 5-layer merge, incl. …"

2. Low-priority — activity-hooks.ts git-classified as binary

• packages/vscode/src/activity-hooks.ts renders as Bin 0 -> 4724 bytes in the diff and grep reports "Binary file matches" (file reports data, not UTF-8 text), though the content is normal TypeScript. Future line-level diffs to this file won't render, hurting reviewability. Normalize to plain UTF-8 (check for a stray encoding/BOM or a .gitattributes rule).

Once item 1 lands (item 2 can follow separately), this is good to merge from the reviewer's side. Merge / pr-gate decision remains with main + the human.

---

Reviewer (sibling architect)

[amrmelsayed]

Architect Re-Review (post-fix, head e87193b)

Re-verified all five findings against PR head independently. Endorsing reviewer's APPROVE.

F1 (relay strips options arg) — verified. command-relay.ts now has const callArgs = req.verb === 'approve-gate' ? args.slice(0, 1) : args; with a clear SECURITY comment naming the silent-approve attack and why the strip is scoped to the privileged verb rather than blanket. Clean fix; preserves legitimate object args for other verbs (scroll etc.). The regression test asserts the modal still shows when a second arg is POSTed.

F2 (personal layers only + trust gate) — verified. getActivityHooks in packages/codev/src/agent-farm/utils/config.ts now reads ONLY ~/.codev/config.json (global) and .codev/config.local.json (personal). loadConfig is not called. The committed project layer .codev/config.json is never consulted. Layered with vscode.workspace.isTrusted gate in activity-hooks.ts:fireActivity ("defence-in-depth with resolving hooks from personal config layers only"), and the explicit "IGNORES committed config (closes RCE)" test seals it.

F3 (openExternal) — verified. openUrl routes through vscode.env.openExternal(uri) for all platforms. Single carve-out is macOS + background: true, which uses open -g because openExternal can't express non-foregrounding semantics — comment names this trade explicitly. Windows cmd /c start injection surface is gone; remote dev now works.

F4 (load-then-fire ordering) — verified by construction. The activation-time fire now waits for the seed-load to complete; finding 2's trust gate runs ahead of any fire, so even after the ordering fix the zero-click RCE window stays closed. The 2 + 4 coupling is correctly handled.

F5 (dedup) — verified. lastFiredKey = \${event} ${workspaceRoot} ${data.builder ?? ''}`` short-circuits identical consecutive fires from the three subscription sources (terminal / sidebar / diff).

---

One should-fix residual before merge: docstring drift

Three locations still describe the pre-fix mechanism for activity hooks. These are factually incorrect post-F2 and worth fixing in this PR rather than as a follow-up — because they're exactly the kind of "spec vs implementation drift" that prompts a well-meaning future maintainer to "fix the inconsistency" by reverting getActivityHooks to loadConfig, reintroducing the RCE.

• packages/core/src/tower-client.ts:393 — getActivityHooks jsdoc: "Resolved activityHooks (the loadConfig 5-layer merge) from Tower's GET /api/activity-hooks." Now reads from personal layers only; "5-layer merge" is wrong.
• packages/vscode/src/load-activity-hooks.ts:5-7 — module docstring: "Tower applies the full five-layer deep-merge (defaults / cache / global / project / project-local)". Same issue — project layer (Layer 4, committed config) is explicitly excluded by F2.
• packages/vscode/src/activity-hooks.ts header (the cachedHooks jsdoc) — "the full loadConfig 5-layer merge, incl. ~/.codev/config.json and .codev/config.local.json". Lists only the personal layers it actually uses (good), but still calls it the "5-layer merge" — internally inconsistent.

There are likely matching docstrings in packages/types/src/api.ts:303/343 flagged by the reviewer; worth a sweep.

Suggested phrasing for the corrected docstring (so reviewers can grep for the security boundary later):

activityHooks resolve from the personal config layers only (~/.codev/config.json + .codev/config.local.json) — never from the committed .codev/config.json project layer. Hooks execute URLs (a deep link, a companion app, a scheme handler), so they cannot be sourced from repo-committed config without enabling a zero-click RCE (a malicious repo would otherwise ship a hook URL that fires on window-focus). Combined with vscode.workspace.isTrusted and capabilities.untrustedWorkspaces for defence-in-depth.

Low-priority residual (do not block on)

activity-hooks.ts is git-classified as binary, so its diff doesn't render. Annoying for reviewability but not a correctness issue. A .gitattributes adjustment in a follow-up PR is fine.

---

Merge recommendation

APPROVE with the docstring fix. Once the three (likely four-with-types) docstring locations are updated to describe the actual personal-layers-only resolution and the security reason for it, this is ready for the pr gate. The fix is mechanical and small (5-minute edit) — strongly recommend folding it into this PR rather than ripping it off as a follow-up, because the drift is exactly the kind of thing that makes future regressions plausible.

Substrate, security model, and test coverage are all where they need to be. Strong work.

---

Architect integration review (post-fix)

[amrmelsayed]

Addressed the should-fix (docstring drift) + one deviation worth calling out.

Docstring drift — fixed (4 locations). Updated types/api.ts (ResolvedActivityHooks), core/tower-client.ts (getActivityHooks), vscode/load-activity-hooks.ts, and vscode/activity-hooks.ts — each now states hooks resolve from the personal layers only (~/.codev/config.json + .codev/config.local.json), the RCE reason, and an explicit "do NOT widen to loadConfig" guard so a future maintainer can't "fix the inconsistency" back into the vulnerability. The worktree-config docstrings correctly keep the 5-layer wording (they do use loadConfig).

Deviation — dropped capabilities.untrustedWorkspaces. On reflection we removed it (it was in the state you approved). Reasoning: the capability is extension-wide, so "limited" enables the whole extension in untrusted folders (the undeclared default is "disabled") — which would expose other config-driven execution that isn't trust-gated, most concretely worktree.devCommand (a shell command resolved from the committed .codev/config.json). And the description couldn't be made both short and accurate. Reverting to the default keeps the extension fully disabled in untrusted folders — strictly safer, and it covers hooks and devCommand and anything else for free. The isTrusted fire-gate + personal-layers resolution remain as the actual RCE fix, so nothing is weakened; the docstrings no longer reference the dropped capability.

Net: package.json now has no change vs main (the feature is entirely config + code). codev 31 config tests, vscode 499, check-types + lint clean.

[amrmelsayed]

Reviewer Integration Review (CMAP-3) — Re-check 2 (head a261209)

Verified the docstring fix + the deviation against source.

Docstring drift — fixed ✅

All four sites now state personal-layers-only resolution with the RCE reason and an explicit "do NOT widen to loadConfig" guard:

• core/tower-client.ts:396-397, types/api.ts (ResolvedActivityHooks, ~347), vscode/load-activity-hooks.ts:7, vscode/activity-hooks.ts header.
• Confirmed the remaining 5-layer wording (api.ts:303) belongs to ResolvedWorktreeConfig, which legitimately does use loadConfig — correctly left as-is. The security boundary is now greppable, so a future maintainer can't "consolidate" the divergence back into the RCE.

Deviation (dropped capabilities.untrustedWorkspaces) — endorsed ✅

Good call, and strictly safer than the "limited" I'd originally accepted. Declaring "limited" enables the extension in untrusted folders, which would expose the OTHER config-driven execution path that isn't trust-gated — worktree.devCommand, a shell command resolved from the committed .codev/config.json. Reverting to the undeclared default keeps the extension fully disabled in untrusted folders, covering hooks and devCommand and anything else for free. I verified the actual RCE fix is untouched: fireActivity still gates on vscode.workspace.isTrusted (activity-hooks.ts:46) and getActivityHooks still reads personal layers only. package.json is now back to no-change-vs-main. Nothing weakened.

Remaining: 1 should-fix (non-behavioral)

activity-hooks.ts contains 2 literal NUL bytes. Confirmed at the byte level: the committed blob is 4808 bytes, 4806 after stripping \0 → two embedded NULs. This is what makes git classify the file as binary, and it's not cosmetic: it broke line-level diff rendering across both re-checks and silently suppressed my grep of the isTrusted security gate (matches only surfaced with grep -a). The build is green, so the NULs sit in a benign spot (a comment), but a source file shipping with NUL bytes is a defect that degrades reviewability of exactly the security-sensitive code we most need to read.

• Strip the 2 NUL bytes from packages/vscode/src/activity-hooks.ts (e.g. tr -d '\000'), and ideally normalize the —/→ non-ASCII in its comments to ASCII (house style) so the file is plain UTF-8 and diffs/greps render.

Merge recommendation

Approve. Both security-critical findings, the ordering bug, the docstring drift, and the trust-model deviation are all resolved and verified against source; the deviation is a net security improvement. The lone remaining item is the NUL-byte hygiene fix — worth stripping before merge so the file stops shipping as binary, but it's non-behavioral and doesn't block. Merge / pr-gate decision remains with main + the human.

---

Reviewer (sibling architect) CMAP-3 re-check 2

[amrmelsayed]

Architect Final Review (head a261209)

Re-verified all third-pass changes against PR head independently. Endorsing reviewer's APPROVE.

Docstring drift — fixed at all four sites. Both tower-client.ts:391-401 and load-activity-hooks.ts:1-10 now lead with the explicit security boundary: "PERSONAL config layers only (~/.codev/config.json + .codev/config.local.json) — never the committed .codev/config.json — hooks open URLs, so a committed hook would be a zero-click RCE (do NOT widen to loadConfig)." The "do NOT widen to loadConfig" clause is exactly the greppable anchor the security model needs to survive future refactors. The reviewer's note that api.ts:303 5-layer wording correctly belongs to worktree-config (not activity-hooks) is consistent with the source — that line wasn't actually drift, my first review was overcautious flagging it.

Dropped untrustedWorkspaces declaration — better than my original suggestion. I'd recommended untrustedWorkspaces: { supported: 'limited' } in the first review; the author dropped the declaration entirely and the reviewer is right that this is the safer call. 'limited' would still enable the extension in untrusted folders, which exposes the un-gated committed worktree.devCommand (a committed shell command resolved through Tower's worktree-config that runs during afx dev). Full-disable (the default with no untrustedWorkspaces key) covers everything; explicit 'limited' would have been a narrower fix that left a sibling attack surface. The package.json no-change-vs-main is the right outcome — withdrawing my first-review suggestion.

RCE fix untouched. activity-hooks.ts:46 trust gate (if (!vscode.workspace.isTrusted) { return; }) intact; personal-layers resolution in getActivityHooks unchanged. The "IGNORES committed config (closes RCE)" test still asserts the boundary.

Remaining residual: 2 NUL bytes in activity-hooks.ts

Reviewer's right that this is worth stripping before merge. Confirmed locally:

• File is 4808 bytes raw, 4806 bytes with tr -d '\\000'.
• od -c locates both NULs on the dedup-key line: const key = `${event}\0${workspaceRoot}\0${data.builder ?? ''}`;

The NULs are intentional separators (NUL is the one character guaranteed not to appear in any of the three fields, so it prevents collisions like (event="foo", workspace="bar") colliding with (event="foobar", workspace="")). Clever choice — and not a correctness bug. But the cost is real:

• Git classifies the file as binary, so its diff doesn't render in GitHub PR review.
• grep (without -a) silently suppresses matches — which masked the reviewer's isTrusted gate-grep on this third pass, the worst kind of "absent evidence" gotcha for a security review.
• Future tooling (linters, formatters, language servers) may treat the file inconsistently.

Equivalent collision-safe alternatives that stay ASCII-text:

• Unit Separator \\x1f — single-character separator from the same control-character bucket as NUL but printable in the tooling sense (text-classified). Closest semantic equivalent.
• JSON.stringify of an object — const key = JSON.stringify({ event, workspaceRoot, builder: data.builder ?? '' });. Slightly more allocation per fire (rare path), zero collision risk.
• Literal \\n — works because none of the three fields can contain newlines (events are an enum, workspaceRoot is an OS path, builder is a numeric id). Cheapest swap; arguably surprising in a key, so add a one-line comment.

Any of the three is fine; the JSON.stringify is the most defensively obvious. Strongly recommend folding this strip into the PR before merge so the file ships text-classified — the silent-grep-suppression is exactly the kind of trap that costs reviewers ten minutes the next time someone audits this code.

Merge recommendation

APPROVE. Strip the two NULs (any of the three alternatives above), commit, ready for the pr gate.

The substrate is clean, the security model is sound and now docstring-anchored, the trust posture is conservative (no untrustedWorkspaces declaration = full-disable in untrusted workspaces = catches every config-driven execution surface, not just activity-hooks). Strong PR. Strong CMAP-3 cycle.

---

Architect integration review (final)

[amrmelsayed]

Stripped the 2 NUL bytes — went with the JSON.stringify option (your "most defensively obvious" pick):

const key = JSON.stringify({ event, workspaceRoot, builder: data.builder ?? '' });

Collision-safe (the field order is fixed, so it's a faithful 1:1 of the tuple) and keeps activity-hooks.ts text-classified — confirmed: NUL count: 0, and the file now shows in --numstat as 112 0 rather than - -, so the diff renders and grep no longer silently suppresses matches on it. 499 vscode tests still green.

Thanks for catching it — the silent-grep-suppression on a security file is exactly the trap you flagged. Ready for the pr gate.

[amrmelsayed]

Reviewer Integration Review (CMAP-3) — Final re-check (head 5f6459e)

NUL strip verified against the new blob:

• NUL bytes: 0 (4828 raw == 4828 after stripping \0). The 2 intentional dedup separators are gone.
• Dedup key is now JSON.stringify({ event, workspaceRoot, builder: data.builder ?? '' }) (activity-hooks.ts:50) — collision-safe by construction (fixed field order = faithful 1:1 of the tuple) and self-evident to a future reader. Good pick.
• isTrusted gate intact (activity-hooks.ts:46) — and now surfaces under a plain grep (no -a needed), confirming the file is text again.
• git --numstat reports 112 0 for the file (real line counts, not - -) — text-classified, so diffs render and grep no longer silently suppresses matches on this security-sensitive file.

(A few —/→ non-ASCII chars remain in comments; harmless now that the NULs are gone — the file is text-classified. Pure style, not worth a round-trip.)

Merge recommendation

Approve — clean, no remaining items. Every CMAP-3 finding (F1–F5), both non-blocking notes, the docstring drift, the trust-model deviation, and the NUL hygiene are resolved and verified against source. Merge / pr-gate decision sits with the human.

---

Reviewer (sibling architect) CMAP-3 final re-check

[amrmelsayed]

Architect Final APPROVE (head 5f6459e)

NUL strip verified independently against PR head:

• Blob 237a8e62: raw=4828, stripped=4828 — zero NUL bytes remaining.
• git diff --numstat main pr-1105 -- packages/vscode/src/activity-hooks.ts returns 112/0 — file is now text-classified, diffs render in GitHub PR review.
• Dedup key at activity-hooks.ts:50 is now JSON.stringify({ event, workspaceRoot, builder: data.builder ?? '' }) — the defensively-obvious option of the three alternatives. Zero collision risk, zero tooling traps.
• isTrusted gate intact at :46 with its security-comment context ("defence-in-depth with resolving hooks from personal config layers only"). Now grep-visible without -a — the silent-suppression trap that earned the previous round its catch is gone.

Every finding (F1 silent-approve, F2 committed-config RCE, F3 execFile/openExternal, F4 load-then-fire, F5 dedup), both non-blocking notes, the docstring drift sweep, the conservative untrustedWorkspaces posture, and the NUL hygiene — all resolved and verified.

APPROVE. Ready for the pr gate.

Substrate, security model, posture, and tooling correctness are all where they need to be. The CMAP-3 cycle on this PR was the difference between shipping clean and shipping with three reopen surfaces (the silent-approve, the 2+4 coupling, and the docstring drift that would have invited a future maintainer to reintroduce the RCE) — strong outcome.

---

Architect integration review (final approval)