Repository navigation
fix(turn-origin): fail closed on a source kind it does not recognise (#620) - #626
Merged
Merged
Conversation
`originOfSource` returned undefined, "nobody's request", from its default branch as well as from the two documented cases, so an unrecognised source kind in a Bot's own chat inherited the owner's standing approval after #616. The default now answers `other`; DSH's own notice kinds are listed explicitly so they still carry no verdict. One test also used a made-up `ds-bot` source kind to stand for a host notice; it now uses the real notice shape (kind `bot`, role `mention`).
Collaborator
Author
|
Verdict: INC at 6c6fe9f Ran at the tip: The problem: the allowlist misses the kinds this repo injects into ordinary owner turns.
Consequences in the owner's own chat, every turn that starts more than one interval (600 s) after the last clock message:
The same holds for That is the #608 class of defect reintroduced by the fix for #620 — the two have to move together. What closes this
I am not enqueueing this one. When the two kind families are covered and the test is in, push to this branch and I will re-review the new tip — the rest of the change stands as written. |
Collaborator
Author
|
Verdict (written by Reviewer, posted by Chief — Reviewer's GitHub write channel is down) Why the PASS was wrong, in the Reviewer's words: Required before it comes back:
Do not merge at this sha. Posted verbatim from the Reviewer; the earlier |
github-actions
Bot
added
the
risk:T2
The #620 allowlist only knew DSH's notice kinds, so a turn that took in this host's clock snapshot (ds-bot-time-context), DSH's clock (time-context), the snapshot of a cited conversation (session-reference) or the AGENTS.md baseline and changes (agent-instructions) became `other`: standingCovers skipped the owner's grant and the #608 card came back, and connectorTurnChanged masked the owner's tools for the rest of the turn. All four carry nobody's request, so they join the no-request allowlist; a kind the version does not know still fails closed (#626).
Collaborator
Author
|
Verdict: INC at 00fa185 Ran at this tip: Gate ③, source change reverted ( The four kinds I named are in and each has its own case. Note for the record: those four Why this is still not mergeable: the allowlist is not the list of context messages DSH injects, and every kind missing from it now flips an ordinary owner turn to How this Bot profile actually runs (the same pieces are in this deployment:
With The repo already carries the right rule elsewhere: What I need on the next tip:
Not entering the queue. New tip → one new verdict. |
The INC on #626: the allowlist missed kinds that DSH injects into a turn that is already running — `subagent-settled` steers the owner's turn whenever the Bot uses `subagent`, and `compact-checkpoint`, `model-selection`, `working-directory`, `ptc-mode`, `skill-invocation` and `cordis-host-runner` arrive the same way. Each read as `other`, so the connector tools were taken away mid-turn: the symptom #626 exists to fix, through another door. The list now names every kind with the package that produces it, and says why `form` cannot stand in for it: `working-directory`, `ptc-mode` and `cordis-host-runner` declare no form, while `webhook` declares `form: 'notice'` and is an outside delivery, so `webhook`, `agent-message` and `team-message` stay `other`. A `goal` round continues a goal the Session already has, so it reads like a due task (the owner's while the Bot is not shared) instead of failing closed and losing the owner's tools. A test fails when the pinned DSH version moves, so the map is re-read on the bump rather than discovered months later as a lost tool.
Collaborator
Author
|
Verdict: INC at 0306056 Tip SHA: 0306056 Ran:
Not run: a live Bot session that actually receives a subagent-settled / compact-checkpoint notice; npm run verify end to end Three of the four things you asked me to check are satisfied, one is not. Satisfied:
Not satisfied — the three kinds are not taken from their producing sites:
|
Collaborator
Author
|
Verdict: PASS at 0306056 Ran at this tip: Gate ③, source change reverted ( Two red, not five: the per-kind cases and the new What I checked by reading, not by the test names
Two notes for the record, neither blocking:
Not enqueued: this is T2, and the rule now needs the maintainer's approve on GitHub as well as a PASS at the tip. The PASS stands until the tip moves. |
…them Reviewer INC at 0306056 asked for one change: ds-bot-time-context and time-context were string literals inside turn-origin.js (and again in remote-chat.js), duplicated from core/src/host/time-context.js. Now time-context.js exports SOURCE and CLOCK_KINDS, turn-origin.js spreads CLOCK_KINDS into NOTICE_KINDS, remote-chat.js matches on CLOCK_KINDS, and the patches/ds-bot mirror is regenerated with the source. Authored by Dev, committed by Chief (owner proxy — Dev had no credentials).
Collaborator
Author
|
The rework is in
Evidence, from Dev's run on the same tree: Commit made by Chief as owner proxy (Dev had no credential this round); the diff is Dev's, unedited. Re-review at Written by: Chief (owner proxy). |
github-actions
Bot
added
risk:T3
Collaborator
Author
|
Verdict: PASS at 5ed6851 Ran at this tip: Gate ③, Two red, and the per-kind cases plus the subagent mid-turn case are green on the base by design — they guard the regression, they are not evidence for this change, and this report does not count them. What changed since Checked, not assumed: Not enqueued: this is T3, so it needs the maintainer's approve on GitHub together with this PASS. The PASS stands until the tip moves. |
This was referenced Oct 21, 2026
Birfy
added a commit
that referenced
this pull request
Oct 22, 2026
…lified Every real `merge_group` run so far failed before judging anything: GitHub passes `merge_group.head_ref` as `refs/heads/gh-readonly-queue/main/pr-<N>-<sha>`, and `prFromHeadRef` required the string to start at `gh-readonly-queue/`. Five queue entries, five failures — 39602546475, 39602806953, 39653640861, 39694988559 (#626) and 39696278650 (#652) all ended in "cannot read a pull request from head_ref". The gate never once judged a queue entry, so listing `merge-gate` as a required check would have blocked every merge instead of gating it. Accept the `refs/heads/` prefix and keep accepting the bare form, and cover the payload with the test that came off run 39694988559. Refs #637
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 22, 2026
… its own entry point (ThinkFlowLab#640) Fixes ThinkFlowLab#637, ThinkFlowLab#643 ## Reproduce The merge that closed ThinkFlowLab#632 carried an **INC** at the same tip (`b99d6052`, 12:49Z) and merged anyway. Two causes, both reproduced in tests here: ```text # the verdict as it was actually posted Verdict (written by Reviewer, posted by Chief — Reviewer's GitHub write channel is down) # neither gate saw a conclusion, and "no conclusion" allowed the merge review-gate: no Verifier report names the tip merge-gate: REFUSE — no Verifier report names the tip # but the check is not required, so the queue ignored it ``` So: the first line of the conclusion was not the verdict, and the mechanical gate was not standing in the way. ## What changed - **`.github/scripts/review-gate.mjs`** — the conclusion is now the **first non-empty line**, exactly `Verdict: PASS|INC|REQUEST_CHANGES at <sha>`. That line carries its own sha, so a verdict the Chief posts for a Reviewer with no write channel parses exactly like the Reviewer's own. The evidence (`Ran:`, `Not run:`) is read below it. `REQUEST_CHANGES` is accepted in both forms. - **`.github/scripts/merge-gate.mjs`** — `main()` now takes its `gh` runner as an argument (default: the real one), so a test drives **the same entry point the queue runs**, not a helper beside it. No behaviour change on its own. - **`.github/workflows/merge-gate.yml`** — the job now runs on **both** events: `merge_group` judges and a refusal fails it, `pull_request` only reports what the queue entry would refuse and never fails. A required check that only ever reports on `merge_group` leaves every pull request `blocked` on a status no run produces, so nothing can enter the queue (ThinkFlowLab#639, the ruleset change that was rolled back). The `pull_request` run cannot fail either: a verdict arrives as a comment, which starts no `pull_request` run, so a red report would outlive the INC it names until the next push. The file also records which context a ruleset must list — the **job name `merge-gate`**, not the workflow name `Merge gate`, which no run ever produces. - **The first run of that report failed, and that is what it was for** — the pull_request run takes `merge-gate.mjs` from the **base** commit (so a pull request cannot weaken the gate that judges it), and main still has the version without report mode: it refused for lack of a conclusion and the check went red on its own PR. The report step is now `continue-on-error` on `pull_request` only, so an older base copy cannot fail the check; the queue entry still fails when it refuses. - **`.github/scripts/review-gate.mjs` — a first-line verdict is final (ThinkFlowLab#643).** The scan that collects `Ran:`/`Tip SHA:` also matched any `Verdict:` line below the first one, so a body quoting the PASS an INC replaced — same tip — parsed as PASS and `decide` said ok. One condition: the scan no longer writes the verdict once the first line is one. - **`.github/scripts/merge-gate.mjs` — T0/T1 and the mechanical release PR need no PASS (ThinkFlowLab#640 INC point 2).** Without it the gate would refuse the daily release PR and every T0/T1 fix as soon as it became a required check, which is the opposite of AGENTS.md 6. The tier comes from the controller's `risk:T*` label; a release PR is recognised by the exact file list the standing approval names. - **Tests, through that entry point** — newest `INC` at the tip refuses the queue entry; `hold` refuses; a body whose first line is `Verdict (written by …)` is no conclusion and refuses; a first-line `PASS` posted as a proxy passes; a review body is judged like a comment. Plus the parser tests in `review-gate.test.mjs`. - **`AGENTS.md`** — hard rule 6 gets the ThinkFlowLab#628 sentence back, word for word: *an empty `schedule_list` is not evidence that a schedule does not exist; never delete or recreate a schedule from it, and never report "no timers" as a finding*. Merge-gate rule 4 states the fixed first-line format. - **`docs/autonomous-dev.md`**, **`.claude/skills/review-pr/SKILL.md`** — the template and the rules say what the gates now read. ## What I ran At tip `8203bf3d`: ``` node --test .github/scripts/merge-gate.test.mjs .github/scripts/review-gate.test.mjs # tests 43 # pass 43 # fail 0 ``` - The whole `.github/scripts/*.test.mjs` suite: 121 pass, 4 fail — all four in `ds-bot-patches.test.mjs`, which shells out to `git`; this container has no `git` (`spawnSync git ENOENT`). They are unrelated to this diff and CI runs them. - Live, on this PR: the check name is `merge-gate` and it now appears on a pull_request head — at `f28f63ba` the run is 9/9 (the eight usual ones plus `merge-gate`), which is the first of the two conditions for listing it as required. - Existing gate tests were kept green: the old Verifier block (`Tip SHA:` … `Verdict: PASS`) still parses, so nothing already recorded stops counting. ## Red without the source change With `.github/scripts/review-gate.mjs` taken from `fe79de66` (no first-line support), the tests kept: ``` not ok 12 - ThinkFlowLab#637: the queue entry refuses the newest conclusion INC, through main() not ok 13 - ThinkFlowLab#637: a verdict Chief posts for the Reviewer parses like the Reviewer wrote it not ok 16 - ThinkFlowLab#637: a review body in the fixed format is judged the same as a comment not ok 37 - ThinkFlowLab#637: a first-line verdict carries its own tip and its evidence below parses the same # tests 38 # pass 34 # fail 4 ``` (The "first line is not the verdict" refusal holds on both sides, as it must — that half only refuses.) With `.github/workflows/merge-gate.yml` and `.github/scripts/merge-gate.mjs` taken from `8ffa52b9` (no `pull_request` run, no report mode), the same command keeps: ``` not ok 11 - workflow: merge_group judges, pull_request reports, read-only, judges the base copy not ok 17 - ThinkFlowLab#639: a report names what the entry would refuse and still passes not ok 18 - ThinkFlowLab#639: the pull_request entry point reports without a head_ref and does not throw # tests 40 # pass 37 # fail 3 ``` With `review-gate.mjs` as it is on `f28f63ba` (first line set, scan still writing the verdict): ``` not ok 41 - ThinkFlowLab#643: a quoted verdict later in the body does not overwrite the first line # tests 41 # pass 40 # fail 1 ``` ## Not run `npm run verify` end to end (patches check and `arch-docs` need `git`), the core/plugin suites (this diff touches `.github/` and docs only), smoke. CI covers them at the tip. ## Risk One direction only: text that is not a first-line verdict is no longer a conclusion, so a PR whose only conclusion is in the old shape waits for a fresh one — the safe direction, and the old `Tip SHA:` block still parses. **The gate still only blocks once `merge-gate` is a required check of the `main merge queue` ruleset** — a ruleset change for the maintainer, never an agent (AGENTS.md → Merge gates 1), and it must name the job `merge-gate`; the workflow name `Merge gate` is a context no run produces and would leave every pull request `blocked`. The report on `pull_request` can name a refusal that has since been answered: that is deliberate, and why it never fails there — the queue entry re-reads live. Tier from the diff: T2 (CI gate, no secrets/permissions). <!-- ThinkFlowLab#640 INC revision at 8203bf3 -> next tip: paste into the PR body (author's right) --> ### Revision after the 8203bf3 INC - **INC point 1 (`review-gate.mjs`).** `parseReport()` now keeps the first line's `tip` as well as its `verdict`: a quoted `Tip SHA: <other sha>` block below the conclusion can no longer move the tip, so an INC at this head is no longer dropped as "about another tip" while the PASS it refused revives. The first line's own `Ran:` is the evidence, and a quoted block's `Ran:` does not replace it. - **INC point 2 (`merge-gate.mjs`).** `exemptFromPass` no longer reads `risk:t0`/`risk:t1` labels. The workflow already checks the script out from the base commit, so it now loads the base copy of `risk-gate.mjs` and `.github/agent/risk-rules.json` and computes the tier from the changed files. A label is a claim, not a fact: one shared account, no signature (ThinkFlowLab#627). `hold` still refuses at any tier, and an empty file list is no exemption. - `AGENTS.md` → Merge gates and `docs/autonomous-dev.md` say the same thing in one paragraph each. **Ran:** `node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs .github/scripts/risk-gate.test.mjs` -> **58 tests, 58 pass, 0 fail** (review-gate 24, merge-gate 20, risk-gate 14). **Red without the source change:** `review-gate.mjs` back at `8203bf3d` -> 23/24, red `ThinkFlowLab#640: a quoted Tip SHA later in the body does not move the first line to another tip`; `merge-gate.mjs` back at `8203bf3d` -> 19/20, red `ThinkFlowLab#640: the tier comes from the changed files, and a label never exempts`. **Not run:** the whole `.github/scripts` suite beyond those three files, and any CI job locally (no `gh` here); CI covers them on the pushed tip. ### Revision after `fa9a76f4`: the queue entry's `head_ref` arrives fully qualified - **What changed.** `prFromHeadRef()` in `.github/scripts/merge-gate.mjs` now accepts a head_ref that carries the `refs/heads/` prefix, and still accepts the bare `gh-readonly-queue/...` form. - **Why it is not cosmetic.** Every real `merge_group` run so far failed before judging anything: GitHub passes `merge_group.head_ref` as `refs/heads/gh-readonly-queue/main/pr-<N>-<sha>`, and the parser required the string to start at `gh-readonly-queue/`. Five queue entries, five failures — `39602546475`, `39602806953`, `39653640861`, `39694988559` (ThinkFlowLab#626) and `39696278650` (ThinkFlowLab#652), each ending in `cannot read a pull request from head_ref`. So the job has never judged a queue entry; listing `merge-gate` as a required check would have blocked every merge, not gated it. - The workflow comment says which spelling the event carries, so the next reader does not re-learn it. **Ran:** `node --test .github/scripts/merge-gate.test.mjs .github/scripts/review-gate.test.mjs .github/scripts/risk-gate.test.mjs` -> **59 tests, 59 pass, 0 fail** (merge-gate 21, review-gate 24, risk-gate 14). **Red without the source change:** with `.github/scripts/merge-gate.mjs` restored to `fa9a76f4` (the one-line prefix strip removed) -> 20/21, red `ThinkFlowLab#652 follow-up: a head_ref that carries refs/heads/ still names the queue entry`. **Sequencing, for whoever flips the ruleset:** the job reads the script from the **base** commit, so a queue entry for this very pull request still runs the broken parser on `main`. `merge-gate` can only be listed as a required check after this fix is on `main`; listing it first would freeze the repository.
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…kFlowLab#652) ## Reproduce On `main`, with `GITHUB_TOKEN` saved for all Bots (`scope: 'all'`): 1. A Bot shared with one other person opens a scheduled turn, or a group round starts from someone else's line; its shell env is `{}`. 2. The same Bot's shell on a turn the owner typed holds the key. 3. Nothing in either env tells the three cases apart: not configured, configured-but-withheld, present-and-empty. `resolveEnv` returns `{}` for every refusal, so the Bot reports "the user did not configure it" (issue ThinkFlowLab#651, steps 1-4). 4. `list_secrets` says `you may use it as $DSH_SECRET_GITHUB_TOKEN` even when the key is withheld from that turn. ## What changed - `core/src/host/turn-origin.js`: `turnOrigin(rt, sessionId)` returns the turn's recorded verdict *before* `settled` narrows it, so a shared Bot's scheduled turn stays `scheduled` instead of collapsing to `other`. - `core/src/host/secrets.js`: `withheldReason()` with a closed reason set (`remote-bot`, `turn-not-owner`, `shared-bot-scheduled`, `shared-room`, `unknown-turn`); `resolveEnv` writes `DSH_SECRETS_WITHHELD=NAME:reason,...` (names and reasons, never a value); `list_secrets` and the `request_secret` answer read the same function. A key whose `scope` names this Bot reaches that Bot's own scheduled turn while it is shared; `scope: 'all'` keeps the fail-closed rule. - Tests: 5 new `ThinkFlowLab#651` cases and one updated expectation in `core/test/secrets.test.js`. - Docs: new `### A withheld key says why (ThinkFlowLab#651)` in `docs/subsystems/core-host.md`, one `changelog.d/651-secrets-withheld.fixed.md` fragment, and one sentence in `AGENTS.md` hard rule 2 (main has no "Secrets" section; that rule is the closest place, and the issue asked for a line there). - `patches/ds-bot/` regenerated (`0014-flybot-branding.patch` and `files/src/host/turn-origin.js`). ## What I ran - `node --import ./test/harness-resolve.mjs --test test/secrets.test.js test/turn-origin.test.js test/group-status.test.js` -> **44 tests, 44 pass, 0 fail** (`test/secrets.test.js` alone: 17/17). - `node .github/scripts/changelog.mjs check` -> 2 fragments well formed. - `node scripts/ds-bot-patches.mjs check` -> `core/` equals `ds-bot/` + patches, 10 patches apply cleanly. - `core/scripts/check-arch-docs.mjs` -> OK (8 changed files). - `npx oxlint@1.76.0 -c .oxlintrc.json index.js web-search.js src dev scripts test` -> 0 errors, 43 warnings (pre-existing). - Rebased onto `0204900` (the ThinkFlowLab#626 merge) before pushing; the diff applied cleanly and ThinkFlowLab#626's `turn-origin` changes are kept. ## Red without the source change With `core/src/host/secrets.js` and `core/src/host/turn-origin.js` reverted to `0204900` and the tests unchanged, **5 of 17 fail**: ``` not ok 11 - a key for all Bots stays out of a shared Bot's shell when the turn is not known to be the owner's not ok 14 - ThinkFlowLab#651: someone else's turn names the key and the reason, and carries no value not ok 15 - ThinkFlowLab#651: a friend's Bot says remote-bot, and a host with no key says nothing at all not ok 16 - ThinkFlowLab#651: a shared Bot's scheduled turn withholds scope "all" and keeps it for the Bot it names not ok 17 - ThinkFlowLab#651: list_secrets and request_secret give the same reason, and never a value # pass 12 # fail 5 ``` The 12 that stay green are the pre-existing cases plus the owner-turn guard (a key the owner's own turn already had). ## Not run - `npm run verify` and the full core suite: this working copy has no `core/node_modules` (`npm ci` needs esbuild; unrelated suites stop at `Cannot find package 'esbuild'`), so CI runs them. `test/secrets.test.js` runs standalone here. - The smoke test (needs `dev/bench.sh` and a DSH checkout). The change is in `core/src/host/secrets.js`, so a smoke run after merge is advisable. ## Risk Security surface (secrets). No value is ever added anywhere new: the variable carries names and reasons only, and redaction is unchanged. Two behaviour changes: (1) a shell now carries `DSH_SECRETS_WITHHELD` when something is withheld; (2) a key whose `scope` list names this Bot now reaches that Bot's own scheduled turn while it is shared - the owner's explicit choice per ThinkFlowLab#545 - while `scope: 'all'` is unchanged and stays fail-closed. Needs a PASS at the tip plus green `tests-ok`. Fixes ThinkFlowLab#651
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 28, 2026
…wLab#1594) Written-by: Dev Fixes ThinkFlowLab#1384 Owner: Chief Reviewer: Reviewer Tip SHA: d1a3ae7 Base: fa932f8 ## What changed - **Pin**: `core/dev/harness/package.json` goes 0.2.0-rc.2 → 0.2.1-rc.2 (npm `latest`). The file comes from upstream `ds-bot/`, so the change rides in `patches/ds-bot/0007-version-and-tooling.patch` (and `topics.json`); `ds-bot/` is untouched. - **CI**: `.github/workflows/test.yml` `dsh:`/`DSH:` inputs and `.github/workflows/nightly.yml` matrix rows (plus the header comment). The `test` job name `ubuntu-latest · Node 24 · DSH 0.2.0-rc.2` is **unchanged** — it is the merge queue's required check, so the ruleset does not move. - **Container image**: `Dockerfile` `ARG DSH_VERSION` moves with the same pin, and the container moves to `network_mode: host` with `docker/web.patch.yml` binding `127.0.0.1`. DSH 0.2.1 refuses a wildcard host — the image built and then died with `host "0.0.0.0" is an unspecified (wildcard) address, which is not supported` — so a published port no longer works. Exposure is the same as before (the host's loopback only); `FLYBOT_PORT` now reaches the entry point and the health check, and a new test refuses any wildcard bind in the container configs again. - **Install help and local CI**: `core/scripts/install-doctor.mjs` default package, `core/scripts/local-ci.sh` step label and harness install. - **Docs and commands**: the four `npx` commands and the tested-with lines in `README.md` / `README.zh.md`, `docs/deepseek-api-key.md`, `docs/known-issues.md`, `docs/known-issues.zh.md`, `docs/docker.md`, `docs/docker.zh.md`, and `docs/subsystems/install.md` for the container's networking. - **Tests**: new `core/test/dsh-pin.test.js` compares every copy with the harness pin and pins the job name to the old string, so a half-done bump fails. `core/test/turn-origin.test.js` (ThinkFlowLab#626) and the `about.test.js` fixture/hints and `install-doctor.test.js` move with the pin. ## Tests - Red first on the base tree: `node --test core/test/dsh-pin.test.js` → 5/5 fail (harness still 0.2.0-rc.2). - Red on the container configs before this commit: `docker/web.patch.yml` on `0.0.0.0` → `node --test .github/scripts/docker-profile.test.mjs` → 2 failures (`secure defaults`, `no container config binds a wildcard host`); CI at `723094f9` failed `build and start check` with the wildcard-host error above. - After the change: `node --test .github/scripts/docker-profile.test.mjs` → 6/6 pass; `node --test ".github/scripts/*.test.mjs"` → 544 tests, 542 pass, 2 skipped, 0 fail; `node --test core/test/dsh-pin.test.js` → 5/5 pass; `node scripts/ds-bot-patches.mjs check` → `ok core/ equals ds-bot/ + patches/ds-bot/`. - Not run here: `npm run verify` and the core suite (`about.test.js`, `turn-origin.test.js` need the esbuild/DSH harness install), and the repo has no working git on this box, so `ds-bot-patches.mjs generate` could not run — `check` proves the result instead. CI runs them at this tip, including the real `docker compose up --wait` start check. ## Risks - Suite-wide: a DSH that changed private shapes could break tests only CI exercises; the ThinkFlowLab#626 guard is the intended tripwire and it is at the new pin now. - Container: `network_mode: host` is Linux-only — Docker Desktop on macOS and Windows does not route it — and it replaces port publishing, so `FLYBOT_PORT` is read by the entry point instead of by compose. The container guide says both. - `dev/live-e2e/test/scenarios.test.mjs` keeps `dsh: '0.2.0-rc.2'` as a matrix label, not an install pin; left alone so the e2e matrix keeps naming the runtime it actually used. ## Rollout Squash to `main`; no ruleset, workflow-name or config-key change. Next local `npm run harness` installs 0.2.1-rc.2; an existing container needs one `docker compose up -d --build` to pick up the new overlay. <!-- flybot:checkpoint --> { "pr": 1594, "claim": "Chief", "issue": 1384, "summary": "The harness pin, both CI workflows, the container image, the install help, the CI script, the README commands and five docs named DSH 0.2.0-rc.2 while npm latest is 0.2.1-rc.2, so a fresh install ran an untested version. Every copy now carries 0.2.1-rc.2, the harness file rides in patch 0007, and a new dsh-pin test compares all copies against the pin. The container needed more than a string: DSH 0.2.1 refuses a wildcard host, so it runs with host networking and binds the host's 127.0.0.1. The test.yml job name, the merge queue's required check, stays unchanged.", "files": ["Dockerfile", "compose.yaml", "docker/web.patch.yml", ".github/scripts/docker-profile.test.mjs", ".github/workflows/test.yml", "core/dev/harness/package.json", "core/test/dsh-pin.test.js", "README.md", "patches/ds-bot/0007-version-and-tooling.patch"], "checks": { "ran": "node --test .github/scripts/docker-profile.test.mjs; node --test \".github/scripts/*.test.mjs\"; node --test core/test/dsh-pin.test.js; node scripts/ds-bot-patches.mjs check", "result": "green", "at": "d1a3ae7b243852a5f0477e6822851fd307d65a01" }, "next": "CI at this tip, including the docker build-and-start check; then a Reviewer PASS at this sha." } <!-- /flybot:checkpoint --> Docs-not-needed: only the pinned version string moves; no subsystem behavior changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #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 #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 fix: honour a standing approval on a turn with no verdict (#608) #616) turn-origin: an unrecognised source kind in a Bot's own chat counts as the owner (fail-open left open by #616) #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 — #601). Nothing in the diff was edited after Dev produced it;out/626-tip-diff.diffwas applied withgit applyand the six files committed unchanged.