post_bot_commits counts local-tier agent commits as human rework - #1941
Conversation
…ommits-counts-local-tier-agent-commits
|
/prflow:review |
PRFlow Review — PR #1941Status: ✅ APPROVE with notes Blueprint
Verdict: APPROVE with notes (correct, well-tested bug fix to
|
|
/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 (correct, null-safe, well-tested bugfix; one Important test-coverage note and minor suggestions, none blocking at the critical threshold)
Issue Compliance
Reviewed against issue #1440: post_bot_commits counts local-tier agent commits as human rework — 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 five acceptance criteria are covered by verification items and pass:
- AC1 (blank-login tail → 0): VC-1 PASS (executed against a fixture).
- AC2 (named human still counted): VC-2 PASS.
- AC3 (merge commits excluded): VC-3 PASS.
- AC4 (comment +
cheap-gate.jqdescription updated): VC-4 PASS. - AC5 (
post_bot_commits > 0non-clean arm unchanged): VC-5 PASS (base↔head diff confirms comment-only change).
Test Evidence
CI for the reviewed commit 1bc72c5483b8b11659c50da5327ced6d098e24a4 is fully green (from the engine ground-truth block, read from the GitHub API): lib + python tests: success, lint (shellcheck + actionlint + ruff): success, and all five shards (modules-pin, python-pool, modules-rest, modules-large, monolith) success. The #1440 fixtures are run.sh-resident (monolith shard), which passed.
Verification Checklist Results
13 passed, 0 failed, 0 inconclusive — 0 via lite probe, 13 via agent.
✅ Passed items (13 of 13) — click to expand
- VC-1: blank-login tail reports
post_bot_commits: 0[lib/fetch-pr-context.sh] - VC-2: non-blank human login after last bot commit is still counted [lib/fetch-pr-context.sh]
- VC-3: merge commits (
parents_count > 1) remain excluded [lib/fetch-pr-context.sh] - VC-4: comment +
cheap-gate.jqfield description state the new rule (blank = agent-side) [lib/cheap-gate.jq] - VC-5:
post_bot_commits > 0non-clean arm unchanged (comment-only diff) [lib/cheap-gate.jq] - VC-6:
is_human/ends_botare null-safe via($l|type)=="string"— null/non-string does not abort the filter [lib/fetch-pr-context.sh] - VC-7: run.sh test extracts the shipped jq blocks; sed end-markers match the real terminators [lib/test/run.sh]
- VC-8:
_pbc/_pbssupplyCOMMITS/AUTHOR/DEVFLOW_JQmatching the shipped block's reads [lib/test/run.sh] - VC-9: coupled
POSTBOT_SHASapplies the byte-identicalis_humanpredicate [lib/fetch-pr-context.sh] - VC-10: changeset's "coupled blocks updated" claim matches the actual edits [.changeset/…]
- VC-11: blank/whitespace/absent/non-string login never classified human (executed falsifying inputs) [lib/fetch-pr-context.sh]
- VC-12: a tail commit by the PR author itself → 0 (self-exclusion via
!= $author) [lib/fetch-pr-context.sh] - VC-13: coverage-map
1440ownerunmodularizedconsistent with run.sh-resident tests [lib/test/modules/coverage-map.json]
Code Review Findings
🟠 Important / Major
- The new
#1440fixtures never assert a count greater than 1: every_pbcfixture that counts a human tops out at1and every_pbsfixture yields a single-element array. The suite therefore proves "≥1 post-bot human ⇒ counted" but not "N humans ⇒ N", and thePOSTBOT_SHASlist is only ever exercised at length 1 (SHA multiplicity/ordering unverified). A regression that collapsed the filtered array to a boolean/first-match would stay green. Adding one two-distinct-human fixture asserting2(and a matching two-element_pbslist) would close it. The shipped code is correct — this is a coverage-hardening note, not a defect. (raised by 1/5 agents) [suspected over-grade: shape 3 — single-source frompr-test-analyzer, no corroboration and no Phase-2 FAIL; observable fail-direction is a test-coverage gap against a hypothetical future regression, milder than the Important label. Advisory only; does not change the verdict.]
🟡 Suggestion / Minor
- The rewritten comment above the
POST_BOT_COMMITSblock inlib/fetch-pr-context.shruns ~12 physical lines and carries derivation/rationale (the account-resolution narrative) beyond the specific wrong change it prevents; under the prevention-only comment standard for library source it could be trimmed, with the narrative left to the changeset/overview prose it already duplicates. Accurate, not misleading. (raised by 1/5 agents) - No fixture exercises a human commit before the last bot/PR-author anchor (e.g.
[human, bot, human2]), which would pin the slice-boundary[($bot_indices | last)+1:]against an off-by-one. (raised by 1/5 agents) - No fixture places a blank-string login commit between two bot commits (
[bot, blank, bot]→ expect0); low value since the anchorselectdemonstrably ignores blanks. (raised by 1/5 agents) - A genuine human whose API login is blank/null (email not linked to any account, not only the local-tier agent identity) is now classified agent-side and undercounted — intentional and documented ("unknown is not a human"), failing in the safe direction for this cheap-gate trigger. No change required. (raised by 1/5 agents)
over-grade annotation: 1 finding flagged (Important test-coverage note, shape 3 — advisory only).
truthfulness sweep: no finding promoted — every diff-added comment and the DEVFLOW_SYSTEM_OVERVIEW.md prose verified true against HEAD.
intra-diff contradiction scan: no contradiction found.
Verdict Criteria
Threshold: critical (default). No checklist FAIL/INCONCLUSIVE; no self-contradicting-diff finding; no finding at or above critical. Only findings below the threshold are present → APPROVE with notes (rule 6). Full checklist ran (engine_self_modifying profile — no skip); 0 of 5 Phase-3 agents failed.
Notes
- The change is a focused, correct bugfix. New
ends_bot/is_humanjq defs type-guard (($l|type)=="string") before every string op, which also fixes a pre-existing latent null-abort in the anchorselect(null | endswithwould have errored). The coupledPOSTBOT_SHASblock, thelib/cheap-gate.jqfield description, the block comment, and the overview prose were all updated in lockstep and are mutually consistent. Tests drive the extracted shipped jq bytes rather than a transcribed copy — the right call for this surface. - Diagnostic (non-blocking): the final-pass reviewer reported
EXTENSION-STATUS: loaded-empty resolved-root=unsetfor therequesting-code-reviewextension while the orchestrator resolved a trustedreviewextension root; recorded as anunestablishedhop-two propagation state per the engine's cross-check (local/interactive tier — held in-run, no reflection sink written).
…ommits-counts-local-tier-agent-commits
…k comment (#1440 review) Addresses the PR #1941 review's Important coverage note and Suggestion 1. - Adds a four-commit fixture (human before the anchor, human author ON the anchor, two distinct humans after it) asserting `2` on POST_BOT_COMMITS and a two-element ordered list on POSTBOT_SHAS. Mutation-checked against three mutants of a copy of lib/fetch-pr-context.sh: collapsing the filtered tail to a boolean (1 instead of 2), dropping the `+1` from the slice (3 instead of 2), and truncating POSTBOT_SHAS to its first match (["B"] instead of ["B","C"]). Each mutant is caught only by the new rows; the pre-existing #1440 fixtures stay green on mutants 1 and 3, which is the gap the review named. - Trims the POST_BOT_COMMITS block comment from 12 lines to 6 under the prevention-only comment standard: the account-resolution narrative, the unchanged-anchor note and the trivial-fixup aside move out (the changeset and DEVFLOW_SYSTEM_OVERVIEW.md already carry them); the AC4-mandated specification statement and both prohibitions stay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on guards (review) Addresses the review-and-fix engine's REJECT driver and its Important/Suggestion findings. REJECT driver — lib/cheap-gate.jq contract header contradicted its own producer. The header said the count is of commits "after the last bot push"; the producer (and lib/fetch-pr-context.sh's sibling comment, written by the same change) anchors on the last bot-OR-PR-author commit. Reproduced: [claude[bot], alice, bob] with AUTHOR=alice yields 1 from the shipped block, 2 under the header's stated rule. The header also read as a claim about the commit while the predicate is an OR over the two logins; both are restated. Important — the "extracted cleanly" guards checked the START marker, which the sed range guarantees present. Replaced with an end-marker check plus a length ceiling. The end-marker check alone is insufficient: a reindented terminator makes sed run on to the producer's NEXT identical terminator, so the extraction still ends in `')"` while carrying 438 extra lines that `bash -c` then executes with live side effects (read-only-filesystem writes observed). Mutation-checked against a reindented-terminator copy: the length guard reports overran:438. Also in this batch: - Pin the two blocks' coupled ends_bot/is_human defs byte-identical (mutation-checked: a one-character edit to the SHAS copy reports differ). - Fixtures for the PR-author re-anchor ordering and the zero-anchor branch, and a shared blank+merge+pre-anchor input asserted on both blocks. - `unset -f _pbc _pbs`, so a later caller cannot abort the suite under `set -u`. - Drop the false "(the other rows all set author==committer)" parenthetical and the comment implying `!= $author` is regression-covered; it is unreachable. - Changeset: drop the null-abort over-claim (the producer normalizes an absent login to "" before the filter sees it) and state the OR semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…spective skill (review) The review engine flagged both rows as describing a superset of what `lib/fetch-pr-context.sh` now emits: `human_postbot_diff` said "commits AFTER the bot's last commit" with no mention of the merge or human-attribution filters, and `post_bot_commits` said "Substantive commits by a human" while a human whose GitHub login is unresolvable is excluded. Both rows now state the positive-attribution rule and the exclusion set, and both name the real anchor (the last bot/PR-author commit) rather than "the bot's last commit" — the same imprecision this branch's REJECT corrected in lib/cheap-gate.jq's header. The rows ship verbatim into consumer repos, so they cite only files a consumer has. No test added: agent-executed prompt prose with no tool reader carries no automated regression coverage by design. Writing-skills evidence: skill-loaded=yes (superpowers:writing-skills invoked via the Skill tool in a dispatched subagent before the target file was opened); guidance-applied=yes (its token-efficiency and no-redundancy guidance folded a duplicated merge-commit clause into one exclusion list and kept both cells to a single line); pressure-scenario=no (the failure was two reference cells naming a superset, not a rule an agent could be pressured to break, so no scenario applies); micro-tests=no (descriptive schema documentation with no behavior-shaping wording to A/B; correctness was verified directly against the two shipped jq blocks instead). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/prflow:review |
PRFlow Review — PR #1941Status: ✅ APPROVE with notes Blueprint
Verdict: APPROVE with notes (correct, null-safe, well-tested fix; only minor test/comment suggestions)Issue ComplianceReviewed against issue #1440: post_bot_commits should not count blank-login (local-tier agent) commits as human rework — criteria from the Criteria came from the All five acceptance criteria are satisfied:
Verification Checklist Results12 passed, 0 failed, 0 inconclusive — 1 via lite probe, 11 via agent/direct verification. ✅ Passed items (12 of 12) — click to expand
Code Review Findings🟡 Suggestion / Minor
over-grade annotation: no finding flagged Verdict Criteria applied
AssessmentThe change is a correct, tightly-scoped, well-tested fix. The new |
…claims the fix added Fix-delta gate findings on the previous commit — the same documented-falsehood class as the REJECT that commit was fixing. - lib/test/run.sh: "ceilings on blocks of 18 and 21 lines" was false; both blocks are 18 lines. - lib/test/run.sh: the `unset -f` rationale was the opposite of the truth. Under `set -u` an unbound deref inside `$( )` exits only the command-substitution subshell, so a later caller fails one row rather than aborting the suite. All 18 call sites are inside `$( )`. Reason restated; `unset -f` kept as hygiene. - lib/test/run.sh: the coupled defs are not byte-identical — they differ by indentation, which the comparison strips. Now says so. - skills/retrospective/SKILL.md: "the same set post_bot_commits counts" overclaimed; the per-commit patch fetch is best-effort, so the delivered diff can be a proper subset and is null when no patch is available. The gate also showed the guards only REPORTED a bad extraction. On a drifted terminator all 18 fixture rows still ran 439 lines of the producer for real (observed: "/gh-checkruns.err: Read-only file system"). The block now classifies each extraction (empty-extraction / terminator-drift / overran:N) and blanks the program to `:` before `_pbc`/`_pbs` are defined, so a bad extraction fails RED with a named cause and executes nothing. Verified against a reindented-terminator copy: overran:438, blanked, no side effects; and against the real file: ok, 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ommits-counts-local-tier-agent-commits
|
/prflow:review |
PRFlow Review — PR #1941Status: ❌ Review failed Review run failed: claude step cancelled — https://github.com/The01Geek/prflow/actions/runs/32908025809 |
|
PRFlow review: no verdict-post receipt was found for this run.
No run-scoped verdict-post receipt was found for this run: either Phase 4.4's No review authored by this run's reviewer identity is recorded in the reviews Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker, This comment is a record of that gap. It is not a verdict, and it neither approves nor |
1 participant
Summary
post_bot_commitsfield miscounted the engine's own local-tier agent commits as human rework, because those commits carry a git identity GitHub cannot resolve to an account (a blank login) and the old filter counted any non-[bot]login as human.Changes
lib/fetch-pr-context.sh: ThePOST_BOT_COMMITSjq block now counts a non-merge commit after the last bot/PR-author commit only whenis_humanholds for itsauthor_loginorcommitter_login— a non-blank string (at least one non-whitespace char) that neither ends in[bot]nor equals the[bot]-stripped PR author. Theis_human/ends_bothelpers type-check first, so anulllogin no longer aborts the filter. Merge commits (parents_count > 1) stay excluded. The coupledPOSTBOT_SHASblock carries the byte-identical predicate. The explanatory comment above the block is updated to state the new counting rule, including the agent-side classification of blank logins.lib/cheap-gate.jq: Thepost_bot_commitsfield description in the header comment is updated to state that a blank/absent login is classified agent-side, never human. Thepost_bot_commits > 0non-clean arm is unchanged.lib/test/run.sh: A#1440fixture block drives the shippedPOST_BOT_COMMITS/POSTBOT_SHASprograms directly against fixture commit arrays — blank-login tail, whitespace-only tail, mixed tail with one named human,null-login record (no filter abort), merge-commit exclusion, missingparents_count, eachis_humanOR arm one-sidedly, PR-author self-exclusion, and the coupledPOSTBOT_SHASSHA selection.lib/test/modules/coverage-map.jsonanddocs/internal/DEVFLOW_SYSTEM_OVERVIEW.md: coverage entry and the retrospective'spost_bot_commitsclause updated to match the new counting rule.Resolves
Resolves #1440
Test Plan
lib/test/run-shard.sh monolithpasses (the#1440block isrun.sh-resident); observed 9850 passed / 0 failed / 0 skipped at head ca72fb7.post_bot_commits: 0.nulllogin does not abort the jq filter.Visual Changes
N/A
Breaking Changes
None. The field's type and name are unchanged; only its counting rule changes, and historical retrospective records are not rewritten.
prompt-surface size: ran
.prflow/vendor/prflow/scripts/prompt-surface-growth.py(vendored path; the prefix-stripped fallback was not needed). Reading: no tracked*.mdunderskills/,agents/, or.prflow/prompt-extensions/changed on this branch — no table rendered.