chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
Close and reopen refuse what no reader would count
.changeset/close-checks-the-item.mdnew+7−0
| @@ -0,0 +1,7 @@ | ||
--- | ||
default: patch | ||
--- | ||
| ||
# Close and reopen refuse what no reader would count | ||
| ||
`pull close`, `pull reopen`, `issue close` and `issue reopen` (and their MCP tools and the pages' buttons) used to write a state record for any URI and say it worked, even for an issue that does not exist, such as one named under the wrong account, or for someone who may not change it. Readers ignored those records, so nothing changed but a stray record in your account. Now they read the item first, and refuse one that is not there, a caller who is not its author, the owner or a collaborator, a merged pull (`pull revert` undoes one), and a state it is in already. | ||
crates/knotless/src/forge.rs+47−0
| @@ -2258,6 +2258,11 @@ pub async fn dismiss( | ||
| ||
/// Closes or reopens a pull or issue; or, for a comment that starts a | ||
/// thread on a pull's lines, resolves it or opens it again ([`resolve`]). | ||
/// The item is read first, from its author's repo, with the newest states | ||
/// of those who may write them and of the caller, read from their repos: | ||
/// one that is not there or not about this repository is refused, as is a | ||
/// caller who may not change its state, a merged pull, and a state it is | ||
/// in already. So nothing is written that readers would only ignore. | ||
pub async fn set_state(ctx: &Ctx, target: &RepoUrl, subject: &str, state: &str) -> Result<String> { | ||
if parse_uri(subject)?.1 == nsid::COMMENT { | ||
return match state { | ||
| @@ -2271,6 +2276,48 @@ pub async fn set_state(ctx: &Ctx, target: &RepoUrl, subject: &str, state: &str) | ||
let t = Target::open(ctx, target).await?; | ||
let subject = t.named(subject); | ||
let subject = subject.as_str(); | ||
t.writable()?; | ||
let me = session::me(ctx).await?; | ||
// A state written a moment ago counts before the appview hears of it, | ||
// the caller's own too: closing and at once reopening reads it closed. | ||
let (around, mut fresh, mine) = tokio::join!( | ||
t.around(ctx, subject), | ||
t.fresh_entries(ctx), | ||
t.newest(ctx, &me.did, nsid::STATE) | ||
); | ||
let mine = mine | ||
.into_iter() | ||
.filter(|e| !fresh.iter().any(|f| f.uri == e.uri)); | ||
fresh.extend(mine.collect::<Vec<_>>()); | ||
let entries = freshened(around?.0, fresh); | ||
let item = thread(&t.record, t.remote.did(), &entries, subject)?.item; | ||
let verb = if state == "closed" { "close" } else { "reopen" }; | ||
// A hidden member of the organization is told why, as a merge tells | ||
// them; the item's author needs no membership. | ||
if me.did != item.author { | ||
let doing = if state == "closed" { | ||
"closing" | ||
} else { | ||
"reopening" | ||
}; | ||
crate::org::needs_public(ctx, t.facts(), &me, doing).await?; | ||
} | ||
if !allowed( | ||
&t.record, | ||
t.remote.did(), | ||
&item.author, | ||
&me.did, | ||
state, | ||
&keys::tid(), | ||
) { | ||
bail!("only {subject}'s author, and {target}'s owner and collaborators, may {verb} it"); | ||
} | ||
if item.state == "merged" { | ||
bail!("{subject} is merged: `pull revert` undoes what it brought in"); | ||
} | ||
if item.state == state { | ||
bail!("{subject} is {state} already"); | ||
} | ||
let record = StateRecord { | ||
repo: t.reference(), | ||
subject: subject.to_string(), | ||
crates/knotless/tests/oauth.rs+8−3
| @@ -121,7 +121,6 @@ fn an_oauth_login_writes_only_what_knotless_keeps() { | ||
s.git(&work, &["push", "-q", &url, "main:topic"]); | ||
let issue = ok(&["issue", &url, "Scoped"]); | ||
ok(&["comment", "add", &url, &issue, "Seen"]); | ||
ok(&["pull", "close", &url, &issue]); | ||
ok(&["mod", "hide", &url, &issue]); | ||
ok(&["pull", "create", &url, "main", &url, "topic", "Topic"]); | ||
assert_eq!( | ||
| @@ -136,12 +135,18 @@ fn an_oauth_login_writes_only_what_knotless_keeps() { | ||
let me = stored(&s); | ||
let ctx = Ctx::new(s.env()); | ||
let mine = knotless::url::RepoUrl::parse(&format!("knotless://{}/_", me.did)).unwrap(); | ||
let refused = rt().block_on(async { | ||
let (closed, refused) = rt().block_on(async { | ||
let remote = knotless::remote::Remote::connect(&ctx, &mine).await.unwrap(); | ||
// Closing reads the item's state from an appview, which this net | ||
// has none of, so the state record is written as closing writes it. | ||
let state = json!({"$type": "dev.knotless.state", "subject": issue, "state": "closed", "createdAt": "2026-01-01T00:00:00Z"}); | ||
let write = knotless::remote::create("dev.knotless.state", "3kclosed", state); | ||
let closed = remote.apply(vec![write], &me.auth()).await; | ||
let post = json!({"$type": "app.bsky.feed.post", "text": "hi", "createdAt": "2026-01-01T00:00:00Z"}); | ||
let write = knotless::remote::create("app.bsky.feed.post", "3kscoped", post); | ||
remote.apply(vec![write], &me.auth()).await | ||
(closed, remote.apply(vec![write], &me.auth()).await) | ||
}); | ||
closed.unwrap(); | ||
let err = format!("{:#}", refused.unwrap_err()); | ||
assert!(err.contains("app.bsky.feed.post"), "{err}"); | ||
} | ||
crates/knotless-serve/tests/forge.rs+31−10
| @@ -244,6 +244,9 @@ fn forks_pulls_reviews_and_merges() { | ||
assert!(made.trim_end().ends_with(pull.trim()), "{made}"); | ||
listed(&owner, &url, &format!("pull\tmerged\t{pull}")); | ||
fails(&owner, &["pull", "merge", &url, &pull]); | ||
// Merged, it is not reopened: a revert undoes it. | ||
let (reopened, _, err) = owner.knotless(&["pull", "reopen", &url, &pull], b""); | ||
assert!(!reopened && err.contains("is merged"), "{err}"); | ||
| ||
// The base moved: a merge commit with both parents. | ||
owner.git(&work, &["pull", "-q", &url, "main"]); | ||
| @@ -1546,14 +1549,32 @@ fn issues_and_who_may_close_them() { | ||
listed(&owner, &url, &quiet); | ||
assert_eq!(ok(&author, &["issue", "close", &url, &issue]), "closed"); | ||
listed(&owner, &url, &format!("issue\tclosed\t{issue}")); | ||
// A stranger's reopening counts for nothing; the owner's does. | ||
ok(&stranger, &["issue", "reopen", &url, &issue]); | ||
std::thread::sleep(Duration::from_millis(500)); | ||
let listing = ok(&owner, &["issue", "list", &url]); | ||
assert!( | ||
listing.contains(&format!("issue\tclosed\t{issue}")), | ||
"{listing}" | ||
// Nothing is written that readers would only ignore: a stranger's | ||
// reopening, closing what is closed already, or an issue that is not | ||
// there, as one named under the wrong account. | ||
let refused = |who: &Sandbox, args: &[&str], why: &str| { | ||
let (done, _, err) = who.knotless(args, b""); | ||
assert!(!done && err.contains(why), "{args:?}: {err}"); | ||
}; | ||
refused( | ||
&stranger, | ||
&["issue", "reopen", &url, &issue], | ||
"may reopen it", | ||
); | ||
refused( | ||
&author, | ||
&["issue", "close", &url, &issue], | ||
"is closed already", | ||
); | ||
let elsewhere = issue.replace(&author.account.did, &stranger.account.did); | ||
refused( | ||
&stranger, | ||
&["issue", "close", &url, &elsewhere], | ||
"no pull or issue", | ||
); | ||
assert!(stranger.records("dev.knotless.state").is_empty()); | ||
assert_eq!(author.records("dev.knotless.state").len(), 1); | ||
// The owner's does. | ||
ok(&owner, &["issue", "reopen", &url, &issue]); | ||
listed(&owner, &url, &format!("issue\topen\t{issue}")); | ||
assert_eq!(ok(&owner, &["pull", "list", &url]), ""); | ||
| @@ -2117,10 +2138,10 @@ fn moving_to_another_account() { | ||
assert_eq!(found["owner"]["did"], heir.account.did.as_str()); | ||
| ||
// Its issues came with it, and what the old owner did before the move | ||
// still counts; nothing they do after it does. | ||
// still counts; nothing they do after it does, so it is refused. | ||
listed(&heir, &new, &format!("issue\tclosed\t{closed}")); | ||
ok(&owner, &["pull", "close", &url, &open]); | ||
std::thread::sleep(Duration::from_secs(1)); | ||
let (shut, _, err) = owner.knotless(&["pull", "close", &url, &open], b""); | ||
assert!(!shut && err.contains("may close it"), "{err}"); | ||
listed(&heir, &new, &format!("issue\topen\t{open}")); | ||
ok(&heir, &["comment", "add", &new, &open, "moved along"]); | ||
ok(&heir, &["pull", "close", &new, &open]); | ||
docs/design/collaboration.md+8−4
| @@ -328,10 +328,14 @@ a collaborator: one named on the repository record, or a member of the | ||
[organization](gateway.md#organizations) the owner is, as the reader can | ||
see them; and a `merged` or `closed` one also from a runner the | ||
repository names. On a public repository an organization's member counts | ||
only when public, so knotless refuses a hidden member's merge, label or | ||
queueing there before writing, which no reader would count. The tools do | ||
not otherwise check permission when writing, and readers ignore a record | ||
from anyone else. | ||
only when public, so knotless refuses a hidden member's merge, label, | ||
queueing, close or reopen there before writing, which no reader would | ||
count. Close and reopen read the item first, from its author's repo, with | ||
the newest states of those who may write them and of the caller, and | ||
refuse one that is not there, a caller no reader would count, a merged | ||
pull, and a state the item is in already, so they write nothing a reader | ||
would ignore. Anyone can still write a state record with other tools, and | ||
readers ignore one from anyone else. | ||
| ||
| State | Item's author | Owner | Collaborator | Named runner | Written by | | ||
| -------- | ------------- | ----- | ------------ | ------------ | ----------------- | | ||
docs/reference/cli.md+1−1
| @@ -158,7 +158,7 @@ A pull's `<at-uri>` is the one `pull create` and `pull list` print. | ||
| `pull diff <url> <at-uri>` | the unified diff a pull would apply: its head, in whichever repository it is, against where it left its base | | ||
| `pull template <url> [<base>]` | what the repository asks a pull's title and body to follow, into the default branch or `<base>` ([templates](../guide/collaborating.md#write-from-the-repositorys-templates)) | | ||
| `pull edit <url> <at-uri> [--title <title>] [-m <body> [--image <file>]...] [--draft \| --ready] [--reviewer <handle>]... [--no-reviewer <handle>]...` | change your pull, issue or comment; `-m ''` drops a pull's or issue's body, `-m -` reads it from stdin; the pictures it keeps are those the new body shows. For a pull, `--draft` makes it a draft and `--ready` marks it ready, asking its reviewers again as of now; `--reviewer` asks another account to review it and `--no-reviewer` stops asking one. Any of these on an issue or comment fails `<at-uri> is not a pull` | | ||
| `pull close <url> <at-uri>` / `pull reopen <url> <at-uri>` | change an item's state | | ||
| `pull close <url> <at-uri>` / `pull reopen <url> <at-uri>` | change an item's state, as its author, the owner or a collaborator; refuses an item that is not there, a merged pull and a state it is in already | | ||
| `pull label <url> <at-uri> <label>... [--undo]` | as owner, collaborator or a triager of their scope, put labels on a pull or take them off with `--undo`; prints those it carries now | | ||
| `pull assign <url> <at-uri> <handle>... [--undo]` | as owner or collaborator, assign a pull to accounts, or with `--undo` take them off it; prints those it is assigned to now ([collaboration.md](../design/collaboration.md#assignees)) | | ||
| `pull react <url> <at-uri> <content> [--undo]` | react to a pull, issue or comment with `+1`, `-1`, `laugh`, `hooray`, `confused`, `heart`, `rocket` or `eyes`, printing the reaction's `at://` URI; one already there is kept; `--undo` takes it back, printing `taken back`; on a private repository the reaction is written in its space, as a comment is | | ||
docs/reference/mcp.md+1−1
| @@ -120,7 +120,7 @@ Parameters beyond a flag's name: | ||
| `pull_show`, `issue_show` | `knotless pull show`, `knotless issue show` | a pull or issue and its comments; on a pull, what it brings in and whether it merges cleanly, and on an issue `mentionedBy`, the pulls whose title or body names it, oldest first, each `{pull, closes}`, `closes` true where it names it after a closing keyword | | ||
| `pull_diff` | `knotless pull diff` | the diff a pull would apply, numbered as `comment_add`'s `lines` take them | | ||
| `pull_edit`, `issue_edit` | `knotless pull edit`, `knotless issue edit` | change your pull's or issue's title and text; `pull_edit` also whether your pull is a draft (`draft`, `ready`) and whom it asks to review it (`reviewers`, `no_reviewers`) | | ||
| `pull_close`, `pull_reopen`, `issue_close`, `issue_reopen` | `knotless pull close` and the rest | change an item's state: its author, the owner or a collaborator | | ||
| `pull_close`, `pull_reopen`, `issue_close`, `issue_reopen` | `knotless pull close` and the rest | change an item's state: its author, the owner or a collaborator; refuses an item that is not there, a merged pull and a state it is in already | | ||
| `pull_label`, `issue_label` | `knotless pull label`, `knotless issue label` | put labels on a pull or issue as owner, collaborator or triager, or with `undo` take them off | | ||
| `pull_assign`, `issue_assign` | `knotless pull assign`, `knotless issue assign` | assign a pull or issue to accounts, or with `undo` take them off; as in [collaboration.md](../design/collaboration.md#assignees), the owner and collaborators assign. Answers `assignees` by DID | | ||
| `pull_ask_review` | `knotless pull ask-review` | ask accounts to review a pull, or with `undo` stop asking: the owner and collaborators, and a pull's author in the pull. Answers `assignees` and `reviewers`, by DID | | ||