DOS-1136: add death-note retry context - #15
Conversation
Co-authored-by: multica-agent <github@multica.ai>
WalkthroughRetry tasks now store structured parent failure context, expose it through agent responses, and inject it into daemon prompts across task types. Tests cover ordinary timeout retries, poisoned-session retries, tail truncation, session clearing, and prompt inclusion. ChangesRetry death-note propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CreateRetryTask
participant AgentHandler
participant PromptBuilder
CreateRetryTask->>AgentHandler: store death_note in retry task context
AgentHandler->>AgentHandler: extract valid death_note JSON
AgentHandler->>PromptBuilder: provide task with DeathNote
PromptBuilder->>PromptBuilder: append Parent Task Death Note section
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@server/internal/daemon/prompt_test.go`:
- Line 4: Extend death-note coverage in prompt_test.go with table-driven cases
for buildCommentPrompt, buildChatPrompt, buildAutopilotPrompt, and
buildQuickCreatePrompt, matching the existing default/issue-path assertions.
Verify each builder invokes writeDeathNote and preserves the expected ordering,
so regressions from dropped or reordered call sites are detected.
In `@server/internal/daemon/prompt.go`:
- Around line 279-290: Update writeDeathNote to generate a CommonMark fence
longer than any consecutive backtick sequence in the trimmed note: start with
three backticks, extend the fence while strings.Contains(note, fence) is true,
and use that same fence for both opening and closing delimiters around the JSON
content.
In `@server/pkg/db/queries/agent.sql`:
- Around line 188-266: The CreateRetryTask query repeats the resume-unsafe
failure list across multiple retry expressions, allowing inconsistent behavior.
Compute a single is_resume_unsafe boolean in a CTE and reference it for
resume_safe, JSON session_id/work_dir/continuation_hint, top-level
session_id/work_dir, and force_fresh_session; update the generated agent.sql.go
query string accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6db31a2c-e519-45fd-a8eb-d03cfbb9b456
⛔ Files ignored due to path filters (1)
server/pkg/db/generated/agent.sql.gois excluded by!**/generated/**
📒 Files selected for processing (6)
server/cmd/server/rerun_session_test.goserver/internal/daemon/prompt.goserver/internal/daemon/prompt_test.goserver/internal/daemon/types.goserver/internal/handler/agent.goserver/pkg/db/queries/agent.sql
| package daemon | ||
|
|
||
| import ( | ||
| "encoding/json" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider extending death-note coverage to the other prompt builders.
Only the default/issue path is tested. writeDeathNote is also called from buildCommentPrompt, buildChatPrompt, buildAutopilotPrompt, and buildQuickCreatePrompt; a similar table-driven case per builder would catch a future regression where one of those call sites is dropped or reordered.
Also applies to: 55-75
🤖 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 `@server/internal/daemon/prompt_test.go` at line 4, Extend death-note coverage
in prompt_test.go with table-driven cases for buildCommentPrompt,
buildChatPrompt, buildAutopilotPrompt, and buildQuickCreatePrompt, matching the
existing default/issue-path assertions. Verify each builder invokes
writeDeathNote and preserves the expected ordering, so regressions from dropped
or reordered call sites are detected.
|
|
||
| func writeDeathNote(b *strings.Builder, task Task) { | ||
| note := strings.TrimSpace(string(task.DeathNote)) | ||
| if note == "" || note == "null" { | ||
| return | ||
| } | ||
| b.WriteString("## Parent Task Death Note\n\n") | ||
| b.WriteString("A previous attempt died and the platform captured this continuation artifact. Use it before deciding whether to resume or start fresh. If `resume_safe` is false, do not resume the parent session; continue from the evidence below instead.\n\n") | ||
| b.WriteString("```json\n") | ||
| b.WriteString(note) | ||
| b.WriteString("\n```\n\n") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fixed 3-backtick fence around untrusted death_note content can be broken out of.
note embeds error_tail/output_tail captured from the parent task's own error/output. If that content contains a ``` sequence, the fence closes early and the remaining text is interpreted as normal prompt content by the next agent turn rather than as fenced data — letting content from a prior (possibly attacker-influenced) run inject instructions into the retry prompt.
Use a fence length longer than any backtick run present in the content (CommonMark convention), rather than a fixed 3 backticks:
🔒️ Proposed fix
func writeDeathNote(b *strings.Builder, task Task) {
note := strings.TrimSpace(string(task.DeathNote))
if note == "" || note == "null" {
return
}
b.WriteString("## Parent Task Death Note\n\n")
b.WriteString("A previous attempt died and the platform captured this continuation artifact. Use it before deciding whether to resume or start fresh. If `resume_safe` is false, do not resume the parent session; continue from the evidence below instead.\n\n")
- b.WriteString("```json\n")
- b.WriteString(note)
- b.WriteString("\n```\n\n")
+ fence := "```"
+ for strings.Contains(note, fence) {
+ fence += "`"
+ }
+ b.WriteString(fence)
+ b.WriteString("json\n")
+ b.WriteString(note)
+ b.WriteString("\n")
+ b.WriteString(fence)
+ b.WriteString("\n\n")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func writeDeathNote(b *strings.Builder, task Task) { | |
| note := strings.TrimSpace(string(task.DeathNote)) | |
| if note == "" || note == "null" { | |
| return | |
| } | |
| b.WriteString("## Parent Task Death Note\n\n") | |
| b.WriteString("A previous attempt died and the platform captured this continuation artifact. Use it before deciding whether to resume or start fresh. If `resume_safe` is false, do not resume the parent session; continue from the evidence below instead.\n\n") | |
| b.WriteString("```json\n") | |
| b.WriteString(note) | |
| b.WriteString("\n```\n\n") | |
| } | |
| func writeDeathNote(b *strings.Builder, task Task) { | |
| note := strings.TrimSpace(string(task.DeathNote)) | |
| if note == "" || note == "null" { | |
| return | |
| } | |
| b.WriteString("## Parent Task Death Note\n\n") | |
| b.WriteString("A previous attempt died and the platform captured this continuation artifact. Use it before deciding whether to resume or start fresh. If `resume_safe` is false, do not resume the parent session; continue from the evidence below instead.\n\n") | |
| fence := " |
🤖 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 `@server/internal/daemon/prompt.go` around lines 279 - 290, Update
writeDeathNote to generate a CommonMark fence longer than any consecutive
backtick sequence in the trimmed note: start with three backticks, extend the
fence while strings.Contains(note, fence) is true, and use that same fence for
both opening and closing delimiters around the JSON content.
| 'queued', p.priority, p.trigger_comment_id, p.trigger_summary, | ||
| jsonb_set( | ||
| COALESCE(p.context, '{}'::jsonb), | ||
| '{death_note}', | ||
| jsonb_strip_nulls(jsonb_build_object( | ||
| 'parent_task_id', p.id, | ||
| 'failure_reason', COALESCE(p.failure_reason, 'agent_error'), | ||
| 'failed_at', COALESCE(p.completed_at, now()), | ||
| 'error_tail', NULLIF(right(COALESCE(p.error, ''), 4000), ''), | ||
| 'output_tail', NULLIF(right(COALESCE(p.result::text, ''), 4000), ''), | ||
| 'attempt', p.attempt, | ||
| 'max_attempts', p.max_attempts, | ||
| 'resume_safe', NOT ( | ||
| COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) | ||
| ), | ||
| 'session_id', CASE | ||
| WHEN COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) THEN NULL | ||
| ELSE NULLIF(p.session_id, '') | ||
| END, | ||
| 'work_dir', CASE | ||
| WHEN COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) THEN NULL | ||
| ELSE NULLIF(p.work_dir, '') | ||
| END, | ||
| 'files_touched_available', false, | ||
| 'files_touched', '[]'::jsonb, | ||
| 'continuation_hint', CASE | ||
| WHEN COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) | ||
| THEN 'Start a fresh session. Use this death note to recover partial progress; do not resume the poisoned parent session.' | ||
| ELSE | ||
| 'Continue from the parent task failure context. Resume the pinned session/workdir when the runtime supports it; otherwise use this note to avoid restarting from scratch.' | ||
| END | ||
| )), | ||
| true | ||
| ), | ||
| CASE | ||
| WHEN COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) THEN NULL | ||
| ELSE p.session_id | ||
| END, | ||
| CASE | ||
| WHEN COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ) THEN NULL | ||
| ELSE p.work_dir | ||
| END, | ||
| p.attempt + 1, p.max_attempts, p.id, | ||
| p.failure_reason IS NOT DISTINCT FROM 'codex_semantic_inactivity', | ||
| COALESCE(p.failure_reason, '') IN ( | ||
| 'iteration_limit', | ||
| 'agent_fallback_message', | ||
| 'api_invalid_request', | ||
| 'codex_semantic_inactivity' | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)agent\.sql$|agent\.sql\.go$|go.mod$' || true
echo "== agent.sql context =="
if [ -f server/pkg/db/queries/agent.sql ]; then
wc -l server/pkg/db/queries/agent.sql
sed -n '150,280p' server/pkg/db/queries/agent.sql | cat -n -v | sed 's/^/agent.sql: /'
fi
echo "== generator context =="
if [ -f server/pkg/db/generated/agent.sql.go ]; then
wc -l server/pkg/db/generated/agent.sql.go
rg -n "func \\(q \\*Queries\\) (RetryQueued|.*Agent)|Restart|Resume|agent_task_queue|failure_reason|resume_safe|force_fresh_session|agent_fallback_message|iteration_limit" server/pkg/db/generated/agent.sql.go || true
fi
echo "== other uses of same list =="
rg -n "iteration_limit|agent_fallback_message|api_invalid_request|codex_semantic_inactivity" server pkg . --glob '!vendor/**' --glob '!*.lock' || true
echo "== deterministic count for current SQL snippet =="
python3 - <<'PY'
import re, pathlib
p=pathlib.Path('server/pkg/db/queries/agent.sql')
if not p.exists():
print('missing')
raise SystemExit
s=p.read_text()
sub=s[s.index('jsonb_set'):s.index('FROM agent_task_queue') if 'FROM agent_task_queue' in s else len(s)]
pat=r"COALESCE\(p\.failure_reason, ''\) IN \(\s*'iteration_limit',\s*'agent_fallback_message',\s*'api_invalid_request',\s*'codex_semantic_inactivity'\s*\)"
finds=re.findall(pat, sub)
print('occurrences of exact predicate:', len(finds))
for i,b in enumerate(finds,1):
print(f'[{i}] len={len(b.strip())}')
PYRepository: firefly-events/multica
Length of output: 47802
Compute the resume-unsafe blacklist once and reference it in every retry path.
CreateRetryTask repeats the same four-value list 7 times, including resume_safe, the JSON session_id/work_dir/continuation_hint CASEs, and the top-level session_id/work_dir/force_fresh_session expressions. A future edit that misses one spot can leave a poisoned session/workdir in a retry that still reports itself as unsafe. A CTE boolean like is_resume_unsafe makes these predicates impossible to drift. Also update server/pkg/db/generated/agent.sql.go by hand if the query string is changed.
[maintainabiliity_and_code_quality]
🤖 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 `@server/pkg/db/queries/agent.sql` around lines 188 - 266, The CreateRetryTask
query repeats the resume-unsafe failure list across multiple retry expressions,
allowing inconsistent behavior. Compute a single is_resume_unsafe boolean in a
CTE and reference it for resume_safe, JSON
session_id/work_dir/continuation_hint, top-level session_id/work_dir, and
force_fresh_session; update the generated agent.sql.go query string accordingly.
1 participant
Summary
death_noteartifact whenCreateRetryTaskclones a failed parent task.Validation
cd server && go test ./internal/daemon -run 'TestBuildPromptIncludesDeathNote|TestBuildQuickCreatePromptRules'cd server && go test ./cmd/server -run 'TestCreateRetryTask(KeepsOrdinaryTimeoutSession|DeathNoteCapturesTailsAndPoisonedFreshSession|FreshensCodexSemanticInactivity)'cd server && go test ./internal/handler -run 'TestDoesNotExist'cd server && go test ./pkg/db/generatedbin/verify verify-specs/dos-1136-death-feedback-continue.jsonfromdostal-swarmNotes
The
sqlcbinary is not installed on this host, so the generated query string was updated manually. The SQL query shape did not add parameters or scan columns.Summary by CodeRabbit
New Features
Bug Fixes