fix(source-control): narrow review-trigger gate contract to pending

[kyle-sexton]

This was generated by AI during autonomous /work-items:work-loop execution.

Closes #324

Summary

reference/review-trigger.md documented the review-trigger signal as <review-gate-context>
"pending or failing", while every gate_state comparison in the engine accepts only pending
— the candidate predicate and request_signal_pending in babysit_review_trigger.py, and the
"awaiting requested review" material signal in babysit_delta.py. #324 called this a genuine
design fork and explicitly forbade resolving it by inference.

Decision: honor the code, narrow the doc. Documentation and test only; no behavior change.

The empirical tiebreaker #324 asked for

#324 named the deciding fact: does the <review-gate-context> the cutover config wires report
FAILING or PENDING when no reviewer has engaged? That fact was located, and it settles the fork
in favor of the code:

• The wired context name is codex-review — babysit_review_gate_context under
  pluginConfigs["source-control@melodic-software"] in the dotfiles chezmoi data
  (melodic-software/dotfiles, .chezmoidata/claude.json).
• The workflow that posts that context is melodic-software/medley,
  .github/workflows/comment-review-gate.yml. Every status it writes is either state: "success"
  (reviewer engaged) or state: "pending" (not engaged). It never posts failure or error:
  the file's only four state: assignments are three success and one pending.
• Its header states the contract directly — "If engaged → 'success' (review received, covers this
  push) … If not → 'pending'", and "its status stays 'pending' until the next push re-checks."

So "no reviewer engaged yet" is PENDING, never FAILING. The honor-doc branch — widening the
code so a failing gate becomes a trigger candidate — would have been correct only if the gate
reported FAILING for that state, and it does not. review-trigger.md's "or failing" was the
erroneous restatement; the code was right.

Scope note for the reviewer: the gate is deployed in medley, not in this repository — recent
claude-code-plugins PRs carry no StatusContext at all, so the evidence above comes from the
config that names the context plus the workflow that posts it, not from a live status on this PR.

Corroborating repo-internal evidence

Three signals inside this repository point the same way, independently of the gate observation:

1. review-trigger.md's own Engagement Gate Semantics section — the mechanism-level definition of
   the gate's vocabulary — defines exactly two states (PENDING = no qualifying reviewer activity
   after the polling window, SUCCESS = may reflect activity from an earlier head) and gives
   FAILING no engagement meaning anywhere in the file. The "pending or failing" phrasing appears
   only in the downstream restatement of the helper's preconditions, one paragraph below a
   separate and correct rule about never discarding "a pending or failed StatusContext" — the
   likelier provenance of the phrase than authored intent to include failing gates.
2. A failing analogue of request_signal_pending (effective_state == "PENDING" and not target_url) would require knowing what target_url a failing gate carries. Nothing records
   that, so honoring the doc would have meant inventing a signal shape inside logic that posts
   comments to GitHub.
3. The motivating harm does not exist. classify_checks buckets the gate StatusContext like any
   other check, so a failing gate already raises babysit_delta's "N failing check(s)" blocker. A
   failing gate is escalated to a human, not silently dropped — the coherent reading of the split:
   an unengaged reviewer is nudged automatically, a broken gate is not.

Changes

• reference/review-trigger.md — the trigger precondition now reads pending, and Engagement
  Gate Semantics states the failing case once (a failing gate is not an engagement signal, is never
  a trigger candidate, and reaches the operator through the ordinary failing-check blocker), so the
  contract is complete rather than merely non-contradictory.
• test_review_trigger_race.py — a regression class pinning the chosen behavior. The rollup
  makes the gate StatusContext the only non-success check and asserts ci_gateway_green and
  non_review_checks_green are both true, so the failing gate is the isolated variable rather than
  one of several reasons request_eligible could be false. It also pins the else-branch
  pass-through (state == "failing") and the failing-check bucketing from point 3 above.
• No code change. All three sites named in fix(source-control): reconcile review-trigger 'pending or failing' gate contract with pending-only code #324, plus pr_queue_snapshot.py, already read
  pending and are correct under this decision; a grep of every gate_state comparison confirms
  the enumeration is complete.
• Plugin version bump to 0.26.9 with the matching CHANGELOG entry.

If you want to override

This is the reversible branch. Should the deployed gate ever be rewired to report FAILING (not
PENDING) for "no reviewer engaged yet", the fix is to widen the candidate predicate and define a
failing analogue of request_signal_pending with the observed target_url shape in hand — and the
regression class added here is what would need inverting.

Test plan

Run locally in the worktree:

• python -m unittest discover over plugins/source-control/skills/babysit-prs/scripts/tests —
  375 tests, OK after integrating main (includes the 3 new regression tests).
• scripts/validate-plugins.sh — all plugin manifests and the catalog validated.
• scripts/check-changelog-parity.sh --check and --check-bump origin/main — both pass.
• markdownlint-cli2 on both changed markdown files — 0 errors.

Left to required CI: scripts/check-changed-skills.sh and scripts/check-skill-portability.sh,
which run in the skill-quality-gate and portability-lint jobs respectively.

Related

Refs #322 — PR-B (capability convergence); Codex flagged this doc/code fork inline there.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 3m 30s —— View job

---

PR Review — fix(source-control): narrow review-trigger gate contract to pending

• Load tools and gather context
• Review PR diff and changed files
• Validate documentation changes in review-trigger.md
• Validate new regression tests in test_review_trigger_race.py
• Check plugin version bump and CHANGELOG
• Synthesize and post review findings

---

Summary

This is a well-scoped, documentation-and-test-only fix. The reasoning in the PR body is thorough and the empirical evidence is sound. The code changes are all correct. One cosmetic note follows; everything else is ready to merge.

---

reference/review-trigger.md

Both changes are precise and correctly placed.

Engagement Gate Semantics (line 29–31): The new sentence — "A failing gate is not an engagement signal: it is never a trigger candidate, and reaches the operator through the ordinary failing-check blocker" — is placed immediately after the PENDING/SUCCESS definitions. This is the right location: it completes the state vocabulary defined there, not just a downstream restatement.

Requesting A Review preconditions (line 60): Changing "pending or failing" to "pending" is correct and verified. Every gate_state comparison in babysit_review_trigger.py is == "pending" (line 421). The "or failing" was a stale restatement; the code was correct.

The pre-existing "Never discard a pending or failed StatusContext" sentence (line 25) is about a different concern — not replacing a StatusContext with a same-name CheckRun — and is unrelated to engagement candidacy. The two uses of "failing/failed" in the document are now clearly distinct and non-contradictory. ✓

---

test_review_trigger_race.py — FailingGateIsNotAnEngagementSignalTests

Traced all three test assertions against the implementation:

test_failing_gate_is_not_a_pending_request_signal — The rollup's "state": "FAILURE" StatusContext maps to effective_state = "FAILURE" → category = "failing" (via check_category in babysit_checks.py). summarized_state returns "failing". The gate is excluded from non_review_checks by the filter in review_gate_state, leaving only the SUCCESS ci-gate, so non_review_checks_green = True. request_signal_pending requires effective_state == "PENDING", which is false. All assertions are correct. ✓

test_failing_gate_never_becomes_a_candidate — candidate is False because gate_state == "pending" fails. In classify_review_request's state cascade, none of the earlier branches match (no reviews, reactions, history), so it falls to the else: state = check_state["gate_state"] = "failing". missing_head_sha is "" because it's only set when candidate is True. All assertions are correct. ✓

test_failing_gate_surfaces_as_an_ordinary_failing_check — classify_checks stores check["name"] for failing checks (not state). The StatusContext has no "name" key, so normalize_check falls back to check.get("context") = "review-gate". classified["failing"] == ["review-gate"] is correct. ✓

The isolation setup is well-designed: making the gate StatusContext the only non-success check, while excluding it from non_review_checks, correctly isolates gate_state as the single variable determining request_eligible. The comment in _rollup() accurately describes this.

---

plugin.json and CHANGELOG.md

The version is consistently bumped to 0.26.5 in both files. The CHANGELOG entry accurately describes the change.

Cosmetic note: The PR body says "Plugin version bump 0.26.3 → 0.26.4". The actual diff is 0.26.4 → 0.26.5 — consistent with 2221a4e having already landed 0.26.4 on main between the PR's initial authoring and the two origin/main merge commits. The code is correct; only the PR description is stale.

---

Verdict

Approved. Documentation and tests are correct, well-isolated, and cover the stated contract completely. No behavior change, no security surface, no issues requiring action.

[claude]

Claude finished @kyle-sexton's task in 1m 51s —— View job

---

Security Review — PR #1392

• Gather PR diff and changed files
• Analyze test_review_trigger_race.py for security issues
• Analyze review-trigger.md for sensitive content / contract issues
• Analyze plugin.json and CHANGELOG.md
• Post findings

---

No security issues found.

This PR is documentation + regression tests only; no production logic is changed. Here is the full rationale by surface:

test_review_trigger_race.py — new FailingGateIsNotAnEngagementSignalTests class

• Test data is fully hardcoded (no external input, no injection surface).
• sys.path.insert(0, ...) is the established pattern in this test suite and runs only in the test process.
• mock.patch.object is correctly scoped to module attributes; no global resolver manipulation.
• The "state": "FAILURE" fixture correctly reaches check_category("FAILURE") == "failing" through normalize_check (line 61 of babysit_checks.py: context_state = str(check.get("state") or "").upper()), then CHECK_FAILURE_STATES (line 21–29), then summarized_state → "failing". The data flow is sound.
• ci_gateway_green is correctly True in the rollup because the ci-gate CheckRun carries conclusion: "SUCCESS", and non_review_checks_green is correctly True because the failing gate StatusContext is excluded from non_review_checks by the existing filter at line 276–284 of babysit_review_trigger.py — the gate's failure surfaces through the ordinary failing-check bucket instead, which is exactly what the third test asserts.
• The review_trigger_allowed=True parameter in test_failing_gate_never_becomes_a_candidate confirms the mutation-policy trust boundary is still required to reach candidacy; the request_eligible: false assertion is the meaningful claim, not an accident of a missing authorization flag.

review-trigger.md — prose-only; no executable content, no secrets, no credentialed references.

plugin.json — version bump 0.26.4 → 0.26.5; no field additions, no schema changes, no new capabilities declared.

CHANGELOG.md — prose-only.

No GitHub Actions files changed. No dependency pins touched. No secret or token handling introduced or modified.

[claude]

Claude finished @kyle-sexton's task in 2m 15s —— View job

---

PR Review — fix(source-control): narrow review-trigger gate contract to pending

• Gather PR diff and changed files
• Review review-trigger.md documentation changes
• Review test_review_trigger_race.py regression tests
• Review plugin.json and CHANGELOG.md
• Synthesize and post findings

---

Summary

Documentation-and-test-only fix. The reasoning is sound and the empirical chain is complete. All changes are correct. One stale line in the PR description is worth noting; the code itself is ready to merge.

---

reference/review-trigger.md

Engagement Gate Semantics (lines 29–31): The new sentence — "A failing gate is not an engagement signal: it is never a trigger candidate, and reaches the operator through the ordinary failing-check blocker rather than through an automatic trigger comment" — is placed immediately after the PENDING/SUCCESS definitions. This is the correct location: it completes the state vocabulary at the definition site rather than in a downstream restatement, so the contract is unambiguous from first reading. ✓

Requesting A Review (line 60): Narrowing "pending or failing" to "pending" is correct and verified against every gate_state comparison in the engine (babysit_review_trigger.py:421, babysit_delta.py). The "or failing" was the erroneous addition; the code was right. ✓

The pre-existing "Never discard a pending or failed StatusContext" sentence (line 25) is about a different concern — not replacing a StatusContext with a same-name CheckRun — and is unrelated to engagement candidacy. The two uses of "failing/failed" in the document are now clearly distinct. ✓

---

test_review_trigger_race.py — FailingGateIsNotAnEngagementSignalTests

Traced all three assertions through the implementation:

test_failing_gate_is_not_a_pending_request_signal: normalize_check resolves name via check.get("name") or check.get("context") → "review-gate". effective_state = "FAILURE" → check_category returns "failing" (FAILURE is in CHECK_FAILURE_STATES). In review_gate_state:

• gate_state = summarized_state(gate_statuses) → "failing" ✓
• request_signal_pending = any(effective_state == "PENDING" and not target_url) → False (effective_state is FAILURE) ✓
• ci_gateway_green: ci-gate CheckRun is success → True ✓
• non_review_checks: excludes the gate StatusContext by type+name filter, leaving only ci-gate → all success → True ✓

test_failing_gate_never_becomes_a_candidate: candidate predicate at line 421 requires gate_state == "pending" — False here. None of the earlier state-machine branches match (no reviews, reactions, history), so execution reaches the else at line 474: state = check_state["gate_state"] = "failing". missing_head_sha is "" (only set when candidate is True). All assertions correct. ✓

test_failing_gate_surfaces_as_an_ordinary_failing_check: classify_checks builds failing = [check["name"] for check in checks if check["category"] == "failing"]. The gate StatusContext normalizes to name = "review-gate", category = "failing" → classified["failing"] == ["review-gate"]. ✓

The isolation design is well-chosen: making the gate StatusContext the only non-success check while excluding it from non_review_checks isolates gate_state as the single variable determining request_eligible. ✓

---

plugin.json and CHANGELOG.md

Version is consistently 0.26.8 → 0.26.9 in both files. The CHANGELOG entry is accurate.

Note on the PR description: The body still says "Plugin version bump to 0.26.5" (written before additional main bumps landed). The diff shows 0.26.8 → 0.26.9; the code and CHANGELOG are correct and consistent. The description is stale only.

---

Verdict

Approved. Documentation and tests are correct, well-isolated, and fully cover the stated contract. No behavior change, no security surface, no action items.

[claude]

Claude finished @kyle-sexton's task in 57s —— View job

---

Security Review — PR #1392

• Load tools and gather context
• Fetch PR diff and read changed files
• Audit test_review_trigger_race.py for security issues
• Audit review-trigger.md for sensitive content / contract issues
• Audit plugin.json and CHANGELOG.md
• Post findings

---

No security issues found.

This PR is documentation + regression tests only; no production logic is changed. Full rationale by surface:

test_review_trigger_race.py — new FailingGateIsNotAnEngagementSignalTests class

All test data is fully hardcoded ("state": "FAILURE", context: "review-gate", a fixed 40-character HEAD SHA). No external input enters any test fixture; there is no injection surface. The sys.path.insert(0, ...) at line 16 is the established and safe pattern used throughout this test suite — it resolves to the adjacent scripts/ directory via pathlib.Path(__file__).resolve().parent.parent and runs only in the test process. mock.patch.object is correctly scoped to module attributes; no global resolver manipulation. No subprocess calls, no filesystem writes on user-controlled paths, no token or credential handling.

The review_trigger_allowed=True parameter in test_failing_gate_never_becomes_a_candidate at line 146 is the mutation-policy trust boundary being exercised, not bypassed — the test asserts request_eligible: false, which confirms the gate state correctly blocks candidacy even when the authorization flag is set.

reference/review-trigger.md — prose-only changes. No executable content, no credentials, no sensitive references. The contract narrowing (removing "or failing") reduces the documented attack surface; it does not expand it.

plugin.json — version bump 0.26.8 → 0.26.9; no field additions, no new permissions or capabilities declared, no schema changes, no new MCP endpoints or remote trust delegation.

CHANGELOG.md — prose-only.

No GitHub Actions files changed. No dependency pins touched. No secret or token handling introduced or modified. No pull_request_target / workflow_run patterns. No supply-chain risk.

[kyle-sexton]

This was generated by AI during autonomous /work-items:work-loop execution.

Review-comment triage — all four bot comments

Four issue-level comments across two review rounds (pre-merge SHA 0f9951b and post-merge SHA 34b2ebb); no inline review threads and no PR review bodies exist on this PR, so thread resolution is not applicable. Both rounds reached the same verdict, and both raised the same single actionable item.

#	Source	Finding	Classification	Evidence
1	PR Review (0f9951b) + PR Review (34b2ebb)	PR description's plugin-version line is stale — body claimed 0.26.5, diff is 0.26.8 → 0.26.9	VALID — fixed	git diff origin/main...HEAD -- plugins/source-control/.claude-plugin/plugin.json shows 0.26.8 → 0.26.9; the CHANGELOG entry is ## [0.26.9]. The body was written before two later main integrations.
2	Both PR Reviews	review-trigger.md doc changes correct and correctly placed; pending narrowing matches every gate_state comparison in the engine	VALID — no action	Confirms the honor-code decision documented in the PR body; no counter-evidence raised against it.
3	Both PR Reviews	All three assertions in FailingGateIsNotAnEngagementSignalTests traced correct against the implementation	VALID — no action	Re-verified locally: pytest tests/test_review_trigger_race.py -q → 29 passed; full suite python -m unittest discover → 375 tests, OK.
4	Both Security Reviews	No security issues; docs + tests only, no production logic, no secrets, no workflow or dependency changes	VALID — no action	Diff touches only plugin.json (version), CHANGELOG.md, reference/review-trigger.md, and test_review_trigger_race.py.

On the design decision

Neither reviewer challenged the honor-code-over-honor-doc resolution; both independently re-derived it and confirmed the pending narrowing matches the engine. The empirical tiebreaker in the PR body (the medley gate workflow posts only success/pending, never failure) stands unchallenged.

Fix applied

Finding 1 was a PR-description defect, not a branch defect — no commit was needed. The body has been read-modify-written in place: the version line now reads 0.26.9, the stale test count 369 is corrected to 375, and the "left to required CI" paragraph now names the jobs that own those two scripts (skill-quality-gate and portability-lint, per ci.yml) instead of narrating a worker timebox. Closes #324 and the ## Related section were preserved.

All 26 check runs on 34b2ebb are success, including both of those jobs.

No follow-up issue was filed — there was no VALID-but-deferred finding.