Finish sync mapping persistence + bulk force-sync, fix dead Linear adapter registration - #95
Conversation
Two stubbed endpoints behind the already-built /sync dashboard (mapping config PUT, bulk force-sync) never got finished. Specs closing both using existing SystemConfig table and TRACKER_SYNC job - no new schema or job types needed.
5 tasks, each red-green tested: SystemConfig-backed mapping persistence, loadStatusMap wiring, initializeSyncEngine override support, worker adapter registration fix (Linear was never wired up), bulk force-sync.
loadStatusMap only truthiness-checked entry.outpostStatus before casting it straight into the StatusMap config. A persisted row with a typo'd or corrupted outpostStatus (e.g. 'GARBAGE') passed through untouched, so toOutpost() later returned that literal garbage string instead of the safe TicketStatus.OPEN fallback -- silently corrupting downstream ticket status logic and, via the sync push path, risking a bad status write to Linear. Fix: reject any entry whose outpostStatus is not a real TicketStatus enum member (Object.values(TicketStatus).includes(...)) before adding it to the config. If every entry for a plugin is invalid, the existing "entries present but map empty -> fallback" logic already covers it. Call-site enumeration (grep -rn "loadStatusMap" packages/outpost apps): - packages/outpost/shared/src/sync/index.ts:4 -- re-export only, signature unchanged, unaffected. - packages/outpost/shared/src/sync/__tests__/status-map.test.ts -- updated with 2 new tests (invalid entry skipped / all-invalid fallback), all 7 tests pass. - apps/worker/src/build-sync-engine.ts:14 -- consumes the returned StatusMap object only; return type and success-path behavior unchanged, stricter filtering only improves correctness for it. - apps/worker/src/__tests__/build-sync-engine.test.ts:7 -- mocks loadStatusMap entirely, never exercises real implementation, unaffected.
Two CR findings on the same function, fixed together (both live in POST /api/sync/force): - Narrowed try/catch to only cover request.json() parsing, so a mid-loop DB/queue failure propagates instead of being mislabeled "Invalid request body" (400). - The "known plugin" gate now also accepts a plugin with a real TicketExternalLink, not just a prior SyncEvent -- previously a plugin's very first force-sync (real links, zero sync history) 404'd. Call-site enumeration: POST is framework-invoked by Next.js; no other code calls it directly. Signature/return type unchanged.
PUT /api/sync/mappings only truthiness-checked statusMappings and
priorityMappings, so a caller could persist a garbage-shaped value
(wrong type, unknown outpostStatus/outpostPriority) and GET would
echo it back as valid config. Adds isValidMappingShape() checking
both fields are { [plugin]: Array<{external*, outpost*}> } with
outpost* values restricted to the real TicketStatus/TicketPriority
enum members, imported from @copilotkit/outpost/shared (not /db,
which only exports the prisma client).
Call-site enumeration: PUT is framework-invoked; no other code calls
it directly. isValidMappingShape is new, no other call sites.
- force/route.ts: "per changed field" implied change detection; the handler unconditionally enqueues both jobs. - build-sync-engine.ts: presented Linear registration as unconditional; it's env-gated (LINEAR_API_KEY + LINEAR_TEAM_ID). - mappings/route.ts: top-of-file comment said mappings are "kept as code defaults," directly contradicted by the PUT/GET persistence logic added right below it. Comment-only, no behavior change, no test required.
…nfig
isValidMappingShape({}, ...) returned true because Object.values({}).every(...)
is vacuously true. A PUT with {statusMappings: {}, priorityMappings: {}} passed
shape validation, got persisted, and GET then returned {} for both fields
instead of falling back to defaults -- silently wiping any real config.
Now require at least one own key on the top-level object before checking
entries, so a fully-empty mapping object is rejected (400) while a config that
legitimately maps some plugins and omits others (e.g. {linear: [...]} with no
github key) still passes.
Call-site enumeration (grep -n "isValidMappingShape" route.ts):
114: function isValidMappingShape(...)
161: if (!isValidMappingShape(body.statusMappings, Object.values(TicketStatus)))
168: if (!isValidMappingShape(body.priorityMappings, Object.values(TicketPriority)))
Exactly two call sites (statusMappings, priorityMappings), both unaffected for
non-empty inputs -- only a fully-empty {} now fails at each site.
PUT /api/sync/mappings validated statusMappings/priorityMappings shape
but persisted body.labelRules verbatim with zero validation, so a
caller sending labelRules: "garbage" or labelRules: 42 would have it
stored and later echoed back by GET as if it matched
Record<string, Array<{externalPrefix, outpostPrefix}>>.
Added isValidLabelRulesShape (mirrors isValidMappingShape's object/array
checks but drops the enum constraint since outpostPrefix can be any
string, including empty). PUT now returns 400 without calling
prisma.systemConfig.upsert when labelRules is present but malformed.
Call-site enumeration: grepped labelRules across apps/web/src and
packages/outpost. Only other reads are mapping-editor.tsx (reads the
already-typed GET response into client state) and mock-sync.ts (unrelated
mock/dev data). Nothing else reads body.labelRules directly, so nothing
depended on the previous unvalidated pass-through.
Tests: added a red/green case (garbage labelRules -> 400, upsert not
called; confirmed failing against old code, passing after) and a
green-only case establishing missing coverage for the valid labelRules
persistence path.
The PUT handler in apps/web/src/app/api/sync/mappings/route.ts wrapped
the entire body — including prisma.systemConfig.upsert — in a single
try/catch that mapped any failure to a 400 "Invalid request body".
A DB failure (connection issue, constraint violation) was therefore
mislabeled as a client error instead of propagating as a 500. Same bug
class already fixed in the sibling apps/web/src/app/api/sync/force/route.ts
(narrowed try/catch to only wrap request.json() parsing) but not
mirrored here.
Narrowed the try/catch to cover only `await request.json()`; the shape
validation checks and the prisma.systemConfig.upsert call now run
outside that catch, so a DB failure propagates as an unhandled error
(Next.js turns it into a 500) instead of a misleading 400.
Added a covering test mirroring the equivalent force-route test:
mocks systemConfig.upsert to reject with an Error and asserts the PUT
handler rejects/throws rather than returning a 400. Verified red
(failed against the old code, which returned a 400 response) then
green (passes with the fix) before committing.
Call-site enumeration: PUT is a framework-invoked Next.js route
handler. Only call site in the codebase is the frontend fetch('/api/sync/mappings',
{ method: 'PUT', ... }) in apps/web/src/app/sync/mappings/page.tsx —
no direct code call-sites to the exported PUT function besides the
test file. Trivially clean.
Round-3 CR finding: the docstring claimed TRACKER_SYNC jobs "no-op"
when no adapter is registered, but the handler returns
{success:false, error:'Plugin "..." is not registered'} — the job
fails and retries per the queue's normal policy, it doesn't silently
do nothing. Comment-only, no behavior change.
…iorityMappings
isValidMappingShape already rejected {} to stop empty config from
silently wiping stored statusMappings/priorityMappings. isValidLabelRulesShape
never got the same guard, so Object.values({}).every(...) vacuously returned
true and PUT with labelRules: {} passed validation and persisted an empty
labelRules object. Flagged independently by 3 reviewers across two CR rounds.
Call-site enumeration: isValidLabelRulesShape is called exactly once
(route.ts:212), guarded by `body.labelRules !== undefined` — undefined
(optional labelRules) still bypasses validation entirely, unaffected.
A non-empty valid labelRules object still passes since the new check
only rejects zero-key objects.
Standing rule going forward, not case-by-case: run copilotkit-internal:cr-loop on the diff before pushing any non-trivial change. Prompted by this branch's CR loop catching real bugs across 3 rounds that would otherwise have shipped.
Cosmetic only -- embedded code-fence reflow in the plan/spec docs to satisfy prettier --check. No content change.
The /sync dashboard persists statusMappings, priorityMappings, and labelRules, but the worker only loaded the status map — priority/label edits saved + displayed yet had zero effect on sync (init always used the hardcoded createLinearPriorityMap/createLinearLabelMapper). Mirror the status pattern: - loadPriorityMap(plugin, db) in priority-map.ts (+ PriorityMapDb) - loadLabelMapper(plugin, db) in label-map.ts (+ LabelMapperDb) - priorityMapOverride / labelMapperOverride options on initializeSyncEngine - buildSyncEngine loads all three (Promise.all) and passes the overrides - export the loaders + Db types from sync/index Same fallback ladder as loadStatusMap (missing row / malformed JSON / no plugin entry / invalid entries -> hardcoded default). Priority validates the TicketPriority enum; label validates string prefixes. Tests: loadPriorityMap + loadLabelMapper (mirroring loadStatusMap), init priority/label overrides, build-sync-engine wiring. 762 outpost + 1 worker test pass; typecheck clean. Stacked on #95 (sync-mapping-persistence-bulk-force-sync); depends on its loadStatusMap/statusMapOverride pattern. Closes #96
Code reviewReviewed the diff (endpoints + worker adapter wiring + shared OverviewThree related fixes to the
Scope is well-drawn: priority/label acting-on, hot-reload, and GitHub are explicitly deferred to #96/#97/#98 and documented in code. What's strong
Issues & suggestions
Security
VerdictApprove with minor comments. No blocking correctness or security issues. The dead-adapter fix is a real latent bug worth catching, and test coverage is genuinely thorough. Items #1 (empty-array divergence) and #2 (sequential enqueue) are the two worth a follow-up. 🤖 Generated with Claude Code |
…pings fix(sync): apply persisted priority + label mappings in worker (#96)
jerelvelarde
left a comment
There was a problem hiding this comment.
The substance here is good, and the Linear-adapter finding is the valuable part of the PR — outbound sync being silently dead because nothing registered the adapter is exactly the kind of bug that survives indefinitely, since every visible layer (adapter, webhook, dashboard) looked present. Extracting buildSyncEngine() into its own module with its own test is the right shape, and the doc comment explaining that a missing LINEAR_API_KEY yields zero adapters and retried failures rather than silent drops is the sort of thing that saves the next person an afternoon.
The mappings route work holds up: persisting to SystemConfig with the code defaults as fallback, validating outpostStatus / outpostPriority against the real TicketStatus / TicketPriority enums instead of accepting free strings, and rejecting an empty object so a save can't blank out a live config.
Requesting changes mainly because this can't merge in its current state, plus two things I'd like addressed while it's being rebased.
It conflicts with main, and the conflict is a trap
apps/worker/src/index.ts conflicts. The reason is that main has moved since this branched — GITHUB_REACTION_POLL landed (commit e6c871b) and touches the same import block you rewrote.
Concretely, main's index.ts has four references this branch has none of:
17: * - GITHUB_REACTION_POLL: Poll GitHub reactions on AI comments (no webhook exists)
34: handleGithubReactionPoll,
59: [JobType.GITHUB_REACTION_POLL]: 1,
78: worker.on(JobType.GITHUB_REACTION_POLL, handleGithubReactionPoll);
Resolving this by taking your side of the file wholesale — which is the tempting resolution, since your side is the one with the real change — silently unregisters the reaction poll. No test would catch it and no type error would fire; the job type would just stop having a handler. Worth resolving by hand rather than with --theirs.
Dropping createJob from the worker's imports is correct, though — I checked, and its only use on main is the SyncEngine construction your change replaces.
No CI has ever run on this branch
gh pr checks 95 reports "no checks reported on the branch." So the 1264-passing figure is a local result against a six-day-old main that has since taken the reaction poll, the job-table cleanup, and a format/lint pass. Worth re-running after the rebase before this goes in.
as never is doing the job as any was criticised for
build-sync-engine.ts has five of these:
loadStatusMap('linear', prisma as never)
loadPriorityMap('linear', prisma as never)
loadLabelMapper('linear', prisma as never)
deps: { prisma: prisma as never, createJob: createJob as never }
identityDeps: prisma as neveras never is strictly worse than as any here — never is assignable to every type, so it silences the check completely and leaves no marker of what the intended contract was. If SyncEngineDeps gains a required field, none of these break.
The frustrating part is that this PR already defines the right types and then doesn't use them. Your own sync/index.ts change newly exports StatusMapDb, PriorityMapDb, and LabelMapperDb — which are precisely what those first three casts should target — and SyncEngineDeps is exported for the fourth. So:
loadStatusMap('linear', prisma as unknown as StatusMapDb)
deps: {
prisma: prisma as unknown as SyncEngineDeps['prisma'],
createJob: createJob as SyncEngineDeps['createJob'],
}Same deliberate coercion, but it breaks loudly if the contract changes.
Worth knowing that #135 changes these exact lines in the opposite direction — it replaces prisma as any / createJob as any in index.ts with as unknown as SyncEngineDeps['prisma'], for this reason. Two of your own PRs are currently pulling the same wiring apart. See the ordering note at the bottom.
A corrupt config row is indistinguishable from no config
async function readPersistedConfig(): Promise<PersistedMappingConfig | null> {
const row = await prisma.systemConfig.findUnique({ where: { key: MAPPING_CONFIG_KEY } });
if (!row) return null;
try {
return JSON.parse(row.value) as PersistedMappingConfig;
} catch {
return null;
}
}A row that exists but won't parse returns null, identically to "never configured" — so GET serves the code defaults, the dashboard renders them as if they were the saved settings, and an admin who deliberately configured a mapping sees it silently reverted with nothing in the logs. Given this PR exists partly because the old endpoint accepted edits and threw them away, that's the same failure shape in a new place. A console.error in the catch (and ideally distinguishing "corrupt" from "absent" to the caller) would make it diagnosable.
Also note the as PersistedMappingConfig on the parse result is unchecked — you wrote isValidMappingShape for the PUT path, but the read path trusts whatever is in the row. A config written by an older version of the code, or hand-edited in the DB, reaches the worker unvalidated.
One question
const syncEngine = await buildSyncEngine(); is a top-level await in the worker's entry module, and it now performs three database queries before the module finishes evaluating. If the database isn't reachable at boot, that throws during import — before the health server starts — so the container crash-loops with no /health at all rather than coming up degraded. Fail-fast may well be what you want here, but it's a change in boot semantics that isn't mentioned, and it interacts with the worker-lifecycle follow-up you flagged on #135 (/health returning 200 on a wedged worker). Worth a sentence either way.
Suggested ordering
Since these overlap: #135 first (it's small, approved, and its SyncEngineDeps casts are in the file you're rewriting), then rebase this one on top. At that point #135's cast lines disappear into your buildSyncEngine() — carry the typed-deps approach into the new module and the as never item above resolves itself in the same pass. Doing it the other way round means #135's typing work lands and is immediately deleted.
The three descoped items (#96 priority/label not acted on, #97 no hot reload, #98 GitHub untouched) are well-drawn boundaries — no objection to any of them being out of scope.
Resolves the conflict in apps/worker/src/index.ts. Both sides changed how the SyncEngine is constructed: - This branch extracts it into apps/worker/src/build-sync-engine.ts, which owns the prisma/createJob coercion and the plugin registration. - main kept it inline and, in the same import block, added handleGithubReactionPoll (registered at the bottom of the file) and createJob. Kept this branch's `await buildSyncEngine()` — the extraction is the point of the PR, and build-sync-engine.ts already imports prisma, createJob, and SyncEngine itself. Dropped main's inline construction along with its now-unneeded SyncEngine and SyncEngineDeps imports from index.ts. Kept main's handleGithubReactionPoll import, which the GITHUB_REACTION_POLL registration needs. `prisma` stays imported in index.ts — still used by the shutdown path's $disconnect(). Verified on the merged tree: build 10/10, typecheck 10/10, 1,746 tests pass.
…t semantics Answers the three change requests on #95 plus the open question. Typed deps instead of `as never` (build-sync-engine.ts, 5 sites) `as never` silenced the check completely and left no record of the intended contract — worse than the `as any` it replaced, since `never` is assignable to everything and nothing breaks if a contract gains a required member. Each cast now names the contract the PR already exports: StatusMapDb, PriorityMapDb, LabelMapperDb, SyncEngineDeps['prisma'], SyncEngineDeps['createJob'], and IdentityMapperDeps. Same deliberate coercion, but it fails loudly on drift. This carries forward the approach #135 introduced (merged 2026-07-28) into the new module, rather than deleting it. A corrupt config row is no longer indistinguishable from no config (mappings route) `readPersistedConfig` returned null both when the row was absent and when it would not parse, so GET served code defaults and the dashboard rendered them as the saved settings — an admin's configuration appearing to silently revert, with nothing logged. It now returns a discriminated result (absent | corrupt | ok), logs the reason with the config key, and GET reports `configSource` ('persisted' or 'defaults') plus `configError` when the row is unusable. The read path also validates rather than casting. `isValidMappingShape` guarded only PUT, so a row written by an older version of the code — or hand-edited in the database — reached the worker unvalidated. Read now runs the same check against the same `TicketStatus` / `TicketPriority` enum sources the PUT path uses. Boot semantics of the top-level await, documented `await buildSyncEngine()` performs three database reads before the entry module finishes evaluating, so an unreachable database throws during import and the process exits before the health server starts listening — the container crash-loops with no /health rather than coming up degraded. That is the intent (a worker running on silently-defaulted mappings would write wrong statuses to Linear), and the comment now says so, including that #138's /health work makes a degraded-but-listening mode a real option worth revisiting. Conflict resolution (b1bd3a2, kept) Resolved by hand rather than taking either side wholesale, precisely because taking this branch's file would have silently unregistered GITHUB_REACTION_POLL. All four of main's references survive: the header docstring, the import, the concurrencyByType entry, and the worker.on registration. Verified by grep. Tests Four new cases in apps/web/src/__tests__/sync-mappings-config-read.test.ts cover absent, valid, unparseable, and parses-but-wrong-shape. Red-green verified: reverting the corrupt-vs-absent distinction fails the unparseable case. Verified: build 10/10, typecheck 10/10, 1,750 tests pass. CI was already green on the merge commit (b1bd3a2) before these changes.
|
Rebased onto The conflict, resolved by hand. You were right that this was a trap: taking this branch's
Corrupt config is no longer indistinguishable from absent. You also flagged that the read path only cast — fixed. It runs On the top-level await. Fail-fast is the intent and the comment now says so: a worker running on silently-defaulted mappings would write wrong statuses to Linear, which is worse than being visibly down, and Railway's restart policy is the retry. The comment also notes the interaction you spotted — once #138's CI. It had never run on this branch; it has now — green on the merge commit Re-review when you have a moment — the PR is mergeable but blocked on this review. |
The 07-18 review approved with five comments; items 4 (`as never`) and 5 (top-level
await) were handled in the previous commit. These are 1-3, which were never
addressed.
1. Dashboard/worker divergence on empty arrays
`isValidMappingShape` rejected an empty OBJECT but accepted `{ linear: [] }` — the
inner array passed `.every` vacuously. Meanwhile `loadStatusMap` /
`loadPriorityMap` treat `entries.length === 0` as "nothing persisted, use the
hardcoded defaults" (status-map.ts:131). So saving `{ linear: [] }` left the
dashboard showing NO mappings while the worker kept applying Linear's defaults.
Empty per-plugin arrays are now rejected on save, for the same reason the
empty-object case already was: a save must not be able to produce a state where
the UI and the engine disagree about what is in effect.
The read path validates per SECTION rather than all-or-nothing, so a row with
unusable priorityMappings no longer discards perfectly good statusMappings. Each
bad section falls back on its own, is logged by name, and is reported to the
caller via `invalidSections` so the dashboard can mark it as defaults.
2. Sequential enqueue in force-sync
`POST /api/sync/force` issued 2xN sequential inserts, a long round-trip chain that
could brush the route timeout on a large workspace. Now enqueued via Promise.all.
Deliberately Promise.all and NOT allSettled: a failed enqueue must still propagate
so the route 500s. I first wrote this as a 207 partial-success response and that
was wrong — it contradicted behaviour this review explicitly praised (narrowing
the try/catch so a DB failure surfaces as 500 rather than a disguised 400) and
pinned by "does not mislabel a mid-loop DB/queue error". Reverted.
3. Force-sync is not atomic
Confirmed as a conscious choice and documented rather than changed. If one insert
fails, jobs already enqueued stay and a retry re-enqueues from scratch. That is
tolerable here because TRACKER_SYNC pushes current ticket state, so a duplicate is
a no-op in effect, and this is a manual admin action rather than an automated path.
Test fixtures
Nine `priorityMappings: { linear: [] }` and six `statusMappings: { linear: [] }`
fixtures encoded the state item 1 now forbids; they carry real mappings now. One
of them mattered beyond bookkeeping: "rejects labelRules of the wrong type" had an
empty statusMappings, so it returned 400 for the wrong reason and would have
passed even with labelRules validation deleted.
New tests: an empty per-plugin array is rejected on save (red-green verified —
removing the check fails it), and the read path keeps the valid section while
naming the invalid one.
Verified: typecheck 10/10, 1,751 tests pass.
|
Reviewed at The dead-adapter catch is the valuable part of this PR — I confirmed against That said, Blocker — saving the dashboard defaults silently breaks all inbound Linear priority mapping
But priority: this.mapPriorityToOutpost(String(data.priority ?? '0')),— i.e. So an admin who edits one status row and hits Save persists a Worth noting Fix: persist Also still open
Writer and readers validate differently.
Two new wrinkles introduced by d57aac8
Related: Unbounded TestsCoverage is genuinely strong — ~20 new cases including negative paths, the 400-vs-500 mislabel, first-force-sync-with-no-history, and the empty-config rejection. The one gap is a round-trip test: Smaller notes
SecurityNo concerns. VerdictArchitecture is right and the direction is good. I'd block on the priority-key mismatch (silent data-correctness bug on the ordinary "open the page, save" path) plus the round-trip test that pins it, and ideally the |
…eview
BLOCKER — saving the dashboard defaults broke all inbound Linear priority mapping
`DEFAULT_PRIORITY_MAPPINGS` served '0 (None)'..'4 (Low)' as the persisted KEYS,
but LinearAdapter maps with `String(data.priority ?? '0')` -> '0'..'4' and
`createLinearPriorityMap()` is keyed '0'..'4'. The dashboard PUTs back whatever
GET served, so one save from the page persisted a PriorityMap no inbound webhook
could ever match: every Linear priority fell through to MEDIUM from then on,
permanently, with nothing logged.
Keys are now the raw values the adapter sends, and the human text moved to a
display-only `label` field the loaders ignore. DEFAULT_STATUS_MAPPINGS already
matched its factory exactly, which is why priority was the odd one out and did
not show up on a read-through.
Round-trip test — the structural fix
apps/web/src/__tests__/sync-mappings-roundtrip.test.ts GETs the defaults, PUTs
them back, then loads the result through the same loadStatusMap/loadPriorityMap/
loadLabelMapper the worker uses and asserts equivalence with the factories.
Red-green verified against BOTH bugs independently: restoring '1 (Urgent)' fails
it, and dropping the exclude carry-over fails it.
loadLabelMapper no longer drops `exclude`
Rebuilding from persisted rules returned `new LabelMapper({ rules })`, silently
disabling GitHub's wontfix/duplicate/invalid exclusions on the first save. It now
inherits the factory's list via a new `getExcludeList()` accessor.
Writer and readers now validate the same thing
`isValidMappingShape` picked its key pair via `'externalStatus' in record`, so a
priority-shaped row inside statusMappings passed PUT and was then discarded by
loadStatusMap — accept-then-discard, one layer down from the bug this PR fixes.
It now takes the expected key pair explicitly, and rejects blank keys that the
loaders' truthiness checks would drop.
Two regressions from d57aac8, mine
- `configSource` lied when the whole row was unusable: `status: 'ok'` whenever the
JSON parsed meant a row with every section invalid reported 'persisted' with no
configError — the exact "defaults presented as saved settings" case the field
exists to prevent. It now derives from whether any section survived, and the
result-type doc comment describes the code again.
- `{...candidate, statusMappings: undefined} as PersistedMappingConfig` asserted
over a non-optional field. Those fields are optional now and the strip uses
`delete` rather than a cast that made the type false.
Bounded enqueue instead of unbounded
An unbounded `Promise.all` over 2N inserts traded a route timeout for Prisma
pool-acquisition timeouts — same outage, less obvious error. Now chunks of 50,
still `Promise.all` within a chunk so a failure propagates as a 500 (pinned by
"does not mislabel a mid-loop DB/queue error"), still documented as non-atomic by
choice.
configSource / configError / invalidSections are now read
They were computed and consumed by nothing, so the page still presented defaults
as saved settings. The mappings page surfaces both a "showing built-in defaults"
notice and a per-section "using defaults for X" notice.
One read, three loaders
buildSyncEngine fired three `systemConfig.findUnique` calls for the same row on
every boot. A request-scoped read-once facade collapses it to one, leaving the
loaders' signatures and independent fallbacks untouched. Pinned by a test.
Silent non-registration now logs
`initializeSyncEngine` skipped Linear registration when `identityDeps` was absent
even with both env vars set — the same invisible non-registration this PR was
written to fix. It logs loudly now. Also corrected `InitOptions.deps`, which was
doc-commented "testing" despite being required in production.
Smaller
Dropped the merge archaeology from apps/worker/src/index.ts, keeping the
boot-semantics paragraph. Force route selects only id/status/priority instead of
whole ticket rows (assertion updated to match).
Test-fixture note: the build-sync-engine test asserted the loaders were called
with `prisma` by identity; they now receive the read-once facade, so it asserts
the contract (plugin + a usable systemConfig.findUnique) plus the single-read
behaviour instead.
Verified: build 10/10, typecheck 10/10, 1,756 tests pass.
Deferred, with reasoning: exporting MAPPING_CONFIG_KEY and a shared
parseMappingConfig() from packages/outpost/shared/src/sync/ (declared in four
files today). It is the right consolidation and would subsume the validation
divergence, but it touches all four call sites and their tests — better as its own
PR than bolted onto this one.
|
Addressed at The blocker. Confirmed the chain: The round-trip test. Added and it is the useful one, exactly as you predicted: GET defaults → PUT them back → load through
Validation divergence. Both regressions you caught in Unbounded The three unread fields. Wired rather than dropped: the mappings page shows a "showing built-in defaults" notice and a per-section "using defaults for X" notice. Also done: one One deliberate deferral. Exporting Also taking the CLAUDE.md note — the review-loop rule shouldn't hard-code a dependency an external contributor can't satisfy. I'll soften it separately so it doesn't ride on this diff.
|
|
@jerelvelarde ready for re-review — all four items from the 2026-07-28 review are addressed. Verified against the branch rather than just pointing at commit messages: Conflict with Corrupt config was indistinguishable from absent.
Top-level await boot semantics. Documented at the call site rather than left implicit. It records that this is a deliberate change, that fail-fast is the intent (a worker with silently-defaulted mappings writes wrong statuses to Linear, which is worse than being visibly down), that Railway's restart policy is the retry mechanism, and that the decision is worth revisiting once #138 makes On your suggested ordering: #135 landed first as you recommended, so its typed-deps approach carried into CI is green ( Worth noting for scope: the sync-engine issues in #139 — echo detection querying the reverse plugin pair, and inbound changes being discarded while reporting success — are pre-existing and not addressed here. This PR fixes the dead adapter registration and the mappings persistence; the echo-loop correctness work is separate. |
…looding
Two data/queue-damaging paths that the 501 stub previously made unreachable.
Both became live when POST /api/sync/force started enqueuing real work.
Force-sync pushed statuses that have no reverse mapping
StatusMap.fromOutpost falls back to the FIRST entry of its config when an
Outpost value has no reverse mapping. TicketStatus has six values and
createLinearStatusMap covers four, so WAITING_ON_CUSTOMER and WAITING_ON_TEAM
both resolved to 'Triage'. Because the route enqueued status_change for every
link unconditionally, one "Force Linear" click moved every waiting ticket's
Linear issue to Triage and recorded each as a successful sync — a silent, bulk,
hard-to-reverse write with nothing logged.
The route now loads the same StatusMap/PriorityMap the worker uses and enqueues
only values that round-trip, via the existing hasOutpost() accessors. Skipped
changes are reported in the response (`skipped` count plus a distinct
`unmappable` list) and logged once, rather than dropped quietly — an operator
who force-syncs needs to know which values have no mapping so they can add one.
Priority is fully covered by the Linear defaults, but a persisted custom map
need not be, so it gets the same guard.
Extending createLinearStatusMap to cover the waiting states would also silence
this, but picking a Linear state for "waiting on customer" is a product
decision, not a code one. Skipping is the safe default until that call is made.
Force-sync accepted plugins with no registered adapter
github-app writes SyncEvent and TicketExternalLink rows, so 'github' cleared the
route's known-plugin probes — while buildSyncEngine registers Linear only. The
dashboard rendered a "Force Github" button from the same plugin list, so one
click enqueued 2N jobs that each failed "Plugin is not registered", exhausted
their retries, and landed in the DLQ.
Adds OUTBOUND_SYNC_PLUGINS + supportsOutboundSync() to the shared sync package,
next to the initializeSyncEngine() registration it mirrors, with a doc note on
both sides that the two must move together. Three call sites use it:
- the force route rejects a non-syncable plugin with 400, checked AFTER the
404 probes so the cases stay distinguishable (404 = no such plugin,
400 = real plugin, no outbound adapter)
- /api/sync/status serves canForceSync per system
- the dashboard renders the force button only for plugins that have it
Resolved server-side rather than in the client because duplicating the
capability list into a client component is how it would drift — the same
four-copies-of-MAPPING_CONFIG_KEY problem already flagged on this PR.
Tests
Four new cases, red-green verified: defeating either guard fails exactly the
three behavioural tests and nothing else.
- a WAITING_ON_CUSTOMER ticket contributes its priority job but not its
status job, and names the value in `unmappable`
- repeated unmappable values collapse to one entry rather than one per ticket
- a known-but-unregistered plugin is refused before any job is enqueued
- /api/sync/status marks github false and linear true
Three existing force-sync assertions updated for the widened response body.
Verified: typecheck 10/10, build 10/10, 1,760 tests pass.
Note: `pnpm lint` fails in @copilotkit/outpost, pre-existing and unrelated — the
repo ships .eslintrc.cjs while the range resolves ESLint 9, which requires flat
config. Reproduces identically with these changes stashed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0148fWn6nmdRiocZfCe5w6rW
Five findings from a review pass over the previous commit. Two of them were fresh instances of criticisms already made against this PR. Share one config read instead of two (S1) loadStatusMap and loadPriorityMap each look up the same sync.mappingConfig row, so the force route was issuing two identical queries per request — the exact pattern flagged on this PR when buildSyncEngine fired three reads for one row. singleReadConfigDb moves out of apps/worker/src/build-sync-engine.ts into packages/outpost/shared/src/sync/config-cache.ts and is exported, so the worker and the route share one implementation rather than the route growing a second copy. Pinned by a new test. The worker's test mock now spreads importActual so singleReadConfigDb resolves to the real function. Stubbing it would have made that test's existing "reads the row once" assertion vacuous. Surface the skip report (S2, S3) The route returned `skipped` / `unmappable` and handleForceSync discarded the whole response — the same computed-but-never-read shape as configSource / configError / invalidSections. Left as-is it would have traded a silent wrong write for a silent no-write, which is a smaller version of the bug the previous commit set out to fix. The dashboard now shows which values were skipped and why, and checks res.ok so the new 400 is visible rather than a no-op. Smaller (S4, S5) - `action` narrowed from `string` to the union it actually holds. - Early return when nothing is linked, so a no-op resync loads no mapping config at all. Pinned by a new test. Verified: typecheck 10/10, build 10/10, 1,762 tests pass (+6 over ad4658e). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148fWn6nmdRiocZfCe5w6rW
The review-changes/create-review skills write per-run handoff, findings, and resolution files under .chalk/reviews/sessions/. That is per-developer working state, not source — the durable record of a review belongs in the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148fWn6nmdRiocZfCe5w6rW
…t the route Adversarial review of the previous two commits found the fix was scoped to the wrong layer. It closed the bulk amplifier and left the underlying defect. The route filter was not enough Bulk force-sync is not the only producer of status_change jobs. triggers.ts onTicketUpdated enqueues one per changed field on ordinary ticket updates, which reaches the same LinearAdapter.pushStatusChange -> mapStatusFromOutpost -> StatusMap.fromOutpost fallback. So a ticket moving to WAITING_ON_CUSTOMER during normal operation still silently dragged its Linear issue to Triage — one ticket at a time rather than all at once. Since PR #95 is what registers the Linear adapter in the first place, that path goes live with it. The guard now lives in LinearAdapter.pushStatusChange, which covers every producer. It returns rather than throws: an unmapped status is a configuration gap, not a transient fault, so retrying it to the DLQ would be noise. Worth noting GitHubAdapter.mapStatusFromOutpost already handles all six TicketStatus values with an explicit switch — Linear was the only adapter delegating to the guessing map. The route filter stays, with its role corrected in the comments: it is not the safety net, it is the operator-facing half. Skipping at enqueue time is what lets the response name which values have no mapping instead of queueing jobs that quietly no-op. Dropped the route's priority filter It checked PriorityMap.hasOutpost, but Linear's outbound priority never consults the PriorityMap — pushPriority and pushNewIssue both use outpostPriorityToLinearNumber, an exhaustive switch over all four TicketPriority values. mapPriorityFromOutpost is defined and never called. The persisted priority config is inbound-only until #96 wires it up. So the filter could only ever produce false skips: with a custom persisted priority map missing an entry, it would have withheld a priority change the adapter handles correctly. Removed, with the reasoning recorded at the call site. This also leaves one loader, so the route makes a single config read. Tests Two new adapter cases, red-green verified — both WAITING_ON_* statuses skip the push and warn, and defeating the guard fails exactly those two. A third case asserts every covered status still pushes, so the guard cannot regress into a silent no-sync. Verified: typecheck 10/10, build 10/10, 1,765 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148fWn6nmdRiocZfCe5w6rW
…l registration
supportsOutboundSync('linear') is unconditionally true, but initializeSyncEngine
only registers the adapter when LINEAR_API_KEY and LINEAR_TEAM_ID are both set.
A deployment missing either has no Linear adapter while the gate says otherwise,
so the DLQ flood the list prevents by design is still reachable by
misconfiguration. Recorded rather than papered over: the web app cannot read the
worker's env (separate deployments), so the real fix is the worker publishing
what it registered, which belongs with the #138 /health work.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0148fWn6nmdRiocZfCe5w6rW
|
Re-reviewed at The mappings-route work holds up. What hadn't had a substantive pass from anyone, across four rounds, is the force-sync path — understandably, since the 501 stub made it unreachable until this PR replaced it. Two problems there, both new data/queue-damaging paths this PR opens. I've fixed them in #160, which targets this branch. 1. Outbound status sync silently writes the wrong Linear state
Bulk force-sync is the amplifier: one click pushes every linked ticket's current status, so it moves every waiting ticket's Linear issue to Triage at once. But it isn't only force-sync — Worth noting #160 puts the guard in 2. Force-sync accepts plugins with no registered adapter
Merge order matters here#160 is based on this branch, so please don't merge #95 first — that deletes the base and strands the fix. Suggested sequence:
Step 2 is load-bearing: Filed rather than blocking
Two things worth flagging
Nothing here has been exercised against a real Linear workspace — the Triage behaviour is traced through the code and pinned with mocked red-green tests, not observed live. Given this PR is what makes outbound Linear sync work at all, a manual smoke test on one ticket before merge would be time well spent. Happy to re-review once #160 is in and CI is green here. Generated by Claude Code |
fix(sync): stop bulk force-sync from writing wrong statuses and DLQ-flooding Two blocking findings from the #95 review, plus the results of a self-review and an adversarial pass over the fix itself. - LinearAdapter.pushStatusChange refuses to guess a Linear state for statuses with no reverse mapping, instead of silently resolving WAITING_ON_* to Triage on every producer path (force-sync AND ordinary ticket updates). - Force-sync rejects plugins with no registered outbound adapter, so a "Force Github" click no longer enqueues 2N jobs that dead-letter.
Round-1 CR found eight issues, all in this branch's own code. Four were second-order effects of the labelRules carry-forward. The carry-forward was a read-then-write across two statements, so a concurrent PUT could land between them and lose its rules -- the same wipe the fix exists to prevent, through a narrower door. Read and write now happen inside one prisma.$transaction. An existing row whose labelRules are unusable, or whose JSON will not parse, still cannot be carried forward -- but it now says so. Previously the rules vanished with nothing in the logs, which is the failure shape this endpoint exists to remove. The client stored the request body after a successful save. With the server carrying labelRules forward, echoing the request back dropped rules that were actually persisted -- they disappeared from the editor until the next page load. The PUT response is now the source of truth, falling back to the request body if the response cannot be parsed. The display-only `label` field added for the priority panel was accepted unvalidated; a non-string reached the editor and rendered as garbage. It is now checked when present. The mock fixture still used pre-#95 cosmetic priority keys ('0 (None)') with no label -- the unmatchable-key shape #95 fixed on the persistence side. Leaving it re-canonicalized the bug in the fixture and left the new label rendering uncovered. Keys are now the adapter's real '0'..'4' values with the human text in `label`, which also resolves the docstring that contradicted it. Two weaknesses in the tests added last commit: `not.toEqual` also passes when the value is undefined, so the labelRules assertion is now positive against the defaults the endpoint serves; and console.error spies were restored at the end of each test body, so a failed assertion left console stubbed for the rest of the file -- restoration moved to afterEach. Tests: 18 across the two mapping suites, up from 13. Red-green verified -- reverting the transaction fails the atomicity test, and removing the label check fails the validation test. Call-site enumeration: - prisma.$transaction (new dependency of the mappings PUT): four test files mock prisma for this route. sync-mappings-roundtrip and sync-mappings-config-read were updated first; the full suite then caught sync-api.test.ts, and api-route-auth.test.ts needed the same. All four now provide $transaction and the mocks it calls. This is the enumeration step working after I initially ran it too narrowly -- on the route's callers rather than on every prisma mock that reaches it. - isValidMappingShape: two callers (statusMappings, priorityMappings) plus the read path; the label check is additive and cannot reject a previously valid entry, since `label` was never part of the accepted shape. - readPersistedConfig: still used by GET; the PUT path now inlines its own transactional read rather than calling it, so its behaviour is unchanged. Verified: typecheck 10/10, tests 10/10, build 10/10.
What this does
Two things on the
/syncdashboard used to just return 501 ("not implemented"). Now they actually work:While digging into why the mapping edits weren't taking effect, found a bigger issue: the worker process never registered a Linear adapter at all. All the code for pushing changes to Linear was there — the adapter, the webhook receiver, the dashboard — but the piece that wires the adapter into the running worker was missing. So outbound sync to Linear has been silently broken this whole time, regardless of any dashboard settings. That's fixed here too.
Also fixed along the way
Ran this through a full review pass (three rounds) before opening the PR, which caught a handful of real bugs:
Also added a rule to
CLAUDE.md: run the internal CR review before pushing, going forward — this PR is the reason why (it caught real bugs that would've shipped otherwise).What's intentionally NOT done here
Testing
515 + 748 + 1 = 1264 tests passing across the three packages touched, full build green, CI green. Also caught and fixed one thing during the pre-push check: a Next.js 15 quirk where route files can't export anything besides the HTTP handlers.