fix(server): restore put_document_file's workspace_name parameter - #248
Conversation
The obstore refactor renamed the parameter to `workspace` but left both callers passing `workspace_name=`, so every document upload raised TypeError and returned a 500: api/handlers/v1/documents.py:82 POST /api/v1/documents contexts/imports.py:432 bulk import v0.7.1's integration suite caught it, which is why that release was never published to PyPI. The unit test passed the argument positionally, so the mismatch was invisible to the unit suite that gates most PRs — it now calls by keyword, which is what makes this class of break detectable there. Renames the parameter rather than the two call sites, keeping the public signature identical to v0.7.0 so no caller outside this repo has to change.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesDocument file workspace handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This restores document uploads and bulk imports that were failing because callers used the public 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Your free Security trial is over. An organization admin can activate billing to continue. 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. Comment |
Blocks a 0.7.2 release.
v0.7.1was cut, failed its integration suite, and was never published — this is the fix that makes it publishable.The break
The obstore refactor (#244/#246) renamed
put_document_file's second parameter toworkspacebut left both callers passingworkspace_name=:api/handlers/v1/documents.py:82POST /api/v1/documentscontexts/imports.py:432Every document upload raised
TypeError: put_document_file() got an unexpected keyword argument 'workspace_name'and returned a 500. Onv0.7.1:Why the unit suite missed it
tests/unit/contexts/test_files_store.py:181called it positionally, so the signature could drift from its callers without any unit test noticing. Integration caught it, but integration doesn't gate most PRs. That test now calls by keyword — the change that makes this class of break visible where it needs to be.Which side to fix
I renamed the parameter rather than the two call sites. Worth stating the tradeoff, because the other direction is defensible:
put_document_fileis exported and itsv0.7.0signature tookworkspace_name. Renaming it back keeps the public signature stable, so no out-of-repo caller has to change across the 0.7.x line.contexts/files.pyotherwise usesworkspaceuniversally —_put,put_object,get_object,delete_object,list_objects,delete_workspace_objects,delete_document_artifactsall do, and the module now contains exactly oneworkspace_name. By the module's own convention the callers were the outliers.Happy to flip it to updating the two call sites if the reviewer prefers internal consistency over signature stability.
Verification
An AST scan of every keyword call against
contexts/files.pysignatures, run on both revisions, confirms the fix is complete and that this was the only such mismatch:tests/unit: 1971 passed, 42 skipped. Three failures intests/unit/security(test_jwt,test_default_security_settings) reproduce identically on unmodifiedmainin the same environment and are unrelated — CI is green onmain.Summary by CodeRabbit