diff --git a/amplifier_app_cli/data/skills/ten-lane-highway/SKILL.md b/amplifier_app_cli/data/skills/ten-lane-highway/SKILL.md index ffc082f..9278854 100644 --- a/amplifier_app_cli/data/skills/ten-lane-highway/SKILL.md +++ b/amplifier_app_cli/data/skills/ten-lane-highway/SKILL.md @@ -89,12 +89,35 @@ repo** (a real run left `.amplifier/bin/` behind as untracked pollution). | `launch_lane.sh BATCH_DIR LANE REPO GOAL [BASE_REF]` | Worktree + branch + tmux + `/goal` session, idempotent; the ONLY writer of `manifest.tsv` | Hand-written manifests diverged on column count and broke a real batch | | `verify_lane.sh BATCH_DIR LANE` | Git-facts probe for one landed lane (DONE.json, ahead-count, three-dot diffstat, uncommitted work) | "Ground truth from git and the filesystem, not from what any session said about itself" | | `highway_watchdog.sh BATCH_DIR WIDTH SESSION_ID [INTERVAL] [MAX_HOURS]` | Detached tmux loop that re-wakes THIS session (`amplifier run --resume`) on lane-end / under-width / stale heartbeat | The highway once froze overnight because the manager stopped monitoring the moment it reported status | -| `infra_ledger.sh BATCH_DIR add TYPE ID DESTROY_CMD...` / `infra_ledger.sh BATCH_DIR sweep` | Records any infrastructure a lane OR the manager stands up (DTU, gitea instance, container, service, background process) into `infra.tsv` at creation, each with its teardown command; `sweep` runs those commands and exits non-zero until nothing is left standing | A run closed with a DTU and a gitea container still live — nothing the highway stands up should outlive it (Rule 14) | +| `infra_ledger.sh BATCH_DIR add TYPE ID DESTROY_CMD...` / `infra_ledger.sh BATCH_DIR sweep --all-owners` | Records any infrastructure a lane OR the manager stands up (DTU, gitea instance, container, service, background process) into `infra.tsv` at creation, each with its teardown command; `sweep` runs those commands and exits non-zero until nothing is left standing | A run closed with a DTU and a gitea container still live — nothing the highway stands up should outlive it (Rule 14) | + +**`sweep` is the MANAGER's batch-close verb, never a lane's.** It runs EVERY +open row's destroy command, so one lane calling it destroys every other lane's +live infrastructure — that is not hypothetical: on 2026-09-02 a single foreign +`sweep` took lane l1's three DTUs and lane 161's three, 35 minutes into their +measurements. The script now **refuses with exit 3, having run nothing**, when +the open rows span more than one owner or any row is unattributable, so: + +- **A lane tearing down its OWN rows uses the batch's lane-scoped teardown + tool** (`lane_teardown.sh BATCH_DIR teardown --yes`; omit `--yes` for a + dry run), which touches only the rows that lane claimed. A lane never calls + `sweep`. +- **The manager closing the batch passes `--all-owners`** — the deliberate + batch-close override. Every close instruction below says + `sweep --all-owners` for exactly this reason: a guard that deadlocks the + documented close is a regression, not a fix. + +A destroy command for infrastructure that is **already gone** closes its row as +`swept:already-absent` — distinct from `swept`, because the sweep did not +perform that teardown. A REAL teardown failure still exits non-zero and leaves +the row open; the already-gone signature is deliberately narrow, so the signal +that a teardown genuinely failed is never lost. State lives in `BATCH_DIR` (create one per highway, e.g. `~/dev/hw-`): `manifest.tsv` (scripts write), `HIGHWAY.md` (you write), `goals/` (pre-composed goal files), `lanes/` (worktrees), `.width` (authoritative width), `infra.tsv` -(the infra ledger), `.manager-heartbeat`, `wake-needed`, `watchdog.log`. +(the infra ledger), `infra.owners.tsv` (which lane claimed which row), +`.manager-heartbeat`, `wake-needed`, `watchdog.log`. ## Phase 1 — Intake @@ -143,7 +166,10 @@ Phase 5 invariant); an item arriving at weave-in gets its goal file composed when it enters the queue, not at refill. Each goal file MUST instruct its lane to register any infrastructure it stands up — DTU, gitea instance, container, service, background process — with `infra_ledger.sh add -` at creation (Rule 14). +` at creation (Rule 14), and to tear down **only its own rows** +via the batch's lane-scoped teardown tool. A goal file must never tell a lane to +run `sweep`: that is the manager's batch-close verb and it destroys every other +lane's live infrastructure. **Success criteria**: a priority queue in `HIGHWAY.md` with a one-line rationale per item tied to the outcome/constraints, and a pre-composed goal file @@ -310,9 +336,13 @@ outcome verified with nothing pending. When you close: final Phase 5 pass; merge or honestly disposition every open lane; then **run `infra_ledger.sh BATCH_DIR -sweep` and do not treat the highway as closed until it exits clean** — it tears +sweep --all-owners` and do not treat the highway as closed until it exits +clean** — it tears down every DTU, gitea instance, container, service, and background process the -run ledgered, whether a lane or the manager stood it up (Rule 14). Kill the +run ledgered, whether a lane or the manager stood it up (Rule 14). `--all-owners` +is the manager's batch-close override; without it `sweep` refuses (exit 3) the +moment the open rows span more than one lane, which is the guard that stops a +lane from destroying its neighbours' infrastructure. Kill the watchdog by the exact name `highway_status.sh` reports (`tmux -L hw kill-session -t `). **Archive the per-lane evidence BEFORE pruning** — pruning the lane dirs otherwise deletes @@ -328,8 +358,8 @@ status, landed list from `landed_from_git.sh`, residuals with named reasons); report with `DONE:` or `GAVE UP:` leading. **Success criteria**: no `hw__` tmux sessions, no stray worktrees/branches, -`infra_ledger.sh BATCH_DIR sweep` exits clean (nothing ledgered still standing), -final report matches git facts. +`infra_ledger.sh BATCH_DIR sweep --all-owners` exits clean (nothing ledgered +still standing), final report matches git facts. ## Rules — each bought with a documented failure @@ -367,8 +397,12 @@ final report matches git facts. highway stands up outlives it.** Any DTU, gitea instance, container, service, or background process a lane OR the manager stands up is recorded with `infra_ledger.sh BATCH_DIR add ...` at creation, and Phase 7 does not - close until `infra_ledger.sh BATCH_DIR sweep` exits clean. (A run closed - leaving a DTU and a gitea container running.) + close until `infra_ledger.sh BATCH_DIR sweep --all-owners` exits clean. + (A run closed leaving a DTU and a gitea container running.) **`sweep` is + the manager's verb; a lane tears down only its own rows via the batch's + lane-scoped teardown tool.** (One foreign `sweep` destroyed two other + lanes' DTUs mid-measurement; `sweep` now refuses a multi-owner ledger with + exit 3 unless `--all-owners` is passed.) ## Known limits (still not built) diff --git a/amplifier_app_cli/data/skills/ten-lane-highway/scripts/infra_ledger.sh b/amplifier_app_cli/data/skills/ten-lane-highway/scripts/infra_ledger.sh index 56802dc..5744e44 100755 --- a/amplifier_app_cli/data/skills/ten-lane-highway/scripts/infra_ledger.sh +++ b/amplifier_app_cli/data/skills/ten-lane-highway/scripts/infra_ledger.sh @@ -7,19 +7,32 @@ # Usage: # infra_ledger.sh BATCH_DIR add TYPE ID DESTROY_CMD... # infra_ledger.sh BATCH_DIR list -# infra_ledger.sh BATCH_DIR sweep +# infra_ledger.sh BATCH_DIR sweep [--all-owners] # # Rows live in BATCH_DIR/infra.tsv, one per line, tab-separated: # ts type id status destroy_cmd # THIS script is the ONLY writer of infra.tsv — never hand-edit that file. +# Rows are attributed to a lane by a sibling BATCH_DIR/infra.owners.tsv +# ts id lane +# written by the batch's lane-scoped teardown tool, not by this script. # # add Append one row with status=open. DESTROY_CMD... is the (possibly # multi-word) command that reclaims the resource; it is stored verbatim # and later run via `bash -c`. # list Print the open rows; always exit 0. -# sweep Run each OPEN row's destroy_cmd. On rc=0 mark it swept; otherwise -# leave it open and print the failure. Exit nonzero if any row is still -# open afterwards. Idempotent: already-swept rows are never re-run, so +# sweep THE MANAGER'S BATCH-CLOSE VERB, NEVER A LANE'S. It runs EVERY open +# row's destroy_cmd, so a lane calling it destroys other lanes' live +# infrastructure. A lane tearing down its OWN rows uses the batch's +# lane-scoped teardown tool (lane_teardown.sh) instead. +# Refuses with exit 3, having run NOTHING, when the open rows span +# more than one owner or any row is unattributable; the manager +# closing the batch passes --all-owners to proceed anyway. +# Otherwise, per open row: rc=0 marks it `swept`; a rc!=0 whose output +# matches the NARROW already-gone signature marks it +# `swept:already-absent` (recorded distinctly — the row is closed but +# this sweep did not perform the teardown); any other failure leaves +# the row open and is printed. Exit nonzero if any row is still open +# afterwards. Idempotent: already-swept rows are never re-run, so # re-sweeping a fully-swept ledger runs nothing and exits 0. # # NOT -e: a failing destroy_cmd during sweep is an expected, handled outcome — @@ -30,6 +43,12 @@ BATCH_DIR=${1:?BATCH_DIR required} CMD=${2:?command required (add|list|sweep)} LEDGER="$BATCH_DIR/infra.tsv" +# Signatures meaning "the thing you asked me to destroy does not exist". +# Deliberately NARROW: a blanket exit-code amnesty would destroy the signal +# that a REAL teardown failed, which is the whole reason sweep checks rc +# (Rule 14: nothing the highway stands up should outlive it). +ALREADY_GONE_RE=${ALREADY_GONE_RE:-"environment not found|not found|no such|does not exist|doesn't exist|unknown (environment|container|project)"} + case "$CMD" in add) TYPE=${3:?TYPE required} @@ -54,17 +73,60 @@ case "$CMD" in sweep) [ -f "$LEDGER" ] || { echo "SWEEP: no ledger ($LEDGER) - nothing to do"; exit 0; } + + # ---- MULTI-LANE GUARD (model_performance-0rg) ------------------------- + # sweep is the MANAGER's BATCH-CLOSE verb. Its only predicate used to be + # `status == open`, so the FIRST caller destroyed EVERY lane's live + # infrastructure. Observed 2026-09-02: one foreign sweep took lane l1's + # three DTUs and lane 161's three, 35 minutes into their measurements. + # A lane tearing down its OWN rows uses the lane-scoped tool: + # .amplifier/evaluation/tools/lane_teardown.sh BATCH_DIR LANE teardown + ALL_OWNERS=0 + for a in "$@"; do [ "$a" = "--all-owners" ] && ALL_OWNERS=1; done + if [ "$ALL_OWNERS" != 1 ]; then + OWNERS_FILE="$BATCH_DIR/infra.owners.tsv" + owners=$(awk -F'\t' -v of="$OWNERS_FILE" ' + BEGIN { while ((getline line < of) > 0) { split(line, f, "\t"); own[f[2]] = f[3] } } + $4 ~ /^open/ { print ($3 in own) ? own[$3] : "" } + ' "$LEDGER" | sort -u) + n=$(printf '%s\n' "$owners" | grep -c . || true) + if [ "${n:-0}" -gt 1 ] || printf '%s' "$owners" | grep -q '^&2 + printf '%s\n' "$owners" | sed 's/^/ owner: /' >&2 + echo "" >&2 + echo "sweep is the MANAGER's batch-close verb and destroys EVERY open row." >&2 + echo "A lane tearing down its OWN rows must use the lane-scoped tool:" >&2 + echo " .amplifier/evaluation/tools/lane_teardown.sh $BATCH_DIR teardown --yes" >&2 + echo "The manager closing the batch passes --all-owners." >&2 + exit 3 + fi + fi + # ---- end guard -------------------------------------------------------- + tmp=$(mktemp "$BATCH_DIR/.infra.XXXXXX") remaining=0 while IFS=$'\t' read -r ts type id status destroy; do [ -z "${ts:-}" ] && continue # skip blank lines if [ "$status" = "open" ]; then echo ">> sweeping type=$type id=$id: $destroy" - if bash -c "$destroy"; then + # Capture output so an "already gone" refusal can be recognised. + out=$(bash -c "$destroy" 2>&1); rc=$? + [ -n "$out" ] && printf '%s\n' "$out" + if [ "$rc" -eq 0 ]; then status=swept echo " swept ok" + elif printf '%s' "$out" | grep -qiE "$ALREADY_GONE_RE"; then + # model_performance-bqu: ALREADY GONE is the DESIRED end state of a + # destroy, not a failure. Before this, infrastructure torn down by any + # other path (a lane by hand, a manager recovery, a crash cleanup) + # left a row that could NEVER be closed: the destroy_cmd failed + # forever, so `sweep` never exited clean and SKILL.md's "do not treat + # the highway as closed until it exits clean" became unsatisfiable. + # Recorded DISTINCTLY from a real teardown so the two are never + # confused in the ledger. + status=swept:already-absent + echo " already absent - closing row (not a teardown this sweep performed)" else - rc=$? echo " FAILED (rc=$rc) type=$type id=$id: $destroy" >&2 remaining=$(( remaining + 1 )) fi diff --git a/docs/lanes/2nz-upstream-skill-guards/DONE-NOTE.md b/docs/lanes/2nz-upstream-skill-guards/DONE-NOTE.md new file mode 100644 index 0000000..57f55c1 --- /dev/null +++ b/docs/lanes/2nz-upstream-skill-guards/DONE-NOTE.md @@ -0,0 +1,239 @@ +# Lane 2nz — upstream the ten-lane-highway skill guards + +**Item:** `model_performance-2nz` (project `model_performance`) +**Repo:** `microsoft/amplifier-app-cli`, branch `lane/2nz-upstream-skill-guards` +**Date:** 2026-09-03 +**Outcome:** **A. RESOLVED** — every required deliverable DONE; the one +OPTIONAL-IF-CAP-PERMITS deliverable is NOT-POSSIBLE for a scope reason, not a cap reason. + +--- + +## What this item was + +Two manager fixes were applied by hand to the **installed** skill directory +(`~/.local/share/uv/tools/amplifier/.../amplifier_app_cli/data/skills/ten-lane-highway`). +An `amplifier` tool update re-installed that directory on 2026-09-03 at 05:54–06:13 +and **silently reverted them**. The reverted mechanism (`model_performance-0rg`'s +multi-lane sweep guard) is the thing that stops one lane's `sweep` from destroying +every other lane's DTUs — the failure that cost lanes l1 and 161 three DTUs each, +35 minutes into their measurements, on 2026-09-02. + +The fix is to put them where a tool update **delivers** them instead of deleting +them: this repo's shipped source. + +--- + +## Deliverables + +### 1. Both guards in this repo's shipped source — **DONE** + +`amplifier_app_cli/data/skills/ten-lane-highway/scripts/infra_ledger.sh`. + +**Ported, not redesigned.** Method: the installed file was copied over the repo +file **byte-for-byte** (`diff -q` clean immediately after the copy), so the +implementation in the PR is literally the measured one. The only subsequent edit +is the top-of-file usage block, which described a `sweep` that no longer exists — +that is documentation of the ported behaviour, not a change to it. The post-port +diff against the installed copy is exactly that comment block and nothing else. + +- **0rg multi-lane guard** (marker `MULTI-LANE GUARD`): `sweep` exits **3** and + runs **no destroy_cmd** when the open rows span more than one owner *or* any + row is unattributable. `--all-owners` is the manager's batch-close override. + Ownership is read from the sibling `infra.owners.tsv` (`ts \t id \t lane`). +- **bqu already-absent handling** (marker `ALREADY_GONE_RE`): a destroy_cmd + failing with a **narrow** not-found signature closes the row as + **`swept:already-absent`** — distinct from `swept`. A real failure still exits + non-zero and leaves the row `open`. + +Both markers remain greppable, so the manager's stopgap +`$BATCH_DIR/check_skill_guards.sh` still finds them: run against the repo source +it prints `skill guards OK (ten-lane-highway)`, exit 0. + +### 2. SKILL.md updated in the same change — **DONE** + +`amplifier_app_cli/data/skills/ten-lane-highway/SKILL.md`, five sites: + +- Instruments table entry now reads `sweep --all-owners`, followed by a block + stating plainly that **`sweep` is the manager's batch-close verb, never a + lane's**, naming the lane-scoped `lane_teardown.sh` as the alternative, and + documenting `swept:already-absent`. +- Phase 2 (goal-file composition): a goal file must never tell a lane to run + `sweep`. +- Phase 7 close instruction: `sweep --all-owners`. +- Phase 7 success criteria: `sweep --all-owners`. +- Rule 14: `sweep --all-owners`, plus the manager-verb/lane-tool split. + +**Note this could not be "ported":** the `--all-owners` documentation was *also* +reverted and was **never re-applied to the installed copy** — `grep -c "all-owners"` +is **0** in both the installed SKILL.md and this repo's pre-change SKILL.md. It was +authored fresh here to 0rg's stated requirement (guard + docs land together, because +a guard that deadlocks the documented close is a regression). + +### 3. Tests in this repo's own suite — **DONE** + +`tests/test_ten_lane_highway_infra_ledger.py` — 12 tests, all five measured cases +plus regression coverage: + +| # | Case | Test | +|---|---|---| +| 1 | already-gone closes, exit 0, `swept:already-absent` | `test_case1_already_gone_closes_row_as_already_absent` | +| 2 | REAL failure exits non-zero, row stays `open` | `test_case2_real_failure_exits_nonzero_and_leaves_row_open` | +| 3 | genuine teardown records `swept` | `test_case3_genuine_teardown_records_swept`, `test_case3_mixed_outcomes_are_recorded_distinctly` | +| 4 | multi-owner without the flag: exit 3, ran nothing | `test_case4_multi_owner_sweep_refuses_and_runs_nothing`, `test_case4_unattributed_rows_also_refuse` | +| 5 | `--all-owners` proceeds | `test_case5_all_owners_proceeds` | + +Plus `test_single_owner_sweep_is_allowed_without_the_flag` (the guard fires on +ambiguity, not on sweeping), `test_sweep_is_idempotent` (closed rows — including +`swept:already-absent` — are never re-run, so the fix does not merely move the +deadlock), `test_skill_md_documents_the_manager_override`, and two +`test_guard_markers_are_greppable` cases pinning the drift-check markers. + +**Observable destroy_cmd, as required:** every guard case uses `touch ` +and asserts the sentinel's **absence**. "Ran nothing" is proven, not inferred — +an exit code alone cannot distinguish *refused before acting* from *acted and then +failed*. + +**Discriminating evidence (the tests are not vacuous):** run against the +pre-port script (`git checkout -- infra_ledger.sh`), **7 of 12 fail** — both +marker tests, case 1, case 3-mixed, both case-4 tests, and idempotence. Against +the ported script, 12/12 pass. Cases 2, 3 and 5 pass on both by design: they are +regression guards on behaviour that must *not* change. + +Windows: module-level skip (POSIX shell script; `bash`/`mktemp`/`awk`), following +the precedent already set by this repo's pty tests. + +### 4. Has anything ELSE in this skill dir drifted? — **DONE. Yes, one more thing, and it is now closed.** + +Method: `diff -rq` of this repo's `amplifier_app_cli/data/skills/` against the +installed tree (installed version 0.1.1, same as this checkout, so a version skew +cannot explain a difference). + +- **Whole shipped skills tree: exactly ONE file differed** — + `ten-lane-highway/scripts/infra_ledger.sh`. Every other file in every other + shipped skill was byte-identical. No further silent manager patches are sitting + in the installed tree. +- **A second, still-live drift, in the other direction:** `SKILL.md` was + byte-identical between repo and installed — and `--all-owners` appeared in + **neither** (`grep -c` = 0 in both). So cycle 49's re-apply restored the *guard* + but not its *documentation*. Until this PR, the shipped skill would have told a + manager to close a batch with a bare `sweep`, which the restored guard refuses + with exit 3 the moment two lanes hold infrastructure — a documented close that + cannot succeed. Closed here by deliverable 2. +- **`merge_gate.sh` is not in this skill dir in either copy, and never was** — + see below. + +### 5. Full suite green — **DONE** + +`uv run pytest -q` → **1682 passed, 1 skipped, 13 deselected, 1 xfailed in 12.05s**. +Pasted in the PR body. + +### 6. `tj2`'s `merge_gate.sh` fix (OPTIONAL-IF-CAP-PERMITS) — **NOT-POSSIBLE (scope, not cap)** + +**What was executed:** located the file, confirmed its repo, confirmed this repo +never ships it. `merge_gate.sh` exists at +`/home/bkrabach/dev/openai-evals-team-ci/.amplifier/evaluation/tools/merge_gate.sh` +(11,553 bytes, mtime 2026-09-02 18:29), alongside `test_merge_gate.sh` and +`README-merge-gate.md`. It is a tool of the **openai-evals-team-ci** repo, not a +ten-lane-highway skill script: it is absent from `data/skills/ten-lane-highway/scripts/` +in **both** this repo and the installed copy, so nothing was reverted from here and +there is nothing here to upstream. + +**Reason it is NOT-POSSIBLE in this lane:** it lives in another repo, and this +lane's scope-outs forbid touching other repos. It is not blocked by the cap +(the cap is $0 and nothing here costs money) and not blocked by inability to +find it. **Recommended follow-up:** a separate item against +`openai-evals-team-ci` — its tools directory is version-controlled by that repo, +so tj2's fix is durable there already *if it is committed*; that is the thing worth +checking, and it is checkable at zero cost. + +### 7. Draft PR + this note — **DONE** + +**Draft PR: [microsoft/amplifier-app-cli#299](https://github.com/microsoft/amplifier-app-cli/pull/299)** on branch +`lane/2nz-upstream-skill-guards`. Publication values in `DONE.json` were read back from +the remote with `publication_readback.sh` (`git ls-remote` + `gh pr list`), not typed +from a local `git log`. + +--- + +## Spend + +**Authority: $0.00 — arithmetic `0 runs × 0 arms × $0.00 / 1.00 valid = $0.00`, slack $0.00.** + +**The arithmetic closes, and it closes trivially.** This deliverable is a pure +source change: no model runs, no arms, no validity rate to divide by. There is no +smallest-indivisible-purchase problem here — the deliverable's price is genuinely +zero, so a $0 authority funds 100% of it. (Contrast lane 1ru, whose authority was +sized for 2 valid runs against a goal asking for 4; nothing of that shape applies.) + +- **API / DTU spend: $0.00.** No DTU, gitea instance, container, service or + background process was created. **Nothing registered in the infra ledger; nothing + to tear down.** `infra_ledger.sh ... sweep` was never run against a real batch + ledger — every sweep in this lane ran against throwaway ledgers in pytest + `tmp_path` directories. +- Agent tokens for this lane are outside the item's $0 API/DTU authority and were + not separately metered. + +--- + +## Deviations and judgment calls + +1. **The SKILL.md `--all-owners` text is authored, not ported.** There was nothing + to port — it was 0 in both copies (see deliverable 4). "Do NOT redesign" was + honoured for the two *guards*, which were copied byte-for-byte; it cannot apply + to prose that does not exist anywhere. Recorded rather than silently absorbed. +2. **The script's usage header was rewritten.** It documented a `sweep` with no + `--all-owners` and no `swept:already-absent`. Leaving it would have shipped a + file that contradicts itself. This is the only post-copy edit to the script and + it touches comments only. +3. **`lane_teardown.sh` is named in SKILL.md without a repo-relative path.** The + ported guard's own error message hard-codes + `.amplifier/evaluation/tools/lane_teardown.sh`, which is an evals-repo path; + SKILL.md ships to every consumer of this skill, so it names the tool by name and + role ("the batch's lane-scoped teardown tool") rather than by a path that will + not exist in most batches. The guard's message was left verbatim as measured. +4. **Windows skip on the new test module.** Chosen over a CI deselect, per this + repo's own CI comment ("an excluded test is a test nobody is watching"). +5. **Artifact root is `docs/lanes/2nz-upstream-skill-guards/`, as the goal names — + and a goal-vs-checker conflict is reported rather than silently resolved.** + The goal names this path twice, once inside SCOPE-OUTS. Run against this repo, + `check_lane_artifact_paths.py` instead resolves to **`ai_working//` + [R2 ai_working/]** (R2 fires because `ai_working/` is tracked at the base ref, + so the `docs/lanes/` fallback R3 never applies) and will grade this lane a + VIOLATION. + + **Both conventions have precedent at `origin/main` in this very repo** — + `docs/lanes/` holds `eem-partial-accumulator-widen`, + `9w0-delegate-timeout-partial-producer` and `n1i-resume-thread-role`; + `ai_working/` holds `3yc-…`, `9kk-…`, `adq-…` and others. So this is not a lane + inventing a location: it is two live conventions in one repo, with the goal + template pointing at one and the checker at the other. + + The goal wins here: it is the authoritative spec for this lane, its path has + three precedents at HEAD, and the SCOPE-OUT's actual hazard — the repo-root + `DONE-NOTE.md` that item `kez` was filed for, where every lane silently + overwrote the last — is avoided by either location. **This lane initially + placed the note under `ai_working/` on the checker's say-so; that was wrong, + and it is corrected here.** Treating a checker's preference as outranking an + explicit instruction is exactly the substitution the goal warns against. + + **For the manager, not for this lane to decide:** `artifact-path/v1` and the + goal template disagree for `amplifier-app-cli`. One of the two should move. + Until it does, every lane in this repo gets graded against a rule its own goal + contradicts. +6. **No `BLOCKED.md`.** Outcome branch A; the item resolves. + +--- + +## The finding worth keeping + +The revert itself is the signal, and it is now recorded in three places that a +tool update cannot erase: the shipped source, a test that fails without it, and +this note. + +But note what the diff caught that the re-apply did not: **the guard came back +without its documentation.** A partial re-apply is *more* dangerous than a total +one, because the surviving half looks like the whole thing. `check_skill_guards.sh` +greps only for the two code markers and would have reported `skill guards OK` +against a SKILL.md that still told the manager to run a `sweep` the guard refuses. +Worth extending that check with a third assertion — `--all-owners` present in +SKILL.md — so the docs half is watched too. diff --git a/tests/test_ten_lane_highway_infra_ledger.py b/tests/test_ten_lane_highway_infra_ledger.py new file mode 100644 index 0000000..2b2cfdc --- /dev/null +++ b/tests/test_ten_lane_highway_infra_ledger.py @@ -0,0 +1,294 @@ +"""Guards on the shipped ten-lane-highway ``infra_ledger.sh``. + +Both behaviours asserted here were fixed by hand in an *installed* copy of this +skill (``~/.local/share/uv/tools/amplifier/.../data/skills/ten-lane-highway``) +and a routine ``amplifier`` tool update re-installed that directory and silently +reverted them (mtimes 2026-09-03 05:54-06:13). Nothing warned; the next symptom +would have been DTUs vanishing mid-measurement. The fixes only stay fixed if +they live in the shipped source -- which is this file's subject -- and only stay +*alive* if something re-runs them, which is this file's job. + +The five cases below are the ones measured by hand before the revert: + +1. a destroy for infrastructure already gone closes the row, exit 0, + status ``swept:already-absent`` +2. a REAL destroy failure exits non-zero and leaves the row ``open`` +3. a genuine teardown still records plain ``swept`` +4. a ledger whose open rows span >1 owner refuses with exit 3 **having run no + destroy command at all** +5. ``--all-owners`` (the manager's batch-close override) proceeds + +Case 4 uses an *observable* destroy command (``touch ``) so "ran +nothing" is proven by the absent sentinel rather than inferred from an exit +code -- an exit code alone cannot distinguish "refused" from "ran and failed". +""" + +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +# The script is POSIX shell (mktemp/awk/sed/grep, tab-delimited read loops) and +# is only ever invoked on the POSIX hosts that run a highway. Skipping at module +# level on Windows follows the precedent already set by the pty tests, which +# likewise exercise a mechanism Windows has no equivalent of. +pytestmark = pytest.mark.skipif( + sys.platform == "win32" or shutil.which("bash") is None, + reason="infra_ledger.sh is a POSIX shell script; requires bash", +) + +SCRIPT = ( + Path(__file__).resolve().parent.parent + / "amplifier_app_cli" + / "data" + / "skills" + / "ten-lane-highway" + / "scripts" + / "infra_ledger.sh" +) + + +def run_ledger(batch_dir: Path, *args: str) -> subprocess.CompletedProcess: + """Invoke the shipped script exactly as a highway would.""" + return subprocess.run( + ["bash", str(SCRIPT), str(batch_dir), *args], + capture_output=True, + text=True, + timeout=60, + ) + + +def add_row(batch_dir: Path, kind: str, ident: str, destroy: str) -> None: + proc = run_ledger(batch_dir, "add", kind, ident, *destroy.split()) + assert proc.returncode == 0, proc.stderr + + +def own(batch_dir: Path, ident: str, lane: str) -> None: + """Attribute a ledger row to a lane, as the lane-scoped teardown tool does.""" + with (batch_dir / "infra.owners.tsv").open("a", encoding="utf-8") as fh: + fh.write(f"2026-09-03T00:00:00Z\t{ident}\t{lane}\n") + + +def rows(batch_dir: Path) -> dict[str, str]: + """Map ledger id -> status.""" + out = {} + for line in (batch_dir / "infra.tsv").read_text(encoding="utf-8").splitlines(): + if not line.strip(): + continue + fields = line.split("\t") + out[fields[2]] = fields[3] + return out + + +# --------------------------------------------------------------------------- +# The two guards must remain FINDABLE by marker, not merely present in spirit. +# +# The manager's stopgap drift check greps the installed tree for exactly these +# two strings and names which fix is missing when one is absent. If a later +# refactor renames them, that check goes quietly blind -- the precise failure +# mode this whole item exists to close. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("marker", "item"), + [ + ("MULTI-LANE GUARD", "model_performance-0rg"), + ("ALREADY_GONE_RE", "model_performance-bqu"), + ], +) +def test_guard_markers_are_greppable(marker: str, item: str) -> None: + text = SCRIPT.read_text(encoding="utf-8") + assert marker in text, f"drift-check marker '{marker}' ({item}) missing from shipped source" + + +# --------------------------------------------------------------------------- +# CASE 1-3: what a destroy_cmd's outcome does to its row. +# --------------------------------------------------------------------------- + + +def test_case1_already_gone_closes_row_as_already_absent(tmp_path: Path) -> None: + """Already-gone is the DESIRED end state of a destroy, not a failure. + + Before this, infrastructure torn down by any other path left a row that + could never be closed: the destroy_cmd failed forever, so `sweep` never + exited clean and "do not treat the highway as closed until sweep exits + clean" became unsatisfiable. + """ + add_row(tmp_path, "dtu", "gone-1", "bash -c 'echo environment not found >&2; exit 1'") + own(tmp_path, "gone-1", "lane-a") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + # Distinct from a teardown this sweep actually performed. + assert rows(tmp_path)["gone-1"] == "swept:already-absent" + + +def test_case2_real_failure_exits_nonzero_and_leaves_row_open(tmp_path: Path) -> None: + """A REAL failure must stay loud. + + "Already gone" is deliberately NOT a blanket exit-code amnesty: that would + destroy the signal that a teardown genuinely failed, which is the entire + reason sweep checks rc at all (Rule 14 -- nothing the highway stands up + should outlive it). + """ + add_row(tmp_path, "dtu", "broken-1", "bash -c 'echo permission denied >&2; exit 1'") + own(tmp_path, "broken-1", "lane-a") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode != 0, f"a real teardown failure exited 0: {proc.stdout}" + assert rows(tmp_path)["broken-1"] == "open" + + +def test_case3_genuine_teardown_records_swept(tmp_path: Path) -> None: + sentinel = tmp_path / "DESTROYED" + add_row(tmp_path, "dtu", "live-1", f"touch {sentinel}") + own(tmp_path, "live-1", "lane-a") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + assert sentinel.exists(), "the destroy_cmd did not run" + assert rows(tmp_path)["live-1"] == "swept" + + +def test_case3_mixed_outcomes_are_recorded_distinctly(tmp_path: Path) -> None: + """The two closed statuses must never collapse into one another.""" + sentinel = tmp_path / "DESTROYED" + add_row(tmp_path, "dtu", "live-1", f"touch {sentinel}") + add_row(tmp_path, "dtu", "gone-1", "bash -c 'echo no such container >&2; exit 1'") + own(tmp_path, "live-1", "lane-a") + own(tmp_path, "gone-1", "lane-a") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + assert rows(tmp_path) == {"live-1": "swept", "gone-1": "swept:already-absent"} + + +# --------------------------------------------------------------------------- +# CASE 4-5: who is allowed to run the batch-close verb. +# --------------------------------------------------------------------------- + + +def test_case4_multi_owner_sweep_refuses_and_runs_nothing(tmp_path: Path) -> None: + """The DTU-protection guard. + + `sweep` runs EVERY open row's destroy command, so one lane calling it + destroys every other lane's live infrastructure. On 2026-09-02 a single + foreign sweep took lane l1's three DTUs and lane 161's three, 35 minutes + into their measurements. + + The sentinels are the point: an exit code alone cannot tell "refused before + doing anything" apart from "ran the destroys and then failed". + """ + sentinel_a = tmp_path / "DESTROYED_A" + sentinel_b = tmp_path / "DESTROYED_B" + add_row(tmp_path, "dtu", "a-1", f"touch {sentinel_a}") + add_row(tmp_path, "dtu", "b-1", f"touch {sentinel_b}") + own(tmp_path, "a-1", "lane-a") + own(tmp_path, "b-1", "lane-b") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 3, f"expected refusal (exit 3), got {proc.returncode}: {proc.stdout}" + assert not sentinel_a.exists(), "a destroy_cmd RAN despite the refusal" + assert not sentinel_b.exists(), "a destroy_cmd RAN despite the refusal" + assert rows(tmp_path) == {"a-1": "open", "b-1": "open"} + # A bare refusal is not actionable at the moment it fires; it must name the + # override and the lane-scoped alternative. + assert "--all-owners" in proc.stderr + assert "lane_teardown.sh" in proc.stderr + + +def test_case4_unattributed_rows_also_refuse(tmp_path: Path) -> None: + """A row nobody claimed cannot be shown to be safe to destroy.""" + sentinel = tmp_path / "DESTROYED" + add_row(tmp_path, "dtu", "orphan-1", f"touch {sentinel}") + # No infra.owners.tsv entry at all. + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 3 + assert not sentinel.exists(), "an unattributed row was destroyed" + assert rows(tmp_path)["orphan-1"] == "open" + + +def test_case5_all_owners_proceeds(tmp_path: Path) -> None: + """The manager's batch-close override. + + This case is load-bearing in the other direction: a guard that deadlocks + the documented close is a regression, not a fix. Phase 7 cannot close until + sweep exits clean, so the override has to work. + """ + sentinel_a = tmp_path / "DESTROYED_A" + sentinel_b = tmp_path / "DESTROYED_B" + add_row(tmp_path, "dtu", "a-1", f"touch {sentinel_a}") + add_row(tmp_path, "dtu", "b-1", f"touch {sentinel_b}") + own(tmp_path, "a-1", "lane-a") + own(tmp_path, "b-1", "lane-b") + + proc = run_ledger(tmp_path, "sweep", "--all-owners") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + assert sentinel_a.exists() and sentinel_b.exists() + assert rows(tmp_path) == {"a-1": "swept", "b-1": "swept"} + + +def test_single_owner_sweep_is_allowed_without_the_flag(tmp_path: Path) -> None: + """The guard fires on ambiguity, not on sweeping as such.""" + sentinel = tmp_path / "DESTROYED" + add_row(tmp_path, "dtu", "a-1", f"touch {sentinel}") + own(tmp_path, "a-1", "lane-a") + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + assert sentinel.exists() + + +def test_sweep_is_idempotent(tmp_path: Path) -> None: + """Re-sweeping a fully-swept ledger runs nothing and exits 0. + + Closed rows -- including ``swept:already-absent`` ones -- must not be + re-attempted, or the already-absent fix would merely move the deadlock. + """ + sentinel = tmp_path / "DESTROYED" + add_row(tmp_path, "dtu", "live-1", f"touch {sentinel}") + add_row(tmp_path, "dtu", "gone-1", "bash -c 'echo does not exist >&2; exit 1'") + own(tmp_path, "live-1", "lane-a") + own(tmp_path, "gone-1", "lane-a") + + assert run_ledger(tmp_path, "sweep").returncode == 0 + sentinel.unlink() + + proc = run_ledger(tmp_path, "sweep") + + assert proc.returncode == 0, f"stdout={proc.stdout}\nstderr={proc.stderr}" + assert not sentinel.exists(), "a already-closed row's destroy_cmd was re-run" + assert rows(tmp_path) == {"live-1": "swept", "gone-1": "swept:already-absent"} + + +def test_skill_md_documents_the_manager_override(tmp_path: Path) -> None: + """0rg's own acceptance required the guard and its docs to land together. + + Without ``--all-owners`` in the close instructions, the guard the manager + must pass through is undocumented at exactly the moment it fires. + """ + skill_md = SCRIPT.parent.parent / "SKILL.md" + text = skill_md.read_text(encoding="utf-8") + assert "--all-owners" in text + assert "lane_teardown.sh" in text + # Every close instruction names the override, not a bare `sweep`. + close_lines = [ + line + for line in text.splitlines() + if "infra_ledger.sh" in line and "sweep" in line and "add " not in line + ] + assert close_lines, "SKILL.md no longer documents sweep at all" + for line in close_lines: + assert "--all-owners" in line, f"close instruction omits the override: {line!r}"