TASK-22490: Require supported clients before API compatibility cleanup - #3222
jjramirezn wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code-analysis diffPainscore total: 7997.19 → 8026.57 (+29.38) 🆕 New findings (49)
…and 29 more. ✅ Resolved (34)
…and 14 more. 📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
|
/chip review |
🖼 Visual diff — 3 screens moved4 of 96 shots changed · 92 identical · baseline
new screens (2)
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
One major finding: the policy gate accepts a pushed but unmerged mono commit, so an unreviewed support floor can pass CI and be deployed.
Findings
-
MAJOR · .github/workflows/tests.yml:333 · Require the policy pin to be on mono main
The lock controlsMONO_SHA, and this job fetches that commit directly without proving it is contained inpeanutprotocol/mono's main branch. The exact pin in this head is currently divergent from mono/main. A future UI PR can therefore pin a pushed branch commit withminimumGenerationraised, update the matching lock and snapshot, and make both policy jobs pass even though mono review never approved that floor; deployment would then block every older client. Before deriving the proof, compare the pin with mono/main and require it to be an ancestor (or otherwise keep deployment gated until the mono change merges), and cover a divergent pin in the workflow test. -
MINOR · src/context/OtaUpdateContext.tsx:201 · [claude-opus] OtaUpdateContext.checkNow ships untested
checkNowis a new method on the shared OTA context (src/context/OtaUpdateContext.tsx:201). It mutates state the rest of the app depends on: on success it pushes a staged bundle intopendingBundleand can setstoreUpdateRequired, andpendingBundleis whatapplyNowthen restarts the app onto and what the Profile OtaUpdateModal renders. src/context/tests/OtaUpdateContext.test.tsx covers essentially everyapplyNowbranch (deadlocking Android plugin, set() rejection, marker retarget, double-tap, off-native) but was not touched by this PR, and a grep forcheckNowshows it is only ever reached through a jest mock in RequiredUpdateScreen.test.tsx and ClientSupportGate.test.tsx — the real implementation runs in no test.
Untested cases, concretely: (1) off native, checkNow returns 'unavailable' without importing the updater chunk; (2) on native, it passes onUpdateAvailable/onStoreUpdateRequired through to setPendingBundle/setStoreUpdateRequired so the bundle this check stages is the same one applyNow restarts onto — the comment at line 199 asserts exactly this and nothing verifies it; (3) a failed importWithChunkRetry is swallowed and reported as 'unavailable' rather than throwing into the screen's check().
Fix: add a checkNow block to src/context/tests/OtaUpdateContext.test.tsx alongside the existing applyNow blocks, using the same queueUpdateCheck stub the file already sets up for init, asserting the returned outcome and the resulting pendingBundle/storeUpdateRequired for each of the three cases.
Checked clean
- Confirmed the detached worktree is at the supplied head and its merge base is the supplied dev base SHA.
- Reviewed the startup and resume gate, strict policy parsing and storage fallback, marketing-route bypass, service-worker and HTTP cache controls, and native OTA/store plus web reload recovery paths.
- Compared the pinned mono registry and compatibility runbook; the pinned commit exists but is not an ancestor of mono/main.
- Required policy, unit, formatting, lint, native-export, and aggregate CI checks were green at this head; visual capture jobs were still running when checked.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 2d4dce195995 · Context: repo, mono, ci, official-docs · Took 18m
| if (!/^[a-f0-9]{40}$/.test(lock.monoSha)) throw new Error("lock.monoSha is not a full sha") | ||
| process.stdout.write(lock.monoSha) | ||
| ') | ||
| GH_TOKEN=$MONO_TOKEN gh api \ |
There was a problem hiding this comment.
MAJOR: Require the policy pin to be on mono main
The lock controls MONO_SHA, and this job fetches that commit directly without proving it is contained in peanutprotocol/mono's main branch. The exact pin in this head is currently divergent from mono/main. A future UI PR can therefore pin a pushed branch commit with minimumGeneration raised, update the matching lock and snapshot, and make both policy jobs pass even though mono review never approved that floor; deployment would then block every older client. Before deriving the proof, compare the pin with mono/main and require it to be an ancestor (or otherwise keep deployment gated until the mono change merges), and cover a divergent pin in the workflow test.
There was a problem hiding this comment.
Fixed in c1c98bc. Before fetching the registry, the source job compares the pin with mono main and accepts only ahead/identical with the merge base equal to the pin. Nine tests execute the actual workflow block and reject divergent/behind/malformed/failed responses. The current pin is unmerged, so this PR now stays blocked until mono #194 merges; no bootstrap bypass was added. The minor checkNow finding is also covered by five real-context tests, including the staged-bundle handoff to applyNow. Full local suite: 622 suites, 7,712 tests passed; typecheck and formatting passed.
|
/chip review |
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The mono ancestry check is implemented, but its failure is skipped out of the required umbrella gate, so an unmerged policy pin still does not block the PR.
Findings
-
MAJOR · .github/workflows/tests.yml:1199 · Failing ancestry check still passes ci-success
ci-successdepends only onclient-support-policy, while that job is skipped whenever itsclient-support-sourcedependency fails. The umbrella condition rejects onlyfailureandcancelled, so it treats that skip as success. This exact head demonstrates the path:client-support-sourcefailed on the divergent mono pin,client-support-policywas skipped, andci-successsucceeded. The required gate therefore still permits a policy pin that is not on mono main. Addclient-support-sourcedirectly to the umbrella dependencies or require every dependency result to equalsuccess, including rejectingskipped. -
MINOR · next.config.js:466 · [claude-opus] No test pins the /client-support.json CORS and no-store headers
next.config.js:465-474addsAccess-Control-Allow-Origin: *andCache-Control: no-storefor/client-support.json. This is not decorative:CapacitorHttpis disabled (capacitor.config.ts:76-78), so native requests go direct from the webview under real CORS, andclientSupportPolicyUrl()(src/utils/client-support.ts:101) makes native read it cross-origin fromhttps://peanut.me. If that header is ever dropped, reordered behind a broader rule, or lost in a config refactor,fetchClientSupportPolicy()returns null,resolveClientSupport()yieldsunavailable, and every iOS/Android user is parked onPolicyUnavailableScreenwith no route into the wallet — a silent total native outage. Theno-storehalf is the other guarantee: a cached floor is either a block nobody can lift or a client still running after the floor moved. The repo already has the exact precedent for this test (src/features/payment-network-explorer/__tests__/headers.test.tsloadsnext.config.jsand asserts the rule for/dev/payment-graph). Untested case to add:headers()returns a/client-support.jsonrule carryingAccess-Control-Allow-Origin: *andCache-Control: no-store, max-age=0, and no later matching rule overrides either key. Worth pairing with an assertion thatsrc/app/sw.tskeeps theNetworkOnlyroute ahead ofdefaultCache, which is the same guarantee on the PWA side. Note this is a missing test, not a defect in the shipped header — the rule as written is correct.
Checked clean
- Exact head and merge base matched the supplied SHAs; reviewed all changed production and workflow paths plus focused tests.
- Client-support parsing, uncached fetch, cached-block fallback, startup/resume gating, provider placement, and web/native recovery paths.
- Policy snapshot, lock, proof derivation, exact mono registry shape, and live mono ancestry response.
- Workflow credential isolation and public proof contents; no private registry fields are emitted.
- Current CI at this SHA: unit, typecheck, format, eslint, and native export succeeded.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: c1c98bc61778 · Context: repo, mono, ci · Took 14m
| eslint, | ||
| typecheck, | ||
| native-export, | ||
| client-support-policy, |
There was a problem hiding this comment.
MAJOR: Failing ancestry check still passes ci-success
ci-success depends only on client-support-policy, while that job is skipped whenever its client-support-source dependency fails. The umbrella condition rejects only failure and cancelled, so it treats that skip as success. This exact head demonstrates the path: client-support-source failed on the divergent mono pin, client-support-policy was skipped, and ci-success succeeded. The required gate therefore still permits a policy pin that is not on mono main. Add client-support-source directly to the umbrella dependencies or require every dependency result to equal success, including rejecting skipped.
This branch was successfully deployed
1 participant
Summary
Require a supported UI bundle before the wallet opens, so temporary API compatibility code can eventually be removed. Startup and resume check a small public policy generated from a pinned mono registry. Unsupported native bundles use the existing OTA restart/check flow or store recovery; web and PWA clients reload.
A stale cached “supported” result never admits the wallet. A cached block survives network failure. Resume checks keep wallet state behind an inert cover; a confirmed unsupported result unmounts the wallet providers. Returning from marketing starts a fresh check. OTA readiness remains above the gate.
Task and dependencies
TASK-22490 — https://www.notion.so/3d7838117579802d84dec4188d981a60
Pairs with mono #194 and API #1613. Mono owns the seven-day retirement report and owner approvals. Merge mono #194 first: the policy pin must be in mono
mainbefore UI CI can pass. If mono is squash-merged, sync the pin from the merged commit, then rerun the UI checks. CI verifies the public policy against the pinned source; only the policy, pin, and digest enter public artifacts.Review fixes and current gate
Chip's unmerged-pin finding is fixed: CI requires the pinned source to be an ancestor of mono
mainbefore it fetches the registry. Nine tests execute the actual workflow check, including divergent pins and failed API calls. Five additional tests exercise the real OTA context'scheckNowand its handoff toapplyNow.The current source pin is still unmerged. The UI policy check must stay red until mono #194 merges. This is the required deployment order, not a bypass. The PR remains draft for that dependency and review of the updated commit.
Risk and rollout
All support floors start at zero. No update is required yet and no backend version check or request header is added. The new client depends on reaching the public policy: an outage shows a retry screen and blocks wallet interaction. Publish the web policy before shipping native bundles that require it.
This cannot retrofit the guard into existing installations. Before raising any floor, publish the replacement, test OTA/store recovery and compatible rollback bundles, and resolve legacy-client coverage. Seven quiet days alone do not authorize cleanup.
The existing PostHog platform property now registers before the first pageview. No new personal-data category or processor is added. Customer help updates belong to a separate change before a nonzero floor is activated.
Validation
Focused client-support, OTA, fixture, locale, and provider tests pass, including the stale-cache and return-from-marketing regressions. Typecheck, production web build, and full formatting pass. Lint reports zero errors (68 warnings). Full unit suite: 622 suites and 7,712 tests pass, with 7 existing skips. The actual pinned GitHub registry matches the public zero-floor policy.
Two web fixtures captured at 375×667 with no browser errors:
update-requiredandupdate-check-failed. Native OTA/store behavior has unit coverage; a device or simulator pass is still required before release.The advisory Screen library capture currently fails on two existing badge fixtures (
36-c-badgedetailmodaland37-c-badgestatusdrawer): their/badges/first-invite.webpasset is absent from both the PR and base trees. The base English capture logs the sameNoFallbackError, but completes; these after captures report a transport failure. Both new client-support fixtures captured successfully. Required CI and the preview deployment passed before the ancestry fix. The source gate now intentionally blocks this PR until mono #194 merges.Design notes
The two new screen compositions reuse OfflineScreen/BackendErrorScreen primitives and the AppLock cover pattern. They need a shared gate above wallet providers; existing modals sit inside those providers. Figma mapping: code-only ❓, no board yet. No new design-system primitive or dependency is introduced.
Screenshots
Fixture screenshot check passed, including both new recovery screens.
Web fixtures at 375×667. Native device validation remains pending.
The PR-only
pr-assets-3222branch can be deleted after merge.