[Minitour]

Summary

Second round of Qodo follow-ups for the v2.4.0 release, on top of #352. All six comments on #352 were valid and are fixed here.

Stacked on #352: merge this into fix/release-240-review first, then #352 into develop.

What changed

• Encrypted empty secrets (secret-crypto.ts): an encrypted empty string is only the IV and auth tag (28 bytes). fix: follow-ups from the v2.4.0 release review #352 required 29, so the migration would have encrypted it a second time, and reads would have returned ciphertext. The minimum is now 28.
• Redaction no longer hides ordinary values (secret-redaction.ts):
  • URL userinfo counts as a credential only as user:pass@, or as a token of 16 or more mixed letters and digits, so ssh://git@github.com/org/repo.git stays visible.
  • A base64-style value counts only with + or =, or as exactly 40 mixed-case characters (an AWS secret key), so relative paths like opt/ghidra11/support/analyzeHeadless stay visible.
• Rollback restores skill claims (plugin-install.ts): a failed entry restores the claims snapshot taken before it, instead of deleting every claim with its install id. Another entry may share that id.
• Sub-agent search (mcp-handler.ts): exposurePolicyReason takes the tools the caller can see, so a tool the sub-agent can't call no longer suppresses the explanation for a hidden one.
• Structured search detail (mcp-tool-defaults.ts): buildSearchPayload takes hidden and note separately instead of checking whether a message starts with "Tool ".
• Doc comments (mcp-handler.ts): each one sits above its own function again.

Test plan

• An encrypted empty secret stays byte-identical across two reopens and reads back as "". This test fails against fix: follow-ups from the v2.4.0 release review #352.
• Redaction: ssh://git@github.com/org/repo.git and opt/ghidra11/support/analyzeHeadless are shown; a 40-character ghp_ token in a URL, password=, Pwd= and an AWS-style secret are blanked.
• exposurePolicyReason: null for the main agent when the tool is callable elsewhere; the except explanation when the caller can't see that tool.
• Search payload tests now use hidden and note.
• Not covered by a test: the rollback snapshot. It needs a plugin failing after the skill loop, which can't be triggered without stubbing module internals.
• bunx tsc --noEmit; CLI, DB, server and shared suites have no assertion failures.

Checklist

• Tests added or updated
• bunx tsc --noEmit passes
• Docs updated (if user-facing): not needed

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix v2.4.0 secret handling, plugin rollback, and tool search

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Preserve encrypted empty secrets across database reopens and avoid redacting ordinary URLs and
 paths.
• Restore skill claims precisely when plugin resolution fails.
• Make missing-tool explanations respect sub-agent visibility and use structured policy details.

Diagram

graph TD
  A["Stored secrets"] --> B["Cipher validation"]
  C["Environment values"] --> D["API redaction"]
  E["Skill claims"] --> F["Snapshot rollback"]
  G["Caller tools"] --> H["Search explanation"]

Loading

High-Level Assessment

Keep the targeted fixes: they correct the existing encryption, redaction, rollback, and search paths without introducing a new mechanism. A broader replacement for heuristic redaction would be a separate architectural change, not a better fit for these release follow-ups.

Files changed (8) +101 / -31

Bug fix (5) +56 / -28

plugin-install.tsRestore skill ownership after a failed plugin entry +6/-4

Restore skill ownership after a failed plugin entry

• Snapshots skill-name claims before resolving each plugin entry and restores that snapshot on failure. This preserves earlier claims even when another entry shares the failed entry's install ID.

src/cli/commands/plugin-install.ts

mcp-handler.tsUse caller-visible tools for missing-tool explanations +15/-9

Use caller-visible tools for missing-tool explanations

• Passes search candidates into exposure-policy checks so sub-agent explanations reflect tools the caller can see. Forwards structured policy details to the search payload and places the affected documentation comment above its function.

src/server/mcp-handler.ts

mcp-tool-defaults.tsBuild search messages from structured policy details +11/-7

Build search messages from structured policy details

• Replaces the message-prefix check with separate hidden-tool and policy-note options. Hidden-tool text replaces the generic missing-tool sentence; a note supplements it.

src/server/mcp-tool-defaults.ts

secret-redaction.tsNarrow credential-value redaction heuristics +22/-6

Narrow credential-value redaction heuristics

• Requires URL userinfo to contain a password or resemble a long token, preserving ordinary usernames. Tightens base64-style detection so relative paths are less likely to be blanked while likely secrets remain protected.

src/server/secret-redaction.ts

secret-crypto.tsRecognize the envelope of an encrypted empty secret +2/-2

Recognize the envelope of an encrypted empty secret

• Lowers the minimum ciphertext-envelope size to the 12-byte IV plus 16-byte authentication tag. This prevents an encrypted empty value from being treated as plaintext and encrypted again during migration.

src/shared/secret-crypto.ts

Tests (3) +45 / -3

secret-at-rest.test.tsCover encrypted empty secrets across reopens +22/-0

Cover encrypted empty secrets across reopens

• Adds a regression test that reopens the database twice, checks that the stored ciphertext remains unchanged, and verifies that the secret reads back as an empty string.

src/db/tests/secret-at-rest.test.ts

mcp-handler.integration.test.tsCover caller-visible tool policy explanations +18/-2

Cover caller-visible tool policy explanations

• Updates search-payload assertions to pass structured policy details. Adds a case showing that a tool callable by the main agent must not suppress a hidden-tool explanation for a sub-agent that cannot see it.

src/server/tests/mcp-handler.integration.test.ts

secret-redaction.test.tsDistinguish credentials from Git URLs and relative paths +5/-1

Distinguish credentials from Git URLs and relative paths

• Extends redaction coverage to retain an SSH Git URL and a relative path while still blanking token-bearing URLs, connection-string passwords, and an AWS-style secret.

src/server/tests/secret-redaction.test.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Letter-only URL tokens reach clients ✓ Resolved

Description

The token-only URL matcher requires both a letter and a digit, so redactEnvValue leaves a URL such
as https://abcdefghijklmnop@host.example/repo unchanged under a nonsensitive environment name.
When that value is configured as a server environment variable, the project API returns it through
redactServerForApi instead of blanking the userinfo.

Code

src/server/secret-redaction.ts[57]

+	/:\/\/(?=[^/\s:@]*\d)(?=[^/\s:@]*[A-Za-z])[^/\s:@]{16,}@/,

Evidence

The new userinfo matcher rejects tokens without digits, and the other value matchers do not cover a
letter-only token embedded in a URL. Nonsensitive environment values that pass these checks are
returned unchanged in the project API's server payload.

src/server/secret-redaction.ts[52-76]
src/server/secret-redaction.ts[89-99]
src/server/secret-redaction.ts[109-129]
src/server/project-routes.ts[195-215]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Letter-only tokens in URL userinfo bypass environment-value redaction and appear in project API responses.
## Fix Focus Areas
- src/server/secret-redaction.ts[53-57]
- src/server/__tests__/secret-redaction.test.ts[94-116]
## Recommended Fix
Expand URL userinfo detection to cover opaque letter-only tokens while retaining the exception for ordinary usernames such as `ssh://git@github.com`; test both cases through `redactServerForApi`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unpadded Base64 secrets reach clients ✓ Resolved

Description

isBase64Secret rejects Base64-shaped values containing / unless they also contain + or =, or
are exactly 40 mixed-case characters. A 32-character value such as
AbcdEFGH/ijkLMNO1234567890QRSTUV therefore bypasses both it and the no-path-separator credential
matcher, allowing a nonsensitive server environment value to appear in the project API response.

Code

src/server/secret-redaction.ts[R74-75]

+	if (/[+=]/.test(value)) return true;
+	return value.length === 40 && /[A-Z]/.test(value) && /[a-z]/.test(value);

Evidence

The new predicate accepts this 32-character example's shape but rejects it at the final condition;
the separate long-token regex excludes /. The environment redactor then preserves the value, and
the project route uses that redactor when returning server definitions.

src/server/secret-redaction.ts[59-76]
src/server/secret-redaction.ts[89-99]
src/server/secret-redaction.ts[109-129]
src/server/project-routes.ts[195-215]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unpadded Base64-shaped credentials containing `/` can bypass server environment-value redaction.
## Fix Focus Areas
- src/server/secret-redaction.ts[65-76]
- src/server/__tests__/secret-redaction.test.ts[94-116]
## Recommended Fix
Adjust Base64 credential detection to cover unpadded secret values with `/` without reclassifying ordinary relative paths, and add API-redaction tests for both cases.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗