feat(file-system): add versioned instructions for desktop folders#61494
Merged
Conversation
Contributor
MCP UI Apps size report
|
Contributor
|
Size Change: 0 B Total Size: 82.2 MB ℹ️ View Unchanged
|
Contributor
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeBrief or no lock, backwards compatible Last updated: 2026-06-06 19:43 UTC (b7aadfa) |
Adds per-folder, versioned markdown instructions to the desktop filesystem surface, modeled on the skills store. - New FileSystemFolderInstructions model (TeamScopedRootMixin), keyed to the FileSystem folder row, with content/version/is_latest/deleted and partial unique constraints (one latest + unique version per folder). - Versioning service (publish/get-latest/list-versions/soft-delete) with optimistic-concurrency base_version check and a 100KB size cap. - instructions + instructions/versions detail actions on DesktopFileSystemViewSet only, so the web tree is unaffected. - API tests covering versioning, size cap, folder guard, cross-team IDOR, web-surface isolation, soft delete, and concurrency conflict. Generated-By: PostHog Code Task-Id: b4b691b4-2b7c-418e-9ffe-b5c37e1f0a22
Lets agents create desktop channels (folders) and manage their instructions, and guarantees every channel has an instruction set. - Auto-create a blank version-1 instruction set when a desktop folder is created, backfilling every ancestor folder along the path. Idempotent ensure_blank_folder_instructions helper; never clobbers existing content. - Enable desktop file-system create/list/retrieve and the instructions retrieve/update/versions tools in the MCP core.yaml. Instructions content can be erased by publishing empty content; the instruction set itself cannot be deleted (destroy tool left disabled). - Update tests for the new versioning baseline (fresh channel = blank v1, first edit = v2) and ancestor backfill. Note: hogli build:openapi + MCP codegen must run in a full dev env to regenerate the OpenAPI spec, generated tool handlers/schemas, and frontend types. Generated-By: PostHog Code Task-Id: b4b691b4-2b7c-418e-9ffe-b5c37e1f0a22
Address failing CI checks on the desktop folder instructions PR: - mypy: drop the invalid .unscoped() on FileSystem's plain manager and cast folder_id to UUID in the test lookups. - IDOR coverage: register FileSystemFolderInstructions in the team-scoped semgrep IDOR rules. - MCP unit tests: commit the generated tool-schema snapshots for the newly enabled desktop channel/instructions tools and the updated exec-tool catalog. Generated-By: PostHog Code Task-Id: b4b691b4-2b7c-418e-9ffe-b5c37e1f0a22
raquelmsmith
force-pushed
the
posthog-code/desktop-folder-instructions
branch
from
d7d489a to
ba54e55
Compare
Contributor
|
🎭 Playwright report
These issues are not necessarily caused by your changes. |
…p-folder-instructions # Conflicts: # posthog/migrations/max_migration.txt # services/mcp/tests/unit/__snapshots__/exec-tool.json
DRF's @instructions.mapping.{put,patch,delete} resolve view.action to
publish_instructions / delete_instructions, which weren't in the default
read/write action lists, so APIScopePermission returned None and rejected
PAK auth with "This action does not support personal API key access".
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
raquelmsmith
added a commit
to PostHog/code
that referenced
this pull request
Jun 6, 2026
Wires the desktop folder-instructions endpoints from PostHog/posthog#61494 into a per-folder CONTEXT.md leaf in the channels sidebar. Clicking opens a deep-linkable route with rendered/edit toggle, save publishes a new version with optimistic concurrency, and a version dropdown surfaces history. Refetches on every mount so the view reflects edits made by other users or agents. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/api/file_system/file_system.py:1044-1053
**Response shape mismatch between implementation and generated client**
`instructions_versions` returns a raw list, but the generated TypeScript client (`desktopFileSystemInstructionsVersionsList`) declares `Promise<PaginatedFolderInstructionsVersionListApi>` — which wraps results in `{ count, next, previous, results: [] }`. Any desktop client using the generated types will attempt to access `.results` on a flat array and get `undefined`.
drf-spectacular infers a paginated schema here because `DesktopFileSystemViewSet` inherits `pagination_class = FileSystemsLimitOffsetPagination`, and the `@extend_schema(responses={200: FolderInstructionsVersionSerializer(many=True)})` decorator is not sufficient to suppress that wrapping. The fix is either:
1. Actually paginate — replace the manual `Response(...)` with `self.paginate_queryset(versions)` / `self.get_paginated_response(...)` (matches the `limit`/`offset` params already present in `DesktopFileSystemInstructionsVersionsListParams`), or
2. Suppress the wrapper — add `paginator=None` inside `@extend_schema` to prevent drf-spectacular from wrapping the response.
The test at line 104 (`[v["version"] for v in versions]`) also silently validates the flat-array shape and will need updating if pagination is added.
Reviews (1): Last reviewed commit: "chore(file-system): format rebased migra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1044
to
+1053
| @extend_schema(responses={200: FolderInstructionsVersionSerializer(many=True)}) | ||
| @action(methods=["GET"], detail=True, url_path="instructions/versions") | ||
| def instructions_versions(self, request: Request, *args: Any, **kwargs: Any) -> Response: | ||
| """List the version history for this folder's instructions, newest first.""" | ||
| folder = self._get_folder_or_400() | ||
| if isinstance(folder, Response): | ||
| return folder | ||
|
|
||
| versions = get_folder_instructions_versions(folder) | ||
| return Response(FolderInstructionsVersionSerializer(versions, many=True).data) |
Contributor
There was a problem hiding this comment.
instructions_versions returns a raw list, but the generated TypeScript client (desktopFileSystemInstructionsVersionsList) declares Promise<PaginatedFolderInstructionsVersionListApi> — which wraps results in { count, next, previous, results: [] }. Any desktop client using the generated types will attempt to access .results on a flat array and get undefined.
drf-spectacular infers a paginated schema here because DesktopFileSystemViewSet inherits pagination_class = FileSystemsLimitOffsetPagination, and the @extend_schema(responses={200: FolderInstructionsVersionSerializer(many=True)}) decorator is not sufficient to suppress that wrapping. The fix is either:
- Actually paginate — replace the manual
Response(...)withself.paginate_queryset(versions)/self.get_paginated_response(...)(matches thelimit/offsetparams already present inDesktopFileSystemInstructionsVersionsListParams), or - Suppress the wrapper — add
paginator=Noneinside@extend_schemato prevent drf-spectacular from wrapping the response.
The test at line 104 ([v["version"] for v in versions]) also silently validates the flat-array shape and will need updating if pagination is added.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/api/file_system/file_system.py
Line: 1044-1053
Comment:
**Response shape mismatch between implementation and generated client**
`instructions_versions` returns a raw list, but the generated TypeScript client (`desktopFileSystemInstructionsVersionsList`) declares `Promise<PaginatedFolderInstructionsVersionListApi>` — which wraps results in `{ count, next, previous, results: [] }`. Any desktop client using the generated types will attempt to access `.results` on a flat array and get `undefined`.
drf-spectacular infers a paginated schema here because `DesktopFileSystemViewSet` inherits `pagination_class = FileSystemsLimitOffsetPagination`, and the `@extend_schema(responses={200: FolderInstructionsVersionSerializer(many=True)})` decorator is not sufficient to suppress that wrapping. The fix is either:
1. Actually paginate — replace the manual `Response(...)` with `self.paginate_queryset(versions)` / `self.get_paginated_response(...)` (matches the `limit`/`offset` params already present in `DesktopFileSystemInstructionsVersionsListParams`), or
2. Suppress the wrapper — add `paginator=None` inside `@extend_schema` to prevent drf-spectacular from wrapping the response.
The test at line 104 (`[v["version"] for v in versions]`) also silently validates the flat-array shape and will need updating if pagination is added.
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Good catch — fixed in 57aef59. Switched instructions_versions to use self.paginate_queryset / self.get_paginated_response so the runtime shape matches the generated PaginatedFolderInstructionsVersionListApi contract, and updated the test to read response.json()["results"]. Pagination matches the limit/offset params already declared on DesktopFileSystemInstructionsVersionsListParams. Will need hogli build:openapi rerun before merge to confirm no schema drift.
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Contributor
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
adamleithp
approved these changes
Jun 6, 2026
Use APIClient.credentials() instead of unpacking **headers, which mypy flagged against the strict get/patch signatures. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DesktopFileSystemViewSet inherits FileSystemsLimitOffsetPagination, so
drf-spectacular generates a paginated response schema for the versions
endpoint. Returning a flat list broke the generated TS client's
{count,next,previous,results} contract.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…p-folder-instructions # Conflicts: # posthog/migrations/max_migration.txt
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+62
to
+72
| if current_latest is None: | ||
| if base_version is not None and base_version != 0: | ||
| raise FolderInstructionsVersionConflictError(current_version=0) | ||
| return FileSystemFolderInstructions.objects.create( | ||
| team=folder.team, | ||
| folder=folder, | ||
| content=content, | ||
| version=1, | ||
| is_latest=True, | ||
| created_by=user, | ||
| ) |
Contributor
There was a problem hiding this comment.
Race condition when creating the first version. If two requests simultaneously try to publish the first version for a folder, both will find current_latest=None (since select_for_update() has nothing to lock), and both will attempt to create version 1. One will succeed and the other will raise an unhandled IntegrityError due to the unique_folder_instructions_version constraint.
Fix: Wrap the create in a try-except to handle the IntegrityError:
if current_latest is None:
if base_version is not None and base_version != 0:
raise FolderInstructionsVersionConflictError(current_version=0)
try:
return FileSystemFolderInstructions.objects.create(
team=folder.team,
folder=folder,
content=content,
version=1,
is_latest=True,
created_by=user,
)
except IntegrityError:
# Concurrent create won; retry to find the latest version
current_latest = (
FileSystemFolderInstructions.objects.select_for_update()
.filter(folder=folder, deleted=False, is_latest=True)
.order_by("-version", "-created_at", "-id")
.first()
)
# Continue to the version increment logic below
Suggested change
| if current_latest is None: | |
| if base_version is not None and base_version != 0: | |
| raise FolderInstructionsVersionConflictError(current_version=0) | |
| return FileSystemFolderInstructions.objects.create( | |
| team=folder.team, | |
| folder=folder, | |
| content=content, | |
| version=1, | |
| is_latest=True, | |
| created_by=user, | |
| ) | |
| if current_latest is None: | |
| if base_version is not None and base_version != 0: | |
| raise FolderInstructionsVersionConflictError(current_version=0) | |
| try: | |
| return FileSystemFolderInstructions.objects.create( | |
| team=folder.team, | |
| folder=folder, | |
| content=content, | |
| version=1, | |
| is_latest=True, | |
| created_by=user, | |
| ) | |
| except IntegrityError: | |
| # Concurrent create won; retry to find the latest version | |
| current_latest = ( | |
| FileSystemFolderInstructions.objects.select_for_update() | |
| .filter(folder=folder, deleted=False, is_latest=True) | |
| .order_by("-version", "-created_at", "-id") | |
| .first() | |
| ) | |
| # Continue to the version increment logic below | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
video description
https://www.loom.com/share/2821ed9df9bf4ca4ae34c518eb6cab7f
Problem
The desktop filesystem surface has folders but no way to describe what a folder contains. We want to attach a markdown "instructions" blob to each individual desktop folder — conceptually like the markdown body that backs a shared skill in the skills store — so the contents of a folder can be documented and that documentation can evolve over time with history preserved.
Changes
Adds per-folder, versioned markdown instructions for the desktop surface only.
FileSystemFolderInstructions(posthog/models/file_system/folder_instructions.py) — a separate, versioned table rather than a column onFileSystem. Rationale: versioning can't live on a single column; a separate table keeps the hotFileSystemtable lean, and keying to the folder's stable row FK survives renames/moves. UsesTeamScopedRootMixin(fail-closed team scoping) withcontent/version/is_latest/deletedand two partial unique constraints (one latest + unique version per folder), mirroring theLLMSkillshape.folder_instructions_service.py) — publish / get-latest / list-versions / soft-delete, with abase_versionoptimistic-concurrency check and a 100 KB content size cap.instructions(GET/PUT/PATCH/DELETE) andinstructions/versions(GET) detail actions added toDesktopFileSystemViewSetonly, so the web sidebar tree is unaffected. Reuses the existing project + access-control scoping viaget_object().help_texton every field and validate content size.No frontend changes — the desktop surface is consumed by a separate client that will wire up the editor against these endpoints.
How did you test this code?
I'm an agent. I added automated API tests in
posthog/api/file_system/test/test_folder_instructions.pycovering: publish + version increment, latest GET, version history, size-cap rejection (400), folder-type guard (400), cross-team IDOR (404), web-surface isolation (404), soft delete + republish, and concurrency conflict (409).I could not run the Python test suite or
makemigrations --check/build:openapiin my environment (no Django virtualenv available). Before merge, please run:hogli test posthog/api/file_system/test/test_folder_instructions.pypython manage.py makemigrations --check(verify the hand-written migration1208_filesystemfolderinstructionsmatches model state)hogli build:openapi(the new serializers/endpoints add to the OpenAPI spec, so generated TS/Zod types + MCP schemas must be regenerated and committed)I verified locally that all new/changed files pass
ruff check,ruff format --check, andpy_compile.🤖 Agent context
Authored with Claude Code (PostHog Code). The user asked to research whether folder instructions should be a column on the file store or a separate model; after confirming the requirements (per-individual-folder, versioned history, desktop surface only) I chose a separate versioned model patterned on the existing skills store (
LLMSkill), since versioning rules out a single column and a separate table avoids widening the heavily-usedFileSystemtable while surviving folder rename/move via the row FK.Open question for the reviewer: confirm whether the desktop client UI lives in this repo (and thus whether follow-up UI work is in scope) or is a separate application consuming this API.