Repository navigation
Conversation
72f6667 to
e64598e
Compare
Alan52254
left a comment
There was a problem hiding this comment.
The server side is well factored: default_duration is the single source for both /api/session and /api/token, which is exactly what docs/interview-length.md asks for. My comments are about where an invalid or missing input silently changes the result, and about two test-scope issues. The one change I would make before merging is the defaultDuration fallback in web/app.js; the others are smaller. I did not repeat the open threads on web/interview.js:355.
e64598e to
0609ef1
Compare
0609ef1 to
ceaea79
Compare
ceaea79 to
46a811e
Compare
46a811e to
32f8528
Compare
The README and docs/interview-length.md call CODETRIAL_DURATION_MIN the length the lobby preselects, but only check-gemini, the setup probe and the recording cap check read it. The lobby always started on 45 for Medium and Hard, and a token request that named no length got the constant, so an operator who set 30 saw no change. /api/session now reports the default as defaultDurationMin, clamped into the range and to the recording ceiling by the same function that /api/token uses for a request that names no length. The lobby preselects it for Medium and Hard, and at most 30 for Easy. A value that is not one of the buttons selects the nearest one, and a length the candidate chose is left alone. With the variable unset, nothing changes. Closes sysprog21#582 Change-Id: If622235c6ab35e570645bdf99c131b013ace41bf
32f8528 to
4d13dd5
Compare
| }); | ||
|
|
||
| const durations = [...document.querySelectorAll("[data-duration]")]; | ||
| defaultDuration = Number( |
There was a problem hiding this comment.
defaultDuration starts as the markup's 45, and syncLobbyControls lets Practice start a manual pick while /api/session is still loading. A candidate who picks a problem and presses Start before the session answers navigates with ?duration=45, so the interview page sends durationMin: 45 and the operator's CODETRIAL_DURATION_MIN=60 is bypassed. The new tests release the held session before they press Start, so they do not cover this. Either keep Start disabled while the duration is still the unconfirmed markup value and the candidate has not chosen one, or leave duration out of the URL until the session has answered or a button was clicked. Add a test that presses Start while /api/session is held.
| /// Whether the URL named a usable length, which `?duration=` and | ||
| /// `?duration=abc` do not: decided once, here, for the token request and for the | ||
| /// round plan alike. | ||
| const namedDuration = Number.parseInt(params.get("duration") ?? "", 10) > 0; |
There was a problem hiding this comment.
?duration=-5 used to be clamped to 10 and requested as such. Now it counts as unnamed, so durationMin is omitted and the server grants the configured default. That changes behavior for a malformed URL even with CODETRIAL_DURATION_MIN unset (still 45 here, 10 before). If the new treatment is intended, say so in docs/interview-length.md and pin it with a test for -5 and 0.
| /// 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>) { |
There was a problem hiding this comment.
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.
| /// 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 { |
There was a problem hiding this comment.
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).
Problem and behavior
CODETRIAL_DURATION_MINis documented as the length the lobby preselects, but onlycheck-gemini, the setup probe and the recording cap check read it, so an operator who set 30 still saw 45 for Medium and Hard, and a token request that named no length got the constant 45.With this change
/api/sessionreports the default asdefaultDurationMin, the lobby preselects it for Medium and Hard and at most 30 for Easy, and a token request that names no length is granted it; with the variable unset nothing changes.For example, with
CODETRIAL_DURATION_MIN=60a Medium lobby preselects 60 and an Easy lobby preselects 30; a value that is not one of the three buttons selects the nearest one in the lobby (20 gives 30, and 75 gives 60 only because there is no 90 button).Approach
One function,
default_durationinsrc/web/token.rs, clamps the configured value into 10 to 90 and to the recording ceiling, and both/api/sessionand/api/tokencall it, so the length that is offered and the length that is granted cannot drift apart.WebServerConfigandTokenConfigcarry the configured value,web/app.jstakes the operator value insuggestedDuration, and it re-suggests after/api/sessiononly when that value differs from the markup's 45, so a length or a problem the candidate picked before the session answered is kept.Snapping to the nearest button happens in the lobby only, and
/api/tokengrants the clamped value as is; the new tests for the token default live intests/web/token_default.rsbecausetests/web/token.rsis at its size ceiling.Validation
cargo test --lockedandcargo clippy --locked --workspace --all-targetspass, andnode --test --test-name-pattern="operator default" tests/browser/lobby.test.jspasses eight tests; I broke the new session, wiring, snapping, latch and manual-problem paths one at a time and each broke a test../scripts/test.shstill failstask-authoring-tests,run-budget-tests,interactive-review-hook-tests,git-hooksandshell-syntaxon this machine, and the same gates fail on a cleanupstream/mainat 8f5ad5c, before #581 merged; the javac and ffmpeg tests were skipped.I did not check the lobby by hand in a browser: the first paint shows 45 until
/api/sessionanswers, and a non-button value snaps without a note, so those two are unverified as user experience.Interactive review
This is one commit of 382 added plus deleted lines, over the 200-line threshold, so I went through the local interactive review on the committed head 8a905a0 and explained the key change in my own words there.
Suggested reading order:
src/web/token.rs(default_duration),src/web/auth.rs(/api/session),web/app.js(suggestedDurationandloadAccount), then the tests; the rest is the new field threaded through fixtures.I am happy to explain the change to a maintainer for the
explainedlabel.Closes #582
Summary by cubic
CODETRIAL_DURATION_MINnow actually preselects the interview length in the lobby and becomes the default granted to a token request that names none; previously it was documented as doing this, but the lobby always started on 45 for Medium and Hard, and a nameless request got the constant 45./api/sessionreports the default asdefaultDurationMin, clamped to the allowed range and the recording cap.durationMinfrom the request when the URL sets none, so the server's default is honored.CODETRIAL_DURATION_MINthat is not a whole number prints a warning instead of silently using 45.Closes #582
Written for commit 4d13dd5. Summary will update on new commits.