Skip to content

Commit 040434e

Browse files
fix(approvals): stop fire-and-forget notify() piling up in "needs you" (#60)
notify() is fire-and-forget — the CLI posts it and moves on, and nothing ever polls it. Only ask() blocks on a human. POST /api/approvals filed both as `pending`, so every notification parked itself in the dashboard's "needs you" queue permanently: the "N waiting on you" count was wrong, and the only way to clear one was to answer a script that had stopped listening. A notify is done the moment it goes out, so ingest now files it as `sent` with submitted_at set, which puts it in "moshed" history where it belongs. ask() is untouched. resolve() already no-ops on anything but `pending`, so a notify can no longer be flipped by the approve form; the approve page now says so instead of rendering an empty "You replied". Co-authored-by: clawedassistant26 <307253840+clawedassistant26@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 413d611 commit 040434e

3 files changed

Lines changed: 190 additions & 6 deletions

File tree

apps/pwa/src/migrations/001_init.sql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ CREATE TABLE IF NOT EXISTS approvals (
8181
message TEXT NOT NULL,
8282
context TEXT, -- json
8383
kind TEXT NOT NULL DEFAULT 'ask', -- ask | notify
84-
status TEXT NOT NULL DEFAULT 'pending', -- pending | submitted | killed
84+
status TEXT NOT NULL DEFAULT 'pending', -- pending | submitted | killed | sent (a fire-and-forget notify)
8585
response TEXT,
8686
cap_token TEXT NOT NULL, -- capability token embedded in the link
8787
channels TEXT, -- json array of kinds it was delivered to

apps/pwa/src/routes/approvals.mjs

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// The human-in-the-loop approvals surface.
22
// POST /api/approvals ingest from the CLI (Bearer API key) → fan out + charge
3+
// ask() lands pending (a human owes a reply); notify()
4+
// is fire-and-forget, so it lands already sent
35
// GET /api/approvals/:id CLI long-poll (Bearer owner, or ?t=cap) → {status,response}
46
// GET /approve/:id human page (session owner, or ?t=cap)
57
// POST /approve/:id submit a response (approve / redirect)
@@ -58,11 +60,20 @@ approvalsRouter.post("/api/approvals", async (req, res) => {
5860
const notified = await fanOut(user, { ...approval, url }, affordable);
5961
const cost = costOf(notified);
6062

63+
// A notify() is fire-and-forget: the script posts it and moves on, and nothing
64+
// ever polls it — only an ask() blocks on a human. Filing both as `pending`
65+
// parks every notification in the dashboard's "needs you" queue for good: the
66+
// count of things actually waiting on the operator is wrong, and the only way
67+
// to clear one is to answer a script that stopped listening. A notify is done
68+
// the moment it goes out.
69+
const isNotify = approval.kind === "notify";
70+
const status = isNotify ? "sent" : "pending";
6171
await run(
62-
`INSERT INTO approvals (id,user_id,script,message,context,kind,status,cap_token,channels,cost,created_at)
63-
VALUES (?,?,?,?,?,?,?,?,?,?,?)`,
72+
`INSERT INTO approvals (id,user_id,script,message,context,kind,status,cap_token,channels,cost,created_at,submitted_at)
73+
VALUES (?,?,?,?,?,?,?,?,?,?,?,?)`,
6474
[approval.id, approval.user_id, approval.script, approval.message, approval.context, approval.kind,
65-
"pending", approval.cap_token, JSON.stringify(notified), cost, approval.created_at]
75+
status, approval.cap_token, JSON.stringify(notified), cost, approval.created_at,
76+
isNotify ? approval.created_at : null]
6677
);
6778

6879
// settle the hold down to what actually went out (0 releases it)
@@ -71,7 +82,7 @@ approvalsRouter.post("/api/approvals", async (req, res) => {
7182
res.status(201).json({
7283
id: approval.id,
7384
url,
74-
status: "pending",
85+
status,
7586
delivered: notified,
7687
charged: cost,
7788
warning: fullCost > 0 && !held ? "insufficient credits — only free channels were used" : undefined,
@@ -114,7 +125,10 @@ approvalsRouter.get("/approve/:id", async (req, res) => {
114125
<h1 style="font-size:1.7rem;letter-spacing:-.02em">${esc(a.message)}</h1>
115126
${cells ? `<div style="display:grid;grid-template-columns:1fr 1fr;gap:8px;margin-top:16px">${cells}</div>` : ""}
116127
${done
117-
? `<div class="notice ok" style="margin-top:18px">${a.status === "submitted" ? `You replied: “${esc(a.response || "")}”` : "This loop was killed."}</div>`
128+
? `<div class="notice ok" style="margin-top:18px">${
129+
a.status === "sent" ? "This was a notification — nothing to respond to."
130+
: a.status === "submitted" ? `You replied: “${esc(a.response || "")}”`
131+
: "This loop was killed."}</div>`
118132
: `<form method="post" action="/approve/${a.id}${t}" style="margin-top:18px">${csrfInput(req)}
119133
<label class="field"><span>Instructions back to the script (optional)</span>
120134
<textarea name="response" rows="3" placeholder="e.g. yes — and bump the tag to v2.1"></textarea></label>
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
// A fire-and-forget notify() must not sit in the operator's "needs you" queue.
2+
//
3+
// These boot the real routers against a throwaway libsql file database. They
4+
// skip cleanly when the PWA dependencies are not installed (a fresh repo clone
5+
// only has the root CLI deps), so the root `npm test` stays green either way.
6+
// Run `npm install` in apps/pwa to enable them.
7+
import assert from "node:assert/strict";
8+
import { mkdtempSync } from "node:fs";
9+
import { tmpdir } from "node:os";
10+
import path from "node:path";
11+
import { createRequire } from "node:module";
12+
import test from "node:test";
13+
14+
const require = createRequire(import.meta.url);
15+
let deps = null;
16+
try {
17+
deps = { express: require("express"), cookieParser: require("cookie-parser") };
18+
} catch {
19+
deps = null; // pwa dependencies not installed — tests below skip
20+
}
21+
22+
// Point the app at a throwaway database BEFORE importing its modules (config
23+
// reads the environment once, at import time).
24+
const workdir = mkdtempSync(path.join(tmpdir(), "moshcode-pwa-notify-test-"));
25+
process.env.DATABASE_URL = `file:${path.join(workdir, "test.db")}`;
26+
process.env.SESSION_SECRET = "test-secret";
27+
28+
async function boot() {
29+
const { migrate } = await import("../src/migrate.mjs");
30+
await migrate();
31+
const { run, all, get } = await import("../src/db.mjs");
32+
const { sessionMiddleware, csrfGuard } = await import("../src/lib/session.mjs");
33+
const { approvalsRouter } = await import("../src/routes/approvals.mjs");
34+
const { pagesRouter } = await import("../src/routes/pages.mjs");
35+
const { createApiKey } = await import("../src/lib/apikey.mjs");
36+
const { id, token } = await import("../src/lib/crypto.mjs");
37+
38+
const app = deps.express();
39+
app.use(deps.express.json({ verify: (req, _res, buf) => { req.rawBody = buf.toString("utf8"); } }));
40+
app.use(deps.express.urlencoded({ extended: false }));
41+
app.use(deps.cookieParser());
42+
app.use(sessionMiddleware);
43+
app.use(csrfGuard);
44+
app.use(approvalsRouter);
45+
app.use(pagesRouter);
46+
const server = await new Promise((resolve) => {
47+
const s = app.listen(0, "127.0.0.1", () => resolve(s));
48+
});
49+
const base = `http://127.0.0.1:${server.address().port}`;
50+
51+
// A user with push enabled (free, and on by default for real signups), plus
52+
// the API key the CLI sends and a browser session for the dashboard.
53+
const seedUser = async (userId) => {
54+
await run(`INSERT INTO users (id, email, display_name, created_at) VALUES (?,?,?,?)`,
55+
[userId, `${userId}@b.c`, "demo", Date.now()]);
56+
await run(`INSERT INTO channels (id,user_id,kind,target,enabled,created_at) VALUES (?,?,?,?,?,?)`,
57+
[id(), userId, "push", null, 1, Date.now()]);
58+
const { plaintext } = await createApiKey(userId, "cli");
59+
const sess = token();
60+
await run(`INSERT INTO sessions (token,user_id,created_at,expires_at) VALUES (?,?,?,?)`,
61+
[sess, userId, Date.now(), Date.now() + 3600e3]);
62+
return { key: plaintext, sess };
63+
};
64+
65+
// What the CLI does: POST /api/approvals with kind ask | notify.
66+
const ingest = async (key, kind, message) => {
67+
const res = await fetch(`${base}/api/approvals`, {
68+
method: "POST",
69+
headers: { authorization: `Bearer ${key}`, "content-type": "application/json" },
70+
body: JSON.stringify({ message, kind, script: "ship.mosh" }),
71+
});
72+
return { status: res.status, body: await res.json() };
73+
};
74+
75+
const dashboard = (sess) => fetch(`${base}/dashboard`, { headers: { cookie: `mc_sess=${sess}` } }).then((r) => r.text());
76+
const approvePage = (id, cap) => fetch(`${base}/approve/${id}?t=${cap}`).then((r) => r.text());
77+
78+
return { base, server, run, all, get, seedUser, ingest, dashboard, approvePage };
79+
}
80+
81+
// The dashboard's "needs you" column links each pending approval; "moshed"
82+
// history renders the message in a <b> instead.
83+
const waitingBanner = (html) => Number(/(\d+) waiting on you/.exec(html)?.[1]);
84+
const needsYou = (html) =>
85+
[...html.matchAll(/href="\/approve\/[^"]*"[\s\S]{0,400}?font-weight:700">([^<]*)</g)].map((m) => m[1]);
86+
const moshed = (html) => [...html.matchAll(/<b style="color:var\(--text\)">([^<]*)</g)].map((m) => m[1]);
87+
88+
test("a notify() is filed as already sent, not pending", { skip: !deps && "pwa deps not installed" }, async () => {
89+
const { server, get, seedUser, ingest } = await boot();
90+
try {
91+
const { key } = await seedUser("u-notify-status");
92+
const { status, body } = await ingest(key, "notify", "deployed to prod");
93+
assert.equal(status, 201);
94+
assert.equal(body.status, "sent");
95+
96+
const row = await get(`SELECT status, submitted_at FROM approvals WHERE id = ?`, [body.id]);
97+
assert.equal(row.status, "sent");
98+
assert.ok(row.submitted_at, "a sent notify is resolved, so it needs a submitted_at to sort history by");
99+
} finally { server.close(); }
100+
});
101+
102+
test("an ask() still waits on a human", { skip: !deps && "pwa deps not installed" }, async () => {
103+
const { server, get, seedUser, ingest } = await boot();
104+
try {
105+
const { key } = await seedUser("u-ask-status");
106+
const { body } = await ingest(key, "ask", "promote to stable?");
107+
assert.equal(body.status, "pending");
108+
109+
const row = await get(`SELECT status, submitted_at FROM approvals WHERE id = ?`, [body.id]);
110+
assert.equal(row.status, "pending");
111+
assert.equal(row.submitted_at, null);
112+
} finally { server.close(); }
113+
});
114+
115+
test("the dashboard counts only what is actually waiting", { skip: !deps && "pwa deps not installed" }, async () => {
116+
const { server, seedUser, ingest, dashboard } = await boot();
117+
try {
118+
const { key, sess } = await seedUser("u-dash");
119+
// a typical script run: chatter along the way, one real gate at the end
120+
for (const m of ["build started", "tests green", "deployed to prod"]) await ingest(key, "notify", m);
121+
await ingest(key, "ask", "promote to stable?");
122+
123+
const html = await dashboard(sess);
124+
assert.equal(waitingBanner(html), 1, "only the ask() is waiting on the operator");
125+
assert.deepEqual(needsYou(html), ["promote to stable?"]);
126+
// the notifications are history, not a queue
127+
assert.deepEqual(moshed(html).sort(), ["build started", "deployed to prod", "tests green"]);
128+
} finally { server.close(); }
129+
});
130+
131+
test("a notify() cannot be answered — it stays sent and records no reply",
132+
{ skip: !deps && "pwa deps not installed" }, async () => {
133+
const { base, server, get, seedUser, ingest } = await boot();
134+
try {
135+
const { key } = await seedUser("u-notify-resolve");
136+
const { body } = await ingest(key, "notify", "deployed to prod");
137+
const cap = (await get(`SELECT cap_token FROM approvals WHERE id = ?`, [body.id])).cap_token;
138+
139+
// the page issues the double-submit CSRF cookie the form posts back
140+
const pageRes = await fetch(`${base}/approve/${body.id}?t=${cap}`);
141+
await pageRes.text();
142+
const csrf = /mc_csrf=([^;]+)/.exec(pageRes.headers.getSetCookie().join("; "))[1];
143+
144+
const res = await fetch(`${base}/approve/${body.id}?t=${cap}`, {
145+
method: "POST",
146+
headers: { "content-type": "application/x-www-form-urlencoded", cookie: `mc_csrf=${csrf}` },
147+
body: new URLSearchParams({ response: "sure, go ahead", _csrf: csrf }),
148+
redirect: "manual",
149+
});
150+
assert.equal(res.status, 302);
151+
152+
const row = await get(`SELECT status, response FROM approvals WHERE id = ?`, [body.id]);
153+
assert.equal(row.status, "sent");
154+
assert.equal(row.response, null);
155+
} finally { server.close(); }
156+
});
157+
158+
test("the notify page says there is nothing to respond to", { skip: !deps && "pwa deps not installed" }, async () => {
159+
const { server, get, seedUser, ingest, approvePage } = await boot();
160+
try {
161+
const { key } = await seedUser("u-notify-page");
162+
const { body } = await ingest(key, "notify", "deployed to prod");
163+
const cap = (await get(`SELECT cap_token FROM approvals WHERE id = ?`, [body.id])).cap_token;
164+
165+
const html = await approvePage(body.id, cap);
166+
assert.match(html, /nothing to respond to/);
167+
assert.doesNotMatch(html, /You replied/);
168+
assert.doesNotMatch(html, /Kill the loop/, "a notification has no loop to kill");
169+
} finally { server.close(); }
170+
});

0 commit comments

Comments
 (0)