Skip to content

Accept null SPM provenance in Stage 12 aggregate artifacts - #787

Merged
anth-volk merged 3 commits into
mainfrom
fix/stage12-null-spm-provenance
Oct 14, 2026
Merged

anth-volk merged 3 commits into
mainfrom
fix/stage12-null-spm-provenance

Conversation

@anth-volk

@anth-volk anth-volk commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #786

Same-Repository Draft PR Check

  • This PR is a draft.
  • This PR is from a branch in PolicyEngine/policyengine-sim-api, not a personal fork.

Summary

Stage 12 rejected every aggregate report without an SPM calculation, which includes every UK report. Between 6 and 13 October 2026 this caused 42 of the 153 production Stage 12 failures. All 42 were UK nationwide reports whose baseline and reform simulations had already succeeded.

  • Cause: SingleYearMacroOutput.spm_provenance is optional and defaults to None, so a report with no SPM calculation serializes "spm_provenance": null. AggregateReportArtifactPayload.validate_spm_provenance checked only that the key was present. It then validated null as an SPMComparisonProvenance object, which failed inside Stage12ArtifactStore.write_aggregate. The coordinator recorded the failure as the generic Stage 12 report coordination failed.
  • Fix: the validator now validates spm_provenance only when a value is supplied. An absent or null field is accepted. Every non-null value is still validated, so malformed or mismatched provenance records are still rejected.
  • Why the tests missed it: the coordinator tests' artifact double, FakeArtifacts.write_aggregate, saved any payload without validating it. It now writes through the real Stage12ArtifactStore, so the coordinator tests enforce the same aggregate contract as production.
  • Deployment: the change is in the contract library used by the Stage 12 coordinator. It takes effect with a stage12-deploy.yml run and needs no change to the selected worker version. The other changes are test-only.

Testing

  • Rebased on main after Release Stage 12 database sessions and keep original failures visible #789 and Add a rules registry that makes every enforced and unenforced rule visible #752. The three coordinator tests Release Stage 12 database sessions and keep original failures visible #789 added now use _aggregate_payload too, because one of them aggregates successfully and the real store rejects {"result": "complete"}.
  • New contract tests in libs/policyengine-simulation-contract/tests/test_stage12_execution.py:
    • An absent spm_provenance and a null one are both accepted.
    • Present but invalid values ({}, a string, a list, 0) are rejected.
  • New executor test in projects/policyengine-simulation-executor/tests/test_stage12_runtime.py:
    • It builds a UK report aggregate with build_aggregate_report, using the real SingleYearMacroOutput serialization.
    • It writes that aggregate with FakeArtifacts.write_aggregate, which now calls the real Stage12ArtifactStore.write_aggregate over an in-memory object store.
    • It checks that the stored artifact keeps spm_provenance: null and matches the returned digest.
  • The coordinator test double now validates aggregates:
    • FakeArtifacts.write_aggregate delegates to a real Stage12ArtifactStore, so it runs the same AggregateReportArtifactPayload validation and encoding as production. The stored aggregate path now follows the production layout under the run's artifact prefix, and one coordinator test checks that path.
    • With validation on, 10 coordinator tests failed because their stand-in aggregators returned payloads that never met the contract, such as {"result": "complete"}. They now return a valid payload from a _aggregate_payload helper, with the same result as before.
    • The stand-in aggregators that must not run also return a valid payload. If one ran, its aggregate would be saved and the existing aggregate_writes == [] checks would fail, rather than a validation error hiding the run.
  • Shared test support, following docs/engineering/skills/testing.md:
    • fixtures/fake_object_store.py holds InMemoryObjectStore. It replaces the two identical copies in test_stage12_artifacts.py and test_stage12_runtime.py.
    • fixtures/macro_output.py, renamed from fixtures/test_simulation_api_contracts.py, holds the shared macro result and its key sets. The Stage 12 runtime, output builder and standalone app tests no longer import the API contract tests' fixture module.
  • The new tests reproduce the defect. With the previous validator, the null-provenance contract case and the UK aggregate test both fail with ValidationError. In a one-off check with the previous validator, a coordinator run whose aggregate carries spm_provenance: null also failed through the validating double, recording ValidationError. The absent-field and invalid-value cases pass both before and after, as guards should.
  • Suites, on Python 3.13 with uv run --frozen:
    • libs/policyengine-simulation-contract: 153 passed.
    • projects/policyengine-simulation-executor, all of tests/: 1058 passed, 22 skipped, 3 deselected (the integration marker).
    • Of those, tests/test_stage12_runtime.py and tests/test_stage12_artifacts.py: 54 passed.
  • Formatting and lint: with CI's pinned ruff 0.15.22, ruff format --check, ruff check and git diff --check pass on every changed file.
  • Replay of a failed report: the coordinator's aggregate for one failed production UK report was rebuilt from its saved simulation outputs. It failed validation before this change and validates after it.

🤖 Generated with Claude Code

@anth-volk
anth-volk force-pushed the fix/stage12-null-spm-provenance branch from f7525ee to 8278e12 Compare October 14, 2026 17:17
anth-volk and others added 3 commits October 14, 2026 23:49
Reports without an SPM calculation, including every UK report, serialize
the optional spm_provenance field as null. The aggregate validator checked
only that the key existed and then validated null as an
SPMComparisonProvenance object, so every such aggregate was rejected
before it was saved.

Validate the field only when a value is supplied. Malformed provenance
records are still rejected.

Fixes #786

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Stage 12 artifact and runtime tests each defined the same in-memory
object store, and the runtime tests imported the macro result from the
API contract tests' fixture module. Move the object store to
fixtures/fake_object_store.py and the macro result and its key sets to
fixtures/macro_output.py, and import both from there in every test that
uses them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FakeArtifacts.write_aggregate encoded whatever it was given, so no
coordinator test enforced AggregateReportArtifactPayload, and the null
SPM provenance rejection reached production without a failing coordinator
test. Write aggregates through the real Stage12ArtifactStore over the
shared in-memory object store instead, so the coordinator tests run the
same validation and encoding as production.

The coordinator tests' stand-in aggregators returned payloads such as
{"result": "complete"} that never satisfied the contract. They now return
a contract-valid payload with the same result. Aggregators that must not
run return one too, so an unexpected run is still caught as a written
aggregate rather than hidden by a validation failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@anth-volk
anth-volk force-pushed the fix/stage12-null-spm-provenance branch from 25c1ee0 to 0951722 Compare October 14, 2026 19:50
@anth-volk
anth-volk force-pushed the fix/stage12-null-spm-provenance branch from 0951722 to d29ce9d Compare October 14, 2026 19:56
@anth-volk
anth-volk marked this pull request as ready for review October 14, 2026 20:13
@anth-volk
anth-volk merged commit fc1ddca into main Oct 14, 2026
14 checks passed
@anth-volk
anth-volk deleted the fix/stage12-null-spm-provenance branch October 14, 2026 20:13
anth-volk added a commit that referenced this pull request Oct 14, 2026
#787 made the coordinator tests' artifact double write aggregates through
the real Stage12ArtifactStore, which rejects anything that is not a valid
AggregateReportArtifactPayload. Use _aggregate_payload for this PR's
aggregator stubs, as #787 did for main's. The redacted comparison-failure
test needs a valid aggregate for the report to succeed. An aggregator that
must not run now shows up as a written aggregate if it does run, instead of
as a rejection hidden by the failure the test expects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk added a commit that referenced this pull request Oct 14, 2026
#787 made the coordinator tests' artifact double write aggregates through
the real Stage12ArtifactStore, which rejects anything that is not a valid
AggregateReportArtifactPayload. Use _aggregate_payload for this PR's
aggregator stubs that must not run, as #787 did for main's, so an
unexpected run shows up as a written aggregate instead of as a rejection
hidden by the failure the test expects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk added a commit that referenced this pull request Oct 14, 2026
#787 made the coordinator tests' artifact double write aggregates through
the real Stage12ArtifactStore, which rejects anything that is not a valid
AggregateReportArtifactPayload. Use _aggregate_payload for this PR's
aggregator stubs that must not run, as #787 did for main's, so an
unexpected run shows up as a written aggregate instead of as a rejection
hidden by the failure the test expects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk added a commit that referenced this pull request Oct 14, 2026
#787 landed on main after this stack branched. It renamed the macro result
fixture module to fixtures.macro_output and added fixtures.fake_object_store,
and those imports in the output builder, Stage 12 artifact and Stage 12
runtime tests are unsorted under the I rule this PR enables.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk added a commit that referenced this pull request Oct 14, 2026
…n class (#791)

* Record a redacted Stage 12 failure cause instead of only the exception class

Stage 12 coordinator, worker and result-comparison failures stored only
type(error).__name__, so a database session limit, a UK aggregate
validation defect and an invalid reform path all appeared as RuntimeError
or ValidationError behind "Stage 12 report coordination failed".

Failure records now store a bounded summary of the form
"<type> in|after <stage> (<role>): <message>", capped at the 512-character
error_summary limit. Messages are redacted first: URL credentials, signed
URL query strings, secret-looking assignments, bearer and known token
formats, this process's secret environment values, Pydantic input values
and SQL statement parameters are removed. Stage tracking follows the
existing registered span names, so the summary names the stage that failed
or the last stage that finished before a failure between spans.

The coordinator no longer overwrites a cause that the failed worker already
persisted, and copies that cause onto the report record. The redacted
cause, with the original stack, is also recorded on the active operation
before the failure is persisted. The exceptions returned to Modal are
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Redact more credential formats in Stage 12 failure causes

Redact quoted JSON and dictionary keys, non-Bearer Authorization schemes
and cookie headers, signed query strings without a URL scheme, and the
row values that PostgreSQL constraint violations echo in their detail,
from both the stored summary and the exported cause.

Patterns that could start inside a run of word characters now start
only where that run starts, so redaction is linear in the message
length. Redaction also reads at most 4,096 characters: the word that
limit cuts is dropped, and quoted values, SQL parameters and validation
inputs whose closing delimiter was cut are redacted to the end.

Take the summary length from the contract's ErrorSummary, detect
SQLAlchemy errors by type, and state in the module docstring that span
exception events are outside this redaction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Share one stage helper for Stage 12 spans and failure progress

Replace the coordinator's stage_scope and stage_span closures and the
worker's _simulation_stage with tracked_stage, which opens a stage's
span or operation and records it as the process's progress. The
coordinator claim span uses it too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Test dispatch and comparison failure summaries in Stage 12

Assert the redacted summary stored on a child and its report when Modal
rejects a dispatch, and the comparison_error_summary stored when the
production result cannot be read. Reword fixture failure messages that
described themselves as sensitive, since the tests now assert those
messages are stored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Return valid aggregates from the Stage 12 test aggregators

#787 made the coordinator tests' artifact double write aggregates through
the real Stage12ArtifactStore, which rejects anything that is not a valid
AggregateReportArtifactPayload. Use _aggregate_payload for this PR's
aggregator stubs, as #787 did for main's. The redacted comparison-failure
test needs a valid aggregate for the report to succeed. An aggregator that
must not run now shows up as a written aggregate if it does run, instead of
as a rejection hidden by the failure the test expects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Anthony Volk <14987227+anth-volk@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stage 12 rejects aggregates whose optional SPM provenance is null

1 participant