[danii1]

Why

Task local-test-e2e-cover-sentry-error-monitor-auto: test(e2e): cover Sentry error-monitor auto-fix against an offline fake API

The worker’s Sentry error-monitor path was entirely untested: issue polling, occurrence gating, actionability validation, durable issue:<id> claiming, generated markdown task handoff, run attribution, branch push, and comment reporting could regress without notice. The new test exercises that full path against an in-process fake, with no Sentry account, token, project, or network access.

What changed

• Added packages/e2e/providers/fake-sentry.ts: a strict 127.0.0.1:0 Bun server implementing the project-scoped and organization-wide issue-list routes plus the issue-comment route. It returns a seeded issue payload, records list parameters and comments, records unexpected requests, and returns diagnostic 404s for wrong routes/scopes.
• Extended WorkerFixture in packages/e2e/helpers/worker.ts with an optional workspaceTomlExtra appended to the generated workspace config, leaving the default TOML byte-for-byte unchanged, and exposed the runs.origin column through WorkerRunRecord.
• Added packages/e2e/tests/local/worker-sentry-monitor.test.ts: starts the real worker with an [[error_monitors]] source, waits for exactly one succeeded error_monitor run, asserts the pushed feature/ branch contains only the greeting edit, validates the generated error-fixes/*.md untrusted block and issue text without credentials, asserts one success comment, and waits for two further polls to prove no duplicate run or comment.
• Updated packages/e2e/README.md with an offline fake Sentry section and default command, and packages/e2e/docs/coverage-matrix.md with both Sentry rows at default, the test filename as evidence, and backlog item 15 marked implemented.

Scope

Included is only the Sentry error-monitor polling/auto-fix/dedupe path and its local test infrastructure. Production Sentry client, error-monitor acquirer, other providers, other e2e helpers, and unrelated tests were not changed. No commit, push, label, or pull request was created.

Tradeoffs

• The expected success comment is duplicated in the test rather than importing production code, keeping the assertion independent but requiring the test and production text to change together.
• The run’s task_key is sentry-e2e-4501, not the bare Sentry id: production derives the workflow key from the generated markdown filename (<monitor-id>-<issue-short-id>). The test asserts that exact key and that it contains the fake issue id; making it equal the bare id would require a production change outside scope.
• The fake also implements the organization-wide list path, but the test asserts the project-scoped request, so a client switching to the wrong path fails.
• The test uses a 120-second cap around bounded polling; the observed runtime is about 2 seconds.

Blast Radius

WorkerFixture.runRecords() now selects the existing origin column, which is additive for current callers. workspaceTomlExtra defaults to undefined, so existing workspace fixtures generate identical TOML. The fake provider and new test are isolated to the e2e package. Documentation consumers are limited to the e2e README and coverage matrix.

Verification

Ran in order from the repository root:

• bun run --filter @getdevintern/code build — exit 0.
• bun run --filter @devintern/e2e test — 134 pass, 16 skip, 4 todo, 0 fail across 24 files (51.54s).
• bun run --filter @devintern/e2e typecheck — exit 0.
• bun run --filter @devintern/e2e lint — exit 0.
• bun run --filter @devintern/e2e format:check — all files correctly formatted.

Also ran bun test tests/local/worker-sentry-monitor.test.ts (1 pass, 2.20s) and with --rerun-each 3 (3 pass, 6.55s) to check stability.

---

This PR was automatically created by @devintern/code

[danii1]

Artifacts & verification

Run: 30 · Commit: 8de64e39c54efab234876bbd0b14830943522e30

Change explanation

Produced by pr-explainer-narrative, execution 1 at 8de64e39c54efab234876bbd0b14830943522e30.

test(e2e): cover Sentry error-monitor auto-fix against an offline fake API

Why this change is needed

The task states that Sentry error monitors are an external integration the repository does not control. The worker fetches unresolved issues over the Sentry API, gates on occurrence count and actionability, claims the issue for deduplication, runs the normal implementation pipeline from a generated markdown task, and optionally comments back on the Sentry issue. None of that path was covered, so a regression in the request path, response parsing, dedupe key, or comment endpoint could ship undetected.

Because the production client already accepts a configurable base_url, the task asks for this to be proven offline with an in-process fake rather than a real Sentry project or token.

What changed

• Added an offline fake Sentry provider, packages/e2e/providers/fake-sentry.ts.

  • Serves one seeded unresolved issue from the project-scoped and organization-wide issue-list routes, plus the organization issue-comment route.
  • Returns a diagnostic 404 that names the method and path for every other route, and records unexpected requests.
  • Captures list requests (query, sort, stats period, page size) and comment bodies for assertions.
  • Binds 127.0.0.1:0, makes no outbound requests, accepts any Bearer token, and does not read or log credentials.
  • Parses raw HTTP and owns its own response shapes; it does not import the production Sentry client.

• Extended the worker fixture in packages/e2e/helpers/worker.ts.

  • workspaceTomlExtra appends an additional TOML section while preserving the existing workspace, execution, defaults, and repo settings. Existing callers that omit it get the same default workspace.toml.
  • runRecords() now includes the origin column, so tests can assert that a run came from error_monitor.

• Added packages/e2e/tests/local/worker-sentry-monitor.test.ts, which starts the real worker daemon against the fake and asserts:

  • Exactly one run exists, with origin error_monitor, status succeeded, and a task key tied to the seeded issue.
  • The client used the project-scoped route with the expected query, sort, stats period, and page size.
  • The pushed feature/ branch contains only the deterministic src/greeting.ts edit.
  • The generated error-fixes/ markdown contains the untrusted-content framing, the issue title and culprit, and no token or token variable name.
  • The seeded issue title, which carries a unique marker, reaches the agent input.
  • The fake receives exactly one success comment for the correct issue.
  • Two further polls of the same issue produce no second run and no second comment.
  • The worker is still running and the fake recorded no unexpected requests.

• Updated documentation: packages/e2e/README.md documents the offline Sentry scenario, and packages/e2e/docs/coverage-matrix.md moves both Sentry rows to default with the new test file and marks backlog item 15 as implemented.

Validation actually performed

Observed evidence from the pipeline:

• bun run --filter @getdevintern/code build passed.
• bun run --filter @devintern/e2e test passed with 134 pass, 16 skip, and 0 fail across 24 files.
• The new Sentry test also passed on its own: 1 pass, 27 expect calls, about 2 seconds.
• bun run --filter @devintern/e2e typecheck, lint, and format:check all passed.
• Auto-review reran the new test (1 pass) and the existing worker and worker-add suites (14 pass), plus typecheck, lint, and format checks.

Also observed during review: a probe importing the fake showed that requests with no Authorization header and with a wrong Bearer token both returned 200. That is a limitation of the fake, not a production failure.

Not recorded: no live Sentry project, real token, or network-backed run was exercised. No manual product verification beyond the automated checks above.

Material risks

• The change is test-only. The shared helper additions are additive: workspaceTomlExtra is optional and unused by existing callers, and runs.origin is an existing NOT NULL column, so existing runRecords() consumers only gain a field.
• The fake does not validate the Authorization header. This test would still pass if SentryClient stopped sending it; that contract is only covered by the existing unit test.
• The dedupe assertion waits for two further fake reads and then immediately checks run and comment counts. If dedupe regressed, a second run dispatched on the later read could be stopped by dispose() before its run row or comment is written, so the failure might not be observed reliably.
• expect(run.task_key).toContain(sentry.issueId) is effectively guaranteed by the chosen fixture, because the default shortId E2E-4501 already contains the issue id 4501. It adds little independent coverage.
• The fake is a hand-written replica of the Sentry API. If production request behavior changes without updating the fake, the test can fail for reasons unrelated to the intended contract, or miss behavior the fake does not model.

Where reviewers should focus

• packages/e2e/tests/local/worker-sentry-monitor.test.ts: the core behavior assertions, especially the dedupe wait and the claim that the Sentry payload reaches the agent input.
• packages/e2e/providers/fake-sentry.ts: routing strictness, response shape fidelity, and the lack of auth-header validation.
• packages/e2e/helpers/worker.ts: whether workspaceTomlExtra preserves the exact default output and whether adding origin to runRecords() affects other consumers.
• packages/e2e/docs/coverage-matrix.md and packages/e2e/README.md: confirm the claimed coverage matches what the test actually proves.

Visual explanation

Produced by pr-explainer-visual, execution 1 at 8de64e39c54efab234876bbd0b14830943522e30.

Show artifact

Summary

Adds the first end-to-end coverage of the Sentry error-monitor auto-fix path: [redacted] in-process fake Sentry API, a small opt-in hook to append an [[error_monitors]] block to the generated workspace, and a local test that drives the real worker through fetch, dedupe, fix run, and comment-back. It matters because the Sentry request path, response parsing, dedupe key, and comment endpoint are external-integration behavior that previously had no test at all. Documentation rows in the coverage matrix move from gap to default.

Evidence

FakeSentryServer (127.0.0.1:0, own state, no product imports)
  GET  /api/0/projects/e2e-org/e2e-project/issues/     -> one seeded issue
  POST /api/0/organizations/e2e-org/issues/4501/comments/ -> captured body
  anything else                                       -> diagnostic 404 + recorded
             |
             v
WorkerFixture + workspaceTomlExtra
  default workspace.toml unchanged, then appended [[error_monitors]]
  base_url=fake, poll_interval=1, min_occurrences=1, max_per_tick=1
             |
             v
real worker daemon
  poll -> dispatch 1 run (origin=error_monitor) -> branch feature/<task_key>
        -> comment once -> error-fixes/<task>.md with framed issue title/culprit
  poll again twice -> still 1 run, still 1 comment (dedupe by issue:<id>)

Validation observed in the pipeline evidence:

• bun run --filter @getdevintern/code build passed.
• bun run --filter @devintern/e2e test passed: 134 pass / 16 skip / 0 fail across 24 files.
• e2e typecheck, lint, and format:check passed.
• The new test passed in isolation (1 pass, 27 assertions, roughly 2 seconds).
• Auto-review also ran the new test and the existing worker suites (14 pass) successfully.

Before/after execution results were not captured, so this explanation only reports the post-change runs above.

Merge Danger

Reverting removes a test, the fake provider, and two additive helper changes, so no production behavior is lost — only the new coverage. The only shared-helper surface is WorkerFixture: workspaceTomlExtra is optional and unused by existing callers, so the default workspace.toml output is preserved, and runs.origin already exists as a NOT NULL column in the run schema, making the added SELECT field additive for existing consumers. Blast radius is confined to packages/e2e, and the change is safe to ship without a staged rollout because it does not alter packages/code or any shipped CLI behavior.

Verification report

Produced by __verify-1, execution 1 at dc0f03929aaecd9d97175879d8211b6347bd2458.

Show artifact

{
  "summary": "Verified the implementation against every functional requirement and acceptance command. `bun run --filter @getdevintern/code build` passed; `bun run --filter @devintern/e2e test` passed with 134 pass / 16 skip / 0 fail across 24 files; e2e `typecheck`, `lint`, and `format:check` all passed; the new test also passes alone (1 pass, 27 expect calls, ~2s). The fake, fixture extension, test assertions, and documentation are present, and the coverage-matrix Sentry rows are updated to `default` with `tests/local/worker-sentry-monitor.test.ts`. Blast-radius safety fact: `workspaceTomlExtra` is optional and no existing `WorkerFixture` caller sets it, so `prepare()` still emits the same default `workspace.toml`; the added `origin` field is an existing NOT NULL column in every `runs` schema (`packages/code/src/lib/state/run-recorder.ts:211`), so existing `runRecords()` consumers only gain an additive field. No unmet functional requirement found.",
  "items": [
    {
      "priority": "low",
      "confidence": "medium",
      "category": "testing",
      "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
      "line": "134",
      "issue": "The dedupe assertion waits for two further fake reads and then immediately asserts one run/comment. If dedupe regressed, a second run dispatched on the second read could be stopped by dispose() before its asynchronous run row or comment is written, so the test may not always fail for a dedupe regression.",
      "evidence": "Pointed at test lines 134-143 (`waitFor` issueReadCount +2 then `toHaveLength(1)`). The run row is written asynchronously by the CLI child after dispatch (`packages/code/src/lib/workspace/fleet-executor.ts:341` spawns the task; `packages/code/src/lib/task/pipeline.ts:555` calls `beginRun`), and `dispose()` stops the worker in `finally`. Full e2e suite passed, so this race was not observed in practice.",
      "suggestion": "After the two reads, add a bounded settle check that proves no new write appears (for example, wait a short bounded interval and re-check run/comment counts, or assert the dedupe/processed state remains stable) before the final count assertions."
    },
    {
      "priority": "info",
      "confidence": "high",
      "category": "documentation",
      "file": "packages/e2e/README.md",
      "line": "304",
      "issue": "The new README section says the fake implements only the project issue-list endpoint, but `fake-sentry.ts` also implements the organization-wide issue-list route.",
      "evidence": "`packages/e2e/README.md:304` says 'implements only the project issue-list'; `packages/e2e/providers/fake-sentry.ts:171-174` implements `GET /api/0/organizations/{org}/issues/`.",
      "suggestion": "Say the fake implements the project- and organization-scoped issue-list routes plus the issue-comment route."
    },
    {
      "priority": "info",
      "confidence": "high",
      "category": "testing",
      "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
      "line": "154",
      "issue": "The test budget is 120s, above the task's suggested 60s. This is not a functional failure because the test completed in ~2s and all waits are bounded.",
      "evidence": "Ran `bun test tests/local/worker-sentry-monitor.test.ts` from `packages/e2e`: 1 pass, 27 expect() calls, 2.18s. Timeout literal is at line 154.",
      "suggestion": "Consider lowering the per-test timeout to 60_000 to match the task guidance."
    }
  ],
  "approved": true
}

Auto-review summary

Produced by auto-review, execution 1 at 8de64e39c54efab234876bbd0b14830943522e30.

Show artifact

{
  "iterations": 2,
  "success": true,
  "finalFeedback": {
    "summary": "The change is safe because it is test-only: the new `FakeSentryServer` and `workspaceTomlExtra` option have exactly one caller (the new e2e test), and the only shared helper change adds `runs.origin` to a SELECT, matching the production schema's `origin TEXT NOT NULL`. I verified this by running the new test (1 pass), the existing worker/worker-add suites (14 pass), and e2e typecheck, lint, and format checks; the remaining findings are minor test-quality nits, not production risks.",
    "items": [
      {
        "priority": "low",
        "confidence": "high",
        "category": "testing",
        "file": "packages/e2e/providers/fake-sentry.ts",
        "line": "161-181",
        "issue": "The fake never reads `request.headers`, so it accepts requests with no Authorization header or a wrong token. The e2e test sets `SENTRY_AUTH_TOKEN` and asserts it is not leaked, but it would still pass if `SentryClient` stopped sending `Authorization`; only the unit test at `packages/code/tests/sentry-acquirer.test.ts:83-101` covers that contract.",
        "evidence": "Ran a probe importing `FakeSentryServer`: a request with no Authorization returned 200 and a request with `Bearer wrong` also returned 200 (`list requests: 2`, `unexpected: 0`).",
        "suggestion": "Capture the `Authorization` header on accepted list/comment requests and assert it equals `Bearer ${authToken}`, or optionally reject missing/non-Bearer [redacted] so a client regression fails immediately."
      },
      {
        "priority": "info",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
        "line": "85-88",
        "issue": "`expect(run.task_key).toContain(sentry.issueId)` is tautological with the chosen fixture because the default shortId `E2E-4501` contains the id `4501`. The comment also says the filename carries the Sentry issue id, but the production filename/key is built from `issue.displayId` (`workspace-worker.ts:1042`), which is mapped from `shortId` (`sentry-client.ts:239`), not from `issue.id`.",
        "evidence": "Pointed to `packages/code/src/lib/workspace/workspace-worker.ts:1042` and `packages/code/src/lib/observability/sentry-client.ts:239`; the test passes with the default shortId containing the id.",
        "suggestion": "Drop the redundant `toContain(issueId)` assertion and fix the comment to say the key carries the monitor id plus the Sentry short id; or seed an id/shortId pair where the shortId does not embed the id if the intent is to prove id inclusion."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
        "line": "138-168",
        "issue": "The 120s test timeout does not account for every bounded wait plus shutdown: the comment wait is an additional 15s and `WorkerFixture.stop()` can take 25s SIGTERM plus 5s SIGKILL (`helpers/worker.ts:461-469`). Worst-case successful ordering (20s readiness + 45s run + 15s comment + 20s dedupe + 30s shutdown) exceeds 120s, leaving no room for prepare/assertions on a slow CI runner.",
        "evidence": "Pointed to the waits at lines 60, 73-80, 138-153 and the shutdown logic at `packages/e2e/helpers/worker.ts:461-469`; the comparable `worker.test.ts` uses a 180_000ms budget for this reason.",
        "suggestion": "Raise the timeout to 150_000-180_000ms, or derive it from the helper's documented worst-case sums including the comment wait and graceful shutdown."
      },
      {
        "priority": "low",
        "confidence": "medium",
        "category": "code-quality",
        "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
        "line": "158-163",
        "issue": "Cleanup stops the fake Sentry server before disposing the still-running worker. Because the monitor polls every second (`poll_interval = 1` at line 29), the worker can issue another poll against a closed port during shutdown, producing avoidable source-failure logs and potentially delaying graceful exit.",
        "evidence": "Pointed to the `finally` block at lines 158-163 and `ErrorMonitorAcquirer.start()` interval scheduling at `packages/code/src/lib/acquirers/error-monitor.ts:172-178`.",
        "suggestion": "Dispose the worker first, then stop the fake server, or assert no further polls after the dedupe check before shutting the fake down."
      }
    ],
    "approved": true
  },
  "history": [
    {
      "iteration": 1,
      "feedback": {
        "summary": "The change is test-only: `git diff --stat HEAD^ HEAD` touches only `packages/e2e`, and the shared helper additions are additive — `runs.origin` is already a NOT NULL column (`run-recorder.ts:211`) and grep shows no other `WorkerRunRecord` consumers, so existing worker tests are unaffected. That safety fact was proven to `ran-it`: `bun run --filter @devintern/e2e test` produced 134 pass / 0 fail / 0 exit, the new scenario passed on its own, and e2e typecheck, lint, and format checks all pass. The scenario is well constructed, but one medium coverage gap remains: it proves the generated `error-fixes` markdown, not that the Sentry issue content reaches the agent prompt.",
        "items": [
          {
            "priority": "medium",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "107-120",
            "issue": "The test asserts the generated error-fixes markdown contains the issue, but never proves that content reaches the agent prompt. The deterministic fake agent ignores prompt content (`helpers/fake-agent.ts:7,62-70`) and always edits `src/greeting.ts`, so a regression that drops the Sentry details from the prompt, or sends an empty description, would still produce a successful run, pushed branch, and comment — the exact end-to-end behavior this PR claims to cover.",
            "evidence": "Read `packages/e2e/helpers/fake-agent.ts:7,62-70`: the marker check only runs when `E2E_EXPECT_DESCRIPTION_MARKER` is set; otherwise the agent performs the fixed implement edit. Ran a variant of the new scenario with `worker.env.E2E_EXPECT_DESCRIPTION_MARKER` set to the seeded issue title; it exited 0, confirming the title does reach the agent argv today while the committed test never asserts it.",
            "suggestion": "Seed a unique marker into the fake issue title or metadata and set `worker.env.E2E_EXPECT_DESCRIPTION_MARKER` before `prepare()`, or assert on the agent invocation/argv, so the test fails if the Sentry payload stops reaching the prompt."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "113-116",
            "issue": "The untrusted-content assertion checks only the BEGIN marker and notice text; it never asserts the END marker `<!-- END UNTRUSTED EXTERNAL CONTENT: sentry issue -->` or that the issue title/culprit sit inside the frame. A regression that drops the closing delimiter, or appends injected text after it, would still pass.",
            "evidence": "`packages/e2e/tests/local/worker-sentry-monitor.test.ts:113-116` asserts only BEGIN plus the notice; `packages/code/src/lib/prompts/untrusted-content.ts:51-58` is responsible for producing both the BEGIN and END markers.",
            "suggestion": "Assert both boundary markers and use an index-based check that `issueTitle` and `issueCulprit` occur between the BEGIN and END markers."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "documentation",
            "file": "packages/e2e/README.md",
            "line": "304-306",
            "issue": "The README says the fake implements \"only the project issue-list and issue-comment endpoints\", but `fake-sentry.ts:171-174` also implements the organization-wide issue list. That makes the documented route coverage inaccurate for anyone debugging an org-wide Sentry configuration.",
            "evidence": "`packages/e2e/README.md:304-306` versus `packages/e2e/providers/fake-sentry.ts:166-179`, which routes both `/api/0/projects/<org>/<project>/issues/` and `/api/0/organizations/<org>/issues/`.",
            "suggestion": "Describe the fake as implementing the issue-list route (project- and organization-scoped) plus the issue-comment route, or intentionally remove the organization-wide route if it is not meant to be supported."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "132-142",
            "issue": "The comment and README claim this proves the durable `issue:<id>` dedupe, but the test only proves dedupe across two polls in one worker process. It never restarts the worker against the same state DB, so an in-memory-only regression could still pass; the durability claim is stronger than the assertion.",
            "evidence": "`packages/e2e/tests/local/worker-sentry-monitor.test.ts:132-142` waits for two more reads in the same process. `packages/code/src/lib/state/webhook-queue.ts:430-437` shows `tryMarkProcessed` does insert into SQLite `processed_events`, but the test never exercises a restart.",
            "suggestion": "Either stop and restart the worker with the same workspace/database and assert no new run or comment, or reword the README/test comment to \"deduped across subsequent polls\"."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "security",
            "file": "packages/e2e/providers/fake-sentry.ts",
            "line": "161-181",
            "issue": "The fake accepts every request without checking or recording `Authorization`, so a Sentry client regression that dropped or malformed the Bearer header would still list issues and post comments. The test's synthetic token proves provider construction, but not that the token reaches the HTTP layer.",
            "evidence": "`packages/e2e/providers/fake-sentry.ts:161-181` never reads `request.headers`; `packages/e2e/tests/local/worker-sentry-monitor.test.ts:40-44` sets `SENTRY_AUTH_TOKEN` and only asserts the token is absent from the generated markdown.",
            "suggestion": "Capture the `Authorization` header on list/comment requests in the fake (without logging it) and assert it equals `Bearer ${authToken}` in the test."
          },
          {
            "priority": "info",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "124-142",
            "issue": "Only the success path is exercised. `SENTRY_ACTION_FAILED_COMMENT` (`sentry-client.ts:21-23`) and the `skipped`/`deferred` branches in `ErrorMonitorAcquirer.tick` (`acquirers/error-monitor.ts:228-243`) are not covered by this scenario, so the failure-comment contract remains untested offline.",
            "evidence": "The test only asserts `EXPECTED_SUCCESS_COMMENT`; `packages/code/src/lib/observability/sentry-client.ts:21-23` defines the failed comment, and `packages/code/src/lib/acquirers/error-monitor.ts:228-243` branches on deferred/skipped results.",
            "suggestion": "Add a second scenario using the fake agent's `fail` mode or a second seeded issue to assert the failure comment, and optionally assert `comment_on_action = false` posts nothing."
          }
        ],
        "approved": false
      },
      "addressed": [
        {
          "priority": "medium",
          "confidence": "medium",
          "category": "testing",
          "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
          "line": "107-120",
          "issue": "The test asserts the generated error-fixes markdown contains the issue, but never proves that content reaches the agent prompt. The deterministic fake agent ignores prompt content (`helpers/fake-agent.ts:7,62-70`) and always edits `src/greeting.ts`, so a regression that drops the Sentry details from the prompt, or sends an empty description, would still produce a successful run, pushed branch, and comment — the exact end-to-end behavior this PR claims to cover.",
          "evidence": "Read `packages/e2e/helpers/fake-agent.ts:7,62-70`: the marker check only runs when `E2E_EXPECT_DESCRIPTION_MARKER` is set; otherwise the agent performs the fixed implement edit. Ran a variant of the new scenario with `worker.env.E2E_EXPECT_DESCRIPTION_MARKER` set to the seeded issue title; it exited 0, confirming the title does reach the agent argv today while the committed test never asserts it.",
          "suggestion": "Seed a unique marker into the fake issue title or metadata and set `worker.env.E2E_EXPECT_DESCRIPTION_MARKER` before `prepare()`, or assert on the agent invocation/argv, so the test fails if the Sentry payload stops reaching the prompt."
        }
      ]
    },
    {
      "iteration": 2,
      "feedback": {
        "summary": "The change is safe because it is test-only: the new `FakeSentryServer` and `workspaceTomlExtra` option have exactly one caller (the new e2e test), and the only shared helper change adds `runs.origin` to a SELECT, matching the production schema's `origin TEXT NOT NULL`. I verified this by running the new test (1 pass), the existing worker/worker-add suites (14 pass), and e2e typecheck, lint, and format checks; the remaining findings are minor test-quality nits, not production risks.",
        "items": [
          {
            "priority": "low",
            "confidence": "high",
            "category": "testing",
            "file": "packages/e2e/providers/fake-sentry.ts",
            "line": "161-181",
            "issue": "The fake never reads `request.headers`, so it accepts requests with no Authorization header or a wrong token. The e2e test sets `SENTRY_AUTH_TOKEN` and asserts it is not leaked, but it would still pass if `SentryClient` stopped sending `Authorization`; only the unit test at `packages/code/tests/sentry-acquirer.test.ts:83-101` covers that contract.",
            "evidence": "Ran a probe importing `FakeSentryServer`: a request with no Authorization returned 200 and a request with `Bearer wrong` also returned 200 (`list requests: 2`, `unexpected: 0`).",
            "suggestion": "Capture the `Authorization` header on accepted list/comment requests and assert it equals `Bearer ${authToken}`, or optionally reject missing/non-Bearer [redacted] so a client regression fails immediately."
          },
          {
            "priority": "info",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "85-88",
            "issue": "`expect(run.task_key).toContain(sentry.issueId)` is tautological with the chosen fixture because the default shortId `E2E-4501` contains the id `4501`. The comment also says the filename carries the Sentry issue id, but the production filename/key is built from `issue.displayId` (`workspace-worker.ts:1042`), which is mapped from `shortId` (`sentry-client.ts:239`), not from `issue.id`.",
            "evidence": "Pointed to `packages/code/src/lib/workspace/workspace-worker.ts:1042` and `packages/code/src/lib/observability/sentry-client.ts:239`; the test passes with the default shortId containing the id.",
            "suggestion": "Drop the redundant `toContain(issueId)` assertion and fix the comment to say the key carries the monitor id plus the Sentry short id; or seed an id/shortId pair where the shortId does not embed the id if the intent is to prove id inclusion."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "138-168",
            "issue": "The 120s test timeout does not account for every bounded wait plus shutdown: the comment wait is an additional 15s and `WorkerFixture.stop()` can take 25s SIGTERM plus 5s SIGKILL (`helpers/worker.ts:461-469`). Worst-case successful ordering (20s readiness + 45s run + 15s comment + 20s dedupe + 30s shutdown) exceeds 120s, leaving no room for prepare/assertions on a slow CI runner.",
            "evidence": "Pointed to the waits at lines 60, 73-80, 138-153 and the shutdown logic at `packages/e2e/helpers/worker.ts:461-469`; the comparable `worker.test.ts` uses a 180_000ms budget for this reason.",
            "suggestion": "Raise the timeout to 150_000-180_000ms, or derive it from the helper's documented worst-case sums including the comment wait and graceful shutdown."
          },
          {
            "priority": "low",
            "confidence": "medium",
            "category": "code-quality",
            "file": "packages/e2e/tests/local/worker-sentry-monitor.test.ts",
            "line": "158-163",
            "issue": "Cleanup stops the fake Sentry server before disposing the still-running worker. Because the monitor polls every second (`poll_interval = 1` at line 29), the worker can issue another poll against a closed port during shutdown, producing avoidable source-failure logs and potentially delaying graceful exit.",
            "evidence": "Pointed to the `finally` block at lines 158-163 and `ErrorMonitorAcquirer.start()` interval scheduling at `packages/code/src/lib/acquirers/error-monitor.ts:172-178`.",
            "suggestion": "Dispose the worker first, then stop the fake server, or assert no further polls after the dedupe check before shutting the fake down."
          }
        ],
        "approved": true
      },
      "addressed": []
    }
  ]
}

[danii1]

🔀 This branch had merge conflicts with main; devintern resolved them and pushed the merge. Please double-check the resolution.