Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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; 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 |
Expand Down
27 changes: 19 additions & 8 deletions docs/interview-length.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,19 @@ 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
Comment thread
RWong04 marked this conversation as resolved.
Resolved
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.

Recording lowers it. `CODETRIAL_RECORDING_MAX_MINUTES` bounds a single
recording, and the sweeper fails one still running five minutes past it, so an
Expand All @@ -24,8 +34,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.
2 changes: 1 addition & 1 deletion scripts/rust-style-ceilings.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,5 @@
"tests/unit/tasks/session.rs": 1861,
"tests/web/accounts.rs": 1808,
"tests/web/recording.rs": 2496,
"tests/web/token.rs": 2151
"tests/web/token.rs": 2143
}
27 changes: 27 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,33 @@ 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<String>) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configured_duration is now the only place that clamps CODETRIAL_DURATION_MIN, while AgentConfig still reads the same variable through optional_u32 at default_duration_min. For 120 or 5, the web server and the agent path (check-gemini, the setup smoke) can then see different lengths unless something downstream clamps. Follow-up: build default_duration_min from configured_duration too, so both paths read one parse.

let value = value.map(str::trim).unwrap_or_default();
if value.is_empty() {
return (DEFAULT_DURATION_MIN, None);
}
let (reason, used) = match value.parse::<u32>() {
Ok(minutes) if (MIN_DURATION_MIN..=MAX_DURATION_MIN).contains(&minutes) => {
return (minutes, None);
}
Ok(minutes) => (
format!("is outside {MIN_DURATION_MIN} to {MAX_DURATION_MIN}"),
minutes.clamp(MIN_DURATION_MIN, MAX_DURATION_MIN),
),
Err(_) => (
"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<String, String>, key: &str, default: u32) -> u32 {
values
.get(key)
Expand Down
25 changes: 16 additions & 9 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ use std::collections::BTreeMap;
use std::path::PathBuf;
use std::sync::Arc;

use codetrial::config::{
AgentConfig, DEFAULT_DURATION_MIN, DEFAULT_ROOM_PREFIX, DEFAULT_WEB_ADDR, DEFAULT_WEB_DIR,
};
use codetrial::config::{AgentConfig, DEFAULT_ROOM_PREFIX, DEFAULT_WEB_ADDR, DEFAULT_WEB_DIR};
use codetrial::web::{RoomDispatcher, WebServerConfig};

/// One installation keeps its config file and its account database together in
Expand Down Expand Up @@ -355,6 +353,7 @@ 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: interview_duration_min(values),
Comment thread
RWong04 marked this conversation as resolved.
Resolved
Comment thread
RWong04 marked this conversation as resolved.
Resolved
recording,
pool,
probe_provider_quota: true,
Expand Down Expand Up @@ -400,6 +399,10 @@ fn run_web(options: CliOptions) -> Result<(), String> {
let max_concurrent = read_max_concurrent(&values);

let production = is_production(&values);

if let Some(warning) = configured_duration(&values).1 {
eprintln!("{warning}");
}
let pool = web_provider_pool(&values, &options, production)?;

let mut config = web_server_config(&values, pool, production, &options)?;
Expand Down Expand Up @@ -1232,13 +1235,17 @@ 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.
/// The configured interview length, held to the range the endpoints offer,
/// 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<String, String>) -> u32 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser tests cover 120 -> 90 and 5 -> 10, but nothing checks that load_recording now receives the clamped value. Add cases for default 120 with cap 90 (starts), default 120 with cap 89 (refused), and an unreadable value with a cap below 45 (refused, since it reads as 45).

nonempty(values, "CODETRIAL_DURATION_MIN")
.and_then(|value| value.parse().ok())
.unwrap_or(DEFAULT_DURATION_MIN)
configured_duration(values).0
}

/// [`codetrial::config::configured_duration`] over the config values.
fn configured_duration(values: &BTreeMap<String, String>) -> (u32, Option<String>) {
codetrial::config::configured_duration(values.get("CODETRIAL_DURATION_MIN").map(String::as_str))
}

/// `CODETRIAL_ALLOWED_HOSTS`, a comma-separated list of `Host` values.
Expand Down
8 changes: 6 additions & 2 deletions src/web/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,8 +540,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.
Expand All @@ -565,6 +567,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,
Expand All @@ -583,6 +586,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,
}),
),
Expand Down
5 changes: 5 additions & 0 deletions src/web/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,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
Expand Down Expand Up @@ -165,6 +168,7 @@ impl fmt::Debug for WebServerConfig {
production,
compiler_explorer_enabled,
guest_mode_enabled,
default_duration_min,
trusted_proxy_hops,
allowed_hosts,
recording,
Expand Down Expand Up @@ -195,6 +199,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)
Expand Down
29 changes: 25 additions & 4 deletions src/web/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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<u32>,
/// `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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -422,14 +428,28 @@ pub(crate) fn duration_ceiling(recording_max_min: Option<u32>) -> u32 {
})
}

pub(crate) fn token_duration_min(value: Option<&Value>, recording_max_min: Option<u32>) -> 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>) -> 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<u32>,
) -> 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))
Comment thread
RWong04 marked this conversation as resolved.
Resolved
} else {
// Fractional minutes stay fractional: the metadata is echoed back to
// the frontend, which renders whatever the lobby asked for.
Expand Down Expand Up @@ -714,6 +734,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,
Comment thread
RWong04 marked this conversation as resolved.
Resolved
},
body,
room_name,
Expand Down
2 changes: 1 addition & 1 deletion tests/browser/account.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
101 changes: 101 additions & 0 deletions tests/browser/interviewer-voice.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,104 @@ 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=60", true],
]) {
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);
if (sent) assert.equal(body.durationMin, 60);
} 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");
await page.waitForFunction(() => document.querySelector("#audio-check"));
assert.equal(await plan.textContent(), "");
release();
await page.waitForFunction(() =>
/30 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();
}
});
Loading
Loading