feat(workflows): add automated publish-release workflow

[flora131]

Summary

Adds a fully automated, agent-driven release pipeline as a project-local Atomic workflow (.atomic/workflows/publish-release.ts), with deterministic GitHub API verification at each irreversible step. Also fixes the @bastani/atomic root export so Jiti-based workflow loading resolves the published entrypoint.

Key Changes

Workflow (.atomic/workflows/publish-release.ts)

Inputs: target_version (no leading v) and release_kind ("release" | "prerelease")

Five sequential, gated stages:

Stage	What it does
prepare-release-branch-and-metadata	Creates release/<version> or prerelease/<version> from HEAD, updates CHANGELOG.md, bumps version via scripts/bump-version.ts, commits
run-release-checks	Runs bun run typecheck and bun run test:unit before opening any PR
open-release-pr	Pushes branch, creates/reuses PR targeting main via gh, verifies base/head/SHA from GitHub API
wait-for-release-ci-and-merge	Polls required CI checks, merges when all pass
tag-and-monitor-publish	Syncs main, pushes the version tag, monitors the GitHub Actions publish run

Version validation — enforces MAJOR.MINOR.PATCH for releases and MAJOR.MINOR.PATCH-alpha.REVISION (revision ≥ 1) for prereleases; rejects malformed versions before any side effects occur.

Helper library (.atomic/workflows/lib/publish-release.ts)

Shared utilities extracted for testability:

• validateReleaseRequest — typed ValidatedRelease output with format enforcement
• hasStatusMarker / hasLeadingStatus — last-wins and strict first-line status parsing
• verifyReleasePullRequestReferenceJson — deterministic PR reference verification (url, number, base/head refs)
• verifyPullRequestMergedJson — deterministic PR merge verification (state, mergedAt, mergeCommit.oid)
• verifyPullRequestChecksJson — CI check status verification
• selectPublishWorkflowRunJson / verifyPublishWorkflowRunJson — GitHub Actions run selection and verification
• releaseVersionPattern / prereleaseVersionPattern — exported regexes
• runCommand — sanitizes the Git environment via createGitEnvironment() before spawning subprocesses

Tests (test/unit/publish-release-helpers.test.ts)

257-line unit test suite covering:

• Version validation: accepts/rejects stable vs alpha formats, leading-v guard
• Status parsing: strict first-line check, last-wins semantics, preamble tolerance, rejects inline/bulleted/partial/wrong-key markers
• GitHub verification: accepts valid PR reference and fully merged PR JSON; rejects unmerged, mismatched refs, or missing fields
• GitHub Actions run selection and verification

Package fix (packages/coding-agent/package.json)

Added "default": "./dist/index.js" to the "." root export condition — fixes Jiti-based workflow loading for user-authored workflows that import from the @bastani/atomic root package.

Spec (specs/2026-06-10-publish-release-workflow.md)

RFC documenting full architecture: Mermaid stage graph, door-set contracts (guarantee/refusal/chokepoint), stage plan with prompt requirements, cross-cutting concerns (security, irreversibility, Bun compliance, evidence requirements).

Notes

• Purely additive — no changes to existing packages, CI configuration, or workflow runtime internals
• The workflow refuses to fabricate success: if gh auth, git state, CI, or publish checks block safe progress, it halts with evidence
• Validated locally with bun run typecheck and bun run test:unit; pre-commit/pre-push hooks passed

[claude]

Code Review: feat(workflows): add publish release workflow

Nice work — this is a well-structured, defensive workflow with a thoughtful RFC. The version validation, the "stop rather than fabricate success" framing, and the separation of the two irreversible doors (merge / tag) are all good. The SDK usage is correct: ctx.task(...).text, Type.* schemas, declared outputs, and export default ...compile() all match @bastani/workflows conventions, and .atomic/workflows/*.ts discovery will pick this up. AGENTS.md (referenced in the prompts) is a symlink to CLAUDE.md, so those references resolve. A few things worth addressing before merge, roughly in priority order.

1. Status-marker detection is fragile for an irreversible workflow (highest priority). hasStatus is a bare substring check: return text.includes(marker). Each gate (PREPARE_STATUS: ready, CHECK_STATUS: passed, MERGE_STATUS: merged, PUBLISH_STATUS: completed) decides whether to advance toward pushing a branch, merging a PR, and pushing a publish-triggering tag. Because the prompts themselves contain the success literal (e.g. "Start with PREPARE_STATUS: ready or PREPARE_STATUS: blocked"), an agent that echoes the format instructions while actually reporting a failure can produce text containing both PREPARE_STATUS: ready and ... blocked. includes() returns true on the first, and the workflow advances. For a flow whose downstream stages have irreversible remote effects, a false-positive gate is the most dangerous failure mode here. Recommend anchoring the check to the first non-empty line and asserting the failure literal is absent, e.g. firstLine.startsWith(ready) && !firstLine.startsWith(failed). Since every prompt already says "Start with X or Y", checking the first line matches the contract you defined and removes the echo ambiguity.

2. context: "fork" is a silent no-op here. Every stage passes context: "fork" but no forkFromSessionFile. In stage-runner.ts the fork branch is if (context === "fork" && forkFromSessionFile !== undefined). Without forkFromSessionFile that branch is skipped and each stage gets a fresh session — the builtin workflows (ralph.ts, goal.ts) always pair context: "fork" with forkFromSessionFile: sessionFile. So the option is dead/misleading as written. If you intend independent fresh stages (which seems to be the case, since handoff is done by injecting excerpt(prev.text) into the next prompt), drop the context field and rely on the default. If you wanted continuity, thread forkFromSessionFile from the prior stage sessionFile.

3. Cross-stage state handoff relies on implicit filesystem git state. Correctness across stages depends on the branch/commit created in stage 1 still being checked out on disk when stages 2/3 run. That works only because no stage sets worktree: true and they share cwd — but nothing in the code enforces or documents that invariant. If anyone later adds worktree: true to a stage (a natural-looking change), the branch/commit handoff silently breaks. Worth a comment near the stage definitions noting these stages intentionally share the working tree and that worktree isolation must not be enabled.

4. No automated tests, despite easily testable pure helpers. CLAUDE.md leans hard on TDD, and the spec Test Plan is entirely manual. Yet validateReleaseRequest, cleanUrl, urlsIn, firstPrUrl, firstActionsUrl, and excerpt are pure and trivially unit-testable — the validation regex logic in particular (alpha revision must start at 1, leading-v rejection, release vs prerelease mismatch) is exactly what should be locked down by tests. They are currently un-exported, so they cannot be reached. Suggest exporting them (or a small test surface) and adding a bun:test suite. High-value, low-cost given the safety-critical nature of the validation.

Minor.

• Best-effort URL fallback can mislead: firstPrUrl/firstActionsUrl fall back to firstUrl(text) when no /pull/ or /actions/runs/ URL is present, so pr_url / publish run URL could point at an unrelated link. Acknowledged as best-effort in the description, but consider returning undefined instead of a wrong URL.
• Unbounded waits: CI/publish monitoring delegates entirely to gh ... --watch. If CI hangs, the stage hangs with no workflow-level timeout. Inherent to the toolful design, but worth noting in the RFC failure section.
• Handoff truncation: prior-stage output is injected at excerpt(text, 1_200). Git evidence (status, SHAs, changed-file lists) can exceed that and get cut before the next stage sees it. Probably fine, but bump the limit for a handoff that needs full prior evidence.
• Spec/impl naming drift: the RFC door/stage names (merge_verified_release_pr, publish_release_tag, prepare_release_branch) do not match the implemented stage names (wait-for-release-ci-and-merge, tag-and-monitor-publish). Not a bug, but aligning them would make the RFC easier to map onto the code.

Overall the structure and safety posture are solid; items 1 and 2 are the ones I would want fixed before this drives a real release.

[claude]

PR Review: publish-release workflow

Nicely structured, purely-additive change. The split into deterministic TypeScript (validation, URL/marker helpers) plus toolful agent stages reads well, the irreversible operations (merge, tag push) are isolated to dedicated stages with explicit gates, and the prompts consistently demand command-backed evidence over prose. The AGENTS.md references resolve fine (it's a symlink to CLAUDE.md). Below is feedback by category.

🐞 Potential bugs / correctness

• hasLeadingStatus second clause is dead code. firstLine === successMarker && !firstLine.includes(failureMarker) — once firstLine is exactly e.g. PREPARE_STATUS: ready, it can never .includes("PREPARE_STATUS: blocked"). The failureMarker argument is effectively unused. Harmless, but either drop the param or make the intent explicit.

• Auto-merge vs. the irreversible tag door (highest-risk path). Stage 4 instructs "merge or enable auto-merge", while stage 5 immediately syncs main, tags, and pushes — an irreversible publish trigger. Enabling auto-merge returns before the PR is actually merged, so if an agent emits MERGE_STATUS: merged on the strength of "auto-merge enabled," stage 5 could tag a main that doesn't yet contain the release commit. The prompt does ask for mergedAt/mergeCommit evidence (good), but the "enable auto-merge" wording invites premature success. Recommend tightening: forbid MERGE_STATUS: merged unless state == "MERGED" and mergedAt is non-null, and make stage 5's ancestor check (step 2) a hard precondition for the tag push.

🧪 Test coverage (biggest gap)

The deterministic, correctness-critical core has zero automated tests, and the spec's test plan only lists typecheck + manual inspection. These are pure, trivially testable functions guarding the irreversible doors:

• validateReleaseRequest — the release/prerelease regexes, leading-v rejection, -alpha.0 rejection, kind/format mismatch.
• hasLeadingStatus / firstNonEmptyLine — the gate that decides whether to proceed past each door.
• urlsIn / cleanUrl / firstPrUrl / firstActionsUrl — URL extraction and trailing-punctuation stripping.

A small bun:test suite over these would lock in the validation contract cheaply and is well worth adding given they gate publishing.

🎨 Code quality / consistency

• releaseKindSchema defined but not reused for the input. The output declaration uses releaseKindSchema, but the release_kind input re-inlines Type.Union([Type.Literal("release"), Type.Literal("prerelease")], …). Reuse the constant for both.
• firstPrUrl / firstActionsUrl fall back to any URL when the /pull/ or /actions/runs/ pattern is absent, which could surface an unrelated URL (e.g. a repo link) as the PR/Actions URL. Low impact (summary string only), but consider returning undefined rather than a possibly-wrong URL.

📄 Spec ↔ implementation drift

The architecture diagram (§4.1) and door set (§4.4) enumerate granular stages — update_release_metadata, sync_main_after_merge, verify_published_release, etc. — that don't exist as separate stages in the implemented 5-stage workflow (§5.4 does match the code). Worth reconciling §4.1/§4.4 so future readers don't look for stages that were intentionally collapsed.

⚠️ Design deviation worth surfacing

CLAUDE.md's release flow (steps 9/12) says to ask the user via ask_user_question when CI/publish checks fail. This workflow instead returns blocked/failed autonomously and never prompts. The spec documents this as intentional ("fully autonomous … pause/report only on failures"), so it's a choice rather than a bug — but maintainers should know the workflow won't escalate interactively on a failed check; ctx.ui.confirm/select is available if you ever want that.

🔒 Security

Reasonable for an internal release tool. The gates trust agent-emitted first-line markers, so a misbehaving agent could in principle assert success without doing the work — inherent to the toolful-agent model and acceptably mitigated by requiring git/gh evidence in every prompt. No secrets are handled; it relies on local gh/git credentials and correctly instructs stages to stop rather than fabricate success when auth is missing.

---

Overall a solid, well-isolated workflow. The two things I'd prioritize before relying on it for a real release: (1) tighten the merge→tag handoff against the auto-merge race, and (2) add unit tests for the validation/gating helpers.

🤖 Automated review by Claude Opus 4.8 — advisory, not a substitute for maintainer judgment.

[claude]

Code Review: feat(workflows): add automated publish-release workflow

Nice, well-structured addition. The workflow correctly uses the @bastani/workflows DSL (defineWorkflow().input().output().run().compile(), ctx.inputs, ctx.task(...).text), the return objects respect the NoExtraOutputs contract and optional-output semantics, all local commands are Bun-only, no build step is introduced, and the two irreversible doors (merge, tag push) are correctly isolated into dedicated stages. The companion spec is thorough. A few things worth addressing before this is relied on for real releases.

🔴 Test coverage — the main gap

CLAUDE.md leans heavily on TDD, but this PR adds zero automated tests. The deterministic helpers are pure and trivially unit-testable, yet they're module-private so they can't be covered as written:

• validateReleaseRequest / the two version regexes — the highest-value thing to lock down. Worth asserting: 1.2.3 accepted as release / rejected as prerelease; 1.2.3-alpha.1 accepted as prerelease / rejected as release; 1.2.3-alpha.0 rejected (revision starts at 1 ✅ your [1-9]\d* handles this); leading v rejected; 1.2.3-beta.1 rejected.
• firstPrUrl / firstActionsUrl / cleanUrl — trailing-punctuation stripping, the /pull/ vs /actions/runs/ selection, and the firstUrl fallback.
• hasLeadingStatus / firstNonEmptyLine — exact-match behavior with leading blank lines / CRLF.

Suggest exporting these (or moving them to a small sibling module) and adding a test/unit spec. The spec's "test plan" is entirely manual; the pure logic deserves real coverage.

🟠 Exact-first-line gating is brittle after the irreversible action

hasLeadingStatus requires the stage's first non-empty line to equal the marker exactly. Failing closed is the right instinct for a release pipeline, but consider the merge and publish stages specifically:

• In wait-for-release-ci-and-merge, the PR merge happens inside the agent task. If the agent merges successfully but its final message leads with any prose before MERGE_STATUS: merged (LLMs frequently add a preamble), the gate returns blocked even though the merge already happened. The reported status then contradicts reality.
• Same for tag-and-monitor-publish: tag push + publish may have already fired, yet a formatting slip yields failed.

The re-entrancy instructions in the prompts ("reuse existing PR", "confirm tag does not already exist") partly mitigate a re-run, but a misleading terminal status on an already-completed irreversible action is a real footgun. Consider a more robust contract — e.g. match the marker anywhere in the text (or on the last non-empty line), or have the agent emit the marker and parse gh/git evidence structurally.

🟡 Deviation from CLAUDE.md's interactive failure handling

The documented release flow (CLAUDE.md steps 9 & 12) says to ask the user via ask_user_question when CI/publish checks fail. This workflow instead stops and returns a blocked/failed summary with no interactive prompt. That's a defensible design for an autonomous workflow (and the spec calls it out), but it's a behavioral divergence worth being explicit about: on failure the maintainer must notice the returned status and intervene manually, rather than being actively prompted.

🟡 Minor

• firstActionsUrl fallback can pick the wrong URL. When no /actions/runs/ URL is present it falls back to firstUrl, which could surface an unrelated URL (e.g. a PR link echoed in the publish transcript) labeled as the publish run. Low impact since it's best-effort, but slightly misleading.
• Inconsistent failure surfacing. validateReleaseRequest throws on bad input while every other failure path returns a structured blockedOutput. It's pre-side-effect so it's safe, but a structured failed output would be more consistent with the rest of the workflow's contract.

✅ Things done well

• Correct DSL usage and output-contract conformance (optional pr_url/tag omission handled correctly).
• Strong typing throughout, no any/unknown.
• Irreversible operations isolated to single, named stages ("sole merge door" / "sole publish trigger").
• Re-entrancy awareness baked into prompts (reuse branch/PR, pre-check tag existence).
• Branch created from current HEAD (not reset to main), matching the documented flow, and the AGENTS.md reference is valid (it's a symlink to CLAUDE.md).

Overall: solid, additive, and safe-by-construction in the happy path. The test gap and the after-the-fact gating brittleness on the two irreversible stages are the two things I'd want resolved before trusting it end-to-end.

🤖 Generated with Claude Code

[claude]

Review: feat(workflows): add automated publish-release workflow

Nice work — this is a well-structured, genuinely additive change. The decomposition into 5 gated stages, the extraction of pure logic into a testable helper module, and the accompanying RFC all reflect real care. A few things I verified and a few suggestions below.

What is good

• Helper extraction + tests. Pulling validation/URL/status parsing into lib/publish-release-helpers.ts and unit-testing them in isolation is the right call — the orchestration body cannot easily be tested without live agents, so this maximizes deterministic coverage.
• lib/ subdir is the correct trick. I confirmed scanWorkflowDir in packages/workflows/src/extension/discovery.ts filters on e.isFile() (non-recursive), so the helper module inside lib/ will not be mis-discovered as a workflow and fail the __piWorkflow sentinel check. Good instinct.
• Two-door isolation is real. Merge (wait-for-release-ci-and-merge) and tag push (tag-and-monitor-publish) are in separate stages, and the tag stage is unreachable unless the merge stage reports MERGE_STATUS: merged. Blast radius is genuinely separated.
• Convention compliance. .js import extensions, no build step, Bun-only command instructions in prompts, defineWorkflow(...).compile(), all outputs declared, optional pr_url spread conditionally to avoid emitting pr_url: undefined. API usage (ctx.task then .text) matches the SDK (WorkflowTaskResult.text).
• Strict status parsing. hasStatusMarker requiring a whole-line exact match (and honoring the last marker for a given key) correctly rejects inline/bulleted/prose mentions — the test matrix for this is thorough.

Suggestions / concerns

1. (Medium, practical) Gate brittleness can produce a blocked result AFTER irreversible work already happened. The gates depend on the agent emitting a free-form line exactly like MERGE_STATUS: merged. The strictness is the right safety tradeoff (false positives would be far worse), but the failure mode is worth calling out: if the merge succeeds but the agent formats the marker slightly off (backticks, trailing period, wraps it in prose), the workflow reports blocked even though the PR is already merged. Re-running then re-enters prepare/pr stages against a half-completed release. Since the dangerous side effects live inside the stage and the gate is post-hoc, the prompts cannot fully guarantee recoverability. Consider documenting the manual-recovery path in the RFC failure section, and/or having the workflow body itself run a cheap deterministic re-check (e.g. a no-tools verification of gh pr view --json state) rather than trusting only the self-reported marker.

2. (Low) firstPrUrl / firstActionsUrl fall back to first URL of any kind. When the stage text contains no /pull/ or /actions/runs/ URL but does contain some other link (a docs URL, a changelog anchor), the fallback surfaces that unrelated URL as pr_url / Publish run. Since pr_url is a structured output field, a misleading value there is mildly worse than undefined. Consider dropping the fallback (return undefined when no typed URL is found) or labeling it as a candidate URL in the summary.

3. (Low) validateReleaseRequest throws instead of returning a structured output. Every other failure path returns a blockedOutput with status blocked/failed. The version-format check throws, so a malformed version surfaces as a raw workflow error rather than the structured failed summary the rest of the design promises. It is a safe input airlock either way, but catching it and returning a structured failed output would make the contract uniform.

4. (Low) Self-reported verification vs. the RFC adversarial verification claim. Section 4.2 describes adversarial verification, but in practice each gate trusts the same agent that did the work to honestly report its own status — there is no independent re-check by the workflow body. That is inherent to the agentic model and acceptable, but the RFC language slightly oversells it; worth softening or backing with an independent check (see item 1).

5. (Low) Dead/test-only export. hasLeadingStatus (and transitively firstNonEmptyLine) is not used by the production workflow — only hasStatusMarker is. It is exercised solely by tests (kept available for strict checks). Either wire it in or consider trimming to avoid implying it gates anything.

6. (Nit) cleanUrl strips close-paren, period, semicolon but not greater-than or close-bracket. If an agent emits a URL wrapped in angle brackets or markdown link syntax, the trailing bracket survives. Edge-casey given GH URLs, but cheap to extend the trailing-punctuation class.

Test coverage

Helpers are well covered (validation incl. leading-v and alpha.0 rejection, URL extraction, status-marker precedence). Gaps are the un-exported pure functions in the workflow module — excerpt, blockedOutput, releaseInstructions. The excerpt truncation math and blockedOutput shape would be worth a couple of tests; moving them into lib/ (or exporting them) would make that trivial.

---

Overall this is solid and safe-by-default. None of the above is blocking; items 1 and 2 are the two I would most want addressed (or at least documented) before merge.

Note: I reviewed statically and against the SDK source; I was not able to execute bun test / bun run typecheck in this environment, so please confirm CI is green.

Generated with Claude Code

[claude]

Review: feat(workflows): add automated publish-release workflow

Thanks for this — it's a well-structured, defensively-designed workflow. Separating the deterministic merge gate from the LLM-driven stages is exactly the right instinct for an irreversible operation, the helper extraction makes the risky logic unit-testable, and the spec/door-set framing is genuinely good. Comments below, ordered roughly by impact.

🐛 Potential bugs / correctness

1. Branch-retention gate will hard-block releases if the repo auto-deletes head branches (highest impact).
verifyReleasePrMerged (publish-release.ts:113-125) treats a missing remote branch as a failure after the merge already happened:

const branchCheck = runCommand(["git", "ls-remote", "--heads", "origin", release.branch]);
if (branchCheck.exitCode !== 0 || branchCheck.stdout.length === 0) {
  return { ok: false, /* ... branch not found ... */ };
}

The prompt tells the agent "Do not delete the release branch after merge," but that instruction cannot override the repo-level "Automatically delete head branches" setting — if it's enabled (a very common default), GitHub deletes the branch on merge regardless of the agent. Result: the PR merges successfully, then this gate reports blocked, and the release is left half-done (merged but never tagged/published). Please confirm delete_branch_on_merge is false for bastani-inc/atomic; if it isn't, this gate needs to be removed or downgraded to a non-blocking warning. Branch retention is orthogonal to "did the merge succeed," which is what the gate exists to prove.

2. PR selection grabs the first /pull/ URL anywhere in the agent transcript.
firstPullRequestUrl (publish-release-helpers.ts:68) returns the first /pull/ URL in the text, which feeds prSelector at publish-release.ts:271. The transcript can easily contain unrelated PR links — e.g. AGENTS.md/CHANGELOG.md attribution examples reference https://github.com/earendil-works/pi-mono/pull/456, and the agent may echo those when summarizing the changelog. If such a URL appears before the real PR URL, gh pr view <wrong-url> runs against the wrong PR. The deterministic headRefName/baseRefName checks in verifyPullRequestMergedJson do catch this (ref mismatch → fail), so it's not a safety hole — but it would surface as a confusing false blocked. Consider preferring release.branch as the selector (gh pr view <branch> works and is unambiguous), or extracting the URL only from the dedicated status line the prompt already asks for.

🧹 Code quality

3. Dead fallback / duplicate helper. firstPrUrl is a pure alias of firstPullRequestUrl (publish-release-helpers.ts:72-74). At publish-release.ts:313:

const prUrl = firstPullRequestUrl(pr.text) ?? mergeVerification.prUrl ?? firstPrUrl(pr.text);

the third operand is identical to the first, so it's unreachable — if firstPullRequestUrl(pr.text) is undefined, firstPrUrl(pr.text) is too. The PR description presents these as two distinct helpers, but one just delegates to the other. Suggest dropping firstPrUrl (and its test) and simplifying to firstPullRequestUrl(pr.text) ?? mergeVerification.prUrl.

4. Title scope nit. feat(workflows) reads as a change to @bastani/workflows, but this is purely a project-local workflow under .atomic/workflows/. Minor — something like feat(release) would be clearer.

🧪 Test coverage

Helper coverage is strong (validation, URL extraction, last-wins status parsing, merge-JSON verification — nicely done, including CRLF and negative cases). The gap is everything that lives in publish-release.ts rather than the lib: verifyReleasePrMerged (the spawn + JSON + branch-retention composition — the riskiest deterministic logic), excerpt, and blockedOutput are untested. Since the .run body can't easily be unit-tested, consider moving excerpt/blockedOutput into the helper lib and factoring verifyReleasePrMerged so the command runner is injectable — then the branch-retention and JSON-parse-failure branches can be covered directly. That logic is exactly where a regression would be most expensive.

🔐 Security / irreversibility

The guardrails are good overall. Two things worth stating explicitly:

• The deterministic gate proves merge state before tagging, but the tag push and publish themselves are still agent-driven (tag-and-monitor-publish), gated only by the PUBLISH_STATUS: completed text marker. Acceptable given publishing needs monitoring, but it means the "GitHub state, not LLM output, is the source of truth" guarantee applies to the merge door, not the tag door. The spec acknowledges this; flagging so reviewers don't over-read the guarantee.
• runCommand uses Bun.spawnSync with no cwd, inheriting the process working directory. Fine here, but a pinned cwd would make it robust against ever running from a different directory.

✅ Conventions

Bun-only commands, .js import specifiers, Type.* schemas, no build step, and .atomic/workflows/**/* is correctly inside tsconfig.json include so bun run typecheck covers these files. All consistent with CLAUDE.md.

Nothing here is blocking except confirming #1 — that one can leave a release stuck between merge and publish, so please verify the repo's auto-delete-branch setting before relying on this end-to-end.

[claude]

Code Review — feat(workflows): add automated publish-release workflow

Reviewed publish-release.ts, lib/publish-release-helpers.ts, the tests, and the spec. Well-architected and genuinely defensive — fail-closed gating, command-array (non-shell) invocation, deterministic merge verification, and strict version validation are all done right. A few substantive points to discuss, then minor notes.

Substantive

1. The publish/tag door is LLM-attested, not deterministically verified — inconsistent with the PR thesis.
The stated guarantee is "GitHub state, not LLM output, is the source of truth before any irreversible side effect." That holds for the merge door (verifyReleasePrMerged re-queries gh pr view --json + git ls-remote). But the publish door — the most irreversible action, since npm publish cannot be undone — is gated only by the agent self-reporting PUBLISH_STATUS: completed via hasStatusMarker(publish.text, ...). There is no deterministic post-check that the tag was actually pushed or that the publish run concluded successfully. Consider closing this symmetrically after the publish stage: git ls-remote --tags origin <version> to confirm the tag/SHA, and/or gh run view <id> --json conclusion to confirm conclusion == success. As written, an agent that believes it published (or emits the marker prematurely) yields status: completed with no GitHub-state backing — exactly the failure mode the rest of the workflow prevents.

2. The branch-retention check can wedge a legitimate release.
verifyReleasePrMerged treats a missing remote branch as a hard failure (ok: false) AFTER the merge has happened but BEFORE the tag is pushed. If the repo has "Automatically delete head branches" enabled (a common default), the branch is gone the instant the PR merges — regardless of the prompt instruction not to delete it, which the agent cannot control at the repo-settings level. Result: a successful merge that then blocks at verification, leaving the release half-done (merged, untagged, unpublished) and needing manual recovery. Recommend either decoupling branch retention from the merge gate (advisory, not blocking), or documenting the hard dependency on the auto-delete setting being off. The merge is fully evidenced by state: MERGED + mergeCommit.oid; branch presence is not load-bearing for the tag step.

Minor

3. hasLeadingStatus (and transitively firstNonEmptyLine) is exported and tested but never used in the production workflow — every gate uses hasStatusMarker. Wire it in or drop it.

4. The PR description references a firstPrUrl helper that does not exist in the code (only firstActionsUrl is present; the PR URL is derived from mergeVerification.prUrl ?? prReference.prUrl). Minor drift.

5. Testability: the pure helpers in lib/ have excellent coverage, but captureReleasePrReference / verifyReleasePrMerged / runCommand call Bun.spawnSync directly and are untestable in isolation. Injecting the command runner (an (args) => CommandResult param defaulting to the real one) would let you unit-test their branching — including the edge case in point 2 — without spawning real gh/git.

Things done well

• hasStatusMarker last-wins, standalone-line semantics correctly reject inline/bulleted/partial markers and tolerate model preambles; thoroughly tested.
• Version inputs validated against strict regexes before any side effect; all gh/git interpolation goes through command arrays (no shell), so no injection surface.
• Isolating the two irreversible doors into dedicated fail-closed stages is the right structure.
• AGENTS.md references resolve correctly (it is a symlink to CLAUDE.md), so the changelog pointer is valid.

I could not execute bun test / bun run typecheck in the review sandbox; the suite reads as correct and the PR reports both pass locally — worth confirming CI is green.

Overall: solid, defensible design. I would want point 1 (symmetric verification of the publish door) and a decision on point 2 (branch-retention coupling) resolved before this drives real releases.

[claude]

Review: feat(workflows): add automated publish-release workflow

Thorough, well-typed addition. The standout strength is the deterministic verification model: gating irreversible steps on gh ... --json parsed in TypeScript rather than trusting LLM status prose is exactly the right architecture for release automation, and the discriminated-union result types (PullRequestMergeVerification, PublishWorkflowRunVerification, etc.) are clean. Version validation, the -alpha.[1-9]\d* revision guard, and the AGENTS.md/publish.yml references all check out against the repo. Helper coverage is solid.

A few things worth addressing before merge.

Substantive

1. Branch-retention check can become a false blocker after the irreversible merge. verifyReleasePrMerged() treats an empty git ls-remote --heads origin <branch> as a hard failure. If this repo ever has GitHub's "Automatically delete head branches" setting enabled, the merge deletes the head branch regardless of any flag — so the workflow reports blocked even though the merge (the operation that actually matters) succeeded, and then refuses to tag. That leaves the release stuck half-done (merged but never published) on a blocker it can't recover from. Consider demoting branch-deletion to a warning in the summary rather than a hard gate, since merge-state is the real invariant.

2. No deterministic gate that CI was green before the merge. The spec's door contract for merge_verified_release_pr guarantees it "merges only a PR whose required checks have passed," but the implementation only deterministically verifies the post-merge state (state === MERGED, mergedAt, mergeCommit.oid, refs). Whether checks were actually green is left entirely to the agent stage (gh pr checks --watch). An agent that merged with --admin over a red PR would pass every deterministic check here. Since you already verify merge-state deterministically, consider also capturing the checks rollup (gh pr checks --json ...) deterministically before accepting the merge, to close the gap between the stated guarantee and what's enforced.

3. Synchronous blocking on long-running waits. runCommand() uses Bun.spawnSync, and verifyPublishWorkflowSucceeded() calls it for gh run watch --exit-status, which can block for minutes. This blocks the workflow's event loop synchronously (the retry loop already uses async Bun.sleep, so the mix is a little inconsistent). For the watch/long-poll calls specifically, an async Bun.spawn would be friendlier to anything else the runtime is hosting.

Minor

4. Dead exports in the production path. firstActionsUrl and hasLeadingStatus are exported and unit-tested but unused by publish-release.ts (the workflow uses hasStatusMarker and pulls the run URL from JSON). Either wire them in or drop them so the public surface reflects what's actually used.

5. nullableStringField is identical to stringField (same "non-empty string or undefined" logic). Collapse to one helper unless a real distinction is intended.

6. selectPublishWorkflowRunJson bails on the first branch/event match even if it's malformed. If the first run matching headBranch+event === "push" is missing databaseId/status, it returns ok:false immediately instead of continuing to scan later (possibly valid) runs. Low-risk, but a continue would be more robust than an early return.

7. Spec/implementation drift. The RFC's mermaid graph and door-set names (prepare_release_branch, update_release_metadata, sync_main_after_merge, merge_verified_release_pr, …) don't match the implemented stage names (prepare-release-branch-and-metadata, wait-for-release-ci-and-merge, tag-and-monitor-publish). Not a code issue, but worth reconciling for readers cross-referencing the two.

Tests

Helper coverage is good. The pure orchestration helpers in publish-release.ts — excerpt, commandSummary, blockedOutput — have no coverage and are trivially testable (no side effects); adding a few cases (e.g. excerpt truncation boundary, commandSummary omitting empty stdout/stderr) would round out the suite without touching the side-effecting paths the spec rightly excludes.

Overall this is careful, security-conscious work — the merge/tag isolation and JSON-as-source-of-truth verification are the right instincts. Items 1 and 2 are the ones I'd most want resolved, since they're the difference between "fails safe" and "blocks after an irreversible step" / "guarantee stated but not enforced."

_{🤖 Automated review by Claude Opus 4.8}

[claude]

Review — feat(workflows): add automated publish-release workflow

Thorough, well-architected PR. The "GitHub API JSON is the source of truth, not LLM prose" design is the right call for an irreversible release pipeline, and isolating the two dangerous doors (merge, tag push) into dedicated stages with deterministic pre/post gates is excellent. Types are tight (no any/unknown, readonly throughout), and all commands use array-arg Bun.spawnSync with regex-validated version strings, so there's no shell-injection surface. Nice work.

Below are findings, roughly by severity.

🐞 Potential bug — merge gate is coupled to branch retention (verify repo setting)

verifyReleasePrMerged treats a missing remote branch as a hard failure, after the irreversible merge has already happened:

const branchCheck = runCommand(["git", "ls-remote", "--heads", "origin", release.branch]);
if (branchCheck.exitCode !== 0 || branchCheck.stdout.length === 0) {
  return { ok: false, /* "PR is merged, but the release branch was not found on origin" */ };
}

GitHub's repo-level "Automatically delete head branches" setting deletes the head branch server-side on merge regardless of what gh pr merge / the agent does. If that setting is on for bastani-inc/atomic, this gate will report failed on every run — immediately after the merge succeeded — which directly defeats the stated goal that "a formatting error … cannot block after a successful merge." (I couldn't confirm the repo's deleteBranchOnMerge setting from the sandbox.)

Suggestion: confirm the setting is off, and consider downgrading branch-retention from a hard gate to an informational note in the merge verification — the merge's success (state/mergedAt/mergeCommit.oid/refs) is the thing that actually gates tagging, not whether the branch still exists.

🧹 Dead code — status-marker & URL helpers are exported + tested but unused by the workflow

firstActionsUrl, cleanUrl, hasStatusMarker, hasLeadingStatus, and firstNonEmptyLine are fully unit-tested but not imported anywhere in publish-release.ts (the workflow now verifies everything via gh … --json). Because they're exported, noUnusedLocals won't catch them. They look like leftovers from an earlier status-marker design (the prompts even say "Do not use a PR_STATUS marker"). Either wire them in or remove them along with their tests to avoid implying they're load-bearing.

🧹 nullableStringField duplicates stringField

The two functions are byte-for-byte identical implementations; the nullable name implies special null handling that isn't there. Collapse to one, or if the distinction is intentional (e.g. conclusion can legitimately be JSON null), add a comment — right now a null conclusion and a missing conclusion are indistinguishable in the output.

⚠️ External-API assumption worth validating live

selectPublishWorkflowRunJson/verifyPublishWorkflowRunJson match the publish run by headBranch === release.version, relying on gh run list reporting the tag name in headBranch for tag-triggered runs. The test fixtures encode this assumption, but it's an external contract — if it doesn't hold, publish verification will always block (safe-fail, not dangerous, but it'd make the workflow unusable). Worth one real end-to-end run to confirm before relying on it.

Two smaller robustness notes in the same area:

• selectPublishWorkflowRunJson early-returns ok:false the moment the first headBranch/event match has a missing databaseId/status, instead of continuing to scan older matching runs. Edge case, safe-fail.
• verifyPublishWorkflowSucceeded retries selection only 6×10s (~60s) before giving up, and gh run watch then blocks synchronously with no timeout. After a tag push the run can take longer than 60s to register; a longer window / backoff would reduce spurious "run not found" blocks, and an outer bound on gh run watch would prevent a hung Actions run from blocking the workflow indefinitely.

📝 PR description out of sync with the implementation

The summary table lists 5 stages (run-release-checks, wait-for-release-ci-and-merge, tag-and-monitor-publish), but the code has 6 agent stages with different names (prepare-release-branch-and-metadata, open-release-pr, wait-for-release-ci, merge-verified-release-pr, sync-main-after-merge, push-release-tag). The spec (specs/…) is accurate — just the PR body's table is stale. Also note prompts reference AGENTS.md for changelog guidance; both AGENTS.md and CLAUDE.md exist, so this resolves, just flagging for intent.

✅ Things I checked that are correct

• releaseChangedFileAllowed allowlist (package.json, bun.lock, packages/*/{package.json,README.md,CHANGELOG.md}) exactly matches what scripts/bump-version.ts writes (it only touches packages/*/README.md badges, never root README) — no false-positive blocks.
• --workflow publish.yml matches .github/workflows/publish.yml.
• Lightweight-tag handling: verifyReleaseTagPublished uses version^{} and compares against ls-remote SHA, which is correct for the lightweight tag git tag <version> creates.
• verifyMainReadyForTag correctly requires local main == origin/main, clean tree, merge commit ancestry, and tag non-existence before the publish door.

Test coverage

Helper coverage is strong (validation, both PR verifications, checks, run selection/verification, status parsing). Gaps: the workflow-body orchestration functions (verifyReleasePreparation, runLocalReleaseChecks, verifyMainReadyForTag, verifyReleaseTagPublished, the merge/branch-retention gate) are untested — these are exactly the deterministic gates guarding irreversible side effects, so even a few fixture-driven tests there would be high-value. I was unable to run bun test/typecheck in this sandbox (commands required approval), so I relied on the PR's stated local pass.

Overall: solid, safety-conscious design. The branch-retention-vs-merge-gate interaction is the one item I'd want resolved before this drives a real publish; the rest are cleanups.

[claude]

Code Review — feat(workflows): add automated publish-release workflow

Thanks for this — it's a thorough, well-structured piece of work. The "let the model do flexible work, but gate every irreversible step on deterministic TypeScript checks reading GitHub API JSON (not LLM prose)" design is exactly right for a release pipeline, and isolating the two dangerous doors (PR merge, tag push) into dedicated stages makes the blast radius easy to reason about. The helper extraction + unit coverage is clean. Comments below are mostly polish; a couple are worth addressing before merge.

🔴 Worth addressing

1. Dead code: the status-marker / URL-extraction subsystem is tested but never used by the workflow.
hasStatusMarker, hasLeadingStatus, firstNonEmptyLine, firstActionsUrl, cleanUrl (and the private urlsIn / statusMarkerPattern) are imported only by test/unit/publish-release-helpers.test.ts — the workflow itself imports none of them, and every stage prompt explicitly instructs the model not to emit a status marker ("Do not use a PR_STATUS marker", "Do not rely on an exact merge status marker"). This looks like leftover machinery from an earlier design iteration. It passes tsc only because exported symbols are never flagged as unused. Recommend either wiring it in or deleting it (and its tests) so the maintenance surface matches what actually runs. Right now ~80 lines + a test suite verify behavior production never exercises.

2. checkPassed state fallback can treat a failed check as passing.
```ts
const state = stringField(value, "state")?.toUpperCase();
return state === "SUCCESS" || state === "PASSING" || state === "PASSED" || state === "COMPLETED";
```
A GitHub check can be state: "COMPLETED" with a failure conclusion — COMPLETED is a status, not an outcome. In practice this branch is dead because gh pr checks --json always populates bucket (and the bucket === "pass" path wins first), but if bucket is ever absent this silently passes failed checks. Since this guards a merge, I'd drop "COMPLETED" from the success set to keep the fallback fail-closed.

🟡 Minor / consider

3. selectPublishWorkflowRunJson early-returns on a malformed first match instead of skipping it.
Once a candidate matches headBranch+event === "push", a missing databaseId/status returns ok:false immediately rather than continuing to the next candidate. Realistically the matching run always has these fields, so this is robustness-only — but continue-ing past a malformed entry would be more resilient than failing the whole selection.

4. Tag-target verification assumes a lightweight tag.
verifyReleaseTagPublished compares git rev-parse <version>^{} and the ls-remote SHA directly against the commit OID. git tag <version> (what the prompt instructs) creates a lightweight tag, so this is correct today. If anyone ever switches to an annotated tag, the remote ls-remote SHA becomes the tag-object OID and this check would false-negative. A one-line comment noting the lightweight-tag assumption would save a future debugging session.

5. Empty required-check list fails closed — confirm that's intended.
verifyPullRequestChecksJson([]) returns ok:false ("contained no required checks"). That's the safe choice, but it means the workflow hard-blocks on any repo/branch without configured required checks. Worth a sentence in the spec so it's a documented decision rather than a surprise.

🟢 Positive notes / nits

• Good call using createGitEnvironment() for every subprocess — preserving process.env (PATH/GH_TOKEN) while stripping GIT_* local vars is the correct approach, and avoids the wrong-repo footgun.
• execFileSync with an args array (no shell) + regex-validated version means no shell/branch/tag injection surface. Nice.
• Fail-closed gating throughout (worktree-clean checks, ancestor checks, remote SHA match) is consistently applied.
• The unit tests for the pure helpers are precise and assert on exact summary strings — high signal.
• Bun compliance is respected (Bun.file().json(), Bun.sleep, bun run …); no build step added; purely additive. 👍

🧪 Test coverage

Pure helpers are well covered. The orchestration layer (verifyReleasePreparation, verifyMainReadyForTag, verifyReleaseTagPublished, verifyPublishWorkflowSucceeded, and the defineWorkflow body) is untested — understandable since it's bound to live git/gh side effects, but it's also where the real release risk lives. Consider extracting the pure decision logic from the I/O (e.g. a decideMainReadiness(commandResults) taking captured CommandResults) so the gate logic can be unit-tested without a live repo. Not a blocker.

📝 Doc nit

The PR description references lib/publish-release-helpers.ts, but the file is actually lib/publish-release.ts (the test is publish-release-helpers.test.ts). Minor, but worth aligning.

Overall: solid, safety-conscious workflow. Addressing the dead-code subsystem (#1) and the COMPLETED fallback (#2) would be my only pre-merge asks.

🤖 Generated with Claude Code

[claude]

PR Review: publish-release workflow

Thorough, well-architected piece of work. The "delegate wording to model stages, gate every irreversible step with deterministic TypeScript checks against GitHub API JSON" design is exactly the right shape for release automation, and the discriminated-union return types plus exhaustive failure aggregation make the verification code easy to audit. The pure helpers in lib/publish-release.ts are cleanly factored and the test file covers them well. Findings below, ordered by impact.

🔴 High — branch-retention check can block a release AFTER a successful (irreversible) merge. In verifyReleasePrMerged (.atomic/workflows/publish-release.ts), after confirming the PR is MERGED, the function runs git ls-remote --heads origin <release-branch> and returns ok:false if the branch is absent. If the repo has "Automatically delete head branches" enabled (a common GitHub setting), or any gh pr merge --delete-branch path is taken, the branch is gone even though the merge succeeded. The workflow then reports blocked and STOPS BEFORE TAGGING — leaving the release half-done (merged but never published) and requiring manual recovery of exactly the irreversible-adjacent step the design tries to protect. This contradicts the spec guarantee in section 7 ("a formatting error in an agent response cannot block after a successful merge") — here a non-formatting, environmental condition blocks after a successful merge. Branch retention is a nice-to-have, not a precondition for tagging: the real precondition ("merge commit is on main") is already enforced independently by verifyMainReadyForTag via git merge-base --is-ancestor. Recommend downgrading the missing-branch case to a non-fatal warning rather than an ok:false that halts the pipeline.

🟠 Medium — publish-run selection assumes headBranch === version for tag-triggered runs. selectPublishWorkflowRunJson matches a run only when headBranch === expectedHeadBranch (the version) AND event === "push". publish.yml is triggered by push:tags, so the run is tag-triggered. GitHub usually populates head_branch with the tag short name for tag pushes, but this is not guaranteed across all run states and has historically been null in some cases. If it comes back empty, selection never matches, the 6x10s retry loop exhausts, and the workflow reports publish failed even though publishing actually succeeded (this gate runs after the tag is pushed, so it can only mislabel a real success). Worth confirming against a real tag run, or additionally matching on headSha === tagTargetOid (already threaded through as expectedHeadSha) as a more robust selector.

🟠 Medium — required-check gate treats skipped/neutral checks as failures. checkPassed (lib/publish-release.ts) only accepts bucket === "pass" (or state SUCCESS/PASSING/PASSED). gh pr checks --required buckets also include "skipping" and "cancel". A required check that is legitimately skipped (e.g. path-filtered jobs that GitHub still counts as satisfying branch protection) is bucketed "skipping" and treated as a failure here, blocking a merge GitHub itself would allow. If intentional, a one-line comment would help; otherwise consider accepting skipping/neutral as non-blocking.

🟡 Low — test coverage stops at the module boundary. The 926-line workflow body holds the highest-stakes deterministic logic — releaseChangedFileAllowed (the changed-file allowlist), the package-manifest private/version invariants, verifyMainReadyForTag, verifyReleaseTagPublished, and the merge/branch logic above — yet none of it is unit-tested, because those functions call runCommand directly rather than accepting injected command results the way the well-tested lib verifiers accept JSON. The allowlist regex and manifest invariants are pure and security-relevant; extracting them (or the "given these CommandResults, produce failures[]" core) into lib would let them be tested in the established style.

🟡 Low — nullableStringField is byte-for-byte identical to stringField. In lib/publish-release.ts both return typeof value === "string" && value.length > 0 ? value : undefined. The name implies null-aware handling that does not exist. Either collapse to one function or give nullableStringField the distinct behavior its name promises.

🟡 Low — PR description and spec drift from the final code. The PR body and specs/...md section 5.1 reference helpers (hasStatusMarker, hasLeadingStatus, firstActionsUrl) and "status parsing" tests that do not exist in the final lib or test file — the status-marker approach was replaced by deterministic gates over the commit history. The spec door names (wait_for_release_ci_and_merge) also differ from the implemented split stages (wait-for-release-ci + merge-verified-release-pr, the better design). Worth reconciling so the RFC matches what shipped.

✅ Things done well

• No shell-injection surface: runCommand uses execFileSync with an argv array (no shell), and target_version is validated against a strict anchored regex before reaching any subprocess.
• Git env sanitization: routing every subprocess through createGitEnvironment() to strip inherited GIT_DIR/GIT_WORK_TREE is a thoughtful guard against hook/worktree context leaking in.
• Bun compliance throughout; no stray npm/node usage, no build step added.
• Captured-SHA guards (expectedHeadRefOid threaded reference -> checks -> merge verification) meaningfully reduce TOCTOU risk between the CI gate and the merge.
• @bastani/atomic resolves to source via the root tsconfig paths, so typecheck (pre-build) and bun test both work without dist, and CI builds before unit tests — no issue there.

Nice work overall — the High finding is the one I would want addressed before this is trusted to drive a real release unattended.

🤖 Generated with Claude Code

[claude]

PR Review: automated publish-release workflow

Thorough, well-architected addition. The core design — model stages handle open-ended work (changelog wording, PR body, CI-log reading) while deterministic TypeScript gates verify every irreversible step (PR merge, tag push, publish run) directly against the GitHub API via gh … --json — is exactly the right pattern for an automated release pipeline. Strong typing throughout (discriminated unions on every verification result, readonly everywhere), no shell injection (execFileSync with arg arrays, strict version-regex validation before any side effect), and the helper library is cleanly unit-tested.

I verified the integration assumptions against the repo: all companion packages are "private": true, createGitEnvironment is exported from @bastani/atomic, AGENTS.md symlinks to CLAUDE.md (so the prompt references resolve), and publish.yml exists, is named Publish, and triggers on push: tags. ctx.task().text matches the WorkflowTaskResult contract. The wiring holds.

A few issues worth addressing:

Medium

1. Branch-retention check can falsely report failure AFTER a successful merge.
verifyReleasePrMerged (.atomic/workflows/publish-release.ts:400-412) fails the whole step when the remote release branch no longer exists on origin after merge (branchCheck.stdout.length === 0). If this repo has "Automatically delete head branches" enabled, GitHub deletes the head branch on merge regardless of the model instruction not to — so a fully successful merge would be reported as blocked. This gate fires AFTER the irreversible merge, so a false negative here is misleading. CLAUDE.md does not require branch retention. Consider making retention a soft/warning signal rather than a hard gate (or gate only on merge state + commit OID, which are the real correctness signals).

Low

2. PR description references helpers that are not in the diff.
The description lists hasStatusMarker / hasLeadingStatus and claims status-marker test coverage, but neither the helper lib nor the test file contains them. The stage prompts deliberately instruct NOT to use status markers, so the removal looks intentional — just sync the PR body/spec with the code.

3. nullableStringField is identical to stringField.
lib/publish-release.ts:197-210 — both return string | undefined for non-empty strings, and stringField already returns undefined for JSON null, so the nullable variant adds nothing. Collapse to one.

4. "Newest run" selection is positional, not sorted.
selectPublishWorkflowRunJson returns the first array element matching branch+event and requests createdAt but never uses it. It relies on gh run list returning newest-first (true today); the test is even named "selects the newest push run" though nothing sorts. If two push runs ever share the tag name, array order is the only tiebreak. Either sort by createdAt to make the guarantee real, or drop the unused field.

5. Publish-run discovery window is ~60s.
verifyPublishWorkflowSucceeded retries 6x with 10s sleeps before giving up locating the run. Under Actions queue latency a tag-triggered run can take longer than 60s to register, surfacing as "run not found" even though the tag (already pushed, irreversible) is fine and the run is merely pending. Consider a longer window / backoff so transient queue delay is not reported as failed.

6. checkPassed treats any non-pass bucket as failing.
lib/publish-release.ts:336-342 — buckets like skipping / neutral / cancel count as failures. With --required this is probably correct, but worth confirming no required check ever legitimately concludes neutral, or merge will be blocked.

Informational

• The workflow assumes its CWD is the repo root (existsSync("package.json"), relative git/gh calls). Fine for a project-local workflow invoked from the repo root, but there is no explicit guard if invoked from a subdirectory.
• Nice touch: even though --match-head-commit is only suggested to the model, verifyReleasePrMerged re-checks headRefOid, so a merge of the wrong SHA is still caught deterministically.

Tests

Helper coverage is solid (version validation, PR reference/merge/checks, run selection/verification, plus negative cases). The orchestration in publish-release.ts (gates, blocked-output paths, retry loop) is necessarily untested given its gh/git side effects — a reasonable boundary, and extracting the pure logic into the tested lib was the right call.

Overall: safe-by-construction work with the right verification architecture. Addressing #1 (and tidying #2-#4) would make it production-ready.

🤖 Generated with Claude Code

[claude]

Review: feat(workflows): add automated publish-release workflow

Thorough, well-architected PR. The core design — deterministic, code-enforced gates after each model stage, with irreversible remote effects funneled through named chokepoints — is exactly right for a release pipeline, and the typed result unions + gh … --json verification (rather than trusting LLM prose) are a real strength. Verified the changed-file allowlist in verifyReleasePreparation against scripts/bump-version.ts: it correctly covers everything that script writes (root + packages/*/package.json, packages/*/README.md badges) plus changelogs and bun.lock. Good.

A few issues worth addressing, roughly by severity.

—— Medium ——

1. Branch auto-delete will produce a false blocked after a successful merge.
verifyReleasePrMerged (.atomic/workflows/publish-release.ts:400-412) hard-fails if the remote release branch is gone (git ls-remote --heads origin <branch> empty → fail). If the repo/org has "Automatically delete head branches" enabled, GitHub deletes the branch on merge, and this gate reports blocked even though the PR merged and tag/publish would have succeeded. The merge itself is already proven by state: MERGED + mergeCommit.oid. Recommend demoting branch retention to informational rather than a hard precondition for the irreversible tag step.

2. Skipped required checks are treated as failures.
checkPassed (lib/publish-release.ts) only accepts bucket === "pass". gh pr checks also emits skipping/neutral/cancel buckets. A required check that is conditionally skipped (path filters, if: conditions) lands in skipping and blocks the merge gate. Consider accepting skipping/neutral as non-blocking, or documenting that every required check must positively report pass.

—— Low ——

3. Zero required checks → permanent block. verifyPullRequestChecksJson returns ok: false on an empty array. If branch protection / required checks aren't configured on main, the workflow can never merge. Defensible safety stance, but surprising — worth calling out in the spec and making the error message hint at the cause.

4. Blocking subprocesses with no timeout. runCommand uses execFileSync, and gh run watch … (publish-release.ts:538) plus bun run test:unit block synchronously. A hung CI/publish run will hang the workflow indefinitely. Consider a timeout on execFileSync for the watch/long-poll calls.

5. nullableStringField is identical to stringField. Same body; the name implies null-aware handling it doesn't actually do. Collapse to one helper or document why both exist.

6. selectPublishWorkflowRunJson selection robustness. Returns on the first candidate matching headBranch/event, relying on gh run list's implicit newest-first ordering (createdAt is fetched but unused as a tiebreak). Also early-returns ok: false if that first match is malformed instead of continuing to scan. An explicit sort by createdAt desc would make selection deterministic.

—— Nits ——

• Duplicate isJsonObject defined in both publish-release.ts:102 and lib/publish-release.ts. Import from the lib.
• Test coverage gap: pure helpers releaseChangedFileAllowed and packageManifestPaths are easily unit-testable but uncovered. The JSON-verification helpers are nicely tested (257 lines) — extending coverage to these two would close the gap on the security-relevant allowlist logic.

—— Checked and fine ——

• No shell injection: execFileSync with arg arrays, and target_version is regex-validated before any interpolation into branches/tags/prompts.
• No secret leakage: runCommand gh calls only request --json fields.
• Git env sanitization via createGitEnvironment() (exported correctly from @bastani/atomic src/index.ts:481).
• AGENTS.md references in prompts resolve correctly — AGENTS.md is a symlink to CLAUDE.md.
• default export added to package.json is correctly ordered last and matches the changelog entry.

Nice work overall — the medium items (especially #1) are worth fixing before this is relied on for a real release, since they can halt an otherwise-successful run.

Reviewed by Claude (Opus 4.8).

[claude]

Code Review — feat(workflows): add automated publish-release workflow

Reviewed the full diff against CLAUDE.md/AGENTS.md conventions and the actual repo state (scripts/bump-version.ts, package manifests, .github/workflows/publish.yml, @bastani/workflows authoring contract). This is a strong, carefully-engineered PR — the "deterministic gate after every model stage" architecture is exactly right for an irreversible release pipeline. Comments below, ordered by impact.

What is done well

• Security posture is excellent. runCommand uses execFileSync with an argv array (no shell), so the validated version cannot inject. Version is validated against strict regexes before any side effect, and createGitEnvironment() sanitizes inherited git env.
• Defense-in-depth on the head SHA — the same commit is verified across the PR reference, the merge, and the publish run. The deterministic gates re-fetch from the GitHub API rather than trusting model prose.
• Preparation allowlist matches reality. releaseChangedFileAllowed plus the per-manifest checks (version === target, coding-agent must be @bastani/atomic, all other packages/* must be private: true) line up exactly with what scripts/bump-version.ts actually mutates and the current manifests.
• Helper library is well-typed (discriminated unions on every verification result) and the unit tests cover the pure helpers thoroughly.

Issues

1. (Medium–High) The post-merge branch-retention check can falsely block a successful release. verifyReleasePrMerged returns ok: false if git ls-remote --heads origin <branch> is empty after the merge ("The PR is merged, but the release branch was not found on origin."). If the repo has GitHub's "Automatically delete head branches" setting enabled (very common), the branch is deleted on merge regardless of the "Do not delete the release branch" instruction given to the model stage — the repo setting wins. The result: a release that actually merged successfully is reported blocked and the workflow halts before tagging/publishing. Critically, no later stage needs the branch — sync-main/push-tag operate on origin/main and the merge commit, not the branch. This gate adds a failure mode with no functional upside. Recommend dropping the retention requirement (or making it advisory/logged, not a gate).

2. (Low–Medium) checkPassed treats skipped/neutral required checks as failures. gh pr checks buckets are pass|fail|pending|skipping|cancel; only bucket === "pass" is accepted. A required check that legitimately resolves to skipping/neutral (conditional job, path filter) would block the merge gate. Safe-but-strict — worth documenting the assumption or explicitly allowing skipping.

3. (Low) Empty required-checks list is treated as failure. verifyPullRequestChecksJson([]) returns ok: false ("no required checks"). This couples the workflow to at least one branch-protection required check existing on main. Fine for this repo today, but an implicit precondition worth a comment.

4. (Low) Performance/responsiveness: synchronous execFileSync blocks the event loop. bun run test:unit, gh run watch --exit-status, and git pull are multi-minute, fully blocking calls. Nothing else (heartbeats, cancellation) can progress while they run. Consider an async spawn for the long-running commands.

5. (Low) Publish-run discovery window is short. verifyPublishWorkflowSucceeded retries selection 6x with 10s sleeps (~60s) for the tag-push run to register in gh run list. API registration after a tag push can lag beyond that under load; consider a longer ceiling or backoff.

Nits / cleanups

• nullableStringField is byte-for-byte identical to stringField (both map null/empty to undefined). The nullable-conclusion case is already handled by stringField. Collapse to one function.
• isJsonObject is duplicated in lib/publish-release.ts and publish-release.ts. Export it from the lib and reuse.
• selectPublishWorkflowRunJson matches headBranch === version for a tag push. This is correct (GitHub sets head_branch to the tag short-name for tag-triggered runs), but it is a non-obvious assumption — a one-line comment would prevent a silent break if the publish trigger ever changes. It also relies on gh run list returning newest-first for "first match = newest" to hold.
• PR description is out of sync with the code. It documents hasStatusMarker/hasLeadingStatus helpers and "status parsing" tests that are not in the diff, and describes "five stages" while the body actually issues seven model ctx.task calls. Please reconcile the description with what shipped.
• packages/coding-agent/package.json export: placing "default" last (after "import") is the correct order. Note it now also resolves bare require() to the ESM dist/index.js; if that entry is ESM-only, CJS consumers would hit a runtime error instead of a resolution error. Almost certainly fine here — just confirm no CJS consumers of the root export.

Test coverage

Helper-level coverage is solid (validation, PR-reference/merge/checks/run verification, edge cases). As expected, the runCommand-backed orchestration functions are untested — reasonable since they shell out, but the branch-retention predicate in issue 1 is exactly the kind of logic worth extracting and unit-testing with a "merged + branch deleted" fixture.

Overall: approve-with-changes. Issue 1 is the one I would want addressed before this is relied on for a real release, since it can mask a successful publish path as a blocked one.

🤖 Generated with Claude Code