Skip to content

fix(pi): hold the first turn until gortex tools are registered - #787

Open
vitaliyslion wants to merge 1 commit into
zzet:mainfrom
vitaliyslion:fix/pi-first-turn-tool-registration
Open

fix(pi): hold the first turn until gortex tools are registered#787
vitaliyslion wants to merge 1 commit into
zzet:mainfrom
vitaliyslion:fix/pi-first-turn-tool-registration

Conversation

@vitaliyslion

@vitaliyslion vitaliyslion commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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_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_000start()'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 readgortex_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-712rebindCurrentSessionbindCurrentSessionExtensionsagent-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.

@zzet

zzet commented Sep 10, 2026

Copy link
Copy Markdown
Owner

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 bare desc.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();
const settle = settleSessionReady; // this invocation's resolver
try {
  await startSession();
} 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.

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