diff --git a/cmd/verk/main_test.go b/cmd/verk/main_test.go index dc1910d..8d76984 100644 --- a/cmd/verk/main_test.go +++ b/cmd/verk/main_test.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "fmt" "io" "os" "os/exec" @@ -165,57 +166,51 @@ func runCLIFromDir(t *testing.T, dir string, args ...string) (string, string, in return string(stdoutRes.data), string(stderrRes.data), code } -// TestRunCLIFromDir_LargeOutput_NoDeadlock verifies that the goroutine-based -// pipe draining in runCLIFromDir does not deadlock when more than 64KB is -// written to stdout or stderr before ExecuteArgs returns. +// TestRunCLIFromDir_LargeOutput_NoDeadlock verifies that pipe draining in +// runCLIFromDir captures full stdout for large outputs that exceed default pipe +// capacity. func TestRunCLIFromDir_LargeOutput_NoDeadlock(t *testing.T) { - const size = 128 * 1024 // 128KB — well above the old 64KB fixed buffer - - // Directly exercise the goroutine+io.ReadAll mechanism used by runCLIFromDir. - stdoutR, stdoutW, err := os.Pipe() - if err != nil { - t.Fatalf("os.Pipe (stdout): %v", err) - } - stderrR, stderrW, err := os.Pipe() - if err != nil { - t.Fatalf("os.Pipe (stderr): %v", err) - } - - type result struct{ data []byte } - stdoutCh := make(chan result, 1) - stderrCh := make(chan result, 1) + repoRoot := t.TempDir() + writeCLIRepo(t, repoRoot) - go func() { - data, _ := io.ReadAll(stdoutR) - stdoutCh <- result{data} - }() - go func() { - data, _ := io.ReadAll(stderrR) - stderrCh <- result{data} - }() + const ( + largeRunID = "run-large-output" + largeCount = 1200 + idSuffixLen = 72 + ) - // Write >64KB to both streams before closing. - large := make([]byte, size) - if _, err := stdoutW.Write(large); err != nil { - t.Fatalf("stdoutW.Write: %v", err) - } - if _, err := stderrW.Write(large); err != nil { - t.Fatalf("stderrW.Write: %v", err) + ticketIDs := make([]string, 0, largeCount) + for i := 0; i < largeCount; i++ { + ticketIDs = append(ticketIDs, fmt.Sprintf("ticket-%04d-%s", i, strings.Repeat("x", idSuffixLen))) } - _ = stdoutW.Close() - _ = stderrW.Close() + writeJSONFixture(t, filepath.Join(repoRoot, ".verk", "runs", largeRunID, "run.json"), state.RunArtifact{ + ArtifactMeta: state.ArtifactMeta{SchemaVersion: 1, RunID: largeRunID}, + Mode: "ticket", + RootTicketID: ticketIDs[0], + Status: state.EpicRunStatusRunning, + CurrentPhase: state.TicketPhaseImplement, + TicketIDs: ticketIDs, + }) - stdoutRes := <-stdoutCh - _ = stdoutR.Close() - stderrRes := <-stderrCh - _ = stderrR.Close() + for _, ticketID := range ticketIDs { + writeJSONFixture(t, filepath.Join(repoRoot, ".verk", "runs", largeRunID, "tickets", ticketID, "ticket-run.json"), map[string]any{ + "schema_version": 1, + "run_id": largeRunID, + "ticket_id": ticketID, + "current_phase": "implement", + "implementation_attempts": 1, + "verification_attempts": 0, + "review_attempts": 0, + }) + } - if got := len(stdoutRes.data); got != size { - t.Errorf("stdout: expected %d bytes, got %d", size, got) + stdout, stderr, code := runCLIFromDir(t, repoRoot, "status", largeRunID) + if code != 0 { + t.Fatalf("status %s failed: code=%d stderr=%s", largeRunID, code, stderr) } - if got := len(stderrRes.data); got != size { - t.Errorf("stderr: expected %d bytes, got %d", size, got) + if got, want := len(stdout), 64*1024; got <= want { + t.Fatalf("expected status output > %d bytes, got %d", want, got) } } diff --git a/docs/plans/2026-04-19-verk-run-repair-oriented-gates.md b/docs/plans/2026-04-19-verk-run-repair-oriented-gates.md index 31ccba9..baee118 100644 --- a/docs/plans/2026-04-19-verk-run-repair-oriented-gates.md +++ b/docs/plans/2026-04-19-verk-run-repair-oriented-gates.md @@ -5,7 +5,7 @@ - Date: 2026-04-19 - Owner: Ronny Unger - Epic: `ver-vyag` -- Status: planned +- Status: implemented ## Summary @@ -175,15 +175,20 @@ Suggested derivations: - YAML files: YAML parse or lint when a local tool exists. - Shell files: shellcheck when available. -Derived checks should be focused and cheap. Missing optional tooling should be -recorded as skipped rather than failing the run by default. +Derived checks should be focused and cheap. Ticket-scoped derived checks are +advisory by default: a failing advisory check may trigger best-effort repair +while implementation attempts remain, but it does not block ticket closure on +its own. Missing optional tooling should be recorded as skipped rather than +failing the run by default. ### 4. Ticket Closeout Ticket closeout should run declared and derived checks before closing. Failing -checks should trigger repair while budget remains. Closeout should block only -when repair fails, repair is unsafe without user input, or the system cannot map -the problem to a safe action. +required checks should trigger repair while budget remains. Failing advisory +checks should trigger best-effort repair while implementation attempts remain, +then remain visible in validation coverage without blocking closure. Closeout +should block only when required repair fails, repair is unsafe without user +input, or the system cannot map the problem to a safe action. The closeout artifact should include validation coverage so the operator and resume logic can understand the state later. diff --git a/docs/plans/2026-04-22-ticket-state-machine.md b/docs/plans/2026-04-22-ticket-state-machine.md new file mode 100644 index 0000000..47dfade --- /dev/null +++ b/docs/plans/2026-04-22-ticket-state-machine.md @@ -0,0 +1,441 @@ +# Ticket State Machine Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Make ticket execution use explicit, user-facing state-machine outcomes so failures, decision points, and true blockers are not all collapsed into `blocked`. + +**Architecture:** Keep `TicketPhase` as the execution cursor (`implement`, `verify`, `review`, `repair`, `closeout`) and add a separate terminal/interruption `TicketOutcome` for what happened when automation stopped. Store the outcome in `ticket-run.json`, derive legacy behavior from it during migration, and only later change scheduler/CLI decisions to use outcomes directly. + +**Tech Stack:** Go state artifacts in `internal/state`, engine orchestration in `internal/engine`, CLI rendering in `internal/cli`, JSON artifacts under `.verk/runs`. + +--- + +## Background + +The current model has two overloaded concepts: + +- `ticket status` in markdown (`open`, `ready`, `in_progress`, `blocked`, `closed`) +- `ticket phase` in run artifacts (`intake`, `implement`, `verify`, `review`, `repair`, `closeout`, `closed`, `blocked`) + +The `blocked` phase currently covers several different situations: + +- safe automatic retry exists +- repair budget was exhausted and the operator should choose what to do +- the ticket has a true external blocker +- run artifacts are missing or unsafe to trust +- the epic is waiting on dependencies + +That makes CLI guidance confusing. It also led to invalid reopen suggestions when ticket-store status said `blocked` but the current run had no per-ticket snapshot. + +The new model should reserve true `blocked` for cases that cannot progress until external conditions change. Most automation failures should be `failed_retryable` first, then `needs_decision` after budgets or repeated failures are exhausted. + +## Target State Model + +### Ticket Store Status + +Markdown ticket status remains coarse and scheduler-friendly: + +- `ready`: claimable work +- `in_progress`: actively owned by a run +- `blocked`: cannot be scheduled until external conditions change +- `closed`: complete + +During migration, `open` remains accepted as an alias/default for unscheduled work. + +### Execution Phase + +`TicketPhase` remains the fine-grained execution cursor: + +- `intake` +- `implement` +- `verify` +- `review` +- `repair` +- `closeout` +- `closed` +- `blocked` (legacy only during migration) + +Long term, `blocked` should stop being an execution phase. It remains readable for old artifacts. + +### Outcome + +Add `TicketOutcome`: + +- `closed`: ticket met closeout requirements +- `failed_retryable`: automation failed, but Verk has enough reliable state to retry automatically +- `needs_decision`: automation stopped because the next step requires an operator choice +- `blocked`: no useful retry exists until an external condition changes +- `cancelled`: operator interrupted the run + +Empty outcome means “still running or legacy artifact with no outcome.” + +## Behavioral Rules + +1. Formatting, linting, tests, verification failures, and review findings are not true blockers on first failure. They enter repair or `failed_retryable`. +2. Exhausted repair budgets should become `needs_decision`, not `blocked`, unless the reason is a true external prerequisite. +3. Scope violations and dirty overlapping worktrees should become `needs_decision`: retrying may be possible, but the operator must choose. +4. Missing credentials, missing tools, claim divergence, malformed artifacts, impossible dependencies, and contradictory tickets are true `blocked`. +5. Ticket-store status alone must never imply automatic retry. Automatic retry requires a trusted run snapshot with a retryable outcome or legal legacy phase. +6. Interactive CLI should ask for decisions when outcome is `needs_decision`. +7. Non-interactive CLI should print exact commands and exit non-zero for `needs_decision`. + +--- + +## Task 1: Add Outcome Types And Snapshot Field + +**Files:** +- Modify: `internal/state/types.go` +- Modify: `internal/engine/ticket_run.go` +- Test: `internal/engine/ticket_run_test.go` + +**Step 1: Write the failing test** + +Add a test that runs a normal ticket and asserts the persisted `ticket-run.json` includes `outcome: "closed"` when `CurrentPhase` is `closed`. + +Also add a direct snapshot test for active phases: + +```go +func TestTicketRunSnapshotOutcome_ActivePhaseOmitsOutcome(t *testing.T) { + st := &ticketRunState{ + req: RunTicketRequest{ + RunID: "run-outcome", + Ticket: tkmd.Ticket{ID: "ver-outcome"}, + }, + currentPhase: state.TicketPhaseVerify, + } + snap := st.snapshot() + if snap.Outcome != "" { + t.Fatalf("expected empty outcome for active phase, got %q", snap.Outcome) + } +} +``` + +**Step 2: Run the focused test** + +Run: + +```bash +go test ./internal/engine -run 'TestRunTicket_HappyPath|TestTicketRunSnapshotOutcome' -count=1 +``` + +Expected: fail because `Outcome` does not exist. + +**Step 3: Add the minimal implementation** + +Add: + +```go +type TicketOutcome string + +const ( + TicketOutcomeClosed TicketOutcome = "closed" + TicketOutcomeFailedRetryable TicketOutcome = "failed_retryable" + TicketOutcomeNeedsDecision TicketOutcome = "needs_decision" + TicketOutcomeBlocked TicketOutcome = "blocked" + TicketOutcomeCancelled TicketOutcome = "cancelled" +) +``` + +Add `Outcome state.TicketOutcome "json:\"outcome,omitempty\""` to `TicketRunSnapshot`. + +Set outcome in `ticketRunState.snapshot()` using a small helper: + +```go +func ticketOutcomeForPhase(phase state.TicketPhase) state.TicketOutcome { + switch phase { + case state.TicketPhaseClosed: + return state.TicketOutcomeClosed + case state.TicketPhaseBlocked: + return state.TicketOutcomeBlocked + default: + return "" + } +} +``` + +This is deliberately conservative: it makes artifacts outcome-aware without changing scheduler behavior yet. + +**Step 4: Run tests** + +Run: + +```bash +go test ./internal/engine -run 'TestRunTicket_HappyPath|TestTicketRunSnapshotOutcome' -count=1 +go test ./internal/engine +``` + +Expected: pass. + +**Step 5: Commit** + +```bash +git add internal/state/types.go internal/engine/ticket_run.go internal/engine/ticket_run_test.go +git commit -m "feat(engine): persist ticket run outcomes" +``` + +--- + +## Task 2: Derive Retry Guidance From Outcome + +**Files:** +- Modify: `internal/engine/epic_run.go` +- Modify: `internal/engine/reopen.go` +- Test: `internal/engine/epic_run_test.go` +- Test: `internal/engine/reopen_test.go` + +**Step 1: Write failing tests** + +Add tests for these cases: + +- snapshot `Outcome=failed_retryable`, `CurrentPhase=verify` offers retry to `verify` or `implement` according to the selected policy. +- snapshot `Outcome=needs_decision` is listed but does not auto-generate a retry command. +- snapshot `Outcome=blocked` is listed as blocked and does not auto-generate a retry command unless the operator explicitly chooses a legal reopen target. +- old snapshot with `CurrentPhase=blocked` and empty outcome keeps legacy retry behavior for backward compatibility. + +**Step 2: Implement outcome-aware retry target helper** + +Introduce: + +```go +func DefaultReopenTargetForSnapshot(snapshot TicketRunSnapshot) (state.TicketPhase, bool) +``` + +Rules: + +- `failed_retryable`: reopen to the best safe phase from the snapshot. Initial policy can use `implement` for simplicity. +- `needs_decision`: not automatically retryable. +- `blocked`: not automatically retryable. +- empty outcome: fall back to `DefaultReopenTargetForPhase`. + +**Step 3: Wire blocked-ticket collection** + +Update `collectBlockedTickets` so trusted snapshots drive retry guidance. Ticket-store status alone may list a ticket as not closed, but cannot produce a retry command. + +**Step 4: Run tests** + +Run: + +```bash +go test ./internal/engine -run 'TestCollectBlockedTickets|TestReopen' -count=1 +go test ./internal/engine +``` + +**Step 5: Commit** + +```bash +git add internal/engine/epic_run.go internal/engine/epic_run_test.go internal/engine/reopen.go internal/engine/reopen_test.go +git commit -m "feat(engine): derive retry guidance from ticket outcomes" +``` + +--- + +## Task 3: Classify Common Stop Reasons + +**Files:** +- Modify: `internal/engine/ticket_run.go` +- Modify: `internal/engine/wave_scheduler.go` +- Test: `internal/engine/ticket_run_test.go` +- Test: `internal/engine/wave_scheduler_test.go` + +**Step 1: Add classification tests** + +Cover: + +- verification failure with repair budget remaining becomes repair, not terminal +- verification failure after budget exhaustion becomes `needs_decision` +- review findings after repair budget exhaustion become `needs_decision` +- worker `needs_context` becomes `blocked` +- missing/expired claim or claim divergence becomes `blocked` +- scope violation becomes `needs_decision` +- operator cancellation becomes `cancelled` + +**Step 2: Add outcome classifier** + +Introduce an internal helper: + +```go +func classifyTicketStop(reason string, phase state.TicketPhase, kind stopKind) state.TicketOutcome +``` + +Keep it deterministic. Do not ask an LLM to classify stop reasons. + +**Step 3: Set outcome at block sites** + +Every transition to legacy `TicketPhaseBlocked` must set an explicit outcome. + +**Step 4: Run tests** + +Run: + +```bash +go test ./internal/engine -run 'TestRunTicket_.*Block|TestRunTicket_.*Decision|TestBuildBlockedTicketSummary' -count=1 +go test ./internal/engine +``` + +**Step 5: Commit** + +```bash +git add internal/engine/ticket_run.go internal/engine/ticket_run_test.go internal/engine/wave_scheduler.go internal/engine/wave_scheduler_test.go +git commit -m "feat(engine): classify ticket stop outcomes" +``` + +--- + +## Task 4: Add Operator Decision Rendering + +**Files:** +- Modify: `internal/cli/run.go` +- Modify: `internal/cli/reopen.go` +- Test: `internal/cli/run_blocked_test.go` +- Test: `internal/cli/reopen_test.go` + +**Step 1: Write CLI rendering tests** + +For a `needs_decision` ticket, assert output includes: + +- the ticket id +- concise reason +- available actions +- exact commands for non-interactive mode + +Example commands: + +```text +verk reopen --to implement +verk reopen --to repair +verk block --reason "..." +``` + +**Step 2: Implement rendering** + +Add separate sections: + +- `Retryable tickets` +- `Tickets needing decision` +- `Blocked tickets` + +Avoid calling everything “blocked.” + +**Step 3: Run tests** + +Run: + +```bash +go test ./internal/cli -run 'TestRun.*Blocked|TestReopen' -count=1 +go test ./internal/cli +``` + +**Step 4: Commit** + +```bash +git add internal/cli/run.go internal/cli/reopen.go internal/cli/run_blocked_test.go internal/cli/reopen_test.go +git commit -m "feat(cli): render ticket decisions separately from blockers" +``` + +--- + +## Task 5: Add Interactive Decisions + +**Files:** +- Modify: `internal/cli/run.go` +- Modify: `internal/engine/reopen.go` +- Test: `internal/cli/run_blocked_test.go` + +**Step 1: Add interaction seam** + +Introduce a small prompt interface so tests can inject answers without a real terminal: + +```go +type decisionPrompter interface { + ChooseTicketDecision(ticket engine.BlockedTicket) (ticketDecision, error) +} +``` + +**Step 2: Implement choices** + +Supported interactive choices: + +- retry from implement +- retry from repair +- leave as needs decision +- mark blocked +- stop + +Do not auto-edit tickets in this task. + +**Step 3: Add tests** + +Cover one accepted retry and one “leave as decision” case. + +**Step 4: Run tests** + +Run: + +```bash +go test ./internal/cli -run 'TestRun.*Decision' -count=1 +go test ./internal/cli +``` + +**Step 5: Commit** + +```bash +git add internal/cli/run.go internal/cli/run_blocked_test.go internal/engine/reopen.go +git commit -m "feat(cli): ask operators for ticket decisions" +``` + +--- + +## Task 6: Update Docs And Migration Notes + +**Files:** +- Modify: `docs/plans/INDEX.md` +- Modify: `docs/plans/2026-04-22-ticket-state-machine.md` +- Modify or create: `docs/ticket-state-machine.md` + +**Step 1: Write user-facing docs** + +Explain: + +- ready vs in-progress vs failed retryable vs needs decision vs blocked +- what `verk run` does in interactive mode +- what non-interactive mode prints +- how old `blocked` artifacts are interpreted + +**Step 2: Run docs checks** + +Run: + +```bash +just docs-check +``` + +If no docs target exists, run: + +```bash +just pre-commit +``` + +**Step 3: Commit** + +```bash +git add docs/plans/INDEX.md docs/plans/2026-04-22-ticket-state-machine.md docs/ticket-state-machine.md +git commit -m "docs: describe ticket state machine outcomes" +``` + +--- + +## Rollout Guardrails + +- Preserve backward compatibility for existing `ticket-run.json` artifacts. +- Do not remove `TicketPhaseBlocked` until all old artifacts and CLI paths are migrated. +- Do not let ticket-store status alone create reopen commands. +- Keep `blocked` as last resort in user-facing output. +- Each task must pass `go test ./internal/engine` or `go test ./internal/cli` as appropriate. +- Run `just pre-commit` before merging. + +## Open Questions + +- Should `failed_retryable` reopen to the failed phase or always to `implement`? +- Should repeated identical `failed_retryable` outcomes auto-promote to `needs_decision` after a run-level threshold? +- Should `needs_decision` be stored in ticket markdown status, or only in run artifacts? +- Do we need a separate `waiting_on_deps` outcome, or is that only an epic scheduling condition? diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index aa3c043..643ad5f 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -7,6 +7,8 @@ or ticket first, then update this file. ## Status Legend - Active: has open tickets or is currently being implemented. +- Implemented: feature track has landed; document is retained as the durable + design/reference. - Planned: design exists, but no active implementation was found. - Blocked: design exists, but another plan or capability must land first. - Reference: baseline or support spec that other plans build on. @@ -17,14 +19,14 @@ or ticket first, then update this file. | Area | Status | Document | Related tickets | Notes | | --- | --- | --- | --- | --- | | Core engine v1 | Reference | [done/initial_v1.md](done/initial_v1.md) | historical baseline | Deterministic engine, phase state machine, artifacts, claims, policy contract, and scope enforcement. | -| Validation coverage artifacts | Reference / Active | [validation-coverage.md](validation-coverage.md) | `ver-vyag`, `ver-rcgh`, `ver-y29o`, `ver-1qru`, `ver-ssp3` | Durable record for declared, derived, executed, skipped, repaired, and blocking checks. | +| Validation coverage artifacts | Reference / Implemented | [validation-coverage.md](validation-coverage.md) | `ver-vyag`, `ver-rcgh`, `ver-y29o`, `ver-1qru`, `ver-ssp3` | Durable record for declared, derived, executed, skipped, repaired, and blocking checks. | | Worker isolation | Active | [worker-isolation.md](worker-isolation.md) | `ver-wi0p`, `ver-wi01` through `ver-wi18` | Per-ticket git worktrees for parallel worker isolation, merge-back, verification CWD split, and cleanup. | ## Feature Tracks | Feature area | Status | Primary document | Related tickets | Scope | | --- | --- | --- | --- | --- | -| Repair-oriented run gates | Active | [2026-04-19-verk-run-repair-oriented-gates.md](2026-04-19-verk-run-repair-oriented-gates.md) | `ver-vyag`, `ver-rcgh`, `ver-laq2`, `ver-y29o`, `ver-1qru`, `ver-tidw`, `ver-amsh`, `ver-ssp3`, `ver-bks9`, `ver-mbvz`, `ver-aw4j` | Ticket, wave, and epic closeout should prefer repair over early blocking and should surface actionable blocker reasons. | +| Repair-oriented run gates | Implemented / Reference | [2026-04-19-verk-run-repair-oriented-gates.md](2026-04-19-verk-run-repair-oriented-gates.md) | `ver-vyag`, `ver-rcgh`, `ver-laq2`, `ver-y29o`, `ver-1qru`, `ver-tidw`, `ver-amsh`, `ver-ssp3`, `ver-bks9`, `ver-mbvz`, `ver-aw4j` | Ticket, wave, and epic closeout prefer repair over early blocking and surface actionable blocker reasons. | | Implementation and verification loop improvements | Planned / Reference | [2026-04-19-impl-verify-improvements.md](2026-04-19-impl-verify-improvements.md) | overlaps `ver-vyag` | Broader roadmap for the impl -> verify -> review -> repair loop, including intent echo, standards, validators, and reviewer gates. | | Per-worker review diffs | Active / Planned | [2026-04-20-per-worker-review-diffs.md](2026-04-20-per-worker-review-diffs.md) | no dedicated epic found in this index pass | Reviewers should inspect the current worker attempt's delta instead of the whole dirty worktree. | | Recursive sub-epic execution | Active | no dedicated plan found | `ver-vmgr` and children | Make sub-epics resumable, retryable, artifact-backed, depth-limited by scheduler policy, and safe when descendants block. | @@ -33,6 +35,7 @@ or ticket first, then update this file. | Verk as skill | Planned | [2026-04-19-verk-as-skill-cross-agent.md](2026-04-19-verk-as-skill-cross-agent.md) | no active epic found in this index pass | Claude Code skill-mode foundation for verk primitives and artifact-compatible execution. | | Skill host portability | Blocked | [2026-04-19-verk-skill-host-portability.md](2026-04-19-verk-skill-host-portability.md) | no active epic found in this index pass | Extend skill-mode support beyond Claude Code after the v1 skill surface is available. | | Ticket quality pre-run gate | Planned | [2026-04-21-ticket-quality-gate.md](2026-04-21-ticket-quality-gate.md) | no active epic found in this index pass | Needed before `verk run`: deterministic ticket lint, planner-role review, traceability checks, and safe auto-repair for underspecified tickets. | +| Ticket state machine outcomes | Active / Planned | [2026-04-22-ticket-state-machine.md](2026-04-22-ticket-state-machine.md) | no dedicated epic yet | Separate retryable failures, operator decisions, and true blockers so `blocked` becomes a last-resort state instead of a generic stop reason. | | Memory learning loop | Planned | [2026-04-21-memory-learning-loop.md](2026-04-21-memory-learning-loop.md) | no active epic found in this index pass | Repo-local escaped-defect memory, human-reviewed lesson promotion, and advisory feedback into ticket quality review. | | Anti-rationalization catalog | Planned | [Rationalizations.md](Rationalizations.md) | none | Detailed spec for P3 (impl-verify-improvements). Full catalog of 91 named rationalizations + verk-specific additions, with injection point mapping per worker phase. | | Agent profiles | Planned | [2026-04-21-agent-profiles.md](2026-04-21-agent-profiles.md) | none | Role-based worker profiles (security-engineer, contract-engineer, frontend-engineer, backend-engineer). Project-agnostic detection, `profile` frontmatter field, pre-run validation, rationalization injection per profile, and prompt placement. Full implementation of P3. | @@ -44,31 +47,30 @@ new escaped defect changes the risk profile. ### Current Priority Order -1. Stabilize active execution correctness: close `ver-wgxh` high-severity - findings and `ver-vmgr` recursive sub-epic execution issues first. -2. Finish the core repair-oriented run gates in - [2026-04-19-verk-run-repair-oriented-gates.md](2026-04-19-verk-run-repair-oriented-gates.md), - especially derived verification, repair routing, visible blocker reasons, - and epic closure behavior. -3. Land reviewer-scope correctness: prioritize +1. Land reviewer-scope correctness: prioritize [2026-04-20-per-worker-review-diffs.md](2026-04-20-per-worker-review-diffs.md), then [worker-isolation.md](worker-isolation.md). -4. Implement the deterministic and planner-reviewed ticket quality gate from +2. Land the ticket state-machine outcome model from + [2026-04-22-ticket-state-machine.md](2026-04-22-ticket-state-machine.md), because + retry/decision/block semantics affect every run failure path. +3. Implement the deterministic and planner-reviewed ticket quality gate from [2026-04-21-ticket-quality-gate.md](2026-04-21-ticket-quality-gate.md). -5. Add the advisory memory and learning loop from +4. Add the advisory memory and learning loop from [2026-04-21-memory-learning-loop.md](2026-04-21-memory-learning-loop.md). -6. Revisit benchmarking, skill packaging, and host portability after the core - run and quality gates are stable. +5. Revisit benchmarking, skill packaging, and host portability after the core + run, review-scope, and quality gates are stable. ### Parallel Work Guidance Safe parallel tracks: -- `ver-wgxh` high-severity remediation. -- `ver-vmgr` recursive sub-epic execution. - Per-worker review diffs. +- Worker-isolation git adapter and placement tasks, before touching shared + epic orchestration. - Deterministic ticket-quality evaluator and inspection CLI, before wiring it into `verk run`. +- Ticket state-machine artifact additions, while CLI interaction and scheduler + behavior remain serialized with other execution changes. - Memory loop storage and CLI skeleton, while keeping it advisory until ticket quality finding codes are stable. diff --git a/docs/plans/validation-coverage.md b/docs/plans/validation-coverage.md index c7bd8db..33a21ef 100644 --- a/docs/plans/validation-coverage.md +++ b/docs/plans/validation-coverage.md @@ -72,6 +72,12 @@ most recent entry based on `FinishedAt`. ## Blocked Closure +Advisory checks are coverage signals first. A failed advisory check may create a +best-effort repair cycle reference while ticket implementation attempts remain, +but it must not create an unresolved blocker by itself. Required checks and +reviewer/policy-promoted findings are the cases that make a coverage artifact +non-closable. + When a scope cannot close: 1. Set `Closable = false` on the `ValidationCoverageArtifact`. diff --git a/internal/adapters/runtime/claude/adapter.go b/internal/adapters/runtime/claude/adapter.go index 494554a..979470b 100644 --- a/internal/adapters/runtime/claude/adapter.go +++ b/internal/adapters/runtime/claude/adapter.go @@ -838,7 +838,9 @@ func defaultRunStreamingCommand(ctx context.Context, binary string, args []strin } if scanErr := scanner.Err(); scanErr != nil { - if cmd.Process != nil { + if cmd.Cancel != nil { + _ = cmd.Cancel() + } else if cmd.Process != nil { _ = cmd.Process.Kill() } _ = cmd.Wait() // wait for stderr copy goroutine to finish before reading stderr diff --git a/internal/adapters/runtime/claude/adapter_test.go b/internal/adapters/runtime/claude/adapter_test.go index c72884b..2c2d854 100644 --- a/internal/adapters/runtime/claude/adapter_test.go +++ b/internal/adapters/runtime/claude/adapter_test.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "os" + "os/exec" + goruntime "runtime" "strings" "testing" "time" @@ -17,14 +19,71 @@ import ( // When _CLAUDE_TEST_WRITE_HUGE_LINE=1, it writes a 2MB line to stdout so that // bufio.Scanner (1MB max) returns bufio.ErrTooLong — enabling a real scanner // error test without external tooling. +// When _CLAUDE_TEST_STREAM_JSON_OUTPUT=1, it writes minimal stream-json lines and +// exits normally. +// When _CLAUDE_TEST_WRITE_HUGE_SLEEP_MS is set, the helper pauses after writing +// the large line, which allows tests to confirm that the parent kills the child +// instead of waiting for normal completion. func TestMain(m *testing.M) { + if marker := os.Getenv("_CLAUDE_TEST_GRANDCHILD_MARKER"); marker != "" { + time.Sleep(300 * time.Millisecond) + _ = os.WriteFile(marker, []byte("grandchild-survived"), 0o644) + os.Exit(0) + } if os.Getenv("_CLAUDE_TEST_WRITE_HUGE_LINE") == "1" { + if marker := os.Getenv("_CLAUDE_TEST_SPAWN_GRANDCHILD_MARKER"); marker != "" { + cmd := exec.Command(os.Args[0], "-test.run=^$") + cmd.Env = claudeTestEnvWithout( + "_CLAUDE_TEST_WRITE_HUGE_LINE", + "_CLAUDE_TEST_WRITE_HUGE_LINE_PREFIX", + "_CLAUDE_TEST_WRITE_HUGE_SLEEP_MS", + "_CLAUDE_TEST_EXIT_MARKER", + "_CLAUDE_TEST_SPAWN_GRANDCHILD_MARKER", + ) + cmd.Env = append(cmd.Env, "_CLAUDE_TEST_GRANDCHILD_MARKER="+marker) + _ = cmd.Start() + time.Sleep(100 * time.Millisecond) + } + if os.Getenv("_CLAUDE_TEST_WRITE_HUGE_LINE_PREFIX") == "1" { + _, _ = fmt.Fprintln(os.Stdout, `{"type":"assistant","message":{"content":[{"type":"tool_use","name":"Read","input":{"file_path":"/tmp/test.txt"}}]}}`) + } _, _ = fmt.Fprintln(os.Stdout, strings.Repeat("x", 2*1024*1024)) + if delay := os.Getenv("_CLAUDE_TEST_WRITE_HUGE_SLEEP_MS"); delay == "1" { + time.Sleep(2 * time.Second) + } + if marker := os.Getenv("_CLAUDE_TEST_EXIT_MARKER"); marker != "" { + _ = os.WriteFile(marker, []byte("normal-exit"), 0o644) + } + os.Exit(0) + } + if os.Getenv("_CLAUDE_TEST_STREAM_JSON_OUTPUT") == "1" { + _, _ = fmt.Fprintln(os.Stdout, `{"type":"assistant","message":{"content":[{"type":"tool_use","name":"Read","input":{"file_path":"/tmp/test.txt"}}]}}`) + _, _ = fmt.Fprintln(os.Stdout, `{"type":"result","is_error":false,"subtype":"success","result":"{\"status\":\"done\"}","duration_ms":5,"num_turns":1}`) + _, _ = fmt.Fprintln(os.Stderr, "streaming stderr") os.Exit(0) } os.Exit(m.Run()) } +func claudeTestEnvWithout(keys ...string) []string { + blocked := make(map[string]struct{}, len(keys)) + for _, key := range keys { + blocked[key] = struct{}{} + } + env := os.Environ() + filtered := env[:0] + for _, value := range env { + key, _, ok := strings.Cut(value, "=") + if ok { + if _, remove := blocked[key]; remove { + continue + } + } + filtered = append(filtered, value) + } + return filtered +} + // mockCLIOutput builds a JSON response matching `claude -p --output-format json`. func mockCLIOutput(resultText string, isError bool) []byte { output := runtime.CLIOutputJSON{ @@ -500,7 +559,23 @@ func TestBuildReviewArgs_IncludesModelWhenSet(t *testing.T) { // cmd.Wait(). The subprocess is this test binary itself, re-invoked with // _CLAUDE_TEST_WRITE_HUGE_LINE=1 (handled by TestMain above). func TestRunStreamingCommand_ScannerError(t *testing.T) { - env := append(os.Environ(), "_CLAUDE_TEST_WRITE_HUGE_LINE=1") + marker, err := os.CreateTemp("", "claude-stream-scan-marker-*") + if err != nil { + t.Fatalf("create temp marker: %v", err) + } + defer os.Remove(marker.Name()) + + // Ensure marker is absent unless the child exits cleanly. + _ = marker.Close() + _ = os.Remove(marker.Name()) + + env := append(os.Environ(), + "_CLAUDE_TEST_WRITE_HUGE_LINE=1", + "_CLAUDE_TEST_WRITE_HUGE_LINE_PREFIX=1", + "_CLAUDE_TEST_WRITE_HUGE_SLEEP_MS=1", + "_CLAUDE_TEST_EXIT_MARKER="+marker.Name(), + ) + start := time.Now() result, err := defaultRunStreamingCommand( context.Background(), os.Args[0], @@ -510,14 +585,125 @@ func TestRunStreamingCommand_ScannerError(t *testing.T) { 10*time.Second, nil, ) + elapsed := time.Since(start) if err == nil { t.Fatal("expected scanner error, got nil") } if !errors.Is(err, bufio.ErrTooLong) { t.Fatalf("expected error wrapping bufio.ErrTooLong, got: %v", err) } - // Partial output may be empty (the oversized token was never delivered) but - // the result must not be nil and stderr is accessible for debugging. + if elapsed > 1500*time.Millisecond { + t.Fatalf("scan error path should return quickly to avoid deadlock; took %s", elapsed) + } + if _, statErr := os.Stat(marker.Name()); statErr == nil { + t.Fatalf("expected marker to remain absent when process is killed") + } else if !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("expected marker to remain absent when process is killed: %v", statErr) + } + // The oversized token should still preserve already-read output for debugging. + if len(result.stdout) == 0 { + t.Fatalf("expected partial stdout to be preserved after scan error") + } + if len(result.stderr) != 0 { + t.Fatalf("unexpected stderr from helper process: %q", string(result.stderr)) + } _ = result.stdout _ = result.stderr } + +func TestRunStreamingCommand_ScannerErrorKillsProcessGroup(t *testing.T) { + if !claudeTestSupportsProcessGroupKill() { + t.Skip("process-group cancellation is only supported on Unix platforms") + } + + marker, err := os.CreateTemp("", "claude-stream-grandchild-marker-*") + if err != nil { + t.Fatalf("create temp marker: %v", err) + } + markerPath := marker.Name() + if err := marker.Close(); err != nil { + t.Fatalf("close marker: %v", err) + } + if err := os.Remove(markerPath); err != nil { + t.Fatalf("remove marker: %v", err) + } + + env := append(os.Environ(), + "_CLAUDE_TEST_WRITE_HUGE_LINE=1", + "_CLAUDE_TEST_WRITE_HUGE_SLEEP_MS=1", + "_CLAUDE_TEST_SPAWN_GRANDCHILD_MARKER="+markerPath, + ) + _, err = defaultRunStreamingCommand( + context.Background(), + os.Args[0], + []string{"-test.run=^$"}, + nil, + env, + 10*time.Second, + nil, + ) + if err == nil { + t.Fatal("expected scanner error, got nil") + } + if !errors.Is(err, bufio.ErrTooLong) { + t.Fatalf("expected error wrapping bufio.ErrTooLong, got: %v", err) + } + + time.Sleep(800 * time.Millisecond) + if _, statErr := os.Stat(markerPath); statErr == nil { + t.Fatalf("expected process-group kill to terminate grandchild before marker write") + } else if !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("expected marker to remain absent when process group is killed: %v", statErr) + } +} + +func claudeTestSupportsProcessGroupKill() bool { + switch goruntime.GOOS { + case "aix", "android", "darwin", "dragonfly", "freebsd", "hurd", "illumos", "ios", "linux", "netbsd", "openbsd", "solaris": + return true + default: + return false + } +} + +func TestRunStreamingCommand_NormalCompletion_SelfBinary(t *testing.T) { + var sawProgress bool + env := append(os.Environ(), "_CLAUDE_TEST_STREAM_JSON_OUTPUT=1") + result, err := defaultRunStreamingCommand( + context.Background(), + os.Args[0], + []string{"-test.run=^$"}, // no tests match; TestMain writes stream-json and exits + nil, + env, + 10*time.Second, + func(msg string) { + sawProgress = true + if msg == "" { + t.Fatalf("expected non-empty progress message") + } + }, + ) + if err != nil { + t.Fatalf("expected normal completion, got error: %v", err) + } + if result.exitCode != 0 { + t.Fatalf("expected exitCode 0, got %d", result.exitCode) + } + if len(result.stdout) == 0 { + t.Fatalf("expected result stdout to be present") + } + if string(result.stderr) != "streaming stderr\n" { + t.Fatalf("unexpected stderr capture: %q", string(result.stderr)) + } + if !sawProgress { + t.Fatalf("expected onProgress callback for assistant tool_use event") + } + + var cliOut runtime.CLIOutputJSON + if err := json.Unmarshal(result.stdout, &cliOut); err != nil { + t.Fatalf("result stdout should be cli json: %v", err) + } + if cliOut.Type != "result" || cliOut.Subtype != "success" || cliOut.Result != `{"status":"done"}` { + t.Fatalf("unexpected cli output: %#v", cliOut) + } +} diff --git a/internal/adapters/runtime/normalize.go b/internal/adapters/runtime/normalize.go index a8c8e7d..63cdd26 100644 --- a/internal/adapters/runtime/normalize.go +++ b/internal/adapters/runtime/normalize.go @@ -3,13 +3,37 @@ package runtime import "strings" // NormalizeKey lowercases and converts hyphens/spaces to underscores, -// producing a canonical key form for switch-based matching. +// producing a canonical key form for map-based matching. func NormalizeKey(raw string) string { raw = strings.TrimSpace(strings.ToLower(raw)) replacer := strings.NewReplacer("-", "_", " ", "_") return replacer.Replace(raw) } +var workerStatusVariants = map[string]WorkerStatus{ + // WorkerStatusDone + "done": WorkerStatusDone, + "completed": WorkerStatusDone, + "complete": WorkerStatusDone, + "success": WorkerStatusDone, + "passed": WorkerStatusDone, + "ok": WorkerStatusDone, + // WorkerStatusDoneWithConcerns + "done_with_concerns": WorkerStatusDoneWithConcerns, + "donewithconcerns": WorkerStatusDoneWithConcerns, + "concerns": WorkerStatusDoneWithConcerns, + // WorkerStatusNeedsContext + "needs_context": WorkerStatusNeedsContext, + "needscontext": WorkerStatusNeedsContext, + "context_needed": WorkerStatusNeedsContext, + "needs_more_context": WorkerStatusNeedsContext, + "needsmorecontext": WorkerStatusNeedsContext, + // WorkerStatusBlocked + "blocked": WorkerStatusBlocked, + "blocked_by_operator_input": WorkerStatusBlocked, + "blockedbyoperatorinput": WorkerStatusBlocked, +} + // NormalizeWorkerStatusString maps raw status strings — including common // synonyms and spelling variants (hyphenated, underscored, camelCase-collapsed) // — to canonical WorkerStatus values. @@ -17,16 +41,6 @@ func NormalizeKey(raw string) string { // Returns the canonical status and true if the raw value was recognized, // or ("", false) otherwise. func NormalizeWorkerStatusString(raw string) (WorkerStatus, bool) { - switch NormalizeKey(raw) { - case "done", "completed", "complete", "success", "passed", "ok": - return WorkerStatusDone, true - case "done_with_concerns", "donewithconcerns", "concerns": - return WorkerStatusDoneWithConcerns, true - case "needs_context", "needscontext", "context_needed", "needs_more_context", "needsmorecontext": - return WorkerStatusNeedsContext, true - case "blocked", "blocked_by_operator_input", "blockedbyoperatorinput": - return WorkerStatusBlocked, true - default: - return "", false - } + status, ok := workerStatusVariants[NormalizeKey(raw)] + return status, ok } diff --git a/internal/adapters/runtime/prompt.go b/internal/adapters/runtime/prompt.go index 2c89280..c070892 100644 --- a/internal/adapters/runtime/prompt.go +++ b/internal/adapters/runtime/prompt.go @@ -240,44 +240,68 @@ func ParseResultBlock(text string) (VerkResultBlock, bool) { // 1. Direct parse — the AI returned only JSON as instructed. var block VerkResultBlock - if err := json.Unmarshal([]byte(text), &block); err == nil && block.Status != "" { - return block, true + if err := json.Unmarshal([]byte(text), &block); err == nil { + return finalizeResultBlock(block) } // 2. Sentinel-prefixed line fallback. - if b, ok := parseSentinelLine[VerkResultBlock](text, ResultSentinel); ok && ValidateWorkerStatus(WorkerStatus(b.Status)) == nil { - return b, true + if b, ok := parseSentinelLine[VerkResultBlock](text, ResultSentinel); ok { + return finalizeResultBlock(b) } // 3. Last JSON object fallback. - if b, ok := parseLastJSON[VerkResultBlock](stripSentinelLines(text, ResultSentinel)); ok && b.Status != "" { - return b, true + if b, ok := parseLastJSON[VerkResultBlock](stripSentinelLines(text, ResultSentinel)); ok { + return finalizeResultBlock(b) } return VerkResultBlock{}, false } +func finalizeResultBlock(b VerkResultBlock) (VerkResultBlock, bool) { + if b.Status == "" { + return VerkResultBlock{}, false + } + status, ok := NormalizeWorkerStatusString(b.Status) + if !ok { + return VerkResultBlock{}, false + } + b.Status = string(status) + return b, true +} + // ParseReviewBlock extracts a VerkReviewBlock from AI output. // Same three-strategy approach as ParseResultBlock. func ParseReviewBlock(text string) (VerkReviewBlock, bool) { text = strings.TrimSpace(text) var block VerkReviewBlock - if err := json.Unmarshal([]byte(text), &block); err == nil && block.ReviewStatus != "" { - return block, true + if err := json.Unmarshal([]byte(text), &block); err == nil { + return finalizeReviewBlock(block) } - if b, ok := parseSentinelLine[VerkReviewBlock](text, ReviewSentinel); ok && ValidateReviewStatus(ReviewStatus(b.ReviewStatus)) == nil { - return b, true + if b, ok := parseSentinelLine[VerkReviewBlock](text, ReviewSentinel); ok { + return finalizeReviewBlock(b) } - if b, ok := parseLastJSON[VerkReviewBlock](stripSentinelLines(text, ReviewSentinel)); ok && b.ReviewStatus != "" { - return b, true + if b, ok := parseLastJSON[VerkReviewBlock](stripSentinelLines(text, ReviewSentinel)); ok { + return finalizeReviewBlock(b) } return VerkReviewBlock{}, false } +func finalizeReviewBlock(b VerkReviewBlock) (VerkReviewBlock, bool) { + switch NormalizeKey(b.ReviewStatus) { + case string(ReviewStatusPassed): + b.ReviewStatus = string(ReviewStatusPassed) + case string(ReviewStatusFindings): + b.ReviewStatus = string(ReviewStatusFindings) + default: + return VerkReviewBlock{}, false + } + return b, true +} + // parseSentinelLine scans for a line starting with the given prefix and parses // the remainder as JSON. func parseSentinelLine[T any](text, prefix string) (T, bool) { diff --git a/internal/adapters/runtime/prompt_test.go b/internal/adapters/runtime/prompt_test.go index 8cf4601..1d494f2 100644 --- a/internal/adapters/runtime/prompt_test.go +++ b/internal/adapters/runtime/prompt_test.go @@ -24,6 +24,56 @@ func TestParseResultBlock_DirectJSON(t *testing.T) { } } +func TestParseResultBlock_ValidatesAndNormalizesAllParsePaths(t *testing.T) { + tests := []struct { + name string + input string + wantFound bool + wantStatus string + }{ + { + name: "direct JSON normalizes synonym", + input: `{"status":"completed","completion_code":"ok"}`, + wantFound: true, + wantStatus: "done", + }, + { + name: "sentinel normalizes hyphenated status", + input: `VERK_RESULT:{"status":"done-with-concerns","completion_code":"ok"}`, + wantFound: true, + wantStatus: "done_with_concerns", + }, + { + name: "last JSON normalizes spaced status", + input: "Result follows:\n{\"status\":\"needs more context\",\"block_reason\":\"missing repo\"}", + wantFound: true, + wantStatus: "needs_context", + }, + { + name: "direct JSON rejects invalid status", + input: `{"status":"finished","completion_code":"ok"}`, + wantFound: false, + }, + { + name: "last JSON rejects invalid status", + input: "Result follows:\n{\"status\":\"finished\",\"completion_code\":\"ok\"}", + wantFound: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + block, found := ParseResultBlock(tt.input) + if found != tt.wantFound { + t.Fatalf("found=%v, want %v", found, tt.wantFound) + } + if found && block.Status != tt.wantStatus { + t.Fatalf("status=%q, want %q", block.Status, tt.wantStatus) + } + }) + } +} + func TestParseResultBlock_SentinelLine(t *testing.T) { text := `Some extra prose the AI shouldn't have written. VERK_RESULT:{"status":"done","completion_code":"ok"}` @@ -122,6 +172,56 @@ func TestParseReviewBlock_Passed(t *testing.T) { } } +func TestParseReviewBlock_ValidatesAndNormalizesAllParsePaths(t *testing.T) { + tests := []struct { + name string + input string + wantFound bool + wantStatus string + }{ + { + name: "direct JSON normalizes case", + input: `{"review_status":"PASSED","summary":"all good","findings":[]}`, + wantFound: true, + wantStatus: "passed", + }, + { + name: "sentinel accepts findings", + input: `VERK_REVIEW:{"review_status":"findings","summary":"needs work","findings":[]}`, + wantFound: true, + wantStatus: "findings", + }, + { + name: "last JSON normalizes spaces", + input: "Review follows:\n{\"review_status\":\" Passed \",\"summary\":\"ok\",\"findings\":[]}", + wantFound: true, + wantStatus: "passed", + }, + { + name: "direct JSON rejects invalid status", + input: `{"review_status":"clean","summary":"ok","findings":[]}`, + wantFound: false, + }, + { + name: "last JSON rejects invalid status", + input: "Review follows:\n{\"review_status\":\"clean\",\"summary\":\"ok\",\"findings\":[]}", + wantFound: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + block, found := ParseReviewBlock(tt.input) + if found != tt.wantFound { + t.Fatalf("found=%v, want %v", found, tt.wantFound) + } + if found && block.ReviewStatus != tt.wantStatus { + t.Fatalf("review_status=%q, want %q", block.ReviewStatus, tt.wantStatus) + } + }) + } +} + func TestParseReviewBlock_SentinelLine(t *testing.T) { text := `Some preamble VERK_REVIEW:{"review_status":"passed","summary":"clean","findings":[]}` diff --git a/internal/adapters/ticketstore/tkmd/claims.go b/internal/adapters/ticketstore/tkmd/claims.go index 6e0eb44..01b6a70 100644 --- a/internal/adapters/ticketstore/tkmd/claims.go +++ b/internal/adapters/ticketstore/tkmd/claims.go @@ -16,6 +16,9 @@ const ( defaultClaimTTL = 30 * time.Minute ) +// ErrInvalidIdentifier marks claim identifiers rejected before filesystem access. +var ErrInvalidIdentifier = errors.New("invalid identifier") + // saveAtomic is the atomic JSON save function used by RenewClaim. // Package-internal; overridable in tests. var saveAtomic = state.SaveJSONAtomic @@ -357,6 +360,12 @@ func parseAcquireClaimRequest(rootDir string, args ...any) (acquireClaimRequest, } req.runID = runID req.ticketID = ticketID + if err := validateClaimIdentifier(req.runID, "run_id"); err != nil { + return req, err + } + if err := validateClaimIdentifier(req.ticketID, "ticket_id"); err != nil { + return req, err + } nextString := 2 for _, arg := range args[2:] { @@ -422,6 +431,12 @@ func parseRenewClaimRequest(rootDir string, args ...any) (renewClaimRequest, err } req.runID = runID req.ticketID = ticketID + if err := validateClaimIdentifier(req.runID, "run_id"); err != nil { + return req, err + } + if err := validateClaimIdentifier(req.ticketID, "ticket_id"); err != nil { + return req, err + } if leaseID, ok := args[2].(string); ok && leaseID != "" { req.leaseID = leaseID @@ -468,6 +483,12 @@ func parseReleaseClaimRequest(rootDir string, args ...any) (releaseClaimRequest, } req.runID = runID req.ticketID = ticketID + if err := validateClaimIdentifier(req.runID, "run_id"); err != nil { + return req, err + } + if err := validateClaimIdentifier(req.ticketID, "ticket_id"); err != nil { + return req, err + } switch len(args) - 2 { case 0: @@ -528,33 +549,165 @@ func claimPaths(rootDir, runID, ticketID string) (string, string, error) { } repoRoot := resolveRepoRoot(rootDir) ticketsDir := resolveTicketsDir(rootDir) - livePath := filepath.Join(ticketsDir, ".claims", ticketID+".json") - durablePath := filepath.Join(repoRoot, ".verk", "runs", runID, "claims", "claim-"+ticketID+".json") + liveDir := filepath.Join(ticketsDir, ".claims") + verkDir := filepath.Join(repoRoot, ".verk") + durableDir := filepath.Join(verkDir, "runs", runID, "claims") + livePath := filepath.Join(liveDir, ticketID+".json") + durablePath := filepath.Join(durableDir, "claim-"+ticketID+".json") + if err := assertPathUnderIntendedBase(liveDir, ticketsDir); err != nil { + return "", "", err + } + if err := assertPathUnderIntendedBase(durableDir, verkDir); err != nil { + return "", "", err + } + if err := os.MkdirAll(liveDir, 0o755); err != nil { + return "", "", fmt.Errorf("create claim dir: %w", err) + } + if err := os.MkdirAll(durableDir, 0o755); err != nil { + return "", "", fmt.Errorf("create durable claim dir: %w", err) + } + if err := assertPathUnderBase(livePath, liveDir); err != nil { + return "", "", err + } + if err := assertPathUnderBase(durablePath, durableDir); err != nil { + return "", "", err + } return livePath, durablePath, nil } +func assertPathUnderBase(target, base string) error { + cleanTarget := filepath.Clean(target) + cleanBase := filepath.Clean(base) + resolvedBase, err := resolveBaseForContainment(cleanBase) + if err != nil { + return fmt.Errorf("claim path resolution failed: resolve base %q: %w", cleanBase, err) + } + resolvedTarget, err := resolvePathForContainment(cleanTarget) + if err != nil { + return fmt.Errorf("claim path resolution failed: resolve target %q: %w", cleanTarget, err) + } + rel, err := filepath.Rel(resolvedBase, resolvedTarget) + 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") + } + return nil +} + +func assertPathUnderIntendedBase(target, base string) error { + cleanTarget := filepath.Clean(target) + cleanBase := filepath.Clean(base) + resolvedBase, err := resolveIntendedBaseForContainment(cleanBase) + if err != nil { + return fmt.Errorf("claim path resolution failed: resolve base %q: %w", cleanBase, err) + } + resolvedTarget, err := resolvePathForContainmentAllowMissingAncestors(cleanTarget) + if err != nil { + return fmt.Errorf("claim path resolution failed: resolve target %q: %w", cleanTarget, err) + } + rel, err := filepath.Rel(resolvedBase, resolvedTarget) + 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") + } + return nil +} + +func resolveBaseForContainment(base string) (string, error) { + if _, err := filepath.EvalSymlinks(base); err != nil { + return "", err + } + return resolveIntendedBaseForContainment(base) +} + +func resolveIntendedBaseForContainment(base string) (string, error) { + parent, err := filepath.EvalSymlinks(filepath.Dir(base)) + if err != nil { + return "", err + } + return filepath.Join(parent, filepath.Base(base)), nil +} + +func resolvePathForContainment(path string) (string, error) { + resolved, err := filepath.EvalSymlinks(path) + if err == nil { + return resolved, nil + } + if !os.IsNotExist(err) { + return "", err + } + parent, parentErr := filepath.EvalSymlinks(filepath.Dir(path)) + if parentErr != nil { + return "", parentErr + } + return filepath.Join(parent, filepath.Base(path)), nil +} + +func resolvePathForContainmentAllowMissingAncestors(path string) (string, error) { + cleaned := filepath.Clean(path) + var missingErr error + for current := cleaned; ; current = filepath.Dir(current) { + _, err := os.Lstat(current) + if err == nil { + resolved, evalErr := filepath.EvalSymlinks(current) + if evalErr != nil { + return "", evalErr + } + rel, relErr := filepath.Rel(current, cleaned) + if relErr != nil { + return "", relErr + } + if rel == "." { + return resolved, nil + } + return filepath.Join(resolved, rel), nil + } + if !isPathNotExist(err) { + return "", err + } + missingErr = err + parent := filepath.Dir(current) + if parent == current { + return "", missingErr + } + } +} + +func isPathNotExist(err error) bool { + if err == nil { + return false + } + return os.IsNotExist(err) || + errors.Is(err, os.ErrNotExist) || + strings.Contains(strings.ToLower(err.Error()), "no such file or directory") +} + // validateClaimIdentifier rejects identifiers that could escape the intended // claim storage directories via path traversal, absolute paths, or embedded // path separators. A valid identifier is a single path component with no // directory semantics. func validateClaimIdentifier(id, label string) error { if id == "" { - return fmt.Errorf("%s is required", label) + return fmt.Errorf("%w: %s is required", ErrInvalidIdentifier, label) } if id == "." || id == ".." { - return fmt.Errorf("%s contains path traversal", label) + return fmt.Errorf("%w: %s contains path traversal", ErrInvalidIdentifier, label) } if filepath.IsAbs(id) { - return fmt.Errorf("%s contains absolute path", label) + return fmt.Errorf("%w: %s contains absolute path", ErrInvalidIdentifier, label) } if strings.Contains(id, "..") { - return fmt.Errorf("%s contains path traversal", label) + return fmt.Errorf("%w: %s contains path traversal", ErrInvalidIdentifier, label) } if strings.ContainsAny(id, "/\\") { - return fmt.Errorf("%s contains path separator", label) + return fmt.Errorf("%w: %s contains path separator", ErrInvalidIdentifier, label) } if cleaned := filepath.Clean(id); cleaned != id { - return fmt.Errorf("%s is not a clean identifier", label) + return fmt.Errorf("%w: %s is not a clean identifier", ErrInvalidIdentifier, label) } return nil } diff --git a/internal/adapters/ticketstore/tkmd/claims_test.go b/internal/adapters/ticketstore/tkmd/claims_test.go index 0f346b1..4d6dd82 100644 --- a/internal/adapters/ticketstore/tkmd/claims_test.go +++ b/internal/adapters/ticketstore/tkmd/claims_test.go @@ -263,13 +263,92 @@ func TestAcquireClaim_RejectsPathTraversalIdentifiers(t *testing.T) { for _, tc := range maliciousIDs { t.Run(tc.name, func(t *testing.T) { _, err := AcquireClaim(dir, tc.runID, tc.ticketID, "lease-x", 10*time.Minute) - if err == nil { - t.Fatalf("expected claim to be rejected for %s", tc.name) - } + assertInvalidIdentifierError(t, err, "claim", tc.name) }) } } +func TestRenewClaim_RejectsPathTraversalIdentifiers(t *testing.T) { + dir := t.TempDir() + + maliciousIDs := []struct { + name string + runID string + ticketID string + }{ + {"dotdot in runID", "../escape", "ticket-1"}, + {"dotdot in ticketID", "run-a", "../escape"}, + {"slash in runID", "run/evil", "ticket-1"}, + {"slash in ticketID", "run-a", "ticket/evil"}, + {"backslash in runID", "run\\evil", "ticket-1"}, + {"backslash in ticketID", "run-a", "ticket\\evil"}, + {"absolute runID", "/etc/passwd", "ticket-1"}, + {"absolute ticketID", "run-a", "/etc/passwd"}, + {"dotdot with slash in runID", "../../etc", "ticket-1"}, + {"dotdot with slash in ticketID", "run-a", "../../etc"}, + {"embedded dotdot in runID", "foo/../bar", "ticket-1"}, + {"embedded dotdot in ticketID", "run-a", "foo/../bar"}, + {"single dot runID", ".", "ticket-1"}, + {"single dot ticketID", "run-a", "."}, + {"double dot runID", "..", "ticket-1"}, + {"double dot ticketID", "run-a", ".."}, + } + + for _, tc := range maliciousIDs { + t.Run(tc.name, func(t *testing.T) { + _, err := RenewClaim(dir, tc.runID, tc.ticketID, "lease-x", 10*time.Minute) + assertInvalidIdentifierError(t, err, "renew", tc.name) + }) + } +} + +func TestReleaseClaim_RejectsPathTraversalIdentifiers(t *testing.T) { + dir := t.TempDir() + + maliciousIDs := []struct { + name string + runID string + ticketID string + }{ + {"dotdot in runID", "../escape", "ticket-1"}, + {"dotdot in ticketID", "run-a", "../escape"}, + {"slash in runID", "run/evil", "ticket-1"}, + {"slash in ticketID", "run-a", "ticket/evil"}, + {"backslash in runID", "run\\evil", "ticket-1"}, + {"backslash in ticketID", "run-a", "ticket\\evil"}, + {"absolute runID", "/etc/passwd", "ticket-1"}, + {"absolute ticketID", "run-a", "/etc/passwd"}, + {"dotdot with slash in runID", "../../etc", "ticket-1"}, + {"dotdot with slash in ticketID", "run-a", "../../etc"}, + {"embedded dotdot in runID", "foo/../bar", "ticket-1"}, + {"embedded dotdot in ticketID", "run-a", "foo/../bar"}, + {"single dot runID", ".", "ticket-1"}, + {"single dot ticketID", "run-a", "."}, + {"double dot runID", "..", "ticket-1"}, + {"double dot ticketID", "run-a", ".."}, + } + + for _, tc := range maliciousIDs { + t.Run(tc.name, func(t *testing.T) { + err := ReleaseClaim(dir, tc.runID, tc.ticketID, "lease-x", "test") + assertInvalidIdentifierError(t, err, "release", tc.name) + }) + } +} + +func assertInvalidIdentifierError(t *testing.T, err error, operation, name string) { + t.Helper() + if err == nil { + t.Fatalf("expected %s to be rejected for %s", operation, name) + } + if !errors.Is(err, ErrInvalidIdentifier) { + t.Fatalf("expected invalid identifier error for %s %s, got %v", operation, name, err) + } + if strings.Contains(err.Error(), "not found") { + t.Fatalf("expected upfront validation error for %s %s, got %v", operation, name, err) + } +} + func TestClaimPaths_PreservesValidIdentifiers(t *testing.T) { dir := t.TempDir() @@ -295,10 +374,123 @@ func TestClaimPaths_PreservesValidIdentifiers(t *testing.T) { if livePath == "" || durablePath == "" { t.Fatal("expected non-empty paths") } + liveRoot := filepath.Join(filepath.Clean(dir), ".tickets", ".claims") + durableRoot := filepath.Join(filepath.Clean(dir), ".verk", "runs", tc.runID, "claims") + assertPathWithin(t, livePath, liveRoot) + assertPathWithin(t, durablePath, durableRoot) }) } } +func TestClaimPaths_RejectsSymlinkEscapingBase(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, dir string) + }{ + { + name: "tickets dir ancestor symlink", + setup: func(t *testing.T, dir string) { + t.Helper() + outside := t.TempDir() + createSymlinkOrSkip(t, outside, filepath.Join(dir, ".tickets")) + }, + }, + { + name: "live claim dir symlink", + setup: func(t *testing.T, dir string) { + t.Helper() + ticketsDir := filepath.Join(dir, ".tickets") + outside := filepath.Join(dir, "outside-live") + if err := os.MkdirAll(ticketsDir, 0o755); err != nil { + t.Fatalf("mkdir tickets dir: %v", err) + } + if err := os.MkdirAll(outside, 0o755); err != nil { + t.Fatalf("mkdir outside dir: %v", err) + } + createSymlinkOrSkip(t, outside, filepath.Join(ticketsDir, ".claims")) + }, + }, + { + name: "verk dir ancestor symlink", + setup: func(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".tickets"), 0o755); err != nil { + t.Fatalf("mkdir tickets dir: %v", err) + } + outside := t.TempDir() + if err := os.MkdirAll(filepath.Join(outside, "runs", "run-a"), 0o755); err != nil { + t.Fatalf("mkdir outside run dir: %v", err) + } + createSymlinkOrSkip(t, outside, filepath.Join(dir, ".verk")) + }, + }, + { + name: "verk runs dir ancestor symlink", + setup: func(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".tickets"), 0o755); err != nil { + t.Fatalf("mkdir tickets dir: %v", err) + } + if err := os.MkdirAll(filepath.Join(dir, ".verk"), 0o755); err != nil { + t.Fatalf("mkdir verk dir: %v", err) + } + outside := t.TempDir() + if err := os.MkdirAll(filepath.Join(outside, "run-a"), 0o755); err != nil { + t.Fatalf("mkdir outside run dir: %v", err) + } + createSymlinkOrSkip(t, outside, filepath.Join(dir, ".verk", "runs")) + }, + }, + { + name: "durable claim dir symlink", + setup: func(t *testing.T, dir string) { + t.Helper() + durableParent := filepath.Join(dir, ".verk", "runs", "run-a") + outside := filepath.Join(dir, "outside-durable") + if err := os.MkdirAll(durableParent, 0o755); err != nil { + t.Fatalf("mkdir durable parent: %v", err) + } + if err := os.MkdirAll(outside, 0o755); err != nil { + t.Fatalf("mkdir outside dir: %v", err) + } + createSymlinkOrSkip(t, outside, filepath.Join(durableParent, "claims")) + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + tc.setup(t, dir) + _, _, err := claimPaths(dir, "run-a", "ticket-1") + if err == nil { + t.Fatal("expected symlink escape to be rejected") + } + if !strings.Contains(err.Error(), "claim path escapes base directory") { + t.Fatalf("expected escape error, got %v", err) + } + }) + } +} + +func createSymlinkOrSkip(t *testing.T, target, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not supported: %v", err) + } +} + +func assertPathWithin(t *testing.T, child, parent string) { + t.Helper() + rel, err := filepath.Rel(filepath.Clean(parent), filepath.Clean(child)) + if err != nil { + t.Fatalf("resolve relative path from %q to %q: %v", parent, child, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("path %q is not within base %q (rel=%q)", child, parent, rel) + } +} + func TestValidateLeaseFence_RejectsLateResult(t *testing.T) { if err := ValidateLeaseFence("lease-current", "lease-old"); err == nil { t.Fatal("expected mismatched lease fence to fail") @@ -364,10 +556,12 @@ func TestRenewClaim_DurableWriteFailure_LiveRestored(t *testing.T) { // before the other) and do not trigger the race detector. func TestConcurrent_AcquireAndRenew_NoRace(t *testing.T) { dir := t.TempDir() - - acquired, err := AcquireClaim(dir, "run-a", "ticket-1", "lease-a", 30*time.Minute) + livePath, _, err := claimPaths(dir, "run-a", "ticket-1") if err != nil { - t.Fatalf("AcquireClaim setup: %v", err) + t.Fatalf("claimPaths: %v", err) + } + if err := os.MkdirAll(filepath.Dir(livePath), 0o755); err != nil { + t.Fatalf("prepare claim lock directory: %v", err) } start := make(chan struct{}) @@ -377,32 +571,72 @@ func TestConcurrent_AcquireAndRenew_NoRace(t *testing.T) { var wg sync.WaitGroup wg.Add(2) - // Goroutine 1: renew the existing claim held by run-a. + // Goroutine 1: attempt to renew the claim for run-a. go func() { defer wg.Done() <-start - _, err := RenewClaim(dir, "run-a", "ticket-1", acquired.LeaseID, 30*time.Minute) + _, err := RenewClaim(dir, "run-a", "ticket-1", "lease-a", 30*time.Minute) renewErr <- err }() - // Goroutine 2: attempt to acquire the same ticket for run-b. - // run-a still holds an active claim, so this must fail. + // Goroutine 2: attempt to acquire the same ticket for run-a. + // One of two serial outcomes is valid: + // - acquire then renew: both succeed + // - renew then acquire: acquire succeeds, renew fails as not-found go func() { defer wg.Done() <-start - _, err := AcquireClaim(dir, "run-b", "ticket-1", "lease-b", 30*time.Minute) + _, err := AcquireClaim(dir, "run-a", "ticket-1", "lease-a", 30*time.Minute) acquireErr <- err }() close(start) wg.Wait() - // Renew must succeed; acquire must fail because run-a still holds the claim. - if err := <-renewErr; err != nil { - t.Errorf("RenewClaim failed unexpectedly: %v", err) + reqAcquireErr := <-acquireErr + reqRenewErr := <-renewErr + + _, durablePath, err := claimPaths(dir, "run-a", "ticket-1") + if err != nil { + t.Fatalf("claimPaths: %v", err) + } + live, err := loadClaimArtifact(livePath) + if err != nil { + t.Fatalf("load live claim: %v", err) } - if err := <-acquireErr; err == nil { - t.Error("AcquireClaim by run-b succeeded while run-a still holds claim") + durable, err := loadClaimArtifact(durablePath) + if err != nil { + t.Fatalf("load durable claim: %v", err) + } + + if reqAcquireErr != nil { + t.Fatalf("Acquire should never lose when both operations race from no-claim state: %v", reqAcquireErr) + } + + switch { + case reqRenewErr == nil: + if live == nil || durable == nil { + t.Fatal("expected both claim files to exist after acquire-then-renew") + } + if live.State != "active" || durable.State != "active" { + t.Fatalf("expected active state in both files after acquire-then-renew, got live=%q durable=%q", live.State, durable.State) + } + if live.LeaseID != "lease-a" || durable.LeaseID != "lease-a" { + t.Fatalf("expected lease-id to remain lease-a, got live=%q durable=%q", live.LeaseID, durable.LeaseID) + } + case reqRenewErr != nil && strings.Contains(reqRenewErr.Error(), "not found for renewal"): + if live == nil || durable == nil { + t.Fatal("expected active claim file for successful acquire") + } + if live.State != "active" || durable.State != "active" { + t.Fatalf("expected both claim files to be active after renew-before-acquire, got live=%q durable=%q", live.State, durable.State) + } + default: + t.Fatalf("expected acquire-then-renew or renew-then-acquire, got acquireErr=%v renewErr=%v", reqAcquireErr, reqRenewErr) + } + + if reqRenewErr != nil && !strings.Contains(reqRenewErr.Error(), "not found for renewal") { + t.Fatalf("unexpected RenewClaim failure: %v", reqRenewErr) } } @@ -411,10 +645,12 @@ func TestConcurrent_AcquireAndRenew_NoRace(t *testing.T) { // and do not trigger the race detector. func TestConcurrent_AcquireAndRelease_NoRace(t *testing.T) { dir := t.TempDir() - - acquired, err := AcquireClaim(dir, "run-a", "ticket-1", "lease-a", 30*time.Minute) + livePath, _, err := claimPaths(dir, "run-a", "ticket-1") if err != nil { - t.Fatalf("AcquireClaim setup: %v", err) + t.Fatalf("claimPaths: %v", err) + } + if err := os.MkdirAll(filepath.Dir(livePath), 0o755); err != nil { + t.Fatalf("prepare claim lock directory: %v", err) } start := make(chan struct{}) @@ -424,18 +660,21 @@ func TestConcurrent_AcquireAndRelease_NoRace(t *testing.T) { var wg sync.WaitGroup wg.Add(2) - // Goroutine 1: release the claim held by run-a. + // Goroutine 1: attempt to release the claim for run-a. go func() { defer wg.Done() <-start - releaseErr <- ReleaseClaim(dir, "run-a", "ticket-1", acquired.LeaseID, "completed") + releaseErr <- ReleaseClaim(dir, "run-a", "ticket-1", "lease-a", "completed") }() - // Goroutine 2: attempt to acquire the same ticket for run-b. + // Goroutine 2: attempt to acquire the same ticket for run-a. + // Valid serial outcomes are: + // - acquire then release: both succeed, durable is released and live is gone + // - release then acquire: acquire succeeds, release fails with not-found go func() { defer wg.Done() <-start - _, err := AcquireClaim(dir, "run-b", "ticket-1", "lease-b", 30*time.Minute) + _, err := AcquireClaim(dir, "run-a", "ticket-1", "lease-a", 30*time.Minute) acquireErr <- err }() @@ -445,16 +684,54 @@ func TestConcurrent_AcquireAndRelease_NoRace(t *testing.T) { rErr := <-releaseErr aErr := <-acquireErr - // Valid outcomes (one serialises before the other): - // - Release wins: rErr==nil; acquire sees a free slot: aErr==nil. - // - Acquire-check wins: aErr!=nil (active claim blocks it); release still succeeds: rErr==nil. - // The only invalid outcome is both failing simultaneously. - if rErr != nil { - t.Errorf("ReleaseClaim failed unexpectedly: %v", rErr) + _, durablePath, err := claimPaths(dir, "run-a", "ticket-1") + if err != nil { + t.Fatalf("claimPaths: %v", err) + } + live, err := loadClaimArtifact(livePath) + if err != nil { + t.Fatalf("load live claim: %v", err) + } + durable, err := loadClaimArtifact(durablePath) + if err != nil { + t.Fatalf("load durable claim: %v", err) + } + + if aErr != nil { + t.Fatalf("Acquire should succeed in both serial orders: %v", aErr) + } + + if rErr == nil { + if durable == nil { + t.Fatal("expected durable claim after successful release") + } + if durable.State != "released" { + t.Fatalf("expected released durable state, got %q", durable.State) + } + if durable.ReleaseReason != "completed" { + t.Fatalf("expected release reason completed, got %q", durable.ReleaseReason) + } + if live != nil { + t.Fatalf("expected live claim removed after successful release") + } + return + } + + if !strings.Contains(rErr.Error(), "not found for release") { + t.Fatalf("expected release to report not-found when raced first, got: %v", rErr) + } + if live == nil || durable == nil { + t.Fatal("expected both claim files after acquire") + } + if live.State != "active" { + t.Fatalf("expected active live claim when acquire precedes release, got %q", live.State) + } + if durable.State != "active" { + t.Fatalf("expected active durable claim when acquire precedes release, got %q", durable.State) + } + if durable.LeaseID != "lease-a" { + t.Fatalf("expected durable lease-a, got %q", durable.LeaseID) } - // aErr is allowed to be non-nil when the lock ordering puts the acquire - // check before the release completes. - _ = aErr } // TestRenewClaim_DurableAndRestoreFailure_ErrorMentionsBoth verifies that when diff --git a/internal/adapters/ticketstore/tkmd/store.go b/internal/adapters/ticketstore/tkmd/store.go index 077df51..9de1d8f 100644 --- a/internal/adapters/ticketstore/tkmd/store.go +++ b/internal/adapters/ticketstore/tkmd/store.go @@ -278,12 +278,12 @@ func extractHeadingTitle(body string) string { // discovery. Only deps are considered as child edges; tk links are navigation // aids and must not be treated as child relationships. func loadEpicChildren(ticketsDir, epicID string) (map[string]struct{}, error) { + children := make(map[string]struct{}) path := filepath.Join(ticketsDir, epicID+".md") ticket, err := LoadTicket(path) if err != nil { - return nil, err + return children, err } - children := make(map[string]struct{}) for _, dep := range ticket.Deps { children[dep] = struct{}{} } @@ -311,6 +311,9 @@ func depsClosed(ticketsDir string, deps []string) (bool, error) { } func claimAllowsReady(ticketsDir, ticketID, currentRunID string) (bool, error) { + if err := validateClaimIdentifier(ticketID, "ticket_id"); err != nil { + return false, err + } claimPath := filepath.Join(ticketsDir, ".claims", ticketID+".json") data, err := os.ReadFile(claimPath) if err != nil { diff --git a/internal/adapters/ticketstore/tkmd/store_test.go b/internal/adapters/ticketstore/tkmd/store_test.go index 5d9c7a1..1dba254 100644 --- a/internal/adapters/ticketstore/tkmd/store_test.go +++ b/internal/adapters/ticketstore/tkmd/store_test.go @@ -181,6 +181,33 @@ func TestUsesCanonicalReadinessPredicate(t *testing.T) { } } +func TestClaimAllowsReady_RejectsPathTraversalTicketID(t *testing.T) { + dir := t.TempDir() + ticketsDir := filepath.Join(dir, ".tickets") + if err := os.MkdirAll(filepath.Join(ticketsDir, ".claims"), 0o755); err != nil { + t.Fatalf("mkdir .claims: %v", err) + } + + maliciousIDs := []string{ + "../escape", + "..", + ".", + "ticket/evil", + "ticket\\evil", + "/tmp/hijack", + "foo/../bar", + } + + for _, ticketID := range maliciousIDs { + t.Run(ticketID, func(t *testing.T) { + _, err := claimAllowsReady(ticketsDir, ticketID, "run-current") + if err == nil { + t.Fatalf("expected ticket id %q to be rejected", ticketID) + } + }) + } +} + func TestLoadEpicChildrenMalformed(t *testing.T) { dir := t.TempDir() // A frontmatter line without a colon causes splitKeyValue to return an error. @@ -194,8 +221,42 @@ func TestLoadEpicChildrenMalformed(t *testing.T) { if err == nil { t.Fatal("expected non-nil error for malformed epic, got nil") } - // children is nil on error — acceptable per spec - _ = children + if len(children) != 0 { + t.Fatalf("expected zero children on parse failure, got %d", len(children)) + } +} + +func TestListReadyChildren_PropagatesMalformedEpicError(t *testing.T) { + dir := t.TempDir() + ticketsDir := filepath.Join(dir, ".tickets") + if err := os.MkdirAll(ticketsDir, 0o755); err != nil { + t.Fatalf("mkdir .tickets: %v", err) + } + + epicPath := filepath.Join(ticketsDir, "epic-bad.md") + malformed := "---\nno_colon_here\n---\nEpic body.\n" + if err := os.WriteFile(epicPath, []byte(malformed), 0o644); err != nil { + t.Fatalf("write malformed epic: %v", err) + } + childPath := filepath.Join(ticketsDir, "child-1.md") + child := strings.Join([]string{ + "---", + "id: child-1", + "parent: epic-bad", + "status: open", + "---", + "", + "Child body.", + "", + }, "\n") + if err := os.WriteFile(childPath, []byte(child), 0o644); err != nil { + t.Fatalf("write child ticket: %v", err) + } + + children, err := ListReadyChildren(dir, "epic-bad") + if err == nil { + t.Fatalf("expected error from ListReadyChildren for malformed epic, got nil and children=%v", children) + } } func TestLoadEpicChildrenValid(t *testing.T) { diff --git a/internal/adapters/verify/command/runner.go b/internal/adapters/verify/command/runner.go index c3c4e0f..c4efa7c 100644 --- a/internal/adapters/verify/command/runner.go +++ b/internal/adapters/verify/command/runner.go @@ -328,10 +328,7 @@ func DeriveVerificationPassed(results []CommandResult) bool { var defaultEnvAllowlist = []string{ "CI", "HOME", - "LOGNAME", "PATH", - "TERM", - "USER", } // verificationEnv builds a deterministic environment for verification commands diff --git a/internal/adapters/verify/command/runner_test.go b/internal/adapters/verify/command/runner_test.go index ac2cc3d..2ae9997 100644 --- a/internal/adapters/verify/command/runner_test.go +++ b/internal/adapters/verify/command/runner_test.go @@ -143,7 +143,7 @@ func TestRunCommands_UsesAllowlistedEnvOnly(t *testing.T) { } } -func TestRunCommands_StartsWithCleanEnv(t *testing.T) { +func TestRunCommands_DefaultEnvExcludesNonAllowlistedVars(t *testing.T) { repoRoot := t.TempDir() t.Setenv("DROP_ME", "secret") @@ -164,7 +164,35 @@ func TestRunCommands_StartsWithCleanEnv(t *testing.T) { t.Fatalf("read stdout artifact: %v", err) } if got := strings.TrimSpace(string(stdoutData)); got != "missing" { - t.Fatalf("expected clean environment to omit inherited variables, got %q", got) + t.Fatalf("expected default environment to omit non-allowlisted inherited variables, got %q", got) + } +} + +func TestRunCommands_DefaultEnvIncludesCommonVars(t *testing.T) { + repoRoot := t.TempDir() + t.Setenv("PATH", "/tmp/verk-test-bin") + t.Setenv("HOME", "/tmp/verk-test-home") + t.Setenv("CI", "true") + t.Setenv("DROP_ME", "secret") + + results, err := RunCommands(context.Background(), repoRoot, []string{ + `printf '%s|%s|%s|%s' "${PATH:-missing}" "${HOME:-missing}" "${CI:-missing}" "${DROP_ME:-missing}"`, + }, 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)) + } + + stdoutData, err := os.ReadFile(results[0].StdoutPath) + if err != nil { + t.Fatalf("read stdout artifact: %v", err) + } + if got := strings.TrimSpace(string(stdoutData)); got != "/tmp/verk-test-bin|/tmp/verk-test-home|true|missing" { + t.Fatalf("expected allowlisted default variables to be present, got %q", got) } } diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 9544060..7bebc9a 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -2,7 +2,10 @@ package cli import ( "bytes" + "errors" + "io" "os" + "path/filepath" "strings" "testing" "verk/internal/policy" @@ -86,7 +89,52 @@ func TestInitCmd_RepeatedRunShowsAndPreservesExistingRuntimeProfilesOnBlankInput } } +type errReader struct { + data []byte + err error + pos int +} + +func (r *errReader) Read(p []byte) (int, error) { + if r.pos >= len(r.data) { + if r.err == nil { + return 0, io.EOF + } + return 0, r.err + } + n := copy(p, r.data[r.pos:]) + r.pos += n + return n, nil +} + +func TestInitCmd_FailingScannerPreventsConfigWrite(t *testing.T) { + dir := t.TempDir() + initCLITestRepo(t, dir) + + reader := &errReader{data: []byte("\n"), err: io.ErrUnexpectedEOF} + stdout, stderr, err := runInitInDirWithReader(t, dir, reader) + if err == nil { + t.Fatalf("expected init to fail on read error") + } + if !strings.Contains(err.Error(), "read input:") { + t.Fatalf("expected read input error, got: %v\nstdout:\n%s\nstderr:\n%s", err, stdout, stderr) + } + if !errors.Is(err, io.ErrUnexpectedEOF) { + t.Fatalf("expected wrapped scanner read error, got: %v\nstdout:\n%s\nstderr:\n%s", err, stdout, stderr) + } + if _, statErr := os.Stat(filepath.Join(dir, ".verk", "config.yaml")); !os.IsNotExist(statErr) { + if statErr == nil { + t.Fatalf("expected config file to not be written after scanner error") + } + t.Fatalf("unexpected stat error: %v\nstdout:\n%s\nstderr:\n%s", statErr, stdout, stderr) + } +} + func runInitInDir(t *testing.T, dir, stdin string) (string, string, error) { + return runInitInDirWithReader(t, dir, strings.NewReader(stdin)) +} + +func runInitInDirWithReader(t *testing.T, dir string, stdin io.Reader) (string, string, error) { t.Helper() originalWD, err := os.Getwd() if err != nil { @@ -105,7 +153,7 @@ func runInitInDir(t *testing.T, dir, stdin string) (string, string, error) { var stderr bytes.Buffer root := newRootCmd() root.SetArgs([]string{"init"}) - root.SetIn(strings.NewReader(stdin)) + root.SetIn(stdin) root.SetOut(&stdout) root.SetErr(&stderr) err = root.Execute() diff --git a/internal/cli/root.go b/internal/cli/root.go index 2600346..e57def1 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -73,7 +73,7 @@ func ExecuteArgs(args []string, stdout, stderr *os.File) int { root.SetOut(stdout) root.SetErr(stderr) root.SetArgs(args) - if err := root.Execute(); err != nil { + if _, err := root.ExecuteC(); err != nil { _, _ = fmt.Fprintln(stderr, err) var exitErr interface{ ExitCode() int } if errors.As(err, &exitErr) { diff --git a/internal/cli/run.go b/internal/cli/run.go index a7a24dc..55117a8 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -39,13 +39,32 @@ func drainProgress(ch <-chan engine.ProgressEvent) { }() } +func writeCurrentRunIDIfArtifactExists(repoRoot, runID string, errw io.Writer) bool { + if runID == "" { + return false + } + runPath := filepath.Join(repoRoot, ".verk", "runs", runID, "run.json") + if _, err := os.Stat(runPath); err != nil { + if !os.IsNotExist(err) { + _, _ = fmt.Fprintf(errw, "warning: could not inspect run artifact: %v\n", err) + } + return false + } + if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { + _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) + return false + } + return true +} + // saveJSONAtomic and saveTicket are package-level variables so tests can inject // fake implementations to exercise error paths without a real filesystem. // saveJSONAtomic is used both for the initial run.json persistence in // doRunTicket and for the final state update in finalizeRun. var ( - saveJSONAtomic func(string, any) error = state.SaveJSONAtomic - saveTicket func(string, tkmd.Ticket) error = tkmd.SaveTicket + saveJSONAtomic func(string, any) error = state.SaveJSONAtomic + saveTicket func(string, tkmd.Ticket) error = tkmd.SaveTicket + runTicket func(context.Context, engine.RunTicketRequest) (engine.RunTicketResult, error) = engine.RunTicket ) // finalizeRun persists ticket and run state after the engine finishes, then @@ -144,7 +163,14 @@ func initRunCmd(root *cobra.Command) { root.AddCommand(runCmd) } -func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { +func doRunTicket(w, errw io.Writer, ticketID string) (runID string, err error) { + emitRunID := true + defer func() { + if emitRunID && runID != "" { + _, _ = fmt.Fprintf(w, "run_id=%s\n", runID) + } + }() + // Cancel engine execution on SIGINT (Ctrl-C) or SIGTERM so that worker // processes and MCP helpers are terminated and claims are released before // the process exits. stop() removes the signal handler when the function @@ -166,7 +192,7 @@ func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { if err != nil { return "", err } - runID := newRunID(ticketID) + runID = newRunID(ticketID) plan, err := engine.BuildPlanArtifact(ticket, cfg) if err != nil { return "", err @@ -175,8 +201,6 @@ func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { plan.CreatedAt = time.Now().UTC() plan.UpdatedAt = plan.CreatedAt - _, _ = fmt.Fprintf(w, "run_id=%s\n", runID) - lock, err := engine.AcquireRunLock(repoRoot, runID) if err != nil { return runID, err @@ -232,15 +256,9 @@ func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { 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 { + if err := saveTicket(filepath.Join(repoRoot, ".tickets", ticketID+".md"), ticket); err != nil { return runID, err } @@ -254,7 +272,7 @@ func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { go func() { defer wg.Done() defer close(ch) - result, runErr = engine.RunTicket(ctx, engine.RunTicketRequest{ + result, runErr = runTicket(ctx, engine.RunTicketRequest{ RepoRoot: repoRoot, RunID: runID, Ticket: ticket, @@ -299,8 +317,14 @@ func doRunTicket(w, errw io.Writer, ticketID string) (string, error) { ticket, run, ); err != nil { + emitRunID = false return runID, err } + // Publish the current-run pointer only after the run is durably resumable: + // initial run.json, ticket state, engine result, and final run.json all exist. + if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { + _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) + } return runID, nil } @@ -335,9 +359,6 @@ func doRunEpic(w, errw io.Writer, ticketID string) (string, error) { runID := newRunID(ticketID) _, _ = fmt.Fprintf(w, "run_id=%s\n", runID) - if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { - _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) - } // Run engine with progress channel ch := make(chan engine.ProgressEvent, 64) @@ -370,13 +391,11 @@ func doRunEpic(w, errw io.Writer, ticketID string) (string, error) { // Wait for the engine goroutine to finish before reading result/runErr. wg.Wait() - if runErr != nil { - // Clear the current-run pointer so downstream commands don't resolve to - // a run whose run.json may never have been written by the engine. - if clearErr := writeCurrentRunID(repoRoot, ""); clearErr != nil { - _, _ = fmt.Fprintf(errw, "warning: could not clear current run: %v\n", clearErr) - } + // If engine.RunEpic persisted run.json, make it resumable even when the + // terminal result is a blocked run that the operator does not retry now. + currentRunWritten := writeCurrentRunIDIfArtifactExists(repoRoot, runID, errw) + if runErr != nil { // If the run ended in a structured blocked state, hand off to the // blocked-run handler so the operator sees which tickets are blocked // and how to retry them. For interactive terminals the handler may @@ -397,6 +416,15 @@ func doRunEpic(w, errw io.Writer, ticketID string) (string, error) { return runID, runErr } + if !currentRunWritten { + // The epic run artifact has been persisted by engine.RunEpic before it + // can return successfully. Write the current-run pointer after that point + // so .verk/current never points at a run without run.json on disk. + if wErr := writeCurrentRunID(repoRoot, runID); wErr != nil { + _, _ = fmt.Fprintf(errw, "warning: could not write current run: %v\n", wErr) + } + } + _, _ = fmt.Fprintf(w, "status=%s phase=%s\n", result.Run.Status, result.Run.CurrentPhase) return runID, nil } diff --git a/internal/cli/run_current_pointer_test.go b/internal/cli/run_current_pointer_test.go index c0155e1..f34d831 100644 --- a/internal/cli/run_current_pointer_test.go +++ b/internal/cli/run_current_pointer_test.go @@ -93,12 +93,51 @@ func TestDoRunTicket_CurrentPointerNotSetOnSaveFailure(t *testing.T) { // If the file doesn't exist, that's the correct outcome too. } -// TestDoRunEpic_CurrentPointerClearedOnEngineFailure verifies the fix for -// doRunEpic: when engine.RunEpic returns an error, writeCurrentRunID("") must -// be called to clear the pointer that was set before the goroutine launched. -// If not cleared, .verk/current keeps pointing at a run whose run.json may -// never have been written. -func TestDoRunEpic_CurrentPointerClearedOnEngineFailure(t *testing.T) { +func TestDoRunTicket_CurrentPointerNotSetOnTicketSaveFailure(t *testing.T) { + dir := t.TempDir() + initCLITestRepo(t, dir) + + ticketsDir := filepath.Join(dir, ".tickets") + if err := os.MkdirAll(ticketsDir, 0o755); err != nil { + t.Fatalf("mkdir .tickets: %v", err) + } + ticket := tkmd.Ticket{ + ID: "ver-ticket-save", + Title: "Ticket save failure test", + Status: tkmd.StatusReady, + } + if err := tkmd.SaveTicket(filepath.Join(ticketsDir, "ver-ticket-save.md"), ticket); err != nil { + t.Fatalf("save ticket: %v", err) + } + + origSaveTicket := saveTicket + defer func() { saveTicket = origSaveTicket }() + saveTicket = func(_ string, _ tkmd.Ticket) error { + return errors.New("injected ticket save error") + } + + t.Chdir(dir) + + var stdout, stderr bytes.Buffer + runID, err := doRunTicket(&stdout, &stderr, "ver-ticket-save") + if err == nil { + t.Fatal("expected error from injected ticket save failure, got nil") + } + if runID == "" { + t.Fatal("expected doRunTicket to return a non-empty runID even on failure") + } + + currentPath := filepath.Join(dir, ".verk", "current") + data, readErr := os.ReadFile(currentPath) + if readErr == nil && strings.TrimSpace(string(data)) == runID { + t.Errorf(".verk/current = %q after ticket save failure; pointer must not advance", runID) + } +} + +// TestDoRunEpic_DoesNotAdvanceCurrentOnEngineFailure verifies that doRunEpic +// does not advance .verk/current to a new runID when engine.RunEpic returns an +// error before it can successfully persist the initial run artifact. +func TestDoRunEpic_DoesNotAdvanceCurrentOnEngineFailure(t *testing.T) { dir := t.TempDir() initCLITestRepo(t, dir) // Intentionally do NOT create the epic ticket file. @@ -120,8 +159,8 @@ func TestDoRunEpic_CurrentPointerClearedOnEngineFailure(t *testing.T) { t.Fatal("expected doRunEpic to return a non-empty runID even on failure") } - // .verk/current must be cleared (empty) after the engine failure so that - // downstream commands don't resolve to a run without a valid run.json. + // .verk/current must not be set to the new run ID after this early engine + // failure because the run.json artifact was never persisted. currentPath := filepath.Join(dir, ".verk", "current") data, readErr := os.ReadFile(currentPath) if readErr != nil { @@ -131,7 +170,61 @@ func TestDoRunEpic_CurrentPointerClearedOnEngineFailure(t *testing.T) { } got := strings.TrimSpace(string(data)) if got == runID { - t.Errorf(".verk/current = %q after engine failure; pointer must be cleared "+ + t.Errorf(".verk/current = %q after engine failure; pointer must not advance "+ "(got non-empty runID pointing at potentially missing run.json)", runID) } } + +func TestDoRunEpic_CurrentPointerSetForPersistedBlockedRun(t *testing.T) { + dir := t.TempDir() + initCLITestRepo(t, dir) + + ticketsDir := filepath.Join(dir, ".tickets") + if err := os.MkdirAll(ticketsDir, 0o755); err != nil { + t.Fatalf("mkdir .tickets: %v", err) + } + epic := tkmd.Ticket{ + ID: "ver-current-epic", + Title: "Current pointer epic", + Status: tkmd.StatusReady, + UnknownFrontmatter: map[string]any{ + "type": "epic", + }, + } + child := tkmd.Ticket{ + ID: "ver-current-child", + Title: "Blocked child", + Status: tkmd.StatusBlocked, + UnknownFrontmatter: map[string]any{ + "parent": epic.ID, + "type": "task", + }, + } + if err := tkmd.SaveTicket(filepath.Join(ticketsDir, epic.ID+".md"), epic); err != nil { + t.Fatalf("save epic: %v", err) + } + if err := tkmd.SaveTicket(filepath.Join(ticketsDir, child.ID+".md"), child); err != nil { + t.Fatalf("save child: %v", err) + } + + t.Chdir(dir) + + var stdout, stderr bytes.Buffer + runID, err := doRunEpic(&stdout, &stderr, epic.ID) + if err == nil { + t.Fatal("expected blocked epic error, got nil") + } + if runID == "" { + t.Fatal("expected doRunEpic to return a non-empty runID") + } + if _, statErr := os.Stat(filepath.Join(dir, ".verk", "runs", runID, "run.json")); statErr != nil { + t.Fatalf("expected persisted run artifact: %v", statErr) + } + data, readErr := os.ReadFile(filepath.Join(dir, ".verk", "current")) + if readErr != nil { + t.Fatalf("read .verk/current: %v", readErr) + } + if got := strings.TrimSpace(string(data)); got != runID { + t.Fatalf("expected .verk/current=%q for blocked persisted run, got %q", runID, got) + } +} diff --git a/internal/cli/run_persistence_test.go b/internal/cli/run_persistence_test.go index f79754b..4f0e4cf 100644 --- a/internal/cli/run_persistence_test.go +++ b/internal/cli/run_persistence_test.go @@ -2,13 +2,98 @@ package cli import ( "bytes" + "context" "errors" + "io" + "os" + "path/filepath" "strings" "testing" "verk/internal/adapters/ticketstore/tkmd" + "verk/internal/engine" "verk/internal/state" ) +// TestDoRunTicket_FinalSaveFailure injects a stubbed engine execution and a +// failing final SaveJSONAtomic call so doRunTicket returns the wrapped error and +// never prints the success status line. +func TestDoRunTicket_FinalSaveFailure(t *testing.T) { + dir := t.TempDir() + initCLITestRepo(t, dir) + + ticketsDir := filepath.Join(dir, ".tickets") + if err := os.MkdirAll(ticketsDir, 0o755); err != nil { + t.Fatalf("mkdir .tickets: %v", err) + } + ticket := tkmd.Ticket{ + ID: "ver-final-save", + Title: "Final persistence failure test", + Status: tkmd.StatusReady, + } + if err := tkmd.SaveTicket(filepath.Join(ticketsDir, "ver-final-save.md"), ticket); err != nil { + t.Fatalf("save ticket: %v", err) + } + + origRunTicket := runTicket + origRunProgress := runProgress + origSaveJSONAtomic := saveJSONAtomic + origSaveTicket := saveTicket + defer func() { + runTicket = origRunTicket + runProgress = origRunProgress + saveJSONAtomic = origSaveJSONAtomic + saveTicket = origSaveTicket + }() + + runTicket = func(_ context.Context, _ engine.RunTicketRequest) (engine.RunTicketResult, error) { + return engine.RunTicketResult{Snapshot: engine.TicketRunSnapshot{CurrentPhase: state.TicketPhaseClosed}}, nil + } + runProgress = func(_ string, ch <-chan engine.ProgressEvent, _ io.Writer, _ func()) error { + for range ch { + } + return nil + } + saveTicket = func(_ string, _ tkmd.Ticket) error { return nil } + + errDiskFull := errors.New("disk full") + callCount := 0 + saveJSONAtomic = func(_ string, _ any) error { + callCount++ + if callCount == 2 { // Fail only the final persistence after the engine completes. + return errDiskFull + } + return nil + } + + t.Chdir(dir) + + var stdout, stderr bytes.Buffer + runID, err := doRunTicket(&stdout, &stderr, "ver-final-save") + if err == nil { + t.Fatal("expected doRunTicket to return an error") + } + if !strings.Contains(err.Error(), "persist run state") { + t.Fatalf("expected wrapped persist error, got %v", err) + } + if !errors.Is(err, errDiskFull) { + t.Fatalf("expected underlying disk full error, got %v", err) + } + if runID == "" { + t.Fatal("expected non-empty runID even on persistence failure") + } + currentPath := filepath.Join(dir, ".verk", "current") + data, readErr := os.ReadFile(currentPath) + if readErr == nil && strings.TrimSpace(string(data)) == runID { + t.Fatalf(".verk/current advanced to %q after final run persistence failed", runID) + } + if stdout.Len() > 0 { + t.Fatalf("expected no success output on stdout, got %q", stdout.String()) + } + if callCount != 2 { + t.Fatalf("expected SaveJSONAtomic to be called twice, saw %d", callCount) + } +} + // TestFinalizeRun_SaveJSONAtomicFailure verifies that when SaveJSONAtomic fails, // finalizeRun returns a wrapped error ("persist run state: ...") and does NOT // print the success status line. doRunTicket calls finalizeRun and propagates diff --git a/internal/cli/run_sync_test.go b/internal/cli/run_sync_test.go index fc10462..59b2218 100644 --- a/internal/cli/run_sync_test.go +++ b/internal/cli/run_sync_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" "verk/internal/engine" + "verk/internal/state" ) // TestEngineGoroutineSync_NoRaceOnEarlyTUIReturn validates the synchronization @@ -230,3 +231,37 @@ func TestDrainGoroutine_PreventsDeadlockOnEarlyTUIReturn(t *testing.T) { t.Errorf("expected nil runErr, got %v", runErr) } } + +// TestEngineGoroutineSync_ResumeReportPath checks that resume report and error +// state are still visible after an early TUI return when the shared variables +// are written after the last progress event has been sent. +func TestEngineGoroutineSync_ResumeReportPath(t *testing.T) { + ch := make(chan engine.ProgressEvent, 4) + + var report engine.ResumeReport + var resumeErr error + var wg sync.WaitGroup + + wg.Add(1) + go func() { + defer wg.Done() + defer close(ch) + ch <- engine.ProgressEvent{Type: engine.EventRunCompleted, Detail: "resuming"} + time.Sleep(10 * time.Millisecond) + report = engine.ResumeReport{Run: state.RunArtifact{Status: state.EpicRunStatusCompleted}} + resumeErr = errors.New("resume finished") + }() + + // Fake TUI returns after consuming one event, simulating an early exit. + <-ch + + // Ensure producer goroutine has committed final shared state before reads. + wg.Wait() + + if report.Run.Status != state.EpicRunStatusCompleted { + t.Fatalf("expected report status=%q, got %q", state.EpicRunStatusCompleted, report.Run.Status) + } + if resumeErr == nil || resumeErr.Error() != "resume finished" { + t.Fatalf("expected resumeErr=%q, got %v", "resume finished", resumeErr) + } +} diff --git a/internal/cli/shared_test.go b/internal/cli/shared_test.go index 1839184..7d800de 100644 --- a/internal/cli/shared_test.go +++ b/internal/cli/shared_test.go @@ -67,16 +67,16 @@ func TestLatestRunID_NoRunsDir(t *testing.T) { } func TestLatestRunID_CrossTicketIDOrder(t *testing.T) { - // run-ticket-z-1000 sorts lex BEFORE run-ticket-a-2000, - // but run-ticket-a-2000 has a larger timestamp and must win. + // run-ticket-a-1000 sorts lex BEFORE run-ticket-z-2000, + // but 2000 > 1000, so run-ticket-z-2000 must win. repoRoot := t.TempDir() runsDir := filepath.Join(repoRoot, ".verk", "runs") if err := os.MkdirAll(runsDir, 0o755); err != nil { t.Fatalf("mkdir: %v", err) } for _, name := range []string{ - "run-ticket-z-1000", - "run-ticket-a-2000", + "run-ticket-a-1000", + "run-ticket-z-2000", } { if err := os.Mkdir(filepath.Join(runsDir, name), 0o755); err != nil { t.Fatalf("mkdir %s: %v", name, err) @@ -87,8 +87,8 @@ func TestLatestRunID_CrossTicketIDOrder(t *testing.T) { if err != nil { t.Fatalf("latestRunID: %v", err) } - if latest != "run-ticket-a-2000" { - t.Fatalf("expected run-ticket-a-2000, got %q", latest) + if latest != "run-ticket-z-2000" { + t.Fatalf("expected run-ticket-z-2000, got %q", latest) } } @@ -98,9 +98,8 @@ func TestLatestRunID_SkipsUnparseableEntries(t *testing.T) { if err := os.MkdirAll(runsDir, 0o755); err != nil { t.Fatalf("mkdir: %v", err) } - // "run-bad-suffix" has a non-numeric suffix; should be skipped. - // "run-ticket-a-9999" is valid and should be returned. for _, name := range []string{ + "run-ticket-b-1000", "run-bad-suffix", "run-ticket-a-9999", } { diff --git a/internal/e2e/helpers_test.go b/internal/e2e/helpers_test.go index b033757..797ab75 100644 --- a/internal/e2e/helpers_test.go +++ b/internal/e2e/helpers_test.go @@ -97,7 +97,7 @@ func testPlanAndClaim(t *testing.T, repoRoot string, cfg policy.Config, runID st } plan.RunID = runID plan.ValidationCommands = append([]string(nil), commands...) - claim, err := tkmd.AcquireClaim(repoRoot, runID, ticket.ID, leaseID, 10*time.Minute, testTime()) + claim, err := tkmd.AcquireClaim(repoRoot, runID, ticket.ID, leaseID, 10*time.Minute, time.Now().UTC()) if err != nil { t.Fatalf("AcquireClaim: %v", err) } diff --git a/internal/e2e/resume_claim_recovery_test.go b/internal/e2e/resume_claim_recovery_test.go index 4832175..caaa4f7 100644 --- a/internal/e2e/resume_claim_recovery_test.go +++ b/internal/e2e/resume_claim_recovery_test.go @@ -53,13 +53,14 @@ func TestResumeBlocksOnLiveDurableClaimDivergence(t *testing.T) { }); err != nil { t.Fatalf("save ticket-run: %v", err) } + now := time.Now().UTC() live := state.ClaimArtifact{ ArtifactMeta: state.ArtifactMeta{SchemaVersion: 1, RunID: runID}, TicketID: ticket.ID, OwnerRunID: runID, LeaseID: "lease-live", - LeasedAt: testTime(), - ExpiresAt: testTime().Add(10 * time.Minute), + LeasedAt: now, + ExpiresAt: now.Add(10 * time.Minute), State: "active", } durable := live diff --git a/internal/engine/closeout_test.go b/internal/engine/closeout_test.go index 82e9969..8669e99 100644 --- a/internal/engine/closeout_test.go +++ b/internal/engine/closeout_test.go @@ -298,6 +298,110 @@ func TestReviewFindingBlocks(t *testing.T) { } } +func TestNormalizeReviewFinding(t *testing.T) { + now := fixedTime() + cases := []struct { + name string + finding state.ReviewFinding + threshold state.Severity + wantOpen bool + wantBlock bool + }{ + { + name: "expired waiver is treated as open and blocks", + finding: state.ReviewFinding{ + ID: "f-1", + Severity: state.SeverityP1, + Disposition: "waived", + Title: "expired waiver", + Body: "waiver has expired", + File: "internal/engine/closeout.go", + Line: 10, + WaivedBy: "reviewer", + WaivedAt: now, + WaiverReason: "accepted risk", + WaiverExpiresAt: now.Add(-24 * time.Hour), + }, + threshold: state.SeverityP1, + wantOpen: true, + wantBlock: true, + }, + { + name: "future waiver stays waived and does not block", + finding: state.ReviewFinding{ + ID: "f-2", + Severity: state.SeverityP1, + Disposition: "waived", + Title: "future waiver", + Body: "waiver has not expired", + File: "internal/engine/closeout.go", + Line: 10, + WaivedBy: "reviewer", + WaivedAt: now, + WaiverReason: "accepted risk", + WaiverExpiresAt: time.Now().Add(24 * time.Hour), + }, + threshold: state.SeverityP1, + wantOpen: false, + wantBlock: false, + }, + { + name: "waiver without expiry is unchanged and does not block", + finding: state.ReviewFinding{ + ID: "f-3", + Severity: state.SeverityP1, + Disposition: "waived", + Title: "no expiry waiver", + Body: "perpetual waiver", + File: "internal/engine/closeout.go", + Line: 10, + WaivedBy: "reviewer", + WaivedAt: now, + WaiverReason: "permanent accepted risk", + WaiverExpiresAt: time.Time{}, + }, + threshold: state.SeverityP1, + wantOpen: false, + wantBlock: false, + }, + { + name: "open finding without waiver still blocks", + finding: state.ReviewFinding{ + ID: "f-4", + Severity: state.SeverityP1, + Disposition: "open", + Title: "open finding", + Body: "open", + File: "internal/engine/closeout.go", + Line: 10, + }, + threshold: state.SeverityP1, + wantOpen: true, + wantBlock: true, + }, + } + + for _, tc := range cases { + got, ok := normalizeReviewFinding(tc.finding) + if !ok { + t.Fatalf("%s: expected finding to normalize", tc.name) + } + if tc.wantOpen { + if got.Disposition != "open" { + t.Fatalf("%s: expected disposition open, got %q", tc.name, got.Disposition) + } + if got.WaivedBy != "" || !got.WaivedAt.IsZero() || got.WaiverReason != "" || !got.WaiverExpiresAt.IsZero() { + t.Fatalf("%s: expected waived metadata to be cleared for expired waiver", tc.name) + } + } else if got.Disposition != tc.finding.Disposition { + t.Fatalf("%s: expected disposition %q, got %q", tc.name, tc.finding.Disposition, got.Disposition) + } + if gotBlock := ReviewFindingBlocks(tc.finding, tc.threshold); gotBlock != tc.wantBlock { + t.Fatalf("%s: expected ReviewFindingBlocks=%v, got %v", tc.name, tc.wantBlock, gotBlock) + } + } +} + func TestDeriveCriteriaEvidence_DiffFromChangedFiles(t *testing.T) { req := baseCloseoutRequest() req.implementation = &state.ImplementationArtifact{ diff --git a/internal/engine/epic_run.go b/internal/engine/epic_run.go index bdc5e79..a4a13f8 100644 --- a/internal/engine/epic_run.go +++ b/internal/engine/epic_run.go @@ -114,10 +114,7 @@ func collectBlockedTickets(repoRoot, runID string, children []tkmd.Ticket) []Blo continue } reason := describeNotReady(child) - phase := state.TicketPhase("") - if child.Status == tkmd.StatusBlocked { - phase = state.TicketPhaseBlocked - } + phase := state.TicketPhaseIntake if runID != "" { var snap TicketRunSnapshot if err := loadTicketSnapshot(repoRoot, runID, child.ID, &snap); err == nil { diff --git a/internal/engine/epic_run_test.go b/internal/engine/epic_run_test.go index dc19376..417716c 100644 --- a/internal/engine/epic_run_test.go +++ b/internal/engine/epic_run_test.go @@ -203,6 +203,53 @@ func TestAcceptWave_ScopeViolationIsFatal(t *testing.T) { } } +func TestCollectBlockedTicketsDoesNotOfferRetryForSnapshotlessBlockedTicket(t *testing.T) { + repoRoot := t.TempDir() + child := tkmd.Ticket{ + ID: "ticket-blocked", + Title: "Blocked ticket", + Status: tkmd.StatusBlocked, + } + + blocked := collectBlockedTickets(repoRoot, "run-without-snapshot", []tkmd.Ticket{child}) + if len(blocked) != 1 { + t.Fatalf("expected one blocked ticket, got %d", len(blocked)) + } + if blocked[0].Phase != state.TicketPhaseIntake { + t.Fatalf("expected derived intake phase without snapshot, got %q", blocked[0].Phase) + } + if blocked[0].RetryPhase != "" { + t.Fatalf("expected no retry phase without a blocked run snapshot, got %q", blocked[0].RetryPhase) + } +} + +func TestCollectBlockedTicketsOffersRetryForBlockedSnapshot(t *testing.T) { + repoRoot := t.TempDir() + runID := "run-with-blocked-snapshot" + child := tkmd.Ticket{ + ID: "ticket-blocked", + Title: "Blocked ticket", + Status: tkmd.StatusBlocked, + } + writeTicketRunFixture(t, repoRoot, runID, TicketRunSnapshot{ + ArtifactMeta: state.ArtifactMeta{SchemaVersion: artifactSchemaVersion, RunID: runID}, + TicketID: child.ID, + CurrentPhase: state.TicketPhaseBlocked, + BlockReason: "review failed", + }) + + blocked := collectBlockedTickets(repoRoot, runID, []tkmd.Ticket{child}) + if len(blocked) != 1 { + t.Fatalf("expected one blocked ticket, got %d", len(blocked)) + } + if blocked[0].Phase != state.TicketPhaseBlocked { + t.Fatalf("expected blocked phase, got %q", blocked[0].Phase) + } + if blocked[0].RetryPhase != state.TicketPhaseImplement { + t.Fatalf("expected implement retry phase, got %q", blocked[0].RetryPhase) + } +} + func TestRunEpicSchedulesOpenAndReadyTickets(t *testing.T) { repoRoot := t.TempDir() baseCommit := initEpicRepo(t, repoRoot) diff --git a/internal/engine/resume.go b/internal/engine/resume.go index 671b2a9..13b8ea3 100644 --- a/internal/engine/resume.go +++ b/internal/engine/resume.go @@ -81,7 +81,8 @@ func ResumeRun(ctx context.Context, req ResumeRequest) (ResumeReport, error) { / if repaired { recovered = append(recovered, ticketID) } - if snapshot.Closeout == nil && (snapshot.CurrentPhase == state.TicketPhaseCloseout || snapshot.CurrentPhase == state.TicketPhaseClosed) { + sourcePhase := snapshot.CurrentPhase + if snapshot.Closeout == nil && (sourcePhase == state.TicketPhaseCloseout || sourcePhase == state.TicketPhaseClosed) { plan, ok := artifacts.Plans[ticketID] if !ok { return ResumeReport{}, fmt.Errorf("resume requires plan artifact for ticket %s", ticketID) @@ -99,7 +100,8 @@ func ResumeRun(ctx context.Context, req ResumeRequest) (ResumeReport, error) { / } snapshot.Closeout = &closeout snapshot.UpdatedAt = stateTime() - if snapshot.CurrentPhase == state.TicketPhaseCloseout || snapshot.CurrentPhase == state.TicketPhaseClosed { + switch sourcePhase { + case state.TicketPhaseCloseout, state.TicketPhaseClosed: if closeout.Closable { snapshot.CurrentPhase = state.TicketPhaseClosed snapshot.BlockReason = "" diff --git a/internal/engine/resume_test.go b/internal/engine/resume_test.go index b851a5d..3f59d1f 100644 --- a/internal/engine/resume_test.go +++ b/internal/engine/resume_test.go @@ -257,10 +257,16 @@ func TestResumeRun_ClosedPhase_NonClosable_BecomesBlocked(t *testing.T) { if snapshot.CurrentPhase != state.TicketPhaseBlocked { t.Fatalf("expected Blocked phase, got %q", snapshot.CurrentPhase) } - if snapshot.BlockReason == "" { - t.Fatal("expected BlockReason to be set, got empty string") + if snapshot.Closeout == nil { + t.Fatalf("expected closeout to be repaired, got %#v", snapshot.Closeout) } - if snapshot.Closeout == nil || snapshot.Closeout.Closable { + if snapshot.Closeout.FailedGate == "" { + t.Fatalf("expected non-empty FailedGate, got %#v", snapshot.Closeout) + } + if snapshot.BlockReason != snapshot.Closeout.FailedGate { + t.Fatalf("expected BlockReason %q, got %q", snapshot.Closeout.FailedGate, snapshot.BlockReason) + } + if snapshot.Closeout.Closable { t.Fatalf("expected non-closable closeout, got %#v", snapshot.Closeout) } } @@ -344,7 +350,10 @@ func TestResumeRun_ClosedPhase_Closable_StaysClosed(t *testing.T) { if snapshot.BlockReason != "" { t.Fatalf("expected BlockReason cleared, got %q", snapshot.BlockReason) } - if snapshot.Closeout == nil || !snapshot.Closeout.Closable { + if snapshot.Closeout == nil { + t.Fatalf("expected repaired closeout, got %#v", snapshot.Closeout) + } + if !snapshot.Closeout.Closable { t.Fatalf("expected closable closeout, got %#v", snapshot.Closeout) } } @@ -424,6 +433,9 @@ func TestResumeRun_CloseoutPhase_Closable_BecomesClosed(t *testing.T) { if snapshot.CurrentPhase != state.TicketPhaseClosed { t.Fatalf("expected Closed phase, got %q", snapshot.CurrentPhase) } + if snapshot.BlockReason != "" { + t.Fatalf("expected BlockReason cleared, got %q", snapshot.BlockReason) + } if snapshot.Closeout == nil || !snapshot.Closeout.Closable { t.Fatalf("expected closable closeout, got %#v", snapshot.Closeout) } diff --git a/internal/engine/ticket_run.go b/internal/engine/ticket_run.go index d1ae834..2d94796 100644 --- a/internal/engine/ticket_run.go +++ b/internal/engine/ticket_run.go @@ -19,7 +19,11 @@ import ( verifycommand "verk/internal/adapters/verify/command" ) -const maxRuntimeRetryAttempts = 2 +const ( + maxRuntimeRetryAttempts = 2 + minClaimRenewalInterval = 25 * time.Millisecond + immediateClaimRenewalThreshold = 3 * minClaimRenewalInterval +) var ( errRuntimeExecutionBlocked = errors.New("runtime execution blocked") @@ -49,6 +53,7 @@ type TicketRunSnapshot struct { state.ArtifactMeta TicketID string `json:"ticket_id"` CurrentPhase state.TicketPhase `json:"current_phase"` + Outcome state.TicketOutcome `json:"outcome,omitempty"` BlockReason string `json:"block_reason,omitempty"` ImplementationAttempts int `json:"implementation_attempts"` VerificationAttempts int `json:"verification_attempts"` @@ -560,6 +565,20 @@ func (st *ticketRunState) executeVerification(ctx context.Context, repoRoot stri if err := st.persist(); err != nil { return false, err } + if verifyPassed { + advisoryFailingIDs := verificationAdvisoryFailingCheckIDs(st.verification) + if len(advisoryFailingIDs) > 0 && st.implementationAttempts < st.cfg.Policy.MaxImplementationAttempts { + appendVerificationRepairCycle(st, advisoryFailingIDs) + st.progressDetail(fmt.Sprintf("advisory checks failed; best-effort repair: %s", strings.Join(advisoryFailingIDs, ", "))) + if err := st.transitionTo(state.TicketPhaseImplement); err != nil { + return false, err + } + if err := st.persist(); err != nil { + return false, err + } + return false, nil + } + } if !verifyPassed { if err := handleVerificationFailure(st, *verifyArtifact); err != nil { return false, err @@ -675,6 +694,59 @@ func verificationFailingCheckIDs(verification *state.VerificationArtifact) []str return failingCheckIDs(*verification.ValidationCoverage) } +// verificationAdvisoryFailingCheckIDs returns failed check ids whose +// ValidationCheck is explicitly advisory. These failures are safe to route +// through best-effort repair, but they must not become blockers on their own. +func verificationAdvisoryFailingCheckIDs(verification *state.VerificationArtifact) []string { + if verification == nil || verification.ValidationCoverage == nil { + return nil + } + return advisoryFailingCheckIDs(*verification.ValidationCoverage) +} + +func advisoryFailingCheckIDs(coverage state.ValidationCoverageArtifact) []string { + checks := make(map[string]state.ValidationCheck, len(coverage.DeclaredChecks)+len(coverage.DerivedChecks)) + for _, check := range coverage.DerivedChecks { + checks[check.ID] = check + } + // Declared checks intentionally override derived checks on ID collisions: + // ticket-authored validation must not be downgraded by an advisory derived duplicate. + for _, check := range coverage.DeclaredChecks { + checks[check.ID] = check + } + + latest := make(map[string]state.ValidationCheckResult, len(coverage.ExecutedChecks)) + for _, exec := range coverage.ExecutedChecks { + latest[exec.CheckID] = exec.Result + } + + advisoryFailures := 0 + for id, result := range latest { + if result != state.ValidationCheckResultFailed { + continue + } + check, ok := checks[id] + if !ok || !check.Advisory { + continue + } + advisoryFailures++ + } + + out := make([]string, 0, advisoryFailures) + for id, result := range latest { + if result != state.ValidationCheckResultFailed { + continue + } + check, ok := checks[id] + if !ok || !check.Advisory { + continue + } + out = append(out, id) + } + sort.Strings(out) + return out +} + // buildVerificationBlockReason composes a clear block reason for a // ticket that exhausted its verify-loop budget. When the coverage // artifact recorded specific failing check ids, they are included so @@ -1157,13 +1229,13 @@ func (st *ticketRunState) startClaimRenewal(ctx context.Context) (context.Contex if ttl <= 0 { ttl = 30 * time.Minute } - remaining := st.remainingTTL() - if remaining <= 0 { - remaining = 30 * time.Minute + remaining, knownRemaining := st.currentClaimRemainingTTL() + if !knownRemaining { + remaining = ttl } interval := remaining / 3 - if interval < 25*time.Millisecond { - interval = 25 * time.Millisecond + if interval < minClaimRenewalInterval { + interval = minClaimRenewalInterval } renewCtx, cancel := context.WithCancel(ctx) @@ -1172,6 +1244,23 @@ func (st *ticketRunState) startClaimRenewal(ctx context.Context) (context.Contex go func() { defer close(done) + renew := func() bool { + if _, err := tkmd.RenewClaim(st.repoRoot, st.req.RunID, st.req.Ticket.ID, st.req.Claim.LeaseID, ttl, time.Now().UTC()); err != nil { + select { + case errCh <- err: + default: + } + cancel() + return false + } + return true + } + if knownRemaining && remaining <= immediateClaimRenewalThreshold { + if !renew() { + return + } + } + ticker := time.NewTicker(interval) defer ticker.Stop() @@ -1180,12 +1269,7 @@ func (st *ticketRunState) startClaimRenewal(ctx context.Context) (context.Contex case <-renewCtx.Done(): return case <-ticker.C: - if _, err := tkmd.RenewClaim(st.repoRoot, st.req.RunID, st.req.Ticket.ID, st.req.Claim.LeaseID, ttl, time.Now().UTC()); err != nil { - select { - case errCh <- err: - default: - } - cancel() + if !renew() { return } } @@ -1216,18 +1300,21 @@ func (st *ticketRunState) claimTTL() time.Duration { return ttl } -// remainingTTL computes the time remaining until the claim expires, -// used for scheduling renewal intervals. Unlike claimTTL which returns -// the original full TTL, this reflects the actual remaining time. -func (st *ticketRunState) remainingTTL() time.Duration { - if st.req.Claim.ExpiresAt.IsZero() { - return 0 +// currentClaimRemainingTTL computes the time left on the active live claim. +// The request claim can become stale across long tickets with multiple worker +// and reviewer phases, while the live claim is updated by every successful +// renewal. Scheduling from the live expiry prevents later phases from waiting +// past the actual lease deadline. +func (st *ticketRunState) currentClaimRemainingTTL() (time.Duration, bool) { + if live, err := loadOptionalClaim(liveClaimPath(st.repoRoot, st.req.Ticket.ID)); err == nil && live != nil { + if live.OwnerRunID == st.req.RunID && live.LeaseID == st.req.Claim.LeaseID && live.State != "released" && !live.ExpiresAt.IsZero() { + return live.ExpiresAt.Sub(time.Now().UTC()), true + } } - remaining := st.req.Claim.ExpiresAt.Sub(time.Now().UTC()) - if remaining <= 0 { - return 0 + if !st.req.Claim.ExpiresAt.IsZero() { + return st.req.Claim.ExpiresAt.Sub(time.Now().UTC()), true } - return remaining + return 0, false } func shouldRetryRuntimeError(err error) bool { @@ -1355,6 +1442,7 @@ func (st *ticketRunState) snapshot() TicketRunSnapshot { }, TicketID: st.req.Ticket.ID, CurrentPhase: st.currentPhase, + Outcome: ticketOutcomeForPhase(st.currentPhase), BlockReason: st.blockReason, ImplementationAttempts: st.implementationAttempts, VerificationAttempts: st.verificationAttempts, @@ -1370,6 +1458,23 @@ func (st *ticketRunState) snapshot() TicketRunSnapshot { return snapshot } +// ticketOutcomeForPhase intentionally maps only legacy terminal phases: +// state.TicketPhaseClosed -> state.TicketOutcomeClosed and +// state.TicketPhaseBlocked -> state.TicketOutcomeBlocked. More specific +// outcomes such as state.TicketOutcomeFailedRetryable, +// state.TicketOutcomeNeedsDecision, and state.TicketOutcomeCancelled are +// deferred to follow-up state-machine work. +func ticketOutcomeForPhase(phase state.TicketPhase) state.TicketOutcome { + switch phase { + case state.TicketPhaseClosed: + return state.TicketOutcomeClosed + case state.TicketPhaseBlocked: + return state.TicketOutcomeBlocked + default: + return "" + } +} + func (st *ticketRunState) releaseClaim() error { reason := st.blockReason if reason == "" && st.currentPhase == state.TicketPhaseClosed { diff --git a/internal/engine/ticket_run_test.go b/internal/engine/ticket_run_test.go index 85b88db..7325a86 100644 --- a/internal/engine/ticket_run_test.go +++ b/internal/engine/ticket_run_test.go @@ -69,6 +69,9 @@ func TestRunTicket_HappyPath(t *testing.T) { if result.Snapshot.CurrentPhase != state.TicketPhaseClosed { t.Fatalf("expected closed phase, got %q", result.Snapshot.CurrentPhase) } + if result.Snapshot.Outcome != state.TicketOutcomeClosed { + t.Fatalf("expected closed outcome, got %q", result.Snapshot.Outcome) + } if result.Snapshot.Closeout == nil || !result.Snapshot.Closeout.Closable { t.Fatalf("expected closable closeout, got %#v", result.Snapshot.Closeout) } @@ -86,6 +89,13 @@ func TestRunTicket_HappyPath(t *testing.T) { if _, err := os.Stat(snapshotPath); err != nil { t.Fatalf("expected snapshot file to exist: %v", err) } + var persistedSnapshot TicketRunSnapshot + if err := state.LoadJSON(snapshotPath, &persistedSnapshot); err != nil { + t.Fatalf("load persisted snapshot: %v", err) + } + if persistedSnapshot.Outcome != state.TicketOutcomeClosed { + t.Fatalf("expected persisted closed outcome, got %q", persistedSnapshot.Outcome) + } durableClaimPath := filepath.Join(repoRoot, ".verk", "runs", "run-happy", "claims", "claim-"+ticket.ID+".json") var durableClaim state.ClaimArtifact @@ -112,6 +122,128 @@ func TestRunTicket_HappyPath(t *testing.T) { } } +func TestExecuteVerification_AdvisoryDerivedFailureTriggersBestEffortRepair(t *testing.T) { + repoRoot := t.TempDir() + installFailingRuff(t) + + cfg := policy.DefaultConfig() + cfg.Policy.MaxImplementationAttempts = 3 + + stubToolSignals(t, ToolSignals{HasRuff: true}) + + st := newVerificationTestState(t, "run-advisory-repair", "ver-advisory-repair", cfg, 1) + st.repoRoot = repoRoot + st.implementation = &state.ImplementationArtifact{ + ChangedFiles: []string{"tests/test_smoke.py"}, + } + + blocked, err := st.executeVerification(context.Background(), repoRoot) + if err != nil { + t.Fatalf("executeVerification: %v", err) + } + if blocked { + t.Fatalf("expected advisory-only derived failure not to block") + } + if st.currentPhase != state.TicketPhaseImplement { + t.Fatalf("expected best-effort advisory repair to return to implement phase, got %q", st.currentPhase) + } + if len(st.repairCycles) != 1 { + t.Fatalf("expected one best-effort repair cycle, got %d", len(st.repairCycles)) + } + if len(st.repairCycles[0].TriggerCheckIDs) != 1 { + t.Fatalf("expected one triggering advisory check id, got %#v", st.repairCycles[0].TriggerCheckIDs) + } + if st.verification == nil || st.verification.ValidationCoverage == nil { + t.Fatalf("expected verification coverage to be recorded") + } + if !st.verification.Passed { + t.Fatalf("expected advisory-only failure to keep verification artifact passed") + } + if len(st.verification.ValidationCoverage.UnresolvedBlockers) != 0 { + t.Fatalf("expected no unresolved blockers for advisory-only failure, got %#v", st.verification.ValidationCoverage.UnresolvedBlockers) + } +} + +func TestAdvisoryFailingCheckIDs_DeclaredCheckWinsIDCollision(t *testing.T) { + coverage := state.ValidationCoverageArtifact{ + DeclaredChecks: []state.ValidationCheck{{ + ID: "same-id", + Command: "just check", + Advisory: false, + }}, + DerivedChecks: []state.ValidationCheck{{ + ID: "same-id", + Command: "just check", + Advisory: true, + }}, + ExecutedChecks: []state.ValidationCheckExecution{{ + CheckID: "same-id", + Result: state.ValidationCheckResultFailed, + }}, + } + + if got := advisoryFailingCheckIDs(coverage); len(got) != 0 { + t.Fatalf("declared check should override advisory derived collision, got %#v", got) + } +} + +func TestExecuteVerification_AdvisoryDerivedFailureDoesNotBlockAfterRepairBudget(t *testing.T) { + repoRoot := t.TempDir() + installFailingRuff(t) + + cfg := policy.DefaultConfig() + cfg.Policy.MaxImplementationAttempts = 1 + + stubToolSignals(t, ToolSignals{HasRuff: true}) + + st := newVerificationTestState(t, "run-advisory-budget", "ver-advisory-budget", cfg, 1) + st.repoRoot = repoRoot + st.implementation = &state.ImplementationArtifact{ + ChangedFiles: []string{"tests/test_smoke.py"}, + } + + blocked, err := st.executeVerification(context.Background(), repoRoot) + if err != nil { + t.Fatalf("executeVerification: %v", err) + } + if blocked { + t.Fatalf("expected advisory-only derived failure not to block after repair budget") + } + if st.currentPhase != state.TicketPhaseReview { + t.Fatalf("expected advisory-only failure to continue to review after budget, got %q", st.currentPhase) + } + if len(st.repairCycles) != 0 { + t.Fatalf("expected no extra advisory repair cycle after budget, got %d", len(st.repairCycles)) + } + if st.blockReason != "" { + t.Fatalf("expected no block reason for advisory-only failure, got %q", st.blockReason) + } + if st.verification == nil || !st.verification.Passed { + t.Fatalf("expected advisory-only failure to keep verification artifact passed") + } +} + +func installFailingRuff(t *testing.T) { + t.Helper() + binDir := t.TempDir() + ruffPath := filepath.Join(binDir, "ruff") + if err := os.WriteFile(ruffPath, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + t.Fatalf("write fake ruff: %v", err) + } + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func stubToolSignals(t *testing.T, signals ToolSignals) { + t.Helper() + originalToolSignals := toolSignalsProvider + t.Cleanup(func() { + toolSignalsProvider = originalToolSignals + }) + toolSignalsProvider = func(string) ToolSignals { + return signals + } +} + func TestRunTicket_VerifyFailureLoopsToImplement(t *testing.T) { repoRoot := t.TempDir() cfg := policy.DefaultConfig() @@ -351,7 +483,10 @@ func TestRunTicket_RenewsClaimDuringLongRunningWorker(t *testing.T) { cfg := policy.DefaultConfig() ticket := testTicket("ver-claim-renewal") plan, claim := testPlanAndClaim(t, repoRoot, ticket, cfg, "run-claim-renewal", "lease-claim-renewal", []string{`true`}) - claim.ExpiresAt = claim.LeasedAt.Add(500 * time.Millisecond) + now := time.Now().UTC() + claim.LeasedAt = now + claim.ExpiresAt = now.Add(500 * time.Millisecond) + durableClaimPath := seedClaimSnapshots(t, repoRoot, "run-claim-renewal", ticket.ID, claim) adapter := &sleepyRuntimeAdapter{ workerDelay: 250 * time.Millisecond, @@ -391,7 +526,6 @@ func TestRunTicket_RenewsClaimDuringLongRunningWorker(t *testing.T) { t.Fatal("expected run result path to be populated") } - durableClaimPath := filepath.Join(repoRoot, ".verk", "runs", "run-claim-renewal", "claims", "claim-"+ticket.ID+".json") var durableClaim state.ClaimArtifact if err := state.LoadJSON(durableClaimPath, &durableClaim); err != nil { t.Fatalf("load durable claim: %v", err) @@ -401,6 +535,68 @@ func TestRunTicket_RenewsClaimDuringLongRunningWorker(t *testing.T) { } } +func TestRunTicket_RenewsFromLiveClaimWhenRequestClaimIsStale(t *testing.T) { + repoRoot := t.TempDir() + cfg := policy.DefaultConfig() + ticket := testTicket("ver-live-renewal") + plan, requestClaim := testPlanAndClaim(t, repoRoot, ticket, cfg, "run-live-renewal", "lease-live-renewal", []string{`true`}) + + now := time.Now().UTC() + requestClaim.LeasedAt = now + requestClaim.ExpiresAt = now.Add(3 * time.Second) + + liveClaim := requestClaim + liveClaim.ExpiresAt = now.Add(900 * time.Millisecond) + durableClaimPath := seedClaimSnapshots(t, repoRoot, "run-live-renewal", ticket.ID, liveClaim) + + adapter := &sleepyRuntimeAdapter{ + reviewDelay: 1300 * time.Millisecond, + workerResult: runtime.WorkerResult{ + Status: runtime.WorkerStatusDone, + RetryClass: runtime.RetryClassTerminal, + LeaseID: requestClaim.LeaseID, + StartedAt: testRunTime(), + FinishedAt: testRunTime().Add(time.Second), + ResultArtifactPath: filepath.Join(repoRoot, "worker.json"), + }, + reviewResult: runtime.ReviewResult{ + Status: runtime.WorkerStatusDone, + RetryClass: runtime.RetryClassTerminal, + LeaseID: requestClaim.LeaseID, + StartedAt: testRunTime().Add(2 * time.Second), + FinishedAt: testRunTime().Add(3 * time.Second), + ReviewStatus: runtime.ReviewStatusPassed, + Summary: "clean", + ResultArtifactPath: filepath.Join(repoRoot, "review.json"), + }, + } + + result, err := RunTicket(context.Background(), RunTicketRequest{ + RepoRoot: repoRoot, + RunID: "run-live-renewal", + Ticket: ticket, + Plan: plan, + Claim: requestClaim, + Adapter: adapter, + Config: cfg, + }) + if err != nil { + t.Fatalf("RunTicket returned error: %v", err) + } + if result.Snapshot.CurrentPhase != state.TicketPhaseClosed { + t.Fatalf("expected closed phase, got %q", result.Snapshot.CurrentPhase) + } + + var durableClaim state.ClaimArtifact + if err := state.LoadJSON(durableClaimPath, &durableClaim); err != nil { + t.Fatalf("load durable claim: %v", err) + } + if !durableClaim.ExpiresAt.After(requestClaim.ExpiresAt) { + t.Fatalf("expected live near-expiry claim to be renewed past stale request expiry %s, got %s", + requestClaim.ExpiresAt.Format(time.RFC3339Nano), durableClaim.ExpiresAt.Format(time.RFC3339Nano)) + } +} + func TestRunTicket_RejectsStaleLeaseID(t *testing.T) { repoRoot := t.TempDir() cfg := policy.DefaultConfig() @@ -607,6 +803,158 @@ func TestRunTicket_ScopeCheckBlocksWhenOwnedPathsEmpty(t *testing.T) { } } +func TestRunTicket_ScopeMissingThenReopensToProceed(t *testing.T) { + repoRoot := t.TempDir() + cfg := policy.DefaultConfig() + + runID := "run-scope-reopen" + ticketID := "ver-scope-reopen" + ticket := testTicket(ticketID) + + // First run: no OwnedPaths set, so single-ticket scope check should block. + firstPlan, firstClaim := testPlanAndClaim(t, repoRoot, ticket, cfg, runID, "lease-scope-reopen-empty", []string{`true`}) + + started, finished := testRunTimes() + firstAdapter := runtimefake.New( + []runtime.WorkerResult{ + { + Status: runtime.WorkerStatusDone, + RetryClass: runtime.RetryClassTerminal, + LeaseID: firstClaim.LeaseID, + StartedAt: started, + FinishedAt: finished, + ResultArtifactPath: filepath.Join(repoRoot, "worker-empty.json"), + }, + }, + nil, + ) + + blockedResult, err := RunTicket(context.Background(), RunTicketRequest{ + RepoRoot: repoRoot, + RunID: runID, + Ticket: ticket, + Plan: firstPlan, + Claim: firstClaim, + Adapter: firstAdapter, + Config: cfg, + EnforceSingleScope: true, + }) + if err != nil { + t.Fatalf("first RunTicket returned error: %v", err) + } + if blockedResult.Snapshot.CurrentPhase != state.TicketPhaseBlocked { + t.Fatalf("expected blocked phase on first run, got %q", blockedResult.Snapshot.CurrentPhase) + } + if !strings.Contains(blockedResult.Snapshot.BlockReason, "single-ticket scope violation") { + t.Fatalf("expected scope violation block reason, got %q", blockedResult.Snapshot.BlockReason) + } + + // Simulate reopen flow artifacts that would normally exist in an epic run. + writeOpRunFixture(t, repoRoot, runID, state.RunArtifact{ + ArtifactMeta: state.ArtifactMeta{SchemaVersion: 1, RunID: runID}, + Mode: "epic", + RootTicketID: "epic-1", + Status: state.EpicRunStatusBlocked, + CurrentPhase: state.TicketPhaseBlocked, + TicketIDs: []string{ticketID}, + WaveIDs: []string{"wave-1"}, + }) + writeWaveFixture(t, repoRoot, runID, state.WaveArtifact{ + ArtifactMeta: state.ArtifactMeta{SchemaVersion: 1, RunID: runID}, + WaveID: "wave-1", + Ordinal: 1, + Status: state.WaveStatusFailed, + TicketIDs: []string{ticketID}, + }) + writeTicketRunFixture(t, repoRoot, runID, blockedResult.Snapshot) + writePlanFixture(t, repoRoot, runID, firstPlan) + writeTicketMarkdownFixture(t, repoRoot, tkmd.Ticket{ + ID: ticketID, + Title: ticket.Title, + Status: tkmd.StatusBlocked, + OwnedPaths: nil, + UnknownFrontmatter: map[string]any{"type": "task"}, + }) + + // Operator adds scope declarations and reopens the ticket. + if err := ReopenTicket(context.Background(), ReopenRequest{ + RepoRoot: repoRoot, + RunID: runID, + TicketID: ticketID, + ToPhase: state.TicketPhaseImplement, + }); err != nil { + t.Fatalf("ReopenTicket returned error: %v", err) + } + + reopenedTicket, err := tkmd.LoadTicket(ticketMarkdownPath(repoRoot, ticketID)) + if err != nil { + t.Fatalf("load reopened ticket: %v", err) + } + if reopenedTicket.Status != tkmd.StatusOpen { + t.Fatalf("expected ticket status to become open after reopen, got %q", reopenedTicket.Status) + } + reopenedTicket.OwnedPaths = []string{"internal/engine"} + if err := tkmd.SaveTicket(filepath.Join(repoRoot, ".tickets", ticketID+".md"), reopenedTicket); err != nil { + t.Fatalf("save reopened ticket: %v", err) + } + + reopenedPlan, reopenedClaim := testPlanAndClaim( + t, + repoRoot, + reopenedTicket, + cfg, + runID, + "lease-scope-reopen-scope", + []string{`true`}, + ) + + started, finished = testRunTimes() + reopenedAdapter := runtimefake.New( + []runtime.WorkerResult{ + { + Status: runtime.WorkerStatusDone, + RetryClass: runtime.RetryClassTerminal, + LeaseID: reopenedClaim.LeaseID, + StartedAt: started, + FinishedAt: finished, + ResultArtifactPath: filepath.Join(repoRoot, "worker-scope.json"), + }, + }, + []runtime.ReviewResult{ + { + Status: runtime.WorkerStatusDone, + RetryClass: runtime.RetryClassTerminal, + LeaseID: reopenedClaim.LeaseID, + StartedAt: finished.Add(time.Second), + FinishedAt: finished.Add(2 * time.Second), + ReviewStatus: runtime.ReviewStatusPassed, + Summary: "clean", + ResultArtifactPath: filepath.Join(repoRoot, "review-scope.json"), + }, + }, + ) + + reopenedResult, err := RunTicket(context.Background(), RunTicketRequest{ + RepoRoot: repoRoot, + RunID: runID, + Ticket: reopenedTicket, + Plan: reopenedPlan, + Claim: reopenedClaim, + Adapter: reopenedAdapter, + Config: cfg, + EnforceSingleScope: true, + }) + if err != nil { + t.Fatalf("second RunTicket returned error: %v", err) + } + if reopenedResult.Snapshot.CurrentPhase != state.TicketPhaseClosed { + t.Fatalf("expected ticket to proceed to closed, got %q", reopenedResult.Snapshot.CurrentPhase) + } + if reopenedResult.Snapshot.BlockReason != "" { + t.Fatalf("expected no block reason after scope is declared, got %q", reopenedResult.Snapshot.BlockReason) + } +} + func TestRunTicket_WorkerBlockReasonRoundTrips(t *testing.T) { repoRoot := t.TempDir() cfg := policy.DefaultConfig() @@ -742,13 +1090,25 @@ func testPlanAndClaim(t *testing.T, repoRoot string, ticket tkmd.Ticket, cfg pol } plan.ValidationCommands = append([]string(nil), verificationCommands...) - claim, err := tkmd.AcquireClaim(repoRoot, runID, ticket.ID, leaseID, 10*time.Minute, testRunTime()) + claim, err := tkmd.AcquireClaim(repoRoot, runID, ticket.ID, leaseID, 10*time.Minute, time.Now().UTC()) if err != nil { t.Fatalf("AcquireClaim: %v", err) } return plan, claim } +func seedClaimSnapshots(t *testing.T, repoRoot, runID, ticketID string, claim state.ClaimArtifact) string { + t.Helper() + if err := state.SaveJSONAtomic(liveClaimPath(repoRoot, ticketID), claim); err != nil { + t.Fatalf("seed live claim: %v", err) + } + durableClaimPath := filepath.Join(repoRoot, ".verk", "runs", runID, "claims", "claim-"+ticketID+".json") + if err := state.SaveJSONAtomic(durableClaimPath, claim); err != nil { + t.Fatalf("seed durable claim: %v", err) + } + return durableClaimPath +} + type sleepyRuntimeAdapter struct { workerDelay time.Duration reviewDelay time.Duration @@ -973,7 +1333,7 @@ func TestCollectChangedFiles_ReturnsErrorOnInvalidBaseCommit(t *testing.T) { } } -func TestRemainingTTL_ComputesFromNow(t *testing.T) { +func TestCurrentClaimRemainingTTL_FallsBackToRequestClaim(t *testing.T) { now := time.Now().UTC() st := &ticketRunState{ req: RunTicketRequest{ @@ -989,24 +1349,28 @@ func TestRemainingTTL_ComputesFromNow(t *testing.T) { t.Fatalf("expected claimTTL to be 30m (original full TTL), got %v", ttl) } - remaining := st.remainingTTL() - // remainingTTL should be approximately 20m, not 30m + remaining, known := st.currentClaimRemainingTTL() + if !known { + t.Fatal("expected request claim expiry to provide known remaining TTL") + } + // currentClaimRemainingTTL should be approximately 20m, not 30m. if remaining >= 30*time.Minute { - t.Fatalf("expected remainingTTL < 30m (should be ~20m), got %v", remaining) + t.Fatalf("expected current remaining TTL < 30m (should be ~20m), got %v", remaining) } if remaining < 15*time.Minute { - t.Fatalf("expected remainingTTL > 15m (should be ~20m), got %v", remaining) + t.Fatalf("expected current remaining TTL > 15m (should be ~20m), got %v", remaining) } } -func TestRemainingTTL_ZeroExpiresAt(t *testing.T) { +func TestCurrentClaimRemainingTTL_ZeroExpiresAt(t *testing.T) { st := &ticketRunState{ req: RunTicketRequest{ Claim: state.ClaimArtifact{}, }, } - if st.remainingTTL() != 0 { - t.Fatalf("expected 0 remainingTTL for zero ExpiresAt, got %v", st.remainingTTL()) + remaining, known := st.currentClaimRemainingTTL() + if known { + t.Fatalf("expected unknown remaining TTL for zero ExpiresAt, got %v", remaining) } } @@ -1234,7 +1598,7 @@ func TestRunTicket_ReleasesClaimOnStartupFailure(t *testing.T) { // TestRunTicket_RenewsResumedClaimBeforeExpiry verifies that a resumed claim // whose LeasedAt was long ago but ExpiresAt is imminent gets renewed before it -// expires (ver-exae). The renewal cadence must use remainingTTL(), not +// expires (ver-exae). The renewal cadence must use current remaining TTL, not // claimTTL(), so a claim with a 30-minute total TTL acquired 29m55s ago // schedules its first renewal within seconds instead of waiting 10 minutes. func TestRunTicket_RenewsResumedClaimBeforeExpiry(t *testing.T) { @@ -1261,8 +1625,8 @@ func TestRunTicket_RenewsResumedClaimBeforeExpiry(t *testing.T) { } // Worker completes in 250ms — within the 500ms expiry window. With the old - // cadence (claimTTL/3 ≈ 10min) renewal would never fire. With the fix - // (remainingTTL/3 ≈ 167ms) renewal fires well before expiry. + // cadence (claimTTL/3 ~= 10min) renewal would never fire. With the fix + // (current remaining TTL/3 ~= 167ms) renewal fires well before expiry. adapter := &sleepyRuntimeAdapter{ workerDelay: 250 * time.Millisecond, workerResult: runtime.WorkerResult{ @@ -1333,16 +1697,66 @@ func TestTicketRunState_snapshotPreservesCreatedAt(t *testing.T) { } } +func TestTicketRunState_snapshotOutcome(t *testing.T) { + cases := []struct { + name string + phase state.TicketPhase + want state.TicketOutcome + }{ + {name: "active", phase: state.TicketPhaseVerify, want: ""}, + {name: "closed", phase: state.TicketPhaseClosed, want: state.TicketOutcomeClosed}, + {name: "legacy_blocked", phase: state.TicketPhaseBlocked, want: state.TicketOutcomeBlocked}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + st := &ticketRunState{ + req: RunTicketRequest{ + RunID: "run-snapshot-outcome", + Ticket: tkmd.Ticket{ID: "ver-snapshot-outcome"}, + }, + currentPhase: tc.phase, + } + + snap := st.snapshot() + if snap.Outcome != tc.want { + t.Fatalf("expected outcome %q for phase %q, got %q", tc.want, tc.phase, snap.Outcome) + } + }) + } +} + func TestTicketRunState_snapshotPreservesRestoredCreatedAt(t *testing.T) { + repoRoot := t.TempDir() + runID := "run-snapshot-restore" + ticketID := "ver-snapshot-restore" fixedCreatedAt := time.Date(2025, 1, 15, 10, 30, 0, 0, time.UTC) + fixture := TicketRunSnapshot{ + ArtifactMeta: state.ArtifactMeta{ + SchemaVersion: artifactSchemaVersion, + RunID: runID, + CreatedAt: fixedCreatedAt, + UpdatedAt: fixedCreatedAt, + }, + TicketID: ticketID, + CurrentPhase: state.TicketPhaseImplement, + } + if err := state.SaveJSONAtomic(ticketSnapshotPath(repoRoot, runID, ticketID), fixture); err != nil { + t.Fatalf("seed ticket snapshot: %v", err) + } + + var loaded TicketRunSnapshot + if err := loadTicketSnapshot(repoRoot, runID, ticketID, &loaded); err != nil { + t.Fatalf("load persisted ticket snapshot: %v", err) + } st := &ticketRunState{ req: RunTicketRequest{ - RunID: "run-snapshot-restore", - Ticket: tkmd.Ticket{ID: "ver-snapshot-restore"}, + RunID: runID, + Ticket: tkmd.Ticket{ID: ticketID}, }, - createdAt: fixedCreatedAt, } + st.createdAt = loaded.CreatedAt snap := st.snapshot() diff --git a/internal/state/types.go b/internal/state/types.go index 05a6523..c95fcb4 100644 --- a/internal/state/types.go +++ b/internal/state/types.go @@ -4,6 +4,7 @@ import "time" type ( TicketPhase string + TicketOutcome string EpicRunStatus string WaveStatus string RetryClass string @@ -21,6 +22,14 @@ const ( TicketPhaseBlocked TicketPhase = "blocked" ) +const ( + TicketOutcomeClosed TicketOutcome = "closed" + TicketOutcomeFailedRetryable TicketOutcome = "failed_retryable" + TicketOutcomeNeedsDecision TicketOutcome = "needs_decision" + TicketOutcomeBlocked TicketOutcome = "blocked" + TicketOutcomeCancelled TicketOutcome = "cancelled" +) + const ( EpicRunStatusRunning EpicRunStatus = "running" EpicRunStatusWaitingOnLeases EpicRunStatus = "waiting_on_leases" diff --git a/tools.mod b/tools.mod index c7731fc..d04d7a8 100644 --- a/tools.mod +++ b/tools.mod @@ -1,3 +1,5 @@ +replace verk => . + module verk/tools go 1.26