Conversation
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 97a8d0cfa117
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 1 |
| policies:conventions | 2 |
| structure:repo-health | 0 |
| security:code-auditor | 1 |
automation:ci-release (1 finding)
Major - Makefile:16
The sign step and its verification are joined with
;, so a failedcodesign --signdoes not fail the build.In each of the four added blocks the recipe is
codesign --force --sign ... ; codesign --verify --strict .... The exit status of theifblock is the status of the last command, so if signing fails (identity string typo, identity not in the login keychain, keychain locked, user cancels the keychain prompt), make only sees the result of--verify. On Darwin,go buildalready emits an ad-hoc signature, socodesign --verify --strictsucceeds against that ad-hoc signature and the recipe exits 0. The developer gets a greenmake buildwith exactly the per-buildcdhashdesignated requirement this PR is meant to eliminate — the failure mode is invisible, and the Keychain re-prompting is blamed on something else.Fix: make the step fail loudly and assert the identity actually applied, e.g.
@if [ -n "$(CODESIGN_IDENTITY)" ] && [ "$$(uname -s)" = Darwin ]; then \ codesign --force --timestamp=none --sign "$(CODESIGN_IDENTITY)" --identifier "$(CFL_CODESIGN_ID)" bin/cfl && \ codesign --verify --strict -R "=identifier \"$(CFL_CODESIGN_ID)\"" bin/cfl; \ fiUsing
&&propagates the signing failure, and the-Rrequirement makes--verifyreject a binary that still carries only the ad-hoc signature instead of rubber-stamping it.
policies:conventions (2 findings)
Major - Makefile:16
This inlines the org signing policy (the
org.open-cli-collective.<tool>identifier scheme and thecodesignflag set) into the repo, and repeats it in four places (lines 16, 21, 48, 55). The repo's own convention keeps that policy in the shared automation source:.goreleaser-cfl.ymldeliberately delegates to$CODESIGN_DARWIN_SCRIPTfromopen-cli-collective/.githubwith the comment "Logic + identity live in open-cli-collective/.github (macos-codesign-setup)", and STANDARDS.md "Conflict Resolution" says to update the shared source rather than copy a family-wide rule here. As written, if the shared script changes the identifier or flags, local builds silently produce a different designated requirement than release binaries — the exact drift this PR is trying to remove.Smallest policy-aligned fix: keep the CODESIGN_IDENTITY gate but stop restating the policy. Either invoke the shared script when it is available, or at minimum factor the block into one
defineand cite the source, e.g.# macOS code-signing for local builds — stable DR so Keychain "Always Allow" # survives a rebuild (cli-common distribution.md §2A). Identity/identifier scheme # mirrors open-cli-collective/.github macos-codesign-setup. CODESIGN_IDENTITY # unset (CI/Linux default) → no-op. define codesign-darwin @if [ -n "$(CODESIGN_IDENTITY)" ] && [ "$$(uname -s)" = Darwin ]; then \ codesign --force --timestamp=none --sign "$(CODESIGN_IDENTITY)" --identifier "org.open-cli-collective.$(1)" bin/$(1); \ codesign --verify --strict bin/$(1); \ fi endefand call
$(call codesign-darwin,cfl)/$(call codesign-darwin,jtk)from the four sites, so the identifier convention exists once and carries a breadcrumb to its source of truth.
Minor - Makefile:14
CODESIGN_IDENTITYis a new developer-facing build variable with no documentation.docs/development.mdis the repo-local source of truth for how to build this monorepo and its "Quick Commands" section listsmake build/make build-cfl/make build-jtkwith no mention of the variable, so the only place a future contributor can learn it exists is an unannotated shell block in the Makefile.Add a short note under "Quick Commands" in
docs/development.md: that settingCODESIGN_IDENTITYto a code-signing cert in the login keychain re-signs local darwin builds with a stable designated requirement (one Keychain "Always Allow" per credential instead of one per rebuild), and that leaving it unset — the default, and on CI/Linux — leaves the build unchanged.
security:code-auditor (1 finding)
Nits - Makefile:16
Category: shell quoting. Confidence: high, impact low.
$(CODESIGN_IDENTITY)is expanded by make directly into the shell text, so the double quotes around it do not protect the value: a value containing",`, or$(...)is re-parsed by the shell and can execute commands or split into extra codesign arguments. The value is developer-supplied on their own machine (make build CODESIGN_IDENTITY=...or their env), so this is a self-inflicted footgun rather than a trust-boundary crossing — no untrusted party controls it — which is why it's a nit and not a real injection finding. It does become a small hazard if the variable is ever set by a shared wrapper script or CI config.Fix: hand it to the shell as an environment variable instead of splicing the text, so the shell quotes apply to the value:
@if [ -n "$$CODESIGN_IDENTITY" ] && [ "$$(uname -s)" = Darwin ]; then \ codesign --force --timestamp=none --sign "$$CODESIGN_IDENTITY" --identifier "org.open-cli-collective.cfl" bin/cfl; \ fiwith
export CODESIGN_IDENTITYnear the existingexport GOFLAGSat line 5 so command-line overrides still reach the recipe. Applies to all four blocks.
Reviewer Coverage
automation:ci-release— complete (constrained); skipped: none; constraints: Scope limited to Makefile; the CI signing logic lives in the sibling open-cli-collective/.github repo (macos-codesign-setup) which is not checked out, so identifier/identity parity with CI could not be verified directly. Signing behavior was not executed; findings are from reading the recipes plus .goreleaser-{cfl,jtk}.yml and .github/workflows/ci.yml (all CI jobs run on ubuntu-latest, so the Darwin guard keeps merge gates unchanged).policies:conventions— complete (constrained); skipped: none; constraints: Only Makefile was in scope; docs/development.md and .goreleaser-*.yml were read for context but not reviewed as changes. The shared macOS signing script (open-cli-collective/.github macos-codesign-setup) and cli-common distribution.md are not present in the checkout, so the exact CI identifier/flag set could not be diffed against this block.structure:repo-health— complete (constrained); skipped: none; constraints: No macOS signing identity available in this environment; codesign behavior was reasoned about, not executed. Scope limited to the assigned changed file (Makefile); docs/development.md and .goreleaser-*.yml were read only as context and cannot be anchored. The CI signing logic (macos-codesign-setup / CODESIGN_DARWIN_SCRIPT) lives in open-cli-collective/.github and was not available in this checkout, so the claimed identifier match with CI could not be verified directly.security:code-auditor— complete (constrained); skipped: none; constraints: Could not execute codesign in this environment; failure-mode analysis is from the recipe text plus documented codesign/Go-linker ad-hoc signing behavior. Scope limited to the assigned Makefile; the CI signing path (.goreleaser-*.yml -> CODESIGN_DARWIN_SCRIPT in open-cli-collective/.github) was read for comparison only and is out of scope.
Inspected files (1)
Makefile
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 19s | ~$2.47 (est.) | claude-opus-5 | cr 0.10.302
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | automation:ci-release, policies:conventions, structure:repo-health, security:code-auditor |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 19s wall · 7m 21s compute |
| Cost | ~$2.47 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 78 in / 24.1k out |
Per-workstream usage
orchestrator-selection— claude-opus-5- In: 6
- Out: 1.7k
- Cache read: 41.2k
- Cache create: 22.2k
- Cost: ~$0.29 (est.)
- Duration: 36s
automation:ci-release— claude-opus-5- In: 14
- Out: 5.2k
- Cache read: 147.6k
- Cache create: 25.5k
- Cost: ~$0.46 (est.)
- Duration: 1m 33s
policies:conventions— claude-opus-5- In: 18
- Out: 4.7k
- Cache read: 208.3k
- Cache create: 28.6k
- Cost: ~$0.51 (est.)
- Duration: 1m 26s
structure:repo-health— claude-opus-5- In: 20
- Out: 6.4k
- Cache read: 234.0k
- Cache create: 26.3k
- Cost: ~$0.54 (est.)
- Duration: 1m 51s
security:code-auditor— claude-opus-5- In: 14
- Out: 4.0k
- Cache read: 140.4k
- Cache create: 28.9k
- Cost: ~$0.46 (est.)
- Duration: 1m 12s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 2.1k
- Cache read: 89.9k
- Cache create: 12.5k
- Cost: ~$0.22 (est.)
- Duration: 39s
| build: | ||
| go build -v -o bin/cfl ./tools/cfl/cmd/cfl | ||
| @if [ -n "$(CODESIGN_IDENTITY)" ] && [ "$$(uname -s)" = Darwin ]; then \ | ||
| codesign --force --timestamp=none --sign "$(CODESIGN_IDENTITY)" --identifier "org.open-cli-collective.cfl" bin/cfl; \ |
There was a problem hiding this comment.
The sign step and its verification are joined with ;, so a failed codesign --sign does not fail the build.
In each of the four added blocks the recipe is codesign --force --sign ... ; codesign --verify --strict .... The exit status of the if block is the status of the last command, so if signing fails (identity string typo, identity not in the login keychain, keychain locked, user cancels the keychain prompt), make only sees the result of --verify. On Darwin, go build already emits an ad-hoc signature, so codesign --verify --strict succeeds against that ad-hoc signature and the recipe exits 0. The developer gets a green make build with exactly the per-build cdhash designated requirement this PR is meant to eliminate — the failure mode is invisible, and the Keychain re-prompting is blamed on something else.
Fix: make the step fail loudly and assert the identity actually applied, e.g.
@if [ -n "$(CODESIGN_IDENTITY)" ] && [ "$$(uname -s)" = Darwin ]; then \
codesign --force --timestamp=none --sign "$(CODESIGN_IDENTITY)" --identifier "$(CFL_CODESIGN_ID)" bin/cfl && \
codesign --verify --strict -R "=identifier \"$(CFL_CODESIGN_ID)\"" bin/cfl; \
fiUsing && propagates the signing failure, and the -R requirement makes --verify reject a binary that still carries only the ad-hoc signature instead of rubber-stamping it.
Reply inline to this comment.
| @@ -12,7 +12,15 @@ all: check | |||
| # Build all binaries into bin/ | |||
| build: | |||
| go build -v -o bin/cfl ./tools/cfl/cmd/cfl | |||
There was a problem hiding this comment.
CODESIGN_IDENTITY is a new developer-facing build variable with no documentation. docs/development.md is the repo-local source of truth for how to build this monorepo and its "Quick Commands" section lists make build / make build-cfl / make build-jtk with no mention of the variable, so the only place a future contributor can learn it exists is an unannotated shell block in the Makefile.
Add a short note under "Quick Commands" in docs/development.md: that setting CODESIGN_IDENTITY to a code-signing cert in the login keychain re-signs local darwin builds with a stable designated requirement (one Keychain "Always Allow" per credential instead of one per rebuild), and that leaving it unset — the default, and on CI/Linux — leaves the build unchanged.
Reply inline to this comment.
…document CODESIGN_IDENTITY
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: ad6bc69aba2d
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 0 |
| policies:conventions | 1 |
| structure:repo-health | 0 |
| security:code-auditor | 0 |
policies:conventions (1 finding)
Blocking - Makefile:28
The
define codesign-darwinconsolidation was added but the four original inline blocks were not removed, so every build target now runs both. Lines 28-31, 34-37, 62-65 and 70-73 still hold the old copy alongside the new$(call codesign-darwin,...)on lines 27, 33, 61 and 69.This is not just leftover duplication — the stale blocks run after the hardened one and undo both fixes this revision claims:
- They re-sign with
sign; verifyjoined by;, so the recipe's exit status iscodesign --verify's alone and a failed--sign(bad identity, locked keychain, cancelled prompt) still does not fail the build. That was the unresolved Makefile:29 finding.- They gate only on
uname -s, not ongo env GOOS, so the new target-GOOS guard is bypassed: on a Darwin host cross-compiling (GOOS=linux make build), the define correctly skips and the stale block then tries tocodesigna Linux ELF binary.- They restate the
org.open-cli-collective.<tool>identifier scheme in four more places, which is exactly the drift the settled thread resolved by naming codesign-darwin.sh as the single source of truth. The policy now lives in five places instead of one.Fix: delete lines 28-31, 34-37, 62-65 and 70-73, leaving only the
$(call codesign-darwin,cfl)/$(call codesign-darwin,jtk)lines after eachgo build. Re-verify withCODESIGN_IDENTITY=<bad-identity> make build-cfl(should exit non-zero) andGOOS=linux make build-cfl(should skip signing).
Reviewer Coverage
automation:ci-release— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: Scope limited to Makefile; docs/development.md also changed but is outside this reviewer's assigned files. Signing was not executed; conclusions come from reading the recipes plus .goreleaser-{cfl,jtk}.yml and .github/workflows/ci.yml, where all jobs run on ubuntu-latest so the Darwin gate leaves merge gates unchanged. The shared signing script (open-cli-collective/.github macos-codesign-setup/codesign-darwin.sh) is not in this checkout, so identifier/flag parity with release signing was taken from the in-repo comment, not verified.policies:conventions— complete (constrained); skipped: none; constraints: Scope limited to Makefile and docs/development.md. open-cli-collective/.github macos-codesign-setup/codesign-darwin.sh is not in the checkout, so the identifier scheme and flags in the new define could not be diffed against the release signer.structure:repo-health— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: Did not re-raise the two threads already settled in discussion (delegating to codesign-darwin.sh; $(CODESIGN_IDENTITY) shell quoting). No macOS signing identity available here; recipe exit-status behavior was reasoned from make/shell semantics, not executed. Scope limited to the assigned changed file (Makefile); docs/development.md was read as context only and cannot be anchored. codesign-darwin.sh lives in open-cli-collective/.github and is not in this checkout, so the identifier scheme and flags could not be diffed against the release signer directly.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: Scope limited to the assigned Makefile; docs/development.md is in the change map but not assigned, and was read only to confirm CODESIGN_IDENTITY is now documented. The shell-quoting nit on $(CODESIGN_IDENTITY) was declined by the author with a reasoned justification in a resolved thread and is not re-raised. codesign could not be executed in this environment; failure-mode analysis is from the recipe text plus documented codesign and Go-linker ad-hoc signing behaviour.
Inspected files (2)
Makefiledocs/development.md
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 3m 56s | ~$1.45 (est.) | claude-opus-5 | cr 0.10.302
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | automation:ci-release, policies:conventions, structure:repo-health, security:code-auditor |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 56s wall · 4m 51s compute |
| Cost | ~$1.45 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 42 in / 13.8k out |
Per-workstream usage
automation:ci-release— claude-opus-5- In: 8
- Out: 3.2k
- Cache read: 152.3k
- Cache create: 15.5k
- Cost: ~$0.31 (est.)
- Duration: 55s
policies:conventions— claude-opus-5- In: 10
- Out: 2.2k
- Cache read: 208.7k
- Cache create: 13.6k
- Cost: ~$0.29 (est.)
- Duration: 44s
structure:repo-health— claude-opus-5- In: 12
- Out: 4.7k
- Cache read: 249.3k
- Cache create: 16.9k
- Cost: ~$0.41 (est.)
- Duration: 1m 28s
security:code-auditor— claude-opus-5- In: 6
- Out: 2.5k
- Cache read: 98.0k
- Cache create: 11.7k
- Cost: ~$0.23 (est.)
- Duration: 45s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 1.3k
- Cache read: 175.8k
- Cache create: 8.5k
- Cost: ~$0.21 (est.)
- Duration: 56s
| build: | ||
| go build -v -o bin/cfl ./tools/cfl/cmd/cfl | ||
| $(call codesign-darwin,cfl) | ||
| @if [ -n "$(CODESIGN_IDENTITY)" ] && [ "$$(uname -s)" = Darwin ]; then \ |
There was a problem hiding this comment.
The define codesign-darwin consolidation was added but the four original inline blocks were not removed, so every build target now runs both. Lines 28-31, 34-37, 62-65 and 70-73 still hold the old copy alongside the new $(call codesign-darwin,...) on lines 27, 33, 61 and 69.
This is not just leftover duplication — the stale blocks run after the hardened one and undo both fixes this revision claims:
- They re-sign with
sign; verifyjoined by;, so the recipe's exit status iscodesign --verify's alone and a failed--sign(bad identity, locked keychain, cancelled prompt) still does not fail the build. That was the unresolved Makefile:29 finding. - They gate only on
uname -s, not ongo env GOOS, so the new target-GOOS guard is bypassed: on a Darwin host cross-compiling (GOOS=linux make build), the define correctly skips and the stale block then tries tocodesigna Linux ELF binary. - They restate the
org.open-cli-collective.<tool>identifier scheme in four more places, which is exactly the drift the settled thread resolved by naming codesign-darwin.sh as the single source of truth. The policy now lives in five places instead of one.
Fix: delete lines 28-31, 34-37, 62-65 and 70-73, leaving only the $(call codesign-darwin,cfl) / $(call codesign-darwin,jtk) lines after each go build. Re-verify with CODESIGN_IDENTITY=<bad-identity> make build-cfl (should exit non-zero) and GOOS=linux make build-cfl (should skip signing).
Reply inline to this comment.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: dbf550bfb158
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| security:code-auditor | 0 |
Reviewer Coverage
automation:ci-release— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: No signing was performed (no code-signing identity available); verification was limited tomake -nexpansion and a shell exit-status check of the guarded && chain. Scope limited to Makefile; docs/development.md also changed but is outside this reviewer's assigned files. The shared signing script (open-cli-collective/.github macos-codesign-setup/codesign-darwin.sh) is not in this checkout, so identifier/flag parity with release signing was taken from the in-repo comment rather than verified against the script.policies:conventions— complete (constrained); skipped: none; constraints: Scope limited to Makefile and docs/development.md. open-cli-collective/.github macos-codesign-setup/codesign-darwin.sh is not in this checkout, so the identifier scheme and codesign flags in the define could not be diffed against the release signer; the comment naming it as source of truth was taken at face value.structure:repo-health— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: Did not re-raise the two threads already settled in discussion (delegating to codesign-darwin.sh; $(CODESIGN_IDENTITY) shell quoting). No macOS signing identity available here; recipe exit-status and gating behavior were reasoned from make/shell semantics rather than executed. Scope limited to the assigned changed file (Makefile); docs/development.md was read as context only and is not an anchorable path in this assignment. codesign-darwin.sh lives in open-cli-collective/.github and is absent from this checkout, so the identifier scheme and flags could not be diffed against the release signer directly.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):Makefile; skipped: none; constraints: Scope limited to the assigned Makefile; docs/development.md is in the change map but not assigned to this reviewer. The $(CODESIGN_IDENTITY) shell-quoting nit was declined by the author in a resolved thread (developer-controlled value, no trust boundary) and is not re-raised. codesign could not be executed here, so the signing behaviour is assessed from the recipe text plus documented codesign and Go-linker ad-hoc signing behaviour.
Inspected files (2)
Makefiledocs/development.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 16s | ~$1.56 (est.) | claude-opus-5 | cr 0.10.302
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | automation:ci-release, policies:conventions, structure:repo-health, security:code-auditor |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 16s wall · 4m 11s compute |
| Cost | ~$1.56 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 50 in / 11.7k out |
Per-workstream usage
automation:ci-release— claude-opus-5- In: 12
- Out: 3.9k
- Cache read: 338.8k
- Cache create: 15.8k
- Cost: ~$0.42 (est.)
- Duration: 1m 15s
policies:conventions— claude-opus-5- In: 8
- Out: 1.7k
- Cache read: 217.2k
- Cache create: 12.4k
- Cost: ~$0.28 (est.)
- Duration: 38s
structure:repo-health— claude-opus-5- In: 10
- Out: 2.6k
- Cache read: 285.5k
- Cache create: 14.1k
- Cost: ~$0.35 (est.)
- Duration: 57s
security:code-auditor— claude-opus-5- In: 14
- Out: 2.7k
- Cache read: 337.2k
- Cache create: 11.3k
- Cost: ~$0.35 (est.)
- Duration: 58s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 791
- Cache read: 197.6k
- Cache create: 4.3k
- Cost: ~$0.16 (est.)
- Duration: 21s
|
Closing without merge. Tested end to end on macOS 15: the stable designated requirement this adds is stored in the Keychain ACL but never honored for a self-signed cert, whether untrusted, user-trusted, or admin-trusted. Every new build still re-prompts once per item, so this change does not fix the problem it was written for. Evidence and the working alternative (routing keychain access through |
2 participants
Release binaries are signed in CI with the org's stable identity so macOS Keychain "Always Allow" grants survive upgrades. Local
make buildoutput was still ad-hoc signed, so every rebuild carried a freshcdhashdesignated requirement and Keychain re-prompted once per stored credential.This adds the same gated step to the Makefile build targets: when
CODESIGN_IDENTITYis set on Darwin, the binary is re-signed with that identity and theorg.open-cli-collective.<tool>identifier, matching the CI signing script. Unset (the default, and on CI/Linux) the build is unchanged.A developer can point the variable at any self-signed code-signing cert in their login keychain. One "Always Allow" per credential then covers every subsequent local build.
Verified locally: with the variable set,
codesign -d -r-reportsidentifier "org.open-cli-collective.<tool>" and certificate leaf = H"…"; with it unset the designated requirement is the previous per-buildcdhash.