[fix] Stop asking for a provider key the vault already holds [AGE-4289] - #6677
[fix] Stop asking for a provider key the vault already holds [AGE-4289]#6677mmabrouk wants to merge 5 commits into
Conversation
The Model section's "Connect key" badge and its "Connect the model's provider key to run this agent." tooltip read presence off the vault row's value. A write-only record never returns its value; it reports presence through hasKey instead. So on every write-only deployment the playground kept asking for a key while the agent ran on that same key. The presence check now goes through hasStoredKey, the one vault presence rule, and the whole gate moves into agentProviderNeedsKey so it cannot drift back.
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 SummarySummary by CodeRabbit
WalkthroughThe change treats empty harness catalogs as unusable, refetches unusable cached catalogs, preserves loading and error states, and centralizes provider-key prompt decisions around stored-key presence and connection state. ChangesHarness catalog validation and recovery
Provider-key prompt handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Low Merge Risk: 🔵 Low · up to Provider-key prompting and catalog recovery are covered for stored keys, unreadable subscriptions, and empty cached catalogs. An empty catalog returned during a cold load may still trigger an extra request, and the recovery test setup needs lifecycle-rule alignment before merge. Sequence Diagram(s)sequenceDiagram
participant CandidateLoader
participant QueryClient
participant HarnessCatalogAPI
CandidateLoader->>QueryClient: inspect cached catalog
QueryClient-->>CandidateLoader: return empty or usable catalog
CandidateLoader->>HarnessCatalogAPI: refetch when catalog is unusable
HarnessCatalogAPI-->>QueryClient: store catalog result or error
QueryClient-->>CandidateLoader: return loading, error, or usable catalog
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
…pty catalog Every model route is built by asking a harness what it supports, so an empty capability map answers "nothing is runnable" for a project whose vault holds working keys. The connect-a-model gate then tells the user to add a provider key they already added, and it keeps telling them across reloads because the catalog query persists to IndexedDB. fetchHarnessCapabilities built that empty map from any 200 that carried no harnesses. It now rejects instead, so the query reports the failure and the persister stores nothing. Candidate resolution also treats an empty map as an unresolved source, so a copy cached by an older build cannot activate the gate either. Also applies the Codex review of the first commit: the prompt rule is named shouldPromptForProviderKey in providerKeyPrompt.ts, takes the normalized ConnectionMode, and its comments no longer claim more than it proves.
Railway Preview Environment
|
|
Browser verification, before and after. Before, on staging v0.115.3 (https://staging.preview.agenta.dev, fresh project, the issue's own steps). Added a working OpenAI key through Settings, AI providers. Test reported After, on this PR's preview (https://gateway-pr-6677.up.railway.app). Same vault row shape, confirmed on the wire: The Model row reads Regression, same preview project. Deleted the only secret and reloaded. Both messages come back and the composer is disabled again: Screenshots are on the dev box under The empty-catalog half of the second commit is covered by unit tests only. Forcing a 200 that carries no harnesses needs request interception, which the QA browser cannot do against a deployed stack. |
|
@coderabbitai review |
|
Rejecting the empty map at the API boundary stops new ones, but a copy an older build already wrote to IndexedDB keeps `data` defined, so the refetch error was swallowed and the playground sat in loading with no notice and no retry. The capability atom and the candidate atom now both read an empty map as no catalog, which surfaces the error and brings back the retry. The imperative loader forces one refetch when the cache holds an unusable map. `ensureQueryData` serves whatever is cached, so a retry after the server recovered made no request at all. From the Codex review of the previous commit.
|
Two Codex reviews at xhigh, one per fix. Both verdicts and what I did with them. On the provider-key prompt. "The presence fix is correct, and I would ship it as a Model-badge fix." It asked for three changes, all applied: the helper is On the empty catalog. "Keep both guards, but don't ship unchanged." It reproduced, against the real query cache and the pinned persister, that rejecting the empty map at the boundary does not recover a copy an older build already persisted: the error was swallowed while It also answered the design questions. Throwing belongs at the API boundary, because this endpoint is built from the shipped harness table and is never legitimately empty. Counting harness keys is the right emptiness test, and demanding a non-empty provider list would be wrong, since the schema permits a harness that genuinely reaches nothing. No request loop: consumers share one query key, focus refetching is off, and the imperative path disables retries. Two things it raised that I did not take, both stated in the PR body: dropping an invalid entry at restore time, and the |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 4 seconds. |
…t none `fetchSubscriptionStatus` returns null when the runner's answer fails the boundary schema. Candidate resolution read that as "this deployment has no subscriptions", so a project with no vault key got the add-a-key banner on a claim we never established. The subscription card already calls the same null a failed check. Unknown now joins the request we could not make: reported only where it changes the answer, which stands the gate down instead of asking for a key. Two tests said null meant "none"; they now spell a genuine none as the runner answering not_configured for every harness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: f87e7c4b-59d6-4288-94ae-99a51f9b73b2
📒 Files selected for processing (9)
web/packages/agenta-entities/src/secret/state/persistence.tsweb/packages/agenta-entities/src/workflow/api/api.tsweb/packages/agenta-entities/src/workflow/state/agentModelCandidates.tsweb/packages/agenta-entities/src/workflow/state/inspectMeta.tsweb/packages/agenta-entities/tests/unit/agent-model-candidate-sources.test.tsweb/packages/agenta-entities/tests/unit/harness-catalog-recovery.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/providerKeyPrompt.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsxweb/packages/agenta-entity-ui/tests/unit/providerKeyPrompt.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review.
| let client: QueryClient | ||
|
|
||
| beforeEach(() => { | ||
| client = new QueryClient({defaultOptions: {queries: {retry: false}}}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge Agenta-AI/agenta /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/conventions /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/learnings
Length of output: 44334
🏁 Script executed:
#!/bin/bash
set -eu
file="web/packages/agenta-entities/tests/unit/harness-catalog-recovery.test.ts"
printf '%s\n' '--- file context ---'
cat -n "$file" | sed -n '1,90p'
printf '%s\n' '--- nearby QueryClient usage in the same test scope ---'
rg -n -C 3 'QueryClient|queryClient|renderHook|wrapper' web/packages/agenta-entities/tests web/packages/agenta-entities/src 2>/dev/null | head -240Repository: Agenta-AI/agenta
Length of output: 28602
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- query-client helpers and test setup candidates ---'
rg -n -C 4 'getHostQueryClient|createQueryClient|queryClientAtom|QueryClientProvider|new QueryClient' \
web/packages/agenta-shared web/packages/agenta-entities/tests web/packages/agenta-entities/src \
-g '*.{ts,tsx}' 2>/dev/null | head -320
printf '%s\n' '--- package test helper files ---'
find web/packages/agenta-entities/tests web/packages/agenta-shared -maxdepth 4 -type f \
\( -iname '*setup*' -o -iname '*helper*' -o -iname '*query*' \) -print | sort | head -160Repository: Agenta-AI/agenta
Length of output: 32229
Use the shared host query client instead of constructing new QueryClient(...) in this test. The web convention prohibits direct QueryClient construction.
Source: Coding guidelines
There was a problem hiding this comment.
Keeping the local client here, deliberately.
The contract in web/AGENTS.md exists so that package code never addresses an orphan cache: whoever hosts must hydrate queryClientAtom with the same object that package code will reach through getHostQueryClient(). This test is the host, and it does exactly that: it creates one client per test and sets it on queryClientAtom, so loadAgentModelCandidates resolves that client and the assertions are about the cache it really used. The invariant holds.
Reaching for the process-wide singleton instead would share one cache across test files, which is what these cases must not do. Each one seeds a specific cached value, an empty catalog or a null subscription answer, and asserts how many requests follow. A shared cache would make them order dependent.
It is also the established pattern for this suite. create-ephemeral-app-from-template.test.ts, session-interaction-answer.test.ts, commit-invalidations.test.ts, session-mounts-store.test.ts and others construct a client the same way and hydrate the atom with it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance ---'
if [ -f web/AGENTS.md ]; then
rg -n -C 4 'QueryClient|queryClientAtom|getHostQueryClient|orphan cache|test' web/AGENTS.md
else
echo 'web/AGENTS.md not found'
fi
printf '%s\n' '--- target test ---'
cat -n web/packages/agenta-entities/tests/unit/harness-catalog-recovery.test.ts | sed -n '40,106p'
printf '%s\n' '--- host query-client symbols and atom hydration ---'
rg -n -C 5 '\bgetHostQueryClient\b|\bqueryClientAtom\b' web/packages -g '*.{ts,tsx}'
printf '%s\n' '--- cited test patterns ---'
for name in \
create-ephemeral-app-from-template.test.ts \
session-interaction-answer.test.ts \
commit-invalidations.test.ts \
session-mounts-store.test.ts
do
fd -a -t f "$name" web/packages | while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 5 '\bnew QueryClient\b|\bqueryClientAtom\b' "$file"
done
doneLength of output: 238030
@mmabrouk, the atom hydration is correct. It ensures that loadAgentModelCandidates() uses the cache that the test seeds.
However, web/AGENTS.md also explicitly requires the shared @agenta/shared/api queryClient singleton for a test harness. It states: “Never construct your own new QueryClient().” The local client therefore does not meet the documented host contract.
The cited tests use the same local-client pattern, but they do not establish an exception to that rule. Please use the shared host client and clear or reset its cache and defaults during test setup and cleanup.
You are interacting with an AI system.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/packages/agenta-entities/src/workflow/state/agentModelCandidates.ts (2)
66-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep each source comment to one short line.
The new explanatory comments use multi-line blocks. Apply the same one-line format at every listed site.
web/packages/agenta-entities/src/workflow/state/agentModelCandidates.ts#L66-L68: shorten the empty-catalog explanation.web/packages/agenta-entities/src/workflow/state/agentModelCandidates.ts#L95-L103: shorten the subscription-unknown explanation.web/packages/agenta-entities/src/workflow/state/agentModelCandidates.ts#L130-L131: shorten the cached-catalog explanation.web/packages/agenta-entities/tests/unit/agent-model-candidate-sources.test.ts#L194-L195: shorten the unreadable-answer explanation.Source: Coding guidelines
166-172: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winLimit recovery refetches to invalid cached data.
When the cold-load
ensureQueryDatarequest returns an empty catalog,harnessCatalogIsUsable(data)is false andfetchQuerysends a second request. Read the cache beforeensureQueryData, and refetch only when an existing cached value is unusable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 8fec56de-b092-4a28-8551-21601d14cb39
📒 Files selected for processing (2)
web/packages/agenta-entities/src/workflow/state/agentModelCandidates.tsweb/packages/agenta-entities/tests/unit/agent-model-candidate-sources.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review.
|
Re-verified on the preview after the later commits, with a probe that counts every banner instance rather than the first one. The playground mounts the banner more than once, and two of the instances sit in collapsed wrappers, so a first-match probe can report either answer. My earlier readings were only trustworthy because screenshots stood beside them. Same preview project, minutes apart, on the build that carries the empty-catalog commit: So the message still appears when the project really has no key, and the composer is still disabled there. Screenshots for both are on the dev box under The preview's runner answers subscription status properly, One correction to my first comment on this PR. The empty-catalog theory does not explain the session in the issue. The QA engineer reproduced the banner in a brand new browser profile whose cached catalog holds all three harnesses, on a project whose vault holds two configured provider keys. I also pulled the deployed staging chunk and the compiled rule matches this branch. So the three inputs I can see cannot produce that banner, and one of them must reach the app differently than it reaches a raw fetch. That project's transformed vault rows are the next measurement, and the issue should stay open until they explain it. |
|
Second and third Codex passes, and where I disagreed with one of them. Pass two, on the empty-catalog commit. "Keep both guards, but do not ship unchanged." It reproduced, against the real query cache, that rejecting the empty map at the boundary does not recover a copy an older build already persisted: the refetch error was swallowed while Pass three, on the unknown-subscription commit. "Request changes." Turning an unreadable answer into a candidate error blocked agent creation, which returns null for any non-ready state, and left the model picker on its loading label. That is why the state now carries Pass four, on the reshape. Three P2 items. Two are in: one classifier now decides whether an answer left the pairs unknown, so a cached The third I did not take, and this is a judgement call worth a second opinion. Codex argues Also left for later, both P2 and both pre-existing: the model picker keeps a subscription-backed selection when the subscription source is unknown, and a settled required-source failure deserves a factual state with Retry rather than the loading label. |
1 participant
Context
Issue: #6660
The playground keeps asking for a model provider key after one has been added, while the agent runs on that same key. Two independent surfaces say it, and they were wrong for different reasons.
The Model section, reproduced on staging. Fresh project, Settings, AI providers, Add provider, OpenAI, paste a working key, Test (
133 models fetched), Done. The provider row lists the key. Back in the playground the Model section still shows the amberConnect keybadge and its "Connect the model's provider key to run this agent." tooltip, across reloads, while the run reaches OpenAI. Vault secrets are write-only on staging and on dev stacks, so/secrets/answers with no value at all:Presence rides on
value_status.configured, which the transform surfaces ashasKey, andhasStoredKeyis the one rule that reads it. The section tested!providerVaultEntry.keyinstead, which is true for every write-only row, so a connected project read as keyless forever.The composer banner is a project-wide count of runnable model routes. Two of its inputs could be read as a definitive "nothing is runnable" when they had established no such thing, and each of those raises the add-a-key message on a claim we never made.
Changes
The provider-key prompt asks
hasStoredKey:It lives in
shouldPromptForProviderKey(providerKeyPrompt.ts) with its exemptions unchanged. Aself_managedconnection signs itself in, and a namedagentaconnection points at one vault record that this rule never looks up, so a missing standard key for the family says nothing about it.vaultLoadedstill gates everything, so nothing prompts while the vault query is pending.An empty harness catalog is no longer a catalog. Every route is built by asking a harness what it supports, so an empty capability map answers "nothing is runnable" for a project whose vault holds working keys.
fetchHarnessCapabilitiesbuilt that map from any 200 that carried no harnesses, and the catalog query persists to IndexedDB, so one bad answer outlived reloads. It now rejects instead. A copy an older build already wrote to disk needs three more guards, because it keepsdatadefined and therefore looked like a catalog downstream: candidate resolution treats it as unresolved, the capability atom and the candidate atom surface the refetch error instead of swallowing it (which brings back the catalog-unavailable notice and its Retry), and the imperative loader forces one refetch.An unknown subscription source no longer reads as "no subscriptions."
fetchSubscriptionStatusreturnsnullfor an answer that fails the boundary schema, and the service answersincompatiblefor a runner whose shape it could not read. Both were counted as absence. The state now carriessubscriptionUnknownand staysready, so the routes we do know about keep working: agent creation, the model picker and the slash commands all key offstatus, and turning this into an error blocked creation outright. Only the reading of an EMPTY list changes.connectModelGatestands down when the source is unknown, which is the one place the claim was being made. A check that could not be made travels the same way, so the two unknowns no longer behave differently. One classifier decides all of this, and the imperative loader uses it to refetch such an answer once, becauseensureQueryDataserves whatever is cached and a retry after the runner recovered made no request at all.unavailabledeliberately stays a true negative, and this one is a judgement call. The service also answers it for a deployment with no runner configured, which is the common self-hosted case, so reading it as unknown would silence the add-a-key prompt for exactly the users who need it. With no vault key and no reachable runner nothing is runnable, and adding a provider key is the remedy, so the message is still correct advice there. Giving the service the vocabulary to tell a confirmed absence from a failed check belongs in its own change.One comment in the vault persister claimed
!!secret.keywas the presence rule. It is not, and it now points athasStoredKey.Tests
packages/agenta-entity-ui/tests/unit/providerKeyPrompt.test.tscovers the row shapes the vault really serves: unconnected, write-only connected, readable connected, both restored from IndexedDB, and a record that says the key is gone while a stale value lingers. It fails against the old rule.packages/agenta-entities/tests/unit/agent-model-candidate-sources.test.tsgains the empty catalog, a failure beside a cached empty map, an unreadable subscription answer, a runner reportedincompatible, a runner reportedunavailable, and a connected runner reportingunsupportedfor a harness.packages/agenta-entities/tests/unit/harness-catalog-recovery.test.tsdrives the real query cache: a cached empty map and a cached unreadable subscription answer are both refetched and recover, their failures are reported, and usable cached values are still served without a request.web/oss/src/components/AgentChatSlice/hooks/useAgentModelKeyStatus.test.tspins the gate standing down on an unknown subscription source.pnpm --filter @agenta/entities exec vitest run tests/unit: 1614 passed.@agenta/entity-ui: 695.@agenta/chat: 894.@agenta/playground: 288.pnpm lint-fixandpnpm run format: clean.unavailablesplit above, all noted in the comments.Browser verification
Before, on staging v0.115.3, with a real OpenAI key: the
Connect keybadge stands over a working key across reloads. After, on this PR's preview, same vault row shape:Screenshots and the probe are on the dev box under
~/agenta-qa-evidence/2026-09-08-issue-6660/. The playground mounts the banner more than once, so a first-match DOM probe reports either answer at random; the probe counts visible instances.What to QA
Connect keybadge, and no "Connect the model's provider key" tooltip on the section header.Add your model provider key to run this agent.line.