Conversation
Add authenticated HEAD /api/v4/user check to periodic health monitor probes (static token mode only; skipped in OAuth mode where no global token is available during background checks). Previously the health monitor stayed in `healthy` state after a token was revoked because the unauthenticated /api/v4/version probe treated 401 as "server is alive" (status < 500). The new authenticatedTokenCheck runs after each successful reachability probe and throws a GitLab API 401 error on revocation. classifyError maps this to 'auth', and the new healthCheckErrorIsAuth guard in both healthy.checking.onError and degraded.checking.onError transitions the instance directly to `failed` state — disabling auto-reconnect until the user fixes the token. Closes #370
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an authenticated static-token probe (HEAD /api/v4/user with PRIVATE-TOKEN) to connect and periodic health-check flows when OAuth is disabled and GITLAB_TOKEN is present; a Changes
Sequence Diagram(s)sequenceDiagram
participant HC as Health Monitor
participant Quick as Quick Health Check
participant Auth as Authenticated Token Check
participant API as GitLab API
participant SM as State Machine
HC->>Quick: HEAD /api/v4/version (unauthenticated)
alt static token present & OAuth disabled
HC->>Auth: authenticatedTokenCheck(instanceUrl, timeout)
Auth->>API: HEAD /api/v4/user (PRIVATE-TOKEN)
alt 401/403
API-->>Auth: 401/403
Auth-->>HC: throw auth error
HC->>SM: healthCheckOnError(auth)
SM-->>SM: healthCheckErrorIsAuth → transition to failed
else network/timeout or 5xx/429
API-->>Auth: error/transient
Auth-->>HC: swallow / treat transient
HC->>SM: recordFailure / continue existing logic
end
else OAuth enabled or no token
HC-->>Auth: skipped
HC->>SM: continue existing health logic
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Test Coverage ReportOverall Coverage: 96.59%
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds token-revocation detection to the HealthMonitor’s periodic probes by layering an authenticated check (static-token mode only) on top of the existing unauthenticated reachability probe, so revoked tokens can transition an instance to failed without relying on tool-call traffic.
Changes:
- Add an authenticated periodic health probe (
HEAD /api/v4/user) to detect mid-session static-token revocation (401 →failed). - Extend the XState machine’s periodic health-check error handling to treat auth failures as terminal (
failed) rather than continuing normal retry behavior. - Add unit tests covering healthy/degraded → failed transitions on 401, OAuth-mode skip, swallowing network errors, and recovery via
forceReconnect; update docs to reflect the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/services/HealthMonitor.ts |
Adds authenticated token validity check and routes periodic auth failures to the terminal failed state. |
tests/unit/services/HealthMonitor.test.ts |
Adds regression tests for token revocation detection, OAuth skip behavior, and recovery via manual reconnect. |
README.md.in |
Documents mid-session token revocation detection behavior in connection resilience section. |
README.md |
Mirrors the documentation update for generated README output. |
- Change authenticated check error from "token revoked or expired" to "token invalid" — a 401 from /api/v4/user can indicate wrong token value, missing scope, or other auth failure, not only revocation - Make GITLAB_TOKEN config mock dynamic via getter - Add test: authenticatedTokenCheck skips when GITLAB_TOKEN is unset
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/HealthMonitor.ts`:
- Around line 313-324: The authenticated-probe currently only maps a 401 to a
terminal auth error so 403s are treated as success; update the HealthMonitor
authenticated probe (the block that checks response.status === 401) to treat
response.status === 403 the same way (e.g., if (response.status === 401 ||
response.status === 403) throw a dedicated auth Error), and update the
corresponding catch check (where it tests error.message.startsWith('GitLab API
error: 401')) to also recognize the 403 case (either check both '401' and '403'
or use a shared prefix like 'GitLab API error: 40') so classifyError /
healthCheckErrorIsAuth will route both statuses to the failed transition rather
than falling through to recordFailure.
In `@tests/unit/services/HealthMonitor.test.ts`:
- Around line 840-993: Add a new test case alongside the existing "token
revocation detection" specs that asserts a 403 from the authenticated endpoint
also moves the monitor to "failed": in the describe block add an it similar to
"should transition to failed when authenticated health check returns 401 (token
revoked)" but have mockFetch.mockImplementation return { status: 403, ok: false
} for urls including '/api/v4/user', then await the health check interval and
assert monitor.getState(TEST_URL) === 'failed' and
monitor.isInstanceReachable(TEST_URL) === false; use the same setup helpers
(mockInitialize, mockGetInstanceInfo, initMonitor, mockGitLabToken if needed)
and mirror the existing 401 test flow so the 403 path is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: daedd02e-974b-427c-a041-2f3e1ee0feb0
📒 Files selected for processing (4)
README.mdREADME.md.insrc/services/HealthMonitor.tstests/unit/services/HealthMonitor.test.ts
…403 regression test
…nate duplicate onError blocks
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
91-91: 🧹 Nitpick | 🔵 TrivialSame as README.md.in: consider mentioning 403 alongside 401.
The generated README has the same gap — both 401 and 403 are treated as auth failures in the implementation. Update the template (
README.md.in) and regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 91, The README text for the authenticated HEAD /api/v4/user health check only mentions 401 but the implementation treats both 401 and 403 as auth failures; update the README.md.in template to mention "401 and 403" (or "401/403") as triggering an immediate transition to failed state for the static token mode HEAD /api/v4/user check, then regenerate README.md so the documentation matches the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md.in`:
- Line 91: Update the documentation sentence that describes the authenticated
HEAD /api/v4/user health check (static token mode) to state that both 401 and
403 responses transition the instance to the `failed` state; specifically modify
the sentence referencing "HEAD /api/v4/user" and "A 401 on this check
transitions the instance to `failed` state immediately" so it mentions "401 or
403" (or "401/403") as terminal auth failures to match the implementation.
In `@src/services/HealthMonitor.ts`:
- Around line 229-233: Update the inline comment for the authenticatedTokenCheck
call to accurately describe the detection path: state that mid-session token
revocation triggers a GitLab API 401 which is detected by the
healthCheckErrorIsAuth guard by inspecting the error message directly (not via
classifyError, which maps 403 → 'permanent'), leading to '#connection.failed'
with no auto-reconnect; also note this is a no-op in OAuth mode and when
GITLAB_TOKEN is unset. Mention the exact functions involved:
authenticatedTokenCheck and healthCheckErrorIsAuth, and remove the incorrect
"classifyError → 'auth'" phrasing.
---
Duplicate comments:
In `@README.md`:
- Line 91: The README text for the authenticated HEAD /api/v4/user health check
only mentions 401 but the implementation treats both 401 and 403 as auth
failures; update the README.md.in template to mention "401 and 403" (or
"401/403") as triggering an immediate transition to failed state for the static
token mode HEAD /api/v4/user check, then regenerate README.md so the
documentation matches the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cacad74-ba73-4d22-aa89-5bd6b9d4390d
📒 Files selected for processing (5)
README.mdREADME.md.insonar-project.propertiessrc/services/HealthMonitor.tstests/unit/services/HealthMonitor.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/HealthMonitor.ts`:
- Around line 229-235: The health probe (authenticatedTokenCheck) is only
invoked from performHealthCheck, so a manual RECONNECT path (performConnect →
quickHealthCheck) can bypass token revalidation and let a revoked token flip a
connection from failed → healthy; update performConnect (or the RECONNECT
handling) to call authenticatedTokenCheck(input.instanceUrl,
HEALTH_CHECK_PROBE_MS) or an equivalent token revalidation before accepting a
RECONNECT transition (i.e., before the fast-path quickHealthCheck succeeds), and
add a regression test that simulates a 401/403 during RECONNECT to assert the
actor remains in failed until credentials are fixed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ffcbecb-6ae8-4cc8-a630-ff342ddcf90a
📒 Files selected for processing (3)
README.mdREADME.md.insrc/services/HealthMonitor.ts
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Lifts stubUserEndpointStatus from inside describe('HealthMonitor') to
true module scope so SonarCloud no longer flags it as a nested function.
Replaces not.toHaveBeenCalledWith matcher with mock.calls.some() for the
skip-path negative assertion.
…e/getActorState methods
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
… and transient only Previously all non-401/403 errors were silently swallowed, hiding programming bugs (invalid URL, bad options) that would leave the instance in healthy state with a broken probe. Now only AbortError (our own timeout) and classifyError transient failures are swallowed; unexpected errors are logged and re-thrown.
ok was hardcoded to false regardless of status. Set it to status >= 200 && status < 300 to match the real fetch contract.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/HealthMonitor.ts`:
- Around line 311-329: The probe currently only throws for 401/403 so other
non-OK responses (e.g., 429, 5xx, 405) are treated as success; update the check
after enhancedFetch(...) in HealthMonitor.ts to throw for any non-2xx response
(use response.ok or response.status !== 200/204 as appropriate) and include the
response.status in the Error message (replace the existing 401/403-only branch
or augment it to cover all non-ok statuses while keeping the specific message
for 401/403). Also add one regression test that mocks enhancedFetch to return a
non-OK status (e.g., 429 or 500) and asserts the probe path throws/enters the
catch to exercise the new behavior. Ensure references: enhancedFetch,
controller.signal, GITLAB_TOKEN, and the error throw site are updated
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94fe33f7-779b-42c0-9f24-d73a5cd76a8d
📒 Files selected for processing (2)
src/services/HealthMonitor.tstests/unit/services/HealthMonitor.test.ts
A 429 rate-limit or 5xx server error from HEAD /api/v4/user was previously treated as a successful probe, silently masking a broken status code. Now any non-2xx, non-auth response throws so the catch block can classify it as transient and swallow it correctly. Add regression tests (429/500/503) verifying the instance stays healthy when the probe returns a transient status.
|
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
polaz
deleted the
feat/#370-feathealth-detect-token-revocation-via-authenticat
branch
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.
Summary
authenticatedTokenCheck()that runsHEAD /api/v4/userwith the static token after each successful unauthenticated reachability probeskipAuth: true+ explicitPRIVATE-TOKENheader so the probe validates only the static token — session cookies cannot mask a revoked tokenparseGitLabApiError(); the newhealthCheckErrorIsAuthguard transitions the instance tofailedstate (no auto-reconnect)GITLAB_TOKENis unsethealthy.checking.onErroranddegraded.checking.onErrorshare a singlehealthCheckOnErrorconstant (eliminates duplicate blocks, fixes SonarCloud CPD)performConnectfast-path also callsauthenticatedTokenChecksoforceReconnectcannot bypass revocation detectionfromJSONextracts numeric PR number from release-please v4 JSON output, preventing bash syntax errors from markdown links in changelogProblem
The periodic health check used only an unauthenticated
HEAD /api/v4/versionprobe, which treated401as "server is alive". If a token was revoked mid-session, the monitor stayed inhealthystate indefinitely. Additionally,forceReconnecttook a fast-path that skipped token validation on reconnect, and the release workflow crashed on changelogs with markdown links.Test plan
forceReconnect, fast-path regression, transient non-auth probes (429/500/503)Closes #370