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.
| return undefined; | ||
| } | ||
| const style = pathStyleForPlatform(osPlatform); | ||
| if (!isSafeAbsolutePath(value, style)) { |
There was a problem hiding this comment.
I feel like a relative path should be OK, and any other characters should be treated as literal (escaped) rather than rejected.
There was a problem hiding this comment.
I dropped this check and passed through verbatim.
The agent still requires absolute or ~/ paths and expands env vars, but a bad pattern just becomes a note in the bundle manifest now
There was a problem hiding this comment.
We could do something like prepend ~/ to relative paths since that is where they would resolve I think, since SSH's cwd is the home directory.
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!
| serverDataFolderName, | ||
| logger, | ||
| }: RemoteServerDataPathOptions): Promise<RemoteServerDataPath> { | ||
| const configured = |
There was a problem hiding this comment.
The ternaries here with undefined branches feel a bit difficult to parse to me. I think it is something like this:
let dataPath
if (remoteAuthority && serverDataFolderName)
dataPath = getConfigured(...)
}
if (!dataPath && remoteAuthority) {
dataPath = await getActive(...)
}
return dataPath || { ... }| // server's default when neither override is present. | ||
| return ( | ||
| active ?? { | ||
| value: `~/${serverDataFolderName ?? ".vscode-remote"}`, |
There was a problem hiding this comment.
Is a blank folder name valid? Should this be ||? I think we want || since that is what VS Code is using.
| async function readServerDataFolderName( | ||
| appRoot: string, | ||
| logger: Logger, | ||
| ): Promise<string | undefined> { | ||
| try { | ||
| const productJson = await fs.readFile( | ||
| path.join(appRoot, "product.json"), | ||
| "utf-8", | ||
| ); | ||
| const { serverDataFolderName } = JSON.parse( | ||
| productJson, | ||
| ) as ProductConfiguration; | ||
| return typeof serverDataFolderName === "string" && serverDataFolderName | ||
| ? serverDataFolderName | ||
| : undefined; | ||
| } catch (error) { | ||
| logger.warn("Could not read the editor's product metadata", error); | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this go in remoteServerDataPath.ts and be called by getRemoteServerDataPath? It feels more directly related to the server data path than to workspace files, IMO.
| ]; | ||
| } | ||
|
|
||
| async function getActiveServerDataPath( |
There was a problem hiding this comment.
I think the difference between getActive and getConfigured is non-obvious, what do you think about names like getServerAgentFolderEnv and getServerInstallPathSetting maybe?
Probably not the best names either but just looking at the names from high-level it is not clear to me why configured takes precedence over active, just based on the names it makes me feel it should be the other way around, like the "active" path should take precedence over the "configured" path.
| import type { Logger } from "../logging/logger"; | ||
|
|
||
| interface RemoteServerDataPathOptions { | ||
| readonly remoteAuthority?: string; |
There was a problem hiding this comment.
Thinking out loud...if support bundles are generated for agents not currently connected, then we might end up getting the completely wrong path right?
Could we perhaps reconstruct the authority for these? Like from a sidebar item I think we could call toRemoteAuthority(). Currently if I try to generate a support bundle from the sidebar, even for the workspace I am currently connected to, I get the wrong paths. Works great from the command palette though!
Alternatively, we could try passing in the workspace and agent here and then construct the remote authority directly here as needed.
Also, for the support bundle command on a workspace directly (rather than an agent) we could use the first agent since I think this is what the support command does, although maybe we could check if one is connected and use that, if not then fall back to the first.
But maybe the env resolver only works when actively connected. In that case...we could resolve the env when connecting and then store that for later use? Just wanna avoid the case where someone generates a bundle and it goes to the wrong place depending on whether they happen to be connected or not.
There was a problem hiding this comment.
I did some more testing and it looks like, at least with VS Code, it always overrides VSCODE_AGENT_FOLDER. So there seems no way to set it anyway? I tried setting it in my profile and bashrc. So it might be we have no need to check it, seems like it only existed as a way for the remote SSH plugin to tell the server the install path rather than a way for users to configure the path.
| // Cursor accepts the product folder itself despite documenting a parent. | ||
| // Its installer strips this suffix before consistently re-appending it. | ||
| const parentPath = | ||
| extensionId === "anysphere.remote-ssh" && | ||
| remotePath.basename(installPath) === serverDataFolderName | ||
| ? remotePath.dirname(installPath) | ||
| : installPath; | ||
| return { | ||
| value: remotePath.join(parentPath, serverDataFolderName), | ||
| style, | ||
| }; |
There was a problem hiding this comment.
So just to make sure I understand, if we have serverDataFolderName as foo and serverInstallPath as /home/coder/foo then:
MS: end path is /home/coder/foo/foo
Cursor: end path is /home/coder/foo
Also if serverDataFolderName is blank, do they default to .vscode-server at this point? Or nah?
Oh...just tested VS Code and it actually created a .vscode-server-oss directory when the folder is blank or missing. Although shortly after I get a connection error.
Now, why would someone edit the product.json and remove the path? No idea lol but I guess it could happen.
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
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.