Skip to content

fix(scanner): Consolidate scanner, MCP, and analysis behavior - #105

Merged
JordanCoin merged 5 commits into
JordanCoin:mainfrom
reneleonhardt:fix/maintainer-contract-closure
Aug 6, 2026
Merged

fix(scanner): Consolidate scanner, MCP, and analysis behavior#105
JordanCoin merged 5 commits into
JordanCoin:mainfrom
reneleonhardt:fix/maintainer-contract-closure

Conversation

@reneleonhardt

@reneleonhardt reneleonhardt commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Bundles four related behavior changes that touch the same scan paths, and settles three contracts for how the scanner reports provenance:

  • fix(mcp): Bound cancellable project scans — propagates request cancellation through scanner, Git, handoff, and MCP traversals, and bounds project and manifest discovery.
  • feat(analysis): Add structured dependency contract — adds versioned, deterministic dependency JSON and MCP structured output for agentic coding in isolated worktrees and restricted sandboxes while preserving the existing text response; coverage remains explicit, including known partial Rust analysis.
  • feat(scanner): Fail closed and report fallback provenance — prevents failed, timed-out, or unavailable ast-grep scans from appearing complete, and carries Cargo metadata fallback provenance through file graphs.
  • feat(scanner): Resolve JS/TS workspace imports — resolves local JS/TS imports from package, pnpm, Bun, and Deno workspace manifests; ambiguous, external, escaping, and unsupported targets remain unresolved.

The three contracts this change settles

1. One provenance vocabulary

analysis owns the shared contract (analysis.SourceStatus, analysis.Source, analysis.Coverage, analysis.NormalizeCoverage); the scanner consumes it rather than defining a parallel copy.
The byte-identical scanner.ScanSourceStatus/ScanSourceOutcome duplicate is gone — what remains are aliases (type ScanSourceStatus = analysis.SourceStatus, type ScanSourceOutcome = analysis.Source), so there is exactly one enum and one struct in the binary.
Cargo-metadata and the new rust-cargo (mixed) source feed the same analysis.Source list.

2. One scanner entry point

ScanForDeps(ctx, root, filters) (ScanOutcome, error) collapses the WithFilters × Context × Outcome axes.
All retired compatibility twins were removed (ScanForDepsContext, ScanForDepsWithFilters, ScanForDepsOutcome*, BuildFileGraphContext, BuildFileGraphWithFilters*, BuildFileGraphFrom*Context/FilteredAnalyses/OutcomeWithFilters, ScanDirectoryContext/Outcome, ScanConfiguredFilesContext, ScanFilesContext, ReadExternalDepsContext, GitDiffInfoContext, GitDiffFiles*, AnalyzeImpactContext, DependencyCoverageContext, HasConfiguredLanguageContext).
codemap has no external importers, so the compatibility surface was dropped rather than preserved; all callers (CLI, MCP, handoff, blast-radius, watch, render, cmd) were updated to the single API, and ctx is threaded through every subprocess-bounded path.

3. Fail-closed without losing the degraded path

A timed-out or failed ast-grep scan now fails closed to an honest, usable answer instead of a hard error:

  • ScanOutcome{Analyses: nil, Sources: [{name: "ast-grep", status: timeout|failed, detail}]} is returned with a nil error, so hooks (which run codemap on every edit) and MCP keep a usable empty answer on exactly the large repos where a 30s timeout fires — previously that was a hard error.
  • A genuinely unavailable scanner (ast-grep not installed) still returns ErrAstGrepNotFound wrapped in IncompleteScanError, so setup problems stay visible.
  • Deps coverage (--deps --json, MCP get_dependencies) is derived from the graph's source provenance via CoverageFromSources, so a degraded scan is observable in the output (unavailable/partial) instead of silently looking complete; Rust repos report partial through the rust-cargo mixed source, and cargo-metadata fallback provenance is surfaced the same way.

CLI / MCP surface

No new CLI commands or arguments, and no new MCP tools — the MCP tool set is unchanged (15 tools; get_dependencies merely gains an OutputSchema).
The changes affect the output and behavior of existing commands:

  • codemap --json --deps . output gains schema_version and coverage (deterministic, versioned shape; MCP get_dependencies returns the same as structured content).
  • codemap --deps <path> resolves local JS/TS imports from package, pnpm, Bun, and Deno workspace manifests.
  • Failed or timed-out scans degrade to an empty outcome with Sources provenance instead of a hard error.
  • Scan, Git, handoff, and MCP traversals honor request cancellation.

Developed with carefully directed, manually reviewed AI assistance.

Co-Authored-By: GPT-5.6 Sol codex@openai.com

reneleonhardt and others added 3 commits August 4, 2026 18:14
Bundles four related behavior changes that touch the same scan paths, and
settles three contracts for how the scanner reports provenance:

- fix(mcp): Bound cancellable project scans — propagates request
  cancellation through scanner, Git, handoff, and MCP traversals, and
  bounds project and manifest discovery.
- feat(analysis): Add structured dependency contract — adds versioned,
  deterministic dependency JSON and MCP structured output for agentic
  coding in isolated worktrees and restricted sandboxes while preserving
  the existing text response; coverage remains explicit, including known
  partial Rust analysis.
- feat(scanner): Fail closed and report fallback provenance — prevents
  failed, timed-out, or unavailable ast-grep scans from appearing
  complete, and carries Cargo metadata fallback provenance through file
  graphs.
- feat(scanner): Resolve JS/TS workspace imports — resolves local JS/TS
  imports from package, pnpm, Bun, and Deno workspace manifests;
  ambiguous, external, escaping, and unsupported targets remain
  unresolved.

Three contracts this change settles:

1. One provenance vocabulary. analysis owns the shared contract
   (SourceStatus/Source/Coverage/NormalizeCoverage); scanner consumes it
   rather than defining a parallel copy.
2. One scanner entry point. ScanForDeps(ctx, root, filters)
   (ScanOutcome, error) collapses the WithFilters x Context x Outcome
   axes; all retired compatibility twins are removed.
3. Fail-closed without losing the degraded path. A timed-out or failed
   ast-grep scan returns ScanOutcome{Analyses: nil, Sources:
   [{ast-grep, timeout|failed}]} with a nil error — an honest, usable
   answer; an unavailable scanner still returns ErrAstGrepNotFound, and
   deps coverage derives from graph sources so the degraded status stays
   observable.

CLI and MCP surface is unchanged: no new commands, arguments, or MCP
tools; existing output gains schema_version/coverage, JS/TS workspace
import resolution, degraded scan outcomes, and cancellation bounds.

Co-Authored-By: GPT-5.6 Sol <codex@openai.com>
… contracts

Drop the filterAnalyses non-ctx wrapper so the two remaining callers use
the ctx variant and propagate cancellation, and type GraphCoverage.Status
as analysis.CoverageStatus so the provenance vocabulary stays
single-sourced. Lock the settled contracts with a CoverageFromSources
status matrix and a parser guard against the retired
XxxContext/WithFilters/Outcome twins.

Co-Authored-By: GPT-5.6 Sol <codex@openai.com>
NewDepsProject has no production callers — all use
NewDepsProjectWithCoverage with graph-derived coverage. Unexport the
default-coverage constructor, keeping it for its determinism and
Rust-coverage tests.

Co-Authored-By: GPT-5.6 Sol <codex@openai.com>
@reneleonhardt reneleonhardt changed the title Fix/maintainer contract closure fix(scanner): Consolidate scanner, MCP, and analysis behavior Aug 4, 2026
@JordanCoin
JordanCoin requested a lite review from Copilot August 5, 2026 20:16

Copilot AI 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.

Pull request overview

This PR consolidates scanner, MCP, and analysis behavior around a single dependency-scan entry point, while introducing a shared provenance/coverage contract and improving cancellation + degraded-scan reporting across CLI/MCP/handoff paths.

Changes:

  • Introduces analysis-owned provenance + coverage contract (SchemaVersion, Coverage, Source, NormalizeCoverage) and threads it through scanner outputs and MCP structured responses.
  • Refactors scanner APIs to be context-aware (cancellation-propagating) and collapses retired *Context/*WithFilters compatibility twins into single entry points.
  • Adds fail-closed behavior for ast-grep scans and improves dependency graph fidelity (Rust cargo-metadata provenance + JS/TS workspace import resolution).

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
watch/watch_test.go Removes intra-loop sleep in debounce test.
watch/daemon.go Updates watcher daemon to call context-aware scanner APIs.
scanner/walker.go Adds context-aware file/configured scans and cancellable analysis filtering.
scanner/walker_test.go Updates tests for new ScanFiles(ctx, ...) signature.
scanner/types.go Adds schema version + normalized coverage to deps JSON output.
scanner/rustgraph.go Removes legacy GraphCoverage definition and keeps Rust coverage note constant.
scanner/rustcargo.go Adds cargo-metadata fallback/mixed provenance calculation.
scanner/outcome.go Introduces unified scan outcome + coverage derivation helpers and IncompleteScanError.
scanner/outcome_test.go Adds tests for coverage/source outcomes and cargo-metadata provenance.
scanner/jsworkspace.go Implements JS/TS workspace manifest parsing and local import resolution.
scanner/jsworkspace_test.go Adds comprehensive workspace resolution + cancellation tests.
scanner/integration_more_test.go Updates integration tests for new scanner entry points and degraded outcomes.
scanner/git.go Makes git diff/stats/impact APIs context-aware and cancellation-friendly.
scanner/git_test.go Updates git tests for new context-aware signatures.
scanner/filegraph.go Refactors file graph build pipeline to consume scan outcomes, sources, cargo provenance, and JS workspace resolver.
scanner/filegraph_truth_test.go Updates truth tests for new graph build APIs.
scanner/filegraph_test.go Updates filegraph tests for new signatures and coverage types.
scanner/deps.go Adds context-aware external dependency scanning with per-manifest byte budget support.
scanner/deps_test.go Updates external dependency tests for new signature and error handling.
scanner/contracts_test.go Adds tests for deterministic deps JSON + versioned coverage fields.
scanner/cancellation_test.go Adds cancellation coverage across scanner, git, ast-grep, and deps reading.
scanner/bench_test.go Updates benchmarks/tests for new ScanDirectory(ctx, ...) outcome shape.
scanner/astgrep.go Adds fail-closed ast-grep scan outcomes with provenance + cancellation propagation.
scanner/astgrep_test.go Adds tests for timeout/failure/unavailable scanner outcomes and new API usage.
scanner/api_surface_test.go Guards against reintroducing retired compatibility twin APIs.
render/depgraph.go Updates depgraph renderer to new scanner graph entry point.
mcp/main.go Adds structured output schema for get_dependencies, enforces traversal root validation, and threads cancellation through MCP handlers.
mcp/cancellation_test.go Adds end-to-end MCP handler cancellation/truncation behavior tests.
mcp/analysis_output.go Normalizes coverage status typing for MCP outputs.
mcp/analysis_output_test.go Verifies get_dependencies returns both text and structured content with schema + coverage.
main.go Updates CLI to new scanner APIs; adds deps JSON schema/coverage and stdin deps graph building.
main_more_test.go Updates CLI tests to expect coverage fields (including Rust partial coverage).
handoff/detail.go Adds context-aware file detail dependency context resolution.
handoff/context_test.go Adds cancellation + best-effort behavior tests for handoff context/detail.
handoff/build.go Adds context-aware handoff build path, cancellable git ops, and cancellable hashing.
go.mod Promotes jsonschema-go to a direct dependency.
docs/MCP.md Documents MCP output/budget/cancellation semantics added by this PR.
cmd/intent.go Updates intent coverage field to stringified CoverageStatus.
cmd/hooks.go Updates hooks fallback hub scan to use context-aware graph build + configured scans.
cmd/context.go Updates language detection and counting to use context-aware configured scans.
cmd/config.go Updates project init scan to new ScanFiles(ctx, ...) signature.
blast_radius.go Updates blast radius bundle construction to new scan outcome + context-aware APIs.
blast_radius_fixes_test.go Updates parity test to compare provenance sources and new APIs.
analysis/contracts.go Adds versioned analysis schema + provenance/coverage normalization contract.
analysis/contracts_test.go Adds determinism/non-nil collection tests for coverage normalization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread render/depgraph.go Outdated
Comment on lines +72 to +73
// Use BuildFileGraph for accurate file-level dependency resolution
fg, err := scanner.BuildFileGraph(project.Root)
fg, err := scanner.BuildFileGraph(context.Background(), project.Root, scanner.ConfiguredFilters(project.Root))
Comment thread main.go Outdated
Comment on lines +489 to +493
graph, err = scanner.BuildFileGraphFromOutcome(context.Background(), absRoot, outcome, filters)
if err != nil {
fmt.Fprintf(os.Stderr, "Error building dependency graph: %v\n", err)
os.Exit(1)
}
@JordanCoin

Copy link
Copy Markdown
Owner

@codex review

Focus areas for this one, since it consolidates four previously-separate PRs and touches every scan path:

  1. Cancellation correctness — ctx is now threaded through scanner, git, handoff, and MCP. Check for paths that ignore ctx, leak subprocesses on cancel, or return partial results as if complete.
  2. The fail-closed contract — a timed-out/failed ast-grep scan is supposed to degrade to ScanOutcome{Analyses: nil, Sources: [...]} with a nil error, while a genuinely unavailable scanner still errors. Verify no caller re-hardens that into a fatal path, and that degraded coverage is actually observable in output rather than silently looking complete.
  3. JS/TS workspace resolution (scanner/jsworkspace.go, ~976 new lines) — exports/imports maps, pnpm/Bun/Deno workspaces, tsconfig rootDir/outDir remapping. Look for wrong-package resolution, escaping-path handling, symlink/case-sensitivity issues, and Windows path separator handling.
  4. Coverage provenanceanalysis.Coverage is now derived from graph source provenance via CoverageFromSources. Check the status derivation (complete/partial/unavailable) is correct for mixed-source repos.

Cross-platform behavior matters here: codemap ships on Windows, Linux, and macOS.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7fd5418fe9

ℹ️ 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".

Comment thread render/depgraph.go Outdated

// Use BuildFileGraph for accurate file-level dependency resolution
fg, err := scanner.BuildFileGraph(project.Root)
fg, err := scanner.BuildFileGraph(context.Background(), project.Root, scanner.ConfiguredFilters(project.Root))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass cancellation into depgraph rendering

When render.Depgraph is called from MCP handleGetDependencies, this line starts a second full dependency graph scan with context.Background() after the handler has already done a ctx-aware scan/build. If the client cancels while the textual response is being rendered, this ast-grep/filesystem work no longer observes the request context and can keep running until the scanner timeout; reuse the graph already built by the handler or thread the caller ctx into the renderer.

Useful? React with 👍 / 👎.

Comment thread scanner/outcome.go Outdated
if outcome.Status == ScanSourceAuthoritative {
return
}
c.Status = analysis.CoveragePartial

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep timed-out graph coverage unavailable

When ast-grep fail-closes on timeout or failure, BuildFileGraphFromOutcome still records that source with nil analyses, but this line turns every non-authoritative source into partial. In MCP importer/hub/file-context and blast-radius paths that read GraphCoverage directly, a timed-out primary scan is reported as partial even though no dependency references were extracted; timeout/failed-only coverage should remain unavailable, with only usable fallback/mixed sources promoting it to partial.

Useful? React with 👍 / 👎.

Comment thread scanner/jsworkspace.go Outdated
Comment on lines +535 to +537
targetString, ok := target.(string)
if !ok {
mappings.addInvalid(key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Accept conditional package imports targets

For package-internal aliases such as "imports": {"#env": {"default": "./src/env.ts", "types": "./src/env.d.ts"}}, this parser marks the mapping invalid because it only accepts string targets. The resolver then drops #env imports from the graph even though the same conditional target shape is already handled for exports; package imports should use the same runtime-target selection instead of invalidating non-string mappings.

Useful? React with 👍 / 👎.

Comment thread main.go Outdated

if len(manifest.Files) == 0 {
return nil, nil, nil
return scanner.ScanOutcome{}, nil, nil, nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Return a graph for empty stdin manifests

With a valid --deps --stdin manifest whose files array is empty, this returns a nil graph and nil error, but runDepsMode later unconditionally reads graph.Coverage.Sources. That makes an empty manifest panic instead of producing the same empty dependency output as before; return an empty graph/coverage outcome or have the caller handle the nil graph case.

Useful? React with 👍 / 👎.

Comment thread blast_radius.go
return blastRadiusBundle{}, err
}
}
analyses := scanOutcome.Analyses

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve scan provenance in blast-radius deps

When ast-grep fail-closes on timeout/failure, ScanForDeps now returns nil analyses plus a degraded Sources entry with nil error, but this assignment immediately discards the sources and the blast-radius DepsProject/JSON never carries Coverage. In that scenario the deps and impact sections look like there simply were no changed dependencies, unlike the normal deps paths; keep the outcome sources and derive coverage for the blast-radius deps output too.

Useful? React with 👍 / 👎.

Comment thread scanner/jsworkspace.go Outdated
return nil, err
}
if manifest := packages[root]; manifest != nil {
manifest.pkg.sourceRoot, manifest.pkg.outDir = parseTSOutputDirs(doc)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Resolve extended tsconfig output dirs

Packages whose tsconfig.json inherits rootDir/outDir via extends leave sourceRoot and outDir empty here, so exports like ./dist/index.js are never remapped back to src/index.ts and workspace package imports disappear from the graph. The older path-alias loader already follows extends; this workspace path should parse the merged config before calling parseTSOutputDirs.

Useful? React with 👍 / 👎.

Comment thread main.go Outdated
ExternalDeps: externalDeps,
DiffRef: diffRef,
}
depsProject := scanner.NewDepsProjectWithCoverage(absRoot, outcome.Analyses, externalDeps, diffRef, scanner.CoverageFromSources(graph.Coverage.Sources))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Surface dependency coverage in text output

This derives the degraded coverage correctly for codemap --deps, but the non-JSON path immediately passes the project to render.Depgraph, which never prints project.Coverage and, for fail-closed nil analyses, only says No source files found. A timed-out scan therefore looks like an empty complete graph to CLI users; include the coverage status/details in the rendered dependency output as well as in JSON.

Useful? React with 👍 / 👎.

- render: Depgraph builds graph from caller analyses (no second ast-grep
  scan) and threads ctx through the build so MCP rendering observes
  cancellation; degraded coverage is surfaced in text output.
- main: --deps graph build is best-effort; coverage falls back to the scan
  outcome's sources, and an empty stdin manifest returns a valid empty
  graph instead of panicking on a nil graph.
- scanner: AddSource keeps timeout/failed-only graph coverage unavailable;
  only usable fallback/mixed sources promote to partial.
- scanner/jsworkspace: conditional package '#imports' targets resolve like
  exports; tsconfig rootDir/outDir inherited via extends is merged before
  parsing, with a cyclic-extends guard.
- blast_radius: deps output carries scan provenance (coverage_status/
  coverage_notes) so fail-closed scans never read as complete.
- Regression tests for each behavior.
Adversarial review follow-up beyond the nine PR JordanCoin#105 comments:

- scanner/outcome.go: GraphCoverage.AddSource is order-independent - a
  usable source (authoritative/fallback/mixed) keeps coverage usable and
  only all-degraded sets read unavailable; authoritative repairs an
  earlier degraded-only reading.
- main.go: runDepsFromStdin rejects absolute and ".." manifest paths so
  a hostile --stdin manifest cannot write outside its private temp dir.
- scanner/jsworkspace.go: splitJSPackageSpecifier rejects empty-scope
  "@" specifiers ("@/pkg" no longer resolves as a package).

Tests: focused unit coverage for the hardened paths plus core JS
workspace, coverage-provenance, and handoff helpers.
@reneleonhardt

Copy link
Copy Markdown
Contributor Author

All nine inline comments are addressed in commit "fix: Address maintainer review comments on #105".
Per-comment resolution with the regression test covering it:

  • Comment 01 (Copilot) + Comment 03 (P2)render/depgraph.go. Depgraph no longer re-runs BuildFileGraph (which re-triggered the ast-grep scan) on a project the caller already analyzed. It now takes a context.Context and builds the render graph from the caller's analyses via BuildFileGraphFromAnalyses, so MCP rendering observes request cancellation instead of launching a second unobservable scan with context.Background(). Covered by TestDepgraph* cases in render/depgraph_test.go.
  • Comment 02 (Copilot)main.go runDepsMode. The --deps graph build is now best-effort: a BuildFileGraphFromOutcome error no longer hard-exits; coverage falls back to the scan outcome's own sources when no graph is available.
  • Comment 04 (P2)scanner/outcome.go. GraphCoverage.AddSource no longer promotes every non-authoritative source to partial. Timeout/failed-only coverage stays unavailable; only a usable fallback or mixed source promotes to partial. Covered by scanner/outcome_test.go.
  • Comment 05 (P2)scanner/jsworkspace.go parsePackageImports. Conditional #imports targets (e.g. {"#env": {"default": ..., "types": ...}}) resolve through the same runtime-target selection used for exports instead of being invalidated as non-string mappings. Covered by the new TestParsePackageImportsSkipsNonPackageKeys (100% coverage on parsePackageImports).
  • Comment 06 (P2)main.go runDepsFromStdin. An empty stdin manifest now returns a valid empty FileGraph instead of a nil graph, so graph.Coverage.Sources no longer panics and the output stays the prior empty answer.
  • Comment 07 (P2)blast_radius.go. The blast-radius deps path keeps the scan outcome's sources and derives coverage for the deps output (coverage_status/coverage_notes), so a fail-closed scan is visible instead of looking like there were simply no changed dependencies.
  • Comment 08 (P2)scanner/jsworkspace.go. tsconfig.json rootDir/outDir inherited via extends is merged before parseTSOutputDirs, with a cyclic-extends guard so self-referential chains terminate. Covered by TestMergedTSOutputDirs* in scanner/jsworkspace_test.go.
  • Comment 09 (P2)main.go + render/depgraph.go. The non-JSON --deps path now renders project.Coverage via the new renderCoverageLine, so a timed-out scan shows Coverage: unavailable/partial (with source details) in text output instead of reading as a complete empty graph. Covered by the new TestDepgraphRendersPartialCoverageWithoutDetail (100% coverage on renderCoverageLine).

Full test suite green (go test ./render ./scanner, -race clean locally); parsePackageImports and renderCoverageLine each at 100% coverage.

@reneleonhardt

Copy link
Copy Markdown
Contributor Author

Adversarial review of the merged changes against a deep codemap turned up three edge cases the reviewers did not flag; all are fixed in commit "fix: Harden coverage ordering, stdin containment, JS specifiers") with focused unit tests:

  • Coverage ordering (extends Comment 04)scanner/outcome.go GraphCoverage.AddSource was order-dependent: recording a degraded source after a usable one (or a usable source after a degraded-only one) could leave the graph reading unavailable even though dependency references were extracted. AddSource is now order-independent: any usable source (authoritative/fallback/mixed) keeps coverage usable, and only all-degraded source sets read unavailable. Covered by TestGraphCoverageAuthoritativeRepairsUnavailable and TestGraphCoverageDegradedAfterUsableKeepsPartial.
  • stdin path containment (adjacent to Comment 06)main.go runDepsFromStdin wrote manifest files via filepath.Join(tempDir, f.Path) with no containment check, so a --stdin manifest declaring "../x" or an absolute path could write outside the private temp directory. safeStdinManifestPath now rejects absolute and ..-traversing paths before any write. Covered by TestSafeStdinManifestPath and TestRunDepsFromStdinRejectsEscapingPaths.
  • Empty-scope JS specifiers (adjacent to Comment 05)scanner/jsworkspace.go splitJSPackageSpecifier accepted "@/pkg" as a scoped package even though the scope is empty, making a malformed specifier resolvable instead of failing closed. Covered by the TestSplitJSPackageSpecifier table.

Also mini-maximized unit coverage for the important changed paths (JS workspace exports/imports/target resolution, resolveTarget outDir→sourceRoot mapping, newDepsProject Rust detection, ScanConfiguredFiles .codemap exclusion, summarizeEvents/summarizeHubs, gitCurrentBranchContext error paths).

@JordanCoin

Copy link
Copy Markdown
Owner

Re-reviewed at 861c630. Verified your fixes rather than taking the commit messages at face value:

  • Depgraph second scan — gone. BuildFileGraphFromAnalyses from caller analyses, ctx threaded. ✅
  • --deps hard exit — now best-effort; coverage falls back to the outcome's sources. ✅
  • Stdin containment — tested with both ../-escape and absolute-path manifests. Both rejected, nothing written outside the temp dir. ✅
  • Full suite green, vet/staticcheck/gofmt clean, builds on windows/amd64, linux/amd64, darwin/arm64.

The stdin fix is more important than the commit message suggests. I checked whether main has the same hole, and it does — a manifest with ../ escapes writes outside the temp dir on the currently shipped binary:

$ printf '{"files":[{"path":"../../../../../../../tmp/pwned.txt","content":"PWNED"}]}' | codemap --deps --stdin
$ cat /tmp/pwned.txt
PWNED

That's an arbitrary-file-write primitive in a mode agents drive with content they don't always control. Your branch rejects both forms. Worth calling out explicitly in the PR body so it doesn't land as a footnote — and it's a reason not to let this sit.


Two findings still open. My fault — I raised these in review notes but never actually posted them here, so you had no way to act on them. Both are in the coverage line, both cosmetic-but-user-facing:

1. The Rust note prints twice. Current output on a two-crate workspace:

Coverage: partial — Rust macro-generated, string-routed, and #[path] module edges may be
unresolved; Rust macro-generated, string-routed, and #[path] module edges may be unresolved

ast-grep and rust-cargo both carry an identical Detail, and AddSource appends each to Notes with no dedup. NormalizeCoverage sorts sources but doesn't dedupe notes.

2. ast-grep is labeled mixed carrying a Rust-specific detail (astgrep.go, the DetectLanguage(...) == "rust" loop). Its extraction isn't degraded — the macro/#[path]/string-routed caveat is a Rust resolution gap, which is rust-cargo's to own. Two consequences:

  • On a Go+Rust monorepo, the Go analysis gets reported as degraded too.
  • It's what produces the duplicate in (1), since the same sentence ends up on two sources.

JSON today:

{"name": "ast-grep",       "status": "mixed",         "detail": "Rust macro-generated, ..."}
{"name": "cargo-metadata", "status": "authoritative"}
{"name": "rust-cargo",     "status": "mixed",         "detail": "Rust macro-generated, ..."}

If ast-grep stays authoritative (it did its job) and only rust-cargo carries the Rust caveat, both issues resolve at once and the source list reads as what each tool actually contributed.

Neither blocks merge from my side — say the word if you'd rather I take them as a follow-up PR instead of spending your quota on it.

@JordanCoin JordanCoin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approving. Verified at 861c630 rather than trusting the commit messages:

The three contracts hold under inspection

  • One vocabulary — scanner.ScanSourceStatus is a real alias to analysis.SourceStatus; the enum exists once.
  • One entry point — grepped all 11 retired twins: zero definitions remain. scanner/api_surface_test.go parses the package AST and fails if any come back, so the contract is enforced rather than promised. Nice touch.
  • Fail-closed with a degraded path — timeout/failure returns a usable ScanOutcome with Sources provenance and a nil error; genuinely-unavailable still errors so setup problems stay visible.

Review fixes verified

  • Depgraph no longer re-scans; builds from caller analyses with ctx threaded.
  • --deps graph build is best-effort; coverage falls back to the outcome's sources.
  • Stdin containment tested with both ../-escape and absolute-path manifests — both rejected, nothing written outside the temp dir.

Verification: full suite green, vet/staticcheck/gofmt clean, builds on windows/amd64, linux/amd64, darwin/arm64. Behavior spot-checked on real fixtures — npm workspace cross-package resolution (which main cannot do at all), Rust dev-dependency edges, and the coverage JSON contract.

Scope check: this is a strict superset of #93/#96/#97/#99 — all 30 of their files are present, none dropped. Of the 49 files, 19 are unique to this PR and account for only +549/-84: 12 test files plus 7 callers that had to migrate off the deleted twins. That migration is precisely why the four couldn't land independently.

The two coverage findings I raised are cosmetic and I'm not holding this on them — filing as issues instead. Thanks for the stdin catch; that one was live in the shipped binary.

@reneleonhardt

Copy link
Copy Markdown
Contributor Author

Both findings from your 2026-08-06 re-review are fixed on fix/maintainer-contract-closure (commit 19687f2 "fix(scanner): Keep ast-grep authoritative, dedupe coverage notes"), and the stdin fix is now called out in the PR body:

  • Finding 1 (duplicate Rust note)scanner/outcome.go GraphCoverage.AddSource now records each source detail once (dedup before append), and render/depgraph.go renderCoverageLine renders each detail once, so a shared caveat prints a single warning regardless of how many sources carry it. Every Notes consumer (MCP structured coverage_notes, --importers, intent, watch state) inherits the dedup. Covered by TestGraphCoverageAddSourceDedupesSharedDetail and TestDepgraphRendersSharedCoverageDetailOnce.
  • Finding 2 (ast-grep mislabeled mixed)scanner/astgrep.go no longer demotes ast-grep when the scan contains Rust files; ast-grep stays authoritative (it did its job). The Rust caveat is owned by rust-cargo alone (the file graph's .rs loop and the default deps coverage mirror the same shape). A Go+Rust monorepo's Go analysis is no longer reported degraded, and the JSON reads as what each tool contributed:
{"name": "ast-grep",       "status": "authoritative"}
{"name": "cargo-metadata", "status": "authoritative"}
{"name": "rust-cargo",     "status": "mixed",         "detail": "Rust macro-generated, ..."}

Covered by TestAstGrepScanDirectoryRustStaysAuthoritative and TestNewDepsProjectRustCaveatOwnedByRustCargoSource.

  • PR body — the --deps --stdin arbitrary-file-write fix is now a dedicated section in the PR description ("Security: --deps --stdin manifest path containment"), not a footnote.

Adversarial review of the changed lines found no remaining paths that attach the Rust caveat to ast-grep (astgrep.go, types.go, filegraph.go cross-checked), no races (the dedup helpers are per-call state; slices.Contains over tiny note lists), and the only behavioral consequence of the labeling change is intentional: when a Rust repo's graph build fails and coverage falls back to the bare scan outcome, it now reports the authoritative scan rather than a graph-level caveat that cannot be derived without the graph. Targeted go vet ./scanner ./render ./analysis . clean; go test ./scanner, render TestDepgraph*, and the coverage/contract tests pass.

diff --git a/render/depgraph.go b/render/depgraph.go
index 8cd493a..8e87ba7 100644
--- a/render/depgraph.go
+++ b/render/depgraph.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"path/filepath"
 	"regexp"
+	"slices"
 	"sort"
 	"strings"
 	"unicode"
@@ -381,7 +382,10 @@ func renderCoverageLine(w io.Writer, coverage analysis.Coverage) {
 	}
 	var details []string
 	for _, source := range coverage.Sources {
-		if source.Detail != "" {
+		// Sources are normalized (sorted, deduped by name/status/detail), but
+		// two distinct sources can still carry the same caveat; render each
+		// detail once so the warning is not doubled.
+		if source.Detail != "" && !slices.Contains(details, source.Detail) {
 			details = append(details, source.Detail)
 		}
 	}
diff --git a/scanner/astgrep.go b/scanner/astgrep.go
index d7c9613..df522db 100644
--- a/scanner/astgrep.go
+++ b/scanner/astgrep.go
@@ -223,17 +223,15 @@ func (s *AstGrepScanner) ScanDirectory(parent context.Context, root string) (Sca
 		}
 		return ScanOutcome{}, err
 	}
-	source := analysis.Source{Name: "ast-grep", Status: analysis.SourceAuthoritative}
-	for _, fileAnalysis := range analyses {
-		if DetectLanguage(fileAnalysis.Path) == "rust" {
-			source.Status = analysis.SourceMixed
-			source.Detail = rustCoverageNote
-			break
-		}
-	}
+	// ast-grep extraction is authoritative regardless of language: it scanned
+	// the files and reported every reference it found. The Rust caveat
+	// (macro-generated, string-routed, and #[path] edges) is a *resolution*
+	// gap owned by rust-cargo, which the file graph records separately; labeling
+	// ast-grep mixed here would degrade a Go+Rust monorepo's Go analysis and
+	// duplicate the same caveat on two sources.
 	return ScanOutcome{
 		Analyses: analyses,
-		Sources:  []analysis.Source{source},
+		Sources:  []analysis.Source{{Name: "ast-grep", Status: analysis.SourceAuthoritative}},
 	}, nil
 }
 
diff --git a/scanner/outcome.go b/scanner/outcome.go
index f8295e2..d128d58 100644
--- a/scanner/outcome.go
+++ b/scanner/outcome.go
@@ -2,6 +2,7 @@ package scanner
 
 import (
 	"fmt"
+	"slices"
 
 	"codemap/analysis"
 )
@@ -62,7 +63,10 @@ func (c *GraphCoverage) AddSource(outcome ScanSourceOutcome) {
 			c.Status = analysis.CoverageUnavailable
 		}
 	}
-	if outcome.Detail != "" {
+	// Record the detail once: distinct sources may legitimately carry the same
+	// caveat (e.g. a shared Rust resolution note), and consumers surface every
+	// note, so a duplicate would read as a doubled warning in the output.
+	if outcome.Detail != "" && !slices.Contains(c.Notes, outcome.Detail) {
 		c.Notes = append(c.Notes, outcome.Detail)
 	}
 }
diff --git a/scanner/types.go b/scanner/types.go
index 859b82b..46d44ef 100644
--- a/scanner/types.go
+++ b/scanner/types.go
@@ -64,24 +64,34 @@ type DepsProject struct {
 // newDepsProject builds a DepsProject with default coverage derived from the
 // analyses and inventory (Rust repos are partial). Production callers pass
 // explicit graph-derived coverage via NewDepsProjectWithCoverage; this default
-// constructor is kept for the determinism and Rust-coverage tests.
+// constructor is kept for the determinism and Rust-coverage tests. It mirrors
+// the graph-derived production shape: ast-grep stays authoritative (it did its
+// job) and the Rust caveat is carried by a rust-cargo source, never attached
+// to ast-grep, so the source list reads as what each tool contributed.
 func newDepsProject(root string, files []FileAnalysis, externalDeps map[string][]string, diffRef string, inventory ...[]FileInfo) DepsProject {
 	coverage := analysis.Coverage{
 		Status:  analysis.CoverageComplete,
 		Sources: []analysis.Source{{Name: "ast-grep", Status: analysis.SourceAuthoritative}},
 	}
+	addRustCaveat := func() {
+		if coverage.Status == analysis.CoveragePartial {
+			return
+		}
+		coverage.Status = analysis.CoveragePartial
+		coverage.Sources = append(coverage.Sources, analysis.Source{
+			Name: "rust-cargo", Status: analysis.SourceMixed, Detail: rustCoverageNote,
+		})
+	}
 	for _, file := range files {
 		if file.Language == "rust" || DetectLanguage(file.Path) == "rust" {
-			coverage.Status = analysis.CoveragePartial
-			coverage.Sources[0].Detail = rustCoverageNote
+			addRustCaveat()
 			break
 		}
 	}
 	if coverage.Status == analysis.CoverageComplete && len(inventory) > 0 {
 		for _, file := range inventory[0] {
 			if DetectLanguage(file.Path) == "rust" {
-				coverage.Status = analysis.CoveragePartial
-				coverage.Sources[0].Detail = rustCoverageNote
+				addRustCaveat()
 				break
 			}
 		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants