Repository navigation
Accept null SPM provenance in Stage 12 aggregate artifacts - #787
Merged
Merged
Conversation
anth-volk
force-pushed
the
fix/stage12-null-spm-provenance
branch
from
October 14, 2026 17:17
f7525ee to
8278e12
Compare
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
force-pushed
the
fix/stage12-null-spm-provenance
branch
from
October 14, 2026 19:50
25c1ee0 to
0951722
Compare
2 tasks done
anth-volk
force-pushed
the
fix/stage12-null-spm-provenance
branch
from
October 14, 2026 19:56
0951722 to
d29ce9d
Compare
anth-volk
marked this pull request as ready for review
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>
2 tasks done
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>
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
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.
Fixes #786
Same-Repository Draft PR Check
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.
SingleYearMacroOutput.spm_provenanceis optional and defaults toNone, so a report with no SPM calculation serializes"spm_provenance": null.AggregateReportArtifactPayload.validate_spm_provenancechecked only that the key was present. It then validatednullas anSPMComparisonProvenanceobject, which failed insideStage12ArtifactStore.write_aggregate. The coordinator recorded the failure as the genericStage 12 report coordination failed.spm_provenanceonly 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.FakeArtifacts.write_aggregate, saved any payload without validating it. It now writes through the realStage12ArtifactStore, so the coordinator tests enforce the same aggregate contract as production.stage12-deploy.ymlrun and needs no change to the selected worker version. The other changes are test-only.Testing
mainafter 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_payloadtoo, because one of them aggregates successfully and the real store rejects{"result": "complete"}.libs/policyengine-simulation-contract/tests/test_stage12_execution.py:spm_provenanceand a null one are both accepted.{}, a string, a list,0) are rejected.projects/policyengine-simulation-executor/tests/test_stage12_runtime.py:build_aggregate_report, using the realSingleYearMacroOutputserialization.FakeArtifacts.write_aggregate, which now calls the realStage12ArtifactStore.write_aggregateover an in-memory object store.spm_provenance: nulland matches the returned digest.FakeArtifacts.write_aggregatedelegates to a realStage12ArtifactStore, so it runs the sameAggregateReportArtifactPayloadvalidation 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.{"result": "complete"}. They now return a valid payload from a_aggregate_payloadhelper, with the sameresultas before.aggregate_writes == []checks would fail, rather than a validation error hiding the run.docs/engineering/skills/testing.md:fixtures/fake_object_store.pyholdsInMemoryObjectStore. It replaces the two identical copies intest_stage12_artifacts.pyandtest_stage12_runtime.py.fixtures/macro_output.py, renamed fromfixtures/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.ValidationError. In a one-off check with the previous validator, a coordinator run whose aggregate carriesspm_provenance: nullalso failed through the validating double, recordingValidationError. The absent-field and invalid-value cases pass both before and after, as guards should.uv run --frozen:libs/policyengine-simulation-contract: 153 passed.projects/policyengine-simulation-executor, all oftests/: 1058 passed, 22 skipped, 3 deselected (theintegrationmarker).tests/test_stage12_runtime.pyandtests/test_stage12_artifacts.py: 54 passed.ruff format --check,ruff checkandgit diff --checkpass on every changed file.🤖 Generated with Claude Code