feat: download native ChatGPT files into workspaces#88
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opt-in ChangesNative file download
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/db/schema.ts (1)
97-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign Drizzle index definitions with the migration's DESC ordering.
migrateArtifactExchangecreatesartifacts_workspace_idx/artifacts_client_idxonlast_used_at descandartifact_uploads_client_idxoncreated_at desc, but these schema definitions omit the descending order. Since the migration is the source of truth, this drift is harmless today, but it misrepresents the applied indexes and would be lost if migrations are ever regenerated from the schema. Add.desc()to match.♻️ Proposed alignment
- index("artifacts_workspace_idx").on(table.workspaceId, table.lastUsedAt), - index("artifacts_client_idx").on(table.clientId, table.lastUsedAt), + index("artifacts_workspace_idx").on(table.workspaceId, table.lastUsedAt.desc()), + index("artifacts_client_idx").on(table.clientId, table.lastUsedAt.desc()),- index("artifact_uploads_client_idx").on(table.clientId, table.createdAt), + index("artifact_uploads_client_idx").on(table.clientId, table.createdAt.desc()),Also applies to: 124-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/schema.ts` around lines 97 - 98, Update the index definitions for artifacts_workspace_idx and artifacts_client_idx to apply descending order to lastUsedAt, and update artifact_uploads_client_idx similarly for createdAt. Match the DESC ordering used by migrateArtifactExchange so the schema accurately represents the migration-defined indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 32: Update the commented DEVSPACE_ARTIFACT_ROOT example in .env.example
to use the generic ~/.local/share/devspace/artifacts path, matching the
documented default and removing the personal username.
In `@src/config.ts`:
- Around line 258-262: Update the artifactDefaultTtlHours configuration parsing
to enforce MAX_TTL_HOURS as its upper bound, while preserving the existing
positive-integer validation and default behavior. Use the existing MAX_TTL_HOURS
symbol rather than introducing a duplicate limit, so uploads using the
configured default remain valid.
---
Nitpick comments:
In `@src/db/schema.ts`:
- Around line 97-98: Update the index definitions for artifacts_workspace_idx
and artifacts_client_idx to apply descending order to lastUsedAt, and update
artifact_uploads_client_idx similarly for createdAt. Match the DESC ordering
used by migrateArtifactExchange so the schema accurately represents the
migration-defined indexes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8458deb-e1ad-48b0-a198-c2656551e358
📒 Files selected for processing (18)
.env.exampleREADME.mddocs/artifact-exchange.mddocs/configuration.mddocs/security.mdpackage.jsonsrc/artifact-tools.tssrc/artifacts.test.tssrc/artifacts.tssrc/config.test.tssrc/config.tssrc/db/migrations.tssrc/db/schema.tssrc/incoming-artifacts.test.tssrc/incoming-artifacts.tssrc/oauth-store.test.tssrc/server.tssrc/user-config.ts
a162350 to
bb1d3e3
Compare
|
Addressed the initial review findings in a single amended commit:
Re-ran |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/artifact-workspace.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for this security-sensitive module.
Only the "error" mode happy path is tested. Given
copyArtifactToWorkspaceis responsible for path-containment, symlink rejection, and multiple conflict-resolution modes, consider adding cases for: destination outside the workspace root, symlinked destination/parent, pre-existing destination underonConflict: "error", and"rename"/"replace"behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/artifact-workspace.test.ts` around lines 1 - 59, Extend the test coverage around copyArtifactToWorkspace with negative and conflict-mode cases: reject destinations outside workspaceRoot, reject symlinked destinations and parents, and preserve the existing destination when onConflict is "error". Add assertions that "rename" creates a distinct destination and "replace" overwrites the existing file, including returned path/renamed metadata where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 31: Update the “test” script in package.json to include
src/local-agent-targets.test.ts in the chained tsx test commands, preserving the
existing ordering and execution behavior of the other suites.
In `@src/artifact-workspace.ts`:
- Around line 48-104: Update the failure cleanup in the artifact materialization
flow around copyVerifiedArtifactToTemp and the surrounding try/catch so that,
once the artifact has been materialized, failures from chmod,
assertContainedWorkspaceRegularFile, or post-move integrity verification also
remove finalPath. Preserve cleanup for the temporary path, avoid deleting
unrelated pre-existing destinations, and ensure replace mode does not leave
failed content behind; consider deferring its destructive rename until
verification can succeed if needed.
---
Nitpick comments:
In `@src/artifact-workspace.test.ts`:
- Around line 1-59: Extend the test coverage around copyArtifactToWorkspace with
negative and conflict-mode cases: reject destinations outside workspaceRoot,
reject symlinked destinations and parents, and preserve the existing destination
when onConflict is "error". Add assertions that "rename" creates a distinct
destination and "replace" overwrites the existing file, including returned
path/renamed metadata where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b152289b-b5d8-488a-a3b6-f0b9a9a3e97d
📒 Files selected for processing (10)
docs/artifact-exchange.mddocs/configuration.mddocs/security.mdpackage.jsonsrc/artifact-tools.tssrc/artifact-workspace.test.tssrc/artifact-workspace.tssrc/incoming-artifacts.test.tssrc/incoming-artifacts.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/security.md
- docs/configuration.md
- src/server.ts
- src/incoming-artifacts.test.ts
- src/incoming-artifacts.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/artifact-workspace.test.ts (1)
9-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage matching the
copyArtifactToWorkspacecontract. Error/rename/replace conflict handling, path escape, and symlink-based unsafe parent/destination cases all correctly assert the documented error codes (workspace_destination_exists,workspace_path_escape,workspace_parent_unsafe,workspace_destination_unsafe).Two edge cases from the contract remain untested:
workspace_destination_invalid(destination equal toworkspaceRoot) andworkspace_rename_exhausted(all rename attempts collide). Not blocking, but would round out coverage of the security boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/artifact-workspace.test.ts` around lines 9 - 145, Extend the copyArtifactToWorkspace test coverage with a destination equal to workspaceRoot and assert workspace_destination_invalid. Add a rename-conflict case that pre-creates every candidate rename path until the configured limit and assert workspace_rename_exhausted, while preserving the existing artifact and cleanup setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/artifact-workspace.test.ts`:
- Around line 9-145: Extend the copyArtifactToWorkspace test coverage with a
destination equal to workspaceRoot and assert workspace_destination_invalid. Add
a rename-conflict case that pre-creates every candidate rename path until the
configured limit and assert workspace_rename_exhausted, while preserving the
existing artifact and cleanup setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df2aa18b-b755-4e9a-8dab-d05d74534527
📒 Files selected for processing (3)
package.jsonsrc/artifact-workspace.test.tssrc/artifact-workspace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/artifact-workspace.ts
|
@jplew Really cool feature and PR I'ven't gone through whole changes yet but these are my first impressions
philosophy of devspace usually is to keep things very minimally expose to model as much as possible and handle at tooling level if it's possible will test it by tomorrow but yeah pretty cool idea which i never thought could be possible (it seems like chatgpt apps sdk are pretty powerful) |
Thanks for reviewing, all good points. I can definitely abstract away internals and consolidate everything into a single call. Will post a follow-up soon. |
|
done, condensed everything into a single call. |
Waishnav
left a comment
Waishnav
left a comment
There was a problem hiding this comment.
Thanks for consolidating this into one model-facing call — that direction matches DevSpace well. I ran the full test suite, typecheck, build, and git diff --check; they pass.
Before merging, I think this should be trimmed to a one-shot handoff rather than a persistent artifact service:
download_artifact({ file, workspaceId }) -> { path }DevSpace can choose a collision-free path under .devspace/incoming/; existing filesystem tools can move/rename/delete it. This would let us remove model-facing destination, onConflict, and expectedSha256, plus the internal upload/chunk/TTL/pinning/quota/database lifecycle.
There are also four filesystem-safety blockers in the current implementation:
ensureSecureDirectory()creates/chmods before proving containment, so an intermediate symlink can cause changes outside the artifact root.- Workspace materialization performs path-based chmod/hash/cleanup after rename, leaving a TOCTOU window where the destination can be swapped.
- Artifact deletion removes the DB row before successfully removing the object; failures leave permanent untracked bytes and incorrect quota/cleanup state.
- Startup unconditionally chmods an existing configured artifact root to
0700, which can silently break shared or service-managed directories.
Please simplify the implementation and address these before merge. The native-file adapter, streaming validation, size enforcement, safe filename handling, and tests are all valuable and can remain in the smaller design.
jplew
changed the title
|
@Waishnav I made the requested change. Test looks like this so it appears to be working: Last message in attached convo is an easy move. |
|
@jplew heyo can you retest real quick, i've added back the |
Yes will retest your changes after a few hours |
|
@Waishnav Yes, I tested on the latest commit (45fb361) and it's still working as expected, see screenshot here: https://media.haft.sh/dev/asset/asset-img-4215022a5cef-png/01cf6042/img-4215022a5cef.png Tested with an image and a markdown file |
|
@coderabbitai can you do one last review before merging this |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/artifact-tools.ts`:
- Around line 46-53: Update openAIFileReferenceInputSchema to use Zod’s strict
object schema so unknown fields are rejected rather than stripped, while
preserving the existing field definitions and validation rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47532b07-4788-4893-a21d-a57689a2d2ea
📒 Files selected for processing (15)
.env.exampleREADME.mddocs/artifact-exchange.mddocs/configuration.mddocs/security.mdpackage.jsonsrc/artifact-download.test.tssrc/artifact-error.tssrc/artifact-tools.tssrc/config.test.tssrc/config.tssrc/incoming-artifacts.test.tssrc/incoming-artifacts.tssrc/server.tssrc/user-config.ts
|
@Waishnav want me to address these coderabbit requested changes? |
| ## Native File Download | ||
|
|
||
| Native file download is an opt-in, one-shot byte-transfer seam. It has no | ||
| persistent artifact root, object database, upload lifecycle, reusable artifact | ||
| ID, public download route, TTL, pinning, or quota ledger. | ||
|
|
||
| The production server declares ChatGPT's top-level `openai/fileParams` contract | ||
| and accepts only the documented `download_url`, `file_id`, optional | ||
| MIME/filename aliases, and optional size. Downloads use HTTPS on | ||
| `files.oaiusercontent.com` or the constrained regional OpenAI Azure account | ||
| family `oaisdmntpr<region>.blob.core.windows.net`, where `<region>` is lowercase | ||
| alphanumeric. Arbitrary Azure Blob accounts, alternate ports, credentials, | ||
| fragments, malformed IDs, extra fields, and redirects outside that boundary fail | ||
| closed. Opaque IDs are bounded metadata and are never used as filenames or path | ||
| components. | ||
|
|
||
| `download_artifact` accepts only the native file value, a `workspaceId` returned | ||
| by `open_workspace`, and a relative destination `path`. Absolute paths, | ||
| traversal, symlinked parents, and existing destinations fail closed. Callers | ||
| cannot supply a conflict mode, expected hash, host path, or storage policy. | ||
|
|
There was a problem hiding this comment.
Addressed in ed2702b: reduced the native file security section to concise operator-facing behavior—native host file only, trusted handling, one-shot workspace transfer, path protections, and no arbitrary URL/local-path ingestion.
| @@ -0,0 +1,96 @@ | |||
| # Native file download | |||
There was a problem hiding this comment.
docs file changes can be trimmed and re-write it from user's POV
There was a problem hiding this comment.
Addressed in ed2702b: rewrote the artifact download guide around the user workflow (open_workspace → download_artifact → returned path), then normal filesystem tools. It remains aligned with the current required path parameter restored on the PR head.
I addressed all the requests. Or rather, I got devspace to do it, so if it looks like slop, I'm sorry but you have to take it up with your own creation. :) |
|
lol i meant it's not the problem with what you did, tests suite and test patterns was slop earlier even before your contribution. But yeah, thanks for sticking with us throughout the review process for this really cool feature :) |
2 participants
Summary
Replace the artifact-service lifecycle with one one-shot handoff:
DevSpace validates the native MCP-host file reference, streams it directly into the selected workspace under
.devspace/incoming/, chooses a normalized collision-free filename, and returns only the workspace-relative path.Tool contract
download_artifact.filevalue and an existingworkspaceId.openai/fileParams: ["file"].{ path }.Implementation
DEVSPACE_ARTIFACT_MAX_FILE_BYTESwhile computing SHA-256..devspace/incoming/with deterministic collision suffixes.Removed lifecycle
Migration behavior
Existing legacy artifact tables and bytes are intentionally left untouched. The new implementation does not read, mutate, or opportunistically delete prior user data; cleanup can be performed manually after review.
Verification
npm testnpm run typechecknpm run buildgit diff --checkNo runtime deployment or service restart was performed as part of this source-only handoff.
Summary by CodeRabbit
New Features
DEVSPACE_ARTIFACTS=1and is available on supported non-Windows platforms.Documentation
Tests