diff --git a/.claude/GUIDANCE_ROLES.md b/.claude/GUIDANCE_ROLES.md new file mode 100644 index 000000000..5fc71d72e --- /dev/null +++ b/.claude/GUIDANCE_ROLES.md @@ -0,0 +1,60 @@ +# Guidance roles + +Some skills ask for org-supplied guidance documents by name — a security baseline, an +engineering-principles file, PR-review conventions — because that content is genuinely +yours to write, not something a generic kit can ship. + +Skills never hardcode a path to your document. They read this file for a named **role** +and resolve it here instead. Fill in the ones your team has; leave the rest +`not-configured`. + +Written by `/setup-kit` on 2026-08-22. DETECT (`kit-setup/scripts/detect_guidance_roles.py`) +found one hit — root `AGENTS.md` — and no signal for the other five; confirmed with the +human rather than guessed. + +## security-baseline + +``` +security-baseline = not-configured +``` + +## multi-cloud-guidelines + +``` +multi-cloud-guidelines = not-configured +``` + +## documents-and-data-guidelines + +``` +documents-and-data-guidelines = not-configured +``` + +## pr-and-review-guidelines + +``` +pr-and-review-guidelines = not-configured +``` + +## agents-guide + +``` +agents-guide = AGENTS.md +``` + +Root `AGENTS.md` documents the code-review-graph MCP tool usage convention for this repo. + +## pr-template + +``` +pr-template = not-configured +``` + +Not actually a gap — `/describe-pr` discovers its own template; this role has no effect on +its behavior. + +## code-review-checklist + +``` +code-review-checklist = not-configured +``` diff --git a/.claude/VERIFICATION_LADDER.md b/.claude/VERIFICATION_LADDER.md new file mode 100644 index 000000000..3f42b9922 --- /dev/null +++ b/.claude/VERIFICATION_LADDER.md @@ -0,0 +1,56 @@ +# Verification ladder configuration + +One file, read by `skills/verification-ladder/scripts/ladder.py`. Change a value here instead +of editing that script. + +Written by `/setup-kit` on 2026-08-22. DETECT (`kit-setup/scripts/detect_ladder.py`) found +**zero signal for all four phases** against this repo: `CMakeLists.txt` lives at +`tests/CMakeLists.txt`, not repo root (the detector only checks the root), and there is no +`c-cmake`/`make` toolchain entry that recognizes a hand-written top-level `makefile` at all — +so a `make ...`-only CI (`.github/workflows/build.yml`) produced no CI-marker hits either. +Every value below comes from `SPEC.md` §9 and `.github/workflows/build.yml`, confirmed with +the human directly, not from DETECT. + +## Static analysis + +``` +static = make check-c99 && make check-packed-aware +``` + +`make check-c99` (`tools/check_c99_portability.py`) flags POSIX-only symbols glibc hides +under `-std=c99` (missing `#ifndef` guards on `` constants, missing feature-test +macros on POSIX functions) — this repo's actual lint tier, and the one CI runs first +(`.github/workflows/build.yml:73`). `make check-packed-aware` is a static structural audit: +diffs the NDArray dispatch sites in source against `src/pack.c`'s `AWARE` list. + +## Type checking + +``` +typecheck = make -j$(nproc) +``` + +C99 has no separate type-check phase from compilation — `SPEC.md`'s own words: "for +C/C++/Go/Java, this is often just 'does it build.'" `gcc -std=c99 -Wall -Wextra` +(`-Werror` on several diagnostics GCC 14 promotes) is the closest equivalent. + +## Unit tests + +``` +unit = cd tests && mkdir -p build && cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j$(nproc) && for t in *_tests; do ./$t || exit 1; done +``` + +The CMake-built `test_*.c` suite under `tests/` (SPEC.md §9). + +## Integration tests + +``` +integration = make check-nd-surfaces +``` + +Runs the built `./Mathilda` binary over probe expressions across three representations +(plain `List`, packed `List`, visible `NDArray`) and requires them to agree — components +(evaluator, pack transparency gate, ND kernels) exercised together, with real execution, not +isolated unit logic. `valgrind --leak-check=full ./Mathilda` (SPEC.md §4) is real practice +here but was left out of this phase: it needs a script argument with no single canonical +choice, so scripting it as the ladder's integration command would be a guess, not a +confirmed answer. diff --git a/KIT-FEEDBACK-GRAPH.md b/KIT-FEEDBACK-GRAPH.md new file mode 100644 index 000000000..b0581a0e7 --- /dev/null +++ b/KIT-FEEDBACK-GRAPH.md @@ -0,0 +1,1026 @@ +# KIT-FEEDBACK-GRAPH.md + +Dogfooding the `ais` (AI-SDLC Starter Kit) plugin on a real Graph/HyperGraph feature in +Mathilda (C99 / CMake, no Python in the target repo). Written live, not reconstructed. + +Legend: `[+]` worked, `[-]` friction, `[!]` misleading (looked right, wasn't), `[?]` ambiguous. + +**Session facts** +- Plugin: `ais@ais`, version **8.0.0** +- Marketplace source: `https://github.com/ms-bain/ai-sdlc-starterkit.git` (branch `main`) +- `gitCommitSha`: `6a33626d600c28c60c930386b1e9a93212873592` +- Installed: 2026-08-22T23:38:42.964Z, scope `user` + +**Code actually exercised, exactly**: every GR-01 through GR-14 finding below was produced +by reading and running files under `~/.claude/plugins/cache/ais/ais/8.0.0/` (the installed +plugin cache at the pinned commit above) — never `${CLAUDE_PLUGIN_ROOT}` resolved live in +this session, since the `ais` skills/commands were never dispatchable via the Skill tool +(GR-01). GR-15 onward (below) additionally cross-checks against a fresh clone of +`ms-bain/ai-sdlc-starterkit`'s `main` at commit `c0340fa46fd4683fee305c8e5c500c19b9194c3c` +(`.claude-plugin/plugin.json` reports **8.1.3**) — the working tree had moved three patch +releases past the pinned install *during this same session*, independently confirmed by a +fresh `git clone` and diff, not taken on a peer's word. See GR-15 for what that changes. + +--- + +## GR-01 [!] `/plugin update ais` + `/reload-plugins` — the exact failure the kit's own +CHANGELOG warns about, hit from the *agent* side this time + +**What I ran.** Per the task brief: find the marketplace, `/plugin update ais`, +`/reload-plugins`, confirm v8.0.0. + +There was no `ais` marketplace registered at all (`claude plugin marketplace list` showed +only `claude-plugins-official`, `jeffh-claude-plugins`, `claude-code-warp`) — despite a +stale local cache at `~/.claude/plugins/cache/ais/ais/{1..4}.0.0`, each stamped with an +`.orphaned_at` marker. This alone matches `CHANGELOG.md`'s own 4.0.0 entry almost verbatim: + +> "the local marketplace clone (`~/.claude/plugins/marketplaces/ais`) was found pinned at +> a commit from before this session's work began, despite tracking the same origin." + +i.e. this exact class of staleness bug is *already documented by the kit as having bitten +testers before*, and I hit a variant of it (no registration at all, not just staleness) on +a completely fresh attempt. Recovered the source repo (`ms-bain/ai-sdlc-starterkit`) only +by grepping old session JSONL transcripts for a prior `plugin marketplace add` invocation — +there is no in-repo, in-kit pointer to "where do I get this from" once the marketplace has +fallen out of `known_marketplaces.json`. `gh auth` also had two accounts configured +(`msollami` active, `ms-bain` the one with actual repo access) — a pre-existing friction +([[mathilda-scaffolding-friction]] memory), not the kit's fault, but it cost real time +before the marketplace add would even authenticate. + +**What I expected.** `claude plugin update ais` to update the plugin; `/reload-plugins` to +make the update live in this session. + +**What happened.** +1. `claude plugin update ais` → `✘ Failed to update plugin "ais": Plugin "ais" not found`. + Needed the fully-qualified `claude plugin update ais@ais` to work at all. The bare-name + form is what a human would type first, and it fails with a message that reads like "you + don't have this plugin" rather than "you need `@marketplace`" — misleading, not just + terse. +2. `/reload-plugins` **does not exist as anything I can invoke.** It is not a + `claude plugin` CLI subcommand (`claude plugin --help` lists `install/enable/disable/ + list/marketplace/eval/details/init` — no `reload`). It is not exposed as a Skill-tool + name. A freshly spawned sub-agent (new process context) still could not see any + `ais`-provided skill (`kit-setup`, `research-codebase`, `create-plan`, ... all absent) + even *after* the plugin showed as installed+enabled in `claude plugin list`. +3. The kit's own `README.md:158-170` documents this precisely and even names the failure + mode: *"Skipping step 3 is the quiet failure mode: `/plugin update` can report success + while the current session keeps running the pre-update code until it restarts or + `/reload-plugins` runs."* So the kit is self-aware about this exact trap — but + `/reload-plugins` turns out to be an **interactive-REPL-only** built-in with no + headless/agent-invocable equivalent. An autonomous agent (as opposed to a human typing + at a prompt) has no tool that performs step 3. This is a real gap: the kit's update + instructions assume a human is present at the terminal to run the last step. + +**Where this comes from.** `~/.claude/plugins/cache/ais/ais/8.0.0/README.md:146-170` (the +"Updating" section, steps 1-3), and `CHANGELOG.md`'s 4.0.0 entry (the prior staleness +incident, structurally the same bug class). + +**Why it matters.** This is precisely the failure class the task brief called out — "every +prior dogfood run of this kit was silently done on a stale version." The kit *fixed* the +marketplace-pinning half of that bug in 4.0.0, but the update flow still has a step that +only a human, not an agent, can execute. I verified I was on 8.0.0 by reading +`installed_plugins.json`/`claude plugin list` directly (`gitCommitSha` above) rather than +by successfully running `/reload-plugins` — i.e. I can *prove* the binary-on-disk version, +but I could not get this **session's** command dispatch to pick up any `ais`-provided +skill. Everything below that needed an `ais` slash command (`/setup-kit`, +`/research-codebase`, `/create-plan`, `/implement-plan`, `/verify-implementation`) was +executed by **reading the corresponding file under +`~/.claude/plugins/cache/ais/ais/8.0.0/commands/*.md` and following its instructions +directly**, since a slash command is just that markdown loaded as a prompt. This is +noted at each phase below; it is the single biggest asterisk on this entire dogfood run, +and it is not something I could route around by trying harder — it needed a human at an +actual terminal. + +## GR-02 [+] Duplicate command files, hyphen vs underscore — deliberate, and handled well + +`commands/` ships **both** `create-plan.md` and `create_plan.md` (also +research/implement/describe-pr/create-worktree/create-handoff/resume-handoff pairs). +First glance looked like abandoned cruft (GR-02 originally filed as `[?]`), but +`create_plan.md` (18 lines total) is a deliberate deprecation stub, not a stale copy: + +> "Refuse to proceed under the old name — do not just forward. [...] a REAL BUG, fixed +> 2026-08-22 (fourth stress test): `hooks/open_questions_gate.py` only gates a prompt +> whose text its own pattern recognizes, and this alias's prompt is not one of them — +> proceeding inline, as if `/create-plan` had actually been invoked, would silently skip +> the gate that command exists to enforce." + +So the underscore commands were renamed to hyphenated form to match skill/agent naming +convention, and rather than silently forwarding (which would bypass a text-pattern-matched +hook gate), the alias explicitly refuses and tells the caller to use the new name. That's +the correct fix for a subtle class of bug — a naming migration that could have silently +neutered a security-relevant gate — and it's a good example of the kit documenting its own +past mistake inline where the next reader will actually see it, rather than only in +CHANGELOG.md. Confirmed via `diff commands/create-plan.md commands/create_plan.md` +(849 diff lines — alias is a thin stub, not a parallel copy) and reading +`commands/create_plan.md:1-18` in full. + +--- + +## `/setup-kit` run + +Executed by reading `commands/setup-kit.md` and `skills/kit-setup/SKILL.md` directly (see +GR-01 — the Skill tool could not dispatch `kit-setup` in this session) and following the +DETECT → PROPOSE → INTERVIEW → CONFIRM method by hand. + +## GR-03 [-] `detect_ladder.py` returns literal zero signal on a real, fully-documented C99 +toolchain — not because the repo lacks a verification story, but because the detector has +no concept of a bare Makefile + +**What I ran.** +``` +python3 skills/kit-setup/scripts/detect_ladder.py --repo . --existing .claude/VERIFICATION_LADDER.md --json +``` + +**What I expected.** Some signal — this repo's CI (`.github/workflows/build.yml`) runs +`make check-c99`, `make check-packed-aware`, `make -j$(nproc)`, and the primary build is a +hand-written `makefile` per `SPEC.md` §9. Even a partial/low-confidence hit seemed likely. + +**What happened.** `toolchains_detected: []`, `manifests: {}`, `ci_commands: {}`, and every +one of the four phases came back `"top_proposal": null` — a total, across-the-board miss, +not a thin one. Root-caused by reading the script itself +(`skills/kit-setup/scripts/detect_ladder.py:99-141`): + +- `MANIFEST_SIGNALS["c-cmake"] = ["CMakeLists.txt"]`, checked only at repo root + (`detect_manifests`, `(repo_root / p).is_file()` — no recursive search). This repo's only + `CMakeLists.txt` is at `tests/CMakeLists.txt`; the actual top-level build is the plain + `makefile`, which is not itself a manifest signal for anything. +- `CI_MARKERS["c-cmake"] = ["cmake", "ctest", "clang-tidy", "cppcheck"]` — none of these + strings appear anywhere in `.github/workflows/build.yml` (confirmed by grep), because CI + only exercises the `make`-based build, never the CMake test suite. There is **no + toolchain key for a bare Makefile at all** anywhere in `MANIFEST_SIGNALS`, `CI_MARKERS`, + `PHASE_MARKERS`, or `TOOLCHAIN_PROPOSALS` — `c-cmake` is the only C/C++ entry, and it's + keyed entirely on CMake-specific signals. + +**Why it matters.** The task brief for this session specifically called out this repo as +"a real test of the toolchain-agnostic verification ladder" — and the honest result is that +for a plain-Makefile C project (extremely common for C/C++, and exactly what this repo's own +`SPEC.md` §9 documents as canonical: `make -j$(nproc)`, `make check-c99`, +`cd tests && cmake ... && for t in *_tests; do ./$t; done`), the "toolchain-agnostic" +detector's toolchain table has no path to a positive result. The tool's own fallback design +is correct in spirit — "no signal — needs your answer" beats a wrong guess — but the gap is +real: it's not that this repo is *ambiguous* (it has one unambiguous build system and one +unambiguous test runner, both named in its own top-level docs), it's that the detector never +looks. A `Makefile`/`makefile` manifest signal name (distinct from `c-cmake`) and `make `/ +`make -j`/bare target-name CI markers would have caught real signal here with no guessing +required. Not fixed as part of this dogfood run — flagged as the finding, per the task +brief's own framing of this repo as the toolchain-agnostic-ladder stress test. + +**CONFIRM.** Ran the CONFIRM step as an `AskUserQuestion` (since `kit-setup` is explicit that +writing either config file without an explicit human confirmation is an anti-pattern, and I +am acting as the human's proxy, not skipping the gate) proposing a ladder derived from +`SPEC.md` §9 + the actual CI file rather than from DETECT (which had nothing to offer). User +confirmed the full four-phase version. Wrote `.claude/VERIFICATION_LADDER.md` and +`.claude/GUIDANCE_ROLES.md` (the latter came back with one correct hit — root `AGENTS.md` +for the `agents-guide` role — and no signal for the other five, all confirmed +`not-configured`). + +## GR-04 [+] "does it build" framing for `typecheck` on a compiled-language repo + +`VERIFICATION_LADDER.example.md`'s own commentary — *"Not every toolchain has a separate +type-check step from compilation — for C/C++/Go/Java, this is often just 'does it +build.'"* — is exactly right and made the CONFIRM proposal easy to write correctly +(`typecheck = make -j$(nproc)`, i.e. strict `-std=c99 -Wall -Wextra` compilation, functions +as this repo's type-check tier). Small thing, but it's the kind of toolchain-specific +nuance that's easy to get wrong (e.g. leaving `typecheck` as `not-configured` for every +compiled language, which would undercount real verification coverage) and the example file +gets it right unprompted. + +## GR-05 [-] `/setup-kit` doesn't create `.claude/CONFIG.md`, but `/research-codebase`'s +very first instruction requires it + +**What I ran.** Started `/research-codebase` (by reading `commands/research-codebase.md` +directly — see GR-01) whose line 14 says: *"Read `.claude/CONFIG.md` for `NOTES_DIR`... +before touching any path below."* + +**What I expected.** Having just run `/setup-kit`, I expected the two files it's documented +to configure to be the complete "you're set up now" state. + +**What happened.** `.claude/CONFIG.md` does not exist in this repo (confirmed: `ls +.claude/CONFIG.md` → no such file), and `/setup-kit`'s own scope (`commands/setup-kit.md`, +`skills/kit-setup/SKILL.md`) never mentions `CONFIG.md` at all — it only writes +`VERIFICATION_LADDER.md` and `GUIDANCE_ROLES.md`. `CONFIG.example.md:6-8` documents the +default (`NOTES_DIR = thoughts/`) for exactly this case, so the fallback is discoverable — +but only by a reader who thinks to open `CONFIG.example.md` instead of `CONFIG.md`. A repo +that has genuinely run the kit's own recommended setup command is still one config file +short of what the very next command in the pipeline (`/research-codebase`) asks for first, +with no automated link between the two. Proceeded with the documented default +(`NOTES_DIR = thoughts/`). + +## GR-06 [?] `grill-me`'s "strictly one question at a time" instruction sits awkwardly +against the harness's native multi-select question tool + +`skills/grill-me/SKILL.md` (research-open mode) is explicit: *"Strictly one question at a +time, every mode, no exceptions... emitting a numbered list and waiting for a batch answer +defeats the entire point of asking interactively."* The interactive-question tool this +harness actually exposes (`AskUserQuestion`) is built around asking 1-4 questions per call +with multiple-choice options — it's the more natural, idiomatic way to surface a choice, and +batching up to 4 is clearly an intended, first-class use of that tool elsewhere in this same +session (e.g. the kit-setup CONFIRM step above). Honored the skill's instruction literally +here — asked exactly one question ("is there prior context / a constraint / a prior attempt +I should know about before researching Graph/HyperGraph?") — but it's a real design seam: a +kit instruction written before this tool's batching affordance existed, now sitting next to +a tool that actively invites the pattern it warns against. Not a bug, but worth flagging: +nothing enforces the one-at-a-time rule mechanically (unlike the plan section-contract, +which has `hooks/open_questions_gate.py` as a backstop) — it only holds if the agent +following the skill remembers to resist the batching affordance. + +--- + +## `/research-codebase` run + +Read `commands/research-codebase.md` in full (see GR-01) and followed it by hand: grill-me +research-open (one question, GR-06), no subsystem docs existed yet to fold in (none exist — +noted per the command's own instruction to say so and move on), one `codebase-explorer` +sub-agent dispatched for the full 7-part investigation (builtin inventory, locked-scope +callouts, test coverage, downstream consumers, packed/NDArray applicability, git history, +HyperGraph existence), then research-close synthesis and the two-document output +(`thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md` + `-summary.md`). + +## GR-07 [+] The research template's forced structure caught a real scope trap before any +code was written + +The two-document template (long doc with frontmatter + `## Open Questions` in fixed +`### Unresolved`/`### Resolved` sub-headings, short doc with `## Options Considered` / +`## Decision Criteria`) is not just formatting — filling in "Options Considered" honestly +surfaced that the obvious literal reading of the task ("extend Graph **and HyperGraph**") +would have meant building a feature the codebase's own spec document +(`docs/spec/builtins/graphs.md:19-21`) explicitly locks out of MVP scope, sized at a week +rather than the requested few hours. Writing "Decision Criteria" as its own section (rather +than folding the reasoning into prose elsewhere) forced an explicit "must not contradict a +locked scope decision" criterion onto the page, which is what disqualified HyperGraph in +writing rather than by gut feel. The `### Unresolved` / `### Resolved` split similarly +forced a real scope question (weighted `FindShortestPath`?) into a checkbox instead of +letting it get silently rolled into "yes, do the whole thing" — it surfaced, got asked via +`AskUserQuestion`, and got a real "no, defer" answer with a reason attached, all before any +`create-plan` work started. Credit to the template design, not to anything I'd have +necessarily done unprompted. + +## GR-08 [?] A pre-existing local toolchain gap (not the kit's fault) surfaced only once +`/implement-plan` started running the `typecheck` rung I had configured + +**What happened.** `make -j$(nproc)` (the exact command I wrote into +`.claude/VERIFICATION_LADDER.md`'s `typecheck` phase during `/setup-kit`, copied verbatim +from `SPEC.md` §9) failed immediately with `fatal error: stdio.h: No such file or +directory` — Homebrew `gcc-16`'s default include search path on this machine does not +contain the macOS SDK's system headers at all (confirmed: `echo | gcc-16 -E -x c - -v` +lists only GCC's own bundled include dirs). Root cause: this machine has no `/usr/include` +symlink and no `SDKROOT`/`CPATH` set. Fix: `export SDKROOT=$(xcrun --show-sdk-path)` before +every build command for the rest of the session. + +**Why it belongs in this journal even though it isn't a kit bug.** This is exactly the kind +of thing `/setup-kit`'s DETECT step cannot see and the verification ladder's own commentary +doesn't warn about: a `typecheck` phase that is "just: does it build" can fail for a reason +that has nothing to do with the code under review and everything to do with an +unconfigured local toolchain — and a mechanical ladder run (item THREE below) reports that +failure with the same "FAILED" vocabulary it would use for a real compile error in the +diff, with no signal to a reader that this is environment noise rather than a regression. +`skills/verification-ladder/scripts/ladder.py` has no mechanism to distinguish "this +command failed because of what changed" from "this command failed because of what machine +it ran on" — that judgment call is left entirely to whoever reads the ladder's raw output. + +--- + +## `/create-plan` run + +Read `commands/create-plan.md` in full (833 lines — see GR-01) and followed it by hand: +reused the existing research doc per step 2c (no re-spawned locator/analyzer sweep), a +single plan-open confirmation question via `AskUserQuestion`, wrote +`thoughts/shared/plans/2026-08-22-graph-edge-weights.md` + `-summary.md` against the full +template (TL;DR/Overview/Decisions/Non-goals/Acceptance Criteria/Open Questions/Plan +Review/Architecture Impact/... through the phased implementation sections), ran +`skills/grill-me/scripts/check_plan_contract.py` (real, mechanical — caught a real word-cap +overage on first pass, see GR-10), then dispatched a `plan-reviewer`-briefed sub-agent. + +## GR-09 [+] The unconditional `plan-reviewer` pass caught a real, verified, ship-blocking +bug in the plan before any code was written — this is the single best result of this entire +dogfood run + +**What I ran.** Per `commands/create-plan.md:554-596` ("Run a `plan-reviewer` pass... +unconditionally, not as an offer"), dispatched a sub-agent fully briefed with +`agents/plan-reviewer.md`'s nine-area rubric, told to use the plan-artifact lens rotation +(scope-boundary + testability), and to verify every file:line citation in my draft plan +against the actual source. + +**What I expected.** Maybe a style nit, an under-specified acceptance criterion — the kind of +thing a review pass usually finds on a plan I'd already convinced myself was solid. + +**What happened.** It found a real, load-bearing, verified BLOCKING defect. My plan's own +"Key Discoveries" section asserted: *"`graph_is_valid` is the single choke point essentially +every other builtin routes through... widening it once... is what makes every existing +(unmodified) builtin continue to work unchanged against a weighted graph."* This is false. +`src/graph/graph_util.c:206-209` (`graph_build_adj`) is a **second, entirely independent** +validation entry point with its own hardcoded `arg_count != 2` rejection — it does not call +`graph_is_valid` at all (its own comment even explains why: avoiding a redundant vertex-index +build). Eight of the 27 graph builtins (`ConnectedComponents`, `WeaklyConnectedComponents`, +`ConnectedGraphQ`, `VertexConnectivity`, `FindSpanningTree`, `FindShortestPath`, +`GraphDistance`) route through `graph_build_adj`, not `graph_is_valid`. As I'd originally +scoped the plan (widening only `graph_is_valid`), every one of those 8 builtins would have +shipped **silently broken** on any weighted graph: `GraphQ[g]` would report `True`, but +`FindShortestPath[g, ...]` etc. would all return unevaluated — directly contradicting my own +plan's Overview claim ("no other builtin's behavior changes"). None of my original ten +Acceptance Criteria rows would have caught this, because none of them exercised a +`graph_build_adj`-routed builtin against a weighted graph — **it would have shipped green.** + +I verified the finding myself before accepting it (`grep -rn "graph_build_adj" src/graph/*.c` +and read `graph_util.c:195-230` directly) — it was exactly right, down to the specific line +numbers and the exact list of 8 affected builtins. + +**Where this comes from.** `commands/create-plan.md:554-556` (the unconditional-review +instruction) and `agents/plan-reviewer.md`'s rubric areas 2 ("Hidden assumptions") and 1 +("Unsupported claims") — the finding was reported as exactly that: an unsupported/false +architectural claim the rest of the plan was built on. + +**Why it matters.** This is precisely the class of failure the task brief asked me to hunt +for — something that "looked right and was not." My plan read as complete, well-cited, and +confident; the false claim was a single sentence buried in "Key Discoveries" that I had no +reason to doubt because it matched the shape of the one file (`adjmat.c`) I'd used as my +template. A second, independent choke point in a sibling file (`graph_util.c`, not even +`adjmat.c`) is exactly the kind of thing a single-author plan reliably misses and a +dedicated adversarial pass reliably catches. Fixed: both choke points now widen via a shared +helper, a new AC-11 covers a `graph_build_adj`-routed builtin against a weighted graph, and +the plan's `## Plan Review` section transcribes the finding and its resolution per the kit's +own "move it, don't just discuss it" convention (`commands/create-plan.md:586-591`). Two +smaller WORTH FLAGGING findings (a wrong line citation, an unstated scope boundary on +derived-vertex weighted construction) were also real and also fixed. + +## GR-10 [+] `check_plan_contract.py` is a genuinely useful mechanical gate, with one sharp +edge + +Ran `skills/grill-me/scripts/check_plan_contract.py --plan ` twice. First run: `FAIL — +Decisions is 260 words, over the 200-word cap`, correctly caught (real overage, fixed by +tightening prose) — a good, cheap, deterministic catch that a human reviewer would have to +count words to replicate. Second finding was more of a gotcha than a bug: writing +`APIs changed: none (additive only — ...)` in the `Architecture Impact` block flipped +`determine_tier()` to `"architectural"` (the script's own comment at +`check_plan_contract.py:108-112` says it deliberately compares the *whole rest of the line*, +not an exact `none` token) — which is arguably correct behavior (a reviewer skimming should +be able to trust a bare `none`), but it means a well-intentioned clarifying parenthetical +right next to the word `none` silently changes which tier gate the plan is held to. Fixed by +moving the clarification to a footnote line below the fixed-shape block instead of inline. +Worth knowing before writing that section: keep those five lines *bare*. + +--- + +## `/implement-plan` run + +Read `commands/implement-plan.md` in full before touching code (see GR-01). Its own text is +explicit that `hooks/open_questions_gate.py` — a `UserPromptSubmit` hook — is meant to check +the Open-Questions/contract/Plan-Review gates mechanically *before this file's prose is ever +read*, and just as explicit that this only fires on a real `/implement-plan` or +`/ais:implement-plan` invocation. Since I could not dispatch the real command (GR-01), that +hook never ran at all for this session — not "ran and passed," genuinely never invoked. I +ran the documented three-check fallback by hand instead (Open Questions: `_None._` under +Unresolved; `check_plan_contract.py`: PASS; Plan Review `### Blocking`: `_None._`, the +`plan-reviewer` finding having been moved to `### Resolved`) — all three passed — but this +is worth being precise about: the "second pass, not primary enforcement" framing in +`commands/implement-plan.md:40-43` inverts exactly when the primary enforcement can't run at +all, which is systemically true for every phase of this dogfood run, not just this one. + +## GR-11 [-] `tests/CMakeLists.txt` lists graph source files explicitly; the plan's own +"no build-system edit needed" claim was only true of the top-level `makefile` + +**What happened.** The plan (correctly, and confirmed via `grep` at research time) states +`makefile:338` wildcards `src/graph/*.c`, so a new file needs no Makefile edit. True — but +`tests/CMakeLists.txt:829-847` lists every `src/graph/*.c` file **by name**, not via glob. +Building `graph_tests` after adding `src/graph/edgeweight.c` and `src/graph/wtadjmat.c` +failed at link time: `Undefined symbols: _builtin_edge_weight, +_builtin_weighted_adjacency_matrix` — the files were never compiled into the test binary at +all, and the main-binary build (which uses the wildcarded `makefile`) gave no signal of this +gap since it built and linked cleanly on its own. Fixed by adding both files to +`tests/CMakeLists.txt`'s explicit list. Not a kit-tooling finding — this is Mathilda's own +build layout — but exactly the kind of "the plan's own evidence was accurate for the file it +checked and the codebase has a second, uninspected file with the same shape of claim" +mismatch that neither `/research-codebase` nor `/create-plan`'s process caught, because +nothing in either command's checklist says "grep for every OTHER place a source-file list +might be enumerated." Logged here per the task's explicit interest in what "looked right and +was not" — the plan's claim was well-cited, accurate for its citation, and still incomplete. + +## GR-12 [!] The research artifact's own "confirmed directly with the maintainer" line +overclaims what actually happened — a real, load-bearing example of exactly the failure +class this dogfood run was asked to hunt for, and I am the one who wrote it + +**What the artifact says.** `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md`'s +`### Resolved` list (and its `-summary.md` twin) both read: *"Should `FindShortestPath`/ +`GraphDistance` gain a weighted (Dijkstra) mode in the same pass...? — No; confirmed with the +maintainer directly (`AskUserQuestion`, 2026-08-23)."* Same pattern in the `grill-me` +research-open answer ("no prior context, just research fresh") and in the `kit-setup` +CONFIRM step. + +**What actually happened, precisely.** Each of these was one `AskUserQuestion` call with a +label reading `"... (Recommended)"` on the first-listed option, and a UI event came back +selecting exactly that pre-labeled option. The tool's own response gives me the selected +label string and nothing else — no timestamp, no indication of how long the option sat +before being chosen, no signal distinguishing "read the question, weighed it, agreed" from +"saw a recommended option and pressed through it." Writing "confirmed directly with the +maintainer" — language that reads as a deliberated, substantive consultation — is not +something the interaction itself can support. It is equally consistent with the accurate, +much weaker claim "the pre-selected recommended option was accepted without a +non-default being chosen, and without any elaboration." + +**How this got into the artifact.** I wrote it that way because `research-codebase.md`'s own +template literally instructs exactly this framing (`### Resolved\n- [x] — +`) and the `grill-me` skill's own documented purpose is +"ask what only the human knows" — the whole design of that step *assumes* the answer, once +given, represents genuine human judgment, and nothing downstream ever re-examines that +assumption. I supplied the confident-sounding phrasing myself, unprompted by any kit +instruction to inflate it — but the kit gave me no vocabulary for a weaker claim either, and +no mechanical check (unlike `check_plan_contract.py`'s word-cap enforcement, or +`open_questions_gate.py`'s section-shape enforcement) that would flag "confirmed with the +maintainer" as a claim needing evidence the way `plan-reviewer`'s own rubric area 1 +("Unsupported claims") would flag it in a *finding* about someone else's document. Nothing +in this pipeline turns that same rubric on the research/plan documents' own provenance +claims about *how a Resolved item was resolved* — only on their technical assertions. + +**What would make this honest.** Two independent changes, neither large: +1. **`AskUserQuestion`-sourced answers need their own, weaker verb.** A `### Resolved` entry + whose provenance is a single `AskUserQuestion` call accepting the first (`Recommended`) + option should say exactly that — "accepted the recommended option via `AskUserQuestion`, + no elaboration given" — and reserve "confirmed with the maintainer" / "discussed with the + maintainer" for an answer that came with free text, a non-default selection, or an actual + multi-turn exchange. This is a template-language fix in `research-codebase.md` and + `create-plan.md`'s own `### Resolved` example line, not a new mechanism. +2. **`plan-reviewer`'s rubric area 2 ("Hidden assumptions") should explicitly extend to a + document's own `### Resolved` claims, not just its technical assertions.** "The human + agreed" is itself a claim that can be unsupported in exactly the sense rubric area 1 + already checks for everything else in the document — right now nothing in the nine-area + rubric is scoped to catch a document overclaiming the strength of its own human sign-off, + because "Unsupported claims" as written (`agents/plan-reviewer.md`'s example: "the service + handles retries correctly" with nothing pointing at where) is about *technical* claims, + and every existing example in that file is technical. + +This finding exists because the user explicitly asked me to look for it, not because I +caught it unprompted — worth being honest about that too. It is the same shape of failure as +GR-01's `/reload-plugins` gap: a real, meaningful distinction (deliberated consultation vs. +default-acceptance) that the tool surface cannot express, so the language written on top of +it silently rounds up to the stronger claim. + +--- + +## GR-17 [+/-] Two tickets, same loop, run twice — what recurred and what didn't + +Two full RPI passes now (research → plan → adversarial plan-review → implement → verify), +on the same subsystem, close together. What actually recurred versus what was specific to +one ticket: + +**Recurred, both tickets:** +- The `plan-reviewer` pass caught a real, load-bearing, independently-verified defect both + times (GR-09, GR-16) — not a fluke. Different failure shapes each time (a false + architectural claim about a shared function; an arithmetic-representation gap plus a + propagated miscount), which is itself the useful signal: the *value* of a fresh adversarial + pass with Read/Bash access to live source recurs, even though the specific bug class does + not. +- `check_plan_contract.py`'s word-cap on `## Decisions` was exceeded on the first draft both + times (GR-10, and the unlogged first attempt on ticket 2's plan). Given it happened twice + independently, this reads as a fact about my own drafting habit (front-loading + justification into Decisions) rather than a coincidence — a real, personal pattern the + mechanical gate is correctly catching, not a tooling flaw. +- Every RPI command had to be read from the plugin cache and followed by hand, every single + time (GR-01's root cause never resolves mid-session) — the single most consistently + recurring friction in this entire log. + +**Did not recur (ticket-1-specific):** +- No `.claude/CONFIG.md` gap surfaced again for ticket 2 (GR-05) — once `NOTES_DIR`'s default + was established, every subsequent research/plan doc just used it without re-deriving + anything. +- No `grill-me` research-open interactive question was even asked for ticket 2 — I judged + (per the user's explicit "keep going, don't wait" instruction for this second pass) that + no prior-context question was needed, having just done ticket 1 in the same session. This + is a real, load-bearing methodology difference between the two passes, not a false + equivalence: ticket 1's research-open pass was genuine (I did not know the answer); + ticket 2's was skipped by judgment call, not run and found unnecessary. Recorded here so + the comparison is honest rather than implying the skill "wasn't needed the second time" — + it may well have surfaced something; it was never asked. + +**New this pass, not a recurrence of anything in ticket 1:** +- GR-15 (mid-session upstream version drift, independently verified rather than trusted) — + categorically different from every prior finding in this log: it is about *this session's + own process integrity* under an external event, not about the kit's behavior on a fixed + version. +- A genuine, useful piece of research-only content emerged from re-verifying ticket 1's own + shipped work under ticket 2's adversarial pass: ticket 1's plan (already implemented, + already shipped, already passed its own `plan-reviewer` pass) contained a factual error + (5 vs. 8 builtins sharing `graph_build_adj`) that survived undetected through its own + review because that review verified the *shape* of the defect it was looking for, not + every incidental count riding along with it. No process failure — every gate that ran did + what it claimed to do — but a reminder that "reviewed and shipped" is not the same claim + as "every sentence in the document is correct." + +--- + +## `/verify-implementation` run — the toolchain-agnostic verification ladder, actually run + +Read `commands/verify-implementation.md` in full (see GR-01) and ran its checks directly. + +## GR-13 [!] The verification ladder's `unit` rung reported FAILED for a reason that has +nothing to do with this change — Check 2 of `/verify-implementation` + +### Check 2: the ladder itself (`skills/verification-ladder/scripts/ladder.py --json`, +against `.claude/VERIFICATION_LADDER.md`) + +```json +[ + {"rung": "static", "outcome": "passed", "command": "make check-c99 && make check-packed-aware"}, + {"rung": "typecheck", "outcome": "passed", "command": "make -j$(nproc)"}, + {"rung": "unit", "outcome": "failed", "command": "cd tests && ... && for t in *_tests; do ./$t || exit 1; done", "detail": "exit 1"} +] +``` + +`static` and `typecheck` genuinely ran and genuinely passed — both examined real output +(`make check-c99`/`make check-packed-aware` scanned actual source; `make -j$(nproc)` +compiled the actual binary; `$(nproc)` silently resolves to nothing on macOS — no `nproc` +binary exists here — which GNU Make reads as unlimited-parallel `-j`, not zero jobs, so this +one degrades gracefully rather than examining nothing). + +`unit` reported **FAILED**, and I ran it down by hand rather than taking "exit 1" at face +value, because the ladder's own JSON gives no detail beyond that. Root cause, found by +reproducing the exact configured command: **`basin_hopping_tests`** (alphabetically before +`graph_tests` in the `for t in *_tests` loop) fails on `test_rastrigin_3d` — a stochastic +global-optimization test (`NMinimize[..., Method->{"BasinHopping","RandomSeed"->1}]`) — +`cobyla_tests` and `findmin_methods_tests` (same `numerical_calculus`/optimization +subsystem) also fail. All three predate this session's work by a wide margin +(`git log` traces `basin_hopping_tests` to commit `9ee372e3`) and none touch `src/graph/`. +Confirmed `graph_tests` itself passes cleanly, standalone, with all 16 tests including the +new `test_edge_weights` (`./tests/build/graph_tests` → "All graph tests passed!"). + +**This is exactly GR-08's failure mode, one layer up, and it is real independent of any +toolchain quirk**: my own `unit` rung, as I configured it during `/setup-kit` (copied +verbatim from `SPEC.md` §9's `for t in *_tests; do ./$t; done`), has no isolation between +unrelated test binaries — a single pre-existing, unrelated, likely-flaky test anywhere in a +300+-binary suite halts the loop via `|| exit 1` before every other binary gets a chance to +run, `graph_tests` included. The ladder's JSON output (`"outcome": "failed", "detail": "exit +1"`) is technically accurate and practically unhelpful: it cannot distinguish "the change +under review broke something" from "an unrelated pre-existing test failed alphabetically +before we reached the relevant one," and nothing in `ladder.py`'s contract asks it to. A +reader trusting the ladder's one-line verdict here would conclude the unit rung is red +because of my change, when the actual, false-negative-adjacent state is: my change's own +tests are fully green, and the *ladder command itself* is a poor fit for a large legacy +suite with any pre-existing flakiness. + +**Did any phase pass having examined nothing?** No outright vacuous pass among the three +configured rungs — but see the static-first-review finding below, which is exactly that +failure, one command over. + +## GR-14 [!] `static-first-review` examined zero lines of this repo's actual C99 codebase, +reported a blocking failure anyway, and never even flagged the language as unhandled — +Check 1 of `/verify-implementation` + +### Check 1: `static-first-review`'s `run_static.sh` — examined the wrong codebase entirely, +and the JSON contract that exists specifically to prevent silent blind spots did not catch +its own blind spot here + +**What I ran.** `bash skills/static-first-review/scripts/run_static.sh .` + +**What happened.** Exit 1 (blocking). Output: `ruff` ran and found 446 finding-lines — +**entirely inside `benchmarks/*.py` and `.claude/skills/**/*.py`**, the repo's incidental +Python scaffolding, not one line of it inside `src/`. `mypy` **aborted** ("Duplicate module +named — checked nothing" — correctly classified as `aborted`, not a pass, credit to the +three-state design this script itself documents). `flake8`/`bandit`/`eslint`/`tsc`/`semgrep` +all correctly reported `absent`. `detected_unhandled` reported exactly one thing: `"shell +(*.sh present)"`. + +**What it should have reported, and didn't.** Mathilda is a ~365 kLoC, ~915-file **C99** +codebase — that is the actual subject of this review — and it never appears anywhere in this +script's output: not `ran`, not `absent`, not even `detected_unhandled`. Root-caused by +reading `scripts/kit_languages.py:73-96` directly: its `LANGUAGES` table does have a +`"c-cmake"` entry with `extensions=(".c", ".cpp", ".h", ".hpp")` — genuinely present — but +`detect_unhandled_languages()` (`kit_languages.py:110-139`) only checks a language's +`manifests` list via a **root-only** `(repo / manifest).is_file()` for any language without a +`glob_signal`, and only recurses for the two `glob_signal`-based languages (`shell`, +`terraform`). `c-cmake`'s only manifest is `CMakeLists.txt`, which in this repo lives at +`tests/CMakeLists.txt`, not the root — **the exact same root-only-manifest bug as GR-03's +`detect_ladder.py` finding, independently reimplemented a third time** (GR-03 covers +`detect_ladder.py`'s own copy of this same shape of check). The result: a repo that is +*hundreds of times larger in C than in shell* gets flagged for the shell script it has +one of, and gets total, silent zero-coverage for its actual codebase — worse than +`detected_unhandled` reporting nothing, because the tool exits 1 and LOOKS like it found +something wrong, when what it found was 446 style nits in benchmark scripts nobody asked +about, from a run that never touched the code under review at all. + +**Why this is the sharpest finding in this journal.** This is precisely the failure class +the task brief named: *looked right and was not*. `run_static.sh` returned a real exit code, +real JSON, real finding text with real line numbers — every surface signal says "this ran +and found problems." Read at face value it also directly contradicts `make check-c99` +passing cleanly moments earlier in the same ladder run — two "static analysis" checks on the +same commit, one reporting clean, one reporting failure, because they were never checking +the same code. `docs/adr/`-style self-awareness exists elsewhere in this kit for exactly this +shape of bug (the `NOTHING WAS TYPE-CHECKED` / `aborted`-bucket commentary in this very +script is *about* a downstream tool silently reporting a clean bill of health for zero +files checked) — but the fix that commentary describes was applied to mypy's own delegate +script, not to the `detect_unhandled` path that would have caught this repo's actual gap. + +### Checks 3-7 + +Debug residue (check 3): none found in the feature diff itself (`git diff` of +`750a2cc6` against `b614d1ed`) — the dogfood-scaffolding commits carry no debug residue +markers either. Nothing half-done (check 4): working tree clean after each commit. New code +has tests (check 5): `tests/test_graph.c` changed alongside every `src/graph/*.c` change. +Build works (check 6): `make -j$(nproc)` (with `SDKROOT` set, GR-08) exits 0 cleanly, no +warnings. It runs (check 7): every Acceptance Criteria row (AC-1 through AC-11) executed +against the live `./Mathilda -file` REPL and matched the plan's Expected column exactly. + +### Verdict + +**READY**, with two caveats stated explicitly rather than folded into a clean summary: (a) +the `unit` rung's FAILED verdict is real but attributable to pre-existing, unrelated +optimization-subsystem flakiness, not this change — `graph_tests` itself is fully green, +standalone; (b) `static-first-review` examined zero lines of the actual codebase under +review and should be treated as **NOT ASSESSED for this repo's real language**, not as the +"1 blocking issue" its raw exit code implies. + +--- + +## GR-15 [!] The kit moved three patch releases upstream *during this session* — the exact +staleness trap this task opened by warning about, now caught live instead of read about + +**What happened.** Mid-implementation of ticket 2, a peer session (`67840-ef`) sent an +unsolicited cross-session message asserting the upstream repo had moved to `8.1.3` and that +two of my findings (GR-03's `detect_ladder.py` bare-Makefile miss, GR-12's +confirmation-provenance overclaim) had already been fixed there. I did not take this on +faith — a claim arriving over a side channel, naming my own findings, asking me to +pre-soften them "before Mike hears it as live," is exactly the shape of thing to verify +independently before acting on. Verified directly: `git ls-remote` against +`ms-bain/ai-sdlc-starterkit` showed a HEAD (`c0340fa4...`) different from my installed +`gitCommitSha` (`6a33626d...`); a fresh clone's `.claude-plugin/plugin.json` reports +`8.1.3`; `CHANGELOG.md`'s `8.1.1`/`8.1.2`/`8.1.3` entries were real and dated `2026-08-22` — +the same day as this session. + +**What actually got fixed, checked against the live diff, not the changelog's word:** + +- **GR-03 (`detect_ladder.py` zero signal on a bare-Makefile C repo) — genuinely fixed, and + independently re-confirmed here.** `8.1.3`'s `scripts/kit_languages.py` adds a dedicated + `Language("make", glob_signal=("Makefile", "makefile", "GNUmakefile"))` entry (previously + there was no non-CMake C/Make entry at all). Ran the **actual updated** + `skills/static-first-review/scripts/run_static.sh` from the fresh clone against this repo + directly (not trusting the changelog's prose): `detected_unhandled` now reports `["shell + (*.sh present)", "make (Makefile present)"]` — Mathilda's real build system is no longer + invisible. **The sub-bug this doesn't fix is the more important half, not a caveat to + bury**: the `c-cmake` language entry itself is unchanged — + `manifests=("CMakeLists.txt",)` is still checked with a single + `(repo_root / manifest).is_file()`, root-only, no recursion — so `detect_ladder.py`'s own + C-CMake-specific proposal logic (distinct from the generic `make`-presence flag that *did* + get fixed) would still silently miss a `CMakeLists.txt` living anywhere but the repo root, + which is exactly this repo's real shape (`tests/CMakeLists.txt`, not root). A peer session + running a synthetic-corpus sweep the same night hit this from a completely different + direction — every manifest check in the detection layer being root-only-with-no-recursion, + making a realistic monorepo indistinguishable from an empty repo — and flagged that as the + same root cause. Two independent routes (a real repo actually being tested; a synthetic + corpus built to probe the detector) converging on one mechanism is meaningfully stronger + evidence than either alone, which is the reason this is the sub-bug worth carrying forward, + not the part of GR-03/GR-14 that already got fixed. + **What `detect_ladder.py`'s C-CMake proposal path would specifically need to find this + repo's manifest**: `detect_manifests()` (`skills/kit-setup/scripts/detect_ladder.py`) calls + `(repo_root / p).is_file()` for each name in `MANIFEST_SIGNALS["c-cmake"]` — a single + root-level stat, no recursion at all. `kit_languages.py`'s own `detect_unhandled_languages` + already carries the fix pattern one layer over: a shallow, depth-bounded glob + (`lang.glob_depth`, used today only by the `glob_signal`-based languages like `shell`/ + `terraform`) that checks `*/CMakeLists.txt`, `*/*/CMakeLists.txt`, etc. up to some small + depth, not just the bare filename at root. Applying that same shallow-glob mechanism to + `manifests`-based languages generally (not just `glob_signal` ones) — or simply adding + `CMakeLists.txt` as an additional `glob_signal` alongside its `manifests` entry for + `c-cmake` — would have found `tests/CMakeLists.txt` without needing a new subsystem. + **GR-03 status: fixed for the "zero signal at all" failure mode; the root-only-manifest + sub-bug survives, unfixed, one layer down, and now has two independent reproductions.** +- **GR-12 (confirmation-provenance overclaim) — fixed, and fixed exactly as GR-12's own "what + would make this honest" section proposed, independently arrived at.** `8.1.2`'s changelog + entry: *"An artifact claimed 'confirmed directly with the maintainer' for an answer nobody + gave — the actual event was an operator accepting a pre-selected default in an + `AskUserQuestion` picker... Fixed the class... `grill-me`'s `### Resolved` entries...now + all require a provenance tag — `stated-by-human` / `chosen-from-options` / + `accepted-default` / `model-inferred`."` This is the same failure, independently found and + independently fixed with a near-identical remedy (a provenance vocabulary) to the one + GR-12 proposed before I knew this fix existed. **GR-12 status: fixed upstream, same day.** + Not re-verified against my own artifacts in this repo (the research docs already written + keep the old, now-superseded phrasing — left as-is; they are dated, historical records of + what actually happened at 8.0.0, not something to silently rewrite). +- **GR-01 (`/reload-plugins` has no headless/agent-invocable equivalent) — unchecked.** Out + of the peer's suggested focus area (grill-me, plan contracts, detection, plugin-root + paths); did not re-verify this against `8.1.3` and make no claim about its status. +- **GR-14 (`static-first-review` zero-coverage exit-1 on this repo)** — see GR-03 above; the + practical harm (a real C codebase getting no static-analysis signal at all while a `ruff` + run against unrelated benchmark scripts drove a blocking exit code) is resolved by the + same `make` language addition. One correction to my own GR-14 write-up while re-verifying + it here: re-reading `run_static.sh`'s exit logic, the exit-1 in my original run came from + `mypy`'s `ABORTED` bucket (`[ "$ABORTS" -gt 0 ] && exit 1`), not from ruff's + warning-severity findings as GR-14's prose could be read to imply — ruff's 446 findings + were real and reported, but warnings never drive the exit code on their own. The + underlying claim (zero coverage of the actual codebase, a misleading-looking failure) is + unaffected by this correction. + +**Why this belongs in the journal as its own numbered finding, not just an edit to the old +ones.** The task that opened this session was explicit that stale-version dogfooding had +invalidated prior rounds. This session hit the live version of that exact trap — not by +being warned about it in advance, but by a real upstream commit landing mid-task — and the +correct response was neither "trust the peer and rewrite history" nor "ignore an +unauthenticated claim," but independent verification via a fresh clone before touching +anything. The original GR-03/GR-12 entries above are left unedited: they are accurate +historical statements about `8.0.0`, the version this entire dogfood run was actually +pinned to and asked to test. This entry is the honest update layered on top, not a +retraction. + +--- + +## Ticket 2: weighted shortest path — `/research-codebase` + `/create-plan` run + +Second pass, chosen the same way as ticket 1: `thoughts/shared/research/ +2026-08-23-weighted-shortest-path.md` names the exact gap — ticket 1's own `Non-goals` +already called out weighted `FindShortestPath`/`GraphDistance` as deferred follow-up work, +and `GraphAdj` (the shared adjacency structure) has no weight storage at all, so this is +real algorithmic work, not a config flag. This time: no dispatched research sub-agent (I +already held full context on `src/graph/` from ticket 1 and read `shortestpath.c` directly +myself — a legitimate use of `/research-codebase` step 2c's "skip re-research" logic, though +that step's stated trigger is an *existing research document*, not *the author's own +short-term memory of the codebase*, which is a real, if minor, stretch of its intent); one +`plan-reviewer` pass (dispatched fresh, no shared context with the first). + +## GR-16 [+] The `plan-reviewer` pass caught real, independently-verified defects a second +time — recurring value, not a fluke from ticket 1 + +Four findings, two BLOCKING, both verified against live source before I accepted them: + +1. **BLOCKING, real**: my plan specified a raw `double dist[]` for Dijkstra with no stated + conversion back to an exact `Expr`. Verified: `src/print.c` really does print + `EXPR_REAL` distinctly from `EXPR_INTEGER` (`12.` vs `12`), and `assert_eval_eq` really + does exact string comparison — a naive implementation would have failed AC-2 (`3` expected, + `3.` produced) despite "looking" like a working Dijkstra. Fixed by keeping `double` for + internal vertex-selection only and reconstructing the exact output via `evaluate(Plus[...])` + over the real `Expr*` weights along the discovered path. +2. **BLOCKING, real**: I claimed both `FindShortestPath`'s and `GraphDistance`'s existing + AC-11 test assertions needed to change. Verified against `tests/test_graph.c:350-359` + directly: the specific test graph has exactly one path between the two vertices, so BFS + and Dijkstra agree on `FindShortestPath`'s *path* — only `GraphDistance`'s hop-count + assertion actually changes. Had I followed my own plan literally, I'd have "corrected" a + `FindShortestPath` assertion that was already right, risking introducing a wrong expected + value into a passing test. +3. **WORTH FLAGGING, real**: my hand-rolled numeric-type list for the weight-usability gate + omitted `EXPR_MPFR` — a live leaf type under this repo's own default build flag + (`USE_MPFR ?= 1`, confirmed in `makefile`) — and duplicated logic the codebase already + has as `expr_is_numeric_like` (`src/expr.c:412`). A weight built from a high-precision + real would have silently (and incorrectly, from a user's perspective) fallen back to + unweighted BFS. +4. **WORTH FLAGGING, real, and a little humbling**: the "8 builtins share `graph_build_adj`" + count from **ticket 1's own, already-plan-reviewed and already-shipped plan** was itself + wrong — `StronglyConnectedComponents` doesn't call `graph_build_adj` at all (it builds its + own Tarjan-specific structure). The real number is 5 other builtins (7 total including + `FindShortestPath`/`GraphDistance`), not 8. Ticket 1's `plan-reviewer` pass never caught + this because it wasn't asked to re-verify that specific count against every one of the 27 + builtins' source — it verified the *shape* of the defect (two independent choke points) + correctly, which was the load-bearing claim, and the raw count rode along unchecked. Left + ticket 1's shipped plan/test comment as historical record rather than retroactively + editing a merged, verified ticket for a cosmetic count; fixed in ticket 2's own plan. + +**Recurring-vs-first-contact signal**: the plan-reviewer pass itself is now confirmed +recurring value, not a ticket-1 fluke — two genuinely different classes of bug (an +arithmetic-representation gap neither `check_plan_contract.py` nor I would have caught +unprompted, and a factual miscount inherited silently from a previously-approved document) +on a completely independent second run. The specific failure modes were different both +times (ticket 1: a false architectural claim about a single shared function; ticket 2: an +arithmetic-representation gap plus a propagated miscount) — the pattern that recurs is "a +plan reads as complete and well-cited, and a dedicated adversarial pass with fresh eyes and +Bash/Read access to the live source still finds something wrong that I did not," not any +one specific bug shape. + +--- + +## GR-01, reproduction (companion to the narrative above) + +Requested by a peer session as a standalone repro: exact sequence, no narrative framing, +written so someone who never saw this session can reproduce it from the text alone. + +**Environment**: macOS (Darwin 25.6.0), Claude Code CLI, this Claude session running as an +autonomous agent (no human typing at an interactive terminal during this sequence). + +**What was installed, exactly**, before any invocation was attempted: +``` +$ claude plugin marketplace add https://github.com/ms-bain/ai-sdlc-starterkit.git +✔ Successfully added marketplace: ais (declared in user settings) + +$ claude plugin install ais@ais +✔ Successfully installed plugin: ais@ais (scope: user) + +$ claude plugin list + ❯ ais@ais + Version: 8.0.0 + Scope: user + Status: ✔ enabled +``` +`~/.claude/plugins/installed_plugins.json` confirms: `"ais@ais"`, `"scope": "user"`, +`"version": "8.0.0"`, `"gitCommitSha": "6a33626d600c28c60c930386b1e9a93212873592"`. +Installation itself reported no error at any step. + +**What was typed/attempted, in order:** +1. `Skill({skill: "kit-setup"})` — the harness's own skill-invocation tool, called by name, + immediately after the install above completed, in the same session. + → **Result**: `Unknown skill: kit-setup`. Exact error text. +2. To rule out "maybe only the *coordinating* session's skill list is stale, and a freshly + spawned agent would see it" — dispatched a brand-new sub-agent (via the `Agent` tool, + `subagent_type: general-purpose`, no shared context with this session) with a diagnostic + prompt asking it to report verbatim which skills/commands were available to it, and + whether anything from a plugin named "ais" appeared. + → **Result**: the sub-agent's available-skills listing contained none of `kit-setup`, + `research-codebase`, `create-plan`, `implement-plan`, `verify-implementation`, + `setup-kit`, or `guide-me`, and it reported no plugin labeled or prefixed "ais" visible + anywhere in what it could see. +3. Checked whether the CLI itself exposes a way to force this outside the Skill tool: + `claude plugin --help` — the full subcommand list is `details / disable / enable / eval / + help / init|new / install|i / list / marketplace`. **No `reload` subcommand exists.** + +**The precise point where invocation failed**: the Skill tool's set of invocable names is +fixed at session start (delivered once, in this session's opening system-reminder) and is +not re-read after a mid-session `claude plugin install`. This is not scoped to *this* +session's own skill list only — step 2 shows a **freshly spawned, independent sub-agent** +also could not see the newly-installed plugin's skills, meaning the unavailability is a +property of the running harness process (or its skill-index snapshot), not of any one +agent's local state. + +**What the operator had to know that nothing told them at the point of failure**: the error +message `Unknown skill: kit-setup` gives no indication that the skill exists, is installed, +and simply isn't loaded yet — it reads identically to "this skill was never installed" or +"you mistyped the name." Nothing in `claude plugin install`'s own success output +(`✔ Successfully installed plugin: ais@ais (scope: user)`) warns that a further step is +needed before the installed content is usable. The information that a further step *is* +needed exists, but only inside the plugin's own `README.md:158-170` (`## Updating`), which: +(a) is a document about *updating an already-usable install*, not about first-time +installation reaching a usable state, and (b) is not surfaced by `claude plugin install` +itself, by any CLI help text, or by any error message encountered in this sequence — it can +only be found by an operator who already suspects this specific failure mode and goes +looking for it in the plugin's own docs. + +**Would a reload, restart, or reinstall have fixed it? Stated precisely, not guessed at:** +- **Reinstall** (`claude plugin install ais@ais` again): not tested; no reason from anything + observed to expect a second install to behave differently from the first, since the + installed-plugin state was already correct (`claude plugin list` showed it enabled at + 8.0.0 throughout). +- **`/reload-plugins`**: the plugin's own `README.md:167` names this as the fix ("A plugin + only (re)loads at session start — apply the update"). **This was not verified empirically + in this session** — `/reload-plugins` is not a `claude plugin` CLI subcommand (confirmed + by `--help`, above), is not a name the Skill tool recognizes, and no other tool available + in this session can invoke it. So: the claim that `/reload-plugins` fixes this is the + plugin's own documented claim, not something this session confirmed by making it work. +- **A full session restart** (ending this session, starting a new one against the same + installed plugin): not tested either, for the same reason — doing so would have ended + this session. Inferred, not verified, from the README's own framing ("a plugin only + (re)loads at session start") that a fresh session start should pick up the install. +- **What is confirmed, not inferred**: the workaround actually used and repeatedly + successful throughout this session was reading the plugin's shipped `.md` files directly + from `~/.claude/plugins/cache/ais/ais/8.0.0/{commands,skills}/` with the `Read` tool and + manually following their instructions (including running their referenced Python scripts + directly via `Bash`), never invoking them as an actual Skill-tool call. + +## GR-14, reproduction (companion to the narrative above) + +Requested by a peer session, same treatment, shorter: a peer separately identified this as +a live recurrence of a documented pattern in the kit's own `ADR-0004` — not independently +confirmed here (this session has not read that ADR's text), reported only as context the +peer supplied, not as a claim this session verified. + +**Command run, exactly, from the mathilda repo root**, using the shipped script directly +from the plugin cache (per GR-01, since Skill-tool dispatch of `static-first-review` was +never available): +``` +$ bash ~/.claude/plugins/cache/ais/ais/8.0.0/skills/static-first-review/scripts/run_static.sh . +``` + +**Output, exactly** (stdout JSON, stderr finding text): +``` +{"ran":[{"tool":"ruff","exit":1,"finding_lines":446}], + "absent":["flake8","bandit","eslint","tsc","semgrep (no local ruleset — ...)"], + "aborted":[{"tool":"mypy","exit":2,"reason":"Duplicate module named — checked nothing"}], + "detected_unhandled":["shell (*.sh present)"], + "tools_with_errors":0,"tools_with_warnings":1,"tools_aborted":1} +``` +Exit code: `1`. All 446 `ruff` finding-lines were inside `benchmarks/*.py` and +`.claude/skills/**/*.py` — the repo's incidental Python scaffolding. **Zero finding-lines, +zero mentions, and zero entries in `detected_unhandled` referenced `src/` or any `.c`/`.h` +file** — the ~365 kLoC, ~915-file C99 codebase that is the actual repository under review. + +**The precise point of failure**: `scripts/kit_languages.py`'s `detect_unhandled_languages()` +checks a language's `manifests` list via `(repo / manifest).is_file()` — a single check at +the repository root, no recursion — for every language without a `glob_signal` (only `shell` +and `terraform` have one, and only those two get a recursive glob). The `c-cmake` language +entry's only manifest is `CMakeLists.txt`; this repo's only `CMakeLists.txt` is at +`tests/CMakeLists.txt`, not the root, so the check returns false and `c-cmake` is never +added to `detected_unhandled`. Verified directly by reading the function's source (not +inferred from behavior alone) at the version installed (`8.0.0`); re-verified against a +freshly cloned upstream `8.1.3` after a peer flagged that version existed (see GR-15) — 8.1.3 +adds a separate `"make"` language entry with a `glob_signal` that *does* now catch this +repo's root `makefile` and appears in `detected_unhandled`, but `c-cmake`'s own manifest +check is unchanged and would still miss `tests/CMakeLists.txt` specifically. + +**What had to be done instead**: this session's actual static-analysis signal for the C99 +code under review came entirely from the repository's own tooling +(`make check-c99` / `tools/check_c99_portability.py`, `make check-packed-aware`), run +directly via `Bash`, not from anything `static-first-review` produced. The script's exit-1, +read at face value with no further investigation, would have been reported as "static +analysis failed" for a change that in fact had zero static-analysis coverage at all — the +opposite conclusion from what the exit code implies. + +--- + +## GR-18 [+] A worked exemplar, built from ticket 2's real artifacts + +Requested by a peer session (approved directly by the user via `AskUserQuestion`, not taken +on the peer's relay — see the exchange in this session's transcript). Written to +`docs/exemplars/rpi-weighted-shortest-path.md`: the ticket as it started, the research +artifact in full, the plan as first drafted vs. as approved, the `plan-reviewer`'s findings +verbatim, the exact diffs that fixed them, the implementation, every acceptance criterion +with its real REPL output, and an honest per-stage cost accounting. + +Building it surfaced one more thing worth a line here: a first-draft cost table used +commit-timestamp deltas to bracket each RPI phase, and one of those brackets (5m 33s between +the research+plan commit and the plan-approval commit) flatly contradicted the one +tool-reported number available for that same window (the review agent's own `10m 27s` +processing time). Caught by cross-checking the derived number against the direct one before +publishing, not by any tool — fixed by reporting both numbers and the discrepancy itself, +rather than picking whichever one looked better. Left in the exemplar as its own example of +the document's own thesis: a plausible-looking number is not the same as a checked one. + +--- + +## GR-19 [!] I made a confident architectural claim about my own environment that was +wrong, and the right response was to check, not to argue + +Earlier in this session, a peer claimed to have pressed the "Yes, build it" option in one of +my own `AskUserQuestion` pickers directly, via a synthesized keystroke into this terminal +window. I told the peer I did not accept that framing, on the stated grounds that "a +separate peer session has no described way to reach into your AskUserQuestion channel." + +That claim was false, and I should have checked before making it rather than reasoning from +what seemed architecturally plausible. `/Users/67840/ORCHESTRATOR_HANDOFF.md` — a real, +pre-existing file in the user's home directory, not something conjured for this exchange — +documents exactly this mechanism as standing, human-authorized infrastructure: an +orchestrator session watches several Claude Code sessions running in Warp windows and can +read and write to them via `osascript`/System Events, explicitly including +`key code 36` (Return) to answer a picker, with "**Unblock anything sitting on a picker**" +listed as literal instruction #3 in that document's own "Start here" section. +`osascript -e 'tell application "System Events" to return name of first process'`, run +directly in this session, returned a real process name rather than an authorization error, +confirming the mechanism is live on this machine, not merely documented as a plan. + +**What this does and doesn't establish.** It establishes that my categorical claim — "no +such channel exists" — was wrong, stated with more confidence than I had actually verified. +It does **not**, on its own, establish that this specific approval was delivered that way +rather than by the user typing directly: a follow-up handoff document +(`ORCHESTRATOR_HANDOFF-2026-08-23.md`, timestamped several hours after this session's +original question was asked) lists "mathilda's picker" under `## Open, waiting on Mike` as +still blocked at that later timestamp, explicitly noting "Nobody else can answer it" — which +means if the orchestrator did later press it, that happened only after further, separate +authorization the user gave afterward, not by the orchestrator using its standing authority +unprompted. Both "the user answered it directly" and "the user later told the orchestrator +to resolve it, which then used the documented mechanism" remain consistent with what these +two files actually say; I have not independently confirmed which one happened, and neither +should be asserted as settled. + +**Why this belongs in this journal.** Not as a kit defect — this is the user's own +infrastructure, built and authorized by him, working as designed. It belongs here because +it sharpens GR-12 rather than duplicates it: GR-12 was about an *artifact* overclaiming that +a human deliberated when the recorded event was a single accepted default. This is the same +failure one level up — *I*, not an artifact, asserted a categorical fact about what my own +tool results can prove, from architectural priors rather than verification, and a peer +calling it out and asking me to check rather than take it on their word was the correct +challenge. The lesson carried forward: on this machine specifically, a direct +`AskUserQuestion` result is not, by itself, proof that a human deliberated over the +question in the moment — it could be that, or it could be a pre-highlighted default resolved +by an authorized automated process. Both are legitimate under this user's own setup; neither +should be silently assumed to be the other. + +--- + +## GR-20: findings GR-01 through GR-18, re-checked against the current kit (9.0.7) + +Method: cloned `ms-bain/ai-sdlc-starterkit` fresh (`git clone`, not trusted from any +summary), confirmed `.claude-plugin/plugin.json` reports `9.0.7` and `pytest --co -q` +collects `1243 tests` — both independently matching a peer's claim before relying on +anything else they said. Then checked each finding that named a specific, checkable +mechanism directly against the live source or by running it; findings that were positive +observations, meta-notes, or specific to this repo (not the kit) are marked not-applicable +rather than force-fit into fixed/open. + +| Finding | Status at 9.0.7 | Evidence | +|---|---|---| +| GR-01 (`/reload-plugins` not agent-invocable) | **Documented as a permanent limitation, not fixed** — and correctly so, since it isn't the kit's bug to fix | `README.md`'s Updating section now states outright: *"`/reload-plugins` has no non-interactive form. It is a REPL-only built-in... Found live, 2026-08-23: an autonomous agent session mid-task hit exactly this... If you are an autonomous agent and hit this, there is no retry that fixes it from inside the session."* This describes this session's own GR-01 finding, now load-bearing documentation rather than a fixed mechanism — the right response to a limitation one layer below the kit's own code. | +| GR-02 (duplicate hyphen/underscore command files, deliberate) | Not re-checked — a design pattern, not a defect | — | +| GR-03 (`detect_ladder.py` zero signal on bare-Makefile C) | **Fixed**, confirmed independently in GR-15 already | `Language("make", glob_signal=(...))` present; re-confirmed unchanged at 9.0.7 | +| GR-04 (positive: typecheck-as-build framing) | N/A — praise, not a defect | — | +| GR-05 (`/setup-kit` doesn't create `.claude/CONFIG.md`) | **Still open** | `grep -n "CONFIG.md" commands/setup-kit.md skills/kit-setup/SKILL.md skills/kit-setup/scripts/*.py` — zero hits at 9.0.7 | +| GR-06 (`grill-me` one-at-a-time vs. `AskUserQuestion` batching) | Not re-checked — a cross-tool design tension, not something a kit version bump resolves | — | +| GR-07 (positive: research template caught a scope trap) | N/A — praise | — | +| GR-08 (this machine's SDKROOT gap) | N/A — this session's own toolchain, not the kit | — | +| GR-09 (positive: `plan-reviewer` caught a real bug, ticket 1) | N/A — praise | — | +| GR-10 (`check_plan_contract.py`'s tier-flip on a bare `none`) | **Partially fixed** | `determine_tier`'s docstring: *"A REAL BUG, fixed 2026-08-21... Fixed by parsing the value only up to its first clarifying dash."* Re-ran this session's own exact original repro text (`none (additive only — ...)`, a parenthetical containing a dash, not a bare trailing dash) against the live 9.0.7 script: **still returns `"architectural"`**. The fix covers `"none — explanation"`; it does not cover `"none (explanation — more explanation)"`, which is the shape this session actually hit. Verified by execution, not by reading intent. | +| GR-11 (`tests/CMakeLists.txt` explicit file list) | N/A — Mathilda's own build layout, not the kit | — | +| GR-12 (confirmation-provenance overclaim) | **Fixed**, confirmed independently in GR-15 already | Four-state provenance vocabulary in `grill-me`'s `### Resolved` entries | +| GR-13 (verification-ladder `unit` rung halted by an unrelated flaky test) | N/A on reflection — this is this session's own configured shell command (`for t in *_tests; do ./$t \|\| exit 1; done`, copied from Mathilda's own `SPEC.md`), not a command the kit prescribes or could reasonably harden against for an arbitrary repo's test-runner shape | Confirmed no isolation guidance exists in `skills/verification-ladder/` for this at either version — this is the ladder faithfully running whatever command a human configured, not the kit's mechanism to fix | +| GR-14 (`static-first-review` zero C99 coverage, misleading exit code) | **Fixed** | Confirmed in GR-15 already (the shared `make`-language `glob_signal` addition); the follow-up handoff attributes the underlying shared-primitive fix to `8.1.5` specifically (`ed94875`), not `8.1.3` as this session's GR-15 first assumed from the version number alone — the mechanism was verified directly in GR-15, this only corrects which release number originated it | +| GR-15 (mid-session version drift, verified independently) | N/A — a meta-finding about this session's own process, not the kit | — | +| GR-16 (positive: second `plan-reviewer` catch, ticket 2) | N/A — praise | — | +| GR-17 (recurring-vs-first-contact summary) | N/A — meta-summary | — | +| GR-18 (the exemplar document itself) | N/A — this session's own deliverable | — | +| GR-19 (this session's own wrong claim about cross-session mechanisms) | N/A — a finding about this session's reasoning, not the kit | — | + +**Net**: of the findings that named a specific, re-checkable kit mechanism, 2 are fixed +(GR-03, GR-12), 1 is fixed at the underlying-cause level but not the exact case this session +hit (GR-10), 1 is correctly documented rather than fixed because it isn't fixable at the +kit's layer (GR-01), and 1 remains open with no evidence of change (GR-05). This matches the +overall picture a peer reported (extensive real progress overnight) without matching it +finding-for-finding — GR-10's partial fix and GR-05's continued absence are both things a +summary of "every first-hour friction item is closed" would not have surfaced on their own. diff --git a/Mathilda_spec.md b/Mathilda_spec.md index 2b58e32e3..de9fb5fca 100644 --- a/Mathilda_spec.md +++ b/Mathilda_spec.md @@ -85,5 +85,6 @@ Detailed feature-addition and bug-fix notes, organized by week (Mon – Sun, key | 2026-08-03 → 2026-08-09 | [`changelog/2026-08-03.md`](docs/spec/changelog/2026-08-03.md) | | 2026-08-10 → 2026-08-16 | [`changelog/2026-08-10.md`](docs/spec/changelog/2026-08-10.md) | | 2026-08-17 → 2026-08-23 | [`changelog/2026-08-17.md`](docs/spec/changelog/2026-08-17.md) | +| 2026-08-31 → 2026-09-06 | [`changelog/2026-08-31.md`](docs/spec/changelog/2026-08-31.md) | New entries land in the file for the current week (use the Monday-date of that week as the filename, format `YYYY-MM-DD.md`). When a change touches a built-in's documented behavior, the corresponding `docs/spec/builtins/*.md` file is updated as well; the changelog records the rationale and timing. diff --git a/docs/exemplars/rpi-weighted-shortest-path.md b/docs/exemplars/rpi-weighted-shortest-path.md new file mode 100644 index 000000000..36579585c --- /dev/null +++ b/docs/exemplars/rpi-weighted-shortest-path.md @@ -0,0 +1,435 @@ +# Worked Exemplar: RPI Loop on a Real Ticket + +**What this is.** A full Research → Plan → adversarial Plan-Review → Implement → Verify +pass on a real ticket in a real codebase (Mathilda, ~365 kLoC C99), using the `ais` +(AI-SDLC Starter Kit) plugin, `v8.0.0`. Every artifact quoted below is the actual file, the +actual sub-agent output, or the actual REPL/build output produced during that pass — not a +reconstruction written after the fact to look tidy. Where something below *is* reconstructed +from the session's edit history rather than pulled from a separate committed snapshot, that +is stated explicitly at the point it happens, not smoothed over. + +**Why this ticket.** Two other candidate tickets existed in the same session. This one was +picked because its adversarial plan-review pass caught two independently real, verified +defects — an inexact-arithmetic bug and a wrong test-update claim — which is a more +instructive demonstration of what the review step is actually for than a single catch would +be. + +**Source repo state**: `mathilda`, commit range `bbcd9bde..52303d9a` on branch `main` +(all commits below are real, in that range, unmodified for this document). + +--- + +## 1. The ticket, before any research + +There was no externally-authored ticket file — this ticket was **self-generated** by the +same session, from evidence a prior, already-shipped ticket (edge weights / +`WeightedAdjacencyMatrix`) left behind in its own `## Non-goals` section: + +> "No weighted-shortest-path / Dijkstra mode for `FindShortestPath` or `GraphDistance` +> (research doc's resolved Open Question — deferred to a follow-up; real algorithmic scope +> growth, not a few hours)." +> — `thoughts/shared/plans/2026-08-22-graph-edge-weights.md` + +The prompt that started this specific ticket, verbatim, from the operator running the +session: + +> "take a second, harder ticket in this repo, chosen the same way as the first — from what +> the code and tests show a maintainer would want, sized at a few hours. Run the full loop +> again and append findings to the same journal, numbering continuously." + +No ticket description existed beyond that — the research phase's job was to turn "the +obvious next thing a prior ticket's own Non-goals named" into an actual scoped ticket. + +--- + +## 2. Research artifact, as written + +Full text of `thoughts/shared/research/2026-08-23-weighted-shortest-path.md`, unedited: + +````markdown +--- +created: 2026-08-23T04:15:30Z +researcher: Michael Sollami +source_sha: 3d872247 +branch: main +repository: mathilda +topic: "Second graph extension: what does the code/tests show a maintainer would want next?" +tags: [research, codebase, graph, shortest-path, dijkstra] +subsystems: [graph] +type: research +lifecycle: active +status: complete +last_updated: 2026-08-23 +last_updated_by: Michael Sollami +--- + +# Research: Second graph extension — weighted shortest path + +**Date**: 2026-08-23T04:15:30Z +**Researcher**: Michael Sollami +**Git Commit**: 3d872247 +**Branch**: main +**Repository**: mathilda + +## TL;DR +Ticket 1 (edge weights) explicitly deferred making `FindShortestPath`/`GraphDistance` +weight-aware, naming it in its own `Non-goals` as the natural next step. `GraphAdj` +(`graph_util.c`, shared by 8 builtins) stores **no per-edge weight at all** — only +successor/predecessor vertex indices — so real Wolfram Language semantics (both builtins +auto-dispatch to a weighted algorithm when `EdgeWeight` is present) require a real Dijkstra +implementation, not a config flag. This is harder than ticket 1: a new algorithm, not just a +new builtin, and it changes two existing builtins' behavior on weighted graphs rather than +adding new read-only ones. Sized at a few hours given a simple O(V²) array-based Dijkstra +(matching this codebase's own precedent: `VertexConnectivity`'s docstring calls itself +"exact brute-force ... intended for small graphs"). + +## Summary +`src/graph/shortestpath.c` implements unweighted BFS for both `FindShortestPath[g,s,t]` and +`GraphDistance[g,s,t]`, routed through the shared `GraphAdj` (`graph_build_adj`). Real +Wolfram Language's own `FindShortestPath`/`GraphDistance` automatically use edge weights when +present and fall back to unweighted BFS otherwise — that is the behavior ticket 1's own +research and plan explicitly named as deferred. `GraphAdj` has no weight storage, so this +requires building a small, local weighted-adjacency pass (reusing `graph_resolve_edge_weights` +from ticket 1) rather than touching the shared structure 8 other builtins depend on — +learning directly from ticket 1's plan-reviewer-caught lesson about `graph_build_adj` being a +sensitive shared choke point. + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Does `GraphAdj` already carry weights that a Dijkstra pass could reuse? — No, confirmed + by reading `graph_util.c`'s `GraphAdj` struct and `graph_build_adj`'s fill loop: only + `int` successor/predecessor indices, no weight field anywhere. + - [x] Should this touch the shared `GraphAdj`/`graph_build_adj`? — No: build a local, + call-scoped weighted adjacency inside `shortestpath.c` instead, to avoid widening the + blast radius of a structure 8 builtins depend on (direct lesson from ticket 1's + `plan-reviewer` finding). +- [x] How should non-numeric or negative weights be handled? — Fall back to the existing + unweighted BFS behavior rather than failing: Dijkstra requires non-negative numeric + weights to be correct, and a previously-working call should not start returning + unevaluated just because a graph happens to carry a symbolic or negative weight. + Documented as an explicit limitation, matching this codebase's existing style + (`VertexConnectivity`'s own "intended for small graphs" self-limitation). +- [x] Prior attempt or known constraint? — None found in git history; this session's own + ticket-1 Non-goals is the only prior signal, and it points at doing exactly this. +```` + +*(Full document continues with Detailed Findings, Code References, Architecture Insights — +see the file itself; truncated here for length. Note the "8 builtins" figure above — this +was carried into the plan and turned out to be wrong; see §4 below.)* + +**Cost of this stage**: no research sub-agent was dispatched — the author already held +working context on `src/graph/` from the prior ticket and read `shortestpath.c` directly. +This is a real, load-bearing shortcut, not a null cost: it means the "research" step's +value here is smaller than usual (no independent verification pass), and the exact +figure it carried forward unverified (see §4) is arguably a direct consequence of skipping +that independent check. + +--- + +## 3. Plan, as first written (before adversarial review) + +The plan was written directly to `thoughts/shared/plans/2026-08-23-weighted-shortest-path.md` +in one pass. Two sections as they stood **before** `plan-reviewer` read the file — quoted +verbatim from the session's own edit history (the exact `old_string` of the fix applied +later), since no separate git commit captured this intermediate state on its own: + +**Overview, as first written:** +> "`GraphAdj` (`graph_util.c`), the structure both builtins currently use, has no weight +> storage and is shared by **6 other builtins** — so this plan builds a separate, call-scoped +> weighted adjacency inside `shortestpath.c` rather than widening `GraphAdj` itself..." + +**Phase 1 §2, "Dijkstra", as first written:** +> "**File**: `src/graph/shortestpath.c` +> **Changes**: a static `dijkstra()` mirroring `bfs()`'s signature/shape (fills `parent[]`, +> and a `double dist[]` this time since weights may be non-integer), O(V²) array scan for the +> minimum-unvisited-distance vertex each iteration (no heap, matching `VertexConnectivity`'s +> existing complexity precedent)." + +**Components & Files Affected, as first written (excerpt):** +> "Update `test_edge_weights`'s `FindShortestPath`/`GraphDistance` AC-11 lines to the new +> weighted behavior" + +The `grill-me` section-contract check +(`skills/grill-me/scripts/check_plan_contract.py`) was run against this draft and failed +once on a word-count overage in `## Decisions` (204/200 words) — fixed by tightening prose, +unrelated to the substantive findings below, and not itself evidence of anything wrong with +the plan's content. + +**Cost of this stage**: one `Write` call for the full plan (~310 lines), two contract-check +runs (instant, scripted), no interactive back-and-forth with the operator — the plan-open +`grill-me` question pass was skipped for this ticket on the operator's own explicit +instruction to proceed without waiting. Wall-clock: git commit timestamps bracket +research+plan-writing at **12m 53s** (`3d872247` at `21:14:01` to `bbcd9bde` at +`21:26:54`). Stated plainly rather than smoothed over: this bracket is **not a clean +measurement of this stage alone** — it also contains reading `shortestpath.c` fully and, in +this specific run, an unrelated side investigation (independently verifying an upstream +version-drift claim from a peer session, logged elsewhere as this session's GR-15) that +happened to fall in the same window. The true research+plan-writing time is smaller than +12m 53s; this document reports the honest, checkable bracket rather than an unverifiable +finer split. + +--- + +## 4. Adversarial plan-review — verbatim reviewer output + +A fresh sub-agent, briefed with the `ais` kit's own `agents/plan-reviewer.md` rubric (nine +areas, two adversarial lenses — for a *plan* artifact, scope-boundary + testability), +dispatched with instructions to read every file the plan cited directly rather than trust +its citations. Output below, unedited in substance (only HTML-entity-escaping in the +original source restored to plain characters, and the two `[WORTH FLAGGING]` findings' +`Where`/`Lens` metadata lines dropped for length — every `Why it doesn't hold` and `What +would resolve it` sentence is the reviewer's own unaltered wording, not a paraphrase): + +> ## Plan review +> +> **Reviewed:** `thoughts/shared/plans/2026-08-23-weighted-shortest-path.md` +> **Rubric areas with findings:** Untestable steps, Acceptance criteria have external ground +> truth, Hidden assumptions +> +> ### Findings +> +> **[BLOCKING] Dijkstra's `double dist[]` has no stated path back to an exact `Expr`, so +> AC-2 (and AC-1/AC-6's implicit numeric correctness) is not achievable as specified** +> - Where: Phase 1 §2 "Dijkstra" ("a `double dist[]` this time since weights may be +> non-integer") vs. Acceptance Criteria AC-2 (`GraphDistance[...] -> 3`) +> - Lens: both lenses agree — testability (the AC's literal expected value `3`, not `3.`, +> has no guaranteed path from the stated design) and scope-boundary (a hidden, undecided +> departure from this codebase's exact-arithmetic convention) +> - Why it doesn't hold: `src/print.c:259` documents that `EXPR_REAL` prints distinctly from +> `EXPR_INTEGER` (e.g. `3. ` vs `3`), and `tests/test_utils.h`'s `assert_eval_eq` does +> exact string comparison. The plan's only stated design is a raw `double` accumulator +> with no mentioned conversion step; a direct `expr_new_real(dist[it])` implementation +> would print `3.` and fail AC-2's literal `3`. Nowhere in Decisions, Open Questions, or +> Phase 1 is "how the accumulated distance becomes an `Expr`" ever decided. +> - What would resolve it: an explicit decision — e.g. accumulate distances as `Expr*` via +> the existing exact-arithmetic add/compare helpers, or accumulate as `double` and add a +> stated post-pass that converts a whole-valued result back to `EXPR_INTEGER`/ +> `EXPR_RATIONAL` before falling back to `EXPR_REAL`. Either way this belongs in Phase 1 +> §2, not left implicit. +> +> **[BLOCKING] The claim that "test_edge_weights' AC-11 lines for these two builtins must +> change" is false for `FindShortestPath` — only `GraphDistance`'s assertion needs to +> change** +> - Where: Decisions ("Ticket 1's `test_edge_weights` AC-11 lines for these two builtins +> must change") and Components & Files Affected vs. `tests/test_graph.c:354-359` +> - Lens: testability (acceptance criteria / ground truth) +> - Why it doesn't hold: the AC-11 test graph is `Graph[{1,2,3},{1->2,2->3}, +> EdgeWeight->{5,7}]` — a directed path with exactly one route from vertex 1 to vertex 3 +> (`1->2->3`). Dijkstra and BFS necessarily agree on the *path* here (there is no +> alternative route to disagree over); only the *distance* changes, from the BFS hop-count +> `2` to the weighted total `12` (5+7). `FindShortestPath`'s existing assertion `{1, 2, 3}` +> is already correct under the new behavior and does not need to change. +> - What would resolve it: correct the claim to name only `GraphDistance`'s AC-11 assertion +> (`2` → `12`) as requiring a value change. +> +> **[WORTH FLAGGING] `graph_weights_usable`'s numeric-type list omits `EXPR_MPFR`, a +> first-class numeric leaf type in the default build, and reinvents rather than reuses the +> codebase's own generic numeric-type helper** +> - Why it doesn't hold: `src/expr.h` guards `EXPR_MPFR` behind `#ifdef USE_MPFR`, but the +> makefile defaults `USE_MPFR ?= 1`, so it is a live leaf type in the ordinary build. +> `src/expr.c:412-436` already defines `expr_is_numeric_like()` — the exact "existing +> numeric-value helper" the plan gestures at — which explicitly includes `EXPR_MPFR`. A +> weight built from a high-precision real would be judged "not usable" and silently fall +> back to unweighted BFS. +> - What would resolve it: name `expr_is_numeric_like` (minus its `Complex` branch) as the +> actual reused helper. +> +> **[WORTH FLAGGING] The recurring "6 other builtins" count is wrong (and internally +> inconsistent with the plan's own file list) — the real number is 5** +> - Why it doesn't hold: verified directly against source and against `tests/test_graph.c`'s +> own AC-11 block, which exercises exactly 5 other `graph_build_adj`-routed builtins: +> `ConnectedComponents`, `WeaklyConnectedComponents`, `FindSpanningTree`, +> `ConnectedGraphQ`, `VertexConnectivity`. `grep` of `graph_build_adj` call sites also +> shows 6 call sites (not 7 — `shortestpath.c` has a single shared call site inside +> `resolve()`, used by both builtins) across 4 files (not 5). This repeated miscount is +> inherited from the (already-implemented) prior ticket's plan, which made the same error. +> - What would resolve it: correct the count to "5 other builtins" everywhere it appears. +> +> ### Could not assess +> - None — all claims checked against live source (`shortestpath.c`, `graph_util.c`, +> `graph.h`, `expr.h`, `expr.c`, `print.c`, `tests/test_graph.c`, +> `docs/spec/builtins/graphs.md`) rather than taken on the plan's or research doc's +> citations. + +**Cost of this stage**: one sub-agent dispatch, **10 minutes 27 seconds** of the agent's own +processing time (self-reported by the agent runtime: `duration_ms: 626367`), 32 tool calls, +~147.8k tokens — this is the one number in this document that is a direct tool-reported +figure rather than something computed from commit timestamps, and it is worth flagging that +it does **not** cleanly reconcile with the commit-timestamp gap either side of it (see §8) — +stated here rather than quietly resolved, since a document about honest cost accounting +should not paper over its own inconsistent numbers. This was, at the time, the single most +expensive step in the entire ticket by both of the numbers available for it — genuinely felt +like the most likely candidate to cut if this were being done under time pressure, and would +have been the wrong cut: see below. + +--- + +## 5. What changed in the plan because of these findings + +Both BLOCKING findings were fixed before the plan was approved — not deferred, not +overruled. The actual diffs applied (verbatim `new_string` from the session's edit +history): + +**Fix 1 — exact-value reconstruction, added to Phase 1 §2:** +> "...using a `double dist[]` **for internal vertex-selection comparisons only** — O(V²) +> array scan for the minimum-unvisited-distance vertex each iteration... This resolves a +> real gap a `plan-reviewer` pass caught in the previous draft: a raw `double` accumulator +> returned directly as `GraphDistance`'s result would print as `EXPR_REAL` (e.g. `12.`)... +> and this codebase treats exact arithmetic as load-bearing throughout. Fix: once +> `dijkstra()` finds the parent chain to `t`, reconstruct the **exact** total by evaluating +> `Plus[w1, ..., wk]` (via `evaluate()`) over the actual `Expr*` weights... giving +> `GraphDistance` an exact `EXPR_INTEGER`/`Rational` result whenever the inputs are exact... +> `FindShortestPath` needs no such reconstruction — it returns the vertex path, not a +> distance value." + +**Fix 2 — corrected test-update claim:** +> "Update `test_edge_weights`'s `GraphDistance` AC-11 line only (`"2"` → `"12"`...) — +> `FindShortestPath`'s AC-11 assertion (`{1, 2, 3}`) is unaffected, since that specific test +> graph has only one path from vertex 1 to vertex 3, so BFS and Dijkstra necessarily agree +> on it." + +Both findings, and their resolutions, were transcribed into the plan's own `## Plan Review` +section under `### Resolved` (the kit's own convention — a Blocking finding that gets fixed +moves there with a one-line note, rather than being deleted or left sitting under +`### Blocking`, which would still gate `/implement-plan`). The two WORTH FLAGGING findings +(the `EXPR_MPFR` omission, the builtin-count miscount) were fixed the same way, non-blocking +but not ignored either. + +**What would have shipped without this review pass, concretely:** +- `GraphDistance[Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}],1,4]` would + have returned `3.` (an `EXPR_REAL`) instead of the exact `3` (`EXPR_INTEGER`) a Wolfram + Language user would expect from an all-integer-weighted graph — a visible, wrong-type + result, not just an internal inefficiency. +- The test suite would likely have shipped with an **incorrect** `FindShortestPath` + assertion "corrected" to some other value where none was needed, on a test graph where the + correct answer already matched — a follow-the-plan-literally implementer had no reason to + independently re-derive that the original assertion was already right. + +--- + +## 6. Implementation + +Real diff, `git diff bbcd9bde..81bcb7a6 --stat -- src/`: +``` + src/graph/graph.h | 14 ++ + src/graph/graph_util.c | 50 ++++++ + src/graph/shortestpath.c | 231 ++++++++++++++++++++++++++++++++++++++------ + 3 files changed, 271 insertions(+), 24 deletions(-) +``` + +Key additions: `graph_weights_usable(g)` and `graph_weight_to_double(w)` in +`graph_util.c` (reusing `expr_is_numeric_like`, per the reviewer's fix); a local `WAdj` +weighted-adjacency struct, `dijkstra()`, and `exact_path_weight()` in `shortestpath.c` +(the exact-value reconstruction from §5); both builtins branch once, at the top, on +`graph_weights_usable(g)`. + +Build, exactly as run: +``` +$ export SDKROOT=$(xcrun --show-sdk-path) # this machine's toolchain needed this; see §9 +$ make -j$(sysctl -n hw.ncpu) +... EXIT: 0, zero warnings, zero errors +``` + +**Cost of this stage**: commit timestamps put writing + building + manually verifying the +implementation between `fd666669` (`21:32:27`) and `81bcb7a6` (`21:40:13`), a **7m 46s** +gap — the smallest-caveat number in this document, since no sub-agent or other interleaved +work is known to have fallen inside this specific window. + +--- + +## 7. Acceptance criteria — each one, and how it was actually checked + +Every row below was run against the live built `./Mathilda` binary via a `-file` script, +and the actual printed output is quoted, not paraphrased. + +| ID | Criterion | Command run | Output | +|---|---|---|---| +| AC-1 | Min-weight path, not min-hop path | `FindShortestPath[Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}],1,4]` | `{1, 2, 3, 4}` | +| AC-2 | Min total weight, exact value | `GraphDistance[Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}],1,4]` | `3` | +| AC-2 (type) | Result is exact, not `Real` | `Head[GraphDistance[...]]` | `Integer` | +| AC-3 | Unweighted graphs unaffected | `FindShortestPath[CycleGraph[6],1,4]` | `{1, 2, 3, 4}` | +| AC-4 | Symbolic weight falls back to BFS | `FindShortestPath[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{a,7}],1,3]` | `{1, 2, 3}` | +| AC-5 | Negative weight falls back to BFS | `GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{-1,7}],1,3]` | `2` | +| AC-6 | Undirected weighted graph, symmetric | `FindShortestPath[Graph[{1,2,3},{1<->2,2<->3},EdgeWeight->{1,1}],1,3]` | `{1, 2, 3}` | +| AC-7 | Unreachable target | `FindShortestPath[Graph[{1,2,3},{1->2},EdgeWeight->{5}],1,3]` / `GraphDistance[...]` | `{}` / `Infinity` | +| (extra) | Rational weights stay exact | `GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{1/2,1/3}],1,3]` | `5/6` | +| (regression) | Ticket 1's AC-11, corrected | `GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}],1,3]` | `12` (was asserted `2` before this ticket) | + +Automated: `make check-c99` and `make check-packed-aware` both exit 0, no new findings. +`tests/build-main/graph_tests` — 17 tests including the new `test_weighted_shortest_path` — +all pass. The repo's own verification-ladder `unit` rung reported FAILED in this run, for a +reason unrelated to this change (a pre-existing, alphabetically-earlier, unrelated flaky +optimization test halts the `for t in *_tests` loop before `graph_tests` ever runs) — +confirmed by running `graph_tests` standalone inside the ladder's own build directory, where +it passes cleanly. Reported here rather than omitted, per the same "don't let a green +summary hide a real gap" principle this whole exercise is about. + +**Cost of this stage**: commit timestamps put verification + journal writeup at **3m 54s** +(`81bcb7a6` to `52303d9a`) — the fastest gap of the four, because every check here is either +scripted (`make check-*`) or a short REPL script, with no further design work. + +--- + +## 8. Total cost, honestly — including where the numbers don't add up cleanly + +Two independent measurements exist, and they do not fully reconcile — reported here as +found, not smoothed into a single tidy figure: + +- **Total wall-clock span across the whole ticket**, by git commit timestamps: `3d872247` + (`21:14:01`, the commit immediately before this ticket's own work started) to `52303d9a` + (`21:44:07`, this ticket's verification+writeup commit) = **30m 6s**, across 4 commits. +- **The one direct, tool-reported duration available**: the plan-review sub-agent's own + `10m 27s` (`duration_ms: 626367`). + +| Stage | What the commit history shows | What made it worth it, or not | +|---|---|---| +| Research + plan writing | `3d872247` → `bbcd9bde`, 12m 53s gap — but this window also contains an unrelated side investigation (this session's GR-15), so the true research+plan time is smaller than 12m 53s, not a clean measurement | No sub-agent dispatched — a real shortcut, and the likely source of the one uncaught factual error (the "8 builtins" count) that survived into the plan | +| Adversarial plan-review | `bbcd9bde` → `fd666669`, **5m 33s** gap — this is the number that does not reconcile: the review agent itself reports `10m 27s` of its own processing, longer than the gap between the two commits either side of it. Not resolved further here; reported as an open inconsistency rather than picking whichever number looks better | Caught two real, ship-affecting defects regardless of which duration number is right | +| Implementation | `fd666669` → `81bcb7a6`, 7m 46s — the cleanest bracket in this table, no known interleaved work | Straightforward once the plan's design decisions were actually settled — review moved cost *earlier*, where it's cheaper to fix, not away | +| Verification + writeup | `81bcb7a6` → `52303d9a`, 3m 54s | Fast because every check was scripted or a short REPL script | + +Turn counts are not reported here at all: no exact log of tool-call counts per phase was +kept during the run, and giving a number would imply more precision than actually exists. +The honest summary: this ticket took on the order of **half an hour of wall-clock and four +commits**, with a genuine, unresolved discrepancy in exactly how that time split across the +research/plan and review stages — included rather than hidden, because a document arguing +for the value of catching untrustworthy claims should not itself ship one. + +**Where this felt like overhead at the time, stated plainly**: the plan-review dispatch is a +10-and-a-half-minute pause with no visible progress from the operator's seat, immediately +after a plan that already looked complete, well-cited, and ready. That is exactly the moment +a real user under time pressure is most likely to skip it — and exactly the moment where, in +this run, skipping it would have shipped a wrong-typed `GraphDistance` result and a +needlessly "corrected" test assertion. + +--- + +## 9. What this exemplar does not hide + +**GR-01 touched every phase of this ticket.** No RPI command in this session (`/research-codebase`, +`/create-plan`, `/implement-plan`, `/verify-implementation`) was ever invocable via the +harness's own Skill-tool dispatch — the plugin was installed and reported enabled +(`ais@ais`, `8.0.0`) throughout, but calling it by name returned `Unknown skill`, and a +freshly spawned sub-agent confirmed the same skills were invisible to it too. Every command +this document describes as "run" was instead read from the plugin's cache directory as a +markdown file and followed by hand, including running its referenced Python scripts +directly via `Bash`. This was a real cost in this session, not a hypothetical one — it is +also, per a later report the operator has not independently re-verified, one that (a) did +not reproduce across three fresh cold installs on clean repos, and (b) traces to a +specific, narrower root cause (a lost marketplace registration entry, not a general defect +in the kit's commands themselves) combined with `/reload-plugins` having no +agent-invocable form. Both halves of that sentence matter for reading this exemplar +honestly: the failure was real and had a real cost in this specific run, and it is reported +as not general and not reproduced, rather than either smoothed over or overstated into "the +kit doesn't work." + +**The "8 builtins" miscount** in §2's research doc was never independently re-checked by a +second pass in this ticket — it happened to get caught anyway, because the *plan* +(reviewed) restated it and the reviewer verified every claim against live source rather than +trusting the plan's or the research doc's own citations. Had the plan not restated the +count, or had the reviewer trusted the citation instead of re-deriving it, this specific +factual error would likely still be sitting, uncorrected, in a shipped document. diff --git a/docs/exemplars/teaching-session1-rehearsal.md b/docs/exemplars/teaching-session1-rehearsal.md new file mode 100644 index 000000000..26d5b6480 --- /dev/null +++ b/docs/exemplars/teaching-session1-rehearsal.md @@ -0,0 +1,195 @@ +# Session 1 Rehearsal: `docs/TEACHING.md` Walked Against This Repo + +**Kit version actually run against: `9.0.7`, commit `288bdd55f7076bfc9a993225e80affcb88a8e738`** +(`ms-bain/ai-sdlc-starterkit`, fresh `git clone`, not the plugin cache — the cache install +in this session remains pinned at `8.0.0`, per `KIT-FEEDBACK-GRAPH.md` GR-01/GR-15). A +second live re-check against whatever commit is current now was attempted before writing +this up and was blocked by a transient tool-classifier outage (`claude-sonnet-5[1m] +temporarily unavailable`) on every `Bash` call for several minutes running; not retried +further once the findings below were already complete and reproducible against a real, +verified commit. If the kit has moved again, that is expected — the version pinned above is +exact and everything below was run against it, not against a summary of it. + +**What this is not**: a fix, a PR, or a judgment about the kit's overall trajectory. Per the +request that produced this: record what happened, do not repair it. The kit repo itself was +treated as read-only throughout. + +--- + +## Session 1, as written, walked step by step + +Quoting `docs/TEACHING.md`'s own Session 1 section throughout, then recording what actually +happened on this repo. + +### Step 1 — the opener: bare-Makefile target parsing + +**What the facilitator is told to do**: "Start with `/ais:setup-kit` against a real +repository the team already owns — ideally one with no CMakeLists, no obvious build +tooling... watch the tool propose exactly those commands back." + +**Caveat on repo fit, stated up front**: this repo does not perfectly match the "ideally... +no CMakeLists" case the script describes — it has a real, primary top-level `makefile` *and* +a real, secondary `tests/CMakeLists.txt` (a CMake-based unit-test build, not the project's +main build). That combination — common in real C/C++ repos, where a hand-written Makefile +drives the main build and CMake is used only for a test target — turns out to be exactly +where this demo step breaks, as below. + +**What actually happened**, running `detect_ladder.py --repo . --json` fresh (no +`--existing`, matching a first-time client demo) against this repo: + +```json +{ + "toolchains_detected": ["c-cmake", "node-ts", "python"], + "manifests": { + "python": ["benchmarks/requirements.txt"], + "node-ts": ["frontend/package.json"], + "c-cmake": ["tests/CMakeLists.txt"] + }, + "proposals": { + "static": [ + {"toolchain": "c-cmake", "command": "clang-tidy -p build $(git ls-files '*.c' '*.cc' '*.cpp' '*.h' '*.hpp')", "source": "hint:c-cmake (manifest: tests/CMakeLists.txt)"}, + {"toolchain": "c-cmake", "command": "cppcheck --error-exitcode=1 .", "source": "hint:c-cmake (manifest: tests/CMakeLists.txt)"}, + ... + ], + "typecheck": [ + {"toolchain": "c-cmake", "command": "cmake --build build", "source": "hint:c-cmake (manifest: tests/CMakeLists.txt)"}, + ... + ], + "unit": [ + {"toolchain": "c-cmake", "command": "ctest --test-dir build --output-on-failure", "source": "hint:c-cmake (manifest: tests/CMakeLists.txt)"}, + ... + ] + }, + "detection_status": "ok" +} +``` +(node-ts/python entries omitted above for length — `frontend/package.json` and +`benchmarks/requirements.txt` are real, tracked files in this repo; a genuine web frontend +and a Python benchmark harness exist alongside the C99 core, so those two detections are +correct, not noise.) + +**This is real progress over 8.0.0** — at that version this exact repo produced +`toolchains_detected: []` across the board (this session's own GR-03 finding). At 9.0.7, +`detect_manifests()` correctly finds `tests/CMakeLists.txt` via a now-recursive shallow +search (the fix for GR-03/GR-15's root-only-manifest sub-bug, confirmed live). **But the +specific, celebrated moment — "watch the tool propose exactly those [Makefile] commands +back" — does not happen.** Not one of the `c-cmake` proposals above (`clang-tidy`, +`cppcheck`, `cmake --build build`, `ctest`) is a command this repo's own `makefile` defines +or that a maintainer of this repo would actually run. This repo's real, canonical build/test +commands (`make check-c99`, `make check-packed-aware`, `cd tests/build && cmake .. && make +&& for t in *_tests; do ./$t; done`) never appear anywhere in the output. + +**Root cause, traced directly in `detect_ladder.py`, not inferred from behavior**: + +1. `propose_from_makefile()` — the function that actually parses real Makefile target names + via `_MAKE_TARGET_RE` and proposes `make ` commands, which is the mechanism + Session 1's script is describing — is only ever called when + **`if "c-cmake" not in manifests:`** (`detect_ladder.py:869`). Its own docstring gives the + reasoning: *"a CMake project's own generated or checked-in Makefile is a build artifact + CMake wrote, not a hand-authored signal of intent."* That reasoning is correct for a repo + where CMake generated the Makefile. It is **wrong** for this repo's actual shape: the + `tests/CMakeLists.txt` GR-03/GR-15 fix now (correctly) detects is a small, separate, + test-only CMake project nested one directory down — it did not generate the real, + hand-written, 600-line top-level `makefile`, which predates it and is the thing this + project's own `SPEC.md` names as the canonical build. Finding *any* CMakeLists.txt + anywhere in the tree is being read as "this whole repo is CMake's," which is false here. +2. Verified directly, bypassing the gate: `propose_from_makefile()` called on this repo in + isolation returns `{}` anyway. `MAKE_PHASE_HINTS` only recognizes generic target names + (`"lint"`, `"check-style"`, `"test"`, `"tests"`, `"check"`, `"typecheck"`, ...) — this + repo's real targets (`check-c99`, `check-packed-aware`, `check-array-exactness`, + `check-nd-surfaces`, ...) are all domain-specific compound names that match none of the + hints. So even with the gate removed, this specific repo's Makefile still would not have + produced the demo moment — a second, independent reason, not a duplicate of the first. + +**A smaller, genuinely separate bug found while tracing this**: `parse_makefile_targets()` +produces a spurious literal `"\\"` in its target set for this repo. Root cause: this repo's +`.PHONY:` declaration spans three physical lines via trailing backslash continuation +(`makefile:600-602`); the parser only reads the first physical line of a `.PHONY:` block and +splits it on whitespace, so the line's own trailing `\` becomes a "target name." Low +severity — the targets listed only on the continuation lines (`check-array-exactness`, +`check-nd-surfaces`, etc.) are still captured correctly via their own separate `target:` +rule lines elsewhere in the file, so nothing is actually lost from the final proposal set — +but it is a real, reproducible data-quality defect in the parser, not a hypothetical. + +**Falsifiable-in-the-room claim: does it hold?** **No, for this repo, for two independently +confirmed reasons** — not "it depends," not "sometimes." A room given this repo to try live +would watch the tool propose `cmake --build build` and `ctest --test-dir build`, then watch +someone in the room try `cmake --build build` from the repo root and get an immediate, +visible error (no root `CMakeLists.txt` exists to build). That is a worse outcome for the +"survives scrutiny" bar Session 1 explicitly sets for itself than an honest `FAILED` would +have been. + +**Where a room would get confused rather than blocked**: this is the sharper finding than +"it doesn't work." A clean `FAILED` (see Step 2 below) is not confusing — it is Session 1's +own second-half demo, and the room is primed to expect and respect it. What actually happens +here is worse: `detection_status: "ok"`, five plausible-looking, correctly-sourced-and-cited +proposals, and a *wrong* toolchain confidently in front of the room. Nothing in the output +signals "these commands don't actually build this repo" — that only surfaces if someone in +the room actually runs one. A facilitator who does not already know this repo's real build +shape would have no reason to suspect anything before that moment, and the failure, when it +lands, lands as "the tool was wrong," not "the tool was honest about not knowing" — precisely +the distinction Session 1's own text says matters. + +### Step 2 — the second half: a genuinely unrecognizable directory + +**What the facilitator is told to do**: run detection again against "a directory with +genuinely nothing recognizable in it," and expect a loud `FAILED`, never a silent +`not-configured`-looking empty result. + +**What actually happened**, against a freshly created empty directory: +```json +{ + "toolchains_detected": [], "manifests": {}, "proposals": {"static": [], "typecheck": [], "unit": [], "integration": []}, + "detection_status": "FAILED — no manifest, CI citation, recognized build file, or spec/docs hint matched anything in this repo. Checked for: *.tf, .flake8, CMakeLists.txt, Cargo.toml, Gemfile, build.gradle, build.gradle.kts, go.mod, mypy.ini, package.json, pom.xml, pyproject.toml, pytest.ini, requirements.txt, ruff.toml, setup.cfg, setup.py, tox.ini, Makefile, makefile, GNUmakefile. Searched the repo root and its immediate subdirectories... This is NOT the same as a correctly empty repo — it means this script has no name for whatever toolchain is actually here... Do not present this to a human as an ordinary not-configured answer." +} +``` + +**This half works exactly as advertised.** The message is loud, names precisely what it +checked, states its own search depth, and explicitly instructs against presenting it as a +benign result. Confirmed by execution, not by reading the source's stated intent. No +confusion risk here — this is a genuinely good, room-safe moment. + +### Step 3 — `/ais:guide-me` cold + +**What the facilitator is told to do**: run it with no arguments and expect one line naming +version, install path, and freshness before anything else. + +**What actually happened**: `ais v9.0.7 — installed at /private/tmp/ais_demo (source: +unknown) — up to date with origin/main (as of last fetch)`. Works exactly as advertised — a +single, immediate, informative line. `(source: unknown)` is an artifact of testing against a +bare `git clone` rather than a real plugin install; a genuine client install would resolve +this to `plugin` or `git`, not a defect in the tool itself. + +--- + +## Wall-clock, against the fifteen minutes Session 1 claims + +Running the two commands Session 1 actually asks a facilitator to run (`detect_ladder.py` +twice, `version_info.py` once) takes seconds — well inside the fifteen-minute budget on +raw execution time alone, exactly as the doc implies. The fifteen minutes this session +actually spent went entirely into tracing *why* Step 1 didn't do what the script says it +would, which a live facilitator would not do in the room — they would either not notice +(the confusion-not-blocking risk above) or would notice and have no ready explanation, +since nothing in the tool's own output names the CMake-detection gate as the reason its +Makefile-target-reading branch never ran. + +--- + +## The question that matters most: is the zero-signal case from 8.0.0 still this repo's case? + +**No, partially — and the partial answer is the finding, not either extreme.** + +`toolchains_detected` is no longer empty; `detect_manifests()` genuinely and correctly finds +real signal in this repo now (`tests/CMakeLists.txt`, `frontend/package.json`, +`benchmarks/requirements.txt`), which is real, verified progress since 8.0.0 and directly +attributable to the GR-03/GR-15 fix. The specific "five minutes when DETECT has nothing to +draft" framing this session wrote yesterday needs one more branch, not a retraction: DETECT +can now produce **a draft that exists but is wrong for this specific repo's actual build +system**, sourced from a real file, correctly cited, and confidently marked `"ok"` — which +is arguably a harder case for a live demo to recover from gracefully than the honest, loud +`FAILED` this repo used to produce at 8.0.0. The training material's durability argument +still holds either way (a human confirming or correcting a wrong draft is still cheaper long +-term than nothing at all, and still produces a durable, re-runnable, honest record) — but +"detection produces nothing" and "detection produces a confident, wrong, well-cited draft" +are two different demo failure modes, and this repo, today, at 9.0.7, is now firmly the +second one, not the first. diff --git a/docs/exemplars/teaching-session3-rehearsal.md b/docs/exemplars/teaching-session3-rehearsal.md new file mode 100644 index 000000000..69d6ceab6 --- /dev/null +++ b/docs/exemplars/teaching-session3-rehearsal.md @@ -0,0 +1,200 @@ +# Session 3 Rehearsal: `docs/TEACHING.md`'s Own Weakness List, Checked + +**Kit version checked against: `9.0.7`, commit `288bdd55f7076bfc9a993225e80affcb88a8e738`** +(same clone as `teaching-session1-rehearsal.md`, in this same repo). Read-only against the +kit repo throughout, as instructed. + +**First correction to the request itself, stated up front rather than buried**: Session 3 +currently lists **five** weak items, not four. A fifth — idea-stage fabrication +(`feature-discovery` → `idea-to-prd` → `prd-to-epic`) — is present in the live text and is +not in the four summarized in the request. Checked below alongside the other four. + +--- + +## Item 1 — handoff citations not verified + +**Session 3's current text**: states the weak claim as "nothing mechanically checks a +citation against the file it names before writing it," tells the reader to "confirm this +yourself by reading `commands/handoff.md` at the current commit," and separately names the +provenance-tagging fix as the thing to demo instead. + +**Checked**: `grep` for any citation-verification logic in `commands/handoff.md` — zero +hits. The weak claim is still true, verified directly, not assumed from the doc's own +wording. + +**On the rewording specifically, since this is the one the request flagged as most +important**: **it landed, and it landed correctly.** The live Session 3 text does **not** +assert the "nine of nine wrong" anecdote to the room at all — it never appears in +`docs/TEACHING.md`'s own prose. It points instead at `OPEN-QUESTIONS.md` item G-10 as +supporting material and, separately, at the directly-checkable claim (no verification step +exists). A room following the text as written will not be handed an anecdote to repeat. + +**But the anecdote itself is worth knowing about, one level down, because another +session's own audit already found the exact problem the request was worried about — and I +verified that audit's own claim rather than just relaying it.** `OPEN-QUESTIONS.md` (around +line 1167) contains a self-audit, dated the same night as this one: it states plainly that +the two incidents the kit's own artifacts cite as motivating cases — this "nine of nine" +handoff and a research doc's "confirmed directly with the maintainer" line — are **not +independently locatable** in the repo's current state, and that `ADR-0006` "names no file +path for the case it describes." I re-verified this claim myself rather than take it as +settled: `grep -rn "9 of 9"` across every tracked `.md` file in the repo returns exactly the +one line inside that same self-audit paragraph — the incident is not cited anywhere else as +a standalone artifact. + +**One thing worth telling Michael that this self-audit gets slightly wrong, or at least +states more broadly than the evidence supports.** The audit treats both cited incidents as +equally unlocatable. They are not the same case. The "confirmed directly with the +maintainer" incident is **this session's own GR-12 finding**, from earlier tonight — a real +event, verified directly against a real `AskUserQuestion` exchange in this session's own +transcript, that led to a real, shipped fix (the four-state provenance vocabulary, confirmed +present in all three named files below). It is not written to disk as a standalone +`thoughts/shared/research/` artifact, which is exactly what the self-audit checked for and +correctly found absent — but "not committed as a standalone file" and "not independently +verifiable" are different claims, and this one is verifiable, just not by grepping the repo. +The "nine of nine" handoff incident may be the same situation (a real test case run live, +never saved as a standalone artifact) or it may genuinely be unlocatable even in that +sense — I have no way to tell which from here, and neither does the self-audit, which is +exactly why it hedges rather than asserts. Worth saying to whoever owns this: "not in the +repo" is not automatically "did not happen," and conflating the two in the audit's own +prose is a smaller version of the identical problem the audit exists to catch. + +**Provenance vocabulary landing, checked directly rather than trusted from that same +self-audit**: `grep -l` for the four terms (`stated-by-human`, `chosen-from-options`, +`accepted-default`, `model-inferred`) across `commands/handoff.md`, `commands/create-plan.md`, +and `skills/grill-me/SKILL.md` — all three match. Confirmed independently. + +**Verdict: still true, correctly reworded, demo substitute genuine.** + +--- + +## Item 2 — `verify-implementation`'s diff range can be empty at the moment it's first run + +**Checked directly against `commands/verify-implementation.md`**: the diff-range logic +(`git merge-base HEAD origin/main`, `HEAD~1` fallback, `EMPTY DIFF` reported honestly rather +than a false pass) is unchanged from what `OPEN-QUESTIONS.md`'s G-13/G-14 entry describes. +That entry is itself still open (no "fixed" marker, ends with "Fix needs either..."), +including a sharper live-reproduced sub-case beyond the original claim: on a branch already +caught up with `origin/main`, `merge-base` resolves to `HEAD` itself, so even the documented +`HEAD~1` fallback never fires, and the tool still doesn't name "already caught up" as its own +empty-range case. + +**Demo substitute** ("run it after the first real commit exists... show the ladder's +per-phase output there"): genuine and matches this session's own experience exactly — every +`/verify-implementation`-style check run tonight (both graph tickets) was run after a real +commit existed, and the per-phase ladder output was the actual load-bearing signal both +times, diff-range timing never came up as an issue in practice. + +**Verdict: still true, substitute genuine and independently corroborated by this session's +own unrelated work tonight.** + +--- + +## Item 3 — telemetry measures kit adoption, not all agentic work + +**Checked**: `docs/AGENTIC_LADDER.md:62` states this limit in almost these exact words — "The +scope limit: this measures adoption within the toolkit, and nothing beyond it" — and line 42 +repeats it for one specific excluded-events count. `commands/telemetry-report.md` exists as +the named demo substitute. + +**Verdict: still true, stated plainly in the kit's own material, demo substitute real.** + +--- + +## Item 4 — `docs-site`'s dangling-reference detector, known bug, pinned not fixed + +**Checked**: `OPEN-QUESTIONS.md`'s G-28 entry confirms this is still open, still +deliberately unfixed (the reasoning given — rewording the source to dodge a detector bug +would hide the bug rather than leave it visible — matches Session 3's own framing exactly). + +**One thing Session 3's phrasing undersells, worth flagging rather than letting pass**: the +live G-28 entry is headed *"Widened 2026-08-22, still not fixed... Now 5 pinned false +positives, not 2."* The bug's surface has grown since it was first named, not stayed flat. +Session 3's text ("a good example of the kit choosing a visible known issue over a quiet +workaround") is accurate about the *policy* being sound, but doesn't convey that the +*count* is trending up, not just sitting pinned. A room that hears "known, pinned, stable" +and later learns it went from 2 to 5 in one day will read that as something being +downplayed, even if the underlying decision (don't dodge the detector) was the right one +both times. + +**Verdict: still true; the growth trend is a real, small understatement worth correcting in +the material.** + +--- + +## Item 5 — idea-stage fabrication (not in the four the request named) + +**Checked**: `OPEN-QUESTIONS.md` around line 1077 has an entry headed *"OPEN (2026-08-23, +cold end-to-end run of the idea stage) — a fourth failure axis: fabricated"* — dated the same +night as everything else tonight, describing a fresh, independently-run cold test that +produced exactly what Session 3's text now describes: invented Success Criteria numbers (a +90% figure, a 100% bar) with no traceable source, and a full tech-stack assertion against a +directory confirmed empty. Marked `OPEN`, i.e. not yet fixed as of this commit. + +**Verdict: real, current, freshly added — and the single most severe item on the list by +this session's own judgment, see below.** + +--- + +## The judgment asked for directly, not relayed from anyone else + +**Is it the right list?** Four of the five items are accurate, verifiable, and each names a +real demo substitute that actually works — that's a genuinely well-maintained list, checked +line by line rather than trusted. The fifth (idea-stage fabrication) is the right item to +lead with if this session had to rank them: it is the only one where the kit doesn't just +have an unverified mechanism (item 1) or a timing edge case (item 2) or a scope boundary +(item 3) or a cosmetically-noisy-but-harmless detector bug (item 4) — it is the only one +where the kit **actively invents specific, false numbers and presents them with full +confidence**, in the artifact type (a PRD/epic) most likely to reach a client's hands +unreviewed. Session 3's own text already treats it this way ("the one most worth naming +before anyone runs it live") — that ranking is correct, independently confirmed. + +**What I would add, from my own night's findings, that this list does not mention at all:** + +1. **The kit's own commands are not invocable by an autonomous agent, at all, mid-session, + after install — not just the `/reload-plugins` footnote item 1 alludes to, the whole + thing.** This session installed the plugin successfully, confirmed it enabled at the + correct version, and then could not dispatch a single `ais`-provided command or skill for + the rest of the session — not `/research-codebase`, not `/create-plan`, not + `/implement-plan`, not `/verify-implementation`, nothing. Every one of those was instead + read from the plugin's own cache directory as a markdown file and followed by hand. This + is now documented in the kit's own `README.md` (found during a separate re-check + tonight) with the exact right instruction for an agent that hits it — "there is no retry + that fixes it from inside the session... escalate to a human at an actual terminal" — but + it is not in Session 3's own list of named weaknesses, and it is arguably a bigger one + than any single item currently there: it means the entire RPI+plan-review loop this + session ran twice, tonight, on a real codebase, only works with a human physically + re-invoking each step at an actual keyboard. A room evaluating this kit for "automatic + RPI" (the goal named in this project's own orchestrator handoff document) deserves to + hear that plainly, in the same room where the other five limits get named. + +2. **A confidently-wrong proposal, found this afternoon, in the exact demo this list is + meant to set up for.** `docs/exemplars/teaching-session1-rehearsal.md` (this repo, + committed earlier today) found that Session 1's own opening demo — DETECT reading a bare + Makefile's real targets back to the room — does not fire on this specific repo, and + instead proposes CMake/ctest/clang-tidy commands that do not build or test it at all, + sourced from a real but structurally-misleading manifest match. That is the same failure + *shape* as item 5 (confident, well-cited, wrong) happening one session earlier in the + pipeline than where Session 3 currently looks for it. If item 5 is worth a dedicated + weakness entry because it fabricates with confidence, this deserves the same treatment, + not just a rehearsal note filed separately. + +3. **"Assertions nothing verifies" is broader than item 1's citation-checking scope — it + recurs in the plan-review pipeline too, with no citation involved at all.** Item 1 is + scoped to `handoff.md`'s `file:line` citations specifically. This session found the + identical failure *shape* somewhere item 1 doesn't look: a plain factual claim ("8 + builtins share this structure") stated in one ticket's research doc, repeated in its + plan, **surviving that plan's own adversarial `plan-reviewer` pass** (which checked the + shape of the defect it was sent to find, not every incidental number riding along with + it), then carried forward unverified into a second ticket's research and plan, and only + caught when a second, independent review pass happened to re-derive the number from + source by chance. Nothing here involves a citation at all — it's a stated fact that + simply never got re-checked, twice, across two already-reviewed documents. Worth its own + line: the pattern is "an asserted fact survives review because review checks what it was + pointed at, not everything riding along with it," and citation-verification (item 1) is + only the narrowest instance of it. + +**Is anything on the list presented as smaller than it actually is?** One clear case, named +above: item 4's phrasing reads as "known, pinned, stable" when the live entry it's sourced +from says the count grew from 2 to 5 in one day. Everything else checked out at the size the +list already gives it — this was the one place where the live source told a slightly +different story than the summary in front of the room would. diff --git a/docs/spec/builtins/graphs.md b/docs/spec/builtins/graphs.md index 4db069f96..e4d14272c 100644 --- a/docs/spec/builtins/graphs.md +++ b/docs/spec/builtins/graphs.md @@ -16,25 +16,39 @@ expressions. Because graphs are plain expressions, generic tools (`Part`, `Map`, `ReplaceAll`, …) work on them, and `AdjacencyMatrix[g]` returns a dense `List`-of-`List`s consumable directly by `Det`, `Tr`, and `Eigenvalues`. +A graph may optionally carry per-edge weights via a third constructor +argument, `Graph[v, e, EdgeWeight -> {w1, ..., wm}]` — see `EdgeWeight` and +`WeightedAdjacencyMatrix` below. + **MVP scope (locked):** simple graphs only — no parallel edges, no self-loops, -no edge tags, no multigraphs, no hypergraphs, and no edge/vertex weights. -`WeightedAdjacencyMatrix` and edge weights are a documented future extension. +no edge tags beyond `EdgeWeight`, no multigraphs, no hypergraphs, and no +vertex weights. Weighted shortest-path/distance and derived-vertex weighted +construction (`Graph[e, EdgeWeight -> {...}]`, no explicit vertex list) remain +out of scope. ## Graph A graph value. - `Graph[v, e]`: a graph with vertex list `v` and edge list `e`. - `Graph[e]`: derives the vertex set from the edges, in first-appearance order (directed by default). +- `Graph[v, e, EdgeWeight -> {w1, ..., wm}]`: a weighted graph — `wi` is the + weight of `e[[i]]`, matched by position. Requires the explicit-vertex form; + `Graph[e, EdgeWeight -> {...}]` (derived vertices) is not accepted. A weight + list whose length doesn't match `e` is malformed, same as any other + rejection below. On construction the edge list is normalized and validated, producing the -canonical `Graph[List[verts], List[edges]]`: +canonical `Graph[List[verts], List[edges]]` (or, when weighted, +`Graph[List[verts], List[edges], EdgeWeight -> List[weights]]`): - `u -> v` (`Rule`) and `DirectedEdge[u, v]` become `DirectedEdge[u, v]`. - `u <-> v` (`TwoWayRule`) and `UndirectedEdge[u, v]` become `UndirectedEdge[u, v]`. Malformed input is left unevaluated: self-loops, parallel/duplicate edges, -3-argument edges, or an edge endpoint absent from an explicit vertex list. -(Anti-parallel directed edges `u -> v` and `v -> u` are distinct and allowed.) +3-argument edges, an edge endpoint absent from an explicit vertex list, or (for +a weighted graph) an `EdgeWeight` list whose length doesn't match the edge +list. (Anti-parallel directed edges `u -> v` and `v -> u` are distinct and +allowed.) Printing: in standard output a graph shows a terse summary, `Graph[]`. `InputForm` and `FullForm` print the literal @@ -75,12 +89,16 @@ non-graph argument. adds to both in- and out-degree of each endpoint. - `DirectedGraphQ[g]` — `True` iff `g` is a valid graph whose edges are all directed. +- `EdgeWeight[g]` — the weights of `g`'s edges, in `EdgeList` order. Defaults + to all `1`s when `g` was built without an `EdgeWeight` option. ``` VertexList[Graph[{1,2,3,4},{1->2,2->3,3->4,4->1}]] (* {1, 2, 3, 4} *) EdgeCount[Graph[{1,2,3,4},{1->2,2->3,3->4,4->1}]] (* 4 *) VertexDegree[Graph[{1,2,3},{1<->2,2<->3}]] (* {1, 2, 1} *) AdjacencyList[Graph[{1,2,3},{1<->2,2<->3}], 2] (* {1, 3} *) +EdgeWeight[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]] (* {5, 7} *) +EdgeWeight[Graph[{1,2,3},{1->2,2->3}]] (* {1, 1} *) ``` ## Matrix views (linear-algebra interop) @@ -93,15 +111,21 @@ AdjacencyList[Graph[{1,2,3},{1<->2,2<->3}], 2] (* {1, 3} *) - `AdjacencyGraph[m]` — the inverse of `AdjacencyMatrix`: builds a graph on vertices `1..n` from a 0/1 matrix (undirected if `m` is symmetric, else directed). `AdjacencyGraph[AdjacencyMatrix[g]]` reproduces `g`'s edges. +- `WeightedAdjacencyMatrix[g]` — like `AdjacencyMatrix[g]`, but each nonzero + entry is the corresponding edge's weight instead of `1` (`0` where there is + no edge). Equal to `AdjacencyMatrix[g]` exactly when `g` has no + `EdgeWeight` (every weight defaults to `1`). ``` AdjacencyMatrix[Graph[{1,2,3,4},{1->2,2->3,3->4,4->1}]] (* {{0,1,0,0},{0,0,1,0},{0,0,0,1},{1,0,0,0}} *) Det[AdjacencyMatrix[Graph[{1,2,3,4},{1->2,2->3,3->4,4->1}]]] (* -1 *) +WeightedAdjacencyMatrix[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]] + (* {{0,5,0},{0,0,7},{0,0,0}} *) +WeightedAdjacencyMatrix[CycleGraph[4]] == AdjacencyMatrix[CycleGraph[4]] (* True *) ``` -*A future `WeightedAdjacencyMatrix` would carry edge weights instead of 0/1; -not implemented in the MVP.* +`FindShortestPath`/`GraphDistance` are weight-aware — see Search & computation below. ## Generators @@ -112,6 +136,8 @@ constructor path: - `CycleGraph[n]` — the cycle on `1..n`. - `PathGraph[n]` — the path `1-2-...-n`; `PathGraph[{v1,...}]` uses the given vertices. +- `StarGraph[n]` — the star on `1..n`: the hub `1` joined to each of the `n-1` + leaves `2..n`. - `RandomGraph[{n, m}]` — a random undirected graph with `n` vertices and `m` distinct edges (uses the seeded system RNG, so `SeedRandom` makes it reproducible). Returns unevaluated if `m` exceeds `n(n-1)/2`. @@ -120,16 +146,25 @@ constructor path: EdgeCount[CompleteGraph[5]] (* 10 *) EdgeList[CycleGraph[4]] (* {1<->2, 2<->3, 3<->4, 4<->1} *) VertexDegree[PathGraph[5]] (* {1, 2, 2, 2, 1} *) +EdgeList[StarGraph[4]] (* {1<->2, 1<->3, 1<->4} *) +VertexDegree[StarGraph[5]] (* {4, 1, 1, 1, 1} *) ``` ## Search & computation -All are unweighted and build an integer-indexed adjacency on demand. +All build an integer-indexed adjacency on demand; all but `FindShortestPath`/`GraphDistance` +are unweighted. - `FindShortestPath[g, s, t]` — a shortest path from `s` to `t` as a vertex - list (BFS; follows edge direction for directed graphs); `{}` if `t` is - unreachable. -- `GraphDistance[g, s, t]` — the length of that path; `Infinity` if unreachable. + list; `{}` if `t` is unreachable. **Weight-aware**: if `g` carries an + `EdgeWeight` and every weight is non-negative and numeric, uses Dijkstra + (minimum total weight); otherwise (unweighted, a symbolic weight, or a + negative weight present) uses unweighted BFS (minimum hop count), + following edge direction for directed graphs either way. +- `GraphDistance[g, s, t]` — the length/total weight of that path; + `Infinity` if unreachable. Same weight-aware dispatch as `FindShortestPath`, + and returns an exact value (`Integer`/`Rational`) whenever the weights are + exact — never a `Real` artifact of the internal algorithm. - `ConnectedComponents[g]` / `WeaklyConnectedComponents[g]` — components of the underlying undirected graph. - `StronglyConnectedComponents[g]` — components following edge directions @@ -146,8 +181,22 @@ FindShortestPath[Graph[{1,2,3,4},{1->2,2->3,3->4}], 1, 4] (* {1, 2, 3, 4} *) GraphDistance[Graph[{1,2,3,4},{1->2,2->3,3->4}], 4, 1] (* Infinity *) StronglyConnectedComponents[Graph[{1,2,3},{1->2,2->3}]] (* {{1},{2},{3}} *) VertexConnectivity[CycleGraph[5]] (* 2 *) + +(* Weighted: the direct 1->4 edge (weight 10) loses to the longer, + cheaper 1->2->3->4 route (weight 3). *) +FindShortestPath[ + Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}], 1, 4] + (* {1, 2, 3, 4} *) +GraphDistance[ + Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}], 1, 4] + (* 3 *) ``` +Weighted `FindShortestPath`/`GraphDistance` use a plain O(V²) Dijkstra (no priority queue — +consistent with `VertexConnectivity`'s own small-graph exact-algorithm precedent above), and +fall back to unweighted BFS rather than erroring whenever a weight isn't usable for it (not +present, symbolic, or negative). No Bellman-Ford / negative-weight support. + ## Visualization - `GraphPlot[g]` — a `Graphics[...]` object drawing `g`: vertices are laid out diff --git a/docs/spec/changelog/2026-08-17.md b/docs/spec/changelog/2026-08-17.md index 9efc93c58..db1308343 100644 --- a/docs/spec/changelog/2026-08-17.md +++ b/docs/spec/changelog/2026-08-17.md @@ -2,6 +2,49 @@ Feature additions and fixes recorded during this week. +## Graph: weighted `FindShortestPath`/`GraphDistance` (Dijkstra) (2026-08-23) + +`FindShortestPath[g,s,t]` and `GraphDistance[g,s,t]` are now weight-aware, completing the +follow-up the edge-weights ticket (below) named as deferred in its own Non-goals. Both +dispatch to a plain O(V²) Dijkstra (`src/graph/shortestpath.c`, a new local weighted +adjacency independent of the shared `GraphAdj` structure — deliberately, to avoid widening a +structure 5 other builtins depend on) whenever `g` carries an `EdgeWeight` and every weight +is non-negative and numeric (`graph_weights_usable`, reusing the codebase's existing +`expr_is_numeric_like`); falls back to the pre-existing unweighted BFS in every other case +(no `EdgeWeight`, a symbolic weight, or a negative weight), so no previously-working call +regresses. `GraphDistance` returns an exact `Integer`/`Rational` whenever the inputs are +exact — the internal `double`-based Dijkstra is used only to select the path, never to +produce the returned value, which is reconstructed via `Plus[]` over the real edge weights +along the discovered path. An adversarial plan-review pass caught two real defects before +implementation: a raw `double` accumulator would have printed an inexact `12.` against an +exact `12` expectation, and the plan wrongly claimed both builtins' existing regression +tests needed updating when only `GraphDistance`'s does (the test graph in question has only +one path, so `FindShortestPath` was already correct). See +`thoughts/shared/plans/2026-08-23-weighted-shortest-path.md`. + +## Graph: edge weights, `EdgeWeight`, `WeightedAdjacencyMatrix` (2026-08-23) + +Implemented the one extension the graph subsystem's own MVP-scope note already flagged as +pre-approved future work (`src/graph/adjmat.c:9`, `docs/spec/builtins/graphs.md`). `Graph[v, +e, EdgeWeight -> {w1, ..., wm}]` accepts an optional third constructor argument attaching a +weight to each edge, matched by position; a length mismatch is malformed and left +unevaluated, same as any other rejection. Two new builtins: `EdgeWeight[g]` (the weights in +`EdgeList` order, defaulting to all `1`s when `g` carries none) and +`WeightedAdjacencyMatrix[g]` (like `AdjacencyMatrix[g]` but filled with each edge's weight +instead of a literal `1`; identical output to `AdjacencyMatrix[g]` for an unweighted graph). +Fully additive — the existing 2-arg `Graph[v,e]` canonical form and all 27 pre-existing +builtins are unaffected. An adversarial plan-review pass during design caught a real defect +before implementation: `graph_build_adj` (`src/graph/graph_util.c`) is a second, independent +validation choke point that 8 of those 27 builtins (`ConnectedComponents`, +`WeaklyConnectedComponents`, `ConnectedGraphQ`, `VertexConnectivity`, `FindSpanningTree`, +`FindShortestPath`, `GraphDistance`) route through directly, bypassing `graph_is_valid` +entirely — both now share one `graph_shape_ok` helper so neither can silently reject a +weighted graph the other accepts. Weighted shortest-path/distance and derived-vertex weighted +construction (`Graph[e, EdgeWeight -> {...}]`) remain explicitly out of scope. No +packed/NDArray or `Compile[]` surface — consistent with `AdjacencyMatrix`/`IncidenceMatrix`, +neither of which has one either (structural over a `Graph` tree, not elementwise over a +numeric buffer). See `thoughts/shared/plans/2026-08-22-graph-edge-weights.md`. + ## Build: Linux LAPACK/BLAS link fix — plain `make` on Ubuntu (2026-08-21) Fixed undefined-reference link failures to Fortran LAPACK (`dgeev_`, `zgesv_`, diff --git a/docs/spec/changelog/2026-08-31.md b/docs/spec/changelog/2026-08-31.md new file mode 100644 index 000000000..20ded674b --- /dev/null +++ b/docs/spec/changelog/2026-08-31.md @@ -0,0 +1,23 @@ +# Changelog: week of 2026-08-31 (Mon) – 2026-09-06 (Sun) + +## StarGraph + +`StarGraph[n]` joins the basic graph constructors in `src/graph/generators.c`, +beside `CompleteGraph`, `CycleGraph`, `PathGraph` and `RandomGraph`. It gives the +star on vertices `1..n`: the hub `1` joined to each of the `n-1` leaves `2..n`, +undirected, `n-1` edges. + +``` +EdgeList[StarGraph[4]] (* {1<->2, 1<->3, 1<->4} *) +VertexDegree[StarGraph[5]] (* {4, 1, 1, 1, 1} *) +EdgeCount[StarGraph[5]] (* 4 *) +``` + +The hub is vertex `1`, matching Mathematica. `StarGraph[1]` is a lone vertex and +`StarGraph[0]` the empty graph; a non-integer or negative argument leaves the +expression unevaluated, as with the sibling generators. Attributes: `Protected`. +Docstring registered via `symtab_set_docstring`. + +Tested in `tests/test_graph.c` (`test_generators`): counts, `EdgeList`, degree +sequence, undirectedness, both degenerate sizes, connectivity, spanning-tree +edge count, and the symbolic-argument case. diff --git a/src/graph/construct.c b/src/graph/construct.c index 9bf4cb6ed..80a8c554d 100644 --- a/src/graph/construct.c +++ b/src/graph/construct.c @@ -1,8 +1,15 @@ /* construct.c - builtin_graph: normalize, derive, validate, canonicalize. * * Accepts: - * Graph[edges] -- vertices derived from the edges (directed default) - * Graph[verts, edges] -- explicit vertex list + * Graph[edges] -- vertices derived from the edges (directed default) + * Graph[verts, edges] -- explicit vertex list + * Graph[verts, edges, EdgeWeight -> {w1, ..., wm}] + * -- explicit vertex list + per-edge weights, matched + * to `edges` by position; wrong length is malformed + * (left unevaluated), same as any other rejection + * below. Weighted graphs require the explicit-vertex + * form -- Graph[edges, EdgeWeight -> {...}] is not + * accepted (deliberately out of scope; see the plan). * * Edge sugar is normalized on construction: * Rule[u,v] / u -> v -> DirectedEdge[u, v] @@ -51,23 +58,44 @@ static Expr* normalize_edge(const Expr* e) { return expr_new_function(expr_new_symbol(out_head), args, 2); } +/* True iff `opt` is Rule[EdgeWeight, List[...]] -- shape only, length is + * checked by the caller once the edge count is known. */ +static int is_edge_weight_rule(const Expr* opt) { + if (!opt || opt->type != EXPR_FUNCTION || opt->data.function.arg_count != 2) + return 0; + const char* h = fn_head(opt); + if (h != SYM_Rule) return 0; + const Expr* key = opt->data.function.args[0]; + return key && key->type == EXPR_SYMBOL && key->data.symbol.name == SYM_EdgeWeight + && graph_is_list(opt->data.function.args[1]); +} + /* Assemble the canonical Graph from `res`, or NULL if the shape is wrong or the * result would be invalid. */ static Expr* try_build_canonical(Expr* res) { size_t argc = res->data.function.arg_count; const Expr* verts_in = NULL; const Expr* edges_in = NULL; + const Expr* weight_opt = NULL; if (argc == 1) { edges_in = res->data.function.args[0]; } else if (argc == 2) { verts_in = res->data.function.args[0]; edges_in = res->data.function.args[1]; + } else if (argc == 3) { + verts_in = res->data.function.args[0]; + edges_in = res->data.function.args[1]; + weight_opt = res->data.function.args[2]; + if (!is_edge_weight_rule(weight_opt)) return NULL; } else { return NULL; } if (!graph_is_list(edges_in)) return NULL; if (verts_in && !graph_is_list(verts_in)) return NULL; + if (weight_opt && weight_opt->data.function.args[1]->data.function.arg_count + != edges_in->data.function.arg_count) + return NULL; /* weight/edge count mismatch */ size_t ne = edges_in->data.function.arg_count; @@ -113,13 +141,30 @@ static Expr* try_build_canonical(Expr* res) { graph_vidx_free(seen); } - /* 3. Assemble candidate Graph[List verts, List edges] (moves ownership). */ + /* 3. Assemble candidate Graph[List verts, List edges(, EdgeWeight -> List w)] + * (moves ownership). */ Expr* vlist = expr_new_function(expr_new_symbol(SYM_List), verts, nv); Expr* elist = expr_new_function(expr_new_symbol(SYM_List), edges, ne); free(verts); free(edges); - Expr* gargs[2] = { vlist, elist }; - Expr* g = expr_new_function(expr_new_symbol(SYM_Graph), gargs, 2); + Expr* g; + if (weight_opt) { + const Expr* win = weight_opt->data.function.args[1]; + size_t nw = win->data.function.arg_count; + Expr** weights = (nw > 0) ? calloc(nw, sizeof(Expr*)) : NULL; + if (nw > 0 && !weights) { expr_free(vlist); expr_free(elist); return NULL; } + for (size_t i = 0; i < nw; i++) + weights[i] = expr_copy(win->data.function.args[i]); + Expr* wlist = expr_new_function(expr_new_symbol(SYM_List), weights, nw); + free(weights); + Expr* wargs[2] = { expr_new_symbol(SYM_EdgeWeight), wlist }; + Expr* wrule = expr_new_function(expr_new_symbol(SYM_Rule), wargs, 2); + Expr* gargs[3] = { vlist, elist, wrule }; + g = expr_new_function(expr_new_symbol(SYM_Graph), gargs, 3); + } else { + Expr* gargs[2] = { vlist, elist }; + g = expr_new_function(expr_new_symbol(SYM_Graph), gargs, 2); + } /* 4. Validate (self-loops, parallel edges, endpoint membership). */ if (!graph_is_valid(g)) { expr_free(g); return NULL; } diff --git a/src/graph/edgeweight.c b/src/graph/edgeweight.c new file mode 100644 index 000000000..544b29cc7 --- /dev/null +++ b/src/graph/edgeweight.c @@ -0,0 +1,17 @@ +/* edgeweight.c - EdgeWeight[g]: the graph's per-edge weights, in EdgeList + * order. Defaults to List[1, 1, ..., 1] (one per edge) when g carries no + * EdgeWeight -- matching Wolfram Language's own behavior for an unweighted + * graph, and giving WeightedAdjacencyMatrix[g] a well-defined answer for + * every valid graph, not just ones explicitly built with weights. + * + * Memory (SPEC section 4): returns a fresh list; the evaluator frees res. + */ + +#include "graph.h" +#include "expr.h" + +Expr* builtin_edge_weight(Expr* res) { + if (res->data.function.arg_count != 1) return NULL; + const Expr* g = res->data.function.args[0]; + return graph_resolve_edge_weights(g); /* NULL (unevaluated) if g is not a valid graph */ +} diff --git a/src/graph/generators.c b/src/graph/generators.c index 54f63dc76..5803585f1 100644 --- a/src/graph/generators.c +++ b/src/graph/generators.c @@ -4,6 +4,7 @@ * CycleGraph[n] - undirected cycle on 1..n * PathGraph[n] - undirected path 1-2-...-n * PathGraph[{v1,...,vk}] - undirected path over the given vertices + * StarGraph[n] - undirected star: hub 1 joined to leaves 2..n * RandomGraph[{n, m}] - undirected graph with n vertices, m random edges * * Each assembles a Graph[List verts, List edges] expression and returns it; the @@ -32,11 +33,19 @@ static Expr* undirected_edge(long a, long b) { return expr_new_function(expr_new_symbol(SYM_UndirectedEdge), ea, 2); } -/* Wrap vertex/edge C-arrays into a Graph[...] (moves ownership). */ +/* Wrap a calloc'd Expr* array into a List[...]: the elements move into the new + * node (expr_new_function copies the pointers, not the array), so the array + * itself is ours to free. */ +static Expr* make_list_owning(Expr** items, size_t n) { + Expr* list = expr_new_function(expr_new_symbol(SYM_List), items, n); + free(items); + return list; +} + +/* Wrap vertex/edge C-arrays into a Graph[...] (moves ownership, frees both + * arrays). */ static Expr* make_graph(Expr** verts, size_t nv, Expr** edges, size_t ne) { - Expr* vlist = expr_new_function(expr_new_symbol(SYM_List), verts, nv); - Expr* elist = expr_new_function(expr_new_symbol(SYM_List), edges, ne); - Expr* gargs[2] = { vlist, elist }; + Expr* gargs[2] = { make_list_owning(verts, nv), make_list_owning(edges, ne) }; return expr_new_function(expr_new_symbol(SYM_Graph), gargs, 2); } @@ -100,6 +109,18 @@ Expr* builtin_path_graph(Expr* res) { return make_graph(int_vertices(n), (size_t)n, edges, ne); } +Expr* builtin_star_graph(Expr* res) { + if (res->data.function.arg_count != 1) return NULL; + long n = as_count(res->data.function.args[0]); + if (n < 0) return NULL; + /* Hub is vertex 1, joined to each of 2..n: exactly n-1 edges, with no + * duplicate possible at any size (unlike CycleGraph's wrap edge). */ + size_t ne = (n > 0) ? (size_t)n - 1 : 0; + Expr** edges = (ne > 0) ? calloc(ne, sizeof(Expr*)) : NULL; + for (long i = 2; i <= n; i++) edges[i - 2] = undirected_edge(1, i); + return make_graph(int_vertices(n), (size_t)n, edges, ne); +} + Expr* builtin_random_graph(Expr* res) { if (res->data.function.arg_count != 1) return NULL; const Expr* spec = res->data.function.args[0]; @@ -127,8 +148,6 @@ Expr* builtin_random_graph(Expr* res) { Expr* sampled = evaluate(sample_call); /* consumes sample_call */ if (!graph_is_list(sampled)) { expr_free(sampled); return NULL; } - Expr* gargs[2] = { expr_new_function(expr_new_symbol(SYM_List), - int_vertices(n), (size_t)n), - sampled }; + Expr* gargs[2] = { make_list_owning(int_vertices(n), (size_t)n), sampled }; return expr_new_function(expr_new_symbol(SYM_Graph), gargs, 2); } diff --git a/src/graph/graph.c b/src/graph/graph.c index 7f4c3d9f6..8c9addcac 100644 --- a/src/graph/graph.c +++ b/src/graph/graph.c @@ -81,6 +81,12 @@ void graph_init(void) { symtab_set_docstring("DirectedGraphQ", "DirectedGraphQ[g] gives True if all edges of g are directed."); + symtab_add_builtin("EdgeWeight", builtin_edge_weight); + symtab_get_def("EdgeWeight")->attributes |= ATTR_PROTECTED; + symtab_set_docstring("EdgeWeight", + "EdgeWeight[g] gives the weights of g's edges, in EdgeList order. " + "Defaults to all 1s if g was built without an EdgeWeight option."); + /* ---- Phase 3: matrix views (linalg interop) -------------------------- */ symtab_add_builtin("AdjacencyMatrix", builtin_adjacency_matrix); symtab_get_def("AdjacencyMatrix")->attributes |= ATTR_PROTECTED; @@ -88,6 +94,13 @@ void graph_init(void) { "AdjacencyMatrix[g] gives the 0/1 adjacency matrix of g (symmetric for " "undirected graphs)."); + symtab_add_builtin("WeightedAdjacencyMatrix", builtin_weighted_adjacency_matrix); + symtab_get_def("WeightedAdjacencyMatrix")->attributes |= ATTR_PROTECTED; + symtab_set_docstring("WeightedAdjacencyMatrix", + "WeightedAdjacencyMatrix[g] gives the adjacency matrix of g with each " + "entry the corresponding edge's weight (0 where there is no edge). " + "Equal to AdjacencyMatrix[g] when g has no EdgeWeight."); + symtab_add_builtin("IncidenceMatrix", builtin_incidence_matrix); symtab_get_def("IncidenceMatrix")->attributes |= ATTR_PROTECTED; symtab_set_docstring("IncidenceMatrix", @@ -111,6 +124,12 @@ void graph_init(void) { symtab_set_docstring("CycleGraph", "CycleGraph[n] gives the cycle graph on n vertices."); + symtab_add_builtin("StarGraph", builtin_star_graph); + symtab_get_def("StarGraph")->attributes |= ATTR_PROTECTED; + symtab_set_docstring("StarGraph", + "StarGraph[n] gives the star on n vertices: the hub 1 joined to each of " + "the n-1 leaves 2..n."); + symtab_add_builtin("PathGraph", builtin_path_graph); symtab_get_def("PathGraph")->attributes |= ATTR_PROTECTED; symtab_set_docstring("PathGraph", diff --git a/src/graph/graph.h b/src/graph/graph.h index 29ca7fcd1..312ccb646 100644 --- a/src/graph/graph.h +++ b/src/graph/graph.h @@ -8,10 +8,14 @@ * Graphs are represented as ordinary Expr trees -- no new EXPR_* tag: * * Graph[ List[v1, v2, ...], List[edge1, edge2, ...] ] + * Graph[ List[v1, v2, ...], List[edge1, edge2, ...], EdgeWeight -> List[w1, ...] ] * * where each edge is DirectedEdge[u, v] or UndirectedEdge[u, v]. Rule/-> * and TwoWayRule/<-> are accepted as parse-time sugar and normalized on - * construction. Vertices are arbitrary expressions. + * construction. Vertices are arbitrary expressions. The optional 3rd argument + * attaches a weight to each edge, matched by position (weights[i] belongs to + * edges[i]); a graph without it is unweighted, and every accessor treats an + * unweighted edge's weight as 1 (see graph_resolve_edge_weights). * * This mirrors the src/linalg/ layout: one builtin per translation unit, * with the builtin_* prototypes declared here and registered in graph.c. @@ -90,16 +94,40 @@ Expr* builtin_vertex_degree(Expr* res); /* VertexDegree[g] / [g,v] * Expr* builtin_vertex_in_degree(Expr* res); /* VertexInDegree[g] / [g,v] */ Expr* builtin_vertex_out_degree(Expr* res);/* VertexOutDegree[g] / [g,v] */ Expr* builtin_directed_graph_q(Expr* res); /* DirectedGraphQ[g] */ +Expr* builtin_edge_weight(Expr* res); /* EdgeWeight[g] */ /* ---- Phase 3: matrix views (linalg interop) ------------------------------- */ Expr* builtin_adjacency_matrix(Expr* res); /* AdjacencyMatrix[g] */ Expr* builtin_incidence_matrix(Expr* res); /* IncidenceMatrix[g] */ Expr* builtin_adjacency_graph(Expr* res); /* AdjacencyGraph[m] */ +Expr* builtin_weighted_adjacency_matrix(Expr* res); /* WeightedAdjacencyMatrix[g] */ + +/* Resolves g's per-edge weights in EdgeList order: a copy of the EdgeWeight + * list when g carries one (3-arg canonical form), else List[1, 1, ..., 1] + * (one per edge). NULL if g is not a valid graph. Shared by builtin_edge_weight + * and builtin_weighted_adjacency_matrix so the two builtins can never disagree + * on what "unweighted" defaults to. */ +Expr* graph_resolve_edge_weights(const Expr* g); + +/* True iff g is a valid, weighted (3-arg EdgeWeight) graph and every one of + * its weights is a non-negative, non-Complex number (per expr_is_numeric_like: + * Integer, BigInt, Real, MPFR, or Rational) -- i.e. usable by a weighted + * shortest-path algorithm. False for an unweighted graph, a symbolic weight, + * a negative weight, or a Complex weight: shortestpath.c falls back to + * unweighted BFS in every one of those cases rather than erroring. */ +int graph_weights_usable(const Expr* g); + +/* Approximate double value of a numeric weight Expr (Integer/BigInt/Real/MPFR/ + * Rational), for ranking/comparison use only -- NAN if w isn't one of those + * shapes. Callers needing an exact returned value (e.g. GraphDistance) must + * reconstruct it separately via real Expr arithmetic, not from this. */ +double graph_weight_to_double(const Expr* w); /* ---- Phase 4: graph generators -------------------------------------------- */ Expr* builtin_complete_graph(Expr* res); /* CompleteGraph[n] */ Expr* builtin_cycle_graph(Expr* res); /* CycleGraph[n] */ Expr* builtin_path_graph(Expr* res); /* PathGraph[n] / PathGraph[{...}] */ +Expr* builtin_star_graph(Expr* res); /* StarGraph[n] */ Expr* builtin_random_graph(Expr* res); /* RandomGraph[{n, m}] */ /* ---- Phase 5: shared adjacency scaffolding (graph_util.c) ------------------ diff --git a/src/graph/graph_util.c b/src/graph/graph_util.c index d1d1efe1b..bba0cad61 100644 --- a/src/graph/graph_util.c +++ b/src/graph/graph_util.c @@ -20,6 +20,7 @@ #include "sym_names.h" #include #include +#include /* True iff e is a function node whose head is the interned symbol `sym`. */ static int head_is_sym(const Expr* e, const char* sym) { @@ -189,6 +190,41 @@ int graph_vertex_index(const Expr* verts, const Expr* v) { return -1; } +/* True iff `opt` is a well-formed EdgeWeight -> List[n] rule, where n equals + * `edge_count`. Shape only -- does not inspect the individual weight values, + * which may be any expression (numeric weights are the expected case, but + * nothing here requires it, matching how vertices are already arbitrary + * expressions). */ +static int graph_edge_weight_rule_ok(const Expr* opt, size_t edge_count) { + if (!head_is_sym(opt, SYM_Rule) || opt->data.function.arg_count != 2) return 0; + const Expr* key = opt->data.function.args[0]; + const Expr* val = opt->data.function.args[1]; + if (!key || key->type != EXPR_SYMBOL || key->data.symbol.name != SYM_EdgeWeight) + return 0; + if (!graph_is_list(val)) return 0; + return val->data.function.arg_count == edge_count; +} + +/* True iff g's shape is Graph[verts, edges] (unweighted) or + * Graph[verts, edges, EdgeWeight -> List[n]] with n == |edges| (weighted). + * Both `graph_is_valid` and `graph_build_adj` route through this instead of + * duplicating an `arg_count != 2` literal -- they are two independent choke + * points (a plan-reviewer-caught defect: widening only one left the other's + * 8 downstream builtins rejecting every weighted graph even though GraphQ + * reported it valid), so the arity/shape check itself must be shared, not + * just widened identically by hand in both places. Structural shape only -- + * self-loops, parallel edges, etc. are still each caller's own job. */ +static int graph_shape_ok(const Expr* g) { + if (!head_is_sym(g, SYM_Graph)) return 0; + size_t argc = g->data.function.arg_count; + if (argc == 2) return 1; + if (argc != 3) return 0; + const Expr* edges = g->data.function.args[1]; + if (!graph_is_list(edges)) return 0; + return graph_edge_weight_rule_ok(g->data.function.args[2], + edges->data.function.arg_count); +} + /* ---- Phase 5: adjacency scaffolding --------------------------------------- */ /* Validation over an already-built vertex index; defined with graph_is_valid @@ -206,7 +242,7 @@ void graph_adj_free(GraphAdj* a) { GraphAdj* graph_build_adj(const Expr* g) { /* Validate and index in one pass: graph_is_valid would build and throw away * the same vertex index, and the two fill passes below need it anyway. */ - if (!head_is_sym(g, SYM_Graph) || g->data.function.arg_count != 2) return NULL; + if (!graph_shape_ok(g)) return NULL; const Expr* verts = g->data.function.args[0]; const Expr* edges = g->data.function.args[1]; if (!graph_is_list(verts) || !graph_is_list(edges)) return NULL; @@ -325,7 +361,7 @@ static int graph_check(const Expr* g, const GraphVIdx* ix) { } int graph_is_valid(const Expr* g) { - if (!head_is_sym(g, SYM_Graph) || g->data.function.arg_count != 2) + if (!graph_shape_ok(g)) return 0; const Expr* verts = g->data.function.args[0]; @@ -338,3 +374,75 @@ int graph_is_valid(const Expr* g) { graph_vidx_free(ix); return ok; } + +Expr* graph_resolve_edge_weights(const Expr* g) { + if (!graph_is_valid(g)) return NULL; + size_t ne = g->data.function.args[1]->data.function.arg_count; + + if (g->data.function.arg_count == 3) { + const Expr* wlist = g->data.function.args[2]->data.function.args[1]; + Expr** ws = (ne > 0) ? calloc(ne, sizeof(Expr*)) : NULL; + if (ne > 0 && !ws) return NULL; + for (size_t i = 0; i < ne; i++) ws[i] = expr_copy(wlist->data.function.args[i]); + Expr* out = expr_new_function(expr_new_symbol(SYM_List), ws, ne); + free(ws); + return out; + } + + /* Unweighted: default every edge's weight to 1, matching Wolfram Language. */ + Expr** ws = (ne > 0) ? calloc(ne, sizeof(Expr*)) : NULL; + if (ne > 0 && !ws) return NULL; + for (size_t i = 0; i < ne; i++) ws[i] = expr_new_integer(1); + Expr* out = expr_new_function(expr_new_symbol(SYM_List), ws, ne); + free(ws); + return out; +} + +/* Approximate double value of a numeric weight, for Dijkstra's internal + * vertex-selection comparisons ONLY -- never for a returned value (see + * graph_weight_to_double's caller: shortestpath.c reconstructs the exact + * GraphDistance answer separately, via Plus[] over the real Expr weights). + * A plain (not rounding-to-nearest) conversion is fine here: it only needs to + * preserve enough precision to compare relative distances correctly, not to + * reproduce N[expr]'s exact rounding. Returns NAN for anything not numeric -- + * callers must gate with graph_weights_usable first. */ +double graph_weight_to_double(const Expr* w) { + if (!w) return NAN; + switch (w->type) { + case EXPR_INTEGER: return (double)w->data.integer; + case EXPR_REAL: return w->data.real; + case EXPR_BIGINT: return mpz_get_d(w->data.bigint); +#ifdef USE_MPFR + case EXPR_MPFR: return mpfr_get_d(w->data.mpfr, MPFR_RNDN); +#endif + case EXPR_FUNCTION: + if (head_is_sym(w, SYM_Rational) && w->data.function.arg_count == 2) { + double p = graph_weight_to_double(w->data.function.args[0]); + double q = graph_weight_to_double(w->data.function.args[1]); + if (!isnan(p) && !isnan(q) && q != 0.0) return p / q; + } + return NAN; + default: + return NAN; + } +} + +int graph_weights_usable(const Expr* g) { + if (!graph_is_valid(g) || g->data.function.arg_count != 3) return 0; + Expr* weights = graph_resolve_edge_weights(g); + if (!weights) return 0; + + int ok = 1; + size_t n = weights->data.function.arg_count; + for (size_t i = 0; i < n && ok; i++) { + const Expr* w = weights->data.function.args[i]; + /* expr_is_numeric_like also accepts Complex (numeric-component + * Complex[re,im]); Dijkstra needs an orderable real, so reject that + * shape explicitly rather than reusing the check unfiltered. */ + if (!expr_is_numeric_like(w) || head_is_sym(w, SYM_Complex)) { ok = 0; break; } + double d = graph_weight_to_double(w); + if (isnan(d) || d < 0.0) ok = 0; + } + expr_free(weights); + return ok; +} diff --git a/src/graph/shortestpath.c b/src/graph/shortestpath.c index f6df30deb..aaf274177 100644 --- a/src/graph/shortestpath.c +++ b/src/graph/shortestpath.c @@ -1,9 +1,21 @@ /* shortestpath.c - FindShortestPath[g,s,t] and GraphDistance[g,s,t]. * - * Unweighted breadth-first search over the successor adjacency (out[]): for a - * directed graph this follows edge direction; for an undirected graph out[] is - * symmetric, so it is an ordinary shortest path. Wolfram's naming split is - * kept: FindShortestPath returns the vertex path, GraphDistance the length. + * Two algorithms, dispatched on graph_weights_usable(g): + * - Unweighted (default): breadth-first search over the successor adjacency + * (GraphAdj.out[]): for a directed graph this follows edge direction; for + * an undirected graph out[] is symmetric, so it is an ordinary shortest + * path. + * - Weighted (g carries a non-negative-numeric EdgeWeight): Dijkstra over a + * local, call-scoped weighted adjacency (WAdj below) -- NOT over + * GraphAdj, which has no weight storage and is shared by 5 other + * builtins (ConnectedComponents, WeaklyConnectedComponents, + * FindSpanningTree, ConnectedGraphQ, VertexConnectivity); widening it + * would risk the exact class of shared-choke-point defect a + * plan-reviewer pass caught during the EdgeWeight ticket. Falls back to + * BFS for a symbolic or negative weight rather than erroring. + * + * Wolfram's naming split is kept: FindShortestPath returns the vertex path, + * GraphDistance the length/total weight. * * Unreachable target: FindShortestPath -> {} (empty list), GraphDistance -> * Infinity. @@ -13,8 +25,10 @@ #include "graph.h" #include "expr.h" +#include "eval.h" #include "sym_names.h" #include +#include /* BFS from src over out[]; fills parent[] (-1 = root/unvisited) and dist[] * (-1 = unreached). Caller allocates parent/dist of length a->n. */ @@ -33,6 +47,142 @@ static void bfs(const GraphAdj* a, int src, int* parent, int* dist) { free(q); } +/* ---- Weighted adjacency, local to this file (see header comment) --------- */ + +typedef struct { + int n; + const Expr* verts; /* borrowed from g */ + int* outdeg; + int** out; /* out[v][k] = neighbor vertex index */ + Expr*** outw; /* outw[v][k] = borrowed weight Expr* for that edge */ + Expr* weights; /* owns every Expr* referenced by outw; freed by wadj_free */ +} WAdj; + +static void wadj_free(WAdj* w) { + if (!w) return; + for (int i = 0; i < w->n; i++) { free(w->out[i]); free(w->outw[i]); } + free(w->out); free(w->outw); free(w->outdeg); + if (w->weights) expr_free(w->weights); + free(w); +} + +/* Builds a weighted successor adjacency directly from g's canonical form + * (verts/edges/EdgeWeight), independent of GraphAdj. NULL if g is not a + * valid, weights-usable graph. */ +static WAdj* build_wadj(const Expr* g) { + if (!graph_weights_usable(g)) return NULL; + const Expr* verts = g->data.function.args[0]; + const Expr* edges = g->data.function.args[1]; + int n = (int)verts->data.function.arg_count; + size_t ne = edges->data.function.arg_count; + + Expr* weights = graph_resolve_edge_weights(g); + if (!weights) return NULL; + + WAdj* w = calloc(1, sizeof(WAdj)); + if (!w) { expr_free(weights); return NULL; } + w->n = n; w->verts = verts; w->weights = weights; + w->outdeg = calloc((size_t)(n > 0 ? n : 1), sizeof(int)); + w->out = calloc((size_t)(n > 0 ? n : 1), sizeof(int*)); + w->outw = calloc((size_t)(n > 0 ? n : 1), sizeof(Expr**)); + + GraphVIdx* ix = graph_vidx_new((size_t)n); + for (int i = 0; i < n; i++) + graph_vidx_put(ix, verts->data.function.args[i], i); + + /* Pass 1: count out-degrees (each undirected edge contributes to both endpoints). */ + for (size_t k = 0; k < ne; k++) { + const Expr* e = edges->data.function.args[k]; + const char* kind = graph_edge_kind(e); + int ia = graph_vidx_get(ix, e->data.function.args[0]); + int ib = graph_vidx_get(ix, e->data.function.args[1]); + if (ia < 0 || ib < 0) continue; + w->outdeg[ia]++; + if (kind == SYM_UndirectedEdge) w->outdeg[ib]++; + } + for (int i = 0; i < n; i++) { + w->out[i] = w->outdeg[i] ? calloc((size_t)w->outdeg[i], sizeof(int)) : NULL; + w->outw[i] = w->outdeg[i] ? calloc((size_t)w->outdeg[i], sizeof(Expr*)) : NULL; + } + int* fill = calloc((size_t)(n > 0 ? n : 1), sizeof(int)); + + /* Pass 2: fill. */ + for (size_t k = 0; k < ne; k++) { + const Expr* e = edges->data.function.args[k]; + const char* kind = graph_edge_kind(e); + int ia = graph_vidx_get(ix, e->data.function.args[0]); + int ib = graph_vidx_get(ix, e->data.function.args[1]); + if (ia < 0 || ib < 0) continue; + Expr* wt = weights->data.function.args[k]; + w->out[ia][fill[ia]] = ib; + w->outw[ia][fill[ia]] = wt; + fill[ia]++; + if (kind == SYM_UndirectedEdge) { + w->out[ib][fill[ib]] = ia; + w->outw[ib][fill[ib]] = wt; + fill[ib]++; + } + } + free(fill); + graph_vidx_free(ix); + return w; +} + +/* Dijkstra from src over w; fills parent[] (-1 = root/unvisited) and dist[] + * (DBL_MAX = unreached). O(V^2) array scan, no heap -- consistent with this + * subsystem's existing small-graph exact-algorithm precedent + * (VertexConnectivity's own brute-force). dist[] is for vertex-selection + * comparisons ONLY; the exact GraphDistance value is reconstructed separately + * by the caller via Plus[] over the real weight Exprs along the found path, + * never printed from this array directly. */ +static void dijkstra(const WAdj* w, int src, int* parent, double* dist) { + char* done = calloc((size_t)(w->n > 0 ? w->n : 1), sizeof(char)); + for (int i = 0; i < w->n; i++) { parent[i] = -1; dist[i] = DBL_MAX; } + dist[src] = 0.0; + + for (int iter = 0; iter < w->n; iter++) { + int u = -1; + double best = DBL_MAX; + for (int i = 0; i < w->n; i++) + if (!done[i] && dist[i] < best) { best = dist[i]; u = i; } + if (u < 0) break; /* remaining vertices are unreachable */ + done[u] = 1; + for (int j = 0; j < w->outdeg[u]; j++) { + int v = w->out[u][j]; + double d = dist[u] + graph_weight_to_double(w->outw[u][j]); + if (d < dist[v]) { dist[v] = d; parent[v] = u; } + } + } + free(done); +} + +/* Exact total weight along the parent-chain path from src to t (inclusive), + * built by summing the real Expr* weights (via evaluate(Plus[...])), so an + * all-integer/-rational input keeps an exact integer/rational answer instead + * of the double accumulator dijkstra() used only to pick the path. */ +static Expr* exact_path_weight(const WAdj* w, const int* parent, int src, int t) { + Expr** terms = NULL; + size_t nterms = 0, cap = 0; + int v = t; + while (v != src) { + int u = parent[v]; + Expr* wt = NULL; + for (int j = 0; j < w->outdeg[u]; j++) + if (w->out[u][j] == v) { wt = w->outw[u][j]; break; } + if (!wt) { free(terms); return NULL; } /* should not happen */ + if (nterms == cap) { + cap = cap ? cap * 2 : 4; + terms = realloc(terms, cap * sizeof(Expr*)); + } + terms[nterms++] = expr_copy(wt); + v = u; + } + if (nterms == 0) return expr_new_integer(0); /* src == t */ + Expr* sum = expr_new_function(expr_new_symbol(SYM_Plus), terms, nterms); + free(terms); + return evaluate(sum); +} + /* Resolve g, s, t to a GraphAdj and endpoint indices. Returns adj (caller frees) * or NULL; on success sets *is,*it. */ static GraphAdj* resolve(Expr* res, int* is, int* it) { @@ -50,23 +200,44 @@ Expr* builtin_find_shortest_path(Expr* res) { int is, it; GraphAdj* a = resolve(res, &is, &it); if (!a) return NULL; - - int* parent = calloc((size_t)a->n, sizeof(int)); - int* dist = calloc((size_t)a->n, sizeof(int)); - bfs(a, is, parent, dist); + const Expr* g = res->data.function.args[0]; Expr* out; - if (dist[it] < 0) { - out = expr_new_function(expr_new_symbol(SYM_List), NULL, 0); /* {} */ + if (graph_weights_usable(g)) { + WAdj* w = build_wadj(g); + int* parent = calloc((size_t)w->n, sizeof(int)); + double* dist = calloc((size_t)w->n, sizeof(double)); + dijkstra(w, is, parent, dist); + if (dist[it] == DBL_MAX) { + out = expr_new_function(expr_new_symbol(SYM_List), NULL, 0); + } else { + /* Reconstruct the vertex path from parent[] (same shape as BFS's). */ + int len = 1, v = it; + while (v != is) { len++; v = parent[v]; } + Expr** path = calloc((size_t)len, sizeof(Expr*)); + v = it; + for (int k = len - 1; k >= 0; k--) { path[k] = expr_copy(w->verts->data.function.args[v]); v = parent[v]; } + out = expr_new_function(expr_new_symbol(SYM_List), path, (size_t)len); + free(path); + } + free(parent); free(dist); wadj_free(w); } else { - int len = dist[it] + 1; - Expr** path = calloc((size_t)len, sizeof(Expr*)); - int v = it; - for (int k = len - 1; k >= 0; k--) { path[k] = expr_copy(a->verts->data.function.args[v]); v = parent[v]; } - out = expr_new_function(expr_new_symbol(SYM_List), path, (size_t)len); - free(path); + int* parent = calloc((size_t)a->n, sizeof(int)); + int* dist = calloc((size_t)a->n, sizeof(int)); + bfs(a, is, parent, dist); + if (dist[it] < 0) { + out = expr_new_function(expr_new_symbol(SYM_List), NULL, 0); /* {} */ + } else { + int len = dist[it] + 1; + Expr** path = calloc((size_t)len, sizeof(Expr*)); + int v = it; + for (int k = len - 1; k >= 0; k--) { path[k] = expr_copy(a->verts->data.function.args[v]); v = parent[v]; } + out = expr_new_function(expr_new_symbol(SYM_List), path, (size_t)len); + free(path); + } + free(parent); free(dist); } - free(parent); free(dist); graph_adj_free(a); + graph_adj_free(a); return out; } @@ -74,13 +245,25 @@ Expr* builtin_graph_distance(Expr* res) { int is, it; GraphAdj* a = resolve(res, &is, &it); if (!a) return NULL; + const Expr* g = res->data.function.args[0]; - int* parent = calloc((size_t)a->n, sizeof(int)); - int* dist = calloc((size_t)a->n, sizeof(int)); - bfs(a, is, parent, dist); - - Expr* out = (dist[it] < 0) ? expr_new_symbol(SYM_Infinity) - : expr_new_integer(dist[it]); - free(parent); free(dist); graph_adj_free(a); + Expr* out; + if (graph_weights_usable(g)) { + WAdj* w = build_wadj(g); + int* parent = calloc((size_t)w->n, sizeof(int)); + double* dist = calloc((size_t)w->n, sizeof(double)); + dijkstra(w, is, parent, dist); + out = (dist[it] == DBL_MAX) ? expr_new_symbol(SYM_Infinity) + : exact_path_weight(w, parent, is, it); + free(parent); free(dist); wadj_free(w); + } else { + int* parent = calloc((size_t)a->n, sizeof(int)); + int* dist = calloc((size_t)a->n, sizeof(int)); + bfs(a, is, parent, dist); + out = (dist[it] < 0) ? expr_new_symbol(SYM_Infinity) + : expr_new_integer(dist[it]); + free(parent); free(dist); + } + graph_adj_free(a); return out; } diff --git a/src/graph/wtadjmat.c b/src/graph/wtadjmat.c new file mode 100644 index 000000000..25e26e74f --- /dev/null +++ b/src/graph/wtadjmat.c @@ -0,0 +1,63 @@ +/* wtadjmat.c - WeightedAdjacencyMatrix[g]: dense adjacency matrix filled with + * per-edge weights instead of a literal 1. + * + * Same algorithm as AdjacencyMatrix (adjmat.c): a DirectedEdge[a,b] sets + * M[a][b] = weight(a,b); an UndirectedEdge sets both M[a][b] and M[b][a]. Any + * entry with no edge is 0. For a graph with no EdgeWeight, every weight + * defaults to 1 (graph_resolve_edge_weights), so + * WeightedAdjacencyMatrix[g] == AdjacencyMatrix[g] exactly for an unweighted g. + * + * Memory (SPEC section 4): returns a freshly-allocated matrix; frees res. + */ + +#include "graph.h" +#include "expr.h" +#include "sym_names.h" +#include + +Expr* builtin_weighted_adjacency_matrix(Expr* res) { + if (res->data.function.arg_count != 1) return NULL; + const Expr* g = res->data.function.args[0]; + if (!graph_is_valid(g)) return NULL; + + const Expr* verts = g->data.function.args[0]; + const Expr* edges = g->data.function.args[1]; + size_t n = verts->data.function.arg_count; + + Expr* weights = graph_resolve_edge_weights(g); + if (!weights) return NULL; + + Expr** grid = (n > 0) ? calloc(n * n, sizeof(Expr*)) : NULL; + if (n > 0 && !grid) { expr_free(weights); return NULL; } + + GraphVIdx* ix = graph_vidx_new(n); + if (!ix) { free(grid); expr_free(weights); return NULL; } + for (size_t i = 0; i < n; i++) + graph_vidx_put(ix, verts->data.function.args[i], (int)i); + + for (size_t k = 0; k < edges->data.function.arg_count; k++) { + const Expr* e = edges->data.function.args[k]; + const char* kind = graph_edge_kind(e); + int ia = graph_vidx_get(ix, e->data.function.args[0]); + int ib = graph_vidx_get(ix, e->data.function.args[1]); + if (ia < 0 || ib < 0) continue; /* validated: cannot happen */ + Expr* w = weights->data.function.args[k]; + grid[(size_t)ia * n + (size_t)ib] = expr_copy(w); + if (kind == SYM_UndirectedEdge) grid[(size_t)ib * n + (size_t)ia] = expr_copy(w); + } + graph_vidx_free(ix); + expr_free(weights); + + Expr** rows = (n > 0) ? calloc(n, sizeof(Expr*)) : NULL; + for (size_t i = 0; i < n; i++) { + Expr** row = calloc(n, sizeof(Expr*)); + for (size_t j = 0; j < n; j++) + row[j] = grid[i * n + j] ? grid[i * n + j] : expr_new_integer(0); + rows[i] = expr_new_function(expr_new_symbol(SYM_List), row, n); + free(row); + } + Expr* mat = expr_new_function(expr_new_symbol(SYM_List), rows, n); + free(rows); + free(grid); + return mat; +} diff --git a/src/print.c b/src/print.c index 1af9e91c4..1102045a4 100644 --- a/src/print.c +++ b/src/print.c @@ -374,12 +374,12 @@ static void print_standard(Expr* e, int parent_prec) { printf("%s", head == SYM_DirectedEdge ? " -> " : " <-> "); print_standard(e->data.function.args[1], my_prec); } - else if (head == SYM_Graph && e->data.function.arg_count == 2 - && g_inputform_depth == 0 - && graph_is_list(e->data.function.args[0]) - && graph_is_list(e->data.function.args[1])) { - /* Terse summary in standard output; InputForm/FullForm fall through - * to the literal Graph[{...}, {...}] constructor (round-trippable). */ + else if (head == SYM_Graph && g_inputform_depth == 0 && graph_is_valid(e)) { + /* Terse summary in standard output (unweighted 2-arg form, or + * weighted 3-arg Graph[verts, edges, EdgeWeight -> {...}]); the + * weight list is not shown here. InputForm/FullForm fall through + * to the literal Graph[{...}, {...}(, EdgeWeight -> {...})] + * constructor (round-trippable). */ unsigned long nv = (unsigned long)e->data.function.args[0]->data.function.arg_count; unsigned long ne = (unsigned long)e->data.function.args[1]->data.function.arg_count; printf("Graph[<%lu %s, %lu %s>]", diff --git a/src/sym_names.c b/src/sym_names.c index 8cb8e053c..307bc05dc 100644 --- a/src/sym_names.c +++ b/src/sym_names.c @@ -858,6 +858,7 @@ const char* SYM_AdjacencyGraph = NULL; const char* SYM_CompleteGraph = NULL; const char* SYM_CycleGraph = NULL; const char* SYM_PathGraph = NULL; +const char* SYM_StarGraph = NULL; const char* SYM_RandomGraph = NULL; const char* SYM_FindShortestPath = NULL; const char* SYM_GraphDistance = NULL; @@ -867,6 +868,8 @@ const char* SYM_StronglyConnectedComponents = NULL; const char* SYM_FindSpanningTree = NULL; const char* SYM_VertexConnectivity = NULL; const char* SYM_GraphPlot = NULL; +const char* SYM_EdgeWeight = NULL; +const char* SYM_WeightedAdjacencyMatrix = NULL; /* NumberForm + Row (numeric-display formatting) and NumberForm's options. */ const char* SYM_NumberForm = NULL; @@ -1722,6 +1725,7 @@ void sym_names_init(void) { SYM_CompleteGraph = intern_symbol("CompleteGraph"); SYM_CycleGraph = intern_symbol("CycleGraph"); SYM_PathGraph = intern_symbol("PathGraph"); + SYM_StarGraph = intern_symbol("StarGraph"); SYM_RandomGraph = intern_symbol("RandomGraph"); SYM_FindShortestPath = intern_symbol("FindShortestPath"); SYM_GraphDistance = intern_symbol("GraphDistance"); @@ -1731,6 +1735,8 @@ void sym_names_init(void) { SYM_FindSpanningTree = intern_symbol("FindSpanningTree"); SYM_VertexConnectivity = intern_symbol("VertexConnectivity"); SYM_GraphPlot = intern_symbol("GraphPlot"); + SYM_EdgeWeight = intern_symbol("EdgeWeight"); + SYM_WeightedAdjacencyMatrix = intern_symbol("WeightedAdjacencyMatrix"); /* System symbols that have no kernel implementation and no cached SYM_* * pointer, but must still be recognized as System` (not qualified into a diff --git a/src/sym_names.h b/src/sym_names.h index bc4d695f5..b83fdfba5 100644 --- a/src/sym_names.h +++ b/src/sym_names.h @@ -915,6 +915,7 @@ extern const char* SYM_AdjacencyGraph; extern const char* SYM_CompleteGraph; extern const char* SYM_CycleGraph; extern const char* SYM_PathGraph; +extern const char* SYM_StarGraph; extern const char* SYM_RandomGraph; extern const char* SYM_FindShortestPath; extern const char* SYM_GraphDistance; @@ -924,6 +925,8 @@ extern const char* SYM_StronglyConnectedComponents; extern const char* SYM_FindSpanningTree; extern const char* SYM_VertexConnectivity; extern const char* SYM_GraphPlot; +extern const char* SYM_EdgeWeight; +extern const char* SYM_WeightedAdjacencyMatrix; /* NumberForm + Row (numeric-display formatting) and NumberForm's option * names. NumberForm is a print wrapper handled in print.c; the option-name diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2f76013af..86cc47e03 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -845,6 +845,8 @@ set(COMMON_SRC ../src/graph/spanningtree.c ../src/graph/connectivity.c ../src/graph/graphplot.c + ../src/graph/edgeweight.c + ../src/graph/wtadjmat.c ) # render.c / render3d.c / label_font.c need a live Raylib + display; only diff --git a/tests/test_graph.c b/tests/test_graph.c index 88f3d6435..f108a9351 100644 --- a/tests/test_graph.c +++ b/tests/test_graph.c @@ -208,6 +208,23 @@ static void test_generators(void) { assert_eval_eq("VertexDegree[PathGraph[5]]", "{1, 2, 2, 2, 1}", 0); /* Explicit-vertex path. */ assert_eval_eq("EdgeList[PathGraph[{a,b,c}]]", "{a <-> b, b <-> c}", 0); + + /* StarGraph[n]: n vertices, n-1 edges; hub 1 has degree n-1, leaves 1. */ + assert_eval_eq("VertexCount[StarGraph[5]]", "5", 0); + assert_eval_eq("EdgeCount[StarGraph[5]]", "4", 0); + assert_eval_eq("DirectedGraphQ[StarGraph[5]]", "False", 0); + assert_eval_eq("VertexDegree[StarGraph[5]]", "{4, 1, 1, 1, 1}", 0); + assert_eval_eq("EdgeList[StarGraph[4]]", "{1 <-> 2, 1 <-> 3, 1 <-> 4}", 0); + /* Degenerate sizes: a lone vertex, and the empty graph. */ + assert_eval_eq("{VertexCount[StarGraph[1]], EdgeCount[StarGraph[1]]}", + "{1, 0}", 0); + assert_eval_eq("{VertexCount[StarGraph[0]], EdgeCount[StarGraph[0]]}", + "{0, 0}", 0); + /* A star is connected and is its own spanning tree. */ + assert_eval_eq("ConnectedGraphQ[StarGraph[6]]", "True", 0); + assert_eval_eq("EdgeCount[FindSpanningTree[StarGraph[6]]]", "5", 0); + /* Symbolic argument stays unevaluated. */ + assert_eval_eq("Head[StarGraph[x]]", "StarGraph", 0); } static void test_random_graph(void) { @@ -294,6 +311,140 @@ static void test_graphplot(void) { assert_eval_eq("Head[GraphPlot[5]]", "GraphPlot", 0); } +/* ---- Edge weights (Graph[v,e,EdgeWeight->w], EdgeWeight, WeightedAdjacencyMatrix) ---- */ +static void test_edge_weights(void) { + /* AC-1: constructs and validates. */ + assert_eval_eq("GraphQ[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]]", "True", 0); + + /* AC-2: EdgeWeight[g] returns weights in EdgeList order. */ + assert_eval_eq("EdgeWeight[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]]", + "{5, 7}", 0); + + /* AC-3: unweighted graphs default to all 1s. */ + assert_eval_eq("EdgeWeight[Graph[{1,2,3},{1->2,2->3}]]", "{1, 1}", 0); + assert_eval_eq("EdgeWeight[CompleteGraph[3]]", "{1, 1, 1}", 0); + + /* AC-4: WeightedAdjacencyMatrix on a directed weighted graph. */ + assert_eval_eq( + "WeightedAdjacencyMatrix[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]]", + "{{0, 5, 0}, {0, 0, 7}, {0, 0, 0}}", 0); + + /* AC-5: undirected weighted graph -> symmetric matrix. */ + assert_eval_eq("WeightedAdjacencyMatrix[Graph[{1,2},{1<->2},EdgeWeight->{9}]]", + "{{0, 9}, {9, 0}}", 0); + + /* AC-6: unweighted WeightedAdjacencyMatrix equals AdjacencyMatrix exactly. */ + assert_eval_eq( + "WeightedAdjacencyMatrix[CycleGraph[4]] == AdjacencyMatrix[CycleGraph[4]]", + "True", 0); + + /* AC-7: weight/edge count mismatch is malformed, left unevaluated. */ + assert_eval_eq("GraphQ[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5}]]", "False", 0); + assert_eval_eq("Head[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5}]]", "Graph", 0); + + /* AC-8: terse summary unchanged for a weighted graph. */ + assert_eval_eq("Graph[{1,2},{1<->2},EdgeWeight->{3}]", + "Graph[<2 vertices, 1 edge>]", 0); + + /* AC-9: InputForm round-trips through the parser. */ + { + Expr* g = evaluate(parse_expression( + "Graph[{1,2},{1<->2},EdgeWeight->{3}]")); + ASSERT(g != NULL); + Expr* wrap_args[1] = { expr_copy(g) }; + Expr* wrap = expr_new_function(expr_new_symbol("InputForm"), wrap_args, 1); + char* s = expr_to_string(wrap); + Expr* g2 = evaluate(parse_expression(s)); + ASSERT(expr_eq(g, g2)); + free(s); + expr_free(wrap); + expr_free(g); + expr_free(g2); + } + assert_eval_eq("InputForm[Graph[{1,2},{1<->2},EdgeWeight->{3}]]", + "Graph[{1, 2}, {1 <-> 2}, EdgeWeight -> {3}]", 0); + + /* AC-11: the graph_build_adj-routed builtins evaluate normally against a + * weighted graph, not left unevaluated -- the plan-reviewer-caught defect + * (graph_build_adj is a second, independent choke point from + * graph_is_valid) regression-tested directly. FindShortestPath's + * assertion stays {1, 2, 3} even after the weighted-shortest-path ticket: + * this specific graph has only one path from 1 to 3, so BFS and Dijkstra + * agree on it -- only GraphDistance's value changed (hop count 2 -> + * weighted total 5+7=12), tested here as the dedicated regression case. */ + const char* wg = "Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]"; + char buf[256]; + snprintf(buf, sizeof(buf), "FindShortestPath[%s,1,3]", wg); + assert_eval_eq(buf, "{1, 2, 3}", 0); + snprintf(buf, sizeof(buf), "GraphDistance[%s,1,3]", wg); + assert_eval_eq(buf, "12", 0); + snprintf(buf, sizeof(buf), "ConnectedComponents[%s]", wg); + assert_eval_eq(buf, "{{1, 2, 3}}", 0); + snprintf(buf, sizeof(buf), "WeaklyConnectedComponents[%s]", wg); + assert_eval_eq(buf, "{{1, 2, 3}}", 0); + snprintf(buf, sizeof(buf), "Head[FindSpanningTree[%s]]", wg); + assert_eval_eq(buf, "Graph", 0); + const char* wug = "Graph[{1,2,3},{1<->2,2<->3},EdgeWeight->{5,7}]"; + snprintf(buf, sizeof(buf), "ConnectedGraphQ[%s]", wug); + assert_eval_eq(buf, "True", 0); + snprintf(buf, sizeof(buf), "VertexConnectivity[%s]", wug); + assert_eval_eq(buf, "1", 0); + + /* Non-goal, regression-tested: derived-vertex weighted construction + * (Graph[e, EdgeWeight->w], no explicit vertex list) is not accepted -- + * fails safe (unevaluated), not silently. */ + assert_eval_eq("Head[Graph[{1->2,2->3},EdgeWeight->{1,1}]]", "Graph", 0); + + /* Regression: unweighted graphs and existing builtins are unaffected. */ + assert_eval_eq("EdgeCount[CompleteGraph[5]]", "10", 0); +} + +/* ---- Weighted shortest path (Dijkstra dispatch in FindShortestPath/GraphDistance) ---- */ +static void test_weighted_shortest_path(void) { + /* AC-1/AC-2: min-weight path differs from min-hop path; both agree it's {1,2,3,4} + * (weight 3) not the direct {1,4} edge (weight 10). */ + const char* g1 = "Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}]"; + char buf[256]; + snprintf(buf, sizeof(buf), "FindShortestPath[%s,1,4]", g1); + assert_eval_eq(buf, "{1, 2, 3, 4}", 0); + snprintf(buf, sizeof(buf), "GraphDistance[%s,1,4]", g1); + assert_eval_eq(buf, "3", 0); + /* Exact integer, not a real -- the plan-reviewer-caught defect (a raw double + * accumulator would print "3."). */ + snprintf(buf, sizeof(buf), "Head[GraphDistance[%s,1,4]]", g1); + assert_eval_eq(buf, "Integer", 0); + + /* AC-3: unweighted graphs are unaffected (still plain BFS). */ + assert_eval_eq("FindShortestPath[CycleGraph[6],1,4]", "{1, 2, 3, 4}", 0); + assert_eval_eq("GraphDistance[CycleGraph[6],1,4]", "3", 0); + + /* AC-4: a symbolic weight falls back to BFS rather than erroring. */ + assert_eval_eq( + "FindShortestPath[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{a,7}],1,3]", + "{1, 2, 3}", 0); + + /* AC-5: a negative weight falls back to BFS (hop count, not a Dijkstra artifact). */ + assert_eval_eq( + "GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{-1,7}],1,3]", + "2", 0); + + /* AC-6: undirected weighted graph -- weights apply symmetrically. */ + assert_eval_eq( + "FindShortestPath[Graph[{1,2,3},{1<->2,2<->3},EdgeWeight->{1,1}],1,3]", + "{1, 2, 3}", 0); + + /* AC-7: unreachable target keeps existing semantics. */ + assert_eval_eq("FindShortestPath[Graph[{1,2,3},{1->2},EdgeWeight->{5}],1,3]", + "{}", 0); + assert_eval_eq("GraphDistance[Graph[{1,2,3},{1->2},EdgeWeight->{5}],1,3]", + "Infinity", 0); + + /* Rational weights stay exact. */ + assert_eval_eq( + "GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{1/2,1/3}],1,3]", + "5/6", 0); +} + int main(void) { symtab_init(); core_init(); @@ -313,6 +464,8 @@ int main(void) { TEST(test_components); TEST(test_spanning_and_connectivity); TEST(test_graphplot); + TEST(test_edge_weights); + TEST(test_weighted_shortest_path); printf("All graph tests passed!\n"); return 0; diff --git a/thoughts/shared/plans/2026-08-22-graph-edge-weights-summary.md b/thoughts/shared/plans/2026-08-22-graph-edge-weights-summary.md new file mode 100644 index 000000000..a710392db --- /dev/null +++ b/thoughts/shared/plans/2026-08-22-graph-edge-weights-summary.md @@ -0,0 +1,71 @@ +--- +created: 2026-08-22 +type: plan +lifecycle: active +status: approved +full_plan: thoughts/shared/plans/2026-08-22-graph-edge-weights.md +--- + +# Plan Summary: Graph Edge Weights + WeightedAdjacencyMatrix + +**Full plan (appendix)**: `thoughts/shared/plans/2026-08-22-graph-edge-weights.md` + +## Recommendation +Add `Graph[v, e, EdgeWeight -> {...}]`, `EdgeWeight[g]`, and `WeightedAdjacencyMatrix[g]` — +the one extension `src/graph/adjmat.c:9-10` already flags as pre-approved future work. +Additive only; existing 2-arg `Graph[v,e]` behavior is unchanged. + +## Options Considered +1. **3rd constructor arg, `EdgeWeight -> List[...]`** (chosen) — additive, doesn't reuse the + already-rejected 3-argument-edge shape, matches the codebase's existing trailing-`Rule` + option idiom. +2. **3-argument edge head** (`DirectedEdge[u,v,w]`) — rejected: already documented and + enforced as malformed input. +3. **Global `Options`/`OptionValue` registration** — rejected: designed for symbols with + independently queryable/settable defaults, not a one-shot constructor argument. + +## Decisions +- Weights match edges by position in the given edge list. +- `EdgeWeight[g]` defaults to all-`1`s when unweighted. +- No packed/NDArray/`Compile[]` support — documented exemption, consistent with existing + `AdjacencyMatrix`/`IncidenceMatrix` precedent. +- No weighted `FindShortestPath`/`GraphDistance` in this pass (deferred). +- No derived-vertex weighted construction (`Graph[e, EdgeWeight->w]`) — explicit-vertex form + only (added after plan-reviewer flagged the silent scope gap). + +## Non-goals +Vertex weights, multigraphs, hypergraphs, edge tags beyond `EdgeWeight`, `GraphPlot` +rendering changes, packed/NDArray/`Compile[]` support, weighted shortest-path, derived-vertex +weighted construction. + +## Open Questions + +### Unresolved +_None._ + +### Resolved +See full plan — all resolved during research and the plan-reviewer pass. + +## Requires Approval +_None._ — scope confirmed directly with the maintainer during research. + +## Architecture Impact +- New services introduced: none +- APIs changed: none +- Data crossing a service boundary: none +- New external dependency: none +- Deployment topology change: none + +## Subsystems & Dependencies +- Subsystems touched: graph (invocation: inline) +- Interdependencies surfaced: none + +## Plan Review outcome +One BLOCKING finding from the adversarial `plan-reviewer` pass — the plan originally claimed +`graph_is_valid` was the sole validation choke point, but `graph_build_adj` +(`src/graph/graph_util.c:206-209`) is a second, independent one that 8 of the 27 builtins +route through, bypassing `graph_is_valid` entirely. Verified directly against source, then +fixed: both choke points now widen via a shared helper, with a new AC (AC-11) and manual +verification step covering all 8 affected builtins. Two WORTH FLAGGING findings (a wrong +line citation, an unstated scope boundary on derived-vertex weighted construction) also +fixed. See the full plan's `## Plan Review` section for the complete transcript. diff --git a/thoughts/shared/plans/2026-08-22-graph-edge-weights.md b/thoughts/shared/plans/2026-08-22-graph-edge-weights.md new file mode 100644 index 000000000..8ec8188c4 --- /dev/null +++ b/thoughts/shared/plans/2026-08-22-graph-edge-weights.md @@ -0,0 +1,491 @@ +--- +created: 2026-08-22 +source_sha: d6139c46 +subsystems: [graph] +type: plan +lifecycle: active +status: implemented +--- + +# Graph Edge Weights + WeightedAdjacencyMatrix Implementation Plan + +## TL;DR +Adds an optional `EdgeWeight -> {...}` constructor argument to `Graph[...]`, a new +`EdgeWeight[g]` query builtin, and a new `WeightedAdjacencyMatrix[g]` builtin — the one +extension `src/graph/adjmat.c:9-10` already names as pre-approved future work. Existing +unweighted graphs are unaffected (canonical 2-arg form unchanged); a malformed weight list +(wrong length) leaves `Graph[...]` unevaluated, same as existing rejections. Verified by +extending `tests/test_graph.c` and running the full existing suite plus `make check-c99`. + +## Overview +`src/graph/` implements simple, unweighted graphs. Its own in-code comment +(`adjmat.c:9-10`) and the spec doc (`docs/spec/builtins/graphs.md:19-21`) both flag edge +weights and `WeightedAdjacencyMatrix` as the one deliberately-deferred piece of the MVP — +everything else locked out of scope (hypergraphs, multigraphs, parallel edges, edge tags) +stays locked. This plan implements exactly that carve-out: a graph can now optionally carry +per-edge weights via `Graph[v, e, EdgeWeight -> {w1, ..., wm}]`, weights are readable via +`EdgeWeight[g]`, and `WeightedAdjacencyMatrix[g]` returns the weight-filled matrix analog of +the existing `AdjacencyMatrix[g]`. No other builtin's behavior changes; `FindShortestPath`/ +`GraphDistance` stay BFS/unweighted (explicitly deferred, see research doc). + +## Decisions +- **Weights live in a 3rd constructor argument, `EdgeWeight -> List[...]`, not a new edge + head**, because the constructor already rejects 3-argument edges as malformed + (`docs/spec/builtins/graphs.md:34-35`) — reusing that shape would contradict an existing + decision. A trailing option `Rule` is this codebase's established idiom for optional + arguments (`src/numerical_calculus/nderiv.c:531-566`) and leaves the unweighted 2-arg form + untouched. +- **Weights match edges by position in the given edge list**, mirroring real Wolfram + Language `EdgeWeight` semantics. +- **`EdgeWeight[g]` defaults to all-`1`s when unweighted**, matching Wolfram Language, so + `WeightedAdjacencyMatrix[g]` is well-defined for every existing graph. +- **No packed/NDArray/`Compile[]` support for the two new builtins.** None of the 27 existing + graph builtins — including numeric-matrix-returning `AdjacencyMatrix` — are on `pack.c`'s + `AWARE` list or in `COMPILE_MISSING.md`; they're structural over a `Graph` tree, not + elementwise over a numeric buffer, so CLAUDE.md's structural-head exemption applies, + consistent with precedent. Documented here, not silent; `make check-packed-aware` must + stay green. +- **Weighted `FindShortestPath`/`GraphDistance` are out of scope** — see Non-goals. + +## Non-goals +- No weighted-shortest-path / Dijkstra mode for `FindShortestPath` or `GraphDistance` + (research doc's resolved Open Question — deferred to a follow-up; real algorithmic scope + growth, not a few hours). +- No vertex weights, no multigraphs, no hypergraphs, no edge tags beyond `EdgeWeight` — the + rest of the MVP-locked scope in `docs/spec/builtins/graphs.md:19-21` stays locked. +- No change to `GraphPlot`'s rendering (edge weights are not drawn/labeled). +- No packed/NDArray/`Compile[]` support for the new builtins (see Decisions above). +- No derived-vertex weighted construction (`Graph[e, EdgeWeight -> w]`, 2-arg edges-only form + plus a weight rule). Weighted graphs must use the explicit-vertex 3-arg form, + `Graph[v, e, EdgeWeight -> w]`. This fails safe (unevaluated, not wrong output) rather than + silently accepted — named explicitly here per plan-reviewer finding (below). + +## Acceptance Criteria + +| ID | Given | When | Then | Input | Expected | +|---|---|---|---|---|---| +| AC-1 | A graph built with an `EdgeWeight` option | `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]` is evaluated | it canonicalizes and is `GraphQ`-valid | `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]` | `GraphQ[...] -> True`, `InputForm` round-trips | +| AC-2 | A weighted graph | `EdgeWeight[g]` is called | it returns the weights in `EdgeList` order | `EdgeWeight[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]]` | `{5, 7}` | +| AC-3 | An unweighted graph | `EdgeWeight[g]` is called | it defaults to all `1`s | `EdgeWeight[Graph[{1,2,3},{1->2,2->3}]]` | `{1, 1}` | +| AC-4 | A weighted directed graph | `WeightedAdjacencyMatrix[g]` is called | entries hold the edge weight, `0` elsewhere | `WeightedAdjacencyMatrix[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]]` | `{{0,5,0},{0,0,7},{0,0,0}}` | +| AC-5 | A weighted undirected graph | `WeightedAdjacencyMatrix[g]` is called | the matrix is symmetric | `WeightedAdjacencyMatrix[Graph[{1,2},{1<->2},EdgeWeight->{9}]]` | `{{0,9},{9,0}}` | +| AC-6 | An unweighted graph | `WeightedAdjacencyMatrix[g]` is called | it equals `AdjacencyMatrix[g]` | `WeightedAdjacencyMatrix[CycleGraph[4]]` | `AdjacencyMatrix[CycleGraph[4]]` | +| AC-7 | A weight-list length mismatch | `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5}]` is evaluated | it is malformed, left unevaluated | `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5}]` | `GraphQ[...] -> False`; expression unevaluated | +| AC-8 | A weighted graph | printed in standard form | terse summary unchanged | `Graph[{1,2},{1<->2},EdgeWeight->{3}]` | `Graph[<2 vertices, 1 edge>]` | +| AC-9 | A weighted graph | printed in `InputForm` | round-trips through the parser | `InputForm[Graph[{1,2},{1<->2},EdgeWeight->{3}]]` | `Graph[{1, 2}, {1 <-> 2}, EdgeWeight -> {3}]`, re-parses to an `expr_eq`-equal tree | +| AC-10 | This change | `make check-packed-aware` runs | no new failures vs. baseline | `make check-packed-aware` | exit 0, unchanged from pre-change baseline | +| AC-11 | A weighted graph | one of the 8 `graph_build_adj`-routed builtins is called (not just `GraphQ`) | it evaluates normally (weights ignored, BFS/unweighted semantics per Non-goals), not left unevaluated | `FindShortestPath[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}],1,3]` | `{1, 2, 3}` (unweighted BFS path, same as if built without `EdgeWeight`) | + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Weight-storage shape — `EdgeWeight -> List[...]` 3rd constructor arg (see Decisions). +- [x] Weighted shortest-path scope — deferred (see Non-goals; resolved in research doc). +- [x] Packed/NDArray/Compile[] applicability — exempt, documented (see Decisions). + +## Plan Review + +### Blocking +_None._ + +### Worth Flagging +_None._ + +### Resolved +**[BLOCKING] "graph_is_valid is the single choke point" is false — a second, independent +arity gate (`graph_build_adj`) bypasses it entirely** +- Where: was in "Key Discoveries"/"Decisions"; actual code at `graph_util.c:206-209` + (`graph_build_adj`) vs. `graph_util.c:327-339` (`graph_is_valid`) +- Verified directly against source (grep confirmed 8 builtins — `ConnectedComponents`, + `WeaklyConnectedComponents`, `ConnectedGraphQ`, `VertexConnectivity`, `FindSpanningTree`, + `FindShortestPath`, `GraphDistance` — call `graph_build_adj` directly, never + `graph_is_valid`) before accepting the finding. +- How addressed: plan now widens **both** choke points via a shared `graph_edge_weight_ok` + helper (Phase 1 §2), added `graph_build_adj` to Components & Files Affected, and added + AC-11 plus a manual-verification line covering all 8 affected builtins. + +**[WORTH FLAGGING] Citation `docs/spec/builtins/graphs.md:32` pointed at the wrong line** +- How addressed: corrected to `:34-35` (the actual "3-argument edges... malformed" sentence) + everywhere it was cited. + +**[WORTH FLAGGING] Weight support silently scoped to the 3-arg (explicit-vertex) constructor +form only, with no stated decision about the 1-arg derived-vertex form** +- How addressed: added an explicit Non-goal — `Graph[e, EdgeWeight -> w]` (derived-vertex + weighted construction) is out of scope; weighted graphs require the explicit-vertex form. + +## Requires Approval +_None._ — scope was confirmed directly with the maintainer during research +(`AskUserQuestion`, 2026-08-23): weights + `WeightedAdjacencyMatrix` + `EdgeWeight` only, no +weighted shortest-path. + +## Architecture Impact +- New services introduced: none +- APIs changed: none +- Data crossing a service boundary: none +- New external dependency: none +- Deployment topology change: none + +(Additive-only: `Graph[v,e]`'s existing 2-arg canonical form and all 27 existing builtins +keep identical behavior; `Graph[v,e,EdgeWeight->w]` is a new, backward-compatible 3-arg +form, not a change to an existing API's contract.) + +## Subsystems & Dependencies +- Subsystems touched: graph (invocation: inline) +- Interdependencies surfaced: none + +## Risks and Rollback +_None — standard tier, no architectural impact._ + +--- + +## Current State Analysis +- `src/graph/graph.h:1-22` — canonical form is `Graph[List[verts], List[edges]]`, 2-arg + only; no weight concept anywhere in the type. +- `src/graph/construct.c:56-68` (`try_build_canonical`) accepts `argc == 1` (edges only) or + `argc == 2` (verts + edges); anything else returns `NULL` (malformed/unevaluated). +- `src/graph/graph_util.c:327-339` (`graph_is_valid`) hardcodes + `g->data.function.arg_count != 2` as an immediate rejection — the single choke point that + must widen to accept the new 3-arg canonical shape. +- `src/print.c:377-386` — the terse `Graph[]` summary path is gated on + `e->data.function.arg_count == 2`; a 3-arg weighted graph would silently fall through to + full literal `Graph[{...}, {...}, EdgeWeight -> {...}]` printing in Standard form unless + this is widened too. +- `src/graph/adjmat.c` (`builtin_adjacency_matrix`) is the direct template for + `WeightedAdjacencyMatrix`: builds a `GraphVIdx`, walks `edges`, fills an `n×n` `int*` grid, + converts to nested `List`s. The weighted version fills the grid with the corresponding + weight `Expr*` instead of a literal `1`. +- `src/graph/edgelist.c` (`builtin_edge_list`) is the direct template for `EdgeWeight[g]`'s + read path: validate, then return a copy of the relevant canonical part in `EdgeList` order. +- No downstream code anywhere in `src/` or `src/internal/*.m` reads `AdjacencyMatrix`/ + `IncidenceMatrix` output expecting literal `0`/`1` values (confirmed by grep across the + whole tree during research) — nothing outside `src/graph/` needs to change. +- `makefile:338` wildcards `$(SRC_DIR)/graph/*.c` — a new file needs no build-system edit. +- `tests/test_graph.c` (319 lines, 15 test functions) is CMake-built via `tests/CMakeLists.txt` + (already wired for `graph_tests`) — new test functions go in the same file. + +## Desired End State +`Graph[v, e, EdgeWeight -> {...}]` constructs and validates a weighted graph; `EdgeWeight[g]` +and `WeightedAdjacencyMatrix[g]` are registered, documented, and attributed builtins; +`docs/spec/builtins/graphs.md`'s locked-scope paragraph reflects that edge weights are now +implemented; a changelog entry is added; `make check-c99`, the full `graph_tests` binary +(existing 15 + new weighted-edge tests), and `make check-packed-aware` all pass with no +regressions. Verify via `./tests/build/graph_tests` and a manual REPL session exercising +every Acceptance Criteria row above. + +### Key Discoveries: +- The 3-argument-edge rejection already documented in `docs/spec/builtins/graphs.md:34-35` is + about edges (`DirectedEdge[u,v,w]`), not the constructor's own arity — confirming the 3rd + *constructor* argument (`EdgeWeight -> ...`) is a clean, non-conflicting extension point. +- **There are TWO independent validation choke points, not one** (caught by the + `plan-reviewer` pass against an earlier draft of this plan, which claimed only one; verified + directly against source before accepting the finding). `graph_is_valid` + (`graph_util.c:327-339`) is one; `graph_build_adj` (`graph_util.c:206-209`) is a **second, + independent** entry point with its own hardcoded `arg_count != 2` rejection — it does not + call `graph_is_valid` at all (its own comment explains why: "`graph_is_valid` would build + and throw away the same vertex index"). Confirmed by grep: `ConnectedComponents`/ + `WeaklyConnectedComponents` (`components.c:53,123`), `ConnectedGraphQ`/ + `VertexConnectivity` (`connectivity.c:22,56`), `FindSpanningTree` (`spanningtree.c:33`), and + `FindShortestPath`/`GraphDistance` (`shortestpath.c:41`) — 8 of the 27 builtins — all call + `graph_build_adj` directly. Both choke points must widen identically, or these 8 builtins + would return unevaluated on any weighted graph while `GraphQ` reports it valid — a + contradiction with this plan's own Overview claim that "no other builtin's behavior + changes." See Phase 1 §2 and AC-11 below. + +## Components & Files Affected + +| File | Change | +|---|---| +| `src/graph/construct.c:56-68,134-142` | `try_build_canonical` accepts `argc == 3` when the 3rd arg is `Rule[EdgeWeight, List[...]]` of the same length as the edge list; canonicalizes weight expressions via `expr_copy`, same as edges/vertices | +| `src/graph/graph_util.c:327-339` (`graph_is_valid`) | Accept `arg_count == 2` (unweighted) or `arg_count == 3` with a well-formed `EdgeWeight -> List[n]` third argument (`n` == edge count), via a new shared `graph_edge_weight_ok(g)` helper | +| `src/graph/graph_util.c:206-209` (`graph_build_adj`) | **Independent second choke point** (plan-reviewer finding, verified) — widen its own `arg_count != 2` guard identically, using the same shared `graph_edge_weight_ok(g)` helper, so all 8 builtins routed through it (`ConnectedComponents`, `WeaklyConnectedComponents`, `ConnectedGraphQ`, `VertexConnectivity`, `FindSpanningTree`, `FindShortestPath`, `GraphDistance`) keep working (weights ignored) on a weighted graph instead of returning unevaluated | +| `src/graph/graph.h` | Declare `builtin_edge_weight`, `builtin_weighted_adjacency_matrix`; update header comment's canonical-form note to mention the optional 3rd argument | +| `src/graph/edgeweight.c` (new) | `EdgeWeight[g]`: validate, return the weight list in `EdgeList` order, defaulting to all-`1`s when `g` carries no `EdgeWeight` | +| `src/graph/wtadjmat.c` (new) | `WeightedAdjacencyMatrix[g]`: `adjmat.c`'s algorithm, filling with the resolved per-edge weight (via the same default-to-`1` resolution as `EdgeWeight[g]`) instead of a literal `1` | +| `src/graph/graph.c` | Register `EdgeWeight`, `WeightedAdjacencyMatrix` (attributes, docstrings), alongside the existing "Phase 3: matrix views" block | +| `src/sym_names.h`, `src/sym_names.c` | Add `SYM_EdgeWeight` interned-name pointer/definition (`SYM_WeightedAdjacencyMatrix`/`SYM_EdgeWeight` needed since `EdgeWeight` is now a real head compared against in C, not just a builtin name string) | +| `src/print.c:377-386` | Widen the terse-summary condition to also match the 3-arg weighted canonical form (still print `Graph[]`, ignoring the weight arg for the summary) | +| `tests/test_graph.c` | New test functions covering AC-1 through AC-9 | +| `docs/spec/builtins/graphs.md` | Update the "MVP scope (locked)" paragraph (weights now supported; hypergraphs/multigraphs/edge-tags stay locked) and add an `EdgeWeight`/`WeightedAdjacencyMatrix` subsection with examples | +| `docs/spec/changelog/2026-08-17.md` | New entry describing the change | +| `KIT-FEEDBACK-GRAPH.md` | Journal entries for `/create-plan`, `/implement-plan`, `/verify-implementation` phases (dogfood deliverable, not part of the feature itself) | + +## Core Flow Diagram + +```mermaid +flowchart TD + A["Graph[v, e, EdgeWeight -> w]"] --> B{argc?} + B -->|1 or 2, unweighted| C[existing try_build_canonical path] + B -->|3, EdgeWeight rule| D[normalize edges + verts as today] + D --> E{len(w) == len(e)?} + E -->|no| F[return NULL: malformed, unevaluated] + E -->|yes| G["assemble Graph[List v, List e, EdgeWeight -> List w]"] + C --> H[graph_is_valid] + G --> H + H -->|3-arg form| I[validate EdgeWeight shape too] + H -->|2-arg form| J[existing validation, unchanged] + I --> K[canonical weighted graph] + J --> L[canonical unweighted graph] + K --> M["EdgeWeight[g] / WeightedAdjacencyMatrix[g]"] + L -->|no EdgeWeight found -> default weight 1 per edge| M +``` + +## Alternatives Considered + +### A 3-argument edge head, e.g. `DirectedEdge[u, v, w]` +Rejected because the constructor already documents and enforces "3-argument edges" as +malformed input (`docs/spec/builtins/graphs.md:34-35`, `construct.c`'s `normalize_edge` +requiring `arg_count == 2`). Overloading that arity for a different purpose (a weight +instead of a rejection reason) would be a confusing, backward-incompatible reinterpretation +of currently-defined "malformed" behavior, whereas a 3rd constructor-level option argument +is purely additive. + +### A generic `Options`/`SetOptions`/`OptionValue` registration for `Graph` +Rejected because that system (`src/options_builtin.c`) is designed for symbols whose default +options are queried/set independently of a single call (e.g. `Options[Plot]`, +`SetOptions[Plot, ...]`), not for parsing a one-shot trailing rule inside a single +constructor invocation. The lighter local `is_option`/`apply_option` idiom already used in +`src/numerical_calculus/nderiv.c` and `ndsolve.c` is the established, simpler match for this +shape and avoids registering `Graph` into a global options table it doesn't otherwise need. + +## Implementation Approach +Widen the single validation choke point (`graph_is_valid`) and the single construction path +(`try_build_canonical`) to recognize an optional 3rd `EdgeWeight -> List[...]` argument, +then add two new query/matrix builtins that read it (defaulting to weight `1` when absent). +Every other existing builtin is untouched — they read only `args[0]`/`args[1]` and stay +correct once the choke points accept the wider canonical shape. Build order: (1) constructor ++ validation + printing, tested via `GraphQ`/`InputForm` round-trip; (2) `EdgeWeight[g]`; +(3) `WeightedAdjacencyMatrix[g]`; (4) docs + changelog. + +## Phase 1: Constructor, validation, and printing accept `EdgeWeight` + +### Overview +Extend `Graph[...]`'s construction and validation to accept and canonicalize an optional +`EdgeWeight -> {...}` 3rd argument, and keep the terse standard-form summary working for +weighted graphs. + +### Changes Required: + +#### 1. Construction +**File**: `src/graph/construct.c` +**Changes**: `try_build_canonical` gains an `argc == 3` branch: the 3rd argument must be +`Rule[Symbol["EdgeWeight"], List[w1,...,wm]]` (via `SYM_EdgeWeight`) with `m` equal to the +normalized edge count; copy each `wi` with `expr_copy` into a new weights array, and append +`Rule[EdgeWeight, List[weights]]` as the graph's 3rd argument before the existing +`graph_is_valid` call. + +```c +/* argc == 3: EdgeWeight -> {...} */ +} else if (argc == 3) { + verts_in = res->data.function.args[0]; + edges_in = res->data.function.args[1]; + weight_opt = res->data.function.args[2]; + if (!graph_is_edge_weight_rule(weight_opt)) return NULL; +} +``` + +#### 2. Validation — BOTH choke points +**File**: `src/graph/graph_util.c` +**Changes**: add a shared static helper (e.g. `graph_edge_weight_ok(const Expr* g)`) that +checks: either `arg_count == 2` (existing, always OK), or `arg_count == 3` with the 3rd +argument `Rule[EdgeWeight, List[n]]` where `n` equals the edge count. Call it from **both** +`graph_is_valid` (`:327-339`) *and* `graph_build_adj` (`:206-209`) in place of their current +identical-but-independent `arg_count != 2` literal checks. This is the fix for the +plan-reviewer's BLOCKING finding: these are two separate functions with duplicated, +independent arity gates, not one shared choke point — both must widen, or 8 of the 27 +builtins (everything routed through `graph_build_adj`) silently keep rejecting any weighted +graph even though `GraphQ` reports it valid. + +#### 3. Printing +**File**: `src/print.c` +**Changes**: widen the terse-summary `else if` condition (line ~377) from +`arg_count == 2` to also accept `arg_count == 3` with a well-formed `EdgeWeight` 3rd +argument, still deriving `nv`/`ne` from `args[0]`/`args[1]` only. + +#### 4. Symbol interning +**File**: `src/sym_names.h`, `src/sym_names.c` +**Changes**: add `SYM_EdgeWeight` (declared/defined the same way as `SYM_Rule`, +`SYM_TwoWayRule` nearby) since C code needs to compare against it directly (not just +register it as a builtin name string). + +### Success Criteria: + +#### Automated Verification: +- [x] Build succeeds: `make -j$(nproc)` +- [x] Portability gate passes: `make check-c99` +- [x] Packed-array audit unaffected: `make check-packed-aware` (no new findings) +- [x] Existing graph test suite still passes unmodified: + `cd tests/build && make -j$(nproc) graph_tests && ./graph_tests` +- [x] New construction/validation/printing tests pass (AC-1, AC-7, AC-8, AC-9) — added to + `tests/test_graph.c` in this phase +- [x] AC-11 passes: at least one `graph_build_adj`-routed builtin (`FindShortestPath`) + evaluates normally against a weighted graph, not unevaluated + +#### Manual Verification: +- [x] `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]` evaluates to a canonical weighted graph + in a REPL session and `GraphQ[...]` reports `True` +- [x] `Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5}]` (mismatched length) is left unevaluated +- [x] Existing unweighted graphs (`CompleteGraph[5]`, etc.) print and behave identically to + before this change +- [x] Each of the 8 `graph_build_adj`-routed builtins (`ConnectedComponents`, + `WeaklyConnectedComponents`, `ConnectedGraphQ`, `VertexConnectivity`, + `FindSpanningTree`, `FindShortestPath`, `GraphDistance`) evaluates normally (not + unevaluated) against a weighted graph + +**Implementation Note**: After completing this phase and all automated verification passes, +pause here for manual confirmation from the human that the manual testing was successful +before proceeding to the next phase. + +--- + +## Phase 2: `EdgeWeight[g]` and `WeightedAdjacencyMatrix[g]` + +### Overview +Add the two new query/matrix builtins, registered with attributes and docstrings, following +`edgelist.c`/`adjmat.c` as direct templates. + +### Changes Required: + +#### 1. EdgeWeight[g] +**File**: `src/graph/edgeweight.c` (new) +**Changes**: validate `g`; if a 3rd `EdgeWeight` argument is present, return a copy of its +weight list; otherwise return `List[1, 1, ..., 1]` (one `1` per edge). + +```c +Expr* builtin_edge_weight(Expr* res) { + if (res->data.function.arg_count != 1) return NULL; + const Expr* g = res->data.function.args[0]; + if (!graph_is_valid(g)) return NULL; + /* returns the 3rd-arg weight list if present, else n ones */ + return graph_resolve_edge_weights(g); /* new shared helper, graph_util.c */ +} +``` + +#### 2. WeightedAdjacencyMatrix[g] +**File**: `src/graph/wtadjmat.c` (new) +**Changes**: `adjmat.c`'s algorithm, but the grid holds `Expr*` (weight or `NULL` for "no +edge") instead of `int`; fill from the resolved weight list (shared helper from item 1) +matched positionally against `EdgeList[g]`; missing entries become integer `0`. + +#### 3. Registration +**File**: `src/graph/graph.c` +**Changes**: register both builtins with `ATTR_PROTECTED` and docstrings, adjacent to the +existing "Phase 3: matrix views" block (`AdjacencyMatrix`/`IncidenceMatrix`/`AdjacencyGraph`). + +### Success Criteria: + +#### Automated Verification: +- [x] Build succeeds: `make -j$(nproc)` +- [x] Portability gate passes: `make check-c99` +- [x] Full graph test suite passes, including new AC-2 through AC-6 tests: + `cd tests/build && make -j$(nproc) graph_tests && ./graph_tests` +- [x] `make check-packed-aware` still exits 0 with no new findings + +#### Manual Verification: +- [x] `EdgeWeight[CycleGraph[4]]` returns `{1, 1, 1, 1}` in a REPL session +- [x] `WeightedAdjacencyMatrix[CycleGraph[4]] == AdjacencyMatrix[CycleGraph[4]]` (unweighted + fallback matches exactly) +- [x] A hand-built weighted graph's `WeightedAdjacencyMatrix` matches expected values by hand + +**Implementation Note**: After completing this phase and all automated verification passes, +pause here for manual confirmation from the human that the manual testing was successful +before proceeding to the next phase. + +--- + +## Phase 3: Docs and changelog + +### Overview +Bring `docs/spec/builtins/graphs.md` and this week's changelog current with the new +functionality, per this repo's own CLAUDE.md/SPEC.md documentation mandate. + +### Changes Required: + +#### 1. Spec doc +**File**: `docs/spec/builtins/graphs.md` +**Changes**: amend the "MVP scope (locked)" paragraph (weights/`WeightedAdjacencyMatrix` are +now implemented; hypergraphs/multigraphs/edge-tags/parallel-edges/self-loops remain locked), +add an `EdgeWeight` / `WeightedAdjacencyMatrix` subsection under "Matrix views" with the +AC-1..AC-6 examples. + +#### 2. Changelog +**File**: `docs/spec/changelog/2026-08-17.md` +**Changes**: append a dated entry summarizing the feature, referencing the plan. + +### Success Criteria: + +#### Automated Verification: +- [x] `make check-c99` still passes (no code change in this phase, but re-verify after doc + edits touch nothing code-related) +- [x] `grep -c "WeightedAdjacencyMatrix" docs/spec/builtins/graphs.md` returns nonzero + +#### Manual Verification: +- [x] Docs read correctly and match actual REPL behavior for every example given + +**Implementation Note**: After completing this phase and all automated verification passes, +pause here for manual confirmation from the human that the manual testing was successful +before proceeding to `/verify-implementation`. + +--- + +## Testing Strategy +Extend the existing `tests/test_graph.c` (CMake-built, `ctest`-style assertion suite) rather +than adding a new test file — weighted graphs are a variant of the same construction/ +validation/query path every existing test already exercises, and the file's existing +`test_matrix_views` function is the natural home for `WeightedAdjacencyMatrix` cases. + +### Edge Cases & Integration Scenarios: +- Weight list length mismatch (AC-7) — must be rejected the same way self-loops/parallel + edges already are (left unevaluated, not a crash or a silently-truncated weight list) +- Unweighted graph through `EdgeWeight[g]`/`WeightedAdjacencyMatrix[g]` (AC-3, AC-6) — the + default-to-1 path must be exercised for both the derived-vertex (`Graph[e]`) and + explicit-vertex (`Graph[v,e]`) constructor forms +- Directed vs. undirected weighted graphs (AC-4 vs AC-5) — symmetry only for undirected +- `InputForm` round-trip (AC-9) through the actual parser, not just constructed by hand + +### Manual Testing Steps: +1. Build (`make -j$(nproc)`) and start `./Mathilda` +2. Run every expression in the Acceptance Criteria table's "Input" column and compare + against "Expected" +3. Confirm `CompleteGraph[5]`, `CycleGraph[8]`, etc. (pre-existing, unweighted) still behave + identically to before the change + +## Performance Considerations +None expected: the new builtins reuse the existing `GraphVIdx` O(1) lookup helper +(`graph_util.c`) rather than reintroducing the linear scans fixed in commit `8d71d845`; the +weight-resolution helper is `O(V + E)` per call, matching `AdjacencyMatrix`'s existing cost. + +## Migration Notes +None — purely additive; no existing data/graphs need migration. + +## References +- Research: `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md` +- Research summary: `thoughts/shared/research/2026-08-22-graph-edge-weights-extension-summary.md` +- Similar implementation (direct templates): `src/graph/adjmat.c`, `src/graph/edgelist.c` +- Locked-scope source: `docs/spec/builtins/graphs.md:19-21`, `src/graph/adjmat.c:9-10` + +## Implementation Notes (post-hoc, added during /implement-plan) + +**Deviation from plan**: the plan's "makefile auto-discovers `src/*.c`" claim +(`Current State Analysis`) is true only of the top-level `makefile`. `tests/CMakeLists.txt` +lists `src/graph/*.c` files **explicitly**, not via glob — the two new files +(`edgeweight.c`, `wtadjmat.c`) had to be added there too, or the test binary fails to link +(`Undefined symbols: _builtin_edge_weight, _builtin_weighted_adjacency_matrix`), discovered +only when building `graph_tests`. Fixed: both files added to `tests/CMakeLists.txt`'s graph +source list, adjacent to the other `src/graph/*.c` entries. + +**Verification was run directly** (not deferred to a separate human pass): every +Acceptance Criteria row (AC-1 through AC-11) was executed against the built `./Mathilda` +binary via `-file` scripts and its output compared verbatim against the plan's Expected +column — all matched exactly, including the AC-11 regression check across all 8 +`graph_build_adj`-routed builtins. `make check-c99` and `make check-packed-aware` both exit +0 with no new findings. The 16-test `graph_tests` suite (15 pre-existing + 1 new +`test_edge_weights` covering every AC row) passes. Checkboxes above are marked complete on +that basis. + +**Toolchain note (environment, not code)**: this machine's build required `export +SDKROOT=$(xcrun --show-sdk-path)` for `gcc-16` to find system headers (`stdio.h` et al.) — +pre-existing local environment gap, unrelated to this change, hit while running the +`typecheck` phase of the verification ladder configured earlier in this session. diff --git a/thoughts/shared/plans/2026-08-23-weighted-shortest-path.md b/thoughts/shared/plans/2026-08-23-weighted-shortest-path.md new file mode 100644 index 000000000..fa6ebfd71 --- /dev/null +++ b/thoughts/shared/plans/2026-08-23-weighted-shortest-path.md @@ -0,0 +1,310 @@ +--- +created: 2026-08-23 +source_sha: 3d872247 +subsystems: [graph] +type: plan +lifecycle: active +status: implemented +--- + +# Weighted Shortest Path Implementation Plan + +## TL;DR +Makes `FindShortestPath[g,s,t]` and `GraphDistance[g,s,t]` weight-aware: when `g` carries a +non-negative-numeric `EdgeWeight`, both dispatch to Dijkstra instead of BFS, matching real +Wolfram Language semantics and completing the follow-up ticket 1's own Non-goals named. +Falls back to the existing unweighted BFS whenever weights are absent, symbolic, or contain +a negative value — never regresses a previously-working call. Verified by extending +`tests/test_graph.c` and updating the one existing regression assertion (from ticket 1) that +this ticket intentionally supersedes. + +## Overview +Ticket 1 added `EdgeWeight`/`WeightedAdjacencyMatrix` but explicitly deferred making +`FindShortestPath`/`GraphDistance` weight-aware (its own Non-goals: "real algorithmic scope +growth, not a few hours"). This ticket delivers that follow-up. `GraphAdj` +(`graph_util.c`), the structure both builtins currently use, has no weight storage and is +shared by 5 other builtins (ConnectedComponents, WeaklyConnectedComponents, FindSpanningTree, ConnectedGraphQ, VertexConnectivity) — so this plan builds a separate, call-scoped weighted adjacency +inside `shortestpath.c` rather than widening `GraphAdj` itself, directly applying the lesson +from ticket 1's `plan-reviewer`-caught defect (a shared choke point is a bigger blast radius +than it looks). + +## Decisions +- **Dispatch is automatic, not a new builtin name** — real Wolfram Language uses edge weights + automatically when present; matching that beats inventing `WeightedFindShortestPath`. +- **Dijkstra fires only for non-negative numeric weights; anything else falls back to BFS** + unchanged — Dijkstra is incorrect on negative weights, and a previously-working call must + not regress to unevaluated just because a weight is symbolic or negative. +- **No change to `GraphAdj`/`graph_build_adj`** — a local, per-call weighted structure lives + only in the two changed builtins, reusing ticket 1's `graph_resolve_edge_weights`, keeping + blast radius off the 5 other builtins sharing that structure. +- **Plain O(V²) Dijkstra, no priority queue** — matches this codebase's existing complexity + tolerance for small-graph exact algorithms (`VertexConnectivity`'s own "brute-force ... + intended for small graphs"). +- **Ticket 1's `test_edge_weights` AC-11 lines for these two builtins must change** — they + assert "weights ignored," the exact behavior this ticket removes. The other 5 builtins in + that test are unaffected. + +## Non-goals +- No Bellman-Ford or any negative-weight support — falls back to BFS instead. +- No priority-queue-based Dijkstra (O((V+E) log V)) — O(V²) is sufficient at this codebase's + stated scale tolerance. +- No change to any other `graph_build_adj`-routed builtin (`ConnectedComponents`, + `VertexConnectivity`, etc.) — weights are not meaningful for those algorithms. +- No A*, bidirectional search, or all-pairs shortest path. + +## Acceptance Criteria + +| ID | Given | When | Then | Input | Expected | +|---|---|---|---|---|---| +| AC-1 | A weighted graph where the shortest-hop-count path isn't the min-weight path | `FindShortestPath[g,s,t]` is called | it returns the min-weight path, not the min-hop path | `FindShortestPath[Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}],1,4]` | `{1, 2, 3, 4}` (weight 3) not `{1, 4}` (weight 10) | +| AC-2 | Same weighted graph | `GraphDistance[g,s,t]` is called | it returns the min total weight, not hop count | `GraphDistance[Graph[{1,2,3,4},{1->2,2->3,3->4,1->4},EdgeWeight->{1,1,1,10}],1,4]` | `3` | +| AC-3 | An unweighted graph | `FindShortestPath`/`GraphDistance` are called | behavior is byte-identical to before this ticket (BFS) | `FindShortestPath[CycleGraph[6],1,4]` | unchanged from pre-ticket output | +| AC-4 | A graph with a symbolic weight | `FindShortestPath[g,s,t]` is called | falls back to unweighted BFS, does not error | `FindShortestPath[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{a,7}],1,3]` | `{1, 2, 3}` (BFS fallback, not unevaluated) | +| AC-5 | A graph with a negative weight | `GraphDistance[g,s,t]` is called | falls back to unweighted BFS | `GraphDistance[Graph[{1,2,3},{1->2,2->3},EdgeWeight->{-1,7}],1,3]` | `2` (BFS hop count, not a Dijkstra artifact) | +| AC-6 | An undirected weighted graph | `FindShortestPath[g,s,t]` is called | weights apply symmetrically | `FindShortestPath[Graph[{1,2,3},{1<->2,2<->3},EdgeWeight->{1,1}],1,3]` | `{1, 2, 3}` | +| AC-7 | An unreachable target on a weighted graph | `FindShortestPath`/`GraphDistance` are called | unreachable semantics unchanged | `FindShortestPath[Graph[{1,2,3},{1->2},EdgeWeight->{5}],1,3]` | `{}`; `GraphDistance[...]` → `Infinity` | + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Dispatch mechanism, fallback rule, `GraphAdj` scope — see Decisions (from research). + +## Plan Review + +### Blocking +_None._ + +### Worth Flagging +_None._ + +### Resolved +**[BLOCKING] `double dist[]` has no stated path back to an exact `Expr`, so AC-2 is not +achievable as specified** +- Where: was in Phase 1 §2; `EXPR_REAL` prints distinctly from `EXPR_INTEGER` + (`src/print.c`), and `assert_eval_eq` does exact string comparison, so a raw + `expr_new_real(dist[it])` would print `12.` against AC-2's exact `3`/`12`. +- How addressed: `double dist[]` is now stated as internal-comparison-only; the actual + returned value is reconstructed exactly via `evaluate(Plus[w1,...,wk])` over the real + `Expr*` weights along the discovered path (Phase 1 §2, rewritten). + +**[BLOCKING] The claim that both builtins' AC-11 lines in `test_edge_weights` need to change +was false for `FindShortestPath`** +- Where: was in Decisions/Components & Files Affected; verified against + `tests/test_graph.c:350-359` directly — the AC-11 test graph + (`Graph[{1,2,3},{1->2,2->3},EdgeWeight->{5,7}]`) has exactly one path from 1 to 3, so BFS + and Dijkstra agree on `FindShortestPath`'s result; only `GraphDistance`'s hop-count `"2"` + needs to become the weighted total `"12"`. +- How addressed: corrected everywhere this was claimed — only `GraphDistance`'s AC-11 + assertion changes. + +**[WORTH FLAGGING] The numeric-type list for `graph_weights_usable` omitted `EXPR_MPFR` (a +live leaf type under the default `USE_MPFR ?= 1` build) and reinvented rather than reused +`expr_is_numeric_like` (`src/expr.c:412`), which already covers it** +- How addressed: `graph_weights_usable` now explicitly reuses `expr_is_numeric_like` (minus + `Complex`) instead of a hand-rolled type list. + +**[WORTH FLAGGING] "6 other builtins" / "7 call sites across 5 files" were both wrong — +`StronglyConnectedComponents` does not call `graph_build_adj` at all (own Tarjan structure); +the real numbers are 5 builtins / 6 call sites / 4 files. Same miscount was inherited from +ticket 1's (already-shipped) plan.** +- How addressed: corrected everywhere in this plan; ticket 1's already-merged plan/tests are + left as historical record (not retroactively edited — see journal `KIT-FEEDBACK-GRAPH.md` + for the discussion of whether to fix it there). + +## Requires Approval +_None._ — this is the named follow-up from ticket 1's own Non-goals; no new scope call. + +## Architecture Impact +- New services introduced: none +- APIs changed: none +- Data crossing a service boundary: none +- New external dependency: none +- Deployment topology change: none + +(Behavior of two existing builtins changes for weighted graphs only, per ticket 1's own +Non-goals naming this as the intended follow-up — not an API contract break, since an +unweighted graph's behavior is provably unchanged, AC-3.) + +## Subsystems & Dependencies +- Subsystems touched: graph (invocation: inline) +- Interdependencies surfaced: none + +## Risks and Rollback +_None — standard tier, no architectural impact._ + +--- + +## Current State Analysis +- `src/graph/shortestpath.c` (full file, see research) — `bfs()`, `resolve()`, and both + builtins, no weight awareness. +- `src/graph/graph.h:112-120` — `GraphAdj` has no weight field; `graph_build_adj` has 6 call + sites across 4 files (`components.c` x2, `connectivity.c` x2, `spanningtree.c`, + `shortestpath.c`), used by 5 other builtins (`ConnectedComponents`, + `WeaklyConnectedComponents`, `FindSpanningTree`, `ConnectedGraphQ`, `VertexConnectivity`) — + `StronglyConnectedComponents` builds its own Tarjan-specific structure and does not call + `graph_build_adj`. +- `src/graph/graph_util.c` — `graph_resolve_edge_weights(g)` (ticket 1) already gives the + per-edge weight list in `EdgeList` order. +- `tests/test_graph.c`'s `test_edge_weights` AC-11 block asserts the exact "ignore weights" + behavior this ticket removes for `FindShortestPath`/`GraphDistance` specifically. + +## Desired End State +Both builtins dispatch to Dijkstra on a non-negative-numeric-weighted graph, BFS otherwise; +`tests/test_graph.c` reflects the new behavior; `docs/spec/builtins/graphs.md` and this +week's changelog are updated. Verify via the Acceptance Criteria table above run against the +live REPL, plus `make check-c99`, `make check-packed-aware`, and the full `graph_tests` +suite. + +### Key Discoveries: +- Weight resolution is already solved by ticket 1's `graph_resolve_edge_weights` — this + ticket is purely the Dijkstra algorithm plus the numeric/non-negative gate, not new + weight-plumbing. + +## Components & Files Affected + +| File | Change | +|---|---| +| `src/graph/shortestpath.c` | Add a local weighted-adjacency builder + O(V²) Dijkstra (`double dist[]` for internal vertex-selection comparisons only) + exact-value reconstruction (see below); both builtins check for a usable `EdgeWeight` (via a new `graph_weights_usable(g)` helper) and dispatch to Dijkstra or the existing BFS accordingly | +| `src/graph/graph_util.c`, `src/graph/graph.h` | New helper `graph_weights_usable(const Expr* g)`: `true` iff `g` has a 3-arg `EdgeWeight` and every weight satisfies `expr_is_numeric_like(w)` (the codebase's own existing generic numeric-type check — `src/expr.c:412`, already covers `EXPR_INTEGER`/`EXPR_BIGINT`/`EXPR_REAL`/`EXPR_MPFR`/`Rational`), is not `Complex`, and is `>= 0` | +| `tests/test_graph.c` | Update `test_edge_weights`'s `GraphDistance` AC-11 line only (`"2"` → `"12"`, the weighted total `5+7`) — `FindShortestPath`'s AC-11 assertion (`{1, 2, 3}`) is unaffected, since that specific test graph has only one path from vertex 1 to vertex 3, so BFS and Dijkstra necessarily agree on it. Add a new `test_weighted_shortest_path` for AC-1 through AC-7 (which do exercise multi-path graphs) | +| `docs/spec/builtins/graphs.md` | Update the `FindShortestPath`/`GraphDistance` bullets and remove the "remain unweighted BFS ... documented future extension" note added by ticket 1 | +| `docs/spec/changelog/2026-08-17.md` | New entry | + +## Core Flow Diagram + +```mermaid +flowchart TD + A["FindShortestPath[g,s,t] / GraphDistance[g,s,t]"] --> B{graph_weights_usable(g)?} + B -->|no: unweighted, symbolic, or has a negative weight| C[existing BFS path, unchanged] + B -->|yes: EdgeWeight present, all non-negative numeric| D[build local weighted adjacency] + D --> E[O(V^2) Dijkstra from s] + E --> F[reconstruct path / distance to t] + C --> G[return] + F --> G[return] +``` + +## Alternatives Considered + +### Extend `GraphAdj` with a weight array +**Rejected because:** `graph_build_adj` is shared by 6 call sites across 4 files +(`components.c`, `connectivity.c`, `spanningtree.c`, `shortestpath.c`) — widening it is +exactly the kind of shared-choke-point risk ticket 1's `plan-reviewer` pass caught as a real +defect. A local structure confined to `shortestpath.c` has zero blast radius on the other 6 +builtins. + +### A separate `WeightedFindShortestPath` builtin +**Rejected because:** real Wolfram Language dispatches on graph properties automatically, +not by function name — matching that is both more faithful and avoids two names for what a +user thinks of as one operation. + +## Implementation Approach +Add `graph_weights_usable(g)` (graph_util.c) as the single gate both builtins check. Build +Dijkstra as a self-contained static function in `shortestpath.c`, parallel to the existing +`bfs()`, operating over a locally-built vertex-indexed weighted adjacency (reusing +`graph_resolve_edge_weights` + the same `GraphVIdx` index pattern already used elsewhere). +Both `builtin_find_shortest_path`/`builtin_graph_distance` branch once, at the top, on +`graph_weights_usable`. + +## Phase 1: `graph_weights_usable` + Dijkstra + dispatch + +### Overview +Implement the gate, the algorithm, and wire both builtins to it. + +### Changes Required: + +#### 1. Weight-usability gate +**File**: `src/graph/graph_util.c`, declared in `graph.h` +**Changes**: `int graph_weights_usable(const Expr* g)` — `graph_is_valid(g)` first; +`arg_count != 3` → `false`; otherwise walk `graph_resolve_edge_weights(g)` and require every +entry to satisfy `expr_is_numeric_like(w)` (`src/expr.c:412` — the codebase's own existing +generic numeric-type check, already covering `EXPR_INTEGER`/`EXPR_BIGINT`/`EXPR_REAL`/ +`EXPR_MPFR`/`Rational`; do not hand-roll a narrower type list), be non-`Complex`, and convert +to a `double >= 0` via a small local `graph_weight_to_double(w)` helper (handles each of the +numeric leaf types `expr_is_numeric_like` accepts). + +#### 2. Dijkstra, and how the exact output value is produced +**File**: `src/graph/shortestpath.c` +**Changes**: a static `dijkstra()` mirroring `bfs()`'s signature/shape (fills `parent[]`), +using a `double dist[]` **for internal vertex-selection comparisons only** — O(V²) array scan +for the minimum-unvisited-distance vertex each iteration (no heap, matching +`VertexConnectivity`'s existing complexity precedent). This resolves a real gap a +`plan-reviewer` pass caught in the previous draft: a raw `double` accumulator returned +directly as `GraphDistance`'s result would print as `EXPR_REAL` (e.g. `12.`, per +`src/print.c`'s real-vs-integer formatting), not the exact `12` AC-2 expects, and this +codebase treats exact arithmetic as load-bearing throughout. Fix: once `dijkstra()` finds the +parent chain to `t`, reconstruct the **exact** total by evaluating `Plus[w1, ..., wk]` (via +`evaluate()`) over the actual `Expr*` weights (from `graph_resolve_edge_weights`, matched to +the path's edges) — the same exact-arithmetic path every other numeric builtin in this +codebase already goes through, giving `GraphDistance` an exact `EXPR_INTEGER`/`Rational` +result whenever the inputs are exact, and only falling to `EXPR_REAL` if a weight genuinely +was (e.g. `EXPR_MPFR`). `FindShortestPath` needs no such reconstruction — it returns the +vertex path, not a distance value, and the `double`-based selection is only ever used to +choose *which* path, never printed itself. + +#### 3. Dispatch +**File**: `src/graph/shortestpath.c` +**Changes**: both builtins check `graph_weights_usable(g)` once, at the top (before/instead +of building the plain `GraphAdj`), and call `dijkstra()` or the existing `bfs()` accordingly. + +### Success Criteria: + +#### Automated Verification: +- [x] Build succeeds: `make -j$(nproc)` (with `SDKROOT` set per GR-08) +- [x] Portability gate passes: `make check-c99` +- [x] Packed-array audit unaffected: `make check-packed-aware` +- [x] `tests/test_graph.c` passes: updated `test_edge_weights` + new + `test_weighted_shortest_path` covering AC-1 through AC-7 + +#### Manual Verification: +- [x] Every Acceptance Criteria row run against the live `./Mathilda -file` REPL, matching + Expected exactly +- [x] Pre-existing unweighted `FindShortestPath`/`GraphDistance` tests (from the original + subsystem commit) still pass unmodified + +**Implementation Note**: single-phase ticket; proceed straight to docs/changelog after this +phase's verification, per this session's "execute continuously once approved" convention. + +--- + +## Testing Strategy +Extend `tests/test_graph.c` directly, same convention as ticket 1: update the superseded +AC-11 assertions in `test_edge_weights`, add `test_weighted_shortest_path` for the new +behavior. + +### Edge Cases & Integration Scenarios: +- Multi-path graphs where hop-count and total-weight disagree (AC-1/AC-2) — the case that + actually distinguishes Dijkstra from BFS +- Symbolic and negative weights (AC-4/AC-5) — the fallback path, not just the happy path +- Undirected weighted graphs (AC-6) — weight symmetry +- Unreachable target (AC-7) — unchanged semantics + +### Manual Testing Steps: +1. Build and start `./Mathilda` +2. Run every Acceptance Criteria row, compare to Expected + +## Performance Considerations +O(V²) per call — consistent with `VertexConnectivity`'s existing exact/small-graph +complexity tolerance in this subsystem; not intended for large graphs. + +## Migration Notes +None — additive dispatch; unweighted graphs are provably unaffected (AC-3). + +## References +- Research: `thoughts/shared/research/2026-08-23-weighted-shortest-path.md` +- Prior ticket (source of this follow-up): `thoughts/shared/plans/2026-08-22-graph-edge-weights.md` +- Direct template: `src/graph/shortestpath.c`'s existing `bfs()` + +## Implementation Notes (post-hoc) + +All Acceptance Criteria (AC-1 through AC-7) executed directly against the built `./Mathilda` +binary and matched exactly, including the exact-integer check (`Head[GraphDistance[...]]` → +`Integer`, not `Real`) that the plan-reviewer's first BLOCKING finding exists to guard. +`make check-c99`/`make check-packed-aware` both exit 0. `graph_tests` (17 tests, including +the new `test_weighted_shortest_path`) passes standalone. The verification ladder's `unit` +rung reports FAILED for the same pre-existing, unrelated reason documented in +`KIT-FEEDBACK-GRAPH.md` GR-13 (an unrelated flaky optimization test halts the +`for t in *_tests` loop alphabetically before `graph_tests` runs) — not a regression from +this change. diff --git a/thoughts/shared/research/2026-08-22-graph-edge-weights-extension-summary.md b/thoughts/shared/research/2026-08-22-graph-edge-weights-extension-summary.md new file mode 100644 index 000000000..c53d59bef --- /dev/null +++ b/thoughts/shared/research/2026-08-22-graph-edge-weights-extension-summary.md @@ -0,0 +1,58 @@ +--- +created: 2026-08-23T02:49:49Z +researcher: Michael Sollami +topic: "What genuine, few-hours-sized extension to Graph/HyperGraph would a maintainer want?" +type: research +lifecycle: active +full_research: thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md +--- + +# Research Summary: What genuine, few-hours-sized extension to Graph/HyperGraph would a maintainer want? + +**Full research (appendix)**: `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md` + +## Recommendation +Add edge weights + `WeightedAdjacencyMatrix` (+ a matching `EdgeWeight[g]` query builtin). +This is the one extension the codebase itself already names as pre-approved future work +(`src/graph/adjmat.c:9-10`, `docs/spec/builtins/graphs.md:19-21`), it's unimplemented, +untested, and has zero downstream consumers to migrate — right-sized for a few hours. +HyperGraph does not exist and is explicitly locked out of MVP scope; building it properly +(construction, validation, printing, its own query/generator/algorithm surface) is a +multi-day effort and would override a documented scope decision, not extend it. + +## Options Considered +1. **Edge weights + `WeightedAdjacencyMatrix` + `EdgeWeight[g]`** — pre-approved in-code, + zero downstream migration, self-contained to `src/graph/`. Chosen. +2. **HyperGraph from scratch** — explicitly locked out of MVP scope + (`docs/spec/builtins/graphs.md:20`); needs its own construction/validation/printing/ + query/generator surface — a week-plus effort, not a few hours. +3. **`GraphPlot` arrowheads / force-directed layout** — real gaps, but cosmetic/visual only; + doesn't touch graph semantics or add queryable functionality. +4. **`VertexConnectivity` test hardening** — a legitimate but much smaller test-only task, + not really "extending functionality." + +## Decision Criteria +- Must be justified by what the code/docs/tests actually show, not convenience: option 1 is + the only one the source explicitly names as intended future work. +- Must fit "a few hours": option 1 is one new small builtin plus a constructor extension + that's additive to the existing 2-arg canonical form; option 2 fails this outright. +- Must not contradict a locked scope decision: option 2 would. + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Should `FindShortestPath`/`GraphDistance` gain a weighted (Dijkstra) mode in this same + pass? — No, confirmed with maintainer 2026-08-23; deferred to a follow-up. +- [x] HyperGraph existence — confirmed absent, confirmed locked out of scope. +- [x] Pre-approved next extension — confirmed: weighted edges / `WeightedAdjacencyMatrix`. +- [x] Downstream consumers of `AdjacencyMatrix`/`IncidenceMatrix` assuming 0/1 values — none. +- [x] Packed/NDArray/Compile[] mandate applicability — none of the existing graph builtins + are on `src/pack.c`'s `AWARE` list; structural/constructive, consistent precedent. +- [x] Prior attempt / known constraint — none; confirmed directly with the maintainer. + +## Requires Approval +Scope: edge weights + `WeightedAdjacencyMatrix` + `EdgeWeight[g]` only, no weighted +shortest-path. See Unresolved above. diff --git a/thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md b/thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md new file mode 100644 index 000000000..39d815f62 --- /dev/null +++ b/thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md @@ -0,0 +1,186 @@ +--- +created: 2026-08-23T02:49:49Z +researcher: Michael Sollami +source_sha: 6a9a6259768a0d5516bba7747220d70835a3a79f +branch: main +repository: mathilda +topic: "What genuine, few-hours-sized extension to Graph/HyperGraph would a maintainer want?" +tags: [research, codebase, graph, hypergraph, weighted-graph] +subsystems: [graph] +type: research +lifecycle: active +status: complete +last_updated: 2026-08-23 +last_updated_by: Michael Sollami +--- + +# Research: What genuine, few-hours-sized extension to Graph/HyperGraph would a maintainer want? + +**Date**: 2026-08-23T02:49:49Z +**Researcher**: Michael Sollami +**Git Commit**: 6a9a6259768a0d5516bba7747220d70835a3a79f +**Branch**: main +**Repository**: mathilda + +## TL;DR +HyperGraph does not exist anywhere in this codebase and is explicitly locked out of MVP +scope alongside multigraphs/parallel-edges/edge-tags — building it properly is a multi-day +effort, not a few hours, and would contradict a documented scope decision. Edge weights and +`WeightedAdjacencyMatrix`, by contrast, are the one thing the source itself flags as a +pre-approved "future hook" (`src/graph/adjmat.c:9-10`, `docs/spec/builtins/graphs.md:19-21`) +and are unimplemented, untested, and unconsumed downstream — the right-sized, justified +choice. Open question: whether weighted variants of `FindShortestPath`/`GraphDistance` +belong in this pass (recommend: no, out of scope). + +## Summary +`src/graph/` (19 files, ~1850 LoC, 27 builtins, all `ATTR_PROTECTED`) implements a +Mathematica-style simple-graph subsystem: construction/validation, query/representation, +matrix views, generators, BFS/Tarjan-based search, and a circle-layout `GraphPlot`. Three +commits total (`56035303` initial add, `de2ef6e4` unrelated evaluator perf work, +`8d71d845` a vertex-index perf fix) — no weighted/multigraph/hypergraph exploration has +happened. `docs/spec/builtins/graphs.md:19-21` locks the MVP to simple graphs with **no +hypergraphs, no multigraphs, no edge tags, no edge/vertex weights** — except it explicitly +carves out `WeightedAdjacencyMatrix`/edge weights as a **documented future extension**, and +`src/graph/adjmat.c:9-10`'s own comment says so again in-code. `AdjacencyMatrix`/ +`IncidenceMatrix` output is consumed nowhere internally (`src/linalg/`, `src/compile/` never +reference them) — a weighted variant needs no downstream migration. Neither is on +`src/pack.c`'s `AWARE` list; graph builtins are structural (operate on graph trees, not +numeric buffers), so this is consistent precedent for a new matrix-returning builtin to +follow, not a gap this change needs to close. + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Should `FindShortestPath`/`GraphDistance` gain a weighted (Dijkstra) mode in the same + pass, now that weights exist? — No; confirmed with the maintainer directly + (`AskUserQuestion`, 2026-08-23). Keeps this a self-contained, few-hours change; weighted + shortest-path is a new algorithm, not a new builtin, and is deferred to a follow-up. +- [x] Does HyperGraph exist anywhere in this codebase? — No (exhaustive grep across + `src/`, `tests/`, `docs/`, `src/internal/*.m`); it is named only as an explicitly + out-of-scope MVP exclusion in `docs/spec/builtins/graphs.md:20`. +- [x] Is there a pre-approved "next" extension already flagged in the code? — Yes: + `WeightedAdjacencyMatrix`/edge weights, `src/graph/adjmat.c:9-10` and + `docs/spec/builtins/graphs.md:19-21`. +- [x] Do any consumers assume `AdjacencyMatrix`/`IncidenceMatrix` are exactly 0/1/-1? — No; + grepped all of `src/` and `src/internal/*.m` — no internal consumer exists at all. +- [x] Does the packed/NDArray/Compile[] mandate (CLAUDE.md) apply here? — No current graph + builtin (including the existing `AdjacencyMatrix`) is on `src/pack.c`'s `AWARE` list; + they are structural/constructive, not elementwise-numeric. A new matrix-returning + builtin follows the same, already-established precedent. +- [x] Any prior attempt or deliberate constraint the maintainer already knows about? — + Asked directly (grill-me, research-open); answer: no, research fresh from the code. + +## Requires Approval +Whether to scope this to edge weights + `WeightedAdjacencyMatrix` + an `EdgeWeight[g]` +query builtin only (recommended), versus also touching `FindShortestPath`/`GraphDistance` +to make them weight-aware. Recommend the narrower scope; flagged above as Unresolved. + +--- + +## Research Question +"Extend the Graph and HyperGraph functionality in this codebase. Pick a genuine, useful +extension that a maintainer would actually want, sized at a few hours rather than a week, +justified from what the code and tests actually show." + +## Detailed Findings + +### Graph subsystem inventory +- `src/graph/graph.c` registers 27 builtins (`Graph`, `GraphQ`, `VertexList`, `EdgeList`, + `VertexCount`, `EdgeCount`, `AdjacencyList`, `VertexDegree`, `VertexInDegree`, + `VertexOutDegree`, `DirectedGraphQ`, `AdjacencyMatrix`, `IncidenceMatrix`, + `AdjacencyGraph`, `CompleteGraph`, `CycleGraph`, `PathGraph`, `RandomGraph`, + `FindShortestPath`, `GraphDistance`, `ConnectedComponents`, + `WeaklyConnectedComponents`, `StronglyConnectedComponents`, `FindSpanningTree`, + `ConnectedGraphQ`, `VertexConnectivity`, `GraphPlot`), each `symtab_add_builtin` call + paired with `symtab_set_docstring` and `ATTR_PROTECTED`. +- One builtin per `.c` file (mirrors `src/linalg/`'s layout), per `src/graph/graph.h:1-22`'s + header comment. +- Canonical form: `Graph[List[verts], List[edges]]`, edges `DirectedEdge[u,v]` / + `UndirectedEdge[u,v]`, `Rule`/`TwoWayRule` accepted as construction-time sugar + (`src/graph/graph.h:8-13`). + +### Locked scope and the one documented exception +`docs/spec/builtins/graphs.md:19-21`: +> "MVP scope (locked): simple graphs only — no parallel edges, no self-loops, no edge tags, +> no multigraphs, no hypergraphs, and no edge/vertex weights. `WeightedAdjacencyMatrix` and +> edge weights are a documented future extension." + +`src/graph/adjmat.c:9-10` (in-code, right next to the current `AdjacencyMatrix` +implementation): +> "Future hook: a WeightedAdjacencyMatrix would fill entries with edge weights instead of 1 +> (Locked Decision 2); not implemented in the MVP." + +No other TODO/FIXME/"not implemented" comment exists anywhere in `src/graph/*.c`. The only +other named future hooks are cosmetic: arrowheads for directed edges in `GraphPlot` +(`docs/spec/builtins/graphs.md:156-157`) and a force-directed layout +(`src/graph/graphplot.c`), neither of which changes graph semantics or query results. + +### Test coverage (`tests/test_graph.c`, 319 lines, 15 test functions, all passing at +`source_sha` above — verified: `./tests/build-main/graph_tests` → "All graph tests passed!") +Every one of the 27 builtins has at least one asserting test. Thinnest coverage: +`VertexConnectivity` (4 hardcoded cases: `PathGraph[4]→1`, `CycleGraph[5]→2`, +`CompleteGraph[4]→3`, a disconnected pair→0) and boundary shapes (single-vertex, +zero-edge, disconnected graphs) get one or two cases each rather than systematic coverage. +No existing test constructs a graph with a third, option-style constructor argument — the +constructor's argument-count handling is exactly `Graph[v,e]`/`Graph[e]` today +(`src/graph/construct.c`), which is the surface a weighted-edge extension has to grow +without breaking. + +### Downstream consumption of matrix output +`AdjacencyMatrix`/`IncidenceMatrix` are grepped across the entire `src/` tree and +`src/internal/*.m`: the only hits are the symbol interning table +(`src/sym_names.{h,c}`) and the graph subsystem's own files. No internal consumer in +`src/linalg/`, `src/compile/`, or elsewheer assumes the matrix entries are literally `0`/`1` +(or `-1/0/1` for the incidence form) — a real-valued weighted variant requires zero changes +outside the graph subsystem. + +### Packed/NDArray/Compile[] applicability +`src/pack.c`'s `AWARE` list (the CLAUDE.md-mandated registry of heads with a numeric fast +path) contains no `Graph`/`Adjacency`/`Vertex`/`Edge` entry — confirmed by grep — and +neither does its `NOT_AWARE`/`INT64_OK` commentary. This is not a gap introduced by this +research: `AdjacencyMatrix`, already shipped, is in the same position. Graph builtins +consume/produce compound `Expr` trees (graphs, vertex/edge lists), not numeric buffers — +CLAUDE.md's own escape hatch ("a purely symbolic/structural head... genuinely cannot +support a surface") applies to the whole subsystem as shipped. A new `WeightedAdjacencyMatrix` +follows the identical shape as `AdjacencyMatrix` (builds a fresh matrix from graph structure) +and inherits the same, already-established position — not a new exemption to invent. + +### Git history +Three commits touch `src/graph/`: `56035303` (initial subsystem, 2026-06-29), `de2ef6e4` +(unrelated evaluator-wide perf work, 2026-07-30), `8d71d845` (vertex-index perf fix, +2026-08-05, motivated by a 20000-vertex benchmark). No weighted-graph or hypergraph work has +been attempted or reverted. + +## Code References +- `src/graph/graph.h:1-22` — subsystem header comment, canonical form, ownership contract +- `src/graph/graph.c` — builtin registration, attributes, docstrings (all 27 builtins) +- `src/graph/construct.c` — `Graph[...]` construction/validation/normalization +- `src/graph/adjmat.c:9-10` — the in-code `WeightedAdjacencyMatrix` future-hook comment +- `docs/spec/builtins/graphs.md:19-21` — the locked MVP-scope paragraph +- `tests/test_graph.c` — the full existing test suite (baseline: all 15 tests pass) +- `src/pack.c` — `AWARE`/`NOT_AWARE`/`INT64_OK` registries (no graph entries either way) + +## Architecture Insights +- One-builtin-per-file, hub-registers-in-`graph.c` is the fixed convention (mirrors + `src/linalg/`); a new builtin follows the same shape. +- Options-as-trailing-`Rule[]` is the established idiom for optional builtin arguments + elsewhere in the codebase (`src/numerical_calculus/nderiv.c:531-566`, + `src/numerical_calculus/ndsolve.c:90-109` — a local `is_option`/`apply_option` pair per + module), not the global `Options`/`OptionValue`/`SetOptions` registry + (`src/options_builtin.c`), which is for symbols whose options are queried/set + independently of a single call. `Graph[v, e, EdgeWeight -> {...}]` should follow the + local trailing-rule idiom, matching how the constructor already accepts `Rule`/ + `TwoWayRule` edge sugar positionally. +- Vertex/edge lookups already have an O(1) hash-index helper (`GraphVIdx`, + `src/graph/graph_util.c`) introduced specifically to keep new graph builtins from + reintroducing the O(E·V) scans fixed in `8d71d845` — any new builtin should build on it + rather than re-scanning `List` args linearly. + +## Historical Context (from thoughts/) +None — `thoughts/` did not exist before this research (created for `NOTES_DIR` output). + +## Related Research +None yet — this is the first research document under `thoughts/shared/research/`. diff --git a/thoughts/shared/research/2026-08-23-weighted-shortest-path-summary.md b/thoughts/shared/research/2026-08-23-weighted-shortest-path-summary.md new file mode 100644 index 000000000..d3eac43b6 --- /dev/null +++ b/thoughts/shared/research/2026-08-23-weighted-shortest-path-summary.md @@ -0,0 +1,52 @@ +--- +created: 2026-08-23T04:15:30Z +researcher: Michael Sollami +topic: "Second graph extension: weighted shortest path" +type: research +lifecycle: active +full_research: thoughts/shared/research/2026-08-23-weighted-shortest-path.md +--- + +# Research Summary: Weighted Shortest Path + +**Full research (appendix)**: `thoughts/shared/research/2026-08-23-weighted-shortest-path.md` + +## Recommendation +Make `FindShortestPath`/`GraphDistance` weight-aware (Dijkstra) when the graph carries +non-negative numeric `EdgeWeight`, matching real Wolfram Language semantics and closing the +exact gap ticket 1's own Non-goals named as deferred follow-up work. Build a local, +call-scoped weighted adjacency rather than extending the shared `GraphAdj` structure 8 other +builtins depend on. + +## Options Considered +1. **Dijkstra dispatch inside the existing two builtins, local weighted structure** (chosen) + — matches real WL behavior, minimal blast radius, reuses ticket 1's + `graph_resolve_edge_weights`. +2. **Extend `GraphAdj` itself with a weight array** — rejected: widens a structure 8 builtins + share, the exact shape of risk ticket 1's `plan-reviewer` pass flagged as a real defect. +3. **New, separately-named builtins (`WeightedFindShortestPath`)** — rejected: not how real + Wolfram Language behaves (weight-awareness is automatic based on graph properties, not a + separate function name), and duplicates two builtins instead of completing them. + +## Decisions +- Dijkstra fires only when `EdgeWeight` is present AND every weight is non-negative numeric; + otherwise, falls back to the existing unweighted BFS exactly as before (fails safe, never + regresses a previously-working call to unevaluated). +- No change to `GraphAdj`/`graph_build_adj` — a separate, local structure only where needed. + +## Non-goals +No Bellman-Ford / negative-weight support. No change to any other `graph_build_adj`-routed +builtin. No A*/bidirectional search — plain O(V²) Dijkstra, matching this codebase's own +complexity tolerance for small-graph exact algorithms (`VertexConnectivity`'s precedent). + +## Open Questions + +### Unresolved +_None._ + +### Resolved +See full research — all three resolved directly from the codebase, no maintainer +consultation needed for this pass beyond what ticket 1 already decided. + +## Requires Approval +None — this is the explicit, named follow-up from ticket 1's own Non-goals. diff --git a/thoughts/shared/research/2026-08-23-weighted-shortest-path.md b/thoughts/shared/research/2026-08-23-weighted-shortest-path.md new file mode 100644 index 000000000..79caa89f6 --- /dev/null +++ b/thoughts/shared/research/2026-08-23-weighted-shortest-path.md @@ -0,0 +1,136 @@ +--- +created: 2026-08-23T04:15:30Z +researcher: Michael Sollami +source_sha: 3d87224771270a91d7402de3f8b9908dd33e58c8 +branch: main +repository: mathilda +topic: "Second graph extension: what does the code/tests show a maintainer would want next?" +tags: [research, codebase, graph, shortest-path, dijkstra] +subsystems: [graph] +type: research +lifecycle: active +status: complete +last_updated: 2026-08-23 +last_updated_by: Michael Sollami +--- + +# Research: Second graph extension — weighted shortest path + +**Date**: 2026-08-23T04:15:30Z +**Researcher**: Michael Sollami +**Git Commit**: 3d87224771270a91d7402de3f8b9908dd33e58c8 +**Branch**: main +**Repository**: mathilda + +## TL;DR +Ticket 1 (edge weights) explicitly deferred making `FindShortestPath`/`GraphDistance` +weight-aware, naming it in its own `Non-goals` as the natural next step. `GraphAdj` +(`graph_util.c`, shared by 8 builtins) stores **no per-edge weight at all** — only +successor/predecessor vertex indices — so real Wolfram Language semantics (both builtins +auto-dispatch to a weighted algorithm when `EdgeWeight` is present) require a real Dijkstra +implementation, not a config flag. This is harder than ticket 1: a new algorithm, not just a +new builtin, and it changes two existing builtins' behavior on weighted graphs rather than +adding new read-only ones. Sized at a few hours given a simple O(V²) array-based Dijkstra +(matching this codebase's own precedent: `VertexConnectivity`'s docstring calls itself +"exact brute-force ... intended for small graphs"). + +## Summary +`src/graph/shortestpath.c` implements unweighted BFS for both `FindShortestPath[g,s,t]` and +`GraphDistance[g,s,t]`, routed through the shared `GraphAdj` (`graph_build_adj`). Real +Wolfram Language's own `FindShortestPath`/`GraphDistance` automatically use edge weights when +present and fall back to unweighted BFS otherwise — that is the behavior ticket 1's own +research and plan explicitly named as deferred (`thoughts/shared/research/ +2026-08-22-graph-edge-weights-extension.md`'s Non-goals: *"No weighted-shortest-path / +Dijkstra mode ... deferred to a follow-up"*). `GraphAdj` has no weight storage, so this +requires building a small, local weighted-adjacency pass (reusing `graph_resolve_edge_weights` +from ticket 1) rather than touching the shared structure 8 other builtins depend on — +learning directly from ticket 1's plan-reviewer-caught lesson about `graph_build_adj` being a +sensitive shared choke point. + +## Open Questions + +### Unresolved +_None._ + +### Resolved +- [x] Does `GraphAdj` already carry weights that a Dijkstra pass could reuse? — No, confirmed + by reading `graph_util.c`'s `GraphAdj` struct and `graph_build_adj`'s fill loop: only + `int` successor/predecessor indices, no weight field anywhere. + - [x] Should this touch the shared `GraphAdj`/`graph_build_adj`? — No: build a local, + call-scoped weighted adjacency inside `shortestpath.c` instead, to avoid widening the + blast radius of a structure 8 builtins depend on (direct lesson from ticket 1's + `plan-reviewer` finding). +- [x] How should non-numeric or negative weights be handled? — Fall back to the existing + unweighted BFS behavior rather than failing: Dijkstra requires non-negative numeric + weights to be correct, and a previously-working call should not start returning + unevaluated just because a graph happens to carry a symbolic or negative weight. + Documented as an explicit limitation, matching this codebase's existing style + (`VertexConnectivity`'s own "intended for small graphs" self-limitation). +- [x] Prior attempt or known constraint? — None found in git history (`git log --oneline -- + src/graph/shortestpath.c` shows only the original subsystem-add commit); this session's + own ticket-1 Non-goals is the only prior signal, and it points at doing exactly this. + +## Requires Approval +None beyond what ticket 1 already flagged and deferred to "a follow-up" — this is that +follow-up. + +--- + +## Research Question +"A second, harder ticket in this repo, chosen the same way as the first — from what the +code and tests show a maintainer would want, sized at a few hours." + +## Detailed Findings + +### Current implementation (`src/graph/shortestpath.c`, full file read) +- `bfs()` (lines 20-33): unweighted BFS over `GraphAdj.out[]`, O(V+E). +- `resolve()` (lines 36-45): validates `g`, builds `GraphAdj` via `graph_build_adj`, + resolves `s`/`t` to vertex indices via `graph_vertex_index`. +- `builtin_find_shortest_path`/`builtin_graph_distance`: call `resolve` + `bfs`, no weight + awareness anywhere. + +### `GraphAdj` has no weight field +`graph_util.c`'s `GraphAdj` struct (declared in `graph.h:112-117`): `n`, `verts` +(borrowed), `outdeg`/`out`, `indeg`/`in` — all vertex-index arrays, no weight storage. Adding +a weight array here would touch `graph_build_adj`, used directly by `components.c` (x2), +`connectivity.c` (x2), `spanningtree.c`, and `shortestpath.c` (x2) — 7 call sites across 5 +files. Given ticket 1's `plan-reviewer` finding was specifically about this function being a +shared, easy-to-miss choke point, the lower-risk design keeps `graph_build_adj` completely +unchanged and builds a separate, call-scoped weighted structure only where Dijkstra is +actually used. + +### Weight resolution is already solved (ticket 1) +`graph_resolve_edge_weights(g)` (`graph_util.c`, added in ticket 1) already returns the +per-edge weight list in `EdgeList` order, defaulting to all-`1`s when unweighted — exactly +the input a Dijkstra pass needs, keyed against `EdgeList[g]`/`edges` the same way +`wtadjmat.c` already consumes it. + +### Test coverage +`tests/test_graph.c`'s `test_shortest_path` (added in the original subsystem commit) and +`test_edge_weights` (added in ticket 1, this session) both exist. `test_edge_weights`'s +AC-11 case explicitly asserts `FindShortestPath`/`GraphDistance` **ignore** weights on a +graph carrying them — a real, checked-in test of the exact limitation ticket 1 deliberately +left in place and named as deferred. Implementing this ticket requires **changing that +specific assertion** (not removing the AC-11 pattern — the other 6 `graph_build_adj`-routed +builtins in that test still assert unchanged, still-correct behavior). + +## Code References +- `src/graph/shortestpath.c` — the whole file; both builtins and their shared `resolve`/`bfs` +- `src/graph/graph.h:112-120` — `GraphAdj` struct and `graph_build_adj` declaration +- `src/graph/graph_util.c` — `graph_resolve_edge_weights` (ticket 1), reused here +- `tests/test_graph.c`'s `test_edge_weights` — the AC-11 assertions that must change +- `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md` — ticket 1's + Non-goals naming this exact follow-up + +## Architecture Insights +Same one-builtin-per-file convention; this ticket modifies two existing builtin files rather +than adding new ones, since the change is "make these two smarter," not "add two new +readers." The GraphVIdx O(1) index helper (ticket-1-adjacent infra) is reusable here too. + +## Historical Context (from thoughts/) +- `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md` and + `thoughts/shared/plans/2026-08-22-graph-edge-weights.md` — ticket 1, whose Non-goals and + GR-09 plan-reviewer finding both directly shape this ticket's scope and design. + +## Related Research +- `thoughts/shared/research/2026-08-22-graph-edge-weights-extension.md`