fix: harden agent skills via iterative adversarial review - #27
Merged
Conversation
Address a full-suite review of the agent skills (except prepare-changesets): - Bundle the review-suite contract and schemas into each review skill under references/review-suite/ so skills work when installed outside this repository; add `just sync-contracts` and a byte-identity drift test. - Use skill-root-relative watcher paths in babysit-pr and document the shared state lock between --watch, --once, and --retry-failed-now. - Harden gh_pr_watch.py: survive transient GitHub CLI failures in watch mode with bounded backoff, cache the authenticated login, report unresolved threads on closed PRs, and emit confirm_feedback_disposition whenever published feedback exists so verify_external_gates is never read as a disposition claim. - Add --max-polls and --stop-when-clear bounded watch modes plus background-task guidance for runtimes with bounded foreground command windows. - Move eval answer keys out of reviewer-visible input directories and rename results.json to expectations.json; document each evals/ directory's consumption and blinding rules. - De-brittle the deterministic forward executor (whitespace-normalized matching) and add a real-runtime Claude headless executor with a `just eval-implement-ticket-claude` target. - Adapt the suite to Claude runtimes: trigger-oriented descriptions, allowed-tools on the read-only review skills, agents/claude.md adapters, a root CLAUDE.md, host-neutral instruction-file naming, and terminal-handoff examples. - Trim contract tests to load-bearing invariants instead of prose string-matching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address the round-two independent review (B1-B10 plus the adapter naming collision): - B1: count gh's `cancel` bucket in check summaries and require empty failed runs/jobs in is_github_candidate_clear so a cancelled-CI PR can never read as clear or satisfy --stop-when-clear. - B2: run the review-suite tests (including the bundled-contract drift test) in the GitHub Actions unit-test step. - B3: bundle the dependency-free validate.py into each review skill via sync-contracts, verify it in the drift test, and point the orchestration protocol and CONTRACT.md at the bundled copy. - B4: make --stop-when-clear imply ready_to_merge when no completion policy is given, reject only an explicit watch_until_closed, and add parse_args tests covering every documented CLI invocation. - B5: remove the dead TemporaryDirectory branch in run_forward.py. - B6: fail closed when `gh pr checks` returns an empty payload unless gh explicitly reports that the PR has no checks. - B7: surface comments from deleted (ghost) authors as new feedback. - B8: move default watcher state into a per-user mode-0700 directory. - B9: unify observed_sequence on lens tokens across eval records. - B10: add forbidden-action grading to the forward harness so an executor cannot pass by emitting the whole action vocabulary. - Rename agents/claude.md to agents/claude-code.md so the adapter no longer collides case-insensitively with the CLAUDE.md memory-file convention on macOS/Windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address the round-three adversarial review (H1, M1-M4, and the lows): - H1: resolve validator schemas beside validate.py first, falling back to ../contracts/, so the bundled copy works in the installed skill layout; execute every bundled copy in place in the drift test and import the bundled validator (not the canonical one) in review-skill tests so this class of bug cannot ship silently again. - M1: scope failed workflow runs and jobs to the runs backing the PR's own checks; other failed head-SHA workflows are reported separately under non_pr_check_failed_runs and can no longer block clearness or produce permanently unretryable retry recommendations. - M2: emit resolve_draft_state and resolve_merge_conflict actions so draft and conflicting PRs never degenerate to a bare idle. - M3: define forbidden_actions on all 18 forward expectations, add a vocabulary-spam canary test that keeps them complete, and stop backfilling target_skill in the Claude executor so the grader's identity check has teeth. - M4: document that forward cases carry pre-classified scenario flags and grade obligation mapping, not evidence classification. - Lows: reject --once with --watch; handle OSError through the clean error channel; document the gh 2.37 floor and default-state durability limits; correct the misleading lint-skills install message; state honestly that allowed-tools does not make Bash read-only and that the worktree-state check is the backstop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Accept cancelled-only check failures in the retry gate, mirroring recommend_actions, so a recommended retry is never refused with no_failed_pr_checks. - Grant review-code-change the Agent/Task/Skill tools its Claude adapter requires for fresh lens subagents, keep file-editing tools excluded, and describe the caller fallback when subagents are unavailable. - Reject --once combined with --retry-failed-now. - Extract run IDs from legacy query-string details links so those failed PR-check runs stay diagnosable and retryable. - Add a repository digest to default state filenames so distinct repositories can never collide on one state file. - Keep diagnose_ci_failure in the recommendation set when the flaky retry budget is exhausted; the budget only ends retries, not diagnosis of a new fixable failure. - Document that --stop-when-clear never fires on a zero-check repository and should be paired with --max-polls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-five follow-up applying the remaining verified lows; the adversarial reviewer judged the suite converged with these as the only items worth a small final commit: - Mirror is_github_candidate_clear in has_failed_pr_checks so a PR-check-backed failed workflow run always yields a diagnosis recommendation instead of `idle`. - Case-normalize the repository before deriving default state files and locks; GitHub slugs are case-insensitive, so --repo Owner/Repo and a URL-derived owner/repo now share one lock. - Extract run IDs from fragment-suffixed details links. - Reject --repo without an explicit --pr at parse time instead of failing at the first gh call. - Make boolean schema constants reject numeric 1/1.0 in the shared validator (and its bundled copies). - Document --poll-seconds and --max-flaky-retries in the watcher reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shaug
added a commit
that referenced
this pull request
Jul 21, 2026
The initial review-code-change pass on PR #27 found the same recommend/refuse divergence this branch fixed for cancelled checks: recommend_actions counted PR-check-backed failed runs as failed PR checks while the retry gate did not, so a recommended retry for a failed-runs-only state was refused with no_failed_pr_checks. Extract one shared has_failed_pr_checks predicate and use it in both sites, with a failed-runs-only regression test. Also apply the ride-along deferred finding: validate_state_target now compares repositories case-insensitively, matching the normalization default_state_file_for already applies, so mixed-case invocations share one state file instead of failing closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The initial review-code-change pass on PR #27 found the same recommend/refuse divergence this branch fixed for cancelled checks: recommend_actions counted PR-check-backed failed runs as failed PR checks while the retry gate did not, so a recommended retry for a failed-runs-only state was refused with no_failed_pr_checks. Extract one shared has_failed_pr_checks predicate and use it in both sites, with a failed-runs-only regression test. Also apply the ride-along deferred finding: validate_state_target now compares repositories case-insensitively, matching the normalization default_state_file_for already applies, so mixed-case invocations share one state file instead of failing closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shaug
force-pushed
the
claude/agent-skills-review-5fc5b9
branch
from
July 21, 2026 15:54
7b2afc4 to
625dae6
Compare
The code-simplicity lens on PR #27 flagged the one remaining inline copy of the failed-PR-check policy inside is_github_candidate_clear — the same duplication class that twice produced self-contradictory watcher states. Replace the inline failed/cancelled/runs/jobs conditions with the shared predicate; behavior-preserving, pinned by the existing cancelled-check and failed-runs-only tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden the agent-skill suite (all skills except
prepare-changesets) throughfive commits driven by iterative adversarial review, until a full adversarial
pass returned no findings worth addressing.
both schemas, and the dependency-free
validate.pyunder its ownreferences/review-suite/(dual-layout schema resolution), kept in sync byjust sync-contractsplus a byte-identity drift test that also executesevery bundled copy in place.
babysit-pr): transient-failure backoff in watchmode; cancelled checks and PR-check-scoped failed runs/jobs in the clear
predicate and retry gate;
resolve_draft_state/resolve_merge_conflictand
confirm_feedback_dispositionactions; bounded watch modes(
--max-polls,--stop-when-clear); exhaustive CLI-combinationvalidation pinned by parse tests; per-user 0700 state directory with
case-normalized, digest-suffixed state files; fail-closed empty
gh pr checkshandling; ghost-author feedback surfacing.directories;
forbidden_actionson all forward expectations with avocabulary-spam canary; no
target_skillbackfill in the Claude headlessexecutor (
just eval-implement-ticket-claude); honest documentation ofthe pre-classified-flag eval design.
allowed-toolsonreview skills (orchestrator additionally gets Agent/Task/Skill),
agents/claude-code.mdadapters (renamed to avoid the case-insensitiveCLAUDE.md collision), root
CLAUDE.md, host-neutral instruction-filenaming, terminal-handoff examples, and background/bounded-window watch
guidance.
load-bearing invariants; review-suite tests now run in CI; review-skill
tests import the bundled validator so installed-layout breakage cannot
ship silently.
Why
Three independent adversarial reviews surfaced portability breaks (skills
unusable when installed standalone), watcher dead-ends (cancelled CI reading
as clear, non-PR-check failures wedging
--stop-when-clear), evalcontamination and grading holes, and Codex-shaped runtime assumptions.
Severity trajectory across rounds: 1 high + 4 mediums → 1 high + 4 mediums →
2 mediums → 6 lows → converged.
Validation
just test— 194 tests across 9 suites, all passingjust lint— ruff, mdformat, andskills-ref validateon all 8 skillsjust eval-implement-ticket— 18/18 forward evaluations🤖 Generated with Claude Code