feat(rqa): escalation and the operator command surface — rqa/escalation, rqa/cli (#2186) - #2269
Conversation
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…lt through front doors Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Batches 1-4 landed on trunk while this branch was in flight (PRs #2228, #2235, #2237, #2238, #2239, #2240, #2241, #2244, #2245). Merging trunk in rather than rebasing, per launchpad/AGENTS.md: the branch is about to be reviewed and a rebase would break the correspondence between what was gated and what lands. This also brings @serina-mcfall's gitleaks allowlist (cbfe9d3), which the escalation fixture added by this branch needs a further entry in. Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…fixture test_rqa_escalation_security.py embeds a deliberately real-looking token so it can prove rqa/escalation never lets one escape -- the same purpose as the three review-queue-automation entries already above it, and the same purpose the security_audit_fixtures and the-professor entries document. The finding it guards is not hypothetical: a first fix in this batch del'd the binding before the direct raise statements but left it live across record.append, store.close and lifecycle.resume, and the gate caught three reachable copies. A fixture gitleaks cannot read as a secret would not exercise that control. Synthetic: a ghp_ prefix on a literal T1CanaryTokenValue followed by zeroes. No real credential is present and nothing needs rotating. Named as a fourth literal path rather than widening to a tests/.* glob, for the reason #389 records for Cargo.lock: a directory-wide exemption would silently cover every present and future file in that tree against every rule, and only this one was verified. Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
There was a problem hiding this comment.
Reviewed commit c79bf20f6bc055d4dde442871d5c73a156680f13 against merge base 69f7a55d9fb88808d2dfe139ce43b10804494c63.
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.
Fetched and empty: 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.
…a macOS keychain
Two sites hardcoded a real `OSKeyStore`, whose `read()` raises
`KeyStoreExplanationUnavailable` off Darwin, which `SQLiteRecordWriter.append`
converts to `AppendFailed` (`P-12-record.md` §3.1 step 5). That made five
`tests/test_rqa_cli_main.py` tests pass on macOS and fail on ubuntu-latest:
- `rqa/cli/composition.py:303` built its `RecordWriter` with
`keystore=OSKeyStore()`, so the `decision` entry `decide` appends through
`comp.record` failed; and
- `tests/test_rqa_cli_main.py:104`'s `_seed_escalated_job` did the same, so the
seed itself failed before any command ran.
`build_composition` now takes `keystore: KeyStore = OSKeyStore()` as a
keyword-only dependency, the way it already takes `clock`, and the way
`SQLiteRecordWriter.__init__`, `verify()` and `SQLiteRecordReader.__init__`
already publish the same seam. The default is evaluated once at definition time,
so no existing caller's behaviour changes; the `Composition` dataclass, `main.py`
and the CLI surface are untouched.
The platform guard in `rqa/record/keychain.py` is deliberately untouched: an
absent key and a machine that cannot answer are distinct there by design, and
inventing a key for either would silently downgrade every Linux record from keyed
to unkeyed. Whether RQA should support a non-macOS platform at all is #2272.
The tests now exercise the keyed path with an injected fake key store that
returns real key bytes (never `None`, which would move them onto the unkeyed
branch) and records the item names it was asked for. `_run_raw` -- the single
funnel every in-process `main()` call in the file goes through -- installs the
injection, so `_cmd_pending`, `_cmd_decide` and `_cmd_explain` are all covered,
including the three tests that call `_run` without `_offline_composition`.
`_assert_every_entry_is_keyed` asserts `keyed = 1`, a 64-hex `hmac`, and that
only `rqa-record-hmac` was ever requested; forcing `append` to stop keying fails
all five tests. No test is skipped on any platform.
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
|
CI was right and my local verification was not. Fixed in What failed. Five Why ten batches of local verification missed it. Every acceptance run in this PRD has been on macOS, where The fix. Verified by forcing the condition rather than assuming it, since the work happens on macOS. Pinning Independently reproduced by the review gate. Full suite unchanged at 2276 — the fix strengthened existing assertions and added no test. Two repairs were considered and rejected. Making a missing platform command return The product question this exposed is filed as #2272 and is deliberately not answered here: RQA cannot append a record on any platform but macOS, and an operator on Linux learns that from |
serina-mcfall
left a comment
There was a problem hiding this comment.
In plain terms
This is the last implementation Feature of PRD #2006 and the first change that lets a human actually run RQA. The architecture holds up, the test suite is genuinely strong — 2276 passing, and I mutation-tested the credential-secrecy claims: every mutation was caught by the test that claims to cover it. That is real coverage, not theatre, and it should not get lost in what follows.
But a human's authority decision can be silently destroyed while the CLI prints "decided" and exits 0. Reproduced at this head. That is a Blocker.
Five Highs sit under it, four of which are the same shape: a command that reports success while having failed. The operator surface is where this PR is weakest, and it is the part with no prior art to lean on.
One finding was refuted — --actor does not let anyone self-attest authority. I ran the attack and _step12b refused it. That one is fine.
Review gate — verdict: FAIL (1 Blocker, 5 Highs)
Head f1f48161a, base 78e78936. Four lanes: review-code, review-tests, a qa explore session against the CLI, and a cross-model review-code lane on Codex. 20 findings → 18 rows: 17 confirmed, 1 refuted, 2 merged. Severities moved on five rows.
Two notes on how this ran, since both affect how much weight to put on it. The roster router does not route .gitleaks.toml to any reviewer, so I assigned it by hand. And the Codex lane initially could not run at all — a 305KB diff package makes a 316KB prompt against a 131KB argument limit — so I rebuilt a source-only package to get the cross-model read. It found the Blocker.
Blocker — a decision can be destroyed while the CLI reports success
rqa/escalation/decide.py:158
Codex flagged this but disclosed its reproduction ran against a different worktree, and review-code had concluded the opposite by tracing main.py:214-220 and finding the rollback correct. So I had it reproduced against this head. It reproduces exactly:
decide() returned: Decision(actor='human-reviewer', ... outcome='changes_requested')
append kinds seen: ['decision', 'transition', 'transition']
--- after the CLI commits (main.py:222) ---
decision rows: 0
escalation status: open
job status: stopped
_cmd_decide then returns exitcodes.OK with {"outcome": "decided"}.
The mechanism is one line, and it is not in this diff — rqa/lifecycle/transition.py:146, with connection:. That is a connection-wide commit/rollback, not a savepoint. decide() has already appended the decision entry and closed the escalation row uncommitted, because the CLI owns that transaction. When an append inside _commit raises, the context manager rolls back the entire connection transaction, erasing both. resume's containment then re-reads the rolled-back row as escalated, safe_stop runs a second transition whose own with connection: commits stopped, and resume returns normally.
No exception ever reaches main.py:214. Its rollback never fires; its commit is a no-op. review-code's analysis of that handler was correct and simply not where the defect lives.
This contradicts decide.py:15-17's own contract — "AppendFailed always propagates … a decision that was not durably recorded never resumes anything" — and main.py:214-220's comment, "nothing it touched may become durable." The 2276-test suite is green and blind to it.
High 1 — every record append fails off Darwin
rqa/cli/composition.py:291
build_composition defaults keystore to OSKeyStore(); main.py passes none at any of its five call sites. On Linux, raise_ and decide both fail with AppendFailed, and pending then returns [] at exit 0 — the CLI presents as a healthy empty queue on a host where nothing can ever be written.
review-tests argued this is not a defect: macOS-only target, documented seam, OSKeyStore tested elsewhere. I had all three premises checked, and they don't hold:
OPERATORS.mdnever mentions this CLI.grepforrqa decide|rqa tick|rqa pending|python -m rqa|rqa.cliacrossOPERATORS.mdandSKILL.mdreturns zero hits. What it documents is the olderscripts/system, and no file underscripts/imports therqapackage. A document that never names this code cannot establish its deployment target.- The quoted docstring is in
composition.py:317-320, notkeychain.py— code this PR adds — and it reads "a caller that cannot reach a platform keychain (CI on Linux, a test process) supplies its own." The seam's own rationale scopes it to CI and tests, not tomain.py, which is the operator's caller. OSKeyStorebeing unit-tested in isolation says nothing about whether the composition root wires it correctly.
Your head commit is "inject the record keystore so the CLI tests do not require a macOS keychain" — the problem was hit and solved for the test process, leaving main.py untouched. High rather than Blocker because it fails closed and loud, and works on Darwin.
Merged with review-tests's finding that _run_raw patches build_composition's keystore unconditionally, so no CLI test exercises the true default wiring on any platform — including the macOS one. Fixing the wiring means writing that test.
While you're there: keychain.py's non-darwin branch raises KeyStoreExplanationUnavailable while its message says "the record can still be appended unkeyed" — it advertises the return None path and takes the raising path. That is the sentence an operator sees at exit 4, with no flag to act on it. Not filed separately; fix it in the same change.
High 2 — a mistyped --state-dir is created and answers "empty queue"
rqa/cli/composition.py:299
$ python3 -m rqa.cli --state-dir .../typodir pending
{"command": "pending", "outcome": "ok", "result": []} exit=0
$ ls -la .../typodir
-rw-r--r-- ... 155648 ... state.db
mkdir(parents=True, exist_ok=True) with nothing asking whether the directory was meant to exist. A typo, or an RQA_STATE_DIR that expanded empty, produces a fresh store and the answer "nothing pending" — indistinguishable from the truth, and unfalsifiable from the output. status and explain degrade the same way.
High 3 — exit codes 2 and 3 are declared and unreachable
rqa/cli/main.py:282
exitcodes.py declares NETWORK = 2 and AUTH = 3 and quotes the DoD verbatim in its docstring: "Exit codes are exactly 0 ok, 1 input error, 2 network, 3 auth, 4 other." Neither constant is referenced anywhere outside its declaration. Everything unclassified lands in the bare except Exception and returns 4.
Found twice by independent methods — review-code by reading the except chain, qa by census across thirteen live invocations, all of which returned 0, 1 or 4. A wrapper that retries on 2 and pages on 4 cannot be written; an expired token and a genuine defect are the same code.
Promoted from qa's Medium to High: a DoD quoted verbatim in the file, that does not hold, in a PR claiming to close the issues carrying it. Narrowing the docstring is a legitimate fix if 2/3 are deliberately deferred — that's your call, but the two cannot both stand.
High 4 — onboard fabricates the repository directory
rqa/cli/main.py:112
onboard 'block/buzz' -> "written" block/buzz/.rqa/config.json exit=0
onboard '' -> "written" .rqa/config.json exit=0
onboard '../../../../tmp/...' -> "written" ../../../../tmp/.../.rqa/... exit=0
onboard 'a; rm -rf /tmp/nope' -> "written" 'a; rm -rf /tmp/nope/.rqa/...' exit=0
repo is a bare positional with no help text, resolved as a filesystem path — while tick, status and explain all take repo as a GitHub slug. This PR is what puts them side by side. The slug spelling is the natural thing to type and produces a phantom directory at exit 0. An empty string is accepted, where decide correctly rejects an empty --actor.
No shell injection — a literal directory named a; rm -rf was created, nothing reached a shell. Fix is CLI-local: help text, plus rejecting empty and slug-shaped input.
High 5 — tick reports a successful sweep during a total GitHub outage
rqa/cli/main.py:173
tick.py:101 appends to repos_admitted before calling github.inventory; a GithubUnavailable is logged to stderr and continued. TickResult has no field for unavailability at all. So the JSON a scheduler parses says admitted-all, refused-none, failed-none, exit 0, during a complete outage — only a human reading stderr can tell.
tick.py and types.py are out of diff, but main.py:173 is new code making a new machine-readable contract, and exit code 2 exists in this PR for exactly this case and is unwired. An honest fix probably needs a TickResult field, which is provider-side work — flag it if that's the wrong scope for this PR.
Refuted — --actor does not bypass authority
qa flagged --actor as unauthenticated free text and marked it unadjudicated (no review-threats in this fleet). I had it settled by execution rather than left open. _step12b admits an authority-requirement outcome only when exactly one facts.reviews row matches actor, outcome, head_sha, and was submitted at or after the escalation was raised:
decide() RAISED: StaleDecisionError 0 submitted reviews match the recorded decision
for job 'job-1'; exactly one same-head, post-escalation review is required
--- after rollback --- decision rows: 0 escalation status: open job status: escalated
Refused, rolled back, nothing durable. The free-text field is a lookup key against a real GitHub review, not a trusted assertion — the P-11 design is sound here.
Filed, not blocking
Eleven confirmed Medium/Low findings, grouped into four tickets rather than added to this PR's scope:
- #2278 — CLI input validation:
--batch-sizeaccepts negatives (silently drops jobs) and0;decidemisses the symmetric outcome check so a forgotten--outcomeexits 4 instead of 1; a successfuldecidenames no subject;explain'sjobmode switch has no help text - #2279 — control characters written raw into the HMAC'd record, and
sanitize_textdeleting rather than escaping (words weld together); plus the bidi claim in its docstring being wider than the C0/C1 class it matches - #2280 —
explain_jobhardcoding its ownOSKeyStoreso a tampered and an intact record render identically, andExplanationUnavailablenever echoing the job id. Both out of diff and byte-identical to base — newly reachable via your new CLI, not broken by it - #2281 — three test gaps: no test drives two real escalation entries through a second decide/resume; an ordering assertion that exercises the fixture's
sorted(); the AST census read as behavioural coverage
What held up well
- The credential-secrecy suite is real. Four mutations to the
delstatements indecide.py/escalate.py, plus a forbidden-import injection — every one caught by the name-matched test._reachable_from, walking__context__/__cause__,vars()and frame locals, is a materially stronger check than astr(exc)substring test, and it earns that. .gitleaks.tomlis anchored to one literal path rather than widened to a glob, the canary is legibly synthetic, and it needs theghp_shape to exercise the path under test. Correct call.- Concurrency is genuinely safe — eight simultaneous
ticks, exactly one swept, seven refused,PRAGMA integrity_checkclean, andflockmeans SIGKILL leaves no stale lock. - Corrupt stores and malformed
repos.jsonfail loudly and correctly, and double-decide is properly refused and rolled back. - Boundary discipline in
test_rqa_escalation_decide.pyis genuine — exact-match versus one-character-different, both directions of the unpinned case as separate named tests.
CI
All checks green on f1f48161a. The Backend Integration lane that is red elsewhere in this repo (#2266, the Docker Hub minio pull) did not fire here.
Re-request review once the Blocker and the five Highs are addressed. The Blocker is the one to look at first — it needs a durable transaction boundary around the decision append and closure before lifecycle resumes, and the fix likely lives in transition.py rather than in decide.py. Reports and adjudication are under .review/pr-2269/.
…escalation-surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…ation-surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…ation-surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
serina-mcfall
left a comment
There was a problem hiding this comment.
In plain terms
Everything from the last round is genuinely fixed — the Blocker and all five Highs, each re-verified by reproduction rather than by reading your diff. The savepoint in transition.py is a real guard, not a decoration: we reverted it and watched your own regression test go red, then restored it and watched it pass.
The problem is that the fix closed the path that was demonstrated, and the class stayed open one layer down. Your decide() leaves the decision and the escalation-close uncommitted on purpose. Then resume()'s first action is a live GitHub read — and SqliteApiCallStore.record() ends in an unconditional connection.commit() on that same connection. So the first successful network call commits work that was meant to be provisional. When validation then fails, the CLI's rollback has nothing left to undo.
The end state is worse than the bug it replaced. The old one you could recover from by retrying decide. This one closes the escalation durably, leaves the job stuck at escalated, and refuses the retry with ALREADY_CLOSED — so the corrected decision can never be applied without editing the database by hand. And it fires on the normal path, not an edge case.
Separately: tick --repo takes an owner/repo slug, onboard takes a checkout path, and both feed the same config_path(). So a repo you onboard and then sweep is refused at admission — the two commands this feature exists to ship cannot currently complete one cycle together.
Review gate — verdict: FAIL (2 Blockers, 3 Highs)
Roster: review-code, review-docs, review-skill, review-tests, plus a cross-model review-code lane on Codex. All five reports validated. Eleven findings, merged to seven, all eleven confirmed, none refuted. The adjudicator reproduced both Blockers and the first High itself from the real classes rather than trusting any reviewer's transcript.
Prior round — the Blocker and all five Highs are fixed ✅
Verified independently, not taken from your PR body:
- Prior Blocker.
transition.py:47-67now branches onconnection.in_transaction— standalone callers keep the original boundary, nested callers getSAVEPOINT rqa_transitionwithROLLBACK TO/RELEASEand never have their transaction committed.resume.py:112,150-155re-raises instead of running itssafe_stopfallback when the caller owns the transaction. Rebinding_transactionto the pre-fix shape makes both of your atomicity tests fail; restoring makes them pass. The guard is real. - High 1 — real
secret-toolLinux backend with a search fallback that distinguishes absent from unreadable, plus a test drivingmain()with the unpatched defaultOSKeyStore(). The misleading "can still be appended unkeyed" message is gone. - High 2 — all four commands exit 1 with a usage error and create nothing. Confirmed by running them against a directory that never existed.
- High 3 — exit codes 2 and 3 wired at
main.py:196-199and reachable. Fixed by wiring rather than by narrowing the prose, which was the better of the two options open to you. - High 4 — slug, empty string and traversal path all rejected, no phantom directories.
- High 5 —
TickResult.repos_failedexists; outcome is"incomplete"with a non-zero exit.
That is a thorough round of work and it holds up.
Blocker 1 — the atomicity fix is defeated by a foreign commit on the same connection
rqa/github/store.py:216
composition.py:307 builds one connection and wires every collaborator over it. SqliteApiCallStore.record() ends in an unconditional self._connection.commit(), and Transport._exchange() calls it after every completed exchange (transport.py:234). resume() calls deps.github.facts(...) first (resume.py:123), which reaches _exchange on line one.
Reproduced against the real Transport and real SqliteApiCallStore, stubbing only the network send:
BEFORE decide(): in_transaction = True
decide() raised: StaleDecisionError
AFTER decide() raised: in_transaction = False
AFTER connection.rollback():
decision rows: 1
escalation status: closed
job status: escalated
retry -> EscalationRefused(reason=ALREADY_CLOSED)
in_transaction is already False when decide() raises, so _cmd_decide's comp.connection.rollback() is a no-op. There is no recovery short of hand-editing the database.
It also re-opens the split-transaction shape on the success path: once record() commits, in_transaction is False, so every later transition() in the same resume() takes the standalone branch and commits as its own separate transaction.
The fix is a commit discipline across the three github stores, not a patch to one method — see High 1 below.
Blocker 2 — tick and onboard disagree about what "repo" means
rqa/cli/main.py:168
main.py:125 types tick --repo through _repo_slug(), which requires owner/repo. main.py:135 gives onboard a positional documented as "existing local repository directory", now enforced by is_dir(). Both reach config_path(repo) → Path(repo)/.rqa/config.json.
onboard(repo='checkouts/buzz') -> Written('checkouts/buzz/.rqa/config.json')
tick admission reads: block/buzz/.rqa/config.json
_check_admission("block/buzz") -> AdmissionRefusal(reason="unreadable ... [Errno 2]",
onboarding_command='rqa onboard block/buzz')
The refusal's own suggested remedy is a command that your High 4 fix is designed to reject.
One precision the adjudicator insisted on: these do reconcile, under exactly one layout — checkouts arranged as <cwd>/<owner>/<repo> with rqa run from the parent. That convention appears nowhere in the code, the help text or the docs, and onboard's own help points away from it. Which is the fix, not a defence.
High 1 — the same defect at two more call sites, and one was misidentified
rqa/github/store.py:162
review-code reported this as SqliteMutationStore.put on the write path. It is SqliteEtagStore.put, on the read path — called from _get_one after every fresh non-304 response carrying an ETag. So it fires on the same facts() read as Blocker 1, by a shorter route than claimed. Reproduced with the api-call store neutered so only the ETag store could commit:
caller in_transaction before the GET: True
caller in_transaction after the GET: False
probe rows surviving the caller's rollback: 1
The correction makes the finding wider, not narrower. Lines 162, 216 and 261 all commit a connection they do not own — patching store.py:216 alone will not close Blocker 1.
High 2 — no test can see any of this
tests/test_rqa_lifecycle_resume_from_record.py:162
Every decide()/resume() test substitutes a fake github: FakeGithub at test_rqa_lifecycle_resume_from_record.py:162 and test_rqa_escalation_ac13_resume.py:148, the same fixture reused by test_rqa_decision_atomicity.py:26, and _OfflineGithub in test_rqa_cli_main.py. None reaches Transport or any Sqlite*Store commit. The reproduction above had to be wired by hand precisely because the suite contains no such wiring.
Your suite is green at 2297 on Linux and structurally blind to this whole class. Held at High rather than Blocker only because the code defect it fails to cover is already Blocker 1 — but it is the row that matters after you fix store.py, because without it nothing proves the fix worked.
High 3 — the replacement CLI has no invocation anywhere in the operator docs
OPERATORS.md:34
grep -nE "rqa decide|rqa tick|rqa pending|rqa onboard|rqa status|rqa explain|python3? -m rqa|rqa\.cli" SKILL.md OPERATORS.md
-> zero hits in both
The two new sections describe per-subcommand keychain behaviour for six subcommands and never show how to run one. OPERATORS.md:560-572's entry-point index still lists only scripts/*.py.
The two reviewers split on severity — review-skill called it a Blocker, review-docs two Highs. Settled at High, because rqa is explicitly pre-cutover (__main__.py:1-2 names CUTOVER.md/#2212 as the unfinished step), the documented scripts/* workflow still functions and imports nothing from rqa, and Blocker 2 means documenting the invocation today would document a cycle that cannot complete. The argument that kept it at High rather than Medium is review-skill's: a SKILL.md is an agent's operating contract, and calling rqa "the replacement" while showing only scripts/human_cli.py decide invites an agent to conflate two disjoint decision surfaces.
Notes — not blocking
- Medium —
architecture/code/P-11-escalation.md:343: new prose enumerates tick's incomplete outcome as exit 2 or 3, omitting exit 4, whichmain.py:198returns for theinternal_errorreasontick.py:123really produces. Short one branch of theif/elif/elseit describes. - Medium —
tests/test_rqa_escalation_security.py:230:decide.py:103'sdel escalationguard has no credential-reachability test; the test at that line exercises the sibling raise atdecide.py:105-112. The guard is real and untested.
What held up
- The credential-secrecy suite is still strong. Four
del-statement mutations indecide.pyall caught, and the forbidden-import scan caught an injectedimport subprocess. It survived the refactor, which is the regression nobody usually checks for. - The savepoint fix is non-tautological — proven by reverting it, not by reading it.
- The new authority wiring narrows rather than loosens.
AuthorityClientnow builds aGate(repos=...)and grants are checked against the configured set; previously-ungatedgrant()calls are now membership-checked. Direction is a restriction. .gitleaks.tomladds one anchored literal path, consistent with the file's stated convention. The canary is legibly synthetic — nothing to rotate.admit/tickare unaffected by the savepoint change —tick.pycommits after everyadmit(), so_transaction()always takes the standalone branch there.
Method, and its limits
Reviewers ran against 5089c83f..997f7608, which is the merge-base with launchpad, not the base branch's current head — cutting the diff the other way produces a two-sided diff that invents findings, and this round was cut correctly.
The Codex lane received the complete production-source slice (all of rqa/ plus .gitleaks.toml, 124 KB); the full 431 KB package exceeds the CLI's argument limit, so that lane did not see the tests. prepare FAILed to route .gitleaks.toml — the gate's route() has no pattern for it — so it was hand-assigned to review-code rather than left unreviewed.
Prior Highs 3 and 5 were confirmed by the adjudicator from the wiring plus two concurring independent reproductions, rather than a third live run — a weaker basis than the other four, and recorded as such.
I am independent of this work: I did not author any part of it. Five reviewer reports and the full adjudication are available if you want the reasoning on anything above.
Re-request review once the commit discipline is fixed across store.py:162, :216 and :261, the slug/path mismatch is resolved, and a test exists that drives a real GithubAdapter through decide().
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…escalation-surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…n-nullability Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…oundaries Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Change-request check: the head moved, but only by merges — nothing was fixedAsked to verify whether the 2026-09-15 review's two Blockers and three Highs were addressed. They are not. Flagging this explicitly because the head has moved since the review, so tooling reports "head moved after the review" and it can read as progress. Everything that landed since the reviewed head The 6,983 changed lines in that range are the documentation-review corpus and an unrelated logging test arriving via Re-checked directly at head
The review's closing ask is unchanged: commit discipline across |
… agree on "repo" Two Blockers from the second review round on #2269. The savepoint in transition.py closed the path that was demonstrated, but not the class. build_composition hands one connection to every collaborator, and each of the three stores in rqa/github/store.py ends its write with an unconditional connection.commit(). decide() leaves the decision entry and the escalation close uncommitted on purpose; resume()'s first action is a live GitHub read, so the first successful exchange committed work that was meant to be provisional. A later validation failure then had nothing to undo: the escalation was durably closed, the job stuck at escalated, and the retry refused ALREADY_CLOSED. The composition root now hands those three stores a connection view whose commit() is inert, rather than stripping the commits from store.py, which would change behaviour for every standalone caller of that module. Their rows become durable when the transaction's owner commits -- tick.py after every admitted repository, main.py at the end of each command. rollback() through the view raises: a collaborator discarding the owner's transaction is the same defect in the other direction, and silence there would hide it. Second: onboard took a checkout path while tick --repo took an owner/repo slug, and both resolved through config_path(), so a repository onboarded one way was refused at admission the other. onboard now requires the same slug tick does, still guarded by is_dir() so no phantom .rqa/ tree is ever created. The one layout under which the two agree -- checkouts at <cwd>/<owner>/<repo> -- is now stated in OPERATORS.md rather than left implicit, along with the invocation neither operator document previously showed. SKILL.md now says plainly that rqa is pre-cutover and that its decision surface is disjoint from scripts/human_cli.py. Tests: the suite was structurally blind to the commit defect because every decide()/resume() test substitutes a fake github that never reaches a store. Three tests now drive the real Transport over the real Sqlite stores; two of them fail against the pre-fix wiring. The onboard contract change is pinned by its non-slug case, which the pre-fix code accepts. Signed-off-by: tucktuck101 <pc2fct5rd8@privaterelay.appleid.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…escalation-surface Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Round-two findings addressed at
|
serina-mcfall
left a comment
There was a problem hiding this comment.
In plain terms
Both Blockers and all three Highs are fixed, verified by reproduction rather than by reading your diff. Nothing blocking remains. I am dismissing my change-request.
And the atomicity fix is better than the one I asked for. I said the commit discipline had to change across store.py:162, :216 and :261. You left all three commits in place — correctly — and interposed _CallerOwnedConnection at the composition root instead, so the stores keep working for a standalone caller that owns its own connection while the composition's transaction survives. That is the right seam, and your docstring argues it better than my finding did.
I checked it does what it says rather than trusting the shape: commit() returns None, rollback() raises, everything else delegates, and all three stores receive the view.
Review — verdict: PASS (2 Mediums open, 1 new Medium)
Head 814d70d0, merge-base 69f7a55d9fb8. 46 files, +5384/-157. Suite at head: PASSED: 2304 test(s), up from 2297.
The remediation commit 0bf4640bc touches exactly six files, which bounds what can and cannot have been fixed — OPERATORS.md, SKILL.md, rqa/cli/composition.py, rqa/cli/main.py, tests/test_rqa_cli_main.py, tests/test_rqa_decision_atomicity.py.
Prior round — everything blocking is fixed ✅
| Finding | Severity | Verdict |
|---|---|---|
| Blocker 1 — atomicity defeated by a foreign commit | Blocker | Fixed, at the composition root |
Blocker 2 — tick and onboard disagree about "repo" |
Blocker | Fixed — a full cycle now completes |
High 1 — same defect at :162, :216, :261 |
High | Fixed — all three, by one guard |
| High 2 — no test can see any of this | High | Fixed — and non-tautological, proven two ways |
| High 3 — replacement CLI absent from the operator docs | High | Fixed — 0 hits → 14 |
Blocker 1. The contrast, reproduced against the real Transport and real Sqlite*Stores with only the network send stubbed — raw wires the stores over the bare connection (the pre-fix shape), composition mirrors what build_composition hands them at head:
MODE = raw MODE = composition
AFTER decide(): in_transaction = False AFTER decide(): in_transaction = True
AFTER connection.rollback(): AFTER connection.rollback():
decision rows: 1 decision rows: 0
escalation status: closed escalation status: open
job status: changes_requested job status: escalated
etag rows: 1 etag rows: 0
api_call rows: 2 api_call rows: 0
The raw lane matters: it confirms the store commits are still live and still destructive, so the guard is doing the work rather than the fixture flattering it.
High 1. All three commits survive at store.py:162,216,261, and all three stores get the view:
github_connection = _CallerOwnedConnection(connection)
transport = Transport(etags=SqliteEtagStore(github_connection),
api_calls=SqliteApiCallStore(github_connection))
github = GithubAdapter(transport=transport,
mutations=SqliteMutationStore(github_connection), clock=clock)Nothing under rqa/github/ uses with <connection>: or calls rollback(), so the raising rollback() is unreachable in practice and the __getattr__ delegation cannot be bypassed by a with block.
Blocker 2. Run for real, from a tree laid out as <cwd>/<owner>/<repo>:
$ python3 -m rqa.cli onboard block # the OLD contract is now refused
{"detail": "repository must be an owner/repo slug", "outcome": "usage_error"}
$ python3 -m rqa.cli onboard block/buzz
{"command": "onboard", "outcome": "written", "result": {"path": "block/buzz/.rqa/config.json"}}
$ python3 -m rqa.cli tick --repo block/buzz
"repos_admitted": ["block/buzz"], "repos_refused": [],
The admission boundary that refused with unreadable ... [Errno 2] now passes. The remaining failure is an unauthenticated network read in the sandbox, not the defect.
High 2, proven non-tautological two ways. Reverting only the composition wiring turns the composition-root test red; making _CallerOwnedConnection.commit() actually commit turns two tests red, including test_a_real_github_read_inside_decide_does_not_commit_the_decision. Worth knowing: that second test constructs the view directly rather than through build_composition, so it alone cannot catch a wiring regression — the composition-root test is what covers that. The pair together is sufficient; neither alone is.
One precision on my own re-request wording. I asked for "a test that drives a real GithubAdapter through decide()". What you built drives a real Transport and real stores, wrapping the fake adapter to perform the exchange itself — deliberately, to avoid assembling every GraphQL fixture. That reaches the defect class, so I read it as satisfied.
High 3. My own check from last round, re-run verbatim, now returns 14 hits — including a full invocation block at OPERATORS.md:47,71-78, the <cwd>/<owner>/<repo> layout convention documented at :64 (the adjudicator's complaint that it "appears nowhere"), and an entry-point index that now cross-references the rqa section instead of listing only scripts/*.py. python3 -m rqa.cli --help lists exactly the six documented subcommands.
Still open — the two Mediums
Neither file is in the remediation commit.
architecture/code/P-11-escalation.md:343— still says tick's incomplete outcome is "exit 2 (network) or 3 (authentication)".main.py:205-208returns three codes;exitcodes.OTHER(4) is returned for theinternal_errorreason thattick.py:123really produces. Short one branch of theif/elif/elseit describes.tests/test_rqa_escalation_security.py:230—decide.py:103'sdel escalationguard still has no credential-reachability test.grep -rn "must include --outcome" tests/returns zero hits across the whole tree; the test at:230exercises the sibling raise at:105-112. The guard is real and remains untested.
New — one Medium the remediation introduced
architecture/code/P-11-escalation.md:344 — the fix changed onboard from "existing local repository directory" to an owner/repo slug, and updated main.py's help, OPERATORS.md and SKILL.md. The architecture document was not updated, one line below the stale exit-code prose:
344:`onboard` requires an existing local directory and checks the platform keychain;
$ ls -d block # an existing local directory
block
$ python3 -m rqa.cli onboard block
{"detail": "repository must be an owner/repo slug", "outcome": "usage_error"}
It is the only sentence in the whole architecture/ + requirements/ tree carrying the old wording, so it is a one-line edit. Filed as new because the document was correct until main.py changed underneath it. Both Mediums live in the same file, so they are one edit's worth of work.
What I looked for and did not find
I went hunting specifically for the "fix that trades one bug for another" shape in _CallerOwnedConnection:
- Dunder bypass —
__getattr__does not serve implicit special-method lookup, sowith view:would raiseTypeError. No code underrqa/github/uses that form, and only those three stores receive the view. - Lost audit rows on rollback — the three stores' rows are now discarded when a command rolls back, so I checked whether anything budgets rate limits off the
api_callstable it can no longer rely on. Withinrqa,api_callsis write-only; the only reader isscripts/dispatcher.py:484, the legacy workflow, on its own connection. - Read-only commands —
status,pendingandexplainnever commit and never touchcomp.github, so nothing is silently dropped there.
Open questions, recorded rather than claimed
- Shared state database.
rqaopensstate_dir/state.db; the legacyscripts/stack opens its owndb_path. I did not establish whether those resolve to the same file in a real deployment. If they do,dispatcher.py:484's rate-limit read would see anapi_callsledger an abortedrqa decidenow discards. Pre-cutover the two surfaces are documented as disjoint, so this is a question, not a finding. - Keychain path. My
onboard/tickcycle ran against a localsecret-toolshim returning a synthetic throwaway value, because this machine has no Secret Service session. The real Linux backend was verified last round, not re-verified here. - CI at this head was not re-checked as part of this pass;
gh pr checks 2269reports no failures.
I am independent of this work: I did not author any part of it.
Dismissing my change-request — every finding in it is resolved above. The three items remaining are all Mediums in one file, and none of them blocks.
Dismissing as addressed. All five findings (2 Blockers, 3 Highs) verified fixed at head 814d70d by reproduction, not by reading the diff: atomicity closed via _CallerOwnedConnection at the composition root (in_transaction stays True, rollback undoes the decision); tick/onboard now share the owner/repo slug and a full cycle completes; store.py:162/216/261 all covered by the one guard; two new tests go red under two separate mutations; OPERATORS.md/SKILL.md grep went 0 hits to 14. Detail in the review posted 2026-09-16.
|
Follow-up issues filed for the three Mediums in my review, so they are not lost now this PR is unblocked:
|
Summary
Provides the operator CLI and escalation lifecycle, with atomic human decisions, configured repository authority, keychain preflight and accurate failure exits. Integrates the approved permission and platform fixes; control text remains visible in the audit trail and offline explanations carry the recorded human basis.
Feature
#2186
Related issue
Closes #2195
Closes #2211
Closes #2186
Closes #2274
Closes #2278
Closes #2279
Closes #2280
Closes #2281
Refs #2006
Issue type
Feature
Agent provenance
anthropic/claude-opus-5. Lanes: #2195anthropic/claude-sonnet-5; the D-B5-2 resume fixanthropic/claude-opus-5; the store-publication laneanthropic/claude-opus-5; #2211openai-codex/gpt-5.6-sol. Review seats:openrouter/z-ai/glm-5.3,openai-codex/gpt-5.6-sol.workmux/plan-rqa.md, gitignored and local to the operator's machineFollow-up implementation and validation: Codex, GPT-6, initiated by @tucktuck101; session reference not exposed.
Objective
Make the operator command surface usable through production collaborators and address the review failures.
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
Preserve the decision caller’s transaction with nested savepoints and propagate resume persistence failures. Supply the configured repository set to Gate; qualify write attestations with OAuth scopes. Check keychain availability before state-changing commands; reject malformed inputs and missing state. Escape control text visibly instead of deleting it. A real two-escalation test fails when resume deliberately reads the oldest entry. Dependency commits are retained without rewriting reviewed history.
Verification
Command run:
Raw output:
The subsequent dependency merge adds one reconstruction test; its focused run:
Recency mutation (oldest escalation selected), expected red; last 12 lines:
Not verified
Live GitHub approvals/merges, real model reviews and a real Linux Secret Service session. Native macOS append/reconstruction used a temporary database; the key was absent, so the row was correctly unkeyed and unverified. Structured escalation specificity is covered by stacked PR #2297; the broader PRD conformance work remains separate.
Authority
@tucktuck101 explicitly instructed these decisions:
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.