PA-10 engine side: resolve page through a browser request channel - #64
Merged
Merged
Conversation
`page` was two typed globals that both threw. The engine process controls no browser page, and the only reverse-direction channel in the tree was the loopback RPC server, which is off by default for a reason its own header states: a local server that can drive the browser is the browser to every other process on the machine. So this adds no listener and opens no socket. The direction that already exists is the client subscribing to `GET /api/event`, so a browser action is a pending request published on that stream and settled by an HTTP reply — the shape `QuestionV2` already uses to ask a person something. The client dials the engine; the engine never dials the browser. schema/browser-request.ts the action union, the typed result, the events core/browser-request.ts the pending map, the deadline, reply and refuse protocol/groups/ list, reply, refuse — session-owned, like questions server/handlers/ with the same ownership check on every settle redrob/tool/domain.ts bridgedPage, replacing unavailablePage at runtime Three decisions worth stating. The result is a tagged union, not one permissive `unknown`. The engine knows which shape each action must produce, so a client answering `page.query` with a string is a protocol error the engine names. Reading it as an empty node list would tell the model "nothing matched the selector" — a claim about the page that nobody made. There is a deadline, and it is the reason a skill can use this at all. Nothing tells the engine whether a client is subscribed: the event stream is a publish/subscribe fan-out with no subscriber count, so "no browser attached" and "the browser is slow" are indistinguishable from here. Waiting forever is the single outcome that is certainly wrong — `page.text()` with no browser running would hang the session instead of failing it. Refusing is its own route rather than a silent non-reply. A tab that is not shared is a normal answer, and routing it through the deadline would cost twenty seconds to say so. `unavailablePage` stays, for the catalog preview and for any harness that binds no browser, with its message corrected to name what is actually missing now. Verified: 998 core tests, 390 redrob tool tests, 0 failures; tsc clean across schema, core, protocol, client, server, redrob. The eleven pre-existing dialog-move-session errors in packages/tui were measured on a stashed clean tree and are unrelated. Both new guards reverse-verified: removing the shape check makes the wrong-shaped-answer test pass a program it should fail, and removing the deadline hangs the unanswered-request test. The generated client SDK is regenerated, which the API change requires. Both browser endpoints end in ".list" and the client method name is the last dot segment, so the location-wide one carries an explicit name in contract.ts.
…ee new events
CI was red on three jobs and all three were this change's own doing.
`test:httpapi` fails on `missing=4`: the exerciser requires a scenario per route
and PA-10 added four without them. Added, mirroring the V2 question scenarios
because the browser request service is deliberately the same shape -- a global
list, a per-session list, and the two settlement routes. The settlements name a
request id that does not exist, so each asserts the 404 path and needs no live
browser on the other end.
Writing those scenarios caught a real mistake in my own reading of my own route,
twice. The reply body is `{ value: { type, value } }`: `value`, not `result`, and
the inner shape is the tagged `BrowserRequest.Value`, not the command echoed
back. The extension's executor already sends exactly that -- it was written
against the same schema file -- so this was the exerciser being wrong, not the
client. Measured by grepping what `page-actions.js` actually emits rather than
assuming the two agree.
The event manifest pinned a bare count of 88 and PA-10 brings three public wire
types. Bumping the number alone would have satisfied the tripwire without saying
what arrived, so the three are asserted by name beside it: `browser.request`
asked, answered, refused. The count stays as the guard against an accidental
addition; the names say which addition was deliberate.
`unit (linux)` and `unit (windows 4/4)` were the same manifest count -- the linux
one is an aggregator reporting its shards, not a separate failure.
httpapi 215/215 in all three modes (coverage, auth, effect), missing=0.
…ough the loader K-2 landed `SkillArming.arm` as a pure function with its own tests and nothing called it. Measured: its only caller was that test file. So every skill that declared keywords and URL globs behaved exactly like one that declared none. Calling it was not enough. The skill surface the system prompt actually reads is a second, older loader -- `packages/redrob/src/skill/index.ts` -- and its `Info` had no `autoInject` field at all, so the data was dropped at load time and the matcher would have had nothing to match on no matter who invoked it. The field is now carried through, using the same schema as the other loader rather than a second copy of it. A malformed block costs only the arming behaviour: the skill still loads and stays selectable by name. Refusing the whole file would lose a working skill over an optional key, and that failure mode is tested. `SystemPrompt.armedSkills` is separate from `SystemPrompt.skills` because the two make different claims. `skills` is the discovery list -- names and descriptions, for the model to pick from with the skill tool. `armedSkills` is the CONTENT of the ones that armed themselves on what the user just typed, which the model did not ask for and is therefore told it is already reading, with the patterns that armed it. A skill with no `autoInject` never arms. The armed-by attribute is escaped with the repo's own `escapeHtml`. The first version used `JSON.stringify` to show each pattern, which put a raw quote inside the attribute and broke the tag; a keyword is author-supplied text from a skill file, so that is reachable, and there is a test with a keyword that contains one. URL arming is wired and deliberately not fed. Getting the active tab means a browser request per message, which with no extension attached spends the request deadline before the model sees anything -- a twenty second pause before every reply is a worse product than URL arming is a better one. Keywords arm today; the reason is at the call site. Also ships one document against the real surface: `page-control`, the six `page` calls that PA-10 made resolve. Checked both ways against the bound tool namespace, so a call the engine does not have cannot be described and a call it has cannot be left out, and the document's own "Six calls" sentence is asserted against the real count. core 998 pass 0 fail. redrob tool+session 854 pass 0 fail. Skill loader 19 pass including the two new ones. Reverse-verified both ways: removing the wiring fails four system tests, removing the loader field fails the end-to-end arming test. Found and NOT fixed here, because it is a seam and not a document: the core `SkillV2` plugin registry feeds an `EmbeddedSource` list with zero consumers in the engine, so its builtin skills are registered and unreachable and only disk skills load. Recorded in the queue.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What this is
PA-10's engine half.
pagewas two typed globals that both threw; this makes it real, withoutopening a socket or adding a listener.
The only reverse-direction channel in the tree was the loopback RPC server, which is off by default
for the reason its own header gives: a local server that can drive the browser is the browser to
every other process on the machine. The direction that already exists is the client subscribing to
GET /api/event— so a browser action is a pending request published on that stream and settled byan HTTP reply, the same shape
QuestionV2already uses to ask a person something. The clientdials the engine. The engine never dials the browser.
packages/schema/src/browser-request.tspackages/core/src/browser-request.tsreplyandrefusepackages/protocol/src/groups/browser-request.tspackages/server/src/handlers/browser-request.tspackages/redrob/src/tool/domain.tsbridgedPage, replacingunavailablePageat runtimeThree decisions worth reading
The result is a tagged union, not one permissive
unknown. The engine knows which shape eachaction must produce, so a client answering
page.querywith a string is a protocol error the enginenames. Reading it as an empty node list would tell the model "nothing matched the selector" — a
claim about the page that nobody made.
There is a deadline, and it is why a skill can use this at all. Nothing tells the engine whether
a client is subscribed: the event stream is a publish/subscribe fan-out with no subscriber count, so
"no browser attached" and "the browser is slow" are indistinguishable from here. Waiting forever is
the one outcome that is certainly wrong —
page.text()with no browser running would hang thesession rather than fail it. 20s, asserted against the TestClock so the suite does not pay for it.
Refusing is its own route, not a silent non-reply. A tab that is not shared is a normal answer;
routing it through the deadline would cost twenty seconds to say so.
unavailablePagestays for the catalog preview and any harness that binds no browser, with itsmessage corrected to name what is actually missing now.
Verification
packages/core: 998 tests, 0 failures (5 new, covering both halves of the deadline)packages/redrobtool suites: 390 tests, 0 failures (6 new for the bridge)tsc -bclean across schema, core, protocol, client, server, redrobpackages/tui/src/component/dialog-move-session.tsxerrors were measured on astashed clean tree — same eleven, unrelated to this branch
.listand the client method name is the last dot segment, so the location-wide one carries anexplicit name in
contract.tsBoth new guards reverse-verified: removing the shape check makes the wrong-shaped-answer test pass a
program it should fail; removing the deadline hangs the unanswered-request test.
What this does NOT do
No client answers these requests yet, so
pagestill refuses in practice — now by deadline, with amessage naming the action and saying the extension must be attached. The extension half is the next
stage.