Skip to content

Conversation

@ojowwalker77
Copy link
Owner

@ojowwalker77 ojowwalker77 commented Jan 14, 2026

Summary

  • Commands to Skills Migration - All 10 Matrix commands now use the new skills format with hot-reload support
  • Background Job System - Async operations with job polling to avoid timeouts
  • once:true Hook Support - One-time hook execution tracking

Changes

Skills Migration

  • Migrated all commands from commands/ to skills/*/SKILL.md format
  • Added model delegation (agent: haiku for simple ops)
  • Added context isolation (context: fork for review/deep-research)
  • Added explicit allowed-tools permissions

Background Job System

  • New matrix_job_status, matrix_job_cancel, matrix_job_list MCP tools
  • matrix_reindex now supports async: true for background execution
  • Job progress tracking in SQLite

One-Time Hooks

  • hasRunThisSession() / markAsRun() helpers for hooks that should run once per session
  • Database-backed tracking with auto-cleanup

Test Plan

  • Build passes
  • All 66 tests pass
  • Verify skills load correctly
  • Test async reindex with job polling

Background Job System:
- Add background_jobs and hook_executions tables to schema
- Create src/jobs/ module with manager, workers, and worker-entry
- Add matrix_job_status, matrix_job_cancel, matrix_job_list MCP tools
- Support async: true for matrix_reindex to avoid timeouts

One-Time Hook Execution:
- Add src/hooks/once.ts with hasRunThisSession/markAsRun helpers
- Database-backed tracking for hooks that should only run once per session
- Auto-cleanup of old records after 7 days

Also exports getRepoInfo from index-tools for use by job workers.
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 14, 2026

Greptile Summary

This PR successfully migrates all commands to the new skills format and introduces a robust background job system with one-time hook execution support.

Key Changes:

  • Background job system enables async operations (e.g., matrix_reindex with async: true) to avoid MCP timeouts
  • New MCP tools (matrix_job_status, matrix_job_cancel, matrix_job_list) for job management with polling-based progress tracking
  • One-time hook execution tracking via hasRunThisSession() and markAsRun() helpers, database-backed with automatic cleanup
  • Database schema expanded with background_jobs and hook_executions tables, properly indexed
  • All changes maintain backward compatibility

Implementation Quality:
The job management system is well-architected with clear separation between manager (state tracking), workers (business logic), and spawner (process management). Error handling is generally solid, with proper job state transitions and cleanup mechanisms. The one-time hook system is simple and effective with appropriate silent error handling.

Minor Concerns:

  • Worker script errors may leave jobs stuck in queued state if the worker crashes before updating job status
  • Detached background processes lack PID tracking for cleanup of orphaned processes on server restart

Confidence Score: 4/5

  • This PR is safe to merge with minor improvements recommended
  • Score reflects solid implementation with proper database schema, error handling, and backward compatibility. Two minor issues identified (worker error handling and orphaned process tracking) are edge cases that don't affect core functionality but should be addressed for production robustness.
  • src/jobs/worker-entry.ts needs improved error handling to prevent stuck jobs

Important Files Changed

Filename Overview
src/jobs/workers.ts New background job worker implementation for async reindexing with progress tracking. Minor concern: spawned processes are detached without monitoring for orphaned processes.
src/jobs/manager.ts Solid job management system with proper state tracking, timestamps, and cleanup functionality. Well-structured with clear separation of concerns.
src/jobs/worker-entry.ts Simple worker entry point for background jobs. Minor issue: error handling doesn't update job status on worker script errors.
src/db/schema.ts Added two new tables (hook_executions, background_jobs) with proper constraints, indexes, and foreign keys. Schema changes are backward compatible.

Sequence Diagram

sequenceDiagram
    participant User
    participant MCP as MCP Server
    participant Handler as handlers.ts
    participant Manager as Job Manager
    participant Spawner as workers.ts
    participant Worker as worker-entry.ts
    participant Indexer as indexRepository

    User->>MCP: matrix_reindex(async: true)
    MCP->>Handler: handleToolCall('matrix_reindex')
    Handler->>Manager: createJob('matrix_reindex', input)
    Manager->>Manager: generateJobId()
    Manager->>Manager: INSERT INTO background_jobs
    Manager-->>Handler: jobId
    Handler->>Spawner: spawnBackgroundJob('reindex', jobId, input)
    Spawner->>Worker: Bun.spawn([bun, run, worker-entry.ts])
    Note over Spawner,Worker: Detached process
    Handler-->>User: {jobId, status: 'queued', message}
    
    Worker->>Worker: Parse args (workerName, jobId, input)
    Worker->>Manager: updateJob(jobId, {status: 'running'})
    Worker->>Indexer: runReindexJob(jobId, input)
    
    loop Progress Updates
        Indexer->>Manager: updateJob(jobId, {progressPercent, progressMessage})
    end
    
    Indexer-->>Worker: result
    Worker->>Manager: updateJob(jobId, {status: 'completed', result})
    Worker->>Worker: process.exit(0)
    
    User->>MCP: matrix_job_status(jobId)
    MCP->>Handler: handleToolCall('matrix_job_status')
    Handler->>Manager: getJob(jobId)
    Manager->>Manager: SELECT FROM background_jobs
    Manager-->>Handler: job data
    Handler-->>User: {status, progress, result}
Loading

Copy link
Contributor

@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.

12 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines 112 to 116
Bun.spawn(['bun', 'run', workerScript, workerName, jobId, JSON.stringify(input)], {
stdio: ['ignore', 'ignore', 'ignore'],
// Detach from parent process
detached: true,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider storing the process PID in the background_jobs table to enable cleanup of orphaned processes on restart. Detached processes without tracking could leak if the parent crashes. Have you considered storing the PID for cleanup of orphaned processes on restart?

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/jobs/workers.ts
Line: 112:116

Comment:
**style:** Consider storing the process PID in the `background_jobs` table to enable cleanup of orphaned processes on restart. Detached processes without tracking could leak if the parent crashes. Have you considered storing the PID for cleanup of orphaned processes on restart?

How can I resolve this? If you propose a fix, please make it concise.

ojowwalker77 and others added 2 commits January 14, 2026 09:16
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…d jobs

- Fix missing closing brace for main() function (CI lint failure)
- Add pid column to background_jobs table for orphan process tracking
- Add updateJobPid() to store spawned process PIDs
- Add cleanupOrphanedProcesses() to detect and mark stale jobs on restart
- Update spawnBackgroundJob() to capture and store PID

Addresses review comment about detached process cleanup.
@ojowwalker77 ojowwalker77 merged commit 9f2e5a4 into main Jan 14, 2026
6 checks passed
@ojowwalker77 ojowwalker77 deleted the feat/migrate-commands-to-skills branch January 17, 2026 18:01
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.

2 participants