Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #326] [MERGED] chore(groom): delete the superseded key-broker.mjs loopback proxy and its test #326

New issue

Closed

opened 2026-10-26 22:38:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/326
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 9/20/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15722-delete-key-broker

📝 Commits (4)

91ea9a7 chore(groom): delete the superseded key-broker.mjs loopback proxy (BE-4419 -> BE-4303)
fac03b9 docs(bump-callers): state the WATCHED_EXEC rule as the run-time test, not "what groom.yml starts"
a1db233 fix(groom): refresh the apt index before installing bubblewrap in preflight
ef6a446 Merge remote-tracking branch 'origin/main' into matt/be-15722-delete-key-broker

📊 Changes

5 files changed (+9 additions, -653 deletions)

View changed files

📝 .github/groom/README.md (+1 -60)
📝 .github/groom/jail-shim.mjs (+1 -1)
➖ .github/groom/key-broker.mjs (+0 -214)
➖ .github/groom/tests/test_key_broker.py (+0 -374)
📝 .github/workflows/bump-groom-callers.yml (+7 -4)

📄 Description

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 uses broker.mjs over 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

Deleted .github/groom/key-broker.mjs and .github/groom/tests/test_key_broker.py.
.github/groom/README.md — removed the key-broker.mjs — the localhost API-key proxy section (heading through its groom.yml wiring blockquote), and dropped the sentence in the tests/ bullet about key-broker tests booting the real script under node.
.github/workflows/bump-groom-callers.yml — rewrote the DELIBERATELY ABSENT comment above WATCHED_EXEC. WATCHED_EXEC, WATCHED_ASSETS and the paths: 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.
The commit carries Skip-caller-bump: true as its sole trailing trailer (verified with git interpret-trailers --parse), so the removal does not itself fan a churn bump. Please keep that trailer intact in the squash-merge message — preflight.sh reads 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.mjs hand-off is described, that paragraph had to move rather than die. It is not. ## The agent sandbox — agent-sandbox.sh + broker.mjs already describes the per-job composition end to end — the broker step holding secrets.ANTHROPIC_API_KEY and starting broker.mjs on $BROKER_SOCK, the sandbox preflight, the agent step running with a DUMMY key and ANTHROPIC_BASE_URL pointed at the in-jail jail-shim.mjs, the literal-key scan/capture tripwires, and the always() 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 run nohup node "$GROOM_ASSETS/broker.mjs" "$BROKER_SOCK" (lines 1501, 2001, 2941). No reference to the deleted file anywhere in groom.yml.
agent-sandbox.sh and jail-shim.mjs reference only broker.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 for key_broker returned nothing.
git log -- .github/groom/key-broker.mjs → one commit, the one that added it. Nothing has touched it since.
The file was never in WATCHED_EXEC, so its deletion cannot trip preflight.sh's decommission probe, and WATCHED_ASSETS (.github/groom) still resolves.
Its GROOM_BROKER_PORT / GROOM_BROKER_UPSTREAM env knobs and port 8199 now appear nowhere in the tree, so removing the README's knob table orphans no live configuration.
Post-change git grep -n key-broker returns nothing.

Judgment call

The ticket asked for two things that cannot both hold: rewrite the DELIBERATELY ABSENT comment to name key-broker.mjs as removed, and have git grep -n key-broker return nothing. I resolved in favour of the mechanical check — the rewritten comment states the rule ("this list names only what groom.yml actually starts") and identifies the deleted file by its originating ticket id (BE-4419) rather than by filename, so nothing dangling remains for a future grep to 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, OK
bash .github/bump-callers/tests/test_paths_contract.sh → 148 passed, 0 failed; the groom fleet's all 13 WATCHED_EXEC entries resolve to tracked files assertion still passes and the entry count is unchanged
bash .github/bump-callers/tests/test_preflight.sh → 445 passed, 0 failed
shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh → clean; shellcheck -x .github/groom/*.sh → clean
python3 .github/workflow-pins/check_workflow_pins.py → OK, 11 workflows, 156 uses: refs SHA-pinned
python3 .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 → OK
node --check .github/groom/jail-shim.mjs → OK
git grep -n key-broker → no output
git log -1 --format=%B | git interpret-trailers --parse → Skip-caller-bump: true

Residual

.github/groom/tests/sandbox-tests.sh was run but could not be made green on this host, and that is pre-existing. It fails at FAIL: broker credential proxy with node: command not found inside the bwrap jail — this host's node lives 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 in jail-shim.mjs.
A pre-existing doc inconsistency is now more exposed, and I did not fix it (out of scope). The deleted blockquote said the pre-BE-4303 state was the BE-4311 loopback-TCP broker with a dummy key; the surviving ## The agent sandbox section says that before BE-4303 "the three agent steps used a hand-rolled chmod/env -u scrub 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.
The fallback path in the source request was not taken and remains available. If a reviewer would rather keep the proxy as a documented standalone tool, the alternative is to exclude it from the groom fleet (paths: negative glob + a WATCHED_PATHSPECS exclude) instead of deleting — note that the WATCHED_PATHSPECS block that approach needs only exists once #324 merges, which it has not. I chose deletion because the evidence above shows no consumer, inside or outside this repo.

Provenance

Authored by: agent-work loop
Verified: groom unittest suite 393 passed / 0 failed; test_paths_contract.sh 148 passed / 0 failed; test_preflight.sh 445 passed / 0 failed; check_workflow_pins.py OK (156 refs pinned); check_agents_md.py passed (2 pre-existing warnings); check-org-repo-literals.sh OK; shellcheck clean on bump-callers + groom shell; git grep -n key-broker empty
Deviations: the DELIBERATELY ABSENT comment identifies the deleted proxy by ticket id rather than filename, so that git grep -n key-broker returns nothing (see "Judgment call"); sandbox-tests.sh could not be run green on this host for a pre-existing environmental reason (see "Residual")

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/326 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15722-delete-key-broker` --- ### 📝 Commits (4) - [`91ea9a7`](https://github.com/Comfy-Org/github-workflows/commit/91ea9a7beeeacaa39587c1f1aea9046942dea083) chore(groom): delete the superseded key-broker.mjs loopback proxy (BE-4419 -> BE-4303) - [`fac03b9`](https://github.com/Comfy-Org/github-workflows/commit/fac03b9339239a8eb7a4f1c8c91a02776ca10f01) docs(bump-callers): state the WATCHED_EXEC rule as the run-time test, not "what groom.yml starts" - [`a1db233`](https://github.com/Comfy-Org/github-workflows/commit/a1db233b9ed574828e4b6a9216d7e5e2f15eaf62) fix(groom): refresh the apt index before installing bubblewrap in preflight - [`ef6a446`](https://github.com/Comfy-Org/github-workflows/commit/ef6a4461b6939b62800f104f57fc311a727fe44c) Merge remote-tracking branch 'origin/main' into matt/be-15722-delete-key-broker ### 📊 Changes **5 files changed** (+9 additions, -653 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+1 -60) 📝 `.github/groom/jail-shim.mjs` (+1 -1) ➖ `.github/groom/key-broker.mjs` (+0 -214) ➖ `.github/groom/tests/test_key_broker.py` (+0 -374) 📝 `.github/workflows/bump-groom-callers.yml` (+7 -4) </details> ### 📄 Description ## 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 uses `broker.mjs` over 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 - **Deleted** `.github/groom/key-broker.mjs` and `.github/groom/tests/test_key_broker.py`. - **`.github/groom/README.md`** — removed the `key-broker.mjs — the localhost API-key proxy` section (heading through its `groom.yml wiring` blockquote), and dropped the sentence in the `tests/` bullet about key-broker tests booting the real script under `node`. - **`.github/workflows/bump-groom-callers.yml`** — rewrote the `DELIBERATELY ABSENT` comment above `WATCHED_EXEC`. `WATCHED_EXEC`, `WATCHED_ASSETS` and the `paths:` 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. - The commit carries `Skip-caller-bump: true` as its sole trailing trailer (verified with `git interpret-trailers --parse`), so the removal does not itself fan a churn bump. **Please keep that trailer intact in the squash-merge message** — `preflight.sh` reads 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.mjs` hand-off is described, that paragraph had to move rather than die. It is not. `## The agent sandbox — agent-sandbox.sh + broker.mjs` already describes the per-job composition end to end — the broker step holding `secrets.ANTHROPIC_API_KEY` and starting `broker.mjs` on `$BROKER_SOCK`, the sandbox preflight, the agent step running with a DUMMY key and `ANTHROPIC_BASE_URL` pointed at the in-jail `jail-shim.mjs`, the literal-key scan/capture tripwires, and the `always()` 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 run `nohup node "$GROOM_ASSETS/broker.mjs" "$BROKER_SOCK"` (lines 1501, 2001, 2941). No reference to the deleted file anywhere in `groom.yml`. - `agent-sandbox.sh` and `jail-shim.mjs` reference only `broker.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 for `key_broker` returned nothing. - `git log -- .github/groom/key-broker.mjs` → one commit, the one that added it. Nothing has touched it since. - The file was never in `WATCHED_EXEC`, so its deletion cannot trip `preflight.sh`'s decommission probe, and `WATCHED_ASSETS` (`.github/groom`) still resolves. - Its `GROOM_BROKER_PORT` / `GROOM_BROKER_UPSTREAM` env knobs and port `8199` now appear nowhere in the tree, so removing the README's knob table orphans no live configuration. - Post-change `git grep -n key-broker` returns nothing. ## Judgment call The ticket asked for two things that cannot both hold: rewrite the `DELIBERATELY ABSENT` comment to *name* `key-broker.mjs` as removed, **and** have `git grep -n key-broker` return nothing. I resolved in favour of the mechanical check — the rewritten comment states the rule ("this list names only what `groom.yml` actually starts") and identifies the deleted file by its originating ticket id (BE-4419) rather than by filename, so nothing dangling remains for a future `grep` to 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, OK - `bash .github/bump-callers/tests/test_paths_contract.sh` → 148 passed, 0 failed; the groom fleet's `all 13 WATCHED_EXEC entries resolve to tracked files` assertion still passes and the entry count is unchanged - `bash .github/bump-callers/tests/test_preflight.sh` → 445 passed, 0 failed - `shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh` → clean; `shellcheck -x .github/groom/*.sh` → clean - `python3 .github/workflow-pins/check_workflow_pins.py` → OK, 11 workflows, 156 `uses:` refs SHA-pinned - `python3 .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` → OK - `node --check .github/groom/jail-shim.mjs` → OK - `git grep -n key-broker` → no output - `git log -1 --format=%B | git interpret-trailers --parse` → `Skip-caller-bump: true` ## Residual - **`.github/groom/tests/sandbox-tests.sh` was run but could not be made green on this host, and that is pre-existing.** It fails at `FAIL: broker credential proxy` with `node: command not found` inside the bwrap jail — this host's `node` lives 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 in `jail-shim.mjs`. - **A pre-existing doc inconsistency is now more exposed, and I did not fix it (out of scope).** The deleted blockquote said the pre-BE-4303 state was the BE-4311 loopback-TCP broker with a dummy key; the surviving `## The agent sandbox` section says that before BE-4303 "the three agent steps used a hand-rolled `chmod`/`env -u` scrub 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. - **The fallback path in the source request was not taken and remains available.** If a reviewer would rather keep the proxy as a documented standalone tool, the alternative is to exclude it from the groom fleet (`paths:` negative glob + a `WATCHED_PATHSPECS` exclude) instead of deleting — note that the `WATCHED_PATHSPECS` block that approach needs only exists once #324 merges, which it has not. I chose deletion because the evidence above shows no consumer, inside or outside this repo. ## Provenance - **Authored by:** agent-work loop - **Verified:** groom unittest suite 393 passed / 0 failed; test_paths_contract.sh 148 passed / 0 failed; test_preflight.sh 445 passed / 0 failed; check_workflow_pins.py OK (156 refs pinned); check_agents_md.py passed (2 pre-existing warnings); check-org-repo-literals.sh OK; shellcheck clean on bump-callers + groom shell; `git grep -n key-broker` empty - **Deviations:** the `DELIBERATELY ABSENT` comment identifies the deleted proxy by ticket id rather than filename, so that `git grep -n key-broker` returns nothing (see "Judgment call"); `sandbox-tests.sh` could not be run green on this host for a pre-existing environmental reason (see "Residual") --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:33 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#326

No description provided.

Rows
Columns