Conversation
|
Waiting for #40 to be merged without conflicts, then I'll mark this ready for review. |
|
The The fix is |
Go ahead. |
The start handler read which snippets were checked before the sign-in round trip, but looked their text up in the extracted lists after it. A JD picked while a gated start was waiting on /api/login replaced those lists, so the stored indexes pointed into the new file and the packet carried snippets the candidate never checked. The lists are now copied alongside the selection, the way the rest of the form is read.
17fbd8d to
57bd1e5
Compare
Copying the extracted lists before the sign-in round trip kept the indices and the text together, but still assembled the packet after it, and a start the packet was always going to refuse spent a login first. The packet is now built with the rest of the form, and only stored once the sign-in has answered.
|
Fixed the implementation according to the review on #40. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| await page.fill("#github-login", "candidate"); | ||
| await page.click("#start"); | ||
| await page.click("#grounding-clear"); | ||
| await page.uncheck("#grounding-consent"); |
There was a problem hiding this comment.
clearGrounding already sets groundingConsent.checked to false, and uncheck returns immediately on a box that is already unchecked, so this line does nothing. It reads as coverage for revoking consent mid-start, but the only assertion here is on the snippet text, so that path is untested. Drop it, or make it a separate test that asserts a revoked consent leaves nothing in codetrial.interview-grounding.v1.
| const consented = nodes.groundingConsent.checked; | ||
| let packet; | ||
| try { | ||
| packet = selectedGroundingPacket(grounding, checkedGrounding(), nodes.groundingConsent.checked); |
There was a problem hiding this comment.
Snapshotting consent here along with the snippets changes what Clear document grounding does during the sign-in wait: on main the clear won that race, since it empties grounding, pick drops the stale indexes, selectedGroundingPacket returns null, and nothing is stored. Now the pre-click packet is stored once the login resolves even though the candidate unchecked consent and cleared the lists, which is the one control whose later state should still win over the snapshot. Re-read nodes.groundingConsent.checked next to storeGroundingPacket and store null when it is no longer set.
2 participants
Summary
Follow-up to the review on #40. On a lobby that requires sign-in, the start handler read the checked snippet indexes before
/api/loginbut looked up their text after it. A JD picked during that wait replaced the lists, so the packet sent snippets the candidate never checked. This predates #39 and #40.Changes
web/app.js: copy the three grounding lists next toselectedbefore the sign-in await, and build the packet from that copy.tests/browser/lobby.test.js: hold/api/login, load a second JD during the hold, and assert the stored packet still has the first JD's snippet. Fails onmain(shipsMust know SQL), passes with the fix.Summary by cubic
Builds the grounding packet when start is pressed, before the sign-in round trip, so a job description picked during login can't replace the checked snippets that get sent. Previously the start read checked indexes before
/api/loginand looked up the text after, which could ship snippets the candidate never checked; a start that would be refused also signed in first. The packet is now built with the rest of the form and stored only after sign-in succeeds, with tests covering both the race and the no-login refusal.Written for commit 2fd628c. Summary will update on new commits.