feat(knowledge): accept portable library_dir value forms (0.9.0)

[kyle-sexton]

Resolves the library_dir portability collision: the seam only took literal paths, so pointing it at a corpus root outside the project and home directories (the intended knowledge-corpus consolidation flow) forced a machine-specific literal into user-level settings — exactly the value class the guardrails hardcoded-path-check blocks. The two plugins were in direct conflict for the primary supported use case.

Closes #798

Decision (the issue's open scheme fork)

Env-var indirection + tilde expansion (options 1 + 2 combined). Machine variance lives in OS user env vars or under home; the stored configuration value stays portable. ghq-derived scheme (option 3) rejected as a seam mechanism: it would couple the seam to ghq presence — the same undeclared-hard-dep defect class #531 flags on #438. A ghq-convention user gets the identical outcome by pointing the env var at the ghq-derived path.

What ships

• expandPathValue (new pure helper, extraction/lib/run-args.js): expands a leading ~ (home-relative, incl. ~\ on Windows) and ${NAME} / %NAME% env-var references anywhere in the value. Fail-loud contract: an unset/empty referenced variable throws, and a value that used either form must expand to an absolute path; literal values pass through byte-identical (back-compat). ~user forms stay literal.
• Launcher wiring (run.mjs): applies the expansion to --work-root before building the child env — single expansion point; YOUTUBE_WORK_ROOT stays an internal launcher-to-child interface. Errors route through the existing exit-2 + usage path.
• Contract docs: youtube-digest SKILL.md artifact-landing section documents the portable forms — passed verbatim in single quotes (literal in bash AND PowerShell, so the launcher expands and fails loud where shell-level expansion would silently substitute empty) — and requires agent-written artifact paths to resolve the root once (printenv NAME / $env:NAME). README option table, plugin manifest option description, and setup mismatch guidance updated to match.
• knowledge 0.9.0 + CHANGELOG entry per changelog-parity.

Verification

• Unit tests: 18/18 in run-args.test.js (tilde variants, both env-ref syntaxes, embedded refs, Windows-backslash substitution integrity, unset-var throw, non-absolute throw, literal passthrough). Full extraction suite: 256 passed; the 3 failing suites are the pre-existing imghash vendor module-resolution gap, identical on the base commit (249 passed there, delta = the 7 new cases). tsc --noEmit clean.
• Acceptance sketch verified empirically against plugins/guardrails/lib/path-detection: ${KNOWLEDGE_CORPUS_DIR}, %KNOWLEDGE_CORPUS_DIR%, and ~/knowledge-corpus all scan CLEAN; the literal D:/repos/... machine path is FLAGGED.
• Local gates green: changelog-parity --check + --check-bump, generate-catalog --check, validate-plugin-contracts, check-changed-skills (0 failed), skill-portability lint, markdownlint.
• Fresh-context acceptance verifier: 7/7 criteria PASS (back-compat, test coverage, fail-loud path, guardrail acceptance, doc coherence, version/CHANGELOG parity, commit scope).

Related

• ci: portability lint lane — enforce declared agnosticism mechanically (19-issue coupling class + top review-churn source) #531 — portability umbrella that carried this member; the scheme decision above is recorded there.
• feat(claude-config): add audit-instructions skill, superseding audit-model-fit (0.9.0) #870 — sibling claude-config effort referencing this as the separate library_dir indirection item.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 3m 15s —— View job

---

PR Review: feat(knowledge): accept portable library_dir value forms (0.9.0)

• Gather context and read changed files
• Review run-args.js — new expandPathValue helper
• Review run-args.test.js — test coverage
• Review run.mjs — launcher wiring
• Review docs: SKILL.md, README.md, setup/SKILL.md
• Review plugin.json and CHANGELOG.md
• Synthesize findings and post full review

---

Summary

This is a well-designed, well-scoped change. The core logic is correct, the fail-loud contract is properly implemented, and test coverage is solid. The documentation — SKILL.md, README, plugin manifest description, and setup guidance — is thorough and internally consistent. A few observations below, roughly ordered by impact.

---

Findings

1. ~\ on a POSIX host silently produces a malformed path

run-args.js line 124

if (expanded === "~" || expanded.startsWith("~/") || expanded.startsWith("~\\")) {

If a POSIX user accidentally sets library_dir to ~\corpus (a ~\ form with a Windows backslash), the check matches, expanded becomes /home/dev\corpus (backslash embedded in a POSIX path component), and isAbsolutePath passes it through (starts with /). The downstream process would then try to write to a directory named literally dev\corpus inside /home/.

This is unlikely in practice — the docs call ~\ out as the Windows form — but a guard like if (process.platform !== 'win32' && expanded.startsWith("~\\")) before setting sawForm could skip the expansion on POSIX and let the literal ~\corpus fail the consumer's own path checks. Or alternatively, just accept the current behavior and add a note to the docs that ~\ is Windows-only. Either is defensible; worth a deliberate call.

2. No test for tilde + embedded env-var combination

run-args.test.js line 113

The implementation handles ~/path-${SUBDIR} correctly by design — tilde expands first, then ENV_REF.replace handles the embedded reference. But there's no test exercising both forms in a single value. Given the code path (sequential: tilde → regex replace), a test covering ~/path/${SUBDIR} would pin the order and protect against future reordering. Low risk to add.

3. Embedded %NAME% form not tested in a path prefix

run-args.test.js line 132

The embedded-path test at line 136–141 uses ${NAME} only. The %NAME% form is tested standalone but not embedded (e.g. C:\Users\%USERNAME%\knowledge). The regex handles it correctly (same replacer path), but a test covering that form embedded would close the gap.

4. Module-scope /g regex is a future footgun

run-args.js line 101

const ENV_REF = /\$\{([A-Za-z_][A-Za-z0-9_]*)\}|%([A-Za-z_][A-Za-z0-9_]*)%/g;

Using a /g regex with String.prototype.replace is correct and safe — replace resets lastIndex before each call. But as a module-scope named constant, it could silently misbehave if anyone later uses ENV_REF.exec() or ENV_REF.test() in a loop (both maintain lastIndex state). Not a current bug, but the constant's name suggests potential reuse. Consider either keeping it function-local or replacing const ENV_REF = /.../g with a factory const ENV_REF = () => /.../g to make the statefulness explicit. Very low priority.

5. Scope gap: book-distill doesn't benefit from portable forms yet

README and CHANGELOG correctly document that the expansion is "the youtube-digest launcher today." But a user who reads the README's option table, sets library_dir to ${KNOWLEDGE_CORPUS_DIR}, and then runs /knowledge:book-distill will find their variable unexpanded — book-distill writes to the named target skill, not through library_dir, so this specific scenario is benign. Worth confirming no other pipeline (course-digest, book-distill's skill path resolution) reads library_dir as a raw value and passes it to a Node API that would receive the literal ${NAME} string unexpanded. A brief comment in expandPathValue's JSDoc noting which callers currently expand would make the scope clear to a future contributor.

---

What's working well

• Fail-loud contract is correctly implemented. Both !resolved (catches undefined and "") and the post-expansion !isAbsolutePath check together cover the failure modes: silent-empty substitution and relative-path misconfiguration.
• isAbsolutePath is dependency-free and covers POSIX, Windows drive, and UNC paths correctly.
• Single expansion point in run.mjs (parsed.workRoot = expandPathValue(...) before buildChildEnv) — clean, no risk of double-expansion.
• The single-quote guidance in SKILL.md is sharp. Instructing agents to pass '${NAME}' in single quotes (literal in both bash and PowerShell) so the launcher handles expansion is the right call and is clearly explained.
• Test suite is thorough for the primary contract: 7 meaningful cases for expandPathValue alone covering all documented forms, error conditions, and back-compat.
• CHANGELOG is accurate, semver bump to 0.9.0 is appropriate for a new capability.

---

Verdict

Approve with minor notes. Findings 2 and 3 are small test gaps worth closing; finding 1 is worth a deliberate design decision even if the answer is "document it as Windows-only and move on." None of these block the merge.

[claude]

Claude finished @kyle-sexton's task in 2m 46s —— View job

---

Security Review — PR #894

• Read PR diff and changed files
• Audit expandPathValue helper for injection / path-traversal risks
• Audit run.mjs launcher wiring
• Review test coverage surface for security-relevant edge cases
• Check docs / manifest for accidental secret or credential exposure
• Final report

---

Findings

SUGGESTION · Confidence: CONFIRMED — Expanded path is not normalized before the absolute-path check

run-args.js L139

expandPathValue checks that the result of env-var / tilde substitution starts with /, \\, or [A-Za-z]:[\\/], but does not normalize the path before applying that check. If the environment variable's stored value contains directory traversal sequences — e.g. /safe/root/../../etc — the value passes isAbsolutePath, flows into YOUTUBE_WORK_ROOT, and the child process writes artifacts under the traversal-resolved location.

This is user-controlled (the consumer sets both library_dir and their own environment variables), so there is no cross-user or privilege-escalation vector. The risk is self-inflicted: a misconfigured env var silently redirects artifact output to an unintended path rather than failing loudly. The fix is one line:

// after the sawForm block, before the isAbsolutePath guard:
if (sawForm) expanded = path.resolve(expanded);

…or defer normalization to the caller (run.mjs) immediately after expandPathValue returns, using path.resolve. The same normalization call eliminates .. sequences while preserving absolute-path intent. path.resolve requires importing node:path, which the module intentionally avoids for purity / unit-test isolation; if that constraint is load-bearing, a purely local alternative is to reject values whose expanded form contains /../ or starts with a platform-matching traversal prefix.

---

Security-positive observations (no findings)

• spawnSync without shell: true — arguments are passed as a JS array; there is no shell interpretation surface for the URL or path arguments the child receives.
• Script path containment — run.mjs L58–62 uses path.relative to reject scripts resolving outside the extraction directory before spawnSync is called; this is correct and tested.
• Env-var name regex — [A-Za-z_][A-Za-z0-9_]* restricts substitution to well-formed identifiers, preventing pattern injection via variable names.
• Fail-loud on unset variables — !resolved catches both undefined and "" (all env values are strings in Node.js); a silent empty substitution that would write artifacts to the wrong root is not possible.
• No secrets in manifest or docs — plugin.json and the updated SKILL.md content correctly treat library_dir as a non-secret path scalar; the cookie-file fields note "never commit cookie files" and carry no credential values.
• ENV_REF global regex — String.prototype.replace with a global regex resets lastIndex at the start of each call; no stale-state risk despite the module-level regex constant.

[claude]

Claude finished @kyle-sexton's task in 2m 41s —— View job

---

PR Review: feat(knowledge): accept portable library_dir value forms (0.9.0)

• Read the PR diff and changed files
• Review run-args.js — expandPathValue helper logic
• Review run-args.test.js — test coverage gaps
• Review run.mjs — launcher wiring and error handling
• Review docs/manifest: SKILL.md, README.md, plugin.json, CHANGELOG.md
• Synthesize and post final review

---

Summary

This review covers all three commits on the branch. The two new commits (fefab77, aef9f3a) landed since the earlier correctness and security passes are both sound. The PR is in good shape overall. Findings are ordered by impact.

---

New commits since prior reviews — assessment

fefab77 — scope README expansion claim to the youtube-digest launcher

Correct and tight. The prior review flagged that book-distill doesn't expand portable forms, and the README previously implied that all consuming skills do. The change replaces "consuming skills expand these when resolving the root" with "expanded when a pipeline resolves the root (the youtube-digest launcher today)." This is accurate and closes finding #5 from the earlier pass cleanly.

aef9f3a — use non-user-path fixtures in expandPathValue tests

The change from home = "/home/dev" to home = "/srv/dev-home" is correct — /home/<name> is the exact pattern the machine-specific-paths gate targets. One observation: the C:\\Users\\dev fixture on line 125 was not changed. The commit message says the gate flags both /home/<user> and C:\Users\<user> literals. If C:\\Users\\dev isn't currently caught, it may be because dev is a short generic token the pattern doesn't match — but this is worth a deliberate confirmation. If the gate ever widens, this fixture would start failing CI for the same reason the /home/dev one did. Fix this →

---

Remaining open findings (from prior passes — not yet addressed)

1. Security: expanded path not normalized — .. traversal passes the absolute-path guard

run-args.js L139

The post-expansion isAbsolutePath check passes a value like /safe/root/../../etc because it starts with /. The expanded string flows into YOUTUBE_WORK_ROOT unexpanded, so the child process would resolve artifacts under the traversal path. This is self-inflicted (the consumer controls both library_dir and their own env vars) — no privilege escalation vector — but a misconfigured env var can silently redirect output instead of failing loudly, which contradicts the fail-loud contract. Since isAbsolutePath is intentionally dependency-free to keep the module pure, the lightest fix is to reject values whose post-expansion form contains /../ (POSIX) or \.. (Windows) as a substring check, staying dependency-free. Fix this →

2. ~\ on a POSIX host silently produces a malformed path

run-args.js L124

If a POSIX user accidentally writes ~\corpus, the expansion produces /srv/dev-home\corpus (backslash embedded in a POSIX path), passes isAbsolutePath (starts with /), and the child process tries to resolve a directory component with a literal backslash. Worthy of a deliberate design call: either guard startsWith("~\\") behind process.platform === 'win32' and throw on POSIX, or document ~\ as Windows-only in the plugin manifest description (where portable forms are already enumerated). The SKILL.md currently describes it as the "Windows form" which is close but doesn't state it's invalid on POSIX.

3. Two small test gaps (low priority)

• No test for tilde + embedded env-var in the same value (e.g. ~/path/${SUBDIR}) — the sequential expansion order (tilde first, then regex replace) is correct but unpinned by a test.
• %NAME% embedded in a path prefix (e.g. C:\Users\%USERNAME%\knowledge) is not covered — only the standalone form is tested.

Neither is likely to regress given the implementation, but they'd protect against future reordering.

---

What's working well

• expandPathValue implementation is correct and clean — tilde expansion before regex replace, both ${NAME} and %NAME% forms handled by a single replacer, fail-loud on unset/empty variable, fail-loud on non-absolute expansion, literal values byte-identical passthrough.
• isAbsolutePath is dependency-free and correct — covers POSIX (/), UNC (\\), and Windows drive ([A-Za-z]:).
• Single expansion point in run.mjs — expandPathValue is called once before buildChildEnv, no double-expansion risk.
• SKILL.md single-quote guidance is sharp — instructing agents to pass '${NAME}' in single quotes (literal in both bash and PowerShell) so the launcher expands the reference is the right call and clearly explained.
• CLAUDE.md compliance — repo-agnostic (uses ${CLAUDE_PLUGIN_ROOT}, ${CLAUDE_PLUGIN_DATA}, ${CLAUDE_PROJECT_DIR}), configurable via userConfig, versioned at 0.9.0 with a correct CHANGELOG entry.

---

Verdict

Approve with notes. The core implementation is correct and the two new commits are sound. Finding #1 (path normalization / .. traversal) is the most actionable remaining item — a two-line fix that closes a gap in the fail-loud contract. Finding #2 (~\ POSIX) is worth a deliberate call even if the answer is "document Windows-only." The test gaps (finding #3) are low risk. None of these block merge, but #1 would strengthen the stated fail-loud guarantee.

[claude]

Claude finished @kyle-sexton's task in 2m 26s —— View job

---

Security Review — PR #894

• Read PR diff and identify changed files
• Audit expandPathValue helper — path traversal, injection surfaces
• Audit run.mjs launcher wiring — spawn safety, env construction
• Check docs and manifest for credential/secret exposure
• Final security report

---

Findings

SUGGESTION · Confidence: CONFIRMED — Expanded path accepted without .. normalization

run-args.js L139

isAbsolutePath checks the leading characters only; it does not reject or normalize directory-traversal sequences embedded in the expanded result. A consumer who sets library_dir to ${KNOWLEDGE_CORPUS_DIR} with KNOWLEDGE_CORPUS_DIR=/safe/root/../../etc receives a value that starts with / and clears isAbsolutePath, so artifact output is silently redirected to /etc (or wherever the traversal resolves). The consumer controls both the config value and the env var, so there is no cross-user privilege vector — but the fail-loud contract promises to catch bad expansions and this one slips through.

Minimal fix — normalize once after expansion before the absolute-path guard, in run.mjs where node:path is already imported:

if (parsed.workRoot) {
  const raw = expandPathValue(parsed.workRoot, process.env, os.homedir());
  parsed.workRoot = path.resolve(raw);   // eliminates .. sequences
}

Alternatively, add the path.resolve call inside expandPathValue behind the sawForm check, at the cost of importing node:path into an otherwise dependency-free module. The run.mjs approach is lower-friction.

Fix this →

---

Security-positive observations (no findings)

• spawnSync without shell: true — the work root lands in YOUTUBE_WORK_ROOT as a plain env-var value, not a shell argument; there is no shell interpretation surface for embedded metacharacters.
• Env-var name regex — [A-Za-z_][A-Za-z0-9_]* restricts substitution to well-formed identifiers; no path separator, shell metacharacter, or Unicode can appear in a matched name.
• Script containment check intact — path.relative(here, target) rejecting an upward or absolute result is unchanged and not weakened by this PR.
• Fail-loud on unset/empty — !resolved catches both undefined and "" in the env-var replacer; silent empty substitution producing a wrong root is not possible.
• ENV_REF global regex with replace — String.prototype.replace resets lastIndex before each call; no stale-state risk from the module-scope /g flag.
• No secrets in manifest or docs — plugin.json description and SKILL.md additions treat library_dir as a plain path scalar; cookie-file fields carry no credential values.
• os.homedir() is the only tilde source — the homedir parameter is supplied as os.homedir() at the single call site in run.mjs; it is not user-controlled.

---

Note on prior finding

The path-normalization gap above was also identified in the security review posted earlier in this PR thread. The latest commit (aef9f3a) adjusted test fixtures to use /srv/dev-home rather than a real user path (security-positive) but did not address the normalization issue. No new security findings were introduced by the three commits on this branch.

[claude]

Claude finished @kyle-sexton's task in 1m 42s —— View job

---

PR Review: feat(knowledge): accept portable library_dir value forms (0.9.0)

• Read changed files at HEAD (c770610)
• Assess new commit vs open findings from prior reviews
• Check remaining open findings (path normalization, ~\ POSIX, test gaps)
• Synthesize and post full review

---

New commit assessment — c770610

The single-line change replaces "C:\\Users\\dev" with "C:\\dev-home" in the Windows tilde-expansion fixture (run-args.test.js line 125).

This closes the concern flagged in the second review pass: C:\Users\<name> is the exact pattern the machine-specific-paths gate scans for, and if the gate's Windows username pattern ever widens, the old fixture would have started failing CI for the same reason the /home/dev POSIX fixture did in aef9f3a. The fix is correct and complete — the expected output ("C:\\dev-home\\corpus") is updated to match.

---

Remaining open findings

Two findings from prior passes are still unaddressed on this branch. Neither blocks merge, but they are worth a deliberate disposition.

1. Expanded path not normalized — .. traversal passes the absolute-path guard

run-args.js L139

isAbsolutePath inspects only the leading characters of the expanded string. A value like ${SAFE_ROOT} with SAFE_ROOT=/trusted/path/../../etc passes the guard (starts with /) and flows into YOUTUBE_WORK_ROOT unexpanded, silently redirecting artifact output. This contradicts the stated fail-loud contract. The consumer controls both the config value and their own env vars, so there is no privilege-escalation vector — but a misconfigured env var produces silent misdirection rather than a loud error.

The lightest fix stays dependency-free (no new import in run-args.js): add a traversal check immediately after the ENV_REF replace block and before isAbsolutePath:

if (sawForm && (expanded.includes("/../") || expanded.includes("\\..\\") ||
    expanded.endsWith("/..") || expanded.endsWith("\\.."))) {
  throw new Error(`work-root value \`${value}\` expanded to a path containing traversal sequences: \`${expanded}\``);
}

Alternatively, run.mjs already imports node:path, so path.resolve could be applied there after expandPathValue returns — one line, zero new imports:

parsed.workRoot = path.resolve(expandPathValue(parsed.workRoot, process.env, os.homedir()));

Fix this →

2. ~\ on a POSIX host silently produces a malformed path component

run-args.js L124

if (expanded === "~" || expanded.startsWith("~/") || expanded.startsWith("~\\")) {

If a POSIX user accidentally writes ~\corpus as their library_dir value, the expansion produces /srv/dev-home\corpus (backslash embedded in a POSIX path component name), which then passes isAbsolutePath (starts with /). The child process would try to create or write to a directory named literally dev-home\corpus. SKILL.md correctly describes ~\ as "the Windows form," but that language doesn't make it fail-loud on POSIX — it just silently misbehaves. A one-line guard is sufficient:

if (process.platform !== "win32" && expanded.startsWith("~\\")) {
  throw new Error(`\`~\\\` is the Windows tilde form and is not valid on this platform`);
}

This converts the silent malformed-path case into a loud error consistent with the module's established contract. Alternatively, documenting ~\ as "valid only on Windows, silently malformed on POSIX" in the JSDoc is also a defensible choice — the docs already scope it, the question is just whether the code enforces it.

Fix this →

---

Minor open items (low priority, no action required before merge)

• Tilde + embedded env-var combination untested — ~/path/${SUBDIR} is handled correctly by the sequential expansion order (tilde first, then ENV_REF.replace), but no test pins that order. Would protect against future reordering.
• %NAME% embedded in a path prefix untested — C:\Users\%USERNAME%\knowledge is not exercised (only standalone %NAME% is). The regex handles it via the same replacer path, but a case would close the gap.

---

What's working well

Every finding above was identified in prior passes. The implementation itself (expandPathValue, isAbsolutePath, the single expansion point in run.mjs, the SKILL.md single-quote guidance, the CHANGELOG / semver bump) remains correct and clean. The three post-a7ac47d commits are all fixture / doc scoping fixes, each closing a concern from review without introducing new issues.

---

Verdict

Approve with notes. The new commit is correct and closes the remaining Windows fixture concern from the prior pass. Two findings are still open: finding #1 (path normalization) is the most substantive — a one-line fix in run.mjs that would fully close the fail-loud gap — and finding #2 (~\ POSIX) is worth a deliberate design call. Neither blocks merge.

[claude]

Claude finished @kyle-sexton's task in 2m 41s —— View job

---

Security Review — PR #894

• Gather PR diff and changed files
• Audit run-args.js — expandPathValue helper (path traversal, injection surfaces)
• Audit run.mjs — launcher wiring, spawn safety, env construction
• Review plugin.json, SKILL.md docs for credential/secret exposure
• Synthesize and post final security report

---

Findings

SUGGESTION · Confidence: CONFIRMED — Expanded path accepted without .. normalization (still open from prior passes)

run-args.js L139

isAbsolutePath checks only the leading byte sequence — /, \\, or [A-Za-z]:[\\/]. A consumer env var whose value contains a directory-traversal sequence (e.g. KNOWLEDGE_CORPUS_DIR=/safe/root/../../etc) expands to a string that starts with / and clears the guard, then flows into YOUTUBE_WORK_ROOT. There is no cross-user privilege vector — the consumer controls both library_dir and their own env vars — but the fail-loud contract promises to catch bad expansions and this one slips through silently.

This finding was independently raised in both prior security review passes and has not been addressed by the three commits on this branch. The two new commits (aef9f3a, c770610) are test-fixture changes only and introduce no new surface.

Minimal fix in run.mjs, where node:path is already imported, after line 44:

// change:
parsed.workRoot = expandPathValue(parsed.workRoot, process.env, os.homedir());
// to:
parsed.workRoot = path.resolve(expandPathValue(parsed.workRoot, process.env, os.homedir()));

path.resolve normalizes .. segments and produces a canonical absolute path without adding any new dependency to the intentionally pure run-args.js module. Fix this →

---

SUGGESTION · Confidence: PLAUSIBLE — ~\ on a POSIX host silently produces a malformed path (still open from prior passes)

run-args.js L124

If a POSIX user accidentally writes ~\corpus as their library_dir value, the tilde branch matches (expanded.startsWith("~\\")), expanded becomes /srv/dev-home\corpus (backslash embedded in a POSIX path component), and isAbsolutePath passes it (starts with /). The child process would attempt to use a directory whose name contains a literal backslash. Low likelihood in practice — the docs describe ~\ as the Windows form — but there is no explicit guard or throw on POSIX. A process.platform !== 'win32' check before entering the ~\ branch, or an explicit note in the manifest description that ~\ is Windows-only and invalid on POSIX, would close this deliberately. Also unchanged from prior review passes.

---

Security-positive observations (new commits only)

c770610 — drop remaining Windows user-path literal from fixture

The Windows tilde-expansion test at run-args.test.js L125 now uses C:\\dev-home instead of C:\\Users\\dev. This directly addresses the concern raised in the most recent prior pass: the C:\\Users\\dev literal was at risk of triggering the machine-specific-paths gate if its Windows-username pattern ever widens. The fix is correct and preemptive. [Confirmed resolved.]

aef9f3a — use non-user-path fixtures in expandPathValue tests

POSIX home fixture moved from /home/dev to /srv/dev-home. Eliminates the /home/<user> pattern that the hardcoded-path gate flags. Correct and security-positive. [Confirmed resolved.]

---

Unchanged security-positive observations

• spawnSync without shell: true — rest args and the target path are passed as a JS array; no shell interpretation surface. Confirmed safe.
• Script containment check — path.relative(here, target) rejecting upward or absolute results is intact and unchanged by this PR.
• Env-var name regex — [A-Za-z_][A-Za-z0-9_]* restricts substitution to well-formed identifiers; no path separator, metacharacter, or Unicode can appear in a matched name.
• Fail-loud on unset/empty env vars — !resolved catches both undefined and "" in the replacer; silent empty substitution is not possible.
• ENV_REF global regex with replace — String.prototype.replace resets lastIndex before each call; no stale-state risk despite the module-scope /g flag.
• No secrets in manifest or docs — plugin.json and SKILL.md additions treat library_dir as a plain path scalar; cookie-file fields carry no credential values and note "never commit cookie files."
• os.homedir() is the sole tilde source — supplied as os.homedir() at the one call site in run.mjs; it is not user-controlled.

---

Summary

No new security issues introduced by the two new commits. One confirmed finding (.. traversal) and one plausible finding (~\ on POSIX) remain open from prior passes. The .. traversal finding is the most actionable: a one-line path.resolve() wrap in run.mjs closes it without touching the dependency-free run-args.js module.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c770610d0b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Quote env-form values before configuring library_dir

When the setup skill recommends ${NAME} as a headless library_dir value, the immediately preceding claude plugin install ... --config library_dir=<value> command will be run through the user's shell; in Bash, an unquoted library_dir=${KNOWLEDGE_CORPUS_DIR} is expanded before Claude Code sees it, so the stored option becomes the machine-specific path (or empty if unset) instead of the portable literal that run.mjs is supposed to expand/fail-loud on. Please make this guidance include literal quoting/escaping for the env-reference form (and %NAME% for cmd.exe) when using --config.

Useful? React with 👍 / 👎.