Suppress long_active_duration productivity review for deliberate future monitors - #33
Merged
Merged
Conversation
…re monitors Adds MonitorScheduledSuppression handling to productivity-review service so a `long_active_duration` review is not created (and any open one is closed) when the source issue has a future `monitorNextCheckAt` set deliberately by the `assignee` or `board` scheduler. `no_comment_streak` and `high_churn` triggers remain active. Records an activityLog row when a long-active review is suppressed or closed by this rule. The issue's manager/CEO wording is mapped onto the platform's existing normalized scheduler values; introducing distinct `manager`/`ceo` scheduler values is a separate schema/API change and intentionally out of scope. Refs BLO-5505, BLO-5509.
5 tasks
kkroo
added a commit
that referenced
this pull request
May 16, 2026
* fix(test): v513 fallout — regex, positional arg, hookTimeout
Three uncorrelated v513 merge fallouts, all blocking master's verify_canary
and PR verify on Blockcast/paperclip:
1. issues-identifier-provider: Drizzle now wraps PG errors in
`Failed query: ...` text that no longer surfaces the constraint name
in the matchable message. Match on the column name instead — the
wrapped SQL still includes `identifier_provider`, which is a stable
signal the DB rejected the unknown value.
2. plugin-orchestration-apis (2 sites): `buildHostServices` grew a 6th
positional `lifecycleManager` argument between the merge bases, so
`{manifest:...}` was landing in the lifecycleManager slot and
`options.manifest` was undefined. Add an explicit `undefined` for
lifecycleManager so the manifest reaches position 7.
3. backup-lib: same `Hook timed out in 10000ms` shape as `5f6f9c04`
bumped for `client.test.ts` — embedded-postgres `afterEach` cleanup
can exceed the 10s default under CI runner load. Bump to 60s to
match the surrounding test-level timeout.
Refs paperclip-v513-merge-fallout-pattern.md. The github-webhook +
environment-runtime fallouts have deeper causes (test isolation
leak from background heartbeat execution, secret-binding gaps,
spy-call drift) and are tracked for separate cleanup commits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(test): github-webhook beforeEach — force-finalize stuck heartbeat runs
The wake-driving test (`drives a wake on check_run.completed ...`) calls
`heartbeat.wakeup(...)` which fires off a real `void executeRun(...)`
fire-and-forget background execution against the embedded postgres. That
execution can outlive the test it was spawned in — when the next test's
beforeEach runs, the queued/running row is still in-flight, holding row
locks that block the FK-cascade through `TRUNCATE companies CASCADE`,
and the 100-iter × 50ms drain loop hits its 10s hookTimeout (max 5s of
polling + DB query time piles up beyond the default budget).
Two-part fix:
- Switch the drain to a wall-clock-deadline (30s) so the loop genuinely
uses the available time budget, not just 100 iterations × 50ms.
- After drain (whether converged or not), force-finalize any remaining
queued/running rows to `failed` before TRUNCATE so the cascade can't
block on in-flight executeRun row locks.
- Bump hookTimeout to 60s to absorb the worst case under CI load.
Closes the 2 remaining github-webhook failures from CI verify on PR #33
and the same 2 reproducing on devbox.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(env-runtime): v513 fallout — restore paperclipApiUrl + seed bindings + 4-arg spy assertions
Three correlated v513 merge fallouts in environment-runtime tests + service:
1. Restore `paperclipApiUrl` to SSH lease metadata. The v513 merge
(f69bb69) "recover upstream changes for 13 medium-complexity files"
dropped this single line from the lease metadata in
server/src/services/environment-runtime.ts. The cache-lookup at the
top of acquireRunLease still READS `lease.metadata.paperclipApiUrl`
for the last-known-good probe optimization (BLO-1489), so the read
side was harmless but the cache could never populate. Three SSH tests
exposed this — restore the write.
2. Seed `company_secret_bindings` rows in both `seedEnvironment` helpers
(environment-runtime.test.ts + environment-runtime-driver-contract).
v513 introduced the bindings table as a required gate on secret_ref
resolution; without a binding row, `secretService.resolve` throws
"Secret is not bound to environment:<id> at <configPath>". Affects
the privateKey-converted SSH tests and the apiKey secret-ref sandbox
test (separate binding insert in the test body for apiKey since the
secret is created outside seedEnvironment).
3. Update spy assertions for v513's added 4th positional argument
(RPC timeout in ms) on `pluginWorkerManager.call(...)` for sandbox
provider paths AND for `executePluginEnvironmentCommand`. Adds
`expect.any(Number)` as 4th arg on five assertions across three
sandbox tests + one plugin-driver test. Plugin-driver
acquire/release/resume/destroy paths use 3 args (no timeout) but
now pass `issueId` in the call object — adds `issueId: null` to
four spy assertions.
All 20 tests across both files pass after these fixes.
Refs paperclip-v513-merge-fallout-pattern.md, BLO-1489.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
Merged
4 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
What Changed
server/src/services/productivity-review.ts: addsMonitorScheduledSuppressionevidence shape,deliberateFutureMonitor()guard,recordMonitorScheduledSuppression()(logsissue.productivity_review_suppressedactivity row), andcloseOpenSuppressedMonitorReviews()(closes any currently openlong_active_durationreview whose source has a deliberate future monitor — backfill path so we don't have to triage the queued BLO-550x batch by hand).reconcileProductivityReviewscalls the closer at the top of each cycle and short-circuits the create path when evidence is a suppression.isProductivityReviewContinuationHoldActiveis type-narrowed so the suppression shape never enters the hold path.server/src/__tests__/productivity-review-service.test.ts: extendsseedAssignedIssue()withmonitorNextCheckAt/monitorScheduledByopts, adds 5 new vitest cases (suppress, expired-monitor still fires, no-comment not suppressed, close-open long-active, do-not-close open no-comment), and adjusts the existing high-churn test to assertmonitorScheduledSuppressed: 0when the source has a future monitor.Reviewer-confirmed scope (BLO-5509 description):
assigneeandboardare the only normalized deliberate scheduler values persisted today. If distinctmanager/ceoscheduler values are desired, that is a separate schema/API change and intentionally out of scope.Verification
pnpm exec vitest run server/src/__tests__/productivity-review-service.test.ts→ 16/16 passed (28.6s)pnpm --filter @paperclipai/server typecheck→ 0 errorsRisks
Low risk. The suppression heuristic is gated narrowly:
long_active_durationis affected —no_comment_streak,high_churn, etc. continue to fire on the same source issue unchanged.monitorNextCheckAt > now()ANDmonitorScheduledBy ∈ {assignee, board}(auto-monitor recovery rows are notassignee/boardscheduled, so they don't trigger suppression).long_active_duration; if the trigger has switched (e.g., review was updated to a no_comment_streak), it stays open.Model Used
Checklist
Refs BLO-5505, BLO-5509.