feat: collect VS Code diagnostics in support bundles#971
Conversation
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
from
6a3af1c to
c3c4c49
Compare
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
6 times, most recently
from
76e7ff2 to
2c03701
Compare
|
/coder-agents-review |
|
Review posted | Chat Review historydeep-review v0.5.0 | Round 4 | Last posted: Round 4, 12 findings (9 P3, 1 P4, 2 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-9 (P4, logFiles.ts:270) - collectVsCodeDiagnostics untested
Round logRound 1Panel. Netero first pass: 2 P3, 1 Note (Note dropped). Panel: 7 P3, 1 P4, 1 Nit new. Reviewed against 87a6831..31a7f15. Round 2BLOCKED. CRF-8 deferred without ticket. 8 addressed, 1 acknowledged (CRF-9), 1 deferred without ticket (CRF-8). No review. Round 3Panel. CRF-8 addressed (162b611). All R1/R2 findings resolved. Netero clean. Panel: 2 P3 new. Reviewed against 21d88bb..162b611. Round 4CRF-12, CRF-13 addressed (7e8a79d). Netero clean. All findings resolved. CI green. APPROVE. Reviewed against 21d88bb..7e8a79d. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean modular decomposition of support bundle diagnostics. The split into four focused modules is well-motivated: each has clear responsibility, independent testability, and the test suite (32 tests across 4 files) exercises real filesystem operations, real zip round-trips, and the settings redaction boundary.
Particular strengths: the symlink guard via lstat in file-read paths, the DO_NOT_LEAK_SECRET canary pattern in the settings redaction test, cross-session log collection scoped to windows that actually hosted the Coder extension, the non-canonical layout fallback, and the atomic rename with permission preservation.
"Bungee Gum. Pull the thread:
sshProcess.ts:searchForLogFile()discovers the log viareaddir(nowithFileTypes, nolstat), stashes the path inthis.logFilePath." -- Hisoka, tracing the active proxy log path
7 P3, 1 P4, 2 Nit.
PR description note: see inline CRF-11.
Body-level observations:
- The promoted SSH log basename can be ambiguous when the source is an exthost log (e.g.,
1.logatvscode-logs/remote-ssh/1.log). Full per-window paths are always present alongside, so no data loss. Worth noting for support engineers. (Meruem) - No size cap on individual file reads. A fresh 500MB proxy log from a debug session gets loaded in full during rezip. Plausible when
coder.httpClientLogLevelisdebug, which is also when support bundles are created. (Hisoka) - 15 of 20 in-scope comments open with a restatement of the code before the actual
why. Each adds ~5 extra words of padding. The comments carry genuine information; the pattern is verbosity, not inaccuracy. (Gon)
🤖 This review was automatically generated with Coder Agents.
- Fix sort comparator equality case in collectWindowLogDirs (CRF-1) - Drop readWindowDir; reuse exported readDirents (CRF-2) - Route active proxy log through readLogFile so it gets the lstat symlink guard and a path-bearing warn message (CRF-4, CRF-10) - Add coder.experimental.oauth, coder.telemetry.level, coder.telemetry.local to COLLECTED_SETTINGS (CRF-5) - Rename logFiles/writeBundleWithLogs to diagnosticFiles/ writeBundleWithDiagnostics and update log copy (CRF-6) - Add JSDoc on collectVsCodeDiagnostics and collectSettingsFile (CRF-7)
- Fix sort comparator equality case in collectWindowLogDirs (CRF-1) - Drop readWindowDir; reuse exported readDirents (CRF-2) - Route active proxy log through readLogFile so it gets the lstat symlink guard and a path-bearing warn message (CRF-4, CRF-10) - Add coder.experimental.oauth, coder.telemetry.level, coder.telemetry.local to COLLECTED_SETTINGS (CRF-5) - Rename logFiles/writeBundleWithLogs to diagnosticFiles/ writeBundleWithDiagnostics and update log copy (CRF-6) - Add JSDoc on collectVsCodeDiagnostics and collectSettingsFile (CRF-7)
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
from
96bb4c6 to
3690c44
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
8 of 11 findings addressed in 3690c44. Clean fixes across the board: sort comparator corrected, readWindowDir consolidated, symlink guard extended via readLogFile factoring, allowlist completed, naming/messages aligned, doc comments added, PR description updated.
CRF-9 (P4) acknowledged with bounded justification: the 10-line composition function's risk is limited by independent test coverage of both composed arms. Accepted.
One finding blocks further review:
CRF-8 (P3) Remote SSH log heuristics duplicated between logFiles.ts and sshProcess.ts. Author acknowledged the duplication and described the fix approach (shared predicate module), but deferred without linking a ticket. The duplication is a maintenance concern: if VS Code changes its log layout, both files need independent, untested-against-each-other updates. This needs a human decision: file a tracking issue, or explicitly accept the gap.
Further panel review is blocked until CRF-8 has a linked ticket or explicit human acceptance. Pushing fixes or responding unblocks the next round.
🤖 This review was automatically generated with Coder Agents.
Address PR #971 CRF-8: extract the Remote-SSH log layout knowledge (extension exthost dirs, `output_logging_` prefix, `Remote - SSH` filename fragment) into `sshExtension.ts` as predicates and consume them from both `sshProcess.ts` and `supportBundle/logFiles.ts`. A future VS Code log layout change now touches one place.
Address code review findings and collapse the support-bundle module from 6 files to 4. Behavioral fixes: - redact Coder paths, URLs, and hostnames in shared bundles - lstat in readRecentFile so a planted symlink can't pull host files in - create the temp zip with the source's mode (no umask window, no chmod failure path that deletes the otherwise-valid bundle) - pick the active SSH log only from the current window, never fall back - drop the literal extension-id check so rebranded forks work; anchor the session-name regex - collect Remote-SSH logs from exthost dirs (the path-branch was dead) and proxy logs named bare <pid>.log - preserve the active proxy log's real filename and dedup it against the dir scan - stable lexicographic tie-break in newestLog - suppress ENOENT warnings during rotation-racey recursive walks Structural cleanup: replace package.json setting discovery with an explicit allowlist, merge vscodeLogs.ts and diagnostics.ts into logFiles.ts, fuse the per-window extension and Remote-SSH walks, drop the readDir overload and the dead collectExtensionLogs fallback.
- Fix sort comparator equality case in collectWindowLogDirs (CRF-1) - Drop readWindowDir; reuse exported readDirents (CRF-2) - Route active proxy log through readLogFile so it gets the lstat symlink guard and a path-bearing warn message (CRF-4, CRF-10) - Add coder.experimental.oauth, coder.telemetry.level, coder.telemetry.local to COLLECTED_SETTINGS (CRF-5) - Rename logFiles/writeBundleWithLogs to diagnosticFiles/ writeBundleWithDiagnostics and update log copy (CRF-6) - Add JSDoc on collectVsCodeDiagnostics and collectSettingsFile (CRF-7)
Address PR #971 CRF-8: extract the Remote-SSH log layout knowledge (extension exthost dirs, `output_logging_` prefix, `Remote - SSH` filename fragment) into `sshExtension.ts` as predicates and consume them from both `sshProcess.ts` and `supportBundle/logFiles.ts`. A future VS Code log layout change now touches one place.
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
from
5386867 to
162b611
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All R1 findings resolved. The CRF-8 fix (shared predicates in sshExtension.ts) is verified clean: both sshProcess.ts and logFiles.ts now consume from one location, no duplicate literals remain in predicate logic. The settings allowlist is in sync with package.json (26/26 coder.* keys). The readLogFile factoring correctly extends the symlink guard to the active proxy log with an explicit comment on the age cutoff bypass.
R3 panel (6 reviewers, Netero clean): 2 new P3 findings, both from Meruem.
"Extension defaults are defined in the public package.json manifest and are identical for every installation. They contain no user data." -- Meruem, on redacting defaultValue
2 P3.
Body-level observations:
setAgetest helper is still duplicated verbatim across files.test.ts:28 and logFiles.test.ts:29. Minor, but consistently flagged across rounds. (Bisky, Robin)collectWindowLogDirsscans all session directories without a directory-level age filter. Individual files are filtered by the 3-day cutoff, so this is overhead, not correctness. VS Code cleans old sessions, and theextLogs.size === 0short-circuit limits downstream work. (Meruem)
🤖 This review was automatically generated with Coder Agents.
Address PR #971 review findings: - CRF-12: tail-truncate log files over 50 MB in readLogFile to prevent OOM during rezip when a long-lived session has a multi-GB debug log - CRF-13: passthrough `defaultValue` in maybeRedact so package.json defaults (public, no user data) stay visible for triage
Address PR #971 review findings: - CRF-12: tail-truncate log files over 50 MB in readLogFile to prevent OOM during rezip when a long-lived session has a multi-GB debug log - CRF-13: passthrough `defaultValue` in maybeRedact so package.json defaults (public, no user data) stay visible for triage
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
from
c1d6bca to
f124bf0
Compare
Address PR #971 review findings: - CRF-12: tail-truncate log files over 50 MB in readLogFile to prevent OOM during rezip when a long-lived session has a multi-GB debug log - CRF-13: passthrough `defaultValue` in maybeRedact so package.json defaults (public, no user data) stay visible for triage
EhabY
force-pushed
the
feat/support-bundle-diagnostics
branch
from
f124bf0 to
7e8a79d
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 13 findings resolved across 4 rounds. CRF-12 (50MB size cap with tail-truncation) and CRF-13 (defaultValue passthrough) verified fixed in 7e8a79d. Netero R4 clean. CI green (13/13).
This PR is solid. The module split is clean, the test coverage is thorough (33 tests, real filesystem I/O, real zip round-trips), the redaction boundary is well-designed with an explicit allowlist, and every finding from 4 review rounds was addressed with proportional fixes.
🤖 This review was automatically generated with Coder Agents.
- Mask only the settings whose value can hold a secret (coder.globalFlags, coder.headerCommand, coder.tlsCertRefreshCommand); collect the rest verbatim. URLs, paths, hostnames, and ssh flags/config are already plaintext elsewhere in the bundle (the CLI bundle's config.json, the coder-ssh proxy logs, the bundled Coder.log, and the Remote-SSH ProxyCommand under verbose ssh), so masking them only hid signal. - Rewrite newestLog as a single-pass max-by-comparator (drops the toSorted copy; empty input returns undefined naturally). - Reorder the supportBundle modules newspaper-style (public functions first, helpers below) and tighten comments. - Share the setAge test helper from testHelpers and drop the duplicated setMtimeAgo in localJsonlSink.test.ts.
2 participants
Summary
vscode-logs/settings.jsonwith inspect-stylecoder.*/remote.*settings. Masks only the values that can hold a secret (coder.globalFlags,coder.headerCommand,coder.tlsCertRefreshCommand) to<set>/<empty>and collects the rest verbatim. The unmasked values (deployment URL, binary path/source, proxy log dir, TLS file paths / alt host, proxy bypass, ssh flags/config) are URLs, paths, hostnames, or flags that are already plaintext elsewhere in the same bundle — the CLI bundle'sconfig.json, thecoder-ssh-*.logproxy logs, the bundledCoder.log, and the Remote-SSHProxyCommandunder verbose ssh — so redacting them only hid diagnostic signal.vscode-logs/proxy/, preserving its original filename (e.g.coder-ssh-12345.log) so diagnostic PIDs survivevscode-logs/proxy/src/supportBundle/modules for ZIP mutation, log discovery, and settings collectionSafety
lstat-guarded so a planted symlink can't pull host files into the bundle, capped at 50 MB (tail-truncated) to avoid OOM during rezip, and limited to a 3-day age windowFollow-up
Tests
pnpm format:checkpnpm lintpnpm typecheckpnpm test:extension ./test/unit/supportBundle/pnpm test:extensionCloses #955.
Refs #970.