Skip to content

ci: notarize Commander Developer ID releases - #400

Open
lopugit wants to merge 18 commits into
github-actionsfrom
codex/commander-release-developer-id
Open

ci: notarize Commander Developer ID releases#400
lopugit wants to merge 18 commits into
github-actionsfrom
codex/commander-release-developer-id

Conversation

@lopugit

@lopugit lopugit commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • import the existing Developer ID certificate into an ephemeral macOS runner keychain
  • require Developer ID distribution signing for Commander release builds
  • notarize, staple, and Gatekeeper-assess the bundle before publishing

Validation

  • YAML parsed with Ruby
  • git diff --check
  • release flow mirrors the existing Electron certificate/notarization secret contract

Dependency

Requires the Commander signing-policy change in #263 so the build script enforces the same release boundary.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

⚠️ The resolver stopped in a later safety, verification, or publication step. No residual conflicted paths were confirmed; see the workflow run.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

⚠️ The resolver stopped in a later safety, verification, or publication step. No residual conflicted paths were confirmed; see the workflow run.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt and committed the merged code graph with graphify update .. LLM semantic extraction was attempted afterward but failed; the valid structural result was preserved (see the workflow log).

Please review the merge commit before relying on it.

# Conflicts:
#	graphify-out/GRAPH_REPORT.md
#	graphify-out/graph.json
#	graphify-out/manifest.json
Structural `graphify update` completed (graphify 0.9.4); LLM semantic extraction failed, so the verified structural graph was preserved.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/32833404228
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Lopu repository review

Lopu reviewed this PR against github-actions as Thingtime's principal PR and repository manager. Using Claude Opus 5.

Lopu made justified improvements and pushed f472d9c to codex/commander-release-developer-id.

PR #400ci: notarize Commander Developer ID releases

Compared: head 1e029523 against base github-actions @ b19f8acc — full head
vs. base, not just the newest commits. Of 19 changed files, the effective change is
74 lines of .github/workflows/commander-release.yml plus the CHANGELOG.md entry;
the other 17 are the regenerated graphify-out/ pair, which arrives from base merges.

What the PR does

Replaces Commander's ad-hoc release signing (the job-level COMMANDER_SIGNING_IDENTITY: "-")
with Developer ID distribution signing: an ephemeral runner keychain holds the imported
certificate, the build runs under COMMANDER_SIGNING_MODE=distribution with
COMMANDER_NOTARIZATION_MODE=external, and a new step notarizes, staples, and
Gatekeeper-assesses Commander/dist/Commander.app before assets are collected and
published. The release-notes trust line is updated to match.

Verification against the surrounding system

  • Build-script contract (the stated dependency, feat: add cross-platform Commander launcher #263). Read
    Commander/script/build_and_run.sh on codex/commander-cross-platform. Every variable
    this workflow exports lines up:
    • COMMANDER_SIGNING_MODE=distribution → script requires a Developer ID Application:
      identity, explicitly rejects the ad-hoc -, and adds --timestamp. Removing the
      job-level "-" is therefore required, not cosmetic — the old value now hard-fails.
    • COMMANDER_SIGNING_IDENTITY=<full identity string> → the workflow's sed capture
      (Developer ID Application:[^"]+) produces exactly the form the script's
      available_signing_identity() greps for.
    • COMMANDER_NOTARIZATION_MODE=externalnotarize_distribution_bundle() returns
      early ([[ "$NOTARIZATION_MODE" == "local" ]] || return 0). This is necessary: the
      script's local path submits with --keychain-profile "Commander Notarization", a
      profile that does not exist on a CI runner. Commander/README.md documents this
      exact pairing.
    • --build-only and DIST_DIRCommander/dist/Commander.app, matching the
      workflow's app_bundle.
  • Artifact chain ordering. Notarize/staple runs before Collect release assets,
    which re-dittos the now-stapled bundle, so the published ZIP carries the ticket.
  • Failure containment. Even if notarytool submit --wait exited 0 on a rejected
    submission, stapler staple, stapler validate, and spctl are hard gates under
    set -euo pipefail. An un-notarized bundle cannot reach gh release create.
  • Secret contract. The six credentials (MAC_CSC_LINK, MAC_CSC_KEY_PASSWORD,
    APPLE_API_KEY_BASE64, APPLE_API_KEY_ID, APPLE_API_ISSUER, APPLE_TEAM_ID) are
    the ones already in use repo-wide, and the import body matches
    electron-pr-release.yml verbatim apart from the Commander-specific filenames — as
    the PR description claims.

Change I made in this worktree

.github/workflows/commander-release.yml — added the missing Remove ephemeral signing material teardown (if: always()).

The PR mirrors electron-pr-release.yml's credential import verbatim but omits its
teardown. electron-pr-release.yml:367-372 is the only teardown in the repo, and this
lane — the one that copied its import — was the only signing lane without one.

Why this matters more here than in the lane it was copied from, rather than less:

  • The import unlocks the Developer ID private key, makes that keychain the runner's
    default keychain and sole user search-list entry, and writes the raw .p12 plus
    the App Store Connect .p8 to RUNNER_TEMP.
  • electron-pr-release.yml deliberately runs Test unsigned source before accessing signing credentials first, so third-party code executes before credentials exist.
    Commander cannot do that: build_and_run.sh builds and signs in one invocation, and
    its build_all() runs pnpm install --frozen-lockfile, pnpm test, cargo build
    (×2, incl. build.rs), and swift build — a full third-party dependency tree —
    after the key is imported and unlocked.
  • So the Commander lane has a strictly larger exposure window than the Electron lane,
    and was the one lacking the mitigation. Restructuring the script to test-before-sign
    belongs to feat: add cross-platform Commander launcher #263, not to a control-plane branch; the teardown is the in-scope fix.
  • if: always() matters specifically because the notarize step can fail or hit its
    45-minute cap, which is exactly when material would otherwise be left behind.

CHANGELOG.md — extended the existing entry to record the teardown and re-dated the
attribution to 2026-08-27.

What I deliberately did not change

  • Keychain search-list handling (security list-keychains -d user -s replaces rather
    than appends) and the exported-but-unused COMMANDER_NOTARY_TEAM_ID. Both mirror
    the Electron lane byte-for-byte. --team-id is genuinely not required for App Store
    Connect API-key auth, and delete-keychain restores the search list. Forking a
    deliberately identical pattern for no behavioural gain is churn.
  • Notarization failure diagnostics. A rejected submission yields no notarytool log
    output. notarytool does print the submission ID and Invalid status before
    stapler aborts the job, which is enough to act on, so I left the lane matching
    Electron rather than adding a bespoke branch. Flagged for the owner instead.
  • Single-architecture publishing. uname -m on macos-latest yields one arm64
    asset. Pre-existing, out of this PR's scope.

Findings that need an owner decision (no code change)

  • The lane still cannot run. Commander/ exists only on
    codex/commander-cross-platform (feat: add cross-platform Commander launcher #263, open, CONFLICTING, targeting develop) —
    not on develop, not on main. Independently, main carries no
    commander-release.yml at all (it has 7 workflows; this is not one), so the
    push/workflow_dispatch triggers in this control-plane copy cannot fire. Landing
    order must be feat: add cross-platform Commander launcher #263developmain, plus a main-side listener calling this
    workflow @github-actions. Not fixable from this branch.
  • All six macOS signing secrets become hard requirements. That is the PR's stated
    intent, and a deliberate divergence from electron-pr-release.yml, which degrades to
    an unsigned lane when all six are empty. Commander would be the first signed release
    lane on main (electron-release.yml still publishes dist:unsigned). Worth
    confirming the secrets are configured before the listener lands. I could not verify
    secret presence: listing repository secrets returns 403 for this token, by design.

Checks

All green on 1e029523: 21 pass, 20 skipping, 0 failing. Nothing to diagnose.

Notably, control-plane / Lopu builds and repairs the all branch — the cancelled-not-failed
false-red I analysed on head fea15a73, and which #395 documents as a by-design
consequence of the serialized lopu-agent-fleet group — executed and passed on this
head (4m54s). No workflow-control-plane defect is implicated, so I made no change to
$GITHUB_WORKSPACE/trusted and wrote no lopu-workflow-fix.md.

CodeQL

The trusted snapshot for this head (400.json) contains no open alerts. The
disposition file is left as the empty array it was initialized to — nothing to fix,
nothing to dispose.

Validation

Run in the PR worktree, after the edit:

Check Result
python3 yaml.safe_load of commander-release.yml parses; 13 steps
ruby -ryaml load of the same file parses
Step-gate audit (printed every step's if:) 6 setup steps ungated, 6 build/publish steps share exists != 'true', teardown always()
Import↔teardown path cross-check (parsed both step bodies, compared literals) all 3 artifacts covered: keychain, .p12, .p8 via AuthKey_*.p8
bash -n on the teardown body ok
Teardown simulated with no keychain/files present (the exists == 'true' path) exit 0 — always() cannot redden a skip-run
git diff --check clean
Full verify jobnode --check ×13 .mjs, bash -n ×3 .sh, git diff --check, build-all-branch --self-test, classify-claude-credential-failure --self-test, electron-pr-release-contract all pass
All 10 deterministic contract advisories (develop-preview, workflow control-plane, signed Electron PR release, conflict-resolver routing, rebase ownership routing, promotion-worker routing, promotion-worker behavior, promotion changelog, feature promoter, all-branch builder) 10/10 OK, 0 failures

graphify-out/ was not regenerated: the binary is not installed on this runner. Being
precise about the cost — the graph indexes this workflow at step granularity for its
notable steps (github_workflows_commander_release, ..._release,
..._import_credentials, ..._notarize), so unlike a pure reordering, my added step
means the committed graph is short one node until refresh. That is re-stamped by the
repo's automated post-merge refresh, which has already produced four such commits on
this branch (most recently 1e029523). Running a differently versioned graphify here
would have rewritten the whole ~21k-line pair for one node.

Assessment

The design is sound and the contract with #263 genuinely holds — I verified it rather
than taking the description's word for it. One real gap closed. The remaining blockers
are sequencing decisions for the owner, not defects in this diff.

View Lopu workflow run

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt and committed the merged code graph with graphify update .. LLM semantic extraction was attempted afterward but failed; the valid structural result was preserved (see the workflow log).

Please review the merge commit before relying on it.

Structural `graphify update` completed (graphify 0.9.4); LLM semantic extraction failed, so the verified structural graph was preserved.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/32858961684
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt the merged code graph first with graphify update ., then ran LLM semantic extraction (graphify extract ., claude-cli backend) and committed the result.

Please review the merge commit before relying on it.

# Conflicts:
#	graphify-out/GRAPH_REPORT.md
#	graphify-out/graph.json
#	graphify-out/manifest.json
Structural `graphify update` followed by LLM semantic `graphify extract` (graphify 0.9.4, claude-cli backend); unchanged content served from the tracked semantic cache.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/32899518951
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt the merged code graph first with graphify update ., then ran LLM semantic extraction (graphify extract ., claude-cli backend) and committed the result.

Please review the merge commit before relying on it.

# Conflicts:
#	graphify-out/GRAPH_REPORT.md
#	graphify-out/graph.json
#	graphify-out/manifest.json
Structural `graphify update` followed by LLM semantic `graphify extract` (graphify 0.9.4, claude-cli backend); unchanged content served from the tracked semantic cache.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/32946000595
@github-actions

Copy link
Copy Markdown
Contributor

Lopu — reviewer note (Thingtime PR manager). Full report lands in my review comment; here are the two things that need your call, and what I pushed to the branch.

The red check is not yours

control-plane / Lopu builds and repairs the all branch is cancelled, which gh pr checks renders as fail. Its job recorded zero steps — it never executed a line of this PR. It sat in the serialized lopu-agent-fleet group, and when that slot turned over at 09:50:22 the queued runs were swept at 09:50:3109:50:53 (thirteen of them, across main, develop, promotions and unrelated PRs). The run that won the slot was created at 08:25:07 — older than several the same sweep cancelled — so surviving the queue is not a function of recency, and no push to this branch can clear it. Repo-wide it is 35 cancelled out of the last 50 Build all branch runs.

#395 already analysed this exact false-red and deliberately left it alone (a per-PR group would license concurrent force-pushes of the single generated all branch). I did not re-litigate that here.

Landing order — needs your decision

This lane cannot run yet, for two independent reasons:

  • Commander/ exists only on codex/commander-cross-platform (feat: add cross-platform Commander launcher #263, open, currently CONFLICTING, targeting develop). It is on neither develop nor main, so Commander/script/build_and_run.sh and release-version.mjs are not there for the job to call.
  • main carries no thin commander-release.yml listener the way it does for electron-release.yml, so the push/workflow_dispatch triggers in this control-plane copy can never fire.

Do you want the main-side listener to ride along with #263's promotion, or land as its own control-plane PR once Commander is on main? Also worth confirming the six macOS signing secrets are configured before it goes live: this PR makes them hard requirements (deliberately — no unsigned Commander releases), and electron-release.yml still publishes dist:unsigned, so Commander would be the first signed release lane on main.

What I changed on this branch

  1. Import signing credentials only when there is something to sign. The import step ran unconditionally, ahead of the duplicate-release check, so a re-run on an already-published tag still unlocked the Developer ID key and wrote the App Store Connect key to disk — and still hard-failed on missing secrets with nothing to build. It now sits after Stop if this release already exists, gated on the same exists != 'true' condition as every other build/publish step, matching the ordering electron-pr-release.yml documents.
  2. Bounded the notarization wait (timeout-minutes: 45 on the notarize step). notarytool submit --wait has no bound of its own and the job sets no timeout, so an Apple-side stall could hold a 10×-billed macOS runner for GitHub's 6-hour default.
  3. Added the missing CHANGELOG.md entry for the release-policy change, in this branch's house style.

The rest verified clean: every variable this workflow exports matches #263's build_and_run.sh contract (external notarization, distribution identity, --build-only, Commander/dist/Commander.app), the secret contract mirrors electron-pr-release.yml verbatim, and notarize/staple correctly runs before asset collection so the published zip carries the ticket. CodeQL: no open alerts on this head.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt the merged code graph first with graphify update ., then ran LLM semantic extraction (graphify extract ., claude-cli backend) and committed the result.

Please review the merge commit before relying on it.

# Conflicts:
#	graphify-out/GRAPH_REPORT.md
#	graphify-out/graph.json
#	graphify-out/manifest.json
Structural `graphify update` followed by LLM semantic `graphify extract` (graphify 0.9.4, claude-cli backend); unchanged content served from the tracked semantic cache.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/32980937883
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🤝 Merged github-actions into codex/commander-release-developer-id — conflicts auto-resolved by Lopu's PR manager.

No AI resolution was needed by merge time; the branch was updated with a plain merge commit.

graphify-out/ was reset wholesale to the github-actions side (repo rule: one side, never mixed — the graph merge driver is unavailable in CI).

Rebuilt the merged code graph first with graphify update ., then ran LLM semantic extraction (graphify extract ., claude-cli backend) and committed the result.

Please review the merge commit before relying on it.

# Conflicts:
#	graphify-out/GRAPH_REPORT.md
#	graphify-out/graph.json
#	graphify-out/manifest.json
Structural `graphify update` followed by LLM semantic `graphify extract` (graphify 0.9.4, claude-cli backend); unchanged content served from the tracked semantic cache.

Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/33017064997
@github-actions

Copy link
Copy Markdown
Contributor

Lopu — reviewer note (Thingtime PR manager). Re-reviewed 1e029523 against github-actions @ b19f8acc. Full report lands in my review comment; here is the one real gap I closed, plus two updates since my last note.

The check I called a false-red is now green

control-plane / Lopu builds and repairs the all branch executed and passed on this head (4m54s). Last time it was cancelled-while-queued in the serialized lopu-agent-fleet sweep, which gh pr checks renders as fail. This head is 21 pass / 20 skipping / 0 failing — nothing left to diagnose, and nothing that implicates the workflow control plane, so I touched no protected controller file.

What I changed: the missing half of the credential contract

The import step in this PR mirrors electron-pr-release.yml verbatim. Its teardown was not carried across — electron-pr-release.yml:367-372 (Remove ephemeral signing material, if: always()) is the only such step in the repo, and this lane was the only signing lane without one. I added it, with the Commander-specific paths.

The reason this is worth fixing here rather than shrugging off as ephemeral-runner hygiene is that Commander's exposure window is strictly larger than the lane it copied:

  • electron-pr-release.yml deliberately runs Test unsigned source before accessing signing credentials first, so third-party code executes before credentials exist.
  • Commander can't do that — build_and_run.sh builds and signs in one invocation, and its build_all() runs pnpm install --frozen-lockfile, pnpm test, cargo build ×2 (including build.rs) and swift build after the Developer ID key is imported and unlocked, with the raw .p12 and App Store Connect .p8 sitting in RUNNER_TEMP and that keychain promoted to the runner's default.

So the lane with the bigger window was the one missing the mitigation. Restructuring the script to test-before-sign belongs to #263, not to a control-plane branch — the teardown is the in-scope fix. if: always() matters specifically because the notarize step can fail or hit its 45-minute cap, which is exactly when material would otherwise be left behind. I verified the teardown covers all three artifacts by parsing both step bodies and comparing the literal paths, and that it exits 0 when nothing exists, so always() can't redden a duplicate-tag skip-run.

I did not touch the keychain search-list handling or the exported-but-unused COMMANDER_NOTARY_TEAM_ID — those mirror the Electron lane byte-for-byte, --team-id genuinely isn't needed for App Store Connect API-key auth, and delete-keychain restores the search list anyway. Forking a deliberately identical pattern for no behavioural gain is churn.

Verified rather than assumed: the #263 contract holds

Every variable this workflow exports lines up with build_and_run.sh on codex/commander-cross-platform. Worth calling out that removing the job-level COMMANDER_SIGNING_IDENTITY: "-" is required, not cosmetic — distribution mode explicitly rejects the ad-hoc identity, so the old value would now hard-fail. And COMMANDER_NOTARIZATION_MODE=external is load-bearing: the script's local path submits with --keychain-profile "Commander Notarization", which doesn't exist on a CI runner. Notarize/staple correctly precedes asset collection, so the published ZIP carries the ticket.

Still needs your call — landing order

Unchanged from my last note, and still the only thing standing between this and a working lane:

  • Commander/ exists only on codex/commander-cross-platform (feat: add cross-platform Commander launcher #263, open, still CONFLICTING, targeting develop).
  • main carries no commander-release.yml at all — it has 7 workflows and this isn't one — so the push/workflow_dispatch triggers in this control-plane copy cannot fire.

Do you want the main-side listener to ride along with #263's promotion, or land as its own control-plane PR once Commander is on main? Also still worth confirming the six macOS signing secrets are configured before it goes live — this PR makes them hard requirements by design, and electron-release.yml still publishes dist:unsigned, so Commander would be the first signed release lane on main. I can't verify secret presence myself; listing repository secrets 403s for this token, by design.

Validation

YAML parses under both Python and Ruby; step-gate audit clean across all 13 steps; the full control-plane verify job passes locally (node --check ×13, bash -n ×3, git diff --check, and all three self-tests), as do 10/10 deterministic contract advisories. CodeQL: no open alerts on this head, so the disposition file stays the empty array.

One honesty note on graphify-out/: the binary isn't installed on this runner, and the graph indexes this workflow at step granularity (..._import_credentials, ..._notarize), so my added step leaves the committed graph one node short until the automated post-merge refresh re-stamps it — the same refresh that has already produced four commits on this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant