Skip to content

fix(social): treat an unknown room count as missing, not zero (#823) - #832

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:fix/823-empty-members-count
Oct 23, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:fix/823-empty-members-count

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #823

Tip SHA: bd6a7c2

faceInvitePreview parsed seen.members/seen.maxMembers with Number(), and Number('') is 0, so a host that sent an empty count drew an unknown room as 0 / 8 in the room. A small roomCount helper now counts only a finite number or a trimmed, non-empty string that parses to one; '', ' ' and 'eight' are treated as missing like null, while 0 and '0' still render 0 / 8.

What I ran

  • node --import ./test/resolve.mjs --test --test-name-pattern "#823" test/social.test.js in plugins/social — red before the fix (true !== false on the empty count), green after.
  • node --import ./test/resolve.mjs --test "test/*.test.js" in plugins/social — 362/362 pass.
  • node .github/scripts/changelog.mjs check — 11 fragments well formed.
  • TZ=UTC npm run verify at the repo root — exit 0 (npm cache redirected to a writable directory).

What I did not run

  • The Bot Social host and the web UI by hand.
  • CI (tests-ok) — runs on this pull request.

Docs-not-needed: the invite card's room-count parsing is not an owned path in docs/subsystems/.

`Number('')` is 0, so `faceInvitePreview` drew a room whose count the
host did not send as `0 / 8 in the room`. A trimmed, non-empty value
that parses to a finite number now counts; `''`, `' '` and `'eight'`
are missing like `null`, while a real `0` still renders.

Docs-not-needed: the invite card's room-count parsing is not an owned path in docs/subsystems/.
@Birfy Birfy added agent-pr Pull request opened by an agent area:social Friends, invites, grants, Bot Social area:ui Web/desktop UI and copy labels Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at bd6a7c2
Written-by: Reviewer2

Ran:

  • https://codeload.github.com/ThinkFlowLab/FlyBot/tar.gz/bd6a7c29… extracted, FLYBOT_CORE=<tip>/core DSH_HARNESS=<harness> then node --import ./test/resolve.mjs --test "test/*.test.js" in plugins/social → 362 tests, 362 pass, 0 fail.
  • node --import ./test/resolve.mjs --test --test-name-pattern "#823" test/social.test.js → 1/1 pass.
  • Knife — the two new lines put back to seen.members === undefined || seen.members === null ? NaN : Number(seen.members) (and the same for maxMembers): #823 red, exactly at an empty string is a missing count, not '0 / 8'; restored → 1/1 green, full suite 362/362.
  • node .github/scripts/changelog.mjs check → 11 fragment(s) in changelog.d/ are well formed.
  • risk-gate.mjs over the three changed files → T2 (plugins/social/client.js and its test), so this pull request needs a PASS and is not exempt.
  • Re-read before writing: GET /repos/ThinkFlowLab/FlyBot/pulls/832 → head bd6a7c292a55… (from HugoXK/FlyBot), base 6e4b637f…, mergeable: true; the nine workflow checks are completed/success (run 39928020942 at 08:54:45Z plus merge-gate, risk tier, reviewed tip); no issue comments and no reviews, so this tip carried no conclusion.

What I checked:

  • It is the issue's exact bug and nothing more: Number('') and Number(' ') are 0, and the old guard only excluded undefined/null, so an unknown count drew 0 / 8 in the room. roomCount now keeps a finite number as-is, trims a string and rejects the empty one, and returns NaN for anything that is not a finite number — '', ' ', 'eight', true, [] all fall through to the same "no count" branch null already used.
  • The new case pins both directions, which is what makes it evidence rather than a tautology: three missing shapes must draw no face-link-preview-members line, and 0 both as a number and as the string '0' must still draw 0 / 8 in the room (plus ' 8 ' trimming). Treating everything as missing would red the second half; keeping the old code would red the first.
  • The card's other fallbacks are untouched: both counts finite → N / M in the room, only members finite → N in the room, neither finite → no line. people stays the only thing that decides whether the row exists, so no other field of the preview card changed.
  • No other count path moved: the remaining Number(...) uses in the file are the invite clock offset (clockOffsetMs, deadline math), not a member count; the changelog fragment and the Docs-not-needed: line match the actual footprint (client + test + fragment).

Notes (non-blocking, not merge-gate items):

  • The enqueue-guard check on this head is red with do not enqueue #832: no Verifier report names the tip bd6a7c292a55. That is not a failing test: it is the guard reporting that no verdict exists yet, and .github/workflows/enqueue-guard.yml re-judges the live head on issue_comment, so this comment flips that same check run without a new commit. All nine code checks were already green before it.
  • roomCount accepts every spelling Number does, so a server that sent '0x10' would count 16 and '1e3' would count 1000. Harmless for a count a host computes itself; a /^\d+$/ would be stricter if that ever matters.
  • The helper is local to the preview card. If the same parse is needed for the room-list counts later, it should move to a shared module instead of being copied.

@github-actions github-actions Bot added the pass label Oct 23, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 23, 2026
Merged via the queue into ThinkFlowLab:main with commit c25b7ed Oct 23, 2026
11 of 12 checks passed
@HugoXK

HugoXK commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Written-by: Security

Post-merge security read of head bd6a7c292a554eadd031e4f71df14c6b4d173207 (base 6e4b637f, 3 files, +38/−2; merged at 09:20:55Z by Birfy). Scope: secrets, authz/gate surface, display-name/security-fence rules, and whether this is the real #823 defect rather than a verification PR.

What I checked:

  • The change is only the count parse. faceInvitePreview still builds name/lead through cleanRoomName unchanged (controls, markup, brackets and @ stripped, 60-char cap); the new roomCount supplies only members/maxMembers (plus the pre-existing clockOffsetMs), so the room preview's untrusted-text path — the only model-visible peer text here — is not touched.
  • No injection through the counts. people is only built when each value is Number.isFinite, and it is rendered as a text child (h('div', {…}, preview.people) at plugins/social/client.js:1488), so the string is "<number> / <number> in the room"; no string from the host reaches markup or a log. '', ' ', 'eight', true, [], NaN, Infinity all fall to the same "no count" branch null already used.
  • No secrets, no new authz or gate surface. The diff touches no workflow, script, permission or config file; a pattern scan over the three changed files finds no token/secret reads (only pre-existing --bt-* style comments and the test fixture's mock token name), and there is no logging, error-message or upload path in the helper.
  • Real fix, failing-test evidence included. The new case pins both directions: three missing shapes must draw no face-link-preview-members line, and 0 as a number and '0' as a string must still draw 0 / 8 in the room.
  • Checks at the tip: 11 completed runs success plus one earlier judge run cancelled (superseded, not a failure); enqueue-guard and the code checks are green.

Ran:

  • GET /repos/ThinkFlowLab/FlyBot/pulls/832, /files, /comments, /check-runs — head bd6a7c29…, 3 files, checks as above.
  • codeload.github.com/ThinkFlowLab/FlyBot/tar.gz/bd6a7c29… and …/6e4b637f… extracted; diff -rq → only the expected 3 files differ.
  • In plugins/social at the tip: FLYBOT_CORE=<tip>/core node --import ./test/resolve.mjs --test --test-name-pattern "#823" test/social.test.js → 1/1 pass.
  • Same directory: node --import ./test/resolve.mjs --test "test/*.test.js" → 362 tests, 362 pass, 0 fail.
  • Knife: put the two old Number() lines back (seen.members === undefined || seen.members === null ? NaN : Number(seen.members), same for maxMembers) → #823 fails with an empty string is a missing count, not '0 / 8'; restored the tip file and cmp against the tarball's copy → byte-identical, back to 1/1 green.
  • Secret-pattern grep over the three changed files → no secret read or carrier.

Not run: the Bot Social host and web UI by hand; npm run verify (read the green tests-ok at this head instead); the changelog checker; CI was not re-run by me. No edits, no push, no merge.

Security conclusion: PASS for #823 — no security-surface change, the fence path is untouched, the parse can only yield finite numbers or "missing", and the test is meaningful in both directions. Non-blocking nit: roomCount accepts every spelling Number does, so a host that sent '0x10' renders 16 and '1e3' renders 1000; harmless for a count a host computes itself, but /^\d+$/ would be stricter if that ever matters.

@HugoXK

HugoXK commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Written-by: Test

PASS at bd6a7c292a554eadd031e4f71df14c6b4d173207 (base 6e4b637f69cfa6a967094e1d2be06f8ef3ed3f05). This is a post-merge record: the pull request merged at 2026-10-23T09:20:55Z while this verification was running, so it is evidence for that merged tip, not a request to merge.

Where: fresh checkout of my own at /home/xuke/.dsh/bot/workspace/v832 — git init, git fetch --depth=100 origin refs/pull/832/head:pr832 main, git checkout pr832 (HEAD bd6a7c29), node_modules symlinked from the shared flybot-dev core, npm_config_cache inside my workspace, TMPDIR=/tmp.

Ran: TZ=UTC npm run verify → exit 0: core 1290/1290, plugins/social 362/362, plugins/tree 120/120, plugins/timeline 125/125, .github/scripts/*.test.mjs 288/288, interop 37/37, live-e2e 16/16; core lint 43 warnings / 0 errors; arch-docs: OK (3 changed file(s)); 10 patches apply cleanly in order and core/ equals ds-bot/ + patches/ds-bot/; build-client --check clean.

Ran: node --import ./test/resolve.mjs --test --test-name-pattern "#823" test/social.test.js in plugins/social → 1/1 pass; the whole test/*.test.js → 362/362 pass.

Ran: a reproduction I wrote myself, taking the shipped roomCount helper out of plugins/social/client.js and exercising it directly rather than reimplementing it: '', ' ', 'eight', null, undefined, NaN, Infinity, true, {} → all read as missing; 0, '0', ' 8 ', 8 → all read as counts. For the issue's own input, the pre-fix expression renders 0 / 8 in the room for { members: '' } while the fixed helper renders no count line — the defect and the fix, same input, side by side.

Ran, knife: git checkout 6e4b637f -- plugins/social/client.js in a copy → the #823 case is red (0/1); restored → 1/1.

Not run: no Bot Social host and no browser (the card is exercised through the dialog harness and the helper directly, not a real render engine); no screenshot of the invite card; nothing about the invite endpoints themselves (unchanged by this diff).

Written-by: Test

@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:social Friends, invites, grants, Bot Social area:ui Web/desktop UI and copy pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Number('') makes an empty members/maxMembers render as 0 / 8 in the room

2 participants