Skip to content

Conversation

@GollyJer
Copy link
Contributor

@GollyJer GollyJer commented Jan 10, 2026

Summary

  • Fix Windows path validation in prometheus-md-only hook that blocked valid .sisyphus/*.md writes
  • Replace brittle string.includes() checks with robust path.resolve/relative validation
  • Add security hardening: workspace confinement, traversal blocking, nested directory rejection

Changes

  • constants.ts: Remove trailing slash from ALLOWED_PATH_PREFIX (.sisyphus/ → .sisyphus)
  • index.ts: Rewrite isAllowedFile() to use Node's path module for cross-platform compatibility
    • Workspace confinement via path.resolve/path.relative
    • Case-insensitive extension and directory matching
    • Block paths outside workspace root, nested .sisyphus, and traversal attempts
  • index.test.ts: Add 7 new tests for Windows separators, mixed separators, case-insensitivity, and security invariants

Testing
bun run typecheck
bun test src/hooks/prometheus-md-only/index.test.ts
All 20 tests pass on Windows.

Specifically asked Prometheus to do a test write and got this.
Yes! The write succeeded without any blocking. I just created .sisyphus/plans/test-plan.md with a comprehensive test plan demonstrating all the fixed behaviors.
Is it "easier now"? Absolutely. Before the fix, Windows users would get blocked even with valid .sisyphus\plans\file.md paths because the hook used brittle string checks (includes(".sisyphus/")) that failed on backslashes. Now the robust path.resolve/relative validation handles:

  • ✅ Windows backslashes: .sisyphus\plans\x.md
  • ✅ Mixed separators: .sisyphus\plans/x.md
  • ✅ Case-insensitive: .SISYPHUS/plans/x.MD
  • ✅ Security: blocks outside root, nested .sisyphus, traversal
    The fix makes Prometheus planning workflow work correctly on Windows without any manual path manipulation. Users can just use natural Windows paths and the hook handles the rest.

Related Issues
Closes #630


Summary by cubic

Fix cross-platform path validation in the prometheus-md-only hook so Prometheus can write .sisyphus/*.md on Windows without false blocks. Adds strict workspace-root checks and security hardening; addresses Linear #630.

  • Bug Fixes
    • Validate paths with Node path.resolve/relative; support backslashes/mixed separators and case-insensitive .md/.sisyphus.
    • Confine writes to ctx.directory; block traversal, outside-root paths, and nested .sisyphus.
    • Normalize ALLOWED_PATH_PREFIX (remove trailing slash).
    • Add 7 tests for Windows and security cases; all tests pass.

Written for commit c721d4f. Summary will update on new commits.

…upport (code-yeongyu#630)

Replace brittle string checks with robust path.resolve/relative validation:

- Fix Windows backslash paths (.sisyphus\plans\x.md) being incorrectly blocked
- Fix case-sensitive extension check (.MD now accepted)
- Add workspace confinement (block paths outside root even if containing .sisyphus)
- Block nested .sisyphus directories (only first segment allowed)
- Block path traversal attempts (.sisyphus/../secrets.md)
- Use ALLOWED_EXTENSIONS and ALLOWED_PATH_PREFIX constants (case-insensitive)

The new isAllowedFile() uses Node's path module for cross-platform compatibility
instead of string includes/endsWith which failed on Windows separators.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 10, 2026

All contributors have signed the CLA. Thank you! ✅
Posted by the CLA Assistant Lite bot.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@GollyJer
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files

Confidence score: 3/5

  • Workspace confinement can be bypassed via symlinks in src/hooks/prometheus-md-only/index.ts, allowing writes outside the workspace, which is a significant security/regression risk.
  • Pay close attention to src/hooks/prometheus-md-only/index.ts – resolve symlink-aware path validation before merging.
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/hooks/prometheus-md-only/index.ts">

<violation number="1" location="src/hooks/prometheus-md-only/index.ts:20">
P1: Workspace confinement can be bypassed via symlinks: resolve/relative validate the path string, but a `.sisyphus` (or subdir) symlink can redirect the actual write outside the workspace. Consider validating against real paths (realpath) and/or rejecting symlinks in the target directory chain.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

*/
function isAllowedFile(filePath: string, workspaceRoot: string): boolean {
// 1. Resolve to absolute path
const resolved = resolve(workspaceRoot, filePath)
Copy link

@cubic-dev-ai cubic-dev-ai bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Workspace confinement can be bypassed via symlinks: resolve/relative validate the path string, but a .sisyphus (or subdir) symlink can redirect the actual write outside the workspace. Consider validating against real paths (realpath) and/or rejecting symlinks in the target directory chain.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/prometheus-md-only/index.ts, line 20:

<comment>Workspace confinement can be bypassed via symlinks: resolve/relative validate the path string, but a `.sisyphus` (or subdir) symlink can redirect the actual write outside the workspace. Consider validating against real paths (realpath) and/or rejecting symlinks in the target directory chain.</comment>

<file context>
@@ -1,16 +1,48 @@
+ */
+function isAllowedFile(filePath: string, workspaceRoot: string): boolean {
+  // 1. Resolve to absolute path
+  const resolved = resolve(workspaceRoot, filePath)
+
+  // 2. Get relative path from workspace root
</file context>
Fix with Cubic

@GollyJer
Copy link
Contributor Author

1 issue found across 3 files

Confidence score: 3/5

  • Workspace confinement can be bypassed via symlinks in src/hooks/prometheus-md-only/index.ts, allowing writes outside the workspace, which is a significant security/regression risk.
  • Pay close attention to src/hooks/prometheus-md-only/index.ts – resolve symlink-aware path validation before merging.

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/hooks/prometheus-md-only/index.ts">

<violation number="1" location="src/hooks/prometheus-md-only/index.ts:20">
P1: Workspace confinement can be bypassed via symlinks: resolve/relative validate the path string, but a `.sisyphus` (or subdir) symlink can redirect the actual write outside the workspace. Consider validating against real paths (realpath) and/or rejecting symlinks in the target directory chain.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

This was a deliberate design decision. I chose logical path validation (resolve/relative) over realpath because:

  1. realpath fails on non-existent paths (Write tool creates new files)
  2. realpath has permission edge cases on Windows
  3. Symlink-based attacks require attacker to already have write access to create the symlink inside .sisyphus/

The threat model here is restricting Prometheus (a planning agent) — not defending against a malicious actor with filesystem access.

@cubic-dev-ai

@GollyJer
Copy link
Contributor Author

recheck

github-actions bot added a commit that referenced this pull request Jan 10, 2026
Gladdonilli added a commit to Gladdonilli/oh-my-opencode that referenced this pull request Jan 10, 2026
…ing and sync mode

## Summary

This commit represents the culmination of extensive debugging and refinement
of the background agent and sisyphus_task systems, building upon changes from
PRs code-yeongyu#592, code-yeongyu#610, code-yeongyu#628, code-yeongyu#638, code-yeongyu#648, code-yeongyu#649, code-yeongyu#652, and code-yeongyu#653.

## Investigation Journey

### Initial Problem
Background tasks were getting stuck indefinitely. User config model overrides
were being ignored for certain agent types.

### Root Cause Analysis
1. Discovered validateSessionHasOutput and checkSessionTodos guards were
   blocking completion even when tasks had finished
2. Found that sync mode (run_in_background=false) was NOT passing categoryModel
   to session.prompt(), while async mode was
3. Traced config loading path and found JSONC files weren't being detected
4. Analyzed kdcokenny/opencode-background-agents for reference implementation

### Trial and Error Log

**Attempt 1: Add model to resume() in manager.ts**
- Hypothesis: Resume needs to pass stored model
- Result: REVERTED - PR code-yeongyu#638 intentionally removed this; agent config handles it

**Attempt 2: Add userAgents lookup for subagent_type**
- Hypothesis: Need to look up agent model from user config
- Result: REVERTED - Agent model already applied at creation in config-handler.ts

**Attempt 3: Add categoryModel to sync mode prompt**
- Hypothesis: Sync mode missing model that async mode passes
- Result: SUCCESS - This was the actual bug

**Attempt 4: Add debug logging throughout pipeline**
- Purpose: Trace model flow through config -> agent -> prompt
- Files: 6 files with appendFileSync to omo-debug.log
- Result: Confirmed fixes working, then REMOVED all debug logging

**Attempt 5: Investigate clickable sessions**
- Hypothesis: parentID should make child sessions clickable in UI
- Result: parentID IS passed correctly, but sessions don't appear clickable
- Analysis: kdcokenny uses same approach; may be OpenCode core limitation
- Status: UNRESOLVED - Needs further investigation or OpenCode core change

## Background Agent Completion Detection (PR code-yeongyu#638)

Simplified the completion detection logic that was causing tasks to get stuck:
- Removed overly complex validateSessionHasOutput and checkSessionTodos guards
- Tasks now complete after MIN_IDLE_TIME_MS (5s) elapsed on session.idle event
- Added 15-minute global timeout (MAX_RUN_TIME_MS) to prevent runaway tasks
- Pattern follows kdcokenny/opencode-background-agents reference implementation

## Model Override Architecture (PRs code-yeongyu#610, code-yeongyu#628, code-yeongyu#638)

Established clear separation between category-based and agent-based model handling:

| Path              | Model Source                              |
|-------------------|-------------------------------------------|
| category=X        | Explicit from category config (passed)    |
| subagent_type=X   | Agent's configured model (at creation)    |
| resume            | Agent's configured model (not passed)     |

Key insight from PR code-yeongyu#638: Don't pass model in prompt body for resume/subagent -
let OpenCode use the agent's configured model set at creation time in
config-handler.ts.

## Sync Mode Category Model Fix (NEW)

Fixed critical bug where sync mode (run_in_background=false) with categories
was NOT passing the categoryModel to session.prompt():

  // BEFORE: Model not passed in sync mode
  body: { agent: agentToUse, system: systemContent, ... }

  // AFTER: Model passed when available
  body: { agent: agentToUse, ...(categoryModel ? { model: categoryModel } : {}), ... }

This ensures category model overrides work consistently in both sync and async modes.

## JSONC Config File Support

Extended config file detection to support both .json and .jsonc extensions:
- getUserConfigDir() now checks for oh-my-opencode.jsonc first
- Both cross-platform (~/.config) and Windows (%APPDATA%) paths support JSONC
- Enables comments in config files for better documentation

## Test Improvements

- Increased sync resume test timeout from 5s to 10s
- Test was flaky because timeout = MIN_STABILITY_TIME_MS (race condition)
- Added clarifying comments about timing requirements

## Code Cleanup

- Removed unused 'os' imports from plugin-config.ts and config-handler.ts
- Removed ALL debug logging (hardcoded paths, appendFileSync calls)
- Added PR code-yeongyu#638 reference comments for future maintainers

## Verified Test Results (8/8 category + subagent tests pass)

| Test              | Type        | Mode  | Result |
|-------------------|-------------|-------|--------|
| quick             | category    | async | ✅     |
| ultrabrain        | category    | async | ✅     |
| most-capable      | category    | async | ✅     |
| quick             | category    | sync  | ✅     |
| librarian         | subagent    | async | ✅     |
| Metis             | subagent    | async | ✅     |
| oracle            | subagent    | sync  | ✅     |
| quick + git-master| category    | async | ✅     |

## Known Issues & Future Work

### 1. Explore Agent Hangs on Non-Exploration Tasks
The explore agent hung when given a simple math query (5+5). This is NOT a
regression - explore is a specialized codebase search agent (contextual grep)
designed for queries like 'Where is X implemented?' not general Q&A.

When given non-exploration tasks, it attempts to search for non-existent
patterns and may hang indefinitely due to no max_steps limit.

**Recommendation**: Add max_steps: 10 to explore agent config in future PR.

### 2. Clickable Child Sessions Not Working
Sessions created via sisyphus_task pass parentID correctly, but don't appear
as clickable child sessions in OpenCode's sidebar UI. Investigation showed:
- parentID IS being passed to session.create()
- kdcokenny/opencode-background-agents uses identical approach
- Sessions exist and complete, just not rendered as clickable in UI

**Recommendation**: May require OpenCode core change or UI setting discovery.

### 3. Prometheus Agent Correctly Blocked
Prometheus (Planner) is a primary agent and correctly rejected when called
via sisyphus_task with subagent_type. This is expected behavior - primary
agents should only be invoked directly, not via task delegation.

## Files Changed

- src/features/background-agent/manager.ts - PR code-yeongyu#638 reference comment
- src/tools/sisyphus-task/tools.ts - Sync mode categoryModel fix
- src/tools/sisyphus-task/tools.test.ts - Test timeout increase
- src/shared/config-path.ts - JSONC extension support
- src/plugin-config.ts - Cleanup unused import
- src/plugin-handlers/config-handler.ts - Cleanup unused import
Copy link
Contributor

@jkoelker jkoelker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a specific escape test with a drive letter like D:\\outside\\.sisyphus\\plan.md. But that's probably overkill. LGTM

@code-yeongyu code-yeongyu merged commit 307d583 into code-yeongyu:dev Jan 11, 2026
3 of 4 checks passed
KNN-07 pushed a commit to KNN-07/oh-my-opencode that referenced this pull request Jan 11, 2026
…upport (code-yeongyu#630) (code-yeongyu#649)

Replace brittle string checks with robust path.resolve/relative validation:

- Fix Windows backslash paths (.sisyphus\plans\x.md) being incorrectly blocked
- Fix case-sensitive extension check (.MD now accepted)
- Add workspace confinement (block paths outside root even if containing .sisyphus)
- Block nested .sisyphus directories (only first segment allowed)
- Block path traversal attempts (.sisyphus/../secrets.md)
- Use ALLOWED_EXTENSIONS and ALLOWED_PATH_PREFIX constants (case-insensitive)

The new isAllowedFile() uses Node's path module for cross-platform compatibility
instead of string includes/endsWith which failed on Windows separators.
kdcokenny pushed a commit that referenced this pull request Jan 13, 2026
kdcokenny pushed a commit that referenced this pull request Jan 13, 2026
…upport (#630) (#649)

Replace brittle string checks with robust path.resolve/relative validation:

- Fix Windows backslash paths (.sisyphus\plans\x.md) being incorrectly blocked
- Fix case-sensitive extension check (.MD now accepted)
- Add workspace confinement (block paths outside root even if containing .sisyphus)
- Block nested .sisyphus directories (only first segment allowed)
- Block path traversal attempts (.sisyphus/../secrets.md)
- Use ALLOWED_EXTENSIONS and ALLOWED_PATH_PREFIX constants (case-insensitive)

The new isAllowedFile() uses Node's path module for cross-platform compatibility
instead of string includes/endsWith which failed on Windows separators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: prometheus-md-only hook blocks valid .sisyphus path writes on Windows

3 participants