feat(dev): target sessions outside workspace - #65
Conversation
patoles
left a comment
There was a problem hiding this comment.
Thanks for this one! I tested it locally and the core behavior all works: bare pnpm dev, relative and absolute paths, the env fallback in the relay, and the early validation errors. One real issue to fix before merge, one strong suggestion, and a few small extras.
1. The dev servers orphan when the wrapper is killed (fix before merge)
The launcher installs error and exit handlers on the child but never forwards signals to it. Interactive Ctrl-C is fine (the terminal signals the whole process group), but kill <pid> on the wrapper, an IDE stop button, or any parent-only SIGTERM kills dev.ts and leaves the concurrently subtree running, with ports 3000 and 3001 still held. I reproduced this locally. The old inline script didn't have the problem because concurrently was the top-level process. The error handler has the same gap: it calls process.exit(1) without killing the child.
The fix is small:
process.on('SIGINT', () => runner.kill('SIGINT'))
process.on('SIGTERM', () => runner.kill('SIGTERM'))and a runner.kill() in the error handler before exiting.
2. Tests: the code is already shaped for them
resolveDevWorkspace, launchDev, and resolveRelayWorkspace are all exported with injectable cwd/env/spawnProcess params, which is exactly the structure a unit test needs, and the root test glob (scripts/**/*.test.ts) would pick up a scripts/dev.test.ts automatically in CI. Since the seams are already there, a small test file covering the -- stripping, the three validation errors, the env precedence in resolveRelayWorkspace (arg over AGENT_FLOW_WORKSPACE over cwd), and the argv/env that launchDev passes to spawn would lock all of this in. Without tests the DI params are dead weight.
3. Small extras
AGENT_FLOW_WORKSPACEdeserves a line in the README next toAGENT_FLOW_RUNTIMEandAGENT_FLOW_TELEMETRY. It's a real public seam now (AGENT_FLOW_WORKSPACE=/path pnpm run dev:relayworks), but nothing documents it.- The
args[0] === '--' ? ...parsing appears three times (twice indev.ts, once inrelay-workspace.ts). A tiny shared helper would keep them from drifting. - The README says relative paths resolve "from the directory where you run the command"; that's true under pnpm (which sets
INIT_CWD) but not when runningnode scripts/dev.tsdirectly. Maybe soften to "when run via pnpm". createDevCommandsonly uses its argument for theenvfield; the two command strings are constants. Inlining it would trim the file a bit.
For what it's worth I did weigh whether the launcher is needed at all, since the relay reads the env var and pnpm run dev:relay -- /path already works. I think it earns its place with the upfront validation and the nicer UX, so no objection to the approach, just the signal handling.
Happy to re-review as soon as item 1 is in. Thanks again!
What does this PR do?
pnpm dev -- <directory>now lets Agent Flow observe sessions from a chosen directory.Until now, development always observed the current repository directory. That made it difficult to validate a fix against a session running in another workspace. The command now validates the target directory, forwards it safely to the relay, and keeps bare
pnpm devtargeting the repository root.How to test
pnpm dev -- /path/to/other/workspace.pnpm devand confirm it still targets this repository.pnpm test.Checklist