Skip to content

refactor(rqa): cut the legacy estate over and delete what nothing claims (#2188) - #2276

Open
tucktuck101 wants to merge 7 commits into
launchpadfrom
feature/2188-estate-tidyup
Open

tucktuck101 wants to merge 7 commits into
launchpadfrom
feature/2188-estate-tidyup

Conversation

@tucktuck101

@tucktuck101 tucktuck101 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Maps and removes the superseded legacy estate, and publishes the replacement CLI operator guide. Integrates the CLI reliability fixes and updates the documented authority, platform and exit-code behaviour. The milestone trace replacement remains unfinished under #2273, so this PR does not close the feature.

Feature

#2188

Related issue

Refs #2212
Refs #2213
Refs #2214
Refs #2188
Refs #2274
Refs #2006

Issue type

Feature


Agent provenance

Field Value
Harness / provider omp (workmux lanes; two orchestrators across a session boundary, three worker lanes, six gate rounds)
Model Orchestrators anthropic/claude-opus-5. Lanes: #2212 anthropic/claude-sonnet-5; #2213 anthropic/claude-opus-5; #2214 anthropic/claude-sonnet-5. Review seats: openrouter/z-ai/glm-5.3, openai-codex/gpt-5.6-sol, openrouter/z-ai/glm-5.3-flash, openai-codex/gpt-5.6-luna
Session reference N/A - the harness exposes no durable run URL; the run's durable record is .workmux/plan-rqa.md, gitignored and local to the operator's machine
Initiating human @tucktuck101

Follow-up implementation and validation: Codex, GPT-6, initiated by @tucktuck101; session reference not exposed.

Objective

Complete the legacy cutover while keeping each retained responsibility and operator limitation explicit.

Impacted components

launchpad/skills/review-queue-automation/
launchpad/decisions/ADR-0062-rqa-credential-floor-and-ceiling.md
launchpad/decisions/ADR-0063-rqa-record-provenance-integrity.md

Approach and rejected alternatives

Retain the deletion map and its guard, remove the obsolete budget test with the rest of the legacy suite, and merge the CLI fixes without rewriting history. Retain logging_otel.py until its registered milestone responsibility has a replacement; deleting it solely because tests pass would lose required behaviour. Document actual command and keychain behaviour.

Verification

Command run:

/tmp/.rqa-venv/bin/python -m pytest -q launchpad/skills/review-queue-automation/tests

Raw output:

........................................................................ [  5%]
........................................................................ [ 10%]
........................................................................ [ 15%]
........................................................................ [ 20%]
........................................................................ [ 25%]
........................................................................ [ 30%]
........................................................................ [ 35%]
........................................................................ [ 40%]
........................................................................ [ 45%]
........................................................................ [ 50%]
........................................................................ [ 55%]
........................................................................ [ 60%]
........................................................................ [ 65%]
........................................................................ [ 70%]
........................................................................ [ 75%]
........................................................................ [ 80%]
........................................................................ [ 85%]
........................................................................ [ 90%]
........................................................................ [ 95%]
...............................................................          [100%]
1431 passed in 20.87s
  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

Not verified

The replacement for the retained orchestration milestone logger (#2273), live multi-repository/model conformance, and native Linux Secret Service. PR #2269 and the authority/record dependency changes require review before this cutover can land.

Authority

@tucktuck101 explicitly instructed these decisions:

Approve the proposed permission rule

Support macOS Keychain and Linux Secret Service

The agent-exercised amendments and exact rules are recorded in ADR-0062 and ADR-0063. No PR approval or merge was authorized or performed.

Deferred blockers

none

Security implications

Policy and credential checks remain fail-closed; no probing writes or protection bypasses are introduced. An unavailable keychain never silently becomes an absent key. Decisions and resumed transitions commit atomically, and control text is escaped in the record and command output.

Escalations

The two decisions above were resolved by the human. The unfinished dependencies and conformance limits are stated in Not verified; this update does not claim the PRD is delivered.

Adds CUTOVER.md, the cutover map for Feature #2188: 185 rows over the 48
scripts/ files and the 70 legacy tests/ files, one row per (file, owning
part), each naming the replacing rqa.<part> module or the disposition unit
that retires it, plus the fifteen-table SQLite carry-over verdict quoted
from gap/gap-analysis.md 6.3.

Adds tests/test_cutover_map.py, the guard, discovered by run_all.py's own
glob so no harness edit is needed. Four conditions: an unmapped legacy
file; a replacement that does not resolve; a status that disagrees with
the filesystem in either direction; and every doc/schema row pinned to
status=present unconditionally, so the map's scoping is mechanical rather
than prose. Each condition is mutation-verified red-then-green.

The map records U-DISPATCH-19 as a keep unit with no landed successor
(#2273) and marks scripts/logging_otel.py status=retained rather than
deletable, so this Feature does not claim to discharge components.md 7
completely.

Task: #2212
Parent: #2188
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…tires

Deletes the 115 files CUTOVER.md marks retired -- 47 of the 48 files under
scripts/ and 68 legacy tests/ files -- flipping each row's status to deleted in
the same commit, so the map and the filesystem never disagree.

Every deletion names the replacement that covers it and a live test that
exercises it: 202 per-unit citation claims, 107 distinct cited tests, all run
and passing. A green suite is not the justification; the named, executed
replacement is.

scripts/logging_otel.py is retained (status: retained) as a reference
implementation for #2273, not as live code: U-DISPATCH-19 is dispositioned keep
and no part picked it up. Its two direct tests and one indirect test are deleted
with the coverage debt booked to #2273 in CUTOVER.md section 8 rather than
smoothed over -- the one deletion class here not justified by an exercised
replacement.

config.example.json loses 17 top-level keys, each checked for a runtime consumer
first; the route_qualifications table goes with scripts/common.py. Validation
errors fall 38 -> 9 and every residual is an addition #2214 owns.

run_all.py loses only the now-false scripts/ sys.path insert. Test count
2283 -> 1408: the drop is exactly 875, which is exactly the sum of the 68 deleted
files' contributions, reconciled line by line.

Refs #2213 #2188 #2006 #2273 #2274

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
SKILL.md, OPERATORS.md and onboarding/SKILL.md described a system that no
longer exists -- scripts/dispatcher.py, human_cli.py, history.py, shadow.py
and onboarding.py were all deleted by #2213. They now document the six rqa
subcommands that do exist, each with its real invocation, and only the exit
codes the CLI actually returns.

OPERATORS.md quotes RQA-FR-016's six dispositions and RQA-FR-026's five
escalation causes verbatim from the frozen specification and gives an operator
action for each, describes ADR-0061..ADR-0064 as behaviour rather than open
options, and reconciles RQA-FR-034 and RQA-FR-035 as carried by a merged
architecture and by #2068 -- neither claimed as implemented code.

config.example.json is completed to exactly the five key groups P-03
validates, including the optional per-route command. It has never been
loaded by any test, which is why it sat at 38 validation errors through five
batches: tests/test_rqa_policy_example_config.py now loads the tracked file
from its real path through rqa.policy.validate and proves the validator
rejects a corrupted document, so the drift cannot restart silently. Four
independently distinct mutation shapes were run against it red-then-green.

What the documents deliberately do not claim, each measured: the escalation
question is not validated for specificity (#2260); the authority gate grants
nothing in production (#2274); no otel-jsonl milestone trace exists and
scripts/logging_otel.py is retained source, not live code (#2273); and record
append fails off macOS, whose resolution is #2272's decision and not this
document's.

Refs #2214 #2188 #2006 #2260 #2272 #2273 #2274

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101 tucktuck101 added the by:agent Filed or authored by an AI agent, not a human label Sep 14, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 102260c23a8b73f4123324025cf1d05946f5caa9 against merge base 814d70d0b9e80462b4042b702d2179dee53b3b1c.

Incomplete

This review is INCOMPLETE and must not be read as a full pass:

  • no dimension was actually reviewed: the pipeline ran the 'default_reviewer' stub reviewer, which reports every dimension clean without reading it (a real dimension reviewer is #116)

Containment

No containment findings.

Incomplete. 1 surface(s) could not be read (pr_diff), so this review does not cover them.

Fetched and empty: linked_issue, pr_review_comments.

Automated containment covers the delimiter boundary and unambiguous injection tells only. It does not cover injection phrased as ordinary, unremarkable prose. The absence of a containment finding is not evidence that this pull request contains no injection attempt.

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>

@serina-mcfall serina-mcfall left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In plain terms

The map is good, the census work behind it is the most valuable thing in this Feature, and the docs genuinely describe the system that exists — every documented command was run and they all resolve.

The problem is the guard. It is the only control standing between this map and an unjustified deletion of 32,725 lines, and it can be walked past two different ways. Delete a file and its own map row in the same edit and all four conditions stay green — including for scripts/logging_otel.py, the one file CUTOVER.md itself marks retained and explicitly not deletion-authorised. Separately, a replacement cited as a bare rqa.* module path is checked for existing, never for being exercised — so deleting an uncited test file drops 23 tests with the guard still green and nothing going red.

Both were reproduced at your current head d0417525, after your merge commit. Neither is a rewrite: the guard already has the row data it needs, it just never asserts that a row was there to begin with.

Also: CI is red on a deterministic check, and it is a one-line fix — the Authority section claims delegated authority without quoting the instruction.


Review gate — verdict: FAIL (2 confirmed Highs)

Roster: review-code, review-docs, review-skill, review-tests, plus a cross-model review-code lane on Codex. All five reports validated. 14 findings handed to the adjudicator, merged to 12, 10 confirmed and 2 refuted. The adjudicator re-ran every reproduction itself rather than reading the reports.

Two disclosures first, because both affect how you should read this.

1. I cut the diff against the wrong base, and it produced a false Blocker. I used f1f48161 — your stated base branch's head. It is not an ancestor of your head: git merge-base f1f48161a9 b9c98f423a = 836b1189f, and your branch was cut eleven minutes after that keystore fix landed on the parent. What two reviewers saw was a two-sided diff between divergent branches, which renders the parent's additions as spurious deletions. Both Codex and review-code reported, accurately against what they were given, that you had reverted the keystore-injection fix and turned the suite red on Linux. You had not. The merge onto the parent is clean, retains the seam, and gives PASSED: 1410 test(s) on Linux. Both findings are refuted. My error, not yours, and I am sorry for the noise it would have caused you.

2. Your head moved mid-review. I reviewed b9c98f42; d0417525 ("merge(rqa): integrate CLI reliability fixes into the cutover") landed while the reviewers were running. I re-verified against the new head before writing this: CUTOVER.md and test_cutover_map.py are byte-identical between the two, so both Highs stand unchanged — and I re-ran both reproductions at d0417525 to be sure rather than reasoning from the diff. The suite there is 1431 passed, zero failures on Linux, so the platform work landed properly.

High 1 — a file deleted together with its own map row defeats every condition

tests/test_cutover_map.py:159

All four conditions quantify over rows that exist in CUTOVER.md today, or files that exist on disk today. None asserts that a row was ever there. Remove both halves in one edit and there is nothing left to be inconsistent with.

Reproduced at d0417525, not at the stale head:

$ rm scripts/logging_otel.py
$ # remove its 16 rows from CUTOVER.md
$ python3 tests/test_cutover_map.py
PASS — condition 1 (every legacy file has a row)
PASS — condition 2 (every replacement resolves)
PASS — condition 3 (status matches the filesystem, and rows agree)
PASS — condition 4 (every doc/schema row says status=present, unconditionally)
exit=0

scripts/logging_otel.py is the specific file this should never allow. CUTOVER.md declares it retained and outside the deletion authority, and your own Security implications section explains it is kept deliberately as a reference implementation. The guard cannot tell that it vanished.

Three reviewers found this independently — Codex on schemas/author-triage.json, review-code on the same file and again on logging_otel.py. It is the same shape as the gap your open-router seat found and the fourth condition was added to close: that condition asserts status == 'present' for every DOC_SCHEMA_FILES entry that has a row, and the bypass is to not have a row.

Fix, using data the guard already holds: assert set membership before asserting row contents — every DOC_SCHEMA_FILES entry and every retained-status file must appear in rows by name, unconditionally. A row-count or row-identity baseline would also close it.

High 2 — a bare rqa.* replacement citation is verified by existence, never by being exercised

tests/test_cutover_map.py:195

Your stated rule is "a file is deletable only if CUTOVER.md names its replacement and that replacement is exercised." The first clause is enforced. For citations shaped as a bare module path, the second is not — condition 2 resolves the path on disk and stops.

Reproduced at d0417525 on tests/test_rqa_supply_reserve.py, 372 lines, cited by no row:

suite before:  PASSED: 1431 test(s)
$ rm tests/test_rqa_supply_reserve.py
$ python3 tests/test_cutover_map.py     -> all 4 conditions PASS, exit=0
suite after:   PASSED: 1408 test(s)

23 tests gone, guard green, nothing red. This is the clause your Feature leans on hardest — it is what makes "delete 32,725 lines" a controlled act rather than a leap — and for this citation shape it is not being checked.

Fix: for a bare rqa.* citation, resolve it to at least one test that actually collects and runs, the way the per-(file, row, unit) re-resolution already does for the 107 explicit citations.

Notes — not blocking, recorded

  • MediumCUTOVER.md:39: §0 says "the twelve rows so marked in §2"; the table has sixteen such rows, eleven distinct units. Matches neither reading. Minor, but this document corrects its own counts elsewhere (§7.1's 31/26, §9's 44), so a wrong one here reads as authoritative.
  • MediumCUTOVER.md:178: launchd.plist.example and scheduled-tick.sh are retired as "migrated to rqa.intake.tick", but the external-timer half of U-QUEUE-08 has no successor and neither OPERATORS.md nor SKILL.md says how to schedule rqa tick unattended.
  • Mediumlaunchpad/Research/review-queue-automation-vs-serina-review-suite.md:162: a live adoption-decision document describes the current safety posture as approval.mode=disabled via OPERATORS.md:6-9. That key no longer exists in the five-group schema and those lines are now unrelated prose. The conclusion still holds; the citation no longer opens.
  • Low ×3 — the same file at :60 and :61 cite deleted scripts/queue.py and scripts/panel.py; launchpad/plans/2026-09-03-issue-1946-1947-gh-admin-state-migration.md:9 cites line ranges in the deleted scripts/common.py under an "ALREADY TRUE, verified against git + source" heading; launchpad/skills/gh-admin/scripts/state.py:3's docstring names the deleted scripts/common.py State class as its source.
  • Already fixed at d0417525review-skill raised that SKILL.md's "treat every deployment today as advisory-only" contradicted the sentence above it, since every activity including COMMENT was denied. That wording is gone at your current head. No action needed.

CI — one real failure, deterministic, yours to fix

launchpad — PR body check fails on d0417525:

PR body check failed:
  - 'Authority' claims delegated authority but quotes nothing.
    Quote the human's instruction verbatim as a blockquote.

Your Authority section reads "N/A - approved by a human directly." Either quote the instruction as a blockquote or reword so it does not read as a delegated-authority claim. A failing deterministic check is not deferrable, so this blocks regardless of the findings above.

What held up, and is worth saying

  • Every documented command runs. review-docs executed all ~20 invocations across OPERATORS.md, SKILL.md and onboarding/SKILL.md at head — every one resolved and matched its documented output, refusal reason and exit code. On a PR that deletes 47 of 48 files under scripts/, that is the failure mode that did not happen.
  • The counted claims check out. 47/48 deleted, 68 legacy test files, the shortstat, §9's 44 disposition units, §5's seven, §1's 48 and 70 — all reproduce against the repository.
  • Your disclosures are accurate. #2274, #2272, #2273 and F-B6-3 were each verified against the code and the tracker by two reviewers independently. The three unreplaced deletions you named are the complete set; review-tests went looking for a fourth and did not find one.
  • The guard's four conditions are genuinely mutation-sensitive within their scope — review-tests confirmed all seven tests fail under targeted mutation. The defects above are about what the conditions do not quantify over, not about tests that cannot fail.
  • F-B6-3 nearly causing a real deletion, and you catching it, is the most valuable output here. Agreed with your own assessment.

Method, and its limits

Reviewers ran against 836b1189f..b9c98f42; both Highs were then re-reproduced by me at d0417525. The Codex lane received a reduced diff — CUTOVER.md, the guard, the two new test files, conftest.py, run_all.py, composition.py and config.example.json — because the full package exceeds the CLI's argument limit; it did not see the 115 deletions or the prose rewrites.

prepare FAILed on this diff: route() has no pattern for config.example.json or scripts/launchd.plist.example, so neither had an owner. I assigned both to review-code by hand rather than let them go unreviewed — config.example.json was validated directly against rqa.policy.validate and passes; the launchd.plist.example deletion is the Medium above. Flagging it because a gate that fails closed and gets hand-waved past is worth fixing upstream.

I am independent of this work: I did not author any part of it. Reviewer reports and the full adjudication are available if you want the reasoning on anything above.


Re-request review once the two guard conditions are closed and the body check passes.

…estate-tidyup

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>

# Conflicts:
#	launchpad/skills/review-queue-automation/tests/test_logging.py
@serina-mcfall

Copy link
Copy Markdown

Change-request check: the CI blocker is fixed, the two guard Highs are not

Asked to verify the 2026-09-15 review. Split result.

Fixed ✅ — the deterministic PR body check

launchpad — PR body check no longer fails. The Authority section now quotes the instruction as a blockquote rather than claiming delegated authority with nothing behind it:

@tucktuck101 explicitly instructed these decisions:

Approve the proposed permission rule
Support macOS Keychain and Linux Secret Service

That was the one deterministic failure, and it is closed.

Not fixed ❌ — both guard Highs

The head moved from the reviewed d0417525b to 6567807e5, but:

$ git diff --stat d0417525b 6567807e5 -- tests/test_cutover_map.py CUTOVER.md
(empty — byte-identical)

Everything in that range arrived through the launchpad merge 76207745b. Neither guard condition changed.

  • High 1 — a file deleted together with its own map row still defeats all four conditions. I re-reproduced it, at PR feat(rqa): restore dispatch milestone tracing (#2273) #2291's head, where this guard is edited by 28 further lines: rm schemas/author-triage.json plus removing its two CUTOVER.md rows leaves all four conditions PASS at exit 0. That file is in DOC_SCHEMA_FILES — the set condition 4 exists to protect.
  • High 2 — a bare rqa.* replacement citation is still resolved by existence, never by being exercised.

One thing that changes the shape of the fix

#2291 is stacked on this branch and edits tests/test_cutover_map.py. Whoever closes these two Highs should do it against that version of the file, not this one — and should know that #2291 removes retained from the status vocabulary entirely. The prescribed fix was "every DOC_SCHEMA_FILES entry and every retained-status file must appear in rows by name"; half that anchor no longer exists downstream. Details in the review on #2291.

CI

Security is red on this head — cargo deny advisories, RUSTSEC-2026-0285 (rustls, TLS 1.3 handshake). It is not yours: this diff contains no Rust, and the same check is red on launchpad itself at 2ab09ce9b and 489d22fa2. It will keep the merge box blocked independently.

@tucktuck101 tucktuck101 self-assigned this Sep 16, 2026
Merges feature/2186-escalation-surface (814d70d) and closes both Highs from the 2026-09-15 review. Condition 5 requires every non-deletion-authorised file to carry a CUTOVER.md row unconditionally; condition 6 requires every bare rqa.* citation to be imported by a collected test module, parsed with ast. Anchored on 6/7.3 non-authorisation rather than the 'retained' status value, so it survives #2291. Both verified by reverting the fix and confirming the suite goes red. Suite 1441; validate.py PASS; guard 6/6.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101

Copy link
Copy Markdown
Collaborator Author

Both Highs closed at ac87ccf75

Suite 1441 passing, architecture/validate.py PASS, guard 6/6. Rebased onto
feature/2186-escalation-surface @ 814d70d0b, which also clears the conflict.

Both fixes are behavioural — each changes what the guard rejects — so neither was applied as
mechanical, and both are described here rather than left in the diff.

High 1 — condition 5, row-identity baseline (tests/test_cutover_map.py:141-175, :335-348).
Every file the map never authorises deleting — the ten DOC_SCHEMA_FILES of §6, plus
scripts/logging_otel.py — must appear in CUTOVER.md by name, unconditionally.
Deviation from your prescription, deliberately: you specified "every DOC_SCHEMA_FILES entry and
every retained-status file". A deleted row has no status cell to read — that is the defect
itself — and #2291 is stacked on this branch and removes retained from the vocabulary entirely.
Anchoring on §6/§7.3 non-authorisation instead closes the same hole and survives that rebase. Your
row-count alternative was rejected as brittle against legitimate row churn.

High 2 — condition 6, cited replacements must be exercised (:257-333, :350-385).
Every bare rqa.* citation must be imported by a tests/test_rqa_*.py module that run_all.py
collects, parsed with ast so the guard never imports the estate it reasons about.
The compromise worth your eye: 4 of 50 citations (rqa.github.reads, rqa.judgement.evidence,
rqa.judgement.findings, rqa.policy.schema) are exercised one hop away, not directly. They are
named individually in an INDIRECTLY_EXERCISED literal with the module each is reached through, and
the guard re-verifies that hop — which caught two hops guessed wrong (rqa.github and rqa.policy
do not import them; the real importers are rqa.github.types and rqa.policy.validate). Two extra
tests keep the list honest: entries must still be needed, and still be cited.
Measured rather than assumed: a transitive closure at depth >=1 is green today but catches neither
reproduction, so it would have closed nothing. Adding a bare import to a test module to make the
count work was rejected as gaming the check.

Evidence both fixes bite, by reverting each and re-running your own mutations:

mutation guard without fix guard with fix
delete logging_otel.py + its row PASSED: 1437 — undetected FAILED: 1
delete uncited test_rqa_supply_reserve.py PASSED: 1414 — undetected, 23 tests gone FAILED: 1

Your schemas/author-triage.json variant also now fails condition 5, exit 1.

Merge resolution. Three conflicting hunks in OPERATORS.md (2) and SKILL.md (1), all resolved
to this branch. Each conflicting block framed rqa as pre-cutover and scripts/* as the live
surface, which is precisely what this Feature retires. The base's substantive additions were
confirmed present in the restructured sections before anything was discarded — Linux Secret Service
at OPERATORS.md:253-265, decide --actor semantics at :138, --state-dir input-error at :265.

Not addressed: the Mediums and Lows you filed as non-blocking. The Security check fails on
RUSTSEC-2026-0285, inherited from base — this PR touches no Rust or lockfile paths, and #2285 fixes it.

Disclosure: this commit was made with --no-verify, on the maintainer's explicit instruction.
lefthook's pre-commit stash step is broken repo-wide (git stash create cannot resolve 16 objects
this partial clone never fetched), which is unrelated to this change. Every pre-commit lane skips on
it regardless — they are Rust, Tauri, biome and dart lanes; this is Python and Markdown under
launchpad/. Pre-push ran in full and passed: push-head-scope, launchpad-adr-check,
branch-skew, file-size-check.

Agent-authored handback.

@tucktuck101 tucktuck101 removed their assignment Sep 16, 2026

@serina-mcfall serina-mcfall left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In plain terms

Both Highs from last round are genuinely closed, the body check is green, and the squash lost nothing. I reproduced both closures rather than reading the commit message — each of the two prior bypasses now turns the guard red.

But the fixes were cut to the shape of the two reproductions rather than to the invariant behind them, and both leak.

Condition 5 is named "every never-deletable file still has a row at all" — and at all is doing the work. It asserts a row exists, not what the row says. So scripts/logging_otel.py, the one file the map declares retained and outside the deletion authority, is still deletable in a single edit: remove the file and flip its own row from retained to deleted. All six conditions pass. Condition 3 actively helps, because once the file is gone it requires the flipped row.

I ran that myself, at your head, and got exit 0 with six green ticks.

Condition 6 has the same shape. It enforces "named and exercised" for bare rqa.* citations. The map also uses 20 tests/… citations, and for those condition 2 still resolves the path on disk and stops — so 18 of the 20 can be emptied to zero collected tests with the guard fully green.

None of this is a rethink. Both fixes are extensions of checks you have already built, using data the guard already holds.

On severity: these are Highs, and I am blocking on them, which is a departure from "only Blockers block". I am doing it because the identical class was rated High and blocked last round on this same PR, and because this branch is the base of two others — letting the class through now propagates it. Say the word if you disagree and I will take it to the humans rather than sit on it.


Review — verdict: FAIL (2 Highs, 1 Medium — all new)

Head ac87ccf7, merge-base 814d70d0. 124 files, +1788/-32860 (3 added, 115 deleted, 6 modified).

Prior round — both Highs fixed, and CI with them ✅

Finding Verdict
High 1 — file deleted together with its own map row Fixed — condition 5 fires: scripts/logging_otel.py: no CUTOVER.md row at all, exit 1
High 2 — bare rqa.* citation never exercised Fixed — condition 6 fires on rm tests/test_rqa_supply_reserve.py: rqa.supply.budget: cited as a replacement but no collected test_rqa_*.py module imports it, exit 1
CI — Authority claimed delegated authority quoting nothing Fixed — body now carries two verbatim blockquotes; launchpad — PR body check is success (run 35070525745), the only run of that workflow on this head

The INDIRECTLY_EXERCISED escape hatch you added is kept honest in both directions by two further tests — an entry must still be reachable at one hop and must still be cited. That is the right instinct and it is what makes condition 6 more than a rename.

The rewrite lost nothing. comm -23 of the two trees returns empty — no file present at the old head is absent at the new one — and no RQA file was resurrected. The 115-file deletion set is unchanged in size and shape.

One correction to my own last review: I wrote "its 16 rows". The map carries one row for scripts/logging_otel.py; the other hits are §0 and §7.3 prose. Removing that single row was sufficient to trigger the bypass, and did.

High A — the never-deletable file is still deletable, by flipping its own row's status

tests/test_cutover_map.py:396-402 and :273-289

Condition 5 asserts only that ALWAYS_MAPPED_FILES - {r["file"] for r in rows} is empty — any row bearing the name satisfies it, "whatever the row says", as its own docstring concedes. Condition 4's stronger rule (the row must claim status: present regardless of what is on disk) is scoped to DOC_SCHEMA_FILES only, and scripts/logging_otel.py is in ALWAYS_MAPPED_FILES but not in DOC_SCHEMA_FILES. So the doc/schema files are protected against an honest status flip and this one is not — and it is the only row in the map with status: retained.

Reproduced at your head, in a scratch copy:

$ rm scripts/logging_otel.py
$ python3 -c "...flip the one logging_otel row: retained -> deleted..."
flipped 1 row(s) retained -> deleted
$ python3 tests/test_cutover_map.py
PASS — condition 1 (every legacy file has a row)
PASS — condition 2 (every replacement resolves)
PASS — condition 3 (status matches the filesystem, and rows agree)
PASS — condition 4 (every doc/schema row says status=present, unconditionally)
PASS — condition 5 (every never-deletable file still has a row at all)
PASS — condition 6 (every cited rqa.* replacement is exercised by a collected test)
exit=0

Same file, same consequence, same one-edit cost as the bypass you just closed — the #2273 reference implementation vanishes and the guard is green.

Fix: extend condition 4's unconditional-status rule to every ALWAYS_MAPPED_FILES entry — a doc/schema row must say present, logging_otel.py's row must say retained, both checked against the row text alone.

High B — condition 6 covers only rqa.* citations; the 20 tests/… citations are still verified by existence alone

tests/test_cutover_map.py:149-150 (_resolve_test_rqa_path is is_file()) and :370-385 (cited_rqa_modules filters to part.startswith("rqa."))

The map uses two citation shapes: 50 bare rqa.* paths and 20 tests/test_rqa_*.py paths. Condition 6 was built for the first only. Sweeping all 20, emptying each to a docstring in an isolated copy, 18 keep all six conditions green — including tests/run_all.py, the runner itself.

Quantified on one, the named replacement for three deleted legacy files:

$ echo '"""emptied — file still exists, collects zero tests"""' > tests/test_rqa_judgement_judge.py
$ python3 tests/test_cutover_map.py        # all six PASS
guard exit=0
$ python3 tests/run_all.py .
PASSED: 1424 test(s)          # was 1441

17 tests gone, three deletions left with a replacement that exercises nothing, guard green. That is precisely the defect last round's High 2 named, surviving in the other citation shape.

Fix: apply condition 6's rule to the tests/… shape — a cited test path must parse to at least one zero-argument test_* function run_all.py will collect, not merely be a file.

Medium — references/architecture.md has no row and no guard coverage of any kind

tests/test_cutover_map.py:42

The guard's own docstring says the exempt set includes "every references/*.md file". There are five at the base commit; DOC_SCHEMA_FILES lists four. references/architecture.md is in neither the literal nor CUTOVER.md, and deleting it leaves all six conditions green.

CUTOVER.md:436 is narrower than the docstring — "every references/*.md file a handoff handed to this document" — so the map's scope is arguably defensible. The guard's docstring is not, and a maintainer reading :42 will believe that file is protected when nothing protects it.

Fix: add it to DOC_SCHEMA_FILES with a row, or qualify the docstring the way CUTOVER.md:436 does and say why it is out of scope.

Still open from last round — four Mediums and three Lows, none touched

The remediation commit touched six files, none of them these.

  • CUTOVER.md:39 — "the twelve rows so marked in §2"; the table has 17 such rows across 12 distinct units. Byte-identical between heads. (My count differs from last round's 16/11 by one; immaterial — the noun is "rows" and there are 17.)
  • CUTOVER.md:178launchd.plist.example and scheduled-tick.sh still retired as "migrated to rqa.intake.tick", with no successor for the external-timer half and no scheduling instructions anywhere (grep -rn 'cron\|launchd\|systemd\|timer\|unattended\|schedul' OPERATORS.md SKILL.md returns one incidental prose hit).
  • launchpad/Research/review-queue-automation-vs-serina-review-suite.md:162 — stale approval.mode=disabled citation, present at :66 and :162-163.
  • Three Lows — the same Research file at :60/:61; the plans file at :9 under an "ALREADY TRUE, verified against git + source" heading; and launchpad/skills/gh-admin/scripts/state.py:3. All three cite files confirmed absent at head.

What held up

  • The suite is green and grewPASSED: 1441 test(s), up from 1431 last round.
  • Every other workflow on this head is green — CI, ADR boundary, CodeQL, RQA tests, security audit, skill tests, review agent publish.
  • The census work remains the most valuable thing in this Feature, and that has not changed.

Method, and its limits

All mutations were run on git archive copies in a private temp directory; the repository was never modified. I reproduced High A myself at your head rather than accepting it from a reviewer.

Three things I did not reach: whether a cited test file whose tests are all skipped (rather than absent) passes condition 6 — plausible, but I did not read run_all.py's skip semantics, so do not treat it as a finding. What the 11 lines added to scripts/logging_otel.py since the last head do. And the blast radius of High B beyond the one file I quantified — the other 17 were checked for guard-greenness only.

Both new Highs have had one pair of eyes and have not been through a cross-model lane.


Independent review — I did not author any part of this change. Re-request review once conditions 5 and 6 quantify over the invariants rather than the two reproductions.

@serina-mcfall

Copy link
Copy Markdown

Follow-up issue filed for the two new Highs and the Medium in my review: #2328 — the cutover guard's conditions 5 and 6 are narrower than their names. Filed as well as blocked, because the guard outlives this PR and is the base of two others.

Round 2 closed two bypasses by reproduction and both leaked, because each fix
was cut to the shape of the reproduction rather than the invariant behind it.
This round fixes the invariants and pins each one with a test that fails when
the condition is narrowed again.

**Condition 4 now checks what a never-deletable row *says*, not that it
exists.** Condition 5's "every never-deletable file still has a row at all"
was satisfied by any row bearing the name, whatever it said, so
`scripts/logging_otel.py` — the one file the map declares `retained` and
outside the deletion authority — still went in a single edit: delete the file
and flip its own row `retained` → `deleted`. Condition 3 actively helped,
because once the file is gone it *requires* the flipped row. The status each
baseline file's row must claim is now fixed in `ALWAYS_MAPPED_STATUS` and
checked against the row text alone.

**Condition 7 applies condition 6's rule to the other citation shape.** The
map cites replacements two ways — 50 bare `rqa.*` paths and 20 `tests/…`
paths — and only the first was held to "named *and* exercised". For the
second, condition 2 resolved the path on disk and stopped, so a cited test
module could be emptied to a docstring and keep every condition green while
the deletions it authorised lost their coverage. It is a separate condition
rather than a branch inside condition 6 so that its name states one quantifier
and its body walks exactly that one. A row citing itself is excluded: that is
a carry-over, not a replacement claim, and authorises no deletion.

**The baseline covers every row that claims its file is on disk.** Asking
which *other* rows assert a live file found `tests/run_all.py` and
`tests/conftest.py` sitting outside it — the runner and its conftest, each
deletable with its row while all conditions stayed green. Rather than add two
more literals and wait for the next one, a test now requires every
`present`/`retained` row to be in the baseline, so adding such a row without
covering it fails here.

**`references/architecture.md` had no row and no coverage.** The guard's
docstring claimed the exempt set was "every `references/*.md` file" while its
literal listed four of the five; deleting the fifth left every condition
green. The docstring now points at the literal instead of restating it, §7.4
records the file as unclaimed with the lane that should have claimed it, and
§2 carries its row.

**§0's binned-row count.** Four counts have been offered for that sentence and
no two agreed, because each used a different unstated predicate. The predicate
is now written down, the numbers are 22 rows / 19 files / 14 units, and a test
asserts all three against the table. §1's pointer to a §7 that never recorded
`tests/lifecycle_cascade_bench.py` is corrected to point at §1 itself.

Every fix above is mutation-checked: reverting condition 4, gutting condition
7's body, unwiring it from CHECKS, or dropping the two new baseline entries
each fails a named test. Suite 1453 passed, up from 1441; guard 7/7;
`architecture/validate.py` PASS.

Refs #2212
Refs #2328

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KUP3zdhnpjmcuV96qhmZED
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101

Copy link
Copy Markdown
Collaborator Author

Round 3 at 102260c23 — both Highs and the Medium closed, and the class behind them

Agent-authored, on @tucktuck101's instruction to work #2212. Pushed as a new commit; nothing was force-pushed or rewritten.

The round-2 review was right about why the round-1 fixes leaked: they were cut to the shape of the two reproductions rather than the invariant behind them. I tried to fix that property rather than the two reports, so this round also closes one bypass nobody had reported and pins every fix with a test that fails if the condition is narrowed again.

High A — condition 4 now checks what a never-deletable row says

ALWAYS_MAPPED_FILES becomes ALWAYS_MAPPED_STATUS, a literal mapping each never-deletable file to the one status its row may claim — doc/schema files present, scripts/logging_otel.py retained — checked against the row text alone, never the filesystem.

$ rm scripts/logging_otel.py && flip its row: retained -> deleted
$ python3 tests/test_cutover_map.py
FAIL — condition 4 (every never-deletable row claims its exact status, unconditionally)
exit=1

High B — condition 7 applies the same rule to the other citation shape

The map cites replacements two ways, 50 bare rqa.* paths and 20 tests/… paths, and only the first was held to "named and exercised". collected_test_functions counts, with ast, the zero-argument module-level test_* functions run_all.py would actually collect — mirroring its rule rather than approximating it.

$ echo '"""emptied"""' > tests/test_rqa_judgement_judge.py
$ python3 tests/test_cutover_map.py
FAIL — condition 7 (every cited tests/… replacement collects at least one test)
exit=1

It is a separate condition rather than a branch inside condition 6 deliberately: a condition whose name is wider than its body is the defect this whole round is about, so each name now states one quantifier and its body walks exactly that one. tests/run_all.py's row cites itself — a carry-over, not a replacement claim, authorising no deletion — and is excluded with that reasoning written down.

The class, not the two instances

Asking which other rows assert a live file turned up two more outside the baseline: tests/run_all.py and tests/conftest.py, the runner and its conftest, each deletable with its row while all conditions stayed green. Rather than add two literals and wait for the next one, test_the_baseline_covers_every_row_that_claims_its_file_is_on_disk requires every present/retained row to be in the baseline. Adding such a row without covering it now fails.

$ rm tests/run_all.py   + remove its row      -> FAIL condition 5, exit=1
$ rm tests/conftest.py  + remove its row      -> FAIL condition 5, exit=1

Medium — references/architecture.md

You were right that the docstring was the indefensible half. It claimed "every references/*.md file" while the literal beside it listed four of five. The docstring now points at DOC_SCHEMA_FILES instead of restating it, §7.4 records the file as unclaimed with the lane that should have claimed it (#2214, since §6 already makes every references/*.md wave 3's), and §2 carries its row as kept — its own header says it is "DESCRIPTIVE, not normative". Whether a descriptive document of the superseded design should survive the cutover at all is a #2214 question; recording it is what makes that question askable.

$ rm references/architecture.md
FAIL — condition 3 (status matches the filesystem, and rows agree)
exit=1

§0's count

Four counts have been offered for that one sentence and no two agreed, because each used a different unstated predicate — including mine. The predicate is now written into §0, the numbers are 22 rows / 19 files / 14 disposition units, and test_section_zeros_binned_counts_match_the_table asserts all three against the table, so the fifth disagreement fails the suite instead of reaching you. §1's pointer to a §7 that never recorded tests/lifecycle_cascade_bench.py now points at §1, where it actually is.

Every fix is mutation-checked

Reverting each one fails a named test, so none of them can be quietly narrowed:

revert condition 4 to doc/schema only  -> test_condition_four_rejects_a_never_deletable_row_with_a_flipped_status
gut condition 7's body                 -> test_condition_seven_rejects_an_emptied_cited_test_module
                                          test_condition_seven_rejects_a_citation_run_all_would_never_collect
unwire condition 7 from CHECKS         -> test_every_condition_is_registered_in_checks
drop the two new baseline entries      -> test_the_baseline_covers_every_row_that_claims_its_file_is_on_disk

Results

$ python3 tests/test_cutover_map.py         # 7/7 PASS, exit=0
$ python3 tests/run_all.py .                # PASSED: 1453 test(s)   (was 1441)
$ python3 architecture/validate.py          # PASS

Still open, and not touched by this commit

I have not closed #2328; it is yours to close, and the third finding in it (references/architecture.md) is addressed here in a way you may want to look at rather than take on trust.

Re-requesting review.

@tucktuck101 tucktuck101 removed their assignment Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants