Skip to content

test(release): locate the signing keychain steps by what they run - #5353

Merged
lidge-jun merged 1 commit into
devfrom
codex/260921-widget-signing-test-repair
Sep 20, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/260921-widget-signing-test-repair

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Summary

dev is red on tests/ci-workflows/release-desktop-scripts.test.ts:

widget extension signing > the certificate is importable before the widget is signed and is removed afterwards
  expect(importStep).toBeGreaterThanOrEqual(0)
  Expected: >= 0   Received: -1

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 under Import the release signing certificate, with a stricter body — a full credential-set preflight, a trap that 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-keychain and security 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 trap dev'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 at 07e2ac9b41 is 10 pass, 1 fail, which is the failure above.
  • No source or workflow change; this is a test-only repair of a test that was pinning the wrong thing.

Checklist

  • Targets dev
  • Fixes a red check on dev rather than adding scope
  • Verified locally against both the broken and the repaired tree
  • No workflow, credential, or signing behaviour is touched

Summary by CodeRabbit

  • Tests
    • Improved widget-signing workflow test reliability by locating certificate import and cleanup steps through their executed commands.
    • Updated cleanup validation to confirm certificate removal is registered through a shell trap.

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.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 20, 2026 16:12
@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

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: 94e011bf-866d-422f-9810-09709dd7d7dd

📥 Commits

Reviewing files that changed from the base of the PR and between 07e2ac9 and 09fe56a.

📒 Files selected for processing (1)
  • tests/ci-workflows/release-desktop-scripts.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 tests now locate certificate steps by command fragments. The certificate cleanup assertion now checks for a trap referencing $certificate.

Changes

Release workflow tests

Layer / File(s) Summary
Step matching and cleanup assertions
tests/ci-workflows/release-desktop-scripts.test.ts:223-228, tests/ci-workflows/release-desktop-scripts.test.ts:242-251
The test adds indexOfStepRunning to find steps by run content. The certificate-import test uses security create-keychain and security delete-keychain, retains the always() check, and verifies trap-based cleanup for $certificate.

Priority: ⬇️ Low

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

Change: Bug fix

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main test change: locating release signing keychain steps by their executed commands instead of their step titles.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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 github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment on lines +250 to +251
expect(steps[importStep]?.run).toContain("trap ");
expect(steps[importStep]?.run).toContain("$certificate");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-20T16:14:43.227115Z 09fe56a PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 58 / 80

이 PR은 dev에서 빨간 테스트 하나만 고칩니다. 위젯 서명 전에 인증서를 키체인에 넣고, 끝난 뒤 지우는 흐름이 아직 있는지는 맞는데, 테스트가 찾던 단계 이름이 틀려졌습니다. #5345가 쓰려던 이름 Import the Apple signing certificate for the widget을 찾았고, #5339가 먼저 넣은 실제 이름은 Import the release signing certificate입니다. 머지 충돌을 풀 때 워크플로는 dev 쪽을 잘 남겼는데, 테스트 글자만 예전 이름에 묶여 indexOfStep이 -1을 냈습니다. 이번 고침은 워크플로를 건드리지 않고, 테스트를 security create-keychain / security delete-keychain이 들어 있는 단계를 찾게 바꿉니다. 이름만 바꿔도 다시 안 깨지게 하려는 방향입니다. p12를 지우는 검사도 단순 rm 글자 대신, import 단계의 trap$certificate를 보게 바꿨습니다. base는 dev이고, 파일은 tests/ci-workflows/release-desktop-scripts.test.ts 하나뿐입니다. 같은 주제의 다른 열린 PR은 보이지 않습니다. types/config 분할과도 무관합니다.

라인 indexOfStepRunning / trap 검사 - trap $certificate를 따로 toContain합니다. 같은 단계 안에 서로 다른 줄에 trap과 $certificate가 있으면, trap이 인증서를 안 지워도 통과할 수 있습니다. 지금 release.yml의 import 단계는 trap '...$certificate...' 한 줄이라 맞지만, 나중에 갈라지면 테스트가 못 잡습니다. 가능하면 trap 한 줄 안에 $certificate가 같이 있는지로 묶으세요.

라인 cleanup 찾기 - security delete-keychain이 있는 첫 단계를 잡고 always()만 봅니다. 지금 워크플로에는 그 명령이 한 번뿐이라 괜찮습니다. 다만 위젯 빌드보다 뒤에 있는지는 검사하지 않습니다. import → build 순서만 있고, cleanup이 build 뒤인지는 안 봅니다. 이번 범위에서는 알아 두면 됩니다.

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

trap과 $certificate를 한 검사로 묶는 작은 보강을 이 PR에 넣을지, 초록만 확인하고 바로 머지할지. 동작 고침 자체는 이미 맞습니다.

너의 추천

dev를 다시 초록으로 돌리는 고침이라 머지해도 됩니다. 워크플로는 그대로 두고 테스트만 고친 선택이 맞습니다. 여력이 있으면 trap 줄을 $certificate와 한 번에 묶는 검사만 더하고, exact-head CI가 초록이면 승인하세요.

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

@lidge-jun
lidge-jun merged commit 64b0eca into dev Sep 20, 2026
32 checks passed
@lidge-jun
lidge-jun deleted the codex/260921-widget-signing-test-repair branch September 20, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant