Skip to content

Add StarGraph[n] and fix the generator argument-array leak - #75

Open
msollami wants to merge 16 commits into
mainfrom
md-graph-star
Open

Add StarGraph[n] and fix the generator argument-array leak#75
msollami wants to merge 16 commits into
mainfrom
md-graph-star

Conversation

@msollami

@msollami msollami commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds StarGraph[n] alongside the existing basic graph constructors, and fixes an
Expr** argument-array leak that the new function surfaced by reproducing it — the
other four generators in src/graph/generators.c had it too.

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 file's other generators already use.

Changes

  • src/graph/generators.cbuiltin_star_graph, modelled on builtin_cycle_graph.
    n = 1 is a lone vertex, n = 0 the empty graph; a non-integer or negative
    argument leaves the expression unevaluated, as with the siblings. No wrap-edge
    special case is needed, unlike CycleGraph[2], because a star has no duplicate
    edge at any size.
  • src/sym_names.{h,c}SYM_StarGraph interned.
  • src/graph/graph.hbuiltin_star_graph declared.
  • src/graph/graph.c — registered in graph_init with Protected and a docstring.
  • tests/test_graph.c — ten assertions in test_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 of
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.

leaks -atExit -- ./graph_tests drops from 715 records / 40944 bytes to
640 / 36816, with no generator frames remaining apart from RandomGraph's
separate RandomSample path (3 records), which is pre-existing and untouched here.

Testing

tests/build/graph_tests — all 17 cases pass, including the new test_generators
assertions.

Verified against the built binary:

{VertexCount[StarGraph[5]], EdgeCount[StarGraph[5]]}   (* {5, 4}                *)
EdgeList[StarGraph[4]]        (* {1 <-> 2, 1 <-> 3, 1 <-> 4}                    *)
VertexDegree[StarGraph[5]]    (* {4, 1, 1, 1, 1}                                *)
DirectedGraphQ[StarGraph[5]]  (* False                                          *)
AdjacencyMatrix[StarGraph[4]] (* {{0,1,1,1},{1,0,0,0},{1,0,0,0},{1,0,0,0}}      *)
{VertexCount[StarGraph[1]], EdgeCount[StarGraph[1]]}   (* {1, 0}                *)
{VertexCount[StarGraph[0]], EdgeCount[StarGraph[0]]}   (* {0, 0}                *)
ConnectedGraphQ[StarGraph[6]]                (* True — a star is its own tree   *)
EdgeCount[FindSpanningTree[StarGraph[6]]]    (* 5                               *)
Count[GraphPlot[StarGraph[5]], _Line, Infinity]        (* 4                     *)
Head[StarGraph[x]]  /  Head[StarGraph[-3]]   (* StarGraph — both unevaluated    *)

Also green: make (clean, -Wall -Wextra), make check-c99,
make check-packed-aware, make check-nd-surfaces.

Not run: the full tests/build suite — it exceeded the time budget locally, so
only graph_tests was executed. Nothing in this diff touches non-graph code, but
that 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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant