feat(linear): v1.1 polish — pre-container feedback, state-on-start, sweep, nits

[isadeks]

Summary

Wraps the v1.1 polish theme for the Linear integration, addressing the silent-drop gaps and PR #63 review recommendations in a single PR. Six rejection paths now produce user-visible feedback; the issue state transitions on agent-start (not just at PR-open); a stale-reaction sweep keeps re-runs clean; and four small review nits are addressed.

What changed

1. Pre-container failure feedback (5 paths in processor + 1 in orchestrator)

Linear-triggered tasks that fail before the agent container starts now produce a Linear comment + ❌ reaction within ~20s of label-apply. Previously, six distinct rejection points all silently dropped:

#	Trigger	Layer	Message
1	Issue has no projectId	processor	"isn't in a project — ABCA needs a Linear project to route tasks to a repo"
2	Project not onboarded / removed	processor	"isn't onboarded; admin can run bgagent linear onboard-project"
3	Webhook missing organization or actor	processor	diagnostic ("report to your ABCA admin")
4	Linear actor has no linked platform user	processor	"v1 only the API-token owner can submit; multi-user OAuth on v3 roadmap"
5	createTaskCore returns non-201	processor	branched on status: guardrail/validation surfaces error string; 503 prompts retry; other 4xx/5xx generic
6	User concurrency cap rejection	orchestrator	"concurrency limit; wait for one to finish, then re-apply the label"

All six call sites use a shared helper cdk/src/handlers/shared/linear-feedback.ts — reportIssueFailure(secretArn, issueId, message) posts the comment and reaction in parallel via Promise.allSettled. Reuses the existing 5-minute getLinearSecret cache.

2. State transition at agent-start

Mirrors the existing In Review transition that fires at PR-open. The prompt addendum's step 1 now instructs the agent to transition Backlog/Todo → In Progress before doing repo work, so Linear's state column reflects "being worked" during the minutes between label-apply and PR-open. Falls back to Todo if In Progress doesn't exist; skips if the issue is already in In Progress or any later state. Doesn't loop on list_issue_statuses.

3. Stale-reaction sweep

Re-runs (label removed and re-applied; or pre-container ❌ followed by a successful retry) used to leave prior 👀/✅/❌ accumulated next to the new run's reaction. Added _sweep_stale_reactions(issue_id) to agent/src/linear_reactions.py:

• Architecture: 👀-first, sweep-async — the user-visible 👀 fires first (~150ms typical). The sweep runs on a daemon thread after the 👀 lands, so cold-network latency (Linear's first call from a fresh container can be ~5s) doesn't gate the user-visible signal or block the agent pipeline.
• Filters by viewer + emoji — only deletes the bot's own 👀/✅/❌, never touches reactions a human added even if they happen to use the same emoji.
• Excludes the just-posted 👀 from its own sweep (passes exclude_id=rid).
• Best-effort — failure leaves the visual-duplication bug we set out to fix, but the pipeline proceeds unaffected.

Smoke-tested on backgroundagent-dev: react_task_started total = 156ms; sweep done in background at +303ms; agent started at +3ms after react_task_started exit.

4. PR #63 review nits (4)

• Auth circuit breaker in linear_reactions.py — track consecutive 401/403s; after 3 strikes, ERROR once and short-circuit until container restart. Replaces the prior behaviour where revoked tokens flooded CloudWatch with WARNs forever.
• Explicit linear_eyes_reaction_id: str | None = None init in pipeline.py instead of locals().get(...) in the crash handler.
• Narrow except Exception in resolve_linear_api_token to (BotoCoreError, ClientError). Switch print() → shell.log("WARN", ...).
• Admin-assisted fallback docs for bgagent linear setup auto-link failures. Replaces misleading "run bgagent linear link <code>" suggestion (the command is non-functional in v1) with explicit DynamoDB put-item steps and a clear v3-OAuth note.

Plumbing

• linear-integration.ts — wires LINEAR_API_TOKEN_SECRET_ARN into the webhook processor + grants read on LinearIntegration.apiTokenSecret.
• agent.ts — same wiring for orchestrator.fn (declared after LinearIntegration so done in the stack rather than as a prop).
• Both grants verified at synth: LinearIntegrationWebhookProcessorFn and TaskOrchestratorOrchestratorFn both carry the env var and IAM policy.

Test plan

Unit tests — 1240 baseline → 1268 CDK, 526 → 532 agent, 196 CLI, all green:

• cdk/test/handlers/shared/linear-feedback.test.ts (new, 13 tests) — mutation shape, auth header, error swallowing in 4 distinct failure modes, Promise.allSettled partial-success semantics.
• cdk/test/handlers/linear-webhook-processor.test.ts — extended with a user-visible feedback describe block, 10 new tests: one assertion per rejection path + happy-path-doesn't-fire + filter-rejection-doesn't-fire (latter is intentional UX).
• cdk/test/handlers/orchestrate-task-feedback.test.ts (new, 5 tests) — covers notifyLinearOnConcurrencyCap with withDurableExecution mocked.
• agent/tests/test_linear_reactions.py — extended with 5 sweep tests covering scoping rules (viewer-owned bgagent emojis only; preserves human reactions even with same emoji; preserves bot reactions of other emojis; sweep failure doesn't block 👀; viewer-id cached across calls).

Lint + synth + agent quality: clean.

Smoke tested on backgroundagent-dev:

• Path 2 (project not onboarded) ✅
• Path 5a (guardrail block) ✅ — ❌ + comment within ~20s of label
• 👀-fast architecture ✅ — eyes-visible at +156ms confirmed via runtime container logs

Smoke tests not run for paths 3 (defensive, no natural reproducer) and 5b (503, no natural reproducer) — covered by unit tests. Path 6 (concurrency cap) requires 4 simultaneous tasks to reproduce; covered by unit tests.

Reviewer notes

• No linear_*_msg_ts fields on TaskRecord — all per-issue state stays in Linear, accessed at runtime.
• No new stream consumers — reactions/comments use direct GraphQL from agent + Lambda; doesn't touch the FanOutConsumer / TaskEventsTable 2-reader-per-shard limit (closed by feat(fanout): migrate SlackNotifyFn to FanOutConsumer subscriber (#64) #79 anyway).
• Sweep is daemon-threaded — never blocks the agent pipeline. If the container terminates before the sweep finishes, the worst case is the visual-duplication bug we set out to fix (status quo from before this PR).
• Auth circuit breaker is per-container — resets on container restart, doesn't persist to DynamoDB. Right tradeoff for a 5-15 minute task; revisit if container lifetimes grow much longer.

[krokoko]

Code Review Summary

Great PR — the architecture is sound, test coverage is thorough (28 new tests), and the "best-effort, never gate the pipeline" philosophy is consistently applied. A few items to address before merge:

---

Must fix

1. Thread-safety of circuit breaker globals (agent/src/linear_reactions.py)

_consecutive_auth_failures and _auth_circuit_open are read/written from both the main thread and the daemon sweep thread. The +=1 increment and the check-then-set pattern are not atomic. Worst case: duplicate "circuit OPEN" ERROR logs or lost counter resets.

→ Add a threading.Lock around the three module-level globals.

2. notifyLinearOnConcurrencyCap inside a retriable durable execution step (cdk/src/handlers/orchestrate-task.ts)

If notifyLinearOnConcurrencyCap throws unexpectedly, the durable execution framework retries the entire admission-control step — re-running failTask and emitTaskEvent (duplicate events).

→ Wrap in a defensive try-catch:

try {
  await notifyLinearOnConcurrencyCap(task);
} catch (feedbackErr) {
  logger.warn('Linear concurrency-cap feedback failed (non-fatal)', { error: ... });
}

---

Should fix

3. ImportError regression in resolve_linear_api_token (agent/src/config.py)

import boto3 is inside the try block but the narrowed except (BotoCoreError, ClientError) no longer catches ImportError. If boto3 is missing from the container image, the agent hard-crashes instead of gracefully degrading.

→ Add ImportError to the except tuple, or move the imports before the try.

4. Daemon thread crash silence (agent/src/linear_reactions.py)

If _sweep_stale_reactions hits an unexpected TypeError/AttributeError, the thread silently dies with no application-level log (stderr may not reach CloudWatch in containerized environments).

→ Wrap the thread target in a top-level try-except that logs via log("ERROR", ...).

5. Sweep delete counter inaccuracy (agent/src/linear_reactions.py)

_graphql(_DELETE_MUTATION, {"id": rid}) return value is discarded but deletes += 1 always fires. The summary log gives a false impression of success when deletes actually failed.

→ if _graphql(...) is not None: deletes += 1

---

Nice to have (non-blocking)

• Tighten buildCreateTaskFailureMessage params — the sole callsite passes APIGatewayProxyResult fields which are number and string, never undefined. The | undefined arms are dead code.
• Add a log when circuit breaker short-circuits — once the circuit opens, all subsequent calls return None with zero trace. A single "circuit still open" DEBUG log aids debugging.
• Log AccessDeniedException at ERROR in resolveToken — IAM misconfig is persistent, not transient. WARN severity means no alert fires for a broken deployment.
• Person name in comment (config.py line 70: "per Alain's feat(linear): add Linear integration, with Slack review fixes #63 review") — prefer referencing only the issue number.

---

Overall: solid work. The two "must fix" items are small changes that prevent real (if unlikely) failure modes. Happy to re-review once addressed.

[isadeks]

Thanks for the thorough review @krokoko. All 9 items addressed across two commits — split must/should-fix from nice-to-have so the load-bearing changes are isolated.

Must-fix (dd5474eb)

1. Thread-safety of circuit breaker globals — added _auth_state_lock around the read-modify-write on _consecutive_auth_failures / _auth_circuit_open. The 401/403 increment-and-check is now atomic; the open-circuit check reads the flag under the lock then drops the lock before logging.
2. notifyLinearOnConcurrencyCap defensive try/catch — wrapped in orchestrate-task.ts. New unit test (reportIssueFailure rejection propagates (caller must catch)) asserts the helper does throw on its caller, so the surrounding catch in admission-control is load-bearing rather than redundant.
3. ImportError regression — split import boto3 into its own try/except ImportError block ahead of the narrowed (BotoCoreError, ClientError) catch. New test_import_error_degrades_gracefully covers it.
4. Daemon thread crash silence — added _sweep_stale_reactions_safe wrapper that catches Exception at the thread top-level and logs at ERROR. Thread name unchanged so existing _join_sweep_thread() test helper still finds it.
5. Sweep delete counter inaccuracy — deletes += 1 now gated on _graphql(_DELETE_MUTATION, ...) is not None.

Also expanded the autouse fixture in test_linear_reactions.py to reset the circuit-breaker globals between tests so future cases don't accumulate state.

Nice-to-have (420fc113)

• DEBUG log on circuit short-circuit — linear_reactions: auth circuit still open; short-circuiting call.
• AccessDeniedException → ERROR — split the catch into ClientError (inspect Error.Code, escalate AccessDeniedException to ERROR) and BotoCoreError (WARN). Two new tests cover both branches.
• Tightened buildCreateTaskFailureMessage params — number, string (not | undefined) since the only caller passes APIGatewayProxyResult fields. Removed dead ?? 'unknown' fallbacks.
• Person name scrub — Alain's #63 review → #63 review in config.py.

Verification

• Agent: 537 tests pass (was 532 — 5 new in test_config.py)
• CDK: focused suites (linear-webhook-processor, orchestrate-task-feedback, linear-feedback, orchestrate-task, create-task-core) all green; 1 new test in orchestrate-task-feedback.test.ts
• Compile + ESLint + ruff format/check clean
• Smoke-tested on backgroundagent-dev

Ready for re-review.

[krokoko]

Re-review of PR #87

First review items: all addressed ✓

All 9 items from the initial review (5 must/should-fix + 4 nice-to-have) are correctly addressed in dd5474e and 420fc11:

• Thread-safety lock on circuit breaker globals — correctly implemented with _auth_state_lock
• Defensive try/catch around notifyLinearOnConcurrencyCap — in place, with a test proving the catch is load-bearing
• ImportError regression — hoisted into its own try/except before the narrowed (BotoCoreError, ClientError) catch
• Daemon thread crash wrapper — _sweep_stale_reactions_safe catches Exception and logs at ERROR
• Sweep delete counter — gated on _graphql(...) is not None
• All 4 nice-to-haves (DEBUG log, AccessDenied → ERROR, param tightening, name scrub) — done

---

New findings

Should fix

1. Missing defensive try/catch around reportIssueFailure in the webhook processor (linear-webhook-processor.ts)

The orchestrator correctly wraps notifyLinearOnConcurrencyCap in try/catch as belt-and-suspenders. The 5 bare await reportIssueFailure(...) calls in the webhook processor have no such defense. While Promise.allSettled structurally guarantees the function cannot reject today, a synchronous TypeError before the Promise.allSettled is constructed (e.g. if the function signature is refactored) would propagate, causing Lambda failure and SQS retries.

Suggestion: extract a local wrapper at the top of the file and call it at all 5 sites:

async function safeReportIssueFailure(issueId: string, message: string): Promise<void> {
  try {
    await reportIssueFailure(API_TOKEN_SECRET_ARN, issueId, message);
  } catch (err) {
    logger.warn('Linear feedback failed (non-fatal)', {
      issue_id: issueId,
      error: err instanceof Error ? err.message : String(err),
    });
  }
}

2. process.env.LINEAR_API_TOKEN_SECRET_ARN! non-null assertion without runtime guard (linear-webhook-processor.ts:30)

The orchestrator path correctly checks if (!secretArn) and logs a warning before returning. The processor uses a ! assertion at module scope — if the env var is missing due to a deploy misconfiguration, every feedback call silently fails with a cryptic WARN from resolveToken ("could not resolve API token") instead of a clear one-time diagnostic.

Suggestion: match the orchestrator pattern — drop the !, add a guard at usage or a startup-time log.

3. Auth circuit breaker has zero tests (test_linear_reactions.py)

The circuit breaker is ~40 lines of thread-safe logic (_AUTH_FAILURE_THRESHOLD, _consecutive_auth_failures, _auth_circuit_open, _auth_state_lock) with no test coverage. Missing scenarios:

• 3 consecutive 401/403s open the circuit
• Once open, calls short-circuit without hitting the network
• A 2xx between failures resets the counter
• Non-auth errors (500) don't reset the counter

The _reset_module_state fixture already handles cleanup, so adding a TestAuthCircuitBreaker class should be straightforward.

Nice to have

4. BotoCoreError handler path untested (test_config.py) — The PR narrowed except Exception to two separate handlers but only ClientError paths have tests. One test for BotoCoreError (e.g. EndpointConnectionError) would close this.

5. exclude_id filter in sweep is never exercised (test_linear_reactions.py) — In test_sweep_deletes_only_viewer_owned_bgagent_emojis, prior reaction IDs never collide with the new reaction ID, so the exclude_id branch is dead code in tests. Add a prior reaction whose ID matches the newly posted one and verify it's preserved.

Out of scope (pre-existing, noting for tracking)

6. resp.json() in linear_reactions.py::_graphql can throw JSONDecodeError — The body = resp.json() if resp.content else {} line is outside any try/except. If Linear returns a 200 with non-JSON body (HTML maintenance page), it raises ValueError. The new sweep code increases the probability of hitting this (3+ _graphql calls per react_task_started). The _sweep_stale_reactions_safe wrapper catches it for the daemon thread, but the main-thread path would propagate. Worth a separate fix.

---

Summary table

#	Finding	Severity
1	Bare await reportIssueFailure(...) — 5 sites without try/catch	Should fix
2	! non-null assertion without runtime guard	Should fix
3	Auth circuit breaker has zero tests	Should fix
4	BotoCoreError path untested	Nice to have
5	exclude_id filter untested	Nice to have
6	Pre-existing resp.json() crash risk	Out of scope

Items 1–3 are worth addressing before merge. 4–5 would improve confidence but aren't blocking. 6 should be tracked separately.

Positive notes

• Promise.allSettled in reportIssueFailure is the right construct — structurally guarantees the never-throw contract
• Thread safety is correctly implemented — single lock, no nested acquisitions, no deadlock risk
• Test quality is strong: _join_sweep_thread() for daemon thread sync, comprehensive positive and negative assertions on feedback paths, and the test proving reportIssueFailure rejection propagates (validating the orchestrator's catch is load-bearing) is particularly thoughtful
• The "best-effort, never gate the pipeline" philosophy is consistently applied across all layers
• CDK wiring (IAM grants + env vars) is correct in both linear-integration.ts and agent.ts
• Docs sync is correct

[isadeks]

Thanks for the second pass @krokoko. All 5 actionable findings addressed across two commits, with the out-of-scope item parked for a follow-up. Ty narrowing pre-existing in f4633be caught by CI and fixed in 0a3711c.

Should-fix (f4633be)

1. Bare await reportIssueFailure(...) at 5 call sites in linear-webhook-processor.ts

Extracted a local safeReportIssueFailure(issueId, message) helper at the top of the file and routed all 5 sites through it. The helper wraps the call in try/catch so a synchronous throw (e.g. from a future signature refactor that breaks the helper's Promise.allSettled never-throw contract) can't propagate, fail the Lambda, and trigger SQS retries on a poison message. Two new tests assert the helper swallows both synchronous throws and async rejections.

2. process.env.LINEAR_API_TOKEN_SECRET_ARN! non-null assertion

Dropped the !. The new safeReportIssueFailure guards on !API_TOKEN_SECRET_ARN and logs a single clear Skipping Linear feedback: LINEAR_API_TOKEN_SECRET_ARN not set diagnostic per skip — matches the orchestrator pattern in notifyLinearOnConcurrencyCap. No more cryptic per-call WARN from resolveToken on deploy misconfig.

3. Auth circuit breaker has zero tests

New TestAuthCircuitBreaker class in test_linear_reactions.py with 5 tests covering all the scenarios you flagged:

• test_three_consecutive_401s_open_the_circuit — threshold semantics
• test_open_circuit_short_circuits_subsequent_calls — once open, no network
• test_2xx_response_resets_failure_counter — 200 between 401s clears the counter
• test_non_auth_errors_do_not_increment_failure_counter — 500 doesn't count
• test_403_treated_same_as_401 — both auth statuses

Nice-to-have (a713c62)

4. BotoCoreError handler path untested

New test_botocore_error_logged_at_warn in test_config.py exercising the BotoCoreError branch with EndpointConnectionError. Asserts WARN severity and exception type in the log message.

5. exclude_id filter never exercised

New test_sweep_preserves_just_posted_eyes_via_exclude_id plants the just-posted reaction id in the prior reactions list and asserts the sweep skips it while still deleting an older ❌. Previously dead branch in tests; now load-bearing.

Out of scope (0a3711c follow-up)

6. Pre-existing resp.json() crash risk on non-JSON 200

Tracked in things-to-do.md for a separate fix — the daemon-thread sweep is already wrapped by _sweep_stale_reactions_safe (added in dd5474e), but the main-thread react_task_started / react_task_finished paths would still propagate a JSONDecodeError. Worth fixing on its own so the diff is reviewable.

CI fix (0a3711c)

The first push of the must-fix commit had a ty check failure I missed locally — the module-level globals _consecutive_auth_failures = 0 / _auth_circuit_open = False were narrowed by ty to Literal[0] / Literal[False], which then rejected the legitimate runtime flips and the test fixture's resets. Fixed with explicit int / bool annotations.

Verification

• Agent: mise //agent:quality clean — lint, format, ty check, 544 tests pass (was 537 — 7 new across test_linear_reactions.py and test_config.py)
• CDK: mise //cdk:test clean — 1273 tests pass (was 1240 baseline — 2 new in linear-webhook-processor.test.ts for safeReportIssueFailure's sync + async failure handling)
• mise //cdk:synth:quiet clean (only pre-existing CdkNag warnings on VPC endpoint security groups)

Ready for re-review.