Bind workflow operations to verified provider revisions - #280
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds provider-bound operation snapshots, secure lifecycle leases, validated executable bindings, managed and pinned provider handling, operation runners, immutable storage, strict schemas, and updates across workflows, skills, documentation, and tests. ChangesProvider operation foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cdd721d to
d8f6f22
Compare
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review: PR #280 — Bind workflow operations to verified provider revisionsThis is a large PR (98 files, ~11.8k additions) introducing a new "operation" subsystem so skill/workflow/executable assets resolve against one immutable per-revision snapshot instead of mutable checkout paths, plus a managed/pinned install split, lifecycle/lease/GC machinery, and routing of autonomous-merge decisions through it. I reviewed it in parallel across the core resolver/store, security-sensitive git/path handling, lifecycle/lease concurrency, install/upgrade scripts, the pr-batch merge-decision code, and the doc/skill glue. Overall, this is unusually defensive code for its class — argv-array subprocess calls throughout (no shell-string interpolation found), Git-tree-hash verification of on-disk snapshots, PATH-independent trusted-executable resolution, atomic stage-then-rename state publication, and genuinely non-happy-path test coverage in most of the security-critical files (symlink/traversal attacks, GC races, forged reentry tokens, malformed state). The concrete issues found are narrow, mostly at the edges of an otherwise careful design, not systemic. Findings posted inline
Additional findings (not on lines included in the diff hunks, so noted here)
Minor/code-quality nits (no action likely needed)
What held up well
No blocking correctness bugs found in the core snapshot-immutability/GC-reference-counting model itself (which has real, non-trivial test coverage for the race conditions it's designed to prevent). The lock-fd-leak finding is the one I'd want addressed before merge given it touches the concurrency guarantee the whole subsystem is built on; the rest are worth triaging but lower urgency. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
bin/agent_workflows_operation/secure_git.rb (2)
176-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the private
capture3.The name matches
Open3.capture3, but the semantics differ. This method enforces@timeout, creates a process group, and raisesGitErroron timeout. A reader at the call sites on Lines 71 and 164 can assume plainOpen3behavior and miss the timeout path.A name such as
capture_with_guardianstates the added behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent_workflows_operation/secure_git.rb` at line 176, Rename the private capture3 method to a name that communicates its timeout and process-group safeguards, such as capture_with_guardian, and update every internal call site including the uses near the reported lines. Preserve the existing command execution and GitError timeout behavior.
239-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
timeoutparameter to distinguish it from@timeout.In this method
timeoutis the pipe write end (used at Line 252), and@timeoutis the duration in seconds (used at Line 240). The two names differ only by the instance-variable sigil, and both appear in the same control path.Rename the parameter to
timeout_writer, and update the keyword at Line 224 and Line 210.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent_workflows_operation/secure_git.rb` around lines 239 - 252, Rename the wait_for_process_group! parameter from timeout to timeout_writer, update its timeout.write usage, and adjust all callers and keywords at the referenced call sites to pass timeout_writer while preserving `@timeout` as the duration.bin/agent_workflows_operation/process_supervisor.rb (1)
55-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
mirror_statusin two guardian implementations. Both files implement the same signal-mirroring contract with byte-identical bodies: exit withexitstatuswhen the child exited, otherwise restoreSYSTEM_DEFAULTfor the terminating signal, re-raise it, and fall back to128 + signal. The shared root cause is one contract expressed twice, so a later correction to one copy can leave the other stale.
bin/agent_workflows_operation/process_supervisor.rb#L55-L66: extract this body into one shared helper that both guardians call.bin/agent_workflows_operation/secure_git.rb#L296-L307: remove this copy and call the shared helper.SecureGitalready referencesAgentWorkflowsOperationconstants, so the helper is reachable without a new dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent_workflows_operation/process_supervisor.rb` around lines 55 - 66, The mirror_status implementation is duplicated across both guardians and should be centralized. In bin/agent_workflows_operation/process_supervisor.rb lines 55-66, extract the existing signal-mirroring body into one shared AgentWorkflowsOperation helper; in bin/agent_workflows_operation/secure_git.rb lines 296-307, remove SecureGit’s duplicate mirror_status body and call that helper instead, preserving the existing exitstatus, signal restoration, re-raise, and fallback behavior.bin/agent-workflows-operation-test.rb (4)
477-478: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not signal a process group through an unverified leader pid.
Line 477 sends
KILLto the group id-git_pid. The test never asserts thatgit_pidleads its own process group, unlike the sibling test at Line 412. Ifgit_pidis not a group leader, this call signals an unrelated process group in CI. Signal the process directly, or assert group leadership first.♻️ Proposed cleanup change
ensure - Process.kill("KILL", -git_pid) if git_pid && process_alive?(git_pid) + Process.kill("KILL", git_pid) if git_pid && process_alive?(git_pid) Process.kill("KILL", guardian_pid) if guardian_pid && process_alive?(guardian_pid) Process.kill("KILL", resolver_pid) if resolver_pid && process_alive?(resolver_pid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent-workflows-operation-test.rb` around lines 477 - 478, Update the cleanup block around git_pid and guardian_pid so it does not signal an unverified process group: replace the negative git_pid target with direct-process signaling, or add the same process-group-leader assertion used by the sibling test before signaling the group. Preserve the existing liveness checks and guardian cleanup behavior.
2095-2099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail with a clear message when procfs does not yield a parent pid.
If the target process exits before Line 2096,
File.readraisesErrno::ENOENT. If the regex does not match, Line 2098 raisesNoMethodErroronnil. The only caller kills processes around this call, so both cases are reachable. Raise an explicit failure instead.♻️ Proposed helper hardening
def process_parent(pid) - status = File.read("/proc/#{pid}/status") - match = status.match(/^PPid:\s+(\d+)$/) - Integer(match[1], 10) + status = begin + File.read("/proc/#{pid}/status") + rescue Errno::ENOENT + flunk "process #{pid} exited before its parent could be read" + end + match = status.match(/^PPid:\s+(\d+)$/) + flunk "no PPid entry for process #{pid}" unless match + Integer(match[1], 10) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent-workflows-operation-test.rb` around lines 2095 - 2099, Harden process_parent so failures reading /proc or parsing the PPid entry raise a clear explicit error instead of exposing Errno::ENOENT or nil indexing. Handle File.read failures for an exited target process and validate the match before calling Integer, while preserving the existing parent-PID return path.
400-401: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWait for parsable pid content, not for file existence. All three sites treat
File.file?(ready)as the readiness signal, but the fixture creates the file and writes the pids in a separate step. A read that lands between those steps returns an empty or partial string, andIntegerthen raisesTypeErrororArgumentErrorinstead of failing on the behavior under test. Gate each wait on successfully parsed content.
bin/agent-workflows-operation-test.rb#L400-L401: wait untilFile.read(ready).split.length == 2, then parsegit_pidandguardian_pid.bin/agent-workflows-operation-test.rb#L459-L460: apply the same content check before parsing the flood pids.bin/agent-workflows-operation-test.rb#L1034-L1035: wait untilFile.read(ready)matches/\A\d+\s*\z/before callingIntegerforcapability_guardian.♻️ Proposed readiness helper
def wait_for_pids(path, count) values = nil assert(wait_until(timeout: 5) do values = File.file?(path) ? File.read(path).split : [] values.length == count && values.all? { |value| value.match?(/\A\d+\z/) } end, "#{path} never reported #{count} pid(s)") values.map { |value| Integer(value, 10) } end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent-workflows-operation-test.rb` around lines 400 - 401, Replace file-existence readiness checks with successful PID-content checks before parsing: at bin/agent-workflows-operation-test.rb#L400-L401 wait for exactly two valid PID tokens before assigning git_pid and guardian_pid; apply the same validation at `#L459-L460` before parsing the flood PIDs; at `#L1034-L1035` wait for content matching a single numeric PID before converting it to capability_guardian. Reuse a shared readiness helper if appropriate.
354-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
SecureGitdirectly or avoid private instance-variable coupling.
AgentWorkflowsOperation::SecureGit#initializeacceptstimeout:and sets@executablefrom known Git candidates, so these tests can instantiateSecureGitdirectly withtimeout:if they use the system Git selector, or define a test double that does not depend on private@executable/@timeoutnames. If a helper remains, put it in one place so the constructor shape is duplicated once instead of three times.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/agent-workflows-operation-test.rb` around lines 354 - 360, Update the test setup around AgentWorkflowsOperation::SecureGit to avoid overriding its initialize method and coupling the tests to private `@executable` and `@timeout` variables. Instantiate SecureGit directly with the existing timeout: argument where the system Git selector is suitable, or centralize a helper/test double in one place if custom executables are required, so the constructor shape is duplicated only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/agent_workflows_operation/secure_git.rb`:
- Around line 268-273: Bound the post-KILL wait loop in the surrounding
process-termination method using the class’s existing monotonic-time deadline
pattern, so it exits after the configured limit even if
process_group_alive?(pid) remains true. After this bounded wait, replace the
conditional reap_blocking(pid) call with a nonblocking reap to avoid blocking
when the leader is still running.
- Around line 192-199: Bound the reader thread wait in the main flow around
wait_for_process_group! by replacing the unbounded readers.each(&:join) with
joins using TERMINATION_GRACE_SECONDS, matching the existing ensure behavior.
Retrieve readers’ values defensively after the bounded joins so incomplete or
terminated reader threads cannot cause an additional blocking wait, while
preserving the existing timeout error and returned stdout, stderr, and status.
---
Nitpick comments:
In `@bin/agent_workflows_operation/process_supervisor.rb`:
- Around line 55-66: The mirror_status implementation is duplicated across both
guardians and should be centralized. In
bin/agent_workflows_operation/process_supervisor.rb lines 55-66, extract the
existing signal-mirroring body into one shared AgentWorkflowsOperation helper;
in bin/agent_workflows_operation/secure_git.rb lines 296-307, remove SecureGit’s
duplicate mirror_status body and call that helper instead, preserving the
existing exitstatus, signal restoration, re-raise, and fallback behavior.
In `@bin/agent_workflows_operation/secure_git.rb`:
- Line 176: Rename the private capture3 method to a name that communicates its
timeout and process-group safeguards, such as capture_with_guardian, and update
every internal call site including the uses near the reported lines. Preserve
the existing command execution and GitError timeout behavior.
- Around line 239-252: Rename the wait_for_process_group! parameter from timeout
to timeout_writer, update its timeout.write usage, and adjust all callers and
keywords at the referenced call sites to pass timeout_writer while preserving
`@timeout` as the duration.
In `@bin/agent-workflows-operation-test.rb`:
- Around line 477-478: Update the cleanup block around git_pid and guardian_pid
so it does not signal an unverified process group: replace the negative git_pid
target with direct-process signaling, or add the same process-group-leader
assertion used by the sibling test before signaling the group. Preserve the
existing liveness checks and guardian cleanup behavior.
- Around line 2095-2099: Harden process_parent so failures reading /proc or
parsing the PPid entry raise a clear explicit error instead of exposing
Errno::ENOENT or nil indexing. Handle File.read failures for an exited target
process and validate the match before calling Integer, while preserving the
existing parent-PID return path.
- Around line 400-401: Replace file-existence readiness checks with successful
PID-content checks before parsing: at
bin/agent-workflows-operation-test.rb#L400-L401 wait for exactly two valid PID
tokens before assigning git_pid and guardian_pid; apply the same validation at
`#L459-L460` before parsing the flood PIDs; at `#L1034-L1035` wait for content
matching a single numeric PID before converting it to capability_guardian. Reuse
a shared readiness helper if appropriate.
- Around line 354-360: Update the test setup around
AgentWorkflowsOperation::SecureGit to avoid overriding its initialize method and
coupling the tests to private `@executable` and `@timeout` variables. Instantiate
SecureGit directly with the existing timeout: argument where the system Git
selector is suitable, or centralize a helper/test double in one place if custom
executables are required, so the constructor shape is duplicated only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9411530-3fce-49a2-9cb5-dead54abcc9a
📒 Files selected for processing (14)
bin/agent-workflows-lifecycle-test.rbbin/agent-workflows-operation-test.rbbin/agent-workflows-resolvebin/agent-workflows-runbin/agent_workflows_operation/lifecycle_lease.rbbin/agent_workflows_operation/process_supervisor.rbbin/agent_workflows_operation/runner.rbbin/agent_workflows_operation/secure_git.rbbin/agent_workflows_operation/state.rbbin/provider-operation-contract-test.rbdocs/host-adapter/contract.mddocs/installation-and-upgrades.mddocs/plans/2026-07-25-bound-provider-snapshot-design.mdworkflows/adversarial-pr-review.md
🚧 Files skipped from review as they are similar to previous changes (10)
- bin/agent-workflows-run
- bin/provider-operation-contract-test.rb
- workflows/adversarial-pr-review.md
- docs/installation-and-upgrades.md
- bin/agent-workflows-resolve
- docs/plans/2026-07-25-bound-provider-snapshot-design.md
- bin/agent_workflows_operation/lifecycle_lease.rb
- bin/agent_workflows_operation/runner.rb
- bin/agent_workflows_operation/state.rb
- docs/host-adapter/contract.md
Review summary — PR 280 (Bind workflow operations to verified provider revisions)This is a large (98 files, ~12.4k additions) rework introducing an immutable, provider-bound "operation" registry for skills/workflows/executables, plus a pinned/managed install split and hardened process/git primitives. I reviewed it across four areas in parallel (core registry/store/resolver, security-critical process/git/path primitives, install/upgrade/status, and the autonomous merge capability scripts). Overall the design is unusually rigorous for this kind of infrastructure: real content-hash verification against Git objects, symlink-ancestor traversal checks, close-on-exec lease descriptors backed by real /proc/self/fd tests, array-form subprocess invocation everywhere (no shell injection surface found anywhere in scope), and consistent unsetenv_others: true hardening at trusted subprocess boundaries. I did not find any place where a registered capability falls back to reading from a mutable checkout path instead of the immutable snapshot, which was the core integrity property this PR set out to establish. I have left inline comments on the specific, actionable findings. Summary, most severe first: High severity
Medium severity
Low severity / consistency / test coverage
Other observations (not filed inline, lower confidence/impact)
What looks solid
Given the size and security sensitivity of this change, I would suggest at minimum addressing the two high-severity findings (upgrade rollback-on-cleanup-failure, and archive symlink-escape-before-validation) before merge, and adding the missing adversarial tests for the runtime-trust and CI-readiness executable-binding checks given how central they are to the autonomous-merge safety story. |
…ling-workflow-provider # Conflicts: # skills/pr-batch/SKILL.md
1 participant
Why
Shared workflow instructions, helper executables, and host plugin state could previously come from different revisions. A session could load a current skill while invoking an older local helper, or continue reading mutable provider files while an upgrade replaced them. Status and upgrade paths also disagreed about what "current" meant, particularly for linked worktrees, Claude commit-based plugins, and managed installations following the canonical provider branch.
This made failures such as merge-queue submission errors recur even though the shared pack already contained the correct helper. More importantly, it meant a long-running operation could not prove that all of its workflow assets came from one reviewed provider state.
What changed
Provider-bound operations
begin,list,run, andreleaseentrypoints with private state, schemas, shared/exclusive leases, explicit capacity limits, and reference-derived garbage collection.Pinned and managed profiles
shakacode/agent-workflowsatrefs/heads/main.main, and reinstall the already-established revision without a second-fetch race.Host and tool integrity
0.1.0manifest version.unsetenv_othersenvironment and bounded timeout; ambientAGENT_WORKFLOWS_CODEX_*variables are no longer authority.Upgrade and recovery safety
User impact
Installed workflows can now operate in either of two explicit modes:
An operation receives paths and runners from one immutable provider revision. Upgrading the installed pack no longer changes the files already bound to an active operation, and missing capabilities fail clearly instead of falling back to unrelated stale copies.
Review guide
The commits are arranged as incremental, validated layers:
The design rationale is documented in
docs/plans/2026-07-25-bound-provider-snapshot-design.md.Validation
Validated after integrating
mainat36709f8d8649f6d50f0cf609896001ac9ea096d3and completing the final provider-lifecycle review fixes at98633dd11e1b68bb13166314c3092b6e67a5e774:The final Linux run supplied a temporary
shasumcompatibility wrapper backed by Ruby's SHA-1 implementation because this machine does not install Perl'sshasum; the affected test uses the digest only to compare before/after metadata identity.Additional evidence developed during implementation includes:
PATHproof that readiness and assurance use the recorded GitHub executable;0777active Ruby interpreter, a0500operation snapshot, real capability execution, and interpreter-swap rejection;243 runs,1,422 assertions, zero failures/errors);--update-shallow, followed by exact-ref import success and a regression test for pinned imports from shallow provider checkouts;commandreproduction matching GitHub's Ubuntu runner, plus lifecycle coverage using the POSIX shell builtin explicitly;SKILL.mdas a directory;No meaningful issues found.Summary by CodeRabbit