jason.woltje
158205c5cc
Unclaimed files left in the canonical checkout. Inventory: canonical-cleanup-inventory.md (commit-as-evidence groups). detect-secrets scan clean on 2026-11-02: hits are sha256 pins, git object ids, test sentinels already in tracked tests and unit-file lines.
172 lines
9.9 KiB
Markdown
172 lines
9.9 KiB
Markdown
# Row 182: a tester message reaches only a tester-owned thread
|
|
|
|
Brief: "# Brief: a tester message reaches only a tester-owned thread (N1)" in
|
|
`docs/plans/2026-10-20_tester-thread-follow-ups.md` (blob 3b4313cb9ffa, 841c56d05). Gitea #1680. Owner Darkwing,
|
|
reviewer Filbert, gate owner Sage. The follow-up is Filbert's N1 on row 177 (#1675 comment 29347).
|
|
|
|
## Files
|
|
|
|
| File | What |
|
|
|---|---|
|
|
| `build.patch` | the change, `git diff 841c56d05` over the paths below |
|
|
| `candidate-manifest.sha256` | one line per changed path |
|
|
|
|
- Base: 841c56d05 (refactor head at the start, after 931651f2d).
|
|
- Line numbers are at the base unless marked as the candidate's.
|
|
- Changed: `packages/bus/src/broker.mjs`, `packages/bus/tests/tester-threads.test.mjs`, `packages/bus/README.md`,
|
|
and `packages/bus/tests/messaging.test.mjs`. The last one is outside the brief's list (next section).
|
|
|
|
### `messaging.test.mjs` is outside the brief's list
|
|
|
|
The first slot run failed one test, "a business with no role definitions at all is unlimited"
|
|
(messaging.test.mjs:203). Its tester leg was:
|
|
|
|
```js
|
|
const opened = send('assistant', 'pm');
|
|
for (const from of ['pm', 'tester']) send(from, 'assistant', { in_reply_to: opened.id });
|
|
assert.deepEqual(refusals(), []);
|
|
```
|
|
|
|
The assistant there runs on the human's thread, so the tester's reply reached a human thread through
|
|
`in_reply_to`. That is the hole this row closes, and the send now refuses `thread-not-owned`. The row 182 rule
|
|
moves the tester send off the human's thread. Per sage-1212 the leg keeps a passing tester send, because the
|
|
test's point is that a business with no definitions limits no one. The tester makes its own project and
|
|
thread, an assistant run claims that thread and sends to the pm, and the tester replies to that message.
|
|
`refusals()` is still `[]`. The new refusal is covered by tests 1 and 4 below.
|
|
|
|
No cli, webui or script test sends a tester reply into a threaded instance. The tester clients send no
|
|
`in_reply_to` (`packages/cli/src`, `packages/webui/src`).
|
|
|
|
No test pins a hash of the four paths (`git grep` over `packages/*/tests` and `scripts/test-*.sh`). The one
|
|
test that reads a changed file as text is `limit-checks.test.mjs:323`, a pattern scan of `broker.mjs` for
|
|
`limit-check/` and `token-limit`. It is in the bus suite and passed in run 1.
|
|
|
|
Correction (Filbert's N2, #1680 comment 29388): the line above is wrong about "the one test".
|
|
`project-runtime.test.mjs:13` also reads `broker.mjs` as text. It pins the file's `import` lines. My grep
|
|
matched it, but I cut the output with `head` before that line. The candidate adds no import, and the test is
|
|
in the bus suite, which passed in both runs.
|
|
|
|
## Design (written before the change, per sage-1201)
|
|
|
|
### Where a tester's message gets its thread
|
|
|
|
A tester `message.send` (broker.mjs:1823, tester branch :1834-1838) checks `in_reply_to` only for existence in
|
|
the business (:1879-1884). It then calls `#threadFor(s, to, in_reply_to)` (:323). For a threaded instance,
|
|
`#threadFor` walks the `in_reply_to` chain and takes the first thread it reaches, by one of two branches:
|
|
|
|
1. A message on the chain is in a thread (:333-336). The check there is `thread-instance-mismatch` only.
|
|
2. Row 143: a message on the chain is a spec notice from `_bus` (:339-343). `#specThread` (:2630) returns the
|
|
newest opened thread in the notice's project whose owner is the business's human (`t.owner=?` bound to
|
|
`this.#business(s.business).human`, :2637).
|
|
|
|
Neither branch checks the owner against the sender. With the shipped assistant lists, `#checkMessaging`
|
|
(:1859) refuses the tester first (`message-sender-refused`), so only an assistant definition with no lists
|
|
reaches either branch.
|
|
|
|
### Can the spec-notice branch return a thread the tester doesn't own?
|
|
|
|
Yes. `#specThread` returns only threads owned by the human, never `tester`, so for a tester sender every
|
|
thread it returns is one the tester doesn't own. The chain reaches it when the tester replies to a spec
|
|
notice, or to any message whose chain reaches one. `in_reply_to` takes any message id in the business,
|
|
including a notice's. So the spec branch gets the same check.
|
|
|
|
### The check
|
|
|
|
One check after the walk picks a thread, on both branches: if the sender is the tester (`s.tester`) and the
|
|
thread's `owner` isn't `tester`, refuse. Comparing with `this.#owner(s)` (:358, `'tester'` for a tester) keeps
|
|
the rule in the place row 177 put it.
|
|
|
|
It applies only to `s.tester`. The brief keeps the reverse direction:
|
|
- A role's reply up a chain into a tester-owned thread is how the chief's answer comes back (test 3).
|
|
- A human sender refuses earlier, at :326 (`thread-required-use-thread-send`).
|
|
- A thread run returns its own thread at :324 before the walk.
|
|
|
|
### The refusal code: `thread-not-owned`
|
|
|
|
I reuse the existing code rather than add one.
|
|
- It names the same rule. REQ-USER-2 at `#thread` (:353) refuses `thread-not-owned` when a session reaches a
|
|
thread another owner made. Row 177 extended it to the tester (`#owner`, :358), so the tester's
|
|
`thread.open`, `thread.send` and `thread.messages` on a human thread already answer `thread-not-owned`
|
|
(tester-threads.test.mjs:84). A tester message whose chain lands in a human thread breaks the same rule.
|
|
- Every client already maps it. The webui tester listener answers 403 with "This thread is not available to
|
|
this user." (`packages/webui/src/chat-http.mjs:15,23`). The cli prints the code
|
|
(`packages/cli/src/thread.mjs:60`). A new code would need a new mapping in each, or fall through as an
|
|
unmapped error.
|
|
- The cost is that the tester didn't name the thread here; the chain did. The README paragraph says so, so a
|
|
tester who sees the code on a `message.send` knows to look at `in_reply_to`.
|
|
|
|
Rejected alternatives:
|
|
- A new code such as `tester-thread-not-owned`. That is more specific, but it needs a mapping in each client
|
|
and adds a second name for one rule.
|
|
- Skip the branch and walk on past a thread the tester doesn't own. The tester's message would then fall
|
|
through to `thread-required-reply-to-thread-message` or land in a thread further up the chain, and the
|
|
test would get a misleading answer.
|
|
|
|
## Tests
|
|
|
|
The tests go in `packages/bus/tests/tester-threads.test.mjs` (row 177's file). All use a business whose
|
|
assistant definition has no lists (`messaging` null), so `#checkMessaging` lets the tester through.
|
|
|
|
1. The tester sends to the assistant in reply to a message in the human's thread H. The send refuses
|
|
`thread-not-owned`. It writes no `messages` row and no `message_threads` row, and the human's
|
|
`thread.messages` on H reads the same before and after.
|
|
2. The same tester send in reply to a message in the tester's thread T lands in T.
|
|
3. The round trip: the tester's `thread.send` in T, the T run's message to the chief, the chief's reply to the
|
|
assistant. The chief's reply lands in T, and the T run receives it.
|
|
4. The spec branch: the tester replies to a spec notice in the project of a human thread. It refuses
|
|
`thread-not-owned` and writes nothing. The test writes the notice row the way `#specNotice` (:2588) does,
|
|
since the save path needs the git project fixture of `spec-review.test.mjs`, and that fixture can't turn on
|
|
`bus.tester` (the broker takes a `structuredClone` of the businesses, :208). On the same chain, the
|
|
chief's relay to the assistant lands in the human's thread, which shows that the fixture notice reaches the
|
|
branch.
|
|
|
|
Test 1 runs a holder on H. Without the check, the tester's message would be stored in H and delivered to
|
|
H's run, not refused `tester-target-not-running`. Test 3 is row 177's round trip with the chief added as a
|
|
plain run. The assistant run in T sends to the chief, and the chief's reply to that message lands in T and
|
|
reaches the T run.
|
|
|
|
Mutant: drop the owner check. Tests 1 and 4 should kill it.
|
|
|
|
## Runs
|
|
|
|
`run182.sh` runs the whole bus suite (`packages/bus`, `node --test tests/*.test.mjs`) and then three
|
|
mutants against `tester-threads.test.mjs` with `--test-name-pattern='row 182'`. Each mutant edits
|
|
`broker.mjs` after a checked backup and puts it back by the backup's sha. TMPDIR and output are in my scratch.
|
|
|
|
Run 1, slot from sage-1209, 2026-10-20T05:09:08.203Z to 05:09:26.237Z:
|
|
|
|
| Run | Result |
|
|
|---|---|
|
|
| bus suite | 451 tests, 450 pass, 1 fail (messaging.test.mjs:203, above) |
|
|
| m1: the `#testerOwns` check line removed | killed |
|
|
| m2: the thread branch returns `m.thread` unchecked | killed |
|
|
| m3: the spec branch returns `spec` unchecked | killed |
|
|
|
|
Run 2, after the messaging.test.mjs change, slot from sage-1215, 2026-10-20T05:24:47.741Z to 05:25:04.691Z:
|
|
|
|
| Run | Result | Failing tests |
|
|
|---|---|---|
|
|
| bus suite | 451 tests, 451 pass, 0 fail | none |
|
|
| m1: the `#testerOwns` check line removed | killed | T9 tests 1 and 4 |
|
|
| m2: the thread branch returns `m.thread` unchecked | killed | test 1 |
|
|
| m3: the spec branch returns `spec` unchecked | killed | test 4 |
|
|
|
|
`broker.mjs` was back at its candidate sha after the mutants. Tests 2 and 3 pass under every mutant, since
|
|
they check that the tester's own thread and the chief's reply still land.
|
|
|
|
## Candidate
|
|
|
|
- Base 841c56d05. Nothing in `841c56d05..origin/refactor` (d1272e714 at 05:25Z) touches the four paths, and
|
|
`build.patch` applies cleanly to both 841c56d05 and d1272e714 (`git apply --cached --check` on a scratch
|
|
index).
|
|
- `build.patch`: sha256 c050948ebdf6e8d6e075b897bf21c7a7cfbffef699ca15e54a45a02a3e1a16f5, patch-id (stable)
|
|
fa9a8988ebfcbe3cb84beeff9d74dead8ea002e1, 4 files, 88 insertions, 6 deletions.
|
|
- `candidate-manifest.sha256`: sha256 9b5cc63c37f57a7a9954d6d3ecd1c5ee23a014b003c653d5ff7c0e055d57e866.
|
|
|
|
| Path | sha256 |
|
|
|---|---|
|
|
| `packages/bus/src/broker.mjs` | 01b5ab3a175f9488b31554a44abeba9b455dedbc3d68cb2dc8147a3fdcc58976 |
|
|
| `packages/bus/tests/tester-threads.test.mjs` | c09a0798cbc297aa5c0bf52bffafa278582c9be6815737d2d4cace56dd7d618f |
|
|
| `packages/bus/README.md` | 4377cc32aed37fa1bb645c0bbc3340eed4ec01740e67327449949474e35d3fdb |
|
|
| `packages/bus/tests/messaging.test.mjs` | c0767449e3f50d74b0ce193fdd2ffeaf123e486d49e03cdf7fddbaec0fb87de7 |
|