From 9c82b31e7a55266d07bcbf7faa5b4a872ebbf108 Mon Sep 17 00:00:00 2001 From: Tim Disney Date: Wed, 29 Jul 2026 16:58:29 -0700 Subject: [PATCH] Unify goal endings with typed close records (#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Unify goal endings with typed close records Co-Authored-By: codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie) * Answer the review on the goal-ending change The fold no longer lets a legacy `archiveGoal{archived:false}` reopen a goal its author had closed. `goal.closed` and `archiveGoal` are two independent legacy dimensions, so an archive record can only ADD its park; a `closeGoal` — the one axis — still decides the state outright in either direction, and a goal carrying both legacy signals reports the closure that actually happened. Also from the review: - The golden scenario now carries all three ending signals on one goal, tied on the instant across collections, so the 200-permutation whole-index test covers the new record; `archive.test.mjs` becomes `ending.test.mjs` with the fold matrix and 100 permutations of its own. The fixture ends a goal each way, so demo mode draws every disposition and an `endedAt`. - `liveTargets` delegating to `activeGoals()` is stated as the deliberate reader-side change it is — docstring, CHANGELOG, and an exact count in `verdicts.test.ts` rather than an existence check. - Dropping the `archiveGoal` and `goal?action=update` permissions narrows an already-issued token's scope, so a scope refusal now says that signing out and in again is the fix, and the CHANGELOG says so too. - Only `completed` keeps the sage ✓; `dropped`, `superseded`, `parked` and an unknown word are `flat`, through one shared `endingBadge`. - `--disposition` is refused when the lexicon could not carry it and warned when it is outside the documented vocabulary; `goal unarchive` no longer writes a `parked` annotation onto a reopen. - The disposition picker is a styled `select.sl` with a paired label. - `isClosed`, `ownsGoal` and `RepoWriteAction`'s unreachable `'update'` are gone. - design.md §4/§7/§12 specify the new model and name what it supersedes; phase6-ui-plan §3.5 and its 6.7 note are marked superseded; the stale "two endings"/"sanctioned owner edit" comments across core, atproto, sidecar, daemon and the UI now describe the model in the tree. Co-Authored-By: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) --------- Co-authored-by: codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie) Co-authored-by: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) --- CHANGELOG.md | 26 ++ docs/design.md | 27 +- docs/phase6-ui-plan.md | 19 +- packages/atproto/src/client.ts | 8 +- packages/core/src/fixture.ts | 88 +++++- packages/core/src/generated/records.ts | 45 +++ packages/core/src/materializer.ts | 99 +++++-- packages/core/src/store.ts | 24 +- packages/core/test/archive.test.mjs | 87 ------ packages/core/test/ending.test.mjs | 280 ++++++++++++++++++ packages/core/test/fixture.test.mjs | 31 +- .../core/test/fixtures/invalid-records.json | 1 + .../core/test/fixtures/valid-records.json | 7 + packages/core/test/scenario.ts | 38 +++ packages/daemon/src/auto-review.ts | 12 +- packages/daemon/src/check-dispatch.ts | 6 +- packages/daemon/src/dispatch.ts | 4 +- packages/daemon/src/merge-poll.ts | 4 +- packages/daemon/test/auto-review.test.mjs | 2 +- packages/daemon/test/merge-poll.test.mjs | 4 +- packages/lexicons/README.md | 25 ++ .../com.disnetdev.radial.closeGoal.json | 20 ++ packages/sidecar/src/cli.ts | 6 +- packages/sidecar/src/commands.ts | 99 +++++-- packages/sidecar/test/human-loop.test.mjs | 105 +++++-- packages/ui/src/app.css | 4 + packages/ui/src/lib/admin.test.ts | 29 +- packages/ui/src/lib/admin.ts | 60 ++-- packages/ui/src/lib/client-metadata.test.ts | 14 +- packages/ui/src/lib/client-metadata.ts | 32 +- .../ui/src/lib/components/GoalActions.svelte | 91 +++--- packages/ui/src/lib/components/GoalRow.svelte | 9 +- packages/ui/src/lib/components/Rail.svelte | 8 +- .../ui/src/lib/components/RequestBar.svelte | 6 +- packages/ui/src/lib/session.test.ts | 4 +- packages/ui/src/lib/space.test.ts | 51 +++- packages/ui/src/lib/space.ts | 11 +- packages/ui/src/lib/units.ts | 49 +-- packages/ui/src/lib/verdicts.test.ts | 24 +- packages/ui/src/lib/write.test.ts | 79 ++--- packages/ui/src/lib/write.ts | 52 +++- .../ui/src/routes/g/[did]/[rkey]/+page.svelte | 14 +- packages/ui/src/routes/logbook/+page.svelte | 25 +- .../ui/src/routes/p/[project]/+page.svelte | 12 +- 44 files changed, 1198 insertions(+), 443 deletions(-) delete mode 100644 packages/core/test/archive.test.mjs create mode 100644 packages/core/test/ending.test.mjs create mode 100644 packages/lexicons/lexicons/com.disnetdev.radial.closeGoal.json diff --git a/CHANGELOG.md b/CHANGELOG.md index fa322ce..cfbd693 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,31 @@ # Changelog +## Unreleased + +- **A goal has one ending, and it is the additive `closeGoal` overlay.** Any active member can end a + goal as `completed`, `dropped`, `superseded` or `parked`, and can reopen it — including a goal whose + author is unavailable, which the old author-only closure made impossible. The two controls the goal + page used to carry ("Close" and "Archive") are one, with a picker asking what happened to the work. + Legacy data stays readable: a `goal.closed` edit reads as `completed`, an `archiveGoal` record reads + as `parked`, and a legacy *un*-archive cannot cancel a closure it never knew about. Nothing writes + either spelling any more. Four notes for an operator upgrading a live space: + + - **Upgrade materializers before browsers begin writing `closeGoal`.** An older daemon does not + recognize the new collection, so it reads an ended goal as open and can keep dispatching it. + - **Already-signed-in browser sessions must sign out and sign in again.** The OAuth scope is derived + from the collections the app writes, so a token issued before this release was granted + `archiveGoal` and `goal?action=update` and *not* `closeGoal`: the first attempt to end a goal is + refused by the PDS. The app now recognizes that refusal and says so; re-authorizing is the fix. + - **The smart lists changed, deliberately.** An ended goal's units leave "For me", "Awaiting", + "With an agent" and the review queue, where a *closed* goal's used to stay. Readers and the daemon + now share one definition of a shelved goal; what an ended goal still owes is visible in the + Logbook, which is also where previously-archived goals appear for the first time (they were on no + list at all before). + - **`radial goal archive|unarchive` are deprecated aliases** that write a `closeGoal` and warn. + `radial goal close --disposition` refuses a value the lexicon cannot carry (over 32 characters, or + empty) rather than writing a record every materializer would drop, and warns on a word outside the + documented vocabulary while still writing it. + - **Refreshable ChatGPT-managed authentication for Codex turns.** A run may now opt into `"codexAuth": { "mode": "chatgpt-session" }` and create a fresh instance-owned login with `radiald codex login`. Radial serializes turns sharing the login, mounts only a scratch diff --git a/docs/design.md b/docs/design.md index 89f8f4d..59a0d2f 100644 --- a/docs/design.md +++ b/docs/design.md @@ -61,7 +61,11 @@ com.disnetdev.radial.join {space} com.disnetdev.radial.project {space, name, gitUrl, defaultBranch, checks: [{name, command}], — machine-verifiable autoReview: {: bool}} — per-type trigger default (§7) -com.disnetdev.radial.goal {space, project, title, body, closed?} — author = any member; owner-edited +com.disnetdev.radial.goal {space, project, title, body, closed?} — author = any member; `closed` is + LEGACY: read forever, never written + (superseded by closeGoal, §7) +com.disnetdev.radial.closeGoal {goal, closed, — a goal's one ending; author = any + disposition?, createdAt} active member, reversible (§7) com.disnetdev.radial.artifactType {space, name, brief, outputSpec, scope: goal|project} — registry entry; author must be admin com.disnetdev.radial.artifactRequest{goal | project, — exactly one, per the type's scope (§8) @@ -92,10 +96,10 @@ com.disnetdev.radial.agent {handleName, harness, models: [{name, costHi Notes: -- **Primary record + typed referencing records**, the same shape atproto itself uses (post + replies + likes). The goal author edits their own goal record (atproto records are owner-mutable, so goal closure is just an owner edit); everyone else contributes requests, artifacts, reviews, messages that reference it. Materialization is a deterministic fold over typed records, not op replay. -- **Records are immutable by convention**; goal closure, claim lease renewal (`expiresAt` **and** `renewedAt`, moved together and bounded — §6), and an agent profile's republish are the sanctioned exceptions. atproto owners *can* rewrite records in place, so materializers keep the earliest repo revision they observe, independent of arrival order, and flag other CID changes on the same record as edit annotations — a review's pinned strongref stays meaningful even if the PDS now serves different bytes. The third exception is what makes §11's "capability sets are added and retired by writing records" true of a profile that already exists: an `agent` record is one operator's self-description, in that agent's own repo, pinned by nothing, so the latest revision wins wholesale. Keeping the earliest instead made `radiald init --update` invisible to every observer that had already synced it — including the daemon whose own drift warning tells the operator to run it. -- **A project's `autoReview` map is mutated via standalone `com.disnetdev.radial.setAutoReview` records** (admin-authored, `{project, type, enabled}`), folded latest-wins per type over the project record's own field — never by editing the project record, which is strongref-pinned and would drop the edit (the fold keeps its earliest CID). Same primary-record-plus-standalone-edit divergence as archiving a goal via a standalone `archiveGoal` record rather than an owner edit. -- **A project's `name`, `gitUrl` and `defaultBranch` move the same way**, via `com.disnetdev.radial.editProject` (`{project, name?, gitUrl?, defaultBranch?}`), folded latest-wins **per field** so an edit naming only the name cannot revert a concurrent edit of the remote. `com.disnetdev.radial.archiveProject` (`{project, archived}`) shelves a project and, with it, every goal beneath it: agents stop dispatching, reviewing, checking and merge-polling anything under it, and the UI's rail and smart lists stop listing it. Both are trusted **only from the project record's author** — not from an admin, and unlike `archiveGoal`, not from any active member: what a project points at decides which repository every future turn clones, which is the thing itself rather than an annotation about where attention should go. Both are reversible (a later record wins), and `checks` has no overlay — it stays fixed at creation. Consumers read the effective values off `ProjectView.name/gitUrl/defaultBranch`; `target.value` remains the bytes at `target.cid`, which is what keeps every goal's strongref to the project honest. +- **Primary record + typed referencing records**, the same shape atproto itself uses (post + replies + likes). Nothing edits a primary record: a goal's ending, a project's name and its archived state are each a standalone overlay record folded over the primary one (atproto records *are* owner-mutable, which is why the rule is a convention the fold enforces rather than a permission). Everyone contributes requests, artifacts, reviews, messages that reference it. Materialization is a deterministic fold over typed records, not op replay. +- **Records are immutable by convention**; claim lease renewal (`expiresAt` **and** `renewedAt`, moved together and bounded — §6), and an agent profile's republish are the sanctioned exceptions. atproto owners *can* rewrite records in place, so materializers keep the earliest repo revision they observe, independent of arrival order, and flag other CID changes on the same record as edit annotations — a review's pinned strongref stays meaningful even if the PDS now serves different bytes. The third exception is what makes §11's "capability sets are added and retired by writing records" true of a profile that already exists: an `agent` record is one operator's self-description, in that agent's own repo, pinned by nothing, so the latest revision wins wholesale. Keeping the earliest instead made `radiald init --update` invisible to every observer that had already synced it — including the daemon whose own drift warning tells the operator to run it. +- **A project's `autoReview` map is mutated via standalone `com.disnetdev.radial.setAutoReview` records** (admin-authored, `{project, type, enabled}`), folded latest-wins per type over the project record's own field — never by editing the project record, which is strongref-pinned and would drop the edit (the fold keeps its earliest CID). Same primary-record-plus-standalone-overlay shape as a goal's ending via a standalone `closeGoal` record rather than an owner edit. +- **A project's `name`, `gitUrl` and `defaultBranch` move the same way**, via `com.disnetdev.radial.editProject` (`{project, name?, gitUrl?, defaultBranch?}`), folded latest-wins **per field** so an edit naming only the name cannot revert a concurrent edit of the remote. `com.disnetdev.radial.archiveProject` (`{project, archived}`) shelves a project and, with it, every goal beneath it: agents stop dispatching, reviewing, checking and merge-polling anything under it, and the UI's rail and smart lists stop listing it. Both are trusted **only from the project record's author** — not from an admin, and unlike a goal's ending, not from any active member: what a project points at decides which repository every future turn clones, which is the thing itself rather than an annotation about where attention should go. Both are reversible (a later record wins), and `checks` has no overlay — it stays fixed at creation. Consumers read the effective values off `ProjectView.name/gitUrl/defaultBranch`; `target.value` remains the bytes at `target.cid`, which is what keeps every goal's strongref to the project honest. - **Large bodies overflow to blobs.** atproto records have size ceilings; an artifact `body` that exceeds them is stored as a blob, with the record carrying a summary and the blob ref. - **Artifact types are data, not lexicon.** `plan` and `implementation` ship as built-ins; a space admin adds `security-review` or `architecture-diagram` by writing a registry record with a brief template and expected output shape. New capabilities are configuration, and the UI grows a button, not a schema. - **Scope is a property of the type.** Registry types declare where their deliverables live: `goal` (the default — plans, implementations) or `project` — **system artifacts** (architecture docs, ADRs) that outlive any one goal and collect in the UI's System section (§8). A request's goal-or-project reference matches its type's scope; `basedOn` carries cross-links between scopes. @@ -137,7 +141,14 @@ Every artifact moves through the same four moments. None of them is a state the **Future trigger rules.** Auto-review generalizes: it is a degenerate trigger rule (`on artifact of type X → request review`). Later rules — auto-revise-once on `request_changes`, auto-implement on plan approval — can layer onto the same substrate as opt-in per-project config. The lesson carried over from the state-machine design: **any rule that can cause generation must carry an iteration bound**, or two models ping-pong tokens forever. v1 sidesteps this entirely by shipping only the terminal hop. -**Goal closure** is an explicit human act (the goal author sets `closed` on their own record). An observed PR merge annotates the implementation artifact (§10) but never closes the goal by itself. +**A goal's ending** is an explicit human act, and a goal has exactly one. It is a standalone `com.disnetdev.radial.closeGoal` record — `{goal, closed, disposition?}` — which **any active member** may write, which is reversible (latest wins, so `closed: false` reopens), and which carries a one-word `disposition` saying which kind of ending it was: `completed` · `dropped` · `superseded` · `parked`. Consumers read one predicate, `GoalView.ended`; nothing in the fold branches on `disposition`, so the vocabulary can grow without two materializers disagreeing about which goals have ended (it is typed as a free string with a documented vocabulary, not a lexicon enum, for exactly that reason — `packages/lexicons/README.md` carries the contract). + +This **supersedes two earlier spellings**, both of which are still READ and neither of which is written any more: + +- `goal.closed`, an owner edit of the goal record, formerly the one sanctioned primary-record edit in the protocol. It ended the goal and could only be written by its author, because an edit lands in the author's repo or nowhere. That rule protected nothing — any member could achieve the identical effect by archiving — and it stranded a goal whose author had gone. Legacy edits are read as `completed`, with no ending instant of their own. The record store keeps adopting the "differs in `closed` alone" version for that data, and goal strongrefs stay lenient (matched by URI, not `uri#cid`) because such an edit minted a second CID. +- `com.disnetdev.radial.archiveGoal` (`{goal, archived}`), any member's reversible "stop looking at this". It is read as `parked`. It could only ever ADD that reading: a legacy un-archive cannot cancel a closure, since a writer of either record knew nothing about the other. + +The two had the identical effect on every daemon pump — one capability with two spellings, two permission rules and two answers to "where does this goal go now" — which is a choice nobody outside the fold could make. An observed PR merge annotates the implementation artifact (§10) but never ends the goal by itself. ## 8. System artifacts (project scope) @@ -273,7 +284,7 @@ The invariant that keeps this from being alternative C (§14) is narrow and test - Agents answer questions through a typed, human-authored `answer` request whose terminal record is a thread reply; the daemon never authors one, and a mention still dispatches nothing. Goal-scoped and assignee-required in v1. (§9, §10) - Artifact types are per-space registry data; `plan` and `implementation` ship as built-ins. (§4) - Provenance (`basedOn`) is recorded whenever the UI knows it, never required by the protocol. (§4) -- Goal closure is an owner edit of the goal record; merge observation annotates the artifact but doesn't close the goal. (§7, §10) +- A goal has one ending: a standalone `closeGoal` record any active member may write, reversible, carrying a `disposition` (`completed` · `dropped` · `superseded` · `parked`) nothing in the fold branches on. The legacy `goal.closed` edit and `archiveGoal` are read forever and written never. Merge observation annotates the artifact but never ends a goal. (§7, §10) - Checks run on every implementation artifact in a clean container; results annotate rather than gate. (§10) - Radial never merges; turns open PRs themselves, humans merge, the daemon observes through the (observation-only) forge adapter; GitHub is the first implementation. (§10, §13) - Turns use the operator's own GitHub auth (`gh` login reuse or OAuth device flow at init; a repo-scoped expiring PAT for operators who want scoping). Agent attribution is authoritative on-protocol and mirrored into commit trailers and PR-body footers — no forge account per agent, no GitHub App in v1. Forge-side controls (branch protection, no merge rights) plus the human merge are the blast-radius bound. The hardened tier — one manifest-created App per operator minting short-lived per-turn installation tokens — is deferred to post-v1. (§10, §13) @@ -288,7 +299,7 @@ The invariant that keeps this from being alternative C (§14) is narrow and test - System artifacts are on-protocol; an in-repo `docs/` mirror is a possible later addition, not v1. (§8) - Living-document staleness is a UI annotation, never a trigger. (§8) - Membership is an append-only event log of exact grants. Removal revokes one grant and retains nothing: all records by a DID with no active grant are ignored, making record trust independent of ingestion timing; re-adding restores trust. Availability of any member's records — active or removed — is best-effort. (§3) -- Records are immutable by convention; materializers keep the earliest observed repo revision and flag in-place edits (goal closure, claim lease renewal, and an agent profile's republish are sanctioned exceptions). Oversized bodies go to blobs. (§4) +- Records are immutable by convention; materializers keep the earliest observed repo revision and flag in-place edits (claim lease renewal and an agent profile's republish are the sanctioned exceptions; a legacy `goal.closed` edit is a third, frozen for data already written). Oversized bodies go to blobs. (§4) - Validation and signing are daemon-side; the in-container sidecar is a thin client and protocol credentials never enter containers — the only in-container secrets are a per-turn forge token and a spend-capped model key. (§2, §9, §13) - A reviewer needs no distinct DID — the same agent may review its own artifact; review value comes from the fresh per-turn context and operator-routed model diversity. (§7, §11) - Messages anchor to a goal or an artifact, and reference the request they respond to (`re`, with `declines` for turn-downs); reviews reference the request they fulfill. (§4, §6, §9) diff --git a/docs/phase6-ui-plan.md b/docs/phase6-ui-plan.md index cf2e6c2..b3d2ee4 100644 --- a/docs/phase6-ui-plan.md +++ b/docs/phase6-ui-plan.md @@ -135,6 +135,12 @@ edit, but there is no `goal close` command and `CredentialClient` has no `put`. needs one. Add `RecordWriter.put` + `goal close|reopen` to the command layer. (Archive already works and is a different thing — reversible, any member.) *Done in 6.0* — and closure turned out to need reversibility in the fold too, not just a command; see the note under 6.0. +***Superseded in 6.8*** — the parenthesis above is the mistake, not a caveat: "a different thing" +described two records with the identical effect on every daemon pump, and it is what put two +controls on the goal page. A goal now has ONE ending, the standalone `closeGoal` record, any active +member's, carrying a disposition (`parked` is what Archive meant). `goal close|reopen` write that +record; `goal archive|unarchive` are deprecated aliases for them; `RecordWriter.put` survives only +for `radiald init --update`. See design §7. **3.6 Agent records never reach the index.** `materialize()` explicitly drops `com.disnetdev.radial.agent`. The assignee picker wants exactly what that record holds: @@ -610,11 +616,14 @@ Six things doing it decided: control on it is auto-review, and it is a control *because* `setAutoReview` was made a standalone record precisely so it could change. The test asserts the project's CID does not move and no `putRecord` is sent, which is what makes that more than a claim. -- **A goal's two endings are two controls, not one.** Closing is the author's edit of their own record — - the only sanctioned edit in the protocol — so it is offered on authorship, not on role, and an admin - gets no say. Archiving is a standalone record any active member may write, and only says agents should - skip it. Both are reversible, and both live at the *bottom* of the goal page: closing a goal is the end - of reading it, and the header already carries the badge that says it happened. +- ~~**A goal's two endings are two controls, not one.**~~ *Superseded in 6.8.* The reasoning was that + closing is the author's edit of their own record — the only sanctioned edit in the protocol — while + archiving is a standalone record any active member may write that only says agents should skip the + goal. What that missed is that the two had the *identical* effect on every daemon pump, so the choice + the screen offered could not be made by anybody who was not reading the fold. There is one ending now, + one control, and a disposition picker asking what happened to the work. What survives unchanged: it is + reversible, and it lives at the *bottom* of the goal page, because ending a goal is the end of reading + it and the header already carries the badge that says it happened. - **`space create` is the write with no rule, because its author becomes the rule.** This is the 6.3 carry-over, closed. `createSpace()` is separate from `write()` for one honest reason: it resolves nothing, pins nothing, and has no repo to refresh afterwards — so it brings its own transport rather diff --git a/packages/atproto/src/client.ts b/packages/atproto/src/client.ts index 909bddc..60dd46d 100644 --- a/packages/atproto/src/client.ts +++ b/packages/atproto/src/client.ts @@ -159,9 +159,11 @@ export class RepoWriter { /** * Rewrite a record the actor already owns, in place. This is how the few sanctioned edits land — - * today only a goal's `closed` flag (design §7). `swapRecord` pins the exact CID the caller read, - * so a concurrent edit fails the write rather than silently clobbering it; pass it whenever the - * new value was derived from a version you fetched. + * today an `agent` record's republish (`radiald init --update`, §11) and a claim's lease renewal. + * A goal's `closed` flag used to be the third, and is not: an ending is a standalone `closeGoal` + * overlay now (design §7), and legacy edits are read but never written. `swapRecord` pins the exact + * CID the caller read, so a concurrent edit fails the write rather than silently clobbering it; + * pass it whenever the new value was derived from a version you fetched. * * It takes the record's full AT URI rather than a bare rkey deliberately. Only a record's own * author may edit it, and a repo write always lands in the *authenticated* repo — so given just diff --git a/packages/core/src/fixture.ts b/packages/core/src/fixture.ts index 4740aa6..7322257 100644 --- a/packages/core/src/fixture.ts +++ b/packages/core/src/fixture.ts @@ -57,6 +57,12 @@ export interface FixtureSpace { onboarding: string /** The two units the review queue is drawn from: one nobody was asked about, one asked of a human. */ ignored: string + /** Ended `dropped` — we decided not to do it. */ + budgets: string + /** Ended `superseded` — the work moved somewhere else. */ + diffViewer: string + /** Ended by a legacy `archiveGoal`, which the fold reads as `parked`. */ + permissioned: string } } @@ -269,9 +275,23 @@ export function fixtureSpace(): FixtureSpace { }) /** - * A goal the author has since closed. Closure is an edit of the goal record itself (design §7), - * so it is a second version of the same record differing in `closed` alone — not a new record, - * and it carries no timestamp of its own. + * A goal's ending: a standalone `closeGoal` record any active member may write, carrying the word + * for what happened to the work. This is how a goal ends now, and the demo shows all four words. + */ + const end = (author: string, rkey: string, goal: StoredRecord, disposition: string, at: string): void => { + put(author, rkey, { + $type: COLLECTIONS.closeGoal, + goal: ref(goal), + closed: true, + disposition, + createdAt: at, + }) + } + + /** + * A goal ended by the LEGACY owner edit: a second version of the goal record differing in `closed` + * alone. Nothing writes this any more, and records like it are in people's repos forever, so the + * demo carries one — it is how the fold's `completed`-with-no-`endedAt` reading gets on screen. */ const close = (record: StoredRecord): void => { revision += 1 @@ -283,6 +303,20 @@ export function fixtureSpace(): FixtureSpace { }) } + /** + * A goal ended by the LEGACY `archiveGoal` annotation, which the fold reads as `parked` — what + * archiving meant, said in the new vocabulary. Also the demo's only sight of a goal that the old UI + * made unreachable: archived goals were on no list at all, and this one is in the Logbook. + */ + const park = (author: string, rkey: string, goal: StoredRecord, at: string): void => { + put(author, rkey, { + $type: COLLECTIONS.archiveGoal, + goal: ref(goal), + archived: true, + createdAt: at, + }) + } + interface RequestInput { author: string rkey: string @@ -788,7 +822,9 @@ export function fixtureSpace(): FixtureSpace { ) review(reviewer, '3lbk9rev0002', u10, 'approve', '2026-07-15T10:40:00Z') merge(builder, '3lbk9mrg0001', u10, 'https://github.com/disnet/radial-ng/pull/228', '2026-07-15T17:44:00Z') - close(g5) + // Ended the way a goal ends now: a standalone record, with the word for what happened and the + // instant it happened at. Ana ends Tim's goal, which is the write the old author-only rule refused. + end(ana, '3lbk9end0001', g5, 'completed', '2026-07-15T18:02:00Z') // ── g6 · Permutation property tests, closed ───────────────────────────────────────────────── const g6 = goal( @@ -836,6 +872,9 @@ export function fixtureSpace(): FixtureSpace { ) review(reviewer, '3lbh2rev0001', u11, 'approve', '2026-07-09T15:02:00Z') merge(builder, '3lbh2mrg0001', u11, 'https://github.com/disnet/radial-ng/pull/196', '2026-07-10T09:15:00Z') + // Deliberately the LEGACY ending — the owner edit nothing writes any more. Kept so demo mode shows + // what the fold does with the data already in people's repos: ended, read as `completed`, and with + // no `endedAt`, because a rewritten goal record carries no instant of its own. close(g6) // ── g7 · Onboarding, on the other project ─────────────────────────────────────────────────── @@ -975,6 +1014,44 @@ export function fixtureSpace(): FixtureSpace { 'You wrote the rule that drops these records. Say whether this reports it honestly before it goes in front of anyone.', }) + // ── g9–g11 · the other three endings ──────────────────────────────────────────────────────── + // A goal ends in one of four ways and the Logbook says which, so the demo carries one of each. g5 + // is `completed` and g6 is the legacy edit read as `completed`; these three are the rest. They are + // titles and bodies with no work under them, which is what most abandoned goals actually look like + // — and it keeps them out of every list except the Logbook and their project's own logbook section. + const g9 = goal( + tim, + '3lbn1dd0ee60a', + radialNg, + 'Per-goal spend budgets', + 'A ceiling per goal, refused at dispatch. Deferred in v1: every generation already has a human click behind it, so the thing a budget protects against cannot happen yet (design.md §9).', + '2026-06-30T09:00:00Z', + ) + end(ana, '3lbn1dd0ee60b', g9, 'dropped', '2026-07-08T16:20:00Z') + + const g10 = goal( + ana, + '3lbn2ee1ff60c', + radialNg, + 'A native diff viewer for implementation artifacts', + 'Read the branch without leaving the goal page. Overtaken by the decision to delegate diff viewing to the forge permanently (design.md §10) — the artifact links out instead.', + '2026-07-02T11:30:00Z', + ) + end(tim, '3lbn2ee1ff60d', g10, 'superseded', '2026-07-11T10:05:00Z') + + const g11 = goal( + tim, + '3lbn3ff2gg60e', + radialNg, + 'Permissioned spaces', + 'Records are public today, and a space that is not is a different trust model rather than a setting. Worth doing, not now (design.md §13).', + '2026-07-05T14:10:00Z', + ) + // Parked with the LEGACY record: this is the one archived goal in the demo, and before the endings + // became one it was on no list at all — project page, rail and Logbook alike — which is exactly the + // defect the Logbook now fixes. The fold reads it as `parked`, with the archive's own instant. + park(ana, '3lbn3ff2gg60f', g11, '2026-07-06T08:45:00Z') + // ── system artifacts (§8) ─────────────────────────────────────────────────────────────────── // Project-scoped types. They outlive any goal, and the current version of each rides in every // turn's bundle — which is the real reason this section exists. @@ -1161,6 +1238,9 @@ export function fixtureSpace(): FixtureSpace { permutation: g6.uri, onboarding: g7.uri, ignored: g8.uri, + budgets: g9.uri, + diffViewer: g10.uri, + permissioned: g11.uri, }, } } diff --git a/packages/core/src/generated/records.ts b/packages/core/src/generated/records.ts index 0fcb822..87281d2 100644 --- a/packages/core/src/generated/records.ts +++ b/packages/core/src/generated/records.ts @@ -22,6 +22,7 @@ export const COLLECTIONS = { artifactType: "com.disnetdev.radial.artifactType", checkrun: "com.disnetdev.radial.checkrun", claim: "com.disnetdev.radial.claim", + closeGoal: "com.disnetdev.radial.closeGoal", editProject: "com.disnetdev.radial.editProject", goal: "com.disnetdev.radial.goal", join: "com.disnetdev.radial.join", @@ -131,6 +132,14 @@ export interface ClaimRecord { createdAt: string } +export interface CloseGoalRecord { + $type: "com.disnetdev.radial.closeGoal" + goal: StrongRef + closed: boolean + disposition?: string + createdAt: string +} + export interface EditProjectRecord { $type: "com.disnetdev.radial.editProject" project: StrongRef @@ -240,6 +249,7 @@ export interface RecordByCollection { [COLLECTIONS.artifactType]: ArtifactTypeRecord [COLLECTIONS.checkrun]: CheckrunRecord [COLLECTIONS.claim]: ClaimRecord + [COLLECTIONS.closeGoal]: CloseGoalRecord [COLLECTIONS.editProject]: EditProjectRecord [COLLECTIONS.goal]: GoalRecord [COLLECTIONS.join]: JoinRecord @@ -760,6 +770,41 @@ export const lexiconSchemas = [ } } }, + { + "lexicon": 1, + "id": "com.disnetdev.radial.closeGoal", + "defs": { + "main": { + "type": "record", + "key": "tid", + "record": { + "type": "object", + "required": [ + "goal", + "closed", + "createdAt" + ], + "properties": { + "goal": { + "type": "ref", + "ref": "com.atproto.repo.strongRef" + }, + "closed": { + "type": "boolean" + }, + "disposition": { + "type": "string", + "maxLength": 32 + }, + "createdAt": { + "type": "string", + "format": "datetime" + } + } + } + } + } + }, { "lexicon": 1, "id": "com.disnetdev.radial.editProject", diff --git a/packages/core/src/materializer.ts b/packages/core/src/materializer.ts index 9585b1c..270ddc3 100644 --- a/packages/core/src/materializer.ts +++ b/packages/core/src/materializer.ts @@ -9,6 +9,7 @@ import { type ArtifactTypeRecord, type CheckrunRecord, type ClaimRecord, + type CloseGoalRecord, type EditProjectRecord, type GoalRecord, type JoinRecord, @@ -86,10 +87,17 @@ export interface TargetView { } export interface GoalView extends TargetView { - // True when the most recent trusted `archiveGoal` record targeting this goal sets `archived: - // true`. Unlike `target.value.closed` (an owner-only edit), any active member may archive, and it - // is reversible — a later `archived: false` record un-archives. Agents ignore archived goals. - archived: boolean + /** + * The single predicate for whether this goal has ended — agents and readers both use it, and there + * is no second reading of "is this goal live". Any active member may write one (`closeGoal`), and + * it is reversible. Resolved from the legacy `goal.closed` base and the ending overlay pool; the + * exact rule, including why the two legacy spellings are not symmetric, is on `endingOf` below. + */ + ended: boolean + /** The ending's annotation. Unknown future values remain valid and do not affect `ended`. */ + disposition?: string + /** The overlay's timestamp; absent for a legacy `goal.closed` edit, which carries none. */ + endedAt?: string } export interface ProjectView extends TargetView { @@ -175,6 +183,15 @@ const compareRecord = ( return compareInstant(leftCreated, rightCreated) || compareCodePoints(left.uri, right.uri) } +/** + * A strongref pinning this exact record — `uri#cid`, which is the whole point of a strongref. + * + * Goals are the one exception, matched by URI alone. That is legacy, not preference: the old + * author-only closure was an in-place edit of the goal record, so it minted a second CID and every + * reference into that goal would otherwise dangle the moment somebody closed it. Nothing writes such + * an edit any more (an ending is a standalone `closeGoal` overlay, design §7), but the CIDs it minted + * have already moved, so the leniency stays. + */ const exactRef = ( reference: StrongRef | undefined, record: Pick | undefined, @@ -701,26 +718,23 @@ export function materialize(store: RecordStore, options: MaterializeOptions): Ma } } - // A goal is archived when the most recent trusted `archiveGoal` record targeting it sets - // `archived: true`. Any active member may archive (trust already dropped non-members), and it is - // reversible: a later record with `archived: false` un-archives. Matched by goal uri, not uri#cid - // — the author editing `closed` mints a new cid, and an archive must survive that (goals are the - // one ref type the index resolves leniently; see exactRef). "Latest" is by (createdAt, uri). - const archiveByGoal = new Map>() - const associatedArchiveUris: string[] = [] - for (const archive of trust.records) { - if (archive.collection !== COLLECTIONS.archiveGoal) continue - const goalUri = (archive.value as ArchiveGoalRecord).goal.uri + // Goal endings are one latest-wins overlay pool. `archiveGoal` remains readable as the legacy + // spelling of parked; new writes use `closeGoal`. Matching by URI survives legacy goal rewrites. + // Which record wins is decided here; what winning MEANS is decided where the view is built, and + // the two legacy spellings are not symmetric there — see `goalViews` below. + type GoalEndingRecord = IndexedRecord + const endingByGoal = new Map() + const associatedEndingUris: string[] = [] + for (const ending of trust.records) { + if (ending.collection !== COLLECTIONS.archiveGoal && ending.collection !== COLLECTIONS.closeGoal) continue + const goalUri = (ending.value as ArchiveGoalRecord | CloseGoalRecord).goal.uri if (!goalByUri.has(goalUri)) continue - associatedArchiveUris.push(archive.uri) - const current = archiveByGoal.get(goalUri) - if (!current || compareRecord(current, archive) < 0) { - archiveByGoal.set(goalUri, archive as IndexedRecord) + associatedEndingUris.push(ending.uri) + const current = endingByGoal.get(goalUri) + if (!current || compareRecord(current, ending) < 0) { + endingByGoal.set(goalUri, ending as GoalEndingRecord) } } - const archivedGoalUris = new Set( - [...archiveByGoal].filter(([, archive]) => archive.value.archived === true).map(([uri]) => uri), - ) // A project's per-type auto-review default is overlaid by standalone `setAutoReview` records so // the setting can change without rewriting the (strongref-pinned) project record. Unlike @@ -859,8 +873,38 @@ export function materialize(store: RecordStore, options: MaterializeOptions): Ma asOf, }) + // A goal's ending: the legacy `closed` edit as the base, overlaid by the pool above. + // + // `closeGoal` is the one axis — a member saying what happened to this work — so it decides `ended` + // outright, in either direction, and reopening a legacy-closed goal needs no rewrite of the goal + // record. The two legacy spellings are NOT one axis and are not read as one: `goal.closed` says the + // work is over, `archiveGoal` says stop looking at this, and a writer of either knew nothing about + // the other. So an `archiveGoal` winner can only ADD its park — a legacy `archived: false` cannot + // cancel a closure it was never talking about, and the goal an author closed stays ended (and stays + // `completed`, which is the ending that actually happened) however its archive flag has been + // toggled. The asymmetry is the whole reason the winner is not simply read off `value`. + const endingOf = (goal: IndexedRecord): Pick => { + // No timestamp of its own: the goal record's `createdAt` is when the goal was opened, not when it + // was closed, so a legacy closure has no `endedAt` to report. Its word is `completed` — what the + // old UI's Close button meant. + const legacy = goal.value.closed === true ? { ended: true, disposition: 'completed' } : { ended: false } + const ending = endingByGoal.get(goal.uri) + if (!ending) return legacy + if (ending.collection === COLLECTIONS.closeGoal) { + const close = ending.value as CloseGoalRecord + if (!close.closed) return { ended: false } + return { + ended: true, + ...(close.disposition === undefined ? {} : { disposition: close.disposition }), + endedAt: close.createdAt, + } + } + if ((ending.value as ArchiveGoalRecord).archived !== true) return legacy + return legacy.ended ? legacy : { ended: true, disposition: 'parked', endedAt: ending.value.createdAt } + } + const goalViews: GoalView[] = goals - .map((goal) => ({ ...buildView(makeInput(goal)), archived: archivedGoalUris.has(goal.uri) })) + .map((goal) => ({ ...buildView(makeInput(goal)), ...endingOf(goal) })) .sort((a, b) => compareCodePoints(a.target.uri, b.target.uri)) const projectViews: ProjectView[] = projects .map((project) => { @@ -907,7 +951,7 @@ export function materialize(store: RecordStore, options: MaterializeOptions): Ma ...outOfContract.map((entry) => entry.uri), ...messages.map((record) => record.uri), ...associatedRetractUris, - ...associatedArchiveUris, + ...associatedEndingUris, ...associatedAutoReviewUris, ...associatedEditUris, ...associatedProjectArchiveUris, @@ -956,7 +1000,7 @@ export const activeProjects = (index: MaterializedIndex): ProjectView[] => index.projects.filter((view) => !view.archived) /** - * Goals agents act on: open, not archived, and not hanging off an archived project. + * Goals agents act on: not ended and not hanging off an archived project. * * Archiving a project shelves the work under it too — a turn cloning a repository the space has put * away is the thing archiving exists to stop — so a goal whose project is archived is set aside even @@ -966,10 +1010,5 @@ export function activeGoals(index: MaterializedIndex): GoalView[] { const shelved = new Set( index.projects.filter((view) => view.archived).map((view) => view.target.uri), ) - return index.goals.filter( - (view) => - !view.archived && - view.target.value.closed !== true && - !shelved.has(view.target.value.project.uri), - ) + return index.goals.filter((view) => !view.ended && !shelved.has(view.target.value.project.uri)) } diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 448cd4b..25f1668 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -129,15 +129,21 @@ export function select(versions: StoredRecord[]): { record: StoredRecord; edit?: const rejectedCids: string[] = [] if (original.collection === COLLECTIONS.goal) { - // `closed` is the one sanctioned owner edit of a goal (design §7), and it is reversible: a - // human may close a goal and reopen it. Every other field is immutable, so a version is adopted - // only when it differs from the original in `closed` ALONE — that guard is what keeps a silent - // body rewrite from riding along. Among the versions that pass it, the LATEST repo revision - // wins in either direction. Latest-wins is what makes closure converge: `rev` is a per-repo TID - // and a goal has exactly one author, so it totally orders the versions, and an observer who - // only ever saw the newest version reads the same `closed` as one that watched every edit land. - // (A monotonic close-only rule would agree with this whenever closes never reverse, and diverge - // between those two observers the moment one did.) + // `closed` was the one sanctioned owner edit of a goal, and this branch is FROZEN rather than + // live: a goal's ending is a standalone `closeGoal` overlay now (design §7), nothing writes a + // `closed` edit any more, and this exists because such edits are in people's repos forever. It + // cannot be removed — a materializer that stopped adopting them would read a goal somebody + // closed years ago as open, and dispatch it. + // + // The rule, for that legacy data: every other field is immutable, so a version is adopted only + // when it differs from the original in `closed` ALONE — that guard is what keeps a silent body + // rewrite from riding along. Among the versions that pass it, the LATEST repo revision wins in + // either direction, because closure was reversible from the same button. Latest-wins is what + // makes it converge: `rev` is a per-repo TID and a goal has exactly one author, so it totally + // orders the versions, and an observer who only ever saw the newest version reads the same + // `closed` as one that watched every edit land. (A monotonic close-only rule would agree with + // this whenever closes never reverse, and diverge between those two observers the moment one + // did.) const baseline = stableWithout(original.value, 'closed') for (const version of sorted.slice(1)) { if (stableWithout(version.value, 'closed') === baseline) { diff --git a/packages/core/test/archive.test.mjs b/packages/core/test/archive.test.mjs deleted file mode 100644 index 97c557f..0000000 --- a/packages/core/test/archive.test.mjs +++ /dev/null @@ -1,87 +0,0 @@ -import assert from 'node:assert/strict' -import { describe, it } from 'node:test' -import { materialize } from '../dist/materializer.js' -import { MemoryRecordStore } from '../dist/store.js' -import { goldenScenario } from '../dist/test/scenario.js' -import { COLLECTIONS } from '../dist/generated/records.js' - -const AS_OF = '2026-02-01T00:00:00Z' -const ROOT = 'did:plc:root' -const HUMAN = 'did:plc:human' -const AGENT_C = 'did:plc:agentc' -const REMOVED = 'did:plc:removed' - -/** An archiveGoal record authored by `did` toggling `archived` on the goal at `goalUri`. */ -function archive(did, rkey, goalUri, archived, createdAt) { - return { - did, - collection: COLLECTIONS.archiveGoal, - rkey, - uri: `at://${did}/${COLLECTIONS.archiveGoal}/${rkey}`, - cid: `cid-${rkey}`, - rev: '9999999999999', - value: { - $type: COLLECTIONS.archiveGoal, - // The goal ref intentionally carries no meaningful cid: archive is matched by goal uri, so it - // survives the author's `closed` edit (which mints a new cid). The golden goal is closed. - goal: { uri: goalUri, cid: 'cid-any' }, - archived, - createdAt, - }, - } -} - -function buildWith(...extra) { - const scenario = goldenScenario() - const store = new MemoryRecordStore() - ;[...scenario.records, ...extra].forEach((record) => store.put(record)) - return { scenario, index: materialize(store, { spaceUri: scenario.spaceUri, asOf: AS_OF }) } -} - -const goalOf = (index, uri) => index.goals.find((view) => view.target.uri === uri) - -describe('materializer goal archival', () => { - it('is not archived before any archiveGoal record', () => { - const { scenario, index } = buildWith() - assert.equal(goalOf(index, scenario.goalUri).archived, false) - }) - - it('archives when any active member (here an agent) writes archived:true', () => { - const { scenario, index } = buildWith() - const goalUri = scenario.goalUri - const next = buildWith(archive(AGENT_C, 'arch-1', goalUri, true, '2026-01-01T01:00:00Z')) - assert.equal(goalOf(next.index, goalUri).archived, true) - }) - - it('archival survives the author closing the goal (matched by uri, not cid)', () => { - const { scenario, index } = buildWith() - // The golden primary goal is closed (a second cid); archival still resolves onto it. - assert.equal(goalOf(index, scenario.goalUri).target.value.closed, true) - const next = buildWith(archive(HUMAN, 'arch-2', scenario.goalUri, true, '2026-01-01T01:00:00Z')) - assert.equal(goalOf(next.index, scenario.goalUri).archived, true) - }) - - it('un-archives when a later record sets archived:false (latest wins)', () => { - const { scenario } = buildWith() - const goalUri = scenario.goalUri - const next = buildWith( - archive(HUMAN, 'arch-on', goalUri, true, '2026-01-01T01:00:00Z'), - archive(AGENT_C, 'arch-off', goalUri, false, '2026-01-01T02:00:00Z'), - ) - assert.equal(goalOf(next.index, goalUri).archived, false) - - // Order is by (createdAt, uri), not ingestion order: an earlier un-archive loses to a later archive. - const reversed = buildWith( - archive(HUMAN, 'arch-off-early', goalUri, false, '2026-01-01T01:00:00Z'), - archive(AGENT_C, 'arch-on-late', goalUri, true, '2026-01-01T02:00:00Z'), - ) - assert.equal(goalOf(reversed.index, goalUri).archived, true) - }) - - it('ignores an archiveGoal from a non-member', () => { - const { scenario } = buildWith() - // REMOVED was granted then removed in the golden scenario; trust drops their records. - const next = buildWith(archive(REMOVED, 'arch-stranger', scenario.goalUri, true, '2026-01-01T01:00:00Z')) - assert.equal(goalOf(next.index, scenario.goalUri).archived, false) - }) -}) diff --git a/packages/core/test/ending.test.mjs b/packages/core/test/ending.test.mjs new file mode 100644 index 0000000..b5e4eb2 --- /dev/null +++ b/packages/core/test/ending.test.mjs @@ -0,0 +1,280 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { activeGoals, materialize } from '../dist/materializer.js' +import { MemoryRecordStore } from '../dist/store.js' +import { goldenScenario } from '../dist/test/scenario.js' +import { COLLECTIONS } from '../dist/generated/records.js' + +const AS_OF = '2026-02-01T00:00:00Z' +const HUMAN = 'did:plc:human' +const AGENT_C = 'did:plc:agentc' +const REMOVED = 'did:plc:removed' + +/** An archiveGoal record authored by `did` toggling `archived` on the goal at `goalUri`. */ +function archive(did, rkey, goalUri, archived, createdAt) { + return { + did, + collection: COLLECTIONS.archiveGoal, + rkey, + uri: `at://${did}/${COLLECTIONS.archiveGoal}/${rkey}`, + cid: `cid-${rkey}`, + rev: '9999999999999', + value: { + $type: COLLECTIONS.archiveGoal, + // The goal ref intentionally carries no meaningful cid: an ending is matched by goal uri, so it + // survives the author's legacy `closed` edit (which mints a new cid). The golden goal is closed. + goal: { uri: goalUri, cid: 'cid-any' }, + archived, + createdAt, + }, + } +} + +/** A closeGoal record: the one ending any active member writes now. */ +function close(did, rkey, goalUri, closed, createdAt, disposition) { + return { + did, + collection: COLLECTIONS.closeGoal, + rkey, + uri: `at://${did}/${COLLECTIONS.closeGoal}/${rkey}`, + cid: `cid-${rkey}`, + rev: '9999999999999', + value: { + $type: COLLECTIONS.closeGoal, + goal: { uri: goalUri, cid: 'cid-any' }, + closed, + ...(disposition === undefined ? {} : { disposition }), + createdAt, + }, + } +} + +function indexOf(records, scenario) { + const store = new MemoryRecordStore() + records.forEach((record) => store.put(record)) + return materialize(store, { spaceUri: scenario.spaceUri, asOf: AS_OF }) +} + +function buildWith(...extra) { + const scenario = goldenScenario() + return { scenario, index: indexOf([...scenario.records, ...extra], scenario) } +} + +const goalOf = (index, uri) => index.goals.find((view) => view.target.uri === uri) + +/** + * The golden scenario carries all three ending signals on its primary goal (`scenario.ts`), and its + * `closeGoal` wins the pool on the URI tie-break. Tests that want a bare legacy signal drop the two + * golden ending records first and add their own. + */ +function withoutGoldenEndings(scenario, ...extra) { + const records = scenario.records.filter( + (record) => record.uri !== scenario.endingUri && record.uri !== scenario.legacyParkUri, + ) + return indexOf([...records, ...extra], scenario) +} + +/** …and, for the parked cases, without the legacy `closed` edit under them either. */ +function withoutGoldenClosure(scenario, ...extra) { + const records = scenario.records.filter( + (record) => + record.uri !== scenario.endingUri && + record.uri !== scenario.legacyParkUri && + record.cid !== 'cid-goal-closed', + ) + return indexOf([...records, ...extra], scenario) +} + +describe('a goal has one ending', () => { + it('reads the pool winner: the golden goal ends as the disposition its closeGoal states', () => { + const { scenario, index } = buildWith() + const goal = goalOf(index, scenario.goalUri) + assert.equal(goal.ended, true) + assert.equal(goal.disposition, 'dropped') + assert.equal(goal.endedAt, '2026-01-01T00:16:00.000Z') + // One predicate: an ended goal is not a goal agents act on. + assert.equal(activeGoals(index).some((view) => view.target.uri === scenario.goalUri), false) + // Both ending records are associated, so neither shows up in the diagnostics. + assert.equal(index.ignored.some((entry) => entry.uri === scenario.endingUri), false) + assert.equal(index.ignored.some((entry) => entry.uri === scenario.legacyParkUri), false) + }) + + it('reads the legacy closed flag alone as completed, with no endedAt', () => { + const { scenario } = buildWith() + const goal = goalOf(withoutGoldenEndings(scenario), scenario.goalUri) + assert.equal(goal.target.value.closed, true) + assert.equal(goal.ended, true) + assert.equal(goal.disposition, 'completed') + assert.equal(goal.endedAt, undefined) + }) + + it('ends when any active member (here an agent) writes a legacy archived:true', () => { + const { scenario } = buildWith() + const index = withoutGoldenEndings( + scenario, + archive(AGENT_C, 'arch-1', scenario.goalUri, true, '2026-01-01T01:00:00Z'), + ) + // The legacy closure the golden goal already carries is the ending that actually happened, so it + // is the word reported — an archive laid on top of it adds nothing to say. + assert.equal(goalOf(index, scenario.goalUri).ended, true) + assert.equal(goalOf(index, scenario.goalUri).disposition, 'completed') + }) + + it('reads a legacy archive as parked when there is no closure under it', () => { + const { scenario } = buildWith() + const index = withoutGoldenClosure( + scenario, + archive(HUMAN, 'arch-park', scenario.goalUri, true, '2026-01-01T01:00:00Z'), + ) + const goal = goalOf(index, scenario.goalUri) + assert.equal(goal.target.value.closed, undefined) + assert.equal(goal.ended, true) + assert.equal(goal.disposition, 'parked') + assert.equal(goal.endedAt, '2026-01-01T01:00:00Z') + }) + + it('un-parks with a later legacy archived:false, latest wins by (createdAt, uri)', () => { + const { scenario } = buildWith() + const off = withoutGoldenClosure( + scenario, + archive(HUMAN, 'arch-on', scenario.goalUri, true, '2026-01-01T01:00:00Z'), + archive(AGENT_C, 'arch-off', scenario.goalUri, false, '2026-01-01T02:00:00Z'), + ) + assert.equal(goalOf(off, scenario.goalUri).ended, false) + + // Order is by (createdAt, uri), not ingestion order: an earlier un-archive loses to a later archive. + const on = withoutGoldenClosure( + scenario, + archive(HUMAN, 'arch-off-early', scenario.goalUri, false, '2026-01-01T01:00:00Z'), + archive(AGENT_C, 'arch-on-late', scenario.goalUri, true, '2026-01-01T02:00:00Z'), + ) + assert.equal(goalOf(on, scenario.goalUri).ended, true) + }) + + it('does not let a legacy un-archive resurrect a goal its author closed', () => { + // The regression the fold's asymmetry exists for. `goal.closed` and `archiveGoal` are two + // independent legacy dimensions — the work is over, and stop looking at this — and a writer of + // either knew nothing about the other. A legacy un-archive (a member archiving, then changing + // their mind, on a goal the author has since closed) must not reopen it: that would put the goal + // back in the rail and back in front of every daemon pump. + const { scenario } = buildWith() + const index = withoutGoldenEndings( + scenario, + archive(HUMAN, 'arch-on', scenario.goalUri, true, '2026-01-01T01:00:00Z'), + archive(AGENT_C, 'arch-off', scenario.goalUri, false, '2026-01-01T02:00:00Z'), + ) + const goal = goalOf(index, scenario.goalUri) + assert.equal(goal.ended, true) + assert.equal(goal.disposition, 'completed') + assert.equal(activeGoals(index).some((view) => view.target.uri === scenario.goalUri), false) + }) + + it('lets a closeGoal reopen a legacy-closed goal without rewriting the goal record', () => { + const { scenario } = buildWith() + const index = withoutGoldenEndings( + scenario, + close(AGENT_C, 'reopen', scenario.goalUri, false, '2026-01-01T02:00:00Z'), + ) + const goal = goalOf(index, scenario.goalUri) + assert.equal(goal.target.value.closed, true) + assert.equal(goal.ended, false) + assert.equal(goal.disposition, undefined) + assert.equal(goal.endedAt, undefined) + assert.equal(activeGoals(index).some((view) => view.target.uri === scenario.goalUri), true) + }) + + it('folds closeGoal and archiveGoal in one latest-wins pool, in either direction', () => { + const { scenario } = buildWith() + const reopened = withoutGoldenEndings( + scenario, + archive(HUMAN, 'legacy-later', scenario.goalUri, true, '2026-01-01T03:00:00Z'), + close(AGENT_C, 'close-earlier', scenario.goalUri, true, '2026-01-01T02:00:00Z', 'dropped'), + close(HUMAN, 'reopen-latest', scenario.goalUri, false, '2026-01-01T04:00:00Z'), + ) + assert.equal(goalOf(reopened, scenario.goalUri).ended, false) + + // …and a legacy archive landing after a modern reopen sets the goal aside again: the member who + // wrote it is saying stop looking at this, in the only word their client had. On this goal the + // legacy closure underneath is still the ending that happened, so `completed` is the word. + const parked = withoutGoldenEndings( + scenario, + close(HUMAN, 'reopen-first', scenario.goalUri, false, '2026-01-01T02:00:00Z'), + archive(AGENT_C, 'legacy-last', scenario.goalUri, true, '2026-01-01T03:00:00Z'), + ) + assert.equal(goalOf(parked, scenario.goalUri).ended, true) + assert.equal(goalOf(parked, scenario.goalUri).disposition, 'completed') + }) + + it('ends for an omitted or unknown disposition', () => { + const { scenario } = buildWith() + const unknown = withoutGoldenEndings( + scenario, + close(HUMAN, 'future-word', scenario.goalUri, true, '2026-01-01T02:00:00Z', 'hibernating'), + ) + assert.equal(goalOf(unknown, scenario.goalUri).ended, true) + assert.equal(goalOf(unknown, scenario.goalUri).disposition, 'hibernating') + + const unstated = withoutGoldenEndings( + scenario, + close(HUMAN, 'no-word', scenario.goalUri, true, '2026-01-01T02:00:00Z'), + ) + assert.equal(goalOf(unstated, scenario.goalUri).ended, true) + assert.equal(goalOf(unstated, scenario.goalUri).disposition, undefined) + assert.equal(goalOf(unstated, scenario.goalUri).endedAt, '2026-01-01T02:00:00Z') + }) + + it('ignores an ending from a non-member, in either collection', () => { + const { scenario } = buildWith() + // REMOVED was granted then removed in the golden scenario; trust drops their records. + const reopen = withoutGoldenEndings( + scenario, + close(REMOVED, 'stranger-close', scenario.goalUri, false, '2026-01-01T03:00:00Z'), + ) + assert.equal(goalOf(reopen, scenario.goalUri).ended, true) + assert.equal(goalOf(reopen, scenario.goalUri).disposition, 'completed') + assert.equal(reopen.ignored.some((entry) => entry.uri.includes('/stranger-close')), true) + + const park = withoutGoldenEndings( + scenario, + archive(REMOVED, 'stranger-arch', scenario.goalUri, true, '2026-01-01T03:00:00Z'), + ) + assert.equal(goalOf(park, scenario.goalUri).disposition, 'completed') + assert.equal(park.ignored.some((entry) => entry.uri.includes('/stranger-arch')), true) + }) + + it('converges over 100 deterministic arrival permutations of all three signals', () => { + // The property the whole change rests on: two materializers handed the same ending signals — a + // legacy closure, legacy archives and `closeGoal` records, toggled in both directions — agree + // about which goals have ended and how. Whole-index deep equality, so `disposition` and `endedAt` + // are covered too. (`materializer.test.mjs` shuffles the golden scenario 200 ways and so carries + // the three signals it holds; this narrows the same property onto a set that toggles the ending.) + const { scenario } = buildWith() + const records = [ + ...scenario.records, + archive(HUMAN, 'perm-park-on', scenario.goalUri, true, '2026-01-01T02:00:00Z'), + close(AGENT_C, 'perm-end', scenario.goalUri, true, '2026-01-01T03:00:00Z', 'superseded'), + archive(AGENT_C, 'perm-park-off', scenario.goalUri, false, '2026-01-01T04:00:00Z'), + close(HUMAN, 'perm-reopen', scenario.goalUri, false, '2026-01-01T05:00:00Z'), + close(REMOVED, 'perm-stranger', scenario.goalUri, true, '2026-01-01T06:00:00Z', 'dropped'), + ] + const expected = indexOf(records, scenario) + // The last trusted word is HUMAN's reopen, over a goal whose own record still says `closed: true`. + assert.equal(goalOf(expected, scenario.goalUri).ended, false) + + let state = 7 + const random = () => { + state ^= state << 13 + state ^= state >>> 17 + state ^= state << 5 + return state >>> 0 + } + for (let seed = 1; seed <= 100; seed += 1) { + const shuffled = [...records] + for (let position = shuffled.length - 1; position > 0; position -= 1) { + const target = random() % (position + 1) + ;[shuffled[position], shuffled[target]] = [shuffled[target], shuffled[position]] + } + assert.deepEqual(indexOf(shuffled, scenario), expected, `seed ${seed}`) + } + }) +}) diff --git a/packages/core/test/fixture.test.mjs b/packages/core/test/fixture.test.mjs index 88826c3..28f7b88 100644 --- a/packages/core/test/fixture.test.mjs +++ b/packages/core/test/fixture.test.mjs @@ -33,10 +33,10 @@ describe('the comp, as records', () => { // Every record resolves into the space. An entry here means the comp drew something the fold // does not accept — a dangling ref, a non-member author, a request that anchors nowhere. assert.deepEqual(view.ignored, []) - // The only edits are the two goal closures, and a sanctioned closure is not flagged. + // The one in-place rewrite is the legacy goal closure, and a sanctioned closure is not flagged. assert.deepEqual(view.edits, []) assert.equal(view.members.length, 6) - assert.equal(view.goals.length, 8) + assert.equal(view.goals.length, 11) assert.equal(view.projects.length, 2) // Three humans, and only two of them have ever opened the space. The third is granted-and-absent, // which is the state a space is in the moment after somebody is added — the members list draws it, @@ -209,16 +209,29 @@ describe('the comp, as records', () => { assert.equal(timeline(later, fixture.goals.onboarding)[0].state, 'open') }) - it('closes the two goals the comp shows as done', () => { + it('ends five goals, one for each way a goal can end', () => { const view = index() - const closed = view.goals.filter((goal) => goal.target.value.closed === true) + const ended = view.goals.filter((goal) => goal.ended) assert.deepEqual( - closed.map((goal) => goal.target.uri).sort(), - [fixture.goals.autoReview, fixture.goals.permutation].sort(), + ended.map((goal) => [goal.target.uri, goal.disposition]).sort(), + [ + // A standalone `closeGoal`, written by a member who is not the goal's author. + [fixture.goals.autoReview, 'completed'], + // The legacy owner edit, read as `completed` — the word the old Close button meant. + [fixture.goals.permutation, 'completed'], + [fixture.goals.budgets, 'dropped'], + [fixture.goals.diffViewer, 'superseded'], + // The legacy `archiveGoal`, read as `parked`. + [fixture.goals.permissioned, 'parked'], + ].sort(), ) - // Both are fully judged, which is what fills their pies. - for (const goal of closed) { - const counted = tally(timeline(view, goal.target.uri)) + // Every ending but the legacy edit carries the instant it happened — the Logbook sorts on it. + const withInstant = ended.filter((goal) => goal.endedAt !== undefined).map((goal) => goal.target.uri) + assert.deepEqual(withInstant.includes(fixture.goals.permutation), false) + assert.equal(withInstant.length, 4) + // The two with work under them are fully judged, which is what fills their pies. + for (const uri of [fixture.goals.autoReview, fixture.goals.permutation]) { + const counted = tally(timeline(view, uri)) assert.equal(counted.judged, counted.total) } }) diff --git a/packages/core/test/fixtures/invalid-records.json b/packages/core/test/fixtures/invalid-records.json index cb53ffc..fd320c5 100644 --- a/packages/core/test/fixtures/invalid-records.json +++ b/packages/core/test/fixtures/invalid-records.json @@ -11,6 +11,7 @@ "com.disnetdev.radial.claim": {"$type":"com.disnetdev.radial.claim","request":{"uri":"at://x","cid":"x"},"expiresAt":"bad","createdAt":"bad"}, "com.disnetdev.radial.retractRequest": {"$type":"com.disnetdev.radial.retractRequest","request":{"uri":"nope","cid":""},"createdAt":"bad"}, "com.disnetdev.radial.archiveGoal": {"$type":"com.disnetdev.radial.archiveGoal","goal":{"uri":"nope","cid":""},"archived":"yes","createdAt":"bad"}, + "com.disnetdev.radial.closeGoal": {"$type":"com.disnetdev.radial.closeGoal","goal":{"uri":"nope","cid":""},"closed":"yes","createdAt":"bad"}, "com.disnetdev.radial.setAutoReview": {"$type":"com.disnetdev.radial.setAutoReview","project":{"uri":"nope","cid":""},"type":"plan","enabled":"yes","createdAt":"bad"}, "com.disnetdev.radial.artifact": {"$type":"com.disnetdev.radial.artifact","request":{"uri":"at://x","cid":"x"},"type":"plan","body":"x","links":{},"createdAt":"bad"}, "com.disnetdev.radial.review": {"$type":"com.disnetdev.radial.review","subject":{"uri":"at://x","cid":"x"},"verdict":"maybe","findings":[],"createdAt":"bad"}, diff --git a/packages/core/test/fixtures/valid-records.json b/packages/core/test/fixtures/valid-records.json index 31753c0..dd0a7e9 100644 --- a/packages/core/test/fixtures/valid-records.json +++ b/packages/core/test/fixtures/valid-records.json @@ -139,6 +139,13 @@ "re": {"uri": "at://did:plc:human/com.disnetdev.radial.artifactRequest/request", "cid": "cid-request"}, "createdAt": "2026-01-01T00:00:10Z" }, + "com.disnetdev.radial.closeGoal": { + "$type": "com.disnetdev.radial.closeGoal", + "goal": {"uri": "at://did:plc:human/com.disnetdev.radial.goal/goal", "cid": "cid-goal"}, + "closed": true, + "disposition": "completed", + "createdAt": "2026-01-01T00:00:10Z" + }, "com.disnetdev.radial.agent": { "$type": "com.disnetdev.radial.agent", "handleName": "planner", diff --git a/packages/core/test/scenario.ts b/packages/core/test/scenario.ts index 824559b..d77808d 100644 --- a/packages/core/test/scenario.ts +++ b/packages/core/test/scenario.ts @@ -68,6 +68,10 @@ export interface GoldenScenario { answerQuestionRef: StrongRef answerRequestUri: string answerReplyUri: string + /** The `closeGoal` that ends the primary goal as `dropped` — the pool winner on the URI tie-break. */ + endingUri: string + /** A legacy `archiveGoal` on the same goal, tied with it on the instant and losing on the URI. */ + legacyParkUri: string } export function goldenScenario(): GoldenScenario { @@ -660,6 +664,38 @@ export function goldenScenario(): GoldenScenario { }, 12, 'cid-goal-closed'), ) + // ── one goal, three ending signals (design §7) ──────────────────────────────────────────────── + // + // The primary goal carries all three spellings of "this work is over", which is what makes the + // whole-index permutation test the real gate on the ending fold: + // + // - the legacy owner edit above (`closed: true`), which has no timestamp of its own; + // - a legacy `archiveGoal` from AGENT_B — any active member's, and matched by goal URI rather + // than uri#cid, so it resolves onto a goal whose `closed` edit minted a second cid; + // - a `closeGoal` from AGENT_C, the modern one-axis record, at the SAME INSTANT as the archive in + // the other legal RFC-3339 spelling. Tied on time, the pool falls through to the URI, and + // `at://did:plc:agentc/…closeGoal/end-goal` beats `at://did:plc:agentb/…archiveGoal/park-legacy` + // — so the goal reads `dropped`, and the cross-collection tie-break plus the mixed-precision + // comparison are both covered by every arrival permutation rather than by a fixed-order unit + // test. + const legacyPark = add( + stored(AGENT_B, 'park-legacy', { + $type: COLLECTIONS.archiveGoal, + goal: { uri: goal.uri, cid: 'cid-goal-stale' }, + archived: true, + createdAt: '2026-01-01T00:16:00Z', + }, 24), + ) + const ending = add( + stored(AGENT_C, 'end-goal', { + $type: COLLECTIONS.closeGoal, + goal: { uri: goal.uri, cid: 'cid-goal-stale' }, + closed: true, + disposition: 'dropped', + createdAt: '2026-01-01T00:16:00.000Z', + }, 25), + ) + return { records, spaceUri: space.uri, @@ -688,5 +724,7 @@ export function goldenScenario(): GoldenScenario { answerQuestionRef: ref(answerQuestion), answerRequestUri: answerRequest.uri, answerReplyUri: answerReply.uri, + endingUri: ending.uri, + legacyParkUri: legacyPark.uri, } } diff --git a/packages/daemon/src/auto-review.ts b/packages/daemon/src/auto-review.ts index 277abfc..081c861 100644 --- a/packages/daemon/src/auto-review.ts +++ b/packages/daemon/src/auto-review.ts @@ -109,7 +109,7 @@ export interface AutoReviewCandidate { /** * Pure selection gate: which artifact versions should have an auto-review request written right now. * Scans EVERY version in every goal's and project's artifacts (each prev-chained version is a - * distinct subject), skipping closed/archived goals. An artifact is a candidate iff: + * distinct subject), skipping ended goals and archived projects. An artifact is a candidate iff: * - its effective auto-review config is on: `request.autoReview ?? projectDefault[type] ?? false`, * where `request` is the artifact's fulfilling request (its `request` backref; always resolvable, * the materializer drops artifacts otherwise) and the project default is read through @@ -195,7 +195,7 @@ export function selectAutoReviewCandidates( } // Shelved goals are set aside: agents stop reviewing their artifacts. `activeGoals` carries the - // whole rule — closed, archived, or hanging off a project its author archived. + // whole rule — ended, or hanging off a project its author archived. for (const goalView of activeGoals(index)) { const projectView = projectViewByUri.get(goalView.target.value.project.uri) scan(goalView, projectView ? projectAutoReviewConfig(projectView) : {}) @@ -211,7 +211,7 @@ export function selectAutoReviewCandidates( * The artifact versions the ledger is allowed to mark as observed: every artifact under a live * project plus every artifact under a live goal. Artifacts under a shelved target are deliberately * EXCLUDED — they stay unobserved so that if the goal reopens, or the project is un-archived (or the - * artifact and its closure arrived in the same first sync), they become candidates then, instead of + * artifact and its ending arrived in the same first sync), they become candidates then, instead of * being permanently wedged out by an early mark. */ const observableArtifacts = (index: MaterializedIndex): Array> => [ @@ -255,7 +255,7 @@ export interface AutoReviewTriggerDeps { * write still in flight from an overlapping pump, and subjects deferred for want of an eligible * reviewer. A failed write therefore leaves its subject unmarked and the next pump reselects and * retries it (the deterministic rkey + adoption make that retry safe against duplicates). - * Closed/archived-goal artifacts are never marked, so they become candidates if the goal reopens. The + * Artifacts under an ended goal are never marked, so they become candidates if the goal reopens. The * first-ever snapshot of a space seeds the ledger without triggering anything (pre-existing artifacts * are never auto-reviewed); thereafter an artifact is a candidate only if it is not yet in the ledger * — which also covers artifacts that landed during daemon downtime (still reviewed on restart) and @@ -349,8 +349,8 @@ export class AutoReviewTrigger { : [] const candidateKeys = new Set(candidates.map((candidate) => refKey(candidate.artifact))) - // AFTER selection, record every OBSERVABLE artifact version (open-goal + project; never - // closed/archived) EXCEPT the ones the ledger must not swallow: this tick's candidates (marked + // AFTER selection, record every OBSERVABLE artifact version (live goal + project; never + // under an ended goal) EXCEPT the ones the ledger must not swallow: this tick's candidates (marked // only once their write lands — see `#write`), subjects deferred for want of a reviewer (must fire // once one is configured), and subjects with a write still in flight from an overlapping pump (so // a failed write always retries). This keeps a failed/crashed/deferred subject reselectable. diff --git a/packages/daemon/src/check-dispatch.ts b/packages/daemon/src/check-dispatch.ts index 94d83cd..b3ba396 100644 --- a/packages/daemon/src/check-dispatch.ts +++ b/packages/daemon/src/check-dispatch.ts @@ -52,9 +52,9 @@ export function selectCheckable( // space's members, so their checkruns index as trusted. No actors → nothing to author with. const author = [...actors.all].sort((a, b) => compareCodePoints(a.did, b.did))[0] - // A shelved goal is set aside: agents run no checks on its artifacts. `activeGoals` is closed, - // archived AND under-an-archived-project in one rule; an archived project's own artifacts go with - // it, because putting a repository away has to stop the machinery that clones it. + // A shelved goal is set aside: agents run no checks on its artifacts. `activeGoals` is ended AND + // under-an-archived-project in one rule; an archived project's own artifacts go with it, because + // putting a repository away has to stop the machinery that clones it. const goals = activeGoals(index) const projects = activeProjects(index) const allArtifacts: Array> = [ diff --git a/packages/daemon/src/dispatch.ts b/packages/daemon/src/dispatch.ts index 66bc7ff..f00f416 100644 --- a/packages/daemon/src/dispatch.ts +++ b/packages/daemon/src/dispatch.ts @@ -514,8 +514,8 @@ export function selectDispatchable( const results: Dispatchable[] = [] // A shelved goal is set aside: agents never pick up its requests. `activeGoals` is the whole rule — - // closure (the author's own `closed` edit), archival (any member's reversible `archiveGoal`), and a - // goal whose PROJECT the project author archived, which shelves everything under it. + // a goal any active member has ended (`GoalView.ended`, whichever way it was ended), and a goal + // whose PROJECT the project author archived, which shelves everything under it. for (const goalView of activeGoals(index)) { for (const request of goalView.openRequests) { const dispatchable = evaluate(goalView, request, 'goal') diff --git a/packages/daemon/src/merge-poll.ts b/packages/daemon/src/merge-poll.ts index b662ace..623dbd3 100644 --- a/packages/daemon/src/merge-poll.ts +++ b/packages/daemon/src/merge-poll.ts @@ -104,8 +104,8 @@ export function selectMergeCandidates( const candidates: MergeCandidate[] = [] const gitUrlByProject = new Map(index.projects.map((view) => [view.target.uri, view.gitUrl])) - // A shelved goal is set aside: agents stop polling/merging its PRs. `activeGoals` covers closure, - // archival, and a goal under a project its author archived. + // A shelved goal is set aside: agents stop polling/merging its PRs. `activeGoals` covers a goal any + // member has ended, and a goal under a project its author archived. for (const goalView of activeGoals(index)) { const gitUrl = gitUrlByProject.get(goalView.target.value.project.uri) for (const artifact of goalView.artifacts) { diff --git a/packages/daemon/test/auto-review.test.mjs b/packages/daemon/test/auto-review.test.mjs index 5a3358a..6eefe95 100644 --- a/packages/daemon/test/auto-review.test.mjs +++ b/packages/daemon/test/auto-review.test.mjs @@ -124,7 +124,7 @@ function makeIndex({ goals: [ { target: { uri: goalRef.uri, cid: goalRef.cid, value: { project: projectRef, closed } }, - archived, + ended: closed || archived, artifacts, requests, openRequests: [], diff --git a/packages/daemon/test/merge-poll.test.mjs b/packages/daemon/test/merge-poll.test.mjs index 4c46723..1292761 100644 --- a/packages/daemon/test/merge-poll.test.mjs +++ b/packages/daemon/test/merge-poll.test.mjs @@ -45,7 +45,7 @@ function makeIndex({ return { members: members.map((did) => ({ did, active: true, kind: 'agent', role: 'agent' })), // Mirror the real ProjectView/GoalView shapes the poller reads: a goal is skipped when it is - // closed, archived, or hanging off an archived project, and its project's `gitUrl` rides along + // ended or hanging off an archived project, and its project's `gitUrl` rides along // to the adapter as a hint (a tangled pull URL cannot name the repo owner a merge is recorded // under — see `PullStateContext`). projects: [{ archived: projectArchived, gitUrl, target: { uri: PROJECT_URI, value: {} } }], @@ -53,7 +53,7 @@ function makeIndex({ { artifacts, merges, - archived, + ended: closed || archived, target: { value: { closed, project: { uri: PROJECT_URI } } }, }, ], diff --git a/packages/lexicons/README.md b/packages/lexicons/README.md index 72480f1..f768fd8 100644 --- a/packages/lexicons/README.md +++ b/packages/lexicons/README.md @@ -1,5 +1,30 @@ # Radial lexicons +## Goal endings + +New clients end and reopen goals with `com.disnetdev.radial.closeGoal`. Its `disposition` is a +free-form string (maximum 32 characters) with the currently documented vocabulary `completed`, +`dropped`, `superseded`, and `parked`. It is intentionally not a lexicon enum: older materializers +must still recognize an ending when a newer writer introduces another disposition. The disposition +is annotation only; `closed` is the state. + +`com.disnetdev.radial.archiveGoal` and the legacy `goal.closed` field remain readable for +compatibility, but new writers must not write them. How a second implementation must read them, since +they are two independent legacy dimensions rather than one: + +- The latest trusted record across `closeGoal` and `archiveGoal` (by `createdAt`, then URI) wins, and + a goal is matched by URI rather than `uri#cid` — a legacy `closed` edit minted a second CID, and an + ending has to survive that. +- A winning `closeGoal` decides the state outright, in either direction: it is the one axis, so it + reopens a legacy-closed goal without any rewrite of the goal record. +- A winning `archiveGoal` can only ADD its park. `archived: false` does **not** cancel a + `goal.closed: true` underneath it: the writer of either record knew nothing about the other, so a + member who archived and then changed their mind has said nothing about a closure. A goal carrying + both reads as `completed` — the ending that actually happened. +- A `closeGoal` with `closed: true` reports `disposition` as written (unknown words included) and the + record's `createdAt` as the ending instant. A legacy closure reports `completed` and no instant; a + legacy park reports `parked` and the archive record's instant. + Radial records use the permanent `com.disnetdev.radial.*` namespace. The JSON files in `lexicons/` are the source of truth. `pnpm codegen` generates typed record definitions and embedded runtime schemas in `packages/core`. diff --git a/packages/lexicons/lexicons/com.disnetdev.radial.closeGoal.json b/packages/lexicons/lexicons/com.disnetdev.radial.closeGoal.json new file mode 100644 index 0000000..343b7dc --- /dev/null +++ b/packages/lexicons/lexicons/com.disnetdev.radial.closeGoal.json @@ -0,0 +1,20 @@ +{ + "lexicon": 1, + "id": "com.disnetdev.radial.closeGoal", + "defs": { + "main": { + "type": "record", + "key": "tid", + "record": { + "type": "object", + "required": ["goal", "closed", "createdAt"], + "properties": { + "goal": {"type": "ref", "ref": "com.atproto.repo.strongRef"}, + "closed": {"type": "boolean"}, + "disposition": {"type": "string", "maxLength": 32}, + "createdAt": {"type": "string", "format": "datetime"} + } + } + } + } +} diff --git a/packages/sidecar/src/cli.ts b/packages/sidecar/src/cli.ts index b15ee16..b5aed00 100644 --- a/packages/sidecar/src/cli.ts +++ b/packages/sidecar/src/cli.ts @@ -35,8 +35,10 @@ export const help = `radial — human CLI for Radial records radial project archive|unarchive --project REF (the project author only) radial project set-auto-review --project REF --type NAME --enabled true|false radial goal create --project REF --title TITLE [--body TEXT | --body-file PATH] - radial goal close|reopen --goal REF (the goal author only) - radial goal archive|unarchive --goal REF (any active member) + radial goal close --goal REF [--disposition completed|dropped|superseded|parked] + (any active member; another word warns) + radial goal reopen --goal REF (any active member) + radial goal archive|unarchive --goal REF (deprecated aliases for close/reopen) radial request create (--goal REF | --project REF) --type TYPE [--assignee HANDLE|DID] [--subject REF] [--based-on REF]... [--brief TEXT] diff --git a/packages/sidecar/src/commands.ts b/packages/sidecar/src/commands.ts index 568000d..4de1deb 100644 --- a/packages/sidecar/src/commands.ts +++ b/packages/sidecar/src/commands.ts @@ -14,10 +14,11 @@ export interface RecordWriter { options?: { rkey?: string }, ): Promise /** - * Rewrite one of the actor's own records in place. Only the sanctioned edits use this — today - * just a goal's `closed` flag (design §7) — and only the record's own author can: a PDS will not - * let one actor put into another's repo. Optional so a writer that never edits (a turn's - * socket-backed writer, say) does not have to implement it; commands that need it say so. + * Rewrite one of the actor's own records in place, which only the record's own author can: a PDS + * will not let one actor put into another's repo. **No command in this file uses it** — the last one + * that did closed a goal by editing it, and a goal's ending is a standalone `closeGoal` overlay now + * (design §7). It stays for `radiald init --update`, which republishes an agent record. Optional so + * a writer that never edits (a turn's socket-backed writer, say) does not have to implement it. */ put?( collection: K, @@ -166,6 +167,50 @@ const builtinTypes = [ */ const RESERVED_TYPE_NAMES = ['review', 'answer'] +/** + * The documented vocabulary for a goal ending's `disposition` (`packages/lexicons/README.md`). + * + * The lexicon deliberately types the field as a free string rather than an enum: an out-of-enum value + * would make the WHOLE record invalid, so a fifth disposition added later would land every new ending + * in `index.ignored` on every un-upgraded materializer — which reads the goal as open and keeps + * dispatching it. The openness is for future writers, though, not for typos, so this command warns on + * a word outside the list and refuses one the lexicon cannot carry at all. + */ +const DISPOSITIONS = ['completed', 'dropped', 'superseded', 'parked'] +/** `com.disnetdev.radial.closeGoal`'s `disposition` ceiling. Over it, every materializer drops the record. */ +const DISPOSITION_MAX = 32 + +/** + * `--disposition`, checked before anything is written. + * + * A PDS knows nothing of this lexicon and accepts whatever it is handed, so an over-long or empty + * value is a record written, acknowledged, reported as a success — and then filed in `index.ignored` + * by every reader, leaving the goal open with agents still picking it up. That failure is invisible + * from the shell, which is why the length is a refusal here rather than a warning. An unrecognised + * *word* is only a cosmetic divergence, so it warns and is written: that is the forward compatibility + * the free-string field exists for. + */ +function endingDisposition(args: string[], deps: CliDependencies): string | undefined { + const disposition = optional(args, '--disposition') + if (disposition === undefined) return undefined + if (disposition.trim() === '') { + throw new Error('--disposition cannot be empty; omit the flag to state no disposition') + } + if (disposition.length > DISPOSITION_MAX) { + throw new Error( + `--disposition must be at most ${DISPOSITION_MAX} characters (got ${disposition.length}) — ` + + 'a longer one is a record every materializer would drop. Put the reason in a message instead.', + ) + } + if (!DISPOSITIONS.includes(disposition)) { + deps.warn?.( + `"${disposition}" is not one of the documented dispositions (${DISPOSITIONS.join(', ')}); ` + + 'writing it anyway — readers will end the goal and show the word as given', + ) + } + return disposition +} + function values(args: string[], flag: string): string[] { const results: string[] = [] for (let index = 0; index < args.length; index += 1) { @@ -476,37 +521,37 @@ export async function runCli(args: string[], deps: CliDependencies): Promise { }) describe('sanctioned goal closure and inline review findings', () => { - it('closes and reopens a goal through put, and writes findings from inline JSON', async () => { + it('ends and reopens a goal through overlays, and writes findings from inline JSON', async () => { const aliceDid = 'did:plc:alice-close' const bobDid = 'did:plc:bob-close' const pdss = new Map([ @@ -434,45 +434,88 @@ describe('sanctioned goal closure and inline review findings', () => { (await ingestor.sync()).goals.find((view) => view.target.uri === goal.primary.uri) assert.equal((await goalView()).target.value.closed, undefined) - // The author closes their own goal: the same record, at the same uri, with only `closed` changed. - const closed = await runCli(['goal', 'close', '--goal', locator(goal)], at(aliceDid, 4)) - assert.equal(closed.primary.uri, goal.primary.uri) - assert.notEqual(closed.primary.cid, goal.primary.cid) - assert.equal((await goalView()).target.value.closed, true) - // Sanctioned, so it is never flagged as a silent rewrite. - const afterClose = await ingestor.sync() - assert.equal(afterClose.edits.some((edit) => edit.uri === goal.primary.uri), false) + const closed = await runCli( + ['goal', 'close', '--goal', locator(goal), '--disposition', 'dropped'], + at(aliceDid, 4), + ) + assert.equal(closed.primary.uri.includes(`/${COLLECTIONS.closeGoal}/`), true) + assert.equal((await goalView()).ended, true) + assert.equal((await goalView()).disposition, 'dropped') - // …and reopens it. A closure is a decision, not a one-way door. - await runCli(['goal', 'reopen', '--goal', locator(closed)], at(aliceDid, 5)) - assert.equal((await goalView()).target.value.closed, false) + await runCli(['goal', 'reopen', '--goal', goal.primary.uri], at(aliceDid, 5)) + assert.equal((await goalView()).ended, false) - // Only the author may edit: Bob's attempt fails before anything is written to his repo. - await assert.rejects( - () => runCli(['goal', 'close', '--goal', goal.primary.uri], at(bobDid, 6)), - /Only a record's author can edit it/, - ) - assert.equal( - [...pdss.get(bobDid).records.values()].some((entry) => entry.value.$type === COLLECTIONS.goal), - false, - ) + // Any active member can end it; the overlay is attributed to that member. + await runCli(['goal', 'close', '--goal', goal.primary.uri, '--disposition', 'parked'], at(bobDid, 6)) + assert.equal((await goalView()).ended, true) + assert.equal((await goalView()).disposition, 'parked') - // A stale swapRecord loses the race rather than clobbering the newer value. - const live = await resolver.resolve(goal.primary.uri, COLLECTIONS.goal) + // A disposition the lexicon cannot carry is refused BEFORE the write. A PDS would accept it + // (it knows nothing of this lexicon) and every materializer would then drop the record, leaving + // the goal open with the shell reporting success — the one failure mode nobody can see. + const before = (await ingestor.sync()).goals.find((view) => view.target.uri === goal.primary.uri) await assert.rejects( () => - sessions.get(aliceDid).put( - COLLECTIONS.goal, - goal.primary.uri, - { ...live.value, closed: true }, - { swapRecord: goal.primary.cid }, + runCli( + ['goal', 'close', '--goal', goal.primary.uri, '--disposition', 'x'.repeat(33)], + at(aliceDid, 20), ), - /InvalidSwap/, + /at most 32 characters/, + ) + await assert.rejects( + () => runCli(['goal', 'close', '--goal', goal.primary.uri, '--disposition', ' '], at(aliceDid, 21)), + /cannot be empty/, + ) + const after = (await ingestor.sync()).goals.find((view) => view.target.uri === goal.primary.uri) + assert.deepEqual(after.disposition, before.disposition) + assert.deepEqual(after.endedAt, before.endedAt) + + // A word outside the documented vocabulary WARNS and is written: the field is a free string on + // purpose, so that a newer writer's disposition still ends the goal for an older reader. + const warnings = [] + const unknown = await runCli( + ['goal', 'close', '--goal', goal.primary.uri, '--disposition', 'hibernating'], + { ...at(aliceDid, 22), warn: (text) => warnings.push(text) }, + ) + assert.equal(unknown.primary.uri.includes(`/${COLLECTIONS.closeGoal}/`), true) + assert.equal(warnings.length, 1) + assert.match(warnings[0], /not one of the documented dispositions/) + assert.equal((await goalView()).ended, true) + assert.equal((await goalView()).disposition, 'hibernating') + + // The deprecated aliases: one ending record each, one warning each, and `unarchive` carries no + // disposition — a reopen annotating the ending it undoes would read as a contradiction. + const aliasWarnings = [] + const parked = await runCli( + ['goal', 'archive', '--goal', goal.primary.uri], + { ...at(aliceDid, 23), warn: (text) => aliasWarnings.push(text) }, + ) + const parkedRecord = await resolver.resolve(parked.primary.uri, COLLECTIONS.closeGoal) + assert.deepEqual(parkedRecord.value.closed, true) + assert.deepEqual(parkedRecord.value.disposition, 'parked') + assert.equal((await goalView()).disposition, 'parked') + + const unparked = await runCli( + ['goal', 'unarchive', '--goal', goal.primary.uri], + { ...at(aliceDid, 24), warn: (text) => aliasWarnings.push(text) }, + ) + const unparkedRecord = await resolver.resolve(unparked.primary.uri, COLLECTIONS.closeGoal) + assert.deepEqual(unparkedRecord.value.closed, false) + assert.deepEqual(unparkedRecord.value.disposition, undefined) + assert.equal((await goalView()).ended, false) + assert.deepEqual(aliasWarnings.length, 2) + assert.match(aliasWarnings[0], /goal archive is deprecated/) + assert.match(aliasWarnings[1], /goal unarchive is deprecated/) + // Nothing writes `archiveGoal` any more, in either spelling. + assert.equal( + [...pdss.values()].some((pds) => + [...pds.records.values()].some((entry) => entry.value.$type === COLLECTIONS.archiveGoal), + ), + false, ) // Findings inline, for a caller with no filesystem — the browser's findings editor. - // A bare uri, not a pinned locator: closing minted a new goal cid, which is exactly why the - // materializer resolves goal refs leniently while everything else pins uri#cid. + // A bare goal URI remains valid while ending records are standalone overlays. const request = await runCli( ['request', 'create', '--goal', goal.primary.uri, '--type', 'plan', '--assignee', bobDid], at(aliceDid, 7), diff --git a/packages/ui/src/app.css b/packages/ui/src/app.css index 53ea792..bc7228d 100644 --- a/packages/ui/src/app.css +++ b/packages/ui/src/app.css @@ -542,6 +542,10 @@ a.btn { text-decoration: none; color: var(--ink); display: inline-block; } .btn-bad { color: var(--bad); border-color: color-mix(in oklch, var(--bad) 28%, var(--line)); } .btn-bad:hover { background: var(--bad-wash); border-color: var(--bad); } .acts .gap { width: 1px; height: 20px; background: var(--line); margin: 0 3px; } +/* A picker standing in a row of buttons — the goal ending's disposition. The same control as every + other `select.sl`, sized to its content instead of to the row, exactly like `.fedit-where`'s. */ +.acts label { font-size: 11.5px; font-weight: 600; color: var(--ink-2); } +.acts select.sl { width: auto; } /* ─── the ask bar ────────────────────────────────────────────────────────── */ /* The registry's request buttons, standing on the goal they are asked against (`RequestBar.svelte`). diff --git a/packages/ui/src/lib/admin.test.ts b/packages/ui/src/lib/admin.test.ts index fa9770a..2b8b97f 100644 --- a/packages/ui/src/lib/admin.test.ts +++ b/packages/ui/src/lib/admin.test.ts @@ -3,7 +3,6 @@ import { FIXTURE_DIDS } from '@radial/core/fixture' import { describe, expect, it } from 'vitest' import { addMemberArgs, - archiveArgs, autoReviewArgs, autoReviewNeedsAgent, autoReviewRows, @@ -13,7 +12,6 @@ import { httpsGitUrl, memberRows, nameTaken, - ownsGoal, ownsProject, participates, projectArchiveArgs, @@ -35,7 +33,7 @@ import { projectByName } from './space.js' // // Every assertion here is really an assertion about `materialize()`: the fold trusts a membership // event, a registry record and an auto-review setting only from an admin, and it trusts a project, a -// goal and an archive annotation from any active member. A button offered outside those conditions +// goal and a goal ending from any active member. A button offered outside those conditions // would be a write that reaches the PDS and changes nothing, which is the same mistake retract was // stopped from making in 6.4. // @@ -452,28 +450,27 @@ describe('auto-review', () => { }) }) -describe('a goal’s two endings', () => { - it('lets only the goal’s own author close it', () => { - const mine = goalBy(tim) - expect(ownsGoal(mine, tim)).toBe(true) - expect(ownsGoal(mine, ana)).toBe(false) - // Being an admin makes no difference: the record lives in its author's repo, and a PDS will not - // let one actor put into another's. - expect(ownsGoal(goalBy(ana), tim)).toBe(false) - expect(ownsGoal(mine, '')).toBe(false) +describe('a goal’s one ending', () => { + it('asks for nothing more than active membership, whoever the author is', () => { + // The rule the author-only one replaced. A goal whose author has gone can be ended by anybody + // still here, and being an admin buys nothing extra: the ending is a standalone signed record. + expect(participates(space.directory, tim)).toBe(true) + expect(participates(space.directory, ana)).toBe(true) + expect(participates(space.directory, '')).toBe(false) }) - it('names the goal by URI rather than pinning it, because the command re-reads to swap', () => { + it('names the goal by URI rather than pinning it, because an ending matches by URI', () => { const goal = goalBy(tim) expect(closeArgs(goal, true)).toEqual(['goal', 'close', '--goal', goal.target.uri]) expect(closeArgs(goal, false)).toEqual(['goal', 'reopen', '--goal', goal.target.uri]) expect(closeArgs(goal, true).join(' ')).not.toContain('#') }) - it('archives with a standalone record, in both directions', () => { + it('includes the selected ending disposition', () => { const goal = goalBy(ana) - expect(archiveArgs(goal, true)).toEqual(['goal', 'archive', '--goal', goal.target.uri]) - expect(archiveArgs(goal, false)).toEqual(['goal', 'unarchive', '--goal', goal.target.uri]) + expect(closeArgs(goal, true, 'parked')).toEqual([ + 'goal', 'close', '--goal', goal.target.uri, '--disposition', 'parked', + ]) }) }) diff --git a/packages/ui/src/lib/admin.ts b/packages/ui/src/lib/admin.ts index ec59d49..732f644 100644 --- a/packages/ui/src/lib/admin.ts +++ b/packages/ui/src/lib/admin.ts @@ -13,15 +13,14 @@ // - Membership, the registry and auto-review are ADMIN-authored-only — a membership event from a // non-admin lands in `index.ignored`, and an `artifactType` or `setAutoReview` from one never // reaches the index at all. (`governs`) -// - Creating a project, writing a goal, and archiving a GOAL are ANY-ACTIVE-MEMBER. Goal archiving -// in particular is deliberately not admin-only: it is a reversible annotation about what agents -// should ignore, not a decision about the work. (`participates`) -// - A goal's `closed` flag is not a role at all. It is the goal author's own edit of their own -// record (design §7), so the PDS refuses it rather than the fold, and nobody else's admin -// status makes any difference. (`ownsGoal`) +// - Creating a project, writing a goal, and ENDING a goal are ANY-ACTIVE-MEMBER. The ending in +// particular is deliberately not narrower: it is a standalone, reversible, signed record saying +// what happened to the work, and the rule it replaced — the goal author's own edit of their own +// record — protected nothing, because any member could already achieve the identical effect by +// archiving. It also stranded a goal whose author had gone. (`participates`) // - Editing a PROJECT — its name, remote, default branch — and archiving one belong to the -// project's author, and to nobody else, admin included. These are standalone records like goal -// archiving, so the PDS accepts anybody's and the fold is what drops them; the rule is narrow +// project's author, and to nobody else, admin included. These are standalone records like a +// goal's ending, so the PDS accepts anybody's and the fold is what drops them; the rule is narrow // because what a project is decides which repository every future turn clones, and archiving one // shelves every goal beneath it. (`ownsProject`) // @@ -53,28 +52,20 @@ export function governs(directory: Directory, did: string): boolean { return actor.active && actor.role === 'admin' } -/** An active member of any role: what a project, a goal, or an archive annotation asks for. */ +/** An active member of any role: what a project, a goal, or a goal's ending asks for. */ export function participates(directory: Directory, did: string): boolean { return Boolean(did) && directory.get(did).active } -/** - * The goal's own author, and nobody else. Closure is the one sanctioned edit in the protocol, and - * an edit lands in the author's repo or nowhere — `RepoWriter.put` refuses a URI that is not the - * signer's before a request is even sent. - */ -export const ownsGoal = (goal: GoalView, did: string): boolean => - Boolean(did) && goal.target.did === did - /** * The project's own author, and nobody else — including an admin. * - * Unlike `ownsGoal` this is not the PDS refusing a foreign repo: a project's name, remote, branch and - * archived state are STANDALONE records (`editProject`, `archiveProject`), so anybody's PDS will - * accept one and the fold is what drops it. So this is the same kind of answer the rest of this file - * gives — a trust rule read off `materialize()` — and the reason for the rule is that what a project - * IS decides which repository every future turn clones. That is not an annotation about where - * attention should go (which is why archiving a *goal* is any member's), it is the thing itself. + * A project's name, remote, branch and archived state are STANDALONE records (`editProject`, + * `archiveProject`), so anybody's PDS will accept one and the fold is what drops it. So this is the + * same kind of answer the rest of this file gives — a trust rule read off `materialize()` — and the + * reason for the rule is that what a project IS decides which repository every future turn clones. + * That is not an annotation about where attention should go (which is why ending a *goal* is any + * member's), it is the thing itself. */ export const ownsProject = (project: ProjectView, did: string): boolean => Boolean(did) && project.target.did === did @@ -514,27 +505,22 @@ export const autoReviewArgs = (project: ProjectView, type: string, enabled: bool export const autoReviewNeedsAgent = (directory: Directory): boolean => !directory.agents().some((agent) => agent.active && agent.artifactTypes.includes('review')) -// ── a goal's two endings ──────────────────────────────────────────────────────────────────────── +// ── a goal's ending ───────────────────────────────────────────────────────────────────────────── /** - * `radial goal close|reopen`. The goal is named by URI and deliberately not pinned: the command - * re-reads the record to derive the new value and passes that CID as `swapRecord`, so a locator - * carrying the CID this tab happens to hold would turn a stale view into a failed write rather than - * into the harmless one it is. + * `radial goal close|reopen`, and the disposition when it is a close. + * + * The goal is named by URI and deliberately not pinned by uri#cid, which is the one thing to get + * right here: the fold matches an ending to its goal by URI alone, because a legacy `closed` edit + * minted a second CID for goals that already carry one. Pinning would make an ending resolve onto + * nothing on exactly those goals. */ -export const closeArgs = (goal: GoalView, close: boolean): string[] => [ +export const closeArgs = (goal: GoalView, close: boolean, disposition?: string): string[] => [ 'goal', close ? 'close' : 'reopen', '--goal', goal.target.uri, -] - -/** `radial goal archive|unarchive` — a standalone record, so anyone active may write one. */ -export const archiveArgs = (goal: GoalView, archive: boolean): string[] => [ - 'goal', - archive ? 'archive' : 'unarchive', - '--goal', - goal.target.uri, + ...(close && disposition ? ['--disposition', disposition] : []), ] // ── creating a space ──────────────────────────────────────────────────────────────────────────── diff --git a/packages/ui/src/lib/client-metadata.test.ts b/packages/ui/src/lib/client-metadata.test.ts index bd709bf..976756d 100644 --- a/packages/ui/src/lib/client-metadata.test.ts +++ b/packages/ui/src/lib/client-metadata.test.ts @@ -82,21 +82,23 @@ describe('the loopback client id', () => { * `@radial/sidecar` that writes a collection nobody authorized would otherwise fail on a human's PDS, * at the end of a compose card, with an error no part of this app could explain. * - * `putRecord` is counted as `create` *and* `update`: the PDS asserts both on the collection before it - * looks at whether the record exists, so an update-only grant would refuse the one edit Radial makes. + * A `deps.writer.put(…)` is matched too, and is an assertion failure rather than an `update` + * permission: no command edits a record any more — the last one, a goal's `closed` flag, became the + * standalone `closeGoal` overlay — so a `put` reappearing here is a command that would be refused by + * every session's scope, and the fix is the command rather than the grant. */ function collectionsTheCliWrites(): Map> { const calls = [...commandSource.matchAll(/deps\.writer\.(create|put)\(\s*COLLECTIONS\.(\w+)/g)] // A refactor that respells the write — a helper, a different receiver — would find nothing and // leave this suite passing vacuously with the scope unchecked, so the count is asserted too. expect(calls.length).toBeGreaterThan(10) + expect(calls.filter(([, method]) => method === 'put')).toEqual([]) const writes = new Map>() - for (const [, method, name] of calls) { + for (const [, , name] of calls) { const collection = COLLECTIONS[name as keyof typeof COLLECTIONS] as string | undefined if (!collection) throw new Error(`commands.ts writes COLLECTIONS.${name}, which is not a collection`) const actions = writes.get(collection) ?? new Set() actions.add('create') - if (method === 'put') actions.add('update') writes.set(collection, actions) } return writes @@ -139,8 +141,8 @@ describe('the scope a Radial session asks for', () => { expect(permissions.allowsRepo({ collection: COLLECTIONS.claim, action: 'create' })).toBe(false) expect(permissions.allowsRepo({ collection: COLLECTIONS.merge, action: 'create' })).toBe(false) expect(permissions.allowsRepo({ collection: COLLECTIONS.agent, action: 'create' })).toBe(false) - // The one edit that is sanctioned is the only one there is: a goal's `closed` flag (design §7). - expect(permissions.allowsRepo({ collection: COLLECTIONS.goal, action: 'update' })).toBe(true) + // Goal endings are standalone overlays; primary goal records are create-only. + expect(permissions.allowsRepo({ collection: COLLECTIONS.goal, action: 'update' })).toBe(false) expect(permissions.allowsRepo({ collection: COLLECTIONS.artifact, action: 'update' })).toBe(false) }) }) diff --git a/packages/ui/src/lib/client-metadata.ts b/packages/ui/src/lib/client-metadata.ts index 220117f..e116de6 100644 --- a/packages/ui/src/lib/client-metadata.ts +++ b/packages/ui/src/lib/client-metadata.ts @@ -9,8 +9,11 @@ // `vite.config.ts` writes the deployed document into the bundle when `PUBLIC_RADIAL_ORIGIN` is set; // `auth.svelte.ts` builds the loopback equivalent at runtime. -/** What a repo permission may authorize. `delete` is deliberately not among them — see `RADIAL_WRITES`. */ -export type RepoWriteAction = 'create' | 'update' +/** + * What a repo permission may authorize. Only `create`: `delete` and `update` are both deliberately + * absent — see `RADIAL_WRITES`. + */ +export type RepoWriteAction = 'create' /** * Every collection a Radial session writes, and what it does to each. This is the whole of what the @@ -18,26 +21,35 @@ export type RepoWriteAction = 'create' | 'update' * — `client-metadata.test.ts` holds it against that source, so a new command that writes a new * collection fails a test here rather than failing at somebody's PDS. * - * Two things are absent on purpose: + * Three things are absent on purpose: * - * - **`delete`.** Radial never removes a record. Retraction, archival and removal are each their - * own record that the fold reads as an annotation, so a session that could delete could rewrite - * history the fold is built on. `update` appears exactly once, on `goal`, because exactly one - * edit is sanctioned (a goal's `closed` flag, design §7) — and `com.atproto.repo.putRecord` - * checks `create` *and* `update` on the collection, so both are listed. + * - **`delete`.** Radial never removes a record. Retraction, a goal's ending and a member's removal + * are each their own record that the fold reads as an annotation, so a session that could delete + * could rewrite history the fold is built on. + * - **`update`.** No collection needs it any more. It used to appear exactly once, on `goal`, for + * the one sanctioned owner edit — a goal's `closed` flag — and `com.disnetdev.radial.closeGoal` + * replaced that edit with a standalone overlay, so every write this app makes is a `create`. + * Legacy `closed` edits are still READ; nothing here writes one, which is why the permission is + * gone rather than kept for compatibility. * - **The collections a daemon writes.** `agent`, `checkrun`, `claim` and `merge` come from a * daemon holding its own credential, never from a tab. A browser session that could mint one * under a human's DID could forge the fold's account of what an agent did. + * + * **Dropping a permission narrows the scope, and a token already issued carries the old one.** A + * session established before `closeGoal` existed was granted `archiveGoal` and `goal?action=update` + * and not the new collection, so its first attempt to end a goal is refused by the PDS. `write.ts` + * recognises that refusal and says what fixes it (sign out, sign in again); `CHANGELOG.md` says so + * too, for the operator who reads release notes before their users do. */ export const RADIAL_WRITES: Readonly> = { 'com.disnetdev.radial.addMember': ['create'], - 'com.disnetdev.radial.archiveGoal': ['create'], + 'com.disnetdev.radial.closeGoal': ['create'], 'com.disnetdev.radial.archiveProject': ['create'], 'com.disnetdev.radial.artifact': ['create'], 'com.disnetdev.radial.artifactRequest': ['create'], 'com.disnetdev.radial.artifactType': ['create'], 'com.disnetdev.radial.editProject': ['create'], - 'com.disnetdev.radial.goal': ['create', 'update'], + 'com.disnetdev.radial.goal': ['create'], 'com.disnetdev.radial.join': ['create'], 'com.disnetdev.radial.message': ['create'], 'com.disnetdev.radial.project': ['create'], diff --git a/packages/ui/src/lib/components/GoalActions.svelte b/packages/ui/src/lib/components/GoalActions.svelte index a1ee75d..04040ea 100644 --- a/packages/ui/src/lib/components/GoalActions.svelte +++ b/packages/ui/src/lib/components/GoalActions.svelte @@ -1,24 +1,29 @@ -{#if live && (mine || member)} +{#if live && member}
- {#if mine} + {#if goal.ended} - {/if} - {#if member} - + + {:else} + + Choose what happened to the work. {/if} - - {#if mine} - Archiving only tells agents to skip this goal. - {:else} - Archiving only tells agents to skip this goal. Closing it is its author's to do. - {/if} -
{#if error} @@ -89,6 +92,6 @@ {:else if !space.fixture && account.status !== 'signed-in'}

- to close or archive this goal. + to end or reopen this goal.

{/if} diff --git a/packages/ui/src/lib/components/GoalRow.svelte b/packages/ui/src/lib/components/GoalRow.svelte index bd4eb68..880064d 100644 --- a/packages/ui/src/lib/components/GoalRow.svelte +++ b/packages/ui/src/lib/components/GoalRow.svelte @@ -2,10 +2,11 @@ import type { GoalView } from '@radial/core' import { tally } from '@radial/core' import { shortDate } from '$lib/format.js' - import { goalHref, isClosed, unitsOf, type Space } from '$lib/space.js' + import { goalHref, isEnded, unitsOf, type Space } from '$lib/space.js' import { agentDids, askWithAgent, + endingBadge, isAwaiting, isMoving, needsVerdict, @@ -42,6 +43,7 @@ ) const open = $derived(units.filter((unit) => unit.state === 'open').length) const toJudge = $derived(units.filter(needsVerdict).length) + const ending = $derived(endingBadge(goal)) // Three discs is a crowd; past that the row is about the goal, not its cast. const actors = $derived( [...new Set(units.flatMap(unitParticipants))].slice(0, 3).map((did) => space.directory.get(did)), @@ -49,7 +51,7 @@
  • - + {goal.target.value.title} @@ -64,6 +66,9 @@ {#if toJudge > 0} {/if} + {#if ending} + + {/if} {#each actors as actor (actor.did)} diff --git a/packages/ui/src/lib/components/Rail.svelte b/packages/ui/src/lib/components/Rail.svelte index 5956338..945d9a4 100644 --- a/packages/ui/src/lib/components/Rail.svelte +++ b/packages/ui/src/lib/components/Rail.svelte @@ -8,7 +8,7 @@ import { goalHref, goalsOf, - isClosed, + isEnded, liveProjects, projectHref, SPACE_HREF, @@ -51,7 +51,7 @@ moving: all.filter(({ unit }) => isMoving(unit)).length + askRows.filter((ask) => askWithAgent(ask, agentSet)).length, - logbook: space.index.goals.filter(isClosed).length, + logbook: space.index.goals.filter(isEnded).length, }) const smart = $derived([ @@ -137,7 +137,7 @@ > Goals - {goalsOf(space.index, project).filter((goal) => !isClosed(goal)).length} + {goalsOf(space.index, project).filter((goal) => !isEnded(goal)).length}
  • @@ -155,7 +155,7 @@ {unitsOf(space.index, project).length}
    • - {#each goalsOf(space.index, project).filter((goal) => !isClosed(goal)) as goal (goal.target.uri)} + {#each goalsOf(space.index, project).filter((goal) => !isEnded(goal)) as goal (goal.target.uri)} {@const units = unitsOf(space.index, goal)} {@const counted = tally(units)}
  • diff --git a/packages/ui/src/lib/components/RequestBar.svelte b/packages/ui/src/lib/components/RequestBar.svelte index 019e4df..891df47 100644 --- a/packages/ui/src/lib/components/RequestBar.svelte +++ b/packages/ui/src/lib/components/RequestBar.svelte @@ -5,7 +5,7 @@ requestDialogId, } from '$lib/compose.svelte.js' import { requestableTypes } from '$lib/requests.js' - import type { Space } from '$lib/space.js' + import { goalByUri, type Space } from '$lib/space.js' import { typeLabel } from '$lib/units.js' import Glyph from './Glyph.svelte' @@ -32,6 +32,7 @@ const { space, target, scope }: Props = $props() const offered = $derived(requestableTypes(space.index, scope)) + const ended = $derived(scope === 'goal' && goalByUri(space.index, target)?.ended === true) let buttons = $state>({}) @@ -60,5 +61,8 @@ {typeLabel(name)} {/each} + {#if ended} + Agents skip an ended goal — reopen it to have this worked on. + {/if} {/if} diff --git a/packages/ui/src/lib/session.test.ts b/packages/ui/src/lib/session.test.ts index 5957d69..fac7478 100644 --- a/packages/ui/src/lib/session.test.ts +++ b/packages/ui/src/lib/session.test.ts @@ -7,7 +7,7 @@ import { memoryDatabase, type SpaceDatabase } from './idb.js' import { Identities } from './identity.js' import { openSpace, session, settle, stop, sync } from './session.svelte.js' import { openSpaceStores } from './store.js' -import { goalsOf, isClosed, projectByName, unitsOf } from './space.js' +import { goalsOf, isEnded, projectByName, unitsOf } from './space.js' // The live read path, end to end, against a PDS that only exists in this file: DID documents, // handle resolution, `getLatestCommit`, `listRecords`. Everything between the stub `fetch` and the @@ -79,7 +79,7 @@ describe('opening a live space', () => { const project = projectByName(space.index, 'radial-ng') if (!project) throw new Error('no project') expect(unitsOf(space.index, project)).toHaveLength(6) - expect(goalsOf(space.index, project).filter(isClosed)).toHaveLength(2) + expect(goalsOf(space.index, project).filter(isEnded)).toHaveLength(5) // Every `getLatestCommit` resolves a PDS first. Asking the directory once per member rather // than once per request is the difference between a poll tick costing one round trip and three. diff --git a/packages/ui/src/lib/space.test.ts b/packages/ui/src/lib/space.test.ts index 8f77f13..1703a1b 100644 --- a/packages/ui/src/lib/space.test.ts +++ b/packages/ui/src/lib/space.test.ts @@ -1,12 +1,14 @@ +import { fixtureSpace } from '@radial/core/fixture' import { describe, expect, it } from 'vitest' import { didOf, rkeyOf } from './format.js' import { buildFixtureSpace } from './fixture.js' -import { goalByRoute, goalHref, goalsOf, isClosed, projectByName, unitsOf } from './space.js' +import { goalByRoute, goalHref, goalsOf, isEnded, projectByName, unitsOf } from './space.js' import { capturedFrom, claimExpiry, distilledInto, driftOf, + endingBadge, findVersion, isAwaiting, needsVerdict, @@ -20,6 +22,8 @@ import { // `timeline()`. A view can only draw what survives that, so what is asserted here is what the // screens are allowed to claim. const space = buildFixtureSpace() +/** The fixture`s own record set, for the URIs of the goals whose endings are asserted below. */ +const fixture = fixtureSpace() describe('the fixture space folds cleanly', () => { it('ignores nothing and annotates no edits', () => { @@ -59,13 +63,13 @@ describe('routes', () => { }) describe('the goal list', () => { - it('lists a project’s goals newest first, with the closed ones still there for the logbook', () => { + it('lists a project’s goals newest first, with the ended ones still there for the logbook', () => { const project = projectByName(space.index, 'radial-ng') if (!project) throw new Error('fixture project missing') const goals = goalsOf(space.index, project) const dates = goals.map((goal) => goal.target.value.createdAt) expect([...dates].sort().reverse()).toEqual(dates) - expect(goals.filter(isClosed)).toHaveLength(2) + expect(goals.filter(isEnded)).toHaveLength(5) }) }) @@ -261,3 +265,44 @@ describe('the system view', () => { expect(architecture?.current?.version).toBe(3) }) }) + +describe('how a goal’s ending reads', () => { + const badgeFor = (uri: string) => { + const found = space.index.goals.find((goal) => goal.target.uri === uri) + if (!found) throw new Error('fixture goal missing') + return endingBadge(found) + } + + it('gives the approving colour to `completed` alone', () => { + // DESIGN.md keeps the coloured washes for verdicts, checks and "for you", and gives `flat` to + // everything factual. "We decided not to do it" is factual, and painting it sage would read as a + // success on the page of a goal that was abandoned. + expect(badgeFor(fixture.goals.autoReview)).toEqual({ + kind: 'ok', + glyph: '✓', + text: 'completed', + title: expect.any(String), + }) + for (const [uri, word] of [ + [fixture.goals.budgets, 'dropped'], + [fixture.goals.diffViewer, 'superseded'], + [fixture.goals.permissioned, 'parked'], + ] as const) { + expect(badgeFor(uri)).toEqual({ kind: 'flat', glyph: '⌁', text: word, title: expect.any(String) }) + } + }) + + it('says a goal ended even when it says nothing about how, and nothing when it has not', () => { + const ended = space.index.goals.find((goal) => goal.ended) + if (!ended) throw new Error('fixture goal missing') + // A disposition this build has never heard of is rendered as the word it carries: the fold ends + // the goal either way, so the badge has to say so either way. + expect(endingBadge({ ...ended, disposition: 'hibernating' })?.text).toBe('hibernating') + // An ending that states no disposition at all: the field is optional, so the badge names the fact + // it is sure of. (`exactOptionalPropertyTypes` is on, so the field is dropped rather than undefined + // — which is exactly what the fold emits.) + const { disposition: _unstated, ...unstated } = ended + expect(endingBadge(unstated)?.text).toBe('ended') + expect(endingBadge({ ...ended, ended: false })).toBeUndefined() + }) +}) diff --git a/packages/ui/src/lib/space.ts b/packages/ui/src/lib/space.ts index 010b8a8..90e3f09 100644 --- a/packages/ui/src/lib/space.ts +++ b/packages/ui/src/lib/space.ts @@ -69,17 +69,22 @@ export const goalByUri = (index: MaterializedIndex, uri: string): GoalView | und export const goalByRoute = (index: MaterializedIndex, did: string, rkey: string): GoalView | undefined => index.goals.find((goal) => goal.target.did === did && goal.target.rkey === rkey) -/** A project's goals, newest first. An archived goal is one agents ignore, so people do too. */ +/** + * A project's goals, newest first — ended ones included, because the project page draws them in its + * own Logbook section and the rail counts them. Callers that want only live work filter on `isEnded`; + * a cross-goal list of *units* has `liveUnits()` (`units.ts`), which honours the same one rule. + */ export function goalsOf(index: MaterializedIndex, project: ProjectView): GoalView[] { return index.goals - .filter((goal) => goal.target.value.project.uri === project.target.uri && !goal.archived) + .filter((goal) => goal.target.value.project.uri === project.target.uri) .sort((left, right) => right.target.value.createdAt.localeCompare(left.target.value.createdAt) || left.target.uri.localeCompare(right.target.uri), ) } -export const isClosed = (goal: GoalView): boolean => goal.target.value.closed === true +/** A goal has ended, however it ended. One name for one concept: there is no second predicate. */ +export const isEnded = (goal: GoalView): boolean => goal.ended /** Units of a target, memoised per space load: `timeline()` is pure, and every view asks for it. */ const units = new WeakMap() diff --git a/packages/ui/src/lib/units.ts b/packages/ui/src/lib/units.ts index d96ea11..80ad3ee 100644 --- a/packages/ui/src/lib/units.ts +++ b/packages/ui/src/lib/units.ts @@ -21,7 +21,7 @@ import type { UnitVersion, UnitView, } from '@radial/core' -import { artifactTypes, claimDeadline, staleness, timeline } from '@radial/core' +import { activeGoals, activeProjects, artifactTypes, claimDeadline, staleness, timeline } from '@radial/core' import type { Directory } from './directory.js' import { relativeTime, summarize } from './format.js' @@ -163,15 +163,19 @@ export function unitsWithContext(index: MaterializedIndex): UnitContext[] { /** * The units a smart list folds: everything except what is under something somebody has shelved. * - * Archiving says agents should skip a target, and `goalsOf()` already takes it at its word — an - * archived goal is not on its project's page. A cross-goal list is the same claim seen from further - * away, so it has to honour the same rule: a review owed on an artifact under a shelved goal is not - * work stopped on anybody, and a list that keeps showing it is a list nobody can empty. + * An ending says agents should skip a goal, and a cross-goal list is the same claim seen from further + * away, so it honours the same rule: a review owed on an artifact under a goal somebody has ended is + * not work stopped on anybody, and a list that keeps showing it is a list nobody can empty. * - * Archiving a PROJECT shelves both kinds at once — its own system units, and every goal under it, the - * same reach `activeGoals()` in core gives the daemon's dispatcher. Closure is deliberately NOT part - * of this: a closed goal can still owe somebody a review, and the daemon's rule sets it aside where - * a reader's does not. + * Archiving a PROJECT shelves both kinds at once — its own system units, and every goal under it. + * + * **This is one rule, not two.** It used to be two: an archived goal's units left the lists and a + * closed goal's stayed, on the grounds that a closed goal can still owe somebody a review. Once a + * goal has one ending that reading had to go — the alternative is a reader and the daemon disagreeing + * about which goals are live, which is the confusion the two controls caused one layer down. So + * `liveTargets` delegates to `activeGoals()` in core and there is one definition of shelved. What an + * ended goal still owes is not lost: the Logbook is where ended goals live, `GoalRow` carries its "N + * to review" badge there, and `unitsWithContext()` above still resolves every reference into it. */ export function liveUnits(index: MaterializedIndex): UnitContext[] { const live = new Set(liveTargets(index)) @@ -181,14 +185,7 @@ export function liveUnits(index: MaterializedIndex): UnitContext[] { /** The targets `liveUnits` folds, on their own — what a cross-goal list of anything that is not a * unit (an answer request, say) has to be built from if it is to honour the same shelving rule. */ export function liveTargets(index: MaterializedIndex): Array { - const shelvedProjects = new Set( - index.projects.filter((project) => project.archived).map((project) => project.target.uri), - ) - return [...index.goals, ...index.projects].filter((target) => - isGoalView(target) - ? !target.archived && !shelvedProjects.has(target.target.value.project.uri) - : !target.archived, - ) + return [...activeGoals(index), ...activeProjects(index)] } /** Landed, and no review pins the version being read. The queue's whole reason to exist. */ @@ -341,6 +338,24 @@ const verdictBadge = (verdict: 'approve' | 'request_changes'): RowBadge => ? { kind: 'ok', glyph: '✓', text: 'approved' } : { kind: 'bad', glyph: '✕', text: 'changes' } +/** + * How a goal ended, as one badge — the same one on the goal's own page, in the Logbook and on a + * project's list, so the three cannot say different things about the same record. + * + * `completed` is the only ending that gets the sage ✓: DESIGN.md reserves the coloured washes for + * verdicts, checks and "for you", and gives `flat` to everything factual, which is what "we decided + * not to do it" is. Painting `dropped` green would read as a success on the page of a goal that was + * abandoned. A disposition this build has never heard of is rendered as the word it carries, flat — + * the fold ends the goal either way, so the badge says so either way. + */ +export function endingBadge(goal: GoalView): RowBadge | undefined { + if (!goal.ended) return undefined + const title = 'agents skip an ended goal; nothing was deleted, and it can be reopened' + return goal.disposition === 'completed' + ? { kind: 'ok', glyph: '✓', text: 'completed', title } + : { kind: 'flat', glyph: '⌁', text: goal.disposition ?? 'ended', title } +} + /** The tail of a unit row: state first, then the review, then what the machines observed. */ export function unitBadges(unit: UnitView, options: { now: string } = { now: '' }): RowBadge[] { const badges: RowBadge[] = [] diff --git a/packages/ui/src/lib/verdicts.test.ts b/packages/ui/src/lib/verdicts.test.ts index 899e5be..29f3b1e 100644 --- a/packages/ui/src/lib/verdicts.test.ts +++ b/packages/ui/src/lib/verdicts.test.ts @@ -250,11 +250,12 @@ describe('the queue', () => { }) }) -describe('archiving a goal empties what it was holding in the queue', () => { - // Both flavours of "For me" are drawn from the ignored-records goal, so archiving that one goal is - // the whole queue going quiet. This is the shape of the bug it fixes: every goal in a space - // archived, its project page empty because `goalsOf()` honours the record, and "For me" still - // holding rows nothing could ever clear. +describe('ending a goal empties what it was holding in the queue', () => { + // Both flavours of "For me" are drawn from the ignored-records goal, so ending that one goal is the + // whole queue going quiet. This is the shape of the bug it fixes: every goal in a space set aside, + // its project page empty because `goalsOf()` honours the record, and "For me" still holding rows + // nothing could ever clear. The ending here is a legacy `archiveGoal`, which is also the compat + // reading under test: an old client's record still shelves the goal for a reader. const shelvedUri = fixtureSpace().goals.ignored const archivedIndex = (): MaterializedIndex => { @@ -282,7 +283,7 @@ describe('archiving a goal empties what it was holding in the queue', () => { it('drops both a review asked of somebody and one nobody asked for', () => { const index = archivedIndex() - expect(index.goals.find((goal) => goal.target.uri === shelvedUri)?.archived).toBe(true) + expect(index.goals.find((goal) => goal.target.uri === shelvedUri)?.ended).toBe(true) expect(reviewQueue(index, FIXTURE_DIDS.tim)).toEqual([]) expect(reviewQueue(index)).toEqual([]) expect(unjudged(index)).toEqual([]) @@ -292,7 +293,16 @@ describe('archiving a goal empties what it was holding in the queue', () => { const index = archivedIndex() const shelved = unitsWithContext(index).filter(({ target }) => target.target.uri === shelvedUri) expect(shelved.length).toBeGreaterThan(0) - expect(liveUnits(index)).toHaveLength(unitsWithContext(index).length - shelved.length) + expect(liveUnits(index).some(({ target }) => target.target.uri === shelvedUri)).toBe(false) + // The exact count, so a unit dropped by some other rule is a failure here rather than a silent + // narrowing: what leaves the smart lists is everything under an ended goal, no more and no less. + // (`liveTargets` delegates to `activeGoals()`, so an ended goal's units go — including a + // `completed` one's, which is the reader-side change 6.8 made deliberately; see `units.ts`.) + const ended = unitsWithContext(index).filter( + ({ target }) => 'ended' in target && target.ended, + ) + expect(ended.length).toBeGreaterThan(shelved.length) + expect(liveUnits(index)).toHaveLength(unitsWithContext(index).length - ended.length) const version = shelved[0]?.unit.versions[0] if (!version) throw new Error('fixture version missing') expect(findVersion(index, version.artifact)?.unit.key).toBe(shelved[0]?.unit.key) diff --git a/packages/ui/src/lib/write.test.ts b/packages/ui/src/lib/write.test.ts index 5977fae..f6805fa 100644 --- a/packages/ui/src/lib/write.test.ts +++ b/packages/ui/src/lib/write.test.ts @@ -1,10 +1,9 @@ -import { FetchRepoTransport, parseAtUri } from '@radial/atproto' +import { FetchRepoTransport, XrpcError, parseAtUri } from '@radial/atproto' import { COLLECTIONS, buildTurnBundle, tally, type StoredRecord } from '@radial/core' import { FIXTURE_DIDS, fixtureSpace } from '@radial/core/fixture' import { afterEach, describe, expect, it } from 'vitest' import { addMemberArgs, - archiveArgs, autoReviewArgs, closeArgs, memberRows, @@ -32,7 +31,7 @@ import { } from './requests.js' import { askAnswerArgs, messageArgs, pendingAnswers, pendingFor } from './replies.js' import { openSpace, session, stop } from './session.svelte.js' -import { goalsOf, isClosed, liveProjects, projectByName, unitsOf } from './space.js' +import { goalsOf, isEnded, liveProjects, projectByName, unitsOf } from './space.js' import { openSpaceStores } from './store.js' import { capturedFrom, distilledInto, liveUnits } from './units.js' import { @@ -43,7 +42,7 @@ import { verdictArgs, type FindingDraft, } from './verdicts.js' -import { createSpace, write } from './write.js' +import { createSpace, write, writeFailure } from './write.js' // The write path, end to end, against the same fake PDS the read tests use. Everything between the // stub `fetch` and the screen is the code a browser runs: `RepoWriter` signing with a session it @@ -899,33 +898,26 @@ describe('editing and archiving a project, from the browser', () => { }) }) -describe('a goal’s two endings, from the browser', () => { +describe('a goal’s ending, from the browser', () => { const mine = () => { const found = space().index.goals.find( - (goal) => goal.target.did === FIXTURE_DIDS.tim && !isClosed(goal), + (goal) => goal.target.did === FIXTURE_DIDS.tim && !isEnded(goal), ) if (!found) throw new Error('fixture goal missing') return found } - it('closes a goal as its author, by editing the one field the protocol lets anyone edit', async () => { + it('ends and reopens a goal with standalone records', async () => { const harness = await open() signInAs(FIXTURE_DIDS.tim, harness.network) const uri = mine().target.uri - const before = mine().target.cid - - await write(closeArgs(mine(), true)) + await write(closeArgs(mine(), true, 'completed')) const closed = space().index.goals.find((goal) => goal.target.uri === uri) - expect(closed && isClosed(closed)).toBe(true) - // A `put`, not a `create`, and pinned to the CID that was read: a concurrent edit loses the race - // instead of being clobbered by a value derived from a stale view. - const put = harness.network.calls.find((call) => call.method === 'putRecord') - expect(put?.did).toBe(FIXTURE_DIDS.tim) - expect(closed?.target.cid).not.toBe(before) - // The store adopted the new version because it differs in `closed` ALONE. Nothing was rejected, - // so this is not an edit annotation — which is exactly how a sanctioned edit differs from a - // record somebody rewrote. + expect(closed && isEnded(closed)).toBe(true) + expect(closed?.disposition).toBe('completed') + expect(harness.network.counts('createRecord')).toBe(1) + expect(harness.network.counts('putRecord')).toBe(0) expect(space().index.edits).toEqual([]) expect(space().index.ignored).toEqual([]) @@ -933,40 +925,39 @@ describe('a goal’s two endings, from the browser', () => { const reopened = space().index.goals.find((goal) => goal.target.uri === uri) if (!reopened) throw new Error('goal missing') await write(closeArgs(reopened, false)) - expect(isClosed(space().index.goals.find((goal) => goal.target.uri === uri)!)).toBe(false) + expect(isEnded(space().index.goals.find((goal) => goal.target.uri === uri)!)).toBe(false) expect(space().index.edits).toEqual([]) }) - it('refuses to close somebody else’s goal before a request is sent', async () => { + it('lets another active member end the goal', async () => { const harness = await open() signInAs(FIXTURE_DIDS.ana, harness.network) const theirs = space().index.goals.find((goal) => goal.target.did === FIXTURE_DIDS.tim) if (!theirs) throw new Error('fixture goal missing') harness.network.reset() - await expect(write(closeArgs(theirs, true))).rejects.toThrow(/author/) + await write(closeArgs(theirs, true, 'dropped')) expect(harness.network.counts('putRecord')).toBe(0) - // No new record either: without the URI check, a bare-rkey edit would have minted a *second* goal - // in her own repo at the same rkey and looked like it worked. - expect(harness.network.counts('createRecord')).toBe(0) + expect(harness.network.counts('createRecord')).toBe(1) + expect(space().index.goals.find((goal) => goal.target.uri === theirs.target.uri)?.disposition).toBe('dropped') }) - it('archives a goal as any active member, and unarchives it the same way', async () => { + it('parks a goal as any active member, and reopens it the same way', async () => { const harness = await open() signInAs(FIXTURE_DIDS.ana, harness.network) - const theirs = space().index.goals.find((goal) => goal.target.did === FIXTURE_DIDS.tim && !goal.archived) + const theirs = space().index.goals.find((goal) => goal.target.did === FIXTURE_DIDS.tim && !goal.ended) if (!theirs) throw new Error('fixture goal missing') const uri = theirs.target.uri - await write(archiveArgs(theirs, true)) - expect(space().index.goals.find((goal) => goal.target.uri === uri)?.archived).toBe(true) + await write(closeArgs(theirs, true, 'parked')) + expect(space().index.goals.find((goal) => goal.target.uri === uri)?.disposition).toBe('parked') const archived = space().index.goals.find((goal) => goal.target.uri === uri) if (!archived) throw new Error('goal missing') - await write(archiveArgs(archived, false)) - expect(space().index.goals.find((goal) => goal.target.uri === uri)?.archived).toBe(false) - // A standalone record each time, so the goal itself was never touched: archiving is an annotation - // any member may make, closing is the author's edit, and the two never became one control. + await write(closeArgs(archived, false)) + expect(space().index.goals.find((goal) => goal.target.uri === uri)?.ended).toBe(false) + // A standalone record each time, so the goal record itself was never touched — and `parked` is a + // word on one ending rather than a second control with its own permission rule. expect(harness.network.counts('putRecord')).toBe(0) expect(space().index.ignored).toEqual([]) }) @@ -1164,3 +1155,25 @@ describe('the fake PDS', () => { expect(stored?.value).toMatchObject({ title: 'Round trip' }) }) }) + +describe('a session that predates the permission it needs', () => { + it('says what fixes a scope refusal, instead of repeating the PDS’s words', () => { + // `RADIAL_SCOPE` is derived from `RADIAL_WRITES`, so shipping a collection — `closeGoal` — narrows + // what an already-issued token authorizes relative to what the app now writes. The PDS refuses in + // its own vocabulary, which names nothing a person can do; re-authorizing is what fixes it. + const refused = writeFailure(new XrpcError(403, 'InvalidToken', 'Bad token scope')) + expect(refused).toBeInstanceOf(Error) + expect((refused as Error).message).toMatch(/sign out and sign in again/i) + expect((refused as Error).message).toMatch(/Nothing was written/) + expect(writeFailure(new XrpcError(401, 'InsufficientScope', 'nope'))).not.toBeInstanceOf(XrpcError) + }) + + it('leaves every other failure exactly as it arrived', () => { + // A message this app invented over a failure it did not understand is worse than the server's own. + const swap = new XrpcError(400, 'InvalidSwap', 'Record was at a different cid') + expect(writeFailure(swap)).toBe(swap) + const rejected = new Error('title must be at most 300 characters') + expect(writeFailure(rejected)).toBe(rejected) + expect(writeFailure('not an error at all')).toBe('not an error at all') + }) +}) diff --git a/packages/ui/src/lib/write.ts b/packages/ui/src/lib/write.ts index 2ed9ac7..8ba8abb 100644 --- a/packages/ui/src/lib/write.ts +++ b/packages/ui/src/lib/write.ts @@ -38,7 +38,14 @@ // Both are answered out of the fold this tab is already holding rather than off the network: the // CLI's versions of them go and look because a shell has nothing to look in. -import { FetchRepoTransport, StrongRefResolver, parseAtUri, resolveHandleDid, type RepoReadTransport } from '@radial/atproto' +import { + FetchRepoTransport, + StrongRefResolver, + XrpcError, + parseAtUri, + resolveHandleDid, + type RepoReadTransport, +} from '@radial/atproto' import { type Collection, type RecordByCollection, type StoredRecord, type StrongRef } from '@radial/core' import { runCli, type CliDependencies, type CommandResult, type RecordWriter } from '@radial/sidecar' import { signedIn, type Credentials } from './auth.svelte.js' @@ -83,6 +90,33 @@ function dependencies( const OPTIMISTIC_REV = '\uffff-ui-write' +/** + * The one write failure a reader cannot act on as reported: their session predates the permission it + * needs. + * + * The scope is derived from `RADIAL_WRITES`, so shipping a new collection — `closeGoal`, most + * recently — narrows what an *already issued* token authorizes relative to what the app now writes. + * The PDS is right to refuse, and it refuses in its own words ("Bad token scope"), which name nothing + * a person can do about it. Signing out and back in re-authorizes with the current scope, so that is + * what the message says. + * + * Detection is by the PDS's own error codes rather than by the prose: `InvalidToken` and + * `InsufficientScope` are what a granular-scope refusal arrives as, and the two are checked with the + * status so an unrelated 400 keeps its own message. Anything else passes through untouched — a + * message the app invented over a failure it did not understand is worse than the server's. + */ +export function writeFailure(error: unknown): unknown { + const scopeRefusal = + error instanceof XrpcError && + (error.status === 401 || error.status === 403) && + ['InvalidToken', 'InsufficientScope', 'BadTokenScope'].includes(error.error) + if (!scopeRefusal) return error + return new Error( + 'This sign-in was authorized before Radial asked for this permission — ' + + 'sign out and sign in again to grant it. Nothing was written.', + ) +} + /** * Keep the validated values sent by runCli beside the durable refs returned by the PDS. That pair * is everything the fold needs except the repo revision, which is observer-local and reconciled by @@ -138,7 +172,15 @@ export async function write(args: string[]): Promise { const records: StoredRecord[] = [] const writer = recordingWriter(credentials.writer, credentials.did, records) - const result = await runCli(args, dependencies(credentials, transport, writer)) + let result: CommandResult + try { + result = await runCli(args, dependencies(credentials, transport, writer)) + } catch (failure) { + // A command can write several records; whatever landed before the refusal is still folded, since + // the PDS acknowledged it and the poll would find it anyway. + publishWritten(records) + throw writeFailure(failure) + } publishWritten(records) return result } @@ -159,5 +201,9 @@ export async function createSpace(args: string[]): Promise { const credentials = signedIn() if (!credentials) throw new Error('Sign in to create a space — it is a record in your own repo.') const transport = spaceTransport() ?? new FetchRepoTransport(new Identities().pdsResolver) - return runCli(args, dependencies(credentials, transport)) + try { + return await runCli(args, dependencies(credentials, transport)) + } catch (failure) { + throw writeFailure(failure) + } } diff --git a/packages/ui/src/routes/g/[did]/[rkey]/+page.svelte b/packages/ui/src/routes/g/[did]/[rkey]/+page.svelte index 5fa6555..24984bf 100644 --- a/packages/ui/src/routes/g/[did]/[rkey]/+page.svelte +++ b/packages/ui/src/routes/g/[did]/[rkey]/+page.svelte @@ -14,8 +14,8 @@ import { dayKey, dayLabel, shortDate } from '$lib/format.js' import { currentSpace } from '$lib/session.svelte.js' import type { ThreadMessage } from '$lib/replies.js' - import { goalByRoute, isClosed, unitsOf } from '$lib/space.js' - import { needsVerdict } from '$lib/units.js' + import { goalByRoute, unitsOf } from '$lib/space.js' + import { endingBadge, needsVerdict } from '$lib/units.js' // The heart of the product: one goal, its work units in the order they happened, and the thread. const space = $derived(currentSpace()) @@ -24,6 +24,8 @@ const counts = $derived(tally(units)) const toJudge = $derived(units.filter(needsVerdict).length) const author = $derived(goal ? space.directory.get(goal.target.did) : undefined) + /** How this goal ended, if it has. The same badge the Logbook and the project page draw. */ + const ending = $derived(goal ? endingBadge(goal) : undefined) /** The message the composer is answering, when Reply pointed it at one. */ let replyTo = $state(undefined) @@ -67,11 +69,9 @@ {#if toJudge > 0} {/if} - {#if isClosed(goal)} - - {/if} - {#if goal.archived} - + {#if ending} + + {#if goal.endedAt}{shortDate(goal.endedAt)}{/if} {/if}

    diff --git a/packages/ui/src/routes/logbook/+page.svelte b/packages/ui/src/routes/logbook/+page.svelte index 52ccb7f..acb1cd5 100644 --- a/packages/ui/src/routes/logbook/+page.svelte +++ b/packages/ui/src/routes/logbook/+page.svelte @@ -3,19 +3,23 @@ import Glyph from '$lib/components/Glyph.svelte' import GoalRow from '$lib/components/GoalRow.svelte' import { currentSpace } from '$lib/session.svelte.js' - import { isClosed } from '$lib/space.js' + import { isEnded } from '$lib/space.js' import { matches, ui } from '$lib/ui.svelte.js' - // Closing is a human edit to the goal record (design §7). An observed merge annotates an - // artifact; it never closes a goal by itself, and it never will. + // Every goal that has ended, however it ended, newest ending first — the list `GoalView.ended` is + // the definition of. It is wider than it used to be: a goal an old client archived was on no list at + // all, and it is here now. An observed merge annotates an artifact; it never ends a goal by itself, + // and it never will. const space = $derived(currentSpace()) // Once for the page, not once per row: the fold walks every live target's open requests. const byGoal = $derived(asksByTarget(asks(space.index))) - const closed = $derived( + const ended = $derived( space.index.goals - .filter((goal) => isClosed(goal) && matches(`${goal.target.value.title} ${goal.target.value.body}`)) + .filter((goal) => isEnded(goal) && matches(`${goal.target.value.title} ${goal.target.value.body}`)) .sort((left, right) => - right.target.value.createdAt.localeCompare(left.target.value.createdAt), + (right.endedAt ?? right.target.value.createdAt).localeCompare( + left.endedAt ?? left.target.value.createdAt, + ), ), ) @@ -25,17 +29,16 @@

    Logbook

    -

    Goals their author closed.

    +

    Goals that have ended, and how.

    -{#if closed.length === 0} +{#if ended.length === 0}
    -

    {ui.query ? 'No closed goal matches that.' : 'Nothing closed yet.'}

    -

    A goal closes when its author closes it, not when a PR merges.

    +

    {ui.query ? 'No ended goal matches that.' : 'Nothing ended yet.'}

    {:else}
      - {#each closed as goal (goal.target.uri)} + {#each ended as goal (goal.target.uri)} {/each}
    diff --git a/packages/ui/src/routes/p/[project]/+page.svelte b/packages/ui/src/routes/p/[project]/+page.svelte index 4663c7c..70f7169 100644 --- a/packages/ui/src/routes/p/[project]/+page.svelte +++ b/packages/ui/src/routes/p/[project]/+page.svelte @@ -5,7 +5,7 @@ import GoalRow from '$lib/components/GoalRow.svelte' import Glyph from '$lib/components/Glyph.svelte' import { currentSpace } from '$lib/session.svelte.js' - import { goalsOf, isClosed, projectByName, settingsHref } from '$lib/space.js' + import { goalsOf, isEnded, projectByName, settingsHref } from '$lib/space.js' import { matches, ui } from '$lib/ui.svelte.js' const space = $derived(currentSpace()) @@ -20,8 +20,8 @@ ) : [], ) - const active = $derived(goals.filter((goal) => !isClosed(goal))) - const closed = $derived(goals.filter(isClosed)) + const active = $derived(goals.filter((goal) => !isEnded(goal))) + const ended = $derived(goals.filter(isEnded)) const autoReview = $derived( project @@ -82,10 +82,10 @@ {/if} - {#if closed.length > 0} -
    Logbook {closed.length}
    + {#if ended.length > 0} +
    Logbook {ended.length}
      - {#each closed as goal (goal.target.uri)} + {#each ended as goal (goal.target.uri)} {/each}
    -- 2.51.2