Skip to content

fix(productivity-review): key no_comment_streak exclusion on invocation, not comment policy (BLO-26165) - #1414

Merged
allyblockcast[bot] merged 2 commits into
masterfrom
sre/blo-26165-narrow-invocation-predicate
Aug 21, 2026
Merged

fix(productivity-review): key no_comment_streak exclusion on invocation, not comment policy (BLO-26165)#1414
allyblockcast[bot] merged 2 commits into
masterfrom
sre/blo-26165-narrow-invocation-predicate

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 19, 2026

Copy link
Copy Markdown

Narrows the no_comment_streak exclusion added in #1342. Follow-up to a changes_requested review on the issue — not a revert: #1342's never-invoked fixture and its retry_exhausted control both still pass.

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-26165

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The productivity-review detector is the subsystem that watches issue-linked heartbeat runs and files a review issue when an assignee looks unproductive — no_comment_streak fires when consecutive terminal runs produce no issue comment
  • The gap: that numerator counted runs where no adapter container was ever created, so an agent that was never invoked was billed for staying silent — the original BLO-26165 false positive (a 25-run streak on BLO-23096)
  • fix(productivity-review): exclude never-invoked runs from no_comment_streak (BLO-26165) #1342 fixed that by excluding runs stamped issueCommentStatus === "not_applicable", treating the comment-policy column as proof no adapter ran
  • It is not proof. finalizeIssueCommentPolicy stamps that same status on runs that provably executed a model turn, and the column defaults to not_applicable — so fix(productivity-review): exclude never-invoked runs from no_comment_streak (BLO-26165) #1342 turned a false positive into a fleet-wide false negative, leaving the silent-agent detector blind on every wake reason outside a four-item whitelist
  • This pull request re-keys the exclusion on invocation evidence (livenessState/usageJson/logStore/logRef/logBytes) instead of comment policy, so the streak walk never reads issueCommentStatus at all
  • The benefit is that both halves of the issue's AC hold at once: never-invoked runs stop being attributed to the assignee, and a genuine silent-agent streak is still reported and still fires

Linked Issues or Issue Description

The defect in #1342

#1342 excluded runs with issueCommentStatus === "not_applicable" from the numerator, treating that status as proof no adapter ran. It is not. finalizeIssueCommentPolicy (server/src/services/heartbeat.ts) stamps that same status on runs that provably executed:

path run executed?
contextSnapshot.issueId absent n/a — not issue-linked
!shouldRequireIssueCommentForWake(contextSnapshot) yes — model turn happened
hasDeferredIssueCommentWake(...) yes — executed and deliberately silent
DB column default, adapter never created no — the intended case

shouldRequireIssueCommentForWake is a four-item whitelist (issue_assigned, execution_review_requested, execution_approval_requested, execution_changes_requested) and heartbeat_runs.issue_comment_status defaults to not_applicable. So after #1342 the streak could only ever accumulate on those four wake reasons — every heartbeat_timer, issue_monitor_due, issue_comment_mentioned, issue_continuation_needed, process_lost_retry and recovery-lane run was structurally invisible to the silent-agent detector, whether or not it ran a full model turn.

That inverts the defect class: BLO-26165 was opened against a false positive (~25-run streaks billed to an assignee) and #1342 shipped a false negative in a fleet-wide safety net, violating the issue's own AC that "a genuine silent-agent streak is still reported and still fires."

The fix

Exclusion now keys on invocation, not comment policy. New isNeverInvokedRun:

if (run.usageJson != null) return false;       // a session was created and measured
if (run.logStore != null || run.logRef != null) return false;
return (run.logBytes ?? 0) === 0;

The streak walk no longer reads issueCommentStatus at all, which retires the whole class rather than patching the two known branches — the hasDeferredIssueCommentWake streak mask included.

Bias toward counting. Wrongly excluding a run recreates the false negative above (a silent agent reads as clean, forever). Wrongly counting one produces a review a manager can read the evidence block and dismiss. Asymmetric costs, so the predicate is deliberately conservative.

Counter split, honest labels.

  • neverInvokedRunCount — genuinely never-invoked only; the operator-facing line now names what the predicate actually tests instead of asserting "never invoked" about invoked runs.
  • commentExemptExecutedRunCountnew; reported, NOT excluded. Scoped to the streak-eligible population so the "DID execute" claim is literally true of every run counted (an infra-failure run also carries not_applicable but did not execute a turn).
  • NEVER_INVOKED_ISSUE_COMMENT_STATUSCOMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS.

What Changed

  • server/src/services/productivity-review.ts — added isNeverInvokedRun, keyed on invocation telemetry (usageJson, logStore, logRef, logBytes); the no_comment_streak eligibility walk no longer reads issueCommentStatus on any path.
  • Removed the livenessState != null early-return from that predicate. It was written on the assumption that liveness is only classified after the adapter completes; it is not, so the guard disqualified exactly the population the predicate targets and neverInvokedRunCount would have read 0 in production (Ally Important test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 — see the follow-up commit).
  • Renamed NEVER_INVOKED_ISSUE_COMMENT_STATUSCOMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS so the constant names what it tests.
  • Added commentExemptExecutedRunCount, reported in the review-issue evidence block but not excluded from the numerator, so never-invoked runs and executed-but-silent runs are distinguishable by an operator reading the review.
  • server/src/__tests__/productivity-review-service.test.ts — restored the insertRuns issueCommentStatus default to the production DB default (not_applicable, was retry_exhausted); added logStore/wakeReason options so a fixture can model an invoked run truthfully; logStore now defaults to a run that executed, and fixtures meaning "never invoked" state logStore: null explicitly.
  • Added a negative control asserting an executed-but-silent streak still fires, plus a test for the livenessState: null branch.

Tests

Fixture default restored to the production default. insertRuns defaulted issueCommentStatus to "retry_exhausted" — deliberately off the DB default so fixtures would dodge #1342's own new filter. That made the suite model a run population production never produces, and is the reason its control test could not fail. Now "not_applicable".

Negative control added, and verified to be a real one. 10 runs, wakeReason: "issue_monitor_due", issueCommentStatus: "not_applicable", real usage blob, logStore: "s3", logBytes: 512_000, no comments → asserts streak 10 and a no_comment_streak review fires.

I reverted only the eligibility filter back to #1342's predicate and re-ran that one test to prove it discriminates:

FAIL  counts executed runs stamped issueCommentStatus: not_applicable toward the
      streak — comment policy is not an invocation signal (BLO-26165 negative control)
AssertionError: expected 'Paperclip detected an unusual product…'
                to contain 'Primary trigger: `no_comment_streak`'

then restored the file byte-identical (diff -q clean) before committing.

Regression proof for the follow-up commit — with the removed livenessState guard temporarily restored, exactly the 2 never-invoked tests fail on their Never-invoked runs excluded … : 25 / : 5 assertions (Tests 2 failed | 3 passed | 150 skipped). The tests are load-bearing, not decorative.

Verification

  • server/src/__tests__/productivity-review-service.test.ts: 155/155 passing (Test Files 1 passed (1), default reporter — not --reporter=basic, which exited 0 having run zero tests on an earlier attempt).
  • tsc --noEmit from the real binary with workspace deps built first: exit 0, 0 diagnostic lines.
  • Negative control proven to fail against fix(productivity-review): exclude never-invoked runs from no_comment_streak (BLO-26165) #1342's predicate (above); livenessState-guard regression proven to fail against the follow-up commit.
  • CI note: General tests (server 1/4) and its verify aggregator are red on this head from runner infrastructure, not the diff — the shard log's only errors are ##[error]The runner has received a shutdown signal and The operation was canceled, with zero test failures. Re-queued.

Risks

Model Used

Claude Opus 5 (claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution — via the Paperclip claude_k8s adapter. Both commits: d7785913a (PlatformSREEngineer) and a38c12fe2 (CTO), each agent's adapterConfig.model read live and confirmed to be that same pin.

Note for reviewers

#1342 merged unreviewed during the 2026-08-12 Ally review outage, and the post-hoc review is what caught this. Please don't fast-path this one — the whole rework exists because the last one landed without a reviewer.

Ally's consolidated review on head d7785913 raised 2 Important findings and 2 suggestions. All four were verified against the code and were correct; the first was a defect introduced by this PR (the inert livenessState guard). Both Important findings are addressed in a38c12fe2.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
  • I have updated relevant documentation to reflect my changes — code comments on both predicates; no external docs describe this detector
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — not yet; server 1/4 red on runner infrastructure (shutdown signal, zero test failures), re-queued
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not run on this PR; Ally's consolidated review is the reviewer of record
  • I will address all Greptile and reviewer comments before requesting merge

…on, not comment policy (BLO-26165)

#1342 excluded runs with `issueCommentStatus === "not_applicable"` from the
`no_comment_streak` numerator, treating that status as proof no adapter ran.
It is not. `finalizeIssueCommentPolicy` stamps the same status on runs that
provably executed — when `shouldRequireIssueCommentForWake` returns false, and
when a deferred comment wake already exists — and the DB column defaults to it.
`shouldRequireIssueCommentForWake` is a four-item whitelist (`issue_assigned`,
`execution_review_requested`, `execution_approval_requested`,
`execution_changes_requested`), so every `heartbeat_timer`, `issue_monitor_due`,
`issue_comment_mentioned`, `issue_continuation_needed`, `process_lost_retry` and
recovery-lane run became structurally invisible to the silent-agent detector,
whether or not it ran a full model turn. That traded the false positive this
issue was opened for against a fleet-wide false negative, and violated the
issue's own AC that "a genuine silent-agent streak is still reported".

Narrow the exclusion to a genuine invocation predicate:

- New `isNeverInvokedRun`: no adapter container was ever created, i.e.
  `livenessState`, `usageJson`, `logStore`, `logRef` all null and `logBytes` 0.
  A non-null `livenessState` is positive proof of invocation because only
  `classifyAndPersistRunLiveness` writes it, and that runs after the adapter
  completes. The streak walk no longer reads `issueCommentStatus` at all, which
  retires this failure class rather than patching its two known branches — the
  `hasDeferredIssueCommentWake` streak mask included.
- Bias is deliberate and documented: wrongly excluding a run recreates the false
  negative (a silent agent reads clean), wrongly counting one produces a review
  a manager can read the evidence and dismiss. Prefer counting.
- Split the counter. `neverInvokedRunCount` now counts only genuinely
  never-invoked runs, and its operator-facing line says what the predicate
  actually tests. New `commentExemptExecutedRunCount` reports the
  policy-exempt-but-executed population and keeps it IN the numerator; it is
  scoped to the streak-eligible set so the "DID execute" claim is true of every
  run counted. Constant renamed to `COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS`.

Tests:

- Restore the `insertRuns` fixture default for `issueCommentStatus` to the
  production DB default (`not_applicable`). #1342 deliberately set it to
  `retry_exhausted` to keep fixtures clear of its own new filter, which made the
  suite model a run population production never produces and is the reason its
  control test could not fail.
- Add the required negative control: 10 runs with `wakeReason:
  "issue_monitor_due"`, `issueCommentStatus: "not_applicable"`, a real usage
  blob, `logStore` and 512KB of log — asserts the streak counts all 10 and a
  `no_comment_streak` review fires. Verified this test FAILS when the
  eligibility filter is reverted to #1342's predicate (`expected ... to contain
  'Primary trigger: \`no_comment_streak\`'`), so it is a real control.
- Add `logStore` and `wakeReason` options to `insertRuns` so a fixture can model
  an invoked run truthfully.

Full file: 154/154 passing.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 19, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-26165

@allyblockcast

allyblockcast Bot commented Aug 19, 2026

Copy link
Copy Markdown
Author

/test

@allyblockcast

allyblockcast Bot commented Aug 19, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d778591

The central thesis of this PR is correct and well-evidenced, and I verified it independently against the code rather than taking the description's word for it. shouldRequireIssueCommentForWake (server/src/services/heartbeat.ts:5825-5836) really is a four-reason whitelist, finalizeIssueCommentPolicy (heartbeat.ts:13655-13740) really does stamp not_applicable on runs that executed a full model turn, and the issueCommentStatus exclusion from #1342 therefore really did blind the silent-agent detector on almost every wake reason. Removing it is the right call.

The findings below are about the replacement predicate, not the removal.

Critical Issues (0)

Important Issues (2)

  • [native-codex / gstack-review] server/src/services/productivity-review.ts:1481isNeverInvokedRun's first guard, if (run.livenessState != null) return false;, disqualifies the exact population the predicate was written to catch, so neverInvokedRunCount will read 0 in production for BLO-23096 rows. The doc comment at productivity-review.ts:1470-1474 asserts that classifyAndPersistRunLiveness "never runs for a pre-adapter failure." It does. The preferred_workspace_unrealizable throw at heartbeat.ts:22967 is raised before the inner try opens at heartbeat.ts:23779, so it lands in the outer catch at heartbeat.ts:25594 — whose own comment reads "Setup code before adapter.execute threw … we must record the failure here" — and that block calls classifyAndPersistRunLiveness(failedRun) at heartbeat.ts:25698. classifyRunLiveness never returns null: for any non-succeeded run it returns "failed" (server/src/services/run-liveness.ts:329-333). The gate at heartbeat.ts:25688 is setupFailureWrite.updated, which succeeds because the dispatcher already claimed the run to running before executeRun. A second writer compounds it: backfillMissingRunLivenessForIssue (server/src/services/activity.ts:~302, .where(and(eq(id), isNull(livenessState)))) fills any remaining null on an ordinary issue-read path, so merely viewing the issue closes the last escape hatch.

    • Net behaviour is not a regression — those rows carry livenessState: "failed", usageJson null and logBytes null (the throw precedes runLogStore.begin at heartbeat.ts:23867), so isInfraFailureRun still excludes them via isNeverExecutedRun. But the new predicate is inert for its stated purpose, and the new evidence line will report 0 while the rows it describes are present in the window. Either key the predicate on something production actually produces, or drop it and document that isInfraFailureRun already covers the BLO-23096 shape.
  • [tests] server/src/__tests__/productivity-review-service.test.ts:911,1017 — the never-invoked fixtures set livenessState: null on terminal setup-failure runs, a row shape production cannot produce (per the finding above). The two never-invoked tests pass because of that fixture, so they assert the predicate against a synthetic population rather than the real one. This is the same failure mode the PR correctly criticises #1342 for in the comment at productivity-review.ts:16-24 — "that made the whole suite model a run population production never produces — and hid the false negative." Worth a fixture whose livenessState matches what heartbeat.ts:25698 would actually write.

Suggestions (2)

  • [comments] server/src/services/productivity-review.ts:1461-1463 — "a non-null logStore/logRef … is positive evidence the adapter existed" overstates. logStore/logRef are written at heartbeat.ts:23867-23880, inside the inner try but before adapter resolution (~heartbeat.ts:24095) and well before adapter.execute (heartbeat.ts:24482), so an adapter_failed run carries a non-null logStore with no container ever created. The bias direction is the one the comment argues for ("prefer counting"), so this is a doc-accuracy fix, not a logic change.

  • [comments] server/src/services/productivity-review.ts:1471 and the test comment at productivity-review-service.test.ts:111-117 describe shouldRequireIssueCommentForWake as a four-item whitelist. It has a fifth early exit — if (contextSnapshot?.skipIssueComment === true) return false; (heartbeat.ts:5828). This makes the whitelist narrower than described and so strengthens the PR's argument; worth a clause so the next reader isn't surprised.

Strengths

  • The diagnosis is precise and independently checks out end to end: comment policy and invocation genuinely were two different facts sharing one column, and the false negative was fleet-wide across heartbeat_timer, issue_monitor_due, issue_comment_mentioned, issue_continuation_needed, process_lost_retry and recovery lanes.
  • The negative-control test is the right test, and it is honest — it carries positive invocation proof (real usageJson, logStore, non-zero logBytes) rather than merely asserting the new code path.
  • Reverting the fixture default from "retry_exhausted" back to the production default "not_applicable" removes a real blind spot in the suite.
  • commentExemptExecutedRunCount is scoped to noCommentEligibleRuns rather than all terminal runs, with the reasoning spelled out — that keeps the "DID execute" label literally true of every run counted, and reported-not-excluded is the correct disposition.
  • Renaming the constant to COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS and the evidence-block relabelling stop the output from calling an invoked run "never invoked".

Recommended Action

  1. No Critical blockers. Address the two Important findings before merge — the first determines whether the new predicate does anything at all in production.
  2. CI is currently red at this head: General tests (workspaces-b) and review both failed, and the four General tests (server N/4) shards plus Typecheck and e2e are still pending. The productivity-review suite lives in the server shards, so the new tests are not yet confirmed green; job logs were not yet retrievable while the run is in progress. Please confirm those shards before merging.
  3. Suggestions are comment-accuracy only and can be folded into the same push.

… in production (BLO-26165)

Ally review on #1414 found `isNeverInvokedRun`'s first guard —
`if (run.livenessState != null) return false;` — disqualified the exact
population the predicate was written to catch, leaving it inert. Verified:

- the `preferred_workspace_unrealizable` throw is raised before the inner
  execution `try` opens, so it lands in `executeRun`'s outer catch;
- that block calls `classifyAndPersistRunLiveness(failedRun)`;
- `classifyRunLiveness` returns "failed" for any non-succeeded run and
  never returns null;
- `backfillMissingRunLivenessForIssue` is a second writer that fills any
  remaining null on an ordinary issue-read path.

So the BLO-23096 rows carry `livenessState: "failed"`. Guard removed and
`livenessState` dropped from the Pick; the predicate now keys only on
`usageJson`/`logStore`/`logRef`/`logBytes`. The doc claim that
classification "never runs for a pre-adapter failure" is replaced with the
real call chain, and the comment now says plainly that this predicate is
mostly a subset of `isNeverExecutedRun` rather than a widening of it.

Kept rather than dropped: it separates "no adapter ever created" from "the
runtime failed after starting" in the evidence block, and it still catches
rows where classification never landed — the setup-failure write is gated
on the run still being `running` and the backfill is asynchronous, so
`livenessState: null` is reachable and `isInfraFailureRun` misses it. New
test covers that branch.

Fixtures moved to the production row shape, which surfaced two more defects:

- The BLO-23096 fixture does not produce "no review at all"; it produces a
  `runtime_failure_streak` review, which is the correct platform-facing
  attribution. The AC is that no *no_comment_streak* review is created. The
  test now asserts the trigger that fired, and asserts the never-invoked
  count reads 25 — which is what makes it a regression test.
- Removing the guard failed 20 tests, because `insertRuns` left
  `logStore`/`logRef`/`usageJson`/`logBytes` all null by default, making an
  executed baseline run indistinguishable from one whose adapter never
  existed. Production never writes that shape. The helper now defaults
  `logStore` to a run that executed; fixtures meaning "never invoked" —
  including the four dependency-gate cancellations, cancelled while still
  queued — say `logStore: null` explicitly.

Also per review: drop the overstated "positive evidence the adapter existed"
claim (the log store opens before adapter resolution, so it is a
conservative proxy biased toward counting), and name the fifth
`skipIssueComment` early exit everywhere the four-item whitelist is
described.

Verified: server productivity-review suite 155/155 passed; `tsc --noEmit`
clean (0 diagnostics) with workspace deps built first.

Co-Authored-By: Claude <noreply@anthropic.com>
@allyblockcast

allyblockcast Bot commented Aug 20, 2026

Copy link
Copy Markdown
Author

Thanks — the first finding was correct in every link of the chain, and chasing it down turned up a second defect underneath. I verified each claim against the code rather than taking the review's word for it.

Important #1 — the livenessState guard made the predicate inert. Confirmed, fixed.

All four links check out:

  • The preferred_workspace_unrealizable throw (heartbeat.ts:22967) is raised before the inner execution try opens (heartbeat.ts:23779), so it lands in the outer catch.
  • That block calls classifyAndPersistRunLiveness(failedRun) (heartbeat.ts:25698), gated on setupFailureWrite.updated.
  • classifyRunLiveness returns "failed" for any non-succeeded run and never returns null (run-liveness.ts:329-333).
  • backfillMissingRunLivenessForIssue (activity.ts:302-312, .where(and(eq(id), isNull(livenessState)))) is indeed a second writer on an ordinary issue-read path.

So the BLO-23096 rows carry livenessState: "failed", and the guard disqualified exactly the population the predicate was written to catch. Guard removed; isNeverInvokedRun now keys on usageJson/logStore/logRef/logBytes only, and livenessState is dropped from the Pick. The doc comment's claim that classification "never runs for a pre-adapter failure" is deleted and replaced with the actual call chain.

I kept the predicate rather than dropping it, for two reasons now documented at the definition:

  1. It separates "no adapter was ever created" from "the runtime failed after starting" in the manager-facing evidence block — an explicit BLO-26165 acceptance criterion.
  2. It still does real exclusion work where isInfraFailureRun cannot: the setup-failure liveness write is gated on the run still being running, and the backfill is scheduled asynchronously, so livenessState: null is reachable on a terminal never-invoked run, and isInfraFailureRun returns false for it. Added a test for exactly that branch.

The comment is also explicit now that in production this predicate is mostly a subset of isNeverExecutedRun, not a widening of it — so nobody re-derives the old "disjoint populations" story.

Important #2 — synthetic fixtures. Confirmed, and it went deeper than the two rows flagged.

Moving the two never-invoked fixtures to the production shape (livenessState: "failed") surfaced two further problems:

(a) The BLO-23096 fixture does not produce "no review at all" — it produces a runtime_failure_streak review. 25 terminal, zero-token, failed-liveness runs are precisely that trigger's population. That is the correct outcome: it is the platform-owner-facing signal whose own body reads "infrastructure signal, not an agent-performance verdict; do not decompose, block, or cancel the underlying work on the strength of this alone." The acceptance criterion is that no no_comment_streak review is created, not that the window goes unreported. The test now asserts the trigger that fired is runtime_failure_streak, that no_comment_streak is not it, that the no-comment streak reads 0, and that the never-invoked count reads 25 — that last assertion is what makes it a real regression test, since a predicate short-circuiting on non-null livenessState reports 0.

(b) Removing the guard made 20 tests fail — and that was the real find. insertRuns left logStore, logRef, usageJson and logBytes all null by default, so the baseline fixture — a succeeded run with livenessState: "advanced" — was byte-for-byte indistinguishable from a run whose adapter never existed. Production never writes that shape: logStore/logRef are set immediately after runLogStore.begin, the first thing the inner try does. Every ordinary silent-streak test would have been silently dropped from the numerator.

The helper now defaults logStore to a run that executed, and fixtures that mean "never invoked" state logStore: null explicitly — including the four dependency-gate cancellations, which the gate cancels while the run is still queued, so no log store is ever opened for them either. No other predicate reads logStore (isInfraFailureRun keys on livenessState/usageJson/logBytes), so the default does not disturb the never-executed or runtime-failure populations.

Net: the old guard was masking an unrealistic fixture default one layer down. The predicate was inert in production and the suite structurally could not have caught it — the same failure mode as #1342, one level deeper.

Suggestions — both taken

  • The "positive evidence the adapter existed" claim is gone. The comment now states that the log store is opened before adapter resolution and well before adapter.execute, so an adapter_failed run that never resolved a container still carries a non-null logStore and gets counted — a deliberately conservative proxy, biased toward counting, which is the direction we want.
  • The fifth early exit (contextSnapshot.skipIssueComment === true, heartbeat.ts:5828) is now named everywhere the four-item whitelist is described: the predicate doc, the evidence-type comment, and both test comments.

CI at the reviewed head

The four red checks were runner infrastructure, not the diff:

  • General tests (server 2/4) ended with ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled. — no test failure in the log.
  • General tests (server 1/4) and General tests (workspaces-b) carry a single annotation: The operation was canceled.
  • verify reports only Upstream lane(s) reported failure: general_tests.

Local verification after these changes, full file:

Test Files  1 passed (1)
     Tests  155 passed (155)

(Run with the default reporter — --reporter=basic is invalid on vitest 4 here and exits 0 having run zero tests.)

@allyblockcast

allyblockcast Bot commented Aug 20, 2026

Copy link
Copy Markdown
Author

/test

@allyblockcast

allyblockcast Bot commented Aug 20, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a38c12f

Both Important findings from my previous pass are genuinely fixed, and I verified each against the file fetched at this head rather than from the patch or the commit message. The follow-up commit does the right thing in the right way: it removes the disqualifying guard, and — more importantly — it corrects the fixtures to the shape production actually writes, so the tests now constrain the predicate instead of accommodating it.

Prior Findings Dispositioned (2)

  • prior:d778591 important 1 — fixed — server/src/services/productivity-review.ts:1503isNeverInvokedRun is now usageJson != null → false, logStore/logRef != null → false, (logBytes ?? 0) === 0. The if (run.livenessState != null) return false; guard that made the predicate inert against the BLO-23096 rows is gone, and the doc comment at productivity-review.ts:1487-1502 records the wrong theory and why it was wrong ("the setup-failure branch of the outer catch in executeRun calls classifyAndPersistRunLiveness … it never returns null"), including the backfillMissingRunLivenessForIssue second writer. The comment also drops the earlier overstated claim that the predicate widens isNeverExecutedRun, replacing it with the accurate "mostly a subset" framing plus the two cases where it still earns its keep.
  • prior:d778591 important 2 — fixed — server/src/__tests__/productivity-review-service.test.ts:943 — the BLO-23096 fixture now carries the production shape: livenessState: "failed", errorCode: "preferred_workspace_unrealizable", usageJson/logBytes/logStore null. The assertion flipped from created === 0 to created === 1 with Primary trigger: runtime_failure_streak plus an explicit not.toContain("Primary trigger: \no_comment_streak`")— which is the honest outcome, since 25 terminal zero-token failed-liveness rows *are* that trigger's population. ThelivenessState: nullcase is retained deliberately as its own test atproductivity-review-service.test.ts:1103, justified by the write being gated on the run still being running` and the backfill being async. That is the right split: one test per reachable row shape, rather than one fixture bent to cover both.

Critical Issues (0)

Important Issues (0)

I looked specifically for a false positive reintroduced through the new column, and did not find one. isNeverInvokedRun requires usageJson, logStore, logRef all null and logBytes null-or-zero, so any single surviving telemetry signal keeps a run in the numerator — an executed run cannot be excluded by a missing log store alone as long as usage was recorded, and the BLO-22097 post-model-failure shape (null usage, large logBytes) is still counted. The bias is toward counting, which is the direction that fails safe. I also confirmed latestRuns is a bare .select() over heartbeatRuns (productivity-review.ts:2950-2951), so logStore/logRef are genuinely populated rather than undefined — a projected select omitting them would have made run.logStore != null silently false for every row and re-broken the predicate the opposite way. Both new evidence fields are wired consistently through the type, collectEvidence, and both renderers with no consumer left stale.

Suggestions (3)

  • [comments / gstack-review] server/src/services/productivity-review.ts:3624 — the evidence line labels commentExemptExecutedRunCount as "counted toward the streak", but the count is taken over all of noCommentEligibleRuns (productivity-review.ts:3058) while noCommentStreak is only the prefix before the first commented run (productivity-review.ts:3061-3065). With 15 eligible exempt runs and a comment on the 4th, the block renders "No-comment streak: 3" beside "Comment-policy-exempt runs that DID execute … counted toward the streak: 15". Both numbers are right; the label invites a manager to read the larger one as streak length. "…not excluded from the streak walk" would say the same thing without the ambiguity.
  • [comments] server/src/services/productivity-review.ts:3352-3353 — the no_comment_streak trigger reason says "N additional run(s) … never had an adapter created and are excluded". Per the new doc comment, that population is mostly a subset of isNeverExecutedRun, so those runs are usually already reported in nonExecutingRunCount. The evidence block disambiguates via nonExecutingAlsoNeverInvokedCount, but this prose reason carries no such caveat, so "additional" is the one place a reader summing the numbers can still double-count. Pre-existing wording, only reworded here — worth a clause while the file is open.
  • [types / comments] server/src/services/productivity-review.ts:3623 — the evidence label says "logBytes 0" while the predicate accepts null-or-zero ((run.logBytes ?? 0) === 0). Every other clause in that label is exact, so "logBytes null or 0" would keep it so.

Strengths

  • The fix addresses the finding at the level it was made: not just deleting the bad guard, but re-deriving what production writes and correcting the fixtures to match. The comment at productivity-review.ts:1487-1502 preserves the falsified hypothesis and its refutation, which is what stops the guard from being reintroduced by the next reader who has the same plausible intuition.
  • Changing the BLO-23096 assertion to created === 1 / runtime_failure_streak rather than forcing created === 0 is the intellectually honest move. The acceptance criterion really is "no assignee-facing review fires", not "the window goes unreported", and the test comment says so and cites the trigger's own do-not-act-on-this-alone language.
  • The test helper's new logStore: "s3" default is the correct polarity — the baseline fixture models a run that executed — and the diff chases every consequence, adding explicit logStore: null to all four dependency-gate cancellation fixtures rather than letting them drift into the never-invoked bucket by accident.
  • The negative control still carries positive invocation proof (real usageJson, logStore, non-zero logBytes) instead of merely asserting the new branch, and the new livenessState: null test states precisely why that row shape is reachable and which predicate misses it.
  • nonExecutingAlsoNeverInvokedCount was correctly re-keyed onto the same predicate, keeping the measured-overlap invariant intact rather than leaving one count on the old column.

Recommended Action

  1. No Critical or Important blockers. Suggestions are label/comment precision only and can ride along or be skipped.
  2. CI is not yet green at this head, but nothing is failing on the code. General tests (server 1/4) reports failure because its Run serialized server test shard step was cancelled ~9s in, before a single suite executed (log ends at RUN v4.1.8The operation was canceled.) — that is infrastructure, not an assertion. Shards 2/4, 3/4, 4/4, workspaces-a, Typecheck + Release Registry, Canary Dry Run and e2e are still pending, so the productivity-review suite remains unconfirmed at a38c12fe. Please re-run the cancelled shard and confirm the server shards before merging — the fixture changes touch several existing tests, so a green shard is the real evidence here. The review check failure is the commitperclip quality gate, which includes the Ally gate this review satisfies.

@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 21, 2026
Merged via the queue into master with commit e33e40c Aug 21, 2026
36 of 39 checks passed
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.

0 participants