Guard subscription deploys without cloud credentials#210
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR extends proactive agent deployment to support user-supplied LLM provider credentials via a new optional ChangesSubscription-based proactive agent deployment with credential gating
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'useSubscription' option for proactive agents, allowing users to run inference using their own connected LLM provider credentials. It updates the agent manager, types, and editor UI to support this toggle, and implements validation to block deployment with a helpful connection command if credentials are missing. A new DOM test suite is also added to verify this deployment guard. The reviewer suggested refactoring the 'subscriptionProviderForDraft' helper function in 'ProactiveAgentEditor.tsx' to use a more data-driven mapping approach and eliminate redundant code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function subscriptionProviderForDraft(draft: ProactiveAgentDraft): string { | ||
| const model = draft.model.trim().toLowerCase() | ||
| if (/(anthropic|claude)/.test(model)) return 'anthropic' | ||
| if (/(openai|codex|gpt)/.test(model)) return 'openai' | ||
| if (/(google|gemini)/.test(model)) return 'google' | ||
| if (/(openrouter|opencode)/.test(model)) return 'openrouter' | ||
|
|
||
| if (draft.harness === 'claude') return 'anthropic' | ||
| if (draft.harness === 'codex') return 'openai' | ||
| if (draft.harness === 'opencode') return 'openrouter' | ||
| return draft.harness | ||
| } |
There was a problem hiding this comment.
This function can be refactored to be more data-driven and avoid repetition, which will make it easier to maintain and extend in the future. You can use maps for model-to-provider and harness-to-provider lookups. Also, the final return draft.harness is currently unreachable because all possible values of draft.harness are handled by the preceding if statements.
function subscriptionProviderForDraft(draft: ProactiveAgentDraft): string {
const model = draft.model.trim().toLowerCase()
const modelProviderMap: Array<[RegExp, string]> = [
[/(anthropic|claude)/, 'anthropic'],
[/(openai|codex|gpt)/, 'openai'],
[/(google|gemini)/, 'google'],
[/(openrouter|opencode)/, 'openrouter']
]
for (const [regex, provider] of modelProviderMap) {
if (regex.test(model)) return provider
}
const harnessProviderMap: Record<Harness, string> = {
claude: 'anthropic',
codex: 'openai',
opencode: 'openrouter'
}
return harnessProviderMap[draft.harness]
}
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
khaliqgant
left a comment
khaliqgant
left a comment
There was a problem hiding this comment.
Code Review: AR-105 — Guard subscription deploys without cloud credentials
Verdict: LGTM — the core behavior is correct on all four criteria. Three minor points below; none block merge.
Checklist
| Check | Result |
|---|---|
| Credentials-absent condition correctly detected | ✅ |
| Deploy actually blocked (not just a warning) | ✅ |
| Command is accurate and copy-paste ready | ✅ (see note #3) |
| Guard only triggers on subscription selection | ✅ |
✅ Deploy is genuinely blocked
handleDeploy returns after setErrors({ deploy: subscriptionBlock }), before saveDraft() or proactiveAgent.deploy() are ever reached. createMock and deployMock stay uncalled — confirmed by the DOM test.
Minor comment 1 — markPhase('bundle', 'error') before markPhase('bundle', 'active')
// ProactiveAgentEditor.tsx ~L774
const subscriptionBlock = subscriptionCredentialBlockMessage(selectedDraft, cloudAgents)
if (subscriptionBlock) {
setErrors({ deploy: subscriptionBlock })
markPhase('bundle', 'error') // ← bundle was never 'active'
return
}Bundle transitions directly to error without ever being active. The phase UI will show bundle as crashed-from-the-start rather than a pre-flight block. Recommend either omitting the markPhase call here (the errors.deploy message is sufficient), or adding a dedicated credentials phase for this pre-bundle check.
Minor comment 2 — lastAuthenticatedAt as the credential proxy
CloudAgentRecord.lastAuthenticatedAt is a generic timestamp with no guarantee it's cleared when provider credentials are revoked or expire. If the backend only sets this once at creation, a user with expired credentials would pass the guard. If this is the intended/correct signal, a short inline comment near subscriptionCredentialBlockMessage stating the assumption would prevent future confusion (e.g. "null = no credentials have been connected; backend nulls this on revocation").
Minor comment 3 — Verbatim harness fallback in subscriptionProviderForDraft
// ProactiveAgentEditor.tsx ~L230
return draft.harness // ← could be 'custom', '', 'gemini-pro', etc.If draft.harness is an unrecognized value the generated command becomes agent-relay cloud connect <arbitrary>. Suggest either adding a 'gemini'→'google' mapping if the Google harness is used, or falling back to the literal string '<provider>' so it's obvious the command needs editing rather than silently wrong.
DOM test
Correctly verifies the inline warning appears before Deploy and the error is set on Deploy without calling create/deploy. One robustness note: the pre-click getAllByText(...) assertion runs synchronously — if the inline warning render is ever async it could flake. Wrapping in waitFor would make it more resilient.
Overall this is clean. Blocking logic is sound and the two-level UX (inline warning + deploy-time error) is the right approach.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Closing per dispatcher instruction: AR-105 belongs in AgentWorkforce/cloud, not pear. This PR will not be merged. |
1 participant
Summary
agent-relay cloud connect anthropic, inline on the deploy screenVerification
npx vitest run --project dom src/renderer/src/components/proactive/ProactiveAgentEditor.dom.test.tsnpm run typecheck:webnpm run typecheckfails in existingtypecheck:nodeerrors on origin/main (examples:src/main/broker.ts,src/main/cloud-agent.ts,src/main/store.ts, existing node tests)