fix: surface permanent turn-failure reasons in Telegram replies (#1831) - #2565
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe change is careful and correct: the redaction chain indexes the tuple return properly, No findings. [OPUS-REVIEWED] 17953db Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Real harm (misleading retry advice on permanent failures), fixed at the right seam — classification at dispatch, display-only in the renderer, fail-closed sanitization. Suggestions
[DESIGN-REVIEWED] 17953db |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/telegram/renderer.py:781 -- A permanent ACP error after a text chunk bypasses False positive or not applicable? A repository writer can comment: |
A turn that failed with a permanent AcpError (e.g. the account lacks the selected model) was finalized to the generic '⚠️ Error — please try again' placeholder, hiding the actionable remedy the error already carried and giving retry advice the error itself says is wrong. The dispatcher's exception handler now derives a bounded, user-safe reason — only for AcpError with transient=False; the message is credential/exfil-redacted, local paths stripped, newline-collapsed and hard-capped — and passes it to TelegramRenderer.close(), which shows it as the finalization placeholder. Transient and unclassified failures keep the retry wording; non-Acp exceptions never leak internals. Discord has the identical pattern (discord/renderer.py) and is left as a follow-up to keep this change scoped to the reported surface. Closes #1831
acb7944 to
17953db
Compare
…dotdev#1831) (kirodotdev#2565) A turn that failed with a permanent AcpError (e.g. the account lacks the selected model) was finalized to the generic '⚠️ Error — please try again' placeholder, hiding the actionable remedy the error already carried and giving retry advice the error itself says is wrong. The dispatcher's exception handler now derives a bounded, user-safe reason — only for AcpError with transient=False; the message is credential/exfil-redacted, local paths stripped, newline-collapsed and hard-capped — and passes it to TelegramRenderer.close(), which shows it as the finalization placeholder. Transient and unclassified failures keep the retry wording; non-Acp exceptions never leak internals. Discord has the identical pattern (discord/renderer.py) and is left as a follow-up to keep this change scoped to the reported surface. Closes kirodotdev#1831
Summary
Fixes the Telegram surface replying
⚠️ Error — please try againto every failed turn, even when the failure is permanent and the underlyingAcpErroralready carries a complete, actionable remedy (e.g. "Your account does not have access to model X — available: …, pick one in the model picker or set agent.model to auto. Retrying will not help."). The reporter's own unblock is switching the session's agent off the unavailable model — this change makes that discoverable from the chat itself, without reading gateway logs.What changed
telegram/transport_dispatch.py: thehandle_messageexception handler derives a bounded, user-safe failure reason via a new_user_safe_failure_reason()and hands it to the renderer's finalization. Only a permanentAcpError(transient is False) qualifies — transient and unclassified failures keep the retry wording, and non-Acp exceptions never leak internals (CWE-209). The reason is treated as untrusted output:security.redact()(credentials + exfil URLs),redact_local_paths(), newline collapse, 500-char hard cap. The sanitizer is fail-closed: any internal error falls back to the generic text. Existinglogger.exception+record_failurebehavior is unchanged.telegram/renderer.py:close()accepts an optional pre-sanitizedfailure_reasonused as the finalization placeholder instead of the generic string. The generic text is preserved as the default, so every other caller is unaffected; the reason is ignored once the turn is finalized. The placeholder path sends plain text (parse_mode=None), so the reason cannot inject Telegram HTML.Scope
Deliberately Telegram-only (the reported surface). Discord has the identical pattern at
src/kiro_crew/discord/renderer.py:541— noted as a follow-up candidate rather than widening this change to every transport.Testing
test/test_telegram.py, verified to fail before the fix:handle_message: permanentAcpErrormessage reaches the finalized placeholder (single-line, no retry text); transient error keeps the retry text; non-Acp exception stays generic and leaks nothing (asserts the message and a/home/...path do not appear)on_doneNoneisort,flake8,mypy(865 files),python -m pytest(40366 passed; 75 failures verified pre-existing parallel-run environment flakes in unrelated app tests — the failing files pass standalone),test_telegram.py190/190.Backend-only: no dashboard surface, no screenshots required.
Closes #1831