fix(agents): Bake plugins into Docker image at build time

[dpup]

Summary

Plugins are now installed during Docker image build rather than at container startup, eliminating startup
latency and ensuring consistent images. This PR also adds SSH grant support for marketplace access and
improves error handling.

Key Changes

Plugin Baking at Build Time

• Plugins defined in agent.yaml are installed via claude plugin marketplace add and claude plugin install during docker build
• Eliminates the "Failed to install Anthropic marketplace" error at startup
• Plugins are cached in Docker layers for faster rebuilds
• Use --rebuild to update plugins after changing configuration

SSH Grant for Marketplace Access

The Anthropic plugin marketplace uses git@github.com:... URLs which require SSH authentication:

grants:
  - ssh:github.com  # Required for marketplace access

Added documentation for the "Failed to install Anthropic marketplace" error with clear fix instructions.

Plugin Configuration in agent.yaml

  claude:
    plugins:
      "plugin-name@marketplace": true
    marketplaces:
      marketplace-name:
        source: github
        repo: owner/repo

Input Validation & Error Handling

• Added regex validation for plugin names and marketplace repos to prevent command injection
• Dockerfile commands now include actionable error messages:
  RUN claude plugin install name@market || (echo 'Failed to install plugin. Verify it exists.' && exit 1)

Code Organization

• Moved Claude-specific Dockerfile logic to internal/claude/dockerfile.go
• MarketplaceConfig struct now lives in the claude package
• Removed dead code (GeneratePluginsList, GenerateMarketplacesList)

Runtime Configuration

• installed_plugins.json generated to tell Claude Code where plugins are located
• settings.json updated with marketplace directory paths
• Host plugins from ~/.claude/settings.json are NOT baked in (applied at runtime only)

[claude]

📦 Previous Review (superseded by newer review)

Code Review: Plugin Baking at Build Time

This PR introduces an architectural improvement by moving plugin installation from runtime to build time, which eliminates startup latency and ensures consistent images. The implementation is solid with good security practices and comprehensive testing. Here's my detailed review:

✅ Strengths

1. Security - Input Validation
The implementation includes proper validation to prevent command injection:

• validMarketplaceRepo regex at internal/claude/dockerfile.go:22 validates repo formats
• validPluginKey regex at internal/claude/dockerfile.go:26 validates plugin names
• Tests at internal/claude/dockerfile_test.go:64-103 verify malicious inputs are rejected
• Invalid inputs trigger build failures rather than silently executing

2. Code Organization

• Clean separation of concerns with new internal/claude/dockerfile.go module
• Moved Claude-specific Dockerfile logic out of the generic deps package
• Clear distinction between build-time (agent.yaml) and runtime (host settings) configuration

3. Error Handling

• Actionable error messages in Dockerfile commands (lines 59, 74 in dockerfile.go)
• Early validation with clear feedback when plugins fail to install
• Good use of || (echo '...' && exit 1) pattern for debugging

4. Test Coverage

• Comprehensive unit tests for Dockerfile generation
• Tests for input validation and edge cases
• E2E tests verify integration without requiring actual plugin repos
• Tests verify deterministic output (sorting)

5. Documentation

• Updated docs clearly explain the SSH grant requirement
• Troubleshooting section added for common errors
• agent.yaml reference updated with build-time caching behavior
• Examples updated with working configurations

🔍 Areas for Consideration

1. Image Tagging and Cache Invalidation
internal/deps/builder.go:940-950 correctly includes plugins in the image hash, ensuring different plugin configurations produce different images. However, there's a potential issue:

• Plugin versions are not tracked in the hash
• If a plugin is updated in a marketplace, --rebuild is required to get the new version
• Consider documenting this behavior more prominently or adding version tracking

2. Error Recovery - Partial Builds
At internal/claude/dockerfile.go:56 and 71, invalid inputs cause build failures with exit 1. This is correct, but consider:

• If one plugin in a list of 10 is invalid, the entire build fails
• Users might prefer warnings for optional plugins vs hard failures
• Current behavior is safer (fail-fast), but could add an option for lenient mode if needed

3. Regex Validation - Potential Edge Case
validMarketplaceRepo at line 22 allows periods, which could match `.." for path traversal:

var validMarketplaceRepo = regexp.MustCompile(`^[a-zA-Z0-9._@:/-]+$`)

While this is passed to claude plugin marketplace add (not directly to shell), consider:

• Stricter validation (e.g., require [a-z0-9-]+/[a-z0-9-]+ for GitHub format)
• Or add explicit blocklist for .. sequences
• Current implementation is likely safe but could be more restrictive

4. Plugin Discovery and Validation
The code doesn't validate that marketplaces or plugins actually exist before building:

• Build fails at Docker build time, not at validation time
• Users don't discover typos until after starting the build
• Consider adding a --dry-run or pre-validation step

5. Mount Path Consistency
ClaudeMarketplacesPath at internal/claude/generate.go:280 hardcodes /home/moatuser/.claude/plugins/marketplaces, but this assumes:

• Container user is always "moatuser"
• The path matches what claude plugin expects
• Consider passing containerUser through or making this configurable

6. Documentation - SSH Grant Timing
The docs at docs/content/guides/01-running-claude-code.md:18-38 correctly explain SSH grants are needed, but could clarify:

• SSH grants are needed at build time not just runtime
• If you add plugins later, you need --rebuild AND the SSH grant
• Sequence matters: grant first, then build

🐛 Potential Issues

1. installed_plugins.json Path Assumption
internal/claude/generate.go:457 constructs paths like:

installPath := filepath.Join(cacheMountPath, "marketplaces", marketplace, "plugins", pluginName)

This assumes marketplace structure is {marketplace}/plugins/{plugin}. Verify all Claude Code marketplaces follow this convention. If some use different structures, this will fail silently.

2. parsePluginKey Edge Case
internal/claude/generate.go:476 uses LastIndexByte to split "plugin@marketplace":

idx := strings.LastIndexByte(key, '@')

Test case shows plugin@market@extra → plugin="plugin@market", marketplace="extra". While tested, this might be unexpected behavior. Consider rejecting multiple @ symbols as invalid.

3. Missing GIT_SSL_CAINFO in Tests
internal/run/manager.go:1329 adds GIT_SSL_CAINFO for proxy CA cert, but I don't see tests verifying this env var is set correctly when plugins are configured. Plugin marketplace cloning might fail without it.

4. Dead Code - SSH Access Validation
The PR removes SSH access validation that was at runtime (see manager.go line 1453-1459 in diff). While plugins are now baked at build time, SSH grants are still needed. Ensure build-time SSH access validation happens elsewhere, or document that Docker build failures provide sufficient feedback.

📊 Performance Considerations

Positive:

• Eliminates startup latency (plugins no longer installed at runtime)
• Docker layer caching speeds up rebuilds when plugins don't change
• Deterministic image tags enable proper caching

Concerns:

• Large plugin marketplaces increase image size
• No lazy loading - all enabled plugins are baked in
• Consider documenting image size implications for users with many plugins

🔐 Security Assessment

Strong points:

• Input validation prevents command injection
• Read-only mounts for marketplace directories
• Separation between agent.yaml (baked) and host settings (runtime)
• No credential exposure in Docker layers

Recommendations:

• Consider stricter regex for repo names (currently allows dots and underscores)
• Document that plugin code becomes part of the trusted image
• Users should review plugins before enabling (supply chain risk)

✨ Best Practices Adherence

Follows CLAUDE.md guidelines:

• ✅ Error messages are actionable ("Grant SSH access", "Verify plugin exists")
• ✅ Documentation matches actual behavior
• ✅ Uses conventional commits format
• ✅ Comprehensive test coverage
• ✅ No Co-Authored-By lines

Code style:

• ✅ Follows Go conventions
• ✅ Good use of sorting for deterministic output
• ✅ Clear comments explaining why not just what

📝 Recommendations

High Priority:

1. Strengthen validMarketplaceRepo regex to reject .. sequences
2. Add pre-build validation to catch typos early
3. Document plugin versioning behavior and --rebuild requirements
4. Add test for GIT_SSL_CAINFO env var with plugins + proxy

Medium Priority:
5. Consider rejecting plugins with multiple @ symbols
6. Add --dry-run flag to validate configuration without building
7. Document image size implications in the troubleshooting guide

Low Priority:
8. Consider lenient mode for optional plugins (currently strict fail-fast)
9. Add plugin version tracking in image hash for better cache invalidation
10. Make container paths configurable rather than hardcoded

🎯 Overall Assessment

This is a well-designed and thoroughly tested improvement. The architectural shift from runtime to build time is the right approach, and the security measures are appropriate. The code quality is high, documentation is thorough, and the changes align well with the project's conventions.

Recommendation: Approve with minor suggestions

The identified issues are mostly edge cases or future enhancements. The core implementation is solid and ready to merge. Consider addressing the high-priority recommendations in a follow-up PR if not in this one.

Testing Suggestions

Before merging, verify:

1. go test ./... passes
2. go test -tags=e2e ./internal/e2e/ passes
3. Manual test: Create agent.yaml with invalid plugin name, verify build fails with clear error
4. Manual test: Run with real plugins to ensure SSH grant + marketplace access works end-to-end

[claude]

📦 Previous Review (superseded by newer review)

Code Review: Bake plugins into Docker image at build time

This PR makes a significant architectural improvement by moving plugin installation from container startup to Docker image build time. The implementation is well-designed with good security considerations and comprehensive testing.

Strengths

Architecture & Design

• Clear separation of concerns: The new internal/claude/dockerfile.go properly encapsulates Claude-specific Dockerfile generation logic
• Smart plugin filtering: Only plugins from agent.yaml are baked into images (line 803-840 in manager.go), keeping host settings separate - this is correct behavior
• Deterministic builds: Marketplace and plugin sorting ensures consistent Dockerfile output for better layer caching

Security

• Command injection prevention: Excellent regex validation for marketplace repos (validMarketplaceRepo) and plugin names (validPluginKey) in dockerfile.go:22,25
• Safe error handling: Invalid inputs trigger build failures with clear error messages rather than being silently executed
• Proper credential isolation: The comment in dockerfile.go:54-56 correctly validates repo format before use

Testing

• Comprehensive unit tests: dockerfile_test.go covers happy paths, empty inputs, and malicious injection attempts
• E2E tests: New tests verify the integration without requiring actual network access (smart approach)
• Edge case coverage: Tests verify invalid formats like bad;rm -rf /@market are properly rejected

Documentation

• Clear troubleshooting guide: The new "Failed to install Anthropic marketplace" section provides actionable steps
• Updated reference docs: agent.yaml documentation correctly notes that plugins are cached in Docker layers

Issues & Concerns

1. Security: GIT_SSL_CAINFO addition (manager.go:1324)

The GIT_SSL_CAINFO env var is added for marketplace cloning during build but without explanation. Consider adding a comment explaining why this is needed and ensure it doesn't expose users to MITM attacks if the proxy is compromised.

2. Potential command injection via containerUser parameter

In dockerfile.go:31,39-40, the containerUser parameter is not validated. While current callers use hardcoded "moatuser", consider adding validation or making it a constant, plus a comment documenting the safety assumption.

3. Error message inconsistency

In dockerfile.go:56,71, error messages only show the name, not the malicious value. While good for security, it makes debugging legitimate typos harder. Consider logging the invalid value to stderr during validation before the build starts.

4. URL parsing logic fragility (manager.go:816-823)

URL parsing only handles GitHub HTTPS URLs. What about SSH URLs, other Git providers, or URLs with query parameters? Consider using url.Parse or documenting that only GitHub HTTPS URLs are automatically converted.

5. parsePluginKey uses LastIndexByte (generate.go:195)

The comment explains this is intentional for plugin@market@extra, but this behavior seems odd. If plugin names containing @ aren't a real use case, using regular IndexByte would be clearer and catch malformed input.

6. Missing validation in image tag hash

In builder.go:54-63, plugins are included in the image hash without validation. Defense in depth suggests validating before including in hash computation.

Minor Issues

7. go install version handling edge case (install.go:1152-1158)

This fix looks correct but is unrelated to the plugin work. Should it be a separate commit? Also, what happens with versions like v1.2.3 passed by users?

8. Unused test helper (e2e_test.go:1277-1282)

createTestWorkspaceWithClaudePlugins is defined but never used. Either use it or remove it.

9. Comment formatting (moat-init.sh:1169-1171)

Good comment, but consider explaining what happens to old containers/images that might still have marketplaces.txt and plugins.txt files.

Performance Considerations

Positive:

• Eliminates startup latency (main goal achieved!)
• Docker layer caching makes rebuilds fast when plugins don't change
• Plugins are sorted for deterministic layer caching

Potential concerns:

• Every plugin config change requires image rebuild (documented with --rebuild flag)
• Large plugins increase image size (acceptable trade-off)
• Sequential RUN commands vs single RUN with && chaining for better caching

Test Coverage Gaps

1. No test for concurrent builds
2. No test for SSH grant validation during build
3. Missing test for installed_plugins.json copying in moat-init.sh

Recommendations

High Priority:

1. Validate or document the containerUser parameter in GenerateDockerfileSnippet
2. Document the GIT_SSL_CAINFO environment variable addition
3. Add comment explaining URL parsing limitations

Medium Priority:
4. Consider using url.Parse for more robust URL handling
5. Remove or use the createTestWorkspaceWithClaudePlugins helper
6. Add test coverage for SSH grant validation during build

Low Priority:
7. Consider consolidating plugin RUN commands into a single layer
8. Document edge cases in version handling for go install

Verdict

This is a high-quality PR with thoughtful architecture, strong security practices, and excellent test coverage. The issues identified are mostly minor improvements and documentation gaps. The core implementation is solid.

Approved with suggestions for follow-up improvements.

---

Summary:

• ✅ Code quality: Excellent separation of concerns
• ✅ Security: Strong input validation, command injection prevention
• ✅ Tests: Comprehensive coverage
• ⚠️ Minor improvements needed: URL parsing robustness, containerUser validation
• ✅ Documentation: Clear and actionable

[claude]

📦 Previous Review (superseded by newer review)

Code Review: Plugin Baking at Build Time

This PR significantly improves the plugin system by baking plugins into Docker images at build time rather than installing them at runtime. The implementation is well-structured, thoroughly tested, and includes excellent documentation. Below is my detailed review.

---

Code Quality & Architecture ✅

Excellent separation of concerns:

• New internal/claude/dockerfile.go cleanly encapsulates Claude-specific Dockerfile logic
• Clear separation between agent.yaml config (baked into image) and host settings (runtime only)
• Proper abstraction with MarketplaceConfig struct in the claude package

Well-documented code:

• Comprehensive inline comments explaining design decisions (e.g., lines 28-32 in dockerfile.go about error message safety)
• Clear function documentation with usage notes
• Good example: GenerateDockerfileSnippet documents the containerUser parameter safety requirements (lines 37-40)

Clean refactoring:

• Removed 50+ lines of dead runtime installation code from run/manager.go:1084-1520
• Consolidated plugin path constants (ClaudePluginsPath, ClaudeMarketplacesPath)
• Eliminated obsolete staging files (marketplaces.txt, plugins.txt) with migration notes (moat-init.sh:1186-1191)

---

Security ✅

Strong input validation:

• validMarketplaceRepo regex prevents command injection in marketplace repos (dockerfile.go:22)
• validPluginKey regex validates plugin@marketplace format (dockerfile.go:26)
• Invalid inputs show error messages with name only, not the malicious value itself (dockerfile.go:66, 81)
• Good security design: prevents ; rm -rf / type attacks from appearing in Dockerfiles

Proper error handling:

• Dockerfile RUN commands include || (echo ... && exit 1) for clear failure messages
• Build-time validation ensures images only contain valid plugins
• SSH grant validation happens early (though now less critical since it's at build time)

Certificate trust improvements:

• Added GIT_SSL_CAINFO for Git HTTPS operations (manager.go:1347)
• Complements existing SSL_CERT_FILE, REQUESTS_CA_BUNDLE, NODE_EXTRA_CA_CERTS

---

Testing ✅

Comprehensive test coverage:

• Unit tests for all new functions (dockerfile_test.go, generate_test.go)
• Security-focused tests for injection prevention (dockerfile_test.go:335-375, dockerfile_test.go:1106-1155)
• Edge case coverage: empty inputs, invalid formats, disabled plugins
• E2E tests verify Dockerfile generation without requiring actual builds (e2e_test.go:1219-1287)

Good test design:

• Tests verify both positive cases (valid plugins) and negative cases (injection attempts)
• Deterministic output verified via sorting (dockerfile.go:52-56, 72-74)
• Tests confirm malicious content doesn't appear in generated output

---

Performance ✅

Significant runtime improvements:

• Eliminates startup latency from plugin installation
• Docker layer caching speeds up rebuilds
• Only changed plugins trigger rebuilds (hash includes plugin list - builder.go:948-961)

Efficient hashing:

• Plugins sorted before hashing for cache stability (builder.go:954-957)
• Hash includes plugin names in deterministic order

---

Documentation ✅

Excellent user-facing docs:

• Clear error message documentation (guides/01-running-claude-code.md:360-378)
• Updated reference docs explaining build-time behavior (reference/02-agent-yaml.md:52-60)
• Working example in examples/agent-claude with real marketplaces

Developer documentation:

• Updated CLAUDE.md would benefit from mentioning the build-time plugin approach
• Code comments explain implementation choices well

---

Issues & Suggestions

1. Minor: Redundant version check (install.go:406-410)

The condition at line 406 is redundant:

if dep.Version == "latest" {
    version = "latest"  // No-op assignment
} else {
    version = "v" + dep.Version
}

Suggestion: Simplify to:

if dep.Version \!= "latest" {
    version = "v" + dep.Version
}

2. Minor: Error message clarity (dockerfile.go:69, 84)

Error messages could be slightly more actionable. Current:

• "Failed to add marketplace %s. Check SSH grants..."
• "Failed to install plugin %s. Verify the plugin exists..."

Suggestion: Add the marketplace/plugin name to help users locate it:

• "Failed to add marketplace '%s' from repo. Check SSH grants..."
• "Failed to install '%s'. Verify it exists in the marketplace."

(Note: This is already pretty good, just a minor polish suggestion)

3. Documentation: CLAUDE.md update needed

The CLAUDE.md file mentions plugins but doesn't explain the build-time baking behavior. Users reading the architecture section would benefit from understanding:

• Plugins defined in agent.yaml are baked at build time
• Host plugins from ~/.claude/settings.json are NOT baked in
• Use --rebuild to update plugins after changing agent.yaml

4. Potential issue: marketplace extraction logic (manager.go:816-828)

The URL-to-repo conversion only handles GitHub HTTPS URLs:

if strings.HasPrefix(repo, "https://github.com/") {
    repo = strings.TrimPrefix(repo, "https://github.com/")
    repo = strings.TrimSuffix(repo, "/")
    repo = strings.TrimSuffix(repo, ".git")
}

Concern: Users might use SSH URLs like git@github.com:owner/repo.git in agent.yaml, which would be passed as-is to the claude plugin marketplace add command. Need to verify this works with the claude CLI.

Recommendation: Add a test case or comment confirming SSH URLs work as-is, OR add handling for SSH format.

5. containerUser parameter safety (dockerfile.go:39-40)

The comment states callers must ensure containerUser is safe, but there's no validation. While "moatuser" is hardcoded in current callers, defensive programming suggests validating it.

Suggestion: Add a simple check:

if \!regexp.MustCompile(`^[a-z][a-z0-9_-]*$`).MatchString(containerUser) {
    return "" // or panic - this is a programming error
}

6. parsePluginKey behavior (generate.go:198-200)

Uses LastIndexByte which means plugin@market@extra becomes plugin=plugin@market, marketplace=extra. The comment explains this is intentional for predictable behavior.

Observation: This is fine since invalid keys are filtered by the knownMarketplaces check. However, it might silently accept malformed input from user config. Consider logging a warning when multiple @ signs are detected.

---

Performance Considerations

Image size:

• Baking plugins increases image size slightly (plugin files included)
• Tradeoff is justified by eliminating runtime overhead
• Layer caching mitigates rebuild costs

Build time:

• Network calls during build (git clone marketplaces, install plugins)
• Acceptable tradeoff for faster startup
• Cached layers make subsequent builds fast

---

Best Practices ✅

Follows Go conventions:

• Proper error wrapping with fmt.Errorf
• Exported functions have godoc comments
• Consistent naming (GenerateDockerfileSnippet, GenerateInstalledPlugins)

Follows project conventions:

• Uses CLAUDE.md style guide (objective, factual tone)
• Conventional Commits would be: fix(agents): bake plugins into Docker image at build time
• No Co-Authored-By line ✅

Good error messages:

• Actionable: "Grant SSH access" with exact command to run
• Context-aware: Different messages for marketplace vs plugin failures

---

Summary

This is high-quality work with excellent architecture, security, testing, and documentation. The few issues noted are minor polish items, not blockers.

Recommendation: Approve with minor suggestions

Priority fixes before merge:

1. ✅ None - all critical concerns addressed

Nice-to-have improvements:

1. Simplify redundant version check (install.go:406)
2. Add CLAUDE.md architecture note about build-time plugins
3. Clarify/test SSH URL handling in marketplace extraction
4. Consider validating containerUser parameter

What I particularly liked:

• Security-first approach to validation
• Comprehensive test coverage including injection attempts
• Clear separation of build-time vs runtime config
• Excellent inline documentation of design decisions
• Working examples in examples/agent-claude

Great work on this feature! 🎉

[claude]

📦 Previous Review (superseded by newer review)

Pull Request Review: Bake plugins into Docker image at build time

This PR successfully moves Claude plugin installation from runtime to build time, improving startup performance and reliability. The implementation is well-structured with strong security considerations and comprehensive test coverage.

✅ Strengths

Architecture & Design

• Clean separation of concerns with new internal/claude/dockerfile.go for Dockerfile generation
• Correctly distinguishes between build-time plugin baking (agent.yaml only) and runtime config merging (includes host settings)
• Deterministic Dockerfile generation with sorted outputs ensures reproducible builds
• Plugin changes properly trigger new image builds via hash inclusion in ImageTag()

Security

• Excellent command injection prevention using regex validation
• Security-first error handling: invalid values show marketplace/plugin NAME in errors, not the malicious content itself
• Proper validation coverage in tests confirms malicious inputs are rejected
• Error messages fail the build explicitly with actionable guidance

Testing

• Comprehensive test coverage across all new functionality
• Validation tests explicitly check command injection scenarios
• Tests verify sorting for deterministic output
• E2E tests validate the full integration

Documentation

• Clear, actionable troubleshooting for "Failed to install Anthropic marketplace" error
• Documentation correctly describes behavior (plugins baked at build time, use --rebuild to update)
• Good inline comments explaining design decisions

🔍 Issues & Suggestions

1. Critical: Missing installed_plugins.json copy in moat-init.sh

The code generates installed_plugins.json in the staging directory (generate.go:84-87), but the init script does not copy it to the container.

Location: internal/deps/scripts/moat-init.sh:73-77

Problem: The comment says plugins are now baked at build time and old staging files are ignored, but GenerateInstalledPlugins() still creates this file. Either the init script should copy it to ~/.claude/plugins/installed_plugins.json or the comment in generate.go:77 is wrong (it says "copied by moat-init").

Impact: Plugins may not be recognized by Claude Code at runtime.

2. Security: containerUser parameter not validated

Location: internal/claude/dockerfile.go:40

The comment says "Callers must ensure this is a safe, validated value" but there is no validation. If a caller passes user-controlled input, it could inject Dockerfile commands. Only deps/dockerfile.go:997 passes hardcoded "moatuser", so this is safe TODAY.

Recommendation: Add validation with regex pattern for valid usernames or document this as an internal-only function.

3. Minor: URL parsing could be more robust

Location: internal/run/manager.go:823-826

Multiple trailing slashes could cause issues. Use strings.TrimRight(repo, "/") instead of single TrimSuffix.

4. Inconsistency: Error handling differs between Generate functions

GenerateInstalledPlugins() silently skips invalid plugin keys (line 161-164), while GenerateDockerfileSnippet() explicitly fails the build. Consider logging warnings about skipped plugins or validating earlier in config parsing.

🎯 Performance Considerations

• Positive: Plugin installation now cached in Docker layers - excellent for rebuild performance
• Positive: Sorting ensures layer cache hits when plugin order changes in config
• Watch: Each plugin change invalidates all layers after the first plugin RUN command

🔒 Security Assessment

Strong points:

• Command injection prevention is thorough and well-tested
• No credentials in Dockerfiles or built images
• Validation happens at multiple layers
• Error messages do not leak potentially malicious input

No critical vulnerabilities identified.

📊 Test Coverage Assessment

• Unit tests: ✅ Excellent
• Integration tests: ✅ Good
• E2E tests: ✅ Present
• Security tests: ✅ Injection scenarios covered

🎬 Recommendations

Must fix before merge:

1. ⚠️ Resolve the installed_plugins.json copy issue in moat-init.sh

Should fix:
2. Add validation or restrict access to GenerateDockerfileSnippet.containerUser parameter
3. Apply consistent plugin key validation in GenerateInstalledPlugins()

Nice to have:
4. Improve URL parsing robustness
5. Add logging for skipped invalid plugins

✨ Conclusion

This is a high-quality PR that significantly improves the plugin system. The architecture is sound, security considerations are thorough, and the code is well-tested. The main concern is the potential missing copy of installed_plugins.json in the init script - please verify this works as intended before merging.

Recommended action: Address issue #1, consider issues #2-3, then merge.

[claude]

Pull Request Review: Bake plugins into Docker image at build time

This is an excellent architectural improvement that addresses a real pain point by moving plugin installation from runtime to build time. The implementation is thorough, well-tested, and follows good security practices.

Strengths

1. Security-First Approach

• Regex validation prevents command injection (internal/claude/dockerfile.go:22-26)
• Error messages exclude invalid values
• Comprehensive validation tests (dockerfile_test.go:64-104)

2. Excellent Test Coverage

• Unit, integration, and E2E tests
• Both positive and negative scenarios

3. Clear Error Messages

• Actionable errors in Dockerfile
• Good troubleshooting documentation

4. Proper Code Organization

• Claude logic cleanly separated
• Removed dead code (-1828 lines)

5. Documentation Quality

• Accurate behavior descriptions
• Real-world examples
• Troubleshooting section

Suggestions for Improvement

1. containerUser Parameter (internal/claude/dockerfile.go:41)

Consider making containerUser a package constant since all callers use moatuser.

2. Plugin Hash (internal/deps/builder.go:61-67)

Invalid plugins are included in hash. Minor impact but could confuse rebuild behavior.

3. Test Isolation (internal/e2e/e2e_test.go:837-868)

TestKeychainKeyPersistence uses real keychain. Consider mocks for better isolation.

4. Edge Cases (internal/claude/dockerfile_test.go)

Add tests for empty strings, long strings, unicode characters.

Security Assessment

• Command injection prevention: Excellent
• Credential handling: Proper (no creds in images)
• Minor: containerUser parameter should be validated

Final Recommendation

APPROVED - Ready to merge with minor suggestions for follow-up.

Follow-up items:

1. Validate or constant-ize containerUser parameter
2. Improve test isolation for keychain tests
3. Document invalid plugin hash edge case
4. Add fuzzing tests long-term

Great work!