✨ Feature/debug tarball #684
Conversation
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
WalkthroughAdds a new VS Code command konveyor.generateDebugArchive that interactively builds a ZIP containing redacted provider settings and extension configuration, and optionally includes LLM traces. Exposes getAllConfigurationValues and exports getWorkspaceRelativePath; adds getProviderConfigKeys and re-exports it. Updates package.json to contribute the command and add adm-zip and its types. Adds an end-to-end test that runs the command, supplies an output path, confirms redaction prompts, and asserts the archive file exists. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
vscode/src/modelProvider/config.ts (1)
38-63: Deterministic ordering: sort flattened keys for stable UX/tests.The flattener works correctly. For redaction prompts and reproducible archives, return keys in a stable order.
- const envKeyValues = flattenObject(parsedConfig.env, "env"); - const configKeyValues = flattenObject(parsedConfig.config, "config"); - return [...envKeyValues, ...configKeyValues]; + const envKeyValues = flattenObject(parsedConfig.env, "env"); + const configKeyValues = flattenObject(parsedConfig.config, "config"); + const pairs = [...envKeyValues, ...configKeyValues]; + pairs.sort((a, b) => a.key.localeCompare(b.key)); + return pairs;tests/e2e/tests/configure-and-run-analysis.test.ts (1)
61-62: Clean up the generated artifact to keep the workspace tidyRemove the debug archive after assertion to avoid leaking artifacts across test runs.
Apply this diff to clean up:
const zipStat = await fs.stat(pathlib.join('.vscode', 'debug-archive.zip')); expect(zipStat.isFile()).toBe(true); + await fs.unlink(pathlib.join('.vscode', 'debug-archive.zip'));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(1 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(2 hunks)vscode/package.json(1 hunks)vscode/src/commands.ts(4 hunks)vscode/src/modelProvider/config.ts(1 hunks)vscode/src/modelProvider/index.ts(1 hunks)vscode/src/utilities/configuration.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:10-29
Timestamp: 2025-08-08T12:30:00.315Z
Learning: tests/e2e/utilities/archive.ts (TypeScript, Playwright e2e utilities): The createZip() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately, and it remains async to align with await-based call sites (e.g., updateLLMCache) and API symmetry. Avoid suggesting defensive error handling or converting it to synchronous in future reviews.
📚 Learning: 2025-08-08T12:30:00.315Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:10-29
Timestamp: 2025-08-08T12:30:00.315Z
Learning: tests/e2e/utilities/archive.ts (TypeScript, Playwright e2e utilities): The createZip() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately, and it remains async to align with await-based call sites (e.g., updateLLMCache) and API symmetry. Avoid suggesting defensive error handling or converting it to synchronous in future reviews.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.tsvscode/src/commands.ts
📚 Learning: 2025-08-08T12:28:43.947Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:80-85
Timestamp: 2025-08-08T12:28:43.947Z
Learning: tests/e2e/utilities/archive.ts: The createSha256Sum() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately; avoid adding defensive error handling here in future reviews.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-07-16T16:08:20.126Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: test/e2e/fixtures/provider-configs.fixture.ts:41-42
Timestamp: 2025-07-16T16:08:20.126Z
Learning: In the test/e2e/fixtures/provider-configs.fixture.ts file, using LLMProviders.openAI for OpenAI-compatible services (like the PARASOL_PROVIDER) is expected and intentional behavior. The Parasol service uses ChatOpenAI with a custom baseURL, making it OpenAI-compatible.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🧬 Code Graph Analysis (3)
vscode/src/modelProvider/config.ts (1)
vscode/src/modelProvider/types.ts (1)
ParsedModelConfig(26-29)
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)scripts/_util.js (1)
fs(27-27)
vscode/src/utilities/configuration.ts (1)
vscode/src/utilities/constants.ts (1)
KONVEYOR_CONFIG_KEY(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
🔇 Additional comments (9)
vscode/src/modelProvider/index.ts (1)
1-1: LGTM: re-export is correct and non-breaking.Public surface now exposes getProviderConfigKeys alongside existing exports. No concerns.
vscode/src/utilities/configuration.ts (2)
3-3: LGTM: use of fs/promises is appropriate for async file I/O.No issues with importing Node’s promises API here.
197-209: LGTM: exporting getWorkspaceRelativePath improves reuse without altering behavior.The function correctly handles absolute paths and file:// URIs across platforms.
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
1-2: LGTM: imports are appropriate for the new test flowpath and fs/promises are the right additions for path handling and file existence checks.
60-62: Verify you’re stat’ing the file in the workspace (not the test runner CWD)fs.stat on a relative path resolves against the test runner’s current working directory, which may not be the VS Code workspace directory used by the extension command. If the test runner CWD differs, this assertion can become flaky or false-negative.
- Prefer deriving the absolute path from the workspace root (e.g., a helper like vscodeApp.workspaceRoot or a method that resolves workspace-relative paths).
- Alternatively assert the information message “Debug archive created at: …” and then stat the reported absolute path.
Do you have a helper to resolve workspace-relative paths in tests? If so, use it here to build the absolute path to .vscode/debug-archive.zip.
vscode/src/commands.ts (4)
1-3: LGTM: new dependencies are appropriateadm-zip, path, and fs/promises are suitable choices for the archive workflow.
46-53: LGTM: configuration utilities importThe additional configuration helpers (including getAllConfigurationValues and getWorkspaceRelativePath) align with the new command’s needs.
64-64: LGTM: provider config parsing utilitiesparseModelConfig and getProviderConfigKeys are the right abstractions for building the redaction list.
644-648: Confirm path sanitization to prevent escaping the workspacegetWorkspaceRelativePath is used to resolve the user-provided path. Please verify it prevents paths like ../../outside.zip from escaping the workspace root, and that it normalizes separators on Windows.
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
vscode/package.json (1)
41-44: Add activation event for konveyor.generateDebugArchiveWithout an onCommand activation event, the command won’t be available in a fresh session until another file-system trigger activates the extension. Wire the command into activationEvents.
Apply this diff:
"activationEvents": [ "onFileSystem:konveyorMemFs", - "onFileSystem:konveyorReadOnly" + "onFileSystem:konveyorReadOnly", + "onCommand:konveyor.generateDebugArchive" ],
🧹 Nitpick comments (1)
vscode/package.json (1)
261-266: Command contribution looks good; consider adding it to the Konveyor submenu for discoverabilityThe command definition is solid (category + icon). For easier discovery via the Explorer context Konveyor submenu, add it to menus.konveyor.submenu.
Example change:
- "konveyor.submenu": [], + "konveyor.submenu": [ + { + "command": "konveyor.generateDebugArchive", + "group": "navigation@99" + } + ],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
tests/e2e/tests/configure-and-run-analysis.test.ts(2 hunks)vscode/package.json(3 hunks)vscode/src/commands.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- vscode/src/commands.ts
- tests/e2e/tests/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:10-29
Timestamp: 2025-08-08T12:30:00.315Z
Learning: tests/e2e/utilities/archive.ts (TypeScript, Playwright e2e utilities): The createZip() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately, and it remains async to align with await-based call sites (e.g., updateLLMCache) and API symmetry. Avoid suggesting defensive error handling or converting it to synchronous in future reviews.
🔇 Additional comments (2)
vscode/package.json (2)
682-682: Dependency placement is correct
- @types/adm-zip under devDependencies is appropriate.
- adm-zip under dependencies is correct for runtime ZIP creation.
Also applies to: 697-697
697-697: No action required — adm-zip@^0.5.16 is up-to-date and not affected by the known zip‑slip advisorynpm dist-tag "latest" = 0.5.16. OSV reports GHSA-3v6h-hqm4-2rg6 (zip‑slip / CVE-2018-1002204) which was fixed in earlier 0.4.x releases; the project’s "^0.5.16" requirement is newer than the fix, so this advisory does not affect the current dependency.
- vscode/package.json — line ~697: "adm-zip": "^0.5.16" (current latest: 0.5.16)
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vscode/src/commands.ts (1)
649-689: Do not write (possibly sensitive) configs to disk; add to ZIP from memory. Also don’t add missing files.Two critical issues still present:
- provider-config.json and extension-config.json are written to the workspace before zipping, risking secret leakage and accidental overwrite of user files.
- ZIP step unconditionally referenced these files in earlier iterations; even now, temp-files-and-cleanup is unnecessary and brittle.
Build JSON in memory and add via AdmZip.addFile; skip adding if prep fails. Remove cleanup entirely. This also avoids partial failures if parse/write fails.
Apply this consolidated diff:
- // redact provider config - let redactedProviderConfig: Record<string, any> = {}; - const providerConfigPath = pathlib.join(pathlib.dirname(archivePath), `provider-config.json`); - let providerConfigWritten = false; + // redact provider config (build in memory) + let providerConfigContent: string | undefined; try { const parsedConfig = await parseModelConfig(paths().settingsYaml); const configuredKeys = getProviderConfigKeys(parsedConfig); const unredactedKeys = await window.showQuickPick( - configuredKeys - .map((obj) => ({ - label: obj.key, - })) - // all envs will be redacted no matter what - .filter((item) => !item.label.startsWith("env.")), + configuredKeys + .map((obj) => ({ label: obj.key })) + // always redact environment-derived values regardless of selection + .filter((item) => !item.label.startsWith("env.")), { title: "Select provider settings values you would like to include in the archive, all other values will be redacted", canPickMany: true, ignoreFocusOut: true, }, ); const unredactedKeySet = new Set((unredactedKeys || []).map((item) => item.label)); - redactedProviderConfig = configuredKeys.reduce( - (acc, keyValue) => { - acc[keyValue.key] = unredactedKeySet.has(keyValue.key) ? keyValue.value : "<redacted>"; - return acc; - }, - {} as Record<string, any>, - ); - await fs.writeFile( - providerConfigPath, - JSON.stringify(redactedProviderConfig, null, 2), - "utf8", - ); - providerConfigWritten = true; + const redactedProviderConfig = configuredKeys.reduce((acc, keyValue) => { + acc[keyValue.key] = unredactedKeySet.has(keyValue.key) ? keyValue.value : "<redacted>"; + return acc; + }, {} as Record<string, any>); + providerConfigContent = JSON.stringify(redactedProviderConfig, null, 2); } catch (err) { - window.showInformationMessage( + window.showWarningMessage( `Failed to parse provider settings file. Archive will not include provider settings.`, ); logger.error("Failed to parse provider settings file", { error: err }); } - // get extension config - const extensionConfigPath = pathlib.join( - pathlib.dirname(archivePath), - `extension-config.json`, - ); - let extensionConfigWritten = false; + // get extension config (build in memory) + let extensionConfigContent: string | undefined; try { - await fs.writeFile( - extensionConfigPath, - JSON.stringify(getAllConfigurationValues(), null, 2), - "utf8", - ); - extensionConfigWritten = true; + const extensionConfig = await getAllConfigurationValues(state.extensionContext.extensionPath); + extensionConfigContent = JSON.stringify(extensionConfig, null, 2); } catch (err) { - window.showInformationMessage( + window.showWarningMessage( `Failed to get extension configuration. Archive will not include extension configuration.`, ); logger.error("Failed to get extension configuration", { error: err }); } @@ try { const zipArchive = new AdmZip(); if (traceDirFound && includeLLMTraces === "Yes") { - zipArchive.addLocalFolder(traceDir as string); + zipArchive.addLocalFolder(traceDir as string); } - if (providerConfigWritten) { - zipArchive.addLocalFile(providerConfigPath); - } - if (extensionConfigWritten) { - zipArchive.addLocalFile(extensionConfigPath); - } + if (providerConfigContent) { + zipArchive.addFile("provider-config.json", Buffer.from(providerConfigContent, "utf8")); + } + if (extensionConfigContent) { + zipArchive.addFile("extension-config.json", Buffer.from(extensionConfigContent, "utf8")); + } await fs.mkdir(pathlib.dirname(archivePath), { recursive: true, }); await zipArchive.writeZipPromise(archivePath); window.showInformationMessage(`Debug archive created at: ${archivePath}`); } catch (error) { logger.error("Error generating debug archive", { error, archivePath }); window.showErrorMessage( `Failed to generate debug archive: ${error instanceof Error ? error.message : String(error)}`, ); } - try { - await fs.unlink(providerConfigPath); - await fs.unlink(extensionConfigPath); - } catch (error) { - logger.error("Error cleaning up temporary files", { - error, - providerConfigPath, - extensionConfigPath, - }); - }Additional benefit: avoids overwriting any existing provider-config.json/extension-config.json in the workspace by never creating them on disk.
Also applies to: 691-708, 730-762
🧹 Nitpick comments (3)
vscode/src/commands.ts (3)
631-639: Return undefined from validateInput and accept .ZIP case-insensitivelyVS Code’s validateInput should return string (error) or undefined (valid). Returning null is non-standard and may trip types. Also handle uppercase extensions.
Apply this diff:
- validateInput: async (value) => { + validateInput: async (value) => { if (!value) { return "Path is required"; } - if (pathlib.extname(value) !== ".zip") { + if (pathlib.extname(value).toLowerCase() !== ".zip") { return "Path must have a .zip extension"; } - return null; + return undefined; },
717-721: Explicitly warn that LLM traces may contain source code and pathsTo reduce accidental PII/code sharing, strengthen the prompt wording.
Apply this diff:
- includeLLMTraces = await window.showQuickPick(["Yes", "No"], { - title: "Include LLM traces?", + includeLLMTraces = await window.showQuickPick(["Yes", "No"], { + title: "Include LLM traces? (Traces may include parts of your source code and file paths.)", ignoreFocusOut: true, });
733-735: Set a stable folder name inside the ZIP for tracesPassing a target path keeps the archive layout predictable and avoids embedding the local folder name.
- zipArchive.addLocalFolder(traceDir as string); + zipArchive.addLocalFolder(traceDir as string, "traces");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
vscode/src/commands.ts(4 hunks)vscode/src/utilities/configuration.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/src/utilities/configuration.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:10-29
Timestamp: 2025-08-08T12:30:00.315Z
Learning: tests/e2e/utilities/archive.ts (TypeScript, Playwright e2e utilities): The createZip() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately, and it remains async to align with await-based call sites (e.g., updateLLMCache) and API symmetry. Avoid suggesting defensive error handling or converting it to synchronous in future reviews.
📚 Learning: 2025-08-08T12:30:00.315Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:10-29
Timestamp: 2025-08-08T12:30:00.315Z
Learning: tests/e2e/utilities/archive.ts (TypeScript, Playwright e2e utilities): The createZip() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately, and it remains async to align with await-based call sites (e.g., updateLLMCache) and API symmetry. Avoid suggesting defensive error handling or converting it to synchronous in future reviews.
Applied to files:
vscode/src/commands.ts
📚 Learning: 2025-08-08T12:28:43.947Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: tests/e2e/utilities/archive.ts:80-85
Timestamp: 2025-08-08T12:28:43.947Z
Learning: tests/e2e/utilities/archive.ts: The createSha256Sum() function is intentionally fail-fast (no existence checks or try/catch) so tests surface missing or corrupt cache immediately; avoid adding defensive error handling here in future reviews.
Applied to files:
vscode/src/commands.ts
🔇 Additional comments (4)
vscode/src/commands.ts (4)
1-3: LGTM: Necessary imports for the debug archive featureThe adm-zip, path, and fs/promises imports are appropriate for the new command.
46-53: LGTM: Configuration utility importsUsing getWorkspaceRelativePath, getTraceEnabled, getTraceDir, and getAllConfigurationValues here makes sense for building the archive and resolving workspace-relative paths.
64-64: LGTM: Provider config helpersparseModelConfig and getProviderConfigKeys are the right abstractions to source/redact provider settings.
697-701: No change required — getAllConfigurationValues is synchronous (no await or context needed)getAllConfigurationValues is exported as a synchronous function returning Record<string, any>, so calling JSON.stringify(getAllConfigurationValues(), ...) is correct.
- Definition: vscode/src/utilities/configuration.ts:71 —
export function getAllConfigurationValues(): Record<string, any> { ... }- Call site: vscode/src/commands.ts:697–701 —
JSON.stringify(getAllConfigurationValues(), null, 2),Original suggested diff (not needed):
- before:
JSON.stringify(getAllConfigurationValues(), null, 2),- suggested:
JSON.stringify(await getAllConfigurationValues(state.extensionContext.extensionPath), null, 2),Conclusion: remove the suggested change; no fix required.
Likely an incorrect or invalid review comment.
Fixes #378
Screencast.From.2025-08-13.19-13-50.mp4
This is what gets added to the archive (I included traces too):
These are the contents of extension-config.json:
This is how the provider settings looks like:
Summary by CodeRabbit
New Features
Tests