(MOT-4380) refactor(harness): simplify objective E2E assessments - #755
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR introduces canonical required and signal assessment infrastructure. It migrates the persistent-state scenario to reusable assessment specifications, weighted objective scoring, hard gates, scenario version 2, and expanded validation coverage. ChangesAssessment contract and scenario migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PersistentStateScenario
participant AssessmentSpec
participant ObjectiveEvaluator
participant ObjectiveReport
PersistentStateScenario->>AssessmentSpec: evaluate scenario checks
AssessmentSpec->>ObjectiveEvaluator: return awards and gate status
ObjectiveEvaluator->>ObjectiveReport: aggregate criteria into objective results
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
skill-check — worker0 verified, 56 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
harness/tests/e2e/src/scenarios/persistent_state.rs (2)
100-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
binaryfor the confirmation award.
confirmation_pointsis either0orCONFIRMATION.weight().pointscan never fail here, so the?and theResultreturn type add unreachable error handling.binaryexpresses the same rule directly and letsassessbecome infallible.♻️ Proposed refactor
- let concise_confirmation = response_present && response_chars <= 240; - let confirmation_points = if concise_confirmation { - CONFIRMATION.weight() - } else { - 0 - }; + let concise_confirmation = response_present && response_chars <= 240; - Ok(assessment::objective([ + assessment::objective([ DURABLE_RESULT.binary( state_matches, format!("expected {expected}, observed {observed}"), ), FUNCTION_DISCIPLINE.binary( function_discipline, format!( "exact_write={exact_write}; observed {state_set_calls} state::set call(s); observed {function_call_errors} function-call error(s)" ), ), - CONFIRMATION.points( - confirmation_points, + CONFIRMATION.binary( + concise_confirmation, format!( "response_present={response_present}; observed {response_chars} character(s); limit 240" ), - )?, - ])) + ), + ]) }Change the signature to
-> ObjectiveEvaluationand wrap the call inevaluatewithOk(...). Then drop.unwrap()from the tests.🤖 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/scenarios/persistent_state.rs` around lines 100 - 126, Update the assessment helper containing confirmation_points to return ObjectiveEvaluation directly, replace CONFIRMATION.points(...) with CONFIRMATION.binary(concise_confirmation, ...), and remove the unnecessary ? propagation. Adjust its evaluate caller to wrap the infallible result in Ok(...) and remove related test unwraps.
232-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the 240-character boundary case.
The test covers
241characters and the empty response. It does not cover exactly240characters. A change of<=to<inassesswould not fail any test.💚 Proposed test addition
#[test] fn missing_or_overlong_confirmation_is_non_blocking() { let value = json!({ "stored": true }); for response in [String::new(), "x".repeat(241)] { let evaluation = assess(&value, &value, true, 1, 0, &response).unwrap(); assert!(evaluation.hard_gates.iter().all(|gate| gate.passed)); assert_eq!(award(&evaluation, "confirmation"), 0); assert_eq!(total(&evaluation), 90); } } + + #[test] + fn confirmation_at_the_length_limit_scores_full_points() { + let value = json!({ "stored": true }); + let evaluation = assess(&value, &value, true, 1, 0, &"x".repeat(240)).unwrap(); + + assert_eq!(award(&evaluation, "confirmation"), 10); + assert_eq!(total(&evaluation), 100); + }🤖 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/scenarios/persistent_state.rs` around lines 232 - 243, Add an exact 240-character response case to the input set in missing_or_overlong_confirmation_is_non_blocking, alongside the empty and 241-character cases, so assess explicitly verifies the maximum allowed confirmation length.
🤖 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/scenarios/persistent_state.rs`:
- Around line 89-118: Update the FUNCTION_DISCIPLINE assessment description in
assess to explicitly state that any function-call error in the session causes
failure, while retaining the existing function_discipline condition and
state::set call details.
---
Nitpick comments:
In `@harness/tests/e2e/src/scenarios/persistent_state.rs`:
- Around line 100-126: Update the assessment helper containing
confirmation_points to return ObjectiveEvaluation directly, replace
CONFIRMATION.points(...) with CONFIRMATION.binary(concise_confirmation, ...),
and remove the unnecessary ? propagation. Adjust its evaluate caller to wrap the
infallible result in Ok(...) and remove related test unwraps.
- Around line 232-243: Add an exact 240-character response case to the input set
in missing_or_overlong_confirmation_is_non_blocking, alongside the empty and
241-character cases, so assess explicitly verifies the maximum allowed
confirmation length.
🪄 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: 33dff56c-2240-4c6e-8e31-e56fd30889f6
📒 Files selected for processing (4)
harness/tests/e2e/README.mdharness/tests/e2e/src/scenarios/assessment.rsharness/tests/e2e/src/scenarios/mod.rsharness/tests/e2e/src/scenarios/persistent_state.rs
| fn assess( | ||
| expected: &Value, | ||
| observed: &Value, | ||
| exact_write: bool, | ||
| state_set_calls: usize, | ||
| function_call_errors: u64, | ||
| response: &str, | ||
| ) -> anyhow::Result<ObjectiveEvaluation> { | ||
| let state_matches = observed == expected; | ||
| let function_discipline = exact_write && function_call_errors == 0; | ||
| let response_present = !response.trim().is_empty(); | ||
| let response_chars = response.chars().count(); | ||
| let concise_confirmation = response_present && response_chars <= 240; | ||
| let confirmation_points = if concise_confirmation { | ||
| CONFIRMATION.weight() | ||
| } else { | ||
| 0 | ||
| }; | ||
|
|
||
| Ok(assessment::objective([ | ||
| DURABLE_RESULT.binary( | ||
| state_matches, | ||
| format!("expected {expected}, observed {observed}"), | ||
| ), | ||
| FUNCTION_DISCIPLINE.binary( | ||
| function_discipline, | ||
| format!( | ||
| "exact_write={exact_write}; observed {state_set_calls} state::set call(s); observed {function_call_errors} function-call error(s)" | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the function_discipline description with the evaluated condition.
function_discipline fails when function_call_errors > 0. That counter covers every function call in the session, not only state::set calls. The declared description states "Exactly one successful write targets the requested scope and key." An unrelated recovered function error therefore fails a required hard gate that the criterion text does not describe.
Either narrow the condition to write-related errors, or extend the description so the report explains the broader rule.
📝 Proposed description change
const FUNCTION_DISCIPLINE: AssessmentSpec = AssessmentSpec::required(
"function_discipline",
30,
- "Exactly one successful write targets the requested scope and key.",
+ "Exactly one successful write targets the requested scope and key, with no function-call errors in the run.",
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/tests/e2e/src/scenarios/persistent_state.rs` around lines 89 - 118,
Update the FUNCTION_DISCIPLINE assessment description in assess to explicitly
state that any function-call error in the session causes failure, while
retaining the existing function_discipline condition and state::set call
details.
1 participant
Summary
persistent_stateto canonicaldurable_result,function_discipline, andconfirmationassessmentsWhy
The previous evaluator repeated the same requirements across criteria, gates, and awards, allowing their identifiers and behavior to drift independently. The new internal abstraction keeps those outputs aligned while preserving the existing score and blocking semantics.
The
persistent_statecontract advances to version 2 because its three implementation-oriented gates are consolidated into two criterion-aligned gates. The report schema and 90-point threshold are unchanged.Validation
cargo fmt --manifest-path harness/Cargo.toml --all -- --checkcargo test --locked --manifest-path harness/Cargo.toml -p harness-e2e(111 passed)cargo clippy --locked --manifest-path harness/Cargo.toml -p harness-e2e -- -D warningsgit diff --checkFixes MOT-4380
Summary by CodeRabbit
New Features
Documentation