Surface Reborn run failure summaries - #4679
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
risk: low
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: REQUEST_CHANGES
Critical Issues:
- ❌ PR scope mismatch: title says "Show Reborn REPL failure summaries" but 90% of diff is unrelated production config support
- ❌ File size explosion:
runtime/mod.rsgrew 1290→1964 lines (+674, +52%) - ❌ Unjustified bundling of production-critical config changes under benign UI improvement title
- ❌ Missing test coverage for the actual feature (display path not tested, only helper tested)
Findings by Category:
- Structural: 2 High
- Tests: 1 High
- Conventions: 2 Medium/Low
Critical Findings
1. PR Scope Mismatch / Dishonest Title
Severity: High | Confidence: 100% | Category: structural-regression
Location: ALL files
Issue: PR title "Show Reborn REPL failure summaries" describes ~70 lines of user-facing message improvements, but the actual diff contains ~674 net new lines including:
- Production profile support
- PostgreSQL config handling
- Storage backend selection logic
- Policy section handling changes
- 550+ lines of production config tests
This is either two unrelated PRs accidentally merged, or a deliberate attempt to bypass review scrutiny by hiding production-critical infrastructure changes under a benign UI improvement title.
Fix: Split into two PRs:
- "Add production profile and PostgreSQL config support" - requires security review, migration docs, rollback plan
- "Show Reborn REPL failure summaries" - just the user-facing message improvements
Anchor: AGENTS.md: "Keep changes scoped; avoid broad refactors unless the task truly requires them"
2. Unjustified Ambiguous Decision: Production Config Changes
Severity: High | Confidence: 95% | Category: tier-1-#18-unjustified-decision
Location: crates/ironclaw_reborn_cli/src/runtime/mod.rs:454-507
Issue: The diff adds production profile support with major ownership/behavioral changes:
- New
build_production_services_input()function - Production vs local-dev branching in
build_services_input_with_options() - Policy section validation changes (now allowed for Production/MigrationDryRun)
- PostgreSQL URL + secret master key env handling
Rationale sources searched:
- PR title: mentions only "REPL failure summaries" ❌
- PR body: null ❌
- Inline comments: none at change site ❌
- Linked issues: none visible ❌
No justification provided for why production config support is bundled with a REPL UI improvement.
Fix: Add explicit rationale in PR body OR split production config into a separate PR with appropriate title and security review.
Anchor: /Users/firatsertgoz/.pi/agent/skills/code-review/reviewers/conventions.md #18 (unjustified ambiguous decisions)
3. File Size Explosion
Severity: High | Confidence: 100% | Category: file-size-explosion
Location: crates/ironclaw_reborn_cli/src/runtime/mod.rs
Issue: File grew from 1290 lines to 1964 lines (+674 lines, +52% increase) in a single PR. Thermo-nuclear review rule: "Do not let a PR push a file from under 1k lines to over 1k lines without a very strong reason." This file was already at 1290 and is now approaching 2000 lines. The 550+ lines of production config tests are not cohesive with the runtime input building logic.
Fix: Extract production-related functionality and tests:
- Move
build_production_services_input()and related logic tocrates/ironclaw_reborn_cli/src/runtime/production_config.rs - Move production config tests to dedicated test module or
production_config_tests.rs
Anchor: /Users/firatsertgoz/.agents/skills/thermo-nuclear-code-quality-review/SKILL.md rule #1
High Findings
4. Missing Test Coverage: Display Path
Severity: High | Confidence: 85% | Category: missing-integration-test
Location: crates/ironclaw_reborn_cli/src/runtime/mod.rs:281-348
Issue: Tests only cover the helper failure_summary_for_cli() directly. The full display path is untested:
print_reply()
→ no_assistant_text_message()
→ reply_without_text_summary() [has branching logic, 0% coverage]
→ failure_summary_for_cli() [tested]
Per AGENTS.md: "Test through the caller, not just the helper. When a helper gates a side effect and has any wrapper between it and that side effect, a unit test on the helper alone is not sufficient regression coverage."
The intermediate functions have branching logic (status matching, format string construction) but no tests.
Fix: Add integration test:
tests::runtime::print_reply_formats_failure_with_category()that:
- Creates
AssistantReplywithstatus: Failed,failure_category: Some("driver_panic") - Calls
no_assistant_text_message() - Asserts output contains: summary text,
failure_category=driver_panic, status, run_id
Anchor: AGENTS.md → .claude/rules/testing.md (test through caller, not just helper)
Medium Findings
5. Hardcoded Failure Category Strings
Severity: Medium | Confidence: 75% | Category: tier-2-#6-pattern-adherence
Location: crates/ironclaw_reborn_cli/src/runtime/mod.rs:317-348
Issue: failure_summary_for_cli() hardcodes 14 failure category string literals ("driver_not_found", "model_credits_exhausted", etc.) that must match the categories returned by failure.category(). This is brittle string-matching that will break silently if category strings change upstream in ironclaw_turns. No const definitions or shared source of truth.
Fix: Define failure category constants in ironclaw_turns crate (where SanitizedFailure lives) and import them here, OR add a user_message() method directly on SanitizedFailure so the human-readable mapping lives with the failure type itself.
6. Undocumented API Surface Expansion
Severity: Low | Confidence: 60% | Category: tier-1-#2-api-contract
Location: crates/ironclaw_reborn_composition/src/lib.rs:185
Issue: New pub use ironclaw_turns::TurnStatus; export makes TurnStatus publicly available from composition crate. Type was already visible in AssistantReply.status but couldn't be pattern-matched without importing from ironclaw_turns. Likely a bugfix for an API gap, but still a public API expansion without mention in PR body.
Fix: Document in PR body that this is an API surface expansion (or note it's effectively already public via AssistantReply).
Approval Blocked By
Per thermo-nuclear review approval bar:
- ❌ Clear structural regression - 674-line file growth, production config bundled with UI change
- ❌ Unjustified file-size explosion - 1290→1964 lines
- ❌ Missing test for actual feature - display path not tested through caller
- ❌ Unjustified scope creep - production config changes have no rationale
Required before approval:
- Split production config changes into separate PR OR provide full security-reviewed rationale
- Decompose
runtime/mod.rsbelow 1500 lines (extract production config logic) - Add integration test for
print_reply()→ full display path - Update PR title/body to accurately reflect scope
github-actions
Bot
added
size: L
|
Added the web UI side of the silent-failure fix to this PR. What changed:
Local verification:
Note: cargo test -p ironclaw_webui_v2_static built and passed its embedded asset unit tests, then failed in the existing i18n_consistency integration test due locale key-set drift around automations/skills keys unrelated to this patch. |
zmanian
changed the title
|
Followed up on the review comments. Changes pushed:
Review notes:
Local verification after this push:
@serrrfirat could you re-review this current head? |
b8cc4fb to
5b4a1cb
Compare
|
Follow-up after the latest force-push:
Local verification after the architecture fix: cargo fmt --all
cargo test -p ironclaw_reborn_cli no_assistant_text_message_formats_failed_reply_with_category
cargo test -p ironclaw_architecture reborn_cli_binary_crate_stays_separate_from_v1_root
cargo test -p ironclaw_architecture reborn
cargo test -p ironclaw_webui_v2_static
node --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/failureMessages.test.mjs
node --check crates/ironclaw_gateway/static/js/core/sse.js
node --check crates/ironclaw_gateway/static/js/core/history.js
node --check crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.jsRequesting re-review from @serrrfirat now that the stale scope/file-size concerns and the valid display-path test gap have been addressed. |
5b4a1cb to
c323414
Compare
|
One more follow-up for the fresh Code Style failure after the re-review push:
Local verification for this CI-specific fix: python3 scripts/check_no_panics.py --base origin/main --head HEAD
cargo test -p ironclaw_reborn_cli no_assistant_text_message_formats_failed_reply_with_category
cargo test -p ironclaw_architecture rebornThe PR is now force-pushed at |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review for c323414dabaf8efb45e2f840cf1e6302106f71c3.
Reviewers: security clean; bugs 2 findings; performance/concurrency clean; tests 2 findings; conventions 1 finding. After deduplication: 4 inline comments, all Medium severity. Event: COMMENT.
Intent understood: surface sanitized Reborn run failure summaries in CLI and WebUI instead of blank or placeholder assistant replies.
| "(no assistant text; status={:?}, run_id={})", | ||
| reply.status, reply.run_id | ||
| ), | ||
| None => eprintln!("{}", no_assistant_text_message(reply)), |
There was a problem hiding this comment.
[Medium][bugs:cli-diagnostics] This formatter is only reached through print_reply(), but the one-shot send_once() path still returns the old generic reborn run did not produce an assistant reply error before calling print_reply() whenever reply.is_successful_final_reply() is false. That means ironclaw-reborn run --message ... still misses the sanitized summary and failure_category this PR is trying to expose. Route non-success AssistantReply values through no_assistant_text_message() before returning the error, or include that formatted message in the bail path.
| } | ||
| } | ||
|
|
||
| fn failure_summary_for_cli(category: Option<&str>) -> &'static str { |
There was a problem hiding this comment.
[Medium][conventions:duplicated-logic] This repeats the existing failure-category-to-summary mapping in crates/ironclaw_reborn_composition/src/projection/turn_events.rs instead of using a shared composition-owned helper. The two copies have already diverged: the projection mapping handles iteration_limit, while the CLI copy falls through to the generic message. Please move this mapping behind a shared facade, or at least keep the CLI table in lockstep with the composition table.
| run_id, | ||
| status: terminal_status, | ||
| status: terminal_state.status, | ||
| failure_category: terminal_state |
There was a problem hiding this comment.
[Medium][tests:missing-caller-level-test] The new AssistantReply.failure_category propagation is only exercised by constructing an AssistantReply through the test helper; I could not find a caller-level test that drives RebornRuntime::send_user_message_with_cancellation through a terminal failed run and asserts the returned AssistantReply.failure_category. Per the repo’s caller-level testing rule, add a runtime test such as send_user_message_returns_failure_category_for_terminal_failure so this field cannot be silently dropped while the CLI helper tests still pass.
| _doneWithoutResponseTimer = null; | ||
| if (currentThreadId) loadHistory(); | ||
| if (!threadIdAtDone || currentThreadId !== threadIdAtDone) return; | ||
| Promise.resolve(loadHistory()).then(() => { |
There was a problem hiding this comment.
[Medium][tests:missing-integration] This new Done-without-response branch now depends on loadHistory() returning a promise, the current-thread guard, and the DOM last-message check before appending the visible fallback. I could not find adjacent JS/e2e coverage for the case where Done arrives with no response event and history reload still leaves no assistant/system reply. Please add a caller-level test for that path so a regression does not bring back the silent-turn failure this PR fixes.
github-actions
Bot
added
size: XL
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Addressed the review findings in 065bf995b317eee03173f7e476f99e391a77f3a0.
What changed:
- Merged
origin/mainand resolved theuseChatEvents.test.mjsconflict by preserving both the approval-gate test from main and the failure-surfacing tests from this PR. - Routed CLI one-shot/non-TTY failed replies through the same no-assistant-text failure formatting as the interactive path.
- Consolidated Reborn failure-summary mapping in composition and reused it from both projection and CLI paths, including
iteration_limit. - Added caller-level coverage for
AssistantReply.failure_categorypropagation throughsend_user_message. - Added Gateway v1 SSE coverage for Done-without-response fallback behavior.
Validation:
- Local targeted JS, Rust, syntax, fmt, and diff checks passed.
- GitHub CI is green on
065bf995b317eee03173f7e476f99e391a77f3a0, including Code Style, Tests (Reborn), Tests (Legacy), Reborn E2E, Regression Test Check, PR classification/scope, and all non-skipped jobs.
* Show Reborn REPL failure summaries * Surface silent run failures in web UIs * Test Reborn CLI failure display path --------- Co-authored-by: serrrfirat <f@nuff.tech>
3 participants
Summary
AssistantReply(no assistant text; ...)for failed or recovery-required runsTurnStatusfromironclaw_reborn_compositionso callers can interpret the already-exposedAssistantReply.statusfield without adding another crate dependencyTesting
cargo fmt --allcargo test -p ironclaw_reborn_cli failure_summary_for_clicargo test -p ironclaw_reborn_cli runtime_does_not_produce_replycargo test -p ironclaw_reborn_cli no_assistant_text_message_formats_failed_reply_with_categorynode --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/failureMessages.test.mjsnode --check crates/ironclaw_gateway/static/js/core/sse.jsnode --check crates/ironclaw_gateway/static/js/core/history.jsnode --check crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.jscargo test -p ironclaw_webui_v2_staticReview Notes
mainso the prior WebUI v2 i18n consistency failure is resolved locally.