feat(#929): agent cap enforces schedule ceiling + clearer SIGKILL msg

[dolho]

Summary

Approach A from #929 — write-time validation makes agent_ownership.execution_timeout_seconds a hard ceiling for agent_schedules.timeout_seconds. Plus the orthogonal SIGKILL error-message cleanup the issue called for regardless.

The reporter's mental model was right; the code wasn't matching it

Exploration found task_execution_service does not compute min(agent, schedule) (issue's stated root cause was wrong). What actually happens today:

• Scheduler path: schedule.timeout_seconds is passed straight through, agent cap is ignored.
• Chat path: db.get_execution_timeout(agent_name) is used; no schedule involved.

So the two settings coexisted as independent knobs with no enforcement between them — schedules silently won. The reporter's min() assumption was a reasonable read of the UI, but it never held in code. Approach A snaps the implementation to match the mental model: schedule timeout can't exceed the agent cap, and the cap can't drop below active schedules. Both fail at config time with a 400, not at SIGKILL after waiting 60 minutes.

Changes

Validation (the meat):

• POST /api/agents/{name}/schedules — 400 error=schedule_timeout_exceeds_agent_cap when the create would breach the cap.
• PUT /api/agents/{name}/schedules/{id} — same 400, only when the PUT actually touches timeout_seconds (pre-existing rows that predate the validation stay editable).
• PUT /api/agents/{name}/timeout — 400 error=agent_timeout_below_active_schedules when the new cap would drop below any non-deleted schedule's timeout_seconds. Response includes the list of blocking schedules so the operator knows what to edit first.

DB:

• db.get_max_active_schedule_timeout(agent_name) — MAX across non-soft-deleted rows; None if empty.
• db.find_active_schedules_exceeding_timeout(agent_name, ceiling) — thin offender list {id, name, timeout_seconds} for the 400 payload.

Orthogonal (same PR per issue's "do regardless"):

• agent_server/services/error_classifier.py — SIGKILL message drops the misleading "schedule/agent timeout exceeded" disjunction. Now that the agent cap can never silently truncate a schedule, "schedule timeout exceeded" is unambiguous. Remediation hint links the schedule's timeout_seconds to the agent cap explicitly.

No retro-validation: rows that predate this PR and already violate the invariant are left alone. The agent-cap-lowering check still sees them, so the operator can't widen the gap.

Approach B (effective-timeout surface) is intentionally left out: under A, timeout_effective == timeout_configured always (no further cap layer), making the new field pure clutter. If we ever add a second cap layer, B is additive.

Tests

tests/unit/test_929_timeout_validation.py — 7 tests:

• DB accessors against ephemeral SQLite — MAX, soft-delete exclusion, empty agent, offender list ordering.
• Router helper _enforce_timeout_below_agent_cap — at-cap + under-cap accept silently, over-cap raises 400 with the structured detail dict.

Router helper loaded via importlib.spec_from_file_location (the same dodge test_voice_auth.py and test_monitoring_router_signatures.py use) so the test doesn't pull in routers/__init__.py's 50+ siblings.

Local results: pytest tests/unit/test_929_timeout_validation.py → 7 passed. Schedule/timeout-tagged subset under broader load → 68 passed, 1 skipped (no regressions).

Test plan

• Unit tests pass locally
• CI green
• Manual: create schedule with timeout_seconds=7200 against agent cap 3600 → 400
• Manual: PUT agent timeout=60 while a schedule has timeout=1800 → 400 with blocking_schedules
• Manual: SIGKILL on long-running schedule emits the cleaned-up message
• Base-image rebuild for the error_classifier change (./scripts/deploy/build-base-image.sh)

Files

• src/backend/routers/schedules.py (+36) — validation helper + applied to POST/PUT
• src/backend/routers/agent_config.py (+24) — agent-timeout PUT validation
• src/backend/db/schedules.py (+43) — two new accessors
• src/backend/database.py (+8) — facade exposure
• docker/base-image/agent_server/services/error_classifier.py (~5 lines) — SIGKILL message cleanup
• docs/memory/requirements.md (+63) — §35 "Schedule Timeout Validation"
• docs/memory/architecture.md (3 cells) — 400 response notes
• tests/unit/test_929_timeout_validation.py (new, 7 tests)

Related to #929.

🤖 Generated with Claude Code

[dolho]

/review Report — Self-Review

Branch: feature/929-effective-timeout-surface → dev
Files Changed: 8 (+481/-6)
Scope: CLEAN — issue #929 calls for validation + SIGKILL message cleanup; diff delivers exactly that.

Critical Findings (block merge)

None.

Informational Findings (review required)

[I1] Dead code: get_max_active_schedule_timeout is unused

• File: src/backend/db/schedules.py:313, src/backend/database.py:703
• Issue: Added the accessor + facade method, but no production caller invokes it. routers/agent_config.py:set_agent_timeout uses find_active_schedules_exceeding_timeout (the right call — it surfaces the offender list directly).
• Suggestion: Delete get_max_active_schedule_timeout + its 3 tests, OR document it as a public utility. Per CLAUDE.md "no abstractions beyond what the task requires," delete is the minimal-scope call.

[I2] Requirements doc inconsistency with code

• File: docs/memory/requirements.md:2394-2398 (§35.1)
• Issue: Says `get_max_active_schedule_timeout` is "Used by the agent-timeout endpoint only" — but the endpoint actually uses find_active_schedules_exceeding_timeout. The MAX accessor isn't wired up.
• Fix: If I1 is resolved by deletion, drop the bullet entirely. Otherwise correct the sentence to name the actually-called function.

[I3] Frontend renders dict-detail as [object Object] for the new 400 case

• File: src/frontend/src/components/SchedulesPanel.vue (save handler)
• Issue: Existing pattern is `formError.value = error.response?.data?.detail || 'Failed to save schedule'`. With my structured `detail={error, message, agent_cap_seconds, requested_seconds}`, the form will render the literal [object Object] instead of the human-readable message.
• Note: This is a pre-existing frontend laggard — routers/chat.py, routers/sessions.py, routers/systems.py already return dict detail and have the same issue. Not strictly a regression caused by this PR, but the PR is the first place a schedule form will hit it.
• Suggestion: Either (a) follow-up PR to teach SchedulesPanel.vue to render detail.message ?? detail; or (b) include a small frontend fix here (1-line `error.response?.data?.detail?.message || error.response?.data?.detail`).

[I4] TOCTOU race between agent-cap lowering and concurrent schedule create

• File: src/backend/routers/agent_config.py:512-534
• Issue: Sequence T1 reads offenders (0 found), T2 another caller creates a schedule at the current-cap value, T3 we write the lower cap → row exists with timeout_seconds > new_cap. Symmetric race exists on the schedule-create side (cap drops between read & write).
• In practice: SQLite single-writer serializes the actual DB ops; window is small; "next edit fixes it" is documented. Acceptable for this PR.
• Suggestion: Document the race window in a code comment, or wrap both check + write in BEGIN IMMEDIATE if we want it tight. Leave for follow-up unless someone has a concurrency story.

[I5] No automated test pins the cleaned SIGKILL error message

• File: docker/base-image/agent_server/services/error_classifier.py:228-240
• Issue: Message change verified manually inside container (good), but no unit test asserts the new string. A future edit could silently regress the phrasing.
• Suggestion: Add a 5-line test against `_classify_signal_exit(-9, ...)` asserting the returned string contains "schedule timeout exceeded" and does NOT contain "schedule/agent". Could land here or as a follow-up.

[I6] PUT /timeout 400 path tested at DB layer only

• File: tests/unit/test_929_timeout_validation.py
• Issue: find_active_schedules_exceeding_timeout is unit-tested. The router's structured 400 detail dict shape (error="agent_timeout_below_active_schedules", blocking_schedules list) is verified live but not asserted by a unit test. Symmetric to _enforce_timeout_below_agent_cap which IS unit-tested.
• Suggestion: Mirror test_enforce_helper_rejects_above_cap_with_structured_detail — load routers.agent_config via importlib, patch db.find_active_schedules_exceeding_timeout, assert the HTTPException detail keys.

Clean Categories

• SQL safety: all DB accessors use parameterized queries (? placeholders); no string interpolation. ✓
• Auth boundaries: validations sit after get_current_user + can_user_share_agent/AuthorizedAgent gates. ✓
• Credential exposure: no credentials touched. ✓
• MCP server sync (Invariant feat: SMARTS trading pipeline with Telegram notifications and Miro visualization #13): MCP create_agent_schedule / update_agent_schedule proxy through the backend HTTP API, so the new 400 surfaces uniformly without TS changes. ✓
• Architectural invariants (CLAUDE.md):
  • Fix: Add missing Docker labels to system agent container #1 Router → Service → DB: ✓ (helpers in router; SQL in db/)
  • Feature/gemini runtime support #2 Class-per-domain DB: ✓ (added to ScheduleOperations)
  • security: implement safe tar extraction with symlink/hardlink validation #8 Auth pattern preserved: ✓
  • Add internal health route, without which main didn't start #14 Pydantic centralized: ✓ (no new models; structured dict in HTTPException is the established pattern in chat/sessions/systems routers)
• Magic numbers: no new ones; 60-7200 bounds reused from existing feat: increase default chat execution timeout from 15m to 60m #665 setting. ✓
• Conditional side effects: validation raises before any state write. ✓
• WebSocket / broadcast: unaffected. ✓
• Backward compat: no migration; pre-existing rows allowed to violate invariant; exclude_unset on PUT means timeout-untouched updates don't re-validate. ✓

Summary

Category	Count	Action
Critical	0	Clear to merge on correctness
Informational	6	Review before merging
Scope	CLEAN	—

Recommended fixes for this PR (minimum-friction set)

The two with the most signal-per-line:

• I1 + I2: delete get_max_active_schedule_timeout + its 3 tests + the bullet in requirements.md (dead code today, requirements doc lies about it)
• I5: add a one-screen unit test pinning the SIGKILL error_classifier message

I3 (frontend) and I6 (agent-timeout router test) are reasonable follow-up issues.
I4 (race) deserves a code comment but no behavioral fix.

🤖 Generated by /review skill (.claude/skills/review)

[dolho]

/review Report — Pass 2 (after f1ba610f)

Branch: feature/929-effective-timeout-surface → dev
Files Changed: 8 (+482/-6) | Commits: 3 (latest: f1ba610f)
Scope: CLEAN — fix commit stayed within #929's surface (DB layer, requirements doc, tests).

Delta since Pass 1

Pass 1 finding	Status
I1 dead get_max_active_schedule_timeout	✅ Resolved — accessor + facade + 3 tests deleted
I2 requirements doc names wrong accessor	✅ Resolved — bullet rewritten to describe find_active_schedules_exceeding_timeout
I5 no test pins SIGKILL message	✅ Resolved — 2 tests added (test_sigkill_message_drops_schedule_agent_disjunction, test_sigkill_message_handles_sigterm_and_shell_encoded_signals); covers SIGKILL/SIGTERM + shell-encoded 137/143
I3 frontend dict-detail render	⏭️ Deferred to follow-up
I4 TOCTOU race	⏭️ Acceptable (documented limitation)
I6 PUT /timeout router unit test	⏭️ Deferred to follow-up

Critical Findings (block merge)

None.

Informational Findings (new on this pass)

None.

Verifications Run

• `pytest tests/unit/test_929_timeout_validation.py` → 6 passed (3 DB + 2 router-helper + 2 SIGKILL — minus 3 deleted MAX-accessor tests)
• Sibling-load: `pytest -k "929 or 904 or error_classifier"` → 37 passed, no regressions (cross-load with test_904_* which patches db globally)
• `python tests/lint_sys_modules.py` → 230 / 237 baseline (no new sys.modules violations)
• `gh pr checks 930` → all green (lint sys.modules, pytest base + head × 3 seeds, schema-parity, verify-non-root, CodeQL, Analyze)

Clean Categories (re-verified)

• SQL safety: find_active_schedules_exceeding_timeout parameterized.
• Auth boundaries: validation helpers fire after get_current_user / AuthorizedAgent gates.
• Architectural invariants (CLAUDE.md): Fix: Add missing Docker labels to system agent container #1 (Router→Service→DB), Feature/gemini runtime support #2 (class-per-domain), security: implement safe tar extraction with symlink/hardlink validation #8 (auth pattern), Add internal health route, without which main didn't start #14 (Pydantic centralized), Security: Agent Credential Leakage in Execution Logs [FIXED] #15 (URL nesting) all preserved.
• MCP server sync (Invariant feat: SMARTS trading pipeline with Telegram notifications and Miro visualization #13): MCP create_agent_schedule / update_agent_schedule proxy through backend HTTP — 400 propagates without TS changes.
• No new credential exposure / WebSocket / Docker / migration concerns.

Note on POST /schedules ordering

create_schedule runs cron-validation + timeout-validation BEFORE the access check (the access check happens implicitly inside db.create_schedule). Considered flagging this as an info leak (probing timeouts could reveal an agent's cap to non-owners) — but GET /api/agents/{name}/timeout already returns the cap to any authenticated user without an ownership check, so no new leak. Not a finding.

Summary

Category	Count	Action
Critical	0	Clear to merge
Informational (new)	0	—
Informational (deferred)	3	I3, I4, I6 documented as follow-up
Scope	CLEAN	—

PR is ready to merge from a structural-review standpoint. Outstanding deferred items (I3/I4/I6) are acknowledged.

🤖 Generated by `/review` skill (`.claude/skills/review`)

[vybe]

Clean, focused implementation. Write-time enforcement is the right model — fail at config time rather than SIGKILL at runtime. DB accessor, router helper, and SIGKILL classifier all unit-tested with good boundary coverage. Two deferred items (I3 frontend dict-detail rendering, I6 agent-timeout router test) are low-risk and explicitly documented. Address requirements §35 status (🚧 → ✅) and frontend rendering check before next release cut.