feat(aimlapi): AI/ML API provider with guided onboarding (top-up + key issuance)#655
feat(aimlapi): AI/ML API provider with guided onboarding (top-up + key issuance)#655StanAIML wants to merge 22 commits into
Conversation
Add the AI/ML API provider integration to the zero CLI: a guided TUI onboarding sub-flow that takes an existing key or an email top-up and writes the issued key into the provider profile, attributed to the Gitlawb rebate partner. - internal/aimlapi: OpenAI-compatible client, partner-checkout top-up, key validation, streaming, and endpoint config (Paths A/B/C). - providercatalog / providermodelcatalog: register the aimlapi provider with default headers and models. - tui: aimlapi onboarding screen wired into the provider wizard and onboarding flow. Default endpoints point at production (auth/app/api/pay/aimlapi.com); each is overridable via AIMLAPI_*_URL env vars. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- topup: use errors.As for the 5xx retry check so a wrapped APIError still retries instead of aborting a paid checkout. - onboarding: a Path-A top-up on an email with no account now lands on a single coherent "key saved, balance not topped up" screen instead of a green "ready" plus a red error. - onboarding: an empty amount field is rejected instead of silently falling through to the $25 default. - collapse the two identical top-up prompt constants into MsgTopUpPrompt and drop the dead amountPrompt() branch. - reuse stdlib maps.Clone instead of a third bespoke header-copy helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds AIMLAPI as a recommended provider with configurable clients, passwordless onboarding, hosted checkout top-ups, API-key provisioning, curated models, attribution headers, and integrated TUI flows. ChangesAIMLAPI API and checkout flow
Provider catalog and TUI integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant AIMLAPI
participant ProviderConfig
User->>TUI: select AIMLAPI onboarding
TUI->>AIMLAPI: validate key or authenticate by email
TUI->>AIMLAPI: create and pay checkout session
AIMLAPI-->>TUI: return provisioned API key
TUI->>ProviderConfig: save key, base URL, model, and headers
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/aimlapi/client_test.go (1)
72-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a test for the exact maximum boundary
$10,000.The table tests "below minimum" (
$19.99) and "above maximum" ($10,001) but never the exact maximum ($10,000→1000000minor). Adding this case confirms the upper bound is inclusive.🤖 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 `@internal/aimlapi/client_test.go` around lines 72 - 97, Add an “exact maximum” case to the TestParseAmountUSD table, using value "10000", expecting 1000000 minor units and no error, to verify the upper bound is inclusive.internal/aimlapi/config_test.go (1)
5-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
VerificationBaseURLinTestResolveEndpointsDefaults.The test sets four
AIMLAPI_*env vars to empty but omitsAIMLAPI_VERIFICATION_BASE_URLand doesn't assertendpoints.VerificationBaseURL. If that env var leaks from the environment, the test won't catch a regression. Addt.Setenv("AIMLAPI_VERIFICATION_BASE_URL", "")and assert the default"https://aimlapi.com/app".🤖 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 `@internal/aimlapi/config_test.go` around lines 5 - 18, Add verification URL coverage to TestResolveEndpointsDefaults: clear AIMLAPI_VERIFICATION_BASE_URL before calling ResolveEndpoints, then assert endpoints.VerificationBaseURL equals "https://aimlapi.com/app" alongside the existing endpoint assertions.internal/aimlapi/config.go (1)
307-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPartner ID is duplicated between
config.goandcatalog.go.
DefaultPartnerIDinconfig.go(line 22) and the hardcoded"part_62yQoGYDq4Yqnrj2R1iGrDNJ"incatalog.go(line 310) are the same value. If one is updated and the other isn't, attribution headers will silently diverge. Consider referencing the shared constant.🤖 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 `@internal/aimlapi/config.go` around lines 307 - 313, The partner ID is duplicated across the configuration and catalog code, risking inconsistent attribution. Update the hardcoded partner ID in the catalog construction logic to reference the existing DefaultPartnerID constant from config.go, removing the duplicate literal while preserving current behavior.internal/cli/provider_setup.go (1)
548-560: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
copyProviderHeadersduplicatescopyStringMapfromcatalog.go.Both functions are identical: nil-safe deep-copy of
map[string]string. Consider consolidating into a single shared helper to avoid divergence.🤖 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 `@internal/cli/provider_setup.go` around lines 548 - 560, The helper copyProviderHeaders duplicates copyStringMap from catalog.go; consolidate them into one shared map-copy helper. Update mergeProviderHeaders and all other callers to use the retained shared function, remove the duplicate, and preserve nil-safe map-copy behavior.
🤖 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 `@internal/aimlapi/config.go`:
- Around line 48-53: Ensure BuildPartnerCheckoutReturnURLs never constructs
relative checkout URLs when appBaseURL is empty or whitespace. After trimming
the input, add validation and either fall back to the existing default
PayBaseURL or use an appropriate safe error-handling path, while preserving
token escaping and both success/cancel URL formats.
In `@internal/aimlapi/topup.go`:
- Around line 52-80: Update pollUntilPaid to retry transient transport errors
from client.GetSession, which are non-APIError failures, alongside APIError
responses with status 500 or higher. Preserve immediate returns for context
cancellation/deadline errors and terminal API errors such as 4xx responses,
using errors.Is with context cancellation/deadline sentinels as needed.
In `@internal/tui/aimlapi_onboard.go`:
- Around line 651-656: Update the amount-validation error handling in the
onboarding flow around ParseAmountUSD: preserve the existing low-amount message
only for the corresponding minimum-amount error, and display the actual
validation error for non-numeric or over-maximum inputs. Keep resetting step and
amountField and returning aimlapiContinue for all validation failures.
---
Nitpick comments:
In `@internal/aimlapi/client_test.go`:
- Around line 72-97: Add an “exact maximum” case to the TestParseAmountUSD
table, using value "10000", expecting 1000000 minor units and no error, to
verify the upper bound is inclusive.
In `@internal/aimlapi/config_test.go`:
- Around line 5-18: Add verification URL coverage to
TestResolveEndpointsDefaults: clear AIMLAPI_VERIFICATION_BASE_URL before calling
ResolveEndpoints, then assert endpoints.VerificationBaseURL equals
"https://aimlapi.com/app" alongside the existing endpoint assertions.
In `@internal/aimlapi/config.go`:
- Around line 307-313: The partner ID is duplicated across the configuration and
catalog code, risking inconsistent attribution. Update the hardcoded partner ID
in the catalog construction logic to reference the existing DefaultPartnerID
constant from config.go, removing the duplicate literal while preserving current
behavior.
In `@internal/cli/provider_setup.go`:
- Around line 548-560: The helper copyProviderHeaders duplicates copyStringMap
from catalog.go; consolidate them into one shared map-copy helper. Update
mergeProviderHeaders and all other callers to use the retained shared function,
remove the duplicate, and preserve nil-safe map-copy behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7f3c735-2a7d-454e-8528-d2188b3957b1
📒 Files selected for processing (25)
.gitignoreinternal/aimlapi/client.gointernal/aimlapi/client_onboard.gointernal/aimlapi/client_test.gointernal/aimlapi/config.gointernal/aimlapi/config_test.gointernal/aimlapi/messages.gointernal/aimlapi/onboard_test.gointernal/aimlapi/stream.gointernal/aimlapi/topup.gointernal/aimlapi/validation.gointernal/cli/command_center_test.gointernal/cli/provider_setup.gointernal/config/resolver.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providermodelcatalog/catalog.gointernal/providermodelcatalog/catalog_test.gointernal/tui/aimlapi_onboard.gointernal/tui/model.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/provider_manager_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
…ore entry Addresses the two CodeRabbit PR checks on Gitlawb#655: - Add doc comments to the exported aimlapi client API (Client, APIError, the session/checkout/onboarding types and their methods) and ParseAmountUSD, to satisfy the docstring-coverage check. - Remove the unrelated /zero.exe~ .gitignore entry that an earlier review-fix commit added; it is out of scope for the provider work, so the PR no longer touches .gitignore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I checked #655 in a fresh worktree and ran the build and touched tests myself. The title is accurate — this is a full guided-onboarding flow, not just a preset line: an internal/aimlapi HTTP client (auth, top-up, key-issuance) and a ~1000-line TUI onboard screen, +2584 against main. The outbound calls to aimlapi.com are the user-initiated top-up and key-issuance steps the feature advertises, not silent phone-home, and no API keys are committed. Build, vet, and the aimlapi/catalog/config tests pass; only the known pre-existing main failures remain.
The thing that blocks me: catalog.go bakes always-on partner-attribution headers into the aimlapi descriptor — X-AIMLAPI-Partner-ID (a specific partner id), X-AIMLAPI-Integration-Repo, X-AIMLAPI-Integration-Version — and resolver.go applies them to every inference request, with an in-code comment that they "attribute usage for the rebate." Crediting a specific partner for a rebate on every request from a community preset is a maintainer policy call — I'd want explicit sign-off or an opt-in before this lands. It's also not a superset of #402/#621: those are minimal preset entries, this is a much larger feature, so which (if any) aimlapi PR is canonical depends on whether you want the onboarding at all.
Requesting changes pending that policy decision.
|
@gnanam1990 would value your read on this one. The blocker is the always-on partner-attribution headers baked into the catalog (X-AIMLAPI-Partner-ID etc., applied to every request — the comment says "for the rebate"). That's a policy call I think needs maintainer sign-off; would be good to have your take too. |
|
Hey! The reason I'm asking is that this PR contains a much more complete onboarding experience, including authentication, top-up, API key creation, and the full guided flow, rather than just adding a preset. Unfortunately, we weren't aware of the other AIMLAPI PRs at the time, and we haven't been able to get in touch with the original author despite trying. We'd be happy to coordinate or adapt if needed, but we'd really like to see this implementation move forward. |
|
@Vasanthdev2004 this would be considered official pr |
There was a problem hiding this comment.
Re-checked #655 at the latest HEAD (79f60ad) in a fresh worktree. Build and the aimlapi/catalog/config tests pass; the base URL, auth wiring, and curated model list are correct, and no credentials are hardcoded the only baked-in identity is the partner ID, which is an account ID, not a secret.
The partner attribution is intentional for this arrangement: X-AIMLAPI-Partner-ID (catalog.go:309-313) rides on each request via the resolver-to-provider header path, the partner-checkout top-up routes to the agreed DefaultPartnerID (config.go:22, client.go:111), and the latest commit renders aimlapi as a standard recommended row alongside the other recommended providers. Treating this as an endorsed partner preset approving.
|
Hi everyone! |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve bundled local-control helpers in the npm install
scripts/postinstall.mjs:277
Release archives always stage thehelpers/directory, but the new postinstall path extracts an archive and copies onlyzeroplus sandbox binaries. The npm wrapper then findsagent-browserandtuistoryonly underpackageRoot/node_modules/.bin, while this package has neither those dependencies nor a copied helpers tree. Consequently every normal npm install loses browser/terminal local-control capabilities that the release archives and previous platform packages supplied. Copy the extracted helpers directory into the package (and point the wrapper at it), or install those helper packages as part of the npm distribution. -
[P1] Do not resolve plugin executables from the caller workspace
internal/plugins/activate.go:461
This change leaves./bin/tool.shand other path-bearing relative commands unchanged even though the documented manifest format says relative plugin paths are resolved at activation. Tool execution prefers the caller workspace as its CWD, so a documented plugin command now either fails because that workspace has no matching path or runs a workspace-controlled executable under the plugin tool's declared permission. Keep resolving relative executable paths underPluginDirand restore coverage for both tool and hook commands. -
[P1] Bind a Path-A top-up to the pasted key's account
internal/tui/aimlapi_onboard.go:394
The existing-key path validates key A only withGetBalance, then asks the user to sign in with an arbitrary email and uses that account's session token forPay. On completion the flow retains and saves key A (applyTopupreplaces the key only for new accounts), so signing into account B and paying funds B while the UI reports a successful top-up for A. Verify the key belongs to the authenticated account before offering the top-up, or use a flow that funds the selected key's actual account. -
[P2] Carry the configured inference endpoint into the saved profile
internal/tui/onboarding.go:672
StreamTopUpreturns the resolvedAIMLAPI_INFERENCE_URL, but both onboarding hosts discard it and later save the catalog's production base URL. A staging/custom-endpoint user can complete registration and top-up against the override, then receive a profile that sends the newly issued key tohttps://api.aimlapi.com/v1. Preserve the onboarding result's base URL (and model where applicable) when constructing the setup/wizard profile. -
[P2] Cancel in-flight account and key requests when the flow is abandoned
internal/tui/aimlapi_onboard.go:164
Escape only cancels the top-up stream; balance, account, code, passwordless, and key-minting requests all usecontext.Background(). For example, a user can cancel whileCreateKeyis in flight, after which the server still mints a key but its result is discarded because the state has been removed. Give each pending operation a cancellable context and cancel it when the flow is abandoned. -
[P2] Do not present unsupported automatic top-up as enabled
internal/aimlapi/client.go:144
The onboarding UI defaults automatic top-up to on, but this code explicitly documents that the backend does not yet honorautoTopUp; the field merely passes through its validation layer. Users can therefore complete checkout believing recurring funding was enabled when it was silently ignored. Hide/disable this control until the backend contract supports it, or surface that it is unavailable. -
[P2] Restore durable credential publication
internal/credstore/credstore.go:250
Removing the temp-fileSyncbefore rename weakens the only data-durability barrier for both the encrypted credential store and its encryption secret (internal/securefile/securefile.go:199). After a crash or power loss, the replacement name can survive without durable contents, losing stored provider keys or making them undecryptable. Restore the pre-rename file sync (and directory sync where supported). -
[P2] Correct the advertised npm/release platform support
docs/INSTALL.md:19
The changed docs say Windows arm64 is supported, but the release matrix publishes onlywindows-x64andscripts/postinstall.mjsexplicitly skips Windows arm64 because no asset exists. A native Windows-on-ARM install completes without a binary andzerothen cannot launch. Restore the x64-emulation wording or add and publish the arm64 artifact. -
[P2] Keep the documented Go requirement consistent with the module
README.md:9
The docs were changed to require Go 1.25+, whilego.modstill declaresgo 1.26.5. A Go-1.25-only source install cannot build this module, particularly in an offline or locked-down environment where automatic toolchain download is unavailable. Restore the 1.26.5 requirement in the documentation or actually lower and validate the module requirement.
…e URL, request cancellation)
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
The out-of-scope findings were already fixed on main while this was in review, so I reverted my duplicate commit for them rather than re-doing the work — which also clears the merge conflicts (every conflict was in exactly those files, and they now match main): Plugin relative exec-path resolution → #627 [P1] Path A top-up — dropped the pasted-key balance/top-up path, so it can't fund a different account than the key. @jatmn Ready for re-review, please check |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/tui/onboarding_test.go`:
- Around line 198-202: Update the low-balance test around state.apply so it
dispatches the message kind handled by applyKeyBalance, or invokes
applyKeyBalance directly, instead of using aimlapiMsgKeyValidation. Assert the
expected low-balance state transition so the test exercises and verifies the
balance-handling behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 182eaa33-271d-4169-b371-19b1a19b351c
📒 Files selected for processing (3)
internal/aimlapi/messages.gointernal/tui/aimlapi_onboard.gointernal/tui/onboarding_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/aimlapi/messages.go
- internal/tui/aimlapi_onboard.go
|
Updated the API key validation, and it should now meet all the current requirements. @jatmn Could you please take another look at the latest changes and re-review the PR? Thanks! |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve an ambiguous checkout session instead of retrying with a new one
internal/aimlapi/stream.go:53
PayandExchangeare side-effecting requests, but their session token is retained only on the stack. If the server completes either operation and the response is lost, the flow reports a failure, drops that token, and the amount screen starts a brand-new session on retry. That can present the customer with a second checkout after the first payment succeeded, and an already-issued key cannot be recovered. Retain and resume/poll the original session after ambiguous failures (or use an idempotency contract) rather than discarding it. -
[P2] Cancel the embedded flow before returning to the provider manager
internal/tui/provider_wizard.go:793
The manager-modeEscbranch runs before the AIMLAPI key delegation and only changes the wizard step. It leaveswizard.aimlapiand its pending operation/top-up context alive, so exiting from a manager-launched flow can still completeCreateKeyor checkout in the background and discard the result. Route this branch through the same reset/cancellation logic as normal AIMLAPI cancellation. -
[P2] Do not attach AIMLAPI attribution headers to an overridden endpoint
internal/tui/provider_wizard.go:1940
Guided onboarding copies the descriptor's partner headers unconditionally, including whenAIMLAPI_INFERENCE_URLsupplied a staging, proxy, or custom base URL. The catalog resolver deliberately applies those headers only for the catalog endpoint, so this path disagrees with the normal provider setup and sends partner attribution to an arbitrary override. Gate the copied headers on the resolved endpoint matching the catalog default. -
[P2] Make catalog header overrides case-insensitive
internal/cli/provider_setup.go:549
HTTP header names are case-insensitive, but the new merge keeps map keys verbatim. For example,--header x-aimlapi-partner-id=...leaves both that key and the catalog'sX-AIMLAPI-Partner-ID; later request construction canonicalizes both while ranging a Go map, so either value can win. Canonicalize keys while merging and add a mixed-case override test. -
[P2] Reject non-finite top-up amounts before converting to cents
internal/aimlapi/topup.go:56
strconv.ParseFloat("NaN", 64)succeeds, and NaN bypasses each comparison in the min/max validation before being converted to an invalid integer minor-unit amount. The amount field accepts arbitrary text, so this reachesPayas a malformed checkout request rather than producing the local validation error promised by the UI. Reject NaN and infinities before conversion. -
[P2] Do not label every verification failure as an incorrect code
internal/tui/aimlapi_onboard.go:544
While on the code screen, every error fromVerifySignInCode—including a timeout, 429, 5xx, or cancelled request—is rewritten as “Code you've entered is incorrect.” This sends users down the wrong recovery path during service failures. Restrict that message to the backend's invalid-code response and surface other errors normally. -
[P2] Keep the documented pasted-key low-balance path reachable
internal/tui/aimlapi_onboard.go:183
The “I already have a key” path callsGetBalancebut discards itsBalanceResult, and successful validation always lands on “Everything is ready.” A valid key withlowBalance: truetherefore can never reach the optional top-up chooser, contrary to the PR's stated onboarding behavior. Either restore a safe account-bound top-up path or correct the advertised behavior and UI copy. -
[P2] Preserve the normal environment-key setup path for AIMLAPI
internal/tui/provider_wizard.go:626
Both TUI routes divert AIMLAPI into guided onboarding before their normal credential step, even though the descriptor advertisesAIMLAPI_API_KEY. The guided pasted-key screen has no environment-key branch and cannot advance blank, so a user who already configured only that environment variable must paste it again or create an account. Offer a “useAIMLAPI_API_KEY” option or retain the standard blank-credential env behavior.
|
I took a second look at jatmn's P1 on stream.go and it checks out. The checkout session token from CreateSession (and the paid session token from pollUntilPaid) are stack-local in StreamTopUp, and Pay and Exchange are side-effecting. If a step fails after the user has paid — e.g. pollUntilPaid times out on a slow checkout — the paid session is lost and a retry calls CreateSession again, opening a fresh checkout and risking a double charge with the original payment orphaned. Worth fixing before merge. My earlier approve was on the partner-attribution question, not the checkout robustness — this is a separate code issue. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/provider_wizard.go (1)
711-719: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetreat from the aimlapi step loses in-progress top-up context.
Unlike the Model-step retreat case (Lines 729-735), which checks
aimlapiProfile(wizard.aimlapiExistingProfile)to route back toproviderWizardStepAimlapiConfigured, this case unconditionally callsresetAimlapiOnboard()(which wipesaimlapiExistingProfile/aimlapiRuntimeKey) and always routes to Name/Endpoint/Provider. When the aimlapi step was entered via the low-balance top-up path (applyExistingAimlapiBalanceinprovider_wizard_discovery.go, which intentionally keepsaimlapiExistingProfilealive), pressing Left here discards that existing-profile context and skips past the "already configured" preflight screen the user came from.🔧 Proposed fix
case providerWizardStepAimlapi: - wizard.resetAimlapiOnboard() - if providerWizardNeedsProfileName(wizard.currentProvider()) { + hadExisting := aimlapiProfile(wizard.aimlapiExistingProfile) + wizard.resetAimlapiOnboard() + if hadExisting { + wizard.step = providerWizardStepAimlapiConfigured + } else if providerWizardNeedsProfileName(wizard.currentProvider()) { wizard.step = providerWizardStepName } else if providerWizardNeedsEndpoint(wizard.currentProvider()) { wizard.step = providerWizardStepEndpoint } else { wizard.step = providerWizardStepProvider }Note
resetAimlapiOnboard()clearsaimlapiExistingProfilebefore you can check it, so the check must happen first (orclearAimlapiExisting()needs to be skipped for this branch).🤖 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 `@internal/tui/provider_wizard.go` around lines 711 - 719, Update the providerWizardStepAimlapi retreat branch to check aimlapiProfile(wizard.aimlapiExistingProfile) before calling resetAimlapiOnboard(). When an existing profile is present, route back to providerWizardStepAimlapiConfigured and preserve the in-progress top-up context; otherwise retain the current reset and Name/Endpoint/Provider routing behavior.
🧹 Nitpick comments (1)
internal/aimlapi/stream.go (1)
135-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate session-resolution logic between
resolveTopupSessionandresolveByKeySession.Both functions repeat the same create-or-resume/
GetSession/status-switch shape, differing only in how a couple of statuses map to phases (andresolveByKeySessioncollapsesPaid/Exchanging/Exchangedall intophaseExchange, overloading that phase's meaning). Since this logic exists specifically to prevent double-charges, having two hand-maintained copies risks a future status/case added to one switch but missed in the other, silently reintroducing the exact bug this code was written to avoid.Consider factoring the shared create-or-resume +
GetSessioncall into one helper that takes afunc(SessionStatus) (topupPhase, error)mapper, with each caller supplying its own status→phase table.Also applies to: 277-304
🤖 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 `@internal/aimlapi/stream.go` around lines 135 - 174, Refactor the duplicated create-or-resume and GetSession flow shared by resolveTopupSession and resolveByKeySession into one helper that accepts a SessionStatus-to-phase/error mapper. Keep each function’s status mapping as its own supplied mapper, preserving resolveTopupSession’s distinct phaseExchange, phaseWaitExchange, and terminal behavior while retaining resolveByKeySession’s intended mappings.
🤖 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 `@internal/tui/provider_wizard_discovery.go`:
- Around line 130-143: Update the AIMLAPI env-var fallback in
providerWizardProfile to populate CustomHeaders using the same catalog-default
attribution logic as the normal profile path, so the returned
config.ProviderProfile preserves partner headers. Extend
TestAdvanceAimlapiWithExistingCredentialShowsConfiguredPreflight and
TestApplyExistingAimlapiPreservesEnvCredentialSource to assert the expected
CustomHeaders.
- Around line 152-198: The existing AIMLAPI profile base URL is not propagated
into model discovery. In applyExistingAimlapiBalance, before calling
providerModelDiscoveryCmd for a successful non-low-balance result, assign
wizard.baseURL from wizard.aimlapiExistingProfile.BaseURL so
providerWizardDiscoveryProfile passes the saved URL instead of the default.
In `@internal/tui/provider_wizard.go`:
- Around line 931-955: Update the busy-cancel branch for aimlapiExistingBusy in
the provider wizard to call clearAimlapiExisting() before returning to
providerWizardStepProvider, matching the non-busy KeyLeft path while preserving
the generation increment and busy-state reset.
---
Outside diff comments:
In `@internal/tui/provider_wizard.go`:
- Around line 711-719: Update the providerWizardStepAimlapi retreat branch to
check aimlapiProfile(wizard.aimlapiExistingProfile) before calling
resetAimlapiOnboard(). When an existing profile is present, route back to
providerWizardStepAimlapiConfigured and preserve the in-progress top-up context;
otherwise retain the current reset and Name/Endpoint/Provider routing behavior.
---
Nitpick comments:
In `@internal/aimlapi/stream.go`:
- Around line 135-174: Refactor the duplicated create-or-resume and GetSession
flow shared by resolveTopupSession and resolveByKeySession into one helper that
accepts a SessionStatus-to-phase/error mapper. Keep each function’s status
mapping as its own supplied mapper, preserving resolveTopupSession’s distinct
phaseExchange, phaseWaitExchange, and terminal behavior while retaining
resolveByKeySession’s intended mappings.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f6669f2-8d42-41fa-80cd-473d9e7c3345
📒 Files selected for processing (16)
internal/aimlapi/client.gointernal/aimlapi/client_test.gointernal/aimlapi/messages.gointernal/aimlapi/stream.gointernal/aimlapi/stream_test.gointernal/aimlapi/topup.gointernal/cli/command_center_test.gointernal/cli/provider_setup.gointernal/cli/setup_test.gointernal/tui/aimlapi_onboard.gointernal/tui/model.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_test.go
💤 Files with no reviewable changes (1)
- internal/aimlapi/messages.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/tui/model.go
- internal/aimlapi/client_test.go
- internal/tui/onboarding.go
- internal/tui/aimlapi_onboard.go
- internal/tui/onboarding_test.go
|
Implemented the requested AIMLAPI follow-up fixes:
Tests and vet pass for the affected packages. @jatmn Could you please re-review when you have a chance? |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P0] Keep ambient credentials out of sandboxed commands
internal/sandbox/runner.go:377
The new default path passesos.Environ()directly to every sandboxed command, while this PR also removes the default credential-file deny list ininternal/sandbox/profile.go:76. A prompt-approved command can now read and exfiltrate provider tokens, cloud credentials, and other parent-process secrets through its environment (and the default network policy permits egress); it can also read the common cloud credential directories that the prior profile denied. Restore secret scrubbing and the credential deny-read defaults (including their explicit allow-read carve-outs) before exposing the new sandbox behavior. -
[P1] Do not send a custom AIMLAPI profile's key to the production API
internal/tui/provider_wizard_discovery.go:167
existingAimlapiConfigurationaccepts any saved profile named/cataloguedaimlapi, including one configured with a proxy, staging, or privateBaseURL, but this preflight validates its key usingaimlapiOnboardClient()and therefore the global public endpoint. Merely selecting that existing profile transmits its credential to the wrong service and reports a valid custom-endpoint key as invalid; the low-balance flow similarly resets its endpoint before discovery. Either restrict this guided path to the canonical endpoint or construct the client/state from the persisted base URL. -
[P1] Keep macOS keychain secrets out of the process argument vector
internal/keyring/keyring.go:67
The macOSSetpath now invokessecurity add-generic-password ... -w <secret>. Same-user processes can observe this argv while the command runs, exposing stored API keys and tokens. The base implementation deliberately usedsecurity -iwith stdin for this reason; restore that non-argv transport (and its input validation). -
[P1] Fail closed when restoring a raced live daemon lock fails
internal/daemon/lock.go:88
After moving a lock aside, a live PID discovered in the moved file is restored with an ignoredos.Renameerror. If that restore fails (for example due to a Windows sharing violation), the canonical path remains absent and the acquisition loop can create a second lock while the live holder continues using the moved one. Propagate/fail closed on the restore failure rather than treating the lock as reclaimable; apply the same correction to the equivalent lock reclaimers changed in this PR. -
[P1] Restore unknown configuration-field diagnostics
internal/config/validate.go:33
This branch deletesunknownfields.go, removes bothunknownFieldIssuescalls, and deletes their coverage. Sinceencoding/json.Unmarshalignores unknown fields,zero config validatenow accepts typos such assandbox.blockUnixSocketormaxTurnand silently runs with different defaults, including for security-relevant policy settings. Restore the recursive unknown-field checks and tests that are present on the PR base. -
[P1] Make Windows-on-ARM installation actually install an executable
docs/INSTALL.md:19
The changed documentation promises Windows arm64 support, but the release matrix only publisheswindows-x64andscripts/postinstall.mjs:111exits successfully without downloading a binary for Windows arm64. Consequentlynpm install -g @gitlawb/zerosucceeds on that platform but leaveszerounable to launch. Download the x64 fallback under emulation or document the platform as unsupported until an arm64 asset is released. -
[P2] Fold catalog and persisted custom headers by HTTP header name
internal/config/resolver.go:1040
The resolver copies catalog headers and then overlays persistedCustomHeaderswith case-sensitive map keys. A user override written asx-aimlapi-partner-idtherefore coexists with catalogX-AIMLAPI-Partner-ID; laterhttp.Header.Setcanonicalizes both while iterating the map, so either value can win nondeterministically. Reuse the case-insensitive merge used byproviders addwhen applying a catalog descriptor. -
[P2] Generate a fresh by-key idempotency ID after a terminal checkout
internal/tui/aimlapi_onboard.go:651
A cancelled, expired, or failed by-key session emits an empty session token, which clearsresumeSessionTokenbut retainspaymentSessionID. The next attempt creates a new partner session while reusing the old idempotency ID, whose documented contract returns the original checkout; it can therefore return/reject the terminal checkout instead of allowing the user to pay again. Clear the payment ID whenever the stream reports that its session is dead, while retaining it for retries of a live session. -
[P2] Do not regress the documented source-build toolchain requirement
README.md:9
The changed source-install instructions say Go 1.25+ is sufficient, butgo.modrequires Go 1.26.5. A Go-1.25-only machine cannot build the project without downloading a newer toolchain, which is particularly problematic in locked-down/offline environments. Restore the 1.26.5+ requirement in all changed installation docs, or lower and validate the module requirement. -
[P2] Rebase onto current main instead of carrying unrelated behavior rollbacks
internal/config/resolver.go:43
The PR's merge base predates currentmain, and the resulting full diff reverts several already-merged fixes unrelated to AIMLAPI, including the default/deep turn budgets (80/160 back to 50), MiniMax M3 vision metadata, and M2 think-tag handling. These regressions make the provider feature unsafe to merge as-is and will create conflict-prone resolution work. Rebase onto currentmain, retain the current behavior, and re-review the resolved diff.
4 participants
AI/ML API guided onboarding
Adds AI/ML API (aimlapi.com) as a built-in provider with a guided TUI onboarding sub-flow. Instead of leaving the CLI to register, top up, and paste a key by hand, the user goes from "no provider" to a working, funded key without leaving Zero.
Reached from both the first-run setup and the
/providerwizard by picking aimlapi.com from the provider list.Fixes #651
Onboarding paths
/v1/billing/balanceand saved. A low balance offers an optional top-up.Changes
internal/aimlapi— OpenAI-compatible client, partner-checkout top-up with streamed progress, email/key validation, and endpoint config.providercatalog/providermodelcatalog— register the aimlapi provider (badged recommended) with default headers and a curated model list.tui— the sharedaimlapiOnboardStatesub-flow, wired into the provider wizard and first-run onboarding; the spinner tick stays alive during the async waits.cli/config— merge the provider's custom headers intoproviders add aimlapiand catalog-resolved profiles.Implementation notes
Testing
gofmt,go vet ./...,go build ./..., andgo test ./...pass locally (all packages).go run ./cmd/zero-release build+smoke,zero-perf-bench --ci, andgovulncheck ./...pass — the blocking gates of the three CI jobs.internal/aimlapi(client, config, onboarding) and theinternal/tuionboarding flow.-racewas not run locally (no cgo toolchain on the dev box); CI'sgo test ./...covers the suite.Summary by CodeRabbit