Repository navigation
fix: address review findings (stream timeouts, credential reader, login hardening, taste learning) - #165
fix: address review findings (stream timeouts, credential reader, login hardening, taste learning)#165patlux wants to merge 9 commits into
Conversation
… content The per-attempt timer aborts the attempt controller, so a body that stalls after the headers surfaced as an AbortError and was classified as a user cancellation: the host saw "Request aborted" and never retried. Classify it as a timeout before the user-abort check so it is retried when no content was emitted and reported as a timeout otherwise.
…orts converters.ts carried a second copy of the auth-file reader with a different precedence (~/.omp before ~/.pi) and without the string form of "command-code", so the generate fallback could stream with a different key than the Provider API path and the quota commands. Re-export the shared reader and honor PI_CODING_AGENT_DIR for the pi auth file so an isolated profile does not fall back to another profile's credentials.
… a deadline validateApiKey always called the production whoami endpoint and had no timeout, so /login validated against the wrong host under COMMANDCODE_API_BASE and hung on a stalled response. Derive the alpha base the same way index.ts does and race the request against a 15s deadline that also covers fetch implementations ignoring the abort signal.
The callback server settled the pending login on any error payload before checking the state token, so a local client or a page on the trusted http://localhost:3000 development origin could cancel a browser login with an arbitrary message. Validate the state first and drop the development origin from the allow list.
The generate request sent x-taste-learning: true unconditionally, even with CMD_ZDR=1, mirroring the CLI's default although this extension never reads the learned profile back. Send false unless COMMANDCODE_TASTE_LEARNING=1 is set and always false under ZDR, and document the working directory, project slug, and session id the request carries.
The router's reset() existed but was never wired, so an account upgraded from Go stayed on the generate fallback until the key changed or the host restarted. Reset the probe from the user's explicit refresh command.
commandCodeCostRatesAt rebuilt the table from the four base rates during DeepSeek V4 peak windows, dropping tiers on the Provider API path. Keep the full table and double tiers alongside the base rates.
The /commandcode-status stream warning was set once and never cleared, so a single malformed reasoning event stayed visible until restart.
runtime.ts kept its own, narrower redaction patterns next to redactCommandCodeErrorText. Strip URL queries as before, then delegate to the shared redactor so both surfaces agree.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Memory benchmarkCompared base
Interpretation: No metric shows a clear Base-to-PR difference beyond its measured run-to-run variation. Extension overhead above pi baseline
Values are medians of 6 alternating, paired runs. The value after Environment: pi
|
karaaslanz
left a comment
karaaslanz
left a comment
There was a problem hiding this comment.
Independent review requested via GitHub notifications. I targeted the high-impact OAuth callback-state fix at exact PR head 8a2dc7310d275665baa96c0a4c2fb14547ed2d33, not all 17 modified files.
I fetched and pinned the actual src/auth-server.ts source from base 3e43a7c787d3e6263a98fafa7b962de453f39e08 (blob 9e76149a0acd5dc7fa86cc992ff1e06fc98eb19c) and PR head (blob 284cf92ffe73cb269f3a470b4f3179fb28a26e8d). With Node 24 TypeScript stripping, I executed the actual startAuthServer() implementation from both commits on ephemeral 127.0.0.1 ports. Only synthetic credentials and state tokens were used; no actual login, paid API, or user secrets.
7/7 pinned-source behavior assertions PASS:
- Base responds HTTP 200 to an attacker-like
{ error: 'access_denied' }callback without the required state and settles the pending login with the forged cancellation. - PR responds HTTP 403 to a missing state and to a mismatched state, without closing the pending login server.
- PR rejects
Origin: http://localhost:3000with HTTP 403 even when a state is present. - PR accepts a valid state-matched callback with synthetic user/key fields (HTTP 200 and the expected resolved result).
- Both versions still honor a legitimate state-matched cancellation error with HTTP 200 and a rejected login promise.
The test script is stored read-only to upstream as pi-165-oauth-state-callback-base-vs-pr-probe.cjs (SHA-256 e07d9dc90db95b6119299b356985d272d4633849d88f92e45782ac952e691f5c), and the pinned source harness completed without repository edits. Head GitHub CI, Memory benchmark and catalog sync checks report success.
Scope/merge note: These are real isolated loopback HTTP behaviors, not a full Pi/OMP authentication E2E test or review of all nine fix groups. This PR still contains fixes for the internal Generate transport while separately requested breaking PR #164 removes Generate from the registered provider. Please account for that relationship when ordering/rebasing the branches, so security fixes not tied to Generate survive and redundant Generate-only work does not complicate the breaking release. I found no issue in the specific state-validation paths tested.
pierreraby
left a comment
pierreraby
left a comment
There was a problem hiding this comment.
Review — head 8a2dc731 (9 commits, base 3e43a7c)
Verified independently at this exact head in an isolated worktree (npm ci --ignore-scripts, no pi config touched): npm run typecheck, npm run format:check and git diff --check clean, full npm test green including the real-host suites ([pi-local] PASS against pi 1.3.0, [omp-compat] PASS). GitHub CI on this head is green as well (14 success, 1 skipped). I also re-ran the new regression tests against the pre-fix src/ (head tests, base sources): test-retry.ts 3 red, test-stream.ts 2 red, test-cost.ts 1 red — the timeout reclassification, the taste-learning default and the peak-tier fix are genuinely covered.
The fixes are correct and well placed:
a918138(security): the state token is checked before every settle path, including error payloads and before the field validation, and every non-settling path (403, malformed body, oversized body, disallowed origin, OPTIONS, 404/405) was checked — I found no remaining unauthenticated settle, and the403body echoes nothing.http://localhost:3000is out of the allowlist and the 120 s timeout → paste fallback is intact.cef7402: the per-attempt timer aborts onlyattemptController, so the newcontroller.signal.abortedline cannot misclassify a genuine user abort; the retry stays gated onoutput.content.length === 0, and every stream-visible event (text, reasoning delta, tool call) also lands inoutput.content, so a retry cannot duplicate visible output.35a774b: exactly one ambient reader is left (src/api-key.ts, re-exported throughconverters.ts/core.ts), with no missed call sites, the"command-code"string form andPI_CODING_AGENT_DIRare covered by tests, and the precedence change on the generate path is documented.8a2dc73: the shared redactor is a strict superset of the inline patterns it replaces, and the URL query stripping is preserved.188a344,c6c8842,0025369: minimal and correct; the?/??base derivation matchesindex.tsand the re-probe uses the same router instance.
Non-blocking notes, in the order I would address them:
- Error-callback contract (worth one live check before release).
stateis now required to honor an error payload. The base code only proves the Studio echoesstateon success callbacks. If the Studio omits it on a user-denied or server-error callback, a cancelled browser login no longer settles immediately and the user falls through the 120 s timeout to the paste prompt — a safe fallback, but degraded. A real browser-login cancellation against production would settle it. - CHANGELOG over-claims profile isolation. Only the pi slot is redirected by
PI_CODING_AGENT_DIR;~/.commandcode/auth.jsonand~/.omp/agent/auth.jsonstill resolve fromhomedir(), so a sandbox that setsPI_CODING_AGENT_DIRwithoutHOMEcan still pick up another profile's.omp/.commandcodekeys. "never falls back to another profile's pi credentials" would be accurate, and the test only proves the pi slot. - README over-states the warning reset.
streamWarningis cleared at the start of the next Responses request (index.ts), not "once a later request streams cleanly"; a later clean generate request leaves it up, and two concurrent Responses requests share one slot. - PR description verification counts.
tests/test-oauth.tsgained six tests: one is a preservation guard that is green on base, two hang on base (no deadline) rather than "failing", so "2 intest-oauth.ts" is not reproducible as written (the head test file also cannot importvalidationApiBasefrom the base module). The new tests intest-api-key.ts,test-cost.ts,test-runtime.ts,test-stream.tsare not mentioned. x-taste-learningunder ZDR....options?.headersis spread after the taste header, so a caller-suppliedx-taste-learning: "true"still wins underCMD_ZDR=1, which contradicts "alwaysfalseunderCMD_ZDR=1" in the README and CHANGELOG. Either set the header after the spread / drop caller overrides under ZDR, or qualify the docs.index.tsis the only touched file without a test (the warning reset and theresetTransportwiring); it is also outsidenpm run typecheckby design, as the PR honestly discloses. Real-host CI is the only guard today.validationApiBaseuses||whereindex.tsuses??:COMMANDCODE_API_BASE=""sends/loginkey validation to production while the chat transports use a relative base.- Hardening nits:
state !== options.expectedStateis not a constant-time comparison (impractical over localhost in the 120 s window), and anundefinedexpectedStateskips the check entirely — production always sets it, but failing closed would be more defensive. - Pre-existing, surfaced by the wiring: a request that observed
upgrade_requiredbefore/commandcode-refreshcan re-stickgenerateif its assignment lands after the reset (theapiKey === requestApiKeyguard holds again); andattemptTimedOutcan mask a genuine stream-error message that arrives at the timeout boundary. Both cosmetic.
Approving: the changes do what their commit messages claim, the security fix is correctly placed, and no P0/P1 code defect was found. The notes above are documentation accuracy and hardening follow-ups; item 1 is worth a live browser check before the next release.
Reviewer context: independent read-only review plus a full local run at the exact head; struck at 8a2dc7310d275665baa96c0a4c2fb14547ed2d33.
|
Hi @patlux — with #157 and #167 now merged, the remaining stack (#158, #159, #162, #163, #164) is waiting on your call, and I'd rather ask than guess:
One heads-up unrelated to the stack: the |
Summary
Fixes from a full review of the extension, one commit per finding (most severe first).
fix(core): report stalled-body timeouts …timeoutMsthat fired while the response body was stalled surfaced asAbortErrorand was classified as a user cancellation:reason: "aborted","Request aborted", never retried. The existing retry test only covered the fetch phase (the mock never flushes headers forevents: []).timed out after Nms. Regression tests for both paths.fix(auth): read ambient credentials through one reader …src/converters.tscarried a second copy of the auth-file reader with a different precedence (~/.ompbefore~/.pi) and without the"command-code": "user_…"string form, so the generate fallback could stream with a different key than the Provider API path and the quota commands. Both readers usedhomedir()regardless ofPI_CODING_AGENT_DIR.src/api-key.ts), re-exported for the generate path; README order everywhere; pi auth file read fromPI_CODING_AGENT_DIRso isolated profiles never fall back to another profile. Test asserts both entry points are the same function.fix(oauth): validate pasted keys against the configured API base …validateApiKeyalways called production and had no timeout: wrong host underCOMMANDCODE_API_BASE, and a stalledwhoamihung/login(quota got a deadline in #152, this path did not).index.ts; 15 s deadline that also covers fetch implementations ignoring the signal.fix(oauth): require the state token before honoring callback errors{error}payload before checkingstate, so any local client — or any page on the trustedhttp://localhost:3000dev origin — could cancel a browser login with an arbitrary message.403and the server keeps waiting.http://localhost:3000removed from the allow list.fix(core): make taste learning opt-in on the generate transportx-taste-learning: truewas sent unconditionally (also underCMD_ZDR=1) and undocumented. The official CLI sends itstasteLearningsetting; this extension never reads the learned profile back.falseby default,COMMANDCODE_TASTE_LEARNING=1opts in, alwaysfalseunder ZDR. README documents the working dir / project slug / session id the generate request carries.fix(core): re-probe the transport on /commandcode-refreshtransport.reset()was dead code; an account upgraded from Go stayed on the generate fallback until key change or restart.fix(cost): keep context tiers when applying peak ratestierson the Provider API path (latent; no time-priced model has tiers today).fix(core): scope the stream warning to the latest Responses request/commandcode-statusstream warning was set once and never cleared.refactor(runtime): reuse the shared credential redactor for diagnosticsredactCommandCodeErrorText.Behavior changes to be aware of
x-taste-learningnow defaults tofalseon the generate transport (opt in withCOMMANDCODE_TASTE_LEARNING=1).statetoken; otherwise the server keeps waiting and the normal timeout/paste fallback applies.http://localhost:3000is no longer an accepted callback origin.Deliberately left out
index.tsis not covered bynpm run typecheck(tsconfig.jsonincludeomits it, andtests/test-package-manifest.tsintentionally forbids pi packages as devDependencies). Checking it against the real pi 1.3.0 types yields 18 errors from structural differences betweensrc/types.ts(*Liketypes) and pi-ai (JsonObjecttool arguments,AssistantMessageEventStreamclass vs. the*Likeinterface,SimpleStreamOptions,OAuthCredentialsindex signature, readonly cost tiers). Aligning those is a separate type-alignment task with OMP-compat trade-offs; the real-host runs in CI remain the guard for now.isRecordis duplicated across seven modules; left as is to keep this PR reviewable (no behavior impact).Verification
npm run typecheck,npm run format:check,git diff --check: clean.npm test: all suites green, includingtests/test-pi-local.mjsagainst a realpi1.3.0 binary ([pi-local] PASS).tests/test-omp-compat.mjsskipped locally (noomp); runs in CI.test-retry.ts, 2 intest-oauth.ts) and green after.pi --no-extensions -e ./index.ts --list-models commandcodelists the catalog.