Add automation run history UI

[henrypark133]

Summary

• persist bounded automation trigger run history across in-memory, libSQL, and PostgreSQL trigger repositories
• expose recent runs through builtin.trigger_list, product workflow, and WebUI v2 /automations
• redesign the Automations page with summary metrics, filters, a detail panel, recent run rows, and chat links

Tests

• cargo test -p ironclaw_reborn_composition automation_facade
• cargo test -p ironclaw_product_workflow --test reborn_services_contract list_automations
• cargo test -p ironclaw_host_runtime builtin_trigger_list
• node --test crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.test.mjs

Notes

• Deferred follow-up: component-level render/navigation harness for the Automations detail panel; presenter/API/handler coverage exists.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d105df4904

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

This PR adds bounded per-trigger automation run history to the triggers subsystem and threads it through the runtime (builtin.trigger_list) and product workflow to power a redesigned WebUI v2 Automations page with recent run visibility, filtering, and a detail panel.

Changes:

• Persist and query bounded trigger run-history (in-memory, libSQL, PostgreSQL), including batch retrieval and retention pruning.
• Extend automation list projections end-to-end to include recent_runs, plus a run_limit request/query parameter with clamping behavior.
• Redesign WebUI v2 Automations page to show summary metrics, new filters (running/failures), recent-run indicators, and a run detail panel with chat links.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
docs/reborn/contracts/triggers.md	Documents V1 bounded trigger run history semantics and API projections.
crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs	Updates handler contract tests for run_limit and recent_runs output.
crates/ironclaw_webui_v2/src/handlers.rs	Adds run_limit query param forwarding to product workflow.
crates/ironclaw_webui_v2/CLAUDE.md	Updates WebUI v2 route contract documentation for run_limit.
crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.test.mjs	Adds presenter coverage for recent-run normalization, summary, and filtering.
crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js	Implements recent-run normalization, run status presentation, and new summary/filter logic.
crates/ironclaw_webui_v2_static/static/js/pages/automations/hooks/useAutomations.js	Requests automations with runLimit and periodic refresh.
crates/ironclaw_webui_v2_static/static/js/pages/automations/components/automations-summary-strip.js	Updates summary strip to show running/failures metrics.
crates/ironclaw_webui_v2_static/static/js/pages/automations/components/automations-list.js	Adds new filters, recent-run dots, and an automation detail panel UI.
crates/ironclaw_webui_v2_static/static/js/pages/automations/automations-page.js	Adds selection state management for the detail panel.
crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs	Updates API tests for run_limit query construction.
crates/ironclaw_webui_v2_static/static/js/lib/api.js	Adds runLimit -> run_limit support to listAutomations.
crates/ironclaw_webui_v2_static/static/js/i18n/en.js	Adds strings for new filters, summary metrics, and detail panel.
crates/ironclaw_triggers/tests/repository_contract.rs	Extends repository contract tests for run history lifecycle + retention.
crates/ironclaw_triggers/src/worker/tests.rs	Ensures worker cleanup records terminal statuses into run history.
crates/ironclaw_triggers/src/worker/ports.rs	Extends active run state to carry terminal TurnStatus.
crates/ironclaw_triggers/src/worker/active_cleanup.rs	Maps terminal turn status to run-history status on cleanup.
crates/ironclaw_triggers/src/postgres.rs	Adds Postgres run-history table, upsert/complete/list APIs, and pruning.
crates/ironclaw_triggers/src/libsql.rs	Adds libSQL run-history table, transactional writes, list APIs, and pruning.
crates/ironclaw_triggers/src/lib.rs	Introduces run-history types/status, repository APIs, in-memory storage, and limits.
crates/ironclaw_reborn_composition/src/trigger_poller.rs	Carries terminal TurnStatus through active-run lookup.
crates/ironclaw_reborn_composition/src/automation.rs	Adds run_limit request flow and parses recent_runs for WebUI automation facade.
crates/ironclaw_product_workflow/tests/reborn_services_contract.rs	Updates services contract tests for recent_runs and run_limit clamping/defaulting.
crates/ironclaw_product_workflow/src/webui_inbound.rs	Extends inbound WebUI list automations request with run_limit.
crates/ironclaw_product_workflow/src/reborn_services/types.rs	Adds DTOs for recent_runs and status.
crates/ironclaw_product_workflow/src/reborn_services.rs	Adds AutomationListRequest, clamps run_limit, and returns recent_runs.
crates/ironclaw_product_workflow/src/lib.rs	Re-exports new automation run-history types and constants.
crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs	Adds runtime tests for embedded recent_runs and run_limit validation.
crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs	Embeds batched recent_runs in builtin.trigger_list output with run_limit support.
crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs	Extends builtin trigger list schema with run_limit.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[henrypark133]

Code Review (multi-agent)

Intent: Persist bounded automation trigger run history across all repo backends; expose recent runs via builtin.trigger_list, product workflow, WebUI v2; redesign Automations page UI.

Stats: 14 findings (from 14 raw, 14 after dedup) across 9 files. Reviewers run: 8. Reviewers failed: 0. Body-only: 2.

Verdict: COMMENT — no Critical/High. Two intertwined themes worth attention:

• Type duplication (Medium): RebornAutomationRecentRunStatus re-declares TriggerRunHistoryStatus's three variants; trigger_run_history_status_text hand-codes what serde already produces; RawAutomationRecentRunRecord exists only as a pure field-copy bridge because the new enum lacks a sanitizing Deserialize. Single fix collapses all three: give RebornAutomationRecentRunStatus a sanitizing impl matching RebornAutomationState, then drop the bridge struct + helpers + manual status mapper.
• Run-history performance (Medium): Default list_trigger_run_history_batch trait impl is N+1 (loops list_trigger_run_history); InMemory variant is O(T×R); libSQL variant uses variable-length placeholder SQL, defeating prepared-statement reuse.
• run_limit=0 semantic mismatch: Schema advertises minimum: 0 but clamp_automation_run_limit forces 1, locked in by the new "clamps zero" test. Either change schema to minimum: 1 or change clamp lower bound to 0; current state is contradictory.

Bugs

1. Medium clamp_automation_run_limit forces minimum 1 even for explicit run_limit=0 (crates/ironclaw_product_workflow/src/reborn_services.rs:2405-2409, confidence 75)
   clamp_automation_run_limit clamps to [1, MAX]. JSON schema for both REST API and trigger_list tool advertise 'minimum': 0 (callers can legitimately request zero embedded runs). When caller sends run_limit=0, clamp silently returns 1, so product facade always fetches and embeds at least one run recor…
   anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:2405

Security

1. Low run_limit schema allows minimum 0, bypassing product-layer floor of 1 (crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs:326-331, confidence 65)
   JSON schema for trigger_list declares 'minimum': 0 for run_limit; product-workflow layer (clamp_automation_run_limit) clamps 0 to 1. LLM agent calling host-runtime tool directly with run_limit=0 sees empty recent_runs arrays on every trigger; same request through WebUI facade returns at least 1 run …
   anchor: crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs:328
2. Low run_id and thread_id are raw String, not typed newtypes (crates/ironclaw_product_workflow/src/reborn_services/types.rs:654-664, confidence 55)
   types.md: domain identifiers must be newtypes. run_id: Option and thread_id: String in RebornAutomationRecentRunInfo serialized directly to browser wire contract. Backend enforces 64-char lowercase hex on TriggerRouteThreadId but validation stripped at facade boundary. If DB ever stores malf…
   anchor: crates/ironclaw_product_workflow/src/reborn_services/types.rs:657

Performance

1. Medium Default trait method issues N serial DB round-trips for run-history batch (crates/ironclaw_triggers/src/lib.rs:800-818, confidence 85)
   Default list_trigger_run_history_batch loops over trigger_ids and awaits separate list_trigger_run_history call per trigger — N sequential DB round-trips. Both PostgreSQL and libSQL override this, but any future TriggerRepository implementor that misses override silently degrades to N+1. At list-pag…
   anchor: crates/ironclaw_triggers/src/lib.rs:800
2. Low InMemory batch history scan is O(trigger_count × total_runs) (crates/ironclaw_triggers/src/lib.rs:1345-1369, confidence 72)
   InMemoryTriggerRepository::list_trigger_run_history_batch iterates full state.runs HashMap once per trigger_id, giving O(T × R) total comparisons. Single pass partitioning all runs would be O(R). At 100 triggers with 500 retained runs each = 50,000 comparisons vs 500. Test/in-memory only but also ex…
   anchor: crates/ironclaw_triggers/src/lib.rs:1357
3. Low libSQL batch run-history query formats variable-length SQL preventing prepared-statement reuse (crates/ironclaw_triggers/src/libsql.rs:987-1040, confidence 65)
   list_trigger_run_history_batch for libSQL generates SQL string with N positional placeholders and passes to conn.query. Because SQL string changes with trigger count, libSQL cannot reuse cached prepared statement. For full page of 100 triggers, query planner re-parses 100-placeholder SQL string on e…
   anchor: crates/ironclaw_triggers/src/libsql.rs:998

Tests

1. Medium list_trigger_run_history_batch error path untested in builtin tool (crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:294-297, confidence 75)
   Builtin list-triggers handler now has two repository calls with ? propagation: list_scoped_triggers (covered) and list_trigger_run_history_batch (not). Repository that succeeds on first but fails on second returns Backend error, but no test exercises that path. FailingTriggerRepository fails on fi…
   anchor: crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:294
2. Low normalizeRuns chat_path=null branch (missing thread_id) has no test (crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js:229-229, confidence 75) (body-only)
   normalizeRuns sets chat_path: run?.thread_id ? /chat/${encodeURIComponent(run.thread_id)} : null. Only truthy branch asserted. Run record without thread_id should produce chat_path: null but path has no test.
   anchor: crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js:229

Conventions

1. Medium trigger_run_history_status_text bypasses TriggerRunHistoryStatus serde (crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:363-368, confidence 80)
   trigger_run_history_status_text manually maps TriggerRunHistoryStatus variants to &'static str strings ('running','ok','error'). TriggerRunHistoryStatus already derives #[serde(rename_all = 'snake_case')], so json!({ 'status': run.status }) would serialize to same strings via type's canonical serde …
   anchor: .claude/rules/types.md — Wire-stable enums
2. Medium RebornAutomationRecentRunStatus duplicates TriggerRunHistoryStatus (crates/ironclaw_product_workflow/src/reborn_services/types.rs:643-650, confidence 65)
   RebornAutomationRecentRunStatus { Running, Ok, Error } identical in variants/semantics to TriggerRunHistoryStatus in ironclaw_triggers/src/lib.rs:398-404. ironclaw_triggers not on ironclaw_product_workflow CLAUDE.md forbidden-dependency list. Duplication forces ironclaw_reborn_composition/src/automa…
   anchor: .claude/rules/types.md
3. Medium RebornAutomationRecentRunStatus missing round-trip serde test (crates/ironclaw_product_workflow/tests/reborn_services_contract.rs:4809-4827, confidence 65)
   RebornAutomationRecentRunStatus is new wire-stable enum re-exported from ironclaw_product_workflow/src/lib.rs. Sibling enum RebornAutomationState has serde round-trip test (reborn_automation_state_round_trips_serde_for_every_variant). No equivalent for RebornAutomationRecentRunStatus. Accidental ser…
   anchor: crates/ironclaw_product_workflow/tests/reborn_services_contract.rs:4809

Local Patterns

1. Low 'ok' renders as 'Done' in LAST_STATUS_PRESENTATION but 'OK' in RUN_STATUS_PRESENTATION (crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js:35-47, confidence 75) (body-only)
   Both tables in same module describe same wire value (ok). Automation-level summary shows 'Done'; per-run rows in same detail panel show 'OK'. Reader toggling between automation header and run list sees two different labels for same terminal-success state on same page.
   anchor: crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js:36
2. Nit New run-history constants introduce DEFAULT/MAX qualifiers absent from sibling TRIGGER_LIST_LIMIT (crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:28-29, confidence 60)
   TRIGGER_LIST_LIMIT serves as both default and cap (single constant). New run-history pair splits into TRIGGER_RUN_HISTORY_DEFAULT_LIMIT and TRIGGER_RUN_HISTORY_MAX_LIMIT — clearer but inconsistent with sibling naming in same file.
   anchor: crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:27

Maintainability

1. Medium RawAutomationRecentRunRecord duplicates RebornAutomationRecentRunInfo with pure field-copy bridge (crates/ironclaw_reborn_composition/src/automation.rs:166-241, confidence 85)
   RawAutomationRecentRunRecord has identical six fields as RebornAutomationRecentRunInfo and is connected only by automation_recent_run_info() copying every field verbatim. Parallel struct exists solely because RebornAutomationRecentRunStatus derives plain Deserialize without unknown-value fallback — …
   anchor: crates/ironclaw_reborn_composition/src/automation.rs:230

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

[henrypark133]

Code Review (multi-agent)

Intent: Add bounded automation run history persistence across all trigger repositories and redesign the Automations WebUI page with metrics, filters, and detail panels (31 files).

Stats: 8 findings (from 15 raw, 9 after dedup, 1 same-line merge) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 0. Note: unified diff was 206KB (truncated at 40KB for reviewer input); reviewers read full files from the worktree.

Prior-review history

Items with sound author replies — not re-flagged:

• Codex P2 (presenter last_run_at fallback) — fixed in f7db96ceb.
• Copilot libsql.rs:1462 (submitted_at fallback) — fixed in f7db96ceb.
• Copilot automations-list.js:310 (aria-selected on role="button") — addressed in 125cf1ff0.

Items verified fixed (no re-flag):

• prior #9 run_id/thread_id raw String → typed newtypes ✅
• prior #11/#12 InMemory batch O(T×R) override added ✅
• prior #14 list_trigger_run_history_batch error path now tested ✅
• prior #15 trigger_run_output now serializes via serde derive ✅
• prior #17 RebornAutomationRecentRunStatus round-trip test added ✅
• prior #18 naming reconciled ✅
• prior #19 no RawAutomationRecentRunRecord duplicate ✅

Items still present at head 125cf1ff0 and re-flagged below: Copilot's postgres parallel of the libSQL fix (#4); prior #10 surfaced now as naming (f-lp-2); prior #11 N+1 default (f-perf-1); prior #15-adjacent duplicated status text in postgres+libSQL (f-lp-1); prior #16 bespoke deserializer (f-bug-2).

bugs

1. 🔴 High PostgreSQL complete_run_history inserts completed_at for submitted_at (crates/ironclaw_triggers/src/postgres.rs:1014-1031, confidence 100) — anchor: crates/ironclaw_triggers/src/postgres.rs:1016. VALUES ($1..$7,$7) with only 7 binds — submitted_at and completed_at both write $7 = &completed_at. libSQL got the ?7,?8 fix in f7db96ceb; postgres did not. Failure-path runs on Postgres record identical submit/complete timestamps. Also flagged by security/performance/conventions.
2. Medium RebornAutomationRecentRunStatus::default() = Error; bespoke Deserialize collapses unknown → Error (crates/ironclaw_product_workflow/src/reborn_services/types.rs:652-670, confidence 75) — anchor: same. Future status variants will be silently miscounted as failures. Sibling RebornAutomationState uses Unknown. Frontend normalizeRunStatus also maps unknown → 'unknown'. Use #[serde(other)] Unknown.

maintainability

1. Medium Magic string "automation_trigger" cross-crate + .ok()-swallowed parse error (crates/ironclaw_product_workflow/src/reborn_services.rs:1709-1717, confidence 85) — anchor: same. Two issues at one site: no shared constant between ironclaw_reborn_composition (writer) and ironclaw_product_workflow (reader); compiler can't detect rename. Same function uses .ok() on serde_json::from_str, banned by error-handling.md. Also flagged by bugs/security/conventions.

performance

1. Medium Default list_trigger_run_history_batch issues N serial DB calls (crates/ironclaw_triggers/src/lib.rs:804-822, confidence 80) — anchor: crates/ironclaw_triggers/src/lib.rs:804. Three production backends override correctly, but the default is a latent N+1 trap for any future implementor (adapter/mock). At list cap 100 = 100 serial round-trips.
2. Low is_automation_trigger_thread full JSON parse per thread on every list_threads (crates/ironclaw_product_workflow/src/reborn_services.rs:1709-1717, confidence 70) — anchor: same. Up to 200 heap-allocating parses per request. Substring pre-filter on "automation_trigger" cheap.

tests

1. Medium No host-runtime test for run_limit=0 or run_limit > MAX (crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:277-285, confidence 75) — anchor: same. Product layer tests confirm zero passes (intentional opt-out); builtin layer has neither zero-case nor oversized-clamp test. Regressions invisible.

local-patterns

1. Low run_history_status_text duplicated in postgres.rs + libsql.rs; bypasses serde derive (crates/ironclaw_triggers/src/postgres.rs:1252-1258, confidence 75) — anchor: crates/ironclaw_triggers/src/libsql.rs:1652. TriggerRunHistoryStatus already derives #[serde(rename_all="snake_case")] producing the same strings. Future variant additions silently miss the manual match.
2. Nit clamp_automation_run_limit name implies symmetric clamp but allows zero (crates/ironclaw_product_workflow/src/reborn_services.rs:2419-2423, confidence 75) — anchor: same. Sibling clamp_automation_list_limit floors at 1; this one allows 0 via .min(). Intentional but misleading by name.

security / conventions / pattern-refactor (standalone)

No standalone findings. Conventions + security findings merged into bugs/maintainability above; pattern-refactor returned empty after assessing structural shape.

---

Recommended action: Address f-bug-1 (postgres submitted_at regression) before merging — it's a data-correctness bug at parity with the libSQL fix that the PR description claimed was already shipped.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.

[henrypark133]

Code Review (multi-agent) — Re-review at head b51f25144

Intent: Verify 11 follow-up commits since prior review at 125cf1ff0 addressed 8 skill findings + 4 Copilot findings. Look for new issues.

Stats: 3 findings (from 5 raw, 4 after dedup, 1 same-line merge) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 0.

Prior findings status — all addressed

Prior	Status
f-bug-1 High — postgres complete_run_history writes submitted_at = completed_at	✅ Fixed — distinct $7/$8 placeholders in 3f330229f. Author kept completed_at as fallback (rejected fire_slot suggestion) — sound reply per principle "fire_slot is scheduled not observed; would make elapsed-time look real when it isn't." Not re-flagged.
f-bug-2 Medium — RebornAutomationRecentRunStatus::default()=Error, bespoke Deserialize	✅ Fixed — Unknown variant added with #[default] + #[serde(other)]
f-maint-1 Medium — Magic string + .ok() parse swallow	✅ Fixed — shared constant in ironclaw_product_workflow::automation_thread_metadata (note: not ironclaw_threads as author reply stated); both producer + consumer use it. .ok() replaced with explicit match + tracing::debug!. Prefilter fast path added.
f-perf-1 Medium — Default list_trigger_run_history_batch N+1 trap	✅ Fixed — tracing::warn! on serial fallback added (triggers/lib.rs:824)
f-test-1 Medium — No host-runtime run_limit=0 / >MAX tests	✅ Fixed — added
f-perf-2 Low — Per-thread JSON parse on list_threads	✅ Fixed — metadata_json.contains(AUTOMATION_TRIGGER_THREAD_SOURCE_TAG) prefilter
f-lp-1 Low — run_history_status_text duplicated	✅ Fixed — moved to triggers/lib.rs::trigger_run_history_status_text; both backends use it
f-lp-2 Nit — clamp_automation_run_limit naming	⚠️ Renamed to resolve_automation_run_limit, but new resolve_thread_list_limit joined — see new f-lp-1 below for the inconsistency this creates
Copilot A — upsert_running_run_history overwrites terminals	✅ Fixed — is_some() check preserves terminal rows; in-memory regression test added
Copilot B — recent_runs[*].status unknown/malformed	✅ Fixed — sanitize_recent_run_statuses normalizes to "unknown" before deserialize
Copilot C — role="button" on <tr> invalid ARIA	✅ Fixed — moved to real <button> inside name cell; row click is pointer-only
Copilot D — trigger_list doc unclear on run_limit default	✅ Fixed — contract clarifies default 25 + run_limit=0 to suppress

New findings (introduced by c60e80a33 fix: hide automation run threads from chat list)

performance / security / maintainability

1. 🔴 High list_visible_threads_for_scope filter loop degrades to O(N) DB round-trips when automation threads dominate; no hard page cap (crates/ironclaw_product_workflow/src/reborn_services.rs:1636-1666, confidence 78) — anchor: reborn_services.rs:1642. Three issues: (1) hot-path N-queries — page size shrinks via remaining = visible_limit - visible_threads.len(), so when automation threads dominate, up to O(automation_thread_count) sequential DB round-trips; (2) micro-page requests near fill limit — 1-2 item pages; (3) no hard iteration cap — cursor-stall guard prevents infinite loops but a user can drive 50,000+ queries on a single request. Fix: fixed full-page size + MAX_FILTER_PAGES cap. Also flagged by security (DoS amplification — single caller drives ~6000 DB queries/minute under read rate limit; multi-tenant DB load) and maintainability.

tests

1. Medium Stall guard in list_visible_threads_for_scope untested (crates/ironclaw_product_workflow/src/reborn_services.rs:1657-1664, confidence 75) — anchor: same. The only infinite-loop protection has no test. InMemorySessionThreadService returning non-advancing cursor with only automation threads would reach this path.

local-patterns

1. Nit Mixed clamp_/resolve_ verb on sibling limit helpers (crates/ironclaw_product_workflow/src/reborn_services.rs:2449-2472, confidence 75) — anchor: same. 4 sibling helpers, 2 naming verbs: clamp_timeline_limit, clamp_automation_list_limit (pre-existing) vs new resolve_thread_list_limit, resolve_automation_run_limit. Reader searching 'how does run_limit get clamped' hits resolve_, same question for automation_list hits clamp_. Prior f-lp-2 partially fixed by renaming but introduced inconsistency.

bugs / conventions / pattern-refactor (standalone)

No standalone findings. PR is in good shape — main concerns are the new filter loop introduced by c60e80a33. Author's sound reply on submitted_at fallback design respected.

---

Recommended: Address f-perf-1 (loop fix) before merge — it's a new performance regression introduced by c60e80a33. Stall test (f-test-1) closes the safety gap. Naming (f-lp-1) is optional polish.

[henrypark133]

Code Review (multi-agent) — Re-review #3 at head cd85b0892

Intent: Verify cd85b0892 fix: bound automation thread filtering addressed 3 prior findings on list_visible_threads_for_scope loop + naming.

Stats: 3 findings (all new minor; from 3 raw, 3 after dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 0.

Prior findings status — all addressed

Prior	Status
f-perf-1 High — Loop degrades to O(N) DB round-trips, no hard page cap	✅ Fixed — THREAD_LIST_FILTER_MAX_PAGES = 20 hard cap added; fetch_limit = visible_limit.max(50).min(200) ensures min full-page size
f-test-1 Medium — Stall guard untested	✅ Fixed — list_threads_breaks_out_when_cursor_does_not_advance_for_automation_threads + list_threads_caps_filtered_pages_when_automation_threads_dominate added
f-lp-1 Nit — Mixed clamp_/resolve_ naming	✅ Fixed — resolve_thread_list_limit → clamp_thread_list_limit; resolve_automation_run_limit → clamp_automation_run_limit

New findings (minor)

tests

1. Medium clamp_thread_list_limit oversize/zero bounds not tested through list_threads facade (crates/ironclaw_product_workflow/src/reborn_services.rs:2480-2484, confidence 90) — anchor: same. Parallel list_automations path has both boundary tests; list_threads lacks them. A regression inverting the clamp would be invisible while the analogous automations path would catch it. Per testing.md 'Test Through the Caller'.

conventions

1. Nit TRIGGER_LIST_MAX_LIMIT used as both default and max while sibling pair uses separate DEFAULT/MAX (crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs:27-29, confidence 75) — anchor: same. PR introduces TRIGGER_RUN_HISTORY_DEFAULT_LIMIT + TRIGGER_RUN_HISTORY_MAX_LIMIT paired naming. Adjacent TRIGGER_LIST_MAX_LIMIT remains single constant doubling as default + max. Two conventions for same concept in same block.

maintainability

1. Low BatchRunHistoryFailingTriggerRepository duplicates 14 delegation stubs from RemoveFailingTriggerRepository (crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs:6471-6757, confidence 75) — anchor: first_party_builtin_tools.rs:6488. ~140 lines of identical boilerplate; only one method differs. Merge into a single parameterized PartiallyFailingTriggerRepository { fail_remove, fail_run_history_batch }.

security / bugs / performance / local-patterns / pattern-refactor (standalone)

No standalone findings. PR is in solid shape.

---

Recommended: All three findings are minor — the prior High/Medium concerns from the previous round are fully resolved. f-test-1 is the highest-value follow-up (parity with automations boundary tests); others are polish.

[henrypark133]

Medium — clamp_thread_list_limit oversize/zero bounds not tested through list_threads facade.

clamp_thread_list_limit clamps 0→1 and oversize→200 (THREAD_LIST_MAX_PAGE_SIZE). The parallel list_automations path has both boundary tests (list_automations_clamps_oversize_limit_before_product_facade, list_automations_clamps_zero_limit_before_product_facade). No test exercises list_threads with limit=0 or limit=u32::MAX. A regression that removed or inverted the clamp would be invisible while the analogous automations path would catch it.

Fix: Add list_threads_clamps_oversize_limit_to_max_page_size + list_threads_clamps_zero_limit_to_minimum_of_one.

#[tokio::test]
async fn list_threads_clamps_oversize_limit_to_max_page_size() {
    let thread_service = Arc::new(ScriptedThreadService::list_pages(vec![
        ListThreadsForScopeResponse { threads: vec![], next_cursor: None },
    ]));
    let services = RebornServices::new(thread_service.clone(), Arc::new(FakeTurnCoordinator::default()));
    services.list_threads(caller(), WebUiListThreadsRequest { limit: Some(u32::MAX), cursor: None })
        .await.expect("list threads");
    assert_eq!(thread_service.list_requests()[0].limit, Some(200));
}

[henrypark133]

Nit — TRIGGER_LIST_MAX_LIMIT used as both default and max while sibling pair uses separate DEFAULT/MAX.

PR introduces TRIGGER_RUN_HISTORY_DEFAULT_LIMIT + TRIGGER_RUN_HISTORY_MAX_LIMIT paired naming. Adjacent TRIGGER_LIST_MAX_LIMIT (renamed from TRIGGER_LIST_LIMIT) remains a single constant doubling as both default and max (unwrap_or(TRIGGER_LIST_MAX_LIMIT).min(TRIGGER_LIST_MAX_LIMIT)). Two different conventions for same concept in same file block.

Fix: Either add TRIGGER_LIST_DEFAULT_LIMIT separate from TRIGGER_LIST_MAX_LIMIT, or rename the new pair to TRIGGER_RUN_HISTORY_LIMIT (single constant). At minimum: comment noting the dual role.

[henrypark133]

Low — BatchRunHistoryFailingTriggerRepository duplicates 14 delegation stubs from RemoveFailingTriggerRepository.

BatchRunHistoryFailingTriggerRepository wraps InMemoryTriggerRepository and overrides only list_trigger_run_history_batch to error. Achieved via copy-paste of all 14 delegation stubs from RemoveFailingTriggerRepository — ~140 lines of identical boilerplate, differing only in which single method fails. Readers must scan 140 lines to confirm only one method differs.

Fix: Merge into a single parameterized PartiallyFailingTriggerRepository { inner, fail_remove: bool, fail_run_history_batch: bool } — one impl covers both test cases; or expose Arc via blanket impl so only the failing method needs override.