Add engine.permission-mode as a first-class Claude engine config field#33658
Add engine.permission-mode as a first-class Claude engine config field#33658Copilot wants to merge 5 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…de engine - Add PermissionMode field to EngineConfig struct - Extract permission-mode from frontmatter in ExtractEngineConfig (both standard and inline-definition paths) - Add validateEnginePermissionMode that accepts: auto, acceptEdits, plan, bypassPermissions - Wire validation in compiler_orchestrator_workflow.go and compiler_string_api.go - Update GetExecutionSteps in claude_engine.go to honor explicit permission-mode before falling back to bash-wildcard logic - Add permission-mode to main_workflow_schema.json (engine_config oneOf variants 1 and 2) - Regenerate docs/public/editor/autocomplete-data.json - Add tests for extraction, validation, and Claude engine override behavior Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
There was a problem hiding this comment.
Pull request overview
Adds engine.permission-mode as a first-class workflow frontmatter field (Claude-only) so users can explicitly control Claude Code CLI’s --permission-mode without relying on engine.args ordering, restoring effective --allowed-tools enforcement when the firewall injects bash: ["*"].
Changes:
- Adds
EngineConfig.PermissionMode, extracts it from frontmatter for both standard and inline engine definitions, and validates it against the allowed enum. - Updates Claude execution-step generation to prefer explicit
engine.permission-modeover the bash-wildcard heuristic defaulting. - Extends the JSON schema + editor autocomplete data to include
engine.permission-mode.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/engine.go | Adds PermissionMode to EngineConfig and extracts engine.permission-mode from frontmatter (inline + non-inline). |
| pkg/workflow/engine_validation.go | Adds validateEnginePermissionMode to enforce allowed values. |
| pkg/workflow/engine_validation_test.go | Adds validation tests for engine.permission-mode (currently needs gofmt/goimports). |
| pkg/workflow/engine_config_test.go | Adds extraction tests for engine.permission-mode (currently needs gofmt/goimports). |
| pkg/workflow/compiler_string_api.go | Wires permission-mode validation into the string parsing path. |
| pkg/workflow/compiler_orchestrator_workflow.go | Wires permission-mode validation into the file parsing path. |
| pkg/workflow/claude_engine.go | Prioritizes explicit permission-mode over bash-wildcard heuristic when building Claude CLI args. |
| pkg/workflow/claude_engine_test.go | Adds tests ensuring explicit permission-mode overrides heuristic selection. |
| pkg/parser/schemas/main_workflow_schema.json | Adds engine.permission-mode enum to engine config schema variants; includes large reformat due to regen. |
| docs/public/editor/autocomplete-data.json | Regenerated autocomplete data to include engine.permission-mode (also includes other unrelated changes). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/10 changed files
- Comments generated: 4
| tests := []struct { | ||
| name string | ||
| workflow *WorkflowData | ||
| expectError bool | ||
| errorSubstr string | ||
| }{ | ||
| { | ||
| name: "nil workflow data", | ||
| workflow: nil, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "nil engine config", | ||
| workflow: &WorkflowData{}, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "permission-mode not set — no error", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude"}, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "valid mode: auto", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "auto"}, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "valid mode: acceptEdits", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "acceptEdits"}, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "valid mode: plan", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "plan"}, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "valid mode: bypassPermissions", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "bypassPermissions"}, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "invalid mode: unknown value", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "superuser"}, | ||
| }, | ||
| expectError: true, | ||
| errorSubstr: "invalid value", | ||
| }, | ||
| { | ||
| name: "invalid mode: case-mismatch", | ||
| workflow: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "claude", PermissionMode: "bypasspermissions"}, | ||
| }, | ||
| expectError: true, | ||
| errorSubstr: "invalid value", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| err := compiler.validateEnginePermissionMode(tt.workflow) | ||
|
|
||
| if tt.expectError { | ||
| require.Error(t, err, "Expected validation error") | ||
| if tt.errorSubstr != "" { | ||
| assert.Contains(t, err.Error(), tt.errorSubstr, "Expected error substring mismatch") | ||
| } | ||
| return | ||
| } | ||
|
|
||
| assert.NoError(t, err, "Expected permission-mode validation to pass") | ||
| }) | ||
| } | ||
| } |
| compiler := NewCompiler() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| frontmatter map[string]any | ||
| expectedMode string | ||
| }{ | ||
| { | ||
| name: "permission-mode auto", | ||
| frontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "id": "claude", | ||
| "permission-mode": "auto", | ||
| }, | ||
| }, | ||
| expectedMode: "auto", | ||
| }, | ||
| { | ||
| name: "permission-mode acceptEdits", | ||
| frontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "id": "claude", | ||
| "permission-mode": "acceptEdits", | ||
| }, | ||
| }, | ||
| expectedMode: "acceptEdits", | ||
| }, | ||
| { | ||
| name: "permission-mode plan", | ||
| frontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "id": "claude", | ||
| "permission-mode": "plan", | ||
| }, | ||
| }, | ||
| expectedMode: "plan", | ||
| }, | ||
| { | ||
| name: "permission-mode bypassPermissions", | ||
| frontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "id": "claude", | ||
| "permission-mode": "bypassPermissions", | ||
| }, | ||
| }, | ||
| expectedMode: "bypassPermissions", | ||
| }, | ||
| { | ||
| name: "permission-mode not set — empty string", | ||
| frontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "id": "claude", | ||
| }, | ||
| }, | ||
| expectedMode: "", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, config := compiler.ExtractEngineConfig(tt.frontmatter) | ||
| if config == nil { | ||
| t.Fatal("Expected non-nil config") | ||
| } | ||
| if config.PermissionMode != tt.expectedMode { | ||
| t.Errorf("PermissionMode = %q, want %q", config.PermissionMode, tt.expectedMode) | ||
| } | ||
| }) | ||
| } | ||
| } |
| "pull_request_reviewer": { | ||
| "type": "null|string", | ||
| "desc": "Experimental synthetic reviewer lifecycle trigger.", | ||
| "leaf": true | ||
| }, |
| // An explicit engine.permission-mode config field overrides both defaults. | ||
| var permissionMode string | ||
| if workflowData.EngineConfig != nil && workflowData.EngineConfig.PermissionMode != "" { | ||
| permissionMode = workflowData.EngineConfig.PermissionMode | ||
| claudeLog.Printf("Using explicit permission mode from config: %s", permissionMode) | ||
| } else if hasBashWildcardInTools(workflowData.Tools) { | ||
| claudeLog.Print("Unrestricted bash detected: using bypassPermissions mode") | ||
| permissionMode = "bypassPermissions" | ||
| } else { | ||
| permissionMode = "acceptEdits" | ||
| } | ||
| claudeArgs = append(claudeArgs, "--permission-mode", permissionMode) | ||
|
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Complete ✅
This PR successfully adds engine.permission-mode as a first-class configuration field for the Claude engine. The implementation is clean, well-tested, and properly integrated.
Strengths:
- Consistent extraction in both inline and non-inline engine paths
- Proper validation with helpful error messages
- Smart defaulting: explicit config → bash wildcard heuristic → safe default
- Comprehensive test coverage
No issues found — ready to merge.
🔎 Code quality review by PR Code Quality Reviewer · ● 8.1M
|
✅ PR Code Quality Reviewer completed the code quality review. |
This comment has been minimized.
This comment has been minimized.
|
🧪 Test Quality Sentinel completed test quality analysis. |
Draft generated by Design Decision Gate from PR #33658 diff. Author must review and finalize before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review Summary
This PR successfully adds engine.permission-mode as a first-class config field with proper extraction, validation, and schema updates. The implementation is well-tested and follows established patterns.
Critical Issue Found:
- Args ordering bug: The explicit
permission-modecan still be overridden byengine.argsdue to flag ordering (custom args are appended after the explicit flag). This undermines the goal of providing deterministic precedence.
Already Noted (from previous reviews):
- Test formatting issues in
engine_validation_test.goandengine_config_test.go(will fail CI) - Unrelated autocomplete changes in
autocomplete-data.json
What's Done Well:
✅ Clean validation with helpful error messages
✅ Comprehensive test coverage for both extraction and validation
✅ Proper schema updates with enum constraints
✅ Good documentation in comments
Please address the args ordering issue to ensure the explicit config has true precedence over any engine.args override.
🔎 Code quality review by PR Code Quality Reviewer · ● 4.3M
Comments that could not be inline-anchored
pkg/workflow/claude_engine.go:214
The explicit engine.permission-mode is set at line 198, but engine.args are appended at line 214 after the permission-mode flag. This means a user can still override the explicit setting via engine.args: ["--permission-mode", "bypassPermissions"], reintroducing the last-flag-wins fragility this PR aims to eliminate.
Suggested fix: Either:
- Append
--permission-modeafter custom args (move lines 198 to after line 215), or - Filter/remove any
--permission-modefrom `Engin…
🧪 Test Quality Sentinel ReportTest Quality Score: 70/100
Test Classification Details
📊 Detailed Test Analysis✅
|
| Test File | Production File | Test Lines | Prod Lines | Ratio |
|---|---|---|---|---|
| claude_engine_test.go | claude_engine.go | +54 | +9 | 6.0:1 |
| engine_config_test.go | engine.go | +72 | +18 | 4.0:1 |
| engine_validation_test.go | engine_validation.go | +89 | +20 | 4.45:1 |
Why this matters: High test-to-production ratios can indicate:
- Table-driven tests with many cases (legitimate)
- Excessive setup boilerplate (refactorable)
- Duplicate test scenarios (consolidatable)
In this PR: The inflation is primarily due to table-driven tests with comprehensive test cases (4-9 cases per function). This is the preferred pattern in this codebase and represents good coverage rather than wasteful duplication. However, the ratio is notably high.
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 3 tests — all unit tests (//go:build !integration)
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%).
Score breakdown:
- Behavioral coverage: 40/40 pts (100% design tests)
- Error/edge case coverage: 10/30 pts (33% have error cases)
- Low duplication: 20/20 pts (no duplicates)
- Proportional growth: 0/10 pts (test inflation penalty)
Recommendations:
- Add error cases to
TestClaudeEnginePermissionModeExplicitOverride(nil EngineConfig, invalid mode, malformed tools) - Convert stdlib assertions in
TestEnginePermissionModeExtractionto testify for consistency - Consider edge cases for type mismatches in extraction/validation tests
Despite the inflation ratio, the tests provide strong behavioral coverage of the new permission-mode feature. The table-driven pattern with multiple cases is appropriate and follows project conventions.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
🧪 Test quality analysis by Test Quality Sentinel · ● 14.1M · ◷
The network egress firewall injects
bash: ["*"], which triggered thehasBashWildcardInToolsheuristic and silently forced--permission-mode bypassPermissions, making--allowed-toolsenforcement ineffective. The only override was a fragileengine.argsworkaround relying on Claude CLI's last-flag-wins behavior.Changes
EngineConfigstruct — addsPermissionMode stringfield; extracted from frontmatterengine.permission-modein both standard and inline-definition code pathsvalidateEnginePermissionModerejects any value outside{auto, acceptEdits, plan, bypassPermissions}; wired into both compiler orchestrator pathsclaude_engine.go— explicitpermission-modetakes priority over the bash-wildcard auto-selection; fallback behavior is unchangedpermission-modeadded with anenumconstraint toengine_configvariants 1 and 2 inmain_workflow_schema.json;autocomplete-data.jsonregeneratedUsage
Setting
permission-mode: auto(oracceptEdits) with the firewall enabled now keeps--allowed-toolsenforcement active, decoupling tool-boundary policy from the firewall's bash wildcard injection.