diff --git a/AGENTS.md b/AGENTS.md index 36e9089..8d4f74a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,7 +58,7 @@ In an initial review, report substantiated blockers together. A follow-up review The review-round budget below applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume the Codex budget nor require repository-owner authorization under it. -Automatic Codex review is the initial Codex review. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](Guidelines/GitHub/PullRequests.md). +When repository-specific evidence establishes that automatic Codex review is enabled and applies to the current pull request/head, it is the initial Codex review. Otherwise, an absent review does not establish a pending gate. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](Guidelines/GitHub/PullRequests.md). ## Validation diff --git a/CHANGELOG.md b/CHANGELOG.md index d82dd17..5797bb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to this project are documented in this file. +## [0.0.35] - 2026-09-27 + +### Changed + +- Require positive repository-specific evidence before automatic Codex review becomes a merge gate. Track configuration separately from current-head execution, default undiscoverable settings to `unknown`, and bound deduplicated start-signal monitoring to five minutes. + ## [0.0.34] - 2026-09-20 ### Changed diff --git a/Guidelines/GitHub/PullRequests.md b/Guidelines/GitHub/PullRequests.md index 48b85df..e7456de 100644 --- a/Guidelines/GitHub/PullRequests.md +++ b/Guidelines/GitHub/PullRequests.md @@ -11,7 +11,7 @@ Use this guide whenever creating, reviewing, updating, or merging a GitHub pull - Follow the repository's pull-request template and local contribution instructions. - Run the relevant local validation and document anything that could not be run. - Open the pull request without auto-merge and keep it unmerged while automated or agent review is pending. Use draft state only when configured reviewers also run on drafts. -- When automatic Codex review is enabled, opening the pull request schedules the review. Do not also post `@codex review` or make another manual request; duplicate reviews waste review capacity and tokens. Do not request a Codex review manually unless the user explicitly asks for one. +- When repository-specific evidence establishes that automatic Codex review is enabled and the current pull request/head meets its trigger, track that review round. Do not also post `@codex review` or make another manual request; duplicate reviews waste review capacity and tokens. Do not request a Codex review manually unless the user explicitly asks for one. ## Consumer subtree review scope @@ -34,7 +34,7 @@ Only unresolved P0 and P1 findings block merge. A finding may be technically cor Opening a pull request starts review; it does not authorize merging it. -1. Wait for the configured Codex review to finish. No review yet means pending, not approved. +1. Establish the Codex review state from the evidence rules below. An absent review is pending only for a positively established current-head review round. Unknown or disabled configuration and no Codex activity do not block merge by themselves. 2. Record the reviewed head SHA and inspect all review summaries, inline threads, checks, and requested changes. 3. Assess each comment for technical correctness, severity, supported reachability, and root cause. 4. Give every thread one explicit disposition: `BLOCKER-P0`, `BLOCKER-P1`, `DEFER-P2`, `DEFER-P3`, `DECLINE`, or `DUPLICATE`. @@ -50,6 +50,40 @@ A thumbs-up or clean Codex review satisfies the agent-review step, but it does n ### Codex review state and round budget +#### Codex review evidence and state + +Track configuration and execution separately in the active task; these are not checked-in runtime files: + +```text +codex_review_configuration = enabled | disabled | unknown +codex_review_configuration_evidence = | unset +codex_review_execution = not_started | scheduled | processing | completed +codex_review_execution_evidence = | unset +codex_review_expected_sha = +codex_review_completed_sha = | unset +``` + +Start at `unknown`, `not_started`, the current head SHA, and an unset completed SHA. The executing agent is not assumed to have access to OpenAI's automatic-review configuration. Only positive repository-specific evidence may change configuration to `enabled` or `disabled`: a durable declaration in the consumer's root `AGENTS.md` or other tracked repository policy, explicit repository-owner confirmation in the active task, an authoritative repository or organization setting observed through an available interface, or another source whose semantics explicitly establish this repository's configuration. A repository may declare that automatic Codex pull-request review is enabled or disabled; no declaration is required. Generic AgentGuidelines or template wording, an automation prompt, another repository's setting, and a previous pull request's behavior are insufficient. + +Execution evidence must identify the active pull request and review round: a current-PR Codex processing reaction or equivalent event, a current review request/event with unambiguous round semantics, a submitted review covering the expected head, or another authoritative GitHub/OpenAI signal identifying the current PR/head. Absence of a review or reaction and elapsed time are not evidence. In particular, `unknown` plus no signal remains `not_started`, never `scheduled` or pending. + +Discovering `disabled` leaves execution `not_started` when no current activity exists. Discovering `enabled` alone does not schedule a review. Set `scheduled` only when positive evidence also establishes that the current PR/head meets the automatic trigger; start the bounded signal monitor then. A current-PR/current-head processing signal advances to `processing`. A completed review may advance `scheduled` or `processing` to `completed`; record its commit SHA when available. The gate is satisfied only when the reviewed SHA equals the expected SHA or another authoritative signal proves coverage of that head. A review for an earlier head is stale. + +When the head changes, set `codex_review_expected_sha` to the new head and clear `codex_review_completed_sha`. Terminate monitoring of the old head. Re-establish execution evidence for the new head; do not transfer `scheduled`, `processing`, or `completed`, or infer a new automatic round from the old one. + +Apply these outcomes to the current head: + +| Evidence | Execution | Merge gate | Start monitor | +| --- | --- | --- | --- | +| Enabled with a current-head processing signal | `processing` | Pending | No; processing already began | +| Enabled with a completed review covering the expected head and dispositioned findings | `completed` | Satisfied | No | +| Unknown with no signal or configuration evidence | `not_started` | Not blocked by absent Codex review | No | +| Disabled with no review | `not_started` | Not blocked by absent Codex review | No | +| Review completed for an old head | Re-establish for new head | Old review does not satisfy the gate | No inherited monitor | +| Enabled and positively eligible, with no start signal | `scheduled` | Pending while required | Yes, at most five minutes | + +For repeated identical snapshots, retain the fingerprint and poll count without re-analysis or notification. A signal inside the budget advances to `processing` while preserving configuration evidence and expected SHA. If the head changes during monitoring, terminate the old monitor and re-establish the new head's review state. If no signal appears by the deadline, terminate monitoring and report the unresolved verified state once; timeout never satisfies the gate. + This round budget applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume this Codex budget nor require repository-owner authorization under it. Do not block an agentic goal waiting for a Codex-budget exception before issuing an otherwise-authorized Reasoning Relay request. Track enough Codex-review state to prevent duplicate requests and unbounded Codex review loops: @@ -82,6 +116,14 @@ Stop the review loop when no unresolved P0/P1 finding remains, every thread has ### Codex review monitoring +Monitor an automatic review for at most five minutes total from entry into `scheduled`, including time spent in `processing`. Take an initial snapshot, then at most one each around 30, 90, 180, and 300 seconds; equivalent non-accelerating schedules are allowed if they stop by five minutes. Do not create an indefinite recurring automation. A processing signal changes the execution state but does not reset the deadline or poll budget. A completion signal ends monitoring. Review completion is not assumed to occur within five minutes. + +A temporary monitor owns `started_at`, `deadline`, `poll_count`, `last_state_fingerprint`, and `last_observed_state`; discard them when it ends. Fingerprint at least repository, PR number, base SHA, head SHA, configuration, execution, signal state, review commit SHA, review-thread state, required checks, and mergeability. An identical fingerprint causes no substantive re-analysis or user notification. Continue only within the deadline. + +At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval. Keep `scheduled` only if positive current-head scheduling evidence remains; retain `processing` only while its current-head signal remains valid. Otherwise use the strongest evidence-supported state. Do not continue polling or block unrelated work. If a positively established review remains a required merge gate, surface that unresolved gate to the user. + +Any generated monitor prompt must preserve repository, PR number, expected head SHA, configuration and its authoritative evidence, current execution state, monitor start, and deadline. Without configuration evidence, do not create a monitor for an absent review. A later heartbeat must not reconstruct `enabled` from its own prompt. + Use GitHub review data, reactions, and checks together. An eyes reaction means Codex is processing the pull request; it is not an approval. A thumbs-up means the review completed without suggestions. A submitted review means its inline threads must be assessed individually. ```text @@ -151,7 +193,7 @@ gh api graphql --paginate \ -F thread= ``` -Continue polling only while an allowed review round is pending. Inspect every returned page for reactions, review threads, and thread comments. Do not treat missing comments, a pending reaction, truncated results, or elapsed time as review completion, and do not submit a duplicate request merely because polling has not completed. +Poll for automatic start or completion only under the five-minute ceiling above. After a verified processing signal, inspect every returned page for reactions, review threads, and thread comments when checking completion. Do not treat missing comments, a pending reaction, truncated results, or elapsed time as review completion, and do not submit a duplicate request merely because polling has not completed. ## Merge method @@ -161,7 +203,7 @@ ThatFactory repositories use squash merges by default. Do not attempt a merge co Do not merge while any of the following is true: -- Codex review is still pending; +- a positively established current-head Codex review round is `scheduled` or `processing` and remains a required gate; - an unresolved P0/P1 finding remains; - a review thread lacks an explicit disposition or remains unresolved; - a required check is pending or failing; diff --git a/README.md b/README.md index caab867..7b82435 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ From the consumer repository root, install a tagged release: git subtree add \ --prefix=AgentGuidelines \ https://github.com/thatfactory/agent-guidelines.git \ - 0.0.34 \ + 0.0.35 \ --squash ``` @@ -147,7 +147,7 @@ Review the target release's changelog, then pull it deliberately: git subtree pull \ --prefix=AgentGuidelines \ https://github.com/thatfactory/agent-guidelines.git \ - 0.0.34 \ + 0.0.35 \ --squash ``` diff --git a/Scripts/validate_guidelines.swift b/Scripts/validate_guidelines.swift index 7c2dddf..abd2c4b 100755 --- a/Scripts/validate_guidelines.swift +++ b/Scripts/validate_guidelines.swift @@ -34,6 +34,7 @@ let cicdGuideline = root.appendingPathComponent("Guidelines/CICD.md") let documentationGuideline = root.appendingPathComponent("Guidelines/Documentation.md") let packagesGuideline = root.appendingPathComponent("Guidelines/Packages.md") let agentsTemplate = root.appendingPathComponent("Templates/AGENTS.md") +let pullRequestsGuideline = root.appendingPathComponent("Guidelines/GitHub/PullRequests.md") let gitignoreTemplate = root.appendingPathComponent("Templates/.gitignore") let gitignoreGuideline = root.appendingPathComponent("Guidelines/Git/IgnoreFiles.md") @@ -772,6 +773,52 @@ func validateAuditSkill(_ errors: inout [String]) { } } +/// Validates positive evidence and bounded monitoring for automatic Codex reviews. +func validateCodexReviewEvidence(_ errors: inout [String]) { + guard let guide = readText(pullRequestsGuideline, errors: &errors) else { return } + let required = [ + "codex_review_configuration = enabled | disabled | unknown", + "codex_review_execution = not_started | scheduled | processing | completed", + "codex_review_expected_sha = ", + "Only positive repository-specific evidence", + "unknown` plus no signal remains `not_started`", + "Discovering `enabled` alone does not schedule a review", + "reviewed SHA equals the expected SHA", + "at most five minutes total from entry into `scheduled`, including time spent in `processing`", + "A processing signal changes the execution state but does not reset the deadline or poll budget", + "An identical fingerprint causes no substantive re-analysis or user notification", + "Without configuration evidence, do not create a monitor", + "do not transfer `scheduled`, `processing`, or `completed`", + "At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval", + "If the head changes during monitoring, terminate the old monitor", + ] + for concept in required where !guide.contains(concept) { + errors.append("Guidelines/GitHub/PullRequests.md: missing Codex review evidence rule: \(concept)") + } + for forbidden in ["No review yet means pending", "Wait for the configured Codex review to finish"] + where guide.contains(forbidden) { + errors.append("Guidelines/GitHub/PullRequests.md: unconditional Codex review gate: \(forbidden)") + } + let scenarioOutcomes = [ + "Enabled with a current-head processing signal | `processing` | Pending | No; processing already began", + "Enabled with a completed review covering the expected head and dispositioned findings | `completed` | Satisfied | No", + "Unknown with no signal or configuration evidence | `not_started` | Not blocked by absent Codex review | No", + "Disabled with no review | `not_started` | Not blocked by absent Codex review | No", + "Review completed for an old head | Re-establish for new head | Old review does not satisfy the gate | No inherited monitor", + "Enabled and positively eligible, with no start signal | `scheduled` | Pending while required | Yes, at most five minutes", + "A signal inside the budget advances to `processing` while preserving configuration evidence and expected SHA", + "If no signal appears by the deadline, terminate monitoring and report the unresolved verified state once; timeout never satisfies the gate", + ] + for outcome in scenarioOutcomes where !guide.contains(outcome) { + errors.append("Guidelines/GitHub/PullRequests.md: missing Codex review scenario outcome: \(outcome)") + } + if let template = readText(agentsTemplate, errors: &errors), + !template.contains("this template does not establish that automatic review is configured") + { + errors.append("Templates/AGENTS.md: missing unknown-configuration safeguard") + } +} + /// Validates the reusable ignore template and its shared reconciliation policy. func validateGitignoreGuidance(_ errors: inout [String]) { guard let template = readText(gitignoreTemplate, errors: &errors) else { return } @@ -822,6 +869,7 @@ func main() -> Int32 { validateXcodeProjectSettingsGuideline(&errors) validatePackageCompilerSettingsGuideline(&errors) validateExternalDependencyPolicy(&errors) + validateCodexReviewEvidence(&errors) validateGitignoreGuidance(&errors) validateExecutable(consumerSetupScript, description: "consumer setup validator", errors: &errors) validateAuditSkill(&errors) diff --git a/Templates/AGENTS.md b/Templates/AGENTS.md index 092c1a9..c57b8a9 100644 --- a/Templates/AGENTS.md +++ b/Templates/AGENTS.md @@ -28,6 +28,8 @@ Read only the guides relevant to the task: For an application that uses Redux, also read [Redux architecture](AgentGuidelines/Guidelines/Architecture/Redux.md). +For Codex pull-request review, use the linked review evidence rules. If authoritative repository-specific evidence establishes that automatic review is enabled, apply its current-head gate. If the executing agent cannot establish the setting, record it as `unknown`; this template does not establish that automatic review is configured. + Keep the following observability contract in the consumer repository's root `AGENTS.md` so implementation agents treat runtime diagnostics as part of lifecycle work. Copy it unchanged and update it when the marker version changes in this template. ```md @@ -90,7 +92,7 @@ In an initial review, report substantiated blockers together. A follow-up review The review-round budget below applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume the Codex budget nor require repository-owner authorization under it. -Automatic Codex review is the initial Codex review. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](AgentGuidelines/Guidelines/GitHub/PullRequests.md). +When repository-specific evidence establishes that automatic Codex review is enabled and applies to the current pull request/head, it is the initial Codex review. Otherwise, an absent review does not establish a pending gate. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](AgentGuidelines/Guidelines/GitHub/PullRequests.md). ## Codex review scope diff --git a/Tests/run_tests.swift b/Tests/run_tests.swift index 57fb159..6f4276a 100755 --- a/Tests/run_tests.swift +++ b/Tests/run_tests.swift @@ -120,6 +120,83 @@ func localization(_ value: String, state: String = "translated") -> String { } let tests: [(String, () throws -> Void)] = [ + ( + "review evidence validator rejects an unconditional pending gate", + { + try withTemporaryDirectory { temporary in + let fixture = temporary.appendingPathComponent("repository") + try copyRepositoryFixture(to: fixture) + let guide = fixture.appendingPathComponent("Guidelines/GitHub/PullRequests.md") + let contents = try String(contentsOf: guide, encoding: .utf8) + try write(contents + "\nNo review yet means pending.\n", to: guide) + let result = try run([fixture.appendingPathComponent("Scripts/validate_guidelines.swift").path]) + try require(!result.succeeded, "unconditional Codex gate unexpectedly passed") + try require(result.output.contains("unconditional Codex review gate"), result.output) + } + } + ), + ( + "review evidence validator requires unknown state", + { + try withTemporaryDirectory { temporary in + let fixture = temporary.appendingPathComponent("repository") + try copyRepositoryFixture(to: fixture) + let guide = fixture.appendingPathComponent("Guidelines/GitHub/PullRequests.md") + let contents = try String(contentsOf: guide, encoding: .utf8) + try write( + contents.replacingOccurrences(of: "enabled | disabled | unknown", with: "enabled | disabled"), + to: guide) + let result = try run([fixture.appendingPathComponent("Scripts/validate_guidelines.swift").path]) + try require(!result.succeeded, "missing unknown state unexpectedly passed") + try require(result.output.contains("missing Codex review evidence rule"), result.output) + } + } + ), + ( + "review evidence validator protects scenario outcomes and deadline", + { + let mutations = [ + ( + "Disabled with no review | `not_started` | Not blocked by absent Codex review | No", + "Disabled with no review | `not_started` | Pending | Yes" + ), + ( + "Enabled with a current-head processing signal | `processing` | Pending", + "Enabled with a current-head processing signal | `processing` | Satisfied" + ), + ( + "Enabled with a completed review covering the expected head and dispositioned findings | `completed` | Satisfied", + "Enabled with a completed review covering the expected head and dispositioned findings | `completed` | Pending" + ), + ( + "Review completed for an old head | Re-establish for new head", + "Review completed for an old head | Completed for new head" + ), + ( + "An identical fingerprint causes no substantive re-analysis or user notification", + "An identical fingerprint may trigger another notification" + ), + ("including time spent in `processing`", "excluding time spent in `processing`"), + ( + "At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval", + "At the deadline, keep waiting" + ), + ] + for (original, replacement) in mutations { + try withTemporaryDirectory { temporary in + let fixture = temporary.appendingPathComponent("repository") + try copyRepositoryFixture(to: fixture) + let guide = fixture.appendingPathComponent("Guidelines/GitHub/PullRequests.md") + let contents = try String(contentsOf: guide, encoding: .utf8) + try require(contents.contains(original), "missing test fixture text: \(original)") + try write(contents.replacingOccurrences(of: original, with: replacement), to: guide) + let result = try run([fixture.appendingPathComponent("Scripts/validate_guidelines.swift").path]) + try require(!result.succeeded, "review scenario mutation unexpectedly passed: \(original)") + try require(result.output.contains("Codex review"), result.output) + } + } + } + ), ( "repository validator accepts the source tree", { diff --git a/VERSION b/VERSION index bb951c8..155069a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.0.34 +0.0.35