feat(advisor): review repository terminology - #8029
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds semantic terminology review to the PR Review Advisor. It introduces Git-backed tracing, validated terminology receipts, workflow integration, lane comparison, and output rendering. It also updates Hermes and security documentation to use PR SHA terminology instead of exact-head wording. ChangesPR Review Advisor terminology review
PR-SHA validation guidance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PRReviewAdvisor
participant traceTerminology
participant TerminologyLedger
participant CorrectnessAnalysis
participant SecurityAnalysis
participant CommentPublisher
PRReviewAdvisor->>traceTerminology: Trace selected terms against base and head
traceTerminology-->>PRReviewAdvisor: Return terminology evidence
PRReviewAdvisor->>TerminologyLedger: Commit validated terminology decisions
TerminologyLedger-->>PRReviewAdvisor: Return canonical receipt
PRReviewAdvisor->>CorrectnessAnalysis: Provide terminology receipt
PRReviewAdvisor->>SecurityAnalysis: Provide terminology receipt
CorrectnessAnalysis-->>PRReviewAdvisor: Return analysis
SecurityAnalysis-->>PRReviewAdvisor: Return analysis
PRReviewAdvisor->>CommentPublisher: Publish normalized terminology results
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8029.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 04bd421 in the TypeScript / code-coverage/cliThe overall coverage in commit 04bd421 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/pr-review-advisor/comment.mts (1)
811-847: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAbsent terminology fields weaken the PR-SHA binding in the publisher. Both sites treat a missing value as acceptable, so a lane artifact without PR-SHA-bound terminology evidence still reaches the published comment.
tools/pr-review-advisor/comment.mts#L811-L847: reject a missingterminologyReviewintrustedLaneStructure, or name the rollout consumer that needs the tolerance; also addterminology?: TrustedLaneTerminologyDecision[]to the declared return type so the fingerprint and lane data stay typed.tools/pr-review-advisor/comment.mts#L632-L643: requiretypeof result?.headSha === "string"inrenderTerminologyDetailsbefore you compare it withdecision.source.headSha, so an undefined-to-undefined match cannot pass.🤖 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 `@tools/pr-review-advisor/comment.mts` around lines 811 - 847, Strengthen terminology PR-SHA validation in tools/pr-review-advisor/comment.mts#L811-L847 by making trustedLaneStructure reject absent terminologyReview and adding the optional terminology field to its declared return type. In tools/pr-review-advisor/comment.mts#L632-L643, update renderTerminologyDetails to require result?.headSha to be a string before comparing it with decision.source.headSha, preventing undefined values from matching.
🧹 Nitpick comments (6)
test/pr-review-advisor-terminology.test.ts (1)
101-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding stage-gating negative-path coverage.
This test drives the trace, update, and read tools through their correct stage sequence and verifies the concrete-contrast rejection path. The controller in
terminology.mtsalso throws whenTERMINOLOGY_TRACE_TOOLruns outside"terminology-review-analysis"orTERMINOLOGY_UPDATE_TOOLruns outside"terminology-review". Add a small negative test that calls these tools beforesetStagereaches the expected value, to confirm the stage guard actually rejects the call through the publicexecute()boundary.Based on path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."🤖 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/pr-review-advisor-terminology.test.ts` around lines 101 - 182, The terminology tool tests currently cover only valid stage sequencing; add negative-path coverage for the public execute() boundary. In the test using createTerminologyToolController, invoke TERMINOLOGY_TRACE_TOOL before setting "terminology-review-analysis" and TERMINOLOGY_UPDATE_TOOL before setting "terminology-review", asserting each call rejects with the stage-guard error while preserving the existing valid-flow assertions.Source: Path instructions
test/pr-review-advisor-writing-guides.test.ts (1)
13-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test into smaller, purpose-named cases.
This single
itblock asserts roughly 70 unrelated claims aboutbuildSystemPrompt(): guide loading, terminology-review wording, obsolete-text removal, finding-eligibility rules, severity mapping, and the multi-turn conversation contract. When one substring check fails, the single title does not indicate which concern broke, and unrelated wording edits elsewhere in the prompt can fail this test even though the specific behavior a reader cares about still works.Group the assertions into a few smaller
itblocks, each with a title naming the specific behavior it verifies (for example, "removes obsolete sandbox-escape wording", "includes terminology-review guidance", "documents finding severity mapping"). This keeps each test's public-boundary assertions but gives clearer failure attribution.As per coding guidelines: "Write behavior-oriented test titles and put local issue references in a final
(#1234)suffix."🤖 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/pr-review-advisor-writing-guides.test.ts` around lines 13 - 82, Split the single test around readTrustedSecurityReviewSkill, readTrustedWritingGuide, readTrustedControlledWords, and buildSystemPrompt into a few behavior-focused it blocks, grouping related assertions for guide loading, terminology and review policy, obsolete wording, finding rules and mappings, and conversation behavior. Give each test a specific behavior-oriented title and append the applicable local issue reference as a final (`#1234`) suffix, while preserving the existing boundary assertions.Source: Coding guidelines
tools/pr-review-advisor/analyze.mts (1)
161-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the terminology enums instead of redeclaring them.
TERMINOLOGY_CHANGES,TERMINOLOGY_DISPOSITIONS, andTERMINOLOGY_IMPACTSduplicateCHANGES,DISPOSITIONS, andSEMANTIC_IMPACTSintools/pr-review-advisor/terminology.mts(Lines 14-24). The same lists also appear intools/pr-review-advisor/schema.jsonand intrustedLaneTerminologyintools/pr-review-advisor/comment.mts(Lines 880-893). A new disposition added in one place can silently drift from the others: the ledger would accept a value thatsanitizeTerminologyReviewmaps back to"define".
analyze.mtsalready imports from./terminology.mts, so export the tuples and reuse them here. Keep the publisher copy incomment.mtsif the trusted-publisher boundary requires an independent list, and document that requirement.As per path instructions: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."
♻️ Proposed change
In
tools/pr-review-advisor/terminology.mts, export the existing tuples:-const CHANGES = ["introduced", "expanded", "redefined"] as const; -const DISPOSITIONS = ["established", "justified", "define", "replace", "conflict"] as const; -const SEMANTIC_IMPACTS = [ +export const CHANGES = ["introduced", "expanded", "redefined"] as const; +export const DISPOSITIONS = ["established", "justified", "define", "replace", "conflict"] as const; +export const SEMANTIC_IMPACTS = [Then in this file:
-const TERMINOLOGY_STATUSES = ["clear", "candidates", "limited"] as const; -const TERMINOLOGY_CHANGES = ["introduced", "expanded", "redefined"] as const; -const TERMINOLOGY_DISPOSITIONS = [ - "established", - "justified", - "define", - "replace", - "conflict", -] as const; -const TERMINOLOGY_IMPACTS = [ - "none", - "behavior", - "security", - "support", - "evidence", - "test", - "release", -] as const; +const TERMINOLOGY_STATUSES = ["clear", "candidates", "limited"] as const; +const TERMINOLOGY_CHANGES = CHANGES; +const TERMINOLOGY_DISPOSITIONS = DISPOSITIONS; +const TERMINOLOGY_IMPACTS = SEMANTIC_IMPACTS;🤖 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 `@tools/pr-review-advisor/analyze.mts` around lines 161 - 178, Export the canonical CHANGES, DISPOSITIONS, and SEMANTIC_IMPACTS tuples from terminology.mts, then update analyze.mts to import and reuse them instead of declaring TERMINOLOGY_CHANGES, TERMINOLOGY_DISPOSITIONS, and TERMINOLOGY_IMPACTS locally. Leave trustedLaneTerminology in comment.mts independent only if required by the trusted-publisher boundary, and document that requirement.Source: Path instructions
test/pr-review-advisor-comment-cli.test.ts (1)
264-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an absent
terminologyReviewand for a duplicate decision ID.This test proves that a wrong head SHA makes the lane unavailable. Two validation branches in
trustedLaneTerminologyremain uncovered:
- An artifact that omits
terminologyReviewentirely.trustedLaneStructureaccepts it and fingerprints an empty list, so the publisher reports matching terminology decisions. See my comment ontools/pr-review-advisor/comment.mtsLines 811-847.- A repeated
T-ID or more than 20 decisions.Add one assertion for each branch so the intended behavior is pinned.
As per path instructions: "Require focused tests for both detection and false-positive behavior."
🤖 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/pr-review-advisor-comment-cli.test.ts` around lines 264 - 289, Add focused assertions in the test covering normalizeAdvisorLaneReport for both trustedLaneTerminology validation branches: an artifact with terminologyReview omitted entirely, and an artifact containing duplicate T- decision IDs or more than 20 decisions. Assert the expected unavailable result for invalid inputs and include a valid control case where appropriate to pin detection without false positives.Source: Path instructions
test/pr-review-advisor.test.ts (1)
490-517: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert guide content in the advisor prompt.
Add assertions that
buildSystemPrompt()contains distinctive content from bothwritingGuideandskill. The current test checks loaded values and section labels, but not actual guide embedding.🤖 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/pr-review-advisor.test.ts` around lines 490 - 517, Update the buildSystemPrompt() test to assert that the generated advisor prompt contains distinctive text from both writingGuide and skill, using representative values already loaded by the test. Keep the existing assertions for loaded values and section labels, and add coverage for the actual embedded guide content.Source: Path instructions
tools/pr-review-advisor/schema.json (1)
288-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd single-line and SHA constraints to terminology fields.
renderSummaryinterpolatesterm,recommendation, andnoChangesReasondirectly into Markdown. Newlines can create extra lines. Add^[^\\r\\n]*$to the terminology text fields andnoChangesReason. Add^[0-9a-f]{40}$tosource.headSha.🤖 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 `@tools/pr-review-advisor/schema.json` around lines 288 - 302, Update the schema fields used by renderSummary to reject newline characters by adding the ^[^\\r\\n]*$ pattern to term, recommendation, and noChangesReason while preserving their existing constraints; also constrain source.headSha with ^[0-9a-f]{40}$. Locate the corresponding property definitions in the schema rather than changing unrelated fields.
🤖 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 `@tools/pr-review-advisor/analyze.mts`:
- Around line 1690-1699: Update the entrypoint flow around
readTrustedControlledWords() and readTrustedWritingGuide() so failures occurring
before writeFailure initialization still produce the required failure or
unavailable artifact. Ensure the caught error is passed through the existing
artifact-writing mechanism before logging and exiting, while preserving the
current error details and normal success path.
- Around line 777-809: Update canonicalRetryFallback to run
reviewLedgerConsistencyIssues and terminologyReviewConsistencyIssues against the
original draft result before applying withCanonicalReviewLedgerFindings and
withCanonicalTerminologyReview, while preserving sourceOfTruthReview validation.
Replace JSON.stringify comparison in terminologyReviewConsistencyIssues with a
stable key-order-independent comparison for same-session receipts. Add coverage
for draft finding drift and equivalent terminologyReview objects with reordered
keys.
In `@tools/pr-review-advisor/terminology.mts`:
- Around line 353-365: Update grepRef to bound git grep output so frequent
variants cannot exceed git()'s 4 MiB buffer, and handle the resulting
buffer-overflow failure as truncated evidence rather than propagating an error.
Preserve the existing deduplication and sorting, and ensure the traceTerminology
flow around the allowFailure handling can continue when output is truncated; use
separate count logic only if exact baseOccurrences and headOccurrences are
required.
---
Outside diff comments:
In `@tools/pr-review-advisor/comment.mts`:
- Around line 811-847: Strengthen terminology PR-SHA validation in
tools/pr-review-advisor/comment.mts#L811-L847 by making trustedLaneStructure
reject absent terminologyReview and adding the optional terminology field to its
declared return type. In tools/pr-review-advisor/comment.mts#L632-L643, update
renderTerminologyDetails to require result?.headSha to be a string before
comparing it with decision.source.headSha, preventing undefined values from
matching.
---
Nitpick comments:
In `@test/pr-review-advisor-comment-cli.test.ts`:
- Around line 264-289: Add focused assertions in the test covering
normalizeAdvisorLaneReport for both trustedLaneTerminology validation branches:
an artifact with terminologyReview omitted entirely, and an artifact containing
duplicate T- decision IDs or more than 20 decisions. Assert the expected
unavailable result for invalid inputs and include a valid control case where
appropriate to pin detection without false positives.
In `@test/pr-review-advisor-terminology.test.ts`:
- Around line 101-182: The terminology tool tests currently cover only valid
stage sequencing; add negative-path coverage for the public execute() boundary.
In the test using createTerminologyToolController, invoke TERMINOLOGY_TRACE_TOOL
before setting "terminology-review-analysis" and TERMINOLOGY_UPDATE_TOOL before
setting "terminology-review", asserting each call rejects with the stage-guard
error while preserving the existing valid-flow assertions.
In `@test/pr-review-advisor-writing-guides.test.ts`:
- Around line 13-82: Split the single test around
readTrustedSecurityReviewSkill, readTrustedWritingGuide,
readTrustedControlledWords, and buildSystemPrompt into a few behavior-focused it
blocks, grouping related assertions for guide loading, terminology and review
policy, obsolete wording, finding rules and mappings, and conversation behavior.
Give each test a specific behavior-oriented title and append the applicable
local issue reference as a final (`#1234`) suffix, while preserving the existing
boundary assertions.
In `@test/pr-review-advisor.test.ts`:
- Around line 490-517: Update the buildSystemPrompt() test to assert that the
generated advisor prompt contains distinctive text from both writingGuide and
skill, using representative values already loaded by the test. Keep the existing
assertions for loaded values and section labels, and add coverage for the actual
embedded guide content.
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 161-178: Export the canonical CHANGES, DISPOSITIONS, and
SEMANTIC_IMPACTS tuples from terminology.mts, then update analyze.mts to import
and reuse them instead of declaring TERMINOLOGY_CHANGES,
TERMINOLOGY_DISPOSITIONS, and TERMINOLOGY_IMPACTS locally. Leave
trustedLaneTerminology in comment.mts independent only if required by the
trusted-publisher boundary, and document that requirement.
In `@tools/pr-review-advisor/schema.json`:
- Around line 288-302: Update the schema fields used by renderSummary to reject
newline characters by adding the ^[^\\r\\n]*$ pattern to term, recommendation,
and noChangesReason while preserving their existing constraints; also constrain
source.headSha with ^[0-9a-f]{40}$. Locate the corresponding property
definitions in the schema rather than changing unrelated fields.
🪄 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: Enterprise
Run ID: 58cef949-c6e9-4a95-9374-c84f21d1a02b
📒 Files selected for processing (23)
.agents/skills/_shared/controlled-words.md.agents/skills/nemoclaw-contributor-update-hermes/SKILL.md.agents/skills/nemoclaw-contributor-update-hermes/references/hermes-contract-map.md.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-skills-guide/SKILL.mddocs/changelog/2026-07-24.mdxdocs/security/fern-5.80.1-dependency-review.mddocs/security/hermes-0.19.0-dependency-review.mdtest/hermes-upgrade-skill.test.tstest/pr-review-advisor-comment-cli.test.tstest/pr-review-advisor-terminology.test.tstest/pr-review-advisor-test-depth.test.tstest/pr-review-advisor-workflow-boundary.test.tstest/pr-review-advisor-writing-guides.test.tstest/pr-review-advisor.test.tstest/skills/check-gates-fork-evidence.test.tstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/review-ledger.mtstools/pr-review-advisor/run-analysis.mtstools/pr-review-advisor/schema.jsontools/pr-review-advisor/terminology.mts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/pr-review-advisor/terminology.mts (1)
313-358: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
ENOBUFSinchangedTermLocations.
git()limits output to 4 MiB, but an overflow raisesENOBUFSwith no numericstatus, soallowFailuredoes not catch it. A large diff can still maketraceTerminologyfail. Truncate the diff or handleENOBUFSexplicitly.🤖 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 `@tools/pr-review-advisor/terminology.mts` around lines 313 - 358, Update changedTermLocations, used by traceTerminology, to handle git output exceeding the 4 MiB buffer: either truncate the diff before processing or explicitly catch ENOBUFS and return the existing truncated-result behavior. Ensure large diffs do not cause traceTerminology to fail when the error has no numeric status.
🧹 Nitpick comments (1)
test/pr-review-advisor-writing-guide.test.ts (1)
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the
readFileSyncmock to the writing-guide path instead of call order.The mock chains two
mockImplementationOncecalls onfs.readFileSyncwithout a path check: the first call returnssecuritySkill, and the second throws to simulate the missing writing guide. This binds the test to the internal call order and count offs.readFileSyncinsidepreparePromptArtifacts. If an unrelated internal change adds, removes, or reorders areadFileSynccall, the test can fail, or pass while exercising the wrong scenario, without the writing guide actually becoming unavailable.Make the mock conditional on the file path being read (for example, throwing only when the path contains
WRITING.md, matching the pattern in the "stops when the trusted guide is unavailable" test) so the test verifies "the writing guide is unavailable" rather than "the secondreadFileSynccall fails".As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."♻️ Proposed path-scoped mock
- const readSpy = vi - .spyOn(fs, "readFileSync") - .mockImplementationOnce(() => securitySkill) - .mockImplementationOnce(() => { - throw new Error("missing guide fixture"); - }); + const readSpy = vi.spyOn(fs, "readFileSync").mockImplementation((file, ...rest) => { + if (String(file).includes("WRITING.md")) { + throw new Error("missing guide fixture"); + } + return fs.readFileSync.wrappedMethod?.(file, ...rest) ?? securitySkill; + });🤖 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/pr-review-advisor-writing-guide.test.ts` around lines 53 - 58, Update the readFileSync mock in the preparePromptArtifacts test to inspect the requested path instead of chaining mockImplementationOnce calls. Throw only when the path identifies WRITING.md, and return securitySkill for other reads, so the test specifically simulates an unavailable writing guide without depending on internal call order.Source: Path instructions
🤖 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.
Outside diff comments:
In `@tools/pr-review-advisor/terminology.mts`:
- Around line 313-358: Update changedTermLocations, used by traceTerminology, to
handle git output exceeding the 4 MiB buffer: either truncate the diff before
processing or explicitly catch ENOBUFS and return the existing truncated-result
behavior. Ensure large diffs do not cause traceTerminology to fail when the
error has no numeric status.
---
Nitpick comments:
In `@test/pr-review-advisor-writing-guide.test.ts`:
- Around line 53-58: Update the readFileSync mock in the preparePromptArtifacts
test to inspect the requested path instead of chaining mockImplementationOnce
calls. Throw only when the path identifies WRITING.md, and return securitySkill
for other reads, so the test specifically simulates an unavailable writing guide
without depending on internal call order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c6ff67bb-68ed-47de-a907-d06f8ee21277
📒 Files selected for processing (11)
test/pr-review-advisor-comment-cli.test.tstest/pr-review-advisor-ledger-tools.test.tstest/pr-review-advisor-terminology.test.tstest/pr-review-advisor-writing-guide.test.tstest/pr-review-advisor-writing-guides.test.tstest/pr-review-advisor.test.tstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/schema.jsontools/pr-review-advisor/terminology.mts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/pr-review-advisor-writing-guides.test.ts
- tools/pr-review-advisor/README.md
- tools/pr-review-advisor/schema.json
- tools/pr-review-advisor/comment.mts
- test/pr-review-advisor.test.ts
- tools/pr-review-advisor/analyze.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/pr-review-advisor/terminology.mts (1)
314-359: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
firstCommitShacase-insensitive and variant-aware.With Git 2.50.1,
--regexp-ignore-casedoes not affect fixed-string-S${normalized}searches. Add--pickaxe-regexand search escapedvariants; otherwise history for differently cased or hyphen/space variants can remainnull.🤖 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 `@tools/pr-review-advisor/terminology.mts` around lines 314 - 359, Update the firstCommitSha git log query in traceTerminology to use the complete variants collection instead of only normalized, and enable regex pickaxe matching with --pickaxe-regex while preserving case-insensitive search. Escape each variant before constructing the search expression so punctuation in hyphen/space variants is matched literally.
🧹 Nitpick comments (1)
tools/pr-review-advisor/terminology.mts (1)
239-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the terminology stage names from a canonical source.
"terminology-review"is duplicated intools/pr-review-advisor/terminology.mtsandtools/pr-review-advisor/analyze.mts. Export the stage constants and use them in both modules.🤖 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 `@tools/pr-review-advisor/terminology.mts` around lines 239 - 312, Define and export canonical constants for the terminology stages in terminology.mts, including the values currently used by the trace and update stage checks. Replace the inline "terminology-review-analysis" and "terminology-review" comparisons in createTerminologyToolController with those constants, then import and use the same constants in analyze.mts instead of duplicating the string literals.Source: Path instructions
🤖 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 `@tools/pr-review-advisor/terminology.mts`:
- Around line 428-476: Update changedTermLocations so failure of both merge-base
and direct diff evidence is surfaced instead of returning an empty locations
array. Normalize command failures, including rejected streamChangedTermLocations
calls, using the existing git() error-handling convention, while preserving []
only when a diff succeeds with no relevant changes.
- Around line 428-446: Update both Git argument arrays in changedTermLocations
to pass -c core.quotePath=false before the diff subcommand and add
--default-prefix, ensuring non-ASCII paths are parsed correctly while preserving
the existing merge-base fallback behavior.
---
Outside diff comments:
In `@tools/pr-review-advisor/terminology.mts`:
- Around line 314-359: Update the firstCommitSha git log query in
traceTerminology to use the complete variants collection instead of only
normalized, and enable regex pickaxe matching with --pickaxe-regex while
preserving case-insensitive search. Escape each variant before constructing the
search expression so punctuation in hyphen/space variants is matched literally.
---
Nitpick comments:
In `@tools/pr-review-advisor/terminology.mts`:
- Around line 239-312: Define and export canonical constants for the terminology
stages in terminology.mts, including the values currently used by the trace and
update stage checks. Replace the inline "terminology-review-analysis" and
"terminology-review" comparisons in createTerminologyToolController with those
constants, then import and use the same constants in analyze.mts instead of
duplicating the string literals.
🪄 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: Enterprise
Run ID: 8780fee1-fed6-4491-8a81-8330b7a5eef8
📒 Files selected for processing (3)
test/pr-review-advisor-terminology.test.tstools/pr-review-advisor/README.mdtools/pr-review-advisor/terminology.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pr-review-advisor/README.md
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
1 participant
Summary
PR Review Advisor now asks each model lane to select and evaluate terminology introduced or changed by a PR. Trusted tooling verifies each decision against repository evidence, while publication compares normalized lane results without changing the primary recommendation.
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updated04bd4214e5b3530d7057736535f242dfd4e00acd; the independent review found no actionable documentation issues after its clarity advisory was applied. The remaining revision terms describe real commit-binding or comparison contracts.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run typecheck:cli, Biome,npm run checks:repository,npm run docs, andgit diff --checkpassed. The unchanged MCP status test passed 5/5 locally after the known CI shard timeout.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm testwas attempted with the contributor SSH agent, but unrelated gateway-host tests require an active OpenShell gateway on this host. GitHub CI remains authoritative.npm run docsbuilds without warnings (doc changes only) — 0 errors and 2 existing Fern warningsSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests