feat(deploy): workforce deploy v1 — persona-as-deployable-agent#90
Merged
Conversation
Extends PersonaSpec with deploy-time fields (cloud, useSubscription, integrations, schedules, sandbox, memory, traits, onEvent) and adds two new packages plus a CLI surface so a persona JSON deploys as a runnable agent end-to-end. Highlights - persona-kit: 8 new optional PersonaSpec fields, full parser coverage, KNOWN_TRIGGERS registry + lintTriggers helper (151 tests). - @agentworkforce/runtime: handler() wrapper, WorkforceCtx + WorkforceEvent discriminated union, gateway-envelope shim, startRunner driving NDJSON stdin, real GithubClient with retryable-status classification (15 tests). - @agentworkforce/deploy: orchestrator with preflight/connect/bundle/ launch steps, esbuild-driven bundle stager, child_process dev launcher with stdin piping + SIGTERM/SIGKILL stop, Daytona sandbox launcher (BYO DAYTONA_API_KEY), env-aware integration resolver (13 tests). - CLI: `workforce deploy` + `workforce login` cases wired into the existing dispatcher, foreground+detach handling for --mode dev. - examples/weekly-digest: real cron-driven persona that searches Brave, clusters by host, upserts a weekly GitHub issue. E2E verified: `workforce deploy ./examples/weekly-digest/persona.json --mode dev` builds the bundle, spawns the runner, dispatches an envelope on stdin, and streams structured logs back through the CLI. Plans authored: - docs/plans/deploy-v1.md (product plan) - docs/plans/deploy-v1-codex-spec.md (codex agent tasks) - docs/plans/deploy-v1-workflow-spec.md (Ricky cross-repo workflow) 361 tests pass across the 7 workspace packages. --mode cloud is the only explicitly-deferred surface (gated on the workforce-cloud deployments endpoint). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds deploy v1: new runtime and deploy packages, PersonaSpec schema/parsers, runtime handler/runner, bundle staging, mode launchers (dev/sandbox/cloud stub), CLI deploy/login commands, tests, examples, docs, and packaging/exports. ChangesWorkforce Deploy v1: Schema, Runtime, Orchestration & CLI
Sequence Diagram(s) (high-level deploy flow) sequenceDiagram
participant CLI as CLI
participant Deploy as Deploy.orchestrator
participant Stager as BundleStager
participant Launcher as ModeLauncher
CLI->>Deploy: runDeploy(parsedOptions)
Deploy->>Stager: stage(persona)
Deploy->>Launcher: launch(bundleDir, workspace, env)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+42
to
+43
| const exit = await result.runHandle.done; | ||
| process.exit(exit.code); |
Contributor
There was a problem hiding this comment.
🔴 Daytona sandbox never cleaned up — stop() is never called on the run handle
After the sandbox runner exits, deploy-command.ts:42-43 awaits result.runHandle.done then immediately calls process.exit(exit.code) without calling result.runHandle.stop(). For --mode sandbox, stop() calls sandbox.delete() (packages/deploy/src/modes/sandbox.ts:96), which is the only path that tears down the Daytona sandbox. Since nobody calls stop(), every --mode sandbox deploy leaks a remote Daytona sandbox.
Additionally, unlike the dev launcher which registers SIGINT/SIGTERM handlers (packages/deploy/src/modes/dev.ts:108-109) to forward signals and clean up the child process, the sandbox launcher has no signal handling at the CLI level. If the user presses Ctrl-C during a sandbox deploy, the CLI process exits immediately and the sandbox is also leaked.
Prompt for agents
The deploy-command.ts CLI entrypoint awaits result.runHandle.done but never calls result.runHandle.stop() before exiting. For --mode sandbox, this leaks the Daytona sandbox (a remote, billed resource). Two fixes are needed:
1. In deploy-command.ts runDeploy(), after done resolves (line 42-43), call await result.runHandle.stop() before process.exit(). Also handle the case where stop() throws (log and continue).
2. Register a SIGINT/SIGTERM handler at the CLI level (similar to dev.ts lines 96-109) that calls result.runHandle.stop() when the user presses Ctrl-C. Without this, Ctrl-C during --mode sandbox will leak the sandbox.
The relevant files are:
- packages/cli/src/deploy-command.ts (runDeploy function, around line 39-43)
- packages/deploy/src/modes/sandbox.ts (stop() at line 92-102 is the cleanup function that calls sandbox.delete())
Note that for --mode dev, the devLauncher itself registers signal handlers (dev.ts:108-109), so the child process is cleaned up on Ctrl-C. The sandbox launcher delegates this responsibility to the caller, but the caller never fulfills it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| method: 'POST', | ||
| pathname: `/repos/${args.owner}/${args.repo}/issues`, | ||
| body: { | ||
| title: args.matchTitle === args.title ? args.title : args.matchTitle, |
Contributor
There was a problem hiding this comment.
🟡 upsertIssue ignores the title parameter — always uses matchTitle as the created issue's title
The ternary at github.ts:163 (args.matchTitle === args.title ? args.title : args.matchTitle) always evaluates to args.matchTitle regardless of input — when they're equal it returns args.title (which equals matchTitle), and when they differ it explicitly returns args.matchTitle. The caller-supplied title is therefore silently ignored in the create path. The update path (packages/runtime/src/clients/github.ts:152-154) also doesn't set the title. This means the title parameter in the upsertIssue interface is dead code that gives callers the false impression they can set an issue title different from the match key. The only current caller (weekly-digest) uses title === matchTitle so this doesn't manifest today, but any future caller passing different values would see surprising behavior.
Suggested change
| title: args.matchTitle === args.title ? args.title : args.matchTitle, | |
| title: args.title, |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
packages/persona-kit/src/parse.test.ts (1)
438-472: ⚡ Quick winAdd regression tests for path and whitespace edge cases.
Please add cases for Windows absolute
onEventpaths (e.g.C:\x\agent.ts) and whitespace-variant schedule names ("weekly"vs"weekly "). These will lock in parser behavior around the new deploy-v1 fields.Also applies to: 511-520
🤖 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/persona-kit/src/parse.test.ts` around lines 438 - 472, Add regression tests in parse.test.ts that cover Windows absolute onEvent paths and whitespace variants for schedule names: add a test calling parseSchedules with names "weekly" and "weekly " to assert current behavior (whether names are preserved or deduped) and add a separate test that exercises the parser path-handling for an onEvent value like "C:\\x\\agent.ts" (the same file(s) that parse onEvent fields in the suite) to lock in the current deploy-v1 behavior; locate and update the existing parseSchedules tests (the parseSchedules function) and the existing onEvent/path parsing tests nearby to include these cases so CI will catch regressions.
🤖 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-workflow-spec.md`:
- Around line 28-37: The markdown fenced code blocks containing the shell
environment variables (e.g., HOME, ROOT, CLOUD_REPO, WORKFORCE_REPO,
AGENT_ASSISTANT_REPO, RELAYFILE_REPO, RELAY_REPO) are missing language
identifiers and trigger MD040; update each fence (the one around the shown env
block plus the fences at the other locations mentioned) to include a shell
language tag such as ```bash or ```sh so markdownlint recognizes them as shell
code. Ensure you modify the fences that wrap the blocks at the same spots (the
blocks containing those variable assignments) and keep the content unchanged.
In `@docs/plans/deploy-v1.md`:
- Around line 16-18: Add explicit fenced-code language identifiers for the
unlabeled code blocks in docs/plans/deploy-v1.md—e.g., change the block
containing the command "workforce deploy ./review-agent.json" to use ```bash,
and likewise add appropriate languages (bash, ts, text, etc.) to the other
unlabeled blocks referenced (around lines 277-285, 406-451, 609-612) so
markdownlint MD040 warnings are cleared; locate blocks by their contents (for
example the block with "workforce deploy ./review-agent.json") and update the
opening fence to include the correct language.
In `@examples/weekly-digest/agent.ts`:
- Around line 60-63: The current parsing of config.repo into const [owner, repo]
= config.repo.split('/') allows malformed values like "owner/repo/extra";
validate WEEKLY_DIGEST_REPO first by splitting config.repo on '/' and ensuring
the resulting array has exactly two non-empty segments before assigning owner
and repo; if the validation fails, throw the existing Error with the same
message. Update the code around config.repo, owner, and repo to perform this
length-and-non-empty check prior to destructuring so only exactly "owner/repo"
is accepted.
- Around line 1-8: The import of symbols like createGithubClient, handler,
WorkforceIntegrationError and the Workforce* types from `@agentworkforce/runtime`
is failing because the runtime package hasn't been built or TypeScript path
mappings aren't configured; fix by building the runtime package so
packages/runtime/dist/ exists (run the monorepo build or npm/yarn workspace
build for runtime) and/or add a tsconfig path mapping or project reference that
points "@agentworkforce/runtime" to the runtime source (or its dist) so the
compiler can resolve the module and its types for files importing
createGithubClient, handler, WorkforceIntegrationError, GithubClient,
WorkforceCtx, and WorkforceEvent.
In `@examples/weekly-digest/persona.json`:
- Around line 7-10: The integration scope in persona.json is hardcoded
(integrations.github.scope.repo) while the runtime uses the configurable
WEEKLY_DIGEST_REPO, causing mismatched permissions; update the
persona/integration configuration so the GitHub scope is derived from or
templated with the WEEKLY_DIGEST_REPO value (or conversely lock the env var to
the hardcoded repo) to ensure a single source of truth—search for
integrations.github.scope.repo in persona.json and align it with the
WEEKLY_DIGEST_REPO configuration used at runtime (or remove the env
configurability if you intend the repo to be fixed).
In `@packages/cli/src/cli.ts`:
- Around line 3820-3829: The subcommand dispatch lets execution fall through
after awaiting runDeploy(rest) or runLogin(rest) so the later check if
(subcommand !== 'agent') will wrongly call die(...); fix by ensuring control
returns after successful handlers: add explicit return statements immediately
after await runDeploy(rest) and await runLogin(rest) (or convert the chained ifs
into an if/else-if/else or switch on subcommand) so that once runDeploy or
runLogin completes the code does not continue to the die(`Unknown subcommand
"${subcommand}".`) path.
In `@packages/cli/src/deploy-command.ts`:
- Around line 160-164: The expectValue function currently treats any non-empty
string as a valid value and can mistakenly accept a subsequent flag token (e.g.,
"--no-connect") as the option's value; update expectValue to also reject tokens
that look like flags by checking whether the provided value (after trimming)
starts with '-' (single or double dash) and call die(`${flag}: missing value`)
if so, so that flag tokens are not accepted as option values; implement this
change inside the expectValue function to validate value.trim() and its leading
character(s) before returning the value.
In `@packages/deploy/src/connect.ts`:
- Around line 187-195: Remove the sentinel string assignment to
subscriptionProvider — do not set subscriptionProvider = '(already-connected)';
instead leave subscriptionProvider unset/undefined when already connected (you
can keep the input.io.info('subscription: already connected') log). If you need
to track the already-connected state internally, use a separate boolean (e.g.,
alreadyConnected) and do not include that in the returned object; keep the
existing return spread ...(subscriptionProvider ? { subscriptionProvider } : {})
so the provider field is omitted for already-connected cases.
In `@packages/deploy/src/io.ts`:
- Around line 10-17: The readline.Interface stored in the module-level variable
rl (created by ensureReadline()) is never closed, keeping the process alive;
update all places that call ensureReadline() for asking a question to call
rl.close() after the question completes and set rl = undefined so the interface
is torn down and can be recreated later (e.g., inside the functions that prompt
the user, call rl.question/promises and in their .then/.finally or try/finally
ensure rl.close() and rl = undefined). Ensure ensureReadline() itself still
lazily creates and returns rl but does not change lifecycle management.
In `@packages/deploy/src/login.ts`:
- Around line 25-39: The workspace and token values can be whitespace-only and
should be normalized before validation; update the assignments for workspace and
token (the variables named workspace and token in this file) to trim any string
source, e.g. use (override ?? process.env.WORKFORCE_WORKSPACE_ID)?.trim() and
process.env.WORKFORCE_WORKSPACE_TOKEN?.trim(), so that subsequent checks (if
(!workspace) / if (!token)) correctly treat whitespace-only values as missing.
In `@packages/deploy/src/modes/dev.ts`:
- Around line 85-88: The escalation timeout never sends SIGKILL because
child.killed is set true immediately after calling child.kill('SIGTERM'); change
the escalation check to detect whether the child actually exited by checking
child.exitCode and child.signalCode (both will be null if the process is still
running) instead of child.killed. In the setTimeout created at
SIGTERM_TO_SIGKILL_MS (the escalation variable), replace the condition `if
(!child.killed)` with a check like `if (child.exitCode === null &&
child.signalCode === null)` so stuck children are escalated to
child.kill('SIGKILL').
In `@packages/deploy/src/modes/sandbox.ts`:
- Around line 151-153: The sandbox launch currently forces installing
"@agentworkforce/runtime@latest" via sandbox.process.executeCommand (the install
call using SANDBOX_BUNDLE_DIR), causing API drift; change this to either remove
the explicit `@latest` so npm resolves the version declared in the staged bundle's
package.json, or read the resolved runtime version from the bundle (e.g., parse
the staged package.json in SANDBOX_BUNDLE_DIR to get
dependencies["@agentworkforce/runtime"] or a lockfile-resolved version) and pass
that exact version to the install command so the install matches the bundle's
intended runtime.
In `@packages/persona-kit/src/parse.ts`:
- Around line 487-494: The current relative-path check only rejects POSIX
absolute paths (value.startsWith('/')) but misses Windows absolute forms; update
the validation where value and context are checked (near ONEVENT_EXT_RE usage)
to also detect Windows drive-letter prefixes and UNC paths (e.g. patterns like
"C:\..." or "\\server\share") — use a regex such as one that matches
/^[A-Za-z]:[\\/]/ or leading double backslashes/forward slashes and throw the
same Error(`${context} must be a relative POSIX path; got absolute "${value}"`)
when matched so these Windows absolute paths are rejected too.
- Around line 612-629: Trim and use normalized values for name/cron (and tz)
before validating, deduping and storing: compute trimmedName = name.trim() and
trimmedCron = cron.trim() (and trimmedTz if tz is a string), run the typeof
checks and assertCronExpression against those trimmed values, use
seenNames.has(trimmedName) / seenNames.add(trimmedName) for dedupe, and push the
trimmed values into out (e.g. name: trimmedName, cron: trimmedCron, tz:
trimmedTz) so whitespace doesn't leak into schedule IDs or dedupe logic while
keeping entryContext and assertCronExpression usage unchanged.
In `@packages/persona-kit/src/triggers.ts`:
- Around line 11-13: The comment in packages/persona-kit/src/triggers.ts embeds
a user-specific absolute path; replace that machine-specific path with a
repo-relative reference or documentation link (for example "relayfile-adapters/"
or "docs/plans/deploy-v1-codex-spec.md Task 6") so the comment reads generically
and does not expose local filesystem/user info; update the comment text
mentioning "Relayfile adapter sources" to use the repo-relative path or a short
doc reference instead.
In `@packages/runtime/src/clients/github.ts`:
- Around line 182-227: fetchDiff currently calls the untrusted pr.diff_url with
the bearer token; change it to construct and call the canonical GitHub API
endpoint for PR diffs (e.g., /repos/{owner}/{repo}/pulls/{number}) and route the
request through the existing request wrapper rather than fetchImpl so
credentials are scoped to the configured API host. Add a request-text variant
(or extend request to accept a responseType:'text') that returns raw text and
reuse existing error handling (WorkforceIntegrationError and isRetryableStatus)
so status/retry behavior stays consistent; update fetchDiff to accept the
necessary target identifiers (owner, repo, number or a request-like config) and
call request(...) to obtain the diff text.
In `@packages/runtime/src/ctx.ts`:
- Around line 112-115: The loop that merges options.integrations into the main
ctx (using Object.assign with provider/client) can overwrite core context
fields; change it to guard against collisions by checking if the provider key
already exists on ctx before assigning — if it exists, either throw a clear
error or instead nest integrations under a dedicated property (e.g.,
ctx.integrations = ctx.integrations || {}; ctx.integrations[provider] = client)
so core members are never overwritten; update the code around
options.integrations, the for (const [provider, client] ...) block, and any call
sites that expect integration clients on ctx accordingly.
In `@packages/runtime/src/runner.ts`:
- Around line 200-207: The EOF tail parsing currently swallows JSON.parse errors
for the trailing buffer (variable tail) and must instead log a warning before
skipping malformed NDJSON; update the catch block around JSON.parse(tail) in the
runner.ts code that yields RawGatewayEnvelope to call the same warning logger
used in the per-line parse path (i.e., the existing logger/warning call used
when individual lines fail to parse) and include the malformed tail content or a
short excerpt in the message, then continue to skip it.
In `@packages/runtime/src/shim.ts`:
- Around line 79-90: The code builds a WorkforceProviderEvent even when env.type
ends with a dot (e.g., "github.") resulting in an empty event type; update the
guard after computing firstDot and providerCandidate to also verify the suffix
is non-empty (i.e., ensure env.type.slice(firstDot + 1) is not an empty string)
and return null for malformed provider events; change the conditional around
isProviderSource(providerCandidate) to also check that the derived type suffix
is truthy before constructing providerEvent so functions/variables like
firstDot, providerCandidate, isProviderSource, providerEvent and
WorkforceProviderEvent are used to locate and implement the fix.
---
Nitpick comments:
In `@packages/persona-kit/src/parse.test.ts`:
- Around line 438-472: Add regression tests in parse.test.ts that cover Windows
absolute onEvent paths and whitespace variants for schedule names: add a test
calling parseSchedules with names "weekly" and "weekly " to assert current
behavior (whether names are preserved or deduped) and add a separate test that
exercises the parser path-handling for an onEvent value like "C:\\x\\agent.ts"
(the same file(s) that parse onEvent fields in the suite) to lock in the current
deploy-v1 behavior; locate and update the existing parseSchedules tests (the
parseSchedules function) and the existing onEvent/path parsing tests nearby to
include these cases so CI will catch regressions.
🪄 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: d2ff68d8-94db-49d6-b398-6d095210433e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.gitignoredocs/plans/deploy-v1-codex-spec.mddocs/plans/deploy-v1-workflow-spec.mddocs/plans/deploy-v1.mdexamples/weekly-digest/README.mdexamples/weekly-digest/agent.tsexamples/weekly-digest/persona.jsonpackages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/deploy-command.tspackages/deploy/package.jsonpackages/deploy/src/bundle.test.tspackages/deploy/src/bundle.tspackages/deploy/src/connect.tspackages/deploy/src/deploy.test.tspackages/deploy/src/deploy.tspackages/deploy/src/index.tspackages/deploy/src/io.tspackages/deploy/src/login.tspackages/deploy/src/modes/cloud.tspackages/deploy/src/modes/dev.tspackages/deploy/src/modes/sandbox.tspackages/deploy/src/preflight.tspackages/deploy/src/types.tspackages/deploy/tsconfig.jsonpackages/persona-kit/package.jsonpackages/persona-kit/src/index.tspackages/persona-kit/src/parse.test.tspackages/persona-kit/src/parse.tspackages/persona-kit/src/triggers.test.tspackages/persona-kit/src/triggers.tspackages/persona-kit/src/types.tspackages/runtime/package.jsonpackages/runtime/src/clients/errors.tspackages/runtime/src/clients/github.test.tspackages/runtime/src/clients/github.tspackages/runtime/src/clients/index.tspackages/runtime/src/ctx.tspackages/runtime/src/handler.tspackages/runtime/src/index.tspackages/runtime/src/raw.tspackages/runtime/src/runner.test.tspackages/runtime/src/runner.tspackages/runtime/src/shim.test.tspackages/runtime/src/shim.tspackages/runtime/src/types.tspackages/runtime/tsconfig.json
CI fix - examples/tsconfig.json: add path mappings for @agentworkforce/runtime (incl. /runner, /clients, /raw) and @agentworkforce/persona-kit so example imports resolve against the workspace sources. Unblocks the failing `pnpm run typecheck:examples` step. Real bugs flagged in review - deploy/modes/dev.ts: SIGKILL escalation now checks exitCode/signalCode instead of child.killed — the latter flips true immediately after the initial SIGTERM, so the old check never escalated stuck children. - deploy/modes/sandbox.ts: drop the explicit `@latest` pin from the in-sandbox `npm install`; let npm resolve the version declared in the staged bundle's package.json so the running runtime matches the bundle's intended version. - runtime/shim.ts: reject envelopes like `github.` whose source is valid but whose event-name suffix is empty. Returns null so the runner logs the envelope as unsupported rather than dispatching an empty-typed event to the handler. - runtime/ctx.ts: refuse to attach integration clients that collide with core ctx fields (`harness`, `sandbox`, etc.). A malformed persona declaring an integration named `harness` no longer silently shadows `ctx.harness.run`. - runtime/clients/github.ts: route the diff fetch through the canonical `/repos/:owner/:repo/pulls/:number` endpoint via the authed request helper, instead of trusting `pr.diff_url`. Keeps the bearer token scoped to the configured API host. - persona-kit/parse.ts: reject Windows-absolute onEvent paths (`C:\...`, `C:/...`, `\\server\share\...`) alongside POSIX `/abs`. - persona-kit/parse.ts: trim schedule name/cron/tz before validating and deduping so `"weekly"` and `" weekly "` collapse correctly. - runtime/runner.ts: log the malformed-tail JSON parse failure with an excerpt instead of silently swallowing it. - deploy/io.ts: open a short-lived readline.Interface per question and close it in finally — keeps stdin from staying in raw mode and pinning the event loop open after the prompt completes. - deploy/login.ts: trim whitespace-only workspace/token env values to "missing" so they fail with a clear setup error instead of a confusing downstream 401. - deploy/connect.ts: drop the `'(already-connected)'` sentinel string; leave subscriptionProvider undefined when no connect ran. - cli/cli.ts: add explicit `return` after deploy/login handlers so the `subcommand !== 'agent'` check below doesn't fire wrongly. - cli/deploy-command.ts: expectValue now rejects flag tokens (`--workspace --detach` fails loudly rather than treating `--detach` as the workspace name). - persona-kit/triggers.ts: strip user-specific absolute path from the registry doc comment. Examples - examples/weekly-digest/agent.ts: validate WEEKLY_DIGEST_REPO is exactly `owner/repo` (two non-empty segments) before destructuring. - examples/weekly-digest/persona.json: drop the hardcoded GitHub scope.repo so the env-configurable WEEKLY_DIGEST_REPO stays the single source of truth. Tests - New regression coverage for: Windows-absolute onEvent rejection, schedule field trimming + dedupe across whitespace variants, shim null on empty provider-event suffix, buildCtx core-field collision guard, github.getPr fetching the diff through the configured API host instead of the untrusted diff_url. 379 tests pass (+18). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/persona-kit/src/parse.ts (1)
490-506:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject backslashes in
onEventpaths too.Line 494 now blocks Windows absolute forms, but
scripts\handler.tsstill passes even though this helper promises a relative POSIX path. That leaves a portability bug in the deploy path resolution instead of failing fast here.Suggested fix
function assertOnEventPath(value: unknown, context: string): string { if (typeof value !== 'string' || !value.trim()) { throw new Error(`${context} must be a non-empty string`); } if (value.startsWith('/') || WIN_ABSOLUTE_RE.test(value)) { throw new Error(`${context} must be a relative POSIX path; got absolute "${value}"`); } + if (value.includes('\\')) { + throw new Error(`${context} must use POSIX separators ('/'), not backslashes`); + } const segments = value.split(/[\\/]+/); if (segments.some((s) => s === '..')) { throw new Error(`${context} must not contain ".." segments`); }🤖 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/persona-kit/src/parse.ts` around lines 490 - 506, assertOnEventPath currently allows backslashes so inputs like "scripts\\handler.ts" pass; update the function to reject any backslash characters to enforce a relative POSIX path. In assertOnEventPath add a guard (e.g. if (value.includes('\\') || /\\/.test(value)) throw new Error(`${context} must use POSIX forward-slash separators; got "${value}"`)) before the split and extension checks (use the existing WIN_ABSOLUTE_RE and ONEVENT_EXT_RE symbols unchanged), so backslash-containing paths fail fast and preserve the subsequent checks for absolutes, ".." segments, and file extensions.
🤖 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/sandbox.ts`:
- Around line 33-42: resolveSandboxAuth currently allows returning { jwtToken }
without DAYTONA_ORGANIZATION_ID which causes the Daytona SDK to throw later;
update resolveSandboxAuth to validate that if process.env.DAYTONA_JWT_TOKEN is
present then process.env.DAYTONA_ORGANIZATION_ID must also be present and, if
missing, throw a clear, workforce-specific error (e.g., mention
DAYTONA_JWT_TOKEN and DAYTONA_ORGANIZATION_ID) instead of returning a partial
SandboxAuth; keep the existing shape-building logic for apiKey and
organizationId when both present so new Daytona(auth) receives a valid auth
object.
In `@packages/persona-kit/src/parse.ts`:
- Around line 581-599: The function parseIntegrations creates a plain object
`out` which allows prototype pollution via a `__proto__` key; change the
creation to use a null-prototype map (e.g., replace the `{}` used for `out` with
an object created via Object.create(null)) so provider keys like "__proto__"
become safe own properties, and retain the rest of the logic that assigns via
out[provider] = parseIntegrationConfig(raw, `${context}.${provider}`) and
returns Object.keys(out).length > 0 ? out : undefined.
---
Duplicate comments:
In `@packages/persona-kit/src/parse.ts`:
- Around line 490-506: assertOnEventPath currently allows backslashes so inputs
like "scripts\\handler.ts" pass; update the function to reject any backslash
characters to enforce a relative POSIX path. In assertOnEventPath add a guard
(e.g. if (value.includes('\\') || /\\/.test(value)) throw new Error(`${context}
must use POSIX forward-slash separators; got "${value}"`)) before the split and
extension checks (use the existing WIN_ABSOLUTE_RE and ONEVENT_EXT_RE symbols
unchanged), so backslash-containing paths fail fast and preserve the subsequent
checks for absolutes, ".." segments, and file extensions.
🪄 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: dc5ddc26-58a6-43a2-9146-7f3dccd40c3e
📒 Files selected for processing (20)
examples/tsconfig.jsonexamples/weekly-digest/agent.tsexamples/weekly-digest/persona.jsonpackages/cli/src/cli.tspackages/cli/src/deploy-command.tspackages/deploy/src/connect.tspackages/deploy/src/io.tspackages/deploy/src/login.tspackages/deploy/src/modes/dev.tspackages/deploy/src/modes/sandbox.tspackages/persona-kit/src/parse.test.tspackages/persona-kit/src/parse.tspackages/persona-kit/src/triggers.tspackages/runtime/src/clients/github.test.tspackages/runtime/src/clients/github.tspackages/runtime/src/ctx.tspackages/runtime/src/runner.test.tspackages/runtime/src/runner.tspackages/runtime/src/shim.test.tspackages/runtime/src/shim.ts
✅ Files skipped from review due to trivial changes (1)
- packages/runtime/src/runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- examples/weekly-digest/persona.json
- packages/deploy/src/login.ts
- packages/runtime/src/shim.test.ts
- packages/persona-kit/src/triggers.ts
- packages/deploy/src/io.ts
- packages/runtime/src/ctx.ts
- packages/runtime/src/clients/github.test.ts
- packages/runtime/src/shim.ts
- packages/persona-kit/src/parse.test.ts
- packages/cli/src/deploy-command.ts
- packages/deploy/src/connect.ts
- packages/runtime/src/runner.ts
- packages/deploy/src/modes/dev.ts
Comment on lines
+33
to
+42
| export function resolveSandboxAuth(): SandboxAuth | undefined { | ||
| const apiKey = process.env.DAYTONA_API_KEY; | ||
| const jwtToken = process.env.DAYTONA_JWT_TOKEN; | ||
| const organizationId = process.env.DAYTONA_ORGANIZATION_ID; | ||
| if (!apiKey && !jwtToken) return undefined; | ||
| return { | ||
| ...(apiKey ? { apiKey } : {}), | ||
| ...(jwtToken ? { jwtToken } : {}), | ||
| ...(organizationId ? { organizationId } : {}) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the sandbox.ts file to understand the context better
head -60 packages/deploy/src/modes/sandbox.tsRepository: AgentWorkforce/workforce
Length of output: 2352
🏁 Script executed:
# Find the SandboxAuth type definition
rg "type SandboxAuth|interface SandboxAuth" -A 5Repository: AgentWorkforce/workforce
Length of output: 761
🏁 Script executed:
# Check package.json for Daytona SDK version
cat packages/deploy/package.json | jq '.dependencies | select(. != null) | keys[]' | grep -i daytonaRepository: AgentWorkforce/workforce
Length of output: 85
🏁 Script executed:
# Look for imports and how Daytona is used
rg "from.*daytona|import.*Daytona" packages/deploy/src/modes/sandbox.ts -A 2 -B 2Repository: AgentWorkforce/workforce
Length of output: 232
🏁 Script executed:
# Check how the auth object is used with Daytona
rg "new Daytona" packages/deploy/src/modes/sandbox.ts -A 5 -B 5Repository: AgentWorkforce/workforce
Length of output: 469
🏁 Script executed:
# Get the exact version of `@daytonaio/sdk`
cat packages/deploy/package.json | grep -A 2 -B 2 "@daytonaio/sdk"Repository: AgentWorkforce/workforce
Length of output: 227
🏁 Script executed:
# Look for any error handling or validation related to auth in the sandbox launcher
rg "organizationId|DAYTONA_ORGANIZATION_ID" packages/deploy/src/modes/sandbox.ts -B 3 -A 3Repository: AgentWorkforce/workforce
Length of output: 963
🏁 Script executed:
# Check if there are any tests or docs that mention auth requirements
fd -t f "(test|spec|doc)" packages/deploy/src/ | head -20Repository: AgentWorkforce/workforce
Length of output: 138
🏁 Script executed:
# Search for any comments or warnings about JWT + organizationId combination
rg "JWT|jwt|jwtToken|organizationId" packages/deploy/src/ -B 2 -A 2Repository: AgentWorkforce/workforce
Length of output: 1735
🌐 Web query:
@daytonaio/sdk 0.148.0 JWT authentication organizationId requirement TypeScript
💡 Result:
Yes, in the @daytonaio/sdk (now @daytona/sdk) TypeScript SDK, including version 0.148.0, the organizationId is required for JWT authentication. The DaytonaConfig interface specifies: "organizationId? string - Organization ID used for JWT-based authentication. Required if a JWT token is provided, and must be set either here or in the environment variable DAYTONA_ORGANIZATION_ID."[1][2][3] If using JWT (via jwtToken in config or DAYTONA_JWT_TOKEN env var), providing organizationId (via config or DAYTONA_ORGANIZATION_ID) is mandatory; otherwise, the SDK throws an error like "Organization ID is required when using JWT token" or the improved "DAYTONA_ORGANIZATION_ID is required when authenticating with DAYTONA_JWT_TOKEN."[4] API key authentication (apiKey or DAYTONA_API_KEY) does not require organizationId.[5][2]
Citations:
- 1: https://github.com/daytonaio/daytona/blob/main/apps/docs/src/content/docs/en/typescript-sdk/daytona.mdx
- 2: https://www.daytona.io/docs/typescript-sdk/daytona.md
- 3: https://github.com/daytonaio/daytona/blob/main/apps/docs/src/content/docs/en/typescript-sdk/index.mdx
- 4: fix(sdk): improve auth error messages across all SDKs daytonaio/daytona#4595
- 5: https://www.daytona.io/docs/en/typescript-sdk.md
🏁 Script executed:
# Check if there's any type information or docs about DaytonaConfig in the repo
rg "DaytonaConfig|Daytona.*config" packages/deploy/src/ -B 3 -A 3Repository: AgentWorkforce/workforce
Length of output: 50
🏁 Script executed:
# Look at the exact Daytona constructor call and how auth is passed
sed -n '75,95p' packages/deploy/src/modes/sandbox.tsRepository: AgentWorkforce/workforce
Length of output: 636
🏁 Script executed:
# Check if there are any integration tests that might show the auth behavior
cat packages/deploy/src/deploy.test.ts | head -100Repository: AgentWorkforce/workforce
Length of output: 3259
🏁 Script executed:
# Let's see the complete Daytona instantiation to understand error handling
sed -n '75,100p' packages/deploy/src/modes/sandbox.tsRepository: AgentWorkforce/workforce
Length of output: 813
🏁 Script executed:
# Check if there's any error handling around the Daytona constructor
rg "new Daytona" packages/deploy/src/modes/sandbox.ts -A 10 -B 2Repository: AgentWorkforce/workforce
Length of output: 385
🏁 Script executed:
# Verify the SandboxAuth interface is used elsewhere and how it's validated
rg "SandboxAuth" packages/deploy/src/ -B 2 -A 2Repository: AgentWorkforce/workforce
Length of output: 1497
Validate JWT auth with DAYTONA_ORGANIZATION_ID.
resolveSandboxAuth() currently returns { jwtToken } without requiring organizationId, but the Daytona SDK (v0.148.0+) mandates organizationId when jwtToken is provided and throws otherwise. This pushes a configuration error down into new Daytona(auth) instead of surfacing it here with a clear workforce-specific message.
💡 Minimal fix
export function resolveSandboxAuth(): SandboxAuth | undefined {
const apiKey = process.env.DAYTONA_API_KEY;
const jwtToken = process.env.DAYTONA_JWT_TOKEN;
const organizationId = process.env.DAYTONA_ORGANIZATION_ID;
if (!apiKey && !jwtToken) return undefined;
+ if (!apiKey && jwtToken && !organizationId) {
+ throw new Error(
+ 'sandbox launcher: DAYTONA_ORGANIZATION_ID is required when DAYTONA_JWT_TOKEN is set.'
+ );
+ }
return {
...(apiKey ? { apiKey } : {}),
...(jwtToken ? { jwtToken } : {}),
...(organizationId ? { organizationId } : {})
};
}🤖 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/sandbox.ts` around lines 33 - 42,
resolveSandboxAuth currently allows returning { jwtToken } without
DAYTONA_ORGANIZATION_ID which causes the Daytona SDK to throw later; update
resolveSandboxAuth to validate that if process.env.DAYTONA_JWT_TOKEN is present
then process.env.DAYTONA_ORGANIZATION_ID must also be present and, if missing,
throw a clear, workforce-specific error (e.g., mention DAYTONA_JWT_TOKEN and
DAYTONA_ORGANIZATION_ID) instead of returning a partial SandboxAuth; keep the
existing shape-building logic for apiKey and organizationId when both present so
new Daytona(auth) receives a valid auth object.
Comment on lines
+581
to
+599
| export function parseIntegrations( | ||
| value: unknown, | ||
| context: string | ||
| ): Record<string, PersonaIntegrationConfig> | undefined { | ||
| if (value === undefined) return undefined; | ||
| if (!isObject(value)) { | ||
| throw new Error(`${context} must be an object if provided`); | ||
| } | ||
|
|
||
| const out: Record<string, PersonaIntegrationConfig> = {}; | ||
| for (const [provider, raw] of Object.entries(value)) { | ||
| if (!provider.trim()) { | ||
| throw new Error(`${context} integration keys must be non-empty strings`); | ||
| } | ||
| out[provider] = parseIntegrationConfig(raw, `${context}.${provider}`); | ||
| } | ||
|
|
||
| return Object.keys(out).length > 0 ? out : undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const out = {};
out["__proto__"] = { polluted: true };
console.log("keys:", Object.keys(out));
console.log("prototype_polluted:", Object.getPrototypeOf(out).polluted === true);
NODERepository: AgentWorkforce/workforce
Length of output: 102
🏁 Script executed:
cat -n packages/persona-kit/src/parse.ts | sed -n '581,599p'Repository: AgentWorkforce/workforce
Length of output: 840
🏁 Script executed:
#!/bin/bash
node <<'NODE'
// Test that Object.create(null) prevents prototype pollution
const out = Object.create(null);
out["__proto__"] = { polluted: true };
console.log("keys:", Object.keys(out));
console.log("prototype_check:", out.__proto__ === undefined);
console.log("polluted_value:", out["__proto__"]);
NODERepository: AgentWorkforce/workforce
Length of output: 148
🏁 Script executed:
cd packages/persona-kit && grep -n "Object.create(null)" src/parse.ts || echo "No Object.create(null) found"Repository: AgentWorkforce/workforce
Length of output: 97
Use null prototype to prevent __proto__ pollution.
Line 590 creates out with a plain object {}. If user input contains a provider key named __proto__, the assignment at line 595 will mutate the prototype chain instead of creating a regular entry, potentially tainting all downstream property lookups that depend on this object's inherited properties.
Suggested fix
- const out: Record<string, PersonaIntegrationConfig> = {};
+ const out = Object.create(null) as Record<string, PersonaIntegrationConfig>;🤖 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/persona-kit/src/parse.ts` around lines 581 - 599, The function
parseIntegrations creates a plain object `out` which allows prototype pollution
via a `__proto__` key; change the creation to use a null-prototype map (e.g.,
replace the `{}` used for `out` with an object created via Object.create(null))
so provider keys like "__proto__" become safe own properties, and retain the
rest of the logic that assigns via out[provider] = parseIntegrationConfig(raw,
`${context}.${provider}`) and returns Object.keys(out).length > 0 ? out :
undefined.
5 tasks
CodeRabbit MD040 nit on #90 — the four unlabeled code-fence opens in docs/plans/deploy-v1.md and the three in deploy-v1-workflow-spec.md are now labeled (sh / text). Pure cosmetic; no content changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cloud#543 merged the POST /api/v1/workspaces/:id/sandboxes endpoint
(plus per-sandbox /exec and /files proxy routes). This commit
finishes the workforce-side half: the sandbox launcher now drives
either path automatically.
Mode picking (resolveSandboxClient):
- BYO: DAYTONA_API_KEY (or JWT + ORGANIZATION_ID) in env → talk to
Daytona directly via @daytonaio/sdk. Zero workforce-cloud
round-trips.
- Workforce-managed: BYO env absent + WORKFORCE_WORKSPACE_TOKEN
present → POST /sandboxes to mint a proxy handle, then route
exec/upload through the returned execUrl + filesUrl. Cloud holds
the org Daytona credentials so users never see them.
- --byo-sandbox CLI flag forces BYO even when both are configured.
- Neither configured: clean error pointing at the two setup paths.
Implementation (sandbox-client.ts):
- SandboxClient interface: mint, uploadBundle, exec, destroy.
- createByoSandboxClient: extracted from the old sandbox.ts.
- createProxySandboxClient: HTTP against cloud endpoints with the
workspace token. Absolutifies returned execUrl/filesUrl against
WORKFORCE_CLOUD_URL when cloud emits relative paths. Tolerates 404
on destroy (idempotent cleanup). npm install in the sandbox runs
through the same exec channel so behavior matches BYO.
Wiring:
- DeployOptions.byoSandbox → ModeLaunchInput.byoSandbox → launcher
overrides.forceByo. The flag was already parsed in the CLI; it
just wasn't threaded through.
Tests (+9):
- sandbox-client.test.ts: proxy mint → upload → exec → destroy round
trip, 4xx surface, idempotent 404 destroy, in-sandbox npm install
failure surfaces.
- sandbox.test.ts: resolveSandboxClient mode selection across env
permutations (BYO present, workforce-only, neither, forceByo +
BYO, forceByo without BYO).
Repo gates: 375 tests pass; typecheck + examples typecheck clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
khaliqgant
added a commit
that referenced
this pull request
May 12, 2026
…t style
Switches workforce's integration clients from direct REST calls to the
Relayfile-VFS writeback pattern used by sage + the cloud workflows.
Handler-side surface (ctx.github.upsertIssue, ctx.linear.comment, etc.)
stays identical; the wire underneath flips from "speak HTTP to GitHub"
to "write a JSON draft inside the Relayfile mount and let the writeback
worker do the actual API call." Aligns workforce with the rest of the
org's integration story and inherits writeback durability + retry for
free.
Substrate
- packages/runtime/src/errors.ts (top-level): WorkforceIntegrationError
moves here with the { provider, operation, cause, retryable } shape
sage/cloud already use. Old clients/errors.ts is removed; the public
surface re-exports it from the same package import path so existing
consumers (mcp-workforce) keep compiling.
- packages/runtime/src/clients/request.ts: shared VFS helpers
(readJsonFile, readTextFile, listJsonFiles, listDirectoryEntries,
writeJsonFile + atomic write-then-rename) with mount-root path
validation and optional writeback-receipt polling.
Clients
- github.ts is rewritten as a VFS client. Same GithubClient interface
(comment, createIssue, upsertIssue, getPr, postReview); each method
now reads/writes files at canonical paths under
`/github/repos/<owner>/<repo>/...`.
- linear, slack, notion, jira ship as new typed clients with the same
pattern. IntegrationClients in types.ts now types all five concretely
instead of leaving four as unknown.
Tests
- github.test.ts is rewritten end-to-end against a tempdir mount.
- linear/slack/notion/jira tests run against tempdir mounts too.
- 29 runtime tests pass (up from 18), 386 across the repo.
Example
- weekly-digest/agent.ts drops the WORKFORCE_INTEGRATION_GITHUB_TOKEN
plumbing; the github client picks up RELAYFILE_MOUNT_ROOT instead.
- weekly-digest/README.md documents the writeback model + Relayfile
mount env requirement, and drops the GITHUB_TOKEN setup step.
Notes
- mcp-workforce (PR #91) imports createGithubClient with a different
construction shape today (`{ token }`); it'll need a follow-up
commit to switch to IntegrationClientOptions once this lands. The
MCP package depends on the new shape, not the old.
- The direct-REST github implementation that shipped in #90 is
replaced wholesale. No persona today depends on it; weekly-digest
is updated in this commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
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
Ships
workforce deploy <persona.json>end-to-end. Every persona gains optional deploy-time fields (cloud,useSubscription,integrations,schedules,sandbox,memory,traits,onEvent); two new packages (@agentworkforce/runtime,@agentworkforce/deploy) provide the handler facade, ctx, gateway-envelope shim, bundle stager, and run-mode launchers; the CLI addsdeployandlogincases.KNOWN_TRIGGERSregistry andlintTriggershelper for unknown-trigger warnings.@agentworkforce/runtime—handler()brand,WorkforceCtx+WorkforceEventdiscriminated union, NDJSON-stdin runner, a realGithubClientwith retryable-status classification, supporting integrationWorkforceIntegrationError.@agentworkforce/deploy— orchestrator (preflight → workspace → connect → bundle → launch), esbuild-driven bundle stager that emitsagent.bundle.mjs+runner.mjs+persona.json+package.json, real--mode dev(child_process with stdin piping and SIGTERM/SIGKILL stop), real--mode sandbox(Daytona SDK, BYODAYTONA_API_KEY), env-aware integration resolver.--mode cloudis the only explicitly-deferred surface — gated on the workforce-cloud deployments endpoint.docs/plans/: the product plan, the codex agent task spec, and the Ricky cross-repo workflow spec.Verified end-to-end
Bundle is staged, child runner spawned, envelope shimmed, handler dispatched, structured logs streamed back through the CLI, runner exits 0 on stream end.
Whole-repo gates
pnpm -r typecheck— green across all 7 packagespnpm -r test— 361 tests pass, 0 fail (persona-kit 151, runtime 15, workload-router 15, deploy 13, cli 166, agentworkforce 1)pnpm -r build— emitsdist/for every packageTest plan
workforce deploy ./examples/weekly-digest/persona.json --dry-runvalidates without side effectsworkforce deploy ./examples/weekly-digest/persona.json --bundle-out /tmp/wf-outemits a runnable bundleWEEKLY_DIGEST_TOPICS,WEEKLY_DIGEST_REPO,BRAVE_API_KEY,WORKFORCE_INTEGRATION_GITHUB_TOKEN,WORKFORCE_WORKSPACE_ID,WORKFORCE_WORKSPACE_TOKEN), piping acron.tickenvelope intoworkforce deploy --mode devruns the digest end-to-endworkforce deploy --mode cloudprints the documented "not yet available" message🤖 Generated with Claude Code