Repository navigation
fix(host): a Bot Social read answers only for the Bot the actor owns (#1788) - #1822
Conversation
|
@Birfy — review request for PR #1822 at tip Scope: one narrowed read. Checks at this tip: A Verdict naming that tip is what this PR is waiting for; it will not be re-pushed for review. |
|
Verdict: PASS at c1fb9ac Ran at this tip: Review notes (tip
|
The plugin read branch admitted any signed-in actor and never looked at the body: the friends and grants of a Bot were served to a member who named another member's botId, and the handler registered that Bot's Bot Social identity and synced its outgoing grants on the way. Narrow that one read, in authz, before the handler runs; every other plugin read keeps its rule. This narrows a design decision written into the code (ThinkFlowLab#1788, owner-approved tightening), it is not a missing-check fix. Tests: core/test/authz.test.js
…t accepts (ThinkFlowLab#1823) Fixes ThinkFlowLab#1820 ## What was wrong The Project settings panel's **Save memory** button never worked, for anyone including the Project owner. The panel wrote `kind: 'note'` (`core/src/client/project-board.js`), and the host accepts only `decision` / `question` (`core/src/host/store.js:502`, `project-assistant.js:185`, `MEMORY_KINDS` at `:28`), so every press ended in `bot/invalid: Unknown Project memory kind note`. The success line was unreachable, and a zh UI showed the host's English refusal verbatim. The old test pinned that payload, so green CI never caught it. The base is `2ea55d8a4d67cd1e4fe489ab2b14b1946cea4dd6` (live `main` when this was pushed); the issue's `adbaf320fed0` is stale (`ThinkFlowLab#1815`, `ThinkFlowLab#1817`, `ThinkFlowLab#1818` and `ThinkFlowLab#1822` have landed since). ## What changed - `core/src/client/project-board.js`: the memory box sends `kind: 'decision'` — the host's own default when a writer omits `kind` (`project-assistant.js:184`), and the only accepted kind that is not an open question. The status line goes through `hostText`, so a refusal is quoted in the UI language. The other two write paths are unchanged. - `core/src/client/i18n.js`: three `HOST_ERRORS` entries, only for refusals this panel's three write paths can actually surface — the owner gate on all three (`Only the owner of this Project may change it`), an empty member id on Bind/Unbind (`A binding needs the member id`), and a stale Project id (`Unknown Project (\S+)`). The id rule is last and takes one token, so it cannot swallow a longer `Unknown Project …` sentence; the test keeps `Unknown Project status active` unchanged so that stays proven. No other entry was added. - `core/test/project-board-panels.test.js`: the stub that pinned `kind: 'note'` is replaced by a real round-trip through `rt.projectEndpoint` (`call: (event, payload) => rt.projectEndpoint(event, payload)`), with a control that a `decision` write lands in the memory, a control that `note` is refused and writes nothing, and a zh assertion on the refusal and on `记忆已保存。`. - `docs/subsystems/core-client.md`: one paragraph on the panel's three write paths and the memory-kind mapping. - `changelog.d/1820-project-memory-save.fixed.md`. - Generated surface: `core/client.js`, `patches/ds-bot/0001-core-fixes.patch`, `patches/ds-bot/0008-client-bundle.patch`, and the two `patches/ds-bot/files/...` mirrors. The bulk of the diff is `patches/ds-bot/0008-client-bundle.patch` (+5,023/−2,573 of the +5,342/−2,598): that patch is machine-generated whenever the client bundle is rebuilt and is never hand-edited — `npm run build-client` regenerates it, and `node scripts/ds-bot-patches.mjs check` printed the same three green lines on two consecutive runs. `0012` / `0014` were reverted when the generator only changed hunk headers. ### No display surface today This fix adds no display surface and no new wording for one. The only reader of a `decision`-kind row is the `project-memory` endpoint, and no client code calls it; the approved P-228 read-back panel is the one future surface, and it labels these rows **memory entries** (记忆条目), never "decisions". The audit below is why `decision` is the correct side of the host's two-kind contract today. ## Memory kind mapping The panel has no type selector: every Save memory press writes one free-text row. `note` is not a kind the host accepts, so the panel sends `decision`. This is a client-side mapping only — `MEMORY_KINDS` (`project-assistant.js:28`) and the host's kind checks (`store.js:502`, `project-assistant.js:185`) are unchanged. **Every reader of a `decision`-kind row (base `2ea55d8a`):** 1. **`project-memory` endpoint** (`store.js:606`, routed by `channel.js:676`, owner-gated at `store.js:605`) — returns `{project, memory: {decisions, questions, digest}}`. Its only caller is a test today: `core/src/client/` contains no `project-memory` read at all (the bundle has `project-memory-write` and the `project-memory-save` action, never the read endpoint), and no page renders the response. This is the endpoint the approved P-228 read-back panel will use; today the row has no display surface. 2. **The digest** (`project-assistant.js:159` `memoryOf` → `projectMemoryFor`) — `digestForOwner` (`:432`) reads only `memory.digest` (`:447`, `:450`); `buildDigest` (`:61`) takes `project/rows/files/actionable` and no `decisions`/`questions`, so a memory row never appears in the digest text, and the word "decision" is never printed from it. 3. **The board's Decisions section** (`store.js:656`) — comes from `project-board.js:150` `decisionsOf(project)`, which lists `workDir/decisions/*.md` on disk. Different source from `state.projectMemory`; a memory row never shows there and is never labelled a decision. 4. **`rt.projectAssistant.read` / `write`** (`:176`, `:182`, exposed at `:508`) — the internal API returns the owner's `{decisions, questions, digest}`, but nothing calls `read` (the assistant Session registers only `project_digest`, `:513`) and no `project_memory_write` tool is registered (the instructions at `:37` name one that does not exist). No assistant-side display path. 5. **The write response** — `project-memory-write` returns the updated `memory` (`store.js:612` → `ownerView`), and the panel accepts it, but `run()` uses it only for the success/refusal status line; `onSaved` is not passed by any caller, so no entry is rendered. Measured on `2ea55d8a` with the real endpoints (throwaway probe, not committed): ```text READBACK decisions => ["memory row: ship Friday"] BOARD decisions => {"dir":"...","files":[{"path":"decisions/2026-10-30-ship.md",...}],"reason":""} DIGEST => "Project Assistant (Ops) — 2026-10-30: no change since the previous digest." ``` **Why `decision` and not `question`.** `question` is the host's list for open questions; the free text a person types on Save memory is a statement or a settled call, so it belongs on the "recorded, no longer open" side. `decision` is also the host's own default when a writer omits `kind` (`project-assistant.js:184`), so the panel sends explicitly what the host would have assumed. **The trade-off and its condition.** Mapping to `decision` is deliberate and holds only while no surface renders such a row under the word "decision": today none does (readers 1–5 above), and P-228 renders them as memory entries. It does not hold by itself. The approved P-228 read-back panel will list both arrays, and if it labels a `decisions` row as a decision, a person's typed memory would read as a decision they never made. For this mapping to stay honest, P-228 must render these rows as **memory entries** (记忆条目) — or separate "recorded memory" from "decisions" — and that requirement is P-228's acceptance, not this PR's. The alternative here would be a third host kind (a new contract with no reader, which the issue excludes) or a type selector in the panel (also out of scope). ## Red first Re-taken on the final base `2ea55d8a4d67cd1e4fe489ab2b14b1946cea4dd6`, with only `core/src/client/project-board.js` reverted to that base and the new tests kept: ```text not ok 3 - the settings panel memory save lands in the Project memory through the real endpoint error: |- the panel reports the state the host answered + actual - expected + 'Unknown Project memory kind note' - 'Memory saved.' not ok 4 - a memory kind the host does not know writes nothing, and the panel quotes a refusal in Chinese error: |- Expected values to be strictly equal: + actual - expected + 'Only the owner of this Project may change it' - '只有这个项目的主人可以修改' # pass 4 # fail 2 ``` The two failures are the two contracts this PR fixes: the panel sends a kind the host refuses, and the panel quotes the host's English refusal instead of the localized one. With the fix, the same file is 6/6. ## Verification at the tip At the pushed tip `7c374bb32f38241d4d0f937f4f4ddcd11f45057c` (tree `b1f5352cd2e2907437b75addc5dd2aba6291aca3`, the tree `npm run verify` ran on locally as `06ac78a59af1738ff420f4e18592a6067394e044`): ```text TZ=UTC ARCH_DOCS_BASE=2ea55d8a4d67cd1e4fe489ab2b14b1946cea4dd6 npm run verify ``` - core suite: **2182/2182** pass, 0 fail. - core lint: **71 warnings, 0 errors**. - `arch-docs`: **OK (10 changed file(s))** with `ARCH_DOCS_BASE=2ea55d8a4d67cd1e4fe489ab2b14b1946cea4dd6`. - `build-client --check`: green. - `ds-bot-patches check` run twice, identical three lines: ```text ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5) ok 10 patches apply cleanly in order ok core/ equals ds-bot/ + patches/ds-bot/ ``` - the repo changelog test (`ok 54`) is green; `changelog.d/1820-project-memory-save.fixed.md` is the new fragment. - the remaining plugin / script suites are green (541, 127, 135, 294 and 549 tests), and `git status` is clean apart from the untracked `node_modules` symlinks the bench needs. 10 files changed, +5342/−2598: `changelog.d/1820-project-memory-save.fixed.md` +1/−0, `core/client.js` +12/−3, `core/src/client/i18n.js` +7/−0, `core/src/client/project-board.js` +7/−4, `core/test/project-board-panels.test.js` +132/−7, `docs/subsystems/core-client.md` +7/−0, `patches/ds-bot/0001-core-fixes.patch` +14/−0, `patches/ds-bot/0008-client-bundle.patch` +5023/−2573, `patches/ds-bot/files/src/client/project-board.js` +7/−4, `patches/ds-bot/files/test/project-board-panels.test.js` +132/−7. Not run: the smoke bench (no `index.js` or `core/src/host/` change), the pack check, and actionlint — `npm run verify` does not include them. Owner: Dev-3 Reviewer: Birfy <!-- flybot:checkpoint --> ```json { "pr": 1823, "claim": "Dev-3", "issue": 1820, "summary": "The Project settings memory box now writes kind decision, the host's own default, so Save memory stores the text and shows the localized success line. Refusals are quoted through hostText, with dictionary entries only for the three refusals this panel's write paths can surface. The stub that pinned kind note became a real projectEndpoint round-trip, with controls that decision lands and note writes nothing. No display surface is added; the future P-228 panel labels these rows memory entries.", "files": ["core/src/client/project-board.js", "core/src/client/i18n.js", "core/test/project-board-panels.test.js", "docs/subsystems/core-client.md", "changelog.d/1820-project-memory-save.fixed.md", "core/client.js", "patches/ds-bot/0001-core-fixes.patch", "patches/ds-bot/0008-client-bundle.patch", "patches/ds-bot/files/src/client/project-board.js", "patches/ds-bot/files/test/project-board-panels.test.js"], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=2ea55d8a4d67cd1e4fe489ab2b14b1946cea4dd6 npm run verify", "result": "green", "at": "7c374bb32f38241d4d0f937f4f4ddcd11f45057c" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
Written-by: Dev-4
Owner: Dev-4
Reviewer: Birfy
Fixes #1788
What was wrong
A Bot Social read served from the
api/botchannel answered for any Bot whose id the caller put in the body.flybot/social/share-friendsis registered as a read (plugins/social/src/social.js), everyflybot/*read went to any signed-in actor, and the handler then registered that Bot's Bot Social identity and synced its grants while serving it — so one member could read another member's friend list and write on their behalf.What changed
authz.jsnow refuses that one case before the handler runs:flybot/social/share-friendscarrying abody.botIdthe actor does not own answersbot/forbidden—You do not own that Bot. The read stays a read: the endpoint remains in the read set and never falls to the shared rule,SHARED_BLOCKED, tokens and the identity model are untouched, and every other plugin read stays open to every signed-in actor.Why this is a decided narrowing, not a missing check
share-friendshad zero occurrences incore/test/authz.test.js: no test ever covered a cross-memberbody.botIdread.flybot/branch ofauthz.js— reads go to any signed-in actor, writes follow the shared rule.{}and{ id: 'x' }, never abotId, so the new coverage is added and nothing is weakened or rewritten.This is a narrowing this project chose to make, deliberately, against a design already in the code — not a missing check being put back. The owner approved it before any code was written, on this issue (owner, comment 6517277684, verbatim):
(Quoted verbatim. One item did not survive contact with the code: item 4 says the single assertion at
core/test/authz.test.js:326-343is flipped, but no existing assertion guarded this allowance, so nothing is flipped — three cases are added instead. That is spelled out again directly under the quote.)Boundary 4 turned out to need nothing flipped: no existing assertion guarded the allowance (checked line by line above), so three cases are added instead and no existing assertion is edited.
Blast radius: members of the same home can no longer read each other's Bot Social friends or grants through this endpoint; the request is refused before the plugin registers that Bot's identity or syncs its grants, so nothing is written either.
Red first
The case was added on the rebased base commit
e4773eaae8903b58ebe327df8bae040b822252b3(tree9e239549fc06597704eec4b744a33a17f57c7a89), on a clean checkout of it whose only change was this branch's new test file, and run before the fix. Commit, tree and the output below come from that one run:{"ok":true}is the old behaviour: another member's Bot was served. The same case now covers the actor's own Bot (stillALLOW) and the no-actor install (un-enforcedALLOWon a single-member host;bot/unauthorizedonce authz applies).Verification at the tip
c1fb9ac6d40f5c8cff333a313c2a30825e090a1c0ecadbc948bcb26ba1a73e95e859ab75dcb6c5cbe4773eaae8903b58ebe327df8bae040b822252b3core/src/host/authz.jscore/test/authz.test.jsdocs/subsystems/core-host.mdchangelog.d/1788-social-read-owner.fixed.mdpatches/ds-bot/files/src/host/authz.jspatches/ds-bot/files/test/authz.test.jsSource 8 lines, tests 36, docs 2, changelog 1. Both generated mirrors are whole-file copies under
patches/ds-bot/files/, re-cut bynode scripts/ds-bot-patches.mjs generate; nothing in them was hand-edited and no generator or switch was added.On this base
generateleft every patch byte-identical tomainexcept0008-client-bundle.patch, which it rewrote with representation-only drift — this box's git 2.34 prints a JavaScript function context after@@ … @@where the committed chain has none — so that one was restored byte-for-byte frommain.git diff e4773eaa HEADtherefore shows the six files above and no patch of the series. Acceptance is by content, not hunk count, andcheckbelow proves the applied series still equalscore/.Checks at the tip
node scripts/ds-bot-patches.mjs check, twice, identical:TZ=UTC ARCH_DOCS_BASE=e4773eaa node core/scripts/lint.mjs-> exit 0: 0 errors / 71 warnings,arch-docs: OK (6 changed file(s)).TZ=UTC node --import ./test/harness-resolve.mjs --test test/authz.test.js-> 21/21.TZ=UTC node --import ./test/harness-resolve.mjs --test "test/*.test.js"-> 2180/2180.node core/scripts/build-client.mjs --check-> exit 0.Not run here:
node .github/scripts/enqueue-guard.mjs --pr <n>(this box has nogh; the workflow's own check run carries it), and the plugin, interop and live-e2e suites ofnpm run verify— no file of theirs changed.Checkpoint
{ "pr": 1822, "claim": "Dev-4", "issue": 1788, "summary": "A Bot Social read served from the channel now answers only for the Bot the actor owns: flybot/social/share-friends with a body.botId the actor does not own is refused bot/forbidden in authz before the handler registers that Bot's Bot Social identity or syncs its grants. The read itself is unchanged — the endpoint stays in the read set, never falls to the shared rule, and every other plugin read stays open to every signed-in actor. No test ever covered a cross-member botId read, so no existing assertion flips; the new case covers the foreign Bot, the actor's own, and the no-actor install.", "files": [ "core/src/host/authz.js", "core/test/authz.test.js", "docs/subsystems/core-host.md", "changelog.d/1788-social-read-owner.fixed.md", "patches/ds-bot/files/src/host/authz.js", "patches/ds-bot/files/test/authz.test.js" ], "checks": { "ran": "node scripts/ds-bot-patches.mjs check (twice); TZ=UTC ARCH_DOCS_BASE=e4773eaa node core/scripts/lint.mjs; TZ=UTC node --import ./test/harness-resolve.mjs --test test/authz.test.js; TZ=UTC node --import ./test/harness-resolve.mjs --test \"test/*.test.js\"; node core/scripts/build-client.mjs --check", "result": "green", "at": "c1fb9ac6d40f5c8cff333a313c2a30825e090a1c" }, "next": "none — ready for review" }