chadtmiller.com/knotless

Switch repository

Loading…

What changes

pull close, pull reopen, issue close and issue reopen now read the item before writing anything. The same goes for their MCP tools and the pages' close button, since all three go through forge::set_state. Each of these is now refused, with nothing written:

• An item that isn't there. That includes an issue named under the wrong account. The error is no pull or issue … on …, the same one comment add already gives.
• A caller no reader would count. allowed decides, the same rule readers apply. The error is only <item>'s author, and <repo>'s owner and collaborators, may close it. A hidden member of an organization gets org::needs_public's message instead, as merge, label and queue already give. The item's author needs no membership.
• A merged pull. The error points to pull revert.
• A state the item is in already. The error is <item> is closed already.

The item's state is read as merge reads it: the appview's entries, with the newest state records of the owner, the collaborators and the lander read from their repos (fresh_entries), plus the caller's own. Without the caller's own, closing and reopening at once would read the item still open and refuse the reopen.

Why

Before this, set_state wrote a state record for whatever URI it was given and answered closed. Readers then ignored it. While triaging issues, an issue close on a URI with the wrong author DID reported success and left a state record in the closer's account pointing at nothing. A stranger's reopen did the same, and so did a reopen of a merged pull. That last one is worse: the owner's open replaced merged and cleared the merge.

This settles three items of Pulls and states: small correctness gaps: set_state writing without checking permission, reopen working on a merged pull, and the missing-item case added to it.

I chose to refuse a state the item is in already, rather than answer success without writing, so that a script learns its view of the item was out of date.

How it was checked

crates/knotless-serve/tests/forge.rs:

• issues_and_who_may_close_them now expects four things:
  • a stranger's reopen is refused;
  • the author's second close is refused as closed already;
  • a close of the issue's key under the stranger's DID is refused as no pull or issue;
  • the stranger's account holds no dev.knotless.state record, and the author's holds one.
• forks_pulls_reviews_and_merges expects a reopen of the merged pull to be refused.
• moving_to_another_account expected the old owner's close after the move to pass and count for nothing. It now expects it to be refused.

crates/knotless/tests/oauth.rs: an_oauth_login_writes_only_what_knotless_keeps used pull close to show that the OAuth scope lets a state record through. Its net has no appview, so close can no longer run there. The test now writes the state record itself, as close would.

Nothing was built or run here. The first CI run passed every test binary but oauth (test), which the second commit fixes; the rest is left to CI.

Shortcomings

• On a public repository, close and reopen now need an appview, as show, merge and retarget already do. With KNOTLESS_APPVIEW set empty they fail, where they used to write blind.
• The readers still accept a reopen of a merged pull, or any state string from an allowed author, written by other tools. That is the separate "Readers accept any state" item of the issue above.
• A hidden item's states are dropped by moderation, so a hidden item reads open. Closing one works, but reopening a hidden closed one is refused as open already.
• Close and reopen now cost a read of the repository's forge entries and a few listRecords, as merge does, rather than one write.
• The stray state record left during triage stays in the account. Nothing in knotless deletes a state record.

set_state now reads the item first, then refuses a missing item, a caller allowed would not count, a merged pull, and a state the item is already in. The state is read as merge reads it, plus the caller's own newest state records. Docs, the changeset and the tests are updated, and I found nothing wrong in the logic.

• Order of checks: org::needs_public runs before allowed, so a hidden org member gets the org message and not the generic one. That matches the PR body.
• Moved repositories: the old owner's close after a transfer is refused, because allowed is given keys::tid() for now. That is the intended behaviour, and the changed test expects it.
• Not checked: I didn't build or run anything. I also didn't confirm that every existing caller that closes or reopens twice in a row still passes. The one I read, knotless-run/tests/run.rs, alternates close and reopen, so it is fine. CI will show the rest.
• Conflict with the "already closed" refusal: a script that retries a close after a timeout now gets an error. You chose that on purpose and documented it, so it is acceptable.

The change since my approval only touches tests/oauth.rs. The test's pull close call is gone because close now reads the item from an appview, and that net has none. The test writes the state record directly instead, so it still checks that the OAuth scope allows dev.knotless.state and refuses app.bsky.feed.post. My earlier approval stands. I didn't build or run anything.