@@ -359,7 +359,7 @@ zero across these commits; several were pre-existing bugs the old markup had bee
 | `c3261d338` | **The post-merge routine, and the check that would have saved the worst hour of it.** Yesterday's merge of 151 upstream commits found four defects and cost roughly two hours on two failures that were **not** the merge — and there was no written routine for any of it. There is now, in three places. **`docs/onboarding.md` gets the ordered eight steps**, in the order that finds things: syntax-check what you touched (a conflict region rarely aligns with a block boundary, and a leftover `end` reads as a merge that went fine); regenerate the lockfile and **check its platform list**; migrate and let Rails rewrite the schema, then prepare the *test* database; rebuild Tailwind **before** any class audit; restart the server; run `undefined-classes.py`; run the new shadow check; then the suite and whatever `which-audits.rb` names. It also records the rule that made 45 conflicts tractable — **design owns the markup, main owns the behaviour** — with `git diff -w` per file, because one file looked like a 19-line conflict and was pure reindentation while another looked identical and carried a whole feature. And the trap that bit twice: **stage each file the moment you resolve it**, because `git checkout --ours` on a still-unmerged path silently discards edits already made to it. **`bin/design/route-shadow.rb` is the new audit.** Rails serves `public/` before the router, so a file there answers a route and the controller never runs — with a 200 and plausible content. That is how untracked rollback debris returned three unfiltered rows for a CSV export while the controller, view model, concern, model, factory and spec were all byte-identical to pre-merge. It reports whether each shadow is **tracked**, because that decides the fix: untracked is debris and can be deleted, tracked is somebody's decision and the answer is a name — which is why every import template here ends in `_template.csv`. Three controls: a file named after a real route fires and is marked untracked, a file matching no route stays silent, and the real templates stay silent. It prints the candidate count either way, so "nothing shadows a route" cannot be read as "nothing was looked at". **The selector picked it up with no edit** — 30 audits considered became 31, and the runner was inferred as `bin/rails runner` from the file mentioning `Rails.`, which is the ratchet working as intended. **A correction went in first**: an hour after the merge I had written that `undefined-classes.py` *cannot* catch a dead class in a Stimulus controller. It can — planting `d-none` back into `reminder_toggle_controller` names the class and the file. I had written it from reading the code instead of running it. Generalised into `design-system-migration` as `reference/merging-upstream.md` (103 lines): which side owns markup and which owns behaviour, what to do with upstream's specs, and the two failures that look like merge damage and are not. |
 | `fe03e1254` | **The migration was finished and the check could not tell: 347 of 384 was a measurement error, and the six views it named were all migrated.** Asked what remained, I read the files `status.rb` reported — **all six were done, and the check was wrong six times out of six**. Three were bare `f.input`, which is the *finished* state rather than an unmigrated one, because `config.default_wrapper = :essentials` makes the wrapper supply the div, the label, the control classes and the error — each of those three files carries a comment saying the hand-pasted classes were *removed* for that reason. The others carried a data-table cell class (`class="percent"`), a Stimulus controller (`data-controller="highchart"`), and a helper call whose argument was a variable rather than the symbol literal the pattern wanted. **Widening it was the wrong instinct, and I did it three times before noticing.** The page wrapper, `link-brand`, `EssentialsUiHelper` and the cell conventions took 347 → 360; resolving "renders a migrated partial", which the check's own comment already claimed it did, took it to 364; simple_form builder calls took it to 346 of 349 — and the last three files would have needed a fourth pass. The comment in the file records that the pattern had **already** been widened once, for 51 pages, for the same reason. **That is a check that cannot be finished**: the design system gains vocabulary with every component, so a positive-marker test is permanently one behind, and every miss reports completed work as outstanding. **So the measure is inverted.** The legacy vocabulary is closed — ADR 0011 deleted both frameworks and they cannot gain words — so the question is now *does this view still use the old system?*, which is what "is the migration done" means, and it reads **0 of 350**. The positive count survives as a labelled indicator, because a four-line table row can be migrated and have nothing to mark. Files that cannot carry a marker for the system they *are* — the 34 components in `shared/essentials/` plus gem and framework partials — are named separately rather than counted as failures, because a silent exclusion reads like an oversight. **The first draft of the inverted check was wrong too**, and the controls caught it: it flagged `admin/dashboard`, `partners/_show_header` and `reports/index`, all three of which carry `card-surface` — ours — because `\bcard\b` matches the "card" inside it. `float-right` came out as well, being a Tailwind utility as much as a Bootstrap one. **Thirteen controls**, one per legacy group, plus those two which must stay silent; without them the inversion would have traded six false negatives for three false positives. `state.rb` reads the new line and says `n/a` rather than 0 when the parse fails, proven against clean, 4-legacy and crashed output. Both status tables corrected — the change log's generated one and `migration-map.md`'s hand-maintained copy, whose own paragraph had guessed "the 37 are not a backlog" correctly and never checked. |
 | `b4e217f4b` | **A stored XSS, found by running the CI gate nobody had run.** The branch is 600 commits ahead of `main` with no pull request, so I checked which workflows one would trigger: six, of which the design suite covers four. **I had never run `brakeman` or `factory-bot-lint`.** Brakeman failed — `LinkToHref`, weak confidence, on `organizations/_details.html.erb` — and it was real. `Organization#url` validated with `URI::DEFAULT_PARSER.make_regexp` and **no scheme argument**, which accepts any scheme, so `javascript:alert(document.cookie)` was a valid organization URL — and that field is rendered with `link_to`. An organization admin sets it; anyone who views the page and clicks runs script in their own session. Weak confidence describes the static analysis, not the exposure. **The more interesting half is that restricting the scheme is not enough.** `BroadcastAnnouncement#link` *already* restricted it to `http https` and was still bypassable, because `format:` is unanchored and matches a substring: `"javascript:alert(1) http://decoy.example.com"` satisfied it. That field is the "More info" link on **every user's dashboard** — the widest exposure of the three, and it had looked fixed. Found by testing the fix rather than trusting the pattern already there. **`HttpUrlValidatable`** is anchored and `http https` only, on all three fields that take a URL from a user (`Organization#url`, `BroadcastAnnouncement#link`, `AccountRequest#organization_website`), and **`essentials_external_link` / `essentials_safe_href`** guard the four render sites. Two layers deliberately: the validation guards one write path, and a row can arrive by CSV import, from the console, or from a backup predating it. The helper is split because the dashboard link carries its own classes and accessible name and needs the href checked rather than the anchor built for it. A dangerous URL renders as **plain text rather than nothing** — dropping it hides the problem from the only people who can fix it. **Checked before changing anything**: 0 rows across all three fields held a non-http(s) value, so nothing was invalidated; had there been any, the data migration would have had to come first. 43 examples, and **21 of 36 watched failing** against the old pattern. Brakeman 1 warning → **0, exit 0**; `factory_bot:lint` exit 0. The lesson is not about URLs: four of six gates were covered, the two that were not are where this was hiding, and it only surfaced because **`main` bumped brakeman 8.0.5 → 8.0.6** in the merge and a new version brings new checks. The post-merge routine has a step 9 now, naming both. |
-| `TBD` | **A staged plan for landing 602 commits, and executing it corrected it twice.** `CONTRIBUTING.md` asks for PRs "limited to one particular issue"; this branch is **1,051 files, +89,606/−24,093**, and one pull request of that size gets approved on trust or left to rot. `docs/review-plan.md` splits it into stages that each leave `main` working: foundation, audit suite, fifteen per-area PRs, retire the old stack, docs. **The order is forced by one fact** — `main` has 20 Sass files and Propshaft compiles no Sass, so the pipeline switch cannot precede the view migration. The branch's own history proves the order works: Tailwind entered at commit **2 of 602** alongside Sprockets, Sass left at **22**, Propshaft arrived at **114**. Composition measured rather than guessed: **41% of added lines are documentation**, and **221 of 602 commits are "Fill in the change log hash"** bookkeeping, leaving 376 substantive — so stages are reviewed as diffs, not commit sequences. **Then I began, and stage 1 disproved itself.** It was to be the ADRs alone — reasoning first, arguable before any code. Built against `main` it failed a link check: **ADR 0010 links to `design.md`**, which arrives with the foundation, and dropping 0010 broke a second link because **0010 and 0011 reference each other**. The ADRs and the spec are one unit; they are stage 1 now, and ADR 0012 travels with the Propshaft change it describes. **The second correction is worse and quieter**: the obvious way to assemble a stage is `git checkout design -- <paths>`, and the tip's `Gemfile` has Propshaft and **no** Sass, Bootstrap or Sprockets — stage 4's removals baked in. Taking it would land stage 4 inside stage 1 and unstyle every unconverted screen. The state each stage needs is the one the branch was in when that stage finished, and it exists: `cda053539` carries `bootstrap`, `sass-rails`, `sprockets` **and** `tailwindcss-rails` together with all twenty `.scss` files. Both findings are in the plan, and the per-stage checklist gained two steps ahead of the tests: every relative `.md` link must resolve against that stage's own files, and no later stage's removals may be present — check `Gemfile` and `app/assets/`. **Stage 0 is not the migration at all**: the stored XSS is live on `main` today, same validations and the same `link_to` in two views, so it should go first and alone — but there is no `SECURITY.md` and no disclosure channel, and a public PR announcing it to an app used by 200+ non-profits is a decision for the maintainers, not for me. |
+| `b666920d4` | **A staged plan for landing 602 commits, and executing it corrected it twice.** `CONTRIBUTING.md` asks for PRs "limited to one particular issue"; this branch is **1,051 files, +89,606/−24,093**, and one pull request of that size gets approved on trust or left to rot. `docs/review-plan.md` splits it into stages that each leave `main` working: foundation, audit suite, fifteen per-area PRs, retire the old stack, docs. **The order is forced by one fact** — `main` has 20 Sass files and Propshaft compiles no Sass, so the pipeline switch cannot precede the view migration. The branch's own history proves the order works: Tailwind entered at commit **2 of 602** alongside Sprockets, Sass left at **22**, Propshaft arrived at **114**. Composition measured rather than guessed: **41% of added lines are documentation**, and **221 of 602 commits are "Fill in the change log hash"** bookkeeping, leaving 376 substantive — so stages are reviewed as diffs, not commit sequences. **Then I began, and stage 1 disproved itself.** It was to be the ADRs alone — reasoning first, arguable before any code. Built against `main` it failed a link check: **ADR 0010 links to `design.md`**, which arrives with the foundation, and dropping 0010 broke a second link because **0010 and 0011 reference each other**. The ADRs and the spec are one unit; they are stage 1 now, and ADR 0012 travels with the Propshaft change it describes. **The second correction is worse and quieter**: the obvious way to assemble a stage is `git checkout design -- <paths>`, and the tip's `Gemfile` has Propshaft and **no** Sass, Bootstrap or Sprockets — stage 4's removals baked in. Taking it would land stage 4 inside stage 1 and unstyle every unconverted screen. The state each stage needs is the one the branch was in when that stage finished, and it exists: `cda053539` carries `bootstrap`, `sass-rails`, `sprockets` **and** `tailwindcss-rails` together with all twenty `.scss` files. Both findings are in the plan, and the per-stage checklist gained two steps ahead of the tests: every relative `.md` link must resolve against that stage's own files, and no later stage's removals may be present — check `Gemfile` and `app/assets/`. **Stage 0 is not the migration at all**: the stored XSS is live on `main` today, same validations and the same `link_to` in two views, so it should go first and alone — but there is no `SECURITY.md` and no disclosure channel, and a public PR announcing it to an app used by 200+ non-profits is a decision for the maintainers, not for me. |