Skip to content

fix: §12 采样先跳过 in-flight 再收集已完结 head(P1-4,ADR-0034) - #120

Merged
randypanding merged 1 commit into
mainfrom
p1-4-sample-fix
Aug 20, 2026
Merged

fix: §12 采样先跳过 in-flight 再收集已完结 head(P1-4,ADR-0034)#120
randypanding merged 1 commit into
mainfrom
p1-4-sample-fix

Conversation

@randypanding

Copy link
Copy Markdown
Contributor

摘要

#85 T1 注入实测发现的采样缺陷(ADR-0034 §12 修订 2):

问题:现实现先取「最近更新前 3」再跳过 in-flight head——并发连开 PR 时(实测:另一工作流同时在 Use-up-Plan 连开 5 个测试 PR),改名注入头被挤出前 3,而 in-flight 头白白占用采样位,导致活体缺失漏检(T1 首轮实测未检出)。

修复:先跳过零-check/in-flight head,再收集最近 3 个「已完结」head 作为样本(最多扫描 10 个候选):

  • in-flight 头不再占用采样位,只计为 SKIP 依据
  • 全部候选 in-flight/零 check → 显式 SKIP(非漂移,下轮复核)
  • 无已完结 PR head 时维持既有 main-HEAD 退化与 fail-closed 语义
  • 查询失败与「已完结 head 缺 gate」照报(T1 检测对象不被稀释)

验证

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@randypanding, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Limit details: You’ve used all 10 included reviews currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e01f08e-ddd5-49b0-b095-192380c7e173

📥 Commits

Reviewing files that changed from the base of the PR and between 604a0e5 and 8f5cbe6.

📒 Files selected for processing (1)
  • governance/drift-check.sh

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

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

修复 required-check 已完结 head 采样并移除队列对账

🐞 Bug fix 🕐 20-40 Minutes

Grey Divider

AI Description

• 先过滤零检查和运行中 head,再采样最多三个已完结 head。
• 不可判定时跳过,查询失败或缺少 required check 时 fail-closed。
• 移除 §14 merge queue 期望状态与未声明队列对账。
Diagram

graph TD
  A["Drift checker"] --> B["Recent PR heads"] --> C["Check-runs API"] --> D{"CI completed?"}
  D -- "No" --> B
  D -- "Yes" --> E["Completed sample"] --> F{"Check present?"}
  F -- "Yes" --> G["Report result"]
  F -- "No" --> G
  C -- "Query failure" --> G
  A -. "Audit removed" .-> H["Merge queue"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. 单次查询并缓存候选结果
  • ➕ 避免按 required context 重复请求相同提交
  • ➕ 确保筛选与验证使用同一 API 快照
  • ➕ 可统一处理分页及查询失败
  • ➖ Shell 中缓存 JSON 会增加少量实现复杂度
  • ➖ 需要明确缓存生命周期和解析失败处理
2. 拆分 merge queue 删除
  • ➕ 保持本 PR 聚焦 §12 采样缺陷
  • ➕ 便于单独解释和评审治理覆盖变化
  • ➕ 避免意外失去 ADR-0042 漂移检测
  • ➖ 需要额外 PR 或明确的后续治理决策

Recommendation: 保留“先跳过、再收集”的总体策略,但应在合并前修正候选来源:当前 HEADS 仍在生成时截断为 [0:3],因此后续循环实际上无法扫描最多 10 个候选。建议改为提供至少 10 个 head,并缓存首次查询结果供 required-check 验证复用;同时恢复 §14,或将其删除拆分为有 ADR 依据的独立变更。

Files changed (1) +43 / -57

Bug fix (1) +43 / -57
drift-check.sh重构 required-check 采样并删除 merge queue 对账 +43/-57

重构 required-check 采样并删除 merge queue 对账

• §12 先过滤零 check 和 in-flight head,再使用已完结样本验证 required contexts,并保留 SKIP 与 fail-closed 输出。文件还删除了 §14 的 merge queue 参数一致性及未声明队列检测。

governance/drift-check.sh

@randypanding
randypanding merged commit d001518 into main Aug 20, 2026
7 checks passed
@randypanding
randypanding deleted the p1-4-sample-fix branch August 20, 2026 06:31
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Candidate scan remains capped 🐞 Bug ≡ Correctness
Description
The new loop can never scan up to 10 candidates because HEADS is still truncated to the first
three eligible PRs before the loop. If those three are in-flight or have zero checks, the script
emits SKIP without examining later completed heads, preserving the sampling defect this PR intends
to fix.
Code

governance/drift-check.sh[R486-487]

+    { [[ $N_CONCL -ge $COLLECT_N ]] || [[ $N_SEEN -ge 10 ]]; } && break
+    N_SEEN=$((N_SEEN+1))
Relevance

●●● Strong

The stated PR intent explicitly requires scanning up to ten candidates; slicing HEADS to three makes
later candidates unreachable.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
HEADS is sliced to [0:3], while the added collector claims and attempts to scan up to 10. Since
the loop only consumes HEADS, candidates four through ten are unreachable, and discarded
first-three heads lead directly to the new SKIP branch.

governance/drift-check.sh[462-469]
governance/drift-check.sh[475-502]
governance/drift-check.sh[503-509]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The completed-head collector has a limit of 10, but its input contains at most three heads. Consequently, skipped in-flight or zero-check heads cannot be replaced by later completed candidates.

## Issue Context
Build `HEADS` from enough recent eligible PRs for the collector to inspect up to 10 candidates, while preserving the default-branch fallback when no eligible PR activity exists.

## Fix Focus Areas
- governance/drift-check.sh[462-469]
- governance/drift-check.sh[484-502]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Merge-queue drift check removed 🐞 Bug ≡ Correctness
Description
Deleting §14 removes the only reconciliation of the repository-level merge-queue rulesets declared
in expected state. The scheduled checker can now report no drift when a queue is deleted or its
merge method, limits, timeout, grouping strategy, or required gate changes.
Code

governance/drift-check.sh[L558-560]

-# ---------- 14. merge queue 对账(P2-7,ADR-0042;repo 级 ruleset)----------
-# org rulesets API 不支持 merge_queue 规则类型(实测 422)——merge queue 只能以
-# repo 级 ruleset 存在(须与 pull_request/required_status_checks 同集)。期望状态
Relevance

●●● Strong

Removing the only repository-level merge-queue reconciliation contradicts declared expected state
and eliminates drift detection.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Expected state still declares merge queues for two repositories and their exact parameters. The
apply script still creates those repository-level rulesets and explicitly delegates parameter
enforcement to drift-check §14, while the remaining generic ruleset check only queries
organization-level rulesets; the scheduled workflow relies on this script for detection.

governance/expected-state.json[83-97]
governance/apply.sh[143-154]
governance/drift-check.sh[24-50]
.github/workflows/governance-drift.yml[34-48]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR deletes the only drift detection for active repository-level merge-queue rulesets, despite those rulesets remaining declared and applied elsewhere.

## Issue Context
Restore §14 or provide equivalent repository-ruleset reconciliation. It must validate declared repositories and parameters, detect undeclared merge queues, and fail closed on API or parsing failures.

## Fix Focus Areas
- governance/drift-check.sh[577-585]
- governance/expected-state.json[83-97]
- governance/apply.sh[143-154]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Query failures become missing checks 🐞 Bug ☼ Reliability
Description
The PR removes response validation from the per-context check-runs lookup, so an API error or
malformed response simply leaves FOUND=0. With QUERY_FAIL unchanged, the script falsely reports
a missing required check instead of identifying an unverifiable query.
Code

governance/drift-check.sh[L481-482]

-      if ! jq -e 'type == "object" and has("check_runs")' <<<"$CRS" >/dev/null 2>&1; then
-        QUERY_FAIL=1; continue
Relevance

●●● Strong

PR #19 accepted shape validation and fail-closed handling for malformed GitHub API responses in this
script.

PR-#19

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The initial lookup explicitly verifies an object with check_runs and records failure, but the
later lookup passes its response directly to jq. The subsequent branch only reports query failure
when the earlier lookup happened to set QUERY_FAIL, so failures from the later request are
mislabeled; past PR #19 establishes the same requirement to validate GitHub API response shapes
fail-closed in this script.

governance/drift-check.sh[488-491]
governance/drift-check.sh[512-525]
PR-#19

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The second check-runs request is not validated, even though the sampling request validates the same endpoint. Failed requests are therefore misclassified as missing required checks.

## Issue Context
Prefer caching each validated response during candidate collection and reusing it for context checks. If requests remain in the inner loop, validate every response and set the query-failure state before evaluating check names.

## Fix Focus Areas
- governance/drift-check.sh[488-501]
- governance/drift-check.sh[512-525]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. Broad token authenticates api 📘 Rule violation ⛨ Security
Description
The added check-runs request uses the shared api helper, which authenticates with the generic
org-admin GH_TOKEN instead of a single-repository token issued by scripts/gh-app-token.sh. This
violates the required GitHub App identity and token-scoping policy.
Code

governance/drift-check.sh[488]

+    CRS=$(api "https://api.github.com/repos/$ORG/$r/commits/$sha/check-runs?per_page=100")
Relevance

● Weak

PR #49 rejected the closely matching objection that new drift-check API calls must use a separate
scoped token.

PR-#49

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2778539 requires GitHub API automation to use cloudbrid-agent credentials generated by
scripts/gh-app-token.sh and restricted to one repository. The changed request at
governance/drift-check.sh[488-488] calls an api helper that uses GH_TOKEN, while the workflow
populates that variable directly from the broad GOVERNANCE_TOKEN secret.

Rule 2778539: Agent operations must use the cloudbrid-agent GitHub App identity via scripts/gh-app-token.sh with single-repo tokens
governance/drift-check.sh[7-22]
governance/drift-check.sh[484-490]
.github/workflows/governance-drift.yml[34-48]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added GitHub check-runs API request is authenticated using the broad `GOVERNANCE_TOKEN` supplied through the shared `api` helper, rather than a single-repository cloudbrid-agent GitHub App token obtained from `scripts/gh-app-token.sh`.

## Issue Context
PR Compliance ID 2778539 requires agent automation to invoke `scripts/gh-app-token.sh` and scope generated credentials to the specific repository being queried. Update the per-repository request flow and its workflow credential setup without exposing tokens in logs.

## Fix Focus Areas
- governance/drift-check.sh[7-20]
- governance/drift-check.sh[484-490]
- .github/workflows/governance-drift.yml[34-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 12 rules
Review mode: ⚖️ Balanced: This changes runtime governance logic for API-driven required-check liveness, including sampling, fail-closed behavior, and SKIP/drift decisions; it is localized but behaviorally consequential and not clearly trivial.
ⓘ  3 issues published inline · 4 in summary

Grey Divider

Tip of the day
💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread governance/drift-check.sh
Comment on lines +486 to +487
{ [[ $N_CONCL -ge $COLLECT_N ]] || [[ $N_SEEN -ge 10 ]]; } && break
N_SEEN=$((N_SEEN+1))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Candidate scan remains capped 🐞 Bug ≡ Correctness

The new loop can never scan up to 10 candidates because HEADS is still truncated to the first
three eligible PRs before the loop. If those three are in-flight or have zero checks, the script
emits SKIP without examining later completed heads, preserving the sampling defect this PR intends
to fix.
Agent Prompt
## Issue description
The completed-head collector has a limit of 10, but its input contains at most three heads. Consequently, skipped in-flight or zero-check heads cannot be replaced by later completed candidates.

## Issue Context
Build `HEADS` from enough recent eligible PRs for the collector to inspect up to 10 candidates, while preserving the default-branch fallback when no eligible PR activity exists.

## Fix Focus Areas
- governance/drift-check.sh[462-469]
- governance/drift-check.sh[484-502]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread governance/drift-check.sh
Comment on lines -481 to -482
if ! jq -e 'type == "object" and has("check_runs")' <<<"$CRS" >/dev/null 2>&1; then
QUERY_FAIL=1; continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

3. Query failures become missing checks 🐞 Bug ☼ Reliability

The PR removes response validation from the per-context check-runs lookup, so an API error or
malformed response simply leaves FOUND=0. With QUERY_FAIL unchanged, the script falsely reports
a missing required check instead of identifying an unverifiable query.
Agent Prompt
## Issue description
The second check-runs request is not validated, even though the sampling request validates the same endpoint. Failed requests are therefore misclassified as missing required checks.

## Issue Context
Prefer caching each validated response during candidate collection and reusing it for context checks. If requests remain in the inner loop, validate every response and set the query-failure state before evaluating check names.

## Fix Focus Areas
- governance/drift-check.sh[488-501]
- governance/drift-check.sh[512-525]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread governance/drift-check.sh
Comment on lines -558 to -560
# ---------- 14. merge queue 对账(P2-7,ADR-0042;repo 级 ruleset)----------
# org rulesets API 不支持 merge_queue 规则类型(实测 422)——merge queue 只能以
# repo 级 ruleset 存在(须与 pull_request/required_status_checks 同集)。期望状态

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

4. Merge-queue drift check removed 🐞 Bug ≡ Correctness

Deleting §14 removes the only reconciliation of the repository-level merge-queue rulesets declared
in expected state. The scheduled checker can now report no drift when a queue is deleted or its
merge method, limits, timeout, grouping strategy, or required gate changes.
Agent Prompt
## Issue description
The PR deletes the only drift detection for active repository-level merge-queue rulesets, despite those rulesets remaining declared and applied elsewhere.

## Issue Context
Restore §14 or provide equivalent repository-ruleset reconciliation. It must validate declared repositories and parameters, detect undeclared merge queues, and fail closed on API or parsing failures.

## Fix Focus Areas
- governance/drift-check.sh[577-585]
- governance/expected-state.json[83-97]
- governance/apply.sh[143-154]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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.

1 participant