Skip to content

Audit remediation: full P0–P3 fix pass (2026-05-11 plan) - #87

Merged
AndresL230 merged 14 commits into
mainfrom
audit-fixes-2026-05-11
May 12, 2026
Merged

Audit remediation: full P0–P3 fix pass (2026-05-11 plan)#87
AndresL230 merged 14 commits into
mainfrom
audit-fixes-2026-05-11

Conversation

@AndresL230

@AndresL230 AndresL230 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the 2026-05-11 deep-audit punch list — 3 P0, 5 P1, 9 P2, 3 P3 findings — across 25 plan tasks. Execution log lives at docs/superpowers/plans/2026-05-11-audit-p0-fixes.md.

Headline changes:

  • src/webview-provider.ts god-file decomposed: 2281 → 791 lines. Four cohesive handlers extracted under src/webview/: ChatHandler (706), KeyManagementHandler (210), SimulationHandler (53), ScanPublishingHandler (788).
  • P0 listener and async fixes: webview message listener now registered with context.subscriptions + disposed on view dispose; dispatchWebviewMessage adds exhaustive default case with never check and a try/catch error boundary; compressClusters is now async so the extension host main thread no longer blocks on fs.readFileSync during AI-context compression.
  • Single source of truth for pricing: estimateLocalMonthlyCost deduplicated — local copies in webview-provider.ts and scan-results.ts deleted; intelligence/cost-utils.ts is now the only implementation, with unit tests.
  • Persistence race fixed: scenario writes serialized through a single-flight scenarioPersistQueue (now owned by SimulationHandler); storage key aligned to recost.simulatorScenarios.
  • Activation hardening: pricing-sync setInterval guarded against double-activate; console.error / console.log leaks routed through the user-facing OutputChannel; debug-export failures surface via showErrorMessage.
  • Build & deps: esbuild^0.28 (closes GHSA-67mh-4wv8-2f99npm audit reports 0 vulnerabilities); openai^4.104 (v4 line; v6 deferred). Release builds drop sourcemaps and minify behind a new --release esbuild flag; vsce package now uses it. VSIX artifact-name mismatch in scripts/build-vsix.sh fixed.
  • Test coverage added: api-client (rc- prefix gate, 401/404/200 paths), key-management (registry, masking, fingerprint, summary), cost-utils (null contract, zero, table, fingerprint), AST regex-fallback when web-tree-sitter is disabled, async compressClusters, dispatchWebviewMessage error/unknown/ok branches, and an activation smoke test for extension.ts (loads dist/extension.js with a stubbed vscode module).
  • CI: new GitHub Actions workflow runs the full test suite + secret scan on push and PR.
  • Types: noImplicitOverride enabled in tsconfig.json.

Known caveats before merge

  • One unrelated commit landed on this branch by accident during the work: 5139dd6 feat(span): add SourceSpan type and helpers (issue #80). It likely belongs on foundation-parser-accuracy — consider cherry-picking it over there and dropping it here.
  • 8 pre-existing test compile errors live on this branch (none introduced by this PR): ast-*-detector.test.ts confidence optionality mismatches, export.test.ts missing costLeaks, recost-mock-calls.ts missing @anthropic-ai/sdk/stripe modules, filesystem-adapter.ts iterator type. Worth a follow-up cleanup pass — CI will surface them on this PR.
  • noUncheckedIndexedAccess and exactOptionalPropertyTypes from the original A2 spec were skipped after a quick scan surfaced too many cascading errors; only noImplicitOverride landed. Promote to a follow-up if desired.

Test plan

  • CI passes (or surfaces only the 8 pre-existing errors noted above)
  • npm run build succeeds locally
  • npm run package produces a valid VSIX
  • Install the VSIX in a clean VSCode instance, run a scan, open simulator, switch AI provider, validate ReCost key — no regressions vs. main
  • npm audit reports 0 vulnerabilities

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • AI-powered review workflow with chat integration
    • Dedicated key/API management UI for multiple providers
    • Simulation scenarios for workspace optimization
    • Enhanced scan publishing with local/remote merging and debug export
    • Precise source-span detection for findings
    • VS Code activation test and test vscode shim for CI
  • Bug Fixes

    • Fixed async compression handling in scan/context flows
  • Chores

    • Bumped OpenAI and esbuild deps; expanded scanner test script and async test infra

Review Change Stack

AndresL230 and others added 13 commits May 12, 2026 00:38
…loader

The persistScenarios helper introduced in 7bed29e wrote to eco.simulatorScenarios
but the constructor loads from recost.simulatorScenarios, so writes through the
queue would never round-trip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves handleRunSimulation, persistScenarios, and the single-flight
scenarioPersistQueue (introduced in C5) into src/webview/simulation-handler.ts.
Structurally pure extraction — no behavioral change. webview-provider.ts
drops from 1556 to 1534 lines.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves handleStartScan plus the URL-canonicalization, endpoint-merging,
local-finding-aggregation, and aggressive-suggestion helpers it depends
on (~470 lines) into src/webview/scan-publishing-handler.ts. The handler
calls back into the provider through a ScanPublishingHandlerContext for
state mutations, key validation updates, project resolution, and the
debug-export side effect.

Structurally pure extraction — no behavioral change. webview-provider.ts
drops from 1534 to 791 lines (under the ~800 target set by the plan).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a lightweight activation test that loads the built dist/extension.js
with vscode hot-swapped for a minimal local stub. Verifies activate() and
deactivate() are exported, and that deactivate() is idempotent without a
prior activate() call (catches state-assumption regressions on edge-case
shutdown sequences).

Loading the built artifact rather than compiling extension.ts under the
scanner tsconfig keeps the test compile graph small — extension.ts pulls
in the entire webview-provider closure and would force the scanner test
tsconfig to grow into a parallel full-extension compile.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
npm audit reports 0 vulnerabilities after the bump.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tightens the floor inside the v4 line; the lockfile was already resolving
to a recent 4.x patch under the prior ^4.73.0 constraint, so this bump is
range-tightening only — no new code installed and no breaking changes.

v6 migration deferred — needs integration tests for the chat adapter
before we can safely move past v4.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 18c51df6-51c1-4b2f-9fff-6b9686cfaa1f

📥 Commits

Reviewing files that changed from the base of the PR and between 3f28a41 and 93c0972.

📒 Files selected for processing (5)
  • src/intelligence/__tests__/export.test.ts
  • src/webview-provider.ts
  • src/webview/chat-handler.ts
  • src/webview/scan-publishing-handler.ts
  • src/webview/simulation-handler.ts

📝 Walkthrough

Walkthrough

This PR extracts chat, key management, simulation, and scan publishing logic into dedicated handler classes while making compressClusters() asynchronous across CLI, extension, and webview call sites. It also adds VS Code test mocks, an activation test, and a SourceSpan utility.

Changes

Webview handler extraction and async compression refactoring

Layer / File(s) Summary
Async compression infrastructure
package.json, src/cli/scan.ts, src/extension.ts, src/intelligence/__tests__/compression.test.ts, src/intelligence/__tests__/export.test.ts, src/webview-provider.ts
Expand test:scanner script and bump deps (openai, esbuild). Convert compressClusters() call sites to awaited invocations and update tests to an async queued execution model.
Source location tracking utilities
src/scanner/source-span.ts
Introduce SourceSpan interface with 1-based line/0-based column semantics and helpers pointSpan() and spanFromMatch() for match-end computation.
Test infrastructure and VS Code mocking
src/test/__mocks__/vscode.ts, src/test/extension-activation.test.ts
Add a minimal vscode shim for tests and a Node-based activation test that requires the built extension artifact and verifies activate/deactivate behavior (safe double-deactivate).
Chat and AI review handler
src/webview/chat-handler.ts
Add ChatHandler to manage chat history, provider/model selection, AI-review context construction with redaction, provider execution (OpenAI retry path), JSON findings parsing/validation, mapping to Suggestion objects with savings estimation, merging/deduplication, and auth error handling.
Key management handler
src/webview/key-management-handler.ts
Add KeyManagementHandler and context: read/set/test keys, persist validation snapshots with fingerprint invalidation, compute/send key-status summaries, and update VS Code context on recost validation.
Simulation handler
src/webview/simulation-handler.ts
Add SimulationHandler that loads/saves simulator scenarios in globalState, validates endpoints before simulation, runs runSimulation with StaticDataSource, posts results/errors, and persists scenarios via a queued promise chain.
Scan publishing handler
src/webview/scan-publishing-handler.ts
Add ScanPublishingHandler to run workspace scans, detect local waste, merge local synthetic endpoints with remote results, generate aggressive/local-rule suggestions with pricing/savings estimation, submit remote scans (with project-creation retry), handle auth errors, and export structured debug payloads.
Webview provider refactoring
src/webview-provider.ts
Refactor ReCostSidebarProvider to instantiate handlers (chat, key management, simulation, scan publishing), delegate implementations to them, wire state callbacks, simplify exportDebugScanResults to accept ExportDebugPayload, and route webview messages to handlers.

Sequence Diagram(s)

sequenceDiagram
  participant Webview
  participant ChatHandler
  participant AIModel
  Webview->>ChatHandler: handleRunAiReview()
  ChatHandler->>ChatHandler: build review input context
  ChatHandler->>ChatHandler: select provider and model
  ChatHandler->>AIModel: post review request with JSON contract
  activate AIModel
  AIModel-->>ChatHandler: JSON findings response
  deactivate AIModel
  ChatHandler->>ChatHandler: parse and validate findings
  ChatHandler->>ChatHandler: map findings to suggestions
  ChatHandler->>ChatHandler: merge with existing suggestions
  ChatHandler-->>Webview: post scanResults + completion event
Loading
sequenceDiagram
  participant Handler
  participant Workspace
  participant RemoteAPI
  participant Webview
  Handler->>Workspace: load and run workspace scan
  Handler->>Handler: detect local waste patterns
  alt has remote ReCost key
    Handler->>Handler: filter submittable endpoints
    Handler->>RemoteAPI: submit eligible API calls
    activate RemoteAPI
    alt 404 for auto project target
      Handler->>RemoteAPI: create project first
      Handler->>RemoteAPI: retry submission
    end
    RemoteAPI-->>Handler: remote endpoints and suggestions
    deactivate RemoteAPI
    Handler->>Handler: merge local and remote endpoints
    Handler->>Handler: generate aggressive suggestions
  end
  Handler->>Webview: post scan results and debug export
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • recost-dev/extension#87: Overlapping changes including async compressClusters usage, webview handler extraction, tests/mocks, SourceSpan, and package/test updates.

Poem

🐰 A rabbit hops through handlers now,
Chat and keys and scans align—
Async clusters bloom anew,
Compress with care, await the way,
Tests that mock the VS Code sky!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Audit remediation: full P0–P3 fix pass (2026-05-11 plan)' directly summarizes the main objective of the changeset, which is a comprehensive audit fix addressing all severity levels.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch audit-fixes-2026-05-11

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/intelligence/__tests__/export.test.ts (1)

202-212: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix TypeScript compilation error: add missing costLeaks property.

The summary object is missing the required costLeaks property, causing the pipeline to fail with: Property 'costLeaks' is missing in type ... but required in type.

🐛 Proposed fix
   const context: ExportedContext = {
     meta: {
       projectName: "demo-project",
       generatedAt: "2026-03-26T12:00:00.000Z",
       generatorVersion: "0.1.0",
       totalFiles: 3,
       totalClusters: 1,
       providers: ["openai", "stripe"],
     },
     summary: {
       topFiles: [
         {
           filePath: "src/chat/loop.ts",
           whyItMatters: "This file runs repeated API work inside an unbounded loop, so it is a strong review target.",
         },
       ],
       keyRisks: ["Unbounded loop API calls", "Rate-limit risk"],
+      costLeaks: [],
     },
     clusters,
   };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/intelligence/__tests__/export.test.ts` around lines 202 - 212, The
summary object returned in the test is missing the required costLeaks property,
causing a TypeScript compile error; update the object (the variable named
summary in the test) to include a costLeaks field with the correct shape/value
expected by the type (e.g., an empty array or appropriate mock leak entries) so
the returned object matches the expected type alongside topFiles and keyRisks;
adjust the test helper or fixture that builds summary (refer to summary and
clusters in this diff) to always provide costLeaks.
src/webview-provider.ts (1)

517-530: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return the simulation promise so dispatch can actually await it.

This wrapper currently returns void, so dispatchWebviewMessage() reports "ok" immediately and its try/catch never sees async failures from SimulationHandler.handleRunSimulation(...).

Suggested fix
-      runSimulation: (input) => { this.simulationHandler.handleRunSimulation(input); },
+      runSimulation: (input) => this.simulationHandler.handleRunSimulation(input),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/webview-provider.ts` around lines 517 - 530, The runSimulation handler
passed into dispatchWebviewMessage is currently calling
this.simulationHandler.handleRunSimulation(input) without returning its promise,
so dispatchWebviewMessage resolves immediately; change the runSimulation entry
to return the promise from SimulationHandler.handleRunSimulation (i.e., make the
arrow handler return this.simulationHandler.handleRunSimulation(input) or mark
it async and await/return the call) so dispatchWebviewMessage can await it and
catch any errors. Ensure you update the runSimulation mapping in the
dispatchWebviewMessage call where runSimulation: (input) => {
this.simulationHandler.handleRunSimulation(input); } is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/webview/chat-handler.ts`:
- Around line 204-209: redactSensitiveText currently only masks OpenAI-style sk-
keys and named tokens, so ReCost-style keys (e.g., rc-<alphanum>) can leak;
update redactSensitiveText to also replace any rc-[A-Za-z0-9_-]{8,} (or similar
length) patterns with "[REDACTED_RECOST_KEY]" and ensure
buildAiReviewInputContext calls redactSensitiveText on any snippet before
forwarding to external models (locate redactSensitiveText and
buildAiReviewInputContext to apply the new regex and confirm all snippet paths
are sanitized).

In `@src/webview/scan-publishing-handler.ts`:
- Around line 566-574: publishLocalOnlyResults currently skips deriving
endpoint-based aggressive suggestions; before calling mergeLocalWasteFindings in
publishLocalOnlyResults (and the similar block around lines 698-705), run
buildAggressiveSuggestions against the merged endpoints produced by
mergeRemoteAndLocalEndpoints (use the same inputs: [], apiCalls, localProjectId,
localScanId or their counterparts) and include the returned aggressive
suggestions when calling mergeLocalWasteFindings (i.e., compute endpoints via
mergeRemoteAndLocalEndpoints, call buildAggressiveSuggestions(endpoints, ...) to
get aggressiveSuggestions, then pass/merge those into mergeLocalWasteFindings
along with localWasteFindings and existing endpoints). Ensure you reference
publishLocalOnlyResults, mergeRemoteAndLocalEndpoints,
buildAggressiveSuggestions, mergeLocalWasteFindings, localWasteFindings and
apiCalls when editing.
- Around line 174-183: The extras entry uses chooseSeverity(...) to compute
severity but then calls calculateSavings with the hardcoded string "medium";
update the extras push so that estimatedMonthlySavings calls calculateSavings
with the computed severity variable (the value returned by chooseSeverity)
instead of "medium" — locate the object literal created in the extras.push(...)
and replace the literal "medium" passed to calculateSavings with the local
severity value returned by chooseSeverity(endpoint.status,
endpoint.monthlyCost).

In `@src/webview/simulation-handler.ts`:
- Around line 42-48: persistScenarios currently assigns this.savedScenarios
before persisting, risking in-memory/persistent divergence if
ctx.context.globalState.update(SimulationHandler.SCENARIOS_STORAGE_KEY, next)
fails; change the flow so the update to this.savedScenarios occurs only after
the queued persistence Promise resolves successfully (or, if you prefer to
optimistically set it, add a catch on scenarioPersistQueue that reverts
this.savedScenarios to the previous value and rethrows/logs the error). Ensure
changes touch persistScenarios, scenarioPersistQueue handling, and the
globalState.update call so the queue still serializes writes but guarantees
in-memory state matches persisted state on success (or is reverted on failure).

---

Outside diff comments:
In `@src/intelligence/__tests__/export.test.ts`:
- Around line 202-212: The summary object returned in the test is missing the
required costLeaks property, causing a TypeScript compile error; update the
object (the variable named summary in the test) to include a costLeaks field
with the correct shape/value expected by the type (e.g., an empty array or
appropriate mock leak entries) so the returned object matches the expected type
alongside topFiles and keyRisks; adjust the test helper or fixture that builds
summary (refer to summary and clusters in this diff) to always provide
costLeaks.

In `@src/webview-provider.ts`:
- Around line 517-530: The runSimulation handler passed into
dispatchWebviewMessage is currently calling
this.simulationHandler.handleRunSimulation(input) without returning its promise,
so dispatchWebviewMessage resolves immediately; change the runSimulation entry
to return the promise from SimulationHandler.handleRunSimulation (i.e., make the
arrow handler return this.simulationHandler.handleRunSimulation(input) or mark
it async and await/return the call) so dispatchWebviewMessage can await it and
catch any errors. Ensure you update the runSimulation mapping in the
dispatchWebviewMessage call where runSimulation: (input) => {
this.simulationHandler.handleRunSimulation(input); } is defined.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c237d870-ede6-44ed-8fa6-990af014be92

📥 Commits

Reviewing files that changed from the base of the PR and between 88cc758 and 3f28a41.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (13)
  • package.json
  • src/cli/scan.ts
  • src/extension.ts
  • src/intelligence/__tests__/compression.test.ts
  • src/intelligence/__tests__/export.test.ts
  • src/scanner/source-span.ts
  • src/test/__mocks__/vscode.ts
  • src/test/extension-activation.test.ts
  • src/webview-provider.ts
  • src/webview/chat-handler.ts
  • src/webview/key-management-handler.ts
  • src/webview/scan-publishing-handler.ts
  • src/webview/simulation-handler.ts

Comment thread src/webview/chat-handler.ts
Comment thread src/webview/scan-publishing-handler.ts
Comment thread src/webview/scan-publishing-handler.ts
Comment thread src/webview/simulation-handler.ts
- chat-handler: redact rc- ReCost keys in addition to sk- OpenAI keys
  before forwarding snippets to external chat providers
- scan-publishing-handler: use computed severity (not hardcoded "medium")
  when calling calculateSavings in buildAggressiveSuggestions
- scan-publishing-handler: run buildAggressiveSuggestions in the
  local-only publish path so offline scans surface the same cache/batch/
  n_plus_one suggestions as the remote-enriched path
- simulation-handler: move savedScenarios in-memory update to after the
  globalState.update resolves, preventing in-memory/persisted divergence
  on storage failure
- webview-provider: return the runSimulation handler's value from the
  dispatch wrapper to match surrounding entries (defensive cleanup)
- export.test.ts: add costLeaks and providerSummary to inline
  ExportedContext literals (TS compile error: both are required fields)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant