Skip to content

fix(approvals): stop fire-and-forget notify() piling up in "needs you" - #60

Merged
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/notify-stuck-in-needs-you
Jul 28, 2026
Merged

fix(approvals): stop fire-and-forget notify() piling up in "needs you"#60
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/notify-stuck-in-needs-you

Conversation

@clawedassistant26

Copy link
Copy Markdown
Contributor

The bug

notify() is fire-and-forget. src/commands.mjs says so, and it never calls pollApproval — it posts, prints the link, and returns. Only ask() blocks on a human.

POST /api/approvals filed both kinds as status = 'pending'. So every notification a script fires parks itself in the dashboard's "needs you" queue and stays there for good.

Repro (executed on unmodified main, real routers + real libsql)

A normal script run: three notify() calls for progress, one ask() for the real gate.

notify("build started")     -> 201, status: pending
notify("tests green")       -> 201, status: pending
notify("deployed to prod")  -> 201, status: pending
ask("promote to stable?")   -> 201, status: pending

GET /dashboard
  banner            : 4 waiting on you
  'Needs you' shows : promote to stable?, deployed to prod, tests green, build started
  'Moshed' history  : (empty)

Exactly one thing is waiting on the operator. The dashboard says four.

It doesn't self-clear either. The only way to get a notification out of the queue is to open it and click Approve & continue (sending a reply to a script that stopped listening) or Kill the loop (killing nothing). A chatty script buries the one ask() that actually needs a human — which is the whole point of the surface.

The fix

apps/pwa/src/routes/approvals.mjs only. A notify is done the moment it goes out, so ingest files it as sent with submitted_at set, and it lands in "moshed" history instead of the queue.

  • ask() is untouched — still pending, still submitted_at = null, still polled by the CLI the same way.
  • resolve() already no-ops on anything but pending, so a notify can't be flipped by the approve form. The approve page now says "This was a notification — nothing to respond to." instead of rendering an empty You replied: "".
  • The dashboard split (status = 'pending' / status != 'pending') and pollApproval (which only reacts to submitted / killed) both handle the new value with no change. No migration needed — status is a plain TEXT column and existing rows are untouched.
  • One comment change in 001_init.sql to keep the documented status vocabulary accurate. Migrations are tracked by filename, so applied databases are unaffected.

Tests

New apps/pwa/test/approvals-notify.test.mjs, 5 tests, with the same skip-guard the other PWA tests use when the app's deps aren't installed.

Stash-verified with git stash push -q -- apps/pwa/src/: 4 of 5 fail unpatched, 5/5 with the fix. The fifth ("an ask() still waits on a human") passes either way on purpose — it's there to lock in that this change doesn't touch the blocking path.

Full root suite npm test: 197 tests / 197 pass / 0 fail (baseline on main measured this run: 192 / 192 / 0).

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: Claude <noreply@anthropic.com>
@ralyodio
ralyodio merged commit 040434e into moshcoder:main Jul 28, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants