fix: cron.sh script_path, unblock t1311, add t1314 for batch concurrency#2193
Conversation
…ck t1311, add t1314 for batch concurrency fix (GH#2163)
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical operational stability and task management within the system. It resolves a bug that prevented proper script execution due to incorrect pathing, thereby ensuring the reliability of scheduled tasks. Additionally, it streamlines the task dispatch process by enabling a key research task and formally documenting a complex concurrency issue for future resolution, improving overall system robustness and clarity in development planning. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis PR modifies the cron.sh script to resolve supervisor-helper.sh using SCRIPT_DIR instead of _CRON_DIR across two function calls. TODO.md is updated with a new batch concurrency task (t1314) replacing t1311, but the file contains unresolved merge conflict markers requiring manual resolution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Mon Feb 23 18:35:18 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TODO.md (1)
1554-1559:⚠️ Potential issue | 🔴 CriticalResolve merge conflict markers before merge.
The file still contains
<<<<<<<,=======,>>>>>>>markers, which will break tooling and parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TODO.md` around lines 1554 - 1559, The file contains unresolved merge conflict markers (<<<<<<<, =======, >>>>>>>) around the t1107/t1094 note; remove these markers and reconcile the two versions by choosing the correct content (either the Updated upstream block with the t1107 bullet and full explanation about t1094 subtasks, or the Stashed changes version with only the CANCELLED note), merging any needed details so the final paragraph is coherent, then save the file and commit the resolved change; ensure no conflict markers remain so tooling/parsers won't fail.
♻️ Duplicate comments (1)
.agents/scripts/supervisor/cron.sh (1)
304-306: Same missingSCRIPT_DIRguard ascmd_cron.The same silent-failure risk exists here: an unset
SCRIPT_DIRcauses the fswatch loop (lines 361–362) to invoke a non-existent script and the launchd watcher to persist a broken path. The same${SCRIPT_DIR:?}guard proposed above applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/cron.sh around lines 304 - 306, script_path is built from SCRIPT_DIR without a fail-fast guard, so unset SCRIPT_DIR can cause the fswatch loop and launchd watcher to invoke a non-existent path; update the assignment for script_path to use the parameter-expansion guard (e.g., script_path="${SCRIPT_DIR:?}/supervisor-helper.sh") and ensure any other uses of SCRIPT_DIR in this script (the fswatch loop and launchd watcher setup referenced alongside cmd_cron) also use ${SCRIPT_DIR:?} so the script fails early with a clear error if SCRIPT_DIR is not set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TODO.md`:
- Line 76: The TODO entry for task t1311 is missing the intended tag; update the
t1311 line in TODO.md to include the `#auto-dispatch` tag (e.g., append "
`#auto-dispatch`" to the t1311 bullet) so the task is correctly marked/unblocked
for auto-dispatch processing; ensure you only modify the t1311 bullet and
preserve the rest of the text (including other tags like
`#research/`#orchestration).
---
Outside diff comments:
In `@TODO.md`:
- Around line 1554-1559: The file contains unresolved merge conflict markers
(<<<<<<<, =======, >>>>>>>) around the t1107/t1094 note; remove these markers
and reconcile the two versions by choosing the correct content (either the
Updated upstream block with the t1107 bullet and full explanation about t1094
subtasks, or the Stashed changes version with only the CANCELLED note), merging
any needed details so the final paragraph is coherent, then save the file and
commit the resolved change; ensure no conflict markers remain so tooling/parsers
won't fail.
---
Duplicate comments:
In @.agents/scripts/supervisor/cron.sh:
- Around line 304-306: script_path is built from SCRIPT_DIR without a fail-fast
guard, so unset SCRIPT_DIR can cause the fswatch loop and launchd watcher to
invoke a non-existent path; update the assignment for script_path to use the
parameter-expansion guard (e.g.,
script_path="${SCRIPT_DIR:?}/supervisor-helper.sh") and ensure any other uses of
SCRIPT_DIR in this script (the fswatch loop and launchd watcher setup referenced
alongside cmd_cron) also use ${SCRIPT_DIR:?} so the script fails early with a
clear error if SCRIPT_DIR is not set.
| - [x] t1309 Intent tracing in OpenCode plugin — add intent field to tool call logging. Inspired by oh-my-pi's `agent__intent` pattern where a hidden field is injected into every tool's JSON Schema requiring the LLM to describe its intent in present participle form. In our case, log the intent alongside tool calls in the observability DB (t1307) for debugging and audit trails. v1.2.7 now passes sessionID and callID to shell.env hook — more context available. Evaluate whether `tool.execute.before` hook provides enough context. ~1h #feature #auto-dispatch #observability #opencode model:sonnet ref:GH#2190 assignee:marcusquinn started:2026-02-22T16:37:42Z status:deployed pr:#2153 completed:2026-02-22 | ||
| - [x] t1310 Document hashline edit format and autocorrect heuristics — create `.agents/reference/hashline-edit-format.md` documenting oh-my-pi's hashline approach: content-addressed line references (xxHash32, custom alphabet), staleness checks, and the battle-tested autocorrect heuristics (anchor echo stripping, line merge detection, wrapped line restoration, indent restoration, bottom-up edit application). This is reference material for when we build custom edit tooling for headless dispatch or the objective runner. Source: `/Users/marcusquinn/Git/oh-my-pi/packages/coding-agent/src/patch/hashline.ts`. ~1h #docs #auto-dispatch #harness #reference model:sonnet ref:GH#2191 assignee:marcusquinn started:2026-02-22T16:38:15Z status:deployed pr:#2151 completed:2026-02-22 | ||
| - [ ] t1311 Evaluate oh-my-pi swarm DAG patterns for supervisor dispatch — compare oh-my-pi's YAML-defined swarm orchestration (`reports_to`/`waits_for` dependency resolution, Kahn's algorithm topological sort, execution waves) with our TODO.md `blocked-by:` system. Identify gaps: our system lacks parallel execution waves, has no `reports_to` concept, and dependency resolution is string-matching not graph-based. Propose concrete enhancements to supervisor dispatch. Source: `/Users/marcusquinn/Git/oh-my-pi/packages/swarm-extension/`. ~2h #research #orchestration #supervisor model:sonnet ref:GH#2135 | ||
| - [ ] t1311 Evaluate oh-my-pi swarm DAG patterns for supervisor dispatch — compare oh-my-pi's YAML-defined swarm orchestration (`reports_to`/`waits_for` dependency resolution, Kahn's algorithm topological sort, execution waves) with our TODO.md `blocked-by:` system. Identify gaps: our system lacks parallel execution waves, has no `reports_to` concept, and dependency resolution is string-matching not graph-based. Propose concrete enhancements to supervisor dispatch. Source: `/Users/marcusquinn/Git/oh-my-pi/packages/swarm-extension/`. ~2h #research #auto-dispatch #orchestration #supervisor model:sonnet ref:GH#2135 |
There was a problem hiding this comment.
Add the intended #auto-dispatch tag to t1311.
PR objectives say t1311 should be unblocked via #auto-dispatch, but the entry currently lacks that tag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TODO.md` at line 76, The TODO entry for task t1311 is missing the intended
tag; update the t1311 line in TODO.md to include the `#auto-dispatch` tag (e.g.,
append " `#auto-dispatch`" to the t1311 bullet) so the task is correctly
marked/unblocked for auto-dispatch processing; ensure you only modify the t1311
bullet and preserve the rest of the text (including other tags like
`#research/`#orchestration).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical path resolution bug in cron.sh that was silently breaking pulse scheduling, unblocks a research task by adding the #auto-dispatch tag, and adds a new task for a batch concurrency bypass issue. The fix in cron.sh correctly switches from _CRON_DIR to SCRIPT_DIR to point to the main supervisor-helper.sh script, which is necessary following the modularization of the supervisor scripts. A comment has been added to ensure proper path resolution for sourced shell script libraries. The additions to TODO.md follow the established project conventions for task tracking and metadata, with a minor suggestion for task organization.
|
|
||
| local script_path | ||
| script_path="${_CRON_DIR}/supervisor-helper.sh" | ||
| script_path="${SCRIPT_DIR}/supervisor-helper.sh" |
There was a problem hiding this comment.
The change from _CRON_DIR to SCRIPT_DIR is correct as supervisor-helper.sh resides in the parent directory of the supervisor modules. However, ensure that SCRIPT_DIR is always defined as an absolute path in the main script that sources this module to prevent issues when the cron job is executed from different working directories.
References
- Shell script libraries should explicitly source their dependencies to ensure they are self-contained and can be sourced from any working directory.
1 participant
Summary
cron.shlines 57 and 305 used_CRON_DIR(resolves toscripts/supervisor/) to build the path tosupervisor-helper.sh, but the script lives atscripts/. Changed toSCRIPT_DIRwhich correctly resolves to the parent directory. This causedcron installandcmd_watchto write paths to a nonexistent file, silently breaking pulse scheduling.#auto-dispatchtag so the supervisor can pick up this research task (evaluate oh-my-pi swarm DAG patterns). The malformedblocked-by:backtick was already auto-cleaned by the supervisor.Also done (GitHub-only, no code changes)
prompt-mention.spec.ts) unrelated to the PR's changesSummary by CodeRabbit
Note: This release contains internal infrastructure and development updates with no user-facing changes.