Tier 2.5 reference chasing + boundary-stitch schema fix + CF model pinning - #42
Merged
Conversation
Findings-driven fixes from the first full 120-question run (58/120):
1. Reference chasing (Tier 2.5): 18/24 MultiHop misses were partial
retrieval -- chain segments linked by identifiers (RPT-064) the question
never names. BuildEvidencePack now follows rare tracked identifiers from
included cards into the cards they link to, iteratively (5-hop chains),
doc-frequency-capped so corpus-filler codes are never chased.
2. Multi-hop answer prompt: the remaining 6/24 retrieved everything but
answered tersely (final value only, one citation). The prompt now
mechanically requires the full chain spelled out and one citation per
link.
3. Boundary-stitch root cause CLOSED (CF_TEST_RESULTS.md section 5):
full-raw-output capture (THEORC_STITCH_RAW_DIR) showed Meta-Llama emits
linkedFacts as objects ({factId,text,type}) instead of strings.
NormalizeLinkedFactObjects flattens that shape before parsing; prompt now
demands plain strings and the referenced fact as its own entry.
Live re-run on Meta-Llama: 0/2 -> 2/2 PASS.
4. CF model pinning: --model <substring> / THEORC_CF_MODEL / -Model script
param restrict depot role resolution via ModelDepot.WithBaseModelFilter,
so shared-depot changes (models toggled by Foundry work) cannot silently
swap or break a benchmark run (two exit-66 false starts on 2026-07-07).
Unit suite: 496 total, 0 failed (6 new tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR tightens Context Fabric evidence selection, adds multi-hop reference chasing, normalizes stitched ChangesContext Fabric changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildEvidencePack
participant TryInclude
participant ChaseTrackedReferences
BuildEvidencePack->>TryInclude: evaluate candidate against EvidenceLimit
TryInclude-->>BuildEvidencePack: include or skip
BuildEvidencePack->>ChaseTrackedReferences: chase tracked identifiers in included cards
ChaseTrackedReferences-->>BuildEvidencePack: bounded chased evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tools/ContextFabricBench/Program.cs (1)
140-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisleading diagnostics when the pin matches nothing.
depotis reassigned to the filtered result beforePrintDepotDiagnostics(depot, modelRoot)is called on failure (line 156). IfmodelPinmatches zero base models, the diagnostics will report(no active base-model .gguf files found)even though the model root actually contains models — just none matching the substring. The "Model pin" line printed earlier gives a partial hint, but the failure diagnostics themselves are factually wrong about the depot's real contents, which can misdirect troubleshooting on long (4-6 hour) benchmark runs.🩹 Proposed fix: clarify the pin-mismatch case
if (researcher is null || (reviewer is null && requiresReviewer)) { Console.Error.WriteLine($"No native base GGUF was resolved beneath '{Path.GetFullPath(modelRoot)}'."); + if (!string.IsNullOrWhiteSpace(modelPin)) + Console.Error.WriteLine($" Model pin '{modelPin}' may have filtered out all otherwise-eligible base models."); PrintDepotDiagnostics(depot, modelRoot); return 65; }🤖 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/ContextFabricBench/Program.cs` around lines 140 - 158, The failure diagnostics in Program.cs are misleading after applying modelPin because depot is overwritten before PrintDepotDiagnostics is called, so a pin that matches nothing looks like the model root has no GGUFs at all. Update the flow around ModelDepot.Scan, WithBaseModelFilter, and PrintDepotDiagnostics so the diagnostic output can distinguish “no models under modelRoot” from “modelPin filtered out all models” (for example by preserving the unfiltered depot or passing the pin context into the diagnostics). Keep the existing Model pin message, but make the error path report the actual mismatch clearly when ResolveRole fails.
🧹 Nitpick comments (5)
OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs (1)
349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that no filler cards were chased.
This fixture’s filler cards do not match the question terms, so any
seg-fill-*inclusion would indicate chase leakage.Count < 7would still pass if the chase incorrectly added only part of the filler set.Proposed test tightening
Assert.That(pack.IncludedSegmentIds, Does.Contain("seg-seed")); - // Filler cards may enter via normal term scoring, but the chase must not add all of - // them: the identifier occurs in 7 cards, above the cap of 4. - Assert.That(pack.IncludedSegmentIds.Count, Is.LessThan(7)); + Assert.That(pack.IncludedSegmentIds.Where(id => id.StartsWith("seg-fill-", StringComparison.Ordinal)), + Is.Empty);🤖 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/ContextFabricEvidencePackTests.cs` around lines 349 - 352, The test in ContextFabricEvidencePackTests should be tightened because Count < 7 still allows partial filler-card chase leakage; update the assertions around IncludedSegmentIds so they explicitly verify that no seg-fill-* identifiers are present, while keeping the existing seg-seed check. Use the pack.IncludedSegmentIds collection directly in the test to assert absence of any filler segment IDs rather than relying on a count threshold.Tools/ContextFabricBench/Program.cs (1)
473-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUsage text doesn't document
--model.
PrintUsage(lines 504-520) isn't updated to mention the new--model <substring>option or itsTHEORC_CF_MODELenv-var equivalent, so operators running--helpwon't discover the pinning feature this PR adds.📝 Proposed usage line
Console.WriteLine(" --max-questions <n> Cap questions processed (useful for smoke tests; default: all)"); + Console.WriteLine(" --model <substring> Pin role resolution to a base GGUF matching this DisplayName substring (env: THEORC_CF_MODEL)"); Console.WriteLine(" --segments <n> Scale-suite segment count (default 640)");🤖 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/ContextFabricBench/Program.cs` around lines 473 - 478, Update PrintUsage so it documents the new model pinning option added in CliOptions parsing: mention --model <substring> and its THEORC_CF_MODEL environment-variable equivalent alongside the other CLI flags. Make sure the usage text clearly explains that this maps to the modelPin value so operators can discover the feature from --help.Tools/ContextFabricBench/Run-CF7GateExpanded.ps1 (2)
113-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment-based help doesn't document
-Model.The
.PARAMETERblock above (coveringMaxQuestions,Context,GpuLayers,SkipBuild,LogFile) has no corresponding entry for the new-Modelparameter, soGet-Helpoutput won't describe it despite the inline comment explaining its purpose.📝 Proposed doc addition
.PARAMETER LogFile Path to write a copy of stdout. Defaults to OutputDir/cf7_expanded_<timestamp>_console.log. Set to empty string to disable log capture. + +.PARAMETER Model + Pin role resolution to a GGUF whose filename contains this substring (e.g. "Meta-Llama"). + Protects the run from other models being added/disabled in a shared depot mid-run.🤖 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/ContextFabricBench/Run-CF7GateExpanded.ps1` around lines 113 - 116, The comment-based help for Run-CF7GateExpanded.ps1 is missing documentation for the new Model parameter, so update the .PARAMETER block that already documents MaxQuestions, Context, GpuLayers, SkipBuild, and LogFile to include Model. Describe its purpose consistently with the inline comment near the parameter declaration so Get-Help on Run-CF7GateExpanded and its parameter metadata includes -Model.
249-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analyzer:
$Argsshadows the automatic variable.PSScriptAnalyzer flags this reuse of the automatic
$args/$Argsvariable name (case-insensitive in PowerShell). This is a pre-existing pattern in the file (line 232) extended here rather than introduced, but a rename would be a clean, low-risk fix given the linter is already flagging it.♻️ Proposed rename (applies to line 232 onward too)
-$Args = @( +$BenchArgs = @( "--suite", "cf7-gate-expanded", ... ) ... -if ($Model) { - $Args += @("--model", $Model) -} +if ($Model) { + $BenchArgs += @("--model", $Model) +}🤖 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/ContextFabricBench/Run-CF7GateExpanded.ps1` around lines 249 - 252, The script uses `$Args`, which conflicts with PowerShell’s automatic `$args` variable and triggers the analyzer; rename the collection variable in `Run-CF7GateExpanded.ps1` from the initial declaration at the earlier `$Args` setup through this `if ($Model)` append block, and update all references consistently so the argument list uses a non-automatic name throughout.Source: Linters/SAST tools
OrchestratorIDE.UnitTests/ContextFabricCf3Tests.cs (1)
218-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the
factfallback branch too.
NormalizeLinkedFactObjectsaccepts bothtextandfact; the new tests only covertext, so the fallback can regress unnoticed.Suggested test
+ [Test] + public void NormalizeLinkedFactObjects_FlattensObjectShapedFacts_ToTheirFactProperty() + { + const string raw = """{"schemaVersion":"cf0-stitch-1.0","caseId":"c1","summary":"S.","linkedFacts":[{"fact":"A fact-only sentence."}]}"""; + + var normalized = FabricBoundaryStitcher.NormalizeLinkedFactObjects(raw); + var draft = FabricJson.ParseModelObject<FabricBoundaryStitchDraft>(normalized); + + Assert.That(draft.LinkedFacts, Is.EqualTo(new[] { "A fact-only sentence." })); + }🤖 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/ContextFabricCf3Tests.cs` around lines 218 - 241, Add a test that covers the `fact` fallback path in `FabricBoundaryStitcher.NormalizeLinkedFactObjects`, since the current `NormalizeLinkedFactObjects_FlattensObjectShapedFacts_ToTheirTextProperty` case only verifies object-shaped linked facts using `text`. Create a similar input payload where `linkedFacts` objects use `fact` instead of `text`, then parse it with `FabricJson.ParseModelObject<FabricBoundaryStitchDraft>` and assert `draft.LinkedFacts` is flattened to the fallback fact strings.
🤖 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/ContextFabricFeasibilityRunner.cs`:
- Around line 786-809: The reference-chasing step in
ContextFabricFeasibilityRunner should happen before the greedy fill loop
exhausts the EvidenceLimit, since ChaseTrackedReferences is currently delayed
until after all candidates are processed. Rework the flow around the candidate
selection loop and TryInclude so you first include the seed/coverage cards,
immediately chase their tracked references, and only then run the remaining
greedy fill over candidates to use any leftover budget.
- Around line 861-863: The carrier selection in ContextFabricFeasibilityRunner
is matching tracked identifiers by substring via ContainsAnchor, which can
incorrectly pull in cards like RPT-1000 when looking for RPT-100. Update the
filtering logic in the carriers query to use exact token membership from
ExtractTrackedIdentifiers instead of ContainsAnchor, and keep the existing
CardHaystack/identifier flow so the match only succeeds when the tracked
identifier is present as a discrete identifier.
In `@OrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.cs`:
- Around line 245-248: The fallback logic in FabricBoundaryStitcher’s text
extraction only checks fact when text is missing, so blank text values still
block a valid fact from being used. Update the loop in FabricBoundaryStitcher to
treat empty or whitespace text as absent and then fall back to obj["fact"]
before calling flattened.Add, keeping the existing null/whitespace guard in
place.
---
Outside diff comments:
In `@Tools/ContextFabricBench/Program.cs`:
- Around line 140-158: The failure diagnostics in Program.cs are misleading
after applying modelPin because depot is overwritten before
PrintDepotDiagnostics is called, so a pin that matches nothing looks like the
model root has no GGUFs at all. Update the flow around ModelDepot.Scan,
WithBaseModelFilter, and PrintDepotDiagnostics so the diagnostic output can
distinguish “no models under modelRoot” from “modelPin filtered out all models”
(for example by preserving the unfiltered depot or passing the pin context into
the diagnostics). Keep the existing Model pin message, but make the error path
report the actual mismatch clearly when ResolveRole fails.
---
Nitpick comments:
In `@OrchestratorIDE.UnitTests/ContextFabricCf3Tests.cs`:
- Around line 218-241: Add a test that covers the `fact` fallback path in
`FabricBoundaryStitcher.NormalizeLinkedFactObjects`, since the current
`NormalizeLinkedFactObjects_FlattensObjectShapedFacts_ToTheirTextProperty` case
only verifies object-shaped linked facts using `text`. Create a similar input
payload where `linkedFacts` objects use `fact` instead of `text`, then parse it
with `FabricJson.ParseModelObject<FabricBoundaryStitchDraft>` and assert
`draft.LinkedFacts` is flattened to the fallback fact strings.
In `@OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs`:
- Around line 349-352: The test in ContextFabricEvidencePackTests should be
tightened because Count < 7 still allows partial filler-card chase leakage;
update the assertions around IncludedSegmentIds so they explicitly verify that
no seg-fill-* identifiers are present, while keeping the existing seg-seed
check. Use the pack.IncludedSegmentIds collection directly in the test to assert
absence of any filler segment IDs rather than relying on a count threshold.
In `@Tools/ContextFabricBench/Program.cs`:
- Around line 473-478: Update PrintUsage so it documents the new model pinning
option added in CliOptions parsing: mention --model <substring> and its
THEORC_CF_MODEL environment-variable equivalent alongside the other CLI flags.
Make sure the usage text clearly explains that this maps to the modelPin value
so operators can discover the feature from --help.
In `@Tools/ContextFabricBench/Run-CF7GateExpanded.ps1`:
- Around line 113-116: The comment-based help for Run-CF7GateExpanded.ps1 is
missing documentation for the new Model parameter, so update the .PARAMETER
block that already documents MaxQuestions, Context, GpuLayers, SkipBuild, and
LogFile to include Model. Describe its purpose consistently with the inline
comment near the parameter declaration so Get-Help on Run-CF7GateExpanded and
its parameter metadata includes -Model.
- Around line 249-252: The script uses `$Args`, which conflicts with
PowerShell’s automatic `$args` variable and triggers the analyzer; rename the
collection variable in `Run-CF7GateExpanded.ps1` from the initial declaration at
the earlier `$Args` setup through this `if ($Model)` append block, and update
all references consistently so the argument list uses a non-automatic name
throughout.
🪄 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: 53b6aaf1-7e41-408f-9ef7-086a7c73c15f
📒 Files selected for processing (7)
OrchestratorIDE.UnitTests/ContextFabricCf3Tests.csOrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.csOrchestratorIDE/Core/Runtime/ModelDepot.csOrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.csOrchestratorIDE/Services/ContextFabric/FabricBoundaryStitcher.csTools/ContextFabricBench/Program.csTools/ContextFabricBench/Run-CF7GateExpanded.ps1
Comment on lines
+786
to
+809
| while (candidates.Count > 0) | ||
| { | ||
| var best = candidates | ||
| .OrderByDescending(c => c.Anchors | ||
| .Where(uncoveredAnchors.Contains) | ||
| .Sum(a => 1.0 / Math.Max(1, anchorDocumentFrequency[a])) | ||
| + PairScore(c.Pairs, uncoveredPairs)) | ||
| .ThenByDescending(c => c.Terms | ||
| .Where(uncoveredTerms.Contains) | ||
| .Sum(t => 1.0 / documentFrequency.GetValueOrDefault(t, 1))) | ||
| .ThenByDescending(c => c.AnchorScore) | ||
| .ThenByDescending(c => c.TermScore) | ||
| .ThenBy(c => c.Card.SegmentId, StringComparer.Ordinal) | ||
| .First(); | ||
| candidates.Remove(best); | ||
|
|
||
| if (!TryInclude(best.Card)) | ||
| continue; // skipped for size: its anchors/terms stay uncovered for later picks | ||
| uncoveredAnchors.ExceptWith(best.Anchors); | ||
| uncoveredPairs.ExceptWith(best.Pairs); | ||
| uncoveredTerms.ExceptWith(best.Terms); | ||
| } | ||
|
|
||
| ChaseTrackedReferences(cards, evidence, included, TryInclude); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Run reference chasing before greedy fill can consume the remaining budget.
Line 809 performs the chase only after the greedy loop has tried every term-overlap candidate. With a tight EvidenceLimit, lower-value greedy cards can fill the pack first, leaving no room for the linked chain cards this PR is trying to recover.
Consider a two-phase flow: include the best seed/coverage cards, chase their tracked references, then use greedy fill for any leftover budget.
🤖 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/ContextFabricFeasibilityRunner.cs`
around lines 786 - 809, The reference-chasing step in
ContextFabricFeasibilityRunner should happen before the greedy fill loop
exhausts the EvidenceLimit, since ChaseTrackedReferences is currently delayed
until after all candidates are processed. Rework the flow around the candidate
selection loop and TryInclude so you first include the seed/coverage cards,
immediately chase their tracked references, and only then run the remaining
greedy fill over candidates to use any leftover budget.
The first Tier 2.5 validation run (58/120, identical MultiHop 0/24 to the pre-fix baseline) showed the reference chase was a no-op. Root cause: ChaseTrackedReferences scanned CardHaystack (Summary + Claims.Text), which is the READER MODEL'S OWN PARAPHRASE of the segment -- tracked identifiers like RPT-064 routinely get reworded or dropped there. The verbatim text survives only in FabricCitation.Quote, which CardHaystack never included. Added a chase-only ChaseHaystack (CardHaystack + citation quotes) used solely by ChaseTrackedReferences; the shared scoring haystack used by anchor/term matching across Tiers 1/1.5/2 is untouched. Rewrote the three chase unit tests to use claim text that paraphrases the identifier away (matching real reader output) with the identifier only in the citation quote -- the original tests embedded identifiers directly in claim text, which is why they passed against the broken implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…was a no-op Honest record of the broken-chase run before the fix in 7d9e3b8 -- the byte-identical MultiHop retrieval distribution to the prior run is what exposed the bug. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live re-validation (still 58/120) showed the chase engaging but leaving long-hop chains partially retrieved (e.g. 1/5, 2/4 hits). Root cause: a legitimate N-hop chain's shared token appears in exactly N segments by construction, and the corpus's longest chains are 5 hops -- so the cap of 4 excluded real 4- and 5-hop chains as "filler" alongside genuine noise. Raised to 6, still well under the measured filler frequency (7+ cards). Also confirmed via live evidence: the 6 fully-retrieved 2-hop MultiHop failures are NOT a retrieval or chase problem at all -- both target segments were present in every case, but the model attached only one citation where the stricter verifier requires two. This is a genuine model-compliance gap (an 8B model under-following a multi-part citation instruction), not a code defect; logging it in CF_TEST_RESULTS.md as a known limitation rather than continuing to iterate the prompt blind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live evidence-card inspection replaced assumption: 2-hop failures are answer-citation-discipline gaps (retrieval succeeded), long-hop failures are the doc-frequency-cap bug fixed in 0f8338a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fourth live 120-question cycle in a row confirms citation-precision and boundary-stitch gates now pass and hold stable, but MultiHop stays flat. Direct inspection of multihop-chain-lh-002 (worst case, 1/5 retrieval) proves the chase itself is now correct: CHN-201's real document frequency is 4, well within the raised cap, and 3 of those 4 cards were confirmed to carry the identifier verbatim. The actual blocker is execution order -- BuildEvidencePack's greedy anchor-fill runs to completion (consuming almost all the token budget on distractor segments that share an entity name with the question but belong to an unrelated chain) before the chase ever gets a turn to add the real linked segments. Documented as a scoped architectural item (two candidate fixes, budget reservation vs. identifier-priority scoring) in CF_RETRIEVAL_IMPROVEMENT_PLAN.md rather than continuing to guess-and-check against 37-minute live runs -- four flat-score cycles (~2.5h of GPU time) is the point to stop and scope properly instead of trying a fifth tweak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the budget-ordering fix scoped in CF_RETRIEVAL_IMPROVEMENT_PLAN.md §3c (option 1): BuildEvidencePack's greedy anchor-fill now caps its own spend at 70% of EvidenceLimit for MultiHop questions specifically, leaving guaranteed headroom for ChaseTrackedReferences to add the segments a question's real chain links to, instead of letting the greedy fill consume the whole budget on distractor segments that share an entity name with the question but belong to an unrelated chain (the corpus does this by construction -- a small shared entityNames pool reused across many facts). Scoped to MultiHop only; every other question kind keeps the full budget Tiers 1/1.5/2 were validated against. Attempted a corpus-scale synthetic reproduction of the starvation scenario directly (two distractors sized to exhaust a tight budget) and found it too fragile for a trustworthy regression guard -- the greedy loop retries every candidate regardless of processing order, so undersized fixtures didn't reliably starve the real segments the way the dense live corpus does. Replaced with a narrower, verified-discriminating unit test: MultiHop must admit strictly fewer cards than an otherwise-identical GlobalSynthesis question under the same tight budget -- confirmed to fail without the fix and pass with it. Full validation is the live 120-question benchmark. Unit suite: 497 total, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
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)
OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs (1)
313-336: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the chase signal exclusive. These MultiHop chase tests still have enough overlap in
Summary/Claims.Textfor the ordinary greedy fill to include the cards, so they can pass even ifChaseTrackedReferencesregresses. Reword the claims so only the citation quote carries the linking identifier, or tighten the budget so the chase path is required.🤖 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/ContextFabricEvidencePackTests.cs` around lines 313 - 336, The MultiHop chase test still overlaps enough with the card summaries and claims that the normal greedy selection can pass without exercising the chase logic. Update BuildEvidencePack_ChasesTrackedIdentifier_IntoLinkedSegmentTheQuestionNeverNames by making the linking identifier appear only in the citation quote for the tracked cards, or otherwise reduce the budget/overlap so ContextFabricFeasibilityRunner.BuildEvidencePack must rely on ChaseTrackedReferences. Keep the assertion focused on IncludedSegmentIds for the linked segments.
🧹 Nitpick comments (1)
OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs (1)
363-388: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLoose assertion on filler exclusion.
Assert.That(pack.IncludedSegmentIds.Count, Is.LessThan(7))would also pass if several (up to 5) filler cards leaked into the pack via the chase. Since fillers share no terms/anchors with the question, a tighter assertion (e.g., asserting the count equals 1, or asserting none of the filler segment IDs are present) would more precisely validate that the frequency-cap skip actually fired.Suggested tightening
- Assert.That(pack.IncludedSegmentIds, Does.Contain("seg-seed")); - // Filler cards may enter via normal term scoring, but the chase must not add all of - // them: the identifier occurs in 7 cards, above the cap of 6. - Assert.That(pack.IncludedSegmentIds.Count, Is.LessThan(7)); + Assert.That(pack.IncludedSegmentIds, Does.Contain("seg-seed")); + // The identifier occurs in 7 cards, above the cap: no filler should be chased in. + Assert.That(pack.IncludedSegmentIds.Where(id => id.StartsWith("seg-fill-")), Is.Empty);🤖 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/ContextFabricEvidencePackTests.cs` around lines 363 - 388, The test in BuildEvidencePack_DoesNotChaseHighFrequencyFillerIdentifiers is too loose because it only checks that IncludedSegmentIds.Count is below 7, which still allows filler segments to slip in. Tighten the assertion on pack.IncludedSegmentIds so it verifies the chase was skipped by checking that only the seed segment is included, or by explicitly asserting none of the filler segment IDs from the fillers array are present. Keep the existing setup around ContextFabricFeasibilityRunner, FabricBenchmarkQuestion, and BuildEvidencePack, but make the expectation precise enough to prove the frequency-cap behavior fired.
🤖 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 `@OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs`:
- Around line 313-336: The MultiHop chase test still overlaps enough with the
card summaries and claims that the normal greedy selection can pass without
exercising the chase logic. Update
BuildEvidencePack_ChasesTrackedIdentifier_IntoLinkedSegmentTheQuestionNeverNames
by making the linking identifier appear only in the citation quote for the
tracked cards, or otherwise reduce the budget/overlap so
ContextFabricFeasibilityRunner.BuildEvidencePack must rely on
ChaseTrackedReferences. Keep the assertion focused on IncludedSegmentIds for the
linked segments.
---
Nitpick comments:
In `@OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.cs`:
- Around line 363-388: The test in
BuildEvidencePack_DoesNotChaseHighFrequencyFillerIdentifiers is too loose
because it only checks that IncludedSegmentIds.Count is below 7, which still
allows filler segments to slip in. Tighten the assertion on
pack.IncludedSegmentIds so it verifies the chase was skipped by checking that
only the seed segment is included, or by explicitly asserting none of the filler
segment IDs from the fillers array are present. Keep the existing setup around
ContextFabricFeasibilityRunner, FabricBenchmarkQuestion, and BuildEvidencePack,
but make the expectation precise enough to prove the frequency-cap behavior
fired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86509367-a776-4045-8e84-cc291853b27d
📒 Files selected for processing (2)
OrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.csOrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs
…compliance Fifth live 120-question cycle with the budget-reservation fix confirms it worked: MultiHop full-retrieval rose 6->9/24, the first change in the retrieval distribution across 5 cycles. Pass rate stayed flat anyway -- every one of the 9 now-fully-retrieved cases fails on the identical citation-discipline gap (1 of 2 or 0 of 4 required citations given) already diagnosed in run #11. This closes the retrieval-side investigation: the remaining MultiHop gap is Meta-Llama-3.1-8B not reliably complying with a multi-part citation instruction, not a retrieval or code defect. CF_RETRIEVAL_IMPROVEMENT_PLAN.md updated with the conclusion and three scoped next options (few-shot, schema-enforced citation count, defer to a larger model), recommending schema enforcement since it doesn't depend on model compliance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ChaseTrackedReferences carrier matching used ContainsAnchor (substring), so "RPT-1000" would wrongly count as a carrier for "RPT-100", pulling an unrelated card into a chain. Switched to exact membership via ExtractTrackedIdentifiers(ChaseHaystack(c)) -- keeping ChaseHaystack, not CardHaystack, since the latter is exactly the bug fixed in 7d9e3b8 (CodeRabbit's suggested diff would have reverted that fix). - NormalizeLinkedFactObjects only fell back to the "fact" property when "text" was missing/null; a model emitting "text": "" with a valid "fact" would silently drop the content. Now falls back on blank too. Third CodeRabbit finding (run reference chasing before the greedy fill, not after) is a real architectural point already captured as Option 2 in CF_RETRIEVAL_IMPROVEMENT_PLAN.md SS3c -- the budget-reservation fix shipped instead (Option 1) was chosen because it was live-validated (measured MultiHop full-retrieval 6->9/24) and lower-risk; reordering the two passes is a heavier restructuring left for a follow-up with its own validation cycle, not applied blind here. Unit suite: 497 total, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Whole tab now sits in a page-level ScrollViewer (right-side scrollbar reaches every section regardless of window size -- previously only the review queue could scroll internally). The three pipeline stages (Generate Dataset / Orc Academy / The Foundry) moved from stacked, individually-expandable accordion sections to numbered, color-coded cards laid out side by side across the full width, so the production line reads left-to-right instead of requiring one-at-a-time expand clicks. Internal field grids switched from horizontal Grid columns to vertical label-above-field stacking to fit the narrower per-stage columns; button rows use WrapPanel instead of a fixed-width row. Inventory tiles (datasets/adapters/models) gained a colored accent bar and a larger count number, reading as stat cards. Removed the now-orphaned Expander.Expanded handlers (ExpGen_Expanded / ExpForge_Expanded / ExpFoundry_Expanded); since all three stages are permanently visible instead of lazily expanded, their RefreshGen() / RefreshForge() / RefreshFoundry() calls now fire once automatically after the panel's initial data load instead of waiting for a user to expand a section that no longer exists. KNOWN ISSUE: opening a workspace folder while this tab is active has been reported to trigger a native stack-overflow crash. Investigated at length (WER crash dump -- unanalyzable, self-contained single-file deployments clean up their DAC/extraction directory on exit; isolated headless repro with the real repo's data, a real shown window, and window widths from 500-1900px -- did not reproduce it). Shipping as-is per explicit user decision; tracked for a follow-up patch once a live repro is caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Findings-driven follow-up to the first complete 120-question CF-7 run (58/120, PR #40's merged fix stack). Status after 5 live 120-question validation cycles (~3h): 2 gates genuinely fixed and stable; MultiHop retrieval fixed and measurably improved but capped by a separate, now-identified model-compliance ceiling.
THEORC_STITCH_RAW_DIRfull-raw capture revealed Meta-Llama emitslinkedFactsas objects instead of strings.NormalizeLinkedFactObjectsflattens that shape before parsing. Live-verified: 0/2 → 2/2 PASS, held stable across all 5 re-runs.BuildEvidencePackfollows tracked identifiers (RPT-064-style tokens) from included cards into linked segments, and reserves 30% of the evidence budget for MultiHop questions so the chase always gets a turn. Three real bugs found and fixed along the way (wrong text field, an overly strict doc-frequency cap, and the budget-ordering bug itself). The fix measurably worked: MultiHop full-retrieval rose 6 → 9/24 — the first change in the retrieval distribution across 5 cycles. But the pass rate stayed flat, because every one of those 9 fully-retrieved cases fails on an identical, separate gap: the model attaches only 1 of 2 (or 0 of 4) required citations despite an explicit prompt instruction. This is a model-instruction-compliance ceiling, not a retrieval or code defect — closed as such inCF_RETRIEVAL_IMPROVEMENT_PLAN.md§3c with three scoped follow-up options (few-shot, schema-enforced citation count, larger model).--model <substring>/THEORC_CF_MODEL/-Modelscript param restrict depot role resolution (ModelDepot.WithBaseModelFilter), so shared-depot churn can't silently swap or break a benchmark run.Test plan
🤖 Generated with Claude Code