Skip to content

Say why a saved key is withheld from a shell (#651) - #652

Merged
Birfy merged 1 commit into
mainfrom
fix/651-secrets-withheld
Oct 21, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/651-secrets-withheld

Conversation

@Birfy

@Birfy Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

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 Secrets: an absent $DSH_SECRET_* says nothing — distinguish not-configured, withheld-this-turn and empty, and honour an explicit per-Bot scope on the owner s scheduled turns #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 #651 cases and one updated expectation in core/test/secrets.test.js.
  • Docs: new ### A withheld key says why (#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

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 - #651: someone else's turn names the key and the reason, and carries no value
not ok 15 - #651: a friend's Bot says remote-bot, and a host with no key says nothing at all
not ok 16 - #651: a shared Bot's scheduled turn withholds scope "all" and keeps it for the Bot it names
not ok 17 - #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 #545 - while scope: 'all' is unchanged and stays fail-closed. Needs a PASS at the tip plus green tests-ok.

Fixes #651

A shell lost every key on turns that were not the owner's and looked
identical to a host with no key saved: `resolveEnv` returned `{}`.
`DSH_SECRETS_WITHHELD` now names each withheld key and a reason from a
closed set (remote-bot, turn-not-owner, shared-bot-scheduled,
shared-room, unknown-turn), never a value, and `list_secrets` and
`request_secret` read the same `withheldReason`.

A key the owner scoped to one Bot (`scope: [<botId>]`) now reaches that
Bot's own scheduled turn while it is shared; `scope: 'all'` keeps the
fail-closed rule. `turnOrigin` exposes the turn's recorded verdict so
`settled` (permission) and the reason (fact) can differ.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 21, 2026
@Birfy Birfy self-assigned this Oct 21, 2026
@Birfy

Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 726eb72
Written-by: Reviewer

Tip SHA: 726eb72149b0b3ab13b3e5865fd742053aff9dae
CI at tip: 8/8 success
Tier: T3 — core/src/host/secrets.js is on the T3 list in .github/agent/risk-rules.json

Ran at this tip:

$ cd core && TZ=UTC DSH_HARNESS=… node --import ./test/harness-resolve.mjs --test test/secrets.test.js
# tests 17 / pass 17 / fail 0

$ export PATH=<portable git>/bin:$PATH && node scripts/ds-bot-patches.mjs check
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/

Gate ③, core/src/host/secrets.js back to 0204900, tests kept:

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 - #651: someone else's turn names the key and the reason, and carries no value
not ok 15 - #651: a friend's Bot says remote-bot, and a host with no key says nothing at all
not ok 16 - #651: a shared Bot's scheduled turn withholds scope "all" and keeps it for the Bot it names
not ok 17 - #651: list_secrets and request_secret give the same reason, and never a value
# tests 17 / pass 12 / fail 5

Checked by reading. withheldReason is a closed set and the only source for all three surfaces (resolveEnv, list_secrets, request_secret); no path returns a value; turnOrigin returns the recorded verdict before settled narrows it, which is the only way to tell a shared Bot's scheduled turn from a room turn, and it reads entry.origin without mutating anything. scope: 'all' keeps the fail-closed rule and only an explicit scope: [<botId>] reaches that Bot's own scheduled turn (B), which is the owner's decision in #651. DSH_SECRETS_WITHHELD cannot collide with a secret's own variable (DSH_SECRET_<NAME>). Docs, AGENTS.md hard rule 2, the changelog fragment and the patch series all match the code, and the 168 lines moved inside 0014-flybot-branding.patch are a regeneration, not a behaviour change — check above proves the series still rebuilds core/ exactly.

Two things the owner should read before approving — they are not reasons to refuse the code, they are consequences of it.

  1. A withheld key names itself to turns that are not the owner's. remote-bot and turn-not-owner put NAME:reason into the environment of that turn's shell. list_secrets already lists every saved key's name to any Bot on the team, so no new class of disclosure arrives through the tool — but the environment variable is ambient: it is present for every command a guest's turn runs, without calling anything. If the owner would rather a turn they are not driving learn only that something was withheld, the fix is to omit the names for REMOTE and NOT_OWNER (report a count or the bare reason) and keep names for the owner's own turns, which is what the feature is for.

  2. B reaches further than "the Bot's own scheduled turn". On a shared Bot, a scheduled turn now carries an explicitly scoped key. A Bot's schedules can be created from a turn someone else is driving (the tool is DSH's, not this repo's), so a guest who can talk the Bot into scheduling something has arranged for the owner's key to be present when that turn runs — with whatever prompt they wrote. Scoping the key to the Bot is what makes it available there; 'all' keys are still refused. If that is not what "only this Bot" was meant to promise, the rule should be "a key reaches a scheduled turn only when the schedule's creator was the owner", and that is a policy decision, not something this diff can settle.

Not enqueued: T3 needs the maintainer's approve together with this PASS. The PASS stands until the tip moves.

@Birfy
Birfy added this pull request to the merge queue Oct 21, 2026
@Birfy Birfy added area:automation Scheduled tasks and automation area:security Auth, grants, approvals, secrets bug Something isn't working labels Oct 21, 2026
Merged via the queue into main with commit a5a887b Oct 21, 2026
8 checks passed
@Birfy
Birfy deleted the fix/651-secrets-withheld branch October 21, 2026 23:55
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 Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:automation Scheduled tasks and automation area:security Auth, grants, approvals, secrets bug Something isn't working

Projects

None yet

1 participant