Make the implement Phase 4.1 documentation gate record only run-specific workpad facts - #2137
Conversation
…he docgate read helper (checkpoint)
…span note and check-command doc-AC discharge in Phase 4.1 (checkpoint)
…e-4-documentation.md (issue #2129)
…mplement-phase-4-1-documentation-gate
…e-4-documentation.md (issue #2129 AC5)
|
/prflow:review |
PRFlow Review — PR #2137Status: ✅ APPROVE with notes Blueprint
Verdict: APPROVE with notes (all 13 checklist items pass and all 9 acceptance criteria are met; only below-threshold notes remain)Issue ComplianceReviewed against issue #2129: Make the implement Phase 4.1 documentation gate record only run-specific workpad facts — criteria from workpad; scope unchanged. Requirement-based checklist items are included in the verification results below. Test evidence (CI, reviewed commit Verification Checklist Results13 passed, 0 failed, 0 inconclusive — 2 via lite probe, 11 via agent. ✅ Passed items (13 of 13) — click to expand
Code Review FindingsAll findings are single-source (no mechanical corroboration; 5 Phase-3 agents returned). None is a documented_falsehood / self-contradicting-diff. None is at or above the 🟠 Important / Major
🟡 Suggestion / Minor
over-grade annotation: finding flagged (shape 3, advisory — verdict unchanged) Verdict Criteria
VerdictAPPROVE with notes — reviewed HEAD Run telemetryPer-phase call/token/wall-clock figures: unavailable (not established this run). Subagent effectiveness traceIteration 1
Recurring defect kinds
|
![prflow-reviewer[bot]](https://avatars.githubusercontent.com/u/223979750?s=60&v=4){kind=small}
…capture (review of #2137) Adds a suppress-flaky-extractor stub whose failing attempts emit the `suppressed a span` breadcrumb, and two run.sh assertions: a clean retry relays no phantom docgate-suppressed line (RED under `2>>`), and the extract-failed token forwards the breadcrumb but relays no span (RED with an emit on the failure arm). Trims the #2129 run.sh comment blocks to the prevention-only standard and names the breadcrumb/parse backtick coupling at both the producer and the consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/prflow:review |
PRFlow Review — PR #2137Status: ✅ APPROVE with notes Blueprint
Verdict: APPROVE with notes (all findings are Suggestions; no Critical/Important; 17/17 checklist items pass; both repo prompt-surface gates discharged)Issue ComplianceReviewed against issue #2129: Make the /prflow:implement Phase 4.1 documentation gate record only real, run-specific workpad facts — criteria from workpad; scope unchanged. Requirement-based checklist items are included in the verification results below. DevFlow-repo prompt-surface gate (extension): the diff touches Verification Checklist Results17 passed, 0 failed, 0 inconclusive — 1 via lite probe, 16 via agent. ✅ Passed items (17 of 17) — click to expand
Code Review Findings🟡 Suggestion / Minor
A second silent-failure-hunter observation (a no-backtick breadcrumb would relay the whole line) was noted as "no change required" — defended by the documented producer-side coupling — and is not filed as a finding. over-grade annotation: no finding flagged Verdict Criteria applied (threshold: critical)
Final-pass reviewer prompt-extension load: Run telemetryPer-phase call/token figures were not instrumented this run ( Subagent effectiveness traceIteration 1
|
1 participant
Summary
/prflow:implementPhase 4.1 documentation gate record only real, run-specific workpad facts, so a workpad no longer carries a fixed once-per-run boilerplate note that is false on runs where nothing was suppressed.scripts/read-doc-needed-deliverables.shonto stdout so Stage 1 can record the actual span, and stop the gate from ticking a check-command doc-AC on evidence that does not yet exist.Changes
Read boundary (
scripts/read-doc-needed-deliverables.sh): The helper now captures the extractor's stderr to a truncated-per-attempt scratch file, forwards it unchanged to its own stderr (so the merged tool result still carries everysuppressed a spanbreadcrumb), and emits one self-identifyingdocgate-suppressed: <span>line after thedocgate-outcome:line on either success token (deliverablesandno-deliverables). The span is the first suppressed span with the breadcrumb's surrounding backticks removed, parsed with bash builtins only. The# STDOUT SHAPEheader documents the new line.Phase 4.1 (
skills/implement/phases/phase-4-documentation.md): The fixed once-per-run disclosure sentence is replaced with a span-suppression note that fires only when adocgate-suppressed:line is present, delivering the span to the workpad through--note-file(never a double-quoted--noteargument). The deferred documentation-AC discharge paragraph gains a check-command rule: a criterion naming a check command is ticked only after the orchestrator runs that command itself (or the covering run the coverage map names) over the landed docs and quotes the result line; a subagent's report does not discharge it, and a tier refusal of both forms takes the existing Blocked arm namingprflow_implement.allowed_tools.Docs (
docs/internal/implement-skill.md): Corrected to state the read boundary captures and relays the breadcrumb as adocgate-suppressed:stdout line rather than the prior claim that the gate does not capture that stderr.Tests (
lib/test/run.sh,lib/test/modules/coverage-map.json): The #1554 adversarial exact-line expectation now asserts thedocgate-suppressed: bash lib/test/run.shline; a new assertion confirms the merged stream still carries thesuppressed a spanbreadcrumb; a new no-deliverables fixture asserts the span is relayed on that branch too. A coverage-map entry for issue #2129 is added.Resolves
Resolves #2129
Test Plan
lib/test/run-shard.sh monolith(the#1554/#2129fixture matrix inlib/test/run.sh): the adversarial body printsdocgate-outcome: deliverables,docgate-suppressed: bash lib/test/run.sh, then thedocgate-path:line; the clean fixtures print nodocgate-suppressed:line.#2129assertion confirmingrdnd_runon the adversarial body still containsextract-doc-needed-paths.sh: suppressed a spanon the merged stream.#2129no-deliverables fixture assertsdocgate-outcome: no-deliverablesplus thedocgate-suppressed:line at exit 10.lib/test/lint-reference-size.pyreports no violation forskills/implement/phases/phase-4-documentation.md.#644pin (suppressed a spanappears once in the phase file) and the#380discharge-heading pin pass unchanged.Visual Changes
N/A
Breaking Changes
None. The shipped helper and phase file reach a consumer in the same vendored plugin fetch, so an old helper beside a new phase file cannot occur.
Prompt-surface size
Derived at
ec6e53f18a7ee66df300f1c9c83e0cbd80e7040bagainst merge-base352dc01ccef94d5ded525d36f4ce20c4107ca682(origin/main). Covered: tracked*.mdunderskills/,agents/,.prflow/prompt-extensions/.skills/implement/phases/phase-4-documentation.mdReview response (PRFlow review at
ec6e53f18a7ee66df300f1c9c83e0cbd80e7040b)1f318a3f):lib/test/run.shgains asuppress-flaky-extractorstub whose failing attempts emit thesuppressed a spanbreadcrumb, plus two assertions: a clean retry relays no phantomdocgate-suppressed:line, and theextract-failedtoken forwards the breadcrumb but relays no span. Mutation-checked on scratch copies of the helper:2>→2>>turns the retry assertion RED withdocgate-suppressed: phantom-spanon the clean retry (andbreadcrumbs=2on the failure case); an emit added to the failure arm turns only the extract-failed assertion RED. The working-tree helper passes 13/13 in the fixture block;lib/test/run-shard.sh monolith: 10201 passed, 0 failed, 0 skipped.handle_spaninscripts/extract-doc-needed-paths.shreturns atntok -eq 0before it can callsuppress_span, so every breadcrumb carries at least one token and[ -n "$SUPPRESSED_SPAN" ]cannot conflate "empty span" with "no span". Revisit only if a second breadcrumb producer appears.suppress_span()(the producer) and one at the parse site inscripts/read-doc-needed-deliverables.sh.#2129comment blocks inlib/test/run.share trimmed to at most three lines, each naming the wrong change it prevents.extract-failedemit restriction is now exercised (see Important 1). The first-span[ -z ]guard stays untested:span_warnedmakes the producer's breadcrumb once-per-run, so a test would have to pin a two-breadcrumb shape the producer never emits; the guard is defence in depth against a future producer change. Revisit ifsuppress_spanever emits more than once per run.origin/main(no conflicts).Generated via /prflow:implement (v2.36.15, claude-opus-4-8, low)