fix: review round

[pedronauck]

Summary by CodeRabbit

• New Features

  • Support bundle creation now requires explicit consent and can include an active config snapshot.
  • Sessions detect stalled state, record recovered/timeout markers, and block attachment when stalled.

• Improvements

  • Session recap includes pending input counts and queue generation.
  • CLI/web support-bundle flows now pass explicit consent (yes) when creating bundles.
  • Better detection/classification of MCP-auth failures and related transcript markers.

• API Changes

  • Scheduler status payload no longer includes drain-related fields.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agh-site	Ready	Preview, Comment	May 22, 2026 9:20pm

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 08edef1e-f0b0-4c1c-9028-80403a70e510

📥 Commits

Reviewing files that changed from the base of the PR and between 45eb577 and e5f3bf3.

⛔ Files ignored due to path filters (13)

• CLAUDE.md is excluded by !**/*.md
• skills/agh/SKILL.md is excluded by !**/*.md
• skills/agh/references/agent-definitions.md is excluded by !**/*.md
• skills/agh/references/capabilities-and-bundles.md is excluded by !**/*.md
• skills/agh/references/contributing-to-agh.md is excluded by !**/*.md
• skills/agh/references/docs-design-and-copy.md is excluded by !**/*.md
• skills/agh/references/memory.md is excluded by !**/*.md
• skills/agh/references/native-tools.md is excluded by !**/*.md
• skills/agh/references/network.md is excluded by !**/*.md
• skills/agh/references/qa-and-verification.md is excluded by !**/*.md
• skills/agh/references/runtime-operations.md is excluded by !**/*.md
• skills/agh/references/tasks-and-orchestration.md is excluded by !**/*.md
• skills/agh/references/tools-and-skills.md is excluded by !**/*.md

📒 Files selected for processing (6)

• internal/acp/handlers_test.go
• internal/api/core/handlers.go
• internal/daemon/settings_runtime_applier_test.go
• internal/store/globaldb/global_db_session.go
• internal/store/globaldb/global_db_session_input_queue.go
• internal/task/scheduler_controls_test.go

---

Walkthrough

This PR preserves structured ACP RequestError payloads in emitted events, detects MCP-auth reason codes in agent Raw JSON to classify transcript markers, adds durable input-queue summaries and recap counts, requires explicit consent for support bundles across CLI/API/UI, wires ConfigSnapshot into bundles and daemon boot, invalidates provider prestart cache after config apply, refactors scheduler status schema and controls, and updates store, extension, event, and web tests.

Changes

Core Feature Implementation

Layer / File(s)	Summary
ACP error preservation and tool error mapping internal/acp/client.go, internal/acp/failure.go, internal/acp/handlers.go, internal/acp/client_test.go, internal/acp/handlers_test.go	Emit raw RequestError JSON via requestErrorRaw() into AgentEvent.Raw; map toolspkg.ToolError/validation errors into structured request data; tests validate MCP auth reason code preservation.
Session stall badges and attachability internal/session/badge.go, internal/api/core/conversions_parsers_test.go	Add BadgeInputs.Stalled, compute BadgeHung when stalled, and block attachments for detected stalls; tests assert hung badge and non-attachable state.
MCP auth detection in transcript markers & prompt activity internal/session/manager_prompt.go, internal/session/manager_prompt_lifetime_test.go, internal/session/prompt_activity.go, internal/session/prompt_activity_test.go	Inspect AgentEvent.Raw JSON for reason_codes up to a depth limit to classify MCP-auth-required markers; emit recovered/prompt-timeout/unhealthy markers with stall-reason evidence; tests for classification, depth limit, persistence, and deduplication.
Input queue summary and recap wiring internal/session/query.go, internal/store/session_input_queue.go, internal/store/globaldb/global_db_session_input_queue.go, internal/api/core/interfaces.go, internal/api/core/handlers.go, internal/api/core/handlers_test.go, internal/api/testutil/session_stub.go	Introduce SessionInputQueueSummary and Manager/Store methods to fetch generation and pending counts; GlobalDB implements summary query; recap now includes PendingInputs, PendingMarkers, and uses queue generation/consistency.
Support bundle consent: CLI, API, UI internal/cli/support.go, internal/cli/support_test.go, internal/api/core/support.go, internal/api/core/support_test.go, internal/api/spec/spec.go, internal/api/contract/support.go, web/src/routes/_app/settings/observability.tsx, web/src/routes/_app/settings/__tests__/-observability.test.tsx, web/src/systems/support/hooks/use-support-bundle-download.ts	Refactor CLI confirm to return (bool, error) and require interactive stdin unless --yes; API requires request.Yes and returns structured consent diagnostic when missing; OpenAPI marks body required; UI hook and tests updated to pass yes:true.
Support bundle config snapshot and daemon integration internal/support/service.go, internal/support/service_test.go, internal/settings/config_apply_service.go, internal/settings/config_apply_service_test.go, internal/settings/models.go, internal/daemon/boot.go	Add ConfigSnapshotFunc and Builder.ConfigSnapshot; daemon boot provides active config snapshot when settings service exposes ActiveConfig; tests ensure snapshot inclusion and ActiveConfig reflect applies.
Daemon provider cache & native tools workspace scope internal/daemon/boot.go, internal/daemon/native_tools.go, internal/daemon/native_tools_test.go, internal/daemon/native_bundle_resource_tools.go, internal/daemon/native_extension_tools.go, internal/daemon/settings_runtime_applier_test.go, internal/daemon/daemon_test.go	Call providers.InvalidatePreStartCache() after successful config apply; thread req.ToolID into native tool workspace resolution and reject foreign workspace inputs; replace string-based marketplace errors with sentinel errors and errors.Is classification; tests added/updated.
Scheduler status schema & controls internal/task/types.go, internal/api/contract/scheduler.go, internal/api/core/scheduler.go, internal/task/scheduler_controls.go, internal/task/scheduler_controls_test.go, internal/task/inspect_test.go	Remove drain fields from scheduler payload, add AsOf and counters; PauseTask rejects terminal tasks with structured diagnostic; DrainScheduler uses detached timeout context; new tests exercise pause rejection and drain flow.
Store/globaldb: attach lock sweep and stall filtering internal/store/globaldb/global_db_session.go, internal/store/globaldb/global_db_session_test.go	Add SweepExpiredSessionAttachLocks and run it before listing/attach attempts; tighten attach acquisition to exclude stalled sessions; classify stalled sessions as not attachable; tests added.
Events, extension lifecycle, and tool event changes internal/events/registry.go, internal/events/registry_test.go, internal/extension/marketplace_lifecycle.go, internal/daemon/tool_events.go, internal/daemon/tool_events_test.go, internal/notifications/presets/match_test.go	Mark TaskRunOperatorRetry notification-eligible; introduce ErrMarketplaceSourceUnavailable and wrap loader errors; EmitToolEvent stops populating Provider; tests updated to match new metadata and stored-event expectations.
Web: hook rename, fixtures, and landing UI web/src/systems/status/hooks/use-status.ts, web/src/systems/status/index.ts, web/src/systems/workspace/hooks/use-workspace-setup-content.ts, web/src/systems/workspace/components/__tests__/workspace-setup.test.tsx, web/src/systems/scheduler/mocks/fixtures.ts, packages/site/components/landing/supported-agents.tsx, packages/site/components/landing/__tests__/landing.test.tsx	Rename useDaemonStatus -> useStatus and update imports/mocks; workspace setup and tests use new hook; scheduler fixtures drop drain_in_progress; landing component replaced with provider quilt UI and tests updated to use within.

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs:

• compozy/agh#130: Related changes to ACP request-error handling and classification.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

internal/task/scheduler_controls_test.go (1)

15-26: ⚡ Quick win

Add compile-time interface assertions for schedulerControlTestStore.

This test double is used across pause, scheduler control, and event summary paths; explicit assertions will catch interface drift at compile-time.

Suggested patch

 type schedulerControlTestStore struct {
 	*inMemoryManagerStore
 	pause            SchedulerPauseState
 	activeClaimCount []int
 	summaries        []storepkg.EventSummary
 	cancelAfterPause context.CancelFunc
 	requireDeadline  bool
 }
 
+var _ taskPauseStore = (*schedulerControlTestStore)(nil)
+var _ schedulerControlStore = (*schedulerControlTestStore)(nil)
+var _ eventSummaryStore = (*schedulerControlTestStore)(nil)
+
 func newSchedulerControlTestStore() *schedulerControlTestStore {
 	return &schedulerControlTestStore{inMemoryManagerStore: newInMemoryManagerStore()}
 }

As per coding guidelines: "Use compile-time interface assertions (var _ InterfaceName = (*ConcreteType)(nil)) to verify interface satisfaction."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/task/scheduler_controls_test.go` around lines 15 - 26, Add
compile-time interface assertions for the test double by adding lines of the
form `var _ InterfaceName = (*schedulerControlTestStore)(nil)` for each
interface schedulerControlTestStore is expected to implement (e.g., the
manager/store, pause/control, and event summary interfaces used by tests); place
these assertions directly after the `type schedulerControlTestStore` block or
after `newSchedulerControlTestStore()` and update imports if necessary so the
compiler will catch any interface drift between `schedulerControlTestStore` and
the interfaces it must satisfy.

internal/acp/handlers_test.go (1)

1224-1247: ⚡ Quick win

Wrap the new MCP-auth assertion flow in a t.Run("Should...") subtest.

This keeps the added case compliant with the required test-case structure and isolates failures cleanly.

♻️ Proposed change

-	toolReqErr := requestError(toolspkg.NewToolError(
-		toolspkg.ErrorCodeUnavailable,
-		toolspkg.ToolID("mcp/github/search"),
-		"MCP login required",
-		errors.New("auth unavailable"),
-		toolspkg.ReasonMCPAuthRequired,
-	))
-	var reasonData struct {
-		ReasonCodes []string `json:"reason_codes"`
-	}
-	encoded, err := json.Marshal(toolReqErr.Data)
-	if err != nil {
-		t.Fatalf("json.Marshal(tool request error data) error = %v", err)
-	}
-	if err := json.Unmarshal(encoded, &reasonData); err != nil {
-		t.Fatalf("json.Unmarshal(tool request error data) error = %v", err)
-	}
-	if !slices.Contains(reasonData.ReasonCodes, string(toolspkg.ReasonMCPAuthRequired)) {
-		t.Fatalf(
-			"tool request error reason codes = %#v, want %q",
-			reasonData.ReasonCodes,
-			toolspkg.ReasonMCPAuthRequired,
-		)
-	}
+	t.Run("Should include mcp_auth_required reason code in tool request error data", func(t *testing.T) {
+		toolReqErr := requestError(toolspkg.NewToolError(
+			toolspkg.ErrorCodeUnavailable,
+			toolspkg.ToolID("mcp/github/search"),
+			"MCP login required",
+			errors.New("auth unavailable"),
+			toolspkg.ReasonMCPAuthRequired,
+		))
+		var reasonData struct {
+			ReasonCodes []string `json:"reason_codes"`
+		}
+		encoded, err := json.Marshal(toolReqErr.Data)
+		if err != nil {
+			t.Fatalf("json.Marshal(tool request error data) error = %v", err)
+		}
+		if err := json.Unmarshal(encoded, &reasonData); err != nil {
+			t.Fatalf("json.Unmarshal(tool request error data) error = %v", err)
+		}
+		if !slices.Contains(reasonData.ReasonCodes, string(toolspkg.ReasonMCPAuthRequired)) {
+			t.Fatalf(
+				"tool request error reason codes = %#v, want %q",
+				reasonData.ReasonCodes,
+				toolspkg.ReasonMCPAuthRequired,
+			)
+		}
+	})

As per coding guidelines: `**/*_test.go`: `MUST use t.Run("Should...") pattern for ALL test cases`.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/handlers_test.go` around lines 1224 - 1247, Wrap the MCP-auth
assertion block in a t.Run subtest using the "Should..." naming pattern: create
t.Run("Should include MCP auth reason code in tool request error", func(t
*testing.T) { ... }) and move the existing logic (construction of toolReqErr,
json.Marshal/json.Unmarshal into reasonData, and the slices.Contains assertion)
inside that closure; ensure you import testing.T in the closure signature and
keep existing variable names (toolReqErr, reasonData) unchanged so the assertion
for toolspkg.ReasonMCPAuthRequired executes as an isolated subtest.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/core/handlers.go`:
- Around line 734-737: The error handling after calling
pendingTranscriptMarkerCount currently returns a hardcoded 500; change it to map
session-domain errors through the existing StatusForSessionError helper used by
the queue-summary path: call StatusForSessionError(err) to get the appropriate
HTTP status and pass that (instead of http.StatusInternalServerError) into
h.respondError alongside the original err so session-removed / session-related
errors surface the correct status. Use the same pattern as the queue-summary
handling to keep behavior consistent.

In `@internal/daemon/settings_runtime_applier_test.go`:
- Around line 32-33: The test currently discards the return values of
providers.PreStart (seen in repeated calls around
providers.PreStart(t.Context(), provider, env) and the later call at line 47),
which allows the test to pass based solely on call count; update the assertions
to explicitly check the returned error/value from providers.PreStart instead of
assigning to `_`—for example, capture the return into a variable and use the
existing test assertion helper (or require/Assert) to assert it is nil or
matches the expected error for each call so regressions in PreStart behavior
will fail the test.
- Around line 12-18: The test "Should invalidate provider prestart cache after
active config apply" mutates global state via providers.InvalidatePreStartCache
but is marked t.Parallel(), causing flakiness; remove the t.Parallel() call(s)
inside this specific subtest (and the outer test's t.Parallel() if it runs the
subtest concurrently) so the subtest runs serially, leaving
providers.InvalidatePreStartCache and the
t.Cleanup(providers.InvalidatePreStartCache) intact and ensuring calls
assertions are not interfered with by other parallel tests.

In `@internal/store/globaldb/global_db_session_input_queue.go`:
- Around line 566-568: The defer currently discards rows.Close() with `_ =
rows.Close()`; change the defer func() that references rows.Close() to capture
and handle the error instead: call closeErr := rows.Close() and if closeErr !=
nil then either attach/wrap it into the function's returned error (if the
function uses a named error return, merge/wrap into `err`) or log/return the
closeErr (e.g., wrap with fmt.Errorf("rows.Close: %w", closeErr)) so the DB
cleanup error is not ignored; update the defer closure around rows.Close()
accordingly.

In `@internal/store/globaldb/global_db_session.go`:
- Around line 268-271: The attach-failure classification is inconsistent: the
attach eligibility check blocks attach when strings.TrimSpace(stallState.String)
is any non-empty value, but the error classification only treats stall_state ==
store.SessionStallStateDetected as non-attachable. Change the classification
condition that returns store.ErrSessionNotAttachable (the if using state,
failureKind.String and stallState.String) to treat any non-empty
strings.TrimSpace(stallState.String) as non-attachable (i.e., replace the
comparison to SessionStallStateDetected with a non-empty check using
strings.TrimSpace) so the error reporting matches the attach predicate; keep the
existing use of strings.TrimSpace on state and failureKind.String and include
sessionID in the returned error as before.

---

Nitpick comments:
In `@internal/acp/handlers_test.go`:
- Around line 1224-1247: Wrap the MCP-auth assertion block in a t.Run subtest
using the "Should..." naming pattern: create t.Run("Should include MCP auth
reason code in tool request error", func(t *testing.T) { ... }) and move the
existing logic (construction of toolReqErr, json.Marshal/json.Unmarshal into
reasonData, and the slices.Contains assertion) inside that closure; ensure you
import testing.T in the closure signature and keep existing variable names
(toolReqErr, reasonData) unchanged so the assertion for
toolspkg.ReasonMCPAuthRequired executes as an isolated subtest.

In `@internal/task/scheduler_controls_test.go`:
- Around line 15-26: Add compile-time interface assertions for the test double
by adding lines of the form `var _ InterfaceName =
(*schedulerControlTestStore)(nil)` for each interface schedulerControlTestStore
is expected to implement (e.g., the manager/store, pause/control, and event
summary interfaces used by tests); place these assertions directly after the
`type schedulerControlTestStore` block or after `newSchedulerControlTestStore()`
and update imports if necessary so the compiler will catch any interface drift
between `schedulerControlTestStore` and the interfaces it must satisfy.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 679080a8-b0d2-469d-a5ca-925d009749b3

📥 Commits

Reviewing files that changed from the base of the PR and between d1b0b40 and 45eb577.

⛔ Files ignored due to path filters (11)

• openapi/agh.json is excluded by !**/*.json
• packages/site/content/runtime/api-reference/index.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/cli-reference/scheduler/drain.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/autonomy/notification-cursors.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/autonomy/notification-presets.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/autonomy/task-runs-and-leases.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/bridges/index.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/bridges/routing.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/resources/bundles.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/sessions/resume.mdx is excluded by !**/*.mdx
• web/src/generated/agh-openapi.d.ts is excluded by !**/generated/**

📒 Files selected for processing (65)

• internal/acp/client.go
• internal/acp/client_test.go
• internal/acp/failure.go
• internal/acp/handlers.go
• internal/acp/handlers_test.go
• internal/api/contract/scheduler.go
• internal/api/contract/support.go
• internal/api/core/conversions_parsers_test.go
• internal/api/core/handlers.go
• internal/api/core/handlers_test.go
• internal/api/core/interfaces.go
• internal/api/core/scheduler.go
• internal/api/core/session_manager_stub_test.go
• internal/api/core/support.go
• internal/api/core/support_test.go
• internal/api/spec/spec.go
• internal/api/spec/spec_test.go
• internal/api/testutil/session_stub.go
• internal/cli/support.go
• internal/cli/support_test.go
• internal/daemon/boot.go
• internal/daemon/daemon_test.go
• internal/daemon/native_bundle_resource_tools.go
• internal/daemon/native_extension_tools.go
• internal/daemon/native_tools.go
• internal/daemon/native_tools_test.go
• internal/daemon/settings_runtime_applier_test.go
• internal/daemon/tool_events.go
• internal/daemon/tool_events_test.go
• internal/events/registry.go
• internal/events/registry_test.go
• internal/extension/marketplace_lifecycle.go
• internal/notifications/presets/match_test.go
• internal/session/badge.go
• internal/session/manager_prompt.go
• internal/session/manager_prompt_lifetime_test.go
• internal/session/prompt_activity.go
• internal/session/prompt_activity_test.go
• internal/session/query.go
• internal/session/session.go
• internal/settings/config_apply_service.go
• internal/settings/config_apply_service_test.go
• internal/settings/models.go
• internal/store/globaldb/global_db_session.go
• internal/store/globaldb/global_db_session_input_queue.go
• internal/store/globaldb/global_db_session_input_queue_test.go
• internal/store/globaldb/global_db_session_test.go
• internal/store/session_input_queue.go
• internal/support/service.go
• internal/support/service_test.go
• internal/task/inspect_test.go
• internal/task/scheduler_controls.go
• internal/task/scheduler_controls_test.go
• internal/task/types.go
• packages/site/components/landing/__tests__/landing.test.tsx
• packages/site/components/landing/supported-agents.tsx
• web/src/routes/_app/__tests__/-tasks.test.tsx
• web/src/routes/_app/settings/__tests__/-observability.test.tsx
• web/src/routes/_app/settings/observability.tsx
• web/src/systems/scheduler/mocks/fixtures.ts
• web/src/systems/status/hooks/use-status.ts
• web/src/systems/status/index.ts
• web/src/systems/support/hooks/use-support-bundle-download.ts
• web/src/systems/workspace/components/__tests__/workspace-setup.test.tsx
• web/src/systems/workspace/hooks/use-workspace-setup-content.ts

💤 Files with no reviewable changes (6)

• web/src/systems/scheduler/mocks/fixtures.ts
• internal/daemon/tool_events.go
• web/src/routes/_app/tests/-tasks.test.tsx
• internal/api/core/scheduler.go
• internal/api/contract/scheduler.go
• internal/task/types.go