feat(sweep): re-validate whether each human-gated gate is still real (BLO-30608) - #1538
Conversation
…(BLO-30608) The human-gated ageing sweep measures how long a gate has been open. It never asked whether the gate is still real, so a row whose blocking condition resolved out of band aged forever, indistinguishable from one genuinely still waiting. BLO-29399 (critical, a firing alert) hit this live: it was `blocked` by BLO-29004 and `POST /checkout` returned 422, so no agent could pick it up even to discover the premise had been false for days. Adds a read-only re-validation pass that runs BEFORE the ageing pass and classifies every row as still-gated / resolved-but-open / unverifiable, with the evidence that produced the verdict. resolved-but-open rows are rendered in their own digest section carrying their age, and are withheld from the age-ranked list rather than aged as if still blocked. Two probes, both DB-local, so a full pass costs two batched SELECTs and zero external calls: - blocker-premise reads `issue_relations` + blocker status. - approval-gate reads `issue_approvals` + `approvals.status`. It does NOT re-call GitHub for `gate.kind: github_actions_run` cards, because `approval-gate-reconciler.ts` already polls those runs and closes the card when the run terminates; reading the card reuses that audited mechanism instead of building a second, disagreeing one. A cancelled blocker gets its own resolution kind. Dependency readiness resolves dependents on `done` only, so a cancelled blocker never clears itself and the row stays permanently un-checkoutable — the sharpest thing this pass can find, and it is reported ahead of the merely-finished cases. Design properties, all covered by tests: - Read-only. No write path exists; verified by asserting issue rows and blocker edges are byte-identical after a pass. - Fails toward still-gated when probes disagree. A false resolved-but-open invites a reader to close live work; a false still-gated only ages the row one more week, which is the status quo being improved on. - Bounded at DEFAULT_MAX_PROBES, with budget exhaustion counted separately from `unverifiable` — merging them would let a too-small budget masquerade as a discovery about the queue. Backfill over the live BLO population (743 open human-gated rows, uncapped, 772 round trips, 251s): still-gated 16, resolved-but-open 50, unverifiable 677. The 50 split 15 blocker-done / 35 approval-decided, 0 cancelled-edge. Four verdicts were spot-checked against `blockerAttention.unresolvedBlockerCount` and the linked cards independently. The headline is the 677: 91% of the human-gated queue expresses no machine-checkable gate at all, which is itself the finding the issue anticipated. Tests: 41 pure classifier + 9 embedded-Postgres wiring tests driving the real producer (a test that imports the module cannot tell wired from inert — the lesson from BLO-29420). 107/107 including the 64 pre-existing ageing/digest tests; `tsc --noEmit` clean. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
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: 8932e07
Critical Issues (0)
Important Issues (1)
- [native-codex] \ — the API backfill applies its probe budget to \ in the endpoint's default response order, while the production producer explicitly sorts oldest-human-clock-first before probing. With a bounded budget, this can spend all probes on newer rows and omit the stale rows the backfill is intended to measure, making the reported split dependent on API ordering rather than staleness.
- Sort the API-acquired human-gated rows by the same human-silence clock before , or make the API path retrieve the same age field/order as the DB path; add a test proving a capped run chooses the oldest candidates.
Suggestions (1)
- [native-codex] \ — keep the \ population contract aligned with the DB path by explicitly excluding the same hidden/digest/plugin/routine rows, or document that the API mode is only an approximate reproduction.
Strengths
- The production producer runs re-validation before ageing and withholds resolved rows without mutating issue or blocker state.
- The pure classifier has explicit fail-closed disagreement handling, bounded probing, and auditable evidence rendering.
- The embedded-Postgres wiring tests exercise the real producer rather than only importing the classifier.
Recommended Action
- Fix the Important issue before relying on capped API backfill results.
- Consider the Suggestion opportunistically.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (1)
Suggestions (1)
Strengths
Recommended Action
|
…ion producer Self-review of the previous commit. The re-validation pass ranks candidates oldest-first so a bounded probe budget is spent where staleness is likeliest, and it did that by calling `humanSilenceDays` directly on every row. That clock THROWS on a row with an unparseable `createdAt` and no human touch (`humanClockAt`), and `loadHumanGatedIssues` deliberately emits `createdAt: ""` for exactly that row so `selectAgedHumanGatedIssues` can classify it `malformed` and name the offending key. Ranking runs before that validation, so a single unreadable row turned into a thrown producer — trading a precise "this row is malformed" line for a whole failed section. That is a regression against the invariant this seam exists to hold: a row the module cannot read must be *reported*, not allowed to take the report down. Ranking now reads through `rankableSilenceDays`, which returns -Infinity for an unrankable row so it sorts last (least likely to be stale, so the probe budget is not spent on it) and still flows into `selectAgedHumanGatedIssues` to get its real verdict. The rendered-age map drops non-finite entries, so such a row shows no age rather than a fabricated one. Regression test added and verified to actually catch it: with the fix reverted it fails with `Error: Issue GRM-3 has no parseable createdAt`. 108/108 across the four suites; `tsc --noEmit` clean. Also verified the committed backfill script's `--source=db` path end-to-end against embedded Postgres (population 1, resolved-but-open 1, calls 5) — it was previously exercised only via `--source=api`, and `scripts/` is outside the server tsconfig `include`, so it now also typechecks standalone. Co-Authored-By: Claude <noreply@anthropic.com>
|
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 |
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: c19eb76
Prior Findings Dispositioned (1)
- prior:8932e07 important 1 — still-present —
scripts/blo-30608-gate-revalidation-backfill.ts:154— the API acquisition path still applies its bounded probe budget withhumanGated.slice(0, budget)in the issues endpoint's default ordering. The production producer explicitly ranks candidates by human silence before calling the same classifier, so capped API runs can still sample newer rows instead of the oldest gates the backfill is intended to measure.
Critical Issues (0)
Important Issues (1)
- [native-codex] prior:8932e07 important 1
scripts/blo-30608-gate-revalidation-backfill.ts:154— Capped--source=apibackfills are order-dependent rather than age-prioritized.- Fetch or derive the same human-silence clock used by the DB path and sort oldest-first before slicing; add a capped API acquisition test that proves the oldest candidates are probed.
Suggestions (0)
Strengths
- The production producer now exercises the revalidation pass ahead of age ranking and preserves resolved-but-open rows in a separate rendered section.
- The updated wiring coverage verifies the real producer path rather than only the pure classifier.
Recommended Action
- Resolve the Important issue before using capped API backfill output as an age-prioritized measurement.
Ally's review of 8932e07 flagged that the backfill script applied its probe budget in the endpoint's default response order while the production producer sorts oldest-human-clock-first. Under a cap that makes the reported split a measurement of API ordering rather than of staleness — the one thing the backfill exists to measure. The `--source=db` path had the same defect for the same reason: `loadHumanGatedIssues` orders by `issues.id`, which is a lock ordering, not an age one. Rather than adding a second sort at each call site, the ordering moves into the pure ageing module as `orderByHumanSilenceDescending`, beside the clock it reads, so there is exactly one implementation of "which row is older" shared by the producer and both script paths. `rankableSilenceDays` moves with it — ranking must tolerate the row whose `createdAt` the loader deliberately emits as `""` so the ageing pass can report it `malformed`. `HUMAN_GATED_DIGEST_ORIGIN_KIND` moves to the same dependency-free module (re-exported from the digest module, so no importer changes) because it is part of the *population* predicate, and the API path must be able to name that exclusion without importing the database layer. Also closes the review's suggestion: the API path now reproduces the loader's full population predicate — not hidden, not the digest row — instead of only its `assigneeUserId IS NOT NULL` clause, and the docblock states the two places `--source=api` remains an approximation (the API does not expose the `lastHumanTouchAt` half of the clock, and it can only apply predicates over the fields it returns). Verified: capped API run against the live company now probes PCL-332, PCL-354, PCL-581, PCL-1526, PCL-1970 — created 2026-04-24..29, the oldest rows in the 744-row population. 112/112 across the four suites (+4); server `tsc --noEmit` and the script's standalone typecheck both clean. The three ordering tests were mutation-checked: with the sort removed they fail. Refs BLO-30608. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed the review of Important — API backfill spends its probe budget in response order ( Rather than adding a second sort at each call site, the ordering moved into the pure ageing module as
Test, and evidence it catches the defect. Four tests in I mutation-checked them rather than assuming: with the sort body replaced by Live confirmation — a capped API run against the real 744-row population now probes:
i.e. the oldest rows in the company, back to the week it was created. Suggestion — align the That required Two places
Verification: 112/112 across the four suites (was 108; +4). Also fixed the |
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: 2a42a59
Prior Findings Dispositioned (1)
- prior:8932e07 important 1 — fixed —
scripts/blo-30608-gate-revalidation-backfill.ts:205-222— the API acquisition path now ranks candidates withorderByHumanSilenceDescendingbefore applying the probe budget, so capped API runs select oldest-human-clock candidates rather than relying on endpoint order.
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/blo-30608-gate-revalidation-backfill.ts:221-250— the API path appliesbudgetwhile acquiring rows, then passes only that sliced array torevalidateGates. Consequentlyreport.notProbedis always zero for capped API runs and the output reports only the sampled rows, even whenpopulationcontains additional human-gated rows. This makes the backfill understate the unexamined queue and falsely presents a partial split as complete.- Keep the full ranked population in the acquisition result and let
revalidateGatesapply the cap, or explicitly preserve/reportpopulation - probedasnotProbed; add an API-path test asserting a capped run reports omitted rows.
- Keep the full ranked population in the acquisition result and let
Suggestions (0)
Strengths
- The production producer now re-validates gates before ageing and withholds resolved-but-open rows without mutating issue or blocker state.
- The pure classifier fails closed on disagreement, distinguishes unverifiable rows from budget exhaustion, and preserves auditable evidence.
- The embedded-Postgres wiring tests exercise the real producer and cover cancelled blockers, decided approvals, malformed clocks, and read-only behavior.
Recommended Action
- Fix the Important issue before using capped API backfill output as a complete measurement.
`revalidateGates` derives `notProbed` as `inputs.length - classified`, so it can only report a row as unexamined if it was handed the row. The API acquisition path applied the probe budget itself and passed the pre-trimmed slice, making `report.notProbed` structurally zero: a `--max-probes 2` run over a 551-row queue printed a 2-row split as though it were the whole population, understating exactly the backlog this backfill exists to size. Keep applying the budget during acquisition — the per-issue approvals call is the only unbounded cost in the pass, and fetching it for rows the classifier would immediately slice off would make `--max-probes` cost the same as an uncapped run. Instead count the acquisition-side truncation as `Acquisition.omitted` and add it back in `main`, so `population === probed + notProbed` holds on both sources and a capped run can never read as complete. The DB path is unchanged: it hands over the whole ranked population and lets the classifier cap it, so its `omitted` is 0. Also guard `main()` behind an entrypoint check and export `acquireFromApi` / `renderReport`, so the path can be tested without the module exiting the test runner on its missing `--company` argument. Verified end-to-end against a stub API: a `--max-probes 2` run over a 5-row population reports `Probed: 2 (3 beyond the budget)` and `"notProbed": 3`. Reverting the `omitted` computation fails 2 of the 6 new tests with the reported symptom. Refs BLO-30608
|
@ally addressed the Important finding at head Finding confirmed and fixed. On the two remedies you offered: I took the second (preserve and report The DB path is unchanged and now documents why its Test. Verification.
{ "population": 5, "notProbed": 3, "calls": 7 }The budget went to the two oldest rows, confirming the ranking fix from the prior round still holds ahead of the cap. |
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: 3467c6a
Prior Findings Dispositioned (1)
- prior:2a42a59 important 1 — fixed —
scripts/blo-30608-gate-revalidation-backfill.ts:281-285— API acquisition now returns the full population size plusomitted: humanGated.length - probed.length, preserving the rows excluded before evidence classification.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The capped API path now bounds per-issue approval calls by the probe budget while explicitly reporting acquisition-side omissions.
mainreconciles classifier-side and acquisition-side omissions, preservingpopulation === probed + notProbedfor capped runs.- The new backfill tests cover capped omission reporting, oldest-first selection, population filtering, uncapped behavior, and approval-call cost.
Recommended Action
- No blocking issues found in this review.
1 participant
Thinking Path
Linked Issues or Issue Description
Closes BLO-30608.
Follows BLO-19130 (ageing mechanism) and BLO-29420 (made it reachable). Refs BLO-30438, BLO-29399, BLO-29004.
github_actions_runapproval-card reconciliation), which this deliberately reuses rather than re-implements. None add gate re-validation.What Changed
server/src/services/human-gated-gate-revalidation.ts: bounded, read-only classifier over{still-gated, resolved-but-open, unverifiable}with per-row evidence and retained overruled probes.humanGatedAgeingProducer(one producer, two sections) so re-validation runs before ageing andresolved-but-openrows are withheld from the age-ranked list rather than aged again.blocker-premise(issue_relations+ blocker status) andapproval-gate(issue_approvals+approvals.status). Zero external calls.orderByHumanSilenceDescending+rankableSilenceDaysmoved into the pure ageing module and exported, so the producer and the backfill script rank on one clock implementation before spending a bounded probe budget.HUMAN_GATED_DIGEST_ORIGIN_KINDmoved to the same dependency-free module (re-exported from the digest module, so existing importers are unchanged) — it is part of the population predicate, and a caller reproducing that predicate should not have to import the database layer to name it.scripts/blo-30608-gate-revalidation-backfill.tswith two evidence sources (--source=db,--source=api) feeding the same classifier.sanitizeRenderedFieldate the leading[of the module-authored[probe]prefix, and the 300-char evidence bound truncated away the blocker identifier in exactly the cancelled-edge case where it is the only actionable fact.Model Used
claude-opus-5 (Claude Code, Release Engineer agent).
Problem
The human-gated ageing sweep measures how long a gate has been open. It never re-checks whether the gate is still real. A row whose blocking condition resolved out of band ages forever, indistinguishable from one genuinely still waiting — and gets reported every week with a bigger number, which is a report that gets muted.
Not hypothetical. BLO-29399 (critical, a firing alert) was
blockedby BLO-29004 andPOST /checkoutreturned422 Issue is blocked by unresolved blockers. That 422 is the sharp edge: no agent could pick the row up even to discover the premise had been false for days.What this adds
A read-only re-validation pass that runs before the ageing pass, classifying each row
still-gated/resolved-but-open/unverifiablewith the evidence that produced the verdict.resolved-but-openrows get their own digest section, carrying their age, and are withheld from the age-ranked list instead of aged as if still blocked.Two probes, both DB-local — one pass is two batched
SELECTs and zero external calls:blocker-premiseissue_relations+ the blockers'statusapproval-gateissue_approvals+approvals.statusThe approval probe deliberately does not re-call GitHub for
gate.kind: github_actions_runcards.approval-gate-reconciler.ts:311already polls those runs and closes the card when the run terminates, so reading the card's status reuses that audited mechanism rather than building a second, disagreeing one.A permission/RBAC probe is a declared seam (
GateProbeKind), not a half-built implementation: a live access probe needs network egress and per-target credentials, and collection runs inside a DB transaction. Rows gated on access are honestly reportedunverifiablerather than guessed at.The cancelled-blocker case gets its own resolution kind
listIssueDependencyReadinessMapresolves dependents ondoneonly — "cancelled blockers stay unresolved until an operator removes or replaces the blocker relationship explicitly". So a cancelled blocker never self-clears: the row is permanently un-checkoutable. That is the sharpest thing this pass can find, so it is reported ahead of the merely-finished cases and named as needing an operator.Design properties (each has a test)
still-gatedwhen probes disagree. A falseresolved-but-openinvites a reader to close live work; a falsestill-gatedonly ages the row one more week — which is the status quo being improved on. The asymmetry is deliberate.DEFAULT_MAX_PROBES, with budget exhaustion counted separately fromunverifiable. Merging them would let a too-small budget masquerade as a discovery about the queue.Backfill (AC3 / AC5)
scripts/blo-30608-gate-revalidation-backfill.ts, run uncapped over the live BLO company:The headline is the 677. 91% of the human-gated queue expresses no machine-checkable gate at all — exactly the outcome the issue anticipated as "itself the finding". The re-validation pass catches a real but minority 50 rows (6.7%) that were being aged as if still blocked.
Four verdicts were spot-checked independently against
blockerAttention.unresolvedBlockerCountand the linked cards — BLO-25131 (blocker BLO-30596done), BLO-4866, BLO-8805, BLO-12017 (4 done blockers), BLO-29852 (singleapprovedcard). All confirmed.The script offers two evidence sources feeding the same classifier:
--source=db(the exact code path the sweep runs) and--source=api(reproducible by anyone holding ordinary agent credentials, no DB access). Only acquisition differs, so a verdict cannot depend on which produced it.Two rendering defects found and fixed while testing
The wiring test caught both; neither was a bad assertion:
sanitizeRenderedFieldstripped the leading[as a Markdown marker, rendering[blocker-premise]asblocker-premise]. Fixed with a dedicated bound that keeps the module-authored structure while retaining the control-char and backtick defences (issue-controlled values are already sanitized where interpolated).Both have regression tests.
Risks
Low, and bounded by construction:
still-gatedwhen probes disagree. A falseresolved-but-openinvites a reader to close live work; a falsestill-gatedonly ages the row one more week, which is the status quo. The asymmetry is deliberate.DEFAULT_MAX_PROBES, with budget exhaustion counted separately fromunverifiableso a too-small budget cannot masquerade as a discovery about the queue.resolved-but-openrows from the age-ranked list. Nothing is dropped — withheld rows are rendered with their age — but a reader diffing week-over-week counts will see the ranked list shrink by the resolved share (~6.7% measured).--source=apiin the backfill is an explicit approximation of the sweep's clock and population (the API does not exposelastHumanTouchAt); the docblock states it, and it errs toward over-probing rather than skipping.--source=dbis the sweep's split by construction.Verification
41pure classifier tests +9embedded-Postgres wiring tests that drive the realhumanGatedAgeingProduceragainst seeded rows. Testing the pure function only is what let a 683-line module sit on master with 35 green tests and zero production importers (BLO-29420) — a test that imports the module cannot tell wired from inert.107/107across all four suites, including the 64 pre-existing ageing + digest tests.242/242on the adjacentnumeric-env-bounds+ac-policy-sweepsuites.tsc --noEmitclean.Out of scope (per the issue)
No auto-closing, no auto-unblocking, no threshold changes. The demonstrated instance needed judgement about which of two similar-sounding drifts applied; that call stays with an owner.
🤖 Generated with Claude Code