Agent-System-Dev/.task/reviews/2026-10-21-agent-stack-b1-independent-review.md

Independent Code Review — feature/agent-stack-audit-remediation-b1

• Reviewer: independent CLI review session (agent-stack-b1-independent-review unit)
• Date: 2026-10-21
• Review object: Dev worktree branch feature/agent-stack-audit-remediation-b1 @ d55e57dd (merged tip) versus main @ b25846d2; five pinned components diffed old-pin → new-pin:
  • deps/agent-native 2fc66be6 → ac91b843
  • deps/agent-squad c7121bda → 0f1daa8b
  • deps/agent-intent 651a2e1e → 765a7e71
  • deps/agent-command 4f583ca3 → fe7987dc
  • modules/backend 528e3a4c → b6141800
• Plan: docs/superpowers/plans/2026-10-21-agent-stack-audit-remediation-plan.md (audit baseline d9e9c0560a)
• Claimed scope: batch 1 = U1, U2, U3a/U3b, U4, U5, U6, U7a/U7b, U10.
• Method: read-only review of git diff per component plus Dev-level files; focused package tests where this worktree permits (see Verification runs). This review made no changes other than this report file.

Findings

Blockers

None.

Major

None.

Minor

• M1 — SessionScope identity string can collide (native). adapters/native/contract_bind.go:60-66 builds the scope identity as tenant+"/"+project+"/"+session. Distinct scopes containing / can collide (tenant a/b + project c vs tenant a + project b/c), which would bind two identities to the same hashed transcript file (contract_bind.go:72-77) and pass the exact-match check in bindSessionScope (adapters/native/backend.go:645-669). Today product identifiers do not contain /, so no legal path regresses; recommend length-prefixed or JSON-encoded identity in a follow-up.
• M2 — Reflection/JSON "probe" of statically present fields (squad). adapters/native/register.go:217-247 marshals the native result through JSON to discover SideEffects/Canceled/Aborted. The squad module is pinned to native ac91b843, where these fields exist at compile time; direct field copy is identical, simpler, and the "deps 尚未跟进" rationale in the comment cannot occur in a statically linked Go build. Behaviorally equivalent today; flagged as avoidable indirection that can silently mask future field renames (JSON key drift would quietly fall back to "possible").
• M3 — trustedUnknownOpID does not survive JSON roundtrip (command). result.go:44-49 (unexported flag) + result.go:76-95 (Validate): an Unknown receipt carrying a trusted OperationID fails Validate() after a marshal/unmarshal cycle. Verified by grep that no in-repo path re-validates a deserialized receipt (backend persists audit envelopes, not receipts; internal/cmdbus has no ExecReceipt unmarshal), so nothing breaks today — but the library contract is sharper than its docs admit. Suggest documenting the roundtrip limitation or providing a rehydration constructor.
• M4 — Stale recovery header comment (squad). squad/engine.go:1053-1069 still states "auto 允许一次重试", but HitlAuto now falls into the default: reconciliation branch (engine.go:1137-1178) and no longer auto-retries running steps without a trusted record. The code is the intended F05 tightening; only the comment contradicts it.
• M5 — Writable preflight does not cover delegate_task (native). U2 preflight (adapters/native/backend.go:213-227) rejects non-empty Writable with exec/MCP/host tools, but the workspace registry still registers subagent.Register (backend.go:278) and code orchestration (run_tool_script). run_tool_script only invokes registry tools (which enforce the allowlist, runtime/tools/registry.go:369-388) and bans the interpreter, so the residual exposure is a spawned sub-agent not inheriting the parent's write-set. The plan scoped this batch's preflight to "进程/MCP/宿主工具"; a follow-up decision on subagent scoping is warranted, not a blocker here.
• M6 — Canceled mapping keyed on ctx, not on the result (native). adapters/native/contract.go:100-105 maps to OutcomeCanceled only via errors.Is(ctx.Err(), context.Canceled). A runRes.Canceled/StopAborted run that is not caused by ctx cancellation falls to the failed branch (with Retryable=false because backend.go:385-396 already clears it, so no unsafe retry — only the outcome label is less precise). Edge case; native abort paths set ctx.

Notes

• N1 — Squad AGENT-EXECUTION.md disables cursor/antigravity adapters (component diff, +10/-2). Operational agent-execution config, not product code; documented on the task card with fail-stop rationale. It will ride the component branch into the component repo — restore enabled: true as the file itself instructs.
• N2 — Command AGENT-EXECUTION.md added (+67). Host-documented deviation (component lacked one; template parity with agent-intent), recorded on the task card.
• N3 — Executor failures without SideEffects=none lose auto-retry. squad/workflow.go:455-459 + squad/port.go:99-117 intentionally default missing qualifier to "possible" (no bounded retry). This is the specified KTD4/U3 behavior change ("缺少副作用证明的失败默认禁止自动重放"), covered by TestSideEffectsPossibleNotRetried / TestSideEffectsNoneRetriedWithinBounds; third-party executors must now declare none to keep transient retry. Listed so the API change is visible to integrators.
• N4 — Generated-command ActionRef now resolves against the full snapshot, not only the top-k catalog (intent/recognizer.go:384-437 resolveActionRef, replacing generatedSpecOf). Deliberate widening that fixes F08's "合法异名目标被拒"; slot keys/values and dispatch still gated by validateProvidedSlots (intent/slots.go:174-214) and backend admission (internal/server/intent_admission.go:41-63).
• N5 — Pin coherence verified. Nested gitlinks agree at every level: intent 765a7e71 (native→deps, squad→deps, backend→deps), native ac91b843 (squad→deps, backend→deps), command fe7987dc (backend→deps), squad 0f1daa8b (backend→deps).
• N6 — Audit-failure unknown receipts keep the trusted ID via unknownReceipt/NewUnknownAfterExecution (internal/cmdbus/cmdbus.go:690-712, deps/agent-command/result.go:64-71); rejected never carries an ID (cmdbus.go:706-712). Confirms U7b's F10 requirement end-to-end with TestCommittedWithAuditFailureKeepsOperationID.

Unit-by-unit review (spec lens, with trigger-condition evidence)

Unit / Finding	Trigger in plan	Blocking change in diff (file:line)	Verdict
U1 / F01	reasoning Targets with ../, absolute, or escaping symlink wrote files before delivery validation	Authorization pass before any write: pathInWriteSet + tools.Resolve per target, then a second write loop (adapters/native/backend.go:336-378); zero-write asserted by TestBackendReasoningProfileRejectsPathEscape (traversal/absolute/symlink, safe.md also absent)	blocked
U1 / F03	ExecConfig env nil → host env inherited	normalizeExecEnv nil→empty whitelist at registration and per-process (runtime/tools/exec.go:42-51, 165); tests nil/explicit-empty/whitelist	blocked
U1 / F15a	CombinedOutput unbounded before 64KiB trim	Continuous drain into fixed head+tail ring (headTailBuffer, exec.go:192-276; runProcess 157-175); bounded-memory + cancel-drain tests	blocked
U2 / F02	Same WorkDir reused across SessionScopes; Writable set on workspace	Scope-keyed transcript transcriptPathForScope (contract_bind.go:72-77); fail-closed identity binding incl. refusal to adopt identity-less stores (backend.go:645-669); Writable enforced on write/edit/multi_edit (runtime/tools/registry.go:356-388, builtin.go:106-109/155-158/339-342) with exec/MCP/host preflight rejection (backend.go:213-227); empty scope keeps legacy path (compat verified)	blocked
U2 / F04	Torn JSONL tail then append → mid-file corruption	repairTailLocked final-line-only repair, valid fragment gets newline, invalid truncated, mid-file untouched (runtime/session/tree.go:156-234); torn/missing-newline/mid-file/append-then-load tests	blocked
U2 / F15b	CapText declared but Text empty	Contract captures final assistant text and surfaces it; pure-text success no longer forces implicit file delivery (withAllowEmptyTargets, contract.go:62-66, 88-96); TestContractPureTextSuccessCarriesText	blocked
U3a	canceled treated as retryable; side-effect honesty	Result gains Canceled/Unknown/SideEffects with EffectiveSideEffects (exec/backend.go:79-105); loop propagates ToolCalls and Canceled, clears Retryable on cancel (runtime/loop.go:202-213, 343-361); adapter maps definite-preflight → none, tool dispatch → possible, run-started transport failure → Unknown+possible (backend.go:334-341, 381-409, sideEffectsForTools 593-599)	blocked
U3b / F05	success=patches-only; failed-with-patches promoted; file-presence completion on recovery; new ExecutionID per retry	(nil,nil)/nil-result stable failure (workflow.go:429-439); promotion path removed — patches materialize only after explicit Success, or as diagnostics without flipping status (workflow.go:444-461); canceled/aborted never retried (workflow.go:440-452, port.go:121-123); recovery trusts only step_terminals.json records, otherwise needs_reconciliation (engine.go:1070-1180, models.go:299-314); RunWaitingReconciliation resume inspects without advancing (engine.go:1199-1206); ExecutionID reused across attempts and recovery (workflow.go:402-409, engine.go:1130-1133; TestRetryReusesExecutionIDAndDeadline)	blocked
U4 / F06	config update reuses stale executor cache	Cache key adds ConfigIdentity(role) + factory generation (squad/backend.go:171-175, config_identity.go:14-33 incl. BashPermission); Register/RegisterInstance invalidate per-backend cache (backend.go:62-75); concurrency gates persist across generations and update max on resolve (backend.go:182-285); allow→deny / factory swap / gate-cap tests	blocked
U5 / F07(intent)	caller mutates registry input or returned nested objects	Deep clone at Register/compile/Snapshot/Options/Catalog boundaries (intent/fold.go:57-129, registry.go:488, 520-528, 554, 588-592, catalog.go:36-60,142); isolation asserted in u5 tests	blocked
U5 / F08	generated fallback bypasses slot validation / loses risk metadata; valid alias rejected	Generated Top built from authoritative compiledSpec with full slot validation, missing/invalid projection, risk/confirm/action metadata (recognizer.go:384-437, slots.go:174-214); classifier scores NaN/Inf/out-of-range rejected and quantized before threshold (recognizer.go:176-201, 322-330); non-finite/low-confidence keep ambiguous	blocked
U6 / F09	InvalidSlots dropped; router only sees Start; text-only params	Gate projects Missing and Invalid with reasons + risk/confirm/action (intentgate/gate.go:42-52, 198-232); router blocks dispatch unless Complete() and structured Clarification (route/route.go:60-72, 173-180); unsupported cancel/set_slot honestly fall back to turn without dispatching same-sentence Start (route/route.go:83-90, 121-131); params mapped per declared schema with type casts and required-param admission (server/intent_admission.go:41-113); high-risk still confirmed by cmdbus (TestHighRiskCommandStillRequiresConfirmation)	blocked
U7a / F07(command)	ParamsSchema/ExposeAsTool shared references	Deep copy in/out of Register/Resolve/List (registry.go:37-47, 62-66, 117, 124-130); mutation conformance test	blocked
U7b / F10	any handler error → "no side effects rejected"; audit failure drops op ID	ErrNotExecuted/NotExecuted() marker; unmarked handler errors → Unknown with trusted op ID (cmdbus.go:75-118, 536-553); audit-failure path preserves ID for committed/accepted/unknown and never for rejected (cmdbus.go:687-712); confirm-card unknown lands warn terminal with reconciliation note (cmdbus.go:615-640); owner/project-scoped bounded operation query + endpoint (cmdbus.go:263-296, server/operation_query.go)	blocked
U10 / F13	squad resolves model/base/key from default account/project	Factory resolves entire route from the run's effective snapshot + profile via the same ResolveChatModel/OpenRouteClient rules as direct path (app/squad_native.go:57-84, runtime/run_config.go:17-62, squad_runner.go:83-86, llm.go:255/604); fallback to default only when host lacks the Settings port; owner-crossing secret tests (TestSquadNativeOptionsNeverBorrowsAnotherOwnerSecret), route-contract EOF parity test	blocked

Regression lens

• Empty SessionScope: legacy fixed transcript path, no identity checks, Targets defaulting preserved (backend.go:144-146, contract_bind.go:72-74). Verified by TestLegacyTranscriptStillLoads and empty-scope branches in regression tests.
• Zero-value results: ExecResult{} zero value = Success false, SideEffects empty → read as "possible" (port.go:110-117) → stable failure, no panic (TestNilNilResultIsStableFailure, TestSideEffectsZeroMeansLegacy).
• Default configs: U10 keeps default-account fallback only for hosts without the run Settings port (squad_native.go:60-66); routeContractFor logic untouched, only re-exported (llm.go:604).
• Retry semantics change is intentional and covered (Note N3): transport failures from executors that do not declare SideEffects=none now fail fast instead of text-heuristic retry — exactly the F05/KTD4 remedy.
• Route fallback for legal matched intents: unconsumedCommandKinds only fires when cancel/set_slot/chitchat/handoff commands are present; the rule funnel emits those only on expectation-hit/ambiguous branches, and matched+complete emits Start alone (intent/command.go:88-113), so normal command dispatch is unaffected (TestValidSlotsReachHandlerMappedPerSchema).

Scope lens

All component diffs map to declared units: native (U1/U2/U3a), squad (U3b/U4), intent (U5, generator.go comment-only), command (U7a), backend (U6/U7b/U10 + mechanical OpenRouteClient rename and nested gitlinks). Dev-level changes are the task card, the plan document, and the five gitlinks. Out-of-unit but documented: squad AGENT-EXECUTION.md adapter disablement and command AGENT-EXECUTION.md addition (Notes N1/N2 — execution-infra config, explicitly recorded on the task card; no product code outside units). No undeclared product behavior changes found.

Test lens

New tests assert the original trigger conditions, not just happy paths: F01 asserts zero writes for all three escape classes; F02 asserts no shared transcript, resume-with-matching-scope, and refusal to adopt identity-less stores; F04 covers torn tail, missing newline, mid-file error, repair-then-append; F05 covers patches-only success, failed-with-full-patches stays failed, canceled/aborted not retried, side-effect-gated retry bounds, all four recovery quadrants (record/files × present/missing), ExecutionID+deadline reuse; F06 covers tighten/loosen/factory-swap/tool-change/cache-reuse/parallel-first/in-flight-cap; F07 mutation attempts on both registries; F08/F09 missing vs invalid vs unknown-key vs required-param blocks with reasons; F10 unknown-with-ID, not-executed-stays-rejected, audit-failure-keeps-ID, repeated-confirm-once, cross-owner query scope; F13 owner-secret isolation, project layer, direct-route parity, default fallback.

Verification runs (this review worktree)

Focused tests with GOWORK=off, per-directory, foreground:

• deps/agent-command: go test ./... — ok (2 packages, includes new registry/result/conformance tests).
• deps/agent-native: go test ./runtime/session/ ./runtime/tools/ — ok (torn-JSONL, env whitelist, bounded buffer, writable allowlist suites). adapters/native — NOT_RUN here: nested deps (error-code-system etc.) are uninitialized in this review worktree and cannot be materialized offline; covered by the recorded green combination run.
• deps/agent-intent, deps/agent-squad, modules/backend: NOT_RUN here for the same missing-nested-deps reason (backend imports all components). Covered by the host-recorded combination make test PASS on the batch candidate (verify-combo-make-test4.json, exit=0), which this review did not re-run per packet instruction.

Verdict

REVIEW VERDICT: APPROVE

Blocking reasons: none. The diff implements the declared batch-1 units with trigger-condition regression coverage; the six minor findings (M1-M6) are quality follow-ups that do not block merge: none of them breaks a legal existing path, all have safe defaults (fail-closed), and the intentional behavior changes (retry tightening, cancel/no-retry semantics) are the specified F05 remedies with test coverage. Recommend filing M1-M5 as follow-up increments and restoring the temporarily disabled agent-execution adapters (N1) per the file's own instruction.