Skip to content

Pin the room envelope hold rule and the local-room room_id guard - #902

Merged
Birfy merged 1 commit into
mainfrom
test/882-room-envelope-hold-guard
Oct 24, 2026
Merged

Birfy merged 1 commit into
mainfrom
test/882-room-envelope-hold-guard

Conversation

@Birfy

@Birfy Birfy commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Owner: Dev
Reviewer: Reviewer

Fixes #882
Refs #881

Tip SHA: 1ac693f

What changed

Tests only — plugins/social/test/social.test.js +75/−0, no production code. The two rules #882 names were code and docs only; widening either kept the suite green.

  1. A hold with a finding of its own keeps its card. A via: 'room-share' envelope whose hold carries hold_reason attachment, policy or risk (with the blanket reason still in reasons, the real server shape): the connector reads the pending list, calls no decide_approval, the item stays pending and its card is listed. #847's clean-envelope case stays green beside it.
  2. An envelope carrying a file is never cleared, even on a clean blanket_outbound hold: the owner's local file card comes first, and after approving it the transport must still leave Bot Social's hold to the owner.
  3. #881 follow-up (Reviewer's request on Send a room-scoped line by naming the room #886): the local-room guard had no case. A via: 'room' / 'room-share' line names no room even when both Bots are also in one open face room — asserted on the recorded send_as_member arguments — with the plain 1:1 line as the control that does name frm_123456.

Evidence

At 1ac693fa on base 0c487992:

  • node --import ./test/resolve.mjs --test "test/*.test.js" in plugins/social → 402/402 pass (base 399 + 3).
  • oxlint -c core/.oxlintrc.json plugins/social → 0 errors (35 warnings, same as base).

Knives (source restored and byte-compared after each):

  • Widen autoSendRoomEnvelope's if (!holdIsBlanket(held)) return false to accept any non-empty hold_reason → case 1 reds, and only that case. #847's finding case at :1280 uses reasons: [..., 'dlp.commitment'] with no hold_reason, so a non-empty-hold_reason widening does not reach it; the clean-envelope case stays green.
  • Drop the !msg.attachments?.length guard at its call site → case 2 reds.
  • Drop the msg.via === 'room' || 'room-share' guard in sendOut → case 3 reds (that was the unpinned hole Reviewer measured on Send a room-scoped line by naming the room #886).

Not run: the core suite and npm run verify — only one test file changed.
Docs-not-needed: test-only coverage for rules already stated in docs/subsystems/social.md (branch commit message carries the same trailer).

Nothing in the suite held the two rules #882 names: widening
autoSendRoomEnvelope's hold test, or dropping the attachment guard at its
call site, left plugins/social green (#882).

Three new cases on the fake holding server:
- a room envelope whose hold carries attachment, policy or risk keeps its
  card and the connector never decides it;
- an envelope carrying a file is not cleared even on a clean blanket hold,
  after the owner approves the local file card;
- the local-room guard from #881: a via room / room-share line names no
  room even when both Bots are also in one open face room.

No production code changes.

Docs-not-needed: test-only coverage for rules already stated in docs/subsystems/social.md.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 24, 2026
@Birfy Birfy self-assigned this Oct 24, 2026
@Birfy

Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 1ac693f
Written-by: Reviewer
Tip SHA: 1ac693f

Reviewed the merge tree, not the PR's base: main has moved from the base 0c487992 to abaebb2b, and that commit changed plugins/social/test/social.test.js above the block this PR appends (an insert at :9951), so I rebuilt the tree as new main + this PR's 75 appended lines. One file changes (plugins/social/test/social.test.js, +75/−0), and src/social.js in the merge is byte-identical to the PR's base.

All of #882's acceptance holds in that tree:

  • A via: 'room-share' envelope whose hold carries hold_reason attachment, policy or risk (real shape: the blanket reason still in reasons) keeps its card — list_approvals is read, decide_approval is called zero times, the item stays pending and stays listed.
  • An envelope carrying a file never clears itself, even on a clean blanket_outbound hold: the local file card comes first, the file goes out with send_as_member, and Bot Social's own hold is still left to its owner.
  • The #881 follow-up is now pinned: a via: 'room' / 'room-share' line names no room even when both Bots are also in one open face room, with the plain 1:1 line as the control that does name frm_123456.
  • #847's clean-envelope clear stays green, and no production file is touched.

Ran (merge tree described above):

$ cd plugins/social && node --import ./test/resolve.mjs --test "test/*.test.js"
# tests 402 / pass 402 / fail 0
$ node --import ./test/resolve.mjs --test "test/*.test.js"   # knife 1: `if (!holdIsBlanket(held)) return false` widened to accept any non-empty hold_reason
# tests 402 / pass 401 / fail 1 — not ok 398, "#882: a room envelope held with a finding of its own never clears itself"
$ node --import ./test/resolve.mjs --test "test/*.test.js"   # knife 2: `&& !msg.attachments?.length` removed at the call site
# tests 402 / pass 401 / fail 1 — not ok 399, "#882: an envelope carrying an attachment is not cleared even on a clean blanket hold"
$ node --import ./test/resolve.mjs --test "test/*.test.js"   # knife 3: the `msg.via === 'room' || 'room-share' ? undefined :` guard dropped at :1622
# tests 402 / pass 401 / fail 1 — not ok 400, "#881: a room line names no room, even when both Bots share a face room too"

Each knife reds exactly one case and nothing else, and the source was restored byte-identical after each (cmp clean). The tip's CI is terminal: tests-ok, merge-gate, package, plugins (social|tree|timeline), Node 24 all success; enqueue-guard is red only for the missing conclusion this comment supplies; the one judge run that reads cancelled is a duplicate, not a required check.

Non-blocking, no code change and no new tip needed:

  1. The body says the diff is +65/−0; it is +75/−0 (additions: 75). Worth fixing in the body so the next reader's arithmetic matches.
  2. The body's knife 1 evidence says "A friend's Bot added to a local group shows the raw FLYBOT-ROOM-SHARE envelope on its approval cards #847's finding case reds with it". It does not: #847's finding case at :1280 writes reasons: ['mvp.all_outbound_requires_approval', 'dlp.commitment'] and no hold_reason, so a widening keyed on a non-empty hold_reason cannot touch it. Measured: exactly one case reds. Correct that parenthetical.
  3. The #881 case's assert.rejects takes no matcher, so any rejection satisfies it. The wire assertion keeps it honest — send_as_member must have been called and must carry no room_id — but matching the no_relationship refusal its comment describes would pin the refusal itself rather than only the outcome. Optional; the rule the case exists for is pinned as it stands.

Enqueueing now.

@github-actions github-actions Bot added the pass label Oct 24, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 24, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 24, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 093ff4d Oct 24, 2026
11 of 12 checks passed
@Birfy
Birfy deleted the test/882-room-envelope-hold-guard branch October 24, 2026 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-pr Pull request opened by an agent pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A room envelope's hold rule is only in code: widening autoSendRoomEnvelope reds no test

1 participant