fix: gate .kimchi/ and .claude/ project resources behind project trust - #1215
Open
mikenorgate wants to merge 7 commits into
Open
mikenorgate wants to merge 7 commits into
mikenorgate wants to merge 7 commits into
Conversation
Kimchi Code Review
Summary📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Coverage is strong: every gated reader gains a fail-closed regression test ( 📝 Found 6 issue(s). See inline comments for details. (1 finding(s) referenced lines no longer in the diff and were omitted.) What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 5/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Coverage is strong: every gated reader gains a fail-closed regression test (config.test.ts, tags.test.ts, kimchi-hooks/definition.test.ts, permissions/config.test.ts, claude-code-skills/definition.test.ts, prompt-enrichment.test.ts), the docs promise a unit cross-check between the patch's embedded KIMCHI_TRUST_REQUIRING_PROJECT_RESOURCES copy and TRUST_REQUIRING_PROJECT_RESOURCES, and an E2E test (tests/e2e/tui/project-trust-gate.test.ts) verifies a malicious cloned repo gets zero traffic while untrusted. Two caveats: the core gate module (src/project-scope-trust.ts, src/project-trust.ts) and several gated readers were omitted from the provided diff (truncated), so their tests could not be examined; and several test files (e.g. agents/skill-loader.test.ts, ferment-v2/evaluator.test.ts) call setProjectScopeTrusted without an afterEach reset, relying on vitest module isolation.
📝 Found 6 issue(s). See inline comments for details. (1 finding(s) referenced lines no longer in the diff and were omitted.)
mikenorgate
added a commit
that referenced
this pull request
Sep 16, 2026
CI root cause: master's newer tests called promptEnrichmentExtension with the old array signature; the merge exposed them against the getter-only signature. Fixed the two call sites. Review fixes (kimchi-review bot, PR #1215): - objective-file: gate the save side too — while the project is untrusted, saveObjectiveFile returns the raw text instead of writing .kimchi/plans/*-objective.md (writing into an untrusted repo would arm the trust prompt on the next launch via the .kimchi/plans scan entry); regression test added - agents memory: skip ensureMemoryDir for project/local scopes in an untrusted project (same self-arming write-side class as above; read side was already gated) - config: permission warnings are now emitted once per path per process — the lazy configuredSkillPaths getter re-runs loadConfig on every resources_discover, which previously re-printed the chmod-600 warning per event - cli: configuredSkillPaths preserves the pre-change dedup ([...new Set(...)]) so duplicate config entries don't multiply downstream expansion work - cli: the pre-main trust gate now falls back to the entry.ts default agent dir when KIMCHI_CODING_AGENT_DIR is unset, instead of silently skipping gate priming - mcp-adapter: loadMcpConfig and getServerProvenance take a cwd parameter (session-scoped callers pass ctx.cwd; ACP sessions with a cwd differing from the server process cwd now gate on their own tree); project-relative import paths resolve against it too - agents /agents menu: findAgentFile and projectAgentsDir take the session cwd from the command context - tests: afterEach resetProjectScopeTrustForTests added to the files that set trust without a matching reset (skill-loader x2, ferment-v2 evaluator/index, permissions index) so the module-global gate cannot leak between tests Co-Authored-By: Kimchi <noreply@kimchi.dev>
A cloned repo shipping project-local config took effect at startup without the trust prompt: pi's trust-requiring scan looked under .config/kimchi/harness (never .kimchi/), and kimchi's own readers never consulted the trust decision at all. That gave an untrusted repo code execution via bash hooks and skills, permission-layer disarmament via .kimchi/permissions.json, and API-key + conversation theft via a project llmEndpoint override. - Patch pi's hasTrustRequiringProjectResources to also scan .kimchi/ and .claude/ trust-requiring entries, so the standard "Trust project folder?" prompt fires (and headless resolution fail-closes) for repos shipping them - Add a fail-closed, cwd-keyed project-scope gate (src/project-scope-trust.ts) consulted by every project-scope reader: loadConfig, permissions, bash hooks, claude-code hooks/skills, skill roots, agent settings/personas/ memory, tags, and ferment-v2 objective files - Open the gate from persisted decisions before the pre-main config read (resolvePreMainProjectTrust) and sync pi's in-session decision at session_start (settings-trust-sync), so trusted projects keep working - Make config-derived skill paths lazy so post-trust resource discovery sees current config instead of a startup snapshot - Regression tests per reader + TUI e2e: an untrusted repo's malicious .kimchi/config.json receives zero traffic; a persisted Trust applies the project endpoint and key on the next launch Co-Authored-By: Kimchi <noreply@kimchi.dev>
Extend the project-trust regression coverage so declining the prompt is proven inert, not just config-inert: - unit (bash-hooks): a project hook the user pre-enabled in global settings is not executed while untrusted, and runs again once trusted - unit (prompt-enrichment): resources_discover contributes no .kimchi/ or .claude/ skill roots while untrusted, and the project root once trusted - e2e: a hostile project surface (executable bash hook with a marker file, prompt-injection skills under .kimchi/ and .claude/, hook pre-enabled in user settings) — after "Do not trust", the bash tool call completes without the hook ever executing (marker absent) and no chat request body carries the hostile skill names or descriptions Also harden the persisted-decision e2e relaunch wait (full process spawn after quit) with an explicit 30s startup timeout. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Review of the project-trust gate found readers with program-execution or prompt-injection semantics that were neither gated nor trust-scanned, plus detection/enforcement scope mismatches and maintainability debt. This closes all of them: - Gate .kimchi/mcp.json (loadMcpConfig + getServerProvenance): an untrusted repo must not register (and thereby spawn) MCP servers at session start - Gate .kimchi/hooks.json and hooks.local.json (kimchi-hooks adapter, mirroring the claude-code-hook-adapter) and register discovered kimchi hooks in the dynamic resource definitions so their default-enabled execution is visible and toggleable in /resources - Gate .kimchi/ferments (resolveFermentsDir falls back to the user-global store while untrusted, so shipped ferment state neither loads nor gets mutated by crash recovery) - Ancestor-walk the kimchi/claude trust-scan entries in the pi patch (with the user's home excluded), matching the ancestor-walking skills/tags readers — launching from a repo subdirectory no longer skips the prompt - Gate the cwd expansion of relative configured skill paths (expandConfiguredSkillPaths), closing the extraPaths route that the default first-run setup's relative .claude/skills/.pi entries fed - Gate the /agents menu's project file reads (findAgentFile) - Export TRUST_REQUIRING_PROJECT_RESOURCES as the canonical scan list from src/project-scope-trust.ts with a unit test that cross-checks the pi patch's embedded copy, so gating a new reader without a scan entry fails CI instead of silently auto-trusting - Make readMemoryIndex's scope/cwd parameters required so the compiler enforces its trust gate at every call site - De-duplicate the config-path classification rule (mapConfigPath now returns the cwdResolved classification; isCwdResolvedConfigPath removed) and require the getter signature in both skill-path factories, removing the union type kept only for array-based test call sites - Update docs/project-trust.md's trigger table and gated-reader list Each new gate lands with a fail-closed regression test; the patch entry list, canonical constant, and docs are kept in sync by construction. Co-Authored-By: Kimchi <noreply@kimchi.dev>
CI root cause: master's newer tests called promptEnrichmentExtension with the old array signature; the merge exposed them against the getter-only signature. Fixed the two call sites. Review fixes (kimchi-review bot, PR #1215): - objective-file: gate the save side too — while the project is untrusted, saveObjectiveFile returns the raw text instead of writing .kimchi/plans/*-objective.md (writing into an untrusted repo would arm the trust prompt on the next launch via the .kimchi/plans scan entry); regression test added - agents memory: skip ensureMemoryDir for project/local scopes in an untrusted project (same self-arming write-side class as above; read side was already gated) - config: permission warnings are now emitted once per path per process — the lazy configuredSkillPaths getter re-runs loadConfig on every resources_discover, which previously re-printed the chmod-600 warning per event - cli: configuredSkillPaths preserves the pre-change dedup ([...new Set(...)]) so duplicate config entries don't multiply downstream expansion work - cli: the pre-main trust gate now falls back to the entry.ts default agent dir when KIMCHI_CODING_AGENT_DIR is unset, instead of silently skipping gate priming - mcp-adapter: loadMcpConfig and getServerProvenance take a cwd parameter (session-scoped callers pass ctx.cwd; ACP sessions with a cwd differing from the server process cwd now gate on their own tree); project-relative import paths resolve against it too - agents /agents menu: findAgentFile and projectAgentsDir take the session cwd from the command context - tests: afterEach resetProjectScopeTrustForTests added to the files that set trust without a matching reset (skill-loader x2, ferment-v2 evaluator/index, permissions index) so the module-global gate cannot leak between tests Co-Authored-By: Kimchi <noreply@kimchi.dev>
Two CI failure classes, both from the trust gate interacting with the CI environment: 1. The ancestor walk scanned the OS temp directory and above: CI runners carry stray .kimchi//.claude artifacts in /tmp (and dev machines can too), which made every temp-dir session trust-requiring — breaking the "trivially trusted" unit tests and hanging every e2e session at the trust prompt. The walk now stops at the temp root (mirroring the home exclusion): the temp dir is not a project tree, and a stray entry there must not gate unrelated temp sessions. Verified by planting a stray .claude/settings.json in tmpdir locally — the previously failing tests now pass. 2. E2E fixtures that seed .kimchi/ resources in their workDir (ACP plan-updates ferments, TUI ferment persistence tests) had no trust decision: ACP is headless and fail-closes, TUI sessions hung at the prompt. Both fixtures now pre-record a persisted trust decision for the workDir (realpath-keyed, matching pi's canonicalization), mirroring what an interactive session's first run persists. The TUI fixture gains a trustWorkDir option (default true); the project-trust-gate scenarios opt out to keep exercising the live prompt. Verified: full unit suite (11,000), check clean, trust-gate e2e 3/3, ferment-draft-proposal-persistence, ACP plan-updates 4/4, mcp-stdio 8/8. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Rebase onto master brought pi 0.85.1 and the published pi-mcp-adapter refactor; this commit restores the branch's trust functionality on top: - Re-apply the trust-scan patch hunks to the 0.85.1 package (patches/@earendil-works__pi-coding-agent@0.85.1.patch): the KIMCHI_TRUST_REQUIRING_PROJECT_RESOURCES entry list checked in cwd and ancestors, stopping at the user's home and the OS temp directory, plus the Kimchi-branded trust prompt and untrusted-warning strings. Header restored with the security section (ticket reference dropped per the earlier request); lockfile patch hash re-synced. - The vendored mcp-adapter is gone upstream; master's new src/extensions/mcp facade already gates project MCP config on pi's project trust (resolveMcpProjectTrust consults hasTrustRequiringProjectResources — which the patch extends with .kimchi/mcp.json), so the MCP gate carries over through that chain. The accidentally resurrected vendored adapter directory is removed. - Point the patch cross-check test and doc references at the 0.85.1 patch file. - Resolve the cli.ts rebase conflict keeping master's mcpAdapterExtensions block and the branch's deduplicated lazy configuredSkillPaths getter. Verified: full unit suite (10,878), tsc + biome clean, trust-gate e2e 3/3, ferment persistence e2e, ACP plan-updates 4/4, scan harness (mcp.json / ancestor skills trigger, empty tree does not). Co-Authored-By: Kimchi <noreply@kimchi.dev>
mikenorgate
force-pushed
the
fix/project-trust-gate
branch
from
September 18, 2026 07:23
3a8f71c to
92b3aa9
Compare
Diagnosis of the acp/mcp-e2e CI failures after the rebase: the ACP test "does not execute repository MCP configuration in a headless session without trust" failed because FermentStorage's constructor eagerly mkdir-ed <workDir>/.kimchi/ferments on every construction — for a trivially-trusted, no-resources project the kimchi gate is open, so the ferment store wrote into it, and that very directory is a trust-requiring scan entry. The mcp facade's later in-session resolveMcpProjectTrust then saw hasTrustRequiringProjectResources as true and short-circuited to trusted, executing the untrusted server. Fix: FermentStorage no longer creates its directory in the constructor and list() treats a missing directory as an empty store; writes create the directory on demand (the save path already mkdirs). A project that never had ferments no longer sprouts .kimchi/ferments just because kimchi started, so trust resolutions that run later in the same session are no longer armed by kimchi's own writes. Also: TUI fixture pretrust is now conditional on the seeded workDir actually being trust-requiring (so master's .mcp.json-only untrusted-MCP scenarios keep working), and the ACP fixture's pretrust is opt-in (plan-updates, the only test that seeds .kimchi/ into its workDir, passes pretrustWorkDir: true). Verified: full unit suite (10,878), check clean, ACP mcp-workflow + plan-updates 11/11, trust-gate e2e 3/3, mcp-project-trust e2e 2/2, ferment persistence e2e. Co-Authored-By: Kimchi <noreply@kimchi.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #
No GitHub issue linked yet — happy to link one if the team creates it.
What does this PR do?
Closes a security hole where a cloned repo's project-local
.kimchi/(and.claude/) configuration took effect at kimchi startup with no trust check:.kimchi/mcp.jsonspawns stdio processes at session start), bash hooks (.kimchi/hooks/bash/), and kimchi-native/claude hooks (hooks.json,.claude/settings.json).kimchi/permissions.jsonllmEndpointoverride (project sets the endpoint, no key → the user's saved global key is sent there)Two bugs were fixed:
.config/kimchi/harnessonly, so a repo shipping.kimchi/was auto-trusted and the "Trust project folder?" prompt never appeared. The pinned pi patch now scans an 18-entry kimchi/claude list in cwd and ancestors (home excluded), and the prompt/untrusted-warning strings speak as Kimchi.src/project-scope-trust.ts) is now consulted by every project-scope reader: config, permissions, hooks (bash/kimchi-native/claude), skills (roots + configured-path cwd expansion), agents settings/personas/memory, tags, ferments, plans, and MCP servers.Startup ordering: the gate opens pre-main from persisted decisions, syncs pi's in-session decision at session_start (with resource-cache invalidation), and is wired per ACP session. A canonical
TRUST_REQUIRING_PROJECT_RESOURCESconstant is CI-cross-checked against the patch's embedded list, so gating a new reader without a scan entry fails tests instead of silently auto-trusting.User-visible change: every project containing
.kimchi/or.claude/(including Claude Code repos) now prompts once per folder. Opt out withdefaultProjectTrust: "always"in global settings. Seedocs/project-trust.mdfor the full trust model, the first-trusted-session timing split, and the release note.Verification
pnpm run checkcleantests/e2e/tui/project-trust-gate.test.ts, 3/3): an untrusted repo's malicious.kimchi/config.json(attacker endpoint + key) receives zero traffic; a pre-enabled hostile bash hook never executes (marker file absent) and hostile skill text never reaches any chat request body; after a persisted Trust, the project endpoint/key apply on the next launchReview follow-ups (known, non-blocking)
.kimchi/thinking-steps.json(preference read) needs a gate-or-document decisionChecklist
pnpm run test)pnpm run check)docs/project-trust.md)