fix(contracts): reject project script ids that cannot form keybinding commands - #7947
fix(contracts): reject project script ids that cannot form keybinding commands#7947IzonIcy wants to merge 2 commits into
Conversation
… commands ProjectScript.id validated as TrimmedNonEmptyString, but the web client rebuilds `script.<id>.run` keybinding commands while rendering thread views and project settings. An id outside that pattern's constraint (e.g. a UUID) persisted fine via project.meta.update, then threw inside the React error boundary on every thread view of the project — with no way to repair it from Settings. Tighten ProjectScript.id to the same constraint as the identifier portion of SCRIPT_RUN_COMMAND_PATTERN by extracting it as ProjectScriptId (single source of truth), so the server rejects such commands with a schema error instead of poisoning read models. Conforming ids decode unchanged; UI-created ids already satisfy the pattern via normalizeScriptId. Covers pingdotgg#7851 (option 1).
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. |
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. Closing this alternative in favor of #7961. Tightening ProjectScript.id also changes decoding of scripts that may already be stored. The retained approach accepts existing IDs when building keybinding commands instead of rejecting their saved data. Please preserve the failing command case in that compatible fix. #7961 is still open. If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look. |
What changed
packages/contracts: the identifier portion ofSCRIPT_RUN_COMMAND_PATTERNis extracted into an exportedProjectScriptIdschema (single source of truth), andProjectScript.idnow uses it instead ofTrimmedNonEmptyString. Behavior change is exactly one: commands carrying script ids that cannot form a validscript.<id>.runkeybinding command are rejected with a schema error at the dispatch boundary instead of being persisted.Two new tests in
orchestration.test.ts: the UUID id from #7851's repro now fails to decode; a conforming id still passes.Why
Fixes #7851, using its first suggested direction. The web client calls
commandForProjectScript(script.id)(SCRIPT_RUN_COMMAND_PATTERN.make("script." + id + ".run")) while rendering thread views and Settings → Projects. The server accepted any trimmed non-empty id, so one dispatched UUID poisoned every read model of that project and both surfaces threw inside the React error boundary with no in-app way to repair — the persisted data re-threw on every render.Client-side tolerance was the alternative direction, but it would add null-handling through ~10 keybinding call sites to accommodate input the system never intended to accept. Rejecting at the schema keeps orchestration pure and the client dumb. UI-created ids already conform via
normalizeScriptId, so nothing changes for normal flows. Note this prevents new poison but does not migrate already-poisoned projects — happy to look at a cleanup path if maintainers want one.Verification
--
Worked by ox-alpha via opencode (x-preview-f-free).
Note
Medium Risk
Tightens decode of project scripts and related events/commands. Existing payloads with UUID or otherwise non-conforming ids will now fail schema decode rather than persist or render.
Overview
Stops persisting project script ids that cannot form a valid
script.<id>.runkeybinding. The web client rebuilds that command while rendering, so a UUID (or other non-conforming id) previously poisoned the project and crashed thread/settings views.Exports a shared
ProjectScriptIdschema (lowercase alphanumerics/hyphens, max length) and uses it for bothSCRIPT_RUN_COMMAND_PATTERNandProjectScript.id. Tests cover rejecting a UUID and accepting a conforming id.Does not migrate already-poisoned projects; it only blocks new ones. UI-created ids already go through
normalizeScriptId/nextProjectScriptId.Reviewed by Cursor Bugbot for commit f5999a9. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reject project script IDs that cannot form keybinding commands
Adds a
ProjectScriptIdschema that enforces lowercase alphanumerics and hyphens (starting alphanumeric, withinMAX_SCRIPT_ID_LENGTH). ChangesProjectScript.idfromTrimmedNonEmptyStringto this schema so payloads with non-conforming IDs fail decoding. TheSCRIPT_RUN_COMMAND_PATTERNnow reusesProjectScriptIdfor the script-id portion.ProjectScript.iddecoding now rejects IDs like UUIDs or strings with uppercase/underscores; existing payloads with such IDs will fail to decode.Macroscope summarized f5999a9.