Resolve the queued-vs-unavailable ambiguity on both surfaces (#19 + #23)#25
Merged
Conversation
Two issues, one predicate. A configured review bot's status check is excluded from the blocking tally so a bot that never reports cannot wedge the watch loop — but that exclusion made an *unavailable* bot indistinguishable from one that reviewed and found nothing (#23), and a merely *queued* bot indistinguishable from a finished one (#19). Neither could be fixed by letting that check block, because the exclusion is the anti-wedge property. The way out is that nothing here touches `decide_converged`. The anti-wedge property lives entirely in that predicate and is left alone; every new signal feeds the merge gate, which already requires someone to explicitly record a review receipt. So the gate can wait longer, but the poll/fix/ack loop can always finish. `summarize_review_bots` resolves each `review.bots` entry to: - unavailable — an `unavailable_markers` hit on a comment body OR on the bot's status-check DESCRIPTION (#23; `gh pr view --json statusCheckRollup` carries no description at all, hence the second `gh pr checks` fetch). Never blocks; it is the action signal to run the fallback, and it survives `--mark-seen` so the gap stays readable at merge time. It also cancels the pending block below — a bot that announced it is not reviewing will never leave pending. - pending — the bot's check is non-terminal and no outage was announced. Blocks `mergeable` and makes `--record-review` refuse (#19: on #16 a receipt was taken against a queued CodeRabbit and four valid findings landed post-merge), until it ages past `review.bot_pending_grace_minutes` (default 15) — the bound that keeps a dead bot from wedging the gate. Also here because the fix does not work without them: - `"review rate limited"` added to `unavailable_markers`. The mechanism alone detects nothing: on #22 the check description used that wording while #24's comment said "review limit reached", an hour apart from the same bot. - kitconfig now parses decimal scalars, gated on a literal `.` so `nan`/`inf`/ `1e5` stay strings as PyYAML resolves them. Without it a `2.5` grace silently fell back to the default. - `_load_review_config` returns a NamedTuple; the shape grew, and positional unpacking makes every future growth a breaking change for every reader. - `safety-critical-changes.md` now matches `pr_watch.py`. It computes the gate `dev_session.sh` merely re-checks, and was matching only the latter. Blockers are strictly additive: `done` (the legacy alias an older `dev_session.sh` reads) can only go true→false, so per-file engine skew makes merges wait, never fire early. Tests 204 → 217. Closes #19 Closes #23 Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
topij
marked this pull request as ready for review
July 25, 2026 12:23
📝 WalkthroughWalkthroughAdds configured review-bot detection for outages and queued checks, prevents premature review receipts, incorporates pending blockers into merge gating, and updates configuration migration, documentation, integrity hashes, YAML parsing, and tests. ChangesReview bot merge-gate safeguards
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant pr_watch
participant GitHubCLI
participant MergeGate
Operator->>pr_watch: run watch or record review
pr_watch->>GitHubCLI: fetch checks and descriptions
pr_watch->>MergeGate: summarize bot state
MergeGate-->>pr_watch: review_bots and merge blockers
pr_watch-->>Operator: render report or reject receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/tests/test_kitconfig.py (1)
67-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptionally pin the parity claim instead of restating it. The docstring's whole argument is "PyYAML resolves these as strings", but nothing here checks that. A
yaml = pytest.importorskip("yaml")plusassert kitconfig.loads(text) == yaml.safe_load(text)on the same document would make the divergence regress loudly rather than by inspection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_kitconfig.py` around lines 67 - 94, Update test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml to import PyYAML via pytest.importorskip("yaml") and compare kitconfig.loads against yaml.safe_load for the same document, while retaining the focused type/value assertions that validate integer and decimal behavior.init.sh (1)
340-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a PID-suffixed temp file like
append_to_sectiondoes.$CONFIG_FILE.tmpis a fixed name; the neighbouring helper already uses.tmp.$$to avoid clobbering a stale/concurrent file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@init.sh` around lines 340 - 346, Use a PID-suffixed temporary path in the migration block instead of the fixed $CONFIG_FILE.tmp, matching append_to_section’s .tmp.$$ convention. Update both the redirected output destination and the subsequent grep, mv, and cleanup references consistently so stale or concurrent runs cannot reuse the same temporary file.scripts/pr_watch.py (1)
447-467: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAn older
ghcould still disable the bot-signal guard for unsupported fields.These fields are documented, but if an older
ghrejects one of them, the non-zero CLI error is ignored andjson.JSONDecodeErrorreturns[], so pending/outage signal can disappear silently. Ifgh pr checksshould be relied on here, pin/require aghversion that supports these fields, or surface one-time stderr warnings when the command fails non-zero and the command appears to use--json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pr_watch.py` around lines 447 - 467, Update the `gh pr checks` execution in the surrounding check-fetching function to handle non-zero results explicitly instead of silently returning an empty list. Require a compatible `gh` version for the requested JSON fields, or emit a one-time warning using the command’s stderr when a JSON-enabled invocation fails, while preserving the existing successful parsing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@init.sh`:
- Around line 326-354: Update the unavailable_markers migration around the
CONFIG_FILE guard and awk block to proceed only when the key has no inline value
and represents a block list. Add an alternate branch for an existing inline
unavailable_markers value that emits the existing by-hand WARNING instead of
modifying the config, while preserving the current post-condition verification
for block-list migrations.
---
Nitpick comments:
In `@init.sh`:
- Around line 340-346: Use a PID-suffixed temporary path in the migration block
instead of the fixed $CONFIG_FILE.tmp, matching append_to_section’s .tmp.$$
convention. Update both the redirected output destination and the subsequent
grep, mv, and cleanup references consistently so stale or concurrent runs cannot
reuse the same temporary file.
In `@scripts/pr_watch.py`:
- Around line 447-467: Update the `gh pr checks` execution in the surrounding
check-fetching function to handle non-zero results explicitly instead of
silently returning an empty list. Require a compatible `gh` version for the
requested JSON fields, or emit a one-time warning using the command’s stderr
when a JSON-enabled invocation fails, while preserving the existing successful
parsing behavior.
In `@scripts/tests/test_kitconfig.py`:
- Around line 67-94: Update
test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml to import
PyYAML via pytest.importorskip("yaml") and compare kitconfig.loads against
yaml.safe_load for the same document, while retaining the focused type/value
assertions that validate integer and decimal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 741e4500-8a9e-455e-bc7d-a9877a119df2
📒 Files selected for processing (9)
.claude/rules/safety-critical-changes.mdconfig/dev-model.yamldocs/agentic-dev-kit/workflows/pr-watch.mdinit.shkit-manifest.jsonscripts/lib/kitconfig.pyscripts/pr_watch.pyscripts/tests/test_kitconfig.pyscripts/tests/test_pr_watch.py
added 3 commits
July 25, 2026 15:29
Found by running the first cut against this PR: CodeRabbit's pending status
context reports `startedAt: 0001-01-01T00:00:00Z` — the zero time, i.e. no
timestamp. So the age fallback ("unmeasurable ⇒ fail open, don't block") was not
an edge case at all: it was the *only* path CodeRabbit ever took, and the #19
guard printed "age unmeasurable, NOT blocking" against a live review-in-progress.
The guard was dead code for the one bot it was written for.
The clock now falls back to when this engine first saw the bot pending,
persisted per PR. Because it is our clock and only ever advances, every pending
bot reaches the grace bound — so the fail-open branch is gone entirely rather
than merely narrowed, and there is no unmeasurable case left to wedge on.
Stored as `bot_pending_since: {"head": sha, "bots": {bot: iso}}` — self-
describing rather than scoped by the sibling `state["head"]`. That field is the
false-settle guard's input; making `record_review` maintain it just to scope
this clock would put two intents on one key, and that key decides whether a
just-pushed commit may settle.
`record_review` persists the first sighting BEFORE refusing. Without it a cold
`--record-review` restarts the clock at zero on every retry, so the refusal
could never expire and the override would be the only way through — a wedge
dressed as a guard.
Verified live on this PR: the merge gate now reports
`review bot coderabbit has not reported yet (check CodeRabbit pending 0.0m
< 15m grace)`. Tests 217 → 221.
Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
All four valid, including one real bug in my own migration. **init.sh corrupted an inline `unavailable_markers` list.** The awk insert anchored on `^ unavailable_markers:`, which also matches an inline flow list (`unavailable_markers: ["a", "b"]`) — equally valid YAML and equally supported by kitconfig. It then appended a ` - ` line under a key that already has a scalar value, producing a malformed mapping, and wrote it straight over the adopter's config. Now only a bare block-list key is migrated; an inline list falls through to the existing by-hand warning. Verified against both shapes. **`fetch_check_details` degraded silently.** Both new guards depend on that fetch, so an older `gh` rejecting one of the requested `--json` fields would disable #19's and #23's detection with no trace — "checked and clean" indistinguishable from "never checked", the failure mode that cost this repo three silent-no-op bugs in one session. It now warns on stderr, once per process rather than per poll (a warning repeated every round of a watch loop is skimmed past exactly like silence). **The PyYAML parity claim is now checked, not restated.** The float-gating docstring argued "PyYAML resolves these as strings" while asserting only the expected values — which would pass just as happily if the reference resolver disagreed with every one of them. Compared against `yaml.safe_load` directly. **`.tmp.$$`** for the migration temp file, matching `append_to_section`. Tests 221 → 222. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Two fresh-context passes over the diff (adversarial + general-correctness) found almost disjoint sets, which is the doctrine's claim about why a gate needs both. **A stale outage comment permanently cancelled the pending block — #19 walking back in through the door built to close it.** `collect_comments` returns the whole PR history, unscoped by head or age, so one transient rate limit on commit 1 put the bot in `unavailable_bots` for the rest of the PR and waved through every later queued review. Rate limits are transient by construction, which makes "this bot was rate-limited earlier" the ORDINARY state of a later poll, not a corner case. Now only a CHECK-surface outage cancels: a check describes the bot's state now, a comment describes the past. Comment outages are still reported — that is #23's action signal — they just cannot speak for the present. **A corrupt or foreign `bot_pending_since` value wedged the merge gate forever.** `age = parse(stored) or 0.0` pinned an unparseable value at the maximally- blocking age *and* wrote it back, so every later poll re-read the same poison. Reachable from a corrupt state file, an older engine, or a richer future format. Anything unreadable is now replaced rather than coerced, so the window stays bounded whatever wrote the state. **The init.sh migration silently orphaned every existing marker on a 2-space block list.** The guard checked the key line's indent while the awk anchored on 4-space items, so a config formatted with dashes at the key's own indent (what several YAML formatters emit, and what kitconfig explicitly supports) got the new marker inserted at the wrong depth — dropping all the originals, while the post-condition grep still passed and `mv`'d it over the adopter's config. That is fail-OPEN on the very markers this PR exists to extend. Now indent-aware, verified against 4-space, 2-space and inline shapes. Also: `review_bots.signal` makes a failed check-read machine-readable (the merge gate consumes stdout; a stderr warning is not a guard on an autonomous path); the override is recorded on the receipt; comment authors match anchored rather than by substring (that input is attacker-controlled on a public repo, check names are not); the grace bound compares the exact age, not the rounded display value; `render` no longer claims "past the grace" for a check cancelled by an outage; kitconfig's float rule is YAML 1.1's own (signed exponent required) with its three remaining divergences pinned as divergences; and init.sh's idempotency grep is scoped to the marker block so it cannot silently skip both the migration and its warning. Tests 222 → 229. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Owner
Author
|
@coderabbitai full review Your completed review covered |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 32 minutes. |
added 2 commits
July 25, 2026 16:06
The doctrine's "re-review after every fix round, not one pass" earning its keep: a third fresh-context pass found that two of the three fixes were incomplete and that the round introduced a new data-loss path. **`record_review` computed the bot signal and threw it away.** When the check read fails there are no blockers to raise, so the receipt was taken with the #19 guard simply switched off — and unlike the explicit override, which the same round had just made auditable, this left no trace at all. The commit message even justified `signal` as machine-readable evidence while no consumer read it. Now recorded on the receipt as `bot_signal`, surfaced by the renderer. Still not a refusal: an old `gh`, or a PR with no checks, is an environment problem, and refusing would turn it into a wedge whose only exit is the override flag. **The poison-clock fix missed the parseable half.** A *future-dated* `bot_pending_since` survives `_age_minutes`, gets clamped to age 0 by `max(0.0, …)`, and is re-persisted verbatim — so the block lasts until real time catches up with the stamp. Measured at 30 days on a copied state file. Anything beyond a small skew tolerance is now treated as unusable and restamped, so the window is bounded whatever wrote it. The previous test only covered unparseable values, which is why this survived. **The new `review.bots` migration lost adopter values.** `grep -q '^ bots:'` misses a 4-space-indented `review:` block and then appends a duplicate at 2 spaces, which the reader resolves last-key-wins — silently dropping a `[bugbot, coderabbit]` down to `[coderabbit]`. It also reported success when it had written nothing, and was satisfied by an unrelated `bots:` under any other section. **The marker migration still no-opped silently, and could write to the wrong section.** Scoping the *idempotency grep* to the block left the outer key anchor whole-file, so: a tab- or 4-space-indented key skipped both migration and warning; a same-named list under an earlier section got the marker while the success message claimed `review.unavailable_markers` had it; a comment or blank line mid-list truncated the idempotency view and re-inserted a marker already present; and a file with no trailing newline failed the `wc -l` post-condition. Both migrations now go through `section_range` / `section_lines`, which bound every read and write to the `review:` section at any indent. Every whole-file `grep '^ key:'` in a migration is a latent bug in two directions at once — it misses the key at another indent and matches a same-named key elsewhere — and this change had shipped one of each. Also: kitconfig's float pattern is now transcribed from PyYAML's resolver rather than approximated. Hoisting the sign out of the alternation diverged on `+.5`, `-.5` and `-.5e+3` — the exact "float() is too loose" class the previous commit claimed to have eliminated. Parity verified over 24 forms; the two remaining gaps (`.inf`/`.nan`, sexagesimal) are pinned as gaps. Plus the blocker string no longer reads `pending 15.0m < 15m grace`, and three docs describing anchored author matching as substring are corrected. Migrations verified against seven real config shapes (4-space, 2-space, inline flow, comment-mid-list, empty list, no-trailing-newline, same-named key under another section), each round-tripped through the real kitconfig. Tests 229 → 231. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
#3) Third pass. It found that 8a1d5df applied its own section-scoping fix to ONE of three guards in the same function, that the marker insertion had a third corruption mode, and that the highest-blast-radius code in the diff had no tests at all. All three are the same underlying mistake: shell text-munging on a file the adopter owns, defended one special case at a time. **The marker migration is gone.** Three review rounds produced three distinct ways for list surgery to corrupt a config — wrong indent orphaned every existing entry; a whole-file key anchor wrote into a same-named list under another section; and inserting before "the first line that is not an item" splices a multi-line item in half, yielding YAML PyYAML refuses to load *entirely*. Each passed the migration's own post-conditions and printed success. The payoff was one string in a list. `init.sh` now detects the gap and prints exactly what to add — a trade that stops being worth defending after the third corruption. **Guards are section-scoped everywhere now, not in one place.** A whole-file `grep '^ key:'` fails in two directions at once: it misses the key when the adopter's section uses another indent, and it is satisfied by a same-named key elsewhere. `migrate_kit_schema` had one of each; `migrate_runtime_schema` still had three more, which is why a 4-space config was non-idempotent — every run re-appended `fallback_commands`. **Per-key review migration.** One `noise_markers` guard used to gate a block defining five keys, so an adopter with `unavailable_markers` but no `noise_markers` got a second definition of it — and both readers resolve last-key-wins, silently replacing their list with the kit's defaults. **`append_to_section` re-indents to the section it is writing into.** Appending a 2-space block to a 4-space section produced a mapping with two indent levels: PyYAML fails on the WHOLE file, kitconfig tolerates it and applies last-key-wins. Note what did NOT ship: making this function return non-zero on "section not found" seemed like the obvious way to stop callers reporting false success — under `set -eu`, with callers that don't test its status, it aborted init.sh on the first config missing any optional section. Caught by running it. **Engine fixes.** The observed clock now wins over a check's own stamp whenever we have one: preferring the check let the age REGRESS when a future-dated stamp slid inside the skew tolerance, restarting a window that had already expired and making merge_blockers non-monotonic in wall-clock time. And `bot_signal` is recorded only for `unavailable`, not for `skipped` — "you configured no bots" was being reported as "the guard did not run", putting a permanent false warning on every receipt a bot-less adopter takes. **Tests, finally.** Eight config shapes (2-space, 4-space, flush-indent, inline flow, decoy section, header with trailing space, multi-line item, no trailing newline) × corruption, idempotency, exactly-once, and the instruct-when-it-can't path. Every corruption above fails at least one. The kitconfig-vs-PyYAML disagreement on multi-line scalars is asserted as a known reader limitation rather than skipped, so closing it later fails loudly. Verified the shipped config is unchanged relative to main (both reformat the same 5 comment-alignment lines via the pre-existing set_field). Tests 231 → 249. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/tests/test_kitconfig.py (1)
121-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe no-PyYAML test covers 3 of the 21 tokens, which inverts the invariant stated at Lines 67-69.
The parity test skips exactly where the engines run (no PyYAML), so the type assertions are the only coverage there. Parametrizing over
_FLOAT_EDGE_CASESwith an expected-type map keeps both halves over the same set.♻️ Proposed refactor
-def test_decimal_scalars_keep_their_python_types_without_pyyaml(): - """The same resolution, asserted directly — the engines run with no PyYAML, - so the parity test above skips in exactly the environment that matters most. - """ - parsed = kitconfig.loads( - "review:\n grace: 2.5\n whole: 15\n bare_exponent: 1e5\n" - ) - - assert parsed["review"]["grace"] == 2.5 - assert isinstance(parsed["review"]["whole"], int) # not silently a float - assert parsed["review"]["bare_exponent"] == "1e5" +_EXPECTED_TYPES = { + "2.5": float, "15": int, "1.": float, "0.0": float, "-0.0": float, + "-0.5": float, "+1.5": float, ".5": float, "-.5": str, "+.5": str, + "1.0e+3": float, "1.0e3": str, "1.0E3": str, "1.5e3": str, "-.5e+3": str, + "1_0.5": float, "._5": float, "nan": str, "inf": str, "1e5": str, + "1.2.3": str, +} + + +def test_every_edge_case_has_an_expected_type(): + """So a token added above cannot slip past the no-PyYAML assertions.""" + assert set(_EXPECTED_TYPES) == set(_FLOAT_EDGE_CASES) + + +@pytest.mark.parametrize("token", _FLOAT_EDGE_CASES) +def test_decimal_scalars_keep_their_python_types_without_pyyaml(token: str): + """The same resolution, asserted directly — the engines run with no PyYAML, + so the parity test above skips in exactly the environment that matters most. + """ + value = kitconfig.loads(f"v: {token}\n")["v"] + assert isinstance(value, _EXPECTED_TYPES[token]), (token, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_kitconfig.py` around lines 121 - 131, Refactor test_decimal_scalars_keep_their_python_types_without_pyyaml to parametrize over every token in _FLOAT_EDGE_CASES, using the shared expected-type map for assertions. Keep the no-PyYAML parsing path and verify each token’s value and Python type so coverage matches the parity test’s full edge-case set.scripts/pr_watch.py (1)
1055-1055: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
zip(..., strict=True)to enforce the pending/exact-age invariant.
pendingandexact_agesare appended in lockstep and should stay that way;strict=Trueis compatible with the script’s Python target and would surface future list divergence instead of truncating blocks silently. This also clears the Ruff B905 hint.♻️ Proposed tweak
- for entry, exact_age in zip(pending, exact_ages): + for entry, exact_age in zip(pending, exact_ages, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pr_watch.py` at line 1055, Update the loop iterating over pending and exact_ages to call zip with strict=True, preserving the existing entry processing while raising an error if the lockstep invariant is violated.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@init.sh`:
- Around line 397-421: Update the unavailable_markers handling in the
section_lines/markers check to detect whether the existing value uses flow-style
brackets. Keep the current block-list suggestion for block-style YAML, but
provide an inline addition instruction for flow-style values so the adopter
inserts "review rate limited" inside the brackets without producing invalid
YAML.
---
Nitpick comments:
In `@scripts/pr_watch.py`:
- Line 1055: Update the loop iterating over pending and exact_ages to call zip
with strict=True, preserving the existing entry processing while raising an
error if the lockstep invariant is violated.
In `@scripts/tests/test_kitconfig.py`:
- Around line 121-131: Refactor
test_decimal_scalars_keep_their_python_types_without_pyyaml to parametrize over
every token in _FLOAT_EDGE_CASES, using the shared expected-type map for
assertions. Keep the no-PyYAML parsing path and verify each token’s value and
Python type so coverage matches the parity test’s full edge-case set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44cbebee-3088-48b1-a48b-84b178b1241a
📒 Files selected for processing (9)
config/dev-model.yamldocs/agentic-dev-kit/workflows/pr-watch.mdinit.shkit-manifest.jsonscripts/lib/kitconfig.pyscripts/pr_watch.pyscripts/tests/test_kitconfig.pyscripts/tests/test_portability.pyscripts/tests/test_pr_watch.py
🚧 Files skipped from review as they are similar to previous changes (3)
- kit-manifest.json
- config/dev-model.yaml
- scripts/lib/kitconfig.py
Its first pass covered `233fdd0`; this is its first look at the actual design,
after the grace clock and three rounds of fallback-panel fixes. All three
findings valid.
**The "ACTION NEEDED" message walked inline-list adopters into the corruption
it replaced.** Dropping the list surgery left an instruction that always prints
`- "review rate limited"` — a block item. An adopter whose `unavailable_markers`
is a flow list (`["a", "b"]`) who follows that literally hangs a block item off
a flow scalar and breaks their config. The step is read-only, so it could not
corrupt anything itself; it would just have talked someone else into doing it.
Now detects the style and says "add the string inside the brackets" instead.
**`zip(pending, exact_ages, strict=True)`** — the lists are appended in lockstep,
so a later edit adding a `continue` between them would pair each entry with the
wrong age, silently, and only for a bot with more than one pending check.
**The no-PyYAML type test covered 3 of 21 tokens**, which inverts the invariant
its own neighbour states ("kept as one list so the parity check cannot quietly
cover a smaller set"). The parity test skips precisely where the engines run —
a bare env with no PyYAML — so those three were the only coverage that
environment had. Now parametrized over the full set with a declared expected
type, plus a guard that the two sets stay equal, so adding a token cannot leave
it untested where it matters most.
Tests 249 → 270.
Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 minutes. |
**The style-detection branch had no test, proven by mutation.** Hardwiring the condition to a constant — so inline-list adopters get the corrupting `- ` advice — still passed the whole suite. The only assertion checked the block direction. Sharpest part: `_MIGRATION_SHAPES` already contained an `inline_flow` fixture and the commit message advertised the path as covered; the fixture was there, the assertion was never written. Now parametrized per style, and the same mutation fails. **Style detection missed a real flow spelling.** `head -n 1 | grep '\['` only sees the key line, so a flow list whose value sits on the FOLLOWING line (`unavailable_markers:\n ["a", "b"]`) read as block style and got the corrupting advice. Presence of `- ` items is the discriminator now, which also gives the empty-key case its own answer: no value means the engine defaults apply, and those already contain the marker, so asking for it is noise. **A marker named only in a trailing comment counted as present.** The grep ran over raw lines, and the kit's own shipped config carries a `# the status-check wording of …` comment on that exact line — so an adopter with the phrase in a comment and not in the list would be told nothing. Matched against comment-stripped values now. **The no-PyYAML type assertions covered 21 of 25 tokens** while the guard's docstring claimed they covered the same set as the parity check. The parity test walks two collections; the guard compared against one. The four missing tokens are the divergences — exactly where this reader departs from PyYAML on purpose, so an accidental change there looks like a fix. Both collections are covered now, and the divergent tokens' string-identity assertion moved out from behind the `importorskip` it never needed. Tests 270 → 281. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Round 6 found that the previous round promoted a list spelling to "supported"
in the instruction table without checking whether the kit's own reader parses
it. `kitconfig` — what every engine uses — resolves a flow list whose brackets
don't close on the key line to `{}`, and a multi-line one to the string `"["`:
unavailable_markers: ["a", "b"] -> ['a', 'b']
unavailable_markers:\n ["a", "b"] -> {}
unavailable_markers: [\n "a",\n "b"\n] -> '['
So an adopter in either of the latter shapes has NO markers in effect at all,
and "add the string inside the brackets" is advice that fixes nothing. Those
shapes now get told what is actually wrong — the whole list is inert — which is
considerably bigger news than one missing marker. The style detection is
unchanged for the shapes that do work.
Two smaller ones from the same pass:
- **Comment-stripping was quote-unaware**, so a marker containing an issue
number (`- "tracked in #23: review rate limited"`) was truncated before the
match and the adopter was asked for a marker they already had. Now scans
character-wise and only cuts a `#` outside quotes.
- **`grep -qi`'s case-insensitivity had no test** — dropping `-i` passed all 281
— which is the same untested-branch class the previous round existed to close.
Also corrected in the PR body: the baseline is 202 tests, not 204 (measured at
the merge-base); "four ways to corrupt" is three, matching what init.sh's own
comment enumerates — the fifth round's finding was about the replacement
message, not surgery; and the migration matrix is 8 shapes × 2 tests plus a
separate 5-row instruction table, not 8 × 4.
Tests 281 → 285.
Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Owner
Author
Merging — review coverage, stated plainlyRecorded receipt: What covered this head. Seven review rounds, every one of which found something real. CodeRabbit gave a full review of the substantive design at That gap is the honest state of this merge, and it is smaller than #22's (where the bot's only pass predated a material redesign) but not zero. What the rounds actually caught, in case it is useful later:
Two things worth carrying forward (going to the friction log rather than being lost here):
|
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
Third verification pass on this branch, third round of real findings — and both HIGH ones were introduced by the previous commit. **`bots_behind_head` was never recorded under `--allow-pending-bot-review`.** `behind` was populated inside `if not allow_pending_bot`, so the override path fetched the reviews and discarded them. That override IS the #22/#25 scenario: a bot queued or rate-limited through a redesign, merged on a fallback receipt. Combined with the second bug below it was a total blind spot — neither the poll nor the receipt said the reviewer's last review was of an old commit. Coverage now comes off the `pr view` snapshot before the branch, independent of the check fetch. **The pending-deference swallowed the case it was written to protect.** It deferred to *any* pending entry, but a pending check that has aged past the grace window or been cancelled by an announced outage is the engine saying "this verdict is not coming" — the reviewer-went-away case. So after ~15 minutes of polling any stuck bot, and on the #22 two-check outage shape, the coverage warning went quiet precisely when it should speak. Only an actively *blocking* pending entry defers now. Both were invisible to the two lenses that reviewed the commit introducing them, and both are now pinned by mutation. Also: `bots=bots` threading was correct and pinned by nothing (dropping it passed all 297, since every other test uses the default list where scoped and unscoped are identical); a `.get` on a key the next lookup indexes directly; an over-long docstring line; and the doc claimed `--record-review` "records the same thing" as the poll, which is no longer true — the receipt deliberately does not defer to a pending bot. Tests 297 → 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* Surface which commit each bot's last review actually saw The cheap half of #27. A receipt binds to the head and a push invalidates it. That answers "was this exact code reviewed" — but not "by whom, and how much of it did they see". A bot can review commit 1, go rate-limited through a material redesign, and the merge proceed on a fallback receipt taken at commit 5. #22 shipped a fail-open rework the primary reviewer never saw; #25 repeated it smaller, three commits behind. Both times the gap was real, both times it was only reconstructible by reading the PR thread afterwards, and both times someone had to notice it by hand. Now `pr_watch` reads it off the reviews it ALREADY fetches — no extra `gh` call: ⚠ review coverage: coderabbit's last review was of 954b93f, not the current head — a receipt taken now does not mean it saw this design Verified against the real #25, where it reproduces exactly the gap I had to work out manually before merging it an hour ago. **Reported, never gating.** The faithful fix — invalidate a receipt when the diff changes *shape*, not merely when the head moves — is the other half of #27 and stays open, because it risks becoming a wedge on a repo whose bot is permanently unavailable. Making the gap visible is the part that is unambiguously safe, and it is what would have changed the operator's judgment on both #22 and #25. Malformed input is dropped rather than reported as coverage of an unknown commit, and the function cannot raise — it runs on the ordinary poll path. Tests 286 → 289. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Pin the two properties a mutation pass proved were unpinned The adversarial review ran 12 mutants; 9 died, 2 were real gaps, 1 equivalent. **`anchored=True` had no test.** Flipping it to `anchored=False` passed all 289 — and with that flip `xcoderabbit` reviewing the head reads as CodeRabbit having covered it, suppressing the exact warning this feature exists to raise. It is the one property here with an actual adversary (anyone may review on a public repo), and the PR body asserted it while nothing checked it. **Newest-per-bot was untested independent of array order.** Replacing the whole comparison with `if True:` — last-in-array wins, timestamps ignored — also passed. The only multi-review fixture listed its reviews ascending, so the two rules were indistinguishable. Now pinned with a descending fixture and a shuffled one. Also pinned the tie-break direction that matters: an undated review must never displace a dated one, since an undated review at the head would otherwise set `covers_head` and hide the gap. **A non-string `oid` crashed `render` on the poll path.** `isinstance(commit, dict)` validates the container, not the value, so `{"oid": 12345}` passed the filter and then died on `sha[:7]` — in the function whose stated job is tolerating malformed input, and in the test that claimed to cover it. Now type-checked. Same for a non-string `submittedAt`, which raised TypeError against a sibling review's string. All four mutants now die. Corrected three claims that overstated the code: the docstring said "one entry per bot that has reviewed at all" (a bot whose reviews carry no usable SHA gets no entry — under-reporting, deliberately, but not what it said); "the function cannot raise" was absolute and wasn't; and the lexicographic timestamp compare's dependence on gh's exact serialization was undocumented. The doc's "known gaps" list now includes coverage's own blind spot — a bot that never reviewed produces no entry and no warning. Tests 289 → 293. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix a bug the first lens missed, and put the warning where it claims to speak The general-correctness lens, run because the doctrine requires both and the adversarial pass had noted it was only one. It found a real bug that pass missed — which is the disjointness claim holding up again. **The `str()` coercion I added last commit was actively wrong.** It stopped the TypeError and, in doing so, rendered garbage as a string sorting ABOVE every real timestamp (`"20260725" > "2026-07-25T…"`). So a malformed review at the head displaced the real dated one and set `covers_head: True` — suppressing the warning, in exactly the direction the docstring beside it calls "the direction that matters". Type-checking instead of coercing is equally crash-proof and sorts garbage to the bottom with the undated ones. The sharpest part: `test_a_non_string_sha_or_timestamp_cannot_break_the_poll` feeds precisely that input and asserts only that it does not raise. It walked over the hole. **`coverage` was a pass-through that arrived empty where it mattered most.** `record_review` calls `summarize_review_bots` without it, so `coverage` was `[]` on the receipt path — "no data" indistinguishable from "every bot is current", on the one path whose entire subject is what a receipt covers. Now computed inside the function like every other key, and recorded on the receipt as `bots_behind_head` next to `override` and `bot_signal`. Its own message says "a receipt taken now does not mean it saw this design"; it was printing everywhere except where a receipt is taken. **It now defers to a bot that is mid-review.** A bot reviewing a just-pushed head is behind it by construction, and the pending line already says a verdict is coming — so the warning was firing on every poll of the healthy window, training the reader to skim past the case it exists for. **Two mutants that survived the first pass now die.** Rendering the line unconditionally, and wiring the wrong head into the coverage call, both passed 292 tests: nothing drove `build_report` -> `render` with a bot AT the head and asserted silence. Selectivity is the whole value of the warning. Also: a comment describing a guard the previous commit deleted (the repo's named failure mode, again); `coverage` missing from both report-schema docstrings; the doc paragraph sitting under a sentence that said every signal feeds the merge gate, which this one does not. Tests 293 → 297. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * The override path was silent about exactly what it exists to record Third verification pass on this branch, third round of real findings — and both HIGH ones were introduced by the previous commit. **`bots_behind_head` was never recorded under `--allow-pending-bot-review`.** `behind` was populated inside `if not allow_pending_bot`, so the override path fetched the reviews and discarded them. That override IS the #22/#25 scenario: a bot queued or rate-limited through a redesign, merged on a fallback receipt. Combined with the second bug below it was a total blind spot — neither the poll nor the receipt said the reviewer's last review was of an old commit. Coverage now comes off the `pr view` snapshot before the branch, independent of the check fetch. **The pending-deference swallowed the case it was written to protect.** It deferred to *any* pending entry, but a pending check that has aged past the grace window or been cancelled by an announced outage is the engine saying "this verdict is not coming" — the reviewer-went-away case. So after ~15 minutes of polling any stuck bot, and on the #22 two-check outage shape, the coverage warning went quiet precisely when it should speak. Only an actively *blocking* pending entry defers now. Both were invisible to the two lenses that reviewed the commit introducing them, and both are now pinned by mutation. Also: `bots=bots` threading was correct and pinned by nothing (dropping it passed all 297, since every other test uses the default list where scoped and unscoped are identical); a `.get` on a key the next lookup indexes directly; an over-long docstring line; and the doc claimed `--record-review` "records the same thing" as the poll, which is no longer true — the receipt deliberately does not defer to a pending bot. Tests 297 → 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Tidy what the first clean pass found Fourth verification pass on this branch, and the first that could not break the code — no HIGH or MEDIUM. Four LOW items: - **Coverage was computed twice on the normal path**, and `reviews=`/`head=` on `record_review`'s `summarize_review_bots` call had gone dead when the direct call was hoisted above the override branch. Harmless today (pure, no I/O) but a latent divergence trap: the two calls resolve their bot list differently, so the receipt and the poll would silently disagree the moment this function takes a `bots` argument — the exact class `test_coverage_honours_a_non_default_review_bots_list` was just written to pin. - **A comment quoted a message string that does not exist** in the tree. Same stale-comment class this repo keeps finding, three lines below the block the previous commit edited. - `record_review`'s docstring enumerated two of the three keys the receipt can carry; `bots_behind_head` was missing. - Noted why `.get("blocking")` sits next to direct indexing: a pending entry missing that key falls out of the deference set and the warning fires, which is the safe direction. Tests unchanged at 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
I had the wrap-up independently fact-checked against `git log` / `gh` rather than self-certifying it, because the handoff is read at the start of every future session. It came back with 13 issues, three HIGH. The one worth naming: **I reintroduced a number that a review round on #25 had explicitly corrected** — "four ways to corrupt an adopter's config" when `321907e` had already changed it to three, and `init.sh`'s own comment enumerates three. I did that while writing up the lesson that PR bodies keep drifting from their diffs. The friction entry recording that failure now records that it recurred inside the document describing it, and is filed at M not L. **The base rate was understated, in my own argument's favour.** I wrote "eleven rounds, ten with findings" and "#29 produced a clean pass on the fourth attempt". Neither is true: session-wide it is **13 rounds across three PRs, all 13 with findings**, and #29's fourth pass found no HIGH/MEDIUM but still found four LOW including a stale comment. Self-inflicted defects — introduced by the previous round's fix — were 7, not 5. So "re-review until a pass finds nothing new" never terminated, which sharpens rather than weakens the point: the rule needs a stopping criterion, and the one actually used was blast radius. **The proposed rule change was aimed at a rule that already exists.** `safety-critical-changes.md` rule 3 already mandates re-review after every fix round and already notes that fix rounds introduce regressions. The proposal now asks for what is genuinely missing — a termination criterion, and the blast-radius calibration that let #29 merge where #25's standard would not have. Also corrected: the mutation-proved tally (five properties session-wide, not two in one place and four in another); "the panel carried the entire review load" (CodeRabbit completed 3 reviews across 17 heads — most, not all, and one of its three caught a real bug); "five manual launches per round" (two lenses per round, ~10 rounds); a "126 report pairs" figure that exists only in a PR comment and is not reproducible from the repo, now pointing at the in-repo tests instead; a dangling "the two entries above this line" after those entries were deleted; and two inbox entries whose issues this session closed, now marked done. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
CodeRabbit, on the wrap-up. My correction commit fixed the handoff's tally to five session-wide and left the friction log saying two — so the two documents the next session reads disagreed with each other about the very number the correction was about. Both now say five, with the properties enumerated (three on #29, two on #25) so the figure is checkable rather than asserted. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* Wrap-up: Phase 3b — the queued-vs-unavailable ambiguity, closed Three PRs merged, three issues closed, two filed. - **#25** (closes #19 + #23) — a configured bot resolves to *unavailable* (outage on a comment body OR a status-check description, the surface that was invisible) or *pending* (a verdict still coming). Nothing reaches `converged`, which is what let both be fixed at once without touching the anti-wedge property. - **#28** (closes #10) — the state_paths suite failed from inside a lane worktree because its fixture cleared every sandbox *env* signal and not the *cwd* one. - **#29** (half of #27) — `review_bots.coverage` reports which commit each bot's last review actually saw. - **#26 / #27 filed** from the friction-log inbox, both with concrete sketches. The handoff and friction log carry the detail. The two entries worth repeating: **Eleven review rounds, ten with findings — and five of those were defects introduced by the previous round's fix**, twice at HIGH. #25 never produced a clean pass. That is not a caution about fix rounds; it is the base rate, and the doctrine currently states it as advice. **Reading is not running.** The #19 guard was dead code for the exact bot it was written for, because CodeRabbit's pending check reports the zero timestamp — no amount of re-reading found that; polling the live PR did. Same for a change that looked plainly correct and aborted `init.sh` under `set -eu`. Also recorded: `pre-push` refused this very commit's first attempt, because the narrative files had been swept into a lane branch. The guard fired on its own author with a message that made the fix obvious — logged as the positive case this log rarely captures. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix 13 errors the fact-check found in this wrap-up I had the wrap-up independently fact-checked against `git log` / `gh` rather than self-certifying it, because the handoff is read at the start of every future session. It came back with 13 issues, three HIGH. The one worth naming: **I reintroduced a number that a review round on #25 had explicitly corrected** — "four ways to corrupt an adopter's config" when `321907e` had already changed it to three, and `init.sh`'s own comment enumerates three. I did that while writing up the lesson that PR bodies keep drifting from their diffs. The friction entry recording that failure now records that it recurred inside the document describing it, and is filed at M not L. **The base rate was understated, in my own argument's favour.** I wrote "eleven rounds, ten with findings" and "#29 produced a clean pass on the fourth attempt". Neither is true: session-wide it is **13 rounds across three PRs, all 13 with findings**, and #29's fourth pass found no HIGH/MEDIUM but still found four LOW including a stale comment. Self-inflicted defects — introduced by the previous round's fix — were 7, not 5. So "re-review until a pass finds nothing new" never terminated, which sharpens rather than weakens the point: the rule needs a stopping criterion, and the one actually used was blast radius. **The proposed rule change was aimed at a rule that already exists.** `safety-critical-changes.md` rule 3 already mandates re-review after every fix round and already notes that fix rounds introduce regressions. The proposal now asks for what is genuinely missing — a termination criterion, and the blast-radius calibration that let #29 merge where #25's standard would not have. Also corrected: the mutation-proved tally (five properties session-wide, not two in one place and four in another); "the panel carried the entire review load" (CodeRabbit completed 3 reviews across 17 heads — most, not all, and one of its three caught a real bug); "five manual launches per round" (two lenses per round, ~10 rounds); a "126 report pairs" figure that exists only in a PR comment and is not reproducible from the repo, now pointing at the in-repo tests instead; a dangling "the two entries above this line" after those entries were deleted; and two inbox entries whose issues this session closed, now marked done. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Align the mutation tally across both narrative files CodeRabbit, on the wrap-up. My correction commit fixed the handoff's tally to five session-wide and left the friction log saying two — so the two documents the next session reads disagreed with each other about the very number the correction was about. Both now say five, with the properties enumerated (three on #29, two on #25) so the figure is checkable rather than asserted. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 3b. Fixes #19 and #23 together, because they are two branches of one
question — has the independent reviewer had its say? — and the obvious local
fix for each is the one move the design forbids.
The constraint that made them unfixable alone
A configured review bot's status check is excluded from the blocking tally
(
review.informational_checks). That exclusion is load-bearing: a review bot'scheck can sit PENDING forever after a trivial follow-up commit, and the watch
loop must still be able to finish. So neither issue can be fixed by "let that
check block."
But the exclusion is also what caused both bugs:
Review rate limitedon aSUCCESScontextPENDING — Review queued; merged; four valid findings landed minutes laterThe resolution
Nothing here touches
decide_converged. The anti-wedge property livesentirely in that predicate and is left alone. Every new signal feeds the merge
gate, which already requires someone to explicitly record a receipt — so it can
delay a merge but can never stall the poll/fix/ack loop. Pinned by a test.
summarize_review_botsresolves eachreview.botsentry to one of:unavailable_markershit on a comment body or on thebot's status-check description. Blocks nothing; it is the action signal to
run
review.fallback_commands, and it survives--mark-seenso the gap staysreadable at merge time. Only a check-surface outage cancels the pending
block — comments are unscoped PR history, and since rate limits are
transient, a stale one would wave through every later queued review.
genuinely coming, so this blocks
mergeableand makes--record-reviewrefuse, until it ages past
review.bot_pending_grace_minutes(default 15).That bound is what keeps a dead bot from wedging the gate.
The grace clock cannot come from the check alone: CodeRabbit's pending status
context reports
startedAt: 0001-01-01T00:00:00Z. It falls back to when thisengine first saw the bot pending — persisted per PR, head-scoped, reset by a
push. A stored value that will not parse, or one dated in the future, is
replaced rather than trusted, so the window stays bounded whatever wrote it.
review_bots.signal(ok/skipped/unavailable) makes a failed check readmachine-readable, and a receipt taken while it was
unavailablerecords that —the gate consumes stdout, so a stderr warning is not a guard on an autonomous
path. The
--allow-pending-bot-reviewoverride is recorded on the receipt too.Why a second
ghcallgh pr view --json statusCheckRollupreturns a fixed sub-shape with nodescription field on either check kind. That omission is the whole of #23.
gh pr checks --jsonexposes it. The two sources stay in separate lanes: thenew fetch feeds only reviewer identity/state, never
all_green.Carried along, because the fix does not work without them
"review rate limited"inunavailable_markers. The mechanism alonedetects nothing — Split the watch-loop predicate from the merge gate (additively) #22's check description used that wording while Wrap-up: Phase 3a — split the watch predicate from the merge gate #24's
comment said
review limit reached, same bot, same outage, an hour apart.init.shno longer edits the marker list. Three review rounds producedthree ways for list surgery to corrupt an adopter's config, each while the
post-conditions passed and success was printed. It now detects the gap and
prints what to add, in the style the adopter's list actually uses — or, for a
flow list the kit's reader cannot parse at all, says that instead.
grep '^ key:'fails intwo directions at once — misses the key at another indent, matches a same-named
key elsewhere — and both migrations had shipped one of each.
kitconfigresolves floats by YAML 1.1's own rule (signed exponentrequired; sign allowed on
digits.digitsbut not.digits), with its fourremaining divergences pinned as divergences. Without it a
bot_pending_grace_minutes: 2.5silently fell back to the default.safety-critical-changes.mdnow matchespr_watch.py— it computes thegate
dev_session.shmerely re-checks.What review found
Seven rounds, seven with real findings — severity decaying (H → M → L →
test-coverage gaps), which is what convergence looks like.
init.shbug + 3 nitpicks.in the new code — worst, a stale outage comment permanently cancelled the
pending block, i.e. pr_watch accepts a review receipt while the primary bot's review is still queued #19 re-entering through the door built to close it.
clock still wedged the gate for 30 days) and the round had introduced a new
data-loss bug in the
review.botsmigration.the same function; a third config-corruption mode; and the highest-blast-
radius code in the diff had zero tests.
the replacement "ACTION NEEDED" message walked inline-list adopters into the
corruption it replaced.
and that the no-PyYAML assertions covered 21 of 25 tokens while claiming 25.
"supported" that
kitconfigcannot parse at all — so the newly style-correctadvice was inert for an adopter whose entire marker list was already being
ignored.
Also caught by running rather than reading: making
append_to_sectionreturnnon-zero on "section not found" aborted
init.shunderset -euon any configmissing an optional section — it would have broken adoption entirely.
Skew safety
Blockers are strictly additive:
done(the legacy alias an olderdev_session.shreads) can only go true→false, so per-file engine skew makesmerges wait, never fire early. Pinned by
test_bot_blockers_only_ever_tighten_the_merge_gate. (That is anengine-version property, not a wall-clock one — a pending bot's blocker does
clear as its grace window expires.)
Verification
Tests 202 → 285. Coverage pins each half of the design against the other, and
the config migrations are now table-driven over eight real config shapes
(2-space, 4-space, flush-indent, inline flow, decoy section, header with
trailing space, multi-line item, no trailing newline) × corruption, idempotency,
exactly-once, and per-style instruction.
Review note: merge gate under
safety-critical-changes.md— operator-mergeonly. Known gaps are documented in
pr-watch.mdrather than left implicit: thepre-registration race window,
signal: unavailable, and older-engine clock reset.Closes #19
Closes #23
https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp