fix(office): reject empty slug on skill update

[nova28]

Tip

PR walkthrough: Open the visual walkthrough

fix(office): reject empty slug on skill update

PATCH /api/v1/skills/:id with {"slug":""} previously short-circuited
past the well-formedness check and persisted an empty slug, producing a
skill row that skillslug delivery silently drops. Empty is now treated
as not well-formed like any other invalid slug and rejected with 400;
omitting the field is still the only way to leave the slug unchanged.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: e9034a54-c6a4-49b3-bb8a-3bfd5a7ca9d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: f90669d2-5ae6-4e28-94b3-690ecdcb4c63

📥 Commits

Reviewing files that changed from the base of the PR and between 77e3cfc and 2466082.

📒 Files selected for processing (3)

• apps/backend/internal/office/skills/handler_test.go
• apps/backend/internal/office/skills/service.go
• apps/backend/internal/office/skills/service_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes

  • Skill updates now reject explicitly empty slugs with a validation error.
  • Omitting the slug continues to preserve the existing value.
  • Valid slugs are normalized before uniqueness checks.

• Tests

  • Added coverage for empty-slug rejection and preserving slugs when the field is omitted.

Walkthrough

Skill updates now reject explicit empty slugs. Updates that omit the slug preserve the existing value. Service and HTTP handler tests cover both behaviors.

Changes

Skill slug update behavior

Layer / File(s)	Summary
Slug validation behavior apps/backend/internal/office/skills/service.go, apps/backend/internal/office/skills/service_test.go	ValidateSkillUpdate rejects empty slugs. The service test verifies the validation error and unchanged value.
HTTP update behavior apps/backend/internal/office/skills/handler_test.go	Handler tests verify a 400 response for an empty slug and successful updates that omit the slug.

Priority: ⬇️ Low — Defer this change because it is limited to rejecting empty skill slugs while preserving omitted-slug updates.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 24660

Skill updates now reject explicit empty slugs while preserving existing slugs when the field is omitted. The covered behavior is ready to merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the problem and intended behavior, but it omits the required Validation and Checklist sections and includes prohibited auto-generated attribution content.	Add the required Validation section with commands or checks performed, restore the Checklist section unchanged, and remove the auto-generated description and attribution comments.
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting empty slugs during skill updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

_{A rabbit checks the slug at night
Empty strings now fail outright
Omitted fields keep their place
Tests confirm the stored state
Hop, the update path is right}

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

[greptile-apps]

Greptile Summary

This PR closes the empty-slug update path by applying normal well-formedness validation before slug normalization and uniqueness checking.

• Explicit {"slug":""} PATCH requests now return HTTP 400 without modifying persisted data.
• Omitting slug continues to preserve the existing value.
• Service-level and HTTP-handler regression tests cover both behaviors.

Confidence Score: 5/5

The PR appears safe to merge and preserves omitted-field PATCH behavior while preventing new empty slugs from being persisted.

The request DTO distinguishes omission from an explicit empty string, the handler validates a stored skill after applying only present fields, and the shared slug predicate definitively rejects empty input; regression tests cover the service and HTTP paths.

Important Files Changed

Filename	Overview
apps/backend/internal/office/skills/service.go	Removes the empty-slug validation bypass so empty values are rejected by the shared well-formedness check.
apps/backend/internal/office/skills/handler_test.go	Adds end-to-end handler coverage for explicit-empty rejection, unchanged persistence, and omitted-field semantics.
apps/backend/internal/office/skills/service_test.go	Adds focused service coverage proving that an empty slug is rejected rather than normalized or coerced.

_{Reviews (1): Last reviewed commit: "fix(office): reject empty slug on skill ..." | Re-trigger Greptile}

[github-actions]

Claude finished @nova28's task in 2m 17s —— View job

---

Code Review

• Gathering context
• Reading changed files
• Analyzing correctness and test coverage
• Posting findings

---

Findings

Suggestion (recommended, doesn't block)

1. Handler test lacks coverage for other not-well-formed slugs — handler_test.go

   The handler-level tests cover {"slug":""} and omitted slug, but not another invalid slug value (e.g. {"slug":"not valid!"}). The service-level TestValidateSkillUpdate_RejectsNotWellFormedSlug covers that case, so the behavior is verified — but a handler-level assertion would confirm the 400 wire-up works for the full not-well-formed class, not just the empty-string specialization.

   This is minor: the service test is sufficient to guard the logic. Handler test would just add defense-in-depth confidence.

---

What this PR does well

• Root cause is correctly identified and fixed. ValidateSkillUpdate previously short-circuited on skill.Slug == "" with return nil, letting an explicit {"slug":""} body bypass validation and persist an empty slug. Removing that guard means skillslug.WellFormed("") (which returns false because the regex requires at least one character) now handles it — no special case needed.

• Omit-vs-explicit-empty is correctly modeled. UpdateSkillRequest.Slug is *string, so a missing field leaves req.Slug == nil → applySkillUpdates never touches skill.Slug → service gets the original DB value, which is already well-formed. An explicit "" sets skill.Slug = "" → WellFormed returns false → 400. The distinction is correctly wired end-to-end.

• Tests are behavior-focused and complete at the key level. TestUpdateSkillHandler_RejectsEmptySlug verifies both the HTTP status code (400) and that the DB row is unchanged — catching persistence leaks, not just response-code behavior. TestUpdateSkillHandler_OmittedSlugLeavesItUnchanged verifies the round-trip response slug and the stored row. TestValidateSkillUpdate_RejectsEmptySlug also verifies the model struct is not coerced.

• Scope is minimal and on-point. Five lines removed from the service, docstring updated to match new behavior, three new focused tests. No unrelated changes.

---

Summary

Severity	Count
Blocker	0
Suggestion	1

Verdict: Ready to merge

[carlosflorencio]

Addressed in commit a20ece6: moved the empty-slug service regression into a focused test file and added handler coverage for another malformed slug. The Office skills package passes 113 tests. Thanks for the contribution. Fresh CI is now running on the pushed head.

[nova28]

Thanks for the review and for tightening up the test coverage in a20ece6 — I've built on top of it (not replaced it) with two more commits that close a related gap:

The gap: ValidateSkillUpdate's empty-slug rejection is correct for an explicit {"slug":""}, but a stored row can already have an empty slug through no fault of the update request — e.g. a row created outside ValidateAndPrepareSkill (the config-import path bypasses it; tracked separately). Before this addition, any content-only PATCH to such a row (no slug field at all) would hit the same WellFormed("") check and 400 — so an already-broken row became permanently un-editable through the normal edit flow, even for a request that never touched the slug.

The fix (ValidateSkillUpdate(ctx, skill, slugRequested bool)):

• Caller sends slug → unchanged from your version: strict WellFormed → Normalize → uniqueness check, "" still 400s.
• Caller omits slug and the stored value is already "" → self-heals to a free name-derived slug (healEmptySlug) so the request succeeds instead of failing on a field it never mentioned; if every candidate collides, the stored empty value is left as-is and the request still succeeds (never fails on slug grounds for a request that didn't ask about the slug).

Added TestValidateSkillUpdate_HealsEmptyStoredSlugWhenNotRequested, TestValidateSkillUpdate_LeavesEmptyStoredSlugOnCollisionWhenNotRequested (service_slug_update_test.go), and the matching handler-level tests. Full internal/office/... suite is green, golangci-lint --new-from-rev is clean, gofmt -l is clean.

Happy to drop this if you'd rather ship the simpler reject-only version and handle the stored-empty-slug case separately — let me know.