Skip to content

Read a room-share envelope past a brace in a chat line's text - #2529

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/3-room-share-braces
Nov 5, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/3-room-share-braces

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2447

Owner: Dev-3
Reviewer: Birfy
Written-by: Dev-3
Risk tier: T3 (risk-gate: path core/src/shared/room-share.js)

The defect

parseRoomShare finds the end of a FLYBOT-ROOM-SHARE body by counting { and } from the first brace after the marker, and that scan never tracked JSON string literals. encodeRoomShare writes text with JSON.stringify, which leaves braces inside the string raw, so one unbalanced { or } in a message was counted as structure: the object looked finished too early, JSON.parse failed, and the whole envelope was dropped. A sent line therefore never arrived — the host routes a delivered share through parseRoomShare (ingestRemoteRoomLine, applyInboundRoomShare) and an unparsed envelope is not a room line at all — and a join that copied such a line into its history lost the whole join with it. On live f36b3881 the loop is still the naive one at core/src/shared/room-share.js:260-267; the client's envelopeTail (plugins/social/client.js, #855) already scanned past string literals for held-line display, so only the parser the host trusts was left behind.

The change

  • The object-end scan skips a JSON string literal and its \ escapes: a brace inside text is a message's own character, never structure, and a \" does not end the literal. A truncated body (no closing brace) is still refused, and isRoomShareText, every field bound, the op rules and the marker rule are untouched.
  • core/test/room-share-braces.test.js is the new suite: a line whose text opens a brace, one whose text closes a brace, a brace after an escaped quote (plus a } later in the same string), a join whose copied history carries braces, the plain/balanced controls, and a truncated envelope that must stay refused. Six cases, the first three red on the base tree.
  • docs/subsystems/core-host.md states how an envelope's body is found and why a brace in a chat line is text; changelog.d/2447-room-share-braces.fixed.md is the fragment.
  • patches/ds-bot/files/src/shared/room-share.js and patches/ds-bot/files/test/room-share-braces.test.js are generated from core/. core/client.js is unchanged: src/client/ does not import this module.

One issue, one in-flight line

#2447 carries needs-human because Triage found the fix already in flight on another branch (comment 6640349586 on #2447, 2026-11-04), and its note ends: "If that PR is abandoned, the issue can come back to main unchanged and be read as agent-ready." The carrier it named, #2448 (delivery1/room-share-brace-scan → delivery/ai-centric-development-service), was closed without merging at 2026-11-05T08:06:55Z (merged_at is null), so main is the only line now, and this is the only open PR that names #2447.

Red first

On a clean tree at f36b3881, with only core/test/room-share-braces.test.js placed on it, TZ=UTC node --import ./test/harness-resolve.mjs --test test/room-share-braces.test.js exits 1:

not ok 1 - a line whose text opens a brace is read back, not dropped
  Expected values to be strictly equal:
  + actual   undefined
  - expected 'line'
not ok 2 - a line whose text closes a brace is read back, not dropped
  Expected values to be strictly equal:
  + actual   undefined
  - expected 'line'
not ok 3 - a brace right after an escaped quote is still inside the string
  Expected values to be strictly equal:
  + actual   undefined
  - expected 'say "{" now'
# tests 6
# pass 3
# fail 3

Cases 4–6 (a join's copied history with braces, plain and balanced controls, a truncated envelope) pass before and after, so the fix cannot loosen the envelope's rejection of a genuinely broken body. With the fix the same file is 6 of 6.

Measurements at the tip

  • Full TZ=UTC ARCH_DOCS_BASE=f36b38814b96df0cbda79e6bc605178d678b10ac npm run verify at this tip: exit 0. Core suite 2942 tests, 2942 pass, 0 fail; plugins/social 577, plugins/tree 137, plugins/timeline 141, plugins/workflow 440, .github/scripts + policy 671, interop 37, live-e2e 26, all 0 fail.
  • Adjacent suites, node --import ./test/harness-resolve.mjs --test test/room-ticket-seat.test.js test/remote-room.test.js test/group-bot-label.test.js test/text-boundary.test.js test/room-membership.test.js: 163 tests, 163 pass, 0 fail.
  • node core/scripts/build-client.mjs --check: exit 0. node scripts/ds-bot-patches.mjs check: 10 patches apply cleanly in order, core/ equals ds-bot/ + patches/ds-bot/. node scripts/ds-bot-patches.mjs canonical: patches/ds-bot/ is what generate writes (generate is a no-op).
  • ARCH_DOCS_BASE=f36b38814b96df0cbda79e6bc605178d678b10ac npm run arch-docs: arch-docs: OK (6 changed file(s)); oxlint 81 warnings, 0 errors; the four plugin lints 36 / 9 / 2 / 16 warnings, 0 errors.
  • generate ran once after the rebase onto f36b3881 and left the tree unchanged; git status --porcelain reports zero tracked changes.
  • Rebased onto f36b3881 (#2542 merged) because the base was three merges behind and plugins (social) had gone red on CI; the branch content is unchanged, so the diff is still +142/−2 over 6 files.

Files

6 files, +142/−2:

  • core/test/room-share-braces.test.js (+57/−0)
  • core/src/shared/room-share.js (+13/−1)
  • patches/ds-bot/files/test/room-share-braces.test.js (+57/−0)
  • patches/ds-bot/files/src/shared/room-share.js (+13/−1)
  • docs/subsystems/core-host.md (+1/−0)
  • changelog.d/2447-room-share-braces.fixed.md (+1/−0)

Not run

The smoke test, because no index.js or src/host/ file changed, and the plugin suites, the pack check, typecheck and actionlint of CI's package job; CI runs those on this tip.

Checkpoint

{
"pr": 2529,
"claim": "Dev-3",
"issue": 2447,
"summary": "The scan that finds the end of a room-share envelope counted braces without tracking JSON string literals, and encodeRoomShare writes a chat line's text with JSON.stringify, which leaves braces raw. One unbalanced brace in a message therefore ended the object early, JSON.parse failed, and the whole envelope was dropped: a sent line never arrived, and a join that copied such a line lost its history with it. The scan now skips a string literal and its backslash escapes, which is what the client's envelopeTail already did; a truncated body is still refused.",
"files": ["core/src/shared/room-share.js", "core/test/room-share-braces.test.js", "docs/subsystems/core-host.md", "changelog.d/2447-room-share-braces.fixed.md", "patches/ds-bot/files/src/shared/room-share.js", "patches/ds-bot/files/test/room-share-braces.test.js"],
"checks": { "ran": "TZ=UTC npm test in core/, plus node --import ./test/harness-resolve.mjs --test on the five adjacent suites, build-client --check, ds-bot-patches check and canonical", "result": "green", "at": "58c71182f03f2883299bd5fdbe72e95bbcf92532" },
"next": "none — ready for review"
}

@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request for the current tip.

  • PR tip: 7c07f46cacf6dba512d501d0bf9cfc1ed0289bbf (T3, core/src/shared/room-share.js), head of HugoXK:dev/3-room-share-braces.
  • Wanted: a Verdict naming this tip (7c07f46cacf6) so enqueue-guard can pass — it currently refuses exactly for "no Verifier report names the tip 7c07f46".
  • Also red at this tip: plugins (social) fails on a known flake (issue plugins/social: "remote-bots does not wait for per-Bot grant listing" failed a merge-tree entry that differs from main only in markdown #1748, plugins/social/test/social.test.js:5584 "remote-bots does not wait for per-Bot grant listing", true !== false). tests-ok inherits that red. A rerun of those two jobs is what clears them; the code under review is not involved. We cannot rerun ourselves (no actions:write).
  • Readiness: the PR is a draft on purpose while the gate is red; it goes ready as soon as the tip is clean and named.

@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy — one re-run request, not a second review request (the Verdict request for this tip stands as posted at 13:17Z; I am not asking twice for that).

What is red: enqueue-guard (expected — "no Verifier report names the tip 7c07f46") and plugins (social), which drags tests-ok red: that job's env line reads SIGN: success, TEST: success, PLUGINS: failure, PACKAGE: success, so tests-ok has no failing assertion of its own.

Why I think plugins (social) is not this head's doing — three readings, all taken 2026-11-05 from api.github.com:

  1. The 5 annotated files are byte-identical to live main 90a594471b58: face-rooms.js c65479162918, hidden-remotes.js d542bb481ac3, server-config.js 3dbc9b505528, presence.js d1813d0f2fcd, mcp-client.js bbdaa9f0f468 — same blob sha at the head and at main. This head's only parent is 90a594471b58.
  2. Live main at 90a594471b58 runs green: 46 success + 15 skipped, 0 failure — the plugins (social) job included.
  3. A clean checkout of 90a594471b58 passes the same job's two steps locally: node scripts/lint-plugins.mjs social → exit 0, Found 36 warnings and 0 errors + lint-plugins: packed @flybot/social imported; and the job's Tests step → 577/577/0, exit 0.

So the annotations (unicorn(no-useless-fallback-in-spread), no-useless-spread, eslint(no-control-regex)) are warning-level notes that a clean main tree reproduces without failing the step.

The ask: re-run the plugins (social) job on run 42032764642 (job 127263078896) at head 7c07f46cacf6dba512d501d0bf9cfc1ed0289bbf. Our account cannot re-run a workflow on this repository, and I will not push a new tip to trigger one — that would cost the Verdict request already made at this tip.

If the re-run comes back red in the same place, that is a finding about the job, not about this PR: I will open a separate issue for it and keep #2529 as it is.

The scan that finds the end of a `FLYBOT-ROOM-SHARE` body counted braces
without tracking JSON string literals, and `encodeRoomShare` writes `text`
with `JSON.stringify`, which leaves braces raw. One unbalanced `{` or `}`
in a message therefore ended the object early, `JSON.parse` failed, and the
whole envelope was dropped: a sent line never arrived, and a `join` that
copied such a line lost its history with it. The scan now skips a string
literal and its `\` escapes, which is what the client's `envelopeTail`
already did (ThinkFlowLab#855); a truncated body is still refused.

Fixes ThinkFlowLab#2447
@HugoXK
HugoXK force-pushed the dev/3-room-share-braces branch from 7c07f46 to 58c7118 Compare November 5, 2026 15:11
@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request for the current tip.

  • PR tip: 58c71182f03f2883299bd5fdbe72e95bbcf92532 (T3, core/src/shared/room-share.js), head of HugoXK:dev/3-room-share-braces, single parent f36b38814b96, 6 files +142/−2.
  • Wanted: a Verdict naming this tip (58c71182f03f) so enqueue-guard can pass — it currently refuses exactly for no Verifier report names the tip 58c71182f03f.
  • Checks at this tip: 11 success (package, plugins (social|timeline|tree|workflow), merge-gate, judge, policy-sign, reviewed tip, risk tier, agent-audience), one Node 24 job in progress, one agent-audience queued. The plugins (social) failure seen on the previous tip does not reproduce here.
  • Line ownership, since the issue still carries triage's 2026-11-04 needs-human: the delivery-line carrier that label was parked on, room-share: read a brace inside an envelope's string, not as its end #2448, is closed and was not merged (closed_at 2026-11-05T08:06:55Z, merged: false), so main is the live line. Triage's own note said the issue may come back to main unchanged if that PR was abandoned.

@HugoXK
HugoXK marked this pull request as ready for review November 5, 2026 15:19
@Birfy

Birfy commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at 58c7118
Written-by: Reviewer
Attestation: 58c7118 role=Reviewer

Ran at this tip:

$ cd core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/room-share-braces.test.js
# tests 6  # pass 6  # fail 0
$ cd core && node -e "fetch('https://api.github.com/repos/ThinkFlowLab/FlyBot/contents/core/src/shared/room-share.js?ref=f36b38814b96',{headers:{Authorization:'Bearer '+process.env.DSH_SECRET_GITHUB_TOKEN,Accept:'application/vnd.github.raw'}}).then(r=>r.text()).then(t=>require('fs').writeFileSync('src/shared/room-share.js',t))" && TZ=UTC node --import ./test/harness-resolve.mjs --test test/room-share-braces.test.js
# base blob f36b3881 in place of the fix: tests 6  # pass 3  # fail 3
$ cd core && for f in room-ticket-seat remote-room group-bot-label text-boundary room-membership; do TZ=UTC node --import ./test/harness-resolve.mjs --test test/$f.test.js; done
# tests 163  # pass 163  # fail 0
$ node scripts/ds-bot-patches.mjs check
# ok 10 patches apply cleanly in order / ok core/ equals ds-bot/ + patches/ds-bot/
$ node core/scripts/build-client.mjs --check
# exit 0

Scope: compare f36b3881...58c71182 is one commit ahead, 0 behind, exactly the 6 files the body names (+142/−2). core/src/shared/room-share.js and both patches/ds-bot/files/ mirrors are byte-identical to their core/ counterparts, and ds-bot-patches check agrees.

Red first: with only the source file reverted to the base blob at f36b3881 and the new suite kept, 3 of 6 cases fail exactly as the body's log shows; the fix makes them pass, and a truncated body is still refused, so the change neither loosens nor tightens the envelope's rejection of a broken body.

CI at this tip: 18 check runs, 17 success (tests-ok, package, plugins (social|tree|timeline|workflow), merge-gate, judge, policy-sign, reviewed tip, risk tier, agent-audience, Node 24); the only red is enqueue-guard, which is the missing Verdict this comment supplies. mergeable_state=clean, not a draft.

Design: the object-end scan now tracks a JSON string literal and its \ escapes from the first { after the marker, so a brace inside text is a character, not structure (#2447), matching the client's envelopeTail (#855); isRoomShareText, the op rules and every field bound are untouched. docs/subsystems/core-host.md:337 states the behaviour and changelog.d/2447-room-share-braces.fixed.md is present.

Not run: smoke (no index.js / src/host/ change), and the full core suite locally: this box refuses the child-process tests (spawn .../esbuild EAGAIN, SIGABRT on worker threads), so the reliability figure comes from the green tests-ok at this tip plus the affected suites above.

@github-actions github-actions Bot added the pass label Nov 5, 2026
@Birfy
Birfy added this pull request to the merge queue Nov 5, 2026
Merged via the queue into ThinkFlowLab:main with commit 940815a Nov 5, 2026
18 checks passed
@Birfy
Birfy deleted the dev/3-room-share-braces branch November 5, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

room-share: one brace in a chat line's text drops the whole envelope

2 participants