Skip to content

fork: CI check that refuses database schema and migration changes - #3

Merged
scuttletoby merged 6 commits into
mainfrom
fork/no-schema-changes
Nov 12, 2026
Merged

scuttletoby merged 6 commits into
mainfrom
fork/no-schema-changes

Conversation

@scuttletoby

@scuttletoby scuttletoby commented Nov 12, 2026 •

Copy link
Copy Markdown
Owner

This adds a CI check that rejects any fork change to the database schema.

T3 has no down-migrations. A fork patch that changes the schema would leave every install that later moves back to upstream with a database upstream does not know. Fork state belongs in existing JSON fields such as settings and model options.

What it adds

  • .github/scripts/fork-schema-check.cjs <base> <head> reads git diff --raw -z merge-base head and reads every changed file as a git blob (before and after). It refuses:
    • a change to a schema root (apps/server/src/persistence/Migrations/**, Migrations.ts, databaseMigrationHead.ts, initializeV2Database.ts, reconcileV2PreviewMigration.ts) or to any file they import, transitively;
    • a change to how those imports map to files: the workspace package list (pnpm-workspace.yaml packages:); in the package.json of an imported workspace package, the fields that answer the imports schema code makes (name, type, main/module/types/browser, imports, and the exports entries for those keys, e.g. "." of contracts and "./assistantCitations" of shared); and the dependency spec of every name schema code imports (@t3tools/contracts, @t3tools/shared, effect): its entry in the dependency fields of the packages holding schema code (apps/server, packages/contracts, packages/shared) and of the root package.json, in pnpm-workspace.yaml catalog:/catalogs:/overrides:/patchedDependencies: (override keys like a>effect@^4 count), and in the root's pnpm.overrides/pnpm.patchedDependencies/resolutions; the patch file of such a name is guarded like schema code. Specs of other names, and other export entries (a new shared module), stay free;
    • an import of that code that should resolve inside the repository but does not, when the branch brings it (a changed or newly guarded file, or a target it moved or broke), naming the import: a workspace package none of whose entry points for that import is a file, a relative import of a missing file, a #name import, a computed import()/require(), import.meta.glob, a file whose comment or template never ends. Ones already on the merge-base (e.g. from an upstream release) are listed as "already on the merge-base (not this branch's doing; the check needs teaching)", not refused. Imports are read from code with comments and string contents blanked, so prose such as require (in order) in a comment is not an import. Only packages outside the workspace resolve to nothing; the output lists them ("Imports from outside this repository (their dependency specs are guarded, their code is not): effect, node:fs, node:sqlite");
    • a change to a shipped source file (.ts/.tsx/.mts/.cts/.js/.mjs/.cjs/.sql under apps/ or packages/, not a test path) that holds a schema statement before or after the change: CREATE [UNIQUE] TABLE/INDEX/VIEW/TRIGGER, CREATE VIRTUAL TABLE, ALTER TABLE, DROP TABLE/INDEX/VIEW/TRIGGER. Between the keywords may stand line breaks, SQL comments (/* */, --) and string joins ("CREATE " + "TABLE"). An interpolation in place of the object kind (CREATE ${kind}) counts too, but only with upper-case keywords, so UI text like `Create ${label}` stays free;
    • a PR copy of the check's own files (the script, its tests, fork-checks.yml) that differs from the base's copy, once the base has the check;
    • a PR copy of any .gitattributes that differs from the base's copy (it can hide changes from review);
    • a branch holding commits already on main (built on an old main that has since been rebased, or cherry-picking main's commits; matched by author, author time and subject). The message says "rebase it onto main". A check file that the branch really edits is still reported as an edit, with "(also rebase onto main)".
  • fork-checks.yml runs the script's tests and then the check on every pull request.
  • The follow-upstream workflow (fork: follow each stable upstream release with the patch series #4) runs the same script over the whole patch series.

Not caught, on purpose (the check stops mistakes; review stops deliberate tricks):

  • DDL kept in a test-path file (/test/, .test., testkit) or in a non-source file (.txt, .json, .md), even when shipped code loads it;
  • a statement whose CREATE/ALTER/DROP keyword is itself computed, or lower-case keywords around an interpolation;
  • re-pointing an import outside the files and specs listed above: a build alias (resolve.alias in apps/server/vite.config.ts, which vp pack uses), tsconfig paths (none today), the lockfile (pnpm-lock.yaml), .npmrc, .pnpmfile.cjs hooks (readPackage can re-point effect or @t3tools/* without touching a guarded spec) and hoisting or linking settings (publicHoistPattern, node-linker, dependenciesMeta, packageExtensions), YAML anchors or flow maps spread over lines in pnpm-workspace.yaml;
  • the code of outside packages (effect): only their specs are guarded;
  • what an import the merge-base already cannot follow reaches (it is listed in the output; a branch could change its target unseen);
  • files schema code reads at run time (readFileSync; none in the persistence code today), and a browser object map in a package.json (the server runs on Node);
  • wording only: when the merge-base already has a computed import() in a guarded file and a PR adds another to the same file, the output lists the new one as "already on the merge-base" (the run is still red: the edit to a guarded file is refused);
  • a regular expression literal right after ) or } is read as a division, which can hide an import on the rest of that line.

Caught on purpose, though not server schema: apps/web/src/components/files/binaryPreview.worker.ts holds CREATE VIRTUAL TABLE inside a LIKE pattern (a web worker reading SQLite files, not the server database), so any edit to it is refused. The rule before this PR did the same; an owner merge past red is the way through.

Decisions

  • pull_request_target, not pull_request. GitHub then runs the workflow file and the script from the default branch, so a pull request cannot weaken, skip or delete the check that judges it. The pull request's commits are only fetched by SHA as git objects (persist-credentials: false) and read as text; nothing from them is checked out or run. Permissions are contents: read; actions/checkout is pinned by commit SHA.
  • Check files are judged against the base's copy, not the merge-base. After the follow workflow rebases main onto a new upstream release, a branch from the old main carries byte-identical check files; those pass, and the branch is told to rebase. Merging it as it is would put the old patch commits onto main a second time.
  • Which files count as schema code is computed, not listed. The script follows relative imports and package imports from the schema roots. Workspace packages come from the pnpm workspace list (scoped or not, at any depth). For a package import it guards every file any export condition can pick (node, import, types, default, custom, nested, arrays, ./* patterns), so it never guesses which condition a tool uses; without exports it follows main, module, types, a string browser and index. Of the package's package.json only the fields that answer those imports are compared (see above), so upstream's and the fork's ordinary edits to packages/shared/package.json (about 400 subpath exports) stay free; an entry schema code imports through cannot be swapped, gain a condition or a competing pattern. For a package root (@t3tools/contracts) it follows export * / export { } from to the files that declare the imported names (e.g. migration 064 -> threadPullRequest.ts and what that imports). The barrels on that path (contracts/src/index.ts) are guarded as well, but their other re-exports are not followed, so the rest of the package (settings.ts, model.ts) stays free. A name the tracer cannot find a declaration for fails closed: the barrel and everything it re-exports are guarded. The closure is the union of the merge-base and head trees. So re-pointing a name schema code imports is caught when the edit is to a barrel on the path, to a new or changed declaring file, to the export entry or fallback fields that map it, or to its dependency spec, catalog, override or patch; the other channels are under Not caught.
  • Bootstrap. pull_request_target runs main's copy, so this PR itself gets no run of the check until it is merged. Edits to the check files are allowed only while the base does not have the check yet.
  • baseHasCheck stays read from the event's base SHA, not from the checkout: a PR into another branch (e.g. fork/ci-base) must be judged against that branch, which the default-branch checkout is not.

Branch protection (owner, after merge; nothing is set yet)

A check only blocks merges when a ruleset requires it. Suggested ruleset for the default branch:

  • target ~DEFAULT_BRANCH; enforcement active
  • rules: deletion; non_fast_forward; pull_request with required_approving_review_count: 0 (an owner cannot approve their own PR); required_status_checks with context No database schema changes and integration_id 15368 (GitHub Actions)
  • bypass actors:
    • repository role admin (id 5) with bypass_mode: pull_request: the owner can merge past a red check only through a visible PR, never by a direct push;
    • for the follow-upstream workflow's force-push of main (fork: follow each stable upstream release with the patch series #4): one dedicated actor with bypass_mode: always, either a deploy key (actor_type: DeployKey, write, this repo only) or a GitHub App installed only on this repo. Unverified: that a deploy-key or App push may change files under .github/workflows/ (a token push needs the workflow scope). Until that is tested, the follow workflow keeps FORK_PUSH_TOKEN, which then needs an always bypass of its own.
  • the push secret (FORK_PUSH_TOKEN, or the deploy key) goes in an Environment (e.g. fork-push) whose deployment branches are limited to main, and the follow job names environment: fork-push (a change in fork: follow each stable upstream release with the patch series #4). Why: a repository secret is handed to every workflow run started from a branch of this repository, including a pull_request workflow that a PR branch adds itself (only PRs from other forks get no secrets). An Environment secret reaches only jobs whose ref passes the branch rule, and a PR run's ref (refs/pull/N/merge) does not.

Tested

  • Round 5: node --test .github/scripts/fork-schema-check.test.cjs: 47 pass at 5c2688e. The same tests against the dee218a script: 14 fail (the importer-side probes, manifest field judging, the merge-base rule (unit and e2e), the lexer, the mapping and imported-name assertions, the browser fallback, the outside wording, and 5 closure tests that no longer list package.json). Each fix broken by hand turns tests red: no spec comparison (1), no pnpm-workspace.yaml specs (1), no root overrides/resolutions (1), holders only the root (1), patch files not guarded (1), unresolved from both trees (2), no lexer (1), an open comment/template not reported (1), no browser fallback (1), the whole package.json compared (1), export entries ignored (2), imported names not collected (2).
  • Round 5, real tree: guarded set 114 files (the 116 of round 4 minus the two package.json files, now judged by field; the file list is otherwise identical), mappings contracts "." and shared "./assistantCitations", "./composerContextReferences"; guarded names @t3tools/contracts, @t3tools/shared, effect; holders root, apps/server, packages/contracts, packages/shared. Probe commits on this head: (A) apps/server @t3tools/contracts -> link:../../x/evil, (B) overrides: "@t3tools/contracts": link:./x/evil, (C) catalog effect -> link:./x/effect, (D) @t3tools/contracts -> npm:left-pad@1.3.0, (E) root pnpm.overrides.effect, (G) shared ./assistantCitations import swapped: the dee218a script exits 0 on A-E, this one exits 1 on all six. (F) a new shared export + module and (H) a ws catalog bump plus an effect-acp spec change: exit 0 (dee218a refused F). An "upstream" commit adding // each step must require (in order) and an import.meta.glob to Migrations.ts, then a README-only PR: dee218a exits 1 (both reported), this one exits 0 and lists the glob as already on the merge-base. This PR against main (ee69d98) and fork: follow each stable upstream release with the patch series #4 (fc08057) against this head: exit 0 (about 1.3 s).
  • Round 4: node --test .github/scripts/fork-schema-check.test.cjs: 41 pass at dee218a. The same tests against the ca47142 script: 13 fail (the three package.json tricks, all three listed as passing there; the unresolvable package e2e; outside list; workspace list; every export condition; main fallback; unresolved imports; workspace members; { S: renamed }; and the 4 closure tests that now include package.json). Each fix broken by hand at dee218a turns tests red: manifest not guarded (8), only the import condition read (1), unresolved package silent (2), relative import silent (1), #name as outside (1), computed import silent (1), test-path targets skipped (1), the old destructuring tracer (1), workspace list unchecked (1), scoped names only (8), no main fallback (2). The e2e "untraced name" test now reaches the fail-closed path: with the round-3 fail-closed return removed it fails (with 3 others).
  • Round 4, real tree: guarded set 114 -> 116 on this head (packages/contracts/package.json, packages/shared/package.json), 115 -> 117 on fork main; no unresolved import; outside: effect, node:fs, node:sqlite. Commits on this head that (1) swap contracts exports["."].import to ./src/settings.ts, (2) add a node condition to a new evil.ts, (3) drop exports for main: ./src/evil.ts: the ca47142 script exits 0 on all three, this one exits 1 on all three (about 1.2 s). This PR against main and fork: follow each stable upstream release with the patch series #4 (62216fc) against dee218a: exit 0.
  • node --test .github/scripts/fork-schema-check.test.cjs: 32 pass. Run against the a7328e4 script, 10 fail: declaration shapes, an untraced name (unit and e2e), a barrel-only re-point, barrels in both closure tests, a stale branch that also edits the check (unit and e2e), a cherry-pick-only branch, and the stale wording. On e9afd0c the 7 round-2 tests failed.
  • Real tree: a commit on this head that only appends export { ProviderKind as ThreadLinkedPullRequest } from "./orchestration.ts"; to contracts/src/index.ts: the a7328e4 script exits 0, this one exits 1. The closure gains exactly packages/contracts/src/index.ts (113 -> 114 files here, 114 -> 115 on fork main). No real import hits the fail-closed path.
  • Real tree: e9afd0c4 + a README commit, judged against 24e77473e + an empty "upstream release" commit + the two check commits cherry-picked (what fork: follow each stable upstream release with the patch series #4 produces): before, exit 1 with three "the owner merges past it"; now, exit 1 with "2 commit(s) of this branch are already on main as rebased copies ... rebase it onto main", in 1.5 s. Against the old main the same branch passes.
  • Real tree, round 2: the import closure grows from 103 to 113 files (threadPullRequest.ts, baseSchemas.ts, pullRequest.ts, sourceControl.ts, vcs.ts, composerContext.ts, environment.ts, guestShare.ts, chatAttachment.ts, assistantCitations.ts); settings.ts and model.ts are not in it. The wider statement rule flags no new shipped file outside the migrations.
  • This PR against main and fork: follow each stable upstream release with the patch series #4 against this head: exit 0.
  • Round 1: scratch branches in a clone of this repository, all refused: a change to the workflow trigger, a multi-line CREATE TABLE, an edit of the check itself, deleting the workflow, a non-ASCII file name, a .gitattributes change, and a change to a helper the migrations import. Settings-only changes and PRs fork: the server and CLI update from the desktop app's repository #1, fork: release workflow for v<upstream>-fork.N on GitHub-hosted runners #2 and Auto-update: server updates itself when quiet, desktop installs on quit #5 pass. 200 upstream commits before v0.1.9: the check flags upstream's migration 081, Migrations.ts and a DROP TRIGGER in messageSearchIndex.ts (not rerun this round).

Fail-open search (round 4)

Searched the resolver and tracer for a lookup that returns nothing and is skipped. Found and fixed: a workspace package with no resolvable entry (silent); a relative import of a missing file (silent); package names matched only when scoped, so effect-acp and the like were never followed; manifests read only at apps|packages/*/package.json, so deeper members were invisible; the tree held only source files, so an import of a .json or other file resolved to nothing; a package.json that is not JSON dropped silently (now an import matching no package is refused while one is broken); #name imports taken for outside packages; computed import()/require() and import.meta.glob not seen at all; test-path files that schema code imports skipped. Already closed: an untraced name and an unresolvable re-export inside a barrel (whole barrel walked). Left, on purpose or out of reach (listed under Not caught).

Round 5 closed the importer side (dependency specs, catalogs, overrides, patches); the remaining channels are in Not caught above.

Not proven yet

  • A live pull_request_target run, and that it satisfies a required status check. Both need this PR merged first.
  • That a deploy key or GitHub App can push workflow-file changes (see Branch protection).
  • Stale detection after a rebase that changes a commit's author or subject (not done by the follow workflow): such a branch then gets the plain check-file messages.

There is deliberately no override. Turning the check off means the owner changing this workflow and merging past the red check through a PR.

🤖 Generated with Claude Code

T3 has no down-migrations, so a fork patch that changes the schema would
strand every install that later returns to upstream. fork-checks.yml runs
on every pull request and fails when the change touches the migration files
or adds a schema statement to shipped source. The follow workflow runs the
same script over the whole patch series.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Nov 12, 2026
@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+design+tests · reviewer: opus high (t3lens-cdt) · read at: 2146267
covered: .github/scripts/fork-schema-check.cjs, .github/scripts/fork-schema-check.test.cjs, .github/workflows/fork-checks.yml (all in full); apps/server/src/persistence/Migrations.ts imports; apps/server/src/orchestration-v2/messageSearchIndex.ts:60-180; imports of every Migrations/*.ts; DatabaseMaintenance.ts compact; branch protection of main (gh api).
checks: Each case below was run on a local scratch branch off this head (never pushed). It used the head's own script, as CI does: node .github/scripts/fork-schema-check.cjs $(merge-base main pr3) HEAD.

  • Renamed migration inside Migrations/ (with and without the Migrations.ts import edit): refused. --no-renames turns it into a delete plus an add, and the deleted side keeps the old path. A move out of the folder is refused the same way. Deleted migration: refused (unit test plus trace).
  • New store file with a one-line CREATE TABLE IF NOT EXISTS / CREATE VIEW: refused. CREATE TEMP TABLE passes, which is fine (not persistent).
  • Unit tests: 4/4 pass.
  • State question: a rerun is deterministic. An empty diff passes with "No database schema changes.", so it says what it did. A missing base sha or an over-256MB diff makes execFileSync throw, and the job fails loudly. A PR on a stale base, or main rebased by the follow workflow: three-dot uses the merge-base, so only head-side commits count. Upstream migrations between the old and new base are not blamed on the PR, and old fork patches get rechecked, which is harmless. The first PR (this one) has no base copy of the check.
  • Security: permissions contents: read. base.sha/head.sha in run: are hex from the event, not free text. pull_request (not _target), so forks' PRs get no secrets. actions/checkout@v6 is a tag, not a SHA, same as upstream (nit).
    finding: major .github/workflows/fork-checks.yml:23-28. The check and its tests run from the PR's own copy, so a PR that changes the check is judged by its changed version. Scratch "selfedit": change line 9 Migrations( -> migrations( (a one-letter slip), update the test paths to match, and append a line to Migrations/081_ProjectionMessageSearchText.ts. Result: tests 4/4 pass, check "No database schema changes.", exit 0. Deleting fork-checks.yml in a PR is worse: no check runs at all, and main has no protection or required checks (gh api repos/scuttletoby/t3code/branches/main: protected false, required_status_checks empty), so nothing blocks the merge. The PR text says turning it off needs a reviewed PR, but nothing marks such a PR. Fix:
    • Run the base branch's copy of the script: git show "$BASE:.github/scripts/fork-schema-check.cjs". Fall back to the head copy only when the base has none, and print that it did.
    • Refuse, or loudly flag, any change to fork-schema-check*.cjs or fork-checks.yml.
    • Ask the owner to make "No database schema changes" a required check on main through a ruleset, with a bypass for the follow workflow's force-push.
      (high)
      finding: major apps/server/src/orchestration-v2/messageSearchIndex.ts:89-148 is schema-defining code outside SCHEMA_PATHS. Migrations 069, 074 and 081 build the FTS triggers by calling createMessageSearchTriggers / recreateMessageSearchTriggers from this file. The trigger bodies span many lines, and SCHEMA_STATEMENT only matches an added line that itself holds CREATE TRIGGER. Scratch "trigger": change messageSearchIndex.ts:109 WHEN (old.streaming = 0 OR new.streaming = 0) -> WHEN (old.streaming = 0). Result: "No database schema changes.", exit 0. A fresh fork install would then run migrations 069/074/081 with a trigger upstream never shipped, which is the schema drift the check exists to stop. The same per-line gap lets a new file with sql\\n CREATE\n TABLE IF NOT EXISTS fork_state (...)`` through (scratch "multiline2": exit 0). Other non-test files migrations import today: orchestration-v2/ProjectionStore.ts, orchestration-v2/EventStore.ts, persistence/Migrations/legacyThreadPullRequestKey.ts and Migrations/OrchestrationV2/* (the last two are already covered by the folder rule). Fix:
    • Treat the import closure of Migrations.ts (non-test files) as schema paths, computed or listed and tested.
    • Match statements with a regex that spans newlines over the whole post-image of each changed shipped file, compared with the pre-image, not added lines alone.
    • Add tests for both cases.
      (high)
      finding: minor .github/scripts/fork-schema-check.cjs:53-58. git quotes unusual paths, so the path rules miss them. Scratch "nonascii2": a new Migrations/082_Forké.ts shows as +++ "b/apps/server/src/persistence/Migrations/082_Fork\303\251.ts" -> "No database schema changes.", exit 0. It is inert until something imports it, and the import in Migrations.ts would be caught, hence minor. Fix: take paths from git diff --name-status -z --no-renames, or at least pass -c core.quotePath=false. Add a test with a quoted path. (high)
      finding: minor .github/scripts/fork-schema-check.cjs:50-60. A file only enters the list when a +++ line appears, and binary diffs have none. A PR's own .gitattributes can make text files binary. Scratch "attr": .gitattributes with apps/server/src/persistence/** -diff, plus edits to Migrations/081 and Migrations.ts -> "No database schema changes.", exit 0. Fix: add --text to the git diff, take the file list from --name-status -z rather than the +++ lines, and treat a .gitattributes change touching apps/ or packages/ as needing the same review as the check files. (high)
      optional nits:
  • actions/checkout@v6 -> pin by SHA (the lens rule; upstream uses tags).
  • The unit tests only feed hand-made diffs. One test that runs the real git diff in a temp repo (rename, quoted path, binary) would have caught the two minors.

Review round 1 of the schema check:
- The workflow runs on pull_request_target, so its file and the check script
  come from the default branch; a pull request can no longer weaken, skip or
  delete the check that judges it. The pull request's commits are only fetched
  and read as blobs, never checked out or run.
- Changes to the check's own files or to any .gitattributes are refused once
  the base has the check; only the owner can merge such a change.
- Files the migrations import (transitively, computed from both trees) count
  as schema files, and a shipped file whose text holds a schema statement
  before or after the change is refused; statements match across lines.
- Paths come from `git diff --raw -z` and texts from the blobs, so quoted
  non-ASCII paths and .gitattributes cannot hide a change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 1: the check can no longer be bypassed by the PR it judges (head e9afd0c479)

Finding Fix Test
A PR could edit or delete the workflow or script that judges it pull_request_target: workflow and script come from main; PR commits are fetched by SHA and only read as blobs, never checked out or run e2e: trigger edit, self-edit, workflow deletion refused
Multi-line CREATE TABLE slipped past the line-based match Statements matched across line breaks over the whole old and new file e2e: multi-line statement refused
Helpers imported by migrations were not covered Import closure computed from the schema roots (relative + package subpath imports) e2e: shared helper change refused
Non-ASCII paths and .gitattributes could hide changes git diff --raw -z with blob reads; .gitattributes changes refused e2e: non-ASCII path, attr change refused

17/17 tests pass; against the round-0 script 14 fail and every e2e case exits 0. The ruleset text for the owner is in the description. Not proven until merge: a live pull_request_target run satisfying a required check.

🤖 Generated with Claude Code

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: security+correctness r2 · reviewer: opus high (t3lens-secr2) · read at: e9afd0c
covered: the fix diff 2146267..e9afd0c in full: .github/scripts/fork-schema-check.cjs, .github/scripts/fork-schema-check.test.cjs, .github/workflows/fork-checks.yml. Also the imports of every schema root (Migrations/*.ts, Migrations.ts, initializeV2Database.ts, reconcileV2PreviewMigration.ts, databaseMigrationHead.ts), packages/shared/package.json exports, migration 064's imports, PR #4's copy of the check files (stacked on this head and identical to it), and the ruleset text in the PR body.
checks:

  • Tests: 17/17 pass on this head.
  • The check judging itself: pull_request_target runs the default branch's workflow and script. The PR head is only fetched as objects and read with ls-tree, cat-file and diff --raw. No checkout, textconv or ext-diff, so the PR's .gitattributes cannot change what the script sees. Permissions are contents: read, and the run: lines get only event SHAs, through env. I found no way for a PR to edit, skip or delete the copy that judges it.
  • Does a pull_request_target check reach the PR head, so a required check can work? On upstream's open PR (head 2b59d9f), the "Label PR size" job from pr-size.yml (pull_request_target) is a check run on the head SHA. So the run counts for the PR.
  • Spoof: a PR could add its own pull_request workflow with a job called "No database schema changes". The real run still lands on the head too, and GitHub requires every same-name check to pass (community report, not tested here), so a red real check still blocks. A pusher with write access is already trusted (agents push as the owner). The check stops mistakes, not a hostile pusher.
  • CI path, run for real: a blob-less clone of scuttletoby/t3code, then git fetch --filter=blob:none origin <base full sha> <PR #4 head full sha>. The fetch by SHA works for a fork-internal PR. The check reads blobs lazily and finished in 7.2 s with exit 0 (5 files), far below the 5 min timeout. A short SHA cannot be fetched, but the event always passes full ones.
  • Auto-found imports: the closure from this head's tree has 103 files. Outside Migrations/ it holds messageSearchIndex.ts, the 4 roots and 5 packages/shared files reached through subpath exports. The closure is the union of the merge-base tree and the head tree. A PR that points a package export at a new file, or drops an import, is therefore still caught. EventStore.ts and ProjectionStore.ts are imported only by tests, so leaving them out is correct. Not followed: package roots (see the 3rd minor).
  • Multi-line statements: CREATE\n TABLE, ALTER\n TABLE … ADD COLUMN and IF NOT EXISTS are refused. Interpolation, concatenation and SQL comments pass (see the 2nd minor). Only 2 shipped non-test files hold statements today: messageSearchIndex.ts and apps/mobile/src/persistence/mobile-database.ts. False positives stay low.
  • Bootstrap: until this merges, nothing runs, as the PR body says. Afterwards, an already-open PR gets its first run only on its next event. With a required check it waits, which fails safe. When a PR targets a branch without the check (e.g. [measurement only, do not merge] fork-tests.yml on v0.1.9 #7 -> fork/ci-base), main's script runs with baseHasCheck false. That is harmless, because the later PR into main is judged against main.
  • State question. A rerun is deterministic. An empty diff reports "0 changed files checked". A missing SHA or a git error throws, and the job goes red. Old or legacy state: main rebased by the follow workflow (fork: follow each stable upstream release with the patch series #4) breaks it, see the major.
    finding: major .github/scripts/fork-schema-check.cjs:111-118 (with 206-208, 223). The check-file rule compares the head with the merge-base, not with the base's content. After the follow workflow force-pushes a rebased main, every open PR that branched from the old main fails. Its merge-base is the old upstream commit, so the check files show up as "changed", although they are byte-identical to main. Scratch trace: branch prbranch = e9afd0c + a README edit. newmain = 24e7747 + an empty "upstream release" commit + cherry-picked 2146267 and e9afd0c (what fork: follow each stable upstream release with the patch series #4 produces). fork-schema-check.cjs newmain prbranch -> exit 1, three times "changes the schema check itself. Only the owner may do that: the check stays red, and the owner merges past it". Against the old main the same PR gives exit 0. This happens on every upstream release. It fails closed, but the reason is wrong, and the advice is to merge past red. Following that advice would also put the old patch commits back onto main a second time. The right advice is "rebase onto main". Fix:
    • For CHECK_FILES and .gitattributes, refuse only when the head blob differs from the base blob (ls-tree base -- f vs ls-tree head -- f, both absent counts as equal). Keep the merge-base diff for the schema rules.
    • When base is not an ancestor of head and the merge-base is not on base's first-parent line, print "this branch is based on an old main; rebase onto main".
    • Test: the newmain/prbranch case above passes, or gives the rebase message.
      (high)
      finding: minor .github/scripts/fork-schema-check.cjs:27-30. The statement rule matches only whitespace between the keywords. These pass (findSchemaChanges, path apps/server/src/fork/state.ts):
    • sqlCREATE ${kind} fork_state …
    • "CREATE " +\n "TABLE …"
    • sqlCREATE /* x */ TABLE …
    • DDL kept in a test-path file such as apps/server/src/fork/test/ddl.ts, or in a .txt asset, that shipped code imports and runs.
      The PR body's "built from pieces on several lines still counts" claims more than this. These are deliberate shapes, not likely slips, hence minor. Fix:
    • Reword the body.
    • Optionally, refuse shipped non-test code that imports a TEST_SOURCE path.
    • Optionally, match CREATE…TABLE with non-word characters between the two words, up to ~20 of them.
      (high)
      finding: minor apps/server/src/persistence/Migrations/064_CanonicalizeThreadPullRequestLinks.ts:1 imports the value schemas ThreadLinkedPullRequest/ThreadPullRequestLink from the @t3tools/contracts root, and they are defined in packages/contracts/src/threadPullRequest.ts:19. Package roots are not followed by design. So a fork change to that schema changes what migration 064 decodes and writes on any install that runs it, and nothing reports it. Fix: for root imports, follow only the barrel files that export the imported names (or add threadPullRequest.ts as a root), and add a test. (medium)
      finding: minor PR body "Branch protection": with the admin role as always bypass, anything holding the owner's token can push main past the check. FORK_PUSH_TOKEN as a repo secret is also readable by any same-repo pull_request workflow, so a PR branch that adds a workflow could read it. Safer, and possible on a personal repo:
      (a) make a deploy key (ruleset actor_type DeployKey, write, this repo only) or a GitHub App the only always bypass actor, and push from the follow workflow with that. Verify that a deploy key can push workflow-file changes.
      (b) give the admin role bypass_mode pull_request, so the owner merges past red only through a visible PR, never a direct push.
      (c) put the push secret in an Environment whose deployment branches are limited to main.
      (d) set required_approving_review_count: 0 in the pull_request rule, because the owner cannot approve their own PRs.
      (high on (b)-(d); medium on (a), whether a deploy key can push workflow files)
      optional nits:
  • fork-schema-check.cjs:223 reads baseHasCheck from the event's base.sha. Under pull_request_target the running copy is the default branch's by definition, so treeHas("HEAD", …) (the checkout) would not depend on base.sha being current.
  • fork-checks.yml:24: actions/checkout@v6 is a tag. It runs in a pull_request_target job, so pin it by SHA.
  • fork-schema-check.cjs:154: parseRawDiff breaks silently on an unexpected record. Throw instead, so a parse change shows up as red.

- A check file or .gitattributes that the branch carries unchanged from
  the base passes. After main is rebased onto a new upstream release, a
  branch from the old main is told to rebase onto main (its commits are
  matched on main by author, author time and subject), never to merge
  past the red check.
- Package root imports (@t3tools/contracts) are followed to the files
  that declare the imported names, so migration 064's schemas in
  threadPullRequest.ts are covered while the rest of the package is not.
- Schema statements also match across SQL comments and string joins, and
  with upper-case keywords around an interpolation (CREATE ${kind}).
- parseRawDiff throws on output it cannot read; actions/checkout is
  pinned by commit SHA.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 2 (head a7328e4), answering the r2 lens comment:

  • major, check files judged against the merge-base -> a check file or .gitattributes passes when the PR's copy equals the base's copy (ls-tree base blob vs the head blob). A branch whose commits are already on main as rebased copies (matched by author, author time and subject) gets "rebase it onto main", never "merges past it". Tests: e2e: after main is rebased onto a new upstream release, an old branch is told to rebase (temp repo: old main, PR, new upstream, cherry-picked patch, then rebase -> pass, then a real edit -> still refused), plus 2 unit tests. Real-tree trace from the lens comment: exit 1 with "2 commit(s) ... rebase it onto main" (was 3x "merges past it").
  • minor, statement shapes -> widened: SQL comments, string joins, and CREATE ${kind} with upper-case keywords. That adds no new flagged file outside the migrations, while Create ${label} UI text stays free. Test: refuses statements built with interpolation, string joins or SQL comments. DDL in test-path or .txt files, and computed keywords, are left out on purpose; the PR body now lists exactly what is caught and what is not.
  • minor, package roots -> root imports are followed through export * / export { } from to the files declaring the imported names. Migration 064 now covers threadPullRequest.ts and its imports, so the closure grows from 103 to 113 files; settings.ts/model.ts stay out. Test: the closure follows a package root only to the files that declare the imported names.
  • minor, ruleset text -> PR body rewritten: admin bypass pull_request, approvals 0, a dedicated always actor for the follow push (deploy key or App, workflow-file push unverified), and the push secret in an Environment limited to main, with the reason. No repo settings changed.
  • nits -> checkout pinned to d23441a4 (v6.1.0), with a test; parseRawDiff throws on unreadable output, with a test. Not done: baseHasCheck from the checkout, because a PR into a non-default branch must be judged against that branch.
  • Tests: 25/25 pass; on e9afd0c the 7 new tests fail.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+security r3 · reviewer: opus high (t3lens-3-r3) · read at: a7328e4
covered: the fix diff e9afd0c..a7328e4 in full: .github/scripts/fork-schema-check.cjs (all of it), .github/scripts/fork-schema-check.test.cjs, .github/workflows/fork-checks.yml. Also packages/contracts/package.json and src/index.ts, the root imports of every closure file, and the PR body's "Branch protection" text.
checks:

  • Tests: on e9afd0c its own suite passes 17/17. The new suite passes 25/25 on a7328e4. The new suite run against the e9afd0c script fails exactly the 7 new tests.
  • r2 major (rebased main): fixed at the root. Check files and .gitattributes are now compared with the base's blob (sameAsBase, :218, :353). A rebased branch is reported before any check-file reason (:209-221).
  • Hardest case 1. Can a PR that reuses a main commit pass while editing the check? No. Every staleCommits > 0 pushes a reason (:209-213), so the check goes red whatever else the branch holds. Scratch runs, each against base a7328e4:
    • (B) branch from 24e7747, cherry-pick 2146267, then edit the check script: exit 1, stale message plus "differs from main's copy".
    • (B3) the same cherry-pick plus a Migrations.ts edit: exit 1, the migration reason is still listed.
    • (B2) a branch behind main that only cherry-picks a main commit: exit 1 with "rebase" (see nit). It fails closed.
      The stale match can only add reasons, never remove them. sameAsBase only skips guarded files equal to the base's copy, so neither can hide a schema change.
  • Hardest case 2, re-export following. I ran synthetic trees through schemaClosure:
    • Followed correctly: an export * chain index->a->b, a renamed named chain ({T as S} -> {S as T}), export * as S, export type { S } with import type, and import { type S }.
    • Fail closed (whole barrel added): a default import, and a // comment inside the import braces.
    • Silently dropped: see the major below.
    • Real tree, head and fork main dc032d7: every name the closure imports from a package root resolves (closure 113 and 114 files), so no live gap today.
  • Hardest case 3, the widened DDL rule over the whole upstream tree. At fork main dc032d7 I scanned 4176 shipped source files, plus every test and other .ts/.js file. The new patterns flag 0 files that the old rule did not. The only shipped hits outside Migrations/ are the same 4 as before: mobile-database.ts, scripts/migrate-dev-db.ts, messageSearchIndex.ts and web binaryPreview.worker.ts.
  • Hardest case 4, the ruleset text, checked against docs.github.com (rules REST API; deployments and environments):
    • bypass_mode pull_request exists and is invalid for DeployKey actors. DeployKey is a valid actor_type (actor_id null).
    • Environment branch rules are matched against GITHUB_REF. A pull_request run's GITHUB_REF is refs/pull/N/merge, so a PR-branch workflow cannot reach a main-only Environment secret.
    • Admin role id 5 and integration_id 15368 match GitHub's values.
    • The body correctly marks a deploy-key push of workflow files as unverified.
  • Nits: checkout is pinned to d23441a4, which is the actions/checkout v6.1.0 tag commit (checked with gh api). parseRawDiff now throws on unreadable output. Keeping baseHasCheck from base.sha is justified, because a PR into a non-default base must be judged against that base.
  • State question:
    • A rerun is deterministic: no cache, blobs are read by SHA.
    • Empty diff: "0 changed files checked", exit 0.
    • A branch from before the check landed: the check files are in neither tree's diff, so nothing is reported.
    • A guarded file absent at both base and head (both deleted) counts as equal to the base, which is correct.
    • Main force-pushed onto a new upstream release: the old branch gets "rebase it onto main" (the real-tree trace from r2 reproduces).
    • A main patch whose author or subject was rewritten during the rebase: falls back to the plain check-file messages. This is stated in "Not proven yet".
    • Half-run: the job only reads, so a stop part-way changes nothing.
    • One silent fallback remains, below.
      finding: major .github/scripts/fork-schema-check.cjs:144-147 with :156-161. When a package-root import names something declaringFiles cannot find, resolveImport returns [], so that import edge leaves the closure and nothing says so. The lens rule makes a silent fallback a major. These declaration shapes are not recognised, each a synthetic tree where migration 001 does import { S } from "@t3tools/c" and index.ts re-exports:
    • export async function S
    • a local export { type S } (the local-export test at :160 does not strip "type")
    • export { default as S } from "./a"
    • export const { S } = o
      Each gives "(NOTHING followed)": the declaring file is not guarded, and the check passes edits to it. Nothing is affected today (every real root import resolves). It becomes live when an upstream release adds a migration that imports one of these shapes, and nobody would see it. Fix:
    • when found is empty for a name, return [file] (the barrel, fail closed) or print a reason that turns the check red;
    • optionally also recognise async function, export { type X } and default as;
    • add a test with an unresolvable name. (high)
      finding: minor .github/scripts/fork-schema-check.cjs:144-147 and the PR body's "re-pointing an export is caught too". The barrel itself is not guarded, so a PR that changes only packages/contracts/src/index.ts can re-point a name migration 064 decodes to an unchanged file. Scratch: append export { ProviderKind as ThreadLinkedPullRequest } from "./orchestration.ts"; (an explicit export shadows the export *). Result: "No database schema changes (1 changed files checked)", exit 0. The head-tree closure does reach orchestration.ts, but that file is unchanged, and the barrel is not in either closure. Re-pointing to a new or changed file is caught. This is a deliberate shape, hence minor. Fix: compare, per schema-root import, the set of declaring files resolved in the merge-base tree and in the head tree, and refuse when they differ. Or reword the body claim. (high)
      optional nits:
  • :219-221: on a stale branch, a real edit to the check (B above) is reported only as "differs because ... rebase it onto main". After the rebase the true "changes the schema check itself" reason appears, so it ends right, but the first message hides the real problem. Saying "differs from main's copy (also rebase onto main)" would cover both.
  • :327-334: a branch behind a main that only moved forward, which cherry-picks one main commit (B2), is called "based on an old main that has since been rebased". The advice (rebase) is still right, but the wording is wrong. "contains commits already on main" covers both cases.
  • apps/web/src/components/files/binaryPreview.worker.ts holds "CREATE VIRTUAL TABLE" (a web worker, not the server DB), so any edit to it is refused. The old rule did the same. Worth one line under "Not caught / caught on purpose" in the body.

…traced names

A name imported from a package root that no file is seen to declare now
guards the whole barrel and everything it re-exports, instead of dropping
out of the guarded set. The tracer also knows export async function, a local
export { type X }, export { default as X } from, and destructured exports.
The barrels on the way to a declaring file are guarded too, so a barrel-only
edit cannot re-point a name a migration imports.

On a branch holding commits already on main, a real edit to a check file is
reported as one, and the message no longer assumes main was rebased.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 3: head ca47142 (was a7328e4)

  • major, untraced package-root name: it now fails closed (the barrel and everything it re-exports are guarded). The tracer also knows export async function, a local export { type X }, export { default as X } from and export const { X } = o. Tests: "the closure traces the declaration shapes a name can have", "a name the closure cannot trace guards the whole barrel ...", "e2e: an edit to a declaring file the closure cannot trace is refused (fail closed)".
  • minor, barrel-only re-point: the barrels on the way to a declaring file are guarded too (their other re-exports are not followed). Test: "e2e: a barrel-only edit that re-points a name a migration imports is refused". On the real tree the exact ProviderKind as ThreadLinkedPullRequest line: a7328e4 exits 0, this head exits 1. Closure +1 file (contracts/src/index.ts). The PR body claim is reworded.
  • nits: a stale branch that really edits the check gets "changes the schema check itself ... (also rebase onto main)" (unit + e2e). The stale message is now "already on main (it is based on an old main, or it copies main's commits)" (cherry-pick e2e). binaryPreview.worker.ts is listed in the body under "Caught on purpose".
  • Suite: 32/32 pass; against the a7328e4 script, 10 fail. fork: CI check that refuses database schema and migration changes #3 vs main and fork: follow each stable upstream release with the patch series #4 (62216fc) vs this head: exit 0.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+security r4 · reviewer: opus high (t3lens-3-r4) · read at: ca47142
covered: the whole fix diff a7328e4..ca47142 (fork-schema-check.cjs: schemaClosure, resolveImport, declaringFiles, the new declaresName, staleCommits/fromOldMain, findSchemaChanges messages; the test file), plus what it calls into (importedNames, packageDirectories, readTree, blobAt, main) and fork-checks.yml (unchanged).
checks:

  • Tests: 32/32 pass at ca47142. The same tests run against a7328e4's script: 10 fail (every new or changed test, including the e2e barrel re-point, the untraced-name test and both stale-branch tests).
  • Broke each fix by hand at ca47142: (1) an untraced name skipped silently again -> 2 tests fail; (2) barrels left out of the closure -> 3 fail (including the e2e barrel re-point); (3) fromOldMain ignored -> 2 fail; (4) the async shape dropped from declaresName -> the shapes test fails. All four fixes are pinned by tests.
  • Round-3 MAJOR is fixed at the root: if declaringFiles cannot trace a name, resolveImport returns the whole barrel, which is then walked and follows every re-export. An unresolvable export {X} from "./missing" and a cycle that ends untraced also fall to the whole barrel (fail closed). The tracer's false positives (for example, a key in export const { S: renamed } = o counts as S) only add files to the closure, and a found file is walked whole, so they over-guard rather than open a hole.
  • Round-3 MINOR is fixed: barrels on the way to a declaring file are added to the closure but not walked further.
  • Nits fixed: the stale message now covers cherry-picks; on a stale branch, a real check edit is still reported as one, plus "(also rebase onto main)". Every guarded-file path still pushes a reason, so a stale branch never passes.
  • Real tree: the closure has 114 files: migrations, the 4 roots, messageSearchIndex, the packages/contracts index.ts barrel, the declaring files in contracts and shared, and nothing like settings.ts. No untraced name forced a whole-barrel walk. Looks right.
  • pull_request_target: the workflow is unchanged. It checks out the default branch with persist-credentials false and only fetches the PR SHAs as objects; the SHAs reach run: lines only through env and are quoted. No PR code runs.
  • State question: a rerun is deterministic and read-only. Empty diff -> passes with "0 changed files", correct. Deleted barrel -> refused (the merge-base closure is unioned in). Renamed declaring file (--no-renames gives a delete plus an add) -> all three paths refused. A PR on an old main or a cherry-pick copy -> refused with a rebase message. Old state: a manifest whose package entry cannot be resolved -> see the finding below.
    finding: major .github/scripts/fork-schema-check.cjs:146 (also :156, and the closure never holds a package.json). Two related holes. (a) The closure guards the files a package import resolves to, but not the package.json that maps the import to them, so an edit to the manifest alone can re-point a name a migration imports. (b) If a workspace package's import cannot be resolved (no exports, or an entry with none of import/default/types), resolveImport returns nothing silently, the same fail-open that round 3 fixed for names. Reproduced in a scratch repo with a migration that does import { S } from "@t3tools/c": (1) the PR changes only packages/c/package.json exports["."].import from ./src/index.ts to the existing ./src/settings.ts, which also exports S -> "No database schema changes", exit 0. (2) The PR sets exports["."] to {"node":"./src/evil.ts","import":"./src/index.ts"} and adds evil.ts. The tracer reads import, but Node picks node first -> exit 0. (3) The PR removes exports, adds "main":"./src/evil.ts" and adds evil.ts -> exit 0. Suggested root fix: add the package.json of every workspace package that resolveImport consults to the closure; the merge-base union then refuses (1)-(3). When a workspace package (pkg found) has no resolvable entry, fail closed (refuse, or walk the whole package) instead of returning none; only packages outside the workspace should resolve to nothing. (confidence: high, reproduced)
    finding: minor .github/scripts/fork-schema-check.test.cjs:496 The e2e "an edit to a declaring file the closure cannot trace is refused (fail closed)" test never takes the fail-closed path: declaresName counts the key S in export const { S: renamed } = o as a declaration, so a.ts is traced. With the fail-closed return removed, the test still passes; only the unit test pins that path. Give a.ts a shape that really cannot be traced (for example only exports.S = 1 plus export default o) so the e2e test goes red without the fix. (confidence: high, checked by breaking fix 1)

…resolve

- The package.json of every workspace package that schema code imports is
  guarded, so an edit that re-points such an import (another export target,
  a condition Node picks first, exports removed in favour of main) is
  refused. Every file any export condition can pick is guarded, and main,
  module and types are followed when a package has no exports.
- An import that should resolve inside the repository but does not is
  refused, naming the import: a workspace package with no entry file, a
  relative import of a missing file, a #name import, a computed import()
  or require(), import.meta.glob. Only packages outside the workspace
  resolve to nothing; the output lists them.
- Workspace packages come from the pnpm workspace list (scoped or not, at
  any depth), and a change to that list is refused. A test-path file that
  schema code imports is guarded like any other.
- The tracer no longer counts the key in `export const { S: renamed }` as
  declaring S.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 4: dee218a (read at ca47142 by the r4 lens)

  • MAJOR package.json not guarded / unresolvable package skipped -> the package.json of every workspace package schema code imports is guarded; every file any export condition can pick is guarded (main/module/types without exports); an in-repo import that resolves to no file is refused, naming it; outside packages are listed in the output. Tests: e2e for the three tricks (all three exit 0 on ca47142) and the unresolvable package, plus unit tests. Real tree: the three tricks on contracts exit 0 with ca47142, exit 1 now.
  • minor e2e untraced test never reached the path -> the tracer no longer counts { S: renamed } as declaring S; the e2e test now goes red with the round-3 fail-closed return removed.
  • Same fail-open pattern elsewhere, fixed: missing relative imports, unscoped and deeper workspace packages, non-source import targets, non-JSON manifests, #name, computed import()/require()/import.meta.glob, test-path files schema code imports, workspace list changes. Listed in the PR body.
  • 41/41 pass; 13 fail against ca47142. Guarded set 114 -> 116. This PR vs main and fork: follow each stable upstream release with the patch series #4 (62216fc) vs dee218a: exit 0.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+security r5 · reviewer: opus high (t3lens-3-r5) · read at: dee218a
covered: fix diff ca47142..dee218a in full (fork-schema-check.cjs: traceSchema, workspacePatterns/workspacePackages, resolveImport, packageTargets, destructuresName, readTree, main; the test file), plus the code it calls (declaringFiles, resolveFile, findSchemaChanges) and fork-checks.yml. Real tree at the PR head (pnpm-workspace.yaml, apps/server/package.json, packages/contracts|shared/package.json, apps/server/vite.config.ts, the schema roots).
checks:

  • Red->green: the dee218a tests on the ca47142 script give 15 fails. 2 of them are only my scratch layout (the workflow tests need ../workflows). The other 13 match the PR body's list. 41/41 pass at dee218a.
  • Each fix broken by hand at dee218a turns tests red: manifest not guarded (8), unresolved package silent (1), relative import silent (1), computed import not seen (1), broken package.json silent (1), workspace list unchecked (1), no main fallback (1), untraced name fail-open (4), ./* patterns ignored (1), arrays ignored (1), #name as outside (1), test-path targets skipped (1). Not covered by any test: dropping manifest.browser from the fallback list (0 fail). It does no harm on this tree.
  • Real tree: this PR against main exits 0. The closure is 116 files: server src 98, contracts src 11 + package.json, shared src 5 + package.json. Nothing is unresolved, and the outside list is effect, node:fs, node:sqlite. That looks right: shared/nodeSqliteClient is reached only from migration tests, so it is correctly outside the closure.
  • Round-4 items are fixed at the root. An exports swap, an added condition and a main fallback are all refused. An unresolvable workspace package, a relative import of a missing file, a computed import and a #name import now fail closed. { S: renamed } reaches the fail-closed path.
  • State question:
    • Deleted barrel (contracts/src/index.ts): refused twice, as unresolved and as a closure edit.
    • Renamed closure file: --no-renames turns it into a delete + add, and the base closure refuses the delete.
    • Empty diff: exit 0.
    • Old main: the union of merge-base and head trees plus stale-commit detection still holds.
    • Rerun or edited event: the result is the same (it is a pure function of the 3 SHAs).
    • Existing state: unresolved imports come from both whole trees, not from the diff. See the minor finding.
  • pull_request_target: the workflow is unchanged in this round. SHAs come in through env and are quoted, nothing from the PR is checked out or run, and permissions are contents: read. The new code compiles PR-controlled workspace globs into RegExp; ? is not escaped, so at worst a crafted list runs slowly (the 5-minute timeout makes that red, which fails closed). A changed list is refused anyway.
    finding: major .github/scripts/fork-schema-check.cjs:293 Only the imported package's manifest is guarded, not the importer's side of the mapping. Probes on the real tree, each with base = pr3 and a probe commit as head, all exit 0:
    (A) apps/server/package.json "@t3tools/contracts": "workspace:*" -> "link:../../x/evil", plus x/evil/{package.json,src/index.ts};
    (B) the same re-point through pnpm-workspace.yaml overrides: "@t3tools/contracts": "link:./x/evil";
    (C) catalog effect: 4.1.2 -> link:./x/effect (effect/sql/SqlClient is imported by 113 migration files);
    (D) "@t3tools/contracts": "npm:left-pad@1.3.0".
    Each one re-points a name a migration imports (064 -> @t3tools/contracts) with a one-line manifest edit. That is the same class as the round-4 major, and it contradicts the PR body's "re-pointing an export is caught". A narrow root fix: for every name the closure imports (workspace and outside), refuse a change to its dependency spec in the manifests of the packages that hold closure files (apps/server, packages/contracts, packages/shared). Also refuse a change to that name's entry under pnpm-workspace.yaml catalog/overrides. That is a few keys, not whole files, so dependency bumps of other names stay free. If the owner prefers to count this as a deliberate trick instead, then at least list the importer spec and overrides of in-repo packages under Not caught, and drop the claim. (repro: high; severity: medium, since this is a trick, not a likely mistake)
    finding: minor .github/scripts/fork-schema-check.cjs:647 Unresolved imports are reported from the whole merge-base and head trees. Each reason tells the PR author to "Fix the import" in a file the PR may not touch. COMPUTED_IMPORT (line 57) also matches prose: a comment // each step must require (in order) ... in Migrations.ts is reported as a computed import (unit-checked). So one upstream release that adds such a comment, an import.meta.glob or a #name import turns every fork PR red. It would also turn the follow-upstream run (fork: follow each stable upstream release with the patch series #4) red, until the owner edits the check, which can itself only land by merging past red. Fail-closed is right, but: (1) say "already on the base; the check needs teaching, not your PR" when the base trace has the same reason; (2) strip comments before matching COMPUTED_IMPORT and IMPORT_SPECIFIER. (high)
    finding: minor .github/scripts/fork-schema-check.test.cjs No test covers the browser string fallback (removing it leaves 41/41 green). (high)
    Listed 'not caught' items, judged on this tree:
  • catalog/overrides/link: re-pointing of outside deps: real today (probe C, effect). It is fixed by the same change as the major, so it is no longer a separate item.
  • Lockfile: a deliberate trick; leaving it in Not caught is fine.
  • tsconfig paths: none today. The same channel already exists as a bundler alias in apps/server/vite.config.ts (vp pack), which the check does not see either. Add it to Not caught; it is not a major.
  • Runtime-read files: none in the persistence code (the only new URL is URL parsing). Not a hole today.
  • Browser object map: the server runs on Node, so it is irrelevant. Not a hole.
    Workability (not blocking): refusing every edit to packages/shared/package.json is costly. It has about 408 subpath exports, upstream edits it in ordinary work (the last 5 commits that touch it are all feat/fix commits), so any fork feature that adds a shared module goes red. Comparing only the fields that map the reached entries (name, the reached exports keys, main/module/types, imports) would keep the guard and free new subpaths. packages/contracts/package.json (3 exports) is cheap to keep whole.

…only new unfollowable imports

- The dependency specs of every name schema code imports (workspace packages and outside names
  such as effect) are compared in the package.json files of the packages holding schema code and
  the root one, plus pnpm-workspace.yaml catalog/catalogs/overrides/patchedDependencies and the
  root pnpm.overrides/patchedDependencies/resolutions. Patch files of those names are guarded.
  Other dependency changes stay free.
- A package.json is judged by the fields that map the imports schema code makes through it
  (name, type, main/module/types/browser, imports, the export entries answering those keys), so
  adding a module to a package stays free.
- An import the check cannot follow is refused only when the branch brings it; ones already on
  the merge-base are listed, not refused. Imports are read from code with comments and string
  contents blanked, so prose like "require (in order)" is not an import.
- Tests: the browser fallback, the four importer-side probes and more, manifest field judging,
  the lexer and the merge-base rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 5 (head 5c2688e), against the r5 lens:

  • MAJOR importer side → the dependency spec of each name schema code imports (@t3tools/contracts, @t3tools/shared, effect) is compared in the dependency fields of the root and the packages holding schema code (apps/server, packages/contracts, packages/shared), in pnpm-workspace.yaml catalog(s)/overrides/patchedDependencies, and in the root's pnpm.overrides/pnpm.patchedDependencies/resolutions; the patch files of those names are guarded too. Other specs stay free. Tests: e2e importer probes, YAML specs, root overrides, holders, patch. Real tree: the probes link: (A), YAML override (B), catalog effect (C), npm: alias (D) and root pnpm.overrides (E) exit 1 now (dee218a exits 0); unrelated bumps (H) exit 0. Deviation: only the holders' manifests and the root are checked, not every workspace package.json (a package with no schema code can't change what schema code resolves to). It is in Not caught.
  • Unresolved / computed imports → refused only when this branch brings them; ones already on the merge-base are listed, not refused. Code is lexed first, so comments and strings are blank (require (in order) in a comment doesn't match), and an unterminated comment or template is reported. Tests: unit + e2e merge-base rule, lexer. Real tree: an upstream comment plus import.meta.glob, then a README-only PR: dee218a exits 1, this exits 0.
  • browser string fallback → test added.
  • package.json field mapping → both packages: only name/type/main/module/types/browser/imports and the export entries schema code imports through are compared. A new shared export passes (F; dee218a refused it), and re-pointing ./assistantCitations is refused (G).
  • PR body → the vite.config.ts alias, tsconfig paths, lockfile, .npmrc/hoisting, packageExtensions/dependenciesMeta, YAML anchors and a regex after ) are under Not caught. The "any package.json edit refused" trade-off is gone.

Tests: 47/47 pass; against the dee218a script, 14 fail; 12 hand-breaks of the fixes each turn tests red. Guarded set: 114 files. This PR vs main, and #4 (fc08057) vs this head: both exit 0, about 1.3 s. Lint wasn't run locally (the oxlint plugin didn't load), so CI will show it.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+security r6 · reviewer: opus high (t3lens-3-r6) · read at: 5c2688e
covered: the whole fix diff dee218a..5c2688e (.github/scripts/fork-schema-check.cjs +502/-86, its test +274), plus the code it calls into (traceSchema, workspacePackages, resolveImport, packageTargets, findSchemaChanges, main) and fork-checks.yml, which this diff does not change. Checked against the real tree (merge-base 24e7747, main ee69d98), .pnpmfile.cjs and pnpm-workspace.yaml.
checks:

  • tests: 47/47 pass on 5c2688e (node 24.21). Script from dee218a with the new tests: 14 fail, so the new tests are red to green (some fail only because compareTrees/lex are not exported yet).
  • each fix broken by hand, at least one test goes red every time: no dependency specs (1), holders limited to the root (1), no pnpm-workspace.yaml catalog/overrides (1), no root pnpm.overrides (1), patch files not guarded (1), override key not split at ">" (1), merge-base findings not exempt (2), whole package.json compared instead of the mapping fields (1), no lexing (1).
  • real tree, local commits on the merge-base (the head's script): empty diff -> green; deleted apps/server/package.json -> red (effect, contracts and shared specs -> (none)); catalog effect 4.1.2->4.1.3 -> red; catalog unified bump -> green; apps/web effect -> npm:effect@4.1.3 -> green; apps/server effect -> npm: alias -> red; root pnpm.overrides effect -> red; pnpm-workspace.yaml overrides "foo>effect" -> red; new contracts export "./zzz" -> green; Migrations.ts renamed -> red (guarded file, plus a new unfollowable import); main..main -> green, nothing pre-existing listed; origin/main..head -> green.
  • narrower spec scope: pnpm resolves a name from the importer's own package.json plus the catalog and overrides, and all of those are guarded for the packages that hold schema code and for the root. A spec change in apps/web or another package does not change what the server resolves. With effect pinned exactly in the catalog, a lockfile-only drift of effect needs a spec change first, and that is caught. The holes left (lockfile, aliases, hoisting, outside code) need deliberate config work, not a normal mistake.
  • can a merge-base finding hide a new one? Tested: the merge-base already holds import(x) in databaseMigrationHead.ts. A PR that adds a second computed import to the same file is still red, because the file is guarded. The same shape in another guarded file is red with its own reason. A PR that does not touch the file is green and lists the old finding. Every unresolved reason comes from a file in the guarded closure, and any edit to one is refused, so a known finding cannot turn a new one green. Only the wording can mislead (minor 2).
  • state question: the run is stateless and reruns give the same result. A half-pushed branch is judged by the head SHA it has. Empty diff, deleted package.json, renamed file and a PR on an older merge-base (24e7747, 2 behind main) were all tested above. Legacy state on the merge-base is listed, never blamed on the PR. An unreadable file on the base is listed and documented under "Not caught". No silent skip found: non-JSON or non-object manifests and non-object dependency fields are compared whole.
  • pull_request_target: unchanged by this diff. SHAs are passed through env, not interpolated into run: lines; persist-credentials is false; the tests and the script come from the default branch; PR commits are only fetched as blobs and read with git cat-file and diff --raw (no filters, no checkout, no PR code run). OK.
  • lint: oxlint 1.89.0 (the lockfile's version) on both files, with the repo's categories (correctness, suspicious, perf as warn; no-array-sort, consistent-function-scoping and no-shadow off, as in vite.config.ts): 0 errors, 2 perf warnings (prefer-set-has at fork-schema-check.cjs:659, no-map-spread in the test at :91). The t3code jsPlugin was not loaded. node --check passes on both files.
  • why no checks on this head: fork-checks.yml is pull_request_target, so GitHub runs the default branch's copy, and main does not have it yet. It starts running after the merge, and this PR cannot run it on itself. ci.yml and fork-tests.yml are disabled_manually on this repo, and the other workflows skip on path filters. This is expected; open a throwaway PR right after the merge to see the check run once.
    finding: minor .github/scripts/fork-schema-check.cjs:615 (PR body "Not caught") the repo has a .pnpmfile.cjs, and its hooks (readPackage, updateConfig returning overrides or catalogs) can re-point effect or @t3tools/* without touching any guarded spec -> the check stays green. Doing that takes deliberate work, but the file is not named under "Not caught" next to .npmrc and should be (high)
    finding: minor .github/scripts/fork-schema-check.cjs:950 the merge-base already has a computed import in guarded file X, and the PR adds another one to X -> the reason key is file+"import(..." and the two get merged, so the output lists the new import under "already on the merge-base... the branch needs no change". The run is still red because X is guarded, so this is only misleading wording (high)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 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