feat(skills): dispute wrong claims, align PR and evidence gates, refresh VM and roster guidance - #201

Summary

Neither pr-ready nor issue-to-pr could say that the thing it was asked to act on was wrong.

• pr-ready: the gate blocked on every unresolved thread unless it was rated P3/P4, nothing told the orchestrator to reply, and the shared rubric's "a finding that does not reproduce is returned, not fixed" only covered the skill's own pr-reviewer. The easiest way through the gate was to change code until the comment went away. The gate also never looked at most of this repository's reviews: the last 60 PRs have 0 inline review threads; every review is a top-level PR comment.
• issue-to-pr: INVALID_OR_CLOSED was an undefined label. Dismissing an Issue needed no independent check and no reply on the Issue. A partly wrong Issue (real symptom, wrong cause, proposed fix or acceptance criterion) had no path: SA writes the acceptance criteria and the gate only checks the ones listed, so a stated criterion could be dropped silently. create_pr.py required Fixes #N even when part of the Issue stays open, and the documented gh issue view did not fetch comments.

What changed

Both skills now treat what they are given as claims to verify, with the same shape:

• Triage first. pr-ready: each review comment is valid, invalid, needs_evidence or nonblocking (P3/P4). issue-to-pr: each Issue claim (symptom, stated cause, proposed fix, stated acceptance criteria) is valid, invalid, needs_evidence or out_of_scope. Only valid claims are implemented. Protocols: pr-ready/references/review-comments.md, issue-to-pr/references/issue-claims.md.

• Independent verdict. The reviewer gets the claim and the code, not SA's or Root's conclusion, and returns holds / does_not_hold / undetermined. A dispute stands only if that verdict agrees; otherwise the claim counts as valid.

• Answer where the author sees it. pr-ready: in the thread (then resolve it) or a PR comment containing the disputed URL. issue-to-pr: a comment on the Issue, containing the source comment's URL when the claim came from a comment. Recorded as disputed_review_comments / disputed_issue_claims.

• No loop. A claim raised again without new evidence after a confirmed dispute goes to a person (DRAFT-BLOCKED / NEEDS_DECISION), not into another fix or reply round.

• Gates check the replies on GitHub. pr_gate.py: both comments on the PR, reply later, inline reply in the same thread and still its newest comment, other replies contain the disputed URL. issue_gate.py: source is the Issue or one of its comments, reply is a later comment on the Issue and links the source comment. Both: entry HEAD-bound and reviewer-confirmed. Comments are fetched only when the evidence has disputes.

• issue-to-pr outcomes. INVALID_OR_CLOSED is split into CLOSED and INVALID; INVALID and NEEDS_DECISION mean no PR, a reply on the Issue, and the Issue left open for a person. issue_resolution: "partial" (needs at least one dispute) makes create_pr.py require Refs #N with no closing keyword; fixes (the default) requires Fixes #N and is refused while a claim stays open: any needs_evidence dispute, or an out_of_scope one without a follow_up_issue_url (same repository, not this Issue, linked in the reply, and confirmed by gh issue view to be an Issue, open or closed). mark_ready.py re-checks the PR body against issue_resolution before gh pr ready. The documented gh issue view now fetches comments.

• Role prompts (SA, Coder, Reviewer of both skills) updated identically on claude/codex/agy; PROJECT-CHANGES.md of each skill records the local edits.

• pr-ready gate on repositories without required checks. gh pr checks --required exits 1 with no JSON and no required checks reported on the '<branch>' branch when nothing is required, and pr_gate.py failed on the missing JSON, so the gate could never pass in this repository. It now reads that as zero required checks only when gh exits 1 with empty stdout and stderr is exactly that message; valid JSON on stdout is always parsed first, so a failing required check whose name contains the phrase is kept. issue-to-pr's pr_ci.py had the same fallback with a substring match (pre-existing on base) and now uses the same rule; pr-ready/tests/test_required_checks_parity.py runs one matrix through both helpers. Every other check must still pass and any other failure still fails closed. The fake gh in the tests always answered --required with JSON, which hid this; it now reproduces the real output.

• No Draft/Ready loop between pr-ready and pr-review-merge. pr-ready could mark a PR Ready in states pr-review-merge returns to Draft: an unresolved P3/P4 thread, a reviewer-confirmed unrelated failing check (every check here is non-required), and CHANGES_REQUESTED. pr-review-merge also never saw the disputes, so a fresh reviewer could repeat a disputed finding. Now:

  • pr-ready requires every thread resolved (P3/P4 threads are disputed as nonblocking and resolved) and rejects the upstream unresolved_nonblocking_threads field.
  • pr-review-merge decides WAITING, never MERGE, for a check listed in unrelated_failing_checks (same shape as pr-ready's) and for CHANGES_REQUESTED (waiting for re-review); open objections still come back as reviewer findings.
  • Root passes disputed findings and their replies to prm-reviewer. A repeated finding carries disputed_reply_url and drafts only with new_evidence; otherwise WAITING for a person, as on the pr-ready side. P4 findings are accepted (shared rubric).
  • pr-review-merge/tests/test_pr_ready_handoff.py runs both gates over the same GitHub states and fails if a state pr-ready accepts is DRAFT in pr-review-merge, or MERGE with a failing check.

• No rm -rf in the shared VM image directory. Four vm-target-spec-testing passages said to sudo rm -rf /var/lib/libvirt/images/pilot/<name> after down, and the clean-room contract's first teardown step removed every VM "belonging to the runbook". Every clone and session on the host shares the pilot data dir and that directory, so both could delete another session's VM. The rm -rf was never needed: down removes the per-target directory, and a Docker bind mount inside the VM lives in the overlay it deletes (no host/VM shared filesystem). The docs now say to down only VMs this session created and to report a directory that survives down; the clean-room step removes only this run's own or user-named VMs. TestDown_RemovesState now asserts the directory is gone.

• freeipa-roster-authoring caught up with the code. Schema v3 (it taught v2); the real lint output; lint had no vault option and exited 1 on an encrypted roster (the skill said it decrypts in memory), so the skill now uses the new roster lint --vault-password-file (below); and a new section on what state: disabled / state: absent do in FreeIPA (absent is a permanent ipa user-del), that deleting a YAML entry revokes nothing, that absent entries are tombstones that stay, and pilot roster remove-user|remove-group for never-applied entries.

• pilot roster lint --vault-password-file (Go). Lint reads the roster once (inventory.ReadRosterPlaintext) and decrypts an ansible-vault roster in memory with the same ansible-vault view call roster migrate uses; no plaintext file is written, and the report is the same as for the plaintext roster. A YAML error in a decrypted roster keeps only the line numbers (yaml.v3 quotes part of the value). Without the flag an encrypted roster exits 1 with "rerun with --vault-password-file"; the flag is not read for a plaintext roster; --upgrade re-encrypts through MigrateRosterFile. Without --upgrade lint takes no lock and writes nothing.

• Schema v3 in the example and docs. playbooks/apply/freeipa-identity.roster.example.yaml gets schema_version: 3 and grants: [], exactly what lint --upgrade and MigrateRosterV2ToV3 produce, so behaviour does not change. docs/runbooks/freeipa-identity.md §3 shows v3 and a new step 5 (lint an encrypted roster) with output from a local run; docs/verification/freeipa-identity.md §0/§1.6 say v3 is current and 1/2/3 are accepted (the apply gate's in [1, 2, 3]), with no checklist row changed; minimal-poc-configuration.md and a trec reference no longer say the wizard writes v1.

• issue-to-pr mark_ready.py refuses open review feedback. It reads every page of the PR's review threads and refuses Ready while one is unresolved (pr-review-merge would return that PR to Draft), and refuses reviewDecision CHANGES_REQUESTED, pointing at pr-ready, which owns review feedback on a Draft. A failed or malformed lookup blocks. GraphQL query and answers captured from gh 2.103.3 (feat(skills): dispute wrong claims, align PR and evidence gates, refresh VM and roster guidance #201, fix: handle non-zipped artifact downloads cli/cli#13013). Also fixed, from the last review: a mistyped issue_resolution such as Partial is reported as such instead of as a missing Fixes #N, and a follow-up Issue number over 10 digits is a format error instead of an int() traceback.

• pr-review-merge checks the mandated actual-run record before MERGE. The gate reads .github/pr-evidence.json at the PR's base SHA (a PR cannot weaken its own requirement) and target_validation: blocked, a missing or malformed record, a required record whose artifact is not committed at HEAD, outside artifact_glob or not naming the tested revision, or a tested revision that is not HEAD or an ancestor with an evidence-only delta, all make it WAITING, never MERGE (DRAFT still needs the reviewer's P1 finding). The manifest gains required_paths (playbooks/**, docs/verification/**, contracts/**), for which pr-review-merge refuses not-needed and issue-to-pr's gate refuses not-applicable (from the merge base, both names of a rename), so an Issue PR does not reach Ready only to wait at merge. The delta logic is issue-to-pr's repo_evidence.py, loaded by path so both gates share one rule; the data comes from GitHub's contents and compare APIs (captured shapes; a compare at GitHub's 300-file cap fails closed). The integration tests now lock a per-test PRM_STATE_DIR: they used ~/.cache/pr-review-merge, so two concurrent make skill-test runs failed each other.

• vm-target-spec-testing rewritten (it was about three months behind). Main flow is vm-target test (syntax, check/diff, snapshot, apply, verify, idempotency) and topology test, with --ephemeral for fresh hosts and cross-playbook ordering; manual up/run/verify is for debugging. Verify playbooks are no longer described as spec --generate output, and output goes to a candidate-bound docs/evidence/ record instead of being pasted into runbooks. AGENTS.md rules are linked from a "your change touches X → read §Y" table, not copied. Other stale claims fixed against --help and the code: --sandbox is not run's default and test has none; run passes -- through as the playbook path; a container image without an ssh client; resize-disk instead of re-provisioning. SKILL.md 426 → 261 lines.

• pilot-trec-verification no longer contradicts AGENTS.md. The sudo gotcha called sss_cache -E && systemctl restart sssd "the fix"; AGENTS.md §5.9.5 says a result that needs a cache flush is a bug symptom. It now records what is known (seen in minimal-poc rounds 13, 23 and 31 even after c45ed559 added a flush to the identity playbook; bug(fixtures): hostauthz/audit sim 的 sudo 檢查依賴 client SSSD 的 sudo refresh 時機，結果隨快取狀態變動 #95, bug(freeipa-identity): 撤銷 SSH key 後，enrolled 主機的 SSSD 快取仍讓該 key 登入一段不定的時間 #114), says the root cause is unknown, and says to capture facts, file or update an issue, and treat that run as a deviation, not passing evidence. Roster guidance uses pilot edit's roster editors (a table of screens from the code) and allows hand edits only for what no screen covers, each named. A formal run starts from a frozen candidate (AGENTS.md §1.5). Runbooks no longer carry driver scripts or cast paths.

• Flaky drift-probe test. "templated ansible_host over ssh" used the bare password as a single-label host name; on CI run 41076444653 two subtests got "details withheld" instead of "Could not resolve hostname" (ssh prints that for every resolver error, so the name most likely resolved through the runner's search list; the raw message is withheld by design). Both copies of the test now append .invalid. (RFC 6761, absolute), so no resolver answers it; the sentinel still reaches ansible's raw output.

Limits

• pr-review-merge's gate checks only the shape of disputed_reply_url (a comment on this PR), not that the comment exists or what it says; an unflagged repeat of a disputed finding is not detectable by the gate, only by Root and the reviewer reading the replies. Ran read-only against this PR live: a repeat without new evidence was WAITING, not DRAFT.
• pr-review-merge's actual-run check: a PR that merges the base branch after its run has a non-evidence delta and waits for a person (strict, per docs/actual-run-evidence.md §4, and likely common); "names the tested revision" accepts any 7+ hex prefix in the record; a manifest with more than one required_evidence entry is WAITING.
• This PR itself changes two files required_paths covers: playbooks/apply/freeipa-identity.roster.example.yaml (data, not a playbook) and docs/verification/freeipa-identity.md (prose, no row). The base manifest has no required_paths, so the gates take the not-applicable below; under the new rule such a change needs a run record or a person's decision.
• mark_ready.py checks review threads and the review decision only. Review findings posted as review bodies or conversation comments (how this repository reviews) are not machine-checked there; --confirm-complete now attests they are answered.
• The flaky test's cause is inferred, not observed: the probe withholds ansible's raw message.
• Found and left alone: make poc-checkmode-test (scripts/topology-checkmode-test.sh) tears down every VM it adopted by name, including another session's (the skill now warns to check vm-target list); AGENTS.md §1.3/§3 cite TestRegression_SpecAndInventoryAgree, which does not exist; trec references still cite deleted tui_multiselect.go/tui_confirm.go/tui_textinput.go; freeipa-identity-apply.yml:89 and topology-checkmode-test.sh:41 comments still mention schema 1/2; minimal-poc-architecture.md still calls the SSSD flush the fix (that runbook has its own update skill).
• The gates cannot judge whether a dispute is right; that is the reviewer's independent verdict.
• For PR conversation comments and Issue comments the gates cannot see a later disagreement that does not quote the reply; only inline PR threads have the "reply is still the newest comment" check. Root and the reviewer read later comments, and the escalation rule applies.
• The reply/resolve commands in the two protocol docs (addPullRequestReviewThreadReply, resolveReviewThread, POST issues/{n}/comments) were checked against the GraphQL schema/REST shape only, not executed, since that would post to GitHub.
• gh_issue_thread relies on gh issue view --json comments returning every comment; no Issue here has more than 100 comments to confirm the paging.

Verification

• make skill-test: issue-to-pr 200, pr-ready 51, pr-review-merge 80, all passing (the base had 119 / 16 / 42).
• go build ./..., go vet, and full go test of cmd/pilot/cmd, internal/inventory and internal/accessgrants pass locally; spec --lint of freeipa-identity.md 0 findings; the identity spec regressions and TestSpecPlaybookTagAlignment pass; the example roster lints ok: schema v3.
• roster lint --vault-password-file: real cobra command against rosters encrypted with ansible-vault: same report as plaintext for six fixtures including the example, wrong and missing password file, flag ignored on plaintext, no decrypted content left in the roster dir, $TMPDIR or ANSIBLE_LOCAL_TEMP, --upgrade re-encrypts. All seven fail against the previous lint.
• mark_ready.py / P4 fixes: 10 mutants of the new checks all fail the tests; an unmutated copy passes. pr-review-merge: 24 of its 32 new tests fail against the 74eefe2 gate (the others are runtime-shape and lock-dir tests); a read-only live run accepted bug(os-patch-sla): target_group 寫死成 sandbox，staging／prod stage 下 play 選不到主機，patch SLA 從未套用 #177's real record and refused it with the baseline SHA as tested_revision.
• Independent review of the code commits (af9189a, c66a5bf, 2f75964): PASS, no P0–P2. It made the old single-label host resolve with HOSTALIASES and showed the old tests fail and the new ones pass; ran unresolved_threads, file_at and compare read-only against live GitHub; and its own mutants of mark_ready and the gate were caught (two survivors are redundant with other checks). Its two P3s are fixed in e99c127: issue-to-pr now refuses not-applicable for required_paths (5 mutants, each caught), and the pr-review-merge hand-off bullet no longer contradicts the reviewer's P1 for a missing record.
• Review round on e5f499e (comment): R1 and R2 fixed in fc54eab and 74eefe2. An independent reviewer confirmed both closed on 74eefe2: the new tests fail at e5f499e for the intended reason (50 failures), the counterexamples are rejected or kept at HEAD, and its own 17 mutants are all killed; the implementer's 35 mutants, 0 survivors. gh issue view (Issue, PR number, missing number) and the no-required-checks stderr fixtures were re-captured from gh 2.103.3.
• Every new guard in the gates and in create_pr.py mutation-checked: removing each one makes a test fail. For the hand-off: putting back pr-ready's thread exemption, drafting unrelated checks or CHANGES_REQUESTED, dropping P4, drafting a repeat without new evidence, accepting short evidence or a reply on another PR, and unconfirmed or short exemptions each fail a test.
• Required-checks fix: run read-only against this PR with gh 2.103.3. Before the fix the gate stopped at Invalid/missing JSON from gh (exit 1): no required checks reported .... After it, the gate evaluated every check with no CI error (only the deliberately minimal evidence file was reported). The fake gh output is copied from that capture. Searched every gh pr checks caller in .agents/skills: pr_ci.py already handled it, and pr-review-merge does not use --required.
• VM teardown: real vm-target up, a file written inside the VM, vm-target down on this host: the per-target directory, libvirt domain and state record were all gone. TestDown_RemovesState fails if teardown stops removing the directory.
• Roster skill: every lint line and failure quoted was captured from a pilot binary built from this branch, on the repository example, its --upgrade copy and an ansible-vault encrypted copy (throwaway password, deleted). State semantics read from freeipa-identity-apply.yml, roster_validate.go and roster remove-*.
• Fixture shapes copied from real output: GraphQL PR threads/comments/reviews (fix(detection-engine): only back up state.db when this run will install the binary #190, fix(freeipa,dns): leave out replicas FreeIPA does not list as DNS servers; one forwarder rule (follow-up to #73) #92, Document extrepo installation on Debian cli/cli#14796) and gh issue view --json number,url,createdAt,comments (bug(os-patch-sla): target_group 寫死成 sandbox，staging／prod stage 下 play 選不到主機，patch SLA 從未套用 #177). Both dispute checks were also run read-only against real data (fix(freeipa,dns): leave out replicas FreeIPA does not list as DNS servers; one forwarder rule (follow-up to #73) #92, bug(os-patch-sla): target_group 寫死成 sandbox，staging／prod stage 下 play 選不到主機，patch SLA 從未套用 #177): a reply without the link or before the claim is rejected, a valid one passes.
• go test ./cmd/pilot/cmd -run TestAgentSkill: layout/prompt consistency passes. py_compile, git diff --check clean.
• actual-run evidence (.github/pr-evidence.json): not applicable. The Go change is roster lint, a read-only CLI path proven by native-tool tests with real ansible-vault; the example roster changes exactly as the loader's automatic v2→v3 migration does; the spec change is prose only (no row, command or playbook); everything else is skills, docs and tests.

🤖 Generated with Claude Code