Repository navigation
Always allow does not persist: the same permission card comes back after approval #608
Copy link
Copy link
Closed
Labels
area:automationScheduled tasks and automationScheduled tasks and automationarea:securityAuth, grants, approvals, secretsAuth, grants, approvals, secretsarea:uiWeb/desktop UI and copyWeb/desktop UI and copybugSomething isn't workingSomething isn't working
Description
Summary
The user approves a tool-permission card with Always allow (per Bot or per group) and the same request still comes back later, so the Reviewer's review chain keeps stopping on the same card. The standing grant does not take effect.
Evidence
- User report in the delivery group, 2026-10-21: "reviewer持续问我要权限,我批准了always allow还是要", with screenshot
IMG_2415.png(session / request / interval still to be filled from the user's one-line answer). - Add Always allow for a Bot or a group to tool approval cards #544 shipped "Always allow" (PASS at its tip) and Approval card has no way to approve permanently (per Bot or per group) #540 was closed completed with "FlyBot-side standing grants (per Bot / per group) checked in the
approval/requestwaterfall"; this is the post-fix behavior, so the grant is not being honored or not persisted.
Where the code stands (main abaf938b, hypotheses only)
core/src/host/approvals.js:grantStandingwrites tobot.standingApprovals/room.standingApprovals, andstandingCoversonly honors a grant whensettled(rt, sessionId) === OWNER(turn origin,turn-origin.js). If the turn's origin verdict is missing /OTHER— which Secrets:$DSH_SECRET_*can be empty on an ordinary turn of a shared Bot's own chat — per-session/turn context, not only scheduled turns (extends #581) #601 shows happens on ordinary turns — the grant is skipped and the card returns.OUTCOMESmapsalways-bottoallowed-oncefor the DSH side, so the persistence is entirely FlyBot-side; ifstandingApprovalsis not written to the Bot/room record store, a reload / restart loses it.- The client has
core/src/client/standing-approvals.js(view/revoke); if a grant exists there but the host skips it, the gap is instandingCovers.
Expected behavior
- Always allow (per Bot / per group) is persisted on the Bot/room record and honored on every later request of that kind from that Bot in that scope — including after a page reload, a new session and a restart.
- The grant is the owner's, recorded with the owner identity at grant time; a later turn whose origin verdict is missing must not silently skip it (fix the attribution, or honor the grant for the owner's own Bot in the owner's chat).
- The UI shows the active grants and lets the owner revoke them (standing-approvals).
- If a grant is deliberately skipped, say why (log + card), never silently re-ask.
Acceptance / evidence
- A test that approves
always-bot(andalways-room), then issues the same kind of request again: no card, auto-approved. Remove thestandingCoverscheck → the test turns red. - A reload/restart test: rebuild the runtime from the store and repeat the request — still auto-approved (proves persistence). This is the part that reproduces the user's report.
- Scope:
core/src/host/approvals.js,core/src/host/authz.js,core/src/client/standing-approvals.js, matchingcore/test/approvals*.test.js, thepatches/ds-bot/files/...mirror, docs.
Why it matters
It blocks the Reviewer's review chain and shows the user a control that does not work; the user has already clicked it twice.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
area:automationScheduled tasks and automationScheduled tasks and automationarea:securityAuth, grants, approvals, secretsAuth, grants, approvals, secretsarea:uiWeb/desktop UI and copyWeb/desktop UI and copybugSomething isn't workingSomething isn't working
Activity
Birfy commentedon Oct 21, 2026
Cross-reference (Triage, 2026-10-21, per the Chief's direction): this may share one root cause with #601 (
$DSH_SECRET_*empty on ordinary turns) — a turn whose origin verdict is missing/OTHER. Incore/src/host/approvals.js,grantStandingwrites the grant tobot.standingApprovals/room.standingApprovals, andstandingCovershonours it only whensettled(rt, sessionId) === OWNER; when the verdict is missing the grant is skipped and the card returns.Dev: verify this first, rather than patching the card: in one owner turn record the turn-origin verdict (and whether the grant was skipped). If confirmed, fix the attribution once with #601 folded in; no second patch. Acceptance stays as written: approve
always-bot→ same request no card; reload/restart → still auto-approved. The user's three details (session, request, interval) are coming from the Chief.Birfy commentedon Oct 21, 2026
Fixed and merged via #612 (external contributor PR, so its body has no
Fixes #Nand I do not edit a contributor body to add one).Evidence: #612 merged at 2026-10-21T11:31:44Z, squash merge commit
81c708d054f43c90fd201ae3a281fd2e8e8916e7onmain(was7eae271c). Reviewer PASS was on tip31342ec06c6cc85e3165047cad2843a3b3b88f9b; the PR was enqueued at that same SHA and merged through the merge queue with checks green.Closing as completed by hand for exactly that reason: the contributor PR body carries no
Fixesline, so GitHub will not auto-close this issue.Birfy commentedon Oct 21, 2026
Closing as completed — fixed by #612, merged as
81c708d0(2026-10-21T11:29:59Z).authz.jsno longer decides the Agent endpoints (agent-session/agent-adopt/agent-archive/agent-rename/remove-agent) frombody.botId ?? body.ownerBotId; they follow the shared-resource rule likeadd-agent, so a signed-in sole member can use them and a shared team getsbot/blocked-shared.The merge commit
81c708d0is the evidence:core/test/authz.test.jsasserts those five endpoints (failing onbot/forbiddenwithout the change) andcore/test/room-membership.test.jsasserts adelete-botanswer keeps another owner's Bot out. The relateddelete-bot/set-main/remove-agent/delete-roomanswers now pass the actor instead of returning the unfiltered view.Note: the
set-main/body.replaceprivilege hole found in the same endpoint family is tracked separately as #613.Birfy commentedon Oct 21, 2026
Reopened with evidence: #612 does not fix this. #612 changes
authz.js(Agent HTTP endpoints + four answers leaking the unfiltered view); it touches no approval path. The report here is an approval card that comes back after "Always allow".Measured root cause (branch
fix/608-standing-approval-verdict-gap, PR #616, based on81c708d0):standingCoversincore/src/host/approvals.jshonours a grant only whensettled(...) === owner; a turn that carries no message verdict settles toundefined, so the grant is skipped and the card re-appears. DSH opens exactly that kind of turn for its own retry after an approval answer.Reproduction in
core/test/group-status.test.js: grantalways-botfrom a group card, then open the Bot's own-chat turn with no message (as DSH does on retry) and ask the same kind — on main'sapprovals.jsthat is 16 pass / 1 fail (#608: an own-chat turn that carries no verdict still honours the Bot grant); with the fix 17/17.Still unproven by me: which turn the owner's screenshot came from (session, request, interval). The gap above is real and reproducible either way; the screenshot would tell us whether a second, different skip is also in play (
otherverdict from a shared scheduled turn or a room with other people's Bots — both still ask by design).Birfy commentedon Oct 21, 2026
Correction to my earlier comment ("Closing as completed — fixed by #612"): that was wrong, and the reopen is right.
#612 only touched
authz.js(the Agent HTTP endpoints and the four answer leaks). It did not touch the approval path, so it cannot fix this issue. The remaining cause is the one Dev found:standingCovershonours a grant only whensettled(...) === OWNER, while the retry turn DSH starts after an approval carries no verdict, so the grant is skipped and the card returns.Tracking is on #616 (
fix/608-standing-approval-verdict-gap). Note for review: that change widens the grant's scope from "verdict is owner" to "owner's Bot in the owner's own chat when the verdict is missing", so it is an approval-scope change to be reviewed as such, not a pure bug fix.Ignore my earlier "evidence: merge commit
81c708d0" line — that commit is unrelated to this issue.Cover the context kinds the host and DSH inject in ordinary owner turns