fix(team): authorise a tools edit after the existence check, not before

[M3gA-Mind]

Summary

Follow-up to #745 that missed the merge. #745 was merged at 10:09 against head 18fd2b13, while @tinysweeper's review thread on src/server/ops/team_agent.rs:382 was still open. The fix for it — 8f1ab438 — had been pushed but GitHub had not yet advanced the PR head, so the merge carried the finding but not the fix. This lands that commit.

Verified against upstream/main before opening:

git merge-base --is-ancestor 8f1ab438 upstream/main   → not in main
grep an_unknown_teammate_is_a_404_whether_or_not_tools_are_sent  → 0 occurrences
team_agent.rs:382  require_admin(...)   ← above the record load at :391

So the defect is live on main.

The defect

#745 added a conditional admin check to PATCH {scope}/team/{agent_id} — tools is admin-only because an empty list means the company's standard grant, making {"tools": []} a widening. That check was placed above the lookup that turns a non-existent id into a 404, which made one route give two answers about whether a teammate exists:

request on an unknown id, as a non-admin	before	after
{"name": "x"}	404	404
{"tools": […]}	403	404

An unrelated field decided whether the thing was there.

The invariant

A non-admin sending a request for a non-existent id must get the same status whether or not tools is present.

The test pins that, not the choice of status — so it survives a future decision to flip the ordering, provided both paths flip together.

Why the ordering is forced, not preferred

• The other path cannot move to match. A name/role/description edit is member-open and has no authority check to run first, so it must stay 404. Authorising after existence is the only order in which the two agree.
• The usual reason to authorise first does not apply here. Ordering 403 ahead of 404 protects against leaking existence to an unauthorised caller — but GET {scope}/team/{agent_id} is open to any signed-in member and already 404s on an unknown id. So 403-first would hide nothing from the very caller it inconveniences.
• Deliberately unlike set_budget, which authorises first. That route is admin-only in full, so admin-first is self-consistent there. This one is admin-only per field, which is exactly what makes the position load-bearing. Both facts are in the code comment so the divergence is not "harmonised" away later.

Tests

One test added, an_unknown_teammate_is_a_404_whether_or_not_tools_are_sent. Driven as a member deliberately — the only actor for whom the two orderings differ. An admin passes the check either way and sees 404 regardless, so an admin-driven test would pass against the broken ordering too.

cargo fmt --all -- --check                                                                     → 0
cargo check  --locked --all-features --all-targets                                             → 0
cargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warnings   → 0
cargo test   --features openhuman,tinycortex   → 3389 + 10 + 1 + 11 + 2 = 3413 passed, 0 failed, 3 ignored

Revert-checked — the check hoisted back above the record load:

filters passed: 1   tests ran: 1   cargo exit: 101
test …::an_unknown_teammate_is_a_404_whether_or_not_tools_are_sent ... FAILED
test result: FAILED. 0 passed; 1 failed
assertion `left == right` failed: an unrelated field must not change whether a
teammate is reported as existing    left: 403   right: 404

That reproduces the exact 403/404 disagreement from the review thread. Tree restored afterwards.

Related test coverage on the same route, re-run on this branch and passing: a_member_cannot_widen_a_teammates_scope, a_member_may_still_edit_a_teammates_name_and_role, editable_names_tools_only_for_an_admin, an_overlay_teammate_can_be_scoped_after_creation — 5 filters passed, 5 tests ran, 5 passed.

How the escape was caught, since it is reusable

Re-running the #619 evidence set on main after #745 merged, I passed 11 test filters and only 10 ran. The missing one was the test added in the commit that never landed. A filter that silently matches nothing reports ok and looks like a pass — comparing filters passed against tests actually run is what turned a green-looking run into a shipped defect.

API Or Behavior Changes

PATCH {scope}/team/{agentId} with a tools key on an unknown teammate id now answers 404 instead of 403. The authority rule itself is unchanged: tools is still admin-only, still conditional, and a member editing only name/role/description is unaffected.

Closes the open review thread on #745. References #619.

[coderabbitai]

Warning

Review limit reached

@M3gA-Mind, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d3088a1-13ab-4b97-9523-29eae17c6ef2

📥 Commits

Reviewing files that changed from the base of the PR and between cae2162 and f4adf4d.

📒 Files selected for processing (1)

• src/server/ops/team_agent.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[M3gA-Mind]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tinysweeper]

tinysweeper found nothing blocking. Approving.

             $0.0150 · 15,849 in / 4,565 out · 12,622 cached (80%) · z-ai/glm-5.2
critique:    $0.0067 · 5,401 in  / 2,252 out · 4,459 cached (83%)  · z-ai/glm-5.2
security:    $0.0029 · 3,124 in  / 901 out   · 2,657 cached (85%)  · z-ai/glm-5.2
tests:       $0.0033 · 3,039 in  / 988 out   · 2,141 cached (70%)  · z-ai/glm-5.2
description: $0.0022 · 4,285 in  / 424 out   · 3,365 cached (79%)  · z-ai/glm-5.2

[oxoxDev]

The defect is real and the table states it better than prose could:

unknown id, as a non-admin	before	after
{"name": "x"}	404	404
{"tools": […]}	403	404

An unrelated field deciding whether the caller learns the teammate exists is a genuine inconsistency, and standardising on 404 is the right direction here specifically because any member can already GET {scope}/team and enumerate the roster — so the 404 discloses nothing the caller could not read a moment earlier. That is what makes this an error-shape fix rather than an information-disclosure trade.

I should own my part in this. I reviewed that exact line on #745 and praised the placement — "doing the check before taking the write lock … is a detail I would not have asked for and is correct on both counts". It was correct about the lock and wrong about the response shape, and I did not think about the second at all. Verifying the defect is live on main before opening — merge-base --is-ancestor, the absent test name, the line number of the check against the line number of the load — is the right way to make that claim, and I would not have caught it on a second pass either.

0 major. 1 question. Approving — the inconsistency is worth fixing now and the exposure is nil.

Question — the invariant the old comment asserted is now silently false

The previous arrangement carried a stated reason:

Authority before the write lock: a refused edit must not hold the lock, and must not have looked at the record either.

In the new order the lock is taken at :386, the record loaded at :392, existence checked at :408, and require_admin runs at :435. So a refused edit now does hold the company write lock, across the whole of require_admin — and that lock serialises every other write to overlay_agents, including add_agent from a live turn.

It is contention rather than a hard denial: require_admin is a header resolve, not a network round trip. But it is reachable by any signed-in member, repeatedly, and it is precisely the property the sentence above existed to protect. Trading it away may well be right — response-shape consistency is worth more than a few milliseconds of lock hold — but it should be a decision that is written down, not an invariant that quietly stops being true when its comment moves.

The two are also separable if you want both: the existence check needs the record, not the lock. Loading unlocked for the 404, authorising, then taking the lock and re-loading for the mutation gives consistent errors and keeps an unauthorised caller off the lock, at the cost of one extra read on the happy path. Whether that read is worth it is your call — what I would not leave is the old comment's claim deleted with nothing saying why.

Before merging: base is current, one file. Just let the lanes report.

Worth raising beyond this PR

This is the second stale-head merge today — #702 landed without the review fixes pushed to it (recovered by #760), and #745 landed at 18fd2b13 while 8f1ab438 sat unmerged. Both times the finding was carried into main and the fix was not; both times recovery depended on someone noticing. That is a merge-hygiene failure with no automated detection, and it has now cost two follow-up PRs in one day. Worth a rule — do not merge while a review thread is unresolved, or re-check the head SHA at merge time — rather than a third recovery.