Remove obsolete Builder, Debugger, and Generalist definitions - #97
Conversation
4992660 to
0cec887
Compare
0cec887 to
f6c59d0
Compare
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES: removing catalog IDs that remain valid in version-1 persisted onboarding records causes those records to fail hydration and reset to the initial onboarding state. The supplied GitHub evidence was inspected; all eight supplied check runs for the exact head SHA completed successfully. Static review only; no local code, tests, builds, package managers, hooks, binaries, or project scripts were run.
Deterministic publication result: 1 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
f636831 to
87e7779
Compare
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
NO PUBLICATION: the fresh full review found that the previously reported persisted-onboarding compatibility issue remains on the established-installation startup path, but the same underlying issue already has an unresolved automated GitHub thread and is therefore suppressed rather than republished. parsePersisted now normalizes retired IDs, but initializeOnboardingGraduation first checks the unnormalized record with isValidPersistedState; for an established installation, that check fails and the function overwrites the record with completedState before hydration, still discarding preserved consent, harness, setup, and work-type fields. The supplied GitHub evidence was inspected: Frontend checks and unit tests failed, three Tauri checks were in progress, two checks were skipped, and two passed at capture time; required checks still govern merge readiness. Static inspection only; no local code, tests, builds, package managers, generated binaries, hooks, or project scripts were run.
Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 1 duplicate(s) suppressed.
87e7779 to
5d4aac1
Compare
loganj
left a comment
There was a problem hiding this comment.
🤖 Carl review at exact head 5d4aac1aea8a624b2c9fd658ac9b9337739b1554.
Blocker — startup graduation still destroys established users’ persisted onboarding outcomes when they selected a retired recommendation.
parsePersisted() now normalizes builder / debugger / generalist before validation (src/features/onboarding/model/onboardingStore.ts:160-188), but normal app startup calls initializeOnboardingGraduation() first (src/main.tsx:190-197). Its preserveExisting branch still passes the raw record directly to isValidPersistedState() (onboardingStore.ts:76-96). Because agentIds no longer contains those three IDs (onboardingStore.ts:39-45), an established install’s otherwise-valid record fails that check. For cohort established-before-landing-v1, lines 96-106 then overwrite the whole record with completedState, discarding selected work types, configured/completed harness setup IDs, and the usage-data consent answer before parsePersisted() can perform the repair.
Affected state: any established-cohort upgrade carrying one of the three retired selectedAgentId values, across completed or in-progress records. Fresh/unknown cohorts reach the new normalization, but established users take the destructive branch. The added parameterized test exercises hydration directly and therefore does not cover the production startup ordering.
Please apply the same retired-ID normalization when deciding whether graduation should preserve a current-version record (or otherwise make the startup preservation check use the repaired state), and add a regression test that calls initializeOnboardingGraduation("established-before-landing-v1") with all durable fields populated before hydrating.
I found no separate product-effect issue in the other seven files: the catalog/i18n removals are confined to the retired recommendations; bundled distro/agents and already-created persona files are separate; the remaining catalog order and three-result limit are deterministic; malformed and unrelated unknown IDs remain rejected. I am not approving while the startup path above remains destructive.
Co-authored-by: Brother Darryl <146fb160a3266e6165bfa385f6048c975eda9e21cf65da097a0b5ea7952532a5@buzz.block.builderlab.xyz> Signed-off-by: loganj <loganj@squareup.com>
5d4aac1 to
5989572
Compare
loganj
left a comment
There was a problem hiding this comment.
🤖 Princess Donut — PASS. I independently audited exact head 598957205be228617ceaac95bf0b749f5600132a on a fresh read-only Blox workstation. The removal is confined to the retired recommendation catalog/copy; both persisted-state entry paths normalize only builder, debugger, and generalist before the existing validator, preserving lifecycle, work types, harness/setup, and consent while unrelated unknown or malformed IDs remain rejected. Agt. Builder and existing bundled/created personas are outside the changed catalog. English/Spanish keys match the four surviving entries, recommendation output remains three entries for every work type, pnpm check passed, onboarding tests passed 80/80, and full Vitest passed 6,667 with 1 skipped. No unexpected product effects found. I attempted APPROVE, but GitHub rejects approval through Logan’s account as self-approval, so this is a COMMENT review.
loganj
left a comment
There was a problem hiding this comment.
🤖 Carl final audit at exact head 598957205be228617ceaac95bf0b749f5600132a: PASS (COMMENT verdict; GitHub does not allow Logan’s account to approve its own PR).
I re-audited the complete eight-file diff and both persisted-state entry paths on a fresh read-only Blox workstation. The prior blocker is coherently repaired: initializeOnboardingGraduation() and parsePersisted() both apply the same narrow normalization before the existing validator (src/features/onboarding/model/onboardingStore.ts:88-92,162-189). Only builder, debugger, and generalist become null; unrelated unknown IDs and malformed records still take the pre-existing rejection path.
The startup-order regression covers all three retired IDs across valid in-progress/recommendations and completed/complete states while preserving work types, selected/completed harness setup, and consent (onboardingStore.test.ts:80-108). I also traced catalog consumers and confirmed the remaining recommendation order/three-result behavior stays deterministic; locale removals match only the removed entries; bundled distro/agents and already-created persona files are outside this catalog. distro/agents/agt-builder.md remains present with app-avatar:gloopies-3.
Independent Blox validation at this exact clean head:
just check: passed, including Biome formatting, i18n, contract freshness, and TypeScript.- Full
just test: 562 files passed; 6,667 tests passed, 1 skipped.
No blocking or non-blocking findings.
🤖 Leo — The persisted-state finding is addressed at 5989572. Both startup entry paths now apply the narrow retired-ID normalization before validation; parent-red/final-green coverage exercises all three retired IDs across in-progress and completed states while preserving work types, harness setup, and consent. All three independent Royal audits passed the exact final head.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE: the previously reported persisted-onboarding compatibility issue is fixed across both direct hydration and the established-installation graduation path. The shared normalization clears only the three retired recommendation IDs before validation, while preserving lifecycle, work types, harness/setup state, and usage-data consent; focused tests now exercise both in-progress and completed startup records for every retired ID. A fresh full review found no new publishable findings. Final self-check covered the changed onboarding and startup flows, accessibility, English/Spanish localization, navigation and consent preservation, async/error/never-completes/lifecycle/race behavior, test honesty, design-system and project rules, duplicate overlap, and blocking-evidence quality. The supplied GitHub evidence was inspected and all eight check runs for the exact head SHA completed successfully. Static inspection only; no local code, tests, builds, package managers, generated binaries, hooks, or project scripts were run.
Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
🤖 Brother Darryl
Summary
Remove Builder, Debugger, and Generalist from Berd’s retired onboarding recommendation catalog while preserving valid version-1 onboarding records that selected one of those retired IDs.
Details
builder,debugger, andgeneralistselections tonullbefore existing validation. Lifecycle, consent, provider/harness setup, work types, and all unrelated valid fields remain intact.Verification
pnpm vitest run src/features/onboarding— 13 files, 80 passed at598957205be228617ceaac95bf0b749f5600132a.in-progress/recommendationsandcompleted/completelifecycle states with work types, harness/setup, and consent populated. It fails at5d4aac1and passes at the final head.