Skip to content

feat(server): always surface the session URL on every start - #1184

Open
technicalpickles wants to merge 2 commits into
backnotprop:mainfrom
technicalpickles:surface-session-url
Open

feat(server): always surface the session URL on every start#1184
technicalpickles wants to merge 2 commits into
backnotprop:mainfrom
technicalpickles:surface-session-url

Conversation

@technicalpickles

@technicalpickles technicalpickles commented Aug 3, 2026

Copy link
Copy Markdown

Part of #1134, criterion 1 only. Leaving the stable-port half for a separate PR as you suggested, since the abandoned-session cleanup looked like its own conversation.

Summary

  • The session URL now goes to stderr on every start, one line, URL last. It previously appeared only for remote sessions, the Codex desktop host, or a failed browser launch, so a local session whose browser opened fine printed nothing and a closed tab left no way back.
  • Pi gets the same unconditional announce via ctx.ui.notify, and the OpenCode stderr filter learned the new phrasing while still forwarding the older two-line shape from older binaries.
  • isCodexDesktopHost() is gone. It existed only to special-case printing the URL, which is now unconditional.
  • PLANNOTATOR_READY_FILE stays exactly what it was, an opt-in side channel that hosts point at their own temp file. No default, per your review. Discovery for everyone else is the stderr line plus the existing plannotator sessions registry, which is what the docs now say.

The format is deliberately boring and stable, since the point is that agents grep it:

  Plannotator session ready: http://localhost:PORT

Two leading spaces, no trailing slash. Remote adds a port-forwarding line after it, and a failed browser launch adds a "could not open" line. The URL itself appears exactly once either way.

Test plan

bun run typecheck is clean.

bun test has four failures that predate this branch, and none of them are in code it touches. Three are in apps/pi-extension/server.test.ts, where a global gpg.format=ssh config leaks into the test's temp HOME and git can't write a commit object; that file imports only ./server.ts plus the generated modules. The fourth is the git-timeout case in packages/shared/workspace-status.test.ts, which lines up with the parallel-timing flakes you mentioned: this branch doesn't touch packages/shared/ at all, and that file passes 6/6 in isolation across three consecutive runs.

New coverage in packages/server/shared-handlers.test.ts pins the URL appearing exactly once on the local, remote, and browser-failed paths, and pins the line's literal bytes including the two-space indent. apps/opencode-plugin/cli-bridge.test.ts imports SESSION_READY_LINE_PREFIX and runs a line built from it through the real filter, so drifting the constant now breaks a test instead of silently costing OpenCode users the URL.

handleServerReady printed the session URL on three paths only: a remote
session, the Codex desktop host, or a browser that failed to open. The
common case, a local session whose browser opened fine, printed nothing,
so a closed tab or a second machine left neither the user nor the agent
driving the session any way back to it.

Two changes, in the order the issue lays out:

1. One unconditional stderr line on every start, "Plannotator session
   ready: <url>". One line, URL last, stable format, because agents grep
   it. The remote and failed-launch branches now add context lines and
   never repeat the URL. The Codex desktop case existed solely to print
   the URL when the browser opened, so it and isCodexDesktopHost go with
   it.

2. PLANNOTATOR_READY_FILE now defaults to <dataDir>/ready.jsonl. The
   side channel already worked, but only amp and the OpenCode bridge set
   it; Claude Code spawns the CLI straight from a hook, so nothing set
   the variable and the metadata the server already publishes landed
   nowhere. apps/hook calls ensureReadyFileEnv() at startup, which
   leaves a host-supplied path alone, so tail -n 1 ready.jsonl is the
   newest session.

The Pi runtime mirrors change 1 through its own surface: it owns the
terminal, so openBrowserForServer notifies unconditionally instead of
writing to stderr. Pi never publishes a ready file, so change 2 has no
counterpart there.

Also: the OpenCode stderr forwarder keeps forwarding the remote
follow-up line under its new phrasing, ready.jsonl joins the uninstall
purge list, and the env-var reference plus the troubleshooting "lost a
tab" section document both recovery routes.

Criterion 2 of the issue, a stable port across plan resubmissions, is
deliberately not here. It is a separate PR.

Refs backnotprop#1134
@backnotprop

Copy link
Copy Markdown
Owner

Review (at 7ae3d3cf)

Verdict: needs changes, and the core of the PR is genuinely good. The unconditional stderr line is correct, protocol-safe, and well executed: the reviewer ran the real PermissionRequest hook path end to end with streams captured separately and stdout carried only the decision JSON, every new write goes to stderr, and the URL prints only after a successful bind with the final port (the retry path cannot double-print or print a stale port). All four Bun server types share one handleServerReady implementation, so exactly-once holds by construction there. The isCodexDesktopHost removal is safe: exactly one production caller on main, and the new line is a strict superset of what it printed. No conflict with in-flight #1183, textually or semantically. Full suite green (the four failures are the documented parallel-timing flakes; all pass in isolation).

Two things to resolve before merge.

1. The grep-stable format contract is unenforced, and drifting it silently breaks OpenCode. The new test asserts against SESSION_READY_LINE_PREFIX itself, which is self-referential: mutating the constant to different text leaves all 24 related tests green while cli-bridge.ts matches a hardcoded /^Plannotator session ready\b/, so OpenCode users would lose the URL entirely with zero test failures. The two leading spaces everything depends on are also never asserted. Cheap fix: pin the literal bytes once (expect(SESSION_READY_LINE_PREFIX).toBe("Plannotator session ready: ") plus an exact-line assertion including the indent), and add a cross-component test in cli-bridge.test.ts importing the constant, which works today since @plannotator/server is already a linked devDependency there.

2. The ready-file default needs a design decision, and this one is on us, not you. Our #1134 reply pointed you at PLANNOTATOR_READY_FILE, but the repo already has a session registry that solves the discovery problem better: ~/.plannotator/sessions/<pid>.json with dead-PID pruning, surfaced by plannotator sessions and documented in the troubleshooting page one paragraph above the one this PR edits. The new ready.jsonl default duplicates it without pid, timestamp, or liveness, grows unboundedly (confirmed: pure append, nothing trims, only uninstall clears it), and the documented tail -n 1 returns the last session started rather than the last one alive, which reintroduces the exact stale-server confusion from your transcript data. Suggestion: keep the stderr line exactly as you built it, keep PLANNOTATOR_READY_FILE as the opt-in side channel amp already uses (host-supplied value respected, which your wiring does correctly), drop the always-on default file, and have the docs point agents at the stderr line and the existing sessions registry instead. If a default file is still wanted, single-line overwrite with pid and startedAt so staleness is detectable.

Smaller items, none blocking:

  • The docs present the ready-file default as global, but only the Claude Code hook calls ensureReadyFileEnv(); Pi never publishes one and OpenCode's in-process paths bypass it. Resolves itself if the default is dropped.
  • OpenCode: the browser-failure line ("Could not open a browser automatically") matches none of the filter regexes and is dropped, where the old shape was forwarded; users get the URL but not the reason the tab never appeared. Also the poller and the stderr forwarder now race for which text wins the toast (deduped, so cosmetic).
  • Pi announces twice on remote (the new unconditional notify plus the existing sessionOpenedMessage URL embed), so the exactly-once claim does not hold there; pre-existing shape, worth one line.
  • PR body nit: it advertises http://127.0.0.1:PORT/ but the emitted URL is http://localhost:PORT, no trailing slash. The docs page is right; the body should match since byte stability is the selling point.
  • CLAUDE.md's data-dir row does not mention the new file; moot if the default goes.

On process: this is a first code contribution to the repo and it is a strong one: scoped exactly as discussed on the issue, both runtimes touched, docs and tests included, and the OpenCode compat matrix (old binary/new plugin and the reverse) was validated by the reviewer against the real filter in all six scenarios. Happy to re-review as soon as the two items above are addressed.

Review follow-up on backnotprop#1184.

Drop the always-on ready file. The default duplicated the session registry that
already exists (`packages/server/sessions.ts`, surfaced by `plannotator
sessions`) with strictly less information: no pid, no timestamp, no liveness. It
also grew unboundedly on pure append, and the documented `tail -n 1` returned
the last session *started* rather than the last one *alive*, which is exactly
the stale-server confusion this branch set out to kill. The registry already
answers the question, so only the default goes. `PLANNOTATOR_READY_FILE` stays
as the opt-in host side channel and host-supplied values behave as before, so
amp and OpenCode are unaffected.

Make the format contract real. The assertions interpolated
`SESSION_READY_LINE_PREFIX` into the expected string, so they compared the
constant against itself: retexting it left every one of those tests green while
`cli-bridge.ts` went on matching a hardcoded `/^Plannotator session ready\b/`,
which means OpenCode users would have silently lost the URL with zero test
failures. The two leading spaces in the emitted line were never asserted
either. Now the literal bytes are pinned once, the emitted line is asserted
including its indent and surrounding newlines, and a cross-component test in
`cli-bridge.test.ts` feeds a line built from the exported constant through
`formatUserFacingCliStderrLine`. Retexting the prefix fails 5 tests.

Forward the browser-launch failure in OpenCode. "Could not open a browser
automatically" matched none of the filter regexes, so the new output shape
dropped the one line that explains a tab which never appeared. The old shape
got forwarded; this was a regression introduced here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@technicalpickles

Copy link
Copy Markdown
Author

This is a better review than I gave you a PR for. Both items are fixed, and you were right on the design call.

1, the format contract. Yeah, that one stings. Asserting against SESSION_READY_LINE_PREFIX meant the test checked the constant against itself, so the one thing OpenCode actually depends on was the one thing nothing verified.

Pinned three ways now: the literal bytes, the emitted line asserted exactly with its two-space indent and newlines, and a cross-component test in cli-bridge.test.ts that imports the constant and pushes a line built from it through the real filter. That import just worked, like you said it would.

Then I mutated the constant to see what happened. Five tests go red, the cross-component one included. Reverted.

2, the ready-file default is gone. You talked me out of it. sessions.ts already carries pid and startedAt and prunes dead PIDs with process.kill(pid, 0), so ready.jsonl was a second session-state mechanism holding strictly less. Your tail -n 1 point is the one that got me, though: last session started instead of last one alive is the exact stale-server confusion I set out to kill. Single-line overwrite with pid and startedAt fixes the staleness, sure. Then it's just a worse copy of the registry. No version of it was worth keeping.

So ensureReadyFileEnv(), getDefaultReadyFilePath(), the purge entry, the apps/hook/ call site, and the docs paragraph all came out. PLANNOTATOR_READY_FILE goes back to opt-in and host-supplied, respected verbatim, which is the part amp leans on and the part that already worked. Troubleshooting now sends people to the stderr line and plannotator sessions. That covers your docs-say-global note too, since the hook was the only thing ever publishing one.

Now the part I didn't fix, before you find it yourself on re-review: the contract is pinned for the Bun runtime and OpenCode, and not for Pi. plannotator-browser.ts hardcodes Plannotator session ready: ${serverUrl} as a bare literal instead of importing the constant, because the Pi extension is a separate Node build that vendors from packages/shared/ rather than packages/server/. So Pi's copy can still drift. Moving the constant into packages/shared/ with a vendor entry would close it, but that's a real change to two runtimes and I didn't want to decide it inside this PR. Your double-announce catch is the same shape: remote does fire twice now, and which of the two should win reads like your call, not mine. Happy to take either as a follow-up.

The dropped browser-failure line was ours, and it's fixed. "Could not open a browser automatically" matched none of the filter regexes, so OpenCode users got a URL and nothing about why no tab showed up, where the old shape forwarded it. Matched and covered.

Body's fixed too. It advertised http://127.0.0.1:PORT/ while the code emits http://localhost:PORT, no trailing slash. Fair hit on a PR whose whole pitch is byte stability.

Marking it ready for review.

@technicalpickles
technicalpickles marked this pull request as ready for review August 3, 2026 20:17
@backnotprop

Copy link
Copy Markdown
Owner

Delta re-review (at d4b006bd)

Verdict from review: merge as-is. Both findings are properly fixed, and the fix came out smaller and better-reasoned than what the review asked for: the delta is net negative against the base, with the hook wiring, the uninstall entry, and the env-docs paragraph all fully reverted rather than patched around.

Verified independently rather than taken on faith:

  • The constant mutation was re-run and fails exactly the five tests you named, the cross-component one included. The exact-line assertion pins the two-space indent and surrounding newlines, and the docstring explaining why the old assertion was self-referential is the part that will stop a future refactor from quietly undoing this. Also checked something you did not claim: the barrel import in cli-bridge.test.ts stays out of the production bundle (devDependency only, and the only available route since the package exports no subpath). Correct call.
  • The ready-file removal sweep is clean: zero orphaned references repo-wide, amp's opt-in path byte-identical to main semantics, and a live server run confirms stderr is unchanged and no file is created. The replacement docs accurately describe the sessions registry's liveness behavior.
  • The six-scenario compat matrix is fully green including the newly forwarded browser-failure line, and the noise control still drops what it should. Anchoring that regex before the tail of the copy was a good instinct.
  • Full suite 2669 pass / 0 fail locally, typecheck clean, merges onto current main with no conflicts (the fix(annotate): resolve natural-language arguments or hand off to the agent #1183 overlap vanished when the hook file reverted). The pre-disclosed Pi items are untouched by this delta, as stated.

CI had not run on the branch (fork PRs need workflow approval); approved now so the merge decision has real signal. The maintainer makes the merge call from here.

The pre-disclosed Pi constant-sharing question and the remote double-announce are logged as follow-up candidates for the maintainer to prioritize.

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