feat(artifacts): versioning, edits, comments & per-artifact sharing platform#78
Open
ianu82 wants to merge 8 commits into
Open
feat(artifacts): versioning, edits, comments & per-artifact sharing platform#78ianu82 wants to merge 8 commits into
ianu82 wants to merge 8 commits into
Conversation
…ion-2) - artifact_edits: POST /artifacts/edits/propose (non-mutating diff) and /accept with optimistic concurrency — base-version compare-and-swap, HTTP 409 (+currentVersionId) on a stale base — reusing the existing patch-apply + snapshot primitives (operation_type="ai_edit"). - identity + artifact_shares: User model, per-artifact ShareGrant, and a lightweight-signup guest accept flow (token hashed at rest, replay-safe); migration a1b2c3d4e5f6 chained on f1c2d3e4a5b6. - Registered models + routers. TODO(M3): enforce per-artifact capability on the share endpoints (currently unguarded by design — scaffold). Full suite 241 passed / 1 skipped + 9 new; single alembic head. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… metadata For shared-DB compatibility when running direction-2 against a copy of the live database: include the direction-a `9b7c6d5e4f3a` artifact-metadata migration and re-chain `a1b2c3d4e5f6` (identity + artifact_shares) onto it, giving a single linear head identical to the DB's history (…f1c2d3e4a5b6 → 9b7c6d5e4f3a → a1b2c3d4e5f6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to the snapshot accept_edit takes operation_type (default "ai_edit"); a user's direct typed edit passes "manual_edit" so versions distinguish human edits from AI edits, and the actor (name/email/subject from the JWT principal) now flows into the version's activity event (was previously accepted but dropped). Default behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rt fresh Artifacts are keyed by folder path (artifacts.path UNIQUE). Deleting an artifact soft-deletes it (folder removed, record + versions kept for recovery) but never released the path — so creating a new artifact at the same folder (e.g. a deck with the same title/slug) resolved straight back to the old record and inherited its entire version history and comments. - delete_artifact_endpoint: after recording the "deleted" event (which preserves the original path in its details), tombstone artifact.path (`<path>#deleted-<uuid>`). A new artifact at the original path now finds no match and is created fresh. - restore_artifact: a deleted artifact's path is tombstoned, so when restoring one, resolve its ORIGINAL folder from the "deleted" event and heal the record's path — restoring to the original location, or a `-restored` sibling if that path is now taken. Recovery is preserved. - Add _deleted_original_path() helper (mirrors _deleted_external_artifact_id). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on restore The path-release fix freed uq_artifacts_path but not uq_artifacts_project_slug, so re-creating an artifact at the same name still hit a UNIQUE(project_id, slug) IntegrityError → 500 on the first edit/propose of the new artifact. - delete: tombstone artifact.slug alongside artifact.path (same uuid). - restore: when landing a deleted artifact back, pick a folder whose path AND slug (basename) are both free (—restored sibling if taken) and heal both fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
From the parallel backend review: - _get_or_create_artifact: pre-uniquify the slug within the project (new _unique_project_slug) so a (project_id, slug) collision no longer throws an unhandled IntegrityError → opaque 500 on the first edit of a new artifact. - restore_artifact landing-spot: the free-spot loop now checks path-taken (not just slug-taken) against other artifact rows, closing a uq_artifacts_path 500 on the heal commit. - _deleted_artifact_card: show the ORIGINAL slug/path (from the deleted event, else strip the #deleted-<uuid> tombstone) instead of leaking the tombstone string into the trash UI. - Remove the unused `_delete_artifact` import; mark the dead hard-delete delete_artifact() DEPRECATED (kept: a test imports it). - Log (warning + exc_info) the previously-silent except blocks in _mark_live_preview_ready / _record_live_preview_failure. - Atomic metadata.json port write (temp file + os.replace) so a concurrent reader can't see truncated JSON (which made artifacts vanish from listings). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Delete tombstone could overflow slug (255) / path (2048) under validate_assignment, turning delete of a long-slug artifact into a 500. Truncate the base before suffixing (new _tombstone helper). - _get_or_create_artifact only uniquified the slug on INSERT; the UPDATE branch could assign a colliding (project_id, slug) and 500 on a rename. Uniquify there too (excludes own path, so an unchanged slug stays stable). - Centralize the "#deleted-" tombstone prefix as a constant; collapse the two byte-identical deleted-event queries into _latest_deleted_details; use _actor_kwargs() in the comment-status endpoints; comment the by-name dispatch aliases so they aren't mistaken for dead code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
Author
|
CodeQL checks are failing but as far as I can tell, not because of changes in this PR: three independent pieces of evidence:
This pattern appears at ~20 sites across services/artifacts.py, endpoints/artifacts.py, and artifact_versions.py (_safe_relative_path). py/path-injection routinely can't follow an exception-based guard, so it false-positives on exactly this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
This is the backend platform behind the artifacts workspace: everything the server needs to version an artifact, edit it safely, comment on it, and share/publish it per-artifact. It's the server half of the feature — the UI is the companion PR in
mindsdb/cowork.It's a big diff, but about a third of it is tests. Here's the map.
What it provides
How to review a ~19k-line diff
tests/test_artifact_*,tests/test_project_collaboration.pyservices/artifact_versions.pyapi/v1/endpoints/artifact_versions.py,endpoints/artifacts.pyservices/project_collaboration.py,artifact_shares.py,publish.pymodels/artifact.py+db/alembic/versions/d3a5c9e7b1f2_artifact_versions.pyHow it's kept safe to merge
main— merges cleanly today.Testing
Comprehensive endpoint + service test suites (the ~6k lines of tests above). Core modules
py_compileclean and the server boots.Heads-up for the reviewer
event_typeindex; propose/accept transaction atomicity) are noted as deferred — none blocking.Companion PR (frontend): mindsdb/cowork#213
🤖 Generated with Claude Code