CF-3 native reader framework - #12
Merged
Merged
Conversation
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughContext Fabric now includes new graph storage, evidence import, native reading, stitching, search, tooling, fixtures, tests, and documentation updates. The PR also changes model admission and prompt fallback behavior, adds local repo settings, and updates the UITests MJPEG encoder for SharpAvi. ChangesContext Fabric, runtime, tests, docs, and wiring
SharpAvi encoder update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 14
🧹 Nitpick comments (1)
OrchestratorIDE.UITests/TestVideoRecorder.cs (1)
255-265: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSlice to the required frame size before allocating.
source.ToArray()copies the entire span, but only_height * stridebytes are used. Since the array path passessource.AsSpan(srcOffset), this can over-allocate if the backing buffer contains trailing data.Proposed localized refactor
private byte[] EncodeFrameToJpeg(ReadOnlySpan<byte> source) { - var raw = source.ToArray(); using var bmp = new Bitmap(_width, _height, PixelFormat.Format32bppRgb); var bits = bmp.LockBits(new Rectangle(0, 0, _width, _height), ImageLockMode.WriteOnly, PixelFormat.Format32bppRgb); try { var stride = Math.Abs(bits.Stride); - Marshal.Copy(raw, 0, bits.Scan0, _height * stride); + var byteCount = checked(_height * stride); + if (source.Length < byteCount) + throw new ArgumentException("Source frame is smaller than the expected bitmap size.", nameof(source)); + + var raw = source[..byteCount].ToArray(); + Marshal.Copy(raw, 0, bits.Scan0, byteCount); } finally { bmp.UnlockBits(bits); }🤖 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.UITests/TestVideoRecorder.cs` around lines 255 - 265, In EncodeFrameToJpeg, avoid copying the entire source span before it is known how many bytes are needed; the current source.ToArray() can over-allocate when called from the array path via source.AsSpan(srcOffset). Slice the input to the exact frame payload size needed for _height * stride before converting to an array or copying into the Bitmap, and keep the change localized in EncodeFrameToJpeg so the frame encoding path only processes the required bytes.
🤖 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 `@docs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.md`:
- Around line 27-43: The manifest table currently documents `DownloadedAtUtc` in
a way that conflicts with `ContextFabricCf1Tests`, which reads it as a
`DateTimeOffset`; update the entry in the benchmark manifest docs so the field’s
type/representation matches the actual contract. Use the `DownloadedAtUtc` row
in the table and describe it as `DateTimeOffset` serialized as ISO-8601, instead
of a plain string, so fixture authors follow the correct schema.
In `@docs/The` Orc Context Fabric.md:
- Around line 3-5: The status banner in TheOrc Context Fabric is out of sync
with the rest of the document because it only reflects CF-0/CF-1 while later
sections reference CF-2 and CF-3 passed exits. Update the banner text near the
top to either include the newer CF-2 and CF-3 status or explicitly state that
the banner is limited to CF-1-only progress, matching the terminology used in
the document.
In `@OrchestratorIDE.UnitTests/ContextFabricCf1Tests.cs`:
- Around line 603-619: The manifest verification in the shared assertion block
is missing checks for two invariants carried by DarwinFixtureManifest: MediaType
and SegmenterVersion. Update the existing Assert.Multiple in
ContextFabricCf1Tests, alongside the current imported.Document and
imported.Segments assertions, to validate imported.Document.MediaType and
imported.Document.SegmenterVersion against the manifest so parser-routing or
chunker-version regressions are caught. Apply the same fix in the other
referenced assertion block as well, using the existing imported, rebuilt, and
manifest symbols to keep the helper comprehensive.
In `@OrchestratorIDE.UnitTests/ContextFabricCf2Tests.cs`:
- Around line 70-107: The seeded provenance spans in ContextFabricCf2Tests are
using hard-coded end offsets that don’t match the actual fixture text. Update
the affected setup blocks around FabricLibraryRepository.ReplaceDocument,
FabricClaimEntry, and FabricClaimCitationEntry so char_end/CharEnd is derived
from the corresponding string length (for both segment text and quote text)
instead of literal numbers. Apply the same fix across all listed test sections
to keep the stored offsets aligned with the fixture content.
In
`@OrchestratorIDE.UnitTests/TestData/ContextFabric/united-states-constitution-full.txt`:
- Around line 285-287: The fixture still contains webpage chrome and duplicated
amendment text that should not be part of the Constitution corpus. Remove the
navigation/footer copy and the repeated amendment block from
united-states-constitution-full.txt, or otherwise rename the fixture and its
manifest entry so the content is clearly treated as a scraped webpage; use the
surrounding Constitution text and the duplicated section to locate the cleanup.
In `@OrchestratorIDE/Core/Runtime/NativePromptBuilder.cs`:
- Around line 62-65: The Gemma fallback in NativePromptBuilder is altering
prompt content by trimming message bodies before appending them. Remove the
Trim() call in the message formatting path so the original msg.Content is
preserved exactly for system, tool, user, and code messages, while keeping the
existing role handling and Tool result prefix logic intact.
In `@OrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.cs`:
- Around line 48-49: The citation upsert in DocumentGraphRepository should
always bind inserted rows to the parent claim being saved, not to each
citation’s existing ClaimId. In the delete-and-reinsert flow around the citation
write logic, update the insert path so the values written by the citation loop
use claim.ClaimId consistently (including any helper used for citation inserts),
preventing mismatched citation objects from attaching to a different claim. Make
this change in the same repository method that executes the DELETE and
subsequent citation inserts.
In `@OrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.cs`:
- Around line 71-82: The stitch validation in FabricBoundaryStitcher currently
only checks that draft.LinkedFacts has enough items, so unrelated facts can pass
the gate. Update the validation logic to iterate through
testCase.ExpectedLinkedFacts and verify each expected fact is present in
draft.LinkedFacts, while keeping the existing forbidden-term and summary checks.
Use the existing draft.LinkedFacts, testCase.ExpectedLinkedFacts, and errors
collection in the boundary validation flow to enforce exact expected fact
coverage.
In `@OrchestratorIDE/Services/ContextFabric/FabricEvidenceGraphImporter.cs`:
- Around line 22-28: The citation import path in FabricEvidenceGraphImporter
allows a citation to override the card’s segment without checking that the
replacement belongs to the same document/corpus. Update the logic that builds
citations so any explicit citation SegmentId is validated against the same
document boundary as the card/segment currently being imported, using the
existing identity checks around documentId/corpusId and the citation handling
near the citation loop. If the citation segment does not match the current
document, reject it with an InvalidDataException instead of importing it.
- Around line 32-35: Blank or null ClaimId values in FabricEvidenceGraphImporter
cause BuildScopedClaimId to generate the same scoped id for multiple claims in a
segment, so the later UpsertClaim can overwrite the earlier one. Update the
claim import flow in the loop over card.Claims to reject empty ClaimId values or
generate a unique fallback using an additional component such as the loop index
or a text hash, and apply the same fix wherever the scoped claim id is built
later in the importer so each claim maps to a distinct id.
In `@OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs`:
- Around line 116-120: Artifact GC currently only iterates over
_artifacts.GetDigests(), so orphan .part files are skipped and never deleted.
Update FabricLibraryService’s cleanup logic to also enumerate partial artifacts
from the artifact store (or equivalent partial-file listing), then apply the
same referenced/dangling check and DeleteIfPresent path used for completed
digests. Keep the fix centered in the GC flow around
_repository.ListReferencedArtifactDigests(), _artifacts.GetDigests(), and
_artifacts.DeleteIfPresent().
- Line 109: DeleteCorpus is still a mutation path and needs the same mutation
lock as the other corpus-changing operations. Update
FabricLibraryService.DeleteCorpus to acquire the existing mutation gate before
calling _repository.DeleteCorpus, using the same locking flow already used
around import/rebuild mutations so it cannot race pre-gate reads or GC reference
snapshots.
In `@OrchestratorIDE/Services/ContextFabric/FabricNativeReaderService.cs`:
- Around line 44-46: The import loop in FabricNativeReaderService.Read currently
only upserts accepted cards via _graphImporter.ImportEvidenceCard, which can
leave stale document graph rows behind on rereads. Update the read flow to make
the import transactional and replace the document/segment-scoped graph slice
before inserting current results, using the existing readReport and
_graphImporter path so previously emitted claims/entities that are no longer
present are deleted or overwritten first.
In `@OrchestratorIDE/Tools/FabricTools.cs`:
- Around line 29-30: The handlers in FabricTools.cs create a SqliteStore and
immediately build repositories without bootstrapping the database, so update
each of the four SqliteStore call sites to invoke Initialize() before
constructing FabricLibraryRepository or any other repository. Keep the change
local to the affected handler methods so every path that uses a fresh
SqliteStore performs the same setup before use.
---
Nitpick comments:
In `@OrchestratorIDE.UITests/TestVideoRecorder.cs`:
- Around line 255-265: In EncodeFrameToJpeg, avoid copying the entire source
span before it is known how many bytes are needed; the current source.ToArray()
can over-allocate when called from the array path via source.AsSpan(srcOffset).
Slice the input to the exact frame payload size needed for _height * stride
before converting to an array or copying into the Bitmap, and keep the change
localized in EncodeFrameToJpeg so the frame encoding path only processes the
required bytes.
🪄 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: f97ac7f8-be99-4f44-86cd-c24777a42134
⛔ Files ignored due to path filters (1)
OrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-primary.pdfis excluded by!**/*.pdf
📒 Files selected for processing (54)
.gitattributes.gitignoreOrchestratorIDE.Avalonia/App.axaml.csOrchestratorIDE.Avalonia/MainWindow.axaml.csOrchestratorIDE.Avalonia/OrchestratorIDE.Avalonia.csprojOrchestratorIDE.NativeRuntime/OrchestratorIDE.NativeRuntime.csprojOrchestratorIDE.UITests/OrchestratorIDE.UITests.csprojOrchestratorIDE.UITests/TestVideoRecorder.csOrchestratorIDE.UnitTests/ContextFabricCf1Tests.csOrchestratorIDE.UnitTests/ContextFabricCf2Tests.csOrchestratorIDE.UnitTests/ContextFabricCf3Tests.csOrchestratorIDE.UnitTests/ModelAdmissionGateTests.csOrchestratorIDE.UnitTests/ModelDepotTests.csOrchestratorIDE.UnitTests/NativePromptBuilderTests.csOrchestratorIDE.UnitTests/OrchestratorIDE.UnitTests.csprojOrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-2009.manifest.jsonOrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-2009.txtOrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-pdf-candidates.jsonOrchestratorIDE.UnitTests/TestData/ContextFabric/darwin-origin-species-primary-pdf.manifest.jsonOrchestratorIDE.UnitTests/TestData/ContextFabric/the-federalist-papers.manifest.jsonOrchestratorIDE.UnitTests/TestData/ContextFabric/the-federalist-papers.txtOrchestratorIDE.UnitTests/TestData/ContextFabric/united-states-constitution-full.manifest.jsonOrchestratorIDE.UnitTests/TestData/ContextFabric/united-states-constitution-full.txtOrchestratorIDE/Core/Runtime/LLamaSharpRuntime.csOrchestratorIDE/Core/Runtime/ModelAdmissionGate.csOrchestratorIDE/Core/Runtime/NativePromptBuilder.csOrchestratorIDE/Core/ScreenRecorder.csOrchestratorIDE/Research/OrcChatToolCatalog.csOrchestratorIDE/Services/ContextFabric/ContextFabricBenchmarkExpansionRunner.csOrchestratorIDE/Services/ContextFabric/ContextFabricContracts.csOrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.csOrchestratorIDE/Services/ContextFabric/ContextFabricIngestionContracts.csOrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.csOrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.csOrchestratorIDE/Services/ContextFabric/FabricDocumentParser.csOrchestratorIDE/Services/ContextFabric/FabricEvidenceGraphImporter.csOrchestratorIDE/Services/ContextFabric/FabricLibraryRepository.csOrchestratorIDE/Services/ContextFabric/FabricLibraryService.csOrchestratorIDE/Services/ContextFabric/FabricNativeReaderService.csOrchestratorIDE/Services/ContextFabric/FabricSearchService.csOrchestratorIDE/Services/ContextFabric/FabricSegmenter.csOrchestratorIDE/Services/Data/Migrations.csOrchestratorIDE/Services/Hive/ContentAddressedStore.csOrchestratorIDE/Services/Hive/DpapiSecretProtector.csOrchestratorIDE/Tools/FabricTools.csREADME.mddocs/ARCHITECTURE.mddocs/CONTEXT_FABRIC_BENCHMARK_CORPUS.mddocs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.mddocs/CONTEXT_FABRIC_CRITIQUE_TRIAGE.mddocs/CONTEXT_FABRIC_PUBLIC_COPY.mddocs/MODEL_ADMISSION_GATE.mddocs/ROADMAP.mddocs/The Orc Context Fabric.md
Comment on lines
+285
to
+287
| For biographies of the non-signing delegates to the Constitutional Convention, see the Founding Fathers page. | ||
|
|
||
| Back to Main Constitution Page |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Trim webpage chrome from this benchmark fixture.
Line 285, Line 287, Line 373, and Line 619 are navigation/footer copy, and the amendment material is duplicated within Lines 289-367. Because the manifest pins this raw text, those lines become searchable corpus content and will skew segment IDs, FTS hits, and native-reader output away from the Constitution itself. Strip the page chrome/duplicate block, or rename the fixture + manifest so they clearly represent a scraped webpage rather than the document corpus.
Also applies to: 289-373, 619-619
🤖 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.UnitTests/TestData/ContextFabric/united-states-constitution-full.txt`
around lines 285 - 287, The fixture still contains webpage chrome and duplicated
amendment text that should not be part of the Constitution corpus. Remove the
navigation/footer copy and the repeated amendment block from
united-states-constitution-full.txt, or otherwise rename the fixture and its
manifest entry so the content is clearly treated as a scraped webpage; use the
surrounding Constitution text and the duplicated section to locate the cleanup.
Comment on lines
+29
to
+30
| using var store = new SqliteStore(dbRoot); | ||
| var repo = new FabricLibraryRepository(store); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize SqliteStore before constructing the repositories.
These handlers open SqliteStore directly, but unlike the existing callers they never call Initialize(). That works only if some other code path already bootstrapped the workspace DB; OrcChatToolCatalog.CreateWorkspaceTools now exposes these tools without that guarantee.
Suggested fix
using var store = new SqliteStore(dbRoot);
+ store.Initialize();
var repo = new FabricLibraryRepository(store);Apply the same change in all four handlers that create a SqliteStore.
Also applies to: 65-68, 108-109, 159-160
🤖 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/Tools/FabricTools.cs` around lines 29 - 30, The handlers in
FabricTools.cs create a SqliteStore and immediately build repositories without
bootstrapping the database, so update each of the four SqliteStore call sites to
invoke Initialize() before constructing FabricLibraryRepository or any other
repository. Keep the change local to the affected handler methods so every path
that uses a fresh SqliteStore performs the same setup before use.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Services/ContextFabric/DocumentGraphRepository.cs`:
- Around line 50-57: ReplaceClaimsForDocument is upserting claims using each
claim’s own DocumentId after deleting only the target document’s rows, so a
replacement payload can leak claims outside the intended scope. In
DocumentGraphRepository.ReplaceClaimsForDocument, validate that every claim in
claims belongs to the provided documentId before any delete/upsert work, and
reject mismatched claims (or force them to the target documentId if that is the
intended behavior). Keep the fix anchored around ReplaceClaimsForDocument and
UpsertClaimOn so the replacement operation stays confined to one document.
In `@OrchestratorIDE/Services/ContextFabric/FabricEvidenceGraphImporter.cs`:
- Around line 32-37: The document validation in
FabricEvidenceGraphImporter.ImportEvidence only inspects the first imported
claim, so mixed-card inputs can slip through. Update ImportEvidence to validate
every result from BuildClaimImports before deleting/replacing evidence, using
the existing documentId and Document/DocumentId checks across the full imports
collection, and reject the batch if any card or claim belongs to a different
document.
🪄 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: 9a90fe5d-e699-4be4-8b37-230713c52417
📒 Files selected for processing (14)
OrchestratorIDE.UITests/TestVideoRecorder.csOrchestratorIDE.UnitTests/ContextFabricCf1Tests.csOrchestratorIDE.UnitTests/ContextFabricCf2Tests.csOrchestratorIDE.UnitTests/ContextFabricCf3Tests.csOrchestratorIDE.UnitTests/NativePromptBuilderTests.csOrchestratorIDE/Core/Runtime/NativePromptBuilder.csOrchestratorIDE/Services/ContextFabric/DocumentGraphRepository.csOrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.csOrchestratorIDE/Services/ContextFabric/FabricEvidenceGraphImporter.csOrchestratorIDE/Services/ContextFabric/FabricLibraryService.csOrchestratorIDE/Services/ContextFabric/FabricNativeReaderService.csOrchestratorIDE/Services/Hive/ContentAddressedStore.csdocs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.mddocs/The Orc Context Fabric.md
✅ Files skipped from review due to trivial changes (1)
- docs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.md
🚧 Files skipped from review as they are similar to previous changes (8)
- OrchestratorIDE/Core/Runtime/NativePromptBuilder.cs
- OrchestratorIDE/Services/ContextFabric/FabricNativeReaderService.cs
- OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs
- OrchestratorIDE.UITests/TestVideoRecorder.cs
- OrchestratorIDE/Services/Hive/ContentAddressedStore.cs
- docs/The Orc Context Fabric.md
- OrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.cs
- OrchestratorIDE.UnitTests/ContextFabricCf1Tests.cs
Contributor
There was a problem hiding this comment.
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 (2)
OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs (2)
61-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate corpus/document state after acquiring the mutation gate.
The current checks happen before the gate and before file I/O, so a delete can invalidate the corpus/document before
ImportBytesAsyncpersists the replacement.Proposed fix
await _mutationGate.WaitAsync(ct).ConfigureAwait(false); try { + if (_repository.GetCorpus(corpusId) is null) + throw new KeyNotFoundException($"Context Fabric corpus '{corpusId}' does not exist."); + return await ImportBytesAsync( corpusId, info.Name, mediaType ?? InferMediaType(info.Extension), bytes, @@ await _mutationGate.WaitAsync(ct).ConfigureAwait(false); try { + if (_repository.GetDocument(documentId) is null) + throw new KeyNotFoundException($"Context Fabric document '{documentId}' does not exist."); + if (_repository.GetCorpus(existing.CorpusId) is null) + throw new KeyNotFoundException($"Context Fabric corpus '{existing.CorpusId}' does not exist."); + return await ImportBytesAsync( existing.CorpusId, existing.DisplayName,Also applies to: 91-101
🤖 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 61 - 69, Revalidate the corpus/document state after entering the mutation gate in FabricLibraryService.ImportBytesAsync so a delete cannot race with the import. Move or repeat the corpus/document existence checks inside the _mutationGate.WaitAsync block, immediately before the import work/persistence, and apply the same safeguard to the other affected import path in FabricLibraryService. Ensure the gate-protected path uses the latest state before calling ImportBytesAsync so stale pre-checks are not relied on.
191-193: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear oversized partials before failing or retrying.
When a stale
.partis longer than the expected object, this throws but leaves the bad partial behind, so every later import of the same digest fails the same way.Proposed fix
var offset = _artifacts.GetResumeOffset(digest); if (offset > bytes.LongLength) - throw new InvalidDataException($"Partial content-addressed object '{digest}' exceeds the expected size."); + { + _artifacts.DeleteIfPresent(digest); + offset = 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/FabricLibraryService.cs` around lines 191 - 193, The oversized partial check in FabricLibraryService.GetResumeOffset leaves a stale .part file behind before throwing, so future imports of the same digest keep failing. Update the oversized-content path around _artifacts.GetResumeOffset/digest handling to clear or delete the partial artifact before raising InvalidDataException, and keep the cleanup local to the resume/validation flow so subsequent retries can start cleanly.
🤖 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.UITests/TestVideoRecorder.cs`:
- Around line 257-265: Restore the explicit frame-size validation in
EncodeFrameToJpeg before the Marshal.Copy call, since the current path no longer
rejects truncated input up front and can fail with a low-level exception. Add
the length guard back using the existing raw buffer and bits/stride values in
TestVideoRecorder so short frames are detected and handled before copying into
the Bitmap.
---
Outside diff comments:
In `@OrchestratorIDE/Services/ContextFabric/FabricLibraryService.cs`:
- Around line 61-69: Revalidate the corpus/document state after entering the
mutation gate in FabricLibraryService.ImportBytesAsync so a delete cannot race
with the import. Move or repeat the corpus/document existence checks inside the
_mutationGate.WaitAsync block, immediately before the import work/persistence,
and apply the same safeguard to the other affected import path in
FabricLibraryService. Ensure the gate-protected path uses the latest state
before calling ImportBytesAsync so stale pre-checks are not relied on.
- Around line 191-193: The oversized partial check in
FabricLibraryService.GetResumeOffset leaves a stale .part file behind before
throwing, so future imports of the same digest keep failing. Update the
oversized-content path around _artifacts.GetResumeOffset/digest handling to
clear or delete the partial artifact before raising InvalidDataException, and
keep the cleanup local to the resume/validation flow so subsequent retries can
start cleanly.
🪄 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: 673f5ca1-649e-4efe-a188-95dfe421da63
📒 Files selected for processing (8)
OrchestratorIDE.UITests/TestVideoRecorder.csOrchestratorIDE.UnitTests/ContextFabricCf1Tests.csOrchestratorIDE.UnitTests/NativePromptBuilderTests.csOrchestratorIDE/Core/Runtime/NativePromptBuilder.csOrchestratorIDE/Services/ContextFabric/FabricLibraryService.csOrchestratorIDE/Services/Hive/ContentAddressedStore.csdocs/CONTEXT_FABRIC_BENCHMARK_CORPUS.mddocs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.md
💤 Files with no reviewable changes (3)
- OrchestratorIDE.UnitTests/NativePromptBuilderTests.cs
- OrchestratorIDE/Services/Hive/ContentAddressedStore.cs
- OrchestratorIDE.UnitTests/ContextFabricCf1Tests.cs
✅ Files skipped from review due to trivial changes (1)
- docs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.md
🚧 Files skipped from review as they are similar to previous changes (2)
- OrchestratorIDE/Core/Runtime/NativePromptBuilder.cs
- docs/CONTEXT_FABRIC_BENCHMARK_CORPUS.md
Comment on lines
+257
to
+265
| var raw = source.ToArray(); | ||
| using var bmp = new Bitmap(_width, _height, PixelFormat.Format32bppRgb); | ||
| var bits = bmp.LockBits(new Rectangle(0, 0, _width, _height), | ||
| ImageLockMode.WriteOnly, | ||
| PixelFormat.Format32bppRgb); | ||
| try | ||
| { | ||
| var stride = Math.Abs(bits.Stride); | ||
| Marshal.Copy(source, srcOffset, bits.Scan0, _height * stride); | ||
| Marshal.Copy(raw, 0, bits.Scan0, _height * stride); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'OrchestratorIDE.UITests/TestVideoRecorder.cs'
wc -l OrchestratorIDE.UITests/TestVideoRecorder.cs
sed -n '220,290p' OrchestratorIDE.UITests/TestVideoRecorder.csRepository: hardcoreerik/TheOrc
Length of output: 2246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "EncodeFrameToJpeg|EncodeFrame\\(" OrchestratorIDE.UITests/TestVideoRecorder.cs OrchestratorIDE.UITests -g '!**/bin/**' -g '!**/obj/**'Repository: hardcoreerik/TheOrc
Length of output: 1307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' OrchestratorIDE.UITests/TestVideoRecorder.csRepository: hardcoreerik/TheOrc
Length of output: 7842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "_height \\* stride|byteCount|ArgumentException|Marshal\\.Copy\\(" OrchestratorIDE.UITests/TestVideoRecorder.csRepository: hardcoreerik/TheOrc
Length of output: 293
Restore the frame-size check in EncodeFrameToJpeg. Marshal.Copy now handles short buffers implicitly, so a truncated frame raises a low-level exception instead of an explicit argument check. Add the length guard back before copying.
🤖 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.UITests/TestVideoRecorder.cs` around lines 257 - 265, Restore
the explicit frame-size validation in EncodeFrameToJpeg before the Marshal.Copy
call, since the current path no longer rejects truncated input up front and can
fail with a low-level exception. Add the length guard back using the existing
raw buffer and bits/stride values in TestVideoRecorder so short frames are
detected and handled before copying into the Bitmap.
This was referenced Jun 29, 2026
6 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
Verification
dotnet test OrchestratorIDE.UnitTests/OrchestratorIDE.UnitTests.csproj --filter "FullyQualifiedName~ContextFabric"Summary by CodeRabbit