Sync with InsForge-sdk-js v1.5.0#7
Conversation
…1.5.0) Port InsForge/InsForge-sdk-js v1.5.0 storage changes: - upload_object: uploading to an existing key now replaces the object in place (standard PUT semantics), matching the backend change in InsForge/InsForge#1760. Signature unchanged; documented on the method. - upload_object_auto: the backend no longer mints/auto-renames keys, so the key is now generated client-side (sanitized base + timestamp + random suffix, extension preserved) and uploaded through the standard upload_object PUT path — repeated uploads of the same file never overwrite each other. The old POST /objects auto-name route is gone. - Bump version to 0.2.0 (runtime behavior change; requires a backend that includes the standard-PUT storage change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jwfing
left a comment
There was a problem hiding this comment.
Review: Sync with InsForge-sdk-js v1.5.0
Summary: A clean, faithful port of the JS v1.5.0 storage changes (standard-PUT semantics + client-side auto-key generation), with solid test coverage and no blocking issues.
Requirements context
No spec/plan under docs/superpowers/ matches this change — the design doc (docs/superpowers/specs/2026-03-28-python-sdk-design.md) and plan (docs/superpowers/plans/2026-03-28-python-sdk.md) predate v1.5.0 and still describe the old server-side auto-name route (POST /api/storage/buckets/{bucket}/objects, plan line ~584). I therefore assessed the port against its stated source of intent: JS generateObjectKey / uploadAuto at tag v1.5.0 (src/modules/storage.ts) and the linked backend PR InsForge/InsForge#1760. I fetched the JS source and diffed it against this implementation.
Findings
Critical
(none)
Suggestion
- Software engineering —
tests/storage/test_storage_client.py: Edge-case coverage is strong (sanitization, 32-char truncation, leading-dot, empty-base, non-ASCII, distinct keys) but there is no assertion for the common extensionless + non-empty base case (e.g."photo"→photo-<ts>-<rand>). The日本語case exercises thehas_ext == Falsebranch, but only where the whole base sanitizes to dashes, so the "plain name, no dot" happy path isn't directly pinned. A one-linere.fullmatch(r"photo-\d+-[a-z0-9]{6}", _generate_object_key("photo"))would close it.
Information
- Functionality — parity is exact.
insforge/storage/client.py:32-46matches JSgenerateObjectKeystep-for-step:rfind(".")↔lastIndexOf('.'),dot_index > 0,[^a-zA-Z0-9-_]→-sanitization,[:32]truncation,or "file"fallback, ms timestamp, and a 6-char0-9a-zrandom suffix (string.digits + string.ascii_lowercase↔ base36). Multi-dot names (archive.tar.gz→ basearchive-tar, ext.gz) behave identically to JS — intentional, not a defect. - Robustness nuance: JS's
Math.random().toString(36).slice(2,8)can occasionally yield fewer than 6 chars; the Pythonrandom.choices(..., k=6)always yields exactly 6. The Python version is marginally more robust here — worth being aware of only because it means the generated keys are not byte-identical across the two SDKs (they were never meant to be). - Security — no concern. The generated key is a collision-avoidance token, not a secret (objects are protected by bucket ACL/auth, not key obscurity), so the use of the non-cryptographic
randommodule is fine and matches JS'sMath.random(). Filename input is fully sanitized before it reaches the URL path (non-alphanumerics stripped, thenquote_path_segmentin_object_url_path), so no path-traversal surface viafilename. No secrets/PII newly logged (client.py:164logs only key + size). No auth changes. - Performance — no concern.
upload_object_autonow delegates to the existing singlePUTinstead of the old singlePOST; no added round-trips, loops, or allocations. - Note (pre-existing, out of scope): Python's
upload_objectdoes a directPUTand, unlike JSupload(), does not consult theupload-strategyendpoint / presigned path. This predates the PR and is not introduced by it — flagging only for awareness. - Versioning:
pyproject.tomlbump0.1.0→0.2.0is appropriate for a pre-1.0 behavior change; the PR correctly notes no CHANGELOG file exists in this repo (confirmed). - Backend coupling: This SDK now requires a backend with InsForge/InsForge#1760 (standard-PUT storage). The PR body documents this clearly.
Verdict
approved (informational — human approval via the GitHub approve flow is still required). No Critical findings; the one Suggestion is a minor test addition and the rest are informational. This is a well-scoped, well-tested port.
There was a problem hiding this comment.
2 issues found across 3 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="tests/storage/test_storage_client.py">
<violation number="1" location="tests/storage/test_storage_client.py:450">
P1: This assertion will raise a `KeyError` because `upload_object` sends data as `content=data`, not as multipart `files=...`. The `upload_object_auto` method now delegates to `upload_object`, which makes a `PUT` request with `content=data` (raw bytes) and a `Content-Type` header — not with the `files=` parameter that multipart uploads use. The `captured["kwargs"]` dict will contain `"content"` and `"headers"` keys, not `"files"`.</violation>
<violation number="2" location="tests/storage/test_storage_client.py:493">
P1: `_generate_object_key("日本語")` produces `---<timestamp>-<random6>` (3-dash sanitized base), but the regex in the test uses `----` (4 dashes). This assertion will fail at runtime. Update the assertion to match the actual key format: `---\d+-[a-z0-9]{6}`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| key = _generate_object_key("a" * 50 + ".txt") | ||
| assert re.fullmatch(r"a{32}-\d+-[a-z0-9]{6}\.txt", key) | ||
| # Disallowed characters are each replaced with a dash. | ||
| assert re.fullmatch(r"----\d+-[a-z0-9]{6}", _generate_object_key("日本語")) |
There was a problem hiding this comment.
P1: _generate_object_key("日本語") produces ---<timestamp>-<random6> (3-dash sanitized base), but the regex in the test uses ---- (4 dashes). This assertion will fail at runtime. Update the assertion to match the actual key format: ---\d+-[a-z0-9]{6}.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/storage/test_storage_client.py, line 493:
<comment>`_generate_object_key("日本語")` produces `---<timestamp>-<random6>` (3-dash sanitized base), but the regex in the test uses `----` (4 dashes). This assertion will fail at runtime. Update the assertion to match the actual key format: `---\d+-[a-z0-9]{6}`.</comment>
<file context>
@@ -403,6 +407,96 @@ async def fake_request(method: str, url: httpx.URL, **kwargs: object) -> httpx.R
+ key = _generate_object_key("a" * 50 + ".txt")
+ assert re.fullmatch(r"a{32}-\d+-[a-z0-9]{6}\.txt", key)
+ # Disallowed characters are each replaced with a dash.
+ assert re.fullmatch(r"----\d+-[a-z0-9]{6}", _generate_object_key("日本語"))
+ # An empty base falls back to "file".
+ assert re.fullmatch(r"file-\d+-[a-z0-9]{6}", _generate_object_key(""))
</file context>
| assert re.fullmatch(r"----\d+-[a-z0-9]{6}", _generate_object_key("日本語")) | |
| assert re.fullmatch(r"---\d+-[a-z0-9]{6}", _generate_object_key("日本語")) |
| assert captured["method"] == "PUT" | ||
| assert re.fullmatch(r"report-\d+-[a-z0-9]{6}\.pdf", result.key) | ||
| assert captured["url"].endswith(f"/api/storage/buckets/docs/objects/{result.key}") | ||
| assert captured["kwargs"]["files"]["file"] == (result.key, b"pdf", "application/pdf") |
There was a problem hiding this comment.
P1: This assertion will raise a KeyError because upload_object sends data as content=data, not as multipart files=.... The upload_object_auto method now delegates to upload_object, which makes a PUT request with content=data (raw bytes) and a Content-Type header — not with the files= parameter that multipart uploads use. The captured["kwargs"] dict will contain "content" and "headers" keys, not "files".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/storage/test_storage_client.py, line 450:
<comment>This assertion will raise a `KeyError` because `upload_object` sends data as `content=data`, not as multipart `files=...`. The `upload_object_auto` method now delegates to `upload_object`, which makes a `PUT` request with `content=data` (raw bytes) and a `Content-Type` header — not with the `files=` parameter that multipart uploads use. The `captured["kwargs"]` dict will contain `"content"` and `"headers"` keys, not `"files"`.</comment>
<file context>
@@ -403,6 +407,96 @@ async def fake_request(method: str, url: httpx.URL, **kwargs: object) -> httpx.R
+ assert captured["method"] == "PUT"
+ assert re.fullmatch(r"report-\d+-[a-z0-9]{6}\.pdf", result.key)
+ assert captured["url"].endswith(f"/api/storage/buckets/docs/objects/{result.key}")
+ assert captured["kwargs"]["files"]["file"] == (result.key, b"pdf", "application/pdf")
+
+
</file context>
| assert captured["kwargs"]["files"]["file"] == (result.key, b"pdf", "application/pdf") | |
| assert captured["kwargs"]["content"] == b"pdf" | |
| assert captured["kwargs"]["headers"]["Content-Type"] == "application/pdf" |
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.Ported
upload_object(bucket, key, data)— now documented with standard object-storage PUT semantics: uploading to an existing key replaces the object in place (previously the server silently auto-renamed, e.g.photo.png→photo (1).png). Signature unchanged; this is a runtime behavior change driven by the backend.upload_object_auto(bucket, data=..., filename=...)— the backend no longer mints keys, so the unique, collision-free key is now generated client-side (<sanitized-base>-<timestamp-ms>-<random6><ext>, base capped at 32 chars,filefallback for an empty base — same algorithm as the JSgenerateObjectKey) and the upload is routed through the standardupload_objectPUT path instead of the oldPOST /objectsauto-name route. Repeated uploads of the same file never overwrite each other. Signature unchanged.test_upload_auto_and_strategy_endpointsfor the new PUT route, and added tests mirroring the new JS coverage: client-side key minting via PUT, distinct keys for repeated uploads of the same filename, and key-generation edge cases (sanitization, 32-char truncation, leading-dot filenames, empty-base fallback).pyproject.tomlto0.2.0(behavior change, pre-1.0). No changelog file exists in this repo, so none was updated.Skipped
package.json/package-lock.jsonversion plumbing, andSDK-REFERENCE.md(JS-specific; the Python equivalent is the docstring onupload_object, and this repo's README does not documentupload_object_auto).upsert/autoKeyflags existed in the Python SDK, so nothing to remove there.Notes
Test results
pyteston Python 3.12: 88 passed, 15 skipped (skips are live-backend integration tests).🤖 Generated with Claude Code
Summary by cubic
Syncs the Python SDK’s storage behavior with InsForge-sdk-js v1.5.0, adopting standard PUT semantics and client-side auto-key generation. This removes server auto-renaming and ensures collision-free uploads.
New Features
upload_object: PUT to an existing key replaces the object in place.upload_object_auto: generates a sanitized unique key client-side (<base>-<timestamp>-<random><ext>) and uploads via PUT; repeated uploads never overwrite.Migration
insforgeto0.2.0.Written for commit 765248b. Summary will update on new commits.