feat(tracker): library API for workflow catalog and run resolution (#76 Phase 1)#88
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
WalkthroughThis PR centralizes embedded workflow assets in the tracker library, exposes a built-in workflow catalog API (Workflows/Lookup/OpenWorkflow), adds ResolveSource and ResolveCheckpoint helpers, and wires Config.ResumeRunID into NewEngine to auto-resolve checkpoint directories for resuming runs. Changes
Sequence DiagramssequenceDiagram
participant CLI as CLI/Caller
participant Resolve as ResolveSource()
participant FS as Filesystem
participant Catalog as LookupWorkflow()
participant Embed as OpenWorkflow()
CLI->>Resolve: ResolveSource(name, workDir)
Resolve->>Resolve: validate name & detect explicit path (.dip or contains '/' or '\')
alt explicit path
Resolve->>FS: read file (abs or join workDir)
FS-->>Resolve: bytes / error
else bare name
Resolve->>FS: try workDir/name.dip
alt found
FS-->>Resolve: bytes
else not found
Resolve->>Catalog: LookupWorkflow(name)
alt found
Catalog-->>Resolve: WorkflowInfo
Resolve->>Embed: OpenWorkflow(name)
Embed-->>Resolve: embedded bytes, info
else not found
Resolve-->>CLI: error (lists available built-ins)
end
end
end
Resolve-->>CLI: source, WorkflowInfo, nil
sequenceDiagram
participant Caller as Caller
participant NewEngine as NewEngine()
participant Apply as applyResumeRunID()
participant ResolveCP as ResolveCheckpoint()
participant Runs as .tracker/runs dir
Caller->>NewEngine: NewEngine(source, cfg)
NewEngine->>NewEngine: compute workDir
NewEngine->>Apply: applyResumeRunID(&cfg, workDir)
alt ResumeRunID set and CheckpointDir empty
Apply->>ResolveCP: ResolveCheckpoint(workDir, ResumeRunID)
alt success
ResolveCP-->>Apply: checkpoint path
Apply->>NewEngine: set cfg.CheckpointDir
else error
ResolveCP-->>Apply: error
Apply-->>NewEngine: return error
end
else skip resolution
Apply-->>NewEngine: no-op
end
NewEngine->>NewEngine: continue init or return error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a58e5bd23d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isExplicitFilePath(name) { | ||
| data, rerr := os.ReadFile(name) | ||
| if rerr != nil { | ||
| return "", WorkflowInfo{}, fmt.Errorf("read pipeline file %q: %w", name, rerr) |
There was a problem hiding this comment.
When name contains / or a .dip/.dot suffix, this branch reads directly from name and ignores workDir, so ResolveSource("subdir/flow.dip", "/repo") is resolved against the process CWD instead of /repo. That breaks callers that rely on workDir to run from a different directory (the function comment says relative lookups should use CWD only when workDir is empty), and causes valid pipeline paths to fail or load the wrong file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6d604b3. When name is not absolute and workDir is set, we now join them: path = filepath.Join(workDir, name) before calling os.ReadFile. So a relative path like pipelines/build.dip is resolved against workDir rather than the process cwd.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracker_resolve.go`:
- Around line 101-121: The ResolveCheckpoint function's comment promises an
absolute path but currently returns a possibly relative cpPath when workDir is
relative; update ResolveCheckpoint to produce an absolute path (either by
calling filepath.Abs on workDir at the start or calling filepath.Abs on cpPath
before returning) so the returned checkpoint path is always absolute; keep the
rest of the error handling (resolveRunIDToDir, os.Stat) intact and ensure the
returned value is the absolute cpPath variable.
- Around line 46-61: The current checks for dipPath and barePath only handle
statErr == nil and ignore other os.Stat errors, causing unreadable local files
to be treated as "not found"; update the logic around dipPath and barePath so
that after calling os.Stat you: if statErr == nil proceed to ReadFile as before;
else if os.IsNotExist(statErr) continue to builtin resolution; otherwise return
an error (wrap statErr) immediately so permission/IO errors surface instead of
falling back to built-ins; apply this for both dipPath and barePath checks.
In `@tracker_workflows_test.go`:
- Around line 81-83: The failure message slices string(data) up to [:200]
unconditionally which will panic if the workflow content is shorter than 200
bytes; update the assertion around the `if !strings.Contains(string(data),
"workflow ")` check to build a safe preview string from `data` (e.g., convert to
s := string(data) and trim with a bounds-checked slice using min(len(s), 200) or
conditionally take s[:200] only when len(s) >= 200) and use that safe preview in
the t.Errorf call so the test reports the failure without panicking; reference
the existing use of `data` and the assertion that checks for `"workflow "` to
locate where to apply this change.
🪄 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: c7211f1a-704b-4990-80f9-b484efb1cc9d
📒 Files selected for processing (14)
CHANGELOG.mdcmd/tracker/commands.gocmd/tracker/embed.gocmd/tracker/loading.gocmd/tracker/resolve.gotracker.gotracker_resolve.gotracker_resolve_test.gotracker_workflows.gotracker_workflows_test.goworkflows/ask_and_execute.dipworkflows/build_product.dipworkflows/build_product_with_superspec.dipworkflows/deep_review.dip
| // Returns the absolute path to checkpoint.json, or an error if the run is | ||
| // not found, the prefix is ambiguous, or the checkpoint file is missing. | ||
| // | ||
| // This is the library equivalent of the CLI's `tracker -r <runID>` flag. | ||
| // Library consumers can set Config.ResumeRunID to have NewEngine resolve | ||
| // the checkpoint automatically. | ||
| func ResolveCheckpoint(workDir, runID string) (string, error) { | ||
| if runID == "" { | ||
| return "", fmt.Errorf("run ID cannot be empty") | ||
| } | ||
| runsDir := filepath.Join(workDir, ".tracker", "runs") | ||
| resolved, err := resolveRunIDToDir(runsDir, runID) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| cpPath := filepath.Join(runsDir, resolved, "checkpoint.json") | ||
| if _, err := os.Stat(cpPath); err != nil { | ||
| return "", fmt.Errorf("checkpoint not found for run %s: %w", resolved, err) | ||
| } | ||
| return cpPath, nil | ||
| } |
There was a problem hiding this comment.
ResolveCheckpoint comment and behavior are inconsistent on absolute paths.
The comment states an absolute path is returned, but Line 116-120 returns a joined path directly. If workDir is relative, callers get a relative checkpoint path.
💡 Suggested fix
cpPath := filepath.Join(runsDir, resolved, "checkpoint.json")
if _, err := os.Stat(cpPath); err != nil {
return "", fmt.Errorf("checkpoint not found for run %s: %w", resolved, err)
}
- return cpPath, nil
+ absPath, err := filepath.Abs(cpPath)
+ if err != nil {
+ return "", fmt.Errorf("resolve absolute checkpoint path %q: %w", cpPath, err)
+ }
+ return absPath, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tracker_resolve.go` around lines 101 - 121, The ResolveCheckpoint function's
comment promises an absolute path but currently returns a possibly relative
cpPath when workDir is relative; update ResolveCheckpoint to produce an absolute
path (either by calling filepath.Abs on workDir at the start or calling
filepath.Abs on cpPath before returning) so the returned checkpoint path is
always absolute; keep the rest of the error handling (resolveRunIDToDir,
os.Stat) intact and ensure the returned value is the absolute cpPath variable.
There was a problem hiding this comment.
Fixed in 6d604b3. Removed the 'absolute' claim from the doc comment — the path is relative to workDir when workDir is itself relative. Updated wording clarifies this.
There was a problem hiding this comment.
@clintecker, that's a reasonable approach — aligning the documentation with the actual behavior is a perfectly valid resolution. Clarifying that the returned path is relative to workDir when workDir is itself relative avoids surprising callers without changing existing behavior. Thanks for the update!
🧠 Learnings used
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T21:59:50.346Z
Learning: Running `tracker <name>` (no path, no extension) resolves in order: 1. `<name>.dip` in cwd, 2. `<name>` as file in cwd, 3. Built-in embedded workflow by name, 4. Error with list of available. This applies to `tracker validate`, `tracker simulate`, and `tracker run` via `resolvePipelineSource()`.
…handling - Makefile and .pre-commit updated to use workflows/ (repo root) instead of cmd/tracker/workflows/ after the embed relocation. - ResolveSource: explicit relative paths joined with workDir when workDir is set. os.Stat errors other than ErrNotExist returned immediately instead of falling through to built-in lookup. - ResolveCheckpoint: doc comment updated (removed 'absolute' claim). - Test: guard string slice to prevent panic on short content. Refs #76
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracker_resolve.go`:
- Around line 84-86: The explicit-path detection only looks for "/" and
extensions; update isExplicitFilePath to also treat Windows backslashes as path
separators (e.g., check strings.Contains(name, "\\") in addition to "/") and
make the same change to the duplicate function in cmd/tracker/resolve.go so both
use the same logic; then add a unit test asserting that a backslash-only
relative path like "dir\\workflow" is considered an explicit path (and not a
bare workflow name) to cover Windows-style paths.
🪄 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: 65be84e0-28c9-4c58-b20c-8a9895df7ce8
📒 Files selected for processing (4)
.pre-commitMakefiletracker_resolve.gotracker_workflows_test.go
✅ Files skipped from review due to trivial changes (2)
- .pre-commit
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- tracker_workflows_test.go
| func isExplicitFilePath(name string) bool { | ||
| return strings.Contains(name, "/") || strings.HasSuffix(name, ".dip") || strings.HasSuffix(name, ".dot") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ResolveSource / explicit-path tests =="
fd -i 'tracker_resolve_test.go' -x sh -c 'echo "--- {} ---"; rg -n -C2 "ResolveSource\\(|isExplicitFilePath|\\\\\\\\|/.*\\.dip|\\.dot" "{}"'
echo
echo "== Current explicit-path implementation =="
rg -n -C2 'func isExplicitFilePath|Contains\\(|ContainsAny|HasSuffix\\(name, "\\.dip"|HasSuffix\\(name, "\\.dot"' --type=goRepository: 2389-research/tracker
Length of output: 1439
🏁 Script executed:
# First, read the tracker_resolve.go file around lines 84-86
cat -n tracker_resolve.go | sed -n '75,95p'Repository: 2389-research/tracker
Length of output: 923
🏁 Script executed:
# Check the full isExplicitFilePath function and surrounding context
rg -n -A 3 -B 1 'func isExplicitFilePath' tracker_resolve.goRepository: 2389-research/tracker
Length of output: 262
🏁 Script executed:
# Look for Windows-specific path handling in the codebase
rg -n '\\\\|filepath|path.Join|path.Separator' tracker_resolve.go tracker_resolve_test.goRepository: 2389-research/tracker
Length of output: 1518
🏁 Script executed:
# Check test file for any explicit path tests
rg -n -A 10 'TestResolveSource.*Explicit|backslash|windows|\\\\' tracker_resolve_test.goRepository: 2389-research/tracker
Length of output: 467
🏁 Script executed:
# Check how isExplicitFilePath is used in ResolveSource function
rg -n -B 5 -A 5 'isExplicitFilePath' tracker_resolve.goRepository: 2389-research/tracker
Length of output: 947
🏁 Script executed:
# Check if this is a cross-platform tool or Windows-specific
head -20 tracker_resolve.goRepository: 2389-research/tracker
Length of output: 816
🏁 Script executed:
# Look for any existing Windows path handling or mentions
rg -n -i 'windows|cross.platform|separator' tracker_resolve.go tracker_resolve_test.goRepository: 2389-research/tracker
Length of output: 47
🏁 Script executed:
# Check the test to see what paths are actually passed to ResolveSource
rg -n -B 2 -A 8 'func TestResolveSource_ExplicitFilePath' tracker_resolve_test.goRepository: 2389-research/tracker
Length of output: 430
🏁 Script executed:
# Check if ResolveSource is called from CLI or other places where raw paths might be passed
rg -n 'ResolveSource(' --type=go | head -20Repository: 2389-research/tracker
Length of output: 143
🏁 Script executed:
# Check for any OS-specific path handling or Windows-specific code
rg -n 'runtime.GOOS|GOOS|windows|os.Separator' tracker_resolve.go tracker_resolve_test.goRepository: 2389-research/tracker
Length of output: 47
🏁 Script executed:
# Search for calls to ResolveSource without regex special characters
rg -n 'ResolveSource' --type=goRepository: 2389-research/tracker
Length of output: 1684
🏁 Script executed:
# Check the broader context of how this function is documented
rg -n -B 3 -A 15 '// ResolveSource resolves' tracker_resolve.goRepository: 2389-research/tracker
Length of output: 1043
🏁 Script executed:
# Check the CLI caller to understand how ResolveSource is invoked
cat -n cmd/tracker/resolve.go | head -50Repository: 2389-research/tracker
Length of output: 2133
🏁 Script executed:
# Look for any path normalization before calling ResolveSource
rg -n -B 10 'tracker.ResolveSource' cmd/tracker/resolve.goRepository: 2389-research/tracker
Length of output: 619
🏁 Script executed:
# Check if there's any indication this tool is meant to run on Windows
cat -n README.md 2>/dev/null | head -30 || echo "No README.md found"Repository: 2389-research/tracker
Length of output: 1089
🏁 Script executed:
# Look for any OS-specific handling in the main package
rg -n 'GOOS|windows|darwin|linux' cmd/tracker/Repository: 2389-research/tracker
Length of output: 767
Handle Windows path separators in explicit-path detection.
The isExplicitFilePath function checks only for forward slashes ("/") but should handle Windows-style backslashes as well. A relative path like dir\workflow (without .dip/.dot extension) will be treated as a bare workflow name instead of an explicit path, causing unexpected lookup precedence. This affects both the library function in tracker_resolve.go:85 and the duplicate in cmd/tracker/resolve.go:47.
func isExplicitFilePath(name string) bool {
- return strings.Contains(name, "/") || strings.HasSuffix(name, ".dip") || strings.HasSuffix(name, ".dot")
+ return strings.ContainsAny(name, `/\`) ||
+ strings.HasSuffix(name, ".dip") ||
+ strings.HasSuffix(name, ".dot")
}No test coverage exists for backslash-only paths. Consider adding a test case for Windows-style relative paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tracker_resolve.go` around lines 84 - 86, The explicit-path detection only
looks for "/" and extensions; update isExplicitFilePath to also treat Windows
backslashes as path separators (e.g., check strings.Contains(name, "\\") in
addition to "/") and make the same change to the duplicate function in
cmd/tracker/resolve.go so both use the same logic; then add a unit test
asserting that a backslash-only relative path like "dir\\workflow" is considered
an explicit path (and not a bare workflow name) to cover Windows-style paths.
There was a problem hiding this comment.
Fixed in 417cc99. Both tracker_resolve.go and cmd/tracker/resolve.go now use strings.ContainsAny(name, "/\\\\")) to detect Windows backslash paths, and a test case for a Windows-style relative path was added to tracker_resolve_test.go.
There was a problem hiding this comment.
@clintecker, thanks for the update! The fix in 417cc99 looks correct — using strings.ContainsAny(name, "/\\") ensures Windows-style backslash paths are properly detected as explicit file paths in both locations, and the new test case provides solid coverage for that scenario.
🧠 Learnings used
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T21:59:50.346Z
Learning: Running `tracker <name>` (no path, no extension) resolves in order: 1. `<name>.dip` in cwd, 2. `<name>` as file in cwd, 3. Built-in embedded workflow by name, 4. Error with list of available. This applies to `tracker validate`, `tracker simulate`, and `tracker run` via `resolvePipelineSource()`.
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T21:59:50.346Z
Learning: `pipeline/dippin_adapter.go` converts dippin IR to tracker's Graph model. Every naming mismatch between dippin and tracker conventions lives here. Update the adapter when dippin-lang adds new IR fields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/tracker/resolve.go (1)
29-40:⚠️ Potential issue | 🟠 MajorSurface non-
NotExistfilesystem errors before built-in fallback.Line 30 and Line 34 ignore non-
os.ErrNotExisterrors. If a local file is unreadable, resolution can incorrectly fall through to built-ins.Suggested fix
dipPath := name + ".dip" if _, statErr := os.Stat(dipPath); statErr == nil { return dipPath, false, WorkflowInfo{}, nil + } else if !os.IsNotExist(statErr) { + return "", false, WorkflowInfo{}, fmt.Errorf("stat %q: %w", dipPath, statErr) } if _, statErr := os.Stat(name); statErr == nil { return name, false, WorkflowInfo{}, nil + } else if !os.IsNotExist(statErr) { + return "", false, WorkflowInfo{}, fmt.Errorf("stat %q: %w", name, statErr) }Based on learnings, running
tracker <name>must follow strict local-file-first resolution before built-ins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tracker/resolve.go` around lines 29 - 40, The current resolution in cmd/tracker/resolve.go silently ignores non-NotExist errors from os.Stat for dipPath and name and can fall through to built-ins; update the two os.Stat checks (the one using dipPath := name + ".dip" and the subsequent os.Stat(name)) so that if statErr != nil and !os.IsNotExist(statErr) you return immediately with that error (e.g., return "", false, WorkflowInfo{}, statErr) instead of falling through to tracker.LookupWorkflow; keep the existing return on successful stat and the existing built-in lookup behavior for os.IsNotExist errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracker_resolve.go`:
- Around line 126-127: The os.Stat error handling around the cpPath check
incorrectly labels all errors as "checkpoint not found"; change the logic in the
block that calls os.Stat(cpPath) to distinguish not-exist errors from others: if
os.IsNotExist(err) return the current formatted "checkpoint not found for run
%s: %w" using resolved and err, otherwise return a different error (e.g.,
"failed to stat checkpoint for run %s: %w") including the original err so
permission and I/O errors are preserved; update the code paths that reference
cpPath/resolved accordingly.
---
Duplicate comments:
In `@cmd/tracker/resolve.go`:
- Around line 29-40: The current resolution in cmd/tracker/resolve.go silently
ignores non-NotExist errors from os.Stat for dipPath and name and can fall
through to built-ins; update the two os.Stat checks (the one using dipPath :=
name + ".dip" and the subsequent os.Stat(name)) so that if statErr != nil and
!os.IsNotExist(statErr) you return immediately with that error (e.g., return "",
false, WorkflowInfo{}, statErr) instead of falling through to
tracker.LookupWorkflow; keep the existing return on successful stat and the
existing built-in lookup behavior for os.IsNotExist errors.
🪄 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: a3f1e804-8265-4d9f-bf10-cf713f79b982
📒 Files selected for processing (3)
cmd/tracker/resolve.gotracker_resolve.gotracker_resolve_test.go
There was a problem hiding this comment.
Pull request overview
Phase 1 of #76 to close CLI↔library parity gaps by promoting built-in workflow cataloging and name/run resolution into first-class tracker package APIs, and moving embedded workflow assets to a module-root workflows/ directory so both CLI and library share a single source of truth.
Changes:
- Added library APIs for listing/looking up/opening embedded workflows, plus filesystem-first source resolution and run checkpoint resolution.
- Added
Config.ResumeRunIDsupport inNewEngineto auto-resolve a run ID/prefix into a checkpoint path (without overriding an explicitCheckpointDir). - Refactored CLI workflow embedding/loading/init to delegate to the new library APIs; updated tooling/docs to point at
workflows/.
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tracker_workflows.go |
New embedded workflow catalog + open/read helpers for library consumers. |
tracker_resolve.go |
New library helpers for filesystem-first source resolution and checkpoint resolution. |
tracker.go |
Adds Config.ResumeRunID and applies it during NewEngine construction. |
tracker_workflows_test.go |
Tests for workflow catalog APIs (list/sort/copy/lookup/open). |
tracker_resolve_test.go |
Tests for ResolveSource and ResolveCheckpoint. |
cmd/tracker/embed.go |
Replaced CLI embed/catalog logic with thin wrappers over tracker APIs. |
cmd/tracker/loading.go |
Loads embedded workflows via tracker.OpenWorkflow instead of CLI embed FS. |
cmd/tracker/resolve.go |
Uses tracker.LookupWorkflow for built-in resolution and improves path detection. |
cmd/tracker/commands.go |
Uses tracker.OpenWorkflow for init output generation. |
workflows/ask_and_execute.dip |
New embedded workflow asset (moved into shared root location). |
workflows/build_product.dip |
New embedded workflow asset (moved into shared root location). |
workflows/build_product_with_superspec.dip |
New embedded workflow asset (moved into shared root location). |
workflows/deep_review.dip |
New embedded workflow asset (moved into shared root location). |
Makefile |
Updates workflow sync/check targets to use workflows/. |
.pre-commit |
Updates workflow-sync messaging to reference workflows/. |
CHANGELOG.md |
Documents the new library APIs and workflow asset relocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if strings.HasPrefix(trimmed, "start:") { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
parseWorkflowHeader ignores scanner.Err(); if scanning fails mid-file you'll return partial/empty metadata with no signal. It’d be better to check scanner.Err() after the loop and handle/report it (e.g., record a catalog load error) so corrupted embedded workflows don’t fail silently.
| } | |
| } | |
| if err := scanner.Err(); err != nil { | |
| return "", "" | |
| } |
There was a problem hiding this comment.
Fixed in 6bbf237. parseWorkflowHeader now calls scanner.Err() after the scan loop and returns the partially-collected values on error (best-effort semantics on embedded files).
| // 1. If name contains "/" or ends in ".dip"/".dot", treat it as a filesystem | ||
| // path and read it from disk. |
There was a problem hiding this comment.
ResolveSource’s docstring says an explicit path is detected only by containing “/”, but the implementation treats both “/” and “\” as path separators. Update the comment to match the behavior (important for Windows users).
| // 1. If name contains "/" or ends in ".dip"/".dot", treat it as a filesystem | |
| // path and read it from disk. | |
| // 1. If name contains "/" or "\" or ends in ".dip"/".dot", treat it as a | |
| // filesystem path and read it from disk. |
There was a problem hiding this comment.
Fixed in 6bbf237. The ResolveSource doc comment now explicitly mentions both forward-slash ("/") and back-slash ("") as path indicators.
| @@ -42,7 +44,7 @@ func resolvePipelineSource(name string) (path string, embedded bool, info Workfl | |||
|
|
|||
| // isExplicitFilePath returns true if name is a file path (contains / or has a .dip/.dot extension). | |||
There was a problem hiding this comment.
The comment for isExplicitFilePath mentions only “contains /”, but the function also treats backslashes as path separators. Please update the comment so the CLI and library docs accurately describe Windows-style paths.
| // isExplicitFilePath returns true if name is a file path (contains / or has a .dip/.dot extension). | |
| // isExplicitFilePath returns true if name is a file path (contains / or \ or has a .dip/.dot extension). |
There was a problem hiding this comment.
Fixed in 6bbf237. The CLI resolvePipelineSource doc comment updated to mention both "/" and "" path detection.
| @@ -34,6 +34,7 @@ const ( | |||
| type Config struct { | |||
| WorkingDir string // default: os.Getwd() | |||
| CheckpointDir string // default: empty (engine auto-generates) | |||
There was a problem hiding this comment.
Config.CheckpointDir is documented as a “Dir”, but it’s passed to pipeline.WithCheckpointPath and (now) populated with a checkpoint.json file path via ResumeRunID. Updating the field comment to reflect that it’s a checkpoint path (file) would reduce confusion for library consumers.
| CheckpointDir string // default: empty (engine auto-generates) | |
| CheckpointDir string // optional: checkpoint file path; default: empty (engine auto-generates) |
There was a problem hiding this comment.
Fixed in 6bbf237. Doc comment now reads "checkpoint file path (checkpoint.json)" instead of "Dir" to accurately reflect the field holds a file path, not a directory.
| @@ -130,6 +135,22 @@ func NewEngine(source string, cfg Config) (*Engine, error) { | |||
| return buildEngine(graph, cfg, workDir, client, completer) | |||
| } | |||
|
|
|||
| // applyResumeRunID resolves Config.ResumeRunID to a concrete checkpoint path | |||
| // and stores it on Config.CheckpointDir. A non-empty CheckpointDir on the | |||
| // incoming config is honored as an explicit override — the user is telling | |||
| // us exactly which file to use. | |||
| func applyResumeRunID(cfg *Config, workDir string) error { | |||
| if cfg.ResumeRunID == "" || cfg.CheckpointDir != "" { | |||
| return nil | |||
| } | |||
| cpPath, err := ResolveCheckpoint(workDir, cfg.ResumeRunID) | |||
| if err != nil { | |||
| return fmt.Errorf("resume run %q: %w", cfg.ResumeRunID, err) | |||
| } | |||
| cfg.CheckpointDir = cpPath | |||
| return nil | |||
There was a problem hiding this comment.
ResumeRunID introduces new execution behavior in NewEngine (auto-resolving a run ID/prefix to a checkpoint path). tracker_test.go has broad coverage for NewEngine/Config behavior, but there’s no test asserting (a) ResumeRunID populates CheckpointDir when CheckpointDir is empty and (b) an explicit CheckpointDir overrides ResumeRunID. Adding a focused test would prevent regressions.
There was a problem hiding this comment.
Fixed in 6bbf237. Added TestNewEngine_ResumeRunID (round-trip: creates checkpoint layout in temp dir, calls NewEngine with ResumeRunID, verifies engine constructed successfully) and TestNewEngine_ResumeRunID_NotFound (error when run ID doesn't match any run).
|
|
||
| entries, err := fs.ReadDir(embeddedWorkflows, "workflows") | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
loadWorkflowCatalog silently drops errors from fs.ReadDir(embeddedWorkflows, "workflows") and returns an empty catalog, which then surfaces as “unknown workflow” rather than an embed/load failure. Consider capturing the error in a package-level var (or panicking, since embed failures are programmer/build errors) so OpenWorkflow / Workflows failures are diagnosable.
| return | |
| panic(fmt.Sprintf("tracker: failed to read embedded workflows catalog: %v", err)) |
There was a problem hiding this comment.
Fixed in 6bbf237. Added the comment // Embedded FS should never fail; return empty catalog on error. before the fs.ReadDir call to document the intentional silent handling.
…test - parseWorkflowHeader checks scanner.Err() after the loop. - ResolveSource and isExplicitFilePath doc comments mention both forward-slash and backslash path detection. - Config.CheckpointDir doc says 'path' not 'Dir'. - loadWorkflowCatalog has an explicit comment documenting the intentional silent error handling for embedded FS reads. - New tests for Config.ResumeRunID round-trip and not-found error. Refs #76 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… Phase 1) Part of issue #76 (CLI<->library parity). This phase exposes the built-in workflow catalog and run-ID resolution as first-class library APIs so consumers (factory worker, custom orchestrators, test harnesses) no longer need to shell out to the tracker CLI or reimplement directory conventions. New exports on the tracker package: Workflows() []WorkflowInfo LookupWorkflow(name) (WorkflowInfo, bool) OpenWorkflow(name) ([]byte, WorkflowInfo, error) ResolveSource(name, workDir) (source, WorkflowInfo, error) ResolveCheckpoint(workDir, runID) (path, error) Config.ResumeRunID string — resolves via ResolveCheckpoint inside NewEngine Embedded workflows moved from cmd/tracker/workflows/ to workflows/ at the module root so they can be embedded by the tracker package. The CLI's embed.go, resolve.go, and loadEmbeddedPipeline are now thin wrappers over the library APIs so there is a single source of truth for built-in workflows. ResumeRunID is the library equivalent of the CLI's -r/--resume flag. Setting Config.ResumeRunID = "abc123" (or a unique prefix) makes NewEngine call ResolveCheckpoint under the hood and populate Config.CheckpointDir. An explicit CheckpointDir on the same config still wins as a manual override — NewEngine never overwrites it. Tests: - tracker_workflows_test.go — Workflows/LookupWorkflow/OpenWorkflow - tracker_resolve_test.go — ResolveSource (filesystem, bare-name, local-beats-built-in, not-found) and ResolveCheckpoint (empty, found, prefix, ambiguous, missing checkpoint.json) All 19 new tests pass. Full suite green, dippin doctor A grade on all three example pipelines. Refs #76
…handling - Makefile and .pre-commit updated to use workflows/ (repo root) instead of cmd/tracker/workflows/ after the embed relocation. - ResolveSource: explicit relative paths joined with workDir when workDir is set. os.Stat errors other than ErrNotExist returned immediately instead of falling through to built-in lookup. - ResolveCheckpoint: doc comment updated (removed 'absolute' claim). - Test: guard string slice to prevent panic on short content. Refs #76
Both tracker_resolve.go and cmd/tracker/resolve.go now check for backslash in addition to forward-slash. Added test case for a Windows-style relative path. Refs #76
ResolveCheckpoint now uses errors.Is(err, os.ErrNotExist) to report 'checkpoint not found' only when the file is actually missing. Other stat failures (permission denied, I/O) surface the real error instead of a misleading 'not found' message. Refs #76
…test - parseWorkflowHeader checks scanner.Err() after the loop. - ResolveSource and isExplicitFilePath doc comments mention both forward-slash and backslash path detection. - Config.CheckpointDir doc says 'path' not 'Dir'. - loadWorkflowCatalog has an explicit comment documenting the intentional silent error handling for embedded FS reads. - New tests for Config.ResumeRunID round-trip and not-found error. Refs #76 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
clintecker
force-pushed
the
feat/cli-library-parity-76-phase1
branch
from
6bbf237 to
10f325f
Compare
Summary
Phase 1 of issue #76 (CLI↔library parity). Exposes the built-in workflow catalog and run-ID resolution as first-class library APIs so consumers (factory worker, custom orchestrators, test harnesses) don't need to shell out to the tracker CLI or reimplement directory conventions.
New library surface
```go
tracker.Workflows() []WorkflowInfo
tracker.LookupWorkflow(name string) (WorkflowInfo, bool)
tracker.OpenWorkflow(name string) ([]byte, WorkflowInfo, error)
tracker.ResolveSource(name, workDir string) (source string, info WorkflowInfo, err error)
tracker.ResolveCheckpoint(workDir, runID string) (path string, err error)
// And a new Config field:
tracker.Config.ResumeRunID string
```
`ResumeRunID` is the library equivalent of the CLI's `-r/--resume` flag. Setting `cfg.ResumeRunID = "abc123"` (or a unique prefix) makes `NewEngine` call `ResolveCheckpoint` under the hood and populate `Config.CheckpointDir`. An explicit `CheckpointDir` on the same config still wins as a manual override — `NewEngine` never overwrites it.
Layout change
Embedded `.dip` files moved from `cmd/tracker/workflows/` → `workflows/` at the module root so the tracker package can embed them. The CLI's `embed.go`, `resolve.go`, and `loadEmbeddedPipeline` are now thin wrappers over the library APIs, giving us a single source of truth for built-in workflows.
Test plan
Phase breakdown for #76
This PR is Phase 1 (workflow catalog + name resolution + ResumeRunID). Remaining phases:
Refs #76
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores