Skip to content

feat: doc-staleness context — mention-based reverse-reference scan#10

Open
aliasunder wants to merge 28 commits into
mainfrom
worktree-doc-reverse-reference
Open

feat: doc-staleness context — mention-based reverse-reference scan#10
aliasunder wants to merge 28 commits into
mainfrom
worktree-doc-reverse-reference

Conversation

@aliasunder

@aliasunder aliasunder commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Summary

  • Extend the workspace scan to doc files (.md, .json) so unchanged docs that describe changed code reach the LLM prompt and staleness becomes a flaggable correctness finding
  • Mention matching is analogous to the import-trace scan: full-path substring matches outrank basename-only, and generic basenames (index, main, config, etc.) require a parent directory segment to avoid false positives
  • Separate candidate pool (RELATED_DOCS_MAX = 4) and token budget (consumes what's left after code-related files)

Changes by module

  • src/context/doc-mentions.ts (new) — pure leaf module: findMentionedChangedPaths (mention extraction with generic-basename guard), byMentionRelevance (ranking comparator), DOC_EXTENSIONS, GENERIC_BASENAMES
  • src/context/workspace.ts — extracted shared BFS walker (scanWorkspaceFiles), new findRelatedDocs method on ContextReader, RELATED_DOCS_MAX = 4
  • src/review/prompt.ts — new relatedDocs param, doc section between related files and diff with staleness instruction header
  • src/review/phases.ts — extended DIMENSION_CORRECTNESS_SECURITY with unchanged-doc staleness instruction
  • src/orchestrate.tsrelatedDocs in ReviewContext, budget chain (remaining after related files → doc scan), wiring through to buildUserPrompt
  • AGENTS.md — module map updated
  • README.md — doc-staleness moved from "In progress" to "Shipped"

Test plan

  • 15 new tests in doc-mentions.test.ts — full-path match, basename match, generic guard, ranking, constants
  • 10 new tests in workspace.test.ts — find docs, exclude conventions/changed docs, cap, budget, ranking, binary skip, JSON extension
  • 3 new tests in prompt.test.ts — section ordering with docs, absent when empty, staleness header
  • 1 new test in phases.test.ts — staleness instruction present
  • 5 new tests in orchestrate.test.ts — budget chain, conventionsFile passthrough, relatedDocs in review context, conditional skip
  • npm test — 242 passing (31 new)
  • npm run lint — clean
  • npm run build — clean
  • docker build . — succeeds

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added doc-mention scanning with relevance ranking for documentation staleness detection.
    • Added token-budgeted relatedDocs to the review context and included a related-docs section in prompts, with stronger documentation verification guidance.
    • Added action inputs for priority_docs and configurable scan/related caps.
  • Documentation
    • Updated README and workspace agent structure docs to describe doc-mention scanning and limits.
  • Refactor
    • Unified workspace scanning and improved related-candidate handling (including skipping empty content).
  • Tests
    • Expanded suites for mention matching/ranking, token budgeting, caps/exclusions, priority behavior, and prompt/context wiring.

Extend the workspace scan to doc files (.md, .json) so unchanged docs
that describe changed code reach the prompt and staleness becomes a
flaggable finding. Mention matching is analogous to the import-trace
scan: full-path matches outrank basename-only, and generic basenames
(index, main, config, etc.) require a parent directory segment.

- Extract shared BFS walker parameterized by extension set
- New pure module: doc-mentions.ts (mention extraction + ranking)
- New ContextReader method: findRelatedDocs (separate pool, cap of 4)
- New prompt section between related files and diff with staleness header
- Extend DIMENSION_CORRECTNESS_SECURITY with unchanged-doc instruction
- Budget chain: docs consume remaining tokens after code-related files
- 31 new tests (242 total), lint/build/docker clean

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@sourcery-ai sourcery-ai 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.

Sorry @aliasunder, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds documentation-file scanning for changed-path mentions, selects related docs within token and count limits, passes them through orchestration, renders them in prompts, and adds documentation-staleness review guidance and coverage.

Changes

Related documentation context

Layer / File(s) Summary
Configuration and workspace scanning
action.yml, src/config.ts, src/main.ts, src/context/workspace.ts, src/context/__tests__/*, .coderabbit.yaml, AGENTS.md
Adds configurable scan and related-context limits, priority documentation inputs, shared workspace scanning, and related-file cap handling.
Documentation mention matching
src/context/doc-mentions.ts, src/context/__tests__/doc-mentions.test.ts
Adds full-path and basename mention detection, generic-basename handling, deterministic relevance ordering, and tests for matching behavior.
Workspace documentation discovery
src/context/workspace.ts, src/context/__tests__/workspace.test.ts, fixtures/workspace/docs/*
Adds priority and mention-matched document selection, binary and exclusion filtering, token budgeting, result limits, fixtures, and tests.
Orchestration context wiring
src/orchestrate.ts, src/__tests__/orchestrate.test.ts
Adds relatedDocs to review context, allocates remaining budget after related files, invokes document discovery, and tests the wiring and disabled behavior.
Prompt and review integration
src/review/prompt.ts, src/review/phases.ts, src/review/__tests__/*, README.md
Renders related docs between related files and the diff, adds documentation-staleness instructions, updates tests, and documents the shipped scan behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and correctly highlights the main change: adding doc-staleness context via a mention-based scan.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-doc-reverse-reference

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

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

aliasunder and others added 2 commits July 12, 2026 23:38
Merge main into PR branch to resolve conflicts:
- README.md: remove shipped "review dedup" in-progress item, keep doc-staleness
- orchestrate.test.ts: keep both FindRelatedDocsParams and UpsertSummaryCommentParams types

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The doc-mention scan is already listed under Shipped; the in-progress
item was stale.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/doc-mentions.ts
Comment thread src/context/workspace.ts
…ounter

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Findings beyond the diff

These are in code the changes touch or depend on, outside the diff's line ranges:

  • [medium/correctness] Pre-existing: BFS walk follows symlinked directories outside workspace — src/context/workspace.ts:279 (confidence: high)
    scanWorkspaceFiles uses entry.isDirectory() to decide whether to recurse, which returns true for directory symlinks. A symlink like docs → /etc would be pushed onto the queue and the walk would descend into the external filesystem. This pre-existing issue now affects the doc scan as well as the source scan. The directory-level pruned check catches .name patterns but not symlink targets.
    Failure scenario: A repo contains docs → /etc as a symlink (committed or created by a prior step). The BFS walk enters /etc, reads all files with matching extensions, and includes their contents in the prompt — potentially exposing sensitive host filesystem data.
Suggested fix
Check `entry.isSymbolicLink()` before recursing into directories, or resolve and validate the real path of each directory entry before pushing it onto the BFS queue.

umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
Comment thread src/orchestrate.ts Outdated
@umm-actually

umm-actually Bot commented Jul 13, 2026

Copy link
Copy Markdown

umm-actually re-reviewed at 4400830

1 new finding(s) posted as inline comments (29 total across all reviews).


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/context/__tests__/doc-mentions.test.ts (1)

189-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the complete generic-basename set.

This test still passes when expected entries such as helpers or constants are removed. Assert the whole deterministic Set instead.

As per coding guidelines, “Prefer exact whole-value or whole-call-parameter assertions over loose matchers.”

🤖 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/context/__tests__/doc-mentions.test.ts` around lines 189 - 193, Update
the GENERIC_BASENAMES test to assert the entire Set value against the complete
expected deterministic set, including helpers and constants, rather than
checking individual memberships in a loop. Preserve the test’s coverage of all
expected common stems through one exact whole-value assertion.

Source: Coding guidelines

🤖 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/context/__tests__/workspace.test.ts`:
- Around line 653-677: Strengthen the exclusion tests around
contextReader.findRelatedDocs by adding deterministic fixtures for both the
excluded document and a matching document that must remain, then assert the
complete returned document value with an exact whole-value assertion. Update
both “excludes the conventions file from results” and “excludes changed docs
from results” so they fail if findRelatedDocs returns an empty array, while
preserving the intended exclusions.

In `@src/context/doc-mentions.ts`:
- Around line 44-69: Update the mention matching in the changed-path loop of
src/context/doc-mentions.ts (lines 44-69) to require complete path-like token
boundaries for full paths, generic two-segment paths, and basenames, preventing
matches inside longer suffixes or prefixed filenames while preserving valid
mentions. Add regression tests in src/context/__tests__/doc-mentions.test.ts
(lines 11-137) covering longer path suffixes and prefixed basenames.
- Around line 76-87: Update byMentionRelevance in src/context/doc-mentions.ts
(lines 76-87) to return 0 when a.path === b.path, while preserving the existing
relevance and path ordering. Add an assertion in
src/context/__tests__/doc-mentions.test.ts (lines 175-181) verifying equal
candidate paths compare as zero.

---

Nitpick comments:
In `@src/context/__tests__/doc-mentions.test.ts`:
- Around line 189-193: Update the GENERIC_BASENAMES test to assert the entire
Set value against the complete expected deterministic set, including helpers and
constants, rather than checking individual memberships in a loop. Preserve the
test’s coverage of all expected common stems through one exact whole-value
assertion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 83e78efe-ce8f-405d-ac75-e95d8bb7ce3f

📥 Commits

Reviewing files that changed from the base of the PR and between af70f65 and ede0fa1.

📒 Files selected for processing (15)
  • AGENTS.md
  • README.md
  • fixtures/workspace/docs/api.md
  • fixtures/workspace/docs/config.json
  • fixtures/workspace/docs/internal.md
  • src/__tests__/orchestrate.test.ts
  • src/context/__tests__/doc-mentions.test.ts
  • src/context/__tests__/workspace.test.ts
  • src/context/doc-mentions.ts
  • src/context/workspace.ts
  • src/orchestrate.ts
  • src/review/__tests__/phases.test.ts
  • src/review/__tests__/prompt.test.ts
  • src/review/phases.ts
  • src/review/prompt.ts

Comment thread src/context/__tests__/workspace.test.ts Outdated
Comment thread src/context/doc-mentions.ts
Comment thread src/context/doc-mentions.ts
Fix four test quality issues and add two coverage gap tests:
- Assert full GENERIC_BASENAMES Set instead of partial 5/8 check
- Replace silent-no-op conventions exclusion test with temp workspace
  where AGENTS.md actually mentions the changed path
- Assert exact doc paths on RELATED_DOCS_MAX cap instead of toHaveLength
- Assert full result objects on ranking test instead of path-only fields
- Add dot-prefixed file (stem="") branch coverage for findMentionedChangedPaths
- Add reason overflow suffix (", +N more") coverage for findRelatedDocs

All three mutation-verified: conventions guard, overflow truncation, stem guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
aliasunder and others added 2 commits July 13, 2026 13:45
The flag now gates both import-tracing (caller regressions) and
doc-mention scanning (staleness detection), but the description only
mentioned caller tracing. Also adds relatedDocs to the buildUserPrompt
JSDoc section-order list.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t clamp

Path mention matching now requires the matched substring not be followed by
path-continuation characters — prevents greeter.ts matching inside greeter.tsx.
Comparator returns 0 for equal paths per the sort contract. Doc budget clamped
to Math.max(0, ...) so the invariant is explicit regardless of callee behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@aliasunder

Copy link
Copy Markdown
Owner Author

Re: umm-actually "Findings beyond the diff" — BFS walk follows symlinked directories

The finding claims entry.isDirectory() returns true for directory symlinks, allowing the BFS walk to follow symlinks outside the workspace. This is incorrect for Dirent objects returned by readdir({withFileTypes: true}):

  • Dirent.isDirectory() returns false for symlinks
  • Dirent.isSymbolicLink() returns true for symlinks
  • Both isFile() and isDirectory() return false — symlinks are their own entry type

The BFS walk at scanWorkspaceFiles (workspace.ts:287) checks entry.isDirectory() for recursion — symlinked directories are not recursed into. And entry.isFile() at line 293 filters out symlinked files. The scan is already safe against symlink escapes at both the directory and file level.


🔍 ship-check · pr-monitor · claude-opus-4-6[1m]

Priority docs (default: README.md) are always included in review context
regardless of mention matching — they get first claim on the doc budget
before mention-matched docs fill the remaining slots.

Scan constants (MAX_SCAN_FILES, MAX_SCAN_BYTES, RELATED_FILES_MAX,
RELATED_DOCS_MAX) are now action inputs with the previous hardcoded
values as defaults, giving users control over context scope.

Both related-files and related-docs log when candidates exceed the cap,
listing excluded paths so the user knows what wasn't considered.

Also fixes a vacuous test assertion (excludes-changed-docs used
not.toContain on an empty array) and updates the doc section prompt
header to cover both priority and mention-matched docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
readPriorityDocOrNull now resolves paths through resolveUnderRoot before
reading — same check applied to conventions files and changed files. A
traversal path (../../etc/passwd) is degraded to a warn-and-skip.

Also disables CodeRabbit auto-reviews (on-demand only via @coderabbitai
review command, matching vault-cortex config).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 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/context/workspace.ts`:
- Around line 223-239: Update readPriorityDocOrNull to resolve each priority
document through the existing realPathIfSafe check before reading it, rather
than directly joining resolvedRoot and docPath. Skip and return null when the
resolved path is outside the reviewable workspace tree or otherwise unsafe,
while preserving the existing missing-file and unreadable-file logging behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fb41c15c-ff49-4317-a2cc-6f7cb3631c19

📥 Commits

Reviewing files that changed from the base of the PR and between ede0fa1 and 9123a5a.

📒 Files selected for processing (14)
  • AGENTS.md
  • README.md
  • action.yml
  • src/__tests__/config.test.ts
  • src/__tests__/orchestrate.test.ts
  • src/config.ts
  • src/context/__tests__/doc-mentions.test.ts
  • src/context/__tests__/workspace.test.ts
  • src/context/doc-mentions.ts
  • src/context/workspace.ts
  • src/main.ts
  • src/orchestrate.ts
  • src/review/__tests__/prompt.test.ts
  • src/review/prompt.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • AGENTS.md
  • src/context/doc-mentions.ts
  • src/orchestrate.ts
  • src/context/tests/doc-mentions.test.ts
  • src/review/tests/prompt.test.ts
  • src/review/prompt.ts
  • src/tests/orchestrate.test.ts

Comment thread src/context/workspace.ts

@umm-actually umm-actually 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.

Reviewed — 3 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/doc-mentions.ts Outdated
Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts

@aliasunder aliasunder left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Phase 1: PR Review

1 finding across 1 file.

Verdict: ship-with-minor-fixes


🔍 ship-check · Phase 1 (PR Review) · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts

@aliasunder aliasunder left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Phase 2: Code Quality

4 findings across 2 files.

Category Count
Naming 3
Structure 1
Module conventions 0
Comments 0
Simplicity 0
Error handling 0

🔍 ship-check · Phase 2 (Code Quality) · deepseek/deepseek-v4-pro

Comment thread src/config.ts
Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts Outdated
Comment thread src/context/workspace.ts Outdated

@aliasunder aliasunder left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Phase 3: Test Audit

1 coverage gap across 1 file. All 306 tests pass (18 test files, 0 failures).

Summary

  • Test files audited: 6 (config, orchestrate, doc-mentions, workspace, phases, prompt)
  • New/modified it() blocks reviewed: ~35 across all files
  • Two-bar rule: All new tests pass both bars — no silent no-ops, no wrong-error passes, no early-return passes
  • Assertion quality: Consistent use of toEqual/toBe on deterministic output; substring matching (toContain) used appropriately for large prompt strings and log assertions on non-deterministic fields (error messages, timestamps)
  • Test hygiene: const per test, explicit callback parameter names, no ! assertions, onTestFinished cleanup in temp-workspace tests, beforeEach not abused
  • Coverage regressions: None — no removed tests, no weakened assertions, no .skip'd tests
  • Coverage gaps: 1 (soft) — see inline

🔍 ship-check · Phase 3 (Test Audit) · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts

@aliasunder aliasunder left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Phase 4: Bug Check

3 bugs found across 2 files. All would-fix (trivial, high confidence).

By dimension:

  • D1 (Description mismatch): 1 — "always included" claim for priority docs is contradicted by budget skip
  • D3 (Type safety / Boundary): 1 — dotfiles silently skipped in mention detection due to lastIndexOf(".") edge case
  • D5 (Behavioral consistency): 1 — readPriorityDocOrNull lacks symlink resolution (asymmetric with readConventions)

Confidence: 3 high, 0 medium, 0 low


🔍 ship-check · Phase 4 (Bug Check) · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts Outdated
Comment thread src/context/doc-mentions.ts Outdated
Add realPathIfSafe check to readPriorityDocOrNull — a committed symlink
README.md → /etc/passwd would otherwise leak content into the prompt.
Rename single-letter callback params (s→segment, i→importer, c→candidate).
Add mutation justification comment on let absolutePath. Fix "always
included" doc comment to "first claim on budget". Add tests for unreadable
and symlink-escaped priority docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 2 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts Outdated
Comment thread src/context/doc-mentions.ts Outdated
When lastIndexOf('.') returns -1 (no extension), the stem is now the
full basename instead of a truncated string. This ensures extensionless
generic names like 'index' correctly trigger the parent-directory guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 2 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/doc-mentions.ts Outdated
Comment thread src/context/__tests__/doc-mentions.test.ts
aliasunder and others added 2 commits July 13, 2026 18:35
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The new maxRelatedDocs config field uses requiredPositiveInteger but had
no field-specific rejection test, unlike its companion maxRelatedFiles.
Mutation-verified: boundary change (<=0 to <0) fails the test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/__tests__/doc-mentions.test.ts Outdated
Fix workspace.ts prettier issue from Phase 2 edits. Rename
"matches generic basename with parent directory context" to clarify
it's testing a full-path match, not a basename match.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/doc-mentions.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
Priority docs are explicitly configured by path — they should not be
gated behind a flag meant for heuristic scan features. findRelatedDocs
now runs when either traceRelatedFiles is true or priorityDocs is
non-empty.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
aliasunder and others added 2 commits July 13, 2026 19:44
Priority doc reading and mention-matched doc scanning are separate
concerns — one is explicit (named files, always-on), the other is
heuristic (workspace scan, gated by traceRelatedFiles). Splitting them
into two ContextReader methods lets the orchestrator own the gating
logic and chain the budget cleanly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace explicit === undefined / === null / !== "" checks with
truthy/falsy equivalents where the falsy set is safe (string chars,
nullable path strings, empty-string filtering). Use early-return
guard in isPathContinuation for TypeScript type narrowing. Rename
`doc` callback to `relatedDoc` for consistency with adjacent
changedFile/relatedFile callbacks. Clarify priority_docs and
trace_related_files README descriptions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 2 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/orchestrate.ts
Comment thread src/context/doc-mentions.ts
Tighten the unreadable priority doc error assertion from expect.any(String) to
expect.stringContaining("EACCES"), matching the pattern used by readChangedFiles
tests. Add four gap tests: excludePaths wired from config.priorityDocs, budget
clamped to zero when related files exceed remaining tokens, budget chain from
readPriorityDocs remainder to findRelatedDocs, and custom relatedDocsMax config
honored over the default constant. All three mutation-tested.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Findings beyond the diff

These are in code the changes touch or depend on, outside the diff's line ranges:

  • [high/security] Pre-existing: Scan candidates read through symlinks without verifying the real path stays within the workspace — src/context/workspace.ts:196 (confidence: high)
    readScannedFileOrNull calls readFile on a path built from the workspace BFS without resolving symlinks. A symlinked file pointing outside the workspace or into .git would be read and its content included in the LLM prompt. The new findRelatedDocs inherits this gap. Changed-file and priority-doc paths are already protected by realPathIfSafe.
    Failure scenario: A repository contains a symlink docs/secret.md → /etc/passwd. The workspace BFS finds docs/secret.md, findRelatedDocs reads it without checking the symlink target, and the content of /etc/passwd appears in the prompt and potentially in the public review.
Suggested fix
Apply realPathIfSafe in readScannedFileOrNull so that scan candidate content cannot escape the workspace via symlinks. Degrade to null (skip) when the real path is outside the safe zone, logging a warning as done for changed files.

umm-actually · deepseek/deepseek-v4-pro

The header "Documentation files related to this change" assumes all docs
in the section were selected by mention-matching, but priority docs are
included unconditionally regardless of whether they reference changed
code. Changed to "Documentation that may describe changed code" which is
accurate for both priority docs and mention-matched docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/doc-mentions.ts
…s accumulators

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/review/prompt.ts
The system prompt's staleness trigger clause only mentioned docs
"provided because they reference changed code" — excluding priority
docs from staleness checks. Broadened to cover all unchanged docs in
context regardless of inclusion reason.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/context/__tests__/workspace.test.ts (1)

922-939: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Make the negative-path tests fail on a no-op implementation.

Each test currently passes if its method always returns an empty result. Add a valid companion document and assert the complete result after the excluded candidate is skipped.

  • src/context/__tests__/workspace.test.ts#L922-L939: include a second matching document and assert it remains after README.md is excluded.
  • src/context/__tests__/workspace.test.ts#L1087-L1101: include a smaller priority document and assert it is read after the oversized document is skipped.
  • src/context/__tests__/workspace.test.ts#L1115-L1120: include a valid priority document and assert it is read after the binary document is skipped.

As per coding guidelines, “Every test must fail when the behavior breaks” and must guard against silent no-op passes.

🤖 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/context/__tests__/workspace.test.ts` around lines 922 - 939, Strengthen
the negative-path tests in src/context/__tests__/workspace.test.ts at lines
922-939, 1087-1101, and 1115-1120 so they cannot pass when the implementation
always returns an empty result: add a valid companion document to the
mention-matched test and assert it remains after README.md is excluded; add a
smaller-priority document and assert it is read after the oversized document is
skipped; and add a valid-priority document and assert it is read after the
binary document is skipped. Preserve each test’s existing exclusion behavior
while asserting the complete expected result.

Source: Coding guidelines

🤖 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/context/workspace.ts`:
- Around line 528-535: Update the exclusion logging around rankedCandidates and
relatedDocs so it reports only candidates rejected specifically because the
related-document cap was reached. Track or derive the actual cap-rejected
candidates during selection rather than comparing lengths or slicing
rankedCandidates, ensuring budget-skipped candidates and later included
documents are not reported as excluded.

---

Outside diff comments:
In `@src/context/__tests__/workspace.test.ts`:
- Around line 922-939: Strengthen the negative-path tests in
src/context/__tests__/workspace.test.ts at lines 922-939, 1087-1101, and
1115-1120 so they cannot pass when the implementation always returns an empty
result: add a valid companion document to the mention-matched test and assert it
remains after README.md is excluded; add a smaller-priority document and assert
it is read after the oversized document is skipped; and add a valid-priority
document and assert it is read after the binary document is skipped. Preserve
each test’s existing exclusion behavior while asserting the complete expected
result.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 10080b78-397c-4c69-ae09-d8a2af868c9f

📥 Commits

Reviewing files that changed from the base of the PR and between 24f0823 and 23d4492.

📒 Files selected for processing (9)
  • README.md
  • src/__tests__/orchestrate.test.ts
  • src/config.ts
  • src/context/__tests__/workspace.test.ts
  • src/context/doc-mentions.ts
  • src/context/workspace.ts
  • src/orchestrate.ts
  • src/review/__tests__/prompt.test.ts
  • src/review/prompt.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/review/tests/prompt.test.ts
  • src/review/prompt.ts
  • src/config.ts
  • src/context/doc-mentions.ts
  • src/tests/orchestrate.test.ts

Comment thread src/context/workspace.ts Outdated

@umm-actually umm-actually 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.

Reviewed — 2 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/__tests__/doc-mentions.test.ts
Comment thread src/context/doc-mentions.ts
…gging

hasPathMention now checks the character before the match — src/greeter.ts
no longer matches inside lib/src/greeter.ts. Cap-exceeded logging now
only fires when the cap (not budget) stopped candidate processing,
tracked via a break index.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.


umm-actually · deepseek/deepseek-v4-pro

Comment thread README.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Findings beyond the diff

These are in code the changes touch or depend on, outside the diff's line ranges:

  • [high/correctness] Update findRelatedFiles return type to include excludedByCapPathssrc/context/workspace.ts:27 (confidence: high)
    Pre-existing: The ContextReader interface and createContextReader factory both declare findRelatedFiles returning Promise<PromptFile[]>, but the implementation since the related-docs refactor now returns { files: PromptFile[]; excludedByCapPaths: string[] }. The interface is out of sync with the implementation. Callers using the interface type (e.g. through dependency injection in tests) receive a type mismatch.
    Failure scenario: When the ContextReader interface is used as a type constraint, a stub returning PromptFile[] would pass type checking but the real implementation returns the wrapper object. Consumers that destructure files from the result get undefined (array destructuring on an object), leading to silent failures or undefined is not iterable at runtime.
Suggested fix
Update the `findRelatedFiles` return type in both the `ContextReader` interface and `createContextReader` return type to:
```typescript
findRelatedFiles: (params: {
  changedPaths: string[]
  budgetTokens: number
}) => Promise<{ files: PromptFile[]; excludedByCapPaths: string[] }>
```

umm-actually · deepseek/deepseek-v4-pro

Comment thread src/orchestrate.ts
When docs or files are excluded by the max_related_docs or
max_related_files cap, a collapsible "Context notes" section appears
in the review body listing what was excluded — so users reading the
PR know what the reviewer didn't see without checking workflow logs.

Also surfaces skipped priority docs (missing, unreadable, or over
budget) in the same section.

findRelatedFiles and findRelatedDocs now return RelatedFilesResult
with excludedByCapPaths alongside the files, using accurate
cap-break-index tracking (not conflating budget skips with cap
exclusions).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.

Context notes
  • Priority docs not included: README.md (missing, unreadable, or over budget)

umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts

@umm-actually umm-actually 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.

Reviewed — 1 finding(s) posted as inline comments.

Context notes
  • Priority docs not included: README.md (missing, unreadable, or over budget)

umm-actually · deepseek/deepseek-v4-pro

Comment thread src/context/workspace.ts
Comment on lines +458 to +470
for (const docPath of priorityDocs) {
const content = await readPriorityDocOrNull(docPath)
if (!content) continue
if (content.includes("\x00")) continue
const contentTokens = estimateTokens(content)
if (contentTokens > remainingTokens) continue
remainingTokens -= contentTokens
files.push({
path: docPath,
content,
includedAs: "full",
reason: "priority documentation",
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium/correctness] Add byte-size guard for priority docs before reading (confidence: medium)

readPriorityDocs reads the entire file without checking its size first. A large priority doc can exhaust memory before the token budget check. readChangedFiles already guards against this with a stat-based byte check; readPriorityDocs should do the same.

Failure scenario: A user sets priority_docs to a path pointing to a multi-GB file (e.g., a binary blob). The action reads the entire file into memory, causing an OOM crash or excessive memory usage before the token budget check can skip it.

Suggested fix
Before reading the file, stat it and skip if `fileStats.size > remainingTokens * CHARS_PER_TOKEN`, similar to the guard in `readChangedFiles`.

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