feat: add v0.8 review truth helpers#48
Merged
Merged
Conversation
Owner
Author
|
@coderabbitai review please 🥕 |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughAdds structural CLI commands (test-coverage, dead-symbols, symbol history, review cooldown), corresponding MCP tools and schemas, structural parsing for GraphQL/Python, provenance-hint collection in git-graft-enhance, renderers, operations for coverage and cooldown, and comprehensive unit/playback tests. ChangesMulti-Feature CLI and MCP Expansion
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant CLI as Command Parser
participant Op as Operation / Tool Logic
participant MCP as MCP Tool
participant WARP as WARP Index / Git
participant FS as File System
participant Render as Renderer
User->>CLI: struct test-coverage --src src --tests test
CLI->>Op: structuralTestCoverageMap(opts)
Op->>FS: read source/test files, parse outlines
Op->>MCP: (local search) run rg/grep via ProcessRunner
Op->>Op: aggregate per-symbol refs -> covered/uncovered
Op-->>Render: StructuralTestCoverageMapResult
Render-->>User: human-readable or JSON output
User->>CLI: struct dead-symbols --limit 100
CLI->>MCP: invoke graft_dead_symbols(maxCommits)
MCP->>WARP: findDeadSymbols(warp, maxCommits)
WARP-->>MCP: removed symbols
MCP-->>Render: structured response
Render-->>User: dead-symbols report
User->>CLI: review cooldown --comments-file comments.json --now T
CLI->>Op: load comments, reviewCooldownStatus(comments, now)
Op->>Op: parse markers, compute expiry/remaining/status
Op-->>Render: ReviewCooldownStatus
Render-->>User: human-readable or JSON cooldown status
User->>CLI: git-graft-enhance ...
CLI->>Op: collect provenance candidates
Op->>MCP: invoke graft_blame per candidate
MCP-->>Op: blame histories
Op->>Op: build provenanceHints, pass to model
Op-->>Render: enhanced graft model output
Render-->>User: enhanced report with provenance hints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 2d3ca4fa25
ℹ️ 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
Validation: |
Owner
Author
|
@coderabbitai review please 🥕 |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/contracts/output-schema-mcp.ts (1)
409-415: 💤 Low valueConsider constraining
tickto an integer.
tick: z.number()accepts floats, but a tick is a discrete ordinal position in commit history — floats are semantically meaningless here.z.number().int().nonnegative()would match the field's intent exactly.♻️ Proposed fix
- tick: z.number(), + tick: z.number().int().nonnegative(),🤖 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 409 - 415, The history array's object schema currently uses tick: z.number(), which allows floats; update the tick property in the history schema (the z.object(...) inside history: z.array(...).strict()) to z.number().int().nonnegative() so ticks are constrained to non-negative integers; keep the surrounding schema and .strict() unchanged.test/unit/cli/structural-blame-render.test.ts (1)
4-39: ⚡ Quick winAdd a test case for the empty-history branch.
The
elsebranch inrenderStructuralBlame(whenmodel.history.length === 0) is a completely distinct, reachable code path that currently has no unit coverage. The renderer outputs a specific "unavailable" sentinel line in this case.🧪 Suggested additional test case
+ it("renders 'unavailable' timeline when history is empty", () => { + const rendered = renderStructuralBlame({ + symbol: "deletedFn", + filePath: "src/lib.ts", + changeCount: 0, + createdInCommit: null, + lastSignatureChange: null, + referenceCount: 0, + history: [], + }); + + expect(rendered).toContain("Graft Symbol History"); + expect(rendered).toContain("created: unknown"); + expect(rendered).toContain("last signature change: none"); + expect(rendered).toContain("- unavailable: symbol was not found in the indexed WARP graph"); + });🤖 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/cli/structural-blame-render.test.ts` around lines 4 - 39, Add a unit test that calls renderStructuralBlame with a symbol-blame model whose history is an empty array (e.g., same shape as in existing test but history: [] and counts as appropriate) to cover the else branch where model.history.length === 0; assert the rendered output contains the specific "unavailable" sentinel line the renderer emits for empty history (and other key header fields like symbol and path) so the empty-history code path in renderStructuralBlame is covered.src/cli/structural-test-coverage-render.ts (1)
4-8: 💤 Low valuePrefer
z.strictObject()over.extend().strict()in Zod v4.In Zod v4,
.strict()and.passthrough()still work, but the preferred approach is to use the top-level constructorsz.strictObject({...})andz.looseObject({...})when defining new schemas. The Zod v4 docs also note that after calling.extend(), you can change the strictness level of the resulting schema by usingz.strictObject()orz.looseObject()— meaning spread-based construction is the idiomatic path.♻️ Idiomatic Zod v4 equivalent
-const structuralTestCoverageRenderSchema = mcpOutputBodySchemas.graft_test_coverage.extend({ - _schema: z.unknown().optional(), - _receipt: z.unknown().optional(), - tripwire: z.unknown().optional(), -}).strict(); +const structuralTestCoverageRenderSchema = z.strictObject({ + ...mcpOutputBodySchemas.graft_test_coverage.shape, + _schema: z.unknown().optional(), + _receipt: z.unknown().optional(), + tripwire: z.unknown().optional(), +});🤖 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/cli/structural-test-coverage-render.ts` around lines 4 - 8, Replace the current use of mcpOutputBodySchemas.graft_test_coverage.extend(...).strict() by constructing a strict object via z.strictObject(...) that merges the original graft_test_coverage shape with the additional keys; specifically, build structuralTestCoverageRenderSchema using z.strictObject({ ...mcpOutputBodySchemas.graft_test_coverage.shape, _schema: z.unknown().optional(), _receipt: z.unknown().optional(), tripwire: z.unknown().optional() }) (referencing structuralTestCoverageRenderSchema and mcpOutputBodySchemas.graft_test_coverage to locate the code).src/operations/structural-test-coverage-map.ts (1)
228-287: 🏗️ Heavy liftSerial synchronous subprocess per symbol is a linear event-loop bottleneck.
runReferenceSearchis synchronous and called once per exported symbol inside the sequentialforloop (line 321). For a project with 100 exported symbols this spawns 100 blocking rg subprocesses one after another, while the enclosingstructuralTestCoverageMapisasyncand runs inside an MCP tool handler that is supposed to yield the event loop.A significantly cheaper path is a single
rginvocation with one-eflag per symbol, then group results by symbol name:♻️ Sketch: batch all symbols in a single rg pass
-for (const sourceFile of sourceFiles) { - const symbols: StructuralTestCoverageSymbol[] = []; - for (const symbol of sourceFile.symbols) { - const refs = runReferenceSearch(opts.cwd, opts.process, symbol.name, codeTestFiles); - ... - } -} +// Collect every unique symbol name across all source files +const allSymbolNames = [...new Set( + sourceFiles.flatMap((f) => f.symbols.map((s) => s.name)) +)]; + +// Single rg run: one -e per symbol +const batchArgs = [ + "--no-heading", "--line-number", "--with-filename", "-F", "--color", "never", + ...allSymbolNames.flatMap((name) => ["-e", name]), + "--", ...codeTestFiles, +]; +const batchResult = opts.process.run({ + command: "rg", + args: batchArgs, + cwd: opts.cwd, + timeoutMs: REFERENCE_SEARCH_TIMEOUT_MS, + maxBufferBytes: REFERENCE_SEARCH_MAX_BUFFER_BYTES, +}); +// parse batchResult.stdout and bucket lines by matched symbol nameAlternatively, expose an
asyncvariant ofrunReferenceSearchand fan out withPromise.allif theProcessRunnersupports concurrent execution.Also applies to: 318-338
🤖 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/operations/structural-test-coverage-map.ts` around lines 228 - 287, runReferenceSearch currently runs a blocking ripgrep/grep subprocess per symbol (function runReferenceSearch) which serializes calls and blocks the event loop; refactor by either (A) batching symbols into a single ripgrep invocation (build one rg command with multiple -e <symbol> patterns created from the array of symbols passed in, parse stdout and group matches by symbol to return ReferenceSearchResult per symbol) or (B) convert runReferenceSearch to an async variant and call it concurrently (change signature of runReferenceSearch to async, use the ProcessRunner's non-blocking run API or spawn/promisified execution, then fan out Promise.all for all symbols from structuralTestCoverageMap) and apply the same approach to the grep fallback; ensure you update parseMatchedFiles/countSearchLines usage to support grouped results and preserve the same return shape for each symbol.test/unit/cli/git-graft-enhance-render.test.ts (1)
34-60: ⚡ Quick winAdd coverage for non-empty provenance hint rendering.
The new renderer path (
model.provenanceHints.length > 0) is currently untested. A focused assertion for both available and unavailable entries would harden this feature.Suggested test addition
describe("cli: git graft enhance renderer", () => { + it("renders provenance hints when present", () => { + const rendered = renderGitGraftEnhance({ + ...model(), + provenanceHints: [ + { + symbol: "updateUser", + filePath: "src/api.ts", + changeKind: "changed", + ambiguous: true, + status: "available", + createdInCommit: "abc123", + lastSignatureChange: "def456", + referenceCount: 2, + changeCount: 3, + }, + { + symbol: "legacyUser", + filePath: "src/old.ts", + changeKind: "removed", + ambiguous: false, + status: "unavailable", + reason: "not_indexed_or_not_found", + }, + ], + }); + + expect(rendered).toContain("Provenance hints"); + expect(rendered).toContain("src/api.ts: updateUser changed"); + expect(rendered).toContain("ambiguous symbol name"); + expect(rendered).toContain("src/old.ts: legacyUser removed unavailable (not_indexed_or_not_found)"); + }); + it("renders a deterministic human review summary from the model only", () => {🤖 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/cli/git-graft-enhance-render.test.ts` around lines 34 - 60, Add a unit test calling renderGitGraftEnhance with a model prepared by model() but with provenanceHints populated (e.g. one hint with name/path and metadata present, and another hint marked unavailable or missing fields) to exercise the model.provenanceHints.length > 0 path; assert the rendered output contains the provenance heading (e.g. "provenance:"), the available hint's identifying text (name/path or URL), and a clear marker for unavailable entries (e.g. "unavailable" or "not found") so both present and missing-provenance cases are validated; place this alongside the existing tests that call renderGitGraftEnhance and reference model() and renderGitGraftEnhance to locate where to add the new it(...) block.test/unit/cli/git-graft-enhance-model.test.ts (1)
107-168: ⚡ Quick winTest intent and assertion scope are mismatched.
This case is named as candidate-selection/ambiguity logic, but it only verifies passthrough of
provenanceHintsinto the model. Please split this into (1) passthrough test and (2) direct collector-behavior test so ambiguity/limit logic is actually exercised.Proposed test refactor
-import { buildGitGraftEnhanceModel } from "../../../src/cli/git-graft-enhance-model.js"; +import { + buildGitGraftEnhanceModel, + collectGitGraftEnhanceProvenanceCandidates, +} from "../../../src/cli/git-graft-enhance-model.js"; -it("selects bounded provenance candidates and marks duplicate symbol names ambiguous", () => { +it("passes provenance hints through the model unchanged", () => { const model = buildGitGraftEnhanceModel({ // existing fixture... }); expect(model.provenanceHints).toEqual([ expect.objectContaining({ symbol: "shared", filePath: "src/a.ts", ambiguous: true, status: "available", }), ]); }); +it("collects provenance candidates with ambiguity marking and limit", () => { + const candidates = collectGitGraftEnhanceProvenanceCandidates({ + base: "HEAD~1", + head: "HEAD", + summary: "changed", + layer: "ref_view", + files: [ + { + path: "src/a.ts", + status: "modified", + summary: "src/a.ts | modified | ~1 changed", + diff: { added: [], removed: [], changed: [{ name: "shared" }], unchangedCount: 0 }, + }, + { + path: "src/b.ts", + status: "modified", + summary: "src/b.ts | modified | -1 removed", + diff: { added: [], removed: [{ name: "shared" }], changed: [], unchangedCount: 0 }, + }, + ], + }, 1); + + expect(candidates).toHaveLength(1); + expect(candidates[0]).toEqual(expect.objectContaining({ + symbol: "shared", + ambiguous: true, + })); +});🤖 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/cli/git-graft-enhance-model.test.ts` around lines 107 - 168, The test currently only verifies passthrough of provenanceHints via buildGitGraftEnhanceModel but is named to exercise candidate-selection/ambiguity logic; split it into two tests: (1) a simple passthrough test that constructs the model with provenanceHints and asserts model.provenanceHints contains the expected object (keep the existing expect block), and (2) a direct collector-behavior test that calls the actual collector/selection function used by buildGitGraftEnhanceModel (locate the collector or selection helper used inside that function) with representative file diffs and symbols to assert bounded-candidate selection and that duplicate symbol names are marked ambiguous; ensure you reference the same symbols ("shared", file paths like "src/a.ts"/"src/b.ts") and assert the collector output (not just model.provenanceHints) for ambiguity and candidate limits.
🤖 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/operations/review-cooldown-status.ts`:
- Around line 66-68: isCodeRabbitComment currently treats undefined authors as
CodeRabbit; change it to require an explicit author match by returning
CODERABBIT_AUTHOR_PATTERN.test(comment.author) only when comment.author is
defined (i.e., return false if author is undefined). For opt-in flexibility, add
an optional parameter (e.g., trustUnknown: boolean = false) to
isCodeRabbitComment so callers can opt into the previous behavior; update
callers of isCodeRabbitComment and any tests to pass trustUnknown where
fixture-driven paths intentionally omit author. Ensure references:
isCodeRabbitComment, ReviewCooldownComment, and CODERABBIT_AUTHOR_PATTERN.
---
Nitpick comments:
In `@src/cli/structural-test-coverage-render.ts`:
- Around line 4-8: Replace the current use of
mcpOutputBodySchemas.graft_test_coverage.extend(...).strict() by constructing a
strict object via z.strictObject(...) that merges the original
graft_test_coverage shape with the additional keys; specifically, build
structuralTestCoverageRenderSchema using z.strictObject({
...mcpOutputBodySchemas.graft_test_coverage.shape, _schema:
z.unknown().optional(), _receipt: z.unknown().optional(), tripwire:
z.unknown().optional() }) (referencing structuralTestCoverageRenderSchema and
mcpOutputBodySchemas.graft_test_coverage to locate the code).
In `@src/contracts/output-schema-mcp.ts`:
- Around line 409-415: The history array's object schema currently uses tick:
z.number(), which allows floats; update the tick property in the history schema
(the z.object(...) inside history: z.array(...).strict()) to
z.number().int().nonnegative() so ticks are constrained to non-negative
integers; keep the surrounding schema and .strict() unchanged.
In `@src/operations/structural-test-coverage-map.ts`:
- Around line 228-287: runReferenceSearch currently runs a blocking ripgrep/grep
subprocess per symbol (function runReferenceSearch) which serializes calls and
blocks the event loop; refactor by either (A) batching symbols into a single
ripgrep invocation (build one rg command with multiple -e <symbol> patterns
created from the array of symbols passed in, parse stdout and group matches by
symbol to return ReferenceSearchResult per symbol) or (B) convert
runReferenceSearch to an async variant and call it concurrently (change
signature of runReferenceSearch to async, use the ProcessRunner's non-blocking
run API or spawn/promisified execution, then fan out Promise.all for all symbols
from structuralTestCoverageMap) and apply the same approach to the grep
fallback; ensure you update parseMatchedFiles/countSearchLines usage to support
grouped results and preserve the same return shape for each symbol.
In `@test/unit/cli/git-graft-enhance-model.test.ts`:
- Around line 107-168: The test currently only verifies passthrough of
provenanceHints via buildGitGraftEnhanceModel but is named to exercise
candidate-selection/ambiguity logic; split it into two tests: (1) a simple
passthrough test that constructs the model with provenanceHints and asserts
model.provenanceHints contains the expected object (keep the existing expect
block), and (2) a direct collector-behavior test that calls the actual
collector/selection function used by buildGitGraftEnhanceModel (locate the
collector or selection helper used inside that function) with representative
file diffs and symbols to assert bounded-candidate selection and that duplicate
symbol names are marked ambiguous; ensure you reference the same symbols
("shared", file paths like "src/a.ts"/"src/b.ts") and assert the collector
output (not just model.provenanceHints) for ambiguity and candidate limits.
In `@test/unit/cli/git-graft-enhance-render.test.ts`:
- Around line 34-60: Add a unit test calling renderGitGraftEnhance with a model
prepared by model() but with provenanceHints populated (e.g. one hint with
name/path and metadata present, and another hint marked unavailable or missing
fields) to exercise the model.provenanceHints.length > 0 path; assert the
rendered output contains the provenance heading (e.g. "provenance:"), the
available hint's identifying text (name/path or URL), and a clear marker for
unavailable entries (e.g. "unavailable" or "not found") so both present and
missing-provenance cases are validated; place this alongside the existing tests
that call renderGitGraftEnhance and reference model() and renderGitGraftEnhance
to locate where to add the new it(...) block.
In `@test/unit/cli/structural-blame-render.test.ts`:
- Around line 4-39: Add a unit test that calls renderStructuralBlame with a
symbol-blame model whose history is an empty array (e.g., same shape as in
existing test but history: [] and counts as appropriate) to cover the else
branch where model.history.length === 0; assert the rendered output contains the
specific "unavailable" sentinel line the renderer emits for empty history (and
other key header fields like symbol and path) so the empty-history code path in
renderStructuralBlame is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee022c6e-96d0-4dd4-812c-d34d7f29d109
⛔ Files ignored due to path filters (11)
CHANGELOG.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mddocs/CLI.mdis excluded by!**/*.mddocs/MCP.mdis excluded by!**/*.mddocs/design/CORE_structural-test-coverage-map.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_structural-test-coverage-map.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/SURFACE_git-graft-enhance-provenance-hints.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/SURFACE_review-cooldown-status.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/WARP_dead-symbol-detection.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/WARP_symbol-history-timeline.mdis excluded by!**/*.mddocs/three-surface-capability-matrix.mdis excluded by!**/*.md
📒 Files selected for processing (37)
src/cli/cli-error.tssrc/cli/command-parser.tssrc/cli/dead-symbols-render.tssrc/cli/git-graft-enhance-model.tssrc/cli/git-graft-enhance-render.tssrc/cli/git-graft-enhance.tssrc/cli/main.tssrc/cli/peer-command.tssrc/cli/review-cooldown.tssrc/cli/structural-blame-render.tssrc/cli/structural-test-coverage-render.tssrc/contracts/capabilities.tssrc/contracts/output-schema-cli.tssrc/contracts/output-schema-mcp.tssrc/contracts/output-schemas.tssrc/mcp/burden.tssrc/mcp/tool-registry.tssrc/mcp/tools/dead-symbols.tssrc/mcp/tools/structural-test-coverage.tssrc/operations/projection-safety.tssrc/operations/review-cooldown-status.tssrc/operations/structural-test-coverage-map.tstest/unit/cli/command-parser.test.tstest/unit/cli/dead-symbols-render.test.tstest/unit/cli/git-graft-enhance-model.test.tstest/unit/cli/git-graft-enhance-render.test.tstest/unit/cli/structural-blame-render.test.tstest/unit/cli/structural-test-coverage-render.test.tstest/unit/contracts/capabilities.test.tstest/unit/contracts/output-schemas.test.tstest/unit/operations/review-cooldown-status.test.tstest/unit/operations/structural-test-coverage-map.test.tstests/playback/0078-three-surface-capability-baseline-and-parity-matrix.test.tstests/playback/CORE_structural-test-coverage-map.test.tstests/playback/SURFACE_review-cooldown-status.test.tstests/playback/WARP_dead-symbol-detection.test.tstests/playback/WARP_symbol-history-timeline.test.ts
Owner
Author
|
Review feedback resolution summary:
Validation after |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/operations/structural-test-coverage-map.test.ts (1)
55-186: ⚡ Quick winAdd regressions for symbol-name collision cases.
Great coverage additions. One gap remains: there’s no test for
foovsfoobar(substring collision) or same export name in different source files. Adding these would lock in correctness of coverage attribution.🤖 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/operations/structural-test-coverage-map.test.ts` around lines 55 - 186, Add two new unit tests to this suite that exercise symbol-name collisions: one where one file exports "foo" and another exports "foobar" to ensure structuralTestCoverageMap only marks the intended "foo" as referenced when tests reference "foo" (use createTestRepo, write files under src and test similar to writeCoverageScenario, call structuralTestCoverageMap and assert per-file symbol statuses and totals), and another where two different source files export the same name (e.g., both export "sharedApi") to verify references are attributed to the correct source file (create both src files, create a test that references only one, then assert that the referenced file shows covered status and the other remains uncovered). Ensure tests also cover batching behavior (use RecordingReferenceSearchRunner) and that totals reflect distinct exportedSymbols and covered/uncovered counts.
🤖 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/operations/review-cooldown-status.ts`:
- Around line 77-87: The current filtering/sorting of markerComments silently
treats timestamp === null as 0 so untimestamped markers are ignored; change the
logic in the marker collection (the block that builds markerComments using
isCodeRabbitComment, RATE_LIMIT_PATTERN, timestampForComment, and
durationMsFromBody) to first detect any marker with timestamp === null and, if
any exist, immediately return the "unknown" status (or otherwise surface
ambiguity) instead of continuing to sort; ensure the same guard is added to the
equivalent logic later (the similar block around the code that computes latest
and uses latest.timestamp/durationMs) so untimestamped markers no longer get
treated as older than timestamped ones.
In `@src/operations/structural-test-coverage-map.ts`:
- Around line 364-376: The current coverage lookup uses only symbol.name (built
into symbolNames and read from referenceResults returned by
runReferenceSearches), which conflates same-named exports across files; change
the keying to a disambiguated identifier (e.g., combine sourceFile path/module
with symbol.name such as `${sourceFile.path}::${symbol.name}` or an export
unique id) when building the symbolNames array passed into runReferenceSearches
and when later looking up refs for each symbol in the loop (replace
referenceResults.get(symbol.name) with referenceResults.get(disambiguatedKey)).
Update any types or StructuralTestCoverageSymbol/StructuralTestCoverageFile
population to map back to the original name and file so coverage still reports
per-file symbols.
- Around line 267-273: The current loop uses string.includes(line.content) which
can falsely match when one symbol is a substring of another; replace that
substring check with an identifier-boundary regex: create or reuse an
escapeRegExp helper to safely escape symbolName, then build a RegExp like new
RegExp(`(?<![A-Za-z0-9_$])${escapedName}(?![A-Za-z0-9_$])`) and use
regex.test(line.content) instead of includes; update the existing loop over
parseMatchedReferenceLines(stdout) / symbolNames and keep the same counts.set
and fileSets.get(symbolName)?.add(line.filePath) logic when the regex matches.
---
Nitpick comments:
In `@test/unit/operations/structural-test-coverage-map.test.ts`:
- Around line 55-186: Add two new unit tests to this suite that exercise
symbol-name collisions: one where one file exports "foo" and another exports
"foobar" to ensure structuralTestCoverageMap only marks the intended "foo" as
referenced when tests reference "foo" (use createTestRepo, write files under src
and test similar to writeCoverageScenario, call structuralTestCoverageMap and
assert per-file symbol statuses and totals), and another where two different
source files export the same name (e.g., both export "sharedApi") to verify
references are attributed to the correct source file (create both src files,
create a test that references only one, then assert that the referenced file
shows covered status and the other remains uncovered). Ensure tests also cover
batching behavior (use RecordingReferenceSearchRunner) and that totals reflect
distinct exportedSymbols and covered/uncovered counts.
🪄 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
Run ID: 21ea1c1a-b0db-4e8c-8794-a792851f909f
⛔ Files ignored due to path filters (24)
CHANGELOG.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mddocs/method/backlog/dependency-dag.dotis excluded by!**/*.dotdocs/method/backlog/dependency-dag.svgis excluded by!**/*.svgdocs/method/backlog/v0.8.0/CORE_c-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_cpp-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_csharp-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_go-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_hcl-structured-config.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_java-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_json-structured-config.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_jupyter-notebook-structure.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_kotlin-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_php-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_python-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_ruby-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_shell-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_sql-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_swift-structural-parsing.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_toml-structured-config.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/CORE_yaml-structured-config.mdis excluded by!**/*.mddocs/method/backlog/v0.8.0/README.mdis excluded by!**/*.mddocs/public-api.mdis excluded by!**/*.mdpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
package.jsonsrc/cli/structural-test-coverage-render.tssrc/contracts/output-schema-mcp.tssrc/contracts/output-schemas.tssrc/operations/review-cooldown-status.tssrc/operations/structural-test-coverage-map.tssrc/parser/extractors/graphql.tssrc/parser/extractors/index.tssrc/parser/extractors/python.tssrc/parser/lang.tssrc/parser/runtime.tssrc/parser/types.tssrc/ports/semantic-enrichment.tstest/fixtures/graphql/continuum-runtime-boundary-family.graphqltest/fixtures/python/agent_service.pytest/unit/cli/git-graft-enhance-model.test.tstest/unit/cli/git-graft-enhance-render.test.tstest/unit/cli/structural-blame-render.test.tstest/unit/operations/review-cooldown-status.test.tstest/unit/operations/structural-test-coverage-map.test.tstest/unit/parser/lang.test.tstest/unit/parser/outline.test.tstests/playback/CORE_git-graft-enhance.test.ts
✅ Files skipped from review due to trivial changes (5)
- package.json
- test/unit/cli/git-graft-enhance-render.test.ts
- test/fixtures/python/agent_service.py
- test/unit/cli/structural-blame-render.test.ts
- test/unit/operations/review-cooldown-status.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/cli/git-graft-enhance-model.test.ts
- src/cli/structural-test-coverage-render.ts
- src/contracts/output-schema-mcp.ts
Owner
Author
|
Review feedback resolution summary:
Validation after @coderabbitai review please 🥕 |
|
Review rate limit: 0/1 reviews remaining, refill in 10 minutes and 37 seconds. |
flyingrobots
commented
May 5, 2026
Owner
Author
flyingrobots
left a comment
flyingrobots
left a comment
There was a problem hiding this comment.
Publishing remaining review-thread reply for f5143f4 feedback fixes.
flyingrobots
commented
May 5, 2026
Owner
Author
flyingrobots
left a comment
flyingrobots
left a comment
There was a problem hiding this comment.
Publishing review-thread replies for f5143f4 feedback fixes.
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
Stacks five v0.8 Review Truth helpers on
cycle/CORE_structural-test-coverage-map:graft_test_coverage/graft struct test-coveragestructural-reference coverage mapping.git graft enhance --sinceusinggraft_blamewhere WARP history is available.graft symbol historyas a timeline-first renderer over the existing provenance-backed symbol blame model.graft_dead_symbols/graft struct dead-symbolsfor removed-and-not-readded WARP history signals.graft review cooldownto calculate CodeRabbit review cooldown readiness from PR comments or fixtures.User Impact
This pushes the v0.8 lane toward Review Truth: reviewers can see what changed structurally, where changed symbols came from, which removed symbols stayed gone, which exports have obvious structural test references, and whether the automated review loop is ready for another pass.
Docs
Updated README, CLI/MCP docs, CHANGELOG, the three-surface capability matrix, and the affected v0.8 backlog cards.
Validation
git diff --checkpnpm vitest run test/unit/cli/command-parser.test.ts test/unit/cli/structural-blame-render.test.ts test/unit/cli/dead-symbols-render.test.ts test/unit/operations/review-cooldown-status.test.ts tests/playback/WARP_symbol-history-timeline.test.ts tests/playback/WARP_dead-symbol-detection.test.ts tests/playback/SURFACE_review-cooldown-status.test.ts test/unit/contracts/output-schemas.test.ts test/unit/contracts/capabilities.test.ts tests/playback/0078-three-surface-capability-baseline-and-parity-matrix.test.tspnpm typecheckpnpm lintpnpm vitest run test/unit/cli/git-graft-enhance-model.test.ts test/unit/cli/git-graft-enhance-render.test.ts tests/playback/CORE_git-graft-enhance.test.ts test/integration/cli/git-graft-enhance-cli.test.tspnpm test(Docker isolated): 216 files / 1568 tests passingSummary by CodeRabbit