feat(harness-e2e): collect lifecycle evidence - #773
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe E2E harness now collects session-tree data, transcripts, and statuses; builds execution evidence; evaluates structural gates; preserves partial observations; and records teardown outcomes in run reports. ChangesE2E execution evidence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2eContext
participant HarnessAPI
participant EvidenceBuilder
participant GateEvaluator
participant E2eRunReport
E2eContext->>HarnessAPI: retrieve session tree, statuses, and transcripts
E2eContext-->>EvidenceBuilder: provide collected session data
EvidenceBuilder->>EvidenceBuilder: build execution evidence
EvidenceBuilder-->>GateEvaluator: provide evidence and session tree
GateEvaluator->>GateEvaluator: evaluate structural gates
GateEvaluator-->>E2eRunReport: add gates and harness evidence
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
harness/tests/e2e/src/evidence.rs (6)
295-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the offending call identifiers in the gate reason.
The failure message reports only counts. When this gate fails in CI, the reader cannot identify which calls lack results without opening the full evidence payload. Add a bounded sample of
(session_id, call_id)pairs to the reason string.♻️ Proposed refactor sketch
- if bad > 0 || orphaned > 0 { - Some(format!( - "{bad} call(s) have missing or duplicate results; {orphaned} result(s) are orphaned" - )) - } else { - None - } + if bad > 0 || orphaned > 0 { + let sample = evidence + .calls + .iter() + .filter(|call| { + !matches!( + call.result, + FunctionResultEvidence::Succeeded { .. } | FunctionResultEvidence::Failed { .. } + ) + }) + .take(5) + .map(|call| { + format!( + "{}:{}", + call.session_id, + call.call_id.as_deref().unwrap_or("<no-call-id>") + ) + }) + .collect::<Vec<_>>() + .join(", "); + Some(format!( + "{bad} call(s) have missing or duplicate results; {orphaned} result(s) are orphaned; sample=[{sample}]" + )) + } else { + None + }🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 295 - 301, Update the gate-reason construction in the bad/orphaned evidence check to include a bounded sample of offending (session_id, call_id) pairs alongside the existing counts. Reuse the available evidence data and keep the output concise, ensuring the reason still reports counts and remains unchanged when no failures exist.
156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
evidence.completefor the tree-completeness gate.
build_evidencealready computestree.complete && all sessions terminalintoevidence.complete. The gate recomputes the same expression. If one definition changes later, the two can diverge.♻️ Proposed refactor
- let tree_complete = - metrics.complete && tree.complete && evidence.sessions.iter().all(|s| s.terminal); + let tree_complete = metrics.complete && evidence.complete;🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 156 - 157, Update the tree-completeness gate in the surrounding evidence evaluation to reuse evidence.complete instead of recomputing tree.complete and session terminality. Preserve the existing metrics.complete requirement while relying on the canonical value produced by build_evidence.
304-360: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSilent fallback when the transcript shape does not match.
Line 311 requires
messages.messagesto be a JSON array. If the transcript payload uses a different envelope,entriesis empty, no calls or timeline events are produced, and every structural gate passes vacuously. The harness then reports success with no evidence.Consider recording a per-session parse indicator in the evidence so a shape change surfaces as a gate failure rather than as empty evidence.
🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 304 - 360, Update normalize_transcripts to record whether each session contains the expected messages.messages JSON array, and propagate that parse indicator through the evidence result. Ensure structural validation treats a session with a missing or differently shaped envelope as a gate failure instead of accepting empty calls and timeline evidence.
430-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
_nudge_substring match is a fragile classifier.Line 433 classifies an entry as
ValidationNudgewhen itsentry_idcontains_nudge_. An entry id that contains that substring for an unrelated reason is misclassified. Theorigin.validationcheck on Line 434 is the reliable signal.If the producer always sets
origin.validationfor nudges, remove the substring heuristic. If it does not, add a comment that names the producer that generates the_nudge_id format.🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 430 - 437, Update the `TimelineEventKind` classification condition to rely on `origin.validation == Some(true)` and remove the fragile `entry_id` `_nudge_` substring check if producers consistently set that field; otherwise retain the heuristic only with a comment identifying the producer responsible for the `_nudge_` ID format.
482-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the structural gate functions.
The tests cover
normalize_transcriptsonly.ownership_error,call_integrity_error, andbuild_evidencecarry the gate logic and have no coverage. Uncovered branches include the duplicate-session-id check, the depth mismatch check, the cycle check, and the orphaned-result count.I can generate table-driven tests for these functions. Do you want me to open an issue to track this?
🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 482 - 538, Extend the existing tests module with focused tests for ownership_error, call_integrity_error, and build_evidence. Cover duplicate session IDs, depth mismatches, cyclic parent relationships, and orphaned-result counts, asserting each gate reports the expected error or evidence outcome; use table-driven cases where practical.
203-205: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the status string from
TurnStatusserialization.
AwaitingFunctionsserializes asawaiting_functions, but the currentDebugconversion producesawaitingfunctions. Use the serialized value instatus_namebefore writing the evidence toresults.json.🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 203 - 205, Update status_name to derive the status string from TurnStatus serialization rather than the Debug representation, preserving the expected snake_case value such as awaiting_functions before evidence is written to results.json.
🤖 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 `@harness/tests/e2e/src/evidence.rs`:
- Around line 461-478: Update classify_entry to emit a timeline event for every
assistant content block whose type is "function_call" instead of selecting only
the first via find. Adjust the surrounding timeline construction to increment
the sequence per emitted call, preserving each block’s call_id and function_id
so parallel calls are all represented in results.json.
In `@harness/tests/e2e/src/suite.rs`:
- Around line 444-485: Update the collection flow around session_tree,
session_transcripts, and statuses so each successful checkpoint is persisted:
assign report.metrics immediately after metrics collection, extract the root
transcript from session_transcripts and assign report.transcript before
requesting statuses, and reuse that transcript for ScenarioObservation. Keep
report.harness_evidence assigned only after build_evidence completes
successfully.
---
Nitpick comments:
In `@harness/tests/e2e/src/evidence.rs`:
- Around line 295-301: Update the gate-reason construction in the bad/orphaned
evidence check to include a bounded sample of offending (session_id, call_id)
pairs alongside the existing counts. Reuse the available evidence data and keep
the output concise, ensuring the reason still reports counts and remains
unchanged when no failures exist.
- Around line 156-157: Update the tree-completeness gate in the surrounding
evidence evaluation to reuse evidence.complete instead of recomputing
tree.complete and session terminality. Preserve the existing metrics.complete
requirement while relying on the canonical value produced by build_evidence.
- Around line 304-360: Update normalize_transcripts to record whether each
session contains the expected messages.messages JSON array, and propagate that
parse indicator through the evidence result. Ensure structural validation treats
a session with a missing or differently shaped envelope as a gate failure
instead of accepting empty calls and timeline evidence.
- Around line 430-437: Update the `TimelineEventKind` classification condition
to rely on `origin.validation == Some(true)` and remove the fragile `entry_id`
`_nudge_` substring check if producers consistently set that field; otherwise
retain the heuristic only with a comment identifying the producer responsible
for the `_nudge_` ID format.
- Around line 482-538: Extend the existing tests module with focused tests for
ownership_error, call_integrity_error, and build_evidence. Cover duplicate
session IDs, depth mismatches, cyclic parent relationships, and orphaned-result
counts, asserting each gate reports the expected error or evidence outcome; use
table-driven cases where practical.
- Around line 203-205: Update status_name to derive the status string from
TurnStatus serialization rather than the Debug representation, preserving the
expected snake_case value such as awaiting_functions before evidence is written
to results.json.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fa64ff9-d959-4bf6-9c65-18bafc6f9d2b
📒 Files selected for processing (6)
harness/tests/e2e/src/context.rsharness/tests/e2e/src/evidence.rsharness/tests/e2e/src/main.rsharness/tests/e2e/src/report.rsharness/tests/e2e/src/scenarios/mod.rsharness/tests/e2e/src/suite.rs
| if message.get("role").and_then(Value::as_str) == Some("assistant") { | ||
| if let Some(block) = message | ||
| .get("content") | ||
| .and_then(Value::as_array) | ||
| .into_iter() | ||
| .flatten() | ||
| .find(|b| b.get("type").and_then(Value::as_str) == Some("function_call")) | ||
| { | ||
| return TimelineEventKind::FunctionCall { | ||
| call_id: block.get("id").and_then(Value::as_str).map(str::to_owned), | ||
| function_id: block | ||
| .get("function_id") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("unknown") | ||
| .into(), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The timeline records only the first function call in a multi-call assistant entry.
classify_entry returns one TimelineEventKind per entry and uses find to select the first function_call block. normalize_transcripts instead emits one FunctionCallEvidence for every block. An assistant entry with parallel tool calls therefore produces N calls but a single timeline FunctionCall event.
The current gates do not depend on this, because orphan detection reads only FunctionResult events. However the persisted timeline in results.json under-reports parallel calls, so any consumer that reconstructs execution order from the timeline sees incomplete data.
Consider emitting one timeline event per function_call block, with the sequence incremented per block.
🤖 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 `@harness/tests/e2e/src/evidence.rs` around lines 461 - 478, Update
classify_entry to emit a timeline event for every assistant content block whose
type is "function_call" instead of selecting only the first via find. Adjust the
surrounding timeline construction to increment the sequence per emitted call,
preserving each block’s call_id and function_id so parallel calls are all
represented in results.json.
| let session_tree = context | ||
| .session_tree(session_id) | ||
| .await | ||
| .map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?; | ||
| let session_transcripts = context | ||
| .transcripts_for_tree(&session_tree) | ||
| .await | ||
| .map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?; | ||
| let statuses = context | ||
| .statuses_for_tree(&session_tree) | ||
| .await | ||
| .map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?; | ||
| let transcript = session_transcripts | ||
| .iter() | ||
| .find(|item| item.session_id == session_id) | ||
| .map(|item| item.messages.clone()) | ||
| .ok_or_else(|| { | ||
| collection_failure( | ||
| FailurePhase::Collect, | ||
| "root transcript is absent from session tree".into(), | ||
| ) | ||
| })?; | ||
| let execution_evidence = build_evidence( | ||
| &root_terminal_status, | ||
| &session_tree, | ||
| &session_transcripts, | ||
| &statuses, | ||
| ); | ||
| let structural_gates = evaluate_structural_gates(&metrics, &session_tree, &execution_evidence); | ||
| let response = common::final_response(&transcript); | ||
| let observation = ScenarioObservation { | ||
| metrics, | ||
| root_terminal_status, | ||
| session_tree, | ||
| session_transcripts, | ||
| execution_evidence, | ||
| transcript, | ||
| response, | ||
| }; | ||
| report.transcript = Some(observation.transcript.clone()); | ||
| report.metrics = Some(observation.metrics.clone()); | ||
| let objective = (spec.evaluate)(context, &observation, run_id) | ||
| report.harness_evidence = Some(observation.execution_evidence.clone()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist each successful collection checkpoint.
Line 447 can return after metrics collection. Lines 451 and 455 can return after additional successful collection. In each case, E2eRunReport loses the already collected metrics and, after transcript collection, the root transcript.
Store report.metrics immediately after metrics collection. Extract and store the root transcript before requesting statuses. Keep harness_evidence conditional on complete collection.
Proposed fix
let metrics = match context
.wait_for_complete_metrics(spec.id, session_id, stuck_timeout, progress_interval)
.await
@@
}
};
+ report.metrics = Some(metrics.clone());
+
let session_tree = context
.session_tree(session_id)
.await
@@
.await
.map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?;
- let statuses = context
- .statuses_for_tree(&session_tree)
- .await
- .map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?;
let transcript = session_transcripts
.iter()
.find(|item| item.session_id == session_id)
@@
)
})?;
+ report.transcript = Some(transcript.clone());
+
+ let statuses = context
+ .statuses_for_tree(&session_tree)
+ .await
+ .map_err(|error| collection_failure(FailurePhase::Collect, error.to_string()))?;🤖 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 `@harness/tests/e2e/src/suite.rs` around lines 444 - 485, Update the collection
flow around session_tree, session_transcripts, and statuses so each successful
checkpoint is persisted: assign report.metrics immediately after metrics
collection, extract the root transcript from session_transcripts and assign
report.transcript before requesting statuses, and reuse that transcript for
ScenarioObservation. Keep report.harness_evidence assigned only after
build_evidence completes successfully.
1 participant
Summary
StatusReportand collect the final session treeHarnessExecutionEvidenceresults.jsonError semantics
Failures to collect the tree, statuses, or transcripts remain infrastructure errors in the collect phase. Structural gates only fail when evidence was successfully collected and violates the lifecycle contract.
Validation
cargo fmt --allcargo test -p harness-e2e— 102 passedSummary by CodeRabbit
New Features
Bug Fixes