fix(label): reject a mistyped label body instead of silently clearing (#1821)

[dolho]

What

PUT /api/agents/{name}/label no longer returns 200 and silently wipes the label when the body uses a field name it doesn't recognise.

Why

label=None means "clear", and the model ignored unknown fields with label defaulting to None — so a malformed body and a deliberate clear were the same request. I hit this by accident while testing the feature, sending display_label (the DB column name):

label before                        : Alpha (Research Lead)
PUT {"display_label":"Typo Name"} → HTTP 200
label after                         : None

The naming makes it easy: the column is display_label, the response field is display_name, only label is accepted.

Change

• model_config = ConfigDict(extra="forbid") — an unknown field 422s.
• label becomes required-but-nullable — {} also 422s, since it carried the same ambiguity and was never a documented way to clear.

Clearing is unchanged in meaning and still supported; it just has to be explicit.

Compatibility

• The frontend already sends { label } on every call (stores/agents.js:393), including when clearing.
• No MCP tool sets labels.
• Behaviour change worth naming: PUT {} now 422s where it used to clear. That path was indistinguishable from a typo, which is the bug.

Verification

Unit — tests/unit/test_1821_label_update_strictness.py, 7 cases: unknown field rejected, empty body rejected, explicit null still clears, blank string still clears, setting works, the ent#181 normalizer still runs, and control characters are still rejected (strictness must not bypass the existing validator).

One test of mine was wrong on the first pass — I asserted the normalizer collapses internal whitespace; it only trims and NFC-normalizes. Corrected to the real behaviour rather than changing the code to match my assumption.

Live, reproducing the issue exactly:

PUT {"display_label":"Typo"}   -> 422   (was 200 + silent wipe)
PUT {}                         -> 422
label survived                 : Alpha (Research Lead)
PUT {"label":null}             -> 200
label now                      : None

Instance reverted to stock afterwards.

Related to #1821

🤖 Generated with Claude Code

[vybe]

This PR requires the following change before merge:

• Update tests/unit/test_1640_display_label_at_creation.py::test_label_update_shares_policy to the new contract. Line 95 asserts AgentLabelUpdate().label is None — the empty-body-means-clear behavior this PR deliberately outlaws — so the regression diff job fails deterministically across all 3 seeds (4978 pass / 1 fail on head vs 4972 / 0 on base).

The fix itself is right (the #1821 silent-clear on an unknown field name is a real bug and extra="forbid" + required-but-nullable is the correct shape), but it changes a contract that an existing test pins, and only the new test file was updated. Merging as-is would land a permanently red test on dev and pollute every future PR's regression-diff base.

Please update the old test to the new contract (empty body → ValidationError) and request re-review.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[dolho]

Fixed in 096eee3 — thanks, that was a clean catch.

test_1640_display_label_at_creation::test_label_update_shares_policy line 95 (AgentLabelUpdate().label is None) is gone. It's replaced by the case it was actually standing in for — label=None, the explicit clear, which still works and is the behaviour that mattered — and the two inputs this PR now rejects moved into their own test: the empty body, and display_label="Typo Name", i.e. the literal #1821 trigger, which had no coverage in that file before.

pytest tests/unit/test_1640_display_label_at_creation.py tests/unit/test_1821_label_update_strictness.py
23 passed

Full unit suite is running; I'll post the count when it lands.

On the nightly bot's merge-conflict note above: stale. origin/dev is an ancestor of this branch as of now, the API reports mergeable: true, and no merge was needed.

Re-review please.

[dolho]

Full unit suite on the branch after 096eee3:

4909 passed, 16 skipped, 4 failed  (15:47)

FAILED tests/unit/test_1472_schedule_validation.py::test_accepts_valid[0 4 * * *-Europe/Kiev]
FAILED tests/unit/test_admin_email_login.py::test_admin_logs_in_with_username
FAILED tests/unit/test_admin_email_login.py::test_admin_logs_in_with_registered_email
FAILED tests/unit/test_admin_email_login.py::test_email_identifier_is_normalized

test_1640_display_label_at_creation is green — the blocker you flagged is gone. The 4 remaining are pre-existing and environmental (ModuleNotFoundError: No module named 'tzdata' for the first; the other three reproduce on an untouched checkout at the same base), and they're the same set showing on #1837 and #1838.

[dolho]

Review item was already fixed in 096eee38 (pushed the day after the review); the branch has now also been rebased onto current dev, so the tip is 2435e58a.

The item. test_1640_display_label_at_creation::test_label_update_shares_policy:95 asserted AgentLabelUpdate().label is None — exactly the empty-body-means-clear behaviour this PR outlaws. Replaced with the explicit-clear case it was standing in for (label=None), and the two now-rejected inputs split into their own test:

def test_label_update_requires_the_label_field(M):
    with pytest.raises(Exception):
        M.AgentLabelUpdate()                              # empty body
    with pytest.raises(Exception):
        M.AgentLabelUpdate(display_label="Typo Name")     # the actual #1821 trigger

Worth noting the second case had no coverage in that file at all — the misnamed-field trigger was only tested in the new file, so the contract's own test module didn't pin the bug it exists to describe.

Checked whether one site was all of them, rather than just doing the named fix. Every AgentLabelUpdate reference in the repo: the model (models.py:158), the router (routers/agents.py:920), and the two test files — both now on the new contract. No other consumer pins the old default.

Frontend compatibility re-verified rather than re-asserted. The PR body claims the UI always sends { label }, but the claim has a sharp edge worth being explicit about: JSON.stringify({label: undefined}) produces {}, which under this PR now 422s — so "always sends { label }" would be false if the clear path passed undefined. It doesn't. AgentHeader.vue:725 emits trimmed || null, so a clear serializes to {"label":null} and still works. Traced the whole path (AgentHeader → AgentDetail:629 → stores/agents.js:393).

Verification after the rebase (47 commits of dev had landed since the green run, so the old CI pass was stale): test_1640 + test_1821 = 23 passed; wider label/models/agent-config selection = 91 passed. CI re-running on the rebased tip.

Re-requesting review.

[obasilakis]

Re-validated after the requested change landed.

vybe's single blocking item is addressed by 2435e58: test_1640_display_label_at_creation.py::test_label_update_shares_policy line 95 no longer asserts AgentLabelUpdate().label is None — it now asserts the explicit-clear case (label=None) it was standing in for, and the two newly-rejected inputs (empty body, the misnamed display_label field) moved into their own named test. The regression diff job is green across all 3 seeds: base 5735 pass / 1 fail vs head 5743 / 1 — the same single pre-existing failure on both sides (test_1069_voip_call_path_param, a FastAPI get_flat_dependant ImportError already red on dev), so no new failures and +8 net passing tests. All 18 checks green.

Contract change verified safe. Every caller sends an explicit {label: <string|null>}: the only writer is stores/agents.js:391 setAgentLabel, reached solely from AgentDetail.vue:629 via AgentHeader.vue:725 emit('set-label', trimmed || null) — the clear path coerces '' to an explicit null, never undefined, so it never serializes to {}. No MCP tool, CLI command, script, agent-server path, or enterprise-submodule caller touches the endpoint, and nothing constructs AgentLabelUpdate programmatically outside tests. Docs already prescribed the strict form — requirements/core-agent.md:60 specifies {label: string|null} and managing-agents.md:65 documents clearing as {"label": null} or a blank string — so this brings the code into conformance rather than changing what was documented. Both fixes match #1821's own suggested remedies.

Approving; not dismissing the prior review.

[dolho]

@vybe the blocking item is addressed in 2435e58a: test_1640_display_label_at_creation.py::test_label_update_requires_the_label_field now pins the new contract — M.AgentLabelUpdate() (empty body) and M.AgentLabelUpdate(display_label="Typo Name") (the actual #1821 trigger) both raises(Exception), and the empty-body-means-clear assertion is gone. regression diff is green across all seeds and every check passes. Ready for re-review.