chore(groom): delete the superseded key-broker.mjs loopback proxy and its test - #326
Merged
Merged
Conversation
…-4419 -> BE-4303) `key-broker.mjs` was the BE-4311 loopback-TCP API-key proxy. BE-4303 replaced that wiring with the agent sandbox: all three groom agent jobs now start `broker.mjs` on a bind-mounted unix socket and bridge it with the in-jail `jail-shim.mjs`. Nothing has loaded `key-broker.mjs` since, so its only live effect was sitting inside the groom fleet's watched surface, where a commit touching it fans a no-op SHA-bump PR across every groom caller. Re-confirmed dead before deleting: `groom.yml` starts `broker.mjs` in all three agent jobs and never names the deleted file; `agent-sandbox.sh` and `jail-shim.mjs` reference only `broker.mjs`; an org-wide code search returned only this repo's own README, test and the bump-fleet comment; its whole history is the one commit that added it. Its `GROOM_BROKER_PORT` / `GROOM_BROKER_UPSTREAM` knobs and port 8199 had no other reader, so the README table goes with it. The README's "The agent sandbox" section already carries the broker.mjs / jail-shim / dummy-key hand-off and the literal-key output scans, so the deleted section's `groom.yml wiring` blockquote had nothing unique to relocate. `WATCHED_EXEC`, `WATCHED_ASSETS` and the `paths:` filter of `bump-groom-callers.yml` are unchanged — the deleted file was never listed. Its DELIBERATELY-ABSENT note is rewritten to keep the rule it recorded (only what `groom.yml` actually starts belongs on the list) without naming a file that no longer exists. Skip-caller-bump: true
mattmillerai
added
cursor-review
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 20 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 20 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 58 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 144 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 1 finding(s).
| Severity | Count |
|---|---|
| 🟢 Low | 1 |
Panel: 6/6 reviewers contributed findings.
… not "what groom.yml starts"
The replacement comment claimed the list "names only what `groom.yml` ACTUALLY
starts — broker.mjs", which is both false and self-contradicting: 13 of the 13
entries include the three briefs, package.json and five .py modules that no step
starts, and the paragraphs directly above already state the real inclusion test
("absence breaks a pinned caller at run time", NOT "is it an executable"). A
maintainer applying the comment as written would prune those entries and
silently weaken the per-file decommission probe.
Restate it as the CONVERSE of the run-time test, and demote the
broker.mjs-vs-the-deleted-proxy point to the worked example it always was.
Raised by 3 of 6 cursor-review reviewers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…flight
`sandbox-tests` has been failing on every run since the runner image's cached
package list fell behind the Ubuntu pool: preflight() ran `apt-get install -y
bubblewrap` with no prior refresh, so apt tried to fetch the exact superseded
version it still believed in and got a 404.
Err:2 ... noble-updates/main amd64 bubblewrap amd64 0.9.0-1ubuntu0.1
404 Not Found
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
Reproduced on a fresh runner (attempt 3, 9s), so it is not a transient mirror
blip. The damage is not just a red check: a stale-index 404 surfaces to the
caller as preflight's fail-loud "sandbox unavailable on this runner image"
verdict, which is a claim about the runner's namespace support, not about apt.
`apt-get update || true` — a flaky mirror during the refresh is survivable, and
the install below stays the step that must fail loud. The fast path (bwrap
present and selftest green) still short-circuits before either call, so this
costs nothing on a warm runner.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…key-broker # Conflicts: # .github/groom/agent-sandbox.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI-5
The groom workflow used to reach Anthropic through a little localhost proxy,
key-broker.mjs. That was replaced a while ago by the agent sandbox, which usesbroker.mjsover a unix socket instead. The old proxy has been sitting in the repo ever since, loaded by nothing — but it lives inside the groom fleet's watched surface, so any commit that touched it would fan a no-op SHA-bump PR to every groom caller. This deletes it, its test, and its README section.What changed
.github/groom/key-broker.mjsand.github/groom/tests/test_key_broker.py..github/groom/README.md— removed thekey-broker.mjs — the localhost API-key proxysection (heading through itsgroom.yml wiringblockquote), and dropped the sentence in thetests/bullet about key-broker tests booting the real script undernode..github/workflows/bump-groom-callers.yml— rewrote theDELIBERATELY ABSENTcomment aboveWATCHED_EXEC.WATCHED_EXEC,WATCHED_ASSETSand thepaths:filter are unchanged — the deleted file was never listed in any of them..github/groom/jail-shim.mjs:7— comment says "the broker (broker.mjs)" instead of "the key-broker (broker.mjs)", removing the last misleading mention.Skip-caller-bump: trueas its sole trailing trailer (verified withgit interpret-trailers --parse), so the removal does not itself fan a churn bump. Please keep that trailer intact in the squash-merge message —preflight.shreads it only from the trailing trailer block.Why the deleted README section needed no relocation
The section's blockquote was the guidance to check: if it were the only place the sandbox /
broker.mjshand-off is described, that paragraph had to move rather than die. It is not.## The agent sandbox — agent-sandbox.sh + broker.mjsalready describes the per-job composition end to end — the broker step holdingsecrets.ANTHROPIC_API_KEYand startingbroker.mjson$BROKER_SOCK, the sandbox preflight, the agent step running with a DUMMY key andANTHROPIC_BASE_URLpointed at the in-jailjail-shim.mjs, the literal-key scan/capture tripwires, and thealways()broker cleanup. Nothing unique was lost, so the blockquote is deleted rather than moved.Evidence it is dead (re-confirmed, not taken on faith)
Deleting a credential-handling file is a capability removal, so I went and looked via every path rather than trusting the prior investigation:
grep -n "broker.mjs" .github/workflows/groom.yml→ the three agent jobs each runnohup node "$GROOM_ASSETS/broker.mjs" "$BROKER_SOCK"(lines 1501, 2001, 2941). No reference to the deleted file anywhere ingroom.yml.agent-sandbox.shandjail-shim.mjsreference onlybroker.mjs//run/broker.sock.gh search code "key-broker.mjs" --owner Comfy-Org→ 6 hits, all in this repo, all now removed by this diff (README ×2, the bump-fleet comment ×2, the test ×2). A second search forkey_brokerreturned nothing.git log -- .github/groom/key-broker.mjs→ one commit, the one that added it. Nothing has touched it since.WATCHED_EXEC, so its deletion cannot trippreflight.sh's decommission probe, andWATCHED_ASSETS(.github/groom) still resolves.GROOM_BROKER_PORT/GROOM_BROKER_UPSTREAMenv knobs and port8199now appear nowhere in the tree, so removing the README's knob table orphans no live configuration.git grep -n key-brokerreturns nothing.Judgment call
The ticket asked for two things that cannot both hold: rewrite the
DELIBERATELY ABSENTcomment to namekey-broker.mjsas removed, and havegit grep -n key-brokerreturn nothing. I resolved in favour of the mechanical check — the rewritten comment states the rule ("this list names only whatgroom.ymlactually starts") and identifies the deleted file by its originating ticket id (BE-4419) rather than by filename, so nothing dangling remains for a futuregrepto surface while the note still explains why the list is short. Flagging it because it is a deliberate deviation from the literal instruction.Verified
python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v→ 393 tests, OKbash .github/bump-callers/tests/test_paths_contract.sh→ 148 passed, 0 failed; the groom fleet'sall 13 WATCHED_EXEC entries resolve to tracked filesassertion still passes and the entry count is unchangedbash .github/bump-callers/tests/test_preflight.sh→ 445 passed, 0 failedshellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh→ clean;shellcheck -x .github/groom/*.sh→ cleanpython3 .github/workflow-pins/check_workflow_pins.py→ OK, 11 workflows, 156uses:refs SHA-pinnedpython3 .github/agents-md-integrity/check_agents_md.py --root .→ passed (2 pre-existing warnings, unrelated)shellcheck -x .github/lint/check-org-repo-literals.sh && bash .github/lint/check-org-repo-literals.sh→ OKnode --check .github/groom/jail-shim.mjs→ OKgit grep -n key-broker→ no outputgit log -1 --format=%B | git interpret-trailers --parse→Skip-caller-bump: trueResidual
.github/groom/tests/sandbox-tests.shwas run but could not be made green on this host, and that is pre-existing. It fails atFAIL: broker credential proxywithnode: command not foundinside the bwrap jail — this host'snodelives outside the paths the jail bind-mounts. I confirmed the identical failure on the unmodified checkout before my change, so it is environmental, not a regression; CI is the authority for that suite. It is nonetheless a named groom artifact I did not observe passing against this diff. My change to the only file that suite exercises here is a one-word comment edit injail-shim.mjs.## The agent sandboxsection says that before BE-4303 "the three agent steps used a hand-rolledchmod/env -uscrub with the real key in the step env". Those describe the same period differently. With the blockquote gone, only the second account remains, and I left it untouched rather than rewrite history prose I cannot verify. Worth one pass by someone who remembers which is right.paths:negative glob + aWATCHED_PATHSPECSexclude) instead of deleting — note that theWATCHED_PATHSPECSblock that approach needs only exists once ci(bump-callers): exclude tests/ and README.md from the agents-md, coderabbit-config, groom and cursor-review fleets #324 merges, which it has not. I chose deletion because the evidence above shows no consumer, inside or outside this repo.Provenance
git grep -n key-brokeremptyDELIBERATELY ABSENTcomment identifies the deleted proxy by ticket id rather than filename, so thatgit grep -n key-brokerreturns nothing (see "Judgment call");sandbox-tests.shcould not be run green on this host for a pre-existing environmental reason (see "Residual")