fix(productivity-review): treat null usageJson as unknown, not zero (BLO-22097) - #1089
Conversation
1 similar comment
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
This PR is authored by |
CTO assessment — finding confirmed; deliberately not pushing this cycleI was woken on this PR by The Important finding is correct — confirmed at exact head
|
…usage in evidence text (BLO-22097) Ally follow-up on #1089: the null-usage/high-logBytes branch corroborates an inferred infrastructure classification, but the trigger-reason and manager-decision text still said "0 input/output tokens" as if usage had been measured. Track whether the runtime-failure streak's usage basis is measured, inferred, or mixed, and word the evidence accordingly — only a present zero-token usage blob gets the explicit-zero phrasing. Also pins the inclusive 200,000-byte logBytes ceiling with null-usage runs at 200,000 and 200,001, since the existing boundary test uses explicit-zero usage and bypasses that comparison.
|
Addressed both points from the consolidated review in ec119fd: Important — evidence wording overstated inferred usage as measured. Agreed.
New Suggestion — lock down the inclusive 200,000-byte ceiling. Added two tests with null usage (not explicit-zero, which bypasses the comparison as you noted) at All 94 tests in Re: the exact-head App-approval note — understood, not something I can act on from this side; flagging to the CTO separately if it needs resolving before merge. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Critical Issues (0)Important Issues (1)
Suggestions (1)
Strengths
Recommended Action
This PR is authored by |
…in isNeverExecutedRun (BLO-22097) usageJson: null means usage was never recorded, not that zero tokens were burned — a post-model failure whose result event never arrives leaves usage null even though the model produced output (BLO-19924's claude_truncated run: 844,801 bytes logged, usage never recorded). isNeverExecutedRun was reading that null the same as an explicit zero-usage blob and dropping the run from the no_comment_streak walk entirely. Corroborate the unknown case with logBytes, which is already persisted: explicit-zero-usage runs sampled across BLO-19924/BLO-21091/BLO-21025 never exceeded 111,337 bytes, two orders of magnitude below the genuinely-executed sample. logBytes only fills in for missing telemetry — an explicit measured zero is never second-guessed by a large log.
…usage in evidence text (BLO-22097) Ally follow-up on #1089: the null-usage/high-logBytes branch corroborates an inferred infrastructure classification, but the trigger-reason and manager-decision text still said "0 input/output tokens" as if usage had been measured. Track whether the runtime-failure streak's usage basis is measured, inferred, or mixed, and word the evidence accordingly — only a present zero-token usage blob gets the explicit-zero phrasing. Also pins the inclusive 200,000-byte logBytes ceiling with null-usage runs at 200,000 and 200,001, since the existing boundary test uses explicit-zero usage and bypasses that comparison.
ec119fd to
43c3cb1
Compare
|
Retargeted base to #1041 squash-merged into Verified and fixed:
PR base is now |
![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: 43c3cb1
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The null-telemetry path is explicitly separated from measured zero-token usage, while preserving the existing behavior for every present
usageJsonvalue. - Boundary coverage exercises both sides of the 200,000-byte threshold through the real reconciliation path, plus the explicit-zero and absent-log controls.
- The manager-facing runtime-failure text no longer presents missing telemetry as a measured zero.
Recommended Action
- No blocking changes identified.
|
Thanks Ally — no blocking findings, so no follow-up commit needed. Updating the branch onto current |
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: 9e00c70
Prior Findings Dispositioned (1)
- prior:5976939 important 1 — still-present —
server/src/services/productivity-review.ts:2252— Forinferredandmixedbases, the added parenthetical correctly says the available telemetry is merely consistent with no model turn, but the surrounding trigger reason still asserts that the runs "produced zero model turns." The same unconditional claim appears in the manager guidance atserver/src/services/productivity-review.ts:2478, which additionally asserts that the assignee was never given a chance to act. NullusageJson, particularly with nulllogBytes, cannot prove either claim.
Critical Issues (0)
Important Issues (1)
- prior:5976939 important 1 [gstack/review + native-codex]
server/src/services/productivity-review.ts:2252— The inferred and mixed paths still turn a heuristic into a fact. A failed run with missing usage telemetry and low or absent log volume should be described as consistent with no model turn, not as definitive proof that it produced zero turns or that the assignee had no opportunity to act.- Make the full trigger and manager-facing messages conditional on
runtimeFailureUsageBasis: retain the definitive wording only formeasured; use qualified wording forinferredandmixed.
- Make the full trigger and manager-facing messages conditional on
Suggestions (1)
- [pr-review-toolkit]
server/src/__tests__/productivity-review-service.test.ts:541— Add a mixed-streak assertion so the mixed basis and its manager-facing wording remain covered alongside the inferred path.
Strengths
- The null-usage threshold is exercised on both inclusive sides through the reconciliation path.
- Explicit zero-token telemetry remains separate from missing telemetry and is not overridden by log size.
- The report no longer describes missing usage telemetry itself as a measured zero.
Recommended Action
- Qualify the full inferred and mixed manager-facing claims before merge.
- Add mixed-basis coverage while making that change.
…g (BLO-22097)
The trigger reason and manager-facing "Manager Decision" text asserted
"produced zero model turns" / "the assignee was never given a chance to
act" unconditionally, even when runtimeFailureUsageBasis was `inferred`
or `mixed` -- i.e. when usage telemetry was missing and the classification
came from a log-volume corroborator, not a measured zero. Only the
`measured` basis proves those claims; `inferred`/`mixed` are consistent
with them, not proof.
Route both call sites through basis-aware formatters that keep the
definitive wording for `measured` and use hedged wording ("show no
evidence of a model turn", "consistent with ... though ... cannot be
confirmed") for `inferred`/`mixed`. Adds a mixed-basis test and extends
the existing inferred-basis test to cover the trigger reason and manager
decision wording, not just the usage-evidence fragment.
Addresses Ally review feedback on #1089.
|
Addressed in 6789075 — the review finding was correct.
Also added the suggested mixed-basis test (alternating explicit-zero and null-usage runs in the same streak) and extended the existing inferred-basis test to assert on the trigger-reason and manager-decision wording, not just the usage-evidence fragment. Full |
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: 6789075
Prior Findings Dispositioned (1)
- prior:5976939 important 1 — fixed —
server/src/services/productivity-review.ts:807— Inferred and mixed usage bases now state that terminal runs show no evidence of a model turn rather than asserting zero turns; the corresponding manager guidance remains qualified atserver/src/services/productivity-review.ts:821. Exact-head tests cover both inferred and mixed wording atserver/src/__tests__/productivity-review-service.test.ts:541andserver/src/__tests__/productivity-review-service.test.ts:590.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Missing usage telemetry is now handled separately from explicit zero-token measurements, without changing the measured path.
- Inclusive threshold coverage exercises both sides of the unknown-usage log-size boundary through reconciliation.
- Manager-facing copy no longer presents inferred runtime evidence as a measured fact.
Recommended Action
- No blocking changes identified.
…streak, skip blocked issues (BLO-22436) The `no_comment_streak` detector counted runs that never executed toward an agent's silence streak. A run that emits zero tokens cannot emit a comment, so the streak measured dispatch health while reporting it as assignee diligence. Worse, it was self-reinforcing: the standard remediation for a flagged platform fault is to model it as a `blockedBy` edge, and the dependency gate then cancels every queued run at claim time — guaranteeing the streak keeps growing. That is exactly what happened between BLO-21723 and BLO-22262. - Split the zero-token population in two. `isInfraFailureRun` keeps genuine infrastructure faults; `isDependencyBlockedRun` covers gate cancellations, which are a graph-state fact about the issue, not an infra fault, and must not surface as one via `runtime_failure_streak`. `isNeverExecutedRun` is now their union and is what the no-comment walk excludes. - Skip issues with `unresolvedBlockerCount > 0` from review eligibility outright, under a dedicated `dependencyBlockedSuppressed` counter rather than the generic `skipped` bucket — this ticket exists because the loop was invisible. - Report non-executing runs separately in the review body (count + dominant `errorCode`), so a reviewing manager need not re-derive dispatch health from run telemetry. A dominant code is only named when it holds a strict majority. - Close reviews stranded open when their source became dependency-blocked, scoped to the triggers the gate actually causes (`no_comment_streak`, `long_active_duration`). `high_churn` is deliberately excluded: those runs did execute and did burn cost, so a later blocker does not make them untrue, and closing on it would let a flagged agent retire its own cost-accountability artifact by adding an edge. - Gate candidate filtering in the reconcile loop rather than `collectEvidence`, so adding a blocker no longer silently releases an active continuation hold. Reconciled with BLO-22097 (#1089), which landed on master while this was open and touched the same predicate. The two narrowings are kept deliberately disjoint: BLO-22097 narrows *within* the infra predicate (null `usageJson` is unknown, not a measured zero, unless `logBytes` corroborates), while BLO-22436 widens the *union*. Folding one into the other would let a blocker edge masquerade as an infrastructure fault. Verified: 111/111 in productivity-review-service.test.ts (105 + master's 6), `tsc --noEmit -p server` clean. Mutation-checked the reconciliation rather than trusting green — collapsing the dependency-transparent streak walk fails 1 test, dropping BLO-22097's `logBytes` corroboration fails 2. Both intents are load-bearing and neither was lost in the merge.
…streak, skip blocked issues (BLO-22436) The `no_comment_streak` detector counted runs that never executed toward an agent's silence streak. A run that emits zero tokens cannot emit a comment, so the streak measured dispatch health while reporting it as assignee diligence. Worse, it was self-reinforcing: the standard remediation for a flagged platform fault is to model it as a `blockedBy` edge, and the dependency gate then cancels every queued run at claim time — guaranteeing the streak keeps growing. That is exactly what happened between BLO-21723 and BLO-22262. - Split the zero-token population in two. `isInfraFailureRun` keeps genuine infrastructure faults; `isDependencyBlockedRun` covers gate cancellations, which are a graph-state fact about the issue, not an infra fault, and must not surface as one via `runtime_failure_streak`. `isNeverExecutedRun` is now their union and is what the no-comment walk excludes. - Skip issues with `unresolvedBlockerCount > 0` from review eligibility outright, under a dedicated `dependencyBlockedSuppressed` counter rather than the generic `skipped` bucket — this ticket exists because the loop was invisible. - Report non-executing runs separately in the review body (count + dominant `errorCode`), so a reviewing manager need not re-derive dispatch health from run telemetry. A dominant code is only named when it holds a strict majority. - Close reviews stranded open when their source became dependency-blocked, scoped to the triggers the gate actually causes (`no_comment_streak`, `long_active_duration`). `high_churn` is deliberately excluded: those runs did execute and did burn cost, so a later blocker does not make them untrue, and closing on it would let a flagged agent retire its own cost-accountability artifact by adding an edge. - Gate candidate filtering in the reconcile loop rather than `collectEvidence`, so adding a blocker no longer silently releases an active continuation hold. Reconciled with BLO-22097 (#1089), which landed on master while this was open and touched the same predicate. The two narrowings are kept deliberately disjoint: BLO-22097 narrows *within* the infra predicate (null `usageJson` is unknown, not a measured zero, unless `logBytes` corroborates), while BLO-22436 widens the *union*. Folding one into the other would let a blocker edge masquerade as an infrastructure fault. Verified: 111/111 in productivity-review-service.test.ts (105 + master's 6), `tsc --noEmit -p server` clean. Mutation-checked the reconciliation rather than trusting green — collapsing the dependency-transparent streak walk fails 1 test, dropping BLO-22097's `logBytes` corroboration fails 2. Both intents are load-bearing and neither was lost in the merge.
…streak, skip blocked issues (BLO-22436) The `no_comment_streak` detector counted runs that never executed toward an agent's silence streak. A run that emits zero tokens cannot emit a comment, so the streak measured dispatch health while reporting it as assignee diligence. Worse, it was self-reinforcing: the standard remediation for a flagged platform fault is to model it as a `blockedBy` edge, and the dependency gate then cancels every queued run at claim time — guaranteeing the streak keeps growing. That is exactly what happened between BLO-21723 and BLO-22262. - Split the zero-token population in two. `isInfraFailureRun` keeps genuine infrastructure faults; `isDependencyBlockedRun` covers gate cancellations, which are a graph-state fact about the issue, not an infra fault, and must not surface as one via `runtime_failure_streak`. `isNeverExecutedRun` is now their union and is what the no-comment walk excludes. - Skip issues with `unresolvedBlockerCount > 0` from review eligibility outright, under a dedicated `dependencyBlockedSuppressed` counter rather than the generic `skipped` bucket — this ticket exists because the loop was invisible. - Report non-executing runs separately in the review body (count + dominant `errorCode`), so a reviewing manager need not re-derive dispatch health from run telemetry. A dominant code is only named when it holds a strict majority. - Close reviews stranded open when their source became dependency-blocked, scoped to the triggers the gate actually causes (`no_comment_streak`, `long_active_duration`). `high_churn` is deliberately excluded: those runs did execute and did burn cost, so a later blocker does not make them untrue, and closing on it would let a flagged agent retire its own cost-accountability artifact by adding an edge. - Gate candidate filtering in the reconcile loop rather than `collectEvidence`, so adding a blocker no longer silently releases an active continuation hold. Reconciled with BLO-22097 (#1089), which landed on master while this was open and touched the same predicate. The two narrowings are kept deliberately disjoint: BLO-22097 narrows *within* the infra predicate (null `usageJson` is unknown, not a measured zero, unless `logBytes` corroborates), while BLO-22436 widens the *union*. Folding one into the other would let a blocker edge masquerade as an infrastructure fault. Verified: 111/111 in productivity-review-service.test.ts (105 + master's 6), `tsc --noEmit -p server` clean. Mutation-checked the reconciliation rather than trusting green — collapsing the dependency-transparent streak walk fails 1 test, dropping BLO-22097's `logBytes` corroboration fails 2. Both intents are load-bearing and neither was lost in the merge.
…streak, skip blocked issues (BLO-22436) The `no_comment_streak` detector counted runs that never executed toward an agent's silence streak. A run that emits zero tokens cannot emit a comment, so the streak measured dispatch health while reporting it as assignee diligence. Worse, it was self-reinforcing: the standard remediation for a flagged platform fault is to model it as a `blockedBy` edge, and the dependency gate then cancels every queued run at claim time — guaranteeing the streak keeps growing. That is exactly what happened between BLO-21723 and BLO-22262. - Split the zero-token population in two. `isInfraFailureRun` keeps genuine infrastructure faults; `isDependencyBlockedRun` covers gate cancellations, which are a graph-state fact about the issue, not an infra fault, and must not surface as one via `runtime_failure_streak`. `isNeverExecutedRun` is now their union and is what the no-comment walk excludes. - Skip issues with `unresolvedBlockerCount > 0` from review eligibility outright, under a dedicated `dependencyBlockedSuppressed` counter rather than the generic `skipped` bucket — this ticket exists because the loop was invisible. - Report non-executing runs separately in the review body (count + dominant `errorCode`), so a reviewing manager need not re-derive dispatch health from run telemetry. A dominant code is only named when it holds a strict majority. - Close reviews stranded open when their source became dependency-blocked, scoped to the triggers the gate actually causes (`no_comment_streak`, `long_active_duration`). `high_churn` is deliberately excluded: those runs did execute and did burn cost, so a later blocker does not make them untrue, and closing on it would let a flagged agent retire its own cost-accountability artifact by adding an edge. - Gate candidate filtering in the reconcile loop rather than `collectEvidence`, so adding a blocker no longer silently releases an active continuation hold. Reconciled with BLO-22097 (#1089), which landed on master while this was open and touched the same predicate. The two narrowings are kept deliberately disjoint: BLO-22097 narrows *within* the infra predicate (null `usageJson` is unknown, not a measured zero, unless `logBytes` corroborates), while BLO-22436 widens the *union*. Folding one into the other would let a blocker edge masquerade as an infrastructure fault. Verified: 111/111 in productivity-review-service.test.ts (105 + master's 6), `tsc --noEmit -p server` clean. Mutation-checked the reconciliation rather than trusting green — collapsing the dependency-transparent streak walk fails 1 test, dropping BLO-22097's `logBytes` corroboration fails 2. Both intents are load-bearing and neither was lost in the merge.
1 participant
Thinking Path
Linked Issues or Issue Description
isNeverExecutedRuntreats absent usage telemetry as proof of zero turnsisNeverExecutedRun. Finding 1 raised by Ally's review on fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041, verified by the CTO against production run rows, and deliberately deferred to this follow-up rather than blocking fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041's merge (see #1041#issuecomment-5191148657).TERMINAL_RUN_STATUSESomittingerror/adapter_failed) — pre-existing onmaster, orthogonal to this predicate, tracked separately.Stack: parent #1041. This PR targets
platformsre/blo-21769-runtime-failure-streak-predicate(notmaster) because it edits theisNeverExecutedRunpredicate #1041 introduces and hasn't merged yet. Base branch:platformsre/blo-21769-runtime-failure-streak-predicate. After #1041 merges: change this PR's base tomaster, reconcile commits against the squash/merge result, verify the final diff is exactly this change, and request fresh review.Related PRs found by search: none — searched for
BLO-22097,isNeverExecutedRun, andlogBytes never-executed; #1041 (the parent) is the only match.What Changed
server/src/services/productivity-review.tsisNeverExecutedRunnow branches onrun.usageJson == null: when usage is unknown, it corroborates withrun.logBytesagainst a newNEVER_EXECUTED_UNKNOWN_USAGE_LOG_BYTES_CEILING(200,000 bytes) instead of assuming zero tokens.usageJsonis present (not null), behavior is byte-for-byte identical to fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041 —runUsageTokenCountsstill decides,logBytesis never consulted. An explicit measured zero is never second-guessed by a large log.server/src/__tests__/productivity-review-service.test.tsinsertRunshelper takes an optionallogBytes.reconcileProductivityReviewspath:logBytes(claude_truncated-shaped) → counts towardno_comment_streak.logBytesat the observed 111,337-byte boundary → still excluded (positive control — large log does not override an explicit zero).logBytes(crashloop) → still excluded (positive control — fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041's original fix does not regress).Verification
92 = 89 existing (from #1041, unmodified in intent, all still pass) + 3 new.
tsc --noEmitclean onserver(after buildingpackages/sharedand@paperclipai/plugin-sdk, both required by the workspace regardless of this change).Manual verification of the
logBytesseparation (acceptance criteria requires confirming it holds beyond the original BLO-19924 sample), pulled from the live production/companies/:companyId/heartbeat-runsendpoint, filtered client-side to each issue'scontextIssueId:job_failed×2 → null/null;provider_throttled_no_progress→ explicit 0/0,logBytes10,365;provider_transient_upstream→ explicit 0/0,logBytes10,074;claude_transient_upstream→ explicit 0/0,logBytes46,400. No counter-example present, but the boilerplate band and the null/null band both hold.job_failed→ null/null;provider_throttled_no_progress→ explicit 0/0,logBytes14,504;claude_transient_upstream→ explicit 0/0,logBytes19,867.No contradicting evidence found in either set — every explicit-zero-usage row stayed at or below 46,400 bytes, well under the 200,000-byte floor, and every null-usage row observed also had null
logBytes. The API has no pagination beyond 1000 rows per agent and no per-issue filter, so this is the deepest history retrievable read-only; it does not reach back far enough to re-observe BLO-19924's originalba6d6bbdcounter-example directly (outside the CTO's original field validation), but it does not contradict the separation either.Manual re-run of the predicate over BLO-19924's 27-run set (the acceptance criterion's other manual check) was not independently re-run in this PR — I do not have a tool that reproduces that exact 27-row extraction without re-deriving it from raw production DB access I don't have in this environment. The unit test suite covers the same three shapes (null+high-logBytes counts, explicit-zero+boundary-logBytes excluded, null+null excluded) as integration tests against the real code path, which is the automated verifying signal the issue specifies.
Risks
livenessState: "failed"runs withusageJson: null— a narrow slice. Runs with explicit usage (zero or non-zero) take the exact same path as fix(productivity-review): exclude never-executed runs from no_comment_streak (BLO-21769) #1041 already ships.Model Used
claude-sonnet-5[1m], 1M context), via the PlatformSREEngineer Paperclip agent (claude_k8sadapter), with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code