fix(server): honour incident windows and bot filtering on the App path - #29
Merged
Conversation
server/triage.ts built `metadata: { author: event.sender, state: "open" }` by
hand. Two problems, both silent.
`upsert` replaces `metadata_json` wholesale, so a `pull_request.opened`
webhook draining after a backlog scan overwrote everything that scan had
learned about the item: labels, additions, ciStatus, closesIssues,
authorIsBot. Bot filtering then fell back to the login heuristic for an item
GitHub had already told us was a bot.
And the literal drifted. It predates closesIssues, authorIsBot and closedAt,
and nothing failed when those were added.
Now only the fields the event can actually observe are written, layered over
the stored row rather than replacing it. Names come from the shared
itemMetadata(), and a name that is not part of it throws: an invented key would
otherwise sit in the row being read by nothing.
Deliberately does not write `labels: []` for an item whose labels the webhook
never looked at. Empty means "has none", not "unknown", and the scan may
already know better.
Leaves the other two parts of #27 (a ServerConfig incidents field, a scheduler
that fetches closed items) alone.
Windows are declared per repo, in that repo's existing config.json rather than in server env vars: an incident affects one repository, and config.json is already structured storage the App reads. loadRepoConfig swallows errors and falls back to defaults, which is right for a file it cannot read at all. It is wrong for a readable file declaring a window that cannot be honoured, so window validation sits outside that catch and throws. Loading a bad window as "no incident" would rank the affected backlog as rejected, which is the failure the setting exists to prevent. A backlog scan fetches closed items only for repos that declare a window. A window matches on closedAt, so a repo with no incidents gains nothing from the extra API calls, and every installation would have paid for them. `cluster.*` config still does not reach this path: ServerConfig has no `cluster` field, so a repo-specific `cluster.bot_authors` is honoured by the CLI and ignored by the App. #27 stays open for that. The fetch-scope decision is a named function rather than an inline ternary because it is the load-bearing part and runBacklogScan is not otherwise reachable from a test without mocking a GitHub client and an embedder.
Closes the last gap in #27. `cluster.bot_authors` and `cluster.include_bot_authors` were honoured by the CLI and ignored by the App, so the same repository produced different clusters depending on which path you looked at, with nothing to indicate why. Declared in the same per-repo config.json as `incidents`, and merged a level deeper than the rest: a file setting only `botAuthors` keeps the default `includeBotAuthors` rather than losing it to a wholesale block replacement. The weekly digest turned out to be worse than the cluster gap alone. It was handed `DEFAULT_REPO_CONFIG.similarityThreshold` and clustered every repo at the default, ignoring each repo's configured value, so its numbers disagreed with the backlog scan's for the same repo and neither was obviously wrong. It now loads that repo's config, which also gets it incident windows and bot logins. `WeeklyDigestConfig` accordingly loses `similarityThreshold` and `autoClose`. The first is now read per repo; the second had no reader at all before this change. A config field nothing reads is a claim the code does not honour.
…untime assert Review follow-ups. The largest is a gap the branch had left open. Bot filtering never reached the webhook triage path. #26 excluded bot items from clustering because automation reuses titles for unrelated content, so consecutive dependabot PRs read as near-identical. The same items flow through triage with no filter, which means opening a dependabot PR posted "this looks like a duplicate of #X" onto a real repository. Filtered on both sides now: a bot-authored incoming item bails before embedding, and a bot-authored stored item is not offered as a duplicate of a human's PR. Reviewed and NOT changed: triage does not pass incident windows to its store. It looked like the same class of gap, but DupeMatch carries no lifecycle state, so statePriority scores every triage candidate 0 and windows could not change a ranking there. Passing them would be cargo cult. The reason is now recorded at the call site instead of waiting to be re-raised. loadRepoConfig cast a hand-editable JSON file to Partial<RepoConfig> and trusted it. `cluster: { botAuthors: 42 }` merged cleanly and surfaced later as `new Set(42)` inside the scheduler, nowhere near the file that caused it. The shapes that leave this function are checked against what other modules assume. The webhook path asserted its own metadata key names at runtime and threw inside a live request. ItemMetadata is now a named type and the webhook writes a Partial of it, so a rename fails the build. Verified both ways: renaming a key is a compile error, and it stopped being one when an index signature crept in, which is how the first attempt at this was wrong. Also corrects the openRepoDB comment, which claimed cluster config does not reach the App path. This branch is what made that false.
Findings from a 24-agent adversarial review. Two are regressions this branch introduced, one is a fix that was wrong in a way its own test did not catch. Making loadRepoConfig throw turned an unguarded loop into a silent skipper. server/index.ts iterates every repo in an installation; one repo's hand-edited config.json aborted the loop, and every repo after it in the list never got a backlog scan, with no log naming them. Deterministic, so a GitHub redelivery reproduced it forever. The weekly digest already had a per-repo try/catch, which is what made the omission an inconsistency rather than a decision. loadRepoConfigIsolated contains it: still loud, still no silent defaults, but scoped to one repository. Both index.ts call sites use it. The bot filter added in the previous commit bailed before store.upsert, so a bot item seen only by webhook was never indexed while the scan path stored everything and filtered at cluster time. The database's contents depended on which path saw the item, and turning includeBotAuthors on later would have needed a full rescan to mean anything. The bail is now after the upsert. Incident bounds were validated twice on the CLI path and once on the App path, and not the same once. The server reached compileIncidentWindows directly from a hand-edited file, where a bare Date.parse accepted an offset-less timestamp and resolved it in the host timezone. The offset rule now lives in compileIncidentWindows, so both paths enforce it. Also from the review: - the queued-webhook drain built its own TriageConfig without `cluster`, so a webhook arriving during a scan was triaged without bot filtering while one arriving a second later was not - a single declared window flipped the scan to every closed item in the repo's history. Open and closed are now fetched separately: `since` aborts a fetch at the first item older than it, so bounding a combined state:"all" call would have dropped untouched open PRs, which are the stale duplicates a backlog scan exists to find - `cluster: "acme"` spread its character indices into the merged config - the triage report counted "open issues and PRs" after the scope stopped being open-only - the changelog gave CLI key names for a file that reads camelCase
![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: da5726050c
ℹ️ 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
+169
to
+170
| prs.push(...(await github.fetchPRs({ state: "closed", since, maxItems: 5000, batchSize: 100 }))); | ||
| issues.push(...(await github.fetchIssues({ state: "closed", since, maxItems: 5000, batchSize: 100 }))); |
There was a problem hiding this comment.
When a repository declares any incident, these calls append every item closed or otherwise updated after the earliest window start, including items whose closedAt falls between or after all configured windows. Those unrelated closed items are then embedded, persisted, counted, and clustered even though the App's normal backlog contains only open items; on active repositories they can also consume the 5,000-item limit before an older incident item is reached. Filter the fetched results by whether closedAt actually belongs to a configured window before adding them to the scan.
Useful? React with 👍 / 👎.
Merged
StressTestor
added a commit
that referenced
this pull request
Jul 30, 2026
Rewrites the accumulated [unreleased] entries as one coherent 4.0.0 entry, led by an upgrading section rather than a feature list. The headline for anyone already running pr-prism is not incident-aware ranking, it is that their database refuses to search until it is converted. Corrects text that went stale while the entries accumulated: an entry pointing at "the vector-geometry fix below" when it sits above, star-map PR #11 described as still needing to land after it merged, server/triage.ts described as still hand-rolling metadata after #29 fixed it, and incident-closed PRs described as ranking "as open" after #30 moved them between open and closed. #19 and #27 are both closed. ARCHITECTURE described similarity.ts as "ANN pre-filtering, matryoshka truncation". It contains neither, only cosineSimilarity and isZeroVector. That was already wrong and the ANN path no longer exists at all. Not corrected, deliberately: the README's "594 duplicate clusters on 6K+ items" predates the candidate-limited path (added in v0.8), so it was measured under exact comparison and is accurate again now. The matryoshka benchmark ran on 2000 items, below the old threshold, so it never took the broken path either. Co-authored-by: StressTestor <StressTestor@users.noreply.github.com>
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.
Closes #27. The App path honoured neither incident windows nor the
clusterblock, so the same repository produced different results depending on whether the CLI or the GitHub App looked at it, with nothing to indicate why.what moved
Windows and bot config are declared per repo in that repo's existing
config.json, not in server env vars: an incident affects one repository and that file is already structured storage the App reads. Key names are camelCase there (botAuthors,includeBotAuthors), matching that file rather than the CLI yaml's snake_case.Three separate defects turned up while wiring it.
The webhook path was erasing scanned data.
upsertreplacesmetadata_jsonwholesale, so apull_request.openedwebhook draining after a backlog scan dropped everything that scan had learned: labels, additions, ciStatus, closesIssues, authorIsBot. Bot filtering then fell back to a login heuristic for an item GitHub had already identified as a bot. Only fields the event can observe are written now, layered over the stored row.Separately, the weekly digest was handed
DEFAULT_REPO_CONFIG.similarityThresholdand clustered every repo at the default, ignoring each repo's configured value. Its counts disagreed with the backlog scan's for the same repo and neither looked wrong on its own.And triage had no bot filtering whatsoever, so opening a dependabot PR posted a "this looks like a duplicate" comment onto a real repository - the same noise excluded from clustering, just louder and on someone else's repo.
review found things i had got wrong
Worth naming, because two are regressions this branch introduced:
loadRepoConfigthrow turned an unguarded loop into a silent skipper: one repo's hand-edited config aborted the installation loop and every repo after it never got scanned, with no log naming them, identically on every redeliverystore.upsert, so the database's contents depended on which path saw the item, and turningincludeBotAuthorson later would have needed a full rescan to mean anythingDate.parsethat accepted an offset-less timestamp and resolved it in the host timezone. The offset rule now lives in one placesinceaborts a fetch at the first item older than it, so bounding a combinedstate:"all"call would have dropped untouched open PRs - the stale duplicates a backlog scan exists to finddeliberately not changed
Triage does not pass incident windows to its store. It looks like the same gap, but
DupeMatchcarries no lifecycle state, sostatePriorityscores every triage candidate 0 and a window could not change a ranking there. The reason is recorded at the call site rather than left to be re-raised.537 tests, lint, typecheck and build clean.