feat(config): multi-layer config overlay (project .jcode + env vars + MCP trust gate) - #173
Merged
Conversation
Add walk-up configuration discovery inspired by codex/opencode/pi:
1. Walk-up .jcode/config.json: from git root → cwd, each directory's
.jcode/config.json is loaded and merged (closer to cwd = higher
precedence). Security denylist blocks providers, telemetry, cloud,
SSH/Docker aliases, memory, developer, and auto-approve fields.
2. Walk-up AGENTS.md: from git root → cwd, each directory's AGENTS.md
is concatenated (root-first ordering). Monorepo roots can define
shared instructions while sub-packages add their own.
3. Standalone mcp.json: supports mcp.json/.mcp.json in ~/.jcode/,
walk-up .jcode/ dirs, and project root. Format is compatible with
Claude Desktop / Cursor ({ "mcpServers": { ... } }).
MCP security: existing servers' command/url cannot be overridden by
project config; new servers are allowed freely. Disabled flag is
one-way (project can disable, cannot re-enable).
All three transports (TUI, web, ACP) apply the overlay via
config.ApplyProjectOverlay() after LoadConfig(). Git root resolution
uses a single subprocess call shared across config and MCP loaders.
macOS symlink resolution (/var → /private/var) handled correctly.
📝 WalkthroughWalkthroughProject-level configuration, standalone MCP files, environment overrides, and walk-up ChangesProject Configuration Overlays
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant ApplyProjectOverlay
participant ProjectConfig
participant MCPFiles
participant SettingsAPI
Startup->>ApplyProjectOverlay: apply working-directory configuration
ApplyProjectOverlay->>ProjectConfig: load and merge project config
ApplyProjectOverlay->>MCPFiles: load and merge MCP files
ProjectConfig-->>ApplyProjectOverlay: effective project settings
MCPFiles-->>ApplyProjectOverlay: merged MCP servers
ApplyProjectOverlay-->>Startup: updated runtime configuration
SettingsAPI->>SettingsAPI: attach MCP and skill provenance
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add Project-Level Configuration section to configuration.md covering: - Walk-up .jcode/config.json discovery order - Mergeable fields and merge behavior - Security denylist (providers, telemetry, cloud, etc.) - Walk-up AGENTS.md for monorepos - Monorepo example Add Project-Level MCP Servers section to mcp.md covering: - Standalone mcp.json/.mcp.json discovery order - Claude Desktop / Cursor compatible format - Merge & security rules (command/url non-overridable) - One-way disable semantics
cnjack
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/config/project.go (2)
316-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment omits
headersfrom the merged tuning-field list.The comment says only "args, env, timeout, disabled" are merged, but the function below also merges
Headers(lines 329-336). Update the comment to keep it accurate, especially sinceHeaderscan carry auth tokens and is relevant to the security reasoning here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/project.go` around lines 316 - 320, Update the comment immediately above mergeProjectFields to include headers in the list of merged tuning fields, while preserving the existing explanation that Command and URL cannot be overridden and that new servers receive their full project definition.
22-40: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMissing
context.Contextpropagation across new project/MCP discovery code. Neitherinternal/config/project.gonorinternal/config/mcp_file.gothread acontext.Contextthrough their new exported discovery functions, even though they perform subprocess exec (git) and file I/O; the shared root cause is that this whole new code path was designed without a ctx parameter.
internal/config/project.go#L22-L40: add acontext.Contextfirst parameter toGitRoot(and thread it throughLoadProjectConfig/loadProjectConfigWithRoot/ApplyProjectOverlay) so callers can bound/cancel the git subprocess instead of relying solely on the internal 2s timeout.internal/config/mcp_file.go#L49-L111: add acontext.Contextfirst parameter toLoadMCPFiles/loadMCPFilesWithRootand pass it through to the sharedGitRootcall once it accepts ctx.As per coding guidelines, "Use
context.Contextas the first parameter and propagate cancellation properly."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/project.go` around lines 22 - 40, The discovery APIs do not propagate caller cancellation. In internal/config/project.go lines 22-40, add context.Context as the first parameter to GitRoot and thread it through LoadProjectConfig, loadProjectConfigWithRoot, and ApplyProjectOverlay, preserving the existing timeout as appropriate; in internal/config/mcp_file.go lines 49-111, add the same parameter to LoadMCPFiles and loadMCPFilesWithRoot and pass it to GitRoot, then update affected callers to provide the context.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/command/web.go`:
- Around line 412-414: Update the Web task agent construction flow around the
project overlay and process-wide mcpToolsPtr so MCP tools are loaded and cached
per task/project from taskCfg.MCPServers rather than reused from the startup
configuration. Apply the same per-task loading to the bootstrap task, while
preserving distinct MCP configurations across projects and the existing
local-task overlay behavior.
In `@internal/config/project.go`:
- Around line 321-346: Update mergeMCPServer so project overlays cannot fully
replace Args or Env for pre-existing MCP servers, preserving the same
immutability boundary as Command and URL. Either deny these overrides entirely
or implement a narrowly scoped additive/keyed merge that blocks dangerous
environment variables and execution-related argument flags; retain existing
behavior for unrelated fields and ensure full Args/Env control remains available
only when creating genuinely new servers.
In `@site/docs/configuration.md`:
- Line 350: Update the fenced directory-tree code block in the configuration
documentation to include the text language identifier, changing the opening
fence to specify text while preserving the block contents.
---
Nitpick comments:
In `@internal/config/project.go`:
- Around line 316-320: Update the comment immediately above mergeProjectFields
to include headers in the list of merged tuning fields, while preserving the
existing explanation that Command and URL cannot be overridden and that new
servers receive their full project definition.
- Around line 22-40: The discovery APIs do not propagate caller cancellation. In
internal/config/project.go lines 22-40, add context.Context as the first
parameter to GitRoot and thread it through LoadProjectConfig,
loadProjectConfigWithRoot, and ApplyProjectOverlay, preserving the existing
timeout as appropriate; in internal/config/mcp_file.go lines 49-111, add the
same parameter to LoadMCPFiles and loadMCPFilesWithRoot and pass it to GitRoot,
then update affected callers to provide the context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0009cadb-59a4-466b-b6dd-d28dcc7e9faf
📒 Files selected for processing (11)
internal/command/acp.gointernal/command/interactive.gointernal/command/web.gointernal/config/mcp_file.gointernal/config/mcp_file_test.gointernal/config/project.gointernal/config/project_test.gointernal/prompts/memory.gointernal/prompts/memory_test.gosite/docs/configuration.mdsite/docs/overview/mcp.md
Comment on lines
+412
to
+414
| if exec == nil { // project config overlay (local tasks only) | ||
| config.ApplyProjectOverlay(taskCfg, taskPwd) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Project MCP files never reach Web task agents.
At Line 413, the overlay populates taskCfg.MCPServers, but agent construction reads only the process-wide mcpToolsPtr, which is initialized from the unoverlaid startup config. A project that defines only mcp.json gets no MCP tools in Web, and tasks from different projects cannot retain distinct MCP configurations. Load/cache MCP tools per task/project from taskCfg.MCPServers, including the bootstrap task.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/command/web.go` around lines 412 - 414, Update the Web task agent
construction flow around the project overlay and process-wide mcpToolsPtr so MCP
tools are loaded and cached per task/project from taskCfg.MCPServers rather than
reused from the startup configuration. Apply the same per-task loading to the
bootstrap task, while preserving distinct MCP configurations across projects and
the existing local-task overlay behavior.
Comment on lines
+321
to
+346
| func mergeMCPServer(base, overlay *MCPServer) { | ||
| // Command and URL are intentionally NOT merged for existing servers. | ||
| if len(overlay.Args) > 0 { | ||
| base.Args = overlay.Args | ||
| } | ||
| if len(overlay.Env) > 0 { | ||
| base.Env = overlay.Env | ||
| } | ||
| if len(overlay.Headers) > 0 { | ||
| if base.Headers == nil { | ||
| base.Headers = make(map[string]string, len(overlay.Headers)) | ||
| } | ||
| for k, v := range overlay.Headers { | ||
| base.Headers[k] = v | ||
| } | ||
| } | ||
| if overlay.TimeoutSeconds > 0 { | ||
| base.TimeoutSeconds = overlay.TimeoutSeconds | ||
| } | ||
| // Disabled can be set to true by project config (to suppress a global | ||
| // server in this project) but not back to false (project cannot | ||
| // re-enable a server the user globally disabled). | ||
| if overlay.Disabled { | ||
| base.Disabled = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Full Args/Env replacement on existing servers undermines the Command/URL immutability guarantee.
mergeMCPServer blocks overriding Command/URL for existing servers specifically to stop a malicious project repo from redirecting a trusted server to a different binary/endpoint. But Args and Env are fully replaced (not restricted) for those same "existing" servers:
if len(overlay.Args) > 0 {
base.Args = overlay.Args
}
if len(overlay.Env) > 0 {
base.Env = overlay.Env
}For a stdio-based trusted server, an attacker-controlled .jcode/config.json/mcp.json in the repo can replace Env wholesale to inject well-known code-execution vectors (LD_PRELOAD, DYLD_INSERT_LIBRARIES, NODE_OPTIONS, PYTHONPATH, PERL5LIB, GIT_SSH_COMMAND, etc.) against the fixed, trusted Command — achieving arbitrary code execution despite the binary path itself being immutable. Likewise, Args replacement can inject flags many interpreters honor for code execution (e.g. -r, -e, -c, --require) without ever changing Command. This effectively defeats the security boundary the comment at line 317 claims to provide.
Consider treating Env/Args the same way as Command/URL for pre-existing servers (deny override entirely, or only allow additive/keyed merges with a denylist of known dangerous variable/flag names), reserving full control only for genuinely new servers the project introduces.
🔒 Illustrative direction (denylist / no full override for existing servers)
func mergeMCPServer(base, overlay *MCPServer) {
// Command and URL are intentionally NOT merged for existing servers.
- if len(overlay.Args) > 0 {
- base.Args = overlay.Args
- }
- if len(overlay.Env) > 0 {
- base.Env = overlay.Env
- }
+ // Args/Env are also security-sensitive for existing (trusted) servers:
+ // unrestricted overrides can achieve code execution via well-known
+ // interpreter env vars/flags (LD_PRELOAD, NODE_OPTIONS, -r/-e, ...)
+ // even though Command itself stays fixed. Do not allow full override
+ // here; either skip entirely or apply a strict denylist/keyed merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/project.go` around lines 321 - 346, Update mergeMCPServer so
project overlays cannot fully replace Args or Env for pre-existing MCP servers,
preserving the same immutability boundary as Command and URL. Either deny these
overrides entirely or implement a narrowly scoped additive/keyed merge that
blocks dangerous environment variables and execution-related argument flags;
retain existing behavior for unrelated fields and ensure full Args/Env control
remains available only when creating genuinely new servers.
Add provenance tracking to MCP servers and skills so the Settings UI can display which config layer defines each entry: Backend: - MCPServer.Source field (json:"-", not persisted) tagged at load time - Skill.Source field set during scan (builtin/agents/user/project) - /api/mcp/list returns scope per server - /api/skills returns granular source per skill Frontend: - MCP tab shows accent 'Project' chip for project-scoped servers - Skills tab shows 'Project' / 'Agents' chips for non-builtin skills - i18n keys added for all 5 locales (en/zh-Hans/zh-Hant/ja/ko)
cnjack
changed the title
…CODE_CONFIG, etc.) Add the highest-precedence config layer: environment variables override both global (~/.jcode/config.json) and project-level (.jcode/config.json) settings. Supported variables: - JCODE_MODEL: active model (provider/model format) - JCODE_SMALL_MODEL: small/fast model - JCODE_MAX_ITERATIONS: agent iteration cap - JCODE_THEME: color theme name - JCODE_LANGUAGE: UI locale - JCODE_DEFAULT_MODE: session mode (approval|plan|auto|full_access) - JCODE_CONFIG: custom config file path Key design decisions: - Env vars MAY override DefaultMode (unlike project config denylist) because they are set by the user/CI directly, not an untrusted repository. - Invalid values are logged and ignored (fail-safe). - JCODE_CONFIG redirects both reads and writes (SaveConfig). - Remote tasks (SSH/Docker) get env overlay even though project config is skipped (env vars are local process state). - Web mode server-level config gets env overlay for provider resolution and startup mode. Includes comprehensive tests and site documentation update.
cnjack
changed the title
- Gate new project MCP servers behind JCODE_MCP_TRUST_PROJECT=1 (same pattern as project hooks). A hostile repo shipping a .jcode/config.json with a new stdio server can no longer achieve arbitrary code execution. - Block dangerous env vars (LD_PRELOAD, DYLD_INSERT_LIBRARIES, NODE_OPTIONS, PYTHONPATH, etc.) in project MCP Env overlay to prevent code-execution vectors against immutable Command binaries. - Move Browser/Computer to the project config denylist — these are capability toggles with preapproved permission lists, same escalation class as AutoApprove/DefaultMode. - Fix markdownlint MD040: add language identifier to fenced code blocks. - Update tests to cover trust-gated MCP server behavior.
cnjack
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/env.go`:
- Around line 39-57: Validate JCODE_MODEL, JCODE_SMALL_MODEL, JCODE_THEME, and
JCODE_LANGUAGE against their existing configuration domains before assigning
them in the environment override logic; log each rejected non-empty value via
Logger() and preserve the prior configuration. In internal/config/env_test.go
lines 44-143, add invalid-value cases for all four variables and assert that
each existing value remains unchanged.
In `@internal/web/skills.go`:
- Around line 27-33: Update the API field contract associated with the skill
source value in the skills response to document all emitted provenance values:
builtin, user, agents, and project. Keep the source-selection logic unchanged,
and prefer the existing shared type if one is already used for this field.
In `@web/src/i18n/locales/ja.ts`:
- Around line 538-541: Translate the agents scope label in
web/src/i18n/locales/ja.ts lines 538-541 to Japanese agent terminology such as
エージェント, and apply the corresponding Korean translation such as 에이전트 in
web/src/i18n/locales/ko.ts lines 538-541; update the agents entries in the scope
objects while leaving project unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f1ffd5e-5f4b-4033-a2ae-6a461d526f8b
📒 Files selected for processing (18)
internal/command/web.gointernal/config/config.gointernal/config/env.gointernal/config/env_test.gointernal/config/mcp_file.gointernal/config/project.gointernal/config/project_test.gointernal/skills/skills.gointernal/web/mcp.gointernal/web/skills.gosite/docs/configuration.mdweb/src/components/SettingsView.tsxweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/lib/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/command/web.go
- internal/config/mcp_file.go
- internal/config/project.go
- site/docs/configuration.md
- internal/config/project_test.go
Comment on lines
+39
to
+57
| if v := os.Getenv(EnvModel); v != "" { | ||
| cfg.Model = v | ||
| } | ||
| if v := os.Getenv(EnvSmallModel); v != "" { | ||
| cfg.SmallModel = v | ||
| } | ||
| if v := os.Getenv(EnvMaxIterations); v != "" { | ||
| if n, err := strconv.Atoi(v); err == nil && n > 0 { | ||
| cfg.MaxIterations = n | ||
| } else { | ||
| Logger().Printf("[config] ignoring invalid %s=%q (must be a positive integer)", EnvMaxIterations, v) | ||
| } | ||
| } | ||
| if v := os.Getenv(EnvTheme); v != "" { | ||
| cfg.Theme = v | ||
| } | ||
| if v := os.Getenv(EnvLanguage); v != "" { | ||
| cfg.Language = v | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate all environment overrides before applying them.
JCODE_MODEL, JCODE_SMALL_MODEL, JCODE_THEME, and JCODE_LANGUAGE currently replace valid configuration with any non-empty string. This violates the PR requirement that invalid environment values be logged and ignored; malformed high-precedence values can leave startup with an unusable model or invalid UI settings.
internal/config/env.go#L39-L57: validate each value against its existing config domain before assignment, and log rejected values throughLogger().internal/config/env_test.go#L44-L143: add invalid-value cases for model, small model, theme, and language, asserting the prior config remains unchanged.
📍 Affects 2 files
internal/config/env.go#L39-L57(this comment)internal/config/env_test.go#L44-L143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/env.go` around lines 39 - 57, Validate JCODE_MODEL,
JCODE_SMALL_MODEL, JCODE_THEME, and JCODE_LANGUAGE against their existing
configuration domains before assigning them in the environment override logic;
log each rejected non-empty value via Logger() and preserve the prior
configuration. In internal/config/env_test.go lines 44-143, add invalid-value
cases for all four variables and assert that each existing value remains
unchanged.
Comment on lines
+27
to
+33
| source := sk.Source | ||
| if source == "" { | ||
| if sk.Builtin { | ||
| source = "builtin" | ||
| } else { | ||
| source = "user" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep the skill-source contract in sync with emitted values.
This logic can return builtin, user, agents, or project, while the API field is still documented as builtin | local. Update the contract comment (or use a shared type) so clients do not reject the new provenance values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/web/skills.go` around lines 27 - 33, Update the API field contract
associated with the skill source value in the skills response to document all
emitted provenance values: builtin, user, agents, and project. Keep the
source-selection logic unchanged, and prefer the existing shared type if one is
already used for this field.
Comment on lines
+538
to
+541
| scope: { | ||
| project: 'プロジェクト', | ||
| agents: 'Agents', | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Translate the new agents scope label in every locale.
The new agents badge remains English in both locales:
web/src/i18n/locales/ja.ts#L538-L541: use Japanese agent terminology such asエージェント.web/src/i18n/locales/ko.ts#L538-L541: use Korean agent terminology such as에이전트.
📍 Affects 2 files
web/src/i18n/locales/ja.ts#L538-L541(this comment)web/src/i18n/locales/ko.ts#L538-L541
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/i18n/locales/ja.ts` around lines 538 - 541, Translate the agents
scope label in web/src/i18n/locales/ja.ts lines 538-541 to Japanese agent
terminology such as エージェント, and apply the corresponding Korean translation such
as 에이전트 in web/src/i18n/locales/ko.ts lines 538-541; update the agents entries
in the scope objects while leaving project unchanged.
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.
Summary
Complete multi-layer configuration system for jcode, combining project-level config overlays, standalone MCP discovery, environment variable overrides, and Settings UI scope badges. Supersedes #172.
Config Precedence (lowest → highest)
~/.jcode/config.json(global)<git-root>/.jcode/config.json→<cwd>/.jcode/config.json(walk-up)~/.jcode/mcp.json→ walk-up.jcode/mcp.json→<cwd>/mcp.jsonJCODE_MODEL,JCODE_CONFIG, etc.)Features
Project Config Overlay
.jcode/config.jsonfrom git root → cwd (monorepo-friendly)AGENTS.mdconcatenated root-firstmcp.json/.mcp.json(Claude Desktop / Cursor compatible)MCP Trust Gate (from review)
JCODE_MCP_TRUST_PROJECT=1(same pattern as project hooks)LD_PRELOAD,DYLD_INSERT_LIBRARIES,NODE_OPTIONS, etc.) blocked in project MCP Env overlaycommand/urlimmutable; only tuning fields mergeableEnvironment Variable Overrides
JCODE_MODELJCODE_SMALL_MODELJCODE_MAX_ITERATIONSJCODE_THEME/JCODE_LANGUAGEJCODE_DEFAULT_MODEJCODE_CONFIGSettings UI Scope Badges
Projectchip for project-scoped serversProject/Agentschips for non-builtin skillsSecurity Fixes (from CodeRabbit review)
JCODE_MCP_TRUST_PROJECT=1Testing
go test ./...)Summary by CodeRabbit
New Features
mcp.json.AGENTS.mdfiles are combined across repository directories.Documentation