Skip to content

Redact credentials at tool-error construction and in source_errors - #234

Merged
wpak-ai merged 4 commits into
cppalliance:mainfrom
jonathanMLDev:fix/redaction-source-errors
Jul 22, 2026
Merged

Redact credentials at tool-error construction and in source_errors#234
wpak-ai merged 4 commits into
cppalliance:mainfrom
jonathanMLDev:fix/redaction-source-errors

Conversation

@jonathanMLDev

@jonathanMLDev jonathanMLDev commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Redact message and suggestion in pineconeToolError / timeoutToolError at construction so DEBUG-mode SDK errors are masked on the returned ToolError, not only at serialization time
  • Redact multi-source source_errors at construction (source-registry.ts) and in redactSensitiveFields (logger.ts)
  • Add redactErrorMessage helper and use it in shared error paths
  • Add/extend tests for direct-object redaction, source_errors unit/construction/e2e coverage, and UUID metadata regression
  • Update docs/SECURITY.md to document construction-layer + boundary-layer redaction

Test plan

  • npm test (434 tests)
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • npm run build

Related issue

Summary by CodeRabbit

  • Bug Fixes

    • Strengthened credential redaction across tool error payloads, retry warnings, per-source source_errors, Pinecone reranking failures, and CLI error output.
    • Improved source_errors sanitization to reliably redact nested string values while preserving non-sensitive metadata.
    • Standardized timeout tool errors so suggestion is always present and correctly handled when omitted or provided.
  • Documentation

    • Updated security documentation to clarify credential redaction behavior and the MCP response redaction boundaries.
  • Tests

    • Expanded redaction and MCP/error construction/serialization coverage (including DEBUG-mode assertions).

@jonathanMLDev
jonathanMLDev requested a review from wpak-ai as a code owner July 21, 2026 18:18
@jonathanMLDev jonathanMLDev self-assigned this Jul 21, 2026
@jonathanMLDev jonathanMLDev changed the title redact credentials at tool-error construction and in source_errors Redact credentials at tool-error construction and in source_errors Jul 21, 2026
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Credential-shaped substrings are now redacted during tool-error construction, source_errors aggregation, response serialization, retry logging, CLI error reporting, and reranking degradation reporting. Tests cover nested source errors, DEBUG-mode tool errors, multi-source failures, timeout suggestions, and non-sensitive UUID preservation.

Changes

Credential Redaction Hardening

Layer / File(s) Summary
Central redaction and tool-error construction
src/logger.ts, src/core/server/tool-error.ts, src/core/server/redaction.test.ts, src/core/server/tool-error.test.ts
Adds redactErrorMessage, recursively redacts source_errors, sanitizes tool-error messages and suggestions, and validates timeout suggestion behavior.
Source-error aggregation and end-to-end validation
src/core/server/source-registry.ts, src/core/server/tools/test-helpers.ts, src/core/server/*test.ts
Redacts per-source failure messages and adds shared failing-client fixtures plus DEBUG and partial-failure coverage.
CLI, retry, and reranking adoption
src/index.ts, src/core/server/retry.ts, src/core/pinecone/rerank.ts
Uses redactErrorMessage for configuration failures, fatal errors, retry warnings, and reranking degradation reasons.
Redaction coverage documentation
docs/SECURITY.md
Documents layered redaction and preservation of non-sensitive metadata.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PineconeSource
  participant SourceRegistry
  participant redactErrorMessage
  participant list_namespaces
  participant redactSensitiveFields
  PineconeSource->>SourceRegistry: reject namespace listing
  SourceRegistry->>redactErrorMessage: sanitize rejection reason
  redactErrorMessage-->>SourceRegistry: redacted source_errors entry
  SourceRegistry->>list_namespaces: return partial result
  list_namespaces->>redactSensitiveFields: serialize MCP response
  redactSensitiveFields-->>list_namespaces: redacted response text
Loading

Possibly related PRs

Suggested reviewers: wpak-ai

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR satisfies #229 by redacting tool-error construction, source_errors, and adding direct and end-to-end regression tests.
Out of Scope Changes check ✅ Passed The changes stay focused on redaction, test coverage, and security docs, with no clear unrelated scope.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: credential redaction during tool-error construction and in source_errors.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@91c7868). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/logger.ts 87.50% 1 Missing and 1 partial ⚠️
src/core/server/tools/test-helpers.ts 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #234   +/-   ##
=======================================
  Coverage        ?   86.00%           
=======================================
  Files           ?       47           
  Lines           ?     2537           
  Branches        ?      856           
=======================================
  Hits            ?     2182           
  Misses          ?      353           
  Partials        ?        2           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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: 1

🤖 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 `@docs/SECURITY.md`:
- Line 29: Update the UUID-preservation statement in SECURITY.md to limit the
claim to MCP response construction, aggregation, and serialization layers.
Remove the broad “at every layer” wording while preserving the separate behavior
that UUID-shaped tokens are redacted from logs.
🪄 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

Run ID: 4875882f-2db0-43d5-a387-c1ff83a2a8a5

📥 Commits

Reviewing files that changed from the base of the PR and between 91c7868 and 378cebc.

📒 Files selected for processing (11)
  • docs/SECURITY.md
  • src/core/pinecone/rerank.ts
  • src/core/server/redaction.test.ts
  • src/core/server/retry.ts
  • src/core/server/source-registry.test.ts
  • src/core/server/source-registry.ts
  • src/core/server/tool-error.ts
  • src/core/server/tools/list-namespaces-tool.context.test.ts
  • src/core/server/tools/test-helpers.ts
  • src/index.ts
  • src/logger.ts

Comment thread docs/SECURITY.md Outdated
@jonathanMLDev
jonathanMLDev requested a review from timon0305 July 21, 2026 18:53
Comment thread src/core/server/tool-error.ts
Comment thread src/core/server/tool-error.ts Outdated

@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: 1

🤖 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/core/server/tool-error.ts`:
- Line 126: Update the suggestion assignment in the tool-error construction to
distinguish an omitted value from an explicitly provided empty string, using an
undefined check or nullish fallback while still applying redactApiKey to
supplied strings. Add a regression test covering an empty options.suggestion and
verify it is preserved instead of replaced by DEFAULT_TIMEOUT_SUGGESTION.
🪄 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

Run ID: 7d7050c1-8bbd-479f-8370-da8dc5775f4a

📥 Commits

Reviewing files that changed from the base of the PR and between 378cebc and 5c916cd.

📒 Files selected for processing (3)
  • docs/SECURITY.md
  • src/core/server/tool-error.test.ts
  • src/core/server/tool-error.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/SECURITY.md

Comment thread src/core/server/tool-error.ts Outdated
@wpak-ai
wpak-ai merged commit 1bdb56a into cppalliance:main Jul 22, 2026
10 checks passed
@jonathanMLDev
jonathanMLDev deleted the fix/redaction-source-errors branch July 30, 2026 18:01
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.

Redact credentials at the error-construction layer and fix source_errors leak

3 participants