Skip to content

fix(serenity): reject empty edit text and modernize edit-path tests#2875

Open
byteclimber wants to merge 3 commits into
feat/serenity-intent-classificationfrom
fix/serenity-edit-path
Open

fix(serenity): reject empty edit text and modernize edit-path tests#2875
byteclimber wants to merge 3 commits into
feat/serenity-intent-classificationfrom
fix/serenity-edit-path

Conversation

@byteclimber

Copy link
Copy Markdown
Contributor

Summary

Three coupled edit-path (PATCH prompt) fixes in the serenity handlers. Base is feat/serenity-intent-classification, not main.

Fix 1 — Empty/whitespace text accepted on edit but rejected on create

parseUpdatePromptBody did String(body.text) with no trim and only rejected text === undefined, so a PATCH with text: "" or " " passed validation, reached renamePrompt, and got classified/defaulted into a blank prompt. It now trims and rejects empty text with a 400 invalidRequest, mirroring the create path (normalizePromptInput) — including the || '' coercion so a falsy non-string (null, 0, false) is treated as empty too, matching create exactly. parseUpdatePromptBody is shared with handleUpdatePromptSubworkspace, so this one fix covers both edit paths.

Fix 2 — Redundant reclassification on no-op text edits (no behavior change, by design)

handleUpdatePrompt always reclassifies from the new text and rewrites tags, even when the PATCH does not change the text. Decision after review: no behavior change. The handler is not sent the old text, and the upstream provider has no GET-by-id, so it cannot detect a no-op text edit without an API contract change (client sends old text) — deliberately out of scope. Classification must also run before renamePrompt for failure-safety (a classify failure leaves the prompt intact), so renamePrompt's is_updated result cannot gate it (it comes back too late). This PR documents the rationale with a code comment and adds a regression test proving an is_updated: false (no-op) rename still writes tags and publishes — guarding against a future "optimization" that would break the documented contract.

Fix 3 — Modernize stale/broken edit-path tests

The handleUpdatePrompt block had 7 stale tests (not 3) that still stubbed the retired delete+create flow (deletePromptsByIds/createPromptsByIds); they failed at runtime (transport.renamePrompt is not a function) and one referenced an undefined createErr. Modernized them to the current renamePrompt + replace-mode updatePromptTagsByIds flow: in-place edit, drops-falsy/malformed tagIds, 409 collision (throw, no tag write, no publish), non-404 rename error (throw, no tag write), tag-write failure after successful rename (throw, publish never fires, partial-mutation warn logged), and 404 promptNotFound — plus a new negative test that a generic Error with .status=404 does NOT map to promptNotFound. Also added the intent tag to the "recomputes the type tag on edit" assertion. The same stale-test class in prompts-subworkspace.test.js (3 edit-path twins) is fixed too, since this PR is the edit-path fix.

Test evidence

  • prompts.test.js: green (was 8 failing → 0).
  • prompts-subworkspace.test.js: edit-path tests green (3 fixed).
  • Full serenity suite: 1046 passing, 3 failing (was 1031 passing / 14 failing on the base branch before this PR).
  • npm run lint and npm run docs:lint clean on the changed files; the changed source (prompts.js) passes type-check.

Pre-existing branch breakage (NOT introduced here, out of scope)

The base branch feat/serenity-intent-classification is already red from the LLMO-6190 dynamic-allocation work, in files this PR does not touch. CI will show these regardless:

  • eslint: src/support/serenity/handlers/prompts-subworkspace.js:251headroom no-shadow.
  • tsc (type-check): src/support/serenity/handlers/markets-subworkspace.js:217 (stale @param headroom) and :537 (Expected 5 arguments, but got 6 — a real call-site arg mismatch to createHeadroomGuard).
  • 3 failing tests: dynamic-allocation fronting — retryOnQuota wiring, prompts-subworkspace … dynamic-allocation ON …, and intent-classification.js — observability.

These should be fixed on the feature branch (or by the LLMO-6190 owner) separately.

🤖 Generated with Claude Code

Fix 1: parseUpdatePromptBody accepted empty/whitespace text on PATCH
(only rejected `undefined`), so an edit could reach renamePrompt with a
blank prompt that then got classified/defaulted. Now trims and rejects
empty text with a 400, mirroring the create path (normalizePromptInput)
including the `|| ''` falsy coercion. Shared with the subworkspace twin,
so both edit paths are covered.

Fix 2: no behavior change. The always-reclassify on edit is intentional
and unavoidable: the handler is not sent the old text and upstream has
no GET-by-id, so it cannot detect a no-op text edit, and classification
must run before the rename for failure-safety. Documented the rationale
with a code comment; added a regression test that a rename returning
is_updated:false still writes tags and publishes.

Fix 3: modernized the stale handleUpdatePrompt tests (and the three
edit-path twins in prompts-subworkspace.test.js) that still stubbed the
retired delete+create flow. They failed at runtime and one referenced an
undefined `createErr`. They now exercise the rename + replace-mode
tag-write flow (409 collision, non-404 rename error, tag-write failure
after rename, 404 promptNotFound plus its generic-Error negative). Added
the intent tag to the recompute assertions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

This PR will trigger a patch release when merged.

The feature branch left the dynamic-allocation headroom guard half-wired,
breaking type-check + build and a metering test:

- markets-subworkspace.js generateAndAttachPrompts: the caller threaded a
  headroom guard as a 6th arg and the JSDoc documented it as required, but the
  function never declared or used it (TS8024 + TS2554). Add the param and front
  headroom.ensure({ prompts: texts.size }, { includeDrafted: true }) before the
  metered createPromptsByIds write loop.

- prompts-subworkspace.js handleCreatePromptsSubworkspace: removed the redundant
  pre-publish ensure that re-created a second headroom guard (no-shadow) and
  issued a second getWorkspaceResources read. LLMO-6190 meters ONCE before the
  write (the create is the metered write, not publish); the pre-write ensure
  already reserves the whole batch, so the pre-publish read was a leftover.

- prompts-subworkspace.test.js: the dynamic-allocation test passed its options
  object in the env positional slot instead of the 8th, so the guard was never
  enabled; corrected to match the controller/signature.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@byteclimber

Copy link
Copy Markdown
Contributor Author

Folded in the LLMO-6190 headroom base fix (was #2883, now closed) because the two are a mutual dependency — each PR's CI runs the full suite/lint, so neither could go green alone. This branch now also fixes:

  • markets-subworkspace.jsgenerateAndAttachPrompts was missing the headroom param the caller/JSDoc require (TS8024 + TS2554); added it and fronted headroom.ensure({ prompts: texts.size }, …) before the metered write loop.
  • prompts-subworkspace.js⚠️ behavior change, please scrutinize: removed the redundant pre-publish ensure (the no-shadow build failure + a second getWorkspaceResources read). LLMO-6190 meters once, before the write; the pre-write ensure already reserves the batch. If a second pre-publish top-up was intentional, push back here.
  • prompts-subworkspace.test.js — corrected a stale positional call (options → 8th slot).

Verified locally: type-check clean, lint clean, 223 serenity handler tests passing.

Completes the LLMO-6190 create-prompts fronting: handleCreatePromptsSubworkspace
now passes headroom.retryOnQuota as publishAffected's wrapPublish, so a disguised
metered-405 on a project publish gets one bounded top-up+retry per project before
being recorded as a failure (the JSDoc on publishAffected already documented this
caller contract; the wiring was missing). Reuses the single guard created before
the write — no extra guard, no extra read on the success path.

Also fixes the stale positional call in the dynamic-allocation-fronting test
(options were passed in the env slot instead of the 8th), matching the controller
and the sibling test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@byteclimber
byteclimber requested review from MysticatBot and removed request for MysticatBot July 22, 2026 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant