fix(cli): orchestrator + list/destroy read active.json; sanitize HTML errors#118
Merged
khaliqgant merged 2 commits intoMay 13, 2026
Merged
Conversation
The deploy orchestrator was still using the legacy `envWorkspaceAuth()` default, which only consults `WORKFORCE_WORKSPACE_TOKEN` env + a long-dead keychain. A user who freshly ran `agentworkforce login` (which writes the shared @agent-relay/cloud accessToken + an active.json pointer) would hit `no workspace resolved` because that flow is invisible to the env-only resolver. PR #113 fixed the cloud launcher and list/destroy commands but missed this orchestrator entry point. The list and destroy CLI commands also defaulted to `https://agentrelay.com` (missing the `/cloud` basePath), so every API call landed on the marketing site's Next.js 404 page — and the full HTML response body was dumped verbatim into the CLI error message. Comprehensive fix: * Add `resolveCloudUrl()` as the single source of truth for cloud URL resolution (flag → env → active.json → canonical default). All three CLI commands and the orchestrator now route through it. The canonical default is now applied via `canonicalizeCloudUrl`, which also remaps the bare apex `agentrelay.com` → `agentrelay.com/cloud` to prevent the marketing-site fallthrough from ever happening again. * Add `formatHttpErrorBody()` — detects HTML response bodies and replaces them with a one-line hint, truncates long non-HTML bodies. list and destroy both use it. * Swap the orchestrator's auth default from `envWorkspaceAuth()` to `resolveWorkspaceToken()`, which respects the same env vars (Tier 1 for CI) but additionally falls through to the shared cloud-auth + active.json pointer. * Add `WORKFORCE_DISABLE_SHARED_AUTH` opt-out for hermetic tests and power users who want strictly env-only operation. Tests: 17 new (cloud-url, error-format, deploy, destroy) covering URL resolution precedence, apex canonicalization, HTML body sanitization, the deploy env-Tier 1 path, deploy noPrompt error message, destroy active.json fallback, and the test isolation hook. Smoke verified against the local build in proactive-agents: `agentworkforce deploy ... --no-prompt` now resolves the workspace from active.json, finds both notion and github already connected, and stages the bundle (failing later on harness creds, which is M3 scope). `agentworkforce deployments list` returns clean results instead of a 404 HTML wall. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors cloud URL resolution and workspace authentication across deploy and CLI. Adds resolveCloudUrl and formatHttpErrorBody, consults active workspace for cloud URL and token resolution (with a shared-auth opt-out), and updates destroy/list to use these utilities with new tests and isolation helpers. ChangesCloud URL Resolution and Auth Workspace Integration
Sequence DiagramsequenceDiagram
participant CLI as CLI (destroy/list)
participant Active as readActiveWorkspace
participant CloudURL as resolveCloudUrl
participant Auth as resolveWorkspaceToken
participant API as Cloud API
participant Formatter as formatHttpErrorBody
CLI->>Active: readActiveWorkspace()
CLI->>CloudURL: resolveCloudUrl(flag, env, active)
CloudURL-->>CLI: canonical cloud URL
CLI->>Auth: resolveWorkspaceToken(cloudUrl, workspace, noPrompt)
Auth-->>CLI: workspace + token
CLI->>API: API request (workspace, token, cloudUrl)
API-->>CLI: Response (ok or error)
alt Non-OK response
CLI->>Formatter: formatHttpErrorBody(responseText, { url, maxLength })
Formatter-->>CLI: suppressed/truncated hint
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/deploy/src/deploy.ts (1)
234-234: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPass the resolved cloud URL into launcher input.
At Line 234, using
opts.cloudUrlbypasses the resolved value from Lines 154–157. That can desync endpoint selection/canonicalization between auth/integration flows and launch.🔧 Proposed fix
- ...(opts.cloudUrl ? { cloudUrl: opts.cloudUrl } : {}), + ...(cloudUrl ? { cloudUrl } : {}),🤖 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/deploy.ts` at line 234, The launcher input is currently using opts.cloudUrl which bypasses the previously resolved/normalized cloud URL computed earlier; update the spread at ...(opts.cloudUrl ? { cloudUrl: opts.cloudUrl } : {}) to use the resolved variable produced in the earlier resolution logic (the canonical cloudUrl/resolvedCloudUrl variable set around lines 154–157) so the launcher receives the same canonical endpoint (preserve the conditional/fallback behavior when the resolved value is undefined).packages/cli/src/destroy-command.test.ts (1)
75-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFully restore cloud URL env vars in
withTokenEnvcleanup.The cleanup only restores
WORKFORCE_DEPLOY_CLOUD_URL/WORKFORCE_CLOUD_URLwhen they were previously defined. If a test mutates either var, state can leak into subsequent tests.Proposed fix
return () => { if (prevToken === undefined) delete process.env.WORKFORCE_WORKSPACE_TOKEN; else process.env.WORKFORCE_WORKSPACE_TOKEN = prevToken; if (prevWs === undefined) delete process.env.WORKFORCE_WORKSPACE_ID; else process.env.WORKFORCE_WORKSPACE_ID = prevWs; - if (prevCloudA !== undefined) process.env.WORKFORCE_DEPLOY_CLOUD_URL = prevCloudA; - if (prevCloudB !== undefined) process.env.WORKFORCE_CLOUD_URL = prevCloudB; + if (prevCloudA === undefined) delete process.env.WORKFORCE_DEPLOY_CLOUD_URL; + else process.env.WORKFORCE_DEPLOY_CLOUD_URL = prevCloudA; + if (prevCloudB === undefined) delete process.env.WORKFORCE_CLOUD_URL; + else process.env.WORKFORCE_CLOUD_URL = prevCloudB; restoreIsolate(); }; }🤖 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/destroy-command.test.ts` around lines 75 - 94, The cleanup in withTokenEnv restores WORKFORCE_DEPLOY_CLOUD_URL and WORKFORCE_CLOUD_URL only when prevCloudA/prevCloudB were defined, causing leaked env state if tests set them to undefined; update the cleanup to mirror the token/ws logic: if prevCloudA was undefined then delete process.env.WORKFORCE_DEPLOY_CLOUD_URL else restore it to prevCloudA, and likewise for prevCloudB/WORKFORCE_CLOUD_URL, keeping the call to restoreIsolate() and referencing the existing withTokenEnv and isolateAuthFiles helpers.
🤖 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.
Outside diff comments:
In `@packages/cli/src/destroy-command.test.ts`:
- Around line 75-94: The cleanup in withTokenEnv restores
WORKFORCE_DEPLOY_CLOUD_URL and WORKFORCE_CLOUD_URL only when
prevCloudA/prevCloudB were defined, causing leaked env state if tests set them
to undefined; update the cleanup to mirror the token/ws logic: if prevCloudA was
undefined then delete process.env.WORKFORCE_DEPLOY_CLOUD_URL else restore it to
prevCloudA, and likewise for prevCloudB/WORKFORCE_CLOUD_URL, keeping the call to
restoreIsolate() and referencing the existing withTokenEnv and isolateAuthFiles
helpers.
In `@packages/deploy/src/deploy.ts`:
- Line 234: The launcher input is currently using opts.cloudUrl which bypasses
the previously resolved/normalized cloud URL computed earlier; update the spread
at ...(opts.cloudUrl ? { cloudUrl: opts.cloudUrl } : {}) to use the resolved
variable produced in the earlier resolution logic (the canonical
cloudUrl/resolvedCloudUrl variable set around lines 154–157) so the launcher
receives the same canonical endpoint (preserve the conditional/fallback behavior
when the resolved value is undefined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f99e98f-9c70-49da-b935-5c85d95aa76a
📒 Files selected for processing (11)
packages/cli/src/destroy-command.test.tspackages/cli/src/destroy-command.tspackages/cli/src/list-command.tspackages/deploy/src/cloud-url.test.tspackages/deploy/src/cloud-url.tspackages/deploy/src/deploy.test.tspackages/deploy/src/deploy.tspackages/deploy/src/error-format.test.tspackages/deploy/src/error-format.tspackages/deploy/src/index.tspackages/deploy/src/login.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+751
to
+755
| await withWorkspaceEnv({ workspace: undefined, token: undefined }, async () => { | ||
| // Point active-workspace file at a definitely-missing path so the test | ||
| // doesn't accidentally pick up the host user's `~/.agentworkforce/active.json`. | ||
| const previousActiveFile = process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE; | ||
| process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE = path.join(os.tmpdir(), 'wf-deploy-test-missing-active.json'); |
Contributor
There was a problem hiding this comment.
🟡 Deploy test missing auth isolation, allowing host credentials to leak in
The new test "deploy: clear error when nothing resolves and noPrompt is set" only isolates WORKFORCE_ACTIVE_WORKSPACE_FILE (line 755) but does NOT set WORKFORCE_DISABLE_SHARED_AUTH or WORKFORCE_LOGIN_FILE to block the other filesystem-backed auth sources. Compare this with the destroy test's isolateAuthFiles() helper (packages/cli/src/destroy-command.test.ts:103-117) which correctly sets all three.
Without full isolation, resolveWorkspaceToken can still resolve credentials through:
- Tier 2:
readSharedAuthForBearer()reads~/.agent-relay/cloud-auth.json— if the developer has a validaccessTokenthere, Tier 2 can succeed (though it also needs a workspace, which is blocked by the missing active file, so this particular path is partially mitigated). - Tier 3:
loadWorkspaceToken→readWorkspaceTokenFromCloudAuthreads workspace tokens from~/.agent-relay/cloud-auth.json, andreadLoginFilereads~/.agentworkforce/login.json. If either returns a valid token+workspace,deploy()succeeds instead of rejecting, and theassert.rejectsassertion fails.
On a developer machine with existing workforce credentials, this test will fail.
Suggested change
| await withWorkspaceEnv({ workspace: undefined, token: undefined }, async () => { | |
| // Point active-workspace file at a definitely-missing path so the test | |
| // doesn't accidentally pick up the host user's `~/.agentworkforce/active.json`. | |
| const previousActiveFile = process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE; | |
| process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE = path.join(os.tmpdir(), 'wf-deploy-test-missing-active.json'); | |
| await withWorkspaceEnv({ workspace: undefined, token: undefined }, async () => { | |
| // Point active-workspace file at a definitely-missing path so the test | |
| // doesn't accidentally pick up the host user's `~/.agentworkforce/active.json`. | |
| const previousActiveFile = process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE; | |
| const previousLoginFile = process.env.WORKFORCE_LOGIN_FILE; | |
| const previousDisableShared = process.env.WORKFORCE_DISABLE_SHARED_AUTH; | |
| process.env.WORKFORCE_ACTIVE_WORKSPACE_FILE = path.join(os.tmpdir(), 'wf-deploy-test-missing-active.json'); | |
| process.env.WORKFORCE_LOGIN_FILE = path.join(os.tmpdir(), 'wf-deploy-test-missing-login.json'); | |
| process.env.WORKFORCE_DISABLE_SHARED_AUTH = '1'; |
Was this helpful? React with 👍 or 👎 to provide feedback.
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
~/.agentworkforce/active.json+~/.agent-relay/cloud-auth.json(matching what fix(cli): reuse @agent-relay/cloud auth as Bearer; drop workspace-token mint #113 already did for the cloud launcher + list/destroy). Previously the orchestrator's default resolver wasenvWorkspaceAuth(), which only honored env vars + a long-dead keychain — so a user who freshly ranagentworkforce loginhitno workspace resolvedbecause the freshly-written shared accessToken + active.json pointer were invisible to it.listanddestroydefaulted tohttps://agentrelay.com(missing the/cloudbasePath), routing every API call to the Next.js marketing 404 page; the wall-of-HTML body was then dumped verbatim into stderr. Centralized URL resolution + HTML body sanitization fix both.WORKFORCE_DISABLE_SHARED_AUTH=1for hermetic tests and power users who want strictly env-only operation.Why now
Reproduced today:
agentworkforce loginsucceeded,whoami+integrations+deploymentscurls returned 200 with the relay accessToken — butagentworkforce deployments listreturned404with the full marketing-site HTML, andagentworkforce deploy --no-promptexited at the workspace-resolution step before ever reaching the network. Live curl of the same endpoints with the same Bearer worked, confirming the auth state was fine — only the CLI plumbing was wrong.Changes
New helpers (
@agentworkforce/deploy):resolveCloudUrl(context)— single source of truth: flag → env → active.json → canonical default, always run throughcanonicalizeCloudUrl.canonicalizeCloudUrlnow also remaps the bare apexhttps://agentrelay.com→https://agentrelay.com/cloudso the marketing-site fallthrough can't happen even if a stale config carries the old default.formatHttpErrorBody(body, opts)— HTML detection + truncation. Replaces the wall-of-script-tags with a one-liner that names the likely cause and the offending URL.WORKFORCE_DISABLE_SHARED_AUTHenv opt-out in bothreadSharedAuthForBearerandreadWorkspaceTokenFromCloudAuth.Wiring changes:
packages/deploy/src/deploy.ts:148— default auth resolver swapped fromenvWorkspaceAuth()toresolveWorkspaceToken(). Explicitresolvers.workspaceAuth(tests, CI harnesses) still wins.packages/cli/src/list-command.ts— usesresolveCloudUrl+readActiveWorkspace+formatHttpErrorBody. Drops the localDEFAULT_CLOUD_URLandnormalizeCloudUrlhelper.packages/cli/src/destroy-command.ts— same. Also now lets workspace come from active.json (parity with list).Tests
17 new tests across 4 files (93 deploy + 186 cli all pass):
cloud-url.test.ts— apex canonicalization,resolveCloudUrlprecedence (flag/env/active/default), whitespace-only candidates skipped.error-format.test.ts— HTML detection, truncation, URL inclusion in hint, bare-angle-bracket false-positive guard.deploy.test.ts— env-Tier 1 happy path through the new default resolver,noPrompterror path with active-file isolation.destroy-command.test.ts— HTML 5xx body sanitization, active.jsoncloudUrlfallback,isolateAuthFiles()helper for hermetic tests.Smoke verified
Built locally and ran in
../proactive-agentswith clean env:Test plan
pnpm -F @agentworkforce/deploy run lintcleanpnpm -F @agentworkforce/deploy run test— 93/93 passpnpm -F @agentworkforce/cli run lintclean (the 2 pre-existinglocal-personas.tstagserrors require apnpm installto refresh@agentworkforce/persona-kitdist; once that's done, lint is clean)pnpm -F @agentworkforce/cli run test— 186/186 passpnpm run check(full repo) — green../proactive-agentsdeploy + list🤖 Generated with Claude Code