Skip to content

plan.md

Latest commit

 

History

History
212 lines (176 loc) · 49.1 KB

plan.md

File metadata and controls

212 lines (176 loc) · 49.1 KB
Raw
spec adversarial-code-loop-engine-hardening
version 1.0
author adversarial-plan
based-on adversarial-spec
findings-input true

Implementation Plan

Caller enumeration

This plan touches two internal API surfaces plus one structural JSON-payload change. All are internal to this repo (no external consumers beyond the sibling adversarial-common repo, called out below).

_read_json — corruption no longer returns None (R6)

File Call site Migration Note
scripts/adversarial_loop_v4.py:1517 pre_gate = _read_json(...) or {} Missing-file tolerance (or {}) unchanged; corruption now raises JsonCorruptionError and propagates uncaught to the top of main() (hard failure, never silent). No change needed.
scripts/adversarial_loop_v4.py:1559 build_gate = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1578 review = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1642 final_gate = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1736 fix = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1772 fix_gate = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1853 verify = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:1911 arb = _read_json(...) or {} Same as above.
scripts/adversarial_loop_v4.py:2184 saved = _read_json(out_dir / "state.json") (--resume path) Only call site that must explicitly catch JsonCorruptionError — must map to EXIT_INFRA with a message naming state.json rather than falling into the existing else: print("starting fresh") branch (AC14). Handled in P4.

Search method: rg "_read_json\(" scripts/adversarial_loop_v4.py — 10 matches (1 definition + 9 call sites) enumerated above; no callers outside this file.

commit_workdir_changes — contract pinned, not changed (R11)

File Function/Method Migration Note
scripts/phases/phase_build.py (run_build, 2 call sites) Already migrated from commit_all in the interim commit; no behavior change. Stale commit_all comment at line ~6 rewritten (AC23: zero commit_all matches).
scripts/phases/phase_fix.py (run_fix, 1 call site) Already migrated; no behavior change.
scripts/phases/phase_git.py (definition, line 16) Docstring rewritten to state the KEEP decision and rationale (AC22).
scripts/phases/test_phases.py New unit tests per AC23 (commit / noop / gitignore).

Search method: rg "commit_workdir_changes|commit_all" scripts/ — the two build call sites and one fix call site are the only production callers; commit_all currently appears only in stale comments (phase_build.py:6, phase_git.py:19), removed in P11.

read_files JSON payload — producer side already migrated (R10)

File Consumer Migration Note
adversarial-common ReadGatePolicy Reads the array Out of this repo's scope; already accepts the array.
tests/mocks/mock_review*.sh, mock_verify_*.sh Emit the array Already migrated (verified: all 5 fixture scripts emit read_files); pinned by a new parser test in P10.
scripts/phases/phase_verify.py, phase_arbiter.py prompt builders Instruct the array Already migrated; pinned by prompt-text assertions in P10. phase_arbiter.py:172 docstring still says "must emit a READ: marker" — stale, corrected in P10 (not part of the prompt sent to the model, but must not contradict actual behavior).

Search method: rg "read_files" scripts/ tests/ confirms producers/consumers above; no unmigrated site found.

Steps

P1: Remove the secret-scanning-bypass runbook [R1, AC1]

  • Files: [references/github-secret-scanning-bypass.md]
  • Description: git rm references/github-secret-scanning-bypass.md. No replacement content — deletion only, per R1. Do not touch any other file in references/.
  • Dependencies: []
  • Tests: git ls-files | rg -q "github-secret-scanning-bypass.md" must exit 1 (no match). rg "github-secret-scanning" references/ scripts/ SKILL.md must return zero matches (AC1).
  • Risks: None — the file has no code consumer (verified: no .md in references/ is imported or executed by any script; it's documentation-only).

P2: Make brief-like artifacts uncommittable [R2, AC2]

  • Files: [.gitignore, SKILL.md, scripts/test_loop_fixes.py]
  • Description: In .gitignore, add two patterns (BRIEF.md, *.brief) under the existing "Adversarial-loop artifacts" comment block. In SKILL.md, add one line under the ## Git workflow section (after the existing "Branch naming" paragraph, ~line 223): a rule stating no brief-like artifact (BRIEF.md, *.brief) may be committed — untracked persona/prompt files are ignored by design. Add a hardening test to scripts/test_loop_fixes.py that: creates a temp git repo seeded with this repo's .gitignore, writes a BRIEF.md-style file into it, and asserts (a) git check-ignore <file> exits 0, (b) git ls-files in that repo does NOT list the file — both before any git add attempt (the file was never tracked) and after the git add/git status --porcelain sequence below (confirming the ignore rule, not merely a missing add, is what keeps it untracked; AC2's explicit git ls-files requirement), and (c) git add <file>; git status --porcelain shows the path as untracked/unstaged (not staged) — proving a subsequent commit cannot include it.
  • Dependencies: []
  • Tests: The new test in scripts/test_loop_fixes.py (e.g. test_brief_artifacts_are_uncommittable) is the acceptance test itself (AC2), and must include the git ls-files assertion described above, not just git check-ignore/git status --porcelain. Also run rg "BRIEF" .gitignore SKILL.md to confirm both files were edited, and git ls-files | rg -q "BRIEF\.md$|\.brief$" against this repo's actual tracked files (must exit 1) as a real-repo sanity check alongside the isolated-repo test.
  • Risks: The test must build its git-ignore check in an isolated tmp repo (not this repo's actual working tree) to avoid mutating the real repo's git state; use tmp_path fixture and copy .gitignore content in, consistent with existing tests in the same file (e.g. test_f6_identity_bootstrapped_when_unset already sets up isolated tmp git repos). git ls-files only lists tracked files regardless of an untracked file's presence on disk, so the assertion must run after the file is written but is only meaningful once combined with the git add step — assert it at both points to rule out a trivially-true check (asserting "not listed" before any add is a weaker but still required proof; the assertion after add is the one that actually demonstrates uncommittability).

P3: Pin scripts/install.sh to explicit refs with integrity checks [R3, AC3, AC4, AC5, AC6]

  • Files: [scripts/install.sh]
  • Description: Add SKILL_CODE_LOOP_PIN, SKILL_COMMON_PIN (explicit commit SHAs, not tags/branches), SKILL_CODE_LOOP_SHA256, SKILL_COMMON_SHA256 constants near the top with a comment block documenting how the SHA-256 values were computed (git -C <dir> archive "$PIN" | sha256sum). Add a _pinned_clone(url, dir, pin) helper that performs no bare git clone at all, so the moving default branch is never fetched: (1) git init -q "$dir" → on failure, rm -rf "$dir" and exit non-zero with a message naming the pin and repo; (2) git -C "$dir" remote add origin "$url" (local-only, no network) → same failure handling; (3) git -C "$dir" fetch --depth 1 origin "$pin" — this is the first and only network fetch, and it fetches exactly the pinned ref/SHA, never the repo's HEAD/default branch → same failure handling; (4) git -C "$dir" checkout FETCH_HEAD → same failure handling. Each of the four sub-steps gets its own if ! <cmd>; then echo "ERROR: <step> failed for $dir (pin $pin, repo $url)" >&2; rm -rf "$dir"; return 1; fi (or equivalent), so any failure at any stage removes the partial directory and exits/returns non-zero naming both the pin and the repo (AC4) — this deliberately diverges from /home/chpo/.hermes/skills/adversarial-spec/scripts/install.sh's git clone --no-checkout --depth 1 "$url" "$dir"-then-pinned-fetch pattern, because that pattern still transfers the default branch's tip commit during the initial clone before the pinned fetch overrides it, which violates AC3's "never clones a moving default branch" — note this divergence in a comment above _pinned_clone so a future sync from the sibling doesn't silently reintroduce the initial unpinned clone. Replace both existing git clone --depth 1 ... main calls (skill dir + common repo) with calls to _pinned_clone, tracking SKILL_CLONED/COMMON_CLONED flags for freshly-cloned dirs. After cloning, run python3 -m compileall -q over both source dirs (static syntax only — matches the existing sanity-check import, but compileall never executes the cloned code, avoiding an integrity gap where an unverified clone's code runs before its checksum is checked). Only for freshly cloned dirs, compute git -C <dir> archive <PIN> | sha256sum and compare to the corresponding SKILL_*_SHA256; mismatch exits 1 naming the pin. Existing-dir skip behavior (the if [ ! -d ... ] guards) is unchanged. To compute the actual pin/SHA256 values: run git -C /home/chpo/.hermes/skills/adversarial-code-loop rev-parse HEAD and the equivalent for adversarial-common, then git archive <sha> | sha256sum in each.
  • Dependencies: []
  • Tests: bash -n scripts/install.sh (syntax check). rg "SKILL_.*_PIN|SKILL_.*_SHA256|_pinned_clone" scripts/install.sh must show all four constants and the helper (AC3). rg "git clone" scripts/install.sh must return zero matches anywhere in the file — no git clone of any kind remains, moving-branch or otherwise (AC3, strengthened: the new helper uses init+remote add+fetch+checkout, so this is a stronger check than only excluding ...main). Manual/CI dry run: bash scripts/install.sh /tmp/install-test-$$ against a scratch target dir, verifying the compileall pass and SHA-256 match print OK (AC5, AC6); a second run with a deliberately wrong SKILL_COMMON_SHA256 (temp edit, reverted) must exit 1 and remove the partial clone (AC4, checksum-mismatch case). Additionally, a dedicated clone/fetch/checkout-failure test (AC4, the case findings review flagged as missing): run _pinned_clone directly (source scripts/install.sh in a subshell, or extract the function) against (i) an unreachable URL — remote add/fetch fails — and (ii) a reachable URL with a syntactically-valid but nonexistent pin SHA (e.g. 0000000000000000000000000000000000dead) — fetch fails to resolve the ref — asserting in both cases: the target $dir no longer exists on disk afterward, the function/script exits non-zero, and the emitted error message contains both the pin value and the repo URL/name.
  • Risks: Pins go stale as soon as either repo's HEAD moves past them — this is intentional (reproducible installs); the person merging this plan's fix step must re-verify the pins are the actual current HEADs of both repos at merge time, not the SHAs captured during planning, since more commits may land before this plan executes. A bootstrap curl | bash install has no local checkout to HEAD-derive from, so the constants must be literal, hardcoded SHAs (not computed at install time). git -C "$dir" fetch --depth 1 origin "$pin" requires the remote to accept fetching an arbitrary SHA (not just refs/branches/tags) — most git hosts (GitHub, GitLab, self-hosted with uploadpack.allowReachableSHA1InWant or allowAnySHA1InWant) support this for reachable commits, but if the target host does not, fetch will fail even for a valid pin; document this prerequisite in the comment block beside the pin constants so a future host migration doesn't silently break installs.

P4: Fail-closed _read_json on JSON corruption [R6, AC13, AC14]

  • Files: [scripts/adversarial_loop_v4.py]
  • Description: Add a new exception class JsonCorruptionError(Exception) at module level (near _WatchdogStallError, ~line 616, or immediately above _read_json at line 98 — place it directly above _read_json since it's that function's contract, not the watchdog's). Rewrite _read_json (line 98): catch OSError from Path(path).read_text(...) and return None (missing-file case, unchanged); separately catch ValueError (json.JSONDecodeError is a ValueError subclass) from json.loads(...) and raise JsonCorruptionError(f"corrupted JSON at {path}: {exc}") from exc instead of returning None. Update the --resume call site (line 2184, inside main()): wrap saved = _read_json(out_dir / "state.json") in try/except JsonCorruptionError as exc:, and on catch, print an error naming state.json (e.g. f"X corrupted state.json: {exc}") and return EXIT_INFRA immediately — do not fall through to the existing else: print("starting fresh") branch. Leave the other 8 _read_json(...) or {} call sites (see caller table above) untouched — corruption now propagates through them as an uncaught JsonCorruptionError, which is correct per R6 ("never silently treated as missing").
  • Dependencies: []
  • Tests: New tests in scripts/test_loop_fixes.py: (a) _read_json on a missing path returns None; (b) _read_json on a file containing "{not valid json" raises JsonCorruptionError whose message contains the file path (AC13); (c) a real call site (e.g. write a corrupted 02_review.json and drive the resume path through that phase) confirms the exception propagates rather than the phase silently treating the artifact as absent (AC13). New test driving --resume (likely in scripts/test_loop_fixes.py or tests/test_orchestrator.py, matching existing resume-test conventions): corrupt state.json with invalid JSON, invoke the CLI/main() with --resume, assert the return code is EXIT_INFRA and the printed/returned error names state.json, and that no "starting fresh" message appears (AC14).
  • Risks: json.JSONDecodeError is a subclass of ValueError, so the existing except (OSError, ValueError) must be split into two separate except clauses (not just re-raise inside one) to keep the missing-file/corruption distinction exact — double-check no other ValueError source exists inside the try body that would be mis-classified as "corruption" (only Path.read_text and json.loads run inside the try, so this is safe).

P5: Harden _write_manifest — no post-success crash, no temp leak, single serialization path [R5, AC10, AC11, AC12]

  • Files: [scripts/adversarial_loop_v4.py]
  • Description: Add a module-level import yaml at the top of the file (with the other stdlib/third-party imports) and delete both in-function import yaml as _yaml statements (lines ~168 and ~279). Add a shared helper _render_frontmatter_document(frontmatter: dict, body: str) -> str (placed just above _write_manifest) that owns the serialization both functions currently duplicate: yaml_str = yaml.dump(frontmatter, default_flow_style=False, sort_keys=False, allow_unicode=True) followed by return "---\n" + yaml_str.rstrip() + "\n---\n\n" + body. Rewrite _write_manifest to build its frontmatter dict as today (lines 169-192) and its body text as today (the # Run Manifest — ... section, lines 204-230, minus the "---"/yaml-frontmatter lines currently prepended at 199-203), then call content = _render_frontmatter_document(frontmatter, body) instead of hand-rolling the "---\n" + yaml_str.rstrip() + ... concatenation inline. Rewrite _write_partial_manifest the same way: keep its frontmatter dict construction (lines 265-277), replace its own inline _yaml.dump(...) + "---\n" + yaml_str.rstrip() + "\n---\n\n" + ... concatenation (lines 279-291) with a call to the same _render_frontmatter_document(frontmatter, body). This is the "single serialization helper" AC11 requires — both _write_manifest and _write_partial_manifest produce their final file content through one shared function, not two independent yaml.dump+string-concatenation call sites. Separately, add a second shared helper _atomic_write_text(content: str, dest_dir: Path, dest_path: Path, *, prefix: str, suffix: str) -> None (placed just above _render_frontmatter_document) that owns the I/O both functions currently duplicate: performs tempfile.mkstemp(suffix=suffix, prefix=prefix, dir=str(dest_dir)), writes+fsyncs+closes the fd, then os.replace(tmp_path, str(dest_path)); on ANY exception after mkstemp succeeds, os.unlink(tmp_path) inside a try/except OSError: pass guard before re-raising the original exception (so a failure at write/flush/close/replace never leaves a manifest_* temp file behind). Rewrite _write_manifest's tail (lines 233-245) to call _atomic_write_text(content, manifest_dir, manifest_path, prefix="manifest_", suffix=".md") instead of its inline mkstemp/write/replace block. Wrap the entire body of _write_manifest (from run_uuid = ... through the _atomic_write_text call) in a try/except Exception as exc: that appends f"manifest write failed: {exc}" to state.setdefault("warnings", []) and returns — mirroring the pattern (not the silence) of _write_partial_manifest, whose own except Exception: pass stays unchanged since the partial-manifest path is invoked from cleanup/interrupt handlers where warnings are moot. Rewrite _write_partial_manifest's tail (lines 292-301) to also call _atomic_write_text, keeping its outer except Exception: pass as the second layer (belt-and-suspenders for the interrupt path, which must never raise). End state: _write_manifest and _write_partial_manifest each build their own frontmatter dict and body text (the only parts genuinely different between a full and partial manifest), then both route through the same two shared helpers — _render_frontmatter_document for serialization, _atomic_write_text for atomic I/O.
  • Dependencies: []
  • Tests: New parametrized test in scripts/test_loop_fixes.py (test_write_manifest_hardening or similar) covering AC10: monkeypatch each of yaml.dump, tempfile.mkstemp, os.write, os.fsync (or os.close), and os.replace in turn to raise, call _write_manifest(state, workdir, out_dir, ledger) for each case, and assert: no exception propagates, state["warnings"] gained an entry, and the manifest dir contains no manifest_*/.*.tmp leftovers (list(manifest_dir.glob("manifest_*")) empty). A success-path test (already likely covered by tests/test_18_manifest.sh, but add/confirm a Python-level one) parses the written manifest and asserts run_id, feature, verdict, findings.total are present in the YAML frontmatter (AC11, success-path clause). A direct unit test on _render_frontmatter_document (e.g. test_manifest_serialization_is_shared) that monkeypatches it once (monkeypatch.setattr wrapping the real function to record call count/args) and asserts it is invoked exactly once by _write_manifest and exactly once by _write_partial_manifest — pinning AC11's "share a single serialization helper" clause directly, not just inferring it from output shape. rg "import yaml" scripts/adversarial_loop_v4.py must show only the module-level import (AC12). rg "_yaml\.dump|yaml\.dump" scripts/adversarial_loop_v4.py must show exactly one call site, inside _render_frontmatter_document (confirms no duplicate inline .dump() call survived the refactor).
  • Risks: os.fsync failing after os.write succeeded still leaves data in the fd's buffer; the finally: os.close(fd) must still run so the fd doesn't leak even when fsync raises — verify the inner try/finally structure inside _atomic_write_text preserves this (write+fsync in try, close in finally, both wrapped by the outer unlink-on-failure logic). Test doubles that monkeypatch os module functions globally (os.write, os.replace) risk breaking pytest's own I/O if not scoped narrowly (e.g. side-effect only on the specific manifest path, or restored via monkeypatch fixture teardown) — use monkeypatch.setattr (auto-restoring) rather than manual patch/restore. _render_frontmatter_document's body-text argument differs in shape between callers (_write_manifest passes a multi-section markdown body assembled via "\n".join(lines) + "\n"; _write_partial_manifest passes a shorter concatenated string) — the helper must not assume any particular body structure beyond "a string to place after the frontmatter fence," so no field-specific logic belongs inside it.

P6: Fix empty-file finding partitioning [R7, AC15]

  • Files: [scripts/adversarial_loop_v4.py]
  • Description: In _partition_findings_by_file (line 684), change the union logic so blank/empty file values never union with each other. Currently file_to_indices keys on name = (f.get("file") or "").strip(), and the if indices: _union(indices[0], i) step merges every finding whose file is "" into one group. Fix: only union on a shared non-empty name — e.g. skip the file_to_indices bookkeeping entirely when name == "" (each empty-file finding keeps its own singleton root, since parent[i] == i by default and nothing unions it to another index). Concretely: guard the indices = file_to_indices.setdefault(...) / union block with if name: so blank/whitespace-only file values never enter the union-find keying at all.
  • Dependencies: []
  • Tests: New test in scripts/test_loop_fixes.py: three findings with file: "" (or missing file key / whitespace-only) produce three independent groups (AC15); two findings sharing a real filename still produce one group; findings on disjoint real filenames stay in separate groups; an empty findings list produces [] (no groups).
  • Risks: None — this is a pure logic fix isolated to one function with no external state; existing tests for the shared-filename and disjoint-filename cases (if any already exist) must still pass unchanged.

P7: Per-invocation watchdog isolation and deterministic selection [R4, AC7, AC8, AC9]

  • Files: [scripts/adversarial_loop_v4.py]
  • Description: In _run_dev_with_watchdog (line 620), the current next(iter(new)) selection has two problems: (a) it picks non-deterministically among simultaneously-available candidates (arbitrary set iteration order), and (b) — the deeper issue — a pure _ACTIVE_PROCESSES - before set-difference plus a claim registry (a "first invocation to observe it wins" scheme) still cannot attribute a process to the invocation that actually spawned it: if two groups' phase_fn calls each register a new subprocess inside the same ~100ms poll tick, both invocations' new sets contain both PIDs, and whichever invocation's watchdog thread happens to grab the lock first claims a process it did not spawn — a claim registry only prevents the same PID being claimed twice, it does not establish which invocation should claim which PID. Fix by establishing ownership at registration time instead of inferring it later from timing: _process_group (line 801) submits each group's work via pool.submit(_process_group, i, g) (line 877), and phase_fn(**phase_kwargs) is called synchronously on that pool worker thread (confirmed by tracing into adversarial_common.runner._execute_attempt, which calls subprocess.Popen(...) and _register_process(proc) synchronously, in whatever thread invoked it, before communicate() blocks — i.e., on the same worker thread that is running this group's _run_dev_with_watchdog/phase_fn call). This means threading.get_ident() at the moment a process is registered reliably identifies which group's invocation spawned it, with no polling-window race. Implementation: at module load (near the _ACTIVE_PROCESSES/_ACTIVE_PROCESSES_LOCK import, line 60), wrap adversarial_common.runner._register_process and _register_process's companion _unregister_process — reassign adversarial_common.runner._register_process = _owner_tracking_register and ..._unregister_process = _owner_tracking_unregister at import time, without editing the sibling adversarial-common repo (out of this repo's scope; the wrap lives entirely in adversarial_loop_v4.py). Add a module-level _PROCESS_OWNER_THREAD: dict[int, int] = {} guarded by the existing _ACTIVE_PROCESSES_LOCK (reuse the lock; do not nest lock acquisitions with the wrapped functions' own locking — call the original function first, then acquire the lock separately to stamp/clear ownership, since threading.Lock is non-reentrant). _owner_tracking_register(proc): call the original _register_process(proc), then with _ACTIVE_PROCESSES_LOCK: _PROCESS_OWNER_THREAD[proc.pid] = threading.get_ident(). _owner_tracking_unregister(proc): call the original _unregister_process(proc), then with _ACTIVE_PROCESSES_LOCK: _PROCESS_OWNER_THREAD.pop(proc.pid, None). Inside _watchdog()'s polling loop, change the candidate computation to mine = sorted((p for p in (_ACTIVE_PROCESSES - before) if _PROCESS_OWNER_THREAD.get(p.pid) == threading.get_ident()), key=lambda p: p.pid) (all under the lock) — this filters to processes both new-since-snapshot AND owned by this exact invocation's thread, so a concurrent group's process is never a candidate regardless of poll timing; picking mine[0] (lowest PID) when non-empty also satisfies deterministic selection (AC8). If _ACTIVE_PROCESSES - before is non-empty but none are owned by this thread yet (the owner-stamp write lands a beat after the set-add — both happen under separate lock acquisitions in the wrapper), keep polling; the owning invocation's own next poll tick will see the stamp. Existing single-group retry/stall semantics (first stall → retry, second stall → _WatchdogStallError) are untouched — only the process-selection step inside _watchdog() changes.
  • Dependencies: []
  • Tests: New tests in scripts/test_loop_fixes.py: (a) two concurrent _run_dev_with_watchdog invocations with staggered process registration (e.g. via threading.Events controlling when each fake phase_fn "spawns" its process, calling the real wrapped _register_process from each of two real worker threads so threading.get_ident() differs per invocation) — assert each is monitored by exactly one watchdog and that stalling one group's process only kills that group's process, the other stays alive (AC7). (b) an adversarial-timing variant of (a) that registers both groups' processes within the same poll tick (drive both registrations from a shared threading.Barrier right before each thread calls the wrapped _register_process, so both PIDs land in _ACTIVE_PROCESSES before either watchdog's next poll) — assert ownership is still attributed correctly per thread, proving the fix addresses the race the claim-registry-only approach could not (this is the regression test for the specific review finding that blocked the claim-registry design). (c) deterministic-selection test: seed _PROCESS_OWNER_THREAD/_ACTIVE_PROCESSES with multiple fake processes owned by the same thread at the same instant (fixed PID set), run the selection logic repeatedly, and assert the same PID (lowest) is picked every time (AC8; use a controlled/seeded PID list, not relying on OS-assigned PIDs). (d) confirm the existing single-group stall/retry/_WatchdogStallError test(s) still pass unmodified (AC9) — locate and re-run tests/test_16_watchdog.sh plus any existing pytest coverage of _run_dev_with_watchdog. (e) a unit test on the wrapper itself: call the wrapped _register_process/_unregister_process directly and assert _PROCESS_OWNER_THREAD gains/loses the entry, independent of the watchdog polling loop.
  • Risks: The ownership design assumes phase_fn's eventual Popen+_register_process call happens synchronously on the same thread that called _run_dev_with_watchdog for that group — true for the current run_phase_cmd → _execute_attempt call chain (verified by reading adversarial_common/runner.py:1561-1590), but if a future change to run_phase_cmd spawns Popen from a helper thread instead, ownership attribution would silently break; add a comment at the wrapper site calling out this assumption explicitly so a future adversarial_common upgrade doesn't invalidate it unnoticed. Monkeypatching a sibling package's module-level function from this repo is inherently fragile if adversarial_common.runner is upgraded to import _register_process under a different name or restructure _execute_attempt to call it via a class method instead of a bare module-global lookup — the wrapper relies on Python resolving _register_process as a global name lookup inside runner.py at call time (confirmed: it's called as a bare name, not self._register_process or an imported bound reference), so reassigning adversarial_common.runner._register_process does intercept the sibling module's own internal calls; a version pin or a comment noting this coupling to _execute_attempt's current implementation is warranted (cross-reference P12's strict_consensus cross-repo-consumer documentation pattern for how this repo already tracks such coupling to the sibling package). A claim/owner-entry leak (never popped) would grow _PROCESS_OWNER_THREAD unboundedly across a long-running process — the pop in _owner_tracking_unregister must run on every exit path where the original _unregister_process runs (it already does, in _execute_attempt's finally/exception-cleanup paths per runner.py:1610,1620), so no new leak surface is introduced by wrapping it, but confirm this holds during implementation.

P8: Formalize empty-pick cherry-pick classification [R8, AC16, AC17]

  • Files: [scripts/adversarial_loop_v4.py]
  • Description: In the merge step (around line 894-933), the current code invokes git cherry-pick --skip whenever CHERRY_PICK_HEAD is present, regardless of why the cherry-pick failed — this does not match R8's explicit classification requirement. Add a message check before the CHERRY_PICK_HEAD probe: define a module-level constant _EMPTY_PICK_MARKER = "The previous cherry-pick is now empty" (the documented git message) near _WatchdogStallError. In the except gitops.GitError as exc: block (line 897), only enter the probe-and-skip path when _EMPTY_PICK_MARKER in str(exc); when the marker is absent, go straight to result["merge_error"] = str(exc) and skip the CHERRY_PICK_HEAD probe/--skip entirely, even if CHERRY_PICK_HEAD happens to be present (AC17's "any other cherry-pick failure ... including one where CHERRY_PICK_HEAD happens to be present ... is NOT classified as an empty pick"). The existing probe→skip logic (lines 912-933) and its three outcomes (skip succeeds → merged, skip fails → merge_error, no CHERRY_PICK_HEAD → merge_error = str(exc)) stay exactly as-is, just gated behind the new marker check. The finally-block _abort_in_progress_cherry_pick cleanup (line 943) is untouched — it already runs unconditionally.
  • Dependencies: []
  • Tests: New tests in scripts/test_loop_fixes.py: (a) gitops.cherry_pick raises a GitError whose message matches _EMPTY_PICK_MARKER, with CHERRY_PICK_HEAD present (mock the subprocess.run probe) — assert git cherry-pick --skip is invoked, the group merges without merge_error, and later groups still merge in order (AC16). (b) a failing --skip (non-zero exit) still records merge_error (AC17, first sub-case, unchanged). (c) marker matches but the CHERRY_PICK_HEAD probe finds nothing — merge_error = str(exc) (AC17, second sub-case, unchanged). (d) a GitError that does NOT contain the marker, even with CHERRY_PICK_HEAD present — assert merge_error is recorded and git cherry-pick --skip is never invoked (AC17, new sub-case — this is the actual behavior change). (e) assert _abort_in_progress_cherry_pick still runs in all cases (mock it and assert call count).
  • Risks: The exact git message text ("The previous cherry-pick is now empty...") is git-version-dependent prose; pin the check to a marker prefix ("The previous cherry-pick is now empty") rather than the full sentence, since git appends different trailing guidance ("...use 'git cherry-pick --skip'" etc.) across versions — the spec already commits to this substring per R8's exact wording.

P9: Document the F1 contract-gate trust boundary + fail-closed test [R9, AC18, AC19]

  • Files: [scripts/adversarial_loop_v4.py, tests/test_contract_gate.py]
  • Description: Rewrite _run_contract_gate's docstring (line 1396) to explicitly state the trust boundary: spec ac-directive blocks are untrusted input (they come from the spec file, which in this pipeline may itself be adversarial/attacker-influenced); directives execute as shell commands only through the shared adversarial_common.gates/contract runner's argv allowlist (binary must be on the allowlist, shlex.split+shlex.join round-trip check, no shell metacharacters, NUL-byte rejection); an unverifiable directive (unparsable shlex, non-allowlisted binary, or any exception from run_contract_gate) must never let the run settle APPROVE — it fails closed to REJECT with infra: True. No change to the gate invocation itself (it already fails closed via the existing except Exception branch) — this step is documentation plus a new regression test.
  • Dependencies: []
  • Tests: New tests in tests/test_contract_gate.py: (a) a spec whose ac-directive contains a non-allowlisted command token (e.g. rm -rf / or any binary outside the shared allowlist) — assert the gate settles REJECT or carries infra: True, never APPROVE (AC19, first case). (b) a directive with unparsable shlex (e.g. unbalanced quotes) — assert the same fail-closed outcome (AC19, second case). Both tests exercise run_contract_gate (or _run_contract_gate if easier to construct the spec/workdir fixture) end-to-end, matching the existing fixture patterns already in test_failing_ac_blocks_approve/test_passing_ac_allows_approve.
  • Risks: None functionally (docstring-only change to production code); the new tests must construct a real ac-directive block matching the shared parser's grammar (see adversarial_common/contract.py) — reuse the existing spec-fixture helper in tests/test_contract_gate.py rather than hand-rolling directive syntax.

P10: Formalize the read_files JSON marker [R10, AC20, AC21]

  • Files: [scripts/phases/test_phases.py, tests/test_orchestrator.py, scripts/phases/phase_arbiter.py]
  • Description: phase_verify.py and phase_arbiter.py's prompt/reminder builders already instruct judges to confirm reads via a read_files JSON array (landed in the interim campaign fix) and no longer demand a plain-text READ: line beside "Output ONLY valid JSON" — this step formalizes that with tests and fixes one stale doc comment. In scripts/phases/phase_arbiter.py, correct run_arbiter's docstring (line 172, "the agent must emit a READ: marker") to say "the agent must confirm via a read_files JSON array entry" — it currently contradicts the actual prompt text built by _build_arbiter_prompt/_arbiter_reminder just above it. In scripts/phases/test_phases.py, add tests asserting _build_verify_prompt/_verify_reminder (or their arbiter equivalents) output contains the literal substring "read_files" and does NOT contain a READ: demand adjacent to "Output ONLY valid JSON" (AC20). In tests/test_orchestrator.py, add a test that parses each of tests/mocks/mock_review*.sh and tests/mocks/mock_verify_*.sh (extract the embedded JSON template each script prints) and asserts each emits a read_files array containing the findings path (AC21) — confirm this doesn't duplicate the existing readgate logic at tests/test_orchestrator.py:98-131, which already handles both marker styles; the new test specifically pins the fixtures' JSON shape rather than the orchestrator's tolerance of it.
  • Dependencies: []
  • Tests: The additions described above ARE the tests (AC20, AC21). Additionally, run the existing full-flow orchestrator tests (tests/test_orchestrator.py, readgate-enforced paths) to confirm they still pass unmodified with array markers — no regression.
  • Risks: tests/test_orchestrator.py:98-131 already has fallback logic for a legacy READ: regex (m2 = re.search(r'READ:\s*(\S+)', stdin)) — leave that fallback in place (it's test-harness tolerance, not production prompt text) unless it's now provably dead code across all fixtures, in which case removing it is optional cleanup, not required by any AC.

P11: Settle the interim commit policy — KEEP commit_workdir_changes [R11, AC22, AC23]

  • Files: [scripts/phases/phase_git.py, scripts/phases/phase_build.py, scripts/phases/test_phases.py]
  • Description: Rewrite commit_workdir_changes's docstring (scripts/phases/phase_git.py, line 16) to state the decision explicitly: this spec (adversarial-code-loop-engine-hardening, R11) decided to KEEP the interim add -A semantics permanently — not as a placeholder for a future allowlist. Rationale (must appear verbatim in substance): the FIX agent may legitimately modify files beyond the findings list it was given (e.g. adding/updating tests, touching manifests); a tight per-finding allowlist would silently drop those extra files from the commit and lose real work at merge time; the engine has no reliable way to enumerate everything a fixer's output surface might touch ahead of time. Remove the now-inaccurate "the code-loop spec will define the proper per-phase allowlist policy" sentence. In scripts/phases/phase_build.py, reword the stale docstring line at ~line 6 ("commit_all forces an empty commit") to describe the current commit_workdir_changes-based behavior instead of the removed commit_all API.
  • Dependencies: []
  • Tests: New tests in scripts/phases/test_phases.py for commit_workdir_changes: (a) commits all non-ignored files (tracked + untracked) with the given message; (b) with nothing to commit, returns the current HEAD sha (noop case) rather than an empty string; (c) gitignored files are excluded from the commit. rg "commit_all" scripts/ must return zero matches (AC23) — verify after editing both doc comments.
  • Risks: None — no behavior change, only documentation and new tests around already-shipped code.

P12: Settle the consensus dependency — KEEP strict_consensus as a live cross-repo consumer [R12, AC24]

  • Files: [scripts/adversarial_loop_v4.py, scripts/test_loop_fixes.py]
  • Description: At the import site (from adversarial_common.consensus import strict_consensus, line 57), replace/extend the existing # P4 strict-consensus mode inline comment with one documenting that this is a live cross-repo consumer: deleting or renaming adversarial_common.consensus breaks this engine (adversarial_loop_v4.py's strict-consensus mode, used at line 596), so any future cleanup pass over adversarial-common that greps for consumers must include this repo.
  • Dependencies: []
  • Tests: New unit test in scripts/test_loop_fixes.py that imports strict_consensus directly and asserts its contract as used here: (a) all-accept votes → accepted; (b) one dissenting vote with strict=True → rejected; (c) a non-bool vote value raises TypeError (AC24).
  • Risks: None — pure documentation plus a contract-pinning test; no vendored copy is introduced in this repo per R12.

P13: Align the injection-probe test's name/docstring with its actual assertions [R13, AC25]

  • Files: [tests/test_threat_model.py]
  • Description: test_injection_probe_behavioral_equivalence (line 206) already asserts exactly what R13 requires — prompt equality (assert prompts[0] == prompts[1], line ~299) and payload-absence (assert injection_payload not in prompts[0], line 300) — because _build_prompt in phase_review.py builds the reviewer's prompt from only the branch name and branch-point SHA, never the diff body, so the injection probe and a clean baseline diff produce byte-identical prompts. Rename the test to test_injection_payload_never_reaches_prompt (or similarly precise) and rewrite its docstring to state directly: "the reviewer prompt is built only from branch name and branch-point SHA, so the injection probe and a clean baseline produce byte-identical prompts — hence identical verdicts/findings — and the injection payload never reaches the model's input at all." Keep every existing assertion (prompt-equality, payload-absence, verdict-equality, finding-set-equality) unchanged.
  • Dependencies: []
  • Tests: python3 -m pytest tests/test_threat_model.py -x -k "injection" passes with the renamed test (AC25). No new assertions are required — this step is a rename/re-docstring only.
  • Risks: If any other file references the old test name (e.g. a CI allowlist, a -k filter in tests/run_all.sh or documentation), grep for test_injection_probe_behavioral_equivalence across the repo before renaming and update any such reference in the same commit.

P14: Pin sandbox-resolution/execution command precedence [R14, AC26]

  • Files: [scripts/phases/test_phases.py]
  • Description: _candidate_command in scripts/phases/phase_review.py (line 56) already implements the required precedence: explicit_cmd is checked and returned first, unconditionally — before the resolver check — so it already wins even when a resolver is active; when only a resolver is active (explicit_cmd is None, resolver is not None), it returns None (no raw command inspected); when neither is set, it returns the legacy review_cmd. This step formalizes that behavior with a pinning test since none currently exists (confirmed: no test references _candidate_command or asserts sandbox/execution precedence in test_phases.py). If, during implementation, the precedence is found to have drifted from this description, fix _candidate_command to match R14's requirement before writing the test.
  • Dependencies: []
  • Tests: New tests in scripts/phases/test_phases.py calling _candidate_command (or exercising it indirectly through run_review with a mock resolve_sandbox_mode that records the command= argument it received) directly: (a) resolver active + explicit_cmd provided → sandbox resolution evaluates explicit_cmd (AC26, first case); (b) resolver active, no explicit_cmd → no raw command inspected (None passed) (AC26, second case); (c) no resolver, no explicit_cmd → the legacy review_cmd is used (AC26, third case).
  • Risks: _candidate_command is a module-private function (leading underscore) — import it directly from scripts.phases.phase_review in the test (matching how other private helpers in this codebase are tested, e.g. _partition_findings_by_file is tested directly in scripts/test_loop_fixes.py).

P15: Fix the pre-existing baseline test failure [R15, AC27]

  • Files: [scripts/test_loop_fixes.py]
  • Description: In test_provider_exhaustion_preserves_completed_concurrent_group (line 198), the monkeypatched run_fix has signature def run_fix(group, *args, **kwargs), but the real call site (scripts/adversarial_loop_v4.py, _process_group, ~line 828) calls phase_fix.run_fix(**fix_kwargs) with fix_kwargs = dict(findings=group, dev_cmd=..., workdir=..., ...) — entirely keyword arguments, with the findings keyword named findings, not group. Because the fake's first parameter is named group (not findings), the keyword call raises TypeError: run_fix() missing 1 required positional argument: 'group', which _process_group's broad except Exception as exc: return idx, {"error": str(exc), ...} (line 865-866) silently absorbs instead of letting the injected NoProviderAvailable propagate. Fix: rename the fake's parameter from group to findings (matching the real keyword), e.g. def run_fix(findings, *args, **kwargs): and use findings in place of group inside the function body (if findings[0]["id"] == "A1": raise ...).
  • Dependencies: []
  • Tests: python3 -m pytest scripts/test_loop_fixes.py -x -k test_provider_exhaustion_preserves_completed_concurrent_group passes: the test's own assertions already prove the two claims (provider exhaustion propagates as NoProviderAvailable; the completed group g1 is still cherry-picked) — fixing the monkeypatch signature is what makes those assertions actually exercise the real code path instead of masking it behind a TypeError.
  • Risks: None — this is a test-fixture bug fix, isolated to one function's monkeypatch signature; no production code changes.

P16: Full-branch review gate + full suite green [AC27]

  • Files: [scripts/adversarial_loop_v4.py, scripts/phases/phase_review.py, scripts/phases/phase_git.py, scripts/install.sh, .gitignore, SKILL.md, scripts/phases/test_phases.py, scripts/test_loop_fixes.py, tests/test_orchestrator.py, tests/test_threat_model.py, tests/test_contract_gate.py]
  • Description: Before this branch is proposed for merge, run a full-branch review over the complete diff (P1 through P15 together), not a per-commit review. Per-commit review is insufficient here by construction: P4 (_read_json fail-closed) and P5 (_write_manifest hardening) both touch scripts/adversarial_loop_v4.py's module-level imports and exception-handling structure — a per-commit review of each in isolation could miss that P5's new _atomic_write_text helper and P4's new JsonCorruptionError need to coexist cleanly in the same file's exception hierarchy; P7 (watchdog per-invocation ownership tracking) and P8 (cherry-pick classification) both touch the same _process_group/merge-step region and must be checked together for interaction (e.g. does a watchdog-killed process during a stall leave CHERRY_PICK_HEAD state that P8's classification logic must also handle?); P11's docstring change and P4/P5's exception-handling changes both touch commit/write paths that interact during a real FIX+merge cycle. A full-branch review reads all changed files as one diff and checks these cross-file interactions, which no single commit's diff can reveal in isolation (component decomposition is not the same as integration).
  • Dependencies: [P1, P2, P3, P4, P5, P6, P7, P8, P9, P10, P11, P12, P13, P14, P15]
  • Tests: python3 -m pytest scripts/ tests/ reports 0 failures; bash tests/run_all.sh exits 0 (AC27, both required for a fully green suite). Re-run the AC1–AC26 verification commands listed in each prior step's Tests field as a consolidated pre-merge checklist.
  • Risks: A step that individually passes its own tests can still break another step's assumptions once merged (e.g. P6's _partition_findings_by_file fix changes group boundaries, which changes how many concurrent _process_group calls P7's watchdog ownership tracking must isolate — P7's tests were written against the old partitioning behavior in isolation and must be re-verified against the merged tree). Any interaction found here is fixed in this step, not deferred.

Ordering rationale

P1-P3 (secret-scanning-bypass removal, brief-artifact ignore rule, install.sh pinning) are pure housekeeping/supply-chain changes with no code dependencies on anything else in the plan — they run first and can be built in any order relative to each other.

P4 (_read_json fail-closed) is sequenced before P5 (_write_manifest hardening) because both introduce error-handling/serialization patterns in the same module; doing the exception-class addition first keeps P5's _atomic_write_text helper free to focus purely on the write path without also having to reconcile a concurrent change to how _read_json fails. Neither strictly requires the other (both are independently buildable), but this order minimizes merge friction within the same file.

P6 (partition fix), P7 (watchdog isolation), and P8 (cherry-pick classification) all touch the concurrent FIX+VERIFY round machinery in adversarial_loop_v4.py, ordered so that partitioning (which determines how many concurrent groups exist) lands before the watchdog fix (which must isolate however many concurrent groups partitioning produces) and before the cherry-pick classification fix (which processes each group's merge independently of how many groups there are, but shares the same file region). None are hard prerequisites of each other, but this order follows the data flow: partition → fix → merge.

P9-P14 (contract-gate docs+test, read_files formalization, commit-policy docs+test, consensus docs+test, injection-test rename, sandbox-precedence test) are each self-contained single-requirement steps with no code dependencies on P1-P8 or each other — they are almost entirely documentation-plus-test additions pinning already-shipped behavior, and can be built in any order or in parallel.

P15 (fix the baseline test) is independent of every other step — it is a pre-existing bug unrelated to this campaign's other findings — but is sequenced near the end so that "full suite green" (AC27) is verified only once all other steps (which also add tests) are in place, avoiding a false-green suite that passes only because later steps haven't yet added their own tests.

P16 (full-branch review gate) depends on all of P1-P15: it is the mandated gate that inspects the complete cross-file diff together, catching interactions between steps that touch the same regions (P4+P5, P6+P7, P7+P8) which no individual step's isolated tests can reveal, and confirms the AC27 full-suite-green requirement holds for the merged whole, not just each step in isolation.