fix(agent): stop scoped-session S3 hang in baseline build + reap hung suites + early ACK

[isadeks]

Closes #615.

Stacked on #598 (pr/ecs-test-hygiene) — extends its "fail hung tests loudly" theme with the scoped-session root cause it doesn't cover. Review/merge after #596→#597→#598.

What hangs

On the ECS substrate the pre-agent baseline build (mise run build) stalls silently for 40+ min. The ECS agent task def sets AGENT_SESSION_ROLE_ARN, so aws_session resolves a scoped session and tenant_client returns session.client(...) — bypassing a @patch("boto3.client") mock. test_attachments then makes a real S3 get_object that blocks forever (no egress) in a socket read pytest-timeout's SIGALRM can't interrupt.

Fix (3 parts)

1. conftest _clean_env — add AGENT_SESSION_ROLE_ARN to the scrub list and reset the aws_session cache each test, so every test resolves the unscoped path where the mock intercepts. The scrub is required in addition to the reset: a cold get_session() re-resolves scoped while the var is still set. + regression test.
2. conftest hang watchdog — an independent daemon-thread reaper (os._exit(1) at 600s). faulthandler.dump_traceback_later(exit=True) doesn't work: pytest's faulthandler_timeout re-arms faulthandler's single timer per-test without exit=True, so a hang only dumps, never exits.
3. pipeline early ACK — move token resolve + 👀 + Jira start comment to before setup_repo() so a large-repo task shows immediate feedback during the multi-minute baseline instead of looking dead. configure_channel_mcp stays after the clone (needs repo dir).

Verification

Diagnosed + deployed + live-verified on the linear-vercel line (dev, ECS substrate): the suite that hung 55 min now runs test_attachments in ~1s and the whole suite in ~12s; the 👀 appears ~3s after task start. Full agent gate green (1201 tests under AGENT_SESSION_ROLE_ARN set), ruff + ty clean.

🤖 Generated with Claude Code

[scottschreckengaust]

Verdict: Request changes

The root-cause diagnosis and the three-part fix are correct, well-reasoned, and I verified the S3-hang fix empirically (with AGENT_SESSION_ROLE_ARN set, test_attachments + the new class now run in <1s instead of hanging). The one blocker is that the headline regression guard does not actually guard the fix — on a PR whose entire purpose is to install that guard, that is load-bearing. Two low-risk follow-ups (watchdog os._exit window + a factually wrong comment) round it out.

Vision alignment

Strongly aligned. This strengthens tenet 1 (fire-and-forget) — the early-ACK move gives the submitter immediate 👀/In-Progress feedback during the multi-minute baseline instead of a dead-looking issue, and lets the crash handler surface a setup-phase failure as ❌. It reinforces tenet 3 (bounded blast radius): the hang watchdog reaps a wedged suite at 600s instead of burning to the 3600s build-verify ceiling, and the env scrub keeps tests on the unscoped path (no accidental real S3 egress). No tenet is traded; no ADR needed. AGENT_SESSION_ROLE_ARN semantics (scoped session, fail-closed) are unchanged — this only fixes test-harness leakage of it.

Blocking issues

1. test_session_role_arn_not_visible_to_tests is a false regression guard (agent/tests/test_aws_session.py:96-101). The docstring says it verifies the conftest _clean_env fixture scrubs AGENT_SESSION_ROLE_ARN. It does not. This module has its own file-local autouse fixture _reset (lines 24-30) that already does monkeypatch.delenv(SESSION_ROLE_ARN_ENV, raising=False) + reset_session_cache(). I confirmed empirically: with AGENT_SESSION_ROLE_ARN set in the parent env and the "AGENT_SESSION_ROLE_ARN" line removed from conftest.py's _AGENT_ENV_VARS, both tests still pass — the local _reset masks the removal. Only when I also removed the local delenv did they fail. So the exact line this PR adds to conftest is a silent no-op for these tests; a future edit that drops the conftest scrub would keep CI green until the ECS substrate hangs again — the precise regression this guard is supposed to catch (AI005: the test asserts what the local fixture guarantees, not that the fix under test is present). test_session_resolves_unscoped_by_default (lines 103-108) has the same defect for the same reason.

Risk: the PR's core deliverable — a durable guard against the ECS hang — provides false confidence.

Fix (validated by the pr-test-analyzer): move both assertions into a fixture-free module (e.g. tests/test_conftest_env_scrub.py) with no local _reset, so only the autouse conftest _clean_env is in play, and let the guard depend on the parent env carrying the var. That is a true bidirectional guard (passes with the scrub, fails without it). Note: monkeypatch.setenv(...) inside the test body is a dead end — autouse fixtures run during setup, before the body, so they can't scrub what the body sets afterward.

Non-blocking suggestions / nits

2. Hang watchdog can hard-exit an already-finished slow run (agent/tests/conftest.py:36-51). _reap_on_hang calls os._exit(1) unconditionally when the 600s daemon Timer fires. daemon=True prevents the timer from blocking shutdown, but not from firing if a legitimately slow-but-passing suite lands near 600s (e.g. still in pytest teardown / coverage write). os._exit skips atexit + buffer flush, so it would turn a green run red with a bewildering thread-dump uncorrelated to any failed test. Suggest cancelling on session finish so it only fires on a true hang: add def pytest_sessionfinish(session, exitstatus): _hang_watchdog.cancel(). Timer.cancel() is a no-op if it already fired.

3. Factually wrong comment in the watchdog rationale (agent/tests/conftest.py:32). It says "per-test cap is 300s" but the actual pytest-timeout config is timeout = 120 (agent/pyproject.toml:136); no 300s override exists. This is comment rot at authoring time in the load-bearing justification for the 600s deadline. Change to 120s (or reframe 600s as a session backstop for hangs SIGALRM can't interrupt, sized between the longest healthy suite and the build ceiling).

4. No pipeline test pins the early-ACK ordering (agent/src/pipeline.py:826-881). The reorder is correct — I confirmed the moved calls (resolve_linear_api_token/resolve_jira_oauth_token/react_task_started/comment_task_started) don't depend on setup_repo() output, configure_channel_mcp correctly stays after the clone (needs repo_dir), and linear_eyes_reaction_id is initialized to None at line 730 so the crash handler is always bound. But the PR sells "a setup-phase failure now has a 👀 to swap to ❌" as a benefit, and no test asserts 👀-before-setup_repo or the ❌-swap on setup failure. A focused test (attach both to a parent mock, assert call order + a setup_repo-raises case) would pin the invariant against future reordering. Follow-up, not a blocker — the individual units are covered in their own module tests.

5. Comment triplication (nit). The ECS-hang narrative is duplicated near-verbatim in three prose blocks (conftest.py:141-149, conftest.py:158-170, test_aws_session.py:83-97). Accurate but a sync/comment-rot surface; consider one authoritative block referenced by the others.

Documentation

No docs changes required or missing. The diff is Python-only (pipeline.py, conftest.py, test_aws_session.py) — no docs/guides/, docs/design/, or CONTRIBUTING.md edits, so no Starlight mirror sync is needed. AGENT_SESSION_ROLE_ARN is already documented (BEDROCK_COST_ATTRIBUTION); its contract is unchanged. No cdk/.../types.ts ↔ cli/src/types.ts sync, no Cedar engine bump, no roadmap item to check off.

Tests & CI

CI green (build agentcore 5m15s, secrets/deps scan, dead-code advisory, title). No CDK construct/stack changes → bootstrap synth-coverage not applicable. Agent gate reportedly green (1201 tests under AGENT_SESSION_ROLE_ARN set). I ran the two new test classes + test_attachments under AGENT_SESSION_ROLE_ARN set: 13 passed in 0.69s (no hang). The watchdog is untested — acceptable (a process-killer is impractical to exercise in-process). Base freshness: head dd7146f is a direct descendant of origin/pr/ecs-test-hygiene tip 32b1370 — not stale.

Review agents run

• pr-review-toolkit:code-reviewer — ran. Confirmed early-ACK dependency/ordering correctness, watchdog import-time/xdist safety, and the 300s→120s comment error; judged 600s correctly not an AI007 constants.json value (Python-test-infra-only, no cross-runtime consumer).
• pr-review-toolkit:silent-failure-hunter — ran (error-handling/ACK/reaping in scope). Confirmed the token resolvers are fail-open by design (never raise; return "" with justified nosemgrep allowlists), so the early-ACK move hides no abort-worthy failure; flagged the watchdog os._exit window (Finding 2).
• pr-review-toolkit:pr-test-analyzer — ran (this PR IS about tests). Independently reproduced the false-guard finding empirically and supplied the validated fix; confirmed the missing early-ACK ordering test.
• type-design-analyzer — omitted: no new types introduced.
• comment-analyzer — folded into code-reviewer (comment accuracy/rot covered above); not separately spawned.
• /security-review — omitted: no IAM, Cedar, network, secrets, or input-gateway change (test-harness + pipeline reorder only).

Human heuristics

• Proportionality — Pass. Fix scope matches the problem; no over-abstraction. Comments are verbose/triplicated (nit #5) but not structurally disproportionate.
• Coherence — Pass. Lands in agent/ per the routing table; same terminology as the existing session-scoping code.
• Clarity — Concern. conftest.py:32 states a wrong per-test cap (nit #3) in load-bearing rationale; otherwise names/intent are clear.
• Appropriateness — Concern (blocking). The regression guard asserts what a local fixture already guarantees rather than what the fix does (AI005) — test_aws_session.py:96-101; see blocking issue #1.

_{🤖 Generated with Claude Code}

[scottschreckengaust]

Blocking (AI005 — false regression guard). This asserts the conftest _clean_env fixture scrubbed AGENT_SESSION_ROLE_ARN, but this module has its own file-local autouse fixture _reset (lines 24-30) that already delenvs the same var. I verified empirically: removing the "AGENT_SESSION_ROLE_ARN" line from conftest.py's _AGENT_ENV_VARS (with the var set in the parent env) leaves this test passing — the local _reset masks it. So this guard cannot detect a regression of the exact fix this PR adds. On a PR whose whole purpose is that guard, this is the blocker.

Move both assertions into a fixture-free module so only the autouse conftest _clean_env is in play (validated as a true bidirectional guard). Note monkeypatch.setenv in the body won't work — autouse fixtures run during setup, before the body.

Suggested change

	assert os.environ.get(SESSION_ROLE_ARN_ENV) is None
	# NOTE: this module's own ``_reset`` autouse fixture (above) also delenv's
	# SESSION_ROLE_ARN_ENV, so this assertion here does NOT actually exercise the
	# conftest ``_clean_env`` scrub — move this guard to a fixture-free module
	# (e.g. tests/test_conftest_env_scrub.py) that relies solely on the autouse
	# conftest fixture, so removing the conftest scrub line makes it fail.
	assert os.environ.get(SESSION_ROLE_ARN_ENV) is None

[scottschreckengaust]

Nit (safety window). _reap_on_hang calls os._exit(1) unconditionally. daemon=True stops the timer from blocking shutdown, but not from firing if a slow-but-passing suite lands near 600s (e.g. mid coverage-write in teardown), turning a green run red with a thread-dump uncorrelated to any failed test. Cancel on session finish so it only fires on a true hang:

Suggested change

	_hang_watchdog.start()
	_hang_watchdog.start()
	def pytest_sessionfinish(session, exitstatus): # noqa: ARG001
	# Close the window where a slow-but-successful run could trip the 600s reaper
	# during teardown/coverage-write. cancel() is a no-op if it already fired.
	_hang_watchdog.cancel()

[scottschreckengaust]

Nit (factual error in load-bearing comment). "per-test cap is 300s" is wrong — the actual pytest-timeout config is timeout = 120 (agent/pyproject.toml:136); no 300s override exists. This is the justification for the 600s deadline, so keep it truthful.

Suggested change

	# cap is 300s) yet well under the build ceiling.
	# ceiling. Deadline 600s: far above the whole suite's normal runtime (per-test
	# cap is 120s) yet well under the build ceiling.