Harden the untrusted fence against whitespace inside the tag by HugoXK · Pull Request #574 · ThinkFlowLab/FlyBot

HugoXK · 2026-10-21T02:09:46Z

Harden the untrusted fence against whitespace inside the tag

The untrusted-fence neutralizer stripped <+optional /+spaces but not whitespace between < and /: < /untrusted-message> survived, and a parser that ignores whitespace inside a tag reads that the same as the real closing marker. Both stripUntrusted and stripMarkers now allow whitespace inside the tag (<\s*\/?\s*untrusted-(message|file)), so < / untrusted-message> and </ untrusted-file> are stripped too; the fixed-point loop is unchanged. It removes more, never less.

core/test/fence-property.test.js fuzzes a corpus of reconstitution attempts (nested, case-varied, spaced, sibling tags, unterminated) and asserts the result has no reachable marker and is idempotent.

Ran at the tip: npm run verify (green, TZ=UTC), ARCH_DOCS_BASE=upstream/main node core/scripts/check-arch-docs.mjs. Not run: smoke, Desktop, real-model steward.

…owLab#9 family) stripUntrusted and stripMarkers now also strip a marker with whitespace inside the tag (< / untrusted-message>), which a parser that ignores whitespace reads the same. Adds core/test/fence-property.test.js, a corpus of reconstitution attempts checked as a fixed point.

Birfy

Verdict: PASS at 902a888

Tip SHA: 902a8886358c59b98d32bb82328c1612b66ff2d9
Read the whole diff: `core/src/host/delivery.js` (+5 -4) widens both marker patterns so whitespace is allowed between `<` and the optional `/` — `<\s*\/?\s*(?:untrusted-message|untrusted-file)` and the matching single-tag form — so `< / untrusted-message>` is neutralized like the real closing marker. The fixed-point loop, the "never reachable in any order" property and `unwrapUntrusted` behaviour are unchanged; the rest is the new property test, one docs sentence, the 0005 patch regenerated with the same two lines, and the new `patches/ds-bot/files/test/fence-property.test.js` mirror.
Why this is the right shape: a parser that ignores whitespace inside a tag reads the spaced form as the same marker, and the fence is exactly the boundary that must not be reachable from peer text (#9 family). The change only widens what is stripped; nothing that was fenced before is left unfenced, and ordinary text that merely contains `< tag`-like words is stripped on purpose (same as before for the unspaced form).
Ran at the tip: CI tests-ok green at this SHA (test, plugins tree/social/timeline, package). On a copy of current main with this diff applied: `core/test/fence-property.test.js` 5/5, `ds-bot-patches.mjs check` OK, `check-arch-docs.mjs` OK (6 files), `changelog.mjs check` OK, core lint 0 errors / 43 warnings, TZ=UTC.
TDD: with only delivery.js at the base commit, 3 of the 5 property tests fail (the spaced forms survive) and all 5 pass with the change.
Merge matrix: merges cleanly with current main and with every other open PR (checked by local 3-way merges against main 23ea5e24).
Not run: one-shot npm run verify (2 GiB OOM), full core suite, smoke, Desktop.

Tier: T3 (core/src/host/delivery.js is on the trust-boundary list) — the owner's own click in the GitHub UI is the approval, and this PASS sits on the tip. I am not approving or merging it myself. Any new commit voids this PASS.

Birfy · 2026-10-21T03:04:06Z

One clarification I owe the owner before the click, after re-reading the fence code at main 33fb55ed (this keeps the PR in the list; it is the one trust-boundary item I am still asking a click for):

What is not reachable: no parser in the host accepts a space between < and the optional /. isFencedUntrusted and unwrapUntrusted require <untrusted-message ...> exactly, so < / untrusted-message> never becomes a fence in code.
What is real: the stripper's job is that peer text can never leave an end-of-data marker a reader can take for one, and the model reading the fenced log line is exactly that reader. The existing pattern already strips < untrusted-message> (a space after <) even though no parser accepts that either, so the spaced-slash form was the one gap left in the same class. The new pattern only widens what is removed; nothing that was stripped before stays, and the fixed-point loop is unchanged.

So: defence-in-depth hardening of a trust boundary with a property test and a red-without-fix proof (3 of 5 fail on the base), not a code-reachable escape. If the owner would rather batch it, it can be parked like the others — say so and I will.

Birfy · 2026-10-21T03:17:18Z

Held, not merged and not closed, by the maintenance round (2026-10-21).

Why I am not deciding this one myself: it changes core/src/host/delivery.js, which is on the trust-boundary list, and my own reading is that it is defence-in-depth rather than a fix for a reachable defect (see the clarification above). The owner's line admits a PR only for a reproducible defect or a routine need, and a change on the security surface still goes to the Chief before it is merged.

The review stands: PASS at 902a8886, CI green, fence-property 5/5 with 3 of them failing when the source change is reverted. One word from the Chief ("take it" or "close it") and it is done either way.

Birfy · 2026-10-21T03:20:23Z

Parked (maintenance round, 2026-10-21).

Per the Chief's call: with no code-reachable reproduction this is not "a reproducible real defect" under the admission line, so it does not take a slot. The PR stays open, the awaiting-approval label is removed, and the review is on record (PASS at 902a8886, fence-property 5/5 with 3 failing when the source change is reverted, CI green).

It comes back the moment there is a reproduction: a spaced form that some parser or reader actually accepts as a marker.

Birfy · 2026-10-21T10:01:34Z

Held per the new merge-gate rule.

Any PASS posted on this PR (including a self-PASS at the tip) is a record only, not a merge signal. Do not add this PR to the merge queue while hold is set.
The gate is mechanical, not advisory: a PASS landing at the tip can be queued within seconds (Record the decisions a Bot considered and rejected #566 was queued 10 s after its PASS: 09:48:18Z PASS → 09:48:28Z queue → 09:50:40Z merged as abaf938b), so a "hold" that lives only in chat cannot stop it.
hold is removed when a real, code-reachable reproduction for Harden the untrusted fence against whitespace inside the tag #574 arrives and the owner scope is settled.

No code change and no queue action taken here.

Birfy · 2026-10-21T11:53:44Z

Owner decision (2026-10-21): merge. Relay: Chief. This is the only status note on this PR from here on — no further held/parked/record-only comments.

PR checks: Verdict: PASS at 902a8886 (earlier review), CI green at tip, 3 of the 5 property tests turn red with the source change reverted. No code-reachable reproduction; merged as defence-in-depth on the owner's call.

Enqueued by: Chief (owner proxy — Reviewer session unavailable).

Birfy · 2026-10-21T11:57:49Z

Post-merge record (reviewer)

Merged: 3995d9b9 (squash, via the merge queue; main tip now 3995d9b9).
Reviewed tip: 902a8886 — unchanged since the PASS, so that verdict stands; CI at the tip was 8/8, and gate ③ is the recorded one (3 of 5 fence-property tests red with the source change reverted).
Owner decision on record above; hold lifted before the queue, and the queue gate was run with that approval attached.

…#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).

…kFlowLab#611 merge dropped from core-host.md (ThinkFlowLab#671) (ThinkFlowLab#672) Fixes ThinkFlowLab#671 — restores the two paragraphs that the `ThinkFlowLab#611` merge silently dropped from `docs/subsystems/core-host.md`. Documentation only: this touches no code, no test, no changelog entry, and does not ride along with `ThinkFlowLab#664`. ## Reproduce Measure the file itself (`GET /contents/docs/subsystems/core-host.md?ref=<ref>`), counting `stripMarkers`, `deployment-wide`, `hiddenByGone`, `remoteChats.sync`: | ref | `stripMarkers` | `deployment-wide` | `hiddenByGone` | bytes | | --- | --- | --- | --- | --- | | `4f6e3e54` (before ThinkFlowLab#611) | 1 | 3 | 0 | 75046 | | `16dc44b2` (ThinkFlowLab#611 merge) | 0 | 1 | 1 | 74379 | | `8005fc9d` (main, base of this PR) | 0 | 1 | 1 | 74379 | `main`'s copy is byte-identical to `16dc44b2`'s, so the loss happened in that one merge and nothing after it touched the file. `ThinkFlowLab#611`'s branch carried an older copy of the whole file; the three-way merge (`git merge-file -p head base main`) exits 0 with no conflict, so GitHub had nothing to warn about and the drop was invisible in review. The number falls because the older copy simply does not contain the `ThinkFlowLab#574` sentence or the `ThinkFlowLab#613` paragraph. ## What changed - `docs/subsystems/core-host.md`, **+2 / −1**, exactly two places, both copied verbatim from `4f6e3e54`: - the `ThinkFlowLab#613` bullet *"The Main Bot list is deployment-wide (ThinkFlowLab#613): `set-main` requires the actor to own `body.id` … and the deployment-wide list itself are unchanged."*, restored as its own bullet under `### Team state and access` right after the merged overview bullet (which keeps main's newer `ThinkFlowLab#612` / `ThinkFlowLab#620`-era wording untouched); - the tail of the `ThinkFlowLab#574` sentence in `### Untrusted fences in every shared room`: `stripUntrusted`'s whitespace-inside-the-tag pattern, `stripMarkers` for one tag, and `core/test/fence-property.test.js` as the fuzz guard. - Everything already on main stays, including `ThinkFlowLab#611`'s own `remoteChats.sync` / `hiddenByGone` paragraph (count still 1) and every later edit. - No other file is in the diff: `git diff --shortstat origin/main...HEAD` → `1 file changed, 2 insertions(+), 1 deletion(-)`. ## What I ran Tip SHA: `5c610391c81e2464a010893020045fcef7f874e5` (Node 22.23.3, `TZ=UTC`, base `8005fc9d`): - Acceptance check on the file itself — `stripMarkers` 1, `deployment-wide` 3, `hiddenByGone` 1, `remoteChats.sync` 1 → **PASS** (script saved as a run-only check, not committed; the numbers are the issue's acceptance line). - `node core/scripts/check-arch-docs.mjs` → `arch-docs: OK (1 changed file(s))`. - `node core/scripts/lint.mjs` (`DSH_HARNESS` oxlint) → **0 errors**, 43 pre-existing warnings, `arch-docs: OK (1 changed file(s))`. - `node scripts/ds-bot-patches.mjs check` → pin `ds-bot/` = `Fei2411/DeepSeek-Bot@2b10c70` (tree `5834c00`, v0.1.5), 10 patches apply cleanly, `core/` = `ds-bot/` + patches. - `node core/scripts/build-client.mjs --check` → exit 0. - `git diff origin/main...HEAD -- docs/subsystems/core-host.md` read line by line: one added bullet, one replaced line, nothing else. ## Red without the source change With `docs/subsystems/core-host.md` back at `origin/main` (`8005fc9d`) and the same acceptance check: - `{"stripMarkers":0,"deployment-wide":1,"hiddenByGone":1,"remoteChats.sync":1}` → **FAIL** (`exit 1`), asking for 1 / 3 / 1 / 1. That is the reported regression, reproduced from the file alone. ## Not run - The full test suites, the three plugin suites and the smoke bench: the diff is one documentation file, no code path reads `core-host.md`, and this box has no `pnpm` (`dev/bench.sh: 62: pnpm: not found`). Green `tests-ok` at the tip is the gate; if a full local run is wanted, it can be added at the next tip. - No `changelog.d/` entry: nothing user-visible changes, and the entry would describe a documentation restore. ## Risk - **T0 by diff** (one file under `docs/`); worth noting the content is a record of host behavior, not executable. Nothing touches authz, secrets, saved state or the client bundle. - Out of scope, flagged for Triage: the same `ThinkFlowLab#611` merge also flattened one more `4f6e3e54` sentence — the `ThinkFlowLab#608` standing-approval clause (*"or for a turn that carries no verdict at all in the Bot's own chat — … (ThinkFlowLab#608)"*, plus the `the standing approval for <kind> was not used` log sentence). It is a third loss of the same accident, not part of this issue's acceptance line, so I did not touch it here; the issue says not to mix documentation restores. It needs its own issue if the owner wants it back. - The `ThinkFlowLab#613` bullet is restored verbatim, so it describes `set-main` / `roster.setMain` as of `4f6e3e54`; a later behavior change would need a separate edit rather than a rewrite here.

…lowLab#611 merge dropped (ThinkFlowLab#673) (ThinkFlowLab#674) Fixes ThinkFlowLab#673. Tip SHA: `46335cbda14f6bec8f55933d11af6c87918fefbc` ## Reproduce The `ThinkFlowLab#611` merge (`16dc44b2`) took a stale copy of `docs/subsystems/core-host.md`, and the same silent overwrite that `ThinkFlowLab#671` records for the `ThinkFlowLab#574` and `ThinkFlowLab#613` paragraphs dropped a third one: the `ThinkFlowLab#608` text about a turn that carries **no verdict at all in the Bot's own chat**, and the sentence about the `the standing approval for <kind> was not used` log line. Counted on the file itself (`GET /contents/docs/subsystems/core-host.md?ref=<ref>`): ```text ref no verdict at all | the standing approval for | was not used 4f6e3e5 1 | 1 | 1 <- before ThinkFlowLab#611 16dc44b 0 | 0 | 0 <- ThinkFlowLab#611 merge 7223a1a (main) 0 | 0 | 0 <- this branch's base ``` ## What changed - **`docs/subsystems/core-host.md` only.** The standing-approval / turn-origin paragraph is brought back to its `4f6e3e54` text: `turn-origin.js` again says that a no-verdict turn in the Bot's own chat is auto-answered (ThinkFlowLab#608), and the `the standing approval for <kind> was not used: the turn is "<verdict>"` sentence is back after "still logs each auto-answered ask". Nothing else in the file moves: no rewording, no reflow, no other paragraph. - One hunk, +1/-1: the paragraph is a single line in this file, so restoring it shows as one changed line. ## What I ran - Byte-compared the restored paragraph with `4f6e3e54`: **identical**, 2088 characters, character for character. - Counted the acceptance phrases on the new tip: `no verdict at all` **1**, `the standing approval for` **1**, `was not used` **1**; and the four that must not move are unchanged: `stripMarkers` **1**, `deployment-wide` **3**, `hiddenByGone` **1**, `remoteChats.sync` **1**. - `diff -u` against main: **1 hunk, 1 line removed, 1 added** — no other text touched. - `node core/scripts/check-arch-docs.mjs` needs `git diff` and this container has no `git` binary, so it is CI's `tests-ok` that runs it (docs-only change; the doc is the owned file's subsystem doc). ## Red without the source change This is documentation only, so no test turns red. The equivalent check is the acceptance count on the file: at `7223a1a1` (the branch's base, without this change) the three phrases are **0 / 0 / 0**; on the new tip they are **1 / 1 / 1**, and the byte-compare against `4f6e3e54` fails on the base and passes on the tip. Reverting the single file restores 0 / 0 / 0. ## Not run - `core/scripts/check-arch-docs.mjs` locally (no `git` in this container); CI's `tests-ok` covers it. - The rest of `npm run verify` (no `node_modules` here); docs-only, nothing in `core/` or `plugins/` changes. ## Risk Docs only: one paragraph, no code, no workflow, no label or config change. The restored text matches the behaviour that ThinkFlowLab#608 shipped and that is still in `core/src/host/approvals.js`; ThinkFlowLab#620's fail-closed wording on unknown source kinds sits in the same paragraph and is left exactly as main has it.

…is missing (ThinkFlowLab#688) Written-by: Dev The gate already refused a PASS with no `Ran:` line; the rule now lives in AGENTS.md, together with the one way we fix such a PASS without breaking "one conclusion per tip". Tip SHA: 96e4383 ## Reproduce `ThinkFlowLab#681`'s PASS at `4af18994` (05:59:29Z) carries no `Ran:` line, so `merge-gate.mjs` reads it as "lists no command it ran" and refuses the queue entry even with an approval; `ThinkFlowLab#574`'s PASS is the same shape. The refusal is correct — a PASS that ran nothing is not evidence — but the rule it enforces was written only in the script's comments and in `docs/autonomous-dev.md`'s description of it; `AGENTS.md`, which is what the routines and reviewers read first, did not say it. Nothing in this change alters the gate. ## What changed `AGENTS.md`, one bullet in **Gates**: - **A PASS must list what it ran.** The `Ran:` line (or a `Ran …:` heading over `$ ` commands) carrying at least one real command is part of the gate; `merge-gate` refuses a PASS that lists none (ThinkFlowLab#681, ThinkFlowLab#574). - When a PASS at a tip cannot be read for that reason, the reviewer may post **one replacement PASS at the same tip**: the body keeps the original conclusion, adds the `Ran:` lines of what was actually run, and carries `Replaces: the PASS at <sha> merge-gate rejected for having no Ran: line`. That is a replacement, not a second conclusion; apart from it one tip still carries one. No code, no workflow, no policy file, no test changes: the parser and the gate already behave this way. ## What I ran The change is prose, so the check is that the text says exactly what the gate does, and that the existing readers accept the replacement shape: ```text $ node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs # tests 69 # pass 69 # fail 0 ``` (That is the suite at the current `ThinkFlowLab#676` tip `ef340354`, not main: main's copies predate the attribution rules, and both versions refuse a PASS with no `Ran:`.) The replacement line itself is an ordinary line to `parseReport` — it is ignored — so a replacement PASS parses as a normal conclusion: verdict on the first line, attribution second, `Ran:` below. ## Red without the source change Not applicable in the usual sense: this adds no behaviour, so there is no test that turns red when the prose is removed. The refusal it documents is already pinned by `merge-gate`'s own case (`a PASS that lists no command it ran is not evidence`), which stays green with and without this bullet. ## Not run - `npm run verify`: docs-only change, no `core/`, plugin or client file touched. - No new test: adding a test that reads AGENTS.md for a sentence would test the copy, not the gate; the gate case already exists. ## Risk T3 by the repository's own `risk-gate` (`AGENTS.md` is a T3 path). The text is the owner's decision of 2026-10-22 (relayed by the Chief): a PASS must carry `Ran:`, and a PASS that `merge-gate` cannot read may be replaced once at the same tip with a `Replaces:` line. It grants no new merge right and changes no gate.

Birfy reviewed Oct 21, 2026

View reviewed changes

Birfy added the awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip label Oct 21, 2026

Birfy mentioned this pull request Oct 21, 2026

Harden the untrusted fence against whitespace inside the tag #573

Closed

Birfy added area:security Auth, grants, approvals, secrets bug Something isn't working and removed awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip labels Oct 21, 2026

github-actions Bot added the risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff label Oct 21, 2026

Birfy added agent-pr Pull request opened by an agent hold A person froze this issue or PR; agents and automation leave it alone labels Oct 21, 2026

This was referenced Oct 21, 2026

Merge queue ignores hold / manual freeze: a PASS at the tip is enqueued ~10 s later #602

Closed

Rule says the controller sets risk:T*, but risk-tier.yml is report-only and never writes labels #609

Closed

Birfy removed agent-pr Pull request opened by an agent hold A person froze this issue or PR; agents and automation leave it alone labels Oct 21, 2026

Birfy added this pull request to the merge queue Oct 21, 2026

Merged via the queue into ThinkFlowLab:main with commit 3995d9b Oct 21, 2026
8 checks passed

Birfy mentioned this pull request Oct 22, 2026

Require a Ran: line in a PASS, and one Replaces: replacement when it is missing #688

Merged

Navigation Menu

Repository navigation

Harden the untrusted fence against whitespace inside the tag - #574

Harden the untrusted fence against whitespace inside the tag#574
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:feat/fence-hardening

HugoXK commented Oct 21, 2026

Uh oh!

Birfy left a comment

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

HugoXK commented Oct 21, 2026

Uh oh!

Birfy left a comment

Choose a reason for hiding this comment

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Uh oh!

Birfy commented Oct 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants