fix: admit Maker orchestrator strategy archive batches - #109
Conversation
WalkthroughThe archive-forwarder now accepts strategy envelopes from ChangesStrategy archive ingestion
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant MakerOrchestrator as Maker orchestrator
participant ArchiveForwarder
participant StrategyContract
participant DurableStrategySpool
MakerOrchestrator->>ArchiveForwarder: Submit archive envelope
ArchiveForwarder->>StrategyContract: Classify source and rows
StrategyContract-->>ArchiveForwarder: Accept strategy batch
ArchiveForwarder->>DurableStrategySpool: Queue strategy batch
ArchiveForwarder-->>MakerOrchestrator: Return HTTP 202
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/archive-forwarder/strategy-contract.ts (1)
64-71: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External · Exploitability: Moderate
Reachability path
● Entry test/archive-forwarder-strategy-contract.test.ts │ ▼ ● Sink services/archive-forwarder/strategy-contract.tsBind
maker_orchestratorto producer authentication.The forwarder checks only one optional bearer token, while
sourceremains caller-controlled. If the token is shared—or unset—an authenticated or unauthenticated caller can submit validmaker_orchestratorrows todependencies.spool.admit. Require producer-specific credentials or inject the source from a trusted boundary before admission.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/archive-forwarder/strategy-contract.ts` around lines 64 - 71, Update the forwarder admission flow around source validation and dependencies.spool.admit so maker_orchestrator rows cannot be authorized solely by the caller-controlled source and shared optional bearer token. Require producer-specific credentials for this source, or overwrite it from a trusted producer boundary before validation and admission; reject requests lacking that trusted binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/archive-forwarder/strategy-contract.ts`:
- Around line 64-71: Update the forwarder admission flow around source
validation and dependencies.spool.admit so maker_orchestrator rows cannot be
authorized solely by the caller-controlled source and shared optional bearer
token. Require producer-specific credentials for this source, or overwrite it
from a trusted producer boundary before validation and admission; reject
requests lacking that trusted binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe27e22-f0da-45ea-9dc1-bce019d8e53a
📒 Files selected for processing (5)
openspec/specs/strategy-runtime-archive-ingestion/spec.mdservices/archive-forwarder/strategy-contract.tstest/archive-forwarder-strategy-contract.test.tstest/archive-forwarder-strategy-request.test.tstest/fixtures/maker_orchestrator_archive_envelope.json
📜 Review details
🔇 Additional comments (7)
openspec/specs/strategy-runtime-archive-ingestion/spec.md (1)
8-28: LGTM!Also applies to: 66-70, 104-108
test/archive-forwarder-strategy-contract.test.ts (3)
5-9: LGTM!
72-94: LGTM!Also applies to: 145-145, 155-156
59-70: 🗄️ Data Integrity & IntegrationResolve the canonical Maker fixture pin. The test already checks exact bytes for
archive_forwarder_envelope.json.maker_orchestrator_archive_envelope.jsonis a separate fixture and must not equal it. The local SHA-256 pin conflicts with the canonical hash recorded in the repository documentation. Align the test and documentation after resolving the canonical source.test/fixtures/maker_orchestrator_archive_envelope.json (1)
1-35: LGTM!test/archive-forwarder-strategy-request.test.ts (1)
9-9: LGTM!Also applies to: 45-67
services/archive-forwarder/strategy-contract.ts (1)
3-6: 🎯 Functional CorrectnessNo compatibility alias is required. No repository consumer references
STRATEGY_ARCHIVE_SOURCE; current consumers useSTRATEGY_ARCHIVE_SOURCES.> Likely an incorrect or invalid review comment.
Problem
The archive forwarder admits exactly one strategy producer.
strategy-contract.tsdefinedSTRATEGY_ARCHIVE_SOURCE = "hb_runtime", so any strategy-table batch whose envelope sourceis
maker_orchestratorclassified asinvalid_strategy_sourceand was refused with HTTP 400before spooling or storage.
The Maker orchestrator is a real, distinct producer: it stamps
maker_orchestratoras theenvelope source, row source, controller type, and producer id. Since the archive-forwarder
rollout on 2026-08-03, its strategy rows have been rejected outright — production Maker
archive rows stopped at the rollout boundary, with roughly 3,142 archive POST failures
observed over ~23 hours.
Runtime decisions are fire-and-forget, so trading behavior is unaffected; what breaks is
replay completeness and operational evidence — retained state and deltas, settlement checks
and status, shared-funding lineage, transfer lifecycle, and Maker market-health snapshots.
Change
Widen the strategy producer admission set to exactly
{hb_runtime, maker_orchestrator}.STRATEGY_ARCHIVE_SOURCEis replaced by a single canonicalSTRATEGY_ARCHIVE_SOURCESset;no consumer hard-codes a producer string.
request.tsneeded no change, because theclassification value it branches on is unchanged — both producers therefore reach the same
durable strategy-spool path and the same 202 ownership contract.
The producer identity is deliberately preserved rather than normalized. Relabeling Maker rows
as
hb_runtimewould have made replay provenance false, which is worse than the rejection itwould have papered over.
Everything else in the gate remains fail-closed and untouched:
hb_runtimeenvelope cannotcarry a
maker_orchestratorrow, or vice versa;schema_versionrules are unchanged;
broker_read/broker_writetraffic still takes the direct synchronousinsert path;
strategy_data.*tables are unchanged.openspec/specs/strategy-runtime-archive-ingestion/spec.mdis updated to describe thetwo-producer admission set, since it previously mandated hb_runtime-only and would otherwise
contradict the code.
Tests
New
maker_orchestratorfixture built from the Maker orchestrator's actual serialized wireshape (
stream_name,event_kind, andschema_versionverified against the producer'sown wire-contract test) rather than a synthesized guess. The existing shared
archive_forwarder_envelope.jsonis deliberately left untouched — it is byte-pinned here andmirrored by a parity test on the Maker side.
Added coverage:
maker_orchestratorenvelope reaches durable admission(202, no ClickHouse insert, one queued batch);
unknown_runtime,broker_read, andbroker_writecarrying strategy rows;The defect-focused test was demonstrated failing on the unfixed parent (
expected 202, received 400) before the fix was accepted.Verification:
bun test test/archive-forwarder-strategy-contract.test.ts test/archive-forwarder-strategy-request.test.ts— 24 pass, 0 failbiome checkovertest/andservices/archive-forwarder/— cleantsc --noEmit— cleanNotes for review
develop. CEX Broker Data Archival Refinement, and E2E Testing #108 independently refactors the same constant into a source arraywhile adding
maker_replayand splitting the classification enum, so whichever lands secondwill need a small conflict resolution on those lines. The resolution is mechanical either
way: on top of CEX Broker Data Archival Refinement, and E2E Testing #108 this reduces to one additional entry in its source set.
readback are tracked separately.
Summary by CodeRabbit
hb_runtimeandmaker_orchestrator.