Skip to content

feat(dev): target sessions outside workspace - #65

Open
mirland wants to merge 1 commit into
patoles:mainfrom
mirland:feat-configure-workspace-path
Open

feat(dev): target sessions outside workspace#65
mirland wants to merge 1 commit into
patoles:mainfrom
mirland:feat-configure-workspace-path

Conversation

@mirland

@mirland mirland commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

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 dev targeting the repository root.

How to test

  1. Run pnpm dev -- /path/to/other/workspace.
  2. Confirm the relay reports that directory as its workspace.
  3. Run pnpm dev and confirm it still targets this repository.
  4. Run pnpm test.

Checklist

@patoles patoles left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_WORKSPACE deserves a line in the README next to AGENT_FLOW_RUNTIME and AGENT_FLOW_TELEMETRY. It's a real public seam now (AGENT_FLOW_WORKSPACE=/path pnpm run dev:relay works), but nothing documents it.
  • The args[0] === '--' ? ... parsing appears three times (twice in dev.ts, once in relay-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 running node scripts/dev.ts directly. Maybe soften to "when run via pnpm".
  • createDevCommands only uses its argument for the env field; 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants