Skip to content

feat(report): show per-stage durations#204

Open
kushin25 wants to merge 2 commits into
alphacrack:mainfrom
kushin25:kushin25/feat-report-stage-durations
Open

feat(report): show per-stage durations#204
kushin25 wants to merge 2 commits into
alphacrack:mainfrom
kushin25:kushin25/feat-report-stage-durations

Conversation

@kushin25

@kushin25 kushin25 commented Jul 22, 2026

Copy link
Copy Markdown

What this changes

Adds a read-only duration helper over the existing stage timestamps and surfaces final-attempt timing in human, JSON, and Markdown reports. Unknown, malformed, missing, and negative durations remain explicit as unavailable; the persisted manifest schema and report exit-code contract are unchanged.

Closes #200.

The grounding invariant

  • This change does not let unverified commands reach tutorial.md, step_by_step.md, commands.sh, or the demo tape.
  • Sandbox hardening flags in sandbox.py are unchanged.

Tests

  • COLUMNS=200 .venv/bin/python -m pytest tests/ -q — 400 passed.
  • .venv/bin/ruff check . — passed.
  • git diff --check — passed.
  • Regression coverage includes valid, missing, malformed, and negative timestamps, skipped/still-running records, JSON/Markdown output, and manifest-schema preservation.

Prompt changes

  • N/A — this PR does not touch prompts/*.md.

Housekeeping

  • One concern per PR.
  • Type hints and docstrings are present on the new helpers.
  • No stages or boundaries moved.

Notes

Prepared with automation assistance and reviewed against the staged diff; no credentials, deployment, dependencies, prompts, sandbox settings, or generated artifacts were changed.

@kushin25
kushin25 requested a review from alphacrack as a code owner July 22, 2026 01:06

@alphacrack alphacrack left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @kushin25 — the design here is right, and the part I'd have worried about most is the part you handled best. Two things to fix before merge, both small.

Verified on your head merged with current main: 400 passed, ruff clean, mypy clean, coverage 87.25% (manifest.py at 100%). Durations reproduce #200's own table exactly against the committed examples — agent 5m 14s, render 9m 26s on toolhive.

What you got right, specifically:

  • You avoided the @computed_field trap and documented why. manifest.py:47 — "This remains a plain helper so duration never changes the manifest schema" — plus a regression test asserting "duration_seconds" not in model_dump(). I checked independently: both committed example manifests round-trip through model_validate_jsonmodel_dump_json byte-identical in shape. Old run dirs are safe.
  • The docs wording is honest about the resume asymmetry. "the final attempt's duration" rather than "total time" — you noticed that stage_start overwrites started_at while cost accumulates, and told the user the truth instead of glossing it.
  • Negative durations return None rather than 0 on clock skew, and it's tested.

1. Blocking — mixed naive/aware timestamps crash report, and flip the exit code. manifest.py:55 catches only ValueError, but subtracting a naive datetime from an aware one raises TypeError:

naive start / aware finish -> TypeError: can't subtract offset-naive and offset-aware datetimes

End-to-end on a manifest with verified: true and one naive stamp:

pre-PR  human exit=0     # verified
post-PR human exit=1     # Rich traceback; --json and --markdown also exit 1

That last part is why this matters more than a normal crash: since #158, exit 1 means "completed but UNVERIFIED", so a verified run would read as a CI gate failure. utcnow() has only ever written aware stamps, so this needs a hand-edited or foreign manifest — but CLAUDE.md explicitly endorses hand-repairing run dirs, and #38 is open about exactly this class of report fragility.

One-token fix, verified working:

    except (ValueError, TypeError):
        return None

Plus a parametrize case: StageRecord(started_at="2026-07-21T12:00:00", finished_at="2026-07-21T12:00:05+00:00")None.

2. Blocking-ish — the human report's duration column is pinned by nothing. I mutation-tested each new behavior by reverting it in isolation. Dropping the duration from the human report entirely (lines.append(f" {name:<10} {rec.status:<10}{meta}{extra}")) → 400 passed, nothing caught it. There's no test for summarize() output at all. Same for the <1s sub-second rendering, which is a deliberate and correct choice in your code but survives being changed to "0s" — the exact thing #200's criteria forbid. Everything else you added is pinned (the JSON field, the markdown cells, malformed/missing/negative timestamps all fail correctly when reverted).

One test calling summarize(make_report_manifest()) and asserting both 1m 25s and the placeholder closes both gaps.

Nits, take or leave: the placeholder collides visually with the error separator (agent failed — — exit 127; - disambiguates for free); the 2.5s → "2s" test passes via banker's rounding, so it pins rounding mode rather than your formatter (2.4 expresses the intent); seconds: float | None at orchestrator.py:380 mixes with the Optional[...] used elsewhere in that module.

No grounding-path files touched, no manifest schema change, no exit-code contract change (beyond the crash above). Genuinely nice first contribution — the schema-preservation reasoning is better than most PRs that touch a persisted pydantic model.

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.

cli: readme2demo report never shows per-stage duration — the timestamps are already in manifest.json

2 participants