CF-5: close native citation exit gate - #29
Merged
Conversation
Native-model reads take 15-20s per segment, so the per-segment count alone looks hung between updates. Cycle a small glyph next to the stage label while a document is in the Reading/Reducing stage as a lightweight "still alive" cue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates Context Fabric citation handling, search fallback, prompt budgeting, PDF text extraction, and citation-opening UI, adds source-file staging/opening, expands tests, and simplifies experimental native runtime role pre-binding. ChangesContext Fabric CF-5 Backend and UI Improvements
Native Runtime Role Pre-binding
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
OrchestratorIDE/Services/ContextFabric/FabricAskService.cs (2)
163-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSource-label resolution uses exact ordinal string matching.
ResolveSourceLabelslooks upcitation.SourceLabelinsourceLabelsvia a default (ordinal, case-sensitive)Dictionary<string,string>built inBuildEvidenceText. Given this PR is explicitly hardening against non-compliant model output elsewhere (rambling JSON, stray artifacts), a model that emits"s1"," S1", or similar near-miss labels will silently fail to resolve back to aSegmentId, defeating the citation-exit-gate fix for that citation without any error signal.Consider making the lookup tolerant of case/whitespace drift.
♻️ Proposed fix
- var sb = new StringBuilder(); - sourceLabels = []; + var sb = new StringBuilder(); + sourceLabels = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);Citations = claim.Citations.Select(citation => string.IsNullOrWhiteSpace(citation.SegmentId) - && sourceLabels.TryGetValue(citation.SourceLabel, out var segmentId) + && sourceLabels.TryGetValue(citation.SourceLabel.Trim(), out var segmentId) ? citation with { SegmentId = segmentId } : citation).ToList(),🤖 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 `@OrchestratorIDE/Services/ContextFabric/FabricAskService.cs` around lines 163 - 203, ResolveSourceLabels currently relies on exact Dictionary lookup for citation.SourceLabel, so near-miss model output like lowercase or padded labels won’t map to a SegmentId. Make the source-label matching in ResolveSourceLabels tolerant of case and surrounding whitespace, and keep the mapping built in BuildEvidenceText consistent with that normalization so citations still resolve even when the model emits slightly malformed labels.
139-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare
catchswallows parse failures without any diagnostic trail.All exceptions from
FabricJson.ParseModelObjectare swallowed and only surfaced as truncated raw text insideAnswer; there's no way to distinguish a schema mismatch from an unexpected bug in the parser itself.🤖 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 `@OrchestratorIDE/Services/ContextFabric/FabricAskService.cs` around lines 139 - 161, The bare catch in ParseAnswerDraft is swallowing all parse failures without preserving any diagnostic details. Update the FabricAskService.ParseAnswerDraft handling to catch the exception as a variable and include its type/message (and, if appropriate, stack trace) in the fallback path so callers can distinguish a schema mismatch from an internal parser failure. Keep the existing FabricJson.ParseModelObject<FabricAnswerDraft> flow, but make the fallback Answer text or logging clearly reference the captured exception instead of only the truncated raw payload.OrchestratorIDE/Services/ContextFabric/ContextFabricContracts.cs (1)
147-154: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCross-file token-budget invariant is comment-only.
The comment correctly documents that
AnswerMaxTokensmust stay<=FabricQueryPlannerOptions.ResponseTokenReserve(currently 2048==2048), but nothing enforces this at runtime if either default is changed independently later — silently reintroducing the exact "Answer schema parse failed" truncation bug this PR fixes.Consider adding a runtime assertion where both options are combined (e.g., in
FabricAskService's constructor or whereverFabricRunOptionsandFabricQueryPlannerOptionsare wired together) so a future mismatch fails fast instead of degrading silently.🤖 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 `@OrchestratorIDE/Services/ContextFabric/ContextFabricContracts.cs` around lines 147 - 154, The token-budget invariant between AnswerMaxTokens and FabricQueryPlannerOptions.ResponseTokenReserve is only documented in a comment and can drift silently. Add a runtime assertion or validation in the place where FabricRunOptions and FabricQueryPlannerOptions are composed, such as FabricAskService, to verify AnswerMaxTokens does not exceed ResponseTokenReserve and fail fast if the values diverge. Use the existing AnswerMaxTokens and FabricQueryPlannerOptions.ResponseTokenReserve symbols so the check stays tied to the intended cross-file contract.OrchestratorIDE/Services/ContextFabric/FabricSearchService.cs (1)
13-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify direct test coverage for the strict→loose retry.
The retry logic here is the crux of the CF-5 retrieval fix. The provided
ContextFabricAskServiceTestssnippet exercises exact-quote/SourceLabel resolution but not the zero-hit fallback path itself. The PR objectives mentionOrcChatContextFabricQueryTestspassed headless — if that suite doesn't directly assert onFabricSearchService.Search's loose-match fallback (e.g., natural-language multi-word query that misses strict AND but hits via OR), consider adding a focused unit test for it here, since it's a critical path for citation retrieval quality.🤖 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 `@OrchestratorIDE/Services/ContextFabric/FabricSearchService.cs` around lines 13 - 23, Add direct test coverage for the strict-to-loose retry in FabricSearchService.Search, since the current tests do not verify the zero-hit fallback path. Add or update a focused unit test around CollectHits/Search that uses a natural-language multi-word query which returns no results with looseMatch:false but does return hits with looseMatch:true, so the fallback behavior is asserted explicitly. Use FabricSearchService and the strict→loose retry branch as the key symbols to locate the code.OrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.cs (1)
531-537: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating duplicated FTS-query builders.
This
BuildFtsQueryandFabricLibraryRepository.BuildFtsQuery(Lines 440-442 there) now both implement the same AND/OR-toggle-with-quoting logic, cross-referencing each other in comments instead of sharing code. Term extraction differs (raw whitespace split vs. regex word matching), so it's not a trivial merge, but extracting the shared "join quoted/escaped terms with AND/OR" portion into a common static helper (taking pre-tokenized terms) would remove the duplication and prevent future drift between the two.🤖 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 `@OrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.cs` around lines 531 - 537, The FTS query construction logic is duplicated between DocumentGraphRepository.BuildFtsQuery and FabricLibraryRepository.BuildFtsQuery, so extract the shared “quote/escape terms and join with AND/OR” behavior into a common static helper that accepts pre-tokenized terms. Keep the existing tokenization differences in each caller, but have both BuildFtsQuery methods delegate to the shared helper so the quoting, escaping, and looseMatch behavior stay consistent and don’t drift.
🤖 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 `@OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs`:
- Around line 40-57: The LibraryDrawerControl constructor starts _pulseTimer but
there is no teardown, so detached or recreated instances can keep ticking and
stay referenced. Add cleanup in LibraryDrawerControl, ideally by overriding
OnDetachedFromVisualTree (or the equivalent detach hook) to stop _pulseTimer and
release the tick-driven update path. Use the existing _pulseTimer and _pulseTick
members to locate the lifecycle code and ensure the timer is stopped when the
control leaves the visual tree.
In `@OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs`:
- Around line 206-227: In ExtractPageText, the words are grouped into lines
correctly, but each line still preserves the global Bottom-descending order,
which can scramble left-to-right reading order. After building each line in the
line-grouping loop, sort the words within that line by BoundingBox.Left before
joining their text. Keep the existing line grouping logic and adjust the final
line rendering so reconstructed text is emitted in visual order.
In `@OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs`:
- Around line 53-68: TryStageSourceFileForOpen is reusing a predictable shared
temp path, which can allow a pre-planted file to be opened instead of the real
source, and its unguarded copy can throw if the artifact is evicted
mid-operation. Update the staging logic in FabricLibraryService so it verifies
the staged file still matches the expected source digest before reusing it,
rather than trusting File.Exists alone. Also wrap the File.Copy path in handling
for missing/evicted blobs so the method returns null instead of letting an
exception escape to SourcePreviewPanel.TryOpenSourceFile. Keep the fix centered
on TryStageSourceFileForOpen and the stagedPath/blobPath flow.
---
Nitpick comments:
In `@OrchestratorIDE/Services/ContextFabric/ContextFabricContracts.cs`:
- Around line 147-154: The token-budget invariant between AnswerMaxTokens and
FabricQueryPlannerOptions.ResponseTokenReserve is only documented in a comment
and can drift silently. Add a runtime assertion or validation in the place where
FabricRunOptions and FabricQueryPlannerOptions are composed, such as
FabricAskService, to verify AnswerMaxTokens does not exceed ResponseTokenReserve
and fail fast if the values diverge. Use the existing AnswerMaxTokens and
FabricQueryPlannerOptions.ResponseTokenReserve symbols so the check stays tied
to the intended cross-file contract.
In `@OrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.cs`:
- Around line 531-537: The FTS query construction logic is duplicated between
DocumentGraphRepository.BuildFtsQuery and FabricLibraryRepository.BuildFtsQuery,
so extract the shared “quote/escape terms and join with AND/OR” behavior into a
common static helper that accepts pre-tokenized terms. Keep the existing
tokenization differences in each caller, but have both BuildFtsQuery methods
delegate to the shared helper so the quoting, escaping, and looseMatch behavior
stay consistent and don’t drift.
In `@OrchestratorIDE/Services/ContextFabric/FabricAskService.cs`:
- Around line 163-203: ResolveSourceLabels currently relies on exact Dictionary
lookup for citation.SourceLabel, so near-miss model output like lowercase or
padded labels won’t map to a SegmentId. Make the source-label matching in
ResolveSourceLabels tolerant of case and surrounding whitespace, and keep the
mapping built in BuildEvidenceText consistent with that normalization so
citations still resolve even when the model emits slightly malformed labels.
- Around line 139-161: The bare catch in ParseAnswerDraft is swallowing all
parse failures without preserving any diagnostic details. Update the
FabricAskService.ParseAnswerDraft handling to catch the exception as a variable
and include its type/message (and, if appropriate, stack trace) in the fallback
path so callers can distinguish a schema mismatch from an internal parser
failure. Keep the existing FabricJson.ParseModelObject<FabricAnswerDraft> flow,
but make the fallback Answer text or logging clearly reference the captured
exception instead of only the truncated raw payload.
In `@OrchestratorIDE/Services/ContextFabric/FabricSearchService.cs`:
- Around line 13-23: Add direct test coverage for the strict-to-loose retry in
FabricSearchService.Search, since the current tests do not verify the zero-hit
fallback path. Add or update a focused unit test around CollectHits/Search that
uses a natural-language multi-word query which returns no results with
looseMatch:false but does return hits with looseMatch:true, so the fallback
behavior is asserted explicitly. Use FabricSearchService and the strict→loose
retry branch as the key symbols to locate the code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cb673e5-1e87-4c0f-b26f-80d503a7d414
📒 Files selected for processing (20)
OrchestratorIDE.Avalonia/MainWindow.axaml.csOrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.csOrchestratorIDE.Avalonia/UI/Controls/SourcePreviewPanel.axamlOrchestratorIDE.Avalonia/UI/Controls/SourcePreviewPanel.axaml.csOrchestratorIDE.Avalonia/UI/Panels/ChatPanel.axamlOrchestratorIDE.Avalonia/UI/Panels/ChatPanel.axaml.csOrchestratorIDE.Avalonia/UI/ViewModels/LibraryViewModel.csOrchestratorIDE.Avalonia/UI/Windows/SourceCitationWindow.axamlOrchestratorIDE.Avalonia/UI/Windows/SourceCitationWindow.axaml.csOrchestratorIDE.UnitTests/ContextFabricAskServiceTests.csOrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-primary-pdf.manifest.jsonOrchestratorIDE/Services/ContextFabric/ContextFabricContracts.csOrchestratorIDE/Services/ContextFabric/ContextFabricIngestionContracts.csOrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.csOrchestratorIDE/Services/ContextFabric/EvidencePackBuilder.csOrchestratorIDE/Services/ContextFabric/FabricAskService.csOrchestratorIDE/Services/ContextFabric/FabricDocumentParser.csOrchestratorIDE/Services/ContextFabric/FabricLibraryRepository.csOrchestratorIDE/Services/ContextFabric/FabricLibraryService.csOrchestratorIDE/Services/ContextFabric/FabricSearchService.cs
Comment on lines
+40
to
+57
| private readonly DispatcherTimer _pulseTimer; | ||
| private int _pulseTick; | ||
|
|
||
| public LibraryDrawerControl() | ||
| { | ||
| InitializeComponent(); | ||
| BuildHeader(); | ||
| BuildStorageFooter(); | ||
| Render(); | ||
|
|
||
| _pulseTimer = new DispatcherTimer { Interval = TimeSpan.FromMilliseconds(250) }; | ||
| _pulseTimer.Tick += (_, _) => | ||
| { | ||
| _pulseTick++; | ||
| if (_progressByDocument.Values.Any(p => p.Stage is IndexStageKind.Reading or IndexStageKind.Reducing)) | ||
| Render(); | ||
| }; | ||
| _pulseTimer.Start(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target file with line numbers in manageable chunks.
wc -l OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs
sed -n '1,220p' OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs
printf '\n--- SEARCH lifecycle overrides ---\n'
rg -n "OnDetachedFromVisualTree|OnUnloaded|OnAttachedToVisualTree|DispatcherTimer|Stop\(\)|Start\(\)" OrchestratorIDE.Avalonia/UI/Controls -g '*.cs'
printf '\n--- Search similar controls for timer lifecycle patterns ---\n'
rg -n "DispatcherTimer" OrchestratorIDE.Avalonia -g '*.cs'Repository: hardcoreerik/TheOrc
Length of output: 12682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Tail of LibraryDrawerControl.axaml.cs ---'
sed -n '220,654p' OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs
printf '\n%s\n' '--- References to LibraryDrawerControl ---'
rg -n "LibraryDrawerControl" OrchestratorIDE.Avalonia -g '*.cs' -g '*.axaml'
printf '\n%s\n' '--- Lifecycle overrides in similar controls ---'
rg -n "OnDetachedFromVisualTree|OnUnloaded|OnAttachedToVisualTree" OrchestratorIDE.Avalonia/UI -g '*.cs'Repository: hardcoreerik/TheOrc
Length of output: 20299
Stop _pulseTimer on detach — LibraryDrawerControl starts a 250ms timer in the constructor and never stops it, so a recreated or detached instance can keep ticking and retain the control. Hook OnDetachedFromVisualTree (or equivalent teardown) to call _pulseTimer.Stop(). OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs:40-57
🤖 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 `@OrchestratorIDE.Avalonia/UI/Controls/LibraryDrawerControl.axaml.cs` around
lines 40 - 57, The LibraryDrawerControl constructor starts _pulseTimer but there
is no teardown, so detached or recreated instances can keep ticking and stay
referenced. Add cleanup in LibraryDrawerControl, ideally by overriding
OnDetachedFromVisualTree (or the equivalent detach hook) to stop _pulseTimer and
release the tick-driven update path. Use the existing _pulseTimer and _pulseTick
members to locate the lifecycle code and ensure the timer is stopped when the
control leaves the visual tree.
Comment on lines
+206
to
+227
| var words = page.GetWords() | ||
| .OrderByDescending(word => word.BoundingBox.Bottom) | ||
| .ThenBy(word => word.BoundingBox.Left) | ||
| .ToList(); | ||
| if (words.Count == 0) | ||
| return ""; | ||
|
|
||
| const double lineTolerance = 3.0; | ||
| var lines = new List<List<Word>> { new() { words[0] } }; | ||
| for (var index = 1; index < words.Count; index++) | ||
| { | ||
| var word = words[index]; | ||
| var currentLine = lines[^1]; | ||
| if (Math.Abs(currentLine[0].BoundingBox.Bottom - word.BoundingBox.Bottom) <= lineTolerance) | ||
| currentLine.Add(word); | ||
| else | ||
| lines.Add([word]); | ||
| } | ||
|
|
||
| return string.Join('\n', lines | ||
| .Select(line => string.Join(" ", line.Select(word => word.Text)).Trim()) | ||
| .Where(line => line.Length > 0)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect word extraction ordering assumptions in the codebase for PdfPig usage.
rg -nP 'GetWords|BoundingBox\.(Bottom|Left|Top|Right)' --type=cs -C2Repository: hardcoreerik/TheOrc
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols.
git ls-files 'OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs'
wc -l OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs
ast-grep outline OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs --view expandedRepository: hardcoreerik/TheOrc
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section with line numbers.
sed -n '170,250p' OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs | cat -nRepository: hardcoreerik/TheOrc
Length of output: 4092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the method and surrounding code.
sed -n '180,240p' OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs | cat -n
# Find call sites / tests that may constrain the line ordering behavior.
rg -n "ExtractPageText|GetWords|BoundingBox\.Bottom|BoundingBox\.Left|BoundingBox\.Top|BoundingBox\.Right" OrchestratorIDE -SRepository: hardcoreerik/TheOrc
Length of output: 4209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact implementation and nearby context.
sed -n '190,235p' OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs | cat -n
# Check whether there are tests or docs covering page text extraction ordering.
rg -n "ExtractPageText|FabricDocumentParser|lineTolerance|GetWords" .Repository: hardcoreerik/TheOrc
Length of output: 5301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Word:
text: str
left: float
bottom: float
words = [
Word("right", 100, 10.0),
Word("left", 50, 9.6),
Word("next", 160, 2.0),
]
sorted_words = sorted(words, key=lambda w: (-w.bottom, w.left))
print([(w.text, w.left, w.bottom) for w in sorted_words])
line_tol = 3.0
lines = [[sorted_words[0]]]
for w in sorted_words[1:]:
current = lines[-1]
if abs(current[0].bottom - w.bottom) <= line_tol:
current.append(w)
else:
lines.append([w])
print([[w.text for w in line] for line in lines])
print("reconstructed:", "\n".join(" ".join(w.text for w in line).strip() for line in lines if line))
PYRepository: hardcoreerik/TheOrc
Length of output: 280
Sort each reconstructed line by BoundingBox.Left
ExtractPageText groups words into lines, but it keeps the global Bottom-descending order inside each line. When words on the same visual line have slightly different baselines, that can flip left-to-right reading order and scramble the reconstructed text.
Suggested fix
return string.Join('\n', lines
- .Select(line => string.Join(" ", line.Select(word => word.Text)).Trim())
+ .Select(line => string.Join(" ", line
+ .OrderBy(word => word.BoundingBox.Left)
+ .Select(word => word.Text)).Trim())
.Where(line => line.Length > 0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var words = page.GetWords() | |
| .OrderByDescending(word => word.BoundingBox.Bottom) | |
| .ThenBy(word => word.BoundingBox.Left) | |
| .ToList(); | |
| if (words.Count == 0) | |
| return ""; | |
| const double lineTolerance = 3.0; | |
| var lines = new List<List<Word>> { new() { words[0] } }; | |
| for (var index = 1; index < words.Count; index++) | |
| { | |
| var word = words[index]; | |
| var currentLine = lines[^1]; | |
| if (Math.Abs(currentLine[0].BoundingBox.Bottom - word.BoundingBox.Bottom) <= lineTolerance) | |
| currentLine.Add(word); | |
| else | |
| lines.Add([word]); | |
| } | |
| return string.Join('\n', lines | |
| .Select(line => string.Join(" ", line.Select(word => word.Text)).Trim()) | |
| .Where(line => line.Length > 0)); | |
| var words = page.GetWords() | |
| .OrderByDescending(word => word.BoundingBox.Bottom) | |
| .ThenBy(word => word.BoundingBox.Left) | |
| .ToList(); | |
| if (words.Count == 0) | |
| return ""; | |
| const double lineTolerance = 3.0; | |
| var lines = new List<List<Word>> { new() { words[0] } }; | |
| for (var index = 1; index < words.Count; index++) | |
| { | |
| var word = words[index]; | |
| var currentLine = lines[^1]; | |
| if (Math.Abs(currentLine[0].BoundingBox.Bottom - word.BoundingBox.Bottom) <= lineTolerance) | |
| currentLine.Add(word); | |
| else | |
| lines.Add([word]); | |
| } | |
| return string.Join('\n', lines | |
| .Select(line => string.Join(" ", line | |
| .OrderBy(word => word.BoundingBox.Left) | |
| .Select(word => word.Text)).Trim()) | |
| .Where(line => line.Length > 0)); |
🤖 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 `@OrchestratorIDE/Services/ContextFabric/FabricDocumentParser.cs` around lines
206 - 227, In ExtractPageText, the words are grouped into lines correctly, but
each line still preserves the global Bottom-descending order, which can scramble
left-to-right reading order. After building each line in the line-grouping loop,
sort the words within that line by BoundingBox.Left before joining their text.
Keep the existing line grouping logic and adjust the final line rendering so
reconstructed text is emitted in visual order.
Comment on lines
+53
to
+68
| public string? TryStageSourceFileForOpen(string documentId) | ||
| { | ||
| var document = _repository.GetDocument(documentId); | ||
| if (document is null) return null; | ||
|
|
||
| string blobPath; | ||
| try { blobPath = _artifacts.GetPath(document.SourceDigest); } | ||
| catch (FileNotFoundException) { return null; } | ||
|
|
||
| var extension = ExtensionForMediaType(document.MediaType); | ||
| var stagedPath = Path.Combine(Path.GetTempPath(), "TheOrc-source-preview", document.SourceDigest + extension); | ||
| Directory.CreateDirectory(Path.GetDirectoryName(stagedPath)!); | ||
| if (!File.Exists(stagedPath)) | ||
| File.Copy(blobPath, stagedPath, overwrite: false); | ||
| return stagedPath; | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Insecure shared-temp reuse can serve a substituted file, and a concurrent artifact eviction can crash the caller.
Two related problems in the staging logic:
stagedPathlives in a predictable, shared location (Path.GetTempPath()/TheOrc-source-preview/<digest>.<ext>— on Linux/macOS this is world-writable/tmp). Because the file is only copiedif (!File.Exists(stagedPath)), a local attacker who can write to that shared temp dir could pre-plant a file at the exact digest-named path; this method would then silently reuse it andSourcePreviewPanel.TryOpenSourceFilewould open the attacker's file viaProcess.Start(UseShellExecute: true)instead of the real source.File.Copy(blobPath, stagedPath, overwrite: false)is unguarded. IfDeleteUnreferencedArtifacts/EvictRebuildableArtifacts(which hold_mutationGate) delete the source blob between theGetPathcheck and this copy,File.CopythrowsFileNotFoundExceptionhere — and the caller (SourcePreviewPanel.TryOpenSourceFile) invokes this method outside anytry/catch, so the exception propagates unhandled out of the button's click handler.
🔒 Proposed fix: verify content before reuse, and guard the copy
public string? TryStageSourceFileForOpen(string documentId)
{
var document = _repository.GetDocument(documentId);
if (document is null) return null;
string blobPath;
try { blobPath = _artifacts.GetPath(document.SourceDigest); }
catch (FileNotFoundException) { return null; }
var extension = ExtensionForMediaType(document.MediaType);
- var stagedPath = Path.Combine(Path.GetTempPath(), "TheOrc-source-preview", document.SourceDigest + extension);
- Directory.CreateDirectory(Path.GetDirectoryName(stagedPath)!);
- if (!File.Exists(stagedPath))
- File.Copy(blobPath, stagedPath, overwrite: false);
- return stagedPath;
+ var stagingDir = Path.Combine(Path.GetTempPath(), "TheOrc-source-preview");
+ Directory.CreateDirectory(stagingDir);
+ var stagedPath = Path.Combine(stagingDir, document.SourceDigest + extension);
+
+ // The staging directory is shared/world-writable on most platforms, so a
+ // pre-existing file at this predictable path must not be trusted blindly --
+ // verify its content actually matches before reusing it (CWE-377), and always
+ // (re)write it via overwrite otherwise.
+ if (!File.Exists(stagedPath) || Digest(File.ReadAllBytes(stagedPath)) != document.SourceDigest)
+ {
+ try
+ {
+ File.Copy(blobPath, stagedPath, overwrite: true);
+ }
+ catch (FileNotFoundException)
+ {
+ return null; // source artifact was evicted/deleted concurrently
+ }
+ }
+
+ return stagedPath;
}🤖 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 `@OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs` around lines
53 - 68, TryStageSourceFileForOpen is reusing a predictable shared temp path,
which can allow a pre-planted file to be opened instead of the real source, and
its unguarded copy can throw if the artifact is evicted mid-operation. Update
the staging logic in FabricLibraryService so it verifies the staged file still
matches the expected source digest before reusing it, rather than trusting
File.Exists alone. Also wrap the File.Copy path in handling for missing/evicted
blobs so the method returns null instead of letting an exception escape to
SourcePreviewPanel.TryOpenSourceFile. Keep the fix centered on
TryStageSourceFileForOpen and the stagedPath/blobPath flow.
5 tasks
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
sourceLabelvalues likeS1back to real segment IDs before citation verificationChecks
dotnet test .\OrchestratorIDE.UnitTests\OrchestratorIDE.UnitTests.csproj --no-restore --filter FullyQualifiedName~ContextFabricAskServiceTests— passed, 6/6dotnet test .\OrchestratorIDE.Avalonia.HeadlessTests\OrchestratorIDE.Avalonia.HeadlessTests.csproj --no-restore --filter FullyQualifiedName~OrcChatContextFabricQueryTests— passed, 1/1dotnet build .\OrchestratorIDE.NativeRuntime\OrchestratorIDE.NativeRuntime.csproj --no-restore— passed, 2 existing nullable warnings inContextFabricValidation.csNotes
origin/masterbefore opening this PR.Summary by CodeRabbit