fix(relation-discovery): improve evidence retries#104
Merged
Conversation
WalkthroughThis PR adds Sequence Diagram(s)sequenceDiagram
participant Build as generateChapterKnowledgeGraphArtifact
participant Match as matchWikispineSentenceCandidates
participant Parser as createWikispineNdjsonParser
participant WikiSpine as WikiSpine process/API
participant Reporter as BuildJobProgressReporter
Build->>Reporter: updatePhase(matching, total=text.length, unit=char)
Build->>Match: matchWikispineSentenceCandidates(onProgress)
Match->>WikiSpine: send sentence text/range
WikiSpine-->>Parser: stream NDJSON output
Parser->>Parser: push(chunk), parse line
Parser-->>Match: onMatch(coveredRangeEnd)
Match-->>Build: onProgress(coveredRangeEnd)
Build->>Reporter: updatePhase(coveredRangeEnd, unit=char, force?)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/wikilink/relation-discovery.ts (1)
386-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor redundancy between adjacent prompt bullets.
Line 393 ("Every relation must connect two mention IDs from the
<mention id="..." qid="...">tags") and the new line 394 ("A relation is valid only when both endpoints are explicitly tagged mentions...") convey largely the same constraint. Not incorrect, just slightly repetitive for the LLM prompt.🤖 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/wikilink/relation-discovery.ts` around lines 386 - 409, The relation prompt in formatRelationSystemPrompt is slightly redundant because the “Every relation must connect two mention IDs” bullet and the “both endpoints are explicitly tagged mentions” bullet repeat the same rule. Merge or remove one of these adjacent bullets in relation-discovery.ts so the prompt stays concise while preserving the mention-ID constraint.
🤖 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/facade/knowledge-graph-build.ts`:
- Around line 141-157: The onProgress callback in
matchWikispineSentenceCandidates currently swallows updatePhase() cancellation
failures via .catch(() => undefined), which prevents BuildJobStoppedError from
stopping the work promptly. Remove the suppression so the promise rejection from
options.progressTracker?.updatePhase() can bubble out of the callback, allowing
knowledge-graph build to abort immediately when stopped.
In `@src/wikimatch/wikispine.ts`:
- Around line 184-190: The wikispine progress callback is swallowing rejected
promises because `void options.onProgress?.(...)` ignores failures, which can
lead to unhandled rejections. Update the `createWikispineNdjsonParser` `onMatch`
handler in `wikispine.ts` to either await/propagate `options.onProgress` or
explicitly attach a catch handler that logs or handles errors, and make the same
adjustment anywhere else the fetch path uses the same `onProgress` pattern. Keep
the fix localized around `onMatch` and the `options.onProgress` call sites.
---
Nitpick comments:
In `@src/wikilink/relation-discovery.ts`:
- Around line 386-409: The relation prompt in formatRelationSystemPrompt is
slightly redundant because the “Every relation must connect two mention IDs”
bullet and the “both endpoints are explicitly tagged mentions” bullet repeat the
same rule. Merge or remove one of these adjacent bullets in
relation-discovery.ts so the prompt stays concise while preserving the
mention-ID constraint.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1fff9db-03d5-47ee-ba4c-6a80340a2493
📒 Files selected for processing (11)
src/cli/queue.tssrc/evidence-selection/prompt.tssrc/evidence-selection/selection-resolver.tssrc/facade/build-queue.tssrc/facade/knowledge-graph-build.tssrc/wikilink/relation-discovery.tssrc/wikimatch/wikispine.tstest/cli/queue.test.tstest/evidence-selection/selection-resolver.test.tstest/facade/build-queue.test.tstest/wikimatch/wikispine.test.ts
Moskize91
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/wikimatch/wikispine.test.ts (1)
167-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp directory not cleaned up.
mkdtempcreates a temp dir but it's never removed after the test, leakingfake-wikispine.mjsfiles across test runs.♻️ Optional cleanup
it("rejects CLI matches when progress reporting fails", async () => { const tempDir = await mkdtemp(join(tmpdir(), "wikispine-test-")); const commandPath = join(tempDir, "fake-wikispine.mjs"); + onTestFinished(() => rm(tempDir, { recursive: true, force: true }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/wikimatch/wikispine.test.ts` around lines 167 - 193, The wikispine test creates a temporary directory with mkdtemp in the matchWikispineSentenceCandidates case but never removes it, leaving fake-wikispine.mjs behind between runs. Add cleanup in the test using the tempDir variable so the temporary directory is deleted after the assertion completes, ideally by wrapping the test body in a try/finally and removing the directory in the finally block.src/wikimatch/wikispine.ts (1)
328-344: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelease the reader when the stream loop aborts early.
When
progress.throwIfFailed()throws (anonProgresscallback rejected), the function exits with theReadableStreamDefaultReaderstill locked and the response body unconsumed, leaking the underlying connection/socket. Cancel the reader on any early exit.♻️ Proposed fix to cancel the reader on early exit
const reader = response.body.getReader(); const decoder = new TextDecoder(); - while (true) { - const { done, value } = await reader.read(); - if (done) { - break; - } - parser.push(decoder.decode(value, { stream: true })); - progress.throwIfFailed(); - } - - parser.push(decoder.decode()); + try { + while (true) { + const { done, value } = await reader.read(); + if (done) { + break; + } + parser.push(decoder.decode(value, { stream: true })); + progress.throwIfFailed(); + } + parser.push(decoder.decode()); + } catch (error) { + await reader.cancel().catch(() => {}); + throw error; + } const matches = parser.finish(); await progress.wait(); return matches;🤖 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/wikimatch/wikispine.ts` around lines 328 - 344, The stream loop in wikispine’s reader handling can exit early when progress.throwIfFailed() throws, leaving the ReadableStreamDefaultReader locked and the response body unconsumed. Update the logic around response.body.getReader(), the read loop, and parser.finish() so the reader is cancelled/released on any early exit path, especially when onProgress fails, while preserving the normal decode/push/finish flow.
🤖 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.
Nitpick comments:
In `@src/wikimatch/wikispine.ts`:
- Around line 328-344: The stream loop in wikispine’s reader handling can exit
early when progress.throwIfFailed() throws, leaving the
ReadableStreamDefaultReader locked and the response body unconsumed. Update the
logic around response.body.getReader(), the read loop, and parser.finish() so
the reader is cancelled/released on any early exit path, especially when
onProgress fails, while preserving the normal decode/push/finish flow.
In `@test/wikimatch/wikispine.test.ts`:
- Around line 167-193: The wikispine test creates a temporary directory with
mkdtemp in the matchWikispineSentenceCandidates case but never removes it,
leaving fake-wikispine.mjs behind between runs. Add cleanup in the test using
the tempDir variable so the temporary directory is deleted after the assertion
completes, ideally by wrapping the test body in a try/finally and removing the
directory in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 835c8eff-311f-4a82-9652-9e615cfe29aa
📒 Files selected for processing (3)
src/facade/knowledge-graph-build.tssrc/wikimatch/wikispine.tstest/wikimatch/wikispine.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/facade/knowledge-graph-build.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
records 0/NcounterTesting
pnpm lint:fixpnpm typecheckpnpm vitest run test/evidence-selection/selection-resolver.test.ts --passWithNoTestspnpm vitest run test/wikilink/relation-discovery.test.ts --passWithNoTestspnpm vitest run test/facade/build-queue.test.ts test/cli/queue.test.ts --passWithNoTests