infra: fix heartbeat directive preservation and global enablement#74471
infra: fix heartbeat directive preservation and global enablement#74471steipete merged 6 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes two heartbeat bugs (regex stripping of non-task directives; global default enablement) and migrates three modules from
Confidence Score: 3/5Hold for the heartbeat-runner regex edge case — blank-line-separated task entries will pollute the generated prompt with raw task YAML. One P1 logic bug in the newly introduced regex: the ` ` stop condition truncates the replacement inside a multi-entry tasks block, causing task definitions to survive into the "Additional context" section of the heartbeat prompt. The remaining changes (logging migrations, heartbeat-summary fix, dotenv import ordering) are low-risk P2 or clean. src/infra/heartbeat-runner.ts — the updated regex on line 746 Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/dotenv.ts
Line: 8-9
Comment:
**`const` declaration split between import statements**
`const logger = createSubsystemLogger("infra:dotenv")` is placed between two `import` groups (lines 6 and 9). While JS/TS hoists all `import` declarations so this works at runtime, the interleaved `const` will trip the `import/first` or `import/order` lint rules and is confusing to readers. Move the logger initialisation after all imports.
```suggestion
import { resolveRequiredHomeDir } from "./home-dir.js";
const logger = createSubsystemLogger("infra:dotenv");
```
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/infra/heartbeat-runner.ts
Line: 746
Comment:
**Regex stops early when tasks block contains blank lines**
The `\n\n` alternative in the lookahead `(?=^[^\s]|\n\n|$)` causes the replacement to stop at the first blank line found *inside* the `tasks:` block. If `HEARTBEAT.md` uses blank lines between task entries (common in YAML-style lists), the remaining task entries are left in `directives` and end up appended to the prompt as "Additional context", which is the opposite of the intended behaviour.
For example, with a file like:
```
tasks:
- name: task1
prompt: do thing 1
- name: task2
prompt: do thing 2
Some global directive
```
The regex only removes through `do thing 1`, leaving `- name: task2 …` in `directives`. Consider dropping the `\n\n` alternative and relying solely on the `^[^\s]` anchor (next top-level key) to terminate the match.
```suggestion
.replace(/^tasks:[\s\S]*?(?=^[^\s]|$(?![\s\S]))/m, "")
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "logging: migrate dotenv and temp-downloa..." | Re-trigger Greptile |
| const logger = createSubsystemLogger("infra:dotenv"); | ||
| import { resolveRequiredHomeDir } from "./home-dir.js"; |
There was a problem hiding this comment.
const declaration split between import statements
const logger = createSubsystemLogger("infra:dotenv") is placed between two import groups (lines 6 and 9). While JS/TS hoists all import declarations so this works at runtime, the interleaved const will trip the import/first or import/order lint rules and is confusing to readers. Move the logger initialisation after all imports.
| const logger = createSubsystemLogger("infra:dotenv"); | |
| import { resolveRequiredHomeDir } from "./home-dir.js"; | |
| import { resolveRequiredHomeDir } from "./home-dir.js"; | |
| const logger = createSubsystemLogger("infra:dotenv"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/dotenv.ts
Line: 8-9
Comment:
**`const` declaration split between import statements**
`const logger = createSubsystemLogger("infra:dotenv")` is placed between two `import` groups (lines 6 and 9). While JS/TS hoists all `import` declarations so this works at runtime, the interleaved `const` will trip the `import/first` or `import/order` lint rules and is confusing to readers. Move the logger initialisation after all imports.
```suggestion
import { resolveRequiredHomeDir } from "./home-dir.js";
const logger = createSubsystemLogger("infra:dotenv");
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -743,7 +743,7 @@ After completing all due tasks, reply HEARTBEAT_OK.`; | |||
|
|
|||
| if (params.heartbeatFileContent) { | |||
| const directives = params.heartbeatFileContent | |||
| .replace(/^[\s\S]*?^tasks:[\s\S]*?(?=^[^\s]|^$)/m, "") | |||
| .replace(/^tasks:[\s\S]*?(?=^[^\s]|\n\n|$)/m, "") | |||
There was a problem hiding this comment.
The \n\n alternative in the lookahead (?=^[^\s]|\n\n|$) causes the replacement to stop at the first blank line found inside the tasks: block. If HEARTBEAT.md uses blank lines between task entries (common in YAML-style lists), the remaining task entries are left in directives and end up appended to the prompt as "Additional context", which is the opposite of the intended behaviour.
For example, with a file like:
tasks:
- name: task1
prompt: do thing 1
- name: task2
prompt: do thing 2
Some global directive
The regex only removes through do thing 1, leaving - name: task2 … in directives. Consider dropping the \n\n alternative and relying solely on the ^[^\s] anchor (next top-level key) to terminate the match.
| .replace(/^tasks:[\s\S]*?(?=^[^\s]|\n\n|$)/m, "") | |
| .replace(/^tasks:[\s\S]*?(?=^[^\s]|$(?![\s\S]))/m, "") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/heartbeat-runner.ts
Line: 746
Comment:
**Regex stops early when tasks block contains blank lines**
The `\n\n` alternative in the lookahead `(?=^[^\s]|\n\n|$)` causes the replacement to stop at the first blank line found *inside* the `tasks:` block. If `HEARTBEAT.md` uses blank lines between task entries (common in YAML-style lists), the remaining task entries are left in `directives` and end up appended to the prompt as "Additional context", which is the opposite of the intended behaviour.
For example, with a file like:
```
tasks:
- name: task1
prompt: do thing 1
- name: task2
prompt: do thing 2
Some global directive
```
The regex only removes through `do thing 1`, leaving `- name: task2 …` in `directives`. Consider dropping the `\n\n` alternative and relying solely on the `^[^\s]` anchor (next top-level key) to terminate the match.
```suggestion
.replace(/^tasks:[\s\S]*?(?=^[^\s]|$(?![\s\S]))/m, "")
```
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: found issues before merge. What this changes: The PR updates HEARTBEAT.md task-block directive stripping, enables non-default agent heartbeats when only global heartbeat defaults are configured, and migrates several console warnings in dotenv, temp-download, and Windows ACL helpers to subsystem loggers. Maintainer follow-up before merge: This is an active contributor PR with a concrete review finding and a heartbeat contract decision for maintainers; the right next action is request changes or clarify the intended behavior, not a separate automated replacement PR. Review findings:
Review detailsBest possible solution: Revise the PR so heartbeat enablement, docs, and tests all agree on the intended defaults-only multi-agent contract, add focused coverage for preserving non-task HEARTBEAT.md directives around task blocks, and keep the logger migration separate and scoped to warning output behavior. 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 df0074768c97. |
|
Codex review: found issues before merge. What this changes: The PR changes heartbeat directive stripping and default heartbeat enablement, and migrates warnings/errors in command auth, config defaults, runtime error handlers, dotenv, temp-download, and Windows ACL helpers to Maintainer follow-up before merge: This is an active contributor PR with concrete blocking findings plus a heartbeat contract decision; the next action is maintainer review/request-changes on this PR rather than a separate automated replacement fix. Review findings:
Review detailsBest possible solution: Revise the PR so the task-block removal preserves non-task HEARTBEAT.md directives while consuming the complete 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 9d03cd15a9a0. |
d03cb66 to
0cf090b
Compare
|
Codex review: found issues before merge. What this changes: The branch preserves Maintainer follow-up before merge: This is an active implementation PR with a concrete patch-level finding and heartbeat behavior semantics to confirm; the next step is maintainer review or request-changes on this PR rather than a separate automated repair branch. Review findings:
Review detailsBest possible solution: Centralize or align task-block stripping with the heartbeat task parser so only recognized task entries are removed and all top-level non-task Markdown before or after 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 e420592bdfd9. |
|
Hi @Sekhar03 . How did you request a review from a team? "Sekhar03 requested a review from a team as a code owner" |
…enclaw#74471) * refactor(security): replace console.warn with structured logger in windows-acl * infra: fix heartbeat directive preservation and global enablement * logging: migrate dotenv and temp-download to subsystem logger * logging: migrate command-auth, unhandled-rejections, and index to subsystem logger * logging: migrate config defaults to subsystem logger * fix(heartbeat): preserve heartbeat task context --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
…enclaw#74471) * refactor(security): replace console.warn with structured logger in windows-acl * infra: fix heartbeat directive preservation and global enablement * logging: migrate dotenv and temp-download to subsystem logger * logging: migrate command-auth, unhandled-rejections, and index to subsystem logger * logging: migrate config defaults to subsystem logger * fix(heartbeat): preserve heartbeat task context --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This PR addresses several infrastructure technical debt items and bugs:
Fixes
src/infra/heartbeat-runner.tsthat stripped non-task instructions fromHEARTBEAT.md.src/infra/heartbeat-summary.tsthat restricted heartbeats to the default agent even when global defaults were set.Technical Debt
windows-acl.ts,dotenv.ts, andtemp-download.tsto the structuredSubsystemLoggerto reduce console noise and improve audit logs.All changes verified with
pnpm tsgo:core.