fix: serve remote daemon request diagnostics to the caller - #1814
Conversation
9d0ed95 to
f457c80
Compare
|
Code review at f457c80 found no implementation or architecture defect in the remote diagnostic-record route. Readiness is blocked by the current merge conflict ( |
…onDir (#1815) safeSessionName only rewrites characters outside [a-zA-Z0-9._-], so the names '.' and '..' survive unchanged and path.join resolves them to the sessions dir itself or its parent, the daemon state dir. A remote caller's --session .. would then land app.log / runner.log / requests/*.ndjson outside the sessions tree. SessionStore.resolveSessionDir is the one place a session name becomes a directory (AGENTS.md: session artifact paths come from session-store), so it now refuses such a name with INVALID_ARGS. Every request goes through it first thing in createRequestExecutionScope, before any artifact path is used, so this is also the admission-time rejection; every other caller passes an already admitted name. isSafeSessionSegment mirrors the predicate PR #1814 adds for its request-diagnostics route; whichever lands second takes the trivial merge. Regression tests were proven red against the pre-fix code: resolveSessionDir returned the sessions dir / state dir for '.', '..', '' and the request scope resolved runnerLogPath to <stateDir>/runner.log.
4112c14 to
efba178
Compare
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Delta re-review at efba178 found no PR-owned code or architecture issue; the prior conflict is resolved, and the added SessionStore segment hardening preserves containment. Readiness remains blocked because exact-head Coverage is red on |
efba178 to
3aa034d
Compare
|
Re-reviewed exact head 3aa034d: clean and merge-ready. The only delta from the previously reviewed head is inherited main fixes (#1816/#1817); the remote diagnostics route/client/error architecture is unchanged. The former inherited Coverage blocker is resolved, and all exact-head required checks plus Android/iOS/macOS/Linux/Web lanes are green. Existing live iOS remote-route evidence remains applicable. |
Summary
A failed command against a remote daemon used to print a diagnostics path on the daemon's
filesystem, which the caller cannot open — a CI runner driving an EAS/limrun simulator got one line
of error and no route to the evidence. Now the record itself comes to the caller.
GET /sessions/<session>/requests/<requestId>/diagnostics,streamed as
application/x-ndjson, behind the same authorize gate as/artifacts/*. Session andrequest ids are validated with the same safe-name rule session artifact paths use, and the path is
built only through
resolveSessionRequestLogPath. A tenant may only read records inside its own<tenant>:session namespace, using the same rule that names those sessions(
session-tenant-scope.ts, now shared withscopeRequestSession).DaemonError.diagnosticsRecord = { session, requestId }— built together with the path in oneresolveSessionRequestLogresult, so the two can never disagree.readable. The daemon-host
logPathis removed at the transport boundary (the stripped payload typemakes it unrepresentable downstream) and the fetched copy at
<state-dir>/remote-diagnostics/<session>/<request-id>.ndjsonis the only thing that can becomelogPath. One attempt, 10s timeout, 8 MiB cap; a failed fetch names no path at all.Diagnostics Log:is now always a path on the calling machine.Remote Diagnostics: unavailable (remote daemon <url>, request <id>: <reason>)prints when therecord stayed remote.
--debugprints the fetched record's tail inline ([remote diagnostics]) andno longer reads the local
daemon.log, which belongs to a different daemon (same gatemaybeStartDaemonLogTailalready had).Before / after, same failing command against a remote daemon:
A CI job can archive
remote-diagnostics/wholesale, or hit the route itself with the base URL andtoken it already holds.
Local daemons are unchanged: the path they print is on the same machine as the caller.
Closes #1801
Validation
Red first. With the production sources reverted to
HEAD~1(new tests kept), the fourcli-remote-diagnosticscases and four of five route cases fail. The motivating failure, verbatim:That is exactly the reported bug: the printed path is the daemon host's. All 12 pass after the fix.
One route case (404 for an unknown record) passes pre-fix too — with no route at all everything 404s;
it is meaningful only next to the 200 case on the same server, and is kept for that pairing.
Tests.
src/daemon/__tests__/request-diagnostics-http.test.ts(real loopback daemon: 200 + ndjson../empty ids sent asa raw request target —
fetchresolves..away client-side, so the traversal case needs ahand-built target — and cross-tenant refusal).
src/remote/__tests__/remote-request-diagnostics.test.ts(fetched copy becomes
logPath; nothing serialized on the error, printed or JSON, equals thedaemon-supplied path; unavailable reasons).
src/__tests__/cli-remote-diagnostics.test.tsdrives thereal CLI through the real client against a real daemon HTTP server, covering human output,
--json,--debugtail, and the no-route case.test/integration/provider-scenarios/remote-daemon-client.test.tshad asserted the old contract(
logPath === '/remote/log.txt'from a remote daemon) and now asserts the new one.Live (iOS simulator iPhone 16,
416440AE-…). Daemon in state dir A (dualtransport) played theremote host; the CLI ran from state dir B with
AGENT_DEVICE_DAEMON_BASE_URL/AGENT_DEVICE_DAEMON_AUTH_TOKEN, afteropen com.apple.Preferences:--debugadditionally printed[remote diagnostics]with both ndjson lines (request_start,request_failed) and no[daemon log].--jsoncarried the B-locallogPathplusdiagnosticsRecord. The same command against daemon A directly (no base URL) still printed A's ownsession path. The route was also exercised directly: authenticated
200 / 1011 bytes / application/x-ndjson, no token401, unknown request id404. Session closed, both daemons stopped,simulator shut down.
Gates.
pnpm check:affected --rungreen (format, lint, typecheck, layering, fallow, unit +provider-integration, wire-compat), plus
pnpm check:daemon-wire-compat:169 declarations, 7 changed, 0 removed, 18 addedagainstv0.20.9.Rebased onto
mainafter #1817, so theframeworkTierparity failure this branch briefly inheritedfrom main is gone; the local gate is clean end to end.
Wire surface
Additive under ADR 0006 — protocol stays 2.
compatibleChangesacks coverDaemonError,NormalizedError,normalizeError,handleDaemonHttpResponseBody,toDaemonHttpRpcError, plus twohelper moves in the artifact route (
readArtifactIdand its mid-stream error branch now use sharedhelpers). Both new route sides — producer and consumer — are listed in
surface.ts.Tradeoffs and follow-ups
open'sinitialSnapshotError.logPathis not covered. It rides inside a successful response'sdata, not through the error rehydration seam, so this change does not reach it. A remote
openwhoseinitial capture failed can still surface a daemon-host path there; worth a follow-up issue.
unavailable (… the daemon named no diagnostics record)rather than a path — deliberate: the oldbehavior was the bug.
Remote Diagnostics:line rather than a secondDiagnostics Log:line, because the CLI's own client-side record may still be named and printing two lines with the
same label would read as a contradiction.
retention job for
requests/*.ndjsonon either side (pre-existing).edits are the mechanical
flushDiagnosticsToSessionFilereturn-type update at its call sites and thetwo shared HTTP helpers fallow's duplication gate required.
isSafeSessionSegment(which this branch introduced) landed on main independently, whereresolveSessionDirnow refuses./..outright; this branch keeps only its route-level use of theshared predicate, so a malformed request target is refused with a 400 before the auth hook runs
rather than by the path constructor afterwards.