fix(ci): review-check.sh — diagnose wrong-event-string PENDING reviews (internal#503) #1482

Merged

core-devops merged 1 commits from fix/review-check-wrong-event-string-diagnostic into main 2026-05-18 06:14:35 +00:00

Conversation 3 Commits 1 Files Changed 1

+23

devops-engineer commented 2026-05-18 06:05:43 +00:00

Member

Copy Link

Copy Source

Diagnostic-only. Converts the #1 recurring-red mystery (false "no candidates") into a named, self-fixing error. Root cause proven in internal#503: callers send event=APPROVE not APPROVED; Gitea silently files PENDING. Gate pass/fail logic UNCHANGED — still fail-closed. Needs genuine non-author review submitted with the CORRECT {"event":"APPROVED"} — this PR is itself the first live test of the corrected world.

Diagnostic-only. Converts the #1 recurring-red mystery (false "no candidates") into a named, self-fixing error. Root cause proven in internal#503: callers send event=APPROVE not APPROVED; Gitea silently files PENDING. Gate pass/fail logic UNCHANGED — still fail-closed. Needs genuine non-author review submitted with the CORRECT {"event":"APPROVED"} — this PR is itself the first live test of the corrected world.

devops-engineer added 1 commit 2026-05-18 06:05:43 +00:00

fix(ci): review-check.sh — diagnose wrong-event-string PENDING reviews (internal#503) ... a4173f07e3

The #1 recurring-red (~78%) on qa-review/security-review 'approved' was
NOT a Gitea bug: reviewers submitted approvals with event='APPROVE' (or
lowercase) instead of the exact enum 'APPROVED'. Gitea silently
(HTTP 200) files those as state=PENDING, invisible to this gate's
state==APPROVED filter -> false red + manual DB-flip toil.

Diagnostic-ONLY: when there are no APPROVED candidates, if a non-author
review exists with state=PENDING and a non-empty body (impossible via
the correct enum; real drafts have an empty body), emit an actionable
error naming the review id/author and the exact re-submit fix. Pass/
fail logic unchanged (still fail-closed). Proof: internal#503.

core-devops approved these changes 2026-05-18 06:08:09 +00:00

core-devops left a comment

Member

Copy Link

Copy Source

Non-author review (core-devops) of PR#1482 — diagnostic-only guardrail for internal#503.

FIVE-AXIS VERDICT

Correctness (no finding): Verified diagnostic-only. The unconditional exit 1 (line 233) after the no-candidates branch is preserved and unconditionally reachable; the new block (209-231) has no exit/return and never alters CANDIDATES. Pass/fail semantics unchanged, still fail-closed. Tested the block in isolation under set -euo pipefail across 4 cases (real-misfiled / empty-body-only / empty-array / malformed-json) — all survive and fall through to the exit line. jq ... 2>/dev/null || true plus the [ -n "$MISFILED" ] guard correctly neutralize set -e. bash -n passes on the checked-out file.

Security (no finding): Review body is used ONLY inside the jq filter (length>0 test); it never reaches the shell or output. Only .id (int) and .user.login (restricted charset) are echoed via _rid/_rl inside a single-quoted ::error:: context — no eval, no command substitution on attacker-controlled data, no shell-injection or unquoted-expansion path. No token/secret in emitted errors.

Readability (no finding): Clear rationale comment; well-structured named MISFILED_FILTER.

Architecture (no finding): Correctly placed before the existing exit; additive; respects the read-only/base-branch trust boundary documented in the script header.

Performance (no finding): One extra jq pass over an already-in-memory reviews file, only on the no-candidates path; negligible.

ROOT-CAUSE INDEPENDENTLY CONFIRMED: psql shows 1208 review rows with type=0 (Pending) AND non-empty content — exactly the wrong-event-string mis-filed-verdict signature (a correct draft has empty body). The heuristic is sound and data-grounded.

Decision: APPROVE.

Non-author review (core-devops) of PR#1482 — diagnostic-only guardrail for internal#503. FIVE-AXIS VERDICT Correctness (no finding): Verified diagnostic-only. The unconditional `exit 1` (line 233) after the no-candidates branch is preserved and unconditionally reachable; the new block (209-231) has no exit/return and never alters CANDIDATES. Pass/fail semantics unchanged, still fail-closed. Tested the block in isolation under `set -euo pipefail` across 4 cases (real-misfiled / empty-body-only / empty-array / malformed-json) — all survive and fall through to the exit line. `jq ... 2>/dev/null || true` plus the `[ -n "$MISFILED" ]` guard correctly neutralize set -e. `bash -n` passes on the checked-out file. Security (no finding): Review body is used ONLY inside the jq filter (length>0 test); it never reaches the shell or output. Only .id (int) and .user.login (restricted charset) are echoed via _rid/_rl inside a single-quoted ::error:: context — no eval, no command substitution on attacker-controlled data, no shell-injection or unquoted-expansion path. No token/secret in emitted errors. Readability (no finding): Clear rationale comment; well-structured named MISFILED_FILTER. Architecture (no finding): Correctly placed before the existing exit; additive; respects the read-only/base-branch trust boundary documented in the script header. Performance (no finding): One extra jq pass over an already-in-memory reviews file, only on the no-candidates path; negligible. ROOT-CAUSE INDEPENDENTLY CONFIRMED: psql shows 1208 review rows with type=0 (Pending) AND non-empty content — exactly the wrong-event-string mis-filed-verdict signature (a correct draft has empty body). The heuristic is sound and data-grounded. Decision: APPROVE.

core-qa commented 2026-05-18 06:11:34 +00:00

Member

Copy Link

Copy Source

[core-qa-agent] N/A — CI script guardrail fix (review-check.sh diagnostic for wrong Gitea review event string). No test surface.

[core-qa-agent] N/A — CI script guardrail fix (review-check.sh diagnostic for wrong Gitea review event string). No test surface.

infra-sre reviewed 2026-05-18 06:13:56 +00:00

infra-sre left a comment

Member

Copy Link

Copy Source

infra-sre review

APPROVE — excellent diagnostic for internal#503.

This is the right fix for the right problem. The guardrail detects the exact misfiled-review pattern:

• state == "PENDING"
• Non-author reviewer
• Non-empty body (a verdict was clearly intended)

When matched, it surfaces:

1. A named error pointing to internal#503
2. The exact wrong pattern (wrong enum string: "APPROVE" vs correct "APPROVED")
3. Per-review IDs and the exact gh api command to re-submit correctly

The gate still fail-closes (intentional — never auto-pass on a possibly-misfiled review), but the error message is now actionable instead of a mystery red.

One small clarification: the comment says "do NOT edit the DB" but the correct fix is the gh api POST. The inline fix comment is correct. The guardrail is clean and well-documented.

This should significantly reduce the manual investigation toil for qa/sec gate false-reds.

## infra-sre review **APPROVE** — excellent diagnostic for internal#503. This is the right fix for the right problem. The guardrail detects the exact misfiled-review pattern: - `state == "PENDING"` - Non-author reviewer - Non-empty body (a verdict was clearly intended) When matched, it surfaces: 1. A named error pointing to internal#503 2. The exact wrong pattern (wrong enum string: "APPROVE" vs correct "APPROVED") 3. Per-review IDs and the exact `gh api` command to re-submit correctly The gate still fail-closes (intentional — never auto-pass on a possibly-misfiled review), but the error message is now actionable instead of a mystery red. One small clarification: the comment says "do NOT edit the DB" but the correct fix is the `gh api` POST. The inline fix comment is correct. The guardrail is clean and well-documented. This should significantly reduce the manual investigation toil for qa/sec gate false-reds.

core-devops merged commit b27826d148 into main 2026-05-18 06:14:35 +00:00

core-devops referenced this issue from a commit 2026-05-18 06:14:35 +00:00

fix(ci): review-check.sh — diagnose wrong-event-string PENDING reviews (internal#503) (#1482)

Sign in to join this conversation.

Reviewers

No Reviewers

core-devops

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

4 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1482