Instrument Phase 2 and Phase 3 boundaries with clock-authored phase events (#1853) - #1961
Conversation
…ubcommand (checkpoint)
…ock-authored phase events
- test the non-object --payload branch (pr-test-analyzer, Important) - make the default-location test behavioral (chdir + assert file lands) - re-ignore .prflow/logs/phase-events/ runtime dir (final-pass)
Two reviewers flagged the comment overclaiming whole-line write atomicity; state only O_APPEND's real guarantee (atomic EOF positioning).
…d-phase-3-carry-no-event
…ndle for #1853 verification-flight.py is not granted in the read-only review profile (AC6 forbids adding it), so the phase3-reviewers-dispatch/return events must not live in skills/review/ (breaks the #363/#540 review-head-grant pins). Move them to skills/implement/phases/phase-3-fix-loop.md (implement profile).
Keep the phase3-shadow-entry event; shorten its lead and tighten unpinned prose to fit under 61750 bytes without touching any pinned literal.
|
/prflow:review |
![prflow-reviewer[bot]](https://avatars.githubusercontent.com/u/223979750?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE with notes (clean, self-contained phase-event instrumentation; one internal-doc file-path inaccuracy and a payload-breadcrumb edge to tidy — none blocking at the critical threshold)
Issue Compliance
Reviewed against issue #1853: Instrument Phase 2 and Phase 3 boundaries with clock-authored phase events — criteria from the linked issue/workpad; scope unchanged. The shipped change satisfies the issue intent: a clock-authored event subcommand on scripts/verification-flight.py (timestamp from the helper's own _now()/_iso(), never the caller), an append-only JSONL log under .prflow/logs/phase-events/, an always-exit-0 non-blocking contract with a stderr breadcrumb on failure, and instrumentation calls at the Phase 2 durability-checkpoint boundaries and the Phase 3 /simplify, reviewer-dispatch/return, and shadow-entry boundaries. CI for the reviewed head (12ef71f6a2160ae89cd13df98d1220733e1b5ad0) reports lib + python tests: success and lint (shellcheck + actionlint + ruff): success, including the python-pool and monolith shards that carry TestPhaseEventAppend and the harness-python guards.
Verification Checklist Results
Verified via the Phase 3 specialist agents rather than a separate lite/agent checklist (focused, single-mechanism diff): 0 failed, 0 inconclusive. Core claims confirmed against HEAD — payload reserved-key protection (event/recorded_at never shadowed), O_APPEND|O_CREAT append semantics, OSError breadcrumb-and-exit-0 failed-write arm, and the .gitignore entry ordered after the !/.prflow/logs/ negation.
Code Review Findings
🟡 Suggestion / Minor
docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md(the new "Phase-boundary event instrumentation" subsection, "instrumented boundaries" bullet) attributes thephase3-reviewers-dispatch/phase3-reviewers-returnevents toskills/review/phases/phase-3-agents.md"around reviewer dispatch", but that file is untouched by this PR — the two events are actually added inskills/implement/phases/phase-3-fix-loop.md, wrapping thereview-and-fixloop invocation (not the review engine's reviewer fan-out). The writing-skills evidence marker on the issue workpad repeats the same wrong file and omits the file actually edited. This is a genuine diff-added documented inaccuracy, but it is behavior-inert internal-doc prose:docs/internal/isrm -rf'd from the vendor slice (vendor-slice.sh:106) so no consumer reads it, and no tool parses its prose to decide behavior — so under the Phase 4.1.5 behavior-inert prose cap it is deterministically capped to Suggestion and does not drive a REJECT at thecriticalthreshold. It should nonetheless be fixed (change the path toskills/implement/phases/phase-3-fix-loop.mdand reword "around reviewer dispatch" to "around thereview-and-fixloop invocation"), and the workpad evidence marker corrected to list the file actually touched. (raised by 3/4 agents)scripts/verification-flight.pycmd_event: a well-formed JSONnullpayload (--payload null) is silently dropped with no breadcrumb —json.loads("null")returnsNone, which is excluded by theelif payload is not None:guard that exists to avoid double-breadcrumbing the parse-error path. Both the docstring and the overview say a "non-object" payload is dropped with a stderr breadcrumb;nullis a non-object value, so this one edge contradicts the stated contract. Impact is nil in practice (base event still recorded, exit 0, and no first-party caller passes--payload), but a distinct parse-error sentinel (so a genuinenullreaches the non-object breadcrumb arm) — or an explicit contract carve-out fornull— would resolve the code-vs-contract gap. (raised by 2/4 agents)- Test-coverage gaps (all optional): the
O_APPENDconcurrent-append offset claim (verification-flight.py:249-251) is unpinned — every existing test passes sequentially and would stay green under a truncating open; a JSONnull/ scalar (42,"str") payload branch, an empty ("") and empty-object ({}) payload, a valid-falsy payload value ({"n":0}/{"b":false}— the repo's off-switch bug class), and theevent-missing-nameargparseSystemExitare unexercised. The phase-prose additions correctly carry no coverage (agent-executed prompt prose, per the repo's recorded decision). (raised by 1/4 agents)
over-grade annotation: finding 1 flagged shape 2 (behavior-inert internal-doc prose) — cap applied, capped to Suggestion.
truthfulness sweep: finding 1 is a diff-added documented inaccuracy but is governed by the Phase 4.1.5 behavior-inert prose cap (both inertness limbs established: not tool-read for behavior; pruned from the vendor slice so no external consumer), so it is filed capped at Suggestion rather than routed into the self-contradicting-diff carve-out.
intra-diff contradiction scan: no contradiction found.
Verdict Criteria
No verification-checklist FAIL/INCONCLUSIVE; no finding at or above the configured critical threshold. All findings are Suggestion-level (the internal-doc inaccuracy is behavior-inert-capped; the remainder are diagnostic/test-coverage). → APPROVE with notes.
Recommended before merge: correct the internal-doc file attribution (finding 1); the payload-null and test-coverage items are optional.
|
/prflow:review |
There was a problem hiding this comment.
Verdict: APPROVE with notes
PR #1961 (issue #1853) adds a clock-authored event subcommand to scripts/verification-flight.py and instruments the Phase 2/3 boundaries. The functional change is correct, well-tested, and satisfies the acceptance criteria. Two non-blocking findings below — one internal-doc inaccuracy worth fixing before merge, one minor consistency suggestion.
Test evidence
Authoritative CI conclusions for the reviewed head c30c025e30484f08cf0002a265b7c4ff36ce4a85 (read from the GitHub API): lib + python tests: success, lint (shellcheck + actionlint + ruff): success, and all five shards (modules-rest, modules-pin, python-pool, modules-large, monolith): success. Test evidence is present and green.
Acceptance criteria
- AC1 (event-append subcommand, own-clock timestamp,
{event, recorded_at}shape, append-only under.prflow/logs/) — satisfied.cmd_eventseedsrecordfrom_iso(_now()), appends viaO_WRONLY|O_APPEND|O_CREATto.prflow/logs/phase-events/phase-events.jsonl. - AC2 (always exits 0; failed write breadcrumbs) — satisfied. The whole
mkdir/open/write/closesequence is inside onetry,OSErroris caught with a specific breadcrumb, and every path returnsEXIT_OK. - AC3 (Phase 3 prose: simplify start/end, reviewer dispatch/return, shadow entry, vendored leading token) — satisfied by the implementation; see Finding 1 for a doc mismatch about where the reviewer events live.
- AC4 (Phase 2 prose at each durability-checkpoint boundary) — satisfied; both
phase2-durability-checkpoint.shsites (phase-2-implement.md,phase-2-sweeps-quality.md) received aphase2-checkpointevent. - AC5 (no new agent-volunteered duration field; no telemetry field changes meaning) — satisfied.
- AC6 (no new
lib/capability-profiles.jsonentry) — satisfied;Bash(.prflow/vendor/prflow/scripts/verification-flight.py:*)already grants the subcommand.
Repo-specific gates
- Prompt-surface edit routing gate (extension) — discharged. The diff touches trigger globs, and issue #1853's workpad carries a
Writing-skills evidence:marker with all four slots explicitly recorded (skill-loaded=yes,guidance-applied=yes,pressure-scenario=no,micro-tests=no). - Verification-evidence advisory — silent; the workpad carries a
Verification evidence:marker. .gitignore—/.prflow/logs/phase-events/is correctly placed after the!/.prflow/logs/negation, so the per-run event log stays ignored.- Shipped-prose cost — the added skill-prose blocks are minimal (one-sentence recipe + a vendored-leading-token fence each); the
shadow-review.mdgrammar trims are net token reductions with no accuracy loss. No unearned wording.
Findings
1. Important (behavior-inert) — canonical internal doc cites the wrong file for the reviewer events.
docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md (new "Phase-boundary event instrumentation" section) states phase3-reviewers-dispatch / phase3-reviewers-return are emitted "around reviewer dispatch (skills/review/phases/phase-3-agents.md)". This is false against the reviewed head: those two events are actually added in skills/implement/phases/phase-3-fix-loop.md (around the Phase 3.3 review-and-fix invocation and its return), and skills/review/phases/phase-3-agents.md is not modified by this PR and carries no such call. A maintainer following the canonical overview to phase-3-agents.md finds nothing. The event names are correct; only the file citation and the "around reviewer dispatch" characterization are wrong. This is internal (not consumer-shipped) documentation and does not affect runtime behavior, hence non-blocking — but it should be corrected in this PR: change the parenthetical to skills/implement/phases/phase-3-fix-loop.md and reword to "around the Phase 3.3 review-and-fix dispatch/return".
2. Suggestion — JSON null payload is silently dropped without a breadcrumb.
In cmd_event (scripts/verification-flight.py), --payload null parses to None, which fails both the isinstance(payload, dict) arm and the elif payload is not None breadcrumb arm, so it records the base event with no stderr note — asymmetric with the non-object arm ([1,2]) which does breadcrumb. Not a swallowed defect (a null payload carries nothing to merge and the base event is still recorded), and the always-exit-0 contract holds. Consider letting the elif also breadcrumb the None case for symmetry, and adding a --payload null test to pin this currently-untested branch.
Verified correct (no action)
- Reserved-key protection:
event/recorded_atcannot be shadowed by a payload key (tested). - Non-blocking contract completeness: every line outside the
try(_iso(_now()),json.dumps) is provably non-raising for the values inrecord. - Test coverage:
TestPhaseEventAppendcovers all seven behavioral arms of the ACs — record shape, accumulation, default location, payload merge + reserved-key protection, non-object/unparseable breadcrumbs, and the failed-write exit-0 path.
…d-phase-3-carry-no-event
…ayload Addresses the two actionable findings from the review of c30c025. Finding 1 — `docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md` cited `skills/review/phases/phase-3-agents.md` for `phase3-reviewers-dispatch` / `phase3-reviewers-return`. That file is untouched by this PR and carries no such call; both events are emitted in `skills/implement/phases/phase-3-fix-loop.md` around the Phase 3.3 `review-and-fix` dispatch and its return. The citation and the "around reviewer dispatch" characterization are corrected. Finding 2 — `cmd_event` reused `None` as the parse-error sentinel, so a well-formed `--payload null` parsed to `None`, matched neither the dict arm nor the `elif payload is not None` arm, and was dropped with no stderr breadcrumb — contradicting the documented contract that a non-object payload drops *with* a breadcrumb. A distinct `unparseable` sentinel now separates the two, so `null` reaches the non-object arm while the parse-error path still breadcrumbs exactly once. The overview's contract sentence is made exact about both the `null` and the empty-payload cases. Tests sweep the payload parser's non-object shapes together (`null`, number and string scalars, array), the empty-string and empty-object shapes, and the valid-falsy merge rows (`0` / `false` / `""`), and pin that the parse-error arm does not double-breadcrumb.
|
/prflow:review |
There was a problem hiding this comment.
Verdict: APPROVE with notes (clean, well-tested change; only Suggestion-level advisories)
Issue Compliance
Reviewed against issue #1853: Phase 2 and Phase 3 carry no event instrumentation — criteria from the /prflow:implement workpad comment; scope unchanged. Requirement-based checklist items are included in the verification results below.
Criteria came from the /prflow:implement workpad comment — this run's authoritative set, possibly narrowed from the issue. No divergence between the workpad's criteria and the issue body's.
All six acceptance criteria are met:
- ✅
scripts/verification-flight.pygains anevent <name>subcommand that appends a{"event", "recorded_at"}record —recorded_atproduced by the helper's own clock via_iso(_now()), never the caller — to an append-only JSONL log at.prflow/logs/phase-events/phase-events.jsonl(O_WRONLY|O_APPEND|O_CREAT). - ✅ The append always returns
EXIT_OK(0); anOSErroron the write path emits a specific stderr breadcrumb and the run continues. - ✅ Phase 3 prose invokes the append with the vendored path as the command's leading token at
/simplifystart/end (phase-3-review.md), reviewer dispatch/return (phase-3-fix-loop.md), and shadow-review entry (shadow-review.md). - ✅ Phase 2 prose invokes the append at both
phase2-durability-checkpoint.shboundaries (phase-2-implement.md,phase-2-sweeps-quality.md) — the only two such sites. - ✅ No new agent-volunteered duration field is added; records are clock-authored and no existing telemetry field changes meaning.
- ✅ No new entry in
lib/capability-profiles.json— the helper is already granted with a wildcard over its arguments.
Verification Checklist Results
Test evidence is the authoritative CI conclusion for the reviewed commit 9836f5d143ebe85f11c82c63cf4d3fd1935bf826 (per this run's engine ground-truth block): every required check is success — lib + python tests, lint (shellcheck + actionlint + ruff), and all shards (monolith, modules-large, modules-rest, modules-pin, python-pool). The new TestPhaseEventAppend suite (9 test methods) covers the subcommand's contract directly, and the change was independently verified by four Phase-3 specialist reviewers (code-reviewer, silent-failure-hunter, pr-test-analyzer, comment-analyzer). Requirement compliance is established above (all six ACs met).
over-grade annotation: no finding flagged
intra-diff contradiction scan: no contradiction found
truthfulness sweep: no finding promoted
Code Review Findings
🟡 Suggestion / Minor
skills/review-and-fix/references/shadow-review.mdsits at ~61,747 bytes against the 61,750-byte reference-size ceiling — a ~3-byte margin. It passes today (the +125-byte event block is offset by ~68 bytes of prose trimming), but any future prose addition to this file will triplint-reference-size.py; the remedy then is to trim, not exempt. (raised by 1/4 agents)scripts/verification-flight.py:304—os.write(fd, line.encode("utf-8"))does not check its return against the buffer length, so a short write would append a truncated JSONL line with no breadcrumb. Negligible in practice (small records, regular file,O_APPEND) and well within the best-effort contract; noted for completeness. (raised by 1/4 agents)- Three test-body comments restate the assertion on the line immediately below them, which §2.3's restatement rule discourages:
lib/test/test_verification_flight.py:83,:128,:183. Behavior-inert test narration. (raised by 1/4 agents) - Documentation/attestation drift (not in the shipped code): the PR description's "Changes" section and the issue #1853 workpad's
Writing-skills evidence:marker both nameskills/review/phases/phase-3-agents.mdas carrying the reviewer dispatch/return events, but the diff actually places them inskills/implement/phases/phase-3-fix-loop.md. The shippeddocs/internal/DEVFLOW_SYSTEM_OVERVIEW.mdis correct; only the PR body prose and the writing-skills marker's file list are stale. No code or shipped-doc falsehood — the prompt-surface routing evidence gate is still discharged (all four dispositions recorded:skill-loaded=yes guidance-applied=yes pressure-scenario=no micro-tests=no). (raised by orchestrator) skills/implement/phases/phase-3-review.md— thephase3-simplify-endfence sits after the commit/push block but before the "if/simplifymade no changes, skip the commit" note; it correctly fires regardless, but reads slightly out of order. Purely cosmetic. (raised by 1/4 agents)
Verdict Criteria
- No verification-checklist FAIL or INCONCLUSIVE (test evidence: CI all-green on the reviewed commit).
- No review-agent finding at or above the
criticalthreshold; all findings are Suggestion-level. - No self-contradicting-diff finding (the shipped diff's docs/comments/tests are accurate against HEAD).
- Only findings below the verdict threshold present → APPROVE with notes.
Prompt-surface routing evidence gate (DevFlow-repo policy)
The diff touches trigger globs (skills/implement/phases/*.md, skills/review-and-fix/references/*.md). The gate is discharged: issue #1853's workpad carries a Writing-skills evidence: marker with all four dispositions explicitly recorded — skill-loaded=yes, guidance-applied=yes, pressure-scenario=no, micro-tests=no. A no never draws a finding on its own.
Verification-evidence advisory (non-blocking)
The workpad records Verification evidence: for the run (§2.4: harness-python-guards module 45 passed 0 failed; full test_verification_flight.py 121 OK; ruff clean), and the workpad carries checkpoint rows (cloud tier). Advisory clause silent.
Shipped-prose cost audit (DevFlow-repo policy)
The added skills/** prose is minimal and earns its place: each insertion is one recipe sentence plus a vendored-leading-token fence carrying the required instruction (append the boundary event, best-effort). No restated rule, no derivable mechanics, no structural narration. No cost finding.
1 participant
Summary
eventsubcommand toscripts/verification-flight.pyand calls it at each Phase 2/3 boundary.Changes
scripts/verification-flight.py: Newevent <name> [--payload <json>] [--log-dir <dir>]subcommand. It appends one{"event": <name>, "recorded_at": <iso>}record — timestamped from the helper's own_now()/_iso()clock, never the caller — to an append-only JSONL log at.prflow/logs/phase-events/phase-events.jsonl(opened withO_WRONLY|O_APPEND|O_CREAT, directory created best-effort). An optional--payloadobject merges extra keys in, but the reservedeventandrecorded_atkeys can never be shadowed by a payload key. A malformed or non-object--payloadis dropped with a stderr breadcrumb and the base event is still recorded. AnyOSErroron the write path emits a stderr breadcrumb and returnsEXIT_OK.Phase 2 prose (
skills/implement/phases/phase-2-implement.md,skills/implement/phases/phase-2-sweeps-quality.md): each durability-checkpoint boundary (wherescripts/phase2-durability-checkpoint.shalready runs) now also appends aphase2-checkpointevent, invoked with the vendored literal as the command's leading token.Phase 3 prose (
skills/implement/phases/phase-3-review.md,skills/implement/phases/phase-3-fix-loop.md,skills/review-and-fix/references/shadow-review.md):phase3-simplify-start/phase3-simplify-endaround/simplify,phase3-reviewers-dispatch/phase3-reviewers-returnaround the Phase 3.3review-and-fixdispatch and its return, andphase3-shadow-entryat shadow-review entry..gitignore: ignores/.prflow/logs/phase-events/(placed after the!/.prflow/logs/negation so the per-run event log is never committed).docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md: new "Phase-boundary event instrumentation (issue #1853)" subsection documenting the subcommand, its non-blocking contract, the instrumented boundaries, and scope.Scope: no new agent-volunteered duration field is added and no existing telemetry field changes meaning. The helper is already granted with a wildcard over its arguments (
Bash(.prflow/vendor/prflow/scripts/verification-flight.py:*)), so the new subcommand adds no entry tolib/capability-profiles.json.Resolves
Resolves #1853
Test Plan
TestPhaseEventAppendinlib/test/test_verification_flight.py(added in this PR) covers theeventsubcommand: a clock-authored record with exactly{event, recorded_at}(DEVFLOW_FLIGHT_NOW-pinned timestamp viavf._iso); append-only accumulation across successive events; the default log location under<cwd>/.prflow/logs/phase-events/; an optional payload merged with the reservedevent/recorded_atkeys protected from a spoofing payload; a non-object payload and an unparseable payload each breadcrumbing while still recording the base event; and a failed write (log dir under a regular file) that exitsEXIT_OKand breadcrumbs. The phase-prose additions are agent-executed prompt prose with no wording pin (per the repository's recorded decision on such prose) and carry no automated coverage.scripts/verification-flight.pyis owned by theharness-python-guardsmodule:lib/test/run-module.sh harness-python-guards.Visual Changes
N/A
Breaking Changes
None
Prompt-surface size
Derived at
e9cdbd97ef09550f44ef6d818d145ab4653f04d4against merge-base3a139ee22b3067a7f5025dfb5cc6065adc2547b6(origin/main). Covered: tracked*.mdunderskills/,agents/,.prflow/prompt-extensions/.skills/implement/phases/phase-2-implement.mdskills/implement/phases/phase-2-sweeps-quality.mdskills/implement/phases/phase-3-review.mdskills/review-and-fix/references/shadow-review.mdskills/implement/phases/phase-3-fix-loop.mdGenerated via /prflow:implement (v2.34.22, claude-opus-4-8, low)