Report unsupported signed launch readiness - #306
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
WalkthroughThe PR adds signed-launch readiness inspection, secure host-owned provisioning, unsupported Codex waivers, waived dispatcher replay, and lifecycle waiver validation. Partial or unsafe host state remains ChangesSigned-launch readiness and waiver handling
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant AgentWorkflowsStatus
participant SignedLaunchReadiness
participant DispatcherPreflight
participant BatchPlanPreflight
Operator->>AgentWorkflowsStatus: request readiness
AgentWorkflowsStatus->>SignedLaunchReadiness: assess host and target
SignedLaunchReadiness-->>AgentWorkflowsStatus: return readiness state
Operator->>DispatcherPreflight: provide unsupported-host waiver
DispatcherPreflight-->>Operator: persist waived-active assignment
Operator->>BatchPlanPreflight: provide lifecycle waiver
BatchPlanPreflight-->>Operator: include waived lane in completion
Possibly related PRs
🚥 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 |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review summaryThis PR adds a "signed-launch readiness" capability that is deliberately fail-closed: it reports What the PR does
Issues found (posted inline)
Other observations (not filed as separate comments)
Overall: solid, well-tested security design with one concrete CI-breaking bug and one latent naming footgun to fix before merge. |
![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: 6f39f42257
ℹ️ 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".
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
bin/agent_doctor/signed_launch_waiver_record.rb (1)
78-82: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider restricting
durable_ref?schemes andmerge_authorityvalues.
durable_ref?accepts any absolute URI.file:///tmp/anythingandhttp://…both pass, so a waiver can point its evidence at a local or unauthenticated location while still satisfying validation.constraints?accepts any nonemptymerge_authoritystring, so a typo such asauto_merge_when_gate_passvalidates and later comparisons silently miss.Add a scheme allowlist for evidence references and an explicit value allowlist for
merge_authority. Both checks are local to this module.Also applies to: 117-121
🤖 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/signed_launch_waiver_record.rb` around lines 78 - 82, Restrict durable_ref? to an explicit allowlist of approved URI schemes instead of accepting every absolute URI, while continuing to return false for invalid or disallowed references. In constraints?, validate merge_authority against its supported values and reject nonempty unknown strings so later comparisons cannot silently miss; keep both allowlists local to this module.skills/plan-pr-batch/bin/batch-plan-preflight-test.rb (1)
667-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
lanerejection variant to exercise the waiver lane binding.The
lanevariant setslane_idto"lane-z", which no planned lane declares.validate_lane_lifecycle_recordsrejects it at thelanes_by_id[waiver["lane_id"]]lookup, beforevalidate_lifecyclecompares the lane againstauthorized_lanes. The variant therefore proves the lane-lookup guard, not the waiver lane binding.Add a second planned lane that is absent from
authorized_lanesto cover the binding itself.♻️ Suggested additional coverage
def test_lifecycle_waiver_rejects_a_planned_lane_outside_authorized_lanes helper, root = installed_unsupported_workflow_control_helper route = { "model" => "gpt-5.6-sol", "effort" => "xhigh" } waiver_path, waiver_record = bootstrap_waiver( root, batch_id: "batch-plan-1", lane_id: "lane-a", route: ) waiver = lane_lifecycle_waiver( path: waiver_path, record: waiver_record, route:, lane_id: "lane-b" ) input = input_for( lanes: [lane("lane-a"), lane("lane-b")], backend: "codex", lifecycle_waivers: [waiver] ) result, _stderr, status = evaluate(input, helper: helper) refute status.success? assert_includes result.fetch("violations").map { |item| item.fetch("code") }, "lane-lifecycle-waiver-invalid" assert_empty result.dig("launch", "completed_lane_ids") 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 `@skills/plan-pr-batch/bin/batch-plan-preflight-test.rb` around lines 667 - 680, Strengthen the lane rejection coverage in the lifecycle waiver tests by adding a case where the waiver targets a second planned lane that is not included in authorized_lanes. Update the relevant test setup around lane_lifecycle_waiver and input_for to include both planned lanes, then assert the existing lane-lifecycle-waiver-invalid violation and no completed lanes, ensuring rejection occurs during lifecycle authorization rather than lane lookup.
🤖 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_doctor/signed_launch_readiness.rb`:
- Around line 82-88: Update clean_unsupported_host? so clean unsupported hosts
such as Claude are handled explicitly: either extend the clean-host eligibility
branch beyond "codex" so a safe, state-free Claude home is waivable, or clearly
identify Claude as out of scope in the readiness reason and installation
documentation. Ensure clean Claude hosts do not fall through to unknown(host)
with an unsatisfiable remediation requirement.
- Line 14: Rename AgentDoctor::SignedLaunchReadiness.inspect to
inspect_readiness (or readiness) so it no longer shadows Module#inspect, then
update all three call sites to use the new method name while preserving the
existing host and target arguments.
In `@bin/agent_doctor/signed_launch_waiver_record.rb`:
- Around line 31-44: Update signed_launch_waiver_record’s read method to
validate every directory ancestor from the waiver path through the filesystem
root, ensuring each is owned by helper_uid and has no group/world write bits;
also eliminate the lstat-to-read replacement window by opening the waiver once,
validating the opened file descriptor’s stat, and reading from that descriptor.
In `@bin/agent_doctor/signed_launch_waiver.rb`:
- Around line 98-103: Update lifecycle_receipt_ref to prevent slash-delimited
identifier collisions by validating or percent-encoding batch_plan_id,
stage_dependency_plan_id, wave, lane_id, and state before interpolation.
Preserve the existing receipt_ref structure while ensuring each component maps
unambiguously to one lifecycle record.
- Around line 105-124: Update dispatcher_observation_valid? so instance_id and
launch_token must each be nonempty on both the observation and assignment before
or alongside their equality checks. Preserve the existing matching behavior
while rejecting nil and empty-string identities, using the same nonempty
validation convention already applied to dispatcher/bootstrap identity fields.
In `@bin/agent-workflows-status`:
- Around line 176-179: Update the signed_launch_readiness call in status_payload
to guard against a nil target, returning the UNKNOWN readiness record directly
when target is missing. Only invoke SignedLaunchReadiness.inspect for a resolved
target, including in the rescue path that passes options[:target], so readiness
is never evaluated against the current working directory.
In `@bin/agent-workflows-status-test.rb`:
- Around line 60-61: Add a minimum RSA modulus-size validation to public_anchor?
in signed_launch_readiness.rb, requiring RSA keys to have at least 2048 bits
before reporting them as supported. Update the RSA fixture generation in
agent-workflows-status-test.rb, including dispatcher_key and workflow_key, to
use a compliant size or a shared per-run key while preserving test behavior.
In `@bin/install-agent-workflows`:
- Around line 763-768: Make the signed_launch_capability probe around
AgentDoctor::SignedLaunchReadiness.inspect non-fatal under set -e by handling a
failed ruby command substitution and assigning UNKNOWN as the fallback value.
Preserve the existing capability output when the probe succeeds so the
completion message, PATH instruction, and successful install status are always
emitted.
- Around line 282-303: Update ensure_signed_launch_directory to validate both
$target and $target/.agents as non-symlink directories with matching helper
ownership and no group/world-writable permissions, emitting a diagnostic that
identifies $target when validation fails. Align the installer’s ownership
reference with signed_launch_readiness.rb’s __FILE__-based check so
shared-checkout installs validate ownership against the readiness script and
installed runner consistently.
In `@bin/install-agent-workflows-test.bash`:
- Line 246: Replace the BSD-specific stat invocation in the .agents permission
assertion with the existing Ruby File.lstat-based permission check used at line
199, preserving the expected 777 comparison and failure message.
---
Nitpick comments:
In `@bin/agent_doctor/signed_launch_waiver_record.rb`:
- Around line 78-82: Restrict durable_ref? to an explicit allowlist of approved
URI schemes instead of accepting every absolute URI, while continuing to return
false for invalid or disallowed references. In constraints?, validate
merge_authority against its supported values and reject nonempty unknown strings
so later comparisons cannot silently miss; keep both allowlists local to this
module.
In `@skills/plan-pr-batch/bin/batch-plan-preflight-test.rb`:
- Around line 667-680: Strengthen the lane rejection coverage in the lifecycle
waiver tests by adding a case where the waiver targets a second planned lane
that is not included in authorized_lanes. Update the relevant test setup around
lane_lifecycle_waiver and input_for to include both planned lanes, then assert
the existing lane-lifecycle-waiver-invalid violation and no completed lanes,
ensuring rejection occurs during lifecycle authorization rather than lane
lookup.
🪄 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: 0aa35604-5057-4a6d-95fe-0ce5c69ad680
📒 Files selected for processing (19)
CHANGELOG.mdCONTEXT.mdbin/agent-workflows-statusbin/agent-workflows-status-test.rbbin/agent_doctor/signed_launch_readiness.rbbin/agent_doctor/signed_launch_waiver.rbbin/agent_doctor/signed_launch_waiver_record.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashdocs/agent-workflows-model-routing.mddocs/installation-and-upgrades.mddocs/pr-batch-skills.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/bin/batch-plan-preflightskills/plan-pr-batch/bin/batch-plan-preflight-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/dispatcher-capability-preflightskills/pr-batch/bin/dispatcher-capability-preflight-test.rbworkflows/pr-processing.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccc37d59d1
ℹ️ 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".
Review feedback addressed at
|
Review: Report unsupported signed launch readiness (#306)This PR adds a new Overview
Correctness issue (likely blocks real installs) Security consistency nit Other observations (not blocking)
Nice test coverage overall (symlink/ownership/permission negative tests, digest-mutation rejection, cross-batch/lane/route reuse rejection, chronology bounds, URI-scheme allowlist). The main actionable item before merge is the installer ownership check. |
ReviewOverviewThis PR adds a The scope is large (19 files, ~2.2k additions) but almost entirely additive, and the new adversarial test coverage is unusually thorough — mutation tests on the waiver record, cross-batch/lane/route/dispatcher reuse rejection, symlink/ancestor-chain attacks, RSA key-length floors, chronology bounds, etc. Code quality / style
FindingsI left two inline comments on concrete issues:
Other observations (not blocking)
RiskLow-to-moderate. The core waiver trust boundary (filesystem ownership of the waiver record, replay binding via canonical SHA-256 digest, exact batch/lane/dispatcher/route matching) is well thought out and heavily tested, and the two issues above are edge cases rather than exploitable bypasses of the readiness/waiver gates themselves. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 181f10e8e2
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/agent-workflows-status-test.rb (1)
227-231: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse a compliant-size private-key fixture.
Line [230] uses a 1024-bit private key. The test can pass because the validator rejects the key size, even if it accepts private-key material. Use
STRONG_RSA_KEY.to_pemfor this scenario and keep the separate 1024-bit test for key-size rejection.Proposed fix
- record["agent_workflow_dispatcher_trusted_public_key_pem"] = OpenSSL::PKey::RSA.generate(1024).to_pem + record["agent_workflow_dispatcher_trusted_public_key_pem"] = STRONG_RSA_KEY.to_pem🤖 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-status-test.rb` around lines 227 - 231, Update the “private-key” scenario in the fixture setup to assign STRONG_RSA_KEY.to_pem instead of generating a 1024-bit RSA key, while preserving the separate 1024-bit fixture for testing key-size rejection.skills/plan-pr-batch/bin/batch-plan-preflight-test.rb (1)
297-316: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd contract coverage for
lane_lifecycle_waivers.
input_foralways emitslane_lifecycle_waivers, while the helper treats a missing key as[]and only validates it if present. Add a test wherelane_lifecycle_waiversis omitted so the pre-v2/backward-compatibility contract is documented explicitly.🤖 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/bin/batch-plan-preflight-test.rb` around lines 297 - 316, Add a contract test for the pre-v2/backward-compatible input path that omits lane_lifecycle_waivers entirely, rather than passing an empty array. Update or extend input_for usage as needed to remove the key from the generated payload, then assert the preflight validation accepts the missing key and treats it as empty.
🧹 Nitpick comments (3)
bin/install-agent-workflows-test.bash (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
statshim decorate the real stat object, as thelstatshim does.The
statoverride returnsStruct.new(:uid), which exposes onlyuid. Thelstatoverride keeps the real stat and redefinesuid. If any code under test reads another field such asmodeordirectory?from the shimmed path, thestatshim raisesNoMethodErrorinstead of failing the intended assertion. Use the same decoration in both overrides.♻️ Proposed symmetric shim
def stat(path, *args) result = agent_workflows_original_stat(path, *args) return result unless File.expand_path(path.to_s) == ENV["AGENT_WORKFLOWS_TEST_SOURCE_OWNER_PATH"] - Struct.new(:uid).new(Process.euid.zero? ? 1 : 0) + result.define_singleton_method(:uid) { Process.euid.zero? ? 1 : 0 } + result 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/install-agent-workflows-test.bash` around lines 65 - 70, Update the stat override to retain the object returned by agent_workflows_original_stat and decorate its uid value, matching the existing lstat override. Preserve all other File::Stat methods and attributes, including mode and directory?, while keeping the existing uid behavior for AGENT_WORKFLOWS_TEST_SOURCE_OWNER_PATH.skills/pr-batch/bin/dispatcher-capability-preflight-test.rb (1)
2760-2764: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the exact rejection reason, as the sibling trust-anchor tests do.
The test asserts only
invalid-input. Every other trust-anchor rejection test in this file also asserts"launch_confirmation must be a well-formed identity-bound confirmation". Without that assertion, the test still passes if a later change rejects the input for an unrelated reason, so it would stop proving that the RSA-1024 anchor is what fails.♻️ Proposed assertion
assert_equal "invalid-input", output.fetch("status") + assert_equal "launch_confirmation must be a well-formed identity-bound confirmation", + output.fetch("reason") refute(Array(output["active_assignments"]).any? do |assignment| assignment["lifecycle"] == "confirmed-active" 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 `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb` around lines 2760 - 2764, Add an assertion in the affected trust-anchor rejection test to verify the exact rejection reason is "launch_confirmation must be a well-formed identity-bound confirmation", alongside the existing status, active-assignment, and dispatch assertions. Use the same output field and assertion pattern as the sibling trust-anchor tests.bin/agent_doctor/signed_launch_readiness.rb (1)
101-112: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlign
read_recordwith the descriptor-based read used bySignedLaunchWaiverRecord.read.
read_recordvalidatesFile.lstat(path)and then callsFile.read(path). The path can be replaced between the two calls. Also,File.readfollows symlinks, so thestat.file?check does not prove the opened file is the inspected inode.bin/agent_doctor/signed_launch_waiver_record.rbalready opens once withFile::NOFOLLOWand validatesfile.stat. Use the same pattern here for consistent trust-material reads.🔒 Proposed descriptor-based read
def read_record(path, root:, agents:) return unless [root, agents].all? { |directory| safe_owned_directory?(directory) } helper_uid = File.stat(__FILE__).uid - stat = File.lstat(path) - return unless stat.file? && stat.uid == helper_uid && (stat.mode & 0o022).zero? - - record = JSON.parse(File.read(path, encoding: "UTF-8")) - record if record.is_a?(Hash) + File.open(path, File::RDONLY | File::NOFOLLOW) do |file| + stat = file.stat + next unless stat.file? && stat.uid == helper_uid && (stat.mode & 0o022).zero? + + record = JSON.parse(file.read.force_encoding("UTF-8")) + record if record.is_a?(Hash) + end rescue Errno::ENOENT, Errno::ENOTDIR nil 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/signed_launch_readiness.rb` around lines 101 - 112, Update read_record to open the record once using the descriptor-based pattern from SignedLaunchWaiverRecord.read, including File::NOFOLLOW, then validate the opened descriptor with file.stat for regular-file type, ownership, and permissions before parsing its contents. Replace the separate File.lstat and File.read calls while preserving the existing directory checks and rescue behavior.
🤖 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-status-test.rb`:
- Around line 371-382: Extend
test_check_failed_payload_without_resolved_target_reports_typed_unknown_readiness
to assert that signed_launch_readiness.ready is false and
signed_launch_readiness.waiver_allowed is false for the unresolved target
payload, while preserving the existing status, target, capability, and reason
assertions.
---
Outside diff comments:
In `@bin/agent-workflows-status-test.rb`:
- Around line 227-231: Update the “private-key” scenario in the fixture setup to
assign STRONG_RSA_KEY.to_pem instead of generating a 1024-bit RSA key, while
preserving the separate 1024-bit fixture for testing key-size rejection.
In `@skills/plan-pr-batch/bin/batch-plan-preflight-test.rb`:
- Around line 297-316: Add a contract test for the pre-v2/backward-compatible
input path that omits lane_lifecycle_waivers entirely, rather than passing an
empty array. Update or extend input_for usage as needed to remove the key from
the generated payload, then assert the preflight validation accepts the missing
key and treats it as empty.
---
Nitpick comments:
In `@bin/agent_doctor/signed_launch_readiness.rb`:
- Around line 101-112: Update read_record to open the record once using the
descriptor-based pattern from SignedLaunchWaiverRecord.read, including
File::NOFOLLOW, then validate the opened descriptor with file.stat for
regular-file type, ownership, and permissions before parsing its contents.
Replace the separate File.lstat and File.read calls while preserving the
existing directory checks and rescue behavior.
In `@bin/install-agent-workflows-test.bash`:
- Around line 65-70: Update the stat override to retain the object returned by
agent_workflows_original_stat and decorate its uid value, matching the existing
lstat override. Preserve all other File::Stat methods and attributes, including
mode and directory?, while keeping the existing uid behavior for
AGENT_WORKFLOWS_TEST_SOURCE_OWNER_PATH.
In `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb`:
- Around line 2760-2764: Add an assertion in the affected trust-anchor rejection
test to verify the exact rejection reason is "launch_confirmation must be a
well-formed identity-bound confirmation", alongside the existing status,
active-assignment, and dispatch assertions. Use the same output field and
assertion pattern as the sibling trust-anchor tests.
🪄 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: d206cf7f-6716-40cd-9214-d525d979ca04
📒 Files selected for processing (17)
CHANGELOG.mdCONTEXT.mdbin/agent-workflows-statusbin/agent-workflows-status-test.rbbin/agent_doctor/signed_launch_readiness.rbbin/agent_doctor/signed_launch_waiver.rbbin/agent_doctor/signed_launch_waiver_record.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashdocs/installation-and-upgrades.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/bin/batch-plan-preflightskills/plan-pr-batch/bin/batch-plan-preflight-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/dispatcher-capability-preflightskills/pr-batch/bin/dispatcher-capability-preflight-test.rbworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- CONTEXT.md
- skills/plan-pr-batch/bin/batch-plan-preflight
- skills/pr-batch/bin/dispatcher-capability-preflight
- workflows/pr-processing.md
- bin/agent_doctor/signed_launch_waiver.rb
- bin/agent-workflows-status
- bin/install-agent-workflows
…gned-launch-readiness * origin/main: fix: allow YAML timestamps in action scanner (#305)
Review: Report unsupported signed launch readiness (#306)OverviewThis PR adds a new, self-contained "signed launch readiness" subsystem: Strengths
Findings (posted inline)
Other notes (not blocking)
No security vulnerabilities (injection, path traversal, TOCTOU beyond the pre-existing pattern) found beyond what's noted above; the new code is fail-closed by default everywhere I checked. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5490d2747d
ℹ️ 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".
Review: Report unsupported signed launch readiness (#299)OverviewThis PR adds a "signed-launch readiness" subsystem that is deliberately independent from install freshness, plus a tightly-scoped human waiver path for Codex hosts that haven't yet integrated host-owned signing. Main pieces:
Code quality
Security
Concerns / suggestions
PerformanceNegligible — a handful of small local Test coverageStrong on both new libraries and the two modified preflight scripts, with good negative-case coverage (mutation, replay-after-expiry, cross-lane/batch/route reuse, weak keys, symlinked install roots, duplicate signed+waived records for one lane). Note: given the size of this diff, I focused on the core trust logic ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b2e1e2c9
ℹ️ 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".
|
Address-review checkpoint for head 3768b06. Scope: review activity since the previous checkpoint at 2026-08-02T10:54:55Z. Resolved outcomes:
Validation:
All five source threads have evidence replies and are resolved. Historical review-summary bodies in scope did not add a distinct unresolved blocker beyond their inline findings. Current-head hosted validation and configured reviews remain the merge gate. |
…gned-launch-readiness * origin/main: Report source lines in security preflight findings (#311)
ReviewOverviewThis PR adds a Strengths
Issues / suggestions
SecurityNo exploitable issue found on manual read-through. The trust boundary described in the PR body holds up under inspection: private keys stay host-owned, the installer never synthesizes capability/trust/waiver material, and partial/malformed/symlinked/writable state fails to Test coverageExtensive new Minitest coverage in |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
skills/plan-pr-batch/bin/batch-plan-preflight (1)
317-319: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject a missing stage dependency plan ID instead of raising
KeyError.Line 318 calls
stage_dependency_plan.fetch("id")beforevalidate_stage_dependenciesvalidates the object. If the plan is{}and a waiver names a valid lane, preflight raisesKeyErrorinstead of returning a rejected result.Proposed fix
def validate_lane_lifecycle_records(receipts, waivers, plan, stage_dependency_plan) receipt_violations, receipt_lane_ids = validate_lane_lifecycle_receipts(receipts, plan, stage_dependency_plan) violations = receipt_violations.dup waiver_lane_ids = [] + stage_dependency_plan_id = + stage_dependency_plan.is_a?(Hash) ? stage_dependency_plan["id"] : nil unless waivers.is_a?(Array) # ... end # ... valid = lane && valid_lane_lifecycle_waiver?( - waiver, plan, stage_dependency_plan.fetch("id"), lane + waiver, plan, stage_dependency_plan_id, lane )🤖 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/bin/batch-plan-preflight` around lines 317 - 319, Update the validation flow around valid_lane_lifecycle_waiver? so a missing "id" in stage_dependency_plan is rejected as an invalid result before calling fetch("id"), while preserving normal validation for plans that include an ID and preventing KeyError from escaping preflight.skills/pr-batch/SKILL.md (2)
839-843: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not expose GitHub tokens to interpreter validation unless the entire interpreter chain is trusted.
Both contracts accept an identity-recorded absolute interpreter spawn after a validated check; replacing
outside the consumer repositorywith ownership/fixed-path validation does not eliminate the TOCTOU risk that a replaced interpreter or writable ancestor directory can run with the guard’s GitHub environment.
skills/pr-batch/SKILL.md#L839-L843: require descriptor-based execution or a trusted-owner, non-writable interpreter chain.workflows/pr-processing.md#L2933-L2937: apply the same validation before spawning the guard.🤖 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/SKILL.md` around lines 839 - 843, Update the interpreter validation and spawn guidance in skills/pr-batch/SKILL.md lines 839-843 and workflows/pr-processing.md lines 2933-2937 so GitHub tokens are exposed only when the entire interpreter chain is trusted: require descriptor-based execution or validate trusted ownership and non-writable permissions for the interpreter and every ancestor directory before spawning the guard, and apply the same rule in both locations.
831-834: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize the source Git origin before invoking the trusted guard.
Both guarded-direct materializations preserve the source
originin the isolated Git root inside.git/config. If that origin carries URL userinfo, Git operations can expose credentials in the protected checkout and to child Git processes. Reject credential-bearing origins or materialize a redacted non-credential origin before the guard runs.🤖 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/SKILL.md` around lines 831 - 834, Sanitize the source Git origin before either guarded-direct materialization invokes the trusted guard: reject credential-bearing URL userinfo or replace it with a redacted non-credential origin before writing the isolated repository’s .git/config. Apply this guidance at skills/pr-batch/SKILL.md lines 831-834 and workflows/pr-processing.md lines 2925-2928, keeping the existing HEAD/index/worktree isolation and source-origin behavior for safe origins.
🧹 Nitpick comments (1)
bin/agent_doctor/signed_launch_installation.rb (1)
158-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the owner-validated JSON reader into one shared helper.
read_metadataduplicatesSignedLaunchWaiverRecord.read(bin/agent_doctor/signed_launch_waiver_record.rblines 36-49) almost exactly: the sameFile.const_defined?(:NOFOLLOW)guard, the same descriptor-levelfile?/uid/0o022checks, the same UTF-8 validation, and the same JSON object parse. Both sites gate trust decisions, so the two copies must not drift.
signed_launch_waiver_record.rbalready requires this file. Expose one reader here, for exampleread_owned_json(path, owner_uid), and call it from the waiver record reader after its ancestor-chain check.🤖 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/signed_launch_installation.rb` around lines 158 - 174, Extract the shared owner-validated JSON reading logic from read_metadata into a reusable private-class helper such as read_owned_json(path, owner_uid), preserving the NOFOLLOW, descriptor ownership/permission, UTF-8 validation, and Hash-only JSON behavior. Update read_metadata and SignedLaunchWaiverRecord.read to use this helper, with the waiver reader retaining its existing ancestor-chain validation before calling it.
🤖 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_doctor/signed_launch_waiver_record.rb`:
- Around line 18-20: Add the complete REQUIRED_GATES token list to the waiver
documentation generated by waiver, using the exact strings—including the entries
shown near REQUIRED_GATES—so authors can copy them and bootstrap? validation
succeeds.
In `@CONTEXT.md`:
- Line 150: Separate confirmation-record versioning from signed-observation
payload versioning so launch-confirmation v2 cannot serialize as version 1;
introduce distinct confirmation and observation version fields and update
replay/migration wording accordingly. Apply the same terminology and distinction
in CONTEXT.md:150, docs/agent-workflows-model-routing.md:308,
docs/pr-batch-skills.md:181, skills/pr-batch/SKILL.md:98,
skills/triage/SKILL.md:257, and workflows/pr-processing.md:781.
In `@docs/agent-workflows-model-routing.md`:
- Line 310: Clarify the naming of the durable bootstrap waiver record across the
documentation: either designate agent-workflow-bootstrap-waiver v1 as the
canonical name wherever launch_waiver v1 refers to the same record, or
explicitly document both as aliases. If they represent distinct records,
document each record’s lifecycle and conversion behavior.
In `@docs/installation-and-upgrades.md`:
- Around line 87-88: Use agent-workflow-bootstrap-waiver v1 consistently as the
durable human-approval record name across
docs/installation-and-upgrades.md:87-88,
docs/agent-workflows-model-routing.md:310, docs/pr-batch-skills.md:182,
skills/pr-batch/SKILL.md:99, and workflows/pr-processing.md:783. Replace
launch_waiver v1 references and descriptions of it as an input or wrapper, or
explicitly document its mapping and canonical serialized type if that wrapper
must remain.
---
Outside diff comments:
In `@skills/plan-pr-batch/bin/batch-plan-preflight`:
- Around line 317-319: Update the validation flow around
valid_lane_lifecycle_waiver? so a missing "id" in stage_dependency_plan is
rejected as an invalid result before calling fetch("id"), while preserving
normal validation for plans that include an ID and preventing KeyError from
escaping preflight.
In `@skills/pr-batch/SKILL.md`:
- Around line 839-843: Update the interpreter validation and spawn guidance in
skills/pr-batch/SKILL.md lines 839-843 and workflows/pr-processing.md lines
2933-2937 so GitHub tokens are exposed only when the entire interpreter chain is
trusted: require descriptor-based execution or validate trusted ownership and
non-writable permissions for the interpreter and every ancestor directory before
spawning the guard, and apply the same rule in both locations.
- Around line 831-834: Sanitize the source Git origin before either
guarded-direct materialization invokes the trusted guard: reject
credential-bearing URL userinfo or replace it with a redacted non-credential
origin before writing the isolated repository’s .git/config. Apply this guidance
at skills/pr-batch/SKILL.md lines 831-834 and workflows/pr-processing.md lines
2925-2928, keeping the existing HEAD/index/worktree isolation and source-origin
behavior for safe origins.
---
Nitpick comments:
In `@bin/agent_doctor/signed_launch_installation.rb`:
- Around line 158-174: Extract the shared owner-validated JSON reading logic
from read_metadata into a reusable private-class helper such as
read_owned_json(path, owner_uid), preserving the NOFOLLOW, descriptor
ownership/permission, UTF-8 validation, and Hash-only JSON behavior. Update
read_metadata and SignedLaunchWaiverRecord.read to use this helper, with the
waiver reader retaining its existing ancestor-chain validation before calling
it.
🪄 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: 108f54d3-a734-42e2-b3a3-61e27f23fc85
📒 Files selected for processing (21)
CHANGELOG.mdCONTEXT.mdbin/agent-workflows-status-test.rbbin/agent_doctor/signed_launch_installation.rbbin/agent_doctor/signed_launch_readiness.rbbin/agent_doctor/signed_launch_waiver.rbbin/agent_doctor/signed_launch_waiver_record.rbbin/install-agent-workflowsbin/install-agent-workflows-test.bashdocs/agent-workflows-model-routing.mddocs/installation-and-upgrades.mddocs/pr-batch-skills.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/bin/batch-plan-preflightskills/plan-pr-batch/bin/batch-plan-preflight-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/dispatcher-capability-preflightskills/pr-batch/bin/dispatcher-capability-preflight-test.rbskills/pr-batch/bin/model-routing-contract-test.rbskills/triage/SKILL.mdworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (8)
- bin/agent_doctor/signed_launch_readiness.rb
- CHANGELOG.md
- bin/install-agent-workflows
- bin/agent-workflows-status-test.rb
- bin/install-agent-workflows-test.bash
- skills/pr-batch/bin/dispatcher-capability-preflight
- skills/pr-batch/bin/dispatcher-capability-preflight-test.rb
- skills/plan-pr-batch/bin/batch-plan-preflight-test.rb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0526c9d4e4
ℹ️ 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".
…gned-launch-readiness * origin/main: Emit coordination telemetry and provenance at workflow checkpoints (#290) # Conflicts: # CONTEXT.md # docs/agent-workflows-model-routing.md # docs/pr-batch-skills.md # skills/pr-batch/SKILL.md # skills/pr-batch/bin/dispatcher-capability-preflight # skills/pr-batch/bin/model-routing-contract-test.rb # skills/triage/SKILL.md # workflows/pr-processing.md
Review: Report unsupported signed-launch readiness (#299)OverviewThis PR adds a Strengths
Issue foundTOCTOU inconsistency in the two pre-existing trust-anchor readers. Minor observations
SecurityThe core trust model looks sound: private keys stay host-owned, Test coverageCoverage is thorough and includes adversarial cases (symlinks, permission bits, weak keys, encoding, chronology, duplicate lifecycle records). No gaps stood out beyond the trust-anchor TOCTOU path itself not having a regression test (understandably, since it's a pre-existing pattern not newly introduced). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
skills/pr-batch/bin/dispatcher-capability-preflight-test.rb (1)
296-312: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe shim swallows
Errno::ENOENTfrom the originalFile.stat.The
rescueclause covers the whole method body, including line 301. Ifagent_workflows_original_statraisesErrno::ENOENTorErrno::ENOTDIR, the shim returnsnilinstead of propagating the error.File.statthen returnsnilfor missing paths for every path in the process, not only paths under the source root. That can change the failure mode of the code under test and maketest_validated_symlink_install_uses_the_installed_home_owner_for_dispatcher_trustpass for the wrong reason.Limit the rescue to the
File.realpathcall.🐛 Proposed fix to scope the rescue
def stat(path, *args) result = agent_workflows_original_stat(path, *args) source_root = ENV.fetch("AGENT_WORKFLOWS_TEST_SOURCE_OWNER_ROOT") - expanded = File.realpath(path.to_s) + expanded = begin + File.realpath(path.to_s) + rescue Errno::ENOENT, Errno::ENOTDIR + nil + end return result unless expanded == source_root || expanded.start_with?("\#{source_root}/") result.define_singleton_method(:uid) { Process.euid.zero? ? 1 : 0 } result - rescue Errno::ENOENT, Errno::ENOTDIR - result 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 `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb` around lines 296 - 312, In the File.stat shim’s singleton method, limit the Errno::ENOENT and Errno::ENOTDIR rescue to only the File.realpath(path.to_s) call. Ensure agent_workflows_original_stat continues propagating missing-path errors, while realpath failures still return the original stat result.skills/plan-pr-batch/SKILL.md (1)
606-607: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winFail closed on ad-hoc targets instead of recording an unqualified preflight bypass.
trusted-direct adhoc:=>skipis not enforced bypr-security-preflight; the helper does not parse ad-hoc inputs. The preflight rule inskills/pr-batch/SKILL.mdalready says do not passadhoc:targets to it and to record the trusted direct user instruction separately. Make the generated Goal prompt follow that rule, and reject ad-hoc lanes in this skill/workflow unless the direct instruction has trusted, authenticated host provenance. Add a negative test for an untrusted ad-hoc target.🤖 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 606 - 607, Update the generated Goal prompt in the lane-card/preflight guidance to exclude adhoc: targets from pr-security-preflight and record trusted-direct instructions separately. Add validation in the relevant plan/batch workflow to reject adhoc lanes unless the direct instruction has trusted, authenticated host provenance, and add a negative test covering an untrusted adhoc target.workflows/pr-processing.md (1)
2096-2100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep mirrored label removal claim-specific.
After
agent-coord release, a replacement can acquire the lane and reapplyagent-claimedbefore this cleanup removes it. The cleanup can then remove the replacement worker's label.Bind label removal to the released holder and generation. Do not remove the label unless it still represents the cancelled claim.
Based on learnings: verify that the lane still matches the claim holder and generation before removing the mirrored label.
🤖 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 `@workflows/pr-processing.md` around lines 2096 - 2100, Update the cancellation cleanup instructions around agent-coord release so mirrored agent-claimed label removal is conditional on the lane still matching the released claim’s holder and generation. Do not remove the label if a replacement has acquired the lane or re-applied the label; only remove it when it still represents the cancelled claim.Source: Learnings
skills/pr-batch/SKILL.md (1)
314-318: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the complete helper-resolution chain.
Line 315 states that the command resolves three locations. Lines 316-317 use only the environment variable and the repo-local fallback. A loaded installed skill can therefore contain
pr-security-preflightbut still fail to run it.Resolve
PR_BATCH_SKILL_DIRthrough the loaded skill base before.agents/skills/pr-batch, then stop with a blocker when no helper exists.Based on learnings: resolve
PR_BATCH_SKILL_DIRthrough environment, loaded skill base, then repo-local pinned copy, and stop.🤖 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/SKILL.md` around lines 314 - 318, Update the PR_BATCH_SKILL_DIR resolution instructions and invocation to check the explicit environment value, then the loaded skill base, then the repo-local pinned copy; verify that pr-security-preflight exists in the selected directory and stop with a blocker if no candidate provides it.Source: Learnings
🧹 Nitpick comments (2)
skills/pr-batch/bin/dispatcher-capability-preflight-test.rb (2)
2352-2437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared waived-active fixture for these two tests.
test_waived_active_replay_rejects_legacy_confirmation_without_its_durable_waiverandtest_waived_active_replay_rejects_pre_actual_host_confirmation_without_its_durable_waiverrepeat the same 20 lines of setup. They differ only in the confirmation flavor and the addedwrite_supported_readiness(root)call. Both assert the same status and reason.Extract a helper that returns the helper path, root, and activated waived assignment. Then drive both cases from one table.
♻️ Sketch of the shared fixture
def activated_waived_lane helper, root = installed_unsupported_dispatcher_helper route = { "model" => "gpt-5.6-sol", "effort" => "xhigh" } input = { "batch_id" => "batch-299", "expected_issue" => "shakacode/agent-workflows#299", "lane_id" => "aw299-implementation", "requested" => { "route" => route, "dispatcher" => "codex-collaboration", "hard_route" => true }, "candidates" => [{ "route" => route, "dispatcher" => "codex-collaboration", "binding" => "dispatcher-bound", "attestation" => "instance-bound", "instance_id" => "live-worker-299" }] } pending = dispatch(input, helper) waiver_path, waiver_record = bootstrap_waiver( root, batch_id: input.fetch("batch_id"), lane_id: input.fetch("lane_id"), route: ) waiver = launch_waiver(path: waiver_path, record: waiver_record, assignment: pending.fetch("dispatch")) activated = dispatch( input.merge("active_assignments" => pending.fetch("active_assignments"), "launch_waiver" => waiver), helper ) [helper, root, input, activated.fetch("active_assignments").fetch(0)] end def test_waived_active_replay_rejects_confirmations_without_their_durable_waiver helper, root, input, active_assignment = activated_waived_lane legacy = { "type" => "launch-confirmation", "version" => 1, "id" => "legacy-launch-confirmation", "assignment" => active_assignment } write_supported_readiness(root) historical = pre_actual_host_v2_confirmation(active_assignment) { "legacy v1" => legacy, "pre-actual_host v2" => historical }.each do |label, confirmation| replay = dispatch( input.merge("candidates" => [], "active_assignments" => [active_assignment], "launch_confirmation" => confirmation), helper ) assert_equal "invalid-input", replay.fetch("status"), label assert_equal "waived-active assignment requires its durable launch_waiver", replay.fetch("reason"), label end 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 `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb` around lines 2352 - 2437, Extract the duplicated waived-active setup from test_waived_active_replay_rejects_legacy_confirmation_without_its_durable_waiver and test_waived_active_replay_rejects_pre_actual_host_confirmation_without_its_durable_waiver into an activated_waived_lane helper returning helper, root, input, and active_assignment. Replace the two tests with one table-driven test covering the legacy and pre-actual_host confirmations, retaining write_supported_readiness(root) only for the latter case and applying the shared status and reason assertions to both.
415-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIgnore fixture directories created in the skill tree.
secure_mktmpdircreates@secure_fixture_rootunderskills/pr-batch/bin, andteardownonly purges it after the run completes. Add a gitignore rule for directories starting withdispatcher-preflight-fixturesin that tree so an interrupted or aborted test cannot leave tracked files exposed.🤖 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/dispatcher-capability-preflight-test.rb` around lines 415 - 425, The test creates temporary dispatcher-preflight-fixtures directories under the skill tree without ignoring them. Add a gitignore rule scoped to the skills/pr-batch/bin tree that matches directories beginning with dispatcher-preflight-fixtures, preserving normal tracking for other files.
🤖 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_doctor/signed_launch_installation_test.rb`:
- Around line 12-23: The test_platform_symlink_ancestor_is_canonicalized fixture
does not guarantee that the selected ancestor is a symlink, so it may bypass the
platform-symlink branch. Update the test to create and use a real symlink
ancestor, or skip it unless the selected ancestor is confirmed to be a symlink,
ensuring the assertion exercises canonicalization rather than direct_helper.
In `@docs/agent-workflows-model-routing.md`:
- Around line 308-310: Replace the duplicated signed-launch policy with one
canonical summary or reference, preserving RSA anchors of at least 2048 bits and
waiver observations bounded in both directions with a non-future grant. Update
docs/agent-workflows-model-routing.md lines 308-310, docs/pr-batch-skills.md
lines 181-182, skills/pr-batch/SKILL.md lines 98-99, and skills/triage/SKILL.md
line 259; keep the SKILL.md guidance concise and portable. Update
skills/pr-batch/bin/model-routing-contract-test.rb lines 117-168 to validate the
canonical rule and required references instead of large verbatim policy copies.
---
Outside diff comments:
In `@skills/plan-pr-batch/SKILL.md`:
- Around line 606-607: Update the generated Goal prompt in the
lane-card/preflight guidance to exclude adhoc: targets from
pr-security-preflight and record trusted-direct instructions separately. Add
validation in the relevant plan/batch workflow to reject adhoc lanes unless the
direct instruction has trusted, authenticated host provenance, and add a
negative test covering an untrusted adhoc target.
In `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb`:
- Around line 296-312: In the File.stat shim’s singleton method, limit the
Errno::ENOENT and Errno::ENOTDIR rescue to only the File.realpath(path.to_s)
call. Ensure agent_workflows_original_stat continues propagating missing-path
errors, while realpath failures still return the original stat result.
In `@skills/pr-batch/SKILL.md`:
- Around line 314-318: Update the PR_BATCH_SKILL_DIR resolution instructions and
invocation to check the explicit environment value, then the loaded skill base,
then the repo-local pinned copy; verify that pr-security-preflight exists in the
selected directory and stop with a blocker if no candidate provides it.
In `@workflows/pr-processing.md`:
- Around line 2096-2100: Update the cancellation cleanup instructions around
agent-coord release so mirrored agent-claimed label removal is conditional on
the lane still matching the released claim’s holder and generation. Do not
remove the label if a replacement has acquired the lane or re-applied the label;
only remove it when it still represents the cancelled claim.
---
Nitpick comments:
In `@skills/pr-batch/bin/dispatcher-capability-preflight-test.rb`:
- Around line 2352-2437: Extract the duplicated waived-active setup from
test_waived_active_replay_rejects_legacy_confirmation_without_its_durable_waiver
and
test_waived_active_replay_rejects_pre_actual_host_confirmation_without_its_durable_waiver
into an activated_waived_lane helper returning helper, root, input, and
active_assignment. Replace the two tests with one table-driven test covering the
legacy and pre-actual_host confirmations, retaining
write_supported_readiness(root) only for the latter case and applying the shared
status and reason assertions to both.
- Around line 415-425: The test creates temporary dispatcher-preflight-fixtures
directories under the skill tree without ignoring them. Add a gitignore rule
scoped to the skills/pr-batch/bin tree that matches directories beginning with
dispatcher-preflight-fixtures, preserving normal tracking for other files.
🪄 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: 0fcfcf7d-5315-4e76-b939-00519dabde2e
📒 Files selected for processing (17)
CHANGELOG.mdCONTEXT.mdbin/agent_doctor/signed_launch_installation.rbbin/agent_doctor/signed_launch_installation_test.rbbin/agent_doctor/signed_launch_waiver.rbbin/agent_doctor/signed_launch_waiver_record.rbdocs/agent-workflows-model-routing.mddocs/installation-and-upgrades.mddocs/pr-batch-skills.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/bin/batch-plan-preflight-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/dispatcher-capability-preflightskills/pr-batch/bin/dispatcher-capability-preflight-test.rbskills/pr-batch/bin/model-routing-contract-test.rbskills/triage/SKILL.mdworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (6)
- CONTEXT.md
- docs/installation-and-upgrades.md
- bin/agent_doctor/signed_launch_waiver.rb
- bin/agent_doctor/signed_launch_waiver_record.rb
- bin/agent_doctor/signed_launch_installation.rb
- skills/plan-pr-batch/bin/batch-plan-preflight-test.rb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55faa816e
ℹ️ 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".
Review: Report unsupported signed-launch readiness (#306)OverviewThis PR adds a new Code quality
Minor nit
Maintainability observation (non-blocking)The long "Accepted binding evidence is..." / "Signed-launch readiness is separate from install freshness..." contract paragraphs are duplicated near-verbatim across 6 files ( SecurityNo blocking issues found. The trust model is consistently local-filesystem-ownership-based (root or a single validated owner uid, no group/world write), applied uniformly to directories, files, and their entire ancestor chains, with explicit tests for symlink/ownership/permission bypass attempts. The waiver path is clearly scoped (exact batch/lane/dispatcher/model/effort/route binding, non- SizeThis is a large, dense PR (25 files, ~3800 additions) implementing a security-critical trust boundary. Per the PR description, independent final-head QA/review is still pending — given the density of the fail-closed logic here, I'd recommend that pass pay particular attention to the two points flagged inline above. |
Address-review summaryScan scope: full history via check all reviews Mattered
Optional
Skipped
Validation: focused dispatcher, batch-plan, installation, routing, installer, and status suites passed; local bin/validate passed; independent Codex review found no remaining actionable defect; exact-head hosted Validate, Lint, Claude review, and CodeRabbit checks passed. All review threads are resolved. Next default scan starts after this comment. Say check all reviews to rescan the full PR. |
|
Closing without merge by maintainer product decision. The implementation is no longer the desired architecture. Agent Workflows is a portable workflow source pack and should not create a project-level signing, trust-anchor, readiness, or waiver system for runtime facts owned by Codex or Claude. Model and effort routes will be advisory; unavailable preferences will not block ordinary work. Superseding ownership:
This closure is a change in product direction, not a claim that the final #306 code was defective. The branch is intentionally preserved for historical review; none of its waiver machinery should be copied into the replacement implementation. |
1 participant
Closes #299.
Summary
supported, clean-hostunsupported, or fail-closedUNKNOWN.agentsdirectory during install without creating keys, trust anchors, capability claims, signatures, or waiverswaived-active, keep them distinct from signedconfirmed-active, and enforce lifecycle/replay constraintsTrust boundary
Private signing keys remain host-owned. Caller or environment input cannot select trust anchors, and partial, malformed, unsafe, or ambiguous host evidence remains
UNKNOWNand cannot be waived. The waiver path preserves all security, dependency, validation, review, CI, merge-eligibility, exact-head, and completed-batch-audit gates.Validation
bin/validate— PASS on the exact staged tree (exit 0; 115 files inspected by RuboCop, no offenses)The authoritative local full gate used macOS
/bin/bashwithBASH_ENVandENVunset because the ambient Homebrew Bash 5.3 environment reproducibly deadlocked existing heredoc fixtures. No source workaround for that environment issue is included.Batch state
Implementation lane is complete at
6f39f4225793646071aff2e25f10c3829c20b103. Independent final-head QA and review are pending and remain required before merge.Summary by CodeRabbit
New Features
Bug Fixes
Documentation