[Critical] readUserKeyterms only lstats the final path component and never canonicalizes or contains the resolved path, so it can read an arbitrary regular file and ship its contents to the remote ASR provider (corpus_text / system message). The leaf-only symlink check is bypassable. I confirmed each of these firsthand against this code (each returns the target's contents, capped to ~2000 chars):

• Parent-directory symlink — default path, no setting needed. Commit .qwen itself as a symlink to a dir containing a real voice-keyterms.txt; lstatSync('<ws>/.qwen/voice-keyterms.txt') follows the symlinked parent and sees a regular file, so the guard passes. This is exactly the "no secret exfiltration" scenario the symlink test claims to cover — but that test only makes the leaf a symlink (voice-keyterms.test.ts:162).
• Configured absolute path outside the workspace. keytermsFile: "/etc/passwd" → 30 passwd lines exfiltrated; newline-delimited secrets (~/.ssh/id_rsa, .env, ~/.aws/credentials) leak nearly in full.
• Configured ../ traversal. keytermsFile: "../../secret.txt" escapes the workspace root (the relative branch path.resolve(path.dirname(qwenDir), configured) does no containment).

Precondition: a trusted workspace — and for the configured-path cases, a workspace-scoped .qwen/settings.json, which merges into settings.merged and overrides the user's value. So a cloned repo a user later trusts can plant the parent-symlink (vector 1, no setting) or a keytermsFile setting (vectors 2–3). It's gated by trust, but it defeats the anti-exfiltration defense this PR explicitly adds (the symlink rejection + the trusted-mode "no secret exfiltration" test), so by the PR's own threat model it's in scope.

Suggested fix — don't trust a leaf stat; canonicalize and constrain by origin:

1. Honor the explicit keytermsFile setting only from user/system scope, not workspace (read it from settings.user/system rather than settings.merged). This is the cleanest fix: it stops a repo from weaponizing the setting while preserving the intended use of a user-chosen absolute path.
2. For auto-discovery and relative paths, canonicalize and contain — realpathSync the resolved path (resolves intermediate and leaf symlinks plus ../) and require it to stay within realpathSync(workspaceRoot):

const real = fs.realpathSync(filePath);
const root = fs.realpathSync(workspaceRoot); // path.dirname(qwenDir)
if (real !== root && !real.startsWith(root + path.sep)) return [];
const stat = fs.statSync(real, { throwIfNoEntry: false });
if (!stat || !stat.isFile() || stat.nlink > 1 || stat.size > MAX_KEYTERMS_FILE_BYTES) return [];

3. realpathSync alone does not catch a hardlink at the leaf (no separate real path) — reject nlink > 1 too, as above.
4. Don't rely on isTrusted as the sole boundary: in the daemon, trust is derived from process.cwd() (not the bound --workspace) and defaults to true on an unknown path, so it doesn't reliably reflect the workspace whose file is read.
5. Add regression tests for the three escape cases above asserting globals-only (they fail against the current code, then pin the fix).

— claude-opus-4-8[1m] via Qwen Code /qreview

[Critical] resolveKeytermsFile accepts absolute paths as-is and resolves relative paths against the workspace root with no containment check. A trusted workspace's .qwen/settings.json could set keytermsFile to /home/user/.ssh/id_rsa or ../../.aws/credentials, and those file contents (up to 64 KB) would be read and sent to the remote ASR provider. The isTrusted gate is the only defense, but once a workspace is trusted, there is no second check on what path is being read.

The codebase already has isSubpath from @qwen-code/qwen-code-core used in 50+ locations for exactly this kind of confinement (sandbox, stats, export, etc.). Consider adding a containment check:

— qwen3.7-max via Qwen Code /review

[Suggestion] There is a TOCTOU race between fs.lstatSync (symlink/regular-file check) and fs.readFileSync (the actual read). Between the two syscalls, a concurrent process could replace the regular file with a symlink to a sensitive target.

The codebase already has an O_NOFOLLOW pattern in customBanner.ts that makes the open+stat+read atomic. Consider adopting it here:

— qwen3.7-max via Qwen Code /review

[Suggestion] Every failure path in readUserKeyterms silently returns [] — untrusted workspace, no workspace path, symlink rejected, oversized file, I/O error. Users whose custom keyterms are silently ignored have no way to diagnose why.

Every sibling file in voice/ (voice-transcriber.ts, voice-stream-session.ts, qwen-asr-realtime-session.ts, etc.) uses createDebugLogger from @qwen-code/qwen-code-core. Consider importing it and logging at each early-return branch so the next person debugging "my keyterms file isn't being picked up" has breadcrumbs.

— qwen3.7-max via Qwen Code /review

[Suggestion] The PR modifies buildKeytermsContext in both the realtime path (resolveVoiceStreamConfig) and the batch path (transcribeVoiceAudio), but this test only verifies custom-term threading for the realtime path. No test verifies a custom term reaches the batch transcription request body. Consider adding a complementary batch-path test to ensure both call sites are covered.

— qwen3.7-max via Qwen Code /review

[Suggestion] This symlink-rejection test only covers the auto-discovery path (.qwen/voice-keyterms.txt). The explicit keytermsFile path goes through resolveKeytermsFile first, then hits the same lstatSync check — but there's no test verifying the guard applies to explicitly configured paths too. A future refactor that separates the two resolution paths could accidentally drop the symlink guard from the explicit path. Consider adding a test that sets keytermsFile to a symlink and asserts the content is not loaded.

— qwen3.7-max via Qwen Code /review

[Suggestion] readKeytermsFileSetting deliberately skips workspace scope (only reads system + user), which is a security boundary: workspace settings originate from the repo and could plant absolute paths that bypass the containment check. But nothing at this call site explains why workspace is excluded.

A future maintainer might see this as an oversight and add settings.workspace?.settings to the chain, unknowingly opening a keyterms-file exfiltration vector via a cloned repo's .qwen/settings.json.

— qwen3.7-max via Qwen Code /review

[Suggestion] The nlink > 1 hard-link guard is checked in both canonicalizeKeytermsFile (here) and readRegularFileNoFollow, but the test suite only covers the symlink equivalent (3 tests). There's no test that creates a hard-linked file and asserts rejection.

Hard links can be created trivially on macOS/Linux with fs.linkSync(source, dest). Adding a test would be straightforward:

it('does not read a hard-linked keyterms file', () => {
  const secret = path.join(workspaceDir, 'secret.txt');
  const link = path.join(qwenDir, 'voice-keyterms.txt');
  fs.writeFileSync(secret, 'HardLinkSecret\n');
  fs.linkSync(secret, link);
  const terms = buildVoiceKeyterms(makeSettings(workspaceDir));
  expect(terms).not.toContain('HardLinkSecret');
  expect(terms).toContain('TypeScript');
});

— qwen3.7-max via Qwen Code /review

[Suggestion] The as cast here asserts the exact type that optional chaining already produces from the parameter annotation. Since settings is typed as { general?: { voice?: { keytermsFile?: unknown } } } | undefined, the expression settings?.general already produces { voice?: { keytermsFile?: unknown } } | undefined — identical to the cast target.

The result is unknown either way, and the next line (typeof value !== 'string') narrows it.

— qwen3.7-max via Qwen Code /review

[Suggestion] This test doesn't actually exercise the char budget (MAX_KEYTERMS_CHARS). All 40 terms are 'x'.repeat(80) — identical strings — so dedupeKeyterms collapses them to a single unique term. The result is 31 globals + 1 user term = 32 terms at ~300 chars, well under both the 200-term count cap and the 2000-char budget. All three assertions pass for the wrong reason. If the char cap were accidentally removed from capKeyterms, this test would still pass.

— qwen3.7-max via Qwen Code /review

[Suggestion] makeSettings always constructs system: { settings: {} } and the keytermsFile parameter is only placed in user.settings.general.voice. No test verifies that a system-scoped keytermsFile is read, or exercises the system-over-user precedence of readKeytermsFileSetting. If the scope chain were accidentally reordered, no test would catch it.

Consider adding two tests: (1) a system-scoped keytermsFile is honored when no user setting exists, and (2) when both system and user scopes set keytermsFile, verify which wins.

— qwen3.7-max via Qwen Code /review

[Suggestion] Tilde (~) is not expanded in the keytermsFile setting value. path.isAbsolute('~/terms.txt') returns false on POSIX, so it falls through to path.resolve(workspaceRoot, configured) which resolves to <workspaceRoot>/~/terms.txt — a path that doesn't exist. The file silently fails to load and the user gets globals-only keyterms with no diagnostic.

Tilde expansion is a common user expectation for file-path settings. The codebase already has resolvePath in @qwen-code/qwen-code-core that handles tilde expansion and is used elsewhere for user-facing path resolution.

— qwen3.7-max via Qwen Code /review

[Critical] On Windows, O_NOFOLLOW is undefined (typeof fs.constants.O_NOFOLLOW is not 'number'), so this falls back to plain O_RDONLY — the fd follows symlinks. The prior lstatSync in canonicalizeKeytermsFile catches symlinks, but there's a TOCTOU window between lstatSync and openSync that O_NOFOLLOW closes on POSIX and leaves open on Windows.

The symlink tests all run on macOS/Linux where O_NOFOLLOW works, so the Windows code path is unverified.

Consider adding a post-fstatSync check: re-run fs.fstatSync(fd) and compare ino/dev against the earlier lstatSync result, or fall back to [] on Windows entirely.

— qwen3.7-max via Qwen Code /review

[Critical] System scope has higher priority than user scope here (system ?? user). Combined with mustBeInWorkspace: false for absolute paths (line 175), a system-scoped keytermsFile can point to any absolute path on disk — the contents are silently uploaded to the remote ASR provider for every user on the machine.

On a shared workstation (CI server, corporate machine), anyone who can write to the system settings file can force exfiltration of arbitrary readable files via dictation.

Consider inverting precedence so user scope overrides system scope:

— qwen3.7-max via Qwen Code /review

[Suggestion] Inline # comments are not stripped. A line like Kubernetes # container orchestration becomes a single malformed keyterm instead of extracting just Kubernetes. The schema description says "#" for comments, which users will interpret as supporting inline comments (consistent with shell/config conventions).

Alternatively, update the schema description to clarify that only full-line comments are supported.

— qwen3.7-max via Qwen Code /review

[Suggestion] The function name canonicalizeKeytermsFile suggests path normalization (realpath), but it performs five security validations: symlink rejection, file-type check, hard-link count, file-size cap, and workspace containment. A future maintainer may not recognize this as the security gate.

Consider renaming to validateKeytermsFile or validateAndCanonicalizeKeytermsFile.

— qwen3.7-max via Qwen Code /review

[Suggestion] The workspace root is derived via path.dirname(path.dirname(workspacePath)), assuming workspace.path always ends in .qwen/settings.json. If the settings file location ever changes, the containment check silently uses a wrong root — either incorrectly rejecting valid files or allowing files outside the actual workspace.

Consider validating the assumption (path.basename(qwenDir) === '.qwen') or accepting the workspace root as an explicit parameter on LoadedSettings.

— qwen3.7-max via Qwen Code /review

[Suggestion] Each call to buildVoiceKeyterms re-reads the file from disk, re-parses, re-dedupes, and re-caps. In the daemon serve path where multiple dictation sessions may share a workspace, this is redundant I/O.

Consider caching the result keyed on file path + mtime from the lstatSync already performed.

— qwen3.7-max via Qwen Code /review

[Critical] This newly added race-condition test fails against the current implementation, which means the TOCTOU mitigation it describes is not actually in place.

Reproduced locally with npx vitest run src/ui/voice/voice-keyterms-race.test.ts:

FAIL  buildVoiceKeyterms race checks > does not read a keyterms file swapped in before open
AssertionError: expected [ 'Qwen', 'MCP', 'grep', …(29) ] to not include 'SwapSecret'
 ❯ src/ui/voice/voice-keyterms-race.test.ts:85:23

The mock atomically swaps .qwen/voice-keyterms.txt from SafeTerm\n to SwapSecret\n (new inode via rmSync + writeFileSync) between lstatSync and openSync in the read path. The post-open fstatSync(fd) dev/ino check in readRegularFileNoFollow was supposed to catch this, but SwapSecret still lands in the returned keyterms list. Concretely, that means the current canonicalizeKeytermsFile → readRegularFileNoFollow handoff has a live TOCTOU window: an attacker (or just a concurrent build task / misbehaving tool) with write access to the workspace can substitute sensitive content between the two syscalls and have it shipped to the remote ASR provider as corpus_text / system-message bias.

The existing Critical comments on voice-keyterms.ts flag the TOCTOU concern conceptually; this comment adds the empirical proof via the PR's own test suite. CI will stay red on Test (ubuntu-latest, Node 22.x) until this is resolved.

Suggested fix: open the file once with O_NOFOLLOW | O_RDONLY before realpathSync, pass the resulting fd through to the reader, and verify dev/ino via fstat on the same fd that is then used for readFileSync. That collapses the lstat → open window to a single syscall. Sketch:

const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW);
try {
  const stat = fs.fstatSync(fd);
  if (stat.isSymbolicLink() || !stat.isFile() || stat.nlink > 1 ||
      stat.size > MAX_KEYTERMS_FILE_BYTES) return undefined;
  // workspace containment via realpath of the fd path (readlink /proc/self/fd/N)
  return fs.readFileSync(fd, 'utf-8');
} finally {
  fs.closeSync(fd);
}

— qwen3.7-max via Qwen Code /review

[Suggestion] capKeyterms uses break when a single term exceeds the remaining char budget, which drops all subsequent terms — including shorter ones that would fit. For example, if the remaining budget is 50 chars and the next term is 80 chars followed by ten 4-char terms, all eleven terms are dropped even though the ten short terms would fit.

The term-count cap (out.length >= MAX_KEYTERMS) correctly uses break (once you hit 200 terms, no more can fit regardless of length), but the char-budget cap should skip the oversized term and try the next.

— qwen3.7-max via Qwen Code /review

[Suggestion] The regex /#.*$/ strips everything from the first # onward, silently truncating terms that legitimately contain #. For a developer-focused feature, common language names like C#, F#, or identifiers like ASP.NET#Core become C, F, ASP.NET respectively. The existing inline comment at line 251 has the analysis backwards — it says inline comments are not stripped, but they are over-stripped.

Consider only treating # as a comment marker when preceded by whitespace or at the start of the line:

Alternatively, document that # within a term is not supported and recommend C Sharp instead.

— qwen3.7-max via Qwen Code /review

[Suggestion] The outer try/catch here returns undefined on error, stripping all keyterms — including the hardcoded globals like "Qwen", "TypeScript", "npm". But buildVoiceKeyterms already has its own internal catch-all that guarantees it never throws (it returns [...GLOBAL_KEYTERMS] on any error). The outer catch can only fire if keyterms.join(' ') throws, which is impossible for a string array.

If a future refactor introduces an unexpected throw inside buildVoiceKeyterms, this wrapper silently degrades to zero keyterms instead of the intended graceful degradation to globals. Either remove the redundant try/catch (since buildVoiceKeyterms already guarantees it never throws), or change the catch to return GLOBAL_KEYTERMS.join(' ') as a belt-and-suspenders fallback.

— qwen3.7-max via Qwen Code /review

[Suggestion] capKeyterms measures the budget in string.length (UTF-16 code units), but the joined string is serialized to UTF-8 for the HTTP request. For non-ASCII terms the byte cost exceeds the code-unit cost: CJK characters consume 3 UTF-8 bytes per code unit, supplementary-plane characters consume 4 bytes per 2 code units. A file of ~2000 CJK characters passes the cap yet produces ~6000 UTF-8 bytes on the wire.

Since the PR description explicitly mentions non-English terms as a use case, consider measuring in UTF-8 bytes:

And in capKeyterms, use Buffer.byteLength(term, 'utf-8') instead of term.length.

— qwen3.7-max via Qwen Code /review

[Suggestion] The fd re-stat guard compares six identity fields, but the mtimeMs/ctimeMs branches are effectively dead-untested. I removed both comparisons and all 56 voice tests still pass. The race test (voice-keyterms-race.test.ts) swaps via rmSync+writeFileSync with a different-length payload (SwapSecret\n 11B vs SafeTerm\n 9B), so the swap is already caught by the ino+size checks — the timestamp branches, which are the only thing that catches a same-inode, same-length in-place rewrite, are never exercised. Consider a race variant whose openSync mock rewrites the file in place with identical byte length but different content (e.g. SafeTerm\n → EvilTrmX\n, both 9 bytes), so mtimeMs/ctimeMs is the sole catcher and a regression dropping it turns the suite red.

— claude-opus-4-8 via Claude Code /qreview

[Suggestion] This mustBeInWorkspace asymmetry is security-load-bearing: a user-scoped absolute path is intentionally allowed to resolve outside the workspace, while a system-scoped one is forced inside. Only the rejection side is pinned by a test (voice-keyterms.test.ts:170). The "honors an explicit absolute keytermsFile" test (:106) writes its file inside workspaceDir, so it passes identically whether mustBeInWorkspace is true or false and doesn't lock in the intended behavior. A future change to mustBeInWorkspace: !isAbsolute (also containing user absolutes) would silently break the documented "absolute, or relative to the workspace root" contract with zero test failure. Suggest adding the mirror of the :170 test — a user-scoped absolute path to a file outside workspaceDir, asserting its term is included.

— claude-opus-4-8 via Claude Code /qreview