Skip to content

feat: implement issue #1232 — [bug] Phase 4 gate wiring ships to adopter stubs but agent-rate-limit-gate.sh is absent at v1 — gate is inert and fail-open - #1234

Merged
don-petry merged 2 commits into
mainfrom
dev-lead/issue-1232-20261002-1725
Oct 2, 2026
Merged

don-petry merged 2 commits into
mainfrom
dev-lead/issue-1232-20261002-1725

Conversation

@don-petry

@don-petry don-petry commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

[bug] Phase 4 gate wiring ships to adopter stubs but agent-rate-limit-gate.sh is absent at v1 — gate is inert and fail-open

From the issue: Phase 4 (#640, PR #1059) wired the agent rate-limit gate into the initiative-driver.yml caller stub. The stub fetches the gate tooling from petry-projects/.github at ref v1. scripts/agent-rate-limit-gate.sh does not exist at v1. v1 is d3d768da, which is 359 commits behind main, and the script is only on main.

Risk

Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.

Test plan

Tests added/updated: test/workflows/initiative-driver/gate-tooling.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

Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.

Closes #1232

Review in cubic

…ter stubs but agent-rate-limit-gate.sh is absent at v1 — gate is inert and fail-open
@chatgpt-codex-connector

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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 38 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: 79e0a42c-a775-419c-8ab8-5f0f0d7cad6a

📥 Commits

Reviewing files that changed from the base of the PR and between cd62129 and 8b8759d.

📒 Files selected for processing (4)
  • .github/workflows/initiative-driver.yml
  • standards/agent-rate-limits.md
  • standards/workflows/initiative-driver.yml
  • test/workflows/initiative-driver/gate-tooling.bats

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 76b52ee5-703c-4b64-a671-649be1734f20

📥 Commits

Reviewing files that changed from the base of the PR and between cd0b167 and cd62129.

📒 Files selected for processing (4)
  • .github/workflows/initiative-driver.yml
  • standards/agent-rate-limits.md
  • standards/workflows/initiative-driver.yml
  • test/workflows/initiative-driver/gate-tooling.bats

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Workflow Updates

    • Rate-limit checks now run after concurrency handling, so superseded runs can still be canceled before dispatch checks begin.
    • Gate failures are reported visibly, while dispatch can continue unless the gate explicitly defers it.
    • Gate tooling is retrieved from a fixed, verified version for more consistent workflow behavior.
    • Workflow setup guidance now supports the current access credential and a legacy fallback.
  • Documentation

    • Clarified rate-limit behavior and setup steps for enrolled workflows.

Walkthrough

The initiative-driver rate-limit gate now uses pinned tooling and reports missing-script and invocation failures. The workflow remains fail-open for dispatch through continue-on-error. Workflow and standards text document concurrency behavior and the PAT fallback. Bats tests cover gate wiring and stub parity.

Changes

Initiative-driver rate-limit gate

Layer / File(s) Summary
Gate policy and tooling pin
.github/workflows/initiative-driver.yml, standards/workflows/initiative-driver.yml, standards/agent-rate-limits.md
The workflow and standards describe throttling, concurrency cancellation, and visible gate errors. The tooling checkout uses a pinned commit SHA. Adoption instructions name GH_PAT_DON_PETRY and retain GH_PAT_WORKFLOWS as a legacy fallback.
Gate failure handling and verification
.github/workflows/initiative-driver.yml, test/workflows/initiative-driver/gate-tooling.bats
The gate step checks for the script and no longer suppresses invocation failures with `

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to cd621

The change pins the gate tooling to a reviewed commit and makes gate failures visible while dispatch stays fail-open. No concrete merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to cd621

The immutable tooling pin improves integrity and failure visibility, but the workflow still lacks repository context for reading rate-limit history, so ordinary runs are expected to remain ungated. Increased exposure was not established; production token permissions and downstream behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Compromise of executed tooling could exercise the supplied PAT's actual grants, potentially beyond one adopter repository. The fixed commit limits moving-reference substitution, but the maximum credential scope cannot be determined from workflow permissions alone.

Security Findings and Attack Paths

  • inferred — Fail-open dispatch remains an existing exposure, not an established new attack path introduced by this PR. The intended repair still lacks history-repository binding; absent or unreadable history permits dispatch rather than enforcing throttling.

Trust Boundaries and Controls

  • observed — Issue-triggered actor and tracking-issue values enter the gate through environment variables and quoted arguments. The executable repository, commit, and central dispatch destination are fixed by the workflow. Tracking-repository identity is explicitly supplied to issue mutation commands, but is not reused for history lookup.

Resilience and Maintainability Implications

  • inferred — Breaker escalation is latent in ordinary empty-history execution. If made reachable, its comment and label writes are independently best-effort, and repeated execution can duplicate comments because deduplication reads the issue body while writing the marker into a comment. This is not retained as an active PR concern because current normal execution does not reach that transition.

Hardening Proposals

  • proposed — Bind history lookup explicitly to the adopter repository and validate allow/defer behavior in a workspace containing only the nested tooling checkout. Before activating escalation, align marker lookup with its persistence location and validate repeated execution and partial comment/label failures.
🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The changes address the main coding requirements in [#1232]. The stub now uses a full commit SHA, checks for the script, reports a missing script with ::error:: and $GITHUB_STEP_SUMMARY, and remov… Provide reviewable evidence that the pinned SHA contains the gate script and that the listed fan-out PRs were regenerated from the fixed standard.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the rate-limit gate wiring defect and the missing script at ref v1. It is lengthy but remains specific and related to the main change.
Description check ✅ Passed The description explains the missing gate script, risk, tests, rollback, and monitoring. It is directly related to the changeset.
Out of Scope Changes check ✅ Passed The changed workflow, standards documentation, and Bats tests directly support [#1232]. The secret-name update and concurrency wording correction address requirements in that issue. No unrelated chang…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Full details: Linked Issues check

Explanation

The changes address the main coding requirements in [#1232]. The stub now uses a full commit SHA, checks for the script, reports a missing script with ::error:: and $GITHUB_STEP_SUMMARY, and removes || true. The header and standards text describe post-concurrency throttling and fail-open dispatch behavior. Bats tests cover these controls. The reviewed evidence does not prove that scripts/agent-rate-limit-gate.sh exists at the pinned SHA, and it does not establish that broodminder-export#145, ContentTwin#469, markets#494, TalkTerm#499, and google-app-scripts#585 were regenerated from the fixed standard.

  • 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

Comment @coderabbitai help to get the list of available commands.

@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #1234
No changes were committed, but the PR still can't be marked done: required check SonarCloud is still pending. The retry cron will re-attempt automatically. Next attempt after: 2026-10-02T17:59:24Z

@don-petry

Copy link
Copy Markdown
Contributor Author

Note

@don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check SonarCloud is still pending. I'll re-check automatically.
Next attempt after: 2026-10-02T17:59:24Z

@don-petry
don-petry enabled auto-merge (squash) October 2, 2026 17:29

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request pins the agent rate-limit gate tooling to a specific commit SHA instead of a moving tag, and adds robust error handling to fail loudly but open if the gate script is missing. It also introduces a new BATS test suite to verify these behaviors. The review feedback focuses on improving the reliability of these BATS tests by replacing generic negations and non-zero exit status checks with explicit assertions of the expected exit status, which prevents false positives from unrelated script errors.

Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
@don-petry

Copy link
Copy Markdown
Contributor Author

No description provided.

@donpetry-bot

donpetry-bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at cd6212977be19b00418253f3b48d650549c996f7 — 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: cd6212977be19b00418253f3b48d650549c996f7
Review mode: triage-approved (single reviewer)

Summary

Pins the initiative-driver gate-tooling checkout to commit SHA cd0b167 instead of the moving tag v1, which was missing the gate script. A missing gate script now fails loudly while the dispatch still runs (fail-open), and the header text is corrected. The change is correct and verified, but 4 review threads are still unresolved and one of them is a real no-op test assertion.

Linked issue analysis

Closes #1232. Acceptance criteria:

  • ✅ Script resolves at the fetched ref. Checked via the API: cd0b16751454d2eb3486ec04477d7ee9cc96c425 exists (it is current main), and all three gate files resolve at it: scripts/agent-rate-limit-gate.sh, scripts/lib/agent-rate-limit.sh, standards/agent-rate-limits.json.
  • ✅ A missing script is detected. An [ -f ] guard emits ::error::, writes a step-summary line, and exits 1. continue-on-error keeps the job running, and the dispatch != 'defer' guard stays fail-open, as the standard requires (ADR §7).
  • ✅ Tooling checkout pinned to a full SHA. The new paragraph in standards/agent-rate-limits.md §3.1 documents how to bump it.
  • ✅ Header matches behaviour. The 'AHEAD of the cancel-in-progress' claim is removed, and adoption step 3 now names GH_PAT_DON_PETRY with GH_PAT_WORKFLOWS as the fallback.
  • ⏳ Fan-out PRs regenerated. This happens after merge through standards-sync and is out of scope for this PR.

Removing || true is safe: the gate script at the pinned SHA documents that it always exits 0, with the decision passed on stdout and in $GITHUB_OUTPUT. So only a crash will now mark the step failed, which is the intended loud behaviour. The live copy and the standard stub are byte-identical (same blob 609e6c405).

Findings

Blocking (gate 4: unresolved review threads): 4 unresolved gemini-code-assist threads on test/workflows/initiative-driver/gate-tooling.bats (lines 41, 52, 56, 64). The dev-lead pass reported "no code changes were needed" but did not reply to or resolve them.

  1. Line 56 is a real no-op assertion. Fix it. ! grep -q 'decision=defer' "$tmp/out" 2>/dev/null is not the last command in the test. Bats does not treat a negated command as a failure under set -e, so this check can never fail. Replace it with run grep -q 'decision=defer' "$tmp/out" followed by [ "$status" -ne 0 ], or touch the file first and assert -eq 1.
  2. Lines 41 and 64 work as written. In both, the ! … negation is the last command in its test, so Bats uses its exit status. Reply and resolve, or convert to run …; [ "$status" -eq 1 ] for consistency.
  3. Line 52 is fine. [ "$status" -ne 0 ] is backed by the ::error:: and summary-content checks that follow. Tightening it to -eq 1 is optional. Reply and resolve.

Non-blocking nits:

  • The pin comment says main @ 2026-10-02, but commit cd0b1675 is dated 2026-09-30. This is cosmetic.
  • The gate-error path (the script crashes) relies on the runner's generic 'Process completed with exit code N' annotation, not a custom ::error::. That matches the header's 'fails loudly' claim well enough.

Security: the PAT was already handed to the fetched tooling before this PR; pinning the ref to an immutable SHA narrows that trust boundary. No new secrets, no new third-party actions, and actions/checkout stays SHA-pinned. No security concerns. The run_secret_scanning MCP tool is not available in this session, so this review did no MCP scan; the gitleaks CI check passed.

CI status

All required checks green: Lint, Lint and bats, bats (×2), ShellCheck, CodeQL / Analyze (actions, python), SonarCloud (Quality Gate passed), Secret scan (gitleaks), Agent Security Scan, AgentShield, duplicate-decl-gate, dependency-audit. The cancelled and skipped runs (dev-lead dispatch/resume/ci-relay, dependabot-automerge) are automation lanes, not checks. CodeRabbit is pending and cubic is still in progress; both are third-party advisory reviewers.


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.

@donpetry-bot

donpetry-bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at cd6212977be19b00418253f3b48d650549c996f7 — click to expand prior review.

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: cd6212977be19b00418253f3b48d650549c996f7
Review mode: triage-approved (single reviewer)

Summary

Re-review at the same head commit (cd62129), with no new commits since the cycle-1 fix request. The fix is correct: the gate-tooling checkout is pinned to SHA cd0b167, which contains all three gate files. A missing gate script now fails loudly while the dispatch stays fail-open, and the header text is corrected. However, the 4 gemini-code-assist review threads are still unresolved, and the no-op assertion at gate-tooling.bats:56 has not been fixed.

Linked issue analysis

Closes #1232. Acceptance criteria:

  • ✅ Script resolves at the fetched ref. Verified via the API at cd0b16751454d2eb3486ec04477d7ee9cc96c425: scripts/agent-rate-limit-gate.sh, scripts/lib/agent-rate-limit.sh and standards/agent-rate-limits.json all resolve.
  • ✅ A missing script is detected. An [ -f ] guard emits ::error::, writes a step-summary line, and exits 1. continue-on-error plus the dispatch != 'defer' guard keep it fail-open.
  • ✅ Tooling checkout pinned to a full SHA. The bump procedure is documented in standards/agent-rate-limits.md §3.1.
  • ✅ Header matches behaviour. The 'AHEAD of the cancel-in-progress' claim is removed, and adoption step 3 names GH_PAT_DON_PETRY with GH_PAT_WORKFLOWS as the fallback.
  • ⏳ Fan-out PRs regenerated. This happens after merge through standards-sync and is out of scope here.

Findings

Carried forward from cycle 1. Nothing was pushed, so none of it is resolved.

Blocking (gate 4: unresolved review threads): 4 unresolved gemini-code-assist threads on test/workflows/initiative-driver/gate-tooling.bats (lines 41, 52, 56, 64).

  1. Line 56 is a real no-op assertion. Fix it. ! grep -q 'decision=defer' "$tmp/out" 2>/dev/null is not the last command in the test. Bash's set -e ignores the status of a !-negated command, so this check can never fail the test. Replace it with touch "$tmp/out"; run grep -q 'decision=defer' "$tmp/out"; [ "$status" -eq 1 ].
  2. Lines 41 and 64 work as written. In both, the negated command is the last one in its test, so Bats uses its exit status. Reply and resolve, or convert to run …; [ "$status" -eq 1 ] for consistency.
  3. Line 52 is fine. [ "$status" -ne 0 ] is backed by the ::error:: and summary-content checks that follow. Tightening it to -eq 1 is optional. Reply and resolve.

Non-blocking nit: the pin comment says main @ 2026-10-02, but commit cd0b1675 is dated 2026-09-30. This is cosmetic.

Security: pinning the PAT-bearing tooling checkout to an immutable SHA narrows the trust boundary. No new secrets, no new third-party actions, and actions/checkout stays SHA-pinned. The run_secret_scanning MCP tool is not available in this session, so no MCP scan was run; the gitleaks CI check passed.

CI status

All required checks green: Lint, ShellCheck, Lint and bats, bats (×2), CodeQL (actions, python), SonarCloud (Quality Gate passed), Secret scan (gitleaks), Agent Security Scan, AgentShield, duplicate-decl-gate, and dependency-audit. The cancelled and skipped runs are automation lanes (dev-lead dispatch/resume/ci-relay, dependabot-automerge), not checks. CodeRabbit (pending) and cubic (in progress) are advisory third-party reviewers.


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.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Oct 2, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread standards/workflows/initiative-driver.yml
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
Comment thread standards/agent-rate-limits.md Outdated
Comment thread .github/workflows/initiative-driver.yml Outdated
Comment thread test/workflows/initiative-driver/gate-tooling.bats Outdated
@don-petry

Copy link
Copy Markdown
Contributor Author

No description provided.

@don-petry
don-petry disabled auto-merge October 2, 2026 17:49
@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — fix-reviews (applied)

Changes committed and pushed. Requested items addressed:

  • test/workflows/initiative-driver/gate-tooling.bats:41 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:52 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:56 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:64 — applied
  • standards/workflows/initiative-driver.yml:154 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:56 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:21 — applied
  • standards/agent-rate-limits.md:131 — applied
  • .github/workflows/initiative-driver.yml:159 — applied
  • test/workflows/initiative-driver/gate-tooling.bats:56 — applied

@don-petry
don-petry enabled auto-merge (squash) October 2, 2026 17:50
@donpetry-bot

donpetry-bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at 8b8759dcd7a449971582286dba48b9d8aee7d302 — 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: cd6212977be19b00418253f3b48d650549c996f7
Cascade: triage → audit (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

At the reviewed SHA cd62129 the gate call has no --repo, so its history lookup fails, it silently allows, and the rate-limit gate stays inert. The PR has since moved to 8b8759d, which adds --repo, a loud error path and stronger tests. I checked those fixes and they look correct from a security standpoint: the tooling pin is a signed main commit (identical to main) containing all three gate files, and the gate script accepts --repo. Escalating rather than approving because this verdict is bound to a stale SHA and every required check on the new head is still pending; a fresh cycle at 8b8759d with green CI should be approvable.

Findings

  • major: {"severity":"major","category":"correctness/safety-gate","message":"At $PR_HEAD_SHA (cd62129) the gate call omits --repo and the job has no checkout of the caller repo. argate_fetch_runs swallows the gh error, returns [], and allows, so concurrency, cooldown, daily-budget and breaker counters never trip. Commit 8b8759d fixes this by passing --repo "$ARL_TRACKING_REPO" (supported by the gate at the pinned SHA, line 413) and adds a bats test. Not approvable at the reviewed SHA.","file":"standards/workflows/initiative-driver.yml","line":154}
  • major: {"severity":"major","category":"process/ci","message":"The PR head moved from cd62129 to 8b8759d during the review cascade. Every required check at the new head (Lint, ShellCheck, bats, gitleaks, AgentShield, CodeQL, SonarCloud) is still pending, and the fix commit carries [skip ci-relay]. The gates do not pass yet.","file":null,"line":null}
  • info: {"severity":"info","category":"supply-chain","message":"Moving the tooling checkout from the tag v1 to a full SHA is a real hardening, because the org PAT is handed to that code. I verified cd0b167: GitHub-verified commit, identical to petry-projects/.github main (ahead 0, behind 0), and scripts/agent-rate-limit-gate.sh, scripts/lib/agent-rate-limit.sh and standards/agent-rate-limits.json all resolve there. persist-credentials: false is kept and the checkout action stays SHA-pinned.","file":"standards/workflows/initiative-driver.yml","line":119}
  • info: {"severity":"info","category":"secrets","message":"The gate step gets only GH_TOKEN (the PAT). AGENT_TOKEN_BUDGET_TELEMETRY_CMD and _FILE are unset, so arl_token_fetch_envelope runs nothing and no Claude OAuth token is exposed. Untrusted values (github.actor, issue number) reach the script through env, not by ${{ }} interpolation in run:, so this change adds no expression-injection surface. The ${{ github.repository }} in the dispatch step's run: is pre-existing and constrained.","file":"standards/workflows/initiative-driver.yml","line":135}
  • minor: {"severity":"minor","category":"fail-open","message":"The gate is still fail-open by design (documented in agent-rate-limits.md). The new ::error:: fires only when the script is missing or exits non-zero. Unreadable run history (e.g. an expired or under-scoped PAT) or a malformed config still returns allow with exit 0 and only a log line, so the gate can still go silently inert without a visible annotation.","file":".github/workflows/initiative-driver.yml","line":160}
  • minor: {"severity":"minor","category":"test-coverage","message":"In the missing-script test (8b8759d), touch \"$tmp/out\" followed by run grep -q 'decision=defer' is vacuous: the script exits before writing GITHUB_OUTPUT, so the file is always empty. The pin test checks a literal SHA but does not confirm the gate files exist at that SHA (I checked that out-of-band), so a future bump relies on the reviewer following agent-rate-limits.md.","file":"test/workflows/initiative-driver/gate-tooling.bats","line":56}
  • info: {"severity":"info","category":"secrets","message":"The stub still prefers a personal PAT (GH_PAT_DON_PETRY) over GH_PAT_WORKFLOWS. This PR only documents it in the adoption header, so it is not a regression, but that personal credential is now handed to the pinned tooling across every adopter repo.","file":"standards/workflows/initiative-driver.yml","line":49}

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.

@sonarqubecloud

sonarqubecloud Bot commented Oct 2, 2026

Copy link
Copy Markdown

@donpetry-bot donpetry-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 8b8759dcd7a449971582286dba48b9d8aee7d302
Review mode: triage-approved (single reviewer)

Summary

Fixes #1232. The initiative-driver stub fetched the rate-limit gate tooling at tag v1, which does not contain agent-rate-limit-gate.sh, so the gate never ran. This PR pins the tooling checkout to a full commit SHA (cd0b167), passes --repo so the gate reads the caller's run history, and swaps the silent '|| true' for a loud failure that still lets the dispatch run (::error:: annotation, step summary, step marked failed, continue-on-error keeps dispatch going). It also corrects the header, documents the pin in agent-rate-limits.md, and adds a bats suite. The live copy and the standard are byte-identical (same blob 1c7c765b2). Every finding from the prior cascade review (at cd62129) is resolved at 8b8759d, and all required checks are green.

Linked issue analysis

#1232 acceptance criteria:

  • Gate script resolves at the fetched ref: done. I confirmed that scripts/agent-rate-limit-gate.sh, scripts/lib/agent-rate-limit.sh and standards/agent-rate-limits.json all resolve at cd0b167, and that the gate parses --repo (line 413).
  • Missing script is detected: done. A [ -f ] guard emits ::error:: and a step-summary line and exits 1. Gate errors are surfaced too, through || { rc=$?; ...; exit $rc; }.
  • Tooling checkout pinned: done. It uses a full SHA, with the bump procedure documented in agent-rate-limits.md.
  • Header matches behaviour: done. The 'AHEAD of the cancel-in-progress' claim is removed, and adoption step 3 now names GH_PAT_DON_PETRY with GH_PAT_WORKFLOWS as fallback.
  • Fan-out PRs regenerated: this is post-merge standards-sync work and can't be shown in this PR. See the note under Findings.

Findings

Prior review findings (cascade at cd62129):

  • major, gate call missing --repo: resolved. 8b8759d adds --repo "$ARL_TRACKING_REPO", the pinned gate supports it, and a bats test covers it.
  • major, CI pending on the new head: resolved. Every required check (SonarCloud, CodeQL, AgentShield, Detect ecosystems) is green at 8b8759d, and so are Lint, ShellCheck, bats, gitleaks and the Agent Security Scan.
  • minor, the gate can still allow silently with exit 0 when run history can't be read: this is fail-open by design and documented. Not a regression; non-blocking.
  • minor, the decision=defer assertion in the missing-script test is weak: non-blocking test-quality nit.

New / remaining notes (non-blocking):

  • nit: The stub header says the concurrency group 'cancels a superseded run before any step (gate included) executes'. agent-rate-limits.md now says, more accurately, that it 'may cancel a superseded run after its steps have started'. The two wordings should eventually agree. This is documentation only.
  • info: The body says Closes #1232, but acceptance criterion 5 (regenerating the stalled fan-out PRs) happens after merge via standards-sync. Whoever owns #1232 may want to confirm the fan-out PRs refresh before treating it as fully done.
  • info: Security posture improves. The PAT-bearing tooling checkout moves from a moving tag to an immutable SHA. The checkout action stays SHA-pinned, persist-credentials stays false, and untrusted values reach the script through env: rather than ${{ }} interpolation in run:. No new secret surface.
  • Secret scan (MCP): the run_secret_scanning tool is not available in this environment, so it was skipped. The gitleaks CI check passed.
  • All review threads (gemini-code-assist, cubic) are resolved, and there are no unanswered human questions.

CI status

All required checks pass at 8b8759d: SonarCloud, CodeQL, agent-shield / AgentShield and dependency-audit / Detect ecosystems. Lint, ShellCheck, bats, Lint and bats, Secret scan (gitleaks), Agent Security Scan, duplicate-decl-gate, Analyze (actions/python) and the AGENTS.md self-check also pass. Two runs are still queued: dependency-audit / npm audit (not required) and pr-auto-review / check-and-dispatch, which is this review's own run.


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.

@donpetry-bot

Copy link
Copy Markdown
Contributor

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

@don-petry
don-petry merged commit 4c52221 into main Oct 2, 2026
32 of 35 checks passed
@don-petry
don-petry deleted the dev-lead/issue-1232-20261002-1725 branch October 2, 2026 18:26

@donpetry-bot donpetry-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 8b8759dcd7a449971582286dba48b9d8aee7d302
Review mode: triage-approved (single reviewer)

Summary

Confirms the triage assessment for #1232. The initiative-driver stub (canonical copy plus a byte-identical live copy) now checks out the gate tooling at a full commit SHA instead of the moving v1 tag, which did not contain the gate script. A missing script or a gate error now fails the gate step loudly (::error:: + step summary + non-zero exit) instead of being hidden by || true. continue-on-error keeps the dispatch fail-open. The step also passes --repo, and the header text is corrected. Note: the PR was already merged (2026-10-02T18:26:18Z) at this same head SHA, so this verdict is a confirmation only.

Linked issue analysis

Closes #1232. Acceptance criteria:

  • Script resolves at the fetched ref: confirmed. Pinned SHA cd0b1675… exists on main (1 commit behind main) and all three gate files resolve there: scripts/agent-rate-limit-gate.sh, scripts/lib/agent-rate-limit.sh, standards/agent-rate-limits.json.
  • Missing script is detected: confirmed. An explicit [ -f ] check emits ::error:: and exits 1, and a bats test runs the extracted step script to prove it.
  • Checkout is pinned: confirmed. It uses a full 40-char SHA, and the bump procedure is documented in standards/agent-rate-limits.md.
  • Header matches behaviour: confirmed. The claim that the gate runs AHEAD of the concurrency group is removed, and adoption step 3 now names GH_PAT_DON_PETRY.
  • Fan-out PRs regenerated: this happens through standards-sync after merge, so it is out of scope for this PR.

Findings

No blocking findings.

  • Non-blocking (docs): the stub header says the concurrency group cancels a superseded run "before any step (gate included) executes". standards/agent-rate-limits.md says it "may cancel a superseded run after its steps have started". The standards doc is the more accurate of the two, since cancel-in-progress cancels the earlier, possibly already-running, run. Consider making the wording match in a follow-up.
  • Verified: the gate script supports --repo <owner/repo> (it is passed to gh run list --repo), so the new flag is valid.
  • Verified: || { rc=$?; …; exit "$rc"; } correctly keeps the gate's exit code under set -e.
  • Security: no new secret handling. The PAT goes to first-party code at an immutable SHA, which is stronger than the moving tag it replaces. persist-credentials: false is kept, and the actions/checkout pin is unchanged.
  • All review threads (gemini, cubic) are resolved, and there are no unanswered human questions.

CI status

All required checks pass: Lint, bats, ShellCheck, CodeQL (actions/python), SonarCloud (Quality Gate passed), gitleaks, AgentShield, Agent Security Scan, and dependency-audit. Some dev-lead orchestration jobs show CANCELLED or SKIPPED; these are superseded automation runs, not test failures.


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Phase 4 gate wiring ships to adopter stubs but agent-rate-limit-gate.sh is absent at v1 — gate is inert and fail-open

2 participants