Narrow per-agent MCP override type and add authz support#63
Conversation
- Add os.Chmod after WriteFile for consistent 0600 on --force overwrite
- Update memory descriptions for ByteSize type ("4g", "512m")
- Narrow per-agent MCP template to enabled + authz only (PR #63)
- Add authz/config subsection checks to template test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jhrozek
left a comment
There was a problem hiding this comment.
Review Summary
1 HIGH, 1 LOW finding (confidence ≥ 8 only).
The type narrowing from *MCPConfig to *MCPAgentOverride is a solid security improvement — making illegal states unrepresentable. DDD layer boundaries are respected throughout. MergeMCPAuthzConfig is well-designed and thoroughly tested.
However, the agent merge loop in MergeConfigs does wholesale replacement of AgentOverride values, which silently drops security-relevant fields set by the operator in global config.
LOW: Dead data flow in composition root
cmd/bbox/main.go:655-656 writes sandboxCfg.MCP.Config and sandboxCfg.MCP.Authz but the sandbox layer never reads them (pkg/sandbox/ has zero references to .MCP.Authz or .MCP.Config). Authz is resolved entirely at the composition root before VMCPProvider construction. These writes are harmless but potentially misleading for future readers. Consider removing them or adding a comment explaining they exist for SDK consumers.
🤖 Generated with Claude Code
Replace AgentOverride.MCP (*MCPConfig) with a new MCPAgentOverride type that only exposes Enabled and Authz fields. This eliminates dead Group/Port/Config override code that was parsed but never had runtime effect, and adds tighten-only per-agent authz merging. - Add MCPAgentOverride type in pkg/domain/config - Export MergeMCPAuthzConfig for reuse by composition root - Simplify resolveMCPConfig to Enabled-only overrides - Merge per-agent authz before VMCPProvider construction - Update warnLocalConfigOverrides for new type - Rewrite tests for new semantics Fixes #62 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
45f40e9 to
1e67a98
Compare
mergeAgentOverride merges field-by-field instead of replacing the entire AgentOverride struct. Security fields (EgressProfile, MCP.Authz) use tighten-only semantics, AllowHosts are additive, and resource fields use local-overrides-when-non-zero — matching existing top-level merge patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* bbox config init
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
* Address review feedback and sync with main
- Add os.Chmod after WriteFile for consistent 0600 on --force overwrite
- Update memory descriptions for ByteSize type ("4g", "512m")
- Narrow per-agent MCP template to enabled + authz only (PR #63)
- Add authz/config subsection checks to template test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
AgentOverride.MCP(*MCPConfig) with a newMCPAgentOverridetype exposing onlyEnabledandAuthz— makes unsupported fields (Group,Port,Config) unrepresentable at compile timeMergeMCPAuthzConfigand add tighten-only per-agent authz merging in the composition root beforeVMCPProviderconstructionresolveMCPConfigto Enabled-only overrides, removing dead Group/Port/Config code that was parsed but never had runtime effectwarnLocalConfigOverridesto warn about per-agentAuthzinstead of the removed fieldsTest plan
task fmt && task lint— 0 issuestask test— all tests pass with race detectoragents.claude-code.mcp.authz.profile: observeparses correctly and the tighter profile propagatesBackward compatibility
Go's
yaml.v3silently drops unknown fields. Users with existing per-agentmcp.group/mcp.port/mcp.configin YAML will see those fields silently ignored — same as today since they never had runtime effect. The Go API change (AgentOverride.MCPtype) is breaking for external consumers, acceptable at v0.0.x.Fixes #62
🤖 Generated with Claude Code