Skip to content

fix(adapter-utils): CAS-retry on concurrent SSH workspace restores - #4

Merged
kkroo merged 3 commits into
masterfrom
omar/v513-ssh-fixture-cas-retry
May 14, 2026
Merged

fix(adapter-utils): CAS-retry on concurrent SSH workspace restores#4
kkroo merged 3 commits into
masterfrom
omar/v513-ssh-fixture-cas-retry

Conversation

@kkroo

@kkroo kkroo commented May 14, 2026

Copy link
Copy Markdown

Fixes the 7th v513 fallout — surfaced by master verify_canary (run 25850357649) on commit 65e8460 after PR #3 merged.

The race

integrateImportedGitHead in packages/adapter-utils/src/ssh.ts read the current head, computed a merge, then did git update-ref <ref> <new> <currentHead> as a compare-and-swap. Under concurrent restoreWorkspace() calls against the same localDir (one prepareRemoteManagedRuntime per run, all sharing workspaceLocalDir), the loser's CAS fails with:

fatal: update_ref failed for ref 'refs/heads/main':
cannot lock ref 'refs/heads/main': is at <A's-sha> but expected <old-sha>

Surfaced by packages/adapter-utils/src/ssh-fixture.test.ts > merges concurrent remote commits through the managed runtime restore path — added by upstream during v513 (the merge commit 27b004a4 noted this file specifically: "kkroo's BLO-1497 stale-files-extraction + upstream's concurrent SSH restores"). The upstream test landed but the impl never picked up CAS-retry behavior, so the test was always going to fail on a real concurrent run.

Fix

Wrap snapshot-read + merge + update-ref in a CAS-retry loop. On update-ref's cannot lock ref error, re-read the (now newer) head and re-merge against it. Capped at 8 attempts.

Test plan

  • CI: master verify_canary goes green on the merge commit (currently red on this single test).
  • The fixture test requires a working SSH env-lab; couldn't repro on devbox, but the CAS retry is the standard fix shape for git update-ref races and the failure mode matches the test's documented intent (the test asserts both A's and B's commits land via a "Paperclip SSH sync merge" — only achievable if the loser of the first CAS re-merges against the winner's new head).

🤖 Generated with Claude Code

kkroo and others added 3 commits May 14, 2026 10:18
`integrateImportedGitHead` in packages/adapter-utils/src/ssh.ts read the
current head, computed a merge, then did `git update-ref <ref> <new>
<currentHead>` as a compare-and-swap. Under concurrent restoreWorkspace()
calls against the same localDir (one prepareRemoteManagedRuntime per run,
all sharing workspaceLocalDir), the loser's CAS fails with:

  fatal: update_ref failed for ref 'refs/heads/main':
  cannot lock ref 'refs/heads/main': is at <A's-sha> but expected <old-sha>

Surfaced by ssh-fixture.test.ts > "merges concurrent remote commits
through the managed runtime restore path" (added by upstream during v513).
Master verify_canary failed on this at commit 65e8460.

Fix: wrap the snapshot-read + merge + update-ref in a CAS-retry loop. On
update-ref's "cannot lock ref" error, re-read the (now newer) head and
re-merge. Capped at 8 attempts — each successful CAS makes the next
merge-base shorter so 2-3 attempts is the practical worst case under
the test's 2-way race; the cap defends against pathological N-way races.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…anup variance

client.test.ts > applyPendingMigrations > "replays migration 0044 safely
when its schema changes already exist" failed on master verify_canary and
PR #4 verify with `Hook timed out in 10000ms` in the afterEach cleanup loop
under CI runner load. The cleanup stops the embedded-postgres instance for
each test; on a loaded self-hosted runner that can exceed vitest's default
10s hook timeout.

Not v513 fallout — file was last touched 2026-04-20 by Dotta, well before
the merge. Just a flake under CI load. Bump to 60s to absorb the variance;
matches the test-level timeout the rest of the file uses for embedded-pg work.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 5d2bde2 FLAKY-SKIP on these two tests blamed a scheduler race in
claimQueuedRun / withAgentStartLock / startNextQueuedRunForAgent. That
hypothesis was wrong. Static reading was correct; there is no race
in those paths for these test scenarios.

Actual cause: two PRODUCTION auto-recovery paths fire fire-and-forget
corrective wakes after each successful run, and the test mock triggers
both:

1. handleSuccessfulRunHandoff (heartbeat.ts:8552) — when a run
   "succeeded but issue lacks a valid disposition" (issue.status =
   in_progress after the executeRun-time checkout, isProductiveSuccessfulRun
   true because the mock's resultJson.summary is non-empty).
2. finalizeIssueCommentPolicy (heartbeat.ts:8549) — when wakeReason is
   one of {issue_assigned, execution_review_requested,
   execution_approval_requested, execution_changes_requested} but no
   issue comment was posted by the run. The mock doesn't post comments,
   so this enqueues a missing_issue_comment retry wake.

Each path enqueues a wake which is then claimed by the agent's queued
scheduler and dispatched via void executeRun(...). Those wakes race the
test's `expect(mockAdapterExecute).toHaveBeenCalledTimes(N)` assertion,
producing N+1 or N+2 spy calls before the test reads the count. A
console.log inside executeRun masks the race not because it fixes
anything, but because it perturbs V8 microtask scheduling enough that
the test's expect() runs before the corrective wake's mockAdapter call.

Fix is test-only, two parts:

- Drop `summary` from the mock's resultJson (and the top-level
  adapterResult.summary). buildDetectedSuccessfulRunProgressSummary
  reads run.nextAction / run.livenessReason / resultJson.summary /
  result / message, so dropping summary is sufficient to make
  isProductiveSuccessfulRun return false → handoff skipped. resultJson
  still satisfies isEmptyResult via the remaining `exitCode: 0` field.

- New adapterCalledForRun(runId) helper. Assertions check WHICH run
  hit the adapter, not raw spy count. Tolerates the missing_issue_comment
  retry wake (which we can't easily suppress short of changing wakeReason
  away from the test's intended `issue_assigned`) without false positives.

Verified: all 5 tests in the file pass 3/3 consecutive runs; server
tsc --noEmit clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kkroo
kkroo merged commit 209c7d5 into master May 14, 2026
1 check passed
@kkroo
kkroo deleted the omar/v513-ssh-fixture-cas-retry branch May 14, 2026 19:11
kkroo added a commit that referenced this pull request May 18, 2026
The UI workspace's vitest config inherited the same 5s test / 10s
hook defaults that bit server tests pre-PR #56. CompanyAccess.test.tsx
test #1 ("keeps the page human-focused...") timed out at 5545ms on
verify_canary run 26011982864 — barely over the 5s cap, classic
cold-import-on-first-test flake.

Mirrors the server-side bump in PR #56:
  testTimeout: 30_000
  hookTimeout: 60_000
  teardownTimeout: 30_000

Doesn't address CompanyAccess test #4 ("shows protected member
removal reasons from the API"), which renders empty container.textContent —
that's a genuine logic failure (component crashed silently when
mock returns a member with `removal` field), not a timeout.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kkroo added a commit that referenced this pull request May 19, 2026
…s cancelled

executeRun's finally block calls startNextQueuedRunForAgent unconditionally,
including when the run finalized as cancelled. cancelRunInternal already
calls startNextQueuedRunForAgent itself when it cancels a run, so the
finally-block dispatch is a duplicate that races with the cancel-path
dispatch.

This is the deferred follow-up #4 from the verify_canary saga (closed via
PR #68's `skipQueuedRunDispatch` option). PR #68 closed the test-side
deadlock by suppressing dispatch in tests entirely; this PR addresses the
production-side duplication by skipping dispatch only when the just-
finalized run was cancelled.

Lease release and runtime-services cleanup above the dispatch site continue
to run unconditionally — those are correctness paths and must complete
regardless of how the run ended.

Defensive details (incorporating review feedback):

1. Re-read run status immediately before the dispatch decision rather than
   reusing the `latestRun` snapshot from the top of the finally block. The
   snapshot can be stale by hundreds of ms (lease + runtime-service release
   + lifecycle-hook scheduling all happen in between), and a concurrent
   `cancelRunInternal` may flip the row in that window.

2. Fail-safe on read error: explicit try/catch around the status re-read.
   On failure, log the error and SKIP dispatch (rather than silently
   defaulting to dispatch, which would re-open the duplicate-dispatch race
   precisely when the DB is unstable). The cost of skipping is a brief
   queue-latency increase until the next wake-cycle picks up the queued
   run; the cost of double-dispatch is double lease release + double
   runtime-service cleanup, which is worse.

   Predicate scope verified: `"cancelled"` is the only terminal status
   written by an external caller (cancelRunInternal) that also dispatches.
   `succeeded`, `failed`, `timed_out` are all written inside executeRun's
   own try block, with no external dispatcher to race against.

Tests:

- New file `heartbeat-finalize-cancelled-skip-dispatch.test.ts` with two
  cases:
  - cancelled finalize: adapter flips the run to cancelled mid-execute,
    and the second queued run for the same agent stays queued. Asserted
    via a 200ms quiescence window (no transition off `queued`) rather than
    a single point read — defends against the duplicate dispatch landing
    on a microtask after executeRun's promise resolves.
  - succeeded finalize (control): default mock returns success, the
    second queued run transitions off `queued` — guards against the
    cancellation skip over-firing. Uses a polling `waitForStatus` helper
    rather than a single read.
- RED verified by temporary revert of the gate (unconditional dispatch
  re-introduced): cancelled-skip test fails (duplicate dispatch claims
  run #2), succeeded-control test still passes. Restored, both green.
- Regression set expanded to six heartbeat suites (was four): adds
  heartbeat-process-recovery and heartbeat-dependency-scheduling on top of
  finalize-cancelled, active-run-output-watchdog, stale-queue-invalidation,
  retry-scheduling. 84 tests across 6 files. Each file passes when run in
  isolation; one inter-suite state-pollution flake in the combined run on
  process-recovery (FK violation on heartbeat_run_events for a run_id from
  a sibling suite's TRUNCATE leak) — same shape as the v513 saga noise,
  not caused by this PR.

Test seam: adds `__test_executeRunForTesting` on the heartbeatService
return object, mirroring the precedent of `__test_unsafelyTrackActiveRunExecution`.
Returns the bare executeRun promise so tests can deterministically await
the finally block; production callers continue to use the fire-and-forget
`void executeRun(...).catch(...)` form.

Background: see memory `paperclip_release_verify_canary_test_infra.md`
for the full saga and PR chronology (#55 through #72).
kkroo added a commit that referenced this pull request May 19, 2026
…s cancelled (#80)

executeRun's finally block calls startNextQueuedRunForAgent unconditionally,
including when the run finalized as cancelled. cancelRunInternal already
calls startNextQueuedRunForAgent itself when it cancels a run, so the
finally-block dispatch is a duplicate that races with the cancel-path
dispatch.

This is the deferred follow-up #4 from the verify_canary saga (closed via
PR #68's `skipQueuedRunDispatch` option). PR #68 closed the test-side
deadlock by suppressing dispatch in tests entirely; this PR addresses the
production-side duplication by skipping dispatch only when the just-
finalized run was cancelled.

Lease release and runtime-services cleanup above the dispatch site continue
to run unconditionally — those are correctness paths and must complete
regardless of how the run ended.

Defensive details (incorporating review feedback):

1. Re-read run status immediately before the dispatch decision rather than
   reusing the `latestRun` snapshot from the top of the finally block. The
   snapshot can be stale by hundreds of ms (lease + runtime-service release
   + lifecycle-hook scheduling all happen in between), and a concurrent
   `cancelRunInternal` may flip the row in that window.

2. Fail-safe on read error: explicit try/catch around the status re-read.
   On failure, log the error and SKIP dispatch (rather than silently
   defaulting to dispatch, which would re-open the duplicate-dispatch race
   precisely when the DB is unstable). The cost of skipping is a brief
   queue-latency increase until the next wake-cycle picks up the queued
   run; the cost of double-dispatch is double lease release + double
   runtime-service cleanup, which is worse.

   Predicate scope verified: `"cancelled"` is the only terminal status
   written by an external caller (cancelRunInternal) that also dispatches.
   `succeeded`, `failed`, `timed_out` are all written inside executeRun's
   own try block, with no external dispatcher to race against.

Tests:

- New file `heartbeat-finalize-cancelled-skip-dispatch.test.ts` with two
  cases:
  - cancelled finalize: adapter flips the run to cancelled mid-execute,
    and the second queued run for the same agent stays queued. Asserted
    via a 200ms quiescence window (no transition off `queued`) rather than
    a single point read — defends against the duplicate dispatch landing
    on a microtask after executeRun's promise resolves.
  - succeeded finalize (control): default mock returns success, the
    second queued run transitions off `queued` — guards against the
    cancellation skip over-firing. Uses a polling `waitForStatus` helper
    rather than a single read.
- RED verified by temporary revert of the gate (unconditional dispatch
  re-introduced): cancelled-skip test fails (duplicate dispatch claims
  run #2), succeeded-control test still passes. Restored, both green.
- Regression set expanded to six heartbeat suites (was four): adds
  heartbeat-process-recovery and heartbeat-dependency-scheduling on top of
  finalize-cancelled, active-run-output-watchdog, stale-queue-invalidation,
  retry-scheduling. 84 tests across 6 files. Each file passes when run in
  isolation; one inter-suite state-pollution flake in the combined run on
  process-recovery (FK violation on heartbeat_run_events for a run_id from
  a sibling suite's TRUNCATE leak) — same shape as the v513 saga noise,
  not caused by this PR.

Test seam: adds `__test_executeRunForTesting` on the heartbeatService
return object, mirroring the precedent of `__test_unsafelyTrackActiveRunExecution`.
Returns the bare executeRun promise so tests can deterministically await
the finally block; production callers continue to use the fire-and-forget
`void executeRun(...).catch(...)` form.

Background: see memory `paperclip_release_verify_canary_test_infra.md`
for the full saga and PR chronology (#55 through #72).
kkroo added a commit that referenced this pull request May 19, 2026
… in same PR

CI on this PR's first run showed BLO-6119's "collapses duplicate queued
runs" test is actually flaky, not "already fixed" — my earlier 2/2 local
passes on master HEAD were lucky. Same shape as the existing v513 fallout:
fire-and-forget background wake from handleSuccessfulRunHandoff +
finalizeIssueCommentPolicy can create a 3rd row before the test reads
heartbeat_runs.

Replace `expect(runs).toHaveLength(2)` with explicit defined checks for
winner + loser. Any 3rd corrective-wake row has a different id from
olderRunId / newerRunId and is tolerated. Same pattern as PR #4's
6a056f8 fix in heartbeat-dependency-scheduling.

Closes BLO-6119 (re-opened — original close was based on a flaky local
pass).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kkroo added a commit that referenced this pull request May 19, 2026
… in same PR (#85)

CI on this PR's first run showed BLO-6119's "collapses duplicate queued
runs" test is actually flaky, not "already fixed" — my earlier 2/2 local
passes on master HEAD were lucky. Same shape as the existing v513 fallout:
fire-and-forget background wake from handleSuccessfulRunHandoff +
finalizeIssueCommentPolicy can create a 3rd row before the test reads
heartbeat_runs.

Replace `expect(runs).toHaveLength(2)` with explicit defined checks for
winner + loser. Any 3rd corrective-wake row has a different id from
olderRunId / newerRunId and is tolerated. Same pattern as PR #4's
6a056f8 fix in heartbeat-dependency-scheduling.

Closes BLO-6119 (re-opened — original close was based on a flaky local
pass).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kkroo pushed a commit that referenced this pull request Jun 5, 2026
…102) (#303)

Diff 1 of BLO-9102 (the quick win; does not block the run→PR-linkage piece).

Closes two cost-attribution gaps in the opencode-local adapter, both
descendants of BLO-7436's token×list-price fallback:

1. $0 holes — any opencode model absent from the pricing table silently
   reported costUsd=0. openai/gpt-5.3-codex did this across the whole
   2026-05-15→06-05 window. Add every advertised + observed in-use model
   (gpt-5.3-codex, gpt-5.2, gpt-5.1-codex-max, gpt-5.1-codex-mini). A new
   coverage test fails if a model is added to the index.ts allowlist /
   modelProfiles without a matching price, so this cannot silently regress.
   (acceptance #3)

2. metered-vs-estimate asymmetry — Claude lines are true metered API cost
   while opencode lines are list-price estimates; rollups compared them as
   equivalent. Add AdapterCostSource ("metered" | "list_estimate" |
   "unknown"), classify it in the adapter (pure, unit-tested
   classifyCostSource), and persist it into usage_json alongside costUsd.
   (acceptance #4)

billingType semantics are intentionally untouched — costSource is a new
orthogonal axis so existing billingType-keyed cost rollups don't shift.

RATE ACCURACY: the new rates are sibling-anchored UNVERIFIED estimates —
the gpt-5.x openai/ versions are largely absent from LiteLLM's
model_prices_and_context_window.json as of 2026-06. Wrong cents misattribute
rollups but cannot break functionality (the fallback is informational and
never gates a run); the list_estimate flag is exactly what makes a later
rate re-verification auditable. Existing 5.4/5.4-mini rates look understated
vs LiteLLM's azure_ai/ variants — flagged in-file for a follow-up re-verify.

Co-authored-by: kkroo <blockcast-ci-packages[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kkroo added a commit that referenced this pull request Jun 5, 2026
…t rollup (BLO-9117) (#308)

* feat(efficiency): issue_pull_requests storage + authored-LOC exclusion util (BLO-9117)

- New issue_pull_requests table (schema + hand-authored idempotent migration
  0106; journal intentionally stays stale, matching 0103-0105). No pr_author
  column: the author-agnostic join guard is structural, not just a convention.
- Shared authored-LOC exclusion set (public/js, public/client-js/assets, vendor,
  *.wasm, lockfiles, *_swaggergen.go, *.pb.go, *_pb2.py, gitlink-only) with a
  per-rule unit test + raw-vs-authored reduction.

Refs BLO-9117 / BLO-9102 Diff 2.

* feat(efficiency): merged-PR forward-capture + author-agnostic linkage/backfill (BLO-9117)

- Centralize extractPaperclipIdentifiers into services/paperclip-identifiers.ts
  (verbatim logic) so the webhook and the linkage service share one
  author-agnostic extractor; add resolveLinkSourceForIdentifier (branch-ref
  preferred, option A).
- github-webhook.ts: on pull_request closed && merged===true, persist the
  issue_pull_requests link for every matched issue and fire-and-forget the
  authored-LOC enrichment. No PR-author is read or stored.
- issue-pull-requests.ts: recordMergedPullRequest (one row/company, strongest
  link source wins), enrichAuthoredLocForRow (paginated pulls/{n}/files),
  reconcileMergedPullRequests (by-repo enumeration, NO author: qualifier; no-ref
  tail stored with issueId=null for honest option-C coverage), and a pending-LOC
  reconciler.

Refs BLO-9117 / BLO-9102 Diff 2.

* feat(efficiency): /issues/:id/efficiency + apportioned adapter rollup + (A) branch enforcement (BLO-9117)

- issue-efficiency.ts: pure apportionment (output-token-share; per-issue
  authored-LOC sums across adapters to the issue total — no double-count) and
  honest coverage (denominator = all merged PRs in window, structurally
  author-agnostic). DB assembly joins cost_events↔agents (adapterType) and
  surfaces costSource (metered|list_estimate|mixed).
- routes: GET /issues/:id/efficiency and
  GET /companies/:companyId/efficiency/adapter-rollup.
- workspace-runtime.ts (option A): enforce the issue identifier into the agent
  branch name even when a custom branchTemplate omits it, so merged PRs
  ref-link at merge time. Case preserved so the uppercase BLO- form matches.

Refs BLO-9117 / BLO-9102 Diff 2.

* test(efficiency): apportionment/coverage + identity-agnostic linkage + author-filter guard (BLO-9117)

- issue-efficiency.test.ts: pure-function assertions for the two named failure
  modes — data-wall #4 (authored-LOC sums across adapters to the issue total,
  not doubled) and data-wall #2 (coverage = ref-linked/total). Plus an embedded-
  Postgres end-to-end (forIssue + adapterRollup against the real 0106 migration)
  and an identity-agnostic linkage case (a non-kkroo PR links by ref identically;
  recordMergedPullRequest takes no author input).
- issue-pull-requests-identity-guard.test.ts: regression guard that fails if any
  linkage/enumeration/rollup path gains an author: search qualifier, a user.login
  read, or a pr_author column.

Refs BLO-9117 / BLO-9102 Diff 2.

* ci: trigger PR checks (bot-opened PR did not auto-run)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(efficiency): avoid optional-chain narrowing in branch identifier enforcement (BLO-9117)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: kkroo <kkroo@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kkroo added a commit that referenced this pull request Jun 5, 2026
…ption-A lock (BLO-9117) (#309)

* review: coverage forward-only honesty signal + window-clip note + option-A lock + drop dead code (BLO-9117)

Addresses the design-author review on PR #308:
- #1 (coverage vacuous ~100% under forward-only capture): CoverageReport now
  carries reconciledTailObserved + forwardOnly. The forward webhook only stores
  ref-linked rows, so a window with no reconciler tail is flagged forwardOnly so
  a consumer can't mistake a vacuous 100% for measured coverage. (Reconciler
  repo-discovery/scheduling remains the tracked follow-up.)
- #2: doc note that rollup cost is intentionally NOT window-clipped (full issue
  cost vs window-bounded LOC).
- #3: extracted applyIssueIdentifierToBranchName + a unit test asserting the
  enforced branch is extractor-matchable (locks option A against a future
  lowercasing sanitizeBranchName).
- #4: dropped the unused resolvePrLinks/ResolvedPrLink dead code.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* ci: trigger PR checks (bot-opened PR does not auto-run)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: kkroo <kkroo@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kkroo pushed a commit that referenced this pull request Jun 10, 2026
…ict unwrap (BLO-6328)

Follow-up to BLO-6291. Hardens the productivity-review reconciler against
the four classes of races Ally flagged on PR #122 and the QA cleanup from
BLO-6356, scoped to the paperclipai master codepath:

- Re-read the candidate row immediately before create/update so a source
  that reached `done`/`cancelled`, was hidden, or was reassigned to a user
  between candidate selection and write is skipped instead of getting a
  new wrapper or a stale blocker (Important #4 equivalent).
- Extract the active-review unique-conflict detector into
  `isActiveProductivityReviewUniqueConflict()` which walks the `cause`
  chain to unwrap Drizzle/Postgres errors, fixing the existing recovery
  path against the `issues_active_productivity_review_uq` constraint.
- Wire a `beforeCreateOrUpdateReview` test hook on the service deps so
  the candidate-failure-isolation and concurrent-dedupe regression tests
  can exercise the race window deterministically.
- Map `issue_productivity_review` recovery wrappers into the blocked-inbox
  attention model so open productivity-review escalations surface as
  `recovery_open` instead of falling through to `missing_disposition`.

Regression coverage:
- No-current-trigger suppression (history-only candidates skipped after
  the post-evidence recheck).
- Candidate-failure try/catch isolation.
- Concurrent dedupe via the recheck hook.
- Terminal-status race (source flips to done between selection and write).
- Blocked-inbox `issue_productivity_review` recovery mapping.

Verification:
  pnpm vitest run \
    server/src/__tests__/productivity-review-service.test.ts \
    server/src/__tests__/issue-blocker-attention.test.ts
  -> Test Files  2 passed (2), Tests  33 passed (33)

Refs BLO-6291, BLO-6328, BLO-6356, BLO-6364.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kkroo added a commit that referenced this pull request Jun 10, 2026
… (BLO-8459) (#320)

* fix(productivity-review): post-evidence source recheck + unique-conflict unwrap (BLO-6328)

Follow-up to BLO-6291. Hardens the productivity-review reconciler against
the four classes of races Ally flagged on PR #122 and the QA cleanup from
BLO-6356, scoped to the paperclipai master codepath:

- Re-read the candidate row immediately before create/update so a source
  that reached `done`/`cancelled`, was hidden, or was reassigned to a user
  between candidate selection and write is skipped instead of getting a
  new wrapper or a stale blocker (Important #4 equivalent).
- Extract the active-review unique-conflict detector into
  `isActiveProductivityReviewUniqueConflict()` which walks the `cause`
  chain to unwrap Drizzle/Postgres errors, fixing the existing recovery
  path against the `issues_active_productivity_review_uq` constraint.
- Wire a `beforeCreateOrUpdateReview` test hook on the service deps so
  the candidate-failure-isolation and concurrent-dedupe regression tests
  can exercise the race window deterministically.
- Map `issue_productivity_review` recovery wrappers into the blocked-inbox
  attention model so open productivity-review escalations surface as
  `recovery_open` instead of falling through to `missing_disposition`.

Regression coverage:
- No-current-trigger suppression (history-only candidates skipped after
  the post-evidence recheck).
- Candidate-failure try/catch isolation.
- Concurrent dedupe via the recheck hook.
- Terminal-status race (source flips to done between selection and write).
- Blocked-inbox `issue_productivity_review` recovery mapping.

Verification:
  pnpm vitest run \
    server/src/__tests__/productivity-review-service.test.ts \
    server/src/__tests__/issue-blocker-attention.test.ts
  -> Test Files  2 passed (2), Tests  33 passed (33)

Refs BLO-6291, BLO-6328, BLO-6356, BLO-6364.

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

* fix(productivity-review): audit-only suppression for terminal-source no_comment_streak (BLO-6243)

A source issue that has reached a terminal status (done/cancelled) by the time
the post-evidence recheck runs is a post-terminal sweep artifact, not a
work-stoppage signal. Instead of folding it into a generic `skipped`, record an
attributable `issue.productivity_review_suppressed` audit decision on the source
issue and bump a distinct `suppressedTerminalSource` counter. No review issue is
created and no wake comment is enqueued.

Builds on the BLO-6328 post-evidence recheck (`evaluateSourceReviewability`).
Tests cover done + cancelled suppression (audit-only, zero reviews) and an
in_progress control that still emits, matching the BLO-6243 verifying signal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(agent-health): evaluator for slot-holder false-starvation (BLO-8459)

Pure evaluateAgentRunHealth() function that detects:
- False-starvation: running/succeeded run exists even if outside the
  createdAt-desc window top-N (the BLO-8456 failure mode)
- Slot-held signal: running run exceeds threshold while queue backs up

Unit tests cover Fixture A (not-starved with old slot-holder), Fixture B
(slot-held signal), genuine starvation, and threshold/min-queue guards.

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

* ci: trigger PR checks (BLO-8459)

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

* chore: trailing newline (CI retrigger)

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

* fix(agent-health): clean up rebase verification issues

* fix(agent-health): require current productivity evidence

---------

Co-authored-by: kkroo <60861014+kkroo@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: kkroo <kkroo@users.noreply.github.com>
Co-authored-by: Paperclip <noreply@paperclip.ing>
Co-authored-by: Omar Ramadan <omar@blockcast.net>
kkroo pushed a commit that referenced this pull request Aug 2, 2026
BLO-18760 review follow-up. `resolveWorkspaceForRun` returns a saved session
cwd before the isolation-aware fallback selection, screened only by
`isUnsafeSessionWorkspaceCwd` — which rejects system temp roots and knows
nothing about isolation mode. The session cwd is persisted per
(agent, adapter, task) and replayed on every resume, while isolation is decided
per run, so a cwd chosen under one mode was inherited by a run in the other.

Both directions broke:

  shared -> run: the persistent agent home (carrying a real `.git` from
  unrelated prior runs) returned as `task_session`, the repo-less selection
  never ran, and the BLO-18147 dispatch guard parked the run — re-opening, via
  a resumed session, the exact strand this PR closes.

  run -> shared: a shared run adopted `empty-workspaces/<agent>` as its live
  cwd and could write a `.git` into it. That directory's repo-less-ness is an
  invariant this PR introduces and is load-bearing for every later
  run-isolated launch. The downstream session/workspace mismatch check fires
  only after `executionWorkspace` is realized, so it could not prevent the
  inheritance, and the breakage surfaced on a later, different run.

Adds `isWorkspaceLessFallbackCwdForOtherIsolationMode` and screens the early
return with it, closing both directions at the same decision point. The
predicate is deliberately narrow: it matches only the two workspace-less
fallback dirs, so project workspaces, per-run worktrees and every other
resumable cwd keep resuming unchanged (AC #3). The fallback warning now names
the real cause rather than the misleading "is not available" (AC #4).

Tests: server/src/__tests__/heartbeat-workspace-session.test.ts, describe
`isWorkspaceLessFallbackCwdForOtherIsolationMode` — 6 cases covering both
mismatch directions, the matching-mode allow path, other-cwd non-regression,
non-cloning adapters, and path normalization. Verified to fail pre-fix (4 of 6
fail when the predicate is neutered to its previous isolation-blind behavior;
the 2 that still pass are the non-regression guards). 199 passed, tsc clean.

Co-Authored-By: Claude <noreply@anthropic.com>
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.

1 participant