[PR #72] [MERGED] feat(groom): add key-broker.mjs — localhost header-injecting API-key proxy + unit tests (BE-4419) #81

New issue

Closed

opened 2026-10-26 22:36:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:02 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/72
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4419-key-broker

📝 Commits (5)

94d57b8 feat(groom): add key-broker.mjs — localhost header-injecting API-key proxy + unit tests (BE-4419)
1241e1f fix(groom): harden key-broker stream teardown, timeouts, and config guards (BE-4419)
55081e2 test(groom): add regression test for stdin key trimming (BE-4419)
dda9133 Merge remote-tracking branch 'origin/main' into matt/be-4419-key-broker
8f3c8ad Merge remote-tracking branch 'origin/main' into matt/be-4419-key-broker

📊 Changes

3 files changed (+639 additions, -1 deletions)

View changed files

📝 .github/groom/README.md (+51 -1)
➕ .github/groom/key-broker.mjs (+214 -0)
➕ .github/groom/tests/test_key_broker.py (+374 -0)

📄 Description

ELI-5

The groom agent that cleans up code runs on a shared runner where it can read any file and any process's environment. If we handed it the real Anthropic API key, it could just read the key straight out of /proc. So instead we run a tiny local doorman (key-broker.mjs): it holds the real key, the agent gets a fake one, and the agent points the Claude CLI at the doorman (ANTHROPIC_BASE_URL=http://127.0.0.1:<port>). Every real request passes through the doorman, which swaps the fake key for the real one before sending it upstream. The key only ever arrives on the doorman's stdin — never in its environment or command line — so it can't be recovered from /proc. This PR adds the doorman script and its tests only; wiring it into groom.yml is a sibling ticket (BE-4311).

What this adds

.github/groom/key-broker.mjs — a dependency-free (node:http/node:https, node ≥18, ESM) localhost proxy that reads the real key from stdin's first line, listens on 127.0.0.1:<port>, answers the CLI's HEAD/GET / connectivity probe locally with 200, forwards /v1/* to the upstream (streaming both directions so SSE works) after stripping inbound x-api-key/authorization/host and injecting the real x-api-key + upstream host, returns 404 for anything else, and 502 (static body) on an upstream connection error. Config via GROOM_BROKER_PORT (default 8199) and GROOM_BROKER_UPSTREAM (default https://api.anthropic.com, overridable for tests). It never logs headers/bodies — at most method path -> status.
.github/groom/tests/test_key_broker.py — a unittest integration suite that boots the real script under node against a local fake upstream (skipped when node is absent). Discovery picks it up automatically via the existing test-groom-scripts.yml.
A key-broker.mjs section in .github/groom/README.md (what it's for, the stdin-not-env rule and why, the env knobs, and the note that groom.yml wiring lands in BE-4311).

Tests

Covers every case the ticket enumerates, plus a startup guard:

injects the real key + strips inbound x-api-key/authorization, and round-trips a non-200 upstream status and body;
request body reaches the upstream byte-for-byte;
HEAD / → 200, not forwarded;
GET /anything-else → 404, not forwarded;
the broker process's /proc/<pid>/{environ,cmdline} do not contain the real key (Linux-only, skipped elsewhere — so it exercises in CI on ubuntu-latest, skips on the macOS dev box);
upstream down → 502 and the broker stays alive for a subsequent request;
(extra) empty/absent stdin key → non-zero exit.

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v → Ran 66 tests ... OK (skipped=1) locally (the 1 skip is the Linux /proc assertion on macOS).

Acceptance criteria

✅ The unittest discovery passes locally (and is expected to pass in CI — ubuntu-latest ships node, so the node-gated class runs there and the /proc assertion is live).
✅ grep -c ANTHROPIC .github/groom/key-broker.mjs → 0 (the key is never read from env/argv; the only env reads are GROOM_BROKER_PORT/GROOM_BROKER_UPSTREAM). The comments were deliberately worded to avoid the provider token so this stays literally 0.
✅ No changes to .github/workflows/groom.yml (nor any workflow file — test-groom-scripts.yml already triggers on .github/groom/** and ubuntu-latest's preinstalled node is sufficient, so no setup-node step was needed).

Judgment calls / notes

Resumed prior crash residue. An untracked, near-complete key-broker.mjs from an earlier interrupted attempt was on the branch; I reviewed it adversarially, rebased the work onto the current origin/main, and kept the sound implementation. The one substantive fix: the residue's header comment falsely claimed grep -c ANTHROPIC "is 0" while its own comments contained the token (actual count was 3) — reworded so the count is genuinely 0.
Added client-side stream-error guards (req/res/upstreamRes error handlers) beyond the ticket's enumerated branches: a CLI that aborts mid-stream would otherwise throw an unhandled stream error and crash this long-lived proxy. These no-op/teardown handlers don't change the specified happy path.
Test hardening: the fake upstream overrides server_bind to skip HTTPServer's socket.getfqdn() reverse-DNS lookup, which hung ~35s per test on the dev box (and is a general flake risk); the suite now runs in ~3s.
Negative-claim falsification (self-review clause e): N/A. The 404/502/startup-die paths are a proxy's correct protocol behavior, not a user-facing capability denial (no "X not supported" dead-end, no product surface to probe), so the falsification discipline doesn't apply here.

_{🔄 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/72 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4419-key-broker` --- ### 📝 Commits (5) - [`94d57b8`](https://github.com/Comfy-Org/github-workflows/commit/94d57b82ec0b9f3351909196eb8f61f5616202c9) feat(groom): add key-broker.mjs — localhost header-injecting API-key proxy + unit tests (BE-4419) - [`1241e1f`](https://github.com/Comfy-Org/github-workflows/commit/1241e1f32eb1197b7fba5fbe974db31ecc0b8a2a) fix(groom): harden key-broker stream teardown, timeouts, and config guards (BE-4419) - [`55081e2`](https://github.com/Comfy-Org/github-workflows/commit/55081e200666b19d8484821f3d7b227d5a7467bf) test(groom): add regression test for stdin key trimming (BE-4419) - [`dda9133`](https://github.com/Comfy-Org/github-workflows/commit/dda9133c32768ab2e19b846939e3d0d317d04848) Merge remote-tracking branch 'origin/main' into matt/be-4419-key-broker - [`8f3c8ad`](https://github.com/Comfy-Org/github-workflows/commit/8f3c8ade59bfa1808c7900b2366bfad149ca9a51) Merge remote-tracking branch 'origin/main' into matt/be-4419-key-broker ### 📊 Changes **3 files changed** (+639 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+51 -1) ➕ `.github/groom/key-broker.mjs` (+214 -0) ➕ `.github/groom/tests/test_key_broker.py` (+374 -0) </details> ### 📄 Description ## ELI-5 The groom agent that cleans up code runs on a shared runner where it can read any file and any process's environment. If we handed it the real Anthropic API key, it could just read the key straight out of `/proc`. So instead we run a tiny local doorman (`key-broker.mjs`): it holds the real key, the agent gets a fake one, and the agent points the Claude CLI at the doorman (`ANTHROPIC_BASE_URL=http://127.0.0.1:<port>`). Every real request passes through the doorman, which swaps the fake key for the real one before sending it upstream. The key only ever arrives on the doorman's stdin — never in its environment or command line — so it can't be recovered from `/proc`. This PR adds the doorman script and its tests only; wiring it into `groom.yml` is a sibling ticket (BE-4311). ## What this adds - `.github/groom/key-broker.mjs` — a dependency-free (`node:http`/`node:https`, node ≥18, ESM) localhost proxy that reads the real key from **stdin's first line**, listens on `127.0.0.1:<port>`, answers the CLI's `HEAD`/`GET /` connectivity probe locally with `200`, forwards `/v1/*` to the upstream (streaming both directions so SSE works) after stripping inbound `x-api-key`/`authorization`/`host` and injecting the real `x-api-key` + upstream `host`, returns `404` for anything else, and `502` (static body) on an upstream connection error. Config via `GROOM_BROKER_PORT` (default `8199`) and `GROOM_BROKER_UPSTREAM` (default `https://api.anthropic.com`, overridable for tests). It never logs headers/bodies — at most `method path -> status`. - `.github/groom/tests/test_key_broker.py` — a `unittest` integration suite that boots the real script under `node` against a local fake upstream (skipped when `node` is absent). Discovery picks it up automatically via the existing `test-groom-scripts.yml`. - A `key-broker.mjs` section in `.github/groom/README.md` (what it's for, the stdin-not-env rule and why, the env knobs, and the note that `groom.yml` wiring lands in BE-4311). ## Tests Covers every case the ticket enumerates, plus a startup guard: - injects the real key + strips inbound `x-api-key`/`authorization`, and round-trips a non-200 upstream status and body; - request body reaches the upstream byte-for-byte; - `HEAD /` → `200`, not forwarded; - `GET /anything-else` → `404`, not forwarded; - the broker process's `/proc/<pid>/{environ,cmdline}` do not contain the real key (Linux-only, skipped elsewhere — so it exercises in CI on `ubuntu-latest`, skips on the macOS dev box); - upstream down → `502` and the broker stays alive for a subsequent request; - (extra) empty/absent stdin key → non-zero exit. `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` → `Ran 66 tests ... OK (skipped=1)` locally (the 1 skip is the Linux `/proc` assertion on macOS). ## Acceptance criteria - ✅ The unittest discovery passes locally (and is expected to pass in CI — `ubuntu-latest` ships `node`, so the `node`-gated class runs there and the `/proc` assertion is live). - ✅ `grep -c ANTHROPIC .github/groom/key-broker.mjs` → `0` (the key is never read from env/argv; the only env reads are `GROOM_BROKER_PORT`/`GROOM_BROKER_UPSTREAM`). The comments were deliberately worded to avoid the provider token so this stays literally 0. - ✅ No changes to `.github/workflows/groom.yml` (nor any workflow file — `test-groom-scripts.yml` already triggers on `.github/groom/**` and `ubuntu-latest`'s preinstalled node is sufficient, so no `setup-node` step was needed). ## Judgment calls / notes - **Resumed prior crash residue.** An untracked, near-complete `key-broker.mjs` from an earlier interrupted attempt was on the branch; I reviewed it adversarially, rebased the work onto the current `origin/main`, and kept the sound implementation. The one substantive fix: the residue's header comment falsely claimed `grep -c ANTHROPIC` "is 0" while its own comments contained the token (actual count was 3) — reworded so the count is genuinely 0. - **Added client-side stream-error guards** (`req`/`res`/`upstreamRes` `error` handlers) beyond the ticket's enumerated branches: a CLI that aborts mid-stream would otherwise throw an unhandled stream `error` and crash this long-lived proxy. These no-op/teardown handlers don't change the specified happy path. - **Test hardening:** the fake upstream overrides `server_bind` to skip `HTTPServer`'s `socket.getfqdn()` reverse-DNS lookup, which hung ~35s per test on the dev box (and is a general flake risk); the suite now runs in ~3s. - **Negative-claim falsification (self-review clause e): N/A.** The `404`/`502`/startup-`die` paths are a proxy's correct protocol behavior, not a user-facing capability denial (no "X not supported" dead-end, no product surface to probe), so the falsification discipline doesn't apply here. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:02 +00:00