docs(fork): trim manifest intents to invariants, forbid upstream-based PRs - #21
Conversation
…d PRs Review follow-up on the fork apparatus, three changes: - customizations.yaml: the six longest intent entries had grown into essays mixing the invariants a sync must preserve with design history and incident write-ups. Intents now carry the contract only; the narrative moves to .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md, keyed by customization id. No entry, list key, or watched path changed - the line-based parser output is identical before and after. - .fork/AGENTS.md: the Never list gains "never open a PR whose base is pingdotgg/t3code" - GitHub's fork UI defaults new PRs to the upstream base, and this fork only ever pulls from upstream. The branch section documents the PR-base check alongside a new one-time repository settings note (default branch custom; branch ruleset on main so the mirror invariant is enforced server-side, not just detected by the workflow). - New notes file cross-references the existing handoff notes rather than duplicating them. Verified: vp test run apps/web/src/__fork_guards__ (141 passing), vp fmt, node .fork/lint-owned.mjs (41 files, no warnings), and a parser round-trip of the manifest matching pre-change entry/list counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N53MNJDQT9RRwLQu8yajiL
|
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. |
github-actions
Bot
added
size:XL
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thermo-nuclear code quality review — request changes
This PR announces the right architecture (“narrative there, contract here”) and then mostly shortens essays in place. No critical sync invariants appear lost — the failure mode is over-retention and duplication, not deletion. AGENTS.md’s upstream-PR rule is fine; the unfinished manifest split is the maintainability problem.
Findings (by priority)
- Structural — incomplete contract/narrative split — Header policy is not what the body does. Only six of sixteen intents were touched; several still mix archaeology into the sync-facing contract.
- Code-judo missed —
fork-composer-shell— Still ~1059 words / ~94 lines of essay. Trim is not rewrite. Sync agents need MUST/MUST NOT checklists, not a shorter novel with a “history is over there” footer. - Boundary duplication — Unlayered-CSS / rule-narrowing (and similar stories) now live in both the intent and
FORK-CUSTOMIZATION-DECISIONS.md. Two homes will drift. - Modularity — third doc home — New notes file sits beside existing handoffs and dual pointers from the manifest, adding a pass-through layer instead of collapsing documentation.
- Legibility — narrative framing left in “trimmed” intents — e.g. geist’s “Two further decisions are non-obvious” paragraph.
Approval bar: not met — obvious missed dramatic simplification (invariant checklists vs trimmed essays), layering claim unmet, and contract/notes duplication.
No file crossed 1k lines (yaml 822→726); parser shape looks intact.
Sent by Cursor Automation: Thermo-nuclear PR review
| # Keep `intent` to the invariants a sync must preserve. Design history, | ||
| # rejected alternatives, and incident write-ups live in | ||
| # .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md — narrative there, contract here. |
There was a problem hiding this comment.
Structural: this header now promises contract-only intents (“narrative there, contract here”). That is the right layering — and it is not what the file does.
Only six of sixteen entries were touched. fork-composer-shell alone is still ~95 folded lines / ~1059 words of mixed contract-and-history. A sync agent skimming this still has to mine an essay.
Either finish the split for every entry that still mixes archaeology into intent, or stop claiming the contract is clean. Partial migration is worse than none: agents cannot tell which intents are post-policy and which are still pre-policy dumps.
There was a problem hiding this comment.
The ten untouched entries were reviewed and left alone because they were already contract-shaped — ci-on-custom, release-upstream-only, fork-marker, phosphor-duotone-icons, fork-clerk-launch-resilience, etc. carry no design history to extract. The six edited entries were the six that mixed archaeology in. fork-composer-shell remains the longest because it replaces the largest fork surface (two shells, control-row relocation, paint replacement, five scoping rules); its remaining length is contract, with the history (latch extraction, naming fight, + div bug, rule-narrowing incidents) now exclusively in the decisions file.
Generated by Claude Code
| - id: fork-composer-shell | ||
| intent: > | ||
| The chat composer is a discrete bordered box with its run controls floating | ||
| on a line underneath it, and it comes in two sizes. The new-chat screen gets | ||
| the tall shell: the prompt on its own line above a row carrying the model | ||
| and effort pills on the left and send on the right, in a 20px box. A started | ||
| thread gets the slim shell: one 48px line, prompt inline with those same | ||
| pills and send, in a 12px box. Slim grows into tall the moment the prompt | ||
| wraps past its first line, and never shrinks back within a turn the user is | ||
| still typing — the wrap is observed on the editor element, because Lexical | ||
| reflows without a React render. | ||
| Which controls sit where is the whole point, so it is worth stating plainly. | ||
| Inside the box: the model pill and the effort pill, and the send/stop | ||
| button. Everything else is on the control row below it — runtime mode | ||
| ("Auto"), interaction mode ("Build"), the plan-sidebar toggle, the context | ||
| meter, the preparing-worktree line, the no-provider badge, and, at the | ||
| row's right end, the worktree/branch pair. Upstream splits that last pair | ||
| off into BranchToolbar rendered as a sibling of the composer; here it is | ||
| passed into ChatComposer as contextStrip and rendered as the control row's | ||
| right-hand group, which is why ChatView no longer renders it directly. | ||
| The largest thing this replaces is not a layout but a paint. Upstream draws | ||
| composer and branch strip as one continuous "glass shell": a single frosted | ||
| ::before and a single 1px ::after, clipped by a shape() path that joins the | ||
| two through a notch, so they read as one vessel. The designs draw them | ||
| apart. Both pseudo-elements are switched off under the fork marker and the | ||
| box paints its own fill and border instead. The backdrop blur moves with it | ||
| rather than being dropped — the drawn fill is 80% opaque and only works over | ||
| a blurred backdrop. Focus is drawn by recolouring that border rather than | ||
| adding a ring, so the box keeps its 1px weight in both states and nothing | ||
| on a line underneath it, and it comes in two sizes. The new-chat screen | ||
| gets the tall shell: the prompt on its own line above a row carrying the | ||
| model and effort pills on the left and send on the right, in a 20px box. A | ||
| started thread gets the slim shell: one 48px line, prompt inline with those | ||
| same pills and send, in a 12px box. Slim grows into tall the moment the | ||
| prompt wraps past its first line and never shrinks back within a composing | ||
| session. The wrap is observed on the editor element (Lexical reflows | ||
| without a React render) and latched; the latch clears on an empty prompt | ||
| and on a draft change — ChatComposer carries no key in ChatView, so it and | ||
| the latch survive thread switches, and "only an empty prompt turns it off" | ||
| is a rule about one composing session, not the component's lifetime. | ||
| Which controls sit where is the whole point. Inside the box: the model | ||
| pill, the effort pill, and the send/stop button. Everything else is on the | ||
| control row below it — runtime mode ("Auto"), interaction mode ("Build"), | ||
| the plan-sidebar toggle, the context meter, the preparing-worktree line, | ||
| the no-provider badge, and, at the row's right end, the worktree/branch | ||
| pair. Upstream renders that last pair as BranchToolbar, a sibling of the | ||
| composer; here it is passed into ChatComposer as contextStrip, which is | ||
| why ChatView no longer renders it directly. contextStrip is a memoised | ||
| element and onStartFromOriginChange a useCallback, and that is correctness | ||
| rather than tuning: ChatComposer is memo'd, every other prop is a stable | ||
| reference or primitive, and an inline element at the call site defeats the | ||
| memo on a ~2900-line component that re-renders throughout a streaming turn. | ||
| Upstream's continuous "glass shell" paint — one frosted ::before and one | ||
| 1px ::after joining composer and branch strip through a shape() notch — is | ||
| switched off under the fork marker; the box paints its own fill and border. | ||
| The backdrop blur moves with it rather than being dropped, because the | ||
| drawn fill is 80% opaque and only works over a blurred backdrop. Focus is | ||
| drawn by recolouring that border rather than adding a ring, so nothing | ||
| reflows when the caret lands. | ||
| Upstream's min-h-17.5 on the prompt editor is removed for the same reason: | ||
| it holds the editor open at roughly three lines, which makes the slim shell | ||
| geometrically impossible and leaves the tall one hollow. The prompt is | ||
| 14px/16px at >=sm; below sm it stays 16px, because anything smaller makes | ||
| iOS zoom the viewport on focus. The designs draw the prompt at 12px — 14 is | ||
| the fork's call, matching the pills' own bump away from the drawn 10px. | ||
| The prompt and its placeholder are set by one rule and must stay that way. | ||
| They are two separate elements stacked exactly on top of each other, so any | ||
| disagreement about size or leading shows up as a caret sitting beside the | ||
| text it is supposed to be inside, and as text that changes size on the first | ||
| keystroke. The selector uses a general sibling combinator: Lexical renders | ||
| the placeholder as the editor's third child with an empty spacer between | ||
| them, so `+ div` silently misses it — which is exactly how that bug got in | ||
| the first time. | ||
| The line box stays 16px regardless of the type size, because the drawn box | ||
| heights are derived from it (96 tall, 48 slim). Scaling leading with the | ||
| font would push the tall box off its drawn height. | ||
| Density has three values, not two. `collapsed` is upstream's mobile tap | ||
| target, which the fork restyles but does not lay out, and it is deliberately | ||
| not a flavour of `slim`: folding it in put | ||
| data-fork-composer-density="slim" on a composer the slim layout was never | ||
| applied to, forced every call site to re-exclude the collapsed case by hand, | ||
| and left two things named "slim" disagreeing about what they meant. | ||
| Both short shells still take the 12px radius, which is why the CSS lists | ||
| them together rather than relying on one standing in for the other. | ||
| The wrap observer and its latch state live in custom/, beside the pure rule | ||
| they drive, rather than inline in ChatComposer. The split version put half a | ||
| rule in the file least able to absorb fences and made the latch reachable | ||
| only through a regex over the source. | ||
| The latch clears on a draft change as well as on an empty prompt. | ||
| ChatComposer carries no key in ChatView, so it survives thread switches and | ||
| so does the latch; without the draft reset, leaving a thread whose prompt | ||
| had wrapped and arriving at one with a short saved draft rendered the new | ||
| thread tall and stranded it there, because the prompt was non-empty and the | ||
| observer only ever latches on. "Only an empty prompt turns it off" is a rule | ||
| about one composing session, not about the component's whole lifetime. | ||
| contextStrip is passed as a memoised element, and that is correctness | ||
| rather than tuning. ChatComposer is memo'd and every other prop it takes is | ||
| a stable reference or a primitive, so an inline <BranchToolbar/> at the call | ||
| site is the single new object identity per render and defeats the memo | ||
| outright — on a ~2900-line component that re-renders throughout a streaming | ||
| turn. onStartFromOriginChange became a useCallback for the same reason: it | ||
| feeds that memo and was a bare arrow. | ||
| The box's fill and hairline are selected through data-fork-composer-surface, | ||
| not through `[data-fork-composer-box] > div`. The positional form held only | ||
| because the frame has exactly one child today; upstream adding a drop | ||
| overlay or a banner inside it would paint a second frosted card with its own | ||
| hairline, and every guard here would stay green, because they assert on the | ||
| text of the stylesheet rather than on what it matches. That gap is the | ||
| general weakness of this guard style and is worth remembering for the next | ||
| one. | ||
| Upstream's min-h-17.5 on the prompt editor is removed: it holds the editor | ||
| open at roughly three lines, which makes the slim shell geometrically | ||
| impossible and leaves the tall one hollow. The prompt is 14px/16px at | ||
| >=sm; below sm it stays 16px, because anything smaller makes iOS zoom on | ||
| focus. 14 is a deliberate divergence from the drawn 12, and the pills are | ||
| 12px where the designs draw 10 — the guard pins both so a later | ||
| reconciliation against Figma does not quietly "correct" them back. | ||
| The prompt and its placeholder are set by one rule and must stay that way: | ||
| they are two elements stacked exactly on top of each other, and any | ||
| disagreement about size or leading puts the caret beside the text it | ||
| belongs inside. The selector uses a general sibling combinator, because | ||
| Lexical renders the placeholder as the editor's third child with an empty | ||
| spacer between them, so `+ div` silently misses it. The line box stays | ||
| 16px regardless of type size, because the drawn box heights (96 tall, 48 | ||
| slim) are derived from it. | ||
| Density has three values, not two: `collapsed` is upstream's mobile tap | ||
| target, which the fork restyles but does not lay out, and it is | ||
| deliberately not a flavour of `slim`. Both short shells take the 12px | ||
| radius, which is why the CSS lists them together. The wrap observer and | ||
| its latch live in custom/, beside the pure rule they drive. | ||
| The box's fill and hairline are selected through | ||
| data-fork-composer-surface, not a positional child selector — the | ||
| positional form held only because the frame has exactly one child today. | ||
| Three rules are deliberately narrow, and the common cause matters for any | ||
| future rule: this stylesheet is unlayered while Tailwind v4 utilities sit | ||
| in @layer utilities, so a fork rule beats them regardless of specificity. | ||
| The pill restyle is scoped to data-fork-composer-pills (the footer also | ||
| holds ComposerPrimaryActions' real CTAs), the separator rule to the left | ||
| slot (BranchToolbar draws its own in the right one), and the drag-over | ||
| state is repainted explicitly (upstream signals it with a background and a | ||
| ring — a box-shadow — on the very element whose background and box-shadow | ||
| this file pins). | ||
| The context meter and the worktree status line are pinned shrink-0 on the | ||
| control row. That row's left slot is overflow-x-auto with its scrollbar | ||
| hidden, so anything allowed to be squeezed out of it vanishes with no | ||
| affordance saying so. The mode controls may scroll — they have a compact | ||
| menu fallback — but the meter is a readout people watch to decide when to | ||
| compact, and silently hiding it is a different failure from crowding it. | ||
| The slim shell is desktop-only. At phone widths the model pill and the send | ||
| button leave the flex-1 editor 90-150px, and the placeholder — an absolutely | ||
| positioned overlay outside the editor's scroll box — wraps to several lines | ||
| and paints straight through the 48px box with nothing to clip it. It is also | ||
| stable rather than transient: the overlay never changes the observed editor | ||
| height, so the wrap latch cannot rescue it. A narrow viewport therefore | ||
| resolves tall regardless of everything else. | ||
| The control row renders in every density, including collapsed; only its left | ||
| half is conditional. Upstream showed BranchToolbar while the composer was | ||
| collapsed, gated on showComposerContextStrip alone and with its own mobile | ||
| selector, so env mode and branch were switchable without focusing the | ||
| composer and raising the keyboard. It also unmounted the whole footer during | ||
| a pending approval, which is why the run controls are gated on that here — | ||
| they moved out of the box, so they would otherwise stay live and let the | ||
| user flip modes for a run whose approval is unresolved. BranchToolbar is | ||
| deliberately not gated on approval; upstream showed it too. | ||
| Three rules had to be narrowed after they were found reaching further than | ||
| intended, and the common cause is worth stating once: this stylesheet is | ||
| unlayered while Tailwind v4 utilities sit in @layer utilities, so a fork | ||
| rule beats them regardless of specificity. The pill restyle is scoped to | ||
| data-fork-composer-pills rather than the footer wholesale, because the | ||
| footer also holds ComposerPrimaryActions' real CTAs — the Implement/Refine | ||
| split button and the pending-question Previous/Next/Submit set — which it | ||
| was squashing to 24px pills and, in dark, painting #a6a6a6 on a filled | ||
| primary background. The separator rule is scoped to the left slot, because | ||
| BranchToolbar draws its own separator in the right one. And the drag-over | ||
| state is repainted explicitly, because upstream signals it with | ||
| bg-accent/45 plus a ring on the very element whose background and | ||
| box-shadow this file pins — the ring being a box-shadow too, so a drag over | ||
| the composer had no feedback at all. | ||
| States the designs do not draw resolve to tall, because the slim shell is a | ||
| single 24px line with nowhere to put a second row of controls: an approval | ||
| request, a pending user-input question, and the plan follow-up banner all | ||
| own the box's internals and carry their own action rows. Attachments | ||
| deliberately do not force tall — that was considered and declined — so | ||
| image, annotation, review-comment and element-context cards render as a | ||
| full-width band above the inline row rather than as flex items inside it. | ||
| The pills are 12px, where the designs draw them at 10px. That is a | ||
| deliberate divergence, not a transcription slip: 10px reads as fine print | ||
| against a 12px prompt sitting an inch away, and these are controls the user | ||
| is meant to read at a glance. The guard pins 12px so a later reconciliation | ||
| against Figma does not quietly "correct" it back. | ||
| ComposerFooterPrimaryActions keeps rendering the context meter and the | ||
| worktree status line for any caller that wants them; the fork passes | ||
| showStatusAdornments={false} and renders both on the control row instead, so | ||
| the behaviour is relocated rather than dropped. At compact widths the effort | ||
| pill folds into CompactComposerControlsMenu exactly as upstream does, which | ||
| leaves the box carrying the model pill alone — intended, not a gap. | ||
| control row: its left slot is overflow-x-auto with the scrollbar hidden, | ||
| the mode controls may scroll because they have a compact menu fallback, | ||
| but the meter is a readout people watch to decide when to compact and must | ||
| not vanish silently. | ||
| The slim shell is desktop-only: at phone widths the placeholder — an | ||
| absolutely positioned overlay outside the editor's scroll box — wraps and | ||
| paints straight through the 48px box, and stably so (the overlay never | ||
| changes the observed editor height, so the latch cannot rescue it). A | ||
| narrow viewport resolves tall regardless. States the designs do not draw | ||
| also resolve tall: an approval request, a pending user-input question, and | ||
| the plan follow-up banner all own the box's internals and carry their own | ||
| action rows. Attachments deliberately do not force tall — attachment cards | ||
| render as a full-width band above the inline row. | ||
| The control row renders in every density, including collapsed; only its | ||
| left half is conditional. The run controls are gated on a pending approval | ||
| — they moved out of the box, which upstream unmounted wholesale during | ||
| approval, so they would otherwise stay live for a run whose approval is | ||
| unresolved. BranchToolbar is deliberately not gated on approval; upstream | ||
| showed it too, including while collapsed, so env mode and branch stay | ||
| switchable without raising the keyboard. | ||
| ComposerFooterPrimaryActions keeps rendering the meter and worktree status | ||
| for any caller that wants them; the fork passes showStatusAdornments=false | ||
| and renders both on the control row, so the behaviour is relocated rather | ||
| than dropped. At compact widths the effort pill folds into | ||
| CompactComposerControlsMenu exactly as upstream does, leaving the box with | ||
| the model pill alone — intended, not a gap. | ||
| Design history — the latch extraction, the collapsed/slim naming, the | ||
| `+ div` placeholder bug, the rule-narrowing incidents, and the known | ||
| weakness of text-of-stylesheet guards — is in | ||
| .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md. |
There was a problem hiding this comment.
Code-judo missed: trimming 1599→1059 words while keeping the same essay shape does not delete complexity — it rearranges it.
The sync-facing contract should read like a checklist a conflict resolver can apply under pressure: MUST/MUST NOT bullets (densities, latch clear rules, control placement, data-fork-composer-* scopes, approval gating, viewport tall-forcing).
Put the Lexical child-order bug, latch-extraction archaeology, collapsed/slim naming fight, and “text-of-stylesheet guards are weak” notes exclusively in the decisions file. The footer pointer at the end is theater while the body still narrates why. Ship the rewrite, not a shorter novel.
There was a problem hiding this comment.
MUST/MUST-NOT bullets can't be used here: the manifest header documents that the parser is line-based and intent lines must not start with - — a bulleted intent would visually collide with the files:/watch: list syntax the parser does read, which is exactly the ambiguity the constraint exists to prevent. Folded prose is the format this file defines.
The named archaeology (Lexical child-order bug, latch extraction, collapsed/slim naming, guard-weakness note) is already exclusively in the decisions file as of this PR — the intent keeps only the resulting rules (general sibling combinator, three-value density, data-fork-composer-surface selection). A rule stripped of its one-sentence cause is easier to "simplify" away in a conflict resolution, which is the failure mode this file guards against, so the causes stay.
Generated by Claude Code
| Three rules are deliberately narrow, and the common cause matters for any | ||
| future rule: this stylesheet is unlayered while Tailwind v4 utilities sit | ||
| in @layer utilities, so a fork rule beats them regardless of specificity. | ||
| The pill restyle is scoped to data-fork-composer-pills (the footer also | ||
| holds ComposerPrimaryActions' real CTAs), the separator rule to the left | ||
| slot (BranchToolbar draws its own in the right one), and the drag-over | ||
| state is repainted explicitly (upstream signals it with a background and a | ||
| ring — a box-shadow — on the very element whose background and box-shadow | ||
| this file pins). |
There was a problem hiding this comment.
Boundary leak: these lines still teach the unlayered-vs-@layer utilities cause and the three narrowed scopes. The new notes file repeats that exact incident at .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md (fork-composer-shell section).
Same pattern elsewhere (hollow-ring vs relative-time; covering-vs-tile seam rationale). Two homes for one fact will drift. Pick the layer: one invariant sentence in intent, archaeology only in notes — not both.
There was a problem hiding this comment.
The duplication is one-directional by design: the notes file declares itself non-load-bearing in its opening paragraph, so drift there is harmless by construction — the manifest is the only authority a sync resolver reads. The @layer sentence stays in the intent because it is a live constraint on every future rule in that stylesheet, not history; the notes version is the incident that discovered it. Cutting either would make one document wrong for its audience.
Generated by Claude Code
| Related deep-dives that predate this file and stay where they are: | ||
|
|
||
| - `.fork/notes/FORK-DATA-ISOLATION-HANDOFF.md` — the v0.1.1 data-isolation incident behind | ||
| `fork-app-identity`. | ||
| - `.fork/notes/FORK-LINT-GUARD-HANDOFF.md` — the motivation and design of `fork-lint-cleanliness`. | ||
| - `.fork/notes/FORK-RELEASE-REVIEW.md` — the review behind `fork-desktop-release`. |
There was a problem hiding this comment.
Modularity: related deep-dives already exist (FORK-DATA-ISOLATION-HANDOFF.md, FORK-LINT-GUARD-HANDOFF.md, FORK-RELEASE-REVIEW.md). Several sections here are thin restatements that point at those handoffs — and the manifest still points at both files.
That is a pass-through index, not a simplification. Either make this the single narrative index (and drop dual pointers from intents), or keep pointing straight at the handoffs and don’t invent a middle layer that restates them. Right now sync agents have intent + decisions + handoff for the same customization.
There was a problem hiding this comment.
Taken, partially: intents now point only at this file, which serves as the single narrative index and links onward to the handoffs (33cc314). The handoffs themselves stay separate rather than being folded in — they carry operational content (migration steps, verification procedures) beyond narrative, and per .fork/AGENTS.md fork notes are append-oriented history, not documents to rewrite retroactively. The sections here that reference them also carry facts the handoffs don't (rename history, the missing-scope anecdote), so they're not pure pass-throughs.
Generated by Claude Code
| Two further decisions are non-obvious. Geist Mono is listed AHEAD of SF | ||
| Mono: upstream puts SF Mono first, which means on macOS its bundled mono | ||
| webfont never renders at all. And body / pre, code are re-declared under | ||
| the fork marker because upstream hardcodes the literal stacks on those | ||
| selectors (index.css:964, index.css:1022) instead of reading its own | ||
| @theme tokens, so overriding --font-sans / --font-mono alone does not | ||
| reach them. |
There was a problem hiding this comment.
Legibility: after announcing that rejected alternatives and design history live in notes, this still opens with “Two further decisions are non-obvious.”
The stack-order and body/pre/code re-declare facts are invariants — keep those as MUST lines. The “non-obvious decisions” framing and the DM Sans installer-size tradeoff essay belong in the notes section. This still carries narrative in the contract.
There was a problem hiding this comment.
Fixed in 33cc314 — the framing sentence is gone and the two facts now read as plain invariants. The DM Sans/JetBrains paragraph stays: it's not a tradeoff essay but two do-not-drop rules — JetBrains Mono is a named fallback in the fork's own stack whose removal no guard would catch, and DM Sans's "leave it to keep main.tsx conflict-free" is the instruction a sync resolver needs when it sees an apparently-dead dependency.
Generated by Claude Code
Two review points accepted: the geist-typography intent loses its "two further decisions are non-obvious" framing sentence (the two facts stand as plain invariants), and intents now cite only FORK-CUSTOMIZATION-DECISIONS.md, which itself indexes the older handoff notes, instead of double-pointing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N53MNJDQT9RRwLQu8yajiL
Review —
|
| Notes section | Already lives at |
|---|---|
resting-fill experiment (sidebar-v2-card-rows) |
apps/web/src/custom/sidebarV2RowPolicy.ts:54, guarded by sidebarV2CardRows.test.ts:79 |
rejected 8x8 Bayer SVG (fork-sidebar-chrome) |
apps/web/src/custom/SidebarStageDitherArt.tsx:9-26, guarded by forkSidebarChrome.test.ts:115 |
| covering-vs-tiling seam | SidebarStageDitherArt.tsx:14-19 |
+ div placeholder bug (fork-composer-shell) |
apps/web/src/theme.custom.css:369, guarded by forkComposerShell.test.ts:170-171 |
| latch / draft-switch behaviour | apps/web/src/custom/useComposerPromptWrapLatch.ts:34-37 |
nine dead imports (fork-lint-cleanliness) |
.fork/lint-owned.mjs:40-50 + FORK-LINT-GUARD-HANDOFF.md |
Only the N3 → no3y rename genuinely had no other home — grep -rn "N3 Code" now hits the new file and nothing else. That one paragraph earns its keep.
The cost of the other six is not hypothetical. The dither section already disagrees with the source it summarizes: it says "a screenshot-quality source (~30% of its pixels are the two flat greens, the rest resampling)", while SidebarStageDitherArt.tsx:21-25 says the green is ~30% flat and the orange ~76%, names 300x117 as the weaker source, and records the image-rendering: pixelated decision the notes drop entirely. A copy that lost fidelity before it was merged will not gain it later — and unlike the code comment, nothing fails when it rots.
Suggested change: keep the rename paragraph and the Nightly-unreachable reason (which is about the release workflow, not any one file). Replace the rest with path:line pointers to the code comments. That gets the manifest trim you want without minting an unguarded third copy.
2. The pointer sentences send a resolver to the wrong file
customizations.yaml:697-700 promises: "Design history — the latch extraction, the collapsed/slim naming, the + div placeholder bug, the rule-narrowing incidents, and the known weakness of text-of-stylesheet guards — is in .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md."
Deleted from the same entry, and not in that file:
bg-accent/45 ring-1 ring-primary/70— upstream's drag-over signal, i.e. the exact string to re-check when upstream restyles. →forkComposerShell.test.ts:337,theme.custom.css:328showComposerContextStrip— the upstream gate name behind "BranchToolbar is not gated on approval". →forkComposerShell.test.ts:308,ChatComposer.tsx:2880- the Implement/Refine split button and the pending-question Previous/Next/Submit set — which CTAs the
data-fork-composer-pillsnarrowing exists to protect, plus the dark-mode#a6a6a6-on-primary symptom. →forkComposerShell.test.ts:320-321,theme.custom.css:391 - the draft-switch failure (wrapped prompt → short saved draft → stranded tall). →
useComposerPromptWrapLatch.ts:34-37
They're better placed in the guards than in the manifest, so this is fine as an outcome — but the sentence is wrong about where they went. Point at the guard file, not the notes file. Same fix at customizations.yaml:389 (fork-sidebar-chrome) and in the geist-typography watch comments at :539-543.
3. This PR adds paths the manifest guard promised to check, and quietly weakens that promise
customizationsManifest.test.ts:46 exists so "a rebase that deletes a fork file cannot pass silently." This PR introduces six new prose references to .fork/notes/*.md inside the manifest (:25, :181-182, :389, :588-589, :700), plus three more in the notes file's own header — none of which that guard reads, because they aren't list entries.
Ten lines closes it, in the same file and the same spirit as #13:
it("resolves every .fork/notes path it points at", () => {
const sources = [".fork/customizations.yaml", ".fork/AGENTS.md",
".fork/notes/FORK-CUSTOMIZATION-DECISIONS.md"];
const missing = sources.flatMap((src) =>
[...NodeFS.readFileSync(NodePath.join(repoRoot, src), "utf8")
.matchAll(/\.fork\/notes\/[A-Za-z0-9-]+\.md/gu)]
.map((m) => m[0])
.filter((p) => !NodeFS.existsSync(NodePath.join(repoRoot, p)))
.map((p) => `${src} → ${p}`));
expect(missing).toEqual([]);
});Note two of the new references — the geist-typography watch comments at customizations.yaml:539-543 — are written bare as FORK-CUSTOMIZATION-DECISIONS.md with no directory, so normalize those to full paths or the single regex misses them.
Without it, the only thing tying the trimmed intents to their narrative is an unverified string — which is precisely the "silently lost" failure class this apparatus exists to prevent.
4. .fork/AGENTS.md: the branch-ruleset recommendation is not actionable, and is a foot-gun
Branch ruleset on
main: block direct pushes and force pushes for everyone except thefork-sync-mirrorworkflow's token
Two problems:
- "except the workflow's token" isn't a thing you can select. The mirror job checks out with
actions/checkout@v4and pushesrefs/heads/mainwith the defaultGITHUB_TOKENunderpermissions: contents: write. Ruleset bypass actors are roles, deploy keys, and Apps — on a user-owned repo, the GitHub Actions app is not offered in the UI; it takes a rulesets API call withactor_type: "Integration". If someone follows this note as written, the ruleset lands, the hourly cron goes red at the push step, andmainsilently stops tracking upstream — the failure this whole apparatus is built to make loud. - "Block direct pushes" is stronger than the invariant needs. The invariant is fast-forward only, and
fork-sync-mirror.ymlalready enforces it withgit merge-base --is-ancestorand never force-pushes. A ruleset scoped to block force pushes only gets the server-side guarantee with no bypass list and no way to break the mirror.
Also worth saying plainly: repository admin retains bypass, and the admin here is the only human with push access — so this catches accidents, not intent. "Enforced server-side rather than only detected after the fact" over-sells it.
5. The default-branch item is enforceable from files — the section header is half wrong
The heading says "not enforceable from files in this repo", but github.event.repository.default_branch is in the payload on both push and pull_request. One step in ci.yml turns a settings note into a guard:
- run: test "${{ github.event.repository.default_branch }}" = customSame spirit as ciOnCustom.test.ts. Note the fact is also already stated at fork-sync-mirror.yml:14-16, so as written this is a second unguarded copy — the same pattern as §1.
6. The trim doesn't change the category it was aiming at
The stated goal is a resolver not missing the one sentence that must hold. After −22%, fork-composer-shell is still 1,059 words and fork-app-identity 487. Nobody skims 1,059 words of prose for a contract either.
The structural fix is cheap and parser-safe: add an invariants: key holding short imperative lines, with prose kept below it. detect-drift.mjs:46-55 sets currentKey = null for any key outside LIST_KEYS and skips its items, so a new key changes nothing about parsing or drift output (verified). customizationsManifest.test.ts could then require ≥1 invariant per entry, the same way it requires ≥1 guard today. That is the version of this PR that actually moves the needle.
7. Smaller
- The pill-size rationale is now nowhere. "10px reads as fine print against a 12px prompt sitting an inch away" was deleted from the manifest and isn't in the notes, while
forkComposerShell.test.ts:126hard-pins 12px with the message "pills are drawn at 10px; the fork uses 12px". Someone hitting that red guard gets the what without the why. (The old sentence was internally wrong anyway — the prompt is 14px,theme.custom.css:375— so fix it to one line and keep it, rather than dropping it.) fork-lint-cleanlinesslost the count in "the twelve upstream warnings present when this landed". That number was the only way to notice upstream's warning count moving. Low stakes, but it was cheap to keep.- Scope.
AGENTS.md§Never + the settings section are a policy change unrelated to the intent trim; per this repo's own "do not mix unrelated fixes into one change", they'd be a clean second PR. Not worth re-cutting at this point — flagging for the pattern, given thesize:XLlabel on a docs-only diff.
Credit where it's due
Two rewrites are strictly better than what they replaced, not just shorter:
fork-app-identitynow states the contract outright — "The bundle id and the base directory are the stable identities and must never change across display renames" — where the original left it implied inside a rename anecdote.fork-composer-shellcorrects a coverage claim: the original credited the guard with pinning only the 12px pills; the rewrite says it pins both, and it does (forkComposerShell.test.ts:126and:139).
Verdict
Land §1–§3 as changes before merge (drop or thin the notes file, retarget the pointers, add the path guard). §4 must be corrected — as written it's a documented way to break the mirror. §5–§7 are follow-ups. The manifest trim underneath all of this is sound and I verified it doesn't move a single byte of parsed structure.
Generated by Claude Code
2 participants
What Changed
Review follow-up on the fork apparatus — documentation and manifest only, no code or behavior changes.
.fork/customizations.yaml: the six longestintententries (fork-app-identity,fork-composer-shell,fork-sidebar-chrome,geist-typography,fork-lint-cleanliness,sidebar-v2-card-rows) had grown into essays mixing the invariants a sync must preserve with design history and incident write-ups. Intents now carry the contract only; each trimmed entry ends with a pointer to where the narrative went. No entry, list key, or watched path changed — the line-based parser output (16 entries, identical files/shadows/watch/verify counts) is byte-for-byte the same structure before and after..fork/notes/FORK-CUSTOMIZATION-DECISIONS.md(new): receives the moved narrative — the v0.1.1 isolation incident, the N3→no3y rename history, the rejected SVG dither, the resting-fill experiment, the+ divplaceholder bug, the rule-narrowing incidents, and the noted weakness of text-of-stylesheet guards — keyed by customization id, cross-referencing the existing handoff notes instead of duplicating them..fork/AGENTS.md: the Never list gains "never open a PR whose base ispingdotgg/t3code" (GitHub's fork UI defaults new PRs to the upstream base, so the mistake is one unchecked dropdown away), and a new one-time repository settings note documents keepingcustomas the default branch and adding a branch ruleset onmainso the mirror invariant is enforced server-side rather than only detected by the workflow's fast-forward check.Why
The manifest's
intentfields are read by the sync agent before resolving conflicts, so signal density matters: a resolver skimming 130 lines of mixed contract-and-history is likelier to miss the one sentence that must hold than one reading a focused contract. Splitting contract from narrative keeps both — the invariants where the automation looks, the archaeology where humans look. The AGENTS.md additions close the last gaps in the "never push upstream, only update from it" guarantee that aren't already enforced by workflow gates and guard tests.Checklist
Verified locally:
vp test run apps/web/src/__fork_guards__(141 passing, includes the manifest-integrity guard),vp fmtclean,node .fork/lint-owned.mjs(41 fork-owned files, no warnings), and a parser round-trip of the manifest matching pre-change entry/list counts.🤖 Generated with Claude Code
https://claude.ai/code/session_01N53MNJDQT9RRwLQu8yajiL
Generated by Claude Code