Fail closed on malformed trust role values

[justin808]

Why

Refs #202. This PR retains contract 2 only: malformed bot trust-role values must fail closed before normalization. Contracts 3-6 remain open.

PR #799 moved the security-preflight trust parser into GithubActorTrust, so the original draft no longer applied cleanly. This revision reconciles the old branch with current main and keeps only the required malformed-role behavior.

What changed

• Validate trusted_bots and trusted_metadata_bots as a legacy nonblank scalar string or an array of nonblank strings before normalization.
• Apply the same rule at the current actor-trust, seam-doctor, downstream-sync, security-preflight, and stale-assignment-sweep boundaries.
• Preserve scalar compatibility, login normalization, packaged metadata bots, overlap checks, comment-attribution validation, and per-repository downstream failure handling.
• Reject malformed direct CLI input with a concise failure instead of an exception backtrace.
• Preserve duplicate-key detection and invalid-Unicode diagnostics across supported JSON gem versions.
• Restore the current-main closeout contract budget with a bounded 4,641-byte buffer pending the existing extraction work.
• Trigger the read-only downstream fleet audit when its new actor-trust runtime dependency changes.

How to review and verify

1. Review GithubActorTrust.strict_string_list and the standalone seam-doctor equivalent.
2. Check that malformed hashes, numbers, mixed arrays, empty entries, and whitespace-only entries fail before normalization.
3. Confirm the downstream CLI and fleet paths fail gracefully without widening validation to trusted_users or trusted_teams.

Test plan

• Historical current-main reproduction accepts trusted_bots: 42 (red evidence).
• ruby skills/pr-batch/bin/github-actor-trust-test.rb — 35 runs, 83 assertions.
• ruby skills/pr-batch/bin/pr-security-preflight-test.rb — 346 runs, 4,107 assertions.
• ruby bin/agent-workflow-seam-doctor-test.rb — 343 runs, 2,780 assertions.
• ruby skills/pr-batch/bin/stale-assignment-sweep-test.rb — 51 runs, 346 assertions.
• ruby skills/pr-batch/bin/target-membership-guard-test.rb — 38 runs, 722 assertions under JSON 2.6.3, 2.21.2, and 3.0.2.
• JSON compatibility probe — 210 cases across all 21 installed JSON versions, zero failures.
• ruby skills/pr-batch/bin/integration-closeout-contract-test.rb — 19 runs, 749 assertions, one unchanged skip.
• Updated ruby bin/push-downstream-test.rb — 163 runs, 1,045 assertions, including the workflow dependency-path contract.
• Trusted secure GitHub Actions scan — seven workflows, zero findings.
• RuboCop on all changed Ruby files — no offenses.
• git diff --check origin/main...HEAD — clean.
• bin/validate — full clean-checkout validation passed, including revision-sensitive installer and stack suites.
• Current-head hosted Validate — pending after push.

Agent details

Exact-head and replay evidence

• Base: b58669affc2da35c728fae80e659ea0db278abf2
• Candidate: 3f31e19023ffef7003b8ae357155dab88660bfbe
• Historical draft: 71566df0b1776a096d931ab7157a136ae89cecdc
• Prior adapted candidate: febda825ea6f2bf5f3f2ca4bfb20cdf544a57a00
• Merged Enforce actor trust at the review-data ingestion boundary #799: 629e35d83745af14f385a7006ab818dab6c2a266
• Changelog classification: deferred_to_update_changelog

Coordination and reviewer telemetry

• One serialized controller updated the existing PR branch; the historical issue Follow-up: Fail closed on six reconciled workflow-pack contracts #202 claim was already released.
• Independent exact-head review covered all fourteen changed paths and found no blocking, discuss, or optional code findings after the malformed-JSON diagnostic regression and downstream audit-trigger omission were fixed and re-reviewed.
• The current-head Claude review found no correctness or security bugs. Its one code-quality-only nit was declined with rationale and resolved.

Decision log

• Non-blocking: Keep seam-doctor validation local.
  • Decision: Match the shared schema without requiring the skill pack.
  • Why: Seam doctor remains installable and runnable in partial-install contexts.
  • Review later: None.
• Non-blocking: Keep trust-error path prefixing at the current caller.
  • Decision: Preserve the tested public error contract without broadening parser ownership in this scoped PR.
  • Why: The reviewer confirmed there is no live bug.
  • Review later: None; no follow-up issue for a style-only suggestion.

Merge confidence

The scoped implementation, maintainer-authorized current-main baseline repairs, independent review, and full local validation are clean at the exact candidate head. Merge authority is auto_merge_when_gates_pass; hosted current-head gates remain authoritative.

---

Note

Medium Risk
Touches trust allowlist parsing at several security-sensitive seams; changes are defensive fail-closed validation with broad test coverage, but misconfigured repos will start failing earlier.

Overview
Malformed trusted_bots / trusted_metadata_bots values now fail closed before login normalization, so hashes, numbers, mixed-type arrays, empty strings, and whitespace-only entries cannot slip into allowlists or vanish during coercion.

GithubActorTrust.strict_string_list is the shared rule (legacy scalar strings still work). The same check is wired through push-downstream contract resolution, pr-security-preflight, stale-assignment-sweep, and agent-workflow-seam-doctor (local strict_trust_role_list so the doctor stays usable without the skill pack). Downstream fleet/registry behavior stays per-repo: one bad consumer still fails that repo with a clear message. The push-downstream CLI rescues RuntimeError so bad --trusted-* flags print FAIL: … without a backtrace. Preflight trust errors keep a consistent Invalid trust config <path>: prefix.

Behavior shift: blank/null bot list entries that used to be ignored during preflight now abort at config load with the same shape error.

^{Reviewed by Cursor Bugbot for commit febda82. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: dd2b9a4d-cfd0-43df-b3de-71a98af23c5e

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_06959dbb-f1ad-4dea-902b-d18ec875c915)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-14T06:24:32.205985Z	3f31e19	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Reviewed. This is a well-scoped fix: Kernel#Array() on a malformed YAML value (e.g. a Hash like {deploy: true}) silently produces something like [["deploy", true]] rather than raising, which could previously let a misconfigured trusted_bots/trusted_metadata_bots entry pass through normalization instead of failing closed — a real security-relevant gap for a trust-role allowlist. The fix addresses it directly and consistently:

• GithubActorTrust.strict_string_list is a genuine shared validator with 4 real call sites (build_config ×2, push-downstream#normalize_trust_config, stale-assignment-sweep#load_trust_config), not a speculative abstraction.
• The near-duplicate strict_trust_role_list in bin/agent-workflow-seam-doctor is justified and commented — that script is intentionally standalone-installable without the skill pack, consistent with how it already avoids other skill-pack dependencies.
• Error propagation is wired correctly end-to-end: normalize_trust_config's rescue GithubActorTrust::Error => e; raise e.message.to_s re-raises as the plain RuntimeError string convention this codebase already uses, and I traced it through resolve_contract → run_registry's per-repo rescue RuntimeError → the exact "FAIL shakacode/bad: trusted_metadata_bots must be..." message asserted in the updated tests.
• The large diff in bin/push-downstream's CLI dispatch is mostly re-indentation to wrap the existing option-chain in begin/rescue RuntimeError, which is needed because default_local_contract (and thus normalize_trust_config) was previously called outside the local reconcile_scaffold rescue in run_local — so a malformed --trusted-bot flag would have crashed with a raw Ruby backtrace before this change. The new test_empty_trusted_bot_flag_fails_without_a_backtrace test confirms the gap this closes.
• Scope matches the stated "Why" (contract 2 of Follow-up: Fail closed on six reconciled workflow-pack contracts #202 only — bot-role shapes); trusted_users/trusted_teams shapes are correctly left untouched for later contracts.
• Test additions are proportionate: one regression test per seam that was wired to the validator, plus the shared-validator unit tests in github-actor-trust-test.rb.

No bugs or scope issues found.

[claude]

Review

Went through the diff focused on correctness, security, and scope.

Scope: Matches the stated goal precisely — fail-closed validation of trusted_bots/trusted_metadata_bots only, applied at exactly the five boundaries named in the description (GithubActorTrust, seam-doctor, push-downstream, pr-security-preflight, stale-assignment-sweep). trusted_users/trusted_teams are correctly left untouched, as called out.

Duplication check: bin/agent-workflow-seam-doctor reimplements strict_trust_role_list instead of calling GithubActorTrust.strict_string_list. Confirmed this is necessary rather than copy-paste: the seam-doctor binstub is explicitly designed to run standalone without the skills/pr-batch skill pack installed (it checks for/loads only its own companion files), so it can't require_relative the shared lib. The comment above the method documents this, so it's justified rather than a scope violation.

Correctness spot checks:

• push-downstream's new top-level begin/rescue RuntimeError in the CLI entrypoint is not redundant with the per-branch rescues in run_local/run_registry/etc. — default_local_contract (and the CLI's own trust_config_empty? pre-checks) can raise before those inner rescues are reached, so the outer rescue is the only thing that catches a malformed --trusted-bot flag. Verified this against the new test_empty_trusted_bot_flag_fails_without_a_backtrace test.
• GithubActorTrust.strict_string_list's scalar/array/else branches correctly preserve legacy scalar-string compatibility while rejecting hashes, numbers, mixed arrays, and blank/whitespace-only strings before normalization.
• The behavior shift called out in the description (previously-ignored blank bot entries now hard-fail at config load) is real and intentional, and the affected test was updated and renamed to document it rather than silently changed.

No bugs, security issues, or out-of-scope additions found. Test additions are proportionate to the five call sites touched.

[claude]

Reviewed the diff. This is a well-scoped, security-positive change: GithubActorTrust.strict_string_list is a single shared validator reused at 3+ call sites (shared lib, push-downstream, stale-assignment-sweep), with a documented, deliberate local duplicate in agent-workflow-seam-doctor (justified by that script's standalone-install constraint, per the PR's decision log). Test coverage per seam matches the stated contract (malformed hashes/numbers/mixed arrays/blank strings fail closed; legacy scalar strings still work), and the scope stays correctly limited to trusted_bots/trusted_metadata_bots without creeping into trusted_users/trusted_teams.

No correctness or security bugs found. Left one inline nit on pr-security-preflight's load_trust_config — a fragile start_with? prefix-matching workaround that exists because the new validator's errors don't carry the "Invalid trust config <path>:" prefix the way other GithubActorTrust::Errors raised in build_config already do. Not a live bug (paths match exactly today), just a design inconsistency worth tightening at the source.

The large diff in bin/push-downstream around the CLI dispatch block is mostly re-indentation from wrapping the existing if/elsif chain in begin/rescue RuntimeError; I checked and didn't find a smaller way to achieve the same backtrace-free CLI failure without a similar reindent, so that's fine as-is.

[justin808]

🤖 Codex

Review follow-up complete

All current review feedback has a recorded outcome. No code change was required.

Next scan: Start after this comment. Say check all reviews to rescan the full PR.

Agent details

Posting runtime: Codex · Astra

Scan scope: Full history; no prior address-review summary existed.

Findings that mattered

None. The current-head Claude review reported no correctness or security bugs.

Optional suggestions

• Trust-error prefix ownership — declined with [auto-deferred]: the current implementation and tests preserve the required stable prefix, while moving path ownership would broaden this PR's contract.

Skipped items

• The current-head Claude summary was informational and required no action.

Deferred-work tracking: No issue created; repository policy does not track unreproduced style-only suggestions.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdbc26db16

ℹ️ 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]

Reviewed the diff (b58669a...4cd5264). Summary:

Core logic is sound. GithubActorTrust.strict_string_list and the local agent-workflow-seam-doctor equivalent (strict_trust_role_list) are behaviorally equivalent across all the cases I traced (nil, empty array, blank/whitespace string, non-string scalar, hash, mixed array) — the duplication is already called out and justified in the PR's decision log (seam-doctor needs to stay usable without the skill pack), so I'm not re-raising it.

Error boundaries check out. push-downstream's normalize_trust_config converts GithubActorTrust::Error to a RuntimeError via raise e.message.to_s, and the new top-level rescue RuntimeError around the CLI dispatch is consistent with this file's existing convention of using RuntimeError as the "expected/handled failure" signal throughout (it's already rescued this way in run_local, run_registry, run_policy_fleet, etc.). It's needed because default_local_contract → normalize_trust_config can now raise before run_local's own inner begin/rescue starts. Traced the call paths (resolve_contract → merge_contract! → merge_trust_configs → normalize_trust_config) and the per-repo FAIL <nwo>: ... attribution still works correctly, confirmed by test_resolve_contract_rejects_malformed_bot_role_values.

Test coverage matches the stated "behavior shift." The blank-bot-entry test in pr-security-preflight-test.rb was correctly updated (not left stale) to assert the new fail-closed behavior instead of the old silent-ignore behavior.

One scope note (not a blocker): the target-membership-guard changes (allow_duplicate_key: true on JSON.parse, and the surrogate-error regex classification fix) are unrelated to trust-role validation — they're JSON-gem-version compatibility fixes for pre-existing duplicate-key/invalid-Unicode diagnostics, called out explicitly in the PR description as collateral from testing across 21 JSON gem versions. Given they're narrow, well-tested (test_malformed_ascii_json_that_mentions_surrogate_keeps_the_json_diagnostic), and not introducing new abstractions, I don't think this needs to be split out, just flagging why it's in this diff for reviewers who weren't expecting JSON-parsing changes in a "trust role fail-closed" PR.

No correctness, security, or performance issues found. No inline comments to add.

[claude]

Review summary

Reviewed the diff focused on correctness, security, and scope. This is a well-targeted fail-closed fix: GithubActorTrust.strict_string_list (and its duplicated-but-justified agent-workflow-seam-doctor counterpart) correctly closes the gap where Array(42)/Array({...}) previously silently coerced malformed trusted_bots/trusted_metadata_bots values into bogus allowlist strings instead of raising. I traced the fix through all five call sites (GithubActorTrust#build_config, push-downstream#normalize_trust_config, stale-assignment-sweep, pr-security-preflight's rescue wrapper, and the standalone seam-doctor copy) and the error-propagation/prefixing (GithubActorTrust::Error → RuntimeError in push-downstream, path-prefix patching in pr-security-preflight) is consistent with each file's existing conventions — no double-prefixing or swallowed backtraces found. fetch-pr-review-data also inherits the fix automatically via the shared load/build_config path, even though it isn't listed in the PR description.

No correctness or security bugs found.

One scope note posted inline: the target-membership-guard JSON allow_duplicate_key/surrogate-pair diagnostic change is a distinct JSON-gem-compatibility fix unrelated to the bot trust-role fail-closed contract that is this PR's stated "Why." It looks correct and is tested, but bundling it here (along with the closeout-contract byte-budget bump) makes the diff cover two unrelated concerns — worth a separate commit/PR if it isn't strictly required to unblock this change.

Test coverage (malformed hash/int/mixed-array/blank-string cases, legacy scalar compatibility, per-repo fleet continuation) looks proportionate to the change.

[justin808]

🤖 Codex

Review follow-up complete

The current-head review found no correctness or security defects. The one integration finding was fixed and both review threads now have recorded outcomes, so the next routine check can start after this comment.

Agent details

Posting runtime: Codex · UNKNOWN

Scan scope: Review activity after the previous summary at 2026-09-14T03:43:15Z.

Findings that mattered

• Handled: added skills/pr-batch/lib/github_actor_trust.rb to the downstream audit workflow trigger and its exact-path contract in 3f31e190; focused tests, full validation, and the trusted Actions scan pass.
• Declined: splitting the maintainer-authorized current-main JSON and closeout baseline repairs into another PR. They are narrow, separately committed, independently reviewed, and required for the unwaived validation gate.

Skipped items

• Reviewer status/summary comments reported no additional correctness, security, or performance findings.
• The current review summary's scope note duplicated the resolved inline scope discussion.

Next scan: Start after this comment. Say check all reviews to rescan the full PR.