Repository navigation
[3483 by Claude Opus 5.5] Durable change-event journal for pull requests and epic queues - #3496
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…LL-DONE" line is the run completing, which I already covered above: 40 of 41 files passed. The one failure was `test/planIssueManager.ultrafix.test.ts`, which now passes (8/8) after I stubbed the journal module in it. Nothing else needs doing.
|
/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 adds a repository event journal, instruments lifecycle writers, and supplies retention, activity notifications, documentation, and focused tests. It needs changes before merge because retention can delete recent events and several journal paths can omit real transitions or report incorrect ones. ✅ Transactional event writes — Review scores, PR outcomes, and guarded queue advancement append events within their database transactions. ✅ Failure isolation coverage — Added tests explicitly exercise missing journal tables while confirming that the underlying operations still succeed. ✅ Shared event vocabulary — Event domains and Ultrafix outcomes are centralized, including validation of the new activity domain. This review is a static trace of the supplied code; no commands were run. The authoritative checks show no failures and five pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Retention deletes unexpired events
F2: 🔴 Timestamp deduplication loses repeated status transitions
F3: 🔴 Status events can describe the wrong mutation
F4: 🔴 Returning to a previous head is silently omitted
SuggestionsThese are optional follow-ups and are not sent to No suggestions. ScoreThe implementation has useful coverage and clear integration points, but the verified defects compromise the journal’s retained history and accuracy. The required corrections are localized; pending checks are not themselves blockers. 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 0c724d6
Summary of ChangesI fixed F1–F4 and each has a regression test. All six new tests fail on the previous branch code and pass now; the full set of 20 tests in the three journal suites passes. Typecheck (core and api) and lint with zero warnings pass on the changed files. F1 – Retention deleted unexpired events. F2 – Repeated transitions within one second were lost. Status events no longer use a timestamp-based dedupe key. Each event is identified by its own F3 – Status events could describe the wrong row or prior status. A new helper,
New tests cover a write landing between another writer's lookup and its update, a newest row held by its linked review while an older row is updated, and a PR update matching several rows. F4 – A force-push back to an earlier head was dropped.
Without a delivery ID the event is written without a key, because neither the commit nor the before/after pair identifies one occurrence. In production both paths always have an ID. New test: B → C → B → C → B is fully recorded, a redelivery adds nothing, and the last recorded head matches the PR. Other changes:
Wider check: 42 test files that touch these paths, including the plan issue, webhook, routing intake and epic queue suites, pass except one. Separate discovery (not changed): There's no visual change, so I made no previews. 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ests fail on the previous branch code and pass now; the full set of 20 tests in the three journal suites passes. Typecheck (core and api) and lint with zero warnings pass on the changed files.
I fixed F1–F4 and each has a regression test. All six new tests fail on the previous branch code and pass now; the full set of 20 tests in the three journal suites passes. Typecheck (core and api) and lint with zero warnings pass on the changed files.
**F1 – Retention deleted unexpired events.** `pruneRepositoryEvents` now deletes only rows older than the 30-day cutoff. The floor (the cursor-expiration boundary) is still one past the highest deleted `seq` and never moves backwards. A cursor below it may have missed pruned events, and unexpired rows below it stay readable. New test: recent events, then an older-stamped late event at a higher `seq`, then another recent one. Only the late event is deleted.
**F2 – Repeated transitions within one second were lost.** Status events no longer use a timestamp-based dedupe key. Each event is identified by its own `seq`. A write that doesn't change the status (a redelivered webhook) is skipped by comparing the row's actual previous status. New test: five transitions with no timestamp manipulation, including `processing → under_review` twice, produce five events.
**F3 – Status events could describe the wrong row or prior status.** A new helper, `updatePlanIssuesWithStatusJournal` in `planIssueStatusJournal.ts`, does three things in one transaction: reads the affected rows and their prior statuses using the writer's own filters (including the linked-review guard), updates exactly those rows, and appends one event per row that actually changed. All three writers use it: `updatePlanIssue`, `updatePlanIssueStatus` and `updatePlanIssueByPR`.
- The production database opens transactions with `BEGIN IMMEDIATE`, so this holds across processes too.
- `stillApplies` still runs before the transaction, because it can do its own database work.
New tests cover a write landing between another writer's lookup and its update, a newest row held by its linked review while an older row is updated, and a PR update matching several rows.
**F4 – A force-push back to an earlier head was dropped.** `head_changed` is now keyed by the GitHub delivery ID: `pr:{n}:head_changed:delivery:{id}`. A redelivery carries the same ID, while a new push is a new delivery. The ID now reaches the journal from both entry points:
- **Direct webhook:** `server.ts` passes it to `processWebhookEvent`.
- **Routing relay:** `RoutingWebSocketIntakeService` passes it the same way.
Without a delivery ID the event is written without a key, because neither the commit nor the before/after pair identifies one occurrence. In production both paths always have an ID. New test: B → C → B → C → B is fully recorded, a redelivery adds nothing, and the last recorded head matches the PR.
**Other changes:**
- `docs/docs/architecture/repository-event-journal.md` is updated to the new retention, status-event and dedupe-key behaviour.
- The fake database in `test/planIssueManager.ultrafix.test.ts` now supports transactions; it passes 8/8.
- To keep `handlePlanIssueTracking` under the four-parameter lint limit, it now builds its own logger from the correlation ID.
**Wider check:** 42 test files that touch these paths, including the plan issue, webhook, routing intake and epic queue suites, pass except one. `packages/core/test/planPRClosedRecovery.test.ts` passes every test it runs but never exits and times out. It does the same on the base code, so this change didn't cause it.
**Separate discovery (not changed):** `linkPRToPlanIssue` sets a plan issue's status to `under_review` but writes no journal event. It was never one of the three journaled writers, so I left it.
There's no visual change, so I made no previews.
PR: #3496
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Opus 5.5
|
⏳ Ultrafix is waiting for CI before the next These blocking checks on
Checks matching this repository's non-blocking checks never gate Ultrafix. The loop continues automatically once the blocking checks pass; if they have not settled within 2 hours, Ultrafix stops. |
|
CI failed: Full Test Suite Shard 4/4 Please investigate and fix this CI failure.
Failure evidence
|
|
🔄 Starting work on follow-up changes requested by @propr-dev[bot] I'll analyze the 1 request and implement the necessary changes. Processing comment ID: 6830820488✓ |
AI Implementation Summary
Closes #3483
Branch:
3483/claude-opus-5-5-durable-change-event-jour-20261112-1601-vhpCommits: ✅ Changes committed (111f8b6)
AI Processing Completed
Execution Details:
Summary:
The monitor that was watching the regression run has finished. Its "ALL-DONE" line is the run completing, which I already covered above: 40 of 41 files passed. The one failure was
test/planIssueManager.ultrafix.test.ts, which now passes (8/8) after I stubbed the journal module in it. Nothing else needs doing.Detailed Logs:
1cd1995a-8869-403a-8ef8-c1b635bd3f7aLog files stored at:
/usr/src/app/logs/executions/issue-3483-2026-11-12T16-39-45-160Z-conversation.json/usr/src/app/logs/executions/issue-3483-2026-11-12T16-39-45-160Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3483.
💡 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!