You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The Pi extension could start its first LLM call before its Gortex tools were registered, and the note explaining Pi's tool renaming could be lost for the whole session.
before_agent_start was synchronous. It ran whenever Pi's agent loop reached it — which, at startup, can be while session_start's await c.start() / await registerGortexTools(pi) is still in flight, because Pi arms the editor's submit handler before it awaits rebindCurrentSession(). It then latched orientationInjected = true unconditionally (decision.orientation from the Go hook is effectively always non-empty), so piAliasNote() — which reads the not-yet-populated gortexToolNames — returned "" and was never retried.
Two consequences, both observed in a deterministic harness against the live index.ts:
The first turn ran with zero Gortex tools registered.
The model then followed Gortex's bare-name guidance into Pi's own read / edit and got a denial naming the tool it had just called.
The same shape reaches further than startup: /reload and /new reach before_agent_start before their session_start fires at all, and /reload re-imports the extension module outright.
Changes
A readiness barrier.session_start's body moves into startSession() wrapped in try/finally, so a readiness promise settles on every exit path — including the early return when a /reload supersedes the bridge mid-handshake. before_agent_start is now async and awaits it; Pi's emit() awaits each listener, so this genuinely holds the turn. Capped at READY_WAIT_MS = 15_000 — start()'s worst case is 60s + 1s backoff + 60s, which must never become first-turn latency.
The promise is armed at factory time./reload re-imports the module (resource-loader.js clears the extension cache; the loader runs jiti with moduleCache: false) and /new builds a fresh runtime, so in both cases an instance exists — and can be asked for a turn — before its own session_start runs. Arming at construction closes that window; armSessionReady() is idempotent so a turn parked in the gap holds the same promise the incoming session_start settles.
The rename now rides the tool.piAliasNote() is replaced by piAliasedDescription(), which front-loads the explanation into the aliased tool's own description. It cannot be lost to ordering: if the model can see gortex_read, it can see what gortex_read is. Non-colliding tools are untouched, and tools promoted later by tools_search get it too, since they take the same registration path.
Orientation is cleared once the context push lands, so it survives a context shape the hook cannot append to.
Testing
All tests pass (go test -race ./...)
New tests added for new functionality — none in this PR, see below
Benchmarks run if performance-relevant — not performance-relevant
Test suite
cmd/gortex/testdata/agent-render/pi.txt is regenerated in this PR.
No tests are added by this PR
The behaviour changed here lives entirely in internal/agents/pi/extension/index.ts, and there is no JS/TS test harness in the tree today; internal/agents/pi/adapter_test.go covers the Go side and stops at the file boundary, so none of the behaviour above is reachable from go test. The change is verified by a standalone harness that derives its subject from the live index.ts — resolving the four {{ SENTINEL }} placeholders exactly as the installer does, and redirecting node:child_process to a mock that controls when the simulated gortex mcp child answers tools/list. It therefore cannot drift from the source: rename a sentinel and it fails loudly.
Four scenarios, 19 assertions, exit 0/1, no dependencies:
asserts
A — before_agent_start fires mid-handshake, nothing awaits session_start
barrier held 626ms for a 300ms×2 handshake; 3 tools live at the first LLM call; orientation present; exactly one injected message
B — descriptions
read → gortex_read explains its own rename and keeps its original description; search untouched
C — registration outlives the cap (forced to 20ms against a 400ms registration)
gave up at 46ms, so no unbounded wait; turn 1 keeps the orientation; nothing injected on later turns; late-registered tools still explain the rename
D — prompt submitted in the /reload gap, module re-imported
turn held through the gap; resumed only after session_start landed; session 2 has its tools and re-injects the orientation
Pointed at a pre-fix tree the suite fails, on the right assertions — four of D's six, including that the pre-fix code loses the orientation entirely on /reload (the turn parks it, then session_start clears pendingOrientation).
That harness is not part of this PR, so the checklist item above is unchecked. Landing it means adding a second toolchain to a Go repo, and that is a decision worth taking on its own: where it runs, whether a missing node skips or fails, and whether the tree gets its first package.json. Filed separately. Happy to fold it into this PR instead if the maintainers would rather not merge the fix without it — in which case this PR should wait on that decision.
Checklist
Code follows existing patterns in the codebase
No unnecessary abstractions added
Language extractor includes Meta["methods"] for interfaces — n/a
Methods have EdgeMemberOf edges to their containing type — n/a
Scope and limits
Startup ordering is closed by construction; the rest rests on a read of Pi 0.85.1. At startup there is no await between setupEditorSubmitHandler() and emit(session_start) (interactive-mode.js:708-712 → rebindCurrentSession → bindCurrentSessionExtensions → agent-session.js:1906), so the handler's synchronous prologue always runs first. If a future Pi inserts an await there, the factory-time arming still covers it.
The 15s cap is a judgement call. A cold daemon plus warmup could exceed it, in which case the turn proceeds without tools. Warm-daemon handshake measured at 41ms, so the cap is only reachable when something is wrong.
Denial text is unchanged.pretooluse.go:568 still renders `read(target:{symbol:"<id>"})` — bare names, in Claude Code's vocabulary — and those snippets are what a model copies. With the tool descriptions in place the model can translate those names itself. The real fix is a host-vocabulary mapping at the internal/hooks/pi.go boundary. Filed separately; not in this PR.
Nice work — the diagnosis is right and the redesign is better than what it replaces. Moving the alias
explanation onto the tool's own description is the correct call; I checked the plumbing and it holds: registerOneTool passes the baredesc.name, safeRegister is what applies piAliasName to def.name, so piAliasedDescription(name, piAliasName(name), …) genuinely names the tool it is
registered under. Non-colliding tools fall through unchanged, and only read/edit collide today, so
the extra prose lands on two tools.
Two things I'd like changed before this lands, then I'm happy with it.
1. The barrier's resolver slot is shared, so a stale session_start can settle a newer session's promise
settleSessionReady is a single mutable variable that armSessionReady() reassigns. If two session_start invocations overlap and a third arms after the first settles, the older handler's finally resolves the new promise, and a turn parked on it is released while registration is still
running — the exact failure this PR exists to prevent.
I transcribed the state machine verbatim out of index.ts into a standalone node script (no Pi needed)
and drove three invocations: A slow, B fast and overlapping, C starting after B settles.
B(fast): registration finished
C: session_start begins (registration will take 400ms)
A(slow): registration finished
TURN released after 180ms
TURN saw: C STILL RUNNING
C: registration finished
Binding the resolver at handler entry fixes it:
armSessionReady();constsettle=settleSessionReady;// this invocation's resolvertry{awaitstartSession();}finally{settle();}
Same script with that change: TURN released after 400ms / TURN saw: C finished.
Whether this is live depends on something the file argues both ways about. Your new comment says /reload re-imports the module, which means a fresh instance and a fresh closure; but safeRegister's
existing comment says "a reload can re-fire session_start without resetting the registry", which
means one instance seeing two. Worth resolving that explicitly in the comment, since the barrier's
safety rests on which is true. Either way the one-liner is cheap insurance.
2. On cap expiry the model gets full orientation and zero tools
After waitForSession times out, bridgeError is still "" — the handshake is in flight, not failed —
so nothing tells the model Gortex is not ready, and decision.orientation is pushed as normal. That is
precisely the combination your own problem statement blames for the model following bare-name guidance
into Pi's read and getting denied. The PR says "the turn proceeds without tools", which is true, but
the sharper statement is that it proceeds with guidance to use tools that are not there.
Parking a short "Gortex tools are still registering; retry in a moment" line when the wait times out
would close it, and it costs nothing on the happy path.
On the harness
I'd lean toward folding it in, and the reason is narrow: the whole barrier rests on Pi's emit()
awaiting each listener. Pi is not vendored here and I could not check that claim from the tree. If it is
ever false — or a future Pi changes it — before_agent_start returns a promise nobody holds, the barrier
silently does nothing, and there is no test anywhere that notices. That is the failure mode worth buying
coverage for, more than the four scenarios themselves.
That said, "does this repo grow a second toolchain" is the maintainer's call, not mine, and you were
right to raise it separately rather than smuggle it in.
Verified
READY_WAIT_MS = 15_000 is well chosen: INIT_TIMEOUT_MS 60s + 1s backoff + retry is ~121s worst
case, so the cap can only bite when something is already wrong.
The context fix is right — clearing after the push means a shape the hook cannot append to no longer
eats the orientation.
The golden drift fence passes, and regenerating with -update-agent-render produces no diff, so the
committed pi.txt is exactly what the source renders.
go test ./internal/agents/... ./internal/hooks/... ./cmd/gortex/ green locally (16 packages), and all
12 CI checks are green.
One minor note, not a blocker: a cold daemon now means up to 15 seconds of silence on the very first
prompt with no user-visible signal.
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
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
The Pi extension could start its first LLM call before its Gortex tools were registered, and the note explaining Pi's tool renaming could be lost for the whole session.
before_agent_startwas synchronous. It ran whenever Pi's agent loop reached it — which, at startup, can be whilesession_start'sawait c.start()/await registerGortexTools(pi)is still in flight, because Pi arms the editor's submit handler before it awaitsrebindCurrentSession(). It then latchedorientationInjected = trueunconditionally (decision.orientationfrom the Go hook is effectively always non-empty), sopiAliasNote()— which reads the not-yet-populatedgortexToolNames— returned""and was never retried.Two consequences, both observed in a deterministic harness against the live
index.ts:read/editand got a denial naming the tool it had just called.The same shape reaches further than startup:
/reloadand/newreachbefore_agent_startbefore theirsession_startfires at all, and/reloadre-imports the extension module outright.Changes
session_start's body moves intostartSession()wrapped intry/finally, so a readiness promise settles on every exit path — including the earlyreturnwhen a/reloadsupersedes the bridge mid-handshake.before_agent_startis nowasyncand awaits it; Pi'semit()awaits each listener, so this genuinely holds the turn. Capped atREADY_WAIT_MS = 15_000—start()'s worst case is 60s + 1s backoff + 60s, which must never become first-turn latency./reloadre-imports the module (resource-loader.jsclears the extension cache; the loader runs jiti withmoduleCache: false) and/newbuilds a fresh runtime, so in both cases an instance exists — and can be asked for a turn — before its ownsession_startruns. Arming at construction closes that window;armSessionReady()is idempotent so a turn parked in the gap holds the same promise the incomingsession_startsettles.piAliasNote()is replaced bypiAliasedDescription(), which front-loads the explanation into the aliased tool's own description. It cannot be lost to ordering: if the model can seegortex_read, it can see whatgortex_readis. Non-colliding tools are untouched, and tools promoted later bytools_searchget it too, since they take the same registration path.contextpush lands, so it survives a context shape the hook cannot append to.Testing
go test -race ./...)Test suite
cmd/gortex/testdata/agent-render/pi.txtis regenerated in this PR.No tests are added by this PR
The behaviour changed here lives entirely in
internal/agents/pi/extension/index.ts, and there is no JS/TS test harness in the tree today;internal/agents/pi/adapter_test.gocovers the Go side and stops at the file boundary, so none of the behaviour above is reachable fromgo test. The change is verified by a standalone harness that derives its subject from the liveindex.ts— resolving the four{{ SENTINEL }}placeholders exactly as the installer does, and redirectingnode:child_processto a mock that controls when the simulatedgortex mcpchild answerstools/list. It therefore cannot drift from the source: rename a sentinel and it fails loudly.Four scenarios, 19 assertions, exit 0/1, no dependencies:
before_agent_startfires mid-handshake, nothing awaitssession_startread→gortex_readexplains its own rename and keeps its original description;searchuntouched/reloadgap, module re-importedsession_startlanded; session 2 has its tools and re-injects the orientationPointed at a pre-fix tree the suite fails, on the right assertions — four of D's six, including that the pre-fix code loses the orientation entirely on
/reload(the turn parks it, thensession_startclearspendingOrientation).That harness is not part of this PR, so the checklist item above is unchecked. Landing it means adding a second toolchain to a Go repo, and that is a decision worth taking on its own: where it runs, whether a missing
nodeskips or fails, and whether the tree gets its firstpackage.json. Filed separately. Happy to fold it into this PR instead if the maintainers would rather not merge the fix without it — in which case this PR should wait on that decision.Checklist
Meta["methods"]for interfaces — n/aEdgeMemberOfedges to their containing type — n/aScope and limits
awaitbetweensetupEditorSubmitHandler()andemit(session_start)(interactive-mode.js:708-712→rebindCurrentSession→bindCurrentSessionExtensions→agent-session.js:1906), so the handler's synchronous prologue always runs first. If a future Pi inserts an await there, the factory-time arming still covers it.pretooluse.go:568still renders`read(target:{symbol:"<id>"})`— bare names, in Claude Code's vocabulary — and those snippets are what a model copies. With the tool descriptions in place the model can translate those names itself. The real fix is a host-vocabulary mapping at theinternal/hooks/pi.goboundary. Filed separately; not in this PR.