mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
main
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
524469b7a3 |
test: no test request goes through the process-global default transport (BUG-3008, closes BUG-2949) (#1324)
Closes BUG-3008. Closes BUG-2949, which filed this failure on 09-07 with the
mechanism explicitly undiagnosed and stopped in the right place: it refuted the
obvious candidate, noted a repo-wide grep found no other caller, and listed
three hypotheses. This is hypothesis (1), and the caller is the standard
library rather than this repo.
httptest.Server.Close() calls CloseIdleConnections() on the PROCESS-WIDE
http.DefaultTransport — deliberately, and the standard library says so in its
own comment, calling it "not part of httptest.Server's correctness". So in a
package where many tests each stand up a server, every `defer ts.Close()`
mutates state every other test's requests depend on. CI observed a request
failing in one test while the server that closed belonged to another:
client_stream_identity_test.go:135: request not sendable:
Get "http://127.0.0.1:45521/api/v1/events/stream?armed=true":
net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called
SCOPE is every test request in internal/server, not just the parallel ones.
Three revisions were needed to get the class right, and each earlier boundary
was wrong in a way worth recording:
- Direct call sites in test bodies missed SHARED HELPERS. connectSSE,
apiRequest and readRawSSEFramesAuthed sent through the default transport
and have parallel callers; connectSSE holds its request open for a whole
test. A helper cannot know which of its callers is exposed, so it no
longer tries to.
- "Serial tests are safe" is true but is the wrong boundary. Serial-ness is
one t.Parallel() away, and CONVE-2086 tells the next author to add exactly
that line.
- A nil Transport IS http.DefaultTransport. listen_serve_test.go's health
probe was exposed identically and appears in no grep for DefaultClient.
internal/cli is deliberately different: its tests drive the PRODUCTION client
from NewClientFromURL, which has a nil Transport by design, so those sends
cannot be isolated without changing the shipped CLI. There the boundary really
is parallel-vs-serial, and the two parallel tests that build their own requests
use srv.Client(). The asymmetry is written into isolatedTestClient's doc
comment rather than left to be inferred.
WHAT IS NOT CLAIMED: Transport.CloseIdleConnections documents that it "does not
interrupt any connections currently in use", and the idle pool is mutated under
idleMu. The interleaving by which errCloseIdleConns reached a caller's Do() is
NOT reconstructed, and the comment says so instead of inventing one. That
unknown is the argument for isolation over a narrower repair: a client with its
own transport is out of reach whichever window it was.
CONVE-2086 amended with the rule, so the convention that creates the exposure
also carries it.
14 files, all _test.go. Codex: four rounds, CLEAN on the fourth; rounds 1-3
each found a real defect in the class boundary or in the comment's claims about
net/http. Gates on
|
||
|
xarmian
|
e40df6b31c |
feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) (#1149)
* feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) The S2 CLI contract S3's plugin skills and S4's web composer build against. S1 gated push delivery on a server-side armed bit declared at stream connect; nothing decided WHETHER to arm or sent the declaration. S2 adds both, defaulting off everywhere. - ResolveAutoArm (internal/cli/arm_consent.go): pure consent resolver. .pad.toml [push] auto_arm is the only per-repo enabler (D4); a per-user config auto_arm=false vetoes it (deny-wins); default off. Config surfaces: PadToml.Push.AutoArm + config.Config.Push.AutoArm (*bool, unset != false), both nil-safe. - Wire contract: StreamSessionIdentity.Armed sends ?armed=true on the event stream — S1's server gate finally has a sender. The monitor announces armed = live local arm OR resolved auto_arm, so a repo opt-in works end to end with a safe default-off skew. - Verbs pad session arm/disarm/status: arm/disarm manage a per-session local arm-state file; status reports the resolved local/auto decision plus the server's own armed/connected counts (new Client.ListSessions), degrading gracefully when padd is unreachable. - Arm-state file (session_arm_state.go): keyed per session by CLAUDE_CODE_MESSAGING_SOCKET (cwd fallback for headless, secondary to auto_arm). Mandatory liveness — a dead-owner file (socket vanished / pid gone) reads as disarmed and is reaped, so a crashed session can never arm a future monitor. Local client state only; the server's armed bit stays the sole delivery authority. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R1 on push-consent (fail-closed config, owner-identity liveness) - HIGH-1: user config.toml read now fails CLOSED. config.LoadPushConfigAutoArm reads the [push] auto_arm value strictly — absent → no opinion, but present-but-unparseable → error — and ResolveAutoArmFromDisk refuses to auto-arm when it can't confirm the user's veto (was: swallowed by the lenient config.Load and treated as no-opinion). - HIGH-2: arm-state liveness now checks owner IDENTITY, not just presence. Socket-keyed files record the socket's mtime and require an exact match, so a reused socket path can't revive a stale file. Headless files record a Linux /proc start-time token (portable fallback documented) to reject a reused pid. - MED-1: arm-state writes are atomic (temp + rename) and reaping is non-destructive (re-checks staleness before removing) — a concurrent re-arm is never clobbered. - MED-2: pad session status applies the .pad.toml URL override, so it queries the same server the monitor connects to. - LOW: malformed arm-state files are now reaped (safe now that writes are atomic — a corrupt file can't be a torn in-progress write). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R2 on push-consent (atomic config write, stronger owner identity) - HIGH-1: Config.Save() is now atomic (temp + rename), so a monitor reconnecting while `pad configure` rewrites config.toml can't read a truncated/partial file, miss a [push] auto_arm=false veto, and arm. - finding 2: socket owner identity now uses inode+device (unix) as the primary signal, with mtime as the non-unix fallback — a rebound socket or a lingering stale node at the same path gets a new inode and is rejected, closing the mtime-collision / reused-node gaps. - finding 3: headless liveness fails closed when a proc-start token was recorded but can't be re-verified (was: fell back to bare pid-liveness, which a reused pid passes); zombies (state 'Z') now report not-alive. - finding 5: `pad session status` applies an explicit --url override too, not just the .pad.toml one. - finding 4 (connect-time TOCTOU): documented as an accepted, bounded residual — a disarm racing an in-flight connect is corrected on the next reconnect; fully closing it needs S3's server-side disarm-on-open signal. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
c84cf7437c |
feat(sessions): announce session identity on the event stream (PLAN-2558 S2, TASK-2560) (#1094)
* feat(sessions): announce session identity on the event stream (TASK-2560) PLAN-2558 S2. S1 gave the presence registry a count of anonymous uuids; this makes each row nameable, which is what S3 needs for an honest empty state and S5 needs for a target picker. A monitor now announces itself when it opens the stream: X-Pad-Session-Label (the working directory's basename) and X-Pad-Session-Pid. The server sanitizes both and stores them on the LiveSession; GET /api/v1/sessions returns them. TRANSPORT. The task body sketched "the stream connect carries it" without picking a mechanism and explicitly left the call open. Headers, because a query param would put the label and pid into every access-log line (this server logs path= for each request) and any proxy log in front of it — which is the same "don't let local detail travel further than it needs to" the privacy line below is about — and a separate registration POST would need its own correlation to the connection it describes, plus a matching lifecycle, when the registry entry already lives and dies with the stream. Headers ride the request that exists and sit alongside Last-Event-ID, already doing this job on this endpoint. Cost, written into the code rather than discovered later: a browser EventSource cannot set headers, so a future web-tab consumer needs a deliberate query-param fallback or a fetch-based SSE reader. PRIVACY. The basename crosses, never the full cwd — "/home/dave/Dev/ docapp" additionally hands over a home directory and usually an account name for no gain — and messaging_socket_path never leaves the machine. Pinned by a test rather than by the implementation being one line. WHAT THIS DELIBERATELY DOESN'T DO: read ~/.pad/sessions/. The task framed S2 as giving `pad session register` its first consumer, and the monitor cannot honestly be one. Registry entries are written by whatever process ran that command — a different pid — and the only matchable fields are pid and cwd, so two agent sessions in one checkout are indistinguishable and "pick the newest" is a coin flip that would put a confident wrong name in the S5 picker. Process ancestry settles it exactly and is platform-specific (this binary ships for macOS and Windows). The monitor's own cwd basename and pid are never wrong and answer the question the label exists to answer; correlating a stream to the agent session that spawned it needs an identifier the harness passes down, which is worth doing when something needs it and worth not faking until then. Also moves S1's STALENESS doc block, which sat above LiveSession.Label where it read as documenting the name rather than the whole entry. Tests: sanitizer units (whitespace collapse, control-char stripping, rune-not-byte truncation), header wiring, the end-to-end labelled session, the unannounced-client compatibility leg (a pre-S2 monitor must still register and still stream), a hostile-input leg over the wire, the client's omit-when-unset behaviour, and the basename promise. Measured rather than assumed: Go's server answers 400 to a header value containing a control byte before any handler runs (verified with a raw socket, since Go's own client refuses to send one and the two refusals are indistinguishable from a normal client test). So that arm of the sanitizer is unreachable over HTTP; it stays as defence in depth for the next caller in, and both the comment and the wire test say so instead of the test quietly passing because the transport refused the input. Mutation-tested four ways, each revert grep-verified: handler ignoring the parsed identity, monitor sending the full cwd, dropping the truncation, and the client always setting the headers. Refs TASK-2560, PLAN-2558 * fix(cli): sanitize the session label client-side per Codex review (round 1) Codex round 1's only finding, and it is a bigger deal than a missing label. Unix directory names may contain control bytes — "doc\napp" is a legal directory — and Go's http.Client REFUSES to send a request whose header value holds one: Do returns "invalid header field value" and nothing is transmitted. In the monitor that is indistinguishable from an unreachable padd, so the retry loop backs off and tries again, forever, printing nothing by contract. A user who named a directory that way would simply stop receiving notifications, with no signal anywhere. The server cannot defend against a request that never arrives. Reproduced before fixing, with a real directory and a real client, rather than reasoned about from the error message. Sanitizing in NewWatchEventsStreamRequest rather than in monitorSessionIdentity: the invariant is "this function never builds an unsendable request", which belongs at the point where a value becomes a header, not at one caller. The client's cap (256 runes) is deliberately looser than and independent of the server's (64): the server decides what a label should look like, the client only has to keep the request sane, and neither has to track the other to stay correct. The regression test does the ROUND TRIP instead of inspecting the header, because the header contents were never the bug — http.Header.Set stores anything, so an assertion on the value passes against the broken version too. Only attempting the request tells the two apart. Mutation-verified: reverting the sanitizer fails the test with exactly the "invalid header field value" error from the field report. |