fix: close the gaps found by the code audit (safety, sessions, plugins, IPC, redaction) - #108
Open
BIackFIame wants to merge 25 commits into
Open
BIackFIame wants to merge 25 commits into
BIackFIame wants to merge 25 commits into
Conversation
An orchestrator with canvastty_agents did not know which providers it
could spawn, so it searched ~/.local/bin and ~/.config instead of
delegating, and had to poll for results.
- list_providers: every agent provider CanvasTTY can launch as a
subagent, with the exact spawn_agent.provider id, name, installed and
available from the provider CLI registry, sign-in state from the last
usage-limits read (ok, signed_out, expired, unknown; LimitsService.peek
never starts a read and nothing reads credentials), subagent and
orchestrator support, and plugin launch options with the plugin tool
that picks them (such as list_routes). The control CLI answers the
same list as `providers`.
- spawn_agent lists the known ids (enum, mirrored from providerCatalog
and kept equal by a test) and refuses an unknown provider with
INVALID_REQUEST naming list_providers. The stdio helper answers
malformed core calls itself, since the bridge drops the connection
over them.
- wait_for_agent({ sessionId, timeoutSeconds <= 600 }): returns when the
subagent is idle (after its screen settled), needs_approval, exited
(done/failed), quiet, closed, or on timeout, with status, exit code,
waited time and the tail masked before it is cut. Own subagents only,
never the orchestrator itself; it stops at once on cancel or
disconnect. It reads only metadata and the output offset while it
waits.
- The MCP instructions, tool descriptions, orchestrator skill, docs and
both refusal messages give the workflow list_providers -> spawn_agent
-> wait_for_agent -> get_agent_result and say not to search the
filesystem for agent CLIs or their configuration.
OpenCode subagents asked "Access external directory ~/Downloads/<name>" for their own project when its name had a letter with two Unicode spellings (Cyrillic "й"). Finder stores names decomposed (NFD); the path an orchestrator types is composed (NFC). macOS opens either, so the CLI started in the NFC path, read its working folder back from getcwd in NFD, and OpenCode's string comparison between its project root and the paths in its prompt made the project external. Codex trust entries had the same mismatch. - TerminalManager.create spells the requested cwd as stored on disk (onDiskPath: each non-ASCII part replaced with the parent's entry of that name, exact match first, else the one entry equal after normalization; symlinks are kept, ambiguous or unreadable parts are left as given). This covers spawn_agent, the control CLI, plugin sessions.create and the launcher. - Every launch gets PWD set to its folder (the wrapped one for plugin environments) instead of inheriting the app's. - OpenCode gets permission.external_directory "allow" for exactly its project folder in its other Unicode spelling (the folder and its contents) in the per-run inline config; a person's own external_directory word is kept as "*", YOLO and ASCII paths are unchanged.
…effort The person asked an orchestrator for subagents on "GLM-5.3 Flash"; they ran on OpenCode's default GLM-5.3 because spawn_agent had no model. - CreateSessionRequest/SessionMetadata carry model and effort; the launch passes them to the CLI's own flags for that run only (--model: OpenCode provider/model, Codex, Claude, Qwen, Kimi alias, Grok, OMP, Pi, Cursor; effort: Codex -c model_reasoning_effort, Claude --effort, Grok --reasoning-effort, as their --help lists them). Hermes, MiniMax, Devin and Antigravity take none. Nothing is written to CLI config. - Checked per provider (shared/launchModel.ts) with the reason in the refusal and a pointer to list_providers / the providers command; kept on restart and restore (persisted, and a stored value the CLI would not take is dropped instead of failing the restore). - spawn_agent gets model and effort (effort enum mirrored and tested); the control CLI gets --model and --effort. - list_providers shows each provider's model format and effort levels, and for OpenCode the models `opencode models` lists: run in the background with a 5 s timeout, cached for 10 minutes, never awaited. - The skill, docs and tool texts say: if the person names a model, pass it as model. - OpenCode stops with only "Unexpected server error" on a model it does not know. A model missing from the cached `opencode models` list is refused before launch (spawn_agent, control create, TerminalManager for the launcher and plugins) with up to five closest ids; when the list cannot be read, the model is passed as given. - A subagent that exits anyway reports the last lines of its screen as plain masked text (exitLines) in wait_for_agent, observe_agent and get_agent_result.
OpenCode subagents asked the person for every action. OpenCode 1.18 has
no auto flag, so its auto profile is an inline OPENCODE_CONFIG_CONTENT,
merged with the core's own inline config and never written to
~/.config/opencode.
Checked against the installed opencode 1.18.33: Permission.evaluate is
rules.findLast(permission and pattern match), fromConfig turns
{ tool: action } into a "*" rule and { tool: { pattern: action } } into
one rule per pattern in order, and an agent's permission is appended
after the top-level one. So the rules go under agent.build (OpenCode's
default agent): after the person's own rules, and every tool not named
keeps what the person's configuration says.
- read, glob, grep, list: allow (".env" files still ask, as OpenCode's
default does); edit (edit, write, apply_patch): allow.
- bash: allow only when base protection is on and CanvasTTY's guard is
installed in that OpenCode (tool.execute.before, hard denies still
deny before OpenCode's own check); otherwise it asks, so auto never
grants more than the person chose. A launch contributor's other model
keeps bash asking, like accept-edits elsewhere.
- external_directory is untouched: outside the project asks as before.
- hasAutoMode includes OpenCode (launcher, card label, control CLI);
TerminalManager learns base protection through configureBaseProtection.
…ofile Subagents were always launched in the normal profile, so a person who ran the orchestrator in auto was still asked about every step of every subagent. - spawn_agent takes an optional profile, "normal" or "auto" (auto only where the subagent's CLI has one). Without it the subagent gets its orchestrator's profile; an auto its CLI lacks becomes normal. - YOLO is never handed to a subagent: core allows it only in an isolated environment the person chose, which spawn_agent cannot pick. An explicit "yolo" is refused with the reason, and a YOLO orchestrator's subagents run in auto (or normal). - The answer reports profile and profileInherited, list_agents shows each subagent's profile (and model), and the card shows the profile it runs in. The control CLI's create --profile stays its equivalent. - The skill and docs describe it.
…screen tail How core decided an OpenCode subagent finished: CanvasTTY's OpenCode plugin reports working on session.status busy and idle on session.idle through the runtime gateway, so wait_for_agent returned at the right time. But get_agent_result only returned the raw PTY tail (the last 8,192 characters of a full-screen TUI's redraw sequences), and state stayed "running" while the CLI was open, so the orchestrator never got the answer text. - The OpenCode plugin, when result capture is on for its session, reads the session's last assistant message (text parts, not tool, reasoning or synthetic parts; v2 and v1 SDK shapes; 2 s timeout) at session.idle and reports it with the turn's end, at most 4,096 characters with the end kept. The runtime gateway accepts a result on OpenCode's session.idle as it does on a Stop hook, still only for a lease that captures results. - spawn_agent turns result capture on for Codex and OpenCode subagents only (TerminalManager accepts it for both); ordinary cards never keep an answer. - TerminalManager keeps the last answer in memory, masks it when read, and clears it when the next turn starts. get_agent_result returns it as answer, plus status; wait_for_agent includes it when the turn ended. - Tests drive the plugin in a separate process with a fake SDK client against a real runtime gateway, and cover masking, truncation, the v1/v2 client shapes and the no-capture path.
…riables Base protection let three kinds of calls through unchecked: - A shell call whose input was over the 40 KB bound reached main as a preview only; no command was read, so nothing was denied and, with no decision plugin, the call ran. With base protection on, a cut shell call, or a cut file write whose target is not at the start of the preview, is now denied with advice to split the work. - A folder named in a variable of the same command (`OUT=/x; rm -rf "$OUT"`, `export OUT=/x`) was unresolved. Standalone assignments and export/declare/local/readonly/typeset now set the value for the rest of the command (a prefix assignment does not, as in the shell), and a value that cannot be known clears it. - `tar -C<dir>` with the folder attached, and `git -c alias.x='!cmd'` or a `-c` key whose value git runs (fsmonitor, pager, editor, filters, helpers) were not read. Their commands are now analyzed like any other. A decision-plugin list that cannot be read is no longer an empty list: the handler fails, so the gateway answers as unavailable (a CLI that can ask asks the person, a fail-closed gate denies), and a launch still installs the hook.
…s restore, close and cancel Saved cards: - A card list written by a newer version is left as it is and never saved over for the rest of the run; one that cannot be read at all is left alone too. A file that is not a card list is kept beside a fresh one (terminal-sessions.json.corrupt-<time>) instead of being replaced by an empty state when the manager saves. - Restore resumes environments only for cards that come back. Cards that do not (not restored, or a subagent whose parent is gone) release their environment with its data kept, instead of leaking it. - Subagents whose parents loop back on each other have no owner and do not come back. Owners: - Closing a card closes its subagents first, so none keeps running without an owner. - An environment a plugin prepared after the launch timed out is still heard of (the plugin call keeps the host-call budget) and released. - A controlled create whose setup failed closes the card it started. Gateways and cancel: - The orchestration gateway starts once for concurrent callers, and a stop during start leaves nothing listening; the browser agent gateway closed while its Unix socket opens closes that socket. - A Windows pipe host that fails and exits late no longer clears the host started after it. - A late start hook of an earlier turn no longer takes over from the newer turn, whose Stop was then dropped as stale. - A cancelled send_to_agent passes its signal down to the delivery: text still waiting for the launch is dropped and the answer is CANCELED.
… keep host replies with their process - Two installs of the same plugin both passed the "already installed" check; the loser's failed move then deleted the winner's directory and registry entry. Install, module changes, update and uninstall of one plugin now run one after another, the check runs inside that order, and a failed install removes only what it created. - A host call a service made before it crashed was answered into the stdin of the restarted process. The reply now goes only to the process that asked. - Uninstall revoked secrets before stopping the plugin, so a secret write in flight could recreate the file, and a failed uninstall left the plugin without its secrets. Uninstall now stops and removes the plugin first, then revokes; secret writes are refused while a revoke is pending and re-check permission when they run.
- The companion (glasses) read the terminal screen and captured answers without the secret registry. The screen text is now masked as a whole before it is cut, so a key the terminal wrapped over two lines is found, and captured answers are masked when they arrive. - An owner holding 64 values dropped the oldest one even when it was still in use. Adding a held value again now makes it the newest (the vault adds each key it reads), and a drop or a refused owner is logged without the value instead of happening silently.
… went in When focusing or selecting the target opened a JavaScript dialog (a focus handler's alert), browser_type inserted nothing but answered typed: true. It now answers DIALOG_OPEN with the dialog type, so the agent handles the dialog and types again. A dialog raised by the page's input handler comes after the text went in and still counts as typed. browser_select sets the selection before it fires its events, so a dialog from those events leaves it selected and its answer is unchanged.
…v names as the OS does - Clipboard, settings reads, file pickers, limits, plugin canvas and window opening, plugin storage and media, the Hermes HUD, and terminal list, resize, bounds, rename, restore and visibility handlers now check that the call comes from the main window's own frame, like the other privileged channels; the terminal ids they take are checked as text. - Plugin environment names: __proto__ is refused as a name (assigning it was silently dropped), a name such as constructor no longer looks like one another plugin already set, and on Windows two spellings of one name (Path, PATH) collide with each other and with what CanvasTTY sets.
…plies with what they belong to - A card whose history snapshot failed also lost its live output: the error disposed the only subscription. The failure is still reported, and the card now goes on with live output from the oldest event queued. - Provider keys and API profiles could be saved twice (Enter bypassed the busy state), and a save that finished late cleared text typed after it was submitted. A second save of the same entry is ignored while the first runs, and a draft is cleared only while it still holds what was saved. Profile edits no longer merge into a draft from an earlier render. - After the launcher's agent changed, the previous agent's launch plugins and environments stayed offered until the new list arrived, so their options could be sent with the new agent. Lists are shown only for the agent they were loaded for. - A plugin frame request still running when the frame reloaded or navigated was answered into the next document (a secrets.get reply included). A reply is now dropped once the frame shows a new document or its window is no longer the one that asked.
…cement candidates - The browser card reports its rectangle on every camera step, resize and layout pass. A report that rounds to the placement already applied now returns before the native view is re-laid out (hidden reports still run: they end gestures). In a simulated slow pan with a duplicate report per frame, 1200 reports caused 1200 view syncs before and 300 after; idle re-renders go from 1200 to 1. A fast pan is unchanged (every report moves the view). - Placing a new card near Home built and sorted a lattice of about 3000 candidate positions every time. The sorted list is kept for the same Home, card size and options, and the search still stops at the first free position: 500 placements beside 20 cards took about 240 ms before and under 3 ms after.
…ants and plugin frame replies - Base protection: rm, rmdir and the other deleters, find -delete, and mv operands whose path is a variable or command output that cannot be resolved are now denied as an unknown target, with advice to write the path out. A variable set in the same command, or a for-loop over plain words, still resolves (a loop over a folder outside is the delete outside it). - Uninstall marks the plugin as being removed: a music folder pick that finishes while it runs, or after it, stores no grant (the grant is checked again right before it is stored). - A plugin frame reply is tied to the window that asked, the plugin the frame served and the frame's document generation, all captured when the request arrived. Pointing the frame at another plugin or entry starts a new generation at once, so a reply that finishes before the new document loads or asks anything is dropped.
…not words in values A contributed argument was refused when its text anywhere contained words such as "dangerously" or "approval_policy", so a context plugin whose rule merely mentioned them blocked the Codex or Grok launch. Arguments are now read by structure: a flag whose name is core-owned or asks to bypass approvals, a config override (`-c key=value`, `--config=key=value`, `-ckey=value`) whose key decides approvals, the sandbox or the hooks, and the core-owned subcommands are refused as before. Text inside a value is the plugin's own.
This was referenced Sep 30, 2026
Owner
|
Reviewed the changes introduced by this PR separately from the later commits in #109–#112. The session-store preservation, per-plugin operation serialization, process-bound service replies, and main-renderer IPC checks are useful changes. I did not find an additional merge-blocking defect in those reviewed paths. Please fix the Windows failures before this is merged. The existing Windows job has two failures:
The branch now conflicts with main after our keyboard/history/companion integration. Please rebase or adjust the stack bases and rerun the required checks on the resulting commits. This is a review of the paths above and existing CI evidence, not an assertion that every execution path has been tested locally. |
…e open On Windows, replacing terminal-sessions.json fails with EPERM, EACCES or EBUSY while any other handle has the file open (a reader, an indexer, antivirus). The failed save was dropped, so the saved cards stayed behind the live ones until the next change; the Windows CI run of session-environments showed exactly that (the saved environment label never arrived while the test was reading the file). The store now retries such a rename for about a second on Windows before giving up; on other platforms the error still fails at once. The rename is injectable so the Windows behaviour is tested on every platform.
…opped, not failed On Windows the gateway listens through the current-user pipe host. When stop() ran while the host was starting, start() rejected with "shutting down" instead of returning with the gateway stopped, as it does on macOS and Linux. The lifecycle test also constructed the gateway without the pipe host, so on Windows it could only fail with the production requirement; it now gets the same built host as the other gateway tests.
… shut down node:test runs after-hooks in the order they were registered. Four environment tests registered the removal of their session-store folder before managerFixture registered the manager's shutdown, so the folder was removed while the store could still write into it; the Linux job failed once with ENOTEMPTY. The removal is now registered after the fixture.
…close The gateways answer a protocol failure (an unauthenticated command, a replayed token) with an error message and close the connection. On Windows both go through the current-user pipe host as a write frame and a destroy frame; the destroy marked the connection closing at once and its writer thread stopped without writing what was still queued, so the client sometimes saw the pipe close without the error. The Windows job failed orchestration-gateway's "unauthenticated commands and replayed bootstrap tokens are rejected" this way (a timeout waiting for the error), intermittently. A destroy now lets the writer drain what was queued before it, bounded by two seconds for a client that stops reading, then closes. The relay self-test (npm run test:windows-pipe-host) sends a last message and the close in one write and requires the message to arrive.
BIackFIame
force-pushed
the
pr/2-audit-fixes
branch
from
September 30, 2026 21:58
c9d2838 to
1bdf3f3
Compare
Contributor
Author
|
Thanks for the review. Rebased onto
Verified: fork CI on tip |
This was referenced Sep 30, 2026
…#111) Flush final replies through the Windows pipe buffer, cancel draining on the original deadline or shutdown, and exercise delayed and stalled readers. Note: pre-existing failure in the local focused-wheel browser smoke is not addressed by this change.
Contributor
Author
A follow-up to the Windows fixes: the current #111 upstream run exposed another real close race, present in this PR. Draining the C++ queue was insufficient: DisconnectNamedPipe discards unread bytes still buffered by Windows. The host now flushes that buffer before disconnecting, within the original two-second deadline. Shutdown and the deadline cancel pending writes/flushes, and the flush worker is joined before handles are closed. Duplicate destroy frames do not reset the deadline; late writes cannot interrupt previously accepted replies. The real Windows relay self-test now covers eight delayed-reader exchanges, ordered final replies, duplicate destroy and late writes, a non-reading client that must actually disconnect, and shutdown during a blocked flush and write. This fix is carried through #109–#112. All three upstream checks on |
Contributor
Author
The lifecycle fixture uses the same built current-user pipe host as the other tests. The production host requirement is retained. Stop during startup leaves the gateway stopped, and fatal-host recovery is bounded and cancels on explicit stop without reviving expired capabilities.
The save failure was Windows EPERM when replacing an open session-store file. The existing Windows-only bounded rename retry and regression remain in place. The later pipe-close failure is fixed by flushing the native pipe buffer before disconnect, within the original two-second deadline; timeout cancels pending I/O and joins the worker before handles close. The real Windows host self-test covers delayed readers, final replies, duplicate destroy/late writes, non-readers and shutdown while I/O blocks. The additional Linux cleanup failure came from removing shared persistence storage before every manager had stopped. One lifetime now shuts down owners in reverse order and removes storage last, attempting all cleanup even if one step fails. The history-HUD fixture also uses deterministic timestamps. These changes keep the original lifecycle assertions. Updated head: Independent GPT-6 Luna reviews found no remaining concrete defect in the reported cases. Final cumulative local validation: 1605 passed, 3 skipped, 0 failed; production build/typecheck, secret audit and real-Electron hidden-terminal proof passed. Lower branches were additionally checked with their own gateway/environment/history tests and typecheck. |
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
2 participants
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.
Branch:
pr/2-audit-fixes→ basemain(stacked on the previous PR).Depends on #107 (orchestration).
Only this PR's commits: BIackFIame/CanvasTTY@pr/1-orchestration...pr/2-audit-fixes
Why
OUT=/x; rm -rf "$OUT"),tar -C<dir>, andgit -ckeys whose value git runs.send_to_agentstill delivering).What changes for users after merging
Fewer ways for an agent command to slip past base protection, cards and subagents that survive restore and close consistently, plugin installs that cannot corrupt each other, and secrets masked in the glasses companion view too. Two small speed-ups come with it.
OUT=/x; rm -rf "$OUT"with base protection onrm -rf /xrm -rf "$(cmd)"(target cannot be known)The last two rows are micro-benchmarks from the unit tests of this branch, not the app-level A/B; the rest are behaviour checks covered by tests.
What's inside
src/main/safety): cut shell input denied, variables andexport/declare/local/readonly/typesetfollowed within a command,tar -C<dir>,git -c alias.x='!cmd'and git keys that run programs analyzed; unknown delete targets (rm,rmdir,find -delete,mv) denied. An unreadable decision-plugin list fails instead of counting as "no plugins".send_to_agentdrops undelivered text.browser_typereportsDIALOG_OPENwhen a focus dialog blocked the text.Path/PATHon Windows,__proto__refused).How to verify
Manual: with base protection on, ask an agent to run
D=$HOME/some-folder; rm -rf "$D"— the call is denied with the resolved path.Risks / follow-ups
PathvsPATH) are tested with an injected platform only.