Skip to content

feat(depgraph): dominator, zone-SCC, and cohesion report summaries - #3285

Merged
thymikee merged 2 commits into
refactor/depgraph-statelyai-graphfrom
feat/ws7-depgraph-reports
Oct 7, 2026
Merged

thymikee merged 2 commits into
refactor/depgraph-statelyai-graphfrom
feat/ws7-depgraph-reports

Conversation

@thymikee

@thymikee thymikee commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Part of #3283 (workstream 7 of the architecture-quality umbrella #3276). Adds three report-only fields to pnpm depgraph, stacked on #3275's @statelyai/graph migration:

  • dominatorSummary — dominator tree of one entry's eager-load closure over value edges only (default src/daemon.ts, override with --dominator-entry <path>). Reproduces Architecture quality: replace custom graph/layering code with maintained tools, close guardrail gaps, fix collocation (umbrella) #3276's own numbers against the current tree: 621/1837 eager files, with request-binding.ts/replay-device-selection.ts pulling in all of maestro and provider-device-runtimes.ts pulling in all of provider-webdriver.
  • zoneSccSummary — strongly connected components of the zone graph over value zone-pairs only (same edge kind R4 keeps acyclic at file level). Reproduces the quoted 9-zone cycle (cli, commands, core, daemon-client, daemon-server, remote, sdk, plugins, (root)).
  • cohesionSummary — Louvain communities vs. declared zones scored with getModularity, plus per-zone cohesion share. Reproduces 0.373 (declared) vs 0.564 (detected) modularity and the five lowest per-zone shares (kernel 26%, contracts 29%, sdk 31%, (root) 37%, host-kit 38%).

Nothing in scripts/layering/ or scripts/check-affected/ reads these fields — no gate behavior changed. getModularity wants Community<N> (full node objects) but getLouvainCommunities returns bare ids; computeCohesionSummary maps ids back onto the graph's own nodes once, in the owning module, rather than at each call site (this mismatch is also one of the four upstream @statelyai/graph issues deliverable 2 of #3283 covers — not yet filed this round, tracked separately in #3283).

7 files touched, +506/-10 lines (git diff --stat), within budget.

pnpm depgraph
pnpm depgraph --dominator-entry src/cli/entry.ts

Validation

Tested at 8368e7b67:

No device-facing or routing changes; docs-only risk is the README section added for the new fields.

View guided diff Turn on auto-fix

Adds three report-only fields to `pnpm depgraph` nothing in scripts/layering
or scripts/check-affected reads: a dominator-tree summary of one entry's
eager-load closure over value edges (default src/daemon.ts, overridable with
--dominator-entry), zone-level strongly connected components over value zone
pairs, and Louvain community detection scored against declared zones with
getModularity, plus a per-zone cohesion share. Each reproduces the numbers
#3276 quotes from the earlier ad hoc analysis (621/1837 eager files, the
maestro/provider-webdriver dominator branches, the 9-zone cycle, 0.373 vs
0.564 modularity, and the five lowest per-zone cohesion shares) against the
current tree.

getModularity wants Community<N> (full node objects) but getLouvainCommunities
returns bare ids; computeCohesionSummary maps ids back onto the graph's own
nodes rather than working around it at every call site.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-07 16:40 UTC

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.12 MB 5.12 MB +43 B
Package (unpacked) 5.12 MB 5.12 MB +43 B
Package (download) 1.54 MB 1.54 MB +19 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.8 ms 26.3 ms -1.6 ms
CLI --help 81.0 ms 80.5 ms -0.5 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 7 files

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
Comment thread scripts/depgraph/README.md Outdated
Comment thread scripts/depgraph/import-graph.ts Outdated
Comment thread scripts/depgraph/build.ts
Review findings on #3285:
- model.test.ts asserted the SCC/modularity summary lines with shape-only
  regexes that accepted any count or score, so a CLI line inconsistent with
  the JSON it was derived from still passed. Compare against the exact
  payload values instead (planted a wrong modularity value to confirm the
  test now fails without the fix).
- flagValue returned an empty string for `--out ""` / `--dominator-entry ""`
  verbatim, which bypassed the `?? <default>` fallback (only nullish values
  trigger it) and resolved --out to the current directory, crashing the
  write with EISDIR instead of falling back like a missing value does. Fixed
  at the shared helper, with a regression test for both flags (confirmed
  both fail against the prior flagValue).
- README overstated dominatorSummary.bottlenecks as covering "every
  reachable file"; the entry itself is excluded by construction.
- Clarified the ALL_EDGES doc comment: computeZoneSccSummary builds its own
  value-only zone graph and never reads this constant, only
  computeCohesionSummary does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Addressed all 4 review findings in 520f5d1:

  1. model.test.ts — the SCC/modularity assertions used shape-only regexes (\d+) that accepted any value, so a CLI line disagreeing with the JSON still passed. Replaced with exact-value comparisons against the payload, formatted with the same toFixed(3) as build.ts. Verified by planting a wrong value and confirming the test fails, then reverting.
  2. README.md — bottlenecks excludes the entry file by construction (reachable.filter((id) => id !== entry)); fixed the prose to say "non-entry file."
  3. import-graph.ts — ALL_EDGES's doc comment wrongly implied zone-SCC reporting uses it; computeZoneSccSummary builds its own value-only zone graph and never reads this constant. Narrowed the comment.
  4. build.ts — confirmed and reproduced the --out "" EISDIR crash. Fixed at the shared flagValue helper (not just the --out call site, since --dominator-entry has the identical gap) so an empty flag value now falls through to the default like a missing one does. Added regression tests for both flags; each fails against the prior flagValue.

pnpm check:affected --run passes on 520f5d1 (pushed).

@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The PR is ready at 520f5d1. The code looks correct. All 17 checks pass, and that includes the depgraph gate, which runs build.ts end to end. No conflicts.

Not blocking, and you can take or leave these: the toCommunities doc in scripts/depgraph/structure-summary.ts (line 150) points to an upstream issue that the README does not record, so drop that parenthetical or add the URL once #3283 files it. The test at scripts/depgraph/model.test.ts line 527 says it "reports a typed error", but it only checks the stderr text from a plain Error, so a name like "fails for a dominator entry outside the graph" would be more accurate. DEFAULT_DOMINATOR_ENTRY in scripts/depgraph/build.ts (line 40) is src/daemon.ts, so renaming that file fails the depgraph gate over a report-only field. Either note this in the README or derive the entry from the daemon entry declaration.

I did not run pnpm depgraph locally, so the #3276 numbers are your claim only. I also could not read getLouvainCommunities (@statelyai/graph 2.4.0 is not installed here). If it uses randomness, the 0.564 and per-zone figures may vary between runs, though the CI assertions are range-only and will not flake.

Nothing blocks this PR. #3283 can close once the four upstream issues are filed and linked there, since this PR is only "Part of" it.

The four cubic-dev-ai threads no longer apply at this commit, so you can resolve them: the exact-payload assertions in model.test.ts (#3285 (comment)), the "every reachable non-entry file" wording in the README (#3285 (comment)), the VALUE zone pairs note in import-graph.ts (#3285 (comment)), and the empty --out '' handling in build.ts (#3285 (comment)).

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 7, 2026
@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review. Decisions on the 3 non-blocking suggestions — all accepted:

  1. toCommunities doc references an unrecorded upstream issue — accepted. Deliverable 2 of Architecture WS7: depgraph report-only summaries; upstream @statelyai/graph issues #3283 (filing the upstream @statelyai/graph issues) hasn't happened yet, so the README has nothing to point at. Dropped the parenthetical rather than leave a reference to something that doesn't exist.
  2. Test named "reports a typed error" tests a plain Error — accepted, confirmed: computeDominatorSummary throws a plain new Error(...), no typed/discriminated error involved. Renamed to build.ts fails for a dominator entry outside the graph.
  3. DEFAULT_DOMINATOR_ENTRY couples to src/daemon.ts existing — accepted as "document the coupling," not "soften the failure." Confirmed the coupling is real: CI's depgraph gate runs depgraph:test, and that suite's default-path test asserts against the default entry's own output, so renaming src/daemon.ts without updating this constant would fail CI. Didn't downgrade the failure to a warning — an unknown --dominator-entry (default or explicit) should keep failing loudly; silently warning on a bad entry would hide a real misconfiguration. Added a comment at the declaration explaining the coupling instead.

Committed in ab50b618b, verified with pnpm depgraph:test (39/39) and pnpm check:quick. Not pushing standalone per your note — batching this with the post-#3275 rebase/retarget.

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.

1 participant