fix: reconcile validation between dippin-lang and tracker#71
fix: reconcile validation between dippin-lang and tracker#71clintecker merged 5 commits intomainfrom
Conversation
Add Graph.DippinValidated flag that is set true after dippin-lang's validator (DIP001–DIP009) runs during .dip source parsing. When set, tracker's own validateGraph skips the structural checks that overlap with dippin's diagnostics (start/exit existence, edge endpoints, reachability, cycles, duplicate edges), preventing false positives and divergence between `dippin doctor` and `tracker validate`. Tracker-specific checks that dippin does not cover still run regardless of source format: shape→handler resolution, conditional-fail-edge warnings, and edge-label consistency warnings. DOT-format graphs (DippinValidated=false) continue to run all structural checks since no upstream validator has covered them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a Dippin-aware load path: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "tracker CLI"
participant Loader as "pipeline.LoadDippinWorkflow"
participant Parser as "dippin-lang/parser"
participant Validator as "dippin-lang/validator"
participant Conv as "FromDippinIR -> Graph"
participant Validate as "pipeline.Validate"
CLI->>Loader: Load(source, filename)
Loader->>Parser: parse(source, filename)
Parser-->>Loader: workflow / parse error
Loader->>Validator: Validate(workflow) %% structural (fatal if errors)
Validator-->>Loader: validation diagnostics / errors
Loader->>Validator: Lint(workflow) %% lint (non-fatal)
Validator-->>Loader: lint diagnostics
Loader->>Conv: FromDippinIR(workflow)
Conv-->>Loader: *Graph / conversion error
Loader-->>CLI: return graph + diagnostics
CLI->>CLI: print diagnostics to stderr
CLI->>Validate: Validate(graph)
Validate->>Validate: if graph.DippinValidated skip structural checks
Validate-->>CLI: validation result / warnings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pipeline/validate_test.go (1)
367-433: Tighten skip-check subtests to fail on any unexpected error.Right now those subtests only fail when the error string contains one token (
start,unreachable,cycle,duplicate). Any different validation error would be ignored and the test would still pass.Proposed assertion tightening
- if err := Validate(g); err != nil { - if strings.Contains(err.Error(), "start") { - t.Errorf("DippinValidated graph should not re-check for missing start node, got: %v", err) - } - } + if err := Validate(g); err != nil { + t.Fatalf("expected no validation errors when DippinValidated=true, got: %v", err) + } ... - if err := Validate(g); err != nil { - if strings.Contains(err.Error(), "unreachable") { - t.Errorf("DippinValidated graph should not re-check reachability, got: %v", err) - } - } + if err := Validate(g); err != nil { + t.Fatalf("expected no validation errors when DippinValidated=true, got: %v", err) + } ... - if err := Validate(g); err != nil { - if strings.Contains(err.Error(), "cycle") { - t.Errorf("DippinValidated graph should not re-check cycles, got: %v", err) - } - } + if err := Validate(g); err != nil { + t.Fatalf("expected no validation errors when DippinValidated=true, got: %v", err) + } ... - if err := Validate(g); err != nil { - if strings.Contains(err.Error(), "duplicate") { - t.Errorf("DippinValidated graph should not re-check duplicate edges, got: %v", err) - } - } + if err := Validate(g); err != nil { + t.Fatalf("expected no validation errors when DippinValidated=true, got: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/validate_test.go` around lines 367 - 433, The subtests for DippinValidated graphs (e.g., "missing start node is ignored when dippin-validated", "unreachable node is ignored when dippin-validated", "unconditional cycle is ignored when dippin-validated", "duplicate edges ignored when dippin-validated") currently only fail if the error string contains a specific token, allowing other unexpected validation errors to pass; update each subtest that calls Validate(g) to fail on any non-nil error (not just errors containing "start"/"unreachable"/"cycle"/"duplicate") by asserting err == nil and reporting the full error from Validate, ensuring Validate, NewGraph, and the DippinValidated flag behavior are being correctly enforced in the tests.cmd/tracker/loading.go (1)
194-196: Consider centralizing the post-validateDippinValidatedassignment.The same marker logic is duplicated in multiple DIP ingestion paths. A shared helper for parse+validate+convert+mark would reduce future divergence risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tracker/loading.go` around lines 194 - 196, Duplicate setting of graph.DippinValidated should be centralized: create a shared helper (e.g., finalizeDippinGraph or processDIPGraph) that performs parse, validate, convert and then sets graph.DippinValidated = true; replace the duplicated post-validate assignments in each DIP ingestion path with a single call to this helper, ensuring the helper accepts the same graph (and any needed context/params) and returns the converted result or error so callers no longer set DippinValidated themselves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pipeline/validate.go`:
- Around line 2-3: Update the stale ABOUTME header to accurately reflect
behavior: mention that duplicate-edge checks (validateNoDuplicateEdges) are
conditional and only run when the graph's DippinValidated flag is false (i.e.,
!g.DippinValidated); adjust the ABOUTME text to remove "always run" and clarify
which checks are skipped when DippinValidated=true and which still execute
unconditionally.
---
Nitpick comments:
In `@cmd/tracker/loading.go`:
- Around line 194-196: Duplicate setting of graph.DippinValidated should be
centralized: create a shared helper (e.g., finalizeDippinGraph or
processDIPGraph) that performs parse, validate, convert and then sets
graph.DippinValidated = true; replace the duplicated post-validate assignments
in each DIP ingestion path with a single call to this helper, ensuring the
helper accepts the same graph (and any needed context/params) and returns the
converted result or error so callers no longer set DippinValidated themselves.
In `@pipeline/validate_test.go`:
- Around line 367-433: The subtests for DippinValidated graphs (e.g., "missing
start node is ignored when dippin-validated", "unreachable node is ignored when
dippin-validated", "unconditional cycle is ignored when dippin-validated",
"duplicate edges ignored when dippin-validated") currently only fail if the
error string contains a specific token, allowing other unexpected validation
errors to pass; update each subtest that calls Validate(g) to fail on any
non-nil error (not just errors containing
"start"/"unreachable"/"cycle"/"duplicate") by asserting err == nil and reporting
the full error from Validate, ensuring Validate, NewGraph, and the
DippinValidated flag behavior are being correctly enforced in the tests.
🪄 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: bf840e54-f894-45ce-b572-8d371d1a4f30
📒 Files selected for processing (5)
cmd/tracker/loading.gopipeline/graph.gopipeline/validate.gopipeline/validate_test.gotracker.go
| // ABOUTME: Tracker-specific checks (shapes, duplicate edges, conditional routing) always run. | ||
| // ABOUTME: Structural checks that dippin-lang already covers are skipped when DippinValidated=true. |
There was a problem hiding this comment.
Fix stale header comment: duplicate-edge checks are not always run.
The ABOUTME note says duplicate-edge checks always run, but Line 134 runs validateNoDuplicateEdges only when !g.DippinValidated.
Suggested comment correction
-// ABOUTME: Tracker-specific checks (shapes, duplicate edges, conditional routing) always run.
+// ABOUTME: Tracker-specific checks (shape validation and conditional-routing warnings) always run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pipeline/validate.go` around lines 2 - 3, Update the stale ABOUTME header to
accurately reflect behavior: mention that duplicate-edge checks
(validateNoDuplicateEdges) are conditional and only run when the graph's
DippinValidated flag is false (i.e., !g.DippinValidated); adjust the ABOUTME
text to remove "always run" and clarify which checks are skipped when
DippinValidated=true and which still execute unconditionally.
There was a problem hiding this comment.
Fixed in 40daf6c. The ABOUTME header now accurately reflects that tracker-specific checks (shapes, conditional routing) always run, while structural checks (start/exit, reachability, cycles, duplicate edges) are skipped for dippin-validated graphs.
There was a problem hiding this comment.
@clintecker, thanks for the update! The corrected ABOUTME header now accurately reflects the actual behavior — structural checks (start/exit, reachability, cycles, duplicate edges) are skipped for dippin-validated graphs, while shape validation and conditional-routing warnings always run unconditionally.
🧠 Learnings used
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/*.go : When a node's outcome is 'fail' and all outgoing edges are unconditional, the pipeline stops
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/dippin_adapter.go : `pipeline/dippin_adapter.go` converts dippin IR to tracker's Graph model — update it when dippin-lang adds new IR fields
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/cmd/tracker/*.go : Pipeline source resolution applies uniformly to `tracker validate`, `tracker simulate`, and `tracker run`
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/*.dip : Pipelines that intentionally handle failure must use `when ctx.outcome = fail` edges
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/*.go : Checkpoints store completed nodes and context snapshots with edge selections stored per-node for deterministic replay
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/cmd/tracker/*.go : `tracker doctor` — preflight health check (API keys, dippin binary, workdir). Run before first pipeline.
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/dippin_adapter.go : The adapter must synthesize implicit edges from `ParallelConfig.Targets` and `FanInConfig.Sources`
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/*.go : Nodes with ANY conditional edges are assumed to have intentional routing
Learnt from: CR
Repo: 2389-research/tracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T18:32:26.002Z
Learning: Applies to **/*.dip : NEVER add an unconditional edge to the same target as a conditional edge (causes infinite loops)
![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: 9e7885c71d
ℹ️ 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".
| // DIP005 — unconditional cycle detected | ||
| // DIP006 — exit node has outgoing edges | ||
| // DIP009 — duplicate edge | ||
| if !g.DippinValidated { |
There was a problem hiding this comment.
validateGraph now bypasses start/exit, endpoint, reachability, cycle, and duplicate-edge checks whenever g.DippinValidated is true. Since DippinValidated is a mutable exported field on Graph, any caller (or a graph that is modified after parsing) can flip this bit and make pipeline.Validate accept malformed graphs; the added table test cases already demonstrate invalid graphs passing once the flag is set. This turns validation into an opt-out and can let bad graphs reach engine execution, where failures become runtime-only and harder to diagnose.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40daf6c. Added a CONTRACT comment on the DippinValidated field in graph.go explaining the mutation responsibility: callers who mutate the graph after dippin validation must clear the flag or re-run validation, otherwise structural checks will be incorrectly skipped.
There was a problem hiding this comment.
Pull request overview
Reconciles pipeline validation behavior between dippin-lang and tracker by marking graphs produced from .dip sources as already structurally validated, and skipping tracker structural checks that overlap with dippin’s DIP001–DIP009 diagnostics to avoid divergence.
Changes:
- Add
Graph.DippinValidatedand set it totruefor graphs produced via.dipparsing/conversion. - Update
validateGraphto skip structural checks (start/exit, endpoints, reachability, cycles, duplicates) whenDippinValidated=true, while still running tracker-specific checks (shapes, conditional warnings, label consistency). - Add a new table-style test to verify the skip behavior and that tracker-specific checks still run.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pipeline/graph.go |
Adds DippinValidated flag to Graph model. |
pipeline/validate.go |
Gates structural validation on !DippinValidated; always runs tracker-specific checks. |
tracker.go |
Marks .dip-parsed graphs as DippinValidated=true. |
cmd/tracker/loading.go |
Marks CLI-loaded .dip graphs as DippinValidated=true. |
pipeline/validate_test.go |
Adds tests covering skip behavior and “still-run” tracker checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ABOUTME: Tracker-specific checks (shapes, duplicate edges, conditional routing) always run. | ||
| // ABOUTME: Structural checks that dippin-lang already covers are skipped when DippinValidated=true. |
There was a problem hiding this comment.
The ABOUTME header says duplicate-edge checks always run, but validateNoDuplicateEdges is now only executed when DippinValidated=false. Please update the header comment to match the actual behavior (duplicate-edge checks are skipped for dippin-validated graphs).
| // ABOUTME: Tracker-specific checks (shapes, duplicate edges, conditional routing) always run. | |
| // ABOUTME: Structural checks that dippin-lang already covers are skipped when DippinValidated=true. | |
| // ABOUTME: Tracker-specific checks such as shapes and conditional routing always run. | |
| // ABOUTME: Structural checks that dippin-lang already covers, including duplicate-edge checks, are skipped when DippinValidated=true. |
| if err := Validate(g); err != nil { | ||
| if strings.Contains(err.Error(), "start") { | ||
| t.Errorf("DippinValidated graph should not re-check for missing start node, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This subtest is meant to confirm that structural validation is bypassed when DippinValidated=true, but it currently allows Validate() to return a non-nil error as long as it doesn't contain "start". That can mask unexpected failures and make the test pass incorrectly. Prefer asserting err == nil here (and similarly in the other "skipped" cases).
There was a problem hiding this comment.
Fixed in 40daf6c. The four skipped-check subtests now assert err==nil directly (not merely 'no error mentioning X'), with comments explaining why each expects no error from tracker-specific checks either.
|
@copilot please address all the review feedback |
Agent-Logs-Url: https://github.com/2389-research/tracker/sessions/2af5f1fe-e71c-4d7a-b7de-eb7f062e7c00 Co-authored-by: clintecker <26985+clintecker@users.noreply.github.com>
Addressed both review comments in commit
|
- Added mutation contract to DippinValidated field: callers must clear the flag or re-run dippin validation after mutating the graph, or structural checks will be skipped for the new shape - Tightened TestDippinValidatedSkipsStructuralChecks to assert err==nil (not merely "no error mentioning X"), with explanatory comments on why each subtest expects no error Refs #60 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // DIP001 — start node missing (partial: we also check for >1 start) | ||
| // DIP002 — exit node missing (partial: we also check for >1 exit) | ||
| // DIP003 — unknown node reference in edge | ||
| // DIP004 — unreachable node(s) from start | ||
| // DIP005 — unconditional cycle detected | ||
| // DIP006 — exit node has outgoing edges | ||
| // DIP009 — duplicate edge |
There was a problem hiding this comment.
The note that DIP001/DIP002 are only "partial" because tracker also checks for ">1 start/exit" is confusing in the DippinValidated flow: .dip workflows have a single workflow.Start/workflow.Exit (see FromDippinIR), and ensureStartExitNodes forces exactly one node to carry each start/exit shape. Consider rewording to clarify that the ">1" check is primarily relevant to DOT graphs, or drop the "partial" qualifier here to avoid implying dippin-lang might allow multiple start/exit nodes.
| // DIP001 — start node missing (partial: we also check for >1 start) | |
| // DIP002 — exit node missing (partial: we also check for >1 exit) | |
| // DIP003 — unknown node reference in edge | |
| // DIP004 — unreachable node(s) from start | |
| // DIP005 — unconditional cycle detected | |
| // DIP006 — exit node has outgoing edges | |
| // DIP009 — duplicate edge | |
| // DIP001 — start node missing | |
| // DIP002 — exit node missing | |
| // DIP003 — unknown node reference in edge | |
| // DIP004 — unreachable node(s) from start | |
| // DIP005 — unconditional cycle detected | |
| // DIP006 — exit node has outgoing edges | |
| // DIP009 — duplicate edge | |
| // Tracker also enforces exactly one start and exit when validating | |
| // non-Dippin/DOT graphs; that multiplicity check is not part of the | |
| // skipped dippin-lang overlap for DippinValidated inputs. |
| // Mark graph as already validated by dippin-lang so that tracker's | ||
| // own validator skips redundant structural checks (DIP001–DIP009). | ||
| graph.DippinValidated = true |
There was a problem hiding this comment.
This graph.DippinValidated = true assignment is duplicated with the same intent in cmd/tracker/loading.go. To reduce the chance of future .dip load paths forgetting to set the flag (and accidentally re-enabling redundant structural validation), consider centralizing this in a single helper (e.g., a shared loader or a small function that wraps parse+validate+convert).
| // Mark graph as already validated by dippin-lang so that tracker's | ||
| // own validator skips redundant structural checks (DIP001–DIP009). | ||
| graph.DippinValidated = true |
There was a problem hiding this comment.
This .dip loader sets graph.DippinValidated = true, and the inline .dip parser does the same in tracker.go. Consider factoring the parse+validate+convert+mark logic into a shared helper to avoid drift between the two code paths and to ensure any future .dip entry point consistently marks graphs as pre-validated.
- Clarify start/exit validation comment to note DIP001/DIP002 give complete coverage for .dip flows (DOT is the only path that needs tracker's >1 start/exit check). - Factor duplicated parse+validate+convert+mark logic out of tracker.go and cmd/tracker/loading.go into a single pipeline helper so future .dip entry points can't forget to mark DippinValidated. Refs #60
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 `@pipeline/dippin_load.go`:
- Around line 26-28: The current early-return drops structured diagnostics by
doing "return nil, nil, fmt.Errorf(...)" when valResult.HasErrors() is true;
change this to preserve and return valResult.Diagnostics (e.g., "return nil,
valResult.Diagnostics, fmt.Errorf(...)" or otherwise include
valResult.Diagnostics in the returned error payload) so callers can inspect
file/line diagnostics; ensure you still include the existing formatted summary
using filename and len(valResult.Errors()) while returning the Diagnostics
instead of nil.
🪄 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: 79ff9a09-7de4-48d1-bc34-115794e0cc1a
📒 Files selected for processing (4)
cmd/tracker/loading.gopipeline/dippin_load.gopipeline/validate.gotracker.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/tracker/loading.go
- pipeline/validate.go
Summary
Graph.DippinValidated boolfield that is set totrueafter the dippin-lang validator (DIP001–DIP009) runs successfully during.dipsource parsingvalidateGraph, skip the structural checks that overlap with dippin's diagnostics whenDippinValidated=true— these are: start/exit existence, edge endpoint validity, reachability, unconditional cycles, and duplicate edgesvalidateShapes(DOT shape→handler resolution),validateConditionalFailEdges, andvalidateEdgeLabelConsistencyDippinValidated=false) continue to run all structural checks, since no upstream validator has covered themWhat was removed vs kept
Removed from
.dipcode path (dippin already covers these via DIP001–DIP009):validateStartExit— DIP001 (missing start) and DIP002 (missing exit)validateEdgeEndpoints— DIP003 (unknown node reference)validateExitOutgoingEdges— DIP006 (exit node has outgoing edges)validateReachability— DIP004 (unreachable nodes)validateNoCycles— DIP005 (unconditional cycles)validateNoDuplicateEdges— DIP009 (duplicate edges)Kept (tracker-specific, dippin does not check these):
validateShapes— maps DOT shapes to handler names; dippin usesrole:not DOT shapesvalidateConditionalFailEdges— warns on diamond nodes missing a fail pathvalidateEdgeLabelConsistency— warns on mixed labeled/unlabeled edges from a single nodeTest plan
go build ./...passesgo test ./... -short— all 15 packages passdippin doctor examples/ask_and_execute.dip examples/build_product.dip examples/build_product_with_superspec.dip— Grade A, 100/100TestDippinValidatedSkipsStructuralCheckstable test covers all 6 sub-cases: missing start, unreachable node, unconditional cycle, duplicate edges (skipped for dippin-validated), and shape check + fail-edge warning (still fire for dippin-validated)TestValidateNoStartNode,TestValidateCycleDetection, etc.) still pass because manually-constructed test graphs haveDippinValidated=falseCloses #60
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Change
Tests