Skip to content

fix(smart-forms): mime type, conflict detection, dup guard#79

Open
gh-corezoid wants to merge 1 commit into
developfrom
fix/smart-forms-mime-type-and-conflicts
Open

fix(smart-forms): mime type, conflict detection, dup guard#79
gh-corezoid wants to merge 1 commit into
developfrom
fix/smart-forms-mime-type-and-conflicts

Conversation

@gh-corezoid

Copy link
Copy Markdown

Summary

  • defaultMimeType fix: pages/<page>/style files are now correctly assigned text/css on creation. The file is always named exactly style (no extension) but contains Less/CSS source — the old check only recognised the top-level styles/ tree and returned application/json here, which the backend cannot correct via a content-only PUT.
  • MIME self-healing on PUT: The diff phase now detects MIME-only drift (content hash unchanged, stored type wrong) and adds such files to modifiedFilePaths. Phase 3 always re-derives the type from the path instead of trusting the stale manifest value, so a wrong Content-Type is corrected in the same PUT that updates the source — no UI delete/recreate needed.
  • Duplicate-file guard in push: Before creating any new file, pushSmartForm fetches the live server tree and checks whether a file already occupies that (folderId, title) slot. If it does the push aborts with a clear error message (including the server file ID and MIME type) and guidance to run pullSmartForm to re-sync the manifest. This mirrors the UI's one-style-file-per-page constraint on the API create path.
  • Conflict detection in pull: pullSmartForm now loads the existing .manifest.json before overwriting and compares on-disk hashes against the last-pulled hashes. If any local file has been edited since the last pull the pull is refused with the list of conflicting paths. Pass force=true to discard local edits and proceed.
  • force bool param added to pullSmartForm registration.
  • Tests: TestDefaultMimeType covers all path patterns including the regression case; TestDetectPullConflicts covers no-manifest, unchanged, modified, missing, and mixed scenarios.

Context

tool: pushSmartForm / pullSmartForm | version: 2.3.5 | install: 6054ea78-1ace-49ad-8e81-e64cdb7b97d6

Checklist

  • make build / make vet pass locally
  • make test passes locally (all packages green)
  • make discovery run — no changes to public/
  • No version bump / CHANGELOG.md change included
  • No credentials or private URLs in any file

Test plan

  • Push a new Smart Form that includes a pages/<page>/style file; verify the server assigns text/css (not application/json)
  • With an existing form that has a wrong-mime style file in the manifest, run pushSmartForm without changing content; verify it issues a corrective PUT and updates manifest.Files[...].MimeType
  • Manually recreate a style file via the UI (different server ID), then run pushSmartForm; verify it aborts with duplicateFiles guidance instead of creating a second file
  • Edit a local file after a pull, then run pullSmartForm again; verify it aborts listing the conflict; verify force=true proceeds and overwrites

- defaultMimeType: add pages/<page>/style → text/css rule; the
  file is named "style" (no extension) but is always Less/CSS;
  old code returned application/json here (unfix-able by PUT)
- push diff phase detects MIME-only drift and includes those
  files in modifiedFilePaths so a PUT self-heals the wrong
  Content-Type without a delete/recreate cycle
- Phase 3 PUT re-derives mimeType from path instead of the
  stale manifest value; updates manifest.MimeType accordingly
- Before creating new files, fetch the live server tree and
  block the push if a file already occupies that (folder,title)
  slot — mirrors the UI one-style-file-per-page constraint and
  surfaces the orphan in the error with guidance to re-pull
- pullSmartForm conflict detection: if any local file's hash
  differs from last-pulled manifest hash, pull aborts listing
  conflicts; pass force=true to discard local edits and proceed
- Add force bool param to pullSmartForm tool registration
- Tests: TestDefaultMimeType + TestDetectPullConflicts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gh-corezoid

Copy link
Copy Markdown
Author

AI Review

Fixes MIME type misassignment for pages/<page>/style files, adds pull conflict detection with force override, and adds server-side duplicate guard before file creation in pushSmartForm.

Checklist

Check Result
U1 Conventional commit format ✅ pass
U2 No leaked credentials ✅ pass
U3 No merge commits ✅ pass
U4 PR targets correct base branch (develop) ✅ pass
U5 Build & test verification (Go changes) ⬜ skip — cannot check out branch in this environment
U6 Architectural & design consequences ⚠️ warning — see below
S1 No manual edits to public/ ⬜ skip — public/ not touched
S2 API path parameter names match papi-openapi.json ⬜ skip — no internal/tools/ files changed
S3 New tools have eval scenarios ✅ pass — no new MCP tool functions added
S4 Discovery artifacts committed if source changed ⬜ skip — no internal/tools/ files changed
S5 All six manifest files version-synced ⬜ skip — no manifest files in diff
S6 README / ARCHITECTURE updated for new tools ✅ pass — no tools added or renamed

Issues found

⚠️ warning (U6) — Hardcoded "env": "develop" in duplicate-file error response (push.go)

In the Phase 2 duplicate detection error path in push.go, the error JSON payload contains:

"env": "develop",

This is a literal string rather than the actual env title being iterated. The current codebase only allows pushing to develop, so this doesn't cause a bug today — but it's the kind of silent assumption that will produce confusing error messages if the tool is extended to support other environments. Consider passing the actual env.Title value here (matching the pattern used in the pullSmartForm conflict error, which correctly uses env.Title).

⚠️ warning (U6) — fetchServerFilesBySlot failure is silently swallowed; duplicate guard is best-effort only

When fetchServerFilesBySlot returns an error, push.go sets serverFiles = nil and proceeds without the duplicate check, logging nothing. This is documented in the comment as intentional ("Non-fatal … Duplicate detection is best-effort"), but a transient API error will silently disable the guard and allow orphan creation. Consider at minimum including a warning field in the success response when the server-tree fetch failed, so callers know the check was skipped.


This review was generated automatically. A human maintainer should still make the merge decision.

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