Skip to content

fix(security): keep the signature probe's cache guard honest on CI runners - #3783

Merged
devantler merged 2 commits into
mainfrom
claude/probe-cleanup-refused-ref-3781
Sep 13, 2026
Merged

fix(security): keep the signature probe's cache guard honest on CI runners#3783
devantler merged 2 commits into
mainfrom
claude/probe-cleanup-refused-ref-3781

Conversation

@devantler

@devantler devantler commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Generated by the Agentic Engineer

Why

The image-signature probe's first live run passed, but it logged a broken-pipe error and asked the operator to remove an image by hand. Looking into the broken pipe turned up something more serious. On GitHub's runners, the check that stops the probe from testing an image the node already has could report a cached image as absent. A cached image is never re-pulled, so the probe could pass without the node verifying anything.

In that run, the error appeared at exactly that check. So the signed control was most likely already on the node, and its "accepted" result says nothing about verification. The unsigned image was refused, and that half of the evidence stands.

What

  • Cache check: it now gives the right answer on CI runners, so a cached image makes the probe inconclusive, as intended.
  • Cleanup: when removing the unsigned image fails, the probe checks whether the node still has it. If the image is gone, for example because the refused pull never stored it, the probe says so. It still warns when the image is present or the node can't be listed.
  • Verdicts: unchanged in every other case.

Fixes #3781

🤖 Generated with Claude Code

@devantler @claude
fix(security): keep the signature probe's cache guard honest on CI ru…
ca0853c 
…nners

The probe checked whether a ref was already cached by piping the node's
image list into grep -q. GitHub Actions ignores SIGPIPE, so when grep
matched early the writer failed with a broken pipe, and pipefail turned
that failure into "not cached". A cached ref could pass the guard.
The checks now use here-strings.

Cleanup also re-lists the node when a removal fails. A ref that is no
longer there, such as a refused pull that stored nothing, is reported as
not present instead of asking for manual cleanup. A ref still present, or
a node that cannot be listed, still warns.

Fixes #3781

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@devantler devantler mentioned this pull request Sep 13, 2026
Closed
@devantler @claude
docs(security): record why the probe's presence checks must not pipe
481e320 
A pipe reports a match as no match on CI runners, which is what let a
cached ref past the cache guard. Say so where the helper is defined, so
a later tidy-up does not restore the pipe.

Part of #3781

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@devantler

devantler commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@devantler I will perform a full review of #3783.

⚠️ Action not completed

Review rate limited.

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 minutes.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 48 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7227b294-04aa-4932-b209-c69340863f4b

📥 Commits

Reviewing files that changed from the base of the PR and between f08d6da and 481e320.

📒 Files selected for processing (2)
  • scripts/probe-image-signature-enforcement.sh
  • scripts/tests/test-probe-image-signature-enforcement.sh

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.

@devantler

devantler commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 13, 2026 •
edited
Loading

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-13T20:14:49.168470Z 481e320 Manual request
ℹ️ 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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 481e320ed1

ℹ️ 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".

@devantler

devantler commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

Readiness at 481e320ed127563ffee940c15c2a3ac4d8fab005

  • Programmatically tested: CI is green at this head (23 passing, 7 skipped). The probe suite ran in the Validate Manifests job and passed all 39 cases, including the four new ones. shellcheck is clean. Test-first:
    • Before the fix: the never-stored ref still warned.
    • Ablation: putting the pipe back makes the cached-ref case exit 1 instead of 3, which is the fail-open this fixes.
  • Reviewed: CodeRabbit refused this head with a rate limit (reply 5655813187). Codex then reviewed commit 481e320ed1 and found no major issues (comment 5655832232). There are no review threads.
  • Tried and evaluated as a user: the probe only runs against a production node, so I exercised it the way CI does. I used the fake-node suite with SIGPIPE ignored, as GitHub's runners ignore it, and an image list larger than a pipe buffer. As its operator:
    • Cached image: the probe now stops as inconclusive instead of claiming both images are absent.
    • Refused image: a refused image that was never stored is reported as not present, with no manual-cleanup request.
    • Real cleanup failure: it still warns.

@devantler
devantler marked this pull request as ready for review September 13, 2026 20:16
@devantler
devantler added this pull request to the merge queue Sep 13, 2026
Merged via the queue into main with commit 35ed221 Sep 13, 2026
30 checks passed
@devantler
devantler deleted the claude/probe-cleanup-refused-ref-3781 branch September 13, 2026 20:54
@github-project-automation github-project-automation Bot moved this from 🫴 Ready to ✅ Done in 🌊 Project Board Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

🌊 Project Board
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(security): stop the signature probe warning about refused refs it never stored

1 participant

@devantler