feat(compute): add compute service CLI commands#52
Conversation
Add 9 commands under `insforge compute`: - list: list all compute services - get: get service details - create: deploy from a pre-built Docker image - deploy: build Dockerfile remotely via flyctl and deploy to Fly.io - update: update service config (image, cpu, memory, port, env) - delete: destroy service and Fly.io resources - start: start a stopped service - stop: stop a running service - logs: view machine lifecycle events The deploy command shells out to flyctl for remote Docker builds (no local Docker required), generates a temporary fly.toml, and syncs machine state back to the InsForge backend after deployment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAdds a new top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| function getFlyToken(): string { | ||
| const config = getProjectConfig(); | ||
| // The backend knows the Fly token, but we need it for flyctl. | ||
| // Check env var first (user may have it set), then fall back to asking. | ||
| const token = process.env.FLY_API_TOKEN; |
There was a problem hiding this comment.
🟢 Low compute/deploy.ts:98
getFlyToken calls getProjectConfig() and assigns the result to config, but then ignores it and only reads from process.env.FLY_API_TOKEN. The dead variable suggests the config might have been intended as a fallback source for the token, or the line should simply be removed.
function getFlyToken(): string {
- const config = getProjectConfig();
- // The backend knows the Fly token, but we need it for flyctl.
// Check env var first (user may have it set), then fall back to asking.
const token = process.env.FLY_API_TOKEN;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/commands/compute/deploy.ts around lines 98-102:
`getFlyToken` calls `getProjectConfig()` and assigns the result to `config`, but then ignores it and only reads from `process.env.FLY_API_TOKEN`. The dead variable suggests the config might have been intended as a fallback source for the token, or the line should simply be removed.
Evidence trail:
src/commands/compute/deploy.ts lines 98-110 (REVIEWED_COMMIT): `getFlyToken()` function shows `const config = getProjectConfig();` on line 99, followed by code that only uses `process.env.FLY_API_TOKEN` and never references `config`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/commands/compute/deploy.ts (1)
99-100: Remove unusedconfigbinding (lint warning).Line 99 assigns
configbut never uses it. This is currently flagged by static analysis and should be cleaned up.Suggested fix
function getFlyToken(): string { - const config = getProjectConfig(); + getProjectConfig();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/compute/deploy.ts` around lines 99 - 100, Remove the unused local binding by deleting the line that calls getProjectConfig() (the `const config = getProjectConfig();` assignment) since `config` is never referenced; ensure any needed project configuration is fetched only where actually used (or rename to `_config` if intentionally unused) and keep the surrounding comment about the Fly token intact.src/commands/compute/get.ts (1)
23-31: Inconsistent fallback handling for optional fields.Only
endpointUrl(line 30) has a nullish coalescing fallback (?? 'n/a'). If other fields are missing from the API response, they'll print asundefined(e.g.,"Status: undefined"). Consider applying consistent fallback handling for a more robust user experience.💡 Optional: Apply consistent fallbacks
- outputInfo(`Name: ${service.name}`); - outputInfo(`ID: ${service.id}`); - outputInfo(`Status: ${service.status}`); - outputInfo(`Image: ${service.imageUrl}`); - outputInfo(`CPU: ${service.cpu}`); - outputInfo(`Memory: ${service.memory}MB`); - outputInfo(`Region: ${service.region}`); - outputInfo(`Endpoint: ${service.endpointUrl ?? 'n/a'}`); - outputInfo(`Created: ${service.createdAt}`); + outputInfo(`Name: ${service.name ?? 'n/a'}`); + outputInfo(`ID: ${service.id ?? 'n/a'}`); + outputInfo(`Status: ${service.status ?? 'n/a'}`); + outputInfo(`Image: ${service.imageUrl ?? 'n/a'}`); + outputInfo(`CPU: ${service.cpu ?? 'n/a'}`); + outputInfo(`Memory: ${service.memory ?? 'n/a'}MB`); + outputInfo(`Region: ${service.region ?? 'n/a'}`); + outputInfo(`Endpoint: ${service.endpointUrl ?? 'n/a'}`); + outputInfo(`Created: ${service.createdAt ?? 'n/a'}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/compute/get.ts` around lines 23 - 31, The output lines use outputInfo with several service.* fields but only service.endpointUrl uses a fallback, causing other missing fields to render "undefined"; update each output to provide a consistent fallback (e.g., use nullish coalescing `?? 'n/a'` or a more specific default like `'0'`/`'unknown'` where appropriate) for service.name, service.id, service.status, service.imageUrl, service.cpu, service.memory, service.region, service.endpointUrl, and service.createdAt so the CLI prints stable values when the API omits fields; keep using the same outputInfo calls and only change the interpolated expressions to include the fallback.src/commands/compute/create.ts (1)
27-29: Consider validating numeric options before sending to API.If a user provides non-numeric input (e.g.,
--port abc),Number()returnsNaN, which gets serialized and sent to the server. While the server should validate, client-side validation provides better UX with immediate feedback.💡 Optional: Add validation for port and memory
+ const port = Number(opts.port); + const memory = Number(opts.memory); + if (Number.isNaN(port) || port <= 0) { + throw new CLIError('Invalid value for --port: must be a positive number'); + } + if (Number.isNaN(memory) || memory <= 0) { + throw new CLIError('Invalid value for --memory: must be a positive number'); + } + const body: Record<string, unknown> = { name: opts.name, imageUrl: opts.image, - port: Number(opts.port), + port, cpu: opts.cpu, - memory: Number(opts.memory), + memory, region: opts.region, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/compute/create.ts` around lines 27 - 29, Validate numeric CLI options before sending to the API: check opts.port and opts.memory (and any other numeric opts) after parsing and before building the payload (where you currently use Number(opts.port) and Number(opts.memory)); ensure port is a valid integer within allowed range and memory is a finite positive number (use Number.isInteger/Number.isFinite checks or parseInt/parseFloat + isNaN), and display a clear error message and exit the command if validation fails instead of sending NaN to the server. Reference the code that constructs the payload (the lines using Number(opts.port), opts.cpu, Number(opts.memory)) to implement the checks and early-return on invalid input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/compute/deploy.ts`:
- Around line 239-244: The unconditional synchronous cleanup in the finally
block (unlinkSync/renameSync operating on existingTomlPath/backupTomlPath with
hadExistingToml) can throw and mask the real deploy result; wrap the unlinkSync
and renameSync calls in their own try/catch so any FS errors are logged or
warned (using the existing logger or console.warn) and not rethrown, ensuring
the original deploy error/result is preserved — do this inside the same finally
block around the unlinkSync/renameSync operations and avoid altering the outer
control flow (do not convert these to throws or let them overwrite the primary
error).
- Around line 208-212: Remove passing flyToken via the CLI and avoid leaving an
unconsumed pipe: update the spawn call that creates child (function using spawn
with args including '--access-token', flyAppId, json, dir) to drop the
'--access-token' / flyToken argument and instead set FLY_API_TOKEN in the spawn
options.env (merge with process.env). Also change stdio so when json is true use
'ignore' (not 'pipe') and when json is false keep 'inherit', ensuring no
stdout/stderr pipes remain unconsumed to prevent hangs.
In `@src/commands/compute/list.ts`:
- Around line 24-27: The early return inside the services.length === 0 branch
prevents the success usage reporting from running for an empty "compute list"
result; instead of returning immediately after console.log('No compute services
found.'), invoke the existing success usage-reporting call (the same reporter
used later for successful runs) before returning or refactor to let the common
success-reporting path execute for both empty and non-empty results, ensuring
the services check (services) and the console.log('No compute services found.')
message remain but do not skip the success-reporting step.
- Line 35: The code uses the expression `${s.memory ?? '-'}MB` which yields
`-MB` when s.memory is null/undefined; update the memory cell formatting to only
append "MB" when s.memory is present (e.g., use a conditional that returns
`${s.memory}MB` when s.memory exists, otherwise return `'-'`)—look for the
memory formatting in the list output where s.memory is referenced and replace
the interpolation accordingly.
In `@src/commands/compute/logs.ts`:
- Around line 27-30: The early return when logs is empty (the if
(!Array.isArray(logs) || logs.length === 0) block) prevents the subsequent
success usage reporting for the "compute logs" command from running; instead of
returning there, ensure you still execute the existing success/usage reporting
call after the logs check (i.e., do not return early — either run the reporting
before returning or remove the return and let the normal success reporting flow
execute), keeping the existing variable names (logs) and the surrounding compute
logs command handler intact.
- Around line 18-21: The current coercion "const limit = Number(opts.limit) ||
50" silently accepts invalid or negative values; replace it with explicit
validation: check opts.limit (the CLI flag) and if present parse it with
parseInt, ensure it's a positive integer (>0), otherwise return/throw a
user-facing error (or print validation message) instead of proceeding; only when
opts.limit is undefined use the default 50; then pass the validated numeric
limit into the ossFetch call for
`/api/compute/services/${encodeURIComponent(id)}/logs?limit=${limit}` to avoid
sending bad query values.
In `@src/commands/compute/update.ts`:
- Around line 25-27: The handlers that populate the request body (the local body
object) currently coerce opts.port and opts.memory with Number without
validation; instead, in the update handler use explicit numeric validation: when
opts.port or opts.memory are present, parse them (Number or parseInt as
appropriate) and check isFinite/!isNaN (and for port optionally that the value
is an integer in valid port range) before assigning to body.port/body.memory; if
validation fails, abort with a clear error message (e.g., throw or process.exit
with explanation) rather than inserting NaN into the payload; apply the same
pattern for opts.cpu if it must be numeric.
---
Nitpick comments:
In `@src/commands/compute/create.ts`:
- Around line 27-29: Validate numeric CLI options before sending to the API:
check opts.port and opts.memory (and any other numeric opts) after parsing and
before building the payload (where you currently use Number(opts.port) and
Number(opts.memory)); ensure port is a valid integer within allowed range and
memory is a finite positive number (use Number.isInteger/Number.isFinite checks
or parseInt/parseFloat + isNaN), and display a clear error message and exit the
command if validation fails instead of sending NaN to the server. Reference the
code that constructs the payload (the lines using Number(opts.port), opts.cpu,
Number(opts.memory)) to implement the checks and early-return on invalid input.
In `@src/commands/compute/deploy.ts`:
- Around line 99-100: Remove the unused local binding by deleting the line that
calls getProjectConfig() (the `const config = getProjectConfig();` assignment)
since `config` is never referenced; ensure any needed project configuration is
fetched only where actually used (or rename to `_config` if intentionally
unused) and keep the surrounding comment about the Fly token intact.
In `@src/commands/compute/get.ts`:
- Around line 23-31: The output lines use outputInfo with several service.*
fields but only service.endpointUrl uses a fallback, causing other missing
fields to render "undefined"; update each output to provide a consistent
fallback (e.g., use nullish coalescing `?? 'n/a'` or a more specific default
like `'0'`/`'unknown'` where appropriate) for service.name, service.id,
service.status, service.imageUrl, service.cpu, service.memory, service.region,
service.endpointUrl, and service.createdAt so the CLI prints stable values when
the API omits fields; keep using the same outputInfo calls and only change the
interpolated expressions to include the fallback.
🪄 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
Run ID: cb7d80b9-6e98-4adf-9594-2e961a8271d6
📒 Files selected for processing (10)
src/commands/compute/create.tssrc/commands/compute/delete.tssrc/commands/compute/deploy.tssrc/commands/compute/get.tssrc/commands/compute/list.tssrc/commands/compute/logs.tssrc/commands/compute/start.tssrc/commands/compute/stop.tssrc/commands/compute/update.tssrc/index.ts
| const child = spawn( | ||
| 'flyctl', | ||
| ['deploy', '--remote-only', '--app', flyAppId, '--access-token', flyToken, '--yes'], | ||
| { cwd: dir, stdio: json ? 'pipe' : 'inherit' }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does flyctl deploysupport authentication via theFLY_API_TOKEN environment variable, and what are the security implications of passing tokens as command-line arguments versus environment variables?
💡 Result:
Yes—flyctl deploy (and flyctl in general) can authenticate using a token from an environment variable:
- Fly’s official “Integrating flyctl” docs state that
FLY_ACCESS_TOKENcan be used to pass an access token to flyctl (the same token you’d use for deploys). [1] - Fly’s “Access tokens” docs show flyctl being authenticated via
FLY_API_TOKEN=…in an example, and Fly’s official GitHub Action for deployments usesFLY_API_TOKENin the environment when runningflyctl deploy. [2][3]
In practice, FLY_API_TOKEN is widely supported/used in CI examples, while FLY_ACCESS_TOKEN is the documented variable name in the “integrating” page. [1][3]
Security implications: CLI args vs environment variables
Passing tokens as command-line arguments (e.g., --access-token ...) is usually riskier because process arguments are commonly visible to other local users and tooling (e.g., ps, /proc/<pid>/cmdline) while the process runs, and they’re easy to accidentally capture in shell history and logs. [4]
Environment variables are often safer than args, but still not “secret storage.” They can be inherited by child processes and may be readable by other processes in the same environment/container; OWASP notes that when secrets are provided via environment variables, other processes in the container can read them. [5]
Rule of thumb: prefer a CI secret store that injects the token as an env var at runtime (as Fly’s action does), avoid putting secrets directly in command lines, and ensure logs don’t echo either. [3][4]
Sources: [1] Fly Docs – Integrating flyctl, [2] Fly Docs – Access tokens, [3] superfly/flyctl-actions README, [4] BashFAQ/069 (process args visibility), [5] OWASP Microservice Security (secrets via env vars visible to processes).
🏁 Script executed:
head -250 src/commands/compute/deploy.ts | tail -50Repository: InsForge/CLI
Length of output: 1878
Remove --access-token argument and pass token via environment variable to prevent secret exposure; fix unconsumed pipe that can cause hangs.
The code passes flyToken via the --access-token CLI argument, which is visible to other processes and users (via ps, /proc/<pid>/cmdline, etc.). Additionally, in JSON mode, stdio: 'pipe' is set but never consumed, leaving output buffered until the process exits—this can cause hangs on verbose output.
Use the FLY_API_TOKEN environment variable instead (supported by flyctl), and change stdio to 'ignore' in JSON mode since the output is not consumed by the code:
Suggested fix
const child = spawn(
'flyctl',
- ['deploy', '--remote-only', '--app', flyAppId, '--access-token', flyToken, '--yes'],
- { cwd: dir, stdio: json ? 'pipe' : 'inherit' },
+ ['deploy', '--remote-only', '--app', flyAppId, '--yes'],
+ {
+ cwd: dir,
+ stdio: json ? 'ignore' : 'inherit',
+ env: { ...process.env, FLY_API_TOKEN: flyToken },
+ },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const child = spawn( | |
| 'flyctl', | |
| ['deploy', '--remote-only', '--app', flyAppId, '--access-token', flyToken, '--yes'], | |
| { cwd: dir, stdio: json ? 'pipe' : 'inherit' }, | |
| ); | |
| const child = spawn( | |
| 'flyctl', | |
| ['deploy', '--remote-only', '--app', flyAppId, '--yes'], | |
| { | |
| cwd: dir, | |
| stdio: json ? 'ignore' : 'inherit', | |
| env: { ...process.env, FLY_API_TOKEN: flyToken }, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/deploy.ts` around lines 208 - 212, Remove passing
flyToken via the CLI and avoid leaving an unconsumed pipe: update the spawn call
that creates child (function using spawn with args including '--access-token',
flyAppId, json, dir) to drop the '--access-token' / flyToken argument and
instead set FLY_API_TOKEN in the spawn options.env (merge with process.env).
Also change stdio so when json is true use 'ignore' (not 'pipe') and when json
is false keep 'inherit', ensuring no stdout/stderr pipes remain unconsumed to
prevent hangs.
| } finally { | ||
| // Restore original fly.toml | ||
| unlinkSync(existingTomlPath); | ||
| if (hadExistingToml) { | ||
| renameSync(backupTomlPath, existingTomlPath); | ||
| } |
There was a problem hiding this comment.
Cleanup can throw and mask the real deploy result.
Lines 241-244 run unconditional sync FS operations in finally. If cleanup fails, it can overwrite the primary error (or turn a successful deploy into a failed command).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/deploy.ts` around lines 239 - 244, The unconditional
synchronous cleanup in the finally block (unlinkSync/renameSync operating on
existingTomlPath/backupTomlPath with hadExistingToml) can throw and mask the
real deploy result; wrap the unlinkSync and renameSync calls in their own
try/catch so any FS errors are logged or warned (using the existing logger or
console.warn) and not rethrown, ensuring the original deploy error/result is
preserved — do this inside the same finally block around the
unlinkSync/renameSync operations and avoid altering the outer control flow (do
not convert these to throws or let them overwrite the primary error).
| if (services.length === 0) { | ||
| console.log('No compute services found.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Empty-list path skips success usage reporting.
At Line 26, returning early means Line 40 is never reached, so successful compute list runs with no services are not reported.
Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/list.ts` around lines 24 - 27, The early return inside
the services.length === 0 branch prevents the success usage reporting from
running for an empty "compute list" result; instead of returning immediately
after console.log('No compute services found.'), invoke the existing success
usage-reporting call (the same reporter used later for successful runs) before
returning or refactor to let the common success-reporting path execute for both
empty and non-empty results, ensuring the services check (services) and the
console.log('No compute services found.') message remain but do not skip the
success-reporting step.
| String(s.status ?? '-'), | ||
| String(s.imageUrl ?? '-'), | ||
| String(s.cpu ?? '-'), | ||
| `${s.memory ?? '-'}MB`, |
There was a problem hiding this comment.
Avoid -MB placeholder in table output.
Line 35 prints -MB when memory is absent, which looks malformed. Prefer '-' for missing values.
Suggested fix
- `${s.memory ?? '-'}MB`,
+ s.memory == null ? '-' : `${String(s.memory)}MB`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `${s.memory ?? '-'}MB`, | |
| s.memory == null ? '-' : `${String(s.memory)}MB`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/list.ts` at line 35, The code uses the expression
`${s.memory ?? '-'}MB` which yields `-MB` when s.memory is null/undefined;
update the memory cell formatting to only append "MB" when s.memory is present
(e.g., use a conditional that returns `${s.memory}MB` when s.memory exists,
otherwise return `'-'`)—look for the memory formatting in the list output where
s.memory is referenced and replace the interpolation accordingly.
| const limit = Number(opts.limit) || 50; | ||
| const res = await ossFetch( | ||
| `/api/compute/services/${encodeURIComponent(id)}/logs?limit=${limit}`, | ||
| ); |
There was a problem hiding this comment.
Validate --limit as a positive integer instead of silently coercing.
At Line 18, invalid values (e.g. --limit abc, --limit -5) are coerced/fallback in a way that can hide user errors and send bad query values.
Suggested fix
- const limit = Number(opts.limit) || 50;
+ const limit = Number.parseInt(String(opts.limit), 10);
+ if (!Number.isInteger(limit) || limit <= 0) {
+ throw new Error('--limit must be a positive integer');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/logs.ts` around lines 18 - 21, The current coercion
"const limit = Number(opts.limit) || 50" silently accepts invalid or negative
values; replace it with explicit validation: check opts.limit (the CLI flag) and
if present parse it with parseInt, ensure it's a positive integer (>0),
otherwise return/throw a user-facing error (or print validation message) instead
of proceeding; only when opts.limit is undefined use the default 50; then pass
the validated numeric limit into the ossFetch call for
`/api/compute/services/${encodeURIComponent(id)}/logs?limit=${limit}` to avoid
sending bad query values.
| if (!Array.isArray(logs) || logs.length === 0) { | ||
| console.log('No logs found.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Early return skips success usage reporting.
At Line 29, returning on empty logs bypasses Line 36, so successful compute logs calls are not fully tracked.
Suggested fix
if (json) {
outputJson(logs);
} else {
if (!Array.isArray(logs) || logs.length === 0) {
console.log('No logs found.');
- return;
+ await reportCliUsage('cli.compute.logs', true);
+ return;
}
for (const entry of logs) {
const ts = new Date(entry.timestamp).toISOString();
console.log(`${ts} ${entry.message}`);
}
}
await reportCliUsage('cli.compute.logs', true);Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/logs.ts` around lines 27 - 30, The early return when
logs is empty (the if (!Array.isArray(logs) || logs.length === 0) block)
prevents the subsequent success usage reporting for the "compute logs" command
from running; instead of returning there, ensure you still execute the existing
success/usage reporting call after the logs check (i.e., do not return early —
either run the reporting before returning or remove the return and let the
normal success reporting flow execute), keeping the existing variable names
(logs) and the surrounding compute logs command handler intact.
| if (opts.port) body.port = Number(opts.port); | ||
| if (opts.cpu) body.cpu = opts.cpu; | ||
| if (opts.memory) body.memory = Number(opts.memory); |
There was a problem hiding this comment.
Validate numeric flags before adding them to request body.
At Lines 25-27, invalid values (--port abc, --memory nope) are coerced and can produce malformed payload fields, causing unclear backend failures.
Suggested fix
+ const parsePositiveInt = (value: string, flag: string): number => {
+ const parsed = Number.parseInt(value, 10);
+ if (!Number.isInteger(parsed) || parsed <= 0) {
+ throw new CLIError(`${flag} must be a positive integer`);
+ }
+ return parsed;
+ };
+
const body: Record<string, unknown> = {};
if (opts.image) body.imageUrl = opts.image;
- if (opts.port) body.port = Number(opts.port);
+ if (opts.port) body.port = parsePositiveInt(opts.port, '--port');
if (opts.cpu) body.cpu = opts.cpu;
- if (opts.memory) body.memory = Number(opts.memory);
+ if (opts.memory) body.memory = parsePositiveInt(opts.memory, '--memory');
if (opts.region) body.region = opts.region;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/compute/update.ts` around lines 25 - 27, The handlers that
populate the request body (the local body object) currently coerce opts.port and
opts.memory with Number without validation; instead, in the update handler use
explicit numeric validation: when opts.port or opts.memory are present, parse
them (Number or parseInt as appropriate) and check isFinite/!isNaN (and for port
optionally that the value is an integer in valid port range) before assigning to
body.port/body.memory; if validation fails, abort with a clear error message
(e.g., throw or process.exit with explanation) rather than inserting NaN into
the payload; apply the same pattern for opts.cpu if it must be numeric.
Tests the full CRUD lifecycle: list, create, get, logs, stop, start, delete — all via --json output against a real backend + Fly.io. Follows existing integration test pattern (INTEGRATION_TEST_ENABLED gate). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/integration/compute.test.ts`:
- Around line 124-131: Introduce a shared variable (e.g., createdServiceName:
string | undefined) at the top of the test file, set it in the create test where
the service name is generated, preserve the value in the delete test before
clearing any ids, and then in the it('compute list --json should be empty after
delete', ...) test use parseJsonOutput(result.stdout) and assert that no item
equals that specific createdServiceName (instead of checking the 'cli-test-'
prefix) so the assertion only verifies the service you created was removed.
- Line 50: The test at src/integration/compute.test.ts currently asserts
payload.status === 'running' immediately after calling the create command; since
src/commands/compute/create.ts returns the API's immediate response and
provisioning is asynchronous, change the test to either (a) retry/poll the
service status (e.g., loop with delay and timeout) until payload.status ===
'running' or a timeout is reached, using a small retry helper, or (b) relax the
assertion to allow intermediate statuses by asserting
expect(['creating','starting','running']).toContain(payload.status); update the
assertion line that references payload.status accordingly and/or add the retry
helper call before the assertion.
🪄 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
Run ID: fcafc6a5-92e7-47b3-b2e1-7c607504b22f
📒 Files selected for processing (1)
src/integration/compute.test.ts
| expect(payload).toHaveProperty('name'); | ||
| expect(payload).toHaveProperty('status'); | ||
| expect(payload).toHaveProperty('endpointUrl'); | ||
| expect(payload.status).toBe('running'); |
There was a problem hiding this comment.
Potential flaky test: Service may not be 'running' immediately after creation.
Based on src/commands/compute/create.ts, the create command returns the API's immediate response without polling. Since Fly.io service provisioning is asynchronous, the status might be 'creating', 'starting', or another intermediate value rather than 'running'.
Consider either:
- Polling/retrying until status becomes
'running'with a timeout - Asserting against a set of acceptable statuses:
expect(['creating', 'starting', 'running']).toContain(payload.status)
♻️ Suggested fix using retry with timeout
- expect(payload.status).toBe('running');
+ // Service may still be provisioning; poll until running or timeout
+ let currentStatus = payload.status;
+ const maxWaitMs = 60_000;
+ const pollIntervalMs = 2_000;
+ const start = Date.now();
+ while (currentStatus !== 'running' && Date.now() - start < maxWaitMs) {
+ await new Promise((r) => setTimeout(r, pollIntervalMs));
+ const getResult = await runCli(['--json', 'compute', 'get', payload.id as string], { apiUrl });
+ if (getResult.code === 0) {
+ const getPayload = parseJsonOutput(getResult.stdout) as Record<string, unknown>;
+ currentStatus = getPayload.status;
+ }
+ }
+ expect(currentStatus).toBe('running');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(payload.status).toBe('running'); | |
| // Service may still be provisioning; poll until running or timeout | |
| let currentStatus = payload.status; | |
| const maxWaitMs = 60_000; | |
| const pollIntervalMs = 2_000; | |
| const start = Date.now(); | |
| while (currentStatus !== 'running' && Date.now() - start < maxWaitMs) { | |
| await new Promise((r) => setTimeout(r, pollIntervalMs)); | |
| const getResult = await runCli(['--json', 'compute', 'get', payload.id as string], { apiUrl }); | |
| if (getResult.code === 0) { | |
| const getPayload = parseJsonOutput(getResult.stdout) as Record<string, unknown>; | |
| currentStatus = getPayload.status; | |
| } | |
| } | |
| expect(currentStatus).toBe('running'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/integration/compute.test.ts` at line 50, The test at
src/integration/compute.test.ts currently asserts payload.status === 'running'
immediately after calling the create command; since
src/commands/compute/create.ts returns the API's immediate response and
provisioning is asynchronous, change the test to either (a) retry/poll the
service status (e.g., loop with delay and timeout) until payload.status ===
'running' or a timeout is reached, using a small retry helper, or (b) relax the
assertion to allow intermediate statuses by asserting
expect(['creating','starting','running']).toContain(payload.status); update the
assertion line that references payload.status accordingly and/or add the retry
helper call before the assertion.
| it('compute list --json should be empty after delete', async () => { | ||
| const result = await runCli(['--json', 'compute', 'list'], { apiUrl }); | ||
| expectCliSuccess(result); | ||
|
|
||
| const payload = parseJsonOutput(result.stdout) as Record<string, unknown>[]; | ||
| const found = payload.find((s) => (s.name as string)?.startsWith('cli-test-')); | ||
| expect(found).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Fragile assertion: Checking all 'cli-test-*' services may cause false failures.
This test asserts that no services with names starting with 'cli-test-' exist. However, if parallel test runs or previous crashed runs left orphaned services, this will fail even though this specific test's delete succeeded.
Consider storing the service name before deletion (in the delete test) and verifying that specific name is absent:
♻️ Suggested fix
Add a variable to track the name:
let createdServiceName: string | undefined;In the create test, store it:
createdServiceId = payload.id as string;
+ createdServiceName = payload.name as string;In the delete test, preserve the name before clearing the id:
+ const deletedName = createdServiceName;
createdServiceId = undefined;
+ createdServiceName = undefined;Then in this test, check for the specific name:
- const found = payload.find((s) => (s.name as string)?.startsWith('cli-test-'));
- expect(found).toBeUndefined();
+ const found = payload.find((s) => s.name === deletedName);
+ expect(found, `Service "${deletedName}" should not exist after deletion`).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/integration/compute.test.ts` around lines 124 - 131, Introduce a shared
variable (e.g., createdServiceName: string | undefined) at the top of the test
file, set it in the create test where the service name is generated, preserve
the value in the delete test before clearing any ids, and then in the
it('compute list --json should be empty after delete', ...) test use
parseJsonOutput(result.stdout) and assert that no item equals that specific
createdServiceName (instead of checking the 'cli-test-' prefix) so the assertion
only verifies the service you created was removed.
…te 404 Error responses now include backend-provided nextActions guidance. Compute-specific 404s get a clear "upgrade or contact admin" message for backward compatibility with older backends. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
insforge compute: list, get, create, deploy, update, delete, start, stop, logsdeploycommand shells out toflyctl deploy --remote-onlyfor remote Docker builds (no local Docker needed)fly.tomlwith InsForge naming, restores original after deployfly.tomlfor port/memory/region defaults--jsonoutput, semantic exit codes, usage reportingDepends on: InsForge/InsForge#1062 (backend deploy + sync endpoints)
Test plan
compute deploytested with real Dockerfile project (audio-analyzer) — remote build, deploy, health check all passcompute listtable output verified with multiple services--jsonoutput verified for all commands🤖 Generated with Claude Code
Note
Add
computeCLI command group for managing compute servicescomputecommand to the CLI with subcommands:list,get,create,update,delete,start,stop,logs, anddeploy./api/compute/servicesREST endpoint, outputting JSON or human-readable text.deploysubcommand builds and deploys a local Dockerfile to Fly.io: it checks forflyctlandFLY_API_TOKEN, parses or generates afly.toml, runsflyctl deploy, and syncs state with the backend via PATCH.ossFetcherror handling to prefermessageovererror, appendnextActionswhen present, and surface a specific message for 404s on/api/computepaths.INTEGRATION_TEST_ENABLED.Macroscope summarized 1a38fee.
Summary by CodeRabbit