Repository navigation
[3549 by Claude Opus 5.5] Plugin event subscriptions: dispatcher, retries, health and the Activity list - #3578
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…-side dispatcher reads the event journal, retries failed handlers, tracks plugin health, and records every delivery in an Activity list on the Plugins page. All the acceptance criteria are covered by new tests, and everything I ran passes: 265 server-side tests, 53 UI component tests and 4 Playwright end-to-end tests. Typecheck and the zero-warning lint checks are clean too. I didn't run the full repository test suite or a docs site build, so docs links are unchecked.
Subscriptions. The manifest format is now `{ on: '<domain>.<type>', handler, filter? }`. `on` must be a real journal event, and `filter` matches event fields or `payload.<key>` values. This replaces the old placeholder `{ event }` shape, so I updated the 12 plugin-check test manifests that still used it. Each handler call now carries a stable `deliveryId` (`evt-<seq>-<handler>`) and the `attempt` number.
Delivery.
- Cursors and order: each plugin keeps a cursor per repository. Events for one repository reach a plugin in order, one at a time; a delivery waiting on a retry holds back only its own repository.
- Retries: a failed or timed-out (30 s) handler is retried after 1 min, 5 min, 30 min and 2 h. After the fifth attempt it is marked failed and the next event follows.
- Lease: only one worker delivers to a plugin at a time. If a worker stalls past its lease, the event is redelivered with the same id. That is why delivery is at least once.
- Gaps: events that left the journal before delivery are skipped, and the gap is recorded once.
- Health: after 3 failures in a row the plugin shows as Degraded, and administrators get one Inbox notification. After 10 (set by `PROPR_PLUGIN_PAUSE_AFTER_FAILURES`), deliveries pause until someone presses Resume.
The dispatcher starts from `src/daemon.ts`, and the hourly cleanup job that prunes the journal also deletes delivery records after 14 days.
Behaviour you might not expect:
- Enabling a plugin again starts its subscriptions fresh, the same as installing it. Events that happened while it was disabled are never delivered.
- A manual Retry gets exactly one attempt, outside the repository's normal order. While deliveries are paused, the retry waits for Resume.
- Retry and Resume are recorded in the plugin's audit trail, using two new audit actions.
API and UI. Three new admin-only routes live in `packages/api/routes/pluginActivityRoutes.ts`: list activity, retry a delivery, and resume. I regenerated the OpenAPI spec and client types. The plugin detail page now has a working Activity list with Retry and a Refresh button, plus Degraded or Paused badges and a Resume button. To keep the end-to-end spec under the 400-line limit, I moved its shared mock API into `propr-ui/e2e/plugins.fixture.ts`. Preview screenshots of the Degraded and Paused states, desktop and phone, are attached separately.
Acceptance criteria
- AC1: met — `packages/core/src/plugins/pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` covers: runs once per matching event, in order per repository, with filters and the repository limit; the retry schedule then failed; the cursor surviving a restart, and a crashed attempt redelivered with the same id; history not replayed.
- AC2: met — `packages/core/test/pluginDispatcher.test.ts` ("a handler on pull_request.merged that throws, hangs or exhausts its retries never affects the merge step") drives the real `recordPullRequestOutcome` merge step.
- AC3: met — `packages/core/test/pluginDispatcher.test.ts` ("two dispatchers running at once deliver each event once (lease)"). Breaking the lease makes this test and the restart test fail.
- AC4: met — `packages/core/src/plugins/pluginDeliveries.ts`, `pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` ("repeated failures make the plugin Degraded with one notification, then pause it until Resume").
- AC5: met — `packages/api/routes/pluginActivityRoutes.ts`, `packages/api/test/pluginActivityRoutes.test.ts`, `propr-ui/src/pages/PluginsPage/PluginActivity.tsx`, `PluginActivity.test.tsx`, `propr-ui/e2e/plugin-activity.pw.ts`.
- AC6: met — `docs/docs/features/plugins.md` (Event subscriptions: events and payloads, delivery guarantees and limits, health, Activity) and `docs/docs/architecture/plugin-runtime.md` (Event dispatcher); the pause setting is also listed in `docs/docs/operations/configuration-reference.md`.
|
/ultrafix goal=8 max=10 |
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR implements journal-driven plugin subscriptions, durable delivery records, retries, health tracking, and an administrative Activity UI. It needs significant corrections before merge: pause enforcement, recovery, and delivery authorization have reachable gaps. ✅ Producer isolation — Delivery runs separately from journal appends, and the added tests exercise the real merge-outcome path with failing and timed-out handlers. ✅ API integration — Activity, Retry, and Resume reuse the administrator/preflight wrapper and include API schemas, client types, and UI coverage. ✅ Stable delivery identity — Delivery IDs consistently derive from event sequence and handler name across ordinary retries and worker recovery. This review is a static analysis of the supplied code; no commands were run. The supplied current-head status reports no failed checks and four pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Pausing does not stop the current pass
F2: 🔴 Crashed manual retries are never recovered
F3: 🔴 Delivery bypasses current installation scope
F4: 🔴 Fully pruned repositories lose gap reporting
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Make the example deduplication sustainableThe new example in ScoreThe implementation covers the principal surfaces and has substantial tests, but the verified lifecycle and authorization defects prevent merge readiness. Passing checks do not cover the demonstrated cross-repository, manual-recovery, and retention cases. Score: 5/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 583db81
Summary of ChangesI fixed all four findings (F1–F4) in the dispatcher. To check the new tests, I also ran them against the PR's previous source: 8 of 10 fail there and all 10 pass now. The old and new dispatcher suites pass together (18 tests). The plugin route, store, contributions and runner suites pass too (37 tests), and core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. F1 – pausing now stops the pass.
F2 – an interrupted manual retry is picked up again.
F3 – deliveries respect the plugin's current settings. The same pre-attempt check stops the pass if:
The handler is now given the installation as it is now, not as it was at the start of the pass. Marking a delivery F4 – emptied repositories still get their gap recorded.
Tests and docs.
Found separately, not changed. When a plugin is enabled again, its unfinished deliveries are marked failed ("Not delivered: the plugin was disabled"), and Retry still accepts them. Retrying one would send an event from before the restart, possibly one from while the plugin was disabled. Changing that wasn't part of these four findings. Review digestRisk-tiered paths
Validation evidence
Diff size: 8 files changed, +362/−114 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ck the new tests, I also ran them against the PR's previous source: 8 of 10 fail there and all 10 pass now. The old and new dispatcher suites pass together (18 tests). The plugin route, store, contributions and runner suites pass too (37 tests), and core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews.
I fixed all four findings (F1–F4) in the dispatcher. To check the new tests, I also ran them against the PR's previous source: 8 of 10 fail there and all 10 pass now. The old and new dispatcher suites pass together (18 tests). The plugin route, store, contributions and runner suites pass too (37 tests), and core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews.
**F1 – pausing now stops the pass.**
- Before every attempt, the dispatcher renews its lease and re-reads the installation and its dispatch state; if the plugin is paused, the pass for that plugin ends.
- An attempt that itself causes the pause also ends the pass at once. This covers the retry-wait path that used to skip on to the next repository, and a manual retry followed by ordinary delivery.
**F2 – an interrupted manual retry is picked up again.**
- Retry now marks the delivery as manual (a new `manual` column in this PR's migration) until its one attempt is final.
- The next lease holder also picks up manual deliveries left `running`, with the same delivery id. The attempt is still final, success or failure, so no automatic retries follow.
**F3 – deliveries respect the plugin's current settings.** The same pre-attempt check stops the pass if:
- the plugin was disabled, enabled again since the pass began, or reinstalled.
- it no longer has the `events.read` permission.
- the event's repository is outside the current repository limit. For a manual retry it marks the delivery failed instead.
The handler is now given the installation as it is now, not as it was at the start of the pass. Marking a delivery `running` only succeeds if its stored state hasn't changed since it was read. One window can't be closed: a change that lands between the check and the handler starting. The next attempt sees it.
**F4 – emptied repositories still get their gap recorded.**
- Besides repositories with recent events, the dispatcher now also visits:
- repositories with unfinished deliveries.
- repositories whose retention floor (the lowest event still guaranteed in the journal) moved past the plugin's start and past what their cursor last recorded.
- So a gap is recorded even when no events are left in the repository.
- When the floor moves, unfinished deliveries whose event is gone become `skipped`, so the 14-day cleanup removes them. Older events still in the journal below the floor are still delivered.
**Tests and docs.**
- The new regressions are in `packages/core/test/pluginDispatcherGuards.test.ts`. The shared setup moved to `packages/core/test/helpers/pluginDispatcherFixture.ts`.
- One of the new tests passes on the old code as well: "disable and enable again while a handler runs". The old lease check already caught that case, so the test only records the behaviour.
- `docs/docs/architecture/plugin-runtime.md` and `docs/docs/features/plugins.md` now describe the pre-attempt check, how a crashed manual retry is recovered, and the repositories the dispatcher visits.
**Found separately, not changed.** When a plugin is enabled again, its unfinished deliveries are marked failed ("Not delivered: the plugin was disabled"), and Retry still accepts them. Retrying one would send an event from before the restart, possibly one from while the plugin was disabled. Changing that wasn't part of these four findings.
PR: #3578
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR adds durable plugin event delivery, retries, health tracking, and an administrator Activity interface. It needs changes before merge: retention can erase unfinished delivery obligations, continuous appends can starve dispatch, and repository casing can break ordering. ✅ Scoped delivery checks — Each attempt refreshes installation permissions and repository limits, and pause transitions stop subsequent attempts. ✅ Integrated administration — Activity, Retry, and Resume share the existing administrator request wrapper and include API schemas, audit records, and UI coverage. ✅ Recovery coverage — Added tests exercise retry schedules, worker takeover, manual retry recovery, and retention gaps. This review is a static analysis of the supplied code; no commands were run. Current checks show 29 passed, five pending, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F5: 🔴 Retention removes records still needed by cursors
F6: 🔴 Continuous appends can prevent dispatch indefinitely
F7: 🔴 Repository casing splits the ordered stream
SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Bound the deduplication exampleThe new example in ScoreThe implementation has substantial integration and recovery coverage, but three verified delivery defects remain. Localized corrections should preserve the overall approach. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 1f801d2
Summary of ChangesI fixed F5, F6 and F7. Each one has a new regression test, and all four new tests fail when run against the PR's previous source. F5 (cleanup deleted records a cursor still needed).
Once the cursor moves past the event, or retention removes the event from the journal, the record is pruned as before. So nothing is kept forever. Both cases from the finding are tested in the new file
F6 (constant appends blocked dispatch). F7 (letter case split one repository into two streams). The dispatcher now treats every spelling of a repository name as one repository, keyed by its lower-case form. That covers which repositories it visits, the cursor, the gap floor (the highest floor of any spelling) and the cleanup of unfinished deliveries. Events are read across all spellings in journal order in a single query, through a new
I also changed the existing ordering test to group repositories case-insensitively, which was the check F7 said it missed. Supporting edits:
Checks: the three dispatcher suites pass (22 tests), and so do the plugin contributions, lifecycle, runner, store and journal suites (64 tests). Core typecheck and the zero-warning lint on the changed source files are clean. I didn't run the full repository suite. Separate observation, not changed: the lower-case key comes from JavaScript, while the database's case-insensitive matching only folds A–Z. They agree for repository names, which GitHub limits to plain ASCII letters. Nothing on screen changes, so there are no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 8 files changed, +158/−28 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…and all four new tests fail when run against the PR's previous source. I fixed F5, F6 and F7. Each one has a new regression test, and all four new tests fail when run against the PR's previous source. **F5 (cleanup deleted records a cursor still needed).** `prunePluginDeliveries` in `pluginDeliveries.ts` now keeps a finished record, even past 14 days, when two things hold: - its event is still in the journal, and - the event is still ahead of the repository's cursor (or of the installation's start, if there is no cursor yet). Once the cursor moves past the event, or retention removes the event from the journal, the record is pruned as before. So nothing is kept forever. Both cases from the finding are tested in the new file `packages/core/test/pluginDispatcherStreams.test.ts`: - **Two handlers, one paused:** the first handler succeeded and the second is retrying while deliveries are paused. Cleanup at 15 days keeps both records, and after Resume only the retrying handler runs. - **Final failure that paused deliveries:** cleanup keeps the failed record, so Resume doesn't start its retries over, and the next event is then delivered. **F6 (constant appends blocked dispatch).** `wake()` now remembers when the scheduled pass is due. An append only brings a pass forward and never pushes one back. The test appends every 40 ms, well inside the 200 ms wake delay, with the 5-second poll set to a minute. It checks that delivery happens while the appends are still arriving. **F7 (letter case split one repository into two streams).** The dispatcher now treats every spelling of a repository name as one repository, keyed by its lower-case form. That covers which repositories it visits, the cursor, the gap floor (the highest floor of any spelling) and the cleanup of unfinished deliveries. Events are read across all spellings in journal order in a single query, through a new `anyCase` option on `listRepositoryEvents`. It uses the case-insensitive journal index that already exists. A single query also avoids a gap where an event under a newly seen spelling could be skipped. The new test checks that: - `owner/allowed`, `Owner/Allowed` and `OWNER/ALLOWED` events arrive in journal order with one cursor. - a retry waiting on one spelling holds back a later event under another. I also changed the existing ordering test to group repositories case-insensitively, which was the check F7 said it missed. **Supporting edits:** - The `fixture-events` test manifest has two new `pull_request.closed` subscriptions (`record` and `flaky`), so one event can match two handlers. No existing test uses that event type. - The docs (`plugin-runtime.md`, `plugins.md`) now describe case-insensitive repository streams, the wake behaviour, and the longer retention while an event still waits. **Checks:** the three dispatcher suites pass (22 tests), and so do the plugin contributions, lifecycle, runner, store and journal suites (64 tests). Core typecheck and the zero-warning lint on the changed source files are clean. I didn't run the full repository suite. **Separate observation, not changed:** the lower-case key comes from JavaScript, while the database's case-insensitive matching only folds A–Z. They agree for repository names, which GitHub limits to plain ASCII letters. Nothing on screen changes, so there are no previews. PR: #3578 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements durable event delivery, retries, health reporting, and an administrative Activity UI. It needs changes before merge: a re-enable race can permanently stall delivery, and failed degradation notifications are never retried. ✅ Stable delivery identity — Automatic retries and crash recovery preserve ✅ Administrative access controls — Activity, Retry, and Resume reuse the existing permission and request-contract checks. ✅ Focused recovery coverage — Added tests cover repository ordering, retention, paused deliveries, and worker takeover. This review used static analysis only; no commands were run. The supplied current-head status reports 30 passed, four pending, and no failed checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F8: 🔴 Stale passes can permanently stall delivery
F9: 🔴 Failed degradation notifications are abandoned
SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Bound the deduplication exampleThe example in ScoreThe implementation has substantial focused coverage and coherent API integration, but the delivery-stall race and lost notification obligation require correction. Pending checks remain additional validation, not evidence against these static findings. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 746a092
Summary of ChangesI fixed F8 and F9. Each fix has new regression tests, and all four new tests fail when run against the PR's previous source. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites all pass (63 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. F8: a pass that started before a re-enable could stall delivery for good
F9: a failed Degraded notification was never sent again
I updated Two limits of the F9 fix you might not expect:
One race no check can close: an administrator can re-enable the plugin between the last check and the handler starting. That gap was already there and is unchanged; the next write sees the change and stops. Review digestRisk-tiered paths
Validation evidence
Diff size: 5 files changed, +196/−25 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ll four new tests fail when run against the PR's previous source. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites all pass (63 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. I fixed F8 and F9. Each fix has new regression tests, and all four new tests fail when run against the PR's previous source. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites all pass (63 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. **F8: a pass that started before a re-enable could stall delivery for good** - A pass now writes a cursor (creating it, moving it on, or recording a gap) only inside a transaction that first checks the dispatch row. The row must still show this dispatcher as lease holder and have the same `installed_at` and `start_seq` the pass read. If it doesn't, the pass stops and writes nothing. This is the new `fenced` helper in `packages/core/src/plugins/pluginDispatcher.ts`. - Moving a cursor on also requires the new position to be ahead of the stored one, so a cursor can never go backwards. - The same stale-start mistake also affected creating delivery records. A stale pass could leave a record of a pre-restart event stuck as "pending" in Activity forever. Creating those records now goes through the same check. - **Regressions** (in `pluginDispatcherGuards.test.ts`): the plugin is disabled and enabled again either between reading the installation and taking the lease, or between creating the cursor and the delivery record. In both cases nothing from the earlier start is left behind, and a newer event is delivered. **F9: a failed Degraded notification was never sent again** - The migration adds a `degraded_notice` column. If a database already ran the earlier copy of the migration, it adds the column there too. - The transaction that sets `degraded_at` now also stores the notification that is owed. That covers both a failed send and the worker exiting before it sends. - `sendPendingDegradedNotices` in `pluginDeliveries.ts` sends what is owed right after the attempt and again at the start of each pass, paused plugins included. After a failure it waits a minute before trying again. It clears the stored notice only once sending succeeds, and only if it is still the same notice. The existing deduplication key keeps a repeat, or two dispatchers sending at once, to one Inbox entry. - Enabling the plugin again keeps a notice that is still owed. - **Regressions:** the Inbox fails first and works later, so the notification arrives exactly once after the plugin has paused. The worker hangs before sending, and the next dispatcher sends it. I updated `docs/docs/architecture/plugin-runtime.md` to describe both changes. **Two limits of the F9 fix you might not expect:** - If the plugin recovers and becomes Degraded again while an earlier notice is still unsent, the newer notice replaces the older one. Administrators get one notification, about the latest time it became Degraded. - Uninstalling deletes the dispatch row, and any notice still owed goes with it. **One race no check can close:** an administrator can re-enable the plugin between the last check and the handler starting. That gap was already there and is unchanged; the next write sees the change and stops. PR: #3578 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements durable plugin subscriptions, retries, health reporting, and Activity controls with substantial focused coverage. It needs a localized correctness fix before merge: stale dispatcher initialization can reset a replacement installation and silently skip its events. ✅ Stable delivery identity — Delivery records and handler inputs preserve ✅ Retention-aware recovery — Cleanup preserves completed handler records while their event remains ahead of the cursor, preventing replay when another handler is still waiting. ✅ Protected activity controls — The new routes reuse administrator authorization and mutation preflight checks, and audit Retry and Resume. This review is a static analysis of the supplied code; no commands were run. The authoritative checks show 31 passed, three pending, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F10: 🔴 Stale initialization resets replacement installations
SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Bound the deduplication exampleThe new example in ScoreThe implementation has strong recovery coverage and coherent API integration, but initialization can silently lose events across a concurrent reinstall. Correcting that ownership check is necessary before merge. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 892bb71
Summary of ChangesI fixed F10. The dispatcher can no longer use an out-of-date view of an installation to create or reset dispatch state, so a replacement installation's start position isn't overwritten and events after it was enabled are delivered. The fix (
Regression tests (
The first two fail against the previous source and pass now. The third passes on both, as a check that the normal path still works. Other code with the same risk: I checked the other places that read an installation and then write. None of them write dispatch state from an out-of-date view, so I changed nothing else:
Docs: I added a sentence on this guard to the Event dispatcher section of Verification: the guards suite passes (17 tests), and the other dispatcher suites plus the plugin store, handler runner and contributions suites pass (47 tests). Core typecheck and lint with zero warnings are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 3 files changed, +85/−18 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…e view of an installation to create or reset dispatch state, so a replacement installation's start position isn't overwritten and events after it was enabled are delivered. I fixed F10. The dispatcher can no longer use an out-of-date view of an installation to create or reset dispatch state, so a replacement installation's start position isn't overwritten and events after it was enabled are delivered. **The fix** (`state()` in `packages/core/src/plugins/pluginDispatcher.ts`): - If the stored dispatch row already matches the installation the pass read, it's used as before. - Otherwise, the write happens in one transaction that first re-reads the current installation. If it's gone, has a different `installed_at`, or is disabled, the transaction writes nothing and that plugin is skipped for this pass. The next pass starts from the installation as it is then. - Inside that transaction the dispatch row is read again before deciding anything. A missing row is inserted at the journal end. A row from an earlier installation is reset with `restartPluginDispatch`. A row that already belongs to the current installation is left alone. - This covers both branches the finding named: the missing-row insert and the earlier-installation reset. **Regression tests** (`packages/core/test/pluginDispatcherGuards.test.ts`): - **Reinstall race:** uninstall, reinstall and enable, then append event E, all between the installation read and the dispatch-row read. The stale pass makes no attempt and leaves the new row unchanged, and E is delivered on the next pass. - **Uninstall race:** with the dispatch row missing, uninstall happens between the two reads. No orphan dispatch row is created, and after installing again the next event is delivered. - **Missing row (normal case):** an installation with no dispatch row still gets one at the journal end, with no history replayed. The first two fail against the previous source and pass now. The third passes on both, as a check that the normal path still works. **Other code with the same risk:** I checked the other places that read an installation and then write. None of them write dispatch state from an out-of-date view, so I changed nothing else: - The pre-attempt check and the cursor and delivery-record writes already confirm the installation and its start before acting. - The attempt's outcome is recorded only while the lease is held, and a restart or reinstall clears the lease. - Install, enable, Retry and Resume run in transactions that read the current installation. **Docs:** I added a sentence on this guard to the Event dispatcher section of `docs/docs/architecture/plugin-runtime.md`. **Verification:** the guards suite passes (17 tests), and the other dispatcher suites plus the plugin store, handler runner and contributions suites pass (47 tests). Core typecheck and lint with zero warnings are clean. I didn't run the full repository suite. Nothing on screen changes, so there are no previews. PR: #3578 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements durable plugin event delivery, retries, health reporting, and an administrator Activity interface. It needs changes before merge: attempt admission can outlive lease ownership, and an unsent health notification can be overwritten. ✅ Durable recovery — Delivery records preserve IDs and attempt counts across worker restarts, with explicit recovery tests. ✅ Retention protection — Cleanup preserves final records needed by events still ahead of their repository cursor, preventing already-completed handlers from running again. ✅ Administrative controls — Activity, Retry, and Resume use the existing administrator and preflight protections. This review is based on static tracing of the supplied code; no commands were run. Current checks show 29 passed, five pending, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F11: 🔴 Fence the attempt claim with ownership
F12: 🔴 Preserve previously owed degradation notices
SuggestionsThese are optional follow-ups and are not sent to S5: 🟢 Bound notification delivery waits
ScoreThe implementation has substantial recovery coverage and coherent API integration, but two durable-state correctness defects remain. Pending checks are not themselves blockers. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 1f77503
Summary of ChangesI fixed both findings, F11 and F12, and added a regression test for each. All four new tests fail against the previous source and pass now. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites pass (70 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. F11: starting an attempt is now tied to holding the lease (
F12: owed Degraded notices are kept until sent (
New tests (
The two extra lease checks I added for manual retries (the "outside the limit" case and the missing-event case) are partly untested. The missing-event case has a test. The outside-the-limit case doesn't, because there's no clean point in a test to pause between its reads and its write. Nothing on screen changes, so there are no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 5 files changed, +199/−32 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…test for each. All four new tests fail against the previous source and pass now. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites pass (70 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. I fixed both findings, F11 and F12, and added a regression test for each. All four new tests fail against the previous source and pass now. The three dispatcher suites, the plugin store, contributions and runner suites, and the two API plugin route suites pass (70 tests). Core typecheck and the zero-warning lint are clean. I didn't run the full repository suite. **F11: starting an attempt is now tied to holding the lease** (`pluginDispatcher.ts`, `attempt()`) - Marking a delivery `running` now happens in one transaction. It only goes through if this worker still holds the lease for the same installation and start position, the installation is still the same one and enabled, and the record hasn't changed. Otherwise the pass stops and the handler doesn't run. - The same transaction renews the lease. Without that, a slow admission could leave the lease already expired when the handler starts, so another worker could take over while the handler runs. A worker that gets past the claim now has a full lease ahead of it. - **Same defect elsewhere:** two other places changed a delivery without checking the lease. One marks a manual retry failed when its event has left the journal; the other marks it failed when its repository is outside the plugin's limit. Both now go through the same lease check and only apply if the record is unchanged. **F12: owed Degraded notices are kept until sent** (`recordHealth()`, `sendPendingDegradedNotices()`) - `degraded_notice` now holds a JSON list instead of a single notice. Each new Degraded transition is added after any notices not yet sent, so an unsent one is never replaced. A value written as a single notice by an earlier copy of this branch is read as a one-item list. - Notices go out oldest first. Each one is removed only after it was sent. The removal re-reads the list inside a transaction, so a notice added while others were being sent stays on the list. The existing deduplication key still keeps a re-sent notice to one Inbox entry. - The column and table are unchanged, so the migration needed only a comment update. `docs/docs/architecture/plugin-runtime.md` now describes the list and the lease-checked start. **New tests** (`packages/core/test/pluginDispatcherOwnership.test.ts`) - A worker paused between admitting an attempt and claiming it, whose lease is taken over meanwhile, doesn't run the handler. The new lease holder then delivers it once. - Claiming renews the lease, so no other worker can take over between the claim and the handler. - A worker that loses the lease before finishing a manual retry whose event is gone changes nothing; the lease holder then marks it failed. - Your scenario: a failed notification send, then recovery, then a second degradation. Both notices are sent, oldest first, once each, and nothing is left on the list. The two extra lease checks I added for manual retries (the "outside the limit" case and the missing-event case) are partly untested. The missing-event case has a test. The outside-the-limit case doesn't, because there's no clean point in a test to pause between its reads and its write. Nothing on screen changes, so there are no previews. PR: #3578 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements durable plugin subscriptions across dispatch, retries, health, administrative controls, and Activity UI. Static review found no verified merge blockers. It is merge-ready within the reviewed scope, conditional on the remaining checks completing successfully. ✅ Durable delivery tracking — Stable delivery IDs, persisted attempts, and per-repository cursors support recovery; retention preserves final records still needed to prevent replay. ✅ Ownership protection — Delivery claims check lease ownership and installation identity transactionally. Added tests cover takeover, reinstall, and re-enable interleavings. ✅ Administrative controls — Activity, Retry, and Resume reuse the administrator/preflight wrapper, with audit records and matching API schemas. This review used the supplied code only; no commands or tests were run. The authoritative check status reports 30 passed, four pending, and no failures. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S6: 🟢 Bound the deduplication exampleIn S7: 🟢 Check case-insensitive query performanceThe new ScoreThe implementation has substantial coverage of delivery and recovery behavior, coherent API integration, and no verified blockers in this static review. Four current-head checks remain pending. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
ℹ️ GitHub auto-merge is turned off for this repository, so ProPR merges this PR itself once it is ready (checks passed or no CI configured, and the review goal met). To let GitHub merge it the moment required checks pass, enable Settings → General → Pull Requests → Allow auto-merge. |
propr-dev
Bot
changed the title
AI Implementation Summary
Closes #3549
Branch:
3549/claude-opus-5-5-plugin-event-subscription-20261115-1434-1neCommits: ✅ Changes committed (ab019fe)
AI Processing Completed
Execution Details:
Summary:
Plugins now receive the repository events they subscribe to. A daemon-side dispatcher reads the event journal, retries failed handlers, tracks plugin health, and records every delivery in an Activity list on the Plugins page. All the acceptance criteria are covered by new tests, and everything I ran passes: 265 server-side tests, 53 UI component tests and 4 Playwright end-to-end tests. Typecheck and the zero-warning lint checks are clean too. I didn't run the full repository test suite or a docs site build, so docs links are unchecked.
Subscriptions. The manifest format is now
{ on: '<domain>.<type>', handler, filter? }.onmust be a real journal event, andfiltermatches event fields orpayload.<key>values. This replaces the old placeholder{ event }shape, so I updated the 12 plugin-check test manifests that still used it. Each handler call now carries a stabledeliveryId(evt-<seq>-<handler>) and theattemptnumber.Delivery.
PROPR_PLUGIN_PAUSE_AFTER_FAILURES), deliveries pause until someone presses Resume.The dispatcher starts from
src/daemon.ts, and the hourly cleanup job that prunes the journal also deletes delivery records after 14 days.Behaviour you might not expect:
API and UI. Three new admin-only routes live in
packages/api/routes/pluginActivityRoutes.ts: list activity, retry a delivery, and resume. I regenerated the OpenAPI spec and client types. The plugin detail page now has a working Activity list with Retry and a Refresh button, plus Degraded or Paused badges and a Resume button. To keep the end-to-end spec under the 400-line limit, I moved its shared mock API intopropr-ui/e2e/plugins.fixture.ts. Preview screenshots of the Degraded and Paused states, desktop and phone, are attached separately.Acceptance criteria
packages/core/src/plugins/pluginDispatcher.ts;packages/core/test/pluginDispatcher.test.tscovers: runs once per matching event, in order per repository, with filters and the repository limit; the retry schedule then failed; the cursor surviving a restart, and a crashed attempt redelivered with the same id; history not replayed.packages/core/test/pluginDispatcher.test.ts("a handler on pull_request.merged that throws, hangs or exhausts its retries never affects the merge step") drives the realrecordPullRequestOutcomemerge step.packages/core/test/pluginDispatcher.test.ts("two dispatchers running at once deliver each event once (lease)"). Breaking the lease makes this test and the restart test fail.packages/core/src/plugins/pluginDeliveries.ts,pluginDispatcher.ts;packages/core/test/pluginDispatcher.test.ts("repeated failures make the plugin Degraded with one notification, then pause it until Resume").packages/api/routes/pluginActivityRoutes.ts,packages/api/test/pluginActivityRoutes.test.ts,propr-ui/src/pages/PluginsPage/PluginActivity.tsx,PluginActivity.test.tsx,propr-ui/e2e/plugin-activity.pw.ts.docs/docs/features/plugins.md(Event subscriptions: events and payloads, delivery guarantees and limits, health, Activity) anddocs/docs/architecture/plugin-runtime.md(Event dispatcher); the pause setting is also listed indocs/docs/operations/configuration-reference.md.Review digest
Acceptance criteria (6 of 6 met, as stated by the agent)
With a test plugin: a subscribed handler runs once per matching event, in order per repository; filters are honoured; a failing handler is retried on the schedule and then marked failed; the cursor survives a worker restart without loss; duplicate delivery ids are stable.—`packages/core/src/plugins/pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` covers: runs once per matching event, in order per repository, with filters and the repository limit; the retry schedule then failed; the cursor surviving a restart, and a crashed attempt redelivered with…A handler that throws, hangs past the timeout or exhausts retries does not affect the originating flow (test with a handler on `pull_request.merged`).—`packages/core/test/pluginDispatcher.test.ts` ("a handler on pull_request.merged that throws, hangs or exhausts its retries never affects the merge step") drives the real `recordPullRequestOutcome` merge step.Two dispatchers running at once deliver each event once (lease test).—`packages/core/test/pluginDispatcher.test.ts` ("two dispatchers running at once deliver each event once (lease)"). Breaking the lease makes this test and the restart test fail.Degraded state, pause after repeated failures, Resume and the single notification work.—`packages/core/src/plugins/pluginDeliveries.ts`, `pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` ("repeated failures make the plugin Degraded with one notification, then pause it until Resume").Activity entries are recorded and visible through the API; the Plugins page shows them with Retry.—`packages/api/routes/pluginActivityRoutes.ts`, `packages/api/test/pluginActivityRoutes.test.ts`, `propr-ui/src/pages/PluginsPage/PluginActivity.tsx`, `PluginActivity.test.tsx`, `propr-ui/e2e/plugin-activity.pw.ts`.Docs: events list with payloads, delivery guarantees and limits in `docs/docs/features/plugins.md` and `docs/docs/architecture/plugin-runtime.md`.—`docs/docs/features/plugins.md` (Event subscriptions: events and payloads, delivery guarantees and limits, health, Activity) and `docs/docs/architecture/plugin-runtime.md` (Event dispatcher); the pause setting is also listed in `docs/docs/operations/configuration-reference.md`.Risk-tiered paths
packages/core/src/db/migrations/20261115000000_plugin_event_deliveries.js(added, +74/−0): database migrationpackages/api/openapi/registeredRoutes.ts(+1/−1)packages/api/openapi/routeDocsPlugins.ts(+11/−0)packages/api/openapi/schemasPlugins.ts(+29/−1)packages/api/pluginRouteRegistry.ts(+6/−2)packages/api/routes/pluginActivityRoutes.ts(added, +31/−0)packages/api/routes/pluginRoutes.ts(+19/−9)packages/api/server.ts(+2/−1)packages/client/src/generated/apiTypes.ts(+64/−1)packages/core/src/index.ts(+3/−0)packages/core/src/plugins/pluginDeliveries.ts(added, +219/−0)packages/core/src/plugins/pluginDispatcher.ts(added, +371/−0)packages/core/src/plugins/pluginStore.ts(+6/−0)packages/shared/src/index.ts(+1/−0)packages/shared/src/pluginDeliveries.ts(added, +77/−0)packages/shared/src/pluginInstallations.ts(+1/−1)packages/shared/src/pluginSdk.ts(+7/−1)packages/shared/src/plugins.ts(+39/−4)propr-ui/src/api/pluginsApi.ts(+11/−0)propr-ui/src/pages/PluginsPage/PluginActivity.tsx(+107/−8)propr-ui/src/pages/PluginsPage/PluginDetail.tsx(+6/−3)propr-ui/src/pages/PluginsPage/usePluginActivity.ts(added, +45/−0)src/daemon.ts(+7/−2)src/daemon/pluginEventDispatch.ts(added, +26/−0)docs/docs/architecture/plugin-runtime.md(+105/−5)docs/docs/features/plugins.md(+139/−8)docs/docs/operations/configuration-reference.md(+1/−0)docs/static/openapi/propr-api.yaml(+326/−2)packages/api/test/pluginActivityRoutes.test.ts(added, +138/−0)packages/api/test/pluginRoutes.test.ts(+5/−2)packages/core/test/fixtures/plugins/fixture-events/handlers.ts(added, +39/−0)packages/core/test/fixtures/plugins/fixture-events/manifest.json(added, +19/−0)packages/core/test/fixtures/plugins/registry.json(+2/−1)packages/core/test/helpers/pluginHandlerFixture.ts(+14/−8)packages/core/test/pluginContributions.test.ts(+3/−1)packages/core/test/pluginDispatcher.test.ts(added, +316/−0)packages/core/test/pluginHandlerRunner.test.ts(+1/−1)packages/core/test/pluginStore.test.ts(+3/−1)packages/plugins/test/testing.test.ts(+1/−0)packages/shared/test/plugins.test.ts(+14/−2)propr-ui/e2e/plugin-activity.pw.ts(added, +69/−0)propr-ui/e2e/plugins.fixture.ts(added, +235/−0)propr-ui/e2e/plugins.pw.ts(+2/−194)propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx(added, +112/−0)propr-ui/src/pages/PluginsPage/PluginDetail.test.tsx(+1/−0)propr-ui/src/pages/PluginsPage/PluginSettingsAndApproval.test.tsx(+3/−1)test/fixtures/plugins-check/manifests/bad-id.json(+2/−1)test/fixtures/plugins-check/manifests/duplicate-server-name.json(+2/−1)test/fixtures/plugins-check/manifests/host-not-hostname.json(+2/−1)test/fixtures/plugins-check/manifests/missing-connection.json(+2/−1)Validation evidence
Diff size: 58 files changed, +2,734/−272 lines
Detailed Logs:
92c5b2b6-c348-4327-bca2-66ec3de55110Log files stored at:
/usr/src/app/logs/executions/issue-3549-2026-11-15T15-12-17-353Z-conversation.json/usr/src/app/logs/executions/issue-3549-2026-11-15T15-12-17-353Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3549.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
Plugin Activity: Degraded
The plugin detail page's Activity list at desktop width with Degraded health and the last error, failed deliveries with Retry, a timed-out delivery waiting to retry, successes with expandable log lines and a retention gap.
Full-size original
Plugin Activity: Paused with Resume
Deliveries paused after 10 consecutive failures, with the Resume deliveries action; the delivery retried earlier is pending again.
Full-size original
Plugin Activity on a phone
The same Paused state and Activity list at 390 px width.
Full-size original