Four inline + one outside-diff actionable comment from the second CodeRabbit
review on claude-code-best/claude-code#386:
- tests/mocks/auth.ts: align mock return contracts with src/utils/auth.ts.
checkAndRefreshOAuthTokenIfNeeded resolves to a Promise<boolean> and
getClaudeAIOAuthTokens returns the full token shape (refreshToken, expiresAt,
scopes, subscriptionType, rateLimitTier) so tests that branch on these
values can not silently drift away from production.
- src/utils/handlePromptSubmit.ts (461-468): clear the freshly-published
abortController before the early return when every claimed autonomy command
was skipped as non-consumable, so this turn's stale controller does not leak
into the next turn.
- src/utils/handlePromptSubmit.ts (621-649): separate execution failure from
finalizer failure. The turn body now writes to a `turnError` slot; a single
pass after the inner try decides whether to finalize claimed commands as
`completed` or `failed`, with each finalize call wrapped in its own
try/catch so a failure inside finalize does not flip a successful turn into
`failed` and double-finalize the same commands. The outer catch only
rethrows the original turn error.
- src/utils/processUserInput/processSlashCommand.tsx (228-276): wrap the
post-success `finalizeDeferredAutonomyRunCompleted()` call in its own
try/catch so a finalize failure no longer falls into the worker-failure
catch path and emits a contradictory `<scheduled-task-result status="failed">`
for a slash command that actually succeeded.
Outside scope (not changed) — the CodeRabbit suggestion to add a `.ts`
extension to the shared `tests/mocks/auth` import contradicts the project's
existing convention: every other test imports the shared mocks without the
extension (e.g. `tests/mocks/log`, `tests/mocks/debug`,
`tests/mocks/file-system`), and the project's tsconfig does not enable
`allowImportingTsExtensions`, so adding the extension fails typecheck. The
import is kept extension-less to match the rest of the suite.
Validation:
- bun run typecheck (clean).
- bun test → 3996 pass / 0 fail across 305 test files.
Twelve actionable items (7 Major + 5 Minor) from the CodeRabbit review on
claude-code-best/claude-code#386:
- docs/internals/autonomy-jira.md: typo "due input close" → "due to input close".
- src/utils/autonomyRuns.ts:
- selectPersistedAutonomyRuns no longer evicts active (queued/running) runs
when the combined list exceeds AUTONOMY_RUNS_MAX. Active runs are kept in
full and the inactive history is capped to the remaining budget so
persisted ownership for live work survives.
- isValidOwnerProcessId now allows pid <= 4_194_304 so a live run owned by
the maximum Linux PID is not treated as stale.
- src/utils/autonomyAuthority.ts: maskCodeFencedLines tracks the active fence
length and only closes the fence when a same-character run of equal-or-
greater length appears with no trailing content, so a nested ```yaml inside
an outer ```` block no longer leaks fake `tasks:` entries into the parser.
- src/cli/print.ts: late-shutdown branches in the cron and scheduled-task
paths now call cancelQueuedAutonomyCommands({ commands: [command] }) instead
of markAutonomyRunCancelled(...). Updating run state alone left the
queue-side record orphaned for resume/recovery.
- src/utils/processUserInput/processSlashCommand.tsx: scheduled-task-result
notification is enqueued before finalizeAutonomyRunCompleted (which queues
follow-up autonomy commands) so both at priority: 'later' land in order and
the next autonomy step can not run before the worker's output is observed.
- src/screens/REPL.tsx + src/utils/handlePromptSubmit.ts:
- onQuery now returns Promise<boolean>: false from the concurrent-guard
skip path, true otherwise. Other call sites use `void onQuery(...)` and
are unaffected. handlePromptSubmit's onQuery prop type matches.
- The autonomy-prompt callsite captures the executed flag, finalizes
claim.claimedCommands as { type: 'completed' } only when onQuery actually
ran, and runs the completed-finalize in its own try/catch so a failure
there does not propagate into the outer catch and trigger a second
finalize as { type: 'failed' } for the same commands.
- Removed the unsafe `command.value as string` cast; createUserMessage
already accepts `string | ContentBlockParam[]`.
- createUserMessage mock in src/__tests__/handlePromptSubmit.test.ts now
matches the new Promise<boolean> shape.
- packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/
RemoteTriggerTool.test.ts:
- Inline auth mock replaced with the shared tests/mocks/auth (added).
- The full mock of src/constants/oauth.js is replaced by a narrow
side-effect-only mock that overrides the env-reading helpers
(getOauthConfig, fileSuffixForOauthConfig, MCP_CLIENT_METADATA_URL) and
delegates pure data exports to the real module.
- tests/integration/dependency-overrides.test.ts:
- mermaid does not export `./package.json` in its exports map, so
require.resolve('mermaid/package.json') throws
ERR_PACKAGE_PATH_NOT_EXPORTED in runtimes that honor exports semantics.
The test now resolves the package entry and walks up to the package
root via a small findPackageJson helper.
- readFileSync from node:fs is replaced with `await Bun.file(...).text()`
to match the project's Bun-API requirement.
Validation:
- bun run typecheck (clean).
- bun test → 3996 pass / 0 fail across 305 test files.
Targets PRs:
- amDosion/claude-code-bast#8 (fork-internal review)
- claude-code-best/claude-code#386 (upstream review, same head branch)
This PR consolidates a coordinated batch of fixes around autonomy run/flow lifecycle, scheduled task deduplication, provider-boundary state finalization, and matching memory-bound treatments for adjacent long-running subsystems (REPL fullscreen scrollback, skill-search/skill-learning runtime activation). All changes were developed and reviewed together because they touched the same lifecycle invariants and were uncovered by the same long-running session reproductions.
## Lifecycle correctness
- Queued autonomy prompts are not injected unless the persisted run was successfully claimed; queued run claiming is now terminal-safe so a once-consumed/cancelled/failed run can not slip back into `queued`.
- Autonomy run/flow finalization happens on completion, provider error, generator close, and cancellation — not just the happy path. New `src/__tests__/queryAutonomyProviderBoundary.test.ts` covers these provider-boundary transitions.
- `requestManagedAutonomyFlowCancel` and `resumeManagedAutonomyFlowPrompt` carry `rootDir` and `currentDir` explicitly across detached async boundaries (proactive-tick, cron, daemon restart) instead of inferring from process state.
- Active runs/flows are protected from janitor pruning so a running step can not be garbage-collected mid-flight (`src/utils/autonomyAuthority.ts`).
- Heartbeat parser now ignores fenced code blocks; the two-phase commit window for autonomy state transitions is documented in `docs/internals/autonomy-jira.md`.
## Ownership and dedup
- `src/utils/autonomyRuns.ts`: ownership stamping (run id + rootDir carried end-to-end), source-based dedup against active runs.
- `src/hooks/useScheduledTasks.ts`: scheduled ticks deduplicate against runs already active on the same source label.
- `src/utils/processUserInput/processSlashCommand.tsx`: forked slash commands now thread the autonomy `runId` so completion finalizers can find the originating run for deferred completion.
- New `src/utils/autonomyQueueLifecycle.ts` and tests collect the queue-side lifecycle invariants in one place.
## Memory bounds (related, same review pass)
- `src/screens/REPL.tsx`: caps fullscreen scrollback after the compact boundary and updates trailing progress rows in place. Long-running fullscreen sessions could otherwise retain thousands of post-compaction messages and duplicate progress rows, keeping Ink trees alive long after their useful context had moved on.
- `src/services/skillSearch/*` and `src/services/skillLearning/*`: runtime activation is strictly opt-in via existing env toggles; session caches are capped so long-running processes can not grow them forever. Build presence is preserved so operators can still discover and opt into the slash commands.
## CI / test contract
- `tests/integration/dependency-overrides.test.ts`: smoke test no longer drives Mermaid's browser renderer; it validates the package-resolution contract directly so CI does not regress on unrelated browser timing.
- New `tests/integration/autonomy-lifecycle-user-flow.test.ts`: end-to-end CLI subprocess flow exercising `status --deep`, `flows`, `flow <id>`, `flow resume`, `flow cancel` against persisted state.
- `src/entrypoints/cli.tsx`: `claude autonomy …` routes through an entrypoint fast path that reuses the slash-command formatter without booting the full interactive CLI. Stdout is flushed before forced exit so coverage subprocesses do not terminate with empty stdout.
- `packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/RemoteTriggerTool.test.ts`: stabilized to prevent audit flake under coverage.
## Tests added
- `src/__tests__/queryAutonomyProviderBoundary.test.ts`
- `src/hooks/__tests__/useScheduledTasks.test.ts`
- `src/utils/__tests__/autonomyAuthority.test.ts`
- `src/utils/__tests__/autonomyFlows.test.ts` (extended)
- `src/utils/__tests__/autonomyPersistence.test.ts` (extended)
- `src/utils/__tests__/autonomyQueueLifecycle.test.ts`
- `src/utils/__tests__/autonomyRuns.test.ts` (extended)
- `src/utils/processUserInput/__tests__/processSlashCommand.test.ts`
- `tests/integration/autonomy-lifecycle-user-flow.test.ts`
## Docs
- `docs/agent/sur-loop-scheduled-oom.md`: System Understanding Report covering the scheduled/loop OOM problem, the call graphs investigated, and the lifecycle invariants this PR establishes.
- `docs/agent/sur-skill-overflow-bugs.md`: SUR for the related skill-overflow context.
- `docs/internals/autonomy-jira.md`: documents the two-phase commit window and ownership stamping invariants.
- `docs/memory-leak-audit.md`: audit notes covering the REPL/scrollback and skill-search bounds.
## Invariants this PR establishes
1. Queued autonomy prompts are not injected unless the persisted run was successfully claimed.
2. Terminal run/flow states are terminal — completion, failure, and cancellation all finalize state regardless of which provider/error path triggered them.
3. Autonomy run/flow `rootDir` is carried explicitly across detached async boundaries instead of inferred from a shared singleton.
4. State-only CLI subcommands (`autonomy status|runs|flows|flow …`) bypass full interactive bootstrap so they do not hold unrelated handles open.
5. REPL fullscreen scrollback and skill-search/skill-learning session caches are explicitly bounded.
## Validation
```bash
bun run typecheck
CI=true GITHUB_ACTIONS=true bun test # 3996 pass / 0 fail across 305 files
bun test src/__tests__/queryAutonomyProviderBoundary.test.ts \
src/hooks/__tests__/useScheduledTasks.test.ts \
src/utils/__tests__/autonomy{Runs,Flows,Authority,QueueLifecycle,Persistence}.test.ts \
src/utils/processUserInput/__tests__/processSlashCommand.test.ts \
tests/integration/autonomy-lifecycle-user-flow.test.ts
```
## Origin
This PR is the consolidated, upstream-targeted version of two fork-side review PRs (fix/loop-scheduled-autonomy-oom and fix/autonomy-lifecycle). The fork-side review history is preserved at https://github.com/amDosion/claude-code-bast/pull/7 . The fork's own internal `chore: keep fork current with upstream` sync commits and the `docs: update contributors` automation are intentionally not included in this PR.
The autonomy CLI handler `rootDir` threading that the fork added (78f64d8a, 98d04ddb) is intentionally omitted here because upstream `a2cfaf91` (fix: 修复 RemoteTriggerTool 和 autonomy 测试的全量运行失败) already performed the equivalent change with an additional `currentDir` option. Keeping the upstream version avoids regressing that improvement.
* fix: keep UDS peer failures structured
CodeRabbit and Claude cross-review identified that timeout and raw peer connection failures should share one observable error contract. UDS peer failures now use UdsPeerConnectionError consistently, and connectToPeer hands the socket lifecycle back to the caller after a successful connection instead of retaining an internal timeout or error listener.
The tests cover the real socket paths with capability files, timeout behavior, connection failure structure, post-connect listener handoff, AgentSummary rescheduling observations, and platform-specific mailbox directory errno handling.
Constraint: Preserve the 5000ms production timeout default while allowing tests to exercise timeout paths quickly.
Rejected: Suppress CodeRabbit warnings in tests | would hide the real timeout/error contract gap.
Rejected: Keep connectToPeer post-connect error listener | it would silently swallow caller-owned socket errors.
Confidence: high
Scope-risk: narrow
Directive: Keep UDS send/connect timeout and socket-error paths on the same structured peer error contract.
Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Tested: omx ask claude simplify review artifact .omx/artifacts/claude-review-only-cross-check-for-pr-374-on-branch-codex-codecov-r-2026-04-27T08-17-47-309Z.md
Tested: omx ask claude security review artifact .omx/artifacts/claude-security-review-cross-check-for-pr-374-current-working-tree--2026-04-27T08-26-54-079Z.md
Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
* docs: clarify UDS peer socket ownership
CodeRabbit's #375 pass found that connectToPeer now correctly hands socket errors to the caller, but the JSDoc needed to spell out that contract. The lifecycle test also uses a less brittle post-connect timeout so slow CI does not turn the ownership check into a connection-speed race.
Constraint: The raw socket API intentionally detaches its internal listener after successful connect so caller-owned errors are not swallowed.
Rejected: Keep the test timeout at 50ms | it tests scheduler speed instead of socket lifecycle ownership.
Confidence: high
Scope-risk: narrow
Directive: connectToPeer callers must attach their own error listener immediately after awaiting the socket.
Tested: bun test src/utils/__tests__/udsMessaging.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: git diff --check
Tested: bun run test:all
Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
* fix: close peer socket listener handoff window
CodeRabbit and Claude review found that documenting caller-owned raw socket errors still left a Promise handoff window and a stale timeout-listener risk. The peer connection API now requires a caller error handler and installs it before resolving, while cleanup removes internal error and timeout listeners on every path.
Constraint: Keep the fix precise to PR #375 review feedback and avoid warning suppression or fallback behavior.
Rejected: Leave the behavior documented only | still permits an unhandled socket error window between resolve and caller listener attachment.
Rejected: Keep a no-op internal error listener | would silently swallow caller-owned socket errors.
Confidence: high
Scope-risk: narrow
Directive: Do not add raw connectToPeer callers without providing a real onSocketError handler and capability handshake.
Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Tested: bun audit
Not-tested: Manual external ACP peer runtime beyond repository tests.
* fix: use a deadline timer for peer connects
The raw socket handoff no longer needs Socket#setTimeout; an ordinary connection deadline keeps the timeout behavior while avoiding an internal socket timeout listener that has no reliable UDS integration path to exercise.
Constraint: Keep Codecov coverage honest without adding ignore pragmas, mocks, or fallback suppression.
Rejected: c8 ignore on the timeout listener | hides the uncovered branch instead of simplifying the lifecycle.
Rejected: keep Socket#setTimeout listener | leaves a socket listener lifecycle to manage for a connect-only deadline.
Confidence: high
Scope-risk: narrow
Directive: Keep connectToPeer errors caller-owned via onSocketError and reject pre-connect failures with UdsPeerConnectionError.
Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun test src/utils/__tests__/udsMessaging.test.ts --coverage --coverage-reporter lcov --coverage-dir coverage-uds
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Tested: bun audit
Not-tested: Manual external ACP peer runtime beyond repository tests.
---------
Co-authored-by: unraid <local@unraid.local>
* test: keep Codecov coverage on real agent communication paths
PR #369 was merged before the final Codecov coverage fix landed, so this follow-up carries only the incremental real-path tests needed on top of main. The tests exercise AgentSummary lifecycle branches, mailbox fail-closed behavior, UDS client connection failure through a real capability file, and UDS response-reader framing without mock.module, warning suppression, feature fallback, or production-code churn.
Constraint: PR #369 is already merged; this branch must contain only the incremental Codecov repair on top of latest main
Rejected: Reopen or keep pushing the merged PR branch | merged PR refs do not update and would leave Codecov stale
Rejected: Mock bun:bundle or hide warnings | would reintroduce cross-test pollution and pseudo coverage
Rejected: Keep unrelated SendMessageTool production diff | it created avoidable patch-coverage debt without improving the runtime path
Confidence: high
Scope-risk: narrow
Directive: Keep these coverage tests on real paths; do not replace them with output suppression or feature-flag mocks
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun test src\utils\__tests__\teammateMailbox.test.ts
Tested: bun test src\services\AgentSummary\__tests__\agentSummary.test.ts src\services\AgentSummary\__tests__\summaryContext.test.ts src\utils\__tests__\teammateMailbox.test.ts src\utils\__tests__\udsMessaging.test.ts src\utils\__tests__\udsResponseReader.test.ts packages\builtin-tools\src\tools\SendMessageTool\__tests__\udsRecipientSanitization.test.ts
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Tested: bun audit
Tested: git diff --check
Tested: Claude simplify review GO (.omx/artifacts/claude-simplify-codecov-20260427-1521.md)
Tested: Claude security review GO (.omx/artifacts/claude-security-codecov-20260427-1522.md)
Not-tested: GitHub-hosted Codecov upload after this amended commit until PR checks rerun
* test: keep review assertions tied to real failure paths
CodeRabbit flagged three non-blocking but valid review gaps: platform-specific mailbox errno checks, brittle UDS connection-failure message assertions, and missing AgentSummary reschedule proof after fork errors. This keeps the fixes narrow by tightening the affected assertions and adding a structured UDS connection error for tests to assert behavior instead of prose.
Constraint: PR #374 is a review follow-up and must not hide warnings, skip tests, or merge the PR.
Rejected: Matching the UDS failure message literal | preserves the brittle coupling CodeRabbit flagged.
Rejected: Asserting only that mailbox writes throw | would allow unrelated pre-path failures to pass.
Confidence: high
Scope-risk: narrow
Directive: Keep UDS connection-failure tests on structured error data, not display wording.
Tested: bun test src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.ts src/utils/__tests__/udsMessaging.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
* test: remove brittle review follow-up assumptions
CodeRabbit's second pass found two valid brittleness issues and one suggested callback-reference assertion that would not match production behavior. This keeps the production behavior unchanged: timers still schedule the summarizer closure, tests now assert timer-handle identity, and UDS connection errors use native Error.cause instead of shadowing it.
Constraint: Do not manufacture behavior just to satisfy a review hint; assertions must match the real AgentSummary scheduling contract.
Rejected: Assert a fresh scheduled callback function | scheduleNext intentionally passes the same runSummary closure each time.
Rejected: Store a custom cause field on UdsPeerConnectionError | native Error.cause is available under ESNext/Bun.
Confidence: high
Scope-risk: narrow
Directive: Timer tests should assert returned handle identity for ownership, not incidental numeric values.
Tested: bun test src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/udsMessaging.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
* test: enforce structured UDS timeout failures
CodeRabbit's follow-up surfaced a real consistency gap: UDS send socket errors used UdsPeerConnectionError while response timeouts still rejected a generic Error. Timeouts now use the same structured peer failure contract, and the test exercises that path through a short explicit timeout instead of waiting for the production default.
The AgentSummary unchanged-fingerprint test now also asserts that the second unchanged tick does not log errors, preserving the existing behavior checks without changing production scheduling semantics.
Constraint: Keep the production timeout default at 5000ms while allowing tests to exercise the timeout path quickly.
Rejected: Leave timeout failures as generic Error | callers would need separate handling for the same peer connection failure class.
Confidence: high
Scope-risk: narrow
Directive: Keep UDS send timeout and socket-error branches on the same structured error contract.
Tested: bun test src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/udsMessaging.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run build
Tested: bun run build:vite
Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
---------
Co-authored-by: unraid <local@unraid.local>
* fix: bound agent communication memory growth
UDS messaging now uses private local capabilities instead of exposing auth tokens through SDK metadata, environment variables, session registry, peer listing, or tool output. The receive path bounds NDJSON frames, response buffers, active clients, and pending inbox bytes, and strips auth metadata before messages enter the prompt queue.
Teammate mailboxes now validate file and message sizes, fail closed on corrupt mutation inputs, compact by count and retained bytes, and use stable message identity for in-process acknowledgements. Agent summaries now fork only a bounded recent context using lazy size estimation and content fingerprints instead of retaining or serializing unbounded histories.
Constraint: PR #361 was already merged; this branch is based on upstream/main@c2ac9a74.
Rejected: Default-disabling COORDINATOR_MODE/TEAMMEM only | explicit feature enablement still hit unbounded paths.
Rejected: Persisting UDS auth in SDK/env/session registry | bridge/remote metadata can leak local capability secrets.
Rejected: Inline uds #token addresses | observable/tool/classifier paths can reflect raw addresses outside the UDS request frame.
Rejected: Positional mailbox marking after compaction | compaction can shift indices across the lock boundary.
Confidence: high
Scope-risk: moderate
Directive: Do not expose UDS capability tokens through SDK messages, environment variables, session registry, peer-list output, or SendMessage result/classifier surfaces.
Directive: Do not reintroduce positional mailbox acknowledgements unless compaction is removed or read+mark is atomic under one lock.
Tested: bun test src/utils/__tests__/ndjsonFramer.test.ts src/utils/__tests__/udsMessaging.test.ts packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bunx biome lint modified src/package files
Tested: bun run test:all (3704 pass, 0 fail, 6734 expects)
Tested: bun audit (No vulnerabilities found)
Tested: bun run build
Tested: bun run build:vite
Tested: git diff --check
Not-tested: End-to-end external UDS client driving a full production headless model turn.
* fix: harden bounded agent communication review fixes
CodeRabbit and Codecov surfaced real gaps in UDS framing, peer discovery, mailbox retention, and summary context coverage. This tightens those paths without suppressing review or coverage signals.
Constraint: PR #369 must address CodeRabbit and Codecov findings without warning suppression or fake fallbacks
Rejected: Suppress Codecov or CodeRabbit warnings | leaves real receive-path and test-isolation gaps
Rejected: Add unreachable feature-gated tests | bun:bundle keeps those branches compile-time gated in local tests
Confidence: high
Scope-risk: moderate
Directive: Keep UDS auth-token rejection outside feature flags; do not reintroduce inline token fallbacks
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage; bun run test:all; bun run lint; bun run build; bun run build:vite; bun audit; git diff --cached --check
Not-tested: Remote Codecov/CodeRabbit refreshed reports until pushed
* fix: prevent agent communication bounds from hiding CI regressions
Tighten the UDS auth, framing, and response-reader boundaries while keeping the AgentSummary lifecycle covered so Codecov and CI fail on real regressions instead of missing coverage. The poorMode settings mock mirrors unrelated real settings defaults to avoid Bun mock retention changing later permission tests.
Constraint: PR #369 must fix Codecov/CI precisely without warning suppression, fallback masking, or mock pollution
Rejected: Delete AgentSummary lifecycle coverage | would hide Codecov loss and stale-summary behavior
Rejected: Store inline UDS rejection in a hidden input sentinel | cloned observable inputs can drop it and bypass rejection
Rejected: Ignore malformed UDS frames until timeout | leaves client slots and SendMessage calls open to exhaustion
Confidence: high
Scope-risk: moderate
Directive: Keep empty #token= markers rejected; do not require a non-empty token value in hasInlineUdsToken
Tested: bun test packages/builtin-tools/src/tools/SendMessageTool/__tests__/udsRecipientSanitization.test.ts src/utils/__tests__/udsMessaging.test.ts src/utils/__tests__/udsResponseReader.test.ts src/utils/__tests__/ndjsonFramer.test.ts
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage
Tested: bun run test:all
Tested: bun audit
Tested: bun run build
Tested: bun run build:vite
Not-tested: GitHub-hosted Codecov upload until pushed PR checks rerun
---------
Co-authored-by: unraid <local@unraid.local>
* fix: harden ACP communication boundaries
Harden ACP communication boundaries
Remote ACP sessions now cannot widen permission mode through untrusted
metadata or client payloads. WebSocket ACP ingress measures payloads by bytes
before binary decode, and prompt queue handoff keeps exactly one prompt active
while queued prompts are drained FIFO.
Constraint: ACP remote clients must not be able to open bypassPermissions without local launch intent
Constraint: WebSocket payload limits must be byte-based and checked before binary decode
Rejected: Keep promptToQueryContent wrapper | no production consumers remained after prompt conversion single-sourcing
Confidence: high
Scope-risk: moderate
Directive: Do not re-enable remote bypassPermissions from _meta unless a local launch gate is verified in both acp-link and agent
Tested: targeted ACP/RCS/acp-link prompt queue, bridge, permission, payload, and prompt conversion tests; bun run typecheck; bun run build
Not-tested: Manual live ACP/RCS session against an external client
* fix: restore repository verification gates
Keep the full repository test, typecheck, build, and Biome lint gates usable
after the ACP fix pass. This commit is intentionally separate from the ACP
behavior change: it fixes Windows-safe Langfuse home redaction, removes stale
lint suppressions, resolves Biome warning/info diagnostics, and keeps env
expansion tests explicit without template-placeholder lint noise.
Constraint: The project completion contract requires full typecheck, lint, test, and build evidence
Rejected: Leave warning/info diagnostics as historical noise | they obscure future gate regressions and weaken flow-impact claims
Confidence: high
Scope-risk: narrow
Directive: Keep repository gate cleanup separate from feature fixes when it is not part of the same runtime path
Tested: bunx biome lint src/; bunx tsc --noEmit; bun test src/services/mcp/__tests__/envExpansion.test.ts src/utils/__tests__/sliceAnsi.test.ts src/utils/__tests__/stringUtils.test.ts; bun test; bun run build
Not-tested: Manual Langfuse export against a real external Langfuse service
* fix: harden ACP failure boundaries after review
Deep review found several paths that made ACP communication failures look normal: prompt errors could finish as end_turn, permission pipeline exceptions could fall through to client approval, tool rawInput was deep-copied with JSON, and acp-link accepted unbounded or unvalidated WebSocket payloads. This keeps the behavior fail-closed, validates WS payloads before dispatch, caps payload size before JSON parse, and preserves cancellation intent with a generation counter.
Constraint: User explicitly rejected pseudo-fixes, fallback behavior, and unbounded payload handling
Rejected: Keep JSON stringify/parse rawInput copy | duplicates large payloads and silently drops non-JSON inputs
Rejected: Delegate permission pipeline errors to client approval | allows a broken local permission check to be bypassed
Confidence: high
Scope-risk: moderate
Directive: Do not convert ACP errors into normal end_turn responses without a protocol-level reason and regression tests
Tested: bun test src/services/acp/__tests__/agent.test.ts src/services/acp/__tests__/bridge.test.ts src/services/acp/__tests__/permissions.test.ts
Tested: bun test packages/acp-link/src/__tests__/server.test.ts
Tested: bunx tsc --noEmit
Tested: bunx biome lint src/ packages/acp-link/src/
Tested: bun run test:all
Tested: bun run build
Not-tested: Manual end-to-end ACP client session over a real editor WebSocket
* fix: prevent ACP coverage runs from seeing partial mocks
GitHub Actions failed under bun test --coverage because permissions.test.ts replaced ../bridge.js with a partial mock that omitted forwardSessionUpdates. Coverage worker ordering on Linux let sibling tests observe that incomplete module.
This isolates ACP test mocks by snapshotting real exports, overriding only requested symbols, and restoring mocks in LIFO order. The shared helper also keeps the same behavior in agent.test.ts without duplicating mock infrastructure.
Constraint: bun:test mock.module is process-global inside a worker.
Rejected: Add fallback exports or production guards | the bridge export exists; the failure was test mock pollution.
Rejected: Keep per-file helper copies | duplication would let restore semantics drift again.
Confidence: high
Scope-risk: narrow
Directive: Prefer safeMockModule for partial mocks of real modules in ACP tests; plain mock.module is only appropriate for fully synthetic modules or isolated tests.
Tested: bun test src/services/acp/__tests__/agent.test.ts src/services/acp/__tests__/bridge.test.ts src/services/acp/__tests__/permissions.test.ts
Tested: bun test --coverage --coverage-reporter=lcov
Tested: bunx tsc --noEmit
Tested: bun run lint
Tested: git diff --check
Not-tested: Linux runner directly before push
* fix: normalize ACP bypass requests without warning noise
The previous CI repair removed the failing partial bridge mock, but it also added a shared safeMockModule helper and left the acp-link bypass normalization warning in the real new_session path.
This tightens the fix: acp-link now treats an unauthorized client bypass request as normal permission-mode normalization without emitting a warning, and the ACP permission test explicitly preserves the real bridge and permission exports instead of using a shared helper. The agent test keeps its local mock preservation but names it by behavior and restores mocks in LIFO order.
Constraint: CI output should not contain expected warning noise for covered policy branches.
Rejected: Silence the test only | the normal new_session path would still warn for an expected normalization branch.
Rejected: Keep the shared safeMockModule helper | the failing module was specific and should be fixed by preserving real exports at the mocking site.
Confidence: high
Scope-risk: narrow
Directive: Treat client-requested bypassPermissions as data to normalize unless the local default explicitly enables bypass.
Tested: bun test packages/acp-link/src/__tests__/server.test.ts
Tested: bun test src/services/acp/__tests__/agent.test.ts src/services/acp/__tests__/bridge.test.ts src/services/acp/__tests__/permissions.test.ts
Tested: bun test --coverage --coverage-reporter=lcov with UPPER_WARN_COUNT=0
Tested: bun run test:all
Tested: bun run lint
Tested: bunx tsc --noEmit
Tested: git diff --check
* fix: harden ACP bypass and CI warning gates
ACP clients must not be able to enter bypassPermissions unless the local ACP gate and process environment both allow it. The same gate now controls session creation, explicit mode changes, and the ExitPlanMode option list, while session setup restores process.cwd so coverage and later work do not inherit ACP session state.
Constraint: CI must stay warning-clean without hiding real ACP permission failures
Rejected: Logging rejected bypass requests on the normal new_session path | it preserves audit text but reintroduces warning noise the runtime should not emit
Rejected: Broad CI=true postinstall skip | it hides explicit Chrome MCP setup checks outside the install path
Confidence: high
Scope-risk: moderate
Directive: Keep bypassPermissions gated through one ACP availability decision before exposing it to clients
Tested: bun test src/services/acp/__tests__/permissions.test.ts src/services/acp/__tests__/agent.test.ts packages/acp-link/src/__tests__/server.test.ts
Tested: bun run test:all
Tested: bun run lint
Tested: bun run build:vite with zero warning matches
Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage produced non-empty lcov with SF records and zero filtered warning matches
Not-tested: GitHub Actions result after this push
* fix: remove remaining CI warning noise
The CI log still had three non-failing warnings after the ACP hardening commit: git init default-branch advice from checkout, a Node 20 action-runtime deprecation, and one additional known Vite dynamic-import diagnostic that only surfaced on Linux. The workflow now provides explicit git config and opts actions into Node 24, while Vite keeps a narrow allowlist for acknowledged optimizer diagnostics.
Constraint: Do not use shell log filtering to hide warnings after they happen
Rejected: Grep warning lines out of CI output | it would make future diagnostics harder to find
Confidence: high
Scope-risk: narrow
Directive: Add new Vite warning allowlist entries only after checking that they are existing optimizer diagnostics, not new application defects
Tested: bunx tsc --noEmit --pretty false
Tested: bunx biome lint .github/workflows/ci.yml vite.config.ts
Tested: bun run build:vite with zero warning matches
Not-tested: GitHub Actions result after this push
* fix: reject unauthorized ACP bypass and harden CI actions
ACP clients now fail closed when permissionMode is malformed, unknown, or requests bypass without a local bypass opt-in. acp-link validates new_session input before forwarding to the agent and returns client error frames for expected unauthorized requests without logging create-failed noise. The direct AcpAgent path independently rejects invalid _meta.permissionMode and unauthorized bypass instead of falling back to settings.
CI workflows and generated GitHub App templates now use Node 24-compatible actions pinned to immutable commit SHAs, and acp-link startup output no longer prints the auth token.
Constraint: Must not hide warnings with test isolation or log filtering
Rejected: Silent fallback to local permission mode | accepts invalid client intent and masks boundary behavior
Rejected: Broad dependency churn from bun update | audit remained failing while package and lockfile churn expanded scope
Confidence: high
Scope-risk: moderate
Directive: Client-provided permissionMode must stay fail-closed before reaching AcpAgent; only local settings.defaultMode may fall back to default on invalid local config
Tested: bun test packages/acp-link/src/__tests__/server.test.ts src/services/acp/__tests__/agent.test.ts src/services/acp/__tests__/permissions.test.ts src/services/skillLearning/__tests__/skillLifecycle.test.ts src/utils/settings/__tests__/config.test.ts
Tested: bunx tsc -p packages/acp-link/tsconfig.json --noEmit --pretty false
Tested: bunx tsc --noEmit --pretty false
Tested: bun run lint
Tested: bun run test:all
Tested: local CI equivalent install/typecheck/coverage/build with warning_scan=0
Not-tested: Pre-existing bun audit vulnerabilities require a separate dependency-hardening PR
* fix: resolve dependency audit findings precisely
Use dependency-native upgrades and lockfile resolution to close the audit findings without suppressions. Keep the chrome MCP setup aligned with the new dependency graph and add real integration coverage so the override behavior stays verified.
Constraint: no audit ignores or warning suppression
Rejected: broad google-auth/protobuf overrides | replaced with upstream-compatible resolution
Confidence: high
Scope-risk: moderate
Directive: keep dependency fixes upstream-compatible; do not reintroduce blanket overrides unless the audit surface changes materially
Tested: bun audit; bun audit --json; bun install --frozen-lockfile with CLAUDE_CODE_SKIP_CHROME_MCP_SETUP=1; bunx tsc --noEmit --pretty false; bun run lint; targeted tests; bun run test:all; bun test --coverage --coverage-reporter lcov --coverage-dir coverage; bun run build:vite
Not-tested: unrelated pre-existing ACP/CORS/token fallback residual risks
* fix: keep ACP auth tokens out of URLs
Replace the ad hoc URL-token flow with crypto UUID-backed transport identifiers so the bearer token stays in structured request data instead of query strings. Keep the server, web client, and transport helpers aligned so the ACP/RCS handshake remains compatible after the API shape change.
Constraint: token must not be embedded in the URL
Rejected: token-as-uuid query fallback | leaked bearer tokens in URLs
Confidence: high
Scope-risk: moderate
Directive: preserve the structured auth path; do not reintroduce query-token fallback when adjusting ACP transport code
Tested: targeted ACP/RCS transport tests
Not-tested: unrelated pre-existing ACP/CORS/token fallback residual risks
* fix: normalize WebFetch request headers
Normalize WebFetch headers before dispatch so canonicalization preserves auth semantics and duplicate forms do not slip through. Keep the behavior locked with a focused header test instead of broadening the request pipeline.
Constraint: preserve header semantics without widening the fetch surface
Rejected: ad hoc caller-side normalization | too easy to bypass in future call sites
Confidence: high
Scope-risk: narrow
Directive: keep header normalization close to the WebFetch utility so future callers inherit the same behavior automatically
Tested: targeted WebFetch header tests
Not-tested: unrelated fetch backend behavior beyond header normalization
* fix: harden ACP remote auth surfaces
Tighten the remaining Claude security artifact items by requiring API keys on ACP global reads and relay upgrades, moving WebSocket tokens out of URLs, and replacing open web CORS with an explicit allowlist.
Constraint: Browser WebSocket clients cannot set arbitrary Authorization headers, so the token is carried in a selected subprotocol instead of a query string.
Rejected: Keep UUID auth for ACP channel groups | any caller can mint a UUID and read global ACP data.
Rejected: Preserve ?token= compatibility | secrets leak into logs, history, referrers, and intermediaries.
Confidence: high
Scope-risk: moderate
Directive: Do not reintroduce query-string bearer tokens; use Authorization or rcs.auth.<base64url-token>.
Tested: bunx tsc --noEmit --pretty false
Tested: bun run typecheck in packages/remote-control-server
Tested: bun run build in packages/acp-link
Tested: bun run lint
Tested: bun audit
Tested: focused RCS/acp-link/web tests, 160 pass
Tested: Edge headless browser WebSocket subprotocol handshake
Tested: bun run test:all, 3669 pass
Tested: bun run build:vite
Tested: bun run build
Not-tested: Manual end-to-end relay with a live external ACP agent
* fix: resolve CI dependency override lookup
The CI runner does not expose @grpc/proto-loader as a root-resolvable package, and the test was relying on local hoisting rather than the real dependency owner. Resolve proto-loader through @opentelemetry/exporter-trace-otlp-grpc and @grpc/grpc-js so the smoke test follows the package graph it is validating.
Constraint: Do not add a new root dependency for a transitive smoke test.
Rejected: Skip or weaken the test | the test protects the protobuf 7 override path and should keep exercising loadSync.
Rejected: Add @grpc/proto-loader directly to root package.json | that hides the owning-package resolution issue and broadens dependency surface.
Confidence: high
Scope-risk: narrow
Directive: Dependency override smoke tests should resolve from the package that actually owns the dependency, not from incidental root hoisting.
Tested: bun test tests/integration/dependency-overrides.test.ts; bunx tsc --noEmit --pretty false; bun run lint; bun audit; bun run test:all; git diff --check
---------
Co-authored-by: unraid <local@unraid.local>
