feat: epic closeouts#3
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds TicketOutcome types and persistence, tightens claim path validation and containment, refactors CLI run persistence and claim-renewal logic, centralizes runtime parsing/normalization, adjusts adapter process-kill behavior, and expands tests and documentation across engine, CLI, adapters, and ticketstore. Changes
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/run.go (1)
377-403:⚠️ Potential issue | 🟠 MajorBlocked-epic runs not resumable when not retried: pointer missing for non-interactive failures.
At line 395, when
handleBlockedEpicRunreturnsfalse(non-TTY environment or user selects no tickets),doRunEpicreturns the originalrunErrwithout writing.verk/current. However,engine.RunEpicalready persistedrun.jsonbefore returning theBlockedRunError(lines 85–87, 159–162 in epic_run.go), so a valid run artifact exists on disk but is unreachable bydoAutoResumedue to the missing pointer.Write
.verk/currentunconditionally afterengine.RunEpicreturns, even on blocked error, to ensure all persisted runs remain resumable. The pointer-write can move before the blocked-error handling (line 377) or be duplicated in the non-retried path (after line 394).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/run.go` around lines 377 - 403, When engine.RunEpic has persisted run.json but returns a BlockedRunError, doRunEpic currently returns early without writing the .verk/current pointer; update doRunEpic so writeCurrentRunID(repoRoot, runID) is invoked unconditionally after engine.RunEpic returns (i.e., before handling asBlockedRunError), or add a writeCurrentRunID call in the non-retried path after handleBlockedEpicRun returns false, ensuring runID is written to disk even when runErr is a blocked error; reference symbols: doRunEpic, engine.RunEpic, runErr, asBlockedRunError, handleBlockedEpicRun, writeCurrentRunID, runID, repoRoot.
🧹 Nitpick comments (4)
internal/cli/shared_test.go (1)
70-71: Fix the lexicographic explanation in the test comment.The statement is reversed:
run-ticket-a-1000sorts beforerun-ticket-z-2000, not the other way around.Suggested comment correction
- // run-ticket-z-2000 sorts lex BEFORE run-ticket-a-1000, + // run-ticket-a-1000 sorts lex BEFORE run-ticket-z-2000, // but 2000 > 1000, so run-ticket-z-2000 must win.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/shared_test.go` around lines 70 - 71, The test comment incorrectly states sort order; update the comment referencing the example branches `run-ticket-a-1000` and `run-ticket-z-2000` to say that `run-ticket-a-1000` sorts lexicographically before `run-ticket-z-2000` (so even though 2000 > 1000, lexicographic ordering places the "a" branch first), and adjust the sentence around that example in the block containing those examples to reflect the correct ordering.internal/adapters/verify/command/runner_test.go (1)
199-215: Consider removing or softening this host-dependent test.Line 203 (
go version) makes this test depend on runtime toolchain availability and overlaps with the PATH assertion already covered byTestRunCommands_DefaultEnvIncludesPath(Line 218-248). Prefer one deterministic PATH test to avoid CI flakiness.♻️ Optional simplification
-func TestRunCommands_PathDependentCommandWorksByDefault(t *testing.T) { - repoRoot := t.TempDir() - - results, err := RunCommands(context.Background(), repoRoot, []string{ - "go version", - }, policy.VerificationConfig{ - DefaultTimeoutMinutes: 1, - }) - if err != nil { - t.Fatalf("RunCommands returned error: %v", err) - } - if len(results) != 1 { - t.Fatalf("expected 1 command result, got %d", len(results)) - } - if results[0].ExitCode != 0 { - t.Fatalf("expected PATH-dependent command to run, got exit code %d", results[0].ExitCode) - } -} +// Covered by TestRunCommands_DefaultEnvIncludesPath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/verify/command/runner_test.go` around lines 199 - 215, The test TestRunCommands_PathDependentCommandWorksByDefault is host-dependent because it runs "go version" and should be removed or softened; either delete this test function entirely or change it to skip on CI (call t.Skip) or replace the command with a deterministic, platform-neutral check already covered by TestRunCommands_DefaultEnvIncludesPath; update/remove any assertions in TestRunCommands_PathDependentCommandWorksByDefault and keep TestRunCommands_DefaultEnvIncludesPath as the single PATH verification for RunCommands.internal/adapters/ticketstore/tkmd/claims.go (1)
562-576: Redundant condition at lines 572-574.The check at lines 572-574 is unreachable. If
relstarts with".."but isn't equal to".."and doesn't start with".."+separator, the only remaining possibility would be something like"..foo"whichfilepath.Relwould never produce for a valid relative path.The condition at line 569 already covers
rel == ".."andstrings.HasPrefix(rel, ".."+separator), making lines 572-574 dead code.🔧 Suggested simplification
func assertPathUnderBase(target, base string) error { cleanTarget := filepath.Clean(target) cleanBase := filepath.Clean(base) rel, err := filepath.Rel(cleanBase, cleanTarget) if err != nil { return fmt.Errorf("claim path resolution failed: %w", err) } if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return fmt.Errorf("claim path escapes base directory") } - if strings.HasPrefix(rel, "..") { - return fmt.Errorf("claim path escapes base directory") - } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/ticketstore/tkmd/claims.go` around lines 562 - 576, The function assertPathUnderBase contains a redundant check: after computing rel via filepath.Rel and already checking rel == ".." and strings.HasPrefix(rel, ".."+string(filepath.Separator)), the subsequent if strings.HasPrefix(rel, "..") branch is unreachable and should be removed; update the function (referencing assertPathUnderBase, rel, filepath.Rel, filepath.Separator) to delete the redundant condition and return nil as before, leaving only the initial two escape checks.internal/engine/resume_test.go (1)
350-355: Remove the redundant nil check in the second assertion.After the explicit nil guard,
snapshot.Closeout == nil || ...can be simplified for readability.♻️ Suggested cleanup
if snapshot.Closeout == nil { t.Fatalf("expected repaired closeout, got %#v", snapshot.Closeout) } -if snapshot.Closeout == nil || !snapshot.Closeout.Closable { +if !snapshot.Closeout.Closable { t.Fatalf("expected closable closeout, got %#v", snapshot.Closeout) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/resume_test.go` around lines 350 - 355, The second assertion redundantly checks nil after the prior guard; update the test so the first check remains (if snapshot.Closeout == nil { t.Fatalf(...) }) and the subsequent assertion only verifies the Closable field (if !snapshot.Closeout.Closable { t.Fatalf("expected closable closeout, got %#v", snapshot.Closeout) }), referencing snapshot.Closeout to locate the code in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/verk/main_test.go`:
- Around line 187-199: The test creates a run-level fixture but not the
per-ticket snapshots that status expects, causing failures; before calling
runCLIFromDir(t, repoRoot, "status", largeRunID) iterate ticketIDs and write a
ticket-level fixture (e.g., using writeJSONFixture) for each ticket with the
expected structure (a ticket run artifact / snapshot containing
RunID/RunArtifact fields such as SchemaVersion, RunID, TicketID/RootTicketID,
Status/CurrentPhase matching state.* types) and the correct path/name (the
ticket-run.json file the status command loads) so that status can load each
ticket artifact during the test.
In `@internal/adapters/runtime/prompt.go`:
- Around line 248-249: The parse logic accepts statuses only for the sentinel
path but not for the direct or last-JSON paths, causing inconsistent acceptance;
after any successful parse (e.g., parseSentinelLine[VerkResultBlock],
parseDirectJSON, parseLastJSON) run the same canonicalization/validation step:
convert the parsed status into WorkerStatus and call
ValidateWorkerStatus(WorkerStatus(...)) (and replace the status with the
canonical/normalized value if needed) before returning the VerkResultBlock so
all parse branches use identical status validation/normalization.
In `@internal/adapters/runtime/types.go`:
- Around line 341-343: The expiry boundary is inconsistent: in
internal/adapters/runtime/types.go the isExpiredWaiver check treats
WaiverExpiresAt == now as expired (!After(time.Now())), while
internal/engine/closeout.go uses Before(time.Now()) (< now). Change the
isExpiredWaiver logic in internal/adapters/runtime/types.go (the isExpiredWaiver
variable computation) to use finding.WaiverExpiresAt.Before(time.Now()) with the
nil-check (i.e., finding.WaiverExpiresAt != nil &&
finding.WaiverExpiresAt.Before(time.Now())) so both code paths use the same
strict-before semantics as internal/engine/closeout.go.
In `@internal/engine/closeout_test.go`:
- Around line 396-398: Replace the weak check that only rejects "open" with an
explicit equality assertion: instead of else if got.Disposition == "open" {
t.Fatalf(...) }, compare got.Disposition to tc.finding.Disposition and fail with
a clear message using tc.name when they differ; i.e., assert got.Disposition ==
tc.finding.Disposition and call t.Fatalf("%s: expected disposition %q, got %q",
tc.name, tc.finding.Disposition, got.Disposition) on mismatch.
---
Outside diff comments:
In `@internal/cli/run.go`:
- Around line 377-403: When engine.RunEpic has persisted run.json but returns a
BlockedRunError, doRunEpic currently returns early without writing the
.verk/current pointer; update doRunEpic so writeCurrentRunID(repoRoot, runID) is
invoked unconditionally after engine.RunEpic returns (i.e., before handling
asBlockedRunError), or add a writeCurrentRunID call in the non-retried path
after handleBlockedEpicRun returns false, ensuring runID is written to disk even
when runErr is a blocked error; reference symbols: doRunEpic, engine.RunEpic,
runErr, asBlockedRunError, handleBlockedEpicRun, writeCurrentRunID, runID,
repoRoot.
---
Nitpick comments:
In `@internal/adapters/ticketstore/tkmd/claims.go`:
- Around line 562-576: The function assertPathUnderBase contains a redundant
check: after computing rel via filepath.Rel and already checking rel == ".." and
strings.HasPrefix(rel, ".."+string(filepath.Separator)), the subsequent if
strings.HasPrefix(rel, "..") branch is unreachable and should be removed; update
the function (referencing assertPathUnderBase, rel, filepath.Rel,
filepath.Separator) to delete the redundant condition and return nil as before,
leaving only the initial two escape checks.
In `@internal/adapters/verify/command/runner_test.go`:
- Around line 199-215: The test
TestRunCommands_PathDependentCommandWorksByDefault is host-dependent because it
runs "go version" and should be removed or softened; either delete this test
function entirely or change it to skip on CI (call t.Skip) or replace the
command with a deterministic, platform-neutral check already covered by
TestRunCommands_DefaultEnvIncludesPath; update/remove any assertions in
TestRunCommands_PathDependentCommandWorksByDefault and keep
TestRunCommands_DefaultEnvIncludesPath as the single PATH verification for
RunCommands.
In `@internal/cli/shared_test.go`:
- Around line 70-71: The test comment incorrectly states sort order; update the
comment referencing the example branches `run-ticket-a-1000` and
`run-ticket-z-2000` to say that `run-ticket-a-1000` sorts lexicographically
before `run-ticket-z-2000` (so even though 2000 > 1000, lexicographic ordering
places the "a" branch first), and adjust the sentence around that example in the
block containing those examples to reflect the correct ordering.
In `@internal/engine/resume_test.go`:
- Around line 350-355: The second assertion redundantly checks nil after the
prior guard; update the test so the first check remains (if snapshot.Closeout ==
nil { t.Fatalf(...) }) and the subsequent assertion only verifies the Closable
field (if !snapshot.Closeout.Closable { t.Fatalf("expected closable closeout,
got %#v", snapshot.Closeout) }), referencing snapshot.Closeout to locate the
code in the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08b2b656-4b62-467f-a03a-b91a42f22851
📒 Files selected for processing (26)
cmd/verk/main_test.gointernal/adapters/runtime/claude/adapter.gointernal/adapters/runtime/claude/adapter_test.gointernal/adapters/runtime/normalize.gointernal/adapters/runtime/prompt.gointernal/adapters/runtime/types.gointernal/adapters/ticketstore/tkmd/claims.gointernal/adapters/ticketstore/tkmd/claims_test.gointernal/adapters/ticketstore/tkmd/store.gointernal/adapters/ticketstore/tkmd/store_test.gointernal/adapters/verify/command/runner.gointernal/adapters/verify/command/runner_test.gointernal/cli/init_test.gointernal/cli/root.gointernal/cli/run.gointernal/cli/run_current_pointer_test.gointernal/cli/run_persistence_test.gointernal/cli/run_sync_test.gointernal/cli/shared_test.gointernal/engine/closeout_test.gointernal/engine/epic_run.gointernal/engine/epic_run_test.gointernal/engine/resume.gointernal/engine/resume_test.gointernal/engine/ticket_run_test.gotools.mod
💤 Files with no reviewable changes (1)
- internal/adapters/verify/command/runner.go
Owner
Author
|
Follow-up for CodeRabbit review #3 (review):
Validation: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/run.go (1)
256-264:⚠️ Potential issue | 🟠 MajorPublish
.verk/currentonly after the run is durably resumable.
writeCurrentRunID()happens beforetkmd.SaveTicket()and before the finalrun.jsonupdate. If either later write fails,doAutoResume()will still follow.verk/currentto the stale startup snapshot and may rerun work that never started cleanly or already finished. Move the pointer write after the remaining required persistence succeeds, or clear it on those error paths.🔧 Minimal direction
if err := saveJSONAtomic(filepath.Join(repoRoot, ".verk", "runs", runID, "run.json"), run); err != nil { return runID, err } - // Write the current-run pointer only after run.json is on disk. An early - // return above (lock, claim, adapter, git, or this save) leaves the pointer - // untouched so subsequent commands never resolve to a run without an artifact. - if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { - _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) - } ticket.Status = tkmd.StatusInProgress if err := tkmd.SaveTicket(filepath.Join(repoRoot, ".tickets", ticketID+".md"), ticket); err != nil { return runID, err } + if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { + _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) + } @@ ); err != nil { + _ = clearCurrentRunID(repoRoot) emitRunID = false return runID, err }Also applies to: 267-268, 319-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/run.go` around lines 256 - 264, The current-run pointer is written too early (writeCurrentRunID called immediately after saveJSONAtomic) so doAutoResume may follow a pointer to a run whose ticket or final run.json wasn't persisted; move the call to writeCurrentRunID so it executes only after tkmd.SaveTicket() and the final run.json update complete successfully (or, alternatively, clear the pointer on those error paths), making sure to update code paths around saveJSONAtomic, tkmd.SaveTicket, and the final run.json write (the run persistence and ticket-save logic referenced in doAutoResume) so .verk/current is only published when the run is durably resumable.
🧹 Nitpick comments (4)
internal/adapters/runtime/prompt_test.go (1)
195-199: “normalizes spaces” case does not currently include spaced input.The label says normalization is being tested, but the payload is already canonical (
"passed"). Consider using a spaced/cased value to actually exercise normalization on the last-JSON path.Suggested test tweak
{ name: "last JSON normalizes spaces", - input: "Review follows:\n{\"review_status\":\"passed\",\"summary\":\"ok\",\"findings\":[]}", + input: "Review follows:\n{\"review_status\":\" PASSED \",\"summary\":\"ok\",\"findings\":[]}", wantFound: true, wantStatus: "passed", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/runtime/prompt_test.go` around lines 195 - 199, The "last JSON normalizes spaces" test case currently uses a canonical "passed" value so it doesn't exercise normalization; update the test input string in the table entry (the case with name "last JSON normalizes spaces") to include extra spaces and/or different casing inside the JSON payload (e.g., " review_status": " Passed " or similar) and keep wantFound true and wantStatus "passed" so the test asserts that the parsing/normalization logic (the code path exercised by this test) trims/spaces and lowercases the status correctly.internal/engine/resume_test.go (1)
260-265: Strengthen this assertion to avoid a false-positive empty/empty match.Great direction asserting
BlockReason == FailedGate. To make it stricter, also assertFailedGateis non-empty before the equality check.Proposed test hardening
if snapshot.Closeout == nil { t.Fatalf("expected closeout to be repaired, got %#v", snapshot.Closeout) } + if snapshot.Closeout.FailedGate == "" { + t.Fatalf("expected FailedGate to be set for non-closable closeout, got empty") + } if snapshot.BlockReason != snapshot.Closeout.FailedGate { t.Fatalf("expected BlockReason %q, got %q", snapshot.Closeout.FailedGate, snapshot.BlockReason) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/resume_test.go` around lines 260 - 265, The test currently only checks snapshot.BlockReason == snapshot.Closeout.FailedGate which can pass if both are empty; strengthen by first asserting snapshot.Closeout.FailedGate is non-empty (e.g., t.Fatalf if ""), then assert snapshot.BlockReason equals snapshot.Closeout.FailedGate; update the checks around snapshot.Closeout, snapshot.Closeout.FailedGate and snapshot.BlockReason in resume_test.go to perform the non-empty assertion before the equality assertion.internal/cli/run_current_pointer_test.go (1)
97-100: Rename the test to match what it now asserts.The body no longer checks “cleared on engine failure”; it only checks that
.verk/currentis not advanced to the newrunID. Keeping the old name will make future failures harder to interpret.Also applies to: 123-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/run_current_pointer_test.go` around lines 97 - 100, Rename the test identifiers and descriptions to reflect the current assertion that .verk/current is not advanced to the new runID when engine.RunEpic errors; specifically change the doRunEpic-related test name(s) (e.g., any TestDoRunEpic_ClearedOnEngineFailure or similar) to something like TestDoRunEpic_DoesNotAdvanceCurrentOnEngineFailure (and likewise update the other affected test name at the second location) and update their test description strings so they assert "does not advance .verk/current to new runID when engine.RunEpic returns an error" while leaving the test body and assertions unchanged (references: doRunEpic, writeCurrentRunID, engine.RunEpic).internal/engine/ticket_run.go (1)
1443-1452: Consider documenting the intentional outcome mapping subset.The function only maps
closedandblockedphases to outcomes, whilestate.TicketOutcomedefines additional constants (failed_retryable,needs_decision,cancelled) that remain unused. The test names thislegacy_blocked, suggesting future refinement.Consider adding a brief comment noting that granular outcomes (e.g., distinguishing retryable vs. needs-decision blockers) are deferred to a follow-up.
📝 Suggested documentation
func ticketOutcomeForPhase(phase state.TicketPhase) state.TicketOutcome { + // Maps terminal phases to outcomes. Non-terminal phases return empty. + // More granular outcomes (failed_retryable, needs_decision, cancelled) + // are reserved for future escalation classification work. switch phase { case state.TicketPhaseClosed: return state.TicketOutcomeClosed case state.TicketPhaseBlocked: return state.TicketOutcomeBlocked default: return "" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/ticket_run.go` around lines 1443 - 1452, Add a short comment above ticketOutcomeForPhase explaining that it intentionally maps only the two legacy phases to outcomes — state.TicketPhaseClosed -> state.TicketOutcomeClosed and state.TicketPhaseBlocked -> state.TicketOutcomeBlocked — and that other outcome constants like state.TicketOutcomeFailedRetryable, state.TicketOutcomeNeedsDecision, and state.TicketOutcomeCancelled are intentionally deferred for a follow-up (the test refers to legacy_blocked). This documents the deliberate subset mapping and signals future refinement without changing behavior in ticketOutcomeForPhase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/ticketstore/tkmd/claims.go`:
- Around line 562-571: assertPathUnderBase currently only does lexical checks
and allows symlink escapes; update it to resolve symlinks before comparing paths
by calling filepath.EvalSymlinks (or equivalent) on both the base and the target
and using those resolved paths for the relative check and separator-prefix
check. Ensure EvalSymlinks errors are returned (wrap with context like "claim
path resolution failed") and keep the same escape detection (rel == ".." or
strings.HasPrefix(rel, ".."+string(filepath.Separator))) but performed on the
resolved paths so symlinked parents cannot escape the base.
---
Outside diff comments:
In `@internal/cli/run.go`:
- Around line 256-264: The current-run pointer is written too early
(writeCurrentRunID called immediately after saveJSONAtomic) so doAutoResume may
follow a pointer to a run whose ticket or final run.json wasn't persisted; move
the call to writeCurrentRunID so it executes only after tkmd.SaveTicket() and
the final run.json update complete successfully (or, alternatively, clear the
pointer on those error paths), making sure to update code paths around
saveJSONAtomic, tkmd.SaveTicket, and the final run.json write (the run
persistence and ticket-save logic referenced in doAutoResume) so .verk/current
is only published when the run is durably resumable.
---
Nitpick comments:
In `@internal/adapters/runtime/prompt_test.go`:
- Around line 195-199: The "last JSON normalizes spaces" test case currently
uses a canonical "passed" value so it doesn't exercise normalization; update the
test input string in the table entry (the case with name "last JSON normalizes
spaces") to include extra spaces and/or different casing inside the JSON payload
(e.g., " review_status": " Passed " or similar) and keep wantFound true and
wantStatus "passed" so the test asserts that the parsing/normalization logic
(the code path exercised by this test) trims/spaces and lowercases the status
correctly.
In `@internal/cli/run_current_pointer_test.go`:
- Around line 97-100: Rename the test identifiers and descriptions to reflect
the current assertion that .verk/current is not advanced to the new runID when
engine.RunEpic errors; specifically change the doRunEpic-related test name(s)
(e.g., any TestDoRunEpic_ClearedOnEngineFailure or similar) to something like
TestDoRunEpic_DoesNotAdvanceCurrentOnEngineFailure (and likewise update the
other affected test name at the second location) and update their test
description strings so they assert "does not advance .verk/current to new runID
when engine.RunEpic returns an error" while leaving the test body and assertions
unchanged (references: doRunEpic, writeCurrentRunID, engine.RunEpic).
In `@internal/engine/resume_test.go`:
- Around line 260-265: The test currently only checks snapshot.BlockReason ==
snapshot.Closeout.FailedGate which can pass if both are empty; strengthen by
first asserting snapshot.Closeout.FailedGate is non-empty (e.g., t.Fatalf if
""), then assert snapshot.BlockReason equals snapshot.Closeout.FailedGate;
update the checks around snapshot.Closeout, snapshot.Closeout.FailedGate and
snapshot.BlockReason in resume_test.go to perform the non-empty assertion before
the equality assertion.
In `@internal/engine/ticket_run.go`:
- Around line 1443-1452: Add a short comment above ticketOutcomeForPhase
explaining that it intentionally maps only the two legacy phases to outcomes —
state.TicketPhaseClosed -> state.TicketOutcomeClosed and
state.TicketPhaseBlocked -> state.TicketOutcomeBlocked — and that other outcome
constants like state.TicketOutcomeFailedRetryable,
state.TicketOutcomeNeedsDecision, and state.TicketOutcomeCancelled are
intentionally deferred for a follow-up (the test refers to legacy_blocked). This
documents the deliberate subset mapping and signals future refinement without
changing behavior in ticketOutcomeForPhase.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0448f41d-d7ba-49df-9752-90a6dd679258
📒 Files selected for processing (19)
cmd/verk/main_test.godocs/plans/2026-04-22-ticket-state-machine.mddocs/plans/INDEX.mdinternal/adapters/runtime/prompt.gointernal/adapters/runtime/prompt_test.gointernal/adapters/ticketstore/tkmd/claims.gointernal/adapters/verify/command/runner_test.gointernal/cli/run.gointernal/cli/run_current_pointer_test.gointernal/cli/shared_test.gointernal/e2e/helpers_test.gointernal/e2e/resume_claim_recovery_test.gointernal/engine/closeout_test.gointernal/engine/epic_run.gointernal/engine/epic_run_test.gointernal/engine/resume_test.gointernal/engine/ticket_run.gointernal/engine/ticket_run_test.gointernal/state/types.go
✅ Files skipped from review due to trivial changes (1)
- internal/e2e/resume_claim_recovery_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/adapters/verify/command/runner_test.go
- internal/engine/epic_run.go
- internal/adapters/runtime/prompt.go
- docs/plans/INDEX.md
- internal/engine/closeout_test.go
- cmd/verk/main_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/engine/ticket_run.go (1)
703-730: Consider: declared/derived check ID collision behavior.When the same
IDexists in bothDeclaredChecksandDerivedChecks, the derived check overwrites the declared one in thechecksmap (lines 705-710). If a declared check is non-advisory but a derived check with the same ID is advisory, the advisory flag from the derived check would be used.If this is intentional (derived checks take precedence), a brief comment would clarify the design. If IDs are guaranteed unique across both slices, this is a non-issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/ticket_run.go` around lines 703 - 730, The code in advisoryFailingCheckIDs currently inserts DeclaredChecks then DerivedChecks into the checks map so DerivedChecks overwrite DeclaredChecks on ID collisions (variables: checks, coverage.DeclaredChecks, coverage.DerivedChecks); decide intended precedence and make it explicit: either add a short comment above advisoryFailingCheckIDs stating "Derived checks intentionally override declared checks on ID collisions" if that behavior is desired, or change the insertion order so declared checks take precedence (populate coverage.DerivedChecks first, then coverage.DeclaredChecks) so the declared entry wins; update the function’s comment accordingly to document the choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/ticketstore/tkmd/claims_test.go`:
- Around line 276-305: Update the tests around the maliciousIDs table so they
assert a validation-specific failure from RenewClaim/ReleaseClaim rather than
any error; locate the test cases using maliciousIDs and the calls to RenewClaim
and ReleaseClaim and change the assertion to check for the identifier-validation
error (e.g. compare against the exported validation error like
ErrInvalidIdentifier if present, or assert the error string contains a clear
validation marker such as "invalid" or "invalid identifier"), and explicitly
fail the test if the error is a generic "not found" or unrelated error so the
test guarantees traversal-hardening rejection up front.
---
Nitpick comments:
In `@internal/engine/ticket_run.go`:
- Around line 703-730: The code in advisoryFailingCheckIDs currently inserts
DeclaredChecks then DerivedChecks into the checks map so DerivedChecks overwrite
DeclaredChecks on ID collisions (variables: checks, coverage.DeclaredChecks,
coverage.DerivedChecks); decide intended precedence and make it explicit: either
add a short comment above advisoryFailingCheckIDs stating "Derived checks
intentionally override declared checks on ID collisions" if that behavior is
desired, or change the insertion order so declared checks take precedence
(populate coverage.DerivedChecks first, then coverage.DeclaredChecks) so the
declared entry wins; update the function’s comment accordingly to document the
choice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 600a81e4-fc9c-4eda-bcc7-53586bc37f13
📒 Files selected for processing (8)
internal/adapters/runtime/prompt_test.gointernal/adapters/ticketstore/tkmd/claims.gointernal/adapters/ticketstore/tkmd/claims_test.gointernal/cli/run.gointernal/cli/run_current_pointer_test.gointernal/cli/run_persistence_test.gointernal/engine/resume_test.gointernal/engine/ticket_run.go
✅ Files skipped from review due to trivial changes (1)
- internal/adapters/runtime/prompt_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/adapters/ticketstore/tkmd/claims.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/engine/ticket_run.go (2)
1221-1223: Magic number coupling between minimum interval and immediate renewal threshold.The immediate renewal threshold
3*25*time.Millisecondis implicitly coupled to the minimum interval25*time.Millisecond. If the minimum interval changes, this threshold should also be updated to maintain the safety margin.Consider extracting these to named constants or computing the threshold from the interval:
♻️ Optional: extract constants
+const ( + claimRenewalMinInterval = 25 * time.Millisecond +) + func (st *ticketRunState) startClaimRenewal(ctx context.Context) (context.Context, func() error) { // ... interval := remaining / 3 - if interval < 25*time.Millisecond { - interval = 25 * time.Millisecond + if interval < claimRenewalMinInterval { + interval = claimRenewalMinInterval } // ... - if knownRemaining && remaining <= 3*25*time.Millisecond { + if knownRemaining && remaining <= 3*claimRenewalMinInterval {Also applies to: 1242-1246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/ticket_run.go` around lines 1221 - 1223, Extract the magic 25*time.Millisecond into a named constant (e.g., MinInterval) and replace the hard-coded 25*time.Millisecond usage in the interval clamping (currently in ticket_run.go around the interval check) with MinInterval; then compute any dependent thresholds (the immediate renewal threshold currently written as 3*25*time.Millisecond around the later check) from that constant (e.g., ImmediateRenewalThreshold := 3 * MinInterval) and use that computed value in the renewal logic so the safety margin stays consistent when MinInterval changes; update all occurrences (the clamp and the immediate-renewal check locations) to reference these constants/derived values instead of literal numbers.
719-719: Consider preallocating slice capacity.The slice could be initialized with a capacity hint to reduce allocations when there are multiple advisory failing checks.
♻️ Optional: preallocate capacity
- out := make([]string, 0) + out := make([]string, 0, len(latest))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/ticket_run.go` at line 719, The slice out is created without capacity which causes extra allocations; change allocation to preallocate capacity by first counting the number of advisory failing checks and then creating out with that capacity (e.g. count failing entries in the loop that inspects advisory checks, then replace out := make([]string, 0) with out := make([]string, 0, count)). Update any code that appends to out (the variable named out) to use the preallocated slice so append operations avoid unnecessary reallocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/ticketstore/tkmd/claims_test.go`:
- Around line 385-439: The test TestClaimPaths_RejectsSymlinkEscapingBase
currently only covers a symlink on the final "claims" directory; add
table-driven cases that create ancestor symlinks for the base paths (e.g.
symlink the ".tickets" directory itself to an outside path and symlink ".verk"
or ".verk/runs" to outside) so claimPaths run against those repo roots also
rejects escapes; update the tests' setup functions to create ancestor symlinks
(use t.TempDir(), os.MkdirAll, os.Symlink and t.Skipf on unsupported platforms)
and assert that claimPaths(dir, "run-a", "ticket-1") returns an error containing
"claim path escapes base directory" for these new cases, referencing the
existing TestClaimPaths_RejectsSymlinkEscapingBase and the claimPaths function
for locating where to add them.
In `@internal/adapters/ticketstore/tkmd/claims.go`:
- Around line 552-567: The current checks validate livePath/durablePath against
liveDir/durableDir but not that those base dirs themselves are anchored under
repo-owned roots; change the logic to first validate liveDir and durableDir are
under trusted repo roots (e.g., assertPathUnderBase(liveDir, repoRoot) or more
specifically the intended repo subroots like filepath.Join(repoRoot, ".tickets")
and filepath.Join(repoRoot, ".verk")) before calling os.MkdirAll, then keep the
existing assertPathUnderBase(livePath, liveDir) and
assertPathUnderBase(durablePath, durableDir) checks afterward; update the code
around the liveDir/durableDir creation (and references to runID/ticketsDir) so
the directory containment checks run prior to any MkdirAll calls.
---
Nitpick comments:
In `@internal/engine/ticket_run.go`:
- Around line 1221-1223: Extract the magic 25*time.Millisecond into a named
constant (e.g., MinInterval) and replace the hard-coded 25*time.Millisecond
usage in the interval clamping (currently in ticket_run.go around the interval
check) with MinInterval; then compute any dependent thresholds (the immediate
renewal threshold currently written as 3*25*time.Millisecond around the later
check) from that constant (e.g., ImmediateRenewalThreshold := 3 * MinInterval)
and use that computed value in the renewal logic so the safety margin stays
consistent when MinInterval changes; update all occurrences (the clamp and the
immediate-renewal check locations) to reference these constants/derived values
instead of literal numbers.
- Line 719: The slice out is created without capacity which causes extra
allocations; change allocation to preallocate capacity by first counting the
number of advisory failing checks and then creating out with that capacity (e.g.
count failing entries in the loop that inspects advisory checks, then replace
out := make([]string, 0) with out := make([]string, 0, count)). Update any code
that appends to out (the variable named out) to use the preallocated slice so
append operations avoid unnecessary reallocations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a76940ff-eccb-47cf-960c-859dfeb709fa
📒 Files selected for processing (4)
internal/adapters/ticketstore/tkmd/claims.gointernal/adapters/ticketstore/tkmd/claims_test.gointernal/engine/ticket_run.gointernal/engine/ticket_run_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/engine/ticket_run_test.go
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Documentation