Skip to content

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

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

feat: doc-staleness context — mention-based reverse-reference scan#10
aliasunder wants to merge 34 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
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
…tecture

Conflict resolution: adopt main's #12/#23 architecture (invisible review
marker, standalone finding comments, one status comment; requestBotReview
removed) and re-home this PR's contextNotes onto buildStatusComment, the
sole narration surface. relatedDocs context gathering grafted unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VB6oNyPpHLhYjXRXJCxkav
@umm-actually

umm-actually Bot commented Jul 19, 2026

Copy link
Copy Markdown

[high/correctness] Verify symlink targets are within workspace bounds in scan file reads (confidence: high)

src/context/workspace.ts:183 — beyond the diff's line ranges, in code the changes touch or depend on.

readScannedFileOrNull reads file content without symlink safety checks. The BFS walk includes symlinks to files (Dirent.isFile() resolves through symlinks), and this function follows them with readFile without verifying the real path is within the workspace. A malicious PR can create a doc file symlinked to .git/config or any file outside the workspace, and its content reaches the prompt via findRelatedDocs or findRelatedFiles. This is pre-existing for the source scan and new in the doc scan where the attack surface is broader (mention matching accepts any path reference, not just imports).

Failure scenario: A PR adds docs/release-notes.md as a symlink to .git/config (which contains the checkout token under persist-credentials). The file's templated content mentions src/greeter.ts (a changed file). findRelatedDocs matches it, reads .git/config content, and includes it in the LLM prompt — which is echoed in the public PR review.

Suggested fix
Apply the same `realPathIfSafe` check used in `readChangedFileOrNull` and `readPriorityDocOrNull` before reading scanned file content. Degrade to a logged skip when the real path escapes the workspace or enters `.git/`.

@umm-actually

umm-actually Bot commented Jul 19, 2026

Copy link
Copy Markdown

umm-actually re-reviewed at 9ba778d

2 new finding(s) posted (13 tracked finding(s) across all runs).

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

umm-actually · deepseek/deepseek-v4-pro

aliasunder and others added 3 commits July 18, 2026 21:10
…escriptions

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VB6oNyPpHLhYjXRXJCxkav
Add 7 tests covering the contextNotes feature that had zero test coverage:
- buildStatusComment: renders collapsible <details> section with context
  notes, omits section when empty, omits when param not provided
- orchestrate: constructs context notes for skipped priority docs,
  relatedFiles cap exclusions, relatedDocs cap exclusions, and all three
  combined; threads them into the status comment

All 3 code paths mutation-tested (removing each contextNotes.push breaks
exactly its covering tests). 379 tests, lint clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VB6oNyPpHLhYjXRXJCxkav
The warning message said "excluding from related files" which was
accurate when only used by the source-file scanner. Now that
findRelatedDocs also calls it for doc files, "excluding from context"
is correct for both callers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VB6oNyPpHLhYjXRXJCxkav
Comment thread .coderabbit.yaml
Comment thread README.md
Comment thread src/context/__tests__/workspace.test.ts
Comment thread src/context/doc-mentions.ts
Comment thread src/orchestrate.ts
Comment thread src/review/__tests__/phases.test.ts Outdated
Comment thread src/review/__tests__/prompt.test.ts
@umm-actually

umm-actually Bot commented Jul 19, 2026

Copy link
Copy Markdown

[medium/correctness] Add well-formedness check for doc content in prompt rendering (confidence: high)

src/review/__tests__/prompt.test.ts:204 — beyond the diff's line ranges, in code the changes touch or depend on.

The isWellFormed() check in the prompt test only applies to the conventions block's surrogate-splitting test. It does not guard priority docs or mention-matched docs from surrogate splitting. A user prompt containing a surrogate-split doc string would be malformed but pass all tests.

Failure scenario: A priority doc with an astral emoji placed such that buildUserPrompt truncates between its UTF-16 code units would produce a malformed user prompt string (.isWellFormed() returns false) yet no test validates isWellFormed() on the full user prompt output when relatedDocs or priority docs are present.

Suggested fix
Add an `isWellFormed()` assertion on the full user prompt output in a test that includes `relatedDocs` with surrogate-splitting content, or add the check to `buildUserPrompt` itself and test it there.

Mirrors readChangedFiles: skip a priority doc whose byte size exceeds the
remaining character budget without pulling it into memory.
@aliasunder

Copy link
Copy Markdown
Owner Author

Re: umm-actually beyond-diff finding — symlink targets in scan file reads

False positive — the premise "Dirent.isFile() resolves through symlinks" is incorrect. readdir with withFileTypes: true derives dirent types from the directory entry itself (lstat semantics): a symlink — whether its target is a file or a directory — reports isSymbolicLink() === true and isFile() === false. The BFS walk's if (!entry.isFile()) continue at src/context/workspace.ts therefore excludes every symlink from the scan list, so readScannedFileOrNull can never be handed a symlinked path — the described docs/release-notes.md → .git/config attack never reaches the read. This is the same lstat-semantics rebuttal as the earlier BFS directory-symlink finding on this PR.


🔍 ship-check · pr-monitor · claude-fable-5

Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts
Comment thread src/context/workspace.ts
@aliasunder

Copy link
Copy Markdown
Owner Author

Re: umm-actually beyond-diff finding — well-formedness check for doc content

False positive — the premise ("buildUserPrompt truncates a doc between its UTF-16 code units") describes a code path that does not exist. The only truncation in the prompt path is truncateToTokenCap, applied solely to the conventions block, and it already restores well-formedness via .toWellFormed() (covered by the surrogate-pair test). Priority docs, mention-matched docs, and changed/related files are never truncated: every budget check (estimateTokens post-read, and the stat-based byte guards) includes a file whole or skips it whole, so no doc string can be surrogate-split.


🔍 ship-check · pr-monitor · claude-fable-5

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