[Minitour]

Summary

Follow-ups to Qodo's review of the v2.4.0 release PR (#351). All six comments were checked against the code. Five are fixed as suggested; the sixth (redaction) is fixed in a narrower way that keeps #349's behavior.

What changed

• Plugin agents keep renamed skills (plugin-install.ts): when a skill is installed under a prefix after a name clash, the plugin's sub-agents that use it now reference the new id (paad-handoff) instead of silently dropping it.

• A failed plugin releases its skill names (plugin-install.ts): the per-plugin rollback removes that plugin's claims, so a later plugin with the same skill keeps the plain name.

• Lockfile pins must be commit SHAs (mirror.ts): a pin from capabilities.lock that isn't a hex SHA (7 to 40 characters) fails with an error naming the fix. A branch or tag there would otherwise float. Abbreviated SHAs from a hand-edited lockfile still work, as an existing test requires.

• Legacy plaintext that looks like ciphertext is encrypted (secret-crypto.ts): a value starting with enc:v1: is only treated as ciphertext when the rest is valid base64, long enough for the IV, auth tag and data. Anything else is legacy plaintext and is encrypted on the next database open. Well-formed ciphertext that doesn't decrypt is still left untouched (Secret migration re-encrypts values it can't decrypt, turning a wrong master key into permanent loss #305).

• Search says why a named tool is missing (mcp-handler.ts, mcp-tool-defaults.ts): the expose policy check from the unknown-tool error is now a shared exposurePolicyReason.

  • A tool hidden by an expose: except list gets the same explanation in search as in call_tool.
  • Under exactly or none, search adds the policy as a note.

• MCP env redaction catches more credential shapes (secret-redaction.ts, from fix: show MCP env placeholders and plain literals in the project API #350):

  • any URL userinfo, including a token alone (https://TOKEN@host);
  • credential parameters in query or connection strings (password=, pwd=, token=, sig=, ...);
  • base64-style secrets containing / or +, such as AWS secret keys.

  Paths and ordinary URLs (?region=eu) still show. Qodo also suggested hiding every literal by default; that would undo [Bug] Project API/UI shows empty MCP server env strings while secrets resolve at spawn #349, which asked for plain config values like REA_ANALYSIS_PROVIDER: ghidra to be visible, so literals are still shown unless their name or value reads as a credential.

Test plan

• Plugin conflict test: an agent declaring skills: [handoff] in the second plugin ends up with paad-handoff.
• resolveRef rejects pins main and v1.0.0; the existing short-SHA test still passes.
• A legacy enc:v1:not-really-ciphertext value is encrypted on open and still reads back unchanged; the wrong-key test still keeps values byte-identical.
• Search: an except-hidden name gets the except explanation, an exactly server adds its note, and an unknown name keeps the plain message.
• Redaction: Postgres URL with password=, token userinfo, ADO-style Pwd=, and an AWS-style secret are blanked; a URL with ?region=eu and a node_modules path are shown.
• Not covered by a test: the rollback releasing skill claims. It needs a plugin that fails after the skill loop, which can't be triggered without stubbing module internals. The change is three lines in the existing rollback.
• bunx tsc --noEmit; CLI, DB, server and shared suites have no assertion failures (Windows EBUSY cleanup and symlink EPERM only).

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 plugin, security, and MCP review findings

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Preserve renamed plugin skills in agents and release skill names after failed installs.
• Reject floating lockfile pins and safely migrate plaintext secrets resembling ciphertext.
• Explain MCP exposure policies in search and redact more credential-shaped environment values.

Diagram

graph TD
  M["Git mirror"] --> P["Plugin installer"] --> C["Merged capabilities"] --> S["Server services"] --> E["Exposure policy"] --> R["Search response"]
  D["Secret storage"] --> S --> V["Env redaction"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use explicit secret metadata instead of value heuristics

• ➕ Redaction would depend less on recognizing credential formats.
• ➖ Requires broader configuration and migration changes; does not protect existing unmarked credential literals.

Recommendation: Keep the targeted fixes for this release follow-up. Shared exposure-policy reasoning avoids divergent search and call errors, while heuristic redaction protects additional credentials without hiding ordinary configuration. Consider explicit secret metadata separately; blanket literal redaction would reverse the intended visibility of non-secret values.

Files changed (11) +175 / -39

Bug fix (6) +92 / -38

plugin-install.tsPreserve renamed skills and release failed-plugin claims +11/-2

Preserve renamed skills and release failed-plugin claims

• Tracks each plugin skill's manifest ID and installed ID so agents reference renamed skills. Rollback now removes the failed plugin's skill-name claims, allowing later plugins to use those names.

src/cli/commands/plugin-install.ts

mcp-handler.tsShare exposure-policy reasoning with MCP search +49/-31

Share exposure-policy reasoning with MCP search

• Extracts the missing-tool policy check into 'exposurePolicyReason' for both call errors and search. Named-tool searches now pass applicable denial or policy context into their response.

src/server/mcp-handler.ts

mcp-tool-defaults.tsInclude exposure context in search messages +5/-2

Include exposure context in search messages

• Accepts an optional missing-tool detail in search payloads. Definite denials replace the generic introduction; other policy notes follow it.

src/server/mcp-tool-defaults.ts

secret-redaction.tsRedact more credential-shaped MCP environment values +7/-1

Redact more credential-shaped MCP environment values

• Recognizes URL userinfo without a password, credential parameters, and long base64-style secrets. Keeps the existing behavior of showing ordinary literal configuration values.

src/server/secret-redaction.ts

mirror.tsRequire hexadecimal commit-SHA pins +7/-1

Require hexadecimal commit-SHA pins

• Validates lockfile pins as 7–40-character hexadecimal SHAs before resolution. The error explains how to remove or regenerate an invalid pin.

src/shared/cache/mirror.ts

secret-crypto.tsDistinguish ciphertext envelopes from prefix-like plaintext +13/-1

Distinguish ciphertext envelopes from prefix-like plaintext

• Checks base64 envelope shape and minimum length before treating a stored value as ciphertext. Malformed prefix-like plaintext is encrypted, while well-formed ciphertext that cannot decrypt remains untouched.

src/shared/secret-crypto.ts

Tests (5) +83 / -1

plugin-skill-conflicts.test.tsTest agent references to renamed plugin skills +12/-0

Test agent references to renamed plugin skills

• Adds a conflict case where an agent in the second plugin requests a skill that installs under a prefixed ID. Verifies the merged agent retains that installed ID.

src/cli/commands/tests/plugin-skill-conflicts.test.ts

secret-at-rest.test.tsTest migration of prefix-like plaintext +21/-0

Test migration of prefix-like plaintext

• Verifies that a legacy value beginning with 'enc:v1:' but lacking a valid envelope is encrypted on database open and still reads back unchanged.

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

mcp-handler.integration.test.tsTest missing-tool exposure explanations in search +26/-1

Test missing-tool exposure explanations in search

• Checks that search reports an 'except' denial, adds an 'exactly' policy note, and retains the plain message when no policy applies.

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

secret-redaction.test.tsTest additional credential shapes and safe values +21/-0

Test additional credential shapes and safe values

• Covers credential parameters, token-only URL userinfo, connection-string passwords, and an AWS-style secret. Confirms an ordinary URL and filesystem path remain visible.

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

cache.test.tsTest rejection of floating lockfile pins +3/-0

Test rejection of floating lockfile pins

• Adds assertions that branch and tag names supplied as pinned commits fail with a commit-SHA validation error.

src/shared/tests/cache.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. Empty secrets change during migration ✓ Resolved

Description

isCipherEnvelope requires 29 decoded bytes, although encryptSecret("") produces a valid 28-byte
IV-and-tag envelope, so canonicalizeStoredSecret treats that ciphertext as plaintext. When
database-open migration encounters a stored empty variable, including under a wrong master key, it
encrypts the envelope again instead of preserving it and counting a decryption failure; a subsequent
read decrypts only the outer layer and returns the former ciphertext rather than an empty string.

Code

src/shared/secret-crypto.ts[R46-54]

+/** IV (12) + auth tag (16) + at least one byte of data, base64 after the prefix. */
+const MIN_CIPHER_BYTES = 12 + 16 + 1;
+
+/** True when `value` has the shape encryptSecret produces (not whether it decrypts). */
+function isCipherEnvelope(value: string): boolean {
+	if (!value.startsWith(SECRET_CIPHER_PREFIX)) return false;
+	const body = value.slice(SECRET_CIPHER_PREFIX.length);
+	if (!/^[A-Za-z0-9+/]+={0,2}$/.test(body) || body.length % 4 !== 0) return false;
+	return Buffer.from(body, "base64").length >= MIN_CIPHER_BYTES;

Evidence

encryptSecret concatenates a 12-byte IV, a 16-byte authentication tag, and ciphertext, which has
zero bytes for an empty string; MIN_CIPHER_BYTES nevertheless requires one data byte.
VariablesRepo.set encrypts empty values, and migration’s zero-length guard checks the nonempty
stored ciphertext before calling canonicalizeStoredSecret. The failed envelope check sends that
ciphertext through the plaintext-encryption branch, while reads decrypt only one layer.

Preserve secrets that cannot be decrypted during migration
Restrict secret migration to eligible formats
src/shared/secret-crypto.ts[46-55]
src/shared/secret-crypto.ts[67-74]
src/db/migrate-secrets.ts[8-17]
src/shared/secret-crypto.ts[20-29]
src/shared/secret-crypto.ts[46-77]
src/db/migrate-secrets.ts[11-20]
src/db/variables.ts[10-18]
src/shared/secret-crypto.ts[20-26]
src/db/database.ts[78-82]
src/db/variables.ts[10-25]

Agent prompt

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

## Issue description
A valid encrypted empty secret contains only the IV and authentication tag, but the envelope check requires an additional byte. Database-open migration consequently encrypts the stored ciphertext again instead of preserving it; reads then return ciphertext, and decryption failures are not counted.
## Fix Focus Areas
- src/shared/secret-crypto.ts[46-55]
- src/db/migrate-secrets.ts[8-17]
- src/db/__tests__/secret-at-rest.test.ts[103-123]
## Recommended Fix
Set the minimum envelope size to `12 + 16` bytes (IV plus authentication tag) and update its comment to allow empty ciphertext data. Preserve structurally valid envelopes unchanged when decryption fails, and ensure those failures increment the undecryptable count exposed through the existing status and health paths. Add a database-open regression test that stores an encrypted empty variable, reopens the database twice, verifies the stored value remains byte-identical, and confirms reads return `""`.

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

---

2. Some configured MCP values disappear ✓ Resolved

Description

CREDENTIAL_VALUE treats username-only URL userinfo and some non-secret, base64-shaped relative
paths as credentials, and redactEnvValue replaces every matching environment value with an empty
string. When an MCP server is configured with a value such as ssh://git@github.com/org/repo.git or
opt/ghidra11/support/analyzeHeadless, the Project API and UI show an empty value even though the
original entry remains configured for spawning.

Code

src/server/secret-redaction.ts[R53-56]

+	// Any URL userinfo, `user:pass@` or a token alone (`https://TOKEN@host`).
+	/:\/\/[^/\s@]+@/,
+	// Credential parameters in a query string or connection string.
+	/(^|[?&;\s])(password|passwd|pwd|secret|token|access[-_]?token|api[-_]?key|access[-_]?key|sig|signature)=[^&;\s]/i,

Evidence

The userinfo regex requires only ://, characters before @, and @, so it matches
ssh://git@github.com/org/repo.git without a password. The base64 regex excludes a leading / but
permits internal slashes and requires only a letter and a digit, so it also matches relative paths
such as opt/ghidra11/support/analyzeHeadless. These patterns apply to every environment value, and
the redaction helper replaces matches with empty strings before the Project API returns them.

Display configured MCP server environment entries safely
src/server/secret-redaction.ts[53-62]
src/server/secret-redaction.ts[76-82]
src/server/project-routes.ts[207-213]
src/server/secret-redaction.ts[52-63]

Agent prompt

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

## Issue description
The redaction patterns blank configured non-secret values, including username-only URLs and base64-shaped relative paths, in the Project API and UI.
## Fix Focus Areas
- src/server/secret-redaction.ts[53-62]
- src/server/secret-redaction.ts[76-82]
- src/server/project-routes.ts[207-213]
## Recommended Fix
Preserve redaction for identifiable credentials, including `user:pass@` URLs, but narrow token-only userinfo detection so ordinary usernames such as `git` are not treated as secrets; for example, require a token of at least 16 characters mixing letters and digits. Narrow base64-shaped value detection so ordinary relative paths are not treated as secrets, using stronger indicators such as a `+`, a mix of uppercase, lowercase and digits, or a credential-specific length. Add regression tests showing that `ssh://git@github.com/org/repo.git` and `opt/ghidra11/support/analyzeHeadless` remain visible alongside the existing credential cases.

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

---

3. Plugin skills can collide after rollback ✓ Resolved

Description

The rollback deletes every skillOwners entry with the failed plugin's install ID, including claims
from an earlier successful entry with that same ID. If duplicate install IDs occur and the second
entry fails, its snapshot preserves the first entry's merged skill while a later plugin can claim
and append that skill's bare name again.

Code

src/cli/commands/plugin-install.ts[R1041-1043]

+        for (const [id, owner] of skillOwners) {
+          if (owner === pluginInstallId) skillOwners.delete(id);
+        }

Evidence

Plugin entries are processed without unique-install-ID validation, and IDs can normalize to the same
slug. The rollback truncates merged skills only to the current entry's snapshot but removes
ownership globally by ID; later name allocation relies on that ownership map.

src/shared/capabilities.ts[236-273]
src/shared/plugin-source.ts[205-211]
src/cli/commands/plugin-install.ts[487-493]
src/cli/commands/plugin-install.ts[765-790]
src/cli/commands/plugin-install.ts[1036-1050]

Agent prompt

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

## Issue description
Rollback removes skill ownership by install ID, although multiple accepted plugin entries can derive the same ID. A failure in the later entry can therefore remove an earlier entry's claim without removing its merged skill.
## Fix Focus Areas
- src/cli/commands/plugin-install.ts[487-493]
- src/cli/commands/plugin-install.ts[1036-1050]
## Recommended Fix
Snapshot skill ownership before processing each entry and restore that snapshot on rollback, rather than deleting every claim with the install ID. Test a failed duplicate-ID entry followed by another plugin claiming the same skill.

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

---

4. Subagents miss a hidden-tool explanation ✓ Resolved

Description

Search decides a name is missing using the subagent-filtered candidates, but
exposurePolicyReason suppresses its explanation when that name matches any tool in the unfiltered
project catalog. When an excluded callable tool shares a bare name with an expose: except tool,
the subagent gets a generic no-match response rather than the applicable policy explanation.

Code

src/server/mcp-handler.ts[R717-720]

+			const unknownName =
+				isToolNameQuery(query) && exactNameMatches(candidates, query).length === 0;
+			// Say why a named tool is missing when an `expose` policy explains it.
+			const policy = unknownName ? exposurePolicyReason(query.trim(), capabilities) : null;

Evidence

The subagent allowlist filters search candidates, whereas the new helper returns null upon finding
an exact match in all project tools, before it examines an except policy. Search includes policy
detail only when that helper returns it.

src/server/mcp-handler.ts[121-142]
src/server/mcp-handler.ts[318-335]
src/server/mcp-handler.ts[680-686]
src/server/mcp-handler.ts[717-723]
src/shared/server-tool-exposure.ts[159-190]

Agent prompt

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

## Issue description
Subagent search checks for an unknown name against its filtered catalog, then checks policy against the full project catalog. An inaccessible tool with the same bare name can suppress the explanation for a hidden tool.
## Fix Focus Areas
- src/server/mcp-handler.ts[121-142]
- src/server/mcp-handler.ts[680-686]
- src/server/mcp-handler.ts[717-723]
## Recommended Fix
Allow the policy helper's callable-name check to use the search caller's visible candidates while retaining the full-catalog behavior for ordinary tool calls. Add a subagent search test with an excluded callable tool and an `except`-hidden tool sharing a bare name.

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

---

5. Tool-not-found doc comment moved to the wrong function ✓ Resolved

Description

exposurePolicyReason and its JSDoc were inserted between the existing doc comment for
unknownToolMessage and that function. The old comment is now orphaned above the new function's
comment, and unknownToolMessage has no doc at all, so editors and readers see the wrong
description.

Code

src/server/mcp-handler.ts[R114-121]

+/**
+ * Why a tool name that isn't in the catalog may be missing, from the servers'
+ * `expose` policies. `hidden` is definite (the name is in an active `except`
+ * list, so the tool exists); `note` is context under `exactly` / `none`,
+ * where a hidden tool and a typo look the same. Null when no policy applies
+ * or the name is callable elsewhere.
+ */
+export function exposurePolicyReason(

Evidence

Lines 111-113 close the doc that describes the unknown-tool message (it mentions the get_issue
mapping). Directly after it, the new /** ... */ block and exposurePolicyReason begin, and
unknownToolMessage follows at line 154 with no doc.

src/server/mcp-handler.ts[105-160]

Agent prompt

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

## Issue description
The new function sits between `unknownToolMessage` and its JSDoc.
## Fix Focus Areas
- src/server/mcp-handler.ts[105-160]
## Recommended Fix
Move `exposurePolicyReason` and its JSDoc above the `unknownToolMessage` doc comment so each comment sits directly above its own function.

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

---

6. Search wording depends on a message prefix ✓ Resolved

Description

buildSearchPayload checks opts.detail?.startsWith("Tool ") to decide whether detail is a full
replacement message (the except case) or a note to append. Rewording either message in
exposurePolicyReason will quietly change or duplicate the search output, with nothing to catch it.

Code

src/server/mcp-tool-defaults.ts[R145-147]

+		? opts.detail?.startsWith("Tool ")
+			? `${opts.detail} `
+			: `No tool named "${query.trim()}" is exposed by this project. ${opts.detail ? `${opts.detail} ` : ""}`

Evidence

exposurePolicyReason already returns separate hidden and note fields, but the handler merges
them into one detail string (policy.hidden ?? policy.note). buildSearchPayload then works out
which one it got from the text prefix.

src/server/mcp-handler.ts[717-724]
src/server/mcp-tool-defaults.ts[136-150]

Agent prompt

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

## Issue description
Search payload output depends on a string-prefix check.
## Fix Focus Areas
- src/server/mcp-tool-defaults.ts[139-148]
- src/server/mcp-handler.ts[717-724]
## Recommended Fix
Change the opts to `{ unknownName?: boolean; hidden?: string; note?: string }`, pass `policy.hidden` and `policy.note` through unchanged, and branch on which field is set.

ⓘ 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 ↗