Require durable review readiness before merge#27
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741e007756
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }; | ||
| const COMMAND_HANDLERS = { | ||
| sync: syncReview, | ||
| gate: gateReview, |
There was a problem hiding this comment.
When the new gate handler is selected, it calls manager.sync(), but githubClient(options) still returns null for every command except sync, so the command always fails with Review sync requires a change-request provider before it can check PR readiness. I verified this path with node scripts/tabellio-review.mjs gate --owner example --remote-repo project --number 7; the fix is to construct and require the GitHub provider/token for gate as well as sync.
Useful? React with 👍 / 👎.
| if (cycle.changeRequest.state !== "open") { | ||
| throw new Error(`Review gate requires an open pull request; change request is ${cycle.changeRequest.state}. Ready evidence cannot be created retroactively.`); | ||
| } | ||
| if (cycle.status !== "ready") { |
There was a problem hiding this comment.
In repositories that have ordinary successful GitHub checks but have not published a Tabellio validation result for this PR head, sync can still derive cycle.status === "ready" because the aggregate check state is success; this new gate then accepts the cycle and records ready evidence even though exact-head validation is missing. I confirmed this with a fake provider returning one successful check and an empty validation ledger: assertPreMergeReviewReady() returned the ready cycle, so the gate needs to assert that a latest validation result for cycle.changeRequest.headCommit exists and passed rather than trusting the aggregate status alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b08d7d5fcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (cycle.status !== "ready") { | ||
| throw new Error(`Review cycle is ${cycle.status}, not ready. Triage feedback, record fixes, publish the exact head, and rerun the gate.`); | ||
| } | ||
| if (!reviewCycleHasReadyEvidence(cycle, cycle.changeRequest.headCommit)) { | ||
| throw new Error("Review cycle is missing exact-head ready evidence."); |
There was a problem hiding this comment.
When the PR head has successful GitHub statuses/check runs but refs/tabellio/validations has no result for that head, sync() can still derive ready because mergeChecks() returns provider checks unchanged when latestValidationResult() is null. Since the new gate only accepts cycle.status === "ready" and the ready event that sync() just recorded, it exits 0 and persists pre-merge readiness without the durable exact-head Tabellio validation that the gate is meant to require; add an explicit successful validation-evidence check before accepting readiness.
Useful? React with 👍 / 👎.
|
|
||
| async function gateReview(manager, options) { | ||
| const result = await manager.sync({ number: options.number, actor: options.actor }); | ||
| assertPreMergeReviewReady(result.cycle); |
There was a problem hiding this comment.
When GitHub returns mergeable: null for a fresh or recently updated pull request while it is still computing mergeability, manager.sync() stores that unknown state and the existing readiness calculation only blocks mergeable === false; this assertion can therefore accept ready and let the pre-merge gate exit 0 before the gate knows whether the PR is mergeable. For a fail-closed gate, require changeRequest.mergeable === true here, or poll/refetch until GitHub reports a known value.
Useful? React with 👍 / 👎.
| const result = await manager.sync({ number: options.number, actor: options.actor }); | ||
| assertPreMergeReviewReady(result.cycle); |
There was a problem hiding this comment.
For a legacy v0.2 cycle that was ready but has no ready event, running tabellio-review gate after the PR is already merged still calls sync() first; that sync migrates the legacy state by writing an exact-head ready event, then the assertion fails because the PR is not open. This leaves durable post-merge readiness evidence behind after a failed gate, so a later release plan can treat the PR as release-ready even though the pre-merge gate never passed; avoid writing/migrating readiness until the freshly synced PR is known to be open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed7204a641
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| && cycle.checks.statuses.some((item) => ( | ||
| item.id.startsWith("validation:") | ||
| && item.context.startsWith("tabellio/") | ||
| && item.state === "success" |
There was a problem hiding this comment.
When the validation ledger contains multiple results for the same PR head, this predicate treats any synthetic tabellio/* success as satisfying the new pre-merge gate. Since tabellio-validate can write results for an alternate --manifest, a later passing secondary suite can mask a failed or missing canonical tabellio.validation.json run, allowing durable release-readiness evidence without the required product validation. Filter the selected validation by the platform/canonical manifest (or carry suite.manifestPath into this check) before marking the cycle release-ready.
Useful? React with 👍 / 👎.
| export function reviewCycleHasReleaseReadiness(cycle, headCommit) { | ||
| return reviewCycleHasReadyEvidence(cycle, headCommit) && readinessStatus(cycle) === "ready"; | ||
| return reviewCycleHasReadyEvidence(cycle, headCommit) | ||
| && reviewCycleHasPassedValidation(cycle, headCommit) | ||
| && readinessStatus(cycle) === "ready"; |
There was a problem hiding this comment.
For cycles that already contain a head-bound ready event from the previous rules, this only combines that old event with the current validation check state. If a passing validation result for the PR head is written after the PR has already merged, the terminal release sync can now see validation success plus the stale ready event and allow planRelease, even though the open-PR gate never passed with validation. Require the ready event to be created after/with the passing validation, or record a fresh ready event when validation first appears before merge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 728cab9120
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return cycle.events.some((item) => ( | ||
| item.type === "ready" | ||
| && item.detail === headCommit | ||
| && Date.parse(item.at) >= validationTime | ||
| )); |
There was a problem hiding this comment.
When the same PR head already has a ready event and a newer canonical validation result is written afterward (for example, CI or tabellio-validate is rerun before merge), this new timestamp comparison makes the existing ready event invalid, but recordReadyEvidence still refuses to append another ready event because any ready event for the head already exists. The next tabellio-review gate therefore persists the cycle and then fails as missing validation-bound evidence, leaving the PR ungateable until a new commit or manual ledger edit; readiness recording should account for whether the ready event is after the selected validation, not just whether one exists.
Useful? React with 👍 / 👎.
| ? latestValidationResult(this.validationLedger, changeRequest.source.commit, this.repositoryId, { | ||
| manifestPath: CANONICAL_VALIDATION_MANIFEST, | ||
| }) |
There was a problem hiding this comment.
For repositories whose tabellio.platform.json selects a validation manifest other than tabellio.validation.json (the platform validator allows any non-empty manifest, and release planning validates the caller's --manifest against that configured value), this hard-coded filter ignores the passed exact-head result under the configured manifest. In that setup the pre-merge gate keeps the cycle in validating/missing-validation state and the later release-readiness sync can never see the evidence it just required, so the manifest used here needs to come from the platform/release configuration or a gate option rather than the repository default.
Useful? React with 👍 / 👎.
| const matchingStatus = [ | ||
| { active: cycle.changeRequest.draft, status: "draft" }, | ||
| { active: cycle.changeRequest.mergeable === false, status: "blocked" }, | ||
| { active: cycle.changeRequest.state === "open" && cycle.changeRequest.mergeable !== true, status: "blocked" }, |
There was a problem hiding this comment.
If an existing stored cycle was written while GitHub returned mergeable: null and the old derivation marked it ready, this stricter rule makes validateReviewCycle(existing) throw status does not match before sync can refresh the live PR state. GitHub commonly reports mergeable as null while computing, so older ready/open cycles can become unreadable after the upgrade; the migration path should either tolerate legacy status derivation while reading or refresh before enforcing the new unknown-mergeability block.
Useful? React with 👍 / 👎.
728cab9 to
a05c9a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a05c9a6738
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return cycle.events.some((item) => ( | ||
| item.type === "ready" | ||
| && item.detail === headCommit | ||
| && Date.parse(item.at) >= validationTime | ||
| )); |
There was a problem hiding this comment.
When an existing v0.3 cycle already has a ready event for the head from the previous readiness model (for example, it became ready from GitHub checks or an alternate validation manifest), adding the canonical tabellio.validation.json result and rerunning tabellio-review gate can still fail forever: this new timestamp binding rejects the old event as predating the canonical validation, while recordReadyEvidence still suppresses appending a newer ready event whenever any ready event for the head exists. That leaves otherwise valid open PRs requiring manual ledger surgery instead of another gate run.
Useful? React with 👍 / 👎.
| ? latestValidationResult(this.validationLedger, changeRequest.source.commit, this.repositoryId, { | ||
| manifestPath: CANONICAL_VALIDATION_MANIFEST, | ||
| }) |
There was a problem hiding this comment.
When a repository uses a non-default platform.validation.manifest path (which the platform config accepts and release planning binds), tabellio-review gate still filters the validation ledger to the hard-coded tabellio.validation.json. In that configuration an exact-head validation result for the configured manifest is ignored, so the PR remains validating and can never satisfy the new pre-merge gate even though the canonical configured validation passed.
Useful? React with 👍 / 👎.
760c58a to
d6ce15b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ce15ba81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ? latestValidationResult(this.validationLedger, changeRequest.source.commit, this.repositoryId, { | ||
| manifestPath: validationManifestPath, | ||
| }) |
There was a problem hiding this comment.
When planRelease is invoked with the manifest named by the merged commit's tabellio.platform.json and that path is not tabellio.validation.json, assertMergedReview still constructs ReviewCycleManager without a validationManifestPath/resolver (scripts/lib/release-planner.mjs:212-220). With this new manifest filter, the terminal post-merge sync looks only for the default manifest on the PR head, so a pre-merge tabellio-review gate that passed using the PR head's configured manifest is treated as missing validation and release readiness fails after merge. Please pass the release/PR-head manifest into this manager, or resolve it from the PR head, before filtering.
Useful? React with 👍 / 👎.
| return cycle.events.some((item) => ( | ||
| item.type === "ready" | ||
| && item.detail === headCommit | ||
| && Date.parse(item.at) >= validationTime |
There was a problem hiding this comment.
When a cycle already has 100 events and the existing ready event is the entry dropped by the next sync, appendSyncedEvent recreates it with the current sync timestamp; this timestamp comparison then treats that synthetic event as occurring after the latest validation. In a terminal post-merge sync, that can convert stale pre-merge readiness into apparently fresh validation-bound readiness after a newer validation run, without any pre-merge gate after that validation. Preserve the original ready event timestamp, or avoid synthesizing readiness during terminal sync, so stale-ready refresh still fails closed.
Useful? React with 👍 / 👎.
| if (status !== "ready") return; | ||
| if (reviewCycleHasReadyEvidence(cycle, cycle.changeRequest.headCommit)) return; | ||
| if (reviewCycleHasValidationBoundReadyEvidence(cycle, cycle.changeRequest.headCommit)) return; | ||
| cycle.events = appendEvent(cycle.events, event("ready", readinessActor(cycle.events), cycle.updatedAt, cycle.changeRequest.headCommit)); |
There was a problem hiding this comment.
When a cycle is synced through a ReviewCycleManager constructed with the default validationLedger = null, a green provider state can still reach ready and append a ready event here even though the sync did not consume any validation: status. If a passed validation result for that head already has an earlier completedAt, a later terminal release sync can treat that older unvalidated event as validation-bound readiness, allowing release planning to pass without a pre-merge gate that actually observed the validation. Only create ready evidence when the cycle already contains the passed exact-head validation status, or bind the event to the validation run.
Useful? React with 👍 / 👎.
d6ce15b to
448053e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 448053ebb2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ledger, | ||
| validationLedger, | ||
| validationManifestResolver: async (commit) => { | ||
| const source = await runGit({ args: ["show", `${commit}:tabellio.platform.json`], cwd: store.repoPath }); |
There was a problem hiding this comment.
When tabellio-review sync or gate is run from a maintainer clone or CI checkout that has not fetched the pull-request head SHA locally, this resolver aborts on git show <sha>:tabellio.platform.json before the command can read GitHub state or the validation ledger. The command only takes a PR number and this path does not fetch refs/pull/<number>/head (unlike release planning), so remote PRs whose head object is absent cannot be synced or gated; fetch the head or fall back to a configured/ledger manifest before resolving it from the commit.
Useful? React with 👍 / 👎.
| tabellio-review gate \ | ||
| --repo . \ | ||
| --owner owner \ | ||
| --remote-repo repository \ | ||
| --number 42 \ |
There was a problem hiding this comment.
In a GitHub-backed repo, this documented gate command omits --repo-id, so tabellio-review derives github.com/owner/repository, but the validation command just above records results under owner/repository; because the new gate filters validation results by repository ID, users following the README will ignore their own exact-head validation and stay stuck in validating. Include the same --repo-id value in both commands or update the validation example to use the GitHub identity.
Useful? React with 👍 / 👎.
902b814 to
461c985
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 461c985205
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ```bash | ||
| tabellio-review gate \ | ||
| --repo . \ | ||
| --repo-id owner/repository \ |
There was a problem hiding this comment.
For release-capable PRs, this explicit --repo-id owner/repository records the ready event under a different ledger path than release planning will read: planRelease() derives the code repository ID from the GitHub origin as github.com/owner/repository and then syncs the terminal review cycle with that ID. If a user follows this example before merging, the pre-merge ready evidence is stored under owner/repository; after merge the release planner cannot see it, and because readiness cannot be backfilled after merge the release is blocked. Omit this option here or show the host-qualified ID (github.com/owner/repository).
Useful? React with 👍 / 👎.
461c985 to
1152d2b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1152d2bf59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| --owner example \ | ||
| --remote-repo repository \ | ||
| --number 42 \ | ||
| --token-file /secure/path/github-token \ | ||
| --actor pre-merge-gate |
There was a problem hiding this comment.
In the getting-started flow, the validation result is written with --repo-id example/repository earlier in this file, but this new gate example omits --repo-id, so tabellio-review derives the repository identity from origin instead. In a normal GitHub checkout that identity is github.com/example/repository, and the gate filters validation results by that ID, so users following these steps will not see the exact-head validation they just recorded and will remain stuck in validating; either include the same repo ID here or change the validation example to the host-qualified ID.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Adds fail-closed exact-head validation and review readiness, release-bound evidence, GitHub outage-safe synchronization, and aligned operator documentation. Entire-Checkpoint: f045bb1ff820 Entire-Checkpoint: d3c91a7e4f22
1152d2b to
5716102
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57161027a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return cycle.events.some((item) => ( | ||
| item.type === "ready" | ||
| && item.detail === headCommit | ||
| && Date.parse(item.at) >= validationTime |
There was a problem hiding this comment.
When validation and the review gate run on different workers with skewed clocks, the gate can read a passed exact-head validation and append a ready event using its local now, but if that timestamp is earlier than validation.updatedAt this comparison rejects the event and the PR remains ungateable despite valid evidence. Bind the ready event to the validation run (or ledger version/order) instead of comparing machine clocks.
Useful? React with 👍 / 👎.
1 participant
Purpose
Prevent release planning from reaching a merged pull request without durable exact-head review-readiness evidence.
PR #26 passed local and hosted validation, received a favorable Codex review, and merged. The v0.5.0 release planner then correctly failed closed because its durable Tabellio review cycle had no pre-merge
readyevent. GitHub state alone was insufficient.Change
tabellio-review gateto live-sync the open pull request and require an exact-head durablereadyeventThis is an operator-side fail-closed gate. It does not claim GitHub server-side branch-protection enforcement.
Exact candidate evidence
1152d2bf599ff9b5f6d13473f5698dd1473faa14f045bb1ff820validation-6e73ecb2-827d-4ca1-b4c8-5eb5d942b412$0/private/tmp/tabellio-release-governance-evidence.jsongenerated and schema-validated locally; not committed because it contains local execution metadata. Its generated default-deny policy is not the audit record for the separately approved branch-history rewrite.External-action policy
Default deny. One explicitly approved destructive workspace action occurred: the PR branch was squashed and force-pushed with an exact lease. No tag, GitHub release, npm publication, deploy, infrastructure, DNS, billing, or production-data action attempted.
Commands
Skipped