From 54be6aaf0f9dd3f02e6f365df2fc67c2b0b128c9 Mon Sep 17 00:00:00 2001 From: Raj D <25481060+radroid@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:47:10 -0400 Subject: [PATCH] fix(t3x): apply the sync test-timeout to every gate, not just fork CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to a393ff0e4, from an independent review of it. Three real defects, two of them regressions that commit introduced. 1. The timeout override never reached the pipeline that needs it most. a393ff0e4 fixed only t3x-ci.yml. The daily sync runs its own verify via scripts/t3x/sync-upstream.sh (`$RUN $script`, plain `vp run test`), and the resolver prompt tells the agent to run plain `vp run test` too. So the next upstream delta would rebase cleanly, fail verify on the exact test that was just diagnosed, and escalate as `verify-failed` — reopening issue #29's class of failure through a different door. Threaded through all three. 2. `--testTimeout=60000` silently HALVED apps/server's budget. apps/server/vite.config.ts pins testTimeout: 120_000, with a comment saying its sqlite+git+orchestration tests "can exceed the default budget on loaded CI hosts". The flag applies to every package, so 60s cut the heaviest suite in half — the same failure class the override exists to prevent. Now 120000, the highest value any package configures, so it lowers nothing: web 15s -> 120s, root 60s -> 120s, server 120s -> 120s. The old comment claiming it "only stops apps/web's override" was simply false. 3. Both SEAMS.md churn commands always returned 0. Git's approxidate silently ignores a relative suffix on an absolute date, so `--since=" -60 days"` means `--since=""` — an empty window. The previous wrong-but-working command was replaced with two non-working ones, and since the doc says "if the ledger and this document disagree, the ledger is right", a literal regeneration would have zeroed every churn and risk figure. Now uses `--since="@"` arithmetic, with the trap documented and a self-check value (ChatView should return ~59, not 0). Also corrects SEAMS.md against the tree a393ff0e4 actually created: merge-base b64ae880e (not 887dd6e45), totals +1468/-112, claude.md row +78/-31. That commit edited a seam file without updating its own row, so a new rule says to update the row and totals in the same commit — the ledger measures the tree a commit creates, not the one it started from. Finally, records in the runbook that an upstream sync is the sanctioned exception to AGENTS.md's "do not run repo-wide checks" — a rebase can break any package, so the full suite is the point. Every file changed here is fork-owned, so the ledger totals above stay accurate. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/t3x-ci.yml | 10 +++++++--- .github/workflows/t3x-sync-resolve.yml | 8 ++++++-- docs/t3x/SEAMS.md | 22 +++++++++++++++------- docs/t3x/sync-agent-runbook.md | 10 ++++++++-- scripts/t3x/sync-upstream.sh | 11 +++++++++-- 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/.github/workflows/t3x-ci.yml b/.github/workflows/t3x-ci.yml index f704ed3498f..1cd7745d46f 100644 --- a/.github/workflows/t3x-ci.yml +++ b/.github/workflows/t3x-ci.yml @@ -55,9 +55,13 @@ jobs: # Proven upstream-inherited, not a fork regression: branch `t3x/ci-control-upstream` — pristine # upstream/main plus this workflow file and nothing else — fails identically (run 30386125253). # - # 60s is not arbitrary: it is what the ROOT vite.config.ts already uses. This only stops - # apps/web's tighter local override from being applied on slower hardware than it assumes. + # 120000 is deliberate: it is the HIGHEST value any package configures + # (apps/server/vite.config.ts pins 120s for sqlite+git+orchestration integration tests). The + # flag applies to every package, so a lower value would silently CUT apps/server's budget in + # half — the same failure class this override exists to prevent. At 120s no package is + # lowered: web 15s -> 120s, root 60s -> 120s, server 120s -> 120s. # The job's own `timeout-minutes` remains the backstop against a genuinely hung test. + # Keep in sync with the `test` case in `scripts/t3x/sync-upstream.sh`. - name: Test if: ${{ !cancelled() }} - run: vp run test --testTimeout=60000 + run: vp run test --testTimeout=120000 diff --git a/.github/workflows/t3x-sync-resolve.yml b/.github/workflows/t3x-sync-resolve.yml index 0fbc9e4310d..39bcd0d6715 100644 --- a/.github/workflows/t3x-sync-resolve.yml +++ b/.github/workflows/t3x-sync-resolve.yml @@ -114,8 +114,12 @@ jobs: where they did NOT textually conflict, and adjust the fork's patches if upstream changed the semantics of an API the fork hooks into. If a fork patch became empty (upstream absorbed it), drop it and note that. - 4. Verify: run `vp run typecheck`, `vp run lint`, then `vp run test`. Fix the fork's - patches until all three pass. + 4. Verify: run `vp run typecheck`, `vp run lint`, then + `vp run test --testTimeout=120000`. The --testTimeout flag is required: apps/web + pins 15s, which is too tight for this fork's 2-core runners, and 120s matches the + fork's CI gate without lowering any package's own budget. Note the flag must NOT + come after a `--` separator or it is silently ignored. Fix the fork's patches until + all three pass. 5. Push the branch: `git push -u origin HEAD`. 6. Open a PR into `main` with `gh pr create`, title "t3x: resolve upstream sync (run ${{ github.run_id }})". In the body summarise: the upstream range merged, diff --git a/docs/t3x/SEAMS.md b/docs/t3x/SEAMS.md index 81e6c7a161f..fd162f2062f 100644 --- a/docs/t3x/SEAMS.md +++ b/docs/t3x/SEAMS.md @@ -2,8 +2,8 @@ **The authoritative list of every upstream-owned file this fork edits.** -Measured, not asserted: **34 upstream-owned files, +1466 / -112 lines**, against merge-base -`887dd6e45` (the 2026-07-28 upstream sync). Everything else the fork adds lives in new files upstream +Measured, not asserted: **34 upstream-owned files, +1468 / -112 lines**, against merge-base +`b64ae880e` (the 2026-07-28 upstream sync). Everything else the fork adds lives in new files upstream has never seen and cannot conflict. The churn and risk columns are still measured against the _previous_ merge-base `89c5a192f`, because @@ -21,6 +21,11 @@ recurring rebase conflict in a file this doc said the fork did not touch) went u > > **Tripwire:** the surface is already far past "a handful of rows". Before adding row 35, re-isolate > something instead. Prefer fork-owned files even when an in-place edit is smaller. +> +> **Self-reference:** if your change edits a file that already has a row here, update that row and +> the header totals **in the same commit**. This ledger measures the tree the commit creates, not the +> tree it started from — a commit that edits `docs/providers/claude.md` and leaves the row alone +> makes this document wrong the moment it lands. That has already happened once. ## Reading the risk column @@ -67,7 +72,7 @@ Sorted by risk, worst first. | `apps/mobile/src/native/T3ComposerEditor.native.tsx` | +3 | 1 | **3** | Plumbs `onComposerSubmit` | | `apps/mobile/src/components/AppSymbol.tsx` | +2 | 1 | **2** | `return:` icon entry | | `apps/mobile/…/T3ComposerEditorModule.kt` | +1 | 1 | **1** | Event-name list entry | -| `docs/providers/claude.md` | +76/-31 | ~0 | **~0** | Fixes the broken multi-account recipe. Near-frozen upstream (last touched 2026-04-29) — **the one row worth upstreaming**, which would remove it | +| `docs/providers/claude.md` | +78/-31 | ~0 | **~0** | Fixes the broken multi-account recipe. Near-frozen upstream (last touched 2026-04-29) — **the one row worth upstreaming**, which would remove it | **Per surface:** `apps/web` 8 · `apps/mobile` 7 · `apps/desktop` 7 · `apps/server` 5 · `packages/**` 5 · `docs/` 1 · repo root 1. @@ -123,12 +128,15 @@ That prints exactly the upstream-owned files the fork edits. Churn for any one o the merge-base date, **not** to today, so the number is reproducible after the fact: ```bash -SINCE=$(git show -s --format=%cI "$MB") -git log --oneline --since="$SINCE" --until="$SINCE" --before="$SINCE" "$MB" -- | wc -l -# simpler and equivalent: the 60 days of upstream history ending at the merge-base -git log --oneline "$MB" --since="$(git show -s --format=%cI "$MB") -60 days" -- | wc -l +MBTS=$(git show -s --format=%ct "$MB") # merge-base commit time, epoch seconds +git log --oneline --since="@$((MBTS - 60 * 86400))" "$MB" -- | wc -l ``` +The `@` form is load-bearing. Git's approxidate parser **silently ignores** a relative suffix +on an absolute date, so `--since=" -60 days"` is treated as `--since=""` — an +empty window that returns 0 for every file. Verify the command is working before trusting it: +`apps/web/src/components/ChatView.tsx` should return a number in the 50s, not 0. + **Re-run both after every upstream sync and update this file — including the merge-base hash in the header.** Every churn and risk figure shifts once the newly absorbed commits fall inside the window, so a ledger quoting an old merge-base is stale even when its file list is still right. If the diff --git a/docs/t3x/sync-agent-runbook.md b/docs/t3x/sync-agent-runbook.md index 361751f3a69..27f3a603477 100644 --- a/docs/t3x/sync-agent-runbook.md +++ b/docs/t3x/sync-agent-runbook.md @@ -106,8 +106,14 @@ This is the checklist the workflow prompt mirrors — follow it if you resolve l still behaves. 4. If a patch commit went **empty/dropped**, upstream absorbed it — confirm the behaviour now exists upstream, drop the patch, and note it. -5. Verify: `vp run typecheck && vp run lint && vp run test`. Fix the fork's patches to match - upstream's new internals until green. +5. Verify: `vp run typecheck && vp run lint && vp run test --testTimeout=120000`. Fix the fork's + patches to match upstream's new internals until green. + + Two notes. **`AGENTS.md` says "do not run repo-wide checks" — an upstream sync is the sanctioned + exception.** A rebase can break any package, so the full suite is the point; that rule is about + routine feature work. And the `--testTimeout` flag is required (see the section above); it must + not follow a `--` separator or it is silently ignored. + 6. **When green:** push the branch and open a PR into `main` (`gh pr create`). A human reviews and **lands it — see [Landing a sync PR](#landing-a-sync-pr-do-not-use-the-github-merge-button); the GitHub merge button does not work on a rebased branch.** The recovery tag diff --git a/scripts/t3x/sync-upstream.sh b/scripts/t3x/sync-upstream.sh index 87c4acfd347..79c885f377e 100755 --- a/scripts/t3x/sync-upstream.sh +++ b/scripts/t3x/sync-upstream.sh @@ -128,10 +128,17 @@ if [[ "$SKIP_VERIFY" == 1 ]]; then exit 0 fi +# Per-script extra args, so the daily verify matches the fork's CI gate exactly. `test` needs +# --testTimeout because apps/web pins 15s (tuned for upstream's blacksmith runners) and this fork +# runs on 2-core ubuntu-latest, where an upstream CPU-bound test reliably blows it. 120s is the +# highest any package configures (apps/server), so it never LOWERS a package's own budget. +# Keep in sync with `.github/workflows/t3x-ci.yml`. for script in $VERIFY; do - echo "→ verify: $RUN $script" + EXTRA="" + if [[ "$script" == "test" ]]; then EXTRA="--testTimeout=120000"; fi + echo "→ verify: $RUN $script $EXTRA" # shellcheck disable=SC2086 - if ! $RUN "$script"; then + if ! $RUN "$script" $EXTRA; then FAILING_STEP="$script" fail "verification step '$script' failed" write_status daily verify-failed "rebase clean but '$script' failed"