feat(rank): treat incident-closed PRs as open, not rejected - #28
Merged
Conversation
A repository-wide event (visibility flip, bulk close, migration) can close
hundreds of PRs for reasons unrelated to their quality. selectCanonical()
ranks lifecycle state before score, so those items ranked as rejections and
sank below genuinely-closed siblings -- exactly inverting the triage order
for the set a maintainer most needs to review.
prism.config.yaml now accepts an `incidents:` list of {start, end, reason}
windows. closedAt is captured from both the GraphQL and REST paths, stored in
metadata_json (no schema migration), and store.ts stamps `incidentClosed`
onto each item at hydration. statePriority() ranks an incident-closed PR as
open.
Only the raw closedAt is persisted; incidentClosed is derived at read time,
so correcting a mis-set window is a config edit rather than a rescan of the
whole backlog. The starmap payload carries `incidentClosed: true` (omitted
when false, keeping the consumer contract additive) so a consumer can bucket
these for re-triage instead of treating them as rejected.
Review found two real bugs in the first cut.
Timezone: window bounds were bare strings fed to Date.parse, so an
offset-less ISO literal resolved in the HOST timezone. The same config
selected a different set of PRs under TZ=America/Denver than under UTC in
CI, and "2026-07-23" parsed as UTC while "2026-07-23T00:00:00" parsed local
-- two spellings of one day disagreeing. Bounds now require an explicit
offset and are rejected at load. Verified across UTC, America/Denver and
Asia/Kolkata: every accepted form yields a byte-identical instant.
Silent misconfiguration: unparseable ("last tuesday"), inverted, and blank
windows were accepted and then matched nothing, forever, with no diagnostic.
A single typo made the whole feature a no-op. Now fails at load.
Scope: incident-aware ranking is CLI-only. An earlier version of this commit
claimed to wire the server/GitHub-App path; it did not. openRepoDB gained a
parameter no caller could supply -- ServerConfig has no `incidents` field --
and the scheduler fetches open items only, so closedAt is never populated
there. The whole server change was removable with the suite still green.
That parameter is gone and the limitation is stated in server/db.ts,
ARCHITECTURE.md and CHANGELOG. Wiring the App path needs a config field, a
scheduler that fetches closed items, and server/triage.ts's hand-rolled
metadata literal replaced; that is its own change.
Kept from that attempt: server/scheduler.ts now builds metadata via
itemMetadata() instead of a hand-rolled literal that had drifted from it,
silently dropping bodyLength, nodeId, headRefOid and closesIssues on every
scheduled sync. Unrelated to incidents, but a real bug and a one-line fix.
Also: the "config edit, not a rescan" claim holds only for rows scanned
since the feature landed; earlier rows need a one-time `prism scan
--state all`, now documented. Extracted the duplicated hydration expression
so the two sites cannot drift, documented `incidents:` in the config
template so init surfaces it, and fixed a stale doc comment.
Three loose ends from the incident-window review. ARCHITECTURE said the server path "ranks exactly as it did before", which is now wrong twice over: it overstates a narrow gap, and bot filtering does reach that path, since scheduler.ts calls findDuplicateClusters and picks up the default. Scoped to what is actually missing there, which is incident windows. "Tracked separately" tracked nothing. Opened #27 for wiring the server path, and pointed both the doc and the db.ts comment at it. The issue also carries the triage.ts hand-rolled metadata literal, which is worth fixing on its own: it has already drifted behind itemMetadata() by three fields and nothing catches it. scheduler.ts moving to the shared helper is now in the changelog, which it was not. Adds config tests for the incident window rules. Checked they discriminate: relaxing `reason` to a bare `z.string()` fails the blank-reason case rather than passing silently.
Adversarial review follow-ups. The comment I added to server/db.ts said incident windows were the only thing missing on the App path. They are not: ServerConfig has no `cluster` field either, so `cluster.bot_authors` and `cluster.include_bot_authors` are honoured by the CLI and ignored by the App. Only the built-in bot list reaches that path. ARCHITECTURE had the same overstatement. A maintainer whose repo-specific bot login is filtered locally and not in the digest would have gone hunting for a clustering bug. The changelog called `prism scan --state all` a one-time backfill. It is per-incident: a default scan fetches open items only, so after a bulk close the affected PRs stop being returned and their rows keep `state: open` with no `closedAt`. Configure a window without re-scanning and it matches nothing. `incidentClosed` is emitted on every reference to an item, not just on items, so a consumer contract has to accept it in six positions. The changelog said "items", which understates what a coordinated importer patch needs. Validation: - the offset rule accepted `2026-07-23 00:00:00+00:00`, which has an offset but a space where the T belongs, putting Date.parse into its implementation-defined fallback. Now requires the full ISO-8601 shape. - an unparseable bound also reported "end must be after start", because NaN > NaN is false. That points the reader at the wrong field, so the ordering check now defers when either bound failed to parse. Tests: - `closedAt` was verified only through itemClosedAt in isolation. Both ways it can silently die (a query stops requesting the field, a mapper stops calling it) now fail a test; checked by breaking each in turn. - the incident hydration test covered getAllItems only. getAllItemsMulti duplicates that literal and is what cross-repo dupes uses. - the "unparseable timestamp" test fed input that tripped the offset rule first, so it passed with the parseability rule deleted. Split into cases that isolate each rule.
Review follow-ups. isIncidentClosed skipped a window whose bounds would not parse and returned false, so a malformed window matched nothing and said nothing. That is the same silent no-op the config schema rejects at load, and windows reach a VectorStore from callers that never went through that schema. Bounds are now resolved once by compileIncidentWindows, which throws and names the offending window by its reason so a long list is searchable. Compiling once also stops re-parsing two constants for every item in the backlog, which was work with no result. The incident windows were a fourth positional constructor parameter. They are now a named options object, so the tail stays one argument as more knobs arrive. The first three positionals are pre-existing and left alone. itemMetadata moves out of pipeline.ts into src/metadata.ts. server/scheduler.ts needs it, and importing it from the CLI pipeline dragged a spinner, a GitHub client and an embedder factory into the server for one object literal. The flag has two spellings on purpose, now stated where they are declared: `boolean` internally, `true`-only on the starmap wire format, because the payload omits the field when false and a consumer contract should reject anything else. Also corrects ARCHITECTURE's "one-time backfill" the same way the changelog was already corrected: `--state all` is needed after each incident.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d91e715c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+69
to
+71
| const store = new VectorStore(undefined, embedder.dimensions, env.EMBEDDING_MODEL, { | ||
| incidentWindows: config.incidents ?? [], | ||
| }); |
There was a problem hiding this comment.
When the config uses repos or the user supplies --repo, createPipelineContext is invoked for each selected repository but forwards the same unscoped config.incidents list every time. An incident recorded for one repository therefore promotes any PR in another repository whose closedAt happens to fall in the same window, producing incorrect canonical selections and starmap data. Associate each window with a repository or otherwise filter the windows before constructing this store.
Useful? React with 👍 / 👎.
Comment on lines
299
to
+301
| state: metadata.state, | ||
| closedAt: metadata.closedAt, | ||
| incidentClosed: this.incidentFlag(metadata), |
There was a problem hiding this comment.
When a closed issue happens to have a closedAt inside an incident window, this hydration path marks it incidentClosed because incidentFlag receives only metadata and never checks row.type. In a PR-majority mixed cluster, statePriority then treats that issue as open, so it can outrank deliberately closed PRs and even become the canonical item; the starmap also incorrectly advertises the issue as incident-closed. Pass the item type into the predicate and limit this override to PRs.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A repository-wide event closes PRs for reasons that have nothing to do with their quality: a visibility flip, a bulk close, a migration.
selectCanonical()ranks lifecycle state before score, so every one of those PRs sinks below genuinely-closed siblings.That inverts triage order for exactly the backlog you would want to look at first.
what changes
prism.config.yamlaccepts incident windows:A PR closed inside a window ranks as open.
closedAtis what gets persisted;incidentClosedis derived at read time, so correcting a mis-set window is a config edit rather than a rescan.Window bounds must carry an explicit UTC offset.
Date.parse("2026-07-23T00:00:00")resolves in the host timezone, so an offset-less bound would select a different set of PRs on a laptop than in CI, and"2026-07-23"parses as UTC while"2026-07-23T00:00:00"parses local. Two spellings of one day disagreeing is not a thing a window can survive. Bounds are compiled once at load: unparseable or inverted throws, naming the offending window by its reason.two things to know before configuring one
Re-scan after each incident, not once: a default scan fetches open items only, so after a bulk close the affected PRs stop being returned at all and their stored rows keep
state: openwith noclosedAt.prism scan --state allis needed per incident or the window matches nothing.Pin windows tightly. A day-wide window sweeps up PRs that were closed deliberately that day.
scope
CLI only. The App path applies no windows:
ServerConfighas noincidentsfield and the scheduler fetches open items only, soclosedAtis never populated there. Tracked in #27, along withserver/triage.ts's hand-rolled metadata literal, which has drifted three fields behinditemMetadata().scheduler.tsmoves to the shared helper here.Worth being precise about, since i got this wrong once already in review: that path is not otherwise frozen. It calls
findDuplicateClusters, so the built-in bot list applies. Thecluster.*config does not, becauseServerConfighas noclusterfield either.starmap
Items and every reference to them carry
incidentClosed: true, omitted when false. Six positions:canonical,runnerUp,partition.prs,partition.issues,tracker.ref,tracker.candidates. A consumer contract has to accept it across all six positions. star-map's side is https://github.com/RaresKeY/star-map-odysseus/pull/11.review found six things worth naming
An adversarial pass and a taste pass ran over this. What they caught:
cluster.*config is also missing there2026-07-23 00:00:00+00:00passing the offset rule with a space where theTbelongs, landing inDate.parse's implementation-defined fallbackNaN > NaNis falseisIncidentClosedsilently skipping a malformed window, the same no-op the config schema rejects loudlyclosedAttested only in isolation, so a dropped query field or mapper call would have killed the feature with every test greenThe last one now has guards for both failure modes, checked by breaking each in turn.
506 tests, lint, typecheck and build clean.