feat: collect remote editor logs in support bundles#1042
Conversation
a44730d to
05c3870
Compare
05c3870 to
1b3db23
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 15 findings (1 P2, 8 P3, 3 Nit, 3 Note), APPROVE. Review Finding inventoryFinding inventory: PR #1042Findings
Dropped
Cross-repo contract verification (coder/coder)
Contested and acknowledgedCRF-2 (P3, workspaceFiles.ts:41) - posix
|
| Reviewer | Focus |
|---|---|
| Bisky | tests |
| Chopper | ops/errors |
| Churn-guard | change verification |
| Ging | language modernization |
| Gon | naming |
| Hisoka | edge cases |
| Killua | perf |
| Kite | change integrity |
| Knov | contracts |
| Knuckle | SQL |
| Komugi | flake/determinism |
| Kurapika | security |
| Law | decomposition |
| Leorio | docs |
| Luffy | product |
| Mafu-san | process |
| Mafuuu | contracts |
| Melody | dispatch/pairing |
| Meruem | structural |
| Nami | frontend |
| Netero | mechanical checks |
| Pariston | premise testing |
| Pen-botter | product gaps |
| Razor | verification |
| Robin | duplication |
| Ryosuke | Go arch |
| Takumi | concurrency |
| Zoro | shape |
🤖 Managed by Coder Agents.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a careful, well-tested feature. Remote editor logs are exactly what someone drowning in a broken remote connection reaches for, and the design fails closed: unresolved or unsafe paths are dropped rather than broadened, the globs are narrow known layouts rather than broad scans, and --workspace-file values are passed as separate execFile args with no shell. Test density is 55.8% and the negative-path tables (unsafe paths, unsafe folder names, per-fork resolution, Windows inference) verify targeting, not just delivery. Hisoka put it well: "I came to fight this one. It fought back well. Most of the traps I set closed on air."
Counts: 1 P2, 8 P3, and several nits/notes. No P0/P1.
The P2 is that the whole resolution path swallows every failure silently with no logger, in the one tool built to diagnose problems, while every sibling file in src/supportBundle/ already threads a Logger and logs on failure. The P3s cluster around three themes: (1) the product-metadata ~/ fallback hardcodes posix and bypasses the module's own safe-path contract, so a Windows remote reaching it collects nothing; (2) parseRemoteAuthority is called unguarded in a code path shared by multiple diagnostic commands, where its sibling call site wraps the identical call in try/catch; (3) the new command-layer wiring (agent/authority resolution + feature gate) has no test, and two test seams no longer assert what they claim.
Cross-repo contract, verified against coder/coder so the panel's unverified flags are settled: --workspace-file exists (coder#26694) and the agent resolves ~/ against the agent user's home and expands ** globs. That disproves the "feature silently collects nothing in the common case" fear that several reviewers raised at P2; the fallback works for POSIX remotes, so CRF-2 is scoped down to the Windows residual and the contract inconsistency. Separately, the [<agent>] positional with RequireRangeArgs(0,2) has existed since the support command shipped (coder#12328, before the 2.10.0 gate), so the ungated agent positional is safe on every supported CLI; that Note is dropped.
A structural alternative worth weighing (CRF-6): the running exec server's VSCODE_AGENT_FOLDER is the ground truth, yet the client-side reconstruction of five forks' serverInstallPath semantics is preferred over it. Inverting that precedence (active first, configured as fallback) would let the per-fork wildcard/suffix machinery run only in the genuine fallback niche and avoid collecting from a stale configured directory.
Minor items not filed as separate inline comments (worth a pass since this code is unlikely to be revisited): the getRemoteServerDataPath doc comment restates the signature and could instead state the config-over-active precedence (Gon); toRemoteLogGlobs doc says "Append" for a function that returns a fresh array (Leorio); serverDataPath/serverDataRoot are near-synonyms three lines apart (Gon); remoteAuthority is set on only one of the three "selected" branches, so sidebar/picker collection guesses the remote folder from the local editor's product.json (Melody); the Cursor suffix-stripping claim is uncited while its sibling cites upstream source (Mafu-san); and the UNC-style and error-swallow branches lack test coverage (Bisky).
🤖 This review was automatically generated with Coder Agents.
c769f15 to
faffb1c
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. The author addressed all twelve round-1 findings and defended the three they disagreed with, each defense pinned to a commit-cited upstream source. This is the inverse of the usual failure mode: claims that drive code carry citations that resolve to the claimed code.
Verified fixed (checked against the code, not just the reply): CRF-1 logger threaded through every former silent catch; CRF-3 parseRemoteAuthority guarded, degrades to agentName: undefined with a warn; CRF-4 new commands.supportBundle.test.ts covers tree-item, active-connection, malformed-authority, and the 2.36 gate; CRF-7 shared hasUnsafePathChars now backs all three sources; CRF-8 default-shape argv asserted; CRF-9 wildcard-precedence test can now fail; CRF-10 .. rejected; CRF-11 raceWithAbort makes the resolver cancellable; CRF-12 renamed to getRemoteEditorLogGlobs. raceWithAbort itself is clean: it documents that work is abandoned not stopped, Promise.race keeps a reaction on the loser so a late rejection is not orphaned, and the listener is removed in finally.
Contested findings, all closed by the panel:
- CRF-5 (host matcher): seven reviewers fetched jeanp413
serverSetup.tsand confirmedhostnamePatternScoreis a byte-faithful mirror ofmatchHostnamePattern, which escapes?literally. ReusingcomputeSshProperties(OpenSSH?=single-char) would diverge from the config open-remote-ssh itself writes and reads. The divergence flagged in R1 is intentional fidelity. Defense holds. - CRF-6 (config-first precedence): confirmed against vscode
server.main.tsL39 (args['server-data-dir'] || VSCODE_AGENT_FOLDER || join(homedir, ...)).serverInstallPathis passed as--server-data-dir, which outranks the env var, so config-first mirrors the server's own resolution. The staleness window is narrow, self-corrects on reconnect, and fails closed. Defense holds; the R1 inversion proposal was withdrawn. - CRF-2 (posix
~/fallback on Windows): closed, and I verified the crux directly. The panel surfaced a sharper version of this (Chopper P2, Hisoka/Knov/Razor P3):toRemoteLogGlobsemitswin32backslash globs (C:\...\**\*.log) and doublestar matches on/only, so if the agent did not normalize the pattern, a Windows remote that resolves a server dir would silently collect nothing. I read the agent-side collector at the ref that adds the flag (coder#26694):bundleFileMatchescanonicalizes the requested pattern (expanding~/viafilepath.Join, keeping globs lexical) then runsdoublestar.SplitPattern(filepath.ToSlash(abs)). The incoming pattern isToSlash'd, so both the posix~/fallback and the win32 backslash globs normalize to forward slashes and match on a Windows agent. The Windows silent-gap premise is disproven; what remains is that the win32 separator branch is functionally redundant (CRF-20, Note).
No P0/P1/P2 survive. The three items below are a Note and two test-hardening Nits. Luffy caught the shape of it: "the person using the product never sees it, and the person debugging their broken remote session suddenly has the logs."
Minor items carried from R1 and still open (worth a pass since this code is unlikely to be revisited): the toRemoteLogGlobs doc still says "Append" for a function that returns a fresh array (CRF-15); serverDataPath/serverDataRoot remain near-synonyms (CRF-16); the Cursor suffix-stripping comment still asserts installer behavior with no citation while both siblings gained commit-pinned @see links this round (CRF-18); and the exec-server catch and no-VSCODE_AGENT_FOLDER branches are still untested while the sibling active-env catch is (CRF-19).
🤖 This review was automatically generated with Coder Agents.
Resolve the active remote editor's server data directory and pass its log glob to the CLI as --workspace-file so support bundles include the remote editor logs. The path resolves from Remote-SSH settings (mirroring each fork's serverInstallPath semantics) with the exec server's VSCODE_AGENT_FOLDER as fallback, matching the server's own precedence, and validates every source against globs, variables, and traversal. Gated on CLI support for workspace files.
Also collect the server's root dotfile logs and CLI launcher logs (cli/servers/*/log.txt) alongside the editor session logs.
193f5cc to
f0fce77
Compare
1 participant
Summary
--workspace-filesupport, gated to Coder 2.36.0+.Remote log coverage
For each resolved server data directory, collect these narrow known layouts:
data/logs/**/*.log— editor runtime, extension-host, and related logs.*.log— legacy server startup logscli/servers/*/log.txt— exec-server startup logsBoth startup layouts are included rather than gated by the current
remote.SSH.useExecServervalue: the setting can change after connection, Open Remote SSH uses the legacy layout independently, and supported IDE forks do not expose one consistent authoritative mode signal.Log path resolution
Paths are resolved in this order:
remote.SSH.serverInstallPathbehavior. This takes precedence because supported implementations may pass it as VS Code's--server-data-dir, and the server itself resolves its data directory as--server-data-diroverVSCODE_AGENT_FOLDERover the home default (server.main.ts#L39), so this order mirrors the server's own precedence tier for tier.VSCODE_AGENT_FOLDERwhen available.~/{product-server-folder}from the editor's validatedproduct.jsonmetadata.The resolver uses the same first supported Remote-SSH extension selected during activation.
serverInstallPathbehaviorcli/servers/*/log.txtobserved*.<commit>.logat the server data rootTesting
pnpm test— 2,248 passed, 1 skippedpnpm lintpnpm typecheckpnpm format:checkgit diff --checkGenerated by Coder Agents.