[#220] feat: breakdown-to-task feedback loop (close tasks + comment) - #468
[#220] feat: breakdown-to-task feedback loop (close tasks + comment)#468rucka wants to merge 12 commits into
Conversation
Verdict
PR: #468 · Author: rucka · Reviewer: independent reviewer agent · Date: 2026-08-28 · Story: US-220 · Type: feature Classification matrix — per dimension
Tier = max(assessed) = yellow — confirms refinement-time classification. AssessmentsSecurity — Input validationVerdict: green — no input surface: the diff is markdown/KB plus one read-only conformance test. Security — Output handlingVerdict: green — the only rendered output is a PM comment body composed by the skill; no encoding surface introduced. Security — AuthenticationVerdict: green — writes ride the already-credentialed Security — AuthorizationVerdict: green — no access-control decision on the changed paths. Security — Introduced vulnerabilitiesVerdict: green — 0 introduced, 0 pre-existing. CostVerdict: Architecture (Coupling)Verdict: green — one owner (guideline), one caller ( Bug fix — Red test before fixVerdict: not applicable — not a bug fix. DetailsFindings by severityCritical (must fix before merge)
Major (must fix before merge)
Minor (must fix before merge — same bar as Major, just lower impact)
Questions (informational, never blocking)
Positive feedback
Functionality & requirements (AC coverage)
Testing & quality gates
Adoption compliance
Tech debt
Documentation
Performance & deployment
|
…st once Review round 1 on PR #468. - Step 3.1's commit-per-task skip targeted 3.2, jumping over the 3.1b flush: the recommended (and supervised-default) strategy posted no comment at all. Skip now targets 3.1b; new conformance assertion rejects ANY Phase-3 skip whose target ranks after 3.1b (heading order cannot see a jump). - Step 3.1b drains the queue and guards on "empty OR already flushed"; the HALT preamble flushes unless 3.1b already did — the Step 3.3 HALTs fire after the post, so they were producing a second comment per iteration. - Guideline: one write per checkbox — a task ticking its item AND a DoD box is two read-patch-write cycles, not a two-line diff the check rejects. - execution.mdx no longer promises locator/patch/fallback detail behind a catalog link that does not host it; it states the three rules inline. - Test header: mechanism asserted on both copies, wiring on the dataset, mirror equality delegated to skill-md-mirror. Refs: #220
…line it pins Review round 2 on PR #468. - The anti-jump detector read raw markdown (`Move to (?:Step|Phase)`) against a file written `Move to **Step 3.1b**`: it matched nothing before the flush, so the assertion passed on an empty set for ANY target. Emphasis is stripped first, and a mutation test restores the verbatim round-1 bug and requires the detector to report it. - Step 2.8 item 2 short-circuited commit-per-story to Step 2.1 BEFORE the tick-and-queue: a 4-task story red on T3 queued only the failure and left T1/T2 unticked. Item 2 now routes through items 7-8; assertion pins that the queue site is not inside a strategy branch. - Step 3.1 item 7 is catch-up only and scoped to THIS invocation, so a re-run that found everything done reaches 3.1b with an empty queue (no duplicate comment); the guideline says the same on both the patch and the batch rule. - Step 2.1 gains the producing call site for `skipped` (unmet dependency, deferral): queued with its reason, not HALTed as `failed`, not silent. - Guideline lead-in said "Three properties" over four; the count is now asserted against the list length rather than spelled by hand. Refs: #220
Escalating — review<->fix loop round 4, human decision neededThree prior fix rounds converged on findings from the first review; round 4 (review of Rounds so farRound 1 (review of Round 2 (review of Round 3 (review of Across all three rounds, every recommendation was taken except where the PR argued a documented alternative (see the log for the per-round "Divergences from the recommendation" — each has a stated reason). Round 4 — still open (escalated, not fixed)
Convention for continuing this cycleAny further rework or re-review on this story from here — including manual, out-of-band rounds — should be appended to Note: that working log is an untracked file living only in the persistent authoring worktree Not merging. Awaiting human decision on the two Questions items and the DoD-locator gap above. |
rucka
left a comment
rucka
left a comment
There was a problem hiding this comment.
Verdict
risk:yellow · cost:green — APPROVED
Open findings: 0. All Major/Minor findings from prior rounds are resolved and verified on the current head
80dea663dce822db6ced0e691355a165ef95db0e.
PR: #468 · Story: #220 · Author/reviewer: rucka · Date: 2026-08-30
Classification
| Dimension | Assessment | Result |
|---|---|---|
| Criticality | shared delivery knowledge and review workflow | yellow |
| Change risk | command/documentation contract, no production runtime path | yellow |
| Business impact | developer workflow correctness | yellow |
| Security | no sensitive-data, auth, or trust-boundary change | green |
| Coupling | existing knowledge-hub components only | balanced |
| Cost | no billable/external-resource surface | green |
Existing labels remain correct: risk:yellow, cost:green.
Findings
Critical
None.
Major
None.
Minor — resolved
| Finding | Resolution | Verification |
|---|---|---|
| Definition-of-Done locator was implicit while task-ID lookup was explicit | Added a criterion-anchored, ambiguity-safe locator to source guidance and the shipped dataset mirror | TDD RED (2 failing tests) → GREEN; conformance suite 4,993 passing |
Questions / residual scope
The story's supervised live-run DoD remains intentionally unexecuted: it requires a real external card/PR and is explicitly outside this PR's side-effect-free verification scope. This is recorded, not a Major/Minor finding.
Review evidence
| Area | Result |
|---|---|
| Functionality / AC | Covered; task and DoD lookup contracts now reject missing or ambiguous targets without a write |
| Test-first remediation | Red test reproduced the omitted DoD locator; green implementation retained regression coverage |
| Target tests | @pair/knowledge-hub: 49 files, 4,993 tests passed |
| Quality gate | pnpm quality-gate passed on exact head |
| CI | build, preview, secret scan, smoke: passed |
| Security | Green — no new input, output, authorization, or vulnerability finding |
| Adoption | Level 1 conformant; no dependency, migration, or undeclared decision |
| Debt | None introduced |
| Documentation | Source guideline and packaged dataset mirror are synchronized |
| Performance / deploy | No runtime hot path, migration, or deployment impact |
Why prior reviews kept finding issues
Each remediation covered the reported path but not its adjacent contract boundary. Here, task-ID lookup was tested while the similar DoD lookup remained prose-only. The current tests assert both lookup contracts and their no-guess/no-write failure behavior, creating the required fixed point rather than relying on prose symmetry.
rucka
added
pr-state:ready-to-merge
Bound re-review to immutable deltas and require a complete finite-state inventory before review or remediation.
Require real boundary evidence for externally-defined state and repair claims.\n\nRefs: #416
80dea66 to
8666322
Compare
…st once Review round 1 on PR #468. - Step 3.1's commit-per-task skip targeted 3.2, jumping over the 3.1b flush: the recommended (and supervised-default) strategy posted no comment at all. Skip now targets 3.1b; new conformance assertion rejects ANY Phase-3 skip whose target ranks after 3.1b (heading order cannot see a jump). - Step 3.1b drains the queue and guards on "empty OR already flushed"; the HALT preamble flushes unless 3.1b already did — the Step 3.3 HALTs fire after the post, so they were producing a second comment per iteration. - Guideline: one write per checkbox — a task ticking its item AND a DoD box is two read-patch-write cycles, not a two-line diff the check rejects. - execution.mdx no longer promises locator/patch/fallback detail behind a catalog link that does not host it; it states the three rules inline. - Test header: mechanism asserted on both copies, wiring on the dataset, mirror equality delegated to skill-md-mirror. Refs: #220
…line it pins Review round 2 on PR #468. - The anti-jump detector read raw markdown (`Move to (?:Step|Phase)`) against a file written `Move to **Step 3.1b**`: it matched nothing before the flush, so the assertion passed on an empty set for ANY target. Emphasis is stripped first, and a mutation test restores the verbatim round-1 bug and requires the detector to report it. - Step 2.8 item 2 short-circuited commit-per-story to Step 2.1 BEFORE the tick-and-queue: a 4-task story red on T3 queued only the failure and left T1/T2 unticked. Item 2 now routes through items 7-8; assertion pins that the queue site is not inside a strategy branch. - Step 3.1 item 7 is catch-up only and scoped to THIS invocation, so a re-run that found everything done reaches 3.1b with an empty queue (no duplicate comment); the guideline says the same on both the patch and the batch rule. - Step 2.1 gains the producing call site for `skipped` (unmet dependency, deferral): queued with its reason, not HALTed as `failed`, not silent. - Guideline lead-in said "Three properties" over four; the count is now asserted against the list length rather than spelled by hand. Refs: #220
Questions are, by the review template's own definition, questions for the human: items the reviewer marks "No change requested". The floor defaulted to none, so they entered the set the fixer must resolve, and each answer enlarged the diff and created fresh surface for the next review. Measured on PR #477: the PR reached a zero-actionable APPROVED twice; both times the following round implemented the Questions and the re-review found new Minor findings inside the code that round had just added (three, then two). Work now completes when Critical, Major and Minor are closed. An explicit severityFloor still wins. The default is applied softly - a template without Minor, or without a usable ranking, falls back to no floor instead of throwing, because a default must not break a run that never asked for it. - ADL 2026-09-03-the-review-severity-floor-defaults-to-minor.md - three tests: Questions-only converges and is carried, a Minor still blocks, a vocabulary without Minor falls back Refs: #219 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… yaml-parsed allow-list (#477) The check-only pre-push gate was the sole enforcement point for formatting, and `--no-verify` skips it. CI now runs `format:check` in a dedicated workflow, and the workflow's own shape is guarded by a composition check. The guard parses `.github/workflows/format.yml` with yaml@2.8.2 and expresses every rule as an allow-list over the parsed document, per ADL 2026-09-01; the hand-rolled line reader it replaces is retired. Nothing is ever rewritten in CI — write-mode formatters are banned by the same guard. Also carries a path-scoped `.gitleaks.toml` allowlist for a synthetic 40-hex SHA fixture in scripts/smoke-tests, which gitleaks matched as a generic-api-key at historical commit 5e99d53. Closes #413 🤖 Generated with [Claude Code](https://claude.com/claude-code)
- new KB guideline task-progress-feedback.md: scope (one mechanism, /implement the only caller), task-ID anchored locator (tolerant renderings, exactly-one match, never guess-tick), tick-only diff-checked patch (never unticks) - conformance guard over dataset + generated mirror (16 assertions) - Task: T1 — Checklist locator + body patcher (task-ID anchored, tick-only edits) Refs: #220
…ocabulary - batching section: run iteration = one /implement invocation, flush once incl. early exit, empty batch posts nothing, never a second comment - D22 format: headline + one line per task, everything longer in <details> - closed outcome set (ticked/failed/skipped/not-found/ambiguous/patch-rejected/write-failed) with the per-outcome checklist effect — failure and skip stay unticked - Task: T2 — Comment batcher (one per iteration, D22 format) + failure/skip recording Refs: #220
- implement: /write-issue as an OPTIONAL composed skill, Step 2.8 ticks + queues per task, new Step 3.1b flushes one batched comment before the checkpoint hand-off, HALT flushes on the way out, Progress row in the output - write-issue: composition interface for /implement — write mode (caller-patched full body) for the tick, comment mode for the batch, neither failure load-bearing - loop: boundary — never posts its own per-task progress comments; the supervised path inherits implement's loop through implement-batch - skill versions bumped (implement 0.7.0, write-issue 0.10.0, loop 0.1.1) - Task: T3 — Wiring into implement (manual + supervised paths) Refs: #220
- guideline: writes confirmed by a read (never an exit status), body-conflict retry from a FRESH read with the locator re-run, exactly one retry, comment-only fallback, PM write failure never blocks the run - "What never happens": no task issues, no other section rewritten, no board state, no untick - ADL: one mechanism owned by implement, batched per invocation (5 alternatives weighed) - docs site: execution journey gains a Progress Feedback section; guidelines catalog lists it - conformance suite now 56 assertions over dataset + mirror + skills + docs - Task: T4 — Conflict/failure fallbacks + tests + docs site Refs: #220
…st once Review round 1 on PR #468. - Step 3.1's commit-per-task skip targeted 3.2, jumping over the 3.1b flush: the recommended (and supervised-default) strategy posted no comment at all. Skip now targets 3.1b; new conformance assertion rejects ANY Phase-3 skip whose target ranks after 3.1b (heading order cannot see a jump). - Step 3.1b drains the queue and guards on "empty OR already flushed"; the HALT preamble flushes unless 3.1b already did — the Step 3.3 HALTs fire after the post, so they were producing a second comment per iteration. - Guideline: one write per checkbox — a task ticking its item AND a DoD box is two read-patch-write cycles, not a two-line diff the check rejects. - execution.mdx no longer promises locator/patch/fallback detail behind a catalog link that does not host it; it states the three rules inline. - Test header: mechanism asserted on both copies, wiring on the dataset, mirror equality delegated to skill-md-mirror. Refs: #220
…line it pins Review round 2 on PR #468. - The anti-jump detector read raw markdown (`Move to (?:Step|Phase)`) against a file written `Move to **Step 3.1b**`: it matched nothing before the flush, so the assertion passed on an empty set for ANY target. Emphasis is stripped first, and a mutation test restores the verbatim round-1 bug and requires the detector to report it. - Step 2.8 item 2 short-circuited commit-per-story to Step 2.1 BEFORE the tick-and-queue: a 4-task story red on T3 queued only the failure and left T1/T2 unticked. Item 2 now routes through items 7-8; assertion pins that the queue site is not inside a strategy branch. - Step 3.1 item 7 is catch-up only and scoped to THIS invocation, so a re-run that found everything done reaches 3.1b with an empty queue (no duplicate comment); the guideline says the same on both the patch and the batch rule. - Step 2.1 gains the producing call site for `skipped` (unmet dependency, deferral): queued with its reason, not HALTed as `failed`, not silent. - Guideline lead-in said "Three properties" over four; the count is now asserted against the list length rather than spelled by hand. Refs: #220
- /write-issue: `$on-failure` arg + Step 8b — the three tracker-side HALTs (Step 7 not-found, Step 7b membership, Step 8 error) return `not-found`/`membership-unconfirmed`/`write-failed` to the caller; carve-out written at each site AND on the HALT Conditions rows, which is where an executing agent reads. Steps 1/3/6 still HALT. - /implement: `skipped` queued at most once per invocation (2.1 re-scans from the top after every task); Idempotent item 4 no longer says "Skips them" over the reserved outcome. - guideline: board-inertness scoped to the STATE field + names the transport's membership beats; the lost-with-its-session batch stated as a tradeoff. ADL Consequences + Alternatives updated. - suite 70 -> 86, every new pin mutation-proved. - Task: T4 — Conflict/failure fallbacks (review round 3) Refs: #220
Define a fail-closed DoD locator and retain task evidence.\n\nRefs: #220
8666322 to
f910723
Compare
7b55900 to
adb9627
Compare
1 participant
PR Information
PR Title: [#220] feat: breakdown-to-task feedback loop (close tasks + comment)
Story/Epic: #220 · epic #212
Type: Feature
Priority: Should-Have (P1)
Assignee: rucka
Labels: risk:yellow
Summary
What Changed
As
/implementadvances through a story's inline task breakdown, it ticks the completed checklistitem and posts ONE batched progress comment per run iteration. Manual and supervised runs share one
mechanism because the supervised path reaches it through
/loop→/implement-batch→/implementrather than reimplementing it.
task-progress-feedback.md— scope + one owner, task-ID checklist locator,tick-only body patch (four properties: full-body-overwrite transport, diff-checked write, one
write per checkbox, never unticks), batching + comment format (D22), outcome vocabulary and
where each outcome is produced, failure/conflict handling, what never happens.
/implement(v0.6.1 → 0.7.3):/write-issueadded as Optional; Step 2.1 owns theskippedpath (a task that cannot be attempted is queued with its reason, at most once perinvocation, not HALTed); Step 2.8 item 7 ticks and queues on every commit strategy (item 2
routes
commit-per-storythrough it instead of short-circuiting); Step 3.1 item 7 is catch-uponly, scoped to tasks completed in this invocation; Step 3.1b flushes exactly one batched
comment before the checkpoint step and drains the queue; Step 3.1's
commit-per-taskskiproutes through 3.1b; the HALT preamble flushes unless 3.1b already did;
Progress:row onthe output block.
/write-issue(v0.9.0 → 0.11.0): Composition Interface entry for/implement, plus a new$on-failureargument (haltdefault /report) and a new Step 8b — underreportthethree tracker-side HALTs return
not-found/membership-unconfirmed/write-failedto thecaller instead of raising. The carve-out is written at each HALT site and on the canonical
## HALT Conditionsrows, because that is what an agent executing the numbered steps reads.Argument/configuration HALTs (Steps 1, 3, 6) still fire for every caller.
/loop(v0.1.0 → 0.1.1): one Boundaries bullet — never posts its own per-task progresscomments.
task-progress-feedback.test.ts— 86 assertions over both guidelinecopies, the three skills and the two docs pages.
developer-journey/execution.mdx"Progress Feedback: Ticks and One Comment"(locator / one-checkbox-per-write / conflict rules stated inline, plus the non-blocking transport);
reference/guidelines-catalog.mdxPM-tools row.2026-08-28-task-progress-feedback-is-one-mechanism-owned-by-implement.md.Why This Change
An unattended run was invisible at task granularity — the checklist and the comments moved only at
PR-open/merge, so a story on task 3 of 4 and a story that failed on task 2 looked identical on the
board, and the only witness to the difference was a session that no longer exists. R4.6 / spec G10 /
D22 ask for that signal without a second implementation the manual and supervised paths could
drift apart on.
Story Context
User Story: As
/implementadvances through a story's inline task breakdown, tick the completedchecklist item and post ONE batched progress comment per run iteration, so an unattended run is
visible at task granularity. Manual and supervised runs must use one mechanism.
Acceptance Criteria coverage:
ticks and queues on BOTH commit strategiesno Phase-3 skip jumps over the flush(+ its mutation test),never flushes twice in one invocation,does not re-queue a task an earlier invocation completedskippedproduced at task selection (Step 2.1), at most once per invocation;failedat HALThas an explicit call site that queues \skipped`,queues `skipped` at most once per invocation, not once per scan,says where `skipped` is produced`/loopnever posts its own; supervised inherits via/implement-batch→/implement/loopBoundaries bullet + conformance assertion/write-issueOptional in Composed Skills and the tick composed with$on-failure: report(its Step 8b) so a tracker failure cannot HALT the callerStep 7 / Step 7b beat 4 / Step 8 returns … instead of HALTing,the canonical HALT Conditions list agrees with the steps,keeps the exemption narrowtask-progress-feedback.test.tsChanges Made
Implementation Details
cfa5533fc3892c47/implement(manual + supervised) —f1bc8684b83388f267a74098,6f86947b,608b77d2(folded into the properties above)Files Changed
.skills/capability/write-issue,.skills/loop,.skills/process/implement) and their generated.claude/skills/**mirrors;.pair/knowledge/.../project-management-tool/README.md(+ dataset source);.pair/llms.txt;apps/website/content/docs/developer-journey/execution.mdx;apps/website/content/docs/reference/guidelines-catalog.mdxpackages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/task-progress-feedback.md(+ its generated
.pair/knowledge/**mirror);packages/knowledge-hub/src/conformance/task-progress-feedback.test.ts;.pair/adoption/decision-log/2026-08-28-task-progress-feedback-is-one-mechanism-owned-by-implement.mdAll content is markdown/KB — no runtime code, no schema, no API, no dependency change.
Testing
Test Coverage
task-progress-feedback.test.ts— 86 assertions; whole@pair/knowledge-hubsuite 4982 passing (49 files).pnpm --filter @pair/website e2e— 39 Playwright tests passing.on issue Breakdown-to-task feedback loop (close tasks + comment) #220 and exactly one batched progress comment posted (
5453972232, headline**Task progress — 4 of 4 tasks this iteration**), with no accretion across the branch'sseveral
/implementre-invocations.Test Results
Testing Strategy
already-
[x]item; a re-invocation that did no work (must post nothing);commit-per-story(auto-selected for every single-task story); a HALT after the flush already posted; a task
blocked and re-scanned on every later pass.
an outcome to queue instead of a HALT that propagates into the run.
test when its target clause is deleted (11 mutations, one per pin).
Quality Assurance
Code Quality Checklist
Documentation
Documentation Updates
task-progress-feedback.md; docs-site section inexecution.mdx(locator / one-checkbox-per-write / conflict rules stated on the page, notpromised behind a link);
guidelines-catalog.mdxupdated.Knowledge Sharing
2026-08-28-task-progress-feedback-is-one-mechanism-owned-by-implement.md— one mechanism, owned by
/implement, batched per invocation; the transport does not HALT forthe tick (
$on-failure: report); a batch lost with its session is not recovered. Rejected:a loop-side implementation, a dedicated
/report-progressskill, one comment per task, onecomment at merge, a tick-only
$modeon/write-issue, a persisted cross-invocation queue.Reviewer Guide
Review Focus Areas
$on-failureas the carve-out shape. The tick rides/write-issuewrite mode, whoseHALTs propagate into the caller. Comment mode's exemption is keyed on
$mode, and atickmodewas already rejected (it would make
/write-issuean owner of the checklist grammar), so theexemption is keyed on a failure-policy argument instead. It is deliberately narrower than
comment mode's: only the three intermittent tracker-side failures. Is that the right line?
commit-per-storyhappens before the single commit exists (Step 3.1 commits). IsStep 2.1's completion rule — commit existence required only under
commit-per-task— the rightreconciliation, or should a
commit-per-storytick wait for the commit at the cost of AC1 on aHALTed run?
guarded ones (Step 3.1b itself and the HALT preamble)?
Not done (deliberately, flagged for the merge gate)
apps/pair-cli) updated" — unticked: n/a, no install/update/distribution surfacewas touched.
residual. The manual half is evidenced live on Breakdown-to-task feedback loop (close tasks + comment) #220 (four ticks + exactly one batched comment,
5453972232). The supervised half has NOT been run live end-to-end. The "holds byconstruction" argument has been refuted three times, each time inside
/implementwhere acomposition argument cannot look: the flush unreachable on the supervised-default strategy;
commit-per-storyreporting nothing per task; and an ordinary tracker error able to HALT anunattended run. Each is now fixed and conformance-asserted with a mutation proof that the pin
fires — mechanical evidence in place of the argument, not a replacement for the live pass. The
live pass was not performed because a supervised iteration implements a real card and opens a real
PR, side effects outside this story's scope. The merge gate decides whether it is required, or
whether the supervised half is accepted unverified end-to-end.
Testing the Changes
git checkout feature/US-220-breakdown-task-feedback-loop pnpm install pnpm --filter @pair/knowledge-hub exec vitest run src/conformance/task-progress-feedback.test.ts pnpm quality-gate