docs: split CF-7 test harness doc into grading spec / bug history / infra notes - #57
Merged
Conversation
…tory, infra notes The single 504-line doc mixed a normative grading specification with a chronological bug-investigation log and fleet/environment notes, making it hard for a reviewer to answer "how does this harness decide pass/fail" without wading through disproved hypotheses. Split per external review (Grok + DeepSeek combined critique): - CONTEXT_FABRIC_GRADING_SPEC.md: current-state normative spec, with a new decision flowchart, the full citation_precision algebra (including the aggregate-vs-per-question-mean distinction and the abstention edge case, neither previously documented), the held-out split's exact stratified 20%-per-category breakdown (pulled from the actual question-suite JSON, not transcribed), and B4's true provenance (a structurally different, smaller CF-6 benchmark, not a 120-question CF-7 run). - CONTEXT_FABRIC_BUG_HISTORY.md: the KV-cache investigation narrative and change history, moved verbatim with section numbers preserved (existing code comments cite §7/§7a directly). - CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.md: fleet quirks plus a first-cut Known Model Compatibility table. Updated all 11 cross-references (6 code comments, 5 docs/config). Phase 1 of 4 (doc hygiene only, no code changes) addressing the combined Grok+DeepSeek review of the CF-7 test harness documentation. Gate metrics, exhaustive-heuristic hardening, and final compatibility-table cleanup are Phases 2-4.
Contributor
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR retires the Context Fabric test-harness document, adds separate grading, bug-history, and infrastructure documentation, and updates related references in runtime diagnostics, tests, workflows, configuration, and existing documentation. ChangesContext Fabric documentation migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ContextFabricContracts.cs used the abbreviated form "CF_TEST_HARNESS.md", which the original 11-file cross-reference sweep missed (it searched for the exact full filename). Retargeted to CONTEXT_FABRIC_BUG_HISTORY.md §7, consistent with every other §7 reference.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/CONTEXT_FABRIC_BUG_HISTORY.md (1)
196-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the issue number to avoid markdownlint warnings.
Starting a line with
#triggers a markdownlint warning (MD018) for an ATX-style heading without a space. Escaping the hash will clear the warning and ensure it's rendered correctly as plain text across all markdown parsers.💡 Proposed fix
-SequenceRecycleThreshold = 24). A follow-up finding (CodeRabbit review, PR -#56): `StreamCompletionAsync`'s `StatelessExecutor` was reusing the same +SequenceRecycleThreshold = 24). A follow-up finding (CodeRabbit review, PR +\`#56`): `StreamCompletionAsync`'s `StatelessExecutor` was reusing the same🤖 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 `@docs/CONTEXT_FABRIC_BUG_HISTORY.md` around lines 196 - 197, Update the PR reference in the documentation text near StreamCompletionAsync to escape the hash character in “#56,” preventing it from being interpreted as a Markdown heading while preserving the displayed issue reference.Source: Linters/SAST tools
docs/CONTEXT_FABRIC_GRADING_SPEC.md (1)
224-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code blocks.
Adding a language identifier resolves markdownlint warnings (MD040) and ensures consistent rendering across markdown viewers.
docs/CONTEXT_FABRIC_GRADING_SPEC.md#L224-L224: Addtextto the opening backticks (```text).docs/CONTEXT_FABRIC_GRADING_SPEC.md#L257-L257: Addtextto the opening backticks (```text).🤖 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 `@docs/CONTEXT_FABRIC_GRADING_SPEC.md` at line 224, Specify the text language on both fenced code blocks in docs/CONTEXT_FABRIC_GRADING_SPEC.md at lines 224-224 and 257-257 by changing each opening fence to use the text identifier.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/CONTEXT_FABRIC_BUG_HISTORY.md`:
- Around line 196-197: Update the PR reference in the documentation text near
StreamCompletionAsync to escape the hash character in “#56,” preventing it from
being interpreted as a Markdown heading while preserving the displayed issue
reference.
In `@docs/CONTEXT_FABRIC_GRADING_SPEC.md`:
- Line 224: Specify the text language on both fenced code blocks in
docs/CONTEXT_FABRIC_GRADING_SPEC.md at lines 224-224 and 257-257 by changing
each opening fence to use the text identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7dcca0c4-3e1d-4c59-89ac-49f42c0462dd
📒 Files selected for processing (15)
.github/workflows/release.ymlOrchestratorIDE.UnitTests/ModelAdmissionGateTests.csOrchestratorIDE/Core/Runtime/AdapterManager.csOrchestratorIDE/Core/Runtime/IRoleRuntime.csOrchestratorIDE/Core/Runtime/LLamaSharpRuntime.csOrchestratorIDE/Core/Runtime/ModelAdmissionGate.csdocs/CF_RETRIEVAL_IMPROVEMENT_PLAN.mddocs/CF_TEST_RESULTS.mddocs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.mddocs/CONTEXT_FABRIC_BUG_HISTORY.mddocs/CONTEXT_FABRIC_GRADING_SPEC.mddocs/CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.mddocs/CONTEXT_FABRIC_TEST_HARNESS.mddocs/README.mdtraining_pit/foundry/configs/fabric_v0.json
💤 Files with no reviewable changes (1)
- docs/CONTEXT_FABRIC_TEST_HARNESS.md
Escape #56 (MD018, avoids ATX-heading misparse) and add text language identifiers to two fenced code blocks (MD040).
The flowchart incremented totalCitations on both the H->L "regardless of outcome" edge and again inside the valid-citation node K, contradicting the code (ContextFabricValidation.cs:908,925) where totalCitations increments exactly once per non-null citation immediately after the null check, and validCitations increments separately only on the fully-valid path. Restructured so totalCitations++ happens on one edge (TC, gated only by the null check) and validCitations++ happens only at the end of the valid path (K), matching the source exactly.
…art (grok review) Flowchart omitted two real verifier paths (ContextFabricValidation.cs:887-891, 897-898): a null claim item (skips that claim entirely) and exceeding maxCitationsPerClaim (adds an error but does NOT skip citation processing, unlike the empty-claim-text check which I'd also mis-modeled as terminal). Re-verified the full 852-975 method line-by-line against this diagram before pushing.
…ates (grok review) Three inaccuracies (ContextFabricValidation.cs:935-957): 1. draftClaims.Count>0 (ERR11) is an independent, always-evaluated check under ExpectAbstention, not gated behind Abstained==true and the "does not establish" phrase passing. 2. The non-abstention branch's ExpectedTerm/ExpectedSegmentId checks always run regardless of the unexpected-abstention check's outcome -- not terminal on that error as previously drawn. 3. ExpectAbstention is evaluated once after the whole claims loop, not per-claim -- detached the node from the per-claim scope. Also fixed the final PASS/FAIL convergence to be an unconditional fan-in from every error node (matching the additive, non-short-circuiting nature already described in the surrounding prose) rather than a malformed multi-source conditional edge.
…ce scope (grok review) Two more inaccuracies: 1. schemaVersion/answer-length/claim-count checks (ContextFabricValidation.cs:862-879) don't return/continue on failure -- they always fall through into claim processing, same pattern already correctly modeled for the per-claim checks. Gave them real forward edges and removed the now-duplicate direct-to-PASS edges for ERR/ERR2/ERR3/ERR4/ERR4c (they already flow forward, listing them in the final fan-in too created a contradictory two-destination edge). 2. groundedTrace (the ExpectedTerms match source) includes claim.Text, not just answer text and citation quotes (ContextFabricValidation.cs:948-950) -- updated both the flowchart node and §6.4 prose.
Carried over ~740 from the original doc without verifying against current source; actual location is line 962. Also re-verified every other line number citation in this doc against current source in this pass.
The link target was correctly retargeted to CONTEXT_FABRIC_BUG_HISTORY.md in the Phase 1 commit, but the surrounding sentence still said "how the harness itself grades answers" -- that's Grading Spec content, not what Bug History §7/§7a (NoKvSlot investigation) actually covers. Split into two sentences pointing at the correct doc each.
…eAnswer (grok review) Section 5.3 described only the entity-scoped/category-wide unigram classification, omitting the higher-precedence Tier 1c anchor-match path (ContextFabricFeasibilityRunner.cs:1006-1015, landed per CF_RETRIEVAL_IMPROVEMENT_PLAN.md §1c) that resolves hyphenated identifiers via verbatim substring match before the unigram heuristic ever runs. Also updated the "known limitation" note: Tier 1c already fixed the specific ledger-01-vs-ledger-09 collision originally cited as the motivating example; the residual heuristic risk now applies specifically to the non-hyphenated-identifier fallback path.
§6.4 didn't specify that citation quotes feeding ExpectedTerm matching are only from citations that already passed NormalizeCitation -- normalizedClaims (the source of groundedTrace) is built from the `citations` list, which only gets appended to at ContextFabricValidation.cs:927, after segment-exists and quote-grounded checks pass. A hallucinated citation's quote text cannot be used to satisfy an ExpectedTerm.
LoadHiveAcceptanceGate also fails closed on questions[].answerValidated and stitchCases[].validated (or either array being empty), not just passed/gateMode/readerNodeCount/verifiers -- ContextFabricBaselineRunner.cs:129-180.
…ity (grok review) 1. Section 5.1 claimed BuildEvidencePack "is the same evidence selection used by FabricNativeReaderService and HiveNativeRoleExecutorAdapter in the real product" -- carried over from the original doc without verification. Neither file references BuildEvidencePack; the actual product path is FabricAskService via the structurally separate EvidencePackBuilder.cs. Corrected to describe them as conceptually similar but independent implementations. 2. Section 5.2 claimed B2 uses "the same IDF-weighted, budget-fill approach as B3" -- also stale. B2 (BuildTopKText) scores whole segments with the stricter 3-char Tokenize and a plain 1/documentFrequency weight; B3 (BuildEvidencePack) scores cards with TokenizeForScoring and ScoreTextIdf. Corrected with the actual algorithmic differences. 3. The flowchart's "all independent checks" edge label on the ERR9/ERR10/P fan-in into Q was ambiguous -- could be misread as claiming ERR10 fires independently of draft.Abstained, when the graph topology already correctly gates it behind O(Abstained)==yes. Reworded to only claim what the edge actually shows: Q runs regardless of which upstream path fired.
…ne, not plain IDF (grok review) Section 5.1 described only the original Tier-0 IDF+budget-fill algorithm from commit c68e01c. The current implementation (ContextFabricFeasibilityRunner.cs:661-830) has accreted anchor/proximity-pair extraction, lexicographic AnchorScore-then- TermScore ranking (not a single ScoreTextIdf call), coverage-aware greedy fill, MultiHop-specific budget reservation, and a reference-chase pass -- the full retrieval-quality work tracked in CF_RETRIEVAL_IMPROVEMENT_PLAN.md. Rewrote to describe the current 5-step pipeline accurately. Also fixed the flowchart's node S label to say "a VALID citation's quote", matching the already-corrected §6.4 prose (commit 86bbc74) that this edge had drifted out of sync with.
…iew) 1. Section 5.2 claimed B3's TermScore comes from ScoreTextIdf; it doesn't -- BuildEvidencePack inlines the same plain 1/documentFrequency formula B2 uses. ScoreTextIdf is exclusive to BuildExhaustiveAnswer. Corrected to note the formulas are actually the same; the real B2/B3 gap is B3's additional AnchorScore/coverage-fill/MultiHop-budget/reference-chase layers B2 entirely lacks. 2. Section 5.1 combined anchor extraction and proximity-pair extraction into one "Tier 1a" step; CF_RETRIEVAL_IMPROVEMENT_PLAN.md marks ExtractProximityPairs as the later Tier 1.5. Split into separate numbered steps with correct tier labels, renumbering the rest of the list.
…rok review) 1. §5.2's "§5.1 steps 1-5" went stale when the prior commit renumbered §5.1 to 6 steps (Tier 1a/1.5 split). 2. fabric_v0.json's "capture point" comment pointed solely at CONTEXT_FABRIC_GRADING_SPEC.md, but "capture point" describes the whole run/capture infrastructure, not just the scoring rules -- added CONTEXT_FABRIC_BENCHMARK_MANIFEST.md (the run/capture recipe) alongside it.
…th claim (grok review) §2's table still called B3 "the actual product answering path" after §5.1 was corrected to say BuildEvidencePack is a separate harness-only implementation from the shipped product's FabricAskService/EvidencePackBuilder path -- an internal contradiction between two sections of the same doc.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
4 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
docs/CONTEXT_FABRIC_TEST_HARNESS.md(504 lines, mixed normative spec + bug-investigation narrative + fleet notes) into three purpose-specific docs, per a combined Grok + DeepSeek external review of the doc.CONTEXT_FABRIC_GRADING_SPEC.md— current-state normative spec only: systems under test, corpus/held-out-split (exact stratified 20%-per-category breakdown pulled from the actual question-suite JSON, not transcribed from prose), a new Mermaid decision flowchart verified line-by-line againstFabricAnswerVerifier.NormalizeAndVerify, the fullcitation_precisionalgebra (including two ambiguities neither review caught: the gate-level metric is a citation-weighted aggregate, not a mean of per-question scores, and abstention questions are invisible to it), and B4's true provenance (a structurally different, smaller CF-6 benchmark against a different corpus — not a 120-question CF-7 run, contrary to how it reads in the systems table).CONTEXT_FABRIC_BUG_HISTORY.md— the KV-cache investigation narrative and change history, moved verbatim, section numbers (§7/§7a/§7b) preserved since existing code comments cite them directly.CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.md— fleet quirks plus a first-cut Known Model Compatibility table (Gemma-4: incompatible upstream; Llama-3.1-8B: confirmed working; qwen2.5-coder-7b: works, weaker capability; Qwen3.5-9B: works as of PR Fix NoKvSlot/OOM crash on recurrent-architecture models (Qwen3.5) #56).AdapterManager.cs/LLamaSharpRuntime.cs/ModelAdmissionGate.cs/IRoleRuntime.cs/ModelAdmissionGateTests.cs, 5 docs/config files) —grep -r CONTEXT_FABRIC_TEST_HARNESSnow returns only the intentional historical note in Bug History explaining the old filename's section numbers.This is Phase 1 of 4 (doc hygiene only, no code changes). Phases 2–4 (graded gate metric, exhaustive-heuristic hardening, final compatibility-table cleanup) follow in separate PRs once this one is reviewed.
Test plan
dotnet buildclean (comment-only changes in.csfiles, verified no compile impact)dotnet test --filter ModelAdmissionGate— 13/13 passgrep -r CONTEXT_FABRIC_TEST_HARNESS— zero stray references (one intentional historical note remains, explained inline)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Diagnostics