Skip to content

Functorize worktree access#290

Merged
subsetpark merged 2 commits into
mainfrom
lift-worktree-into-functor
May 20, 2026
Merged

Functorize worktree access#290
subsetpark merged 2 commits into
mainfrom
lift-worktree-into-functor

Conversation

@subsetpark
Copy link
Copy Markdown
Collaborator

@subsetpark subsetpark commented May 20, 2026

Summary

  • add a Worktree.S capability and Worktree.make client for repo-scoped git/worktree effects
  • functorize Worktree_setup, Startup_reconciler, and Session_driver over the worktree client
  • wire a single WorktreeClient through bin/main.ml and remove app-path process_mgr/repo_root threading for worktree effects

Tests

  • dune build
  • dune fmt
  • env HOME=/private/tmp/onton-empty-home XDG_CONFIG_HOME=/private/tmp/onton-empty-xdg GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 dune runtest
  • pre-commit hook: dune build, dune runtest, format check

View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

Summary by cubic

Functorized all worktree operations behind a Worktree.S interface and wired a single client through the app, dropping process_mgr/repo_root plumbing across session APIs. This simplifies session flow and makes repo-scoped git actions easier to test and mock.

  • Refactors

    • Added Worktree.S and moved worktree operations behind it (create/remove, list/find, hooks, fetch, status).
    • Functorized Worktree_setup, Startup_reconciler, and Session_driver over Worktree.S.
    • Removed process_mgr/repo_root from Session_driver.Make (run, run_long_lived) and updated runner_fiber call sites.
    • Updated bin/main.ml to construct and pass one worktree client; removed ad-hoc threading of process_mgr and repo_root.
  • Migration

    • Provide a Worktree.S implementation and pass it to Worktree_setup.Make, Startup_reconciler.Make, and Session_driver.Make.
    • Update calls to Session_driver.run/run_long_lived to remove process_mgr/repo_root.
    • Use Startup_reconciler.Make(...).recover_worktrees; the top-level recover_worktrees was removed.

Written for commit ca18634. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • Refactor
    • Restructured internal architectural patterns to improve dependency injection and module composition across session management, worktree operations, and startup reconciliation components.

Review Change Stack

@vercel
Copy link
Copy Markdown

vercel Bot commented May 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
onton Ready Ready Preview, Comment May 20, 2026 3:57pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cc322209-72fe-4c96-a178-f5bc5309ef08

📥 Commits

Reviewing files that changed from the base of the PR and between 52b43eb and ca18634.

📒 Files selected for processing (9)
  • bin/main.ml
  • lib/session_driver.ml
  • lib/session_driver.mli
  • lib/startup_reconciler.ml
  • lib/startup_reconciler.mli
  • lib/worktree.ml
  • lib/worktree.mli
  • lib/worktree_setup.ml
  • lib/worktree_setup.mli

📝 Walkthrough

Walkthrough

This PR refactors the runner to eliminate direct process_mgr/repo_root threading by introducing a Worktree.S module abstraction, constructing a concrete worktree client once at startup, and functorizing session execution, startup reconciliation, and worktree setup to accept the injected client, enabling cleaner dependency management throughout the orchestration stack.

Changes

Worktree Abstraction and Module-Based Dependency Injection

Layer / File(s) Summary
Worktree.S module type and client
lib/worktree.ml, lib/worktree.mli
Introduces module type S capturing repo-bound worktree operations (create, remove, query, fetch, rebase, push, hooks, conflict introspection), plus type client = (module S) and make ~process_mgr ~repo_root constructor that packages these operations into a first-class module.
Startup reconciliation with injected worktree client
lib/startup_reconciler.ml, lib/startup_reconciler.mli
Refactors Make functor to accept Worktree.S, introduces recover_worktrees_with helper using injected listing, and removes repo_root/process_mgr parameters from reconcile; worktree discovery now unconditionally delegates to the client.
Worktree setup functorization
lib/worktree_setup.ml, lib/worktree_setup.mli
Moves resolve_worktree_path and ensure_worktree under Make (W : Worktree.S) functor, removing process_mgr/repo_root parameters and routing creation, status queries, and hook execution through W.
Session driver orchestration refactoring
lib/session_driver.ml, lib/session_driver.mli
Functorizes over Worktree.S, removes process_mgr/repo_root threading, and refactors streaming to detect PR numbers in bounded windows and conditionally persist session metadata; updates result handling to override "no-commits" for human-directed operations.
Main runner integration
bin/main.ml
Extends Make_fibers to accept Worktree.S, instantiates reconciler and session/setup modules with injected client, and updates all polling, rebase, push, conflict, and delivery paths to call worktree operations through W without process_mgr/repo_root.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

  • flowglad/onton#279: Overlaps with lib/session_driver.ml session orchestration refactoring (streaming improvements, telemetry/artifact sink registration, and session UUID handling) that this PR builds upon.
  • flowglad/onton#286: Both PRs modify lib/startup_reconciler.ml's Make functor and reconcile wiring; this PR extends that to inject Worktree.S and eliminates process_mgr/repo_root parameters.

Poem

🐰 Hops through the worktree maze,
Dependencies untangled now in cleaner days,
Modules functor-bound with grace,
Each thread a ribbon, finding its place.
The runner dances, lighter, more free—
Abstraction's gift to the codebase, you see! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Functorize worktree access' clearly and concisely summarizes the main change: introducing a Worktree.S capability and functorizing modules over it to encapsulate worktree operations, which is the primary objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lift-worktree-into-functor

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@flowglad-review-service flowglad-review-service Bot left a comment

Choose a reason for hiding this comment

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

Code Review: 3 comments

Comment thread lib/session_driver.mli
Comment thread lib/startup_reconciler.ml
Comment thread lib/worktree.ml
@flowglad-review-service
Copy link
Copy Markdown

Review Summary

This is an infra-class refactor that functorizes worktree operations behind Worktree.S and wires a single client through bin/main.ml. The mechanical plumbing is correct and the intent is clear. Two actionable issues:

  1. Dead parameters in session_driver.mli (process_mgr and repo_root still appear in the run/run_long_lived signatures inside Make but are silently ignored in the implementation). These should be removed from the interface to avoid misleading callers.

  2. Minor naming inconsistency in startup_reconciler.ml (recover_worktrees_with at module level vs the rest of the codebase convention) — low priority, not a correctness issue.

No correctness bugs, no security issues, no violation of the busy/complete invariant, and no effectful dependencies leaked into lib_core.

Severity Count
Must-fix 0
Should-fix 2
Note 1

Variant: convergence-v2

Candidates: 3 | Posted: 3 | Suppressed: 0


3 comments posted · Model: claude-sonnet-4-6 (sonnet) · Tokens: 38683 in / 907 out · Cache: 0 created / 8838 read · Review mode: one-shot

…tures

Remove `process_mgr` and `repo_root` from `run`, `run_long_lived`, and the
internal `run_with_backend` helper — these were already ignored after the
worktree functor injection landed. Update the two call sites in `runner_fiber`
and drop the now-unused `process_mgr` binding there.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@flowglad-review-service
Copy link
Copy Markdown

Review Summary

This delta introduces the Worktree.S capability interface and functorizes Worktree_setup, Startup_reconciler, and Session_driver over it, wiring a single WorktreeClient through bin/main.ml. The refactor is mechanically correct and clean: process_mgr/repo_root threading is consistently removed from the public interfaces and the make factory correctly captures both values in the returned first-class module. No new correctness, security, or data-loss issues are introduced by this push. The three outstanding comments from round 1 remain live on the PR; no new actionable findings are present in this incremental diff.

Severity Count
Must-fix 0
Should-fix 0
Note 0

Variant: convergence-v2

Candidates: 0 | Posted: 0 | Suppressed: 0


0 comments posted · Model: claude-sonnet-4-6 (sonnet) · Tokens: 40893 in / 202 out · Cache: 0 created / 8838 read · Review mode: one-shot

@subsetpark subsetpark merged commit ef49f86 into main May 20, 2026
9 checks passed
@subsetpark subsetpark deleted the lift-worktree-into-functor branch May 20, 2026 16:07
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.

1 participant