feat(box): curated 3-image box boot + #715 build-debt fixes - #731
Closed
law-chain-hot wants to merge 16 commits into
Closed
feat(box): curated 3-image box boot + #715 build-debt fixes#731law-chain-hot wants to merge 16 commits into
law-chain-hot wants to merge 16 commits into
Conversation
The pre-commit lint-fix hook runs `make lint:fix` repo-wide. Nine apps/api TypeScript files and four apps/runner Go files on the #730 baseline don't match the fixer's output (CI lints only changed files, so drift accumulated), so every commit reproduces this churn and aborts. Commit the fixer's own output once to clear the loop; verified idempotent.
- sdks/go: add WithPort so apps/runner compiles. #715 added boxlite.WithPort() call sites without the SDK function; the C ABI, Rust FFI, and runtime port-forwarding all already exist, only the Go layer was missing. Mirrors the WithVolume pattern; arg order (guest, host) matches the C ABI. - api test: fix OrganizationService mock argument order in the default-org-membership spec so configService lands in the right constructor slot (was crashing on configService.getOrThrow). Pre-existing on main; verified by reproducing on a clean checkout.
Minimal image rebuild after the box_template/catalog deletion: a hardcoded
allowlist of 3 curated images, no DB table. Runner pulls any OCI ref directly
(runtime.Create(artifactRef)), so the catalog lives in code.
- curated-images.constant.ts: keys 'base'|'python'|'node' -> ghcr digest refs
from BOXLITE_SYSTEM_{BASE,PYTHON,NODE}_IMAGE env (already set in sst.config),
resolveCuratedImageRef() validates at the API boundary (400 if off-list)
- create-box.dto (internal) gains image?; mapper now threads dto.image (was dropped)
- box.service.createBoxInternal: resolve ref, stash on box.labels['boxlite.io/image-ref']
(zero migration)
- runnerAdapter.createBox (v2): enqueue CREATE_BOX job with hand-built payload whose
key is artifactRef (NOT the generated client's snapshot field — V2 Go validate trap)
- box-start.action: read the label, call createBox, set CREATING; existing
handleCreateBoxJobCompletion drives CREATING->STARTED
api/src tsc 0. Contract = opaque keys (user can't pass arbitrary OCI ref). Images
stay private ghcr (runner token already wired). Dashboard 3-image picker is a
separate follow-up (mid-rebuild).
…ho curated key
Three follow-ups on the curated-image create path:
- replaceLabels must not let a user-supplied label overwrite the reserved
boxlite.io/image-ref label: it holds the resolved curated OCI ref the
runner pulls, so overwriting it would escape the curated allowlist and
make the runner pull an arbitrary image with its private-registry token.
- createForWarmPool must stash the default curated image ref on the same
reserved label; without it warm-pool boxes ERROR on start ("missing
image ref") and the refill loop recreates them indefinitely.
- Box responses now echo the curated key the box was created with
(reverse-mapped from the reserved label) instead of a hardcoded '';
the shared REST contract documents that hosted deployments restrict
image to the curated keys.
Unit tests cover the allowlist boundary (invalid key -> 400), the
CREATE_BOX payload contract (artifactRef key, never snapshot), the
reserved-label guard, the warm-pool default, and the create/echo mapping.
…image field Generated with the repo's own pipeline from this branch's API (the DTO gained image two commits ago): spec via apps/api generate-openapi.ts (SKIP_CONNECTIONS=true), then openapi-generator-cli 7.23.0 (typescript-axios / go with the repo's templates) plus the hack/*/postprocess.sh scripts, matching apps/libs/api-client/project.json and apps/api-client-go/project.json command-for-command. CI's api-client-drift workflow regenerates and diffs these clients, so the regen output must be committed once the DTO changes.
The API now accepts only curated image keys (base|python|node) on box create. Switch the suite's image literals from alpine:3.23 to the 'base' key; bootstrap maps the curated env overrides to the same public refs the suite pulled before, so runner-side behavior is unchanged. Drop fixture_setup's snapshot registration: it targets the /snapshots endpoint and snapshot table that the box_template removal deleted (bootstrap hard-fails on main today). e2e-stack has been red on main since that removal; this adaptation is a precondition for it to go green again.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds curated image-key support end-to-end (schema, resolver, storage as reserved box label, runner create flow), updates dashboard and TypeScript SDK to use curated keys, changes many e2e defaults/fixtures to use curated keys, adds Go SDK port-forwarding, updates CI workflows, and introduces a squashed pre-deploy migration baseline. ChangesCurated Box Images
Sequence Diagram (create + boot path) sequenceDiagram
actor Client
participant Dashboard
participant API
participant BoxService
participant RunnerAdapterV2
Client->>Dashboard: user selects image key (e.g. "base")
Dashboard->>API: POST /boxes { image: "base", ... }
API->>BoxService: createBox(dto)
BoxService->>BoxService: resolveCuratedImageRef('base') -> OCI ref
BoxService->>API: persist box with BOX_IMAGE_REF_LABEL
BoxService->>RunnerAdapterV2: runnerAdapter.createBox(box, artifactRef)
RunnerAdapterV2->>JobService: createJob(CREATE_BOX, payload with artifactRef)
E2E, Fixture, and CI
Go SDK Port Forwarding
Migrations
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
sdks/go/options.go (1)
170-176: ⚡ Quick winConsider validating port ranges.
The function does not validate that
guestPortandhostPortare within the valid range[0, 65535]. The downstream Rust layer (context snippet 2) casts these values tou16, so negative values or values exceeding 65535 will cause wrapping or truncation errors. Adding early validation would fail fast with a clear error message at the API boundary.🛡️ Proposed validation
func WithPort(guestPort, hostPort int) BoxOption { return func(c *boxConfig) { + if guestPort < 1 || guestPort > 65535 { + // Consider returning error or panicking + } + if hostPort < 0 || hostPort > 65535 { + // Consider returning error or panicking + } c.ports = append(c.ports, portEntry{guestPort, hostPort}) } }Note: Since
BoxOptionis currentlyfunc(*boxConfig), validation would require either panic or changing the signature to return an error. Alternatively, validation could be deferred tobuildCOptions.🤖 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 `@sdks/go/options.go` around lines 170 - 176, Validate port ranges before casting to u16: add a check in the conversion layer (e.g., buildCOptions that consumes boxConfig and portEntry) to ensure each portEntry.guestPort and portEntry.hostPort is between 0 and 65535, and return a clear error if not; if you cannot change buildCOptions' return type, then instead validate in WithPort (or change BoxOption signature) and either return an error from the builder or panic with a descriptive message. Ensure you reference portEntry, WithPort, boxConfig, and buildCOptions so callers fail fast at the Go API boundary rather than allowing silent truncation when values are cast to u16.apps/api/src/box/constants/curated-images.constant.ts (2)
74-79: 💤 Low valueMinor:
curatedImageKeyForRefcallsresolveCuratedImageRefup to 3 times.The
findcallback invokesresolveCuratedImageRef(key)for each curated key until a match is found. With only 3 keys this overhead is negligible, but the pattern could be optimized by inverting the mapping (building a ref→key lookup once) if the allowlist grows.Not actionable now, but worth noting for future maintainers.
🤖 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 `@apps/api/src/box/constants/curated-images.constant.ts` around lines 74 - 79, curatedImageKeyForRef currently calls resolveCuratedImageRef repeatedly inside CURATED_IMAGE_KEYS.find which recomputes refs multiple times; to fix, build a single lookup mapping from resolved ref to CuratedImageKey (e.g., a module-level CONST like CURATED_IMAGE_REF_MAP computed once from CURATED_IMAGE_KEYS using resolveCuratedImageRef) and then have curatedImageKeyForRef return CURATED_IMAGE_REF_MAP[ref] (or undefined) after the early ref falsy check so each ref is resolved only once and lookups are O(1).
25-47: ⚡ Quick winConsider adding automated verification for fallback digest freshness.
The hardcoded SHA256 fallback refs provide good security, but they risk drifting from the "official" refs deployed via SST config. While the comment acknowledges they should be kept in sync, there's no automated check to detect staleness.
Consider adding a CI check or integration test that compares these fallback digests against the actual deployed image digests to catch drift early.
🤖 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 `@apps/api/src/box/constants/curated-images.constant.ts` around lines 25 - 47, Add an automated CI check that verifies the hardcoded fallback sha256 refs in CURATED_IMAGE_ENV remain in sync with the deployed/expected image digests: create a small script or test (e.g., verify-curated-images or a Jest test) that iterates CURATED_IMAGE_ENV, for each entry reads the env var named by envVar (fallback to the hardcoded fallbackRef if unset), queries the registry (or the SST/deployed config) for the actual image digest for that image reference, and fails with a clear message if the digest differs from fallbackRef; ensure the check runs in CI and returns non-zero on mismatch so drift is caught automatically.apps/api/src/box/constants/curated-images.constant.spec.ts (1)
33-37: 💤 Low valueOptional: Consider tightening test assertions.
The tests use
toContainto check for theghcr.io/boxlite-ai/boxlite-agent-{base,python,node}@sha256:prefix, which validates the format but not the exact digest. While this approach reduces test brittleness when digests change, it also means a wrong digest would pass.Consider whether exact matches (e.g.,
toBe('ghcr.io/...')) would better catch regressions, accepting that the tests will need updating when the fallback refs are rotated.🤖 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 `@apps/api/src/box/constants/curated-images.constant.spec.ts` around lines 33 - 37, Tighten the assertions in the test for resolveCuratedImageRef by either asserting exact expected refs or validating the full digest format: replace the toContain checks in the it('resolves each key to its private ghcr ref', ...) block with exact equality assertions using toBe('ghcr.io/boxlite-ai/boxlite-agent-base@sha256:<expected-digest>') for base/python/node if you want strict matching, or use a regex-based assertion (e.g., toMatch(/^ghcr\.io\/boxlite-ai\/boxlite-agent-(base|python|node)`@sha256`:[0-9a-f]{64}$/)) to ensure the full sha256 digest format is correct while avoiding brittle hardcoded digests.apps/api/src/box/dto/create-box.dto.ts (1)
22-28: ⚡ Quick winConsider adding enum validation at the DTO boundary.
The description states the field accepts "one of: base, python, node", but there's no
@IsIn(['base', 'python', 'node'])decorator to enforce this constraint at the DTO validation layer. WhileresolveCuratedImageRefvalidates downstream, rejecting invalid keys at the DTO boundary provides earlier feedback, clearer OpenAPI documentation with an enum constraint, and prevents invalid values from reaching service logic.🔒 Proposed validation enhancement
+import { IsEnum, IsIn, IsObject, IsOptional, IsString, IsNumber, IsBoolean, IsArray } from 'class-validator' -import { IsEnum, IsObject, IsOptional, IsString, IsNumber, IsBoolean, IsArray } from 'class-validator' +export type CuratedImageKey = 'base' | 'python' | 'node'; + `@ApiSchema`({ name: 'CreateBox' }) export class CreateBoxDto { `@ApiPropertyOptional`({ description: 'The curated image key for the box (one of: base, python, node). Defaults to base.', example: 'base', + enum: ['base', 'python', 'node'], }) `@IsOptional`() `@IsString`() + `@IsIn`(['base', 'python', 'node']) image?: string🤖 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 `@apps/api/src/box/dto/create-box.dto.ts` around lines 22 - 28, Add enum-level validation and OpenAPI enum metadata for the image field on the CreateBoxDto: import and apply `@IsIn`(['base','python','node']) to the image property and update the `@ApiPropertyOptional` decorator to include enum: ['base','python','node'] (and ensure you import IsIn from class-validator). This enforces valid keys at DTO validation time and surfaces the allowed values in the generated OpenAPI schema for the image property.apps/api/src/box/runner-adapter/runnerAdapter.v2.createBox.spec.ts (1)
7-7: 💤 Low valueRemove unused uuid mock.
The
uuidmodule is mocked to return a deterministic value, but it's never imported or used anywhere in this test file. The mock appears to be leftover from copy-paste or a misunderstanding.🧹 Suggested fix
-jest.mock('uuid', () => ({ v4: () => '00000000-0000-4000-8000-000000000000' })) - import { Box } from '../entities/box.entity'🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.v2.createBox.spec.ts` at line 7, Remove the unused uuid mock declaration in the test file: delete the jest.mock('uuid', () => ({ v4: () => '00000000-0000-4000-8000-000000000000' })) line from runnerAdapter.v2.createBox.spec.ts since no code in this spec imports or uses uuid; ensure no leftover imports/reference to uuid remain after removing the mock.apps/api/src/box/runner-adapter/runnerAdapter.v0.ts (1)
147-150: ⚡ Quick winAdd docstring for clarity.
While this method intentionally throws an error for V0 runners, it should still have a docstring explaining that this operation is unsupported for V0 runners and why. As per coding guidelines, public methods require comprehensive docstrings.
📝 Suggested docstring
+ /** + * V0 runners do not support direct box creation via HTTP. + * Only V2 (job-based) runners support createBox. + * `@throws` {Error} Always throws as this operation is not supported + */ async createBox(_box: Box, _artifactRef: string): Promise<void> {🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.v0.ts` around lines 147 - 150, Add a JSDoc comment above the public async createBox(_box: Box, _artifactRef: string): Promise<void> method explaining that create is intentionally unsupported for V0 runners, that V0 uses direct HTTP which is out of MVP scope, and that box creation is implemented only in the V2/job-based runner; include mention that the method always throws an Error('createBox is not supported for V0 runners') so callers should use the V2 adapter or job API instead.Source: Coding guidelines
apps/api/src/box/managers/box-actions/box-start.action.ts (1)
71-77: 💤 Low valueConsider a more user-friendly error message.
The error message exposes the internal label name
boxlite.io/image-ref, which may confuse users unfamiliar with the system's internals. Consider rephrasing to focus on the user-facing concept (missing image configuration) rather than the implementation detail (missing label).💬 Suggested improvement
await this.updateBoxState( box, BoxState.ERROR, lockCode, undefined, - `Box has no image ref (missing label ${BOX_IMAGE_REF_LABEL})`, + 'Box creation failed: no image configuration found. This box may have been created before the curated image feature was enabled.', )🤖 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 `@apps/api/src/box/managers/box-actions/box-start.action.ts` around lines 71 - 77, The error message sent via updateBoxState when setting BoxState.ERROR currently exposes the internal label BOX_IMAGE_REF_LABEL; change the message to a user-friendly phrasing like "Box has no image configured" (or "Missing image configuration for box") and keep the internal label constant for logs only if needed; update the call in box-start.action.ts that invokes updateBoxState (the block using BOX_IMAGE_REF_LABEL and BoxState.ERROR) to pass the user-facing text and, if you still want to capture the missing label for debugging, add a separate debug/processLogger.debug log that references BOX_IMAGE_REF_LABEL rather than including it in the user-facing error string.apps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.ts (1)
50-74: ⚡ Quick winConsider adding test coverage for the legacy ref fallback path.
The mapper's fallback chain (line 24 in the mapper) returns the raw ref when
curatedImageKeyForRefreturns undefined (e.g., after env rotation or for legacy boxes). The existing tests cover the curated-key happy path and the missing-label case, but not the scenario where a label exists but doesn't match any current curated ref.📝 Suggested test case
+ + it('returns the raw ref when the label holds a non-curated OCI ref', () => { + const response = boxToBoxResponse({ + boxId: 'aB3cD4eF5gH6', + state: 'started', + labels: { [BOX_IMAGE_REF_LABEL]: 'custom-registry.io/my-image:v1' }, + } as unknown as BoxDto) + + expect(response.image).toBe('custom-registry.io/my-image:v1') + })🤖 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 `@apps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.ts` around lines 50 - 74, Add a test that covers the legacy ref fallback: call boxToBoxResponse with a BoxDto whose labels include BOX_IMAGE_REF_LABEL set to a raw/legacy ref string (one that curatedImageKeyForRef would not map), and assert that response.image equals that raw ref; place it alongside the existing tests in box-to-box.mapper.spec.ts to validate the mapper returns the raw ref when curatedImageKeyForRef returns undefined.
🤖 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 `@apps/api-client-go/api/openapi.yaml`:
- Around line 10251-10255: The OpenAPI schema for CreateBox.image is currently
an unconstrained string; update the CreateBox schema's image property to include
an enum with the allowed curated keys ["base","python","node"] (and you can keep
the existing example/default "base") so generated clients are prevented from
sending invalid values; locate the CreateBox schema and modify the image
property to add the enum array while retaining type: string and the description.
In `@apps/api/src/box/constants/curated-images.constant.spec.ts`:
- Around line 29-52: Add unit tests for the exported curatedImageKeyForRef
function in curated-images.constant.spec.ts: call
resolveCuratedImageRef('base'|'python'|'node') and assert
curatedImageKeyForRef(thatRef) returns 'base'|'python'|'node' respectively;
assert curatedImageKeyForRef('ghcr.io/unknown/image@sha256:deadbeef') returns
undefined; assert curatedImageKeyForRef(undefined) returns undefined; ensure
tests reference CURATED_IMAGE_KEYS, resolveCuratedImageRef and
curatedImageKeyForRef to locate the functions and reset any env overrides (e.g.,
BOXLITE_SYSTEM_PYTHON_IMAGE) to avoid flakiness.
In `@apps/api/src/box/managers/box-actions/box-start.action.ts`:
- Around line 81-85: Wrap the runnerAdapter.createBox call in a try/catch inside
the method in box-start.action (where runnerAdapter is created) so any exception
transitions the box to an error state: on success continue to call
this.updateBoxState(box, BoxState.CREATING, lockCode) and return SYNC_AGAIN; on
failure call this.updateBoxState(box, BoxState.ERROR, lockCode) (include the
error message in logs via processLogger/errorLogger) and return a terminal or
retry status as appropriate. Ensure you reference runnerAdapter.createBox,
this.updateBoxState, BoxState.CREATING and BoxState.ERROR so the catch handles
runner unavailability/job creation failures and prevents the box remaining in
UNKNOWN.
In `@apps/api/src/box/runner-adapter/runnerAdapter.ts`:
- Line 49: Add a descriptive docstring above the public createBox method
declaration explaining its purpose (what creating a Box entails), the parameters
(Box — describe expected fields/shape and artifactRef — what format/meaning),
the return type (Promise<void> and when it resolves/rejects), any side effects
(e.g., network calls, state changes, persisted artifacts), and possible
errors/exceptions it may throw; place this docstring directly above the
createBox(signature) in runnerAdapter.ts (the public interface method named
createBox) so callers understand usage, preconditions, and failure modes.
In `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts`:
- Around line 117-142: Add a clear docstring to the public async createBox(box:
Box, artifactRef: string): Promise<void> method that states the purpose,
describes the required payload contract (must match Go dto.CreateBoxDTO json
tags), lists parameters (box and artifactRef) and their important fields used
(id, boxId, organizationId, osUser, cpu/gpu/mem/disk quotas, env,
networkBlockAll, networkAllowList, authToken, region), and mentions side effects
(it enqueues a CREATE_BOX job via this.jobService.createJob and logs the
action); also include a note to keep this payload in sync with dto.CreateBoxDTO
and where to update if the Go DTO changes (reference dto.CreateBoxDTO in the
comment).
- Around line 121-137: The payload built in runnerAdapter.v2.ts (the payload
object in the runnerAdapter v2 flow) currently includes all required
CreateBoxDTO fields but omits optional keys (entrypoint, fromVolumeId, metadata,
otelEndpoint, registry, skipStart, volumes); review the Go CreateBoxDTO and
either (a) add these optional properties to the payload with the corresponding
box.* or derived values (e.g., box.entrypoint, box.fromVolumeId, box.metadata,
box.otelEndpoint, box.registry, box.skipStart, box.volumes) so the backend
receives explicit values, or (b) confirm backend defaults/validation allow
omission and add comments referencing CreateBoxDTO to justify each excluded
field. Ensure the payload variable and any serialization code preserve correct
naming (artifactRef, cpuQuota, gpuQuota, id, memoryQuota, osUser, storageQuota,
userId) and type shapes expected by CreateBoxDTO.
In `@openapi/box.openapi.yaml`:
- Around line 1527-1533: The OpenAPI schema's image field has a mismatched
default: the description for the "image" property says the hosted deployment
default is `base` (and curated allowlist includes `base`, `python`, `node`) but
the schema default is set to "alpine:latest"; update the schema default for the
"image" property to "base" so it matches the description and curated allowlist
(look for the "image" property/default and description in the OpenAPI fragment
and change default: "alpine:latest" to default: "base").
In `@sdks/go/options.go`:
- Around line 170-176: The docstring for WithPort is misleading — update the
comment above the WithPort(guestPort, hostPort int) function to state that a
hostPort <= 0 does NOT request an OS-assigned ephemeral port but instead results
in a 1:1 mapping where the host port mirrors the guest port value; reference the
WithPort function and the boxConfig.ports/portEntry usage so readers know this
deterministic behavior (or, if true dynamic allocation is desired, change the
mapping logic where c.ports is appended to interpret 0 as a kernel-assigned port
and document that behavior instead).
---
Nitpick comments:
In `@apps/api/src/box/constants/curated-images.constant.spec.ts`:
- Around line 33-37: Tighten the assertions in the test for
resolveCuratedImageRef by either asserting exact expected refs or validating the
full digest format: replace the toContain checks in the it('resolves each key to
its private ghcr ref', ...) block with exact equality assertions using
toBe('ghcr.io/boxlite-ai/boxlite-agent-base@sha256:<expected-digest>') for
base/python/node if you want strict matching, or use a regex-based assertion
(e.g.,
toMatch(/^ghcr\.io\/boxlite-ai\/boxlite-agent-(base|python|node)`@sha256`:[0-9a-f]{64}$/))
to ensure the full sha256 digest format is correct while avoiding brittle
hardcoded digests.
In `@apps/api/src/box/constants/curated-images.constant.ts`:
- Around line 74-79: curatedImageKeyForRef currently calls
resolveCuratedImageRef repeatedly inside CURATED_IMAGE_KEYS.find which
recomputes refs multiple times; to fix, build a single lookup mapping from
resolved ref to CuratedImageKey (e.g., a module-level CONST like
CURATED_IMAGE_REF_MAP computed once from CURATED_IMAGE_KEYS using
resolveCuratedImageRef) and then have curatedImageKeyForRef return
CURATED_IMAGE_REF_MAP[ref] (or undefined) after the early ref falsy check so
each ref is resolved only once and lookups are O(1).
- Around line 25-47: Add an automated CI check that verifies the hardcoded
fallback sha256 refs in CURATED_IMAGE_ENV remain in sync with the
deployed/expected image digests: create a small script or test (e.g.,
verify-curated-images or a Jest test) that iterates CURATED_IMAGE_ENV, for each
entry reads the env var named by envVar (fallback to the hardcoded fallbackRef
if unset), queries the registry (or the SST/deployed config) for the actual
image digest for that image reference, and fails with a clear message if the
digest differs from fallbackRef; ensure the check runs in CI and returns
non-zero on mismatch so drift is caught automatically.
In `@apps/api/src/box/dto/create-box.dto.ts`:
- Around line 22-28: Add enum-level validation and OpenAPI enum metadata for the
image field on the CreateBoxDto: import and apply
`@IsIn`(['base','python','node']) to the image property and update the
`@ApiPropertyOptional` decorator to include enum: ['base','python','node'] (and
ensure you import IsIn from class-validator). This enforces valid keys at DTO
validation time and surfaces the allowed values in the generated OpenAPI schema
for the image property.
In `@apps/api/src/box/managers/box-actions/box-start.action.ts`:
- Around line 71-77: The error message sent via updateBoxState when setting
BoxState.ERROR currently exposes the internal label BOX_IMAGE_REF_LABEL; change
the message to a user-friendly phrasing like "Box has no image configured" (or
"Missing image configuration for box") and keep the internal label constant for
logs only if needed; update the call in box-start.action.ts that invokes
updateBoxState (the block using BOX_IMAGE_REF_LABEL and BoxState.ERROR) to pass
the user-facing text and, if you still want to capture the missing label for
debugging, add a separate debug/processLogger.debug log that references
BOX_IMAGE_REF_LABEL rather than including it in the user-facing error string.
In `@apps/api/src/box/runner-adapter/runnerAdapter.v0.ts`:
- Around line 147-150: Add a JSDoc comment above the public async
createBox(_box: Box, _artifactRef: string): Promise<void> method explaining that
create is intentionally unsupported for V0 runners, that V0 uses direct HTTP
which is out of MVP scope, and that box creation is implemented only in the
V2/job-based runner; include mention that the method always throws an
Error('createBox is not supported for V0 runners') so callers should use the V2
adapter or job API instead.
In `@apps/api/src/box/runner-adapter/runnerAdapter.v2.createBox.spec.ts`:
- Line 7: Remove the unused uuid mock declaration in the test file: delete the
jest.mock('uuid', () => ({ v4: () => '00000000-0000-4000-8000-000000000000' }))
line from runnerAdapter.v2.createBox.spec.ts since no code in this spec imports
or uses uuid; ensure no leftover imports/reference to uuid remain after removing
the mock.
In `@apps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.ts`:
- Around line 50-74: Add a test that covers the legacy ref fallback: call
boxToBoxResponse with a BoxDto whose labels include BOX_IMAGE_REF_LABEL set to a
raw/legacy ref string (one that curatedImageKeyForRef would not map), and assert
that response.image equals that raw ref; place it alongside the existing tests
in box-to-box.mapper.spec.ts to validate the mapper returns the raw ref when
curatedImageKeyForRef returns undefined.
In `@sdks/go/options.go`:
- Around line 170-176: Validate port ranges before casting to u16: add a check
in the conversion layer (e.g., buildCOptions that consumes boxConfig and
portEntry) to ensure each portEntry.guestPort and portEntry.hostPort is between
0 and 65535, and return a clear error if not; if you cannot change
buildCOptions' return type, then instead validate in WithPort (or change
BoxOption signature) and either return an error from the builder or panic with a
descriptive message. Ensure you reference portEntry, WithPort, boxConfig, and
buildCOptions so callers fail fast at the Go API boundary rather than allowing
silent truncation when values are cast to u16.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ada88c4-e146-4a72-857f-545c6f23d462
📒 Files selected for processing (46)
apps/api-client-go/api/openapi.yamlapps/api-client-go/model_create_box.goapps/api/src/admin/services/observability.service.tsapps/api/src/app.service.tsapps/api/src/box/box.module.tsapps/api/src/box/constants/curated-images.constant.spec.tsapps/api/src/box/constants/curated-images.constant.tsapps/api/src/box/controllers/runner.controller.tsapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/managers/box-actions/box-start.action.tsapps/api/src/box/runner-adapter/runnerAdapter.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.createBox.spec.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.replace-labels.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/box.service.warm-pool.spec.tsapps/api/src/box/services/job-state-handler.service.tsapps/api/src/box/services/volume.service.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/api/src/config/configuration.tsapps/api/src/organization/organization.module.tsapps/api/src/organization/services/default-organization-membership.spec.tsapps/libs/api-client/src/docs/CreateBox.mdapps/libs/api-client/src/models/create-box.tsapps/runner/pkg/boxlite/daemon_env_test.goapps/runner/pkg/boxlite/toolbox_ports.goapps/runner/pkg/boxlite/toolbox_ports_test.goapps/runner/pkg/models/enums/box_state.goopenapi/box.openapi.yamlscripts/test/e2e/README.mdscripts/test/e2e/bootstrap.shscripts/test/e2e/cases/conftest.pyscripts/test/e2e/cases/test_c_entry.pyscripts/test/e2e/cases/test_cli_detach_recovery.pyscripts/test/e2e/cases/test_cli_entry.pyscripts/test/e2e/cases/test_error_code_mapping.pyscripts/test/e2e/cases/test_go_entry.pyscripts/test/e2e/cases/test_node_entry.pyscripts/test/e2e/cases/test_quota_enforcement.pyscripts/test/e2e/fixture_setup.pyscripts/test/e2e/sdks/c/e2e_basic.cscripts/test/e2e/sdks/go/e2e_basic.goscripts/test/e2e/sdks/node/e2e_basic.tssdks/go/options.go
💤 Files with no reviewable changes (1)
- apps/api/src/app.service.ts
Comment on lines
+10251
to
+10255
| image: | ||
| description: "The curated image key for the box (one of: base, python, node).\ | ||
| \ Defaults to base." | ||
| example: base | ||
| type: string |
There was a problem hiding this comment.
Constrain CreateBox.image with an enum to preserve the client/server contract
image is currently a free-form string in the schema, but the API behavior is curated-key only (base, python, node). This mismatch lets generated clients send invalid values that fail at runtime.
Suggested OpenAPI tightening
image:
description: "The curated image key for the box (one of: base, python, node).\
\ Defaults to base."
example: base
+ enum:
+ - base
+ - python
+ - node
+ default: base
type: string📝 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.
Suggested change
| image: | |
| description: "The curated image key for the box (one of: base, python, node).\ | |
| \ Defaults to base." | |
| example: base | |
| type: string | |
| image: | |
| description: "The curated image key for the box (one of: base, python, node).\ | |
| \ Defaults to base." | |
| example: base | |
| enum: | |
| - base | |
| - python | |
| - node | |
| default: base | |
| type: string |
🤖 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 `@apps/api-client-go/api/openapi.yaml` around lines 10251 - 10255, The OpenAPI
schema for CreateBox.image is currently an unconstrained string; update the
CreateBox schema's image property to include an enum with the allowed curated
keys ["base","python","node"] (and you can keep the existing example/default
"base") so generated clients are prevented from sending invalid values; locate
the CreateBox schema and modify the image property to add the enum array while
retaining type: string and the description.
Comment on lines
+29
to
+52
| it('exposes exactly the three curated keys', () => { | ||
| expect(CURATED_IMAGE_KEYS).toEqual(['base', 'python', 'node']) | ||
| }) | ||
|
|
||
| it('resolves each key to its private ghcr ref', () => { | ||
| expect(resolveCuratedImageRef('base')).toContain('ghcr.io/boxlite-ai/boxlite-agent-base@sha256:') | ||
| expect(resolveCuratedImageRef('python')).toContain('ghcr.io/boxlite-ai/boxlite-agent-python@sha256:') | ||
| expect(resolveCuratedImageRef('node')).toContain('ghcr.io/boxlite-ai/boxlite-agent-node@sha256:') | ||
| }) | ||
|
|
||
| it('defaults to base when no key is supplied', () => { | ||
| expect(resolveCuratedImageRef(undefined)).toBe(resolveCuratedImageRef('base')) | ||
| }) | ||
|
|
||
| it('prefers the env-configured ref over the pinned fallback', () => { | ||
| process.env.BOXLITE_SYSTEM_PYTHON_IMAGE = 'ghcr.io/boxlite-ai/override@sha256:deadbeef' | ||
| expect(resolveCuratedImageRef('python')).toBe('ghcr.io/boxlite-ai/override@sha256:deadbeef') | ||
| }) | ||
|
|
||
| it('rejects any key outside the allowlist at the boundary (no arbitrary OCI ref)', () => { | ||
| expect(() => resolveCuratedImageRef('alpine:3.23')).toThrow(BadRequestError) | ||
| expect(() => resolveCuratedImageRef('ghcr.io/evil/image:latest')).toThrow(BadRequestError) | ||
| expect(() => resolveCuratedImageRef('ubuntu')).toThrow(BadRequestError) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for curatedImageKeyForRef.
The test suite comprehensively covers resolveCuratedImageRef, but the exported curatedImageKeyForRef function has no tests. This function performs reverse-mapping from OCI ref to curated key and should be tested for:
- Returning the correct key when given a resolved ref (for each curated key)
- Returning
undefinedwhen given an unknown ref - Returning
undefinedwhen givenundefinedas input
✅ Suggested test cases to add
+ it('reverse-maps a resolved ref back to its curated key', () => {
+ const baseRef = resolveCuratedImageRef('base')
+ const pythonRef = resolveCuratedImageRef('python')
+ const nodeRef = resolveCuratedImageRef('node')
+
+ expect(curatedImageKeyForRef(baseRef)).toBe('base')
+ expect(curatedImageKeyForRef(pythonRef)).toBe('python')
+ expect(curatedImageKeyForRef(nodeRef)).toBe('node')
+ })
+
+ it('returns undefined for unknown refs', () => {
+ expect(curatedImageKeyForRef('ghcr.io/unknown/image@sha256:abc123')).toBeUndefined()
+ expect(curatedImageKeyForRef(undefined)).toBeUndefined()
+ })📝 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.
Suggested change
| it('exposes exactly the three curated keys', () => { | |
| expect(CURATED_IMAGE_KEYS).toEqual(['base', 'python', 'node']) | |
| }) | |
| it('resolves each key to its private ghcr ref', () => { | |
| expect(resolveCuratedImageRef('base')).toContain('ghcr.io/boxlite-ai/boxlite-agent-base@sha256:') | |
| expect(resolveCuratedImageRef('python')).toContain('ghcr.io/boxlite-ai/boxlite-agent-python@sha256:') | |
| expect(resolveCuratedImageRef('node')).toContain('ghcr.io/boxlite-ai/boxlite-agent-node@sha256:') | |
| }) | |
| it('defaults to base when no key is supplied', () => { | |
| expect(resolveCuratedImageRef(undefined)).toBe(resolveCuratedImageRef('base')) | |
| }) | |
| it('prefers the env-configured ref over the pinned fallback', () => { | |
| process.env.BOXLITE_SYSTEM_PYTHON_IMAGE = 'ghcr.io/boxlite-ai/override@sha256:deadbeef' | |
| expect(resolveCuratedImageRef('python')).toBe('ghcr.io/boxlite-ai/override@sha256:deadbeef') | |
| }) | |
| it('rejects any key outside the allowlist at the boundary (no arbitrary OCI ref)', () => { | |
| expect(() => resolveCuratedImageRef('alpine:3.23')).toThrow(BadRequestError) | |
| expect(() => resolveCuratedImageRef('ghcr.io/evil/image:latest')).toThrow(BadRequestError) | |
| expect(() => resolveCuratedImageRef('ubuntu')).toThrow(BadRequestError) | |
| }) | |
| it('exposes exactly the three curated keys', () => { | |
| expect(CURATED_IMAGE_KEYS).toEqual(['base', 'python', 'node']) | |
| }) | |
| it('resolves each key to its private ghcr ref', () => { | |
| expect(resolveCuratedImageRef('base')).toContain('ghcr.io/boxlite-ai/boxlite-agent-base@sha256:') | |
| expect(resolveCuratedImageRef('python')).toContain('ghcr.io/boxlite-ai/boxlite-agent-python@sha256:') | |
| expect(resolveCuratedImageRef('node')).toContain('ghcr.io/boxlite-ai/boxlite-agent-node@sha256:') | |
| }) | |
| it('defaults to base when no key is supplied', () => { | |
| expect(resolveCuratedImageRef(undefined)).toBe(resolveCuratedImageRef('base')) | |
| }) | |
| it('prefers the env-configured ref over the pinned fallback', () => { | |
| process.env.BOXLITE_SYSTEM_PYTHON_IMAGE = 'ghcr.io/boxlite-ai/override@sha256:deadbeef' | |
| expect(resolveCuratedImageRef('python')).toBe('ghcr.io/boxlite-ai/override@sha256:deadbeef') | |
| }) | |
| it('rejects any key outside the allowlist at the boundary (no arbitrary OCI ref)', () => { | |
| expect(() => resolveCuratedImageRef('alpine:3.23')).toThrow(BadRequestError) | |
| expect(() => resolveCuratedImageRef('ghcr.io/evil/image:latest')).toThrow(BadRequestError) | |
| expect(() => resolveCuratedImageRef('ubuntu')).toThrow(BadRequestError) | |
| }) | |
| it('reverse-maps a resolved ref back to its curated key', () => { | |
| const baseRef = resolveCuratedImageRef('base') | |
| const pythonRef = resolveCuratedImageRef('python') | |
| const nodeRef = resolveCuratedImageRef('node') | |
| expect(curatedImageKeyForRef(baseRef)).toBe('base') | |
| expect(curatedImageKeyForRef(pythonRef)).toBe('python') | |
| expect(curatedImageKeyForRef(nodeRef)).toBe('node') | |
| }) | |
| it('returns undefined for unknown refs', () => { | |
| expect(curatedImageKeyForRef('ghcr.io/unknown/image@sha256:abc123')).toBeUndefined() | |
| expect(curatedImageKeyForRef(undefined)).toBeUndefined() | |
| }) |
🤖 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 `@apps/api/src/box/constants/curated-images.constant.spec.ts` around lines 29 -
52, Add unit tests for the exported curatedImageKeyForRef function in
curated-images.constant.spec.ts: call
resolveCuratedImageRef('base'|'python'|'node') and assert
curatedImageKeyForRef(thatRef) returns 'base'|'python'|'node' respectively;
assert curatedImageKeyForRef('ghcr.io/unknown/image@sha256:deadbeef') returns
undefined; assert curatedImageKeyForRef(undefined) returns undefined; ensure
tests reference CURATED_IMAGE_KEYS, resolveCuratedImageRef and
curatedImageKeyForRef to locate the functions and reset any env overrides (e.g.,
BOXLITE_SYSTEM_PYTHON_IMAGE) to avoid flakiness.
Comment on lines
+81
to
+85
| const runnerAdapter = await this.runnerAdapterFactory.create(runner) | ||
| await runnerAdapter.createBox(box, artifactRef) | ||
|
|
||
| await this.updateBoxState(box, BoxState.CREATING, lockCode) | ||
| return SYNC_AGAIN |
There was a problem hiding this comment.
Add error handling for createBox call.
The runnerAdapter.createBox call at line 82 has no error handling. If it throws an exception (e.g., job creation fails, runner unavailable), the box will remain stuck in UNKNOWN state without transitioning to ERROR. Users will see a hanging box with no clear indication of what went wrong.
🛡️ Proposed fix with error handling
const runnerAdapter = await this.runnerAdapterFactory.create(runner)
- await runnerAdapter.createBox(box, artifactRef)
+ try {
+ await runnerAdapter.createBox(box, artifactRef)
+ } catch (error) {
+ await this.updateBoxState(
+ box,
+ BoxState.ERROR,
+ lockCode,
+ undefined,
+ `Failed to initiate box creation: ${error.message || String(error)}`,
+ )
+ return DONT_SYNC_AGAIN
+ }
await this.updateBoxState(box, BoxState.CREATING, lockCode)🤖 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 `@apps/api/src/box/managers/box-actions/box-start.action.ts` around lines 81 -
85, Wrap the runnerAdapter.createBox call in a try/catch inside the method in
box-start.action (where runnerAdapter is created) so any exception transitions
the box to an error state: on success continue to call this.updateBoxState(box,
BoxState.CREATING, lockCode) and return SYNC_AGAIN; on failure call
this.updateBoxState(box, BoxState.ERROR, lockCode) (include the error message in
logs via processLogger/errorLogger) and return a terminal or retry status as
appropriate. Ensure you reference runnerAdapter.createBox, this.updateBoxState,
BoxState.CREATING and BoxState.ERROR so the catch handles runner
unavailability/job creation failures and prevents the box remaining in UNKNOWN.
| runnerInfo(signal?: AbortSignal): Promise<RunnerInfo> | ||
|
|
||
| boxInfo(boxId: string): Promise<RunnerBoxInfo> | ||
| createBox(box: Box, artifactRef: string): Promise<void> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring for public interface method.
The createBox method is a new public interface method but lacks a docstring explaining its purpose, parameters, and behavior. As per coding guidelines, comprehensive docstrings are required for all public functions and classes.
📝 Suggested docstring
boxInfo(boxId: string): Promise<RunnerBoxInfo>
+ /**
+ * Initiates box creation on the runner using the specified artifact reference.
+ * For V2 runners, enqueues a CREATE_BOX job. For V0 runners, not supported.
+ * `@param` box - The box entity to create
+ * `@param` artifactRef - The OCI image reference (digest-pinned) to use for the box
+ */
createBox(box: Box, artifactRef: string): Promise<void>📝 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.
Suggested change
| createBox(box: Box, artifactRef: string): Promise<void> | |
| /** | |
| * Initiates box creation on the runner using the specified artifact reference. | |
| * For V2 runners, enqueues a CREATE_BOX job. For V0 runners, not supported. | |
| * `@param` box - The box entity to create | |
| * `@param` artifactRef - The OCI image reference (digest-pinned) to use for the box | |
| */ | |
| createBox(box: Box, artifactRef: string): Promise<void> |
🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.ts` at line 49, Add a
descriptive docstring above the public createBox method declaration explaining
its purpose (what creating a Box entails), the parameters (Box — describe
expected fields/shape and artifactRef — what format/meaning), the return type
(Promise<void> and when it resolves/rejects), any side effects (e.g., network
calls, state changes, persisted artifacts), and possible errors/exceptions it
may throw; place this docstring directly above the createBox(signature) in
runnerAdapter.ts (the public interface method named createBox) so callers
understand usage, preconditions, and failure modes.
Source: Coding guidelines
Comment on lines
+117
to
+142
| async createBox(box: Box, artifactRef: string): Promise<void> { | ||
| // Hand-built payload: keys MUST match the Go dto.CreateBoxDTO json tags | ||
| // (apps/runner/pkg/api/dto/box.go). The image ref is passed as `artifactRef` — | ||
| // the runner uses it directly in runtime.Create (NOT `snapshot`). | ||
| const payload = { | ||
| id: box.id, | ||
| boxId: box.boxId, | ||
| userId: box.organizationId, | ||
| artifactRef, | ||
| osUser: box.osUser, | ||
| cpuQuota: box.cpu, | ||
| gpuQuota: box.gpu, | ||
| memoryQuota: box.mem, | ||
| storageQuota: box.disk, | ||
| env: box.env, | ||
| networkBlockAll: box.networkBlockAll, | ||
| networkAllowList: box.networkAllowList, | ||
| authToken: box.authToken, | ||
| organizationId: box.organizationId, | ||
| regionId: box.region, | ||
| } | ||
|
|
||
| await this.jobService.createJob(null, JobType.CREATE_BOX, this.runner.id, ResourceType.BOX, box.id, payload) | ||
|
|
||
| this.logger.debug(`Created CREATE_BOX job for box ${box.id} on runner ${this.runner.id}`) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and verify payload contract.
This method builds a critical payload that must match the Go dto.CreateBoxDTO structure, but it lacks a docstring explaining the contract and parameters. As per coding guidelines, public methods require comprehensive docstrings.
📝 Suggested docstring
+ /**
+ * Enqueues a CREATE_BOX job for the specified box on this runner.
+ * The payload structure must match apps/runner/pkg/api/dto/box.go CreateBoxDTO json tags.
+ * `@param` box - The box entity containing configuration and resource quotas
+ * `@param` artifactRef - The OCI image reference (digest-pinned) for runtime.Create
+ */
async createBox(box: Box, artifactRef: string): Promise<void> {📝 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.
Suggested change
| async createBox(box: Box, artifactRef: string): Promise<void> { | |
| // Hand-built payload: keys MUST match the Go dto.CreateBoxDTO json tags | |
| // (apps/runner/pkg/api/dto/box.go). The image ref is passed as `artifactRef` — | |
| // the runner uses it directly in runtime.Create (NOT `snapshot`). | |
| const payload = { | |
| id: box.id, | |
| boxId: box.boxId, | |
| userId: box.organizationId, | |
| artifactRef, | |
| osUser: box.osUser, | |
| cpuQuota: box.cpu, | |
| gpuQuota: box.gpu, | |
| memoryQuota: box.mem, | |
| storageQuota: box.disk, | |
| env: box.env, | |
| networkBlockAll: box.networkBlockAll, | |
| networkAllowList: box.networkAllowList, | |
| authToken: box.authToken, | |
| organizationId: box.organizationId, | |
| regionId: box.region, | |
| } | |
| await this.jobService.createJob(null, JobType.CREATE_BOX, this.runner.id, ResourceType.BOX, box.id, payload) | |
| this.logger.debug(`Created CREATE_BOX job for box ${box.id} on runner ${this.runner.id}`) | |
| } | |
| /** | |
| * Enqueues a CREATE_BOX job for the specified box on this runner. | |
| * The payload structure must match apps/runner/pkg/api/dto/box.go CreateBoxDTO json tags. | |
| * `@param` box - The box entity containing configuration and resource quotas | |
| * `@param` artifactRef - The OCI image reference (digest-pinned) for runtime.Create | |
| */ | |
| async createBox(box: Box, artifactRef: string): Promise<void> { | |
| // Hand-built payload: keys MUST match the Go dto.CreateBoxDTO json tags | |
| // (apps/runner/pkg/api/dto/box.go). The image ref is passed as `artifactRef` — | |
| // the runner uses it directly in runtime.Create (NOT `snapshot`). | |
| const payload = { | |
| id: box.id, | |
| boxId: box.boxId, | |
| userId: box.organizationId, | |
| artifactRef, | |
| osUser: box.osUser, | |
| cpuQuota: box.cpu, | |
| gpuQuota: box.gpu, | |
| memoryQuota: box.mem, | |
| storageQuota: box.disk, | |
| env: box.env, | |
| networkBlockAll: box.networkBlockAll, | |
| networkAllowList: box.networkAllowList, | |
| authToken: box.authToken, | |
| organizationId: box.organizationId, | |
| regionId: box.region, | |
| } | |
| await this.jobService.createJob(null, JobType.CREATE_BOX, this.runner.id, ResourceType.BOX, box.id, payload) | |
| this.logger.debug(`Created CREATE_BOX job for box ${box.id} on runner ${this.runner.id}`) | |
| } |
🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts` around lines 117 - 142,
Add a clear docstring to the public async createBox(box: Box, artifactRef:
string): Promise<void> method that states the purpose, describes the required
payload contract (must match Go dto.CreateBoxDTO json tags), lists parameters
(box and artifactRef) and their important fields used (id, boxId,
organizationId, osUser, cpu/gpu/mem/disk quotas, env, networkBlockAll,
networkAllowList, authToken, region), and mentions side effects (it enqueues a
CREATE_BOX job via this.jobService.createJob and logs the action); also include
a note to keep this payload in sync with dto.CreateBoxDTO and where to update if
the Go DTO changes (reference dto.CreateBoxDTO in the comment).
Source: Coding guidelines
Comment on lines
+121
to
+137
| const payload = { | ||
| id: box.id, | ||
| boxId: box.boxId, | ||
| userId: box.organizationId, | ||
| artifactRef, | ||
| osUser: box.osUser, | ||
| cpuQuota: box.cpu, | ||
| gpuQuota: box.gpu, | ||
| memoryQuota: box.mem, | ||
| storageQuota: box.disk, | ||
| env: box.env, | ||
| networkBlockAll: box.networkBlockAll, | ||
| networkAllowList: box.networkAllowList, | ||
| authToken: box.authToken, | ||
| organizationId: box.organizationId, | ||
| regionId: box.region, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CreateBoxDTO json tags match the TypeScript payload keys
echo "=== Go CreateBoxDTO json tags ==="
rg -A 1 'type CreateBoxDTO struct' apps/runner/pkg/api/dto/ -A 50 | \
rg 'json:"([^"]+)"' -o --replace '$1' | sort
echo ""
echo "=== TypeScript payload keys (from runnerAdapter.v2.ts) ==="
rg 'const payload = \{' apps/api/src/box/runner-adapter/runnerAdapter.v2.ts -A 20 | \
rg '^\s+(\w+):' -o --replace '$1' | sort
echo ""
echo "Compare the two lists above. Missing or mismatched keys will cause job failures."Repository: boxlite-ai/boxlite
Length of output: 1017
Ensure runnerAdapter v2 payload covers Go CreateBoxDTO required fields (and review optional omissions).
The Go CreateBoxDTO required json tags (artifactRef, cpuQuota, gpuQuota, id, memoryQuota, osUser, storageQuota, userId) are present in the TypeScript payload. However, the TypeScript payload omits several optional DTO keys (entrypoint, fromVolumeId, metadata, otelEndpoint, registry, skipStart, volumes); confirm backend defaults/validation don’t rely on them (or include them if they should be sent).
🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts` around lines 121 - 137,
The payload built in runnerAdapter.v2.ts (the payload object in the
runnerAdapter v2 flow) currently includes all required CreateBoxDTO fields but
omits optional keys (entrypoint, fromVolumeId, metadata, otelEndpoint, registry,
skipStart, volumes); review the Go CreateBoxDTO and either (a) add these
optional properties to the payload with the corresponding box.* or derived
values (e.g., box.entrypoint, box.fromVolumeId, box.metadata, box.otelEndpoint,
box.registry, box.skipStart, box.volumes) so the backend receives explicit
values, or (b) confirm backend defaults/validation allow omission and add
comments referencing CreateBoxDTO to justify each excluded field. Ensure the
payload variable and any serialization code preserve correct naming
(artifactRef, cpuQuota, gpuQuota, id, memoryQuota, osUser, storageQuota, userId)
and type shapes expected by CreateBoxDTO.
Comment on lines
+1527
to
1533
| description: | | ||
| OCI image reference. | ||
| Hosted multi-tenant deployments restrict this to a curated set of | ||
| image keys (`base`, `python`, `node`; default `base`) and reject | ||
| other values with 400. | ||
| default: "alpine:latest" | ||
| example: python:3.11-slim |
There was a problem hiding this comment.
Inconsistent default value for image field.
The description on line 1530 states "default base", but the schema's default on line 1532 is set to "alpine:latest". For consistency with the curated image allowlist described in the comment, the default should be "base".
📝 Proposed fix
description: |
OCI image reference.
Hosted multi-tenant deployments restrict this to a curated set of
image keys (`base`, `python`, `node`; default `base`) and reject
other values with 400.
- default: "alpine:latest"
+ default: "base"
example: python:3.11-slim📝 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.
Suggested change
| description: | | |
| OCI image reference. | |
| Hosted multi-tenant deployments restrict this to a curated set of | |
| image keys (`base`, `python`, `node`; default `base`) and reject | |
| other values with 400. | |
| default: "alpine:latest" | |
| example: python:3.11-slim | |
| description: | | |
| OCI image reference. | |
| Hosted multi-tenant deployments restrict this to a curated set of | |
| image keys (`base`, `python`, `node`; default `base`) and reject | |
| other values with 400. | |
| default: "base" | |
| example: python:3.11-slim |
🤖 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 `@openapi/box.openapi.yaml` around lines 1527 - 1533, The OpenAPI schema's
image field has a mismatched default: the description for the "image" property
says the hosted deployment default is `base` (and curated allowlist includes
`base`, `python`, `node`) but the schema default is set to "alpine:latest";
update the schema default for the "image" property to "base" so it matches the
description and curated allowlist (look for the "image" property/default and
description in the OpenAPI fragment and change default: "alpine:latest" to
default: "base").
Comment on lines
+170
to
+176
| // WithPort forwards a guest port to a host port. A hostPort of 0 lets the | ||
| // runtime assign one dynamically. | ||
| func WithPort(guestPort, hostPort int) BoxOption { | ||
| return func(c *boxConfig) { | ||
| c.ports = append(c.ports, portEntry{guestPort, hostPort}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify the hostPort=0 behavior in the docstring.
The current documentation states that "a hostPort of 0 lets the runtime assign one dynamically," but the actual behavior (per the Rust implementation in context snippet 2 and runtime logic in snippet 4) is that hostPort <= 0 results in a 1:1 port mapping where the host port mirrors the guest port value. This is not dynamic kernel-assigned port allocation; it's a deterministic guest-to-host port mirroring.
Users expecting true dynamic allocation (e.g., OS-assigned ephemeral ports) will be misled.
📝 Suggested docstring correction
-// WithPort forwards a guest port to a host port. A hostPort of 0 lets the
-// runtime assign one dynamically.
+// WithPort forwards a guest port to a host port. A hostPort of 0 creates a
+// 1:1 mapping where the guest port is mirrored on the host.
func WithPort(guestPort, hostPort int) BoxOption {📝 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.
Suggested change
| // WithPort forwards a guest port to a host port. A hostPort of 0 lets the | |
| // runtime assign one dynamically. | |
| func WithPort(guestPort, hostPort int) BoxOption { | |
| return func(c *boxConfig) { | |
| c.ports = append(c.ports, portEntry{guestPort, hostPort}) | |
| } | |
| } | |
| // WithPort forwards a guest port to a host port. A hostPort of 0 creates a | |
| // 1:1 mapping where the guest port is mirrored on the host. | |
| func WithPort(guestPort, hostPort int) BoxOption { | |
| return func(c *boxConfig) { | |
| c.ports = append(c.ports, portEntry{guestPort, hostPort}) | |
| } | |
| } |
🤖 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 `@sdks/go/options.go` around lines 170 - 176, The docstring for WithPort is
misleading — update the comment above the WithPort(guestPort, hostPort int)
function to state that a hostPort <= 0 does NOT request an OS-assigned ephemeral
port but instead results in a 1:1 mapping where the host port mirrors the guest
port value; reference the WithPort function and the boxConfig.ports/portEntry
usage so readers know this deterministic behavior (or, if true dynamic
allocation is desired, change the mapping logic where c.ports is appended to
interpret 0 as a kernel-assigned port and document that behavior instead).
Allow the TypeScript SDK and dashboard to create boxes from curated image keys. Squash pre-launch database migrations into a fresh baseline.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/migrations/pre-deploy/1782000000000-migration.ts (1)
281-370: 💤 Low valueExtension not dropped in
down()— acceptable but worth documenting.The
uuid-osspextension created inup()(line 7) is not dropped indown(). This is a common pattern since shared extensions may be used by other schemas, and theIF NOT EXISTSclause makesup()idempotent. Consider adding a comment near line 7 explaining this is intentional, so future maintainers don't add a corresponding DROP and break other dependents.🤖 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 `@apps/api/src/migrations/pre-deploy/1782000000000-migration.ts` around lines 281 - 370, The migration's up() creates the "uuid-ossp" extension but down() doesn't drop it; add an inline comment in the up() function next to the CREATE EXTENSION "uuid-ossp" statement clarifying this is intentional (extension is shared and may be used by other migrations/schemas and up() uses IF NOT EXISTS), so future maintainers don't attempt to DROP the extension in down(); update the up() method in this migration file to include that explanatory comment near the uuid-ossp extension creation.apps/libs/sdk-typescript/src/BoxLite.ts (1)
430-452: ⚡ Quick winAdd local curated-key allowlist validation before API call.
At Line [430], non-string images are rejected, but at Line [451] any string is still forwarded. Add a small preflight allowlist check (
base|python|node) so invalid keys fail fast with a clear SDK error instead of a remote 400.Suggested patch
+const CURATED_IMAGE_KEYS = new Set(['base', 'python', 'node']) ... - if ('image' in params && typeof params.image !== 'string') { + if ('image' in params && typeof params.image !== 'string') { throw new BoxliteError('Declarative Image objects are no longer supported by the API; pass a curated image key.') } + if ('image' in params && typeof params.image === 'string' && !CURATED_IMAGE_KEYS.has(params.image)) { + throw new BoxliteError(`Unsupported curated image key: ${params.image}. Supported keys: base, python, node.`) + }🤖 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 `@apps/libs/sdk-typescript/src/BoxLite.ts` around lines 430 - 452, The SDK currently only rejects non-string images but still forwards any string to the API; add a local curated-key allowlist check before calling this.boxApi.createBox so invalid keys fail fast. In the method that calls this.boxApi.createBox (where params.image is read), if 'image' in params and typeof params.image === 'string' validate it against the allowlist ["base","python","node"] (allow exact matches or keys prefixed/qualified as your format requires); if it does not match, throw a BoxliteError with a clear message about allowed curated keys. Ensure this check runs before the this.boxApi.createBox invocation so bad keys do not reach the remote API.
🤖 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.
Nitpick comments:
In `@apps/api/src/migrations/pre-deploy/1782000000000-migration.ts`:
- Around line 281-370: The migration's up() creates the "uuid-ossp" extension
but down() doesn't drop it; add an inline comment in the up() function next to
the CREATE EXTENSION "uuid-ossp" statement clarifying this is intentional
(extension is shared and may be used by other migrations/schemas and up() uses
IF NOT EXISTS), so future maintainers don't attempt to DROP the extension in
down(); update the up() method in this migration file to include that
explanatory comment near the uuid-ossp extension creation.
In `@apps/libs/sdk-typescript/src/BoxLite.ts`:
- Around line 430-452: The SDK currently only rejects non-string images but
still forwards any string to the API; add a local curated-key allowlist check
before calling this.boxApi.createBox so invalid keys fail fast. In the method
that calls this.boxApi.createBox (where params.image is read), if 'image' in
params and typeof params.image === 'string' validate it against the allowlist
["base","python","node"] (allow exact matches or keys prefixed/qualified as your
format requires); if it does not match, throw a BoxliteError with a clear
message about allowed curated keys. Ensure this check runs before the
this.boxApi.createBox invocation so bad keys do not reach the remote API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ab746b6-def1-4f31-8b67-f779135b5fd1
📒 Files selected for processing (107)
apps/api/src/migrations/1741087887225-migration.tsapps/api/src/migrations/1741088165704-migration.tsapps/api/src/migrations/1741088883000-migration.tsapps/api/src/migrations/1741088883001-migration.tsapps/api/src/migrations/1741088883002-migration.tsapps/api/src/migrations/1741877019888-migration.tsapps/api/src/migrations/1742215525714-migration.tsapps/api/src/migrations/1742475055353-migration.tsapps/api/src/migrations/1742831092942-migration.tsapps/api/src/migrations/1743593463168-migration.tsapps/api/src/migrations/1743683015304-migration.tsapps/api/src/migrations/1744028841133-migration.tsapps/api/src/migrations/1744114341077-migration.tsapps/api/src/migrations/1744378115901-migration.tsapps/api/src/migrations/1744808444807-migration.tsapps/api/src/migrations/1744868914148-migration.tsapps/api/src/migrations/1744971114480-migration.tsapps/api/src/migrations/1745393243334-migration.tsapps/api/src/migrations/1745494761360-migration.tsapps/api/src/migrations/1745574377029-migration.tsapps/api/src/migrations/1745840296260-migration.tsapps/api/src/migrations/1745864972652-migration.tsapps/api/src/migrations/1746354231722-migration.tsapps/api/src/migrations/1746604150910-migration.tsapps/api/src/migrations/1747658203010-migration.tsapps/api/src/migrations/1748006546552-migration.tsapps/api/src/migrations/1748866194353-migration.tsapps/api/src/migrations/1749474791343-migration.tsapps/api/src/migrations/1749474791344-migration.tsapps/api/src/migrations/1749474791345-migration.tsapps/api/src/migrations/1750077343089-migration.tsapps/api/src/migrations/1750436374899-migration.tsapps/api/src/migrations/1750668569562-migration.tsapps/api/src/migrations/1750751712412-migration.tsapps/api/src/migrations/1751456907334-migration.tsapps/api/src/migrations/1752494676200-migration.tsapps/api/src/migrations/1752494676205-migration.tsapps/api/src/migrations/1752848014862-migration.tsapps/api/src/migrations/1753099115783-migration.tsapps/api/src/migrations/1753100751730-migration.tsapps/api/src/migrations/1753100751731-migration.tsapps/api/src/migrations/1753185133351-migration.tsapps/api/src/migrations/1753274135567-migration.tsapps/api/src/migrations/1753430929609-migration.tsapps/api/src/migrations/1753717830378-migration.tsapps/api/src/migrations/1754042247109-migration.tsapps/api/src/migrations/1755003696741-migration.tsapps/api/src/migrations/1755356869493-migration.tsapps/api/src/migrations/1755464957487-migration.tsapps/api/src/migrations/1755521645207-migration.tsapps/api/src/migrations/1755860619921-migration.tsapps/api/src/migrations/1757513754037-migration.tsapps/api/src/migrations/1757513754038-migration.tsapps/api/src/migrations/1759241690773-migration.tsapps/api/src/migrations/1759768058397-migration.tsapps/api/src/migrations/1761912147638-migration.tsapps/api/src/migrations/1761912147639-migration.tsapps/api/src/migrations/1763561822000-migration.tsapps/api/src/migrations/1764073472179-migration.tsapps/api/src/migrations/1764073472180-migration.tsapps/api/src/migrations/1764844895057-migration.tsapps/api/src/migrations/1764844895058-migration.tsapps/api/src/migrations/1765282546000-migration.tsapps/api/src/migrations/1765366773736-migration.tsapps/api/src/migrations/1765400000000-migration.tsapps/api/src/migrations/1765806205881-migration.tsapps/api/src/migrations/1766415256696-migration.tsapps/api/src/migrations/1767830400000-migration.tsapps/api/src/migrations/1768306129179-migration.tsapps/api/src/migrations/1768461678804-migration.tsapps/api/src/migrations/1768475454675-migration.tsapps/api/src/migrations/1768485728153-migration.tsapps/api/src/migrations/1768583941244-migration.tsapps/api/src/migrations/1769516172576-migration.tsapps/api/src/migrations/1769516172577-migration.tsapps/api/src/migrations/1770043707083-migration.tsapps/api/src/migrations/1770212429837-migration.tsapps/api/src/migrations/1770823569571-migration.tsapps/api/src/migrations/1770880371265-migration.tsapps/api/src/migrations/README.mdapps/api/src/migrations/default-organization-membership.migration.spec.tsapps/api/src/migrations/post-deploy/1770880371266-migration.tsapps/api/src/migrations/post-deploy/1774438866002-migration.tsapps/api/src/migrations/post-deploy/1774454800508-migration.tsapps/api/src/migrations/post-deploy/1780200000000-migration.tsapps/api/src/migrations/post-deploy/1780531200000-migration.tsapps/api/src/migrations/post-deploy/1780999000000-migration.tsapps/api/src/migrations/post-deploy/1781000000000-migration.tsapps/api/src/migrations/post-deploy/1781016743403-migration.tsapps/api/src/migrations/post-deploy/1781072797240-migration.tsapps/api/src/migrations/post-deploy/1781100000000-migration.tsapps/api/src/migrations/post-deploy/1781200000000-migration.tsapps/api/src/migrations/pre-deploy/1770900000000-migration.tsapps/api/src/migrations/pre-deploy/1773744656413-migration.tsapps/api/src/migrations/pre-deploy/1773916204375-migration.tsapps/api/src/migrations/pre-deploy/1774438866001-migration.tsapps/api/src/migrations/pre-deploy/1774454790153-migration.tsapps/api/src/migrations/pre-deploy/1780600000000-migration.tsapps/api/src/migrations/pre-deploy/1780912800000-migration.tsapps/api/src/migrations/pre-deploy/1782000000000-migration.tsapps/dashboard/src/components/Box/CreateBoxSheet.tsxapps/dashboard/src/components/Playground/Box/CodeSnippets/python.tsapps/dashboard/src/components/Playground/Box/CodeSnippets/typescript.tsapps/dashboard/src/lib/onboarding-code-examples.tsapps/dashboard/src/providers/PlaygroundProvider.tsxapps/libs/sdk-typescript/src/BoxLite.tsapps/libs/sdk-typescript/src/__tests__/BoxLite.create-guards.test.ts
💤 Files with no reviewable changes (98)
- apps/api/src/migrations/1763561822000-migration.ts
- apps/api/src/migrations/1748866194353-migration.ts
- apps/api/src/migrations/1755003696741-migration.ts
- apps/api/src/migrations/1741088883001-migration.ts
- apps/api/src/migrations/1747658203010-migration.ts
- apps/api/src/migrations/post-deploy/1780531200000-migration.ts
- apps/api/src/migrations/1759241690773-migration.ts
- apps/api/src/migrations/1753274135567-migration.ts
- apps/api/src/migrations/1753430929609-migration.ts
- apps/api/src/migrations/1754042247109-migration.ts
- apps/api/src/migrations/1749474791343-migration.ts
- apps/api/src/migrations/1770043707083-migration.ts
- apps/api/src/migrations/1741087887225-migration.ts
- apps/api/src/migrations/1753100751730-migration.ts
- apps/api/src/migrations/1755521645207-migration.ts
- apps/api/src/migrations/1746604150910-migration.ts
- apps/api/src/migrations/1742475055353-migration.ts
- apps/api/src/migrations/1745494761360-migration.ts
- apps/api/src/migrations/1761912147639-migration.ts
- apps/api/src/migrations/pre-deploy/1773916204375-migration.ts
- apps/api/src/migrations/1770880371265-migration.ts
- apps/api/src/migrations/1764844895058-migration.ts
- apps/api/src/migrations/1741088165704-migration.ts
- apps/api/src/migrations/pre-deploy/1780600000000-migration.ts
- apps/api/src/migrations/1744114341077-migration.ts
- apps/api/src/migrations/1761912147638-migration.ts
- apps/api/src/migrations/1750436374899-migration.ts
- apps/api/src/migrations/1757513754037-migration.ts
- apps/api/src/migrations/1770212429837-migration.ts
- apps/api/src/migrations/1753099115783-migration.ts
- apps/api/src/migrations/1766415256696-migration.ts
- apps/api/src/migrations/post-deploy/1781000000000-migration.ts
- apps/api/src/migrations/1741877019888-migration.ts
- apps/api/src/migrations/1750668569562-migration.ts
- apps/api/src/migrations/1755464957487-migration.ts
- apps/api/src/migrations/1753185133351-migration.ts
- apps/api/src/migrations/1752494676205-migration.ts
- apps/api/src/migrations/1764073472179-migration.ts
- apps/api/src/migrations/1749474791344-migration.ts
- apps/api/src/migrations/1757513754038-migration.ts
- apps/api/src/migrations/1748006546552-migration.ts
- apps/api/src/migrations/1741088883002-migration.ts
- apps/api/src/migrations/1746354231722-migration.ts
- apps/api/src/migrations/1753717830378-migration.ts
- apps/api/src/migrations/pre-deploy/1774438866001-migration.ts
- apps/api/src/migrations/1759768058397-migration.ts
- apps/api/src/migrations/1744868914148-migration.ts
- apps/api/src/migrations/1745574377029-migration.ts
- apps/api/src/migrations/1744971114480-migration.ts
- apps/api/src/migrations/1765400000000-migration.ts
- apps/api/src/migrations/1741088883000-migration.ts
- apps/api/src/migrations/1755356869493-migration.ts
- apps/api/src/migrations/1764844895057-migration.ts
- apps/api/src/migrations/pre-deploy/1780912800000-migration.ts
- apps/api/src/migrations/post-deploy/1774454800508-migration.ts
- apps/api/src/migrations/1765366773736-migration.ts
- apps/api/src/migrations/1769516172577-migration.ts
- apps/api/src/migrations/post-deploy/1770880371266-migration.ts
- apps/api/src/migrations/1752848014862-migration.ts
- apps/api/src/migrations/1770823569571-migration.ts
- apps/api/src/migrations/1745840296260-migration.ts
- apps/api/src/migrations/1768583941244-migration.ts
- apps/api/src/migrations/1750077343089-migration.ts
- apps/api/src/migrations/post-deploy/1774438866002-migration.ts
- apps/api/src/migrations/1751456907334-migration.ts
- apps/api/src/migrations/1764073472180-migration.ts
- apps/api/src/migrations/post-deploy/1780200000000-migration.ts
- apps/api/src/migrations/1743683015304-migration.ts
- apps/api/src/migrations/1749474791345-migration.ts
- apps/api/src/migrations/1750751712412-migration.ts
- apps/api/src/migrations/1768485728153-migration.ts
- apps/api/src/migrations/post-deploy/1780999000000-migration.ts
- apps/api/src/migrations/1744808444807-migration.ts
- apps/api/src/migrations/1745864972652-migration.ts
- apps/api/src/migrations/pre-deploy/1773744656413-migration.ts
- apps/api/src/migrations/1765282546000-migration.ts
- apps/api/src/migrations/1742831092942-migration.ts
- apps/api/src/migrations/pre-deploy/1774454790153-migration.ts
- apps/api/src/migrations/1755860619921-migration.ts
- apps/api/src/migrations/1744028841133-migration.ts
- apps/api/src/migrations/1767830400000-migration.ts
- apps/api/src/migrations/post-deploy/1781200000000-migration.ts
- apps/api/src/migrations/1768306129179-migration.ts
- apps/api/src/migrations/1769516172576-migration.ts
- apps/api/src/migrations/default-organization-membership.migration.spec.ts
- apps/api/src/migrations/1753100751731-migration.ts
- apps/api/src/migrations/1768461678804-migration.ts
- apps/api/src/migrations/1768475454675-migration.ts
- apps/api/src/migrations/pre-deploy/1770900000000-migration.ts
- apps/api/src/migrations/post-deploy/1781016743403-migration.ts
- apps/api/src/migrations/1745393243334-migration.ts
- apps/api/src/migrations/post-deploy/1781100000000-migration.ts
- apps/api/src/migrations/post-deploy/1781072797240-migration.ts
- apps/api/src/migrations/1743593463168-migration.ts
- apps/api/src/migrations/1752494676200-migration.ts
- apps/api/src/migrations/1765806205881-migration.ts
- apps/api/src/migrations/1744378115901-migration.ts
- apps/api/src/migrations/1742215525714-migration.ts
✅ Files skipped from review due to trivial changes (2)
- apps/api/src/migrations/README.md
- apps/dashboard/src/lib/onboarding-code-examples.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if not _is_transient_remove_error(exc) or attempt == 8: | ||
| raise | ||
| await asyncio.sleep(min(0.5 * attempt, 2.0)) | ||
| raise last_error |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/build-runner-binary.yml (2)
114-121: ⚡ Quick winAdd defensive validation to match the 'build' path.
The 'source' path lacks the explicit validation checks present in the 'build' path (lines 106, 109). Adding similar checks would provide clearer error messages and maintain consistency.
♻️ Proposed improvement for consistency and clarity
- name: Build libboxlite.a from current source if: inputs.libboxlite_source == 'source' run: | + set -euo pipefail make setup:build runtime dist:go - mkdir -p sdks/go + [ -f target/release/libboxlite.a ] || { echo "::error::libboxlite.a not found after build"; exit 1; } + mkdir -p sdks/go cp target/release/libboxlite.a sdks/go/libboxlite.a ls -lh sdks/go/libboxlite.a🤖 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 @.github/workflows/build-runner-binary.yml around lines 114 - 121, Add the same defensive validation used in the 'build' branch to the 'source' branch: after running the build commands in the "Build libboxlite.a from current source" step (when inputs.libboxlite_source == 'source'), verify the expected artifact exists and exit with a clear error if not; reference the step name and artifact path (sdks/go/libboxlite.a and target/release/libboxlite.a) when adding the checks so the step mirrors the checks present in the earlier "build" path.
82-93: 💤 Low valueConsider adding validation for downloaded artifact.
While
curl -fwill fail the step if the download fails, adding explicit validation would provide clearer error messages, similar to the defensive checks in the 'build' path (lines 106, 109).♻️ Proposed improvement for clearer error messages
- name: Download prebuilt libboxlite.a if: inputs.libboxlite_source == '' || inputs.libboxlite_source == 'release' env: VERSION: ${{ steps.version.outputs.version }} GH_REPO: ${{ github.repository }} run: | + set -euo pipefail ARCHIVE="boxlite-c-v${VERSION}-linux-x64-gnu.tar.gz" URL="https://github.com/${GH_REPO}/releases/download/v${VERSION}/${ARCHIVE}" echo "Downloading $URL" curl -fsSL "${URL}" | tar xz -C /tmp/ + [ -f "/tmp/boxlite-c-v${VERSION}-linux-x64-gnu/lib/libboxlite.a" ] || { echo "::error::libboxlite.a not found after extraction"; exit 1; } cp "/tmp/boxlite-c-v${VERSION}-linux-x64-gnu/lib/libboxlite.a" sdks/go/libboxlite.a + ls -lh sdks/go/libboxlite.a🤖 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 @.github/workflows/build-runner-binary.yml around lines 82 - 93, In the "Download prebuilt libboxlite.a" step ensure the downloaded artifact is validated after extraction: after running curl | tar, check that "/tmp/boxlite-c-v${VERSION}-linux-x64-gnu/lib/libboxlite.a" exists and is readable (e.g. test -f) before copying to sdks/go/libboxlite.a; if the file is missing, print a clear error including the attempted URL/ARCHIVE and exit non‑zero so the workflow fails with an informative message.
🤖 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 @.github/workflows/build-runner-binary.yml:
- Around line 23-30: Update the workflow_dispatch inputs for libboxlite_source
to match workflow_call: add the missing 'build' option to the options list for
libboxlite_source and change its default from 'source' to 'release' so both
triggers use the same default behavior; modify the inputs block that defines
libboxlite_source (the inputs: libboxlite_source, description, type, default,
options entries) to include 'build' and set default='release' to align with
workflow_call and the runtime logic that checks libboxlite_source.
- Line 78: Replace the unpinned action reference uses:
actions-rust-lang/setup-rust-toolchain@v1 with a pinned commit SHA for
supply-chain security; locate the line containing uses:
actions-rust-lang/setup-rust-toolchain@v1 and update it to use the specific
commit hash for the v1 tag (i.e., uses:
actions-rust-lang/setup-rust-toolchain@<COMMIT_SHA>), ensuring you fetch and
insert the correct SHA from the upstream repository for the v1 tag.
- Line 96: Replace the loose action reference actions/download-artifact@v4 with
a pinned commit SHA to mitigate supply-chain risk: update the uses line to
actions/download-artifact@<commit-sha> (fetch the exact v4 tag commit hash from
the actions/download-artifact repo) so the workflow uses the specific immutable
commit instead of the floating tag.
- Around line 31-37: The workflow accepts workflow_call.inputs.libboxlite_source
as a free-form string which can silently skip all conditionals and leave
sdks/go/libboxlite.a missing; add an explicit validation step (after the
checkout step) that reads the libboxlite_source input and fails fast unless it
equals one of the allowed values ("release", "build", "source"), emitting a
clear error message and exiting non‑zero so the job fails early; place this
validation as a separate step named e.g. "Validate libboxlite_source" and
reference workflow_call.inputs.libboxlite_source in the step (and still preserve
the existing conditional steps that check that variable to produce
sdks/go/libboxlite.a).
---
Nitpick comments:
In @.github/workflows/build-runner-binary.yml:
- Around line 114-121: Add the same defensive validation used in the 'build'
branch to the 'source' branch: after running the build commands in the "Build
libboxlite.a from current source" step (when inputs.libboxlite_source ==
'source'), verify the expected artifact exists and exit with a clear error if
not; reference the step name and artifact path (sdks/go/libboxlite.a and
target/release/libboxlite.a) when adding the checks so the step mirrors the
checks present in the earlier "build" path.
- Around line 82-93: In the "Download prebuilt libboxlite.a" step ensure the
downloaded artifact is validated after extraction: after running curl | tar,
check that "/tmp/boxlite-c-v${VERSION}-linux-x64-gnu/lib/libboxlite.a" exists
and is readable (e.g. test -f) before copying to sdks/go/libboxlite.a; if the
file is missing, print a clear error including the attempted URL/ARCHIVE and
exit non‑zero so the workflow fails with an informative message.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e31577f5-0d53-45a7-9fa7-fd498e954ee4
📒 Files selected for processing (7)
.github/workflows/build-runner-binary.yml.github/workflows/e2e-test.ymlscripts/test/e2e/cases/conftest.pyscripts/test/e2e/cases/test_box_management.pyscripts/test/e2e/cases/test_path_verification.pyscripts/test/e2e/cases/test_quota_enforcement.pyscripts/test/e2e/fixture_setup.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/e2e-test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test/e2e/fixture_setup.py
Comment on lines
+23
to
+30
| inputs: | ||
| libboxlite_source: | ||
| description: 'libboxlite.a source for manual runs' | ||
| type: choice | ||
| default: source | ||
| options: | ||
| - source | ||
| - release |
There was a problem hiding this comment.
Add missing 'build' option and align default with workflow_call.
The workflow_dispatch input is missing the build option that is documented in the workflow_call description (line 34) and implemented in the workflow logic (lines 94-113). This prevents manual testing of the artifact-based build path. Additionally, the default differs between triggers: workflow_dispatch defaults to source while workflow_call defaults to release, creating inconsistent behavior.
✅ Proposed fix to add 'build' option and align defaults
workflow_dispatch:
inputs:
libboxlite_source:
description: 'libboxlite.a source for manual runs'
type: choice
- default: source
+ default: release
options:
- source
- release
+ - build📝 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.
Suggested change
| inputs: | |
| libboxlite_source: | |
| description: 'libboxlite.a source for manual runs' | |
| type: choice | |
| default: source | |
| options: | |
| - source | |
| - release | |
| inputs: | |
| libboxlite_source: | |
| description: 'libboxlite.a source for manual runs' | |
| type: choice | |
| default: release | |
| options: | |
| - source | |
| - release | |
| - build |
🤖 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 @.github/workflows/build-runner-binary.yml around lines 23 - 30, Update the
workflow_dispatch inputs for libboxlite_source to match workflow_call: add the
missing 'build' option to the options list for libboxlite_source and change its
default from 'source' to 'release' so both triggers use the same default
behavior; modify the inputs block that defines libboxlite_source (the inputs:
libboxlite_source, description, type, default, options entries) to include
'build' and set default='release' to align with workflow_call and the runtime
logic that checks libboxlite_source.
Comment on lines
+31
to
+37
| workflow_call: | ||
| inputs: | ||
| libboxlite_source: | ||
| description: 'How to obtain libboxlite.a: release, build, or source' | ||
| type: string | ||
| default: release | ||
| required: false |
There was a problem hiding this comment.
Validate libboxlite_source input to prevent silent failures.
The workflow_call input uses type: string without validation, allowing invalid values like 'foo' or 'typo'. When an invalid value is passed, all conditional steps (lines 77, 83, 95, 102, 115) will be skipped, causing sdks/go/libboxlite.a to be missing and the runner build to fail with an unclear error message at link time.
🛡️ Proposed fix to add early validation
Add a validation step after checkout:
steps:
- uses: actions/checkout@v4
+ - name: Validate libboxlite_source input
+ run: |
+ VALID_VALUES="release build source"
+ VALUE="${{ inputs.libboxlite_source }}"
+ if [ -n "$VALUE" ] && ! echo "$VALID_VALUES" | grep -qw "$VALUE"; then
+ echo "::error::Invalid libboxlite_source='$VALUE'. Must be one of: $VALID_VALUES"
+ exit 1
+ fi
+
- name: Set up Go
uses: actions/setup-go@v5🧰 Tools
🪛 zizmor (1.25.2)
[error] 18-37: use of fundamentally insecure workflow trigger (dangerous-triggers): workflow_run is almost always used insecurely
(dangerous-triggers)
🤖 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 @.github/workflows/build-runner-binary.yml around lines 31 - 37, The workflow
accepts workflow_call.inputs.libboxlite_source as a free-form string which can
silently skip all conditionals and leave sdks/go/libboxlite.a missing; add an
explicit validation step (after the checkout step) that reads the
libboxlite_source input and fails fast unless it equals one of the allowed
values ("release", "build", "source"), emitting a clear error message and
exiting non‑zero so the job fails early; place this validation as a separate
step named e.g. "Validate libboxlite_source" and reference
workflow_call.inputs.libboxlite_source in the step (and still preserve the
existing conditional steps that check that variable to produce
sdks/go/libboxlite.a).
|
|
||
| - name: Set up Rust | ||
| if: inputs.libboxlite_source == 'source' | ||
| uses: actions-rust-lang/setup-rust-toolchain@v1 |
There was a problem hiding this comment.
Pin action to commit SHA for supply chain security.
The action reference actions-rust-lang/setup-rust-toolchain@v1 is not pinned to a commit hash, creating a supply chain security risk if the tag is moved or the action is compromised.
🔒 Proposed fix to pin the action
- name: Set up Rust
if: inputs.libboxlite_source == 'source'
- uses: actions-rust-lang/setup-rust-toolchain@v1
+ uses: actions-rust-lang/setup-rust-toolchain@b113a30d27a8e59c969077c0a0168cc13dab5ffc # v1
with:
toolchain: ${{ needs.config.outputs.rust-toolchain }}Note: Replace the SHA with the actual commit hash for the v1 tag from the upstream repository.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 78-78: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/build-runner-binary.yml at line 78, Replace the unpinned
action reference uses: actions-rust-lang/setup-rust-toolchain@v1 with a pinned
commit SHA for supply-chain security; locate the line containing uses:
actions-rust-lang/setup-rust-toolchain@v1 and update it to use the specific
commit hash for the v1 tag (i.e., uses:
actions-rust-lang/setup-rust-toolchain@<COMMIT_SHA>), ensuring you fetch and
insert the correct SHA from the upstream repository for the v1 tag.
Source: Linters/SAST tools
|
|
||
| - name: Download libboxlite.a from sibling Build C SDK job | ||
| if: inputs.libboxlite_source == 'build' | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
Pin action to commit SHA for supply chain security.
The action reference actions/download-artifact@v4 is not pinned to a commit hash, creating a supply chain security risk.
🔒 Proposed fix to pin the action
- name: Download libboxlite.a from sibling Build C SDK job
if: inputs.libboxlite_source == 'build'
- uses: actions/download-artifact@v4
+ uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4
with:
name: c-sdk-linux-x64-gnu
path: /tmp/c-sdk/Note: Replace the SHA with the actual commit hash for the v4 tag from the actions/download-artifact repository.
📝 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.
Suggested change
| uses: actions/download-artifact@v4 | |
| - name: Download libboxlite.a from sibling Build C SDK job | |
| if: inputs.libboxlite_source == 'build' | |
| uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 | |
| with: | |
| name: c-sdk-linux-x64-gnu | |
| path: /tmp/c-sdk/ |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 96-96: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/build-runner-binary.yml at line 96, Replace the loose
action reference actions/download-artifact@v4 with a pinned commit SHA to
mitigate supply-chain risk: update the uses line to
actions/download-artifact@<commit-sha> (fetch the exact v4 tag commit hash from
the actions/download-artifact repo) so the workflow uses the specific immutable
commit instead of the floating tag.
Source: Linters/SAST tools
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+81
to
+146
| name: Detect relevant changes | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| # Each downstream job/step gates on the narrow component that | ||
| # would actually be invalidated by its source paths, instead of | ||
| # one giant `relevant` flag — saves the ~11 min libboxlite + | ||
| # runner build chain when a PR only touches API or SDK code. | ||
| api: ${{ steps.filter.outputs.api }} | ||
| runner_chain: ${{ steps.filter.outputs.runner_chain }} | ||
| sdk_py: ${{ steps.filter.outputs.sdk_py }} | ||
| tests_or_workflow: ${{ steps.filter.outputs.tests_or_workflow }} | ||
| any: ${{ steps.filter.outputs.api == 'true' || steps.filter.outputs.runner_chain == 'true' || steps.filter.outputs.sdk_py == 'true' || steps.filter.outputs.tests_or_workflow == 'true' }} | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - uses: dorny/paths-filter@v3 | ||
| id: filter | ||
| with: | ||
| filters: | | ||
| # API container — apps/api Dockerfile.source bakes from these. | ||
| api: | ||
| - 'apps/api/**' | ||
| - 'apps/dashboard/**' | ||
| - 'apps/libs/**' | ||
| # Runner binary build chain. libboxlite.a (Rust src/**) is | ||
| # statically linked into the runner via CGo, so any Rust src | ||
| # change OR any Go runner-side change invalidates the runner | ||
| # binary. Touching scripts/build/** also invalidates the | ||
| # libboxlite build chain output. | ||
| runner_chain: | ||
| - 'apps/runner/**' | ||
| - 'apps/daemon/**' | ||
| - 'apps/common-go/**' | ||
| - 'apps/api-client-go/**' | ||
| - 'apps/libs/computer-use/**' | ||
| - 'sdks/go/**' | ||
| - 'src/boxlite/**' | ||
| - 'src/api-client/**' | ||
| - 'src/shared/**' | ||
| - 'src/deps/**' | ||
| - 'scripts/build/**' | ||
| - 'sdks/c/src/exec/**' | ||
| # Python SDK. PyO3 wheel pulls libboxlite.a sources too via | ||
| # boxlite-c crate, so Rust src changes also invalidate the | ||
| # SDK. (No separate "sdk needs libboxlite rebuild" output — | ||
| # build_c_sdk gates on either runner_chain or sdk_py.) | ||
| sdk_py: | ||
| - 'sdks/python/**' | ||
| # Test code + workflow self-modifications. Doesn't trigger | ||
| # any rebuild — just runs the existing deployed stack. | ||
| tests_or_workflow: | ||
| - 'scripts/test/e2e/**' | ||
| - '.github/workflows/e2e-cloud.yml' | ||
| - '.github/workflows/build-runner-binary.yml' | ||
| - '.github/workflows/build-c.yml' | ||
|
|
||
| # Build libboxlite.a from THIS PR's Rust source using the existing | ||
| # build-c.yml workflow (manylinux container, libseccomp.a cross-build, | ||
| # fix-go-symbols.sh — same chain release builds use). target_filter | ||
| # constrains the matrix to just linux-x64-gnu (skip macos-15 + | ||
| # ubuntu-24.04-arm matrix entries; Tokyo runner is amd64). | ||
| # Uploads artifact c-sdk-linux-x64-gnu consumed by build_runner. | ||
| # | ||
| # Fires only when libboxlite source OR runner Go OR Python SDK | ||
| # changed — pure API / dashboard / test-code PRs skip the ~9 min | ||
| # libboxlite compile entirely. | ||
| build_c_sdk: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/tests/run.rs (1)
6-13: ⚡ Quick winConsider extracting the duplicated helper to the common test module.
The
reserve_host_port()helper is identical in bothcreate.rsandrun.rs. Extracting it to thecommonmodule would eliminate duplication and make the pattern available to other test files.♻️ Suggested refactor
Move the helper to
src/cli/tests/common/mod.rs(or a dedicatedsrc/cli/tests/common/ports.rs):+pub fn reserve_host_port() -> (std::net::TcpListener, u16) { + let listener = std::net::TcpListener::bind("127.0.0.1:0") + .expect("bind ephemeral host port"); + let port = listener + .local_addr() + .expect("read ephemeral host port") + .port(); + (listener, port) +}Then import it in both test files:
-fn reserve_host_port() -> (TcpListener, u16) { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind ephemeral host port"); - let port = listener - .local_addr() - .expect("read ephemeral host port") - .port(); - (listener, port) -} +use common::reserve_host_port;🤖 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 `@src/cli/tests/run.rs` around lines 6 - 13, The helper function reserve_host_port() is duplicated across tests; extract this function into the shared test common module (e.g., common::reserve_host_port) and remove the duplicate from both create.rs and run.rs; update each test file to import and call common::reserve_host_port (or pub use it from common::ports) so the single implementation is reused by all CLI tests.
🤖 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.
Nitpick comments:
In `@src/cli/tests/run.rs`:
- Around line 6-13: The helper function reserve_host_port() is duplicated across
tests; extract this function into the shared test common module (e.g.,
common::reserve_host_port) and remove the duplicate from both create.rs and
run.rs; update each test file to import and call common::reserve_host_port (or
pub use it from common::ports) so the single implementation is reused by all CLI
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9dc3dad-9b54-4879-822e-44600e2befee
📒 Files selected for processing (4)
src/boxlite/tests/health_check.rssrc/boxlite/tests/jailer.rssrc/cli/tests/create.rssrc/cli/tests/run.rs
DorianZheng
reviewed
Jun 11, 2026
| - total | ||
| - totalPages | ||
| type: object | ||
| CreateBox: |
Member
There was a problem hiding this comment.
Please remove these, use box.openapi.yaml only
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/e2e-cloud.yml (1)
149-152: ⚡ Quick win
sdk_pycurrently pays for a C artifact that nothing consumes.On Python-SDK-only PRs,
build_c_sdkstill runs, butbuild_runnerstays skipped and thee2ejob never downloadsc-sdk-linux-x64-gnu. That adds the expensive libboxlite build back into the exact path this workflow is trying to keep cheap.♻️ Suggested change
if: | needs.changes.outputs.runner_chain == 'true' - || needs.changes.outputs.sdk_py == 'true' || github.event_name == 'workflow_dispatch'🤖 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 @.github/workflows/e2e-cloud.yml around lines 149 - 152, The workflow currently triggers build_c_sdk whenever needs.changes.outputs.sdk_py == 'true', causing the expensive C artifact to be built even when build_runner (the only consumer) is skipped; remove sdk_py from the condition that gates the C build so build_c_sdk runs only when runner_chain is true or on manual dispatch. Update the job condition that controls the build_c_sdk job (referencing the needs.changes.outputs.sdk_py and needs.changes.outputs.runner_chain checks) to drop sdk_py, ensuring only needs.changes.outputs.runner_chain == 'true' || github.event_name == 'workflow_dispatch' will trigger building the C SDK so e2e can download c-sdk-linux-x64-gnu only when required.
🤖 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 @.github/workflows/build-c.yml:
- Around line 28-34: The reusable C build workflow
(.github/workflows/build-c.yml) currently includes the upload-to-release job
which requires permissions: contents: write and forces callers (e.g., the
build_c_sdk invocation in .github/workflows/e2e-cloud.yml) to request write
perms; to fix, extract the upload-to-release job into a separate release-only
workflow (or reusable workflow) that is triggered only for release events and
retains permissions: contents: write, then remove the upload-to-release job and
any contents: write permission from build-c.yml so the reusable build path
remains read-only; update callers like the build_c_sdk workflow to call the new
release workflow when performing releases (or call the upload workflow directly
from release workflows) so PR workflow_call invocations no longer require
expanded permissions.
In @.github/workflows/e2e-cloud.yml:
- Around line 347-350: The request/done keys collide across reruns because they
only use GITHUB_SHA; update the key construction to include a per-run unique
identifier (e.g., GITHUB_RUN_ID or GITHUB_RUN_NUMBER) so the files are unique to
each workflow run; change BIN_KEY/REQ_KEY/DONE_KEY (and any other places where
PREFIX is reused in the same workflow block) to incorporate the chosen run-id
variable (for example: "${PREFIX}/request-${GITHUB_SHA}-${GITHUB_RUN_ID}.txt"
and similarly for DONE_KEY and BIN_KEY) and ensure all other occurrences in the
same region (the repeated block at the later range) are updated consistently.
- Around line 94-95: Replace mutable action tags with pinned full commit SHAs
for all reusable actions used in the workflow: change uses: actions/checkout@v5,
uses: dorny/paths-filter@v3, uses: aws-actions/configure-aws-credentials@v4,
uses: aws-actions/amazon-ecr-login@v2, uses: actions/download-artifact@v4, and
uses: actions/upload-artifact@v4 to their corresponding commit SHA references
(e.g., actions/checkout@<sha>); locate the lines that contain those exact uses:
strings and update each to the canonical commit SHA for the released version you
want to pin, commit the updated workflow, and verify the workflow still runs as
expected.
- Line 94: The checkout steps using actions/checkout@v5 are leaving GITHUB_TOKEN
in the local git config; update both occurrences of actions/checkout@v5 in the
workflow to include the input persist-credentials: false (i.e., add
persist-credentials: false under the step that uses actions/checkout@v5) so the
checkouts do not persist credentials into repo-controlled builds and tests.
---
Nitpick comments:
In @.github/workflows/e2e-cloud.yml:
- Around line 149-152: The workflow currently triggers build_c_sdk whenever
needs.changes.outputs.sdk_py == 'true', causing the expensive C artifact to be
built even when build_runner (the only consumer) is skipped; remove sdk_py from
the condition that gates the C build so build_c_sdk runs only when runner_chain
is true or on manual dispatch. Update the job condition that controls the
build_c_sdk job (referencing the needs.changes.outputs.sdk_py and
needs.changes.outputs.runner_chain checks) to drop sdk_py, ensuring only
needs.changes.outputs.runner_chain == 'true' || github.event_name ==
'workflow_dispatch' will trigger building the C SDK so e2e can download
c-sdk-linux-x64-gnu only when required.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f04ac953-e732-4631-be11-18ca99c18ddc
📒 Files selected for processing (2)
.github/workflows/build-c.yml.github/workflows/e2e-cloud.yml
Comment on lines
+28
to
+34
| workflow_call: | ||
| inputs: | ||
| target_filter: | ||
| description: 'Optional. Build only the given target (e.g. "linux-x64-gnu"). Empty = build all matrix platforms.' | ||
| type: string | ||
| default: '' | ||
| required: false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workflows
ls -la .github/workflows || true
# Show the relevant sections of build-c.yml
echo "----- build-c.yml (workflow_call + permissions/upload-to-release) -----"
python3 - <<'PY'
import pathlib, re
p = pathlib.Path(".github/workflows/build-c.yml")
print("exists:", p.exists())
if not p.exists():
raise SystemExit(1)
text = p.read_text()
# Print a small window around workflow_call, if present
m = re.search(r'^\s*workflow_call:\s*$', text, re.M)
if m:
start = max(0, text.rfind('\n', 0, m.start()-1))
# Print the whole workflow_call input section (around lines 20-60-ish)
lines = text.splitlines()
for i, line in enumerate(lines, start=1):
if "workflow_call:" in line:
a=i-5; b=i+80
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
print("\n--- upload-to-release job section ---")
# Print permissions and job definition for upload-to-release
for i, line in enumerate(lines, start=1):
if re.search(r'^\s*upload-to-release\s*:\s*$', line):
a=i-10; b=i+120
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
else:
# try search by job id substring
for i, line in enumerate(lines, start=1):
if "upload-to-release" in line:
a=i-5; b=i+80
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
print("\n--- top-level permissions section (if any) ---")
for i, line in enumerate(lines, start=1):
if re.match(r'^\s*permissions:\s*$', line):
a=i-3; b=i+10
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
PY
# Show the relevant parts of e2e-cloud.yml where build-c.yml is called
echo "----- e2e-cloud.yml (workflow_call to build-c.yml) -----"
python3 - <<'PY'
import pathlib, re
p = pathlib.Path(".github/workflows/e2e-cloud.yml")
print("exists:", p.exists())
if not p.exists():
raise SystemExit(1)
text = p.read_text()
lines=text.splitlines()
# find job(s) invoking build-c reusable workflow
for i,line in enumerate(lines, start=1):
if "build-c.yml" in line or "build-c" in line and "uses:" in line:
pass
# print jobs with uses: .github/workflows/build-c.yml or similar
for i,line in enumerate(lines, start=1):
if re.search(r'uses:\s*.*\.github/workflows/build-c\.yml', line):
a=i-15; b=i+80
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
else:
# fallback: show any job that looks like workflow call
for i,line in enumerate(lines, start=1):
if re.search(r'uses:\s*.*\.github/workflows/build-c', line):
a=i-15; b=i+80
for j in range(max(1,a), min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
print("\n--- any top-level permissions in e2e-cloud.yml ---")
for i,line in enumerate(lines, start=1):
if re.match(r'^\s*permissions:\s*', line):
a=i; b=i+15
for j in range(a, min(len(lines),b)+1):
print(f"{j:4d}:{lines[j-1]}")
break
PY
# Show any other workflows that call build-c.yml (to check PR behavior)
echo "----- Find other callers of build-c.yml -----"
rg -n "uses:.*\.github/workflows/build-c\.yml" .github/workflows || trueRepository: boxlite-ai/boxlite
Length of output: 12289
Split the GitHub Release upload out of the reusable C build workflow to keep PR permissions read-only.
.github/workflows/build-c.yml is invoked via workflow_call, but it also contains upload-to-release with permissions: contents: write (guarded by the release-only if:). This still forces the caller in .github/workflows/e2e-cloud.yml (build_c_sdk) to request contents: write, expanding the PR token even though the release-upload job won’t run for PR/workflow_call builds. Move the asset upload to a release-only workflow (or a separate reusable workflow) and keep the reusable build path read-only.
🤖 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 @.github/workflows/build-c.yml around lines 28 - 34, The reusable C build
workflow (.github/workflows/build-c.yml) currently includes the
upload-to-release job which requires permissions: contents: write and forces
callers (e.g., the build_c_sdk invocation in .github/workflows/e2e-cloud.yml) to
request write perms; to fix, extract the upload-to-release job into a separate
release-only workflow (or reusable workflow) that is triggered only for release
events and retains permissions: contents: write, then remove the
upload-to-release job and any contents: write permission from build-c.yml so the
reusable build path remains read-only; update callers like the build_c_sdk
workflow to call the new release workflow when performing releases (or call the
upload workflow directly from release workflows) so PR workflow_call invocations
no longer require expanded permissions.
| tests_or_workflow: ${{ steps.filter.outputs.tests_or_workflow }} | ||
| any: ${{ steps.filter.outputs.api == 'true' || steps.filter.outputs.runner_chain == 'true' || steps.filter.outputs.sdk_py == 'true' || steps.filter.outputs.tests_or_workflow == 'true' }} | ||
| steps: | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workflow file and show the relevant sections with line numbers
ls -la .github/workflows/e2e-cloud.yml
sed -n '1,220p' .github/workflows/e2e-cloud.yml | nl -ba | sed -n '60,220p'Repository: boxlite-ai/boxlite
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e-cloud.yml"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Lines 1-220 (with line numbers) =="
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,220p'
echo
echo "== Checkout step occurrences =="
rg -n "actions/checkout@|checkout:" "$FILE" || true
echo
echo "== persist-credentials occurrences =="
rg -n "persist-credentials" "$FILE" || trueRepository: boxlite-ai/boxlite
Length of output: 11854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e-cloud.yml"
echo "== Find build/test commands =="
rg -n "docker build|maturin build|pytest\b" "$FILE" || true
echo
echo "== Show around e2e checkout (lines 190-360) =="
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '180,360p'
echo
echo "== Show any later checkout steps (should be only 2) =="
rg -n "uses: actions/checkout@" "$FILE" || trueRepository: boxlite-ai/boxlite
Length of output: 11383
🌐 Web query:
actions/checkout@v5 persist-credentials default value
💡 Result:
The default value for the persist-credentials input in actions/checkout@v5 is true [1][2][3]. When set to true, the action configures the authentication token or SSH key with the local git configuration, allowing your workflow scripts to run authenticated git commands [1][3][4]. The credentials are removed during the post-job cleanup process [1][5]. You can set this input to false to opt out of this behavior [1][4].
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://git.woni.link/actions/checkout/src/branch/releases/v5/action.yml
- 3: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 4: https://github.com/actions/checkout/tree/refs/heads/main
- 5: https://github.com/actions/checkout
Set persist-credentials: false on both actions/checkout@v5 steps.
actions/checkout@v5 defaults persist-credentials to true, so the checkouts write GITHUB_TOKEN into local git config. The e2e job later runs repo-controlled docker buildx build, maturin build, and pytest; disable credential persistence on the checkout steps at lines ~94 and ~201.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 94-94: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 94-94: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/e2e-cloud.yml at line 94, The checkout steps using
actions/checkout@v5 are leaving GITHUB_TOKEN in the local git config; update
both occurrences of actions/checkout@v5 in the workflow to include the input
persist-credentials: false (i.e., add persist-credentials: false under the step
that uses actions/checkout@v5) so the checkouts do not persist credentials into
repo-controlled builds and tests.
Source: Linters/SAST tools
Comment on lines
+94
to
+95
| - uses: actions/checkout@v5 | ||
| - uses: dorny/paths-filter@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the specific snippet around the reported lines
FILE=".github/workflows/e2e-cloud.yml"
echo "=== $FILE: lines 70-120 ==="
nl -ba "$FILE" | sed -n '70,120p'
echo "=== $FILE: lines 180-220 ==="
nl -ba "$FILE" | sed -n '180,220p'
echo "=== $FILE: lines 280-340 ==="
nl -ba "$FILE" | sed -n '280,340p'
echo "=== $FILE: lines 300-340 (alt) ==="
nl -ba "$FILE" | sed -n '300,340p'
echo "=== $FILE: lines 700-760 ==="
nl -ba "$FILE" | sed -n '700,760p'
# List all uses: lines in this workflow with their ref
echo "=== $FILE: all uses: lines ==="
rg -n "^\s*-\s*uses:\s*([^\s@]+)@(.+)\s*$|^\s*uses:\s*([^\s@]+)@(.+)\s*$" "$FILE" || trueRepository: boxlite-ai/boxlite
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e-cloud.yml"
echo "=== $FILE: lines 70-120 ==="
cat -n "$FILE" | sed -n '70,120p'
echo "=== $FILE: lines 180-220 ==="
cat -n "$FILE" | sed -n '180,220p'
echo "=== $FILE: lines 280-340 ==="
cat -n "$FILE" | sed -n '280,340p'
echo "=== $FILE: lines 700-760 ==="
cat -n "$FILE" | sed -n '700,760p'
echo "=== $FILE: all uses: refs ==="
rg -n '^\s*-\s*uses:\s*[^@\s]+@([^\s]+)\s*$|^\s*uses:\s*[^@\s]+@([^\s]+)\s*$' "$FILE"Repository: boxlite-ai/boxlite
Length of output: 12329
Pin reusable actions to full commit SHAs
This workflow still uses mutable tags (on a job that assumes AWS roles via OIDC), e.g. actions/checkout@v5 (94, 201), dorny/paths-filter@v3 (95), aws-actions/configure-aws-credentials@v4 (204), aws-actions/amazon-ecr-login@v2 (293), actions/download-artifact@v4 (315), actions/upload-artifact@v4 (725). Pin to commit SHAs to prevent retag risk.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 94-94: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 94-94: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 95-95: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/e2e-cloud.yml around lines 94 - 95, Replace mutable action
tags with pinned full commit SHAs for all reusable actions used in the workflow:
change uses: actions/checkout@v5, uses: dorny/paths-filter@v3, uses:
aws-actions/configure-aws-credentials@v4, uses: aws-actions/amazon-ecr-login@v2,
uses: actions/download-artifact@v4, and uses: actions/upload-artifact@v4 to
their corresponding commit SHA references (e.g., actions/checkout@<sha>); locate
the lines that contain those exact uses: strings and update each to the
canonical commit SHA for the released version you want to pin, commit the
updated workflow, and verify the workflow still runs as expected.
Source: Linters/SAST tools
Comment on lines
+347
to
+350
| PREFIX="builds/runner-deploy" | ||
| BIN_KEY="${PREFIX}/binary.tar.gz" | ||
| REQ_KEY="${PREFIX}/request-${GITHUB_SHA}.txt" | ||
| DONE_KEY="${PREFIX}/done-${GITHUB_SHA}.txt" |
There was a problem hiding this comment.
The runner deploy acknowledgement is not unique per workflow run.
request-${GITHUB_SHA}.txt / done-${GITHUB_SHA}.txt collide across reruns of the same commit. A stale done-<sha>.txt from an earlier successful run will satisfy the poll immediately, so this job can continue before the newly uploaded runner binary is ever applied.
🛠️ Minimal fix
REQ_KEY="${PREFIX}/request-${GITHUB_SHA}.txt"
DONE_KEY="${PREFIX}/done-${GITHUB_SHA}.txt"
+ aws s3 rm "s3://${S3_BUCKET}/${DONE_KEY}" 2>/dev/null || trueAlso applies to: 365-393
🤖 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 @.github/workflows/e2e-cloud.yml around lines 347 - 350, The request/done
keys collide across reruns because they only use GITHUB_SHA; update the key
construction to include a per-run unique identifier (e.g., GITHUB_RUN_ID or
GITHUB_RUN_NUMBER) so the files are unique to each workflow run; change
BIN_KEY/REQ_KEY/DONE_KEY (and any other places where PREFIX is reused in the
same workflow block) to incorporate the chosen run-id variable (for example:
"${PREFIX}/request-${GITHUB_SHA}-${GITHUB_RUN_ID}.txt" and similarly for
DONE_KEY and BIN_KEY) and ensure all other occurrences in the same region (the
repeated block at the later range) are updated consistently.
Contributor
Author
|
Superseded by smaller reviewable PRs:
The workflow/e2e-cloud infrastructure changes from this branch are intentionally not included in those PRs and will be split into a separate draft later. |
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
3 participants
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
Rebuild the box boot pipeline that #715 removed, around 3 curated, digest-pinned images (
base|python|node), and repair two #715 build/test-debt issues that surfaced along the way.On current main,
box-start.actionis an explicit ERROR stub ("Box image resolution is unavailable") left by the box_template/snapshot removal — no box can boot. This PR replaces that stub with a curated-image resolution layer. Users pass an opaque key; the API resolves it to a pinned private-ghcr OCI ref; the runner pulls it. Anything off the allowlist is rejected with 400 at the request boundary.Commits
style: absorb make lint:fix driftfix(build): #715 runner build break + org test mockapps/runnercallsboxlite.WithPort()(added by #715) but the Go SDK never had the function → runner failed to compile (undefined: boxlite.WithPort) on every clean build. Adds it tosdks/go(C ABI / Rust FFI / port-forwarding already exist; mirrorsWithVolume). org test: fixesOrganizationServicemock arg order (was crashingconfigService.getOrThrow). Both pre-existing on main.feat(box): create box from 3 curated imagesimagekey,curated-images.constantallowlist,box.serviceresolve + reserved label,box-startrebuilt from the ERROR stub,runnerAdapter.createBox(v2 enqueues CREATE_BOX withartifactRef; v0 throws), boxlite-rest mapper threadsimage.fix(api): reserved-label guard + warm-pool default + echoreplaceLabelsstrips the reservedimage-reflabel so users can't escape the allowlist. Correctness: warm-pool boxes get the default image label (else ERROR loop). UX: responses echo the curated key. + spec files.chore: regen api-client TS+Goimage(generated).test(e2e): adapt e2e to curated keysbasekey; bootstrap maps curated env to public refs; drop fixture_setup's snapshot registration (targets the deleted/snapshotsendpoint).Verification
make test— runner Go compiles (WithPort), api jest passes (incl. curated specs + org fix),test:changed:go/test:changed:appsgreen.make testfailures werejailer/health_checkrust integration tests, proven pre-existing on a clean checkout (identical SIGABRT / shim-cleanup with none of these changes) — a separate dev-env / test-cleanup issue, out of scope.test:unit:gohits a cgo-link toolchain mismatch (libboxlite.a26.2 vs 26.0) — host artifact, passes on Linux.Security notes
resolveCuratedImageRef→ 400) and at label mutation (replaceLabelsguard).boxlite.io/image-refis system-owned: user values stripped on create and on label replace.Follow-ups (out of scope)
image; UI wiring separate).POST create-box {image:'base'}→ STARTED).Summary by CodeRabbit
New Features
Bug Fixes / Behavior
SDK
Tests