Skip to content

refactor(app): hold one outcome per check and enrichment report - #323

Merged
kstonekuan merged 1 commit into
Hebbian-Robotics:mainfrom
chiruu12:fix/205-report-outcome
Aug 31, 2026
Merged

refactor(app): hold one outcome per check and enrichment report#323
kstonekuan merged 1 commit into
Hebbian-Robotics:mainfrom
chiruu12:fix/205-report-outcome

Conversation

@chiruu12

Copy link
Copy Markdown
Contributor

Closes #205.

CheckRunReport and EnrichmentRunReport now carry one outcome field instead of three optional ones. The variants are Measured(result), Errored(error) and NotRun(step_not_run), with PublishFailed(result, error) on the enrichment side. outcome has no default, so a report with no outcome is a TypeError at construction and the measured fallback is gone.

status on both is a match over the outcome. No precedence, no fallback. result, error and not_run stay as read-only properties derived from the outcome, which is why 53 reads in app.py and about 100 in the tests did not have to move.

Nothing reads a partially built report. Every reader of TestReport.checks and .enrichments runs after its loop finishes: _check_run_rows, has_errors, summary, _yield_defaults_superseded_by_the_pipeline and the artifact publishing pass. Every other .checks / .enrichments in the tree is App.checks or Manifest.checks, a different attribute. So moving the append to after the outcome exists changes when an entry becomes visible, and nothing observes the difference.

One state I had to keep. At the artifact publishing pass, a publish failure sets error on an enrichment that already holds a result, and _check_run_rows still records that result's labels. Collapsing it to Errored would drop those labels. So it gets a fourth named variant, PublishFailed, which keeps them while the run still reports error.

No test asserted that an empty report is measured. Nothing in the suite constructs either report directly, so there was no such assertion to change.

Validation

  • uv run pytest -q: 1246 passed, 11 skipped, no test file edited
  • uv run ruff check / ruff format / uv run ty check: clean
  • check_runs.status unchanged: 114 rows over a 3 episode corpus run twice, once tripping a critical check and once not, covering measured, passed, failed, error, skipped and superseded. Identical on fbfd553 and this branch, sha256 7ac8466f.

tests/test_run_report_outcome.py pins that neither report can be built without an outcome, that each variant still produces the status it produced before, and that a publish failure keeps its labels.

Copilot AI lite review requested due to automatic review settings August 31, 2026 07:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kstonekuan kstonekuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging. PublishFailed is the part I want to call out: I wrote the issue asserting three variants and you found a fourth by reading the publish pass, which is the one place the old shape genuinely needed two fields set. Collapsing it to Errored would have dropped the labels and no test would have caught it, because nothing asserts labels survive a publish failure. Now one does.

The derived-property move is what made this land in three files instead of thirty. Reads keep working, writes don't compile, which is exactly the right half to break.

Validated on your branch: ruff check, ruff format --check, ty check clean, 1266 passed / 6 skipped, no test file touched except the new one. That last part is the evidence for DoD 3: every status assertion in test_default_checks.py, test_gates.py and test_run_profiles.py held without edits.

I checked the invariant statically, since a runtime TypeError test passes even if the type checker has nothing to say. ty on the bad constructions:

error[missing-argument]: No argument provided for required parameter `outcome`
error[unknown-argument]: Argument `result` does not match any known parameter
error[unknown-argument]: Argument `error` does not match any known parameter

So the old three-field spelling is a type error, not just a convention.

Two notes, neither blocking.

The else Errored(combined_error) branch at the publish failure is unreachable. The loop above it does if enrichment_result is None: continue, so enrichment_run.result is non-None every time you get there, including on a second failing artifact (PublishFailed.result keeps it, which is why the accumulation still works). Defensive rather than wrong, so I'm leaving it.

Measured is frozen but shallow: _apply_gate still mutates result.verdict in place after construction. Correct, and the same as before, but "set once" is about which variant the report holds, not about the result's contents. Worth knowing before someone reads frozen=True as more than it is.

Also grepped docs/, examples/, packages/ and README.md for the old construction spelling: nothing outside app.py builds either report, so there's nothing else to update.

@kstonekuan
kstonekuan merged commit e6009fc into Hebbian-Robotics:main Aug 31, 2026
9 checks passed
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.

CheckRunReport can hold two outcomes at once, or none, and an empty one reports 'measured'

3 participants