Repository navigation
fix(proxy): block prompts immediately when SSO session is invalid - #624
Conversation
8e103a4 to
719dcc3
Compare
The UserPromptSubmit auth gate awaited the browser sign-in (up to 120 s) before returning the block decision. Claude Code kills the hook after its timeout and a killed hook does not block, so the prompt reached the model. The gate now starts the sign-in in a detached process (sso-login) and returns the block decision right away. A lock file prevents concurrent sign-ins and is removed by the child after a successful sign-in. The block message reflects whether a sign-in was started, is already in progress, or is cooling down. Refs: EPMCDME-15378 Generated with AI
ab65ea7 to
677a5c8
Compare
…cycle test The rebase moved isProcessAlive from daemon-manager.ts to utils/processes.ts (needed by the SSO detached-login lock), but daemon-manager no longer exports it, breaking dm.isProcessAlive(...) call sites in this test. Generated with AI Co-Authored-By: codemie-ai <codemie.ai@gmail.com>
tryCreateLock wrote the lock with writeFile(path, data, { flag: 'wx' }),
which performs open(O_EXCL) and write() as separate syscalls. A concurrent
caller could read the lock in between — seeing an empty file, failing JSON
parsing, and treating it as corrupt — then remove and recreate it, letting
two concurrent calls both win and both start a sign-in.
Write the lock content to a private temp file first, then publish it with
link(), which fails EEXIST like wx but only once the content is complete,
so a reader never observes a half-written lock.
Generated with AI
Co-Authored-By: codemie-ai <codemie.ai@gmail.com>
…gress The previous fix (link() publish) did not address the main cause of the flaky concurrency test: after spawning, the lock is rewritten with the child pid via writeFile, which truncates before writing. A concurrent caller reading in that window saw an empty lock, treated it as corrupt, and started a second sign-in. Under Promise.all both calls also shared the same temp file name, and since a hard link shares the inode, the second temp write truncated the already published lock, making the flake more frequent. Revert tryCreateLock to the plain wx write and fix it on the reader side: an unreadable lock modified within the last 5s is reported as in-progress, covering both the wx open/write gap and the child-pid rewrite. Older unreadable locks are still replaced as corrupt. Generated with AI Co-Authored-By: codemie-ai <codemie.ai@gmail.com>
Bondimm
left a comment
There was a problem hiding this comment.
- CI is green on
c8d9232b(Validate Commits & PR, Secrets Detection, Build, Test Ubuntu, Test Windows). The branch is up to date withmainand merges cleanly. - Checked locally on
c8d9232b:npm run typecheckandnpm run lintpass.vitest --project uniton the SSO, processes, proxy, claude-code-otlp, hook-routing and agents/core tests passes (93 files, 1406 tests). Afternpm run build,vitest --project clionanalytics-auth-gateandproxy-daemon-lifecyclepasses (12 tests). - I also ran the built CLI on Windows.
codemie hookblocks (exit 2 with the new text; the OTLP path prints the block JSON) while the background sign-in process keeps running. The hook's stdout/stderr close without waiting for that process. A script path with spaces reaches the child intact. A prompt sent within 30 s of a sign-in process that exited gets the "previous sign-in attempt did not complete" text. - Checked against EPMCDME-15378: AC1, AC2, AC4 and AC5 are met in the code and tests, apart from the spawn-failure path in the thread below. For AC3 the child runs the same
CodeMieSSO.authenticateas before, which stores the credentials and clears the invalid-auth marker. I didn't re-run it here because it opens a real browser. The verification comment in Jira tested8e103a4. The two lock commits after it (e757ce5e,c8d9232b) are covered by unit tests only. The ticket's "to check after merge" item (a real Claude Code session with a sign-in slower than 30 s) is still open. isProcessAlivemoved out ofproxy/daemon-manager.ts, buttests/integration/vscode-models.live.test.ts:29still imports it from there. That test is opt-in (CODEMIE_VSCODE_LIVE=1), so CI doesn't catch it. When no URL/key env is set, its "running proxy" branch would fail withisProcessAlive is not a function. Consider importing it fromsrc/utils/processes.js, asproxy-daemon-lifecycle.test.tsnow does.- Feature flag: none. The new behaviour applies to every prompt with an invalid SSO session, a login URL and no API key. Revert: code only. The old code ignores a leftover
~/.codemie/sso-login.lock, and a sign-in process started before a revert ends on its own within 120 s. - Open PRs touching the same files: #613 conflicts with this PR in the import block of
connect-orchestrator.ts(the movedisProcessAliveimport), so whichever merges second needs a small rebase. #639 changes the re-login hint tocodemie profile loginacross the CLI, which matches the new messages here; it merges cleanly.
What needs rechecking after the next push:
- a failed spawn of the sign-in process still blocks the prompt (exit 2 / block JSON);
- CI on the new head.
| } | ||
|
|
||
| const pid = spawnDetached(process.execPath, [script, '--url', codeMieUrl], { windowsHide: true }); | ||
| if (pid <= 0) { |
There was a problem hiding this comment.
A failed spawn of the sign-in process crashes the hook, and the prompt goes through
Problem: startDetachedSsoLogin handles pid <= 0 from spawnDetached as a failed start (line 137) and returns 'failed'. But spawnDetached in src/utils/processes.ts doesn't attach an 'error' listener to the child. When Node can't start the process, spawn returns a child without a pid and emits 'error' on the next tick. With no listener, that is an uncaught exception, and it fires while this function is still awaiting removeLock. The 'failed' result and the block decision are never produced.
I ran the built CLI with CODEMIE_PROVIDER=ai-run-sso, a CODEMIE_URL and an empty CODEMIE_HOME, plus a preload that makes only the sso-login spawn fail. Both codemie hook and codemie hook --agent claude-code-otlp exit with code 1 and a Node stack trace on stderr. There is no block message and no {"decision":"block"}. With the spawn succeeding, the same setup exits 2 with the block text, or prints the block JSON on the OTLP path. The unit test "fails and removes the lock when spawn returns -1" mocks spawnDetached, so it doesn't see this.
Impact: Claude Code treats exit code 1 as a non-blocking hook error. The prompt then reaches the model without a valid SSO session, which is the bypass this ticket closes, on a rarer path. The trigger is the OS refusing to create the process (process or handle limits, a security product blocking the child), and while that lasts every prompt gets through.
Recommendation: Consider adding a child.on('error', ...) listener in spawnDetached that logs at debug level, like the one exec() in src/utils/exec.ts (line 104) has for its child. A failed spawn then lands in the existing pid <= 0 → 'failed' branch, and spawnDaemon gets the same protection. A test that calls the real spawnDetached with a command that doesn't exist, and expects the gate to block with the "Could not open the browser sign-in automatically" text, would cover it.
There was a problem hiding this comment.
Checked on d3e346ce. With only the sso-login spawn redirected to an executable that doesn't exist, the built CLI now blocks: codemie hook exits 2 with "Could not open the browser sign-in automatically. Run codemie profile login, then re-send your prompt.", and codemie hook --agent claude-code-otlp prints {"decision":"block",...} with the same text (3 of 3 runs each, no lock file left behind). On c8d9232b both exited 1 with an unhandled 'error' event. The new test in processes-detached.test.ts fails with the listener removed and passes with it. It calls the captured handler on a mocked child rather than the real spawnDetached, so the end-to-end path is covered only by the manual run above.
…art still blocks When the OS refuses to create the process (ENOENT, EACCES, EAGAIN, EMFILE), spawn() returns a child without a pid and emits 'error' on the next tick. spawnDetached had no listener, so the error was uncaught and the hook exited with code 1, which Claude Code treats as non-blocking: the prompt reached the model without a valid SSO session. Log the error at debug level instead, so the caller sees pid -1 and startDetachedSsoLogin returns 'failed', which blocks the prompt with the manual sign-in hint. The proxy and codebase daemon managers get the same protection. Also import isProcessAlive from utils/processes in the opt-in vscode-models live test, since it moved out of proxy/daemon-manager. Generated with AI Co-Authored-By: codemie-ai <codemie.ai@gmail.com>
|
@Bondimm I pushed the fix to your |
|
Approve. Will be deployed after code freeze |
The UserPromptSubmit auth gate awaited the browser sign-in (up to 120 s) before returning the block decision. Claude Code kills the hook after its timeout and a killed hook does not block, so the prompt reached the model.
The gate now starts the sign-in in a detached process (sso-login) and returns the block decision right away. A lock file prevents concurrent sign-ins and is removed by the child after a successful sign-in. The block message reflects whether a sign-in was started, is already in progress, or is cooling down.
Refs: EPMCDME-15378
Test result
Checklist