Skip to content

fix(contracts): tolerate unknown keybinding commands on client decode - #238

Closed
bulgadev wants to merge 89 commits into
patroza:fork/changesfrom
bulgadev:vscode-keybinding-fwdcompat
Closed

fix(contracts): tolerate unknown keybinding commands on client decode#238
bulgadev wants to merge 89 commits into
patroza:fork/changesfrom
bulgadev:vscode-keybinding-fwdcompat

Conversation

@bulgadev

Copy link
Copy Markdown

Problem

When a T3 Code server introduces new keybinding commands (e.g. filePicker.toggle, projectSearch.toggle), clients built against an older contracts build fail to decode the entire server.getConfig response because ResolvedKeybindingsConfig uses a closed union (KeybindingCommand) that rejects unknown values.

This manifested in the VS Code extension as:

T3 Code pairing failed: Could not connect to T3 Code: Expected "sidebar.toggle" | "terminal.toggle" | ... | "thread.jump.9", got "filePicker.toggle" at ["value"]["keybindings"][42]["command"]

The pairing itself succeeded (bearer token exchanged and stored), but the subsequent getConfig RPC failed during schema validation, blocking the connection.

Solution

Make the decode side of ResolvedKeybindingsConfig forward-compatible: rules whose command does not match the known union are silently dropped on decode, while known rules round-trip unchanged. Encoding is unaffected, so the server (which only ever emits known commands) and the strict authoring path (KeybindingRule / KeybindingsConfig) keep their exact behavior.

This is the same forward-compat pattern used elsewhere in the codebase (e.g. ProviderOptionSelections in model.ts tolerates legacy shapes via Schema.decodeTo + transformOrFail).

Scope

ResolvedKeybindingsConfig is consumed by all four wire schemas that carry resolved keybindings:

  • server.getConfig (initial config fetch)
  • subscribeServerConfig (config stream)
  • serverUpsertKeybinding / serverRemoveKeybinding (mutation results)

All three clients (vscode, web, mobile) share the same RpcClient.make(WsRpcGroup) decode path, so this fix unblocks all of them simultaneously.

Changes

  • packages/contracts/src/keybindings.ts: wrap ResolvedKeybindingsConfig with a decodeTo transform that filters unknown rules via Schema.decodeUnknownOption(ResolvedKeybindingRule) per element.
  • packages/contracts/src/keybindings.test.ts: two new tests verifying unknown commands are dropped and all-unknown arrays decode as empty.

Verification

  • pnpm --filter @t3tools/contracts test: 238 tests passed (19 files)
  • pnpm exec vp check packages/contracts/src/keybindings.ts packages/contracts/src/keybindings.test.ts: 0 errors, 0 warnings
  • ELECTRON_SKIP_BINARY_DOWNLOAD=1 pnpm exec vp run -r --cache --log labeled typecheck: exit 0

tim-smart and others added 30 commits July 30, 2026 18:43
Add custom "Open with" applications

Source: tim-smart#4
Source head: 8c4bdfb
Source commits: 08e1a4f,cd43a401c6c148f1fe26cff72104ac527ea189f3,a8370e7502c552ebb064436e42e1c00f86f0946b,8c4bdfbc5b57f6b600233244d330f9efa41dc498
Imported: complete product delta from the source PR.

(cherry picked from commit 9fae005)
Load direnv environments for provider sessions

Source: tim-smart#5
Source head: 8f5fc87
Source commits: e4f0701,0d1463af61e0bd174f698b2519ebf3b207a2eaca,a66e4160d5f4b79140ec8fbcbc6aa66af750a991,8f5fc87c13f4628c179cda44d4f32f7fe4d316b2
Imported: complete product delta from the source PR.

(cherry picked from commit 0da8bfe)
Add unsigned retry for commit signing failures

Source: tim-smart#6
Source head: 7d65c5a
Source commits: 18ee567,72a6fae23c86708080c4fed346d5bf0f136f0221,6614b28239ed2330a8f601357a413f2d50da195a,ec169369daa554541511aa28f551b36f3dd26485,7d65c5a224e97a6b811b0a84892f1fda065c5963
Imported: complete product delta from the source PR.

(cherry picked from commit 03671a2)
Add /new command for contextual threads

Source: tim-smart#7
Source head: 2051a80
Source commits: 2051a80
Imported: complete product delta from the source PR.

(cherry picked from commit 4d94f31)
Add session dashboard board

Source: tim-smart#8
Source head: d9f8e4d
Source commits: 268fb8d,dde20f271f674da22dd8f3a08201c2acf5e58ee5,df4a145e7b2cd2dc17a7a595267d2d8eb0a2a3f0,ce5723ddb0bf630a18d4cb8227b5344d12626e72,ad8c1a6af41161e1fc38a52f681b306517c7b918,6281887e6125317da0c7b4252d59bfd41c9bf35e,550db6316c634febdbe1cb27334d1347c23c7b2a,d9f8e4d0a8dc22231ca315f3c595c3597f3b13e5
Imported: complete product delta from the source PR.

(cherry picked from commit cd0e281)
Recover interrupted provider turns after server restarts

Source: tim-smart#9
Source head: b181832
Source commits: 7f69028,1d52bce1376766d804ef884d7d50b8b6d1b48cf7,b181832560177250b90bbfe07b0882c9e5b93493
Imported: complete product delta from the source PR.

(cherry picked from commit 83de8f5)
Avoid repeated thread snapshot loads during subscription retries

Source: tim-smart#10
Source head: c8c9ead
Source commits: c8c9ead
Imported: complete product delta from the source PR.

(cherry picked from commit 9e400c3)
Add image upload button to compact chat composer

Source: tim-smart#11
Source head: 1ff63f9
Source commits: 1ff63f9
Imported: complete product delta from the source PR.

(cherry picked from commit 720ec65)
Truncate mobile branch toolbar controls

Source: tim-smart#12
Source head: 1b7d444
Source commits: 1b7d444
Imported: complete product delta from the source PR.

(cherry picked from commit dc2bbb4)
Clean up worktrees when archiving threads

Source: tim-smart#13
Source head: a23f42d
Source commits: 4a19470,1b6688aa5b641010cb2e9dad23d36d87257403ad,9ed32aa3923fb674380564b1ffcb3268290069b9,a23f42d6ac671ea36b8db5d03934c089a31be448
Imported: complete product delta from the source PR.

(cherry picked from commit 7e02dc9)
Pass hosted app channel into Vercel web builds

Source: tim-smart#14
Source head: de6966a
Source commits: de6966a
Imported: complete product delta from the source PR.

(cherry picked from commit 6333d8d)
Allow worktrees to reuse the selected branch

Source: tim-smart#15
Source head: 2d3900b
Source commits: cd60531,2d3900ba36c9397dc4fbe879c613a809f6b45384
Imported: complete product delta from the source PR.

(cherry picked from commit 5e7dff2)
Add optional worktree removal confirmation

Source: tim-smart#16
Source head: c3f509f
Source commits: 76f063e,c3f509fe8f690b704bb34692d9c132c0644db777
Imported: complete product delta from the source PR.

(cherry picked from commit 9886109)
Stop retrying unavailable thread subscriptions

Source: tim-smart#17
Source head: 1359af8
Source commits: 1359af8
Imported: complete product delta from the source PR.

(cherry picked from commit 7b37a7a)
Compatibility fix for running the selected Tim stack on the fork CI matrix. Source adaptation review: patroza#31.

(cherry picked from commit 6edd39a)
Keep the selected Tim Open With feature portable on non-macOS builders and avoid treating custom app definitions as macOS bundles. Source adaptation review: patroza#33.

(cherry picked from commit 15a7d2a)
…nd; green tip

Bring Tim layer tip to typecheck green by joining main ref-refresh VCS client
state with fork failureKind/worktree-cleanup contracts, restoring
filterBrowseEntries/reuse-base-branch surfaces Tim dropped, and fixing
ChatView/Board call-site type errors left by incomplete Tim joins.

(cherry picked from commit 0e24917)
Bring fork/tim typecheck/test green after main pingdotgg#2679 + Tim client-runtime
rewrite: rejoin EnvironmentSubscriptionRpcTag/localApi/ws scopes, wire
BackgroundPolicy/ResourceTelemetry layers, force openpgp for signing tests
on hosts with gpg.format=ssh, and treat TRACE2 child_exit without
child_class as hook finish (git 2.55+).
…troza#29)

Source: pingdotgg#4018
Source SHA: de8fd65

Imported: bounded server activity snapshots, cursor pagination, lazy web history loading, reconnect-safe reset/dedup, and disabled eager browser sidebar hydration.
Adapted: preserved Tim thread lifecycle handling and Omega composer/minimap behavior while resolving current-stack conflicts.
Excluded: none of the source PR behavior; native mobile pagination remains separate because pingdotgg#4018 intentionally excludes it.
…#3510) (patroza#35)

Source: pingdotgg#3510
Source SHA: 034f493

Imported: native mobile lazy loading for older thread activity, a 1,000-event subscription catch-up ceiling with snapshot fallback, and synchronized stale snapshot watermarks.
Adapted: applied above the refreshed pingdotgg#4018 web/server candidate and preserved Tim lifecycle handling plus our mobile composer changes.
Excluded: pingdotgg#3510 server/web pagination duplicated by pingdotgg#4018, the later shared-hook refactor, formatting-only commits, and contract comments. The shared refactor can be revisited independently after production validation.
…oza#34)

Source: pingdotgg#4176
Source SHA: 56b6615

Imported: O(1) command read-model maps, deleted-thread eviction, VCS cache cleanup, browser surface cleanup, preview idle TTL, and per-thread UI cleanup.
Adapted: preserved our thread settlement, snooze, and sequential worktree deletion actions while wiring upstream cleanup into the current hook.
Excluded: none.

Co-authored-by: Rusiru Sadathana <27785781+RusiruSadathana@users.noreply.github.com>
patroza#44)

Source: pingdotgg#4506

Source SHA: f7eaa00

Imported unchanged as one candidate provenance commit.
…tgg#4558)

Imported from https://github.com/pingdotgg/t3code/pull/4558\n\nAdapted to retain our provider restart-recovery constants while replacing the local default-title check with the shared policy.
After rebasing candidates onto the green tim tip, restore missing
EnvironmentThread loading fields, ChatView sendDisabledReason/threadSyncPhase
wiring, and orchestration.getThreadActivities auth coverage.
@patroza

patroza commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Thanks a lot for the contribution!

Requesting changes:

  • decodeUnknownOption(ResolvedKeybindingRule) drops every invalid rule, not only rules with unknown commands. Malformed rules using known commands should continue to fail validation; only genuinely unknown command names should be skipped.
  • The maximum-length check runs after filtering. A large input containing unknown or malformed rules can decode to an empty array and bypass the 256-rule limit. Please enforce the limit on the input array before filtering.

@patroza
patroza force-pushed the fork/changes branch 23 times, most recently from 2d1f67b to 271c4b2 Compare August 5, 2026 14:38
omegent-app Bot added a commit that referenced this pull request Aug 6, 2026
Ports [#237](#237) by
[@bulgadev](https://github.com/bulgadev) onto `fork/dev`.

The original targets the now-frozen `fork/vscode` overlay branch and
lives in a fork this repository
cannot push to, so it could not be rebased in place.

## Attribution

The commit is **authored by `bulgadev <me@bulgaaw.com>`**, not merely
credited — `git cherry-pick`
preserved it and the amend kept it. `Co-authored-by` is added on top as
insurance: `fork/dev` is
squash-only, and a squash can rewrite the author while trailers survive
in the body either way.

Please **merge this rather than #237**, and close #237 pointing here.

## Content

Adds a *T3 Code: Pair with Server…* command accepting a full pairing URL
(`http://host:port/pair#token=…`) or a bare pairing token, exchanges it
for a bearer access token via
the OAuth token-exchange endpoint, and stores it in `SecretStorage` so
`ensureConnected` picks it up
as the bearer tier. Removes the need to hand-exchange tokens with `curl`
before using *Set Server
Bearer Token*.

Unchanged from the original — 5 files, cherry-picked cleanly onto
`fork/dev` with no conflicts.

## Validation

- `apps/vscode` suite: **13 files, 44 tests pass**.
- Full recursive typecheck clean across all 17 packages.

## Note on the other external PR

[#238](#238) is **not** ported,
because it is already fixed on
`fork/dev` by a different route and porting it would duplicate the
behaviour:

- `ResolvedKeybindingsConfig` already uses the generic
`ForwardCompatibleArray` helper, whose decode
keeps only elements that decode and encodes unchanged — identical
semantics to the bespoke
  `filterKnownResolvedKeybindingRules` the PR introduces.
- `fork/dev`'s tests are **strictly broader**: unknown commands, unknown
`when`-node types, and
  malformed entries. #238 covers only the first.
- One of #238's assertions would now **fail**: it asserts
`filePicker.toggle` decodes away as
  unknown, but that command has since shipped and is known.

Recommend closing #238 as already fixed, with credit to @bulgadev for
reporting the class of bug.

Co-authored by [@patroza](https://github.com/patroza),
[@bulgadev](https://github.com/bulgadev)

opened by [Patrick Roza](https://discord.com/users/95218063095377920) in
chat thread **Discord** ·
[Discord](https://discord.com/channels/1083767712431480922/1534783738322485399/1534783738322485399)
· [T3](https://t3vm/?thread=584a9ad3-243e-4308-8a13-49acdd758b17)

---------

Co-authored-by: bulgadev <me@bulgaaw.com>
Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Co-authored-by: omegent-app[bot] <306514130+omegent-app[bot]@users.noreply.github.com>
@omegent-app

omegent-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thanks for this one too, @bulgadev — you were right about the bug, and it is fixed. Closing because
fork/dev already has it, arrived at independently.

ResolvedKeybindingsConfig now goes through a generic ForwardCompatibleArray helper:

export const ResolvedKeybindingsConfig = ForwardCompatibleArray(ResolvedKeybindingRule).check(
  Schema.isMaxLength(MAX_KEYBINDINGS_COUNT),
);

It decodes each element and keeps only the ones that decode, passing encode through untouched —
the same semantics as your filterKnownResolvedKeybindingRules, generalised so other contracts get
it for free. Merging yours on top would add a second, bespoke implementation of behaviour already in
place.

The test coverage that landed with it is also a superset of the two cases here:

  • rules whose command this build does not know — your case
  • rules with unknown when-node types
  • malformed entries ("garbage", null)

One thing worth flagging, since it says something about how long this sat: your second assertion
would now fail. It expects filePicker.toggle to decode away as unknown, but that command shipped in
the meantime and is in the known set — so a payload containing it decodes to one rule, not zero. The
class of bug you found is exactly why that matters, and the generic fix covers it.

For context on why this went quiet rather than getting reviewed: it targeted fork/changes, which
has since been frozen. This repository moved to a stable, never-rebased fork/dev as the contributor
target — partly because PRs like yours were being silently invalidated by rebases underneath them.
Your other PR (#237) is merged as #351 with your authorship on the commit.

fork/dev is the branch to target from here. Thanks again. 🙏

@omegent-app omegent-app Bot closed this Aug 6, 2026
omegent-app Bot added a commit that referenced this pull request Aug 6, 2026
Records the `fork/dev` development and release model — **adopted and
live since 2026-08-06**. This
started as a proposal; the migration then ran ahead of it, so the
document is now the record rather
than the plan.

Documentation only. Every mechanism it describes is already merged and
running.

## The model

`fork/dev` is the default branch, the contributor target and the release
source. It is never rebased.
The provenance stack `main → fork/base → fork/tim → fork/candidates`
stays rebased and feeds
`fork/dev` through reviewed tree deltas, so contributor bases are never
invalidated by an upstream
update.

| In place | |
| --- | --- |
| `fork/dev` cut from green `fork/integration` `21badd04e`, trees proven
identical | tag `fork-dev/2026-08-06.1` |
| Default branch, ruleset, squash-only, required checks | live |
| CI for `fork/dev` PRs and merges | #343 |
| Deployment promoting exact green `fork/dev` SHAs | ops `deploy.env` |
| Validation and release split | #347, #349 |
| First provenance sync, upstream `2a04db134..a2ca89a` | #345, tag
`fork-dev/2026-08-06.2` |
| Upstream ancestry recorded so "behind" reads true | `3a7e7a458` |
| Overlays drained and deregistered | #348 |

## What this revision corrects

The document had drifted from what was actually built:

- **Release is two workflows, not one.** `fork-ci` decides whether a SHA
is valid; `fork-release`
acts on that verdict via `workflow_run`. A release action must never be
able to veto a validation
verdict — when mobile dispatch lived inside `fork-ci`, one failed EAS
call marked a valid SHA
  unapprovable and stranded the whole fleet.
- **Check selection is *not* path-inferred**, and the document
previously implied it should be. Every
PR runs all four required checks; only *release* scope is classified. A
path filter that errs
narrow silently skips a check on a protected branch, which is worse than
a slightly slower suite.
- **`fork/changes` and `fork/integration` are frozen**, not fallbacks.
- Ops parameterization and the `deploy.env` cutover are **done**, not
pending.
- Steps that were "do now" are recorded as done, with real SHAs, tags
and ruleset contents.

## What the cutover surfaced

Added as a section, because each cost a round trip and the old path hid
all of them:

- `fork/dev` had **no CI path at all** — no `push` trigger, not listed
as a `pull_request` base.
- **Mobile releases would have stopped silently**; nothing errors when a
gated job just never fires.
- Both mobile workflows **hardcoded `ref: fork/integration`** and
rejected every `fork/dev` SHA.
- A release failure could **strand the fleet**.
- **Every PR based on `fork/changes` was already broken** by earlier
rebases — GitHub reported them
as 60–100 commits and 629–741 files. Each was one commit of real work on
stale history, fixed by
  cherry-picking that commit rather than replaying the branch.

That last one is the clearest evidence for the whole premise: the old
model was silently corrupting
in-flight work, and nobody could see it.

## Deliberately not done

Clean downstream projection is deferred indefinitely and nothing depends
on it. Provenance sync stays
manual. The overlay machinery is still present and still passes its
tests with an empty manifest;
removing it touches ~20 files and is a separate decision.

## Still open

PRs #317, #226 and #185 conflict when cherry-picked onto `fork/dev`;
#237 and #238 live in an
external fork and need their author. `fork/changes` and
`fork/integration` can be deleted once those
are drained.

## Also: no guidance targets an overlay any more

The overlays were drained in #348, but the instructions an agent or
contributor actually reads before
opening a PR still sent them at `fork/discord`, `fork/vscode`,
`fork/identity`, the desktop
deep-links branch, or `fork/changes`. Left alone, the next client-owned
change would have been opened
against a **closed overlay on a frozen branch**.

- **`CLAUDE.md`** (`AGENTS.md` symlinks to it): branch from and target
`fork/dev` for every kind of
work; `main`, `fork/changes` and `fork/integration` named as bases never
to use; the
"register an `integrationOverlays` entry" instructions replaced with a
record that it is empty.
- **`apps/discord-bot/docs/agent-turn-rules.md`**: recovery branches
pointed at *"the correct base
  (`fork/discord` overlay / `fork/changes` / etc.)"* → `fork/dev`.
- **`fork-stack.md`, `stack-ship-path.md`, `client-overlays.md`**:
bannered as superseded rather than
rewritten — the provenance stack they document is still current and they
are the record of how the
fork worked before the cutover. The two lines that literally instructed
a base are corrected.

Verified by grep: nothing in the repository still directs a PR anywhere
but `fork/dev`.

## Validation


`vp fmt --check` clean; internal anchors checked. Documentation only —
no code, tooling or workflow
changes in this PR.

Co-authored by [@patroza](https://github.com/patroza)

opened by [Patrick Roza](https://discord.com/users/95218063095377920) in
chat thread **Discord** ·
[Discord](https://discord.com/channels/1083767712431480922/1534783738322485399/1534783738322485399)
· [T3](https://t3vm/?thread=584a9ad3-243e-4308-8a13-49acdd758b17)

---------

Co-authored-by: T3 Code PR Stack <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: omegent-app[bot] <306514130+omegent-app[bot]@users.noreply.github.com>
Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
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.

3 participants