oficina / sheets Public

<task-notification> <task-id>aea5115a570d8d214</task-id> <tool-use-id>toolu_01G6JpApAb58Jwx7mvon3WMt</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-sheets-core/aa9e75b3-42dc-4311-8286-65350f750d67/tasks/aea5115a570d8d214.output</output-file> <status>completed</status> <summary>Agent "Fable review of AGENTS.md branch" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**VERDICT: REQUEST CHANGES.** The branch does what it says for the three named retractions, and the gate-2 wording now matches §12. But it misses a fourth retraction of exactly the same kind, from the same spec commit, in the same section it edited, and its one new paragraph misstates the spec's reasoning it claims to be preserving. Everything below was verified against the worktree at `/home/paulca/amadan.net/oficina/home/.claude/worktrees/agents-md-spec-sync` (`b19f0fa`, parent `ad6dd30`, one file changed: `AGENTS.md`). Reviewer disclosure, per §Review: I am Fable 5.1; the author was Claude Opus 5. Record that in the discussion however the capability ordering is resolved. --- ## Findings, by severity ### 1. Confirmed, blocking. A fourth retraction from `da52b09` was missed: the boundary test `AGENTS.md` lines 107-110 still say: &gt; A claim may ride an assertion only if an instance would &gt; behave identically given a hostile value. A wrong theme is ugly; a wrong &gt; role is a breach. Spec §5.4, lines 373-378, added in `da52b09`: &gt; **Two things the first wording got wrong**, recorded so they are not &gt; reintroduced. "The instance would behave identically" is false even for &gt; theme — a hostile value gives an ugly page, which is different behaviour — &gt; and a test its own example fails is a gap someone smuggles through. And &gt; "hostile value" assumed the validation that makes it safe: a free-string &gt; theme interpolated into CSS is an injection, not an ugly page. I traced it: the first-draft spec (`53268ec` line 223) had "claim may ride the assertion only if the instance would behave identically given a hostile value"; `AGENTS.md` copied it; `da52b09` replaced it in the spec with the two-sided test and explicitly retracted it, while in the same commit adding an `AGENTS.md` bullet (now lines 120-124) that *cites the retracted test by name* ("the 'hostile value changes nothing' test"). So the binding file carries the retracted wording plus the spec's own counter-example ("A wrong theme is ugly") as if it were support. It also carries only half the rule. The spec's test (lines 355-359) is: &gt; A claim may ride the assertion only if **(a)** the instance makes no trust &gt; decision based on it, validating against a **closed enumerated set** and &gt; treating a failed match as *absent* rather than as the raw value; **and &gt; (b)** the home already holds it for its own purposes, so carrying it adds &gt; nothing to what the home knows. Side (b) is absent from `AGENTS.md`, and (b) is the side that blocks "which documents you can see" (spec line 361-363), the most plausible good-faith proposal. This is the same failure class as the three fixed, two bullets above the one the diff touched, and it is in scope for a commit whose subject is "carry the spec's three retractions". There were four. ### 2. Confirmed, in scope. The new paragraph misstates §3.3's mechanism `AGENTS.md` line 32-33 (new): &gt; §3.3 retracts that too, because one &gt; known membership identifies the row and reveals the rest. Spec §3.3 lines 194-197: &gt; Hostnames approximate company names, so each person carries a team-set &gt; fingerprint. One outside fact — "alice consults for Acme and BigCo" — &gt; identifies the row and reveals the rest of their memberships, guest rows &gt; included. The spec's mechanism is a team-*set* fingerprint plus one outside fact about that set; the example is two memberships. "One known membership identifies the row" is not what the spec says and is generally false (one membership in a fifty-person team identifies nothing). The same misstatement appears in the commit message ("one known membership identifies the row") and the discussion table. For a paragraph whose purpose is to stop the retraction being reverted by "anyone who has not read the spec's reasoning", it gets the reasoning wrong. Smaller drifts in the same paragraph: - "each is a claim the schema cannot keep" applies the spec's reason for the *address* retraction (line 32-33: "Saying 'never' would have been a claim the schema could not keep") to "unidentified" as well. The "unidentified" retraction is about what the published graph reveals, not the schema. - The parenthetical cites "(§5.1)" for "sealed, enumerated and short-lived"; the spec's own pointer at §0 line 29 is "see §3.2", and §5.1 itself says "enumerated in §3.2". ### 3. Confirmed, in scope. "Sharing is membership" is unsupported and contradicted `AGENTS.md` line 54-55: &gt; **Never federate.** Data does not cross an instance boundary. Sharing is &gt; membership. Spec §2.2 line 124: "**Never federates.** Data does not cross an instance boundary." — nothing more. The phrase "Sharing is membership" appears nowhere in the spec, and §6 line 432-434 rules the opposite distinction: &gt; `kind` is member or guest, because a row implying an org &gt; membership someone does not have is a lie the directory tells on the org's &gt; behalf. §5.5 adds link-share visitors whose guest grant is minted at sign-in. Sharing with an outsider is a *guest grant on the instance*, which the spec insists is not membership. If a sentence is wanted here: "Sharing is a grant on the instance — member or guest — never a copy." ### 4. Confirmed, in scope. The "Two tests ... from the first commit" sentence is still wrong in three ways `AGENTS.md` lines 36-39: "Two tests enforce it from the first commit: a plaintext grep over the raw SQLite file and WAL, and a no-address-at-rest test ..." - Gate 1 is still short. Spec §12 gate 1: "raw SQLite file and WAL **plus this app's store objects**, against a seeded corpus of known addresses." The commit updated gate 2 and left gate 1 without the bucket — the very place the Data bullet says a transient write ends up. - Spec §13 step 1 is "Skeleton, `AGENTS.md`, gates 1, 2 **and 9**." Three first-step gates, not two. - "from the first commit" is false at source. The skeleton (`ef8a30f`) has shipped; `internal/home/app_test.go` contains `TestIndex` and `TestHealthz` only, and `app.go` line 21-23 says "AGENTS.md's 'no address at rest' tests have nothing to guard until real storage exists." Honest wording: "from the first commit that opens a database." ### 5. Confirmed, in scope by the same-commit rule. "The home does not hibernate" never reached `AGENTS.md` Spec §2.1 lines 105-107, added in `da52b09`: &gt; **The home does not hibernate.** It is the one app on the estate where &gt; cold-wake latency is a suite-wide outage. Stated because hibernation is &gt; the platform default and would otherwise apply silently. `AGENTS.md` mentions hibernation only for instances (lines 112-113, 126). The Deploys section has nothing. This is the highest-consequence omission in the sweep because the spec says the default applies silently; an agent deploying from `AGENTS.md` alone will ship a hibernating identity authority. Same commit, same spec revision, same rule that motivated this branch. ### 6. Confirmed, minor drift in the diff. Expiry bullet `AGENTS.md` line 114: "Widening it costs nothing: **single-use `jti` is what stops replay, not brevity.**" Spec line 338: "Widening it costs nothing **against the threat model**, because **single-use `jti` is what stops replay**, not brevity." The qualifier is doing work: widening does cost something absent `jti` (a larger replay window) and with it (a larger `jti` store, which §5.3 sizes "by assertion lifetime"). The rest of the bullet matches the spec's wording and reasoning; dropping "process start plus decrypt plus migrate-check" is acceptable compression. ### 7. Confirmed, minor. Log rule drops one item `AGENTS.md` line 149-150: "No address, team name, or queue payload in any log line, error string or metric label." Spec §3.4 line 211-213: "no address, team name, or queue payload in any log line, error string, **job status message** or metric label." Job status messages are what the drain writes; it is the one an implementer of §7 would hit. ### 8. Confirmed. Commit message and discussion accuracy - **`Reviewed-By: Fable (dispatched; found the divergence)`** trailer on a commit the discussion describes as "**Not yet reviewed.**" Fable reviewed a different brief and noticed the staleness as an aside; it did not review this diff. A `Reviewed-By` trailer is the review record the §Review section says must be attributable, and this one attributes a review that did not happen. Drop it or rename it (`Found-By:`). - **"The revision in da52b09 changed the spec and left this file behind"** (commit message; the brief repeats it as "left behind by the spec revision"). Not quite: `da52b09` edited `AGENTS.md` too, 29 lines, adding the Data bullets, the identity-exemption bullet, the prune bullet and the Frontend correction. The file was synced *partially* in the same commit, not forgotten. That matters for what to guard against: the failure mode is an incomplete same-commit sweep, which is also what this branch just repeated (findings 1, 4, 5). - **"All four .amadan/ci.d gates green"**: true. I ran `10-vet`, `20-fmt`, `30-build`, `40-go-test` on the worktree; all exit 0. But the four gates are `go vet`, `gofmt -l`, `go build`, `go test` over four Go files; none reads `AGENTS.md`. The claim is accurate and is not verification of this change. - "No behaviour change; `internal/home` is untouched": true, the diff touches only `AGENTS.md`. - "matching the spec word for word" for the one-rule sentence: true (line 19-21 vs spec 17-19 identical). ### 9. Confirmed. The `main` problem `git ls-remote --heads origin` on `https://amadan.net/oficina/home`: ``` da52b09... refs/heads/design b19f0fa... refs/heads/fix/agents-md-matches-spec ad6dd30... refs/heads/skeleton ``` No `main`; `refs/remotes/origin/HEAD` is not set. `design` is an ancestor of `skeleton`, and this branch was cut from `skeleton` (`ad6dd30`). So `AGENTS.md` lines 81-88 ("Never directly on `main`", "cut from `main`", "Squash-merge to `main` only when...") describe a repository that does not exist, and the branch under review already violated the letter of rule 1 by necessity. The spec is silent on branch names, squash, or worktrees (grepped: none), so this is an `AGENTS.md`-level ruling with no spec backing either way. Honest options, none of which I am recommending over the others because it is Paul's ruling to amend: (a) Paul designates `skeleton` as trunk for now and the cadence section says so, with `main` as the eventual name; (b) the section is left as written and each merge to `skeleton` records in its discussion that it is merging to the de-facto trunk under an unmet rule; (c) Paul creates `main` himself. The discussion body already flags this correctly. What is not acceptable is merging this branch while `AGENTS.md` asserts a cadence the merge itself will not follow, without the discussion saying which of (a)/(b) applies. --- ## Item 4: the "do not restore these" note **Keep side.** The file already records tempting mistakes inline ("This shipped twice in one spec before it was caught"; the Frontend parenthetical; the identity-exemption bullet says "or it proves too much and gets argued around"), so the note is not a new precedent. Quoting the retracted phrases verbatim makes a future regression greppable. The address half genuinely needs a reason: "readable" reads like weasel-wording until you know about the magic link. **Cut side.** The reasoning for the address half already lives in the Data bullet (lines 132-138: sealed before written, Litestream, `secure_delete`), so the note duplicates it in the most-read part of the file, before the four nevers. The "unidentified" half is the only new information, and it is wrong (finding 2). Eight lines that begin "The first draft said" is spec-history, not a rule; `AGENTS.md`'s job is to compress the spec, and the spec's §0 and §3.3 already carry this history for anyone who follows the pointer at line 11. **Recommendation: shorten to a two-line footnote after the falsifiable form, with the mechanism corrected.** Proposed text: &gt; *Two absolutes were argued and lost, and stay lost: "never stores your address" (a magic link's address is at rest between send and click — sealed, §3.2, not absent) and "some unidentified person" (a team-set fingerprint plus one outside fact names the row, §3.3).* --- ## Item 3: the systematic sweep — remaining divergences In scope for this commit (same section touched, or same-commit rule): findings 1, 2, 3, 4, 5, 6, 7 above. Follow-up (real omissions or drift, but not this branch's job): - **§14 and §15 are unreflected.** `AGENTS.md` line 11-13 points at §1 as the settled table; nothing points at §14's not-yet table with triggers. Most relevant for an agent: per-team 2FA "**NOT in slice one**" (ruling 15) and "Ruling 15 exists because the per-team 2FA floor would have breached this" (§2.2 line 122-123), which `AGENTS.md`'s never-2 omits; no operator reset; suite-wide sign-out named as an exposure; account deletion. §15's `rastrillo/rastrillo` #1, `carlos/platform` #4, `carlos/pegamento` #1 are unmentioned (only #2 is). One line "§14 is the not-yet table; do not build a row without its trigger" would do. - **RP ID is the full host, never a suffix** (ruling 13; §4 line 239-244, "asserted by a test"). A permanent constraint with its own test, like the credential-origin rule `AGENTS.md` does carry. Absent. - **"The capability never rides the assertion"** and the server-built return URL with nonce (§5.5 line 392-396), plus the redirect protocol's return-host allowlist and browser-bound state nonce (§5.6). These are the rules Sheets and Docs will violate in good faith. Absent. - **Audience**: `AGENTS.md` line 117 says "app and instance host"; spec `aud` row adds "The directory origin has its own audience form." The directory also exchanges assertions (§4 line 230-232). - **What is sealed** (§3.2 line 170-171): team display names, queue payloads, rekey-journal addresses, link state. `AGENTS.md` line 133-134 gives the magic link as the only address-at-rest case; §7 line 494-497 says the queue "holds readable addresses ... That is new custody." An agent reading only `AGENTS.md` would not know. - **Coarsened-to-the-day timestamps** (§3.3 line 199-200) — a mechanical rule, absent. - **Rate-limit keys stored hashed**, entry TTL with sealed-payload purge, name reserved at enqueue (§7 queue mechanics) — absent. - **Written RTO** (§2.1, §12 gate 8): `AGENTS.md` line 171 has the drill but not the RTO. - **`kid` handling** (refetch once on unknown, then fail closed) and per-verifier self-pruning `jti` store (§5.3) — absent; the prune bullet gestures at it. - **§3.1's second threat model** (pepper leaks: "a dump with names") and its mitigation "an expensive KDF rather than a bare HMAC" — absent. Note for the spec owner, not this branch: ruling 3 and §6 line 431 both say `HMAC(pepper, lowercased address)` while §3.1 line 163-164 says the index is *not* a bare HMAC. The spec contradicts itself; `AGENTS.md` should not copy either until that is settled. - Statements in `AGENTS.md` the spec does not support but does not contradict, kept honest by other sources: "The bare-attribute grammar is ruled but not released" (no spec mention; presumably rastrillo); "One binary, `CGO_ENABLED=0`, cgo-free SQLite" (Makefile supports it; spec silent); "A promote is not a deploy" (platform knowledge). Fine as is, but they are the file's own claims, not the spec's. - The Review section's "highest-capability model available", "discovered, not hardcoded", and "A review by a weaker model than the author is not a review" have no spec counterpart; §12 line 683-684 says only "including which model reviewed it". Not a divergence (the spec is silent), but worth knowing that this ruling lives only in `AGENTS.md`. --- ## What must change before approval 1. Replace the boundary-test bullet with the spec's two-sided test and fix the later bullet that cites the retracted name (finding 1). 2. Correct the "one known membership" mechanism, or adopt the shortened note above (finding 2). 3. Drop or replace "Sharing is membership" (finding 3). 4. Fix the gates sentence: add gate 1's store objects and corpus, count gate 9, and stop claiming tests exist that do not (finding 4). 5. Add "The home does not hibernate" to Deploys (finding 5). 6. Restore "against the threat model" and "job status message" (findings 6, 7). 7. Remove or rename the `Reviewed-By: Fable` trailer, and correct "left this file behind" (finding 8). 8. In the discussion, say which trunk this merges to and under which reading of the cadence section (finding 9). Do not create `main` to satisfy the rule.</result> <usage><subagent_tokens>87118</subagent_tokens><tool_uses>15</tool_uses><duration_ms>290572</duration_ms></usage> </task-notification>