Implement config saving logic in gateway server - #5
Conversation
- Add `saveConfig` method to `GatewayServer` using `node:fs/promises`. - Update `init_config` request handler to save payload to `scholar.json`. - Ensure success response and event broadcast occur after successful write. - Add error handling for configuration persistence. - Document design choices in `DESIGN_CHOICES.md`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded design documentation for config persistence strategy and implemented async file-based persistence in the gateway server. The server now saves received Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Review Summary by QodoImplement configuration persistence in gateway server
WalkthroughsDescription• Implement config persistence to scholar.json file • Add saveConfig method using node:fs/promises • Update init_config handler with async/await pattern • Include error handling for file I/O operations • Document design choices and rationale Diagramflowchart LR
A["init_config request"] --> B["saveConfig method"]
B --> C["writeFile to scholar.json"]
C --> D["Success response"]
D --> E["Broadcast config_updated event"]
C --> F["Error handling"]
F --> G["Error response to client"]
File Changes1. src/daemon/gateway/server.ts
|
Code Review by Qodo
1. Unauthenticated config overwrite
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| case 'init_config': | ||
| console.log('Received config init payload:', request.payload); | ||
| // Implementation for saving the config logic would go here | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
|
|
||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); | ||
| this.saveConfig(request.payload) | ||
| .then(() => { | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to save config:', err); | ||
| ws.send(JSON.stringify(createResponse(request.id, false, undefined, `Failed to save config: ${err.message}`))); | ||
| }); |
There was a problem hiding this comment.
1. Unauthenticated config overwrite 🐞 Bug ⛨ Security
init_config persists client-provided payloads to scholar.json without any authentication/authorization, so any connected client can overwrite local configuration (including secret fields like API keys/tokens). This enables unauthorized configuration tampering via the Gateway WebSocket.
Agent Prompt
## Issue description
`init_config` writes arbitrary client-provided data to `scholar.json` without any authentication/authorization. Any connected WebSocket client can therefore persist attacker-controlled config (including credentials), which is a security vulnerability.
## Issue Context
The Gateway currently accepts connections and processes requests without identifying the caller. The `ScholarConfig` schema includes sensitive fields (API keys/tokens), increasing the impact of an unauthorized overwrite.
## Fix Focus Areas
- src/daemon/gateway/server.ts[10-33]
- src/daemon/gateway/server.ts[44-67]
- src/daemon/gateway/server.ts[83-87]
## Suggested fix (implementation guidance)
- Restrict exposure by binding the server to loopback only (e.g., set `host: '127.0.0.1'` / `localhost`) and/or validating the remote address.
- Add an authentication mechanism for mutating commands (at minimum `init_config`), e.g.:
- Require a pre-shared token from env (`SCHOLAR_DAEMON_TOKEN`) and validate it in the request payload/headers before calling `saveConfig`.
- Optionally enforce an Origin/Host allowlist if used from browsers.
- Validate that `request.payload` conforms to `ScholarConfig` before persisting.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| this.saveConfig(request.payload) | ||
| .then(() => { | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to save config:', err); | ||
| ws.send(JSON.stringify(createResponse(request.id, false, undefined, `Failed to save config: ${err.message}`))); | ||
| }); |
There was a problem hiding this comment.
2. Send on closed socket 🐞 Bug ☼ Reliability
The init_config handler calls ws.send() after an async file write without checking ws.readyState, so a client disconnect during the write can make ws.send() throw. The error-path ws.send() in .catch() is also unguarded, which can lead to an unhandled rejection and potentially crash the server.
Agent Prompt
## Issue description
`ws.send()` is called in the `init_config` `.then()` and `.catch()` without verifying the socket is still open. Because the send happens after an awaited file write, the client may have disconnected, causing `ws.send()` to throw; the `.catch()` send is currently unprotected by any outer error handler.
## Issue Context
`broadcastEvent()` already guards sends with `client.readyState === WebSocket.OPEN`, but `init_config` responses do not.
## Fix Focus Areas
- src/daemon/gateway/server.ts[51-63]
- src/daemon/gateway/server.ts[69-77]
## Suggested fix (implementation guidance)
- Before calling `ws.send`, check `ws.readyState === WebSocket.OPEN`.
- Wrap `ws.send` calls in `try/catch`, or use the callback form `ws.send(data, (err) => ...)` and log errors.
- Consider centralizing send logic into a helper like `safeSend(ws, msg)` to avoid repeating this pattern.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Implements persistence for init_config requests handled by the Gateway WebSocket server by writing the received configuration to scholar.json before responding and broadcasting config_updated.
Changes:
- Added
saveConfighelper that writes the incoming config payload to./scholar.json. - Updated
init_configrequest handling to write the file before sending the success response and broadcastingconfig_updated, with a basic error response on failure. - Added
DESIGN_CHOICES.mddocumenting the persistence and error-handling approach.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/daemon/gateway/server.ts | Adds config persistence via writeFile and wires it into the init_config request flow. |
| DESIGN_CHOICES.md | Documents the chosen persistence location and async error-handling strategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private async saveConfig(config: any) { | ||
| const configPath = './scholar.json'; | ||
| await writeFile(configPath, JSON.stringify(config, null, 4), 'utf-8'); | ||
| console.log(`Config saved to ${configPath}`); |
There was a problem hiding this comment.
configPath is a relative path, so the daemon will write scholar.json relative to the process working directory, which may not be the project root (e.g., when started via a service manager). Consider resolving an absolute path for the intended root (e.g., via path.resolve(...) from a known base) to match the stated behavior.
| case 'init_config': | ||
| console.log('Received config init payload:', request.payload); | ||
| // Implementation for saving the config logic would go here | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
|
|
||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); | ||
| this.saveConfig(request.payload) | ||
| .then(() => { | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); |
There was a problem hiding this comment.
request.payload is optional per RequestMessage, but saveConfig assumes it can always be stringified and written. If payload is missing/invalid, JSON.stringify can produce undefined and writeFile will throw a TypeError. Consider validating request.payload (and ideally its shape) before calling saveConfig, and return a clear client-facing error like "init_config requires a config payload".
| this.saveConfig(request.payload) | ||
| .then(() => { | ||
| ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' }))); | ||
| // Broadcast an event to all connected clients that config changed | ||
| this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' }); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to save config:', err); | ||
| ws.send(JSON.stringify(createResponse(request.id, false, undefined, `Failed to save config: ${err.message}`))); | ||
| }); |
There was a problem hiding this comment.
In the async init_config flow, ws.send(...) is called inside the Promise callbacks without checking ws.readyState. If the client disconnects before the write completes, ws.send can throw, and the .catch handler may also throw when trying to send the error response. Consider guarding sends with if (ws.readyState === WebSocket.OPEN) (similar to broadcastEvent) or wrapping sends in a try/catch.
| const configPath = './scholar.json'; | ||
| await writeFile(configPath, JSON.stringify(config, null, 4), 'utf-8'); | ||
| console.log(`Config saved to ${configPath}`); |
There was a problem hiding this comment.
Writing directly to scholar.json with writeFile risks leaving a partially-written/truncated config if the process crashes mid-write. For a config file that controls startup/runtime behavior, consider an atomic write strategy (write to a temp file in the same directory, then rename it over the target).
|
|
||
| ## Error Handling in WebSocket Requests | ||
| - **Choice**: Use `.then().catch()` for asynchronous file operations within the request handler. | ||
| - **Rationale**: This prevents the server from hanging if an error occurs during file I/O and allows sending a meaningful error response to the client. |
There was a problem hiding this comment.
The rationale says using .then().catch() "prevents the server from hanging" during file I/O. The server/event loop won't inherently hang with async/await either; the key benefit here is ensuring errors are handled and a response is sent back. Consider rewording this to focus on handled rejections / returning an error response rather than implying .then().catch() prevents hangs.
| - **Rationale**: This prevents the server from hanging if an error occurs during file I/O and allows sending a meaningful error response to the client. | |
| - **Rationale**: This ensures that promise rejections from file I/O are explicitly handled so the server can always send a meaningful error response to the client instead of leaving the request unresolved. |
This change implements the logic to save the updated configuration to a persistent
scholar.jsonfile in the project root whenever aninit_configrequest is received by the Gateway server. The implementation ensures that the file is written before broadcasting theconfig_updatedevent and sending a success response to the client. It also includes error handling for the file I/O operations.PR created automatically by Jules for task 4704657250173284408 started by @Ayush-Vish
Summary by CodeRabbit
Documentation
New Features