Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment has been minimized.
This comment has been minimized.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1196 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # on this org plan). If a plan-gated key is "disabled" (present, not null), | ||
| # that is a failure — the key exists but was not enabled. | ||
| if [ "$is_plan_gated" = true ] && [ "$post_actual" = "null" ]; then | ||
| if [[ "$is_plan_gated" = true ]] && [[ "$post_actual" = "null" ]]; then |
There was a problem hiding this comment.
Suggestion: When GitHub leaves a plan-gated setting disabled after an accepted PATCH, this condition treats it as a failure and makes repository setup fail.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/lib/push-protection.sh
**Line:** 197:197
**Comment:**
*Api Mismatch: When GitHub leaves a plan-gated setting disabled after an accepted PATCH, this condition treats it as a failure and makes repository setup fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Skipping — after validation this is a deliberate, documented design decision (#1038, AC6b), not a defect. The apply-path verify models an unavailable plan-gated feature as reporting null (see the secret_scanning_ai_detection note in PP_REQUIRED_SA_SETTINGS: "absent means the feature is unavailable for the current org plan"), and treats a plan-gated key that is present-but-disabled as a genuinely-fixable failure. Unlike the GHAS-gated keys — which have an independent advanced_security.status signal to confirm the plan is absent — there is no plan-status signal to distinguish "disabled because the plan silently ignored the PATCH" from "disabled but enable-able." Treating every disabled plan-gated key as a skip would silently accept a disabled security control, so I'm leaving the fail-closed behavior in place. If GitHub is confirmed to return disabled (not null) for an unavailable plan feature, a maintainer should reopen with that evidence.
| fi | ||
| done | ||
| if [ "$is_plan_gated" = true ] && [ "$actual" = "null" ]; then | ||
| if [[ "$is_plan_gated" = true ]] && [[ "$actual" = "null" ]]; then |
There was a problem hiding this comment.
Suggestion: When a plan-gated setting is reported as disabled, this condition still creates a compliance finding even though unsupported plans may silently ignore the setting.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/lib/push-protection.sh
**Line:** 295:295
**Comment:**
*Api Mismatch: When a plan-gated setting is reported as disabled, this condition still creates a compliance finding even though unsupported plans may silently ignore the setting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Skipping — same rationale as the apply-path thread at line 197: this is the deliberate, documented behavior from #1038, not a defect. The audit models an unavailable plan-gated feature as null (skipped, no finding) and a present-but-disabled plan-gated key as a genuinely-fixable finding. There is no independent plan-status signal (as GHAS has via advanced_security.status) to tell "disabled because the plan silently ignores it" apart from "disabled but enable-able," so suppressing a finding for every disabled plan-gated key would let a disabled security control read as compliant. Note the finding is warning severity, not error. Leaving as-is; a maintainer who can confirm the disabled-when-unavailable API behavior should reopen.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag plan-gated settings as API mismatches when present-but-disabled values are intentionally treated as fixable findings; only unavailable settings represented as null should be skipped.
Applied to:
scripts/lib/push-protection.sh
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| rc=1; continue | ||
| fi | ||
| if [ -z "$maintainer" ] || [ "$maintainer" = "—" ] || [ "$maintainer" = "-" ]; then | ||
| if [[ -z "$maintainer" ]] || [[ "$maintainer" = "—" ]] || [[ "$maintainer" = "-" ]]; then |
There was a problem hiding this comment.
Suggestion: The validator still rejects non-empty maintainer names that are not GitHub handles, contradicting the required non-empty attribution-only rule.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/agents-md-cycle-log.sh
**Line:** 140:140
**Comment:**
*Logic Error: The validator still rejects non-empty maintainer names that are not GitHub handles, contradicting the required non-empty attribution-only rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Skipping — this appears to be a false positive. The required rule for agents-md-cycle-log is not "non-empty attribution only"; the standard requires the "Determined by" field to be a GitHub @handle. That is enforced deliberately (line 144) and covered by an explicit test — tests/agents_md_cycle_log.bats: "validate rejects a maintainer that is not a GitHub @handle" — whose rationale is that arbitrary attribution text like "Alice" is not attribution the append-only log can hold anyone accountable to. Relaxing the validator to accept any non-empty name would regress that tested requirement, so no change. (If the standard itself is meant to change from @handle to free-text attribution, that's a standards-doc decision to make first.)
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Require the 'Determined by' maintainer field to be a GitHub @handle; do not relax validation to accept arbitrary non-empty names.
Applied to:
scripts/agents-md-cycle-log.sh
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
No description provided. |
This comment has been minimized.
This comment has been minimized.
![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 systematically migrates conditional checks across numerous shell scripts from single brackets [ to double brackets [[ for improved robustness. Additionally, it updates sonar-project.properties to exclude node_modules/** from SonarCloud analysis to prevent third-party code findings. A critical issue was identified in scripts/lib/canary-rollout.sh where using double brackets [[ for numeric comparisons of version components (e.g., _semver_gt) can trigger octal interpretation errors and runtime crashes if components contain leading zeros (like 08 or 09); it is recommended to revert to single brackets [ for these specific comparisons.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Acknowledged — this is a Codex usage-limit notice, not a code finding; no action required on the PR. Code review for this branch is otherwise covered by the other reviewers (CodeAnt, SonarCloud, CodeQL). |
This comment has been minimized.
This comment has been minimized.
|
Acknowledged — SonarCloud reports the Quality Gate as Passed for this PR; this is a status notice with no finding to action. |
|
Reviewed all 4 nitpicks — each is a real, small robustness issue (truncation warning in |
Dev-Lead — fix-reviews (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
Resolved — the `needs-human-review` hold was lifted; dev-lead has picked this item up. Click to expand the prior hold notice.dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
Dev-Lead — fix-bot-comment (applied)Changes committed and pushed. |
|
No description provided. |
|
All actionable code review findings have been addressed in commits on this branch:
The pre-merge scope/linkage checks are advisory guidance for future PRs, not blocking issues. All CI checks pass. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
No description provided. |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Dev-lead rebase aborted: this branch has no merge base with 🤖 Generated with Claude Code |
Dev-Lead — rebase (failed)The engine run completed but PR #1196 is still CONFLICTING per GitHub's mergeable state — the rebase did not converge. Recorded as a failure so repeated no-op 'success' runs can no longer accumulate. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Dev-lead rebase aborted: unresolvable add/add conflict in This PR's only unique commit (37b4012) adds Likely resolution: the PR is superseded by main and should be closed. If it intends to change something specific beyond what main has, please re-apply that delta on top of current 🤖 Generated with Claude Code |
Dev-Lead — rebase (failed)The engine run completed but PR #1196 is still CONFLICTING per GitHub's mergeable state — the rebase did not converge. Recorded as a failure so repeated no-op 'success' runs can no longer accumulate. |
Dev-Lead — rebase (exhausted)This PR's rebase conflict failed automated resolution 2 time(s) (timeouts or unresolvable conflicts). Automated rebasing is paused to stop repeated full-timeout runs from the auto-rebase-conflict sentinel. Reason for last failure: The engine run completed but PR #1196 is still CONFLICTING per GitHub's mergeable state — the rebase did not converge. Recorded as a failure so repeated no-op 'success' runs can no longer accumulate. Resolve the conflict manually, then remove the |
|
No description provided. |
|
All CodeRabbit findings have been addressed in prior commits. Verified fixes in place across scripts/agents-md-cycle-log.sh, scripts/compliance-audit.sh, scripts/lib/agent-rate-limit.sh, scripts/lib/push-protection.sh, scripts/sonarcloud-audit.sh, and scripts/canary-rollout.sh. All CI checks passing. |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
2 participants
Problem
SonarCloud: shell script hygiene
From the issue: 1212 open SonarCloud finding(s) in
petry-projects/.github, worst severity CRITICAL. Generated by the org SonarCloud Audit.Risk
Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.
Test plan
No test files were added or updated. Verification:
bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the existing CI (bats + lint) guards the change.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 #1193