-
Notifications
You must be signed in to change notification settings - Fork 0
substrate: requiredApprovingReviewCount=0 calibration + always-double-check-threads-after-CI (Aaron 2026-04-28) #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
345e4fc
1aa3775
d62514c
6b3dbbb
003b697
eae3a7f
f81e100
6fb1c75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,333 @@ | ||||||
| --- | ||||||
| name: >- | ||||||
| BOTH FORKS — requiredApprovingReviewCount=0 on Zeta; BLOCKED never | ||||||
| means "waiting for reviewer approval"; the only blockers are | ||||||
| unresolved review threads, failing/pending status checks, or merge | ||||||
| conflicts; this is a CALIBRATION CONSTANT for the project's branch | ||||||
| protection ruleset on AceHack/Zeta and Lucent-Financial-Group/Zeta | ||||||
| — verified empirically via gh api graphql against | ||||||
| branchProtectionRule 2026-04-28; Aaron 2026-04-28 caught me | ||||||
| parroting "blocked on reviewer approval" multiple times in this | ||||||
| session despite zero approvals being required | ||||||
| description: >- | ||||||
| Aaron 2026-04-28 input after I claimed LFG #660 was "BLOCKED | ||||||
| awaiting reviewer" — he asked *"are you sure, it's not something | ||||||
| simple you can figure out?"* prompting me to actually query the | ||||||
| branch-protection rule via GraphQL. Result — | ||||||
| requiredApprovingReviewCount=0 on origin/main (LFG). The same is | ||||||
| true on AceHack/main. **No approval is required to merge on this | ||||||
| project — neither fork.** A mergeStateStatus=BLOCKED with green CI | ||||||
| on Zeta MUST mean one or more of: (1) unresolved review threads | ||||||
| (requiresConversationResolution=true), (2) pending or failing | ||||||
| status checks in the required list, (3) merge conflicts. NEVER | ||||||
| means "waiting for human reviewer approval" — there is no | ||||||
| human-reviewer-approval gate configured. I made this same | ||||||
| misdiagnosis multiple times in this session despite Otto-355 | ||||||
| (BLOCKED-investigate-threads-first) being a wake-time discipline. | ||||||
| Aaron explicit ask — save somewhere that | ||||||
| requiredApprovingReviewCount=0 on this project — this memory IS | ||||||
| that durable reminder, indexed in MEMORY.md so fresh sessions hit | ||||||
| it before falling into the same misdiagnosis. | ||||||
| type: project | ||||||
| --- | ||||||
|
|
||||||
| # Calibration constant — `requiredApprovingReviewCount: 0` on Zeta | ||||||
|
|
||||||
| ## The constant (verified 2026-04-28) | ||||||
|
|
||||||
| Both forks of Zeta have `requiredApprovingReviewCount: 0` configured | ||||||
| on the `main` branch protection ruleset: | ||||||
|
|
||||||
| - `https://github.com/AceHack/Zeta` — main | ||||||
| - `https://github.com/Lucent-Financial-Group/Zeta` — main | ||||||
|
|
||||||
| **No human reviewer approval is required to merge any PR.** The | ||||||
| `requiresApprovingReviews: true` flag is on (so the review system is | ||||||
| *enabled*) but the *count* required is zero — meaning a PR can merge | ||||||
| with zero approving reviews as long as the other gates clear. | ||||||
|
|
||||||
| ## What `mergeStateStatus: BLOCKED` actually means on Zeta | ||||||
|
|
||||||
| When the GitHub API reports `mergeStateStatus: BLOCKED` on a Zeta PR, | ||||||
| the blocker is **one OR MORE** of these three classes (they CAN | ||||||
| coexist — e.g., a PR can simultaneously have unresolved threads AND | ||||||
| pending CI; fixing only one class won't unblock the merge until ALL | ||||||
| classes are clear; the diagnostic playbook below MUST check all three | ||||||
| before declaring the diagnosis exhausted): | ||||||
|
|
||||||
|
AceHack marked this conversation as resolved.
|
||||||
| 1. **Unresolved review threads.** `requiresConversationResolution: true` | ||||||
| is set on both forks. Even ONE unresolved thread blocks merge. Even | ||||||
| if the thread is `isOutdated: true` after a force-push, it still | ||||||
| blocks until explicitly resolved (per | ||||||
| `feedback_outdated_review_threads_block_merge_resolve_explicitly_after_force_push_2026_04_27.md`). | ||||||
|
|
||||||
| 2. **Failing or pending required status checks.** The required-context | ||||||
| list on both forks includes: | ||||||
| - `lint (semgrep)` | ||||||
| - `lint (shellcheck)` | ||||||
| - `lint (actionlint)` | ||||||
| - `lint (markdownlint)` | ||||||
| - `build-and-test (macos-26)` | ||||||
| - `build-and-test (ubuntu-24.04)` | ||||||
| - `build-and-test (ubuntu-24.04-arm)` | ||||||
|
|
||||||
| Any of these in `FAILURE`, `IN_PROGRESS`, or `QUEUED` blocks merge. | ||||||
|
|
||||||
| 3. **Merge conflicts** with the base branch. Surfaces as | ||||||
| `mergeable: CONFLICTING` in the same API response and as `DIRTY` | ||||||
| in `mergeStateStatus`. | ||||||
|
|
||||||
| ## What BLOCKED does NOT mean on Zeta | ||||||
|
|
||||||
| **It does NOT mean "waiting for a human reviewer to approve."** | ||||||
|
|
||||||
| There is NO human-reviewer-approval gate configured. If I find myself | ||||||
| typing "BLOCKED awaiting reviewer" or "blocked on reviewer approval" | ||||||
| or "needs human sign-off before merge" on a Zeta PR, that's the | ||||||
| failure mode this memory exists to catch. | ||||||
|
|
||||||
| ## How to verify the actual blocker (correct diagnostic command) | ||||||
|
|
||||||
| ```bash | ||||||
| gh api graphql --field query='query { | ||||||
| repository(owner:"<owner>",name:"Zeta"){ | ||||||
| pullRequest(number:<N>){ | ||||||
| mergeStateStatus | ||||||
| mergeable | ||||||
| reviewDecision | ||||||
| reviewThreads(first:100){pageInfo{hasNextPage} nodes{isResolved}} | ||||||
| commits(last:1){nodes{commit{statusCheckRollup{state contexts(first:100){pageInfo{hasNextPage} nodes{__typename ... on CheckRun{name conclusion status} ... on StatusContext{context state}}}}}}} | ||||||
| } | ||||||
|
AceHack marked this conversation as resolved.
|
||||||
| } | ||||||
| }' | ||||||
| ``` | ||||||
|
|
||||||
| Then check, in order: | ||||||
|
|
||||||
| 1. Are any threads `isResolved: false`? If yes — that's the blocker. | ||||||
| 2. Are any required status checks `FAILURE` / `IN_PROGRESS` / `QUEUED`? | ||||||
| If yes — that's the blocker. | ||||||
| 3. Is `mergeable: CONFLICTING`? If yes — rebase needed. | ||||||
| 4. Are 1-3 all clear and BLOCKED still shows? Then check the branch- | ||||||
|
||||||
| 4. Are 1-3 all clear and BLOCKED still shows? Then check the branch- | |
| 6. Are 1-5 all clear and BLOCKED still shows? Then check the branch- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The frontmatter/summary says BLOCKED with green CI “MUST mean” only 3 classes (threads / status checks / conflicts), but the body immediately defines FIVE classes (adds “required check missing from rollup” and “repository ruleset gates”). This is internally inconsistent and will mis-train future readers; update the frontmatter wording (avoid “only blockers”) or expand the summary taxonomy to match the 5-class list.