Repository navigation
fix: prevent false task completion via prompt kickoff + worktree diff-check (#144) - #145
Merged
pedronauck merged 4 commits intoMay 8, 2026
Conversation
…ozy#144) `compozy tasks run` would mark a task as `status: completed` whenever the ACP session ended without a transport-level error — even when the agent produced no file changes. Subsequent runs then skipped those tasks, so a no-op session became invisible to the runner. Capture a `git`-based fingerprint of the workspace before the agent is dispatched and again after `afterTaskJobSuccess` runs. When both captures succeed and match, suppress the `MarkTaskCompleted` rewrite, leave the frontmatter at its prior status, and emit a new `task.file_skipped` event so the omission is observable. Workspaces without `git` metadata or with an unavailable `git` binary fall back to legacy behavior, so this is a strict additional safety check rather than a behavior break for non-git users. - Add `internal/core/run/internal/worktree` for snapshot capture and comparison, with coverage for git/no-git/added/modified paths. - Thread the pre-dispatch snapshot through `runner.go` → `afterJobSuccess` → `afterTaskJobSuccess`. - Add `events.EventKindTaskFileSkipped` plus `kinds.TaskFileSkippedPayload` and document the new event in `docs/events.md`. - Cover the no-op and changed-workspace paths with new executor tests. Closes compozy#144
Tighten `worktree.Capture` to never swallow a `git rev-parse HEAD` failure
into a fabricated `"no-head"` sentinel. Any failure (missing binary, empty
repo, corrupted refs) now yields an unsupported `Snapshot{}` so the runner
falls back to legacy completion behavior. Add an empty-repo regression
test alongside the existing non-git workspace test.
Addresses no-workarounds Category 3 (error swallowing) feedback during PR
preparation for compozy#144.
Contributor
WalkthroughAdds workspace snapshot capture and comparison to task post-success flow: a pre-run git snapshot is captured for PRDTasks, passed to the post-success hook, and compared to a post-run snapshot; if unchanged, the executor emits ChangesWorkspace Snapshot Detection for Task Completion
sequenceDiagram
participant Runner as Job Runner
participant PreSnap as PreSnapshot
participant JobExec as Job Execution
participant PostSnap as PostSnapshot
participant Detector as WorkspaceDetector
participant Events as EventEmitter
participant Store as TaskStore
Runner->>PreSnap: Capture workspace (HEAD + git status)
PreSnap-->>Runner: Pre-snapshot
Runner->>JobExec: Execute task job
JobExec-->>Runner: Job completes
Runner->>PostSnap: Capture workspace (HEAD + git status)
PostSnap-->>Runner: Post-snapshot
Runner->>Detector: Compare pre vs post
alt Unchanged (no work)
Detector-->>Events: Emit task.file_skipped (reason: no_workspace_changes)
Events->>Store: Preserve task status (remain pending)
else Changed (work done)
Detector-->>Store: Mark task completed
Store->>Events: Emit task.file_updated
Store->>Events: Emit task.metadata_refreshed
end
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/core/run/executor/execution_test.go (1)
697-859: ⚡ Quick winRefactor these two tests into table-driven
t.Run("Should...")subtests.
TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchangedandTestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChangedduplicate most setup/teardown logic. Converting them into one table-driven test witht.Run("Should ...")subtests will reduce drift and align test conventions.As per coding guidelines,
**/*_test.go: "Use table-driven tests with subtests (t.Run) as the default pattern" and "MUST use t.Run("Should...") pattern for ALL test cases".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/run/executor/execution_test.go` around lines 697 - 859, Combine the two tests TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchanged and TestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChanged into a single table-driven test that uses t.Run("Should ...") subtests; create a cases slice with fields like name, mutateWorkspace (bool), expectedStatus ("pending" or "completed"), and expectedEventKinds, then loop cases and inside each t.Run perform the shared setup (initTaskWorkspaceRepo, writeRunTaskFile, tasks.RefreshTaskMeta, commitTaskWorkspace, capture preSnapshot, openRuntimeEventCapture, build jobExecutionContext) and conditionally simulate workspace change (write produced.txt) before calling execCtx.afterJobSuccess; finally assert the task file content and emitted events per case. Keep references to existing symbols so you update TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchanged/TestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChanged into a single TestAfterJobSuccess_TableDriven with t.Run subtests and reuse helpers like initTaskWorkspaceRepo, commitTaskWorkspace, worktree.Capture, openRuntimeEventCapture, execCtx.afterJobSuccess and collectRuntimeEvents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/run/executor/execution_test.go`:
- Around line 805-809: The test captures preSnapshot via worktree.Capture but
never asserts that snapshot support exists; add an assertion immediately after
preSnapshot, e.g., call preSnapshot.IsSupported() and fail the test if it
returns false so the "workspace-changed" test only proceeds when snapshots are
supported; locate the capture call in the test (preSnapshot, err :=
worktree.Capture(...)) and insert a check that calls preSnapshot.IsSupported()
and t.Fatalf (or t.Skip) on false to prevent false positives from legacy
fallback behavior.
- Line 864: The git init calls pass the "-b main" flag which requires Git 2.28+
and can fail on older CI; update the tests by removing the "-b" and "main"
arguments from the runWorkspaceGit invocations (e.g., change runWorkspaceGit(t,
dir, "init", "-q", "-b", "main") to runWorkspaceGit(t, dir, "init", "-q")), and
apply the same removal to the other occurrences in the snapshot tests so the
branch name is not specified explicitly.
In `@internal/core/run/internal/worktree/snapshot.go`:
- Around line 95-104: runGit inherits the parent environment which can leak
GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR or GIT_INDEX_FILE and cause git to
operate on the wrong repo; update runGit to sanitize the environment before exec
by filtering out those four variables from os.Environ() (or overriding them to
empty) when building cmd.Env so git runs against the provided root; refer to
runGit, and the env vars GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_INDEX_FILE
when making the change.
---
Nitpick comments:
In `@internal/core/run/executor/execution_test.go`:
- Around line 697-859: Combine the two tests
TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchanged and
TestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChanged into a single
table-driven test that uses t.Run("Should ...") subtests; create a cases slice
with fields like name, mutateWorkspace (bool), expectedStatus ("pending" or
"completed"), and expectedEventKinds, then loop cases and inside each t.Run
perform the shared setup (initTaskWorkspaceRepo, writeRunTaskFile,
tasks.RefreshTaskMeta, commitTaskWorkspace, capture preSnapshot,
openRuntimeEventCapture, build jobExecutionContext) and conditionally simulate
workspace change (write produced.txt) before calling execCtx.afterJobSuccess;
finally assert the task file content and emitted events per case. Keep
references to existing symbols so you update
TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchanged/TestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChanged
into a single TestAfterJobSuccess_TableDriven with t.Run subtests and reuse
helpers like initTaskWorkspaceRepo, commitTaskWorkspace, worktree.Capture,
openRuntimeEventCapture, execCtx.afterJobSuccess and collectRuntimeEvents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8c4e922-2fc3-4489-bfa0-0e6159bf42b1
⛔ Files ignored due to path filters (1)
docs/events.mdis excluded by!**/*.md
📒 Files selected for processing (9)
internal/core/run/executor/execution_test.gointernal/core/run/executor/review_hooks.gointernal/core/run/executor/runner.gointernal/core/run/internal/worktree/snapshot.gointernal/core/run/internal/worktree/snapshot_test.gopkg/compozy/events/docs_test.gopkg/compozy/events/event.gopkg/compozy/events/event_test.gopkg/compozy/events/kinds/task.go
Strip GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_INDEX_FILE, and GIT_NAMESPACE from the environment passed to git so cmd.Dir is the only signal git uses to locate the working tree. Without this, a parent process that set any of these (a hook, a wrapper script mid-operation on another repo, or a CI runner) would silently produce a snapshot of the wrong tree and the no-op detector could misclassify a real change as no-op or vice versa. Also assert preSnapshot.IsSupported() in the workspace-changed test so it cannot pass through the legacy fallback path. Addresses CodeRabbit review on PR compozy#145.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/run/internal/worktree/snapshot.go (1)
67-78: 💤 Low valueDead code: both branches of the
isExecLookupErrorcheck return the same value.The comment on line 69 correctly documents the intent ("Any rev-parse failure…yields an unsupported Snapshot"), but the
if isExecLookupError(err)guard on line 74 and the fallthrough on line 77 both returnSnapshot{}, nil. The exec-lookup branch is unreachable / functionally identical to the catch-all.♻️ Proposed cleanup
head, err := runGit(ctx, root, "rev-parse", "HEAD") if err != nil { - // Any rev-parse failure (missing git binary, empty repo with no commits, - // corrupted refs) yields an unsupported Snapshot rather than an error so - // the runner falls back to legacy completion behavior. Surfacing the - // failure here would force every non-git or fresh-repo workspace through - // the error path even though the no-op check is purely advisory. - if isExecLookupError(err) { - return Snapshot{}, nil - } - return Snapshot{}, nil + // Any rev-parse failure (missing git binary, empty repo, corrupted refs) + // yields an unsupported Snapshot so the runner falls back to legacy + // completion behavior. The no-op check is purely advisory. + return Snapshot{}, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/run/internal/worktree/snapshot.go` around lines 67 - 78, The error handling after calling runGit (head, err := runGit(...)) contains a redundant isExecLookupError(err) branch that returns the same Snapshot{}, nil as the fallthrough; remove the if/else and collapse to a single error path: keep the explanatory comment, delete the isExecLookupError check, and simply return Snapshot{}, nil whenever err != nil; reference runGit, isExecLookupError, and Snapshot{} to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/core/run/internal/worktree/snapshot.go`:
- Around line 67-78: The error handling after calling runGit (head, err :=
runGit(...)) contains a redundant isExecLookupError(err) branch that returns the
same Snapshot{}, nil as the fallthrough; remove the if/else and collapse to a
single error path: keep the explanatory comment, delete the isExecLookupError
check, and simply return Snapshot{}, nil whenever err != nil; reference runGit,
isExecLookupError, and Snapshot{} to locate and update the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dba5e4c5-b613-43f9-a81f-61c24dcfa760
📒 Files selected for processing (3)
internal/core/run/executor/execution_test.gointernal/core/run/internal/worktree/snapshot.gointernal/core/run/internal/worktree/snapshot_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/run/internal/worktree/snapshot_test.go
…zy#144) Root cause of compozy#144: when `compozy tasks run --ide claude` dispatches a session, the prompt assembled by buildPRDTaskPrompt was structured as descriptive context (title, rules, memory paths, task spec, file paths) without any imperative directive instructing the agent to act. Combined with the model handoff that opens an ACP session ("Model set to default..."), Claude treats the entire payload as preparatory context and emits its standby greeting ("Ready when you are — let me know if you want me to start on task_NN") instead of beginning work. The session then ends cleanly without producing any diff, which the runner mistakes for a successful task completion and rewrites the frontmatter to status: completed. Subsequent runs skip the task entirely. This adds two unambiguous imperatives: - buildPRDKickoffDirective: opens with <action_required> right after the title, naming the specific task, pointing at the cy-execute-task skill, and explicitly forbidding "ask the user for confirmation". - buildPRDClosingDirective: <begin_now> at the very end of the prompt so the action requirement is the most recent context when the agent generates its first turn (recency effect). Tested with TestBuildPRDTaskPromptIncludesActionDirectives, which asserts both directives are present, the opener references the task by name and forbids confirmation prompts, and the section ordering puts the kickoff between the title and the descriptive Task Context. Together with the worktree snapshot diff-check from a1d12ed/c489708, this provides defense in depth: the prompt fix prevents the standby behavior at the source, and the diff-check guards against any remaining no-op session paths (early agent exit, agent decision to skip, transport error swallowed upstream, etc.).
alexvinicius
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/prompt/prompt_test.go (1)
235-293: ⚡ Quick winUse
t.Run("Should...")subtest shape in the new directive testPlease wrap this test body in at least one
t.Run("Should ...", func(t *testing.T) { ... })subtest to align with the repository’s required test pattern.As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/prompt/prompt_test.go` around lines 235 - 293, The test function TestBuildPRDTaskPromptIncludesActionDirectives must be refactored to use the repository's required subtest pattern: wrap the existing test body inside a t.Run("Should ...", func(t *testing.T) { ... }) subtest (keeping t.Parallel() as appropriate), so locate TestBuildPRDTaskPromptIncludesActionDirectives and move its assertions and setup into a t.Run call (preserve calls to buildPRDTaskPrompt, the opener/closer checks, requiredOpenerSnippets loop, and ordering assertions) and ensure the outer test function invokes t.Run with a descriptive "Should ..." name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/core/prompt/prompt_test.go`:
- Around line 235-293: The test function
TestBuildPRDTaskPromptIncludesActionDirectives must be refactored to use the
repository's required subtest pattern: wrap the existing test body inside a
t.Run("Should ...", func(t *testing.T) { ... }) subtest (keeping t.Parallel() as
appropriate), so locate TestBuildPRDTaskPromptIncludesActionDirectives and move
its assertions and setup into a t.Run call (preserve calls to
buildPRDTaskPrompt, the opener/closer checks, requiredOpenerSnippets loop, and
ordering assertions) and ensure the outer test function invokes t.Run with a
descriptive "Should ..." name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9f293fd-77b0-4a99-af97-f3dcdec051fd
📒 Files selected for processing (2)
internal/core/prompt/prd.gointernal/core/prompt/prompt_test.go
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #144.
compozy tasks run --ide claudewas rewritingstatus: completedon every dispatched task as long as the ACP session ended without a transport-level error — even when the agent never started work. Investigation (see screenshots in the issue) revealed two distinct failure modes feeding the same observable bug:Root cause — non-actionable prompt:
buildPRDTaskPromptassembled a prompt made entirely of descriptive sections (title, context, rules, memory paths, task spec, file paths) without any imperative directive. Combined with the model handoff that opens an ACP session ("Model set to default…"), Claude treats the payload as preparatory context and emits its standby greeting ("Ready when you are — let me know if you want me to start on task_NN") instead of beginning work. The session then ends cleanly with no diff and no tool use.Symptom amplifier — completion logic: the runner's success path was gated solely on
sessionErr == nil, so a clean standby session was indistinguishable from a successful implementation, andMarkTaskCompletedrewrote the frontmatter regardless of whether anything actually happened.This PR addresses both layers.
Implementation
Primary fix — prompt kickoff directive (
internal/core/prompt/prd.go)buildPRDTaskPromptnow wraps the descriptive sections between two unambiguous imperatives:<action_required>opener (buildPRDKickoffDirective) inserted right after the title. Names the specific task, points at thecy-execute-taskskill, and explicitly forbids "ask the user for confirmation" / "wait for further instructions" / "reply with a greeting before starting". The phrasing is deliberately direct because Claude Code (and other ACP clients) treat ambiguous descriptive prompts as conversational context.<begin_now>closer (buildPRDClosingDirective) appended at the very end of the prompt so the action requirement is the most recent context when the agent generates its first turn (recency effect).Test:
TestBuildPRDTaskPromptIncludesActionDirectivesasserts both directives are present, the opener references the task by name and forbids confirmation prompts, and the section ordering puts the kickoff between the title and the descriptive Task Context.Secondary fix — workspace diff-check (defense in depth)
Even with the prompt fix, the completion logic should not declare success without evidence of work. New internal package
internal/core/run/internal/worktreeprovides an opaqueSnapshotderived fromgit rev-parse HEAD+git status --porcelain=v1 -z --untracked-files=all, hashed with sha256.jobRunner.runcaptures a pre-dispatch snapshot and threads it throughafterJobSuccess→afterTaskJobSuccess. A post-success capture is compared against the pre-capture; when both are supported and equal,MarkTaskCompletedis suppressed and a newtask.file_skippedevent is emitted..git, missinggitbinary, or empty repository all yield an unsupportedSnapshot{}that never compares equal — preserving legacy behavior for non-git workspaces.task.file_skippedwith payloadkinds.TaskFileSkippedPayload{TasksDir, TaskName, FilePath, PreservedStatus, Reason}. Currently emitted withReason: "no_workspace_changes". Documented indocs/events.md.runGitstripsGIT_DIR,GIT_WORK_TREE,GIT_COMMON_DIR,GIT_INDEX_FILE, andGIT_NAMESPACEfrom the inherited environment so the snapshot is locked tocmd.Dirand cannot be silently misclassified by a parent process that was mid-operation on another repo.This catches the residual cases where the prompt fix alone wouldn't help — agent early exit, agent explicit refusal, transport-level oddities, or any future prompt regression that re-introduces standby behavior.
Tests
internal/core/prompt:TestBuildPRDTaskPromptIncludesActionDirectives(new) plus existingTestBuildPRDTaskPromptUsesInstalledSkillsAndLeavesOnlyTaskSpecificContextupdated to cover the new directive snippets.internal/core/run/internal/worktree: blank-root unsupported, non-git unsupported, empty-repo unsupported (no HEAD), equal snapshots when unchanged, added-file detection, modified-tracked-file detection, unsupported snapshots never compare equal, andTestCaptureIgnoresInheritedGitRepoSelectionEnv(new) which setsGIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/GIT_COMMON_DIR/GIT_NAMESPACEto a bogus path viat.Setenvand verifiesCapturestill returns a supported snapshot. Without sanitization that test fails because GIT_DIR overrides cmd.Dir andgit rev-parse HEADerrors withnot a git repository.internal/core/run/executor:TestAfterTaskJobSuccessSkipsMarkCompletedWhenWorkspaceUnchangedandTestAfterTaskJobSuccessMarksCompletedWhenWorkspaceChanged(withIsSupported()assertion to prevent legacy-fallback false positives). Both initialize a temp git repo andt.Skipwhen thegitbinary is unavailable.afterJobSuccesssignature; they all pass the zero-valueworktree.Snapshot{}(unsupported) so existing assertions on legacy behavior still hold.Verification
Ran the full
make verifypipeline (fmt → lint → test → build) plus the targeted package suites that cover this PR's changes.make lint(full pipeline gate) reports 51 issues, but all 51 pre-exist onorigin/mainand none touch files modified by this PR:goconstininternal/setup/*andsdk/extension/*gosec(G124) ininternal/api/httpapi/browser_middleware.goProof: stashing this branch's working-tree state and running
golangci-lint runproduces 54 issues on the committed branch (50 goconst + 1 gosec + 3 govet forreflect.Ptrdeprecation, the latter three already fixed locally but intentionally kept out of this PR scope).golangci-lint --new-from-rev=origin/mainreports 0 new issues introduced by this branch. The lint baseline is an existing repository state, not a regression from this work.Targeted package tests for the changes in this PR:
End-to-end verification (real CLI dispatch)
To confirm the fix lands in the actual prompt that reaches the agent (not just unit-test fixtures), I rebuilt the binary at this branch (
v0.2.1-9-gbb60109), reset 6 falsely-completed tasks back tostatus: pendingin a real workspace that had reproduced #144, killed the running global daemon, and ran:This generates the prompts that would be dispatched without spawning an ACP session. Inspecting the artifacts under
~/.compozy/runs/.../jobs/*.prompt.md:Both directives are present in every dispatched task, with the correct task name interpolated, and
--dry-rundid not mutate any frontmatter (the 6 tasks remainedpending, confirming the diff-check defense path is also healthy when no ACP session runs).Notes
cy-execute-taskskill that is already named in the existing<required_skills>block, so no new skill contracts are introduced.Summary by CodeRabbit
Bug Fixes
task.file_skippedevent is emitted.New Features
GIT_DIR/GIT_WORK_TREE/etc. cannot misdirect the snapshot.task.file_skippedto signal no-op sessions.Tests