M4b review, and follow-up tasks 16 to 18
The review of tasks 01 to 15 goes in the implementer log, with the device check against the owner's Mattermost. Three follow-ups: - 16: the refusal without a runner says why. - 17: the texts that say who asked. - 18: approvals from non-approvers in direct or group messages go to an approver. Task 14's given serve_approvals.rs becomes serve_approvals.rs-task14, since task 18 hands over a new version. Tip T31 is added. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
@@ -61,13 +61,11 @@ How it is used:
|
||||
| T24 | A task that changes a `Cargo.toml` must stage `Cargo.lock` in its `git add` line. Better, put `Cargo.lock` in every task's `git add`; it is a no-op when unchanged. | M3b task 04: committed correctly, left the lock behind, and the driver stopped on an unclean tree. |
|
||||
| T25 | Size a task by the largest function the model must hold in one turn, not by the task. Ornith writes one function with a few branches well; a function with half a dozen branches and threads (M3b's `run`) it plans in its head until the turn runs out, with nothing written. Give such a task a compiling skeleton with the big function already written as glue over small `todo!()` helpers, and say to fill one at a time with `cargo check` between. | M3b task 11: four sessions. Two wrote nothing; a whole-file skeleton got five of six functions; the finer skeleton finished it in ten minutes, and tasks 12 and 13 followed without a stop. M4a task 08: again, a skeleton whose `header` was one `todo!()` with a dozen branches; nothing written. Size every `todo!()`, not only the task: the largest comment above a `todo!()` is a fair proxy, and one over about six lines needs splitting. M4a task 14: a task with a dozen small `todo!()`s still stalled, because the model read the whole crate to learn the APIs first; give a table of every call the task makes, with its signature, and make copying and the failing test the first two actions. |
|
||||
| T26 | Replay each task's end state on its own, **and** read the task file against the reference for anything the reference has that the task does not ask for. The replay proves the tests can pass; only the reading finds a field the reference reads through a getter the task never mentions. | M3b task 11: the task stored `egress_dir` without the reference's getter, so the field was never read and clippy failed; the replay passed because the reference had the getter. |
|
||||
|
||||
| T27 | Every wait in a given test has a limit (`recv_timeout`, a deadline loop, `is_finished` before `join`). A test that waits forever on a broken implementation hangs the driver instead of failing, and the implementer cannot tell a hang from slowness. | M4a planning: breaking the reference's ping made `ws_conn` hang on `rx.recv()`; retrying a refused token made the serve test hang on `join()`. Both now fail within 5 s. M4b planning: a failed assertion inside `std::thread::scope` hangs instead of failing, because the scope joins its threads first; give such a test a guard that stops the thread on drop, and wait on `is_finished()` with a deadline instead of `join()`. |
|
||||
| T28 | Before hand-over, break the reference on purpose, one line at a time (drop a check, move a bound by one, skip a save), and run the given tests against each change. A change the tests still pass is a missing test, unless it cannot change behaviour. Rustfmt reflows lines: match on text that survives formatting. | M4a planning: 69 changes over six modules; the tests missed 8. Five were real gaps and got tests (a reconnect that re-sent "interrupted" for a running turn was one), two could not change behaviour, and one was left (an event frame with another id, which `loopd` never sends). |
|
||||
|
||||
| T29 | Check a skeleton's comments, not only its code. Print every `todo!()` beside its signature and read each pair before hand-over. Filling the bodies from your own code proves the split can pass; it cannot show that a comment sits on the wrong function. Edit comments by function, never by "the next comment after this name". And tell the implementer to stop and quote a comment that does not fit. | M4a task 14, second session: `post`'s comment was `now_ms`'s text, put there by a script; Ornith saw the contradiction and deliberated over it until the turn ran out. M4a task 16: a comment naming the function it replaced made the task's own `grep` fail; and a given test copied into the plan before `cargo fmt` came back reformatted. Run the task's checks, and `cargo fmt`, on the end state before hand-over. |
|
||||
|
||||
| T30 | Every behaviour a task writes must be reachable by that task's own tests. When a function's effect shows only after a later task (here: a refusal's reason, which only task 15 could send), give a unit test in the first task, or write that function in the later one. And never let one name mean two things in a comment: say which `reason`. | M4b task 14 dropped a refusal's reason, reading `Refused -> Ended::Refused { by, reason }` as the `DenyReason` in scope; its tests passed, and task 15's caught it, in a file task 15 could not touch. |
|
||||
| T31 | In a spec, for every post or message the harness sends, name who can read it and whether that reader can act on it. Walk each kind of sender through each kind of channel (the owner, an allowed non-approver, a stranger; direct, group, channel). | M4b: an approval asked by a non-approver in a direct message went into that direct message, where no approver could see it. Every test passed, because the tests used the spec's cases; the device check with a second account found it (review finding 1, fixed by tasks 17 and 18). |
|
||||
|
||||
## What worked and should be kept
|
||||
|
||||
|
||||
@@ -600,3 +600,48 @@ The one deviation, reported, was the plan's fault: step 6 of the comment said "f
|
||||
`check_file`", which made step 5's `grep` print a line the task said it would not. The given test
|
||||
also differed from the plan's copy, in rustfmt's layout only: the design model copied it into the
|
||||
plan before formatting it. The plan's copy is now the formatted file.
|
||||
|
||||
### M4b, tasks 01 to 15 — reviewed 2026-09-24 by the design model (Claude)
|
||||
|
||||
Accepted, with three follow-up tasks (16 to 18). All work by Ornith-1.5-35B-A3B through
|
||||
`tools/run-plan.sh`. The code does what the spec says and is equivalent to the reference; against
|
||||
the owner's Mattermost, every claim held. One gap is in the spec, not the code (finding 1).
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| 15 task commits; 1 plan commit by the design model during the run (`af7137d`) | pass; tasks 05 and 08 lack the `Implemented-By` trailer (finding 3) |
|
||||
| All given tests, fixtures and the runbook entry identical to the plan | pass |
|
||||
| Protected files (`AGENTS.md`, `CLAUDE.md`, `docs/design.md`, specs, `Makefile`, `scripts/`, `deny.toml`) | untouched |
|
||||
| `make gate` | `gate: ok`, 845 tests, the count the plan gave (the gate log counts 846: a different count) |
|
||||
| Banned constructs in new library code | none: no `unwrap`, `expect`, `panic!`, `#[allow]`, `unsafe` or bare `as` casts |
|
||||
|
||||
**Against the owner's server** (2026-09-24; this branch's binaries on the owner's machine,
|
||||
`inferproxy`, `brokerd` with `[gateway]` and an `ask` grant but no `[runner]`, `loopd` against
|
||||
Ornith slot 0 on straylight, `gatewayd` against the owner's Mattermost; the owner sent the
|
||||
messages):
|
||||
|
||||
| Claim | Seen |
|
||||
|---|---|
|
||||
| An `ask` call is posted in the thread that asked for it | yes, with the arguments fenced and the grant named; the owner saw the bot typing while the turn ran (the pause while it waits is covered by `serve_answers.rs`) |
|
||||
| `!approve <id>` from an approver in the thread approves it, as `bxctl` would | approvals 0 and 6 approved; the audit log names the approver (`by`) and the answering post (`post`); `bxctl audit verify` ok |
|
||||
| `!deny <id> <reason>` refuses it, with the reason | approval 4 denied "not now"; the post says so |
|
||||
| Nobody else, and nothing else, answers it | a non-approver's `!approve` refused with the reason; `!approbe` answered "unknown command" |
|
||||
| An approval answered elsewhere is marked | `bxctl deny 3`: the post became "No longer pending" |
|
||||
| A restart marks what it left behind and reposts | `gatewayd` restarted with approval 9 pending: its post marked; `brokerd`'s courier sent it again, and it went to the owner's direct messages because the session was no longer followed, as specified |
|
||||
| The bot can edit its own posts and look up users | yes |
|
||||
| No post left looking answerable | yes; Mattermost's edit history shows up as deleted copies in `posts?since` (`original_id`, `delete_at`), which `gatewayd` already skips |
|
||||
|
||||
| # | Severity | Owner | Finding | Fix |
|
||||
|---|---|---|---|---|
|
||||
| 1 | medium | spec | An approval asked by a non-approver in a direct message went into that direct message, where no approver can see it: only `bxctl` could answer it. The same in a group message the approver is not in. The owner's requirement for the fix: the approver must see who asked, their message, and what Boxmaker is trying to do. | Spec changed (`d2e8141`, section 6); tasks 17 and 18 |
|
||||
| 2 | low | code (M3a) | Without `[runner]`, every allowed call was refused with "the runner arrives in M3b", which is stale since M3b merged, and the model repeated it to the owner. | Task 16 |
|
||||
| 3 | nit | implementer (05, 08) | Two task commits lack the `Implemented-By` trailer that `AGENTS.md` asks for. The log rows are there. | None needed; noted for the lessons |
|
||||
| 4 | low | plan (14, 15) | Task 14's `ended` had two values both called `reason` and a comment that did not say which went where; task 14's tests could not send a refusal. The refusal's text was dropped, and task 15's given test found it. | Task 15 (`af7137d`); tip T30 |
|
||||
|
||||
What was good: 14 of 15 tasks finished on their first session. The courier, the registry's six
|
||||
checks and the text rendering follow their comments exactly. When task 15 found the cause of its
|
||||
failure in a file it was not given, it stopped and said so instead of editing that file, which was
|
||||
right.
|
||||
|
||||
**The run.** Task 15's first session stopped on finding 4. After the listed fix, task 15 finished
|
||||
on its first session.
|
||||
|
||||
@@ -75,7 +75,7 @@ both first). To fill, each with its steps in its comment: `place`, `ended`, `mar
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.** `git switch m4b`, then
|
||||
`cp docs/plans/M4b/files/crates/gatewayd/src/serve/approvals.rs crates/gatewayd/src/serve/ && cp docs/plans/M4b/files/crates/gatewayd/tests/serve_approvals.rs crates/gatewayd/tests/ && cp docs/plans/M4b/files/crates/gatewayd/tests/support/brokerd_client.rs docs/plans/M4b/files/crates/gatewayd/tests/support/fake_mm.rs crates/gatewayd/tests/support/`
|
||||
`cp docs/plans/M4b/files/crates/gatewayd/src/serve/approvals.rs crates/gatewayd/src/serve/ && cp docs/plans/M4b/files/crates/gatewayd/tests/serve_approvals.rs-task14 crates/gatewayd/tests/serve_approvals.rs && cp docs/plans/M4b/files/crates/gatewayd/tests/support/brokerd_client.rs docs/plans/M4b/files/crates/gatewayd/tests/support/fake_mm.rs crates/gatewayd/tests/support/`
|
||||
- [ ] **2. Make the six `serve/mod.rs` changes.** `cargo build -p gatewayd` must succeed.
|
||||
- [ ] **3. See it fail.** `cargo test -p gatewayd --test serve_approvals`. Expected: 4 fail, 1
|
||||
passes (the socket is there while `gatewayd` runs).
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
# M4b task 16: `brokerd`'s refusal without a runner says why
|
||||
|
||||
**Branch:** `m4b` (run `git switch m4b`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `brokerd: the refusal without a runner names the missing [runner] section`
|
||||
|
||||
## Goal
|
||||
|
||||
When `brokerd.toml` has no `[runner]` section, every allowed call is answered with the fixed
|
||||
sentence `REFUSING`. It still says `the runner arrives in M3b`, which stopped being true when M3b
|
||||
merged: the model, and the owner reading its answer, are told to wait for something that has
|
||||
already happened. Found in the M4b device check (review of 2026-09-24, finding 2). The sentence
|
||||
becomes:
|
||||
|
||||
```text
|
||||
no tool runner is configured: brokerd.toml has no [runner] section
|
||||
```
|
||||
|
||||
It is still a fixed sentence (it reaches the model), so nothing else changes.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy: a **new version** of the given `crates/brokerd/tests/runner.rs` (one assertion: the new
|
||||
sentence)
|
||||
- Modify: `crates/brokerd/src/runner.rs`, `docs/implementer-log.md`
|
||||
- Do **not** change `crates/loopd/tests/broker_port.rs`: it has the old sentence as test data
|
||||
only, and `loopd` never checks it.
|
||||
|
||||
## The change, exactly
|
||||
|
||||
In `crates/brokerd/src/runner.rs`, the line
|
||||
|
||||
```rust
|
||||
pub const REFUSING: &str = "the runner arrives in M3b";
|
||||
```
|
||||
|
||||
becomes
|
||||
|
||||
```rust
|
||||
pub const REFUSING: &str = "no tool runner is configured: brokerd.toml has no [runner] section";
|
||||
```
|
||||
|
||||
The doc comment above it stays as it is.
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.** `git switch m4b`, then
|
||||
`cp docs/plans/M4b/files/crates/brokerd/tests/runner.rs crates/brokerd/tests/`
|
||||
- [ ] **2. See it fail.** `cargo test -p brokerd --test runner`. Expected: one test fails,
|
||||
`the_production_runtime_refuses_every_call`, on the sentence.
|
||||
- [ ] **3. Make the change.** Run `cargo fmt --all`.
|
||||
- [ ] **4. See it pass.** `cargo test -p brokerd --test runner`: every test passes. Then
|
||||
`cargo test -p brokerd`: every suite passes.
|
||||
- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`, with the same number of
|
||||
tests as before this task (about 846).
|
||||
- [ ] **6. Log and commit.** `git add crates/brokerd docs/implementer-log.md Cargo.lock && git commit`
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo test -p brokerd` passes; `make gate` prints `gate: ok`.
|
||||
|
||||
## Stop and report if
|
||||
|
||||
- Any other test fails because of the sentence. Do not change that test.
|
||||
@@ -0,0 +1,115 @@
|
||||
# M4b task 17: the texts of a redirected approval
|
||||
|
||||
**Branch:** `m4b` (run `git switch m4b`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `gatewayd: the texts that say who asked for a redirected approval`
|
||||
|
||||
## Goal
|
||||
|
||||
Texts only; task 18 posts them. When someone who may talk to the bot but not approve asks for
|
||||
something in a direct or group message, no approver can answer the approval there, so it goes to
|
||||
the first approver's direct messages (M4b spec, section 6, as changed after the review of
|
||||
2026-09-24). The approver must see who asked, where, and what they asked. Three texts:
|
||||
|
||||
1. `asked_post`: the first post of a redirected approval. The asker's message is escaped with
|
||||
`proto::escape::escape_model_text` and fenced (`fence`, already in this file). It is context,
|
||||
not what is approved, so only its first `PROMPT_SHOWN` (2,000) characters are shown, then a
|
||||
line saying how many more there were. For example:
|
||||
|
||||
````text
|
||||
**Approval 3** was asked by @alex in a direct message with the bot. Their message:
|
||||
```
|
||||
Read the file /srv/notes/q3.txt and summarise it.
|
||||
```
|
||||
````
|
||||
|
||||
2. `unfollowed_post`: the first post of an approval whose session `gatewayd` does not follow
|
||||
(`bxctl chat`, or a turn from before a restart).
|
||||
3. `sent_to_approver`: the line left in the asking thread.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy: `crates/gatewayd/tests/approvals_asked.rs`
|
||||
- Modify: `crates/gatewayd/src/approvals/text.rs` (append only), `docs/implementer-log.md`
|
||||
|
||||
## The change, exactly
|
||||
|
||||
Append this to the end of `crates/gatewayd/src/approvals/text.rs`, after `with_ending`, exactly
|
||||
as it is. Nothing else in the file changes, and there are no new imports: `proto` is already a
|
||||
dependency, and `code` and `fence` are in this file.
|
||||
|
||||
```rust
|
||||
|
||||
/// The most characters of an asker's message shown to the approver: it is context, not what is
|
||||
/// approved, so it may be cut.
|
||||
pub const PROMPT_SHOWN: usize = 2_000;
|
||||
|
||||
/// Who asked for an approval that is shown elsewhere (M4b spec, section 6).
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct Asked {
|
||||
/// `@username`, or the user id when the name could not be found.
|
||||
pub by: String,
|
||||
/// The channel's type: `D` a direct message, `G` a group message.
|
||||
pub channel_type: String,
|
||||
/// The text of the turn that asked.
|
||||
pub prompt: String,
|
||||
}
|
||||
|
||||
/// The first post of a redirected approval: who asked, where, and their message.
|
||||
pub fn asked_post(id: u64, asked: &Asked) -> String {
|
||||
let place = if asked.channel_type == "G" {
|
||||
"a group message"
|
||||
} else {
|
||||
"a direct message"
|
||||
};
|
||||
let escaped = proto::escape::escape_model_text(&asked.prompt);
|
||||
let total = escaped.chars().count();
|
||||
let shown: String = escaped.chars().take(PROMPT_SHOWN).collect();
|
||||
let mut post = format!(
|
||||
"**Approval {id}** was asked by {} in {place} with the bot. Their message:\n{}",
|
||||
asked.by,
|
||||
fence(&shown)
|
||||
);
|
||||
if total > PROMPT_SHOWN {
|
||||
post.push_str(&format!("\n(… {} more characters)", total - PROMPT_SHOWN));
|
||||
}
|
||||
post
|
||||
}
|
||||
|
||||
/// The first post of an approval whose session `gatewayd` does not follow.
|
||||
pub fn unfollowed_post(id: u64, session: &str) -> String {
|
||||
format!(
|
||||
"**Approval {id}** was asked in session {}, which gatewayd does not follow now (a restart, or a session from outside Mattermost).",
|
||||
code(session)
|
||||
)
|
||||
}
|
||||
|
||||
/// The line left in the asking thread when its approval goes to an approver.
|
||||
pub fn sent_to_approver(id: u64) -> String {
|
||||
format!("waiting for approval {id}: it was sent to an approver")
|
||||
}
|
||||
```
|
||||
|
||||
Characters are counted with `chars()`, never `len()`: `len()` counts bytes, and a message in any
|
||||
language but English would be cut short.
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.** `git switch m4b`, then
|
||||
`cp docs/plans/M4b/files/crates/gatewayd/tests/approvals_asked.rs crates/gatewayd/tests/`
|
||||
- [ ] **2. See it fail.** `cargo test -p gatewayd --test approvals_asked`. Expected: it does not
|
||||
compile (`Asked` and the three functions do not exist yet).
|
||||
- [ ] **3. Make the change.** Run `cargo fmt --all`.
|
||||
- [ ] **4. See it pass.** `cargo test -p gatewayd --test approvals_asked`: 4 passed. Then
|
||||
`cargo test -p gatewayd`: every suite passes.
|
||||
- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`, with 4 more tests than
|
||||
before this task (about 850).
|
||||
- [ ] **6. Log and commit.** `git add crates/gatewayd docs/implementer-log.md Cargo.lock && git commit`
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo test -p gatewayd` passes; `make gate` prints `gate: ok`.
|
||||
|
||||
## Stop and report if
|
||||
|
||||
- A test in `approvals_asked.rs` still fails with the code above appended exactly. Do not change
|
||||
the test or the texts to make it pass.
|
||||
@@ -0,0 +1,274 @@
|
||||
# M4b task 18: an approval a non-approver asked for goes to an approver
|
||||
|
||||
**Branch:** `m4b` (run `git switch m4b`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `gatewayd: approvals asked by non-approvers in direct messages go to an approver`
|
||||
|
||||
## Goal
|
||||
|
||||
M4b spec, section 6, as changed after the review of 2026-09-24. Today an approval always goes
|
||||
into the thread whose turn asked for it. When that turn was asked by someone who may talk to the
|
||||
bot but not approve (in `[allow] users`, not in `[approvals] users`), in a **direct or group
|
||||
message**, no approver can answer it there: it could only be answered with `bxctl`. Now:
|
||||
|
||||
1. **Redirected.** Such an approval goes to the first approver's direct messages as a new thread.
|
||||
Its first post is task 17's `asked_post`: who asked (`@username`), where, and their message.
|
||||
Then come the approval's own posts, in that thread, and the asking thread gets one line,
|
||||
`sent_to_approver`. The approver answers in the new thread.
|
||||
2. **Stays.** An approval asked by an approver, or asked in a channel (`O` or `P`: an approver
|
||||
can reply in the thread there), stays in its thread, as today.
|
||||
3. **Not followed.** An approval whose session has no turn in flight here already goes to the
|
||||
first approver's direct messages; now its first post is task 17's `unfollowed_post`, and the
|
||||
approval's posts follow in that thread.
|
||||
|
||||
To know who asked, the serve loop keeps two maps in memory, not in the state file (after a
|
||||
restart the turn is interrupted anyway): `authors`, the author and channel type of the last
|
||||
message queued for each session, and `askers`, for each turn running, its author, channel type
|
||||
and text. `start` fills `askers` from `authors`; `finished` removes both.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy: `crates/gatewayd/tests/serve_redirect.rs`, and a **new version** of the given
|
||||
`crates/gatewayd/tests/serve_approvals.rs` (an unfollowed session's approval now has a first
|
||||
post: 3 posts, not 2)
|
||||
- Modify: `crates/gatewayd/src/serve/approvals.rs`, `crates/gatewayd/src/serve/mod.rs`,
|
||||
`crates/gatewayd/src/serve/handle.rs`, `docs/implementer-log.md`
|
||||
|
||||
## The changes, exactly
|
||||
|
||||
### `serve/mod.rs`
|
||||
|
||||
In `struct Gateway`, after the field `open`, add:
|
||||
|
||||
```rust
|
||||
/// The author and channel type of the last message queued in each session.
|
||||
authors: HashMap<SessionId, (String, String)>,
|
||||
/// Who asked for each turn running, by session id.
|
||||
askers: HashMap<String, approvals::Asker>,
|
||||
```
|
||||
|
||||
and in `Gateway::new`, after `open: HashMap::new(),`, add `authors: HashMap::new(),` and
|
||||
`askers: HashMap::new(),`. `HashMap` and `SessionId` are already imported there.
|
||||
|
||||
### `serve/approvals.rs`
|
||||
|
||||
1. The import `use crate::approvals::text::{self, Ended};` becomes
|
||||
`use crate::approvals::text::{self, Asked, Ended};`.
|
||||
2. Insert these two types just before the doc comment of `pub(super) struct Open`:
|
||||
|
||||
```rust
|
||||
/// Who asked for the turn now running in a session: the author of the last message queued, the
|
||||
/// channel's type, and the turn's text. Kept in memory only, and forgotten when the turn ends.
|
||||
#[derive(Debug, Clone)]
|
||||
pub(super) struct Asker {
|
||||
pub(super) user: String,
|
||||
pub(super) channel_type: String,
|
||||
pub(super) prompt: String,
|
||||
}
|
||||
|
||||
/// Where an approval goes, and what is posted besides it.
|
||||
struct Place {
|
||||
channel: String,
|
||||
root: String,
|
||||
/// A post before the approval's own: who asked and what, or whose session.
|
||||
first: Option<String>,
|
||||
/// The asking thread, told where the approval went.
|
||||
note: Option<(String, String)>,
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
3. Replace the whole function `place`, from its doc comment (`/// Where an approval goes: its
|
||||
session's turn in flight, …`) to its closing `}`, including the numbered comment inside it,
|
||||
by these two functions:
|
||||
|
||||
```rust
|
||||
/// Where an approval goes (M4b spec, section 6): its turn's thread; or, when a non-approver
|
||||
/// asked in a direct or group message, the first approver's direct messages with who asked
|
||||
/// and what, and a note in the asking thread; or, for a session not followed here, the first
|
||||
/// approver's direct messages with a line saying so.
|
||||
fn place(&self, info: &PendingApproval) -> Result<Place, String> {
|
||||
let id = info.approval;
|
||||
let session = info.session.as_str();
|
||||
let Some(turn) = self.state.turn_of(session) else {
|
||||
return Ok(Place {
|
||||
channel: self.approver_dm()?,
|
||||
root: String::new(),
|
||||
first: Some(text::unfollowed_post(id, session)),
|
||||
note: None,
|
||||
});
|
||||
};
|
||||
let redirect = self.askers.get(session).filter(|a| {
|
||||
matches!(a.channel_type.as_str(), "D" | "G")
|
||||
&& !self.config.approvers().contains(&a.user)
|
||||
});
|
||||
let Some(asker) = redirect else {
|
||||
return Ok(Place {
|
||||
channel: turn.channel,
|
||||
root: turn.root,
|
||||
first: None,
|
||||
note: None,
|
||||
});
|
||||
};
|
||||
let by = match self.client.username(&asker.user) {
|
||||
Ok(name) => format!("@{name}"),
|
||||
Err(_) => asker.user.clone(),
|
||||
};
|
||||
let asked = Asked {
|
||||
by,
|
||||
channel_type: asker.channel_type.clone(),
|
||||
prompt: asker.prompt.clone(),
|
||||
};
|
||||
Ok(Place {
|
||||
channel: self.approver_dm()?,
|
||||
root: String::new(),
|
||||
first: Some(text::asked_post(id, &asked)),
|
||||
note: Some((turn.channel, turn.root)),
|
||||
})
|
||||
}
|
||||
|
||||
/// The first approver's direct channel with the bot.
|
||||
fn approver_dm(&self) -> Result<String, String> {
|
||||
let first = self
|
||||
.config
|
||||
.first_approver()
|
||||
.ok_or("no approver is configured")?;
|
||||
self.client
|
||||
.direct_channel(&self.me.id, first)
|
||||
.map_err(|e| format!("no direct channel with the first approver: {e}"))
|
||||
}
|
||||
```
|
||||
|
||||
`place` checks in this order: no turn in flight → not followed; an asker who is not an
|
||||
approver, in a `D` or `G` channel → redirected; anything else → its thread. A user name that
|
||||
cannot be looked up is shown as the user id, as it is elsewhere.
|
||||
|
||||
4. In `show`:
|
||||
- Its doc comment (two lines, `/// Post an approval's parts in order, …`) becomes:
|
||||
|
||||
```rust
|
||||
/// Post an approval's first post, if any, and its parts in order, record them, and register
|
||||
/// it for answers. An `Err` inside is why it could not be posted; the outer one stops
|
||||
/// `gatewayd` (the state file).
|
||||
```
|
||||
|
||||
- These lines
|
||||
|
||||
```rust
|
||||
let (channel, mut root) = match self.place(info) {
|
||||
Ok(place) => place,
|
||||
Err(why) => return Ok(Err(why)),
|
||||
};
|
||||
let shown = text::render(info);
|
||||
```
|
||||
|
||||
become
|
||||
|
||||
```rust
|
||||
let place = match self.place(info) {
|
||||
Ok(place) => place,
|
||||
Err(why) => return Ok(Err(why)),
|
||||
};
|
||||
let (channel, mut root) = (place.channel, place.root);
|
||||
let shown = text::render(info);
|
||||
// A first post saying who asked, or whose session, then the approval's own posts.
|
||||
let bodies: Vec<String> = place
|
||||
.first
|
||||
.into_iter()
|
||||
.chain(shown.posts.iter().cloned())
|
||||
.collect();
|
||||
```
|
||||
|
||||
- Everywhere else in `show`, `shown.posts` becomes `bodies`. There are three:
|
||||
`for body in &shown.posts {` → `for body in &bodies {`;
|
||||
`shown.posts.get(posts.len().wrapping_sub(1))` → `bodies.get(posts.len().wrapping_sub(1))`;
|
||||
`(posts.last(), shown.posts.last())` → `(posts.last(), bodies.last())`.
|
||||
`shown.answerable` stays: it is not a post.
|
||||
- Just before the last line, `Ok(Ok(posts))`, add:
|
||||
|
||||
```rust
|
||||
if let Some((note_channel, note_root)) = place.note {
|
||||
self.post(¬e_channel, ¬e_root, &text::sent_to_approver(id));
|
||||
}
|
||||
```
|
||||
|
||||
The note is posted only after the approval was posted and registered; a failure to post it is
|
||||
only logged (that is what `self.post` does).
|
||||
|
||||
### `serve/handle.rs`
|
||||
|
||||
1. Add the import `use crate::serve::approvals::Asker;` just before
|
||||
`use crate::serve::{Gateway, Stop};`.
|
||||
2. In `handle_post`, the arm `Route::Queue(message) => {` gets, as its first statement:
|
||||
|
||||
```rust
|
||||
self.authors.insert(
|
||||
message.session.clone(),
|
||||
(post.user_id.clone(), channel_type.to_string()),
|
||||
);
|
||||
```
|
||||
|
||||
3. In `start`, just before the line
|
||||
`let (registry, wait) = (Arc::clone(&self.registry), self.notice);`, add:
|
||||
|
||||
```rust
|
||||
if let Some((user, channel_type)) = self.authors.get(&batch.session) {
|
||||
let asker = Asker {
|
||||
user: user.clone(),
|
||||
channel_type: channel_type.clone(),
|
||||
prompt: batch.text.clone(),
|
||||
};
|
||||
self.askers
|
||||
.insert(batch.session.as_str().to_string(), asker);
|
||||
}
|
||||
```
|
||||
|
||||
It must come before that line: the thread spawned below takes `batch` by `move`.
|
||||
4. Replace `finished`, from its doc comment to its closing `}`, by:
|
||||
|
||||
```rust
|
||||
/// Turns that ended: out of flight, who asked forgotten, and the next batch of each session
|
||||
/// started. Never blocks.
|
||||
pub(super) fn finished(&mut self) -> Result<(), Stop> {
|
||||
while let Ok(session) = self.done_rx.try_recv() {
|
||||
self.state.end_turn(session.as_str())?;
|
||||
self.askers.remove(session.as_str());
|
||||
match self.queues.finish(&session) {
|
||||
Some(batch) => self.start(batch)?,
|
||||
None => {
|
||||
self.authors.remove(&session);
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
```
|
||||
|
||||
(The old numbered comment inside it goes: it described the loop before this change.)
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.** `git switch m4b`, then
|
||||
`cp docs/plans/M4b/files/crates/gatewayd/tests/serve_redirect.rs docs/plans/M4b/files/crates/gatewayd/tests/serve_approvals.rs crates/gatewayd/tests/`
|
||||
- [ ] **2. See it fail.** `cargo test --no-fail-fast -p gatewayd --test serve_redirect --test serve_approvals`.
|
||||
Expected: in `serve_redirect`, 3 fail and 1 passes (`in_a_channel_it_stays_in_the_thread`); in
|
||||
`serve_approvals`, 1 fails (`without_a_turn_in_flight_…`) and 4 pass.
|
||||
- [ ] **3. Make the changes**, one file at a time, in the order above, with
|
||||
`cargo check -p gatewayd` after each. Run `cargo fmt --all`.
|
||||
- [ ] **4. See it pass.**
|
||||
`cargo test -p gatewayd --test serve_redirect --test serve_approvals --test serve_answers --test serve_restart`,
|
||||
three times: 4, 5, 4 and 7 passed. Then `cargo test -p gatewayd`: every suite passes.
|
||||
- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`, with 4 more tests than
|
||||
before this task (about 854). No file over 500 lines: `serve/approvals.rs` is about 260.
|
||||
- [ ] **6. Log and commit.** `git add crates/gatewayd docs/implementer-log.md Cargo.lock && git commit`
|
||||
|
||||
This is the last task of M4b. Stop after the commit.
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo test -p gatewayd` passes; `make gate` prints `gate: ok`.
|
||||
|
||||
## Stop and report if
|
||||
|
||||
- A given test still fails after every change above is made exactly. Do not edit the tests.
|
||||
- A test passes only sometimes, or takes more than a few seconds.
|
||||
- The compiler needs a change in a file not listed here.
|
||||
@@ -1,10 +1,11 @@
|
||||
# M4b implementation plan: approvals over Mattermost
|
||||
|
||||
> **For the implementing model:** do not work from this file. The owner gives you one task file at
|
||||
> a time (`01-…` to `15-…`). This file is the index for the owner and the reviewer.
|
||||
> a time (`01-…` to `18-…`). This file is the index for the owner and the reviewer.
|
||||
|
||||
**Goal:** an `ask` call is posted in Mattermost, in the thread that asked for it (or, for a session
|
||||
not in Mattermost, in the first approver's direct messages), and an approver answers it there with
|
||||
**Goal:** an `ask` call is posted in Mattermost, in the thread that asked for it (or, when no
|
||||
approver could answer it there or the session is not followed, in the first approver's direct
|
||||
messages, with who asked and what), and an approver answers it there with
|
||||
`!approve <id>` or `!deny <id> [reason]`. The answer is recorded exactly as `bxctl`'s is, naming
|
||||
the approver and the post; the post then shows what came of it. Nobody else can answer, and no
|
||||
post is left looking answerable when it is not.
|
||||
@@ -16,7 +17,9 @@ gets the **courier**, which carries each pending approval to `gatewayd` and its
|
||||
commands (08), can edit a post and look up a name (09), renders an approval's posts (10), keeps
|
||||
the approvals it posted and judges commands (11), records their posts in the state file (12),
|
||||
listens on `gateway.sock` (13), posts and marks approvals from its serve loop (14), and wires the
|
||||
commands, the typing pause and the fallback notice (15).
|
||||
commands, the typing pause and the fallback notice (15). After the review: `brokerd`'s refusal
|
||||
without a runner says why (16); the texts that say who asked (17); and approvals asked by
|
||||
non-approvers in direct or group messages go to an approver (18).
|
||||
|
||||
**Spec:** `docs/specs/2026-09-24-m4b-approvals.md` (approved; P16 applied to the brief). Runbook:
|
||||
task 06 adds `gateway-unreachable`, task 07 widens `gatewayd-start-failed`.
|
||||
@@ -29,7 +32,7 @@ for `brokerd`, and for `gatewayd` a fake Mattermost, a fake `loopd` and a fake `
|
||||
- Everything in `AGENTS.md`, including "Lessons from earlier reviews".
|
||||
- No new dependency.
|
||||
- Branch `m4b`. One task, one fresh OpenCode session, one commit. Run `cargo fmt --all` before the
|
||||
gate. Review happens once, after task 15.
|
||||
gate. Review happens once, after task 15; tasks 16 to 18 are its follow-ups.
|
||||
- **New files** come as skeletons: types, constants and the glue written, `todo!()` bodies with the
|
||||
code to write in the comment above. **Existing files** are changed by the exact edits in the
|
||||
task, never replaced whole: a whole-file copy would overwrite an earlier task's work.
|
||||
@@ -62,8 +65,11 @@ signature (tip T29).
|
||||
| 13 | `13-gatewayd-gateway-sock.md` | the listener and connection threads | `gateway_sock.rs` | reference; 6 mutations, 5 caught, 1 that cannot change behaviour |
|
||||
| 14 | `14-gatewayd-serve-approvals.md` | posting, marking, the restart, the listener in `run` | `serve_approvals.rs`, `support/brokerd_client.rs`; `fake_mm.rs` updated | reference; 5 mutations caught |
|
||||
| 15 | `15-gatewayd-answers.md` | commands judged, typing paused, the notice | `serve_answers.rs`, `deliver_notice.rs`; `deliver.rs`, `serve.rs`, `support/gateway.rs` updated | reference; 4 mutations caught |
|
||||
| 16 | `16-brokerd-refusing-message.md` | the refusal without a runner says why (review finding 2) | `runner.rs` updated | reference |
|
||||
| 17 | `17-gatewayd-asked-texts.md` | the texts that say who asked, and the note | `approvals_asked.rs` | reference; 5 mutations caught |
|
||||
| 18 | `18-gatewayd-redirect.md` | a non-approver's approval goes to an approver (review finding 1) | `serve_redirect.rs`; `serve_approvals.rs` updated (task 14's copy is `serve_approvals.rs-task14`) | reference; 9 mutations caught; 5 runs |
|
||||
|
||||
At the end of task 15: about 845 tests (768 before task 01).
|
||||
At the end of task 15: about 845 tests (768 before task 01); at the end of task 18, about 854.
|
||||
|
||||
## Changes during the run
|
||||
|
||||
@@ -78,12 +84,20 @@ At the end of task 15: about 845 tests (768 before task 01).
|
||||
Task 15 now lists `serve/approvals.rs` with the exact fix. Checked in a scratch copy: the saved
|
||||
attempt (`.state/runs/M4b/15-first-attempt*`) plus that fix passes every suite three times and
|
||||
the gate, 845 tests. Resume from task 15.
|
||||
- 2026-09-24, review: accepted (`docs/implementer-log.md`, "M4b, tasks 01 to 15"). The device
|
||||
check found that an approval asked by a non-approver in a direct message could be answered only
|
||||
with `bxctl`; the spec was changed (`d2e8141`), and tasks 16 to 18 were added, each with a
|
||||
reference built from `m4b`, gated, and moved off this machine. Task 14's given
|
||||
`serve_approvals.rs` is now `serve_approvals.rs-task14`, since task 18 hands over a new version.
|
||||
Resume from task 16.
|
||||
|
||||
## Running it
|
||||
|
||||
```sh
|
||||
git switch m4b
|
||||
BOXMAKER_MODEL=straylight/ornith-1.5-35b-a3b tools/run-plan.sh docs/plans/M4b
|
||||
# the review's follow-ups:
|
||||
BOXMAKER_MODEL=straylight/ornith-1.5-35b-a3b tools/run-plan.sh docs/plans/M4b 16
|
||||
```
|
||||
|
||||
Keep the OpenCode TUI closed while it runs.
|
||||
|
||||
@@ -0,0 +1,178 @@
|
||||
//! The runner seam: what `run` puts in the `RunSpec` for each tool, and what it answers. Do not
|
||||
//! edit.
|
||||
|
||||
#[path = "support/build.rs"]
|
||||
mod build;
|
||||
#[path = "support/runtime.rs"]
|
||||
mod runtime;
|
||||
|
||||
use brokerd::args::{ToolArgs, ToolName};
|
||||
use brokerd::policy::{Decision, Outcome, SessionState, decide};
|
||||
use brokerd::runner::{Mount, REFUSING, Refusing, RunError, RunOutput, run};
|
||||
use build::{grant, now, read, request, set};
|
||||
use proto::{DataClass, Mode, ToolRequest, ToolResponse};
|
||||
use runtime::Recording;
|
||||
|
||||
fn allowed(grants: Vec<build::Build>, request: ToolRequest) -> Decision {
|
||||
match decide(request, &set(grants), SessionState::default(), now()) {
|
||||
Outcome::Allowed(decision) => decision,
|
||||
other => panic!("the test's call is not allowed: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
fn mount(path: &str, writable: bool) -> Mount {
|
||||
Mount {
|
||||
path: path.to_string(),
|
||||
writable,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn read_file_mounts_the_matched_path_read_only_and_has_no_network() {
|
||||
let d = allowed(
|
||||
vec![grant("notes", "read_file", Mode::Auto).paths(&["/h/notes", "/h/notes/deep"])],
|
||||
read("/h/notes/deep/a.md"),
|
||||
);
|
||||
let rt = Recording::answering("text");
|
||||
run(d, rt.as_ref());
|
||||
let seen = rt.seen();
|
||||
assert_eq!(seen.len(), 1);
|
||||
assert_eq!(seen[0].tool, ToolName::ReadFile);
|
||||
assert_eq!(
|
||||
seen[0].arguments,
|
||||
ToolArgs::ReadFile {
|
||||
path: "/h/notes/deep/a.md".to_string()
|
||||
}
|
||||
);
|
||||
// The longest path that holds the argument, and only that one.
|
||||
assert_eq!(seen[0].mounts, [mount("/h/notes/deep", false)]);
|
||||
assert_eq!(seen[0].egress, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_file_mounts_the_matched_path_writable() {
|
||||
// A grant path equal to the argument does not count, so `/s/out` is written through `/s`.
|
||||
let d = allowed(
|
||||
vec![grant("s", "write_file", Mode::Auto).paths(&["/s", "/s/out"])],
|
||||
request("write_file", r#"{"path":"/s/out","content":"x"}"#),
|
||||
);
|
||||
let rt = Recording::answering("");
|
||||
run(d, rt.as_ref());
|
||||
let seen = rt.seen();
|
||||
assert_eq!(seen[0].tool, ToolName::WriteFile);
|
||||
assert_eq!(seen[0].mounts, [mount("/s", true)]);
|
||||
assert_eq!(seen[0].egress, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shell_mounts_every_path_of_the_grant_writable() {
|
||||
let d = allowed(
|
||||
vec![grant("sh", "shell", Mode::Auto).paths(&["/a", "/b/c"])],
|
||||
request("shell", r#"{"command":"ls","cwd":"/b/c/d"}"#),
|
||||
);
|
||||
let rt = Recording::answering("");
|
||||
run(d, rt.as_ref());
|
||||
let seen = rt.seen();
|
||||
assert_eq!(seen[0].tool, ToolName::Shell);
|
||||
assert_eq!(seen[0].mounts, [mount("/a", true), mount("/b/c", true)]);
|
||||
assert_eq!(seen[0].egress, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shell_without_paths_mounts_nothing() {
|
||||
let d = allowed(
|
||||
vec![grant("sh", "shell", Mode::Auto)],
|
||||
request("shell", r#"{"command":"date"}"#),
|
||||
);
|
||||
let rt = Recording::answering("");
|
||||
run(d, rt.as_ref());
|
||||
let seen = rt.seen();
|
||||
assert_eq!(seen[0].mounts, []);
|
||||
assert_eq!(seen[0].egress, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn http_fetch_mounts_nothing_and_may_reach_the_grants_hosts_only() {
|
||||
let d = allowed(
|
||||
vec![grant("web", "http_fetch", Mode::Auto).hosts(&["example.com", "*.example.org"])],
|
||||
request("http_fetch", r#"{"url":"https://www.example.org/x"}"#),
|
||||
);
|
||||
let rt = Recording::answering("");
|
||||
run(d, rt.as_ref());
|
||||
let seen = rt.seen();
|
||||
assert_eq!(seen[0].tool, ToolName::HttpFetch);
|
||||
assert_eq!(seen[0].mounts, []);
|
||||
assert_eq!(
|
||||
seen[0].egress,
|
||||
Some(vec!["example.com".to_string(), "*.example.org".to_string()])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_result_carries_the_label_combined_over_every_matching_grant() {
|
||||
// `b-keys` has the longer path and wins the mount; the label is the highest class of both
|
||||
// grants, and untrusted because `a-home` says so.
|
||||
let d = allowed(
|
||||
vec![
|
||||
grant("a-home", "read_file", Mode::Auto)
|
||||
.paths(&["/home/kyle"])
|
||||
.class(DataClass::Private),
|
||||
grant("b-keys", "read_file", Mode::Auto)
|
||||
.paths(&["/home/kyle/keys"])
|
||||
.class(DataClass::Secret)
|
||||
.trusted(),
|
||||
],
|
||||
read("/home/kyle/keys/id"),
|
||||
);
|
||||
let rt = Recording::with(Ok(RunOutput {
|
||||
content: "key".to_string(),
|
||||
truncated: true,
|
||||
}));
|
||||
let answer = run(d, rt.as_ref());
|
||||
assert_eq!(
|
||||
answer,
|
||||
ToolResponse::Result {
|
||||
content: "key".to_string(),
|
||||
class: DataClass::Secret,
|
||||
untrusted: true,
|
||||
truncated: true,
|
||||
}
|
||||
);
|
||||
assert_eq!(rt.seen()[0].mounts, [mount("/home/kyle/keys", false)]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_run_error_is_a_failure_with_the_runtimes_sentence() {
|
||||
for error in [
|
||||
RunError::Failed("the tool timed out".to_string()),
|
||||
RunError::Unavailable("the container could not start".to_string()),
|
||||
] {
|
||||
let d = allowed(
|
||||
vec![grant("n", "read_file", Mode::Auto).paths(&["/n"])],
|
||||
read("/n/a"),
|
||||
);
|
||||
let text = match &error {
|
||||
RunError::Failed(t) | RunError::Unavailable(t) => t.clone(),
|
||||
};
|
||||
let rt = Recording::with(Err(error));
|
||||
assert_eq!(run(d, rt.as_ref()), ToolResponse::Failed { message: text });
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_production_runtime_refuses_every_call() {
|
||||
assert_eq!(
|
||||
REFUSING,
|
||||
"no tool runner is configured: brokerd.toml has no [runner] section"
|
||||
);
|
||||
let d = allowed(
|
||||
vec![grant("n", "read_file", Mode::Auto).paths(&["/n"])],
|
||||
read("/n/a"),
|
||||
);
|
||||
assert_eq!(
|
||||
run(d, &Refusing),
|
||||
ToolResponse::Failed {
|
||||
message: REFUSING.to_string()
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,86 @@
|
||||
//! The posts that tell an approver who asked for a redirected approval and what, and the line left
|
||||
//! in the asking thread (M4b spec, section 6, after the review). Do not edit.
|
||||
|
||||
use gatewayd::approvals::text::{
|
||||
Asked, PROMPT_SHOWN, asked_post, sent_to_approver, unfollowed_post,
|
||||
};
|
||||
|
||||
/// The escape for one code point, built here so this file holds no such character itself.
|
||||
fn esc(code: u32) -> String {
|
||||
format!("{}u{code:04x}", char::from(92))
|
||||
}
|
||||
|
||||
fn asked(prompt: &str, channel_type: &str) -> Asked {
|
||||
Asked {
|
||||
by: "@alex".to_string(),
|
||||
channel_type: channel_type.to_string(),
|
||||
prompt: prompt.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn who_where_and_what_they_asked() {
|
||||
assert_eq!(
|
||||
asked_post(
|
||||
3,
|
||||
&asked("Read the file /srv/notes/q3.txt and summarise it.", "D")
|
||||
),
|
||||
"**Approval 3** was asked by @alex in a direct message with the bot. Their message:\n```\nRead the file /srv/notes/q3.txt and summarise it.\n```"
|
||||
);
|
||||
let group = asked_post(4, &asked("first\n\nsecond", "G"));
|
||||
assert_eq!(
|
||||
group,
|
||||
"**Approval 4** was asked by @alex in a group message with the bot. Their message:\n```\nfirst\n\nsecond\n```",
|
||||
"a burst keeps its lines"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_message_is_escaped_and_fenced() {
|
||||
let hostile = format!(
|
||||
"see ``` and @channel{}{}",
|
||||
char::from_u32(0x202e).unwrap(),
|
||||
char::from(7)
|
||||
);
|
||||
let post = asked_post(5, &asked(&hostile, "D"));
|
||||
assert!(
|
||||
post.contains(&format!(
|
||||
"\n````\nsee ``` and @channel{}{}\n````",
|
||||
esc(0x202e),
|
||||
esc(7)
|
||||
)),
|
||||
"{post}"
|
||||
);
|
||||
assert!(!post.contains(char::from_u32(0x202e).unwrap()) && !post.contains(char::from(7)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_long_message_is_cut_and_says_how_much() {
|
||||
let long = "é".repeat(PROMPT_SHOWN + 25);
|
||||
let post = asked_post(6, &asked(&long, "D"));
|
||||
assert!(
|
||||
post.ends_with(&format!(
|
||||
"\n```\n{}\n```\n(… 25 more characters)",
|
||||
"é".repeat(PROMPT_SHOWN)
|
||||
)),
|
||||
"{}",
|
||||
&post[post.len() - 80..]
|
||||
);
|
||||
let exact = asked_post(6, &asked(&"x".repeat(PROMPT_SHOWN), "D"));
|
||||
assert!(
|
||||
!exact.contains("more characters"),
|
||||
"nothing cut, nothing said"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unfollowed_session_and_the_note() {
|
||||
assert_eq!(
|
||||
unfollowed_post(9, "chat-1789700000-1"),
|
||||
"**Approval 9** was asked in session `chat-1789700000-1`, which gatewayd does not follow now (a restart, or a session from outside Mattermost)."
|
||||
);
|
||||
assert_eq!(
|
||||
sent_to_approver(3),
|
||||
"waiting for approval 3: it was sent to an approver"
|
||||
);
|
||||
}
|
||||
@@ -112,28 +112,41 @@ fn without_a_turn_in_flight_it_goes_to_the_first_approvers_direct_messages_in_pa
|
||||
let long = format!("{{\"content\":\"{}\"}}", "x".repeat(20_000));
|
||||
let asking = ask(&socket(&home), pending(7, "chat-1789700000-1", &long)).unwrap();
|
||||
let posts = mm.posts();
|
||||
assert_eq!(posts.len(), 2, "two parts");
|
||||
assert_eq!(posts.len(), 3, "a first post, then two parts");
|
||||
assert_eq!(
|
||||
(posts[0].0.as_str(), posts[0].1.as_str()),
|
||||
(DM, ""),
|
||||
"a new thread in the direct channel"
|
||||
);
|
||||
assert_eq!(
|
||||
(posts[1].0.as_str(), posts[1].1.clone()),
|
||||
(DM, FakeMm::post_id(0)),
|
||||
"the next part in its thread"
|
||||
);
|
||||
assert!(
|
||||
posts[1]
|
||||
posts[0]
|
||||
.2
|
||||
.starts_with("**Approval 7** was asked in session `chat-1789700000-1`"),
|
||||
"{}",
|
||||
posts[0].2
|
||||
);
|
||||
for (n, part) in posts[1..].iter().enumerate() {
|
||||
assert_eq!(
|
||||
(part.0.as_str(), part.1.clone()),
|
||||
(DM, FakeMm::post_id(0)),
|
||||
"part {n} in its thread"
|
||||
);
|
||||
}
|
||||
assert!(posts[1].2.starts_with("**Approval 7**"), "{}", posts[1].2);
|
||||
assert!(
|
||||
posts[2]
|
||||
.2
|
||||
.contains("Reply in this thread with `!approve 7`")
|
||||
);
|
||||
assert_eq!(asking.posts, [FakeMm::post_id(0), FakeMm::post_id(1)]);
|
||||
assert_eq!(
|
||||
asking.posts,
|
||||
[FakeMm::post_id(0), FakeMm::post_id(1), FakeMm::post_id(2)]
|
||||
);
|
||||
asking.close();
|
||||
let edits = mm.wait_edits(1, WAIT);
|
||||
assert_eq!(
|
||||
edits[0].0,
|
||||
FakeMm::post_id(1),
|
||||
FakeMm::post_id(2),
|
||||
"only the last part is edited"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,199 @@
|
||||
//! The serve loop posts what `gateway.sock` brings, where it belongs, and marks it when it ends
|
||||
//! or when a restart left it behind (M4b spec, sections 6 and 8). Do not edit.
|
||||
|
||||
#[path = "support/brokerd_client.rs"]
|
||||
mod brokerd_client;
|
||||
#[path = "support/fake_loop.rs"]
|
||||
mod fake_loop;
|
||||
#[path = "support/fake_mm.rs"]
|
||||
mod fake_mm;
|
||||
#[path = "support/gateway.rs"]
|
||||
mod gateway;
|
||||
#[path = "support/tmp.rs"]
|
||||
mod tmp;
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::{Mutex, mpsc};
|
||||
use std::time::Duration;
|
||||
|
||||
use brokerd_client::{ask, pending};
|
||||
use fake_loop::{done, serve_loop};
|
||||
use fake_mm::{DM, FakeMm, KYLE, id, post};
|
||||
use gateway::{WAIT, config, loop_dir, read_state, start};
|
||||
use tmp::TempDir;
|
||||
|
||||
fn socket(home: &TempDir) -> PathBuf {
|
||||
home.path().join("run/gateway/gateway.sock")
|
||||
}
|
||||
|
||||
/// A turn that waits until the returned sender says so.
|
||||
fn held_loop(home: &TempDir) -> mpsc::Sender<()> {
|
||||
loop_dir(home);
|
||||
let (release_tx, release_rx) = mpsc::channel::<()>();
|
||||
let release = Mutex::new(release_rx);
|
||||
let _turns = serve_loop(&home.path().join("run/loop/loop.sock"), move |_, _| {
|
||||
let _ = release
|
||||
.lock()
|
||||
.unwrap()
|
||||
.recv_timeout(Duration::from_secs(20));
|
||||
vec![done("done")]
|
||||
});
|
||||
release_tx
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_approval_goes_into_the_asking_thread_and_is_marked_when_it_ends() {
|
||||
let home = TempDir::new("sa-thread");
|
||||
let release = held_loop(&home);
|
||||
let mm = FakeMm::start();
|
||||
let _running = start(config(&home, &mm.url(), ""));
|
||||
let mut ws = mm.next_ws(WAIT);
|
||||
let p1 = id('p', 1);
|
||||
ws.posted(&post(&p1, KYLE, DM, "", "read my notes", 5), "D");
|
||||
let until = std::time::Instant::now() + WAIT;
|
||||
let path = home.path().join("gateway/state.json");
|
||||
let in_flight = || -> bool {
|
||||
let Ok(text) = std::fs::read_to_string(&path) else {
|
||||
return false;
|
||||
};
|
||||
let v: serde_json::Value = serde_json::from_str(&text).unwrap();
|
||||
!v["in_flight"].as_array().unwrap().is_empty()
|
||||
};
|
||||
while !in_flight() {
|
||||
assert!(std::time::Instant::now() < until, "the turn never started");
|
||||
std::thread::sleep(Duration::from_millis(20));
|
||||
}
|
||||
let asking = ask(
|
||||
&socket(&home),
|
||||
pending(42, &format!("mm-{p1}"), r#"{"path":"/n/a"}"#),
|
||||
)
|
||||
.unwrap();
|
||||
let posts = mm.posts();
|
||||
assert_eq!(posts.len(), 1, "{posts:?}");
|
||||
let (channel, root, text) = &posts[0];
|
||||
assert_eq!(
|
||||
(channel.as_str(), root.as_str()),
|
||||
(DM, p1.as_str()),
|
||||
"in the asking thread"
|
||||
);
|
||||
assert!(text.starts_with("**Approval 42** · tool `read_file` · grant `notes` · taint private · expires 2026-09-24 08:20 UTC\n```\n{\"path\":\"/n/a\"}\n```\n"), "{text}");
|
||||
assert_eq!(asking.posts, [FakeMm::post_id(0)]);
|
||||
assert_eq!(
|
||||
read_state(&home)["approvals"],
|
||||
serde_json::json!([{"approval": 42, "channel": DM, "posts": [FakeMm::post_id(0)]}])
|
||||
);
|
||||
asking.close();
|
||||
let edits = mm.wait_edits(1, WAIT);
|
||||
assert_eq!(edits.len(), 1, "{edits:?}");
|
||||
assert_eq!(edits[0].0, FakeMm::post_id(0));
|
||||
assert_eq!(
|
||||
edits[0].1,
|
||||
format!("{text}\n\nNo longer pending (answered elsewhere, expired, or its turn ended).")
|
||||
);
|
||||
let until = std::time::Instant::now() + WAIT;
|
||||
while !read_state(&home)["approvals"]
|
||||
.as_array()
|
||||
.unwrap()
|
||||
.is_empty()
|
||||
{
|
||||
assert!(std::time::Instant::now() < until, "still recorded");
|
||||
std::thread::sleep(Duration::from_millis(20));
|
||||
}
|
||||
release.send(()).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn without_a_turn_in_flight_it_goes_to_the_first_approvers_direct_messages_in_parts() {
|
||||
let home = TempDir::new("sa-dm");
|
||||
loop_dir(&home);
|
||||
let mm = FakeMm::start();
|
||||
let _running = start(config(&home, &mm.url(), ""));
|
||||
let _ws = mm.next_ws(WAIT);
|
||||
let long = format!("{{\"content\":\"{}\"}}", "x".repeat(20_000));
|
||||
let asking = ask(&socket(&home), pending(7, "chat-1789700000-1", &long)).unwrap();
|
||||
let posts = mm.posts();
|
||||
assert_eq!(posts.len(), 2, "two parts");
|
||||
assert_eq!(
|
||||
(posts[0].0.as_str(), posts[0].1.as_str()),
|
||||
(DM, ""),
|
||||
"a new thread in the direct channel"
|
||||
);
|
||||
assert_eq!(
|
||||
(posts[1].0.as_str(), posts[1].1.clone()),
|
||||
(DM, FakeMm::post_id(0)),
|
||||
"the next part in its thread"
|
||||
);
|
||||
assert!(
|
||||
posts[1]
|
||||
.2
|
||||
.contains("Reply in this thread with `!approve 7`")
|
||||
);
|
||||
assert_eq!(asking.posts, [FakeMm::post_id(0), FakeMm::post_id(1)]);
|
||||
asking.close();
|
||||
let edits = mm.wait_edits(1, WAIT);
|
||||
assert_eq!(
|
||||
edits[0].0,
|
||||
FakeMm::post_id(1),
|
||||
"only the last part is edited"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_approval_that_cannot_be_posted_is_refused_to_brokerd() {
|
||||
let home = TempDir::new("sa-refused");
|
||||
loop_dir(&home);
|
||||
let mm = FakeMm::start();
|
||||
let _running = start(config(&home, &mm.url(), ""));
|
||||
let _ws = mm.next_ws(WAIT);
|
||||
mm.refuse_posts(true);
|
||||
let why = ask(&socket(&home), pending(8, "chat-1789700000-1", "{}"))
|
||||
.err()
|
||||
.expect("refused");
|
||||
assert!(
|
||||
why.starts_with("cannot post approval 8: status 400"),
|
||||
"{why}"
|
||||
);
|
||||
assert!(
|
||||
read_state(&home)["approvals"]
|
||||
.as_array()
|
||||
.unwrap()
|
||||
.is_empty()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_restart_marks_the_posts_it_left_behind() {
|
||||
let home = TempDir::new("sa-restart");
|
||||
let (a, b) = (id('q', 1), id('q', 2));
|
||||
let state = serde_json::json!({
|
||||
"channels": {}, "recent": [], "threads": [], "in_flight": [],
|
||||
"approvals": [{"approval": 5, "channel": DM, "posts": [a, b]}]
|
||||
});
|
||||
home.write("gateway/state.json", &state.to_string());
|
||||
loop_dir(&home);
|
||||
let mm = FakeMm::start();
|
||||
let _running = start(config(&home, &mm.url(), ""));
|
||||
let _ws = mm.next_ws(WAIT);
|
||||
let edits = mm.wait_edits(1, WAIT);
|
||||
assert_eq!(edits, [(b, "**Approval 5**\n\nNo longer answerable here: gatewayd restarted. If it is still pending it is posted again.".to_string())]);
|
||||
assert!(
|
||||
read_state(&home)["approvals"]
|
||||
.as_array()
|
||||
.unwrap()
|
||||
.is_empty()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_socket_is_there_while_gatewayd_runs() {
|
||||
let home = TempDir::new("sa-socket");
|
||||
loop_dir(&home);
|
||||
let mm = FakeMm::start();
|
||||
let running = start(config(&home, &mm.url(), ""));
|
||||
let _ws = mm.next_ws(WAIT);
|
||||
use std::os::unix::fs::{FileTypeExt, PermissionsExt};
|
||||
let meta = std::fs::metadata(socket(&home)).unwrap();
|
||||
assert!(meta.file_type().is_socket());
|
||||
assert_eq!(meta.permissions().mode() & 0o777, 0o600);
|
||||
drop(running);
|
||||
}
|
||||
@@ -0,0 +1,198 @@
|
||||
//! An approval asked for by someone who may talk but not approve, in a direct or group message,
|
||||
//! goes to the first approver's direct messages: first who asked and what, then the approval, and
|
||||
//! the asking thread is told where it went. In a channel it stays in its thread (M4b spec,
|
||||
//! section 6). Do not edit.
|
||||
|
||||
#[path = "support/brokerd_client.rs"]
|
||||
mod brokerd_client;
|
||||
#[path = "support/fake_loop.rs"]
|
||||
mod fake_loop;
|
||||
#[path = "support/fake_mm.rs"]
|
||||
mod fake_mm;
|
||||
#[path = "support/gateway.rs"]
|
||||
mod gateway;
|
||||
#[path = "support/tmp.rs"]
|
||||
mod tmp;
|
||||
|
||||
use std::sync::{Mutex, mpsc};
|
||||
use std::time::Duration;
|
||||
|
||||
use brokerd_client::{Asking, ask, pending};
|
||||
use fake_loop::{done, serve_loop};
|
||||
use fake_mm::{DM, EVE, EVE_DM, FakeMm, KYLE, WsPeer, id, post};
|
||||
use gateway::{Running, WAIT, loop_dir, start};
|
||||
use gatewayd::config::Config;
|
||||
use proto::{GatewayOutcome, Turn};
|
||||
use tmp::TempDir;
|
||||
|
||||
const GROUP: &str = "g0000000000000000000000000";
|
||||
const CHANNEL: &str = "o0000000000000000000000000";
|
||||
|
||||
/// KYLE and EVE may talk, in direct messages, GROUP and CHANNEL; only KYLE approves.
|
||||
fn config(home: &TempDir, url: &str) -> Config {
|
||||
let text = format!(
|
||||
"[mattermost]\nurl = \"{url}\"\n[secrets.mattermost_token]\nenv = \"NOT_READ_BY_RUN\"\n[allow]\nusers = [\"{KYLE}\", \"{EVE}\"]\nchannels = [\"{GROUP}\", \"{CHANNEL}\"]\n[approvals]\nusers = [\"{KYLE}\"]\n[paths]\nhome = \"{}\"\n[limits]\ntyping_every_ms = 100\n",
|
||||
home.path().display()
|
||||
);
|
||||
Config::parse(&text).unwrap()
|
||||
}
|
||||
|
||||
/// A loop whose turns wait for `go`, then answer "done".
|
||||
fn held(home: &TempDir) -> (mpsc::Receiver<Turn>, mpsc::Sender<()>) {
|
||||
loop_dir(home);
|
||||
let (go_tx, go_rx) = mpsc::channel::<()>();
|
||||
let go = Mutex::new(go_rx);
|
||||
let turns = serve_loop(&home.path().join("run/loop/loop.sock"), move |_, _| {
|
||||
let _ = go.lock().unwrap().recv_timeout(Duration::from_secs(20));
|
||||
vec![done("done")]
|
||||
});
|
||||
(turns, go_tx)
|
||||
}
|
||||
|
||||
struct Stage {
|
||||
_home: TempDir,
|
||||
mm: FakeMm,
|
||||
_running: Running,
|
||||
ws: WsPeer,
|
||||
go: mpsc::Sender<()>,
|
||||
asking: Asking,
|
||||
root: String,
|
||||
}
|
||||
|
||||
/// gatewayd up, EVE's message `text` in `channel` (of `channel_type`), its turn held, and
|
||||
/// approval 42 asked for its session.
|
||||
fn stage(tag: &str, channel: &str, channel_type: &str, text: &str) -> Stage {
|
||||
let home = TempDir::new(tag);
|
||||
let (turns, go) = held(&home);
|
||||
let mm = FakeMm::start();
|
||||
let running = start(config(&home, &mm.url()));
|
||||
let mut ws = mm.next_ws(WAIT);
|
||||
running.wait_log("gatewayd: connected to ");
|
||||
let root = id('p', 1);
|
||||
ws.posted(&post(&root, EVE, channel, "", text, 5), channel_type);
|
||||
turns.recv_timeout(WAIT).expect("the turn started");
|
||||
let socket = home.path().join("run/gateway/gateway.sock");
|
||||
let session = format!("mm-{root}");
|
||||
let asking = ask(&socket, pending(42, &session, r#"{"path":"/n/a"}"#)).unwrap();
|
||||
Stage {
|
||||
_home: home,
|
||||
mm,
|
||||
_running: running,
|
||||
ws,
|
||||
go,
|
||||
asking,
|
||||
root,
|
||||
}
|
||||
}
|
||||
|
||||
/// EVE's posts in `channel`, redirected: who asked, the approval, and the note, in that order.
|
||||
fn redirected(s: &Stage, channel: &str, place: &str) {
|
||||
let posts = s.mm.wait_posts(3, WAIT);
|
||||
assert_eq!(posts.len(), 3, "{posts:?}");
|
||||
assert_eq!(
|
||||
(posts[0].0.as_str(), posts[0].1.as_str()),
|
||||
(DM, ""),
|
||||
"a new thread in the approver's direct messages"
|
||||
);
|
||||
assert!(
|
||||
posts[0].2.starts_with(&format!(
|
||||
"**Approval 42** was asked by @user-e000 in {place} with the bot. Their message:\n"
|
||||
)),
|
||||
"{}",
|
||||
posts[0].2
|
||||
);
|
||||
assert_eq!(
|
||||
(posts[1].0.as_str(), posts[1].1.clone()),
|
||||
(DM, FakeMm::post_id(0)),
|
||||
"the approval in that thread"
|
||||
);
|
||||
assert!(posts[1].2.starts_with("**Approval 42**"), "{}", posts[1].2);
|
||||
assert!(
|
||||
posts[1]
|
||||
.2
|
||||
.contains("Reply in this thread with `!approve 42`")
|
||||
);
|
||||
assert_eq!(
|
||||
posts[2],
|
||||
(
|
||||
channel.to_string(),
|
||||
s.root.clone(),
|
||||
"waiting for approval 42: it was sent to an approver".to_string()
|
||||
),
|
||||
"the note, in the asking thread"
|
||||
);
|
||||
assert_eq!(s.asking.posts, [FakeMm::post_id(0), FakeMm::post_id(1)]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_direct_message_from_a_non_approver_goes_to_the_approver_who_answers_it_there() {
|
||||
let mut s = stage("rd-dm", EVE_DM, "D", "read my notes");
|
||||
redirected(&s, EVE_DM, "a direct message");
|
||||
assert!(
|
||||
s.mm.posts()[0].2.contains("\n```\nread my notes\n```"),
|
||||
"the prompt, fenced: {}",
|
||||
s.mm.posts()[0].2
|
||||
);
|
||||
let thread = FakeMm::post_id(0);
|
||||
s.ws.posted(&post(&id('c', 1), KYLE, DM, &thread, "!approve 42", 6), "D");
|
||||
let reply = s.asking.answer(WAIT).expect("the answer reaches brokerd");
|
||||
assert_eq!(
|
||||
(reply.approval, reply.approved, reply.by.as_str()),
|
||||
(42, true, KYLE)
|
||||
);
|
||||
s.asking.outcome(GatewayOutcome::Runs {});
|
||||
let edits = s.mm.wait_edits(1, WAIT);
|
||||
assert_eq!(
|
||||
edits[0].0,
|
||||
FakeMm::post_id(1),
|
||||
"the approval's post is marked"
|
||||
);
|
||||
let approval_text = s.mm.posts()[1].2.clone();
|
||||
assert!(
|
||||
edits[0].1.starts_with(&approval_text) && edits[0].1.len() > approval_text.len(),
|
||||
"its own text, with the ending: {}",
|
||||
edits[0].1
|
||||
);
|
||||
s.go.send(()).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_long_prompt_is_cut() {
|
||||
let long = "y".repeat(2_500);
|
||||
let s = stage("rd-long", EVE_DM, "D", &long);
|
||||
redirected(&s, EVE_DM, "a direct message");
|
||||
let first = &s.mm.posts()[0].2;
|
||||
assert!(first.contains(&"y".repeat(2_000)), "{first}");
|
||||
assert!(!first.contains(&"y".repeat(2_001)), "{first}");
|
||||
assert!(first.ends_with("\n(… 500 more characters)"), "{first}");
|
||||
s.go.send(()).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_group_message_goes_to_the_approver_with_the_mention_stripped() {
|
||||
let s = stage("rd-group", GROUP, "G", "@boxmaker-straylight read my notes");
|
||||
redirected(&s, GROUP, "a group message");
|
||||
let first = &s.mm.posts()[0].2;
|
||||
assert!(first.contains("\n```\nread my notes\n```"), "{first}");
|
||||
s.go.send(()).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn in_a_channel_it_stays_in_the_thread() {
|
||||
let s = stage(
|
||||
"rd-channel",
|
||||
CHANNEL,
|
||||
"O",
|
||||
"@boxmaker-straylight read my notes",
|
||||
);
|
||||
s.mm.wait_posts(1, WAIT);
|
||||
std::thread::sleep(Duration::from_millis(300));
|
||||
let posts = s.mm.posts();
|
||||
assert_eq!(posts.len(), 1, "{posts:?}");
|
||||
assert_eq!(
|
||||
(posts[0].0.as_str(), posts[0].1.as_str()),
|
||||
(CHANNEL, s.root.as_str())
|
||||
);
|
||||
assert!(posts[0].2.starts_with("**Approval 42**"), "{}", posts[0].2);
|
||||
s.go.send(()).unwrap();
|
||||
}
|
||||
Reference in New Issue
Block a user