agent-exec transport 8515f314ceb985a1c630e7e8db3b8b0f/3af7c5319e38
This commit is contained in:
agent-exec-transport
@@ -0,0 +1,72 @@
|
||||
# Independent Code Review — feature/audit-followups-pin-gate-m5
|
||||
|
||||
- Reviewer: independent CLI review session (audit-followups-independent-review unit; fresh session, not the implementer)
|
||||
- Date: 2026-10-22
|
||||
- Review object: this checkout @ `a2b4cf73` (branch tip `feature/audit-followups-pin-gate-m5`, detached HEAD) versus `origin/main`. No `origin/*` refs exist in this sandbox, so the diff was taken against the merged authoritative main tip `d65195bd` (PR #691 merge, second parent of `a874ee60` "Merge authoritative main into the task branch"); `merge-base(d65195bd, HEAD) = d65195bd`, so `git diff d65195bd...HEAD` ≡ `git diff origin/main...HEAD`.
|
||||
- Scope claims under review:
|
||||
1. Dev: new `scripts/check_pin_regression.sh` wired into `scripts/check-pins.sh` and `scripts/task_pr_check.py`; tests `tests/test_check_pins.py` (+1 case) and `tests/test_check_pin_regression.py` (new, 13 cases). Motivating incident: PR #682 (stale branch silently resetting component pins over main).
|
||||
2. `deps/agent-native` `cff69355` → `f2fb9ffa` (review-M5 fix: Writable preflight rejects subagent delegation).
|
||||
3. Gitlink-only pin advances in `deps/agent-squad` `9edb6ba8` → `b6d87638` (nested native `f2fb9ffa`) and `modules/backend` `7bcbbdb3` → `a75783fd` (merged upstream `7bcbbdb` line + parallel work `443e7af`; nested native `f2fb9ffa`).
|
||||
- Method: read-only review of `git diff d65195bd...HEAD` (9 files, +479/−4) plus per-submodule commit/tree diffs; focused checks run where this worktree permits (see Verification runs). No code was changed; the only write is this report.
|
||||
|
||||
## Findings
|
||||
|
||||
### Blockers
|
||||
|
||||
None.
|
||||
|
||||
### Major
|
||||
|
||||
- **MA1 — The CI leg of the pin-regression gate is a structural no-op; all enforcement rests on local full-clone runs.** All three CI checkout jobs fetch a raw object id shallowly and detach (`.gitea/workflows/ci.yaml:152,336,661` — `git fetch --depth=1 origin <sha>` then `checkout -q --detach FETCH_HEAD`), which writes only `FETCH_HEAD` and never creates `refs/remotes/origin/main` (empirically confirmed in a scratch repo: after a raw-SHA shallow fetch, `git rev-parse --verify origin/main^{commit}` fails). `check_pin_regression.sh:24-27` treats an unresolvable `origin/main` as "warn + exit 0" (documented shallow-checkout escape). Consequently `task_pr_check.py ci` (ci.yaml:162) and the CI `make test` → `test-pins` leg can never fail a PR on pin regression — the exact incident shape #682 (a stale branch merged through CI) would again pass CI today. Mitigation: the delivery policy mandates a local PR check before acceptance/merge, and in a full clone (`make task-pr-check local`) the gate does enforce (verified below). Not blocking — the change is a strict improvement and the local leg is the mandated enforcement point — but the gate's stated purpose ("gitlink 相对 origin/main 不得倒退" as a PR gate) is not achieved in CI. Recommendation: in the reviewed checkout step, additionally fetch the base ref (e.g. `git fetch --depth=1 origin "refs/heads/${GITHUB_BASE_REF}:refs/remotes/origin/main"`) so `pin_regression_problems()` verifies real ancestry at gate time; the submodule materialization already produces full child clones, so the per-component `merge-base --is-ancestor` checks would then work offline.
|
||||
|
||||
### Minor
|
||||
|
||||
- **MI1 — Gitlink removals relative to `origin/main` pass silently.** The gate iterates only HEAD's gitlinks (`check_pin_regression.sh:71-72,95`); a component dropped from the Dev tree (present on base, absent in HEAD) is never compared, and `check-pins.sh` likewise only checks the containers it iterates (a container dropping one of its `deps/*` links is equally invisible). Not the #682 shape (reset, not removal), and "new pin not on base" is an intentional pass (line 74-76), but silent composition shrinkage is adjacent to the incident class this gate exists for. Suggest a follow-up that diffs the base/HEAD gitlink path sets in both directions.
|
||||
- **MI2 — `pin_regression_problems()` runs the gate with no subprocess timeout** (`scripts/task_pr_check.py:222-224`). `ensure_commit`'s `git fetch` (`check_pin_regression.sh:48-54`) can hang on a black-holed connection and would stall the PR check until the runner/job timeout. Low practical likelihood (unchanged pins short-circuit at line 73 before any fetch; CI submodules are full clones), but a `timeout=` on the `subprocess.run` would bound it.
|
||||
- **MI3 — Unreachable-but-confusing fail mode if a CI submodule lacks the old pin.** If `ensure_commit` ever needed to fetch in CI, the component remotes persist only the public URL (credentials live solely inside the checkout step's per-command env), so the fetch fails and the gate fails closed ("missing locally and could not be fetched", lines 80-86) — correct direction, but the PR author sees an unfetchable-pin error whose real cause is credentials/absence. With full submodule clones this should never trigger; noting for diagnosis only.
|
||||
|
||||
### Notes
|
||||
|
||||
- **N1 — Preflight placement is consistent with the sibling F02 checks; "zero side effects" holds for the meaningful claims.** The new check (`deps/agent-native/adapters/native/backend.go:228-230`) sits inside the existing `len(writable) > 0 && profile == exec.ToolProfileWorkspace` block (line 216) before any registry/session/model work. Workdir preparation above it (`MkdirAll`, `INSTRUCTIONS.md`/`TASK.md` writes) predates rejection — identical to the pre-existing exec/MCP/host rejections, and `TestContractWritableRejectsSubagentInPreflight` asserts the substantive zero-side-effect properties: no stream call, no factory call, no deliverable file, no `.agentx` session state.
|
||||
- **N2 — `pin regression ok` (line 100) prints even when `PIN_REGRESSION_SKIP=1` downgraded failures to warns.** Exit status is the contract (0 with warns), and the warns are loud on stderr; cosmetic only.
|
||||
- **N3 — Pre-existing gofmt deviation in `adapters/native/backend.go`** (struct-literal alignment around `runtime.NewAgentSession`, ~line 447). The file at the base pin `cff69355` fails `gofmt -l` identically; the changed hunk (212-230) is clean. Out of scope for this fix.
|
||||
- **N4 — Pre-existing stderr noise `fatal: not a tree object` when a container directory exists but is uninitialized.** Reproduced as `git -C deps/agent-intent ls-tree <sha>` from an empty dir discovering the parent Dev repo (process substitution in `check-pins.sh:68`); pre-dates this branch and does not change its exit status. In this sandbox only `modules/backend`, `deps/agent-native`, `deps/agent-squad` are materialized.
|
||||
- **N5 — Go tests NOT_RUN in this workspace (environment, not code).** `deps/agent-native/go.mod:10,12` replace `error-code-system`/`logging-system` with nested checkouts that are not materialized here, and the pinned commit objects (`a3ee67bb`, `49b5d8be`) are absent from every local object store (Dev, native, squad, backend); network access is forbidden for this unit, so the modules cannot be completed. `GOWORK=off go test ./adapters/native/...` therefore fails at module setup, not in code. Fallbacks applied: both changed files parse (`gofmt -e`) and were line-reviewed; the author-recorded evidence (`verify-m5.json` / `verify-m5-host.json` exit=0, quoted in commit `f2fb9ffa`) is uncorroborated in this session and treated as author-claimed.
|
||||
- **N6 — Composition coherence verified (lens 3).** Nested pins agree at every level: native `f2fb9ffa` (Dev `deps/agent-native` = squad `deps/agent-native` = backend `deps/agent-native`), squad `b6d87638` (Dev = backend), intent `765a7e71` (Dev `deps/agent-intent` = native = squad = backend), core `0cf03446` (Dev `modules/core` = backend `deps/agent-system-core`), command `fe7987dc`, document-renderer `3f259990`, error-code-system `a3ee67bb`, logging-system `49b5d8be`; guidelines/manager deltas (`2688fd2`/`67784f43`/`bd3a7de5` vs Dev `a15db508`/`2c2e3725`) are warn-only by design (`check-pins.sh:57-62`). `bash scripts/check-pins.sh` exits 0 ("pins ok") in this worktree.
|
||||
|
||||
## Spec lens (1) — gate catches the incident shape; M5 preflight is exact
|
||||
|
||||
- Gate trigger: HEAD pin not a descendant of the base pin fails — stale reset (`test_pr682_stale_branch_carrying_old_pin_fails`) and diverged/rebased pins (`test_diverged_pin_fails`) both exit 1 with `moves BACKWARD` + `PR #682` guidance; forward move, same pin, base-pin-absent (`new, not on origin/main`), unresolvable `origin/main`, and `PIN_REGRESSION_SKIP=1` downgrade all exit 0. `task_pr_check.py ci` wiring hard-fails (`PR_CHECK_FAILED`) on regression and passes with the skip env (`test_task_pr_check_ci_hard_fails_on_regression`, `test_task_pr_check_ci_skip_env_passes`). All 19 Python tests pass in this workspace (Verification runs).
|
||||
- M5 preflight: the rejection condition is exactly non-empty Writable ∧ workspace profile ∧ delegation enabled (`backend.go:216,228-229`). Delegation exists iff `b.opts.Subagent != nil` (`runtime/subagent/subagent.go:62-71` no-ops on nil factory; `delegate_task` is registered only in the workspace branch, `backend.go:265`), and Writable reaches `Execute` only via `withWritable(ctx, req.Workspace.Writable)` (`adapters/native/contract.go:66`) — the reasoning profile has no registry at all. Empty-Writable delegation is covered by `TestContractSubagentWithoutWritableStillDelegates` (delegate_task declared, factory invoked with the request fields). Side-effect escape confirmed closed: `tools.ApplyWriteAllowlist` runs before `subagent.Register`, so `delegate_task` never inherited the write-set — rejecting the combination is the right call, and `run_tool_script` cannot reach it either (it only invokes registered tools and bans re-dispatch).
|
||||
|
||||
## Regressions lens (2)
|
||||
|
||||
- `check-pins.sh` behavior when `origin/main` is absent: unchanged — live run exits 0 with "pins ok", the gate only warns (`check_pin_regression.sh:24-27`). The appended call is `|| fail=1` (check-pins.sh:67), so any gate crash also fails closed.
|
||||
- `task_pr_check` callers: no signature/behavior changes to existing functions (`PIN_REGRESSION` const and `pin_regression_problems()` are additive; `cmd_local`/`cmd_ci` append/inject problems only). `scripts/test_affected.py:61-62` and `scripts/task.sh:699` import unaffected names. Remote-failure `Stop` paths still precede the gate (fail-closed elsewhere). `make task-pr-check` runs from the repo root, matching the gate's cwd→toplevel resolution.
|
||||
- Delegation flows without Writable: backend `a75783fd` and squad `b6d87638` contain zero `Writable`/`ContractRequest` usage in non-test Go code (grepped) — no live composition flow can trip the new rejection; the preflight only fires for contract requests that declare both.
|
||||
|
||||
## Scope lens (3)
|
||||
|
||||
Dev diff touches exactly the declared files plus the task card (`.task/**`, host domain): `scripts/check-pins.sh` (+2), `scripts/check_pin_regression.sh` (new, 100 lines), `scripts/task_pr_check.py` (+17/−1 docstring+gate), `tests/test_check_pins.py` (+17), `tests/test_check_pin_regression.py` (new, 187 lines). Submodule ranges are integration-only as declared: native = 1 commit (`f2fb9ff` "Reject subagent delegation under a Writable write-set", parent `cff6935`, no other content); squad = 1 gitlink-only commit (`b6d8763`, `diff-tree` = `deps/agent-native` only); backend = 3 commits (`3ab7cd7` and `92c2b4e` gitlink-only pins; `a75783f` = merge of `7bcbbdb` into the pin line; tree delta vs `7bcbbdb3` is exactly the two nested gitlinks; contains both `7bcbbdb` and `443e7af`). Nothing rides along outside the declared fixes.
|
||||
|
||||
## Tests lens (4)
|
||||
|
||||
New tests assert the trigger conditions, not just exit codes: exact rejection reason string, no stream/factory invocations, no deliverable/session files (native); `moves BACKWARD` / `PR #682` / `PIN_REGRESSION_SKIP=1` message substrings, `ok <path>` / `pin regression ok` / warn lines, and `[verdict] PR_CHECK_FAILED` / `PR_CHECK_OK` plumbing (Dev). `test_check_pins.py` additionally proves the gate is actually wired inside `check-pins.sh` (copies both scripts into the fixture). Offline discipline is good: temp-dir repos, hermetic env, `GIT_CONFIG_NOSYSTEM=1`, env vars scrubbed in setUp, local bare remotes for the fetch paths.
|
||||
|
||||
## Verification runs (this session, foreground)
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| `git diff --stat d65195bd...HEAD` | 9 files, +479/−4 — matches declared scope |
|
||||
| `bash scripts/check-pins.sh` | exit 0, "pins ok"; gate warn-only (no origin/main); guidelines warns only |
|
||||
| `PYTHONDONTWRITEBYTECODE=1 python3 -m unittest tests.test_check_pin_regression tests.test_check_pins -v` | 19/19 OK (0.796s) |
|
||||
| `cd deps/agent-native && GOWORK=off go test -count=1 ./adapters/native/...` | NOT_RUN — module setup fails on unmaterialized replace targets (`go.mod:10,12`); objects absent everywhere; network forbidden (see N5) |
|
||||
| `gofmt -e` on both changed native files | parse OK; `gofmt -l` deviation pre-existing (N3) |
|
||||
| Raw-SHA shallow fetch → `origin/main` resolvability experiment | does NOT resolve (basis of MA1) |
|
||||
| `make test` | intentionally NOT run per unit packet (known pre-existing `test_svc.SvcDbRecoveryTests.test_addr_change_keeps_database_and_auth` governance failure on clean main `b7636742`, recorded on the task card; CI is the final arbiter) |
|
||||
|
||||
## Verdict
|
||||
|
||||
The pin-regression gate correctly detects the #682 backward-pin shape (and diverged pins) while passing forward/same/new-pin moves and degrading safely without `origin/main` or via the owner-reviewed skip; the M5 preflight rejects exactly the unenforceable Writable+delegation combination with zero behavioral fallout for empty-Writable or non-contract flows; composition pins advance cleanly with no source riding along; the new tests assert real trigger conditions. The one substantive weakness (MA1 — CI leg cannot enforce due to the shallow raw-SHA checkout) is an enforcement-depth gap on a strictly-improving change whose mandated local enforcement point works; MI1–MI3 are bounded follow-ups.
|
||||
|
||||
REVIEW VERDICT: APPROVE
|
||||
Reference in New Issue
Block a user