Deploy v1 CLI runtime credentials and customer example#109
Merged
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughImplements deploy-v1 cloud auth and credential flows, Relayfile-backed integration resolver, runtime FilesContext and cloud memory, GitHub PR client, CLI runtime/deployments/login/logout, Notion→essay example + persona, extensive tests, and onboarding/docs. ChangesDeploy-v1 Feature Complete
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 (1)
packages/cli/src/cli.ts (1)
2148-2158: ⚡ Quick winRemove the duplicate deployments dispatch path in
runList.The early
args.includes('--deployments')branch already routes and exits, so the laterif (deployments)block is redundant and can drift over time.♻️ Suggested simplification
async function runList(args: readonly string[]): Promise<never> { if (args.includes('--deployments')) { await runDeploymentList(args.filter((arg) => arg !== '--deployments')); process.exit(0); } - const { json, filterHarness, filterTag, display, deployments } = parseListArgs(args); - if (deployments) { - await runDeploymentList(args.filter((arg) => arg !== '--deployments')); - process.exit(0); - } + const { json, filterHarness, filterTag, display } = parseListArgs(args);🤖 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 `@packages/cli/src/cli.ts` around lines 2148 - 2158, In runList, remove the redundant second dispatch that checks `if (deployments)` and calls `runDeploymentList`/`process.exit(0)` because the earlier `if (args.includes('--deployments'))` branch already handles that; locate the `runList` function and the `if (deployments)` block (after the call to `parseListArgs`) and delete that block so only the initial `args.includes('--deployments')` branch routes to `runDeploymentList` (references: runList, parseListArgs, runDeploymentList).
🤖 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 `@docs/plans/deploy-v1-credentials-runtime-checklist.md`:
- Line 3: Replace the machine-specific absolute path after the "Source spec:"
label with a repo-relative path to the spec file (e.g.,
"docs/plans/deploy-v1-credentials-and-runtime-spec.md") so the reference
resolves for all contributors; update the literal string following "Source
spec:" to the repo-relative path format and ensure it points to the correct file
within the repository.
In `@packages/deploy/src/connect.ts`:
- Around line 89-123: The isConnected and connect functions are ignoring the
resolver's workspace argument and always use opts.workspaceId; update both
functions to prefer the passed-in workspace value (the function parameter
object, e.g. the workspace property in isConnected({ provider, workspace }) and
connect({ provider, workspace })) when building URLs and query params (use
encodeURIComponent(workspace) if present, otherwise fall back to
opts.workspaceId), including the connect-session POST URL and the status URL
(and when setting connectionId query param). Ensure all uses of opts.workspaceId
in isConnected, connect (session creation, sessionUrl handling, and status
polling) are replaced with this workspace-aware value.
In `@packages/runtime/src/ctx.ts`:
- Around line 242-277: The save and recall methods perform fetch calls without
timeouts; add an AbortController-based timeout for both functions (e.g., create
const controller = new AbortController(); set a const timer = setTimeout(() =>
controller.abort(), timeoutMs) where timeoutMs is a reasonable default or
configurable), pass controller.signal to fetch (in save: fetch(endpoint, { ...,
signal: controller.signal }), in recall: fetch(url, { headers: ..., signal:
controller.signal })), clearTimeout(timer) after the fetch completes
successfully or in finally, and update the catch blocks to treat an aborted
request (AbortError) as a timeout while logging via args.log and returning
undefined as before.
---
Nitpick comments:
In `@packages/cli/src/cli.ts`:
- Around line 2148-2158: In runList, remove the redundant second dispatch that
checks `if (deployments)` and calls `runDeploymentList`/`process.exit(0)`
because the earlier `if (args.includes('--deployments'))` branch already handles
that; locate the `runList` function and the `if (deployments)` block (after the
call to `parseListArgs`) and delete that block so only the initial
`args.includes('--deployments')` branch routes to `runDeploymentList`
(references: runList, parseListArgs, runDeploymentList).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e9873a-e914-499f-bf88-40a380bc0fd9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
docs/customers/proactive-agents-onboarding.mddocs/plans/deploy-v1-credentials-runtime-checklist.mdexamples/notion-essay-pr/agent.tsexamples/notion-essay-pr/notion-essay-pr.smoke.test.tsexamples/notion-essay-pr/persona.jsonpackages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/deploy-command.tspackages/cli/src/list-command.test.tspackages/cli/src/list-command.tspackages/cli/src/runtime-picker.test.tspackages/cli/src/runtime-picker.tspackages/deploy/src/connect.test.tspackages/deploy/src/connect.tspackages/deploy/src/deploy.test.tspackages/deploy/src/deploy.tspackages/deploy/src/index.tspackages/deploy/src/login.test.tspackages/deploy/src/login.tspackages/deploy/src/modes/cloud.test.tspackages/deploy/src/modes/cloud.tspackages/deploy/src/modes/input-values.test.tspackages/deploy/test/e2e/notion-essay-pr.smoke.test.tspackages/runtime/src/clients/github.test.tspackages/runtime/src/clients/github.tspackages/runtime/src/ctx.test.tspackages/runtime/src/ctx.tspackages/runtime/src/index.tspackages/runtime/src/proactive.test.tspackages/runtime/src/runner.tspackages/runtime/src/types.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
added 2 commits
May 13, 2026 14:15
…runtime-workforce # Conflicts: # packages/cli/src/cli.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/deploy/src/login.ts (1)
275-277: 💤 Low valueDeleting object entries during iteration is safe but consider clarity.
The code deletes entries from
tokenswhile iterating overObject.entries(tokens)at lines 275-277. In JavaScript,Object.entries()creates a snapshot of entries before iteration, so this is safe. However, using a filter or reduce pattern would make the intent clearer and avoid potential confusion.♻️ Alternative approach for clarity
- for (const [key, login] of Object.entries(tokens)) { - if (key === target || workspaceMatches(login, target)) delete tokens[key]; - } + const tokens = Object.fromEntries( + Object.entries(current.workforce?.workspaceTokens ?? {}) + .filter(([key, login]) => !(key === target || workspaceMatches(login, target))) + );🤖 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 `@packages/deploy/src/login.ts` around lines 275 - 277, The loop deletes entries from the tokens object during an Object.entries() iteration which is safe but unclear; replace it by building a new tokens object using a filter/reduce pattern (e.g., use Object.fromEntries(Object.entries(tokens).filter(...)) or a reduce) that keeps only entries where the key !== target && !workspaceMatches(login, target), so update the code that references tokens (the variable named tokens and the workspaceMatches(login, target) check) to assign the filtered object back to tokens instead of deleting while iterating.packages/deploy/src/modes/cloud.ts (3)
591-601: ⚡ Quick winProvider detection might incorrectly match substring patterns.
The
deriveModelProviderfunction usesincludes()checks on the lowercase model string (lines 594-597), which could produce false positives. For example:
- A model named
"my-openai-alternative"would match"openai"- A model named
"pseudo-anthropic"would match"anthropic"After the substring checks, line 599 splits on
/or:and takes the first part, but if the model string doesn't contain these delimiters, it returns the entire lowercased model name. If that's empty or whitespace-only, it falls back topersona.harnessat line 600.Consider adding word boundary checks or using exact prefix matching to reduce false positives.
♻️ More precise provider detection
function deriveModelProvider(persona: PersonaSpec): string { const model = typeof persona.model === 'string' ? persona.model.trim() : ''; + if (!model) return persona.harness; const lower = model.toLowerCase(); - if (lower.includes('anthropic') || lower.includes('claude')) return 'anthropic'; - if (lower.includes('openai') || lower.includes('codex') || lower.includes('gpt')) return 'openai'; - if (lower.includes('google') || lower.includes('gemini')) return 'google'; - if (lower.includes('openrouter') || lower.includes('opencode')) return 'openrouter'; + // Check provider prefixes first + if (lower.startsWith('anthropic/') || lower.startsWith('anthropic:') || lower.startsWith('claude')) return 'anthropic'; + if (lower.startsWith('openai/') || lower.startsWith('openai:') || lower.startsWith('gpt') || lower.startsWith('codex')) return 'openai'; + if (lower.startsWith('google/') || lower.startsWith('google:') || lower.startsWith('gemini')) return 'google'; + if (lower.startsWith('openrouter/') || lower.startsWith('openrouter:')) return 'openrouter'; + // Fall back to substring checks for compatibility + if (lower.includes('anthropic') || lower.includes('claude')) return 'anthropic'; + if (lower.includes('openai') || lower.includes('gpt')) return 'openai'; const [provider] = model.split(/[/:]/, 1); if (provider?.trim()) return provider.trim().toLowerCase(); return persona.harness; }🤖 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 `@packages/deploy/src/modes/cloud.ts` around lines 591 - 601, The deriveModelProvider function currently uses loose substring includes() checks that can false-match names like "my-openai-alternative"; update deriveModelProvider to use stricter token/prefix matching (e.g., regex with word boundaries or start-of-string + (end|separator) checks) for the known providers ("anthropic", "claude", "openai", "codex", "gpt", "google", "gemini", "openrouter", "opencode"), and only fall back to splitting the model on / or : and returning the first token (trimmed and lowercased) if no strict provider match is found; ensure the final fallback still returns persona.harness when the token is empty.
172-173: 💤 Low valueRedundant field names sent for compatibility.
Both
credentialSelections(camelCase) andcredential_selections(snake_case) are sent in the deploy payload at lines 172-173. Similarly, the BYOK endpoint sends bothmodelProvider/model_providerandkey/api_keyat lines 580-583. While this redundancy ensures compatibility with backend variations, it's worth documenting why both forms exist or consolidating once the backend contract stabilizes.🤖 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 `@packages/deploy/src/modes/cloud.ts` around lines 172 - 173, The payload currently sends duplicate fields for compatibility—credentialSelections and credential_selections (and similarly modelProvider/model_provider and key/api_key) —so add a short in-code comment at the payload construction site explaining that both camelCase and snake_case keys are intentionally included to support mixed backend contracts and that duplicates should be removed once the API is standardized; alternatively, if you can guarantee the backend contract, consolidate to a single naming style by removing the redundant keys (update places where credentialSelections, credential_selections, modelProvider, model_provider, key, and api_key are set).
694-710: ⚡ Quick winToken refresh failure silently returns null, losing error context.
At line 698, when
refreshStoredAuthfails, the error is caught andnullis returned silently. This loses valuable error context that could help debug authentication issues. The caller then proceeds as if no auth exists, which may lead to confusing "run workforce login" messages even when the user is logged in but refresh failed for another reason (e.g., network issue, revoked token).Consider logging the refresh failure or propagating specific error types.
🔍 Log refresh failures for better diagnostics
async function readUsableCloudAuth(apiUrl: string): Promise<StoredAuth | null> { let auth = await cloudCredentialDeps.readStoredAuth().catch(() => null); if (!auth) return null; if (isAuthExpired(auth.accessTokenExpiresAt)) { - auth = await cloudCredentialDeps.refreshStoredAuth(auth).catch(() => null); + auth = await cloudCredentialDeps.refreshStoredAuth(auth).catch((err) => { + // Log refresh failure for diagnostics, then proceed as if no auth + console.warn(`cloud: token refresh failed: ${err instanceof Error ? err.message : String(err)}`); + return null; + }); } if (!auth) return null; return { ...auth, apiUrl }; }🤖 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 `@packages/deploy/src/modes/cloud.ts` around lines 694 - 710, readUsableCloudAuth is silently swallowing errors from cloudCredentialDeps.refreshStoredAuth which hides why refresh failed; update readUsableCloudAuth so the catch around cloudCredentialDeps.refreshStoredAuth captures the thrown error (e) and either (1) logs it with context (e.g., processLogger.error or a module logger) including e.message and e.stack before returning null, or (2) only swallow and return null for known "no-stored-auth" / expired-token cases but rethrow or wrap and throw for other errors (network, revoked token) so callers can differentiate; reference the readUsableCloudAuth function and cloudCredentialDeps.refreshStoredAuth (and isAuthExpired) when making the change.
🤖 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 `@packages/deploy/src/login.ts`:
- Line 224: The current line auth = await refreshStoredAuth(auth).catch(() =>
auth) silently swallows refresh failures and continues with possibly expired
credentials; replace this with explicit error handling in the calling function
(e.g., wrap refreshStoredAuth(auth) in try/catch) so that on failure you either
throw the refresh error to fail fast or, if you must fallback, first verify the
original auth is still valid (check expiry fields on the auth object) and only
use it when unexpired; update the code paths referencing auth to handle the
thrown error or validated fallback accordingly (referencing refreshStoredAuth
and the surrounding login logic that consumes auth).
In `@packages/runtime/src/cloud-defaults.ts`:
- Around line 436-453: The fetch to usageUrl when reporting harness usage
currently has no timeout and can hang the runner; modify the send so it uses an
AbortController with a short timeout (e.g., few seconds), pass controller.signal
into the fetch options, and ensure the call is wrapped in a try/catch that
ignores or logs abort/timeouts (so the harness run doesn't block); update the
fetch invocation that uses token and body built from args (workspaceId,
deploymentId, agentId, personaId, harness, model, durationMs, exitCode, usage)
to include the signal and clear the timeout on completion.
---
Nitpick comments:
In `@packages/deploy/src/login.ts`:
- Around line 275-277: The loop deletes entries from the tokens object during an
Object.entries() iteration which is safe but unclear; replace it by building a
new tokens object using a filter/reduce pattern (e.g., use
Object.fromEntries(Object.entries(tokens).filter(...)) or a reduce) that keeps
only entries where the key !== target && !workspaceMatches(login, target), so
update the code that references tokens (the variable named tokens and the
workspaceMatches(login, target) check) to assign the filtered object back to
tokens instead of deleting while iterating.
In `@packages/deploy/src/modes/cloud.ts`:
- Around line 591-601: The deriveModelProvider function currently uses loose
substring includes() checks that can false-match names like
"my-openai-alternative"; update deriveModelProvider to use stricter token/prefix
matching (e.g., regex with word boundaries or start-of-string + (end|separator)
checks) for the known providers ("anthropic", "claude", "openai", "codex",
"gpt", "google", "gemini", "openrouter", "opencode"), and only fall back to
splitting the model on / or : and returning the first token (trimmed and
lowercased) if no strict provider match is found; ensure the final fallback
still returns persona.harness when the token is empty.
- Around line 172-173: The payload currently sends duplicate fields for
compatibility—credentialSelections and credential_selections (and similarly
modelProvider/model_provider and key/api_key) —so add a short in-code comment at
the payload construction site explaining that both camelCase and snake_case keys
are intentionally included to support mixed backend contracts and that
duplicates should be removed once the API is standardized; alternatively, if you
can guarantee the backend contract, consolidate to a single naming style by
removing the redundant keys (update places where credentialSelections,
credential_selections, modelProvider, model_provider, key, and api_key are set).
- Around line 694-710: readUsableCloudAuth is silently swallowing errors from
cloudCredentialDeps.refreshStoredAuth which hides why refresh failed; update
readUsableCloudAuth so the catch around cloudCredentialDeps.refreshStoredAuth
captures the thrown error (e) and either (1) logs it with context (e.g.,
processLogger.error or a module logger) including e.message and e.stack before
returning null, or (2) only swallow and return null for known "no-stored-auth" /
expired-token cases but rethrow or wrap and throw for other errors (network,
revoked token) so callers can differentiate; reference the readUsableCloudAuth
function and cloudCredentialDeps.refreshStoredAuth (and isAuthExpired) when
making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef88374c-6eb1-4664-bdd5-5ad9dd706228
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
docs/plans/deploy-v1-credentials-runtime-checklist.mdexamples/notion-essay-pr/persona.jsonpackages/cli/src/cli.tspackages/cli/src/deploy-command.test.tspackages/cli/src/deploy-command.tspackages/deploy/package.jsonpackages/deploy/src/connect.test.tspackages/deploy/src/connect.tspackages/deploy/src/deploy.tspackages/deploy/src/index.tspackages/deploy/src/login.tspackages/deploy/src/modes/cloud.test.tspackages/deploy/src/modes/cloud.tspackages/runtime/src/cloud-defaults.tspackages/runtime/src/ctx.test.tspackages/runtime/src/ctx.tspackages/runtime/src/runner.test.tspackages/runtime/src/runner.tspackages/runtime/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- examples/notion-essay-pr/persona.json
- packages/runtime/src/types.ts
- packages/runtime/src/ctx.test.ts
- packages/deploy/src/connect.test.ts
- packages/deploy/src/connect.ts
- packages/cli/src/deploy-command.ts
- packages/deploy/src/deploy.ts
- packages/cli/src/cli.ts
- packages/runtime/src/ctx.ts
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Merge after cloud PR https://github.com/AgentWorkforce/cloud/pull/580 is deployed, then publish the CLI/onboarding docs.