Skip to content

fix(publish): restore the already-UTC fast path — it was load-bearing, not dead weight - #38

Open
stephschofield wants to merge 4 commits into
worktree-ponytail-audit-applyfrom
fix/pr34-utc-fastpath
Open

stephschofield wants to merge 4 commits into
worktree-ponytail-audit-applyfrom
fix/pr34-utc-fastpath

Conversation

@stephschofield

Copy link
Copy Markdown
Contributor

Stacked on #34. Fixes the one HIGH finding an adversarial review confirmed against it.

The regression

The audit removed this from publish.py:_normalize_utc_z as redundant:

if s.endswith("Z") and "+" not in s:
    return s

It is not redundant. The strftime("%Y-%m-%dT%H:%M:%SZ") below emits whole seconds only, so an already-conformant UTC value loses sub-second precision:

input main #34 head
2026-07-15T15:36:06.123456Z 2026-07-15T15:36:06.123456Z 2026-07-15T15:36:06Z
2026-07-15T15:36:06.500Z 2026-07-15T15:36:06.500Z 2026-07-15T15:36:06Z
2026-07-15T15:36:06-05:00 2026-07-15T20:36:06Z 2026-07-15T20:36:06Z

This contradicts the PR's headline "Behavior-neutral: no feature, guard, or error path changes."

Reachable, not theoretical

  • leaderboard.py:75 — the schema explicitly admits fractional seconds: ^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$
  • publish.py:432publish build --updated-at passes caller input straight through

So the value being truncated is one the schema itself declares valid.

Why 1123 tests missed it

Every existing timestamp fixture uses whole seconds, and the one normalization test asserts only doc["updated_at"].endswith("Z") — which a truncated value still satisfies. The assertion cannot distinguish the regression from correct behavior. That gap is now closed by an exact-equality assertion.

TDD

RED, against #34 head:

FAILED test_normalize_preserves_fractional_seconds_on_utc_input[2026-07-15T15:36:06.123456Z]
FAILED test_normalize_preserves_fractional_seconds_on_utc_input[2026-07-15T15:36:06.500Z]
2 failed, 4 passed

GREEN: 42 passed. Full hermetic suite: 1121 passed, 35 skipped. The 5 test_containment.py failures reproduce identically on clean origin/main (local userns unavailable) — pre-existing and environmental.

test_normalize_still_converts_offsets_to_z guards the other direction, so restoring the fast path cannot shadow the conversion it precedes.

The rest of the audit's cuts verified clean — 506 deletions/61 insertions is exact, all four DEMO_FIX_PLAN items confirmed shipped, dead symbols have zero remaining references.

Evidence: docs/proof/pr-review-fleet-2026-08/pr34-utc-truncation.png

…, not dead weight

The audit removed:

    if s.endswith("Z") and "+" not in s:
        return s

as redundant with the fromisoformat/astimezone path below. It is not. The
strftime format emits whole seconds only, so an already-conformant value with
sub-second precision is silently truncated:

    2026-07-15T15:36:06.123456Z -> 2026-07-15T15:36:06Z

That is a behavior change, contradicting the PR's headline 'behavior-neutral'
claim. It is reachable: leaderboard.py:75's schema explicitly admits fractional
seconds ((\.\d+)?Z) and 'publish build --updated-at' passes caller input
straight through, so the truncated value is one the schema declares valid.

1123 tests missed it because every timestamp fixture uses whole seconds and the
one normalization test asserts only .endswith('Z') — which a truncated value
still satisfies.

RED: 2 failed, 4 passed. GREEN: 42 passed. Hermetic: 1121 passed, 35 skipped.
…e order

Records all 18 findings across the 6 open PRs including the 9 that did NOT
survive verification — a review that logs only confirmations is not auditable.

Headline: #29 and #33 rewrite the same submit.py line with incompatible
semantics and neither conflicts with main, so a per-branch conflict check clears
both. Merging #33 first silently reintroduces issue #32.
BOTH reviewers independently returned NAUGHTY: the restored fast path
'endswith("Z") and "+" not in s' returns junk VERBATIM, skipping validation.

Verified reachable — publish build --updated-at passes caller input straight
through into published leaderboard.json:

  'not-a-timestampZ'            -> returned verbatim
  '2026-13-45T99:99:99Z'        -> returned verbatim AND the schema ACCEPTS it
                                   (digit-shaped: month 13, day 45 publish clean)
  '2026-07-15T15:36:06-05:00Z'  -> negative offset carries no '+', so it slipped

Confirmed byte-identical on origin/main, so the flaw is PRE-EXISTING, not
introduced here. Hardened anyway: both reviewers rated it FAIL, it is one line,
and shipping a known hole because it predates us is how holes persist.

Fix: match the SCHEMA'S OWN pattern instead of a bare suffix test, then parse to
reject digit-shaped impossible dates the regex alone admits. Everything failing
the match falls through to fromisoformat then _EPOCH, so it stays fail-closed.

  precision preserved : ...06.123456Z -> ...06.123456Z   (the fix's purpose)
  conversion intact   : ...06-05:00   -> ...20:36:06Z    (no regression)
  malformed rejected  : all five vectors -> _EPOCH

test_fast_path_admits_exactly_the_schema_shape asserts the regex equals the
schema's pattern, so the two cannot drift apart and reopen the bypass.
@stephschofield

Copy link
Copy Markdown
Contributor Author

🎅 Santa Loop Review — Round 1

SANTA VERDICT: NAUGHTY — by protocol only. Both reviewers scored Correctness, Security, Error handling, Completeness, Internal consistency, and No regressions as PASS. The single dissent is Reviewer B failing Test coverage on uncovered edge classes. No defect was found in the shipped code.

Reviewer Model Verdict
A Claude Opus PASS
B GPT-5.4 (codex) FAIL

Rubric

Criterion A B Notes
Correctness (CRITICAL) PASS PASS The fast path is deliberately not identical to the slow path — that divergence is the fix. _UTC_Z_RE intercepts only schema-shaped UTC-Z; naive, offset-aware, and +00:00 inputs still take the slow path unchanged.
Security PASS PASS The endswith("Z") and "+" not in s bypass on main is closed. Verified: 2026-13-45T99:99:99Z, not-a-timestampZ, and ...-05:00Z all now return _EPOCH instead of landing verbatim in leaderboard.json. This PR is strictly safer than main.
Error handling PASS PASS Fail-closed on both branches. Regex-matched values are re-parsed in their own try/except; everything else falls to _EPOCH.
Completeness PASS PASS The stated regression is fixed.
Internal consistency PASS PASS Regex is duplicated rather than imported, but test_fast_path_admits_exactly_the_schema_shape asserts equality against LEADERBOARD_SCHEMA and will catch drift.
No regressions PASS PASS Verified empirically: -05:0020:36:06Z, +00:00Z. The fast path does not shadow the conversion it precedes.
Test coverage PASS FAIL Both agree test_normalize_preserves_fractional_seconds_on_utc_input would have caught #34's deletion — the core ask. B fails it for missing leap-second, naive, and DST cases. A scores PASS: leap second is covered behaviorally (verified 23:59:60Z_EPOCH), and the rest are suggestions, not blockers.

Agreement

  • Both flagged: nothing blocking. 6/7 criteria unanimous PASS.
  • B only: test-coverage gaps for leap-second 23:59:60Z, naive no-suffix timestamps, and DST-boundary inputs.
  • A only: two behavioral observations below, both verified by execution, neither a regression from this PR.

Critical issues

None. No reviewer found a defect in the code as shipped. B's blocking item is a test-coverage gap, not a bug — the behaviors it wants pinned were verified correct by hand:

2026-12-31T23:59:60Z        -> 1970-01-01T00:00:00Z   (leap second, fail-closed ✅)
2026-03-08T02:30:00-06:00   -> 2026-03-08T08:30:00Z   (DST spring-forward ✅)
2026-11-01T01:30:00-05:00   -> 2026-11-01T06:30:00Z   (DST fall-back ✅)

Suggestions

  1. Naive datetimes are interpreted as host-local (A, verified). 2026-07-15T15:36:062026-07-15T20:36:06Z on a CST host, but would return 15:36:06Z on a UTC runner. Same input, different published timestamp depending on where publish build runs. Pre-existing on main, not introduced here — but undocumented and worth an explicit decision: reject naive input as fail-closed, or document the host-local assumption.

  2. +00:00 inputs still truncate fractional seconds (A, verified). 2026-07-15T15:36:06.999+00:002026-07-15T15:36:06Z. This is the same UTC instant with the same sub-second loss the PR's own thesis calls load-bearing — the fix just doesn't reach the +00:00 spelling. Low impact (git %cI emits whole seconds), but the asymmetry is worth either closing or noting.

  3. Add the edge-case tests B asked for23:59:60Z, naive, and DST parametrizations. The behavior is already correct; these would pin it against the next audit that reads the fast path as dead code.

  4. Centralize the pattern (B). Import one updated_at regex into both leaderboard.py and publish.py instead of duplicated text plus a sync test.

Verification performed

  • pytest tests/test_publish.py at PR head (a98ab6b): 48 passed.
  • All timezone classes in the rubric executed directly against the built function.
  • Confirmed finding 1 is present on origin/main — pre-existing, not a regression.

Dual independent adversarial review — no shared context between reviewers.

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.

1 participant