Remove shims - #11
Conversation
Drop the text-only image description path now that Composer 2.5 supports images natively, and clean related config, docs, and tests.
Why: - Grok models now consume Pi native tool schemas and no longer require Cursor-compatible aliases. What: - Remove compatibility tool registration, scoping, delegation, and related dependencies. - Keep image_gen preference synchronization across model and session changes. - Remove obsolete shim documentation and tests. Validation: - bun run check (263 tests)
Why: - Keep one Grok CLI provider while separate Pi sessions use separate logged-in accounts. What: - Store account credentials in the extension vault and route each request through the session selection. - Migrate only released main-branch grok-cli-N credentials and saved model settings. - Protect login, quota, and file updates from concurrent session races. Validation: - bun run check
Why: - The hover glow ring drew round corners over a squircle card, so the ring separated from the card edge at each corner. What: - Let the ring pseudo-element inherit corner-shape, not only border-radius. Validation: - bun run check
📝 WalkthroughWalkthroughThe PR replaces legacy provider, vision, web-search, and compatibility-tool systems with a versioned account vault, session-scoped routing, migration support, native image input, and unified ChangesAccount vault and migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant AccountRoute as Account Route Resolver
participant AccountVault as Account Vault
participant GrokProvider as Grok Provider
Session->>AccountRoute: Resolve session account
AccountRoute->>AccountVault: Read account credentials
AccountVault-->>AccountRoute: Return account snapshot
AccountRoute-->>GrokProvider: Return token and base URL
GrokProvider->>GrokProvider: Stream Grok request
GrokProvider-->>Session: Associate response with account
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR removes compatibility tool and vision-routing shims while consolidating legacy provider aliases into one
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect remaining after reviewing migration, request routing, rotation, and dashboard security paths. The new vault writes are locked and atomic, migration retries preserve recoverability, session requests capture account ownership, and dashboard mutations retain capability, same-origin, and CSRF enforcement. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Legacy v1/v2 configuration] --> B[Acquire migration lock]
B --> C[Copy OAuth credentials into account vault]
C --> D[Preserve legacy configuration backup]
D --> E[Migrate quota keys to account IDs]
E --> F[Write v3 non-account configuration]
F --> G[Rewrite saved provider aliases]
G --> H[Register single grok-cli provider]
H --> I[Restore per-session account selection]
I --> J[Resolve and capture request account]
J --> K{Balance exhausted?}
K -- No --> L[Complete request]
K -- Yes --> M[Select eligible vault account]
M --> J
Reviews (1): Last reviewed commit: "fix(dashboard): match hover ring corners..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e96576c8ca
ℹ️ 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".
| if (selected) latest.activeAccountId = selected.id; | ||
| }); | ||
| } | ||
| if (Date.now() < account.credential.expires - 120_000) { |
There was a problem hiding this comment.
OAuth login and refresh already store expires 120 seconds early in src/auth/oauth.ts, but this condition subtracts another 120 seconds. Normal credentials are therefore refreshed four minutes before their real expiry, and tokens lasting four minutes or less trigger a refresh on every route resolution, increasing latency and exposing otherwise valid requests to refresh failures. Treat the stored expiry as already skewed, or remove the skew when credentials are created.
Useful? React with 👍 / 👎.
| job.resolveManualCode(''); | ||
| if (removeOnFailure) { | ||
| newAccountIds.delete(id); | ||
| await manager.remove(ctx, id).catch(() => undefined); |
There was a problem hiding this comment.
When two Pi/dashboard processes start the first login for a newly added account, one can successfully store the credential and increment its revision while the creator's attempt subsequently fails the revision check. Because removeOnFailure was captured when the account was created, this unconditional cleanup then deletes the account and the credential saved by the successful process. Remove it only if the account is still unauthenticated and unchanged from its creation revision.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (14)
tests/stateTestHelpers.ts (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
deferredto hold one resolver.The
Promiseexecutor runs synchronously, so the array always holds exactly one resolver. A single variable states the intent more directly and removes the optional-call fallback.♻️ Proposed simplification
export function deferred<T>() { - const resolvers: ((value: T) => void)[] = []; - const promise = new Promise<T>((resolve) => resolvers.push(resolve)); - return { promise, resolve: (value: T) => resolvers[0]?.(value) }; + let resolve!: (value: T) => void; + const promise = new Promise<T>((resolver) => { + resolve = resolver; + }); + return { promise, resolve }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stateTestHelpers.ts` around lines 20 - 24, Update deferred to store its Promise resolver in a single variable rather than the resolvers array, then have the returned resolve function call that resolver directly without optional chaining. Preserve the existing promise and generic value behavior.tests/provider/accounts.test.ts (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
useEnvironmentTokenfor token teardown.Lines 53 and 147-150 reimplement the save-and-restore logic that
useEnvironmentTokenintests/stateTestHelpers.tsalready provides. The other suites in this PR use that helper. Reusing it removes the duplicated teardown and keeps one behavior for the variable.♻️ Proposed consolidation
import { deferred, oauthCredential, setAccount1Credential, + useEnvironmentToken, useTempHome, } from '../stateTestHelpers.js';-const originalToken = process.env.GROK_CLI_OAUTH_TOKEN; const setupHome = useTempHome(); +const setToken = useEnvironmentToken();beforeEach(() => { setupHome(); - delete process.env.GROK_CLI_OAUTH_TOKEN; + setToken(undefined);-afterEach(() => { - if (originalToken === undefined) delete process.env.GROK_CLI_OAUTH_TOKEN; - else process.env.GROK_CLI_OAUTH_TOKEN = originalToken; -});Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider/accounts.test.ts` at line 53, Replace the manual GROK_CLI_OAUTH_TOKEN save-and-restore logic around the affected test setup and teardown with the existing useEnvironmentToken helper from tests/stateTestHelpers.ts. Update both the originalToken declaration and the cleanup at lines 147-150, preserving the current token setup behavior while delegating restoration to the shared helper.tests/provider/sessionAccountSelection.test.ts (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIsolate
GROK_CLI_OAUTH_TOKENin this suite.
accountIdinsrc/provider/sessionAccountSelection.tsreturnsACCOUNT_1_IDimmediately whenGROK_CLI_OAUTH_TOKENis set. If that variable exists in the developer environment, the first two tests fail for an unrelated reason.useEnvironmentTokenfromtests/stateTestHelpers.tsclears and restores the variable.♻️ Proposed isolation
-import { useTempHome } from '../stateTestHelpers.js'; +import { useEnvironmentToken, useTempHome } from '../stateTestHelpers.js'; const setupHome = useTempHome(); +const setToken = useEnvironmentToken();beforeEach(async () => { setupHome(); + setToken(undefined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider/sessionAccountSelection.test.ts` around lines 29 - 31, Update the session account selection test setup around beforeEach to use useEnvironmentToken from stateTestHelpers, ensuring GROK_CLI_OAUTH_TOKEN is cleared for the suite and restored afterward. Preserve the existing setupHome and mutateAccountVault initialization flow.tests/provider/register.test.ts (1)
269-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePi credential fixtures are duplicated across two test files. Both files write
.pi/agent/auth.jsonwith agrok-cliOAuth entry, and both build the same vault-marker credential. The shared root cause is a missing fixture helper intests/stateTestHelpers.ts, which this PR already introduces for other shared test state. AddwritePiCredentialand a marker helper there, then import them in both suites.
tests/provider/register.test.ts#L269-L285: remove localwriteAuthandwriteMarker, and import the shared helpers instead.tests/provider/accounts.test.ts#L103-L123: remove localwritePiCredentialandconnectPi, and import the shared helpers instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider/register.test.ts` around lines 269 - 285, The Pi credential fixtures are duplicated across both test suites. In tests/stateTestHelpers.ts, add shared writePiCredential and vault-marker helpers; in tests/provider/register.test.ts lines 269-285, remove local writeAuth and writeMarker and import the shared helpers; in tests/provider/accounts.test.ts lines 103-123, remove local writePiCredential and connectPi and import the shared helpers instead.src/provider/quotaCache.ts (1)
122-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept a
pathoverride for consistency.
saveQuotaUsageandremoveQuotaUsageaccept apathparameter, butsaveQuotaUsageWhenalways uses the default cache path. This blocks path-isolated tests for the conditional write path and makes the module API inconsistent.♻️ Proposed refactor
export function saveQuotaUsageWhen( accountId: string, usage: BillingUsage, shouldSave: () => boolean, + path = getQuotaCachePath(), ) { return updateQuotaCache((cache) => { if (!shouldSave()) return false; storeQuota(cache, accountId, usage, new Date().toISOString()); return true; - }); + }, path); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/quotaCache.ts` around lines 122 - 132, Update saveQuotaUsageWhen to accept an optional path override, pass it through to updateQuotaCache, and preserve the default path behavior when omitted so its API matches saveQuotaUsage and removeQuotaUsage.src/provider/accountVault.ts (2)
231-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the intermediate
vaultvariable.
currentis already the value to clone. Theletand the assignment add no behavior.♻️ Proposed change
export function getAccountVault() { - let vault: AccountVault | undefined; - return withVaultLock((current) => { - vault = current; - return structuredClone(vault); - }, false); + return withVaultLock((current) => structuredClone(current), false); }As per coding guidelines: "Reduce total variable count by inlining when a value is only used once" and "Prefer
constoverlet".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/accountVault.ts` around lines 231 - 237, In getAccountVault, remove the intermediate vault declaration and assignment, and return a structuredClone of the withVaultLock callback’s current value directly. Preserve the existing lock invocation and false argument.Source: Coding guidelines
117-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport a distinct reason when the vault root is not an object.
Both branches emit
unsupported version. A truncated or replaced file that parses to an array or a string then produces a misleading error.♻️ Proposed change
- if (!isObject(value)) return invalid('unsupported version'); + if (!isObject(value)) return invalid('the vault must contain a JSON object'); if (value.version !== 1) invalid('unsupported version');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/accountVault.ts` around lines 117 - 118, Update the validation around the vault root in the account-vault parsing flow: have the !isObject(value) branch report a distinct invalid-root/object error, while retaining unsupported version only for the value.version !== 1 branch.tests/provider/accountMigration.test.ts (1)
231-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a case for an unsupported config version.
readReleasedConfigthrows for any version other than 1, 2, or 3.src/provider/register.tsconverts that rejection intomigrationError, which blocks login and streaming. No test pins that contract. Add a case that writes{ version: 4 }and asserts the rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider/accountMigration.test.ts` around lines 231 - 267, Extend the migration tests around migrateReleasedAccounts to write a released configuration with version 4 and assert that the operation rejects with migrationError. Use the existing setup and configuration-writing helpers, and pin the unsupported-version behavior without changing handling for supported versions.tests/config.test.ts (1)
125-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe permission-based test can fail when the suite runs as root.
chmodSync(paths(home).pi, 0o500)does not block writes for uid 0. In a root container the migration succeeds andresult.warningisundefined. Skip the test whenprocess.getuid?.() === 0, or simulate the failure with a non-directory path astests/storage.test.tsLine 68 does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/config.test.ts` around lines 125 - 133, Make the “preserves a legacy file when the destination is not writable” test reliable under root by skipping it when process.getuid?.() === 0, or replace the chmod-based setup with the non-directory-path failure approach used in storage tests. Keep the existing migration warning assertion for environments where the failure is actually simulated.tests/storage.test.ts (1)
120-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the conditional branch and assert acquisition directly.
The barrier records a live pid with an epoch mtime, so
recoveryInProgressdeletes it and the first acquisition attempt succeeds.pendingis therefore already settled whenPromise.raceruns, and thequeueMicrotaskresolver loses. The!result.acquiredbranch is dead, yet Line 145 assertsresult.acquiredistrue. If microtask ordering ever shifts, the test takes the dead branch and then fails at Line 145.💚 Proposed simplification
- const pending = acquireFileLock(getQuotaCachePath()); - const result = await Promise.race([ - pending.then((release) => ({ acquired: true as const, release })), - new Promise<{ acquired: false }>((resolve) => { - queueMicrotask(() => resolve({ acquired: false })); - }), - ]); - if (!result.acquired) { - rmSync(recoveryPath, { force: true }); - await vi.advanceTimersByTimeAsync(25); - await (await pending)(); - } else { - await result.release(); - } - expect(existsSync(recoveryPath)).toBe(false); - expect(result.acquired).toBe(true); + const release = await acquireFileLock(getQuotaCachePath()); + await release(); + expect(existsSync(recoveryPath)).toBe(false); + expect(existsSync(lockPath)).toBe(false);This also removes the
elsestatement and the unusedrmSyncpath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/storage.test.ts` around lines 120 - 149, Update the test around acquireFileLock to await the pending lock acquisition directly, remove the Promise.race and queueMicrotask logic, and eliminate the conditional release/removal branch. Assert that the recovery barrier is absent after releasing the acquired lock, preserving cleanup of fake timers in the existing finally block.Source: Coding guidelines
tests/provider/package.test.ts (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant dependency assertions.
Line 42 asserts that
dependenciesis absent. Thejitiandtypeboxchecks on lines 40-41 can never fail after that, so they add noise.♻️ Proposed refactor
- expect(packageJson.dependencies?.jiti).toBeUndefined(); - expect(packageJson.dependencies?.typebox).toBeUndefined(); expect(packageJson.dependencies).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider/package.test.ts` around lines 40 - 43, Remove the redundant jiti and typebox dependency assertions from the packageJson checks, keeping the dependencies absence assertion and the existing devDependencies assertion in the relevant package test.src/provider/dashboard/app.js (1)
785-787: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename provider-based identifiers to account identifiers.
These variables now hold account IDs, but the names still say
provider(pendingProviders, theproviderloop variable, anddataset.providerat line 675). The mismatch makes the ID contract harder to follow after this migration. Rename them topendingAccountIds,accountId, anddataset.accountId, and update the matching selectors at lines 769-775 and 816-823.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/dashboard/app.js` around lines 785 - 787, Rename the account-ID variables throughout the relevant flow: change pendingProviders to pendingAccountIds, the loop variable provider to accountId, and dataset.provider to dataset.accountId. Update all matching references and selectors, including the sections around the pending-account selection and subsequent lookup logic, while preserving behavior.src/provider/accounts.ts (2)
363-366: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSelect the environment account by ID.
The environment path refreshes
accounts[0]positionally.resolveAccountRoutereports the environment route asACCOUNT_1_ID, andsnapshotmarks the environment account byaccount.id === ACCOUNT_1_ID. If the vault order ever differs from that assumption, the quota is stored under the wrong account. Select by ID to keep all three code paths consistent.♻️ Proposed refactor
const accounts = environment - ? [getAccountVaultSync().accounts[0]].filter(Boolean) + ? getAccountVaultSync().accounts.filter((account) => account.id === ACCOUNT_1_ID) : getAccountVaultSync().accounts.filter((account) => account.credential);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/accounts.ts` around lines 363 - 366, Update the environment branch in the account selection flow to choose the account whose id matches ACCOUNT_1_ID instead of using accounts[0]. Preserve the existing filtering behavior and leave the non-environment credential filtering unchanged, keeping it consistent with resolveAccountRoute and snapshot.
234-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or repurpose the unused Grok account shims.
resolveGrokProviderandhandleModelSelectare no-op helpers; runtime code only callsaccountManagement.handleModelSelect(), and tests only import this surface as part of the account export. Replace them with a shared no-op/no-op implementation if the contract is needed, or remove the exports and dependent imports/tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/accounts.ts` around lines 234 - 236, Remove or repurpose the no-op account shims resolveGrokProvider and handleModelSelect in the accounts export: either provide one shared no-op implementation where the contract requires it, or delete these exports and update all dependent imports and tests, preserving the runtime accountManagement.handleModelSelect() path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 179-200: Update the legacy cleanup logic in the
consolidated-config branch so a recognized legacy Imagine setting is not
discarded when the consolidated config omits the imagine key. Before unlinking
legacyPath, either persist the legacy value into the consolidated configuration
or retain the legacy file when the raw parsed config does not define imagine;
preserve consolidated-config precedence when the key is present.
In `@src/provider/accountMigration.ts`:
- Around line 141-163: Make migrateQuota asynchronous and acquire the same
quota-cache lock used by updateQuotaCache via
acquireFileLock(getQuotaCachePath()) before reading the cache, holding it
through the migration rewrite and releasing it afterward. Preserve the existing
validation and migration behavior, and update the call site to await
migrateQuota(accountIds).
In `@src/provider/dashboard/server.ts`:
- Around line 388-395: Update the rejection handler around the job state
assignment and account cleanup so removal occurs only when removeOnFailure is
true and controller.signal.aborted is false. Preserve the cancelled state and
account row for explicit login cancellation while retaining cleanup for other
failures.
In `@src/provider/modelMigration.ts`:
- Around line 8-11: Update migrateSettings so SettingsManager.create receives
directory as the real cwd and agent directory, ensuring flush() persists changes
to directory/settings.json; preserve the existing migration flow and avoid using
the temporary .pi-grok-cli-migration path as the cwd.
In `@src/provider/sessionAccountSelection.ts`:
- Around line 35-46: Make restore in the session account selection
implementation safe when invoked detached by removing its this-dependent
accountId call, or by binding restore at every callback registration such as
sessionSelection.on or pi.on. Prefer directly deriving the session ID from
ctx.sessionManager when calling accountId, while preserving the existing
selection restore and deletion behavior.
In `@src/provider/usage.ts`:
- Around line 34-38: Update the route resolution handling around resolveRoute so
its caught error message is preserved and included in the user notification
alongside the existing formatQuota output. Keep the no-route return behavior,
and ensure the actionable reason from resolveAccountRoute is surfaced instead of
being discarded.
In `@src/storage.ts`:
- Around line 30-49: Update abandonedLock in src/storage.ts#L30-L49 to parse and
validate the lock owner before applying the mtime threshold; for a valid PID,
return !processIsRunning(pid), using LOCK_STALE_MS only when the owner cannot be
identified. Update the corresponding test case in tests/storage.test.ts#L96-L118
to use a non-running PID or rename it to explicitly describe the bounded mtime
behavior.
- Around line 113-141: Update the acquisition loop around recoveryInProgress to
check the elapsed timeout once at the start of every iteration, before sleeping
or attempting the lock. Reuse LOCK_STALE_MS for the deadline and preserve the
existing timeout error; remove the duplicate 30-second literal from the EEXIST
branch so recovery polling and lock contention share the same limit.
In `@tests/imagine/helpers.ts`:
- Around line 10-12: Update the cleanup call in the afterEach hook to pass
force: true to rmSync, ensuring missing temporary directories do not fail test
cleanup while preserving recursive removal.
In `@tests/provider/register.test.ts`:
- Around line 994-1008: Update the quota-cache assertion in the successful OAuth
login test to check the seeded `'account-1'` entry instead of `'grok-cli'`. Keep
the existing login invocation and success expectations unchanged, and verify
that loadQuotaCache().accounts['account-1'] is undefined after login.
- Around line 720-747: Update the concurrent stream assertion in the test around
streamSimple to avoid relying on mockProviderStream call order. Associate each
recorded API key with its session ID from the call arguments, then assert the
resulting mapping contains session-a → one and session-b → two while preserving
the existing stream behavior.
In `@tests/stateTestHelpers.ts`:
- Around line 41-54: Update the returned setup function in useTempHome to add
each mkdtempSync-created directory to the dirs collection before returning it,
so the existing afterEach cleanup removes all temporary directories.
In `@tests/storage.test.ts`:
- Around line 96-118: Update the stale-lock test around acquireFileLock to avoid
asserting mtime-first reclamation when the lock contains the current
process.pid. Make the test represent a genuinely stale or non-running owner
while still verifying recovery after the lock timeout, and preserve the release
and lock-file removal assertions.
---
Nitpick comments:
In `@src/provider/accounts.ts`:
- Around line 363-366: Update the environment branch in the account selection
flow to choose the account whose id matches ACCOUNT_1_ID instead of using
accounts[0]. Preserve the existing filtering behavior and leave the
non-environment credential filtering unchanged, keeping it consistent with
resolveAccountRoute and snapshot.
- Around line 234-236: Remove or repurpose the no-op account shims
resolveGrokProvider and handleModelSelect in the accounts export: either provide
one shared no-op implementation where the contract requires it, or delete these
exports and update all dependent imports and tests, preserving the runtime
accountManagement.handleModelSelect() path.
In `@src/provider/accountVault.ts`:
- Around line 231-237: In getAccountVault, remove the intermediate vault
declaration and assignment, and return a structuredClone of the withVaultLock
callback’s current value directly. Preserve the existing lock invocation and
false argument.
- Around line 117-118: Update the validation around the vault root in the
account-vault parsing flow: have the !isObject(value) branch report a distinct
invalid-root/object error, while retaining unsupported version only for the
value.version !== 1 branch.
In `@src/provider/dashboard/app.js`:
- Around line 785-787: Rename the account-ID variables throughout the relevant
flow: change pendingProviders to pendingAccountIds, the loop variable provider
to accountId, and dataset.provider to dataset.accountId. Update all matching
references and selectors, including the sections around the pending-account
selection and subsequent lookup logic, while preserving behavior.
In `@src/provider/quotaCache.ts`:
- Around line 122-132: Update saveQuotaUsageWhen to accept an optional path
override, pass it through to updateQuotaCache, and preserve the default path
behavior when omitted so its API matches saveQuotaUsage and removeQuotaUsage.
In `@tests/config.test.ts`:
- Around line 125-133: Make the “preserves a legacy file when the destination is
not writable” test reliable under root by skipping it when process.getuid?.()
=== 0, or replace the chmod-based setup with the non-directory-path failure
approach used in storage tests. Keep the existing migration warning assertion
for environments where the failure is actually simulated.
In `@tests/provider/accountMigration.test.ts`:
- Around line 231-267: Extend the migration tests around migrateReleasedAccounts
to write a released configuration with version 4 and assert that the operation
rejects with migrationError. Use the existing setup and configuration-writing
helpers, and pin the unsupported-version behavior without changing handling for
supported versions.
In `@tests/provider/accounts.test.ts`:
- Line 53: Replace the manual GROK_CLI_OAUTH_TOKEN save-and-restore logic around
the affected test setup and teardown with the existing useEnvironmentToken
helper from tests/stateTestHelpers.ts. Update both the originalToken declaration
and the cleanup at lines 147-150, preserving the current token setup behavior
while delegating restoration to the shared helper.
In `@tests/provider/package.test.ts`:
- Around line 40-43: Remove the redundant jiti and typebox dependency assertions
from the packageJson checks, keeping the dependencies absence assertion and the
existing devDependencies assertion in the relevant package test.
In `@tests/provider/register.test.ts`:
- Around line 269-285: The Pi credential fixtures are duplicated across both
test suites. In tests/stateTestHelpers.ts, add shared writePiCredential and
vault-marker helpers; in tests/provider/register.test.ts lines 269-285, remove
local writeAuth and writeMarker and import the shared helpers; in
tests/provider/accounts.test.ts lines 103-123, remove local writePiCredential
and connectPi and import the shared helpers instead.
In `@tests/provider/sessionAccountSelection.test.ts`:
- Around line 29-31: Update the session account selection test setup around
beforeEach to use useEnvironmentToken from stateTestHelpers, ensuring
GROK_CLI_OAUTH_TOKEN is cleared for the suite and restored afterward. Preserve
the existing setupHome and mutateAccountVault initialization flow.
In `@tests/stateTestHelpers.ts`:
- Around line 20-24: Update deferred to store its Promise resolver in a single
variable rather than the resolvers array, then have the returned resolve
function call that resolver directly without optional chaining. Preserve the
existing promise and generic value behavior.
In `@tests/storage.test.ts`:
- Around line 120-149: Update the test around acquireFileLock to await the
pending lock acquisition directly, remove the Promise.race and queueMicrotask
logic, and eliminate the conditional release/removal branch. Assert that the
recovery barrier is absent after releasing the acquired lock, preserving cleanup
of fake timers in the existing finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bfe9631-0021-4fda-aa6e-bf8c7f6a7c87
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
CONFIGURATION.mdREADME.mdSECURITY.mdknip.jsonpackage.jsonsrc/config.tssrc/imagine/register.tssrc/imagine/tool.tssrc/imagine/workflow.tssrc/models/catalog.tssrc/provider/accountMigration.tssrc/provider/accountRouting.tssrc/provider/accountVault.tssrc/provider/accounts.tssrc/provider/dashboard/app.csssrc/provider/dashboard/app.jssrc/provider/dashboard/server.tssrc/provider/modelMigration.tssrc/provider/quotaCache.tssrc/provider/register.tssrc/provider/requestOwnership.tssrc/provider/rotation.tssrc/provider/sessionAccountSelection.tssrc/provider/toolScope.tssrc/provider/usage.tssrc/storage.tssrc/tools/files.tssrc/tools/glob.tssrc/tools/read.tssrc/tools/register.tssrc/tools/rendering.tssrc/tools/search.tssrc/tools/shell.tssrc/tools/webSearch.tssrc/tools/webSearchDelegate.tssrc/vision/cache.tssrc/vision/describe.tssrc/vision/register.tstests/config.test.tstests/imagine/config.test.tstests/imagine/helpers.tstests/imagine/preview.test.tstests/imagine/register.test.tstests/imagine/save.test.tstests/imagine/tool.test.tstests/models/catalog.test.tstests/provider/accountMigration.test.tstests/provider/accountRouting.test.tstests/provider/accountVault.test.tstests/provider/accounts.test.tstests/provider/dashboard.test.tstests/provider/modelMigration.test.tstests/provider/package.test.tstests/provider/quotaCache.test.tstests/provider/register.test.tstests/provider/rotation.test.tstests/provider/sessionAccountSelection.test.tstests/provider/toolScope.integration.test.tstests/provider/toolScope.test.tstests/stateTestHelpers.tstests/storage.test.tstests/tools/files.test.tstests/tools/glob.test.tstests/tools/nativeAdapter.integration.test.tstests/tools/read.test.tstests/tools/register.test.tstests/tools/rendering.test.tstests/tools/search.test.tstests/tools/shell.test.tstests/tools/toolTestHelpers.tstests/tools/webSearch.test.tstests/tools/webSearchDelegate.test.tstests/tools/webSearchDelegateRetry.test.tstests/vision/cache.test.tstests/vision/config.test.tstests/vision/describe.test.tstests/vision/helpers.tstests/vision/register.test.ts
💤 Files with no reviewable changes (32)
- src/tools/register.ts
- src/tools/shell.ts
- src/tools/files.ts
- src/tools/webSearch.ts
- src/tools/rendering.ts
- src/tools/glob.ts
- src/vision/describe.ts
- src/tools/read.ts
- src/vision/cache.ts
- src/tools/webSearchDelegate.ts
- src/provider/toolScope.ts
- tests/tools/register.test.ts
- tests/tools/nativeAdapter.integration.test.ts
- tests/tools/glob.test.ts
- tests/vision/config.test.ts
- tests/vision/cache.test.ts
- src/tools/search.ts
- tests/provider/toolScope.test.ts
- tests/tools/rendering.test.ts
- tests/tools/files.test.ts
- tests/vision/register.test.ts
- tests/tools/webSearchDelegateRetry.test.ts
- tests/tools/toolTestHelpers.ts
- tests/tools/webSearch.test.ts
- tests/provider/toolScope.integration.test.ts
- tests/tools/webSearchDelegate.test.ts
- src/vision/register.ts
- tests/tools/search.test.ts
- tests/vision/describe.test.ts
- tests/vision/helpers.ts
- tests/tools/read.test.ts
- tests/tools/shell.test.ts
| async function migrateSettings(directory: string) { | ||
| if (!existsSync(join(directory, 'settings.json'))) return false; | ||
| const settings = SettingsManager.create(join(directory, '.pi-grok-cli-migration'), directory); | ||
| const current = settings.getGlobalSettings(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the SettingsManager.create signature and the file each argument controls.
rg -nP -B 5 -A 40 'static create' --glob '**/@earendil-works/pi-coding-agent/**' | head -120
rg -nP -A 10 'getGlobalSettings|setDefaultProvider|setEnabledModels|drainErrors' --glob '**/@earendil-works/pi-coding-agent/**/*.d.ts' | head -60Repository: kenryu42/pi-grok-cli
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== locate modelMigration and settings files =="
fd -a 'modelMigration|SettingsManager|.*settings.*|.*provider.*' . | sed -n '1,200p'
echo "== search SettingsManager references =="
rg -n "SettingsManager|modelMigration|migrateSettings|pi-grok-cli-migration|getGlobalSettings|setDefaultProvider|setEnabledModels|drainErrors|existsSync|join\\(" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: kenryu42/pi-grok-cli
Length of output: 21050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json =="
cat -n package.json
echo "== lock/package settings excerpts =="
rg -n "`@earendil-works/pi-coding-agent`|pi-coding-agent|SettingsManager|agentDir" package.json bun.lock --glob '!node_modules' --glob '!.release-tools/**' || true
echo "== modelMigration source =="
cat -n src/provider/modelMigration.ts
echo "== modelMigration tests =="
cat -n tests/provider/modelMigration.test.ts
echo "== local package metadata =="
node - <<'JS'
const fs = require('fs');
const path = require('path');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
console.log(JSON.stringify({
imports: pkg.imports,
alias: pkg._metabuddy?.imports,
dependencies: pkg.dependencies,
devDependencies: pkg.devDependencies
}, null, 2));
JS
echo "== check for vendored package files =="
fd -a '`@earendil-works`|pi-coding-agent' . | sed -n '1,80p' || trueRepository: kenryu42/pi-grok-cli
Length of output: 10175
🌐 Web query:
@earendil-works/pi-coding-agent 0.80.9 SettingsManager.create agentDir cwd getGlobalSettings flush
💡 Result:
In the @earendil-works/pi-coding-agent SDK, SettingsManager provides a centralized way to manage and persist agent configurations, which are merged from global (~/.pi/agent/settings.json) and project-specific (.pi/settings.json) files [1][2]. SettingsManager.create(cwd?: string, agentDir?: string) This is a static factory method used to initialize a SettingsManager that loads and merges settings from the file system [2][3][4]. - cwd: An optional string representing the current working directory, used to locate project-specific settings [2][5]. It defaults to process.cwd [5]. - agentDir: An optional string representing the directory where global settings are stored [2][5]. It typically defaults to a standard configuration path (often resolved via getAgentDir) [5]. getGlobalSettings This method returns a deep clone of the current global settings object [5][6]. Because SettingsManager handles merging of global and project settings internally, this method allows you to specifically inspect the global configuration scope [2][5]. flush Since setters in SettingsManager update the internal memory state synchronously but enqueue persistence writes (file I/O) asynchronously to optimize performance, flush is used to ensure durability [7][2][3]. You should call await settingsManager.flush whenever you need a strict durability boundary, such as before exiting the process or validating that changes have been written to disk in a test environment [2][3]. General Workflow 1. Initialization: Create the manager using SettingsManager.create(cwd) [7][2]. 2. Manipulation: Use provided setter methods (e.g., setQuietStartup, applyOverrides) to modify settings in memory [7][2][8]. 3. Durability: Call await settingsManager.flush to persist changes to the file system [7][2]. 4. Error Handling: Because persistence is asynchronous, I/O errors are not thrown immediately; use settingsManager.drainErrors to retrieve and handle any recorded configuration errors [7][2][3].
Citations:
- 1: https://github.com/earendil-works/pi/blob/v0.80.3/packages/coding-agent/docs/settings.md
- 2: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/sdk.md
- 3: https://pi.dev/docs/latest/sdk
- 4: https://cdn.jsdelivr.net/npm/open-multi-agent-kit@0.80.6/dist/core/settings-manager.d.ts
- 5: https://github.com/badlogic/pi-mono/blob/a3bf1eb3/packages/coding-agent/src/core/settings-manager.ts
- 6: https://cdn.jsdelivr.net/npm/@mariozechner/pi-coding-agent@0.63.2/dist/core/settings-manager.d.ts
- 7: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/examples/sdk/10-settings.ts
- 8: https://app.unpkg.com/@mariozechner/pi-coding-agent@0.68.0/files/dist/core/settings-manager.d.ts
🌐 Web query:
npm @earendil-works/pi-coding-agent 0.80.9 dist/core/settings-manager.d.ts
💡 Result:
The file dist/core/settings-manager.d.ts is part of the @earendil-works/pi-coding-agent package, a minimal terminal coding agent harness [1][2]. In version 0.80.9, released on July 16, 2026, this component is responsible for managing both global and project-specific settings [3][4][5]. The SettingsManager class facilitates loading, merging, and persisting user configurations [6][7]. It supports two configuration locations: 1. Global settings: Located at ~/.pi/agent/settings.json [7]. 2. Project settings: Located at.pi/settings.json [7]. Project settings automatically override global settings, with nested objects merged during the configuration process [7]. The SDK allows programmatic access to this functionality via the SettingsManager API, enabling developers to create agent sessions with custom configurations, including in-memory settings for testing without file I/O [6][8][9]. For further implementation details, you can refer to the official source code [5] and documentation [6][7] in the earendil-works/pi repository.
Citations:
- 1: https://www.npmjs.com/package/@earendil-works/pi-coding-agent
- 2: https://github.com/earendil-works/pi
- 3: https://github.com/earendil-works/pi/releases/tag/v0.80.9
- 4: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/CHANGELOG.md
- 5: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/src/core/settings-manager.ts
- 6: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/sdk.md
- 7: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/settings.md
- 8: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/examples/sdk/10-settings.ts
- 9: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/src/core/sdk.ts
🌐 Web query:
raw.githubusercontent.com/earendil-works/pi v0.80.9 packages/coding-agent/src/core/settings-manager.ts create getGlobalSettings flush
💡 Result:
In the earendil-works/pi repository, the SettingsManager class within packages/coding-agent/src/core/settings-manager.ts handles global and project-specific configuration [1]. The create method is a static factory function used to instantiate the SettingsManager [1][2]. It loads and merges settings from two sources: 1. Global: ~/.pi/agent/settings.json [2] 2. Project: <cwd>/.pi/settings.json [2] The SettingsManager class does not have a method named getGlobalSettings. Instead, it exposes the merged settings via a settings property (which combines global and project settings, with project settings taking precedence) and tracks them internally via globalSettings and projectSettings fields [1][2]. Regarding flush, the SettingsManager uses a writeQueue (a Promise chain) to manage asynchronous I/O operations and ensure settings are saved correctly [1]. While there is no explicit public method named flush, saving behavior is triggered by various setter methods (e.g., setDefaultProjectTrust) which call an internal save() method [1][3]. The save() method typically orchestrates the writing of modified global settings back to the filesystem [1][2][3].
Citations:
- 1: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/src/core/settings-manager.ts
- 2: Support project-specific settings.json earendil-works/pi#270
- 3: earendil-works/pi@5cb4f59
🌐 Web query:
raw.githubusercontent.com/earendil-works/pi v0.80.9 packages/coding-agent/dist/sdk.d.ts SettingsManager create getGlobalSettings
💡 Result:
In the @earendil-works/pi repository, the SettingsManager class provides functionality for managing application configuration [1][2]. The create and getGlobalSettings methods are part of this management system [1][2]. Specifically, SettingsManager.create(cwd, agentDir) is used to instantiate a settings manager, where cwd is the working directory and agentDir is the directory where global settings (like auth.json or models.json) are stored [1][2]. Within the coding agent SDK, this is often used to initialize the environment for an agent session [1][3]. While the exact method signature in v0.80.9 may vary, the established pattern in the @earendil-works/pi codebase—as seen in its SDK definitions—is: SettingsManager.create(cwd?: string, agentDir?: string): SettingsManager Once a SettingsManager instance is created, you can access global settings. If getGlobalSettings is implemented on the instance, it typically retrieves the persisted global configuration for the agent [1][2]. For most users and developers, the SettingsManager is handled internally by the coding agent when you run the pi command or initialize a session using createAgentSession [4][3]. If you are working directly with the SDK, you would typically use these managers to configure the session as shown in the package documentation [1][3]: const loader = new DefaultResourceLoader({ cwd: process.cwd, agentDir: getAgentDir, settingsManager: SettingsManager.create, });
Citations:
- 1: https://cdn.jsdelivr.net/npm/open-multi-agent-kit@0.80.6/dist/core/sdk.d.ts
- 2: https://cdn.jsdelivr.net/npm/@mariozechner/pi-coding-agent@0.63.2/dist/core/sdk.d.ts
- 3: https://app.unpkg.com/@mariozechner/pi-coding-agent@0.68.0/files/dist/core/sdk.d.ts
- 4: https://raw.githubusercontent.com/earendil-works/pi/main/packages/coding-agent/docs/quickstart.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fetch 0.80.9 sdk.d.ts =="
curl -fsSL 'https://cdn.jsdelivr.net/npm/@earendil-works/pi-coding-agent@0.80.9/dist/sdk.d.ts' -o /tmp/sdk.d.ts
sed -n '1,220p' /tmp/sdk.d.ts
echo "== fetch 0.80.9 settings manager source =="
curl -fsSL 'https://raw.githubusercontent.com/earendil-works/pi/v0.80.9/packages/coding-agent/src/core/settings-manager.ts' -o /tmp/settings-manager.ts
rg -n "static create|getGlobalSettings|flush|getGlobal|agent" /tmp/settings-manager.ts
sed -n '1,260p' /tmp/settings-manager.tsRepository: kenryu42/pi-grok-cli
Length of output: 237
🌐 Web query:
unpkg @earendil-works/pi-coding-agent 0.80.9 dist/sdk.d.ts
💡 Result:
The package @earendil-works/pi-coding-agent, version 0.80.9, was released on July 16, 2026 [1][2][3]. The file dist/sdk.d.ts is contained within this package, which provides the SDK for the Pi coding agent [4][5]. You can access the TypeScript definition file directly through the unpkg CDN at the following URL: https://unpkg.com/browse/@earendil-works/pi-coding-agent@0.80.9/dist/sdk.d.ts The Pi coding agent toolkit is a minimal terminal harness used for building AI-driven coding workflows [6][7]. The SDK included in the @earendil-works/pi-coding-agent package allows for programmatic access to agent capabilities, such as session management and model runtime execution [5]. Developers typically use this package by installing it via npm [8][7] or by defining it as a peer dependency when creating custom extensions [9][10]. Note that newer versions of this package have been released since 0.80.9 [2][8][11]. For the latest features and documentation, it is recommended to refer to the official repository at https://github.com/earendil-works/pi [4].
Citations:
- 1: https://github.com/earendil-works/pi/releases/tag/v0.80.9
- 2: https://newreleases.io/project/npm/@earendil-works/pi-coding-agent/release/0.80.9
- 3: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/CHANGELOG.md
- 4: https://github.com/earendil-works/pi
- 5: https://github.com/badlogic/pi-mono/blob/main/packages/coding-agent/docs/sdk.md
- 6: https://pi.dev/
- 7: https://github.com/badlogic/pi-mono/tree/main/packages/coding-agent
- 8: https://registry.npmjs.org/@earendil-works/pi-coding-agent
- 9: https://unpkg.com/pi-yaml-hooks@2026.6.14/docs/setup.md
- 10: https://unpkg.com/pi-yaml-hooks@2026.6.14/README.md
- 11: https://www.npmjs.com/package/@earendil-works/pi-coding-agent
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== npm package metadata =="
npm view `@earendil-works/pi-coding-agent`@0.80.9 dist.tarball name version --json
echo "== fetch package manifests and declarations =="
python3 - <<'PY'
import json,urllib.request,urllib.error,io,tarfile,zipfile,os
url='https://registry.npmjs.org/@earendil-works%2Fpi-coding-agent/0.80.9'
try:
meta=json.load(urllib.request.urlopen(url,timeout=20))
except Exception as e:
print("ERR meta", e)
raise
print("tarball", meta["dist"]["tarball"])
tarball=meta["dist"]["tarball"]
data=urllib.request.urlopen(tarball,timeout=30).read()
print("bytes", len(data))
t=tarfile.open(fileobj=io.BytesIO(data),mode='r:gz')
names=[m.name for m in t if m.filetype==tarfile.RegFileType]
names=[n for n in names if ("dist/sdk.d.ts" in n or "dist/core/settings-manager" in n or "settings-manager.ts" in n or "package.json" in n)]
for n in names:
print("FOUND", n)
print("--- package.json ---")
print(t.extractfile("package.json").read().decode())
print("--- sdk.d.ts excerpts ---")
sdk=t.extractfile("dist/sdk.d.ts").read().decode(errors="ignore")
for line_i,line in enumerate(sdk.splitlines(),1):
if "SettingsManager" in line or "create" in line or "getGlobalSettings" in line or "flush" in line or "drainErrors" in line:
print(line_i, line)
print("--- settings-manager.ts excerpts ---")
sm=t.extractfile("packages/coding-agent/src/core/settings-manager.ts").read().decode(errors="ignore")
for line_i,line in enumerate(sm.splitlines(),1):
if line_i<=80 or 80 <= line_i <=220:
print(line_i, line)
PYRepository: kenryu42/pi-grok-cli
Length of output: 3149
🌐 Web query:
@earendil-works pi coding agent 0.80.9 SettingsManager.create
💡 Result:
In the @earendil-works/pi-coding-agent toolkit (version 0.80.9 and later), SettingsManager.create is a static factory method used to initialize a settings manager that handles the configuration for agent sessions [1][2][3]. Key details regarding SettingsManager.create include: Functionality: It loads settings from the filesystem by merging global and project-level configuration files [1][2]. Signature: SettingsManager.create(cwd?, agentDir?) [1][2]. Parameters: cwd (optional): The current working directory from which to load project settings [1][3]. agentDir (optional): A custom directory path for agent settings [1][2]. Usage: It is typically used when configuring an agent session via createAgentSession [1][2]. For example, to load default settings: const { session } = await createAgentSession({ settingsManager: SettingsManager.create, }); You can also use it to define custom directories: const { session } = await createAgentSession({ settingsManager: SettingsManager.create("/custom/cwd", "/custom/agent"), }); If you need to test without interacting with the filesystem, you can use SettingsManager.inMemory(settings?) instead [1][2][3]. Note that settings changes made via the manager are synchronous for in-memory state but enqueue persistence writes asynchronously; calling await settingsManager.flush is recommended to ensure durability [1][2][3].
Citations:
- 1: https://pi.dev/docs/latest/sdk
- 2: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/sdk.md
- 3: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/examples/sdk/10-settings.ts
Keep the migration tied to the real settings path.
With SettingsManager.create(cwd, agentDir), this call swaps the locations: cwd is directory/.pi-grok-cli-migration, so flush() persists to the temporary migration directory instead of directory/settings.json. The project then migrates an in-memory-only copy. Reuse SettingsManager.create(directory, directory) or copy directory/settings.json into the temporary agent directory first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/provider/modelMigration.ts` around lines 8 - 11, Update migrateSettings
so SettingsManager.create receives directory as the real cwd and agent
directory, ensuring flush() persists changes to directory/settings.json;
preserve the existing migration flow and avoid using the temporary
.pi-grok-cli-migration path as the cwd.
Why: - Stored OAuth expiry was skewed twice, and failed dashboard login cleanup could remove an account authenticated by another process. What: - Treat stored OAuth expiry as already skewed. - Remove a new account after login failure only when its revision is unchanged and it has no credential. - Add regression tests for both cases. Validation: - bun run test -- tests/provider/accountRouting.test.ts tests/provider/dashboard.test.ts - bun run check - autoreview --mode local
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/provider/accounts.ts (1)
552-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
elsebranch.Return after activation. Then perform login after the conditional.
Proposed change
- if (!account.authenticated) await manager.login(ctx, account.id, interaction); - else await manager.activate(ctx, account.id); + if (account.authenticated) { + await manager.activate(ctx, account.id); + return; + } + await manager.login(ctx, account.id, interaction);As per coding guidelines, “Avoid
elsestatements. Prefer early returns.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/accounts.ts` around lines 552 - 554, Update the authentication flow around manager.login and manager.activate to remove the else branch: when account.authenticated is true, await manager.activate(ctx, account.id) and return immediately; otherwise continue after the conditional to perform manager.login(ctx, account.id, interaction).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/provider/accounts.ts`:
- Around line 552-554: Update the authentication flow around manager.login and
manager.activate to remove the else branch: when account.authenticated is true,
await manager.activate(ctx, account.id) and return immediately; otherwise
continue after the conditional to perform manager.login(ctx, account.id,
interaction).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7051ade9-a86f-4eed-8a44-b1280b2df6ec
📒 Files selected for processing (25)
src/config.tssrc/provider/accountMigration.tssrc/provider/accountRouting.tssrc/provider/accountVault.tssrc/provider/accounts.tssrc/provider/dashboard/app.jssrc/provider/dashboard/server.tssrc/provider/quotaCache.tssrc/provider/sessionAccountSelection.tssrc/provider/usage.tssrc/storage.tstests/config.test.tstests/imagine/helpers.tstests/provider/accountMigration.test.tstests/provider/accountRouting.test.tstests/provider/accountVault.test.tstests/provider/accounts.test.tstests/provider/dashboard.test.tstests/provider/package.test.tstests/provider/quotaCache.test.tstests/provider/register.test.tstests/provider/sessionAccountSelection.test.tstests/provider/usage.test.tstests/stateTestHelpers.tstests/storage.test.ts
💤 Files with no reviewable changes (1)
- tests/provider/package.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/provider/quotaCache.test.ts
- tests/imagine/helpers.ts
- src/provider/accountRouting.ts
- src/provider/usage.ts
- src/storage.ts
- src/provider/sessionAccountSelection.ts
- tests/provider/sessionAccountSelection.test.ts
- src/provider/quotaCache.ts
- src/provider/accountMigration.ts
- src/provider/dashboard/app.js
- tests/provider/accounts.test.ts
- src/config.ts
1 participant
Summary by CodeRabbit
New Features
Changes
Documentation