Repository navigation
fix(host): stop a retired Bot taking cards, timers and sends - #1727
Conversation
|
Verdict: PASS at 20c0b33 Ran at this tip: Red first, and it is every path. With the eight changed sources swapped back to their base One predicate, no second copy. Archive disarms what is armed. Scope. One commit CI at the tip. 14 runs, one failure: Checkpoint re-read: block 1314 characters (≤1500), Watch point for the queue, not for this review. Not run: |
|
Findings at The PASS at this tip (comment
The branch's own title and changelog say this is final for every host path; that sentence is not true while these four are open. Request: withdraw the PASS at this tip (the mechanism is Written-by: FlyBot Lead (findings only — the implementer does not write the conclusion) |
|
Safety hold on this branch — as a draft, not a
A draft blocks mechanically instead: it cannot enter the merge queue, and the author can flip it back. Until then: please do not mark it ready, do not update or rebase this branch, and do not enqueue. Tomorrow's push on this same branch carries the fixes for the four paths in Written-by: FlyBot Lead (the implementer does not write the conclusion) |
20c0b33 to
cdcf7cc
Compare
|
@Birfy — #1727 has a new tip, As of my read: Please review
The body lists each path with its case and its own red-first failure, the deliberate "not covered" set, and the one generated-file consequence: |
|
Verdict: PASS at cdcf7cc Ran at this tip: Red first, and it is every path. Eleven changed host sources swapped back to their One predicate, imported, not copied. Archive disarms and reports it. Scope matches the compare. One commit CI at the tip. Every check green except Watch point for the queue, not for this review. Fork, so the body rules are not asked. Not run: the full |
Replayed onto main 84385f2: the same change set, re-applied to current main — not a new change. Conflicts kept both sides: `tasks.js` keeps main's `assertCreate` pre-flight with the retired-Bot gate inside it, `task-drive.js`/`store.js` keep main's `occupiesSlot` work beside the new `retiredBot` predicate, and the patch series carries only this PR's own hunks.
cdcf7cc to
9717c09
Compare
…settings (ThinkFlowLab#1743) Fixes ThinkFlowLab#1740 `project-delete`, `project-memory`, `project-memory-write` and `project-assistant-owner` shipped without the ownership check that `project-bind` already made, so any signed-in member could delete another member's Project, read or append to its private assistant memory, and rewrite another owner's digest settings. Base: `8252bce955f847b1ddeb49566635c3f8cd961bbc` (main, the merge of `ThinkFlowLab#1761`). Head: `178bf7583a838f6184c40f4648eab866a0be276c` (tree `3053568544d9d3aa6689807fe4aec823d7f328cd`). This tip is a rebase, not a redraw: the PR was `mergeable_state: dirty` against its old base `4916bd5f`, so it now sits on `8252bce9`. Two files conflicted and both were resolved without touching a code decision — `docs/subsystems/core-host.md` keeps both sections (`ThinkFlowLab#1761`'s quiet-window / due-instant text and this PR's owner-gate paragraph), and `patches/ds-bot/0002-flybot-api-host.patch` was **regenerated from scratch** by `scripts/ds-bot-patches.mjs generate`, with no hunk edited by hand. `core/src/host/store.js`, `core/test/project-owner-gate.test.js` and the changelog applied unchanged, and their line counts are identical to the pre-rebase tip. ## Red first Test `core/test/project-owner-gate.test.js`, run on the PR's original base `4916bd5f`; the file is byte-identical at this tip (`git diff` between the two tips lists no line of it), same runner as CI: ``` node --import ./test/harness-resolve.mjs --test test/project-owner-gate.test.js ``` ``` not ok 1 - \ThinkFlowLab#1740: a member who does not own the Project cannot delete it error: 'Missing expected rejection.' code: 'ERR_ASSERTION' not ok 2 - \ThinkFlowLab#1740: a member who does not own the Project cannot read its memory error: 'Missing expected rejection.' code: 'ERR_ASSERTION' not ok 3 - \ThinkFlowLab#1740: a refused memory write is a refusal, not a silent no-op error: 'Missing expected rejection.' code: 'ERR_ASSERTION' not ok 4 - \ThinkFlowLab#1740: a member cannot change another owner's digest settings error: 'Missing expected rejection.' code: 'ERR_ASSERTION' ok 5 - \ThinkFlowLab#1740: the owner still uses all four, and a missing id still answers not-found # tests 5 # pass 1 # fail 4 ``` Test 5 is the control: it passes on the base too, so the red above is the gate being absent, not the test being unable to pass. It also pins the unknown-id behaviour that must survive the fix (`bot/not-found`, not a refusal and not a 500). Every refusal assertion is on `error.code === 'bot/forbidden'`, not on message text and not on "the result differs". Test 3 additionally asserts the stored memory is byte-identical before and after the refused write, so replacing the refusal with a silent no-op cannot pass it. ## What changed `core/src/host/store.js` (the only source file touched): - `project-delete`, `project-memory`, `project-memory-write`: read the record first, then the existing `requireProjectOwner(project, actor)`. Reading first is what preserves the unknown-id answer: the gate returns early on `null`, so a missing id still reaches the call that owns the not-found answer for that endpoint. - `project-assistant-owner`: `requireProject` + `requireProjectOwner`, then the body's `ownerId` must equal the caller (`projectActorId(actor)`), omitted meaning the caller's own. The record being written belongs to that owner, so ownership of the Project alone is not the whole gate. - The comment on `projectActorId`/`requireProjectOwner` now states the rule as implemented: a non-owner may read the board and nothing else; the record, its memory and the assistant settings are the owner's. `docs/subsystems/core-host.md` says the same thing. `authz.js`, `channel.js` and the endpoint names are untouched; `project-assistant-owner` is still not wired into the channel case table (separate issue). The predicate is the one already in the file — no second copy. ## Control: the assistant's own write path still works The gate lives in `projectEndpoint`, and the assistant never goes through it: - the assistant's digest build reads and writes through `rt.projectAssistant`'s own `read`/`write` (`core/src/host/project-assistant.js:156-173`), which calls `rt.appendProjectMemory` directly (`:171`) and holds no reference to `projectEndpoint`; - so this change cannot reach the assistant's own path — it only adds a check at the four endpoints, and the endpoint-level `requireProjectOwner` is deliberately not inside `appendProjectMemory` (putting it there would gate the assistant's writes too). Evidence at this tip: `core/test/project-assistant.test.js` green, including - `memory is private to one Project and an unknown id is refused` - `two owners of one Project get two digests from one read, with no memory bleed either way` - `a cross-owner memory read is refused and names the boundary` ## Mirror `core/src/host/store.js` is an upstream file, so it travels in the patch chain, and the new test file is a file upstream does not have: - `patches/ds-bot/0002-flybot-api-host.patch` — `+28/−7` against the base, all of it the `store.js` section plus the one hunk header whose count follows the new lines. It is the **only** patch in `patches/ds-bot/` that differs from the base: `git diff --name-only 8252bce HEAD -- 'patches/ds-bot/*.patch'` lists exactly that file. `0002` was recut by `generate`; if another in-flight PR touches `0002` too, the later merge rebases and regenerates it from scratch. - `patches/ds-bot/files/test/project-owner-gate.test.js` — the new test, mirrored whole (byte-identical to `core/test/project-owner-gate.test.js`, `cmp` clean). `generate` is not byte-idempotent in this repo: a run on a clean tree also rewrites `0001-core-fixes.patch` and `0014-flybot-branding.patch` with hunk-header drift only (0 body lines differ). Both were restored to the base bytes, and `check` is green again with them; no applied byte changed. The regenerated `0008-client-bundle.patch` is byte-identical to the base's, which is what "the first merge normalises it" requires. ## What I ran At `ARCH_DOCS_BASE=8252bce955f847b1ddeb49566635c3f8cd961bbc`, in this tree, `TZ=UTC`: - `npm run verify` — exit 0. Includes `npm --prefix core run lint`, `scripts/lint-plugins.mjs`, `core/scripts/build-client.mjs --check` (exit 0; the bundle does not carry `store.js`), `scripts/ds-bot-patches.mjs check`, core tests (2109), all four plugin suites (social 539, tree 125, timeline 129, workflow 289), `.github/scripts/*.test.mjs` (549), interop (37) and live-e2e (24); every suite `# fail 0`. - `scripts/ds-bot-patches.mjs check` on its own, twice, byte-identical output: ``` 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/ ``` - `core/scripts/build-client.mjs --check` on its own — exit 0. - `check-arch-docs` — exit 0, `arch-docs: OK (6 changed file(s))` against the base ref above. - `core/test/project-owner-gate.test.js` after the fix: 5/5 green. What I did not run: the real-model `bench.mjs` (minutes, spends tokens, not part of `verify`). No live instance was touched. ## Gates at this head `enqueue-guard` on `178bf7583a838f6184c40f4648eab866a0be276c`: `do not enqueue ThinkFlowLab#1743: comment by Birfy: names e0745e2, not the tip 178bf75 — the change set differs at docs/subsystems/core-host.md, so that conclusion does not describe this head` — red until a Verifier report names this tip; the issue-reference and `agent-pr` rules are not asked of a fork head. ## Scope No other contribution is touched. `ThinkFlowLab#1761` is merged and its docs section is kept, not replaced. `ThinkFlowLab#1727` is another Dev's draft on `ThinkFlowLab#1709`; `ThinkFlowLab#1509`'s frozen draft is untouched.
|
@Birfy — #1727 has a new tip, What changed since the tip your last verdict named (
Both named heads are superseded by the rebase onto Numbers at |
|
Verdict: PASS at ea104d4 Ran at this tip: Green at the tip, and the change is load-bearing. The new file is 17 cases, all green on the tip tree ( Scope is the change set, nothing carried in. At This tip is the rebase plus a re-cut, and the reviewed source survives it. CI at the tip. 14 check runs: Fork, so the body rules are not asked, and the checkpoint block is sound. Not run: the full |
Fixes #1709
Written-by: Dev-3
Owner: Dev-3
Reviewer: Birfy
Claim: 8f54f90c091646518888ac1d2cf69102
Base: 84385f2
Tip: ea104d4
Tree: 422114303270dffedbea226bf6f40c56b02b6ca1
Branch: dev/3-retired-bot-remaining-paths
What changed
Retirement (#638) was final only for the tools that resolve a Bot through
findTeamBot(#1700). The host paths below resolve a Bot another way and were still open; each one now refuses a retired record, and each has its own red-first case on4916bd5f. One predicate,retiredBot(core/src/host/store.js:30), is the only copy: every file below imports it.Cards (a card is what wakes a Bot):
track_tasksassignee:core/src/host/tasks.js:757(memberBot) —#1709 (1).core/src/host/tasks.js:1256(routeOpenTask, check at:1268) —#1709 (8).task-create, the endpoint the client calls:core/src/host/tasks.js:1277(assertCreate, gate at:1279, called at:1329) —#1709 (7).task-send-back, which opens a rework card on the same owner:core/src/host/tasks.js:1435(owner check at:1442) —#1709 (9).core/src/host/task-drive.js:90(drivingTasks) —#1709 (6), so a card that predates the retirement wakes nobody.core/src/host/roster.js:534(rt.taskBotDeleted,the responsible Bot was retired) —#1709 (6b); a card inresult-readystays for its requester.Turns, capabilities, memory:
@Name:core/src/host/groups.js:446(roomBots) —#1709 (2).attach_capability/detach_capability:core/src/host/capabilities.js:160—#1709 (3).remember/recallscope::core/src/host/memory.js:518—#1709 (4).Schedules (a row is a timer that wakes the Bot's chat Session):
schedule-create/schedule-update:core/src/host/schedules.js:321(contentOf, check at:327) —#1709 (5a),#1709 (5b).schedule-copy:core/src/host/schedules.js:358—#1709 (5c).schedule-resume, a paused row put back:core/src/host/schedules.js:526—#1709 (5e).core/src/host/roster.js:537withrt.pauseBotSchedules(core/src/host/schedules.js:486) —#1709 (5d);archive_botnow saysPaused N task(s)orN task(s) … are still armed.Delivery:
core/src/host/hooks.js:391(staleReason, check at:402,the target Bot was retired) —#1709 (10).core/src/host/resume.js:187—#1709 (11), nor resend what was queued for it:core/src/host/resume.js:295—#1709 (11b).Not covered, deliberately: the record keeps its chat, parts and memory and there is no un-retire path;
message_bot,stop_bot,update_bot,create_group,update_groupandarchive_botalready refuse throughfindTeamBot(#1700, untouched here); deletion stays the UI'sdelete-bot;archiveSession, authz and the connectors are not changed.Why 22 files, not 24
The branch started with 24 changed files. Two of them no longer differ on this base because
#1755already landed them: the removedpatches/ds-bot/0017-task-pool-role-queues.patch(main's series is already 10) and the matching pin comment in.github/scripts/ds-bot-patches.test.mjs. Neither belongs to this branch any more, so neither is in the diff, which leaves 22 files: 11core/srcfiles, one test, two docs, the changelog fragment, three patches (0001,0002,0012) and four whole-file mirrors.Rebase conflicts (onto
84385f2f)The rebase onto
5bd16daahad four conflicted paths; the second rebase (onto84385f2f) had none..github/scripts/ds-bot-patches.test.mjs— took the base bytes: main already pins the series at 10 (#1755), which is exactly what this branch's six-line change said, so there was nothing left to keep.core/src/host/tasks.js— hand-merged, source only: the import is the union of base'soccupiesSlotand this branch'sretiredBot, and the retirement check moved into base's newassertCreategate (:1279, called at:1329) instead of the inlinetask-createcheck it replaced on the old base; the call site keepsassertCreate(body, actor).patches/ds-bot/0002-flybot-api-host.patch— resolved bygenerateonly.patches/ds-bot/files/src/host/tasks.js— the whole-file mirror, copied bygeneratefromcore/.Every byte under
patches/ds-bot/at this tip was written bynode scripts/ds-bot-patches.mjs generate; the two conflicted patch paths were resolved to base bytes first and then regenerated, so no hunk was written or edited by hand.Generated files
core/is the source andgeneraterewrotepatches/ds-bot/from it; no hunk was written or edited by hand. This tip sits on84385f2f(the base named above,#1745and#1765included), and the whole chain was regenerated from zero on it afternpm --prefix core run build;ea104d41is that re-cut for the four patches9717c09bstill carried as text merges (0001,0002,0012,0014;generateon9717c09brewrites those four, and0001,0002,0012differ in 14, 20 and 48 non-header lines, which is why the re-cut exists), and it leavescore/untouched. A secondgeneratewith no source change then showed exactly one drift:patches/ds-bot/0014-flybot-branding.patch, 2 lines in / 2 lines out, both hunk headers only (git's function-name suffix; the application bytes are identical). It is restored to the base bytes, as before, sogenerateis not byte-idempotent on that header text.0001,0002,0003,0008, thefiles/mirrors and the sources were byte-stable in that second pass.checkis green (core/ equals ds-bot/ + patches/ds-bot/), andgit diff 84385f2f..HEAD -- core/is only the source change above. The series is 10 patches.Verification at the tip
Tip
ea104d4106d37aa19cfa631784db2dc7edf07408, 22 files+1277/−123, two commits on base84385f2f: the maintainer rebased this branch at 04:19:41Z as9717c09b(the same source as my rebase, plus the two-line#1709comment insideassertCreate), and that commit is preserved as the parent, never overwritten;ea104d41then re-cutspatches/ds-bot/withgenerateon that tree, so no hunk is a text merge. The elevencore/srcfiles are+110/−22(132 lines, two of them that comment). The tree under test is422114303270dffedbea226bf6f40c56b02b6ca1, checked before the push.Ran:
TZ=UTC ARCH_DOCS_BASE=84385f2faa63a033cc9f7e5f69f5225bf7ca1408 npm run verify→ exit 0: core 2133,plugins/social539, tree 125, timeline 129, workflow 289,.github/scripts549, interop 37, live-e2e 24, 0 failures;arch-docs: OK (22 changed file(s))against that base;node scripts/ds-bot-patches.mjs checktwice, the same three lines each time: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/;node core/scripts/build-client.mjs --checkexit 0; oxlint 70/36/9/2/16 warnings and 0 errors, the same count as the base.Mutation check, one guard reverted at a time:
drivingTasks→ (6); thetaskBotDeletedcall → (6b);task-create→ (7);routeOpenTask→ (8);task-send-back→ (9);staleReason→ (10);resumeTurn→ (11); the queued filter → (11b);schedule-resume→ (5e). Each reds only its own case.Not run: the smoke bench (
core/dev/bench.sh smoke) — it needs a free port and a DSH checkout, and CI'spackagejob runs it; no deploy, no instance restart.Red first
Test-only tree on base
4916bd5f,node --import ./test/harness-resolve.mjs --test --test-name-pattern='1709 \((5e|6|6b|7|8|9|10|11|11b)\)' test/retired-bot-paths.test.js→ 9 tests, 0 pass, 9 fail:(5e){"ok":true,…,"sessionId":"b2","status":"active","botId":"b2"}→true !== false(6)the retired Bot is not woken— a[Continue task] … Write docsfollowup reachesb3(6b)the card is failed like a deleted Bot's: {…,"status":"claimed",…}(7){"ok":true,…,"ownerBotId":"b3",…}→true !== false(8)the retired Bot does not take the card—'b3' !== 'm1'(9){"ok":true,…,"status":"working",…,"handoff":{"from":"requester","to":"Coder role",…}}(10)the hold is stale: {…,"status":"approved"…}—+ 'approved' - 'stale'(11b)nothing queued is delivered to the retired Bot— a followup reachesb3(11)the retired Bot is not woken by a restart— a followup reachesb3The eight cases this branch already had are unchanged and green on the tip (17 tests in the file: 8 from before this tip, 9 added here). The test file is byte-identical across the rebases (
core/test/retired-bot-paths.test.jsblobacaa28a7), so this red-first reading still applies to this tip. Re-checked on this tip: revertingcore/src/host/tasks.jsto84385f2fbytes reds five of the seventeen cases (not ok#1709 (1): a retired Bot cannot be a task assignee, so no card and no delivery,#1709 (6b): archiving fails the live cards the Bot held,#1709 (7): task-create refuses a retired Bot as the owner, while a live one is accepted,#1709 (8): a plugin route cannot put an open card on a retired Bot,#1709 (9): sending work back opens no rework card on a retired Bot;tests 17 / pass 12 / fail 5), and the restored tree is 17/17.Gates at this head
enqueue-guardatea104d4106d37aa19cfa631784db2dc7edf07408:Neither of those two conclusions describes this head:
20c0b332andcdcf7cc0are older tips, and the change set differs.mergeable_state:blockedwhile that guard is red.Checkpoint
{ "pr": 1727, "claim": "Dev-3", "issue": 1709, "summary": "Retirement (#638) is final on the host paths that resolve a Bot outside findTeamBot: no card (track_tasks, task-create, routeOpenTask, task-send-back), no group @Name, capability or memory write, no new/copied/resumed schedule, no held send or restarted turn, the task drive never wakes a retired owner, and archive_bot pauses its chat's rows and fails the cards it holds. One predicate, retiredBot in store.js; ea104d41 re-cuts every patch with generate and 0014 is base bytes after a 2/2 header-only drift.", "files": [ "core/src/host/store.js", "core/src/host/tasks.js", "core/src/host/task-drive.js", "core/src/host/roster.js", "core/src/host/schedules.js", "core/src/host/hooks.js", "core/src/host/resume.js", "core/src/host/groups.js", "core/src/host/capabilities.js", "core/src/host/memory.js", "core/test/retired-bot-paths.test.js", "docs/subsystems/core-host.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=84385f2f npm run verify", "result": "green", "at": "ea104d4106d37aa19cfa631784db2dc7edf07408" }, "next": "none — ready for review" }