fix: preserve resume target when spawning agent#74
Conversation
📝 WalkthroughWalkthroughAdds ChangesResume Session Forwarding
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardServer
participant BrokerProxy
participant BrokerServer
Client->>DashboardServer: POST /api/spawn { name, continueFrom }
DashboardServer->>BrokerProxy: forward request
BrokerProxy->>BrokerProxy: extract continueFrom from rawBody
BrokerProxy->>BrokerServer: POST /api/spawn { name, continueFrom }
BrokerServer-->>BrokerProxy: { success: true }
BrokerProxy-->>Client: response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements forwarding of the continueFrom parameter in the broker proxy and mock routes to ensure session context is preserved when resuming, accompanied by integration tests. The feedback highlights that replacing res.sendFile with fs.readFileSync in utils.ts introduces synchronous, blocking file system operations that can degrade server performance under concurrent load, and recommends reverting to res.sendFile or using asynchronous alternatives.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ): void { | ||
| if (fs.existsSync(filePath)) { | ||
| res.sendFile(path.resolve(filePath)); | ||
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); |
There was a problem hiding this comment.
Using synchronous file system operations (fs.readFileSync) inside an Express request handler or response helper blocks the Node.js event loop. This can severely degrade server performance and throughput under concurrent load.
Unless there is a specific reason to avoid res.sendFile, it is highly recommended to keep using it as it is asynchronous, non-blocking, and handles streaming and caching headers efficiently. If you must read the file manually, please use asynchronous file system methods (e.g., fs.promises.readFile).
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); | |
| res.sendFile(path.resolve(filePath)); |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Architect spawn route does not forward continueFrom
The /api/spawn route at packages/dashboard-server/src/routes/broker-proxy.ts:112-118 now explicitly extracts and forwards continueFrom. However, the /api/spawn/architect route at packages/dashboard-server/src/routes/broker-proxy.ts:127-144 does NOT do the same — it uses ...rawBody which would pass through continueFrom if present, but without the type validation applied to the regular spawn route. This inconsistency may be intentional (architects may not support resume) or an oversight. If the architect route should also support session resumption, it should get the same explicit handling.
(Refers to lines 127-144)
Was this helpful? React with 👍 or 👎 to provide feedback.
| ): void { | ||
| if (fs.existsSync(filePath)) { | ||
| res.sendFile(path.resolve(filePath)); | ||
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); |
There was a problem hiding this comment.
🚩 File serving changed from async to synchronous, losing Express built-in features
The change from res.sendFile(path.resolve(filePath)) to res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')) has several behavioral implications:
- Blocking I/O:
fs.readFileSyncblocks the event loop during file reads. For small HTML files in a dashboard this is unlikely to matter, but it's a regression from the asyncsendFile. - Lost Express features:
res.sendFile()automatically handles ETag generation, Last-Modified headers, conditional GET (304 responses), and graceful error handling (calls next(err) on failure). The new code loses all of these. - Error handling: If the file is deleted between
fs.existsSync(line 219) andfs.readFileSync(line 220), the old code would trigger Express error handling; the new code throws an unhandled exception that could crash the process.
For a dashboard serving a handful of small HTML files, these are unlikely to cause real problems, but the tradeoffs should be intentional.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (res.status === 200) { | ||
| const data = await res.json(); | ||
| // BUG: The response doesn't indicate whether session was resumed | ||
| // because the server never processes continueFrom | ||
| expect(data.success).toBeDefined(); | ||
| expect(data.continueFrom).toBe('test-resume-agent'); | ||
| expect(data.resumed).toBe(true); | ||
| } |
There was a problem hiding this comment.
🚩 Bug-7 test assertions are conditional on status 200, silently passing on failure
The test at test/bugs/bug7-resume-session.test.ts:64-69 wraps its key assertions (expect(data.continueFrom).toBe(...) and expect(data.resumed).toBe(true)) inside if (res.status === 200). If the server returns any non-200 status (e.g., 501 in standalone mode, or 502 if the broker is down), the test silently passes without verifying anything. This means the test provides no guarantee that the fix actually works — it only checks the fix if the server happens to respond with 200. The test should either assert the status is 200 first, or restructure to not conditionally skip assertions.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dashboard-server/src/lib/utils.ts`:
- Around line 219-221: The HTML fallback path is using a synchronous disk read
via fs.readFileSync, which blocks the event loop and bypasses streaming/caching
behavior. Update the response logic in the utils.ts HTML-serving handler to use
res.sendFile() again for the filePath fallback, with an error callback for
missing/failed files, or replace the sync read with async I/O plus caching if
sendFile is not suitable. Keep the existing existence check flow but remove the
synchronous read/send pattern.
In `@test/bugs/bug7-resume-session.test.ts`:
- Around line 66-69: The resume-session test is still allowing non-200 responses
to bypass the regression checks. Update the test in bug7-resume-session.test.ts
so the status assertion is performed outside the res.status === 200 guard, and
make the body parsing/assertions for data.success, data.continueFrom, and
data.resumed run unconditionally in the relevant test block. Use the existing
test case around the resume session flow to locate and move the checks without
changing the intended expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4852c525-39b9-453f-a859-2502abb0addd
📒 Files selected for processing (5)
packages/dashboard-server/src/lib/utils.tspackages/dashboard-server/src/mocks/routes.tspackages/dashboard-server/src/proxy-server.test.tspackages/dashboard-server/src/routes/broker-proxy.tstest/bugs/bug7-resume-session.test.ts
| if (fs.existsSync(filePath)) { | ||
| res.sendFile(path.resolve(filePath)); | ||
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); | ||
| return; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the function/call sites with line numbers.
git ls-files packages/dashboard-server/src/lib/utils.ts packages/dashboard-server/src/proxy-server.ts
echo "---- utils.ts outline ----"
ast-grep outline packages/dashboard-server/src/lib/utils.ts --view expanded || true
echo "---- proxy-server.ts outline ----"
ast-grep outline packages/dashboard-server/src/proxy-server.ts --view expanded || true
echo "---- utils.ts relevant lines ----"
sed -n '180,260p' packages/dashboard-server/src/lib/utils.ts | cat -n
echo "---- proxy-server.ts relevant routes ----"
sed -n '430,520p' packages/dashboard-server/src/proxy-server.ts | cat -nRepository: AgentWorkforce/relay-dashboard
Length of output: 9779
🌐 Web query:
Express res.sendFile documentation ETag Last-Modified range handling callback
💡 Result:
The res.sendFile method in Express is used to transfer a file at a given path, automatically setting the Content-Type header based on the file extension [1][2]. ETag and Last-Modified Handling Express automatically handles ETag generation and Last-Modified headers when using res.sendFile [3][4]. These mechanisms allow browsers and clients to perform conditional requests, improving performance by validating whether a cached version of the file is still current. Range Requests The method includes built-in support for HTTP Range requests [4]. This allows clients to request specific portions of a file, which is essential for tasks like resuming interrupted downloads or streaming media. Support for range requests is enabled by default [5]. In the options object, you can configure the acceptRanges property (defaults to true) to control this behavior [1][2]. Callback Function The res.sendFile method accepts an optional callback function, fn(err), which is invoked when the transfer is complete or if an error occurs [1][6]. Important Behaviors: 1. Error Handling: If a callback is provided and an error occurs, you are responsible for handling the response process [1][2]. This means you must explicitly end the request-response cycle (e.g., via res.end or res.status.send) or pass control to the next route using next(err) [1][2]. 2. Default Behavior: If you do not provide a callback, Express will automatically pass errors to your error-handling middleware via next(err) [2][4]. Usage Example: app.get('/file/:name', (req, res, next) => { const options = { root: path.join(dirname, 'public'), dotfiles: 'deny' }; res.sendFile(req.params.name, options, (err) => { if (err) { // You must handle the error explicitly when a callback is provided next(err); } else { console.log('Transfer complete'); } }); }); [1], [2] Official Express API documentation for res.sendFile [3] Express repository documentation on ETag support [5] Express release notes regarding range handling [4] Technical overview of file operation error handling in Express
Citations:
- 1: https://expressjs.com/en/5x/api/response/
- 2: https://expressjs.com/en/4x/api/response/
- 3: response: add support for ETag option in res.sendFile expressjs/express#6073
- 4: https://deepwiki.com/expressjs/express/5.2-file-operations
- 5: https://github.com/expressjs/express/releases/tag/4.14.0
- 6: https://github.com/expressjs/expressjs.com/blob/gh-pages/_includes/api/en/5x/res-sendFile.md
Avoid synchronous reads for these HTML responses. res.sendFile() streams the file and preserves ETag/Last-Modified/Range handling, while fs.readFileSync() blocks the event loop on every request and re-reads from disk. Switch back to sendFile() with an error callback for the fallback, or use async I/O plus caching.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 219-219: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(filePath), 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-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/dashboard-server/src/lib/utils.ts` around lines 219 - 221, The HTML
fallback path is using a synchronous disk read via fs.readFileSync, which blocks
the event loop and bypasses streaming/caching behavior. Update the response
logic in the utils.ts HTML-serving handler to use res.sendFile() again for the
filePath fallback, with an error callback for missing/failed files, or replace
the sync read with async I/O plus caching if sendFile is not suitable. Keep the
existing existence check flow but remove the synchronous read/send pattern.
| expect(data.success).toBeDefined(); | ||
| expect(data.continueFrom).toBe('test-resume-agent'); | ||
| expect(data.resumed).toBe(true); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target test and nearby related files.
git ls-files test/bugs/bug7-resume-session.test.ts
wc -l test/bugs/bug7-resume-session.test.ts
cat -n test/bugs/bug7-resume-session.test.ts | sed -n '1,220p'
# Find the endpoint handler and any resume-session related code paths.
rg -n "continueFrom|resumed|resume-session|res.status === 200|status === 200" test src .Repository: AgentWorkforce/relay-dashboard
Length of output: 11363
Fail the test on non-200 responses. The continueFrom / resumed assertions are still inside if (res.status === 200), so a bad response can skip the regression check entirely. Move the status assertion outside the guard and read the body unconditionally.
🤖 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 `@test/bugs/bug7-resume-session.test.ts` around lines 66 - 69, The
resume-session test is still allowing non-200 responses to bypass the regression
checks. Update the test in bug7-resume-session.test.ts so the status assertion
is performed outside the res.status === 200 guard, and make the body
parsing/assertions for data.success, data.continueFrom, and data.resumed run
unconditionally in the relevant test block. Use the existing test case around
the resume session flow to locate and move the checks without changing the
intended expectations.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 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="test/bugs/bug7-resume-session.test.ts">
<violation number="1" location="test/bugs/bug7-resume-session.test.ts:67">
P2: The key regression assertions (`continueFrom`, `resumed`) are inside `if (res.status === 200)`, so a non-200 response causes the test to pass silently without verifying any resume behavior. This defeats the purpose of the regression test. Assert `expect(res.status).toBe(200)` unconditionally before reading the body, or restructure so the assertions are not conditionally skipped.</violation>
</file>
<file name="packages/dashboard-server/src/lib/utils.ts">
<violation number="1" location="packages/dashboard-server/src/lib/utils.ts:220">
P2: Replacing Express `sendFile()` with `fs.readFileSync()` on the request path blocks the event loop and buffers the entire file into memory before sending, which is a performance regression on frequently hit routes like `/app` and `/metrics` compared to the prior streaming behavior.</violation>
</file>
<file name="packages/dashboard-server/src/routes/broker-proxy.ts">
<violation number="1" location="packages/dashboard-server/src/routes/broker-proxy.ts:112">
P2: The explicit `continueFrom` type validation added here is not applied to the `/api/spawn/architect` route, which uses a raw body spread. If architect sessions should also support resume, the same validation should be applied there; otherwise `continueFrom` could pass through unvalidated. Consider whether this inconsistency is intentional.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // BUG: The response doesn't indicate whether session was resumed | ||
| // because the server never processes continueFrom | ||
| expect(data.success).toBeDefined(); | ||
| expect(data.continueFrom).toBe('test-resume-agent'); |
There was a problem hiding this comment.
P2: The key regression assertions (continueFrom, resumed) are inside if (res.status === 200), so a non-200 response causes the test to pass silently without verifying any resume behavior. This defeats the purpose of the regression test. Assert expect(res.status).toBe(200) unconditionally before reading the body, or restructure so the assertions are not conditionally skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/bugs/bug7-resume-session.test.ts, line 67:
<comment>The key regression assertions (`continueFrom`, `resumed`) are inside `if (res.status === 200)`, so a non-200 response causes the test to pass silently without verifying any resume behavior. This defeats the purpose of the regression test. Assert `expect(res.status).toBe(200)` unconditionally before reading the body, or restructure so the assertions are not conditionally skipped.</comment>
<file context>
@@ -63,22 +63,22 @@ describe('BUG 7 — Resume session loses context', () => {
- // BUG: The response doesn't indicate whether session was resumed
- // because the server never processes continueFrom
expect(data.success).toBeDefined();
+ expect(data.continueFrom).toBe('test-resume-agent');
+ expect(data.resumed).toBe(true);
}
</file context>
| ): void { | ||
| if (fs.existsSync(filePath)) { | ||
| res.sendFile(path.resolve(filePath)); | ||
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); |
There was a problem hiding this comment.
P2: Replacing Express sendFile() with fs.readFileSync() on the request path blocks the event loop and buffers the entire file into memory before sending, which is a performance regression on frequently hit routes like /app and /metrics compared to the prior streaming behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dashboard-server/src/lib/utils.ts, line 220:
<comment>Replacing Express `sendFile()` with `fs.readFileSync()` on the request path blocks the event loop and buffers the entire file into memory before sending, which is a performance regression on frequently hit routes like `/app` and `/metrics` compared to the prior streaming behavior.</comment>
<file context>
@@ -217,7 +217,7 @@ export function sendHtmlFileOrFallback(
): void {
if (fs.existsSync(filePath)) {
- res.sendFile(path.resolve(filePath));
+ res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8'));
return;
}
</file context>
| res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')); | |
| res.sendFile(path.resolve(filePath)); |
| typeof rawBody.model === 'string' ? rawBody.model : undefined, | ||
| ); | ||
| const model = parsed.model; | ||
| const continueFrom = typeof rawBody.continueFrom === 'string' ? rawBody.continueFrom : undefined; |
There was a problem hiding this comment.
P2: The explicit continueFrom type validation added here is not applied to the /api/spawn/architect route, which uses a raw body spread. If architect sessions should also support resume, the same validation should be applied there; otherwise continueFrom could pass through unvalidated. Consider whether this inconsistency is intentional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dashboard-server/src/routes/broker-proxy.ts, line 112:
<comment>The explicit `continueFrom` type validation added here is not applied to the `/api/spawn/architect` route, which uses a raw body spread. If architect sessions should also support resume, the same validation should be applied there; otherwise `continueFrom` could pass through unvalidated. Consider whether this inconsistency is intentional.</comment>
<file context>
@@ -109,11 +109,13 @@ export function registerBrokerProxyRoutes(app: Express, ctx: RouteContext): void
typeof rawBody.model === 'string' ? rawBody.model : undefined,
);
const model = parsed.model;
+ const continueFrom = typeof rawBody.continueFrom === 'string' ? rawBody.continueFrom : undefined;
return {
...rawBody,
</file context>
1 participant
Summary
continueFromwhen the dashboard server forwards/api/spawnrequests through broker proxy mode.continueFromandresumedfrom the mock spawn route so resume behavior is observable in tests.Testing
packages/dashboard-server/src/proxy-server.test.tsfor forwardingcontinueFromto the broker/api/spawnendpoint.test/bugs/bug7-resume-session.test.tsto assert resumed mock responses and static broker-proxy handling.Notes
originis read-only for the authenticated GitHub account, so this PR is opened from theship9599fork.