feat: implement issue #1872 — Harden the issue-comment trust filter: a missing author_association defaults to OWNER, the one fail-open in a trust boundary

[don-petry]

Problem

Harden the issue-comment trust filter: a missing author_association defaults to OWNER, the one fail-open in a trust boundary

From the issue: The issue-comment trust filter added by PR #1800 (for #1566) defaults a missing author_association to OWNER — the most-trusted value — on a filter whose entire purpose is to decide whose comments may steer dev-lead's implementation.

Risk

Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.

Test plan

Tests added/updated: tests/dev-lead/unit/test_fix_issue.bats,tests/dev-lead/unit/test_issue_comments.bats. Verification: bash scripts/dev-lead-lint.sh (shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.

Rollback

Revert this PR. No non-revertible side effects (no tags, migrations, or external state).

Monitoring

This PR's Lint (shellcheck) and bats checks show pass/fail; watch subsequent dev-lead / pr-review runs for behavioral regressions.

Closes #1872

[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, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[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

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

Next included review available in 4 minutes.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: db5346e3-d196-4e5b-a7d3-5eb537b08975

📥 Commits

Reviewing files that changed from the base of the PR and between 776c958 and 34040e0.

📒 Files selected for processing (3)

• scripts/lib/issue-comments.sh
• tests/dev-lead/unit/test_fix_issue.bats
• tests/dev-lead/unit/test_issue_comments.bats

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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 security of the GitHub issue comment trust filter by failing closed when the 'author_association' field is missing, null, or empty. It also sanitizes control characters in user logins to prevent forgery of kept comments and logs dropped comments to STDERR. Unit tests have been updated and expanded to verify these changes. Feedback on the changes suggests appending '|| [ -n "$tag" ]' to the 'read' loop condition in the shell script to ensure the final line of input is processed correctly even if it lacks a trailing newline.

[cubic-dev-ai]

All reported issues were addressed across 3 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[don-petry]

Dev-Lead — review-changes (applied)

Changes committed and pushed. Requested items addressed:

• scripts/lib/issue-comments.sh:78 — applied
• tests/dev-lead/unit/test_issue_comments.bats:250 — applied

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T14:36:22Z.

[donpetry-bot]

Superseded by automated re-review at e9516b324a74eb4741d7c60e21f71dd48d03d45c — 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: e9516b324a74eb4741d7c60e21f71dd48d03d45c
Cascade: triage → deep+duck (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7])

Summary

Both reviewers rate the risk MEDIUM and both escalate, so they agree fully. Neither found a security or logic defect: the fail-closed author_association change is correct and the control-character stripping blocks forged K records. Both reviewers flagged the same two things at scripts/lib/issue-comments.sh:78. The || [ -n "$tag" ] no-trailing-newline branch is untested and its review thread is still open. IFS=tab read collapses consecutive tabs, which only mangles a log line. The 'unit' and 'bats' checks were still pending at review time, and the rubber duck independently confirmed the security reasoning.

Cross-engine agreement

full

Findings

• major: {"severity":"major","category":"test-coverage","message":"Unresolved cubic-dev-ai thread: the || [ -n \"$tag\" ] final-record-without-newline branch has no regression test, because jq always newline-terminates its output. Add a direct test or reply in the thread and resolve it. Both reviewers flagged this. The open thread fails the no-unresolved-threads gate.","file":"scripts/lib/issue-comments.sh","line":78,"sources":["deep","rubber-duck"]}
• minor: {"severity":"minor","category":"robustness","message":"IFS=$'\t' read collapses consecutive tabs, so a D record with an empty login shifts the fields and the log line is mangled. This is cosmetic and affects stderr only. Real logins are never empty.","file":"scripts/lib/issue-comments.sh","line":78,"sources":["deep","rubber-duck"]}
• info: {"severity":"info","category":"security","message":"Fail-open is now fail-closed. A missing, null, empty, false or object-valued author_association is dropped, and lowercase 'member' is still kept. The only production caller uses REST output, which always includes author_association.","file":"scripts/lib/issue-comments.sh","line":62,"sources":["deep"]}
• info: {"severity":"info","category":"injection","message":"A malicious login cannot forge a 'K' record. Login and association are stripped of control characters before framing, and K payloads are base64. Tested with an injected newline and tab in the login.","file":"scripts/lib/issue-comments.sh","line":66,"sources":["deep"]}
• info: {"severity":"info","category":"security","message":"Control-character sanitization of the login in D lines stops K-line forgery. A jq failure yields empty output, so the pipeline fails closed.","file":"scripts/lib/issue-comments.sh","line":71,"sources":["rubber-duck"]}
• minor: {"severity":"minor","category":"ci","message":"The 'unit' check (Test Dev-Lead Agent) was still IN_PROGRESS and several validate-* checks were queued at review time. bats could not be run locally.","file":null,"line":null,"sources":["deep"]}
• info: {"severity":"info","category":"secret","message":"The MCP secret scan was unavailable and was skipped. The gitleaks CI check passed.","file":null,"line":null,"sources":["deep"]}

---

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7]). 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]

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: e9516b324a74eb4741d7c60e21f71dd48d03d45c
Cascade: triage → deep+duck (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7])

Summary

Both reviewers agree on MEDIUM risk and approve: the issue-comment trust filter now fails closed on missing author_association, and control characters are stripped from logged fields so K lines cannot be forged. Both independently flagged the untested no-trailing-newline path (|| [ -n "$tag" ]) at scripts/lib/issue-comments.sh:78, but under different category labels, so the findings are kept separate at their original severities; it is defensive-only and non-blocking. The rubber duck alone noted that IFS tab-splitting collapses consecutive tabs in drop log lines, a cosmetic issue.

Cross-engine agreement

full

Findings

• info: Fail-open default removed: missing/null/empty author_association now routes to an explicit drop record; only OWNER/MEMBER/COLLABORATOR are kept. Strict tightening. (scripts/lib/issue-comments.sh:63)
• info: Record-forging surface checked: kept payloads are base64, drop records strip control chars from login and association, and the stderr prefix cannot start a workflow command. (scripts/lib/issue-comments.sh:66)
• info: The only production caller uses the REST endpoint, which always includes author_association, so trusted comments are unaffected. A future GraphQL switch (camelCase authorAssociation) would visibly drop all comments, which is the intended fail-closed behavior. (scripts/dev-lead-fix-issue.sh:572)
• minor: Cubic P3 advisory: no test exercises the || [ -n "$tag" ] final-record-without-trailing-newline guard. jq always newline-terminates, so it is unreachable today. Non-blocking. (scripts/lib/issue-comments.sh:78)
• info: No direct test for the final-record-without-trailing-newline path; defensive only since jq always emits a trailing newline. (scripts/lib/issue-comments.sh:78)
• info: IFS=$'\t' read collapses consecutive tabs, so an empty login would shift fields in D log lines. Cosmetic only: login defaults to 'unknown', and K records carry base64 with no tabs. (scripts/lib/issue-comments.sh:78)
• info: MCP run_secret_scanning was unavailable, so no MCP scan ran. The gitleaks CI check passed and the diff has no credentials.

---

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.

Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T14:50:25Z.

[donpetry-bot]

Review — fix requested (cycle 3/3)

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

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: 30e0c665f9b47e9a5d4a16dc0cf5d9f3ff8be5e1
Review mode: triage-approved (single reviewer)

Summary

Fails the dev-lead issue-comment trust filter closed (#1872). A missing, null, or empty author_association is now untrusted and dropped instead of defaulting to OWNER. Every trust drop is logged to stderr, and the log tells a missing association apart from an untrusted one. The code is correct and all of #1872's acceptance criteria are met. The one blocker is CI: about 30 required checks, including Lint, bats, ShellCheck and unit, are still queued or running on this commit, so it cannot be approved yet.

Linked issue analysis

Closes #1872. All four acceptance criteria are met:

1. Missing or empty field is untrusted: (.author_association? // "") | tostring | ascii_upcase, and "" goes to a drop record. Null, empty and absent were all checked by hand and all are dropped.
2. Trust is set where comment objects are built: the only production caller (scripts/dev-lead-fix-issue.sh:572) feeds real gh api .../issues/N/comments output, which always carries the field. Every synthetic test fixture (test_issue_comments.bats, test_fix_issue.bats mock) now sets author_association explicitly.
3. Separate log lines: no author_association (treated as untrusted) vs author_association=NONE (no write access). Both go to stderr only; the comment body is never logged.
4. Tests: cover a missing key, empty and null values, stderr-only logging, a forged K record through control characters in the login, and the last-record-without-newline case.

Findings

Incremental since e9516b3: one new commit (30e0c66). It only adds the regression test that cubic asked for (issue-comments.sh:78, the || [ -n "$tag" ] read guard). The test shadows jq with a function that prints a K record with no trailing newline, then checks that _ic_filter_human still returns it. I ran the same scenario by hand and it decodes correctly. All 3 review threads are resolved.

Manual checks on 30e0c66: shellcheck --severity=warning scripts/lib/issue-comments.sh passes. I ran the filter on these inputs:

• Absent, null or empty association: dropped.
• Lowercase member: kept, because the value is upper-cased first.
• Numeric association: dropped.
• NONE: dropped and logged.
• Untrusted login carrying \nK\t<base64>: control characters become ? and nothing forged renders.
• Tab in the login of a dropped comment: neutralised.

Non-blocking (existing behaviour, not caused by this PR): on the kept path, login is rendered into the markdown heading without the control-character cleanup. It travels inside base64, so it cannot forge a record. It only reaches the rendered heading, and real GitHub logins cannot contain control characters. Could be tidied up in a follow-up.

No blocking code findings.

CI status

Not yet green. Completed checks pass: CodeQL, SonarCloud (quality gate passed), gitleaks, AgentShield, shellcheck, unit-tests, holdout-guard and others. Still QUEUED or IN_PROGRESS on 30e0c66:

• Lint, ShellCheck, bats, unit
• actionlint, validate-fixtures, check-model-pins, caller-stub-freeze, stub-structure, template-drift
• cubic and several other validators

Approval requires all checks to pass. Re-review once they finish; if they come back green this should be approvable as-is.

---

Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). 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.

[cubic-dev-ai]

All reported issues were addressed across 1 file (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[don-petry]

Dev-Lead — fix-reviews (applied)

Changes committed and pushed. Requested items addressed:

• tests/dev-lead/unit/test_issue_comments.bats:268 — applied

[donpetry-bot]

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 30e0c665f9b47e9a5d4a16dc0cf5d9f3ff8be5e1
Cascade: triage → deep+duck (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7])

Summary

Both reviewers agree on MEDIUM risk and approve: the trust filter now fails closed on missing, null or empty author_association, and control characters in logged fields are neutralised. Both engines converged on the GNU-only base64 -d in the no-trailing-newline test; this is raised to major by the cross-engine rule, but CI runs on Linux and the newer head 34040e0 already adds a base64 -D fallback. The empty-login log-field collapse was flagged by both as cosmetic only, with no effect on trust decisions. The head-drift note is unique to the deep reviewer, and the rubber duck found nothing unique.

Cross-engine agreement

full

Findings

• major: {"severity":"major","category":"test-portability","message":"The 'final record without a trailing newline' test decodes with GNU-only base64 -d; other tests use a portable fallback. CI runs on Linux, and the newer head 34040e0 adds a base64 -D fallback (test-only change).","file":"tests/dev-lead/unit/test_issue_comments.bats","line":268,"sources":["deep","rubber-duck"]}
• info: {"severity":"info","category":"logging","message":"If a login is empty, the tab separators collapse under IFS=tab read, so the drop log line loses or shifts fields. The comment is still dropped, so trust is unaffected.","file":"scripts/lib/issue-comments.sh","line":78,"sources":["deep"]}
• info: {"severity":"info","category":"logging","message":"The drop-log read uses IFS=tab, which collapses adjacent tabs, so an empty login would shift the fields in the stderr log line. This is cosmetic, because K lines carry base64 only.","file":"scripts/lib/issue-comments.sh","line":80,"sources":["rubber-duck"]}
• info: {"severity":"info","category":"head-drift","message":"The PR head moved from the reviewed SHA 30e0c66 to 34040e0 during the review. The only change is the test-only base64 fallback; scripts/lib/issue-comments.sh is unchanged.","file":null,"line":null,"sources":["deep"]}

---

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.

[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-10-02T14:57:40Z.