feat(deploy): --mode cloud (OSS-generic persona+bundle POST)#102
Merged
Conversation
Track H: Track H — Workforce (OSS-generic implementation) See workforce/docs/plans/deploy-v1-schema-cascade-spec.md
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements cloud deployment mode: extends CLI login and deploy parsing, adds token-based workspace auth with browser OAuth and persistent storage, forwards cloud options through deploy orchestration, implements a full cloud launcher (credentials, integrations, existing-persona handling, polling, stop), and adds comprehensive tests. ChangesCloud Deploy Feature
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs:
🚥 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)
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: 1
🧹 Nitpick comments (2)
packages/cli/src/deploy-command.ts (1)
11-11: 💤 Low valueConsider extracting DEFAULT_CLOUD_URL to a shared constant.
DEFAULT_CLOUD_URLis duplicated here and incloud.ts. While acceptable for now, a shared constant would prevent drift.🤖 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/deploy-command.ts` at line 11, Extract the duplicated DEFAULT_CLOUD_URL constant into a single shared exported constant (e.g., export const DEFAULT_CLOUD_URL) in a new or existing shared module (e.g., a constants/shared module), then update both references in deploy-command.ts (currently using DEFAULT_CLOUD_URL) and cloud.ts to import and use that shared constant; ensure you remove the local literal and export the single source of truth so both modules import the same identifier DEFAULT_CLOUD_URL.packages/deploy/src/login.ts (1)
167-176: 💤 Low valueTokens in URL query parameters are logged to browser history.
OAuth tokens (
access_token,refresh_token) arrive via query parameters, which browsers persist in history. Since this is a localhost callback for CLI auth, exposure risk is low, but consider documenting this or clearing browser history programmatically if security requirements tighten.🤖 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 167 - 176, The callback handler extracts tokens from requestUrl (token, refreshToken, expiresAt, cloudUrl) and currently leaves them in the browser history; update the login callback response generation (the code around token / refreshToken extraction in packages/deploy/src/login.ts) so the HTTP response served to the browser includes a small client-side script that immediately clears the query string (use window.history.replaceState to remove access_token/refresh_token/expires_at from the URL) before rendering any UI or closing the tab, ensuring tokens are not retained in browser history; keep server-side token handling unchanged but ensure the response includes this JS action and any minimal UX to run it.
🤖 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/modes/cloud.ts`:
- Around line 813-828: The JSON.parse call in readInputsOverride should be
wrapped in a try-catch so malformed JSON yields a clearer error; catch the parse
error around JSON.parse(raw) and rethrow a new Error that mentions
WORKFORCE_DEPLOY_INPUTS_JSON is invalid and includes the original error.message
(and optionally the raw value) for context, while preserving the subsequent type
checks and the existing error shapes thrown for non-object or non-string values.
---
Nitpick comments:
In `@packages/cli/src/deploy-command.ts`:
- Line 11: Extract the duplicated DEFAULT_CLOUD_URL constant into a single
shared exported constant (e.g., export const DEFAULT_CLOUD_URL) in a new or
existing shared module (e.g., a constants/shared module), then update both
references in deploy-command.ts (currently using DEFAULT_CLOUD_URL) and cloud.ts
to import and use that shared constant; ensure you remove the local literal and
export the single source of truth so both modules import the same identifier
DEFAULT_CLOUD_URL.
In `@packages/deploy/src/login.ts`:
- Around line 167-176: The callback handler extracts tokens from requestUrl
(token, refreshToken, expiresAt, cloudUrl) and currently leaves them in the
browser history; update the login callback response generation (the code around
token / refreshToken extraction in packages/deploy/src/login.ts) so the HTTP
response served to the browser includes a small client-side script that
immediately clears the query string (use window.history.replaceState to remove
access_token/refresh_token/expires_at from the URL) before rendering any UI or
closing the tab, ensuring tokens are not retained in browser history; keep
server-side token handling unchanged but ensure the response includes this JS
action and any minimal UX to run it.
🪄 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: 32ec386a-c5b6-4ab2-bacb-203a1ba62951
📒 Files selected for processing (9)
packages/cli/src/deploy-command.tspackages/deploy/src/deploy.test.tspackages/deploy/src/deploy.tspackages/deploy/src/index.tspackages/deploy/src/login.tspackages/deploy/src/modes/cloud.test.tspackages/deploy/src/modes/cloud.tspackages/deploy/src/modes/sandbox.tspackages/deploy/src/types.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+244
to
+249
| if (stdout.trim()) { | ||
| return { | ||
| workspace, | ||
| token: stdout.trim() | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🔴 readMacKeychainLogin returns raw JSON string as auth token when stored workspace doesn't match
When parseStoredLogin successfully parses a keychain entry but the stored workspace doesn't match the requested one, the code falls through to the if (stdout.trim()) check and returns the entire raw JSON string as the token. For example, if the keychain stores {"workspace":"other-ws","token":"real-token"} under the generic agentworkforce service name, and the caller requests workspace my-ws, the function returns { workspace: "my-ws", token: '{"workspace":"other-ws","token":"real-token"}' }. This serialized JSON object is then used as a Bearer token in HTTP Authorization headers, causing 401 errors with confusing messages. The fix should continue to the next service name when parsed is non-null but the workspace doesn't match, reserving the raw-stdout-as-token fallback for cases where parseStoredLogin returns null (non-JSON values).
Suggested change
| if (stdout.trim()) { | |
| return { | |
| workspace, | |
| token: stdout.trim() | |
| }; | |
| } | |
| if (parsed) continue; | |
| if (stdout.trim()) { | |
| return { | |
| workspace, | |
| token: stdout.trim() | |
| }; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Member
Author
There was a problem hiding this comment.
Already addressed in 0a9d893 (morning sweep). readMacKeychainLogin now uses if (parsed) continue; to skip generic-service keychain entries that parse cleanly but whose workspace field doesn't match — preventing the raw-JSON-as-bearer-token fallback. Reaffirmed clean on the post-rebase commit 0db4fef.
- login: skip keychain entry when stored workspace mismatches instead of returning the raw JSON blob as a bearer token (devin-ai) - cloud: wrap WORKFORCE_DEPLOY_INPUTS_JSON parse in try/catch for a clearer error message on malformed JSON (coderabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 tasks
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 `@packages/persona-kit/schemas/persona.schema.json`:
- Around line 454-456: The schema allows an empty workspace_service_account
name; update the persona.schema.json schema for the workspace_service_account ->
name property to forbid empty strings by adding a validation constraint (e.g.,
"minLength": 1 or a non-empty pattern) to the "name" property so that "" no
longer validates; keep the existing "type": "string" and only add the non-empty
constraint to the "name" field to enforce user-supplied service account names.
🪄 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: 4c59fab3-85b2-4303-b9e2-eaf486302e9b
📒 Files selected for processing (6)
packages/cli/src/deploy-command.tspackages/deploy/src/index.tspackages/deploy/src/modes/input-values.test.tspackages/deploy/src/modes/sandbox.tspackages/deploy/src/types.tspackages/persona-kit/schemas/persona.schema.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/deploy/src/types.ts
- packages/deploy/src/modes/sandbox.ts
- packages/cli/src/deploy-command.ts
…ce_account.name
CodeRabbit (cloud#102 review) flagged that the generated JSON schema
accepted empty strings for `integrations.<provider>.source.name` when
`kind === "workspace_service_account"` — even though parse.ts has
rejected those at runtime since the source-discriminator landed.
The generator emits a bare `{ "type": "string" }` because the constraints
live in the parser (INTEGRATION_SOURCE_NAME_RE, max 64) and the TS source
type is a union literal whose `name` field has no annotatable position
for ts-json-schema-generator to pick up.
Mirror the three parser rules at the schema layer via a post-process walk
in emit-schema.mjs:
"name": {
"type": "string",
"minLength": 1,
"maxLength": 64,
"pattern": "^[a-z0-9]+(?:-[a-z0-9]+)*$"
}
Now schema-validation rejects malformed names with the same precision the
parser does, instead of pushing the failure to deploy-time.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Spec reference
Source spec: workforce/docs/plans/deploy-v1-schema-cascade-spec.md
Track section: Track H — Workforce (OSS-generic implementation)
Final signoff
SIGNOFF_FINAL: COMPLETE Track-H
Final gate (typecheck + tests)
Self-reflection report
Known gaps after this PR
Co-Authored-By: Ricky deploy-v1 schema cascade noreply@agentworkforce.com