Add StarGraph[n] and fix the generator argument-array leak - #75
Open
msollami wants to merge 16 commits into
Open
Add StarGraph[n] and fix the generator argument-array leak#75msollami wants to merge 16 commits into
msollami wants to merge 16 commits into
Conversation
Marketplace re-added from ms-bain/ai-sdlc-starterkit, plugin installed at v8.0.0 (gitCommitSha 6a33626d...). Ran kit-setup DETECT/PROPOSE/CONFIRM by hand (session-scoped plugin loading meant the Skill tool couldn't dispatch kit-setup directly — see KIT-FEEDBACK-GRAPH.md GR-01) and wrote .claude/VERIFICATION_LADDER.md + .claude/GUIDANCE_ROLES.md for this repo's make+CMake C99 toolchain.
Research doc concludes: extend Graph with edge weights + WeightedAdjacencyMatrix + EdgeWeight[g], the extension the code itself flags as pre-approved future work (src/graph/adjmat.c:9). HyperGraph does not exist and is explicitly locked out of MVP scope (docs/spec/builtins/graphs.md:19-21), so it is excluded rather than built from scratch. Weighted shortest-path deferred to a follow-up per explicit scope decision.
…eights Plan: Graph[v,e,EdgeWeight->w] + EdgeWeight[g] + WeightedAdjacencyMatrix[g]. plan-reviewer caught a real blocking bug (graph_build_adj is a second, independent validation choke point that 8 of 27 builtins route through, bypassing graph_is_valid entirely -- the first draft would have shipped those 8 builtins silently broken on any weighted graph). Verified and fixed. Plan approved.
Graph[v, e, EdgeWeight -> {w1, ..., wm}] accepts an optional 3rd
constructor argument attaching a weight to each edge, matched by position;
a length mismatch is malformed and left unevaluated. Two new builtins:
EdgeWeight[g] (weights in EdgeList order, defaulting to all 1s when
unweighted) and WeightedAdjacencyMatrix[g] (like AdjacencyMatrix but filled
with edge weights; identical to AdjacencyMatrix for an unweighted graph).
Both graph_is_valid and graph_build_adj -- two independent validation
choke points, not one -- now share a graph_shape_ok helper, so the 8
builtins routed through graph_build_adj (ConnectedComponents,
WeaklyConnectedComponents, ConnectedGraphQ, VertexConnectivity,
FindSpanningTree, FindShortestPath, GraphDistance) keep working on a
weighted graph instead of silently rejecting it while GraphQ reports the
graph valid.
Fully additive: unweighted Graph[v,e] and all pre-existing builtins are
unchanged. No packed/NDArray or Compile[] surface, consistent with
AdjacencyMatrix/IncidenceMatrix (structural over a Graph tree, not
elementwise over a numeric buffer). Weighted shortest-path/distance and
derived-vertex weighted construction remain out of scope.
make check-c99 and make check-packed-aware both pass with no new findings;
tests/test_graph.c gains test_edge_weights covering every acceptance
criterion, including a regression test for the graph_build_adj choke
point.
GR-08 (toolchain SDKROOT gap), GR-11 (tests/CMakeLists.txt explicit file list vs the plan's wildcard claim), GR-12 (confirmation-provenance: 'confirmed with the maintainer' overclaims a single AskUserQuestion accept of the recommended option), GR-13 (verification-ladder unit rung failed on an unrelated pre-existing flaky test, not this change), GR-14 (static-first-review examined zero lines of the actual C99 codebase, reported blocking anyway, never flagged the language as unhandled -- same root-only-manifest bug as GR-03, independently reimplemented a third time in kit_languages.py).
Ticket 2: make FindShortestPath/GraphDistance weight-aware (Dijkstra) when EdgeWeight is present and non-negative numeric, falling back to the existing BFS otherwise -- the explicit follow-up ticket 1's own Non-goals named. Local weighted adjacency, no change to the shared GraphAdj structure (direct lesson from ticket 1's plan-reviewer finding). GR-15: mid-session, the upstream ais repo moved to 8.1.3 (confirmed via a fresh clone, not taken on a peer session's word) and independently fixed two of this journal's findings (GR-03's bare-Makefile detection miss, GR-12's confirmation-provenance overclaim) the same day. Annotated status against the live diff rather than editing the original 8.0.0-era entries.
Two BLOCKING findings fixed: (1) double dist[] had no path back to an exact Expr, would have failed AC-2's exact-integer expectation; (2) the plan wrongly claimed both FindShortestPath and GraphDistance test assertions needed updating, when the specific AC-11 test graph has only one path so only GraphDistance's changes. Two WORTH FLAGGING: omitted EXPR_MPFR from the weight-usability gate (should reuse expr_is_numeric_like directly), and a builtin-count error (5 not 8) inherited from ticket 1's own already-shipped plan.
FindShortestPath[g,s,t] and GraphDistance[g,s,t] dispatch to Dijkstra over a local, call-scoped weighted adjacency (not the shared GraphAdj -- kept separate to avoid widening a structure 5 other builtins depend on) when g carries a non-negative-numeric EdgeWeight; falls back to the existing unweighted BFS in every other case (absent, symbolic, or negative weight), so no previously-working call regresses. GraphDistance returns an exact Integer/Rational whenever inputs are exact: the internal double-based Dijkstra selects the path only, never the returned value, which is reconstructed via Plus[] over the real edge weights along the discovered path -- fixing a real defect an adversarial plan-review pass caught before implementation (a raw double accumulator would have printed an inexact 12. against an exact 12 expectation). tests/test_graph.c gains test_weighted_shortest_path covering every acceptance criterion; the one existing GraphDistance regression assertion from the prior edge-weights ticket is corrected to the new weighted value (FindShortestPath's assertion is unaffected -- that specific test graph has only one path, so BFS and Dijkstra already agreed on it).
… summary Verified: all 7 acceptance criteria pass against the live REPL, exact-value AC-2 check confirmed (Integer not Real), make check-c99/check-packed-aware both green, graph_tests (17 tests) passes standalone. Verification ladder's unit rung fails for the same pre-existing unrelated reason as GR-13, not a regression. GR-17: what recurred across two independent RPI passes (plan-reviewer catching real defects both times, the Decisions word-cap overage, GR-01's plugin-cache friction) versus what was ticket-1-specific (CONFIG.md gap, a genuine grill-me question) versus what was new (GR-15's mid-session version drift; ticket 1's own shipped plan turning out to have a factual error -- 5 vs 8 builtins -- that survived its own plan-review pass).
Precise, self-contained repro write-ups per a peer request: exact commands typed, exact output, the precise point of failure with file:line, and an honest separation of what was verified (the workaround that was actually used) from what was only inferred from the kit's own documentation (whether /reload-plugins, a restart, or a reinstall would fix GR-01 -- none of the three were empirically testable from within this session).
Real artifacts, not a reconstruction: the ticket as it started, the research doc in full, the plan before/after the plan-reviewer's pass (two BLOCKING findings verbatim, the exact fixes applied), the implementation diff, every acceptance criterion with its real REPL output, and an honest per-stage cost accounting -- including a genuine discrepancy caught while writing it (a commit-timestamp bracket that contradicted the review agent's own self-reported duration), reported rather than resolved by picking whichever number looked better. GR-18 in the feedback journal.
…, GR-20 (re-check 18 findings against 9.0.7) GR-19: corrected a wrong categorical claim I made to a peer about cross- session mechanisms being architecturally impossible -- Michael's own ORCHESTRATOR_HANDOFF.md documents a real, authorized AppleScript/System Events keystroke-injection mechanism for exactly this. Verified the mechanism is live on this machine; did not verify which mechanism resolved this specific approval, and said so. GR-20: fresh clone of the real upstream repo (independently confirmed 9.0.7, 1243 tests), each findable finding checked against live source or by execution rather than trusted from a summary. GR-03/GR-12 fixed; GR-01 correctly documented as an unfixable-at-this-layer limitation (quoting the kit's own README, which now describes this session's exact failure); GR-10 only partially fixed (the exact parenthetical-clarifier repro this session hit still misclassifies, verified by running it); GR-05 still open with no evidence of change; GR-13 reclassified as not a kit defect at all, since it was this session's own configured command.
…ecked Four of five items verified accurate and current: handoff citations still unverified (rewording away from the unlocatable 'nine of nine' anecdote confirmed landed correctly -- the room won't hear it asserted), the provenance vocabulary confirmed present in all three named files, verify-implementation's empty-diff-range gap still open and its demo substitute corroborated by this session's own unrelated work tonight, telemetry's scope limit stated plainly in AGENTIC_LADDER.md, and the docs-site detector bug still deliberately pinned rather than fixed (though its count grew from 2 to 5 false positives in one day, which the current phrasing doesn't convey). A fifth item not in the original four-item summary (idea-stage fabrication) is real, current, and by this session's own judgment the most severe item on the list. Added judgment on what the list is missing from this session's own 20+ findings: the kit's total inoperability for an autonomous agent mid-session (not just the /reload-plugins footnote); this afternoon's own Session-1-rehearsal finding of a confidently-wrong Makefile-detection proposal, the same failure shape as item 5 one stage earlier in the pipeline; and a broader 'assertions nothing verifies' recurrence in the plan-review pipeline with no citation involved at all (the 8-vs-5-builtins finding), which item 1's citation-only framing doesn't cover.
StarGraph[n] gives the star on vertices 1..n: the hub 1 joined to each of the n-1 leaves 2..n, undirected. Hub vertex 1 matches Mathematica and the int_vertices() convention the other generators in this file already use. n = 1 is a lone vertex, n = 0 the empty graph, and a non-integer or negative argument leaves the expression unevaluated, as with the siblings. Wired the usual way: SYM_StarGraph interned in sym_names, builtin_star_graph declared in graph.h, registered in graph_init with Protected and a docstring. Documented in docs/spec/builtins/graphs.md and the week's changelog. Ten assertions in tests/test_graph.c cover the counts, EdgeList, degree sequence, undirectedness, both degenerate sizes, connectivity, the spanning-tree edge count, and the symbolic-argument case. Separately, this fixes a leak the new function surfaced by reproducing it. expr_new_function() memcpy's the argument array rather than taking it, so the calloc'd Expr** is the caller's to free -- and of the five generators only RandomGraph did. The shared make_graph helper now owns both arrays via a new make_list_owning(), which covers CompleteGraph, CycleGraph, PathGraph and StarGraph in one place; RandomGraph's inline vertex list uses it too. graph_tests drops from 715 leak records to 640, with no generator frames remaining apart from RandomGraph's separate RandomSample path.
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
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
Adds
StarGraph[n]alongside the existing basic graph constructors, and fixes anExpr**argument-array leak that the new function surfaced by reproducing it — theother four generators in
src/graph/generators.chad it too.StarGraph[n]gives the star on vertices1..n: the hub1joined to each of then-1leaves2..n, undirected. Hub vertex1matches Mathematica and theint_vertices()convention the file's other generators already use.Changes
src/graph/generators.c—builtin_star_graph, modelled onbuiltin_cycle_graph.n = 1is a lone vertex,n = 0the empty graph; a non-integer or negativeargument leaves the expression unevaluated, as with the siblings. No wrap-edge
special case is needed, unlike
CycleGraph[2], because a star has no duplicateedge at any size.
src/sym_names.{h,c}—SYM_StarGraphinterned.src/graph/graph.h—builtin_star_graphdeclared.src/graph/graph.c— registered ingraph_initwithProtectedand a docstring.tests/test_graph.c— ten assertions intest_generators.docs/spec/builtins/graphs.md,docs/spec/changelog/2026-08-31.md,Mathilda_spec.md— reference, changelog entry, changelog table row.The leak fix
expr_new_function()memcpy's the argument array rather than taking ownership ofit, so the
calloc'dExpr**is the caller's to free — and of the five generatorsonly
RandomGraphdid. The sharedmake_graphhelper now owns both arrays via anew
make_list_owning(), which coversCompleteGraph,CycleGraph,PathGraphand
StarGraphin one place;RandomGraph's inline vertex list uses it too.leaks -atExit -- ./graph_testsdrops from 715 records / 40944 bytes to640 / 36816, with no generator frames remaining apart from
RandomGraph'sseparate
RandomSamplepath (3 records), which is pre-existing and untouched here.Testing
tests/build/graph_tests— all 17 cases pass, including the newtest_generatorsassertions.
Verified against the built binary:
Also green:
make(clean,-Wall -Wextra),make check-c99,make check-packed-aware,make check-nd-surfaces.Not run: the full
tests/buildsuite — it exceeded the time budget locally, soonly
graph_testswas executed. Nothing in this diff touches non-graph code, butthat is an argument rather than a measurement. No packed/NDArray or
Compile[]surfaces apply: this is a structural constructor returning a
Graph[...]expression, not a numeric head.
JIRA Ticket
n/a