fix(cron): suppress stale descendant subagent fallback summaries#44205
fix(cron): suppress stale descendant subagent fallback summaries#44205dddabtc wants to merge 3 commits into
Conversation
openclaw-barnacle
Bot
added
channel: telegram
Greptile SummaryThis PR fixes the cron isolated-agent fallback logic to suppress stale descendant subagent summaries by returning only the freshest settled descendant reply, rather than concatenating up to four older replies that may contain outdated information. It also introduces a dedicated Key changes:
One minor style note: the Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cron/isolated-agent/subagent-followup.ts
Line: 83-85
Comment:
**Redundant sort on already-ordered map values**
Since `descendants` is already sorted descending by `endedAt` (line 69), and `latestByChild` is populated by iterating that sorted array with first-occurrence-wins, `latestByChild.values()` is already in descending `endedAt` order (Map preserves insertion order in JS). The `.toSorted()` here is a no-op and can be dropped to avoid misleading future readers into thinking a re-sort is needed.
```suggestion
const latestRuns = [...latestByChild.values()];
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 999cb72 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const latestRuns = [...latestByChild.values()].toSorted( | ||
| (a, b) => (b.endedAt ?? 0) - (a.endedAt ?? 0), | ||
| ); |
There was a problem hiding this comment.
Redundant sort on already-ordered map values
Since descendants is already sorted descending by endedAt (line 69), and latestByChild is populated by iterating that sorted array with first-occurrence-wins, latestByChild.values() is already in descending endedAt order (Map preserves insertion order in JS). The .toSorted() here is a no-op and can be dropped to avoid misleading future readers into thinking a re-sort is needed.
| const latestRuns = [...latestByChild.values()].toSorted( | |
| (a, b) => (b.endedAt ?? 0) - (a.endedAt ?? 0), | |
| ); | |
| const latestRuns = [...latestByChild.values()]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/isolated-agent/subagent-followup.ts
Line: 83-85
Comment:
**Redundant sort on already-ordered map values**
Since `descendants` is already sorted descending by `endedAt` (line 69), and `latestByChild` is populated by iterating that sorted array with first-occurrence-wins, `latestByChild.values()` is already in descending `endedAt` order (Map preserves insertion order in JS). The `.toSorted()` here is a no-op and can be dropped to avoid misleading future readers into thinking a re-sort is needed.
```suggestion
const latestRuns = [...latestByChild.values()];
```
How can I resolve this? If you propose a fix, please make it concise.
999cb72 to
dc6b8b0
Compare
openclaw-barnacle
Bot
added
size: S
and removed
channel: telegram
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc6b8b0087
ℹ️ 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".
| if (!childKey || latestByChild.has(childKey)) { | ||
| continue; |
There was a problem hiding this comment.
This has(childKey) early-continue keeps the first run seen for a child and drops later ones, which is a regression when two runs share the same endedAt millisecond. listDescendantRunsForRequester preserves insertion order from the registry map (older runs first), and toSorted is stable for equal keys, so equal-endedAt ties now keep the older run and can surface stale fallback text instead of the freshest run. Reintroduce replacement-on-tie behavior (or a secondary tie-breaker like createdAt) so equal timestamps still select the newest record.
Useful? React with 👍 / 👎.
83fcab8 to
dc6b8b0
Compare
dc6b8b0 to
7d2d95a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d2d95aa28
ℹ️ 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".
| if (!childKey || latestByChild.has(childKey)) { | ||
| continue; |
There was a problem hiding this comment.
When two runs for the same child share the same endedAt millisecond, this dedupe path keeps the first entry seen and drops later ones, which can surface stale frozenResultText instead of the most recent result. Fresh evidence in this commit is the new latestByChild.has(childKey) early-continue after a stable toSorted pass, so equal-timestamp ties preserve insertion order from the registry and the newer run never replaces the older one. Please keep replacement-on-tie behavior (or add a secondary sort key like createdAt) so equal endedAt still resolves to the newest run.
Useful? React with 👍 / 👎.
# Conflicts: # src/cron/isolated-agent/subagent-followup.test.ts
7d2d95a to
825a8be
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 825a8bec60
ℹ️ 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".
| // Fallback mode should surface the freshest settled descendant result only. | ||
| // Older descendant summaries are easily stale/obsolete and can pollute | ||
| // current-status reminders when multiple subagents were used in one workflow. | ||
| return reply; |
There was a problem hiding this comment.
readDescendantSubagentFallbackReply now returns the first non-NO_REPLY descendant message, but it does not filter out interim acknowledgements like "on it" before return reply. In runs where the freshest descendant ended with an interim status while an older descendant has real output, this function will return the interim text and delivery-dispatch uses it directly as the final delivery payload (src/cron/isolated-agent/delivery-dispatch.ts:325-359), so users can receive placeholder text instead of an actual summary.
Useful? React with 👍 / 👎.
|
Addressed all 3 review comments in 64bfa2c:
All 25 tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64bfa2cd80
ℹ️ 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".
| // Fallback mode should surface the freshest settled descendant result only. | ||
| // Older descendant summaries are easily stale/obsolete and can pollute | ||
| // current-status reminders when multiple subagents were used in one workflow. | ||
| return reply; |
There was a problem hiding this comment.
In fan-out cron runs (the code explicitly expects cases like both subagents are running), this helper is the last-resort path when the parent never posts a synthesis. Returning on the first usable entry means src/cron/isolated-agent/delivery-dispatch.ts:325-359 delivers only the newest child's reply and silently drops the other settled children, so a cron job that split complementary work across two subagents can now send a partial answer. The per-child dedupe above is useful, but this fallback should still aggregate the latest usable reply from each distinct child instead of stopping after the first one.
Useful? React with 👍 / 👎.
| if (isLikelyInterimCronMessage(reply)) { | ||
| continue; |
There was a problem hiding this comment.
isLikelyInterimCronMessage is tuned for short cron acknowledgements, but here it is applied to arbitrary child-session output. Any real descendant result under 45 words that happens to contain one of the hint substrings (for example retrying now or should have your summary) gets discarded, and when src/cron/isolated-agent/delivery-dispatch.ts:325-359 asks for a completed descendant reply because the parent text is still on it, that can leave the stale parent message as the only deliverable text. This filter needs to be stricter here than a broad substring match.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. source inspection gives a high-confidence reproduction path for the PR regression: current main has a focused multi-child aggregation test, while the PR replaces that expectation and returns after the first usable descendant reply. Next step before merge Security Review findings
Review detailsBest possible solution: Keep latest-run-per-child selection, aggregate one usable latest reply per distinct child, narrow acknowledgement filtering to true child acknowledgements, and add the required changelog entry. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path for the PR regression: current main has a focused multi-child aggregation test, while the PR replaces that expectation and returns after the first usable descendant reply. Is this the best way to solve the issue? No. Selecting the newest run per child is useful, but the maintainable fix should preserve distinct-child fan-out aggregation and avoid applying the broad cron interim heuristic to arbitrary child output. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a90be474f441. |
1 participant
Summary
Testing