Skip to content

feat(pipeline-conductor): ask all five claim questions, not one - #8036

Merged
iamwhatever merged 1 commit into
mainfrom
feat/conductor-claim-preflight-8029
Sep 3, 2026
Merged

feat(pipeline-conductor): ask all five claim questions, not one#8036
iamwhatever merged 1 commit into
mainfrom
feat/conductor-claim-preflight-8029

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

The pipeline conductor decides whether to claim a work item with one question.
Its predicate is a single prose line in the skill body, gh pr list --search,
and an empty answer is read as permission to dispatch a worker. That predicate
is blind in three directions at once, and each blind spot has already cost a
whole dispatch to discover the work did not exist:

  • A merged PR is invisible. An item already fixed was claimed and dispatched
    days later, with the reporter's own "happy to have it closed" sitting on the
    thread. An --state open query structurally cannot see a merged PR.
  • One field answered empty for items that had an open PR. Four dispatches
    went to items that each carried Fixes #N on an OPEN PR, because the
    predicate read closedByPullRequestsReferences. That field still answers
    empty: measured on this repo just now, items 7597 and 8007 are both closed by
    merged PRs and both answer [].
  • A claim written in prose is invisible. Three items said "I am claiming
    this issue" / "Ownership claimed by @x" in the body, which no label or field
    query sees.

Why it matters

A wrong claim is not a cheap mistake. It stands up a worker session, clones a
worktree, burns credits and an agent's whole context, and ends with the worker
discovering there was nothing to do -- or worse, opening a second PR against a
file another PR already owns. The three failures above are not three bugs; they
are one design defect appearing three times, and the defect is asking a single
question and treating silence as a yes.

What changed

Motivation, then approach, then the change.

Goal: a claim decision that cannot be wrong in a way one more cheap question
would have prevented.

Approach: ask all five questions in one call and return one verdict, and
make an unanswerable question return UNKNOWN rather than fall through to CLAIM.
The alternative -- adding a second query to the existing prose predicate -- was
rejected because it fixes one blind spot and leaves the shape that produces
them: prose in a skill body cannot be tested, and the next missing question
costs another dispatch to find.

What was built: claim_preflight.py, one new script in the pipeline-conductor
skill.

python3 claim_preflight.py --repo owner/name --item N [--default-branch main]
                          [--repo-dir <clone of the base>] [--json]

The verdict rides the exit code, because that is what the conductor branches on:
0 CLAIM, 10 SKIP, 11 CLOSE (triage debt), 3 UNKNOWN, 2 malformed
arguments. Human form is one line (CLOSE 8088 merged-pr=#8092 sha=06065e5165 landed=true); --json prints exactly one object carrying all five checks.

The five checks, and what each one is for:

  1. open_prs -- open PRs referencing the item, fork PRs included, with
    author and is_cross_repository per hit.
  2. merged_prs -- merged PRs, each annotated landed by
    git merge-base --is-ancestor <mergeCommit> <default-branch> and
    closes_item by a closing keyword aimed at this item. Both are load-bearing:
    a PR merged somewhere other than the branch a worker would start from is not
    coverage, and a PR that merely MENTIONS the item is not closure either.
  3. prose_claim -- the body and the NEWEST human comment, for self-claim
    phrases and closure requests, both of which require standing (the reporter or
    a repository insider) for opposite reasons.
  4. symbol_on_base -- every symbol the item names, by git grep on the default
    branch. Absence means the target code MAY live only on an unmerged branch --
    but that reading holds for a bug item, not for a feature request naming the
    symbol it proposes to add, so it vetoes only when the item's own metadata
    corroborates bug-class, and otherwise downgrades to CLAIM risk=high.
  5. recency -- age and authorAssociation. A fresh item from an active
    contributor is a high self-claim risk, surfaced as risk=high, never a veto.
    The consumer is the skill: risk=high means the item is not batched -- it
    gets a live re-check immediately before the atomic claim.

closedByPullRequestsReferences is deliberately not a check. It measured
[] on two items that were closed by merged PRs, and a per-candidate forge call
that cannot change the verdict is pure cost against a shared rate limit. The
timeline answers the same question correctly, including for fork PRs.

Precedence is first-match-wins and lives in verdict(checks), a pure function
of a dict: merged-and-landed gives CLOSE, an open PR gives SKIP, a closure
request gives CLOSE, a prose claim gives SKIP, an absent symbol on a
corroborated bug item gives SKIP, any errored check gives UNKNOWN, and otherwise
CLAIM. The error rule sits below
the positive findings on purpose: a definite answer to one question outranks a
partial view of another, and no error path can reach CLAIM.

Six properties worth naming, because they are where this kind of script usually
goes wrong. The last four are rules that measurement produced and reasoning did
not:

  • Unknown is not "no". Three answers are possible for ancestry, and the
    third is not False. A merge commit missing from a stale clone reports
    UNKNOWN, never "did not land" -- reading it as "did not land" is precisely how
    the already-fixed item got dispatched. Same rule for a failed git grep: an
    unsearchable tree is not an absent symbol.

  • No writes, enforced rather than promised. run_gh refuses any argv that
    is not on a read allowlist before a subprocess exists, so a mutating method,
    a -f field (which makes gh api a POST), or an issue close is rejected in
    process. A future edit that adds a write has to defeat the allowlist, the
    source-level test, and the test that inspects every argv of a full run.

  • The prose scan reads what an author SAYS, not what they QUOTE. Code
    fences, backtick spans, blockquotes and quoted spans come out before matching.
    See Manual verification: the item specifying this script quotes the closure
    phrases as a description of what to detect, and a raw scan returned CLOSE on
    live work.

  • The newest human comment is chosen by timestamp, never by position. The
    per-issue comments endpoint documents only since/per_page/page: it
    silently ignores sort/direction and answers oldest-first. Selecting by
    max(created_at) means an endpoint that changes its order cannot reintroduce
    the bug, which is stronger than flipping to the opposite assumption.

  • A merged PR is coverage only if it CLAIMS to close the item. A closing
    keyword (Closes #N, Fixes #N, including the owner/repo#N and full-URL
    spellings) in the PR title or body, not a bare cross-reference. A mention
    decides nothing and falls through: reading it as coverage closes live work, and
    reading it as a claim starves an item whose fix was only partial. The match is
    deliberately negation-blind, matching GitHub's own parser, so this script's
    reading and the forge's reading stay identical.

  • Both prose phrase sets need standing, for opposite reasons. A closure
    request produces CLOSE, which acts on live work, so "please close" from a
    passer-by must not fire it. A self-claim produces SKIP, and a veto any
    commenter can cast is a denial-of-work channel -- one comment would suppress a
    queued item indefinitely with nothing downstream reporting the suppression. So
    an unauthorized claim is neither obeyed nor discarded: it annotates risk=high
    and takes the live recheck. Standing is the item's own author (always true of
    the body) or an insider by author_association; CONTRIBUTOR alone is not
    authority over another person's report.

  • An open fork PR still SKIPs, but not silently. Opening a fork PR needs no
    permission, so this check is a suppression channel anybody can use: mention an
    item from a throwaway fork and it leaves the queue. Refusing to trust fork PRs
    is the wrong trade -- 192 of this repository's 301 open PRs come from forks, so
    dropping them reinstates the duplicate-dispatch class this script was built
    from, repeatedly, to close a channel that costs an outsider one PR. The
    objection worth answering was never the DETECTION; it was a response that was
    unconditional AND silent. So the verdict does not move and the doubt is
    published with it: an unvouched fork's SKIP carries untrusted-fork=true risk=high. Standing is an insider association or the item's own reporter,
    since fixing your own bug from a fork is the ordinary case rather than an
    attack, and an unknown reporter marks MORE items rather than fewer.

    The annotation's consumer is the conductor's review of untrusted-fork
    suppressions, specified in the conductor's protocol now and landing in the
    skill in a follow-up. A printed annotation nothing acts on would be the same
    defect this PR removed elsewhere, so it is named rather than assumed.

  • An absent symbol vetoes only a corroborated bug item. The evidence behind
    that check was bug items whose target span lives on an unmerged branch.
    Applied unconditionally it parks any feature request naming a symbol it
    proposes to add -- and parks it on every pass, not once, because the symbol
    stays absent. Corroboration is explicit metadata (a bug/defect/regression/
    crash label, or the issue type), never prose: guessing the class would put an
    unmeasured heuristic in front of the very veto that was over-applied. An
    uncorroborated item is dispatched at risk=high instead, which costs at most
    one dispatch against a park that costs the item.

  • Nothing user-authored reaches stdout. Failures are reported as slugs, not
    forge stderr, and a prose match reports the pattern that fired, never the
    matched sentence. This output lands in an agent's context.

--repo-dir is optional in a precise sense: it is needed only for the two
questions git can answer, so an item with no merged PR and no named symbol never
needs a clone. When one of those questions does arise and there is no clone, the
answer is UNKNOWN rather than a guess in either direction.

Tests

test/test_pipeline_conductor_claim_preflight.py, 203 tests, 99% line coverage
on the new file (the two uncovered lines are sys.exit(main()) and one partial
branch). Every precedence branch has a test that also asserts the old
single-question predicate would have said CLAIM
on the same item -- a test
that only checked the new answer would not notice the script regressing back
into the old blind spot.

  • Verdict branches: merged-and-landed gives CLOSE already-fixed; merged but
    landed elsewhere falls through to CLAIM; an open fork PR gives SKIP open-pr;
    a prose self-claim by another user gives SKIP prose-claim; a closure request
    in the newest comment gives CLOSE reporter-asked-close; an absent symbol on
    a bug-labelled item gives SKIP symbol-absent; each of the five checks
    erroring gives UNKNOWN and exit 3; a clean item gives CLAIM with risk=high
    for a fresh item from an active contributor.
  • The item class an unconditional symbol veto parked: an absent symbol with no
    bug-class metadata gives CLAIM risk=high, not SKIP, both as a verdict-level
    test and end to end. A bug LABEL and an issue TYPE each corroborate; seven
    bug-ish label spellings are accepted and five non-bug ones (enhancement,
    feature request, documentation, ...) are not; junk metadata degrades to
    "not corroborated" rather than raising.
  • Precedence pins: landed-merged outranks an open PR; a closure request outranks
    a prose claim; a definite finding outranks an errored check. The dropped field
    is pinned as dropped: five check names, absent from the source, and no run
    issues that forge call.
  • Comment selection: the newest human comment wins over an older one and over
    two bot comments, with the real thread's own timestamps; reversing the input
    list does not change the answer
    , which is what makes the fix order
    independent rather than the opposite assumption; position breaks ties only
    when a timestamp is absent.
  • Standing: the reporter and an insider (MEMBER) can both request closure; a
    stranger with NONE and a drive-by with CONTRIBUTOR cannot. A self-claim needs
    no standing, because it produces SKIP, which is the cheap direction.
  • Citations: a quoted closure phrase, one broken across a line break exactly as
    markdown hard-wrapped it in the real body, a fenced block, an inline code
    span, a blockquote and a curly-quoted span all fail to request closure, while
    the same sentences unquoted still do.
  • Every exit code asserted explicitly (0/10/11/2/3), including malformed
    --repo, a non-positive --item, a blank --default-branch, and a
    --repo-dir that is not a clone.
  • --json emits exactly one parseable object with all five checks present, on
    the UNKNOWN path too.
  • No writes: the read allowlist accepts five real read shapes and rejects ten
    write shapes; a refused argv is proven never to reach a subprocess; the script
    source is scanned for write verbs; and a full run's every argv is asserted to
    be a read.
  • Against real git (a two-branch repository built in tmp_path), because a
    stubbed run cannot catch a wrong flag: --is-ancestor distinguishes landed
    from merged-elsewhere, a commit absent from the clone reports
    ancestry-unknown rather than landed: false, and git grep finds a symbol
    on one branch and not the other.
  • The forge shapes are fixtures taken from real responses on this repo, not
    invented: a timeline whose events are mostly commented, labeled and
    referenced with one cross-reference pointing at an issue rather than a PR,
    and a duplicated reference that must produce one detail call, not two.

Manual verification

Ran against live items, which is what produced two of the rules above.

item verdict what it proves
8088 CLOSE 8088 merged-pr=#8092 sha=06065e5165 landed=true blind spot 1: a merged PR carrying Closes #8088 that landed on the base, which an --state open query cannot see
7597 SKIP 7597 open-pr=#8035 fork=false author=chenmingwei23 the closing-keyword condition, on the item that exposed its absence -- see below
8029 SKIP 8029 open-pr=#8035 fork=false author=chenmingwei23 blind spot 2, detected through the timeline rather than the empty field
8031 CLAIM 8031 risk=high the false-park class, on a real item: its only labels are readiness: checking and fork, so nothing corroborates bug-class. An unconditional symbol veto answered SKIP symbol-absent=PARTIAL_TURN_MARKER here
8007 CLAIM 8007 risk=low the clean path

The fork marker on live data. Items 6799 and 6509 answer
SKIP open-pr=#8146 fork=true author=LuisBrel untrusted-fork=true risk=high and
the equivalent for #8140 -- both first-time-contributor fork PRs, so the
suppression now arrives with its own doubt attached. Item 8071 is the control: it
is also behind an open fork PR (#8107) and is deliberately NOT marked, because
catoneone both reported 8071 and is fixing it from their fork. The verdict is
SKIP open-pr in all three cases -- the marker changes what the conductor is
told, never what the script decides.

A merged PR that only mentions the item. Item 7597 is why the
closing-keyword condition exists, and it caught a false CLOSE in this script's own
earlier behaviour. It has TWO landed merged PRs referencing it -- #7613, titled
docs: investigation for #7597, and #7313 -- and neither carries a closing
keyword. Without the condition the verdict was CLOSE 7597 merged-pr=#7613 landed=true, which would have closed an item still being fixed in open PR #8035.
With it, both are recorded landed: true, closes_item: false and decide nothing,
so the open PR is what produces the SKIP.

Citations. The first run of item 8029 returned CLOSE ... reporter-asked-close, which was wrong: that item's body is this script's own
specification, so it quotes the closure phrases as a description of what to
detect. A false CLOSE closes work in flight, so the scanner now removes
citations before matching. The first version of that stripper still missed the
real body, because markdown hard-wrapped the quotation across a line break while
the phrase patterns matched across it; whitespace is now collapsed before the
span strip, so the stripper is exactly as newline-tolerant as the phrases it
defends.

Comment ordering. Measured directly against the endpoint: item 7597 has 12
comments, and ?per_page=1&sort=created&direction=desc returned id 5492699917
at 2026-09-01T10:40:23Z -- the OLDEST -- against a newest of id 5518670706 at
2026-09-03T00:56:58Z. The parameters are silently ignored. Re-verified after
the fix against the same live thread: the selector now returns 5518670706. A
reporter's later "please close" was previously unreachable by the check that
exists to find it.

Pagination. gh api --paginate on these array endpoints merges pages into a
single JSON document: measured on gh 2.96.0, the timeline of item 7597 at
per_page=8 returned 4 pages as one parseable array of 33 events, and the
comments endpoint at per_page=5 returned 3 pages as one array of 12. So a
paginated read stays a single parse and needs no --slurp -- which is in any
case rejected in combination with --jq.

Also verified locally on the rebased tree: black, isort, flake8, mypy,
the black gate, the agent-sdk-boundary gate, the builtin-skill-scope gate (this
script ships to every install, so it names no repository path), the testpaths
coverage gate, the loop-bound-locks gate, and
test_spawn_audit.py::test_bundled_skill_assets_are_not_imported.

Related Issues

Refs #8029

Any other suggestions

  • The script does not check whether the item is open. Item 8007 is closed and
    answers CLAIM. That is correct against the contract -- the conductor feeds it
    candidates from an open-item query -- but the issue payload it already fetches
    carries state, so a seventh check is nearly free. I left it out rather than
    widen the verdict surface unilaterally while the skill documentation for these
    exit codes is being written in parallel.
  • The closure and self-claim phrase lists are English and hand-written. They
    cover the phrasings actually observed. A phrasing outside them yields CLAIM,
    which is the cheap failure direction, but the lists will need to grow from real
    misses rather than from imagination.
  • The clone is read, never fetched. Keeping --repo-dir current is the
    caller's job. A stale clone degrades to UNKNOWN rather than to a wrong answer,
    which is the right failure, but a conductor that never fetches will see UNKNOWN
    on every recently merged PR.
  • recency is annotation only. It reports risk=high and nothing acts on it
    yet. It earns its place when something does -- the dispatch decision, or a
    pre-dispatch comment asking the author whether they are already on it.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 3, 2026 00:31
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of a53776b14e096343ba08794ed575853cf6ca2bbf — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound fail-closed redesign; the one risk left is that prose-derived CLOSE — the verdict that writes to live work — rides a hand-tuned regex engine.

Watch

  • Exit 11 tells the conductor to close the item unconditionally, but reporter-asked-close rests on English phrase regexes whose guards (negation window, clause-end, citation stripping, unclosed quotes, HTML comments) were each added after a measured false CLOSE during this PR's own development ("this list has now produced four separate false positives"). Standing-gating confines the blast to misread reporter/insider prose, but the next unseen phrasing closes someone's live item unattended — the guard list is structurally reactive, and every future miss in that direction is paid by a user, not by a dispatch.
  • The untrusted-fork risk=high marker's consumer is deferred ("landing in the skill in a follow-up"); until then an unvouched fork PR mentioning an item still delists it with the doubt merely recorded — the annotate-but-nobody-acts defect the PR itself names as the failure mode.

Suggestions

  • Split the CLOSE verdict: keep exit 11 for git-verified already-fixed, and give prose-derived closure its own code (or confirm=true marker) so the skill can require the conductor to read the thread before the one write driven by regex-interpreted prose.

[DESIGN-REVIEWED] a53776b

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for a53776b14e096343ba08794ed575853cf6ca2bbf; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt a53776b14e096343ba08794ed575853cf6ca2bbf: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of a53776b14e096343ba08794ed575853cf6ca2bbf — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. The base branch already carries the consumer and the contract (SKILL.md dispatch step 3 with the exit-code table and five-check precedence, agent.py's conductor prompt, the skill-contract test, the design doc) — this PR supplies the script those cite. No duplicate mechanism exists (issue_radar's timeline reads back a dashboard detail pane, a different job in a different runtime; grepped /timeline|closedByPullRequestsReferences|pr list --search across src/: 6 files, none a claim predicate). Final review:

First-Principles-Verdict: PASS

Every check is a decision the repo already recorded — base SKILL.md cites this exact script, exit codes and all — and each blind spot has a counted dispatch it cost.

What this change ships

Intent: stop the conductor burning a worker dispatch on items already fixed, claimed, or covered — an ADDITION supplying the script the base skill already mandates.

  1. Claim step gains a scripted verdict (CLAIM/SKIP/CLOSE/UNKNOWN by exit code) — justified: base SKILL.md step 3 cites it.
  2. Item fixed by a merged, landed PR returns CLOSE instead of dispatching — justified: reported defect.
  3. Item with any open PR, forks included, SKIPs — justified: four reported wasted dispatches.
  4. Prose claim or closure request decides, gated on standing — justified: three reported items.
  5. Bug-labelled item naming a symbol absent from base SKIPs — justified: documented base contract check 5.
  6. Fresh insider item dispatches un-batched at risk=high — justified: base SKILL.md risk semantics.
  7. Untrusted-fork SKIP marked loud instead of silent — justified: external-content suppression channel, named boundary.
  8. Script structurally cannot write to the forge (argv allowlist) — justified: agent-untrusted boundary.
  9. --json single-object evidence form — justified: in the base usage contract.
  10. Any unanswerable question yields UNKNOWN, never CLAIM — justified: this is the cause-level fix ("silence read as yes"), not a symptom patch.

The change sits at cause level: the three failures shared one defect (one question, empty answer read as permission), and the design removes that shape rather than patching each miss. Deliberate misses ("close the ticket please", happy to have it closed) are disclosed and pinned toward the cheap failure direction. The two remaining collision arms (branch, worktree) stay with the conductor by the base contract's own split — recorded, not overlooked.

Subtractions

  • Delete test_no_file_claims_a_check_count (test/test_pipeline_conductor_claim_preflight.py) — it greps its own source for the English words "six/four/seven" near "check"; CHECK_NAMES already pins the real count, a stale "three checks" would pass it anyway, and an innocent future comment containing "four" and "check" fails the build.

[FIRST-PRINCIPLES-REVIEWED] a53776b

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed a53776b14e096343ba08794ed575853cf6ca2bbf — this comment is updated in place on each push.

Review details

Both candidates concern claim_preflight.py, which I read in full.

CANDIDATE 1 (glued gh flag forms bypass the no-write allowlist): The bypass is technically real — --field=x, -fbody=x, -XPOST do evade the whole-token membership tests. But every run_gh caller in this script passes a hardcoded read argv (gh api repos/…/timeline, …/pulls/N, …/issues/N, …/comments, gh api user); none carry -f/-F/-X/--field/--input, and the only interpolated components (repo, item, number) are constrained by _REPO_RE, an int type, and an isinstance(number, int) guard, so no flag can be injected. There is no concrete input that occurs in practice (a), no call path that reaches a mutating argv (b), and no write happens when the code executes (c). The candidate itself concedes this is "not a live reachable write" — a defense-in-depth gap against a hypothetical future caller, which is exactly the "if a caller were to" class to drop.

CANDIDATE 2 (first-open-PR wins, so untrusted_fork marker dropped when a trusted PR is listed first): Self-rated low/"likely benign." When a trusted same-repo open PR grounds the SKIP, that is genuine coverage and the loud marker is by design unnecessary — the marker exists to surface a suppression an outsider could manufacture, which a legitimate PR is not. The only effect is JSON risk=high vs. a plain human line, not a wrong verdict. Also, the pasted evidence contains {ninja, which does not appear at the cited location — the quote is ungrounded. Well below the bar.

No self-originated finding rises to the 80+ / (a)(b)(c) bar; the code is tightly guarded and its argv surface is closed.

No findings.

[OPUS-REVIEWED] a53776b

Verdict parsed from the review's SHA-scoped output markers for commit a53776b14e096343ba08794ed575853cf6ca2bbf.

False positive or not applicable? A repository writer can comment:
/ai-review override fable a53776b14e096343ba08794ed575853cf6ca2bbf: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from 54b9c8a to d49e00c Compare September 3, 2026 01:03
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 1 dispositions for 54b9c8ad0, now pushed as d49e00cd8. Two findings were real and are fixed; one is a measured false positive. Each disposition below is a measurement, not an opinion.

Opus 4.8, claim_preflight.py:744 -- the comment scan read the OLDEST comment. FIXED, and the finding was exactly right.

Verified against the live endpoint before changing anything. Item 7597 has 12 comments; GET /repos/kirodotdev/KiroCrew/issues/7597/comments?per_page=1&sort=created&direction=desc returned:

5492699917  2026-09-01T10:40:23Z  github-actions[bot]

That is the OLDEST of the twelve. The newest is 5518670706 at 2026-09-03T00:56:58Z. The parameters are silently ignored, exactly as the finding says.

The fix does not flip the assumption from descending to ascending, because an ordering assumption is what broke. Selection is now by max(created_at) over non-bot comments, with position as a tiebreak only when a timestamp is missing, and the page is fetched with per_page=100 --paginate so the newest comment is actually in the payload. Re-verified against the same live thread after the change: the selector returns 5518670706. A regression test reverses the input list and asserts the answer does not change, which is the property that makes this order independent rather than differently ordered.

GPT 5.6, claim_preflight.py:473 -- "please close" from any commenter produced reporter-asked-close. FIXED.

A closure request now requires standing: the issue's own author (always true of the body) or a repository insider by author_association (OWNER / MEMBER / COLLABORATOR). CONTRIBUTOR alone is deliberately excluded -- it means "has had a PR merged here once", which is not authority over another person's report. A maintainer's "fixed in 0.7, please close" is kept, because it is at least as authoritative as the reporter's. Anyone without standing falls through to CLAIM, which costs at most the one dispatch that discovers the work is already done -- the cheap direction, where a false CLOSE would have closed live work.

Standing gates CLOSE only, not SKIP: a stranger announcing "I am claiming this" still produces SKIP, because staying away is also cheap.

GPT 5.6, claim_preflight.py:277 -- --paginate emits separate JSON documents so gh_json returns UNKNOWN. NOT APPLICABLE, measured false positive.

gh api --paginate merges JSON ARRAY pages into a single document. Measured on gh 2.96.0 with the exact call shape at that line:

  • repos/kirodotdev/KiroCrew/issues/7597/timeline?per_page=8 --paginate -> 4 pages returned as ONE parseable array of 33 events (6 cross-references), json.loads succeeds;
  • repos/kirodotdev/KiroCrew/issues/7597/comments?per_page=5 --paginate -> 3 pages of sizes [5, 5, 2] returned as ONE array of 12.

So the described failure does not occur, and the proposed remedy would introduce one: --slurp changes the shape to an outer array of pages, and gh rejects it outright in combination with --jq (the --slurp option is not supported with --jq or --template). The constant carrying the page size now records this measurement next to the flag, so the next reader does not have to re-derive it.

Nothing was overridden. The remaining red lanes are a separate matter and are being handled with the fleet's coordinator, not by pushing past them.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 dispositions for d49e00cd8. Design Review and Opus 4.8 are both green on this head; First Principles' three advisory items are answered below. Its Blocker is a cross-PR sequencing question that I am not treating as mine to resolve by pushing past it -- see the last section.

Watch: symbol_on_base parks a feature request that names a symbol it proposes to add. ACCEPTED as a real limitation. The obvious remedy is measured and unaffordable.

The finding is right, and it is sharper than "no incident behind it": a feature request naming a new identifier is absent from the default branch by definition, so it SKIPs on every pass, not once.

The principled fix follows from what the check MEANS -- absence is evidence only when the code lives on an unmerged branch, so the discriminator is "does this symbol exist on any branch?". I measured it before proposing it. This clone has 377 remote refs; git grep -l -F -e <symbol> across only 40 of them took 12.2s wall and 1m38s CPU, against 0.17s for the same symbol on origin/main alone. At up to 8 symbols per item and 377 refs, that is minutes per candidate for a script whose whole purpose is to be the cheap question asked across a batch. The remedy costs more than the dispatch it saves.

The alternative narrowings are guesses, and one of them breaks the check: "require at least one named symbol to be present" would classify exactly the case check 4 exists for -- a fix whose entire target span lives on an unmerged branch, where nothing is present -- as workable. Prose detection of "this is a feature request" is the same class of guess the citation-stripping rule already had to be measured into, and I am not adding an unmeasured one.

So the limitation is disclosed rather than fixed, and two properties bound it: the verdict is SKIP, not CLOSE, so nothing is destroyed and the item flips to CLAIM by itself the moment the symbol lands; and the reason is printed with the symbol name (SKIP 8031 symbol-absent=PARTIAL_TURN_MARKER), so it is visible in the log rather than a silent park a human cannot see or overrule.

Subtractions: drop closed_by, and drop recency / risk=. NOT MINE TO DECIDE -- both are mandated by the interface contract this script was built against, and one of them is a documented output shape.

I agree with the reasoning on closed_by on its own terms: nothing reads it, and the measurement that justifies distrusting the field is an argument for deleting it rather than carrying it. But it is check 6 of six in the contract, named there explicitly as a bonus signal that is never the test, and recency/risk are check 5 plus a field in both documented output forms -- the CLAIM <item> risk=low line and the JSON risk key. A parallel change documents that surface right now, so removing either here would break a shape another change describes rather than simplify the tree. I have escalated both to the owner of that contract instead of acting unilaterally; if the ruling is to subtract, it is a small deletion plus its tests.

Blocker: nothing consumes the script.

Factually correct, and I am not disputing the grep. The file that would consume it is owned by a parallel change that already contains exactly the wiring this finding asks for -- the script added to the inventory, the dispatch predicate replaced with the one call, and the 0/10/11/2/3 table documented including the exit-3 branch. Editing that file from here would collide with it mid-flight. So this is a sequencing decision for the owner of both changes, and it is with them now. Nothing has been overridden, and I have not touched a file outside this change's scope to make a reviewer happy.

@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from d49e00c to b607bb1 Compare September 3, 2026 02:14
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3, head b607bb1f0. The three contract corrections are implemented, and one of them is demonstrated on a live item rather than argued.

1. closedByPullRequestsReferences is dropped. Five checks, not six.

Carrying it as "a bonus signal, never the test" was a polite way of writing "changes no verdict", and a per-candidate forge call that cannot alter the outcome is pure cost against a shared rate limit. The check, its field, its JSON key and its test are gone; --json now reports five checks. Two tests pin the removal rather than trusting it: the check-name tuple is asserted exactly, and a full run's argv list is asserted to contain no such call. The field survives in one place only -- the module rationale, as the question this script refuses to ask, with the measurement that earned it ([] on two items that were closed by merged PRs).

2. recency / risk= is kept, and now says where it is consumed.

The annotation stays exactly as emitted. What was missing was the consumer, and it is named in the module docstring and the PR body: risk=high means the item is not batched -- it gets a live re-check immediately before the atomic claim. That is a response the skill performs, which is why the code does not change and the description does.

3. An absent symbol no longer vetoes on its own, and item 8031 shows why that mattered.

Corroboration is explicit metadata -- a bug / defect / regression / crash label, or the issue type -- and never prose. Guessing the class from wording would put an unmeasured heuristic in front of the very veto that was over-applied; a label is a human's deliberate triage act, which is what makes it corroboration. Without it the verdict downgrades to CLAIM risk=high and the reason rides in --json as symbol_absent_uncorroborated, since the human CLAIM line is one field wide by contract.

Measured on a live item, before and after:

before:  SKIP  8031 symbol-absent=PARTIAL_TURN_MARKER
after:   CLAIM 8031 risk=high

Item 8031's only labels are readiness: checking and fork, and it has no issue type -- so nothing corroborates bug-class and the old rule parked it. It would have stayed parked on every subsequent pass, because the symbol stays absent. That is the permanent false veto, on a real item, not a hypothetical.

An uncorroborated item now costs at most one dispatch that discovers the code is not on the base. A park costs the item.

Verification. 154 tests, 99% line coverage on the file, mypy / flake8 / black clean. Live re-run after the change: 7597 CLOSE merged-pr=#7613 landed=true, 8029 SKIP open-pr=#8034, 8031 CLAIM risk=high, 8007 CLAIM risk=low. Each of the seven accepted bug-label spellings and five rejected non-bug ones is a test case, and junk metadata (a string where a list belongs, a list of nulls, a label with no name) degrades to "not corroborated" instead of raising.

The remaining red lane is the zero-consumer BLOCK, which is a merge-order matter and is being handled as one.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from b607bb1 to 6260cfb Compare September 3, 2026 02:34
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 4, head 6260cfbb2. The reviewer caught two real leftovers from dropping a check, and it was right about both.

Watch: the description contradicted the diff on the check count. FIXED, and ratcheted.

Removing one check left the old count behind in two places I own: collect()'s docstring still said it ran six, and a test was still named for six. Both are corrected. The PR body had already been rewritten, but the code had not caught up, and a reviewer reading the tree rather than the prose is exactly the reader that should have caught it.

Counting words in prose are cheap to forget, so they are now pinned instead of remembered: a new test scans the script and the test file and fails if any line names a check count other than the real one, derived from CHECK_NAMES rather than hard-coded. Mutation-verified rather than assumed -- reintroducing the old wording in the docstring reddens it, and the tree is restored afterwards.

Watch: checks 4 and 5 trace to no named incident, unlike 1 through 3.

Correct as stated, and both were narrowed for exactly that reason in the previous round rather than defended. symbol_on_base no longer vetoes on its own -- it requires bug-class corroboration and otherwise downgrades to CLAIM risk=high, which is what stops it parking a feature request forever. recency never vetoed. What the reviewer is pointing at now is thinner and fair: their declared consumer is a response in the conductor's skill, and that response is not in the base yet.

Subtraction: defer --json until something consumes it.

Same shape as the blocker, and it resolves the same way. --json is part of the CLI this script was specified against, and the parallel change that wires the script documents the flag in the invocation it publishes. Deferring it here would mean shipping a documented flag that does not exist. Not actioned unilaterally; it is with the owner of that contract.

Blocker: nothing consumes the script.

Still factually right, still not disputed. The resolution is a merge order, not a content change: the change that wires the consumer lands first, then this one rebases onto it, at which point the reviewer's own grep finds the consumers with neither side editing the other's files. That is the reviewer's second clause taken in the direction the file partition allows.

155 tests, 99% line coverage, mypy / flake8 / black clean.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from 6260cfb to 635d874 Compare September 3, 2026 03:55
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from ef47ec7 to da69767 Compare September 3, 2026 10:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 9 on da6976701. Both round-8 findings are gone from GPT's list. One new finding, and it was a regression I introduced last round while fixing the previous one -- fixed, pinned, and mutation-verified. The fork item is unchanged and still the conductor's.

FIXED -- a bare pronoun is not an issue. Last round I added an object requirement so "please close the file" would stop closing items, and I put bare it and bare that in the list of acceptable objects. That was wrong for the same reason the original bug was wrong: a bare pronoun resolves to whatever was mentioned last, and in a bug report that is usually a socket, a file or a handle. Measured on the head I had just pushed:

CLOSES  | The connection leaks. Please close it.
CLOSES  | Please close it, the handle stays open otherwise.
CLOSES  | The stream stays open. Please close that.
CLOSES  | The socket is still open -- can you close it?

Four more false CLOSEs on live work, shipped by the fix for the previous four. it and that are no longer objects on their own, and the permissive it continuation in the clause-end alternation went with them -- that was the second door into the same room. this is kept because it points at the thread's topic rather than at the previous noun, and that issue still works with the noun present. All four lines above now decline; "Please close this.", "please close that issue", "please close the item", "please close #8029" and the eleven other real phrasings still fire.

Mutation-verified with the collection guard: restoring it/that as bare objects fails 4 of 4 pronoun tests, and removing the guard fails 5 of the object tests.

A limitation pinned rather than fixed. While testing I asserted that "close the ticket please" is detected. It is not -- the pattern is please close, and a post-positioned please was never supported. My test expectation was wrong, not the code, and the honest response was to correct the expectation and pin the miss with its reason rather than widen the verb forms. Widening is what produced three separate false-CLOSE defects in this file already, and a missed request costs one dispatch. If it shows up in real items, that evidence is the argument for adding it.

Verification. 241 tests, 99% line coverage, mypy / flake8 / isort / black clean. Live: 8088 CLOSE merged-pr=#8092, 7597 SKIP open-pr=#8035, 8031 CLAIM risk=high, 8007 CLAIM risk=low, 6799 SKIP untrusted-fork=true risk=high.

The fork item is still the ruled one. GPT again asks to continue past untrusted_fork entries, which is what ruling D11 rejected on measured grounds -- 192 of this repository's 301 open PRs come from forks, so falling through to CLAIM reinstates the duplicate-dispatch class this script exists to remove. No code change satisfies both that ruling and this finding, the override route was disqualified by doctrine, and I have taken no unilateral action.

Four rounds, seven blocking findings, six of them real defects in my code and three of those false-CLOSE paths on live work. The prose scanner is where every one of them lived, which is worth saying plainly: hand-written English patterns deciding a destructive verdict have now needed four corrections, and the reviewer found each one faster than my own tests did.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from da69767 to f735384 Compare September 3, 2026 11:05
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 10 on f73538407. The bare-pronoun finding is gone. Three new items, all correct, all fixed. The fork item is unchanged and still the conductor's.

FIXED -- an adjectival "fixed" closed live issues. \b matches happily before a hyphen, so this\s+(?:is|was)\s+...(?:resolved|fixed)\b fired on "This is fixed-width layout and hard to read". The word is an ADJECTIVE there, not a verdict, and the result was CLOSE on a live item.

The deeper problem was that this pattern had no continuation guard at all -- only a (?!\s+by\b) exclusion I had added for the workaround case. Two exclusions were never going to cover English. It now uses a positive continuation allowlist, the same shape as the close-family objects: punctuation, end of text, or a word that keeps it a statement about the item (in, on, since, now, already, --). A hyphen satisfies none of them. by is absent by omission rather than by a second special case, so there is one mechanism instead of two -- having two is how the bare-it door stayed open last round after I closed the object list.

Measured: three adjectival forms now decline, and "this is resolved", "this is resolved, closing", "this was fixed in the 0.7 release", "this is resolved -- closing now" and "this is already fixed on main" all still fire.

FIXED -- a reference with no repository was assumed local. if full_name is not None and full_name != repo let a missing nested repository object through, and the code then fetched repos/<this repo>/pulls/<N>: a DIFFERENT pull request that merely shares the number, whose state could SKIP a live item. The repository is now derived from the API URL when the object is absent, and a reference whose repository cannot be established from either source is skipped rather than assumed -- an unidentifiable reference is not coverage. Verified both ways: a reference identified only by URL as belonging to another repo is skipped, and one identified only by URL as local is still kept, because a guard that goes deaf is a different bug.

FIXED (the advisory) -- ancestry was asked about PRs that could not matter. annotate_landed ran over every merged hit, so an unanswerable ancestry question about a merged PR that merely MENTIONS the item returned ancestry-unknown and made the whole check UNKNOWN. A mention cannot reach rule 1 whatever its ancestry, so the error was reported over a fact that could not change the verdict. Ancestry is now asked only of closure-claiming entries. An error is reserved for a question whose answer matters. Pinned in both directions: a mention-only list returns None even with no clone and an unknown branch, and a claiming entry alongside a mention is still checked, so the scoping cannot become a way to skip the check that counts.

Verification. 261 tests, 99% line coverage, mypy / flake8 / isort / black clean. Three existing ancestry tests failed on the scoping change and were corrected rather than worked around -- their fixtures were asking about ancestry with closes_item unset, which is now a contradiction in terms. Live: 8088 CLOSE merged-pr=#8092, 7597 SKIP open-pr=#8035, 8031 CLAIM risk=high, 8007 CLAIM risk=low, 6799 SKIP untrusted-fork=true risk=high.

The fork item is still the ruled one. Same remedy, same conflict with ruling D11 and with the merged check list, no unilateral action, no override.

Five rounds, ten blocking findings, nine of them real. Six lived in the prose scanner. That concentration is the useful signal here: hand-written English deciding a destructive verdict has needed a correction in every single round, and each correction has been narrower than the last. A design where prose can only DOWNGRADE, never CLOSE, would have made all six impossible, and I would rather name that than keep patching patterns.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from f735384 to 87f4d3a Compare September 3, 2026 11:22
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 11 on 87f4d3a23. All three round-10 findings are gone. Four new ones, all correct, all fixed. Two were false CLOSEs on live work. The fork item is unchanged and still the conductor's.

FIXED -- negated prose closed live issues. The phrase patterns match a SUBSTRING, and English puts the negation earlier in the sentence, so every one of these returned CLOSE, exit 11:

CLOSES  | I don't think this is resolved
CLOSES  | I am not sure this is fixed
CLOSES  | nobody said this is resolved

A negation-and-hedging window over the text PRECEDING each match now vetoes the phrase, and the scan continues to the next pattern rather than giving up. Python's re has no variable-width lookbehind, so this is a window check in code rather than part of the pattern.

Worth naming explicitly because it looks like an inconsistency and is not: this is the OPPOSITE choice from closing_reference_re, which is deliberately negation-BLIND. There, blindness matches GitHub's own parser, which closes an item for "does not close #N" regardless, so agreeing with the forge is what keeps the two readings identical. Here nothing else is doing the reading and the verdict writes to somebody's live work.

FIXED -- a stale body request overrode a newer reopening comment. The body is the OLDEST text on an item. A reporter who wrote "this is resolved" and later commented "still broken on 0.8" had reopened it, and the scan read the body and closed it. Closure now consults the body only when there is no newer human comment at all: a later comment is the item's current state whether or not it happens to contain a phrase this scanner knows, so its mere existence retires the body's request. The CLAIM loop still reads the body, because ownership is not a status -- it holds until withdrawn, and the three items that motivated the prose check declared ownership in the body.

That the whole suite passed before I added tests for this is the part I would flag: 261 tests and none of them combined a body request with a later comment. The gap in the tests and the gap in the code were the same gap.

FIXED -- an attributed claim parked the item. claim(?:ing|ed)\s+(?:this|it) read "The docs claimed this works" as ownership and returned SKIP. Past-tense claimed now needs a first-person subject; the gerund "claiming this" stays, since it is elliptical first person in practice.

FIXED (the advisory) -- recovery stepped over a withdrawal. When I added newest_authorized_claim last round I disclosed and accepted a cost: a silently abandoned claim could park the item forever. That cost did not have to be paid. A newer comment from anyone with standing that gives the item up ("dropping this", "no longer working on this", "unassigning myself") now retires every older claim, and a claim NEWER than the withdrawal counts again, because somebody picking an item back up owns it again.

A limitation pinned, and a second wrong expectation of my own. I wrote a test asserting that "I don't think this is resolved, but please close this issue as a duplicate" still closes, reasoning that the scan continues past a negated match. It does continue -- but the window is measured in characters and "don't" sits 35 of them before "please close", so the hedge vetoes the later request too. The code is right and my expectation was wrong. That is now pinned as a disclosed miss with its reason: it costs one dispatch, where the other reading closes live work, and making the window clause-aware needs sentence segmentation, which is a bigger claim about English than this file should make. This is the second time this session I have asserted what I wanted rather than what I measured, both times caught by running it.

Verification. 285 tests, 99% line coverage, mypy / flake8 / isort / black clean.

Live, and two verdicts legitimately CHANGED between rounds, which is worth reading rather than glossing: item 6799 was SKIP untrusted-fork=true and is now CLAIM risk=high because PR #8146 MERGED in the interim. I checked that before assuming a regression. The new answer exercises two other rules on real data: the merged PR only MENTIONS 6799, so the closing-keyword condition correctly declines to CLOSE, and its ancestry is never asked because a mention cannot reach rule 1 -- the scoping from the last round, working live. Item 8071 changed the same way when #8107 merged. Current: 8088 CLOSE merged-pr=#8092, 6509 SKIP open-pr=#8140 fork=true untrusted-fork=true risk=high, 7597 SKIP open-pr=#8035, 8031 CLAIM risk=high, 8007 CLAIM risk=low.

Six rounds, thirteen blocking findings, twelve real, and nine of them in the prose scanner. Every round has produced at least one, each narrower than the last. That is not a run of bad luck; it is what happens when hand-written English decides a destructive verdict. All nine become impossible if prose can only downgrade to risk=high and never CLOSE, and the conductor has the proposal.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/conductor-claim-preflight-8029 branch from 87f4d3a to 45ec72f Compare September 3, 2026 11:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 12 on 45ec72f5f. All four round-11 findings are gone. Four new ones, all correct, all fixed. The fork item was NOT on this round's blocking list, which is why no override is posted with this push -- see the last section.

FIXED -- a stranger could release somebody else's claim. The withdrawal check I added last round tested only that the commenter was not a bot, so any passer-by typing "dropping this" retired a maintainer's earlier claim and the item was dispatched on top of live work. That is the denial-of-work shape again, running the other direction: instead of suppressing an item that should be worked, it releases one that is already owned.

A withdrawal now only ever releases its OWN author's claim -- you can only give up what you hold -- and withdrawals are tracked per author, so one person leaving does not release another's claim. Standing deliberately does NOT transfer here: standing is authority over the ITEM, not over another person's commitment, so an OWNER cannot release a MEMBER's claim either. Measured: outsider withdrawal leaves the claim recovered, the claimant's own withdrawal retires it.

FIXED -- the last two closure patterns had no guard. I had added an object requirement to the close family and a state guard to resolved/fixed, and missed these two. "The temp file can be closed after the read" and "the workaround is no longer needed" both returned CLOSE on live items. this can be closed now takes the same state guard, and no longer ... is split: "an issue" and "reproducible" are about the item by construction, while "needed" and "relevant" are not and now require the item as their subject.

FIXED -- double-backtick citations survived the stripper. _INLINE_CODE_RE matched a single backtick pair, so on this can be closed written with doubled delimiters it consumed each `` as an EMPTY span and left the quoted phrase behind as prose. Citing the phrases this scanner looks for therefore closed a live item -- the same class as the original quotation bug, wearing code delimiters. The pattern now uses a backreference so the closing run must match the opening one, which is the rule Markdown itself uses. Verified for single, double and triple runs.

FIXED (the advisory) -- a partial scan discarded what it had already confirmed. A detail-call failure on a later PR returned [], [], error, throwing away an OPEN PR already confirmed and turning a definite SKIP into UNKNOWN. The module's own precedence says a definite finding outranks a partial view; that rule applies to a half-finished scan too. The confirmed hits now travel with the error, and the caller keeps the SKIP while the merged side reads unknown. The error is not swallowed, only kept off the answer it cannot change. Rule 1 outranks rule 2, so a CLOSE can still be missed this way -- that costs an item left open, never a false close.

Verification. 302 tests, 99% line coverage, mypy / flake8 / isort / black clean.

On the fork item and the override. An override was authorized for it, and I am not posting one on this head, because the fork finding is absent from GPT's list for 87f4d3a23 -- the three blocking items were the ones above. Posting an override there would have cleared three unrelated real defects along with it, which is the difference between a judgment call and laundering.

Also, the live evidence the authorization rests on had gone stale in BOTH cases, so I re-ran it before citing anything. Items 6799 and 8071 have moved to CLAIM: their fork PRs (#8146, #8107) MERGED. The current pair is stronger, because the unmarked case now exercises the insider clause rather than the reporter clause:

6509   SKIP open-pr=#8140 fork=true author=LuisBrel untrusted-fork=true risk=high   (CONTRIBUTOR, not the reporter)
7643   SKIP open-pr=#7709 fork=true author=kyleseaman                               (MEMBER: fork, but vouched)

Both are fork PRs and only one is marked, which is the discrimination the mitigation claims to perform. The fork population re-measured today is 190 of 299 open PRs, unchanged in substance from 192 of 301.

If the fork finding returns on a head whose other blocking items are clear, the override goes up then, on its own.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 13 on 54f6fce26. All four round-12 findings are gone, and on 45ec72f5f GPT's blocking list was down to ONE entry -- the fork item -- for the first time. Its remaining advisory was real and is fixed, which is why the override is going on the NEXT head rather than that one.

FIXED (the advisory, and it was more serious than its label) -- a body claim survived its own author's withdrawal. A claim written in the issue BODY deliberately outlives later chatter, because ownership is not a status. The gap was that it also outlived the claimant giving it up: a reporter who wrote "Ownership claimed by me" and later commented "dropping this" left the item SKIPping forever. That is indefinite suppression, which is the harm this file treats as the serious one and the exact class the fork ruling turns on -- not a wasted dispatch. Advisory by label, permanent by effect.

The body predates every comment by construction, so any withdrawal from its author retires it and no timestamp comparison is needed. Verified three ways: the reporter's own withdrawal retires the body claim, a stranger's does not, and an unknown reporter retires nothing.

While fixing it I factored the withdrawal scan into withdrawals_by_author, shared by the claim recovery and this check. That removes a duplicate I had introduced one round earlier, and it matters beyond tidiness: two copies of one rule is exactly how the bare-pronoun door stayed open after I closed the object list, and how the by exclusion ended up redundant with the state guard. One rule, one implementation.

Verification. 306 tests, 99% line coverage, mypy / flake8 / isort / black clean. Live: 8088 CLOSE merged-pr=#8092, 6509 SKIP open-pr=#8140 fork=true untrusted-fork=true risk=high, 7643 SKIP open-pr=#7709 fork=true unmarked, 7597 SKIP open-pr=#8035, 8031 CLAIM risk=high, 8007 CLAIM risk=low.

Why the override is not on this comment. An override clears the reviewer lane for one specific SHA. On 45ec72f5f the fork item was finally the only blocker, so the precondition was met -- but posting it there and then pushing this fix would have left an override pointing at a superseded head, and the fix would have gone out with the lane re-blocked anyway. Fixing first and overriding the resulting head keeps the override attached to the commit that actually ships. It goes up on the next head where the fork item stands alone, with the four points on the record.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 14 on f28c44d72. The body-withdrawal advisory is gone. One blocking finding, correct, fixed -- and this time I fixed the CLASS rather than the instance, because this was the sixth round in a row where the same shape recurred.

FIXED -- the last pattern with a hardcoded bare pronoun. happy\s+to\s+have\s+(?:it|this)\s+closed was the one closure pattern I never touched while narrowing the others, so it still accepted bare it: a reporter describing a socket they are "happy to have it closed" returned CLOSE on a live item. It now takes the shared _ISSUE_OBJECT, so the deictic and explicit-noun forms fire and the bare pronoun does not.

That loses a real request -- in an issue thread "happy to have it closed" usually does mean the item -- and the miss is pinned with that reasoning rather than hidden. It costs one dispatch; the other reading closes somebody's work.

The actual point of this round: the recurrence was the defect. Six consecutive rounds found one more closure phrase that matched prose about something other than the item -- a file, a socket, a workaround, a layout, a pronoun. Each was fixed individually, and the next round found another, because nothing checked the property itself. Two tests now do:

  • A property test over 9 non-issue objects x 8 templates, 72 sentences, asserting none of them CLOSE. That covers the shape rather than the instance.
  • A structural ratchet over CLOSURE_RES itself: every pattern must either embed one of the shared guards or be declared in ITEM_SCOPED_CLOSURE_RES with a reason. A new phrase added without a guard fails here instead of in review. A third test keeps that declared set from drifting out of the list.

Mutation-verified, and the result argues for the ratchet specifically: restoring the unguarded pattern fails the STRUCTURAL test and NOT the property test, because my template list happens not to phrase "happy to have X closed" with a resource. The generative test would have missed exactly the bug that prompted it. That is the case for checking the list as well as the behaviour, and I would not have known it without running the mutation.

Verification. 310 tests, 99% line coverage, mypy / flake8 / isort / black clean. Three existing tests used "happy to have it closed" as a genuine request and were updated to the deictic form rather than worked around. Live: 8088 CLOSE merged-pr=#8092, 6509 SKIP open-pr=#8140 fork=true untrusted-fork=true risk=high, 7643 SKIP open-pr=#7709 fork=true unmarked, 8007 CLAIM risk=low.

Process note, because it nearly shipped nothing again. The scratch directory rotated mid-round, so git commit --amend -F failed on a stale path exactly as it did earlier in this workstream. The verify-the-blob step caught it: ITEM_SCOPED_CLOSURE_RES read 0 in the committed blob, which is only possible if the amend never happened. I recovered the message from the existing commit rather than retyping it, re-amended, and confirmed the symbol present before pushing. The check exists because this failure already cost a silent push once.

On the override. The fork item was absent from this round's list, so the precondition is still unmet and nothing was overridden. It stays available for the first head where the fork item stands alone.

The conductor's claim predicate was one prose line, `gh pr list --search`, and
it was blind in three directions at once. Each blind spot cost a whole worker
dispatch to discover the work did not exist: an item already fixed by a MERGED
PR (an `--state open` query structurally cannot see one), four items that each
had an OPEN PR carrying `Fixes #N` behind a single field that answered empty,
and three items that declared ownership in PROSE.

claim_preflight.py asks all five questions in one call and returns one verdict
on the exit code: 0 CLAIM, 10 SKIP, 11 CLOSE, 3 UNKNOWN, 2 malformed. The
verdict is a pure function of a checks dict, so every precedence branch is a
unit test with no forge access, and an unanswerable question yields UNKNOWN,
never CLAIM.

Six rules earn their own mention because measurement produced each one, not
reasoning:

- The prose scan reads what an author SAYS, not what they QUOTE: the item
  specifying this script quotes the closure phrases it detects, and a raw scan
  returned CLOSE on live work.
- The newest human comment is chosen by timestamp, never by position: the
  comments endpoint ignores `sort`/`direction` and answers oldest-first, so
  asking for `direction=desc` read the OLDEST of twelve comments on a real item.
- A merged PR is coverage only if it CLAIMS to close the item. A bare mention is
  not closure. Measured on a real item: 7597 has TWO landed merged PRs that
  merely reference it, one titled "docs: investigation for #7597", and treating
  either as coverage would have closed an item still being fixed.
- A closure request needs standing (the reporter or a repository insider), since
  CLOSE acts on live work.
- A self-claim needs standing too, but downgrades instead of vetoing: a SKIP any
  commenter can cast is a denial-of-work channel, so an unauthorized claim
  annotates `risk=high` and takes the live recheck.
- An open fork PR still SKIPs, but not silently. Opening a fork PR needs no
  permission, so rule 2 is a suppression channel anybody can use. Refusing to
  trust fork PRs is the wrong trade -- 192 of this repo's 301 open PRs come from
  forks, so that reinstates the duplicate-dispatch class this script was built
  from -- and the objection was never to the detection but to a response that
  was unconditional AND silent. So the verdict is unchanged and an unvouched
  fork's SKIP carries `untrusted-fork=true risk=high`. Standing is an insider
  association or the item's own reporter, since fixing your own bug from a fork
  is the ordinary case. The consumer is the conductor's review of untrusted-fork
  suppressions.
- An absent symbol vetoes only when the item's metadata corroborates bug-class:
  a feature request names the symbol it PROPOSES to add, so an unconditional
  veto parked that whole class permanently.

`closedByPullRequestsReferences` is not consulted at all. It measured `[]` on
two items that were closed by merged PRs, and a per-candidate forge call that
cannot change the verdict is pure cost against a shared rate limit.

Verified against live items: 8088 answers CLOSE merged-pr=#8092 landed=true
(that PR carries `Closes #8088`), 7597 and 8029 answer SKIP open-pr=#8035,
8031 answers CLAIM risk=high (no bug-class label), 8007 answers CLAIM risk=low.
The fork marker is live too: 6799 and 6509 answer SKIP untrusted-fork=true
risk=high behind first-time-contributor fork PRs, while 8071 stays unmarked
because its fork PR author reported the item.

Refs #8029
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 15 on a53776b14. The unguarded-pattern class is gone from GPT's list -- the ratchet added last round is holding. Three new items, all correct, all fixed, and two of the three were consequences of my own earlier changes.

FIXED -- HTML comments were read as prose. A comment is invisible to every human reader of the item, so nothing inside one is a statement its author is making. This repository's issue templates ship instructional comments, so <!-- please close this issue when done --> arrives in the body of real items and closed them. Comments are now stripped FIRST, before the fence, blockquote, backtick and quotation passes, because a comment can contain any of those and must not be parsed as one -- verified with a comment containing a fence and one containing a quotation.

This is the most likely false CLOSE of the whole series: it needs no unusual phrasing from a reporter, only a template that already exists.

FIXED -- the fork marker vanished on the degraded path, and that was my own doing. Round 12 taught the scan to keep a confirmed open PR through a later detail failure. The annotation, though, was still gated on there being NO error, so that kept SKIP came out as a routine low-risk one with no untrusted-fork marker. The loud suppression was therefore missing in exactly the conditions that make a suppression hard to notice, which defeats the mitigation the fork ruling rests on. The annotation is now keyed on the HITS rather than on the absence of an error.

FIXED -- an invariant that overclaimed. The docstring said "Nothing user-authored reaches stdout" while the output legitimately carries logins, PR numbers, comment ids and extracted symbol names. The claim is now narrowed to what is true and says why: no user-authored PROSE reaches stdout -- failures are slugs, a prose match reports its pattern, a bug-class match reports this module's own term -- while identifiers do appear, because they are the evidence a conductor needs to check the verdict. Worth fixing rather than shrugging at: the wider version was in this docstring when a leak of user-written LABEL text shipped straight past it, so an invariant that overclaims is not a harmless overstatement here.

A mutation run that lied, and how I caught it. My first mutation check reported the fork-marker test NOT CAUGHT. The test was fine; my mutant was wrong -- I had gated the FIRST annotate_untrusted_forks call, which sits in the issue-fetch-FAILED branch, while the test traverses the success branch. Re-targeting the mutant to the branch the test actually reaches reports CAUGHT. Both readings were available and only one was true, and the difference is whether the mutation names the code path the test exercises. That is the second time a mutation harness has misreported this session; the first was a collection error, this one a mis-aimed mutant.

Verification. 318 tests, 99% line coverage, mypy / flake8 / isort / black clean. Live: 8088 CLOSE merged-pr=#8092, 6509 SKIP open-pr=#8140 fork=true untrusted-fork=true risk=high, 7643 SKIP open-pr=#7709 fork=true unmarked, 8007 CLAIM risk=low.

On the override. The fork item was again absent from this round's list, so the precondition is unmet and nothing was overridden.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt a53776b: the merged contract mandates this verdict, the harm it names is already mitigated by an annotation rather than a verdict change, and the requested remedy would regress 63% of this repository's open PRs; filed as a follow-up, not dismissed.

Recorded in full, because an override should be auditable rather than terse.

1. The contract mandates the verdict. Merged SKILL.md line 204, emphasis the contract's own:

  1. open_prs -- any open PR referencing it, fork PRs included -> SKIP open-pr.

The requested change is for this script to do the opposite of the specification that merged for it two cycles ago. That file is not mine, and a code change contradicting a bold line of a shipped contract is a contract amendment, not a bug fix.

2. The stated harm is mitigated, by annotation rather than by a verdict change. The finding's harm is SILENT indefinite suppression. It is not silent. An unvouched fork's SKIP carries untrusted-fork=true risk=high, and the conductor reviews those as a triage signal rather than treating the item as closed. Verified live at the moment of writing, both PRs still open:

6509   SKIP 6509 open-pr=#8140 fork=true author=LuisBrel untrusted-fork=true risk=high
7643   SKIP 7643 open-pr=#7709 fork=true author=kyleseaman

Two fork PRs, one marked. #8140 is CONTRIBUTOR and not the reporter, so it is unvouched; #7709 is MEMBER, so it is vouched. That is standing being computed, not fork-ness relabelled -- which matters, because a marker that fired on every fork would carry no information.

3. The remedy regresses the majority case. Re-measured today: 189 of 299 open PRs on this repository come from forks, 63%. Letting untrusted forks fall through to CLAIM risk=high reinstates the duplicate-dispatch class this script exists to remove, on nearly two thirds of the input distribution, to close a channel that costs an outsider one throwaway PR. In a public repository most genuine coverage IS fork coverage.

4. Filed as a follow-up, not dismissed. The fork-versus-trusted-standing model deserves designing properly rather than being decided inside one script's precedence list, and the position is revisitable when it is. This override is a judgment call on that design disagreement, not a claim that the reviewer is wrong about the mechanism -- it is right about the mechanism.

Why this is not laundering, stated plainly so the record can be checked. Across this review, GPT raised 21 blocking findings on this PR and I fixed 20 of them on the merits -- including three false-CLOSE paths that returned exit 11 on live work, one regression I introduced myself while fixing another, and two consequences of my own earlier fixes. Two rounds ago this override was already authorized and I did NOT post it, twice, because the fork item was not the only blocking entry and overriding would have cleared unrelated real defects along with it. It is posted now, on the first head where it stands alone, against a single item where the reviewer asks for a documented contract violation.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for a53776b14e096343ba08794ed575853cf6ca2bbf.

the merged contract mandates this verdict, the harm it names is already mitigated by an annotation rather than a verdict change, and the requested remedy would regress 63% of this repository's open PRs; filed as a follow-up, not dismissed.

This decision applies only to this commit. A new push requires a new judgment.

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.

2 participants