Decouple Claude permission mode from bash wildcard and add first-class engine.permission-mode#34525
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
engine.permission-mode
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR makes Claude’s permission mode an explicit engine setting (engine.permission-mode) rather than implicitly deriving it from bash wildcard detection, to avoid unintentionally forcing bypassPermissions and weakening --allowed-tools enforcement. It also deduplicates --permission-mode CLI flags by stripping legacy engine.args forms when the new setting is used (or when legacy args provide the mode).
Changes:
- Added
EngineConfig.PermissionModeparsing forengine.permission-modeand schema validation (auto | acceptEdits | plan | bypassPermissions). - Updated Claude engine permission-mode selection defaults (decoupled from bash wildcards;
tools.edit=falsedefaults toauto; explicitengine.permission-modeoverrides). - Ensured Claude emits exactly one
--permission-modeflag by stripping user-supplied permission-mode flags fromengine.args(while preserving legacy override behavior).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/engine.go | Adds PermissionMode to EngineConfig and parses engine.permission-mode. |
| pkg/workflow/engine_args_test.go | Adds unit tests for extracting engine.permission-mode. |
| pkg/workflow/claude_engine.go | Updates permission-mode selection logic and strips legacy --permission-mode flags from engine.args. |
| pkg/workflow/claude_engine_test.go | Expands tests for permission-mode selection and arg stripping helpers. |
| pkg/parser/schemas/main_workflow_schema.json | Adds schema enum for engine.permission-mode and allows boolean tools.edit. |
| pkg/parser/schema_test.go | Adds schema validation tests for engine.permission-mode and tools.edit: false. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/claude_engine.go:547
stripClaudePermissionModeArgstreats any token following--permission-modeas the mode value, even if it looks like another flag. For input like["--permission-mode", "--foo", "bar"]this will (1) set permissionMode to "--foo" and (2) drop the real--fooflag from the args, which is unintended. Consider only consuming the next token when it is a non-empty value that does not start with-(or otherwise validate against the known permission-mode enum) and treat other cases as a dangling flag so subsequent args are preserved.
switch {
case arg == "--permission-mode":
if i+1 < len(args) {
permissionMode = args[i+1]
i++
}
pkg/workflow/claude_engine_test.go:405
- Test coverage for
stripClaudePermissionModeArgsdoesn’t cover the case where--permission-modeis followed by another flag (e.g.--permission-mode --foo bar). With the current implementation this would incorrectly treat--fooas the mode value and drop it from the filtered args. Adding a test case for that scenario would prevent regressions once the parsing logic is tightened.
name: "dangling permission mode flag is removed",
inputArgs: []string{"--foo", "--permission-mode"},
expectedArgs: []string{"--foo"},
expectedPermission: "",
},
- Files reviewed: 6/6 changed files
- Comments generated: 3
| // Default to "acceptEdits" so Claude Code honours --allowed-tools as the effective | ||
| // MCP tool boundary. Workflows that explicitly set tools.edit=false default to | ||
| // "auto" because they don't rely on acceptEdits write auto-approval behavior. | ||
| permissionMode := "acceptEdits" | ||
| if hasBashWildcardInTools(workflowData.Tools) { | ||
| claudeLog.Print("Unrestricted bash detected: using bypassPermissions mode") | ||
| permissionMode = "bypassPermissions" | ||
| if isEditToolExplicitlyDisabled(workflowData.Tools) { | ||
| claudeLog.Print("tools.edit=false detected: using auto permission mode") | ||
| permissionMode = "auto" | ||
| } |
| // Extract optional 'permission-mode' field | ||
| if permissionMode, hasPermissionMode := engineObj["permission-mode"]; hasPermissionMode { | ||
| if permissionModeStr, ok := permissionMode.(string); ok { | ||
| config.PermissionMode = permissionModeStr | ||
| } | ||
| } |
| expectedMode: "acceptEdits", | ||
| notExpectedMode: "bypassPermissions", | ||
| }, | ||
| { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good structural improvement — decoupling permission mode from bash wildcard detection removes an implicit security-by-side-effect pattern and replaces it with an explicit, schema-validated field. The three-tier priority (edit-disabled default → engine.permission-mode → legacy args stripping) is well thought-out and the test coverage for stripClaudePermissionModeArgs and isEditToolExplicitlyDisabled is thorough.
Observations
Maintainability
- Fragile slice-index mutation (
permissionModeValueIndex): the index-capture + in-place-update pattern is brittle; extracting permission mode resolution into a helper before appending toclaudeArgswould eliminate the risk.
Test coverage gap
bash: nilbehavioral change is untested: the old case was removed without a replacement, leaving the newacceptEditsdefault for unrestricted-bash workflows undocumented in the test suite.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.2M
| if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Args) > 0 { | ||
| claudeArgs = append(claudeArgs, workflowData.EngineConfig.Args...) | ||
| // stripClaudePermissionModeArgs returns an empty permission-mode string when no | ||
| // override flag is present. |
There was a problem hiding this comment.
Fragile index-based slot mutation: capturing permissionModeValueIndex and mutating the slice in-place later is brittle — any reordering of append calls between here and the mutation silently breaks the logic.
💡 Suggested refactor
Finalize the permission mode value before appending it to claudeArgs, eliminating the index bookkeeping entirely:
// Resolve the final permission mode from all priority layers before emitting.
permissionMode := resolvePermissionMode(workflowData, defaultPermissionMode)
claudeArgs = append(claudeArgs, "--permission-mode", permissionMode)
// No index needed — no later mutation.A small helper resolvePermissionMode(wd, defaultMode) string can encode the three-tier precedence (edit-disabled default → engine.permission-mode → legacy engine.args flag), making the priority chain independently testable and impossible to accidentally break by inserting new append calls between the capture and the mutation.
| "edit": false, | ||
| }, | ||
| engineConfig: &EngineConfig{ | ||
| PermissionMode: "acceptEdits", |
There was a problem hiding this comment.
Missing regression test for bash: nil behavioral change: the old "bash nil value (unrestricted) — bypassPermissions" test case was removed but no test replaces it, leaving the new acceptEdits behavior for bash: nil (unrestricted bash) entirely uncovered.
💡 Suggested addition
Add a test case to the table to document the new behavior explicitly:
{
name: "bash nil (unrestricted) now uses acceptEdits",
tools: map[string]any{
"bash": nil,
},
expectedMode: "acceptEdits",
notExpectedMode: "bypassPermissions",
},This guards against accidentally re-introducing the old bypass logic in a future patch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (314 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you — especially for security-relevant decisions like decoupling permission-mode selection from unrelated tool configuration. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /grill-with-docs — requesting changes on a few targeted issues.
📋 Key Themes & Highlights
Issues Found
permissionModeValueIndexfragility (claude_engine.go:190) — index-based mutation ofclaudeArgsis brittle across future edits- Missing
bash: niltest — the removed test case has no replacement documenting the newacceptEditsdefault PermissionModeon genericEngineConfig— Claude-specific field silently accepted and ignored for other engines- Schema description missing security note —
bypassPermissionsin the schema enum has no warning about--allowed-toolsbypass - Dead code + stale docstring —
hasBashWildcardInTools()is no longer called from production code but its docstring contradicts this PR's changes
Stale documentation (not in diff, needs follow-up)
AGENTS.md lines 715 and 721 still describe the old auto-selection behaviour:
- "This mode is only used when the workflow grants unrestricted bash access (e.g.,
bash: "*")" - "
hasBashWildcardInTools()inclaude_tools.godetermines which mode is selected"
Both statements are now incorrect. The security model section needs updating to reflect that bypassPermissions is only reached via the explicit engine.permission-mode field.
Positive Highlights
- ✅ Excellent test coverage for the new
stripClaudePermissionModeArgshelper - ✅ The
TestIsEditToolExplicitlyDisabledtable is thorough, including edge cases (edit: nil,edit: "false"as string) - ✅ The "exactly one
--permission-modeflag" assertion is a nice regression guard - ✅ Backward-compatibility for legacy
engine.argsis well thought-out
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.2M
| claudeLog.Printf("Using engine.permission-mode override: %s", permissionMode) | ||
| } | ||
| claudeArgs = append(claudeArgs, "--permission-mode", permissionMode) | ||
| permissionModeValueIndex := len(claudeArgs) - 1 |
There was a problem hiding this comment.
[/zoom-out] permissionModeValueIndex tracks a slice index to mutate claudeArgs later. If any code is inserted between the append at line 189 and the mutation at line 212, the index silently points to the wrong position.
💡 Suggested alternative: resolve permission mode before appending
Resolve all overrides before the single append, eliminating the index dependency:
// Resolve permission mode (engine config takes precedence, then legacy args, then default)
var legacyEngineArgs []string
if workflowData.EngineConfig != nil {
if workflowData.EngineConfig.PermissionMode != "" {
permissionMode = workflowData.EngineConfig.PermissionMode
} else if len(workflowData.EngineConfig.Args) > 0 {
stripped, modeFromArgs := stripClaudePermissionModeArgs(workflowData.EngineConfig.Args)
legacyEngineArgs = stripped
if modeFromArgs != "" {
permissionMode = modeFromArgs
}
}
}
claudeArgs = append(claudeArgs, "--permission-mode", permissionMode)
// ... later ...
claudeArgs = append(claudeArgs, legacyEngineArgs...)This makes the data-flow explicit and removes the implicit index contract.
| expectedMode: "bypassPermissions", | ||
| expectedMode: "acceptEdits", | ||
| notExpectedMode: "bypassPermissions", | ||
| }, |
There was a problem hiding this comment.
[/tdd] The old test case "bash nil value (unrestricted) — bypassPermissions" was removed without a replacement. bash: nil previously forced bypassPermissions; it should now fall through to acceptEdits — but there is no test documenting this intent.
💡 Suggested test case to add
{
name: "bash nil value no longer forces bypassPermissions",
tools: map[string]any{
"bash": nil,
},
expectedMode: "acceptEdits",
notExpectedMode: "bypassPermissions",
},This is a behavioural change for existing workflows that relied on bash: null implicitly enabling bypassPermissions. Covering it explicitly prevents a future refactor from silently restoring the old behaviour.
| ID string | ||
| Version string | ||
| Model string | ||
| PermissionMode string |
There was a problem hiding this comment.
[/zoom-out] PermissionMode is a Claude-specific concept added to the shared EngineConfig struct. For Copilot, Codex, or custom engines, this field will be silently parsed from frontmatter and silently ignored — no validation error is emitted.
💡 Options to consider
- Add engine-specific validation in the compiler: emit a warning or error when
permission-modeis set for a non-Claude engine. - Move it to a
ClaudeEngineConfigsub-struct — if the Claude engine already has engine-specific config, that's the right home. - Document the scope in a comment on the field:
// PermissionMode is only meaningful for the Claude engine; ignored by all other engines.
Option 3 is the lowest-effort fix; option 2 is the most architecturally clean.
| }, | ||
| "permission-mode": { | ||
| "type": "string", | ||
| "enum": ["auto", "acceptEdits", "plan", "bypassPermissions"], |
There was a problem hiding this comment.
[/grill-with-docs] The schema description for permission-mode doesn't mention the security implications of bypassPermissions. A workflow author who sets engine.permission-mode: bypassPermissions may not realise this causes Claude Code to silently ignore --allowed-tools, making the MCP gateway allowed: filter the sole enforcement boundary.
💡 Suggested description update
"description": "Claude permission mode override. Defaults to acceptEdits (or auto when tools.edit is false). Use bypassPermissions with caution: it disables --allowed-tools enforcement; the MCP gateway allowed: filter becomes the sole tool boundary."This mirrors the security model documented in AGENTS.md and helps authors make an informed choice.
| return defaultGetSquidLogsSteps(workflowData, claudeLog) | ||
| } | ||
|
|
||
| func isEditToolExplicitlyDisabled(tools map[string]any) bool { |
There was a problem hiding this comment.
[/zoom-out] hasBashWildcardInTools() in pkg/workflow/claude_tools.go is no longer called from production code — it's only referenced in its own test (claude_engine_tools_test.go). Its docstring still says --permission-mode bypassPermissions is safe and produces a smoother headless experience, which directly contradicts this PR's intent.
Consider either removing the function (and its test) or updating its docstring to reflect that it is no longer used for permission-mode selection. Leaving dead code with a stale security-oriented comment creates confusion for future contributors.
|
|
|
|
|
|
|
@copilot merge main, recompile, review all comments from copilot and GitHub-actions |
…ission-mode-from-bash-wildcard Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in b1d3edb. I merged
Targeted workflow tests and Claude wasm golden checks pass, and parallel validation completed with no review findings. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 5e9e923. I reproduced the failing CGO job tests locally, updated stale Codex test expectations (anchored include_only patterns and web-fetch assertion), and re-ran |
|
|
|
|
Claude permission mode was implicitly derived from bash wildcard detection, which forced
bypassPermissionsand could nullify--allowed-toolsenforcement in firewall-enabled workflows. This change makes permission mode an explicit, validated engine setting and separates it from tool wildcard expansion.Config surface
engine.permission-modeparsing inEngineConfig(auto | acceptEdits | plan | bypassPermissions).engine.permission-mode.tools.edit: true|false.Claude permission-mode selection
bypassPermissionsbased on bash wildcard presence.acceptEdits.tools.edit: falseand no explicit override is set, default becomesauto.engine.permission-modetakes precedence over defaults.CLI arg emission correctness
--permission-modeflag.engine.argspermission-mode flags (--permission-mode Xand--permission-mode=X) to avoid duplicate flags.engine.permission-modeis unset, legacyengine.argscan still supply the mode.Example
Result: compiled Claude args include a single explicit
--permission-mode auto, and bash wildcard no longer forcesbypassPermissions.pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/26373384298
pr-sous-chef: synced the branch in run https://github.com/github/gh-aw/actions/runs/26374674857