fix: address review findings on the v2.3.0 release

[Minitour]

Summary

Fixes the four findings Qodo raised on the v2.3.0 release PR (#291). All four are in code merged to develop this cycle.

What changed

• Partial master.key read (from fix: use the OS keychain in compiled binaries for new installs #283): where hard links aren't available the key file is created with a direct create-new write, so a concurrent process could read fewer than 32 bytes and fail as "corrupt". A file shorter than a key is now re-read for up to a second before it counts as invalid; longer or wrong files still fail immediately.
• Credentials in capabilities.yaml (from Install skills and plugins from any git host #279): capa add https://user:token@host/repo.git wrote the token into the project file. parseGitUrlSource now strips http(s) userinfo (ssh git@ is kept); private hosts use the git credential helper.
• Registry disabled mid-refresh (from feat(web): add Refresh all to registry settings #284): the enabled flag was only checked before the download, so a registry disabled while its new adapter downloaded still executed it. Both the server and CLI refresh re-check after the download and before running the adapter (RegistryDisabledError, 409 from the server; status left untouched).
• Anchored MCP matchers (from fix: record Claude skills, sub-agents and MCP calls in agent activity #288): ^mcp__github__.*$ or a wrapping (?:...) group failed the "already within the family" check and was unioned with mcp__.*, firing for every MCP tool. Anchors and one wrapping group are now ignored for that check; other groups still fall back to the union.

Screenshots / logs

N/A.

Test plan

• master-key-store.test.ts: a separate process finishes writing the key while capa reads it (16 pass).
• git-url.test.ts: credentials dropped for https, ssh://git@ kept (16 pass).
• registries-routes.test.ts: a registry disabled during the fetch gets 409, its adapter never runs (marker file absent) and it stays disabled; with registry.test.ts and src/shared/registries: 111 pass.
• hook-handlers.test.ts: anchored and grouped scoped matchers stay scoped; multi-group matchers still union (32 pass with src/cli/utils/hooks).
• bunx tsc --noEmit -p . and -p web-ui.

Checklist

• Tests added or updated
• bunx tsc --noEmit passes
• bun run smells clean vs base (or CI Qlty Smells job green)
• Docs updated (if user-facing)

🤖 Generated with Claude Code

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

PR Summary by Qodo

Fix four v2.3.0 release review findings

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Wait briefly for partially written master keys instead of treating concurrent writes as
 corruption.
• Remove HTTP credentials from stored Git URLs and prevent disabled registry adapters from
 executing.
• Keep anchored MCP hook matchers scoped to their intended tools.

Diagram

graph TD
  CLI["CLI refresh"] --> Stage["Stage adapter"] --> Enabled{"Registry enabled?"} -->|yes| Execute["Execute adapter"]
  API["API refresh"] --> Stage
  Enabled -->|no| Abort["Abort refresh"]

Loading

High-Level Assessment

The targeted approach fits these independent fixes. Reusing the existing HTTP URL sanitizer avoids another credential-handling path, while a pre-execution callback lets both refresh entry points enforce the registry check without coupling the shared installer to the database.

Files changed (10) +119 / -21

Bug fix (6) +72 / -13

registry.tsStop CLI refresh when a registry is disabled mid-download +9/-2

Stop CLI refresh when a registry is disabled mid-download

• Re-checks the registry's enabled state before executing its staged adapter. A mid-refresh disable aborts without marking the registry failed.

src/cli/commands/registry.ts

registries-routes.tsGuard API refresh immediately before adapter execution +16/-1

Guard API refresh immediately before adapter execution

• Adds a post-download enabled-state check to the server refresh path. A registry disabled during the fetch returns 409 without executing its adapter or setting a failed status.

src/server/registries-routes.ts

git-url.tsStrip HTTP userinfo from parsed Git sources +3/-1

Strip HTTP userinfo from parsed Git sources

• Uses the existing URL sanitizer before returning a parsed clone URL, preventing embedded credentials from reaching stored capability definitions. Non-HTTP transports retain their existing behavior.

src/shared/git-url.ts

master-key-store.tsRetry reads of incomplete master keys +22/-7

Retry reads of incomplete master keys

• Polls a short master.key for up to one second so a concurrent create-new write can finish. Oversized files remain invalid immediately.

src/shared/master-key-store.ts

hook-handlers.tsRecognize scoped matchers despite anchors and wrapping groups +8/-1

Recognize scoped matchers despite anchors and wrapping groups

• Ignores anchors and one simple wrapping group when checking whether a user matcher is already within a tool family. Other grouped patterns retain the conservative union behavior.

src/shared/providers/hook-handlers.ts

installer.tsProvide a pre-execution guard for staged adapters +14/-1

Provide a pre-execution guard for staged adapters

• Adds a callback that runs after staging but before adapter execution, plus a dedicated error for registries disabled during refresh. The callback allows callers to enforce their current registry state without adding database access to the installer.

src/shared/registries/installer.ts

Tests (4) +47 / -8

registries-routes.test.tsVerify disabled registries cannot execute fetched adapters +10/-6

Verify disabled registries cannot execute fetched adapters

• Changes the race test to disable a registry during its adapter fetch and expect a 409 response. It also checks that the adapter did not run and the registry remains disabled.

src/server/tests/registries-routes.test.ts

git-url.test.tsCover credential-free HTTP Git URLs +10/-0

Cover credential-free HTTP Git URLs

• Verifies that parsing removes HTTP credentials while preserving a version suffix. It also confirms that SSH usernames remain intact.

src/shared/tests/git-url.test.ts

master-key-store.test.tsCover a master key completed by another process +20/-2

Cover a master key completed by another process

• Adds a test in which a separate process completes a partial key while it is being read. The corrupt-key test now uses an oversized file to verify immediate invalidity.

src/shared/tests/master-key-store.test.ts

hook-handlers.test.tsCover anchored and grouped hook matcher scope +7/-0

Cover anchored and grouped hook matcher scope

• Tests that anchored or simply wrapped matchers remain scoped to their tool family. A matcher with internal groups still takes the existing union fallback.

src/shared/providers/tests/hook-handlers.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. Re-enabled registries fail to load ✓ Resolved

Description

stageRegistry replaces the installed adapter before beforeExecute checks whether the registry
was disabled, and the disabled-error path leaves that replacement on disk without updating its
stored hash. When a user re-enables the registry after disabling it during a refresh, the loader
compares the new file with the old hash and rejects it.

Code

src/server/registries-routes.ts[R316-317]

+				beforeExecute: () => {
+					if (!db.getRegistry(slug)?.enabled) throw new RegistryDisabledError(slug);

Evidence

Staging removes the existing directory and writes new bytes before the newly added guard runs. The
guard's error bypasses the installed-record update, while re-enabling changes only the enabled flag;
the loader then checks the staged bytes against the unchanged stored hash.

src/shared/registries/installer.ts[133-183]
src/server/registries-routes.ts[66-98]
src/server/registries-routes.ts[313-323]
src/db/registries.ts[127-145]
src/shared/registries/loader.ts[58-64]
src/shared/registries/loader.ts[89-121]

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 refresh disabled after staging leaves new adapter bytes on disk but keeps the old installed hash, so re-enabling the registry fails to load it.
## Fix Focus Areas
- src/shared/registries/installer.ts[133-183]
- src/server/registries-routes.ts[313-323]
- src/cli/commands/registry.ts[429-443]
## Recommended Fix
Stage refreshes separately from the installed adapter and preserve or restore the previous installation when the enabled check aborts. Add a disable-then-re-enable test that verifies the adapter still loads.

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

---

2. Write-only hooks also fire on edits ✓ Resolved

Description

matcherWithinPrefix tries to remove a wrapping group before removing anchors, so ^(Write)$
remains (Write) and is treated as outside the canonical family. For that matcher, resolveMatcher
unions it with Edit|MultiEdit|Write, causing a hook scoped to writes to run for edits as well.

Code

src/shared/providers/hook-handlers.ts[R130-134]

+		const inner = /^\((?:\?:)?([^()]*)\)$/.exec(s);
+		return (inner ? inner[1]! : s).replace(/^\^/, "").replace(/\$$/, "");
+	};
+	const parts = unwrap(userMatcher).split("|").map(unwrap);
+	if (parts.some((p) => /[()]/.test(p))) return false;

Evidence

The new unwrap expression cannot match ^(Write)$ until its anchors are removed, but it removes
them only afterward. The parentheses check therefore returns false, and the existing fallback builds
a family-wide union; the PR's new test explicitly expects the narrower matcher instead.

src/shared/providers/hook-handlers.ts[126-148]
src/shared/providers/tests/hook-handlers.test.ts[158-167]
src/cli/utils/hooks/install.ts[161-179]

Agent prompt

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

## Issue description
An anchored wrapping group is rejected as out of scope, widening a narrowly matched hook to its entire canonical tool family.
## Fix Focus Areas
- src/shared/providers/hook-handlers.ts[126-148]
- src/shared/providers/__tests__/hook-handlers.test.ts[158-167]
## Recommended Fix
Strip outer anchors before attempting to unwrap one enclosing group, then check the resulting alternatives against the family. Ensure the new `^(Write)$` test passes and cover an anchored grouped MCP matcher.

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

---

3. Malformed Git URLs can still save tokens ✓ Resolved

Description

parseGitUrlSource calls stripHttpUrlUserinfo, which returns its input unchanged when new URL()
rejects a URL accepted by the Git-source regex. A value such as
https://user:token@host:bad-port/repo.git therefore reaches a skill definition and is appended to
capabilities.yaml with the token intact.

Code

src/shared/git-url.ts[38]

+		url: stripHttpUrlUserinfo(url),

Evidence

The parser's regex accepts content that new URL() rejects. The stripping helper then falls back to
the original string, and the skill-add path persists the parsed definition before installation.

src/shared/git-url.ts[14-42]
src/shared/git-url.ts[65-83]
src/cli/commands/add-parse-skill.ts[159-174]
src/cli/commands/add.ts[262-285]

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 credential-stripping call leaves malformed, regex-accepted HTTP Git URLs unchanged, allowing credentials into the project file.
## Fix Focus Areas
- src/shared/git-url.ts[14-42]
- src/shared/git-url.ts[65-83]
- src/shared/__tests__/git-url.test.ts[7-17]
## Recommended Fix
Validate HTTP(S) clone URLs before returning them from `parseGitUrlSource`, and fail closed when userinfo cannot be safely removed. Test a malformed URL containing credentials through the skill-add persistence path.

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

---

4. A short key stalls every health check ✓ Resolved

Description

readFileKey uses synchronous Atomics.wait polling for up to one second whenever an existing
master.key is shorter than 32 bytes. If that file remains short and cannot be repaired, each
/health request repeats the wait on the server event loop because failed key loads are not cached.

Code

src/shared/master-key-store.ts[R196-198]

+		if (buf.length === 32) return buf;
+		if (buf.length > 32 || Date.now() >= deadline) return "invalid";
+		Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, PARTIAL_KEY_POLL_MS);

Evidence

The new retry synchronously waits until its deadline for a short file. An unsuccessful repair leaves
the key uncached, and /health calls getSecretStorageTier, which loads the key on each request.

src/shared/master-key-store.ts[176-199]
src/shared/master-key-store.ts[263-278]
src/shared/master-key-store.ts[315-323]
src/shared/master-key-store.ts[31-33]
src/server/index.ts[425-435]

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 persistently short master key makes every health check synchronously poll for a second before failing.
## Fix Focus Areas
- src/shared/master-key-store.ts[176-199]
- src/shared/master-key-store.ts[315-323]
- src/server/index.ts[425-435]
## Recommended Fix
Keep the bounded retry for a concurrent writer without blocking the server event loop on every failed load. Cache or otherwise rate-limit retries for an unchanged invalid file, while allowing a subsequently completed write to be recognized.

ⓘ 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 copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗