fix(review): close the three gaps found in stack review #23

Closed

maximus wants to merge 1 commit from review-fixes into issue-15-local-agent

maximus commented

2026-08-16 18:26:20 +00:00

Owner

Follow-up to the /pr-review pass on the #18-#22 stack. Three findings were
raised, none blocking, none behavioural — each one weakened a guarantee the
stack was meant to provide. All three are closed here, plus a doc claim the
review proved false.

1. The safety net from #12 did not cover the routes #13 added

auth.test.js carries const ROUTES = [...] under the comment "Any new route
must be added here." When #13 introduced GET /hosts and POST /hosts/<id>,
that array was not updated, so no test asserted GET /hosts -> 401 on a
missing Authorization header. Structurally the exposure was near-nil
(checkAuth is a single choke point), but this is exactly the drift class the
file exists to catch, and it slipped on its first outing.

ROUTES now includes /hosts, and a dedicated block pins both directions of
the token separation: a read token cannot write, an ingest token cannot read.

Verified by mutation: collapsing checkAuth to a single expected token turns
72 tests red, including the new ingest-route cases.

2. The oversized-body tests could not fail for the right reason

ingestOversized() mapped any transport error to 413, so all four ceiling
tests would have stayed green if the server had stopped writing the status and
merely destroyed the socket - on the one path where "a status code, not a dead
socket" is the entire client contract.

Transport errors are now surfaced instead of swallowed.

Verified by mutation: replacing the 413 write with a bare req.destroy()
turns 3 tests red with expected 'transport error instead of a status: ...' to be 413. Before this change, that same mutation left them green.

3. `HOST_AGENT_TIMEOUT_MS` was dead under the documented install

push-metrics.js reads it, but run-push.sh exported only HOSTS_API_URL,
HOSTS_INGEST_TOKEN and HOST_ID. Setting it in the env file did nothing and
the agent always ran on its 10s default. It was also absent from the README, so
nothing promised it worked. Now exported and documented.

4. A security claim the review disproved

agent/README.md stated the ingest/read token split protects the Defenseurs
reports if the workstation is stolen. It does not: the ThinkPad already stores
HEALTH_TOKEN in cleartext for the defenseur-auto cron, so losing that laptop
compromises both tokens. The separation is still worth having - independent
rotation, and future hosts that only push never need the read token - but the
containment claim is gone, replaced by the instruction to rotate both together.

Verification

244 -> 251 tests, all green
Both new guards proven non-vacuous by mutation (see above)
No production code changed: the diff is two test files, one shell wrapper
export, and one README

Generated autonomously by /autopilot run of 2026-08-16 (review follow-up)

Follow-up to the `/pr-review` pass on the #18-#22 stack. Three findings were raised, none blocking, none behavioural — each one weakened a guarantee the stack was meant to provide. All three are closed here, plus a doc claim the review proved false. ## 1. The safety net from #12 did not cover the routes #13 added `auth.test.js` carries `const ROUTES = [...]` under the comment *"Any new route must be added here."* When #13 introduced `GET /hosts` and `POST /hosts/<id>`, that array was not updated, so **no test asserted `GET /hosts` -> 401 on a missing Authorization header**. Structurally the exposure was near-nil (`checkAuth` is a single choke point), but this is exactly the drift class the file exists to catch, and it slipped on its first outing. `ROUTES` now includes `/hosts`, and a dedicated block pins both directions of the token separation: a read token cannot write, an ingest token cannot read. Verified by mutation: collapsing `checkAuth` to a single expected token turns **72 tests red**, including the new ingest-route cases. ## 2. The oversized-body tests could not fail for the right reason `ingestOversized()` mapped any transport error to `413`, so all four ceiling tests would have stayed green if the server had stopped writing the status and merely destroyed the socket - on the one path where *"a status code, not a dead socket"* is the entire client contract. Transport errors are now surfaced instead of swallowed. Verified by mutation: replacing the `413` write with a bare `req.destroy()` turns **3 tests red** with `expected 'transport error instead of a status: ...' to be 413`. Before this change, that same mutation left them green. ## 3. `HOST_AGENT_TIMEOUT_MS` was dead under the documented install `push-metrics.js` reads it, but `run-push.sh` exported only `HOSTS_API_URL`, `HOSTS_INGEST_TOKEN` and `HOST_ID`. Setting it in the env file did nothing and the agent always ran on its 10s default. It was also absent from the README, so nothing promised it worked. Now exported and documented. ## 4. A security claim the review disproved `agent/README.md` stated the ingest/read token split protects the Defenseurs reports if the workstation is stolen. It does not: the ThinkPad already stores `HEALTH_TOKEN` in cleartext for the `defenseur-auto` cron, so losing that laptop compromises both tokens. The separation is still worth having - independent rotation, and future hosts that only push never need the read token - but the containment claim is gone, replaced by the instruction to rotate both together. ## Verification - 244 -> **251 tests**, all green - Both new guards proven non-vacuous by mutation (see above) - No production code changed: the diff is two test files, one shell wrapper export, and one README Generated autonomously by /autopilot run of 2026-08-16 (review follow-up)

maximus added 1 commit 2026-08-16 18:26:20 +00:00

fix(review): close the three gaps found in stack review e181a9691c

Follow-up to the /pr-review pass on PRs #18-#22. Three findings, none of
which changed behaviour, all of which weakened a guarantee the stack was
supposed to provide.

1. auth.test.js carried "any new route must be added here" but /hosts was
   never added when #13 introduced it, so no test asserted GET /hosts -> 401
   on a missing header. The drift class the file exists to catch slipped on
   its first outing. ROUTES now covers /hosts, and a dedicated block pins
   both directions of the token separation: a read token cannot write, an
   ingest token cannot read.

2. ingestOversized() folded any transport error into 413, so the four
   oversized-body tests would have stayed green if the server had stopped
   writing the status and merely killed the socket - on the one path where
   "a status, not a dead socket" is the whole client contract. Transport
   errors are now surfaced instead of swallowed.

3. HOST_AGENT_TIMEOUT_MS was read by push-metrics.js but never exported by
   run-push.sh, so setting it in the documented env file did nothing. Now
   exported and documented.

Also drops a claim from agent/README.md that the review proved false: the
ingest/read token split buys no containment on the ThinkPad, which already
stores HEALTH_TOKEN in cleartext for defenseur-auto. Losing that laptop
compromises both, so they rotate together.

maximus commented

2026-08-16 18:27:39 +00:00

Author

Owner

Mergee localement sur main en fast-forward (pile chainee : l API de merge Forgejo ne peut pas traiter une pile dont la base n est pas main). Tip integre : e181a96. Forgejo ne detecte pas un merge ff local, donc cette PR est fermee a la main — le code EST sur main.

Mergee localement sur `main` en fast-forward (pile chainee : l API de merge Forgejo ne peut pas traiter une pile dont la base n est pas `main`). Tip integre : `e181a96`. Forgejo ne detecte pas un merge ff local, donc cette PR est fermee a la main — le code EST sur `main`.