Skip to content

fix(skills/ten-lane-highway): ship the two sweep guards a tool update kept reverting - #299

Merged
Brian Krabach (bkrabach) merged 3 commits into
mainfrom
lane/2nz-upstream-skill-guards
Sep 3, 2026
Merged

fix(skills/ten-lane-highway): ship the two sweep guards a tool update kept reverting#299
Brian Krabach (bkrabach) merged 3 commits into
mainfrom
lane/2nz-upstream-skill-guards

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

What happened

Two 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 — no warning, no version check.

The reverted mechanism is the one that stops one lane's sweep from destroying every
other lane's DTUs. 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.
For some window on 2026-09-03 that harm was live again. Nothing was lost only because
every live lane that cycle happened to have no infrastructure registered — luck, not design.

It was found only because a later item's acceptance criteria happened to require
re-testing the case, and the case was run rather than assumed.

Re-applying by hand after every update is not a fix. It requires (a) knowing an update
happened and (b) remembering every fix ever applied. Neither is true of a system that has
already forgotten twice. This PR puts them where a tool update delivers them.

Ported, not redesigned

The installed file was copied over the repo file byte-for-byte (diff -q clean
immediately after), so the implementation here is literally the measured one. The only
subsequent edit is the top-of-file usage block, which described a sweep that no longer
exists.

  • Multi-lane guard (model_performance-0rg, 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.
  • Already-absent handling (model_performance-bqu, 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 — deliberately not a blanket exit-code amnesty,
    or the signal that a teardown genuinely failed would be destroyed.

SKILL.md lands in the same change — and this half could not be ported

0rg's own acceptance required guard and docs together: a guard that deadlocks the
documented close is a regression, not a fix.
sweep is now documented as the manager's
batch-close verb (never a lane's), the lane-scoped teardown tool is named as the
alternative, and every close instruction says sweep --all-owners.

grep -c "all-owners" was 0 in both the installed SKILL.md and this repo's — so the
re-apply restored the guard but not its documentation. Until this PR the shipped skill
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.
A partial re-apply is worse than a total one, because the surviving half looks like the
whole thing.

Tests — and why they are not vacuous

tests/test_ten_lane_highway_infra_ledger.py, 12 tests covering all five measured cases:

# Case Result
1 already-gone closes, exit 0, swept:already-absent
2 REAL failure exits non-zero, row stays open
3 genuine teardown records swept
4 multi-owner without the flag: exit 3, ran nothing
5 --all-owners proceeds

Plus idempotence (closed rows — including swept:already-absent — are never re-run, so the
fix does not merely move the deadlock), single-owner-allowed, a SKILL.md docs assertion, and
two tests pinning the two drift-check markers so a later rename cannot blind the manager's
stopgap check.

Observable destroy_cmd. Every guard case uses touch <sentinel> 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. Against the pre-port script, 7 of 12 fail:

FAILED test_guard_markers_are_greppable[MULTI-LANE GUARD-model_performance-0rg]
FAILED test_guard_markers_are_greppable[ALREADY_GONE_RE-model_performance-bqu]
FAILED test_case1_already_gone_closes_row_as_already_absent
FAILED test_case3_mixed_outcomes_are_recorded_distinctly
FAILED test_case4_multi_owner_sweep_refuses_and_runs_nothing
FAILED test_case4_unattributed_rows_also_refuse
FAILED test_sweep_is_idempotent
7 failed, 5 passed in 0.19s

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), following this repo's pty-test
precedent rather than a CI deselect.

Did anything else drift?

diff -rq of this repo's whole amplifier_app_cli/data/skills/ tree against the installed
tree (installed version 0.1.1, same as this checkout, so version skew cannot explain a
difference): exactly one file differedinfra_ledger.sh. Every other file in every
other shipped skill was byte-identical. No further silent manager patch is sitting in the
installed tree.
The only other drift is the SKILL.md documentation gap described above,
which drifted in the opposite direction (missing from both) and is closed here.

merge_gate.sh is not in this skill dir in either copy and never was — it belongs to the
evals repo's .amplifier/evaluation/tools/, so there is nothing to upstream here.

Full suite

1682 passed, 1 skipped, 13 deselected, 1 xfailed in 8.22s

The manager's stopgap drift check run against this branch's source:

$ check_skill_guards.sh amplifier_app_cli/data/skills/ten-lane-highway
skill guards OK (ten-lane-highway)   # exit 0

Spend

$0.00. Authority 0 runs × 0 arms × $0.00 / 1.00 valid = $0.00. Pure source change; no
DTU, container, or service was created, nothing was registered in the infra ledger, and every
sweep exercised in this lane ran against throwaway pytest tmp_path ledgers — never a real
batch ledger.

Lane note: docs/lanes/2nz-upstream-skill-guards/DONE-NOTE.md.

…pdate kept reverting

Both fixes below were applied by hand to the INSTALLED skill dir
(~/.local/share/uv/tools/amplifier/.../data/skills/ten-lane-highway) and an
`amplifier` tool update re-installed that directory on 2026-09-03 at 05:54-06:13
and silently reverted them. No warning, no version check. The reverted mechanism
is the one that stops a 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. It was found only because a later item's acceptance
happened to re-test the case, and the case was run instead of assumed.

Re-applying by hand after each update requires knowing an update happened AND
remembering every fix ever applied. Neither is true of a system that has already
forgotten twice. The fix is to put them where a tool update DELIVERS them.

PORTED, NOT REDESIGNED. The installed file was copied over the repo file
byte-for-byte, so the implementation here is literally the measured one; the only
subsequent edit is the usage header, which described a `sweep` that no longer
exists.

  * multi-lane guard (model_performance-0rg): `sweep` exits 3 and runs NO
    destroy_cmd when open rows span >1 owner or any row is unattributable.
    `--all-owners` is the manager's batch-close override.
  * already-absent handling (model_performance-bqu): 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, so the signal that a teardown genuinely failed is never lost.

SKILL.md lands in the same change because 0rg's acceptance required it: a guard
that deadlocks the documented close is a regression, not a fix. `sweep` is now
documented as the manager's batch-close verb (never a lane's), the lane-scoped
teardown tool is named as the alternative, and every close instruction says
`sweep --all-owners`. Note this half could not be ported -- `--all-owners` was 0
matches in BOTH the installed and repo SKILL.md, i.e. cycle 49's re-apply restored
the guard but not its docs. A partial re-apply is worse than a total one, because
the surviving half looks like the whole thing.

Tests cover all five measured cases and use an observable destroy_cmd
(`touch <sentinel>`) so "ran nothing" is PROVEN, not inferred from an exit code.
Against the pre-port script 7 of 12 fail; against this one, 12/12 pass.

Full skills-tree diff vs the installed copy: exactly one file had drifted
(infra_ledger.sh). No other silent manager patch is sitting in the installed tree.
…names

The goal names `docs/lanes/2nz-upstream-skill-guards/DONE-NOTE.md` twice, once
inside SCOPE-OUTS. This lane first placed it under `ai_working/` because
`check_lane_artifact_paths.py` resolves this repo to `ai_working/<lane>/`
[R2 ai_working/]. That was the wrong call: a checker's preference does not
outrank an explicit instruction in the spec, and the goal's path has three
precedents at origin/main in this very repo (eem-partial-accumulator-widen,
9w0-delegate-timeout-partial-producer, n1i-resume-thread-role).

The SCOPE-OUT's real hazard -- the repo-root DONE-NOTE.md that item kez was
filed for, where every lane silently overwrote the last -- is avoided either
way; the disagreement is only between two live conventions in one repo.

Recorded in the note as a goal-vs-checker conflict for the manager to settle,
not resolved unilaterally: artifact-path/v1 and the goal template disagree for
amplifier-app-cli, and until one of them moves, every lane here is graded
against a rule its own goal contradicts.
@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review September 3, 2026 14:01
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Manager verification — the durability fix for today's silent guard revert. Merging.

Head 7e0c813, base 8c83a9b4 = current origin/main. CI green on all 9 jobs.

(a) Drift — I re-ran the comparison myself, not from the marker

diff -rq of origin/main's whole shipped ten-lane-highway tree against the installed copy:

Files .../scripts/infra_ledger.sh ... differ

Exactly one file, and only one. So no other silent manager patch is hiding in that tree — which is the thing I most wanted to know after today's revert, and it is the reassuring answer.

(b) Guards present, and the tests are the strong kind

MULTI-LANE GUARD ×1 and ALREADY_GONE_RE ×2 in the PR's file, ported byte-for-byte (the only edit is the usage-comment header, which previously documented a sweep with no --all-owners and no swept:already-absent — leaving it would have shipped instructions the code refuses).

The tests RUN the thing, they do not grep for it. Of 11 functions only test_guard_markers_are_greppable is a marker check; the rest invoke sweep via subprocess and assert real outcomes — returncode == 3 for the refusal, and an observable touch <sentinel> destroy_cmd (29 uses) so "ran nothing" is proven by the sentinel's absence rather than inferred from an exit code. That is exactly the distinction that matters here: a guard that returns 3 after destroying things would pass a weaker test.

Fail-before against origin/main's source: 8 failed, 4 passed. The marker claims 7 — I measured one more failure, not fewer, so the fail-before is stronger than advertised (the extra is test_skill_md_documents_the_manager_override, which fails on main because of the doc gap below). Full suite 1682 passed, 1 skipped, 13 deselected, 1 xfailed.

(c) The --all-owners doc gap — closed, and it was the sharper half of the finding

--all-owners matches:  PR SKILL.md 8   |   repo origin/main 0   |   INSTALLED copy 0

So before this PR, an update would have shipped the guard but not its documented escape hatch — and the lane's transferable finding names why that is worse than a clean revert: "A PARTIAL re-apply is more dangerous than a total revert, because the surviving half looks like the whole thing." My own check_skill_guards.sh greps only the two code markers, so it printed skill guards OK against a state whose SKILL.md still told the manager to close a batch with a bare sweep — which the restored guard refuses with exit 3 the moment two lanes hold infrastructure. I am extending that check with a third assertion as a direct result.

Also correctly dispositioned rather than skipped: tj2's merge_gate.sh is NOT-POSSIBLE here — located at openai-evals-team-ci/.amplifier/evaluation/tools/merge_gate.sh, absent from this repo's skills tree and the installed copy, so nothing was reverted from here and there is nothing here to upstream.

One flagged conflict worth routing, not blocking: check_lane_artifact_paths.py resolves this repo to ai_working/<lane>/ while the goal template says docs/lanes/<lane>/, and both have precedent at HEAD. The goal is a lane's authoritative spec so the lane followed it; the two rules should be reconciled.

Squash + --admin per the base-branch policy.

@bkrabach
Brian Krabach (bkrabach) merged commit 35ab604 into main Sep 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants