Repository navigation
docs(adr): 044-047 — sync engine revitalization, capability traits, canonical models, orchestrator/worker - #648
seanspeaks wants to merge 2 commits into
Conversation
…, orchestrator Four connected ADRs, plus register and cross-link updates. ADR-044 Sync Module Revitalization. packages/core/syncs/ is 1,552 lines that have never been used: unexported, zero consumers, zero tests, and a method that throws ReferenceError on any call. Meanwhile five codebases each built their own sync. Crossbeam's GeneralUtil.diff() does in O(n+m) what core's initialSync() does in ~5n·m, and was never carried back. Consolidates all five, adds tiered storage (stateless passthrough where both endpoints allow it), declarative relationships, conflict precedence, and a canonical/rendering split. ADR-045 Capability Traits. API module Definitions declare nothing about data behaviour, so 044's storage tiers cannot be derived. Adds a closed, verifiable trait vocabulary on capability declarations, authored inline or as an OpenAPI Overlay so vendor specs are annotated rather than forked. ADR-046 Canonical Models. Forkable per-category models with declared provider skew and agent-assisted mapping. ADR-047 Orchestrator/Worker Queue Pattern. Ratifies the 2026-05-14 design. Self-publishing workers trip AWS Lambda recursion detection at 16 invocations; 51 Zoho/Quo integrations stalled at 750 contacts. Stateless reducer, atomic state+outbox, group-FIFO ordering, three durability tiers, XState for Tier 3. Numbered 044-047 to sit above the reconciliation in #644 (028-041) and the existing 042/043. ADR-013's "to be authored separately" now points at the Integration Versioning draft in #620 and the open questions in #647. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AD57K2eA29SxojjgA5prG1
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request, or push a new commit, to trigger a review.
✅ Deploy Preview for friggframework-org canceled.
|
Reviewing the register reconciliation surfaced four ADRs these four should defer to rather than re-argue. No redundancy found — none of 044-047 duplicates an existing or pending ADR — but several cross-references were missing. ADR-047: provider-agnosticism was argued as a standing principle. It is in fact already ratified. ADR-028 Multi-Provider Support is Accepted, and ADR-029 Decouple AWS SDK Dependencies is Accepted and defines the QueueClientInterface port the orchestrator should ride rather than reinvent. Also records the gap: 029 is accepted but unimplemented — Worker.js and queuer-util.js still require @aws-sdk/client-sqs directly on next — so the orchestrator's portability is contingent on 029 landing. ADR-046: the authoring skill is now authored per ADR-036 Skills rather than inventing a shape, and its scope/build/review flow is ADR-037 Agent Pipeline. ADR-044: distinguishes sync-record tombstones from ADR-032 Integration Deletion Data Cleanup, which governs orphaned Entity and Credential rows on teardown. Sync records are a third category that ADR-032's scope should name. 044, 046, 047: the versioning question is pointed at ADR-030 Integration Versioning (#620), the contract ADR-013 defers to, rather than at ADR-013 alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AD57K2eA29SxojjgA5prG1
Redundancy check against the ADRs in #644Read the pending ADRs the reconciliation surfaced. No redundancy — none of 044–047 duplicates an existing or pending ADR. But four cross-references were missing, and one framing was wrong. Pushed in 476e3cc. ADR-047 was arguing a settled pointI wrote provider-agnosticism as "a standing Frigg principle rather than a preference." Understated — it is ratified:
That materially strengthens 047's rejection of Step Functions: not a taste call, a ratified constraint. It also exposes a dependency I had missed. ADR-029 is Accepted but unimplemented — ADR-046 was inventing shapes that already have ownersIts agent-assisted authoring proposed "a skill" and an authoring flow from scratch. Both are governed:
ADR-044 vs ADR-032 — adjacent, not redundantADR-032 Integration Deletion Data Cleanup governs Frigg's own records on teardown: orphaned Versioning now points at the right ADR044, 046 and 047 pointed at ADR-013 for the versioning question. ADR-030 Integration Versioning (#620) is the contract ADR-013 explicitly defers to, and is where that question belongs. All three now reference it. Checked and found clean031 / 042 (OAuth concurrency), 033 (Aurora), 034 (API-key login), 035 (app-init), 038–041 (config/secrets) — no overlap with these four. |
|
|
|
||
| ### 1. Split primitives from engine | ||
|
|
||
| - **`packages/core/syncs/`** keeps `Sync`, `SyncManager`, and the repositories, and **exports them |
| { module: 'qbo', capability: 'accounting.invoice.upsert' }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
where would that code live?
| // @friggframework/extension-sync | ||
| capabilities: { | ||
| 'sync.records.reconcile': { | ||
| surface: 'sync', |
There was a problem hiding this comment.
I checked ADR-20, looks like "surface" is a free text? Should this be an enum?
| capabilities: { | ||
| 'sync.records.reconcile': { | ||
| surface: 'sync', | ||
| spec: { kind: 'json-schema', ref: './specs/sync-config.schema.json' }, |
There was a problem hiding this comment.
what would this spec contain? input and output formats?
| modules: { aspire: { definition: aspire.Definition }, qbo: { definition: qbo.Definition } }, | ||
| extensions: { | ||
| sync: { | ||
| extension: require('@friggframework/extension-sync'), |
| direction: 'push', | ||
| trigger: 'delta', | ||
| precedence: { strategy: 'module', master: 'aspire', fallback: 'fillEmpty' }, | ||
| onSourceDelete: 'archive', |
|
|
||
| ### 5. Fix the constructor gap | ||
|
|
||
| `primaryModule` and `secondaryModule` become explicit, validated constructor parameters. |
There was a problem hiding this comment.
didn't we want to get rid of this concept of primary vs secondary module?
| 'classification', // regular | deposit | intermediate | final — source-derived | ||
| 'amount', 'contractTotal', 'depositAmount', | ||
| 'lines', // economic lines: { ref, description, amount } | ||
| ], |
There was a problem hiding this comment.
Feels weird having to declare Invoice properties in the InvoiceSync configuration. The keys are about the Invoice and not about the syncing mechanism.
| cascade: 'ifMissing', // 'never' | 'ifMissing' | 'always' | ||
| onUnresolved: 'defer', // 'defer' | 'skip' | 'fail' | ||
| }, | ||
| } |
There was a problem hiding this comment.
where would this code live? Can you please add more context on the snippets you present?
| policy among several, and direction becomes explicit: | ||
|
|
||
| ```js | ||
| direction: 'push', // 'push' | 'pull' | 'bidirectional' | 'hub' |
|
|
||
| precedence: { | ||
| strategy: 'module', // 'module' | 'field' | 'recency' | 'custom' | ||
| master: 'aspire', |
There was a problem hiding this comment.
how would master behave if we choose direction bidirectional?
| 'crm.contact.list': { | ||
| surface: 'data-read', | ||
| spec: { kind: 'openapi', ref: './specs/hubspot-openapi.yaml#/paths/~1crm~1v3~1objects~1contacts/get' }, | ||
| implementedBy: { kind: 'api-class-method', ref: 'HubSpotApi.listContacts' }, |
There was a problem hiding this comment.
what is api-class-method? api.js ?
| 'write.upsertByExternalKey': 'idProperty', | ||
| 'write.batchMax': 100, | ||
| 'write.partialUpdate': true, | ||
| }, |
There was a problem hiding this comment.
Isn't that basically duplicating the API spec?
| 'write.batchMax': 100, | ||
| 'write.partialUpdate': true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Should we version the definition as well? What if these traits change?
| x-frigg-traits: | ||
| delta.modifiedSince: true | ||
| read.pageMax: 100 | ||
| ``` |
There was a problem hiding this comment.
I don't understand why we need this
|
|
||
| - Stateless-first becomes achievable rather than aspirational. The engine can *know* when it may | ||
| store nothing, instead of defensively persisting a record per pair forever. | ||
| - Assumptions currently buried in integration code become declared, reviewable, and testable. |
There was a problem hiding this comment.
how can we trust that what's declared is indeed what's implemented?
| matchOn: ['number'], | ||
| skew: { | ||
| aspire: { | ||
| keys: ['billingScheduleSortOrder', 'opportunityId'], |
| hashed: false, // carried and preserved, but excluded from the change hash | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
where would this code live? In the integration?
|
ADR reconciliation needed after Stack A/B — drafted with Claude Code at Sean's request Severity: must-reconcile. Stack B implements parts of ADR-044/045/047 under ADR-063, and Stack A (#686) implements part of the ADR-029 port that ADR-047 §5 depends on. Proposed edits: ADR-044
ADR-045
ADR-046
ADR-047
Merge order: this PR first. #695 then cites 044/047. #686 and #644 settle ADR-029's final text. |
2 participants
Four connected ADRs. They can be read and reviewed independently, but 045 and 047 exist because 044 needs them.
ADR-044 — Sync Module Revitalization
packages/core/syncs/is 1,552 lines that have never been used: not exported fromindex.js, zero consumers, zero tests, andsync.js:74throwsReferenceErroron any call (moduleMap[name]wherenameis never bound — verified by executing it).Meanwhile five codebases each built their own sync and none of it is shared:
faulkners-nursery--friggfastspring--integration-servicefreshbooks--friggquo--friggcrossbeam--integration-serviceThe finding that reframes the ADR: core's
initialSync()matches by nested scan — fourfilter/somepasses plus a fifth.find()inside the loop, with an awaited DB write per record. That's ~5·n·m.GeneralUtil.diff()in the same repo as crossbeam's ownLHSyncManagerdoes it in O(n+m), is instrumented withperformance.mark, and runs at 750,000-record batch scale — and the sync manager next door never calls it.So this is not export-plus-tests. The reconciliation loop is quadratic and the execution model is unbounded in memory; the three codebases that ran sync in production at volume each proved it by not using it.
Also decided here: storage is a negotiated tier, not an assumption. Where the destination upserts on a key we control and the source has delta or events, sync needs no database at all — and the canonical model, rendering split and relationship resolution still earn their keep. Sync objects are valuable without sync records.
ADR-045 — Capability Traits
044 §2 defines that tier ladder and then can't implement it: an API module
DefinitioncarriesAPI,getName,moduleName,modelName,requiredAuthMethods,envand stops. Nothing says whether listing supportsmodifiedSinceor whether writes are idempotent.Adds a closed, versioned trait vocabulary to ADR-020 capability declarations — additive,
surface/spec/implementedBy/dependsOnunchanged. Two authoring paths: inline, or an OpenAPI Overlay (stable 1.0.0) so a vendor's published spec is annotated withx-frigg-traitsrather than forked.Two properties held firm:
unknownis the default and means not declared, never not supported, so a half-annotated library degrades safely; and every trait ships a conformance probe, because a module claimingwrite.upsertByExternalKeythat can't will silently duplicate every record.ADR-046 — Canonical Models
Forkable per-category packages with declared skew. The worked case: Aspire's deposit and final draw are positions in a billing schedule, not invoice types. A unified accounting schema has nowhere to put that, so it gets rounded off and the invoice is short by a deposit with nobody noticing.
The honest counter-argument is in Negative Consequences and is not fully answered: canonical schema maintenance is where unified API vendors spend most of their engineering, funded by subscription revenue, and a half-maintained canonical model is worse than none.
ADR-047 — Orchestrator/Worker Queue Pattern
Ratifies the design from 2026-05-14 rather than proposing a new one. Self-publishing workers trip AWS Lambda recursion detection at 16 invocations; 51 Zoho ↔ Quo integrations stalled at 750 contacts, and one integration had been stuck since 13 April.
reduce(machine, state, event) → (state', actions[]), atomic state+outbox, group-FIFO ordering by instance ID, three durability tiers, XState for Tier 3 only. The orchestrator never publishes to the queue it consumes, so the detector stays enabled and never fires — structural, not an account-level override, which preserves detection of genuine runaway loops.Two consequences called out rather than discovered later: group-FIFO ordering makes a poison message an instance-wide outage until the retry cap routes it to DLQ; and the transactional outbox is the easiest thing here to approximate badly.
Numbering
Claimed 044–047, above the reconciliation in #644 (028–041), the merged 031/042, and 043 in #646. My first draft used 028–031 against a stale local
nextand collided with four open PRs — caught before pushing.Review
Adversarially reviewed by subagents before opening, instructed to falsify rather than confirm. Fixed as a result:
diff()was quoted withaMap.delete(...)— a syntax error — and three undeclared variables. Now faithful to source.useDatabase: trueon the extension bundle contradicted ADR-018's explicit rule that a webhook receiver shouldn't pay for a DB connection.static Configdroppedname; the moduleDefinitionlist droppedmoduleName.v1-readyAPI modules; it's 30.Related
🤖 Generated with Claude Code
https://claude.ai/code/session_01AD57K2eA29SxojjgA5prG1