Skip to content

feat(launchpad): ProjectGraph with typed edges and traversal over buzz-core - #214

Merged
tucktuck101 merged 6 commits into
launchpadfrom
task/207-project-graph
Aug 20, 2026
Merged

tucktuck101 merged 6 commits into
launchpadfrom
task/207-project-graph

Conversation

@serina-mcfall

@serina-mcfall serina-mcfall commented Aug 18, 2026

Copy link
Copy Markdown

Summary

Implements all 6 steps of #207's plan: a ProjectGraph that materializes 4 of the design doc's 8
edge types (calls/called_by, tested_by, configured_by, documented_by) from #206's
already-indexed Symbol records, plus real BFS traversal (reachable()).

Stacked on #213 (this branch is task/206-project-indexer + this PR's own commits) since
#207 ingests #206's output directly. Base is task/206-project-indexer, not launchpad --
review this PR's own two commits; the earlier ones are #213's, already under review there.

Related issue

Refs #207 (a stacked PR's base isn't the default branch, so GitHub won't create a real closing
link from "Closes" here -- retarget to launchpad once #213 merges, then this can close #207
for real)

Issue type

Task


Agent provenance

Field Value
Harness / provider Claude Code
Model claude-sonnet-5
Session reference N/A - harness does not expose a session/run URL
Initiating human @serina-mcfall

Objective

Build a ProjectGraph that ingests #206's Symbol records into typed, directional edges and
answers traversal queries, per #207.

Impacted components

launchpad/project-intelligence/graph.py
launchpad/project-intelligence/test_graph.py

Approach and rejected alternatives

Chosen: implement 4 of the design doc's 8 edge types (calls/called_by, tested_by,
configured_by, documented_by) -- the ones #206's Symbol schema already produces directly.
Rejected: also building imports, deployed_by, owns, depends_on in this task. Those need
extraction #206 does not currently do (Rust use statements, deployment units, Cargo.toml
dependencies), which is a larger task than "ingest a ProjectIndexer's Symbol records" as #207's
own Objective states it. Flagged explicitly as an OPEN scope question in the plan rather than
silently deciding either way -- this needs a human call before #207 is considered fully done
against the design doc
, even though it satisfies #207's own stated Definition of done.

Chosen: derive called_by as calls[]'s own structural inverse inside ProjectGraph, rather
than also reading #206's separately-computed called_by[] field. Rejected reading both, since
#206 already computes called_by[] from calls[] itself (with_called_by()) -- treating them
as two independent sources would double every edge from the same underlying fact.

Verification

Command run:

$ python3 -m unittest test_symbol test_indexer test_graph

Raw output:

..................
----------------------------------------------------------------------
Ran 18 tests in 0.001s

OK

Command run:

$ python3 graph.py buzz-core

Raw output:

=== STEP 6: every edge for one chosen real symbol ===

is_shared_gated_kind --called_by--> tests::shared_gated_kinds_membership  (Symbol.calls[] (inverse))
is_shared_gated_kind --calls--> contains  (Symbol.calls[])
is_shared_gated_kind --tested_by--> tests::shared_gated_kinds_membership  (Symbol.tests[])
is_shared_gated_kind --documented_by--> ARCHITECTURE.md  (Symbol.documentation_links[])
is_shared_gated_kind --documented_by--> launchpad/plans/2026-08-18-issue-207-project-graph.md  (Symbol.documentation_links[])
is_shared_gated_kind --called_by--> is_unshared_gated_event  (Symbol.calls[] (inverse))

=== STEP 4: a 2-hop relationship the flat Symbol record alone cannot show ===

reachable('tests::is_unshared_gated_event_author_always_allowed', max_hops=2) includes 'is_shared_gated_kind': True
  path: tests::is_unshared_gated_event_author_always_allowed -> is_unshared_gated_event -> is_shared_gated_kind

=== STEP 5: the negative case -- vague questions need a starting symbol ===

Query: reachable(graph, 'the code that checks kind gating', ...)
Result: [] -- empty, because 'the code that checks kind gating' is not a symbol_id
this graph knows about. Resolving a description to a starting symbol is
#210's (SemanticIndex) job, not this graph's -- confirming the boundary.

Also separately confirmed a real tested_by edge (tests::shared_gated_kinds_membership) and a
real configured_by edge (service_resource -> OTEL_SERVICE_NAME, in buzz-relay) during
development, both cross-checked against real source lines.

A real bug was found and fixed during self-review, not by a test: tested_by and
documented_by initially had source/target reversed (read "the test is tested_by the symbol"
instead of "the symbol is tested_by the test") -- caught by reading the STEP 6 CLI output
literally, fixed, and a new EdgeDirectionTest (3 tests) added so the class of bug (right
construction, wrong direction, invisible to a test that only checks an edge exists) is caught by
the suite next time.

  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

Not verified

Only tested against buzz-core's indexed data (matching #206's own single-crate scope). Did not
measure traversal performance at a larger scale (the in-memory dict-of-lists structure is
adequate for 453 symbols; unverified beyond that). Did not verify behavior when #206's indexer
is re-run after the underlying code changes (a "dirty" re-index scenario) -- this task assumes a
fresh build_index() call each time, matching #206's own scope.

Security implications

N/A - a pure in-memory data structure over already-indexed data; no code execution, network
access, or trust-boundary changes beyond what #206 already does (which this PR does not modify).

Escalations

The OPEN item from the plan, restated here because it matters for whoever reviews this: is
4-of-8 edge types acceptable for #207 to be "done" against the design doc
, or does the design
doc's full edge-type list need a follow-up task (or a scope note added to #207 itself, the way
#193 did for the lefthook exception)? This PR satisfies #207's own Definition of done exactly as
written, but that list is narrower than the design doc's 8 edge types, and I did not decide
silently which reading governs.

… traversal

Plans the second task under PRD #4's Project Intelligence Layer scope:
a 6-step plan building a typed-edge graph over #206's already-indexed
Symbol records (calls/called_by, tested_by, configured_by,
documented_by -- the 4 of the design doc's 8 edge types directly
derivable from what #206 produces), plus real BFS traversal.

Worked example grounded in a real, verified chain in the already-
indexed buzz-core data:
tests::is_unshared_gated_event_author_always_allowed ->
is_unshared_gated_event -> is_shared_gated_kind.

OPEN flags the real scope question this plan doesn't resolve: whether
4-of-8 edge types (the ones #206's schema already produces) is
sufficient, since the other 4 (imports, deployed_by, owns, depends_on)
need extraction beyond "ingest a ProjectIndexer's Symbol records" as
#207's own Objective states it.

Mechanical checks clean via check-plan.sh.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…traversal

Implements the first 3 steps of #207's plan
(launchpad/plans/2026-08-18-issue-207-project-graph.md):

STEP 1: the Edge record (source, target, edge_type, evidence) and the
design doc's 9 edge-type names as a Literal.

STEP 2 (RUNS HERE): ProjectGraph.from_symbols() materializes 4 of the
design doc's 8 edge types from #206's Symbol records --
calls/called_by, tested_by, configured_by, documented_by. called_by is
derived as calls[]'s own structural inverse, not re-read from Symbol's
separately-computed called_by[] field, which would double-count the
same fact (#206 computes called_by[] from calls[] itself). Verified
against real buzz-core data: edges_from("is_shared_gated_kind")
exactly matches that symbol's own calls[]/called_by[] fields; separate
runs confirmed real tested_by (tests::shared_gated_kinds_membership)
and configured_by (service_resource -> OTEL_SERVICE_NAME, in buzz-relay)
edges too.

STEP 3: reachable() -- BFS over materialized edges, filtered by edge
type, bounded by max_hops. Fixture-based tests mirror the real 2-hop
chain (same reasoning as test_indexer.py: no rql dependency in the
unit suite), then verified live against buzz-core: querying from
tests::is_unshared_gated_event_author_always_allowed with
edge_types=("calls",), max_hops=2 puts is_unshared_gated_event at hop
1 and is_shared_gated_kind at hop 2, exactly as the plan specified.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Implements the last 3 steps of #207's plan:

STEP 4: reachable() finds a 2-hop relationship
(tests::is_unshared_gated_event_author_always_allowed ->
is_unshared_gated_event -> is_shared_gated_kind) that
is_shared_gated_kind's own flat called_by[] field does not contain --
verified explicitly, both are false and true respectively.

STEP 5: the negative case. A vague, terminology-free query has no
symbol_id to start reachable() from, so it correctly returns empty --
demonstrated running, not just asserted, confirming the documented
boundary with #210 (SemanticIndex).

STEP 6: graph.py's __main__ prints every edge for one chosen real
symbol, each explicitly typed (X --edge_type--> Y), matching the
design doc's checkout-flow trace shape.

Bug found while re-reading STEP 6's own printed output (not by any
test): tested_by and documented_by had source/target reversed relative
to configured_by's correct convention -- "test --tested_by--> symbol"
instead of "symbol --tested_by--> test", which reads backwards ("the
test is tested by the symbol"). Fixed both directions and added
EdgeDirectionTest (3 tests) so this exact class of bug -- correct
construction, wrong direction, invisible to a test that only checks
edges exist -- is caught by the suite next time, not just by eyeballing
CLI output.

Verified live against real buzz-core data after the fix:
is_shared_gated_kind now correctly shows "--tested_by-->
tests::shared_gated_kinds_membership" and "--documented_by-->
ARCHITECTURE.md", both readable the right way round.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
@serina-mcfall serina-mcfall added the by:agent Filed or authored by an AI agent, not a human label Aug 18, 2026
Codex's independent review of PR #214 confirmed all 3 findings (P2):

- Synthetic config-key and doc-path nodes were bare strings, so a
  symbol whose qualified_name happened to match a config key or doc
  path (e.g. a symbol literally named the same as an env var it reads)
  would collide onto the same graph node -- a traversal permitting
  multiple edge types could then wander from the config key straight
  into that symbol's own outgoing edges. Namespaced both
  ("config:KEY", "doc:PATH") so they can never collide with a real
  qualified_name.

- reachable()'s bound check was `hop == max_hops`, which a negative
  max_hops never satisfies, silently traversing the entire reachable
  graph instead of respecting the caller's bound. Now rejects negative
  max_hops explicitly and checks `hop >= max_hops` defensively.

- The inverse-deduplication test's callee fixture left called_by empty,
  so an implementation that incorrectly read both calls[] and
  called_by[] as independent sources -- doubling the edge -- would
  still have passed. Populated called_by=("caller",) so the test
  actually exercises the regression it claims to guard.

Added SyntheticNodeNamespacingTest and
ReachableRejectsInvalidBoundsTest (3 new tests) covering the first two
fixes directly, not just by inspection.

Verified: 21 unit tests pass; live re-run against buzz-core confirms
namespaced doc:/config: targets in the CLI output.

Also resolved a recurring RepoQL host lock conflict along the way --
a second stale host process (separately auto-launched by this script's
own `rql` CLI subprocess calls, distinct from the MCP bridge's host)
was holding the DuckDB lock. Killed it and restarted the MCP bridge's
host twice to clear its cached connection reference each time.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
@serina-mcfall

Copy link
Copy Markdown
Author

Codex's independent review confirmed all 3 findings (all P2) — fixed in the follow-up commit:

  • Namespaced synthetic nodes: config keys and doc paths are now config:KEY / doc:PATH, so a symbol whose qualified_name happened to match one (e.g. a symbol literally named the same as an env var it reads) can no longer collide onto the same graph node.
  • Rejected negative max_hops: the old hop == max_hops check silently ignored negative bounds, traversing the whole graph instead of respecting the caller's limit. Now raises ValueError for negative values and checks hop >= max_hops.
  • Fixed the inverse-dedup test: the fixture left callee.called_by empty, so a regression (reading both calls[] and called_by[] as independent sources, doubling the edge) would have passed anyway. Populated it so the test actually exercises what it claims.

Added 3 new tests covering the first two directly. All 21 tests pass; live re-verified against real buzz-core data with the namespaced output.

@serina-mcfall
serina-mcfall marked this pull request as ready for review August 18, 2026 03:19
benmitchell11
benmitchell11 previously approved these changes Aug 18, 2026

@benmitchell11 benmitchell11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewing this PR's own two commits (graph.py, test_graph.py, the plan doc) — confirmed the diff against task/206-project-indexer doesn't re-touch #213's files, so the stacking is legitimate, not accidental scope creep.

Genuinely good catch during self-review: the initially-reversed tested_by/documented_by edge direction is exactly the kind of bug a test that only checks 'edge exists' would never catch — confirmed EdgeDirectionTest actually asserts direction, not just presence. The STEP 5 negative-case output ('a vague description isn't a symbol_id this graph knows about') is a good, honest boundary check against scope creep into #210's job.

Approving the code on its merits. Do not merge as-is — base is task/206-project-indexer, not launchpad; per the PR body this needs retargeting once #213 lands, or merging now would pull #213's commits in through the wrong PR. Also flagging, not deciding: the open question of whether 4-of-8 design-doc edge types is 'done enough' for #207, or needs a follow-up task/scope note the way #193 did for the lefthook exception — that's a real scope call for a maintainer, not something to wave through silently.

@joshuavial joshuavial left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Produced with the pr-review-panel skill: two reviewers on different models (Claude Fable and codex gpt-5.6-sol at xhigh reasoning) reviewed this PR independently with no shared context, their verdicts were consolidated, and the result was reviewed and approved by a human before posting. Claims marked "verified" or "reproduced" were re-checked by hand against the code, not taken from a reviewer's word.

Review panel: two independent reviewers (Fable, codex gpt-5.6-sol @ xhigh)

Each read the diff, the worktree at PR head, #207 and this PR's comments; neither saw the other's output. Scope: this PR's own commits only (graph.py, test_graph.py, the plan doc) — symbol.py/indexer.py from #213 read for context, not reviewed.

python3 -m unittest test_symbol test_indexer test_graph → 21/21 pass, confirmed by both. The live graph.py buzz-core run could not be reproduced (rql not on PATH in this checkout), so the pasted output is taken on trust.

The implementation is sound. The four self-review and Codex-review fixes are real and mutation-verified: reverting the tested_by direction, re-reading called_by[] as an independent source, restoring hop == max_hops, or dropping config: namespacing each fails the suite. BFS handles cycles, self-return, zero hops and nearest-first ordering correctly. Findings below are test-coverage gaps and one scope decision, not defects in shipped behaviour.


Blocking

1. Base branch — do not merge as-is. Base is task/206-project-indexer, not launchpad. Merging now lands these commits into #213's branch and no closing link to #207 ever forms. Already acknowledged in the PR body and by @benmitchell11; retarget once #213 lands. Process, not code.

2. 4-of-8 edge types needs an explicit maintainer decision. #207's Objective names all eight (imports, calls/called_by, configured_by, tested_by, documented_by, deployed_by, owns, depends_on), and the DoD's "edge types are explicit and directional as specified" points back at that list. The PR body's claim that this "satisfies #207's own Definition of done exactly as written" is the weaker reading — the narrower checkbox list doesn't override the Objective it refers to.

Both reviewers reached this independently, and we agree with the technical call: imports, deployed_by, owns and depends_on need extraction #206 does not perform (Rust use statements, deployment units, Cargo.toml deps), so building them here would be a different task. Declaring all nine strings in EdgeType (graph.py:20-30) while only four can ever be materialized is the visible symptom. The fix is a scope decision — narrow #207 with a written note and file the remainder as a follow-up, the way #193 did for the lefthook exception — not more code in this PR. The author flagged this openly rather than deciding silently, which is the right call; it just needs someone to actually make the decision.

High

3. reachable()'s edge_types filter is not pinned by any test — removing it entirely leaves the suite green. graph.py:90-94. Verified directly: deleting the filter from edges_from() so it always returns every edge still passes all 12 tests. Every traversal fixture in test_graph.py has only one outgoing edge type per node, so filtering is never exercised. edge_types is the feature #207's DoD asks for by name ("filtered by edge type"), and it is currently unprotected. Needs a fixture where one node has outgoing calls and tested_by/configured_by edges to different unvisited nodes, asserting the filtered traversal reaches only one of them.

(Found by codex; independently reproduced.)

4. Reachable.path is never read by any test — a truncated path passes the suite. graph.py:129, documented at graph.py:101 as "the node sequence from the start, inclusive of both ends". Verified: mutating new_path = path + (edge.target,) to new_path = (edge.target,) keeps all 12 tests green, while the STEP 4 CLI output would silently print a wrong path. This is precisely the class of bug the PR's own EdgeDirectionTest exists to prevent — right node, wrong provenance, invisible to a test that only checks presence. Fix is cheap: in test_graph.py:143, also assert the full expected tuple for both the 1-hop and 2-hop results.

(Both reviewers flagged this.)

Medium

5. Unresolved external call targets become un-namespaced hub nodes. graph.py:61-67. When #206 cannot resolve a callee (std-lib or other-crate names — contains appears in the PR's own STEP 6 output), the bare short name becomes a graph node, and the inverse edge makes it a hub: every symbol calling anything named contains gets a called_by edge from that single node. A mixed-type traversal such as reachable(g, "A", ("calls", "called_by"), 2) then reports unrelated symbol B as reachable from A purely because both call same-named methods on different types.

The root cause is #206's accepted best-effort resolution, but this PR amplifies it by adding the inverse edges — and the config:/doc: namespacing comments at graph.py:74-83 read as if synthetic-node collisions are now fully handled, when this one is not. Minimum: document the limitation where the inverse edge is built. Better (reasonable follow-up): namespace unresolved targets too, e.g. extern:contains, via a membership check against the input qualified names.

(Fable.)

6. Edge.source's docstring contradicts the actual node identity. graph.py:35 says "symbol_id or a synthetic node id", but nodes are keyed by qualified_name (graph.py:66) — so edges_from(sym.symbol_id) always returns [], and two Symbol records sharing a qualified name silently merge into one node. The class docstring at graph.py:46-47 does state the qualified_name choice, so this is a stale field comment, not a design error.

To be clear about what we are not recommending: codex proposed keying nodes by symbol_id and mapping qualified names through it. We disagree — Symbol.calls[] contains qualified names (indexer.py:168), so symbol_id-keyed nodes would break traversal outright. qualified_name is the correct key here. Fix the comment, and note the same-qualified-name merge as a known limitation. (Whether real rql output actually produces colliding qualified names is unverified — no rql in this checkout.)

(codex, narrowed.)

Low

7. STEP 4's done-when is only half-demonstrated. The plan (launchpad/plans/2026-08-18-issue-207-project-graph.md:43-44) says to print is_shared_gated_kind's own called_by[] alongside the reachable() result, so the "the flat record alone can't show this" contrast is visible. graph.py:166-172 prints only the traversal side. One extra print closes it.

8. EdgeFieldsTest asserts fields on edges[0] only (test_graph.py:29-32), where STEP 1's done-when says "asserts every field". Cosmetic — and the test is near-tautological anyway, constructing dataclasses rather than exercising graph logic.

9. Placeholder-free f-strings at graph.py:158, graph.py:166, graph.py:174 (ruff F541).


What's right

  • Edge direction is coherent across all four types, and EdgeDirectionTest (test_graph.py:65-98) pins direction rather than mere existence — reverting it fails.
  • Deriving called_by as the structural inverse instead of re-reading Symbol.called_by[] is correct: indexer.py:151-156 computes that field from calls[] over the same symbol list, so reading both would double every edge from one fact. The fixture fix in the follow-up commit makes test_graph.py:48 genuinely catch that regression.
  • BFS is right: visited seeded with the start node prevents cycles and self-return; hop >= max_hops and the negative-bound ValueError are both mutation-pinned; edges_from uses .get on the defaultdict (graph.py:91) so lookups don't pollute _by_source.
  • config:/doc: namespacing is real, tested from both directions, and those nodes are correctly sinks.
  • Nothing security-, migration-, or persistence-shaped: a pure in-memory structure over already-indexed data, adding only new isolated files, so no existing caller can break.
  • The negative case (STEP 5) is an honest boundary check against creeping into #210's job, and the plan doc's OPEN escalation is the right way to surface finding 2.

Recommendation

Comment. The shipped behaviour is correct and the suite is real — findings 3 and 4 are missing tests around two guarantees #207 asks for by name, both cheap to add and both in the spirit of the test discipline this PR already demonstrates. Findings 1 and 2 must be settled before merge, but neither is a code change: one is a retarget, the other is a maintainer's scope call.

Dropped

  • codex's "valid called_by data is silently discarded" — its premise is wrong. The code explicitly documents not re-reading that field (graph.py:62-65), and indexer.py:151 derives called_by from calls[] across the same list, so no edge can be lost for any index #206 actually produces. Only a hypothetical future producer supplying materialized inverses would be affected; not actionable today.
  • codex's prescription to key nodes by symbol_id — see finding 6.

@serina-mcfall

Copy link
Copy Markdown
Author

Fix plan for the review panel's findings (not implemented yet)

Verified each finding against this PR's actual code at 2964938f before planning a fix —
mutation-tested findings 3 and 4 (deleting the edge_types filter, and truncating path
accumulation, both leave all 12 tests green), reproduced finding 5 live (two unrelated
symbols sharing an unresolved external call target become falsely reachable from each
other), and read the code/plan doc directly for the rest. All findings hold as stated.

Not a fix — needs a decision first:

Planned code changes, in implementation order

High — pin invariants #207's DoD names by number:

  1. Add a fixture where one node has two outgoing edge types to different unvisited
    targets (e.g. calls to one node, tested_by to another); assert a filtered
    reachable()/edges_from() call reaches only the matching-type target. Closes the gap
    where the edge_types filter can be deleted with no test failing (graph.py:90-94).
  2. Extend test_two_hop_chain_matches_the_verified_real_example (and the 1-hop case) to
    assert .path equals the full expected tuple, not just .hop. Closes the gap where
    path accumulation can be truncated to a single element with no test failing
    (graph.py:129).

Medium:
3. Namespace unresolved external call targets (e.g. extern:contains) by checking
membership against the input symbols' own qualified names, so a bare unresolved name
can't become a collision hub across unrelated symbols (graph.py:61-67) — this is the
bug reproduced above. Add a test with two unrelated callers of the same unresolved
name, asserting neither is reachable from the other via ("calls","called_by").
Alternative if you'd rather keep this PR narrower: document the limitation at
graph.py:61-67 and file a follow-up issue instead of fixing it here — your call.
4. Fix the stale Edge.source docstring at graph.py:35 (says "symbol_id or a synthetic
node id"; nodes are actually keyed by qualified_name), and note the known limitation
that two Symbols sharing a qualified_name would silently merge onto one node.

Low:
5. STEP 4's __main__ block: also print is_shared_gated_kind's own called_by[]
alongside the reachable() result, per the plan's own done-when
(launchpad/plans/2026-08-18-issue-207-project-graph.md:43-44).
6. EdgeFieldsTest.test_constructs_at_least_four_distinct_edge_types should assert
source/target/evidence on every edge in the list, not just edges[0]
(test_graph.py:29-32).
7. Drop the f prefix on the three placeholder-free print strings at graph.py:158,166,174
(ruff F541).

Waiting for the go-ahead before implementing any of this.


🤖 Plan drafted by Claude Code (claude-sonnet-5) for @serina-mcfall — verification methods
noted per item above; not implemented pending her review.

Namespaces unresolved external call targets (extern:NAME) and drops
the inverse called_by edge for them, since a synthetic external node
has no real Symbol behind it -- fanning every caller of the same
unresolved name into each other through one hub node made unrelated
symbols falsely reachable via ("calls", "called_by"), reproduced and
now covered by a regression test.

Also: fixes the stale Edge.source docstring (nodes are keyed by
qualified_name, not symbol_id); prints is_shared_gated_kind's own flat
called_by[] alongside the reachable() result in STEP 4's demo, per the
plan's own done-when; asserts every field on every edge in
EdgeFieldsTest instead of just edges[0]; and drops the f-prefix on
three placeholder-free print strings (ruff F541).

Findings 1 (base branch retarget) and 2 (4-of-8 edge-type scope) are
process/decision items, not code, and are intentionally left for a
human call.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
@serina-mcfall

Copy link
Copy Markdown
Author

Findings 3–7 fixed in ce30f6c

Per go-ahead — findings 1 (base retarget) and 2 (4-of-8 edge-type scope) are
left untouched, as planned; they're decisions, not code.

python3 -m unittest test_symbol test_indexer test_graph → 22/22 pass (was
21; +1 for the new regression test). Live python3 graph.py buzz-core rerun
and output checked by hand.


🤖 Fixes implemented by Claude Code (claude-sonnet-5) for @serina-mcfall, per her go-ahead on findings 3-7 only.

@tucktuck101 tucktuck101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: request changes

The graph construction and traversal are correct at head — BFS handles cycles and self-references (visited seeded with the start node), synthetic config:/doc:/extern: nodes can't collide with real qualified names, extern: nodes are pure sinks so the false-reachability hub is gone, and negative max_hops is rejected. The edge-direction convention is coherent across all four types and genuinely pinned by EdgeDirectionTest. Good work, and the review-response discipline on this PR has been excellent.

But three things need to happen before merge:

1. Two High-priority panel findings were dropped in a numbering slip

The fix-plan comment listed the panel's findings 3 (edge_types filter unpinned) and 4 (Reachable.path never asserted) first, as "High — pin invariants #207's DoD names by number". The follow-up commit ce30f6c then said "findings 3–7 fixed" — but its list maps to the panel's 5–9. The two High items were never implemented. Re-verified against head just now, both mutations still pass all 13 tests:

  • Replacing the filter in edges_from() (graph.py:112-116) with return list(edges) → suite green. "Filtered by edge type" is the DoD's own words and is currently unprotected, because no fixture node has two outgoing edge types.
  • Replacing new_path = path + (edge.target,) (graph.py:151) with new_path = (edge.target,) → suite green. No test reads .path; the STEP 4 demo would print a wrong path with no test failing — exactly the "right node, wrong provenance" bug class EdgeDirectionTest exists to prevent.

Both fixes are the ones already sketched in the fix plan: a fixture with mixed outgoing edge types asserting a filtered traversal reaches only the matching target, and asserting the full expected path tuple in the 1-hop and 2-hop ReachableTest cases.

2. Base branch (known, restating as the merge gate)

Base was task/206-project-indexer. Now that #213 has merged, make sure this retargets to launchpad (GitHub should have retargeted automatically when the head branch was deleted — please verify) so #207 gets its closing link. The existing approval predates the panel review and ce30f6c and itself says "do not merge as-is", so the green APPROVED badge is not clearance.

3. The 4-of-8 edge-type scope call is still open

#207's Objective names eight edge types; EdgeType declares all nine literal strings (graph.py:21-31) while only four can ever be materialized. The author escalated this correctly instead of deciding silently — a maintainer should now either narrow #207 with a written scope note + follow-up issue (per the #193 lefthook precedent) or state that the checkbox DoD governs. Cheap optional cleanup once decided: a comment on EdgeType marking which members are materialized vs. reserved.

Non-blocking observations

  • CI is green but hollow for this change: only path-detection, ADR-boundary, and PR-body checks ran; nothing in CI executes the project-intelligence Python suite. Worth a tiny workflow step so these 22 tests run on PRs touching launchpad/project-intelligence/** — otherwise the mutation gaps above can regress invisibly.
  • qualified_name-keyed nodes merging same-named symbols is documented (graph.py:35-38) and is the right key given calls[] holds qualified names — fine as a known limitation.
  • Performance is appropriate for the stated single-crate scale (O(V+E) build and BFS); the "unverified beyond 453 symbols" caveat in the PR body is honest and acceptable for this task.

Happy to re-review once the two missing tests land and the retarget is confirmed.

@tucktuck101 tucktuck101 reopened this Aug 19, 2026
@tucktuck101
tucktuck101 changed the base branch from task/206-project-indexer to launchpad August 19, 2026 01:54
@tucktuck101
tucktuck101 dismissed benmitchell11’s stale review August 19, 2026 01:54

The base branch was changed.

review-final on PR #214 found that the fix-plan's own numbering slip left
findings 3-4 (from the original review panel) unimplemented, even though a
follow-up commit claimed "findings 3-7 fixed":

- edges_from()'s edge_types filter was correct but unprotected: no fixture
  gave one node two different outgoing edge types, so deleting the filter
  entirely (`return list(edges)`) still passed all 13 tests. Added a mixed
  node and asserted the filtered result excludes the non-matching type.
- Reachable.path was built correctly but nothing read it: truncating
  `new_path = path + (edge.target,)` to just `(edge.target,)` also still
  passed. Asserted the full path tuple in the 1-hop and 2-hop cases.

Both regressions confirmed by mutation-testing before writing the fix (see
PR description) -- the new assertions fail exactly as expected against each
mutation, and pass against the real code.

Also documents the EdgeType scope question review-final raised: the
Objective names 8 edge types, 5 of 9 declared literals are materialized.
Left as reserved literals with an inline comment per type, rather than
trimmed -- #207's own checkbox reads "edge types are explicit and
directional as specified", which the materialized set satisfies without
claiming the other 4 are built. Flagged in the PR/issue for Serina's call
if she'd rather narrow the type instead.

Base branch confirmed already retargeted to `launchpad` (not the deleted
task/206 branch) -- no action needed there.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
@serina-mcfall

Copy link
Copy Markdown
Author

review-final findings fixed — ready for re-review

Fixed the two dropped High findings and documented the scope question, in 5f8b957b9:

1. edges_from()'s edge-type filter, unprotected. Every existing fixture gave a node only one outgoing edge type, so the filter ([e for e in edges if e.edge_type in edge_types]) could be deleted entirely and all 13 tests still passed. Added a mixed-type node and asserted the filtered call excludes the non-matching type. Confirmed by mutation-testing before writing the fix: with the filter removed, the new test fails exactly as expected; with it restored, it passes.

2. Reachable.path, never asserted. Nothing read .path anywhere, so truncating new_path = path + (edge.target,) to just (edge.target,) also passed every test. Asserted the full path tuple in the 1-hop and 2-hop ReachableTest cases. Same mutation-testing confirmation.

3. The 4-vs-8 edge-type scope call. Left EdgeType as all 9 literals (not trimmed), with an inline comment marking which 4 (imports, deployed_by, owns, depends_on) are reserved for the design doc's full graph rather than materialized by this task. #207's own DoD checkbox reads "edge types are explicit and directional as specified" — the materialized set satisfies that without claiming the other 4 are built. This is my recommendation, not a decision — if you'd rather narrow the type and file a follow-up issue instead (the other option review-final offered), that's a one-line change away.

Base branch: confirmed already retargeted to launchpad (not the deleted task/206 branch) via gh pr view --json baseRefName — no action was needed there.

Full test suite: 23/23 pass (python3 -m unittest discover -p "test_*.py" from launchpad/project-intelligence/).

@tucktuck101 — ready for another look when you have a chance.

@tucktuck101
tucktuck101 merged commit 08f81ff into launchpad Aug 20, 2026
21 checks passed
tucktuck101 pushed a commit that referenced this pull request Aug 20, 2026
Codex's independent review of PR #214 confirmed all 3 findings (P2):

- Synthetic config-key and doc-path nodes were bare strings, so a
  symbol whose qualified_name happened to match a config key or doc
  path (e.g. a symbol literally named the same as an env var it reads)
  would collide onto the same graph node -- a traversal permitting
  multiple edge types could then wander from the config key straight
  into that symbol's own outgoing edges. Namespaced both
  ("config:KEY", "doc:PATH") so they can never collide with a real
  qualified_name.

- reachable()'s bound check was `hop == max_hops`, which a negative
  max_hops never satisfies, silently traversing the entire reachable
  graph instead of respecting the caller's bound. Now rejects negative
  max_hops explicitly and checks `hop >= max_hops` defensively.

- The inverse-deduplication test's callee fixture left called_by empty,
  so an implementation that incorrectly read both calls[] and
  called_by[] as independent sources -- doubling the edge -- would
  still have passed. Populated called_by=("caller",) so the test
  actually exercises the regression it claims to guard.

Added SyntheticNodeNamespacingTest and
ReachableRejectsInvalidBoundsTest (3 new tests) covering the first two
fixes directly, not just by inspection.

Verified: 21 unit tests pass; live re-run against buzz-core confirms
namespaced doc:/config: targets in the CLI output.

Also resolved a recurring RepoQL host lock conflict along the way --
a second stale host process (separately auto-launched by this script's
own `rql` CLI subprocess calls, distinct from the MCP bridge's host)
was holding the DuckDB lock. Killed it and restarted the MCP bridge's
host twice to clear its cached connection reference each time.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
serina-mcfall added a commit that referenced this pull request Aug 20, 2026
…254)

* docs(launchpad): plan issue #210 -- SemanticIndex and concept-retrieval pipeline

Plans the fifth task under PRD #4's Project Intelligence Layer scope:
a 10-step plan implementing ConceptEntry and a two-stage concept ->
subsystem -> candidate symbol -> confirmed-reference pipeline, per the
design doc's Data Model item 3 and Concept Retrieval reasoning rules.

Confirmed before planning: #210 has a REAL stated dependency on #206
(unlike #208/#209) -- "Depends on #206 for the content to embed and
summarize" -- so this branches from origin/task/207-project-graph
(which carries both #206's and #207's already-merged-locally commits)
rather than from `launchpad` directly, since #213/#214 haven't merged
into launchpad yet.

Key design decision recorded in the plan: "subsystem" is implemented
as a real second ConceptEntry level scoped to file (the schema's own
stated scope kinds are symbol_id | file | doc_section), not collapsed
into a single flat symbol ranking -- so the pipeline's stated shape is
actually built as two literal ranking stages, not simplified away.

Mechanical checks clean via check-plan.sh.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 1 -- ConceptEntry and store skeleton

Adds ConceptEntry, matching the design doc's schema (scope, embedding,
summary), and SemanticIndex, an in-process store keyed by scope.

embedding is a tuple of (token, weight) pairs, not a dict, so the
frozen dataclass stays genuinely immutable -- same reasoning as #209's
MemoryEntry.evidence being a tuple rather than a mutable list.

scope accepts symbol_id, file, or doc_section per the design doc's own
schema -- this is deliberate groundwork for STEP 4/5's two-level
pipeline (per-symbol and per-file "subsystem" entries), not unused
generality.

Verified: `python3 -m unittest test_semantic_index` -- 3 passed.
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 2 -- summarize_symbol

Adds summarize_symbol(): a deterministic natural-language gloss built
only from #206's already-extracted Symbol structural facts
(qualified_name, kind, signature, calls, tests, config_dependencies,
documentation_links) -- generated once, not guessed fresh per query,
matching the design doc's own stated constraint. Empty fields are
omitted rather than printed blank.

Test fixture is a real Symbol from buzz-core, hand-constructed (not
built via indexer.build_index(), which shells out to rql and is kept
out of this committed hermetic suite, same reasoning as
test_indexer.py/test_graph.py) from fields cross-checked directly
against crates/buzz-core/src/kind.rs:219-221 and confirmed against
ARCHITECTURE.md:142 (which references kind.rs by file path -- the
documentation_links match is on file mention, not literal function
name, matching #206's with_documentation_links() logic).

Verified: `python3 -m unittest test_semantic_index` -- 6 passed.
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 3 -- tokenize, embed_text, cosine_similarity

Adds tokenize() (word-boundary plus camelCase/snake_case splitting, so
identifiers decompose into the same word tokens a natural-language
concept query would use), embed_text() (a bag-of-words frequency
vector -- a deliberate, documented lightweight stand-in for a trained
ML embedding model, matching #210's own "out of scope: any embedding-
model selection process beyond what's needed to demonstrate the
pipeline once"), and cosine_similarity() between two such vectors,
guarding the zero-vector case rather than dividing by zero.

Verified: `python3 -m unittest test_semantic_index` -- 13 passed,
including hand-computed (not real-symbol) cases per the plan's STEP 3
done-when: a vector against itself is 1.0, disjoint vocabularies are
0.0, and one fully hand-worked partial-overlap case (a a b / a c c ->
2/5) checked by hand in the test's own comment before being asserted.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 4 -- from_symbols, two ConceptEntry levels (RUNS HERE)

Adds SemanticIndex.from_symbols(), building two levels of ConceptEntry
from real Symbol records: one per symbol, and one per file (aggregating
that file's symbols' summaries) -- the design doc's own ConceptEntry
schema names file as a valid scope kind alongside symbol_id, so this
is the schema's own coarser "subsystem" level, not invented machinery.

Self-caught bug from live verification (not from a test I wrote in
advance): the first version keyed per-symbol entries by qualified_name,
which raised ValueError on real buzz-core data -- multiple distinct
symbols (e.g. several "build_event" functions in different modules)
share one qualified_name, so it is not a safe unique key. Switched to
symbol_id (a real, per-symbol-unique RepoQL URI from #206's
index_crate()), which the design doc also explicitly names as a valid
scope kind. Added qualified_name_for(), since #207's ProjectGraph
addresses nodes by qualified_name, not symbol_id -- the pipeline's
later confirmation step needs to translate between the two.

Verified: `python3 -m unittest test_semantic_index` -- 17 passed,
including a synthetic two-symbols-sharing-one-qualified_name
regression test for the exact collision this fix addresses.

Verified live against ALL 453 real buzz-core symbols (not just the
hand-picked fixtures): building the full index raises nothing, and
crates/buzz-core/src/kind.rs's file-level entry aggregates both
is_shared_gated_kind and is_unshared_gated_event's real content.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 5 -- two-stage search, embed_symbol identity weighting

Adds SemanticIndex.search(): rank file-level "subsystem" entries
first, then rank symbol-level entries scoped to the top file(s) --
concept -> candidate subsystem(s) -> candidate symbols, as two literal
ranking stages, returned as SearchResult(subsystem, subsystem_score,
candidate, candidate_score) sorted by candidate_score.

Adds embed_symbol(), weighting a symbol's own identity (kind +
qualified_name + signature) 2x over context mentions (calls/tests/
config/docs) in its embedding. Without this, a caller's summary
absorbs its callees' name tokens too (since "calls X" contributes X's
own identifier tokens), so a caller can outrank the callee it calls
for a query about the callee's own behavior -- found empirically
verifying this step's own worked example: is_unshared_gated_event
(which calls is_shared_gated_kind) initially outranked
is_shared_gated_kind itself once the query touched a token unique to
the caller's own name ("event"). identity_weight=2.0 was checked
empirically against the real worked example, not derived
analytically, and is documented as such in embed_symbol()'s docstring.
Retrofitted STEP 4's from_symbols() to use embed_symbol() instead of a
plain embed_text(summary) for both the per-symbol and per-file
embeddings.

Verified: `python3 -m unittest test_semantic_index` -- 19 passed,
including a hand-checked identity-weighting assertion (event's weight
> kind's absorbed-context weight for is_unshared_gated_event) and the
corrected two-stage search test (kind.rs wins as subsystem,
is_shared_gated_kind wins as candidate, over an unrelated real symbol
from crates/buzz-core/src/invite.rs).

Verified live against the full real buzz-core index (453 symbols, via
indexer.build_index): searching "which function decides if a kind is
gated for shared visibility" ranks is_shared_gated_kind first (0.5706)
by a clear margin over the next real result
(tests::shared_gated_kinds_membership, 0.4300), with kind.rs correctly
winning as the top subsystem.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 6 -- confirm_via_graph

Adds confirm_via_graph(): the pipeline's final confirmation step,
calling directly into #207's ProjectGraph.edges_from() for
tested_by/called_by edges on a candidate symbol -- real structural
confirmation, not semantic similarity alone. Returns empty tuples
(never hidden) when nothing confirms a candidate, so a caller can see
an unconfirmed guess for what it is.

Verified: `python3 -m unittest test_semantic_index` -- 21 passed.
Cross-checked confirm_via_graph(graph, "is_shared_gated_kind") against
#207's own already-proven demo output for the identical symbol
(graph.py's __main__, STEP 6): tested_by ->
tests::shared_gated_kinds_membership, called_by -> is_unshared_gated_event
-- matches exactly.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 7 -- find_it_for_me full pipeline

Adds find_it_for_me(): concept -> subsystem -> candidate -> confirm,
tied into one call. Translates the top candidate's scope (symbol_id)
back to its qualified_name via SemanticIndex.qualified_name_for()
before confirming through #207's ProjectGraph, since the two
components address symbols differently. Returns an empty result
(candidate/confirmation None) rather than crashing when the index has
nothing to rank.

Verified: `python3 -m unittest test_semantic_index` -- 23 passed. One
test builds both a SemanticIndex and a ProjectGraph from the same
three real symbols and confirms find_it_for_me() ties all of it
together correctly in one call: is_shared_gated_kind as the resolved
qualified_name, kind.rs as the subsystem, and real
callers/tests matching STEP 6's own already-verified edges.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 8 -- positive worked example end to end

Adds WORKED_EXAMPLE_CONCEPT, a named module-level constant for STEP
8's own worked concept-search example against this repo's real code
(not the design doc's fictional OnboardingMailer example): "which
function decides if a kind is gated for shared visibility" --
resolves to is_shared_gated_kind via subsystem -> candidate ->
confirmed-reference.

Verified: `python3 -m unittest test_semantic_index` -- 25 passed,
including an explicit check that the concept sentence contains no
contiguous substring match of "is_shared_gated_kind" (nor its
underscores-as-spaces form) -- proving this is genuine token/concept
overlap, not an accidental literal substring hit, per #210's own
Definition of done.

Verified live against the FULL real buzz-core index (453 symbols, via
indexer.build_index + graph.ProjectGraph.from_symbols): find_it_for_me
resolves the exact same concept sentence to is_shared_gated_kind
(candidate_score 0.5706, subsystem kind.rs at 0.4184), confirmed by 2
real called_by edges (tests::shared_gated_kinds_membership,
is_unshared_gated_event) and 1 real tested_by edge.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* test(launchpad): SemanticIndex STEP 9 -- negative flow-tracing boundary case

Demonstrates the documented boundary from #210's own side (#207's
graph.py already showed the mirror: a vague description has no
symbol_id for reachable() to start from). Poses the SAME 2-hop
relationship #207's own reachable() demo already proves
(tests::is_unshared_gated_event_author_always_allowed ->
is_unshared_gated_event -> is_shared_gated_kind, all real symbols
cross-checked against crates/buzz-core/src/kind.rs:997-1007) through
THIS pipeline instead.

No new production code -- STEPS 1-7 already implement everything this
exercises; this step is the issue's own required negative
demonstration, not new functionality.

Verified: `python3 -m unittest test_semantic_index` -- 27 passed.
Confirms structurally (PipelineResult/Confirmation have no hop or path
field at all -- checked via dataclasses.fields(), not by reading the
source) and behaviorally (confirm_via_graph() only ever returns direct
edges; is_shared_gated_kind never appears in a one-hop confirmation
for a symbol two hops away) that this pipeline cannot express or
verify a multi-hop path, while reachable() answers the identical
relationship exactly.

Verified live against the FULL real buzz-core index (453 symbols): the
same flow-tracing question resolves this pipeline to an unrelated weak
match (tests::test_unspecified) with an EMPTY confirmation -- it
cannot even find a sensible candidate for a flow-tracing question, let
alone a verified 2-hop path -- while reachable() returns the exact
real path (tests::is_unshared_gated_event_author_always_allowed ->
is_unshared_gated_event -> is_shared_gated_kind).

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

* feat(launchpad): SemanticIndex STEP 10 -- final CLI, both worked examples

Wires __main__ to run both worked examples end to end against the
real buzz-core index, matching #206/#207/#208/#209's demo style: the
positive concept -> subsystem -> candidate -> confirmation trace, and
the negative flow-tracing contrast (this pipeline's result vs
ProjectGraph.reachable()'s exact answer), side by side.

Named the negative example's constants (NEGATIVE_EXAMPLE_FLOW_QUESTION,
NEGATIVE_EXAMPLE_START_SYMBOL) alongside STEP 8's WORKED_EXAMPLE_CONCEPT,
and updated STEP 9's test to reference them instead of duplicating the
literal strings.

Verified live: `python3 semantic_index.py` prints both traces with
real results -- is_shared_gated_kind resolved and confirmed for the
positive example, and tests::test_unspecified (an unrelated weak
match, empty confirmation) for the negative example, contrasted
against reachable()'s real verified 2-hop path.

Verified: `python3 -m unittest test_semantic_index` -- 27 passed.

This completes all 10 steps of #210's plan.

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

---------

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

task: build a ProjectGraph with typed edges and traversal over ProjectIndexer output

4 participants