[danii1]

Why

Task local-test-e2e-cover-worker-harness-failover-o: test(e2e): cover worker harness failover on a usage limit

The worker's harness failover path (AGENT_HARNESS=claude-code,codex) had no end-to-end coverage: only argv acceptance and fake-agent modes were tested, so a regression in usage-limit detection, exit 75 handling, retry-on-fallback, or persisted failover state would go unnoticed. The new test drives the real built devintern worker daemon through that state machine offline.

What changed

• packages/e2e/tests/local/worker-harness-failover.test.ts (new): two scenarios using WorkerFixture.
  • Failover: primary stub (CLAUDE_CODE_CLI_PATH) emits Claude usage limit reached + adjacent resets in 1 hour and exits 1; fallback stub (CODEX_CLI_PATH) execs helpers/fake-agent.ts with the original argv. Asserts the row settles done, run statuses are ["deferred", "succeeded"], the primary spawned exactly once, the fallback produced the canonical edit, the pushed branch contains export const greeting = "Hello, world!"; and diff-vs-base is only src/greeting.ts, worker logs name claude-code → codex (no generic failure / kept-worktree message), and webhook_meta holds a future rate_limit:claude-code with failover:active_harness=codex.
  • All-limited: both stubs limit; asserts the row is requeued pending, two deferred runs, both future windows persisted, no branch ref on the bare remote, and the daemon stays alive.
• packages/e2e/helpers/worker.ts: added rateLimits() and activeHarness() read helpers over the existing query() read-only pattern (with key constants), so tests do not duplicate webhook_meta SQL.
• packages/e2e/README.md: new “Worker harness failover (usage limit)” section covering the stub strategy, the AGENT_CLI_PATH precedence trap, assertions, the attempt-suffix detail, and the default command.
• packages/e2e/docs/coverage-matrix.md: both requested rows moved from gap to default with the exact test file; backlog item 14 struck through as implemented.

Scope

Included: the new local test, the two queue read helpers, README and coverage-matrix updates. No production code, detection patterns, failover policy, or unrelated helpers were changed. Nothing was committed, pushed, labelled, or opened as a PR.

Tradeoffs

• The task text expected the pushed branch to be feature/<task_key>. In reality a usage limit is detected after prepare-branch has already created the unsuffixed ref, so the failover retry is a fresh attempt and findAvailableFeatureBranchName produces feature/<task_key>-attempt-2. The test asserts the recorded/pushed branch explicitly and the README documents why; the actual primary failure was observed before choosing that assertion.
• The fixture's global AGENT_CLI_PATH is cleared in the test. Env-driven resolution prefers it over every <HARNESS>_CLI_PATH, so leaving it set would send the codex retry back to the primary stub and the fallback would never run.
• Failback after the window elapses and restart-time restore of persisted failover state remain untested; they are named in the matrix’s “Missing” column. Both covered scenarios keep one-hour windows so no resume timer fires mid-test.

Blast Radius

• WorkerFixture consumers: the new methods only add API; no existing method or default changed, so all current worker tests are unaffected.
• Coverage docs/backlog: rows now claim default, which changes what the e2e scout considers already covered.
• No runtime/CLI behavior changed; packages/code/dist was rebuilt only to run the suite.

Verification

Ran from the repository root, in order:

• bun run --filter @getdevintern/code build — passed.
• bun run --filter @devintern/e2e test — 135 pass, 16 skip, 4 todo, 0 fail (155 tests / 24 files, ~50s), including both new scenarios.
• bun run --filter @devintern/e2e typecheck — passed.
• bun run --filter @devintern/e2e lint — passed.
• bun run --filter @devintern/e2e format:check — passed.
  Also ran the new file alone twice: 2 pass, 0 fail. The first draft failed the literal feature/<task_key> assertion with Received: "feature/local-expand-the-greeting-across-harnesses-attempt-2", which established the suffix behavior corrected above.

---

This PR was automatically created by @devintern/code

[danii1]

Artifacts & verification

Run: 38 · Commit: a669967b06a3cb70be8fa546416c024a94f34b4c

Change explanation

Produced by pr-explainer-narrative, execution 1 at a669967b06a3cb70be8fa546416c024a94f34b4c.

No PR template was found in this repository (.github/ has no PULL_REQUEST_TEMPLATE, and there is no pull_request_template or PULL_REQUEST_TEMPLATE.md at the root), so the structure below follows the requested sections.

test(e2e): cover worker harness failover on a usage limit

Why this change is needed

The worker supports a priority chain such as AGENT_HARNESS=claude-code,codex. When the primary harness hits an account-wide usage limit, the worker must detect the provider message, persist the reset window, retry the same task on the next harness, and later fail back. This is a user-facing reliability path, and it depends on external agent CLIs whose limit messages and exit behavior drift.

Before this change, e2e coverage only exercised installed-harness argv and the deterministic fake agent. Nothing drove the real worker's failover state machine end to end.

(Motivation above is taken from the task section. The diff alone does not state it.)

What changed

New end-to-end failover test. packages/e2e/tests/local/worker-harness-failover.test.ts drives the real built devintern worker daemon with a claude-code,codex chain. Both harness CLIs are local shell stubs written under the fixture's temporary root:

• The primary stub prints a recognized limit pair (Claude usage limit reached / resets in 1 hour) and exits non-zero. It never edits the repository.
• The fallback stub execs the deterministic helpers/fake-agent.ts with the original argv, so it makes the canonical implementation edit.

The happy-path scenario asserts: the row settles done; run records are deferred then succeeded; exactly one agent invocation reached the fake agent; the primary stub ran exactly once (no retry after the limit); the pushed branch contains export const greeting = "Hello, world!"; and differs from base only in src/greeting.ts; logs name the claude-code → codex failover and contain no generic-failure text; and webhook_meta persists a future primary limit window with failover:active_harness=codex, while the worker stays alive.

A second, deterministic scenario makes both stubs report a limit. It asserts the row is requeued pending with two deferred runs, both windows are persisted, no branch is pushed, the active harness is codex, and the daemon stays alive.

Read-only queue inspection. packages/e2e/helpers/worker.ts gains rateLimits() and activeHarness(), which read webhook_meta through the fixture's existing read-only query pattern instead of duplicating SQLite schema knowledge in the test.

Documentation. packages/e2e/README.md describes the failover suite, the stub strategy, why the fixture's global AGENT_CLI_PATH is cleared (env-driven resolution prefers it over <HARNESS>_CLI_PATH, which would send the retry back to the primary), and the -attempt-2 branch naming. packages/e2e/docs/coverage-matrix.md moves both requested rows from gap to default with the new test as evidence, and lists the still-untested branches: real harness limit-text drift, failback after the reset window, and restart-time restore of the persisted chain.

No production failover, detection, or worker behavior was changed. The diff touches only packages/e2e.

Validation actually performed

Observed in the pipeline evidence (verify step):

• The full acceptance sequence was run and reported passing: bun run --filter @getdevintern/code build, bun run --filter @devintern/e2e test, bun run --filter @devintern/e2e typecheck, bun run --filter @devintern/e2e lint, bun run --filter @devintern/e2e format:check.
• Full local e2e suite: 135 pass, 16 skip, 4 todo, 0 fail.
• Auto-review independently reports bun test tests/local → 135 pass, 0 fail.
• The new test was run 3 times and passed on each run, at roughly 2s per run (no observed flake).

Not performed / not observed:

• No real agent CLI, model call, account, or network path was exercised, by design.
• No validation of failback after a reset window or of restart-time restore of the persisted chain.
• No manual or live-environment validation is recorded.

Material risks

• Test robustness, not product risk. The second scenario's 120_000 ms budget is below the summed worst case of its bounded waits (about 150s including readiness, output waits, limit/requeue polls, and dispose). On a slow or partially broken run, Bun could report a test timeout instead of the helper's diagnostic. No timeout was observed in three runs.
• Coverage wording may overstate scope. The matrix rows are now default, while failback after the reset window and restart-time restore remain untested; they are recorded in the Missing column, but readers may not notice.
• Harness-resolution coupling. The test clears AGENT_CLI_PATH because that global override wins over <HARNESS>_CLI_PATH. If fixture defaults or resolution order change, the test could stop exercising the fallback.
• Branch-name expectation. The test asserts feature/<task_key>-attempt-2 because the limit lands after prepare-branch created the unsuffixed ref. The task text asked for feature/<task_key>; the implementation documents the suffix as the fresh-attempt naming. If that naming changes, the test fails.
• Queue query precision. rateLimits() interpolates the rate_limit: prefix into a LIKE pattern; SQLite's _ is a single-character wildcard, so unrelated keys could match. Only the new test consumes it.

Where reviewers should focus

1. packages/e2e/tests/local/worker-harness-failover.test.ts — assertion strength (exactly one succeeded run, exactly one primary spawn, no generic-failure log line, persisted window/active harness) and the -attempt-2 branch expectation.
2. packages/e2e/helpers/worker.ts (rateLimits, activeHarness) — read-only access, prefix matching precision, and whether a table-driven or exact-match query would be safer.
3. packages/e2e/docs/coverage-matrix.md — whether default is the right status given the documented untested branches.
4. The second scenario's timeout and disposal margin.

Suggested checks (not performed): raise the second scenario timeout to match the first (180s) or derive it from helper deadlines; bind the LIKE pattern and use an underscore-safe matcher; confirm the branch-naming assertion is intentional.

Visual explanation

Produced by pr-explainer-visual, execution 1 at a669967b06a3cb70be8fa546416c024a94f34b4c.

Show artifact

Summary

Adds packages/e2e/tests/local/worker-harness-failover.test.ts, a local end-to-end test that drives the real built devintern worker daemon through harness failover: a two-entry AGENT_HARNESS=claude-code,codex chain whose primary stub reports a provider usage limit and whose fallback delegates to the deterministic fixture agent. This matters because failover on an account-wide usage limit is a user-facing reliability path that depends on external agent CLIs whose limit text and exit behavior drift, and until now only installed-harness argv and the fake agent were covered — no test exercised the worker's real failover state machine end to end. Supporting changes are two read-only WorkerFixture accessors for persisted webhook_meta state plus README and coverage-matrix updates. No production failover, detection, or worker behavior was modified.

Evidence

addTask → worker daemon
            │
            ├─ claude-code stub (CLAUDE_CODE_CLI_PATH)
            │    "Claude usage limit reached" / "resets in 1 hour" → exit 1
            │    (no repo edit)
            ▼
     persist rate_limit:claude-code (future) + failover:active_harness=codex
            ▼
     codex stub (CODEX_CLI_PATH) → exec helpers/fake-agent.ts "$@"
            ▼
     push feature/<key>-attempt-2 → row "done", runs [deferred, succeeded]

Asserted by the test, per the diff:

Claim	Where
Row settles done, exactly one succeeded run	worker-harness-failover.test.ts
Primary spawned once (no post-limit retry), fallback once	stub invocation logs
Branch contains export const greeting = "Hello, world!";, only src/greeting.ts differs	branchFile + git diff --name-only
Logs name claude-code → codex, no generic failure	worker.logs
Future rate_limit:claude-code window persists, active_harness=codex	new rateLimits() / activeHarness()
Second scenario: both limited → row pending, no branch pushed, daemon alive	same file

Validation, quoted from the pipeline verification step (observed, not re-run here): the required sequence passed — @getdevintern/code build, full local e2e suite with 135 pass, 16 skip, 4 todo, 0 fail, typecheck, lint, and format check. The auto-review step independently reported bun test tests/local → 135 pass, 0 fail.

Before/after execution results were not captured. The change introduces a test where the matrix previously recorded a gap, so there is no prior run to compare against.

Merge Danger

All four files are inside packages/e2e, a private, non-published workspace; no runtime, API, or shipped package behavior changed, so this can ship without a staged rollout. Reverting it removes coverage and the two new accessors only — nothing at runtime reads them, and the accessors are additive with a single consumer (the new test). The residual risk is test robustness rather than product regressions: auto-review flagged that rateLimits() filters with LIKE 'rate_limit:%', where SQLite treats _ as a wildcard (harmless today, one consumer, mirrors production webhook-queue.ts), and that the all-limited scenario's 120s budget sits below the ~150s combined worst case of its bounded waits and dispose().

Verification report

Produced by __verify-1, execution 1 at a669967b06a3cb70be8fa546416c024a94f34b4c.

Show artifact

{
  "summary": "Implementation satisfies the task's functional requirements. The new test drives the real built worker through a stubbed claude-code usage-limit failure and codex fallback, plus a deterministic all-harnesses-limited deferral scenario; the helper gained read-only webhook_meta accessors and the docs/matrix were updated. The required acceptance sequence passed: build, full local e2e suite (135 pass, 16 skip, 4 todo, 0 fail), typecheck, lint, and format:check. Blast radius is limited because no production failover, detection, or worker behavior changed: the only non-test code is two additive read-only WorkerFixture methods and the new test mutates only its own fixture environment and temporary root.",
  "items": [
    {
      "priority": "low",
      "confidence": "medium",
      "category": "testing",
      "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
      "line": "251",
      "issue": "The all-limited scenario's 120_000 ms test budget is below the combined worst-case of its bounded waits: 20s readiness + 20s + 20s output waits + 30s limits poll + 30s requeue poll + up to 30s dispose is about 150s. On a very slow or partially broken run Bun can report a test timeout before the helper emits its diagnostic.",
      "evidence": "pointed at real lines: test file:251; helper defaults at helpers/worker.ts:261 (20s readiness), :469 (20s output), :451 (30s waitFor), :487 (25s SIGTERM + 5s SIGKILL). Ran the test 3 times: 2 pass each run (~2s), so no observed flake.",
      "suggestion": "Raise the second scenario's timeout to 180_000 ms, matching the first scenario, or derive it from the sum of the helper deadlines."
    },
    {
      "priority": "info",
      "confidence": "high",
      "category": "documentation",
      "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
      "line": "172",
      "issue": "The task text asked to assert the pushed feature/<task_key> branch, while the test asserts feature/<task_key>-attempt-2. This reflects the real daemon behavior after the first attempt created the unsuffixed ref without pushing it, and the naming is documented rather than an incorrect assertion.",
      "evidence": "ran a check: the test passes asserting branch === `${expectedBranch}-attempt-2`; docs/code/worker.md:236 documents fresh retries as feature/{key}-attempt-2; packages/e2e/README.md:148 documents the same for this scenario.",
      "suggestion": "No change required; optionally also assert that the unsuffixed branch is absent from the bare remote if stricter fidelity to the task wording is desired."
    },
    {
      "priority": "info",
      "confidence": "high",
      "category": "code-quality",
      "file": "packages/e2e/helpers/worker.ts",
      "line": "410",
      "issue": "Blast-radius check: the helper change is purely additive and the new test is isolated, so existing WorkerFixture consumers are unaffected.",
      "evidence": "pointed at real lines: git show --stat HEAD lists only README.md, coverage-matrix.md, helpers/worker.ts, and the new test; helpers/worker.ts:410-438 adds rateLimits/activeHarness without changing existing methods; all 135 local e2e tests and typecheck/lint/format passed.",
      "suggestion": "No change; the safety fact is that no existing WorkerFixture method or production failover/worker code was modified."
    }
  ],
  "approved": true
}

Auto-review summary

Produced by auto-review, execution 1 at a669967b06a3cb70be8fa546416c024a94f34b4c.

Show artifact

{
  "iterations": 1,
  "success": true,
  "finalFeedback": {
    "summary": "This is a test-only change: it adds one local e2e test that drives the real built worker through harness failover, two read-only WorkerFixture accessors, and README/coverage-matrix updates. The fact that makes it safe is that the commit touches only packages/e2e (a private, non-published workspace) and both new accessors are additive with exactly one consumer, the new test — proven by `git show --stat` (4 e2e files only), `rg` (only worker-harness-failover.test.ts:192,224,195,231 call them), and the full local suite staying green (`bun test tests/local` -> 135 pass, 0 fail). Remaining findings are test-robustness and coupling issues, not behavior regressions.",
    "items": [
      {
        "priority": "low",
        "confidence": "high",
        "category": "code-quality",
        "file": "packages/e2e/helpers/worker.ts",
        "line": "410-424",
        "issue": "`rateLimits()` interpolates `RATE_LIMIT_KEY_PREFIX` into the SQL string (`key LIKE '${RATE_LIMIT_KEY_PREFIX}%'`) instead of binding it, and the pattern is not exact: SQLite treats `_` in LIKE as a single-character wildcard, so the prefix `rate_limit:` also matches keys such as `rateXlimit:...`. The only consumer is the new test (packages/e2e/tests/local/worker-harness-failover.test.ts:192,224), so no product behavior is affected, but it mirrors the same imprecise pattern in production (packages/code/src/lib/state/webhook-queue.ts:331) and the `.slice(RATE_LIMIT_KEY_PREFIX.length)` at line 420 would then produce a garbage harness name.",
        "evidence": "Ran a bun:sqlite probe: `SELECT key FROM webhook_meta WHERE key LIKE 'rate_limit:%'` returned both `rate_limit:codex` and `rateXlimit:claude`; production parity at packages/code/src/lib/state/webhook-queue.ts:331.",
        "suggestion": "Bind the pattern as a parameter (`WHERE key LIKE ?`, [`${RATE_LIMIT_KEY_PREFIX}%`]) and use an underscore-safe matcher such as `GLOB 'rate_limit:*'` or `substr(key, 1, ?) = ?` so the helper reads exactly the keys WebhookQueue writes."
      },
      {
        "priority": "low",
        "confidence": "low",
        "category": "performance",
        "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
        "line": "191-193",
        "issue": "The waitFor probe calls `worker.rateLimits()` twice per iteration on the truthy branch. Each call opens and closes a new read-only bun:sqlite connection (packages/e2e/helpers/worker.ts:528-541), so at the default 100ms interval this issues ~20 connections/second for up to 30s (worker.ts:451) just to return the same map that was already read.",
        "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:191-193 plus connection-per-call in packages/e2e/helpers/worker.ts:528-541.",
        "suggestion": "Read once into a local (`const windows = worker.rateLimits(); return (windows[\"claude-code\"] ?? 0) > Date.now() ? windows : undefined;`), matching the second test at lines 223-228."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
        "line": "250",
        "issue": "The second test's 120s budget is below the fixture's own worst case on the same machine: waitForReady 20s (helpers/worker.ts:261-262) + two waitForOutput calls at 20s each (469) + two waitFor calls at 30s each (451) + stop's 25s SIGTERM and 5s SIGKILL (487-494) is ~150s before prepare()/daemon work. The first test explicitly documents the opposite rule (lines 201-203: 180s must exceed the helper worst case), so a hung daemon in the second test trips Bun's timeout instead of the fixture's own diagnostic errors.",
        "evidence": "Timeout defaults at packages/e2e/helpers/worker.ts:262, 451, 469, 487-494; test budget at packages/e2e/tests/local/worker-harness-failover.test.ts:250.",
        "suggestion": "Give the second test the same 180_000 budget (or lower the nested waits) and mirror the worst-case comment from lines 201-203."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/helpers/worker.ts",
        "line": "72-76, 410-438",
        "issue": "The two new constants and the parsing logic duplicate private implementation details of WebhookQueue: the key format (packages/code/src/lib/state/webhook-queue.ts:280-282), the tolerant row parsing (329-350), and the active-harness key (352-355). If the key scheme is renamed or moved (both live in the same monorepo), the test does not fail with a clear assertion: `query()` swallows every error (worker.ts:535-537), so `rateLimits()` returns `{}` and the test burns the full 30s waitFor (worker.ts:451) and reports only a generic timeout.",
        "evidence": "Key scheme duplicated between packages/e2e/helpers/worker.ts:72-76,410-424 and packages/code/src/lib/state/webhook-queue.ts:280-282,331,354; error swallowing at packages/e2e/helpers/worker.ts:535-537.",
        "suggestion": "Export the meta-key helpers (or a `readFailoverState` accessor) from @getdevintern/code and use it here, or at minimum fail loudly for assertions that do not need polling (e.g. assert `activeHarness()` directly and surface the DB path in the message) so drift is diagnosed immediately."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
        "line": "238-246",
        "issue": "Unlike the first test's `diagnostics()` helper (lines 138-147), every bare expectation in the exhausted-chain test carries no context, so a failure of `runs.map(...)` (line 240), the invocation counts (241-242), `remoteBranches` (245), or `worker.running` (246) prints only `expected [...] received [...]` with no run rows, stub counts, or worker tail. These are exactly the assertions that diagnose the failover state machine, where the worker log is the primary evidence.",
        "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:238-246 vs the first test's diagnostics at 138-147.",
        "suggestion": "Build a `diagnostics()` closure for the second test too (status/runs, both invocation counts, remote refs, `worker.tail()`) and pass it as the `expect` message, as the first test does."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
        "line": "165-187",
        "issue": "The test pins behavior that is not the contract: the exact branch suffix `-attempt-2` (line 172, produced by packages/code/src/lib/utils/feature-branch.ts:387) and exact log prose (lines 184-187, produced by packages/code/src/lib/agent/harness-failover.ts:257 and the injected acquirer at packages/code/src/lib/workspace/injected-acquirer.ts:208). A legitimate product change (e.g. resuming the first attempt's branch instead of starting a fresh attempt, or rewording the failover log) fails this e2e even though failover still works, and the test already proves the real state via run records and the remote ref. The `not.toContain(\"keeping worktree for debugging\")` check is also a broad log-string guard unrelated to failover.",
        "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:172,184-187; branch naming at packages/code/src/lib/utils/feature-branch.ts:361-390; log text at packages/code/src/lib/agent/harness-failover.ts:253-259.",
        "suggestion": "Assert the stable observable instead of the wording where possible (succeeded run's branch exists on the remote, exactly one succeeded attempt, primary invoked once) and keep at most one loose log check such as /hit a usage limit/; if the branch suffix is intentional contract, note it in the coverage matrix so a future change is a deliberate decision."
      },
      {
        "priority": "info",
        "confidence": "medium",
        "category": "documentation",
        "file": "packages/e2e/docs/coverage-matrix.md",
        "line": "98",
        "issue": "The row is promoted from `gap` to `default` while neither the failback path nor restart-time restore is exercised, which the Missing column admits. Note also that the second test's `expect(worker.activeHarness()).toBe(\"codex\")` (worker-harness-failover.test.ts:231) is not what protects a restart in the exhausted state: `HarnessFailover.restore()` parks on index 0 when every entry is limited (packages/code/src/lib/agent/harness-failover.ts:167-181), so the retry protection comes from the persisted per-harness windows, not the persisted active name. A short reset window (the stub can emit `resets in 1 second`, which resetHintToMs parses at packages/agent-harness/src/detect-usage-limit.ts:343-362) would let the same fixture cover failback cheaply.",
        "evidence": "packages/e2e/docs/coverage-matrix.md:98; packages/code/src/lib/agent/harness-failover.ts:167-181; reset parsing at packages/agent-harness/src/detect-usage-limit.ts:343-362; full suite already runs the new file in ~2s (`bun test tests/local` -> 50.6s, 135 pass).",
        "suggestion": "Either add a third scenario with a one-second window that asserts `windowElapsed` resumes the primary (and optionally restart the daemon to assert the limited primary is not retried), or soften the claim in the matrix to say the switch path is covered and failback/restore are not."
      }
    ],
    "approved": true
  },
  "history": [
    {
      "iteration": 1,
      "feedback": {
        "summary": "This is a test-only change: it adds one local e2e test that drives the real built worker through harness failover, two read-only WorkerFixture accessors, and README/coverage-matrix updates. The fact that makes it safe is that the commit touches only packages/e2e (a private, non-published workspace) and both new accessors are additive with exactly one consumer, the new test — proven by `git show --stat` (4 e2e files only), `rg` (only worker-harness-failover.test.ts:192,224,195,231 call them), and the full local suite staying green (`bun test tests/local` -> 135 pass, 0 fail). Remaining findings are test-robustness and coupling issues, not behavior regressions.",
        "items": [
          {
            "priority": "low",
            "confidence": "high",
            "category": "code-quality",
            "file": "packages/e2e/helpers/worker.ts",
            "line": "410-424",
            "issue": "`rateLimits()` interpolates `RATE_LIMIT_KEY_PREFIX` into the SQL string (`key LIKE '${RATE_LIMIT_KEY_PREFIX}%'`) instead of binding it, and the pattern is not exact: SQLite treats `_` in LIKE as a single-character wildcard, so the prefix `rate_limit:` also matches keys such as `rateXlimit:...`. The only consumer is the new test (packages/e2e/tests/local/worker-harness-failover.test.ts:192,224), so no product behavior is affected, but it mirrors the same imprecise pattern in production (packages/code/src/lib/state/webhook-queue.ts:331) and the `.slice(RATE_LIMIT_KEY_PREFIX.length)` at line 420 would then produce a garbage harness name.",
            "evidence": "Ran a bun:sqlite probe: `SELECT key FROM webhook_meta WHERE key LIKE 'rate_limit:%'` returned both `rate_limit:codex` and `rateXlimit:claude`; production parity at packages/code/src/lib/state/webhook-queue.ts:331.",
            "suggestion": "Bind the pattern as a parameter (`WHERE key LIKE ?`, [`${RATE_LIMIT_KEY_PREFIX}%`]) and use an underscore-safe matcher such as `GLOB 'rate_limit:*'` or `substr(key, 1, ?) = ?` so the helper reads exactly the keys WebhookQueue writes."
          },
          {
            "priority": "low",
            "confidence": "low",
            "category": "performance",
            "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
            "line": "191-193",
            "issue": "The waitFor probe calls `worker.rateLimits()` twice per iteration on the truthy branch. Each call opens and closes a new read-only bun:sqlite connection (packages/e2e/helpers/worker.ts:528-541), so at the default 100ms interval this issues ~20 connections/second for up to 30s (worker.ts:451) just to return the same map that was already read.",
            "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:191-193 plus connection-per-call in packages/e2e/helpers/worker.ts:528-541.",
            "suggestion": "Read once into a local (`const windows = worker.rateLimits(); return (windows[\"claude-code\"] ?? 0) > Date.now() ? windows : undefined;`), matching the second test at lines 223-228."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
            "line": "250",
            "issue": "The second test's 120s budget is below the fixture's own worst case on the same machine: waitForReady 20s (helpers/worker.ts:261-262) + two waitForOutput calls at 20s each (469) + two waitFor calls at 30s each (451) + stop's 25s SIGTERM and 5s SIGKILL (487-494) is ~150s before prepare()/daemon work. The first test explicitly documents the opposite rule (lines 201-203: 180s must exceed the helper worst case), so a hung daemon in the second test trips Bun's timeout instead of the fixture's own diagnostic errors.",
            "evidence": "Timeout defaults at packages/e2e/helpers/worker.ts:262, 451, 469, 487-494; test budget at packages/e2e/tests/local/worker-harness-failover.test.ts:250.",
            "suggestion": "Give the second test the same 180_000 budget (or lower the nested waits) and mirror the worst-case comment from lines 201-203."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/helpers/worker.ts",
            "line": "72-76, 410-438",
            "issue": "The two new constants and the parsing logic duplicate private implementation details of WebhookQueue: the key format (packages/code/src/lib/state/webhook-queue.ts:280-282), the tolerant row parsing (329-350), and the active-harness key (352-355). If the key scheme is renamed or moved (both live in the same monorepo), the test does not fail with a clear assertion: `query()` swallows every error (worker.ts:535-537), so `rateLimits()` returns `{}` and the test burns the full 30s waitFor (worker.ts:451) and reports only a generic timeout.",
            "evidence": "Key scheme duplicated between packages/e2e/helpers/worker.ts:72-76,410-424 and packages/code/src/lib/state/webhook-queue.ts:280-282,331,354; error swallowing at packages/e2e/helpers/worker.ts:535-537.",
            "suggestion": "Export the meta-key helpers (or a `readFailoverState` accessor) from @getdevintern/code and use it here, or at minimum fail loudly for assertions that do not need polling (e.g. assert `activeHarness()` directly and surface the DB path in the message) so drift is diagnosed immediately."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
            "line": "238-246",
            "issue": "Unlike the first test's `diagnostics()` helper (lines 138-147), every bare expectation in the exhausted-chain test carries no context, so a failure of `runs.map(...)` (line 240), the invocation counts (241-242), `remoteBranches` (245), or `worker.running` (246) prints only `expected [...] received [...]` with no run rows, stub counts, or worker tail. These are exactly the assertions that diagnose the failover state machine, where the worker log is the primary evidence.",
            "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:238-246 vs the first test's diagnostics at 138-147.",
            "suggestion": "Build a `diagnostics()` closure for the second test too (status/runs, both invocation counts, remote refs, `worker.tail()`) and pass it as the `expect` message, as the first test does."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-harness-failover.test.ts",
            "line": "165-187",
            "issue": "The test pins behavior that is not the contract: the exact branch suffix `-attempt-2` (line 172, produced by packages/code/src/lib/utils/feature-branch.ts:387) and exact log prose (lines 184-187, produced by packages/code/src/lib/agent/harness-failover.ts:257 and the injected acquirer at packages/code/src/lib/workspace/injected-acquirer.ts:208). A legitimate product change (e.g. resuming the first attempt's branch instead of starting a fresh attempt, or rewording the failover log) fails this e2e even though failover still works, and the test already proves the real state via run records and the remote ref. The `not.toContain(\"keeping worktree for debugging\")` check is also a broad log-string guard unrelated to failover.",
            "evidence": "packages/e2e/tests/local/worker-harness-failover.test.ts:172,184-187; branch naming at packages/code/src/lib/utils/feature-branch.ts:361-390; log text at packages/code/src/lib/agent/harness-failover.ts:253-259.",
            "suggestion": "Assert the stable observable instead of the wording where possible (succeeded run's branch exists on the remote, exactly one succeeded attempt, primary invoked once) and keep at most one loose log check such as /hit a usage limit/; if the branch suffix is intentional contract, note it in the coverage matrix so a future change is a deliberate decision."
          },
          {
            "priority": "info",
            "confidence": "medium",
            "category": "documentation",
            "file": "packages/e2e/docs/coverage-matrix.md",
            "line": "98",
            "issue": "The row is promoted from `gap` to `default` while neither the failback path nor restart-time restore is exercised, which the Missing column admits. Note also that the second test's `expect(worker.activeHarness()).toBe(\"codex\")` (worker-harness-failover.test.ts:231) is not what protects a restart in the exhausted state: `HarnessFailover.restore()` parks on index 0 when every entry is limited (packages/code/src/lib/agent/harness-failover.ts:167-181), so the retry protection comes from the persisted per-harness windows, not the persisted active name. A short reset window (the stub can emit `resets in 1 second`, which resetHintToMs parses at packages/agent-harness/src/detect-usage-limit.ts:343-362) would let the same fixture cover failback cheaply.",
            "evidence": "packages/e2e/docs/coverage-matrix.md:98; packages/code/src/lib/agent/harness-failover.ts:167-181; reset parsing at packages/agent-harness/src/detect-usage-limit.ts:343-362; full suite already runs the new file in ~2s (`bun test tests/local` -> 50.6s, 135 pass).",
            "suggestion": "Either add a third scenario with a one-second window that asserts `windowElapsed` resumes the primary (and optionally restart the daemon to assert the limited primary is not retried), or soften the claim in the matrix to say the switch path is covered and failback/restore are not."
          }
        ],
        "approved": true
      },
      "addressed": []
    }
  ]
}