Repository navigation
Conversation
69ca930 to
16e2bce
Compare
16e2bce to
7792ace
Compare
5fb22be to
763f8e5
Compare
763f8e5 to
4365539
Compare
ColtenOuO
left a comment
There was a problem hiding this comment.
Changes look good to me, before merge this PR, please squash your commit.
4365539 to
63e6cab
Compare
|
The browser CI failure also occurs on upstream main at fd7c23c, before the Gemini changes in this PR: AssertionError: 57 !== 4 The same test passed at the preceding main commit, 0116b9b, then failed after merging #401 at fd7c23c. This PR's browser job reports the same assertion failure. Code inspection points to an interaction between #410's layout test and #401's new .problem-entry wrapper. The test temporarily changes .problem-card.hidden and later clears it, but #401's filtering also hides the enclosing .problem-entry. Clearing the button's hidden property therefore does not necessarily make it visible. The subsequent .problem-card:not([hidden]) query still includes buttons inside hidden wrappers; their zero bounding rectangles cause them to be counted as sharing one row. The layout test needs to account for the wrapper when changing visibility and count cards that are actually rendered. This analysis is based on the CI logs and source comparison; I have not rerun a corrected test yet. |
63e6cab to
0ef4e22
Compare
| } | ||
| // A deployment with no live session still offers the old timed practice. | ||
| // A failed readiness check never turns a joined interview into that clock. | ||
| if (!error?.startup && !state.joinedRoom) { |
There was a problem hiding this comment.
After a failed startup check, connect() returns null, so init() stops at if (!startup) return; and never reaches updateBeforeUnloadGuard(). Meanwhile the banner tells the candidate "You can still work the problem offline." shouldWarnBeforeUnload only checks endsAt, so a candidate who keeps coding offline can close or reload the tab and lose everything without a prompt. Before this PR, a degraded start always set endsAt and turned the guard on. Turn the guard on for this path without starting the clock, for example with a flag passed to shouldWarnBeforeUnload. The same early return also hits any non-startup failure after state.joinedRoom is set (for example, publishIntegrityEvent throwing). If that is intended, the comment should say so, because it currently mentions only readiness.
| language: state.language, | ||
| }); | ||
| } | ||
| const startup = await state.interviewerReadiness.wait; |
There was a problem hiding this comment.
Outside Strict mode, nodes.end and nodes.pause stay enabled during this wait, which now lasts the whole server-side startup check. Pressing End here sends end_interview to an agent that only defers it until readiness, so when the check then fails the candidate is left on the ending overlay waiting for a report the agent will never send, and when it succeeds they get a report for an interview whose clock never started. Keep both buttons disabled until readiness settles, the way init() already does for Strict, or have endInterview skip the report wait while state.interviewerReadiness is still pending.
0ef4e22 to
35e28ce
Compare
| if ( | ||
| participant?.identity === state.agentIdentity || | ||
| (!state.agentIdentity && isAgent(participant)) | ||
| ) |
There was a problem hiding this comment.
isolate_local_agent removes duplicate agent participants during startup. That runs before this agent publishes its audio track, so the browser has usually not adopted an agent yet. When the duplicate's ParticipantDisconnected lands, !state.agentIdentity && isAgent(participant) holds, so fail() settles readiness as failed. The candidate then gets the "did not answer the startup check" banner just before the real interviewer would have reported ready. Fail only when no interviewer is left in the room.
| if ( | |
| participant?.identity === state.agentIdentity || | |
| (!state.agentIdentity && isAgent(participant)) | |
| ) | |
| if ( | |
| isAgent(participant) && | |
| !roomInterviewer(roomParticipants(), state.agentIdentity) | |
| ) |
Live setup can succeed without the model answering. Starting the clock at setup spends candidate time against a silent provider. Require model output before timing an interview, and retain untimed practice after a failed opening without retrying that generation. Readiness must preserve existing setup failover, keepalive, candidate media and reconnect grace. Retain provider failure reasons and observed usage so failed starts remain diagnosable without duplicate generation or accounting. Protect editable work before the clock starts and after an untimed failure. Detect early interviewer departures, keep recording delays from blocking cleanup, and prevent premature report requests. Keep Strict failure guidance consistent with its disabled controls. Isolate startup tests from unrelated model loading and rendering so they measure startup ordering reliably. Closes sysprog21#159
35e28ce to
d89b4a2
Compare
jserv
left a comment
There was a problem hiding this comment.
Before asking someone to review, you MUST resolve any regressions flagged by GitHub Actions.
| usage: &mut crate::gemini::TokenUsage, | ||
| publication: impl std::future::Future<Output = Result<(), Box<dyn std::error::Error + Send + Sync>>>, | ||
| ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> { | ||
| let result = tokio::time::timeout(RECONNECT_NOTICE_LIMIT, publication) |
There was a problem hiding this comment.
RECONNECT_NOTICE_LIMIT is one second, sized for a reconnect notice that retry_cold_open logs and drops on timeout. Here the same bound makes the ready notice fatal: when publish_data takes longer than a second on a congested data channel, an interview whose greeting Gemini already generated (and billed) is torn down, and the failure notice that follows carries unavailable, which the page words as "Gemini did not answer the startup check". Give the ready notice its own bound that fits inside the browser's readiness wait, and do not report a publication failure as a provider failure.
| interviewerReadiness: gate, | ||
| }; | ||
| let updates = 0; | ||
| const handler = runInNewContext(`(${handlerSource})`, { |
There was a problem hiding this comment.
The handler now calls roomInterviewer and roomParticipants, but this context still passes only state, isAgent and updateAgentState, so this test and the next one throw ReferenceError: roomInterviewer is not defined in the browser gate. Pass roomInterviewer and a roomParticipants stub that returns who is still in the room, and rewrite the cases for the new rule: an agent's departure fails readiness only when no interviewer remains, so another agent leaving while the pinned one stays settles nothing, and the pinned one leaving fails it only when no other agent is left.
| } | ||
|
|
||
| async function assertPendingControls(page) { | ||
| assert.equal(await page.locator("#end").isDisabled(), true); |
There was a problem hiding this comment.
init now leaves End enabled outside Strict mode during the readiness wait, so this helper fails both practice-mode tests that call it (false !== true in the browser gate), and its click on End would start the ending flow rather than do nothing. Assert End disabled and inert only when the page is in Strict mode, and for practice mode assert what pressing End before readiness is meant to do, so the test pins the behavior the open thread on End during the startup wait settles on.
jserv
left a comment
There was a problem hiding this comment.
Rebase latest main branch and resolve conflicts.
Live setup can complete without model output. Use the normal opening generation as one bounded readiness attempt, preserve its events for delivery, and start the interview clock only after the first model output. Startup failures do not retry.
Carry outcomes on the existing
interviewer_statemessage and degraded-start banner. Name provider-specific errors only when the provider identifies them.Validation
./scripts/test.shpassed after rebasing onto546f559, with Chromium, commentflow, and actionlint enabled. Startup tests cover setup without output, timeout, a single generation attempt, event preservation, and timer ordering.web/audio/test-voice.wavcapture fixture.df170a8; the release binary reports that revision.Browser tests used a temporary Liberation Sans alias; the default-font lobby failure also reproduces on base
a3f3e80. Live provider integration was not run.Closes #159
Summary by cubic
Live setup can finish without model output, so the interview clock can run against a silent provider. This change starts the interview timer and hard deadline only after the first model output—nonempty audio, model text, output transcription, or a tool call—using the opening greeting as a single bounded readiness attempt that never retries. A failed check ends the interview; offline practice that continues stays untimed.
Failure reporting
interviewer_statecontrol topic withstartup: "ready"orstartup: "failed"plus a classified reason (quota,billing,credentials,refused, orunavailable), shown in the degraded-start banner.startup_receivedreceipt for failed readiness, hashed and published under the same bound as report receipts, and keeps the diagnosis through room teardown.Socket and client behavior
Written for commit d89b4a2. Summary will update on new commits.