feat(pod): add the authenticated pod api command - #8218
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound verb and attestation redesign, but the durable Watch
Suggestions
[DESIGN-REVIEWED] 5979f98 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is in: the contract, the intent file, the full patch, and the repo greps for consumer counts, the existing First-Principles-Verdict: CONCERNS Every pod now binds 127.0.0.1 and the whole unix-socket transport ships undeclared, in a description that claims "No lifecycle changes" — plus a Windows fallback whose beneficiary cannot exist. What this change shipsIntent: let a recipe/agent call a running pod's HTTP API without ever touching its credential — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 5979f98 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've analyzed the diff and the three candidates. Let me verify the key structural fact underpinning all three — whether the weakened All three candidates share one root cause: Falsifying this:
None of (a)/(b)/(c) survives without assuming a specific version-transition sequence. All three drop. Step 2 turned up no independently groundable defect in the changed lines. No findings. [OPUS-REVIEWED] 5979f98 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @pepmach overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
cc576f0 to
9637d9f
Compare
Review disposition —
|
9637d9f to
4fc72bb
Compare
|
|
|
4fc72bb to
727e465
Compare
|
|
727e465 to
301f7ee
Compare
The earlier response to this finding made Branch taken, measured rather than read off the source: The leg the chain actually depends on. This finding needs a stale sidecar — a record naming a PID that has since been recycled onto a foreign process. Listener attribution was only ever standing in for that. So the record now proves its own freshness instead: Fails closed on every way of not knowing: no record, no start-time line (a pre-binding record), or a host that will not report a start time, which covers a Windows box whose query handle cannot be opened. Line 1 of the sidecar is still the bare PID, so Red-first. Three tests failed with Two pre-existing cases encoded the invariant being replaced and were rewritten rather than deleted — Live acceptance at this head: Gates: black, subprocess-encoding, isort, flake8, |
301f7ee to
2c76f94
Compare
Confirmed reachable and fixed. Two things about the mechanism are worth stating, because they are why enumerating exception types kept failing here. Why the previous round's fix did not cover this. The decode sat in the Catching The restructure. The envelope is now the single exit from
A successful request keeps its status and Mutation-checked, so the guards are not vacuous. Narrowing the decode guard back to Validation. 54 passed in |
2c76f94 to
ca2c8bd
Compare
Correct, and the docstring claim was mine — I asserted in an earlier disposition on this PR that line 1 staying the bare pid left Rather than document the skew, the format change is withdrawn. The start identity moved into its own sidecar:
Proved cross-checkout rather than simulated. With a live pod on port 7974, the on-disk pid file was The Validation. 712 passed / 1 skipped across the pod, instances and CLI-help suites; 2105 passed / 24 skipped on the broader pod/seed/instances/platform-compat sweep. black, subprocess-encoding, isort, flake8, |
The prescription is correct and was implemented, not rebutted. Earlier rounds on this span asked for exact IPv4 listener attribution, which is unsatisfiable on this host ( No fallback, structurally. The pod's home, not the host's.
Red-first. Written before the fix. The harness is the load-bearing part: Mutation-checked four ways, each restored after: plain TCP → 6 failed / 9 errors; fallback restored → 1 failed / 1 error ( Live acceptance (mandatory on this PR's attestation path). Pod up on 7974 with Validation. |
|
The |
8a24ca6 to
06c79ca
Compare
06c79ca to
5979f98
Compare
Correct. A pod's gateway is the checkout's own venv binary, so a worktree branched before this change writes no start identity — and restarting it writes none either, which made the advice actively misleading for the one case it was most likely to be read in.
Grepped the repository for other places making the "until restart" claim about this case: line 1750 was the only one. ( Both texts were verified live, not just unit-tested. Against a running pod that was demonstrably serving: removing the (Reposted verbatim from the original disposition of 2026-09-03 22:42 UTC, reformatted only: the readiness disposition gate counts top-level bold bullets as finding records, and the original's two explanatory sub-bullets made one record look like three. One finding, one record, same rationale.) |
Real: Same push also fixed the four Windows shard reds, which were ours: this test module failed collection on Windows ( |
The lazy import defended itself with |
An invalid value for a supplied argument reaches the JSON envelope — that is the settled earlier fix on this file (an unrecognised METHOD prints fixed-key JSON). An absent positional is a different class: |
The prescription ("mint through |
Taking the premise seriously rather than by round-count: the widened window this round adds over the crash-race residual is a pod that is alive but IPv6-bound, leaving First leg: a pod booted by this code cannot be IPv6-bound. Second leg: a legacy pod that could be IPv6-bound cannot attest. A pod booted before this PR predates the What remains is exactly the crash-window residual dispositioned last round: pod exits between attestation and the mint's TCP connect. That is pre-existing ( Seventh consecutive blocking round on this span. Rounds 5–7 each prescribed a different remedy: round 5's was implemented (bind pin), round 6's was owner-ruled to #8552, round 7 re-prescribes round 6's. Per the recorded stall rule, further rounds re-prescribing #8552's content will be escalated to the owner rather than re-argued. |
|
/ai-review override gpt 5979f98: mint-over-TCP residual is owner-ruled to #8552 (widening the token_auth._BYPASS_EXACT gate is its own reviewed change); the IPv6 premise is unreachable here — new pods are bind-pinned to 127.0.0.1 (live-verified) and pre-sidecar pods are refused by the owner-confirmed migration rule before any secret is sent. |
Human judgment recorded@pepmach marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
buluoray
left a comment
There was a problem hiding this comment.
Reviewed against source at 5979f98, weighted toward the authentication path since that is what this adds. 0 blocking findings.
The credential never reaches the caller, and that is the design's best property. The CLI surface is api <wt> <METHOD> <path> [--data JSON] [--allow-write] — no token argument, so nothing lands in ps or shell history. pod_api mints internally via mint_token(cfg, name) at runtime.py:1995. It is not logged either: the audit at :1867 records the credential-free normalized path, and the URLError handler at :2018 deliberately declines to render exception values because urllib would carry the token-bearing full_url. It is scrubbed from the response body in raw, percent, quote_plus and \uXXXX forms.
Transport is the pod's private unix socket with no TCP fallback (unix_socket_urlopen, runtime.py:2004), which is the structural version of the guarantee rather than a check. The code comment states the threat plainly — ordinary loopback can be held by a process that is not the pod, so a minted credential must never be sent there — and an absent socket refuses through the envelope before minting, since mint_token itself spends the pod's .local_secret to obtain one. Nothing to squat.
Fail-closed on ownership, mutation-verified. A missing record, missing start-identity or start-token mismatch all resolve to OWNER_UNPROVEN and mint_token raises before any secret moves. Non-POSIX returns OWNER_UNPROVEN without touching host tools.
| Mutation (source) | Guard | Result |
|---|---|---|
Remove the start-time freshness check in _pod_recorded_pid |
recycled PID cannot attest | RED — assert 'pod' == 'unproven' |
Disable the --allow-write gate |
write refused before host access | RED — the test also catches "guard ran too late" |
Drop the caller token= param refusal |
caller token refused and scrubbed from audit | RED — 5 tests, including the URL-encoded %74oken evasion |
Cross-platform and invariants both hold. The .start / .pid / .secret sidecars go through atomic_write(..., mode=0o600) (mkstemp O_EXCL 0600 + os.replace), not a bare os.chmod under if IS_POSIX — the diff adds zero os.chmod calls, and a code comment shows the author knew why. The keystone sensitive-path and denied-command matchers are untouched. cli.md:241, instances.md §12.2 and pod/README.md are all updated in the same commit.
It also closes an exposure rather than opening one: build_pod_env now pins KIROCREW_BIND=127.0.0.1 (runtime.py:2384), overriding an inherited 0.0.0.0/::1 from the official image. pod api adds no listener — it is a client.
On the MCP-first convention (AGENTS.md), which is the one repo rule this could plausibly trip: I checked rather than assumed, and it is not violated. Zero pod verbs ship as MCP tools — mcp_core.py and mcp_cron.py contain no pod reference at all — so pod is an established CLI-only operator surface and the recently merged pod scenarios (#8127) sets the precedent this verb follows. Worth a human sanity-check only if the team intends pod verbs to become MCP tools as a class; as things stand it is consistent with what ships.
Non-blocking:
- The residual you already ruled on and routed to #8552 is correctly scoped:
mint_tokenstill fetches over TCP to127.0.0.1:<port>, so a squatter winning the crash window captures the host-side.local_secret— not a pod credential, since a fabricated token is only ever handed to the pod's own socket, which rejects it. Pre-existing on main viapod token, and no new fail-open. pid_start_token's Windowsprocess_start_timefallback leg is currently dead-benefit for the pod path: its only reader_pod_recorded_pidis reached throughport_owner, which returnsOWNER_UNPROVENwhennot IS_POSIX. Defensive, not defective.- The body's "No lifecycle changes" line understates the
KIROCREW_BINDpin, which is a real behavior change for an operator who exported a non-loopback bind. Documented in the code comment; the prose is just stale. - Design's suggestion to mint with a one-shot short TTL instead of the 2h default is reasonable hardening.
Not verified: the socket 0600 + pod-home 0700 property this depends on lives in dashboard/server.py and pod provisioning, outside this diff — I confirmed the dependency but did not re-audit the pre-existing creation code. macOS/Windows pid_start_token behavior was not executed (both degrade to "" = unproven, so fail-closed). test_an_unrecognised_method_reaches_the_envelope_not_argparse_prose fails in my worktree on empty-stdout JSONDecodeError, which is a pristine-subprocess capture artifact — running the command by hand emits the correct fixed-key envelope and exits 1, matching your disposition.
Add the authenticated
pod apicommandSlice 3 of the verification-controller plan, and the last pod verb in it. A pod's HTTP API is reachable today only by reading its credential out of the pod's env file and hand-assembling a URL — the step that made every earlier verification recipe fragile. This adds one verb that does it correctly and nothing else.
Output is a fixed-key JSON document on every path —
{name, method, path, status, ok, body}— so a caller never parses prose. A non-2xx response prints the same shape and exits 1, which is what lets a recipe assert onstatusinstead of on stderr text.Why the original implementation was replaced
The version carried on the closed combined branch was dead on this host, and the live smoke on #7841 is what proved it:
lsofalone. On a host without a listener tool the proof could never be produced, so the credential was withheld and the verb always failed. Ownership is now the agreement between the pod's own PID record and systemd'sMainPID; a listener tool, when present, only corroborates. A listener attribution alone never vouches for a pod, and a listener that disagrees with the attested PID marks the port foreign.Bearerheader. The dashboard does not accept that. It now uses the query-credential contract intoken_auth.py, pinned by a test that asserts the header form is not used.Missing, malformed, or stale records stay unproven rather than degrading to a guess, and a non-POSIX host reaches that verdict without touching host tools at all.
Credential hygiene
The credential is minted only after ownership is attested, and it is treated as unprintable from that point:
Safety surface
GETandHEADare the default surface;POST/PUT/PATCH/DELETErequire--allow-write, and the refusal happens before any host access./api/is prepended when absent; methods are case-insensitive.Verification
GET healthreturned 200 through the new verb, a caller-supplied credential was refused without echo, the--allow-writegate was exercised in both directions, teardown left zero residue, and three unrelated pods running on the host were untouched.test/test_pod_api.pycovering every claim above, red-first.--platform linux), harness parity, brand, and docs lint all green.Scope and rebase
Command, ownership attestation, run marker, focused tests, and the two command-doc surfaces. 10 files, +938/−65. No lifecycle changes, no seeding changes, no recipes.
The run-marker reader shared with host gateway ownership now caps PID sidecars at 64 bytes and accepts only positive ASCII decimal. The gateway writes
os.getpid()in that sidecar, so valid gateway lifecycle behavior is unchanged; only malformed or non-contract records are newly refused.Rebased onto current main after #8127 (
pod scenarios) and #8125 (manifest publication) merged. The four conflicts were all union merges in verb tables and doc rows — the subcommandmetavar, the dispatch table, the usage string, and the CLI spec rows — resolved so both verbs are listed; no behavior was renegotiated in the rebase.With this, the pod verb surface planned by the controller judgment is complete: no further verbs. The remaining slice is recipes and discoverability, which composes these verbs with the existing
/api/session-control/*routes.Follow-ups
mint_tokenover the pod's unix socket too. Deliberately not a rider here:api_token_localrefuses AF_UNIX peers today (is_loopback("")isFalse) and sits intoken_auth._BYPASS_EXACT, so admitting unix peers widens a middleware-bypassed auth gate that deserves its own review. Its residual leaks no pod credential; what a port-squatter could capture during the mint window is the host-side.local_secret, which ispod token's pre-existing exposure on main.