Add cross-language qualified reference diagnostics - #240
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds cross-language qualified reference analysis, committed-reference scanning, import-shadow diagnostics, confidence reporting, and MCP and CLI interfaces. ChangesReference diagnostics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant StructuralReview
participant WARPGraph
participant CommittedReferenceScan
participant QualifiedReferenceResolver
participant GitRef
StructuralReview->>WARPGraph: Query reference edges
StructuralReview->>CommittedReferenceScan: Scan qualified references at ref
CommittedReferenceScan->>GitRef: Read committed files
GitRef-->>CommittedReferenceScan: Return source contents
CommittedReferenceScan->>QualifiedReferenceResolver: Analyze bindings and accesses
QualifiedReferenceResolver-->>CommittedReferenceScan: Return references, warnings, confidence
CommittedReferenceScan-->>StructuralReview: Return scan result
StructuralReview-->>StructuralReview: Propagate warnings and confidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51976aec0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
src/warp/go-reference-context.ts (1)
65-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive imported package directories from the parsed import block, not a whole-file string regex.
Lines 65-72 scan the entire file source for any double-quoted substring. A struct tag, a string constant, or a comment containing the module path is treated as an import. The result is extra entries in
packageDirectories, which widens the candidate set at lines 73-79 and causes extra file reads and parses.The result is not wrong, because
goBindingsinsrc/warp/qualified-reference-resolver.tsre-derives the directory from realimport_specnodes at lines 265-274. The cost is wasted work and a fragile heuristic.Parse
importingSourceonce and read theimport_declarationnodes, or accept the tree from the caller, which already parses the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/warp/go-reference-context.ts` around lines 65 - 72, Replace the whole-file importPattern scan in the relevant go-reference-context flow with import_declaration nodes from the parsed source (or reuse the caller’s existing syntax tree). Derive packageDirectories only from actual import specs, preserving the modulePath matching behavior while excluding strings, struct tags, and comments.test/unit/warp/qualified-reference-resolver.test.ts (1)
172-196: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a Go case with a grouped
varorconstdeclaration.Every Go fixture in this file declares exported names with
func.declarationNamesinsrc/warp/go-reference-context.tshandlesfuncthrough a dedicated branch, and handlesvar,const, andtypethrough a separate branch that reads only the firstnamefield per spec.A fixture such as
package sources\nvar Alpha, Beta intwould exercise that second branch and would show whetherBetaresolves. The gap is the reason the multi-name defect flagged onsrc/warp/go-reference-context.tslines 34-40 is not caught here.Do you want me to write this test case?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/warp/qualified-reference-resolver.test.ts` around lines 172 - 196, Add a Go fixture case covering a grouped declaration such as “var Alpha, Beta int” in the existing qualified-reference resolver test, and reference both exported names from the source so resolution of the non-first name is exercised. Update the expected accesses to verify Beta resolves correctly, preserving the existing checks for shadowing and duplicate declarations.src/contracts/output-schema-mcp.ts (1)
89-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe import-binding diagnostic object is defined four times. The shared root cause is that no named schema exists for this shape, so each surface inlines its own copy. A new
languagevalue, a new field, or a change toshadowKindmust be applied in four places, and any missed copy causes a strict-validation failure at runtime instead of a type error at build time.
src/contracts/output-schema-mcp.ts#L89-L95: defineimportBindingDiagnosticSchemaonce in this file, then setdiagnostics: z.array(importBindingDiagnosticSchema).src/contracts/output-schemas.ts#L966-L966: replace the inline object with a reference tomcpOutputBodySchemas.graft_import_diagnostics, matching how lines 1469 and 1549 already reuse it.src/contracts/output-schemas.ts#L1359-L1365: replace the inlinereferenceWarningselement object with the shared diagnostic schema.src/contracts/output-schema-mcp.ts#L484-L490: replace the inlinereferenceWarningselement object with the shared diagnostic schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/contracts/output-schema-mcp.ts` around lines 89 - 95, Define a shared importBindingDiagnosticSchema in src/contracts/output-schema-mcp.ts at lines 89-95 and use it for graft_import_diagnostics.diagnostics; replace the inline graft diagnostic at src/contracts/output-schemas.ts:966-966 with mcpOutputBodySchemas.graft_import_diagnostics, and replace the inline referenceWarnings element schemas at src/contracts/output-schemas.ts:1359-1365 and src/contracts/output-schema-mcp.ts:484-490 with the shared diagnostic schema.src/warp/qualified-reference-resolver.ts (1)
336-480: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
collectShadowRegionsinto per-language strategies.The function spans 145 lines. A single
walkcallback carries the rules for five languages, and the per-language node-type lists are rebuilt on every visited node. Lines 398-405, 409-413, 418-422, and 461-465 each allocate a freshSetor array for every node in the tree. For a large file that is one allocation set per AST node.The structure also hides the behavioral differences. The Python branch returns early at line 395, so the shared parameter and local rules below never apply to Python, which is correct but not obvious from the layout.
Extract one shadow-rule module per language behind a small interface, and hoist the node-type sets to module constants. That removes the per-node allocations and makes each language's rules reviewable on its own.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/warp/qualified-reference-resolver.ts` around lines 336 - 480, Refactor collectShadowRegions into separate per-language strategy helpers/modules behind a small shared interface, while preserving all existing shadow-region behavior. Move Python, Rust, Go, and TypeScript/JavaScript rules out of the single walk callback, hoist every language-specific node-type Set/array (including functionTypes, blockTypes, parameterTypes, localTypes, and ancestor-type sets) to reusable module-level constants, and have collectShadowRegions select the appropriate strategy without allocating these collections per AST node.src/warp/python-import-resolver.ts (1)
5-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a single AST anchor helper across the resolvers.
astNodeId/emitAstAnchorare duplicated insrc/warp/python-import-resolver.ts,src/warp/ast-import-resolver.ts, andsrc/warp/qualified-reference-resolver.ts. Move them to the existingsrc/warp/ast-emitter.tsor a focused shared module, and import the helper from all three resolvers. Also use a non-cryptographic hash implementation instead ofcreateHash("sha1")for this ID namespace if the static-analysis CWE-327/328 finding should be addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/warp/python-import-resolver.ts` around lines 5 - 34, Centralize astNodeId and emitAstAnchor in the existing ast-emitter module or a focused shared module, then remove the duplicate implementations from python-import-resolver.ts, ast-import-resolver.ts, and qualified-reference-resolver.ts and import the shared helpers. Replace the SHA-1 createHash usage with a non-cryptographic hash suitable for generating these IDs, preserving the existing ID format and anchor properties.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/tools/structural-review.ts`:
- Around line 9-24: Update countReviewReferences so scanQualifiedReferencesAtRef
is not invoked when graph.referenceCount already provides a positive result,
unless scan warnings or confidence are explicitly required; alternatively,
create and reuse one ref-analysis context across all countReferences calls
within detectBreakingChanges. Preserve the existing graph and scan result fields
while eliminating repeated full-repository scans per symbol.
- Around line 9-24: Update countReviewReferences to wrap the
scanQualifiedReferencesAtRef call in try/catch; if the committed-reference scan
fails, return the usable graph result instead of propagating the exception and
failing the structural review. Preserve the existing scan warnings, confidence,
and scan result behavior when the call succeeds.
In `@src/warp/committed-reference-scan.ts`:
- Around line 138-197: Update scanQualifiedReferencesAtRef and its supporting
analysis so direct imported identifier uses in Python, Rust, and Go are
detected, not only qualified accesses. Resolve direct calls such as imported
function or symbol references against opts.filePath and opts.symbolName, add
their files to referencingFiles, and ensure unresolved direct-import evidence
cannot produce a complete zero-reference result; classify it as partial or
resolve it fully.
In `@src/warp/go-reference-context.ts`:
- Around line 83-102: Cache the result of buildGoReferenceContext in
indexHeadFile and reuse that single context throughout the indexing pass,
including commitDiagnosticsAtRef, scanQualifiedReferencesAtRef, and
importDiagnosticsAtRef. Ensure the context is built once per manifest/indexing
operation rather than re-reading and re-parsing the same first-party Go files
for each head.
- Around line 34-40: Update the declaration-name collection loop in the
reference-context builder to retrieve every name child from each spec, adding
all returned names to names before applying the existing exported-name filter;
do not rely on childForFieldName("name"), which only captures the first name in
grouped declarations.
In `@src/warp/index-head.ts`:
- Around line 173-181: Route the initial HEAD content fetch in the file-indexing
flow through readHeadFile instead of a direct git.run call, preserving the
resulting content in the headContent cache for buildGoReferenceContext to reuse.
Update prepareFileSemanticEnrichment and estimatePatchPayloadBytes to use the
returned content value rather than contentResult.stdout, while leaving the
existing Go reference-context behavior unchanged.
In `@src/warp/qualified-reference-resolver.ts`:
- Around line 423-437: Update the loop detection in the local-binding handling
around nearestAncestor so it only identifies a loop when the declaration node
belongs to that loop’s initializer or range clause, not merely its body or
another nested location. Keep body-local declarations using their enclosing
block scope and local_binding classification, while preserving loop_binding and
loop scope behavior for genuine loop-header declarations.
- Around line 344-348: The targetByBinding construction in the
qualified-reference resolution flow assigns an arbitrary Go declaration file to
diagnostics. For Go bindings, use the package directory as the diagnostic
target, or rebuild the published diagnostics from the per-access targets
resolved in the accesses flow around the existing per-member override. Ensure
the diagnostics returned by the resolver no longer retain the first
declaration-map file while preserving accurate targets for non-Go bindings.
- Around line 116-121: Update the candidates array in resolveRelativeModule to
include extensionless `.mts` and `.cts` file candidates, plus `.jsx`, `.mts`,
and `.cts` index candidates alongside the existing extensions. Preserve the
current raw and compiledSpecifierSourceCandidates entries and candidate
ordering.
In `@test/unit/warp/python-import-resolver.test.ts`:
- Around line 150-160: Replace the JSON.stringify assertion in the “preserves
the TypeScript resolver edge vocabulary byte-for-byte” test with structural
toEqual assertions for result.edges and result.metadata, following the patterns
used by other tests in the file. Preserve validation of the expected edge and
metadata contents while avoiding dependence on serialization order or hardcoded
AST anchor hashes.
In
`@tests/playback/0078-three-surface-capability-baseline-and-parity-matrix.test.ts`:
- Around line 50-54: Replace the exact markdown bullet assertions in the
capability matrix test with a formatting-independent check that parses the
published numeric counts from docs/three-surface-capability-matrix.md and
compares them with counts derived from CAPABILITY_REGISTRY. Ensure the
registry-derived keys include the CLI-only category by validating the
corresponding surfaceCount("api+cli") value, while preserving checks for the
other documented capability categories.
---
Nitpick comments:
In `@src/contracts/output-schema-mcp.ts`:
- Around line 89-95: Define a shared importBindingDiagnosticSchema in
src/contracts/output-schema-mcp.ts at lines 89-95 and use it for
graft_import_diagnostics.diagnostics; replace the inline graft diagnostic at
src/contracts/output-schemas.ts:966-966 with
mcpOutputBodySchemas.graft_import_diagnostics, and replace the inline
referenceWarnings element schemas at src/contracts/output-schemas.ts:1359-1365
and src/contracts/output-schema-mcp.ts:484-490 with the shared diagnostic
schema.
In `@src/warp/go-reference-context.ts`:
- Around line 65-72: Replace the whole-file importPattern scan in the relevant
go-reference-context flow with import_declaration nodes from the parsed source
(or reuse the caller’s existing syntax tree). Derive packageDirectories only
from actual import specs, preserving the modulePath matching behavior while
excluding strings, struct tags, and comments.
In `@src/warp/python-import-resolver.ts`:
- Around line 5-34: Centralize astNodeId and emitAstAnchor in the existing
ast-emitter module or a focused shared module, then remove the duplicate
implementations from python-import-resolver.ts, ast-import-resolver.ts, and
qualified-reference-resolver.ts and import the shared helpers. Replace the SHA-1
createHash usage with a non-cryptographic hash suitable for generating these
IDs, preserving the existing ID format and anchor properties.
In `@src/warp/qualified-reference-resolver.ts`:
- Around line 336-480: Refactor collectShadowRegions into separate per-language
strategy helpers/modules behind a small shared interface, while preserving all
existing shadow-region behavior. Move Python, Rust, Go, and
TypeScript/JavaScript rules out of the single walk callback, hoist every
language-specific node-type Set/array (including functionTypes, blockTypes,
parameterTypes, localTypes, and ancestor-type sets) to reusable module-level
constants, and have collectShadowRegions select the appropriate strategy without
allocating these collections per AST node.
In `@test/unit/warp/qualified-reference-resolver.test.ts`:
- Around line 172-196: Add a Go fixture case covering a grouped declaration such
as “var Alpha, Beta int” in the existing qualified-reference resolver test, and
reference both exported names from the source so resolution of the non-first
name is exercised. Update the expected accesses to verify Beta resolves
correctly, preserving the existing checks for shadowing and duplicate
declarations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd49da2-c8f0-47ec-8f95-4f06b66061e2
⛔ Files ignored due to path filters (9)
CHANGELOG.mdis excluded by!**/*.mddocs/CLI.mdis excluded by!**/*.mddocs/MCP.mdis excluded by!**/*.mddocs/design/CORE_continuum-structural-reading-port.mdis excluded by!**/*.mddocs/design/WARP_cross-language-qualified-reference-resolution.mdis excluded by!**/*.mddocs/method/backlog/bad-code/committed-reference-scan-repeats-repository-analysis.mdis excluded by!**/*.mddocs/method/retro/WARP_cross-language-qualified-reference-resolution/WARP_cross-language-qualified-reference-resolution.mdis excluded by!**/*.mddocs/method/retro/WARP_cross-language-qualified-reference-resolution/witness/verification.mdis excluded by!**/*.mddocs/three-surface-capability-matrix.mdis excluded by!**/*.md
📒 Files selected for processing (35)
src/cli/cli-error.tssrc/cli/command-parser.tssrc/cli/structural-review-render.tssrc/contracts/capabilities.tssrc/contracts/output-schema-cli.tssrc/contracts/output-schema-mcp.tssrc/contracts/output-schemas.tssrc/echo/structural-reading-generated-model.tssrc/mcp/burden.tssrc/mcp/tool-registry.tssrc/mcp/tools/import-diagnostics.tssrc/mcp/tools/structural-review.tssrc/operations/import-reference-impact.tssrc/operations/structural-review.tssrc/ports/structural-reading.tssrc/warp/committed-reference-scan.tssrc/warp/go-reference-context.tssrc/warp/import-diagnostic.tssrc/warp/index-head.tssrc/warp/python-import-resolver.tssrc/warp/qualified-reference-resolver.tssrc/warp/structural-reading-adapter.tstest/unit/cli/command-parser.test.tstest/unit/cli/structural-review-render.test.tstest/unit/contracts/capabilities.test.tstest/unit/contracts/output-schemas.test.tstest/unit/echo/generated-model-parity.test.tstest/unit/mcp/import-diagnostics.test.tstest/unit/mcp/structural-review-cold-warp.test.tstest/unit/warp/committed-reference-scan.test.tstest/unit/warp/python-import-resolver.test.tstest/unit/warp/qualified-reference-index.test.tstest/unit/warp/qualified-reference-resolver.test.tstest/unit/warp/structural-reading-adapter.test.tstests/playback/0078-three-surface-capability-baseline-and-parity-matrix.test.ts
💤 Files with no reviewable changes (1)
- src/operations/import-reference-impact.ts
Owner
Author
Code Lawyer self-audit findingsThe branch audit found the following issues in addition to the existing review threads. These are now part of the same P0-P5 repair queue.
Cc: @codex — please provide a second opinion on these additional findings, especially SELF-01 through SELF-05. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51976aec0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
Activity Summary — exact-head closure
Validation at code head |
Owner
Author
|
@coderabbitai review please |
|
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review please |
|
✅ Action performedReview finished.
|
Owner
Author
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3862c7a0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
Code Lawyer exact-head audit addition
Cc: @codex — please confirm the adapter registry should be the sole runtime language-membership authority. |
|
To use Codex here, create an environment for this repo. |
Owner
Author
Code Lawyer exact-head audit addition
Cc: @codex — please verify the intended source-kind precedence for extensionless JavaScript imports. |
Owner
Author
Activity Summary — Code Lawyer exact-head closure
Validation at code head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf538f765b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
Code Lawyer Activity Summary — final closureFinal published head:
Verification witness:
@codex Please confirm the final head has no remaining actionable issue. |
|
To use Codex here, create an environment for this repo. |
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
1 participant
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
Repairs cold-WARP review impact counts with an exact-ref fallback, adds first-party qualified reference inference for Python, TypeScript/JavaScript, Rust, and Go, and reports lexical-shadow confidence through review and import-diagnostic surfaces.
The bounded WARP indexing policy remains unchanged. Exact committed scans take precedence over stale graph evidence; unsupported dynamic and interprocedural aliases are deliberately excluded and reported as partial confidence.
Validation
Validated code head
e8dd3011; final evidence headb3862c7a.pnpm test— 252 files, 1,968 testspnpm lintpnpm typecheckpnpm buildImport diagnostics fail closed on this repository intentionally malformed tracked fixture instead of returning a false empty result; clean disposable-repository CLI/MCP/schema tests cover successful empty and non-empty payloads. Exact commands and synthetic witness SHAs are recorded in the committed verification witness.
Summary by CodeRabbit