Skip to content

feat(impact): codemap impact — symbol/file blast-radius walker (impl of PR #49 plan)#50

Merged
SutuSebastian merged 7 commits intomainfrom
feat/impact-engine
May 3, 2026
Merged

feat(impact): codemap impact — symbol/file blast-radius walker (impl of PR #49 plan)#50
SutuSebastian merged 7 commits intomainfrom
feat/impact-engine

Conversation

@SutuSebastian
Copy link
Copy Markdown
Contributor

@SutuSebastian SutuSebastian commented May 3, 2026

Summary

Implementation of PR #49's codemap impact plan. Single verb walks the calls / dependencies / imports graphs (callers, callees, dependents, dependencies) — replaces the "agent composes WITH RECURSIVE by hand" tax that's the highest-friction loop left after watch (#47) shipped.

Three transports, one engine: CLI (codemap impact <target>), MCP (impact tool), HTTP (POST /tool/impact). All three dispatch the same pure findImpact engine in application/impact-engine.ts per the post-PR #41 layering.

Tracers

  1. application/impact-engine.ts — pure walker. One WITH RECURSIVE per (direction, backend) combo, JS-side merge + dedup + summary. Cycle detection via path-string instr check (per plan §D6). 27 unit tests cover symbol + file targets, up/down/both, depth caps, limit caps, self-loop + 3-cycle, --via skipped_backends, case-sensitive name match.
  2. cmd-impact.ts — CLI verb mirroring cmd-show.ts shape. Parser for --direction / --via / --depth / --limit / --summary / --json. Wired into main.ts + bootstrap.ts + printCliUsage. 14 parser tests.
  3. MCP impact toolimpactArgsSchema + handleImpact in tool-handlers.ts; registerImpactTool one-line wrapper in mcp-server.ts. 5 integration tests (tools/list, symbol target, file target, summary trim semantics, Zod rejects float depth).
  4. HTTP POST /tool/impactTOOL_NAMES gains impact; one switch arm in dispatchTool with Zod validation. 4 integration tests (happy path, missing target, non-int depth, bad direction enum).
  5. Docs sync — README, docs/architecture.md (new "Impact wiring" section), docs/glossary.md, docs/research/fallow.md adjacent-shipped bullet, .agents/ + templates/agents/ rule + skill (Rule 10 lockstep), changeset, plan deleted (docs-governance Rule 3).

Smoke

```bash
$ bun src/index.ts impact handleQuery --json --summary | jq '.summary'
{
"nodes": 65,
"max_depth_reached": 3,
"by_kind": { "symbol": 65 },
"terminated_by": "depth"
}

$ bun src/index.ts impact src/db.ts --direction up --depth 1

impact file:src/db.ts direction=up via=dependencies+imports depth=1

[1] up depended_on_by src/api.ts
[1] up depended_on_by src/application/audit-engine.ts
... (real fan-in across the codebase)
```

Decisions worth knowing (full set in changeset)

  • Target auto-resolution. Contains / or matches files.path → file; otherwise symbol. Symbol → walks calls; file → walks dependencies + imports. Mismatched explicit --viaskipped_backends (no error).
  • Cycle detection. SQLite has no native predicate; comma-bounded path string + instr check inside the recursive CTE. Bounded depth + --limit (default 500) keep cyclic graphs cheap.
  • Termination classification. summary.terminated_by: limit > depth > exhausted.
  • --summary shape. Trims matches for transport bandwidth but preserves summary.nodes count — the jq '.summary.nodes' CI-gate pattern still works.
  • No SARIF / annotations. Impact rows are graph traversals, not findings.

Test plan

  • CodeRabbit review for the WITH RECURSIVE SQL shape, dedup logic, termination classification heuristic
  • CI green on all 666 + 50 = 716 tests

Summary by CodeRabbit

  • New Features
    • Added codemap impact command for blast-radius analysis of symbols and files across dependency and call graphs
    • Supports configurable traversal direction (up/down/both), depth limiting, and cycle detection
    • Outputs available in JSON and trimmed summary modes
    • Accessible via CLI command, HTTP endpoint (POST /tool/impact), and MCP tool interface

application/impact-engine.ts — single WITH RECURSIVE per (direction, backend),
JS-side merge + dedup + summary. Backends:

- calls (symbol-level, up=callers / down=callees)
- dependencies (file-level)
- imports (file-level, resolved_path only)

Plan §D2 backend split, §D6 cycle detection (path-string + instr), §D8 edge
tags. 27 unit tests cover: walks (up/down/both), depth caps, limit caps,
self-loop + 3-cycle, file-vs-symbol resolution, --via skipped_backends.
…cer 2)

- src/cli/cmd-impact.ts — parser + runner mirroring cmd-show.ts shape
- main.ts dispatch, bootstrap.ts validateIndexModeArgs + printCliUsage
- 14 parser tests cover happy paths + 9 error cases (--depth/limit/direction/via validation)

Smoke: `bun src/index.ts impact src/db.ts --direction up --depth 1` lists
the real fan-in on this repo (every file in src/ that depends on db.ts).
`bun src/index.ts impact handleQuery --json --summary` returns the summary
shape ready for jq pipelines.
- impactArgsSchema + handleImpact in tool-handlers.ts (Zod + ToolResult shape)
- registerImpactTool in mcp-server.ts (one-line wrapper, like show/snippet)
- 5 MCP integration tests (tools/list, symbol target, file target, summary
  trims matches but keeps summary.nodes count, Zod rejects float depth)

HTTP POST /tool/impact lights up automatically next tracer via the existing
http-server.ts dispatcher (one switch arm).
- TOOL_NAMES gains 'impact' (auto-listed in /tools)
- dispatchTool switch arm with Zod validation (400 on bad body)
- 4 integration tests: happy path, missing target, non-int depth, bad direction enum

Both transports (MCP + HTTP) now expose impact via the same handleImpact
pure function.
…l + changeset (Tracer 5)

- README.md: new 'Impact analysis' code block + impact in MCP tools list
- docs/architecture.md: 'Impact wiring' section + impact-engine in application/ inventory
- docs/glossary.md: 'codemap impact / impact tool' entry under I (full envelope spec)
- docs/roadmap.md: drop the impact backlog item (it shipped)
- docs/research/fallow.md: Adjacent-shipped bullet referencing PR #49 (plan) + #50 (impl)
- .agents/rules/codemap.md + templates/agents/rules/codemap.md: new table row + 'Impact' paragraph + impact in MCP tools list (Rule 10 lockstep)
- .agents/skills/codemap/SKILL.md + templates/agents/skills/codemap/SKILL.md: impact tool description (Rule 10 lockstep)
- .changeset/codemap-impact.md: minor — full design rationale + decisions
- docs/plans/impact.md: deleted per docs-governance Rule 3 (canonical descriptions now live in architecture / glossary / README)
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 3, 2026

🦋 Changeset detected

Latest commit: 8762100

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@stainless-code/codemap Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 3, 2026

Warning

Rate limit exceeded

@SutuSebastian has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 42 minutes and 17 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 134373df-2eeb-4e7d-bac1-66e44327880e

📥 Commits

Reviewing files that changed from the base of the PR and between 97de830 and 8762100.

📒 Files selected for processing (2)
  • src/application/impact-engine.test.ts
  • src/application/impact-engine.ts
📝 Walkthrough

Walkthrough

The pull request implements and ships the codemap impact <target> feature—a cycle-detected, depth-bounded symbol and file blast-radius walker. The feature includes a shared graph traversal engine (findImpact), CLI command, MCP tool, HTTP endpoint, comprehensive test coverage, and documentation updates across architecture/glossary/README and agent rules.

Changes

Impact Feature Launch

Layer / File(s) Summary
Type Definitions
src/application/impact-engine.ts (lines 1–68)
Defines ImpactDirection, ImpactBackend, ImpactTargetKind, ImpactNode, ImpactTarget, ImpactTermination, ImpactSummary, and ImpactResult types for impact traversal queries and result envelopes.
Core Engine Implementation
src/application/impact-engine.ts (lines 70–397)
findImpact() resolves targets (symbol vs file), selects compatible graph backends (dependencies, calls, imports), executes recursive CTEs with cycle detection via path strings, enforces depth/row limits, deduplicates results, and returns an envelope with terminated_by classification and per-kind node counts.
Tool Handler & Schema
src/application/tool-handlers.ts (lines 661–704)
Exports impactArgsSchema, ImpactArgs interface, and handleImpact() function; normalizes --summary by clearing matches when requested, delegates to findImpact, and wraps results in a ToolResult envelope.
CLI Command Implementation
src/cli/cmd-impact.ts (lines 1–295)
Implements parseImpactRest() for flag/argument validation, printImpactCmdHelp() for usage output, and runImpactCmd() to orchestrate DB initialization, engine invocation, optional terminal rendering, and error handling with JSON-or-stderr output.
CLI & MCP Wiring
src/cli/main.ts (lines 223–248), src/cli/bootstrap.ts (lines 42–78), src/application/mcp-server.ts (lines 24–269)
CLI main() dispatches to impact subcommand; bootstrap validates impact as an index-mode entry; MCP server registers impact tool via registerImpactTool() with schema validation and result wrapping.
HTTP Server Routing
src/application/http-server.ts (lines 1–92, 461–469)
HTTP server imports impact handler/schema, adds "impact" to TOOL_NAMES, and routes POST /tool/impact requests through schema validation to handleImpact().
Engine Tests
src/application/impact-engine.test.ts (lines 1–395)
Validates findImpact behavior: symbol/file target resolution, traversal direction (up/down/both) and backend selection, depth/limit/cycle semantics, call-site navigation metadata, and result envelope structure (summary node counts, termination reason, deduplication).
HTTP & MCP Tests
src/application/http-server.test.ts (lines 262–427), src/application/mcp-server.test.ts (lines 951–1056)
HTTP tests verify envelope shape, response codes, and input validation; MCP tests check tool listing, traversal results, summary-mode trimming, and schema error handling.
CLI Tests
src/cli/cmd-impact.test.ts (lines 1–117)
Validates parseImpactRest() parsing, help output, flag defaults, invalid options, boundary conditions (--depth 0, missing target, etc.).
Architecture & Glossary Documentation
docs/architecture.md (lines 17–131), docs/glossary.md (lines 218–221)
Updated layering and key-files tables; added "Impact wiring" section explaining target resolution, backend/direction mapping, cycle/termination handling, and format constraints; added glossary entry for codemap impact tool.
README & Agent Documentation
README.md (lines 149–161), .agents/rules/codemap.md (lines 33–66), templates/agents/rules/codemap.md (lines 38–70)
Added command usage examples, MCP tool listing, agent rule documentation with parameter/output schema, and impact section describing transport-agnostic behavior (CLI/MCP/HTTP dispatch same engine).
Changelog & Planning
.changeset/codemap-impact.md, docs/plans/impact.md, docs/roadmap.md (line 36–38), docs/research/fallow.md (line 37)
Added changeset documenting feature transport and implementation details; removed plan and backlog entries; added fallow-period documentation noting feature post-refresh delivery.
Skill Documentation
.agents/skills/codemap/SKILL.md, templates/agents/skills/codemap/SKILL.md
Added MCP tool impact specification: input parameters, output envelope structure, and SARIF/annotation non-support.

Sequence Diagram

sequenceDiagram
    participant Client as Client (CLI/MCP/HTTP)
    participant Handler as Tool Handler
    participant Engine as findImpact Engine
    participant DB as CodemapDatabase
    
    Client->>Handler: impact(target, direction, via, depth, limit)
    Handler->>Engine: findImpact(db, opts)
    
    Engine->>DB: resolveTarget(target)
    DB-->>Engine: ImpactTarget (kind, name, file_path[])
    
    Engine->>Engine: resolveBackends(via, target.kind)
    Engine-->>Engine: backend[], skipped[]
    
    loop for each (direction, backend)
        Engine->>DB: walk() recursive CTE<br/>(calls|dependencies|imports)
        DB-->>Engine: ImpactNode[] (with depth, edge_type)
    end
    
    Engine->>Engine: deduplicate by<br/>(direction, kind, name, file_path)
    Engine->>Engine: compute terminated_by<br/>(limit vs depth vs exhausted)
    Engine->>Engine: summarize by_kind counts
    Engine-->>Handler: ImpactResult
    
    alt summary mode
        Handler->>Handler: clear matches, keep summary
    end
    
    Handler-->>Client: JSON envelope<br/>(target, matches, summary)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • PR #49: Introduced docs/plans/impact.md plan document describing intended codemap impact behavior; this PR realizes that design spec with engine, CLI/MCP/HTTP wiring, and comprehensive tests.
  • PR #44: Refactored MCP and HTTP tool-handler registration patterns; this PR reuses the same transport-agnostic dispatch infrastructure to add the impact tool.
  • PR #35: Introduced MCP/HTTP tool infrastructure; this PR leverages the established tool-handler and schema-validation patterns introduced there.

Suggested labels

enhancement

Poem

🐇 Through graphs the rabbit leaps with care,
Calls and dependencies everywhere,
Cycles caught in compass deep,
Symbols traced where matches sleep,
Blast-radius whispers, "Impact's here!" ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.74% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat(impact): codemap impact — symbol/file blast-radius walker (impl of PR #49 plan)' accurately captures the main change: implementing the impact analysis feature (a blast-radius walker for both symbols and files) as specified in the plan from PR #49.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/impact-engine

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 17 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Re-read every comment authored in this PR; cut/slim ones a teammate could
re-derive in <30s. Targets:

- impact-engine.ts: drop trivial type doc, slim findImpact JSDoc, slim
  termination heuristic from 6 lines → 2, drop redundant inline notes
- cmd-impact.ts + tool-handlers.ts: shorten the --summary trim explanation

No behaviour change; checks green (89 tests + lint + typecheck).
@SutuSebastian
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 3, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (5)
src/application/impact-engine.ts (1)

281-331: 💤 Low value

Consider documenting the empty file_path fallback behavior.

The lookupSymbolFile fallback (line 320) is a good addition for cross-module callees. However, when the lookup also fails (line 327), the fallback to an empty string "" could be unexpected for consumers. While this is better than null/undefined for type safety, consider adding a brief comment explaining this edge case.

💡 Suggested comment
     const symFile = r.file_path ?? lookupSymbolFile(db, r.node);
     const node: ImpactNode = {
       depth: r.depth,
       direction: opts.direction,
       edge,
       kind: "symbol",
       name: r.node,
+      // Empty string when symbol not found in index (external/dynamic calls)
       file_path: symFile ?? "",
     };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/impact-engine.ts` around lines 281 - 331, Add a brief inline
comment in the walkCalls function near the fallback that sets file_path to an
empty string (where symFile ?? "" is used) explaining that lookupSymbolFile(db,
r.node) may return null for cross-module symbols and we intentionally use an
empty string for type-safety and predictable consumer behavior (navigation
absent), so callers should treat "" as "no known file path". Reference symbols:
walkCalls, lookupSymbolFile, ImpactNode.file_path.
src/cli/cmd-impact.ts (1)

268-275: 💤 Low value

Redundant skipped_backends !== undefined guard — field is required in ImpactResult.

ImpactResult.skipped_backends is typed as a required (non-optional) array, so the !== undefined check is always true. The code is functionally correct (an empty array produces no output), but the guard implies optionality that doesn't exist and may confuse future readers.

♻️ Suggested fix
-  if (result.skipped_backends !== undefined) {
-    for (const s of result.skipped_backends) {
+  for (const s of result.skipped_backends) {
       console.error(`# skipped backend "${s.backend}": ${s.reason}`);
-    }
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/cmd-impact.ts` around lines 268 - 275, The check
`result.skipped_backends !== undefined` is redundant because
ImpactResult.skipped_backends is a required array; remove the guard and iterate
directly over `result.skipped_backends` (e.g., `for (const s of
result.skipped_backends) { ... }`). Update the block around the `result` usage
so it assumes `skipped_backends` is always an array and let an empty array
naturally produce no output; reference `ImpactResult.skipped_backends` and the
`result` variable when making the change.
src/application/tool-handlers.ts (1)

663-679: 💤 Low value

impactArgsSchema and ImpactArgs lack documentation per coding guidelines.

As per coding guidelines, "All public APIs must have accompanying documentation." Both impactArgsSchema and ImpactArgs are newly exported public symbols without JSDoc.

📝 Suggested doc additions
+/** Zod schema for the `impact` MCP/HTTP tool — mirrors `FindImpactOpts` plus the
+ *  transport-level `summary` trim flag. */
 export const impactArgsSchema = {
   target: z.string().min(1, "target must be a non-empty string"),
   ...
 };

+/** Validated args passed to {`@link` handleImpact}. */
 export interface ImpactArgs {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/tool-handlers.ts` around lines 663 - 679, Add JSDoc for the
exported impactArgsSchema and ImpactArgs to satisfy public API documentation
rules: for impactArgsSchema, document that it validates impact query options
(describe each key: target - non-empty string, direction - "up" | "down" |
"both" optional, via - "dependencies"|"calls"|"imports"|"all" optional, depth -
non-negative int optional, limit - positive int optional, summary - boolean
optional) and mention it uses Zod; for the ImpactArgs interface, add a JSDoc
comment describing the shape and purpose of each property and reference the
related types ImpactDirection and ImpactBackend where applicable, including any
default behaviors if used elsewhere.
src/application/impact-engine.test.ts (2)

203-210: ⚡ Quick win

Strengthen the exact-fit limit test with a positive terminated_by assertion.

The test asserts only that terminated_by is not "limit". If a future regression caused the engine to return "depth" instead of "exhausted" for a fully-explored graph, this test would silently pass.

✅ Proposed improvement
  expect(r.matches).toHaveLength(3);
- expect(r.summary.terminated_by).not.toBe("limit");
+ expect(r.summary.terminated_by).toBe("exhausted");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/impact-engine.test.ts` around lines 203 - 210, The test for
findImpact currently only asserts that r.summary.terminated_by is not "limit",
which is weak; instead update the assertion to check the exact expected
termination reason (e.g., expect(r.summary.terminated_by).toBe("exhausted")) to
ensure the engine reports the correct completion state for a fully-explored
graph; locate the test using the findImpact call and its r.summary.terminated_by
assertion and replace the negative check with a positive equality check to the
intended terminal value.

280-288: Prefer order-insensitive assertion for the via array since backend order is an implementation detail.

expect(r.via).toEqual(["dependencies", "imports"]) is order-sensitive but the engine only guarantees which backends apply to file targets, not their order. The hardcoded fileBackends array at line 221 of impact-engine.ts determines order, which is an implementation choice rather than a contractual requirement. Consider using:

- expect(r.via).toEqual(["dependencies", "imports"]);
+ expect(r.via.slice().sort()).toEqual(["dependencies", "imports"]);

This test will remain stable if the backend enumeration order is ever refactored without changing behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/impact-engine.test.ts` around lines 280 - 288, The assertion
on r.via is order-sensitive but backend order is an implementation detail;
update the test case "via=all on file target uses dependencies + imports (skips
calls)" to assert membership rather than exact order by using an
order-insensitive check (e.g.,
expect(r.via).toEqual(expect.arrayContaining(["dependencies","imports"])) plus
an assertion on length or use a Set comparison) and keep the skipped_backends
assertion as-is; locate the test using findImpact and the fileBackends/impact
behavior references to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/application/impact-engine.test.ts`:
- Around line 203-210: The test for findImpact currently only asserts that
r.summary.terminated_by is not "limit", which is weak; instead update the
assertion to check the exact expected termination reason (e.g.,
expect(r.summary.terminated_by).toBe("exhausted")) to ensure the engine reports
the correct completion state for a fully-explored graph; locate the test using
the findImpact call and its r.summary.terminated_by assertion and replace the
negative check with a positive equality check to the intended terminal value.
- Around line 280-288: The assertion on r.via is order-sensitive but backend
order is an implementation detail; update the test case "via=all on file target
uses dependencies + imports (skips calls)" to assert membership rather than
exact order by using an order-insensitive check (e.g.,
expect(r.via).toEqual(expect.arrayContaining(["dependencies","imports"])) plus
an assertion on length or use a Set comparison) and keep the skipped_backends
assertion as-is; locate the test using findImpact and the fileBackends/impact
behavior references to make this change.

In `@src/application/impact-engine.ts`:
- Around line 281-331: Add a brief inline comment in the walkCalls function near
the fallback that sets file_path to an empty string (where symFile ?? "" is
used) explaining that lookupSymbolFile(db, r.node) may return null for
cross-module symbols and we intentionally use an empty string for type-safety
and predictable consumer behavior (navigation absent), so callers should treat
"" as "no known file path". Reference symbols: walkCalls, lookupSymbolFile,
ImpactNode.file_path.

In `@src/application/tool-handlers.ts`:
- Around line 663-679: Add JSDoc for the exported impactArgsSchema and
ImpactArgs to satisfy public API documentation rules: for impactArgsSchema,
document that it validates impact query options (describe each key: target -
non-empty string, direction - "up" | "down" | "both" optional, via -
"dependencies"|"calls"|"imports"|"all" optional, depth - non-negative int
optional, limit - positive int optional, summary - boolean optional) and mention
it uses Zod; for the ImpactArgs interface, add a JSDoc comment describing the
shape and purpose of each property and reference the related types
ImpactDirection and ImpactBackend where applicable, including any default
behaviors if used elsewhere.

In `@src/cli/cmd-impact.ts`:
- Around line 268-275: The check `result.skipped_backends !== undefined` is
redundant because ImpactResult.skipped_backends is a required array; remove the
guard and iterate directly over `result.skipped_backends` (e.g., `for (const s
of result.skipped_backends) { ... }`). Update the block around the `result`
usage so it assumes `skipped_backends` is always an array and let an empty array
naturally produce no output; reference `ImpactResult.skipped_backends` and the
`result` variable when making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bc53c33a-49b8-4d0d-a649-197be78e3748

📥 Commits

Reviewing files that changed from the base of the PR and between c76f09a and 97de830.

📒 Files selected for processing (22)
  • .agents/rules/codemap.md
  • .agents/skills/codemap/SKILL.md
  • .changeset/codemap-impact.md
  • README.md
  • docs/architecture.md
  • docs/glossary.md
  • docs/plans/impact.md
  • docs/research/fallow.md
  • docs/roadmap.md
  • src/application/http-server.test.ts
  • src/application/http-server.ts
  • src/application/impact-engine.test.ts
  • src/application/impact-engine.ts
  • src/application/mcp-server.test.ts
  • src/application/mcp-server.ts
  • src/application/tool-handlers.ts
  • src/cli/bootstrap.ts
  • src/cli/cmd-impact.test.ts
  • src/cli/cmd-impact.ts
  • src/cli/main.ts
  • templates/agents/rules/codemap.md
  • templates/agents/skills/codemap/SKILL.md
💤 Files with no reviewable changes (2)
  • docs/roadmap.md
  • docs/plans/impact.md

- impact-engine.ts:333 — one-line comment explaining empty-string file_path
  fallback (external/dynamic call sites)
- impact-engine.test.ts:209 — strengthen exact-fit limit test from
  not.toBe('limit') to toBe('exhausted')

Pushed back on nitpicks 2+3 (hallucinated — see PR comment) and 5 (style;
deterministic order is fine to lock).
@SutuSebastian
Copy link
Copy Markdown
Contributor Author

Triaged the 5 nitpicks per pr-comment-fact-check.md:

Applied (2):

  • ✅ Nitpick 1 — impact-engine.ts empty-string file_path fallback comment. Real edge case, one-line cost. → 8762100.
  • ✅ Nitpick 4 — impact-engine.test.ts exact-fit limit test strengthened from not.toBe("limit")toBe("exhausted"). → 8762100.

Pushed back (2 — hallucinated):

  • ❌ Nitpick 2 — "skipped_backends !== undefined guard is redundant; field is required."
    The field is optional (skipped_backends?: Array<...> in impact-engine.ts:64) and only set when skipped.length > 0 (impact-engine.ts:171). Removing the guard would crash on `for (const s of undefined)` for the common `via=all` case.
  • ❌ Nitpick 3 — "All public APIs must have accompanying documentation."
    No such rule exists in .agents/ — verified via grep. The 11 sibling *ArgsSchema / *Args exports in tool-handlers.ts (queryArgsSchema, queryRecipeArgsSchema, queryBatchArgsSchema, auditArgsSchema, contextArgsSchema, validateArgsSchema, saveBaselineArgsSchema, listBaselinesArgsSchema, dropBaselineArgsSchema, showArgsSchema, snippetArgsSchema) all follow the same no-JSDoc convention because the Zod calls + tool registry descriptions in mcp-server.ts are the documentation. Adding JSDoc to impactArgsSchema only would be inconsistent — and our concise-comments rule explicitly cuts comments a teammate could re-derive in <30s.

Declined (1 — style):

  • 💭 Nitpick 5 — via array order assertion. Backend order is deterministic from the fileBackends literal in impact-engine.ts; downstream consumers (terminal renderer + JSON envelope) iterate in document order. Locking the order in the test makes any future enumeration change a deliberate decision, which is the value of the assertion.

@SutuSebastian SutuSebastian merged commit 90092ae into main May 3, 2026
10 checks passed
@SutuSebastian SutuSebastian deleted the feat/impact-engine branch May 3, 2026 10:54
SutuSebastian added a commit that referenced this pull request May 3, 2026
…(v1.x backlog) (#51)

* docs(plan): codemap audit --base <ref> — worktree + reindex strategy (v1.x backlog)

Plan for the next-best agent-value loop: PR-review structural-diff. Replaces today's 3-step --baseline dance with one verb. Reuses 90% of the existing audit infrastructure (PR #33); only new piece is the worktree+reindex snapshot path.

Cache-by-resolved-sha; LRU 5/500 MiB; mutual-exclusive with --baseline; per-delta override compatible. Hard error on non-git projects (no graceful fallback — there's no meaningful 'ref' without git).

Plan only — implementation follows after CodeRabbit review per the impact (#49#50) / watch (#46#47) workflow.

* docs(plan): address CodeRabbit findings — atomic cache populate (D11), worktree-as-cache lifecycle clarity, TS type widening callout

- D1/D2/D8 rewritten: worktree IS the cache entry (kept until LRU evicts);
  cleanup runs only on reindex failure rollback OR LRU eviction. The
  earlier ambiguity (D2 said 'cache by sha' while D8 said 'remove in
  finally') is resolved.
- D11 added: atomic cache populate via per-pid temp dir + POSIX rename
  → free single-flight semantics; no lock files needed. Same pattern
  for eviction. Closes the race CodeRabbit flagged on concurrent
  CI matrix runs against the same sha.
- AuditBase TS type widening to discriminated union called out
  explicitly above the Decisions table (Tracer 1 ships it).
- CODEMAP_AUDIT_CACHE_SIZE env var mention dropped — was promising an
  unimplemented config knob; v1 hardcodes the limits, defer to v1.x+.
SutuSebastian added a commit that referenced this pull request May 3, 2026
…f + 2 nitpicks)

Real bug:
- main.ts: runListBaselinesCmd was being called without stateDir — `codemap query --baselines` would fall back to the default DB instead of the caller-selected one. Fixed.

Stale doc refs:
- audit-worktree.ts: 3 JSDoc strings still said `.codemap.db` after Tracer 1's CACHE_ENTRY_DB_REL move; bumped to `.codemap/index.db`.
- bootstrap.ts: printCliUsage() had two `.codemap.db` refs + missing --state-dir/CODEMAP_STATE_DIR docs in Environment+Options. Fixed.
- config.ts: Zod databasePath.describe() said default was `<root>/.codemap.db`; corrected to `<state-dir>/index.db`.
- .agents/skills + templates skills: 2 hard-coded `.codemap/` refs reworded to `<state-dir>/` with `(default .codemap/)` callout (state-dir is configurable).

Nitpicks applied:
- state-dir.test.ts: dropped redundant `require('node:fs')` for mkdirSync (already imported).
- bootstrap-codemap.ts: consolidated two single-import lines from state-dir into one statement.

Nitpicks declined:
- changeset code-fence missing 'text' lang — purely cosmetic.
- cmd-index.ts JSDoc on runIndexCmd — 'all public APIs need JSDoc' is a fabricated rule (sibling cmds inconsistent; same hallucination rejected on PR #50).
SutuSebastian added a commit that referenced this pull request May 3, 2026
…es (impl of PR #53 plan) (#54)

* feat(state-dir): resolveStateDir + DB at <state-dir>/index.db (Tracer 1)

- application/state-dir.ts: resolveStateDir({root, cliFlag, env}) per plan §D7. Constants STATE_DIR_DEFAULT='.codemap', STATE_DB_NAME='index.db'. 12 unit tests cover precedence + relative/absolute paths.
- config.ts: ResolvedCodemapConfig gains stateDir; resolveCodemapConfig 3rd arg opts.stateDir; databasePath defaults to <stateDir>/index.db. User-supplied databasePath wins (escape hatch).
- config.ts: loadUserConfig reads <state-dir>/config.{ts,js,json} (D8); legacy <root>/codemap.config.* dropped (pre-v1).
- runtime.ts: getStateDir() getter.
- bootstrap.ts: --state-dir + CODEMAP_STATE_DIR; precedence per D7.
- bootstrap-codemap.ts (new): single helper extracts the loadUserConfig+resolveCodemapConfig+initCodemap+configureResolver dance from 9 cmd-* files. Tracer 4's ensureStateDir attaches here.
- All 9 cmd-* files refactored; stateDir threaded through interfaces + main.ts dispatch + ServerOpts (mcp/serve).
- audit-worktree.ts: cached entries at <sha>/.codemap/index.db (recursive layout — each cached worktree is its own self-contained codemap project).
- audit-engine.ts: makeWorktreeReindex stops hard-coding db path; openCodemapDatabase() reads from initialised runtime.
- sqlite-db.ts: openCodemapDatabase mkdirs the parent (state-dir may not exist on fresh project).

Dogfood:
- .codemap/.gitignore (self-managed, blacklist) — codemap repo + fixtures/minimal/
- root .gitignore: dropped .codemap.* and .codemap/audit-cache/ (nested .gitignore handles them)
- fixtures/minimal/.codemap.db* removed (stale legacy)

703 tests pass.

* feat(state-dir): ensureStateGitignore reconciler — self-healing (Tracer 2)

- STATE_GITIGNORE_BODY constant — single source of truth for the canonical blacklist.
- ensureStateGitignore(stateDir) — pure-shape return ({before, after, written}); idempotent (no write on steady state); auto-mkdir; user-edits rewritten back per D11 (file is codemap-managed; header line declares it).
- 5 tests cover: fresh write, idempotent, user-modified, older-version self-heal, returned shape matches disk.

Bumping STATE_GITIGNORE_BODY in a future PR is the entire migration — every consumer's project repairs itself on next codemap run.

* feat(state-dir): ensureStateConfig reconciler (Tracer 3)

* feat(state-dir): bootstrap orchestrator + agents-init delegation (Tracer 4)

* docs(state-dir): sync README + architecture + glossary + agent rule/skill + changeset (Tracer 5)

- README.md: --state-dir flag, config-file location, self-healing callout
- docs/architecture.md: state-dir resolver in src/config.ts intro; bulk .codemap.db → .codemap/index.db; User config section rewritten with self-healing details (ensureStateGitignore + ensureStateConfig from src/application/, single attachment point in cli/bootstrap-codemap.ts)
- docs/glossary.md: new entries for '.codemap/' / <state-dir> / CODEMAP_STATE_DIR; '.codemap/index.db'; '.codemap/.gitignore' / self-healing files
- docs/roadmap.md: drop the consolidated-dir backlog item (it shipped)
- docs/research/fallow.md: Adjacent-shipped bullet referencing PR #53 (plan) + #54 (impl)
- .agents/rules/codemap.md + templates/agents/rules/codemap.md: bulk .codemap.db → .codemap/index.db; one-paragraph addition explaining state-dir + self-healing .gitignore (Rule 10 lockstep)
- .agents/skills/codemap/SKILL.md + templates/agents/skills/codemap/SKILL.md: bulk path update (Rule 10 lockstep)
- .changeset/codemap-dir-consolidation.md: minor — full design rationale + cleanup steps
- docs/plans/codemap-dir-consolidation.md: deleted per docs-governance Rule 3

* docs(state-dir): refresh stale path refs across docs/ + slim self-authored comments

Doc staleness sweep (after Tracer 5):
- docs/glossary.md, docs/agents.md, docs/benchmark.md, docs/why-codemap.md, docs/research/competitive-scan-2026-04.md, docs/research/fallow.md (B.6 row): bulk `.codemap.db` → `.codemap/index.db` everywhere except the intentional 'old → new' migration callouts.
- docs/architecture.md, docs/research/fallow.md, docs/packaging.md: `codemap.config.{ts,json}` → `<state-dir>/config.{ts,js,json}`.
- docs/agents.md § Git: rewritten to describe the self-managed <state-dir>/.gitignore reconciler instead of root-.gitignore patching.
- docs/benchmark.md: 'where the DB lives' updated; manual .gitignore note dropped (reconciler handles it).

Concise-comments sweep on this turn's authored comments:
- src/application/state-config.ts: 2 inline comments slimmed (TS/JS-validation-only and passthrough-rationale).

* fix(state-dir): address CodeRabbit findings (1 inline + 4 outside-diff + 2 nitpicks)

Real bug:
- main.ts: runListBaselinesCmd was being called without stateDir — `codemap query --baselines` would fall back to the default DB instead of the caller-selected one. Fixed.

Stale doc refs:
- audit-worktree.ts: 3 JSDoc strings still said `.codemap.db` after Tracer 1's CACHE_ENTRY_DB_REL move; bumped to `.codemap/index.db`.
- bootstrap.ts: printCliUsage() had two `.codemap.db` refs + missing --state-dir/CODEMAP_STATE_DIR docs in Environment+Options. Fixed.
- config.ts: Zod databasePath.describe() said default was `<root>/.codemap.db`; corrected to `<state-dir>/index.db`.
- .agents/skills + templates skills: 2 hard-coded `.codemap/` refs reworded to `<state-dir>/` with `(default .codemap/)` callout (state-dir is configurable).

Nitpicks applied:
- state-dir.test.ts: dropped redundant `require('node:fs')` for mkdirSync (already imported).
- bootstrap-codemap.ts: consolidated two single-import lines from state-dir into one statement.

Nitpicks declined:
- changeset code-fence missing 'text' lang — purely cosmetic.
- cmd-index.ts JSDoc on runIndexCmd — 'all public APIs need JSDoc' is a fabricated rule (sibling cmds inconsistent; same hallucination rejected on PR #50).
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.

1 participant