Sync with InsForge-sdk-js v1.5.0#6
Conversation
Ports the v1.4.5..v1.5.0 storage changes (paired with the backend change in InsForge/InsForge#1760): - upload(path, bytes): uploading to an existing key now replaces the object in place (standard PUT semantics) instead of the server auto-renaming the key. Signature unchanged. - uploadAutoKey(filename, bytes): generates a unique, collision-free key client-side (sanitized base + timestamp + random suffix) and uploads through the standard upload() path — the backend no longer mints keys server-side via POST /objects. - Deprecate the now no-op upsert flag on upload/uploadAutoKey/FileOptions; the x-upsert header is no longer sent. - Update unit + integration tests, READMEs, changelogs; bump both packages to 0.2.0 in lockstep (tool/set_version.dart). Requires an InsForge backend that includes the standard-PUT storage change; do not run against a pre-change backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jwfing
left a comment
There was a problem hiding this comment.
Code review — Sync with InsForge-sdk-js v1.5.0
Summary: A clean, well-scoped port of the JS SDK v1.5.0 storage change (standard PUT create-or-replace + client-side key minting, deprecated upsert), with tests and versioning updated in lockstep — no blocking issues.
Requirements context
The repo keeps plans/specs under docs/superpowers/. The relevant artifacts (plans/2026-06-08-insforge-flutter-sdk-04-storage.md, specs/2026-06-08-insforge-flutter-sdk-design.md) describe the original v0.1.0 storage design (server-minted auto-key via POST /objects, x-upsert header). This PR deliberately supersedes that design to match the new backend contract, so there is no dedicated spec for this sync — I assessed against the PR description, the paired backend change (InsForge/InsForge#1760), and the upstream source. To verify the claimed exact parity I fetched the real generateObjectKey from InsForge-sdk-js at tag v1.5.0 (src/modules/storage.ts) and compared it line-by-line against the Dart port.
Findings
Critical
(none)
Suggestion
- Functionality —
uploadAutoKeyforwardsmetadatainto a sink (packages/insforge/lib/src/storage/storage_file_api.dart:112-130,:83-102):uploadAutoKeynow passesmetadata: metadatadown toupload, butuploadnever serializesmetadatainto the request (it only builds the singlefileform part). Sometadatais silently dropped for both entry points. This is pre-existing (the olduploadalready ignored it) and out of this PR's stated scope, but the new explicit forwarding makes the dead parameter more visible/misleading. Consider either wiringmetadatathrough (e.g. as form fields the backend accepts) or removing it from the signatures in a follow-up.
Information
- Software engineering — JS parity confirmed, with one benign deviation (
storage_file_api.dart:36-54): the Dart_generateObjectKeymatches JSgenerateObjectKey@v1.5.0on all observable behavior —dotIndex = lastIndexOf('.'),hasExt = dotIndex > 0, extension preserved, sanitize[^a-zA-Z0-9_-] → '-', truncate-to-32-then-'file'-fallback ordering, ms timestamp, 6-char[0-9a-z]suffix. One deviation: JS derives the suffix fromMath.random().toString(36).slice(2,8), which can occasionally yield fewer than 6 chars; the Dart version always emits exactly 6 (arguably more robust). Cross-SDK keys can never be byte-identical anyway (random), so this is fine — noting it because the PR body says "mirroring … exactly." - Security — key randomness (
storage_file_api.dart:28):_keyRandom = Random()is non-cryptographic. That is correct here — object keys are collision-avoidance identifiers, not capability tokens (they're echoed bygetPublicUrl), and it matches the JSMath.random()source. No change needed. - Housekeeping: the historical planning docs (
docs/superpowers/plans/...-04-storage.md, design spec) still describe the oldx-upsert/server-minted-POST behavior. These read as point-in-time planning artifacts rather than maintained reference docs, and the PR correctly updated the living surfaces (doc comments, README, changelogs). No action required, but a one-line "superseded by v0.2.0 sync" note in the plan could prevent future confusion.
Dimension coverage
- Software engineering: Strong. Tests were rewritten to mirror the new JS unit tests — PUT to a client-minted key, filename sanitization, no-extension and empty-filename
'file'fallback, distinct keys across repeated uploads, and theupsertno-op (nox-upsertheader). Integration test updated to assert replace-in-place and a client-generated key.@Deprecatedannotations are applied consistently with matching// ignore:in same-package tests. Conventions (import order,dart:mathaddition, doc-comment style) match surrounding code. - Functionality: Solves the stated goal.
uploadnow PUTs directly (create-or-replace);uploadAutoKeymints the key client-side and delegates toupload, so repeated uploads no longer collide/overwrite.upsertretained for source-compat as a documented no-op. Sanitized auto-keys contain only[a-zA-Z0-9_.-], so the unencoded.../objects/$pathinterpolation is safe for auto keys (the lack ofencodeURIComponenton user-supplieduploadpaths is pre-existing and intentional — nested/keys). - Security: No new attack surface. No secrets/PII newly logged or returned, no auth/authorization changes, no new dependencies (only
dart:math). See key-randomness note above. - Performance: No regressions — no new network round-trips (auto-key path drops the previous server POST and reuses
upload), no loops, no large allocations. Key generation is O(filename).
Verdict
approved (informational — zero Critical findings; explicit GitHub approval remains a human action). Version bumps (0.2.0 across version.dart, both pubspec.yaml, dep constraints, samples, integration_tests) and the backend-coordination caveat are consistent and correctly documented.
🤖 Automated review by the InsForge review bot.
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/insforge/lib/src/storage/storage_file_api.dart">
<violation number="1" location="packages/insforge/lib/src/storage/storage_file_api.dart:53">
P2: Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:22">
P2: Repeated `uploadAutoKey` calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 6, | ||
| (_) => _keyAlphabet[_keyRandom.nextInt(_keyAlphabet.length)], | ||
| ).join(); | ||
| return '$sanitizedBase-$timestamp-$random$ext'; |
There was a problem hiding this comment.
P2: Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/insforge/lib/src/storage/storage_file_api.dart, line 53:
<comment>Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.</comment>
<file context>
@@ -23,6 +24,35 @@ class StorageFileApi {
+ 6,
+ (_) => _keyAlphabet[_keyRandom.nextInt(_keyAlphabet.length)],
+ ).join();
+ return '$sanitizedBase-$timestamp-$random$ext';
+ }
+
</file context>
| return '$sanitizedBase-$timestamp-$random$ext'; | |
| return '$sanitizedBase-$timestamp-$random${ext.replaceAll(RegExp('[^a-zA-Z0-9._-]'), '-')}'; |
| method signature is unchanged; the friendly auto-rename UX now lives in the | ||
| InsForge dashboard rather than the API. | ||
| - **Storage `uploadAutoKey(filename, bytes)`** — now generates a unique, | ||
| collision-free key client-side (sanitized base + timestamp + random suffix) |
There was a problem hiding this comment.
P2: Repeated uploadAutoKey calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 22:
<comment>Repeated `uploadAutoKey` calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.</comment>
<file context>
@@ -4,6 +4,33 @@ All notable changes to the InsForge Dart/Flutter SDK are documented here. This
+ method signature is unchanged; the friendly auto-rename UX now lives in the
+ InsForge dashboard rather than the API.
+- **Storage `uploadAutoKey(filename, bytes)`** — now generates a unique,
+ collision-free key client-side (sanitized base + timestamp + random suffix)
+ and uploads through the standard `upload` path, so repeated uploads of the
+ same file never overwrite each other. The backend no longer mints keys
</file context>
Ports the storage changes from InsForge-sdk-js v1.5.0 (diff
v1.4.5..v1.5.0), paired with the backend change in InsForge/InsForge#1760.What was ported
upload(path, bytes)— standard PUT create-or-replace semantics. Uploading to a key that already exists now replaces the object in place; previously the server silently auto-renamed the key (photo.png→photo (1).png). Method signature unchanged — runtime behavior change only. Doc comments updated to state the new contract.uploadAutoKey(filename, bytes)— client-side key minting. Now generates a unique, collision-free key client-side (<sanitized-base>-<timestamp>-<random><ext>, mirroring the JSgenerateObjectKeyexactly: invalid chars →-, base truncated to 32 chars,filefallback, extension preserved) and uploads through the standardupload()path via PUT, instead of POSTing to/objectsfor a server-generated key — the storage API no longer mints keys server-side.upsertflag onupload,uploadAutoKey, andFileOptions(this repo's Dart-specific knob; the JS SDK never had it). Since PUT always replaces, the flag does nothing — thex-upsertheader is no longer sent. Kept in the signatures for source compatibility with@Deprecated.file_upload_test.dart(PUT to client-minted key, sanitization, no-extension and empty-filename fallbacks, distinct keys for repeated uploads, upsert no-op) mirroring the new JS unit tests; updated the integration lifecycle test to assert replace-in-place on re-upload to the same key, and theuploadAutoKeyintegration test to expect a client-generated key.0.1.0 → 0.2.0in lockstep viadart run tool/set_version.dart 0.2.0(regeneratesversion.dart), updated root + per-package changelogs, updated the root README storage snippet, and bumped the internalinsforge/insforge_flutterdep constraints inintegration_tests/andsamples/twitter_app/so the workspace resolves.What was skipped and why
storage.test.tsblocks coveringgetPublicUrlencoding,createSignedUrl/createSignedUrls, and the legacy POST download-strategy fallback — pre-existing JS surface/tests unrelated to the v1.5.0 API change (this SDK has no signed-URL helpers to sync here)..github/workflows/*,package.json/package-lock.json— JS repo plumbing.SDK-REFERENCE.mdone-liner — no equivalent reference doc here; the same note went into the Dart doc comments and README instead.Notes
Test results
Ran the full CI sequence locally with Flutter 3.44.7 stable (downloaded into the workspace; no toolchain was preinstalled):
dart format --output=none --set-exit-if-changed .— cleandart analyze .— no issuesdart run tool/set_version.dart --check— 0.2.0 in sync across both packages + version.dartpackages/insforge:dart test— 185/185 passedpackages/insforge_flutter:flutter analyze— no issues;flutter test— 5/5 passeddart pub publish --dry-runon both packages — only the expected dirty-git-state warning pre-commitLive integration tests (
integration_tests/) were not run — they are opt-in and need a real project viaINSFORGE_INTEGRATION_*env vars, which are not available here.🤖 Generated with Claude Code
Summary by cubic
Syncs storage with InsForge JS SDK v1.5.0:
uploadnow uses standard PUT replace semantics, anduploadAutoKeygenerates unique keys client‑side. Bumpsinsforgeandinsforge_flutterto 0.2.0 with updated docs and tests.New Features
upload(path, bytes): re-uploads replace in place (no auto-rename).uploadAutoKey(filename, bytes): mints a unique key client-side (<sanitized-base>-<timestamp>-<random><ext>) and PUTs to it.upsertflag (header removed); signatures kept with@Deprecated.x-upsert.Migration
upsert(it’s ignored).insforge0.2.0andinsforge_flutter0.2.0.Written for commit 1232198. Summary will update on new commits.