Skip to content

fix(pr-verify): install the screenshot font and match the key proxy by command line - #13707

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/12232-capture-font-proxy-stop
Nov 6, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
fix/12232-capture-font-proxy-stop

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Refs: #12232

Summary

Phase 5b's runner probe for #12232 found two defects in the merged capture job (pr-verify-capture.yml, PR #13684). The capture stays switched off (merge.verify-capture.app unset), so neither has run on a PR yet.

  • The driver's screenshot rasterizer refuses the capture when no installed font has U+23F8: control-claude-code driver: recording rasterizer could not render glyphs: '' (U+23F8); install an outline font containing them (run 42223922880). The driver names Noto Sans Symbols 2 (plugins/harness-ops/skills/control-claude-code/driver/driver.mjs:77). With fonts-noto-core installed, every outer verb returned 0 and the screenshot appeared at the same path outside the sandbox (run 42224592725).
  • "Stop the key proxy" waited on pgrep -u ccd-proxy -x node, but node's comm is MainThread (ps in run 42223922880: ccd-pro+ MainThread /opt/ccd/bin/node /opt/ccd/key-proxy/...), so the step passed without proving the proxy stopped.

Fix

  • New step "Install the screenshot font" before isolation, gated on the key like the other sandbox steps: apt-get install fonts-noto-core, then fail when fc-list ':charset=23f8' lists no Noto Sans Symbols 2.
  • "Stop the key proxy" matches the proxy by command line, ^/opt/ccd/bin/node /opt/ccd/key-proxy/key-proxy\.mjs , anchored so the sh watcher whose -c text names the same script does not match.

Verification

  • node --test .github/scripts/pr-verify/workflow.test.mjs: 75/75 pass (73 before).
  • Two new tests, both failing on the old workflow text: the font step precedes "Isolate the runner" and checks U+23F8; the stop step's pattern matches the node command line "Start the key proxy" execs and not its sh watcher, and no pgrep -x node remains.
  • scripts/check-changelog-fragments.sh --check-required origin/main: no fragment-mode plugin changed.

Related

Remaining

🤖 Generated with Claude Code

…y command line

The runner probe for #12232 (runs 42223922880 and 42224592725) found
that the driver's screenshot rasterizer refuses U+23F8 without Noto Sans
Symbols 2, and that the proxy's node process has comm MainThread, so
"Stop the key proxy" never matched it with pgrep -x node.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 6, 2026 13:27
@chatgpt-codex-connector

Copy link
Copy Markdown
Codex Review Summary: ❌ Failed

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

Review Status Commit Review trigger
📝 Code Review ❌ Failed 2026-11-06T13:27:13.483524Z
We couldn’t complete this review. Comment @codex review to try again.
09807e9 Draft marked ready
ℹ️ 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

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 09807e94f777a67bd5add1e1b8bca9c02a84945b: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Security review: no security issues found

I read every hunk of both changed files. .github/workflows/pr-verify-capture.yml is on the trusted-path list, so I read its whole change and the steps around it (L340-L510 and the stop step at L1079-L1100).

New step "Install the screenshot font" (L426-L433)

  • It runs sudo apt-get update and installs fonts-noto-core from the runner's apt sources, which are signed system sources.
  • It runs no pull-request code and reads no PR-controlled input. The package name and the fc-list query are fixed literals, with no ${{ }} expression.
  • It runs before "Isolate the runner". That is where every other sudo-needing setup step already sits, under the same steps.credential.outputs.present == 'true' gate.
  • It adds no token, secret, permission or environment value. The key proxy is not started until later (L494), so the step never sees PROXY_KEY.

"Stop the key proxy": pgrep -x node replaced by pgrep -u ccd-proxy -f "$proxy" (L1086-L1099)

  • The pattern ^/opt/ccd/bin/node /opt/ccd/key-proxy/key-proxy\.mjs is a fixed, single-quoted literal and is passed quoted, so nothing can be injected into it.
  • It is anchored at the start of the command line. It matches the exec command line at L494 and not the sh -c watcher, whose command line starts with /bin/sh.
  • Before this change, -x node never matched (node's comm is MainThread), so the step passed without showing that the proxy had stopped. Now the step really checks that the credential-holding proxy has exited. The check got stricter; no control was weakened.

.github/scripts/pr-verify/workflow.test.mjs: test-only. It adds the new step to SANDBOX_STEPS and adds two assertions that pin the font step's order and the pgrep pattern. No security surface.

Other checks

  • No trust control changed: no trusted-actor, token-scope, kill-switch or merge-hold logic is touched.
  • No instruction surface is deleted or softened, and no plugin manifest changed.
  • The PR context has no embedded directives aimed at the review.

Spend: none. The new step adds up to 5 minutes of a standard hosted runner. It runs only while the capture credential is set, and the PR says the capture stays off (merge.verify-capture.app unset).

critical-findings: 0

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed 09807e94f777a67bd5add1e1b8bca9c02a84945b: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Code review: no findings

I read every hunk of both changed files: .github/workflows/pr-verify-capture.yml and .github/scripts/pr-verify/workflow.test.mjs. I also read the unchanged steps the changes depend on: "Start the key proxy", "Start the sibling sandbox" and the job's default shell. This was a full review, not blocking-only, and there were no prior findings.

Install the screenshot font (L426-L433)

  • The step runs only when the key is set, like the other sandbox steps, and runs before "Isolate the runner". The new test checks that order.
  • The [[ ... ]] test works because the job sets shell: bash (L97).
  • The sandbox can see the installed font. It mounts the root filesystem read-only with --ro-bind / / (L568), so it sees /usr/share/fonts and the system fontconfig cache, which the package's install step rebuilds.

Stop the key proxy (L1086-L1099)

  • "Start the key proxy" (L494) runs exec /usr/bin/env -i PATH=... /opt/ccd/bin/node /opt/ccd/key-proxy/key-proxy.mjs --socket .... Because env replaces itself with node, the running proxy's command line begins /opt/ccd/bin/node /opt/ccd/key-proxy/key-proxy.mjs . The anchored pattern matches that.
  • The watcher subshell keeps the command line /bin/sh -c ..., so the ^ anchor keeps it out of the match.
  • The wait loop now really waits, up to 10 s. That is longer than the watcher's worst case: up to 1 s to notice the stop file, then 5 s before kill -9. It is well inside the step's 1-minute timeout.
  • The new test pulls the node command line and the watcher text out of the start step's own script, so it will fail if either step's text drifts.

Closing issue: the PR references #12232 but closes no issue, so I checked no acceptance criteria. I found no embedded instructions in the PR context.

Verification: I did not run anything. The 75/75 test pass and the probe-run results are the author's claims, which I have not verified.

Spend: the new step adds one apt-get update and font install, up to 5 minutes, to capture runs that have the key set. That is ordinary hosted-runner time, not a billing change.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 6, 2026
Merged via the queue into main with commit 5dd9a12 Nov 6, 2026
71 of 72 checks passed
@kyle-sexton
kyle-sexton deleted the fix/12232-capture-font-proxy-stop branch November 6, 2026 13:41
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Post-merge mutation testing

Killed 6, survived 1, timeout 0, no coverage 0, invalid 0, not run 0.
Covered-code score 86%; mutation score 86%.

File Killed Survived Timeout No coverage Invalid Not run
lib/read-verification.sh 6 1 0 0 0 0

Surviving mutants

Location Operator Disposition Cause
lib/read-verification.sh:106 other productive input-gap

The run's summary has the full report.

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