fix(budgets): reject personal workspaces in get_user_budget_row

[aivong-openhands]

HUMAN:

• A human has tested these changes.

AGENT:

---

Why

get_user_budget_row was the one budget entry point with no personal-workspace check. get_budget_state, update_budget_settings, upsert_user_override and delete_user_override all begin with _reject_personal_org; this one went straight to _get_or_create_settings, so reading a user row for a personal workspace created the OrgBudgetSettings row those four exist to prevent — the same row migration 148 exists to delete, and the row an existing test asserts must never appear.

Today the only route to it runs after upsert_user_override has already rejected personal orgs, so this is a missing guard rather than a live leak. It is worth closing because nothing stops the next caller from reaching it unguarded, and the failure is silent: a stray settings row for a personal workspace, not an error.

Summary

• Call _reject_personal_org at the top of get_user_budget_row, matching the other four entry points.
• Un-skip the reproduction test, which asserts the 400 and that no settings row is written.

Issue Number

N/A

How to Test

.venv/bin/python -m pytest -q \
  tests/unit/test_org_budget_service.py \
  tests/unit/server/routes/test_orgs.py

Expect 168 passed, 6 skipped. Removing the guard makes test_user_budget_row_rejects_personal_org_without_creating_settings fail, leaving a settings row behind for the personal workspace.

Video/Screenshots

N/A — no UI change.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

One of a set of draft PRs, each carrying a single defect the Quint model for org budgets surfaced, together with the reproduction test that was already committed but skipped.

🤖 Generated with Claude Code

---

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-bb03c58

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
server/services
org_budget_service.py					1146-1148
Project Total

This report was generated by python-coverage-comment-action

[aivong-openhands]

🟢 Taste Rating: Good taste

One line, added where four sibling entry points already have it. The guard goes before _get_or_create_settings, which is the only ordering that actually prevents the write. The un-skipped test asserts both the 400 and the absence of the settings row, so it fails if the guard moves or disappears. Nothing to argue with.

[IMPROVEMENT OPPORTUNITIES] (non-blocking)

• [server/services/org_budget_service.py, _get_or_create_settings] Special Case: This PR is the fifth copy of the same guard. The underlying shape is that a read path (get_user_budget_row, get_budget_state) calls a helper that writes. Every new entry point has to remember the guard, and forgetting it fails silently — exactly the defect this PR closes. The design that eliminates the special case is splitting the helper: a read-only accessor for read paths, and _get_or_create_settings only where a row genuinely must exist. Then the guard is load-bearing in one place instead of five. Out of scope for a one-line bug fix, but worth a follow-up issue rather than a sixth copy later.

• [server/routes/orgs.py:1311-1317] upsert_org_budget_override now runs _is_personal_org twice per request (once inside upsert_user_override, once inside get_user_budget_row). One extra indexed single-row lookup on a low-traffic admin endpoint — not worth changing, just noting it is intentional and not free.

[TESTING GAPS]

None. test_user_budget_row_rejects_personal_org_without_creating_settings exercises the real service against a real Postgres session and asserts on state (no settings row) rather than on mock calls. It is a genuine regression test.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Adds a rejection to a path that, per the PR's own analysis, is only reachable today behind an identical guard — so no live caller changes behaviour. The blast radius if that analysis is wrong is a 400 on an admin-only budgets read, not data loss. CI is green across all checks.

One note on evidence: the How to Test block is a pytest invocation, and test output alone is normally not accepted as proof of a working change. It is acceptable here because the change is a guard whose entire observable behaviour is the assertion (400 raised, no row written) — there is no separate runtime artifact to produce. No UI change, so no screenshot is expected.

VERDICT:
✅ Worth merging: The fix is correct, minimal, and properly pinned.

KEY INSIGHT:
The bug is not the missing guard, it is that a read helper writes; this PR correctly patches the symptom, and the root cause deserves its own follow-up before a sixth caller forgets.

---

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.

---

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands.

[aivong-openhands]

Filed the architectural follow-up from the review as #430 — splitting _get_or_create_settings into a read-only accessor and a create path, so the personal-workspace guard is load-bearing in one place instead of six. It also notes get_reconciliation_state as the one remaining unguarded caller (safe today only because its sole route calls it after delete_user_override).

Leaving this PR as the minimal one-line fix.

---

This comment was created by an AI agent (OpenHands) on behalf of @aivong-openhands.

[aivong-openhands]

Mutation review of the tests in this PR

I hand-wrote a small mutant set against the one test this PR un-skips
(test_user_budget_row_rejects_personal_org_without_creating_settings) and ran
each against a pristine copy of the worktree. Baseline: 1 passed in ~0.5s.

Controls — the fix is genuinely asserted

Mutant	Result
C1 delete the new _reject_personal_org guard (revert the fix)	❌ caught
C2 keep the guard but move it after _get_or_create_settings — the 400 still raises, but the personal-org settings row gets written first	❌ caught

Both controls die, and C2 is the one that matters: your test doesn't stop at
the status code, it also asserts scalar_one_or_none() is None on
OrgBudgetSettings for the personal org. That second assertion is what pins the
ordering — the guard has to run before _get_or_create_settings, not merely
somewhere in the method. Without it, a future refactor could reintroduce exactly
the stray-settings-row defect (the one migration 148 exists to delete) while
still returning a 400, and the test would stay green. Asserting the absence of
the row rather than just the exception is what makes this suite carry the PR's
actual claim ("without creating settings"), so nice.

Survivors

Mutant	Result
M1 change the label argument _reject_personal_org(org_id, 'get_user_budget_row') → _reject_personal_org(org_id, None)	✅ survived

M1 — I think this is an equivalent mutant, not a real gap

Dropping the quint_action label means quint_oracle.log(..., outcome='rejected')
is never called for this entry point. But in the shipped image quint_oracle is
the no-op SimpleNamespace fallback (quint-specs/ isn't vendored into the
app image), so this argument has no observable production behaviour — the
log call is lambda *a, **k: None either way. The label only does anything under
the Quint model-checking harness. So I'd call M1 an equivalent mutant rather than
a test gap: there's nothing to assert in the environment the app actually runs in.

If you do want the oracle observation pinned (it's the signal the Quint model
keys on, and the sibling entry points pass their labels too), this kills M1:

@pytest.mark.asyncio
async def test_user_budget_row_rejection_is_recorded_by_quint_oracle(
    async_session_maker, personal_org
):
    async with async_session_maker() as session:
        service = OrgBudgetService(session)
        oracle = MagicMock()
        oracle.In = lambda value, domain: value
        with patch('server.services.org_budget_service.quint_oracle', oracle):
            with pytest.raises(HTTPException):
                await service.get_user_budget_row(personal_org.id, personal_org.id)
    oracle.log.assert_called_once()
    assert oracle.log.call_args.args[0] == 'get_user_budget_row'

I verified both halves: it passes on this branch unmodified, and it fails once
M1 is applied. Whether it's worth adding depends on whether you consider the
oracle label part of the contract or just harness plumbing — given none of the
four sibling guards assert their label either, treating it as out of scope is a
perfectly defensible call.

Not a test gap

The single test carries this one-line change well — nothing else in the diff is
under-asserted. Worth noting get_user_budget_row is the only budget entry point
this test covers (the sibling test_budget_operations_reject_personal_org_without_creating_settings
exercises get_budget_state and update_budget_settings only), so this test is
the sole guard on the line you added.

---

This comment was generated by an AI assistant on behalf of the user.

[aivong-openhands]

Thanks for the mutation review — agreed on the controls and on the analysis of M1.

On the environment where the app actually runs, M1 is an equivalent mutant as you say (quint_oracle is the no-op SimpleNamespace fallback). But since this PR is one of the Quint-model-driven set, the entry-point label is the signal the model keys on, and all four sibling guards pass theirs — so I'd rather pin it than leave the one guard this PR adds as the odd one out. Added your suggested test in bb03c58:

@pytest.mark.asyncio
async def test_user_budget_row_rejection_is_recorded_by_quint_oracle(
    async_session_maker, personal_org
):
    oracle = MagicMock()
    oracle.In = lambda value, domain: value
    async with async_session_maker() as session:
        service = OrgBudgetService(session)
        with patch('server.services.org_budget_service.quint_oracle', oracle):
            with pytest.raises(HTTPException):
                await service.get_user_budget_row(personal_org.id, personal_org.id)
    oracle.log.assert_called_once()
    assert oracle.log.call_args.args[0] == 'get_user_budget_row'

Verified both halves: passes on the branch, and fails with Expected 'log' to have been called once. Called 0 times. once M1 (_reject_personal_org(org_id, None)) is applied — so M1 is now killed. The original test is unchanged.

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