G2 S4: hardening — bounded journal scan, node-token guard, doc/test follow-ups#158
Merged
Conversation
fetch_pipeline_events(topic="orders.status") in the stuck-orders worklist (ops.py) and R1 reconciliation (reconciliation.py) had no limit — an unbounded full-table scan of the pipeline_events journal, an OOM/DoS risk at production scale (fine at demo scale today). Both now pass newest_first=True + a bounded journal_scan_limit() (env-tunable via AGENTFLOW_OPS_JOURNAL_SCAN_LIMIT, default 20k rows), and the "latest row per key" dict-building logic is flipped to "first row seen per key wins" to match the new descending scan order. A safety net, not a functional change — demo-scale data is far under the default cap either way. G2 audit m13.
…n notes
Four small hardening items from the G2 audit (n4), none blockers:
1. node/config.py: resolve_node_config now fails fast if AGENTFLOW_NODE_TOKEN
equals the public demo API key (DEMO_API_KEY, default "demo-key") — that
key is published in the demo docs, so reusing it as the node-federation
bearer token would let any public demo caller also authenticate as
node-to-node federation. Covered by new tests in test_node_config.py.
2. node/ingest.py: documented-as-deferred. Idempotency is a Python
check-then-act filter (_existing_event_ids), not a DB constraint — safe
today because _INGEST_LOCK already serializes every write in the single
center process (no concurrent-writer scenario exists). A DB backstop is
also not a clean fit: the idempotency scope is narrower than the whole
table (event_id unique only within topic IN ('events.validated',
'events.deadletter')), and DuckDB has no partial/filtered unique index
support, so a table-wide UNIQUE(event_id) would over-constrain the
journal. Revisit if a multi-center topology is ever built.
3. query/engine.py (committed alongside m13 in the previous commit):
documented the tenant_id=None invariant on fetch_pipeline_events
explicitly, and cross-referenced it in ops.py's _resolve_tenant_id —
confirmed the only reachable path is AUTH_DISABLED dev/demo mode, not an
oversight.
4. clickhouse_backend.py: documented-as-deferred. No `branch` column on
ClickHouse's pipeline_events (unlike the DuckDB embedded schema, ADR 0012
N4) because the three-node demo's node-ingest path is DuckDB-only today
(clickhouse_sink=None) — no ClickHouse caller needs it yet. Comment notes
the additive ALTER-loop pattern to use if that ever changes.
G2 audit n4.
…fanout §10 (edited in the recently-merged S3/n2 fix) stated verbatim that no generator or seed performs FX conversion at runtime and every branch seeds in ₽ — contradicted by postgres_oltp/fanout/02_seed.sql, which seeds the dxb branch's orders with currency='AED' on purpose (a CDC/multi-currency replication fixture proving the fan-out carries a real per-row currency column through CDC). No runtime bug: 04_ch_bridge.sql just replicates rows per-branch preserving the currency tag, nothing sums AED+RUB into one figure. Reworded to scope the ₽-only claim to the main vault seeds (synthetic_seed.sql, postgres_oltp/seed.sql) and explicitly carve out the fanout fixture as an intentional exception, converted to ₽ only in doc/comment prose via the §10 FX constants, never at runtime. G2 audit B1 (follow-up to the S3/n2 fix).
The two DDL tests used to assert "x5__" not in raw as a negative guard
against the old retail-dataset record_source prefix reappearing in header
comments; removed during X5 removal (S2b) because keeping the literal
string "x5__" in test source would itself violate the no-X5-anywhere rule.
That left the invariant "the DDL's record_source example prefixes only use
known-good conventions" untested.
Restored brand-neutrally: extract the prefix tokens (e.g. 1c__, mp__,
pg_ops__) named in each file's "source convention"/"source-agnostic"
header-comment example, and assert they're all within an explicit allowlist
{1c__, mp__, pg_ops__, wms__, ref__} — the conventions actually used across
warehouse/agentflow/dv2/business_vault/ and the postgres DDL, confirmed by
inspection. Catches a reintroduced bad prefix without the test ever naming
it.
G2 audit B2.
…e path vault_rows.py is a production module (warehouse/agentflow/dv2/loaders/) that nothing in the shipped code imports — only tests/unit/test_dv2_postgres_ingestion.py does. It was relocated here during X5 removal (S2b) to carry generic vault-row Pydantic models formerly in the deleted X5 loader's schemas.py. Confirmed no other importer exists (grep across the repo); left in place rather than moved to tests/ (less invasive — warehouse/agentflow/dv2/* is already the established convention other test files import DDL/generation helpers from, e.g. dialects.py, generate_satellites.py; mypy's src/-only scope doesn't touch warehouse/ either way). Reworded vault_rows.py's module docstring and the test file's module docstring / ORDER_FEED_TABLES comment, which described a Python Postgres per-branch-order-feed write path that no longer exists — the real feed today is the SQL script promote_to_raw_vault_pg.sql (covered separately, further down the same test file). vault_rows's models exist only for (a) DDL-coverage/column-shape pinning and (b) generic fixture shapes for PostgresVaultWriter's own unit tests, not because the order feed calls the writer. G2 audit B3.
DORA Metrics
MTTR note: No failed mainline CI runs in the selected window. |
3 tasks
brownjuly2003-code
added a commit
that referenced
this pull request
Jul 6, 2026
The bounded journal scan added in S4 (PR #158) ordered by `{time_column} DESC` with no secondary key, unlike the existing ascending path's `ASC, event_id ASC` total order. Two rows sharing an identical timestamp for the same entity resolved to whichever the backend happened to emit first — nondeterministic across backends and across reloads. Add `, event_id DESC` to restore a total order and match the old last-write-wins tie behavior deterministically. Found in an Opus review pass standing in for the unavailable Codex review step. Co-authored-by: JuliaEdom <uedomskikh@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Step S4 of
new_plen_05_07_26.md(G2 audit closure): mechanical hardening, no spec/data judgment calls.Part A — original S4 scope (m13 + n4)
fetch_pipeline_events(topic="orders.status")in the stuck-orders worklist (ops.py) and R1 reconciliation (reconciliation.py) scanned the wholeorders.statusjournal with no bound — an OOM/DoS risk at production scale. Both now usenewest_first=True+ a bounded, env-tunablejournal_scan_limit()(default 20k rows); the "latest row per key" logic is flipped to "first row seen wins" to match. Demo-scale output is unchanged.node/config.pynow fails fast ifAGENTFLOW_NODE_TOKENequals the public demo API key — prevents an operator from accidentally letting the well-known demo key double as the node-federation bearer secret._INGEST_LOCKserializes writes); a DB-level backstop isn't a clean fit because the idempotency scope is narrower than the whole table and DuckDB has no partial/filtered unique index support. Revisit if multi-center is ever built.fetch_pipeline_events(tenant_id=None)is only reachable viaAUTH_DISABLEDdev/demo mode from the ops/stream routers (not an oversight) — added explicit docstring/comment invariant rather than relying on incidental behavior.pipeline_eventshas nobranchcolumn (unlike the DuckDB embedded schema) because the three-node demo's node-ingest path is DuckDB-only today — no ClickHouse caller needs it. Comment documents the additive pattern to add it later.Part B — three follow-ups from prior merged steps
docs/generator-spec.md§10 overclaimed "all seeded amounts are ₽ in every branch" — contradicted bypostgres_oltp/fanout/02_seed.sql, an intentional CDC/multi-currency fixture (dxb seedscurrency='AED'). Reworded to scope the claim to the main vault seeds and explicitly carve out the fanout exception.record_sourceprefix allowlist guard in the two DDL tests (dropped during X5 removal because the literal bad string couldn't stay in test source). Now checks header-comment source-convention examples against an explicit allowlist (1c__,mp__,pg_ops__,wms__,ref__) without ever naming the retired prefix.warehouse/agentflow/dv2/loaders/vault_rows.py— confirmed it's test-only (sole importer istest_dv2_postgres_ingestion.py); left in place (less invasive, matches existing convention) and reworded its docstring + the test's stale docstring, which described a Python order-feed write path that no longer exists (the real feed ispromote_to_raw_vault_pg.sql).Test plan
ruff check src/ tests/ scripts/— cleanruff format --check src/ tests/ scripts/— cleanmypy src/ --ignore-missing-imports— clean, 124 filespytest tests/unit/ tests/property/— 1663 passedpytest tests/integration/ -m "not live"— 264 passed, 51 skipped, 4 errors (Docker-daemon-unavailable on this Windows host, pre-existing/unrelated to this diff)test_stuck_orders.py,test_exceptions_inbox.py,test_tenant_isolation.py,test_pipeline_events_scan.py,test_node_config.py,test_node_topology.py,test_clickhouse_backend.py,test_dv2_business_vault_ddl.py,test_dv2_postgres_ddl.py,test_dv2_postgres_ingestion.py— all green🤖 Generated with Claude Code