fix: match lowercase relationship types returned by API#17
Closed
jonathanpopham wants to merge 2 commits intosupermodeltools:mainfrom
Closed
fix: match lowercase relationship types returned by API#17jonathanpopham wants to merge 2 commits intosupermodeltools:mainfrom
jonathanpopham wants to merge 2 commits intosupermodeltools:mainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "supermodel restore\n(cmd/restore.go)"
participant Restore as "restore.Run/Render\n(internal/restore)"
participant Local as "BuildProjectGraph\n(internal/restore/local.go)"
participant API as "AnalyzeDomains\n(internal/api/client.go)"
participant Supermodel as "Supermodel API\n(/v1/graphs/supermodel)"
participant Stdout as "stdout"
User->>CLI: run `supermodel restore [--local]`
CLI->>CLI: findGitRoot, build opts
alt use API (no --local && API key)
CLI->>API: restoreViaAPI (zip + AnalyzeDomains)
API->>Supermodel: upload zip, poll job
Supermodel-->>API: SupermodelIR result
API-->>CLI: SupermodelIR
CLI->>Restore: FromSupermodelIR(ir)
else local
CLI->>Local: BuildProjectGraph(root)
Local-->>CLI: ProjectGraph
CLI->>Restore: Render(graph, opts)
end
Restore-->>Stdout: rendered Markdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Ports the "Uncompact" context-bomb functionality from github.com/supermodeltools/Uncompact into the supermodel CLI as `supermodel restore`. The command generates a high-level project summary (domains, critical files, tech stack, stats) and writes it to stdout so Claude can re-establish codebase understanding after a context compaction event. Two modes: - API mode (default when authenticated): calls /v1/graphs/supermodel via the new AnalyzeDomains() method which returns the full SupermodelIR — semantic domains, responsibilities, subdomains, and dependency relationships — then converts to a ProjectGraph. - Local mode (--local or no API key): scans the repo file tree, groups files by directory, detects language and external deps from manifest files (go.mod, package.json, requirements.txt, Cargo.toml, Gemfile, pyproject.toml), no network calls required. New packages: - internal/restore: ProjectGraph model, FromSupermodelIR() conversion, local graph builder, Markdown renderer with configurable token budget (default 2000) and progressive truncation strategy. New API types (internal/api/types.go): - SupermodelIR and IR* raw response types for /v1/graphs/supermodel. New API method (internal/api/client.go): - AnalyzeDomains() — shares polling infrastructure with Analyze() via the extracted pollUntilComplete() helper. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The API returns relationship types in lowercase (imports, calls, defines_function) but all handlers compared against uppercase (IMPORTS, CALLS, DEFINES_FUNCTION). This caused blast-radius to return 0 affected files, dead-code to report all functions as dead, and focus to produce empty context. Fixed in each slice independently: - blastradius: imports, wildcard_imports - deadcode: calls, contains_call - focus: defines_function, defines, calls, contains_call - mcp: calls, contains_call, imports, wildcard_imports - check-architecture: imports, wildcard_imports Unit test fixtures updated to match.
jonathanpopham
force-pushed
the
fix/lowercase-relationship-types
branch
from
ad0cbb3 to
082df5e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/deadcode/handler_test.go (1)
49-49: Worth coveringcontains_callhere too.The handler changed both lowercase call-edge variants, but this fixture only locks down
calls. A second case usingcontains_callwould cover the other branch of the fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/deadcode/handler_test.go` at line 49, Add a second test case in internal/deadcode/handler_test.go that mirrors the existing call-edge fixture but uses Type "contains_call" so the branch handling lowercase call-edge variants is exercised; specifically add a record similar to the existing {ID:"r1", Type:"calls", StartNode:"f2", EndNode:"f1"} but with Type:"contains_call" (and a unique ID like "r2") and update any expected results/assertions in the same test function to account for this additional edge so both "calls" and "contains_call" branches are covered.internal/blastradius/handler_test.go (1)
40-41: Add awildcard_importsregression here too.Right now this fixture only proves the
importsbranch. SincefindBlastRadius()changed both relationship variants, one extra subtest usingwildcard_importswould keep the other half of the fix from quietly drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/blastradius/handler_test.go` around lines 40 - 41, Add a second subtest that exercises the wildcard_imports relationship variant so the other branch in findBlastRadius() is covered; copy the existing imports fixture in the test (the table containing {ID: "r1", Type: "imports", ...} and {ID: "r2", ...}) and add a parallel case where the relationships use Type: "wildcard_imports" (or add an additional relationship entry with Type "wildcard_imports") then assert the same expected output as the imports case; ensure the new subtest uses the same test helper and calls findBlastRadius() so both "imports" and "wildcard_imports" branches remain covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/focus/handler.go`:
- Around line 228-229: The code only updated some edge-name checks to lowercase
but left other filters using uppercase constants, so CalledBy and Imports remain
empty; update all comparisons of rel.Type (including the checks that currently
match "CALLS"/"CONTAINS_CALL" and "IMPORTS"/"WILDCARD_IMPORTS") to use the
lowercase names ("calls", "contains_call", "imports", "wildcard_imports") or
normalize rel.Type with strings.ToLower/strings.EqualFold once before the
switch/filtering. Locate the checks around the existing rel.Type usage (the
map-building block that appends rel.EndNode for rel.Type == "calls" /
"contains_call" and the other scan blocks that currently test
"CALLS"/"CONTAINS_CALL" and "IMPORTS"/"WILDCARD_IMPORTS") and make them
consistently compare against the lowercase edge names so CalledBy and Imports
are populated correctly.
- Around line 209-210: The fallback currently appends any function node that has
a file/path value, producing repo-wide matches; modify the fallback so it only
adds functions that belong to the target file: when no defines_* edge matched
for fileID, filter candidate function nodes by their association to fileID
(e.g., check the relation StartNode == fileID or compare the function node's
file/path value to the target fileID/file path) before appending to ids,
ensuring ids only contains functions for that specific file.
In `@internal/mcp/server.go`:
- Around line 342-343: get_graph advertises uppercase relationship names but
code (checks on rel.Type and filterGraph) uses lowercase, causing mismatches;
fix by normalizing relationship names consistently — e.g., in filterGraph() and
any places reading rel.Type (the rel.Type checks in get_graph and the
called/contains logic) call strings.ToLower on incoming rel_type values before
matching, and also update the get_graph schema text that lists rel_type values
to the lowercase forms (or vice-versa if you prefer uppercase) so the advertised
contract matches the runtime checks.
---
Nitpick comments:
In `@internal/blastradius/handler_test.go`:
- Around line 40-41: Add a second subtest that exercises the wildcard_imports
relationship variant so the other branch in findBlastRadius() is covered; copy
the existing imports fixture in the test (the table containing {ID: "r1", Type:
"imports", ...} and {ID: "r2", ...}) and add a parallel case where the
relationships use Type: "wildcard_imports" (or add an additional relationship
entry with Type "wildcard_imports") then assert the same expected output as the
imports case; ensure the new subtest uses the same test helper and calls
findBlastRadius() so both "imports" and "wildcard_imports" branches remain
covered.
In `@internal/deadcode/handler_test.go`:
- Line 49: Add a second test case in internal/deadcode/handler_test.go that
mirrors the existing call-edge fixture but uses Type "contains_call" so the
branch handling lowercase call-edge variants is exercised; specifically add a
record similar to the existing {ID:"r1", Type:"calls", StartNode:"f2",
EndNode:"f1"} but with Type:"contains_call" (and a unique ID like "r2") and
update any expected results/assertions in the same test function to account for
this additional edge so both "calls" and "contains_call" branches are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fae57ff0-5033-41b3-af7f-b19b83673c62
📒 Files selected for processing (7)
internal/blastradius/handler.gointernal/blastradius/handler_test.gointernal/deadcode/handler.gointernal/deadcode/handler_test.gointernal/focus/handler.gointernal/mcp/server.goscripts/check-architecture/main.go
Comment on lines
+209
to
210
| if (rel.Type == "defines_function" || rel.Type == "defines") && rel.StartNode == fileID { | ||
| ids = append(ids, rel.EndNode) |
There was a problem hiding this comment.
Fallback matches every function, not just this file's functions.
If no defines_* edge exists, the fallback on Line 216 only checks that a function has any file/path value, so it appends basically every function node in the repo instead of only the ones belonging to fileID. That turns a missing-edge case into repo-wide noise in focus.
💡 Tighten the fallback to the target file
func functionNodesForFile(g *api.Graph, fileID string, rels []api.Relationship) []string {
var ids []string
for _, rel := range rels {
if (rel.Type == "defines_function" || rel.Type == "defines") && rel.StartNode == fileID {
ids = append(ids, rel.EndNode)
}
}
// Fallback: match by file property on Function nodes.
if len(ids) == 0 {
+ fileNode, ok := g.NodeByID(fileID)
+ if !ok {
+ return ids
+ }
+ targetPath := fileNode.Prop("path", "name", "file")
for _, n := range g.NodesByLabel("Function") {
- if n.Prop("file", "path") != "" {
+ if pathMatches(n.Prop("file", "path"), targetPath) {
ids = append(ids, n.ID)
}
}
}
return ids
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/focus/handler.go` around lines 209 - 210, The fallback currently
appends any function node that has a file/path value, producing repo-wide
matches; modify the fallback so it only adds functions that belong to the target
file: when no defines_* edge matched for fileID, filter candidate function nodes
by their association to fileID (e.g., check the relation StartNode == fileID or
compare the function node's file/path value to the target fileID/file path)
before appending to ids, ensuring ids only contains functions for that specific
file.
Comment on lines
+228
to
229
| if rel.Type == "calls" || rel.Type == "contains_call" { | ||
| m[rel.StartNode] = append(m[rel.StartNode], rel.EndNode) |
There was a problem hiding this comment.
The lowercase migration is still incomplete in focus.
These helpers now read lowercase edge names, but Line 134 still filters CALLS/CONTAINS_CALL and Line 182 still filters IMPORTS/WILDCARD_IMPORTS. On the lowercase graph, CalledBy and Imports stay empty, so the command is only half-fixed.
💡 Finish the remaining scans
- if rel.Type != "CALLS" && rel.Type != "CONTAINS_CALL" {
+ if rel.Type != "calls" && rel.Type != "contains_call" {
continue
}- if rel.Type != "IMPORTS" && rel.Type != "WILDCARD_IMPORTS" {
+ if rel.Type != "imports" && rel.Type != "wildcard_imports" {
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/focus/handler.go` around lines 228 - 229, The code only updated some
edge-name checks to lowercase but left other filters using uppercase constants,
so CalledBy and Imports remain empty; update all comparisons of rel.Type
(including the checks that currently match "CALLS"/"CONTAINS_CALL" and
"IMPORTS"/"WILDCARD_IMPORTS") to use the lowercase names ("calls",
"contains_call", "imports", "wildcard_imports") or normalize rel.Type with
strings.ToLower/strings.EqualFold once before the switch/filtering. Locate the
checks around the existing rel.Type usage (the map-building block that appends
rel.EndNode for rel.Type == "calls" / "contains_call" and the other scan blocks
that currently test "CALLS"/"CONTAINS_CALL" and "IMPORTS"/"WILDCARD_IMPORTS")
and make them consistently compare against the lowercase edge names so CalledBy
and Imports are populated correctly.
Comment on lines
+342
to
343
| if rel.Type == "calls" || rel.Type == "contains_call" { | ||
| called[rel.EndNode] = true |
There was a problem hiding this comment.
get_graph still advertises the old uppercase relationship names.
This file now treats relationship types as lowercase, but the tool schema on Line 113 still tells clients to send values like IMPORTS/CALLS, and filterGraph() on Line 422 does exact matching. MCP clients following the advertised contract will still get empty relationship slices unless they guess the lowercase form. Either normalize rel_type before filtering or update the schema text.
Also applies to: 369-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/server.go` around lines 342 - 343, get_graph advertises
uppercase relationship names but code (checks on rel.Type and filterGraph) uses
lowercase, causing mismatches; fix by normalizing relationship names
consistently — e.g., in filterGraph() and any places reading rel.Type (the
rel.Type checks in get_graph and the called/contains logic) call strings.ToLower
on incoming rel_type values before matching, and also update the get_graph
schema text that lists rel_type values to the lowercase forms (or vice-versa if
you prefer uppercase) so the advertised contract matches the runtime checks.
Contributor
Author
Live test results (before → after)dead-code: blast-radius on AuthConstants.java: focus on codegraph.service.ts: Unit tests: all pass. Integration test against live API: pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The API returns relationship types in lowercase (
imports,calls,defines_function) but every handler compared against uppercase (IMPORTS,CALLS,DEFINES_FUNCTION). This broke all graph-dependent commands:dead-codeblast-radiusfocusChanges
Fixed in each slice independently (no shared helpers):
internal/blastradius/handler.go—imports,wildcard_importsinternal/deadcode/handler.go—calls,contains_callinternal/focus/handler.go—defines_function,defines,calls,contains_callinternal/mcp/server.go—calls,contains_call,imports,wildcard_importsscripts/check-architecture/main.go—imports,wildcard_importsTest plan
go test ./...— all passsupermodel blast-radius AuthConstants.java→ 25 affected files across 3 hopssupermodel dead-code→ 717 (down from 1,337)supermodel focus codegraph.service.ts→ shows function definitionsSummary by CodeRabbit
New Features
Bug Fixes
Documentation