fix(delegation): preserve redacted output on timeout - #65824
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Preserves redacted output on delegation timeout. Note: The "secret" in diff is a test fixture (sk-... pattern in test code), not a real credential. No security concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving bounded, force-redacted evidence from timed-out children. The synchronous timeout record and subagent.complete progress payload are covered, but the background completion path still drops the new data.
Problems
delegate_task(background=true)uses the batch completion route (tools/delegate_tool.py:2803-2812). That route retains task results, but_format_async_delegation()renders timeout/error tasks solely fromsummaryanderror(tools/process_registry.py:2091-2121); it never readspartial_output_tail. The new field added attools/delegate_tool.py:2110therefore is not reinjected into the parent session for the background workflow.
Suggested changes
- Render validated
partial_output_tailentries in both async batch and single-result formatting, and add a regression test throughformat_process_notification()using a timeout result with a redacted preview.
This is an automated hermes-sweeper review.
5b214dd to
d2c64ef
Compare
|
Addressed review feedback: Changes:
Post-fix validation: 188 passed, 0 failed (canonical focused suite). |
d2c64ef to
37d99f0
Compare
|
Added New tests in
Validation: 174 + 16 = 190 passed, 0 failed. New head: |
|
Independent production confirmation + E2E verification of this PR. Production repro (v0.19.0 / tag Worth noting for triage: the code default is already no-cap ( Verification (head
Also +1 on the ordering choice: applying Correction 2026-07-28: an earlier version of this comment attributed the 600 s to the release default. Verified against |
37d99f0 to
be54e7e
Compare
|
Thanks for the independent production confirmation and E2E verification — this validates the remaining opt-in timeout case even after I have also rebased the PR onto current Post-rebase validation:
New head: |
|
Re-verified at the new head
Conflict resolution looks right from here — both test families coexist and pass. No further asks from our side. |
be54e7e to
dab408e
Compare
|
Thanks for re-verifying the rebased head and sabotage-checking the regressions. This is confirming evidence rather than a change request: it shows the structured timeout-metadata tests and partial-tail tests coexist, still fail on the pre-fix behavior, and retain redact-before-truncate / fail-closed ordering. No scope expansion is needed.\n\nThe PR later became conflicted again, so I rebased it onto current |
|
Re-verified at the new head
Still merge-ready from our side. |
|
Thanks for re-verifying the current head |
SummaryNine PRs address or reference two adjacent delegation issue cores: #17312/#17329/#63379 target structured N-API-call timeout state, #31207/#37724 extend timeout artifacts, #15105 covers zero-call diagnostics, #65824 preserves redacted partial output, and #37368/#38983 isolate child session identity. Related pull requests
Duplicates#17312, #17329, and #63379 address the same structured N-API-call timeout-trace gap, with #63379 the corrected salvage of #17329; #31207 and #37724 overlap on all-timeout diagnostic artifacts, while #37368 is the narrower predecessor of #38983 for #37356. #65824 is complementary partial-output preservation, not a duplicate of the trace cluster. Suggested consolidationKeep #65824 open with a salvage path for its independently tested redacted partial-output preservation; its current diff addresses the visible async-rendering review, and it should remain separate from #17308's trace work. For #17312, author action: rebase onto main and replace the non-populating timeout path with the live-transcript, conservative-classifier implementation demonstrated by closed best-fix reference #63379, including its timeout tests; retain #17329 as its superseded predecessor and leave #31207/#37724 closed. Keep #38983 open with its worker-scoped session-isolation path because the revised diff explicitly addresses the contributor review, while #37368 remains closed as its narrower predecessor. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I17308(["issue #17308 (open)"])
P65824["PR #65824 (open)"]
P65824 -.->|partial| I17308
class I17308 open
class P65824 open
class P65824 target
click I17308 "https://github.com/NousResearch/hermes-agent/issues/17308"
click P65824 "https://github.com/NousResearch/hermes-agent/pull/65824"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 9 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 169 kB of PR diffs, 29 kB of issue/PR text, 22 kB of discussion (29 comments), 10 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Thanks for the cross-PR triage. Agreed: #65824 is complementary to, not a duplicate of, the #17308 structured last-tool trace cluster. Its scoped invariant is preserving a bounded, redact-before-truncate, fail-closed partial-output tail across synchronous, batch, and background-notification paths; last-tool state classification remains out of scope. The current head already includes the reviewed async rendering coverage, so this consolidation note does not require a code change. |
When a delegate_task child times out after completing real work, preserve a bounded tail of completed tool results so the parent can continue from verified evidence instead of receiving only an empty timeout summary. Redact each complete tool result with force=True before truncation, cap the evidence to 8 entries and 600 characters per preview, and fail closed at the timeout caller if sanitization cannot be completed. Preserve the legacy schema for timeouts before the first model call and for ordinary exception paths. Render the sanitized tail through the existing foreground and background notification paths. The original NousResearch#65824 already covered background batch and single-result rendering; this current-main rebuild preserves those paths rather than introducing them as a separate salvage feature. Author and implementation ownership remain with Charles Cha (@ypwcharles), from the original NousResearch#65824.
Review on NousResearch#84085 caught a real regression in the previous commit. `_extract_output_tail` is SHARED. It powers the ordinary delegation Output overlay on the non-timeout success path (delegate_tool.py, the cc-swarm-parity feature), not only the new timeout tail. The blanket `return []` on redaction failure meant a single redactor hiccup — a config issue or one pattern edge case — silently blanked the entire Output section for ALL delegations, where upstream degraded gracefully. Confirmed by reproduction before changing anything: with a redactor patched to raise, `_extract_output_tail` returned `[]` and the overlay lost output it had shown moments earlier with a healthy redactor. Add a `fail_closed` parameter, default False: - Default (display callers, ordinary overlay): best-effort. Skip only the tool result that could not be redacted and keep rendering the rest, so one bad entry cannot destroy a working feature. - `fail_closed=True` (timeout evidence call-site): unchanged hard fail. Evidence surfaced from a FAILED child is a security boundary, not a display nicety, so it emits nothing rather than risk an unredacted preview. Verified with a three-result fixture where the redactor raises on the middle one: the overlay keeps both healthy findings and drops the unredactable one, while the fail-closed caller emits nothing. Also address the review's test-gap suggestion with a truncation-boundary case. The first attempt was VACUOUS — it passed even under truncate-before-redact sabotage, because `sk-`/`ghp_`-style patterns still match after truncation. Searched for a case that actually bites: only patterns with a minimum-length quantifier are vulnerable. The Codex pattern `gAAAA[A-Za-z0-9_=-]{20,}` cut to 25 chars drops below the 20-char body minimum, stops matching, and leaks `credential gAAAABBBBBBBBB` in the clear. Note the redactor elides the middle of a matched token (`gAAAAB...BBBB`) rather than deleting it, so the test asserts on the surviving raw body run rather than absence of the prefix. Non-vacuity is proven: sabotaging the implementation to `redact(content[:max_chars])` fails this test. Tests: the previous fail-closed test is split into a default best-effort case and an explicit `fail_closed=True` case, plus the boundary test. `tests/tools/test_delegate.py` 81 passed; focused trio 168 passed / 0 failed. Full `tests/tools/` 6057 passed / 73 failed, the identical 73 failing on unmodified origin/main on this host — no new failures. The end-to-end timeout reproduction still returns `partial=True` with the child's real tool output. Co-authored-by: Charles Cha <92324143+ypwcharles@users.noreply.github.com>
Carry timeout partial_output_tail through the real single-background dispatch, completion-queue, durable-state, and process-notification path. Treat the runner result as untrusted at the event boundary: accept evidence only for timeout + partial results, validate every entry, retain at most the final 8 entries, force-redact complete previews before truncating to 600 characters, normalize untrusted tool labels, and fail closed on malformed data or redaction errors. Ordinary and non-timeout results retain the legacy event schema. Persist the sanitized event evidence for restart delivery, but omit the pre-sanitized partial_output_tail from durable result_json so status reads cannot re-expose raw runner data. Add transport- and durable-level RED/GREEN regressions covering the valid path, timeout-only scoping, malformed input, redaction and bounds, redactor failure, and durable-state sanitization.
dab408e to
f02bbbd
Compare
|
Rebased the canonical PR onto current The final three-commit range keeps the original implementation and follow-up hardening separately attributable. Independent pre-push review also found a single-background transport gap; the third commit fixes that path with timeout-only, re-redacted, bounded, fail-closed event transport while preserving the legacy schema for ordinary results. Validation on the final head:
#84085 can now close in favor of this canonical consolidation; its genuine incremental work remains credited in git history. |
What does this PR do?
When a
delegate_taskchild times out after completing real work, the parent currently receives a timeout entry with no summary and loses every completed tool result. A research child can finish severalweb_search,x_search, or terminal calls, hang on a final step, and leave the parent with nothing to continue from even though useful work existed.This PR preserves a bounded, redacted snapshot of completed child tool outputs on the timeout path and returns it as
partial: truepluspartial_output_tail[]. It also renders that evidence through the async/background notification path used bydelegate_task(background=true).Attribution and consolidation
main; it keeps both contributors' git authorship intact.What changed
Timeout evidence
partial: truepartial_output_tail: [{tool, preview, is_error}, ...]Security boundary
redact_sensitive_text(..., force=True)to the complete output before truncation. This prevents truncation from splitting a credential into a fragment that no longer matches the redactor.fail_closed=True: any redaction failure drops the entire tail.Background delivery
partial_output_tailentries in async batch and single-result completion formatting.async_delegationcompletion-event boundary.result_json/ status reads.format_process_notification()route used to re-inject background delegation results into the parent.Scope guards
before_first_llm_call) diagnostic behavior is unchanged.Schema
{ "status": "timeout", "summary": null, "error": "...", "diagnostic_path": "...", "partial": true, "partial_output_tail": [ { "tool": "terminal", "preview": "Build succeeded...", "is_error": false } ] }How to test
Validated on Ubuntu 24.04 / WSL2 with Python 3.11.15.
Focused canonical runner on the final three-commit rebased head:
Result: 205 passed, 0 failed, 4 skipped. The skipped cases are Windows-only and run in the Windows CI lane.
The suite includes real single-background dispatch → queue → durable-state → notification coverage, malformed-tail and non-timeout legacy-schema coverage, forced redaction/bounds coverage, and redactor-failure fail-closed coverage. All new transport/security regressions were observed failing for the expected reason before the production fix, then passing after it.
Static gates:
Result: all passed.
Full canonical suite was also executed. It reported 31 files with 92 failures, plus 2 files that did not complete collection/run, all outside this PR's six changed files. This isolated worktree intentionally reused the runtime dependency set and added only pytest tooling, so optional-provider/environment failures were not treated as PR regressions. The related delegation, timeout, and process suites above remained clean.
Related work
Checklist