CF benchmark remediation: honest evidence-selection fix + Foundry F-1 start - #34
Conversation
Response to the four independent adversarial reviews of the CF-7 closure (prior-art prosecution, benchmark red-team, Grok, Codex): the single sharpest, unanimous finding was that the frozen synthetic fixture marks every scored fact with a literal "EVIDENCE:" line, so the reader task is extraction-with-answers-highlighted rather than comprehension. This is the foundational, additive fix -- it does not touch the frozen 16-segment fixture, its CorpusId, or any test/tool that depends on it. - DeterministicExpandedFabricCorpus: a new 128-section corpus generator matching docs/The Orc Context Fabric.md's "Corpus A" spec -- facts embedded in ordinary prose with no marker, 30 two-hop chains, 15 three-to-five-hop chains, 20 contradiction/resolution pairs, 20 unanswerable gaps, 15 exhaustive-enumeration categories, adversarial "ignore your instructions" injections, and 16 theme clusters for global-synthesis authoring. Emits a private ground-truth manifest the model under test never sees. A generator self-check verifies every planted statement appears verbatim in its claimed segment before the fixture is returned; a unit test cross-checks this from the outside too. - FabricQuestionKind gains Paraphrased and GlobalSynthesis (additive enum members; the one switch statement over this type has a default arm). - FabricRunOptions gains OpenExtractionReading (default false, preserves existing behavior everywhere). When true, ContextFabricFeasibilityRunner uses a reader prompt that asks the model to find and cite whatever factual claims the prose actually contains, instead of "one claim per evidenceLines item, no claims for other source text" -- which, against a segment with zero marked lines, would instruct the model to extract nothing. A regression test proves the old prompt genuinely fails against the new corpus (zero claims, hard-rejected) and the new prompt fixes it. Remaining remediation work (question-suite authoring, verification, dev/held-out split, re-running CF-7) is scoped in .orc/adversarial/ remediation-scope.md (gitignored, local evidence directory). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds QuestionText to the manifest record types (populated at generation time, while entity names are still in scope, rather than reverse-parsing rendered prose) and ExpandedFabricQuestionGenerator, which turns the manifest directly into FabricBenchmarkQuestion instances for the four structurally-regular categories: Needle/local fact (40), Exhaustive enumeration (15), Unanswerable (20), and Contradiction/change (10 of the 20 generated pairs -- the other 10 remain in the manifest as dev-set candidates). 85 total, matching the docs' category minimums exactly. Tests prove exact category counts, unique question IDs, that every expected segment ID is real, and -- the same mechanical check task #14 will need for the externally-authored 65 questions -- that every expected term actually appears in its claimed expected segments' rendered text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…real bug caught by verification Adds the pipeline for Paraphrased retrieval, Multi-hop, and Global synthesis -- the three categories needing natural-language phrasing diversity rather than deterministic templating: - ExpandedFabricLedgerExport: builds private authoring ledgers (no full corpus text, just structured ground truth) split across two model families so no single voice dominates the suite -- Grok gets Paraphrased (20 local facts) + 15 two-hop chains, Codex gets Global synthesis (15 theme clusters) + 15 long chains. New `--suite export-ledger` CLI mode. - ExpandedFabricAuthoredQuestionMerger: parses each model's strict-JSON question drafts, merges them back onto their manifest ground truth, and mechanically verifies every resulting question (including the 85 host-templated ones) against the actual rendered corpus text before trusting it. Global-synthesis questions are exempt from exact-term matching -- they're rubric-graded, per remediation-scope.md. - New `--suite merge-authored` CLI mode runs the full pipeline end to end. Ran for real against Grok Build and OpenAI Codex (prompts and raw output archived under .orc/adversarial/, gitignored). First pass: 135/150 verified, all 15 rejections on the three-to-five-hop chain questions. The verifier had found a real generator bug, not a bad authored question: the closing hop's derived reference advanced one step past what its own sentence actually stated, so the manifest recorded a value that never appeared anywhere in the rendered corpus. Fixed in DeterministicExpandedFabricCorpus (rendered segment text is unchanged -- only the manifest's derived-answer bookkeeping was wrong) and pinned with a regression test. Second pass: 150/150 verified, 0 rejected, matching the docs' exact category minimums (40/20/30/15/10/15/20). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per docs/The Orc Context Fabric.md:963, prompt tuning may only use development questions. No ratio was specified there, so this uses a small, deterministic, stratified 20% development share (every kind represented) and reserves the remaining 80% as held-out -- the set that actually gates CF-7. Deterministic by question-ID ordering, not random, so the split is exactly reproducible from the same verified list. `--suite merge-authored` now also writes expanded-question-suite-dev.json and expanded-question-suite-heldout.json alongside the full verified set. Real run: 30 development / 120 held-out, from the actual 150-question Grok+Codex+host-templated suite assembled in the previous two commits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on gate, expanded suite
Three-layer JSON recovery in FabricJson.ParseModelObject (strict → lenient → keyword-suffix
sanitizer → throw) handles token-boundary artifacts like falseC/trueX/nullValue that
autoregressive models emit when a JSON keyword token runs together with the next word token.
TrySanitizeLiteralSuffixes walks the JSON string-aware to strip only out-of-string garbage
suffixes, validated with JsonDocument.Parse before returning.
ContextFabricBaselineRunner splits the single catch into JsonException (Succeeded=true,
incorrect answer recorded) vs Exception (Succeeded=false, runtime failure), so a B0 run
always reaches RunCompleted=true unless the executor itself crashes.
ContextFabricFeasibilityRunner reader prompt narrowed: cap at ~4-5 genuine facts per segment,
explicit instruction to skip routine narration and long lists of place/team names; raises
precision on the open-extraction reading path without changing the marked-checklist path.
ModelAdmissionGate lowers the CF hard-reject floor from 7B to 3B; 3-7B now gets Provisional
("compact model, benchmark to verify") rather than hard-rejected. The benchmark run IS the
verification — parameter count alone is not sufficient evidence of unfitness.
Program.cs adds Cf7GateExpanded suite (--suite cf7-gate-expanded) wiring up the 128-segment
expanded corpus with held-out questions and open-extraction reading against B0/B1/B2/B3/B4.
7 new unit tests cover the sanitizer, trailing-comma lenience, and the new admission gate tiers.
452 pass, 4 skipped, 0 fail.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Run-CF7GateExpanded.ps1 is the canonical recipe for re-running the 120-question cf7-gate-expanded benchmark on any machine. It auto-locates the B4 artifact, validates prereqs, builds from source, runs with full stdout tee to a timestamped log, and prints a GO/NO-GO summary. Designed to hand off to Codex, Grok, or any agent picking up the CF closure work without needing to trace the original command. Program.cs --help now lists --heldout-questions, --max-questions, and the cf7-gate-expanded suite name; previously those flags existed in the parser but were invisible in usage output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AnalyzeQuoteAnchor returns the ambiguity error string into Errors (IReadOnlyList<string>) inside branches already guarded by string.Equals against a non-null literal, so the value is provably non-null; the compiler can't see through string.Equals, so annotate with the null-forgiving operator. No runtime change (identical IL). .gitignore publish/ -> publish*/ so per-machine publish outputs (publish-4b/, publish-hardcorepc/) are ignored like the base publish/ dir. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Proposed spec for scoring, badges, ranks, and streaks tied to real engineering discipline (safe execution, clean swarm roles, review quality, dataset hygiene, Context Fabric evidence, HIVE/Warband reliability) rather than raw activity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…idator skeleton Freezes the theorc-toolcaller v0 tool universe against live code rather than the F-0 proposal text. Verified all 6 proposed tools (read_file, list_files, grep_code, write_file, run_shell, ask_user) against their actual ToolDefinition registrations and recorded two real gaps found during verification instead of glossing over them: ToolPolicyEngine only actively risk-evaluates 4 of the 6 (grep_code and ask_user fall through to the default assessment), and swarm roles are Researcher/Coder/ UIDeveloper/Tester, not the boss/coder/reviewer/worker framing F-0 assumed. Per-role available-tool subsets are recorded since dataset examples must reflect the originating role's real subset, not the full frozen 6. docs/TOOLCALLER_V0_FROZEN_INVENTORY.md — the F-1 deliverable, with a SHA-256 over the checked-in tool JSON (plain file-byte hash, reproducible with sha256sum or SHA256.HashData, not a re-serialized canonical form that would only match one language's JSON library). training_pit/TOOLCALLER_CAPTURE_SCHEMA.md — new sibling dataset schema (neither DATASET_SCHEMA.md nor PLAN_CAPTURE_SCHEMA.md can hold a tool-call example: no available_tools, decision enum, or tool+arguments shape exists in either). Includes a reason-code taxonomy and the mechanical dataset admission gates from THEORC_TOOLCALLER_V0.md. Tools/ToolcallerBench — a working (not stubbed) mechanical validator implementing those admission gates: frozen-tool membership, invented/missing arguments, reason codes, lineage-group/split consistency, and stale schema-hash detection. Verified against real fixtures (clean example passes 0 findings; a deliberately malformed one trips all 5 expected gates). Two schema-doc gates are explicitly NOT mechanically checked and documented as such rather than faked: approval_state semantic misuse (needs reviewer judgment, not a keyword heuristic) and live ToolPolicyEngine cross-verification (that class only compiles into OrchestratorIDE.Avalonia.csproj today; pulling in the full UI stack for this bench tool was rejected as disproportionate to what a validator skeleton needs). Also fixes stale doc drift in PLAN_CAPTURE_SCHEMA.md: DatasetCapture.cs exists and is wired into SwarmSession.RunInternalAsync() today; the doc claimed it was not yet built. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…diation Pulls in THEORC_FOUNDRY.md, FOUNDRY_ARENA.md, and THEORC_TOOLCALLER_V0.md, which the F-1 deliverables on this branch (TOOLCALLER_V0_FROZEN_INVENTORY.md, TOOLCALLER_CAPTURE_SCHEMA.md, Tools/ToolcallerBench) reference and depend on. Without this merge those cross-references pointed at files that didn't exist on this branch.
TOOLCALLER_V0_FROZEN_INVENTORY.md and TOOLCALLER_CAPTURE_SCHEMA.md existed on this branch with no entry in docs/README.md's Foundry section, and Run-CF7GateExpanded.ps1 had no pointer from CONTEXT_FABRIC_BENCHMARK_MANIFEST.md — all three were effectively orphaned, discoverable only by browsing Tools/ or training_pit/ directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TheOrc now generates its own Foundry F-1 training data as a byproduct of normal swarm use, rather than relying on synthetic-only authoring. This is the first concrete step toward the broader goal of TheOrc developing itself. ToolcallerDatasetCapture.cs mirrors DatasetCapture.cs's proven precedent (same staging convention, same best-effort/exception-swallowing design, on by default) but targets toolcaller-v0 instead of plan captures. Wired into SwarmSession.RunWorkerLoopAsync's real tool-execution loop at two points: - StageCallAsync fires once per dispatched tool call, capturing the worker's actual proposed tool+arguments as a "call" decision. Because this runs inside the real app (unlike the standalone Tools/ToolcallerBench, which only ever had ToolPolicyEngine.cs unavailable to it), it can call the live ToolPolicyEngine.Evaluate() for a real policy_outcome cross-check -- closing the exact gap flagged in docs/TOOLCALLER_V0_FROZEN_INVENTORY.md. - StageNoToolAsync fires when a worker turn produces substantive content with no tool call, capturing a "no_tool" decision (skips trivial/near-empty replies). Per user direction: organic signals only for this pass (no scripted bootstrap tasks), on by default matching DatasetCapture.cs's existing precedent. Recorded the resulting category-coverage gap (clarify beyond ask_user, and unsupported, have no organic signal in the current worker loop) directly in TOOLCALLER_V0_FROZEN_INVENTORY.md rather than glossing over it. Captures stage as pending/unreviewed to .orc/swarm/dataset-staging/toolcaller/ -- mechanical validation (Tools/ToolcallerBench), the existing sanitizer, and human review remain required before any example reaches a train/eval split. The hook never assigns a split or promotes anything itself. Verified end-to-end, not just unit-tested in isolation: a real capture produced by this hook was run through the actual ToolcallerBench validator and passed with 0 findings. 5 new unit tests cover call/no_tool shape, policy_gap_tool flagging, trivial-content skipping, and the IsEnabled kill switch. 457 pass, 0 fail, 4 skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ead of fixed k=4
The old BuildTopKText had two real problems, found while reviewing why the B2
baseline exists as a comparison floor:
1. Take(4) was a hardcoded segment count. GlobalSynthesis questions in this
corpus need evidence spanning up to 8 segments -- B2 was structurally
incapable of answering that category regardless of retrieval quality, which
makes any "B3 beats B2 on GlobalSynthesis" comparison meaningless (B2 was
guaranteed to lose by construction, not by a fair retrieval contest).
2. Ranking was a raw term-overlap count with no stopword filtering or term
weighting. Common words ("the", "and", "this", "with") matched almost every
segment equally, diluting the signal that should come from distinctive terms
(names, codes, values). This made B2 a weaker RAG implementation than a
"conventional" baseline should be, understating what real lexical RAG can do.
Rewrite: score segments by IDF-weighted term overlap (rare terms count more,
common stopwords are excluded from scoring entirely via a standard English
stopword list), then greedily select ranked segments until the same finite-
context budget B1 uses is filled -- not a fixed count. A higher-scoring segment
that doesn't fit is skipped in favor of shorter, still-relevant, lower-ranked
ones, so budget is used rather than left on the table.
Document frequency is computed once per corpus (cached by CorpusId) rather than
recomputed per question, since the corpus is constant across all 120 questions
in a single B2 run.
Note: the currently in-flight NEWCOREPC CF-7 gate run started before this
change and is running the old compiled binary -- this does not retroactively
affect that run's B2 result. This rewrite is queued for a follow-up run via
Tools/ContextFabricBench/Run-CF7GateExpanded.ps1 once the current run completes.
5 new unit tests exercise BuildTopKText directly (made internal for testability):
rare-term preference over common-word overlap, selecting more than 4 segments
when budget allows, empty result when the question has no non-stopword terms,
empty result when budget is exhausted, and skipping an over-budget
higher-scoring segment in favor of a shorter one that fits. 462 pass, 0 fail,
4 skipped.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fixed card cap Root cause of the CF-7 gate NO-GO (56/120, 47% question pass rate): per-question citation precision was 100% (CF never fabricates evidence) but question_pass_rate failed hard because CF frequently never gathered the right segments for questions spanning multiple parts of the corpus. Comparing expectedSegmentIds against includedSegmentIds on failing questions showed this was an evidence-selection bug, not a model reasoning failure. BuildEvidencePack (ContextFabricFeasibilityRunner.cs, the real production Context Fabric answering path used by FabricNativeReaderService and HiveNativeRoleExecutorAdapter -- not benchmark-only code) had the identical two flaws already found and fixed in the B2 benchmark baseline: 1. A hardcoded maxCards cap by question kind (LocalFact=1, MultiHop/ Contradiction=2, everything else=4) unrelated to actual context budget. GlobalSynthesis questions can need evidence from up to 8 segments; capped at 4, it was structurally impossible regardless of ranking quality. No documentation anywhere justified these specific numbers as a deliberate latency/cost tradeoff, and the real evidence budget (6144 tokens default, 3072 HIVE) has ample headroom for far more than 1-4 short evidence cards. 2. Score() was naive raw term-count with no stopword filtering -- common words diluted the ranking signal that should come from distinctive terms. Fix: IDF-weighted, stopword-aware scoring (document frequency computed per call over the supplied cards, since they're short structured summaries rather than raw segment text -- cheap enough to not need cross-question caching), plus removing the Take(maxCards) cap so the method's existing greedy budget-check loop (already present, unchanged) considers every relevant card instead of only the first few. Cards scoring 0 are excluded so leftover budget doesn't get padded with irrelevant evidence. Scoping note recorded during investigation: FabricQuestionKind.Exhaustive does NOT go through BuildEvidencePack -- AnswerQuestionAsync routes it to a separate method, BuildExhaustiveAnswer, with its own literal-keyword-overlap filter. Its 0/12 failure has a different root cause and is not addressed by this fix; it needs its own follow-up investigation. BuildEvidencePack, EvidencePack, and the AnswerEvidence/-Claim/-Citation record chain are now internal (from private) so unit tests can exercise evidence selection directly, mirroring the ContextFabricBaselineRunner precedent. 5 new unit tests: rare-term preference over common-word overlap, selecting more than 4 cards when budget allows, excluding cards with no non-stopword term overlap, preferring a shorter fitting card over a higher-scoring one that doesn't fit, and confirming the Exhaustive path (BuildExhaustiveAnswer, unusued Score()) is untouched. 467 pass, 0 fail, 4 skipped. Per plan: this fix is not re-run against the full 120-question NEWCOREPC benchmark as part of this change -- that decision is separate, pending both this fix and the already-committed B2 rewrite being ready together. PR remains held per prior instruction until this routing-bug fix landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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 (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR expands Context Fabric with an expanded deterministic corpus, authored-question tooling, IDF-based scoring, open-extraction reading, and JSON recovery. It also adds live toolcaller capture staging, a frozen-tool validation benchmark, CLI/runner wiring, an admission-gate adjustment, and documentation updates including the Warpath whitepaper. ChangesContext Fabric Expansion and Scoring
Estimated code review effort: 4 (Complex) | ~75 minutes Toolcaller Dataset Capture and Validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Repository Configuration and Documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant SwarmSession
participant ToolcallerDatasetCapture
participant StagingDir
SwarmSession->>ToolcallerDatasetCapture: StageCallAsync(...)
ToolcallerDatasetCapture->>StagingDir: write capture JSON
SwarmSession->>ToolcallerDatasetCapture: StageNoToolAsync(...)
ToolcallerDatasetCapture->>StagingDir: write capture JSON
sequenceDiagram
participant Program
participant ToolcallerCaptureValidator
participant ToolcallerReportWriter
Program->>ToolcallerCaptureValidator: Validate(captures, frozenTools, hash)
ToolcallerCaptureValidator-->>Program: ToolcallerValidationReport
Program->>ToolcallerReportWriter: WriteAsync(report, outputDir)
ToolcallerReportWriter-->>Program: JSON + Markdown paths
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (14)
OrchestratorIDE/Agents/SwarmSession.cs (1)
2066-2070: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUn-timeboxed, awaited capture write sits in the hot tool-dispatch path.
StageCallAsyncis awaited synchronously before every real tool call is dispatched (once per tool call, not once per run like the existingDatasetCapture.StageAsyncprecedent). Its internaltry/catchswallows exceptions, but a slow/unresponsive staging directory (e.g. network share, low disk) has no timeout and will stall the worker loop rather than just failing the capture. Consider fire-and-forget (_ = StageCallAsync(...)) or wrapping the write in a short timeout so capture latency/hangs never propagate to real tool execution.🤖 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/Agents/SwarmSession.cs` around lines 2066 - 2070, `SwarmSession` is blocking the hot tool-dispatch path by awaiting `ToolcallerDatasetCapture.StageCallAsync` before each real tool call, so capture latency can stall execution. Update the call site to avoid propagating staging delays by making the capture fire-and-forget or by wrapping `StageCallAsync` in a short timeout, while keeping the existing best-effort behavior and not letting failures or hangs affect tool dispatch.OrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.cs (1)
98-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
policy_gap_toollist duplicatesToolPolicyEngineinternals.
policy_gap_tool = call.Name is "grep_code" or "ask_user"hardcodes knowledge thatToolPolicyEngine.Evaluate()has no dedicated case for these two tools (perdocs/TOOLCALLER_V0_FROZEN_INVENTORY.md's verification notes). IfToolPolicyEngineever adds a dedicated case for either tool, this literal silently goes stale and captures will keep reporting a gap that no longer exists.Consider adding a unit test in the
ToolPolicyEnginetest suite that fails if a tool this file lists is given a dedicatedEvaluate()case, so the two stay in sync.🤖 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/Swarm/ToolcallerDatasetCapture.cs` around lines 98 - 107, The Toolcaller dataset capture is hardcoding `policy_gap_tool` in `ToolcallerDatasetCapture` using `call.Name is "grep_code" or "ask_user"`, which duplicates `ToolPolicyEngine` knowledge and can drift. Move this knowledge into a sync check by adding a unit test in the `ToolPolicyEngine` test suite that asserts any tool listed here still lacks a dedicated `Evaluate()` case, and update the capture logic only through that shared contract if the engine changes.training_pit/TOOLCALLER_CAPTURE_SCHEMA.md (1)
187-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFenced code block missing language identifier.
Flagged by markdownlint (MD040).
📝 Fix
-``` +```text training_pit/datasets/toolcaller/ toolcaller_capture_{split}_{example_id}.json</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@training_pit/TOOLCALLER_CAPTURE_SCHEMA.mdaround lines 187 - 190, The fenced
code block in TOOLCALLER_CAPTURE_SCHEMA.md is missing a language identifier and
should be updated to satisfy markdownlint MD040. Locate the Markdown snippet
containing the dataset path example and change the opening fence to use a text
language tag so the rendered block remains unchanged while complying with the
linter.</details> <!-- cr-comment:v1:495ec2b30960f5ea899030ac --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>OrchestratorIDE.UnitTests/ToolcallerDatasetCaptureTests.cs (1)</summary><blockquote> `79-95`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Test name promises "ask_user" coverage but only exercises `grep_code`.** `StageCallAsync_FlagsPolicyGapTool_ForGrepCodeAndAskUser` never constructs an `ask_user` call, so the `ask_user` branch of `policy_gap_tool` is unverified by this test despite the name. <details> <summary>💡 Suggested addition</summary> ```diff [Test] public async Task StageCallAsync_FlagsPolicyGapTool_ForGrepCodeAndAskUser() { var stagingDir = NewTempDir(); var workspaceRoot = NewTempDir(); var task = new SwarmTask { Title = "Find usages", Description = "Find usages of Foo.", Role = SwarmWorkerRole.Researcher }; var call = new ToolCall { Name = "grep_code", Arguments = new() { ["pattern"] = "Foo" } }; var availableTools = new List<ToolDefinition> { new() { Name = "grep_code", Description = "Search.", Parameters = new() } }; await ToolcallerDatasetCapture.StageCallAsync( "20260703_130000", task, "qwen2.5-coder:14b", call, availableTools, workspaceRoot, stagingDir); var file = Directory.GetFiles(stagingDir, "toolcaller_capture_*.json").Single(); using var doc = JsonDocument.Parse(await File.ReadAllTextAsync(file)); Assert.That(doc.RootElement.GetProperty("policy_outcome").GetProperty("policy_gap_tool").GetBoolean(), Is.True); + + // also verify ask_user + var askCall = new ToolCall { Name = "ask_user", Arguments = new() { ["question"] = "Which config?" } }; + await ToolcallerDatasetCapture.StageCallAsync( + "20260703_130001", task, "qwen2.5-coder:14b", askCall, availableTools, workspaceRoot, stagingDir); + var askFile = Directory.GetFiles(stagingDir, "toolcaller_capture_*20260703_130001*.json").Single(); + using var askDoc = JsonDocument.Parse(await File.ReadAllTextAsync(askFile)); + Assert.That(askDoc.RootElement.GetProperty("policy_outcome").GetProperty("policy_gap_tool").GetBoolean(), Is.True); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OrchestratorIDE.UnitTests/ToolcallerDatasetCaptureTests.cs` around lines 79 - 95, The test name implies coverage for both grep_code and ask_user, but StageCallAsync_FlagsPolicyGapTool_ForGrepCodeAndAskUser only exercises grep_code. Update ToolcallerDatasetCaptureTests by adding an ask_user ToolCall path in the same test or splitting into a separate test, and assert the policy_outcome.policy_gap_tool result for both branches so the behavior in StageCallAsync is actually covered.Tools/ToolcallerBench/ToolcallerCaptureValidator.cs (1)
146-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLineage-group-split gate is currently unreachable for organic captures.
Per the
ToolcallerDatasetCapture.cscontext snippet,lineage_group_idis set equal to the uniqueexample_idfor organic swarm captures ("organic capture, no paraphrase/repair siblings yet"), so every group here will have exactly one member and this gate can never fire today. Not a bug, just worth noting it's dormant until paraphrase/repair lineage groups are introduced.🤖 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/ToolcallerBench/ToolcallerCaptureValidator.cs` around lines 146 - 158, The lineage_group_split_conflict check in ToolcallerCaptureValidator is currently dormant for organic captures because lineage_group_id maps 1:1 to example_id, so each GroupBy(c => c.LineageGroupId) bucket has only one capture. Update this validation to only run when a lineage group can actually contain multiple members (for example, after paraphrase/repair siblings exist), or add an explicit guard around the group.Count() case so the Fail path in the Split consistency check only applies to real multi-capture lineage groups.OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs (5)
642-652: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
<summary>XML doc blocks on the same method.Two separate
<summary>elements are stacked back-to-back onBuildEvidencePack(lines 642-651 and 652). This is invalid/confusing XML doc structure; most tooling will only surface the first block, making the second effectively dead documentation.✏️ Proposed fix: merge into a single summary
/// <summary> /// Builds the evidence pack sent to the answerer for a question. Ranks every evidence card by /// IDF-weighted term overlap (rare, distinctive terms count more than common words) and greedily /// fills the actual context budget, rather than a fixed per-question-kind card count. A fixed /// count is structurally unable to answer questions whose evidence spans more cards than that /// count regardless of ranking quality -- GlobalSynthesis questions can need up to 8 segments' /// worth of evidence, and the old hardcoded caps (1/2/4) had no documented latency or cost /// justification. This mirrors the same fix already applied to the B2 benchmark baseline /// (ContextFabricBaselineRunner.BuildTopKText). + /// Internal (not private) so unit tests can exercise evidence selection directly. /// </summary> - /// <summary>Internal (not private) so unit tests can exercise evidence selection directly.</summary> internal EvidencePack BuildEvidencePack(🤖 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 642 - 652, Merge the two stacked XML documentation summaries on BuildEvidencePack into one valid <summary> block, and keep all of the existing description in a single doc comment so tooling only sees one authoritative summary for ContextFabricFeasibilityRunner.BuildEvidencePack.
264-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated reader prompt boilerplate between the open/marked branches.
Both the
[FABRIC_READER_OPEN]and[FABRIC_READER]system prompts repeat the same output-shape JSON contract, citation rules, and IDs-exactly language almost verbatim, differing mainly in extraction strategy. Consider factoring the shared suffix (schema/output-shape/citation-offset instructions) into a constant reused by both branches to avoid drift between the two prompts over time.🤖 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 264 - 290, The two branches in ContextFabricFeasibilityRunner’s SystemMessage are duplicating the same schema/output-contract and citation guidance, which risks prompt drift. Factor the shared boilerplate (schemaVersion, promptVersion, citation rules, IDs-exactly wording, and output shape JSON) into a reusable constant/helper, then compose it into both the openExtraction and non-openExtraction prompts while keeping only the branch-specific extraction instructions different.
926-939: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming convention:
_evidencePackStopwordsdeviates from the locals_static-field convention.
FabricJsonin the same PR usess_lenientOptions/s_jsonKeywordsfor private static readonly fields, while this new field uses a bare underscore prefix, which typically denotes an instance field in this codebase's convention.🤖 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 926 - 939, The new private static readonly stopword set in ContextFabricFeasibilityRunner should follow the established static-field naming convention used by symbols like s_lenientOptions and s_jsonKeywords. Rename _evidencePackStopwords to a s_-prefixed name and update any references in BuildEvidencePack or related scoring helpers so the field name clearly indicates it is static.
653-704: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winQuadratic re-serialization cost in the evidence-budget loop.
For each candidate card,
BuildEvidencePackappends it toevidence, re-serializes the entire growing evidence array viaFabricJson.Serialize(input), and re-estimates tokens -- an O(n²) cost as the number of ranked cards grows. The docstring on the sibling test file confirms this is the real production answering path (used byFabricNativeReaderService/HiveNativeRoleExecutorAdapter), and the corpus this now needs to scale to is 128 segments (up from the old 16-segment fixture), so this cost is no longer negligible.Consider tracking a running token estimate incrementally (base overhead once, plus each candidate's own estimated contribution) instead of re-serializing the full accumulated evidence set on every iteration.
🤖 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 653 - 704, BuildEvidencePack is re-serializing the full growing evidence list on every candidate, causing quadratic token-estimation cost. Update the loop to avoid calling FabricJson.Serialize(input) and EstimateTokens over the entire projected AnswerInput each time; instead, in BuildEvidencePack track a running budget incrementally using a fixed base cost plus each candidate’s estimated contribution. Keep the existing ranking/filtering flow around ordered, evidence, and included, but compute the budget check without reconstructing projected arrays on every iteration.
920-925: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
Scorehelper
OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs:920no longer calls this method;BuildExhaustiveAnswerinlines the same token-count logic andBuildEvidencePackusesScoreIdf. Keeping it adds dead-code drift.🤖 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 920 - 925, The unused Score helper in ContextFabricFeasibilityRunner should be removed because its token-count logic is no longer referenced by BuildExhaustiveAnswer or BuildEvidencePack. Delete the Score method and keep CardHaystack only if it is still used elsewhere in the class, ensuring there are no remaining references to Score or dead-code drift in the feasibility runner.OrchestratorIDE.UnitTests/ContextFabricCf0Tests.cs (1)
206-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise the sanitizer it's named for.
The payload
{"...,"answer":"falsehood is trueColor nullValue","abstained":false,"claims":[]}is already valid JSON, soParseModelObject's strict fast-path deserializes it successfully beforeTrySanitizeLiteralSuffixesis ever invoked (seeContextFabricValidation.csParseModelObject). The test would pass identically even if the sanitizer had a bug that corrupted string contents, since that code path is never reached.✅ Proposed fix: force an outside-string defect so sanitization actually runs
public void JsonParser_SanitizesKeywordSuffix_DoesNotCorruptStringContents() { // "falsehood" and "trueColor" inside JSON string values must be preserved verbatim — // the sanitizer is only allowed to modify tokens that appear outside string boundaries. + // "abstained":falseX forces the sanitizer path to actually run. var parsed = FabricJson.ParseModelObject<FabricAnswerDraft>( - "{\"schemaVersion\":\"cf0-answer-1.0\",\"answer\":\"falsehood is trueColor nullValue\",\"abstained\":false,\"claims\":[]}"); + "{\"schemaVersion\":\"cf0-answer-1.0\",\"answer\":\"falsehood is trueColor nullValue\",\"abstained\":falseX,\"claims\":[]}"); Assert.That(parsed.Answer, Is.EqualTo("falsehood is trueColor nullValue")); Assert.That(parsed.Abstained, Is.False); }🤖 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/ContextFabricCf0Tests.cs` around lines 206 - 216, The JsonParser_SanitizesKeywordSuffix_DoesNotCorruptStringContents test is currently hitting the strict ParseModelObject fast-path, so TrySanitizeLiteralSuffixes is never exercised. Change the payload in ContextFabricCf0Tests so it includes a non-string JSON defect that forces sanitization to run in ParseModelObject/FabricJson, while still keeping the answer text with falsehood/trueColor/nullValue inside string boundaries. Keep the assertions on FabricAnswerDraft.Answer and Abstained to verify the sanitizer preserves string contents and still parses the object correctly.Tools/ContextFabricBench/Run-CF7GateExpanded.ps1 (1)
223-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$Args— it shadows PowerShell's automatic variable.Static analysis flags
$Argsas an automatic variable; assigning to it can have undesired side effects (e.g. if this logic is ever moved into a function,$Args/$argswould collide with the function's own unbound-argument collection).♻️ Proposed rename
-$Args = @( +$benchArgs = @( "--suite", "cf7-gate-expanded", ... ) if ($MaxQuestions -gt 0) { - $Args += @("--max-questions", $MaxQuestions) + $benchArgs += @("--max-questions", $MaxQuestions) } if ($GpuLayers -ne -1) { - $Args += @("--gpu-layers", $GpuLayers) + $benchArgs += @("--gpu-layers", $GpuLayers) } ... -& $BenchExe `@Args` 2>&1 | Tee-Object -FilePath $LogFile +& $BenchExe `@benchArgs` 2>&1 | Tee-Object -FilePath $LogFile🤖 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 223 - 238, Rename the $Args array used in the cf7-gate-expanded argument assembly so it does not shadow PowerShell’s automatic $args variable. Update the variable and all append/use sites in the script’s argument-building block, including the conditional additions for max-questions and gpu-layers, to a distinct name that clearly represents the command arguments.Source: Linters/SAST tools
OrchestratorIDE/Services/ContextFabric/ExpandedFabricAuthoredQuestionMerger.cs (1)
20-31: 🚀 Performance & Scalability | 🔵 TrivialBracket-slicing JSON extraction has no recovery path for truncated/malformed model output.
ParseDraftstakes everything between the first[and last]. If the authored draft text contains any[/]characters before/after the actual array (e.g. markdown, footnotes, or a truncated completion missing the closing bracket), this either mis-slices the JSON or throws unhandledJsonException, aborting the entiremerge-authoredrun rather than isolating the bad draft. The PR description mentions a JSON-recovery fix was added elsewhere for token-boundary artifacts; consider reusing that same recovery logic here for consistency, since authored drafts plausibly come from LLM completions subject to the same truncation risk.🤖 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/ExpandedFabricAuthoredQuestionMerger.cs` around lines 20 - 31, ParseDrafts currently slices from the first `[` to the last `]`, which can mis-extract or fail hard on markdown noise or truncated LLM output. Update `ExpandedFabricAuthoredQuestionMerger.ParseDrafts` to reuse the same JSON recovery/parsing approach used elsewhere in the merge flow instead of relying on bracket slicing, so malformed authored draft text is isolated and does not abort the entire `merge-authored` run. Keep the fix localized to the JSON extraction/deserialization path and ensure `JsonException` from one bad draft is handled consistently with the existing recovery logic.Tools/ContextFabricBench/Program.cs (1)
216-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftLarge duplication between
Cf7GateExpandedandCf7Gategate-running logic.The new
Cf7GateExpandedblock (Lines 216-303) and the existingCf7Gateblock (Lines 305-357) both run quote-anchor diagnostics, boundary-stitch diagnostics, the B0/B1/B2 baseline loop, load the B4 artifact, evaluate the gate, and print the same shaped verdict/report output — differing mainly in which fixture/run-options feed the calls. Consider extracting a sharedRunGateAsync(fixture, runOptions, diagnosticsFixture, label, ...)helper to avoid the two paths drifting independently as gate logic evolves.🤖 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 216 - 357, The Cf7GateExpanded and Cf7Gate paths duplicate nearly the same gate-evaluation flow, so extract the shared quote/stitch diagnostics, B0/B1/B2 baseline loop, B4 artifact loading, gate evaluation, and verdict/report printing into a common helper in Program.cs. Keep the helper parameterized by the fixture and runOptions differences so both the Cf7GateExpanded and Cf7Gate branches call the same logic and won’t drift as the gate pipeline evolves.
🤖 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/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.md`:
- Around line 774-789: The trophy schema for The Black Anvil depends on
foundry.candidate.deployed_artifact_verified, but that event is not defined
anywhere in the canonical event inventory. Add the missing event definition to
the Foundry event inventory/source-of-truth and ensure the trophy definition in
the trophy schema references that exact event name consistently, so the
validator and badge rules use a single canonical symbol.
- Around line 324-349: The Warpath event identifier format in the event record
needs to avoid collisions across concurrent emitters. Update the event
schema/example around the immutable event object to use a collision-resistant ID
in event_id (for example UUID or ULID) while keeping occurred_at as a separate
timestamp field, and adjust any text or examples that currently imply sequential
wp_evt_..._0001-style IDs. Refer to the event payload shown in the Warpath event
section when making the change.
- Around line 888-896: The GitHub badge export section currently relies on
remote shields.io image URLs, which breaks the local-only export goal and can
leak metadata. Update the “GitHub Badge Export” content to use embedded/local
assets or route this example through a plain-text export path instead of
external badge links, keeping the change localized to the badge export snippet
in the whitepaper.
- Around line 699-710: The starter badge list in the HIVE and Warband Badges
section includes a cloud-specific reward that conflicts with the local-first
Warpath spec. Remove the Cloud Raider badge from the MVP badge table, or
rename/reframe it so it only rewards local or distributed execution with no
cloud dependency. Keep the remaining badges in the list unchanged and ensure the
badge triggers described in the whitepaper stay aligned with the product
promise.
In `@OrchestratorIDE.UnitTests/ContextFabricB2TopKRagTests.cs`:
- Around line 111-133: The test in
BuildTopKText_SkipsOverBudgetSegment_ButStillFitsShorterLowerRankedOne does not
match the intended ranking because Tokenize dedupes terms per segment, so
repeated “checksum” in the big segment does not raise its score. Update the test
data so the segment referenced by big genuinely outranks small under the
BuildTopKText scoring logic in ContextFabricBaselineRunner, while still being
too large for the token budget; keep small as the shorter fallback that fits and
is selected when big is skipped.
In `@OrchestratorIDE/Services/ContextFabric/ContextFabricContracts.cs`:
- Around line 93-101: `FabricQuestionKind` must remain ordinal-stable because
`FabricJson.Options` persists the enum as numeric values. Update the
`FabricQuestionKind` declaration in `ContextFabricContracts.cs` so existing
members keep their current numeric meanings by assigning explicit values or only
appending new members at the end; if `Paraphrased` must stay in the middle, add
a migration for any stored payloads that depend on the old ordinals. Make the
change in the `FabricQuestionKind` enum without reordering existing values.
In `@OrchestratorIDE/Services/ContextFabric/DeterministicExpandedFabricCorpus.cs`:
- Around line 261-282: ValidateManifestAgainstRenderedText currently skips
ExhaustiveCategories, leaving the deterministic case-ledger rows unverified.
Extend the existing self-check in DeterministicExpandedFabricCorpus so it also
validates each FabricExhaustiveCategory by reconstructing the expected row
statement from CategoryId and OccurrenceIds, then comparing it against the
rendered slot text. Use the exhaustiveCategories/exhaustiveRowSlots data already
built in this method to assert every exhaustive row is present and exact, and
surface mismatches the same way the other manifest checks do.
In
`@OrchestratorIDE/Services/ContextFabric/ExpandedFabricAuthoredQuestionMerger.cs`:
- Around line 33-81: The three merge helpers in
ExpandedFabricAuthoredQuestionMerger currently assume draft.TargetId is always
present, but a null value will make the dictionary lookup throw. Update
MergeParaphraseQuestions, MergeMultiHopQuestions, and
MergeGlobalSynthesisQuestions to filter out drafts with missing TargetId before
calling ContainsKey or indexing by ID, keeping the existing byId lookup logic
unchanged.
In `@Tools/ContextFabricBench/Program.cs`:
- Around line 467-476: The `PrintUsage` help text in `Program.cs` is missing the
`export-ledger` and `merge-authored` suite options even though `ParseSuite` and
the unknown-suite message already support them. Update the `Console.WriteLine`
inside `PrintUsage` so the `--suite` list includes `export-ledger` and
`merge-authored` alongside the existing `BenchmarkSuite` values, keeping the
help output aligned with `ParseSuite`.
In `@Tools/ContextFabricBench/Run-CF7GateExpanded.ps1`:
- Around line 152-181: The pre-flight checks in Run-CF7GateExpanded.ps1 are
using Write-Error with string concatenation directly, which PowerShell parses as
separate arguments and causes a positional-parameter error. Update each of the
four Write-Error call sites in the held-out, B4 artifact, ModelRoot, and
GgufCount checks so the full message is parenthesized or built first in a
variable before passing it to Write-Error. Use the existing guard blocks and
identifiers like $HeldOutPath, $B4Artifact, $ModelRoot, and $GgufCount to locate
them.
In `@Tools/ToolcallerBench/ToolcallerCaptureValidator.cs`:
- Around line 64-131: Add explicit validation in ToolcallerCaptureValidator so
unexpected capture.Expected.Decision values fail fast instead of falling through
the non-call branch. Introduce a known allowed set in the validator around the
existing decision gates (the clarify/unsupported reason_code check and the
call/non-call split), and if Decision is anything other than the recognized
values, call Fail with a clear invalid_decision error before any other checks.
In `@Tools/ToolcallerBench/ToolcallerContracts.cs`:
- Around line 26-43: The ToolcallerCapture contract currently relies on
non-nullable types alone, which still allows omitted JSON fields to deserialize
as null; update the ToolcallerCapture record so the critical members are
explicitly required (or otherwise enforced via stricter deserialization
settings) to reject malformed capture payloads at admission. Focus on the
ToolcallerCapture record and its JsonPropertyName-backed members, especially the
non-optional fields like SchemaVersion, ToolSchemaHash, ExampleId,
LineageGroupId, Provenance, Role, Request, AvailableTools, ApprovalState,
Expected, ReviewStatus, and Split.
In `@training_pit/TOOLCALLER_CAPTURE_SCHEMA.md`:
- Around line 4-9: The documented toolcaller staging location is incorrect and
should match the implementation in ToolcallerDatasetCapture and
RunWorkerLoopAsync. Update the description to say captures are written under
.orc/swarm/dataset-staging/ and identified by the toolcaller_capture_ filename
prefix, rather than implying a toolcaller/ subdirectory. Keep the wording
aligned with the actual capture flow and naming used by the dataset staging
code.
---
Nitpick comments:
In `@OrchestratorIDE.UnitTests/ContextFabricCf0Tests.cs`:
- Around line 206-216: The
JsonParser_SanitizesKeywordSuffix_DoesNotCorruptStringContents test is currently
hitting the strict ParseModelObject fast-path, so TrySanitizeLiteralSuffixes is
never exercised. Change the payload in ContextFabricCf0Tests so it includes a
non-string JSON defect that forces sanitization to run in
ParseModelObject/FabricJson, while still keeping the answer text with
falsehood/trueColor/nullValue inside string boundaries. Keep the assertions on
FabricAnswerDraft.Answer and Abstained to verify the sanitizer preserves string
contents and still parses the object correctly.
In `@OrchestratorIDE.UnitTests/ToolcallerDatasetCaptureTests.cs`:
- Around line 79-95: The test name implies coverage for both grep_code and
ask_user, but StageCallAsync_FlagsPolicyGapTool_ForGrepCodeAndAskUser only
exercises grep_code. Update ToolcallerDatasetCaptureTests by adding an ask_user
ToolCall path in the same test or splitting into a separate test, and assert the
policy_outcome.policy_gap_tool result for both branches so the behavior in
StageCallAsync is actually covered.
In `@OrchestratorIDE/Agents/SwarmSession.cs`:
- Around line 2066-2070: `SwarmSession` is blocking the hot tool-dispatch path
by awaiting `ToolcallerDatasetCapture.StageCallAsync` before each real tool
call, so capture latency can stall execution. Update the call site to avoid
propagating staging delays by making the capture fire-and-forget or by wrapping
`StageCallAsync` in a short timeout, while keeping the existing best-effort
behavior and not letting failures or hangs affect tool dispatch.
In `@OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs`:
- Around line 642-652: Merge the two stacked XML documentation summaries on
BuildEvidencePack into one valid <summary> block, and keep all of the existing
description in a single doc comment so tooling only sees one authoritative
summary for ContextFabricFeasibilityRunner.BuildEvidencePack.
- Around line 264-290: The two branches in ContextFabricFeasibilityRunner’s
SystemMessage are duplicating the same schema/output-contract and citation
guidance, which risks prompt drift. Factor the shared boilerplate
(schemaVersion, promptVersion, citation rules, IDs-exactly wording, and output
shape JSON) into a reusable constant/helper, then compose it into both the
openExtraction and non-openExtraction prompts while keeping only the
branch-specific extraction instructions different.
- Around line 926-939: The new private static readonly stopword set in
ContextFabricFeasibilityRunner should follow the established static-field naming
convention used by symbols like s_lenientOptions and s_jsonKeywords. Rename
_evidencePackStopwords to a s_-prefixed name and update any references in
BuildEvidencePack or related scoring helpers so the field name clearly indicates
it is static.
- Around line 653-704: BuildEvidencePack is re-serializing the full growing
evidence list on every candidate, causing quadratic token-estimation cost.
Update the loop to avoid calling FabricJson.Serialize(input) and EstimateTokens
over the entire projected AnswerInput each time; instead, in BuildEvidencePack
track a running budget incrementally using a fixed base cost plus each
candidate’s estimated contribution. Keep the existing ranking/filtering flow
around ordered, evidence, and included, but compute the budget check without
reconstructing projected arrays on every iteration.
- Around line 920-925: The unused Score helper in ContextFabricFeasibilityRunner
should be removed because its token-count logic is no longer referenced by
BuildExhaustiveAnswer or BuildEvidencePack. Delete the Score method and keep
CardHaystack only if it is still used elsewhere in the class, ensuring there are
no remaining references to Score or dead-code drift in the feasibility runner.
In
`@OrchestratorIDE/Services/ContextFabric/ExpandedFabricAuthoredQuestionMerger.cs`:
- Around line 20-31: ParseDrafts currently slices from the first `[` to the last
`]`, which can mis-extract or fail hard on markdown noise or truncated LLM
output. Update `ExpandedFabricAuthoredQuestionMerger.ParseDrafts` to reuse the
same JSON recovery/parsing approach used elsewhere in the merge flow instead of
relying on bracket slicing, so malformed authored draft text is isolated and
does not abort the entire `merge-authored` run. Keep the fix localized to the
JSON extraction/deserialization path and ensure `JsonException` from one bad
draft is handled consistently with the existing recovery logic.
In `@OrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.cs`:
- Around line 98-107: The Toolcaller dataset capture is hardcoding
`policy_gap_tool` in `ToolcallerDatasetCapture` using `call.Name is "grep_code"
or "ask_user"`, which duplicates `ToolPolicyEngine` knowledge and can drift.
Move this knowledge into a sync check by adding a unit test in the
`ToolPolicyEngine` test suite that asserts any tool listed here still lacks a
dedicated `Evaluate()` case, and update the capture logic only through that
shared contract if the engine changes.
In `@Tools/ContextFabricBench/Program.cs`:
- Around line 216-357: The Cf7GateExpanded and Cf7Gate paths duplicate nearly
the same gate-evaluation flow, so extract the shared quote/stitch diagnostics,
B0/B1/B2 baseline loop, B4 artifact loading, gate evaluation, and verdict/report
printing into a common helper in Program.cs. Keep the helper parameterized by
the fixture and runOptions differences so both the Cf7GateExpanded and Cf7Gate
branches call the same logic and won’t drift as the gate pipeline evolves.
In `@Tools/ContextFabricBench/Run-CF7GateExpanded.ps1`:
- Around line 223-238: Rename the $Args array used in the cf7-gate-expanded
argument assembly so it does not shadow PowerShell’s automatic $args variable.
Update the variable and all append/use sites in the script’s argument-building
block, including the conditional additions for max-questions and gpu-layers, to
a distinct name that clearly represents the command arguments.
In `@Tools/ToolcallerBench/ToolcallerCaptureValidator.cs`:
- Around line 146-158: The lineage_group_split_conflict check in
ToolcallerCaptureValidator is currently dormant for organic captures because
lineage_group_id maps 1:1 to example_id, so each GroupBy(c => c.LineageGroupId)
bucket has only one capture. Update this validation to only run when a lineage
group can actually contain multiple members (for example, after
paraphrase/repair siblings exist), or add an explicit guard around the
group.Count() case so the Fail path in the Split consistency check only applies
to real multi-capture lineage groups.
In `@training_pit/TOOLCALLER_CAPTURE_SCHEMA.md`:
- Around line 187-190: The fenced code block in TOOLCALLER_CAPTURE_SCHEMA.md is
missing a language identifier and should be updated to satisfy markdownlint
MD040. Locate the Markdown snippet containing the dataset path example and
change the opening fence to use a text language tag so the rendered block
remains unchanged while complying with the linter.
🪄 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: 14eef075-a8a0-42c0-9671-3a4bec991b9c
📒 Files selected for processing (40)
.gitignoreOrchestratorIDE.Avalonia/OrchestratorIDE.Avalonia.csprojOrchestratorIDE.NativeRuntime/OrchestratorIDE.NativeRuntime.csprojOrchestratorIDE.UnitTests/ContextFabricB2TopKRagTests.csOrchestratorIDE.UnitTests/ContextFabricCf0Tests.csOrchestratorIDE.UnitTests/ContextFabricEvidencePackTests.csOrchestratorIDE.UnitTests/ContextFabricExpandedCorpusTests.csOrchestratorIDE.UnitTests/ContextFabricOpenExtractionTests.csOrchestratorIDE.UnitTests/ExpandedFabricAuthoredQuestionMergerTests.csOrchestratorIDE.UnitTests/ExpandedFabricQuestionGeneratorTests.csOrchestratorIDE.UnitTests/ExpandedFabricQuestionSplitterTests.csOrchestratorIDE.UnitTests/ModelDepotTests.csOrchestratorIDE.UnitTests/ToolcallerDatasetCaptureTests.csOrchestratorIDE.slnxOrchestratorIDE/Agents/SwarmSession.csOrchestratorIDE/Core/Runtime/ModelAdmissionGate.csOrchestratorIDE/Services/ContextFabric/ContextFabricBaselineRunner.csOrchestratorIDE/Services/ContextFabric/ContextFabricContracts.csOrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.csOrchestratorIDE/Services/ContextFabric/ContextFabricValidation.csOrchestratorIDE/Services/ContextFabric/DeterministicExpandedFabricCorpus.csOrchestratorIDE/Services/ContextFabric/ExpandedFabricAuthoredQuestionMerger.csOrchestratorIDE/Services/ContextFabric/ExpandedFabricLedgerExport.csOrchestratorIDE/Services/ContextFabric/ExpandedFabricQuestionGenerator.csOrchestratorIDE/Services/ContextFabric/ExpandedFabricQuestionSplitter.csOrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.csTools/ContextFabricBench/Program.csTools/ContextFabricBench/Run-CF7GateExpanded.ps1Tools/ToolcallerBench/Program.csTools/ToolcallerBench/ToolcallerBench.csprojTools/ToolcallerBench/ToolcallerCaptureValidator.csTools/ToolcallerBench/ToolcallerContracts.csTools/ToolcallerBench/ToolcallerReportWriter.csdocs/CONTEXT_FABRIC_BENCHMARK_MANIFEST.mddocs/README.mddocs/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.mddocs/TOOLCALLER_V0_FROZEN_INVENTORY.mdtraining_pit/PLAN_CAPTURE_SCHEMA.mdtraining_pit/TOOLCALLER_CAPTURE_SCHEMA.mdtraining_pit/schemas/toolcaller_v0_frozen_tools.json
| A Warpath event is an immutable record of something that happened. | ||
|
|
||
| ```json | ||
| { | ||
| "event_id": "wp_evt_20260703_183012_0001", | ||
| "schema_version": "warpath-event-v1", | ||
| "occurred_at": "2026-07-03T18:30:12-07:00", | ||
| "workspace_id": "sha256-of-normalized-workspace-root-or-null", | ||
| "run_id": "optional-swarm-or-chat-run-id", | ||
| "event_type": "swarm.run.completed", | ||
| "source_system": "SwarmSession", | ||
| "actor": "system", | ||
| "role": "CODER", | ||
| "model": "qwen2.5-coder:14b", | ||
| "node_id": "optional-hive-node-id", | ||
| "payload": { | ||
| "success": true, | ||
| "files_changed": 3, | ||
| "tests_passed": true, | ||
| "review_verdict": "CLEAN" | ||
| }, | ||
| "privacy": { | ||
| "contains_user_content": false, | ||
| "safe_for_share_card": true | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use collision-resistant event IDs.
wp_evt_..._0001-style IDs will collide once multiple emitters/workers produce events, which breaks dedupe and badge traceability. Use UUID/ULID (and keep the timestamp as a separate field) instead.
Proposed fix
- "event_id": "wp_evt_20260703_183012_0001",
+ "event_id": "018f3e8e-7c2b-7f8d-9c6a-1d2f6e7b9a01",
+ "sequence": 1,📝 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.
| A Warpath event is an immutable record of something that happened. | |
| ```json | |
| { | |
| "event_id": "wp_evt_20260703_183012_0001", | |
| "schema_version": "warpath-event-v1", | |
| "occurred_at": "2026-07-03T18:30:12-07:00", | |
| "workspace_id": "sha256-of-normalized-workspace-root-or-null", | |
| "run_id": "optional-swarm-or-chat-run-id", | |
| "event_type": "swarm.run.completed", | |
| "source_system": "SwarmSession", | |
| "actor": "system", | |
| "role": "CODER", | |
| "model": "qwen2.5-coder:14b", | |
| "node_id": "optional-hive-node-id", | |
| "payload": { | |
| "success": true, | |
| "files_changed": 3, | |
| "tests_passed": true, | |
| "review_verdict": "CLEAN" | |
| }, | |
| "privacy": { | |
| "contains_user_content": false, | |
| "safe_for_share_card": true | |
| } | |
| } | |
| A Warpath event is an immutable record of something that happened. | |
🤖 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/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.md` around lines 324 - 349, The
Warpath event identifier format in the event record needs to avoid collisions
across concurrent emitters. Update the event schema/example around the immutable
event object to use a collision-resistant ID in event_id (for example UUID or
ULID) while keeping occurred_at as a separate timestamp field, and adjust any
text or examples that currently imply sequential wp_evt_..._0001-style IDs.
Refer to the event payload shown in the Warpath event section when making the
change.
| #### HIVE and Warband Badges | ||
|
|
||
| | Badge | Tier | Trigger | | ||
| |---|---|---| | ||
| | Campfire Lit | Bone | HIVE enabled | | ||
| | First Ally | Bone | First node paired | | ||
| | Crowned | Iron | Local machine elected Warchief | | ||
| | Crown Transfer | Blood | Warchief election succeeds after node loss | | ||
| | Three Fires Burning | Blood | 3 nodes online | | ||
| | Warband Deployed | Blood | First headless Warband connected | | ||
| | Cloud Raider | Blood | First cloud Warband completes a task | | ||
| | Fleet Commander | Gold Crown | 5+ nodes paired | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove cloud-work rewards from the starter badge list.
Cloud Raider rewards cloud-hosted execution even though the spec says Warpath is local-first and has no cloud dependency. That incentive conflicts with the product promise; keep it out of the MVP list or reframe it around local/distributed execution only.
Proposed fix
-| Cloud Raider | Blood | First cloud Warband completes a task |📝 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.
| #### HIVE and Warband Badges | |
| | Badge | Tier | Trigger | | |
| |---|---|---| | |
| | Campfire Lit | Bone | HIVE enabled | | |
| | First Ally | Bone | First node paired | | |
| | Crowned | Iron | Local machine elected Warchief | | |
| | Crown Transfer | Blood | Warchief election succeeds after node loss | | |
| | Three Fires Burning | Blood | 3 nodes online | | |
| | Warband Deployed | Blood | First headless Warband connected | | |
| | Cloud Raider | Blood | First cloud Warband completes a task | | |
| | Fleet Commander | Gold Crown | 5+ nodes paired | | |
| #### HIVE and Warband Badges | |
| | Badge | Tier | Trigger | | |
| |---|---|---| | |
| | Campfire Lit | Bone | HIVE enabled | | |
| | First Ally | Bone | First node paired | | |
| | Crowned | Iron | Local machine elected Warchief | | |
| | Crown Transfer | Blood | Warchief election succeeds after node loss | | |
| | Three Fires Burning | Blood | 3 nodes online | | |
| | Warband Deployed | Blood | First headless Warband connected | | |
| | Fleet Commander | Gold Crown | 5+ nodes paired | |
🤖 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/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.md` around lines 699 - 710, The
starter badge list in the HIVE and Warband Badges section includes a
cloud-specific reward that conflicts with the local-first Warpath spec. Remove
the Cloud Raider badge from the MVP badge table, or rename/reframe it so it only
rewards local or distributed execution with no cloud dependency. Keep the
remaining badges in the list unchanged and ensure the badge triggers described
in the whitepaper stay aligned with the product promise.
| ### 11.2 Trophy Definition Schema | ||
|
|
||
| ```json | ||
| { | ||
| "trophy_id": "the_black_anvil", | ||
| "schema_version": "warpath-trophy-v1", | ||
| "name": "The Black Anvil", | ||
| "tier": "mythic", | ||
| "description": "A candidate beat the current baseline under frozen evaluation and passed deployed-artifact verification.", | ||
| "requirements": [ | ||
| { "event_type": "foundry.candidate.evaluated", "payload_match": { "beats_baseline": true } }, | ||
| { "event_type": "foundry.candidate.deployed_artifact_verified", "payload_match": { "passed": true } } | ||
| ], | ||
| "share_safe": true | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the canonical Foundry event before depending on it.
foundry.candidate.deployed_artifact_verified is required by the trophy schema, but it never appears in the event inventory. That leaves the validator and badge rules without a single source of truth for the event name.
Proposed fix
+foundry.candidate.deployed_artifact_verified🤖 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/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.md` around lines 774 - 789, The
trophy schema for The Black Anvil depends on
foundry.candidate.deployed_artifact_verified, but that event is not defined
anywhere in the canonical event inventory. Add the missing event definition to
the Foundry event inventory/source-of-truth and ensure the trophy definition in
the trophy schema references that exact event name consistently, so the
validator and badge rules use a single canonical symbol.
| ### 13.4 GitHub Badge Export | ||
|
|
||
| Optional future export: | ||
|
|
||
| ```md | ||
|  | ||
|  | ||
|  | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid remote badge URLs in share-card exports.
These badge images make the “local markdown only” export fetch third-party assets and leak run metadata when rendered. Prefer embedded/local assets or a plain-text export path.
Proposed fix
-
-
-
+
+
+📝 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.
| ### 13.4 GitHub Badge Export | |
| Optional future export: | |
| ```md | |
|  | |
|  | |
|  | |
| ``` | |
| ### 13.4 GitHub Badge Export | |
| Optional future export: | |
🤖 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/THEORC_WARPATH_GAMIFICATION_WHITEPAPER.md` around lines 888 - 896, The
GitHub badge export section currently relies on remote shields.io image URLs,
which breaks the local-only export goal and can leak metadata. Update the
“GitHub Badge Export” content to use embedded/local assets or route this example
through a plain-text export path instead of external badge links, keeping the
change localized to the badge export snippet in the whitepaper.
| [Test] | ||
| public void BuildTopKText_SkipsOverBudgetSegment_ButStillFitsShorterLowerRankedOne() | ||
| { | ||
| // The highest-scoring segment is too large to fit; a shorter, lower-scoring but still | ||
| // relevant segment should still be included rather than leaving the budget unused. | ||
| var big = new FabricSegment("seg-big", 1, "Big", | ||
| "checksum CK-777 checksum CK-777 checksum CK-777 padding padding padding padding padding padding padding", | ||
| FabricHashing.Sha256("big"), 500); | ||
| var small = new FabricSegment("seg-small", 2, "Small", "checksum CK-777 noted briefly.", | ||
| FabricHashing.Sha256("small"), 15); | ||
| var corpus = new FabricCorpus("corpus-5", "doc-5", "gen-5", "digest-5", "1.0", [big, small], 515); | ||
| var question = new FabricBenchmarkQuestion( | ||
| "q-5", FabricQuestionKind.LocalFact, "What checksum was noted?", ["CK-777"], ["seg-small"]); | ||
| var fixture = new FabricBenchmarkFixture(corpus, [question]); | ||
|
|
||
| // AnswerMaxTokens tuned so the effective budget (~50 tokens) fits "small" (15 tokens) but | ||
| // not "big" (500 tokens), even though "big" scores higher (three checksum mentions). | ||
| var runner = new ContextFabricBaselineRunner(new ScriptedFabricRuntime(), Options(answerMaxTokens: 1632)); | ||
| var text = runner.BuildTopKText(fixture, question); | ||
|
|
||
| Assert.That(text, Does.Contain("noted briefly")); | ||
| Assert.That(text, Does.Not.Contain("padding")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test doesn't actually exercise its stated scenario — Tokenize dedupes per segment, so "big" doesn't outscore "small".
Tokenize returns a HashSet<string> per segment (ContextFabricBaselineRunner.cs line 478-483), so repeated occurrences of "checksum" in big don't increase its score — only presence matters. Computing IDF scores here: checksum has document-frequency 2 (df across both segments), noted has df 1 (only in small). That gives small a higher score (0.5 + 1.0 = 1.5) than big (0.5), not lower as the in-line comment claims ("even though 'big' scores higher"). The test still passes because the budget happens to only fit small regardless of ranking, but it never actually exercises the intended case — a higher-ranked-but-oversized segment being skipped in favor of a genuinely lower-ranked-but-smaller one that still fits.
🧪 Suggested fix: make "big" genuinely outrank "small"
- var big = new FabricSegment("seg-big", 1, "Big",
- "checksum CK-777 checksum CK-777 checksum CK-777 padding padding padding padding padding padding padding",
- FabricHashing.Sha256("big"), 500);
- var small = new FabricSegment("seg-small", 2, "Small", "checksum CK-777 noted briefly.",
- FabricHashing.Sha256("small"), 15);
+ // "big" must match strictly more distinct question terms than "small" to actually outrank
+ // it under presence/IDF scoring (Tokenize dedupes per segment, so repeated mentions of the
+ // same term do not increase score).
+ var big = new FabricSegment("seg-big", 1, "Big",
+ "checksum CK-777 was noted and verified with padding padding padding padding padding padding padding",
+ FabricHashing.Sha256("big"), 500);
+ var small = new FabricSegment("seg-small", 2, "Small", "checksum CK-777 noted briefly.",
+ FabricHashing.Sha256("small"), 15);📝 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.
| [Test] | |
| public void BuildTopKText_SkipsOverBudgetSegment_ButStillFitsShorterLowerRankedOne() | |
| { | |
| // The highest-scoring segment is too large to fit; a shorter, lower-scoring but still | |
| // relevant segment should still be included rather than leaving the budget unused. | |
| var big = new FabricSegment("seg-big", 1, "Big", | |
| "checksum CK-777 checksum CK-777 checksum CK-777 padding padding padding padding padding padding padding", | |
| FabricHashing.Sha256("big"), 500); | |
| var small = new FabricSegment("seg-small", 2, "Small", "checksum CK-777 noted briefly.", | |
| FabricHashing.Sha256("small"), 15); | |
| var corpus = new FabricCorpus("corpus-5", "doc-5", "gen-5", "digest-5", "1.0", [big, small], 515); | |
| var question = new FabricBenchmarkQuestion( | |
| "q-5", FabricQuestionKind.LocalFact, "What checksum was noted?", ["CK-777"], ["seg-small"]); | |
| var fixture = new FabricBenchmarkFixture(corpus, [question]); | |
| // AnswerMaxTokens tuned so the effective budget (~50 tokens) fits "small" (15 tokens) but | |
| // not "big" (500 tokens), even though "big" scores higher (three checksum mentions). | |
| var runner = new ContextFabricBaselineRunner(new ScriptedFabricRuntime(), Options(answerMaxTokens: 1632)); | |
| var text = runner.BuildTopKText(fixture, question); | |
| Assert.That(text, Does.Contain("noted briefly")); | |
| Assert.That(text, Does.Not.Contain("padding")); | |
| } | |
| [Test] | |
| public void BuildTopKText_SkipsOverBudgetSegment_ButStillFitsShorterLowerRankedOne() | |
| { | |
| // The highest-scoring segment is too large to fit; a shorter, lower-scoring but still | |
| // relevant segment should still be included rather than leaving the budget unused. | |
| // "big" must match strictly more distinct question terms than "small" to actually outrank | |
| // it under presence/IDF scoring (Tokenize dedupes per segment, so repeated mentions of the | |
| // same term do not increase score). | |
| var big = new FabricSegment("seg-big", 1, "Big", | |
| "checksum CK-777 was noted and verified with padding padding padding padding padding padding padding", | |
| FabricHashing.Sha256("big"), 500); | |
| var small = new FabricSegment("seg-small", 2, "Small", "checksum CK-777 noted briefly.", | |
| FabricHashing.Sha256("small"), 15); | |
| var corpus = new FabricCorpus("corpus-5", "doc-5", "gen-5", "digest-5", "1.0", [big, small], 515); | |
| var question = new FabricBenchmarkQuestion( | |
| "q-5", FabricQuestionKind.LocalFact, "What checksum was noted?", ["CK-777"], ["seg-small"]); | |
| var fixture = new FabricBenchmarkFixture(corpus, [question]); | |
| // AnswerMaxTokens tuned so the effective budget (~50 tokens) fits "small" (15 tokens) but | |
| // not "big" (500 tokens), even though "big" scores higher (three checksum mentions). | |
| var runner = new ContextFabricBaselineRunner(new ScriptedFabricRuntime(), Options(answerMaxTokens: 1632)); | |
| var text = runner.BuildTopKText(fixture, question); | |
| Assert.That(text, Does.Contain("noted briefly")); | |
| Assert.That(text, Does.Not.Contain("padding")); | |
| } |
🤖 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/ContextFabricB2TopKRagTests.cs` around lines 111 -
133, The test in
BuildTopKText_SkipsOverBudgetSegment_ButStillFitsShorterLowerRankedOne does not
match the intended ranking because Tokenize dedupes terms per segment, so
repeated “checksum” in the big segment does not raise its score. Update the test
data so the segment referenced by big genuinely outranks small under the
BuildTopKText scoring logic in ContextFabricBaselineRunner, while still being
too large for the token budget; keep small as the shorter fallback that fits and
is selected when big is skipped.
| "export-ledger" => BenchmarkSuite.ExportLedger, | ||
| "merge-authored" => BenchmarkSuite.MergeAuthored, | ||
| "cf7-gate-expanded" => BenchmarkSuite.Cf7GateExpanded, | ||
| _ => throw new ArgumentException("Unknown suite. Use cf0, quote-anchor, stitch, cf7-gate, scale, export-ledger, merge-authored, or cf7-gate-expanded."), | ||
| }; | ||
|
|
||
| private static void PrintUsage() | ||
| { | ||
| Console.WriteLine("Usage: context-fabric-bench --model-root <folder> [options]"); | ||
| Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | scale (default cf0)"); | ||
| Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale (default cf0)"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
PrintUsage suite list omits export-ledger and merge-authored.
ParseSuite accepts export-ledger, merge-authored, and cf7-gate-expanded (Line 467-470), and the unknown-suite error message lists all three, but the --suite help line at Line 476 only mentions cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale. Users running --help won't discover the two new suites.
📝 Proposed fix
- Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale (default cf0)");
+ Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale | export-ledger | merge-authored (default cf0)");📝 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.
| "export-ledger" => BenchmarkSuite.ExportLedger, | |
| "merge-authored" => BenchmarkSuite.MergeAuthored, | |
| "cf7-gate-expanded" => BenchmarkSuite.Cf7GateExpanded, | |
| _ => throw new ArgumentException("Unknown suite. Use cf0, quote-anchor, stitch, cf7-gate, scale, export-ledger, merge-authored, or cf7-gate-expanded."), | |
| }; | |
| private static void PrintUsage() | |
| { | |
| Console.WriteLine("Usage: context-fabric-bench --model-root <folder> [options]"); | |
| Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | scale (default cf0)"); | |
| Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale (default cf0)"); | |
| "export-ledger" => BenchmarkSuite.ExportLedger, | |
| "merge-authored" => BenchmarkSuite.MergeAuthored, | |
| "cf7-gate-expanded" => BenchmarkSuite.Cf7GateExpanded, | |
| _ => throw new ArgumentException("Unknown suite. Use cf0, quote-anchor, stitch, cf7-gate, scale, export-ledger, merge-authored, or cf7-gate-expanded."), | |
| }; | |
| private static void PrintUsage() | |
| { | |
| Console.WriteLine("Usage: context-fabric-bench --model-root <folder> [options]"); | |
| Console.WriteLine(" --suite <name> cf0 | quote-anchor | stitch | cf7-gate | cf7-gate-expanded | scale | export-ledger | merge-authored (default cf0)"); |
🤖 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 467 - 476, The `PrintUsage`
help text in `Program.cs` is missing the `export-ledger` and `merge-authored`
suite options even though `ParseSuite` and the unknown-suite message already
support them. Update the `Console.WriteLine` inside `PrintUsage` so the
`--suite` list includes `export-ledger` and `merge-authored` alongside the
existing `BenchmarkSuite` values, keeping the help output aligned with
`ParseSuite`.
| # Held-out questions | ||
| if (-not (Test-Path $HeldOutPath)) { | ||
| Write-Error "Held-out question file not found: $HeldOutPath`n" + | ||
| "Expected at .orc/adversarial/expanded-question-suite-heldout.json in the repo root.`n" + | ||
| "This file is generated by the question-suite build process and must be present." | ||
| exit 1 | ||
| } | ||
|
|
||
| # B4 artifact | ||
| if (-not $B4Artifact) { | ||
| Write-Error "No CF-6 acceptance artifact found in: $B4ArtifactDir`n" + | ||
| "Expected a file matching cf6-acceptance-*.json.`n" + | ||
| "Ensure the CF-6 HIVE acceptance run has been completed and its artifact is checked in." | ||
| exit 1 | ||
| } | ||
| Write-Host "B4 artifact: $B4Artifact" | ||
|
|
||
| # Model directory | ||
| if (-not (Test-Path $ModelRoot)) { | ||
| Write-Error "Model root not found: $ModelRoot`n" + | ||
| "Install a qualifying model (7B+ Admitted for CF) and ensure the directory exists." | ||
| exit 1 | ||
| } | ||
|
|
||
| $GgufCount = (Get-ChildItem $ModelRoot -Filter "*.gguf" -Recurse -ErrorAction SilentlyContinue).Count | ||
| if ($GgufCount -eq 0) { | ||
| Write-Error "No .gguf files found under: $ModelRoot`n" + | ||
| "The CF gate requires at least one 7B+ Admitted model (e.g. gemma-4-12B-it-qat-q4_0.gguf)." | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target region.
nl -ba Tools/ContextFabricBench/Run-CF7GateExpanded.ps1 | sed -n '145,190p'
# Probe PowerShell parsing/runtime behavior for the exact pattern if pwsh is available.
if command -v pwsh >/dev/null 2>&1; then
pwsh -NoLogo -NoProfile -Command '
$ErrorActionPreference = "Stop"
try {
Write-Error "A" + "B"
"NO_ERROR"
} catch {
"CAUGHT: $($_.FullyQualifiedErrorId)"
"MESSAGE: $($_.Exception.Message)"
}
'
else
echo "pwsh not available"
fiRepository: hardcoreerik/TheOrc
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("Tools/ContextFabricBench/Run-CF7GateExpanded.ps1")
start, end = 145, 190
for i, line in enumerate(path.read_text().splitlines(), 1):
if start <= i <= end:
print(f"{i:4}: {line}")
PY
if command -v pwsh >/dev/null 2>&1; then
pwsh -NoLogo -NoProfile -Command '
$ErrorActionPreference = "Stop"
foreach ($expr in @(
{ Write-Error "A" + "B" },
{ $x = "A" + "B"; Write-Error $x },
{ Write-Error ("A" + "B") }
)) {
try {
& $expr
"NO_ERROR"
} catch {
"CAUGHT: $($_.FullyQualifiedErrorId)"
"MESSAGE: $($_.Exception.Message)"
}
"---"
}
'
else
echo "pwsh not available"
fiRepository: hardcoreerik/TheOrc
Length of output: 2576
Parenthesize the Write-Error messages Write-Error "..." + "..." is parsed as separate arguments in PowerShell, so these pre-flight checks fail with a positional-parameter error instead of showing the intended message. Wrap each concatenation in parentheses (or assign the string first) at all four call sites.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'Run-CF7GateExpanded.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 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 152 - 181, The
pre-flight checks in Run-CF7GateExpanded.ps1 are using Write-Error with string
concatenation directly, which PowerShell parses as separate arguments and causes
a positional-parameter error. Update each of the four Write-Error call sites in
the held-out, B4 artifact, ModelRoot, and GgufCount checks so the full message
is parenthesized or built first in a variable before passing it to Write-Error.
Use the existing guard blocks and identifiers like $HeldOutPath, $B4Artifact,
$ModelRoot, and $GgufCount to locate them.
| // Gate: reason_code required for clarify/unsupported. | ||
| var needsReasonCode = capture.Expected.Decision is "clarify" or "unsupported"; | ||
| if (needsReasonCode && string.IsNullOrWhiteSpace(capture.Expected.ReasonCode)) | ||
| { | ||
| Fail(capture, "missing_reason_code", | ||
| $"Decision '{capture.Expected.Decision}' requires a non-null reason_code."); | ||
| } | ||
|
|
||
| if (capture.Expected.Decision == "call") | ||
| { | ||
| // Gate: call examples must name a tool. | ||
| if (string.IsNullOrWhiteSpace(capture.Expected.Tool)) | ||
| { | ||
| Fail(capture, "call_missing_tool", "Decision 'call' requires expected.tool."); | ||
| } | ||
| else | ||
| { | ||
| // Gate: target tool must exist in the frozen universe. | ||
| if (!toolsByName.TryGetValue(capture.Expected.Tool, out var tool)) | ||
| { | ||
| Fail(capture, "tool_outside_frozen_universe", | ||
| $"expected.tool '{capture.Expected.Tool}' is not in the frozen v0 tool set."); | ||
| } | ||
| else | ||
| { | ||
| // Gate: target tool must be in this example's own available_tools. | ||
| if (!capture.AvailableTools.Contains(capture.Expected.Tool, StringComparer.Ordinal)) | ||
| { | ||
| Fail(capture, "tool_outside_available_tools", | ||
| $"expected.tool '{capture.Expected.Tool}' is not in this example's available_tools."); | ||
| } | ||
|
|
||
| // Gate: no invented arguments, no missing required arguments. | ||
| var arguments = capture.Expected.Arguments ?? new Dictionary<string, System.Text.Json.JsonElement>(); | ||
| var invented = arguments.Keys.Where(k => !tool.Parameters.ContainsKey(k)).ToArray(); | ||
| if (invented.Length > 0) | ||
| { | ||
| Fail(capture, "invented_argument", | ||
| $"Argument(s) not in {tool.Name}'s frozen schema: {string.Join(", ", invented)}."); | ||
| } | ||
|
|
||
| var missingRequired = tool.Required.Where(r => !arguments.ContainsKey(r)).ToArray(); | ||
| if (missingRequired.Length > 0) | ||
| { | ||
| Fail(capture, "missing_required_argument", | ||
| $"{tool.Name} requires argument(s) not present: {string.Join(", ", missingRequired)}."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Gate: a proposed call must have policy_outcome evaluated. | ||
| if (capture.PolicyOutcome is null || !capture.PolicyOutcome.Evaluated) | ||
| { | ||
| Fail(capture, "call_missing_policy_outcome", | ||
| "Decision 'call' requires policy_outcome.evaluated == true."); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Non-call decisions should not carry an evaluated policy outcome — | ||
| // there is no proposed call to evaluate against ToolPolicyEngine. | ||
| if (capture.PolicyOutcome is { Evaluated: true }) | ||
| { | ||
| Info(capture, "policy_outcome_evaluated_without_call", | ||
| $"Decision '{capture.Expected.Decision}' has policy_outcome.evaluated == true; " + | ||
| "expected only for 'call' decisions."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unrecognized expected.decision values pass validation silently.
The gate logic only branches on Decision == "call" (Line 72) vs an implicit "everything else" (Line 121), and only checks reason_code for "clarify"/"unsupported" (Line 65). A capture with a typo'd or invalid decision string (e.g. "cal", "unknown") falls through both branches without any failure — it's silently treated like a valid no_tool-style decision and can be marked Passed. Since this tool's entire purpose is mechanical admission gating, an unrecognized decision value should be a hard failure.
Proposed fix: validate decision against a known set
+ var knownDecisions = new HashSet<string>(StringComparer.Ordinal) { "call", "no_tool", "clarify", "unsupported" };
+
foreach (var capture in captures)
{
+ if (!knownDecisions.Contains(capture.Expected.Decision))
+ {
+ Fail(capture, "unknown_decision", $"expected.decision '{capture.Expected.Decision}' is not a recognized value.");
+ }
+
// Gate: stale schema hash — ...📝 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.
| // Gate: reason_code required for clarify/unsupported. | |
| var needsReasonCode = capture.Expected.Decision is "clarify" or "unsupported"; | |
| if (needsReasonCode && string.IsNullOrWhiteSpace(capture.Expected.ReasonCode)) | |
| { | |
| Fail(capture, "missing_reason_code", | |
| $"Decision '{capture.Expected.Decision}' requires a non-null reason_code."); | |
| } | |
| if (capture.Expected.Decision == "call") | |
| { | |
| // Gate: call examples must name a tool. | |
| if (string.IsNullOrWhiteSpace(capture.Expected.Tool)) | |
| { | |
| Fail(capture, "call_missing_tool", "Decision 'call' requires expected.tool."); | |
| } | |
| else | |
| { | |
| // Gate: target tool must exist in the frozen universe. | |
| if (!toolsByName.TryGetValue(capture.Expected.Tool, out var tool)) | |
| { | |
| Fail(capture, "tool_outside_frozen_universe", | |
| $"expected.tool '{capture.Expected.Tool}' is not in the frozen v0 tool set."); | |
| } | |
| else | |
| { | |
| // Gate: target tool must be in this example's own available_tools. | |
| if (!capture.AvailableTools.Contains(capture.Expected.Tool, StringComparer.Ordinal)) | |
| { | |
| Fail(capture, "tool_outside_available_tools", | |
| $"expected.tool '{capture.Expected.Tool}' is not in this example's available_tools."); | |
| } | |
| // Gate: no invented arguments, no missing required arguments. | |
| var arguments = capture.Expected.Arguments ?? new Dictionary<string, System.Text.Json.JsonElement>(); | |
| var invented = arguments.Keys.Where(k => !tool.Parameters.ContainsKey(k)).ToArray(); | |
| if (invented.Length > 0) | |
| { | |
| Fail(capture, "invented_argument", | |
| $"Argument(s) not in {tool.Name}'s frozen schema: {string.Join(", ", invented)}."); | |
| } | |
| var missingRequired = tool.Required.Where(r => !arguments.ContainsKey(r)).ToArray(); | |
| if (missingRequired.Length > 0) | |
| { | |
| Fail(capture, "missing_required_argument", | |
| $"{tool.Name} requires argument(s) not present: {string.Join(", ", missingRequired)}."); | |
| } | |
| } | |
| } | |
| // Gate: a proposed call must have policy_outcome evaluated. | |
| if (capture.PolicyOutcome is null || !capture.PolicyOutcome.Evaluated) | |
| { | |
| Fail(capture, "call_missing_policy_outcome", | |
| "Decision 'call' requires policy_outcome.evaluated == true."); | |
| } | |
| } | |
| else | |
| { | |
| // Non-call decisions should not carry an evaluated policy outcome — | |
| // there is no proposed call to evaluate against ToolPolicyEngine. | |
| if (capture.PolicyOutcome is { Evaluated: true }) | |
| { | |
| Info(capture, "policy_outcome_evaluated_without_call", | |
| $"Decision '{capture.Expected.Decision}' has policy_outcome.evaluated == true; " + | |
| "expected only for 'call' decisions."); | |
| } | |
| } | |
| // Gate: reason_code required for clarify/unsupported. | |
| var needsReasonCode = capture.Expected.Decision is "clarify" or "unsupported"; | |
| if (needsReasonCode && string.IsNullOrWhiteSpace(capture.Expected.ReasonCode)) | |
| { | |
| Fail(capture, "missing_reason_code", | |
| $"Decision '{capture.Expected.Decision}' requires a non-null reason_code."); | |
| } | |
| if (capture.Expected.Decision == "call") | |
| { | |
| // Gate: call examples must name a tool. | |
| if (string.IsNullOrWhiteSpace(capture.Expected.Tool)) | |
| { | |
| Fail(capture, "call_missing_tool", "Decision 'call' requires expected.tool."); | |
| } | |
| else | |
| { | |
| // Gate: target tool must exist in the frozen universe. | |
| if (!toolsByName.TryGetValue(capture.Expected.Tool, out var tool)) | |
| { | |
| Fail(capture, "tool_outside_frozen_universe", | |
| $"expected.tool '{capture.Expected.Tool}' is not in the frozen v0 tool set."); | |
| } | |
| else | |
| { | |
| // Gate: target tool must be in this example's own available_tools. | |
| if (!capture.AvailableTools.Contains(capture.Expected.Tool, StringComparer.Ordinal)) | |
| { | |
| Fail(capture, "tool_outside_available_tools", | |
| $"expected.tool '{capture.Expected.Tool}' is not in this example's available_tools."); | |
| } | |
| // Gate: no invented arguments, no missing required arguments. | |
| var arguments = capture.Expected.Arguments ?? new Dictionary<string, System.Text.Json.JsonElement>(); | |
| var invented = arguments.Keys.Where(k => !tool.Parameters.ContainsKey(k)).ToArray(); | |
| if (invented.Length > 0) | |
| { | |
| Fail(capture, "invented_argument", | |
| $"Argument(s) not in {tool.Name}'s frozen schema: {string.Join(", ", invented)}."); | |
| } | |
| var missingRequired = tool.Required.Where(r => !arguments.ContainsKey(r)).ToArray(); | |
| if (missingRequired.Length > 0) | |
| { | |
| Fail(capture, "missing_required_argument", | |
| $"{tool.Name} requires argument(s) not present: {string.Join(", ", missingRequired)}."); | |
| } | |
| } | |
| } | |
| // Gate: a proposed call must have policy_outcome evaluated. | |
| if (capture.PolicyOutcome is null || !capture.PolicyOutcome.Evaluated) | |
| { | |
| Fail(capture, "call_missing_policy_outcome", | |
| "Decision 'call' requires policy_outcome.evaluated == true."); | |
| } | |
| } | |
| else | |
| { | |
| // Non-call decisions should not carry an evaluated policy outcome — | |
| // there is no proposed call to evaluate against ToolPolicyEngine. | |
| if (capture.PolicyOutcome is { Evaluated: true }) | |
| { | |
| Info(capture, "policy_outcome_evaluated_without_call", | |
| $"Decision '{capture.Expected.Decision}' has policy_outcome.evaluated == true; " + | |
| "expected only for 'call' decisions."); | |
| } | |
| } |
🤖 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/ToolcallerBench/ToolcallerCaptureValidator.cs` around lines 64 - 131,
Add explicit validation in ToolcallerCaptureValidator so unexpected
capture.Expected.Decision values fail fast instead of falling through the
non-call branch. Introduce a known allowed set in the validator around the
existing decision gates (the clarify/unsupported reason_code check and the
call/non-call split), and if Decision is anything other than the recognized
values, call Fail with a clear invalid_decision error before any other checks.
| public sealed record ToolcallerCapture( | ||
| [property: JsonPropertyName("schema_version")] string SchemaVersion, | ||
| [property: JsonPropertyName("tool_schema_hash")] string ToolSchemaHash, | ||
| [property: JsonPropertyName("example_id")] string ExampleId, | ||
| [property: JsonPropertyName("lineage_group_id")] string LineageGroupId, | ||
| [property: JsonPropertyName("captured_at")] DateTimeOffset? CapturedAt, | ||
| [property: JsonPropertyName("provenance")] ToolcallerProvenance Provenance, | ||
| [property: JsonPropertyName("role")] string Role, | ||
| [property: JsonPropertyName("request")] string Request, | ||
| [property: JsonPropertyName("available_tools")] IReadOnlyList<string> AvailableTools, | ||
| [property: JsonPropertyName("approval_state")] string ApprovalState, | ||
| [property: JsonPropertyName("expected")] ToolcallerExpected Expected, | ||
| [property: JsonPropertyName("policy_outcome")] ToolcallerPolicyOutcome? PolicyOutcome, | ||
| [property: JsonPropertyName("review_status")] string ReviewStatus, | ||
| [property: JsonPropertyName("reviewer")] string? Reviewer, | ||
| [property: JsonPropertyName("split")] string Split, | ||
| [property: JsonPropertyName("notes")] string? Notes, | ||
| [property: JsonPropertyName("tags")] IReadOnlyList<string>? Tags); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does System.Text.Json enforce non-nullable reference type properties on deserialization by default in .NET 10?
💡 Result:
No, System.Text.Json does not enforce non-nullable reference type properties by default in.NET 10 [1][2]. While support for respecting nullability annotations was introduced in.NET 9, it is an opt-in feature to maintain backward compatibility with existing applications [3][4]. To enable this behavior, you must explicitly set the RespectNullableAnnotations property to true in your JsonSerializerOptions [3][1]: JsonSerializerOptions options = new { RespectNullableAnnotations = true }; JsonSerializer.Deserialize(json, options); You can also enable this globally for an application using the System.Text.Json.Serialization.RespectNullableAnnotationsDefault feature switch in your project file [1][2]. Important limitations to consider: - Enforcement is restricted to non-generic properties, fields, and constructor parameters due to how nullable reference types are represented at runtime [3][1]. - It does not apply to top-level types, collection elements, or generic parameters [5][1]. - Nullability validation is independent of "required-ness" validation; to ensure properties are actually present in the JSON payload, you should use the required keyword or enable the RespectRequiredConstructorParameters option [1][6]. Microsoft recommends that new applications enable both RespectNullableAnnotations and RespectRequiredConstructorParameters to ensure stricter data validation [1][6].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.respectnullableannotations?view=net-9.0
- 2: https://learn.microsoft.com/uk-ua/dotnet/api/system.text.json.jsonserializeroptions.respectnullableannotations?view=net-10.0&viewFallbackFrom=net-9.0-pp
- 3: https://devblogs.microsoft.com/dotnet/system-text-json-in-dotnet-9/
- 4: Add a STJ feature flag recognizing nullable reference type annotations on properties dotnet/runtime#100144
- 5: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/nullable-annotations
- 6: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/required-properties
Mark critical capture fields as required Non-nullable ToolcallerCapture members can still deserialize as null when omitted unless required or stricter System.Text.Json options are enabled. That lets malformed capture JSON slip past the admission gate and fail later in validation or downstream access.
🤖 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/ToolcallerBench/ToolcallerContracts.cs` around lines 26 - 43, The
ToolcallerCapture contract currently relies on non-nullable types alone, which
still allows omitted JSON fields to deserialize as null; update the
ToolcallerCapture record so the critical members are explicitly required (or
otherwise enforced via stricter deserialization settings) to reject malformed
capture payloads at admission. Focus on the ToolcallerCapture record and its
JsonPropertyName-backed members, especially the non-optional fields like
SchemaVersion, ToolSchemaHash, ExampleId, LineageGroupId, Provenance, Role,
Request, AvailableTools, ApprovalState, Expected, ReviewStatus, and Split.
| > **Status:** Defined and auto-populated. `OrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.cs` | ||
| > stages real, organic "call" and "no_tool" examples from live swarm tool-call decisions to | ||
| > `.orc/swarm/dataset-staging/toolcaller/`, called from `RunWorkerLoopAsync`'s tool-execution | ||
| > loop. This is TheOrc generating its own F-1 training data from real usage rather than | ||
| > synthetic-only authoring — see the coverage-strategy note in | ||
| > [TOOLCALLER_V0_FROZEN_INVENTORY.md](../docs/TOOLCALLER_V0_FROZEN_INVENTORY.md). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no other code path appends a "toolcaller" subfolder before writing captures
rg -n 'DatasetStagingDir|dataset-staging' OrchestratorIDE/Agents/SwarmSession.cs OrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.cs OrchestratorIDE/Services/Swarm/DatasetCapture.csRepository: hardcoreerik/TheOrc
Length of output: 856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SwarmSession relevant lines ==\n'
sed -n '115,140p;2040,2080p' OrchestratorIDE/Agents/SwarmSession.cs
printf '\n== DatasetCapture header ==\n'
sed -n '1,40p' OrchestratorIDE/Services/Swarm/DatasetCapture.cs
printf '\n== Toolcaller capture references ==\n'
rg -n 'StageCallAsync|StageNoToolAsync|toolcaller_capture_|toolcaller' OrchestratorIDE/Services/Swarm/ToolcallerDatasetCapture.cs OrchestratorIDE/Agents/SwarmSession.csRepository: hardcoreerik/TheOrc
Length of output: 6673
Align the documented staging path with the code — toolcaller captures go to .orc/swarm/dataset-staging/ and are distinguished by the toolcaller_capture_ filename prefix, not a toolcaller/ subdirectory.
🤖 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 `@training_pit/TOOLCALLER_CAPTURE_SCHEMA.md` around lines 4 - 9, The documented
toolcaller staging location is incorrect and should match the implementation in
ToolcallerDatasetCapture and RunWorkerLoopAsync. Update the description to say
captures are written under .orc/swarm/dataset-staging/ and identified by the
toolcaller_capture_ filename prefix, rather than implying a toolcaller/
subdirectory. Keep the wording aligned with the actual capture flow and naming
used by the dataset staging code.
Reviewer passStrong PR overall. I like the honesty here: the previous CF-7 “GO” was overclaimed, the new expanded corpus/held-out suite exposes that, and the PR reports the real result as NO-GO instead of trying to dress it up. That is exactly the kind of truth discipline this project needs. What looks good
Main concern before mergeI would not treat this as fully merge-ready until the capture behavior is clarified:
The docs already say captures need sanitizer + human review, which is good. I’d still prefer the runtime hook default to off unless the operator intentionally enables Foundry capture. Merge caveats
VerdictGood direction, good honesty, good tests. I would call this approve-after-capture-default/visibility is resolved and CodeRabbit completes, with the full 120-question rerun as the next required proof artifact, not a hidden assumption. |
…filtering
All 12 Exhaustive failures in the CF-7 gate run hit the identical error: "answer
claim ... contains more than N citations". The old filter accepted a claim if it
shared ANY word with the question (Tokenize(claim.Text).Any(terms.Contains)).
For "list every case-file ID under ledger case-ledger-01", corpus-idiomatic
filler words ("ledger", "recorded") appear in nearly every claim across all 15
ledgers, not just case-ledger-01's -- so the answer pulled in claims from every
unrelated ledger, blowing past FabricAnswerVerifier's runaway-answer sanity cap.
This went through three design iterations, each caught by actually testing
against realistic data rather than assuming success:
1. First attempt: IDF-weighted aggregate score, threshold relative to the best
match. Verified NOT to work: with ~15 similarly-sized ledgers, "case-ledger-
01"'s distinguishing suffix and "case-ledger-09"'s are each about equally
rare corpus-wide, so their aggregate scores come out identical -- IDF alone
measures overall rarity, not "is this the specific entity the question
names," and can't discriminate among many equally-rare alternatives.
2. Second attempt: identify the question's single rarest present term and hard-
require it. Broke an existing test on the frozen 16-segment corpus
("list every archive token in section order") where every segment is
genuinely relevant -- there's no single rare instance to filter on, since
"archive"/"token"/"section" appear in literally every card.
3. Actual fix: classify the question by whether its rarest present term is
still rare relative to the corpus (appears in a minority of cards) vs.
common (appears in a majority). Minority -> entity-scoped, hard-require that
term (fixes the ledger case). Majority -> category-wide, fall back to "any
non-stopword term matches" (fixes the archive-token case). Also caught mid-
fix: "every" (from "list EVERY archive token") isn't a stopword and happened
to have the lowest document frequency in the frozen corpus purely by
incidental phrasing in 3 unrelated sentences, wrongly getting treated as the
distinguishing entity -- added "every", "list", "order" to the stopword list
as generic exhaustive-question quantifiers that should never be treated as
content-bearing.
Research note: confirmed via web search that IDF/BM25-style weighting is the
standard technique for this class of problem (not something to replace with
embeddings/NER here); BM25's term-saturation refinement was considered but not
adopted, since scoring here is bounded by the question's own small fixed term
set rather than open-ended claim length, which already limits the risk BM25's
saturation guards against.
TokenizeForScoring (min length 2, vs. the shared Tokenize's min length 3) is
introduced as a separate tokenizer used only by this scoring path -- Tokenize's
3-character minimum would silently drop 2-digit identifiers like "01" from
"case-ledger-01" (split on the hyphen), destroying the exact signal needed to
tell ledger-01 apart from ledger-09. Scoped narrowly to avoid affecting
Tokenize's other unrelated callers in this file.
3 new unit tests reproduce the exact real-world identifier pattern (not a
simplified toy example) and the entity-scoped/category-wide distinction. Full
suite: 470 pass, 0 fail, 4 skipped.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.UnitTests/ContextFabricExhaustiveAnswerTests.cs`:
- Around line 79-88: The exhaustive-answer test setup is inconsistent with the
“all entries” expectation, since FabricBenchmarkQuestion is only initialized
with CASE-01-0 and segmentIds[0] while the assertion expects all five segments.
Update the question fixture in ContextFabricExhaustiveAnswerTests to use
expectations that match the full ledger-case scenario, and keep the
BuildExhaustiveAnswer assertion aligned so the test verifies the intended
all-entries behavior rather than a partial case.
In `@OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs`:
- Around line 996-1005: Add “under” to the _scoringStopwords set in
ContextFabricFeasibilityRunner so it is ignored by mostDistinctiveTerms. Update
the stopword list where the existing terms are defined to prevent “under” from
being selected as a distinctive term and causing ledger cards to be missed.
- Around line 771-785: The selection logic in ContextFabricFeasibilityRunner
currently drops all but one matching claim per card because the projection uses
FirstOrDefault() after filtering claims. Update the selected pipeline so it
returns every claim that matches the distinctive-term filter, preserving the
existing card ordering by corpus.Segments ordinal and the ScoreTextIdf ranking
only where needed, and use the unique symbols cards, Claim, TokenizeForScoring,
ScoreTextIdf, and selected to locate the query.
🪄 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: 836ccc3f-aa02-4b4c-8a8c-eb23ecc2b6da
📒 Files selected for processing (2)
OrchestratorIDE.UnitTests/ContextFabricExhaustiveAnswerTests.csOrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs
| var question = new FabricBenchmarkQuestion( | ||
| "q-2", FabricQuestionKind.Exhaustive, | ||
| "List every case-file ID recorded under ledger case-ledger-01, in any order.", | ||
| ["CASE-01-0"], [segmentIds[0]]); | ||
|
|
||
| var runner = new ContextFabricFeasibilityRunner(new ScriptedFabricRuntime(), FabricRunOptions.Default); | ||
| var result = runner.BuildExhaustiveAnswer(corpus, question, cards); | ||
|
|
||
| Assert.That(result.IncludedSegmentIds, Has.Count.EqualTo(5)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the benchmark question expectations with the “all entries” scenario.
This test says all five ledger entries should be included, but the question only declares CASE-01-0 and segmentIds[0] as expected. That can mask verifier regressions and makes the fixture contradict the behavior under test.
Proposed test tightening
+ var expectedAnswers = Enumerable.Range(0, 5)
+ .Select(i => $"CASE-01-{i}")
+ .ToArray();
var question = new FabricBenchmarkQuestion(
"q-2", FabricQuestionKind.Exhaustive,
"List every case-file ID recorded under ledger case-ledger-01, in any order.",
- ["CASE-01-0"], [segmentIds[0]]);
+ expectedAnswers, segmentIds);
var runner = new ContextFabricFeasibilityRunner(new ScriptedFabricRuntime(), FabricRunOptions.Default);
var result = runner.BuildExhaustiveAnswer(corpus, question, cards);
- Assert.That(result.IncludedSegmentIds, Has.Count.EqualTo(5));
+ Assert.Multiple(() =>
+ {
+ Assert.That(result.IncludedSegmentIds, Is.EquivalentTo(segmentIds));
+ foreach (var expected in expectedAnswers)
+ Assert.That(result.Answer?.Answer, Does.Contain(expected));
+ });📝 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.
| var question = new FabricBenchmarkQuestion( | |
| "q-2", FabricQuestionKind.Exhaustive, | |
| "List every case-file ID recorded under ledger case-ledger-01, in any order.", | |
| ["CASE-01-0"], [segmentIds[0]]); | |
| var runner = new ContextFabricFeasibilityRunner(new ScriptedFabricRuntime(), FabricRunOptions.Default); | |
| var result = runner.BuildExhaustiveAnswer(corpus, question, cards); | |
| Assert.That(result.IncludedSegmentIds, Has.Count.EqualTo(5)); | |
| } | |
| var expectedAnswers = Enumerable.Range(0, 5) | |
| .Select(i => $"CASE-01-{i}") | |
| .ToArray(); | |
| var question = new FabricBenchmarkQuestion( | |
| "q-2", FabricQuestionKind.Exhaustive, | |
| "List every case-file ID recorded under ledger case-ledger-01, in any order.", | |
| expectedAnswers, segmentIds); | |
| var runner = new ContextFabricFeasibilityRunner(new ScriptedFabricRuntime(), FabricRunOptions.Default); | |
| var result = runner.BuildExhaustiveAnswer(corpus, question, cards); | |
| Assert.Multiple(() => | |
| { | |
| Assert.That(result.IncludedSegmentIds, Is.EquivalentTo(segmentIds)); | |
| foreach (var expected in expectedAnswers) | |
| Assert.That(result.Answer?.Answer, Does.Contain(expected)); | |
| }); |
🤖 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/ContextFabricExhaustiveAnswerTests.cs` around lines
79 - 88, The exhaustive-answer test setup is inconsistent with the “all entries”
expectation, since FabricBenchmarkQuestion is only initialized with CASE-01-0
and segmentIds[0] while the assertion expects all five segments. Update the
question fixture in ContextFabricExhaustiveAnswerTests to use expectations that
match the full ledger-case scenario, and keep the BuildExhaustiveAnswer
assertion aligned so the test verifies the intended all-entries behavior rather
than a partial case.
| private static readonly HashSet<string> _scoringStopwords = new(StringComparer.Ordinal) | ||
| { | ||
| var haystack = string.Join(' ', card.Claims.Select(claim => claim.Text).Prepend(card.Summary)); | ||
| return Tokenize(haystack).Count(terms.Contains); | ||
| "the", "and", "for", "are", "was", "were", "this", "that", "these", "those", "with", | ||
| "from", "into", "onto", "than", "then", "there", "here", "when", "where", "what", | ||
| "which", "who", "whom", "whose", "why", "how", "not", "nor", "but", "does", "did", | ||
| "has", "have", "had", "will", "would", "should", "can", "could", "may", "might", | ||
| "shall", "must", "its", "his", "her", "their", "our", "your", "you", "she", "him", | ||
| "they", "them", "been", "being", "any", "all", "each", "some", "such", "own", "same", | ||
| "is", "at", "to", "of", "in", "on", "by", "no", "an", "we", "it", "as", "or", "be", "do", | ||
| "every", "list", "order", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add under to scoring stopwords to avoid rare-term misselection.
The exhaustive questions use “under ledger ...”. If any unrelated card contains under once, it becomes rarer than 01, so mostDistinctiveTerms can become {"under"} and exclude the actual ledger cards.
Proposed fix
- "every", "list", "order",
+ "every", "list", "order", "under",📝 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.
| private static readonly HashSet<string> _scoringStopwords = new(StringComparer.Ordinal) | |
| { | |
| var haystack = string.Join(' ', card.Claims.Select(claim => claim.Text).Prepend(card.Summary)); | |
| return Tokenize(haystack).Count(terms.Contains); | |
| "the", "and", "for", "are", "was", "were", "this", "that", "these", "those", "with", | |
| "from", "into", "onto", "than", "then", "there", "here", "when", "where", "what", | |
| "which", "who", "whom", "whose", "why", "how", "not", "nor", "but", "does", "did", | |
| "has", "have", "had", "will", "would", "should", "can", "could", "may", "might", | |
| "shall", "must", "its", "his", "her", "their", "our", "your", "you", "she", "him", | |
| "they", "them", "been", "being", "any", "all", "each", "some", "such", "own", "same", | |
| "is", "at", "to", "of", "in", "on", "by", "no", "an", "we", "it", "as", "or", "be", "do", | |
| "every", "list", "order", | |
| private static readonly HashSet<string> _scoringStopwords = new(StringComparer.Ordinal) | |
| { | |
| "the", "and", "for", "are", "was", "were", "this", "that", "these", "those", "with", | |
| "from", "into", "onto", "than", "then", "there", "here", "when", "where", "what", | |
| "which", "who", "whom", "whose", "why", "how", "not", "nor", "but", "does", "did", | |
| "has", "have", "had", "will", "would", "should", "can", "could", "may", "might", | |
| "shall", "must", "its", "his", "her", "their", "our", "your", "you", "she", "him", | |
| "they", "them", "been", "being", "any", "all", "each", "some", "such", "own", "same", | |
| "is", "at", "to", "of", "in", "on", "by", "no", "an", "we", "it", "as", "or", "be", "do", | |
| "every", "list", "order", "under", |
🤖 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 996 - 1005, Add “under” to the _scoringStopwords set in
ContextFabricFeasibilityRunner so it is ignored by mostDistinctiveTerms. Update
the stopword list where the existing terms are defined to prevent “under” from
being selected as a distinctive term and causing ledger cards to be missed.
Ran Tools/grok-review.ps1 against the fix (3ef5fb0) per request. Real findings addressed: - BLOCKER: BuildExhaustiveAnswer's segment-ordinal lookup used corpus.Segments.First(...), throwing if a card's SegmentId isn't in the corpus. Pre-existing behavior (verified via git history), but making the method internal for direct unit testing made it more reachable than the real RunAsync call path (which always keeps cards and corpus in sync) would ever hit. Now filters out any card whose segment isn't found instead of throwing. - MINOR: the old Score(FabricEvidenceCard, HashSet<string>) helper had zero remaining callers after BuildEvidencePack and BuildExhaustiveAnswer both switched to ScoreTextIdf -- removed. - MINOR: TokenizeForScoring duplicated Tokenize's split/trim/lowercase logic, differing only in minimum token length -- both now delegate to a shared TokenizeWithMinLength(value, minLength) helper. One finding not acted on: flagging OrchestratorIDE.UnitTests/*.cs against a "T##_*.cs in OrchestratorIDE.UITests/Tests" convention -- that convention is for FlaUI UI-automation tests in a different project; every other test in this PR (and the pre-existing suite) already lives directly under OrchestratorIDE.UnitTests, so this is a false positive from the review script's generic convention note, not a real violation. One finding accepted as a known, considered tradeoff rather than changed: BuildExhaustiveAnswer hardcodes Abstained=false even when nothing survives the new stricter filter, which fails verification with "no valid citation" rather than abstaining. Setting Abstained=true would not actually help: for a normal (non-ExpectAbstention) Exhaustive question, FabricAnswerVerifier flags abstention itself as an error ("answer unexpectedly abstained"). Either way a genuinely-empty match correctly fails verification instead of fabricating content; that's the honest outcome, not a silent regression. Also noted, not changed here: Grok correctly flagged that the entity-scoped vs. category-wide heuristic (minDocumentFrequency < cards.Count/2) is a heuristic, not a proof -- a category-wide question whose real content terms happen to have <50% document frequency by corpus coincidence would still mis-classify as entity-scoped. Both real scenarios this session uncovered (ledger-scoped, archive-token-wide) are covered by tests; a case built specifically to sit at that boundary is not, and remains a known residual risk worth watching if a future gate run surfaces a new Exhaustive failure pattern. 470 pass, 0 fail, 4 skipped. Full grok output: .orc/reviews/grok_20260703_185505.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ToolcallerDatasetCapture.IsEnabled now defaults to false and is driven by a new AppSettings.ToolcallerDatasetCaptureEnabled toggle in the Settings panel, addressing the PR #34 review concern that silent, on-by-default capture of raw tool-call arguments (paths, shell commands, file contents) doesn't fit a local-first/privacy-first project even though the staging directory is gitignored. Whenever capture is on, a "Dataset Gathering Active" pill appears in the status bar (click to open Settings) so the behavior is never invisible to the user.
|
Addressed in 2e18805: Confirmed Full test suite still green (470/0/4). Still waiting on the full 120-question re-run before claiming any benchmark improvement — not done yet. |
Three real bugs in BuildExhaustiveAnswer/tests that were posted by
CodeRabbit before merge but not addressed in time:
- FirstOrDefault() kept only the single highest-scoring claim per
card, silently dropping other genuinely matching claims when a card
lists multiple distinct entries. Now keeps every matching claim,
ordered by segment then score; IncludedSegmentIds deduped since a
card can contribute more than one claim.
- "under" wasn't a scoring stopword, so "list every X under ledger Y"
could let an incidental single mention of "under" in an unrelated
card look rarer than the actual distinguishing identifier and wrongly
become the sole distinctive term, excluding the real target cards.
Same failure class as the earlier "every" bug.
- ContextFabricExhaustiveAnswerTests' 5-entry test only asserted a
count, not that all 5 case IDs were actually present -- wouldn't
have caught the FirstOrDefault() bug above. Tightened, plus added a
dedicated multi-claim-per-card regression test.
- ContextFabricB2TopKRagTests' big-vs-small budget test repeated the
same term three times expecting it to outscore a single mention, but
Tokenize() returns a per-segment HashSet so repetition doesn't
increase score -- the test passed without exercising the ranking
behavior it claimed to. Rewritten so "big" genuinely outranks "small"
via distinct rare terms ("batch", "inspector") "small" lacks.
Full suite: 471/0/4 (was 470/0/4; +1 new regression test).
Summary
This branch does two things:
falseC/trueX), and a live re-run script.theorc-toolcallerv0): a frozen, code-verified tool inventory, a new dataset capture schema, a working mechanical validator, and a live capture hook so TheOrc generates its own F-1 training data from real swarm tool-call decisions instead of synthetic-only authoring.The honest result
The full 120-question gate run against the real held-out set closed NO-GO:
CF clearly beats every baseline, and every honesty metric passed cleanly (
citation_precision100%,segment_terminal_coverage100%,boundary_stitch_pass_rate100%) — CF never fabricates evidence. The only failing gate isquestion_pass_rate(target 100%).Root cause, found and fixed
Comparing
expectedSegmentIdsagainstincludedSegmentIdson failing questions showed CF frequently never gathered the segments containing the answer — an evidence-selection bug, not a reasoning failure. Two real production-code fixes landed:BuildEvidencePack(ContextFabricFeasibilityRunner.cs— the actual production Context Fabric answering path used byFabricNativeReaderServiceandHiveNativeRoleExecutorAdapter, not benchmark-only code) had a hardcodedmaxCardscap (1/2/4) with no documented cost/latency justification, structurally incapable of answering GlobalSynthesis questions needing up to 8 segments. Replaced with IDF-weighted, stopword-aware scoring and budget-fill selection instead of a fixed count.ContextFabricBaselineRunner.BuildTopKText) first — it had the identical bug, and its old naive implementation actually scored worse (21%) than the dumber B1 baseline (26%), which is itself good evidence the bug was real.Known, explicitly out-of-scope remainder:
FabricQuestionKind.Exhaustive(0/12) doesn't go throughBuildEvidencePackat all — it hits a separate method,BuildExhaustiveAnswer, with its own literal-keyword-overlap filter. Different root cause, not fixed here, tracked as a follow-up.Neither fix has been validated against a fresh full 120-question run yet — that's the natural next step once this PR is reviewed.
Test plan
Tools/ToolcallerBenchvalidator and passed with 0 findings🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
toolcaller-benchvalidator that outputs JSON/Markdown reports.Bug Fixes
Tests
Documentation