feat: implement issue #756 — Provision <id>:hands-off labels so persona opt-out actually works - #757
Conversation
…na opt-out actually works
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
Good work on the derivation — One defect, and it's an ironic one.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic derivation of persona opt-out labels from persona manifests in scripts/apply-repo-settings.sh, updates the corresponding documentation in standards/github-settings.md, and adds a new Bats test suite to verify the functionality. The review feedback highlights several robust improvements: safely handling potential gh api command failures and stripping carriage returns to prevent script crashes, using the jq optional operator (.[]?) to handle non-array API responses, optimizing the Bats test suite by sourcing the script once in setup(), and utilizing $BATS_TEST_TMPDIR for cleaner temporary directory management during testing.
There was a problem hiding this comment.
Pull request overview
Implements issue #756 by making persona opt-out labels (<id>:hands-off) automatically provisioned across repos, derived from persona manifests (instead of being hand-maintained), and documents the derived label family in the org standards.
Changes:
- Add
persona_opt_out_label_configs()to derive per-persona opt-out label configs from.github-privatepersona manifests, and extendapply_labels()to provision them. - Update
standards/github-settings.mdto describe the opt-out labels as a derived family (with a single shared color) and reconcile existingdev-lead:hands-offdrift. - Add bats unit tests plus a gating GitHub Actions workflow to shellcheck + test the applier behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/apply-repo-settings.sh |
Derives and provisions <id>:hands-off labels from persona manifests; makes script sourceable for bats tests. |
standards/github-settings.md |
Documents opt-out labels as a derived family and reconciles dev-lead:hands-off as part of that family. |
test/scripts/apply-repo-settings/apply-repo-settings.bats |
Adds hermetic bats coverage for derivation, dry-run, idempotency, and degradation behavior (via gh stubs). |
.github/workflows/apply-repo-settings-tests.yml |
Adds CI gate to run shellcheck + bats when the applier/tests change. |
Superseded by automated re-review at
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Superseded by automated re-review at
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: ff8c6bf5b9c61b1a22cda2b1ae7b8ead40c7a8b4
Review mode: triage-approved (single reviewer)
Summary
Implements issue #756: persona opt-out labels (:hands-off) are now provisioned by apply-repo-settings.sh, derived dynamically from the persona manifests in petry-projects/.github-private instead of a hand-maintained list. Adds a hermetic bats suite (11 tests) with a gh stub, a dedicated test workflow, and updates the label taxonomy in standards/github-settings.md including reconciliation of the pre-existing dev-lead:hands-off drift. All five findings from the prior review round are resolved in the current head commit.
Linked issue analysis
Issue #756 requires: (1) derive opt-out labels from persona manifests in apply_labels() — done via persona_opt_out_label_configs(), which lists personas// dirs and reads each manifest's opt_out_label with a :hands-off convention fallback; (2) document the family as derived, not enumerated, in standards/github-settings.md — done (new 'Derived family' section); (3) reconcile dev-lead:hands-off drift — done (documented as a member of the derived family, retained since the applier never deletes). The 'derive, do not enumerate' constraint is honored. The docs' claim that .github-private is public was independently verified (private: false).
Findings
All 5 prior review threads are resolved and each fix was verified present in the diff: (1) gh api failure inside persona_opt_out_label_configs is wrapped in if/else with a stderr warning and empty fallback, so set -e no longer aborts the run; (2) jq uses .[]? to tolerate non-array API responses; (3) the bats suite sources the script once in setup() instead of re-sourcing per test; (4) mktemp -d now uses $BATS_TEST_TMPDIR templates; (5) manifest derivation is cached across repos in --all mode via _PERSONA_OPT_OUT_CONFIGS_CACHE (correctly placed at apply_labels level since the derivation function runs in a process-substitution subshell). New workflow follows org standards: permissions: {} at top level, contents: read at job level, SHA-pinned checkout, concurrency group, 10-min timeout. Label names derived from remote content are sanitized (first whitespace token, quotes/CR stripped) and sourced from an org-controlled repo. No security concerns. Secret scanning MCP tool was unavailable this run; the gitleaks CI check passed.
CI status
All checks green: Lint and bats, ShellCheck, CodeQL (actions), Secret scan (gitleaks), SonarCloud, Agent Security Scan, AgentShield, npm audit, CodeRabbit. No failures; inapplicable ecosystem audits skipped.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
* fix(labels): persona opt-out sync must not fail open #757 shipped with the fail-open flagged in review (#755). It is live on main: ids=$(gh api .../contents/personas ...) || { warn " Could not list persona manifests ... — skipping opt-out labels" return 0 } A transient API error emits no labels and returns success, so a repo gets "All repos processed successfully" while the <id>:hands-off family is silently absent — and the escape hatch §4 rule 4 MANDATES does not exist there. Nobody finds out until someone tells a persona to leave an issue alone and it ignores them. That is a fifth instance of the systemic pattern #755 names as its headline finding: reading an error as a negative answer. Keeps both halves, which is the actual requirement: - RESILIENCE — the static 7 labels still land; a persona hiccup must not block unrelated label work (the original rationale, and it was right). - HONESTY — the run can no longer CLAIM success. persona_opt_out_label_configs returns non-zero, apply_labels records it, and both main() exit paths (--all and single-repo) fail the run. Why it survived review: the caller used `mapfile < <(persona_opt_out_label_configs)`. Process substitution runs the function in a subshell whose exit code is unreachable — mapfile reports its own status — so a failure there literally could not be seen. Switched to $( ), also a subshell but its status propagates. Also marks the GUESSED-label path as a failure. <id>:hands-off is only a CONVENTION (§4 rule 4); the schema lets a persona declare any opt_out_label. If the manifest is unreadable we may create a label nobody uses while the real one stays absent — opt-out silently broken. The guess is still emitted (it is the best available), but the run says so. Two tests ENCODED the defect as intended behaviour and are rewritten: - "unavailable manifest listing degrades to no opt-out labels (returns 0)" -> "... returns NON-ZERO (does not fail open)" - "apply_labels still succeeds when persona derivation is unavailable" -> split into the resilience half (static 7 still land) and the honesty half (the failure flag is set) The gh stub always exited 0, even for a missing fixture, so the fetch-failure path was untestable — a real `gh api` on an absent manifest exits non-zero (404). Made the stub faithful; that is what let the new guessed-label test fail first. Verification - bats test/scripts/apply-repo-settings/apply-repo-settings.bats: 14/14 (11 + 3 new: non-zero on listing failure, non-zero on unreadable manifest, flag unset on a healthy derivation). - shellcheck --severity=warning -x: clean. - Live check against the real public manifests: derives 'qa-lead:hands-off|ededed|...' and exits 0 on the happy path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(labels): don't cache a failed derivation; don't truncate labels at whitespace Both from Copilot's review of this PR; both correct. 1. A failed derivation was cached (my bug, introduced here) _PERSONA_OPT_OUT_CONFIGS_CACHED was set to true even on failure. In --all mode the cache is shared across all 11 repos, so ONE transient API blip on the first repo denied opt-out labels to every LATER repo too, even after the API recovered — turning a hiccup into a fleet-wide gap. Now only a GOOD derivation is cached; a failure retries on the next repo. The retry costs a few API calls on the failure path only. _PERSONA_OPT_OUT_SYNC_FAILED stays STICKY on purpose: the repo that failed went without its labels, so the run did not do what it claims even if later repos recover. Resilience and honesty are separate concerns and both hold. 2. awk '{print $1}' truncated opt_out_label at the first word opt_out_label is free-form in the schema and GitHub label names may contain spaces, so a persona declaring "needs human review" would have provisioned a label named "needs" — leaving the real opt-out absent and the hatch silently broken, which is the exact class of bug this PR exists to fix. Now takes the whole scalar and strips a trailing YAML comment, trailing space, and surrounding quotes. 3. Tests used `|| true`, which masks failures `|| true` would let the flag assertions pass even if apply_labels started returning non-zero or tripped errexit. Replaced with an explicit status capture under `set +e`. The first attempt used $(...) to return the status — which defeated itself, because command substitution is a subshell and the flag mutations being asserted happen in the current shell. The helper now records the status in a global instead. Verification - bats test/scripts/apply-repo-settings/apply-repo-settings.bats: 18/18 (14 + 4 new: failure not cached / success cached / spaces preserved / trailing comment stripped). - shellcheck --severity=warning -x: clean. - Live check against the real public manifests: still derives 'qa-lead:hands-off|ededed|...' and exits 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes #756
Implemented by dev-lead agent. Please review.