[don-petry]

Closes #1781

Implemented by dev-lead agent. Please review.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 9f6bdec9-eb18-4b52-acd6-c10302ac331d

📥 Commits

Reviewing files that changed from the base of the PR and between 781b63c and e4e48d1.

⛔ Files ignored due to path filters (1)

• tests/fixtures/agent-ingress/if-filter-rulings.tsv is excluded by !**/*.tsv

📒 Files selected for processing (2)

• scripts/validate-ingress-if.sh
• tests/test_validate_ingress_if.bats

---

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

[gemini-code-assist]

Code Review

This pull request enhances the ingress validation script to detect and forbid bare context roots (such as vars, secrets, and env) when they are used as whole values without a trailing dot, while also ensuring that policed names inside string literals are ignored. The feedback recommends improving the robustness of the newly added BATS tests by asserting specific non-zero exit codes (e.g., [ "$status" -eq 1 ]) instead of using generic -ne 0 checks, which can mask unexpected syntax or command errors.

[don-petry]

Dev-Lead — review-changes (applied)

Changes committed and pushed. Requested items addressed:

• scripts/validate-ingress-if.sh:81 — applied
• tests/test_validate_ingress_if.bats:184 — applied
• tests/test_validate_ingress_if.bats:195 — applied
• tests/test_validate_ingress_if.bats:224 — applied

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-21T04:02:13Z.

[don-petry]

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

- `[ -z "$output" ]` for empty output assertions
No generic `-ne 0` patterns appear in the new test blocks.
---
**Result:**
**No actionable issues found.** The PR has:
- ✅ Quality gate passing
- ✅ All CI checks succeeding
- ✅ No tier-1 blockers
- ✅ Recommended test patterns already implemented
No changes needed.

[donpetry-bot]

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

[don-petry]

dev-lead is withholding action on this item.

It is labeled needs-human-review (flagged for human review — this label is applied by automation as well as by people, so an item can become held without anyone noticing), so dev-lead will not pick it up while that label is present. This notice is posted once so the withhold is visible rather than looking like a stalled run.

To re-enable automated pickup: remove the needs-human-review label.

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-21T14:26:23Z.

[donpetry-bot]

Superseded by automated re-review at e4e48d115dcbeaf4e6a43d6295792c003918153a — click to expand prior review.

Review — fix requested (cycle 1/3)

The automated review identified the following issues. Please address each one:

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: 84db87e5f913a91cd146fa3d2bce153499456fdb
Cascade: triage → audit (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5)

Summary

Genuine security hardening of scripts/validate-ingress-if.sh that closes three real bypasses (bare context roots like toJSON(secrets), case-insensitive roots like VARS.X/GitHub.actor, and literal false-positives), independently verified sound: token-boundary regexes are correct in both directions, the literal-strip refactor cannot swallow a real context reference in any valid Actions expression, and the sole caller of the refactored helper pre-strips as required. Escalating anyway: the PR description gate fails (all 5 required template sections absent, template escalates at 3+), and the change deliberately extends the frozen rulings table — a reviewed security boundary that CODEOWNERS routes to org-leads, so human sign-off must not be short-circuited by an AI approval.

Findings

• major: PR body is 'Closes [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781 / Implemented by dev-lead agent. Please review.' — all 5 required template sections (Problem, Risk, Test plan, Rollback, Monitoring) are absent. The repo PR template explicitly states triage escalates any PR missing 3+ of them; no TRUSTED_STUB_SYNC carve-out applies to this code change. Gate fails regardless of code quality.
• info: Bare-root token matching independently verified: left boundary (^|[^._[:alnum:]-]) prevents false positives on payload fields like github.event.inputs.; right boundary for vars/secrets/needs permits '.' so dotted forms remain caught, while bare github excludes '.' preserving the github.event. allowlist. Change is strictly stricter and incidentally closes the whitespace-index gap (vars ['X'] normalizes to 'vars .X', whose bare token now matches) flagged as an open question in [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781.
• info: Deep review asked Tier-3 to confirm the literal-strip contract of viif_reaches_unlisted_context. Confirmed: code search shows the only caller repo-wide is viif_forbidden (which strips literals after normalization, correct ordering), and the bats suite exercises only viif_forbidden. Additionally verified the sed pair-stripping cannot hide a real context reference in any syntactically valid Actions expression (single-quoted literals with '' escapes always consume an even quote count, so sed's left-to-right pairing stays inside parser literals; double quotes outside YAML quoting are invalid expressions that fail workflow load).
• minor: toJSON(github.event) — whole-payload serialization — remains ALLOWED and no ruling was recorded in the frozen TSV, though issue [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781's open questions asked for an explicit recorded decision. The serialized payload includes snapshot fields forbidden individually (repository.default_branch, the labels array). Non-blocking per the issue, but recommend a follow-up ruling row so the boundary is explicit.
• minor: The frozen rulings table (the machine-readable security boundary consumed directly by the guard) gains 5 new FORBID rows and 1 ALLOW row. The additions are correct and match [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781 AC Add @claude delegation, auto-merge, and rebase handling #3, but a frozen-boundary change is exactly the class of edit that must receive explicit human (org-leads) review; merge is correctly BLOCKED with needs-human-review and REVIEW_REQUIRED.

---

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5). Reply if you need a human review.

Additional tasks

1. Resolve all unresolved review thread comments from other reviewers
2. Ensure all CI checks pass after your changes
3. Rebase on the target branch if behind
4. Do NOT modify files unrelated to the findings above

The review cascade will automatically re-review after new commits are pushed.

[donpetry-bot]

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-22T02:48:46Z.

[donpetry-bot]

Review — fix requested (cycle 2/3)

The automated review identified the following issues. Please address each one:

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: e4e48d115dcbeaf4e6a43d6295792c003918153a
Cascade: triage → audit (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5)

Summary

Audit of the hardened agent-ingress if: validator (issue #1781) found the enforcement change technically sound: I executed the PR-head viif_forbidden against 29 adversarial cases beyond the PR's own tests (bare roots, mixed-case roots and bracket indices, secrets[format(...)], (secrets).X, whitespace-before-index vars ['X'], escaped-quote literals, literal-smuggled policed names) and every bypass attempt is FORBIDDEN while all legitimate event filters stay ALLOWED; literal-stripping runs after normalization with no un-stripped caller, and token-boundary asymmetry (left excludes dot, right permits it) is correct. Escalating on gates, not code: all 5 required description sections are missing for a change to an authorization control's enforcement logic, the PR carries the needs-human-review label with an explicit dev-lead hold (mergeStateStatus BLOCKED, review required), and advisory-bot evidence was partial (4/6, rate-limited). The only remaining over-reach, toJSON(github.event) whole-payload serialization, is the pre-existing gap #1781 explicitly deferred as a non-blocking open question — record the ruling in a follow-up, not this PR.

Findings

• major: All 5 required PR description sections (problem, risk, test-plan, rollback, monitoring) are missing; body is only 'Closes [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781 ... Please review.' For a change to the enforcement logic of a security/authorization control, risk, rollback, and monitoring documentation is mandatory before merge.
• minor: PR is labeled needs-human-review with an explicit dev-lead hold notice, mergeStateStatus is BLOCKED, and the latest advisory-bot cycle was rate-limited (prior cycle proceeded on partial 4/6 evidence). Human sign-off is required regardless of this audit's technical verdict.
• info: Enforcement logic verified sound by execution: sourced the PR-head script and ran 29 adversarial expressions. Bare roots (toJSON(env|vars|secrets|needs|inputs|github)), nested roots (fromJSON(toJSON(secrets))), case variants (VARS.X, Secrets['T'], GitHub.actor, GITHUB.REPOSITORY), computed indices (secrets[format('{0}','X')]), parenthesized roots ((secrets).X), and whitespace-index (vars ['X'] — closing [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781's open question incidentally) are all FORBIDDEN with the construct named; all legitimate event filters and literal-only mentions of policed names (including Actions ''-escaped quotes) remain ALLOWED. No false-negative or false-positive found.
• info: Residual known gap, deliberately out of scope: toJSON(github.event) (whole-payload serialization, which includes individually-forbidden snapshot fields like repository.default_branch and the labels array) is still ALLOWED. Issue [Phase 2b] Close the bare-context-root bypass in the agent-ingress if: allowlist (toJSON(env|vars|secrets|github|needs|inputs)) #1781 explicitly defers this as an advisory open question; recommend a follow-up ruling in the frozen TSV with rationale.
• info: Multiline-literal edge (a quoted literal spanning lines, if GitHub's parser ever accepts it) can only produce false positives with the per-line sed stripping, never a bypass — the stripping char class [^']* cannot cross a quote, so unquoted code is never consumed. Fails closed; no action required.

---

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5). Reply if you need a human review.

Additional tasks

1. Resolve all unresolved review thread comments from other reviewers
2. Ensure all CI checks pass after your changes
3. Rebase on the target branch if behind
4. Do NOT modify files unrelated to the findings above

The review cascade will automatically re-review after new commits are pushed.