Skip to content

fix(pushSmartForm): reconcile missing file id after create#78

Open
gh-corezoid wants to merge 1 commit into
developfrom
fix/pushSmartForm-reconcile-missing-file-id
Open

fix(pushSmartForm): reconcile missing file id after create#78
gh-corezoid wants to merge 1 commit into
developfrom
fix/pushSmartForm-reconcile-missing-file-id

Conversation

@gh-corezoid

Copy link
Copy Markdown

Summary

  • pushSmartForm no longer aborts immediately when the batch create response omits a file id
  • Instead it re-fetches the env struct (app_content/struct/{actorId}/{envId}) to recover the server-assigned id and writes it to .manifest.json
  • If recovery succeeds, the push completes normally and a new reconciledFiles field in the response lists the affected paths
  • If recovery also fails, a distinct error is returned — "run pullSmartForm to reconcile before retrying (retrying without reconciliation risks duplicate files)" — clearly different from a total failure where nothing was created

Context

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

Root cause

The batch POST to /papi/1.0/app_content/{actorId} sometimes returns a response that is missing one or more of the created files (id absent or object omitted). The old code returned a bare [Error] server did not return id for created file immediately — without writing the manifest — so retrying re-sent the same create and produced a duplicate file on the server on every attempt.

Checklist

  • make build passes
  • make vet passes
  • make test passes
  • make discovery run — no public/ changes (tool interface unchanged)
  • No version bump / CHANGELOG.md change included
  • No credentials or private URLs

Test plan

  • Push a new file whose server response omits the id → confirm manifest is updated via reconciliation, no error returned
  • Confirm a genuine server-side failure (file not found after reconciliation) surfaces the new distinct warning message
  • Confirm the happy path (id present in response) is unaffected

When the batch create response omits a file's id, instead of
aborting with an opaque error, re-fetch the env struct to
recover the id and update .manifest.json. This prevents the
caller from retrying a create that already succeeded, which
produced duplicate files/pages on each retry.

If reconciliation also fails, a distinct error is surfaced —
"run pullSmartForm to reconcile before retrying" — clearly
distinguishable from a total failure where nothing happened.

Adds reconciledFiles field to the success response so callers
can see when recovery was needed.

install: 6054ea78-1ace-49ad-8e81-e64cdb7b97d6

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

Copy link
Copy Markdown
Author

AI Review

pushSmartForm now recovers from batch-create responses that omit a file's id by re-fetching the env struct, writing the recovered id to .manifest.json, and surfacing a clear, distinct error if recovery also fails — preventing silent duplicate-file creation on retried pushes.

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 & tests (Go) ✅ pass
U6 — Architectural & design consequences ⚠️ warning (see below)
S1 — No manual edits to public/ ✅ pass
S2 — API path parameter names match papi-openapi.json ⏭️ skip (change is in internal/engines/, not internal/tools/)
S3 — New tools have eval scenarios ⏭️ skip (no new tool added)
S4 — Discovery artifacts committed if source changed ⏭️ skip (PR confirms make discovery produced no public/ changes; cannot independently regenerate from diff alone)
S5 — All six manifest files version-synced ⏭️ skip (no manifest files in diff; no version bump intended)
S6 — README.md / ARCHITECTURE.md updated for new tools ⏭️ skip (no new tool added or renamed)

Build detail: go build ./... ✅ and go test ./... ✅ pass on the PR branch. Base branch (develop) tests also pass; no regressions introduced. Note: internal/engines/smartform has no test files — the new reconciliation path has no automated coverage.

Issues found

  1. warning — reconciledFiles serialises as null instead of [] in the happy path
    var reconciledFiles []string is a nil slice. When no reconciliation is needed (the common case), json.Marshal produces "reconciledFiles": null rather than "reconciledFiles": []. Callers who check len(reconciledFiles) == 0 in Go are fine, but API consumers comparing against [] will be surprised. Fix: initialise as reconciledFiles := make([]string, 0) or add ,omitempty to the field if the intent is to omit it entirely when empty.

  2. warning — path-separator mismatch in collectCreatedFilesByPath vs. newFilePaths (portability)
    collectCreatedFilesByPath builds keys with "/" as the folder separator (relDir + "/" + child.Title), while the missingFromResponse entries come directly from newFilePaths, which may carry OS-native separators (\ on Windows). On Linux (production) this is harmless, but the reconciliation lookup would silently fail for any subdirectory path on Windows. Low practical risk; consider normalising with filepath.ToSlash(relPath) when building the lookup or when inserting into byPath.


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