feat(cloudbees): Operations Center + Feature Management enterprise support#468
Open
beng360 wants to merge 19 commits into
Open
feat(cloudbees): Operations Center + Feature Management enterprise support#468beng360 wants to merge 19 commits into
beng360 wants to merge 19 commits into
Conversation
Contributor
WalkthroughThis PR extends the CloudBees CI integration with an enterprise platform surface supporting Operations Center and Feature Management APIs. It introduces new connector clients for both services, server credential routes for managing platform credentials, extended RCA actions for cross-controller deployments and feature-flag queries, a multi-step auth UI for the three connection modes, and updated documentation with enterprise action guidance. ChangesCloudBees Enterprise Platform Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/backend/agent/tools/cloudbees_rca_tool.py`:
- Around line 157-171: The code returns a field named "warnings" mapped from the
variable error after calling oc_client.query_recent_builds_across_controllers,
which is confusing; update this by either changing the client's return name or
mapping the returned error value to a clearer local name before building the
JSON. Specifically, modify the call/response handling around
query_recent_builds_across_controllers (the tuple currently unpacked as success,
builds, error) so that the third element is named warnings (or warnings_list)
and use that when returning {"builds": builds, "count": len(builds),
"time_window_hours": time_window_hours, "warnings": warnings}, or alternatively
change the client function signature to return (success, builds, warnings)
consistently; keep oc_client.close() in the finally block unchanged.
In `@server/connectors/cloudbees_connector/fm_client.py`:
- Around line 81-89: The retry branch after receiving a 429 does not update the
rate limiter state, so update _last_request_time (or call _rate_limit_wait())
after the successful retry to reflect the retry's timestamp; specifically, in
the block around the second client.request (the retry using
client.request(method=method, url=url, params=params)), ensure you update the
module/class attribute _last_request_time (or invoke _rate_limit_wait())
immediately after the retry response is obtained and before returning or
proceeding, and preserve existing 429 handling (i.e., still return the same
error if the retry is 429).
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Around line 175-231: The time_window_hours parameter in
query_recent_builds_across_controllers is unused; filter builds by timestamp
before returning them. After collecting builds into all_builds (and before
sorting), compute a cutoff = current_time_ms - time_window_hours * 3600 * 1000
and keep only builds where build.get("timestamp", 0) >= cutoff; ensure you
handle None/missing timestamps by excluding them, preserve existing
controller/job annotations (_controller, _job), and then sort and return as
before (using MAX_CONTROLLERS and MAX_BUILDS_PER_CONTROLLER unchanged).
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Around line 366-368: The code returns the raw error from
oc_client.discover_controllers() directly in the HTTP response (the error
variable used in jsonify), which can leak sensitive info; change the response to
sanitize or replace the error with a safe message (e.g., "Failed to discover
controllers") and log the original error internally using the existing logger
before returning the generic message. Update the block that checks success from
oc_client.discover_controllers() to: log the original error
(logger.error/exception with the error variable), set a sanitized_error
string/fallback, and return jsonify({"connected": True, "controllers": [],
"error": sanitized_error}), 502 instead of returning the raw error.
- Around line 268-270: The handler currently returns the raw error from
oc_client.get_server_info() (variables success, error, user_id); change it to
use the same safe_errors whitelist logic used in the /connect route: if error is
in safe_errors return that message to the client, otherwise return a generic
"Failed to validate Operations Center credentials" message while keeping the
full error in the server log via logger.warning; ensure you reference the
safe_errors set and apply the check before building the jsonify response.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bf9f2d1-257b-425e-8746-5503dea60749
📒 Files selected for processing (6)
server/chat/backend/agent/tools/cloudbees_rca_tool.pyserver/connectors/cloudbees_connector/__init__.pyserver/connectors/cloudbees_connector/fm_client.pyserver/connectors/cloudbees_connector/oc_client.pyserver/connectors/cloudbees_connector/platform_client.pyserver/routes/cloudbees/cloudbees_routes.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/hooks/use-github-status.ts (1)
120-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove dead cleanup code for non-existent
focusevent listener.Line 120 removes a
focusevent listener that is never added in this effect (lines 114-116 show onlyproviderStateChanged,message, andvisibilitychangeare registered). This cleanup is dead code left from incomplete refactoring.🧹 Proposed fix
return () => { window.removeEventListener('providerStateChanged', handleProviderChange); window.removeEventListener('message', handleAuthMessage); - window.removeEventListener('focus', checkStatus); document.removeEventListener('visibilitychange', handleVisibility); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/hooks/use-github-status.ts` at line 120, The cleanup in the effect contains a stale call removing a 'focus' listener that is never added; remove the dead window.removeEventListener('focus', checkStatus) line and ensure the effect's cleanup only unregisters the actual listeners that were registered (e.g., providerStateChanged, the message handler, and 'visibilitychange') so that removeEventListener targets match the corresponding addEventListener calls in use-github-status (look for the providerStateChanged and checkStatus/message handler registrations).client/src/components/github-provider-integration.tsx (1)
314-321:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix GitHub metadata regeneration/pending UX (no-op polling still called)
In
client/src/components/github-provider-integration.tsx(lines 314-321),startMetadataPollingimmediately returns after clearingpollingRef(disabled polling). It’s still invoked:
- from
loadSavedRepos(around line 329) after fetching selections- from
handleRegenerate(around line 551) after settingmetadata_status: 'generating'Since the
/repo-metadata/generateendpoint enqueues async work and only updatesconnected_repos.metadata_statusin the DB, the client never re-fetchesrepo-selectionsto transition repometadata_statusfrompending/generatingtoready—so users will likely see “Generating description…” until they manually refresh/reconnect.Either re-enable a lightweight revalidation/polling for GitHub (like the GitLab connect page does), or remove
startMetadataPollingand its call sites and update the UI to reflect that only manual refresh will show completion. Also keep theissue#471`` rationale scoped to dev-only if that’s truly the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/github-provider-integration.tsx` around lines 314 - 321, startMetadataPolling currently clears pollingRef then returns, so callers (startMetadataPolling invoked from loadSavedRepos and handleRegenerate) never revalidate repo selections after triggering repo-metadata/generate, leaving connected_repos.metadata_status stuck in pending/generating; either re-enable a lightweight revalidation loop or remove the no-op and update callers/UI. Fix: modify startMetadataPolling to implement a lightweight polling/revalidation (e.g., setInterval that fetches repo selections every N seconds and clears itself when no repos are pending) or delete startMetadataPolling and remove its invocations in loadSavedRepos and handleRegenerate and ensure the UI for metadata_status explicitly indicates manual refresh is required; reference startMetadataPolling, loadSavedRepos, handleRegenerate, and connected_repos.metadata_status to locate changes and keep the issue `#471` dev-only rationale if you retain a disabled path.server/routes/cloudbees/cloudbees_routes.py (1)
344-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPAT/Bearer connections are misreported as disconnected in
/platform-status.Line 345 requires
username, but PAT mode intentionally stores OC credentials with an empty username. Valid bearer connections will showconnected: false.Suggested fix
- if oc_creds and oc_creds.get("base_url") and oc_creds.get("username") and oc_creds.get("api_token"): + if oc_creds and oc_creds.get("base_url") and oc_creds.get("api_token"): + auth_mode = (oc_creds.get("auth_mode") or "basic").lower() + has_required_identity = auth_mode == "bearer" or bool(oc_creds.get("username")) + if not has_required_identity: + return jsonify({ + "operations_center": {"connected": False}, + "feature_management": fm_status, + }) oc_status = { "connected": True, "url": oc_creds["base_url"], "username": oc_creds["username"], }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/cloudbees/cloudbees_routes.py` around lines 344 - 350, The code is incorrectly requiring a non-empty username for OC PAT/bearer auth, causing valid bearer connections to appear disconnected; update the condition that builds oc_status (where oc_creds is returned from get_token_data(CLOUDBEES_OC_PROVIDER)) to consider the credentials "connected" when base_url and api_token are present even if username is empty, and populate oc_status["username"] using oc_creds.get("username") (which may be None or empty) rather than gate connection on it; keep using oc_creds["base_url"] and oc_creds["api_token"] for validation so bearer/PAT users are reported connected.
♻️ Duplicate comments (1)
server/connectors/cloudbees_connector/oc_client.py (1)
208-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
time_window_hoursis still ignored in cross-controller queries.Line 209 accepts
time_window_hours, but returned builds are never filtered by cutoff before sorting/return. This causes stale build data in enterprise RCA results.Suggested fix
+from datetime import datetime, timedelta, timezone ... # Sort by timestamp descending + if time_window_hours > 0: + cutoff_ms = int( + (datetime.now(timezone.utc) - timedelta(hours=time_window_hours)).timestamp() * 1000 + ) + all_builds = [ + b for b in all_builds + if isinstance(b.get("timestamp"), (int, float)) and b.get("timestamp", 0) >= cutoff_ms + ] + all_builds.sort(key=lambda b: b.get("timestamp", 0), reverse=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/connectors/cloudbees_connector/oc_client.py` around lines 208 - 264, query_recent_builds_across_controllers currently accepts time_window_hours but never applies it; compute a cutoff (e.g., cutoff_ms = int(time.time() * 1000) - time_window_hours * 3600 * 1000) and filter all_builds to only include builds with build.get("timestamp", 0) >= cutoff_ms before sorting/returning; ensure time is imported (or use time.time()) and keep the filtering step just prior to the existing sort to avoid changing job/build-fetch logic inside get_controller_client/list_builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsx`:
- Around line 109-111: The fetch response mapping in the CloudBeesAuthPage
component is using the wrong path; update the handler for
fetch("/api/cloudbees/status?full=true") so it checks and uses d?.summary (not
d?.status?.summary") when calling setSummary, i.e. replace the nested status
access with the top-level summary in the .then callback that currently invokes
setSummary.
- Around line 336-343: The success UI is shown even when the PUT to
/api/cloudbees/rca-settings fails because non-2xx responses aren't checked;
update the fetch handling in the CloudBeesAuthPage component to inspect the
fetch response (e.g., response.ok) after the await
fetch("/api/cloudbees/rca-settings", ...) call and only call
setRcaEnabled(checked) and toast(...) when the response is OK; if not OK, throw
or handle the error and show an error toast instead so the toggle UI doesn't
reflect an unpersisted change.
- Around line 144-145: Replace the generic CustomEvent("providerStateChanged")
dispatches with the CloudBees-specific event required by the connector contract:
dispatch new Event("cloudBeesStateChanged") (use Event, not CustomEvent). Update
each dispatch site in CloudBeesAuthPage.tsx where window.dispatchEvent(new
CustomEvent("providerStateChanged")) is called so they instead call
window.dispatchEvent(new Event("cloudBeesStateChanged")), ensuring disconnect
and status revalidation emit the provider-specific event name.
In `@client/src/hooks/use-github-status.ts`:
- Around line 111-116: Remove the no-op visibility handler and its registration:
delete the empty handleVisibility function and the
document.addEventListener('visibilitychange', handleVisibility) line so no
unused listener is attached; ensure only the remaining listeners
(window.addEventListener('providerStateChanged', handleProviderChange') and
window.addEventListener('message', handleAuthMessage') remain unchanged to
preserve behavior.
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Around line 73-97: The current host-comparison in the controller validation
(variables oc_host, ctrl_host, oc_domain, ctrl_domain) is insecure because it
simply joins trailing labels and can be bypassed (e.g., co.uk or IP octet
suffixes); replace this heuristic with a robust check: parse both hosts, treat
IP literals separately by using ipaddress.ip_address and require exact IP
equality for IPs, and for domain names use a public suffix list-based extraction
(e.g., tldextract or publicsuffix2) to get the registrable/registered domain for
self.base_url and controller_url and compare those registered domains for
equality; ensure you raise the same ValueError when they do not match and keep
the invalid-URL guard for empty ctrl_host.
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Around line 270-274: The CloudBeesOCClient created in the route handlers
(instances in the CloudBeesOCClient creation blocks and used to call
get_server_info and similar methods around lines shown) are never closed; update
the handlers to ensure the client is always closed after use by wrapping usage
in a try/finally (or using the client's context manager if it implements
__enter__/__exit__) and call the client's close method (e.g., oc_client.close())
in the finally block; apply the same pattern to the other route path(s) where
CloudBeesOCClient is constructed (also the block around the other occurrence
noted) so connections/file descriptors are released under error and normal
execution.
---
Outside diff comments:
In `@client/src/components/github-provider-integration.tsx`:
- Around line 314-321: startMetadataPolling currently clears pollingRef then
returns, so callers (startMetadataPolling invoked from loadSavedRepos and
handleRegenerate) never revalidate repo selections after triggering
repo-metadata/generate, leaving connected_repos.metadata_status stuck in
pending/generating; either re-enable a lightweight revalidation loop or remove
the no-op and update callers/UI. Fix: modify startMetadataPolling to implement a
lightweight polling/revalidation (e.g., setInterval that fetches repo selections
every N seconds and clears itself when no repos are pending) or delete
startMetadataPolling and remove its invocations in loadSavedRepos and
handleRegenerate and ensure the UI for metadata_status explicitly indicates
manual refresh is required; reference startMetadataPolling, loadSavedRepos,
handleRegenerate, and connected_repos.metadata_status to locate changes and keep
the issue `#471` dev-only rationale if you retain a disabled path.
In `@client/src/hooks/use-github-status.ts`:
- Line 120: The cleanup in the effect contains a stale call removing a 'focus'
listener that is never added; remove the dead
window.removeEventListener('focus', checkStatus) line and ensure the effect's
cleanup only unregisters the actual listeners that were registered (e.g.,
providerStateChanged, the message handler, and 'visibilitychange') so that
removeEventListener targets match the corresponding addEventListener calls in
use-github-status (look for the providerStateChanged and checkStatus/message
handler registrations).
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Around line 344-350: The code is incorrectly requiring a non-empty username
for OC PAT/bearer auth, causing valid bearer connections to appear disconnected;
update the condition that builds oc_status (where oc_creds is returned from
get_token_data(CLOUDBEES_OC_PROVIDER)) to consider the credentials "connected"
when base_url and api_token are present even if username is empty, and populate
oc_status["username"] using oc_creds.get("username") (which may be None or
empty) rather than gate connection on it; keep using oc_creds["base_url"] and
oc_creds["api_token"] for validation so bearer/PAT users are reported connected.
---
Duplicate comments:
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Around line 208-264: query_recent_builds_across_controllers currently accepts
time_window_hours but never applies it; compute a cutoff (e.g., cutoff_ms =
int(time.time() * 1000) - time_window_hours * 3600 * 1000) and filter all_builds
to only include builds with build.get("timestamp", 0) >= cutoff_ms before
sorting/returning; ensure time is imported (or use time.time()) and keep the
filtering step just prior to the existing sort to avoid changing job/build-fetch
logic inside get_controller_client/list_builds.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbdd80f0-6a98-4e52-9a20-a7f066b715b6
⛔ Files ignored due to path filters (1)
client/public/cloudbees.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
client/src/app/api/cloudbees/connect-platform/route.tsclient/src/app/api/cloudbees/controllers/route.tsclient/src/app/api/cloudbees/platform-status/route.tsclient/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsxclient/src/app/cloudbees/auth/page.tsxclient/src/components/github-provider-integration.tsxclient/src/hooks/use-github-status.tsserver/chat/backend/agent/skills/integrations/cloudbees/SKILL.mdserver/chat/backend/agent/skills/integrations/cloudbees_oc/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/background/prediscovery_task.pyserver/connectors/cloudbees_connector/oc_client.pyserver/routes/cloudbees/cloudbees_routes.pywebsite/docs/integrations/cloudbees.md
beng360
added a commit
that referenced
this pull request
Jun 4, 2026
Backend: - Use 'with' for OC/FM client context managers in RCA tool - Remove unused 'time' import - Update rate limit tracking after 429 retry - Use time_window_hours parameter in cross-controller query - Sanitize error messages in routes (no internal hostnames) - Close OC clients properly in routes - Strengthen domain matching (prevent evil-company.com bypass) Frontend: - Remove unused imports (Button, Badge, router) - Fix status summary mapping - Dispatch providerStateChanged after connect - Only toast RCA toggle success on actual success - Remove empty visibility handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e support - CloudBeesOCClient: discovers managed controllers, queries across them - CloudBeesFMClient: queries feature flag changes for incident correlation - CloudBeesPlatformClient: orchestration layer combining OC + FM - Enhanced RCA tool: new actions (flag_changes, cross_controller_deployments, controller_list) - New routes: /connect-platform, /platform-status, /controllers, /disconnect-platform - Graceful degradation: missing credentials return helpful messages, not errors - Additive: existing single-controller flow completely unchanged Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Path injection: URL-encode app_id/flag_name in FM client - SSRF: validate controller URLs match OC domain before sending creds - Resource leak: add context manager support + close() in action handlers - Remove trace_context delegation to wrong provider - Add single retry on FM 429 responses - Cache platform_status instead of hitting FM API every call - Sanitize error messages from OC (no raw exceptions to caller) - Validate URL schemes (require http/https) - Guard against empty app_id in flag_changes action Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three connection modes in one clean flow: - Single Controller: direct Jenkins/CloudBees CI connection (existing) - Operations Center: multi-controller management with controller discovery - Personal Access Token: platform-level auth for organizations using PATs Optional Feature Management section under OC mode for flag correlation. Displays discovered controllers after successful OC connection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Critical fixes: - Field name alignment: frontend now sends oc_url/api_token/fm_api_token (matches backend) - PAT mode: backend now supports Bearer token auth when username is empty - OC client: supports both Basic Auth and Bearer token modes - Disconnect: removes both single-controller AND platform credentials - Controllers: fetched on page load from /controllers endpoint - Connect response: includes discovered controllers list Other fixes: - Client-side URL scheme validation (http/https only) - 0 controllers: shows guidance message instead of dead-end - Clear all form fields on disconnect - Remove trace_context from tool description (action doesn't exist) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove all Card wrappers and bordered containers - Mode picker: divider-separated list with hover arrows - Forms: transparent inputs, rounded-xl, generous spacing - Connected: minimal data display with status dots - Feature management: collapsible <details> instead of card section - Matches sign-in page typography and spacing philosophy - No decorative elements, trusts the content Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SKILL.md: document enterprise actions (flag_changes, cross_controller_deployments, controller_list) - Add cloudbees_oc skill entry for SkillRegistry detection - SSRF: relax validation to allow sibling subdomains (same registrable domain) - Prediscovery: add OC exploration instructions to prompt - MCP: verify cloudbees_rca is available via MCP tool registry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers all three connection modes (Single Controller, Operations Center, PAT), webhook setup, RCA actions (standard + enterprise), feature flag correlation, and auto-trigger configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Complete redesign as a multi-step wizard: - Step 1: Choose connection mode (OC, Single, PAT) with descriptive cards - Step 2: Enter credentials with inline instruction box - Step 3: Webhook setup with copy-able URL and Jenkinsfile snippet - Connected: Controller list + 'what happens next' section - Progress bar tracks position through the flow - Full max-w-2xl width, generous spacing, no cramped forms Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… webhook, deployments) Connected state is now a full dashboard: - 4-column stats grid (jobs, nodes, executors, queue) - Job health bar with colored segments + legend - RCA auto-trigger toggle switch - Webhook URL with copy + Jenkinsfile snippet (collapsible) - Recent deployments list with status dots - Controllers list (OC mode) - All in the same dark minimal aesthetic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refs #471 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ctions - Removed all trace_context references (action no longer exists) - Flag_changes: explicitly states 'only call if FM is connected + have app_id' - cross_controller_deployments: only if OC is connected - Prevents agent from calling actions that will always error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…DBEES Hidden by default (.env.example=false). Set NEXT_PUBLIC_ENABLE_CLOUDBEES=true to show. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Backend: - Use 'with' for OC/FM client context managers in RCA tool - Remove unused 'time' import - Update rate limit tracking after 429 retry - Use time_window_hours parameter in cross-controller query - Sanitize error messages in routes (no internal hostnames) - Close OC clients properly in routes - Strengthen domain matching (prevent evil-company.com bypass) Frontend: - Remove unused imports (Button, Badge, router) - Fix status summary mapping - Dispatch providerStateChanged after connect - Only toast RCA toggle success on actual success - Remove empty visibility handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
beng360
force-pushed
the
feat/cloudbees-enterprise
branch
from
9823bad to
e5e5617
Compare
…use HTTP) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
server/connectors/cloudbees_connector/oc_client.py (1)
71-108:⚠️ Potential issue | 🟠 MajorReview:
_validate_controller_urluses a fragile registrable-domain heuristic; switch to public-suffix extraction
server/connectors/cloudbees_connector/oc_client.pycomputes the “base domain” via alen(oc_parts[-2]) <= 3heuristic and then allows controllers by suffix match. Repo-wide search shows notldextract(or equivalent public-suffix/registrable-domain helper) used, so this approach isn’t robust for complex/multi-part public suffixes and can miscompute the allowlist boundary.Use a public-suffix–based registrable-domain extractor (e.g.,
tldextractor another PSList-backed library) to derive the registrable domain, then perform the suffix check from that value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/connectors/cloudbees_connector/oc_client.py` around lines 71 - 108, The current _validate_controller_url in oc_client.py uses a fragile heuristic on oc_host parts to compute a base domain; replace that with a public-suffix-aware registrable domain extractor (e.g., tldextract). Import and use tldextract.extract(self.base_url_hostname).registered_domain (or similar API) to derive oc_registrable_domain (fall back to oc_host when empty), and then validate controller_url by parsing ctrl_host and ensuring either ctrl_host == oc_host or ctrl_host == oc_registrable_domain or ctrl_host.endswith("." + oc_registrable_domain). Update _validate_controller_url to use these names (_validate_controller_url, self.base_url, oc_host, ctrl_host) and add the tldextract dependency to the project requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsx`:
- Around line 966-975: The list rendering in CloudBeesAuthPage uses the array
index `i` as the React key which can break reconciliation; update the key on the
map over `deployments.slice(0, 5).map((d, i) => ...)` to use a stable unique
identifier from each deployment (e.g., `d.id` or `d.build_number` if `id` is not
available) instead of `i`, ensuring the outer div's `key` prop is changed to
that unique field so React can track items reliably.
In `@server/chat/backend/agent/tools/cloudbees_rca_tool.py`:
- Around line 136-167: Normalize and validate time_window_hours before calling
FM/OC methods: in the "flag_changes" and "cross_controller_deployments"
branches, check the time_window_hours variable (used when calling
fm_client.get_recent_flag_changes and
oc_client.query_recent_builds_across_controllers) and if it's None, non-numeric,
or <= 0 return a structured json error (e.g., {"error": "time_window_hours must
be a positive integer"}); alternatively coerce valid string/float input to an
int > 0 and use that value for the calls. Ensure this validation happens before
obtaining/using fm_client or oc_client so you don't open connections
unnecessarily.
In `@server/chat/background/prediscovery_task.py`:
- Around line 204-213: The preflight org lookup and hook invocation
(get_org_id_for_user(user_id) and get_hook("before_llm_call")(org_id, user_id))
run outside the task's try/except so exceptions bypass the function’s structured
failure path; move these calls into the existing try block (or wrap them in
their own try/except that funnels exceptions into the same failure return),
ensure logger.warning usage remains but that any exception from
get_org_id_for_user or get_hook is caught and returned as the function's
standard error response (e.g., {"status": "error", "error": ...}) so the task
always follows the structured failure handling.
In `@server/connectors/cloudbees_connector/fm_client.py`:
- Around line 106-108: Replace the logger.error call in the HTTP exception
handler that catches httpx.HTTPError with logger.exception so the traceback is
preserved; specifically, in the except httpx.HTTPError as e block in
fm_client.py (the Feature Management API request handler) change
logger.error("FM API request failed: %s", e) to logger.exception("FM API request
failed: %s", e) and keep the existing return False, None, "Feature Management
API request failed." so the error is logged with its stack trace while the
function behavior remains the same.
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Line 263: The tuple returned by client.list_builds(...) unpacks into ok,
builds, err but err is never used; change the unpack to use a deliberately
ignored name (e.g., _err or _) so the unused variable is clearly intentional.
Update the call site in oc_client.py where client.list_builds(job_name,
limit=MAX_BUILDS_PER_CONTROLLER) is assigned (the ok, builds, err unpack) to use
ok, builds, _err (or ok, builds, _) instead, leaving the rest of the logic
unchanged.
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Line 275: In the unpacking statements where oc_client.get_server_info() (and
similar calls at the other noted spots) return multiple values, rename the
unused variables server_data, ctrl_error, and error to _ (or _server_data/_error
if you prefer distinct discard names) so linters stop flagging them as unused;
update the unpacking near the oc_client.get_server_info() call and the analogous
unpackings referenced around the other occurrences to use the _ placeholder for
discarded values while keeping the used variable (e.g., success) intact.
- Around line 354-361: The platform-status check incorrectly requires
oc_creds.get("username") to be truthy which fails for PAT mode where username is
stored as an empty string; update the check in the block that reads oc_creds =
get_token_data(user_id, CLOUDBEES_OC_PROVIDER) to treat a missing/empty username
as valid for PAT by only requiring oc_creds["base_url"] and
oc_creds["api_token"] (or explicitly checking that "username" key exists even if
empty), and when building oc_status ensure you use oc_creds.get("username")
(allowing empty string) or fall back to None/empty string so OC shows connected
for PAT users.
In `@website/docs/integrations/cloudbees.md`:
- Around line 96-97: Update the documentation for the `flag_changes` tool to
state that `app_id` is required in addition to Feature Management; specifically
edit the `flag_changes` bullet to mention that callers must provide `app_id`
(e.g., "requires Feature Management and a valid app_id") so readers know to pass
`app_id` to avoid the immediate tool error when invoking `flag_changes`.
---
Duplicate comments:
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Around line 71-108: The current _validate_controller_url in oc_client.py uses
a fragile heuristic on oc_host parts to compute a base domain; replace that with
a public-suffix-aware registrable domain extractor (e.g., tldextract). Import
and use tldextract.extract(self.base_url_hostname).registered_domain (or similar
API) to derive oc_registrable_domain (fall back to oc_host when empty), and then
validate controller_url by parsing ctrl_host and ensuring either ctrl_host ==
oc_host or ctrl_host == oc_registrable_domain or ctrl_host.endswith("." +
oc_registrable_domain). Update _validate_controller_url to use these names
(_validate_controller_url, self.base_url, oc_host, ctrl_host) and add the
tldextract dependency to the project requirements.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60a1b57e-dafb-4510-9c4f-50e1f6feca4a
⛔ Files ignored due to path filters (1)
client/public/cloudbees.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
.env.exampleclient/src/app/api/cloudbees/connect-platform/route.tsclient/src/app/api/cloudbees/controllers/route.tsclient/src/app/api/cloudbees/platform-status/route.tsclient/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsxclient/src/app/cloudbees/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/github-provider-integration.tsxclient/src/hooks/use-github-status.tsclient/src/lib/feature-flags.tsserver/chat/backend/agent/skills/integrations/cloudbees/SKILL.mdserver/chat/backend/agent/skills/integrations/cloudbees_oc/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/cloudbees_rca_tool.pyserver/chat/background/prediscovery_task.pyserver/connectors/cloudbees_connector/__init__.pyserver/connectors/cloudbees_connector/fm_client.pyserver/connectors/cloudbees_connector/oc_client.pyserver/connectors/cloudbees_connector/platform_client.pyserver/routes/cloudbees/cloudbees_routes.pywebsite/docs/integrations/cloudbees.md
💤 Files with no reviewable changes (1)
- client/src/hooks/use-github-status.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
♻️ Duplicate comments (1)
server/connectors/cloudbees_connector/oc_client.py (1)
71-108:⚠️ Potential issue | 🟠 MajorReview:
_validate_controller_urluses a fragile registrable-domain heuristic; switch to public-suffix extraction
server/connectors/cloudbees_connector/oc_client.pycomputes the “base domain” via alen(oc_parts[-2]) <= 3heuristic and then allows controllers by suffix match. Repo-wide search shows notldextract(or equivalent public-suffix/registrable-domain helper) used, so this approach isn’t robust for complex/multi-part public suffixes and can miscompute the allowlist boundary.Use a public-suffix–based registrable-domain extractor (e.g.,
tldextractor another PSList-backed library) to derive the registrable domain, then perform the suffix check from that value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/connectors/cloudbees_connector/oc_client.py` around lines 71 - 108, The current _validate_controller_url in oc_client.py uses a fragile heuristic on oc_host parts to compute a base domain; replace that with a public-suffix-aware registrable domain extractor (e.g., tldextract). Import and use tldextract.extract(self.base_url_hostname).registered_domain (or similar API) to derive oc_registrable_domain (fall back to oc_host when empty), and then validate controller_url by parsing ctrl_host and ensuring either ctrl_host == oc_host or ctrl_host == oc_registrable_domain or ctrl_host.endswith("." + oc_registrable_domain). Update _validate_controller_url to use these names (_validate_controller_url, self.base_url, oc_host, ctrl_host) and add the tldextract dependency to the project requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsx`:
- Around line 966-975: The list rendering in CloudBeesAuthPage uses the array
index `i` as the React key which can break reconciliation; update the key on the
map over `deployments.slice(0, 5).map((d, i) => ...)` to use a stable unique
identifier from each deployment (e.g., `d.id` or `d.build_number` if `id` is not
available) instead of `i`, ensuring the outer div's `key` prop is changed to
that unique field so React can track items reliably.
In `@server/chat/backend/agent/tools/cloudbees_rca_tool.py`:
- Around line 136-167: Normalize and validate time_window_hours before calling
FM/OC methods: in the "flag_changes" and "cross_controller_deployments"
branches, check the time_window_hours variable (used when calling
fm_client.get_recent_flag_changes and
oc_client.query_recent_builds_across_controllers) and if it's None, non-numeric,
or <= 0 return a structured json error (e.g., {"error": "time_window_hours must
be a positive integer"}); alternatively coerce valid string/float input to an
int > 0 and use that value for the calls. Ensure this validation happens before
obtaining/using fm_client or oc_client so you don't open connections
unnecessarily.
In `@server/chat/background/prediscovery_task.py`:
- Around line 204-213: The preflight org lookup and hook invocation
(get_org_id_for_user(user_id) and get_hook("before_llm_call")(org_id, user_id))
run outside the task's try/except so exceptions bypass the function’s structured
failure path; move these calls into the existing try block (or wrap them in
their own try/except that funnels exceptions into the same failure return),
ensure logger.warning usage remains but that any exception from
get_org_id_for_user or get_hook is caught and returned as the function's
standard error response (e.g., {"status": "error", "error": ...}) so the task
always follows the structured failure handling.
In `@server/connectors/cloudbees_connector/fm_client.py`:
- Around line 106-108: Replace the logger.error call in the HTTP exception
handler that catches httpx.HTTPError with logger.exception so the traceback is
preserved; specifically, in the except httpx.HTTPError as e block in
fm_client.py (the Feature Management API request handler) change
logger.error("FM API request failed: %s", e) to logger.exception("FM API request
failed: %s", e) and keep the existing return False, None, "Feature Management
API request failed." so the error is logged with its stack trace while the
function behavior remains the same.
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Line 263: The tuple returned by client.list_builds(...) unpacks into ok,
builds, err but err is never used; change the unpack to use a deliberately
ignored name (e.g., _err or _) so the unused variable is clearly intentional.
Update the call site in oc_client.py where client.list_builds(job_name,
limit=MAX_BUILDS_PER_CONTROLLER) is assigned (the ok, builds, err unpack) to use
ok, builds, _err (or ok, builds, _) instead, leaving the rest of the logic
unchanged.
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Line 275: In the unpacking statements where oc_client.get_server_info() (and
similar calls at the other noted spots) return multiple values, rename the
unused variables server_data, ctrl_error, and error to _ (or _server_data/_error
if you prefer distinct discard names) so linters stop flagging them as unused;
update the unpacking near the oc_client.get_server_info() call and the analogous
unpackings referenced around the other occurrences to use the _ placeholder for
discarded values while keeping the used variable (e.g., success) intact.
- Around line 354-361: The platform-status check incorrectly requires
oc_creds.get("username") to be truthy which fails for PAT mode where username is
stored as an empty string; update the check in the block that reads oc_creds =
get_token_data(user_id, CLOUDBEES_OC_PROVIDER) to treat a missing/empty username
as valid for PAT by only requiring oc_creds["base_url"] and
oc_creds["api_token"] (or explicitly checking that "username" key exists even if
empty), and when building oc_status ensure you use oc_creds.get("username")
(allowing empty string) or fall back to None/empty string so OC shows connected
for PAT users.
In `@website/docs/integrations/cloudbees.md`:
- Around line 96-97: Update the documentation for the `flag_changes` tool to
state that `app_id` is required in addition to Feature Management; specifically
edit the `flag_changes` bullet to mention that callers must provide `app_id`
(e.g., "requires Feature Management and a valid app_id") so readers know to pass
`app_id` to avoid the immediate tool error when invoking `flag_changes`.
---
Duplicate comments:
In `@server/connectors/cloudbees_connector/oc_client.py`:
- Around line 71-108: The current _validate_controller_url in oc_client.py uses
a fragile heuristic on oc_host parts to compute a base domain; replace that with
a public-suffix-aware registrable domain extractor (e.g., tldextract). Import
and use tldextract.extract(self.base_url_hostname).registered_domain (or similar
API) to derive oc_registrable_domain (fall back to oc_host when empty), and then
validate controller_url by parsing ctrl_host and ensuring either ctrl_host ==
oc_host or ctrl_host == oc_registrable_domain or ctrl_host.endswith("." +
oc_registrable_domain). Update _validate_controller_url to use these names
(_validate_controller_url, self.base_url, oc_host, ctrl_host) and add the
tldextract dependency to the project requirements.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60a1b57e-dafb-4510-9c4f-50e1f6feca4a
⛔ Files ignored due to path filters (1)
client/public/cloudbees.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
.env.exampleclient/src/app/api/cloudbees/connect-platform/route.tsclient/src/app/api/cloudbees/controllers/route.tsclient/src/app/api/cloudbees/platform-status/route.tsclient/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsxclient/src/app/cloudbees/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/github-provider-integration.tsxclient/src/hooks/use-github-status.tsclient/src/lib/feature-flags.tsserver/chat/backend/agent/skills/integrations/cloudbees/SKILL.mdserver/chat/backend/agent/skills/integrations/cloudbees_oc/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/cloudbees_rca_tool.pyserver/chat/background/prediscovery_task.pyserver/connectors/cloudbees_connector/__init__.pyserver/connectors/cloudbees_connector/fm_client.pyserver/connectors/cloudbees_connector/oc_client.pyserver/connectors/cloudbees_connector/platform_client.pyserver/routes/cloudbees/cloudbees_routes.pywebsite/docs/integrations/cloudbees.md
💤 Files with no reviewable changes (1)
- client/src/hooks/use-github-status.ts
🛑 Comments failed to post (8)
client/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsx (1)
966-975: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use a stable key instead of array index for deployment list items.
Using array index
ias the React key can cause rendering issues if the list is reordered or items are removed. If deployments have a uniqueidfield, use that instead.🔧 Proposed fix
- {deployments.slice(0, 5).map((d, i) => ( - <div key={i} className="flex items-center justify-between py-2.5 px-4 rounded-xl bg-white/[0.02]"> + {deployments.slice(0, 5).map((d) => ( + <div key={d.id || `${d.service}-${d.build_number}`} className="flex items-center justify-between py-2.5 px-4 rounded-xl bg-white/[0.02]">🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 967-967: Do not use Array index in keys
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/cloudbees/auth/components/CloudBeesAuthPage.tsx` around lines 966 - 975, The list rendering in CloudBeesAuthPage uses the array index `i` as the React key which can break reconciliation; update the key on the map over `deployments.slice(0, 5).map((d, i) => ...)` to use a stable unique identifier from each deployment (e.g., `d.id` or `d.build_number` if `id` is not available) instead of `i`, ensuring the outer div's `key` prop is changed to that unique field so React can track items reliably.server/chat/backend/agent/tools/cloudbees_rca_tool.py (1)
136-167:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize and validate
time_window_hoursbefore OC/FM calls.Line 149 and Line 163 pass
time_window_hoursstraight into methods that do numeric time math. IfNone(or non-positive) arrives, these branches can fail at runtime instead of returning a structured tool error.💡 Suggested patch
def cloudbees_rca( @@ user_id = kwargs.get("user_id", "") @@ + window_hours = ( + time_window_hours + if isinstance(time_window_hours, int) and time_window_hours > 0 + else 24 + ) + # --- Enterprise actions (OC / FM) --- if action == "flag_changes": @@ success, changes, error = fm_client.get_recent_flag_changes( - app_id, since_hours=time_window_hours + app_id, since_hours=window_hours ) @@ - return json.dumps({"flag_changes": changes, "count": len(changes), "time_window_hours": time_window_hours}) + return json.dumps({"flag_changes": changes, "count": len(changes), "time_window_hours": window_hours}) @@ success, builds, error = oc_client.query_recent_builds_across_controllers( - service=service, time_window_hours=time_window_hours + service=service, time_window_hours=window_hours ) @@ - return json.dumps({"builds": builds, "count": len(builds), "time_window_hours": time_window_hours, "warnings": error}) + return json.dumps({"builds": builds, "count": len(builds), "time_window_hours": window_hours, "warnings": error})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/cloudbees_rca_tool.py` around lines 136 - 167, Normalize and validate time_window_hours before calling FM/OC methods: in the "flag_changes" and "cross_controller_deployments" branches, check the time_window_hours variable (used when calling fm_client.get_recent_flag_changes and oc_client.query_recent_builds_across_controllers) and if it's None, non-numeric, or <= 0 return a structured json error (e.g., {"error": "time_window_hours must be a positive integer"}); alternatively coerce valid string/float input to an int > 0 and use that value for the calls. Ensure this validation happens before obtaining/using fm_client or oc_client so you don't open connections unnecessarily.server/chat/background/prediscovery_task.py (1)
204-213:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove hook preflight into guarded error handling.
Line 206 and Line 210 execute before the
tryblock. If org lookup or hook resolution raises, the task exits without the function’s structured failure response path.💡 Suggested patch
- from utils.auth.stateless_auth import get_org_id_for_user - from datetime import datetime, timezone - org_id = get_org_id_for_user(user_id) - - # Hook: check if LLM call is allowed - from utils.hooks import get_hook - hook_allowed, hook_message = get_hook("before_llm_call")(org_id, user_id) - if not hook_allowed: - logger.warning(f"[Prediscovery] Hook blocked for user {user_id}: {hook_message}") - return {"status": "hook_blocked", "error": hook_message} - try: + from utils.auth.stateless_auth import get_org_id_for_user + from datetime import datetime, timezone + from utils.hooks import get_hook + + org_id = get_org_id_for_user(user_id) + hook_allowed, hook_message = get_hook("before_llm_call")(org_id, user_id) + if not hook_allowed: + logger.warning("[Prediscovery] Hook blocked for user %s: %s", user_id, hook_message) + return {"status": "hook_blocked", "error": hook_message} + providers = get_user_providers(user_id)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 212-212: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/background/prediscovery_task.py` around lines 204 - 213, The preflight org lookup and hook invocation (get_org_id_for_user(user_id) and get_hook("before_llm_call")(org_id, user_id)) run outside the task's try/except so exceptions bypass the function’s structured failure path; move these calls into the existing try block (or wrap them in their own try/except that funnels exceptions into the same failure return), ensure logger.warning usage remains but that any exception from get_org_id_for_user or get_hook is caught and returned as the function's standard error response (e.g., {"status": "error", "error": ...}) so the task always follows the structured failure handling.server/connectors/cloudbees_connector/fm_client.py (1)
106-108: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use
logging.exceptionto preserve stack trace.When catching exceptions,
logging.exceptionautomatically includes the traceback, which aids debugging.♻️ Proposed fix
except httpx.HTTPError as e: - logger.error("FM API request failed: %s", e) + logger.exception("FM API request failed: %s", e) return False, None, "Feature Management API request failed."🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 107-107: Use "logging.exception()" instead.
🪛 Ruff (0.15.15)
[warning] 107-107: Use
logging.exceptioninstead oflogging.errorReplace with
exception(TRY400)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/connectors/cloudbees_connector/fm_client.py` around lines 106 - 108, Replace the logger.error call in the HTTP exception handler that catches httpx.HTTPError with logger.exception so the traceback is preserved; specifically, in the except httpx.HTTPError as e block in fm_client.py (the Feature Management API request handler) change logger.error("FM API request failed: %s", e) to logger.exception("FM API request failed: %s", e) and keep the existing return False, None, "Feature Management API request failed." so the error is logged with its stack trace while the function behavior remains the same.server/connectors/cloudbees_connector/oc_client.py (1)
263-263: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Unused variable
errshould be prefixed with underscore.The
errvariable fromclient.list_builds()is unpacked but never used.♻️ Proposed fix
- ok, builds, err = client.list_builds(job_name, limit=MAX_BUILDS_PER_CONTROLLER) + ok, builds, _err = client.list_builds(job_name, limit=MAX_BUILDS_PER_CONTROLLER)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ok, builds, _err = client.list_builds(job_name, limit=MAX_BUILDS_PER_CONTROLLER)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 263-263: Unpacked variable
erris never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/connectors/cloudbees_connector/oc_client.py` at line 263, The tuple returned by client.list_builds(...) unpacks into ok, builds, err but err is never used; change the unpack to use a deliberately ignored name (e.g., _err or _) so the unused variable is clearly intentional. Update the call site in oc_client.py where client.list_builds(job_name, limit=MAX_BUILDS_PER_CONTROLLER) is assigned (the ok, builds, err unpack) to use ok, builds, _err (or ok, builds, _) instead, leaving the rest of the logic unchanged.server/routes/cloudbees/cloudbees_routes.py (2)
275-275: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Rename unused unpacked variables to
_to satisfy linters.Static analysis flags
server_data,ctrl_error, anderroras unused. Prefix them with_to indicate intentional discard.🔧 Proposed fix
- success, server_data, error = oc_client.get_server_info() + success, _server_data, error = oc_client.get_server_info()- ctrl_success, ctrl_list, ctrl_error = oc_client.discover_controllers() + ctrl_success, ctrl_list, _ctrl_error = oc_client.discover_controllers()- success, controllers, error = oc_client.discover_controllers() + success, controllers, _error = oc_client.discover_controllers()Also applies to: 305-305, 396-396
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 275-275: Replace the unused local variable "server_data" with "_".
🪛 Ruff (0.15.15)
[warning] 275-275: Unpacked variable
server_datais never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/cloudbees/cloudbees_routes.py` at line 275, In the unpacking statements where oc_client.get_server_info() (and similar calls at the other noted spots) return multiple values, rename the unused variables server_data, ctrl_error, and error to _ (or _server_data/_error if you prefer distinct discard names) so linters stop flagging them as unused; update the unpacking near the oc_client.get_server_info() call and the analogous unpackings referenced around the other occurrences to use the _ placeholder for discarded values while keeping the used variable (e.g., success) intact.
354-361:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPAT mode connection will always report
not connectedin/platform-status.Line 356 requires
oc_creds.get("username")to be truthy, but PAT mode stores an empty string for username (Line 291). This causes platform status to incorrectly report OC as disconnected for PAT-authenticated users.🔧 Proposed fix
# Check OC credentials exist oc_creds = get_token_data(user_id, CLOUDBEES_OC_PROVIDER) - if oc_creds and oc_creds.get("base_url") and oc_creds.get("username") and oc_creds.get("api_token"): + if oc_creds and oc_creds.get("base_url") and oc_creds.get("api_token"): + # Note: username may be empty for PAT/bearer auth mode oc_status = { "connected": True, "url": oc_creds["base_url"], - "username": oc_creds["username"], + "username": oc_creds.get("username") or None, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/cloudbees/cloudbees_routes.py` around lines 354 - 361, The platform-status check incorrectly requires oc_creds.get("username") to be truthy which fails for PAT mode where username is stored as an empty string; update the check in the block that reads oc_creds = get_token_data(user_id, CLOUDBEES_OC_PROVIDER) to treat a missing/empty username as valid for PAT by only requiring oc_creds["base_url"] and oc_creds["api_token"] (or explicitly checking that "username" key exists even if empty), and when building oc_status ensure you use oc_creds.get("username") (allowing empty string) or fall back to None/empty string so OC shows connected for PAT users.website/docs/integrations/cloudbees.md (1)
96-97:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
app_idas required forflag_changes.The current bullet says Feature Management is required, but callers also need
app_idto avoid an immediate tool error.💡 Suggested patch
-- `flag_changes` — Recent feature flag toggles (requires Feature Management) +- `flag_changes` — Recent feature flag toggles (requires Feature Management and `app_id`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/integrations/cloudbees.md` around lines 96 - 97, Update the documentation for the `flag_changes` tool to state that `app_id` is required in addition to Feature Management; specifically edit the `flag_changes` bullet to mention that callers must provide `app_id` (e.g., "requires Feature Management and a valid app_id") so readers know to pass `app_id` to avoid the immediate tool error when invoking `flag_changes`.
These belong in a separate PR (refs #471). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/cloudbees/cloudbees_routes.py (1)
354-361:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPAT mode connections incorrectly reported as disconnected.
The status check on line 356 requires
oc_creds.get("username")to be truthy. However, PAT mode stores an empty string for username (line 291 inconnect_platform), and""evaluates toFalsein Python. This causes all PAT-authenticated OC connections to appear disconnected.Proposed fix
# Check OC credentials exist oc_creds = get_token_data(user_id, CLOUDBEES_OC_PROVIDER) - if oc_creds and oc_creds.get("base_url") and oc_creds.get("username") and oc_creds.get("api_token"): + if oc_creds and oc_creds.get("base_url") and oc_creds.get("api_token"): + # PAT mode stores empty username, so don't require it for connection status oc_status = { "connected": True, "url": oc_creds["base_url"], - "username": oc_creds["username"], + "username": oc_creds.get("username") or None, + "auth_mode": oc_creds.get("auth_mode", "basic"), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/cloudbees/cloudbees_routes.py` around lines 354 - 361, The connection check incorrectly treats an empty string username (used for PAT mode in connect_platform) as falsy; update the conditional that builds oc_status to accept an existing username key even if empty by replacing the truthiness check oc_creds.get("username") with a presence/null check (e.g., oc_creds.get("username") is not None or "username" in oc_creds) so PAT-authenticated OC connections with "" usernames are reported connected when base_url and api_token exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/routes/cloudbees/cloudbees_routes.py`:
- Around line 354-361: The connection check incorrectly treats an empty string
username (used for PAT mode in connect_platform) as falsy; update the
conditional that builds oc_status to accept an existing username key even if
empty by replacing the truthiness check oc_creds.get("username") with a
presence/null check (e.g., oc_creds.get("username") is not None or "username" in
oc_creds) so PAT-authenticated OC connections with "" usernames are reported
connected when base_url and api_token exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 820ee76a-9b00-4286-b312-e096601cd482
📒 Files selected for processing (1)
server/routes/cloudbees/cloudbees_routes.py
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.
Summary
Enhances the CloudBees connector from a simple Jenkins reskin to full enterprise support with Operations Center multi-controller management and Feature Management flag correlation.
What's new
Operations Center Client
Feature Management Client
Enhanced RCA Tool — 3 new actions
flag_changes— "show me recent feature flag toggles that might correlate with this incident"cross_controller_deployments— "what deployed across all controllers in the last 4 hours?"controller_list— "what controllers are managed by this Operations Center?"New routes
POST /connect-platform— connect OC + FM credentialsGET /platform-status— check what's connectedGET /controllers— list discovered controllersPOST /disconnect-platform— remove platform credentialsArchitecture
Security hardening (from review)
app_id/flag_nameURL-encoded viaurllib.parse.quoteBackwards compatibility
/connect, all existing RCA actions) completely unchangeduser_tokenstable with new provider names)Test plan
flag_changesaction — verify flag data returnedcross_controller_deployments— verify cross-controller aggregation🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Configuration