Conversation
- Integrated Rolldown for hot module replacement (HMR) in `arkor dev`, allowing real-time updates to the training interface without page refresh. - Implemented `requestEarlyStop` and `replaceCallbacks` methods in the Trainer API to facilitate graceful stopping of training jobs and dynamic callback updates during execution. - Updated documentation to reflect new features and usage patterns for improved developer guidance. - Adjusted cleanup logic for better resource management during development sessions.
…l shutdown - Introduced `replaceCallbacks` method in the Trainer API to allow dynamic updates of lifecycle callbacks during training runs. - Enhanced signal handling for graceful early stopping, ensuring in-flight checkpoints are preserved during HMR rebuilds. - Added `registerCleanupHook` for streamlined resource management on process exit, improving cleanup logic across development commands. - Updated documentation to reflect new features and usage patterns for better developer guidance.
…internal callback swapping - Removed the public `replaceCallbacks` method from the Trainer interface to prevent exposure of the hot-swapping functionality. - Introduced an internal mechanism for callback swapping using a `Symbol.for`-keyed brand, allowing for dynamic updates during training runs without affecting the public API. - Updated signal handling to ensure seamless integration with the new callback swapping logic, enhancing the hot module replacement (HMR) experience. - Revised documentation to reflect changes in the Trainer API and clarify the internal callback management process.
…lement internal early-stop handling - Removed the public `requestEarlyStop` method from the Trainer interface to prevent exposure of the early-stop functionality. - Introduced an internal mechanism for early stopping using a `Symbol.for`-keyed brand, allowing for graceful stopping after the next checkpoint without affecting the public API. - Updated signal handling to ensure seamless integration with the new early-stop logic, enhancing the hot module replacement (HMR) experience. - Revised documentation to reflect changes in the Trainer API and clarify the internal early-stop management process.
…llback replacement - Updated the Trainer API to remove public exposure of `requestEarlyStop` and `replaceCallbacks` methods, enhancing encapsulation. - Implemented internal mechanisms for early stopping and callback swapping using `Symbol.for`-keyed brands, ensuring seamless integration during training runs. - Revised signal handling to improve the hot module replacement (HMR) experience and maintain clean resource management. - Updated documentation to reflect these changes and clarify the internal management processes for developers.
- Replaced esbuild with Rolldown for building the project, ensuring external dependencies are resolved correctly. - Implemented a cleanup hook system to manage resource disposal on process termination signals. - Enhanced the development experience with hot module replacement (HMR) capabilities, allowing for seamless updates during training runs. - Updated documentation to reflect changes in the development loop and HMR behavior. - Added tests for new signal handling and configuration hashing functionality.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8abc594369
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hotSwapTargets = activeTrains.notifyCallbackReload(nextHash); | ||
| const restartTargets = | ||
| activeTrains.requestEarlyStopOnMismatch(nextHash); |
There was a problem hiding this comment.
notifyCallbackReload/requestEarlyStopOnMismatch run inside each /api/dev/events subscriber callback, so one rebuild sends signals once per connected SSE client (and again on reconnect when createHmrCoordinator.subscribe replays lastEvent). In the common case of two open Studio tabs, the same child can receive two SIGTERMs for one config-changing rebuild; the second signal triggers the runner’s forced-exit path (exit(143)) instead of checkpoint-preserving early stop. Move signal dispatch out of per-subscriber delivery (or dedupe per rebuild hash) so each child is signaled at most once per rebuild event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR upgrades the local Studio dev experience by migrating the trainer bundling pipeline from esbuild to Rolldown, adding an HMR/SSE channel to propagate rebuild events to the Studio SPA, and introducing signal-based “hot-swap callbacks vs early-stop + restart” behavior for in-flight training subprocesses.
Changes:
- Replace esbuild-based trainer bundling with Rolldown (one-shot build + watch/HMR).
- Add
/api/dev/eventsSSE stream + SPA client wiring to refresh manifest and coordinate restarts/hot-swaps. - Introduce internal trainer inspection/callback replacement/early-stop hooks (via
Symbol.forbrands), plus cleanup utilities and updated docs/tests.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removes esbuild and adds rolldown to the lockfile graph. |
| packages/arkor/package.json | Swaps dependency from esbuild to rolldown. |
| packages/arkor/src/core/rolldownConfig.ts | Centralizes Rolldown input options + entry/outDir resolution and Node target derivation. |
| packages/arkor/src/cli/commands/build.ts | Implements arkor build via Rolldown bundling instead of esbuild. |
| packages/arkor/src/studio/hmr.ts | Adds lazy Rolldown watcher coordinator that emits ready/rebuild/error events. |
| packages/arkor/src/studio/hmr.test.ts | Tests HMR coordinator behavior (ready/rebuild/error/replay/dispose). |
| packages/arkor/src/studio/manifest.ts | Adds configHash to manifest summary and splits “summarise built manifest” vs “build + summarise”. |
| packages/arkor/src/studio/trainRegistry.ts | Adds per-child registry + policy for SIGUSR2 hot-swap vs SIGTERM early-stop. |
| packages/arkor/src/studio/trainRegistry.test.ts | Tests TrainRegistry signaling decisions and error tolerance. |
| packages/arkor/src/studio/server.ts | Adds /api/dev/events SSE route and integrates TrainRegistry into rebuild handling. |
| packages/arkor/src/studio/server.test.ts | Adds tests for /api/dev/events token rules, loopback guard, and SSE framing. |
| packages/arkor/src/core/configHash.ts | Adds stable hashing of JobConfig for “callbacks-only vs restart” decisions. |
| packages/arkor/src/core/configHash.test.ts | Tests determinism and sensitivity of hashJobConfig. |
| packages/arkor/src/core/trainerInspection.ts | Introduces internal inspection + callback replacement + early-stop brands via Symbol.for. |
| packages/arkor/src/core/trainer.ts | Implements callback hot-swap support + early-stop latch; attaches internal brands. |
| packages/arkor/src/core/trainer.test.ts | Adds coverage for early-stop and callback hot-swap behavior via internal brands. |
| packages/arkor/src/core/runnerSignals.ts | Adds signal handlers: SIGTERM graceful early-stop + SIGUSR2 callback reload. |
| packages/arkor/src/core/runnerSignals.test.ts | Tests SIGTERM/SIGUSR2 handler behaviors with branded trainers. |
| packages/arkor/src/core/runner.ts | Installs/removes new signal handlers around trainer.start()/trainer.wait(). |
| packages/arkor/src/core/runner.test.ts | Adds a test for SIGTERM early-stop behavior in the runner. |
| packages/arkor/src/cli/cleanupHooks.ts | Adds reusable cleanup-hook registration for exit/signals. |
| packages/arkor/src/cli/commands/dev.ts | Wires HMR coordinator into Studio server and refactors shutdown cleanup via cleanup hooks. |
| packages/arkor/src/cli/commands/start.test.ts | Updates test comment to refer to Rolldown instead of esbuild. |
| packages/studio-app/src/lib/api.ts | Adds typed DevEvent + openDevEvents() for SSE notifications. |
| packages/studio-app/src/components/RunTraining.tsx | SPA listens to dev SSE events, refreshes manifest, and coordinates restart/hot-swap UI state. |
| docs/concepts/studio.mdx | Documents HMR workflow, including hash-based hot-swap vs restart behavior (EN). |
| docs/ja/concepts/studio.mdx | Documents HMR workflow (JA). |
| AGENTS.md | Updates repository policy/docs for new SSE allow-list, HMR internals, and Rolldown build pipeline. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [log, setLog] = useState(""); | ||
| const [manifest, setManifest] = useState<ManifestResult | null>(null); | ||
| const [hmrStatus, setHmrStatus] = useState< | ||
| "idle" | "rebuilding" | "early-stopping" | "restarting" | "hot-swapped" |
| import { ensureProjectState } from "../core/projectState"; | ||
| import { readState } from "../core/state"; | ||
| import { readManifestSummary } from "./manifest"; | ||
| import { readManifestSummary, summariseBuiltManifest } from "./manifest"; |
| function scheduleHmrCleanup(hmr: { dispose: () => Promise<void> }): void { | ||
| // Registered before the studio-token cleanup so it runs first on | ||
| // shutdown — Node fires signal handlers in registration order, and we | ||
| // want the watcher to release file handles before the outermost | ||
| // process.exit. | ||
| registerCleanupHook({ cleanup: () => hmr.dispose() }); | ||
| } |
| requestEarlyStopOnMismatch( | ||
| nextConfigHash: string | null, | ||
| ): RestartTarget[] { | ||
| const targets: RestartTarget[] = []; | ||
| for (const [pid, entry] of this.entries) { | ||
| if ( | ||
| nextConfigHash === null || | ||
| entry.configHash === null || | ||
| entry.configHash !== nextConfigHash | ||
| ) { | ||
| try { | ||
| entry.child.kill("SIGTERM"); | ||
| } catch { | ||
| // child already exited; close handler will clean up. | ||
| } | ||
| targets.push({ pid, trainFile: entry.trainFile }); | ||
| } |
| await callbacks.onCheckpoint?.(ctx); | ||
| // Early-stop latch: a checkpoint just landed, so the in-flight work | ||
| // is durable. Cancel the cloud job and end `wait()` cleanly. | ||
| if (earlyStopRequested && earlyStopDeferred) { | ||
| await trainer.cancel(); | ||
| if (earlyStopDeferred.timer) clearTimeout(earlyStopDeferred.timer); | ||
| earlyStopDeferred.resolve(); | ||
| earlyStopDeferred = null; | ||
| return { terminal: true, artifacts: terminalResult?.artifacts ?? [] }; | ||
| } |
| function startWatcher(): void { | ||
| if (watcher || disposed) return; | ||
| if (!existsSync(resolved.entry)) { | ||
| broadcast({ | ||
| type: "error", | ||
| message: `Build entry not found: ${resolved.entry}. Create ${BUILD_DEFAULTS.entry} or pass an explicit entry argument.`, | ||
| }); | ||
| return; | ||
| } |
…ous BUNDLE_END on macOS - Modified the test to assert that the late subscriber receives the same event as the prior subscriber, addressing issues with spurious BUNDLE_END events in rolldown@1.0.0-rc.17 on macOS. - Added comments to clarify the reasoning behind the change and noted a future task to revisit the assertion after rolldown stabilizes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f510869c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| restartPendingRef.current = true; | ||
| setHmrStatus(runningRef.current ? "early-stopping" : "idle"); |
There was a problem hiding this comment.
restartPendingRef is set for every payload.restart event even when this tab is not running a training stream. Since /api/dev/events broadcasts restart events to all connected tabs, a passive tab can cache restartPendingRef=true from another tab’s run; the next time the user runs training in this tab, finally will auto-trigger an extra run, creating an unintended duplicate job and extra cloud/GPU spend. Only mark restart pending when runningRef.current is true (or otherwise correlate the restart target to the local run).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch { | ||
| // child already exited; close handler will clean up. | ||
| } | ||
| targets.push({ pid, trainFile: entry.trainFile }); |
| import { ensureProjectState } from "../core/projectState"; | ||
| import { readState } from "../core/state"; | ||
| import { readManifestSummary } from "./manifest"; | ||
| import { readManifestSummary, summariseBuiltManifest } from "./manifest"; |
| const [log, setLog] = useState(""); | ||
| const [manifest, setManifest] = useState<ManifestResult | null>(null); | ||
| const [hmrStatus, setHmrStatus] = useState< | ||
| "idle" | "rebuilding" | "early-stopping" | "restarting" | "hot-swapped" |
| // Early-stop latch: a checkpoint just landed, so the in-flight work | ||
| // is durable. Cancel the cloud job and end `wait()` cleanly. | ||
| if (earlyStopRequested && earlyStopDeferred) { | ||
| await trainer.cancel(); | ||
| if (earlyStopDeferred.timer) clearTimeout(earlyStopDeferred.timer); | ||
| earlyStopDeferred.resolve(); | ||
| earlyStopDeferred = null; | ||
| return { terminal: true, artifacts: terminalResult?.artifacts ?? [] }; | ||
| } | ||
| return { terminal: false, artifacts: terminalResult?.artifacts ?? [] }; |
…ce failures - Added a test to verify that the SIGINT exit handler is armed even if the `persistStudioToken` function fails, preventing the dev server from idling indefinitely. - Updated the `runDev` function to register the studio-token cleanup hook unconditionally, ensuring it executes on process termination signals regardless of token persistence success. - Enhanced cleanup logic to maintain proper signal handling during development sessions, improving overall stability and user experience.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55b4a2e8f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (payload.restart) { | ||
| // `/api/dev/events` is a broadcast — every open Studio tab gets | ||
| // this event. Only flip the auto-restart latch when *this* tab | ||
| // is actually running a stream right now; otherwise a passive | ||
| // tab would silently auto-spawn an extra job the next time the | ||
| // user clicks Run training, doubling cloud spend. | ||
| if (runningRef.current) { | ||
| // Training run is early-stopping; the active stream will |
There was a problem hiding this comment.
The handler sets restartPendingRef for any payload.restart while this tab is running, but rebuild events are broadcast across all tabs and can contain mixed outcomes (restartTargets and hotSwapTargets) for different subprocesses. In a multi-run setup, a tab whose run was only hot-swapped can still latch restartPendingRef=true, then auto-spawn an extra training job when its current run eventually finishes, increasing unintended cloud/GPU usage.
Useful? React with 👍 / 👎.
| entry.child.kill("SIGTERM"); | ||
| entry.earlyStopRequested = true; |
There was a problem hiding this comment.
ChildProcess.kill() returns a boolean and does not throw when a process is already gone, but this path treats any non-throw as success. If kill("SIGTERM") returns false during an exit race, the entry is still marked earlyStopRequested and added to targets, so the UI may wait for/restart a run that was never actually signaled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| const es = openDevEvents(); | ||
| const onMessage = (raw: MessageEvent) => { |
| setHmrStatus("hot-swapped"); | ||
| window.setTimeout(() => { | ||
| setHmrStatus((s) => (s === "hot-swapped" ? "idle" : s)); | ||
| }, 1500); | ||
| } else { |
| }, | ||
| cancel() { | ||
| activeTrains.unregister(child.pid); | ||
| child.kill(); |
| // Synchronous wrapper so signal handlers preserve "cleanup landed | ||
| // before this function returns" — important for sync cleanups (e.g. | ||
| // `unlinkSync`) and for tests that assert the side effect right after | ||
| // invoking the handler. Async cleanups are fire-and-forget with a | ||
| // catch so a hung dispose doesn't block exit. | ||
| const run = (): void => { | ||
| if (done) return; | ||
| done = true; | ||
| try { | ||
| const result = options.cleanup(); | ||
| if (result && typeof (result as Promise<void>).catch === "function") { | ||
| (result as Promise<void>).catch(() => { | ||
| // best-effort: shutdown is racing other cleanup paths | ||
| }); | ||
| } | ||
| } catch { | ||
| // best-effort | ||
| } | ||
| }; | ||
|
|
||
| process.on("exit", run); | ||
|
|
||
| for (const sig of TERMINATING_SIGNALS) { | ||
| process.on(sig, () => { | ||
| run(); | ||
| if (options.exitOnSignal) process.exit(0); |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| cancel() { | ||
| child.kill(); | ||
| // The SPA-side cancel is always *user-initiated*: either an | ||
| // explicit Stop click or tab-close/navigation, which the | ||
| // user just as explicitly chose. HMR-driven SIGTERMs go | ||
| // straight from the server to the runner via | ||
| // `dispatchRebuild`; they DO NOT trigger this handler | ||
| // (the SPA waits for the train stream's `exit=` line and | ||
| // schedules auto-restart, never aborting). So manual stop | ||
| // takes precedence over any in-flight HMR graceful path: | ||
| // we POST cloud cancel + SIGKILL unconditionally. |
There was a problem hiding this comment.
Bug: A race condition where ReadableStream.cancel() can run before start() completes, causing the cancelTeardown function to be null and preventing proper cloud job cancellation.
Severity: HIGH
Suggested Fix
Synchronize the start() and cancel() methods. A promise should be used to ensure that cancel() waits for start() to complete its initialization, including the assignment of cancelTeardown, before proceeding. This prevents the race condition by guaranteeing that cancelTeardown is not null when cancel() is called.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/arkor/src/studio/server.ts#L852-L861
Potential issue: A race condition exists in the `ReadableStream` handling for training
jobs. The WHATWG Streams specification allows `cancel()` to be called before the
`start()` method completes. If a client aborts a `/api/train` request immediately,
`cancel()` can execute before `start()` has initialized the `cancelTeardown` function.
This causes the cancellation logic to fail silently because `cancelTeardown` is `null`,
and the subsequent logic to cancel the cloud job cannot retrieve the `parsedJobId`. This
results in the cloud GPU job being orphaned, incurring unnecessary costs.
Also affects:
packages/arkor/src/studio/server.ts:656~656
Did we get this right? 👍 / 👎 to inform future reviews.
|
Code review by qodo was updated up to the latest commit fef3bf2 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 45 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="eslint.config.ts">
<violation number="1" location="eslint.config.ts:579">
P2: These new ESLint-only path exceptions make lint behavior diverge between ESLint and oxlint for the same `process.exit` usage. Mirroring these file globs in `oxlint.config.ts` would keep CI/tooling consistent with repository policy and avoid rule drift.</violation>
</file>
<file name="packages/arkor/src/core/runner.ts">
<violation number="1" location="packages/arkor/src/core/runner.ts:125">
P2: Embedding `runTrainer()` in a host process now gives the SDK process-wide signal ownership: SIGTERM/SIGINT/SIGHUP can call `process.exit(...)`, and SIGUSR2 can re-import the entry even when the caller is not Studio. Consider moving these handlers to the CLI/Studio path or making signal/HMR handling opt-in for public `runTrainer` callers.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| "**/bin.ts", | ||
| "**/bin.mjs", | ||
| "packages/arkor/src/cli/commands/**/*.ts", | ||
| "packages/arkor/src/cli/cleanupHooks.ts", |
There was a problem hiding this comment.
P2: These new ESLint-only path exceptions make lint behavior diverge between ESLint and oxlint for the same process.exit usage. Mirroring these file globs in oxlint.config.ts would keep CI/tooling consistent with repository policy and avoid rule drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At eslint.config.ts, line 579:
<comment>These new ESLint-only path exceptions make lint behavior diverge between ESLint and oxlint for the same `process.exit` usage. Mirroring these file globs in `oxlint.config.ts` would keep CI/tooling consistent with repository policy and avoid rule drift.</comment>
<file context>
@@ -565,11 +565,19 @@ export default defineConfig(
"**/bin.ts",
"**/bin.mjs",
"packages/arkor/src/cli/commands/**/*.ts",
+ "packages/arkor/src/cli/cleanupHooks.ts",
+ "packages/arkor/src/core/runnerSignals.ts",
],
</file context>
| process.stdout.write( | ||
| `Job ${result.job.id} finished with status=${result.job.status}\n`, | ||
| ); | ||
| const removeShutdown = installShutdownHandlers(trainer); |
There was a problem hiding this comment.
P2: Embedding runTrainer() in a host process now gives the SDK process-wide signal ownership: SIGTERM/SIGINT/SIGHUP can call process.exit(...), and SIGUSR2 can re-import the entry even when the caller is not Studio. Consider moving these handlers to the CLI/Studio path or making signal/HMR handling opt-in for public runTrainer callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/arkor/src/core/runner.ts, line 125:
<comment>Embedding `runTrainer()` in a host process now gives the SDK process-wide signal ownership: SIGTERM/SIGINT/SIGHUP can call `process.exit(...)`, and SIGUSR2 can re-import the entry even when the caller is not Studio. Consider moving these handlers to the CLI/Studio path or making signal/HMR handling opt-in for public `runTrainer` callers.</comment>
<file context>
@@ -60,10 +122,20 @@ export async function runTrainer(file?: string): Promise<void> {
- process.stdout.write(
- `Job ${result.job.id} finished with status=${result.job.status}\n`,
- );
+ const removeShutdown = installShutdownHandlers(trainer);
+ const removeCallbackReload = installCallbackReloadHandler(trainer, abs);
+ try {
</file context>
… failure Windows CI (red since round 81): the manual-stop-during-HMR test pinned the POSIX cancel-POST count, but on win32 the HMR dispatch path fires a compensation cancel POST (SIGTERM is an abrupt kill there) before the manual Stop fires its own; both are correct, so the assertion now branches per platform. Review round 85 (sentry / cubic): - configHash: unbox Number/String/Boolean wrappers via the Object.prototype.toString tag for JSON.stringify parity - trainerInspection: inspection guard also requires `callbacks` so a partial brand thunk can't rotate the live callback cell to undefined - trainRegistry: rpc snapshots mirror the entry lifecycle on re-register, so pid reuse can't inherit a stale bearer token - hmr: a synchronous watch() throw broadcasts an error frame instead of escaping subscribe(); broadcast iterates a snapshot so a nested subscribe can't double-deliver - rolldownConfig: resolve() the cwd to honor the absolute-path contract - RunTraining: the "[hmr] auto-restart suppressed" note only appears when a restart was actually latched (state reset stays unconditional) - runner.test: settle the hung wait() in an inner finally so assertion failures can't leak live SIGTERM handlers into later tests - oxlint: mirror eslint's new no-process-exit globs (parity policy) - server: document why cancel() cannot observe a null cancelTeardown (sync start), refuting the reported init-order race - moduleCacheBust/runner: document the accepted key-vs-import read race and runTrainer's process-wide signal ownership - docs (EN/JA): early-stop timeout falls back to immediate cancel; abortSignal + cancel() stops now, combine with onCheckpoint for the wait-for-checkpoint behaviour
Greptile SummaryThis PR moves Studio development builds to Rolldown and adds live trainer updates. The main changes are:
Confidence Score: 4/5This should be fixed before merging.
Files Needing Attention: packages/arkor/src/studio/server.ts
|
| Filename | Overview |
|---|---|
| packages/arkor/src/studio/server.ts | Adds HMR event handling and train-process cancellation paths, but pre-marker cancellation can still leave cloud jobs running. |
| packages/arkor/src/studio/trainRegistry.ts | Tracks active train processes and rebuild decisions for hot-swap or restart behavior. |
| packages/arkor/src/core/rolldownConfig.ts | Centralizes Rolldown bundle configuration and Node target selection. |
Prompt To Fix All With AI
### Issue 1
packages/arkor/src/studio/server.ts:1059-1067
**Stop Can Still Orphan Jobs**
When `trainer.start()` takes longer than this 2s poll, `parsedJobId` stays null, the child is killed, and the null-job-id guard skips the cloud cancel POST. The runner only prints the nonce-prefixed `Started job` marker after `trainer.start()` resolves, so a slow create-job or first-run setup can still leave the remote training job running after the local stream stops. This path needs a pending cancel that fires when the marker arrives, or it needs to avoid killing the child until the job can be cancelled.
### Issue 2
packages/arkor/src/studio/server.ts:1257
**Restart Skips Late Job Ids**
On Windows, a rebuild can arrive after the cloud job exists but before stdout parsing records `entry.jobId`. The snapshot then has no job id, this branch skips the server-side cancel, and the following restart signal kills the child abruptly. Because the child cannot run its own early-stop cancel path on Windows, the old cloud job can keep running while the SPA starts the replacement run. This restart path needs late-marker or pending-cancel handling before killing the child.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (4): Last reviewed commit: "fix: address round-86 review findings (s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/studio-app/src/components/RunTraining.tsx (1)
696-703: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHMR status banner isn't announced to assistive tech.
The banner conveys transient state ("Stopping at next checkpoint…", "Restarting with updated code…", "Callbacks hot-swapped…") purely visually. Because it's a plain
<div>with no live region, screen-reader users get no notification whenhmrStatustransitions. Addrole="status"/aria-live="polite"so the update is announced. Keep the wrapper mounted and toggle the inner content so the live region can observe the change rather than being added/removed.♿ Proposed fix
- {hmrStatus !== "idle" && ( - <div className="text-xs text-zinc-500 dark:text-zinc-400"> - {hmrStatus === "early-stopping" && "Stopping at next checkpoint…"} - {hmrStatus === "restarting" && "Restarting with updated code…"} - {hmrStatus === "hot-swapped" && - "Callbacks hot-swapped: run continues."} - </div> - )} + <div + role="status" + aria-live="polite" + className="text-xs text-zinc-500 dark:text-zinc-400" + > + {hmrStatus === "early-stopping" && "Stopping at next checkpoint…"} + {hmrStatus === "restarting" && "Restarting with updated code…"} + {hmrStatus === "hot-swapped" && "Callbacks hot-swapped: run continues."} + </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio-app/src/components/RunTraining.tsx` around lines 696 - 703, Add role="status" and aria-live="polite" to the HMR status wrapper in RunTraining, keeping the wrapper mounted continuously and conditionally rendering or updating only its inner message so assistive technologies announce hmrStatus transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/studio.mdx`:
- Around line 19-20: Update the Studio concepts documentation to qualify the
same-hash callback hot-swap behavior: on win32, skip SIGUSR2 and use the SIGTERM
restart path because Node treats the signal as forceful termination. Add the
equivalent platform caveat to the Japanese Studio documentation, preserving the
existing behavior descriptions for other platforms.
---
Outside diff comments:
In `@packages/studio-app/src/components/RunTraining.tsx`:
- Around line 696-703: Add role="status" and aria-live="polite" to the HMR
status wrapper in RunTraining, keeping the wrapper mounted continuously and
conditionally rendering or updating only its inner message so assistive
technologies announce hmrStatus transitions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8357445a-7378-4ad5-883f-0e1f00f0b64c
📒 Files selected for processing (20)
AGENTS.mddocs/concepts/studio.mdxdocs/ja/concepts/studio.mdxoxlint.config.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/core/configHash.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/studio-app/src/components/RunTraining.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (87)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: CodeRabbit
- GitHub Check: Seer Code Review
- GitHub Check: install · pnpm-11 · macos-latest · node 26.0.0
- GitHub Check: install · yarn-berry · macos-latest · node 26.0.0
- GitHub Check: install · pnpm-11 · macos-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-10 · macos-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-10 · macos-latest · node 26.0.0
- GitHub Check: install · npm · macos-latest · node 26.0.0
- GitHub Check: install · yarn · macos-latest · node 26.0.0
- GitHub Check: install · bun · macos-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · macos-latest · node >=26.0.0 <27
- GitHub Check: install · npm · macos-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · macos-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · macos-latest · node 26.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node >=24.12.0 <25
- GitHub Check: install · npm · windows-latest · node 26.0.0
- GitHub Check: install · npm · windows-latest · node 24.12.0
- GitHub Check: install · bun · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-11 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node 26.0.0
- GitHub Check: install · yarn · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node 24.12.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-9 · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node >=22.22.0 <23
- GitHub Check: install · npm · windows-latest · node 22.22.0
- GitHub Check: install · yarn-berry · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-10 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=22.22.0 <23
- GitHub Check: coverage · upload to Codecov
🧰 Additional context used
📓 Path-based instructions (18)
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Do not use em dashes (U+2014) in comments, string literals, or template literals anywhere in the codebase; use colons, periods, commas, parentheses, spaced hyphens (
" - "), or rephrasing instead.
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Node.js
>=22.22.0compatibility for published package code; use Node 24 as the preferred development version.
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{js,mjs,cjs,ts,tsx,json,css,html}
📄 CodeRabbit inference engine (AGENTS.md)
Use oxfmt for formatting; do not manually alter whitespace, wrapping, quote style, or trailing commas contrary to
oxfmt.config.ts.
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
!**/*.{md,mdx,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Do not apply oxfmt to Markdown, MDX, YAML, or YML files; these are intentionally excluded from formatting.
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{ts,tsx,js,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,mjs,cjs}: Run both linters for package linting:oxlint --deny-warnings . && eslint .; add shared rule changes to the rootoxlint.config.tsoreslint.config.ts, not per-package configs.
Do not enable unsafe automatic fixes forunicorn/no-typeof-undefinedor@typescript-eslint/no-unnecessary-type-assertion; treat their findings as manual-review items unless a safer upstream fix exists.
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**
⚙️ CodeRabbit configuration file
**: # Arkor Development GuideNote: Claude Code automatically loads this file.
Repository shape
pnpm + Turbo monorepo. Workspaces are declared in
pnpm-workspace.yaml(packages/*,e2e/*,examples/*,docs).
Path Role packages/arkor Published arkorSDK + CLI + bundled local Studio server (Hono).bin/arkor→dist/bin.mjs. Library entry →dist/index.mjs.packages/create-arkor Published create-arkorscaffolder (pnpm create arkor).packages/cli-internal Private workspace package. Source is bundled into arkorandcreate-arkorvia tsdown'sdeps.alwaysBundle. Never appears as a runtime dependency on npm.packages/studio-app Private Vite + React 19 SPA. pnpm --filter@arkor/studio-appbundlebuilds it;packages/arkor/scripts/copy-studio-assets.mjscopiesdist/intopackages/arkor/dist/assets/.e2e/cli Private vitest suite that spawns the built dist/bin.mjsof both CLIs in temp dirs.e2e/studio Private Playwright suite that spawns arkor devagainst an in-process fake cloud-api and drives the Studio SPA in Chromium.examples/doc-drift Private use-case example: documentation drift check against an Arkor deployment (zero-dependency script + copy-me workflow). docs Mintlify sources for docs.arkor.ai. Common commands
Root scripts fan out via Turbo (which respects
^builddeps in turbo.json):pnpm install pnpm build # turbo run build across all packages pnpm typecheck # tsc --noEmit across all packages pnpm lint # oxlint --deny-warnings, then strict ESLint 10 (single root config each) pnpm format # oxfmt --write across the repo (config in oxfmt.config.ts) pnpm format:check # oxfmt --check; CI gates on this (no wri...
Files:
oxlint.config.tspackages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tsdocs/concepts/studio.mdxdocs/ja/concepts/studio.mdxpackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tsAGENTS.mdpackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in comments, string literals, and template literals in any package file; this includes CLI runtime messages, generated-file template bodies, and test names.
Files:
packages/arkor/src/core/rolldownConfig.tspackages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/*/src/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Add Vitest tests for SDK, CLI, and scaffolder logic changes; consider an
e2e/cliscenario for CLI flow changes.
Files:
packages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runner.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/server.test.ts
packages/arkor/src/{studio,cli}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When changing Studio server or SPA fetch behavior, preserve the token header for
fetch, query token forEventSource, host-header allow-list, absence of CORS, and timing-safe token comparison.
Files:
packages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/cli/commands/build.close.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/arkor/src/studio/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
For HMR restarts, use
configHashto choose callback hot-swap versus graceful SIGTERM restart; do not replace the graceful HMR path with SIGKILL or bypass the hash check.
Files:
packages/arkor/src/studio/hmr.watchFailure.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Keep paired English and Japanese documentation synchronized: update
README.mdwithREADME.ja.md,CONTRIBUTING.mdwithCONTRIBUTING.ja.md, and files underdocs/with theirdocs/ja/mirrors in the same change.
Files:
docs/concepts/studio.mdxdocs/ja/concepts/studio.mdxAGENTS.md
docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
Verify Mintlify-generated heading IDs before adding cross-page anchors; preserve
/,=, and full-width Japanese parentheses, while ASCII parentheses and backticks are stripped.
Files:
docs/concepts/studio.mdxdocs/ja/concepts/studio.mdx
packages/arkor/src/core/runner.ts
📄 CodeRabbit inference engine (AGENTS.md)
Preserve project entry-point discovery order: named
arkor, namedtrainer, default manifest or Trainer, thendefault.trainer.
Files:
packages/arkor/src/core/runner.ts
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain AGENTS.md as a living document with current agent status and architectural decisions
Files:
AGENTS.md
*.{yml,yaml,json,html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in YAML, Markdown, JSON, and HTML files.
Files:
AGENTS.md
packages/studio-app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/studio-app/**/*.{ts,tsx}: Declare React, React DOM, and bundled font packages as runtime dependencies when they are inlined into the Studio SPA bundle.
Keep the Vite Studio-token plugin dev-only (apply: "serve"); never bake a per-launch token into productionvite buildoutput.
Files:
packages/studio-app/src/components/RunTraining.tsx
packages/arkor/src/core/trainerInspection.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep HMR-only trainer operations such as callback replacement and early-stop requests behind
Symbol.forbrands; do not exposereplaceCallbacks()orrequestEarlyStop()on the publicTrainerinterface.
Files:
packages/arkor/src/core/trainerInspection.ts
packages/arkor/src/studio/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
Only add routes to
eventStreamPathPatternwhen they are GET-only streaming endpoints; never allow mutation endpoints to use query-token authentication.
Files:
packages/arkor/src/studio/server.ts
🪛 ast-grep (0.44.1)
packages/arkor/src/studio/trainRegistry.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
packages/arkor/src/studio/server.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessByStdio } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/concepts/studio.mdx
[style] ~20-~20: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...l work is preserved as an artifact. One caveat: if no checkpoint arrives within the ea...
(CAVEAT)
[style] ~22-~22: Since ownership is already implied, this phrasing may be redundant.
Context: ... checkpoint. Stopping a run early from your own code (rather than from the dev loop) is...
(PRP_OWN)
AGENTS.md
[style] ~86-~86: Consider using the typographical ellipsis character here instead.
Context: ...r a code change is contained inside the callbacks: { ... } object. (win32 exception: Node's `ki...
(ELLIPSIS)
🔇 Additional comments (20)
docs/ja/concepts/studio.mdx (1)
19-20: Mirror the Windows HMR fallback.The Japanese page has the same omission: on win32, matching hashes use the SIGTERM restart path instead of in-place SIGUSR2 hot-swapping.
AGENTS.md (1)
70-79: LGTM!Also applies to: 80-92, 93-97
packages/studio-app/src/components/RunTraining.tsx (1)
513-548: LGTM!oxlint.config.ts (1)
113-118: 📐 Maintainability & Code QualityDrop this comment:
eslint.config.tsalready includespackages/arkor/src/cli/cleanupHooks.tsandpackages/arkor/src/core/runnerSignals.ts, and both modules callprocess.exit(...).> Likely an incorrect or invalid review comment.packages/arkor/src/cli/commands/build.close.test.ts (1)
59-61: LGTM!packages/arkor/src/core/moduleCacheBust.ts (2)
56-68: LGTM!
80-88: LGTM!packages/arkor/src/core/configHash.test.ts (1)
258-287: LGTM!packages/arkor/src/core/configHash.ts (1)
94-120: LGTM!packages/arkor/src/core/rolldownConfig.ts (1)
29-40: LGTM!packages/arkor/src/core/trainerInspection.ts (1)
84-97: LGTM!packages/arkor/src/core/trainerInspection.test.ts (1)
179-201: LGTM!packages/arkor/src/core/runner.test.ts (1)
273-302: LGTM!packages/arkor/src/core/runner.ts (1)
141-156: LGTM!packages/arkor/src/studio/hmr.ts (1)
322-335: LGTM!Also applies to: 479-501
packages/arkor/src/studio/hmr.watchFailure.test.ts (1)
1-56: LGTM!packages/arkor/src/studio/trainRegistry.ts (1)
216-221: Deleting the stale snapshot on a pid re-register withoutrpccorrectly closes the pid-reuse credential-inheritance hole, and keepingrpcoffentryInitmeanslist()stays token-free.
LGTM!packages/arkor/src/studio/trainRegistry.test.ts (1)
505-542: LGTM!packages/arkor/src/studio/server.ts (1)
656-664: LGTM!packages/arkor/src/studio/server.test.ts (1)
2924-2927: Platform-branchedcancelHitscount matches the server's win32 compensation POST plus manual-Stop POST (both idempotent against the same job id), while POSIX stays at the single manual cancel. Correctly unblocks Windows CI without loosening the URL assertion.
LGTM!Also applies to: 3007-3027
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55e598fb81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const manifest = await readManifestSummary(trainCwd, { | ||
| prebuiltOutFile: hmrOutFile, | ||
| }); |
There was a problem hiding this comment.
When a direct API caller starts an alternate file with POST /api/train { file }, the spawned arkor start <file> rebuilds the shared .arkor/build/index.mjs to that alternate entry. Because the HMR manifest path here now trusts the same hmrOutFile and never rebuilds the default entry, that alternate artifact can persist indefinitely until the default watcher happens to republish, so Studio can show—and a later default Run can execute—the wrong trainer. Fresh evidence beyond the earlier custom-entry hash report: the current null-hash guard only changes routing; it leaves this shared artifact overwrite in place.
Useful? React with 👍 / 👎.
| }); | ||
| const bundle = await rolldown(rolldownInputOptions({ cwd, entry })); | ||
| try { | ||
| await bundle.write({ file: outFile, format: "esm" }); |
There was a problem hiding this comment.
For projects whose trainer source uses a relative dynamic import() (for example lazy-loading a local eval/helper module inside a callback), Rolldown's documented default is automatic code splitting, but this path writes to a single output.file and the runtime only ever imports .arkor/build/index.mjs. That regresses from the old esbuild single-artifact build: such projects can fail at build/watch time or require chunks that arkor start never manages. Set codeSplitting: false (or the equivalent inline-dynamic-imports option) for both this build output and the HMR watcher output.
Useful? React with 👍 / 👎.
| // early-stop that auto-restart is designed for. (User aborts | ||
| // never reach here: the `ac.signal.aborted` branch above returns | ||
| // first.) | ||
| if (streamFailed || exitCode !== 0) { |
There was a problem hiding this comment.
On Windows HMR restarts, Node documents child.kill("SIGTERM") as killing the child forcefully rather than delivering the runner's graceful handler, so the /api/train stream will close with a nonzero or missing exit= code even though the server marked this run in restartTargets and fired its win32 cancel fallback. This unconditional exitCode !== 0 suppression then leaves Windows users with the old run killed/cancelled but no auto-respawn after any config-changing rebuild. Fresh evidence beyond earlier Windows SIGTERM reports: the new server-side cancel fallback releases the cloud job, but this SPA-side gate still rejects the restart it advertised.
Useful? React with 👍 / 👎.
# Conflicts: # AGENTS.md # package.json # packages/arkor/src/cli/commands/dev.test.ts # packages/arkor/src/cli/commands/dev.ts # packages/arkor/src/core/trainer.test.ts # packages/arkor/src/core/trainer.ts # packages/arkor/src/studio/manifest.ts # packages/arkor/src/studio/server.test.ts # packages/arkor/src/studio/server.ts
| return () => { | ||
| es.close(); | ||
| }; |
There was a problem hiding this comment.
Bug: The useEffect hook creates an EventSource and attaches listeners but fails to remove them on cleanup. Calling es.close() is insufficient, causing a memory leak on component unmount.
Severity: MEDIUM
Suggested Fix
In the useEffect cleanup function, explicitly call removeEventListener for each listener that was added to the EventSource object. This should be done before calling es.close(). The listener functions should be defined so they can be referenced in both the setup and cleanup phases.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/studio-app/src/components/RunTraining.tsx#L335-L337
Potential issue: The component creates an `EventSource` and registers listeners
(`ready`, `rebuild`, `error`) within a `useEffect` hook. The cleanup function correctly
calls `es.close()` to terminate the connection but fails to call `removeEventListener`
for the attached listeners. According to the API specification, `close()` does not
remove event listeners. This results in a memory leak, as the listener closures, which
capture component state and refs, are retained in memory after the component unmounts.
Repeated navigation or component remounts will cause leaked memory to accumulate,
potentially degrading application performance over time. A similar issue exists in
`JobDetail.tsx`.
Also affects:
packages/studio-app/src/components/JobDetail.tsx:138~173
- dev.ts: the HMR coordinator's dispose hook now registers inside the listening callback (bind-first flow), not before the token cleanup at module scope - moduleCacheBust: manifest.ts no longer imports through the hash-query URL; its reads go through main's content-addressed snapshot copies, leaving hmr.ts and runnerSignals.ts as the two callers
There was a problem hiding this comment.
🟡 Not ready to approve
Unresolved lifecycle, cancellation, hashing, concurrency, and fresh-scaffold installation issues can cause hangs or overlapping cloud jobs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/arkor/src/studio/hmr.ts:500
- A synchronous
watch()failure permanently disables HMR for the Studio server. The server creates only one coordinator subscription at startup; browser SSE clients subscribe to the server's fan-out set, not to this coordinator, so the stated “later subscribe retries” path never occurs. Add a retry timer (similar to the missing-entry poll) or otherwise reattempt transient EMFILE/EACCES startup failures.
packages/arkor/src/cli/commands/dev.ts:375 - The build watcher is started by
buildStudioAppbefore the HTTP bind succeeds, but the disposer is only registered in the successfullisteningcallback. Ifserverejects withEADDRINUSE,runDevrejects while Rolldown's watcher remains active; becausebin.tsonly setsprocess.exitCode, that watcher can keep the failed CLI process alive. Disposehmrbefore rejecting any pre-bind server error.
// attempt above failed (e.g. cloud-api was unreachable at launch).
packages/arkor/package.json:61
- Replacing the published runtime dependency with Rolldown leaves fresh pnpm scaffolds configured for the removed bundler:
packages/cli-internal/src/scaffold.tsstill emits onlyallowBuilds.esbuild, and the CLI/docs describe the same policy. Under pnpm 11, Rolldown and its native resolver need explicitallowBuildsdecisions (as this repository's ownpnpm-workspace.yaml:132-135demonstrates), otherwise a newly scaffolded project can fail installation withERR_PNPM_IGNORED_BUILDS. Update the scaffolder, both CLIs, paired docs, and tests as part of this migration.
"rolldown": "^1.1.1",
- Files reviewed: 46/47 changed files
- Comments generated: 3
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (typeof fn !== "function") { | ||
| // Best-effort fallback for unbranded trainers: trainer.cancel() | ||
| // is part of the public Trainer interface, so it's always safe | ||
| // to call. Catch/swallow because the documented contract for | ||
| // cancel() is "best-effort" and the SIGTERM handler needs the | ||
| // returned promise to settle either way. | ||
| try { | ||
| await trainer.cancel(); | ||
| } catch { | ||
| // intentionally ignored; see comment above. | ||
| } | ||
| return; | ||
| } | ||
| await fn.call(trainer, opts); |
| const maybeToJSON = (value as { toJSON?: unknown }).toJSON; | ||
| if (typeof maybeToJSON === "function") { | ||
| return stableStringifyRec( | ||
| (maybeToJSON as (key: string) => unknown).call(value, key), | ||
| key, | ||
| seen, | ||
| ); |
| try { | ||
| if (existsSync(path)) { | ||
| // Newest writer wins: discard the claimed older copy. | ||
| unlinkSync(reapPath); | ||
| } else { | ||
| renameSync(reapPath, path); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/arkor/src/studio/server.ts (3)
399-407: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the stale comment about the
runBuild()fallback.The comment states that
readManifestSummaryfalls back torunBuild()when the prebuilt file does not exist yet.manifest.tsno longer does that. WhenprebuiltOutFileis set but missing, it returns the empty summary and never callsrunBuild(). Only theundefinedcase reaches the build path.📝 Proposed fix
// Pre-resolved outFile for the HMR fast path. The path is // deterministic per cwd (defaults from `BUILD_DEFAULTS`), so we // compute it once at app build time rather than on every request. - // Only used when HMR is enabled; `readManifestSummary` falls - // back to `runBuild()` when this is undefined or the file doesn't - // exist yet (fresh scaffold pre-watcher-bootstrap). + // Only used when HMR is enabled. `readManifestSummary` falls back + // to `runBuild()` only when this is undefined; when it is set but + // the artefact is missing (fresh scaffold pre-watcher-bootstrap) + // the empty summary is returned instead, so the watcher stays the + // sole writer of that path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/studio/server.ts` around lines 399 - 407, Update the comment above hmrOutFile to state that readManifestSummary uses the build path only when prebuiltOutFile is undefined; when the configured prebuilt file is missing, it returns an empty summary without calling runBuild().
1274-1281: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRemove a dead SSE listener when its enqueue fails.
Both catch blocks swallow the failure and rely on
cancel()to remove the listener fromsseListeners. That guarantee only holds when the stream'scancelalgorithm actually runs. If the underlying response is torn down without invokingcancel(an aborted socket, a controller already errored), the listener stays in the Set forever. Every later rebuild then pays aJSON.stringifyplus a throwingenqueueper dead listener, and the Set grows with each SPA reconnect over a longarkor devsession.Delete the listener at the point of failure.
cancel()stays as the normal path andSet.deleteis idempotent.♻️ Proposed fix
const send = (event: AugmentedEvent): void => { const payload = JSON.stringify(event); try { controller.enqueue( enc.encode(`event: ${event.type}\ndata: ${payload}\n\n`), ); } catch { - // controller closed mid-write; cancel() removes us. + // Controller closed mid-write. Self-evict rather than + // waiting for `cancel()`: an aborted socket can tear the + // response down without running the cancel algorithm, and + // a retained dead listener is re-encoded on every rebuild. + sseListeners.delete(send); } };Also applies to: 1289-1301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/studio/server.ts` around lines 1274 - 1281, Update both SSE listener catch blocks in the listener notification loops to delete the failing listener from sseListeners immediately after enqueue failure. Keep the existing cancel-based cleanup unchanged, relying on Set.delete idempotency.
1213-1232: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle late
Started joblines in the win32 HMR restart cancel.
recordJobId()still leavesentry.jobIdnull until the stdout parser emits it, while manual Stop pollsparsedJobIdfor 500 ms. On win32, HMR SIGTERM terminates the child before the runner can cancel on its own, so a rebuild occurring aftercreateJobbut before the parsed marker can skip the cloud cancel and orphan the job. Add the same short deferred read here before proceeding with the cancel POST.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/studio/server.ts` around lines 1213 - 1232, The win32 HMR restart path can miss cancellation when `entry.jobId` is not yet populated by the stdout parser. Before the cancel POST in the `restartTargets` loop, add the same short deferred `parsedJobId` polling used by manual Stop, then use the resolved job ID and scope for cancellation while preserving the existing skip behavior when no identifiers become available.packages/arkor/src/core/trainer.test.ts (1)
2398-2581: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a concurrency test for checkpoint-vs-timeout cancel overlap.
This range covers timeout-fallback cancellation tests, but none of the described scenarios exercise a
checkpoint.savedevent landing while the timeout-triggeredtrainer.cancel()is still in flight (or vice versa). Given the race identified incore/trainer.ts(checkpoint branch and timeout fallback can both calltrainer.cancel()concurrently), add a regression test that armsrequestEarlyStop()with a shorttimeoutMs, delays the mockedcancelJobresponse so it overlaps with acheckpoint.savedevent, and assertscancelJobis invoked at most once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/core/trainer.test.ts` around lines 2398 - 2581, Add a regression test covering overlap between the checkpoint branch and timeout fallback in requestTrainerEarlyStop: delay the mocked cancel response, emit a checkpoint.saved event while cancellation is in flight, and assert the cancel endpoint is invoked at most once. Reuse the existing trainer setup and fetcher patterns in the timeout tests, ensuring the short timeout reliably races with the checkpoint event.packages/arkor/src/core/trainer.ts (1)
396-528: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftFix the race between checkpoint-triggered cancel and the timeout fallback cancel.
The checkpoint branch in
dispatch()(Line 421) and the timeout fallback inrequestEarlyStop()(Line 828) both guard onearlyStopRequested && earlyStopDeferred(or the capturedactive), but neither clears the shared timer, nor marks a cancel as in-flight, before callingtrainer.cancel(). If acheckpoint.savedevent arrives close to the deadline, both paths pass their guard and issuetrainer.cancel()concurrently.Two consequences:
- The cloud API receives two concurrent
cancelJobcalls, andstartedJobgets overwritten twice from two independent branches.- If the timeout branch's
.finally()(Line 899) resolves first and setsearlyStopDeferred = null, the checkpoint branch'scancelFailedpath (Line 489) later dereferencesearlyStopDeferred.timeron a now-null value. This throws aTypeErrorinsidedispatch(), which propagates as await()rejection unrelated to the real cancel outcome.The success path (
settleEarlyStopLatch(), Line 503) already null-checksearlyStopDeferredbefore use. The failure path (Lines 483-501) does not. The timeout callback already capturesearlyStopDeferredinto a localactivereference before its own await (Line 833) and never re-reads the mutable field afterward; the checkpoint branch should follow the same pattern.Clear the timer synchronously before starting the checkpoint-triggered cancel, so the timeout can never fire concurrently with it, and capture the deferred into a local variable before any
awaitso a concurrent settlement elsewhere cannot leave a stale reference:🔒 Proposed fix
if (earlyStopRequested && earlyStopDeferred) { + // Capture and disarm the shared timer synchronously, before any + // await, so the timeout fallback in requestEarlyStop() can never + // fire concurrently with this checkpoint-triggered cancel. + const activeDeferred = earlyStopDeferred; + if (activeDeferred.timer) clearTimeout(activeDeferred.timer); let cancelError: unknown = null; let cancelFailed = false; try { await trainer.cancel(); } catch (err) { cancelError = err; cancelFailed = true; } startedJob = { ...startedJob, status: cancelFailed ? "failed" : "cancelled", ...(cancelFailed && { error: `Early-stop cancel failed: ${ cancelError instanceof Error ? cancelError.message : String(cancelError) }`, }), completedAt: event.timestamp, }; if (cancelFailed) { - if (earlyStopDeferred.timer) clearTimeout(earlyStopDeferred.timer); - earlyStopDeferred.reject( + activeDeferred.reject( cancelError instanceof Error ? cancelError : new Error(String(cancelError)), ); - earlyStopDeferred = null; + if (earlyStopDeferred === activeDeferred) earlyStopDeferred = null; earlyStopRequested = false; } else { - settleEarlyStopLatch(); + if (earlyStopDeferred === activeDeferred) settleEarlyStopLatch(); + else activeDeferred.resolve(); }This closes the crash but a caller-supplied short
timeoutMsracing a checkpoint arriving at almost the same instant can still land both branches inside theirtrainer.cancel()await before either clears the timer. Confirm whethertrainer.cancel()(Lines 776-780, delegating toclient.cancelJob) is safe to call twice concurrently against the cloud API; if not, consider sharing a singlecancelInFlightpromise between the checkpoint branch and the timeout fallback, mirroring the existingstartInFlightpattern used instart().Also applies to: 783-930
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/core/trainer.ts` around lines 396 - 528, Make early-stop cancellation mutually exclusive between checkpoint handling in dispatch and the timeout fallback in requestEarlyStop. Capture earlyStopDeferred locally and clear its timer synchronously before awaiting trainer.cancel(), then use the captured reference for settlement or rejection without rereading the shared field. Add or reuse a shared cancelInFlight promise so both paths await one trainer.cancel() request rather than issuing concurrent cloud cancellations, and preserve the existing terminal status and error propagation behavior.packages/arkor/src/studio/manifest.ts (1)
157-165: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTolerate a lost snapshot-write race instead of failing the request.
Two concurrent requests with the same digest can both pass
existsSync(contentFile) === falseand both attemptrename(tmp, contentFile). On POSIX the second rename silently replaces an identical file, so the outcome is benign. On Windows,fs.renameover a destination that another process (or this process's ESM loader) currently holds open can fail withEPERM/EBUSY, and the error propagates out ofsummariseBuiltManifest./api/manifestthen answers 400 for that poll even though a valid snapshot exists on disk.Treat "destination already present with the correct digest" as success.
🛡️ Proposed fix
const tmp = `${contentFile}.${process.pid}.${randomUUID()}.tmp`; try { await writeFile(tmp, bytes); await rename(tmp, contentFile); } catch (err) { await rm(tmp, { force: true }).catch(() => undefined); - throw err; + // A concurrent request with the SAME digest may have published + // the snapshot first; on Windows the losing rename can fail with + // EPERM/EBUSY against the already-open destination. The bytes are + // identical by construction, so treat that as success. + if (!existsSync(contentFile)) throw err; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/studio/manifest.ts` around lines 157 - 165, Update the snapshot write flow around the temporary-file write and rename so EPERM/EBUSY caused by a concurrent destination race is tolerated when contentFile now exists with the expected digest. After such a rename failure, verify the existing destination’s digest matches the requested content and treat it as success; otherwise preserve the current cleanup and error propagation behavior.
♻️ Duplicate comments (1)
packages/arkor/src/cli/commands/dev.ts (1)
305-329: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClose the restore-path race with an atomic create-if-absent.
This branch still probes with
existsSync(path)and then callsrenameSync(reapPath, path).renamereplaces an existing destination rather than failing. If anotherarkor devpublishes a fresh token between the probe and the rename, this shutdown overwrites that newer token with the older claimed copy, and the sibling Studio serves 403s to the Vite SPA workflow until its next rewrite. The comment at Lines 305-315 acknowledges the window and accepts it.The window is avoidable at no real cost.
linkSyncandcopyFileSync(..., COPYFILE_EXCL)both fail withEEXISTwhen the destination already exists, which turns the probe and the publish into one atomic step. Discard the claimed copy onEEXIST.🔒️ Proposed fix
-import { existsSync, readFileSync, renameSync, unlinkSync } from "node:fs"; +import { + constants as fsConstants, + copyFileSync, + readFileSync, + renameSync, + unlinkSync, +} from "node:fs";- // Foreign token: restore it, UNLESS the other process already - // re-wrote `path` after our rename claimed the old copy. Rename - // REPLACES an existing destination rather than failing, so a - // bare rename-back would clobber that fresher token with the - // older claimed one. The existence probe shrinks the clobber - // window from "read → unlink" (previous design, milliseconds - // spanning a token comparison) to the few instructions between - // existsSync and renameSync, and the losing outcome in that - // residual window is restore-the-older-token (the other dev - // server 403s until its own next rewrite), not delete-the-token - // outright. - try { - if (existsSync(path)) { - // Newest writer wins: discard the claimed older copy. - unlinkSync(reapPath); - } else { - renameSync(reapPath, path); - } - } catch { - try { - unlinkSync(reapPath); - } catch { - // best-effort - } - } + // Foreign token: restore it, UNLESS the other process already + // re-wrote `path` after our rename claimed the old copy. Rename + // REPLACES an existing destination rather than failing, so a + // bare rename-back would clobber that fresher token with the + // older claimed one. `COPYFILE_EXCL` makes the "publish only if + // absent" decision atomic in the kernel: no existsSync probe, no + // residual window. EEXIST means the other process won the race, + // so its newer token stays and we drop the claimed older copy. + try { + copyFileSync(reapPath, path, fsConstants.COPYFILE_EXCL); + } catch { + // EEXIST (newest writer already republished) or any other + // failure: nothing to restore. + } + try { + unlinkSync(reapPath); + } catch { + // best-effort + }As per coding guidelines: "clean it up using atomic claim-and-verify logic so shutdown cannot delete another session's token".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/cli/commands/dev.ts` around lines 305 - 329, Update the foreign-token restore branch around existsSync/renameSync to use an atomic create-if-absent operation, such as linkSync or copyFileSync with exclusive creation, instead of probing and then replacing the destination. Treat EEXIST as the newer-token-wins case and discard reapPath; preserve best-effort cleanup for other failures so shutdown cannot overwrite another session’s token.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/arkor/src/cli/commands/dev.test.ts`:
- Around line 833-839: Update the affected test declaration to use
it.skipIf(process.platform === "win32") with the existing test name and body,
and remove the early platform-based return. Preserve the explanatory comment so
the test runner reports Windows as skipped with the reason visible in CI.
---
Outside diff comments:
In `@packages/arkor/src/core/trainer.test.ts`:
- Around line 2398-2581: Add a regression test covering overlap between the
checkpoint branch and timeout fallback in requestTrainerEarlyStop: delay the
mocked cancel response, emit a checkpoint.saved event while cancellation is in
flight, and assert the cancel endpoint is invoked at most once. Reuse the
existing trainer setup and fetcher patterns in the timeout tests, ensuring the
short timeout reliably races with the checkpoint event.
In `@packages/arkor/src/core/trainer.ts`:
- Around line 396-528: Make early-stop cancellation mutually exclusive between
checkpoint handling in dispatch and the timeout fallback in requestEarlyStop.
Capture earlyStopDeferred locally and clear its timer synchronously before
awaiting trainer.cancel(), then use the captured reference for settlement or
rejection without rereading the shared field. Add or reuse a shared
cancelInFlight promise so both paths await one trainer.cancel() request rather
than issuing concurrent cloud cancellations, and preserve the existing terminal
status and error propagation behavior.
In `@packages/arkor/src/studio/manifest.ts`:
- Around line 157-165: Update the snapshot write flow around the temporary-file
write and rename so EPERM/EBUSY caused by a concurrent destination race is
tolerated when contentFile now exists with the expected digest. After such a
rename failure, verify the existing destination’s digest matches the requested
content and treat it as success; otherwise preserve the current cleanup and
error propagation behavior.
In `@packages/arkor/src/studio/server.ts`:
- Around line 399-407: Update the comment above hmrOutFile to state that
readManifestSummary uses the build path only when prebuiltOutFile is undefined;
when the configured prebuilt file is missing, it returns an empty summary
without calling runBuild().
- Around line 1274-1281: Update both SSE listener catch blocks in the listener
notification loops to delete the failing listener from sseListeners immediately
after enqueue failure. Keep the existing cancel-based cleanup unchanged, relying
on Set.delete idempotency.
- Around line 1213-1232: The win32 HMR restart path can miss cancellation when
`entry.jobId` is not yet populated by the stdout parser. Before the cancel POST
in the `restartTargets` loop, add the same short deferred `parsedJobId` polling
used by manual Stop, then use the resolved job ID and scope for cancellation
while preserving the existing skip behavior when no identifiers become
available.
---
Duplicate comments:
In `@packages/arkor/src/cli/commands/dev.ts`:
- Around line 305-329: Update the foreign-token restore branch around
existsSync/renameSync to use an atomic create-if-absent operation, such as
linkSync or copyFileSync with exclusive creation, instead of probing and then
replacing the destination. Treat EEXIST as the newer-token-wins case and discard
reapPath; preserve best-effort cleanup for other failures so shutdown cannot
overwrite another session’s token.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f98f378a-4cd3-4066-9672-6bf3690d04cc
📒 Files selected for processing (11)
AGENTS.mdpackages/arkor/package.jsonpackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/studio/manifest.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.tspackages/studio-app/src/lib/api.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (198)
- GitHub Check: Seer Code Review
- GitHub Check: install · pnpm-9 · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · macos-latest · node 24.0.0
- GitHub Check: install · yarn-berry · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-9 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-10 · macos-latest · node 22.22.0
- GitHub Check: install · npm · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=26.0.0 <27
- GitHub Check: install · bun · macos-latest · node 22.22.0
- GitHub Check: install · yarn-berry · macos-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn-berry · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node 24.12.0
- GitHub Check: install · yarn · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node >=24.12.0 <25
- GitHub Check: install · npm · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-9 · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node >=22.22.0 <23
- GitHub Check: install · bun · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-10 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node 22.22.0
- GitHub Check: install · bun · macos-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-10 · macos-latest · node 26.0.0
- GitHub Check: install · bun · macos-latest · node 26.0.0
- GitHub Check: install · pnpm-9 · macos-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · macos-latest · node 24.0.0
- GitHub Check: install · bun · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · macos-latest · node 24.12.0
- GitHub Check: install · yarn · macos-latest · node >=22.22.0 <23
- GitHub Check: install · npm · macos-latest · node 22.22.0
- GitHub Check: install · yarn · macos-latest · node 24.0.0
- GitHub Check: install · pnpm-9 · macos-latest · node 24.12.0
- GitHub Check: install · yarn-berry · macos-latest · node 24.0.0
- GitHub Check: install · yarn · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · macos-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · macos-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node 26.0.0
- GitHub Check: install · yarn · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-10 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-9 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn-berry · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · bun · windows-latest · node >=26.0.0 <27
- GitHub Check: install · bun · windows-latest · node 24.12.0
- GitHub Check: install · yarn-berry · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · windows-latest · node 26.0.0
- GitHub Check: install · npm · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 26.0.0
- GitHub Check: install · npm · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 26.0.0
- GitHub Check: install · npm · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.12.0
- GitHub Check: install · yarn · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-11 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · bun · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 24.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node >=22.22.0 <23
- GitHub Check: install · bun · windows-latest · node 22.22.0
- GitHub Check: install · yarn-berry · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-10 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-9 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 22.22.0
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: typecheck · lint · test · build · windows-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=22.22.0 <23
- GitHub Check: coverage · upload to Codecov
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · macos-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.5.0 <24.8.0
- GitHub Check: coverage · upload to Codecov
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,ts,jsx,tsx,json,css,html}
📄 CodeRabbit inference engine (AGENTS.md)
Use oxfmt for formatting; do not manually override its whitespace, wrapping, quotes, or trailing-comma decisions. Keep package-key and import sorting disabled as configured.
Files:
packages/arkor/package.jsonpackages/arkor/src/studio/manifest.test.tspackages/arkor/src/core/trainer.tspackages/studio-app/src/lib/api.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain AGENTS.md as a living document with current agent status and architectural decisions
Files:
AGENTS.md
*.{yml,yaml,json,html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in YAML, Markdown, JSON, and HTML files.
Files:
AGENTS.md
packages/**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in comments, string literals, and template literals in any package file; this includes CLI runtime messages, generated-file template bodies, and test names.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/core/trainer.tspackages/studio-app/src/lib/api.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Do not use em dashes (U+2014) in comments, string literals, or template literals anywhere in the codebase; use colons, periods, commas, parentheses, spaced hyphens (
" - "), or rephrasing instead.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/core/trainer.tspackages/studio-app/src/lib/api.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve Studio security controls: send the studio token in the
X-Arkor-Studio-Tokenheader forfetch, usestudioTokenquery parameters only for approved GET-only SSE routes, retain the localhost host-header allow-list, omit CORS, and use timing-safe token comparison.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/core/trainer.tspackages/studio-app/src/lib/api.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/arkor/src/studio/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
packages/arkor/src/studio/**/*.ts: Only add routes toeventStreamPathPatternwhen they are GET-only streaming endpoints; never allow mutation endpoints to authenticate with query-string tokens.
For HMR rebuilds, use SIGUSR2 only when the spawn-time and rebuiltconfigHashvalues match; otherwise use the graceful SIGTERM early-stop/restart path. Do not replace HMR SIGTERM with SIGKILL.
Keep callback hot-swap and early-stop operations behind the documentedSymbol.forbrands; do not addreplaceCallbacks()orrequestEarlyStop()to the publicTrainerinterface.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/arkor/src/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
SDK, CLI, and scaffolder logic changes must include Vitest tests under
packages/*/src/**/*.test.ts; CLI flow changes should additionally consider ane2e/cliscenario.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/studio/server.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable unsafe automatic fixes for
unicorn/no-typeof-undefinedor@typescript-eslint/no-unnecessary-type-assertion; review their findings manually unless a safer upstream fix exists.
Files:
packages/arkor/src/studio/manifest.test.tspackages/arkor/src/core/trainer.tspackages/studio-app/src/lib/api.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/core/trainer.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.ts
packages/arkor/src/cli/commands/dev.ts
📄 CodeRabbit inference engine (AGENTS.md)
packages/arkor/src/cli/commands/dev.ts: Generate a fresh 32-byte base64url Studio token per launch, persist it with mode0600, allow persistence failure without blocking startup, and clean it up using atomic claim-and-verify logic so shutdown cannot delete another session's token.
Signal handlers for Studio shutdown must exit with conventional statuses: SIGINT 130, SIGTERM 143, and SIGHUP 129.
The manual Stop path may SIGKILL a child only after issuing its server-side cloud cancellation request; HMR restart paths must let the child perform graceful cancellation, with the documented Windows exception.
Files:
packages/arkor/src/cli/commands/dev.ts
🪛 ast-grep (0.45.0)
packages/arkor/src/studio/server.ts
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
AGENTS.md
[uncategorized] ~135-~135: The official name of this software platform is spelled with a capital “H”.
Context: ...TOCOL`. - Release SBOMs (build.yaml): each release sh...
(GITHUB)
[uncategorized] ~135-~135: The official name of this software platform is spelled with a capital “H”.
Context: ...ifact, and postflight in release.yaml attaches them t...
(GITHUB)
[uncategorized] ~135-~135: The official name of this software platform is spelled with a capital “H”.
Context: ...npm emits 2.3), so release-dry-run.yaml regener...
(GITHUB)
🔇 Additional comments (18)
AGENTS.md (3)
70-79: LGTM!
80-92: LGTM!
93-97: LGTM!packages/arkor/src/studio/manifest.ts (1)
195-214: LGTM!Also applies to: 251-277
packages/arkor/src/studio/manifest.test.ts (1)
41-76: LGTM!Also applies to: 84-113, 119-161, 167-210, 216-265
packages/arkor/src/studio/server.test.ts (1)
588-628: LGTM!Also applies to: 2986-3031
packages/arkor/src/studio/server.ts (1)
253-299: LGTM!Also applies to: 519-532, 717-736, 1865-1909
packages/arkor/src/cli/commands/dev.test.ts (2)
696-725: LGTM!Also applies to: 939-971, 973-1035
845-865: 🩺 Stability & AvailabilityDirectory-permission restore is already unconditional.
The
chmodSync(dirname(path), 0o755)restore runs in thefinallyblock for the read-only-directory test setup.packages/arkor/src/cli/commands/dev.ts (1)
167-204: LGTM!Also applies to: 274-304, 386-493
packages/studio-app/src/lib/api.ts (2)
3-9: LGTM!Also applies to: 439-439, 462-464
151-155: 🔒 Security & PrivacyNo change needed for token placement.
withStudioToken()is restricted toopenJobEvents()andopenDevEvents(), bothEventSourceconsumers for GET-only SSE endpoints.packages/arkor/package.json (1)
58-61: 🔒 Security & PrivacyDependency swap looks correct.
Rolldown's latest published version is 1.1.1, so pinning
^1.1.1targets a current, valid release. No known vulnerabilities were found for the package at this version.packages/arkor/src/core/trainer.ts (3)
538-567: LGTM! Terminal completion and failure branches settle the latch safely.Both
try/finallyblocks callsettleEarlyStopLatch(), which is null-safe, so a throwingonCompleted/onFailedcannot strand the SIGTERM handler.
570-581: LGTM!Also applies to: 589-628
631-774: LGTM! Reconnect/error classification changes are consistent with the stated ENG-933 intent.The permanent-4xx short-circuit, ping/malformed-frame exclusion from the reset counter, and
FatalStreamErrorsentinel for dispatch failures are internally consistent and match the accompanying tests.packages/arkor/src/core/trainer.test.ts (2)
2844-3245: LGTM!
4-12: LGTM!Also applies to: 205-303
| // whoever just registered. One bad subscriber must not be able | ||
| // to break HMR initialisation for the rest of the process. | ||
| if (lastEvent) { | ||
| try { | ||
| fn(lastEvent); | ||
| } catch { | ||
| // Swallow: subscribers own their own teardown; we just | ||
| // shouldn't poison their `subscribe()` call site. | ||
| } | ||
| } | ||
| startWatcher(); |
There was a problem hiding this comment.
Bug: When multiple subscribers join while an HMR entry file is missing, new subscribers receive the same 'error' event twice due to re-broadcasting in startWatcher.
Severity: LOW
Suggested Fix
Modify the startWatcher function to prevent it from re-broadcasting an error if one has already been sent. This can be achieved by setting a state flag or modifying the if (watcher || disposed) guard to account for the 'waiting for entry' state, ensuring the error broadcast logic is only executed once for the initial missing entry condition.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/arkor/src/studio/hmr.ts#L591-L613
Potential issue: When multiple subscribers connect to the HMR service while the entry
file is missing, they will receive duplicate 'error' events. The first subscriber
triggers an error broadcast and caches the event. A subsequent subscriber receives this
cached event and then re-triggers the `startWatcher` function. Because the `watcher`
instance is not created when the entry is missing, the logic re-enters the error path
and broadcasts the same error again to all subscribers. This results in redundant SSE
messages being sent to the client, causing minor performance overhead and potentially
duplicate UI updates.
There was a problem hiding this comment.
🟡 Not ready to approve
Startup cleanup, unbranded cancellation, and Windows restart paths contain blocking correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/arkor/src/core/trainerInspection.ts:203
- Swallowing
trainer.cancel()failures makes the unbranded HMR fallback unsafe. Unbranded trainers always haveconfigHash: null, so a rebuild sends SIGTERM; if their cancel rejects, this helper still resolves,installShutdownHandlersexits 0, and the SPA auto-restarts even though the original cloud job may still be running. Let the rejection reach the existing shutdown-handler catch so it emits a nonzero exit and suppresses the replacement run; update the fallback tests accordingly.
packages/arkor/src/studio/trainRegistry.ts:466 - The Windows fallback cannot complete the advertised auto-restart flow. On win32 this SIGTERM is an abrupt
TerminateProcess, so the child closes with a null/nonzero code;streamTrainingreturns that status andRunTrainingexplicitly suppresses every restart unlessexitCode === 0. Thus callback and config edits kill the run but never respawn it on Windows. The server needs a distinct, cancel-confirmed restart outcome (rather than the raw child exit code) that the SPA can safely recognize.
packages/arkor/src/cli/commands/dev.ts:492 - This pre-bind rejection leaves the HMR coordinator alive.
buildStudioApp({ hmr })subscribes immediately, so the Rolldown watcher starts beforeserve()binds; because cleanup hooks are only registered after a successful bind,EADDRINUSEcan retain native watcher handles and keep the failedarkor devprocess running. Disposehmrbefore rejecting startup.
reject(err instanceof Error ? err : new Error(message));
- Files reviewed: 46/47 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a58f8ad927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // exit-owning token hook below stays outermost; if the process | ||
| // dies before this callback runs, the un-disposed watcher is | ||
| // reaped by process death anyway. | ||
| scheduleHmrCleanup(hmr); |
There was a problem hiding this comment.
When src/arkor/index.ts exists and serve() reports EADDRINUSE, buildStudioApp() has already subscribed to the coordinator and started its long-lived Rolldown watcher, but this cleanup is registered only inside the successful-listening callback. runDev() therefore rejects while the watcher keeps the event loop alive; bin.ts merely sets process.exitCode, so a second arkor dev prints the port error but does not terminate. Dispose the coordinator on the pre-bind error path.
AGENTS.md reference: AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
| // handler. Swallow it; the close handler above has already | ||
| // taken the entry out of the registry. | ||
| try { | ||
| child.kill("SIGKILL"); |
There was a problem hiding this comment.
When Stop lands after the cloud has accepted createJob but before the runner has printed its job-id marker, the polling IIFE reaches its first 25 ms await and execution immediately continues here to SIGKILL the child. The runner can no longer finish start() or emit the marker, so parsedJobId remains null and the cloud cancel POST is skipped, potentially leaving the paid job running. Fresh evidence after the earlier parser fix is that stdout parsing now remains attached, but this immediate kill still prevents the awaited late marker from being produced; delay the kill through the polling/cancel initiation window.
AGENTS.md reference: AGENTS.md:L91-L91
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| } | ||
| replaceTrainerCallbacks(trainer, inspection.callbacks); |
There was a problem hiding this comment.
On a callback-only rebuild, inspection.callbacks comes from a newly imported module and therefore closes over that module's newly constructed trainer-local state, while this call installs those closures onto the old live trainer. For example, the documented early-stopping shape creates an AbortController beside createTrainer; after hot-swap, controller.abort() targets the new signal rather than the signal the live wait() is using, and a callback closing over the newly imported trainer.cancel() similarly calls an unstarted trainer and no-ops. The threshold callback can then fail to stop the live run and leave GPU spend continuing, so the hot-swap must preserve/rebind associated control state or conservatively restart these callbacks instead.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
| * path. | ||
| */ | ||
| async function inspectBundle(outFile: string): Promise<InspectionResult> { | ||
| const mod = (await import(moduleCacheBustUrl(outFile))) as Record< |
There was a problem hiding this comment.
When a bundle's top-level evaluation fails because of an external runtime condition, this content-derived URL is cached by Node as a rejected module record. If the condition is then fixed and a later watcher build emits identical bundle bytes, moduleCacheBustUrl() returns the same URL and inspectBundle() receives the cached rejection without evaluating the now-runnable artifact, leaving the coordinator and Run button stuck in the HMR error state until the source changes enough to alter bundle bytes or arkor dev restarts. The manifest importer already adds a retry salt for this exact ESM behavior; the HMR importer needs the same failure-aware retry.
AGENTS.md reference: AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
5 issues found across 47 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/arkor/src/studio/server.test.ts">
<violation number="1" location="packages/arkor/src/studio/server.test.ts:157">
P3: The inline fake HMR coordinator is copy-pasted across several new tests in this file even though the `/api/dev/events` describe block already defines a richer, reusable `fakeHmr()` factory. Extracting a shared helper (e.g. hoist `fakeHmr` to the file or describe level, or add a minimal `stubHmr()` above the first use) and reusing it here would remove ~5 duplicate six-method object literals and keep future changes to the coordinator surface in one place.</violation>
</file>
<file name="packages/arkor/src/core/runnerSignals.ts">
<violation number="1" location="packages/arkor/src/core/runnerSignals.ts:83">
P1: HMR restarts of hand-rolled trainers can report success and start a replacement after `cancel()` fails, leaving the original cloud run active. Preserve the fallback cancellation rejection so this handler takes its nonzero failure path instead of swallowing it.</violation>
</file>
<file name="packages/arkor/src/core/trainer.ts">
<violation number="1" location="packages/arkor/src/core/trainer.ts:807">
P2: A first SIGTERM can now wait indefinitely when credential/project-state resolution or job creation never settles. Bound the `startInFlight` wait or otherwise let shutdown progress after a deadline, since the signal handler cannot reach its early-stop timeout until this await returns.</violation>
</file>
<file name="packages/studio-app/src/lib/api.ts">
<violation number="1" location="packages/studio-app/src/lib/api.ts:461">
P3: Long subprocess spawn errors are classified as an unknown/truncated exit instead of a spawn failure because the 4 KiB tail can drop the error-marker prefix. Preserve marker-detection state while streaming (or use a bounded server-side error payload) so this path still rejects with the actual failure.</violation>
</file>
<file name="packages/arkor/src/core/runner.ts">
<violation number="1" location="packages/arkor/src/core/runner.ts:155">
P2: A SIGUSR2 reload already awaiting its dynamic import can still evaluate user code after this run has finished, because removing the listener does not cancel or invalidate that in-flight reload. Invalidate pending reloads in the disposer (for example, advance `loadSeq`) so completed runs do not execute a late HMR import or log that training continues.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| // when `arkor dev` wired in an HMR coordinator. Verify it lands | ||
| // in `<head>` next to the studio-token tag. | ||
| const fakeHmr = { | ||
| subscribe: () => () => undefined, |
There was a problem hiding this comment.
P3: The inline fake HMR coordinator is copy-pasted across several new tests in this file even though the /api/dev/events describe block already defines a richer, reusable fakeHmr() factory. Extracting a shared helper (e.g. hoist fakeHmr to the file or describe level, or add a minimal stubHmr() above the first use) and reusing it here would remove ~5 duplicate six-method object literals and keep future changes to the coordinator surface in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/arkor/src/studio/server.test.ts, line 157:
<comment>The inline fake HMR coordinator is copy-pasted across several new tests in this file even though the `/api/dev/events` describe block already defines a richer, reusable `fakeHmr()` factory. Extracting a shared helper (e.g. hoist `fakeHmr` to the file or describe level, or add a minimal `stubHmr()` above the first use) and reusing it here would remove ~5 duplicate six-method object literals and keep future changes to the coordinator surface in one place.</comment>
<file context>
@@ -137,6 +138,46 @@ describe("Studio server", () => {
+ // when `arkor dev` wired in an HMR coordinator. Verify it lands
+ // in `<head>` next to the studio-token tag.
+ const fakeHmr = {
+ subscribe: () => () => undefined,
+ getCurrentConfigHash: () => null,
+ getCurrentArtifactHash: () => null,
</file context>
| // silently downgrade the failure to a `null` resolve. Bounded so | ||
| // a multi-MB log can't grow this string. | ||
| let tailBuf = ""; | ||
| const TAIL_MAX = 4096; |
There was a problem hiding this comment.
P3: Long subprocess spawn errors are classified as an unknown/truncated exit instead of a spawn failure because the 4 KiB tail can drop the error-marker prefix. Preserve marker-detection state while streaming (or use a bounded server-side error payload) so this path still rejects with the actual failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/studio-app/src/lib/api.ts, line 461:
<comment>Long subprocess spawn errors are classified as an unknown/truncated exit instead of a spawn failure because the 4 KiB tail can drop the error-marker prefix. Preserve marker-detection state while streaming (or use a bounded server-side error payload) so this path still rejects with the actual failure.</comment>
<file context>
@@ -268,28 +446,65 @@ export async function streamTraining(
+ // silently downgrade the failure to a `null` resolve. Bounded so
+ // a multi-MB log can't grow this string.
+ let tailBuf = "";
+ const TAIL_MAX = 4096;
+ const trackTail = (text: string): void => {
+ tailBuf = (tailBuf + text).slice(-TAIL_MAX);
</file context>
… restart gating) Manual Stop / cancel path: - move the SIGKILL after the job-id marker poll (was before it, which guaranteed the marker never arrived for a Stop landing inside trainer.start()) and widen the window to 2s with an early exit once the child is provably dead - win32 HMR restart cancels now resolve a null spawn scope from .arkor/state.json at cancel time (first-run anon parity with the manual-Stop path) Hot-swap safety: - gate SIGUSR2 dispatch on the recorded Started-job marker: SIGUSR2's default disposition kills a child whose handler isn't armed yet, and dispatch used to report that as hotSwapTargets success - win32 restart targets carry forcedKill: true and the SPA lets that latch through its nonzero-exit suppression (config rebuilds on Windows killed + cancelled the old run and never respawned) - document the closure-rebinding caveat of callback hot-swap and the win32 restart fallback (docs EN/JA + AGENTS.md) Restart gating (SPA): - a latched restart is deferred while the build is broken and released by the next successful rebuild, instead of respawning the last-good artifact under a live manifest error - pre-spawn event buffer survives an error frame (same rationale as the latch itself) configHash JSON parity (both misroute config changes to hot-swap): - functions with toJSON serialise via toJSON (JSON.stringify does) - a toJSON result is serialised structurally, without re-applying its own toJSON Other: - reject POST /api/train body.file with 400 while HMR owns the build artifact (a custom-entry rebuild persisted over the shared index.mjs until the next source edit) - dispose the HMR watcher on pre-bind serve errors so an EADDRINUSE launch actually exits - propagate unbranded trainer cancel() rejections so a failed cancel exits nonzero and suppresses auto-restart - early-stop timeout re-checks the latch/terminal state and both cancel branches share a single-cancel guard - retry-salt HMR bundle imports after a cached failed evaluation; broadcast the missing-entry error once per wait period - inlineDynamicImports on both build outputs (single-artifact contract; relative dynamic imports split into orphaned chunks) - truncate the error= marker payload to 2KB; keep marker detectable in the SPA's bounded tail - linkSync-based no-clobber restore for the studio-token reap; runnerSignals disposer invalidates in-flight reloads; manifest degrades unhashable configs to configHash null - test robustness: bounded polls replace fixed sleeps (server cancel tests, runnerSignals stale-reload), it.skipIf for the win32-skipped persist test, descriptive e2e SSE timeout errors - drop dead surface: HmrCoordinator.getCurrentArtifactHash, registry isEarlyStopRequested getter
| while (parsedJobId === null && Date.now() - start < 2000) { | ||
| // Stop waiting the moment the child is provably dead: | ||
| // its stdout is closed, so no marker can ever arrive | ||
| // and every further tick only delays the kill below. | ||
| if (child.exitCode !== null || child.signalCode !== null) { | ||
| break; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 25)); | ||
| } |
There was a problem hiding this comment.
When trainer.start() takes longer than this 2s poll, parsedJobId stays null, the child is killed, and the null-job-id guard skips the cloud cancel POST. The runner only prints the nonce-prefixed Started job marker after trainer.start() resolves, so a slow create-job or first-run setup can still leave the remote training job running after the local stream stops. This path needs a pending cancel that fires when the marker arrives, or it needs to avoid killing the child until the job can be cancelled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/arkor/src/studio/server.ts
Line: 1059-1067
Comment:
**Stop Can Still Orphan Jobs**
When `trainer.start()` takes longer than this 2s poll, `parsedJobId` stays null, the child is killed, and the null-job-id guard skips the cloud cancel POST. The runner only prints the nonce-prefixed `Started job` marker after `trainer.start()` resolves, so a slow create-job or first-run setup can still leave the remote training job running after the local stream stops. This path needs a pending cancel that fires when the marker arrives, or it needs to avoid killing the child until the job can be cancelled.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| // addressable to cancel. The narrow marker-less window | ||
| // shares the manual-Stop path's accepted residual: the | ||
| // cloud reaper / TTL is the safety net. | ||
| if (!snap?.jobId) continue; |
There was a problem hiding this comment.
On Windows, a rebuild can arrive after the cloud job exists but before stdout parsing records entry.jobId. The snapshot then has no job id, this branch skips the server-side cancel, and the following restart signal kills the child abruptly. Because the child cannot run its own early-stop cancel path on Windows, the old cloud job can keep running while the SPA starts the replacement run. This restart path needs late-marker or pending-cancel handling before killing the child.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/arkor/src/studio/server.ts
Line: 1257
Comment:
**Restart Skips Late Job Ids**
On Windows, a rebuild can arrive after the cloud job exists but before stdout parsing records `entry.jobId`. The snapshot then has no job id, this branch skips the server-side cancel, and the following restart signal kills the child abruptly. Because the child cannot run its own early-stop cancel path on Windows, the old cloud job can keep running while the SPA starts the replacement run. This restart path needs late-marker or pending-cancel handling before killing the child.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
🟡 Not ready to approve
Unresolved signal and snapshot races can prematurely exit, miss HMR recovery, or overlap cloud training jobs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/arkor/src/studio/hmr.ts:536
- A synchronous watcher-start failure is permanent in the real server.
buildStudioAppcreates one coordinator subscription, while later SSE clients subscribe only to the server’s local listener set, so the “later subscribe retries” path never occurs. A transient EMFILE/EACCES therefore leaves HMR disabled untilarkor devis restarted; schedule a bounded/repeating retry here, similar to missing-entry recovery.
- Files reviewed: 47/48 changed files
- Comments generated: 5
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if ( | ||
| !earlyStopRequested || | ||
| !earlyStopDeferred || | ||
| (startedJob && TERMINAL_STATUSES.has(startedJob.status)) | ||
| ) { | ||
| settleEarlyStopLatch(); | ||
| return; | ||
| } |
| const spawnArtifactContentHash: string | null = | ||
| options.hmr && !trainFile | ||
| ? options.hmr.getCurrentArtifactContentHash() | ||
| : null; |
| // Hash mismatch (or one side is null), or a hash match whose | ||
| // child isn't provably ready for SIGUSR2: graceful restart. | ||
| // (On win32 "graceful" is aspirational: the kill is abrupt, so | ||
| // flag the target for the SPA's suppression carve-out.) | ||
| const r = safeKill(entry.child, "SIGTERM"); |
| if ( | ||
| result && | ||
| typeof result === "object" && | ||
| "config" in result && | ||
| "name" in result && | ||
| "callbacks" in result | ||
| ) { | ||
| return result as TrainerInspection; | ||
| } |
| void (async () => { | ||
| try { | ||
| // A null spawn-time scope is the first-run anon flow: | ||
| // `ensureProjectState` writes `.arkor/state.json` from | ||
| // inside the child during `trainer.start()`, i.e. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/arkor/src/core/trainer.ts (1)
421-511: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCapture the deferred before the
cancel()await; the timer can null it mid-flight.Line 428 clears
earlyStopRequestedbeforeawait trainer.cancel(), but it does not clear the latch timer and it does not captureearlyStopDeferredin a local. IftimeoutMselapses while the cancel POST is in flight, the timer callback at Line 846 observesearlyStopRequested === false, callssettleEarlyStopLatch(), and setsearlyStopDeferred = null. When the cancel then rejects, Line 496 readsearlyStopDeferred.timeronnulland throws aTypeError. That throw escapesdispatch(), becomes aFatalStreamError, and rejectswait()for a run that was only being cancelled.The timeout path already avoids this by capturing
const active = earlyStopDeferredbefore its async work. Mirror that here, and clear the timer at the same time you drop the request flag.🐛 Proposed fix
earlyStopRequested = false; + // Capture the deferred and disarm its timer BEFORE the async + // cancel: the timeout callback can otherwise settle and null + // the shared slot while the POST is in flight, leaving the + // branches below dereferencing null. + const active = earlyStopDeferred; + if (active.timer) clearTimeout(active.timer); @@ if (cancelFailed) { - if (earlyStopDeferred.timer) clearTimeout(earlyStopDeferred.timer); - earlyStopDeferred.reject( + active.reject( cancelError instanceof Error ? cancelError : new Error(String(cancelError)), ); - earlyStopDeferred = null; + if (earlyStopDeferred === active) earlyStopDeferred = null; earlyStopRequested = false; } else { + active.resolve(); + if (earlyStopDeferred === active) earlyStopDeferred = null; settleEarlyStopLatch(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/arkor/src/core/trainer.ts` around lines 421 - 511, In the early-stop branch around trainer.cancel(), capture earlyStopDeferred in a local active latch and clear its timer immediately when dropping earlyStopRequested, before awaiting cancel(). Use the captured latch for all subsequent reject or settle bookkeeping, so a timeout callback cannot null the shared earlyStopDeferred and cause a null dereference while cancellation is in flight; preserve the existing status and error propagation behavior.packages/studio-app/src/components/RunTraining.tsx (1)
476-489: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated hot-swap status flash.
Line 480-489 repeats the block at Line 345-358 byte for byte: set
hot-swapped, clear the previoushotSwapTimerRef, arm a new 1500 ms timer, and reset the status inside the timer. Two copies of a timer lifecycle can drift, and a drift here leaks a timer or pins the label. Extract one helper (for exampleflashHotSwapped()) and call it from both sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio-app/src/components/RunTraining.tsx` around lines 476 - 489, Extract the duplicated hot-swap status and timer lifecycle into a shared helper such as flashHotSwapped(), preserving the existing setHmrStatus, clearTimeout, 1500 ms reset, and ref-nullification behavior. Replace the inline block in the restart/hotSwap handling near hotSwapHit and the matching block near the earlier site with calls to that helper.
🤖 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 `@AGENTS.md`:
- Line 86: Update the SPA auto-restart contract described later in AGENTS.md to
include the forcedKill exception for win32 restart targets, allowing advertised
restarts despite nonzero exit suppression. Also document that latched restart
requests are deferred while the build is broken and released after the next
successful rebuild, keeping the existing suppression behavior for nonzero or
missing exit markers and rejected streams otherwise.
In `@docs/concepts/studio.mdx`:
- Line 19: The Windows fallback explanation is too broad about POSIX signals.
Update the sentence in docs/concepts/studio.mdx at lines 19-19 to state that
Node forcefully terminates for unknown POSIX signals such as SIGUSR2, and apply
the equivalent correction in docs/ja/concepts/studio.mdx at lines 19-19.
In `@packages/arkor/src/cli/commands/build.test.ts`:
- Around line 156-162: Update the relative dynamic-import assertion in the build
test to match every relative import with a literal “./” prefix, without
excluding helper imports. Preserve the existing inlined-content and artifact
checks while ensuring any remaining relative dynamic import, including
“./helper.mjs”, fails the test.
In `@packages/arkor/src/cli/commands/build.ts`:
- Around line 54-65: Update the bundle.write configuration in the build command
to replace the deprecated inlineDynamicImports option with output.codeSplitting:
false, preserving the single-artifact behavior expected by arkor start.
In `@packages/arkor/src/cli/commands/dev.test.ts`:
- Around line 834-837: Update the test’s it.skipIf predicate to also skip when
process.getuid exists and returns 0, preserving the typeof guard for win32
safety. Remove the in-body root check and early return so root containers are
reported as skipped rather than passed.
In `@packages/arkor/src/cli/commands/dev.ts`:
- Around line 322-361: Update the two surrounding comments for the restore logic
around linkSync, unlinkSync, and renameSync so they no longer describe
existsSync as the primary mechanism. State that the instruction-scale TOCTOU
residual applies only to filesystems using the no-hard-link fallback, and
describe the main path as atomic link-and-unlink restoration that preserves a
reappearing destination.
In `@packages/arkor/src/core/runnerSignals.ts`:
- Around line 273-279: Update the comment above the loadSeq increment to state
that it invalidates the pending reload result and suppresses the post-run
continuation log via the seq !== loadSeq check, but does not cancel or prevent
an already-started dynamic import or its module evaluation.
In `@packages/arkor/src/studio/hmr.ts`:
- Around line 515-528: Replace the deprecated inlineDynamicImports option with
codeSplitting: false in the watcher output configuration and the corresponding
runBuild() output configuration. Preserve the existing single-artifact behavior
and update related comments to describe disabling code splitting.
In `@packages/arkor/src/studio/server.ts`:
- Around line 561-579: Update the downstream hash-guard comments near configHash
and spawnArtifactContentHash to reflect that custom-entry runs are rejected
before reaching the spawn path, while retaining the guards as defensive checks
if desired. Remove the “Known residual wart” paragraph describing custom-entry
rebuilds overwriting the watcher-owned artifact, since this route now prevents
that behavior.
In `@packages/arkor/src/studio/trainRegistry.ts`:
- Around line 478-500: Deduplicate the platform-specific restart target
construction within the loop by defining one per-iteration helper or local value
for the target, applying forcedKill on win32 and preserving the original target
otherwise. Replace both restartTargets.push expressions in the
“gone”/“unsupported” and graceful-restart paths with that shared value so both
paths remain consistent.
In `@packages/studio-app/src/components/RunTraining.tsx`:
- Around line 584-591: Remove the repeated inline
`@typescript-eslint/no-unnecessary-condition` disable/enable directives around
forcedRestart and the other affected reads in RunTraining, then add one scoped
override for this file in the root eslint.config.ts. Include a comment
explaining that cross-closure writes to restartPendingRef and forcedKillLatchRef
are not recognized by TypeScript flow analysis, and preserve the existing rule
behavior elsewhere.
- Line 727: Move the runRef.current assignment out of the render body in
RunTraining and update it inside a dependency-free useEffect that tracks the
latest run callback. Keep the SSE effect’s use of runRef unchanged so it reads
the callback from the committed render.
In `@packages/studio-app/src/lib/api.ts`:
- Around line 212-221: Export the restart-target object shape in api.ts as a
named RestartTarget type, then change the restartTargets declaration to use it.
In RunTraining.tsx, import RestartTarget and replace the inline type on
restartHit with the shared type so both declarations stay synchronized.
---
Outside diff comments:
In `@packages/arkor/src/core/trainer.ts`:
- Around line 421-511: In the early-stop branch around trainer.cancel(), capture
earlyStopDeferred in a local active latch and clear its timer immediately when
dropping earlyStopRequested, before awaiting cancel(). Use the captured latch
for all subsequent reject or settle bookkeeping, so a timeout callback cannot
null the shared earlyStopDeferred and cause a null dereference while
cancellation is in flight; preserve the existing status and error propagation
behavior.
In `@packages/studio-app/src/components/RunTraining.tsx`:
- Around line 476-489: Extract the duplicated hot-swap status and timer
lifecycle into a shared helper such as flashHotSwapped(), preserving the
existing setHmrStatus, clearTimeout, 1500 ms reset, and ref-nullification
behavior. Replace the inline block in the restart/hotSwap handling near
hotSwapHit and the matching block near the earlier site with calls to that
helper.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6eb0f22e-42fb-4481-be27-0d712b378382
📒 Files selected for processing (26)
AGENTS.mddocs/concepts/studio.mdxdocs/ja/concepts/studio.mdxe2e/studio/src/specs/hmr.spec.tspackages/arkor/src/cli/commands/build.test.tspackages/arkor/src/cli/commands/build.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/core/configHash.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/hmr.test.tspackages/arkor/src/studio/hmr.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/server.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/studio-app/src/components/RunTraining.tsxpackages/studio-app/src/lib/api.ts
💤 Files with no reviewable changes (1)
- packages/arkor/src/studio/hmr.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (200)
- GitHub Check: Seer Code Review
- GitHub Check: install · pnpm-9 · macos-latest · node >=22.22.0 <23
- GitHub Check: install · npm · macos-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · windows-latest · node 26.0.0
- GitHub Check: install · yarn · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · macos-latest · node >=22.22.0 <23
- GitHub Check: install · npm · macos-latest · node 24.0.0
- GitHub Check: install · yarn-berry · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · macos-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn-berry · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn-berry · windows-latest · node 26.0.0
- GitHub Check: install · npm · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-9 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node 24.0.0
- GitHub Check: install · npm · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · windows-latest · node 24.12.0
- GitHub Check: install · bun · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · windows-latest · node >=24.12.0 <25
- GitHub Check: install · bun · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.12.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.0.0
- GitHub Check: install · npm · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node 22.22.0
- GitHub Check: install · yarn · windows-latest · node 22.22.0
- GitHub Check: install · yarn · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node 22.22.0
- GitHub Check: install · npm · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-9 · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-10 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · npm · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · macos-latest · node 26.0.0
- GitHub Check: install · bun · macos-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · macos-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-9 · macos-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · macos-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · macos-latest · node 24.0.0
- GitHub Check: install · yarn-berry · macos-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · macos-latest · node 24.0.0
- GitHub Check: install · bun · macos-latest · node 24.0.0
- GitHub Check: install · npm · macos-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · macos-latest · node 24.0.0
- GitHub Check: install · pnpm-9 · macos-latest · node 24.0.0
- GitHub Check: install · pnpm-10 · macos-latest · node >=22.22.0 <23
- GitHub Check: install · npm · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · macos-latest · node >=22.22.0 <23
- GitHub Check: install · bun · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn-berry · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 26.0.0
- GitHub Check: install · yarn · macos-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · pnpm-11 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · windows-latest · node >=24.12.0 <25
- GitHub Check: install · yarn-berry · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · yarn · windows-latest · node 26.0.0
- GitHub Check: install · bun · windows-latest · node 26.0.0
- GitHub Check: install · yarn · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-10 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-10 · windows-latest · node >=26.0.0 <27
- GitHub Check: install · npm · windows-latest · node 26.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node 26.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.12.0
- GitHub Check: install · bun · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.12.0
- GitHub Check: install · npm · windows-latest · node 24.12.0
- GitHub Check: install · bun · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node >=24.12.0 <25
- GitHub Check: install · pnpm-9 · windows-latest · node 24.12.0
- GitHub Check: install · bun · windows-latest · node 24.12.0
- GitHub Check: install · pnpm-10 · windows-latest · node 24.0.0
- GitHub Check: install · npm · windows-latest · node 24.0.0
- GitHub Check: install · yarn-berry · windows-latest · node 24.0.0
- GitHub Check: install · yarn · windows-latest · node 24.0.0
- GitHub Check: install · pnpm-11 · windows-latest · node 24.0.0
- GitHub Check: install · npm · windows-latest · node 22.22.0
- GitHub Check: install · yarn-berry · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-11 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-9 · windows-latest · node 24.0.0
- GitHub Check: install · bun · windows-latest · node >=22.22.0 <23
- GitHub Check: install · pnpm-10 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · npm · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node >=22.22.0 <23
- GitHub Check: install · yarn-berry · windows-latest · node 22.22.0
- GitHub Check: install · bun · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-10 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-9 · windows-latest · node 22.22.0
- GitHub Check: install · pnpm-11 · windows-latest · node 22.22.0
- GitHub Check: install · yarn · ubuntu-latest · node 24.12.0
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: typecheck · lint · test · build · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · windows-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=22.22.0 <23
- GitHub Check: coverage · upload to Codecov
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · macos-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · macos-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · macos-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node 22.22.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 24.12.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=24.12.0 <25
- GitHub Check: typecheck · lint · test · build · windows-latest · node 26.0.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=26.0.0 <27
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.1.0 <24.3.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.8.0 <24.10.0
- GitHub Check: typecheck · lint · test · build · windows-latest · node >=22.22.0 <23
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.10.0 <24.12.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.0.0 <24.1.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.5.0 <24.8.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node >=24.3.0 <24.5.0
- GitHub Check: typecheck · lint · test · build · ubuntu-latest · node 22.22.0
- GitHub Check: coverage · upload to Codecov
🧰 Additional context used
📓 Path-based instructions (17)
{README.md,README.ja.md,CONTRIBUTING.md,CONTRIBUTING.ja.md,docs/**/*.md,docs/**/*.mdx}
📄 CodeRabbit inference engine (AGENTS.md)
When editing English documentation, update its Japanese counterpart in the same change.
Files:
docs/concepts/studio.mdxdocs/ja/concepts/studio.mdx
docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.mdx: For CLI documentation CodeGroups, use the five package-manager forms exactly as specified and do not replace the npm tab withnpxornpm exec.
Verify Mintlify heading slugs before adding cross-page anchors; preserve punctuation behavior such as retained/,=, and full-width parentheses.
Files:
docs/concepts/studio.mdxdocs/ja/concepts/studio.mdx
packages/**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in comments, string literals, and template literals in any package file; this includes CLI runtime messages, generated-file template bodies, and test names.
Files:
packages/arkor/src/cli/commands/build.test.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/lib/api.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/cli/commands/build.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/studio/server.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Do not use em dashes (U+2014) in comments, string literals, or template literals anywhere in the codebase; use colons, periods, commas, parentheses, spaced hyphens (
" - "), or rephrasing instead.
Files:
packages/arkor/src/cli/commands/build.test.tse2e/studio/src/specs/hmr.spec.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/lib/api.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/cli/commands/build.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/studio/server.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
{oxfmt.config.ts,**/*.{js,ts,tsx,json,css,html}}
📄 CodeRabbit inference engine (AGENTS.md)
Use oxfmt for formatting, preserve its configured scope and exclusions, and do not let package-key or import sorting alter authored order.
Files:
packages/arkor/src/cli/commands/build.test.tse2e/studio/src/specs/hmr.spec.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/lib/api.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/cli/commands/build.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/studio/server.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
{oxlint.config.ts,eslint.config.ts,packages/**/*.{js,ts,tsx},e2e/**/*.{js,ts,tsx},examples/**/*.{js,ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use the single root oxlint and ESLint configurations; add scoped root overrides rather than per-package configs or repeated inline disables.
Files:
packages/arkor/src/cli/commands/build.test.tse2e/studio/src/specs/hmr.spec.tspackages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/configHash.tspackages/studio-app/src/lib/api.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/cli/commands/build.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/cli/commands/dev.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/studio/server.tspackages/studio-app/src/components/RunTraining.tsxpackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
packages/*/src/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Add Vitest tests for SDK, CLI, and scaffolder logic changes; consider an
e2e/cliscenario for CLI flow changes.
Files:
packages/arkor/src/cli/commands/build.test.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/cli/commands/dev.test.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/server.test.ts
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests must run against built CLI artifacts, use the documented workspace/build dependency flow, and preserve the install-matrix environment overrides and relative tarball behavior.
Files:
e2e/studio/src/specs/hmr.spec.ts
e2e/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use c8 for E2E child-process coverage; retain lcov for
e2e/cli, exclude its empty cobertura output from GitHub uploads, and use Playwright JUnit reporting for Studio E2E.
Files:
e2e/studio/src/specs/hmr.spec.ts
packages/arkor/src/{studio,core}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
For HMR subprocesses, use
configHashto choose callback hot-swap versus graceful SIGTERM restart; do not replace the HMR restart path with SIGKILL or widen SIGUSR2 indiscriminately.
Files:
packages/arkor/src/core/moduleCacheBust.tspackages/arkor/src/core/trainerInspection.test.tspackages/arkor/src/core/configHash.tspackages/arkor/src/core/runnerSignals.test.tspackages/arkor/src/core/trainer.tspackages/arkor/src/core/configHash.test.tspackages/arkor/src/core/trainerInspection.tspackages/arkor/src/studio/manifest.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/core/runnerSignals.tspackages/arkor/src/studio/server.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
packages/studio-app/**/*.{ts,tsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Studio token and HMR metadata injection dev-only; do not bake a per-launch token into Vite production builds. Gate
/api/dev/eventson the injected HMR metadata.
Files:
packages/studio-app/src/lib/api.tspackages/studio-app/src/components/RunTraining.hmr.test.tsxpackages/studio-app/src/components/RunTraining.tsx
packages/arkor/src/core/{trainerInspection.ts,trainer.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Keep
requestEarlyStopandreplaceCallbacksbehindSymbol.forbrands and out of the publicTrainerinterface.
Files:
packages/arkor/src/core/trainer.tspackages/arkor/src/core/trainerInspection.ts
packages/arkor/src/cli/commands/build.ts
📄 CodeRabbit inference engine (AGENTS.md)
Build user projects to
.arkor/build/index.mjs, keep bare package specifiers external, and target the published Node engine floor rather than the build host runtime.
Files:
packages/arkor/src/cli/commands/build.ts
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain AGENTS.md as a living document with current agent status and architectural decisions
Files:
AGENTS.md
*.{yml,yaml,json,html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Avoid using the em dash character (U+2014) in YAML, Markdown, JSON, and HTML files.
Files:
AGENTS.md
packages/arkor/src/{cli/studio,studio}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve Studio API security controls: send the token in
X-Arkor-Studio-Tokenfor fetches, usestudioTokenonly for approved GET-only EventSource routes, enforce the localhost host allow-list, use timing-safe comparison, and do not configure CORS.
Files:
packages/arkor/src/studio/manifest.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
packages/arkor/src/{cli/commands/dev.ts,studio}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not allow Studio token cleanup to delete another live session's token; retain atomic claim-and-compare cleanup and conventional signal exit codes.
Files:
packages/arkor/src/studio/manifest.tspackages/arkor/src/studio/trainRegistry.test.tspackages/arkor/src/studio/trainRegistry.tspackages/arkor/src/studio/server.tspackages/arkor/src/studio/server.test.tspackages/arkor/src/studio/hmr.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: arkorlab/arkor
Timestamp: 2026-08-02T22:24:32.218Z
Learning: Use the repository's pnpm + Turbo workspace structure and preserve declared workspace boundaries (`packages/*`, `e2e/*`, `examples/*`, and `docs`).
🪛 ast-grep (0.45.0)
packages/arkor/src/studio/trainRegistry.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
packages/arkor/src/studio/server.ts
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/concepts/studio.mdx
[style] ~19-~19: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... "Callbacks hot-swapped" indicator. Two caveats. On Windows there is no in-place swap: ...
(CAVEAT)
AGENTS.md
[style] ~86-~86: Consider using the typographical ellipsis character here instead.
Context: ...r a code change is contained inside the callbacks: { ... } object. (win32 exception: Node's `ki...
(ELLIPSIS)
[style] ~86-~86: Consider using “who” when you are referring to a person instead of an object.
Context: ...ult disposition would TERMINATE a child that hasn't registered its handler yet. Hot-...
(THAT_WHO)
[style] ~86-~86: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...e, which is documented as a user-facing caveat in docs/concepts/studio.mdx. Don't add ...
(CAVEAT)
🔇 Additional comments (35)
packages/arkor/src/core/trainer.ts (2)
837-862: The interaction between this re-check and the checkpoint branch is already reported at Lines 421-511, where the root cause sits. No separate comment here.
596-635: LGTM!Also applies to: 916-934
packages/arkor/src/core/configHash.ts (1)
43-79: LGTM!Also applies to: 114-131
packages/arkor/src/core/configHash.test.ts (2)
258-287: LGTM!Also applies to: 300-333
297-299: 📐 Maintainability & Code QualityRemove the duplicated
toJSONkey if it exists.The
nestedliteral shown here definestoJSONonce; if the source does not contain a duplicatetoJSON: () => ({ toJSON: () => "x", p: 1 }),line, this check is not applicable.packages/arkor/src/core/moduleCacheBust.ts (1)
84-87: LGTM!packages/arkor/src/core/trainerInspection.ts (1)
194-204: LGTM!packages/arkor/src/core/trainerInspection.test.ts (1)
264-286: LGTM!packages/arkor/src/core/runnerSignals.test.ts (1)
405-413: LGTM!packages/studio-app/src/components/RunTraining.tsx (2)
103-116: LGTM!Also applies to: 144-144, 237-288, 329-343, 419-419, 629-651, 653-662, 709-717, 729-734, 795-795
590-590: 🎯 Functional CorrectnessEnsure the forced-win32 kill path always emits
exit=before the stream closes.
RunTrainingshort-circuits theforcedKillrestart whenstreamFailedis true, while win32 SIGTERM leavesforcedKill: true. If an abrupt Nodekill()can re-enter the SSEerror(...)path instead of lettingchild.on("close", onClose)appendexit=${code}, HMR restart gets suppressed on the platform that relies on the flag.packages/studio-app/src/components/RunTraining.hmr.test.tsx (1)
384-462: LGTM!AGENTS.md (1)
70-79: LGTM!Also applies to: 87-87, 93-97
e2e/studio/src/specs/hmr.spec.ts (1)
141-157: LGTM!packages/arkor/src/studio/hmr.ts (5)
448-463: 🎯 Functional Correctness | 💤 Low valueConfirm the missing-entry error still reaches a first subscriber that arrives during the wait.
The
!entryWaitTimergate suppresses the broadcast on every re-entry. That is correct for existing subscribers, becausesubscribe()replayslastEvent. The replay only works whilelastEventstill holds the missing-entry error. A latererrorbroadcast (for example awatch()startup failure) overwriteslastEvent, and a subsequent poll cycle that finds the entry still missing never re-publishes the missing-entry message. This is a narrow window and the state is still an error frame, so the user-visible impact is limited to a stale message string.
667-675: 🚀 Performance & Scalability | ⚡ Quick winRe-check the identical-bytes rebuild path against the staging rename.
getCurrentArtifactContentHash()reads content at call time, so it is stable across identical-byte rebuilds.inspectBundlekeys onmoduleCacheBustUrl(outFile), andmoduleCacheBustUrlderives the key frommoduleCacheBustKey(filePath). ConfirmmoduleCacheBustKeyis content-derived and notmtime-derived. If it is content-derived, the earlier concern about the staging rename creating a new import URL for identical bytes no longer applies, and the ESM registry record count stays bounded to one per real edit.#!/bin/bash # Description: Inspect moduleCacheBustKey to confirm the cache key is content-derived. set -euo pipefail fd -t f 'moduleCacheBust.ts' packages/arkor/src --exec cat -n {}
220-264: LGTM!
553-585: LGTM!
685-699: LGTM!packages/arkor/src/studio/trainRegistry.ts (2)
107-118: LGTM!Also applies to: 191-246, 403-431
433-476: 🎯 Functional CorrectnessNo change needed for the readiness gate.
The supported
/api/trainflow only acceptstrainFile, usesarkor startfrom.arkor/bin/arkor.mjs, and always writes the job marker aftertrainer.start(). CustombinPath/ hand-rolled trainers can omit the marker, anddispatchRebuildthen routes safely via SIGTERM restart; the SPA restart behavior is unchanged.packages/arkor/src/studio/trainRegistry.test.ts (1)
60-64: LGTM!Also applies to: 113-113, 156-156, 187-187, 257-311, 381-381, 485-489, 535-535
packages/arkor/src/studio/manifest.ts (1)
210-229: LGTM!packages/arkor/src/studio/server.ts (4)
935-941: LGTM!
1033-1068: 🩺 Stability & Availability | 💤 Low valueThe 2 s marker window delays teardown for every marker-less child.
The loop exits early when the child is provably dead, which covers the common fast-exit case. A child that stays alive and never prints the marker (custom
binPath, a runner that fails beforetrainer.start()resolves, a user bin that writes no marker) now holds the SIGKILL for the full 2 s on every Stop click. With several concurrent runs stopped at once, each cancel holds its own timer and its own child.Confirm this is acceptable for the interactive Stop path, or shorten the wait when the child has produced no stdout at all.
1069-1087: LGTM!
1252-1285: LGTM!packages/arkor/src/cli/commands/dev.ts (2)
2-8: LGTM!
399-402: LGTM!Also applies to: 513-529
packages/arkor/src/cli/commands/dev.test.ts (1)
838-884: LGTM!packages/arkor/src/studio/server.test.ts (5)
83-99: LGTM!
922-922: LGTM!Also applies to: 1044-1044, 1143-1143, 1242-1242, 1346-1346, 1452-1452
1515-1527: LGTM!
2987-2989: LGTM!
767-817: 📐 Maintainability & Code QualityNo change needed: both HMR getter and spawn contracts are asserted.
The test already checks
configHashCalls === 0andcontentHashCalls === 0; because the request is handled through the samebuildStudioApp()codepath where/api/trainis the only place that reachesspawn(), these assertions cover the absence of hash getter reads and child spawning for this rejection path.
|
|
||
| When a rebuild lands while a `/api/train`-spawned subprocess is in flight, the server makes a per-child decision in [packages/arkor/src/studio/trainRegistry.ts](packages/arkor/src/studio/trainRegistry.ts): | ||
|
|
||
| - **`configHash` matches the spawn-time hash** → SIGUSR2. The child's `installCallbackReloadHandler` re-imports the artifact and rotates the trainer's callback cell via the internal `Symbol.for("arkor.trainer.replaceCallbacks")` brand exposed by [packages/arkor/src/core/trainerInspection.ts](packages/arkor/src/core/trainerInspection.ts). The cloud-side run is untouched. Use this whenever a code change is contained inside the `callbacks: { ... }` object. (win32 exception: Node's `kill()` treats unknown POSIX signals as a forceful termination there, so `dispatchRebuild` skips the SIGUSR2 attempt entirely on Windows and routes hash-matches through the SIGTERM-restart path; callback edits still take effect via a brief restart rather than an in-place swap. Those win32 restart targets carry `forcedKill: true` so the SPA's nonzero-exit auto-restart suppression lets the advertised restart through.) SIGUSR2 dispatch is additionally gated on the runner's `Started job` marker having been recorded: the marker doubles as the "handler is armed" handshake, because SIGUSR2's default disposition would TERMINATE a child that hasn't registered its handler yet. Hot-swap also replaces callback FUNCTIONS only; module-scope state they close over (an `AbortController` beside `createTrainer`, the trainer reference itself) rebinds to the rebuilt module, which is documented as a user-facing caveat in docs/concepts/studio.mdx. Don't add a `replaceCallbacks()` method to the public `Trainer` interface: keeping the mutator behind a `Symbol.for` brand is what stops the dev-only HMR primitive from leaking into the SDK's published surface. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The new forcedKill contract contradicts the suppression sentence later in this section.
Line 86 now states that win32 restart targets carry forcedKill: true so the SPA lets the advertised restart through the nonzero-exit suppression. Line 89 still states the rule without the carve-out: a nonzero or missing exit= marker or a rejected stream suppresses the auto-restart. Line 89 also omits the second new behavior in RunTraining.tsx Line 637-651, where a latched restart is deferred while the build is broken and released on the next successful rebuild. Update Line 89 so the section describes one consistent contract.
📝 Proposed wording for Line 89
-While all this is in flight, `RunTraining` surfaces the state via `hmrStatus` labels ("Stopping at next checkpoint…", "Restarting with updated code…", "Callbacks hot-swapped: run continues."); a nonzero / missing `exit=` marker or a rejected stream suppresses the auto-restart instead of re-spawning.
+While all this is in flight, `RunTraining` surfaces the state via `hmrStatus` labels ("Stopping at next checkpoint…", "Restarting with updated code…", "Callbacks hot-swapped: run continues."); a nonzero / missing `exit=` marker or a rejected stream suppresses the auto-restart instead of re-spawning, except for a `forcedKill` restart target (the win32 case above), which is let through because the server already fired its compensation cancel POST. A latched restart is also DEFERRED while the last SSE frame was a build `error`, and released by the next successful rebuild, so the re-spawn uses the fixed artifact rather than the last-good one.🧰 Tools
🪛 LanguageTool
[style] ~86-~86: Consider using the typographical ellipsis character here instead.
Context: ...r a code change is contained inside the callbacks: { ... } object. (win32 exception: Node's `ki...
(ELLIPSIS)
[style] ~86-~86: Consider using “who” when you are referring to a person instead of an object.
Context: ...ult disposition would TERMINATE a child that hasn't registered its handler yet. Hot-...
(THAT_WHO)
[style] ~86-~86: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...e, which is documented as a user-facing caveat in docs/concepts/studio.mdx. Don't add ...
(CAVEAT)
🤖 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 `@AGENTS.md` at line 86, Update the SPA auto-restart contract described later
in AGENTS.md to include the forcedKill exception for win32 restart targets,
allowing advertised restarts despite nonzero exit suppression. Also document
that latched restart requests are deferred while the build is broken and
released after the next successful rebuild, keeping the existing suppression
behavior for nonzero or missing exit markers and rejected streams otherwise.
| A note on the dev loop: Studio's `/api/manifest` endpoint rebuilds and re-imports your trainer on every request (with a cache-bust query, see `packages/arkor/src/studio/manifest.ts`), but the UI only fetches it when the Run training page mounts. So if you edit `src/arkor/` and stay on the same Run training page, the next click reuses the existing `.arkor/build/index.mjs` and runs your old code. Refresh the page (or run `arkor build` from the terminal) between edits and clicks to pick up the new code reliably. | ||
| A note on the dev loop: Studio runs a [Rolldown](https://rolldown.rs) watcher over `src/arkor/` and pushes rebuild notifications to the SPA over a Server-Sent Events stream (`/api/dev/events`). Edit a file, save, and the Run training button updates with the new trainer name without a refresh. If a training run is in flight, the Studio compares the new bundle's cloud-side `JobConfig` hash to the one captured when the run was spawned: | ||
|
|
||
| - **Same hash (only callbacks changed).** The runner is signalled with SIGUSR2; it re-imports the rebuilt artifact and rotates the trainer's callback cell in place via an internal HMR brand. The cloud-side training run is untouched, no GPU time is wasted, and the SPA shows a brief "Callbacks hot-swapped" indicator. Two caveats. On Windows there is no in-place swap: Node treats POSIX signals as a forceful termination there, so callback-only edits take the restart path below instead (the outcome is the same, via a brief restart). And a hot-swap replaces the callback *functions* only: state a callback closes over from module scope (an `AbortController` created next to `createTrainer`, a reference to the trainer itself) now belongs to the rebuilt module's fresh instances, not the live run. Keep control state inside the callbacks, or accept that such edits behave like config changes and restart the run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both Studio pages state an over-broad reason for the Windows fallback. Each page says Node treats POSIX signals as a forceful termination on Windows. AGENTS.md Line 86 gives the precise reason: Node's kill() treats unknown POSIX signals as a forceful termination there, which is why dispatchRebuild skips the SIGUSR2 attempt on Windows. As written, the sentence reads as "all POSIX signals", which conflicts with the next bullet, where the SIGTERM restart path is presented as the graceful, checkpoint-aware route.
docs/concepts/studio.mdx#L19-L19: change "Node treats POSIX signals as a forceful termination there" to name the unknown-signal case, for example "Node treats unknown POSIX signals such as SIGUSR2 as a forceful termination there".docs/ja/concepts/studio.mdx#L19-L19: apply the matching correction, for example "Node は SIGUSR2 のような未知の POSIX シグナルを強制終了として扱うため".
As per coding guidelines: "When editing English documentation, update its Japanese counterpart in the same change."
🧰 Tools
🪛 LanguageTool
[style] ~19-~19: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... "Callbacks hot-swapped" indicator. Two caveats. On Windows there is no in-place swap: ...
(CAVEAT)
📍 Affects 2 files
docs/concepts/studio.mdx#L19-L19(this comment)docs/ja/concepts/studio.mdx#L19-L19
🤖 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 `@docs/concepts/studio.mdx` at line 19, The Windows fallback explanation is too
broad about POSIX signals. Update the sentence in docs/concepts/studio.mdx at
lines 19-19 to state that Node forcefully terminates for unknown POSIX signals
such as SIGUSR2, and apply the equivalent correction in
docs/ja/concepts/studio.mdx at lines 19-19.
Source: Coding guidelines
| const files = readdirSync(join(cwd, ".arkor/build")); | ||
| // One artifact, no chunk siblings. | ||
| expect(files).toEqual(["index.mjs"]); | ||
| // The helper's code is inlined, not referenced via a chunk import. | ||
| const content = readFileSync(result.outFile, "utf8"); | ||
| expect(content).toContain("lazy-helper-loaded"); | ||
| expect(content).not.toMatch(/import\(["'].\/(?!helper)/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Tighten the "no relative chunk import" assertion.
Line 162 has two defects. The . before \/ is unescaped, so it matches any character, not a literal dot. The (?!helper) exemption also lets the exact regression pass: if rolldown emitted import("./helper.mjs"), this assertion would not fire, and only the files check would catch it. After inlineDynamicImports: true, no relative dynamic import must remain at all.
💚 Proposed fix for the regex
- expect(content).not.toMatch(/import\(["'].\/(?!helper)/);
+ expect(content).not.toMatch(/import\(\s*["']\.\//);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const files = readdirSync(join(cwd, ".arkor/build")); | |
| // One artifact, no chunk siblings. | |
| expect(files).toEqual(["index.mjs"]); | |
| // The helper's code is inlined, not referenced via a chunk import. | |
| const content = readFileSync(result.outFile, "utf8"); | |
| expect(content).toContain("lazy-helper-loaded"); | |
| expect(content).not.toMatch(/import\(["'].\/(?!helper)/); | |
| const files = readdirSync(join(cwd, ".arkor/build")); | |
| // One artifact, no chunk siblings. | |
| expect(files).toEqual(["index.mjs"]); | |
| // The helper's code is inlined, not referenced via a chunk import. | |
| const content = readFileSync(result.outFile, "utf8"); | |
| expect(content).toContain("lazy-helper-loaded"); | |
| expect(content).not.toMatch(/import\(\s*["']\.\//); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/arkor/src/cli/commands/build.test.ts` around lines 156 - 162, Update
the relative dynamic-import assertion in the build test to match every relative
import with a literal “./” prefix, without excluding helper imports. Preserve
the existing inlined-content and artifact checks while ensuring any remaining
relative dynamic import, including “./helper.mjs”, fails the test.
| // `inlineDynamicImports`: this build contract is a SINGLE | ||
| // artefact (`arkor start` only ever imports `index.mjs`), but a | ||
| // relative dynamic `import()` in user code would trigger | ||
| // rolldown's default code splitting and emit sibling chunks the | ||
| // runtime never loads: the lazy-helper pattern the docs suggest | ||
| // for callbacks would break at run time (Codex P2, round 86). | ||
| // esbuild's previous single-file behaviour inlined these. | ||
| await bundle.write({ | ||
| file: outFile, | ||
| format: "esm", | ||
| inlineDynamicImports: true, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
rolldown 1.1.1 output option inlineDynamicImports documentation
💡 Result:
In Rolldown version 1.1.1, the inlineDynamicImports output option is marked as deprecated [1]. Users are officially advised to use the codeSplitting: false option instead [2][1]. The deprecated inlineDynamicImports option was intended to control whether dynamic imports are inlined into a single bundle instead of creating separate chunks [2][1]. This functionality is now handled by the codeSplitting option [2][3]. Specifically: - To replicate the behavior of inlineDynamicImports: true (creating a single bundle), you should set codeSplitting: false [2][4]. - The default behavior, which supports automatic code splitting, corresponds to codeSplitting: true (or effectively not disabling code splitting) [2]. Because this option is deprecated, it is recommended to migrate to the codeSplitting option to ensure continued compatibility with future versions of Rolldown [5][1].
Citations:
- 1: https://rolldown.rs/reference/OutputOptions.inlineDynamicImports
- 2: https://rolldown.rs/reference/Interface.OutputOptions
- 3: https://github.com/rolldown/rolldown/blob/df2ec37cc664906329fd5a16782d1a66cc35aebb/packages/rolldown/src/options/output-options.ts
- 4: deprecate
inlineDynamicImports: trueand replace it withcodeSplitting: falserolldown/rolldown#7537 - 5: https://github.com/rolldown/rolldown/releases/tag/v1.1.1
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching package manifests:"
git ls-files | rg '(^|/)(package.json|package-lock.json|pnpm-lock.yaml|yarn.lock|bun.lockb)$' || true
echo
echo "Rolldown references:"
rg -n '"rolldown"|rolldown|inlineDynamicImports|codeSplitting' -S --glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'package-lock.json' --glob 'yarn.lock' --glob 'bun.lockb' .
echo
echo "Target file excerpt:"
test -f packages/arkor/src/cli/commands/build.ts && sed -n '1,110p' packages/arkor/src/cli/commands/build.ts || trueRepository: arkorlab/arkor
Length of output: 10191
🌐 Web query:
Rolldown 1.1.1 OutputOptions codeSplitting false prevents output chunks rollup compatibility
💡 Result:
In Rolldown, the output.codeSplitting: false option is designed to disable automatic code splitting and inline all dynamic imports into a single bundle, functioning as the intended replacement for the deprecated inlineDynamicImports: true option [1][2][3][4]. Regarding your query about compatibility and technical behavior: 1. Intended Functionality: Setting codeSplitting: false tells Rolldown to merge all dynamically imported modules into the entry chunk rather than creating separate output files for them [2][4]. This is functionally equivalent to the legacy inlineDynamicImports behavior in Rollup [2][5]. 2. Known CLI Limitations: As of recent versions, there is a known issue where passing --codeSplitting false via the CLI may be rejected or incorrectly parsed as a string rather than a boolean [6]. If you are experiencing issues with the CLI, this is a reported bug rather than a fundamental incompatibility with the option itself [6]. 3. Rollup Compatibility: While Rolldown aims for high compatibility with Rollup, its code splitting engine is fundamentally different—using a BitSet-based reachability model rather than Rollup’s entry-set coloring [7]. Because of these architectural differences, while the configuration options (like codeSplitting) are intended to provide a similar user experience, the internal bundle construction process is specific to Rolldown [7]. If you are using Rolldown via a configuration file (e.g., rolldown.config.js), ensuring codeSplitting is explicitly set as the boolean false (not a string) should correctly trigger the expected inlining behavior [2][3]. If you continue to see unexpected chunking, ensure you are not also using deprecated options that might conflict, such as manualChunks [1][2].
Citations:
- 1: https://github.com/rolldown/rolldown/blob/1e8ecb03cfe5a9d4d923be2aaf7f61270fb70e68/packages/rolldown/src/options/output-options.ts
- 2: https://rolldown.rs/reference/Interface.OutputOptions
- 3: https://github.com/rolldown/rolldown/blob/44203f0a016d23e0d8d2729f292b0002eb6e2b83/packages/rolldown/src/options/output-options.ts
- 4: https://github.com/rolldown/rolldown/blob/a5b0b75d7d4f9d1bf44655281fa03f010119cedd/packages/rolldown/src/options/output-options.ts
- 5: deprecate
inlineDynamicImports: trueand replace it withcodeSplitting: falserolldown/rolldown#7537 - 6: [Bug]: CLI:
--codeSplitting falseis rejected as a string instead of being parsed as boolean rolldown/rolldown#10125 - 7: https://github.com/rolldown/rolldown/blob/main/meta/design/code-splitting.md
Use output.codeSplitting: false for the single-artifact build contract.
Rolldown 1.1.1 supports inlineDynamicImports, but it is deprecated and the documented replacement is codeSplitting: false; use the supported option so arkor start does not rely on a legacy bundler flag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/arkor/src/cli/commands/build.ts` around lines 54 - 65, Update the
bundle.write configuration in the build command to replace the deprecated
inlineDynamicImports option with output.codeSplitting: false, preserving the
single-artifact behavior expected by arkor start.
| if (typeof process.getuid === "function" && process.getuid() === 0) { | ||
| // Root bypasses chmod permission checks: skip on root containers. | ||
| return; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Report the root case as skipped, not as passed.
The it.skipIf change is correct for win32. The root check inside the body still uses an early return, so a root container reports this test green while it asserts nothing. Fold the root condition into the same skipIf predicate. process.getuid is undefined on win32, and the typeof guard keeps the expression safe at collection time on every platform.
♻️ Proposed refactor
- it.skipIf(process.platform === "win32")(
+ // Root bypasses chmod permission checks, so the read-only-parent
+ // trigger cannot make the atomic persist fail on root containers.
+ it.skipIf(
+ process.platform === "win32" ||
+ (typeof process.getuid === "function" && process.getuid() === 0),
+ )(
"does NOT unlink a pre-existing token file when this process failed to persist its own token (concurrent arkor dev safety)",
async () => {
@@
- if (typeof process.getuid === "function" && process.getuid() === 0) {
- // Root bypasses chmod permission checks: skip on root containers.
- return;
- }📝 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.
| if (typeof process.getuid === "function" && process.getuid() === 0) { | |
| // Root bypasses chmod permission checks: skip on root containers. | |
| return; | |
| } | |
| // Root bypasses chmod permission checks, so the read-only-parent | |
| // trigger cannot make the atomic persist fail on root containers. | |
| it.skipIf( | |
| process.platform === "win32" || | |
| (typeof process.getuid === "function" && process.getuid() === 0), | |
| )( | |
| "does NOT unlink a pre-existing token file when this process failed to persist its own token (concurrent arkor dev safety)", | |
| async () => { | |
| // Existing test body, without the root early return. | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/arkor/src/cli/commands/dev.test.ts` around lines 834 - 837, Update
the test’s it.skipIf predicate to also skip when process.getuid exists and
returns 0, preserving the typeof guard for win32 safety. Remove the in-body root
check and early return so root containers are reported as skipped rather than
passed.
| // Custom-entry runs are refused while HMR owns the build artefact | ||
| // (Codex P2, round 86). `arkor start <file>` rebuilds the SHARED | ||
| // `.arkor/build/index.mjs` from the custom entry, and since the | ||
| // HMR manifest fast path never rebuilds the default entry itself, | ||
| // that overwrite would persist until the watcher's next | ||
| // source-triggered publish: Studio would display (and a later | ||
| // default Run would execute) the wrong trainer. The SPA never | ||
| // sends `file`; direct API users can run `arkor start <file>` | ||
| // in a terminal, which doesn't fight the watcher-owned artefact | ||
| // lifecycle Studio depends on. | ||
| if (trainFile && options.hmr) { | ||
| return c.json( | ||
| { | ||
| error: | ||
| "custom file runs are not supported while arkor dev's HMR watcher owns the build artifact; run `arkor start <file>` directly instead", | ||
| }, | ||
| 400, | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The new 400 makes the downstream custom-entry hash guards unreachable.
This branch returns 400 whenever trainFile is set and options.hmr is present. After it, options.hmr && !trainFile can only evaluate the !trainFile operand as true, so the !trainFile conditions on configHash and spawnArtifactContentHash are now dead. Their explanatory comments still describe a custom-entry run that reaches the spawn path, which no longer happens.
Keep the conditions as defence in depth if you prefer, but update the comments so a future reader does not treat the custom-entry spawn flow as live. Also remove the "Known residual wart" paragraph, because the custom-entry rebuild can no longer overwrite the watcher-owned artifact through this route.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import {
spawn,
type ChildProcess,
type ChildProcessByStdio,
} from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/arkor/src/studio/server.ts` around lines 561 - 579, Update the
downstream hash-guard comments near configHash and spawnArtifactContentHash to
reflect that custom-entry runs are rejected before reaching the spawn path,
while retaining the guards as defensive checks if desired. Remove the “Known
residual wart” paragraph describing custom-entry rebuilds overwriting the
watcher-owned artifact, since this route now prevents that behavior.
| restartTargets.push( | ||
| process.platform === "win32" | ||
| ? { ...target, forcedKill: true } | ||
| : target, | ||
| ); | ||
| } | ||
| // "gone" / "unsupported" again → drop silently; the close | ||
| // handler (or operator-driven restart) will recover. | ||
| continue; | ||
| } | ||
|
|
||
| // Hash mismatch (or one side is null), or a hash match whose | ||
| // child isn't provably ready for SIGUSR2: graceful restart. | ||
| // (On win32 "graceful" is aspirational: the kill is abrupt, so | ||
| // flag the target for the SPA's suppression carve-out.) | ||
| const r = safeKill(entry.child, "SIGTERM"); | ||
| if (r === "ok") { | ||
| entry.earlyStopRequested = true; | ||
| restartTargets.push( | ||
| process.platform === "win32" | ||
| ? { ...target, forcedKill: true } | ||
| : target, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Deduplicate the win32 restart-target construction.
The same platform-conditional object spread appears twice in one loop body. A single per-iteration helper keeps the two restart paths in step. If a future change adds a third field to the forced-kill target, one site can be missed.
♻️ Proposed refactor
for (const [pid, entry] of this.entries) {
if (entry.earlyStopRequested) continue;
const target: RestartTarget = { pid, trainFile: entry.trainFile };
+ // On win32 every POSIX signal name terminates the child
+ // abruptly, so the SPA needs the forcedKill carve-out on any
+ // restart target this pass produces.
+ const restartTarget: RestartTarget =
+ process.platform === "win32" ? { ...target, forcedKill: true } : target;
@@
const fallback = safeKill(entry.child, "SIGTERM");
if (fallback === "ok") {
entry.earlyStopRequested = true;
- restartTargets.push(
- process.platform === "win32"
- ? { ...target, forcedKill: true }
- : target,
- );
+ restartTargets.push(restartTarget);
}
@@
const r = safeKill(entry.child, "SIGTERM");
if (r === "ok") {
entry.earlyStopRequested = true;
- restartTargets.push(
- process.platform === "win32"
- ? { ...target, forcedKill: true }
- : target,
- );
+ restartTargets.push(restartTarget);
}📝 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.
| restartTargets.push( | |
| process.platform === "win32" | |
| ? { ...target, forcedKill: true } | |
| : target, | |
| ); | |
| } | |
| // "gone" / "unsupported" again → drop silently; the close | |
| // handler (or operator-driven restart) will recover. | |
| continue; | |
| } | |
| // Hash mismatch (or one side is null), or a hash match whose | |
| // child isn't provably ready for SIGUSR2: graceful restart. | |
| // (On win32 "graceful" is aspirational: the kill is abrupt, so | |
| // flag the target for the SPA's suppression carve-out.) | |
| const r = safeKill(entry.child, "SIGTERM"); | |
| if (r === "ok") { | |
| entry.earlyStopRequested = true; | |
| restartTargets.push( | |
| process.platform === "win32" | |
| ? { ...target, forcedKill: true } | |
| : target, | |
| ); | |
| for (const [pid, entry] of this.entries) { | |
| if (entry.earlyStopRequested) continue; | |
| const target: RestartTarget = { pid, trainFile: entry.trainFile }; | |
| // On win32 every POSIX signal name terminates the child | |
| // abruptly, so the SPA needs the forcedKill carve-out on any | |
| // restart target this pass produces. | |
| const restartTarget: RestartTarget = | |
| process.platform === "win32" ? { ...target, forcedKill: true } : target; | |
| if (...) { | |
| const fallback = safeKill(entry.child, "SIGTERM"); | |
| if (fallback === "ok") { | |
| entry.earlyStopRequested = true; | |
| restartTargets.push(restartTarget); | |
| } | |
| } | |
| // "gone" / "unsupported" again → drop silently; the close | |
| // handler (or operator-driven restart) will recover. | |
| continue; | |
| } | |
| // Hash mismatch (or one side is null), or a hash match whose | |
| // child isn't provably ready for SIGUSR2: graceful restart. | |
| // (On win32 "graceful" is aspirational: the kill is abrupt, so | |
| // flag the target for the SPA's suppression carve-out.) | |
| const r = safeKill(entry.child, "SIGTERM"); | |
| if (r === "ok") { | |
| entry.earlyStopRequested = true; | |
| restartTargets.push(restartTarget); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/arkor/src/studio/trainRegistry.ts` around lines 478 - 500,
Deduplicate the platform-specific restart target construction within the loop by
defining one per-iteration helper or local value for the target, applying
forcedKill on win32 and preserving the original target otherwise. Replace both
restartTargets.push expressions in the “gone”/“unsupported” and graceful-restart
paths with that shared value so both paths remain consistent.
| /* eslint-disable @typescript-eslint/no-unnecessary-condition -- | ||
| const-narrowing re-pins `forcedRestart` to the literal `false` | ||
| even through the boolean annotation, so the `if` below trips | ||
| the rule too; the disable spans both lines. */ | ||
| const forcedRestart: boolean = | ||
| restartPendingRef.current && forcedKillLatchRef.current; | ||
| if (streamFailed || (exitCode !== 0 && !forcedRestart)) { | ||
| /* eslint-enable @typescript-eslint/no-unnecessary-condition */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the repeated no-unnecessary-condition disables into the root ESLint config.
This function now carries four disables of the same rule for the same root cause: Line 584-591, Line 613, Line 636, and Line 656. All of them exist because TypeScript's flow analysis cannot see the cross-closure writes to restartPendingRef and forcedKillLatchRef. A single scoped override in eslint.config.ts for this file, with the reason in a comment, keeps the explanation in one place and stops the next ref read from needing a fifth disable.
As per coding guidelines: "Use the single root oxlint and ESLint configurations; add scoped root overrides rather than per-package configs or repeated inline disables." AGENTS.md states the same preference: "prefer a scoped override in eslint.config.ts with the why in a comment over inline // eslint-disable at every site."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio-app/src/components/RunTraining.tsx` around lines 584 - 591,
Remove the repeated inline `@typescript-eslint/no-unnecessary-condition`
disable/enable directives around forcedRestart and the other affected reads in
RunTraining, then add one scoped override for this file in the root
eslint.config.ts. Include a comment explaining that cross-closure writes to
restartPendingRef and forcedKillLatchRef are not recognized by TypeScript flow
analysis, and preserve the existing rule behavior elsewhere.
Source: Coding guidelines
| }, 250); | ||
| } | ||
|
|
||
| runRef.current = run; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
React 19 assigning ref current during render vs useEffect latest callback ref pattern
💡 Result:
In React 19, the core rules for ref management remain consistent with established best practices: you must not read or write ref.current during rendering [1][2]. Doing so makes component behavior unpredictable because React does not track changes to ref objects for re-rendering purposes [2]. Ref current assignment during render: Writing or reading ref.current during render is generally prohibited [1][2]. The single exception is initialization, such as checking if a ref is null and setting it if it is empty (e.g., if (!ref.current) ref.current = new Thing), which is considered safe because it is predictable and only occurs during the initial render [1][2]. React's Compiler enforces these rules and may flag violations as errors [3][4]. Ref callback pattern in React 19: React 19 introduces significant improvements to the ref callback pattern, specifically the support for cleanup functions [5]. You can now return a cleanup function from a ref callback, which React will execute when the component unmounts or when the ref is detached [6][5]. This eliminates the need for manual cleanup in useEffect in many cases [5]. When you provide a function as a ref, React calls it with the DOM node when the component mounts and with null when it unmounts [7][8]. If you return a cleanup function, React uses it instead of calling the ref with null during unmounting [5]. Key considerations: 1. Ref as a Prop: React 19 now allows you to pass refs as ordinary props, which makes forwardRef largely unnecessary [6][5][9]. 2. Compiler Enforcement: The React Compiler rigorously checks for illegal ref access during render [3][4]. If you are experiencing false positives, ensure you are using the latest stable versions, as fixes for these issues are actively landed [3][4][10]. 3. useEffect vs. Ref Callback: Use useEffect when you need to perform side effects synchronized with rendering (e.g., responding to prop changes) [11]. Use ref callbacks when you need to respond immediately to the lifecycle of a specific DOM element, such as measuring it or managing its cleanup [7]. The ability to return a cleanup function directly from the ref callback provides a cleaner, more declarative way to manage these resources [5]. Top results: [1], [5], [7], [2]
Citations:
- 1: https://react.dev/reference/react/useRef
- 2: https://react.dev/learn/referencing-values-with-refs
- 3: [Compiler Bug]: InvalidReact: Ref values (the
currentproperty) may not be accessed during render. False Positive react/react#32320 - 4: [Compiler Bug]:
ref.current === undefined,!ref.currentare not allowed react/react#34278 - 5: https://react.dev/blog/2024/12/05/react-19
- 6: https://github.com/react/react/blob/refs/heads/main/CHANGELOG.md
- 7: https://react.dev/learn/manipulating-the-dom-with-refs
- 8: https://legacy.reactjs.org/docs/refs-and-the-dom.html
- 9: https://generalistprogrammer.com/tutorials/react-forwardref-complete-guide
- 10: [Compiler Bug]: Linter complains about ref
currentaccess during render when returning array with Typescript'sas constreact/react#31864 - 11: [Compiler]: Ref values (the
currentproperty) may not be accessed during render. (eslint) react/react#31330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant component sections without running repo code.
fd -a 'RunTraining\.tsx$' . | sed 's#^\./##'
FILE="$(fd 'RunTraining\.tsx$' . | head -n 1)"
echo "FILE=$FILE"
wc -l "$FILE"
echo "---- outline ----"
ast-grep outline "$FILE" --view condensed | sed -n '1,220p' || true
echo "---- lines 360-410 ----"
sed -n '360,410p' "$FILE" | nl -ba -v360
echo "---- lines 700-735 ----"
sed -n '700,735p' "$FILE" | nl -ba -v700
echo "---- usages of runRef and run in file ----"
rg -n "runRef|run\\(\\)|useEffect|useCallback|useRef|Source|EventSource|api/dev/events" "$FILE"Repository: arkorlab/arkor
Length of output: 573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/studio-app/src/components/RunTraining.tsx"
echo "---- package metadata (React versions) ----"
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
[ -f "package.json" ] && echo "--- package.json ---" && sed -n '1,220p' package.json
done
echo "---- RunTraining relevant hook/dependency refs ----"
awk 'BEGIN{RS=ORS="\n"} /^[[:space:]]*(const|function|function[[:space:]]+)/{print NR": "$0}' "$FILE" | sed -n '1,120p'
echo "---- runRef and run usage ----"
grep -n -E 'runRef|run\s*\(|useEffect|useCallback|useRef|Source|EventSource|/api/dev/events' "$FILE" | sed -n '1,240p'
echo "---- lines 350-415 ----"
awk 'NR>=350 && NR<=415 {printf "%6d: %s\n", NR, $0}' "$FILE"
echo "---- lines 700-735 ----"
awk 'NR>=700 && NR<=735 {printf "%6d: %s\n", NR, $0}' "$FILE"Repository: arkorlab/arkor
Length of output: 17626
🌐 Web query:
React useRef useEffect latest callback pattern discard render ref.current during render React documentation
💡 Result:
The "latest callback" pattern is a technique used to keep a reference to a callback or value that is always up-to-date, without triggering re-renders or unnecessary effect executions [1][2]. It is commonly used to access the most recent props or state within asynchronous operations (like timers) or event listeners that are not re-bound on every render [1][3]. The pattern works by storing the value or callback in a useRef object and updating that ref within a useEffect (or useLayoutEffect) hook [1][2]. Because useEffect runs after the component renders, this ensures the update is safe and happens during the commit phase, rather than during the render phase [2][4]. Key principles regarding this pattern and useRef: 1. Do Not Read/Write Refs During Rendering: The official React documentation explicitly warns against reading or writing ref.current during the render phase [5][6]. Rendering must be pure and predictable [4]. Mutating or reading a ref while rendering can cause unpredictable behavior, especially with concurrent rendering [5][4]. The only exception is initialization (e.g., lazy initialization) where the code is fully predictable [5][6]. 2. Implementation: - Create a ref: const callbackRef = useRef(callback); [1] - Update the ref in an effect: Use useEffect (or useLayoutEffect) to sync the ref with the latest value on every render [1][2]. javascript useEffect(() => { callbackRef.current = callback; }); - Use the ref: Access callbackRef.current inside your asynchronous code, event handlers, or other effects [1][3]. Since the ref is a stable object, accessing it does not require adding it to dependency arrays [1]. 3. Dependency Management: A common mistake is including ref.current in a useEffect dependency array [1]. Because updating a ref does not trigger a re-render, React cannot know when to re-run an effect based on ref.current changes [1]. Including it in dependencies will not produce the reactive behavior you might expect and often indicates that state or a different pattern should be used instead [1][5]. By separating the "latest value" (stored in a ref) from the "reactive value" (used for rendering or effect triggers), you can maintain stability and prevent unnecessary re-renders while ensuring that your side effects always use the most recent data [1][7].
Citations:
- 1: https://www.epicreact.dev/the-latest-ref-pattern-in-react
- 2: Accessing state/props in callback ref with hooks react/react#16154
- 3: https://react.dev/learn/referencing-values-with-refs
- 4: https://react.dev/reference/rules/components-and-hooks-must-be-pure
- 5: https://react.dev/reference/react/useRef
- 6: https://uk.react.dev/reference/react/useRef
- 7: https://evanliu.dev/programming/react/latest-ref/
Move runRef.current = run out of the render body.
runRef is used by the SSE effect after mounting, but assigning it during render writes the ref to a render that may be discarded. Store the latest run callback in a dependency-free useEffect so the committed render’s closure is used.
Suggested change
- runRef.current = run;
+ useEffect(() => {
+ runRef.current = run;
+ });📝 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.
| runRef.current = run; | |
| useEffect(() => { | |
| runRef.current = run; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio-app/src/components/RunTraining.tsx` at line 727, Move the
runRef.current assignment out of the render body in RunTraining and update it
inside a dependency-free useEffect that tracks the latest run callback. Keep the
SSE effect’s use of runRef unchanged so it reads the callback from the committed
render.
| restartTargets?: { | ||
| pid: number; | ||
| trainFile?: string; | ||
| /** Set (true) when the SIGTERM behind this target is an abrupt | ||
| * kill on the server's platform (win32), so the exit marker | ||
| * cannot be the clean `exit=0` the SPA's restart gate normally | ||
| * requires; the server has already fired its compensation | ||
| * cancel POST for the cloud job. */ | ||
| forcedKill?: boolean; | ||
| }[]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Export the restart-target shape as a named type.
restartTargets is declared as an inline object literal type. RunTraining.tsx re-declares the same shape by hand at Line 462-464 ({ pid: number; trainFile?: string; forcedKill?: boolean }) to type the restartHit variable. The two declarations can drift when a new field is added, and the drift is silent because find() returns a structurally compatible value. Export the shape once and reuse it.
♻️ Proposed refactor
+/** One child process targeted by an HMR restart dispatch. */
+export interface RestartTarget {
+ pid: number;
+ trainFile?: string;
+ /** Set (true) when the SIGTERM behind this target is an abrupt
+ * kill on the server's platform (win32), so the exit marker
+ * cannot be the clean `exit=0` the SPA's restart gate normally
+ * requires; the server has already fired its compensation
+ * cancel POST for the cloud job. */
+ forcedKill?: boolean;
+}- restartTargets?: {
- pid: number;
- trainFile?: string;
- /** Set (true) when the SIGTERM behind this target is an abrupt
- * kill on the server's platform (win32), so the exit marker
- * cannot be the clean `exit=0` the SPA's restart gate normally
- * requires; the server has already fired its compensation
- * cancel POST for the cloud job. */
- forcedKill?: boolean;
- }[];
+ restartTargets?: RestartTarget[];Then in RunTraining.tsx, import RestartTarget and replace the inline literal at Line 462-464.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio-app/src/lib/api.ts` around lines 212 - 221, Export the
restart-target object shape in api.ts as a named RestartTarget type, then change
the restartTargets declaration to use it. In RunTraining.tsx, import
RestartTarget and replace the inline type on restartHit with the shared type so
both declarations stay synchronized.
There was a problem hiding this comment.
4 issues found across 25 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/arkor/src/cli/commands/dev.ts">
<violation number="1" location="packages/arkor/src/cli/commands/dev.ts:525">
P2: An EADDRINUSE launch can still remain alive indefinitely when Rolldown's `watcher.close()` wedges: rejection only sets the CLI exit code, while this detached close retains the watcher handle. Add bounded pre-bind teardown with a forced-exit fallback (or otherwise ensure this failure path cannot wait forever).</violation>
</file>
<file name="packages/studio-app/src/components/RunTraining.tsx">
<violation number="1" location="packages/studio-app/src/components/RunTraining.tsx:590">
P1: Windows HMR can start a replacement cloud job after the old job's compensation cancel was rejected, leaving both jobs running. Only advertise/bypass `forcedKill` after the server has confirmed a successful cancel response; otherwise retain nonzero-exit suppression.</violation>
<violation number="2" location="packages/studio-app/src/components/RunTraining.tsx:727">
P3: The deferred-restart path invokes the current render's `run` through `runRef`, and `runRef.current` is assigned directly during render (after the `run` declaration). React discourages writing refs during render — the assignment can be discarded or land on an aborted render in StrictMode/concurrent flows, which is only safe here because every `run` closure reaches all its state through refs/setState. To make this robust rather than accidentally-correct, move the assignment into a mount effect (e.g. an empty-deps `useEffect(() => { runRef.current = run; })`), or refactor so the effect re-subscribes rather than mutating during render. No behavioral bug today; this is a fragility/convention note.</violation>
</file>
<file name="packages/arkor/src/core/trainer.ts">
<violation number="1" location="packages/arkor/src/core/trainer.ts:428">
P2: In the checkpoint-triggered early-stop branch you flip `earlyStopRequested = false` before the async `cancel()`, but the early-stop timeout timer stays armed until `settleEarlyStopLatch()` runs after the cancel resolves. If the checkpoint lands just before the timeout boundary and the cancel POST is still pending when the timer fires, the timer sees `earlyStopRequested === false`, calls `settleEarlyStopLatch()` first, and nulls `earlyStopDeferred` (and resolves it as a clean success). Then, if the cancel then fails, the checkpoint branch's failure path dereferences `earlyStopDeferred.timer` / `.reject(...)` on the nulled slot and throws a TypeError — and because the timer already resolved the deferred, the shutdown handler can exit 0 for a cancel that actually failed, masking the failure this round was meant to surface. Consider clearing the timer when the checkpoint branch takes ownership of the early-stop, and guarding the failure path against an already-nulled deferred.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| the rule too; the disable spans both lines. */ | ||
| const forcedRestart: boolean = | ||
| restartPendingRef.current && forcedKillLatchRef.current; | ||
| if (streamFailed || (exitCode !== 0 && !forcedRestart)) { |
There was a problem hiding this comment.
P1: Windows HMR can start a replacement cloud job after the old job's compensation cancel was rejected, leaving both jobs running. Only advertise/bypass forcedKill after the server has confirmed a successful cancel response; otherwise retain nonzero-exit suppression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/studio-app/src/components/RunTraining.tsx, line 590:
<comment>Windows HMR can start a replacement cloud job after the old job's compensation cancel was rejected, leaving both jobs running. Only advertise/bypass `forcedKill` after the server has confirmed a successful cancel response; otherwise retain nonzero-exit suppression.</comment>
<file context>
@@ -510,7 +571,24 @@ export function RunTraining() {
+ the rule too; the disable spans both lines. */
+ const forcedRestart: boolean =
+ restartPendingRef.current && forcedKillLatchRef.current;
+ if (streamFailed || (exitCode !== 0 && !forcedRestart)) {
+ /* eslint-enable @typescript-eslint/no-unnecessary-condition */
// The HMR-specific "auto-restart suppressed" note is only
</file context>
| // handle drops. | ||
| void (async () => { | ||
| try { | ||
| await hmr.dispose(); |
There was a problem hiding this comment.
P2: An EADDRINUSE launch can still remain alive indefinitely when Rolldown's watcher.close() wedges: rejection only sets the CLI exit code, while this detached close retains the watcher handle. Add bounded pre-bind teardown with a forced-exit fallback (or otherwise ensure this failure path cannot wait forever).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/arkor/src/cli/commands/dev.ts, line 525:
<comment>An EADDRINUSE launch can still remain alive indefinitely when Rolldown's `watcher.close()` wedges: rejection only sets the CLI exit code, while this detached close retains the watcher handle. Add bounded pre-bind teardown with a forced-exit fallback (or otherwise ensure this failure path cannot wait forever).</comment>
<file context>
@@ -478,6 +510,23 @@ export async function runDev(options: DevOptions = {}): Promise<void> {
+ // handle drops.
+ void (async () => {
+ try {
+ await hmr.dispose();
+ } catch {
+ // best-effort teardown on an already-failing launch
</file context>
| // checkpoint racing in) observes `earlyStopRequested === | ||
| // false` and doesn't issue a duplicate cancel or settle | ||
| // the same deferred twice. | ||
| earlyStopRequested = false; |
There was a problem hiding this comment.
P2: In the checkpoint-triggered early-stop branch you flip earlyStopRequested = false before the async cancel(), but the early-stop timeout timer stays armed until settleEarlyStopLatch() runs after the cancel resolves. If the checkpoint lands just before the timeout boundary and the cancel POST is still pending when the timer fires, the timer sees earlyStopRequested === false, calls settleEarlyStopLatch() first, and nulls earlyStopDeferred (and resolves it as a clean success). Then, if the cancel then fails, the checkpoint branch's failure path dereferences earlyStopDeferred.timer / .reject(...) on the nulled slot and throws a TypeError — and because the timer already resolved the deferred, the shutdown handler can exit 0 for a cancel that actually failed, masking the failure this round was meant to surface. Consider clearing the timer when the checkpoint branch takes ownership of the early-stop, and guarding the failure path against an already-nulled deferred.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/arkor/src/core/trainer.ts, line 428:
<comment>In the checkpoint-triggered early-stop branch you flip `earlyStopRequested = false` before the async `cancel()`, but the early-stop timeout timer stays armed until `settleEarlyStopLatch()` runs after the cancel resolves. If the checkpoint lands just before the timeout boundary and the cancel POST is still pending when the timer fires, the timer sees `earlyStopRequested === false`, calls `settleEarlyStopLatch()` first, and nulls `earlyStopDeferred` (and resolves it as a clean success). Then, if the cancel then fails, the checkpoint branch's failure path dereferences `earlyStopDeferred.timer` / `.reject(...)` on the nulled slot and throws a TypeError — and because the timer already resolved the deferred, the shutdown handler can exit 0 for a cancel that actually failed, masking the failure this round was meant to surface. Consider clearing the timer when the checkpoint branch takes ownership of the early-stop, and guarding the failure path against an already-nulled deferred.</comment>
<file context>
@@ -419,6 +419,13 @@ export function createTrainer(
+ // checkpoint racing in) observes `earlyStopRequested ===
+ // false` and doesn't issue a duplicate cancel or settle
+ // the same deferred twice.
+ earlyStopRequested = false;
// Capture the cancel error (if any) but DON'T swallow
// silently; propagate via the deferred's reject path so
</file context>
| earlyStopRequested = false; | |
| earlyStopRequested = false; | |
| if (earlyStopDeferred.timer) clearTimeout(earlyStopDeferred.timer); |
| }, 250); | ||
| } | ||
|
|
||
| runRef.current = run; |
There was a problem hiding this comment.
P3: The deferred-restart path invokes the current render's run through runRef, and runRef.current is assigned directly during render (after the run declaration). React discourages writing refs during render — the assignment can be discarded or land on an aborted render in StrictMode/concurrent flows, which is only safe here because every run closure reaches all its state through refs/setState. To make this robust rather than accidentally-correct, move the assignment into a mount effect (e.g. an empty-deps useEffect(() => { runRef.current = run; })), or refactor so the effect re-subscribes rather than mutating during render. No behavioral bug today; this is a fragility/convention note.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/studio-app/src/components/RunTraining.tsx, line 727:
<comment>The deferred-restart path invokes the current render's `run` through `runRef`, and `runRef.current` is assigned directly during render (after the `run` declaration). React discourages writing refs during render — the assignment can be discarded or land on an aborted render in StrictMode/concurrent flows, which is only safe here because every `run` closure reaches all its state through refs/setState. To make this robust rather than accidentally-correct, move the assignment into a mount effect (e.g. an empty-deps `useEffect(() => { runRef.current = run; })`), or refactor so the effect re-subscribes rather than mutating during render. No behavioral bug today; this is a fragility/convention note.</comment>
<file context>
@@ -616,10 +724,13 @@ export function RunTraining() {
}, 250);
}
+ runRef.current = run;
+
function stop() {
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a495476dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entry.spawnArtifactContentHash = nextArtifactContentHash; | ||
| hotSwapTargets.push(target); |
There was a problem hiding this comment.
When SIGUSR2 is delivered but the child's asynchronous bundle import or callback replacement later fails, this assignment still records the new artifact as loaded and immediately reports the child in hotSwapTargets. A subsequent identical-byte rebuild then hits the artefactsAgree fast path and sends no retry, while the SPA has already claimed the callbacks were hot-swapped even though the live trainer still uses the old ones. Only update the loaded-content hash and report success after a child acknowledgement, or retain a pending state that retries failed reloads.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
| attachTrainerInspection(trainer, () => ({ | ||
| name: input.name, | ||
| config, | ||
| callbacks: currentCallbacks, |
There was a problem hiding this comment.
When a user edits only TrainerInput.name, the inspection exposes the new name separately but hashes only config; buildJobConfig() excludes the name because start() sends it as a separate createJob field. The resulting unchanged configHash routes the rebuild through SIGUSR2, which replaces only callbacks, so the current cloud job keeps its old name while Studio's refreshed manifest displays the new one and no restart ever applies the edit. Include every cloud-side creation input, including the run name, in the restart fingerprint.
AGENTS.md reference: AGENTS.md:L82-L86
Useful? React with 👍 / 👎.
| const forcedRestart: boolean = | ||
| restartPendingRef.current && forcedKillLatchRef.current; | ||
| if (streamFailed || (exitCode !== 0 && !forcedRestart)) { |
There was a problem hiding this comment.
When a rebuild's SIGTERM races a job that is already completing or failing naturally, the server can latch a restart target while the child still exits with code 0 through the ordinary runTrainer() completion path. This condition treats that generic zero exit as proof that HMR early-stop cancelled the cloud job and automatically starts another full training run, even though the previous experiment merely reached its terminal state and should remain finished; the same race can repeatedly re-run a naturally failed job. Require an explicit runner marker or acknowledgement that the zero exit came from the HMR early-stop path rather than accepting any exit=0.
AGENTS.md reference: AGENTS.md:L87-L89
Useful? React with 👍 / 👎.
This pull request migrates the Arkor build and development workflow from esbuild to Rolldown, introduces a robust hot module replacement (HMR) system for the Studio dev server, and refines the cleanup and shutdown logic for long-lived resources. Documentation has been updated to explain the new HMR behavior in both English and Japanese, and dependency management has been adjusted accordingly. These changes streamline the developer experience, enable live code updates during training runs, and improve resource handling during shutdown.
Build system migration and HMR integration:
arkor build, ensuring bundles target the current Node version and keeping external dependencies unbundled. (packages/arkor/package.json,packages/arkor/src/cli/commands/build.ts,packages/arkor/src/core/rolldownConfig) [1] [2] [3]src/arkor/in dev mode, enabling HMR for trainer code. The watcher notifies the SPA via SSE (/api/dev/events), and the dev server can hot-swap callbacks or gracefully early-stop training runs based on config changes. (packages/arkor/src/cli/commands/dev.ts,packages/arkor/src/studio/hmr.ts,packages/arkor/src/studio/trainRegistry.ts) [1] [2] [3] [4] [5] [6]Cleanup and shutdown improvements:
registerCleanupHookutility to handle resource cleanup on process exit and signals, ensuring proper teardown order (e.g., HMR watcher before token file removal). (packages/arkor/src/cli/cleanupHooks.ts,packages/arkor/src/cli/commands/dev.ts) [1] [2]Documentation updates:
docs/concepts/studio.mdx,docs/ja/concepts/studio.mdx,AGENTS.md) [1] [2] [3]AGENTS.md)Dependency and test adjustments:
packages/arkor/package.json,packages/arkor/src/cli/commands/start.test.ts) [1] [2]Security and internal API notes:
Symbol.forbrands, not exposed on the public SDK surface. (AGENTS.md) [1] [2]Summary by CodeRabbit
New Features
Bug Fixes
Documentation