Skip to content

refactor(graph): use shared engine for gate traversals - #3316

Merged
thymikee merged 6 commits into
mainfrom
refactor/ws10-shared-graph-traversals
Oct 8, 2026
Merged

thymikee merged 6 commits into
mainfrom
refactor/ws10-shared-graph-traversals

Conversation

@thymikee

@thymikee thymikee commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Summary

Validation

  • Head 8ed2225f0811ad2932c34daf08942dfa20907a17: pnpm check:affected --run passed every runnable check; new CI pending.
  • Against rebased main bf2246499a: all 10,182 resolved edges, 22 depgraph cycles, layering stdout and seven ownership sets (3,813 assignments) match exactly.
  • Chord regression failed before the fix, then passed; 1,024 synthetic cycle comparisons match the old helper.
  • Temporary tracked violations failed with R4 value-import-cycle and R6 type-spine-inversion, then were removed. Chord fixture now claims value-cycle selection only; independent type/dynamic fixtures cover those kinds. Review responses contain the comparison details.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.12 MB 5.12 MB 0 B
Package (unpacked) 5.12 MB 5.12 MB 0 B
Package (download) 1.54 MB 1.54 MB -4 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 30.0 ms 27.8 ms -2.2 ms
CLI --help 88.9 ms 82.5 ms -6.4 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/layering/provider-snapshot-presentation-policy.test.ts Outdated
Comment thread scripts/layering/provider-snapshot-presentation-policy.ts Outdated
Comment thread scripts/depgraph/import-graph.ts Outdated
@thymikee
thymikee added this pull request to stack #3320 October 8, 2026 11:06
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

The code looks good at cf887a4, and all 22 checks pass, including check:layering, depgraph:test and the Mutation Affected run on this head. I did not run check:layering or pnpm depgraph at head and at the merge base. So the claims of an identical layering summary, depgraph cycle list and ownership sets are not yet proven. Please post that comparison, or a note that it matches, and this is ready. No conflicts. Nothing else blocks merge.

Not blocking, and you can take or leave these: (1) the old findCyclePath in scripts/layering/model.ts could return a different cycle than genCycles(componentGraph).next() for an SCC of 3 or more nodes with chords, and collectCycles in scripts/depgraph/model.ts dedupes by member set, so the depgraph cycle list may shift; a test on a 3-node SCC with a chord and a cycle-list diff against the merge base would settle it; (2) the export type { EdgeKind } re-export in scripts/depgraph/model.ts has no importer and can go; (3) the LANE_CANARY change in scripts/mutation/modules.ts is a separate CI-budget fix and could be its own commit; (4) ownership in scripts/mutation/ownership.ts now reads only tracked sources, which the PR body does not say; (5) a one-line note on why the BFS in src/tests/eager-import-closure.fixtures.ts stays hand-rolled (item 4 of #3277) would help. Also, when two SCCs tie in size, largestTypeCycleMembers may now pick a different one; the ratchet uses the new function on both sides, so it stays consistent.

Is there a smaller shape? I looked and found none. importGraphFromResolvedEdges overlaps a little with collapseEdges, but collapsing to the strongest kind before a kind filter would drop pairs whose only kind is the weaker one, so the separate filtered dedupe makes sense.

The cubic-dev-ai thread on the presentation policy test (#3316 (comment)), the one on the graph being built once per call (#3316 (comment)) and the one on edgeKind ownership (#3316 (comment)) are all fixed at this head, so you can resolve them.

@thymikee
thymikee force-pushed the refactor/ws10-shared-graph-traversals branch from cf887a4 to 98b6e83 Compare October 8, 2026 13:42
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Addressed the broader review in 98b6e832c42edfa9a9a2c31efacf8bca0c3a1d82, rebased onto bf2246499a.

The explicit comparison caught a real cycle-representative difference: the library’s first cycle changed the contracts type cycle and the Limrun dynamic cycle. The helper now builds the first-outgoing-edge subgraph inside each SCC and uses library DFS from the lexical root. Inside an SCC the old DFS reaches its first back edge before backtracking, so this preserves its representative without restoring a traversal implementation.

Comparison against rebased main Result
All 10,182 resolved production edges, including symbols/kinds/order Identical
Full depgraph cycle list (22 entries, paths and kinds) Identical
Layering checker stdout Byte-identical
Per-module ownership test sets (7 modules, 3,813 assignments) Identical

Two chorded three-file fixtures cover both a cycle through the root and a cycle closing below it. The latter failed before the fix; both pass now. An exhaustive comparison of all 512 three-node directed graphs in both edge orders (1,024 cases) also matches the old helper. Temporary tracked violations still fail specifically with R4 value-import-cycle and R6 type-spine-inversion; all fixtures were removed.

Removed the unused EdgeKind re-export and corrected the deriver’s cache comment. The canary budget fix remains its own commit (ffafd8ab0). Ownership reads tracked sources; the PR body now says so. WORKSPACE_SPECIFIER normalizes already-extracted strings; the eager-closure BFS was delivered by #3298 and explicitly excluded from this migration.

pnpm check:affected --run passed on this exact head. New CI is pending. The previous successful Mutation Affected shard at cf887a45e took 19:14, versus the former kernel-errors cancellations at 30:18/30:20; no timeout was increased. All three inline threads remain resolved.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/depgraph/model.test.ts Outdated
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

The code looks good at 8ed2225. The chord fixture in scripts/depgraph/model.test.ts no longer has the extra type and dynamic edges, and the test name now says value-cycle. The leftover EdgeKind re-export is gone, and edgeKind lives once in scripts/depgraph/import-graph.ts. Your posted comparison (edges, cycle list, layering output and ownership sets) closes the evidence gap from the last review, and the two chorded three-node tests match the old findCyclePath result when traced by hand.

Evidence limits: I could not confirm two @statelyai/graph 2.4.0 behaviors, because it is not installed in my checkout: that getSubgraph keeps the parent's edge order, and that genDFS(graph, stringId) yields only reachable nodes in preorder. The equivalence depends on both, and your identical cycle-list comparison is the evidence for them. I did not rerun that comparison.

The four earlier review threads are fixed at this commit.

Smoke Tests and Mutants (snapshot-occlusion) are still running, and nothing has failed. Smoke Tests cover the device and daemon route, which this diff does not touch. Mutants (snapshot-occlusion) selects its tests through scripts/mutation/ownership.ts and modules.ts, so this PR overlaps that job; if it goes red, please check whether ownership changed. No conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 8, 2026
@thymikee
thymikee merged commit efcbba2 into main Oct 8, 2026
22 checks passed
@thymikee
thymikee deleted the refactor/ws10-shared-graph-traversals branch October 8, 2026 14:57
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-08 14:58 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Architecture WS1: replace hand-rolled graph traversals with @statelyai/graph

1 participant