Repository navigation
fix: honour a standing approval on a turn with no verdict (#608) - #616
Conversation
DSH opens its own retry turn after the owner answers an approval. That turn carries no message verdict, so turn-origin.js settles nothing, and standingCovers skipped the owner's "Always allow" grant and showed the same card again. A turn with no verdict in the Bot's own chat now uses that Bot's grant; a friend's message or another member's line still asks, and a deliberate skip logs one line instead of staying silent.
Birfy
left a comment
There was a problem hiding this comment.
Verdict: PASS at 2ca04f6 — 仅记录、等用户确认;权限面改动,我不入队、不合并。
Tip SHA: 2ca04f636551b3fafdb8801368e923b11bdedb83
CI at tip: all 8 checks success (tests-ok, package, plugins social/tree/timeline,
ubuntu-latest Node 24, reviewed tip, risk tier)
Tier (from the diff): T3 — core/src/host/approvals.js is a T3 path in
.github/agent/risk-rules.json (the controller is report-only, so this is read
from the rules, not from a label)
Both directions reproduced locally at this tip, not inferred from CI:
$ cd core && TZ=UTC DSH_HARNESS=… node --import ./test/harness-resolve.mjs --test test/group-status.test.js
# tests 17 / pass 17 / fail 0
$ # approvals.js restored from base 81c708d0, new tests kept
not ok 13 - #608: an own-chat turn that carries no verdict still honours the Bot grant
# tests 17 / pass 16 / fail 1
Exactly the one new test goes red, and only it — gate ③ holds. The restart test passing on both sides is honest: the defect is the verdict gap, not the store.
I checked the new predicate against the store instead of assuming:
ownChat()readsrt.chatOwners, whichstore.jsfills only from a Bot's own-chat session (chatOwners.set(chatOf(bot.id), bot.id)); group Sessions go togroupOwnersand earlier segments topastOwners. So the widened clause cannot fire in a room, a group Session or an archived segment — those still ask.settled()still returns'other'first for a friend's line, another member's line and a shared Bot's scheduled turn, so the grant stays off there. The test's fourth case (a remote friend's message in the own chat) pins that.- The skip log is bounded (clears past 200) and writes one line per session+kind, so a grant that cannot be used is no longer silent.
One note for the owner, not a blocker: turn-origin.js documents "The caller treats none as 'other' (fail closed)", and originOfSource() returns undefined both for DSH's own notices and for any source kind it does not recognise (default:). After this change, a verdict-less turn in the owner's own chat applies the grant — correct for the retry turn DSH opens after an answer, but it also means an unrecognised future source kind arriving in a Bot's own chat would be covered by a standing approval instead of failing closed. Suggested follow-up, separate from this PR: either update that turn-origin.js header, or narrow the clause to turns with no message at all.
Ready to merge on the owner's word; not queued by me.
|
Verdict: PASS at 2ca04f6 — owner approved 2026-10-21, merging. The review above stands unchanged (tip |
|
Owner decision (2026-10-21): merge. Relay: Chief. This is the only status note on this PR from here on. PR checks: Follow-up (same round): fail-open on unknown source kind in Enqueued by: Chief (owner proxy — Reviewer session unavailable). |
|
Post-merge record (reviewer)
|
…hinkFlowLab#620) (ThinkFlowLab#626) ## Summary core/src/host/turn-origin.js promises a fail-closed default, but originOfSource() returned undefined ("nobody's request") both for the documented no-request cases and for any source kind it did not recognise. After ThinkFlowLab#616, "no verdict in the Bot's own chat" means the owner's standing approval applies, so a future source kind would silently inherit a grant instead of failing closed. This is option 3 of ThinkFlowLab#620: an unrecognised kind is other. The review found the other half of the same promise twice: the no-request list has to be complete, or the fix turns real owner turns into other. The first INC named four injected context kinds; the second named seven more that DSH delivers into a turn that is already running. Reproduce - originOfSource({ kind: 'some-future-kind' }) returned undefined on main (turn-origin: an unrecognised source kind in a Bot's own chat counts as the owner (fail-open left open by ThinkFlowLab#616) ThinkFlowLab#620). - The INC case: the owner's turn calls subagent; when the child settles, DSH steers the running turn with a user/message carrying kind: 'subagent-settled', form: 'notice' (@deepseek-ai/dsh-subagent). At 00fa185 that message is other, and a turn keeps the strictest verdict it took in, so standingCovers stops applying and connectorTurnChanged masks the connector tools for the rest of the turn. compact-checkpoint (a compaction's replacement summary), model-selection, working-directory, ptc-mode, skill-invocation and cordis-host-runner arrive the same way. A Bot that uses subagent once hits it. What changed - core/src/host/turn-origin.js — one list, NOTICE_KINDS, naming every kind that carries nobody's request, with the package that produces it: DSH's notices (tool, system-prompt, plan-mode, user-approval); the context this host and DSH append to an owner turn (ds-bot-time-context, time-context, session-reference via constants.js REFERENCE_KIND, agent-instructions, model-selection, working-directory, ptc-mode, cordis-host-runner); the one-off accounts (subagent-settled, compact-checkpoint, skill-invocation). The comment says why form cannot stand in for the list either way: three of those kinds declare no form at all, and webhook declares form: 'notice' while being an outside delivery. webhook, agent-message (form: 'relay') and team-message therefore stay other. A goal round continues a goal the Session already has, so it reads like a due task (SCHEDULED, which settled turns into other on a shared Bot) instead of failing closed. A kind this version does not know still fails closed. - core/test/turn-origin.test.js — the verdict table covers every listed kind, webhook / agent-message / team-message as other, goal as scheduled, and a test that fails when the pinned DSH version moves (core/dev/harness/package.json), so the map is re-read on a bump instead of a new kind being discovered months later as a lost tool. - core/test/group-status.test.js — the shape that actually happens: an always-bot grant, an owner turn, tool/call subagent, then the settled notice as a user/message; the next ask still answers allowed-once. The four per-kind cases from the first round stay. - docs/subsystems/core-host.md — the turn-origin paragraph states the complete list, the form caveat and the goal rule. - patches/ds-bot/files/ — byte-identical mirrors of the two whole-file entries (turn-origin.js, turn-origin.test.js). This container has no git, so generate could not run: the group-status.test.js section of 0001-core-fixes.patch was regenerated from ds-bot/ and core/ with diff -u and then verified by replay — applying those hunks to ds-bot/test/group-status.test.js reproduces core/test/group-status.test.js byte for byte. - changelog.d/620-turn-origin-fail-closed.fixed.md. What I ran At 5ed6851 — this commit; the tree is identical to `03060560`, which is where the run below was made (TZ=UTC, DSH_HARNESS pointing at this container's DSH checkout): node --import ./test/harness-resolve.mjs --test test/turn-origin.test.js test/group-status.test.js # tests 27 # pass 27 # fail 0 (turn-origin 4, group-status 23) ## Who wrote this The three rounds of rework are **Dev's**; the commit that carries them (`5ed6851f`) was made by **Chief as owner proxy**, because Dev's session had no injected credential (`DSH_SECRET_*` absent, connector refused — ThinkFlowLab#601). Nothing in the diff was edited after Dev produced it; `out/626-tip-diff.diff` was applied with `git apply` and the six files committed unchanged.
…#670) Fixes ThinkFlowLab#609 Tip SHA: 4c8d98c ## Reproduce The rule text says `risk:T*` is set by the controller from the diff, never by hand, and the mechanism does not hold up: `github-actions[bot]` labelled ThinkFlowLab#574 at 07:28:43Z, while ThinkFlowLab#606, ThinkFlowLab#610, ThinkFlowLab#611, ThinkFlowLab#612, ThinkFlowLab#616, ThinkFlowLab#617, ThinkFlowLab#618 and ThinkFlowLab#619 carry no `risk:T*` at all even though `risk tier (report only)` succeeded on every one of them. The owner decision on the issue is ②: keep the workflow read-only, never label — and make the computed tier auditable instead of remembered. At this tip, `node .github/scripts/risk-gate.mjs --pr <n>` ends its report with risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3 so a review can quote the tier the rules produced instead of restating a number. ## What changed - `.github/scripts/risk-gate.mjs`: `judge()` records every changed path (and a rename's old path) with the tier `tierOf()` gave it, and `summaryLine()` renders the final line of the report from that same result — highest tier first, paths capped at 10 with a `+N more` tail. `render()` ends the job summary with it, after the prose, so the two can never disagree. - `.github/scripts/risk-gate.test.mjs`: four cases — the line's exact shape for a T3 PR, the ordering and the cap, the no-changed-files case, and "the line is the last line of the report". - `AGENTS.md` gains a bullet under the label section: quote the summary line, the workflow never writes a label, the controller copies it on its poll, no review transcribes a tier. - `.github/labels.json`: every `risk:T*` description keeps `advisory` and `the merge gate recomputes it` and now also names the controller as the writer (all ≤ 100 characters). - `docs/controller.md`: the tier stays auditable without the label — quote the summary line. - `.github/workflows/risk-tier.yml`: header comment only — no permission added, no step changed. It keeps `contents: read` and `pull-requests: read` and still never labels, comments or merges. ## What I ran - `TZ=UTC node --test .github/scripts/*.test.mjs` at this tip (with `git` on `PATH`) → **144/144**. - `node --test .github/scripts/risk-gate.test.mjs` → **18/18**, including the four new cases. - `node .github/scripts/risk-gate.mjs` behaviour checked by hand through the harness result: `judge([AGENTS.md, plugins/social/src/social.js])` → `risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3`; no files → `risk-tier: T0; basis: none (no changed files)`. - `changelog.mjs check` not run: a change users cannot see needs no fragment (changelog.d/README). ## Red without the source change - Removing the `basis.push([name, t])` line from `judge()` (the tests untouched) → `risk-gate.test.mjs` **16/18, 2 red**, exactly `summaryLine: one machine-readable line…` and `summaryLine: highest tier first…`. - Removing `summaryLine(result)` from the end of `render()` instead → **17/18, 1 red**, exactly `render: the machine-readable line is the last line of the report`. Restored → 18/18. ## Not run - The workflow itself (it needs Actions): the change to `risk-tier.yml` is a comment, and the existing `the workflow stays report only` test still passes with it. - `npm run verify` end to end: no `core/node_modules` in this sandbox (the same on `main`). - The `risk-tier` job on this PR runs the base copy of the script, so its summary will only show the new line after merge — that is the workflow's normal base-copy behaviour, not a gap here. ## Risk T3 by the repository's own rules (`.github/**`, `AGENTS.md`): a reporting script plus text. It adds no permission, no network call, no label, comment or merge, and cannot block anything — the job is still `continue-on-error: true` and not a required check. The only behaviour it changes is the last line of that job's summary. The one thing a reader must not conclude is that the label now arrives on time: it does not, and the docs keep saying why (ThinkFlowLab#629).
The owner pressed Always allow for this Bot and the same permission card came back. Root cause:
standingCovers(core/src/host/approvals.js) only honoured a grant whensettled(rt, sessionId) === OWNER; a turn that carries no message verdict settles toundefined, so the grant was skipped and the card re-appeared. DSH opens exactly such a turn for its own retry after an approval answer.复现
In the
group-statusharness: grantalways-botfrom a group card, then open the Bot's own-chat turn the way DSH retries after an answer — no message, or only a host notice — and ask for the same kind:改了谁
core/src/host/approvals.js:standingCoversnow also answers a turn with no verdict in the Bot's own chat (rt.chatOwners.get(sessionId) === botId) — nobody's request is in that turn. A friend's message, another member's line or a room with other people in it still asks (verdictother). A grant a turn cannot use is no longer skipped silently: the first such ask per session and kind logsthe standing approval for <kind> was not used: the turn is …(bounded set, 200).core/test/group-status.test.js: the red test above, plus a restart test that reopens the same home with a fresh runtime and asserts the grant still answers (persistence guard; passes onmaintoo — the defect is the verdict gap, not the store).patches/ds-bot/0001-core-fixes.patch: regenerated withnode scripts/ds-bot-patches.mjs generate.docs/subsystems/core-host.md: the grant rule and the skip line.changelog.d/608-always-allow-own-chat-turn.fixed.md.安全面:权限 grant 的适用范围(请按这段核)
这个 PR 动的是审批 grant 的适用范围,不是授权规则本身。逐条边界:
other:好友、另一个成员、另一个 owner 的 Bot)settled映射成other)authz.js的 owner 规则判定条件写死在
standingCovers里:只在rt.chatOwners.get(sessionId) === botId(这个私聊属于这个 Bot 自己)且该 turn 无 verdict 时才算覆盖;任何带 verdict 的 turn 走原来的分支,因此other一律照旧要问。测试#608: an own-chat turn that carries no verdict still honours the Bot grant覆盖前者;同文件里既有的other用例覆盖后者,未改动、仍绿。给 Chief 的说明:这属于安全面(grant 扩大),已由实现 Lead 转报。范围仅限上表第二行;不新增配置项,不改存储格式,不触碰 secret/sharing/authz。
跑了什么
core/test/group-status.test.js17/17,member-status9/9,inbox19/19,authz18/18,turn-origin3/3,turn-reasoning4/4,turn-tail9/9,rounds38/38 (each withDSH_HARNESS=/opt/flybot --import ./core/test/harness-resolve.mjs).node scripts/ds-bot-patches.mjs check→ ok (pin v0.1.5, 10 patches,core/equalsds-bot/+ patches).node .github/scripts/changelog.mjs check→ 20 fragments well formed.core/scripts/lint.mjs→ oxlint 0 errors / 43 warnings (same warnings asmain);check-arch-docs→ OK (5 changed files).撤掉源码改动后变红
Restored:
# tests 17 / pass 17 / fail 0.没跑什么
core 全量一次性(本容器 pid 上限 256)、三个插件套件、smoke、真机、完整
npm run verify;CI 在 tip 上跑tests-ok。风险
Permission surface: the owner's standing approval now also answers a verdict-less turn in their own Bot's chat; every turn carrying someone else's request still asks. (
risk:T*is computed by the report-only workflow; not hand-set.)Fixes #608