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.
193f5cc to
f0fce77
Compare
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
Nice! This is a partial review and I have not tried it out yet but here is what I have so far.
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.
f0fce77 to
ed4231e
Compare
Pass server data paths through verbatim instead of rejecting unsafe-looking values; the agent canonicalizes patterns and records per-pattern failures in the bundle manifest. Glob metacharacters are escaped with character classes, which survive the agent's separator normalization on Windows where backslash escapes would not. Fold the home-directory fallback into getRemoteServerDataPath and default the folder name to .vscode-remote, matching the server's own resolution. Track the connected agent on Commands instead of re-parsing the remote authority.
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
I have one main concern about generating the support bundle from the sidebar but otherwise looking good!
Drop the exec-server VSCODE_AGENT_FOLDER read: every supported Remote-SSH implementation derives the server data dir from its install path setting (or a fixed home default) and hands it to the server at launch, so local settings resolve the same path deterministically. Reconstruct the authority for sidebar and picker flows so the lookup works without an active connection, anchor relative install paths to the remote home, and fold workspaceFiles.ts into remoteServerDataPath.ts.
2 participants
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
Resolution is deterministic from local state; no remote connection is needed:
remote.SSH.serverInstallPathfor the workspace's SSH host. Relative paths are anchored to~/since SSH resolves them against the home directory.~/{product-server-folder}from the editor'sproduct.json, the server's own default.The SSH host comes from the live authority when connected and is otherwise reconstructed from the workspace and agent (first agent for workspace-level items, matching the CLI), so sidebar, picker, and palette flows resolve identically.
An earlier revision also read
VSCODE_AGENT_FOLDERfrom the active exec server. Dropped after verifying each implementation's actual code (local extension bundles for Microsoft, Cursor, Windsurf, and Antigravity; source for Open Remote SSH):serverInstallPaththemselves and export it at server launch, overriding any user-set value, so settings resolve the identical path.--server-data-dirinstead.$HOME/{product-server-folder}.getRemoteExecServeris not a passive read: when not connected it spawns a real SSH connection and installs the VS Code CLI on the remote, so it could never safely serve the sidebar flow.The resolver uses the same first supported Remote-SSH extension selected during activation.
serverInstallPathbehavior*~/.devin-server(formerly~/.windsurf-server)~/.antigravity-serverTesting
pnpm test— 2,255 passed, 1 skippedpnpm lintpnpm typecheckpnpm format:checkgit diff --checkGenerated by Coder Agents.