Skip to content

fix(openhands): add budget support bundle diagnostics - #1226

Open
ak684 wants to merge 5 commits into
mainfrom
alona/budget-support-bundle-diagnostics
Open

ak684 wants to merge 5 commits into
mainfrom
alona/budget-support-bundle-diagnostics

Conversation

@ak684

@ak684 ak684 commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add an openhands-budget-diagnostics support-bundle collector for OHE-3260
  • capture sanitized Enterprise budget settings, per-member overrides, schema version, and recent budget maintenance results
  • flag missing legacy cycle baselines and spend snapshots
  • query live LiteLLM team/member financial state and report desired-versus-applied caps, roster drift, and missing expected private member budgets
  • distinguish LiteLLM roster-only members on the shared team budget from members with private budget rows
  • never query or emit managed/custom LLM keys or encrypted key columns

Why

Today a customer support bundle cannot establish whether an org has the budget upgrade/reconciliation states tracked in OHE-3253 through OHE-3256. This gives Support an initial diagnostic slice that can identify missing baselines, failed maintenance, and stale LiteLLM caps without direct customer database access.

This intentionally does not attempt repair and does not fully diagnose OHE-3252 managed-key ownership; that needs a separately reviewed key-safe diagnostic or targeted operator inspection.

Validation

  • all Helm unit tests: 137 passed
  • collector Python extracted from rendered bundle and compiled successfully
  • full chart render and Helm lint passed
  • git diff --check passed
  • ran the collector read-only against the internal beta instance on Enterprise 1.59.1
    • correctly identified 13 LiteLLM roster members
    • identified 3 matching private member caps and 10 known members missing both cycle baselines and expected private budgets
    • reported the failed last reconciliation even though recent outer maintenance tasks were marked completed with zero task errors
    • emitted no key material

Tracking

Comment thread charts/openhands/Chart.yaml Outdated

@aivong-openhands aivong-openhands left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟡 Acceptable — the collector is isolated and read-only, but one core comparison conflates numeric caps with LiteLLM budget source.

[CRITICAL ISSUES]

  • [charts/openhands/templates/troubleshoot/support-bundle.yaml, Line 893] Correctness: budget_matches treats desired_max_budget_in_team == None as “the applied max budget must also be None”. That is not the reconciliation contract. In the Enterprise budget sync code, an expected member budget of None means “this user should be on the shared team budget”; readback only fails if the user is not using the shared budget. With this collector, an org with a team monthly limit but no per-user default limit will report every shared-budget member as mismatched because applied is team.max_budget, not None. The inverse can also slip through: a private membership with max_budget: null can match even though it is not sharing the team budget. Please make the match source-aware: when requires_private_budget is false, healthy should mean shared/no private budget; when it is true, healthy should require a private budget and compare the numeric private cap.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
    This is an isolated, read-only support-bundle collector, so it does not affect runtime behavior. The risk is elevated because it handles budget diagnostics and LiteLLM financial state; a false degraded result is likely to send Support down the wrong path for the exact incident class this collector is meant to diagnose.

VERDICT:
❌ Needs rework: the collector should preserve the reconciliation distinction between shared team budgets and private member budgets before this diagnostic is reliable.

KEY INSIGHT:
The diagnostic can only mirror budget reconciliation if “budget source” and “budget value” stay separate in the match logic.

This review comment was created by an AI agent (OpenHands) on behalf of the user.


Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., “Security concerns about X do not apply here because Y”). See the customization docs for the required frontmatter format.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

@ak684 ak684 changed the title feat(openhands): add budget support bundle diagnostics fix(openhands): add budget support bundle diagnostics Sep 10, 2026
@github-actions github-actions Bot added type: fix A bug fix and removed type: feat A new feature labels Sep 10, 2026

@dylan-openhands dylan-openhands left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a great add to support bundles. Approved and my review focused mainly on the avoidance of including any sensitive information in the resulting output (already noted in the comments/PR description, but was double checking)

Edit: I see the review above and commentary around it but dont feel equipped to confirm the accuracy of that claim. Any way we can get a sample support bundle? Maybe via the unstable deployment?

@ak684

ak684 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the source/value distinction in 58e6b53. Shared-budget policy now matches only when LiteLLM has no private member budget, regardless of the inherited team cap value. Private-budget policy now requires a private membership budget and then compares its numeric cap. Unknown baseline/live-spend states remain non-assertive (null). Validation: 137 Helm tests, chart lint, rendered Python compile, a five-case source-aware truth table, and a read-only run against beta (3 matching private members; 10 unknown known-members with missing baselines; no false member mismatches).

@aivong-openhands aivong-openhands left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟢 Good taste — the collector is isolated, read-only, and follows the existing support-bundle exec pattern without adding chart configuration surface area.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
    This support-bundle collector reads budget and LiteLLM financial state from customer environments, so the review focus is data exposure and diagnostic accuracy. I verified the collector avoids managed/custom LLM key columns, does not emit the LiteLLM API key, uses a read-only DB session, and the latest member_budget_matches logic preserves the shared-team-budget vs private-member-budget distinction used by Enterprise reconciliation. GitHub checks, including Helm Unit Tests and chart lint/test, are passing.

VERDICT:
✅ Worth merging: the diagnostic is scoped to observability, matches the relevant reconciliation semantics, and has coverage for rendering plus key-column regressions.

KEY INSIGHT:
The change adds useful budget reconciliation visibility without attempting repair or expanding the self-hosted chart interface.

This review was created by an AI agent (OpenHands) on behalf of the user.

@ak684

ak684 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@dylan-openhands Yes — I ran the rendered collector read-only against the internal beta deployment. Here is the sanitized summary (opaque org/user IDs removed):

{
  "budget_org_count": 1,
  "degraded_org_count": 1,
  "team_budget_matches": true,
  "member_match_counts": {"true": 3, "unknown": 10},
  "applied_budget_sources": {
    "private_membership": 3,
    "team_shared_roster_only": 10
  },
  "degraded_reasons": [
    "members_without_expected_private_budget",
    "known_member_cycle_baseline_missing",
    "last_budget_sync_failed"
  ]
}

That is the real state we were trying to make visible: the team cap is correct, the 3 members with stored baselines/private caps match, and 10 legacy known members have neither a recoverable stored baseline nor their expected private cap. No managed/custom key columns or LiteLLM credentials are selected or emitted. I can also run the same collector on a disposable replicated instance when we exercise the repair PR.

@ak684

ak684 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Additional packaging validation: this branch built successfully as an isolated Replicated release (sequence 1606) with the released Enterprise 1.59.1 image, confirming the diagnostics can ship without an Enterprise repair image.

The fresh install on replicated-01 stopped before application deployment on the shared test infrastructure's storage preflight: its nominal 200 GB EBS volume exposes only ~192.7 GiB to Kubernetes, below the 200 GiB gate. That is unrelated to this chart diff and is tracked separately in OpenHands/replicated-testing#1. The earlier rendered-collector and read-only beta validations remain green.

@ak684

ak684 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

The minimal 1Finity maintenance-line backport is now open as #1234. It targets release/0.55, retains Enterprise 1.56.0, and is green. Replicated-02 validation will exercise the exact 0.55.0 customer baseline before any release promotion.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants