Replies: 1 comment
|
Branch: Phase 0 audit corrections to this planThe Phase 0 audit (#4089) found several inventory inaccuracies in the original plan: Route counts:
Updated tier classification:
New exclusions (not in original plan):
Pilot route selection corrected:
Other audit findings (all green):
Phase 1 design is unblocked. See full audit comment on #4089. Baseline LOC: 14,160 total tablebase route LOC. ~8,500 in factory candidates. Target reduction across Phases 1-3: ~4,500 lines (~53% of candidate routes). |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TableBase Sync Handler Factory — Implementation Plan
TL;DR
30 TableBase POST /sync handlers in
apps/wiki-server/src/routes/tablebase/share ~750 lines of identical post-upsert boilerplate but only 3/30 have audit logging — 8/11 recent bugs were directly caused by this duplication. Build acreateSyncHandler<T>()factory that returns a Hono handler function (preserving RPC type inference), pilot it on 3 routes spanning all tiers, then migrate routes tier-by-tier behind a per-route feature flag. Top open question: shouldentities/thingsstay permanently hand-rolled, or push for "factory or bust"?Problem
Each new sync route requires copy-pasting ~150-200 lines of boilerplate: parse → validate → natural-key dedup → transaction { build values → fetch existing → upsert → audit → resolveEntityFKs → upsertThingsInTx → writeInlineVerdicts } → link claims → return counts. Developers must remember which features to include, leading to systematic gaps (3/30 audit, 2/30 source-check, 12/30 still using per-item loops with values listed twice). Recent bugs traced to missing utility calls in routes that nobody enforced consistency on (PR #4029 added
validateEntityRefsto 8 missing endpoints; PR #4057/#4059 fixed inconsistent SQL array parameterization).Current State
apps/wiki-server/src/routes/tablebase/crux/validate/validate-tablebase-completeness.tsapps/wiki-server/src/routes/tablebase/audit-log.tslogAuditEntries()shared utilityapps/wiki-server/src/routes/tablebase/write-inline-verdicts.tswriteInlineVerdicts()shared utilityapps/wiki-server/src/routes/shared/resolve-entity-fks.tsresolveEntityFKs()shared utilityapps/wiki-server/src/routes/shared/validate-entity-refs.tsvalidateEntityRefs()shared utilityapps/wiki-server/src/routes/shared/source-check-enforcement.tsenforceSourceCheck()shared utilitycrux/wiki-server/sync-common.tsbatchSync<T>()factoryKey Decisions
Factory returns handler function, not sub-app. Confirmed via PR feat: tablebase factories — registry, client delete, paginated query helper #4067's
paginatedQueryprecedent — returning raw shapes preservesInferResponseType<>. Type test in pre-Phase-1 audit (#0) must verify this empirically before any code changes.Pilot spans all tiers, not just Tier 1. Phase 1 must convert 1 Tier 1 + 1 Tier 2 + 1 Tier 3 route to stress phase-omission paths and escape hatches simultaneously — picking only Tier 1 routes gives false confidence because they exercise every phase.
Hard cap: 1 escape hatch per route.
preValidate,postUpsert,conflictSetare escape hatches, not extension points. Routes needing >1 hook stay hand-rolled. Validator enforces the budget. Without this, the factory drifts toward god-object.Per-route feature flag for safe migration.
USE_SYNC_FACTORY_ROUTES=grants,personnelenv var lets each route fall back to legacy handler. Legacy handler kept alive for 7 days after each migration; deletion is its own commit.Phase 4 (source-check enforcement rollout) cut from this plan. It's a policy rollout, not deduplication. Filed as separate epic. This plan ends at Phase 3 + validator promotion.
Excluded routes (~2 stay hand-rolled):
entities.ts(slug displacement),things.ts(it IS the things table).bluesky.ts /sync/:did,research-areas.ts /sync-organizations//sync-papersare secondary endpoints, not the main/sync— those primary endpoints DO migrate to the factory.Phase ordering inverted from initial proposal. Original "audit logging first" is backwards: adding audit to per-item loops creates N+1 query patterns. Factory adoption happens first; audit/verdicts/enforcement arrive as side effects.
Architecture
apps/wiki-server/src/routes/tablebase/sync-factory.tscreateSyncHandler<T>()+SyncConfiginterface (~250 lines). Auto-chunks batches based on Postgres param limit (floor(65535 / columnCount)). Wraps every phase inSyncPhaseError({ route, phase, cause })for stack-trace context.apps/wiki-server/src/routes/tablebase/sync-factory.test.tsapps/wiki-server/src/routes/tablebase/sync-factory.test-d.tsexpectTypeOfassertingInferResponseType<>resolves to the exact expected shape on a factory-built route.crux/commands/tablebase/sync-scaffold.tspnpm crux tb sync scaffold <table>emits a starter route file with minimum config. Without this, agents will copy-paste instead of adopting the factory.crux/validate/validate-tablebase-completeness.tscrux/validate/validate-insert-set-parity.tsapps/wiki-server/src/routes/tablebase/{3 pilot routes}.tsapps/wiki-server/src/routes/tablebase/{14 Tier 2 + 13 Tier 3 routes}.tsapps/wiki-server/src/config/feature-flags.tsUSE_SYNC_FACTORY_ROUTESenv var parsing.claude/rules/tablebase-sync-factory.mdcontent/docs/internal/sync-factory-reference.mdxImplementation Phases
Phase 0: Pre-Phase-1 Audit Session — S
Goal: Resolve the 6 blocking technical gaps from red team review BEFORE writing any factory code.
rg -l "\.post\(\"/sync\"" apps/wiki-server/src/routes/tablebase/and classify each file exactly once into Tier 1/2/3/excludedaudit-log.tsfor batch safety: confirmlogAuditEntries()batches the existing-row pre-fetch in a singleSELECT ... WHERE id IN (...). If per-item, fix it.wccolumns × default batch size. Document any route exceeding ~200 rows × 30 cols and design chunking strategy.txhandle, MUST only do DB work on it, external side effects forbidden, throwing rolls back the entire sync.sync-factory.test-d.ts) FIRST against a hand-mocked factory signature to verifyInferResponseType<>works before implementing.wc -l apps/wiki-server/src/routes/tablebase/*.ts > /tmp/tb-baseline.txtfor measuring savings.Quality gates: Audit document committed to PR description before any code changes. Type-level test compiles. No factory code written yet.
Exit criteria: All 6 blocking gaps resolved. Route inventory matches between this plan and reality.
Phase 1: Factory + 3 Tier-Spanning Pilot Routes — M
Goal: Working
createSyncHandler<T>()factory adopted by 3 routes spanning all tiers (1 Tier 1 + 1 Tier 2 + 1 Tier 3). Net code change negative. Validates the design before any tier sweep.sync-factory.tswith the 7-phase pipeline. Every phase wraps errors inSyncPhaseError({ route, phase, cause }). Auto-chunks batches per pre-Phase-1 calculation.pnpm crux tb sync scaffold <table>emits minimum-config route fileUSE_SYNC_FACTORY_ROUTESenv var; factory routes check the flag and fall back to legacy handler if excludedpersonnel.ts(Tier 1, exercises all phases includingpostUpsertfornew:prefix backfill)divisions.ts(Tier 2, exercises phase-omission — no claims, no FK resolve)political-scores.ts(Tier 3, exercises batch conversion from per-item loop)pnpm tsc --noEmitcompile time hasn't regressed >10% (DX gate)Quality gates:
pnpm test apps/wiki-serverpassespnpm crux w validate gate --fixpassesnpx tsc --noEmitshows no new errors AND compile time within 10% of baselineInferResponseType<>resolves on a factory-built routecurl -X POST localhost:3001/api/personnel/sync -d @fixture.jsonreturns expected shapeExit criteria: 3 pilot routes use factory, RPC inference works, scaffolder works, feature flag works, compile time acceptable. STOP-AND-JUDGE GATE: produce a Phase 1 retrospective answering "did the factory save what we expected? did escape hatches stay ≤1 per route? did RPC inference survive? compile time impact?" Phase 2 only proceeds with explicit user sign-off.
Phase 2: Migrate 14 Tier 2 Routes — L
Goal: All Tier 2 routes use the factory. Each migration adds audit logging + verdicts as side effects. Net code change: ~2,800 line reduction.
/sync, bluesky main/sync(mixed)Quality gates:
pnpm test apps/wiki-serverpasses after each batchExit criteria: 17 of 28 non-excluded routes (3 pilot + 14 Tier 2) use the factory. Hook usage ≤ 1 per route. STOP-AND-JUDGE GATE before Phase 3.
Phase 3: Migrate 12 Tier 3 Per-Item-Loop Routes — L
Goal: All Tier 3 routes use the factory. Batch conversion happens during factory adoption. Net code change: ~2,500 line reduction PLUS performance improvement.
validate-insert-set-parity.tsBEFORE converting any route. Run on full codebase to find existing drift bugs.Quality gates:
wiki-serverfor 24h after each deployExit criteria: 28 of 28 non-excluded routes use the factory. Validator promoted to blocking for factory adoption + hook budget. Documentation written:
.claude/rules/tablebase-sync-factory.md+content/docs/internal/sync-factory-reference.mdx.Scope Cuts
Explicitly NOT included:
mode: "off" | "warn" | "block"flag flipping is a policy rollout, not deduplication.paginatedQueryalready exists; widening adoption is separatePOST /api/tablebase/syncendpoint — breaks Hono RPC typesentities.tsmigration — slug displacement too uniqueQuality & Verification Infrastructure
sync-factory.test.ts(factory unit tests against real test postgres).sync-factory.test-d.ts(TypeScript-level RPC inference test). Per-route round-trip tests written BEFORE conversion: happy path + FK-invalid + duplicate-id + 500-item batch + concurrency race. Parameterized adversarial tests reproducing PR #4057, #4029, #3400 bug classes — run for every factory-adopted route.rg "for \(const item of items\)" apps/wiki-server/src/routes/tablebase/must return 0 after Phase 3.rg "createSyncHandler" apps/wiki-server/src/routes/tablebase/must show 28 routes. Validator's coverage grid must show 28/28 factory adoption.pnpm crux tb sync <table> --batch=500 fixtures/sample.jsonthenpsql -c "SELECT COUNT(*) FROM tablebase_audit_log WHERE record_type=$1 AND created_at > now() - interval '1 minute'"— verify audit count matches insert count.cd apps/web && PLAYWRIGHT_BASE_URL=https://www.longtermwiki.com npx playwright test e2e/render-audit.spec.tsafter each phase to confirm directory pages still render.USE_SYNC_FACTORY_ROUTES=<csv>(Phase 1 onward). No DB migrations, no new tables. Standard deploy via image build + ArgoCD. Per-route rollback = remove route from env var CSV..claude/rules/tablebase-sync-factory.md(terse, <200 lines, auto-loaded),content/docs/internal/sync-factory-reference.mdx(deep-dive with per-tier worked examples), JSDoc on everySyncConfigfield annotated with "Used by phase N".logSourceCheckCoverage().SyncPhaseErrorcarries route+phase context into structured logs. Audit log table queryable for change history. Compile-time impact tracked in CI.Risks & Mitigations
logAuditEntriesbatches its pre-fetch.chunkSize = floor(65535 / columnCount). Test with 1000-item fixture.tx, DB-only side effects, throwing = full rollback. Test asserts zero rows persist whenpostUpsertthrows..values()and.set()(PR #4057 class)validate-insert-set-parity.tsruns in Phase 3 BEFORE any conversion. Factory auto-derives SET fromtoRow()keys, making drift structurally impossible in adopted routes.SyncPhaseError({ route, phase, cause }). Phase 1 test asserts error message contains route name.granteeIdlook like FKs but aren'tentityRefFieldscallback excludes legacy fields explicitly.entities.tsexcluded permanently. Validator: if route importsdisplaceEntitySlug, cannot use factory.tsc --noEmittime must stay within 10% of baseline. Factory's exported handler type stays non-generic.Open Questions
entitiesandthingspermanently stay hand-rolled, or push for "factory or bust" with custom escape hatches per route?Rejected Approaches
runPostSyncPipeline()only) — Doesn't enforce parity; developers still must remember to call it. Preserved as fallback if Phase 1 factory pilot fails.Red Team Log
Technical critic (24 issues, 6 blocking): route inventory contradictions, hook rollback contract undefined, audit pre-fetch N+1 risk, Postgres param limit ignored, enforcement window assumes weekly cadence, RPC type inference unverified, secondary sync endpoints unaddressed, non-entities FK targets, conflictSet defeats validator, test harness too thin, round-trip tests don't catch the targeted bugs, custom logic inventory missing, no rollback plan, audit log retention scope creep. Resolutions: Phase 0 audit session added; chunking required; type test added; hook contract specified; Phase 4 cut and audit retention listed as blocking-for-Phase-3 separate issue.
Scope critic (10 cuts): Factory may be unnecessary (minimalist gets 80%); Phase 4 is scope creep; test harness premature; column-drift validator unnecessary if factory generates SET; Phase 1 not truly independent (Tier 1 only doesn't stress phase-omission); rules doc premature; ship Phase 1 alone option; 5-route exclusion creep; publications double-listed; no go/no-go between phases. Resolutions: Phase 4 cut; pilot now spans tiers (1/2/3); test harness simplified to per-route plain tests; column-drift validator scoped to one-shot Phase 3 use; rules doc deferred to Phase 3 exit; explicit stop-and-judge gates added; exclusions reduced from 5 to 2; route inventory bug fixed.
DX critic (8 issues): Cognitive cost vs typing cost (need scaffolder); stack traces eat context (need per-phase error wrapping); escape hatch slippery slope (need hook budget); fixed response shape (need extendResponse); docs underspecified (need split rule + reference); regression revert cost (need feature flag); LLM agents confused (need discriminated unions + examples); compile time (need non-generic handler type). Resolutions: Scaffolder added as Phase 1 deliverable; per-phase error wrapping required; hook budget validator added;
USE_SYNC_FACTORY_ROUTESfeature flag added; documentation strategy expanded; compile time gate added.Tasks
Phase 0: TableBase factory pre-flight audit #4089 — Phase 0: TableBase factory pre-flight audit
Phase 1: Factory + 3 tier-spanning pilot routes #4090 — Phase 1: Factory + 3 tier-spanning pilot routes
All reactions