feat(runtime): Phase D real-model E2E lane with retained evidence artifact - #81
Merged
Merged
Conversation
…ifact Native Runtime v2.0 Phase D's remaining DoD item: "the lane runs green on a real reference box and emits a retained evidence artifact." The negative fail-closed test (#78) and the cancellation leg (#79) both landed already; this closes the happy-path piece. NativeRuntimeE2ELaneTests drives discovery (ModelDepot.Scan) through a REAL admission decision -- a real OrcScheduler against a real live-queried NativeVramProbe budget, deliberately not the allowUnbudgetedExecution opt-out every other gated lane in this workstream uses -- through adapter lifecycle, real inference, and telemetry reads at three stages (before/mid-flight/after), writing a retained JSON evidence artifact regardless of pass or fail. NativeE2ELaneEvidenceStore follows the same schema_version/timestamp_utc/ app_version convention as the existing NativeRuntimeFallbackEvidenceStore and NativeRuntimeComparisonReportStore, under .orc/native-e2e-lane/ -- a sibling store, not a fork of the pattern, since this is the successful-run case those two don't cover (the fallback store only persists non-success outcomes). Verified on real hardware, 3 repeated runs: 25.7 tok/s, 284ms TTFT, 2.5GB measured VRAM. The captured telemetry surfaced a genuinely interesting, correct distinction: residency (ActiveCount) drops to 0 once the conversation disposes, but the VRAM reservation deliberately stays live with the loaded model rather than releasing -- confirming those two lifecycles are intentionally decoupled, not a leak. Full 637-test suite (including every gated real-model lane) stays green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a retained JSON evidence store, integrates it with a real-model Native Runtime lifecycle test, and updates the Native Runtime v2 specification with the landed Phase D evidence lane. ChangesNative E2E evidence lane
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test as NativeRuntimeE2ELaneTests
participant Depot as Model depot
participant Runtime as NativeRoleRuntime
participant Store as NativeE2ELaneEvidenceStore
participant Disk as JSON evidence file
Test->>Depot: scan GGUF model depot
Test->>Runtime: create runtime with live NVIDIA budget
Test->>Runtime: capture before telemetry
Test->>Runtime: stream worker completion
Test->>Runtime: capture mid-flight and after telemetry
Test->>Store: write NativeE2ELaneRunResult
Store->>Disk: persist sanitized JSON evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
OrchestratorIDE.UnitTests/NativeRuntimeE2ELaneTests.cs (1)
61-90: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cancellation/timeout on the real-inference streaming call.
StreamRoleCompletionAsyncat Line 70 is awaited with noCancellationToken. A real-hardware stall (driver/GPU issue) during this Phase D validation run would hang the test indefinitely instead of failing with retained evidence.Suggested fix
+ using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); ... - await foreach (var token in runtime.StreamRoleCompletionAsync(RuntimeRole.Worker, messages, maxTokens: 64)) + await foreach (var token in runtime.StreamRoleCompletionAsync(RuntimeRole.Worker, messages, maxTokens: 64, cancellationToken: cts.Token))🤖 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/NativeRuntimeE2ELaneTests.cs` around lines 61 - 90, Add a bounded cancellation or timeout to the real-inference streaming call in the test around StreamRoleCompletionAsync, pass its token into the streaming API, and ensure timeout cancellation is caught so the test records the failure in errorMessage while preserving retained telemetry snapshots.
🤖 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/NATIVE_RUNTIME_V2_SPEC.md`:
- Line 656: Update the measurements in the referenced performance text to
include spaces between each numeric value and its unit, such as “284 ms” and
“25.7 tok/s,” while preserving the existing values and wording.
- Around line 14-19: Update the Phase D status paragraph in
NATIVE_RUNTIME_V2_SPEC.md to reflect that the evidence-artifact E2E lane has
landed and passed, using the documented results in the Phase D section. Replace
the “in flight (PR pending)” status and retain only extending the lane to a
second concurrent role as remaining work.
In `@OrchestratorIDE.UnitTests/NativeRuntimeE2ELaneTests.cs`:
- Around line 92-124: Update the NativeE2ELaneEvidenceStore.WriteAsync call in
the NativeE2ELaneRunResult flow to pass the workspace root explicitly, ensuring
the evidence artifact is written under the workspace’s .orc/native-e2e-lane
directory instead of the ApplicationData fallback.
In `@OrchestratorIDE/Core/Runtime/NativeE2ELaneEvidence.cs`:
- Around line 70-82: Update NativeE2ELaneEvidence.WriteAsync so the output
filename timestamp uses UTC, matching the timestamp_utc value produced by
BuildRecord; replace the local-time source in the native_e2e_lane filename while
preserving the existing format and file-writing behavior.
---
Nitpick comments:
In `@OrchestratorIDE.UnitTests/NativeRuntimeE2ELaneTests.cs`:
- Around line 61-90: Add a bounded cancellation or timeout to the real-inference
streaming call in the test around StreamRoleCompletionAsync, pass its token into
the streaming API, and ensure timeout cancellation is caught so the test records
the failure in errorMessage while preserving retained telemetry snapshots.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3654a0-3256-480e-a6a8-0442cf17207e
📒 Files selected for processing (4)
OrchestratorIDE.NativeRuntime/OrchestratorIDE.NativeRuntime.csprojOrchestratorIDE.UnitTests/NativeRuntimeE2ELaneTests.csOrchestratorIDE/Core/Runtime/NativeE2ELaneEvidence.csdocs/NATIVE_RUNTIME_V2_SPEC.md
Comment on lines
+14
to
+19
| > [Phase D](#phase-d--real-model-native-path-proof-lane) is in progress: the negative | ||
| > "admission denial fails closed, no silent Ollama substitution" test (§3.4, PR #78) and the | ||
| > cancellation leg (PR #79 — found and fixed a real bug in the process, see the Phase D section | ||
| > below) have both landed as always-on tests, one of them hardware-gated. The evidence-artifact | ||
| > E2E lane (discovery through telemetry, with a real admission budget and a retained JSON | ||
| > artifact) is in flight (PR pending). Still open: extending that lane to a second concurrent |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the Phase D status to match the landed evidence lane.
This block says the evidence-artifact E2E lane is “in flight (PR pending),” but lines 645-657 document it as landed and passing across three runs. Since this is the authoritative shipped-state section, update it to say the lane has landed and leave only the second concurrent role as remaining work.
Proposed correction
-[Phase D](`#phase-d--real-model-native-path-proof-lane`) is in progress: ...
-The evidence-artifact E2E lane ... is in flight (PR pending).
+[Phase D](`#phase-d--real-model-native-path-proof-lane`) is in progress: ...
+The evidence-artifact E2E lane ... has landed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| > [Phase D](#phase-d--real-model-native-path-proof-lane) is in progress: the negative | |
| > "admission denial fails closed, no silent Ollama substitution" test (§3.4, PR #78) and the | |
| > cancellation leg (PR #79 — found and fixed a real bug in the process, see the Phase D section | |
| > below) have both landed as always-on tests, one of them hardware-gated. The evidence-artifact | |
| > E2E lane (discovery through telemetry, with a real admission budget and a retained JSON | |
| > artifact) is in flight (PR pending). Still open: extending that lane to a second concurrent | |
| > [Phase D](`#phase-d--real-model-native-path-proof-lane`) is in progress: the negative | |
| > "admission denial fails closed, no silent Ollama substitution" test (§3.4, PR `#78`) and the | |
| > cancellation leg (PR `#79` — found and fixed a real bug in the process, see the Phase D section | |
| > below) have both landed as always-on tests, one of them hardware-gated. The evidence-artifact | |
| > E2E lane (discovery through telemetry, with a real admission budget and a retained JSON | |
| > artifact) has landed. Still open: extending that lane to a second concurrent |
🤖 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/NATIVE_RUNTIME_V2_SPEC.md` around lines 14 - 19, Update the Phase D
status paragraph in NATIVE_RUNTIME_V2_SPEC.md to reflect that the
evidence-artifact E2E lane has landed and passed, using the documented results
in the Phase D section. Replace the “in flight (PR pending)” status and retain
only extending the lane to a second concurrent role as remaining work.
Three real, valid findings from the first genuine CodeRabbit pass under the new .coderabbit.yaml config: 1. No bound on the real streaming call -- a hardware stall would hang the test indefinitely instead of failing with retained evidence. Added a 2-minute CancellationTokenSource. 2. Evidence was written to %AppData% instead of the repo's .orc/native-e2e-lane/ convention every other evidence store in this workstream uses, because WriteAsync was never given a workspaceRoot. Added a FindRepoRoot() helper (walks up from the test binary looking for OrchestratorIDE.slnx) and passed it through. Verified: evidence now lands under the actual repo, confirmed on a real run. 3. The evidence filename used DateTime.Now while the record's own timestamp_utc field used UtcNow -- inconsistent, and could sort wrong across machine timezones. Now captures one DateTimeOffset.UtcNow and threads it through both. Also fixed a minor unit-spacing nitpick in the spec doc (284ms -> 284 ms, 2.5GB -> 2.5 GB). Skipped one suggestion: CodeRabbit's prompt asked the spec's Phase D banner to say the E2E lane "has landed" -- this PR isn't merged yet, so that would be the overclaiming this whole workstream has deliberately avoided. Left as "in flight (PR pending)," accurate as of this commit. Re-verified on real hardware: full 637-test suite green, E2E lane test passes with evidence now correctly retained in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three real nits from the cloud multi-agent review, all verified against current code before fixing: 1. NativeE2ELaneRunResult dropped the exception TYPE on failure, only keeping ex.Message -- forking the sibling NativeRuntimeFallbackEvidenceStore pattern this store's own doc claims to follow (that one captures both ErrorType and ErrorMessage). A failed run's evidence couldn't distinguish an admission denial from a native load crash from a timeout. Added ErrorType, populated as ex.GetType().Name, threaded through to the JSON record. 2. Assert.Multiple unconditionally indexed snapshots[1]/[2] assuming 3 entries, but the mid-flight snapshot is only appended once a token is actually yielded -- a failure BEFORE the first token (an admission denial or native load failure, exactly the case this lane's real, non-opted-out admission is designed to exercise) leaves Count at 2, and the unconditional index would throw ArgumentOutOfRangeException, burying the real failure already surfaced via `success`/`errorMessage` under confusing noise. Guarded the count-dependent asserts behind an explicit count check. 3. Two doc-only contradictions in NATIVE_RUNTIME_V2_SPEC.md, both introduced across this PR and earlier commits: the Phase B addendum still said "PR #77, in flight" after #77 had actually merged; and this PR's own Phase D section said the E2E lane "landed" while the banner correctly said "in flight (PR pending)" -- fixed the stale #77 reference to "landed", and reworded the Phase D section to match the banner's honest "PR pending" framing rather than claim this PR landed before it's merged. Re-verified on real hardware: full 637-test suite green, E2E lane test passes with error_type now present (null on the successful run) in the retained evidence artifact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 23 minutes. |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
hardcoreerik
added a commit
that referenced
this pull request
Jul 20, 2026
… criteria (#83) Maps each of NATIVE_RUNTIME_V2_SPEC.md §6's entry criteria to a concrete, evidence-bearing campaign phase (HV-0..HV-6) across the real three-machine fleet: NewcorePC (Warchief, RTX 5070 Ti 16GB), HardcorePC (RTX 3050 6GB), HardcoreLaptopMSI (RTX 4060 8GB). Fleet surveyed live over SSH during planning (GPUs, drivers, repo paths/states verified, both remotes synced to master @ aa07f41); pinned test-GGUF distribution to both workers initiated with SHA-256 recorded for per-box verification. Honest blockers listed up front: HardcorePC's unresolved native-lib regression is a hard HV-0 precondition (and doubles as the campaign's first real diagnosability exercise), the laptop's CUDA-redist bundling must be verified per-box, and PR #81/#82 must merge first. The plan produces the §6 EVIDENCE only -- the flip itself stays gated on an explicit recorded product decision, per the spec. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
hardcoreerik
added a commit
that referenced
this pull request
Jul 20, 2026
CodeRabbit finding: ROADMAP.md's banner still said the E2E lane was "built, verified on real hardware, and pending merge" while CURRENT_STATE.yaml's parallel note omitted that qualifier entirely -- inconsistent, and now stale either way since PR #81 actually merged. Updated both to say landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hardcoreerik
added a commit
that referenced
this pull request
Jul 20, 2026
…endum + D (#82) * docs: sync ROADMAP/CURRENT_STATE with Native Runtime v2.0 Phase B addendum + D Both files still described Phase B's cost-estimate half as deliberately deferred and Phase D as untouched -- stale since PR #76 (cost estimate), #77 (load-time measurement), #78 (negative fail-closed test), and #79 (cancellation-corruption fix) all landed since the last sync. Updates the Native Runtime v2.0 direction table in ROADMAP.md and the native_runtime note in CURRENT_STATE.yaml to reflect current reality: Phase B fully landed (estimate + measurement), Phase D essentially complete pending the E2E evidence-lane PR (#81, open). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: mark E2E evidence lane landed (PR #81 merged), not pending CodeRabbit finding: ROADMAP.md's banner still said the E2E lane was "built, verified on real hardware, and pending merge" while CURRENT_STATE.yaml's parallel note omitted that qualifier entirely -- inconsistent, and now stale either way since PR #81 actually merged. Updated both to say landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NativeRuntimeE2ELaneTestsdrives discovery → real admission (a realOrcScheduleragainst a real live-queriedNativeVramProbebudget — deliberately not theallowUnbudgetedExecutionopt-out every other gated lane uses) → adapter lifecycle → real inference → telemetry reads at three stages (before/mid-flight/after), writing a retained JSON evidence artifact regardless of pass/fail.NativeE2ELaneEvidenceStorefollows the same convention as the existing evidence stores (schema_version/timestamp_utc/app_version, under.orc/native-e2e-lane/) — a sibling, not a fork of the pattern.Real hardware result (attached as a captured example, not the artifact itself — that's gitignored/local)
ActiveCount) drops to 0 once the conversation disposes, but the VRAM reservation deliberately stays live with the loaded model rather than releasing — confirming those two lifecycles are intentionally decoupled, not a leak.Test plan
dotnet build OrchestratorIDE.slnx— clean🤖 Generated with Claude Code
Summary by CodeRabbit