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.
| try { | ||
| await fs.rm(outputBundlePath, { force: true }); | ||
| } catch (cleanupError) { | ||
| logger.warn( | ||
| `Could not clean up partial bundle at ${outputBundlePath}`, | ||
| cleanupError, | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: Cleanup sounds to me like "finally" step.
| `Adding ${diagnosticFiles.size} VS Code diagnostic file(s) to support bundle`, | ||
| ); | ||
|
|
||
| const outputBundlePath = vscodeBundlePath(zipPath); |
There was a problem hiding this comment.
nit: should we append ".incomplete" or something to indicate that the artifact is pending renaming?
| ); | ||
| } | ||
|
|
||
| function newestLog(logs: CollectedFile[]): CollectedFile | undefined { |
There was a problem hiding this comment.
hmm... should we check logs earlier than return undefined here?
| * and a redacted `coder.*` / `remote.*` settings snapshot. Keys are | ||
| * zip-relative paths under `vscode-logs/`. | ||
| */ | ||
| export async function collectVsCodeDiagnostics( |
There was a problem hiding this comment.
readability and ordering: should we move exported functions to the top and let a developer just read the flow from top to the bottom?
| return files; | ||
| } | ||
|
|
||
| async function collectVsCodeWindowLogs( |
There was a problem hiding this comment.
Do we need special support for Windows FS?
|
|
||
| // Paths, hostnames, URLs, and command strings: anything user-supplied that | ||
| // could identify a machine or deployment in a shared bundle. | ||
| const REDACTED_SETTINGS: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
curious if we should make redacting configurable, sometimes these options can be crucial for root cause analysis
2 participants
Summary
vscode-logs/settings.jsonwith inspect-stylecoder.*/remote.*settings; redacts 14 sensitive values (paths, hostnames, URLs, command strings) to<set>or<empty>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 collectionFollow-up
Tests
pnpm format:checkpnpm lintpnpm typecheckpnpm test:extension ./test/unit/supportBundle/appendVsCodeLogs.test.tspnpm test:extensionCloses #955.
Refs #970.
Generated by Coder Agents.