feat: epos integration#5
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a new EPOS ticket/claims adapter and migrates engine, CLI, and tests from TKMD to EPOS. Adds strict identifier/worktree validation, hardens git subprocess environment, updates diff handling, bumps dependencies, and removes TKMD code. Documentation and build scripts are updated accordingly. ChangesEPOS adapter migration and safety hardening
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/adapters/repo/git/repo.go (1)
113-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently fall back to the linked worktree root.
If
--path-format=absoluteisn't supported (Git < 2.31.0), returningr.rootmakesCreateWorktreeanchor new worktrees under a linked checkout instead of the main worktree. That reintroduces the nested-worktree case this PR is explicitly trying to avoid.🛠️ Safer fallback
commonDir, err := gitOutput(r.root, "rev-parse", "--path-format=absolute", "--git-common-dir") if err != nil { - // Fallback: if git doesn't support --path-format, use the current root - return r.root, nil //nolint:nilerr // intentional fallback — error handled by returning root + commonDir, err = gitOutput(r.root, "rev-parse", "--git-common-dir") + if err != nil { + return "", err + } } commonDir = filepath.Clean(strings.TrimRight(commonDir, "\r\n")) + if !filepath.IsAbs(commonDir) { + commonDir = filepath.Join(r.root, commonDir) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/repo/git/repo.go` around lines 113 - 117, Don't silently fall back to r.root when git doesn't support --path-format; instead call gitOutput(r.root, "rev-parse", "--git-common-dir") to get the common dir, then if that output is a relative path resolve it against r.root (e.g., filepath.Join(r.root, commonDir)) to produce an absolute path, return that absolute commonDir, and only return an error if gitOutput fails — update the logic around the commonDir variable where gitOutput(... "--path-format=absolute", "--git-common-dir") is used so CreateWorktree doesn't get anchored to r.root.internal/engine/epic_gate.go (1)
577-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't opt epic closure checks out of the isolated subprocess env.
Both verify-runner calls pass
""for the new worktree/state hint. That disables the cache-root isolation added in this PR, so epic closure and derived checks will still share host/global caches instead of running under the worktree-scoped.verk/process-cache.Suggested fix
- results, err := verifycommand.RunQualityCommands(ctx, repoRoot, "", cfg.EpicClosureCommands, cfg) + results, err := verifycommand.RunQualityCommands(ctx, repoRoot, repoRoot, cfg.EpicClosureCommands, cfg) @@ - results, err := verifycommand.RunCommands(ctx, repoRoot, "", commands, cfg) + results, err := verifycommand.RunCommands(ctx, repoRoot, repoRoot, commands, cfg)Based on learnings: If a task touches worktree behavior, preserve the cache-root semantics described in
README.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/epic_gate.go` around lines 577 - 620, In executeEpicChecks, don't pass the empty string as the worktree/state hint to verifycommand.RunQualityCommands and verifycommand.RunCommands because that opt-outs of the new cache-root isolation; update both calls (verifycommand.RunQualityCommands and verifycommand.RunCommands) to pass the appropriate worktree hint (e.g., repoRoot or the worktree-scoped hint you use elsewhere) instead of "" so epic closure commands and derived checks run under the worktree-scoped cache root.
🧹 Nitpick comments (8)
internal/engine/closeout.go (1)
272-313: ⚡ Quick winImprove type-mismatch diagnostics in variadic closeout parsing.
parseCloseoutRequest(args ...any)silently drops unsupported types, so a stale caller can fail with onlycloseout requires ticket metadata, which is hard to debug during adapter migration. Including received arg types in that error would make failures immediately actionable.Proposed patch
func parseCloseoutRequest(args ...any) (closeoutRequest, error) { var req closeoutRequest + gotTypes := make([]string, 0, len(args)) for _, arg := range args { + gotTypes = append(gotTypes, fmt.Sprintf("%T", arg)) switch v := arg.(type) { case epos.Ticket: req.ticket = v case *epos.Ticket: if v != nil { req.ticket = *v } @@ } if req.ticket.ID == "" { - return closeoutRequest{}, fmt.Errorf("closeout requires ticket metadata") + return closeoutRequest{}, fmt.Errorf( + "closeout requires ticket metadata; received arg types: %s", + strings.Join(gotTypes, ", "), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/closeout.go` around lines 272 - 313, The parseCloseoutRequest variadic parser currently ignores unsupported argument types, leading to unhelpful "closeout requires ticket metadata" errors; update parseCloseoutRequest to collect the runtime types of all args (e.g., via fmt.Sprintf("%T", arg) or reflect.TypeOf) and include a concise summary of received argument types in the returned error when req.ticket.ID == "" (and/or include unsupported types encountered), referencing parseCloseoutRequest, closeoutRequest and ticket.ID so callers can immediately see what types were passed during adapter migrations.docs/plans/worker-isolation.md (1)
349-367: Add the repo-standard verification targets here.This checklist should call out
just pre-commitandjust test-raceso the documented validation path matches the repo's required gates for engine/worktree/orchestration changes.Based on learnings: Use the repo's
justtargets instead of ad hoc command bundles; run at leastjust pre-commitbefore engine changes, and runjust test-raceif the change affects orchestration, isolation, or cleanup semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/worker-isolation.md` around lines 349 - 367, Update the "Verification" checklist to include the repo-standard Just targets by adding explicit steps to run `just pre-commit` (before engine/worktree changes) and `just test-race` (when changes affect orchestration, isolation, or cleanup semantics); modify the Verification section (the numbered checklist under the "Verification" header) to insert these two targets as required gates alongside the existing unit/integration/resume tests so the documented validation path matches repo requirements.internal/adapters/ticketstore/epos/claims.go (1)
653-655: 💤 Low valueHeuristic for distinguishing lease ID from release reason may be fragile.
The
looksLikeLeaseIDfunction checks for"lease-"or"fence-"prefixes. If a release reason accidentally starts with these prefixes (e.g.,"lease expired by timeout"), it would be misclassified as a lease ID, causing validation to fail.Consider documenting this constraint or using a more robust disambiguation strategy (e.g., always requiring explicit argument positions when both are needed).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/ticketstore/epos/claims.go` around lines 653 - 655, The heuristic in looksLikeLeaseID is too loose and can misclassify freeform release reasons that start with "lease-" or "fence-"; update looksLikeLeaseID to use a stricter pattern (e.g., a regex that requires the expected suffix format like lease-[0-9a-fA-F]+ or a UUID pattern for fence-) or change call sites to require an explicit positional flag instead of guessing; also add a short comment/docstring on looksLikeLeaseID explaining the accepted lease/fence formats so future callers know the constraint.internal/adapters/ticketstore/epos/convert.go (1)
300-311: 💤 Low valueConsider extending integer type coverage in
asInt.The function handles
int,int64, andfloat64, but silently returns0for other numeric types likeint32,uint,uint64. If YAML unmarshaling or external sources provide these types, valid integers could be lost.♻️ Optional: extend numeric type handling
func asInt(v any) int { switch t := v.(type) { case int: return t case int64: return int(t) + case int32: + return int(t) + case uint: + return int(t) + case uint64: + return int(t) case float64: return int(t) default: return 0 } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/ticketstore/epos/convert.go` around lines 300 - 311, The asInt function currently only handles int, int64 and float64 and returns 0 for other numeric types; update asInt to handle additional numeric types (e.g., int8, int16, int32, uint, uint8, uint16, uint32, uint64) by adding type switch cases that convert them to int safely (casting where appropriate) and optionally handle json.Number or string numeric input by parsing to int; ensure conversions for unsigned types check for overflow if needed and keep the function name asInt to locate the change.docs/epos-integration.md (1)
218-230: ⚡ Quick winAlign the execution plan with the repo’s
justgates.The phase and verification sections still center ad hoc
go build ./.../go test ./...commands, but this change touches engine/worktree/isolation behavior. The handoff should explicitly call outjust pre-commitandjust test-race, withjust checkas the final gate, so the plan matches the repo’s expected validation path. Based on learnings: Use the repo'sjusttargets instead of ad hoc command bundles, and for worktree/isolation changes run at leastjust pre-commitandjust test-race.Also applies to: 387-399, 424-425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/epos-integration.md` around lines 218 - 230, Update the execution and verification steps in the "Phase 1 — Pre-flight + dependency add" section (and the other referenced blocks) to use the repository's just targets instead of ad hoc go commands: replace step 5's "go mod tidy && go build ./..." and the "**Exit:** go build ./... + go test ./... green" with explicit calls to "just pre-commit" and "just test-race" during worktree/isolation validation and use "just check" as the final gate; apply the same replacement to the other affected blocks (the later sections noted in the comment) so all verification steps consistently call just pre-commit, just test-race, and just check.internal/engine/wave_commands_test.go (1)
24-24: ⚡ Quick winAdd a test that exercises non-empty
workDirbehavior.Line 24 updates the signature but still validates only the empty/default path. Please add one case that passes an explicit workDir and asserts outputs are written there (not repo root), so the new argument is actually covered.
✅ Suggested test addition
func TestRunWaveVerificationLoop_RunsWaveCommandsInAdditionToQualityCommands(t *testing.T) { @@ err := runWaveVerificationLoop(context.Background(), makeEpicReq(repoRoot, adapter), cfg, wave, wavePath, nil, "") @@ } + +func TestRunWaveVerificationLoop_UsesExplicitWorkDir(t *testing.T) { + repoRoot, wavePath, wave := makeWaveVerifyFixture(t) + workDir := filepath.Join(repoRoot, "wt") + if err := os.MkdirAll(workDir, 0o755); err != nil { + t.Fatalf("mkdir workDir: %v", err) + } + adapter := runtimefake.New(nil, nil) + cfg := policy.DefaultConfig() + cfg.Verification.QualityCommands = []policy.QualityCommand{ + {Path: ".", Run: []string{"printf scoped >> wave-checks.log"}}, + } + + err := runWaveVerificationLoop(context.Background(), makeEpicReq(repoRoot, adapter), cfg, wave, wavePath, nil, workDir) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if _, err := os.Stat(filepath.Join(repoRoot, "wave-checks.log")); !os.IsNotExist(err) { + t.Fatalf("expected no repo-root output file when workDir is set, got err=%v", err) + } + content, err := os.ReadFile(filepath.Join(workDir, "wave-checks.log")) + if err != nil { + t.Fatalf("read workDir wave checks log: %v", err) + } + if got, want := string(content), "scoped"; got != want { + t.Fatalf("expected workDir output %q, got %q", want, got) + } +}Based on learnings: "If a task touches worktree behavior, preserve the cache-root semantics described in
README.md."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/wave_commands_test.go` at line 24, The test currently calls runWaveVerificationLoop only with an empty/default workDir; add a new subtest in wave_commands_test.go that calls runWaveVerificationLoop (via makeEpicReq(repoRoot, adapter) or similar) but passes a non-empty workDir (e.g., a temp dir) and then asserts that outputs/artifacts (the same files the existing test checks) are created under that workDir and not under repoRoot; ensure you clean up the temp dir and preserve the cache-root semantics described in README when constructing the expected paths so the new argument is actually covered.internal/adapters/verify/command/runner.go (1)
181-185: ⚡ Quick winUpdate the
QualityCommand.Pathcontract comment.The implementation now resolves
qc.PathfromworkDir, not directly fromrepoRoot. Leaving the old comment here will send future callers and config changes at the wrong base directory once verification runs inside a worktree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/verify/command/runner.go` around lines 181 - 185, Update the comment for QualityCommand.Path to reflect that paths are resolved relative to workDir (the current working tree used by RunQualityCommands) rather than repoRoot; change the contract text near the QualityCommand.Path declaration and any related docblock in runner.go to state that qc.Path is interpreted relative to workDir so callers/configs use the worktree base when verification runs inside a worktree.internal/engine/wave_verify.go (1)
31-205: Please gate this flow with the repo's race coverage.This PR changes worktree isolation, wave verification resume, and pending integration cleanup semantics. It is worth running the repo’s engine/worktree checks with
just pre-commitandjust test-racebefore merge.Based on learnings: Run at least
just pre-commitbefore engine changes, and runjust test-raceif the change affects orchestration, isolation, or cleanup semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/wave_verify.go` around lines 31 - 205, Add a guard at the start of runWaveVerificationLoop to run the repo's race/engine checks and abort if they fail: call a helper like ensureRepoRaceCoverage(ctx, req.RepoRoot) (or implement verifyPreCommitAndRace that runs the equivalent of `just pre-commit` and `just test-race`) before computing plan := waveDerivationPlan(...); if that helper returns an error, return it (wrap with context like "race coverage checks failed") so wave verification is gated by the repo's race/engine checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 37-42: The "Useful focused runs" block conflicts with the repo
rule to use the project's just targets; either remove the ad hoc go test lines
or mark them explicitly as exceptions and explain why (e.g., for quick local
troubleshooting) and link or reference the canonical just targets (the "just"
targets / rule) so readers know the preferred commands; update the AGENTS.md
section title or a one-line note so it clearly resolves the contradiction.
In `@docs/epos-integration.md`:
- Around line 5-6: The handoff document contains hardcoded workstation-specific
absolute paths (e.g., /Users/runger/workspaces/...) which break portability;
update the occurrences (including the ones around lines referenced) to use
repo-relative paths or explicit placeholders (e.g., $VERK_ROOT or $EPOS_ROOT)
and update any example commands to reference these placeholders or relative
paths instead of absolute user directories so CI and other developers can run
them reliably.
In `@docs/plans/2026-04-21-ticket-quality-gate.md`:
- Around line 232-233: The plan mixes old `tkmd` names with the new `epos`
adapter: update every occurrence of `tkmd.Ticket`, `tkmd.LoadTicket`,
`tkmd.SaveTicket`, and `tkmd.ListAllChildren` (and any other `tkmd.*`
references) to the corresponding `epos` names (e.g., `epos.Ticket`,
`epos.LoadTicket`, `epos.SaveTicket`, `epos.ListAllChildren`) so the task
descriptions, instructions, and reuse lines consistently reference `epos`
throughout the document; search for `tkmd.` tokens in the plan and replace them
with `epos.` and verify the two reuse bullets already pointing to
`internal/adapters/ticketstore/epos/*` align with the updated symbols.
In `@internal/adapters/runtime/cacheenv.go`:
- Around line 81-93: The function resolveProcessStateRoot should not create a
new .verk directory when filepath.EvalSymlinks(verkPath) fails because that
masks worktree mappings; instead fail fast. In resolveProcessStateRoot, remove
or bypass the os.MkdirAll(verkPath, 0o755) fallback and return a wrapped error
indicating that resolving verkPath (variable verkPath) failed; keep the initial
Abs/TrimSpace handling and the successful EvalSymlinks branch unchanged so
callers can detect unresolved worktree cases and handle canonical state-root
resolution.
In `@internal/adapters/ticketstore/epos/live.go`:
- Around line 34-35: The persisted LastSeenLiveClaimPath uses filepath.Join
which emits OS-specific separators; change liveClaimRelativePath to use
path.Join instead of filepath.Join so the stored relative path always uses
forward slashes, update the corresponding test in claims_test.go to assert using
path.Join (not filepath.Join), and adjust imports (add "path" and remove
"path/filepath" if it becomes unused) so the on-disk JSON format is stable
across platforms.
In `@internal/adapters/verify/command/runner_test.go`:
- Around line 311-339: The test creates workDir as an unrelated temp dir and
never ties it to repoRoot, so the prefix check is ineffective; change workDir to
be a path under repoRoot (e.g. workDir := filepath.Join(repoRoot, "worktree")),
create that directory, and then keep the existing
BuildIsolatedProcessEnv(verificationEnv(nil), repoRoot) call but assert that
TMPDIR, GOCACHE, GOTMPDIR, XDG_CACHE_HOME do not have a prefix matching the
canonicalized workDir (use filepath.Clean and filepath.Separator as already
used) so the test verifies isolation against the real worktree location; update
references to workDir in the prefix check accordingly in
TestVerificationEnv_IsolatesToolCachesOutsideWorkingTreeOutputs.
In `@internal/cli/run_current_pointer_test.go`:
- Around line 38-58: testGitEnv currently strips repo-local GIT_* entries but
still allows system/global Git configs to be read; update testGitEnv (and the
similar block at the other occurrence) to disable system/global configs by
appending environment variables such as GIT_CONFIG_NOSYSTEM=1 plus a sanitized
home/config location (e.g. set HOME and XDG_CONFIG_HOME to an isolated empty
directory path or a known non-existent path) so Git cannot pick up ~/.gitconfig
or system config; change references to testGitEnv to include these additional
env entries alongside the existing GIT_CONFIG_* and core.hooksPath entries.
In `@internal/e2e/helpers_test.go`:
- Around line 55-72: testGitEnv currently filters only GIT_DIR-style vars but
leaves GIT_CONFIG_*/GIT_CONFIG_COUNT and doesn't disable hooks; update
testGitEnv to also strip any environment entries whose key starts with
"GIT_CONFIG_" and "GIT_CONFIG_COUNT" and ensure it sets core.hooksPath to an
empty path for repository commands (e.g., by adding "GIT_CONFIG_PARAMETERS=" or
an env that forces core.hooksPath to empty) so initRepo/git commit cannot read
host config or run host hooks; search for the function testGitEnv and the
initRepo usage to implement these changes and ensure the returned env vector
includes the necessary variables that harden git (remove GIT_CONFIG_* and
GIT_CONFIG_COUNT and add the hook-disable env) before returning.
In `@internal/engine/resume.go`:
- Around line 708-739: resumeEpicMode currently dereferences waveManager inside
the merge branch (calling changedFilesFromManager and waveManager.ChangedFiles)
without guarding for nil, which can crash when worktree preparation was skipped;
add a nil-check at the start of that branch (the block starting with "if
len(mergeEligibleTicketIDs) > 0 && conflictErr == nil && acceptErr == nil") to
skip merge-related operations when waveManager == nil, or alternatively handle
the case by returning a clear error after cleaning up; specifically, ensure you
check waveManager == nil before calling changedFilesFromManager,
waveManager.ChangedFiles, waveManager.WorktreePath or any methods on waveManager
so resumeEpicMode and prepareWaveIntegration/integration.ApplyAcceptedTicketRefs
only run when waveManager is present.
In `@internal/engine/ticket_run_test.go`:
- Around line 249-255: The linked worktree is being created inside repoRoot
(worktreePath := filepath.Join(repoRoot, "worktree-review")), which can mask
bugs that ignore WorktreePath; change the setup to create the worktree outside
repoRoot (e.g., use os.MkdirTemp to get a temp directory and assign worktreePath
to that temp dir, or join a parent of repoRoot), then call mustRunGit with the
new worktreePath and create/commit "ticket-only.txt" there; also add cleanup
(defer os.RemoveAll(worktreePath)) so the external temp worktree is removed
after the test.
In `@internal/engine/wave_verify.go`:
- Around line 1172-1179: The code currently rejects recovery unless
finalChangedFiles exactly equals tx.ChangedFiles; change this to accept cases
where the finalChangedFiles include the originally recorded tx.ChangedFiles
(i.e., tx.ChangedFiles is a subset of finalChangedFiles) so legitimately
added/replaced files during repair don’t block clearing the pending cursor. In
the block after finalIntegratedChangedFiles(...) replace the samePathSet(...)
equality check with a subset check that verifies every path in tx.ChangedFiles
exists in finalChangedFiles, and if so continue to call
persistCompletedPendingWaveIntegration(...); only return false when any
tx.ChangedFiles entry is missing from finalChangedFiles.
- Around line 1091-1098: The code blindly reapplies tx.AcceptedRefs during
rehydrate which can be empty for older/partial pending_wave_integration blobs
and cause accepted ticket changes to be dropped; add a fail-closed guard in the
rehydrate path: before calling
integration.ApplyAcceptedTicketRefs(context.Background(), tx.AcceptedRefs) check
that tx.AcceptedRefs is present and non-empty (or otherwise validate its shape)
and if it is missing/empty return a clear error (after cleaning up the
integration via integration.Cleanup()) instead of proceeding; update logic
around prepareWaveIntegration, ApplyAcceptedTicketRefs and any callers that
assume missing accepted_refs is safe so the system preserves previous behavior
for incompatible on-disk payloads.
In `@internal/engine/worktree.go`:
- Around line 84-106: Reject unsafe run/ticket ID values before using them in
filesystem paths or git refs: add a validation helper (e.g.,
validateRunOrTicketID) that checks trimmed IDs for empty, any path separators
(os.PathSeparator or "/" and "\"), any ".." path traversal segments, and
ref-special sequences (leading '.', ending '.', or sequences invalid for git
refs), then call it from WorktreeManager.CreateWorktree (validate wm.runID and
ticketID before building worktreePath and refs) and from the other location
referenced (lines ~337-343) so the same validated values are reused whenever
constructing filepath.Join(...) or refs/verk/runs/.... If validation fails,
return a clear error; keep locking and rest of CreateWorktree unchanged.
- Around line 1356-1375: resolveWithinRoot currently only does lexical checks;
update it to be symlink-aware by resolving existing ancestor paths (not the
possibly-nonexistent target) and rejecting any that escape absRoot. After
computing absRoot and absTarget, walk from filepath.Dir(absTarget) up to absRoot
(inclusive) and for each existing ancestor call filepath.EvalSymlinks (or
os.Lstat + EvalSymlinks) to get the real path and verify the resolved path has
absRoot as its prefix; if any resolved ancestor is outside absRoot return an
error like "target %q is outside root %q". Keep the original lexical Rel check
as a final guard but ensure ancestors that are symlinks that would escape are
detected and rejected in resolveWithinRoot.
---
Outside diff comments:
In `@internal/adapters/repo/git/repo.go`:
- Around line 113-117: Don't silently fall back to r.root when git doesn't
support --path-format; instead call gitOutput(r.root, "rev-parse",
"--git-common-dir") to get the common dir, then if that output is a relative
path resolve it against r.root (e.g., filepath.Join(r.root, commonDir)) to
produce an absolute path, return that absolute commonDir, and only return an
error if gitOutput fails — update the logic around the commonDir variable where
gitOutput(... "--path-format=absolute", "--git-common-dir") is used so
CreateWorktree doesn't get anchored to r.root.
In `@internal/engine/epic_gate.go`:
- Around line 577-620: In executeEpicChecks, don't pass the empty string as the
worktree/state hint to verifycommand.RunQualityCommands and
verifycommand.RunCommands because that opt-outs of the new cache-root isolation;
update both calls (verifycommand.RunQualityCommands and
verifycommand.RunCommands) to pass the appropriate worktree hint (e.g., repoRoot
or the worktree-scoped hint you use elsewhere) instead of "" so epic closure
commands and derived checks run under the worktree-scoped cache root.
---
Nitpick comments:
In `@docs/epos-integration.md`:
- Around line 218-230: Update the execution and verification steps in the "Phase
1 — Pre-flight + dependency add" section (and the other referenced blocks) to
use the repository's just targets instead of ad hoc go commands: replace step
5's "go mod tidy && go build ./..." and the "**Exit:** go build ./... + go test
./... green" with explicit calls to "just pre-commit" and "just test-race"
during worktree/isolation validation and use "just check" as the final gate;
apply the same replacement to the other affected blocks (the later sections
noted in the comment) so all verification steps consistently call just
pre-commit, just test-race, and just check.
In `@docs/plans/worker-isolation.md`:
- Around line 349-367: Update the "Verification" checklist to include the
repo-standard Just targets by adding explicit steps to run `just pre-commit`
(before engine/worktree changes) and `just test-race` (when changes affect
orchestration, isolation, or cleanup semantics); modify the Verification section
(the numbered checklist under the "Verification" header) to insert these two
targets as required gates alongside the existing unit/integration/resume tests
so the documented validation path matches repo requirements.
In `@internal/adapters/ticketstore/epos/claims.go`:
- Around line 653-655: The heuristic in looksLikeLeaseID is too loose and can
misclassify freeform release reasons that start with "lease-" or "fence-";
update looksLikeLeaseID to use a stricter pattern (e.g., a regex that requires
the expected suffix format like lease-[0-9a-fA-F]+ or a UUID pattern for fence-)
or change call sites to require an explicit positional flag instead of guessing;
also add a short comment/docstring on looksLikeLeaseID explaining the accepted
lease/fence formats so future callers know the constraint.
In `@internal/adapters/ticketstore/epos/convert.go`:
- Around line 300-311: The asInt function currently only handles int, int64 and
float64 and returns 0 for other numeric types; update asInt to handle additional
numeric types (e.g., int8, int16, int32, uint, uint8, uint16, uint32, uint64) by
adding type switch cases that convert them to int safely (casting where
appropriate) and optionally handle json.Number or string numeric input by
parsing to int; ensure conversions for unsigned types check for overflow if
needed and keep the function name asInt to locate the change.
In `@internal/adapters/verify/command/runner.go`:
- Around line 181-185: Update the comment for QualityCommand.Path to reflect
that paths are resolved relative to workDir (the current working tree used by
RunQualityCommands) rather than repoRoot; change the contract text near the
QualityCommand.Path declaration and any related docblock in runner.go to state
that qc.Path is interpreted relative to workDir so callers/configs use the
worktree base when verification runs inside a worktree.
In `@internal/engine/closeout.go`:
- Around line 272-313: The parseCloseoutRequest variadic parser currently
ignores unsupported argument types, leading to unhelpful "closeout requires
ticket metadata" errors; update parseCloseoutRequest to collect the runtime
types of all args (e.g., via fmt.Sprintf("%T", arg) or reflect.TypeOf) and
include a concise summary of received argument types in the returned error when
req.ticket.ID == "" (and/or include unsupported types encountered), referencing
parseCloseoutRequest, closeoutRequest and ticket.ID so callers can immediately
see what types were passed during adapter migrations.
In `@internal/engine/wave_commands_test.go`:
- Line 24: The test currently calls runWaveVerificationLoop only with an
empty/default workDir; add a new subtest in wave_commands_test.go that calls
runWaveVerificationLoop (via makeEpicReq(repoRoot, adapter) or similar) but
passes a non-empty workDir (e.g., a temp dir) and then asserts that
outputs/artifacts (the same files the existing test checks) are created under
that workDir and not under repoRoot; ensure you clean up the temp dir and
preserve the cache-root semantics described in README when constructing the
expected paths so the new argument is actually covered.
In `@internal/engine/wave_verify.go`:
- Around line 31-205: Add a guard at the start of runWaveVerificationLoop to run
the repo's race/engine checks and abort if they fail: call a helper like
ensureRepoRaceCoverage(ctx, req.RepoRoot) (or implement verifyPreCommitAndRace
that runs the equivalent of `just pre-commit` and `just test-race`) before
computing plan := waveDerivationPlan(...); if that helper returns an error,
return it (wrap with context like "race coverage checks failed") so wave
verification is gated by the repo's race/engine checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e49b0295-2e29-4460-8724-055f5d24a0a5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (83)
.gitignore.verk/config.yamlAGENTS.mdCLAUDE.mdREADME.mdcmd/verk/main_test.godoc.godocs/epos-integration.mddocs/plans/2026-04-19-verk-as-skill-cross-agent.mddocs/plans/2026-04-21-agent-profiles.mddocs/plans/2026-04-21-ticket-quality-gate.mddocs/plans/INDEX.mddocs/plans/done/initial_v1.mddocs/plans/worker-isolation.mddocs/review-prompts/2026-04-25-worker-isolation-distributed-workflow-crash-recovery-reviewer.mddocs/review-prompts/2026-04-25-worker-isolation-git-plumbing-repository-safety-reviewer.mdgo.modinternal/adapters/repo/git/repo.gointernal/adapters/repo/git/repo_test.gointernal/adapters/runtime/cacheenv.gointernal/adapters/runtime/cacheenv_test.gointernal/adapters/runtime/claude/adapter.gointernal/adapters/runtime/claude/adapter_test.gointernal/adapters/runtime/claude/adapter_unix_test.gointernal/adapters/runtime/codex/adapter.gointernal/adapters/runtime/codex/adapter_test.gointernal/adapters/runtime/prompt.gointernal/adapters/runtime/prompt_test.gointernal/adapters/runtime/types.gointernal/adapters/ticketstore/epos/claims.gointernal/adapters/ticketstore/epos/claims_test.gointernal/adapters/ticketstore/epos/containment.gointernal/adapters/ticketstore/epos/convert.gointernal/adapters/ticketstore/epos/doc.gointernal/adapters/ticketstore/epos/live.gointernal/adapters/ticketstore/epos/store.gointernal/adapters/ticketstore/epos/store_test.gointernal/adapters/ticketstore/epos/types.gointernal/adapters/ticketstore/epos/types_test.gointernal/adapters/ticketstore/tkmd/claims.gointernal/adapters/ticketstore/tkmd/claims_lock_unix.gointernal/adapters/ticketstore/tkmd/claims_lock_windows.gointernal/adapters/ticketstore/tkmd/claims_test.gointernal/adapters/ticketstore/tkmd/store.gointernal/adapters/ticketstore/tkmd/store_test.gointernal/adapters/verify/command/runner.gointernal/adapters/verify/command/runner_test.gointernal/cli/run.gointernal/cli/run_blocked_test.gointernal/cli/run_current_pointer_test.gointernal/cli/run_persistence_test.gointernal/e2e/epic_multi_wave_test.gointernal/e2e/helpers_test.gointernal/e2e/resume_claim_recovery_test.gointernal/engine/closeout.gointernal/engine/closeout_test.gointernal/engine/epic_gate.gointernal/engine/epic_gate_test.gointernal/engine/epic_run.gointernal/engine/epic_run_test.gointernal/engine/intake.gointernal/engine/intake_test.gointernal/engine/ops_support.gointernal/engine/reopen_test.gointernal/engine/resume.gointernal/engine/resume_test.gointernal/engine/status.gointernal/engine/status_test.gointernal/engine/ticket_run.gointernal/engine/ticket_run_test.gointernal/engine/ticket_validation.gointernal/engine/ticket_validation_test.gointernal/engine/wave_commands_test.gointernal/engine/wave_scheduler.gointernal/engine/wave_verify.gointernal/engine/wave_verify_test.gointernal/engine/worktree.gointernal/engine/worktree_test.gointernal/state/types.gojustfilepkg/verk/types.goscripts/pre-commitscripts/pre-push
💤 Files with no reviewable changes (7)
- internal/adapters/ticketstore/tkmd/claims_lock_unix.go
- internal/adapters/ticketstore/tkmd/store.go
- internal/adapters/ticketstore/tkmd/claims_lock_windows.go
- internal/adapters/ticketstore/tkmd/claims.go
- internal/adapters/ticketstore/tkmd/claims_test.go
- .verk/config.yaml
- internal/adapters/ticketstore/tkmd/store_test.go
php-workx
force-pushed
the
feat/epso-integration
branch
from
f48d0c9 to
06da653
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/engine/worktree_test.go (1)
1269-1281:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a consistent “current” timestamp for claim seeding in both tests.
2026-04-02 12:00:00 UTCis already in the past as of May 11, 2026, so the stale test can accidentally seed an expired lease and pass for the wrong reason. The active test also uses a different semantic (now + 24h). Usingtime.Now().UTC()in both places keeps intent clear.Suggested patch
- now := time.Date(2026, 4, 2, 12, 0, 0, 0, time.UTC) + now := time.Now().UTC() ... - now := time.Now().UTC().Add(24 * time.Hour) + now := time.Now().UTC()Also applies to: 1364-1376
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/worktree_test.go` around lines 1269 - 1281, Tests seed claims with inconsistent timestamps (a hardcoded past `now` and a different `now+24h`), which can make the stale test pass for the wrong reason; replace the hardcoded time.Date and the other ad-hoc timestamp with a single consistent current time variable (e.g., currentTime := time.Now().UTC()) and use that same variable when calling epos.AcquireClaim for both the stale and active tests (use currentTime for the stale lease and currentTime.Add(24*time.Hour) for the active lease) so both tests use the same "now" reference.
♻️ Duplicate comments (1)
internal/cli/run_current_pointer_test.go (1)
38-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
testGitEnvagainst host git config leakage.Line 52+ still allows global/system git config to influence
gitcommands, which keeps this test host-dependent/flaky. This is the same unresolved concern previously raised.Suggested patch
func testGitEnv() []string { env := os.Environ() - out := make([]string, 0, len(env)+4) + out := make([]string, 0, len(env)+6) @@ out = append(out, "GIT_OPTIONAL_LOCKS=0", + "GIT_CONFIG_GLOBAL="+os.DevNull, + "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=core.hooksPath", - "GIT_CONFIG_VALUE_0=/dev/null", + "GIT_CONFIG_VALUE_0="+os.DevNull, ) return out } @@ case "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_COMMON_DIR", "GIT_CONFIG", "GIT_CONFIG_COUNT", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_NOSYSTEM", "GIT_CONFIG_PARAMETERS", "GIT_DIR", @@ "GIT_SUPER_PREFIX", + "GIT_OPTIONAL_LOCKS", "GIT_WORK_TREE": return true#!/bin/bash # Verify git test env hardening flags are present in test helpers. rg -nC3 --type=go 'func testGitEnv\(|GIT_CONFIG_GLOBAL|GIT_CONFIG_NOSYSTEM|GIT_OPTIONAL_LOCKS|GIT_CONFIG_VALUE_0=.*(os\.DevNull|/dev/null)'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/run_current_pointer_test.go` around lines 38 - 86, testGitEnv still allows host global/system git config to leak; update the helper so it neutralizes those by (1) adding "GIT_CONFIG_NOSYSTEM=1" and "GIT_CONFIG_GLOBAL=/dev/null" to the appended env in testGitEnv and (2) adding "GIT_CONFIG_GLOBAL" and "GIT_CONFIG_NOSYSTEM" to the isGitLocalEnv switch so any existing host values are filtered out from os.Environ before we set the hardened values; use the existing function names testGitEnv and isGitLocalEnv to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/engine/worktree_test.go`:
- Around line 1269-1281: Tests seed claims with inconsistent timestamps (a
hardcoded past `now` and a different `now+24h`), which can make the stale test
pass for the wrong reason; replace the hardcoded time.Date and the other ad-hoc
timestamp with a single consistent current time variable (e.g., currentTime :=
time.Now().UTC()) and use that same variable when calling epos.AcquireClaim for
both the stale and active tests (use currentTime for the stale lease and
currentTime.Add(24*time.Hour) for the active lease) so both tests use the same
"now" reference.
---
Duplicate comments:
In `@internal/cli/run_current_pointer_test.go`:
- Around line 38-86: testGitEnv still allows host global/system git config to
leak; update the helper so it neutralizes those by (1) adding
"GIT_CONFIG_NOSYSTEM=1" and "GIT_CONFIG_GLOBAL=/dev/null" to the appended env in
testGitEnv and (2) adding "GIT_CONFIG_GLOBAL" and "GIT_CONFIG_NOSYSTEM" to the
isGitLocalEnv switch so any existing host values are filtered out from
os.Environ before we set the hardened values; use the existing function names
testGitEnv and isGitLocalEnv to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a7c84c9-1485-419f-b1f6-002ed4db8199
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (54)
cmd/verk/main_test.godoc.godocs/epos-integration.mddocs/plans/2026-04-19-verk-as-skill-cross-agent.mddocs/plans/2026-04-21-agent-profiles.mddocs/plans/2026-04-21-ticket-quality-gate.mddocs/plans/done/initial_v1.mdgo.modinternal/adapters/ticketstore/epos/claims.gointernal/adapters/ticketstore/epos/claims_test.gointernal/adapters/ticketstore/epos/containment.gointernal/adapters/ticketstore/epos/convert.gointernal/adapters/ticketstore/epos/doc.gointernal/adapters/ticketstore/epos/live.gointernal/adapters/ticketstore/epos/store.gointernal/adapters/ticketstore/epos/store_test.gointernal/adapters/ticketstore/epos/types.gointernal/adapters/ticketstore/epos/types_test.gointernal/adapters/ticketstore/tkmd/claims.gointernal/adapters/ticketstore/tkmd/claims_lock_unix.gointernal/adapters/ticketstore/tkmd/claims_lock_windows.gointernal/adapters/ticketstore/tkmd/claims_test.gointernal/adapters/ticketstore/tkmd/store.gointernal/adapters/ticketstore/tkmd/store_test.gointernal/cli/run.gointernal/cli/run_blocked_test.gointernal/cli/run_current_pointer_test.gointernal/cli/run_persistence_test.gointernal/e2e/epic_multi_wave_test.gointernal/e2e/helpers_test.gointernal/e2e/resume_claim_recovery_test.gointernal/engine/closeout.gointernal/engine/closeout_test.gointernal/engine/epic_gate.gointernal/engine/epic_gate_test.gointernal/engine/epic_run.gointernal/engine/epic_run_test.gointernal/engine/intake.gointernal/engine/intake_test.gointernal/engine/ops_support.gointernal/engine/reopen_test.gointernal/engine/resume.gointernal/engine/resume_test.gointernal/engine/status.gointernal/engine/status_test.gointernal/engine/ticket_run.gointernal/engine/ticket_run_test.gointernal/engine/ticket_validation_test.gointernal/engine/wave_scheduler.gointernal/engine/worktree_test.gojustfilepkg/verk/types.goscripts/pre-commitscripts/pre-push
💤 Files with no reviewable changes (6)
- internal/adapters/ticketstore/tkmd/claims_lock_windows.go
- internal/adapters/ticketstore/tkmd/store.go
- internal/adapters/ticketstore/tkmd/claims_lock_unix.go
- internal/adapters/ticketstore/tkmd/claims_test.go
- internal/adapters/ticketstore/tkmd/claims.go
- internal/adapters/ticketstore/tkmd/store_test.go
✅ Files skipped from review due to trivial changes (9)
- docs/plans/2026-04-19-verk-as-skill-cross-agent.md
- internal/adapters/ticketstore/epos/types.go
- doc.go
- docs/plans/done/initial_v1.md
- internal/adapters/ticketstore/epos/doc.go
- internal/engine/intake.go
- docs/plans/2026-04-21-agent-profiles.md
- go.mod
- docs/plans/2026-04-21-ticket-quality-gate.md
🚧 Files skipped from review as they are similar to previous changes (27)
- internal/engine/ops_support.go
- internal/e2e/helpers_test.go
- internal/engine/status.go
- internal/engine/ticket_validation_test.go
- internal/engine/epic_gate_test.go
- internal/engine/reopen_test.go
- internal/adapters/ticketstore/epos/containment.go
- internal/cli/run_blocked_test.go
- internal/adapters/ticketstore/epos/live.go
- internal/engine/intake_test.go
- internal/e2e/epic_multi_wave_test.go
- internal/adapters/ticketstore/epos/store.go
- cmd/verk/main_test.go
- internal/engine/closeout_test.go
- internal/cli/run.go
- internal/cli/run_persistence_test.go
- internal/e2e/resume_claim_recovery_test.go
- internal/adapters/ticketstore/epos/convert.go
- internal/adapters/ticketstore/epos/types_test.go
- internal/adapters/ticketstore/epos/claims.go
- internal/adapters/ticketstore/epos/store_test.go
- internal/engine/epic_gate.go
- internal/engine/ticket_run_test.go
- internal/engine/ticket_run.go
- internal/engine/epic_run.go
- internal/engine/resume.go
- internal/engine/epic_run_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/e2e/helpers_test.go`:
- Around line 68-80: The environment sanitizer in testGitEnv() is missing
GIT_TEMPLATE_DIR, allowing host templates to leak into temp repos; add
"GIT_TEMPLATE_DIR" to the switch-case list of keys that are skipped (the same
list that currently contains "GIT_DIR", "GIT_WORK_TREE", ...
"GIT_CONFIG_PARAMETERS") so it is filtered out before building the returned env
and the hardcoded config/hook entries are appended.
In `@internal/engine/resume.go`:
- Around line 217-223: The call to epos.ReleaseClaim in resume.go should not
ignore unexpected errors: check the returned error from
epos.ReleaseClaim(artifacts.RepoRoot, req.RunID, ticketID,
"resume_reacquisition") and if it is non-nil and not the known sentinel for
“missing claim” (e.g., ErrClaimNotFound or equivalent from the epos package),
return or propagate that error instead of proceeding to AcquireClaim; apply the
same pattern to the other silent drops of ReleaseClaim found around the other
resume code paths (the occurrences mentioned at the later locations) so only the
allowed-not-found sentinel is ignored and all other errors fail fast.
In `@internal/engine/wave_verify.go`:
- Around line 1185-1186: The current shortcut uses
pathSetIncludes(finalChangedFiles, tx.ChangedFiles) even when tx.ChangedFiles is
nil/empty, which vacuously succeeds; change the condition to require
tx.ChangedFiles to be non-empty (e.g., len(tx.ChangedFiles) > 0) AND
pathSetIncludes(...) before treating the wave as already applied so
empty/missing changed_files fall through to the normal completion path; make the
same guard update for the analogous checks in the same block (the other
pathSetIncludes usages around the 1191-1201 range) so all early-return shortcuts
require a non-empty tx.ChangedFiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e069c515-05a9-4068-83c2-62f29237965b
📒 Files selected for processing (23)
cmd/verk/main_test.godocs/epos-integration.mddocs/plans/2026-04-21-ticket-quality-gate.mddocs/plans/worker-isolation.mdinternal/adapters/repo/git/repo.gointernal/adapters/repo/git/repo_test.gointernal/adapters/ticketstore/epos/claims.gointernal/adapters/ticketstore/epos/claims_test.gointernal/adapters/ticketstore/epos/convert.gointernal/adapters/ticketstore/epos/live.gointernal/adapters/verify/command/runner.gointernal/adapters/verify/command/runner_test.gointernal/cli/run_current_pointer_test.gointernal/e2e/helpers_test.gointernal/engine/closeout.gointernal/engine/epic_gate.gointernal/engine/resume.gointernal/engine/ticket_run_test.gointernal/engine/wave_commands_test.gointernal/engine/wave_verify.gointernal/engine/wave_verify_test.gointernal/engine/worktree.gointernal/engine/worktree_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/adapters/verify/command/runner_test.go
- internal/adapters/verify/command/runner.go
- docs/plans/2026-04-21-ticket-quality-gate.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/verk/main_test.go
- internal/engine/ticket_run_test.go
- internal/adapters/ticketstore/epos/claims_test.go
- internal/adapters/ticketstore/epos/convert.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/engine/wave_verify_test.go (1)
87-97: ⚡ Quick winAdd explicit empty-slice coverage for transaction files.
You validate
nil, but not[]string{}. Adding that assertion locks in the intended “nil/empty both reject” contract.Proposed test addition
func TestPathSetIncludesAllowsAdditionalFinalFiles(t *testing.T) { if !pathSetIncludes([]string{"docs/a.md", "docs/b.md"}, []string{"docs/a.md"}) { t.Fatal("expected superset final file list to include original transaction files") } if pathSetIncludes([]string{"docs/b.md"}, []string{"docs/a.md"}) { t.Fatal("expected missing original transaction file to be rejected") } if pathSetIncludes([]string{"docs/a.md"}, nil) { t.Fatal("expected empty transaction file list to be rejected") } + if pathSetIncludes([]string{"docs/a.md"}, []string{}) { + t.Fatal("expected explicit empty transaction file list to be rejected") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/wave_verify_test.go` around lines 87 - 97, Update the TestPathSetIncludesAllowsAdditionalFinalFiles test to assert that an explicitly empty slice is rejected the same as nil: inside TestPathSetIncludesAllowsAdditionalFinalFiles, add a check calling pathSetIncludes([]string{"docs/a.md"}, []string{}) and fail the test (t.Fatal) if it returns true so the contract that nil/empty transaction file lists are rejected is enforced; reference the existing test name TestPathSetIncludesAllowsAdditionalFinalFiles and the helper pathSetIncludes to locate where to add the assertion.internal/engine/resume_test.go (1)
1052-1053: ⚡ Quick winRename
closedtest fixture variable to reflect initial blocked state.At Line 1052,
closedis initialized asepos.StatusBlocked. Renaming it (e.g.,willClose/eventuallyClosed) would reduce cognitive friction when reading this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/resume_test.go` around lines 1052 - 1053, Rename the test fixture variable currently named closed (created via epicChildTicket(..., epos.StatusBlocked, ...)) to a name that reflects its initial blocked state such as eventuallyClosed (or willClose); update the declaration that calls epicChildTicket and every subsequent reference to that variable in this test (assertions, uses in setup/teardown, and any logs) so the code reads clearly that the ticket starts blocked (epos.StatusBlocked) and later transitions as the test expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/engine/resume_test.go`:
- Around line 1052-1053: Rename the test fixture variable currently named closed
(created via epicChildTicket(..., epos.StatusBlocked, ...)) to a name that
reflects its initial blocked state such as eventuallyClosed (or willClose);
update the declaration that calls epicChildTicket and every subsequent reference
to that variable in this test (assertions, uses in setup/teardown, and any logs)
so the code reads clearly that the ticket starts blocked (epos.StatusBlocked)
and later transitions as the test expects.
In `@internal/engine/wave_verify_test.go`:
- Around line 87-97: Update the TestPathSetIncludesAllowsAdditionalFinalFiles
test to assert that an explicitly empty slice is rejected the same as nil:
inside TestPathSetIncludesAllowsAdditionalFinalFiles, add a check calling
pathSetIncludes([]string{"docs/a.md"}, []string{}) and fail the test (t.Fatal)
if it returns true so the contract that nil/empty transaction file lists are
rejected is enforced; reference the existing test name
TestPathSetIncludesAllowsAdditionalFinalFiles and the helper pathSetIncludes to
locate where to add the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7f27391-06e2-4849-b5a2-20b6a73affc4
📒 Files selected for processing (7)
internal/adapters/ticketstore/epos/claims.gointernal/adapters/ticketstore/epos/claims_test.gointernal/e2e/helpers_test.gointernal/engine/resume.gointernal/engine/resume_test.gointernal/engine/wave_verify.gointernal/engine/wave_verify_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/engine/wave_verify.go
- internal/engine/resume.go
- internal/adapters/ticketstore/epos/claims.go
- internal/e2e/helpers_test.go
- internal/adapters/ticketstore/epos/claims_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/engine/epic_run.go (1)
1632-1651:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClaim-held children will now be reported as
status=open.After the switch to claim-aware
epos.ListReadyChildren, an open child can be filtered out solely because another run holds an active claim. This helper still only looks at markdown status/deps, so the blocked summary andBlockedRunErrorcan hide the real wait-on-lease condition and tell the operator the wrong thing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/epic_run.go` around lines 1632 - 1651, describeNotReady currently treats open-but-claim-held children as plain "status=open"; update describeNotReady to detect claim-held tickets (check the ticket's claim fields such as Claim, ClaimHolder, ClaimRunID or similar on epos.Ticket) and return a distinct message like "held by claim" or "waiting on lease (held by <id>)" when a claim exists, falling back to the existing deps/status logic otherwise; also ensure the BlockedRunError path (and any callers relying on describeNotReady) will surface this new claim-aware string so operators see a lease/claim wait instead of "open".
🧹 Nitpick comments (2)
internal/engine/runlock.go (1)
44-46: 💤 Low valueConsider whether the unexported wrapper is needed.
The
validateArtifactIdentifierwrapper simply forwards toValidateArtifactIdentifier. If both functions have identical behavior, consider usingValidateArtifactIdentifierdirectly at call sites unless there's a planned divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/runlock.go` around lines 44 - 46, The private wrapper validateArtifactIdentifier just forwards to the exported ValidateArtifactIdentifier and offers no added behavior; either remove validateArtifactIdentifier and replace all its call sites to call ValidateArtifactIdentifier directly, or if you intend future divergence keep the wrapper but add a comment explaining its purpose. Locate usages of validateArtifactIdentifier and update them to call ValidateArtifactIdentifier (or preserve the wrapper and document it) and run tests/build to ensure no references remain.internal/engine/ops_support.go (1)
478-484: ⚡ Quick winMake this helper write
ready, or rename it.
setTicketReadynow persistsepos.StatusOpen, which makes the helper name misleading and drops the explicit ready state introduced elsewhere in this PR.Suggested change
func setTicketReady(repoRoot, ticketID string) error { ticket, err := epos.LoadTicket(ticketMarkdownPath(repoRoot, ticketID)) if err != nil { return err } - ticket.Status = epos.StatusOpen + ticket.Status = epos.StatusReady return epos.SaveTicket(ticketMarkdownPath(repoRoot, ticketID), ticket) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/engine/ops_support.go` around lines 478 - 484, The helper setTicketReady currently loads a ticket and sets ticket.Status = epos.StatusOpen which contradicts its name and the new explicit "ready" state; change the implementation to persist epos.StatusReady instead of epos.StatusOpen (i.e., set ticket.Status = epos.StatusReady) and keep the rest (LoadTicket, SaveTicket, ticketMarkdownPath) unchanged, or if the intended semantics were to mark tickets open, rename the function to setTicketOpen and leave the status as epos.StatusOpen; also update any callers to use the new status value or new function name accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/engine/epic_run.go`:
- Around line 1632-1651: describeNotReady currently treats open-but-claim-held
children as plain "status=open"; update describeNotReady to detect claim-held
tickets (check the ticket's claim fields such as Claim, ClaimHolder, ClaimRunID
or similar on epos.Ticket) and return a distinct message like "held by claim" or
"waiting on lease (held by <id>)" when a claim exists, falling back to the
existing deps/status logic otherwise; also ensure the BlockedRunError path (and
any callers relying on describeNotReady) will surface this new claim-aware
string so operators see a lease/claim wait instead of "open".
---
Nitpick comments:
In `@internal/engine/ops_support.go`:
- Around line 478-484: The helper setTicketReady currently loads a ticket and
sets ticket.Status = epos.StatusOpen which contradicts its name and the new
explicit "ready" state; change the implementation to persist epos.StatusReady
instead of epos.StatusOpen (i.e., set ticket.Status = epos.StatusReady) and keep
the rest (LoadTicket, SaveTicket, ticketMarkdownPath) unchanged, or if the
intended semantics were to mark tickets open, rename the function to
setTicketOpen and leave the status as epos.StatusOpen; also update any callers
to use the new status value or new function name accordingly.
In `@internal/engine/runlock.go`:
- Around line 44-46: The private wrapper validateArtifactIdentifier just
forwards to the exported ValidateArtifactIdentifier and offers no added
behavior; either remove validateArtifactIdentifier and replace all its call
sites to call ValidateArtifactIdentifier directly, or if you intend future
divergence keep the wrapper but add a comment explaining its purpose. Locate
usages of validateArtifactIdentifier and update them to call
ValidateArtifactIdentifier (or preserve the wrapper and document it) and run
tests/build to ensure no references remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84a3f4e1-ba87-42d0-9c5e-be4b9e268ead
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
cmd/verk/main_test.godocs/plans/done/epos-integration.mdgo.modinternal/adapters/ticketstore/epos/convert.gointernal/adapters/ticketstore/epos/store_test.gointernal/cli/run.gointernal/cli/run_current_pointer_test.gointernal/engine/epic_run.gointernal/engine/epic_run_test.gointernal/engine/ops_support.gointernal/engine/reopen.gointernal/engine/reopen_test.gointernal/engine/resume.gointernal/engine/runlock.gointernal/engine/runlock_test.gointernal/engine/runlock_unix.gointernal/engine/runlock_windows.gointernal/engine/status_test.gointernal/engine/ticket_run.gointernal/engine/ticket_run_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- internal/engine/reopen_test.go
- internal/adapters/ticketstore/epos/convert.go
- internal/engine/ticket_run_test.go
- internal/engine/epic_run_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores