Fix/regeneration

[ashutosh-opptra]

No description provided.

[dobby-opptra]

Dobby review · opptra/catalog-service#30

Based on my analysis of the diff and the checkout, here's my review.

Dobby review

⚠️ The headline an author needs: this PR replaces schema-level maxLength enforcement with a code-side bounded rewrite loop and simplifies regeneration to reuse the stored brief. The rewrite loop and a global provider routing change carry the real risk; there are no tests for either.

🚨⚠️ Must fix

No Critical or High issues found.

📝 Medium

• 📝 [Correctness][Length-correction retry drops the brief] — server/generation/text.py submit_text_attribute

  • The first call passes cache_prefix=generation_parts.prefix (RULES + PRODUCT DATA + BRAND DNA), but every correction retry calls client.call_tool(f"{prompt}\n\n{correction}", ..., session_id=...) with no cache_prefix. So the model compresses the over-limit value without PRODUCT DATA / BRAND / RULES in context, and the retry loses prompt-cache benefit entirely. Fix: pass cache_prefix=cache_prefix on every retry so the full brief stays in scope.

• 📝 [Reliability][Global provider routing] provider.require_parameters: True set unconditionally — server/core/clients/openrouter.py call_tool

  • This is applied to every tool call (text gen, gallery plan, common-context extraction). OpenRouter only routes to providers that advertise support for the required structured-output params; if none qualify the request fails with 400/no-provider — a regression risk for calls that previously worked. Fix: scope it to the strict text-tool path (or make it opt-in/configurable) and handle the no-qualifying-provider error explicitly.

• 📝 [Correctness][Fail-fast on a single field] — server/generation/text.py submit_text_attribute

  • After _MAX_LENGTH_ATTEMPTS it raises ValueError, which propagates and aborts the whole generation job even if only one bullet is a few chars over a cap. Given the stated goal is a converging loop, a hard throw is heavy-handed; consider a graceful final path (best-effort value, or per-attribute error) that doesn't kill the rest of the job.

Focus check

Area	Result
Security	No issues found
Correctness	2 Medium — brief/cache dropped on length retries; last-attempt fail-fast aborts whole job
Performance	1 Medium — require_parameters can eliminate all qualifying providers on a call
Tests	No tests at all for the new submit_text_attribute / _merge_shorter / over_limit_report / regen-identical-retry logic

❓ Questions

• In _merge_shorter the merge only keeps an item when the candidate is shorter. For list attributes the correction prompt says "copy every other item EXACTLY" — but a model that (harmlessly) rephrases a passing item shorter than the original silently replaces it, dropping original wording that was already compliant. Intentional?
• For text regeneration, previous_prompt is the stored full generation brief (generation_parts.as_sent(), which still embeds the "call submit_text_attributes" tool instruction and RULES/product/brand). This whole brief is then re-sent inside regeneration_text_prompt as "PREVIOUS PROMPT". Was that the intended "brief", as opposed to stripping the tool/rules scaffolding?

🧪 Tests to add

• Bounded length loop: over-limit input converges under cap within _MAX_LENGTH_ATTEMPTS; a stubborn over-limit value raises the ValueError.
• _merge_shorter: over-limit items shrink, passing items stay unchanged even when the candidate differs; non-list/scalar behavior; malformed non-list candidate.
• Text regeneration identical-retry: first result equal to CURRENT OUTPUT triggers exactly one retry and (if still identical) persists without an infinite loop.
• prompt_with_user_edit / strip_user_edit round-trip on a prompt that already contains a prior === USER EDIT === block.