From 09d0d81fd6a7bbb54facacc4a01fde325170a416 Mon Sep 17 00:00:00 2001 From: agent-exec-transport Date: Wed, 21 Oct 2026 19:32:11 +0000 Subject: [PATCH] agent-exec transport 1f3d84fb8dd9472d3b7e8e2121660e21/c435ffbd9286 --- ...10-21-agent-stack-b1-independent-review.md | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 .task/reviews/2026-10-21-agent-stack-b1-independent-review.md diff --git a/.task/reviews/2026-10-21-agent-stack-b1-independent-review.md b/.task/reviews/2026-10-21-agent-stack-b1-independent-review.md new file mode 100644 index 00000000..3e17de16 --- /dev/null +++ b/.task/reviews/2026-10-21-agent-stack-b1-independent-review.md @@ -0,0 +1,90 @@ +# 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.