Harden untrusted contributor intake into an executable preflight

[justin808]

Fixes #168

Why

skills/untrusted-contributor-intake/SKILL.md carried roughly 250 lines of POSIX
shell that agents were expected to transcribe verbatim: a trusted-origin
producer, a PR_REF classifier, a metadata resolver, a canonical-authority
parser, and a canonical PR URL parser. Four of those blocks each contained their
own copy of the same host / port / DNS-label rules, and three contained their own
copy of the owner/repo rules. Duplicated security rules drift, and a
transcription slip in a prose snippet silently weakens a boundary rather than
failing a test.

This PR turns that prose into one executable, tested helper and leaves SKILL.md
to state policy, host enforcement, reporting, and disposition guidance.

What changed

• New helper skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight.
  Stdlib-only Ruby, executable, runnable from a trusted base checkout. It
  validates the trusted origin policy, classifies PR_REF, performs exactly one
  gh pr view --json number,url lookup pinned to the trusted host and
  repository, validates the server-returned canonical URL, and prints nine
  KEY=value lines. Any failure prints one BLOCKED: ... line to stderr and
  exits 1.
• Deduplicated validation. normalize_authority (host, optional port,
  DNS labels, :443-only stripping for https), normalize_repository, and
  parse_pull_request_url each exist once. The trusted origin, the PR_REF URL,
  and the canonical URL all flow through the same code, and the helper test
  asserts both the call-site counts and behavioral parity across all three
  inputs.
• SKILL.md binds to the helper. The five shell blocks collapse to one
  documented # Intake preflight: snippet that resolves
  UNTRUSTED_CONTRIBUTOR_INTAKE_SKILL_DIR, runs the helper, and consumes its
  output through an allow-listed case (no eval), then asserts key count,
  scheme, and host/repo agreement.
• Metadata-only boundary preserved and made testable. The helper never reads
  PR bodies, issue text, comments, review text, or fork content; a test asserts
  its code contains no body / comments / reviews / graphql / pr diff /
  checkout / backtick / system( / exec( references and exactly one
  Open3.capture3. pr-security-preflight is still explicitly not reused.
• Documented host-level enforcement. A new Host Boundary list enumerates the
  expected read-only, no-execution, no-secrets, no-writes, named-override, and
  "report BLOCKED if unenforceable" expectations.
• Marker uniqueness. Each of the three bash blocks now opens with a unique
  marker (# Intake preflight:, # Metadata gathering:, # Actor authority:),
  and a contract test asserts each appears exactly once and opens a block. The
  previous metadata-gathering extractor keyed off a line that appears twice in
  the document.
• Tests. New untrusted-contributor-intake-preflight-test.rb (24 tests, 823
  assertions) carries the Add safe untrusted contributor intake skill #158 behavior matrix against the real helper. The
  contract test keeps its markdown-extraction tests and gains exact
  snippet/helper binding tests; its end-to-end tests now execute the documented
  snippet against the real helper with a stubbed gh. bin/validate gained one
  appended line for the new test.

Decision log

1. Helper location diverges from the issue's illustrative path. The issue
   says "e.g. bin/untrusted-contributor-intake-preflight". It lives at
   skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight
   instead, per AGENTS.md → Editing Rules: "Keep helper scripts in the skill
   folder that invokes them, unless the helper is repo-wide like
   bin/agent-workflow-seam-doctor." Only this skill invokes it, and that folder
   already holds untrusted-contributor-intake-contract-test.rb.
2. Policy still arrives as three explicit env inputs; the helper reads no
   files. It would have been convenient to have the helper read
   .agents/agent-workflow.yml itself, but that reintroduces exactly the
   checkout-derived trust the Add safe untrusted contributor intake skill #158 design removed (a fork or repointed checkout
   could supply the file). The seam contract is unchanged: the invoker reads the
   three untrusted_contributor_intake.* keys from the trusted base and exports
   them.
3. Where the same input now fails, it can fail earlier and under a different
   BLOCKED message. With one code path instead of five snippets:
   • a malformed TRUSTED_GH_REPO now reports trusted origin is invalid
     (previously metadata resolution is invalid, because the repo check that
     fired depended on which snippet the caller had pasted);
   • for URL input, a server number that disagrees with the raw URL number now
     stops at metadata resolution is invalid instead of reaching the canonical
     parser.
     Both are strictly earlier fail-closed stops on the same rejected inputs; the
     five documented BLOCKED message strings are otherwise unchanged.
4. The delimiter record (--jq '"\(.number)|\(.url)"') was kept rather than
   switching to JSON parsing, so the Add safe untrusted contributor intake skill #158 malformed-record rejection vectors port
   over one-for-one and the documented gh call shape is unchanged.
5. No eval. The snippet consumes helper output through an explicit
   allow-listed case and a key count, preserving the skill's existing "no eval,
   no standalone jq" posture.
6. CHANGELOG.md / README.md untouched. Both are outside this lane's owned
   paths. The skill is already listed in both from Add safe untrusted contributor intake skill #158, and this PR adds no new
   skill. Flagging for the coordinator in case a changelog line is wanted.

Validation

All commands run from the branch head in a clean worktree.

$ ruby skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight-test.rb
24 runs, 823 assertions, 0 failures, 0 errors, 0 skips

$ ruby skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-contract-test.rb
49 runs, 750 assertions, 0 failures, 0 errors, 0 skips

$ rubocop _1.87.0_ skills/untrusted-contributor-intake/bin/
3 files inspected, no offenses detected

$ LANG=en_US.UTF-8 bin/validate
...
== rubocop 1.87.0 ==
Inspecting 119 files
.......................................................................................................................

119 files inspected, no offenses detected
PASS agent-workflows validation
# exit code 0; both intake suites ran inside it
# (contract 49 runs / 750 assertions, preflight 24 runs / 823 assertions, 0 failures)

$ ruby skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-contract-test.rb   # mutation check
# with `blocked(CANONICAL_UNTRUSTED) unless gh_host == policy.fetch(:host)` disabled in the helper:
49 runs, 744 assertions, 2 failures, 0 errors, 0 skips
# reverted; the documented snippet is genuinely bound to the real helper, not to a stub.

Note on bin/validate: with LANG unset (the default in a non-interactive agent
shell) bin/push-downstream-test.rb errors with
Encoding::CompatibilityError: incompatible character encodings: US-ASCII and UTF-8
at test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes
(line 1576). This is the pre-existing locale dependency tracked in #281,
reproducible on clean base 282a87a; the test is untouched by this PR and passes
under LANG=en_US.UTF-8, which is why every run above sets it.

Confidence note: high on the helper and its behavior coverage — the #158
rejection matrix is ported vector-for-vector, exercised against the real
executable, and a deliberate mutation of the canonical-host check fails the
suite. Medium on prose-assertion churn: SKILL.md wording moved, so several
contract-test string assertions were rewritten to match the new text; those
assertions verify documentation, not behavior. The helper has not been exercised
against a live GitHub or GitHub Enterprise host in this branch — every gh
interaction in the tests is stubbed.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 68164fc0-42ad-4ac2-9e48-7bd528aaa5da

📥 Commits

Reviewing files that changed from the base of the PR and between 282a87a and 1ff7828.

📒 Files selected for processing (5)

• bin/validate
• skills/untrusted-contributor-intake/SKILL.md
• skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-contract-test.rb
• skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight
• skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight-test.rb

---

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.}

[claude]

Review: Harden untrusted contributor intake into an executable preflight

Overall this is a strong, well-executed refactor. It replaces roughly 250 lines of duplicated POSIX shell (repeated host/port/DNS-label and owner/repo validation logic scattered across five separate snippets) with a single, tested, stdlib-only Ruby helper (bin/untrusted-contributor-intake-preflight), and rewires SKILL.md to invoke it instead of transcribing the logic in prose. I traced the new helper's logic against the removed bash line-by-line and it is a faithful (and in several places stricter) port; no security regression found.

What I verified

• Shared validators are actually shared and correct. normalize_authority (host/port/DNS-label rules), normalize_repository, and parse_pull_request_url are each defined once and reused for the trusted-origin policy, the raw PR_REF URL, and the server-returned canonical URL, matching the PR's stated goal of eliminating drift between the three previously-duplicated copies.
• Fail-closed ordering preserved. Trusted policy is validated before any network call; for URL-form PR_REF, authority/repo are checked against the trusted policy before the one gh pr view call; the canonical URL returned by gh is validated against both the trusted host and the classified/server PR number afterward. No path reads PR body/comments/review text, confirmed both by reading the helper and by the test_requests_only_number_and_url_metadata contract test that checks for absence of body, comments, graphql, backtick, system(, etc.
• No shell-injection surface. Open3.capture3 is called with an explicit argv array (not a shell string), so the PR number / repo / host values passed to gh can't be interpreted by a shell even though they're also independently regex-validated.
• Env scrubbing is correct. The Open3.capture3 call unsets GH_REPO and pins GH_HOST for the child process exactly like the old env -u GH_REPO GH_HOST=... gh ..., and this is exercised by test_ambient_intake_state_cannot_authorize_or_redirect_a_run.
• Edge cases match the old bash behavior: default-port (443) stripping only for https, port length/range bounds (1-65535, <=5 digits), DNS label length (<=63) and leading/trailing-hyphen rejection, reserved dot/dot-dot repo segments, case-insensitive repo matching with case-sensitive "pull" keyword, rejection of extra path segments/query/fragment/control characters/embedded newlines. I spot-checked several of these by hand against parse_pull_request_url/normalize_authority and they line up with both the old removed shell and the new preflight-test.rb cases.
• SKILL.md's consumption of the helper's output is injection-safe: it parses KEY=value lines via an explicit allow-listed case statement (no eval), and independently re-validates scheme/host/repo agreement before trusting the values, which is good defense-in-depth given a shell script is consuming external process output.
• Test coverage is genuinely comprehensive for a security-critical helper: numeric/URL classification, ambient-env-can't-authorize, malformed/missing trusted policy, malformed canonical URLs (IPv6 literals, userinfo, double slashes, dot-dot segments, percent-encoded separators, oversized ports/labels), gh failure and gh-not-on-PATH, and a mutation test noted in the PR description that fails when the canonical-host check is disabled.

Minor observations (non-blocking)

• Left one inline comment: gh's stderr is captured but discarded in gh_pr_view_number_and_url, so all gh pr view failures (auth, rate limit, network, wrong repo) collapse into the same generic BLOCKED: metadata resolution is invalid. Likely an intentional "don't leak raw CLI output" choice, but worth a conscious call since it reduces operator debuggability of legitimate failures.
• .agents/skills/untrusted-contributor-intake as the default UNTRUSTED_CONTRIBUTOR_INTAKE_SKILL_DIR differs from the repo's skills/untrusted-contributor-intake dev path. Confirmed this is an existing, consistent convention used by every other skill in this repo (pr-batch, task-observer, address-review, etc.), so not an issue specific to this PR.
• The decision log flags that CHANGELOG.md/README.md are intentionally untouched since the skill was already listed from Add safe untrusted contributor intake skill #158. Seems reasonable, deferring to the coordinator as the author suggests.

Risk/Security

No injection, TOCTOU, or fail-open paths found. The helper's own network access is limited to exactly one pinned, metadata-only gh pr view --json number,url call, matching the stated boundary, and this is enforced structurally (single Open3.capture3 call site, asserted by test) rather than just by convention.

Nice work; this is exactly the kind of change (prose to tested executable) that reduces the risk of a future transcription slip silently weakening a security boundary.

[justin808]

QA Evidence — independent Batch QA Lane (checker-aw-f)

Independent check by an agent that did not write this code. Everything below was re-run from a detached checkout of 1ff7828649af81d011c81d56bf5a87344ee7710b; maker claims were not taken at face value.

Automated checks

Command	Result
LANG=en_US.UTF-8 bin/validate	exit 0 — PASS agent-workflows validation; rubocop 1.87.0, 119 files, no offenses; zero failures/errors anywhere in the log
ruby .../untrusted-contributor-intake-preflight-test.rb	24 runs, 823 assertions, 0 failures, 0 errors, 0 skips
ruby .../untrusted-contributor-intake-contract-test.rb	49 runs, 750 assertions, 0 failures, 0 errors, 0 skips
main @ 282a87a baseline (contract test)	61 runs, 1180 assertions — recorded before checkout to compare the split

Neither known environment issue interfered: the push-downstream-test.rb locale error (#281) did not occur under LANG=en_US.UTF-8, and the agent-coord-bounded-test.rb flake (#338) did not reproduce on this head (also run 2x green on clean main).

Security boundary — metadata-only (read line by line)

skills/untrusted-contributor-intake/bin/untrusted-contributor-intake-preflight is confirmed metadata-only:

• One network call, gh pr view <number> --repo <repo> --json number,url (line 216-229). No PR body, comments, reviews, diff, or fork content is ever requested.
• No execution of untrusted content: grep for eval|system(|%x|exec(|popen|Kernel|.send|require_relative|File.|Dir.|checkout|clone|worktree returns only two prose comment hits. No git invocation at all.
• No shell: Open3.capture3 is used in array form, so no shell is spawned and no interpolation of attacker-controlled strings occurs. The PR number reaching gh is always \A[0-9]+\z.
• require_printable (line 85) gates every externally supplied value to bytes 0x20..0x7E before any regex, so invalid-UTF-8 input fails closed rather than raising — the failure class behind bin/push-downstream-test.rb is locale-dependent and aborts bin/validate when LANG is unset #281 cannot occur on this path.

Coverage split — no rejection vector lost

The contract test went 61/1180 → 49/750 with a new 24/823 preflight test. I did not accept the net assertion increase as proof. I extracted all 110 URL-shaped attack vectors from the main contract test and diffed them against the union of both post-change files, then empirically probed every class that did not survive textually. All still fail closed:

Dropped vector class	Live behavior
trusted host containing ?, #, newline, tab, non-ASCII	BLOCKED, 0 network calls
canonical host containing ?, CR injection, non-ASCII	BLOCKED after exactly 1 call
ftp://, git@host:owner/repo.git, https://user@/…	BLOCKED, 0 calls
negative / leading-zero PR numbers	BLOCKED

The .git-remote vectors from main are architecturally obsolete, not lost: SKILL.md now sources trust only from trusted-base .agents/agent-workflow.yml and explicitly forbids deriving it from a checkout remote, and test_ambient_intake_state_cannot_authorize_or_redirect_a_run covers the replacement.

One real (non-blocking) coverage gap: the trusted-host rejection list at untrusted-contributor-intake-preflight-test.rb:237-255 covers / ] [ @ and space but drops the ?, #, and embedded-control-character vectors that main exercised. Behavior is correct (verified above by direct probe) — this is lost test coverage, not a live defect.

Mutation check (independently performed)

Commented out the canonical-host trust check (blocked(CANONICAL_UNTRUSTED) unless gh_host == policy.fetch(:host)) and re-ran the contract test: it went red — 2 failures, test_canonical_authority_must_match_the_trusted_host_policy (line 741) and test_requires_fresh_atomic_trusted_policy_inputs_in_a_persistent_shell (line 577). Restored; git diff against the head is empty. SKILL.md is bound to the real executable — run_documented_preflight runs the documented snippet against UNTRUSTED_CONTRIBUTOR_INTAKE_SKILL_DIR, not a stub or a transcribed copy.

Envelope and acceptance criteria

5 files, no deletions or renames, all within the lane's envelope. bin/validate receives exactly one appended line (+1/-0) registering the new test directly, satisfying #168's acceptance criteria. The helper is executable and runs from a trusted base checkout.

[justin808]

Batch handoff — AW F 08-05 08:40 (lane intake)

Final state: ready-human-review-required. All gates are clean; the only remaining blocker is a maintainer merge decision.

Gate	Result at head 1ff7828649af81d011c81d56bf5a87344ee7710b
pr-ci-readiness v2	READY (exact head)
autonomous-merge-eligibility	human-approval-required — triggered: architectural-product-judgment, changed-lines-limit (2819 > 999), public-compatibility, security-auth-privacy
merge-assurance	eligible: false, verdict: BLOCKED — "auto merge authority lacks a qualifying exact-head autonomous verdict"
Evidence failures	none (clean policy result, not UNKNOWN)
Rollback	code-only-rollback-established

merge_authority for this batch was auto_merge_when_gates_pass. That authority does not apply here: the repo's own autonomous_merge policy in .agents/agent-workflow.yml triggers the gates above, so the batch terminates at human review rather than merging. Separately, the seam sets merge_submission: mode: merge_queue_only and no merge queue is enabled on main, so pr-merge-submit would refuse before mutation regardless.

QA: independent Batch QA lane (checker-aw-f, distinct from every maker) — qa-evidence v2 marker on this PR, status: satisfied, release_blocking: clear, bound to head 1ff7828649af81d011c81d56bf5a87344ee7710b.

⚠️ Merge-order dependency: this PR and #343 both append to bin/validate in overlapping context. They are independently mergeable, but whichever merges second needs a rebase. #342 is independent of both.

coordination: unavailable — repo seam sets coordination_backend: "n/a" (.agents/agent-workflow.yml:10), single-operator source repo.

Completed-batch audit: not published. completed-batch-publication-preflight cannot run here for two independent reasons — there is no coordination status to supply (backend n/a), and no target is merged, which it refuses on its own.

Local merge-gate artifacts (CI-readiness / eligibility / assurance JSON) were transient and did not survive temp cleanup; regenerate from the helpers in skills/pr-batch/bin/ against base 282a87aaf4bf1dd602723efddc78252f7a138235.