Skip to content

fix(release): fail closed when draft state lookup fails - #5763

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:fix/release-draft-fail-closed-628
Sep 25, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:fix/release-draft-fail-closed-628

Conversation

@luvs01

@luvs01 luvs01 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fail closed when the draft-state lookup fails during release publication. Assigning gh release view --json isDraft --jq .isDraft to draft_state outside a conditional preserves its failing exit status under set -euo pipefail, rather than silently leaving the release as a draft.
  • After a successful asset upload and lookup, only true publishes the draft and only false completes without an edit. Empty, null, malformed or otherwise unexpected output fails the step. The original production workflow correction is unchanged by the latest follow-up.
  • Follow-up f1690aae69b1fd16a5ec27fbd351aaadd0e903e7 changes only the existing release contract test file. It separates static contracts from executable Bash checks, discovers native Git Bash on Windows without invoking the System32 WSL launcher, and adds success, command-order and failure-path coverage.
  • Shell tests use an isolated temporary working directory, an allowlisted environment without inherited credentials or shell startup hooks, a bounded subprocess, and a mock for exactly the expected gh operations. No real release upload or publication is performed.
  • A Windows developer machine without Bash explicitly skips only executable shell cases; all static contracts remain active. A separate guard requires Bash on CI and POSIX hosts, so missing shell coverage cannot silently produce a green CI result.

Verification

Latest follow-up: f1690aae69b1fd16a5ec27fbd351aaadd0e903e7, applied as a non-force fast-forward from 236aa05951a19b50e91125057daeb56d0b5887fd.

Native Bun 1.4.0 verification of this exact commit:

https://github.com/luvs01/opencodex/actions/runs/36090520098

All three read-only hosted jobs checked out f1690aa directly, not the helper workflow commit:

Environment Focused result
Linux 21 passed, 0 failed, no skipped tests; 154 assertions
macOS 21 passed, 0 failed, no skipped tests; 154 assertions
Windows (launched from PowerShell, native Git Bash discovered by the test) 21 passed, 0 failed, no skipped tests; 154 assertions
bun install --frozen-lockfile
bun test tests/ci-workflows/release-pipeline-contract.test.ts

The 21 tests comprise the nine existing static contracts, one shell-availability guard and eleven shell cases. They cover lookup failure even with true on stdout; six unexpected values including empty/null/multiline output; normal draft publication; the no-edit path after an explicit false; upload failure stopping before lookup; and edit failure preserving the failing result. Exact command order/arguments and whether execution reaches completion are asserted, not only exit codes.

Also passed on Linux:

bun run typecheck
bun run privacy:scan
bun run structure:check

git diff --exit-code passed after validation on all three hosts. The uploaded test blob matches the locally prepared source (2296c027de35e4e7bbedce1cdf61029e905adef2). The isolated helper workflow is a later commit on a separate validation branch; it is absent from this PR's source tree and commit ancestry.

Full repository tests and test:changed were not run in the isolated helper: this follow-up is one test file exercising a bounded release-shell change, and the local container has neither Bun nor network access. The relevant source-as-data test was run explicitly on all three platforms; the full repository matrix remains the upstream PR CI's responsibility. This is not an end-to-end production release run and does not establish that external GitHub operations will succeed.

The previous head's cancelled CI is not passing evidence. On the new head, Service lifecycle and React Doctor have passed; Cross-platform CI is still in progress at this update. Required current-head checks and maintainer review remain necessary before merge.

Security scope review: this follow-up introduces no production trigger, permission, publication destination or credential change. The existing workflow correction preserves verified-bundle/receipt ordering and fails on unknown publication state. The test subprocess receives no GitHub token and mocks gh. No release workflow was dispatched, no release/tag was changed and no merge was performed. Independent maintainer review of the release workflow is still required.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
  • Focused native Bun tests passed on Linux, macOS and Windows for the exact follow-up commit.
  • Project typecheck, privacy scan and structure checks passed.
  • All required current-head PR CI has completed successfully and maintainer review is complete.

Summary by CodeRabbit

  • Bug Fixes
    • Release publishing now stops when the release status cannot be retrieved or is unexpected, rather than treating an unknown status as already public. Draft releases are published after a successful upload and status check; already-public releases are left unchanged. Errors during upload, status checks, or publishing are reported instead of being silently ignored.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions github-actions Bot added the bug Something isn't working label Sep 24, 2026
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 15b9fc08-0c39-4465-9a8f-416a714e16e0

📥 Commits

Reviewing files that changed from the base of the PR and between 236aa05 and f1690aa.

📒 Files selected for processing (1)
  • tests/ci-workflows/release-pipeline-contract.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The release workflow checks the output of gh release view and publishes a release only when its draft state is exactly true. It leaves releases with state false unchanged and exits with an error for other output. Contract tests run the workflow script with a mocked gh command.

Changes

Release draft-state check

Layer / File(s) Summary
Draft-state check and failure handling
.github/workflows/release.yml, tests/ci-workflows/release-pipeline-contract.test.ts
The workflow publishes draft releases and rejects unexpected draft-state output. Bash-based contract tests check upload, lookup, and edit failures; exact true and false output; and draft versus public release behavior. The tests discover and probe Bash, and skip execution when it is unavailable.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f1690

The release step now fails on lookup errors and unexpected draft states. No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f1690

The change makes release publication stop when draft state cannot be confirmed. The existing upload-before-publication sequence remains intact, and no new security exposure was identified. Live GitHub behavior was not verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed control affects the GitHub release selected by a dispatched release run, not a new application-facing entrypoint. The functions marked as public entrypoints in the tests are local shell-test helpers.

Trust Boundaries and Controls

  • observed — The workflow checks draft state through the GitHub CLI before using its token to make the release public. A failed lookup or response other than exact true or false cannot pass that check; only true reaches the edit.

Resilience and Maintainability Implications

  • observed — The tests show upload failure stops before lookup, lookup failure stops before edit, and edit failure leaves the step failed. They mock GitHub CLI outcomes rather than exercising the live provider.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: release publication now fails when the draft-state lookup fails.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

이 풀리퀘스트는 바탕이 dev예요. 검증된 파일을 릴리스에 붙인 다음, 그 릴리스를 초안에서 공개로 바꾸는 단계예요.

예전 문장은 초안인지 묻는 명령을 if 안의 $(...)에 넣었어요. 그 자리에서 명령이 실패해도 셸은 멈추지 않아요. 실패하면 결과가 빈 글자가 되고, 빈 글자는 true와 달라서 공개로 바꾸는 줄을 건너뛰어요. 단계는 성공으로 끝나요. 파일은 올라갔는데 릴리스는 초안으로 남아요.

이제는 조회 결과를 draft_state에 먼저 넣어요. 이 스크립트 맨 위에는 이미 set -euo pipefail이 있어요 (.github/workflows/release.yml 682행). 조회가 실패하면 그 줄에서 단계가 실패로 끝나요. bash 5.2에서 옛 문장은 종료 0으로 끝나고, 새 문장은 종료 41로 멈춰요.

attach-release는 ubuntu-latest에서 돌아요 (630행). 테스트는 조회가 41로 실패하면 스크립트도 41로 끝나는지 봐요. 풀리퀘스트 CI의 테스트 샤드는 우분투라 bash가 있어요. 작성자가 본 한 건의 실패는 bash가 없는 윈도우 로컬이에요.

types.ts와 config.ts는 안 건드려요. 같은 고침을 한 다른 열린 글은 없어요.

라인 - .github/workflows/release.yml 693행 — 조회가 종료 0으로 끝났는데 글자가 true가 아니면, 공개로 바꾸는 명령을 건너뛰고 단계가 성공해요. 빈 글자와 null도 그래요. gh는 릴리스를 못 찾거나 인증이 안 되면 0이 아닌 코드로 끝나요. 그 실패는 이제 단계를 멈춰요. 남은 구멍은 종료 0인데 글자가 true도 false도 아닌 경우예요.

라인 - tests/ci-workflows/release-pipeline-contract.test.ts 171–181행 — 새 테스트는 조회가 41로 실패하는 경우만 봐요. 결과가 false라서 공개 전환을 건너뛰는 경우와, 결과가 빈 글자인 경우는 안 봐요.

메인테이너의 판단이 필요한 지점

조회가 성공했는데 값이 true도 false도 아니면 단계를 실패로 볼지 정해 주세요. false일 때만 건너뛰고 나머지 값은 실패로 두면, 빈 결과로 초안이 조용히 남는 경우까지 막아요.

파일 업로드는 조회보다 먼저예요 (684행). 조회가 실패하면 파일은 초안 릴리스에 붙어 있고, 단계만 실패해요. 다시 돌리면 --clobber로 다시 올리고, 조회가 되면 공개로 바꿔요.

너의 추천

방향은 맞아요. 바탕 dev도 맞아요. 닫을 중복 글은 없어요. 이대로 넣어도 돼요.

693행에서 false만 통과시키고 다른 값은 실패로 두면 빈 결과까지 막아요. 조회 명령이 실패했는데 단계가 초록으로 끝나는 버그는 지금 코드와 테스트로 막혀요.

이 댓글은 grok-bot이 작성했습니다

A successful draft-state query that answers neither true nor false — an
empty body or an unexpected shape — used to skip the publish step and
leave the release a silent draft. Only an explicit false may pass now;
anything else fails the step. The contract test covers the unexpected
value alongside the existing failing-query case.
@luvs01

luvs01 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Applied your suggestion: only an explicit "false" passes now — a successful query returning anything else (empty body, unexpected shape) fails the step instead of leaving a silent draft. The contract test covers the unexpected-value case alongside the failing-query case.

@luvs01

luvs01 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

리뷰 반영 확인: 236aa05951에서 693행을 case로 바꿔 false만 통과시키고, true는 draft 해제, 그 외 값(빈 결과·예상 밖 형태)은 exit 1로 실패 처리합니다. 조회 명령 실패 시 단계가 초록으로 끝나는 경로도 e06cad077c에서 이미 fail-closed로 막혀 있습니다. (로컬 테스트 1건 실패는 bash 부재의 환경 문제이며 이 변경과 무관합니다.)

…ash regressions

Keep the existing release workflow fix unchanged. Separate static contracts
from shell execution, find native Git Bash on Windows, and refuse silently
skipped shell coverage on CI. Mock only the expected gh commands in an empty
working directory with no inherited credentials or executable search path.
Assert success paths, command order, six malformed states, and upload/view/edit
failure propagation. Add subprocess time and output bounds.

Local preliminary validation: TypeScript syntax and 12 checks through a Node
adapter invoking actual Bash passed; restoring the original bug or making the
script always fail was detected. Native Bun cross-platform validation pending.

luvs01 commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up applied in f1690aa as a non-force fast-forward.

The production fail-closed correction already addressed the earlier review; this follow-up leaves that workflow unchanged and repairs the verification gap in the existing test file:

  • Discover native Git Bash on Windows instead of relying solely on bash being on PATH, and exclude the System32 WSL launcher. Static contracts remain runnable on a Windows workstation without Bash; a dedicated guard fails CI/POSIX hosts rather than silently skipping shell coverage.
  • Execute the actual YAML attachment block against an allowlisted gh mock in a fresh temporary directory, with no inherited credentials/startup hooks and with time/output bounds.
  • Assert successful draft publication, explicit-public no-edit behavior after upload, exact call ordering, six unexpected status values, and upload/view/edit failure propagation. These success assertions also prevent an unconditional failure from looking like a valid fix.

Exact-commit native validation: https://github.com/luvs01/opencodex/actions/runs/36090520098

Linux, macOS and Windows each ran the same 21 tests: 21 passed, 0 failed, no skipped tests, 154 assertions with project-pinned Bun 1.4.0. Project typecheck, privacy scan and structure checks passed on Linux. All three hosts checked out f1690aa directly and verified no tracked source changed. The read-only helper workflow is on a separate later validation commit and is not included in this PR or its ancestry.

The PR description now records these results instead of the old 8-pass/1-fail Windows result. Full repository CI remains separate: Service lifecycle and React Doctor passed on the new head; Cross-platform CI is still running. No real release command, release/tag mutation, paid review setting change, or merge was performed.

@Ingwannu @lidge-jun Please review the updated head once the required checks settle. Existing reviewer requests are preserved.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact head f1690aae69b1fd16a5ec27fbd351aaadd0e903e7.

Moving the draft-state lookup out of the if condition restores Bash fail-fast behavior when gh release view fails, and the explicit three-way case permits publication only from a literal true while accepting an already-public literal false idempotently.

Focused validation under a 2-CPU / 4 GiB cgroup passed: 21 tests, 0 failures. The portable shell harness covers lookup/upload/edit failures, malformed successful output, draft publication exactly once, and the already-public path. I found no remaining blocker.

@lidge-jun
lidge-jun merged commit 1846fe6 into lidge-jun:dev Sep 25, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants