feat: create ppg serve command and Fastify server scaffold#94
Open
2witstudios wants to merge 3 commits intomainfrom
Open
feat: create ppg serve command and Fastify server scaffold#942witstudios wants to merge 3 commits intomainfrom
2witstudios wants to merge 3 commits intomainfrom
Conversation
- Add `ppg serve` command with --port, --host, --token, --daemon, --json options - Create Fastify server with CORS, /health endpoint, bearer token auth - Graceful shutdown on SIGTERM/SIGINT with state file cleanup - State file (serve.json) and PID file with 0o600 permissions - LAN IP detection via os.networkInterfaces() - Path helpers: serveStatePath(), servePidPath() in lib/paths.ts - Lazy import registration in cli.ts Closes #63
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI (src/cli.ts)
participant Cmd as Command (src/commands/serve.ts)
participant Server as Server (src/server/index.ts)
participant FS as File System
User->>CLI: ppg serve --port 3100 --token xyz
CLI->>Cmd: serveCommand(options)
Cmd->>Cmd: resolveProjectRoot() via git
Cmd->>FS: Verify project initialized
Cmd->>Server: startServer({projectRoot, port, host, token, json})
Server->>Server: detectLanAddress()
Server->>Server: Initialize Fastify instance
Server->>Server: Register CORS plugin
Server->>Server: Register token auth middleware
Server->>Server: Register /health GET route
Server->>FS: Write serve.json (state)
Server->>FS: Write serve.pid
Server->>CLI: Log startup info or JSON
Server->>Server: Listen on port + host
User->>Server: SIGTERM/SIGINT signal
Server->>FS: Remove state files
Server->>Server: Close server gracefully
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Line 288: Replace the loose coercion in the CLI .option for port with a strict
parser: implement a parsePort(v: string) function that converts v to a number,
validates Number.isInteger(n) and n is between 1 and 65535, and throws an Error
with a clear message if validation fails; then pass parsePort as the third
argument to the existing .option('-p, --port <number>', ..., parsePort, 3100) so
invalid ports are rejected at parse time (refer to the parsePort function and
the .option call).
In `@src/commands/serve.ts`:
- Around line 33-46: serveCommand currently ignores ServeCommandOptions.daemon;
update it to honor the flag by launching the server in background when
options.daemon is true. Implement by either (A) passing daemon through to
startServer (add a daemon parameter to startServer and propagate it from
serveCommand) or (B) spawning a detached child process that re-runs the CLI with
the same args (use child_process.spawn with detached: true, stdio: 'ignore',
unref()) and exit the parent process. Ensure you reference and modify
ServeCommandOptions, serveCommand, and startServer (or the new helper that
spawns the detached process) so the CLI's --daemon behavior is actually
implemented.
In `@src/server/index.ts`:
- Around line 113-123: Startup output currently omits the QR payload; when not
running with --json and a token exists you should generate and display a
terminal QR for the connection URL+token and include the QR payload in the
printed state when --json is used. In practice, update the block that checks
json/host/port/token (variables: json, host, port, token, lanAddress, state) so
that: if json is true, add a qr_payload field to state (the URL including token
and LAN address variant if present) and JSON.stringify(state) as before; if json
is false and token is present, call the terminal QR generator (e.g., a utility
function you add like renderTerminalQr(qr_payload)) to print the QR after the
success/info logs and also info() the plain URL; ensure behavior remains
unchanged when token is falsy (no QR generation) and that LAN address URL is
used when lanAddress is present.
- Around line 42-50: The writeStateFile and writePidFile functions only pass
mode: 0o600 to fs.writeFile which affects permissions on creation but does not
change existing file permissions; after writing the file (statePath and pidPath)
call fs.chmod(statePath, 0o600) and fs.chmod(pidPath, 0o600) respectively (await
the calls and handle errors as appropriate) to ensure existing files are
hardened to 0o600 regardless of prior permissions.
- Around line 3-10: Replace the CommonJS interop usage by removing the
createRequire import and the const require = createRequire(import.meta.url)
declaration, and instead import the package JSON directly as pkg using an ESM
JSON import (so that pkg.version is available); update any references to pkg
(currently declared via require('../../package.json')) accordingly and delete
the unused createRequire symbol and the require variable to keep the module
ESM-native.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/cli.tssrc/commands/serve.tssrc/lib/paths.tssrc/server/index.ts
Comment on lines
33
to
46
| export async function serveCommand(options: ServeCommandOptions): Promise<void> { | ||
| const projectRoot = await resolveProjectRoot(); | ||
|
|
||
| const port = options.port ?? 3100; | ||
| const host = options.host ?? '127.0.0.1'; | ||
|
|
||
| await startServer({ | ||
| projectRoot, | ||
| port, | ||
| host, | ||
| token: options.token, | ||
| json: options.json, | ||
| }); | ||
| } |
There was a problem hiding this comment.
--daemon is declared but not implemented.
ServeCommandOptions includes daemon (Line 29), but serveCommand ignores it (Line 39-Line 45). The CLI advertises daemon mode, so this currently behaves as a silent no-op.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/serve.ts` around lines 33 - 46, serveCommand currently ignores
ServeCommandOptions.daemon; update it to honor the flag by launching the server
in background when options.daemon is true. Implement by either (A) passing
daemon through to startServer (add a daemon parameter to startServer and
propagate it from serveCommand) or (B) spawning a detached child process that
re-runs the CLI with the same args (use child_process.spawn with detached: true,
stdio: 'ignore', unref()) and exit the parent process. Ensure you reference and
modify ServeCommandOptions, serveCommand, and startServer (or the new helper
that spawns the detached process) so the CLI's --daemon behavior is actually
implemented.
src/server/index.ts
Outdated
Comment on lines
3
to
10
| import { createRequire } from 'node:module'; | ||
| import Fastify from 'fastify'; | ||
| import cors from '@fastify/cors'; | ||
| import { serveStatePath, servePidPath } from '../lib/paths.js'; | ||
| import { info, success, warn } from '../lib/output.js'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const pkg = require('../../package.json') as { version: string }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and display tsconfig to check JSON import support
echo "=== Searching for tsconfig files ==="
fd tsconfig --type f
echo -e "\n=== Content of tsconfig files ==="
fd tsconfig --type f -x cat {}
echo -e "\n=== CommonJS patterns in src/server/index.ts ==="
rg -n "createRequire|require\(" src/server/index.ts -A 2Repository: 2witstudios/ppg-cli
Length of output: 958
Replace createRequire/require with ESM-native JSON import.
Lines 3, 9–10 use CommonJS interop to load the package version. Replace with:
import pkg from '../../package.json' assert { type: 'json' };Then remove the createRequire import and const require declaration. The tsconfig already enables resolveJsonModule, making this ESM-compliant approach available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/index.ts` around lines 3 - 10, Replace the CommonJS interop usage
by removing the createRequire import and the const require =
createRequire(import.meta.url) declaration, and instead import the package JSON
directly as pkg using an ESM JSON import (so that pkg.version is available);
update any references to pkg (currently declared via
require('../../package.json')) accordingly and delete the unused createRequire
symbol and the require variable to keep the module ESM-native.
Comment on lines
+42
to
+50
| async function writeStateFile(projectRoot: string, state: ServeState): Promise<void> { | ||
| const statePath = serveStatePath(projectRoot); | ||
| await fs.writeFile(statePath, JSON.stringify(state, null, 2) + '\n', { mode: 0o600 }); | ||
| } | ||
|
|
||
| async function writePidFile(projectRoot: string, pid: number): Promise<void> { | ||
| const pidPath = servePidPath(projectRoot); | ||
| await fs.writeFile(pidPath, String(pid) + '\n', { mode: 0o600 }); | ||
| } |
There was a problem hiding this comment.
Harden file permissions for existing serve.json/serve.pid.
Line 44 and Line 49 pass mode: 0o600, but this only applies on file creation. If files already exist, prior permissions may persist.
Suggested fix
async function writeStateFile(projectRoot: string, state: ServeState): Promise<void> {
const statePath = serveStatePath(projectRoot);
await fs.writeFile(statePath, JSON.stringify(state, null, 2) + '\n', { mode: 0o600 });
+ await fs.chmod(statePath, 0o600);
}
async function writePidFile(projectRoot: string, pid: number): Promise<void> {
const pidPath = servePidPath(projectRoot);
await fs.writeFile(pidPath, String(pid) + '\n', { mode: 0o600 });
+ await fs.chmod(pidPath, 0o600);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/index.ts` around lines 42 - 50, The writeStateFile and
writePidFile functions only pass mode: 0o600 to fs.writeFile which affects
permissions on creation but does not change existing file permissions; after
writing the file (statePath and pidPath) call fs.chmod(statePath, 0o600) and
fs.chmod(pidPath, 0o600) respectively (await the calls and handle errors as
appropriate) to ensure existing files are hardened to 0o600 regardless of prior
permissions.
Comment on lines
+113
to
+123
| if (json) { | ||
| console.log(JSON.stringify(state)); | ||
| } else { | ||
| success(`Server listening on http://${host}:${port}`); | ||
| if (lanAddress) { | ||
| info(`LAN address: http://${lanAddress}:${port}`); | ||
| } | ||
| if (token) { | ||
| info('Bearer token authentication enabled'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Startup flow is missing the QR code output required by the issue objective.
The startup output currently prints text/JSON only; no QR payload is generated/displayed for token + connection URL.
If you want, I can draft a minimal QR integration patch (terminal QR + JSON-safe behavior under --json).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/index.ts` around lines 113 - 123, Startup output currently omits
the QR payload; when not running with --json and a token exists you should
generate and display a terminal QR for the connection URL+token and include the
QR payload in the printed state when --json is used. In practice, update the
block that checks json/host/port/token (variables: json, host, port, token,
lanAddress, state) so that: if json is true, add a qr_payload field to state
(the URL including token and LAN address variant if present) and
JSON.stringify(state) as before; if json is false and token is present, call the
terminal QR generator (e.g., a utility function you add like
renderTerminalQr(qr_payload)) to print the QR after the success/info logs and
also info() the plain URL; ensure behavior remains unchanged when token is falsy
(no QR generation) and that LAN address URL is used when lanAddress is present.
- Use crypto.timingSafeEqual() for bearer token comparison (timing attack) - Return reply from auth hook after 401 to halt request processing - Replace hand-rolled resolveProjectRoot with getRepoRoot + requireManifest - Remove unimplemented --daemon flag from CLI and options interface - Add port validation (integer, 1-65535) via parsePort helper - Remove duplicate defaults (Commander already provides them) - Handle unhandled promise rejection in signal shutdown handlers - Remove unused warn import from server/index.ts - Add tests: timingSafeTokenMatch (6), detectLanAddress (3), path helpers (2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ppg servecommand with--port,--host,--token,--daemon,--jsonoptionsGET /healthreturns{ status: "ok", uptime, version }serve.json) and PID file written with0o600permissionsos.networkInterfaces()serveStatePath(),servePidPath()inlib/paths.tscli.tsCloses #63
Test plan
npm run typecheckpasses (pre-existing TS error in spawn.test.ts is unrelated)npm test— all 218 tests passppg servestarts server on default port 3100ppg serve --port 8080 --host 0.0.0.0binds correctlyGET /healthreturns ok response--tokenflag enables bearer auth on non-health routesserve.jsonandserve.pidwritten with 0o600 permissionsSummary by CodeRabbit
servecommand to start a development server with configurable port and host settings