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

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

The CLA check is failing due to branch protection rules preventing the CLA bot from updating signatures/cla.json.

Error: Repository rule violations found - Changes must be made through a pull request

Could a maintainer please either:

1. Allow the CLA Assistant bot to bypass branch protection, or
2. Manually add my signature to the signatures file?

Thank you!

recheck

@sisyphus-dev-ai review

Code Review - PR #731

Summary

Verdict: ✅ APPROVE with minor suggestions

This PR effectively solves the problem of background tasks scanning the entire home directory by ensuring they inherit the parent session's working directory. The implementation is solid, consistent, and handles edge cases appropriately.

✅ Strengths

1. Correct Root Cause Fix: Addresses the issue at the source by fetching parent session directory before creating background sessions
2. Consistent Implementation: All 5 files follow the same pattern:
   • Fetch parent session via client.session.get()
   • Extract directory with fallback: parentSession?.data?.directory ?? this.directory
   • Pass directory via query: { directory: parentDirectory }
3. Proper Error Handling: All files handle fetch failures gracefully with .catch()
4. Non-Breaking: Changes are purely additive and maintain backward compatibility
5. Interface Update: Correctly updates SisyphusTaskToolOptions interface and wires directory from index.ts

⚠️ Minor Issues

1. Logging Inconsistency

Files with good logging:

• manager.ts (lines 78-87): Logs both error and success cases
• call-omo-agent/tools.ts (lines 147-157): Logs error and parent directory

Files with silent error handling:

• look-at/tools.ts (line 68): .catch(() => null) - no logging
• sisyphus-task/tools.ts (line 405): .catch(() => null) - no logging

Recommendation: Add consistent logging to look-at/tools.ts and sisyphus-task/tools.ts for better debugging:

const parentSession = await ctx.client.session.get({
  path: { id: toolContext.sessionID },
}).catch((err) => {
  log(`[tool_name] Failed to get parent session: ${err}`)
  return null
})
const parentDirectory = parentSession?.data?.directory ?? ctx.directory
log(`[tool_name] Parent dir: ${parentSession?.data?.directory}, using: ${parentDirectory}`)

2. No Tests

The PR doesn't include tests for the new directory inheritance behavior.

Recommendation: Add tests to verify:

• Directory is inherited from parent session when available
• Fallback to context directory when parent session fetch fails
• Behavior when both parent and context directories are undefined

📋 Verification Checklist

• ✅ All 5 files consistently implement directory inheritance
• ✅ Error handling present in all files
• ✅ Fallback mechanism works correctly
• ✅ Interface changes properly propagated
• ✅ No breaking changes introduced
• ⚠️ Logging could be more consistent
• ⚠️ Tests would strengthen confidence

💡 Optional Enhancement

Consider adding a comment in one of the files explaining the "why" behind this change:

// Inherit parent session's directory to prevent background tasks from
// defaulting to $HOME and scanning unrelated files (causing high CPU/memory)
const parentSession = await this.client.session.get({ ... })

🎯 Conclusion

The implementation is solid and production-ready. The logging inconsistency and lack of tests are minor concerns that don't block merging. This PR will significantly improve the user experience by preventing background tasks from consuming excessive resources.

Great work @oussamadouhou! 🎉

Reviewed by: @sisyphus-dev-ai
Files reviewed: 5/5
Confidence level: High

lgtm, thanks for the pr!