From ae2b2c0d4e35231173aaefc91940b237542058ac Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sun, 22 Feb 2026 17:21:23 +0900 Subject: [PATCH] fix: plan mode review gate and exec guard quoted-path false positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Interview guide now instructs LLM to transition to "review" instead of "executing", ensuring user approval via /plan start is required. - Guard logic extended to handle interviewing→review transitions with validation and plan display. - exec safety guard now uses quote-aware tokenizer so that quoted arguments like "/review skip-git-repo-check" are not mistaken for absolute file paths. Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop.go | 10 +++++----- pkg/agent/memory.go | 2 +- pkg/tools/shell.go | 42 ++++++++++++++++++++++++++++++++++++++--- pkg/tools/shell_test.go | 19 +++++++++++++++++++ 4 files changed, 64 insertions(+), 9 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 953c7aca9..a79362976 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -833,10 +833,10 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt // 5a. Auto-advance plan phases after LLM iteration postStatus := agent.ContextBuilder.GetPlanStatus() - if agent.ContextBuilder.HasActivePlan() && postStatus == "executing" { - // Intercept: if AI changed status to executing without user approval - // (from interviewing or review), validate and set to "review". - if preStatus == "interviewing" || preStatus == "review" { + if agent.ContextBuilder.HasActivePlan() && (postStatus == "executing" || postStatus == "review") { + // Intercept: if AI changed status to executing or review without user approval + // (from interviewing or review), validate and hold at "review". + if preStatus == "interviewing" || (preStatus == "review" && postStatus == "executing") { if err := agent.ContextBuilder.ValidatePlanStructure(); err != nil { _ = agent.ContextBuilder.SetPlanStatus("interviewing") logger.WarnCF("agent", "Reverted plan to interviewing: "+err.Error(), @@ -856,7 +856,7 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt }) } } - } else if agent.ContextBuilder.GetTotalPhases() == 0 { + } else if postStatus == "executing" && agent.ContextBuilder.GetTotalPhases() == 0 { // Safeguard: executing but no phases (shouldn't happen, but be safe). _ = agent.ContextBuilder.SetPlanStatus("interviewing") logger.WarnCF("agent", "Reverted plan to interviewing: no phases defined", diff --git a/pkg/agent/memory.go b/pkg/agent/memory.go index 05ad547f7..4cb825898 100644 --- a/pkg/agent/memory.go +++ b/pkg/agent/memory.go @@ -489,7 +489,7 @@ func (ms *MemoryStore) GetInterviewContext() string { sb.WriteString("- When you have enough information, use edit_file to add ## Phase, ## Commands, and ## Context sections BELOW the header block.\n") sb.WriteString("- Each step MUST use checkbox syntax: `- [ ] description`. The system parses checkboxes to track progress.\n") sb.WriteString("- Organize into 2-5 phases with 3-5 steps each.\n") - sb.WriteString("- After writing Phases, change `> Status: interviewing` to `> Status: executing` via edit_file.\n") + sb.WriteString("- After writing Phases, change `> Status: interviewing` to `> Status: review` via edit_file. The user must approve with /plan start before execution begins.\n") sb.WriteString("\n### Target Format (MANDATORY — system parses this exact structure)\n") sb.WriteString("\n") sb.WriteString("# Active Plan\n") diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 29f3aac23..f2e232663 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -291,11 +291,11 @@ func (t *ExecTool) guardCommand(command, cwd string) string { } // Token-based absolute path detection. - // Uses strings.Fields instead of regex to avoid false positives - // from slashes in relative paths (e.g., "tests/cold/file.py"). + // Uses shellTokenize to respect quoted strings (e.g., "/review ..." + // is a single argument, not a file path). // Flags like -I/usr/local/include are naturally skipped because // filepath.IsAbs returns false for tokens starting with "-". - for _, token := range strings.Fields(cmd) { + for _, token := range shellTokenize(cmd) { token = strings.Trim(token, "\"'") if !filepath.IsAbs(token) { @@ -321,6 +321,42 @@ func (t *ExecTool) guardCommand(command, cwd string) string { return "" } +// shellTokenize splits a command string into tokens while respecting +// single and double quotes. Quoted substrings are returned as a single +// token (with the quotes still attached so the caller can trim them). +// This prevents false positives where a quoted argument like +// "/review skip-git-repo-check" would be split into "/review" and +// "skip-git-repo-check" by strings.Fields. +func shellTokenize(s string) []string { + var tokens []string + var cur strings.Builder + var quote byte // 0 = none, '\'' or '"' + for i := 0; i < len(s); i++ { + ch := s[i] + switch { + case quote != 0: + cur.WriteByte(ch) + if ch == quote { + quote = 0 + } + case ch == '\'' || ch == '"': + cur.WriteByte(ch) + quote = ch + case ch == ' ' || ch == '\t': + if cur.Len() > 0 { + tokens = append(tokens, cur.String()) + cur.Reset() + } + default: + cur.WriteByte(ch) + } + } + if cur.Len() > 0 { + tokens = append(tokens, cur.String()) + } + return tokens +} + // isExecutable checks if a path points to an executable file. // On Unix, checks the execute permission bits. // On Windows, checks for known executable extensions. diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 9a023ae68..4e111ec0c 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -486,3 +486,22 @@ func TestGuardCommand_CdWithAbsoluteWorkspacePath(t *testing.T) { t.Errorf("cd to workspace subdir should be allowed: %q → %s", cmd, result) } } + +func TestGuardCommand_QuotedSlashArgNotBlocked(t *testing.T) { + workspace := t.TempDir() + tool := NewExecTool(workspace, true) + + // A quoted argument starting with "/" is not a file path — it's a + // command argument that happens to contain a slash. + cmds := []string{ + `codex exec --yolo "/review skip-git-repo-check"`, + `echo '/hello world'`, + `grep "/etc/passwd" file.txt`, + } + for _, cmd := range cmds { + result := tool.guardCommand(cmd, workspace) + if result != "" { + t.Errorf("Quoted argument should not be blocked: %q → %s", cmd, result) + } + } +}