fix(lease): a present-but-empty schema field is refused, not read as the oldest - #948
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe lease parser now refuses bodies with an empty Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The lease parser now rejects malformed empty schema fields while preserving supported legacy inputs, and the requested pull-request read permission is explicitly scoped. No actionable merge risk remains. 🚥 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 |
…the oldest
`parse_body` splits each line on `": "`, so a line written `schema:` — the
field present, the value empty — never reaches the arm that parses the major.
It fell through the empty-value block's `_ => {}`, left `schema` at `None`,
and the reading below turns `None` into `BODY_SCHEMA`. A body whose writer
disagrees with us about what the field IS was parsed loosely and acted on,
which is the exact failure the major was added to stop.
`None => BODY_SCHEMA` is right for a body written BEFORE the field existed. It
is wrong for one carrying the field and saying nothing in it, and the two are
indistinguishable once the line is dropped on the floor.
The existing case asserts this property and passed throughout: its fixture is
`schema: tomorrow`, which DOES contain `": "`, reaches the match arm, fails
`parse::<u32>()`, and takes the refusal path. Two spellings of one case took
different branches and the suite pinned the branch that already worked.
`Body::render` always writes a number, so nothing in this crate emits a bare
`schema:`; the exposure is a future writer, a hand-edited ref, or a partial
write. It is worth closing because it is a fail-open arm inside a mechanism
whose entire purpose is to fail closed.
`//MUTANT empty-schema-reads-as-oldest` reverts the new arm. Measured: the
declared case dies under it and the two twins — the absent-field hinge and a
good `schema: 1` body — stay green, so the refusal is keyed to the emptiness
rather than to the field being present.
Closes CLOUD-1792
`mcp__github__pull_request_read` is the one MCP call this session made repeatedly (8 times) that was neither already in `permissions.allow` nor covered by a broader entry: it is how a land reads CI and review state off a PR, and it prompted on every one. It is read-only, so it widens nothing a land could not already observe. No other candidate survived the scan — every Bash hit was already subsumed by an existing wildcard or auto-allowed by the harness, the whole `mcp__serena` server is allowed, and Linear is granted at the connector. Nothing was removed, and nothing was added to `deny` or `ask`. Whether this grant actually reaches the remote harness is CLOUD-1426's question, not this commit's answer — the entry is written where the repo keeps its grants and that row owns the layer. Refs: CLOUD-1426
wenzowski
force-pushed
the
claude/batten-pr-934-takeover-mtdtnq
branch
from
September 11, 2026 21:31
840251c to
728aa84
Compare
wenzowski
marked this pull request as ready for review
September 11, 2026 21:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Author
|
/fast-forward |
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.
Body::schemaexists so a reader can refuse a lease body it does not speak. Anempty value defeated it.
parse_bodysplits each line on": ". A line writtenschema:— the fieldpresent, the value empty — has no
": ", so it never reached the arm thatparses the major. It fell through the empty-value block's
_ => {}, leftschemaatNone, and the reading below turnsNoneintoBODY_SCHEMA. Abody whose writer disagrees with us about what the field is was parsed
loosely and acted on: the exact failure the major was added to stop.
None => BODY_SCHEMAis right for a body written before the field existed.It is wrong for one carrying the field and saying nothing in it, and once the
line is dropped on the floor the two are indistinguishable.
Why the existing case missed it
an_unreadable_major_is_refused_rather_than_treated_as_the_oldestasserts thisproperty and passed throughout. Its fixture is
schema: tomorrow, which doescontain
": ", so it reaches the match arm, failsparse::<u32>(), and takesthe refusal path. Two spellings of one case took different branches and the
suite pinned the branch that already worked.
The fix
One arm —
"schema" => schema = Some(None)— routing the empty value to thesame refusal an unparseable one takes. No new state, no second matcher.
Evidence
//MUTANT empty-schema-reads-as-oldestreverts the arm. Measured under it: thedeclared case
a_body_carrying_an_empty_major_is_refuseddies, and the twoanti-vacuity twins stay green — the absent-field hinge
(
a_body_with_no_major_at_all_still_reads_as_the_oldest, without which the fixis satisfied by refusing everything and stopping the fleet on deploy) and a
good
schema: 1body. So the refusal is keyed to the emptiness, not to thefield being present.
Full suite green (
mise run test, exit 0).Blast radius
Body::renderalways writes a number, so nothing in this crate emits a bareschema:. The exposure is a future writer, a hand-edited ref, or a partialwrite. It is worth closing because it is a fail-open arm inside a mechanism
whose entire purpose is to fail closed.
Also here
One unrelated commit:
mcp__github__pull_request_readadded topermissions.allow. Read-only, and the only candidate from a scan of thissession's tool calls that was not already covered by an existing entry.
Still owed from #934
lease::coolinghas no writer — nothing recordspoisoned_aton a red CIwait, so the cooldown predicate is landed but unreachable.
lease::noticehas no sender verb — the stand-down field, mint and readerare landed; no verb writes one.
Closes CLOUD-1792
DO-NOT-CLOSE CLOUD-1426
That row asks whether a committed
permissions.allowentry reaches the remoteharness at all — its own measurement shows
mcp__serena__*granted in this fileand prompting anyway. This commit writes one more entry into the same file; it
does not answer the question, and this session gave it fresh evidence rather
than a fix (
mcp__Linear__save_issueprompted here despite the connector beingset to always-allow). The row stays open.
https://claude.ai/code/session_014zmrMLGEsPxiTyFRq28uXX