@qwen-code /triage

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual body complete.

On direction: clean security hardening that closes the last unvalidated source-slug-to-path construction site in the session MCP credential cache. Natural follow-up to #5829 and #5911. The three gaps — credential cache path construction without slug guard, validation diagnostics echoing unsafe paths, and RPC errors misclassified as I/O failures — are all legitimate defensive edges. Clearly aligned with the project's security posture.

On approach: scope is tight and each change addresses a named gap. Extracting credential cache code from index.ts into credential-cache.ts is justified (needs modification + independently testable). Reusing assertValidSourceSlug from @craft-agent/session-tools-core (already re-exported there) is correct. The validation file helpers (getSourceValidationFile, getSourcePermissionsValidationFile) are simple and focused. Removing the unnecessary storage mock fixes test isolation without changing behavior. No scope creep, no drive-by refactors. Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal (from PR description, before reading diff): add assertValidSourceSlug to credential cache path construction, sanitize validation result file fields with placeholders, separate RPC error logging by error type, add debug logging to raw permissions loader, remove test mock leaking.

Comparison with diff: the PR's approach matches or exceeds the independent proposal on every point.

credential-cache.ts — Clean extraction from index.ts. assertValidSourceSlug(sourceSlug) before join() is correct. The try-catch in readCredentialCache wraps the full function body (including path construction), so invalid slugs return null through the credential manager — preserving the existing missing-credential behavior contract. Reuses assertValidSourceSlug from @craft-agent/session-tools-core where it's already re-exported.

sources.ts (RPC handler) — isInvalidSourceSlugError() uses error message string matching, which is slightly fragile but pragmatic. The error message is controlled by assertValidSourceSlug in the same monorepo, and this is a common Node.js pattern. Correctly downgrades invalid slug errors from error to warn severity.

validators.ts — getSourceValidationFile() and getSourcePermissionsValidationFile() use a clean try-catch pattern to return sanitized paths on invalid slugs. The file variable hoisting in validateSourcePermissions (before the try block) is needed so the catch block has access to the sanitized path.

permissions-config.ts — Single-line addition of debug() logging in the catch block, matching the nearby normalized loader behavior. Minimal and correct.

credential-manager-renew.test.ts — Removing the unused mock.module('../storage.ts') fixes cross-test contamination. The mock was unnecessary since the credential module is already mocked separately.

No blockers found. All changes are focused, correct, and follow project conventions.

Testing

This PR is non-UI security hardening of internal desktop path validation (credential cache reads, validation diagnostics, RPC error classification in the Electron main process). There's no CLI invocation path that exercises this behavior — tmux real-scenario testing is not applicable here. The behavior is best validated through the unit tests included in the PR (8 + 14 tests passing per the author's report).

Unable to run the desktop Bun test suite in this CI environment (Bun not installed).

— Qwen Code · qwen3.7-max

Reflection

Going back to the independent proposal: I would have made the same changes — add slug validation before path construction, sanitize diagnostic paths, separate error logging, add debug logging, fix test isolation. The PR matches on every point and the module extraction is a reasonable improvement for testability.

The code is straightforward. assertValidSourceSlug before join() in credential-cache.ts is exactly the right fix for the credential cache gap. The validation file helpers are three lines each — no over-engineering. The RPC handler's error classification is pragmatic. The debug logging addition is a one-liner for parity. Test mock removal is a clean isolation fix.

Every change in the diff serves the stated goal. No drive-by refactors, no scope creep, no formatting churn. 9 files changed, +203/-56 — proportional to the work.

This is a focused, correct security hardening PR that closes the last unvalidated source-slug-to-path construction site. The included tests cover the key scenarios (traversal rejection, valid slug acceptance, null returns for missing/expired caches, separated RPC diagnostics, test isolation). The approach is the right one, and the execution is clean.

LGTM, looks ready to ship. ✅

— Qwen Code · qwen3.7-max

✅ Maintainer local verification — PASS (recommend merge)

I built and ran this PR locally on a clean worktree at the PR head (712b435), including adversarial mutation tests to confirm the new tests are load-bearing (i.e. they fail when the fix is reverted). All checks reproduce the PR's claims, and I additionally verified it on macOS, which the PR description marked as not tested.

Environment: macOS (Darwin 25.5.0), Bun 1.3.14, Node v22.22.2 · isolated worktree, ELECTRON_SKIP_BINARY_DOWNLOAD=1 bun install (1560 packages, clean).

1. Focused tests — reproduce the PR's results

2. Adversarial mutation testing (the part that matters)

A passing test proves nothing unless it fails with the fix removed — so I reverted each fix and confirmed the regression is real:

(A) The path-traversal guard is load-bearing. Removing assertValidSourceSlug() from getCredentialCachePath() makes credential-cache.test.ts fail, and readCredentialCache(workspaceRoot, '../sessions') returns the planted "secret-token" instead of null — a real credential disclosure out of the sources/ directory:

Received value: ".../credential-cache-AGbhsP/sessions/.credential-cache.json"  // traversed out of sources/
expect(readCredentialCache(...)).toBeNull()  →  Received: "secret-token"

With the PR's guard in place, both cases return null. ✅

(B) The test-isolation fix is real. Re-adding the removed mock.module('../storage.ts') to credential-manager-renew.test.ts makes the next file in the same Bun process fail — loadWorkspaceSources() returns [] instead of ['valid-source'] because the leaked loadSourceConfig mock survives across files:

(fail) storage-source-slug … legacy invalid source directories
- ["valid-source"]
+ []

Removing the unused mock (as this PR does) makes the pair order-independent again. ✅

3. Typecheck & build

I confirmed every one of the 77 tsc errors lives under ../session-tools-core/src/** (the .ts-import-extension config issue that predates this PR). None touch the changed session-mcp files — the new imports use extensionless / package form, matching the PR description.

4. Reviewer-side audit (no blockers)

• Caller safety: getCredentialCachePath (now throwing) is consumed only by readCredentialCache, which wraps the call inside its try → throw is caught → returns null. Its two real consumers (index.ts:90,95, the credential manager's getToken/hasValidCredentials) already treat null as "no credential", so the graceful contract holds end-to-end. No unguarded caller exists.
• Minor / non-blocking: validateSource / validateSourcePermissions now call assertValidSourceSlug twice (once in the new getSource*ValidationFile helper, once on the existing path). It's an idempotent regex check — purely cosmetic redundancy. isInvalidSourceSlugError matches on the 'Invalid source slug:' message prefix; it's string-coupled to the thrown error but pinned by sources.test.ts.

5. CI / merge note

Test (ubuntu-latest, Node 22.x) is green; mac/win/integration legs show skipping (named-job gating — normal). mergeStateStatus: BLOCKED here is REVIEW_REQUIRED (awaiting a maintainer approval), not a CI failure. Note that packages/desktop/ is a separate Bun workspace excluded from the root CI, so these desktop changes are not exercised by the root pipeline — the local run above is the actual evidence.

Verdict

Behavior matches every claim in the PR description; both fixes are proven load-bearing by mutation; types/build are clean for the changed packages; the only tsc noise is pre-existing and unrelated. Looks good to merge. 👍

Received value: ".../credential-cache-AGbhsP/sessions/.credential-cache.json"  // 穿越出了 sources/
expect(readCredentialCache(...)).toBeNull()  →  Received: "secret-token"

(fail) storage-source-slug … legacy invalid source directories
- ["valid-source"]
+ []