From 4b8baabf2727d7ce641c8137ed44ceff687de42d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 19:52:09 +0000 Subject: [PATCH] Add a staged merge, test, and deploy plan for the open PRs Reviews the ten open non-dependabot PRs (excluding #457) against current main: verifies that none of them conflict textually, identifies the four semantic collisions that do matter, and sequences the merges around what each one needs in order to be tested on live Word or Google Docs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CCBMDpN2d8w7bnK6NhDEih --- docs/pr-merge-and-deploy-plan.md | 233 +++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 docs/pr-merge-and-deploy-plan.md diff --git a/docs/pr-merge-and-deploy-plan.md b/docs/pr-merge-and-deploy-plan.md new file mode 100644 index 00000000..9e1dcab6 --- /dev/null +++ b/docs/pr-merge-and-deploy-plan.md @@ -0,0 +1,233 @@ +# Open-PR merge, test, and deploy plan + +Written 2026-08-04 against `main` @ `c16d6ba`. Covers the open non-dependabot PRs +except #457 (add-in UX redesign, draft, deliberately parked). + +## The headline + +**Nothing conflicts textually.** Every open branch merges clean into today's +`main`, and every pair of them merges clean with each other: + +``` +for b in ; do git merge-tree --write-tree origin/main origin/$b; done +``` + +reports zero conflicts for all ten branches, and the full pairwise matrix is +clean too. The only shared files are `frontend/CLAUDE.md` (four PRs append to +different sections), `backend/CLAUDE.md`/`docker-compose.yml` (#574 and #594), +and `backend/src/app.ts` + `frontend/src/account/AccountPage.tsx` (#566 and +#594) — all disjoint hunks. + +The collisions that matter are **semantic**, and there are four. They are the +reason for the staging below, not merge order for its own sake. + +## Four things to fix, not just merge + +### 1. Google Docs staging is untestable today — blocks the whole test plan + +`frontend/vite.config.ts:57` compiles the backend origin into the gdocs bundle: + +```ts +'process.env.GDOCS_BACKEND_URL': JSON.stringify(isDev ? '' : urlProd), +``` + +Every non-dev build therefore points at `app.thoughtful-ai.com`. But +`google-docs-addon/sidebar.html:200` offers a **staging** source, and the +add-in image is built once per commit on `main` and deployed to both hosts +(`build-addin-image.yml`; CD pins by SHA). So picking "staging" in the sidebar +loads the staging *bundle* and talks to the **prod backend** — prod auth, prod +consent records, prod study logs. + +That makes it impossible to safely test #566 (consent), #585 (doc-read errors), +or #581 (serialization) on Docs without touching production data. #595 notes +this in passing and correctly leaves it out of scope; it needs its own small PR. + +**Fix:** drop the compile-time constant and let `resolveServerUrl()` derive the +origin from the bundle's own script URL — the fallback it already has at +`frontend/src/api/index.ts:28`. The gdocs bundle is a synchronously-loaded IIFE +and `SERVER_URL` is computed at module top level, so `document.currentScript` is +populated. Keep `urlProd` as the last-resort fallback rather than `/api`. + +This is ~5 lines and it is stage 0. + +### 2. #585 ships the bug its own description names + +The PR body flags it and the code confirms it. The new `requestSuggestion` +catch calls `updateErrorInfo(describeGenerationError(err))` directly +(`frontend/src/pages/draft/index.tsx`, new block), but `lastRequestRef` is only +assigned inside `getSuggestion` (`:319`). `retryLastRequest` (`:377`) re-runs +`lastRequestRef.current`. So on a non-auth `ScriptError` — `retryable: true`, so +the Retry button renders — Retry re-issues **the last request that succeeded**, +with its stale `docContext`. The writer gets a fresh-looking suggestion +generated from a ten-minute-old document. If there was no earlier request, Retry +silently does nothing. + +Chat and Revise both re-enter from the top and are correct; draft is the odd one +out. Two options: + +- **Minimal:** stash the mode in the catch and have Retry re-enter + `requestSuggestion(mode)`. ~6 lines. +- **What the PR body floats:** drop retry from the read-error path entirely, on + the grounds that a failed read shouldn't enter history at all. + +I'd take the minimal fix now — it makes draft consistent with the other two +pages, and the "should errors be in history" question is a bigger design call +that shouldn't gate a fix for a live Google Docs failure. + +### 3. #581 makes paragraph coordinates host-dependent + +`getParagraphs()` is the coordinate system for `view` and insert-after. After +#581, Google Docs drops blank lines: + +```ts +return text.split('\n').filter((line) => line.trim() !== ''); +``` + +Word (`wordEditorAPI.ts:190`) returns every paragraph item including empty ones, +and the standalone editor (`editor/editor.tsx:159`) returns every root child. +So paragraph index *N* means a different thing on Docs than on the other two +hosts. + +The filter is right *for Markdown* — blank lines are block separators there, not +content. But it should be a documented, host-uniform decision, not a side effect +of one host's serializer. Either normalize all three hosts to drop empties, or +keep them and have the gdocs serializer emit an explicit block index. Worth +raising on the PR; not worth blocking it, since Docs' current behaviour +(the entire document as a single paragraph, per #588) is strictly worse. + +### 4. #581 and #587 point in different directions on text matching + +#581 makes `getDocText()` return Markdown on Docs (`## Heading`, `- item`, +`1. step`). #587 makes needle-matching tolerant of *typography* — dashes, +quotes, spaces — but not of Markdown syntax. On Docs the model will read +`## Introduction` and the host document contains `Introduction`, so any +match-and-edit round-trip through Apps Script fails. + +This is inert right now: gdocs `applyEdit` is unimplemented (#590), and +`selectPhrase` is still a TODO in `googleDocsEditorAPI.ts:333`. But it means +**#590 must be implemented against Markdown-serialized text**, and it +strengthens the case for #589 ("send hosts ranges, not needles") — a range is +immune to both the typography problem and the Markdown problem at once. + +Neither PR should be held for this. Both should land, and #589 should be +re-read afterwards with #587's `textMatching.ts` in hand: that module is a +better starting point for #589 than what's on `main` today, because it puts all +the folding in one place instead of four. + +## Per-PR verdicts + +| PR | What it is | Verdict | Live test needed | +|---|---|---|---| +| **#574** PostHog off the request path | Backend infra. Telemetry `await flush()` was deciding response status; a 526 from the proxy 500'd healthy requests and failed a deploy. Non-vacuous test (restoring the await fails 3 of 4). | **Merge first.** 80 commits behind but only `backend/CLAUDE.md`/`README.md` moved under it. Deploy-stability fix that everything else's testing depends on. | No | +| **#595** Manifest template per environment | Replaces build-time regex surgery with one template + env table; emits all three manifests every build. Prod manifest is byte-identical to the old output. | **Merge second.** This is what makes Word beta testing possible at all. | Sideload `manifest-staging.xml` once | +| **#596** Design docs | Docs only, 2 files, no code. | **Merge any time.** Zero risk. | No | +| **#581** GDocs → Markdown | Fixes #588 (paragraph breaks lost). Real `Code.gs` under test via stubbed `DocumentApp`, which is the right call. Adds a Debug page at `lab` tier. | **Merge, after raising the paragraph-index question.** Needs `clasp push` — bundle and add-on deploy separately, so this one is not done when it's merged. | **Yes, Docs** | +| **#585** Doc-read error handling | Good error taxonomy in `api/errors.ts`; the Apps Script authorization-expiry case is the one writers actually hit. | **Fix the stale-retry bug, then merge.** | **Yes, Docs** (let a sidebar sit until the grant expires) | +| **#566** Consent PR 2 | The one you want. First-run gate, account page, cross-tab sync failing closed at `none`. Code quality is good — `broadcastConsentChange` resolving `localStorage` *inside* the try is exactly the right instinct for the Office iframe. | **Rebase, regenerate snapshots, then merge.** 149 commits behind; E2E is red on a demo-page screenshot that `main` has since regenerated (`535f970`). | **Yes, Word and Docs** — see checklist | +| **#587** My Words typography tolerance | Leniency ladder, exhausted tier by tier, index-mapped back to source so replacements use the writer's real characters. The anchored run-together tier and the `notable`/`not able` guard are both correct calls. | **Merge.** Highest-quality PR in the set. | **Yes, Word** (voice edits) | +| **#598** Draft the brief from the document | Candidates render beside fields, never in them; session-only, never serialized; accept path goes through the same `setField` as typing. | **Merge last of the feature PRs.** No blocker found; it's the one that most deserves a real read on the *product* question rather than the code. | Optional (Word or Docs) | +| **#594** Room-scoped OAuth PKCE | Draft PoC. This *is* the tool-launch rearchitecture — it replaces the launcher grant with rooms + Authorization Code/PKCE. | **Leave draft.** Decide the paradigm before merging any of it. Rebase after #566 (both touch `AccountPage.tsx` and `app.ts`). | Not yet | +| **#519** Google add-on ToS | One static HTML file. Inert. | **Merge when the store listing is being prepared.** | No | + +Also open and stale, not covered above: #449, #446, #435 (MaryChen68, 2+ months, +no activity). Worth an explicit close-or-refresh decision rather than leaving +them to rot. + +## Staged plan + +### Stage 0 — Unblock the test path (no product change) + +1. Merge **#574**. Then remove/retarget `POSTHOG_HOST` in the k8s deployment — + the new default does nothing while that env var is set explicitly. The PR + flags this and it's easy to miss. +2. Merge **#595**. +3. New small PR: **derive the gdocs backend origin at runtime** (finding 1). +4. Deploy `main` to staging. Sideload `dist/manifest-staging.xml` in Word; run + `npm run validate:staging` first, since `office-addin-manifest validate` + couldn't run in the sandbox that produced #595. + +Exit criteria: staging reachable in Word ("Beta Thoughtful") **and** in Docs +via the sidebar picker, with the Docs sidebar demonstrably hitting the staging +backend — check `/api/ping` or a study-log write, not just that the UI loads. + +### Stage 1 — Free merges + +Merge **#596** now. Merge **#519** whenever the store listing is being prepared. +Neither interacts with anything. + +### Stage 2 — Google Docs correctness chain + +1. Merge **#581**, then `clasp push` the add-on. Verify on a live doc: headings + arrive as `#`, lists as `-`/`N.`, and `beforeCursor + selectedText + + afterCursor` still reconstructs the document exactly (the Debug page this PR + adds is the fastest way to see it). +2. Fix the stale-retry bug in **#585**, then merge. Verify against a real + expired grant: open the sidebar, leave it until authorization lapses, then + click a Draft feature and confirm the notice names "close and reopen the + sidebar" and offers **no** Retry. +3. Re-read #589 and #590 with #587's `textMatching.ts` in hand before writing + any gdocs `applyEdit`. + +Do these before #566, so that when consent testing on Docs goes sideways you +already know the document-read path is sound. + +### Stage 3 — Consent (#566) + +1. Rebase onto `main`, regenerate Playwright snapshots, confirm E2E green. +2. Verify `consentUpdatedAt` survives the whole round trip on a **fresh real + user** — it's exposed via Better Auth `additionalFields` + (`backend/src/auth.ts:76`) and the gate keys on + `hasSetConsent: !!device.user?.consentUpdatedAt`. A user whose session omits + the field is permanently gated. +3. Live-test on staging, both hosts. This is the highest-blast-radius PR in the + set: it is a **required** first-run gate for every authenticated user, so a + failure here locks everyone out of the tool, not just the consent screen. +4. Merge, deploy to staging, soak, then prod. + +Then rebase #594 on top. + +### Stage 4 — My Words (#587) + +Merge, then live-test voice editing in Word specifically: Word's autocorrect is +the source of the en-dash and curly-quote mismatches the PR exists to fix, so +the bug is only reproducible there. Confirm the notice strip retracts on a +landed edit (invariant 6) rather than persisting across turns. + +### Stage 5 — Brief proposals (#598) + +Merge and deploy. Then watch `brief_proposal_resolved` for the measurement the +PR was built around: candidates almost always accepted unedited would mean the +tool is writing the brief rather than co-creating it. + +## Live-test checklists + +**Word (staging manifest):** + +- Consent gate appears once on first run, and not again after Continue (#566) +- Account & privacy opens in the external browser; the sign-in-again notice is + accurate on this surface (#566) +- Consent lowered in the account tab silences the app tab's logging (#566) — + the task-pane iframe is exactly where `localStorage` may be partitioned, so + confirm the fail-closed path, not just the happy path +- Voice edit whose target contains an autocorrected en dash or curly quote now + lands (#587) +- `move` re-inserts the characters it removed, not the model's spelling (#587) + +**Google Docs (sidebar → staging, after stage 0):** + +- Sidebar talks to the staging backend, not prod +- Headings/lists arrive as Markdown; concatenation invariant holds (#581) +- Paragraph count matches authored blocks (#588 fixed) +- Expired grant → correct notice, no Retry button (#585) +- Consent gate and cross-tab sync behave as in Word (#566) + +## Decisions I need from you + +1. **#585 retry:** minimal fix (stash the mode) or drop retry from the + read-error path? I'd do the minimal fix now. +2. **Paragraph coordinates (finding 3):** normalize all three hosts to drop + empty paragraphs, or keep them and index blocks explicitly? +3. **#594:** is room-scoped OAuth the direction? Until that's settled, #594 sits + and the launcher grant stays. Nothing else in the open set depends on the + answer, so this doesn't block stages 0–5. +4. **#449 / #446 / #435:** close, or assign someone to refresh them?