fix: resolve sequential incident references via filter[sequential_id]#161
Merged
Conversation
Resolving an incident by its human-readable number (57597, #57597, INC-57597) walked the entire /v1/incidents list to find the matching sequential_id. The resolver binary-searched pages sorted by -created_at, and its first midpoint probe requested a high page[number] (e.g. 245). The Rootly API rejects deep pagination with 400 Bad Request once an account has enough incidents, so the first probe failed regardless of the number passed — sequential lookups were effectively dead while UUID lookups (which skip this path) kept working. Replace the page-walk with a single direct filter[sequential_id] lookup, which the API supports specifically for the "123 in INC-123". This removes the deep-pagination failure entirely and cuts the resolver from up to log2(total_pages) requests down to one. A defensive exact-match check guards against the filter being ignored so we never resolve to the wrong incident. This one resolver backs get_incident, update_incident, list_incident_roles, find_related_incidents, suggest_solutions, and the incident resource, so all reference paths are fixed together. Also harden two related issues found while auditing: - Make the INC- prefix case-insensitive (re.IGNORECASE) so lowercase inc-57597 resolves as sequential instead of falling through to a literal /v1/incidents/inc-57597 path (404). - Cap suggest_solutions page[size] at the API maximum of 100 (was 150, which the API rejects). Tests: updated resolver tests to expect the single filter call, added a lowercase inc- case and an ignored-filter guard test, and switched three fixtures off the unrealistic "inc-123" id (now parsed as sequential) to a UUID. ruff + pyright clean; full unit suite (507) passes.
Greptile SummaryThis PR replaces page-based incident-number resolution with a direct filtered lookup. The main changes are:
Confidence Score: 5/5The change looks mergeable, but unsupported-filter behavior can incorrectly report existing incidents as missing.
src/rootly_mcp_server/tools/incidents.py Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/rootly_mcp_server/tools/incidents.py:134-136
**Unsupported Filter Causes False Not-Found**
If an API deployment ignores or does not support `filter[sequential_id]`, this one-item request returns an unrelated incident. The exact-match guard then raises `LookupError`, so every existing sequential reference is reported as not found with no fallback lookup.
Reviews (1): Last reviewed commit: "fix: resolve sequential incident referen..." | Re-trigger Greptile |
Comment on lines
+134
to
+136
| params={ | ||
| "filter[sequential_id]": target_sequential_id, | ||
| "page[size]": 1, |
There was a problem hiding this comment.
Unsupported Filter Causes False Not-Found
If an API deployment ignores or does not support filter[sequential_id], this one-item request returns an unrelated incident. The exact-match guard then raises LookupError, so every existing sequential reference is reported as not found with no fallback lookup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/rootly_mcp_server/tools/incidents.py
Line: 134-136
Comment:
**Unsupported Filter Causes False Not-Found**
If an API deployment ignores or does not support `filter[sequential_id]`, this one-item request returns an unrelated incident. The exact-match guard then raises `LookupError`, so every existing sequential reference is reported as not found with no fallback lookup.
How can I resolve this? If you propose a fix, please make it concise.
kwent
approved these changes
Jul 22, 2026
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
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.
Problem
get_incident(and every other tool that accepts an incident reference) fails with 400 Bad Request whenever a bare sequential incident number (e.g.57597) is passed. UUIDs work fine — only the sequential-number path breaks. Reported from production; started failing in the last 1–2 weeks.Observed:
Cause
_resolve_incident_reference_to_uuidresolved a sequential number by walking the whole/v1/incidentslist: it binary-searched pages sorted by-created_atto find the matchingsequential_id. With a large incident history the first midpoint probe requests a highpage[number](e.g. 245 → offset 24,500), and the Rootly API rejects deep pagination with a 400. Because the first probe always lands deep, the lookup failed regardless of which number was passed. UUID lookups skip this path (GET /v1/incidents/{uuid}), which is why they kept working. It "worked until recently" because the account's incident volume crossed the API's pagination-depth limit.Fix
Resolve the number with a single direct
filter[sequential_id]request — the filter the API provides specifically for "the 123 in INC-123":log2(total_pages)requests to one.This resolver backs
get_incident,update_incident,list_incident_roles,find_related_incidents,suggest_solutions, and the incident resource, so all reference paths are fixed together.Related hardening (found while auditing)
INC-prefix — lowercaseinc-57597previously fell through to a literal/v1/incidents/inc-57597path (404); LLM clients lowercase constantly.suggest_solutionspage[size]150 → 100 — 150 exceeds the API maximum and can itself 400.Tests
filter[sequential_id]call.inc-case and an "ignored filter → not_found" guard test.inc-123id (now correctly parsed as sequential) to a UUID.