feat(sweep): classify question-card gates and name the unverifiable residual (BLO-30627) - #1541
Conversation
…esidual (BLO-30627) BLO-30608 shipped the gate re-validation pass and its AC3 said a high `unverifiable` count "is itself the finding". It was 90.9% of 746 rows. This is the follow-up. Three changes, all still read-only. 1. Third gate kind: `pending-interaction`, reading `issue_thread_interactions`. BLO-30608 named permission/RBAC as the obvious next probe; it is still the wrong one to build first, for the reason its own seam comment gave -- a live access probe needs network egress and per-target credentials, and this pass collects inside a database transaction. Question cards are where this codebase actually expresses "a human has to answer something", they are the largest unprobed category, and they are DB-local, so all three of the module's stated properties survive. `permission-rbac` stays a declared seam. Two resolution kinds, deliberately distinct: `interaction-answered` (a human decided) and `interaction-abandoned` (every card withdrawn, expired, or failed -- so the human was asked and never replied, and no reply is coming). The second is ranked with the stuck cancelled edge because neither can self-clear. An unrecognised status counts as live, which is property 2 applied to schema drift. 2. `unverifiable` is subdivided by `UnverifiableReason`, derived from the row's own status so it costs no extra query. Two are contradictions a reader can act on (`blocked` with no blocker edge; `in_review` with no approval card); the rest are honest absences. 3. `DEFAULT_MAX_PROBES` 600 -> 2000, with the derivation in the comment. The measured 99s/775-call figure belongs to the `--source=api` backfill, whose per-issue call is O(rows); the shipping DB path batches at 500, so it is O(ceil(rows/500)) -- 9 queries at the measured population, 15 at this cap. The cap is a runaway guard, not a cost control, and is set with headroom rather than tight against a population that grows. Measured on the live population, `--source=api --all`, 2026-08-29T05:1xZ (748 rows; the baseline's was 746): still-gated 16 -> 24 resolved-but-open 52 -> 77 unverifiable 678 -> 647 90.9% -> 86.5% of the 647: 456 queued and waiting on attention rather than a gate, 174 `in_review` with no approval card, 17 in progress, 0 `blocked` without a blocker edge. New actionable finding the pass could not previously see: 18 rows whose every question card was withdrawn or expired. Cost moved 775 -> 1525 round trips and 99.0s -> 149.8s, all of it the API path's second per-issue call; the DB path gained 2 batched queries. One side effect is stated rather than left implicit: on `--source=api` only, `GET /api/issues/:id/interactions` first runs `expireRequestConfirmationsSupersededByHistoricalComments`, so reading it can expire a confirmation card a later comment had already superseded. It touches no issue field and is the endpoint's normal behaviour for any reader including the UI. The shipping DB path reads the table directly and has no such effect. Tests: 144 green across the five human-gated suites (76 in the three re-validation suites). Each new classifier branch was mutation-checked -- dropping the probe from PROBES, collapsing abandoned into answered, and pinning the reason to `status-unreadable` fail 3, 5, and 7 cases respectively. The wiring suite's teardown now clears `issue_thread_interactions` before `issues`; without it the FK fails the delete and every subsequent test inherits the undeleted company. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head c080f8a — BLO-30627. Three focus areas:
|
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c080f8a
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/blo-30608-gate-revalidation-backfill.ts:180— the DB backfill reports a hard-codedcalls: 6, butloadGateEvidencenow runs three chunked queries per 500-row batch (blockers, approvals, and interactions) atserver/src/services/human-gated-ageing-digest.ts:380,:410, and:434. For the 748-row population cited by this PR, that is 9 evidence queries, plus the candidate query and two clock aggregates, not 6 total. The report therefore understates the measured round-trip cost and makes the documented AC5 output incorrect for any population above 500 (and also overstates it for smaller populations where fewer chunks are used).- Derive the count from the actual candidate count and
AGGREGATE_CHUNK_SIZEfor each query family, or instrument the DB client/query path so the reported value reflects executed round trips. Add coverage for populations spanning 0, 1, 500, and 501 rows.
- Derive the count from the actual candidate count and
Suggestions (0)
Strengths
- The interaction probe fails closed for unknown text statuses and distinguishes human decisions from abandoned cards.
- Interaction evidence is loaded in bounded database batches and the API path documents its superseding-comment side effect.
- The new classifier branches have focused pure and wiring tests.
Recommended Action
- Fix Important issues before merge.
- Address Suggestions opportunistically.
…ing it `acquireFromDb` reported a hard-coded `calls: 6`. That is the AC5 cost figure the backfill exists to measure, and a constant cannot be right across the range: the DB path runs five *batched* query families — the candidate query's two human-clock aggregates plus `loadGateEvidence`'s blockers, approvals, and interactions — each once per `AGGREGATE_CHUNK_SIZE` chunk, so the true cost is `1 + 5 * ceil(n / chunkSize)`. The constant was therefore correct only for a single-chunk population. It understated any population spanning two or more chunks (the live 746-row pass costs 11 round trips, not 6) and overstated an empty one, where `chunk([])` yields no iterations and the candidate query is the only trip. Derives the count from the real population and the real chunk size, and exports `AGGREGATE_CHUNK_SIZE` so the arithmetic tracks the constant rather than restating the literal `500`. The arithmetic is factored into a pure `dbRoundTrips` because `acquireFromDb` needs a live `DATABASE_URL` and cannot be exercised in unit tests. Covered across the chunk boundary at 0, 1, 500, 501, the measured 746, and a non-default chunk size; mutation-checked by restoring `return 6`, which fails 4 of the 7 new cases. Found in review of #1541. Read-only is unchanged: this touches only how the pass reports its own cost.
|
Addressed in Confirmed. One correction to the arithmetic, which changes the number but not the verdict. The review counts "3 chunked evidence queries + the candidate query + two clock aggregates", treating the clock aggregates as fixed cost. They aren't — For the 746-row population that is 11, not the 9 the review derived (3 families × ceil(746/500) = 6 evidence queries, not 9) and not the 6 that shipped. Both directions the review names are real, and the empty case is the sharper one: What changed:
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8853e4f
Prior Findings Dispositioned (1)
- prior:c080f8a important 1 — fixed —
scripts/blo-30608-gate-revalidation-backfill.ts:167—dbRoundTripsnow derives the DB cost as1 + 5 * ceil(population / chunkSize), andacquireFromDbpasses the candidate count and exportedAGGREGATE_CHUNK_SIZEat:210, covering populations across the 500-row chunk boundary.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/human-gated-gate-revalidation.ts:713— theinteraction-answereddigest heading says “Every question card has been answered,” butprobePendingInteractionreturns this resolution wheneverdecided.length > 0, even when the remaining cards arecancelled,expired, orfailed(:460-471). Mixed rows are therefore rendered under a heading that contradicts the evidence line (“closed, N by a human decision”) and can lead operators to believe no unanswered card was abandoned.- Use wording that covers the actual predicate, such as “At least one question card was answered; remaining cards closed without an answer,” or split the resolution kinds so the heading is truthful. Add a renderer test for one answered plus one expired card.
Suggestions (0)
Strengths
- The interaction probe fails closed for unknown text statuses, preserving a live-gate interpretation during schema drift.
- Interaction evidence is loaded in bounded DB batches and the API path documents its superseding-comment side effect.
- The backfill cost calculation is now derived and tested at empty, single-chunk, and multi-chunk boundaries.
Recommended Action
- Fix the Important issue before merge.
- Preserve the current exact-head cost derivation and interaction status handling.
`interaction-answered` is assigned whenever *at least one* question card got
a real human decision, so a row with one answered and one expired card lands
under that kind. The digest heading read "Every question card has been
answered", which contradicted the evidence line printed directly beneath it
("closed, 1 by a human decision") and hid the abandoned ask from an operator
scanning headings.
Same class of overstatement the evidence line was already fixed for, one
layer up. Reworded the heading, the `GateResolutionKind` doc, and the
backfill's summary label to match the actual predicate. Added a renderer test
for the mixed case; mutation-checked by restoring the old heading, which
fails it.
Behaviour unchanged: classification, counts and read-only posture are
untouched — this is wording only.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed the Important finding in The finding is correct, and it is the same defect one layer up from one already fixed on this PR. The heading and the line beneath it contradict each other, and an operator scanning headings sees no abandoned ask. Fix — wording only, no behaviour change. I took the reword branch rather than splitting the resolution kind: the kind's job is to separate a human engaged from nobody ever did ( Three call sites overstated the same predicate; all three now match it:
Test. Added the renderer case you asked for — one 84 tests green across the three |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a8e4248
Prior Findings Dispositioned (1)
- prior:8853e4f important 1 — fixed —
server/src/services/human-gated-gate-revalidation.ts:723— the mixed interaction case now uses the truthful heading “At least one question card was answered — any remaining cards closed without an answer,” matching thedecided.length > 0predicate and avoiding the earlier claim that every card was answered.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The interaction probe fails closed for unknown text statuses, preserving a live-gate result during status drift.
- Interaction evidence is loaded in bounded, company-scoped batches and the API-path superseding-comment side effect is explicitly documented.
- The DB round-trip estimate is derived from the actual chunk size and covers empty, single-chunk, and multi-chunk populations with focused tests.
- The unverifiable residual is classified from the row status and rendered as actionable named categories without adding a query.
Recommended Action
- No Critical or Important issues found; this App-authored PR can receive the required formal review comment.
- Preserve the exact-head prior-finding disposition when re-reviewing future revisions.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3e36f41
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The interaction probe fails closed for pending and unknown statuses, while distinguishing human decisions from abandoned cards.
- Interaction evidence is loaded with company scoping and bounded batches, and API-side supersession behavior is explicitly documented.
- The unverifiable residual is classified from row status without adding a query, and the DB round-trip estimate follows the actual chunk size.
Recommended Action
- No Critical or Important issues found; this App-authored PR has the required formal review comment.
- Preserve the exact-head prior-finding disposition when re-reviewing future revisions.
|
@ally please re-review at head No source change since your clean review of Review focus — carrying the prior disposition forward. Please confirm the finding remains fixed at this head so the gate can attest |
Thinking Path
Linked Issues or Issue Description
resolved-but-openrows get dispositionedRelated PRs found by dedup search (
gh pr list --state all, filtered on gate/revalidation/interaction):a13753f0) — shipped the pass this extends. Direct parent; this PR is its follow-up.services/recovery/issue-graph-liveness.ts; this touchesservices/human-gated-*) and neither mutates interactions. But both now read pending interactions as a liveness signal with different staleness rules — fix(liveness): expire stale pending interactions from waiting paths #1533 applies a 24-hour boundary, this probe treats any pending card asstill-gatedregardless of age. See Risks.No other open or merged PR covers gate-kind coverage in the re-validation classifier.
Checked
ROADMAP.md: the only adjacent line is "explicit review and approval stages as first-class workflow steps" (L45). That is about creating approval workflows; this is read-only reporting over gates that already exist. No overlap with planned core work.What Changed
probePendingInteractionreadingissue_thread_interactions— the cards this codebase creates when an agent asks its human a question, confirmation, or checkbox verdict. Pending (or unrecognised-status) →still-gated; at least one human decision →interaction-answered; every card withdrawn/expired/failed →interaction-abandoned.interaction-abandonedis ranked withblocker-cancelled-edge-stuckin a newNON_SELF_CLEARING_RESOLUTION_KINDSlist. Neither can self-clear, so both lead the report ahead of merely-finished kinds.UnverifiableReasonsplitting the residual five ways, derived from the row's ownstatusso it costs no extra query. Surfaced on the classification, counted incountsByUnverifiableReason, and rendered in the digest.DEFAULT_MAX_PROBES600 → 2000, with the derivation in the comment rather than as a bare number.GateEvidenceInputgainsinteractions(required) andstatus(optional).interactionsis required so a loader that forgets to populate it fails to compile — a silently-absent array would make a row with a live question card read as expressing no gate.loadGateEvidenceissues a third batched query at the sameAGGREGATE_CHUNK_SIZE(500) and threadsstatusthrough.Verification
Tests —
npx vitest run server/src/__tests__/human-gated→ 144 passed, 5 files, 0 failures. 76 of those are the three re-validation suites (64 pure + backfill, 12 wiring/embedded-Postgres).npx tsc --noEmit -p server/tsconfig.jsonclean.Mutation-check — each new classifier branch was reverted and the suite re-run:
probePendingInteractionfromPROBESinteraction-abandonedintointeraction-answeredclassifyUnverifiableReasontostatus-unreadableLive measurement —
npx tsx scripts/blo-30608-gate-revalidation-backfill.ts --source=api --all, 2026-08-29T05:1xZ. Population had grown 746 → 748 since the baseline, so shares are near- but not exactly-comparable:still-gatedresolved-but-openunverifiableThe 647 residual, now named:
in_reviewbut no approval card existsblockedbut no blocker edge existsThat 456 is the honest answer to why most of the queue is unverifiable: it is un-started human work, not work gated on anything a probe could re-test. New actionable finding the pass could not previously see: 18 rows whose every question card was withdrawn or expired — a human was asked and never replied.
Cost moved 775 → 1525 round trips and 99.0s → 149.8s, all of it the API path's second per-issue call. The DB path gained 2 batched queries.
Two defects the live run caught that the tests now pin: an evidence line that described a mixed row as uniformly answered (observed on BLO-2880, where one card had actually
expired), and a wiring-suite teardown that did not clearissue_thread_interactionsbeforeissues— the FK failed the delete and every subsequent test inherited the undeleted company, so one missing line read as eight unrelated failures.Risks
--source=apionly,GET /api/issues/:id/interactionsfirst runsexpireRequestConfirmationsSupersededByHistoricalComments, so reading it can expire a confirmation card a later comment had already superseded. It touches no issue field and is the endpoint's normal behaviour for any reader including the UI — but "read-only" is an acceptance criterion, so an unstated write would quietly break it. I ran this across 748 live rows to produce the measurement above. The shipping DB path reads the table directly and has no such effect. If a reviewer judges this unacceptable, the fix is to drop interaction evidence from the API path and measure via--source=db; the classifier is unaffected either way. Documented in the script docblock.still-gatedregardless of age. That is defensible — the ageing sweep already ranks by human silence separately, and this probe answers "is a gate expressed?" not "is it fresh?" — but the two should not silently drift into contradicting each other. Flagged for a reviewer rather than resolved unilaterally.interactionsbecame a required field onGateEvidenceInput. Deliberate, and it is a compile error rather than a runtime surprise; both call sites are updated in this PR. A missing array could never produce a falseresolved-but-open(that branch requires a non-empty array), so the failure direction was already safe — required makes it non-reachable.loadGateEvidencebatches at 500, so the sweep is O(ceil(rows/500)): 9 queries at the measured population, 15 at the new cap.Model Used
Claude Opus 4.5 (
claude-opus-5[1m]as configured on this agent), 1M context, extended thinking, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code