[akramcodez]

#628

[github-actions]

nc-review: needs work — 1 blocking, 4 important, 1 nit

@akramcodez — there is a blocking item below.

Massive PR (~8191 diff lines, 43 files) adding a localhost browser web mode plus a grab-bag of 'runtime reliability' fixes, with the description only stating '#628' and not actually closing the issue. The architecture mostly matches the existing codebase, and most of the new code is correctly wired, but the AcpInitContext public interface is extended with a customCommandExecutor field that no consumer in the diff or repository reads — dead code on a contract surface. The size and lack of focused spec also warrant a maintainer conversation per CONTRIBUTING.

🔴 blocking · correctness · source/acp/acp-types.ts:10

The customCommandExecutor: CustomCommandExecutor field added to AcpInitContext is never read anywhere. The PR also adds the executor to PlainInitResult (source/plain/initialize.ts) and threads it through acp-server.ts, but AcpAgent only consults initContext.customCommandLoader, never initContext.customCommandExecutor. This is a dead field on a public contract surface the project rubric calls out (AcpInitContext is consumed by external AcpAgent implementations), which means every future ACP server must now construct a CustomCommandExecutor and pass it through for nothing.

Either wire the executor into AcpAgent (the obvious consumer — it already runs custom commands via the loader, but does not currently invoke them through an executor) and update the ACP conversation loop to use it, or drop both the interface field and the construction in initializePlain until a real consumer exists.

🟠 important · warranted · source/cli.tsx:932

The PR is ~8191 lines across 43 files: a brand-new localhost browser surface (source/web/*, ~7 new files including a 2800-line HTML/JS page) plus seven separate 'web-mode runtime reliability' fixes (delivery, offline startup, queues/cancellation, reconnect, multi-tab, session races, image handling, tool errors) bundled into the same PR. CONTRIBUTING explicitly asks contributors to draft a spec and discuss substantial changes in an issue first. The PR description is just #628 (which the metadata says is not actually closed) — there is no spec to read against.

Per the project rubric this is the most likely kind of 'small PRs are welcome, but undiscussed changes this size are not' case. Splitting this into (a) the web-mode UI itself and (b) one PR per reliability fix would let each be reviewed against a known reproduction. The reliability fixes in particular should each have an issue or at minimum a repro in the PR body.

🟠 important · design · source/web/page.ts:1

source/web/page.ts is a 2806-line file that contains an entire client-side SPA as a single template literal (CSS + SVG icons + JS), and the browser spec (source/web/browser.spec.ts) drives it by runInContext-ing the embedded script in node:vm. vm does not implement DOM events, CSS, WebSocket lifecycle, or focus / pointer interactions, so the tests are exercising a text-only smoke harness: socketListeners.set('close', callback) replaces WebSocket, document.createElement is a thin wrapper, and scrollIntoView is a no-op. Real-browser bugs (CSS layout, focus management, scrollback races, theme attribute propagation) cannot be caught by this harness. The reliability claims in the changeset ('reply delivery, reconnect, multi-tab state') all live in this layer, and the tests prove that the embedded script does not throw, not that the browser behaviour is correct.

If the project keeps web mode, the page logic should be split (CSS in one block, prose in another) and the runtime surface should be exercised against an actual headless browser, or at minimum jsdom + a websocket stub. As written the test suite will go green on a build that is broken on first click.

🟠 important · tests · source/hooks/chat-handler/conversation/tool-executor.spec.ts:16

The new 'web lifecycle reports structured tool failures as failed' test claims to verify that executeApprovedTool reports failed runs to the web publisher. The publisher is a module-global (set via setWebToolLifecyclePublisher) so it can be mutated by any other test running in the same process; AVA runs serially but the module-level state survives across tests and there is no test.beforeEach reset. If a future test calls setWebToolLifecyclePublisher(...) without t.teardown(() => setWebToolLifecyclePublisher(null)), this test will silently report whatever it set instead of the failure under test. Either reset the publisher in test.beforeEach (or wrap it in a withWebToolLifecyclePublisher(publisher, fn) helper), or scope it per-test via injection rather than a module global.

🟠 important · correctness · package.json:37

"dev": "tsc --watch & tsc-alias --watch" runs tsc-alias in the background with &. On macOS / Linux pnpm uses /bin/sh, where the trailing & backgrounds tsc-alias and pnpm exits as soon as tsc exits, leaking tsc-alias as an orphan and breaking path-aliased imports in any subsequent run that depends on them. The previous one-line tsc --watch had no such leak. Run them in series with concurrently (already in the dependency tree via lint-staged/pnpm's own deps — check whether it is a direct dep) or use npm-run-all, and clean up both children on SIGINT.

⚪ nit · completeness · .changeset/calm-web-world.md

The changeset entry says 'Add browser approvals, questions, and live tool status to local web mode so coding-agent turns can finish safely in the browser UI', but package.json also adds a new pnpm web script and a new dev script. These are user-facing (the pnpm web script is what the changeset implies people should use), so the changeset ought to mention them, or each gets its own entry.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}