fix(release): support verified squash publication - #33
Conversation
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRelease admission supports verified one-parent and two-parent candidates. It binds reviewed blobs, commit topology, workflow provenance, projection policy, immutable release targets, and repository readiness safeguards. ChangesRelease admission and repository safeguards
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant GitHubWorkflow
participant ReleaseValidation
participant Git
participant ReleaseTargets
GitHubWorkflow->>Git: resolve candidate parents, changed files, and blob SHAs
GitHubWorkflow->>ReleaseValidation: validate topology, provenance, and projection
ReleaseValidation-->>GitHubWorkflow: return admission result and projection digest
GitHubWorkflow->>ReleaseValidation: replay admission before mutation
ReleaseValidation-->>GitHubWorkflow: return proven candidate checkout
GitHubWorkflow->>ReleaseTargets: verify immutable tag and release target
ReleaseTargets-->>GitHubWorkflow: return target SHA confirmation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/release-projection.ts (1)
160-171: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDisable Git path quoting when you enumerate changed files.
git diff --name-onlyquotes paths that contain non-ASCII or special bytes whencore.quotePathis enabled, which is the default. A quoted path never equals the GitHub-reported filename, so the comparison at Line 92 fails with a misleading message instead of the real cause. Pass-c core.quotePath=false, or use-zand split on NUL.♻️ Proposed change
function gitChangedFiles(base: string, head: string): string[] { const result = Bun.spawnSync({ - cmd: ["git", "diff", "--name-only", base, head, "--"], + cmd: ["git", "-c", "core.quotePath=false", "diff", "-z", "--name-only", base, head, "--"], cwd: process.cwd(), stdout: "pipe", stderr: "pipe", }) if (result.exitCode !== 0) { throw new Error(`cannot resolve candidate changed files: ${result.stderr.toString().trim()}`) } - return result.stdout.toString().split("\n").filter(Boolean) + return result.stdout.toString().split("\0").filter(Boolean) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-projection.ts` around lines 160 - 171, Update gitChangedFiles to disable Git path quoting when invoking git diff, preferably by adding -c core.quotePath=false before the diff arguments, so returned filenames match GitHub-reported paths exactly.scripts/repository-readiness.test.ts (1)
101-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the remaining
matchesDefaultBranchpatterns.
matchesDefaultBranchaccepts four include patterns:~ALL,~DEFAULT_BRANCH, the bare branch name, andrefs/heads/<branch>. These tests exercise only~DEFAULT_BRANCH. A regression that dropsrefs/heads/mainfrom the accepted list would pass.Two more gaps: a non-empty
excludemust fail closed, and an empty-stringdefaultBranchmust returnunavailable(line 225 ofscripts/repository-readiness.ts).💚 Suggested additional cases
+ test.each([ + ["~ALL", ["~ALL"]], + ["bare branch name", ["main"]], + ["fully qualified ref", ["refs/heads/main"]], + ] as const)("accepts a ruleset that targets the default branch by %s", (_condition, include) => { + expect( + classifyDirectPushProtection( + [reviewedDefaultBranchRuleset({ conditions: { ref_name: { include, exclude: [] } } })], + "main", + ), + ).toMatchObject({ status: "ready" }) + }) + + test.each([ + [ + "exclusion", + [ + reviewedDefaultBranchRuleset({ + conditions: { ref_name: { include: ["~DEFAULT_BRANCH"], exclude: ["refs/heads/main"] } }, + }), + ], + "main", + ], + ["empty default branch", [reviewedDefaultBranchRuleset()], ""], + ] as const)("fails closed for %s", (_condition, rulesets, defaultBranch) => { + expect(classifyDirectPushProtection(rulesets, defaultBranch)).toMatchObject({ + status: "unavailable", + }) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/repository-readiness.test.ts` around lines 101 - 152, Add test cases in the “default branch direct-push protection” suite for each remaining accepted matchesDefaultBranch include pattern: ~ALL, the bare branch name, and refs/heads/<branch>. Also verify a non-empty conditions.ref_name.exclude fails closed with status unavailable, and that passing an empty defaultBranch to classifyDirectPushProtection returns unavailable.scripts/repository-readiness.ts (3)
312-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the
merge-commitscheck name no longer matches its meaning.The check now proves squash-merge availability. The name
merge-commitsand the constantmergeCommitRepairstill describe merge commits. Renaming both tosquash-mergewould align the readiness output with the new contract. The rename touchesscripts/repository-readiness.test.tsand the readiness output documented inREADME.md, so defer it if the output string is treated as a stable contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/repository-readiness.ts` around lines 312 - 332, Optionally rename the merge-method readiness symbols from mergeCommitRepair and the "merge-commits" check name to squash-merge equivalents so they accurately represent squash-merge availability. Update all references in scripts/repository-readiness.test.ts and the documented readiness output in README.md; defer the rename if the existing output string is a stable contract.
233-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider skipping non-branch rulesets instead of failing closed.
Line 234 returns
unavailablefor any element whosetargetis not"branch". The production caller filters branch summaries first, so this path is unreachable today. Any future caller that passes a mixed ruleset list would reportunavailablefor a correctly protected repository.classifyTagRulesetusescontinuefor the same condition.Separating "not a record" (unreadable, fail closed) from "different target" (skip) keeps the two classifiers consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/repository-readiness.ts` around lines 233 - 241, Update the ruleset iteration in the direct-push classifier to continue past records whose target is not "branch", matching classifyTagRuleset. Keep the unavailable fail-closed result for values that are not records, and continue evaluating remaining branch rulesets.
1050-1062: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repository payload once.
The guard
repositoryResponse.ok && isRecord(repositoryResponse.data)is repeated on lines 1050-1053 and 1058-1060.♻️ Extract `repositoryData`
- const squashMergeEnabled = - repositoryResponse.ok && isRecord(repositoryResponse.data) - ? repositoryResponse.data.allow_squash_merge - : undefined + const repositoryData = + repositoryResponse.ok && isRecord(repositoryResponse.data) ? repositoryResponse.data : undefined const checks: ReadinessCheck[] = [ checkTagRuleset(repository), - checkDirectPushProtection( - repository, - repositoryResponse.ok && isRecord(repositoryResponse.data) - ? repositoryResponse.data.default_branch - : undefined, - ), - checkMergeHistoryPolicy(repository, squashMergeEnabled), + checkDirectPushProtection(repository, repositoryData?.default_branch), + checkMergeHistoryPolicy(repository, repositoryData?.allow_squash_merge), ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/repository-readiness.ts` around lines 1050 - 1062, In the readiness-check setup, extract the guarded repository payload into a single repositoryData value before defining squashMergeEnabled and checks. Reuse repositoryData for both allow_squash_merge and default_branch, preserving undefined when the response is unsuccessful or not a record.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 386: Update the README guidance in the “Protect main” step so the
required checks are configured through classic branch protection, matching the
data source used by bun run readiness; alternatively, update the readiness
implementation to read effective ruleset requirements and retain equivalent
coverage. Ensure the listed Conventional Commit title, Release impact, canary,
Compatibility, and Deterministic package checks remain required.
In `@scripts/release-validate.ts`:
- Around line 297-309: Update admitCandidate’s topology validation to require
every candidate parent, trustedBaseSha, mergedPrBaseSha, and reviewedPrHeadSha
value used in comparisons to be a valid 40-hex commit SHA before performing
equality checks. Reuse the same validation for repair inputs, ensuring empty or
malformed SHAs are rejected even when values match.
---
Nitpick comments:
In `@scripts/release-projection.ts`:
- Around line 160-171: Update gitChangedFiles to disable Git path quoting when
invoking git diff, preferably by adding -c core.quotePath=false before the diff
arguments, so returned filenames match GitHub-reported paths exactly.
In `@scripts/repository-readiness.test.ts`:
- Around line 101-152: Add test cases in the “default branch direct-push
protection” suite for each remaining accepted matchesDefaultBranch include
pattern: ~ALL, the bare branch name, and refs/heads/<branch>. Also verify a
non-empty conditions.ref_name.exclude fails closed with status unavailable, and
that passing an empty defaultBranch to classifyDirectPushProtection returns
unavailable.
In `@scripts/repository-readiness.ts`:
- Around line 312-332: Optionally rename the merge-method readiness symbols from
mergeCommitRepair and the "merge-commits" check name to squash-merge equivalents
so they accurately represent squash-merge availability. Update all references in
scripts/repository-readiness.test.ts and the documented readiness output in
README.md; defer the rename if the existing output string is a stable contract.
- Around line 233-241: Update the ruleset iteration in the direct-push
classifier to continue past records whose target is not "branch", matching
classifyTagRuleset. Keep the unavailable fail-closed result for values that are
not records, and continue evaluating remaining branch rulesets.
- Around line 1050-1062: In the readiness-check setup, extract the guarded
repository payload into a single repositoryData value before defining
squashMergeEnabled and checks. Reuse repositoryData for both allow_squash_merge
and default_branch, preserving undefined when the response is unsuccessful or
not a record.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 607c336e-1f0e-41be-bd72-d332b6698b00
📒 Files selected for processing (9)
.github/workflows/release.ymlREADME.mddocs/adr/0003-reviewed-versioned-releases.mdscripts/release-projection.test.tsscripts/release-projection.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/repository-readiness.test.tsscripts/repository-readiness.ts
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/release-projection.test.ts (1)
163-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the newline-path regression assertion.
The test creates a newline-containing filename but checks only the generic changed-file-set error. A line-delimited parser would produce the same error and still pass this test.
Use a temporary projection that includes the exact path, then assert that the
unsupported patherror containsunreviewed-☃\nfile.txtas one value. Alternatively, test the changed-file helper directly.Also applies to: 192-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-projection.test.ts` around lines 163 - 179, Strengthen the newline-path regression test around the projection setup and assertion by using a temporary projection containing the exact path unreviewed-☃\nfile.txt, then verify the unsupported path error reports that filename as one value rather than only asserting the generic changed-file-set error. Apply the same assertion update to the related case at the second referenced location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/repository-readiness.ts`:
- Around line 156-158: Update the branch matching logic around matchesBranch to
use GitHub-compatible fnmatch semantics instead of exact string membership, so
wildcard patterns such as refs/heads/* match the corresponding branch refs in
both include and exclude lists. Preserve the existing ~ALL, ~DEFAULT_BRANCH, and
default-branch handling, and add coverage for wildcard include and exclude
cases.
---
Nitpick comments:
In `@scripts/release-projection.test.ts`:
- Around line 163-179: Strengthen the newline-path regression test around the
projection setup and assertion by using a temporary projection containing the
exact path unreviewed-☃\nfile.txt, then verify the unsupported path error
reports that filename as one value rather than only asserting the generic
changed-file-set error. Apply the same assertion update to the related case at
the second referenced location.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64a4dacf-74f1-4bf4-9173-a0f58cee625f
📒 Files selected for processing (7)
README.mdscripts/release-projection.test.tsscripts/release-projection.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/repository-readiness.test.tsscripts/repository-readiness.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/repository-readiness.test.ts
- README.md
- scripts/release-projection.ts
- scripts/release-validate.ts
- scripts/release-validate.test.ts
|
@codex review |
![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: 36fc067fc3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex-gate approve 4f653a8 5231078558 |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Release publication now supports GitHub's squash path without weakening the existing provenance and immutable-release checks. Ordinary pull requests and Release Please pull requests can stay squashable; repositories no longer need merge-commit-only history to publish safely.
This replaces the one-off recovery used in #31 with the durable release contract. PR #32 remains blocked until this change is merged and the new path is qualified on its exact head.
Design decisions
Validation
bun test: 563 passed, 3 explicitly manual/hosted cases skipped, 0 failed.bun run prove:all: passed from clean commit99af6ad, including native Claude/Codex install mechanics, runtime custody, deterministic packaging, offline execution, distribution checksums, and DX proof.bun run generate:check,bun run release:validate -- --json,actionlint .github/workflows/release.yml, andgit diff --check: passed.Post-Deploy Monitoring & Validation
v0.2.0release.Related: #32
Summary by CodeRabbit
Release Process
Documentation
Tests