test(release): locate the signing keychain steps by what they run - #5353
Conversation
dev is red on tests/ci-workflows/release-desktop-scripts.test.ts. The case locating the certificate import keyed on the step name "Import the Apple signing certificate for the widget", which is the name #5345 proposed. #5339 landed the same import first under the name "Import the release signing certificate", and the conflict resolution that merged #5345 correctly kept dev's workflow and dev's stricter import while carrying #5345's test text forward. The subject of the assertion is still present and still correct; only the label it searched for is gone, so indexOfStep returned -1. Locate the import and the cleanup by the codesign keychain commands they run instead of by their titles, which is what the case actually cares about and what a rename cannot silently take away. The p12 assertion moves to the trap dev's step installs, which is stronger than the plain deletion it replaces because it also fires when a later command in the step fails.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe release workflow tests now locate certificate steps by command fragments. The certificate cleanup assertion now checks for a trap referencing ChangesRelease workflow tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09fe56a40c
ℹ️ 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".
| expect(steps[importStep]?.run).toContain("trap "); | ||
| expect(steps[importStep]?.run).toContain("$certificate"); |
There was a problem hiding this comment.
Ensure the trap actually deletes the certificate
These independent substring checks do not prove that the trap removes the decoded credential: $certificate already appears in the assignment, decode, and import commands, so changing the trap to trap 'true' EXIT or using it only for unrelated cleanup would still pass while leaving the .p12 on disk. Match the trap body itself and require it to invoke shred or rm on $certificate, preserving the intended release-secret regression guard.
AGENTS.md reference: AGENTS.md:L420-L426
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 58 / 80이 PR은 라인 라인 cleanup 찾기 - 메인테이너의 판단이 필요한 지점 trap과 너의 추천
이 댓글은 grok-bot이 작성했습니다 |
Summary
devis red ontests/ci-workflows/release-desktop-scripts.test.ts:The case located the certificate import by step name. #5345 proposed
Import the Apple signing certificate for the widget; #5339 had already landed the same import underImport the release signing certificate, with a stricter body — a full credential-set preflight, atrapthat shreds the decoded p12 on any exit, and a cleanup guarded on the keychain actually existing. The conflict resolution that merged #5345 kept dev's workflow and dev's import, which was the right call, and carried #5345's test text forward unchanged. The subject of the assertion is still there and still correct; only the label it searched for is gone.Nothing about the workflow needs to change. The test does.
The import and the cleanup are now located by the keychain commands they run —
security create-keychainandsecurity delete-keychain— rather than by their titles. That is what the case actually cares about: the import has to precede the widget build, because codesign resolves an identity through the keychain search list and Tauri does not create its own keychain until the bundling step. A rename cannot silently take that away again.The p12 assertion moves to the
trapdev's step installs, which is a stronger claim than the plain deletion it replaces because it also fires when a later command in the step fails.Verification
bun test tests/ci-workflows/release-desktop-scripts.test.ts— 11 pass, 0 fail on this branch. The same file at07e2ac9b41is 10 pass, 1 fail, which is the failure above.Checklist
devdevrather than adding scopeSummary by CodeRabbit