jared/cc-os

Watch

Fork 0

os: FinishCommand takes git: and after_merge: collaborators; Invocation from bin/worktree; remove env: and thread-local #717

New issue

Closed

opened 2026-09-25 15:04:07 +00:00 by jared · 2 comments

jared commented

2026-09-25 15:04:07 +00:00

Owner

Blocked-by: #716

Problem

plugins/os/lib/worktree/cli.rb:7-8,31-32 stashes a caller-supplied env: hash in Thread.current[:worktree_cli_env] and falls back to ENV when nothing is threaded.
FinishCommand#initialize (finish_command.rb:14) defaults env: ENV and threads @env to every git spawn (Open3/system calls at lines 51, 60, 80, 84, 94, 95, 105, 109, 114, 119, 121, 125), to ConflictScan.new(..., env:), Removal.new(..., env:), RepoRoot.new(env:), and to UsageSweep.new(..., env:).
UsageSweep#resolve_runner (usage_sweep.rb:22-23) calls BinaryLocator.resolve('os-sdlc-runner', env: @env), which reads env.fetch('PATH', '') (binary_locator.rb:8) to find the sibling os-sdlc-runner binary by scanning PATH.
This is the shape #716 supersedes: library code should take explicit collaborators, not an env hash threaded through a thread-local.
base_branch (finish_command.rb:59-61) also runs git symbolic-ref --short HEAD on every call site that needs it (up to 3 times per finish) because it is not memoized; this ticket's git: collaborator change is the dependency ticket 4 needs before it can assert "called once" against a fake.

Approach

Required work:

Add Worktree::Git to plugins/os/lib/worktree/git.rb: the only class in plugins/os/lib that spawns git, modeled on os-sdlc's Adapter::Git::Repo (plugins/os-sdlc/lib/os_sdlc/adapter/git/repo.rb); methods read(*args), succeeds?(*args), run(*args), each doing Open3.capture3('git', '-C', @root, *args) (or Open3.capture2e where the caller needs combined output, matching ConflictScan#report's current capture2e merge-tree call), no env: parameter.
Add Worktree::Invocation as Data.define(:cwd, :stdout, :stderr), built in bin/worktree from Dir.pwd, $stdout, $stderr, and passed into Worktree::CLI.start(ARGV, invocation: Worktree::Invocation.new(...)); commands fetch it via config.fetch(:invocation) (mirror os-sdlc's ADR-0207 Invocation wiring), replacing the Thread.current[:worktree_cli_env] mechanism in cli.rb.
Add Worktree::AfterMerge (plugins/os/lib/worktree/after_merge.rb): owns the three post-merge effects now spread across finish_command.rb:101-126. Constructor AfterMerge.new(root:, git:, sweep:, stop_server:) where sweep is a UsageSweep and stop_server is a callable taking the worktree path (production default: ->(path) { system('rubocop', '--stop-server', chdir: path) }). Single public method call(worktree_path:, pre_merge_head:, implementation_id:): runs bin/refresh-plugins under root when git.read('diff', '--name-only', pre_merge_head, 'HEAD') lists a plugins/ path, then stop_server.call(worktree_path), then sweep.call(worktree_path, implementation_id) when implementation_id is non-nil. FinishCommand calls only @after_merge.call(...); FinishCommand tests pass a recording double that records those three keyword arguments; AfterMerge's own tests pass a FakeGit, a recording sweep, and a recording stop_server lambda, so no real rubocop runs anywhere in the suite.
Change UsageSweep.new(runner_path:): production resolves the sibling os-sdlc-runner by fixed relative path from the running binary's location (copy os-sdlc's Adapter::BinaryLocator.default_sibling idea, plugins/os-sdlc/lib/os_sdlc/adapter/binary_locator.rb), not by scanning PATH; delete Worktree::BinaryLocator (plugins/os/lib/worktree/binary_locator.rb) since nothing else in plugins/os/lib uses it.
Change FinishCommand.new(branch, root:, git:, after_merge:, implementation_id: nil); delete the env: keyword; merge_into_base, worktree_path_for_branch, base_branch, ancestor?, verify_ancestry!, conflicted_files route through git: instead of Open3/system calls against @env.
Change ConflictScan.new(root, base, branch, git:), Removal.new(root, path, git:), RepoRoot.new(git: Worktree::Git.new) (or equivalent) to take git: instead of env:; delete the env: keyword from each.
Change StartCommand.new(name, root:, repo_root:) to source root from the passed repo_root/git: rather than a fresh RepoRoot.new.path default that reads ENV transitively.
Delete the env: keyword and every ENV/Thread.current reference from plugins/os/lib (verify with the acceptance-criteria grep below).
Memoize base_branch with @base_branch ||= inside FinishCommand.
Add FakeGit to plugins/os/tests/support (mirror os-sdlc's FakeGit, plugins/os-sdlc/tests/test_helper.rb:209: run/read/succeeds? plus recorded calls) and a recording after_merge double; convert the finish tests listed under Retired tests to use FakeGit and the double instead of PATH-planted fake binaries; keep finish_command_conflicts_test.rb, finish_command_clean_path_test.rb, and finish_command_removal_failure_test.rb as real-git integration tests through RepoSeeder (clean merge, conflict, removal-failure paths) so the suite still exercises real git at least once per path.

Acceptance criteria

Given the finished change, when a maintainer runs grep -rn "ENV\|Thread.current" plugins/os/lib, then the command prints no matches.
Given the finished change, when a maintainer runs grep -rln "'git'" plugins/os/lib, then only plugins/os/lib/worktree/git.rb matches (no other file spawns git directly).
Given the test suite, when it runs to completion, then it spawns zero real rubocop processes (the rubocop-server test asserts against the after_merge/AfterMerge double, not a PATH-planted rubocop script).
Given the test suite, when it runs to completion, then it is green.

Owner sketch

# plugins/os/lib/worktree/git.rb
module Worktree
  class Git
    def initialize(root)
      @root = root
    end

    def read(*args) = capture(*args).first
    def succeeds?(*args) = capture(*args).last.success?
    def run(*args) = system('git', '-C', @root, *args)

    private

    def capture(*args) = Open3.capture3('git', '-C', @root, *args)
  end
end

# plugins/os/lib/worktree/finish_command.rb
module Worktree
  class FinishCommand
    def initialize(branch, root:, git:, after_merge:, implementation_id: nil)
      @branch = branch
      @root = root
      @git = git
      @after_merge = after_merge
      @implementation_id = implementation_id
    end

    private

    def base_branch
      @base_branch ||= @git.read('symbolic-ref', '--short', 'HEAD').strip
    end
  end
end

# plugins/os/lib/worktree/after_merge.rb
module Worktree
  class AfterMerge
    def initialize(root:, git:, sweep:, stop_server:)
      @root = root
      @git = git
      @sweep = sweep
      @stop_server = stop_server
    end

    def call(worktree_path:, pre_merge_head:, implementation_id:)
      refresh_plugins if plugins_changed?(pre_merge_head)
      @stop_server.call(worktree_path)
      @sweep.call(worktree_path, implementation_id) if implementation_id
    end
  end
end

# plugins/os/lib/worktree/cli.rb (production wiring)
git = Worktree::Git.new(root)
after_merge = Worktree::AfterMerge.new(
  root: root, git: git,
  sweep: Worktree::UsageSweep.new(runner_path: Worktree::SdlcRunner.sibling),
  stop_server: ->(path) { system('rubocop', '--stop-server', chdir: path) }
)
FinishCommand.new(branch, root: root, git: git, after_merge: after_merge, implementation_id: options[:implementation]).run

Retired tests

plugins/os/tests/worktree/cli_env_threading_test.rb
plugins/os/tests/worktree/finish_command_stops_rubocop_server_test.rb
plugins/os/tests/worktree/finish_command_sweep_binary_resolution_test.rb

Non-goals

Extracting Worktree::Git into a shared gem consumed by both os and os-sdlc (#699 tracks the cross-plugin duplication survey; ADR-0025 item 4 forbids source coupling for now).
plugins/os-backlog changes (ticket 2, separate ticket).
Folding ticket 4's base_branch memoization observability check into this ticket's acceptance criteria (ticket 4 is a small follow-on that asserts call count against the FakeGit this ticket introduces).

Origin

Session: 89876543-c418-46b0-b994-dd6dc6887802
Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/89876543-c418-46b0-b994-dd6dc6887802.jsonl
Trigger: review of #634/#707/#708 (commits 8ef12ae8, e2817ffe, ADR-0218)
Improvised this session: none
Chain: env:/thread-local in lib ← ADR-0218 item 1 chose spawn-env-hash + Thread.current ← DESIGN (docs/adr/ADR-0218…, contradicts ADR-0213 and ADR-0207)
Root candidate: #716 (the superseding ADR)

Blocked-by: #716 ## Problem `plugins/os/lib/worktree/cli.rb:7-8,31-32` stashes a caller-supplied `env:` hash in `Thread.current[:worktree_cli_env]` and falls back to `ENV` when nothing is threaded. `FinishCommand#initialize` (`finish_command.rb:14`) defaults `env: ENV` and threads `@env` to every git spawn (`Open3`/`system` calls at lines 51, 60, 80, 84, 94, 95, 105, 109, 114, 119, 121, 125), to `ConflictScan.new(..., env:)`, `Removal.new(..., env:)`, `RepoRoot.new(env:)`, and to `UsageSweep.new(..., env:)`. `UsageSweep#resolve_runner` (`usage_sweep.rb:22-23`) calls `BinaryLocator.resolve('os-sdlc-runner', env: @env)`, which reads `env.fetch('PATH', '')` (`binary_locator.rb:8`) to find the sibling `os-sdlc-runner` binary by scanning `PATH`. This is the shape #716 supersedes: library code should take explicit collaborators, not an env hash threaded through a thread-local. `base_branch` (`finish_command.rb:59-61`) also runs `git symbolic-ref --short HEAD` on every call site that needs it (up to 3 times per finish) because it is not memoized; this ticket's `git:` collaborator change is the dependency ticket 4 needs before it can assert "called once" against a fake. ## Approach Required work: 1. Add `Worktree::Git` to `plugins/os/lib/worktree/git.rb`: the only class in `plugins/os/lib` that spawns `git`, modeled on os-sdlc's `Adapter::Git::Repo` (`plugins/os-sdlc/lib/os_sdlc/adapter/git/repo.rb`); methods `read(*args)`, `succeeds?(*args)`, `run(*args)`, each doing `Open3.capture3('git', '-C', @root, *args)` (or `Open3.capture2e` where the caller needs combined output, matching `ConflictScan#report`'s current `capture2e` merge-tree call), no `env:` parameter. 2. Add `Worktree::Invocation` as `Data.define(:cwd, :stdout, :stderr)`, built in `bin/worktree` from `Dir.pwd`, `$stdout`, `$stderr`, and passed into `Worktree::CLI.start(ARGV, invocation: Worktree::Invocation.new(...))`; commands fetch it via `config.fetch(:invocation)` (mirror os-sdlc's ADR-0207 Invocation wiring), replacing the `Thread.current[:worktree_cli_env]` mechanism in `cli.rb`. 3. Add `Worktree::AfterMerge` (`plugins/os/lib/worktree/after_merge.rb`): owns the three post-merge effects now spread across `finish_command.rb:101-126`. Constructor `AfterMerge.new(root:, git:, sweep:, stop_server:)` where `sweep` is a `UsageSweep` and `stop_server` is a callable taking the worktree path (production default: `->(path) { system('rubocop', '--stop-server', chdir: path) }`). Single public method `call(worktree_path:, pre_merge_head:, implementation_id:)`: runs `bin/refresh-plugins` under `root` when `git.read('diff', '--name-only', pre_merge_head, 'HEAD')` lists a `plugins/` path, then `stop_server.call(worktree_path)`, then `sweep.call(worktree_path, implementation_id)` when `implementation_id` is non-nil. `FinishCommand` calls only `@after_merge.call(...)`; FinishCommand tests pass a recording double that records those three keyword arguments; AfterMerge's own tests pass a `FakeGit`, a recording `sweep`, and a recording `stop_server` lambda, so no real `rubocop` runs anywhere in the suite. 4. Change `UsageSweep.new(runner_path:)`: production resolves the sibling `os-sdlc-runner` by fixed relative path from the running binary's location (copy os-sdlc's `Adapter::BinaryLocator.default_sibling` idea, `plugins/os-sdlc/lib/os_sdlc/adapter/binary_locator.rb`), not by scanning `PATH`; delete `Worktree::BinaryLocator` (`plugins/os/lib/worktree/binary_locator.rb`) since nothing else in `plugins/os/lib` uses it. 5. Change `FinishCommand.new(branch, root:, git:, after_merge:, implementation_id: nil)`; delete the `env:` keyword; `merge_into_base`, `worktree_path_for_branch`, `base_branch`, `ancestor?`, `verify_ancestry!`, `conflicted_files` route through `git:` instead of `Open3`/`system` calls against `@env`. 6. Change `ConflictScan.new(root, base, branch, git:)`, `Removal.new(root, path, git:)`, `RepoRoot.new(git: Worktree::Git.new)` (or equivalent) to take `git:` instead of `env:`; delete the `env:` keyword from each. 7. Change `StartCommand.new(name, root:, repo_root:)` to source `root` from the passed `repo_root`/`git:` rather than a fresh `RepoRoot.new.path` default that reads `ENV` transitively. 8. Delete the `env:` keyword and every `ENV`/`Thread.current` reference from `plugins/os/lib` (verify with the acceptance-criteria grep below). 9. Memoize `base_branch` with `@base_branch ||=` inside `FinishCommand`. 10. Add `FakeGit` to `plugins/os/tests/support` (mirror os-sdlc's `FakeGit`, `plugins/os-sdlc/tests/test_helper.rb:209`: `run`/`read`/`succeeds?` plus recorded calls) and a recording `after_merge` double; convert the finish tests listed under Retired tests to use `FakeGit` and the double instead of PATH-planted fake binaries; keep `finish_command_conflicts_test.rb`, `finish_command_clean_path_test.rb`, and `finish_command_removal_failure_test.rb` as real-git integration tests through `RepoSeeder` (clean merge, conflict, removal-failure paths) so the suite still exercises real git at least once per path. ## Acceptance criteria 1. Given the finished change, when a maintainer runs `grep -rn "ENV\|Thread.current" plugins/os/lib`, then the command prints no matches. 2. Given the finished change, when a maintainer runs `grep -rln "'git'" plugins/os/lib`, then only `plugins/os/lib/worktree/git.rb` matches (no other file spawns `git` directly). 3. Given the test suite, when it runs to completion, then it spawns zero real `rubocop` processes (the rubocop-server test asserts against the `after_merge`/`AfterMerge` double, not a PATH-planted `rubocop` script). 4. Given the test suite, when it runs to completion, then it is green. ## Owner sketch # plugins/os/lib/worktree/git.rb module Worktree class Git def initialize(root) @root = root end def read(*args) = capture(*args).first def succeeds?(*args) = capture(*args).last.success? def run(*args) = system('git', '-C', @root, *args) private def capture(*args) = Open3.capture3('git', '-C', @root, *args) end end # plugins/os/lib/worktree/finish_command.rb module Worktree class FinishCommand def initialize(branch, root:, git:, after_merge:, implementation_id: nil) @branch = branch @root = root @git = git @after_merge = after_merge @implementation_id = implementation_id end private def base_branch @base_branch ||= @git.read('symbolic-ref', '--short', 'HEAD').strip end end end # plugins/os/lib/worktree/after_merge.rb module Worktree class AfterMerge def initialize(root:, git:, sweep:, stop_server:) @root = root @git = git @sweep = sweep @stop_server = stop_server end def call(worktree_path:, pre_merge_head:, implementation_id:) refresh_plugins if plugins_changed?(pre_merge_head) @stop_server.call(worktree_path) @sweep.call(worktree_path, implementation_id) if implementation_id end end end # plugins/os/lib/worktree/cli.rb (production wiring) git = Worktree::Git.new(root) after_merge = Worktree::AfterMerge.new( root: root, git: git, sweep: Worktree::UsageSweep.new(runner_path: Worktree::SdlcRunner.sibling), stop_server: ->(path) { system('rubocop', '--stop-server', chdir: path) } ) FinishCommand.new(branch, root: root, git: git, after_merge: after_merge, implementation_id: options[:implementation]).run ## Retired tests plugins/os/tests/worktree/cli_env_threading_test.rb plugins/os/tests/worktree/finish_command_stops_rubocop_server_test.rb plugins/os/tests/worktree/finish_command_sweep_binary_resolution_test.rb ## Non-goals Extracting `Worktree::Git` into a shared gem consumed by both `os` and `os-sdlc` (#699 tracks the cross-plugin duplication survey; ADR-0025 item 4 forbids source coupling for now). `plugins/os-backlog` changes (ticket 2, separate ticket). Folding ticket 4's `base_branch` memoization observability check into this ticket's acceptance criteria (ticket 4 is a small follow-on that asserts call count against the `FakeGit` this ticket introduces). ## Origin Session: 89876543-c418-46b0-b994-dd6dc6887802 Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/89876543-c418-46b0-b994-dd6dc6887802.jsonl Trigger: review of #634/#707/#708 (commits 8ef12ae8, e2817ffe, ADR-0218) Improvised this session: none Chain: env:/thread-local in lib ← ADR-0218 item 1 chose spawn-env-hash + Thread.current ← DESIGN (docs/adr/ADR-0218…, contradicts ADR-0213 and ADR-0207) Root candidate: #716 (the superseding ADR)

jared added the

plugin/os

filed-by/user

labels

2026-09-25 15:04:07 +00:00

jared referenced this issue

2026-09-25 15:04:24 +00:00

os: memoize base_branch so finish runs git symbolic-ref once #719

jared added the

ready-for-agent

label

2026-09-25 15:04:27 +00:00

jared referenced this issue

2026-09-25 15:04:41 +00:00

Decide the home for RequiredWork shared by os-backlog and os-sdlc (cc-os gem or CLI seam) #699

jared referenced this issue

2026-09-25 15:04:41 +00:00

ADR: supersede ADR-0218 item 1 — os and os-backlog libraries take collaborators and paths as parameters, only bin/ reads process globals #716

jared referenced this issue

2026-09-25 15:05:59 +00:00

ADR: supersede ADR-0218 item 1 — os and os-backlog libraries take collaborators and paths as parameters, only bin/ reads process globals #716

jared commented

2026-09-25 15:26:56 +00:00

Author

Owner

Work started on branch ticket-717 via /os-sdlc:implement (session 89876543). #718 merged first (289a1654).

Work started on branch ticket-717 via /os-sdlc:implement (session 89876543). #718 merged first (289a1654).

jared commented

2026-09-25 16:23:02 +00:00

Author

Owner

Resolution

Done: Worktree::Git is the sole git spawner; AfterMerge(root:, git:, sweep:, stop_server:) owns refresh-plugins, rubocop stop and usage sweep; FinishCommand/ConflictScan/Removal/RepoRoot/StartCommand take git:; Invocation built in bin/worktree and fetched with config.fetch(:invocation); env:, Thread.current and BinaryLocator deleted; base_branch memoized; FakeGit and RecordingAfterMerge doubles added

Evidence: branch ticket-717 commit aa3475d2; os suite 26 runs 0 failures; grep ENV|Thread.current|Dir.pwd in plugins/os/lib empty; zero real rubocop spawns verified with a PATH-planted logger; pipeline impl 43 ran through programmer green then looped red/review 5 times because criteria were already satisfied (#724), hand-finished

Follow-ups: #719 (base_branch memoization) already satisfied by this change, close by verification; #724 map cannot converge after verifier fail when production is already green; #725 shared scratchpad payload file name; Worktree::Git duplication noted on #699

## Resolution **Done:** Worktree::Git is the sole git spawner; AfterMerge(root:, git:, sweep:, stop_server:) owns refresh-plugins, rubocop stop and usage sweep; FinishCommand/ConflictScan/Removal/RepoRoot/StartCommand take git:; Invocation built in bin/worktree and fetched with config.fetch(:invocation); env:, Thread.current and BinaryLocator deleted; base_branch memoized; FakeGit and RecordingAfterMerge doubles added **Evidence:** branch ticket-717 commit aa3475d2; os suite 26 runs 0 failures; grep ENV|Thread.current|Dir.pwd in plugins/os/lib empty; zero real rubocop spawns verified with a PATH-planted logger; pipeline impl 43 ran through programmer green then looped red/review 5 times because criteria were already satisfied (#724), hand-finished **Follow-ups:** #719 (base_branch memoization) already satisfied by this change, close by verification; #724 map cannot converge after verifier fail when production is already green; #725 shared scratchpad payload file name; Worktree::Git duplication noted on #699

jared closed this issue

2026-09-25 16:23:03 +00:00

jared referenced this issue

2026-09-25 16:25:10 +00:00

os: memoize base_branch so finish runs git symbolic-ref once #719

jared referenced this issue from a commit

2026-09-26 13:57:00 +00:00

refactor(os): FinishCommand takes git: and after_merge: collaborators; Worktree::Git is the only git spawner; Invocation built in bin/worktree; env:, thread-local and BinaryLocator removed (#717, ADR-0220)

jared referenced this issue from a commit

2026-09-26 13:57:00 +00:00

docs: History rows for #717 and #718 (ADR-0220)