[🐛 Bug]: ServiceNow pagination loops with page 1 data and incorrect exception handling in notify_update

[Veydop]

The sysparm_offset passed by the caller is not used the code always sets params["sysparm_offset"] to 0, so ServiceNow results keep looping the first page data.
Separately, _notify_update's except calls resp.raise_for_status(), but resp is not defined
A few other minor bugs like missing comma and auth handling in notify_update

Steps to reproduce the behavior:

1. Trigger _get_incidents so that sysparm_offset > 0. Observe that the same first page of results is returned every time.
2. Trigger _notify_update failure branch and a NameError instead of the HTTPError.

Expected behavior

• _query should use the caller's sysparm_offset so pagination works page over page.
• _notify_update failure path should call .raise_for_status() on the response object.

[dosubot]

Hi @Veydop! I'm Dosu and I'm helping the Keep team.

I confirmed all the bugs you described in the current code. Here's a breakdown:

1. Pagination always returns page 1 [1]

params = {"sysparm_offset": 0, "sysparm_limit": 100}
if sysparm_limit:
    params["sysparm_limit"] = sysparm_limit
if sysparm_offset:
    params["sysparm_offset"] = 0  # ← Bug: should be sysparm_offset, not 0

The condition if sysparm_offset: correctly checks that a non-zero offset was passed, but then hardcodes 0 instead of using the sysparm_offset variable. Fix:

params["sysparm_offset"] = sysparm_offset

2. NameError in _notify_update failure branch [2]

else:
    self.logger.info("Failed to update ticket", extra={"resp": response.text})
    resp.raise_for_status()  # ← Bug: 'resp' is not defined here; should be 'response'

resp is only assigned inside the if response.status_code == 200 block, so hitting the else branch triggers a NameError instead of the intended HTTPError. Fix:

response.raise_for_status()

3. Inverted auth logic in _notify_update [3]

auth = (
    (self.authentication_config.username, self.authentication_config.password,)
    if self._access_token   # ← Bug: should be `not self._access_token`
    else None
)

This is the opposite of the pattern used in _notify and _query, where basic auth is used when not self._access_token. Here it's inverted, so basic auth is only used when an OAuth token is present.

4. Missing comma in pull_topology fields list [4]

"owned_by.name"
"manufacturer.name",  # ← Missing comma on the line above; these get concatenated

These two string literals are silently concatenated by Python into "owned_by.namemanufacturer.name", so neither field is fetched correctly.

All four issues are straightforward to fix — would you like to open a PR with patches for them?

To reply, just mention @dosu.

---

Docs are dead. Just use Dosu.

[Prabal864]

Confirmed all four against the current code - opened a PR: #6724.