Skip to content

feat(scanner): Fail closed and report fallback provenance - #97

Draft
reneleonhardt wants to merge 1 commit into
JordanCoin:mainfrom
reneleonhardt:feat/scanner-generic-fallback
Draft

feat(scanner): Fail closed and report fallback provenance#97
reneleonhardt wants to merge 1 commit into
JordanCoin:mainfrom
reneleonhardt:feat/scanner-generic-fallback

Conversation

@reneleonhardt

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents failed, timed-out, or unavailable ast-grep scans from appearing complete and carries Cargo metadata fallback provenance through file graphs. Agentic coding consumers running in restricted sandboxes can distinguish authoritative dependency results from degraded topology.

This PR provides the provenance foundation for a fallback stack that will keep dependency analysis useful when optional scanners or toolchains are unavailable:

codemap --deps .

Follow-up language-specific fallback branches can preserve degraded topology while reporting its provenance.

Type of change

  • Bug fix
  • New feature
  • New language support
  • Documentation
  • Other (describe below)

Checklist

  • I've tested this locally with go build && ./codemap .
  • I've read CONTRIBUTING.md (for new language support)
  • I've updated documentation if needed

Additional notes

Successful empty ast-grep results remain authoritative. Existing API signatures remain unchanged; incomplete ast-grep executions now return errors instead of potentially incomplete results.

Verification

go test ./... -count=1; go vet ./...; go test -race -cover ./... -count=1; staticcheck ./...; go build.

Developed with carefully directed, manually reviewed AI assistance.

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

Distinguish authoritative scanner results from degraded fallback topology, fail closed on incomplete ast-grep execution, and preserve source outcomes through file-graph and blast-radius construction.

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

Copy link
Copy Markdown
Owner

Reviewed — the direction is right, and cargoMetadataOutcome reporting mixed/fallback topology is exactly the kind of honesty codemap has been missing. Three things before this lands:

1. Nothing consumes IncompleteScanError, so most of the new vocabulary is unreachable. ScanDirectoryOutcome returns (ScanOutcome{}, err) on any failure, so a ScanOutcome can only ever carry ScanSourceAuthoritative for ast-grep. ScanSourceTimeout, ScanSourceUnavailable, and ScanSourceFailed are constructed and then thrown away with the error. The type you built is the right shape for degrading honestly — returning ScanOutcome{Analyses: nil, Sources: [{ast-grep, timeout}]} so the caller can render "coverage: unavailable" — but the code hard-errors instead. Right now only cargo-metadata actually reports fallback provenance.

2. It duplicates the vocabulary #96 introduces. scanner.ScanSourceStatus and analysis.SourceStatus have byte-identical enums (authoritative/mixed/fallback/timeout/unavailable/failed), and scanner.ScanSourceOutcome{Source,Status,Detail} is analysis.Source{Name,Status,Detail} renamed. If #96's analysis package is meant to be the shared contract, this should use it rather than define a parallel one in scanner. Worth settling before both land.

3. Fail-closed on timeout is a UX cliff for the hook path. Hooks run codemap on every edit; previously an ast-grep timeout warned on stderr and degraded to tree-only output, now the whole command errors. That's defensible under "don't lie", but it trades a partial answer for no answer on exactly the large repos where the timeout fires. (2) would let you keep both.

Minor: dropping the if len(out) == 0 { return nil, nil } guard removes the documented "sg scan returns non-zero if no matches" path. I checked against the bundled ast-grep — zero matches exits 0 with [], so this is fine today; just noting the comment it was guarding is now stale.

Heads-up on ordering: this and #93 both rewrite ScanForDepsWithFilters and the ScanDirectory error block in incompatible ways. See my note on #93.

@reneleonhardt

Copy link
Copy Markdown
Contributor Author

The model shot itself in the foot 😅
Are we allowed to consolidate the new merge order #93#96#97#99 into one PR branch resolving all conflicts and your questions and suggestions, closing those four previous PRs?
But it would mean one big review for you...

@JordanCoin

Copy link
Copy Markdown
Owner

The model shot itself in the foot 😅 Are we allowed to consolidate the new merge order #93#96#97#99 into one PR branch resolving all conflicts and your questions and suggestions, closing those four previous PRs? But it would mean one big review for you...

Yes, please consolidate #93#96#97#99 into one replacement PR based on current main, and close the old PRs after the replacement is open and cross-linked.
Please keep the branch reviewable as four logical commits in that order—don’t squash them yet—and resolve the feedback as one coherent design:
one context-aware scan API rather than Context/WithFilters/Outcome variants multiplying;
one shared provenance vocabulary from the analysis contract, not duplicate scanner enums;
cancellation must stop subprocesses and return cancellation;
unavailable, timed-out, or failed optional analyzers should return an honest degraded outcome rather than making hook output disappear;
JS/TS workspace resolution should consume that final API, avoid widening filtered scans across the entire repository, and normalize paths consistently on Windows;
preserve the structured MCP output now on main.
Include a short migration/design note in the PR body and run the full test, race, vet, Staticcheck, and build suites. The review will be large, but resolving these together is cleaner than merging mutually incompatible intermediate APIs.

@reneleonhardt

reneleonhardt commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

I fear the model can't preserve 4 separate commits, that's kind of the problem.
I will ask if it can split something similar into 4 new commits afterwards if possible, and feed your detailed instructions.

Latest news after 1.5h:

The reviewers found additional overlapping integration gaps—the reason none of the four could merge independently. I’ve now switched all three user-facing render paths to accept the already-built graph, and am validating the remaining filter, fallback-deduplication, and stdin-coverage boundaries before re-reviewing.

@reneleonhardt
reneleonhardt marked this pull request as draft August 2, 2026 19:20
@reneleonhardt

Copy link
Copy Markdown
Contributor Author

In other news, I burned not only my MacBook with hundreds of builds, but also my Max quota (7 days) in 2 days, I hope those few open PRs get merged so I can post the next wave when I have quota again 😄

reneleonhardt added a commit to reneleonhardt/codemap that referenced this pull request Aug 2, 2026
Consolidate the JordanCoin#93, JordanCoin#96, JordanCoin#97, and JordanCoin#99 maintainer corrections into one compatible scanner, graph, CLI, and MCP contract.\n\nPreserve provenance across recoverable scans and fallbacks, apply configured filters consistently, reuse one dependency graph inventory for coverage and rendering, and make unmatched Rust source coverage explicit.\n\nCo-Authored-By: GPT-5.6 Sol <codex@openai.com>
@JordanCoin

Copy link
Copy Markdown
Owner

Yes — consolidate #93#96#97#99 into one branch and close these four. Please don't burn effort trying to preserve four commits; one coherent branch is genuinely the better outcome here, and I'd rather you spend the quota you have left on the design than on git surgery.

The reason is that these four don't conflict textually so much as semantically, and that's not something a rebase can settle:

So one review is the honest price. Your own note matches what I found: none of the four could merge independently.

Three things I'd like the consolidated branch to settle, since they're the decisions the four were making inconsistently:

  1. One provenance vocabulary. analysis is the natural home if it's meant to be the shared contract; scanner should consume it rather than define a parallel copy.
  2. One scanner entry point. Right now we're heading for WithFilters × Context × Outcome. ScanForDeps(ctx, root, filters) (ScanOutcome, error) collapses all three axes. module codemap has no external importers, so there's no compatibility reason for the XxxContext twins — just take the break.
  3. Fail-closed without losing the degraded path. feat(scanner): Fail closed and report fallback provenance #97 builds ScanSourceTimeout/Unavailable/Failed and then discards them with the error, so no caller can ever observe them — only cargo-metadata actually reports fallback provenance today. Returning ScanOutcome{Analyses: nil, Sources: [{ast-grep, timeout}]} gets you honesty and a usable answer, which matters because hooks run codemap on every edit and a timeout currently turns into a hard error on exactly the large repos where it fires.

Status on your other two, both merged:

  • fix(root): Decouple project and setup selection #94 — thanks for dropping internal/projectpath; keeping InvocationRoots as a value is exactly right for the multi-project MCP server, and logicalRootForPhysical addresses the /tmp/private/tmp canonicalization cleanly. Merged.
  • fix(watch): Track configured file counts separately #95 — merged as-is. I landed the one open point myself in fix(watch): Rebuild the dependency graph after a filter change #101 rather than pushing to your branch, so your commit went in untouched: refreshConfiguredFiles now rebuilds the graph behind the same size guard Start uses, since the "rebuild lazily on restart" the comment referred to didn't exist and one config.json edit was permanently stripping hub data from the running daemon. I also had to retarget TestConfiguredFilterChangeInvalidatesDependencyState, which was asserting the graph stayed nil — details in the PR.

Sorry about the quota, and about the MacBook. Take your time — nothing here is urgent, and there's no rush to get the consolidated branch up before your quota resets.

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.

2 participants