diff --git a/README.md b/README.md index 65854d65..1ccda359 100644 --- a/README.md +++ b/README.md @@ -336,7 +336,7 @@ The common ones: | Variable | Default | Purpose | |---|---|---| | `CODETRIAL_WEB_ADDR` | `127.0.0.1:3000` | Listen address | -| `CODETRIAL_DURATION_MIN` | `45` | Interview length preselected in the lobby (10–90); see [interview length](docs/interview-length.md) | +| `CODETRIAL_DURATION_MIN` | `45` | Interview length preselected in the lobby, snapped to the nearest enabled button, and granted unsnapped (75 stays 75) by `/api/token` to any request that names none, such as a link that skips the lobby; clamped to 10–90; where recording is on, startup refuses a `CODETRIAL_RECORDING_MAX_MINUTES` below the clamped value, see [interview length](docs/interview-length.md) | | `GEMINI_LIVE_MODEL` | `gemini-3.1-flash-live-preview` | Realtime interviewer model | | `GEMINI_REPORT_MODEL` | `gemini-3.1-flash-lite` | Report model | | `CODETRIAL_MAX_INTERIM_REVIEWS` | `6` | Quiet-pause report-model reviews per interview; `0` disables them and `72` is the maximum | diff --git a/docs/interview-length.md b/docs/interview-length.md index 327e6935..ed2d57dd 100644 --- a/docs/interview-length.md +++ b/docs/interview-length.md @@ -5,9 +5,21 @@ both. One rule matters more than the rest: the length offered and the length enforced come from one function, so they cannot drift apart. The lobby offers 30, 45, and 60 minutes, and the candidate picks one before -starting. `CODETRIAL_DURATION_MIN` is only the length preselected for a -candidate who does not choose; the endpoint accepts 10 to 90 either way, so a -deployment that lowers the default has not lowered the ceiling. +starting. `CODETRIAL_DURATION_MIN` is the length preselected for a candidate who +does not choose; the endpoint accepts 10 to 90 either way, so a deployment that +lowers the default has not lowered the ceiling. + +`/api/session` reports the default as `defaultDurationMin`, clamped into 10 to +90; where recording is on, startup refuses a cap below that clamped length, as +the last paragraph says. The lobby preselects it for Medium and Hard, and +for Easy at most 30. A default that is not one of the three buttons selects the +nearest enabled one, and a length the candidate chose is never replaced by the +default. The Easy cap and the snapping are the lobby's suggestion only. +`/api/token` does not look at the problem: a request that names no length, such +as a link that skips the lobby, is granted `defaultDurationMin` itself, not +snapped, whatever the difficulty. A `duration` in the interview URL that is not +a positive number (`0`, `-5`, `30abc`, `abc` or empty) names no length, so the +interview page sends none and gets the same default. Recording lowers it. `CODETRIAL_RECORDING_MAX_MINUTES` bounds a single recording, and the sweeper fails one still running five minutes past it, so an @@ -24,8 +36,9 @@ a request that skips the lobby is bounded too, and both endpoints read it from one function so the length that is offered and the length that is enforced cannot drift apart. -Startup refuses a cap below `CODETRIAL_DURATION_MIN`, so the length this -deployment preselects is always one it can record; the pairing is a -configuration error rather than a quietly shortened interview. Should a cap ever -sit below every length the row offers, the lobby leaves the row alone rather -than rendering a page with nothing to press. +Where recording is on, startup refuses a cap below `CODETRIAL_DURATION_MIN`, +held to 10 to 90 like the default itself, so the length this deployment +preselects is always one it can record; the pairing is a configuration error +rather than a quietly shortened interview. Should a cap ever sit below every +length the row offers, the lobby leaves the row alone rather than rendering a +page with nothing to press. diff --git a/src/config.rs b/src/config.rs index 819c7e10..7f1bb1fa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -476,7 +476,10 @@ pub fn load_from_pairs( DEFAULT_GEMINI_END_SENSITIVITY, ), room_prefix: optional(&values, "CODETRIAL_ROOM_PREFIX", DEFAULT_ROOM_PREFIX), - default_duration_min: optional_u32(&values, "CODETRIAL_DURATION_MIN", DEFAULT_DURATION_MIN), + default_duration_min: configured_duration( + values.get("CODETRIAL_DURATION_MIN").map(String::as_str), + ) + .0, gemini_candidate_video_enabled: gemini_candidate_video_enabled( values .get("CODETRIAL_GEMINI_CANDIDATE_VIDEO_ENABLED") @@ -638,6 +641,52 @@ pub fn max_concurrent_interviews( } } +/// The length `CODETRIAL_DURATION_MIN` stands for, held to the range the +/// endpoints offer, and what to say when it is not used as written: unreadable, +/// so the built-in length applies, or outside the range, so it is clamped. +/// One function for both, so the number in the message is the one the server +/// runs on and the operator who mistyped is not left guessing. +pub fn configured_duration(value: Option<&str>) -> (u32, Option) { + let value = value.map(str::trim).unwrap_or_default(); + if value.is_empty() { + return (DEFAULT_DURATION_MIN, None); + } + + // A number past `i64` is still a number, only a very large or small one, so + // it is clamped like the rest and only text that is no number falls back. + let digits = value.strip_prefix(['+', '-']).unwrap_or(value); + let number = value.parse::().ok().or_else(|| { + (!digits.is_empty() && digits.bytes().all(|byte| byte.is_ascii_digit())).then_some( + if value.starts_with('-') { + i64::MIN + } else { + i64::MAX + }, + ) + }); + let (reason, used) = match number { + Some(minutes) => { + let used = u32::try_from( + minutes.clamp(i64::from(MIN_DURATION_MIN), i64::from(MAX_DURATION_MIN)), + ) + .unwrap_or(DEFAULT_DURATION_MIN); + if i64::from(used) == minutes { + return (used, None); + } + ( + format!("is outside {MIN_DURATION_MIN} to {MAX_DURATION_MIN}"), + used, + ) + } + None => ( + "is not a usable number of minutes".to_string(), + DEFAULT_DURATION_MIN, + ), + }; + let warning = format!("codetrial: CODETRIAL_DURATION_MIN {value:?} {reason}; read as {used}"); + (used, Some(warning)) +} + fn optional_u32(values: &BTreeMap, key: &str, default: u32) -> u32 { values .get(key) diff --git a/src/launch.rs b/src/launch.rs index f9f646e5..30f8296b 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -11,7 +11,7 @@ mod peer; pub use peer::{PeerServices, peer_services}; -use crate::config::{DEFAULT_DURATION_MIN, DEFAULT_ROOM_PREFIX, DEFAULT_WEB_DIR}; +use crate::config::{DEFAULT_ROOM_PREFIX, DEFAULT_WEB_DIR}; use crate::web::WebServerConfig; /// One installation keeps its config file and its account database together in @@ -62,7 +62,7 @@ pub fn web_server_config( // does any work: a half-configured recording block is a startup failure, // never a surprise at the moment a candidate's interview ends. let recording = - crate::config::load_recording(values, &pool, interview_duration_min(values), production) + crate::config::load_recording(values, &pool, duration_setting(values).0, production) .map_err(|error| error.to_string())?; recording_needs_verified_identity( recording.is_some(), @@ -104,6 +104,7 @@ pub fn web_server_config( allowed_hosts: allowed_hosts(values)?, compiler_explorer_enabled: compiler_explorer_enabled(values), guest_mode_enabled: guest_mode_enabled(values), + default_duration_min: duration_setting(values).0, recording, peer_observer, peer, @@ -567,13 +568,13 @@ pub fn recording_needs_verified_identity(recording: bool, oauth: bool) -> Result Ok(()) } -/// The configured interview length, which is what a recording's maximum -/// duration has to clear. Read here rather than through `load_from_pairs` -/// because `codetrial web` deliberately runs without a full agent config. -fn interview_duration_min(values: &BTreeMap) -> u32 { - nonempty(values, "CODETRIAL_DURATION_MIN") - .and_then(|value| value.parse().ok()) - .unwrap_or(DEFAULT_DURATION_MIN) +/// The configured interview length, held to the range the endpoints offer, +/// which is what a recording's maximum duration has to clear, and what to say +/// when it is not used as written. Read here rather than through +/// `load_from_pairs`, because `codetrial web` deliberately runs without a full +/// agent config. +fn duration_setting(values: &BTreeMap) -> (u32, Option) { + crate::config::configured_duration(values.get("CODETRIAL_DURATION_MIN").map(String::as_str)) } /// `CODETRIAL_ALLOWED_HOSTS`, a comma-separated list of `Host` values. @@ -838,6 +839,10 @@ pub fn prepare( let max_concurrent = read_max_concurrent(values); let production = crate::config::is_production(values); + + if let Some(warning) = duration_setting(values).1 { + eprintln!("{warning}"); + } let pool = web_provider_pool(values, options, production)?; let config = web_server_config(values, pool, production, options)?; diff --git a/src/web/auth.rs b/src/web/auth.rs index 41b186f4..a4218a07 100644 --- a/src/web/auth.rs +++ b/src/web/auth.rs @@ -551,8 +551,10 @@ pub(crate) async fn session_handler( // The lobby offers lengths this deployment may not be able to record, and // only the server knows the recording cap. Answered on both branches // because the duration row is live before anyone signs in. - let max_duration_min = - super::token::duration_ceiling(state.config.recording.as_ref().map(|it| it.max_minutes)); + let recording_max_minutes = state.config.recording.as_ref().map(|it| it.max_minutes); + let max_duration_min = super::token::duration_ceiling(recording_max_minutes); + let default_duration_min = + super::token::default_duration(state.config.default_duration_min, recording_max_minutes); // Null on the web half of a split deployment, where the lobby keeps its // billing notice hidden rather than describe an agent it cannot see. @@ -578,6 +580,7 @@ pub(crate) async fn session_handler( "guestModeAvailable": state.guest_mode_available(), "deviceLogin": device_client_id(&state.config).is_some(), "maxDurationMin": max_duration_min, + "defaultDurationMin": default_duration_min, "candidateVideo": candidate_video, "user": { "login": user.login, @@ -599,6 +602,7 @@ pub(crate) async fn session_handler( "guestModeAvailable": state.guest_mode_available(), "deviceLogin": device_client_id(&state.config).is_some(), "maxDurationMin": max_duration_min, + "defaultDurationMin": default_duration_min, "candidateVideo": candidate_video, }), ), diff --git a/src/web/mod.rs b/src/web/mod.rs index f0aff159..9c693d8e 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -92,6 +92,9 @@ pub struct WebServerConfig { pub compiler_explorer_enabled: bool, /// Guest interviews require explicit operator opt-in. pub guest_mode_enabled: bool, + /// `CODETRIAL_DURATION_MIN`: the lobby's preselected length, and the one + /// a token request that names none gets. + pub default_duration_min: u32, /// Number of trusted reverse proxies in front of this server. Zero, the /// default, keys rate limiting on the TCP peer. Anything higher reads the /// client from `X-Forwarded-For`, which is only safe once every request is @@ -180,6 +183,7 @@ impl fmt::Debug for WebServerConfig { production, compiler_explorer_enabled, guest_mode_enabled, + default_duration_min, trusted_proxy_hops, allowed_hosts, recording, @@ -212,6 +216,7 @@ impl fmt::Debug for WebServerConfig { .field("production", production) .field("compiler_explorer_enabled", compiler_explorer_enabled) .field("guest_mode_enabled", guest_mode_enabled) + .field("default_duration_min", default_duration_min) .field("trusted_proxy_hops", trusted_proxy_hops) .field("allowed_hosts", allowed_hosts) .field("recording", recording) diff --git a/src/web/token.rs b/src/web/token.rs index 080836e2..f7e2e9c5 100644 --- a/src/web/token.rs +++ b/src/web/token.rs @@ -14,7 +14,7 @@ use axum::response::{IntoResponse, Response}; use ring::rand::SecureRandom; use serde_json::{Value, json}; -use crate::config::{DEFAULT_DURATION_MIN, MAX_DURATION_MIN, MIN_DURATION_MIN}; +use crate::config::{MAX_DURATION_MIN, MIN_DURATION_MIN}; use crate::current_epoch_seconds; use crate::token::{LivekitTokenInput, TOKEN_TTL_SECONDS, livekit_observer_token, livekit_token}; @@ -39,6 +39,8 @@ pub struct TokenConfig<'a> { /// recording/sweeper.rs reaps one still running past this many minutes, so /// a longer interview loses exactly the stretch it was recorded for. pub recording_max_min: Option, + /// `CODETRIAL_DURATION_MIN`: the length of a request that names none. + pub default_duration_min: u32, } /// Same reason, and the sharpest case of it: `token` is a minted LiveKit @@ -300,7 +302,11 @@ pub fn token_response( .get("problemId") .and_then(Value::as_str) .unwrap_or(crate::agent::DEFAULT_PROBLEM_ID); - let duration_min = token_duration_min(request.get("durationMin"), config.recording_max_min); + let duration_min = token_duration_min( + request.get("durationMin"), + config.default_duration_min, + config.recording_max_min, + ); let interview_loop = crate::agent::InterviewLoop::parse(request.get("interviewLoop").and_then(Value::as_str)); let interview_mode = @@ -422,14 +428,28 @@ pub(crate) fn duration_ceiling(recording_max_min: Option) -> u32 { }) } -pub(crate) fn token_duration_min(value: Option<&Value>, recording_max_min: Option) -> Value { +/// The operator's `CODETRIAL_DURATION_MIN` as the lobby and `/api/token` both +/// use it: inside the range the endpoint offers and no longer than the +/// deployment can record. One function for the same reason as +/// [`duration_ceiling`]. +pub(crate) fn default_duration(configured_min: u32, recording_max_min: Option) -> u32 { + configured_min + .clamp(MIN_DURATION_MIN, MAX_DURATION_MIN) + .min(duration_ceiling(recording_max_min)) +} + +pub(crate) fn token_duration_min( + value: Option<&Value>, + default_duration_min: u32, + recording_max_min: Option, +) -> Value { let ceiling = duration_ceiling(recording_max_min); let duration = value.and_then(crate::agent::json_number).unwrap_or(0.0); if duration == 0.0 || duration.is_nan() { // The default is a request like any other where the cap is shorter than // it. Handing back a length this server cannot record, for a caller who // named no length at all, is the same lost stretch of interview. - json!(DEFAULT_DURATION_MIN.min(ceiling)) + json!(default_duration(default_duration_min, recording_max_min)) } else { // Fractional minutes stay fractional: the metadata is echoed back to // the frontend, which renders whatever the lobby asked for. @@ -717,6 +737,7 @@ fn minted_token( api_secret: &provider.api_secret, server_url: &provider.url, recording_max_min: state.config.recording.as_ref().map(|it| it.max_minutes), + default_duration_min: state.config.default_duration_min, }, body, room_name, diff --git a/tests/browser/account.test.js b/tests/browser/account.test.js index 39a257e3..bd20e99b 100644 --- a/tests/browser/account.test.js +++ b/tests/browser/account.test.js @@ -225,7 +225,7 @@ test("the lobby offers an editor or a whiteboard and carries the choice into the ); assertIncludesCompact( interview, - "durationMin, interviewId, interviewLoop, interviewMode: mode, ...(!whiteboard ? { language: state.language } : {}), interviewProfile, ...(interviewGrounding", + "...(namedDuration ? { durationMin } : {}), interviewId, interviewLoop, interviewMode: mode, ...(!whiteboard ? { language: state.language } : {}), interviewProfile, ...(interviewGrounding", ); assertIncludesCompact( interview, diff --git a/tests/browser/interviewer-voice.test.js b/tests/browser/interviewer-voice.test.js index eac3ec58..deaf21f6 100644 --- a/tests/browser/interviewer-voice.test.js +++ b/tests/browser/interviewer-voice.test.js @@ -170,3 +170,202 @@ test("the interviewer controls fit narrow and desktop preflights", async (t) => await page.close(); } }); + +// A URL that skips the lobby names no length, so the server's configured +// default decides; sending the provisional 45 would override it. +for (const [query, sent] of [ + ["", false], + // Neither names a usable length, so neither may override the server default. + ["&duration=", false], + ["&duration=abc", false], + ["&duration=0", false], + ["&duration=-5", false], + // Trailing text is not a number, so it names no length either. + ["&duration=30abc", false], + ["&duration=60", 60], + // The server keeps fractional minutes, and so does the page. + ["&duration=42.8", 42.8], +]) { + test(`the token request ${sent ? "names" : "omits"} the length for ${query || "a URL without duration"}`, async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + await page.route("**/api/token", (route) => + route.fulfill({ + status: 503, + contentType: "application/json", + body: JSON.stringify({ + error: "Token request captured by the test.", + }), + }), + ); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match${query}`, + { waitUntil: "domcontentloaded" }, + ); + await page.locator("#audio-heard").click(); + await page.locator("#camera-skip").click(); + await page.waitForFunction( + () => !document.querySelector("#audio-check-join").disabled, + ); + const request = page.waitForRequest((r) => + r.url().endsWith("/api/token"), + ); + await page.locator("#audio-check-join").click(); + const body = (await request).postDataJSON(); + assert.equal(Object.hasOwn(body, "durationMin"), sent !== false); + if (sent !== false) assert.equal(body.durationMin, sent); + } finally { + await page.close(); + } + }); +} + +// The round plan is drawn from the length, so a URL with no `duration` waits +// for the server's default instead of showing 45 minutes and jumping. +for (const query of ["", "&duration=", "&duration=abc"]) { + test(`the round plan for ?${query || "no duration"} waits for the server default`, async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + let release; + const held = new Promise((resolve) => (release = resolve)); + await page.route("**/api/session", async (route) => { + await held; + await route.fulfill({ + json: { signedIn: false, defaultDurationMin: 30, maxDurationMin: 90 }, + }); + }); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match&loop=coding_only${query}`, + { waitUntil: "domcontentloaded" }, + ); + const plan = page.locator("#round-plan-summary"); + // `renderProblem` writes the title in the same task as `bindEvents`; the + // static `#audio-check` is there before either has run. + await page.waitForFunction( + () => + document.querySelector("#problem-title").textContent !== + "Loading interview...", + ); + assert.equal(await plan.textContent(), ""); + release(); + await page.waitForFunction(() => + /30 minute coding budget/.test( + document.querySelector("#round-plan-summary").textContent, + ), + ); + } finally { + await page.close(); + } + }); +} + +// With no answer from the server either, the plan falls back to the 45 minutes +// a URL without a length shows. `-5` once read as 10 here, because the length +// shown was parsed apart from the decision that it named none. +for (const query of ["&duration=-5", "&duration=0", "&duration=30abc"]) { + test(`the round plan for ?${query} falls back to 45 when the session fails`, async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + await page.route("**/api/session", (route) => + route.fulfill({ status: 500, body: "nope" }), + ); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match&loop=coding_only${query}`, + { waitUntil: "domcontentloaded" }, + ); + await page.waitForFunction(() => + /45 minute coding budget/.test( + document.querySelector("#round-plan-summary").textContent, + ), + ); + } finally { + await page.close(); + } + }); +} + +// Fractional minutes are kept, so the split must not show the float error of +// subtracting the reserve: 33.3 - 8 is 25.299999999999997 in JavaScript. +test("the round plan rounds a fractional length's coding budget", async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + await page.route("**/api/session", (route) => + route.fulfill({ + json: { signedIn: false, defaultDurationMin: 30, maxDurationMin: 90 }, + }), + ); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match&loop=coding_behavioral&duration=33.3`, + { waitUntil: "domcontentloaded" }, + ); + await page.waitForFunction(() => + /25\.3 minute coding budget/.test( + document.querySelector("#round-plan-summary").textContent, + ), + ); + } finally { + await page.close(); + } +}); + +// The server echoes the length it granted and the page redraws the split from +// it, so that second draw rounds too. +test("the round plan rounds a fractional length the server granted", async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + await page.route("**/api/token", (route) => + route.fulfill({ + json: { + serverUrl: "wss://test.invalid", + token: "t", + durationMin: 33.3, + }, + }), + ); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match&loop=coding_behavioral&duration=20`, + { waitUntil: "domcontentloaded" }, + ); + await page.locator("#audio-heard").click(); + await page.locator("#camera-skip").click(); + await page.waitForFunction( + () => !document.querySelector("#audio-check-join").disabled, + ); + await page.locator("#audio-check-join").click(); + await page.waitForFunction(() => + /25\.3 minute coding budget/.test( + document.querySelector("#round-plan-summary").textContent, + ), + ); + } finally { + await page.close(); + } +}); + +test("the round plan for a URL with a duration is drawn at once", async (t) => { + if (!browser) return t.skip("playwright chromium unavailable"); + const page = await browser.newPage(); + try { + await page.route("**/api/session", (route) => + route.fulfill({ + json: { signedIn: false, defaultDurationMin: 30, maxDurationMin: 90 }, + }), + ); + await page.goto( + `${base}/interview.html?problem=chargeback-pair-match&loop=coding_only&duration=60`, + { waitUntil: "domcontentloaded" }, + ); + await page.waitForFunction(() => + /60 minute coding budget/.test( + document.querySelector("#round-plan-summary").textContent, + ), + ); + } finally { + await page.close(); + } +}); diff --git a/tests/browser/lobby.test.js b/tests/browser/lobby.test.js index 7bf2df76..fbd09c03 100644 --- a/tests/browser/lobby.test.js +++ b/tests/browser/lobby.test.js @@ -3295,6 +3295,218 @@ lobbyTest( }, ); +for (const [configured, medium, easy] of [ + // The operator's own length replaces the built-in forty-five. + [30, "30", "30"], + [60, "60", "30"], + // Not a button: the nearest one is selected rather than none. + [20, "30", "30"], + [75, "60", "30"], +]) { + lobbyTest( + `an operator default of ${configured} minutes preselects ${medium}, or ${easy} for Easy`, + async (page) => { + session.defaultDurationMin = configured; + const state = await lobby(page); + assert.equal(state.duration, medium); + + await setLevel(page, "Easy", true); + await setLevel(page, "Medium", false); + assert.equal((await snapshot(page)).duration, easy); + }, + ); +} + +lobbyTest( + "a restored lobby suggests again after a level was ticked by hand", + async (page) => { + session.defaultDurationMin = 40; + session.maxDurationMin = 40; + await lobby(page); + // Ticking a level by hand skips the history-based suggestion on restore, + // so only the session can move the length now. + await setLevel(page, "Hard", true); + await setLevel(page, "Medium", false); + assert.equal((await snapshot(page)).duration, "30"); + + session.maxDurationMin = 60; + await page.evaluate(() => + window.dispatchEvent( + new PageTransitionEvent("pageshow", { persisted: true }), + ), + ); + await page.waitForFunction( + () => + document.querySelector("[data-duration].selected")?.dataset.duration === + "45", + ); + }, +); + +lobbyTest( + "a restored lobby leaves a length the candidate chose when the ceiling moves", + async (page) => { + session.maxDurationMin = 45; + await lobby(page); + await page.click('[data-duration="30"]'); + + session.maxDurationMin = 90; + await page.evaluate(() => + window.dispatchEvent( + new PageTransitionEvent("pageshow", { persisted: true }), + ), + ); + // The 60 button comes back only once the restored lobby has read the new + // ceiling, so the length below is checked after the session answered. + await page.waitForFunction( + () => !document.querySelector('[data-duration="60"]').disabled, + ); + assert.equal((await snapshot(page)).duration, "30"); + }, +); + +/// The session answers after the candidate has already acted, which is where +/// "left alone" has to hold: the latch is set before the default arrives. +const heldSession = async (page) => { + let release; + holdSession = new Promise((resolve) => (release = resolve)); + const requested = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "domcontentloaded" }); + await requested; + return async () => { + release(); + await page.waitForFunction( + () => document.querySelector("#recommendation").textContent !== "", + ); + }; +}; + +lobbyTest( + "an operator default does not overwrite a length chosen before the session answers", + async (page) => { + session.defaultDurationMin = 30; + const answer = await heldSession(page); + await page.click('[data-duration="60"]'); + await answer(); + assert.equal((await snapshot(page)).duration, "60"); + }, +); + +lobbyTest( + "an operator default does not replace a remembered length", + async (page) => { + session.defaultDurationMin = 60; + await page.addInitScript( + ([key, value]) => localStorage.setItem(key, value), + ["codetrial.interviewSettings", JSON.stringify({ durationMin: 45 })], + ); + assert.equal((await lobby(page)).duration, "45"); + }, +); + +lobbyTest( + "an operator default above the recording cap is moved down to the cap", + async (page) => { + // The server never sends this pair, since it holds the default to the cap + // itself; this is the lobby's own guard. + session.defaultDurationMin = 60; + session.maxDurationMin = 45; + assert.equal((await lobby(page)).duration, "45"); + }, +); + +lobbyTest( + "an operator default does not overrule a problem picked before the session answers", + async (page) => { + session.defaultDurationMin = 60; + const answer = await heldSession(page); + // Programmatic, because the picker is closed and the card is hidden. + await page.evaluate(() => + document.querySelector('[data-problem][data-difficulty="Easy"]').click(), + ); + await answer(); + assert.equal( + (await snapshot(page)).duration, + "30", + "the checkboxes still say Medium, which would suggest the operator's 60", + ); + }, +); + +lobbyTest( + "a start pressed before the session answers leaves the length to the server", + async (page) => { + session.defaultDurationMin = 60; + let release; + holdSession = new Promise((resolve) => (release = resolve)); + const requested = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "domcontentloaded" }); + await requested; + // Programmatic, because the picker is closed and the card is hidden. + await page.evaluate(() => + document + .querySelector('[data-problem][data-difficulty="Medium"]') + .click(), + ); + await page.click("#start"); + release(); + + // The markup's 45 is not the operator's length, so it must not ship. + await page.waitForURL(/\/interview/); + assert.equal(new URL(page.url()).searchParams.has("duration"), false); + }, +); + +lobbyTest( + "a start pressed before the session answers sends a length the lobby changed", + async (page) => { + session.defaultDurationMin = 60; + let release; + holdSession = new Promise((resolve) => (release = resolve)); + const requested = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "domcontentloaded" }); + await requested; + // An Easy problem makes the row say 30, which the candidate saw, so the + // server must grant 30 and not the 60 it would pick for a request that + // names nothing. + await page.evaluate(() => + document.querySelector('[data-problem][data-difficulty="Easy"]').click(), + ); + await page.click("#start"); + release(); + + await page.waitForURL(/\/interview/); + assert.equal(new URL(page.url()).searchParams.get("duration"), "30"); + }, +); + +lobbyTest( + "a start after the session failed sends the length the row shows", + async (page) => { + // The failed read re-arms Start. The lobby no longer waits for an answer, so + // the 45 on screen is what the candidate sees, and it goes in the URL. + session = null; + failing = new Set(["/api/session"]); + await page.goto(`${base}/`, { waitUntil: "domcontentloaded" }); + await awaitReady(page); + await page.evaluate(() => + document + .querySelector('[data-problem][data-difficulty="Medium"]') + .click(), + ); + await page.click("#start"); + + await page.waitForURL(/\/interview/); + assert.equal(new URL(page.url()).searchParams.get("duration"), "45"); + }, +); + lobbyTest( "the recommendation does not move once it is on screen", async (page) => { diff --git a/tests/unit/bin.rs b/tests/unit/bin.rs index 095508d1..ae348079 100644 --- a/tests/unit/bin.rs +++ b/tests/unit/bin.rs @@ -478,6 +478,59 @@ bm90IGEga2V5 assert!(super::recording_can_deliver(Some(&recording)).is_err()); } +fn recording_values(pairs: &[(&str, &str)]) -> super::BTreeMap { + let mut values: std::collections::BTreeMap = [ + ("CODETRIAL_RECORDING_ENABLED", "true"), + ("CODETRIAL_RECORDING_GCS_BUCKET", "codetrial-staging"), + ("CODETRIAL_RECORDING_DRIVE_ID", "0AKfixtureDriveId"), + ( + "CODETRIAL_RECORDING_SERVICE_ACCOUNT_JSON", + r#"{"type":"service_account","client_email":"a@b.iam.gserviceaccount.com","private_key":"KEYMATERIAL-4bd2"}"#, + ), + ( + "CODETRIAL_RECORDING_TEMPLATE_BASE_URL", + "https://recording.codetrial.example", + ), + ("SESSION_SECRET", "a-real-secret"), + ] + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect(); + for (key, value) in pairs { + values.insert(key.to_string(), value.to_string()); + } + values +} + +/// The recording check sees the length the endpoints grant, not the raw text: +/// `120` is served as 90 so a cap of 90 holds it, and a value that cannot be +/// read is served as 45 so a cap below that is refused. +#[test] +fn the_recording_cap_is_checked_against_the_clamped_length() { + let pool = codetrial::config::ProviderPool { + providers: vec![provider(codetrial::config::PRIMARY_PROVIDER_ID)], + }; + let refusal = |duration: &str, cap: &str| { + let values = recording_values(&[ + ("CODETRIAL_DURATION_MIN", duration), + ("CODETRIAL_RECORDING_MAX_MINUTES", cap), + ]); + super::web_server_config(&values, pool.clone(), false, &super::CliOptions::default()).err() + }; + + // Past the cap check the fixture still lacks an OAuth app, so what matters + // is only whether the refusal is about the cap. + let about_the_cap = |error: &Option| { + error + .as_deref() + .is_some_and(|text| text.contains("CODETRIAL_RECORDING_MAX_MINUTES")) + }; + assert!(!about_the_cap(&refusal("120", "90"))); + assert!(refusal("120", "89").unwrap().contains("89, below the 90")); + assert!(refusal("30min", "44").unwrap().contains("44, below the 45")); + assert!(!about_the_cap(&refusal("30min", "45"))); +} + /// Half an OAuth app is not an OAuth app. /// /// The rule below is tested on its own; this is the wiring that feeds it, @@ -488,29 +541,6 @@ bm90IGEga2V5 /// half alone has to be refused, and so does neither. #[test] fn recording_starts_only_with_both_halves_of_the_oauth_app() { - let recording_values = |pairs: &[(&str, &str)]| { - let mut values: std::collections::BTreeMap = [ - ("CODETRIAL_RECORDING_ENABLED", "true"), - ("CODETRIAL_RECORDING_GCS_BUCKET", "codetrial-staging"), - ("CODETRIAL_RECORDING_DRIVE_ID", "0AKfixtureDriveId"), - ( - "CODETRIAL_RECORDING_SERVICE_ACCOUNT_JSON", - r#"{"type":"service_account","client_email":"a@b.iam.gserviceaccount.com","private_key":"KEYMATERIAL-4bd2"}"#, - ), - ( - "CODETRIAL_RECORDING_TEMPLATE_BASE_URL", - "https://recording.codetrial.example", - ), - ("SESSION_SECRET", "a-real-secret"), - ] - .into_iter() - .map(|(key, value)| (key.to_string(), value.to_string())) - .collect(); - for (key, value) in pairs { - values.insert(key.to_string(), value.to_string()); - } - values - }; let pool = codetrial::config::ProviderPool { providers: vec![provider(codetrial::config::PRIMARY_PROVIDER_ID)], }; @@ -660,6 +690,30 @@ fn a_data_dir_decides_every_path_a_launch_reads_or_writes() { ); } +/// The operator's `CODETRIAL_DURATION_MIN` is what the lobby and `/api/token` +/// default to, so it has to reach the web config. +#[test] +fn web_server_config_carries_the_configured_default_length() { + let options = super::CliOptions::default(); + let pool = codetrial::config::ProviderPool { + providers: vec![provider(codetrial::config::PRIMARY_PROVIDER_ID)], + }; + let configured = |values: &super::BTreeMap| { + super::web_server_config(values, pool.clone(), false, &options) + .expect("a plain local config should be accepted") + .default_duration_min + }; + assert_eq!( + configured(&values(&[])), + codetrial::config::DEFAULT_DURATION_MIN + ); + assert_eq!(configured(&values(&[("CODETRIAL_DURATION_MIN", "30")])), 30); + assert_eq!( + configured(&values(&[("CODETRIAL_DURATION_MIN", " 30 ")])), + 30 + ); +} + /// Made once and kept, so a relaunch keeps everyone signed in, and owner-only /// because any program on the machine can reach a loopback port. #[test] diff --git a/tests/unit/dispatch_http.rs b/tests/unit/dispatch_http.rs index f6596e45..fd253114 100644 --- a/tests/unit/dispatch_http.rs +++ b/tests/unit/dispatch_http.rs @@ -239,6 +239,7 @@ impl Probe { production: !pinned, compiler_explorer_enabled: false, guest_mode_enabled: true, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, trusted_proxy_hops: 1, allowed_hosts: Vec::new(), recording: None, diff --git a/tests/unit/launch.rs b/tests/unit/launch.rs index 28009123..5b796a60 100644 --- a/tests/unit/launch.rs +++ b/tests/unit/launch.rs @@ -85,10 +85,13 @@ fn the_launch_reads_its_cap_length_and_compiler_switch_from_the_values() { 5 ); assert_eq!( - interview_duration_min(&values(&[("CODETRIAL_DURATION_MIN", "45")])), + duration_setting(&values(&[("CODETRIAL_DURATION_MIN", "45")])).0, 45 ); - assert_eq!(interview_duration_min(&values(&[])), DEFAULT_DURATION_MIN); + assert_eq!( + duration_setting(&values(&[])).0, + crate::config::DEFAULT_DURATION_MIN + ); for (value, enabled) in [("true", true), ("false", false)] { assert_eq!( compiler_explorer_enabled(&values(&[("CODETRIAL_COMPILER_EXPLORER_ENABLED", value)])), @@ -96,3 +99,62 @@ fn the_launch_reads_its_cap_length_and_compiler_switch_from_the_values() { ); } } + +/// The length a launch runs on and what the operator is told about it come from +/// one function, so the number in a warning is the one the server uses, and the +/// recording check sees the same clamped length the endpoints grant. +#[test] +fn a_duration_setting_is_read_once_and_named_when_not_used_as_written() { + let values = |pairs: &[(&str, &str)]| -> BTreeMap { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + }; + let read = |value: &str| { + let pairs = values(&[("CODETRIAL_DURATION_MIN", value)]); + duration_setting(&pairs) + }; + let unset = values(&[]); + assert_eq!( + duration_setting(&unset), + (crate::config::DEFAULT_DURATION_MIN, None) + ); + for (value, used) in [("", 45), (" ", 45), ("10", 10), (" 60 ", 60), ("90", 90)] { + assert_eq!(read(value), (used, None), "{value:?}"); + } + for (value, used) in [ + ("30min", 45), + ("45.5", 45), + ("abc", 45), + ("-30", 10), + ("0", 10), + ("5", 10), + ("91", 90), + ("120", 90), + ("500", 90), + ("4294967296", 90), + ("99999999999999999999", 90), + ("-99999999999999999999", 10), + ("+99999999999999999999", 90), + ("-", 45), + ("+", 45), + ] { + let (length, said) = read(value); + assert_eq!(length, used, "{value:?}"); + let said = said.unwrap_or_else(|| panic!("{value:?} was accepted silently")); + assert!(said.contains(value), "{said}"); + assert!(said.ends_with(&format!("read as {used}")), "{said}"); + + // The agent path (`check-gemini`, `run-livekit`) reads the same length. + let agent = crate::config::load_from_pairs([ + ("LIVEKIT_URL", "wss://primary.example"), + ("LIVEKIT_API_KEY", "primary-key"), + ("LIVEKIT_API_SECRET", "primary-secret"), + ("GOOGLE_API_KEY", "primary-google"), + ("CODETRIAL_DURATION_MIN", value), + ]) + .unwrap(); + assert_eq!(agent.default_duration_min, used, "{value:?}"); + } +} diff --git a/tests/unit/livekit/recovery_live_parts/launcher_and_scenario_judging.rs b/tests/unit/livekit/recovery_live_parts/launcher_and_scenario_judging.rs index 580d7214..cd0c8187 100644 --- a/tests/unit/livekit/recovery_live_parts/launcher_and_scenario_judging.rs +++ b/tests/unit/livekit/recovery_live_parts/launcher_and_scenario_judging.rs @@ -24,6 +24,7 @@ impl<'a> LiveEffects<'a> { api_secret: &config.livekit_api_secret, server_url: &config.livekit_url, recording_max_min: recording_cap, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, }, body.as_bytes(), &room_name, diff --git a/tests/unit/livekit/recovery_live_parts/report_generation_cardinality.rs b/tests/unit/livekit/recovery_live_parts/report_generation_cardinality.rs index bc797fae..9976ffb9 100644 --- a/tests/unit/livekit/recovery_live_parts/report_generation_cardinality.rs +++ b/tests/unit/livekit/recovery_live_parts/report_generation_cardinality.rs @@ -245,6 +245,7 @@ impl G2NativeRun { api_key: &self.config.livekit_api_key, api_secret: &self.config.livekit_api_secret, server_url: &self.config.livekit_url, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, recording_max_min: None, }, br#"{"problemId":"two-sum","durationMin":10}"#, diff --git a/tests/unit/web/policy.rs b/tests/unit/web/policy.rs index a84640de..28ec7820 100644 --- a/tests/unit/web/policy.rs +++ b/tests/unit/web/policy.rs @@ -25,6 +25,7 @@ fn policy_for(url: &str) -> String { production: true, compiler_explorer_enabled: false, guest_mode_enabled: false, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, diff --git a/tests/unit/web/pool.rs b/tests/unit/web/pool.rs index dd9caa1e..4d0e3c9c 100644 --- a/tests/unit/web/pool.rs +++ b/tests/unit/web/pool.rs @@ -32,6 +32,7 @@ fn config_with(ids: &[&str]) -> WebServerConfig { production: false, compiler_explorer_enabled: false, guest_mode_enabled: false, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, diff --git a/tests/unit/web/setup_edit.rs b/tests/unit/web/setup_edit.rs index 6a8dcea8..91dbe385 100644 --- a/tests/unit/web/setup_edit.rs +++ b/tests/unit/web/setup_edit.rs @@ -217,6 +217,7 @@ impl Server { production: false, compiler_explorer_enabled: false, guest_mode_enabled: true, + default_duration_min: crate::config::DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, diff --git a/tests/web.rs b/tests/web.rs index 58103168..bfbb6583 100644 --- a/tests/web.rs +++ b/tests/web.rs @@ -143,6 +143,7 @@ fn web_config() -> WebServerConfig { production: false, compiler_explorer_enabled: true, guest_mode_enabled: true, + default_duration_min: DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, @@ -1389,6 +1390,9 @@ async fn spawn_counting_quota_stub( #[path = "web/token.rs"] mod token; +#[path = "web/token_default.rs"] +mod token_default; + #[path = "web/policy.rs"] mod policy; diff --git a/tests/web/accounts.rs b/tests/web/accounts.rs index e8f6d234..2e87eef4 100644 --- a/tests/web/accounts.rs +++ b/tests/web/accounts.rs @@ -129,7 +129,7 @@ async fn account_routes_record_interviewee_github_login() { assert_eq!( session.json::().await.unwrap(), json!({"signedIn": false, "loginRequired": true, - "guestModeAvailable": true, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, + "guestModeAvailable": true, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, "defaultDurationMin": DEFAULT_DURATION_MIN, "peerEnabled": false, "peerMaxRoundMinutes": null, "peerObserverAvailable": false, "peerAudioAvailable": false, "candidateVideo": null}) ); @@ -329,7 +329,7 @@ async fn an_unmigratable_account_database_refuses_rather_than_disabling_login() assert_eq!( session, json!({"signedIn": false, "loginRequired": true, - "guestModeAvailable": false, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, + "guestModeAvailable": false, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, "defaultDurationMin": DEFAULT_DURATION_MIN, "peerEnabled": false, "peerMaxRoundMinutes": null, "peerObserverAvailable": false, "peerAudioAvailable": false, "candidateVideo": null}), "a database this binary cannot migrate still requires a login" ); @@ -371,7 +371,7 @@ async fn an_unopenable_account_database_refuses_rather_than_disabling_login() { assert_eq!( session, json!({"signedIn": false, "loginRequired": true, - "guestModeAvailable": false, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, + "guestModeAvailable": false, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, "defaultDurationMin": DEFAULT_DURATION_MIN, "peerEnabled": false, "peerMaxRoundMinutes": null, "peerObserverAvailable": false, "peerAudioAvailable": false, "candidateVideo": null}), "a broken database still requires a login; it cannot serve one" ); @@ -428,7 +428,7 @@ async fn an_unopenable_account_database_refuses_rather_than_disabling_login() { "signedIn": false, "loginRequired": false, "guestModeAvailable": true, "deviceLogin": false, - "maxDurationMin": MAX_DURATION_MIN, + "maxDurationMin": MAX_DURATION_MIN, "defaultDurationMin": DEFAULT_DURATION_MIN, "peerEnabled": false, "peerMaxRoundMinutes": null, "peerObserverAvailable": false, "peerAudioAvailable": false, "candidateVideo": null }), "nothing to sign in to, and the lobby has to be told so" @@ -1084,7 +1084,7 @@ async fn account_reports_are_scoped_to_the_signed_in_user() { assert_eq!( signed_out, json!({"signedIn": false, "loginRequired": true, - "guestModeAvailable": true, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, + "guestModeAvailable": true, "deviceLogin": false, "maxDurationMin": MAX_DURATION_MIN, "defaultDurationMin": DEFAULT_DURATION_MIN, "peerEnabled": false, "peerMaxRoundMinutes": null, "peerObserverAvailable": false, "peerAudioAvailable": false, "candidateVideo": null}), "accounts exist here, so the browser has to know to demand a sign-in" ); diff --git a/tests/web/contract.rs b/tests/web/contract.rs index abbf012c..28f8efea 100644 --- a/tests/web/contract.rs +++ b/tests/web/contract.rs @@ -59,6 +59,7 @@ async fn static_home_markup_matches_frontend_contract() { production: false, compiler_explorer_enabled: true, guest_mode_enabled: false, + default_duration_min: DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, @@ -121,6 +122,7 @@ async fn static_interview_markup_exposes_offline_surface() { production: false, compiler_explorer_enabled: true, guest_mode_enabled: false, + default_duration_min: DEFAULT_DURATION_MIN, trusted_proxy_hops: 0, allowed_hosts: Vec::new(), recording: None, @@ -299,7 +301,7 @@ fn static_interview_script_leaves_candidate_identity_to_the_server() { // The shared request fields follow the mode-specific problem selection. // Strict assignment must retain the optional execution restriction too. assert!(compact(&source).contains( - "durationMin,interviewId,interviewLoop,interviewMode:mode,...(!whiteboard?{language:state.language}:{}),interviewProfile,...(interviewGrounding?{interviewGrounding}:{}),...(nodes.interviewerAccent.value?{interviewerAccent:nodes.interviewerAccent.value}:{}),...(nodes.interviewerVoice.value?{interviewerVoice:nodes.interviewerVoice.value}:{}),...(nodes.hideExamples.checked?{hideExamples:true}:{}),...(exampleDrawingEnabled?{exampleDrawing:true}:{}),...(candidateLedEnabled?{candidateLed:true}:{}),...(!editorOptions.execution?{codeExecution:false}:{})})" + "...(namedDuration?{durationMin}:{}),interviewId,interviewLoop,interviewMode:mode,...(!whiteboard?{language:state.language}:{}),interviewProfile,...(interviewGrounding?{interviewGrounding}:{}),...(nodes.interviewerAccent.value?{interviewerAccent:nodes.interviewerAccent.value}:{}),...(nodes.interviewerVoice.value?{interviewerVoice:nodes.interviewerVoice.value}:{}),...(nodes.hideExamples.checked?{hideExamples:true}:{}),...(exampleDrawingEnabled?{exampleDrawing:true}:{}),...(candidateLedEnabled?{candidateLed:true}:{}),...(!editorOptions.execution?{codeExecution:false}:{})})" )); assert!(!source.contains("candidateIdentity")); } @@ -1194,11 +1196,13 @@ fn browser_interview_duration_matches_the_server_clamp() { "web/interview.js clamps above the server maximum" ); - // The fallback the query string gets when it carries no duration. - let fallback = clamp[0] - .rsplit("||") + // The fallback the query string gets when it names no usable duration: the + // literal after the colon of `namedDuration ? clamp(...) : `. + let declaration = &interview[interview.find("let durationMin = ").unwrap()..]; + let fallback = declaration[..declaration.find(';').unwrap()] + .rsplit(':') .next() - .expect("the clamped value falls back to a literal") + .expect("the declaration falls back to a literal") .trim(); assert_eq!( parsed(fallback), diff --git a/tests/web/routes.rs b/tests/web/routes.rs index d9d30128..ccbae9cf 100644 --- a/tests/web/routes.rs +++ b/tests/web/routes.rs @@ -259,6 +259,44 @@ async fn the_oauth_callback_requires_the_state_it_minted() { remove_database(path).await; } +/// The lobby preselects the operator's length, so the server has to say what +/// it is; the page cannot read `CODETRIAL_DURATION_MIN` itself. What it says is +/// the length `/api/token` would grant: inside the range, and no longer than +/// the recording cap. +#[tokio::test] +async fn the_session_reports_the_default_length_it_would_grant() { + for (configured, recording, expected) in [ + (30, None, 30), + (5, None, MIN_DURATION_MIN), + (120, None, MAX_DURATION_MIN), + ( + 120, + Some(recording_config()), + recording_config().max_minutes, + ), + ] { + let mut config = web_config(); + config.default_duration_min = configured; + config.recording = recording; + let (base, server) = spawn_web_server(config).await; + + let session = http_client() + .get(format!("{base}/api/session")) + .send() + .await + .unwrap() + .json::() + .await + .unwrap(); + + assert_eq!( + session["defaultDurationMin"], expected, + "configured {configured}" + ); + server.shutdown().await; + } +} + /// The same endpoint on a server that records nothing, so the cap is the range /// the endpoint has always offered rather than a number the lobby invents. #[tokio::test] diff --git a/tests/web/token.rs b/tests/web/token.rs index 895f040e..7ba4c514 100644 --- a/tests/web/token.rs +++ b/tests/web/token.rs @@ -103,6 +103,7 @@ fn token_response_matches_frontend_contract() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, br#"{"problemId":"merge-intervals","durationMin":120}"#, "interview-fixed", @@ -136,6 +137,7 @@ fn token_ignores_a_mode_a_stale_client_still_sends() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; for body in [ br#"{"mode":"practice"}"#.as_slice(), @@ -163,6 +165,7 @@ fn token_interview_mode_is_allowlisted_and_defaults_to_the_editor() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; for (body, expected) in [ ( @@ -192,6 +195,7 @@ fn token_interview_loop_is_allowlisted_and_defaults_to_combined() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; for (body, expected) in [ ( @@ -218,6 +222,7 @@ fn only_explicit_false_disables_execution_in_signed_metadata() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; for value in [ json!(false), @@ -263,6 +268,7 @@ fn token_profile_is_bounded_and_enum_validated_before_signed_metadata() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, serde_json::to_string(&json!({"interviewProfile": { "role": format!(" {}\n", "r".repeat(100)), @@ -300,6 +306,7 @@ fn token_profile_is_bounded_and_enum_validated_before_signed_metadata() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, br#"{"interviewProfile":{"seniority":"founder","role":4}}"#, "room", @@ -320,6 +327,7 @@ fn token_grounding_is_signed_only_after_valid_consent_and_shape() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; let signed = token_response(&config, br#"{"interviewGrounding":{"consentVersion":1,"requirements":["Must know Rust"],"skills":["Rust"],"anchors":["Built a parser"]}}"#, "room", "candidate", 2_000).unwrap(); let metadata: Value = @@ -350,6 +358,7 @@ fn token_carries_hidden_examples_only_when_true() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; let metadata = |body: &[u8]| -> Value { let response = token_response(&config, body, "room", "candidate", 2_000).unwrap(); @@ -393,6 +402,7 @@ fn token_carries_candidate_led_only_when_true() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; let metadata = |body: &[u8]| -> Value { let response = token_response(&config, body, "room", "candidate", 2_000).unwrap(); @@ -428,6 +438,7 @@ fn token_carries_only_an_offered_language() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; let metadata = |body: &[u8]| -> Value { let response = token_response(&config, body, "room", "candidate", 2_000).unwrap(); @@ -466,6 +477,7 @@ fn token_response_mints_the_candidate_identity() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, br#"{"candidateIdentity":"candidate-a1b2c3"}"#, "interview-fixed", @@ -501,6 +513,7 @@ fn token_duration_matches_current_frontend_clamp() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, &body, "interview-fixed", @@ -525,6 +538,7 @@ fn token_duration_preserves_fractional_frontend_metadata() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }, br#"{"durationMin":42.8}"#, "interview-fixed", @@ -561,6 +575,7 @@ fn token_duration_stops_at_the_recording_cap() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min, + default_duration_min: DEFAULT_DURATION_MIN, }, &body, "interview-fixed", @@ -600,6 +615,7 @@ fn token_response_reports_the_length_it_granted() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min, + default_duration_min: DEFAULT_DURATION_MIN, }, &body, "interview-fixed", @@ -631,32 +647,6 @@ fn token_response_reports_the_length_it_granted() { } } -#[test] -fn token_duration_default_stops_at_the_recording_cap() { - // A caller who names no length gets the default, and the default is a - // length like any other. Handing back forty-five to a deployment that can - // only record thirty loses the same stretch, for the want of a request. - let response = token_response( - &TokenConfig { - api_key: "devkey", - api_secret: "devsecret", - server_url: "wss://example.livekit.cloud", - recording_max_min: Some(30), - }, - b"{}", - "interview-fixed", - "candidate-fixed", - 2000, - ) - .unwrap(); - let claims = claims(&response.token); - - assert_eq!( - serde_json::from_str::(claims["metadata"].as_str().unwrap()).unwrap()["durationMin"], - 30 - ); -} - #[test] fn token_duration_survives_a_recording_cap_under_the_floor() { // `f64::clamp` panics when its bounds cross, so a cap below the shortest @@ -667,6 +657,7 @@ fn token_duration_survives_a_recording_cap_under_the_floor() { api_secret: "devsecret", server_url: "wss://example.livekit.cloud", recording_max_min: Some(1), + default_duration_min: DEFAULT_DURATION_MIN, }, br#"{"durationMin":45}"#, "interview-fixed", @@ -1561,6 +1552,7 @@ fn token_resolves_voice_and_accent_independently() { api_secret: "secret", server_url: "wss://example.test", recording_max_min: None, + default_duration_min: DEFAULT_DURATION_MIN, }; for voice in ["", "Default", "Random"] .into_iter() diff --git a/tests/web/token_default.rs b/tests/web/token_default.rs new file mode 100644 index 00000000..f5ebe05c --- /dev/null +++ b/tests/web/token_default.rs @@ -0,0 +1,106 @@ +//! The length a token request that names none is granted: the operator's +//! `CODETRIAL_DURATION_MIN`, held to the endpoint's range and the recording +//! cap. +//! +//! Split out of `token.rs`, which is at its size ceiling. + +use super::*; + +/// A request that names no length gets the operator's `CODETRIAL_DURATION_MIN`, +/// held to the same range and recording cap as a length the lobby asked for. +#[test] +fn token_without_a_length_gets_the_configured_default() { + for (default_duration_min, recording_max_min, expected) in [ + (30, None, json!(30)), + (DEFAULT_DURATION_MIN, None, json!(DEFAULT_DURATION_MIN)), + // Inside the range the endpoint offers, whatever the file says. + (5, None, json!(MIN_DURATION_MIN)), + (120, None, json!(MAX_DURATION_MIN)), + // A default the cap cannot record is shortened like any other length. + (60, Some(45), json!(45)), + (30, Some(45), json!(30)), + // The built-in default on a deployment that can only record thirty + // minutes: handing back forty-five loses the same stretch. + (DEFAULT_DURATION_MIN, Some(30), json!(30)), + ] { + let response = token_response( + &TokenConfig { + api_key: "devkey", + api_secret: "devsecret", + server_url: "wss://example.livekit.cloud", + recording_max_min, + default_duration_min, + }, + b"{}", + "interview-fixed", + "candidate-fixed", + 2000, + ) + .unwrap(); + + assert_eq!( + response.duration_min, expected, + "default {default_duration_min} under a cap of {recording_max_min:?}" + ); + // The agent is handed the length in the signed room metadata. + let metadata = claims(&response.token)["metadata"] + .as_str() + .unwrap() + .to_string(); + assert_eq!( + serde_json::from_str::(&metadata).unwrap()["durationMin"], + expected, + "metadata for default {default_duration_min} under a cap of {recording_max_min:?}" + ); + } +} + +/// The route hands the operator's length to the token, not the built-in one: +/// `token_response` is tested above with the field already set, so this is the +/// test that notices `/api/token` going back to the constant. +#[tokio::test] +async fn the_token_route_grants_the_configured_default_to_a_request_naming_none() { + for (configured, expected) in [(30, 30), (60, 60), (120, MAX_DURATION_MIN)] { + let mut config = web_config(); + config.guest_mode_enabled = true; + config.default_duration_min = configured; + let (base, server) = spawn_web_server_with_guest_dispatcher(config).await; + + let response = http_client() + .post(format!("{base}/api/token")) + .json(&json!({"problemId": "two-sum"})) + .send() + .await + .unwrap(); + + assert_eq!(response.status(), 200, "configured {configured}"); + let body: Value = response.json().await.unwrap(); + assert_eq!(body["durationMin"], expected, "configured {configured}"); + server.shutdown().await; + } +} + +/// `/api/session` reports the operator's length to a signed-in candidate as it +/// does to a signed-out one. A default of 60 cannot be mistaken for the +/// built-in 45, which is what a branch that forgot the field would send. +#[tokio::test] +async fn the_signed_in_session_reports_the_configured_default() { + let (mut config, cookie, path) = signed_in_web_config("signed-in-default-duration"); + config.default_duration_min = 60; + let (base, server) = spawn_web_server_with_guest_dispatcher(config).await; + + let session: Value = http_client() + .get(format!("{base}/api/session")) + .header("cookie", cookie) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + + assert_eq!(session["signedIn"], true); + assert_eq!(session["defaultDurationMin"], 60); + server.shutdown().await; + remove_database(path).await; +} diff --git a/web/app.js b/web/app.js index f7ce345a..7d9b4aa1 100644 --- a/web/app.js +++ b/web/app.js @@ -103,6 +103,11 @@ let searchVisibilityException = null; /// drawn from changes. let keptDraw = null; let manualDuration = false; +/// Whether the lobby has stopped waiting on `/api/session`, which answered or +/// failed. Until then a length still on the markup's 45 is not the candidate's +/// to send, so a Start pressed meanwhile leaves it out and the operator's +/// default applies; any other length showing was picked here and goes. +let durationSettled = false; // Preserve the explicit preference independently of the effective server limit. let chosenDuration; let manualDifficulty = false; @@ -127,6 +132,9 @@ let deletingReports = false; /// file is still in its temporal dead zone, and reading one there throws /// before the lobby has rendered anything at all. let durationCeiling = Infinity; +/// The length a Medium or Hard problem suggests: the preselected button until +/// `/api/session` reports the operator's own `CODETRIAL_DURATION_MIN`. +let defaultDuration; /// Whether the interviewer sends candidate video, from `/api/session`. Null /// until it answers, when it fails, and when this server does not host the /// interviewer itself; the billing notice above Start stays hidden while null. @@ -691,6 +699,10 @@ nodes.attemptDownloadSelected.addEventListener("click", async () => { }); const durations = [...document.querySelectorAll("[data-duration]")]; +defaultDuration = Number( + document.querySelector("[data-duration].selected")?.dataset.duration, +); +const markupDuration = defaultDuration; for (const button of durations) { button.addEventListener("click", () => { if (starting) return; @@ -783,7 +795,8 @@ start.addEventListener("click", async () => { for (const difficulty of selectedDifficulties()) destination.searchParams.append("difficulty", difficulty); } else destination.searchParams.set("problem", problem.id); - destination.searchParams.set("duration", String(duration)); + if (manualDuration || durationSettled || duration !== markupDuration) + destination.searchParams.set("duration", String(duration)); destination.searchParams.set("loop", interviewLoop); destination.searchParams.set("mode", mode); if (mode === "coding" && nodes.exampleDrawing.checked) @@ -1118,7 +1131,20 @@ async function loadAccount(generation) { ) { throw new Error("Invalid account response"); } + durationSettled = true; accountHistory = false; + // Re-suggest only when the operator's length differs from the markup's, so + // a lobby on the built-in default keeps a problem the candidate picked, or + // when a restored lobby learns a ceiling other than the one it settled + // under (a suggestion made by hand-ticking a level is not a choice). + const operatorDuration = + Number.isFinite(session.defaultDurationMin) && + session.defaultDurationMin !== defaultDuration; + const ceilingMoved = + Number.isFinite(durationCeiling) && + Number.isFinite(session.maxDurationMin) && + session.maxDurationMin !== durationCeiling; + if (operatorDuration) defaultDuration = session.defaultDurationMin; const peerLink = document.getElementById("peer-practice"); if (peerLink) peerLink.hidden = !( @@ -1127,6 +1153,17 @@ async function loadAccount(generation) { (session.identityVerified ?? session.user?.identityVerified) ); applyDurationCeiling(session.maxDurationMin); + if (operatorDuration || ceilingMoved) { + // A problem picked by hand before the session answered suggested its own + // length from its own difficulty; the checkboxes would overrule it. + setDuration( + suggestedDuration( + manualProblem && problem + ? new Set([problem.difficulty]) + : selectedDifficulties(), + ), + ); + } candidateVideo = typeof session.candidateVideo === "boolean" ? session.candidateVideo @@ -1171,6 +1208,7 @@ async function loadAccount(generation) { // can no longer tell who pays, not because nothing will be billed: Start // stays enabled, and an interview started now may still be. if (generation === historyLoad) { + durationSettled = true; candidateVideo = null; applyCostNote(); } @@ -1703,13 +1741,16 @@ function mark(button, on) { button.setAttribute("aria-pressed", String(on)); } -/// Capped at the server's own default length, which src/config.rs holds at or -/// under the recording cap, so nothing the lobby picks on its own outlives its -/// recording. Only the suggestion is capped: the sixty minute button is one -/// click away and a deployment can set the two environment variables apart, so -/// this narrows the default rather than guaranteeing anything. +/// The operator's default length for Medium and Hard, and no more than thirty +/// for Easy. Snapped to a button the row offers, so a default of 20 or 75 still +/// leaves one selected; the ceiling is applied afterwards by `setDuration`. function suggestedDuration(difficulties) { - return difficulties.has("Medium") || difficulties.has("Hard") ? 45 : 30; + const hard = difficulties.has("Medium") || difficulties.has("Hard"); + const wanted = hard ? defaultDuration : Math.min(30, defaultDuration); + const offered = durations.map((button) => Number(button.dataset.duration)); + return offered.reduce((best, value) => + Math.abs(value - wanted) < Math.abs(best - wanted) ? value : best, + ); } function setInterviewLoop(value) { diff --git a/web/interview.js b/web/interview.js index 50f41daf..2e9fe05a 100644 --- a/web/interview.js +++ b/web/interview.js @@ -352,15 +352,17 @@ let languages = []; // A delayed judge may make the requested C tab available after the first // paint. Do not switch to it after the candidate has chosen a tab or edited. let languageChoiceCommitted = false; +const named = Number(params.get("duration")); +/// Whether the URL named a usable length, which `?duration=`, `?duration=abc`, +/// `?duration=30abc`, `0` and `-5` do not: decided once, here, for the token +/// request and for the round plan alike. +const namedDuration = named > 0; /// What the lobby asked for, until `/api/token` says what it got. The range -/// here mirrors the server's own and is the fallback for a URL that arrives -/// without passing through the lobby; the answer below is what the interview -/// actually runs on. -let durationMin = clamp( - Number.parseInt(params.get("duration") || "45", 10) || 45, - 10, - 90, -); +/// here mirrors the server's own, and 45 is only the length shown until the +/// answer lands for a URL that arrives without passing through the lobby: such +/// a request names no length, so the server's configured default decides. The +/// answer below is what the interview actually runs on. +let durationMin = namedDuration ? clamp(named, 10, 90) : 45; const interviewLoop = codingLoop(params.get("loop")); /// Whether this interview is held at a whiteboard rather than in the editor. /// @@ -444,7 +446,11 @@ const board = { }; let behavioralMinutes = interviewLoop === "coding_behavioral" ? Math.min(8, durationMin) : 0; -let codingMinutes = durationMin - behavioralMinutes; +/// Rounded because fractional minutes are kept, and `33.3 - 8` is +/// 25.299999999999997 in front of the candidate otherwise. +const minutesAfter = (total, reserve) => + Math.round((total - reserve) * 100) / 100; +let codingMinutes = minutesAfter(durationMin, behavioralMinutes); /// The server clamps the requested length to its range and, where it records, /// to the recording cap, and it is that number the agent is handed in the room @@ -452,17 +458,17 @@ let codingMinutes = durationMin - behavioralMinutes; /// round split and the saved report describing an interview nobody is having. /// /// An answer that is missing or unusable leaves the request in place: the page -/// still runs, on the length the URL named, exactly as it did before the -/// server sent this. +/// still runs, on the length shown so far (the URL's, else the provisional 45), +/// exactly as it did before the server sent this. function applyGrantedDuration(granted) { if (typeof granted !== "number" || !Number.isFinite(granted) || granted <= 0) return; durationMin = granted; behavioralMinutes = interviewLoop === "coding_behavioral" ? Math.min(8, durationMin) : 0; - codingMinutes = durationMin - behavioralMinutes; + codingMinutes = minutesAfter(durationMin, behavioralMinutes); // The budget is on screen by now: `bindEvents` wrote it during setup, from - // the length the URL asked for. Leaving it there would put the old number in + // the length shown so far. Leaving it there would put the old number in // front of the candidate for the whole interview. renderRoundPlan(); } @@ -1183,7 +1189,14 @@ function bindEvents() { nodes.jimMute.addEventListener("click", () => jimVolume.toggleMute()); nodes.jimVolumeReset.addEventListener("click", () => jimVolume.reset()); renderJimVolume(); - renderRoundPlan(); + // A URL that names no length gets the server's default, so the plan waits for + // `/api/session` rather than show 45 minutes and then jump to the real one. + if (namedDuration) renderRoundPlan(); + else + void sessionPromise.then((session) => { + applyGrantedDuration(session?.defaultDurationMin); + renderRoundPlan(); + }); nodes.problemTab.addEventListener("click", () => selectTab("problem")); nodes.transcriptTab.addEventListener("click", () => selectTab("transcript")); nodes.mic.addEventListener("click", toggleMicrophone); @@ -1992,7 +2005,7 @@ async function connect(preflight, presenting = false) { ...(previous ? { previousAssignment: previous } : {}), } : { problemId: problem.page }), - durationMin, + ...(namedDuration ? { durationMin } : {}), interviewId, interviewLoop, interviewMode: mode,