Skip to content

fix(projects): prevent invalid script IDs from crashing threads - #10019

Merged
juliusmarminge merged 4 commits into
pingdotgg:mainfrom
saphid:fix/project-script-id-guards
Sep 7, 2026
Merged

fix(projects): prevent invalid script IDs from crashing threads#10019
juliusmarminge merged 4 commits into
pingdotgg:mainfrom
saphid:fix/project-script-id-guards

Conversation

@saphid

@saphid saphid commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What Changed

Prevent invalid project script IDs from crashing thread views and project settings. New script IDs are checked against the existing shortcut schema before the server emits an update. Previously saved IDs remain readable, editable, runnable and removable; unsupported shortcut commands return null instead of throwing.

Why

A saved ID such as install-javascript-dependencies passes the project schema but exceeds the shortcut schema's 24-character limit. Rendering the script menu then throws Expected a string matching template literal parts and replaces the entire thread with an error page. Validating only newly introduced IDs avoids locking users out of repairing existing scripts or replaying old events.

Verification

CI passed on 0543a40fe, including all server test shards, release smoke, Rust and the repository check job. Cursor Bugbot and all Macroscope checks passed with no actionable findings; Macroscope submitted an approval. CodeRabbit automatic review is disabled.

  • Regression tests failed on the base with the exact crash and accepted invalid updates: 15 failures, 23 passes.
  • vp test run apps/web/src/projectScripts.test.ts apps/web/src/lib/projectScriptKeybindings.test.ts apps/web/src/keybindings.test.ts apps/server/src/orchestration/decider.projectScripts.test.ts — 91 passed.
  • vp exec tsgo --noEmit -p apps/web/tsconfig.json and -p apps/server/tsconfig.json — passed.
  • Targeted formatting and lint on the eight changed files — passed; existing ChatView effect warnings remain.
  • Disposable web app: reproduced the base crash, reopened the same thread with the same invalid ID after the fix, saved and ran the legacy action, edited it in project settings, verified persistence, then deleted it and verified the valid action remained. An invalid API update was rejected without changing saved scripts.

The server guard applies to all clients and connection modes; the rendering fix covers shared web/desktop UI. Native mobile and Electron-specific behavior were not exercised. HTTP retains its existing generic 500 response for command-invariant failures; the decider supplies the specific validation message. No wire or persisted-event schema changes.

UI Changes

Same synthetic project, script ID, dark theme and 1280×800 viewport. Before: upstream 363cde411; after: 0543a40fe. The action runs a harmless echo command.

Before: opening the thread crashes while building script shortcuts.

Before: invalid script ID replaces the thread with an error page

After: the thread opens and the same legacy action runs successfully.

After: thread and legacy action remain usable

Save and run the legacy action — 10-second web demo. Idle time removed; no playback speed change or timing claim.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for UI changes
  • I included a video for the affected interaction

Implemented by Codex (OpenAI) in the T3 Code Codex harness. Independent review command: claude --model claude-opus-5 --effort high --tools '' -p …; it exited 1 because OAuth was expired, so no Claude review occurred.


Note

Medium Risk
Touches orchestration invariants and shared script/keybinding UI paths, but behavior is narrowly scoped to script ID validation and graceful null handling rather than auth or data migration.

Overview
Fixes thread crashes when a project has legacy script IDs (e.g. install-javascript-dependencies) that pass project storage but fail the 24-character shortcut command schema.

Server: project.meta.update now rejects newly introduced script IDs that cannot form a valid script.{id}.run keybinding command, while still allowing existing IDs to be edited, removed, or kept alongside valid additions.

Web: commandForProjectScript returns null instead of constructing an invalid command; keybinding helpers and script persistence accept a nullable command so script menus and threads render without throwing, with no shortcut label for unsupported IDs.

Reviewed by Cursor Bugbot for commit 0543a40. Bugbot is set up for automated code reviews on this repo. Configure here.

Current-base verification

Updated to upstream main b2e15185aee22bfdee78806f2734b0a4de9c2c77 at 29e3cf42d2ff5ee18daa6b913d467b67c182c4a5. All 6 applicable focused verification commands passed (38 passing test executions). Earlier runtime observations and media retain their stated revisions and limitations; these fresh runs do not constitute new client recordings. Fresh independent Claude review is unavailable because its OAuth credentials have expired. Refreshed by GPT-6 Astra in the Codex harness.

Note

Reject invalid script IDs in project.meta.update decider and handle null commands in web client

  • Adds a script-command schema matcher and max ID-length constant to the orchestration decider. During project.meta.update, only newly introduced script IDs are validated; persisted legacy IDs pass through so they can be edited or removed.
  • commandForProjectScript in projectScripts.ts now returns a nullable command. IDs that don't match the script-run format produce no keybinding command, so legacy scripts without shortcuts still work.
  • Web keybinding utilities (shortcutLabelForCommand, keybindingValueForCommand, decodeProjectScriptKeybindingRule) and the ChatView persistence callback accept null commands. Null commands yield null shortcuts and cannot be assigned a non-empty keybinding rule.
  • Behavioral Change: newly submitted script IDs that fail the format check now raise an OrchestrationCommandInvariantError; existing persisted invalid IDs remain accepted but produce no shortcut command.

Macroscope summarized 29e3cf4.

Latest-main refresh — 7 September 2026

Merged upstream main 08c715ed93f8a6bfd85d42f5a8b7dbe12404554d into the existing branch at 8ce906ce62539a4f0bd7a69d87133a52128aa47a. The contribution remains eight focused paths and is still absent from main.

Fresh verification on that exact head passed: 16 project-script decider tests, 22 web script/keybinding tests, server and web typechecks, scoped lint, scoped formatting, and git diff --check. Scoped lint retains only existing ChatView React effect warnings. Every current commandForProjectScript caller was reviewed for the nullable legacy-ID result; no actionable issue was found.

Earlier real-client evidence and media retain their recorded revisions and limitations; this refresh did not run a client or create new media. No fresh independent Claude review occurred because the known OAuth credential remains expired. Refreshed and self-reviewed by GPT-6 in the Codex harness.

Latest-base follow-up

After upstream advanced, merged pingdotgg/t3code@5b68b2c8e1e971e4ee6956157a1698646ef52a55 and pushed exact head 5b62d5a9d0383e1e16fa05832415a9c30b14400f. The additional upstream commit changes React Native mobile and shared orchestration timing paths, with no overlap in this contribution's eight files. On the final head, the 16 server tests and 22 web tests passed again; server and web package typechecks, scoped lint, formatting, and git diff --check also passed. Existing lint warnings in ChatView.tsx and Effect typecheck suggestions remain outside the contribution.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 5, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 0543a40

Macroscope's review found this PR approvable — This is a narrowly scoped fix for legacy project-script IDs, adding server validation and nullable shortcut handling while preserving existing valid and persisted scripts. The affected paths are well covered by targeted regression tests, with no schema, deployment, security, billing, default, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 2da3a1cd-a945-46b5-bfc9-73f76c6748eb

📥 Commits

Reviewing files that changed from the base of the PR and between 08c715e and 8ce906c.

📒 Files selected for processing (8)
  • apps/server/src/orchestration/decider.projectScripts.test.ts
  • apps/server/src/orchestration/decider.ts
  • apps/web/src/components/ChatView.tsx
  • apps/web/src/keybindings.ts
  • apps/web/src/lib/projectScriptKeybindings.test.ts
  • apps/web/src/lib/projectScriptKeybindings.ts
  • apps/web/src/projectScripts.test.ts
  • apps/web/src/projectScripts.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The server now validates new project script IDs against the supported format while preserving existing legacy scripts. The web application represents unsupported script commands as null and handles them during shortcut labeling, decoding, persistence, and testing.

Changes

Project script compatibility

Layer / File(s) Summary
Server script ID validation
apps/server/src/orchestration/decider.ts, apps/server/src/orchestration/decider.projectScripts.test.ts
Project updates reject invalid new script IDs, accept IDs up to MAX_SCRIPT_ID_LENGTH, and preserve existing legacy scripts for editing and removal.
Nullable project script commands
apps/web/src/projectScripts.ts, apps/web/src/projectScripts.test.ts, apps/web/src/components/ChatView.tsx, apps/web/src/keybindings.ts
commandForProjectScript returns null for unsupported IDs. Command persistence and shortcut labels accept nullable commands.
Legacy keybinding handling
apps/web/src/lib/projectScriptKeybindings.ts, apps/web/src/lib/projectScriptKeybindings.test.ts
Keybinding lookup returns null for absent commands, while decoding rejects shortcut rules for legacy commands. Tests cover these cases.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 8ce90

Project scripts with unsupported legacy IDs no longer crash thread views or settings, while new invalid IDs are rejected without changing saved scripts. The covered validation and compatibility paths indicate no remaining merge-blocking risk.

Suggested reviewers: juliusmarminge, justsomelegs, t3dotgg

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing invalid script IDs from crashing thread views.
Description check ✅ Passed The description covers the required What Changed, Why, UI Changes, and Checklist sections. It explains the validation behavior, legacy-ID handling, verification results, and UI evidence.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge
juliusmarminge merged commit d8bc683 into pingdotgg:main Sep 7, 2026
24 checks passed
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 7, 2026
## What's Changed
* fix(mobile): keep pending messages in the chat timeline by @juliusmarminge in pingdotgg/t3code#10449
* fix(mobile): show connection status in the floating pill instead of a second one by @juliusmarminge in pingdotgg/t3code#10440
* fix: use Pierre icons consistently for attachments by @juliusmarminge in pingdotgg/t3code#10475
* feat(mobile): open the thread screen as soon as a new task is submitted by @juliusmarminge in pingdotgg/t3code#10435
* fix(devcontainer): make repository setup work by @saphid in pingdotgg/t3code#7875
* fix(projects): prevent invalid script IDs from crashing threads by @saphid in pingdotgg/t3code#10019
* fix(mobile): hide changed-files navigator and restore refresh in raw diff fallback by @lnieuwenhuis in pingdotgg/t3code#9828
* fix(ios): scroll short source files from blank space by @juliusmarminge in pingdotgg/t3code#10178
* feat(mobile): start a new thread on an existing branch by @StiensWout in pingdotgg/t3code#10359
* fix(mobile): improve font-size slider performance and prevent maximum update depth errors by @bbernag in pingdotgg/t3code#7138
* fix(web): keep composer toolbar controls anchored during transitions by @juliusmarminge in pingdotgg/t3code#10478
* fix(web): resize the floating preview from any edge by @juliusmarminge in pingdotgg/t3code#10467
* fix(mobile): prevent chat from disappearing when scrolling by @juliusmarminge in pingdotgg/t3code#10479
* fix(mobile): smooth composer status pill resizing by @juliusmarminge in pingdotgg/t3code#10484
* fix(mobile): release initial scroll target after dragging by @juliusmarminge in pingdotgg/t3code#10483
* fix(mobile): animate thread lifecycle transitions consistently by @juliusmarminge in pingdotgg/t3code#10487
* fix(mobile): restore assistant message bottom padding by @juliusmarminge in pingdotgg/t3code#10491
* fix(mobile): preserve chat rows when toggling commands by @juliusmarminge in pingdotgg/t3code#10492

## New Contributors
* @bbernag made their first contribution in pingdotgg/t3code#7138

**Full Changelog**: pingdotgg/t3code@v0.0.39-nightly.20260907.1332...v0.0.40-nightly.20260907.1346

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.40-nightly.20260907.1346
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 7, 2026
Merges 141 upstream commits (`b438447f6..8b2838e`) into the fork,
following the
`fork-upstream-merge` skill. Landed as a merge commit; conflicts
resolved by the
path-policy verdicts in `docs/fork/inventory.json`.

## Merge shape

563 files landed (`git diff --stat HEAD^1 HEAD`) against 563 in the
upstream
range, so no upstream work was dropped. Fork delta 720 files.

## Conflicts

14 files, resolved by concern. The merge commit message names each. Two
are worth
carrying forward:

- Upstream extracted the project action rows into
`ProjectActionsList.tsx`. The
fork's Edit gate now rides an `editable` prop that defaults to
upstream's
  always-editable behavior, so the gate survives the next extraction.
- Upstream moved the `agent-browser-access` setting onto its new
`/settings/projects` page. `settingsSearch.ts` points that item there
and drops
  a fork delta.

## Two things a clean merge did not show

**Upstream can delete an export the fork still imports.** pingdotgg#10225 removed
`ClientTracingLive` as unused. `apps/web/src/lib/runtime.ts` still
installs it,
and neither file conflicted, so the typecheck failed four ways on a
merge git
called clean. Restored with a `// Fork:` comment naming the consumer.

**A green test step can hide a suite that never finished.** `vp run -r
test`
kills the packages still running when one of them fails. A
`@t3tools/desktop`
failure truncated `apps/web` and `@t3tools/mobile` after each had
reported
hundreds of passing files, and `verify.mjs` counted any package with
labeled
output as tested. Four failing web tests went unreported. The check now
keys on
the closing `Test Files` line and runs every unfinished package alone.

## Unsupported methods

`unsupported-methods.mjs` reports 0 ADD, 0 DROP, 2 KEEP, against 61
dispatched
backend methods and 131 contract methods.

Getting there took a fix. The backend moved its dispatch from
`crates/t3code/src/lib.rs` to `crates/t3code/src/rpc/dispatch.rs`, where
every
arm is a one-line call into a handler below the match. The script read
the old
path and reported zero dispatched methods, then read the new one and
called
`vcs.switchRef` a DROP, because the `unsupported_exit` that refuses it
had moved
out of the arm. It now tries both paths and follows an arm two calls
deep.

Contract changes: `provider.consumeResetCredit` and
`server.getHostResources`
gained `UnsupportedMethodError`; `server.getUsageSummary` lost it, which
closes
the item the previous merge left open.

## Feature classification

**Usable as-is** — client-only, nothing new on the wire. Sidebar drag
across
sections with destination cues and a named drop action (pingdotgg#9731, pingdotgg#9750,
pingdotgg#10378,
pingdotgg#10453, pingdotgg#10464). Composer behavior: a multiline draft survives timeline
scrolling (pingdotgg#10444), the composer stops collapsing on blur (pingdotgg#10437) and
regains
focus when you tab back (pingdotgg#10463). Panel and preview chrome: resize the
floating
preview from any edge (pingdotgg#10467), toolbar controls stay anchored (pingdotgg#10478),
the
stuck resize cursor clears (pingdotgg#10461), the browser hides as the right
panel closes
(pingdotgg#10385), manual panel choices hold during a turn (pingdotgg#10113). Settings and
accessibility polish (pingdotgg#10177, pingdotgg#10262, pingdotgg#10415, pingdotgg#10258, pingdotgg#10124, pingdotgg#10125,
pingdotgg#10127,
pingdotgg#10128, pingdotgg#10175). Performance (pingdotgg#10413, pingdotgg#10190, pingdotgg#10118). Plus the GitHub
mark on
`github.com` links (pingdotgg#10324), the Tux icon for WSL (pingdotgg#8511), a remembered
usage
page selection (pingdotgg#10189), project settings in the legacy sidebar menu
(pingdotgg#10021),
and text-only preview snapshots (pingdotgg#10232).

**Unsupported in Moatless** — resolves to a refusal, or falls through to
its own
empty state. Each is recorded in `docs/fork/gaps.md`:

- Reset credits through the hub and CLIProxyAPI (pingdotgg#10462, pingdotgg#10395,
pingdotgg#10308).
`provider.consumeResetCredit`, new union entry. `UsageLimits.tsx`
catches the
  refusal and shows "Could not use the reset credit."
- Balancing new threads across connected machines (pingdotgg#9895, pingdotgg#10433,
pingdotgg#10407).
`server.getHostResources`, new union entry. Nothing polls until a user
picks
automatic routing, and the composer then reads "Auto balance
unavailable."
- Onboarding: import grouped by repository (pingdotgg#10493), the shared
multi-computer
  wizard (pingdotgg#10465), agent install without Node or npm (pingdotgg#10402). All ride
  `agentSessions.scan` and `.import`, an existing gap.
- Shared project defaults and scoped overrides (pingdotgg#9754). The page reads,
and every
  write goes through `server.updateSettings`, which the backend does not
  dispatch.
- Two new `orchestration.dispatchCommand` types: `thread.active.reorder`
(pingdotgg#9729)
and `thread.user-input.dismiss` (pingdotgg#10431). Both are ordinary controls, a
sidebar
drag and a Dismiss button, and a dispatched command cannot be refused
per type.
That is the standing _A command cannot be refused_ gap, now 26 members
wide.

**Backend behavior to consider reproducing in Moatless** — upstream
server fixes
whose behavior the fork's client assumes:

- Invalid script IDs no longer crash threads (pingdotgg#10019). The fork ships
project
  scripts, so this one is worth reading first.
- Settlement: settle inactive threads without a PR lookup (pingdotgg#10103), skip
disabled
settlement lookups (pingdotgg#10424), settle threads with unanswered async
questions
  (pingdotgg#10400).
- Interrupted threads stay resumable after a restart (pingdotgg#10421).
- Completed requests stay closed across clients (pingdotgg#10123).
- Placeholder branches are followed after a checkout updates (pingdotgg#10441).
- A thread's PR links without an open client (pingdotgg#10101), and checkpoints
are
  captured before a PR status refresh (pingdotgg#10347).
- Adapters declare their own context compaction (pingdotgg#10112).
- Transcripts with oversized tool records import (pingdotgg#10430), and git
status scans
  are skipped while the index is locked (pingdotgg#9845).
- Usage limits pool per provider across accounts and environments
(pingdotgg#10300). The
client renders what `server.getUsageSummary` returns, so this shows
something
  only if the Moatless payload carries per-account limits.

Mobile, marketing, desktop, provider adapters and release tooling are
not this
fork's surface and are not classified.

## Also fixed here, and not upstream's doing

- Three `browser-*` search items still routed to
`/settings/integrations`, which
the fork owns for its Moatless administration page. A non-administrator
who
  searched for them was redirected away from the result.
- `moatless/listSearch.ts` carried no fork-only declaration.
- `pnpm fmt:check` failed on 294 files, 293 of them orval output. The
generator
  now formats what it writes through an `afterAllFilesWrite` hook.
- `@t3tools/moatless-api` exported `./generated`, a barrel that is never
checked
  in.

## Verification

`inventory-check.mjs` clean. `verify.mjs` green on seven checks:
duplicate-adds,
tripwires, resolution-check, unsupported-methods, `fmt:check` (3876
files),
`lint` and `typecheck`.

`test` is red on one package, and it is the machine.
`@t3tools/desktop`'s
`bundled libsecret helper` shells out to `pkg-config` for `libsecret-1`,
which
this sandbox does not have; it fails the same way when retried alone.
Everything
else passes: `apps/web` 369 files, `t3` 291, `@t3tools/mobile` 149,
`t3code-relay` 27, `@t3tools/client-runtime` 71, plus the smaller
packages.

`spec:check` cannot run in a sandbox: it needs a sibling `moatless`
checkout or a
deployment URL and has neither.

Written by Claude Opus 5 in Claude Code.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/8d348ecf-f9cb-4e65-b96b-6c1054a8ed33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants