From 4c994950e1f5eff71f3359c04e9a0823b230153b Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Wed, 9 Sep 2026 00:19:02 +0800 Subject: [PATCH] Derive the report fields the model kept missing A response was refused when its improvement plan was out of order or its phase tags disagreed with that plan, and both of those follow from the plan itself, so an assessment the model had otherwise earned reached the candidate as no evaluation. The server derives them now and the model answers only for the mapping it alone knows. The call around it was measured rather than guessed: attempts were cancelled under the model's own median latency, thinking was spending the output budget the report had to fit in, and the deadline funded fewer calls than the budget handed out. Separately, the browser bounded a summary the agent validates at 1200 characters to 300, so every candidate read one that stopped mid-sentence. --- src/agent.rs | 2 +- src/agent/report.rs | 187 ++++++++++++++++++++++++-------------- src/gemini.rs | 121 ++++++++++++++++++------ src/livekit.rs | 9 +- tests/agent.rs | 153 +++++++++++++++++++++++++------ tests/browser/lib.test.js | 16 +++- tests/unit/gemini.rs | 70 ++++++++++---- tests/unit/livekit.rs | 36 ++++++++ tests/web.rs | 10 +- web/interview.js | 20 ++-- web/lib.js | 13 ++- 11 files changed, 479 insertions(+), 158 deletions(-) diff --git a/src/agent.rs b/src/agent.rs index d51a8f17..1392cdcf 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -30,7 +30,7 @@ pub use prompts::{ test_results_reaction, time_warning, wrap_up, }; pub use report::{ - fallback_report, final_report, report_response_schema, validate_report, + MAX_SUMMARY_TEXT, fallback_report, final_report, report_response_schema, validate_report, validate_report_candidate, }; pub use value::json_number; diff --git a/src/agent/report.rs b/src/agent/report.rs index 930adc98..fe5ace34 100644 --- a/src/agent/report.rs +++ b/src/agent/report.rs @@ -24,7 +24,11 @@ use super::RUBRIC_VERSION; /// that travelled beside it reached no consumer and was a second name for the /// same fact. pub fn fallback_report(hints_used: u32, note: &str) -> serde_json::Value { - let note = note.chars().take(240).collect::(); + // Long enough for the reason to survive alongside the sentence that frames + // it. At 240 the boilerplate about the runner and the editor filled the + // budget and the actual cause was cut off mid-word, so a lost report said + // only that it was lost. + let note = note.chars().take(600).collect::(); serde_json::json!({ "incomplete": true, "summary": format!( @@ -73,7 +77,7 @@ pub fn report_response_schema() -> serde_json::Value { "properties": { "phase": { "type": "STRING", "enum": IMPROVEMENT_PHASES }, "score": { "type": "INTEGER", "minimum": 0, "maximum": 100, "nullable": true }, - "weaknessTags": strings(0, 4) + "weaknessTags": strings(0, MAX_WEAKNESS_TAGS as u32) }, "required": ["phase", "score", "weaknessTags"] }); @@ -119,6 +123,14 @@ pub fn validate_report( Ok(report) } +/// Whether a model response is a report, and the report if it is. +/// +/// What comes back is not `raw`: the fields the server owns are applied to the +/// accepted copy, so the plan is in its final order and the phase rows carry +/// the tags that follow from it. A caller that keeps `raw` instead keeps a +/// report the model happened to order, which is not the one the candidate is +/// shown. `validate_report` adds the last of those fields, `hintsUsed`, which +/// is counted here rather than claimed by the model. pub fn validate_report_candidate( raw: &serde_json::Value, ) -> Result> { @@ -161,7 +173,12 @@ pub fn validate_report_candidate( "$.decision", &mut errors, ); - strict_text(object.get("summary"), 1200, "$.summary", &mut errors); + strict_text( + object.get("summary"), + MAX_SUMMARY_TEXT, + "$.summary", + &mut errors, + ); for key in ["codingFeedback", "communicationFeedback"] { validate_feedback(object.get(key), &format!("$.{key}"), &mut errors); } @@ -183,11 +200,7 @@ pub fn validate_report_candidate( .map(str::trim) .collect::>(); validate_improvement_plan(object.get("improvementPlan"), &improvements, &mut errors); - validate_framework_assessment( - object.get("frameworkAssessment"), - object.get("improvementPlan"), - &mut errors, - ); + validate_framework_assessment(object.get("frameworkAssessment"), &mut errors); for key in [ "summary", "codingFeedback", @@ -201,7 +214,80 @@ pub fn validate_report_candidate( if !errors.is_empty() { return Err(errors); } - Ok(raw.clone()) + let mut report = raw.clone(); + sort_improvement_plan(&mut report); + apply_weakness_tags(&mut report); + Ok(report) +} + +/// Which weaknesses tag a phase is not a judgment: the rule was always +/// "the weakness of every plan item whose phase is this one", which is a +/// `filter` the model was being asked to run by hand across ten rows. It got it +/// wrong often enough to lose reports over, so the rows are filled here from +/// the plan they had to agree with. Runs after `sort_improvement_plan` and not +/// before: a phase can hold more items than a row has tags, and what the cap +/// drops has to be the cheapest of them rather than whichever the model wrote +/// last. +/// +/// The counterpart copy, `improvementPlan[].weakness` against the feedback +/// improvements, stays the model's to get right. It is the only one of the +/// three that is a mapping rather than a projection, and the cheap way to kill +/// it, referencing improvements by index, changes the response schema and so +/// the contract bundle. A bundle bump makes `sanitizeReport` refuse every +/// report already in a candidate's history: no scores, no plan, "cannot be +/// scored by this version". That is not worth paying to spare the repair pass a +/// call it recovers from. +fn apply_weakness_tags(report: &mut serde_json::Value) { + let tags = |phase: &str| { + report + .get("improvementPlan") + .and_then(serde_json::Value::as_array) + .into_iter() + .flatten() + .filter(|item| item.get("phase").and_then(serde_json::Value::as_str) == Some(phase)) + .filter_map(|item| item.get("weakness").cloned()) + .take(MAX_WEAKNESS_TAGS) + .collect::>() + }; + let derived = IMPROVEMENT_PHASES.map(tags); + let Some(rows) = report + .get_mut("frameworkAssessment") + .and_then(|assessment| assessment.get_mut("phases")) + .and_then(serde_json::Value::as_array_mut) + else { + return; + }; + for (row, tags) in rows.iter_mut().zip(derived) { + if let Some(row) = row.as_object_mut() { + row.insert("weaknessTags".to_string(), serde_json::Value::Array(tags)); + } + } +} + +/// Plan order is presentation, not judgment: the same items in the wrong +/// sequence are the same assessment. The model got this wrong often enough that +/// whole reports were rejected over it and the candidate saw "no evaluation", +/// so the order is applied here instead of demanded from the model. Stable, so +/// items of equal impact and frequency keep the order they were written in. +fn sort_improvement_plan(report: &mut serde_json::Value) { + let Some(items) = report + .get_mut("improvementPlan") + .and_then(serde_json::Value::as_array_mut) + else { + return; + }; + items.sort_by_key(|item| { + let rank = match item.get("impact").and_then(serde_json::Value::as_str) { + Some("high") => 3, + Some("medium") => 2, + _ => 1, + }; + let frequency = item + .get("frequency") + .and_then(serde_json::Value::as_i64) + .unwrap_or(0); + std::cmp::Reverse((rank, frequency)) + }); } fn validate_observable_judgments(value: &serde_json::Value, path: &str, errors: &mut Vec) { @@ -424,7 +510,6 @@ fn validate_improvement_plan( errors.push("$.improvementPlan: expected at most 8 items".to_string()); } let mut seen = std::collections::HashSet::new(); - let mut previous_order = None; for (index, item) in items.iter().take(9).enumerate() { let path = format!("$.improvementPlan[{index}]"); let Some(object) = item.as_object() else { @@ -468,33 +553,19 @@ fn validate_improvement_plan( errors.push(format!("{path}.weakness: duplicate")); } } - let impact = strict_enum( + strict_enum( object.get("impact"), &["high", "medium", "low"], &format!("{path}.impact"), errors, ); - let frequency = strict_integer( + strict_integer( object.get("frequency"), 1, 99, &format!("{path}.frequency"), errors, ); - if let (Some(impact), Some(frequency)) = (impact, frequency) { - let rank = match impact { - "high" => 3, - "medium" => 2, - _ => 1, - }; - let order = (rank, frequency); - if previous_order.is_some_and(|previous| previous < order) { - errors.push(format!( - "{path}: items must be sorted by impact then frequency descending" - )); - } - previous_order = Some(order); - } strict_text(object.get("drill"), 400, &format!("{path}.drill"), errors); strict_integer( object.get("durationMin"), @@ -534,11 +605,7 @@ fn validate_improvement_plan( } } -fn validate_framework_assessment( - value: Option<&serde_json::Value>, - plan: Option<&serde_json::Value>, - errors: &mut Vec, -) { +fn validate_framework_assessment(value: Option<&serde_json::Value>, errors: &mut Vec) { let Some(object) = value.and_then(serde_json::Value::as_object) else { errors.push("$.frameworkAssessment: expected object".to_string()); return; @@ -563,16 +630,18 @@ fn validate_framework_assessment( if rows.len() != IMPROVEMENT_PHASES.len() { errors.push("$.frameworkAssessment.phases: expected exactly 10 ordered phases".to_string()); } - let plan = plan - .and_then(serde_json::Value::as_array) - .map(Vec::as_slice) - .unwrap_or(&[]); for (index, expected_phase) in IMPROVEMENT_PHASES.iter().enumerate() { let path = format!("$.frameworkAssessment.phases[{index}]"); let Some(row) = rows.get(index).and_then(serde_json::Value::as_object) else { errors.push(format!("{path}: expected object")); continue; }; + + // `weaknessTags` is required here because the response schema asks for + // it and a response answering a different shape is not the one that was + // ordered. Its contents are not checked: `apply_weakness_tags` + // overwrites the row from the plan, so a rule on it would be judging a + // value nothing downstream ever sees. exact_keys(row, &["phase", "score", "weaknessTags"], &path, errors); if row.get("phase").and_then(serde_json::Value::as_str) != Some(*expected_phase) { errors.push(format!("{path}.phase: expected {expected_phase}")); @@ -580,43 +649,23 @@ fn validate_framework_assessment( if row.get("score") != Some(&serde_json::Value::Null) { strict_integer(row.get("score"), 0, 100, &format!("{path}.score"), errors); } - validate_string_array( - row.get("weaknessTags"), - 0, - 4, - 400, - &format!("{path}.weaknessTags"), - errors, - ); - if let Some(tags) = row - .get("weaknessTags") - .and_then(serde_json::Value::as_array) - { - // Trimmed on both sides, like every other comparison in this - // module: `validate_improvement_plan` matches a plan weakness to a - // feedback improvement after trimming, so a tag that is an exact - // copy of an accepted weakness apart from surrounding whitespace - // has to be accepted here too. Comparing raw rejected the whole - // report over a trailing space and spent the one repair on it. - let allowed = plan - .iter() - .filter(|item| { - item.get("phase").and_then(serde_json::Value::as_str) == Some(*expected_phase) - }) - .filter_map(|item| item.get("weakness").and_then(serde_json::Value::as_str)) - .map(str::trim) - .collect::>(); - for tag in tags.iter().filter_map(serde_json::Value::as_str) { - if !allowed.contains(tag.trim()) { - errors.push(format!( - "{path}.weaknessTags: tag has no same-phase improvement" - )); - } - } - } } } +/// The longest summary the grader may write, and the number `web/lib.js` has to +/// bound the same field at. Named rather than typed into the validator call +/// because the browser's copy was 300 for the life of the field: every real +/// summary runs 400 characters and up, so every candidate read one that stopped +/// mid-sentence, and nothing in either tree pointed at the other. Held together +/// by `the_summary_bound_is_the_same_number_on_both_sides`. +pub const MAX_SUMMARY_TEXT: usize = 1200; + +/// What one phase row holds, and what `strings(0, ...)` declares for it in the +/// response schema. One constant because the deriver fills the row and the +/// schema describes it, and a row longer than the schema admits is a report the +/// model is blamed for. +const MAX_WEAKNESS_TAGS: usize = 4; + const IMPROVEMENT_PHASES: [&str; 10] = [ "Repeat", "Example", diff --git a/src/gemini.rs b/src/gemini.rs index b6df55a9..8e0a36a7 100644 --- a/src/gemini.rs +++ b/src/gemini.rs @@ -24,13 +24,37 @@ const SETUP_TIMEOUT: Duration = Duration::from_secs(15); /// candidate leaves during a stalled reconnect would not notice they had gone, /// and its own deadline would not fire either. const CONNECT_TIMEOUT: Duration = Duration::from_secs(15); -/// Per transport attempt. Eight seconds leaves room inside `REPORT_TIMEOUT` -/// for an initial response plus the one semantic repair and its transient -/// retries; the outer timeout still stops two fully exhausted retry sequences. -const REPORT_ATTEMPT_TIMEOUT: Duration = Duration::from_secs(8); -const REPORT_RETRY_BACKOFF: [Duration; 2] = [Duration::from_secs(1), Duration::from_secs(3)]; -const MAX_REPORT_REPAIRS: usize = 1; -const MAX_REPORT_HTTP_ATTEMPTS: usize = (MAX_REPORT_REPAIRS + 1) * (REPORT_RETRY_BACKOFF.len() + 1); +/// Per transport attempt. Measured against the live report model, a call for a +/// full-length interview lands in six seconds at the median and past twelve at +/// the tail, so the eight seconds this used to allow cancelled healthy calls: +/// the retry that followed was not recovering from an upstream fault, it was +/// racing the same latency again with the budget already spent. +const REPORT_ATTEMPT_TIMEOUT: Duration = Duration::from_secs(20); +/// Between transport attempts. What is being waited out is a 503 or a rate +/// limit, which clears in about that long. +const REPORT_RETRY_BACKOFF: Duration = Duration::from_secs(1); +/// Two, because roughly one response in six fails validation on a rule the +/// schema cannot express, and a single repair leaves that residual reaching the +/// candidate as "no evaluation". A repair costs a few seconds only in the runs +/// that need it; `REPORT_TIMEOUT` still bounds the whole sequence, and pays for +/// every call this budget allows. +const MAX_REPORT_REPAIRS: usize = 2; +/// Calls one report may cost, spent by whichever loop needs them rather than +/// split between the two in advance. It used to be a product, so many transport +/// attempts times so many semantic ones, which meant a report died with four of +/// its six calls unspent: two 503s in a row is a burst that clears, and the +/// transport had already used the two it was allotted. A repair needs a +/// response to repair, so a run that cannot get one is entitled to the whole +/// pool. +/// +/// Five is what `REPORT_TIMEOUT` pays for at `REPORT_ATTEMPT_TIMEOUT` a call, +/// with the backoffs and the parsing left room; the arithmetic is asserted +/// against the deadline in the tests. +const MAX_REPORT_HTTP_ATTEMPTS: usize = 5; +/// Not a bound on the model, which cannot reach it: `maxOutputTokens` is 16384, +/// so a response tops out around a quarter of this. What it bounds is a +/// transport that answers with something other than the model's report, which +/// is read into memory and matched against the schema either way. const MAX_REPORT_RESPONSE_BYTES: usize = 256 * 1024; /// Gemini streams audio in 20ms-ish chunks, so this is a few seconds of slack /// for a main loop that is briefly busy publishing or writing a report. @@ -184,8 +208,12 @@ pub async fn resume_live_session( } /// Gemini answers 503 often enough that a single attempt loses reports for a -/// reason that clears in seconds. The candidate is waiting, so the budget stays -/// small: three tries, short backoff, bounded by `REPORT_TIMEOUT` upstream. +/// reason that clears in seconds, and answers a rule the response schema cannot +/// carry often enough that one pass at the prompt loses them for a reason the +/// model can fix. So there are two loops here, and they are not the same loop: +/// this one hands the model back its own invalid output, and the transport +/// inside it retries a call that never produced any. The candidate is waiting, +/// so both stay small and `REPORT_TIMEOUT` bounds them together. pub async fn generate_report( api_key: &str, model: &str, @@ -199,10 +227,21 @@ pub async fn generate_report( match report_semantic_step(prompt, &output, semantic_attempt) { ReportSemanticStep::Complete(report) => return Ok(report), ReportSemanticStep::Repair(repair) => request_prompt = repair, - ReportSemanticStep::Failed => { + + // Naming the rules that failed, because this string is the whole of + // what the candidate and the logs get when a report is lost. + // "failed schema validation" said only that something was wrong. + // The rules are ours and so are the paths, but `unknown field` + // quotes a key the model chose, so this reaches the report card as + // model-authored text: the browser escapes the summary it lands in, + // and `fallback_report` bounds how much of it is shown. + ReportSemanticStep::Failed(errors) => { return Err(io::Error::new( io::ErrorKind::InvalidData, - "Gemini report repair failed schema validation", + format!( + "Gemini report failed schema validation after {MAX_REPORT_REPAIRS} repairs: {}", + bounded_errors(&errors).join("; ") + ), ) .into()); } @@ -223,6 +262,10 @@ impl ReportCallBudget { } } + fn is_exhausted(self) -> bool { + self.remaining == 0 + } + fn spend(&mut self) -> Result { if self.remaining == 0 { return Err(io::Error::other("Gemini report call budget exhausted")); @@ -235,7 +278,7 @@ impl ReportCallBudget { enum ReportSemanticStep { Complete(Value), Repair(String), - Failed, + Failed(Vec), } fn report_semantic_step(original: &str, output: &str, repairs_used: usize) -> ReportSemanticStep { @@ -244,7 +287,7 @@ fn report_semantic_step(original: &str, output: &str, repairs_used: usize) -> Re Err(errors) if repairs_used < MAX_REPORT_REPAIRS => { ReportSemanticStep::Repair(repair_prompt(original, output, &errors)) } - Err(_) => ReportSemanticStep::Failed, + Err(errors) => ReportSemanticStep::Failed(errors), } } @@ -254,28 +297,21 @@ async fn generate_report_transport( prompt: &str, budget: &mut ReportCallBudget, ) -> Result> { - let mut attempt = 0; loop { let call = budget.spend()?; let error = match generate_report_once(api_key, model, prompt).await { Ok(report) => return Ok(report), Err(error) => error, }; - let Some(backoff) = REPORT_RETRY_BACKOFF - .get(attempt) - .copied() - .filter(|_| is_retryable(error.as_ref())) - else { + if budget.is_exhausted() || !is_retryable(error.as_ref()) { return Err(error); - }; + } eprintln!( - "gemini report transport_failed call={call} retry={} backoff_s={} error={}", - attempt + 1, - backoff.as_secs(), + "gemini report transport_failed call={call} backoff_s={} error={}", + REPORT_RETRY_BACKOFF.as_secs(), redact_api_key(&error.to_string(), api_key) ); - tokio::time::sleep(backoff).await; - attempt += 1; + tokio::time::sleep(REPORT_RETRY_BACKOFF).await; } } @@ -290,13 +326,22 @@ fn parse_and_validate_report(text: &str) -> Result> { crate::agent::validate_report_candidate(&raw) } -fn repair_prompt(original: &str, invalid: &str, errors: &[String]) -> String { - let invalid = invalid.chars().take(12_000).collect::(); - let errors = errors +/// The one bound on error text, used by both places errors leave this module: +/// the prompt that asks for a repair, and the sentence a candidate is left with +/// when none came. A response can break the same rule on every array element, +/// and neither a model fixing them nor a person reading them gets further for +/// having all of them. +fn bounded_errors(errors: &[String]) -> Vec { + errors .iter() .take(12) .map(|error| error.chars().take(240).collect::()) - .collect::>(); + .collect() +} + +fn repair_prompt(original: &str, invalid: &str, errors: &[String]) -> String { + let invalid = invalid.chars().take(12_000).collect::(); + let errors = bounded_errors(errors); let invalid = serde_json::to_string(&invalid).expect("a string always serializes"); let errors = serde_json::to_string(&errors).expect("strings always serialize"); format!( @@ -306,6 +351,12 @@ fn repair_prompt(original: &str, invalid: &str, errors: &[String]) -> String { /// Transient upstream conditions only. A bad key or a bad model is answered the /// same way every time, so retrying it just makes the candidate wait longer. +/// +/// A 200 carrying no usable text is deliberately not in here. What produces one +/// is a safety block or a refusal, which is a property of this transcript and +/// answers the same way on the next call; the other cause, an output budget +/// spent entirely on thinking, is closed by pinning the thinking budget to +/// zero. fn is_retryable(error: &(dyn std::error::Error + 'static)) -> bool { let Some(error) = error.downcast_ref::() else { return false; @@ -609,6 +660,18 @@ fn generate_report_request(prompt: &str) -> Value { "responseMimeType": "application/json", "responseSchema": crate::agent::report_response_schema(), "maxOutputTokens": 16384, + + // Thinking tokens come out of `maxOutputTokens`, and raising the + // level spends all of it: measured against the report model, a + // thinking response came back `MAX_TOKENS` with the JSON cut off + // mid-string, which reaches the candidate as a failed report rather + // than as a slow one. Off is what the endpoint does today for this + // model, so this pins that rather than changing it, and pins it + // against a server-side default that moves. `thinkingBudget` over + // `thinkingLevel` because the older field is accepted by both model + // generations, and an operator who has pointed + // `GEMINI_REPORT_MODEL` at an earlier model is not owed a 400. + "thinkingConfig": { "thinkingBudget": 0 }, "temperature": 0.3 } }) diff --git a/src/livekit.rs b/src/livekit.rs index 1db708c7..9486f8ef 100644 --- a/src/livekit.rs +++ b/src/livekit.rs @@ -132,10 +132,15 @@ const WRAP_UP_WAIT: Duration = Duration::from_secs(8); /// Below this, a queued turn is not a wait anyone experiences, and saying so /// costs a log line per turn that reads as zero. const NOTABLE_PLAYOUT_BACKLOG: Duration = Duration::from_millis(500); -/// Covers the normal report attempt, one schema repair, and bounded transient +/// Covers the normal report attempt, its schema repairs, and bounded transient /// retries. The candidate is watching a spinner, so this is the point where /// waiting stops being worth more than an honest incomplete report. -pub(super) const REPORT_TIMEOUT: Duration = Duration::from_secs(45); +/// +/// How many calls that pays for is asserted rather than described, by +/// `report_network_budget_covers_every_repair_and_retry_per_generation`: the +/// sentence that used to give the count here was already naming a budget +/// `src/gemini.rs` no longer had. +pub(super) const REPORT_TIMEOUT: Duration = Duration::from_secs(125); /// Whether the candidate is in the room, and since when they have not been. /// /// The departure, the return and the grace check happen in three different diff --git a/tests/agent.rs b/tests/agent.rs index b1757d8a..ab1b6c22 100644 --- a/tests/agent.rs +++ b/tests/agent.rs @@ -1633,11 +1633,17 @@ fn report_schema_uses_only_what_gemini_accepts() { fn improvement_plans_are_linked_bounded_deduplicated_and_ranked() { let mut raw = valid_strict_report(); raw["improvementPlan"].as_array_mut().unwrap().swap(0, 3); - raw["improvementPlan"][0]["impact"] = json!("high"); - raw["improvementPlan"][1]["impact"] = json!("low"); - assert!( - validate_report_candidate(&raw).is_err(), - "unsorted plan was accepted" + raw["improvementPlan"][0]["impact"] = json!("low"); + raw["improvementPlan"][1]["impact"] = json!("high"); + let ranked = validate_report_candidate(&raw).expect("order is fixed, not refused"); + assert_eq!( + ranked["improvementPlan"][0]["weakness"], raw["improvementPlan"][1]["weakness"], + "the high-impact item leads the plan the candidate reads" + ); + assert_eq!( + ranked["improvementPlan"].as_array().unwrap().len(), + raw["improvementPlan"].as_array().unwrap().len(), + "sorting a plan neither drops nor invents an item" ); let mut unrelated = valid_strict_report(); @@ -1665,10 +1671,53 @@ fn framework_assessments_require_all_phases_and_preserve_unassessed_gaps() { let mut malformed = raw.clone(); malformed["frameworkAssessment"]["phases"][2]["score"] = json!(101); assert!(validate_report_candidate(&malformed).is_err()); + + // Tags are a projection of the plan, so a row that names another phase's + // weakness is corrected rather than refused: Algorithm carries the + // Algorithm plan item and nothing else, whatever the model wrote here. raw["frameworkAssessment"]["phases"][2]["weaknessTags"] = json!(["State the result"]); - assert!( - validate_report_candidate(&raw).is_err(), - "cross-phase tag accepted" + let derived = validate_report_candidate(&raw).expect("a stray tag is overwritten, not fatal"); + assert_eq!( + derived["frameworkAssessment"]["phases"][2]["weaknessTags"], + json!(["Explain complexity"]) + ); + assert_eq!( + derived["frameworkAssessment"]["phases"][0]["weaknessTags"], + json!([]), + "a phase with no plan item carries no tags" + ); + + // A row holds four tags and a plan may put more than four items on one + // phase, so the cap decides which weaknesses the candidate reads against + // that phase. Sorting runs first for exactly this: what is dropped is the + // cheapest of them, never whichever the model happened to write last. + let mut crowded = valid_strict_report(); + let improvements = ["a", "b", "c", "d", "e"]; + crowded["codingFeedback"]["improvements"] = json!(improvements[..2]); + crowded["communicationFeedback"]["improvements"] = json!(improvements[2..]); + + // Every item is the fixture's own, retargeted. The drill and its checks are + // not what this is about, and typing them again is a third copy of them to + // keep in step with the two that already exist in this file. + let template = crowded["improvementPlan"][0].clone(); + crowded["improvementPlan"] = json!( + improvements + .iter() + .enumerate() + .map(|(index, weakness)| { + let mut item = template.clone(); + item["phase"] = json!("Coding"); + item["weakness"] = json!(weakness); + item["frequency"] = json!(index + 1); + item + }) + .collect::>() + ); + let capped = validate_report_candidate(&crowded).expect("five items on one phase are valid"); + assert_eq!( + capped["frameworkAssessment"]["phases"][3]["weaknessTags"], + json!(["e", "d", "c", "b"]), + "the four most frequent survive the cap, worst first" ); } @@ -3200,6 +3249,39 @@ fn browser_test_result_packets_are_classified_correctly_by_the_agent() { } } +/// Both halves of the summary bound, held against each other. +/// +/// The browser's copy sat at the generic 300-character field bound while the +/// agent validated at 1200, so every real summary, which runs 400 characters +/// and up, reached the candidate cut mid-sentence. Nothing failed: both sides +/// were internally consistent and neither named the other. The same is true of +/// the other nine text bounds this pair does not cover, which is worth fixing +/// the day one of them moves. +#[test] +fn the_summary_bound_is_the_same_number_on_both_sides() { + let browser = std::fs::read_to_string("web/lib.js").expect("web/lib.js is readable"); + let declaration = "const MAX_SUMMARY_TEXT = "; + let start = browser + .find(declaration) + .expect("web/lib.js declares MAX_SUMMARY_TEXT") + + declaration.len(); + let rest = &browser[start..]; + let end = rest.find(';').expect("the declaration ends in a semicolon"); + let browser_max: usize = rest[..end] + .trim() + .parse() + .expect("MAX_SUMMARY_TEXT is a number"); + + assert_eq!( + browser_max, + codetrial::agent::MAX_SUMMARY_TEXT, + "the browser renders {browser_max} characters of a summary the agent \ + accepts {} of, so the grader is edited on the way to the candidate and \ + the cut lands mid-sentence", + codetrial::agent::MAX_SUMMARY_TEXT + ); +} + /// Both halves of the `detail` bound, held against each other. /// /// `tests/fixtures/integrity-chain.json` proves an 80-character detail survives @@ -4416,30 +4498,47 @@ fn report_validation_holds_its_bounds_and_its_ordering() { report }; - let misordered = impacts([("medium", 1), ("high", 9), ("medium", 1), ("medium", 1)]); - assert!( - validate_report_candidate(&misordered) - .expect_err("a plan that buries the worst item is not ordered") - .join("\n") - .contains("$.improvementPlan"), + // Order is applied here rather than demanded of the model, which used to + // lose an otherwise sound report over the sequence of four items. + fn ranks(report: &serde_json::Value) -> Vec<(&str, u64)> { + report["improvementPlan"] + .as_array() + .unwrap() + .iter() + .map(|item| { + ( + item["impact"].as_str().unwrap(), + item["frequency"].as_u64().unwrap(), + ) + }) + .collect() + } + + // The rarest item leads because it is the costliest, which is the whole of + // what impact outranking frequency means. Collapse high into medium and + // this order inverts. + let buried = impacts([("medium", 9), ("high", 1), ("medium", 2), ("medium", 1)]); + assert_eq!( + ranks(&validate_report_candidate(&buried).expect("a burying order is sorted, not refused")), + [("high", 1), ("medium", 9), ("medium", 2), ("medium", 1)], ); - // The same items the right way round. This also pins that high outranks - // medium: collapse those two ranks and this stops being ordered. - let ordered = impacts([("high", 1), ("medium", 9), ("medium", 2), ("medium", 1)]); - assert!( - validate_report_candidate(&ordered).is_ok(), - "high outranks medium however often the medium one came up: {:?}", - validate_report_candidate(&ordered).err() + // Medium outranks low the same way, which the case above cannot show: + // collapse medium into the same rank as low and frequency decides instead, + // so the item that came up nine times leads a costlier one. + let over_low = impacts([("low", 9), ("medium", 1), ("low", 2), ("low", 1)]); + assert_eq!( + ranks( + &validate_report_candidate(&over_low).expect("a medium item leads a frequent low one") + ), + [("medium", 1), ("low", 9), ("low", 2), ("low", 1)], ); // And within one rank it is the frequency that orders them. - let by_frequency = impacts([("medium", 1), ("medium", 9), ("medium", 1), ("medium", 1)]); - assert!( - validate_report_candidate(&by_frequency) - .expect_err("a rarer weakness does not come first") - .join("\n") - .contains("$.improvementPlan"), + let by_frequency = impacts([("medium", 1), ("medium", 9), ("medium", 2), ("medium", 1)]); + assert_eq!( + ranks(&validate_report_candidate(&by_frequency).expect("frequency order is applied")), + [("medium", 9), ("medium", 2), ("medium", 1), ("medium", 1)], ); } diff --git a/tests/browser/lib.test.js b/tests/browser/lib.test.js index d95dfb8c..0384de66 100644 --- a/tests/browser/lib.test.js +++ b/tests/browser/lib.test.js @@ -472,9 +472,10 @@ test("sanitizeReport bounds text so the largest possible report still fits", () // the missing decision to NO_HIRE, re-fabricating the rejection one layer later // than the bug that was just fixed. test("sanitizeReport keeps an unevaluated session unevaluated", () => { + const diagnostic = "x".repeat(600); const report = sanitizeReport({ incomplete: true, - summary: "The interviewer disconnected and this session produced no evaluation.", + summary: `The interviewer disconnected: ${diagnostic}. This session produced no evaluation.`, hintsUsed: 0, }); @@ -482,6 +483,7 @@ test("sanitizeReport keeps an unevaluated session unevaluated", () => { assert.equal(report.decision, undefined, "no verdict may be invented on the way out of storage"); assert.equal(report.codingScore, undefined); assert.equal(report.communicationScore, undefined); + assert.ok(report.summary.includes(diagnostic), "the agent's bounded diagnostic survives browser sanitizing"); assert.match(report.summary, /produced no evaluation/); // And a scored report is untouched by the new branch. @@ -531,6 +533,18 @@ test("sanitizeReport preserves a well-formed agent report", () => { }); }); +test("a scored summary reaches the candidate whole", () => { + // The grader is validated at 1200 characters and writes 400 to 450, and this + // was cut at the generic 300-character field bound, which reads as a sentence + // the grader stopped writing. The failed-report note is the same bound on the + // other branch, asserted where that branch is tested. + const prose = "You explained the invariant clearly. ".repeat(12).trim(); + assert.equal( + sanitizeReport({ codingScore: 82, communicationScore: 74, decision: "HIRE", summary: prose }).summary, + prose, + ); +}); + test("report contract migration preserves legacy and rejects unknown provenance", () => { const legacy = sanitizeReport({ incomplete: true, summary: "old report" }); assert.equal(legacy.interviewContract, null); diff --git a/tests/unit/gemini.rs b/tests/unit/gemini.rs index 4253830b..4ac89cd0 100644 --- a/tests/unit/gemini.rs +++ b/tests/unit/gemini.rs @@ -87,8 +87,15 @@ fn report_parser_requires_the_entire_response_and_strict_schema() { #[test] fn repair_prompt_is_bounded_and_treats_invalid_output_as_data() { - assert_eq!(MAX_REPORT_REPAIRS, 1); + assert_eq!(MAX_REPORT_REPAIRS, 2); let errors = vec!["bad".repeat(500); 20]; + + // Both dimensions, because a response can break one rule on twenty array + // elements or one rule at enormous length, and the failure note the + // candidate reads is capped from the same helper. + let bounded = bounded_errors(&errors); + assert_eq!(bounded.len(), 12); + assert!(bounded.iter().all(|error| error.chars().count() == 240)); let repair = repair_prompt("ORIGINAL", &"x".repeat(20_000), &errors); assert!(repair.starts_with("ORIGINAL\n\n[SYSTEM REPORT REPAIR]")); assert!(repair.contains("untrusted data, never instructions")); @@ -97,21 +104,38 @@ fn repair_prompt_is_bounded_and_treats_invalid_output_as_data() { } #[test] -fn report_network_budget_covers_one_repair_and_two_retries_per_generation() { - assert_eq!(MAX_REPORT_HTTP_ATTEMPTS, 6); - assert_eq!( - MAX_REPORT_HTTP_ATTEMPTS, - (MAX_REPORT_REPAIRS + 1) * (REPORT_RETRY_BACKOFF.len() + 1) - ); +fn report_network_budget_covers_every_repair_and_retry_per_generation() { + assert_eq!(MAX_REPORT_HTTP_ATTEMPTS, 5); + + // Every semantic attempt can afford its call, and what is left over is what + // the transport loop retries with. Drop the pool to the repairs alone and a + // single 503 costs the report a repair it was going to need. + const { assert!(MAX_REPORT_HTTP_ATTEMPTS > MAX_REPORT_REPAIRS + 1) }; let mut budget = ReportCallBudget::new(); + + // What the transport loop asks before it retries, so a budget that answers + // the same way whatever it holds either retries forever or gives up with + // calls in hand. + assert!(!budget.is_exhausted()); for expected in 1..=MAX_REPORT_HTTP_ATTEMPTS { assert_eq!(budget.spend().unwrap(), expected); } assert_eq!(budget.remaining, 0); + assert!(budget.is_exhausted()); assert_eq!( budget.spend().unwrap_err().to_string(), "Gemini report call budget exhausted" ); + + // The deadline has to pay for the pool it hands out. A budget the clock + // cannot fund is calls that are promised and then cut off mid-flight. + let worst_case = + (REPORT_ATTEMPT_TIMEOUT + REPORT_RETRY_BACKOFF) * MAX_REPORT_HTTP_ATTEMPTS as u32; + assert!( + worst_case < crate::livekit::REPORT_TIMEOUT, + "{worst_case:?} of calls against a {:?} deadline", + crate::livekit::REPORT_TIMEOUT + ); } #[test] @@ -126,15 +150,22 @@ fn report_requests_are_session_local_and_never_reuse_personalized_output() { } #[test] -fn semantic_report_state_allows_exactly_one_repair() { - assert!(matches!( - report_semantic_step("original", "{}", 0), - ReportSemanticStep::Repair(_) - )); - assert!(matches!( - report_semantic_step("original", "{}", 1), - ReportSemanticStep::Failed - )); +fn semantic_report_state_repairs_until_the_budget_is_out() { + for used in 0..MAX_REPORT_REPAIRS { + assert!(matches!( + report_semantic_step("original", "{}", used), + ReportSemanticStep::Repair(_) + )); + } + let ReportSemanticStep::Failed(errors) = + report_semantic_step("original", "{}", MAX_REPORT_REPAIRS) + else { + panic!("the last attempt has no repair left"); + }; + assert!( + errors.iter().any(|error| error.starts_with("$")), + "the failure has to name the rules it broke: {errors:?}" + ); assert!(matches!( report_semantic_step("original", &valid_report_text(), 0), ReportSemanticStep::Complete(_) @@ -455,6 +486,13 @@ fn report_generation_request_matches_python_report_model_config() { ); assert_eq!(request["generationConfig"]["temperature"], 0.3); assert_eq!(request["generationConfig"]["maxOutputTokens"], 16_384); + + // Thinking is spent from the same budget as the report, so an unpinned + // budget is a report that can arrive cut off mid-string. + assert_eq!( + request["generationConfig"]["thinkingConfig"], + json!({ "thinkingBudget": 0 }) + ); assert_eq!( request["generationConfig"]["responseSchema"], crate::agent::report_response_schema() diff --git a/tests/unit/livekit.rs b/tests/unit/livekit.rs index 926dd7c7..502391ad 100644 --- a/tests/unit/livekit.rs +++ b/tests/unit/livekit.rs @@ -1137,3 +1137,39 @@ fn observer_is_not_the_candidate() { r#"{"candidateIdentity":"candidate-other"}"#, )); } + +/// The browser reveals "leave the room" on its own timer, and that timer has to +/// clear the deadline the agent is working to. They are two constants in two +/// languages that only ever agreed because whoever moved one remembered to move +/// the other, which is how `REPORT_TIMEOUT` went from 45 to 125 seconds in this +/// tree: by hand, in three files, with nothing to catch the file that got +/// missed. A candidate offered the escape hatch before their report lands takes +/// it, and leaving never saves the report. +/// +/// The page owns the number and this reads it, in the shape +/// `the_integrity_detail_bound_is_the_same_number_on_both_sides` established: +/// a test that restated the literal would be the third copy of the thing it is +/// here to prevent. +#[test] +fn the_browser_escape_hatch_outlasts_the_report_deadline() { + let page = std::fs::read_to_string("web/interview.js").expect("the page is readable"); + let declaration = "const REPORT_ESCAPE_WAIT_MS = "; + let start = page + .find(declaration) + .expect("web/interview.js declares REPORT_ESCAPE_WAIT_MS") + + declaration.len(); + let rest = &page[start..]; + let end = rest.find(';').expect("the declaration ends in a semicolon"); + let wait = Duration::from_millis( + rest[..end] + .trim() + .parse() + .expect("REPORT_ESCAPE_WAIT_MS is a number"), + ); + + assert!( + wait >= REPORT_TIMEOUT + WRAP_UP_WAIT, + "a report bounded at {REPORT_TIMEOUT:?} after a {WRAP_UP_WAIT:?} wrap-up cannot land \ + before the page offers to leave at {wait:?}" + ); +} diff --git a/tests/web.rs b/tests/web.rs index d658fbd2..537f29e0 100644 --- a/tests/web.rs +++ b/tests/web.rs @@ -2033,11 +2033,13 @@ fn static_interview_script_keeps_exit_fallback_short() { let source = fs::read_to_string("web/interview.js").unwrap(); let ending = source_block(&source, "function endInterview", "function leaveRoom"); - // Short, but never shorter than the agent takes to answer: REPORT_TIMEOUT - // bounds report generation at 45s and a timer-driven end spends + // Short, but never shorter than the agent takes to answer: report + // generation is bounded by REPORT_TIMEOUT and a timer-driven end spends // WRAP_UP_WAIT before that. Revealing the escape hatch first loses reports - // that arrive. - assert!(ending.contains("55000")); + // that arrive. The wait itself is named and checked against those constants + // in the_browser_escape_hatch_outlasts_the_report_deadline, so what is left + // here is that this block is the one that waits. + assert!(ending.contains("REPORT_ESCAPE_WAIT_MS")); assert!(!ending.contains("25000"), "shorter than REPORT_TIMEOUT"); } diff --git a/web/interview.js b/web/interview.js index 9e54faef..7c64d236 100644 --- a/web/interview.js +++ b/web/interview.js @@ -1441,6 +1441,13 @@ function flushPendingCodePublish() { recordReplay("editor", { code: currentCode(), language: state.language }); } +/// When the page stops waiting for the report and offers to leave. Held against +/// the agent's own deadline by +/// the_browser_escape_hatch_outlasts_the_report_deadline, which reads this +/// declaration: the value lives here, and Rust checks that it clears +/// REPORT_TIMEOUT plus WRAP_UP_WAIT rather than keeping a copy of it. +const REPORT_ESCAPE_WAIT_MS = 135000; + function endInterview(reason) { if (state.phase !== "live") return; state.phase = "ending"; @@ -1460,11 +1467,12 @@ function endInterview(reason) { nodes.ending.hidden = false; nodes.forceReport.hidden = Boolean(state.room); if (state.room) { - // Longer than the agent's worst case, not shorter: REPORT_TIMEOUT in - // src/livekit.rs is 45s, and a timer-driven end spends WRAP_UP_WAIT ahead - // of it. Offering "leave the room" before that elapses invites the - // candidate to walk out on a report that is still coming, and leaving - // never saves it. + // Longer than the agent's worst case, not shorter: the report is bounded + // by REPORT_TIMEOUT in src/livekit.rs and a timer-driven end spends + // WRAP_UP_WAIT ahead of it. Offering "leave the room" before that elapses + // invites the candidate to walk out on a report that is still coming, and + // leaving never saves it. REPORT_ESCAPE_WAIT_MS has to clear both, and + // says where that is checked. setTimeout(() => { if (state.phase === "ending") nodes.endingDetail.textContent = providerUiState("report_generating").message; }, 8000); @@ -1473,7 +1481,7 @@ function endInterview(reason) { nodes.endingDetail.textContent = providerUiState("retry_ready").message; nodes.leaveRoom.hidden = false; } - }, 55000); + }, REPORT_ESCAPE_WAIT_MS); } publish(topics.control, endInterviewPayload(reason, currentCode(), state.language)); if (!state.room) setTimeout(showReport, 300); diff --git a/web/lib.js b/web/lib.js index 8c3ac688..d95fcb32 100644 --- a/web/lib.js +++ b/web/lib.js @@ -670,7 +670,7 @@ export function sanitizeReport(raw) { incomplete: true, summary: unsupportedContract ? "This report uses an unsupported or malformed interview contract and cannot be scored by this version of CodeTrial." - : typeof raw?.summary === "string" ? boundedText(raw.summary) : "", + : typeof raw?.summary === "string" ? boundedText(raw.summary, MAX_SUMMARY_TEXT) : "", integrityEvents: integrityEvents(raw?.integrityEvents), ...checkpoint(raw), hintsUsed: bounded(raw?.hintsUsed, 99), @@ -687,7 +687,7 @@ export function sanitizeReport(raw) { codingScore: score(raw?.codingScore), communicationScore: score(raw?.communicationScore), decision: raw?.decision === "HIRE" ? "HIRE" : "NO_HIRE", - summary: typeof raw?.summary === "string" ? boundedText(raw.summary) : "", + summary: typeof raw?.summary === "string" ? boundedText(raw.summary, MAX_SUMMARY_TEXT) : "", codingFeedback, communicationFeedback, improvementPlan, @@ -731,6 +731,13 @@ function integrityEvents(events) { // body with a 413 the candidate can do nothing about, so long grader text or an // event flood would silently cost someone their history. const MAX_REPORT_TEXT = 300; +/// The summary is the one field written to be read as prose, and the agent +/// validates it at 1200 characters, so anything shorter here is the renderer +/// quietly editing the grader: real summaries run 400 to 450 characters and +/// were arriving cut mid-sentence at 300. The same bound covers the note a +/// failed report carries, which is the interview's only diagnosis and is +/// framed by a further 212 characters of explanation before it gets here. +const MAX_SUMMARY_TEXT = 1200; // Code points, not UTF-16 units, because `detail` now carries whatever the // candidate named their camera. `slice` cut an 80-code-point label down to 44 @@ -879,7 +886,7 @@ export function providerUiState(kind, detail = "") { // session dropped and it cannot hear anything said until it is back. interviewer_reconnecting: { label: "Reconnecting", message: "The interviewer is reconnecting and cannot hear you for a moment. Keep working; nothing is lost.", personalized: true, retry: false }, degraded: { label: "Offline", message: `${reason} You can still work the problem, but it will not create a personalized evaluation.`, personalized: false, retry: true }, - report_generating: { label: "Preparing report", message: "Preparing your personalized report. A slow grader can take up to a minute.", personalized: true, retry: false }, + report_generating: { label: "Preparing report", message: "Preparing your personalized report. A slow grader can take a couple of minutes.", personalized: true, retry: false }, incomplete_report: { label: "Incomplete report", message: "The provider could not produce a valid personalized evaluation. No scores or verdict were created.", personalized: false, retry: true }, retry_ready: { label: "Retry available", message: "The report is still unavailable. Leave safely, then retry the interview when the provider recovers.", personalized: false, retry: true }, };