Upstream sync 2026-07-27: absorb 34 commits through v0.0.29 - #26
Conversation
…otgg#4556) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tgg#4397) Co-authored-by: codex <codex@users.noreply.github.com>
…queue (pingdotgg#4453) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Prepare Relay production infrastructure for the PlanetScale PS_20 HA topology, upgrade Effect and Alchemy to compatible betas, reconcile migration state, and preserve patched MCP session termination behavior.\n\nCo-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…ts down (pingdotgg#4531) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…tgg#4670) Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # apps/web/src/components/ProjectScriptsControl.tsx # apps/web/src/components/chat/ChatHeader.tsx
Upstream's prompt stash (pingdotgg#4453) and preview picture-in-picture (pingdotgg#4397) introduced two lucide icons the shim did not export; the phosphor-duotone guard caught both. Bookmark maps to BookmarkSimple and PictureInPicture2 to PictureInPicture, both at the default duotone weight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Thermo-nuclear code quality review
Request changes. This sync lands real features, but several landings fail the maintainability bar: unjustified growth of already-huge modules, two first-party files crossing 1k lines, duplicated client heuristics instead of a projection contract, and an incomplete stash extraction that left ~400 lines of orchestration inside ChatComposer.
Fork conflict resolutions (ChatHeader, ProjectScriptsControl) and the Phosphor shim mappings look clean. The structural debt is in the upstream feature landings being absorbed as-is.
(Inline comments skipped — GitHub rejects reviews on this PR because the diff exceeds the 300-file limit. Anchors below are HEAD line numbers.)
1. Structural regression — PiP/frame-capture in a 3.6k-line god module
apps/desktop/src/preview/Manager.ts — 3043 → 3679; Manager.test.ts +771.
frameCaptureSessionsRef / pictureInPictureSessionsRef (~L485–498), withTabLifecycleLock (~L529), and openPictureInPicture (~L2264) are bolted into makeNativeOperations.
Extract PiP + frame-capture (+ session maps / locks / aspect-ratio state) into dedicated modules. Keep Manager as thin orchestration. Growing this file further makes every preview change unreadable. Once extracted, own tab-generation / closingTabIds / semaphore checks as one lifecycle policy — not ad-hoc predicates in every create/close/register path.
2. Missed code-judo — stash orchestration left in ChatComposer
apps/web/src/components/chat/ChatComposer.tsx — 2905 → 3353.
Store/menu/badge/compression were extracted, then ~400 lines of restore/delete/stash/shortcut/pulse orchestration stayed in the composer (~L1936–2126+).
Pull into useComposerPromptStash (or similar) so ChatComposer only wires the hook. Incomplete extraction leaves every future composer change entangled with stash.
3. Boundary / duplicated magic — WS payload projection
apps/server/src/orchestration/ActivityPayloadProjection.ts ~L176–181.
collectChangedFiles is effectively copied from apps/web/src/session-logic.ts. The comment admits both clients still walk path-like keys. Don't encode the client heuristic twice — project an explicit contract field (typed files / summary shape) and teach web/mobile to prefer it so the dual unknown-walkers can die.
4. 1k crossing — scripts/dev-runner.test.ts
749 → 1184. Extracting scripts/lib/dev-share.ts was the right implementation move; the test surface didn't follow. Split port/single-origin policy tests (isBrowserAllowedPort ~L622) from --share integration cases; lean on dev-share.test.ts.
5. 1k crossing — apps/server/src/cloud/http.ts
963 → 1034 via releaseManagedTunnelOnShutdown (~L634). Coherent feature, wrong home. Move release (+ CLI-desired/managed gating) into a small adjacent module next to link reconcile — don't let cloud HTTP keep absorbing every environment lifecycle concern.
Not blocking (positive)
- Prompt stash store + compression + menu/badge are well-bounded
- Web mini-player split (
ThreadPreviewMiniPlayer, store, layout helpers) is fine - Tailscale
--shareextraction todev-share.tsis good judo - PR-branch scoping (
resolveBranchToolbarPrBranch) simplifies rather than adds - Fork presentation keeps on conflict files are intentional and clean
Approval bar: not met — clear structural regressions, unjustified file-size crossings, duplicated heuristic projection, and an obvious decomposition left on the table.
Sent by Cursor Automation: Thermo-nuclear PR review
Review — upstream sync 2026-07-27Static review of The merge mechanics are clean. The problem is on the ChatHeader side, and it is the specific thing this fork's whole safety model rests on: after this PR, the customization it touched is less guarded than before it. 1. The workspace-header guard now passes on the wrong element
expect(chatHeader).toMatch(/font-semibold text-muted-foreground/u);Before this PR, that matched the project name ( Concretely: revert the project name to upstream's Fix: anchor the assertion to the name, not to a loose class-pair — e.g. match 2. Nothing asserts the behaviour that was deliberately adoptedThis is the only file in the merge where upstream's new lines landed inside a fork fence — I checked all six fenced components:
That is the right call for a hunk the fork owns presentationally — but it has a consequence the PR doesn't close.
So dropping 3. Dead class in the fork's own hunk
Inherited from upstream — not blockers, but you now own themRaw NUL bytes in Zero lines, exit 0. That file carries 17 fenced hunks — the densest in the fork.
Nit, upstream's: Verified clean
Fix 1 and 2 and this is good to land; 3 is a token. Generated by Claude Code |
Follow-up: CI Test is red, and it isn't a fork guardClosing the loop on the one thing my review couldn't check — run 30323510546: Every fork guard passed. The failure is in an upstream file that arrived with the prompt stash (pingdotgg#4453), so the description's focused-run results were accurate as far as they went — this test simply wasn't in the focused set. It is not flake. The stub returns a constant 8 MB for every encode, so nothing ever fits the budget and the give-up path runs to completion:
The underlying inefficiency is real and upstream's: The fork-specific part: Cheapest unblock is a Generated by Claude Code |
Review fixes for the 2026-07-27 sync PR: - Anchor the semibold assertion to the project name itself; the loose class-pair match was satisfied by the '/' separator alone, so reverting the name to upstream's font-medium stayed green. Assert the muted colour separately on the breadcrumb trigger where it now lives. - Guard the adopted pingdotgg#4638 behaviour: the new-thread click and tooltip sit inside the fork fence, so a future sync will not re-apply them from upstream; without an assertion, dropping them is silent. Record the same contract in the fork-workspace-header manifest intent, which is what the sync routine reads. - Drop the dead gap-1.5 on the breadcrumb trigger (spaced the favicon the fork removed). - Raise the give-up-path timeout in stashImageCompression.test.ts to 60s, fenced under ci-runners: 15 encodes of an 8 MB stub through chunked base64 is deterministic work at the margin of 15s on GitHub-hosted runners, which are slower than the Blacksmith runners upstream tunes against. Test-only on purpose — the byte-size short-circuit is upstream's fix to make, and diverging in hot upstream logic costs a conflict on every sync. Losing the hunk turns CI red by itself, which is the enforcement; the manifest entry records the policy. - Document the ChatComposer.tsx raw-NUL grep hazard in .fork/AGENTS.md: plain grep classifies the fork's densest fenced file as binary and reports zero matches with exit 0; fence audits must use grep -a or rg. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes landed — 5d58ee9Also merged @NoahHendrickson's review1. Guard anchored to the right element — the semibold assertion now matches 2. Adopted behaviour is now asserted and manifested — new guard case asserts 3. Dead CI red — decided, not re-run: took the fenced per-test timeout (60s) on the give-up-path case, not the NUL-byte hazard — adopted. New "Auditing fences with grep" section in
Verification: 156/156 guards green (includes #27's new relay guard), Cursor "thermo-nuclear" reviewDeclining the structural asks (Manager.ts extraction, Generated by Claude Code |
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review — sync merge resolution + fork rework
Scope: the fork-authored substance of this sync (the two conflict resolutions, the icon-shim and guard-hardening commits, and the manifest/docs changes), not the 34 upstream commits. 8 finder angles, one verification pass per surviving candidate — most verified mechanically (mutating the merged source and re-running the guard assertions, recomputing the base64 budget arithmetic, checking vitest 4.1.9's type overloads).
The core merge is sound. All 146 lucide identifiers imported across the merged aliased graph resolve in the Phosphor shim (including the two new ones, with the right export names for upstream's bindings); onNewThreadInProject is declared and wired from ChatView; the actionsMenuOpen controlled-menu merge reproduces upstream pingdotgg#4660 faithfully including close-on-edit; ProjectFavicon is fully gone (no unused import); ChatComposer's fences survived the stash feature's no-conflict merge intact; the release-upstream-only gate still wraps every release job; and the vendored subtrees were synced with the effect beta.78→beta.102 bump.
The findings below are all about the durability of the fork machinery, not broken behavior. Ranked by severity.
1. The guard hardening dropped the only tripwire for the separator restyle — apps/web/src/__fork_guards__/forkWorkspaceHeader.test.ts:93 [CONFIRMED, test-coverage]
The old /font-semibold text-muted-foreground/u assertion's only match in the merged file was the / separator span's class list — so it doubled as the tripwire for the fork's separator restyle (upstream has text-muted-foreground/40). The two replacement assertions anchor to the project name and the breadcrumb trigger; neither touches the separator.
Demonstrated end-to-end: swapping the merged separator for upstream's <span aria-hidden className="text-muted-foreground/40">/</span> leaves all six new guard assertions green, while the removed assertion would have failed. A future sync that keeps upstream's separator merges with CI green and the fork presentation silently reverts — the exact silent-loss class this commit says it was written to eliminate. Fix: add a separator-anchored assertion (e.g. toContain('font-semibold text-muted-foreground">') scoped to the separator, or not.toContain("text-muted-foreground/40")).
2. The ci-runners timeout fence has no guard asserting it — .fork/customizations.yaml:60 [CONFIRMED, conventions]
.fork/AGENTS.md ("Every customization requires") mandates a guard test in __fork_guards__/ "so an upstream sync that silently drops the customization turns CI red instead of passing quietly." ciRunners.test.ts only regex-checks runner labels in ci.yml; nothing reads the newly watched stashImageCompression.test.ts (detect-drift is advisory-only; customizationsManifest.test.ts checks the converse direction — a wholesale upstream replacement passes all four of its assertions).
The manifest's substitute rationale — "losing one turns CI red by itself" — is undercut by this PR's own commit message calling the test "at the margin of 15s": a sync that takes upstream's version can pass on a fast run, and the loss resurfaces later as apparent upstream flake instead of a flagged fork regression. One assertion in ciRunners.test.ts (fence marker or timeout: 60_000 present in the watched file) closes it.
3. Second data-fork-pill carrier is unfenced and unguarded — apps/web/src/components/ProjectScriptsControl.tsx:433 [CONFIRMED, conventions]
The file's only fence (337–342) wraps the Group-branch carrier; the importable-scripts MenuTrigger's data-fork-pill at line 433 sits outside any fence (.fork/AGENTS.md rule 4: inline edits must be fenced), and the pill guard is a single toContain("data-fork-pill") per file — satisfied by the other carrier. A future sync that rewrites the imports-branch MenuTrigger drops its pill with CI green, and the "Add action"-only state renders as an upstream button.
The unfenced placement pre-exists on custom, but this hunk was one of the two declared conflicts and the resolution hand-rewrote exactly these lines — this was the moment to fence it or strengthen the guard.
4. The fence swallows upstream's pingdotgg#4638 behavior lines — apps/web/src/components/chat/ChatHeader.tsx:91 [CONFIRMED, altitude]
The resolution placed upstream's entire new-thread breadcrumb — button element, aria-label, onClick, Tooltip/TooltipPopup, ~50 lines arrived verbatim from upstream — inside fork:begin fork-workspace-header. The sync machinery now treats upstream-owned behavior as fork-owned; the manifest concedes the cost ("port upstream's intent by hand when that hunk drifts") and the new guard pins today's snapshot of upstream's text, not currency with upstream. When upstream iterates on the breadcrumb, the fork silently ships the frozen copy with CI green.
The deeper mechanism already exists three files over in this same customization: data-fork-pill + theme.custom.css. Every fork delta here except the favicon removal (font weights, gaps, separator color) is pure CSS — a data-fork attribute on the trigger plus Tier-1 rules would leave upstream's behavior lines byte-identical and auto-merging, shrinking the fence to the one-line favicon removal. (CSS-hiding the favicon is not equivalent — ProjectFavicon would still mount and fetch via useAssetUrl — so that one line legitimately stays a code edit.)
5. Muted-colour guard regex false-fails on a prop reorder — forkWorkspaceHeader.test.ts:97 [PLAUSIBLE, test-coverage]
Measured: the trigger's text-muted-foreground sits 142 chars after the aria-label anchor; sorting the JSX props so className precedes aria-label moves it to 414 — past the [\s\S]{0,400} window — so a formatting-only edit turns the guard red pointing at nothing. (The false-green path via the separator is numerically out of reach: 619 chars, only ~156 strippable.) Strictly stronger and window-free: toContain("text-muted-foreground transition-colors hover:text-foreground") — occurs exactly once (only the trigger has hover:text-foreground) and fails precisely when the muted colour is lost.
6. The 60s timeout raise is unnecessary — apps/web/src/lib/stashImageCompression.test.ts:128 [CONFIRMED, efficiency]
The give-up path's 15 encodes are fixed by constants (3 dimension passes × [1 WebP probe + 4 quality steps]), not stub size. A ~1.1 MB stub (stubCanvasPipeline(() => 1_100_000) + makeFile(1_100_000)) encodes to ~1,466,691 data-URL chars — over the 1,300,000 budget on every attempt — driving the identical 15 encodes, the same {ok:false, reason:"too-large"}, and the same finally-block close(), at ~16.5 MB of base64 work instead of ~120 MB, comfortably inside the default 15s. Both fixes live inside the same fenced it() block, so the sync-conflict footprint is identical — but the timeout raise burns 10–15s of CI every run and, if the loop ever regresses into a hang, delays failure reporting to 60s.
7. The timeout rewrite re-indents the whole test body — stashImageCompression.test.ts:127 [CONFIRMED, simplification]
The options-object signature re-indented every body line (the -w diff confirms indentation is the only body change), so any upstream edit inside this test now produces a whole-hunk conflict on every future sync — contradicting the commit's own minimize-divergence rationale. @vitest/runner@4.1.9 still declares the trailing-number overload (name, fn?, options?: number) first-class, no @deprecated tag: appending , 60_000 before the closing paren is the same timeout as a 1-line delta.
8. Dead classes on the new-thread button — ChatHeader.tsx:115 [CONFIRMED, simplification]
min-w-0 is inert (the parent span is shrink-0, no width pressure ever reaches the button) and items-center renders identically with the single text child. Both came from upstream's two-child (favicon + text) button — and the same commit already pruned gap-1.5 on exactly the removed-favicon reasoning, so the remaining two are an oversight, not a choice. Fork-fenced lines every future resolution re-reasons about; two fewer tokens to carry.
9. Fence comment duplicates the manifest prose — ChatHeader.tsx:103 [CONFIRMED, simplification]
The fence comment's three paragraphs (weight inversion, favicon removal, pingdotgg#4638 adoption) are near-verbatim copies of the manifest's fork-workspace-header intent — against the manifest's own "narrative there, contract here" division, and against house style elsewhere (main.tsx, SidebarV2.tsx, ChatComposer.tsx use a bare pointer or short local note). Two independently edited copies will drift; the manifest is what the sync routine reads. Shrink the in-file comment to a one-liner pointing at the anchor the fence header already cites. (The ci-runners fence comment in the stash test was checked for the same issue and is fine — it carries local detail the manifest doesn't.)
Also checked and clean: guard regexes match the merged source character-for-character and fail under regression simulation (except as noted above); it(name, {timeout}, fn) is valid under vitest 4.1.9; the new watch: entry satisfies the manifest parser and fence-claim invariant; the ChatComposer NUL-byte claim in .fork/AGENTS.md is factually true (6 raw NULs); --share exists in the merged dev-runner; the phosphor guard's completeness mechanism genuinely scans all lucide import forms.
🤖 Generated with Claude Code


Summary
Merges
upstream/main(476d69c, through v0.0.29) intocustom— 34 upstream commits. Merged fromupstream/maindirectly rather thanorigin/mainbecause the mirror is 13 commits stale (knownfork-sync-mirrortoken-permission failure on upstream'srelease.ymlchange).What upstream changed (highlights)
release-upstream-onlygate untouched and verifiedConflicts resolved (2)
Both were fork
fork-workspace-headerstyling colliding with new upstream behavior; resolved per manifest intent — behavior adopted, fork presentation kept:ChatHeader.tsx— upstream made the project breadcrumb a "new thread" button with favicon + tooltip. Kept the click behavior, tooltip, and a11y label; kept fork presentation (no favicon, semibold project name, semibold/). Fence comment updated to record the adoption. Upstream'sProjectFaviconimport stays out.ProjectScriptsControl.tsx— upstream made the imports menu controlled (close-on-edit, fix: close actions dropdown when editing pingdotgg/t3code#4660). Took upstream'sopen/onOpenChangeand kept the fork'sdata-fork-pillon the trigger.Customization rework
phosphor-duotone-icons— guard caught two unmapped lucide icons from the new features:BookmarkIcon(stash) →BookmarkSimple,PictureInPicture2(PiP) →PictureInPicture, both duotone.Verification
ChatHeader.test.ts(4 tests) passing; webtsgo --noEmitclean;.fork/lint-owned.mjsclean (46 files, no warnings)🤖 Generated with Claude Code