diff --git a/CHANGELOG.md b/CHANGELOG.md index ea68b7c..f94d955 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-19 — Epic workflow and review contract cleanup +- feat: integrate repository-owned review into epic execution - feat: compose repository-owned code review + (556fea80b6970b97c31e819693f43c251b7b3796) - feat: add local code simplicity review (d6ed890f6924a2ae7ae4b04fa95072ee853c9b97) - feat: add whole-solution simplicity review diff --git a/README.md b/README.md index 884f2d2..8aaea79 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,8 @@ Current skills: - `skills/review-code-change` — orchestrate the repository-owned review lenses into one evidence-bound, deduplicated verdict - `skills/implement-epic-sequence` — execute GitHub or Linear epics through - dependency-aware implementation, review, merge, cleanup, and closeout + dependency-aware implementation, repository-owned review, remote gates, merge, + cleanup, and closeout - `skills/prepare-changesets` — decompose a large, review-ready branch into a deterministic chain of smaller, reviewable changesets and GitHub PRs - `skills/review-correctness` — find material behavioral, security, diff --git a/skills/implement-epic-sequence/SKILL.md b/skills/implement-epic-sequence/SKILL.md index 8aec80f..012e528 100644 --- a/skills/implement-epic-sequence/SKILL.md +++ b/skills/implement-epic-sequence/SKILL.md @@ -42,6 +42,12 @@ Before changing code, resolve: deletion, and deployment; - named architecture, design, contract, or rollout documents. +For a merge-inclusive run, verify before implementation that +`review-code-change` is available and readable. It is the single local +adversarial-review dependency. If it is unavailable, stop with an explicit +missing-dependency result; do not substitute a third-party skill, a generic +reviewer, or an unreviewed merge path. + Unless the user or repository contract states otherwise, apply this authority matrix: @@ -188,39 +194,27 @@ same files. Follow `references/review-and-merge-gates.md`. -- Implement directly in the primary execution context. -- After local validation, require one fresh, read-only adversarial - `code-review-pro` pass in a separate review-only subagent or equivalent - isolated context unless the user explicitly waives it or independent review - tooling is unavailable. -- Use a fresh or minimally inherited context. Give the reviewer only raw task - artifacts: repository instructions, the live ticket, named specifications, the - complete `base...HEAD` diff for the captured head/base SHA pair, and - validation evidence. Do not provide the implementation transcript, intended - answer, prior conclusions, or suspected findings. -- Before review, require every intended ticket change to be committed and the - implementation worktree to be clean. If unrelated user artifacts prevent a - clean state, classify and preserve them and prove they are irrelevant to the - candidate. -- Before delegation, capture HEAD, commit history, and tracked, untracked, and - ignored worktree state. After review, verify that all remain unchanged. Treat - any reviewer mutation as an integrity failure and inspect it without - discarding user work. -- Review correctness, acceptance criteria, regressions, failure paths, security, - authorization, architecture, public surface area, tests, documentation, and - scope. -- Apply only material, tractable, in-scope findings. -- Re-run affected and required validation after fixes. -- Run a fresh pass after material fixes and invalidate prior remote review - signals after every head or base change. -- Use at most three adversarial passes by default. If the final pass still has a - material finding, do not merge; report the unresolved finding and request - direction. - -Do not silently count an unavailable independent reviewer as a passed gate. -Record the limitation, perform a fresh adversarial self-review, and proceed only -when repository policy does not require independence and the user has accepted -or already authorized the unavailable-tool fallback. +- Implement and apply fixes only in the primary execution context. +- Commit and validate a clean candidate, then invoke `review-code-change` in a + fresh or minimally inherited read-only context with raw ticket, repository, + diff, validation, and worktree artifacts. +- Exclude the implementation transcript, intended solution, prior conclusions, + and suspected findings. +- Consume the suite's validated aggregate verdict, findings, dispositions, + limitations, and next action without reproducing its lens rubrics or ordering. +- Apply only blocking and strong-recommendation findings that are material, + tractable, and ticket-scoped. Preserve deferred findings without expanding the + PR. +- After fixes, run affected and required validation, commit, push, build a new + head-bound evidence packet, and invoke the suite according to its re-review + instructions. +- Use at most three full fix/re-review cycles by default. Keep the PR open and + report unresolved material findings after the final cycle. + +Treat a missing dependency, malformed result, `blocked` verdict, reviewer +mutation, or unavailable required evidence as a failed local review gate. The +review suite remains read-only; this workflow owns every code and GitHub +mutation. Create a follow-up ticket only when ticket management is authorized and the gap is real, evidenced, and intentionally outside the current PR. Otherwise report a @@ -235,18 +229,21 @@ Follow both bundled gate references. Merge only when: irrelevant; - required local validation passed; - required CI passed or the repository explicitly has no such checks; -- every applicable required adversarial, human, and connector review has a clean - or approving verdict explicitly tied to the exact current head and base SHA - pair; +- the repository-owned local review has a clean result bound to the current head + and an explicit disposition for any later base drift; +- every applicable required human and connector review has a current approving + or clean verdict under repository policy; - no undispositioned actionable conversation comment, formal review, connector feedback, or review thread remains; - the PR still matches its ticket and base; - no other PR superseded it. -Treat every head change, including a push, rebase, conflict resolution, or -update-branch operation, and every base-branch advance as invalidating older -merge-candidate evidence. Rebuild and revalidate the current head/base candidate -before merge. +Treat every head-changing edit, push, rebase, conflict resolution, or +update-branch operation as invalidating head-bound evidence. When only the base +advances, inspect the current merge candidate and record why each gate was +retained or invalidated. Retain evidence only when the effective diff and +resulting tree are unchanged, no conflict or relevant overlap exists, and +repository policy permits retention. Otherwise rerun every affected gate. After merge, verify the remote state and base-branch result before cleanup. diff --git a/skills/implement-epic-sequence/agents/openai.yaml b/skills/implement-epic-sequence/agents/openai.yaml index 480b794..d6b41e4 100644 --- a/skills/implement-epic-sequence/agents/openai.yaml +++ b/skills/implement-epic-sequence/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Implement Epic Sequence" - short_description: "Execute epics safely from ticket to merge" - default_prompt: "Use $implement-epic-sequence to implement the remaining ready children of this epic through review, merge, cleanup, and closeout." + short_description: "Execute epics through safe reviewed merges" + default_prompt: "Use $implement-epic-sequence to implement the remaining ready children of this epic through repository-owned review, merge, cleanup, and closeout." diff --git a/skills/implement-epic-sequence/evals/cases.json b/skills/implement-epic-sequence/evals/cases.json new file mode 100644 index 0000000..a3433a7 --- /dev/null +++ b/skills/implement-epic-sequence/evals/cases.json @@ -0,0 +1,150 @@ +[ + { + "id": "correctness-fix-and-rereview", + "request": "Implement and merge the ready child after all required gates pass.", + "ticket": { + "goal": "Stop retry scheduling at the configured maximum.", + "acceptance_criteria": ["No retry is scheduled when retry_count equals max_retries."], + "non_goals": ["Redesign the queue."], + "preserved_behaviors": ["Retries below the maximum still schedule normally."] + }, + "candidate": { + "head_sha": "1111111111111111111111111111111111111111", + "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "diff_summary": "The retry branch still schedules when retry_count equals max_retries.", + "validation": ["focused passed", "full passed"] + }, + "review_dependency_available": true, + "suite_result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "1111111111111111111111111111111111111111", "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "verdict": "changes_required", + "findings": [{ + "id": "correctness.retry-bound", "lens": "correctness", "severity": "blocking", "confidence": "high", + "rule": "Retries must stop at max_retries.", + "evidence": [{"location": "worker retry branch", "detail": "Equality still reaches schedule_retry."}], + "concern": "The maximum does not stop retry scheduling.", "impact": "Work can exceed the configured retry budget.", + "proposed_change": "Return before scheduling when retry_count is at least max_retries.", "expected_effect": "Bound retries at the configured maximum." + }], + "blocking_reasons": [], + "next_action": "Apply the correctness fix, validate, commit and push a new head, then rerun correctness and affected downstream lenses." + }, + "followup_suite_result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "1212121212121212121212121212121212121212", "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "verdict": "clean", "findings": [], "blocking_reasons": [], + "next_action": "No further local review action is required." + }, + "remote_policy": {"ci": "passed", "human": "not_required", "connector": "not_configured"} + }, + { + "id": "solution-redesign-early-exit", + "request": "Implement and merge the ready child after all required gates pass.", + "ticket": { + "goal": "Persist one local audit record.", + "acceptance_criteria": ["One local record is written."], + "non_goals": ["Support remote storage providers."], + "preserved_behaviors": ["Existing local writes remain atomic."] + }, + "candidate": { + "head_sha": "2222222222222222222222222222222222222222", + "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "diff_summary": "Adds a provider registry, dispatch layer, and remote-provider configuration for one local write.", + "validation": ["focused passed", "full passed"] + }, + "review_dependency_available": true, + "suite_result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "2222222222222222222222222222222222222222", "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}, + "verdict": "changes_required", + "findings": [{ + "id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", + "rule": "Remote providers are outside scope.", + "evidence": [{"location": "candidate diff", "detail": "Provider configuration and dispatch exist for one local write."}], + "concern": "The implementation strategy exceeds the ticket.", "impact": "It adds unsupported ownership and failure modes.", + "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove provider configuration, dispatch, and remote failure modes." + }], + "blocking_reasons": [], + "next_action": "Redesign the implementation, capture a new head, and restart the full review sequence." + }, + "remote_policy": {"ci": "required", "human": "not_required", "connector": "not_configured"} + }, + { + "id": "code-simplicity-targeted-correctness", + "request": "Implement and merge the ready child after all required gates pass.", + "ticket": { + "goal": "Authorize two new admin actions.", + "acceptance_criteria": ["Only active admins may invoke either action."], + "non_goals": ["Create another authorization framework."], + "preserved_behaviors": ["The shared active-admin policy remains authoritative."] + }, + "candidate": { + "head_sha": "3333333333333333333333333333333333333333", + "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc", + "diff_summary": "Both actions repeat the existing active-admin policy instead of calling its helper.", + "validation": ["focused passed", "full passed"] + }, + "review_dependency_available": true, + "suite_result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "3333333333333333333333333333333333333333", "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc"}, + "verdict": "changes_required", + "findings": [{ + "id": "code-simplicity.reuse-admin-policy", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", + "rule": "Reuse the repository authorization helper.", + "evidence": [{"location": "two new actions", "detail": "Both repeat require_active_admin logic."}], + "concern": "Authorization policy has three owners.", "impact": "The repeated checks can drift.", + "proposed_change": "Call require_active_admin from each action.", "expected_effect": "Restore one authorization-policy owner." + }], + "blocking_reasons": [], + "next_action": "Apply the code-simplicity fix on a new head, rerun code simplicity, then run targeted correctness." + }, + "remote_policy": {"ci": "required", "human": "not_required", "connector": "not_configured"} + }, + { + "id": "unrelated-base-drift-retained", + "request": "Merge the clean reviewed child when all current gates pass.", + "ticket": {"goal": "Add a parser boundary test.", "acceptance_criteria": ["The boundary is covered."], "non_goals": [], "preserved_behaviors": ["Parser behavior is unchanged."]}, + "candidate": { + "head_sha": "4444444444444444444444444444444444444444", + "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd", + "diff_summary": "Adds one parser test.", "validation": ["focused passed", "full passed"] + }, + "review_dependency_available": true, + "suite_result": {"schema_version": "1.0", "lens": "aggregate", "candidate": {"head_sha": "4444444444444444444444444444444444444444", "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "base_drift": {"new_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", "effective_diff_changed": false, "resulting_tree_changed": false, "conflict": false, "relevant_overlap": false, "repository_requires_reset": false}, + "remote_policy": {"ci": "passed", "human": "not_required", "connector": "not_configured"} + }, + { + "id": "relevant-base-drift-restarts-gates", + "request": "Merge the clean reviewed child when all current gates pass.", + "ticket": {"goal": "Change retry classification.", "acceptance_criteria": ["Permanent errors do not retry."], "non_goals": [], "preserved_behaviors": ["Transient errors retry."]}, + "candidate": { + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff", + "diff_summary": "Changes the retry classifier.", "validation": ["focused passed", "full passed"] + }, + "review_dependency_available": true, + "suite_result": {"schema_version": "1.0", "lens": "aggregate", "candidate": {"head_sha": "5555555555555555555555555555555555555555", "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "base_drift": {"new_base_sha": "abababababababababababababababababababab", "effective_diff_changed": true, "resulting_tree_changed": true, "conflict": true, "relevant_overlap": true, "repository_requires_reset": false}, + "remote_policy": {"ci": "required", "human": "required", "connector": "required"} + }, + { + "id": "missing-review-dependency", + "request": "Implement and merge the ready child after all required gates pass.", + "ticket": {"goal": "Add one command.", "acceptance_criteria": ["The command runs."], "non_goals": [], "preserved_behaviors": []}, + "candidate": {"head_sha": "6666666666666666666666666666666666666666", "comparison_base_sha": "1212121212121212121212121212121212121212", "diff_summary": "Adds one command.", "validation": ["focused passed", "full passed"]}, + "review_dependency_available": false, + "suite_result": {"schema_version": "1.0", "lens": "aggregate", "candidate": {"head_sha": "6666666666666666666666666666666666666666", "comparison_base_sha": "1212121212121212121212121212121212121212"}, "verdict": "blocked", "findings": [], "blocking_reasons": ["Required review skill review-code-change is unavailable."], "next_action": "Make review-code-change available before continuing the merge-inclusive run."}, + "remote_policy": {"ci": "required", "human": "not_required", "connector": "not_configured"} + }, + { + "id": "clean-local-review-remote-policy-pending", + "request": "Merge the clean reviewed child when all current gates pass.", + "ticket": {"goal": "Expose one read-only status field.", "acceptance_criteria": ["The field is returned."], "non_goals": [], "preserved_behaviors": ["Existing fields are unchanged."]}, + "candidate": {"head_sha": "7777777777777777777777777777777777777777", "comparison_base_sha": "3434343434343434343434343434343434343434", "diff_summary": "Adds one read-only field and test.", "validation": ["focused passed", "full passed"]}, + "review_dependency_available": true, + "suite_result": {"schema_version": "1.0", "lens": "aggregate", "candidate": {"head_sha": "7777777777777777777777777777777777777777", "comparison_base_sha": "3434343434343434343434343434343434343434"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "remote_policy": {"ci": "passed", "human": "required_pending", "connector": "required_pending", "unresolved_threads": 0} + } +] diff --git a/skills/implement-epic-sequence/evals/results.json b/skills/implement-epic-sequence/evals/results.json new file mode 100644 index 0000000..466f339 --- /dev/null +++ b/skills/implement-epic-sequence/evals/results.json @@ -0,0 +1,44 @@ +[ + { + "case_id": "correctness-fix-and-rereview", + "workflow_state": "ready_for_remote_gates", + "observed_actions": ["apply ticket-scoped correctness fix", "run affected and full validation", "commit and push new head", "build new evidence packet", "follow suite re-review instructions", "accept clean new-head aggregate"], + "review_cycles": 2 + }, + { + "case_id": "solution-redesign-early-exit", + "workflow_state": "redesign_required", + "observed_actions": ["stop current candidate review", "redesign in primary context", "validate and commit new head", "restart full review sequence"], + "review_cycles": 1 + }, + { + "case_id": "code-simplicity-targeted-correctness", + "workflow_state": "fix_and_targeted_rereview_required", + "observed_actions": ["apply ticket-scoped code simplification", "run affected and full validation", "commit and push new head", "build new evidence packet", "rerun code simplicity", "run targeted correctness"], + "review_cycles": 1 + }, + { + "case_id": "unrelated-base-drift-retained", + "workflow_state": "merged", + "observed_actions": ["accept clean local review for current head", "inspect effective candidate after base advance", "record unrelated base-drift retention", "retain head-bound validation and review evidence", "confirm remote gates passed", "merge candidate", "verify merge and mainline result before cleanup"], + "review_cycles": 1 + }, + { + "case_id": "relevant-base-drift-restarts-gates", + "workflow_state": "affected_gates_invalidated", + "observed_actions": ["inspect current merge candidate", "record relevant overlap and conflict", "resolve conflict on a new head", "rerun local validation", "rerun repository-owned review", "restart required CI human connector and thread gates"], + "review_cycles": 1 + }, + { + "case_id": "missing-review-dependency", + "workflow_state": "blocked", + "observed_actions": ["return explicit missing-dependency result", "do not substitute another reviewer", "do not merge"], + "review_cycles": 0 + }, + { + "case_id": "clean-local-review-remote-policy-pending", + "workflow_state": "waiting_for_remote_review", + "observed_actions": ["accept clean local aggregate", "keep human review gate pending", "keep connector review gate pending", "do not infer approval from CI or zero threads", "do not merge"], + "review_cycles": 1 + } +] diff --git a/skills/implement-epic-sequence/references/github.md b/skills/implement-epic-sequence/references/github.md index 82a4686..2644da1 100644 --- a/skills/implement-epic-sequence/references/github.md +++ b/skills/implement-epic-sequence/references/github.md @@ -89,12 +89,12 @@ or an equivalent thread-aware API when flat PR output does not expose thread resolution. For required human review, record the reviewer, review state, reviewed commit -OID, submission time, and base SHA observed when the review arrives. Accept the -approval for the captured candidate only when its reviewed commit and observed -base equal the captured head/base pair. After a base advance, request fresh -approval after capturing the new pair. When the host cannot bind fresh approval -to an unchanged head plus a new base, update the branch so the candidate has a -new reviewable head and require approval on it. +OID, submission time, and base SHA observed when the review arrives. Require its +reviewed commit to equal the current head. After a base advance, apply the +generic base-drift gate: retain approval only when the effective candidate is +unchanged and repository policy permits it; otherwise request fresh approval. +When the host cannot bind a required fresh approval to an unchanged head plus a +new base, update the branch so the candidate has a new reviewable head. When the repository uses a connector bot, use the identity, initiation mode, and clean signal recorded during PR-host preflight. Accept a clean result only when @@ -109,7 +109,8 @@ all of these conditions hold: - a configured PR-object thumbs-up from the connector bot that appeared as the completion signal for the connector run on the captured SHA; - the PR head still equals the captured SHA; and -- the current base-branch head still equals the captured base SHA; and +- any base advance has passed the generic base-drift gate and the connector + policy permits retaining its current-head signal; and - there are zero unresolved connector-authored review threads. For a PR-object thumbs-up, require repository evidence that this is the @@ -136,12 +137,14 @@ For every actionable conversation comment, formal review, and inline thread: Require zero undispositioned actionable items across conversation comments, formal reviews, connector feedback, and inline threads before merge. -After any head or base change, capture the new head/base pair and wait for a -fresh connector verdict tied to that candidate. Trigger the repository's -configured connector when review is request-driven. When review is automatic, -record evidence that a run began for the captured candidate after it was pushed -or rebuilt. Do not accept a verdict until its review request or automatic run is -tied to that candidate. +After a head change, capture the new head/base pair and wait for a fresh +connector verdict tied to that candidate. After base-only drift, apply the +generic drift gate and request a fresh connector verdict only when the effective +candidate changed, relevant overlap exists, or connector/repository policy +requires it. Trigger the configured connector when review is request-driven. +When review is automatic, record evidence that a run began for the captured +candidate after it was pushed or rebuilt. Do not accept a required fresh verdict +until its review request or automatic run is tied to that candidate. Resolve the connector polling window and interval from repository instructions. When none is configured, state and use a 30-minute window with a 30-to-60-second @@ -177,12 +180,13 @@ incorrect, outside scope, polish, or hypothetical hardening. - Report unavailable external checks without weakening or bypassing the gate. - If the repository has no branch checks, state that explicitly and use the documented local and review gates. -- Immediately before merge, re-read the base SHA. If it differs from the base - used for validation or review, build an up-to-date merge candidate and rerun - applicable local validation, CI, adversarial review, human review, connector - review, and feedback disposition. When a review system cannot bind evidence to - an unchanged head plus a new base, update the branch so the new candidate has - a reviewable head SHA. +- Immediately before merge, re-read the base SHA. If it differs from the + comparison base, build or inspect an up-to-date merge candidate and apply the + generic base-drift gate. Record why each signal is retained or rerun every + affected local validation, CI, local review, human review, connector review, + and feedback disposition. When a review system cannot bind required fresh + evidence to an unchanged head plus a new base, update the branch so the new + candidate has a reviewable head SHA. - Use the repository's approved merge method. - If local worktree ownership prevents the CLI from switching to the base branch, merge through GitHub's API and perform local cleanup separately. diff --git a/skills/implement-epic-sequence/references/review-and-merge-gates.md b/skills/implement-epic-sequence/references/review-and-merge-gates.md index 570e9df..769c4a4 100644 --- a/skills/implement-epic-sequence/references/review-and-merge-gates.md +++ b/skills/implement-epic-sequence/references/review-and-merge-gates.md @@ -13,13 +13,15 @@ stricter requirements but must not silently weaken them. ## Bounded review loop -After implementation and required local validation, require one fresh, read-only -adversarial review using `code-review-pro` in a separate review-only subagent or -equivalent isolated context unless the user explicitly waives independent review -or independent review tooling is unavailable. Use a fresh or minimally inherited -context containing only raw task artifacts. Exclude the implementation -transcript, intended answer, prior conclusions, and suspected findings. Do not -silently treat unavailability as a passed independent-review gate. +Before a merge-inclusive run, require the repository-owned `review-code-change` +skill. Fail closed with an explicit missing-dependency result when it is not +available or readable. Do not substitute another review skill, generic +self-review, or an unreviewed merge path. + +After implementation and required local validation, invoke the suite in a fresh +or minimally inherited read-only context containing only raw task artifacts. +Exclude the implementation transcript, intended solution, prior conclusions, and +suspected findings. Before review, require every intended ticket change to be committed and the implementation worktree to be clean. If unrelated user artifacts prevent a clean @@ -31,42 +33,28 @@ ignored worktree state. After the reviewer returns, verify that all remain unchanged. Treat any mutation as an integrity failure, inspect it, and preserve user work rather than resetting or deleting it. -Give the reviewer: +Give the suite: - the live ticket and acceptance criteria; - every named architecture, design, contract, migration, and rollout document; -- the exact captured head and base SHAs and their complete `base...HEAD` diff, - not only the latest commit; and +- the exact captured head and comparison-base SHAs and their complete + `base...HEAD` diff, not only the latest commit; and - exact focused and full validation evidence, including unavailable checks. -Require the reviewer to inspect correctness, acceptance coverage, regressions, -failure paths, security, authorization boundaries, architecture, public surface -area, tests, documentation, and scope. Classify each finding as: - -- must fix now: correctness, security, acceptance, architecture, or validation - failure; -- obvious low-cost correctness or safety improvement: addresses demonstrated - current risk without widening scope; -- already satisfied or incorrect; -- valid but outside the ticket; -- blocked by a missing product decision. - -Apply only the first two categories. Reply with evidence for rejected findings. -When authorized, create a focused follow-up only for a real, evidenced gap. -Apply cognitive-load refactoring only when the user explicitly requests it or -when it is necessary to make the ticket's correctness evident. Do not use -reviewer convenience alone to expand the active PR. - -Run a fresh pass after material fixes so the review covers the resulting diff. -Use at most three adversarial passes by default. A clean pass ends the loop. If -the third pass still reports a material finding, do not merge; report the -remaining issue and request direction. Do not spend passes on style, polish, -hypothetical hardening, or future compatibility. - -When independent review tooling is unavailable, record why, perform a fresh -adversarial self-review using the same inputs and coverage, and proceed only if -repository policy does not require independence and the user has accepted or -already authorized this fallback. +Validate and consume the aggregate result according to the suite's shared +contract. Do not restate or override lens ordering, severity semantics, +deduplication, or correctness-versus-simplicity rules here. Apply only blocking +and strong-recommendation findings that are material, tractable, and +ticket-scoped. Preserve deferred findings without expanding the current PR. +Reply with evidence for findings that no longer apply. When authorized, create a +focused follow-up only for a real, evidenced gap outside active scope. + +After a material fix, run affected and required validation, commit and push the +new head, rebuild the packet, and invoke the suite according to its returned +re-review instructions. Use at most three full fix/re-review cycles by default. +A clean aggregate ends the local loop. If the final cycle still reports a +material finding, keep the PR open and report the unresolved evidence. Do not +spend cycles on deferred findings. ## Revalidation @@ -77,27 +65,27 @@ After every fix cycle: - commit every intended ticket change and confirm that no uncommitted ticket work is absent from `base...HEAD`; - push the updated head; -- capture the current head and base SHAs; +- capture the current head and comparison-base SHAs; - re-read current-candidate review and check state. -Do not carry an older clean review signal across any head change, including a -push, rebase, conflict resolution, or update-branch operation, or across a base -advance that changes the merge candidate. +Do not carry older head-bound evidence across an edit, push, rebase, conflict +resolution, or update-branch operation that changes the head. A base advance +uses the separate drift gate below. ## Base-drift gate -Bind validation, CI, adversarial review, required human approval, connector -review, and feedback disposition evidence to the captured head and base SHA -pair. Immediately before merge, re-read both SHAs. +Bind local review evidence to the captured head. Immediately before merge, +re-read the head and base and inspect the effective merge candidate when the +base advanced. -When the base changed, build an up-to-date merge candidate and rerun applicable -local validation, CI, adversarial review, human review, connector review, and -feedback disposition. For required human approval, require proof that it was -submitted for the captured pair or request fresh approval after capturing the -new pair. When rebasing, merging, conflict resolution, or update-branch changes -the head, restart every current-head gate. When a review system cannot bind a -fresh verdict to an unchanged head plus a new base, update the branch so the -candidate receives a new reviewable head SHA. +Retain head-bound evidence across base-only drift only when the effective diff +and resulting tree are unchanged, no conflict exists, no relevant base code +overlaps the candidate, repository policy permits retention, and the reason is +recorded. Otherwise invalidate and rerun each affected local-validation, CI, +local-review, human-review, connector-review, and feedback-disposition gate. +Repository policy may require a complete reset even for unrelated drift. Any +rebase, merge, conflict resolution, or update that changes the head restarts +every head-bound gate. ## Merge gate @@ -110,9 +98,10 @@ Require all applicable conditions: - required remote checks passed; - no undispositioned actionable conversation comment, formal review, connector feedback, or review thread remains; -- every applicable required adversarial, human, and connector review has a clean - or approving verdict explicitly tied to the exact current head and base SHA - pair; +- the repository-owned local review has a clean aggregate bound to the current + head, with any later base drift explicitly retained or re-reviewed; +- every applicable required human and connector review is current under + repository policy; - no merge conflict or superseding PR exists; - the diff still satisfies one ticket and its non-goals; - rollout or migration prerequisites required before merge are complete. diff --git a/skills/implement-epic-sequence/scripts/tests/test_review_integration_contract.py b/skills/implement-epic-sequence/scripts/tests/test_review_integration_contract.py new file mode 100644 index 0000000..ea0f56b --- /dev/null +++ b/skills/implement-epic-sequence/scripts/tests/test_review_integration_contract.py @@ -0,0 +1,120 @@ +from __future__ import annotations + +import importlib.util +import json +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] +REPOSITORY_ROOT = SKILL_ROOT.parents[1] +REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +SPEC = importlib.util.spec_from_file_location( + "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" +) +assert SPEC and SPEC.loader +VALIDATOR = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(VALIDATOR) + + +def load(path: Path): + return json.loads(path.read_text()) + + +class ReviewIntegrationContractTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.skill = (SKILL_ROOT / "SKILL.md").read_text() + cls.gates = ( + SKILL_ROOT / "references" / "review-and-merge-gates.md" + ).read_text() + cls.github = (SKILL_ROOT / "references" / "github.md").read_text() + cls.cases = { + case["id"]: case for case in load(SKILL_ROOT / "evals" / "cases.json") + } + cls.results = { + item["case_id"]: item + for item in load(SKILL_ROOT / "evals" / "results.json") + } + + def test_review_suite_is_the_only_local_review_dependency(self): + combined = self.skill + self.gates + self.assertNotIn("code-review-pro", combined) + self.assertIn("single local\nadversarial-review dependency", self.skill) + self.assertIn("`review-code-change`", self.skill) + self.assertIn("explicit\nmissing-dependency result", self.skill) + self.assertIn("Do not substitute another review skill", self.gates) + + def test_epic_skill_owns_mechanics_not_lens_rubrics(self): + self.assertIn("primary execution context", self.skill) + self.assertIn("commit, push, build a new", self.skill) + self.assertIn("without reproducing its lens rubrics or ordering", self.skill) + self.assertNotIn("Review correctness, acceptance criteria", self.skill) + self.assertIn("thread remains", self.skill) + self.assertIn("current-head signal", self.github) + + def test_review_context_is_raw_fresh_and_read_only(self): + combined = self.skill + self.gates + self.assertIn("fresh or minimally inherited read-only context", combined) + self.assertIn("raw ticket, repository", combined) + self.assertIn("Exclude the implementation transcript", combined) + self.assertIn("worktree state", self.gates) + + def test_fix_scope_cycles_and_deferred_findings_are_bounded(self): + self.assertIn("blocking and strong-recommendation findings", self.skill) + self.assertIn("Preserve deferred findings", self.skill) + self.assertIn("at most three full fix/re-review cycles", self.skill) + self.assertIn("Keep the PR open", self.skill) + + def test_base_drift_is_risk_based_without_weakening_remote_gates(self): + combined = self.skill + self.gates + self.github + self.assertIn("effective diff", combined) + self.assertIn("resulting tree are unchanged", combined) + self.assertIn("no conflict or relevant overlap", combined) + self.assertIn("repository policy permits retention", combined) + self.assertIn("rerun every affected gate", combined) + self.assertIn("required human and connector review", self.skill) + self.assertIn("zero unresolved connector-authored review threads", self.github) + self.assertIn("CI success alone is not a clean review", self.gates) + + def test_forward_cases_cover_ticket_scenarios_and_suite_results_conform(self): + required = { + "correctness-fix-and-rereview", + "solution-redesign-early-exit", + "code-simplicity-targeted-correctness", + "unrelated-base-drift-retained", + "relevant-base-drift-restarts-gates", + "missing-review-dependency", + "clean-local-review-remote-policy-pending", + } + self.assertEqual(required, set(self.cases)) + self.assertEqual(required, set(self.results)) + for case_id, case in self.cases.items(): + with self.subTest(case=case_id): + self.assertEqual([], VALIDATOR.validate_result(case["suite_result"])) + if followup := case.get("followup_suite_result"): + self.assertEqual([], VALIDATOR.validate_result(followup)) + + def test_forward_results_preserve_required_boundaries(self): + self.assertEqual( + "blocked", self.results["missing-review-dependency"]["workflow_state"] + ) + self.assertEqual( + "merged", + self.results["unrelated-base-drift-retained"]["workflow_state"], + ) + self.assertIn( + "retain head-bound validation and review evidence", + self.results["unrelated-base-drift-retained"]["observed_actions"], + ) + self.assertEqual( + "affected_gates_invalidated", + self.results["relevant-base-drift-restarts-gates"]["workflow_state"], + ) + remote = self.results["clean-local-review-remote-policy-pending"] + self.assertIn("do not merge", remote["observed_actions"]) + targeted = self.results["code-simplicity-targeted-correctness"] + self.assertIn("run targeted correctness", targeted["observed_actions"]) + + +if __name__ == "__main__": + unittest.main()