Skip to content

fix(tangle-provider): remove terminal mutation preflight - #292

Merged
drewstone merged 1 commit into
mainfrom
fix/tangle-terminal-input-validation
Sep 3, 2026
Merged

fix(tangle-provider): remove terminal mutation preflight#292
drewstone merged 1 commit into
mainfrom
fix/tangle-terminal-input-validation

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • Send attached terminal input and resize through the authenticated terminal connection without a redundant REST validation request.
  • Let a stale terminal handle close only its own socket; keep control validation for attach, prompt, and stop.
  • Update the shared conformance contract and document the deployment capability boundary.
  • Test against the five current dependency releases; pnpm outdated reports none.

Proof

  • Focused interactive provider: 10/10 passed.
  • Provider testkit: 26/26 passed.
  • Workspace: 97 files and 1,695/1,695 tests passed.
  • Typecheck: 9/9 packages passed.
  • Build: 9/9 packages passed.
  • Package artifacts: 9 packages, 31 exports, 62 targets passed.
  • Packed control contracts: 7 files and 156/156 tests passed.
  • Independent review: approved after correcting stale socket-close semantics.

The exact live Tangle keyboard flow will run after this package publishes and Braid adopts it. That run is the production transport proof, not inferred from these unit tests.

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 66fc48e2

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-03T02:35:54Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 3 (3 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 329.2s (2 bridge agents)
Total 329.2s

💰 Value — sound

Removes a redundant per-keystroke REST control-validation preflight before terminal input/resize/close, moving mutation fencing to the server-authorized terminal transport and fixing stale-handle socket-close semantics — coherent, in-grain, and correctly gated; ship.

  • What it does: Before: the Tangle provider's validatedTerminal() wrapper ran a REST validateControl roundtrip before every attached-terminal input, resize, AND close (old tangle-interactive.ts:455-470). After: input/resize/close pass straight through to the terminal handle (tangle-interactive.ts:453-458, with a comment stating the authenticated terminal transport authorizes them), while attach/sendPrompt/stop ke
  • Goals it achieves: Three concrete improvements readable from the diff: (1) Latency — every keystroke and resize previously paid an extra REST roundtrip before the real write; on an interactive terminal that doubles per-input cost on the hottest path. (2) Race removal — the preflight could pass and then control goes stale before the authoritative operation, or reject an operation the transport would still authorize (
  • Assessment: Good on its merits, and squarely in the codebase's grain. The adapter's own doctrine (tangle-interactive.ts:71-77) is that the Sandbox owns process identity, terminal binding, and control generations, and the adapter 'never creates a second record for any of those facts' — a client-side REST preflight was exactly such a second, weaker record: advisory only (any client could skip it) and racy. Movi
  • Better / existing approach: none — this is the right approach. Searches run: runInteractiveSessionConformance is consumed only by the testkit index and the tangle provider's tests (rg across packages/), so there is no second provider implementing this contract to duplicate or diverge from. validatedTerminal (tangle-interactive.ts:430-461) is now mostly pass-through for the mutation methods but still earns its keep by freezin
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788403757316,"sessionID":"ses_f9ad4506dffeaid11hNPWSAO70","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_9d9c1369"}}}

🎯 Usefulness — sound-with-nits

Removes a racy client-side REST control preflight before attached-terminal input/resize/close and lets a stale handle close only its own socket — server-authoritative, in-grain, reachable through the provider's environment surface today.

  • Integration: Fully reachable now: createTangleInteractiveAgentRegistry is wired into the environment session at packages/agent-provider-tangle/src/tangle-environment.ts:156-160 and exposed as startInteractive/interactive at tangle-environment.ts:421-435, capability-gated by the deployment document (interactiveAgent.start+control). The shared conformance (runInteractiveSessionConformance) is exercis
  • Fit with existing patterns: Improves fit rather than competing with a pattern: the adapter's stated design is 'The Sandbox owns … control generations. This adapter validates each returned value and never creates a second record' (tangle-interactive.ts:71-77), and the interface itself says 'every write remains bound to this claim' at attach (packages/agent-interface/src/environment-interactive-control.ts:532). A REST prefligh
  • Real-world viability: Error paths are covered, not just the happy path: the new tests pin exact validation counts (attach=1, prompt=2, stop=3 at interactive.test.ts:620-656) and prove stale input/resize reject with the server's 'stale or expired interactive control' while stale close resolves {status:'unknown'} (interactive.test.ts:658-710). The stale-close semantics change is contract-coherent: post-close, the confo
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Transport-side authorization is asserted, not yet proven on live Tangle [robustness] ``

The safety of dropping the preflight rests entirely on 'The sidecar authorizes those mutations' (README paragraph added in this commit). Unit tests exercise a mocked sandbox (fakeInteractive), so they prove contract shape, not live transport enforcement, and the PR body itself defers the production proof to Braid adoption. Also note the peer floor stays >=0.34.6 (README) while the property is tested against 0.37.0 only. Acceptable to ship — the old preflight was racy and added no real fencin

💰 Value Audit

🟡 Retained REST preflight before sendPrompt/stop is the same pattern the PR argues against [maintenance] ``

tangle-interactive.ts:343 and :383 still call validateControl before sendPrompt and stop, yet those commands carry their own control digest that the server authorizes — the same redundancy removed from the terminal path. Low-frequency operations make the latency cost negligible and the changeset explicitly scopes this PR to terminal mutations, so this does not gate shipping; a follow-up could drop the remaining preflights for consistency.

🟡 validatedTerminal wrapper is now thin enough to invite future simplification [maintenance] ``

After this change, 5 of 7 members (input, resize, close, detach, events) at tangle-interactive.ts:455-459 are direct pass-throughs; only the ref/cursor getters and frozen control add value. Fine as-is; worth revisiting if the getters ever become pass-throughs too.


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260903T025445Z

@drewstone
drewstone merged commit 92526ff into main Sep 3, 2026
2 checks passed
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