fix(ai): derive the auth-broker presentation sidecar default at construction - #4819
Conversation
0475ae0 to
130876b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0475ae0123
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const target = authPath(); | ||
| await Bun.write(target, JSON.stringify(storage, null, 2)); |
There was a problem hiding this comment.
Pin the auth path across an OAuth refresh
When an E2E OAuth credential needs refreshing and the trusted home changes while getOAuthApiKey() is awaited, loadAuthStorage() reads profile A but this second authPath() resolves profile B, causing the complete storage snapshot from A to be written into B's testauth.db. This recreates the cross-profile credential-coherence problem during a single operation; resolve the path once at the start of resolveApiKey() and use that same path for both loading and saving.
Useful? React with 👍 / 👎.
|
Maintainer self-review risk record for exact head 130876b (base b30ca75): gajae.pr-self-review.v1 verdict:merge-self-approved base:b30ca75c4d1f6bfbcd14db0bc6df592c931e02b3 head:130876b19cbedd3a0a243acecb288a5df838b0e9 sha256:707ab330a1c5f60cfd96ad908160ec7fa00bb2beffd51690fed754ca15a72929 reviewer-id:Yeachan-Heo risk:low-risk extra:none evidence:out-of-process probe repro at dev head; regression + auth-broker/auth-gateway suites green at 130876b Rationale for low-risk: the only semantic change is when the sidecar default is derived (construction vs import); for a process whose home never changes after load — the overwhelmingly common path — behavior is byte-identical, and explicit presentationPath options are untouched. Verified: regression fails on base and passes at head; auth-broker (39) + auth-gateway (83) + helper-dependent (17) suites green; packages/ai tsc + biome clean. self-review-signature: sha256:5a1c7e12c924036abcf2862243652bd0cb7b85833e2d5d0b09460b9ca72ecd07 Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Maintainer self-review risk record for exact head 130876b (base b30ca75). Supersedes the earlier record whose digest was computed over a shell-mangled diff. gajae.pr-self-review.v1 verdict:merge-self-approved base:b30ca75c4d1f6bfbcd14db0bc6df592c931e02b3 head:130876b19cbedd3a0a243acecb288a5df838b0e9 sha256:753c86061cce16d9cf4626cee1274efa177182f9eba67cbf19e34bf0f168208b reviewer-id:Yeachan-Heo risk:low-risk extra:none evidence:out-of-process probe repro at dev head; regression + auth-broker/auth-gateway suites green at 130876b Rationale for low-risk: the only semantic change is when the sidecar default is derived (construction vs import); for a process whose home never changes after load — the overwhelmingly common path — behavior is byte-identical, and explicit presentationPath options are untouched. Verified at the exact head: regression fails on base and passes at head; auth-broker (39) + auth-gateway (83) + helper-dependent (17) suites green; packages/ai tsc + biome clean. self-review-signature: sha256:9a1d786a8372bb805a978ebb53c4fd8f8086dc444aa632d203ba46c9ba18d61a Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
130876b to
4c1a24d
Compare
|
Maintainer self-review risk record for exact head 4c1a24d (base e73212b, after rebasing onto advanced dev). Supersedes all earlier records on this PR. gajae.pr-self-review.v1 verdict:merge-self-approved base:e73212bfb19232f920630c049761cc2c7d39e15f head:4c1a24d31c162b6e49dcbb1d8b0f7b2881535a36 sha256:001085638e3701bc7ecb04135a86399639434151799ef7c6cc7da64f2920a293 reviewer-id:Yeachan-Heo risk:low-risk extra:none evidence:rebased onto dev e73212b; regression + auth-broker(39)/auth-gateway(83) suites green at 4c1a24d; packages/ai tsc clean Rationale for low-risk: the only semantic change is when the sidecar default is derived (construction vs import); for a process whose home never changes after load — the overwhelmingly common path — behavior is byte-identical, and explicit presentationPath options are untouched. Verified at this exact head: regression fails on base and passes at head; auth-broker (39) + auth-gateway (83) suites green; packages/ai tsc clean. self-review-signature: sha256:6a8c13e697b36054badc9fc0cc91c9bf196511af72588f6e54aae8fe31065a75 Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
4c1a24d to
fb26d0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb26d0ce7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function tempDir(): string { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gjc-auth-broker-home-")); | ||
| tempDirs.push(dir); | ||
| return dir; |
There was a problem hiding this comment.
Use asynchronous directory operations in the test
This newly added test uses mkdtempSync here and rmSync in afterEach, even though the repository contract requires node:fs/promises for directory operations. Make tempDir and the cleanup hook asynchronous and await fs.mkdtemp/fs.rm so this regression test does not block the shared Bun test runner.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
…ruction The trusted config root is call-time state since #4761/#4772, so a module-level constant built from getConfigRootDir() at import kept pointing at the home in effect when the module first loaded. A process whose home was established or changed after load then read and wrote one logical profile through two different roots -- the same split that made user-scope skills and MCP disappear, one layer out, with credential impact because <configRoot>/.env is one of the files $credentialEnv treats as trusted. Derive the default when the store is constructed instead; explicit presentationPath options are unchanged. Lore-id: 4786a1 Constraint: getConfigRootDir stays call-time; no consumer may pin an import-time root Tested: bun test packages/ai/test/auth-broker-remote-store-home.test.ts (out-of-process probe fails on dev head, passes on this change) Tested: bun test auth-gateway + auth-broker suites in packages/ai (83 + 39 pass) Confidence: high Scope-risk: narrow Reversibility: revert-commit
Same defect shape as #4786 in the E2E-only test helper: the agent dir is call-time state, so the module-level AUTH_PATH constant froze the home that was in effect at import. Lore-id: 4786a2 Tested: bun test auth-storage-api-key-login, kimi-headers-printable, synthetic-login, auth-no-borrow-env, auth-storage-broker-no-sentinel (17 pass) Confidence: high Scope-risk: narrow
fb26d0c to
4c45708
Compare
|
Maintainer self-review risk record for exact current head
Rationale for low-risk: the ordinary same-home path is byte-identical; only the timing of default derivation changes, explicit
Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Maintainer self-review risk record for exact current head
Rationale for low-risk: the ordinary same-home path is byte-identical; only the timing of default derivation changes, explicit
Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Maintainer self-review risk record for exact current head 4c45708 on immutable dev base bbbdb5f. gajae.pr-self-review.v1 verdict:merge-self-approved base:bbbdb5fcfa03a0fd6c7a8df1e4d7320420f50965 head:4c4570885261001dec15cac05a3d1dcaebebca94 sha256:b95ef61e8d65e68b4ce6df2451b26f76e74d6d033c019c3cce7c09d4199b5b82 reviewer-id:Yeachan-Heo risk:low-risk extra:none evidence:exact-current-dev replacement; home regression 1 pass; auth-broker 34 pass; auth-gateway 83 pass; helper suites 19 pass; packages/ai check and lint pass; five-file diff only Rationale for low-risk: the ordinary same-home path is byte-identical; only the timing of default derivation changes, explicit presentationPath options remain untouched, and the E2E helper uses the same call-time agent-dir resolver for both reads and refreshed writes. The out-of-process probe covers module import under temp home A, home switch to temp home B, and construction under B. Manual exact-head architecture, product, code, and adversarial review found no concrete blocker. self-review-signature: sha256:96466583e1f69c8c8268678ea9c81489efa413677f1056da240c36c54f3d73e5 Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
What
Fixes #4786.
This exact-current-dev replacement ports only the two issue-owned semantic commits from the immutable source branch onto live
origin/devbbbdb5fcfa03a0fd6c7a8df1e4d7320420f50965:packages/ai/src/auth-broker/remote-store.tsnow derives the default presentation sidecar path atRemoteAuthCredentialStoreconstruction time; explicitpresentationPathremains unchanged.packages/ai/test/oauth.tsnow derives the E2E-onlytestauth.dbhelper path at use time.The resulting diff is limited to five
packages/aipaths. No stale source-branch changes fromcrates/pi-natives,packages/tui, or any other unrelated path are present.Why
Since #4761/#4772, the trusted config root and agent directory are call-time state. The former module-level sidecar constant and test helper path outlived that authority, so a process whose home changed after module load could read and write one logical profile through different roots. The fix changes only derivation timing and preserves explicit path overrides.
Exact-head verification
Base:
bbbdb5fcfa03a0fd6c7a8df1e4d7320420f50965Head:
4c4570885261001dec15cac05a3d1dcaebebca94Change-set SHA-256 (
git diff --binary --full-index --no-ext-diff base...head):b95ef61e8d65e68b4ce6df2451b26f76e74d6d033c019c3cce7c09d4199b5b82bun test packages/ai/test/auth-broker-remote-store-home.test.ts— 1 pass; the out-of-process probe exercises the import-time-home regression.auth-storage-api-key-login,kimi-headers-printable,synthetic-login,auth-no-borrow-env,auth-storage-broker-no-sentinel) — 19 pass on current dev.bun --cwd=packages/ai run checkandbun --cwd=packages/ai run lint— pass; only two pre-existing informational diagnostics in an unrelated file.bun run check:tsreached repository SDK-closure checks but is blocked by unrelated environment/fixture failures: a transient file-lock failure (the isolated suite passes on rerun) andsdk-downgrade-rollback.test.tsfinding Bun1.3.14in a spawned helper while the pinned runtime is1.4.0. No touched-path check failed.Risk classification
low-risk— the ordinary same-home path is byte-identical; only default derivation timing changes, and explicitpresentationPathoptions are untouched.GJC exact-head verdict
dev—
[repo owner's gaebal-gajae (clawdbot) 🦞]\n\n