feat(cron): inject output history into isolated agent prompts#44957
feat(cron): inject output history into isolated agent prompts#44957Zynoo71 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
openclaw-barnacle
Bot
added
app: web-ui
Greptile SummaryThis PR adds a per-job Key issue to resolve before merging:
Minor:
Confidence Score: 2/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 344-349
Comment:
**Output history never recorded for `delivery.mode: "none"` jobs**
The recording gate requires `result.delivered === true`, but `delivered` is only ever set to `true` inside the `if (params.deliveryRequested …)` branch of `dispatchCronDelivery`. When a job uses `delivery.mode: "none"`, `deliveryRequested` is `false`, that branch is never entered, and `delivered` stays `false`. The consequence is that `outputHistory: true` is silently inert for any job that does not deliver to a channel — including the "Monitoring" use case explicitly listed in the PR description.
The integration tests in `service.output-history-recording.test.ts` all build jobs with `delivery: { mode: "none" }` but then mock `runIsolatedAgentJob` to return `delivered: true`, which does not reflect what the real code path produces for that delivery mode, so the tests do not catch this gap.
One approach is to record history on a successful run regardless of delivery, or only gate on `result.status === "ok"` with a non-empty output:
```ts
// Record output for history when enabled and the run succeeded (even without delivery).
if (
result.status === "ok" &&
result.outputText?.trim() &&
job.payload.kind === "agentTurn" &&
job.payload.outputHistory
) {
```
If the intent really is to only record *delivered* outputs, the feature description, tool docs, and PR description should call this out explicitly, and the "Monitoring" use case should be updated to require a delivery channel.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cron/isolated-agent/run.ts
Line: 484-487
Comment:
**Output history appended after delivery instructions**
`commandBody` at this point already contains the delivery instruction (e.g. "Return your summary as plain text…"). Appending the output history block after the delivery instruction means the previous-outputs context lands at the very end of the prompt, after the agent has already received its marching orders. In practice this is fine, but many prompting guides recommend placing context *before* instructions. Consider injecting the history block before `appendCronDeliveryInstruction` so the ordering is: task → history context → delivery instruction.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 2c6068b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| result.delivered && | ||
| result.outputText?.trim() && | ||
| job.payload.kind === "agentTurn" && | ||
| job.payload.outputHistory | ||
| ) { |
There was a problem hiding this comment.
Output history never recorded for delivery.mode: "none" jobs
The recording gate requires result.delivered === true, but delivered is only ever set to true inside the if (params.deliveryRequested …) branch of dispatchCronDelivery. When a job uses delivery.mode: "none", deliveryRequested is false, that branch is never entered, and delivered stays false. The consequence is that outputHistory: true is silently inert for any job that does not deliver to a channel — including the "Monitoring" use case explicitly listed in the PR description.
The integration tests in service.output-history-recording.test.ts all build jobs with delivery: { mode: "none" } but then mock runIsolatedAgentJob to return delivered: true, which does not reflect what the real code path produces for that delivery mode, so the tests do not catch this gap.
One approach is to record history on a successful run regardless of delivery, or only gate on result.status === "ok" with a non-empty output:
// Record output for history when enabled and the run succeeded (even without delivery).
if (
result.status === "ok" &&
result.outputText?.trim() &&
job.payload.kind === "agentTurn" &&
job.payload.outputHistory
) {If the intent really is to only record delivered outputs, the feature description, tool docs, and PR description should call this out explicitly, and the "Monitoring" use case should be updated to require a delivery channel.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 344-349
Comment:
**Output history never recorded for `delivery.mode: "none"` jobs**
The recording gate requires `result.delivered === true`, but `delivered` is only ever set to `true` inside the `if (params.deliveryRequested …)` branch of `dispatchCronDelivery`. When a job uses `delivery.mode: "none"`, `deliveryRequested` is `false`, that branch is never entered, and `delivered` stays `false`. The consequence is that `outputHistory: true` is silently inert for any job that does not deliver to a channel — including the "Monitoring" use case explicitly listed in the PR description.
The integration tests in `service.output-history-recording.test.ts` all build jobs with `delivery: { mode: "none" }` but then mock `runIsolatedAgentJob` to return `delivered: true`, which does not reflect what the real code path produces for that delivery mode, so the tests do not catch this gap.
One approach is to record history on a successful run regardless of delivery, or only gate on `result.status === "ok"` with a non-empty output:
```ts
// Record output for history when enabled and the run succeeded (even without delivery).
if (
result.status === "ok" &&
result.outputText?.trim() &&
job.payload.kind === "agentTurn" &&
job.payload.outputHistory
) {
```
If the intent really is to only record *delivered* outputs, the feature description, tool docs, and PR description should call this out explicitly, and the "Monitoring" use case should be updated to require a delivery channel.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in ce3675de6 — recording gate changed from result.delivered to result.status === "ok", so output history works regardless of delivery mode. Updated integration test to verify error runs are not recorded.
| const outputHistoryBlock = buildOutputHistoryBlock(params.job); | ||
| if (outputHistoryBlock) { | ||
| commandBody = `${commandBody}\n\n${outputHistoryBlock}`; | ||
| } |
There was a problem hiding this comment.
Output history appended after delivery instructions
commandBody at this point already contains the delivery instruction (e.g. "Return your summary as plain text…"). Appending the output history block after the delivery instruction means the previous-outputs context lands at the very end of the prompt, after the agent has already received its marching orders. In practice this is fine, but many prompting guides recommend placing context before instructions. Consider injecting the history block before appendCronDeliveryInstruction so the ordering is: task → history context → delivery instruction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/isolated-agent/run.ts
Line: 484-487
Comment:
**Output history appended after delivery instructions**
`commandBody` at this point already contains the delivery instruction (e.g. "Return your summary as plain text…"). Appending the output history block after the delivery instruction means the previous-outputs context lands at the very end of the prompt, after the agent has already received its marching orders. In practice this is fine, but many prompting guides recommend placing context *before* instructions. Consider injecting the history block before `appendCronDeliveryInstruction` so the ordering is: task → history context → delivery instruction.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in ce3675de6 — history block now injected before appendCronDeliveryInstruction, so prompt order is: task → history context → delivery instruction.
![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: 2c6068bfcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -492,6 +516,7 @@ function applyOutcomeToStoredJob(state: CronServiceState, result: TimedCronRunOu | |||
| status: result.status, | |||
| error: result.error, | |||
| delivered: result.delivered, | |||
| outputText: result.outputText, | |||
There was a problem hiding this comment.
applyJobResult now records recentOutputs from result.outputText, but startup catch-up runs still construct TimedCronRunOutcome without copying outputText from executeJobCoreWithTimeout (in runStartupCatchupCandidate). When cron restarts and executes missed isolated jobs via startup catch-up, this field reaches applyJobResult as undefined, so output-history recording is skipped even if payload.outputHistory is enabled and delivery succeeded; the next run then loses the expected prior-output context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ce3675de6 — startup catch-up now uses spread ({ jobId, ...result, startedAt, endedAt }) matching the normal execution path, so outputText is propagated.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
2c6068b to
ce3675d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3675de60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| result.status === "ok" && | ||
| result.outputText?.trim() && | ||
| job.payload.kind === "agentTurn" && | ||
| job.payload.outputHistory |
There was a problem hiding this comment.
applyJobResult records recentOutputs for every status === "ok" run, but isolated runs can be ok with delivered: false (for example when dispatchCronDelivery falls back in best-effort delivery-target failures or when delivery is suppressed). In those cases nothing was actually delivered, yet the next run will treat that text as prior output context, which can cause missed updates/repetition avoidance based on content users never received. Please require a successful delivery outcome before appending to recentOutputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping result.status === "ok" as the sole gate, intentionally not checking delivered.
The output history serves the agent's memory, not the user's inbox. If we gate on delivery, this happens:
- Run 1: agent produces "Market up 2%", delivery fails → not recorded
- Run 2: agent has no memory, produces identical "Market up 2%" → delivery succeeds
This is exactly the repetition problem output history exists to solve. Delivery failure is an infrastructure concern — it should be handled by retry/alerting, not by making the agent forget what it produced.
Updated in a9a200b9f with explicit comment explaining the design decision.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
ce3675d to
a9a200b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9a200b9f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const outputHistoryBlock = buildOutputHistoryBlock(params.job); | ||
| if (outputHistoryBlock) { | ||
| commandBody = `${commandBody}\n\n${outputHistoryBlock}`; |
There was a problem hiding this comment.
In runCronIsolatedAgentTurn, external-hook messages are safety-wrapped via buildSafeExternalPrompt, but the new outputHistoryBlock is appended afterward as raw prompt text. For hook-driven jobs with outputHistory enabled, prior agent outputs can contain attacker-controlled text echoed from earlier external inputs; appending that text outside the guard reintroduces untrusted instructions in a privileged prompt position on later runs. Please sanitize/quote history entries or route them through the same untrusted-content boundary before concatenation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 779a1dafc — for external-hook jobs (shouldWrapExternal), output history is now included inside the buildSafeExternalPrompt content, so it goes through the same security boundary as the hook message. For internal/trusted jobs, history is appended directly as before.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
2 times, most recently
from
779a1da to
ad7257f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad7257fb17
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Type.Array( | ||
| Type.Object( | ||
| { | ||
| text: Type.String(), | ||
| timestamp: Type.Integer({ minimum: 0 }), |
There was a problem hiding this comment.
recentOutputs is defined as an unbounded array of unbounded strings, and CronJobPatchSchema permits direct state updates, so callers can bypass the runtime FIFO/truncation logic and store arbitrarily large history blobs; buildOutputHistoryBlock then injects all entries into the isolated-agent prompt, which can exceed context windows and break scheduled runs or cause unexpectedly high token usage. Enforce the same invariants in schema/normalization (max 5 items, max text length) before accepting state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89f73e69a — buildOutputHistoryBlock now applies defense-in-depth: slices to last 5 entries via slice(-OUTPUT_HISTORY_MAX_ENTRIES) and re-truncates each text via truncateOutputForHistory() at consumption time, preventing prompt blow-ups even if state was patched directly.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
2 times, most recently
from
2ee9029 to
89f73e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89f73e69a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const shouldRecordHistory = | ||
| result.status === "ok" && | ||
| outputText && | ||
| job.payload.kind === "agentTurn" && | ||
| job.payload.outputHistory; |
There was a problem hiding this comment.
The new history gate now requires result.outputText to append state.recentOutputs, but manual executions still call applyJobResult from finishPreparedManualRun without passing coreResult.outputText (src/cron/service/ops.ts, applyJobResult call). This means cron.run (force/due) jobs with payload.outputHistory=true never record the latest output, so the next scheduled run misses that context even though the run itself succeeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8fdd3ac9 — finishPreparedManualRun now spreads coreResult (same pattern as normal execution path), so outputText is propagated to applyJobResult for manual runs too.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
89f73e6 to
d8fdd3a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8fdd3ac99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { "kind": "agentTurn", "message": "<prompt>", "model": "<optional>", "thinking": "<optional>", "timeoutSeconds": <optional, 0 means no timeout>, "outputHistory": <optional-bool> } | ||
| - outputHistory: set true to include recent delivery outputs as context for the agent. Useful for any recurring task where seeing previous outputs helps — avoiding repetition, tracking changes, building on prior results, etc. Default: false. |
There was a problem hiding this comment.
The new tool docs advertise payload.outputHistory, but the add flat-params recovery path (used for non-frontier models that flatten arguments, per the #11310 logic in this file) still does not carry outputHistory into the reconstructed job payload. In that scenario, calls like action=add with top-level message/schedule and outputHistory: true silently create jobs without the flag, so output-history recording/prompt injection never activates and recurring jobs keep repeating content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f743b324e — added outputHistory to both JOB_KEYS (add flat-params) and PATCH_KEYS (edit flat-params) so non-frontier models that flatten arguments correctly propagate the flag.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
d8fdd3a to
f743b32
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f743b324ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -474,6 +476,7 @@ Use jobId as the canonical identifier; id is accepted for compatibility. Use con | |||
| "wakeMode", | |||
| "failureAlert", | |||
| "allowUnsafeExternalContent", | |||
| "outputHistory", | |||
There was a problem hiding this comment.
Including "outputHistory" in the flat PATCH_KEYS list makes cron.update accept flattened model output, but that value is never copied into patch.payload (unlike model/thinking/timeoutSeconds), so the update becomes a silent no-op. In the non-frontier flattening scenario this recovery path targets, requests like action=update, jobId=..., outputHistory=true now pass validation yet leave payload.outputHistory unchanged, so history injection never activates even though the tool call appears successful.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96f31ba60 — flat-params recovery now moves outputHistory into payload: { kind: "agentTurn", outputHistory } before normalization, so both cron.add and cron.update correctly place it inside the payload. Also added outputHistory to copyTopLevelAgentTurnFields and stripLegacyTopLevelFields in normalize.ts for defense-in-depth. Tests updated to verify params.payload.outputHistory (not top-level).
Zynoo71
force-pushed
the
feat/cron-output-history
branch
2 times, most recently
from
4d58687 to
96f31ba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d5868798a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -331,6 +332,7 @@ Use jobId as the canonical identifier; id is accepted for compatibility. Use con | |||
| "thinking", | |||
| "timeoutSeconds", | |||
| "allowUnsafeExternalContent", | |||
| "outputHistory", | |||
There was a problem hiding this comment.
Adding "outputHistory" to the flat add JOB_KEYS causes flattened tool calls to forward outputHistory at the job root, but normalizeCronJobCreate only promotes a small set of top-level agent-turn fields (model/thinking/timeoutSeconds/allowUnsafeExternalContent) and leaves outputHistory untouched, so the gateway sees an unknown top-level property and rejects the request under CronAddParamsSchema (additionalProperties: false). Fresh evidence after the earlier thread: tracing this commit’s new key through normalizeCronJobCreate shows no path that maps it to payload.outputHistory, so non-frontier flattened cron.add calls with outputHistory now fail instead of enabling history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also fixed in the same commit — same approach applied to both JOB_KEYS (add) and PATCH_KEYS (update) recovery paths.
| @@ -474,6 +476,7 @@ Use jobId as the canonical identifier; id is accepted for compatibility. Use con | |||
| "wakeMode", | |||
| "failureAlert", | |||
| "allowUnsafeExternalContent", | |||
| "outputHistory", | |||
There was a problem hiding this comment.
Including "outputHistory" in flat update PATCH_KEYS builds params.patch = { outputHistory: true }, but that field is not converted into patch.payload.outputHistory during normalization, so it remains an unsupported top-level patch property and fails gateway validation (CronJobPatchSchema also uses additionalProperties: false). In the flattened-model scenario this was meant to recover, cron.update now returns an invalid-params error instead of updating the flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f31ba60d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!payload.kind) { | ||
| payload.kind = "agentTurn"; | ||
| } |
There was a problem hiding this comment.
When flat-params recovery sees top-level outputHistory, this block unconditionally injects payload.kind = "agentTurn" if kind is missing. In the same recovery path, non-frontier models may send valid system-event style fields (text, schedule, etc.) plus optional booleans; forcing agentTurn causes normalizeCronJobCreate to drop top-level text and send an agentTurn payload without message, which then fails gateway validation (CronPayloadSchema requires agentTurn.message). This turns previously recoverable flattened cron.add calls into hard errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b0db90bee — both add and update recovery paths now check for systemEvent signals (existingPayload?.kind === "systemEvent" or typeof synthetic.text === "string") and skip the agentTurn promotion when present, silently dropping outputHistory (meaningless for systemEvent).
| if (!payload.kind) { | ||
| payload.kind = "agentTurn"; | ||
| } |
There was a problem hiding this comment.
The flat update recovery does the same unconditional payload.kind = "agentTurn" promotion for top-level outputHistory, which can break unrelated updates when models flatten optional fields. For system-event jobs, this coerces a kind switch and applyCronJobPatch ends up in buildPayloadFromPatch without a message, throwing cron.update payload.kind="agentTurn" requires message; for non-payload updates this means a benign update can fail solely because outputHistory was included as an extra flattened field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same fix applied — see reply above.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
96f31ba to
b0db90b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0db90bee4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!payload.kind) { | ||
| payload.kind = "agentTurn"; |
There was a problem hiding this comment.
In flat cron.add recovery, this branch sets payload.kind = "agentTurn" whenever outputHistory is present and synthetic.text is not top-level, but it does not recognize synthetic.payload.text as a system-event signal. If a non-frontier model sends partially nested args like payload: { text: "..." } plus top-level outputHistory: true, recovery rewrites that payload to agentTurn without a message, and the subsequent cron.add request is rejected by gateway validation (agentTurn.message required), so a previously recoverable flattened add call now fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57c1d71ed — removed the payload wrapper from the add recovery path entirely. For add, outputHistory now stays at the top level and the normalizer handles it: message auto-creates the payload, then copyTopLevelAgentTurnFields copies outputHistory in, then stripLegacyTopLevelFields cleans up. This avoids all kind-forcing issues and preserves message. The update path still uses a payload wrapper (no message to trigger auto-creation). Test now also verifies message survives.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
b0db90b to
57c1d71
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57c1d71edf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isSystemEvent = existingPayload?.kind === "systemEvent"; | ||
| if (!isSystemEvent) { |
There was a problem hiding this comment.
The flat cron.update recovery only treats payload.kind === "systemEvent" as a system-event signal, so common lowercase/mixed-case variants (for example "systemevent") still enter the !isSystemEvent branch and get outputHistory injected into the payload. normalizeCronJobPatch then canonicalizes the kind to systemEvent but keeps outputHistory, which violates the systemEvent schema (additionalProperties: false) and makes an otherwise recoverable flattened update fail validation instead of ignoring the extra flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 271d13f3f — systemEvent guard now normalizes kind with .trim().toLowerCase() before comparison, matching what coercePayload does.
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
57c1d71 to
271d13f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 271d13f3fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof payload.outputHistory !== "boolean" && typeof next.outputHistory === "boolean") { | ||
| payload.outputHistory = next.outputHistory; |
There was a problem hiding this comment.
Adding outputHistory support here only copies top-level fields into an already-agentTurn payload, but coercePayload still doesn't treat outputHistory as an agent-turn hint. As a result, updates like cron.update with patch: { payload: { outputHistory: true } } normalize to a kindless payload and are then rejected by CronPayloadPatchSchema (which requires kind), so enabling history via nested patch fails even though other agent-turn-only patch fields are kind-inferred.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 10eef11fb — added typeof next.outputHistory === "boolean" to hasAgentTurnHint in coercePayload, consistent with allowUnsafeExternalContent. Nested patches like { payload: { outputHistory: true } } now correctly infer kind: "agentTurn".
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
271d13f to
10eef11
Compare
Isolated cron sessions start fresh each run (forceNew: true), so the agent has no memory of previous outputs. This makes recurring tasks produce repetitive content and prevents change tracking or iterative analysis. Add per-job `outputHistory` flag that, when enabled, records outputs in job state (capped at 5, FIFO) and injects them into the agent's system prompt on subsequent runs. Recording gates on `status === "ok"` regardless of delivery outcome — the agent should remember what it produced even if delivery failed, otherwise it repeats itself when delivery recovers. For external-hook jobs, output history is included inside the security boundary (buildSafeExternalPrompt) to prevent prior outputs from bypassing the untrusted-content guard. Defense-in-depth: buildOutputHistoryBlock caps entries and truncates text at consumption time, preventing prompt blow-ups even if state was patched directly bypassing runtime guards. Concrete use cases: - Daily briefings: avoid repeating the same news/updates - Monitoring: track metric changes between runs - Iterative analysis: build on previous findings - Content scheduling: maintain variety across outputs Changes: - Add `outputHistory` field to CronAgentTurnPayloadFields - Add `recentOutputs` array to CronJobState - New output-history.ts: buildOutputHistoryBlock + truncateOutputForHistory (head+tail truncation within 600 char limit) - Inject history block before delivery instruction in run.ts - Route history through security boundary for external-hook jobs - Record output on successful run in timer.ts applyJobResult - Propagate outputText in startup catch-up and manual run paths - Add outputHistory to flat-params recovery (JOB_KEYS + PATCH_KEYS) - Support outputHistory in mergeCronPayload/buildPayloadFromPatch - Guard against invalid timezone in output formatting - Consumption-side bounds on entries and text length - Update gateway protocol schema and cron tool description - Unit, integration, and e2e tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Zynoo71
force-pushed
the
feat/cron-output-history
branch
from
10eef11 to
5ed79b2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed79b2456
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const outputText = result.outputText?.trim(); | ||
| const shouldRecordHistory = | ||
| result.status === "ok" && | ||
| outputText && | ||
| job.payload.kind === "agentTurn" && |
There was a problem hiding this comment.
This now appends any successful result.outputText to history, but runCronIsolatedAgentTurn fills outputText from the last non-empty payload before it decides to suppress user-visible delivery (src/cron/isolated-agent/helpers.ts:109-116, src/cron/isolated-agent/run.ts:733-760) and even returns the NO_REPLY sentinel unchanged in dispatchCronDelivery (src/cron/isolated-agent/delivery-dispatch.ts:540-547). For jobs with payload.outputHistory=true, a no-op run can therefore store HEARTBEAT_OK/NO_REPLY as the “previous output”, so the next prompt sees control text instead of the last substantive result.
Useful? React with 👍 / 👎.
| typeof next.model === "string" || | ||
| typeof next.thinking === "string" || | ||
| typeof next.timeoutSeconds === "number" || | ||
| typeof next.allowUnsafeExternalContent === "boolean"; | ||
| typeof next.allowUnsafeExternalContent === "boolean" || | ||
| typeof next.outputHistory === "boolean"; |
There was a problem hiding this comment.
coercePayload now recognizes outputHistory as an agent-turn hint, but unlike allowUnsafeExternalContent it never strips or coerces non-boolean values before returning the payload. In the same malformed-model scenarios this recovery path is meant to handle, inputs like payload: { kind: "agentTurn", message: "...", outputHistory: "true" } survive normalization and are then rejected by CronAddParamsSchema/CronJobPatchSchema, because the gateway schema requires a real boolean. That turns add/update calls into validation errors instead of recovering from a common stringified-boolean shape.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
|
Codex review: needs changes before merge. What this changes: The PR adds an opt-in cron Required change before merge: A repair worker can make a narrow replacement or updated PR: rebase the useful source work, filter silent/control output before history recording, normalize the new field consistently, add changelog coverage, and run the targeted cron/protocol tests. Security review: Security review cleared: The diff does not change dependencies, workflows, secrets, permissions, or package execution paths; the prompt-state issue is covered as a functional finding. Review findings:
Review detailsBest possible solution: Land a rebased or replacement version of the feature that keeps the opt-in API but filters non-substantive control replies before recording history, preserves current prompt/delivery contracts, normalizes the new field consistently, and documents the user-facing cron behavior in the changelog. Do we have a high-confidence way to reproduce the issue? Yes. A source-level reproduction is clear: an isolated cron run that returns only Is this the best way to solve the issue? No, not as-is. The feature direction is maintainable, but the current patch needs a current-main rebase plus output filtering, normalization cleanup, and changelog coverage before it is the narrow safe solution. 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 62e1be2b9804. |
1 participant
Problem
Isolated cron sessions start fresh each run (
forceNew: true), so the agent has zero memory of previous outputs. This causes:Solution
Add a per-job
outputHistoryflag. When enabled:job.state.recentOutputs(capped at 5 entries, FIFO)Use cases
Changes
outputHistoryfield toCronAgentTurnPayloadFieldsandrecentOutputstoCronJobStateoutput-history.ts:buildOutputHistoryBlock+truncateOutputForHistoryrun.tstimer.tsapplyJobResultoutputHistoryinmergeCronPayload/buildPayloadFromPatchTest plan
buildOutputHistoryBlockguard conditions, formatting, timezone fallback, truncation boundsrunCronIsolatedAgentTurncode path🤖 Generated with Claude Code