feat: enhance agent parallelism guidance and fix skill loader issues #494
+184
−38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, skills with invalid YAML frontmatter would still be loaded with undefined metadata, causing unpredictable behavior. Now the loader properly checks for parseError and returns null, skipping broken skills. - Add parseError check in loadSkillFromPath() and loadSkillFromPathAsync() - Return empty string on load() if frontmatter is invalid - Update test to expect undefined for malformed skill instead of fallback
The discoverSkillsInDirAsync function was hardcoding 'opencode-project' as the scope for all discovered skills, ignoring the actual scope context. This fix: - Add scope parameter to discoverSkillsInDirAsync() with default value - Propagate scope from worker input to discovery function - Ensure skills inherit correct scope from their source directory
Address cross-platform issues that cause problems on Windows: - rules-injector/finder: Normalize backslash paths to forward slashes for consistent .github/instructions matching across platforms - opencode-config-dir: Preserve POSIX-style XDG_CONFIG_HOME paths on Windows (common in CI/test environments using WSL-style paths) - env-cleaner: Filter out literal 'undefined' string values in env vars, not just undefined type (occurs when stringified incorrectly)
The original reminder only fired once when agents hadn't been used. This enhancement adds recurring reminders during the implementation phase to encourage continued parallelism: - Track directToolCallsSinceAgent counter in session state - Show IMPLEMENTATION_PHASE_REMINDER every 5 direct tool calls - Reset counter when agent is used (background_task, call_omo_agent) - New reminder focuses on delegating UI/docs work during coding This addresses the pattern where developers use agents for exploration but forget to parallelize during implementation.
Strengthen the message that agents should be used during ALL phases, not just exploration. This is a coordinated update across multiple prompt injection points: sisyphus.ts: - Add 'Parallel Implementation' section with task type table - Include concrete example showing UI + Backend + Docs parallelism - Add 5 implementation parallelism rules keyword-detector (ultrawork mode): - Update TODO rules to mention 'strategic parallelism' - Expand AGENT DEPLOYMENT section with phase-specific examples - Add code example showing exploration AND implementation parallelism todo-continuation-enforcer: - Rewrite continuation prompt to scan for delegatable TODOs - Add explicit instructions to fire multiple background agents - Focus on verification via background_output before completion The goal is consistent reinforcement: 'Don't stop parallelizing just because you're implementing!'
Contributor
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement (CLA). To sign the CLA, please comment on this PR with: This is a one-time requirement. Once signed, all your future contributions will be automatically accepted. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Greptile SummaryThis PR enhances agent parallelism guidance during the implementation phase and fixes several robustness issues in the skill loader and Windows compatibility. Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Sisyphus as Sisyphus Agent
participant Hook as agent-usage-reminder Hook
participant BgAgents as Background Agents
participant Worker as discover-worker
Note over User,Worker: Implementation Phase Parallelism Flow
User->>Sisyphus: Request implementation task
Sisyphus->>Sisyphus: Create TODO list
loop For each TODO
Sisyphus->>BgAgents: background_task(frontend-ui-ux-engineer)
Sisyphus->>BgAgents: background_task(document-writer)
Sisyphus->>Sisyphus: Work on core logic directly
alt Direct tool call (Grep, Glob, etc)
Hook->>Hook: Increment directToolCallsSinceAgent
alt Count >= 5
Hook-->>Sisyphus: IMPLEMENTATION_PHASE_REMINDER
Hook->>Hook: Reset counter
end
else Agent tool call
Hook->>Hook: Reset directToolCallsSinceAgent to 0
end
Sisyphus->>BgAgents: background_output(task_id)
BgAgents-->>Sisyphus: Results
Sisyphus->>Sisyphus: Mark TODO complete
end
Note over Worker: Skill Discovery with Scope
Worker->>Worker: discoverSkillsInDirAsync(dir, scope)
Worker-->>Worker: Return skills with correct scope
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
src/features/opencode-skill-loader/discover-worker.ts, line 34-36 (link)logic: Using
input.scopes[0]applies the same scope to all directories. If the caller passes multiple directories with corresponding scopes (e.g.,dirs: [projectDir, userDir],scopes: ["project", "user"]), all skills will incorrectly receive the first scope.Consider iterating with index to apply the correct scope per directory:
13 files reviewed, 1 comment
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Confidence score: 3/5
- Scope handling in
src/features/opencode-skill-loader/discover-worker.tsapplies onlyscopes[0], so directories tied to other scopes will execute with the wrong permissions and likely degrade feature isolation. - Persisted state created before this PR will load with
undefinedcounters insrc/hooks/agent-usage-reminder/index.ts, producing NaN increments and breaking reminder logic for existing users. - The claimed Windows POSIX preservation in
src/shared/opencode-config-dir.tsnever executes because the early return prevents hitting that branch, so Windows paths still normalize unexpectedly. - Pay close attention to
src/features/opencode-skill-loader/discover-worker.ts,src/hooks/agent-usage-reminder/index.ts, andsrc/shared/opencode-config-dir.ts- they carry the identified regressions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/opencode-skill-loader/discover-worker.ts">
<violation number="1" location="src/features/opencode-skill-loader/discover-worker.ts:34">
P1: Using only `scopes[0]` applies the same scope to all directories, ignoring the parallel array relationship. Each directory should use its corresponding scope from the array.</violation>
</file>
<file name="src/hooks/agent-usage-reminder/index.ts">
<violation number="1" location="src/hooks/agent-usage-reminder/index.ts:46">
P1: Missing backward compatibility for new state fields. When loading persisted state created before this PR, `lastAgentUseAt` and `directToolCallsSinceAgent` will be `undefined`. This causes `undefined++` to produce `NaN`, and the periodic reminder condition `NaN >= N` will always be `false`, breaking the feature for existing users. Provide defaults for the new fields when loading persisted state.</violation>
</file>
<file name="src/shared/opencode-config-dir.ts">
<violation number="1" location="src/shared/opencode-config-dir.ts:72">
P1: This POSIX path preservation logic is unreachable on Windows due to the early return at line 63. The comment says "preserve it even on Windows" but this code only executes on non-Windows platforms where `path.join` and `path.posix.join` are already equivalent. Consider moving this logic inside the Windows block or restructuring to check before the platform-specific early return.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+34
to
+37
| const scope = input.scopes[0] ?? "opencode-project" | ||
| const results = await Promise.all( | ||
| input.dirs.map((dir) => discoverSkillsInDirAsync(dir, scope)) | ||
| ) |
There was a problem hiding this comment.
P1: Using only scopes[0] applies the same scope to all directories, ignoring the parallel array relationship. Each directory should use its corresponding scope from the array.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/discover-worker.ts, line 34:
<comment>Using only `scopes[0]` applies the same scope to all directories, ignoring the parallel array relationship. Each directory should use its corresponding scope from the array.</comment>
<file context>
@@ -31,9 +31,10 @@ parentPort.once("message", (data: { port: MessagePort }) => {
- const results = await Promise.all(
- input.dirs.map(dir => discoverSkillsInDirAsync(dir))
- )
+ const scope = input.scopes[0] ?? "opencode-project"
+ const results = await Promise.all(
+ input.dirs.map((dir) => discoverSkillsInDirAsync(dir, scope))
</file context>
Suggested change
| const scope = input.scopes[0] ?? "opencode-project" | |
| const results = await Promise.all( | |
| input.dirs.map((dir) => discoverSkillsInDirAsync(dir, scope)) | |
| ) | |
| const results = await Promise.all( | |
| input.dirs.map((dir, index) => discoverSkillsInDirAsync(dir, input.scopes[index] ?? "opencode-project")) | |
| ) |
| agentUsed: false, | ||
| reminderCount: 0, | ||
| updatedAt: Date.now(), | ||
| lastAgentUseAt: 0, |
There was a problem hiding this comment.
P1: Missing backward compatibility for new state fields. When loading persisted state created before this PR, lastAgentUseAt and directToolCallsSinceAgent will be undefined. This causes undefined++ to produce NaN, and the periodic reminder condition NaN >= N will always be false, breaking the feature for existing users. Provide defaults for the new fields when loading persisted state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/agent-usage-reminder/index.ts, line 46:
<comment>Missing backward compatibility for new state fields. When loading persisted state created before this PR, `lastAgentUseAt` and `directToolCallsSinceAgent` will be `undefined`. This causes `undefined++` to produce `NaN`, and the periodic reminder condition `NaN >= N` will always be `false`, breaking the feature for existing users. Provide defaults for the new fields when loading persisted state.</comment>
<file context>
@@ -37,6 +43,8 @@ export function createAgentUsageReminderHook(_ctx: PluginInput) {
agentUsed: false,
reminderCount: 0,
updatedAt: Date.now(),
+ lastAgentUseAt: 0,
+ directToolCallsSinceAgent: 0,
};
</file context>
| const xdgConfig = process.env.XDG_CONFIG_HOME || join(homedir(), ".config") | ||
|
|
||
| // If user provided a POSIX-style XDG path (common in tests/CI), preserve it even on Windows. | ||
| if (process.env.XDG_CONFIG_HOME && xdgConfig.startsWith("/") && !xdgConfig.includes("\\")) { |
There was a problem hiding this comment.
P1: This POSIX path preservation logic is unreachable on Windows due to the early return at line 63. The comment says "preserve it even on Windows" but this code only executes on non-Windows platforms where path.join and path.posix.join are already equivalent. Consider moving this logic inside the Windows block or restructuring to check before the platform-specific early return.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/shared/opencode-config-dir.ts, line 72:
<comment>This POSIX path preservation logic is unreachable on Windows due to the early return at line 63. The comment says "preserve it even on Windows" but this code only executes on non-Windows platforms where `path.join` and `path.posix.join` are already equivalent. Consider moving this logic inside the Windows block or restructuring to check before the platform-specific early return.</comment>
<file context>
@@ -67,6 +67,12 @@ function getCliConfigDir(): string {
const xdgConfig = process.env.XDG_CONFIG_HOME || join(homedir(), ".config")
+
+ // If user provided a POSIX-style XDG path (common in tests/CI), preserve it even on Windows.
+ if (process.env.XDG_CONFIG_HOME && xdgConfig.startsWith("/") && !xdgConfig.includes("\\")) {
+ return posix.join(xdgConfig, "opencode")
+ }
</file context>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR enhances Sisyphus's parallelism behavior during implementation (not just exploration) and fixes several bugs in the skill loader and Windows compatibility.
Motivation
Observed that agents effectively parallelize during exploration (firing multiple explore/librarian agents) but fall into sequential mode during implementation. This PR addresses the gap by:
Changes
Bug Fixes
1.
fix(skill-loader): skip skills with malformed frontmatterparseErrorfrom frontmatter parser, returnnullto skip broken skillsloader.ts,loader.test.ts2.
fix(skill-loader): propagate scope parameter through discoverydiscoverSkillsInDirAsynchardcoded"opencode-project"scope, ignoring actual contextscopeparameter through the discovery chainasync-loader.ts,discover-worker.ts3.
fix: improve Windows compatibility for paths and env handling.github/instructionsmatching; POSIX XDG paths fail on Windows; literal"undefined"strings in env"undefined"string values in env-cleanerfinder.ts,opencode-config-dir.ts,env-cleaner.tsFeatures
4.
feat(agent-usage-reminder): add periodic reminders during implementationdirectToolCallsSinceAgentcounterIMPLEMENTATION_PHASE_REMINDERevery 5 direct tool callsconstants.ts,index.ts,types.ts5.
feat(agents): enhance parallelism guidance throughout agent promptssisyphus.ts,keyword-detector/constants.ts,todo-continuation-enforcer.tsTesting
bun test)bun run typecheck)Commits (5 atomic commits)
fix(skill-loader): skip skills with malformed frontmatterfix(skill-loader): propagate scope parameter through discoveryfix: improve Windows compatibility for paths and env handlingfeat(agent-usage-reminder): add periodic reminders during implementationfeat(agents): enhance parallelism guidance throughout agent promptsSummary by cubic
Improves implementation-phase parallelism with stronger prompt guidance and periodic reminders, and fixes skill loader robustness and Windows compatibility.
New Features
Bug Fixes
Written for commit e4e6c22. Summary will update on new commits.