fix: four audit defects — MCP stdio, ambient store rebinding, git option injection, and --local in the global namespace

[williamttruong]

Four defects from an audit of main, each one small, independent, and covered by a test that was
checked against a mutant before being trusted. Four commits so any one can be reverted alone.

Found by a 16-dimension sweep over the engine at ee251fb; every finding in it faced three
independent refutation attempts, and these four are the ones whose blast radius is a wrong result
rather than a missing nicety. The rest of the sweep is not in this PR.

---

1. fix(skills): the run banner belongs on stderr, not the MCP transport

runSkillPackage writes its banner with console.log. It has two callers that disagree about what
stdout is: the CLI, where it is a terminal, and knowl_skill_run, where it is the JSON-RPC frame
stream of a stdio MCP server. On that path the banner is interleaved into the protocol and the
client fails to parse the response to a call whose skill actually ran — an action taken, reported
as a transport error. knowl serve already makes this exact choice for its own startup banner.

The test asserts on the stream, not the string. tests/skills/global-layer.test.ts covers what
the banner says and stayed green throughout, because a banner's content and its destination are
separate claims and only one was ever checked. Worth noting for the review: spying on
process.stdout.write alone does not catch a console.log regression under vitest, which the
console intercepts first — the first version of this test passed against the mutant. Both writers
are watched now.

2. fix(store): creating the global store must not rebind the process

ensureGlobalStore bootstraps ~/.knowl/global.db through initDbPath, which assigns the
module-level globalContext — the handle every unscoped store operation resolves through. On the
CLI that is invisible because the process exits. Under knowl serve the process outlives the call,
so one knowl_store with namespace: 'global' rebound the ambient database and every later
project write in that session went to the wrong file while reporting success.

Bootstrap only ever needed a scoped handle, so this moves to withDbPath — the same distinction
the module already draws for namespace hops. Safe for every caller: both global write paths
re-enter explicitly with their own withDbPath(globalStorePath(), ...), so none of them read the
ambient afterwards.

3. fix(store): a caller-chosen revision must reach git as an operand

listChangedFilesSince and listRenamedPathsSince interpolate a commit into an argv option
position, and the caller is not always the engine — knowl_drift exposes since as an MCP tool
argument accepting any string up to 200 characters. spawnSync runs without a shell, so this was
never command injection; it was git-option injection, which is enough, because git diff --output=<path> exits 0 and writes the diff over the named file.

The separator is --end-of-options and deliberately not --: to git diff a bare -- means
"pathspecs follow", which would demote the range to a path and make every drift check silently
return nothing. The third test in the file is what tells those two apart, and it fails under a --
mutant while the security assertions still pass.

4. fix(cli): --local must exclude in the global namespace too

The project branch of knowl store pairs excludeFromPublish with unstagePublish after the
write. The global branch printed the same Marked local. It will not be published. and did
neither, so the promise was made by the message alone.

It reaches data because the global namespace is not exempt from the staging seam: maybeAutoStage
skips exactly one namespace, session, and the machine store can now itself be cloud-connected. So
an atom someone marked local was queued for the team while being told it never would be. Tested end
to end through the built CLI, because the missing code was missing from a commander branch rather
than from the helper it should have called — no unit test could see it.

---

Verification. npx tsc --noEmit clean, npm run lint clean, full suite green. Each fix was
reverted and re-run to confirm its test fails without it; the two-mutant check on #3 covers both
halves of the separator choice.

Not fixed here, flagged for you. The same sweep found that capture.events' correction half is
unreachable on Claude, Codex and Hermes and works only on OpenClaw, because the installed prompt
hook routes to agent-reminder, which never runs the lifecycle, and the Hermes plugin buries the
prompt under extra where host-hook.ts reads a root field. That one changes behaviour people
depend on, so it wants your call on the shape rather than a patch from me.

---

🤖 Generated with Claude Code

[dat999zx]

Reviewed all four fixes and verified each against a mutant rather than reading the diff. Three are correct as written; the fourth was correct on the CLI and had a twin on the MCP surface that this branch made worse, so I pushed a fifth commit fixing it. Detail below.

Verified, not just read

Each claim in the PR body I could check, I checked by breaking the fix and confirming which tests notice:

mutant	result
END_OF_OPTIONS → '--'	2 of 3 fail: "refuses the hostile value" and "still reports a real range". The --output write test still passes. Exactly the split the PR body predicts — the third test is what separates the security fix from the correctness regression.
drop END_OF_OPTIONS from listChangedFilesSince	a different 2 of 3 fail: the --output write test and the refusal test. The range test passes.
writeRunBanner → console.log	both banner-stream tests fail.
withDbPath → initDbPath in ensureGlobalStore	2 of 3 ambient tests fail; the "still creates the store" test passes, which is right — bootstrap is not what broke.

I also confirmed the git-option injection is real on this machine rather than theoretical: git diff --name-only "--output=<path>" HEAD~1..HEAD exits 0 and writes the file. --end-of-options needs git ≥ 2.24 (Nov 2019); worth noting in case anything still targets older, but every CI runner here is well past it.

The note about process.stdout.write spies not catching console.log under vitest is accurate and worth the paragraph it got — I'd have written the weaker test.

---

🔴 The --local fix is half of the defect

src/mcp/tools.ts:690 — same bug, other surface, and this branch turns it from latent to live.

knowl_store writes the atom inside withDbPath/withNamespaceDatabase and runs the exclusion outside it, against the ambient handle. Both excludeFromPublish and unstagePublish go through getClient(), so for every non-project namespace the atom lands in one database and its cloud_excluded row lands in another. The namespace store's publisher reads its own exclusion table, finds nothing, and stages an atom the caller was told would never be published — the same failure commit 4 describes for the CLI, reached through the tool an agent actually calls.

Measured through the real MCP server with namespace: 'global', local: true:

base (main):  EXCLUDED IN GLOBAL ["d9a5dc19..."]   EXCLUDED IN PROJECT []
this branch:  EXCLUDED IN GLOBAL []                EXCLUDED IN PROJECT ["7e4e4de0..."]

Note which column is which. On main it lands correctly — by accident. ensureGlobalStore rebound the process to the global store on its way past, so the later ambient write happened to hit the right file. Commit 2 removes that rebinding, correctly, and in doing so removes the accident. So commit 2 is right and commit 4 is incomplete, and together they ship a regression for the one namespace that used to work.

session and organization were broken on main too and stay broken either way — commit 4 only fixes global, and only on the CLI.

Fixed in 59fa0d3: the exclusion moves inside the store callback, so all three routing branches get it from one place rather than three. Two things fell out of doing it there:

• Guarded on action !== 'duplicate'. A duplicate write returns the existing item, so excluding it would let a local: true call retroactively unpublish someone else's shared atom by restating its content. That hole is on main today.
• The test drives the real server over an in-memory transport (the tests/mcp/act-as-repo.test.ts shape) and asserts on both stores — present in global, absent from project. Asserting only the first passes against the unfixed engine on main, for the accidental-rebinding reason above.

---

🔵 Small things, take or leave

• src/store/drift.ts:99 — END_OF_OPTIONS is scoped to this module, but spawnSync('git', ...) with caller-influenced argv also appears in src/cloud/publish-gate.ts:19 and src/store/session-evidence.ts:19. Both are engine-controlled today (session-evidence has no importer at all), so nothing is exploitable — but if this ever becomes a repo-wide rule, runGit is the place, not the two call sites.
• Commit 5's message says "lines 145 and 179"; line numbers in a commit body go stale on the next edit to that file. The sentence works without them.

---

Verification

Full suite 421 files / 3955 tests green on Windows, plus eslint, tsc --noEmit, docs:check and check-version-sync.

CHANGELOG.md had no ## Unreleased — the 5.22.0 release commit renames it in place and nothing recreates it, so its absence is not "this repo skips changelogs". Recreated it with all five fixes (36c6f58).

Four independently revertable commits was the right call, and the commit bodies are the standard this repo should hold to. Good PR; it just needed its own fourth finding applied to the surface that matters most.