Skip to content

fix(release): avoid SIGPIPE in artifact verification - #283

Merged
steipete merged 1 commit into
mainfrom
round7/verifier-pipe
Oct 1, 2026
Merged

steipete merged 1 commit into
mainfrom
round7/verifier-pipe

Conversation

@steipete

@steipete steipete commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Prevent valid macOS release artifacts from failing verification with exit 141. Under pipefail, grep -q could close the codesign output pipe early; this was reproduced against the signed, notarized 0.7.0 server archive. Capture complete codesign and otool output before applying early-exiting parsers.

The real verifier now passes both preserved 0.7.0 server archives, including Foundation identity, explicit notarization, architecture, minimum OS, and native version checks. Six focused tests pass, including a subprocess regression with large tool output; independent P0–P2 review is clean.

Document recovery through the trusted main verifier, which the release workflow already checks out separately from the immutable release tag. The signed tag and built code are unchanged.

@steipete
steipete requested a review from a team as a code owner October 1, 2026 06:37
@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 1, 2026
@clawsweeper

clawsweeper Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed October 1, 2026, 2:39 AM ET / 06:39 UTC.

ClawSweeper review

What this changes

The PR captures complete macOS inspection-tool output before parsing it, adds a large-output regression test, and documents recovery using the trusted release verifier.

Merge readiness

✅ Ready for maintainer review

The fix remains necessary: current main retains both vulnerable pipelines. The patch is focused, preserves release validation, and introduces no actionable correctness finding.

Priority: P2
Reviewed head: c2f07bb48a56d5db11b0a6c9be6728bede3980d5

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, maintainable repair with targeted regression coverage and no blocking finding.
Proof confidence 🌊 off-meta tidepool Not applicable: Repository State identifies a member-authored PR, so ordinary contributor proof is exempt. The body reports successful after-fix verification of both preserved 0.7.0 native archives; the synthetic subprocess test is supplemental, and no runtime validation was executed here. No stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Repository State identifies a member-authored PR, so ordinary contributor proof is exempt. The body reports successful after-fix verification of both preserved 0.7.0 native archives; the synthetic subprocess test is supplemental, and no runtime validation was executed here. No stored-data contract changes.
Evidence reviewed 7 items Introduced change: The pinned introduction delta changes only the verifier, its signing test file, and release documentation. Capturing command output removes the producer-to-early-exiting-parser pipes while retaining command failure propagation under set -e.
Current main still needs the fix: The fetched main verifier directly pipes codesign into grep -Eq and otool into an early-exiting awk under pipefail. The patch’s central repair is absent.
Regression coverage: The added subprocess test invokes the production verifier against two synthetic archives, with large codesign and otool output supplied by fake tools. This targets pipe closure directly but is supplemental test coverage; tests were not executed during this read-only review.
Findings None None.
Security None None.

How this fits together

ClickClack’s macOS release verifier checks built server archives before publication. Local packaging and the release workflow use its results to accept or reject release candidates.

flowchart TD
  A[Built macOS archives] --> B[Checksum and extraction checks]
  B --> C[Signature and notarization checks]
  C --> D[Capture complete tool output]
  D --> E[Runtime and minimum OS checks]
  E --> F[Accept or reject release candidates]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +4/-2, tests +65/-1; documentation +15 The small production change is justified by preventing early pipe closure, with focused subprocess regression coverage.

Technical review

Best possible solution:

Keep release validation strict while fully consuming inspection-tool output before parsing it.

Do we have a high-confidence way to reproduce the issue?

Yes, the source establishes a focused reproduction path: large tool output feeds an early-exiting parser under pipefail. The added subprocess regression exercises that mechanism; this review did not execute it.

Is this the best way to solve the issue?

Yes. Fully capturing output is a narrow repair that preserves tool exit-status checks and existing validation criteria.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning medium; reviewed against 2408cf117adc.

Labels

Label changes:

  • add P2: This repairs false failures in a bounded release-verification path without changing shipped application behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Repository State identifies a member-authored PR, so ordinary contributor proof is exempt. The body reports successful after-fix verification of both preserved 0.7.0 native archives; the synthetic subprocess test is supplemental, and no runtime validation was executed here. No stored-data contract changes.

Label justifications:

  • P2: This repairs false failures in a bounded release-verification path without changing shipped application behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Repository State identifies a member-authored PR, so ordinary contributor proof is exempt. The body reports successful after-fix verification of both preserved 0.7.0 native archives; the synthetic subprocess test is supplemental, and no runtime validation was executed here. No stored-data contract changes.

Evidence

What I checked:

  • Introduced change: The pinned introduction delta changes only the verifier, its signing test file, and release documentation. Capturing command output removes the producer-to-early-exiting-parser pipes while retaining command failure propagation under set -e. (apps/desktop/scripts/verify-macos-server-release.sh:34, c2f07bb48a56)
  • Current main still needs the fix: The fetched main verifier directly pipes codesign into grep -Eq and otool into an early-exiting awk under pipefail. The patch’s central repair is absent. (apps/desktop/scripts/verify-macos-server-release.sh:34, 2408cf117adc)
  • Regression coverage: The added subprocess test invokes the production verifier against two synthetic archives, with large codesign and otool output supplied by fake tools. This targets pipe closure directly but is supplemental test coverage; tests were not executed during this read-only review. (apps/desktop/scripts/macos-signing.test.mjs:130, c2f07bb48a56)
  • Release trust boundary preserved: The existing workflow checks out github.workflow_sha for verification and requires the verification job before publishing. The new recovery documentation uses reviewed main without changing signed artifacts; signature identity, explicit notarization, checksums, architecture, and version checks remain intact. (.github/workflows/release.yml:204, c2f07bb48a56)
  • Release and history checks: The local v0.7.0 tag resolves to the fetched main SHA, which still contains the old pipelines. The latest supplied published release is v0.6.0, whose tree lacks this server verifier. No merged fixing PR was established; GitHub related-PR searches were unavailable in limited mode. (apps/desktop/scripts/verify-macos-server-release.sh:34, 2408cf117adc)
  • Feature-history routing: Main-line blame and path history connect the server verifier to Peter Steinberger’s earlier release-signing work. Raw commit records identify its parent, and parent-tree inspection shows the verifier path absent there. (apps/desktop/scripts/verify-macos-server-release.sh:32, bc909123584d)

Likely related people:

  • Peter Steinberger: Raw commit bc90912 adds apps/desktop/scripts/verify-macos-server-release.sh:32 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: bc909123584d; files: apps/desktop/scripts/verify-macos-server-release.sh)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 5781ea2 into main Oct 1, 2026
16 checks passed
@steipete
steipete deleted the round7/verifier-pipe branch October 1, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant