🖊️ fix: Persist Skill Subfile Edits - #16362
Conversation
|
Ready for review at head 668c582. This commit makes nested skill text files editable in the live, permission-aware file viewer through the existing multipart replacement route. It includes Mongo-backed read/replace/read coverage, UI save/retry/permission tests, a passing frontend typecheck and staged static checks. Lighthouse's production build passed, but the local Playwright server could not start MongoDB in this sandbox; the browser audit did not run. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 668c58268c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const formData = new FormData(); | ||
| formData.append('relativePath', relativePath); | ||
| formData.append('file', new File([content], file.filename, { type: file.mimeType })); | ||
| upload.mutate({ skillId, formData }); |
There was a problem hiding this comment.
When two editors—or an editor and an agent—modify the same subfile, this mutation sends only the path and file bytes, so the upload endpoint unconditionally upserts the row even if the skill version changed after the editor captured file.content. Saving an older draft therefore silently destroys the newer revision; include a version/file revision precondition and surface a conflict instead of overwriting stored data.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in d7f7120. The file read now returns its storage revision; the editor submits that captured revision, and MongoDB matches it in the atomic replacement query with upsert disabled. A competing editor or agent replacement invalidates the draft; a deleted file cannot be recreated by a stale save. HTTP 409 keeps the draft and prevents blind retries. Conditional saves use a new POST file-path route so an old server rejects the request rather than ignoring the precondition.
Coverage: real MongoDB tests for two concurrent writers, an intervening unconditional authoring write, and deletion; HTTP tests for a race after blob storage and loser-only cleanup; UI conflict/draft preservation. The invariant review also scoped delayed API and agent read-cache writes to their captured file revision and covered forwarding through the deployment adapter. Legacy uploads without a precondition retain their existing unconditional contract.
| queryClient.invalidateQueries([ | ||
| QueryKeys.skillFileContent, | ||
| variables.skillId, | ||
| skillFile.relativePath, | ||
| ]); |
There was a problem hiding this comment.
After a successful upload, the editor first installs the confirmed content in the cache, but this unconditional invalidation immediately refetches it. In React Query v4, a transient failure of that background GET sets isError even while retaining cached data, and SkillFileViewer then hides that successfully saved content behind the load-error state; keep cached data visible for refetch errors or avoid forcing this reread.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in d7f7120. The viewer renders retained file data even if a background verification GET fails; the load-error-only view is now restricted to a query with no data. Confirmed save content and its new revision remain visible.
Coverage: SkillFileViewer regression exercises the real query/mutation hooks with a successful upload followed by a failed verification GET, waits for React Query's error state, and verifies the saved text remains visible.
| const isSkillMd = relativePath === 'SKILL.md'; | ||
| const isText = data != null && !data.isBinary && data.content != null; | ||
| const isEditing = editingFile != null; | ||
| const canEdit = skill != null && !permissions.isLoading && permissions.canEdit; |
There was a problem hiding this comment.
For a skill whose canonical source is github, an admin or ACL editor still satisfies this canEdit check and is offered the local file editor. The multipart replacement writes only LibreChat storage and cannot update the upstream repository, while GitHub synchronization compares the file's upstream blob metadata and will replace this local revision on a later upstream change, so a successful-looking edit is eventually lost; exclude externally managed skills from this action or provide an upstream-aware edit flow.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in d7f7120. The edit affordance now requires an inline skill in addition to edit permission. The typed upload handler also rejects GitHub- and Notion-managed files before storage, so the protection is not just a UI check. Deployment files retain their existing read-only ACL gate.
Coverage: parameterized UI and real-route tests for github/notion sources with an authorized user; route tests assert no blob write or file row is created.
| const queryClient = useQueryClient(); | ||
| const [content, setContent] = useState(file.content); | ||
|
|
||
| const upload = useUploadSkillFileMutation({ |
There was a problem hiding this comment.
When this editor is reached from a direct file URL, or the initial file-list request failed, [skillFiles, skillId] can be absent when the upload succeeds. useUploadSkillFileMutation then seeds that cache with only the edited file and never invalidates it; because useListSkillFilesQuery disables mount, focus, and reconnect refetches, the sidebar can subsequently treat that singleton as the complete list and hide every other subfile until the cache is cleared. Invalidate/fetch the file list when no prior list exists rather than manufacturing a partial response. CLAUDE.mdL277-L281
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in d7f7120. Successful uploads no longer manufacture a singleton list when no file-list cache exists. They cancel stale list reads, update an existing complete cache when available, and invalidate the list for a fresh complete response.
Coverage: mutation-hook regressions for a direct-file save followed by the first sidebar mount, and an in-flight stale list response settling after replacement. Both retain the edited file and its sibling.
|
Ready for another review at exact head All four findings from Local verification: 34 client tests, 59 real MongoDB route/tenant tests, 14 focused SkillFile database tests, and 333 related agent/deployment/sync tests passed. A maintainer needs to trigger the next Codex review; this handoff does not trigger it. GitHub has not registered CI checks or workflow runs for this SHA yet; checks on the previous head do not verify this commit. |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7f712087b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )} | ||
|
|
||
| {data.isBinary && !isImage && ( | ||
| {isError && !data && ( |
There was a problem hiding this comment.
When a save conflicts because another actor deleted the file, the conflict handler invalidates this query and the resulting GET returns 404, but React Query retains the old data. This !data guard then suppresses the error state after the user cancels, leaving the deleted content and Edit action visible; reopening only produces another conflict. Distinguish a not-found refetch by evicting the cached file or navigating away, while continuing to retain data for transient verification failures.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid and reproduced against d7f7120 before the fix. Fixed in fe145ce.
The content query replaces cached bytes with a null unavailable state for HTTP 404/410/403; transient network/5xx failures still retain confirmed content. The editor keeps its local draft until cancellation, then the unavailable state has neither stale text nor an Edit action, including when reopening with the same QueryClient. A later successful invalidation restores the file normally.
The final cache-writer sweep also cancels older in-flight content reads before publishing a successful save and publishes the captured content/revision even if a previous reread had cleared the cache. Regression coverage exercises deletion/conflict/cancel/reopen, access denial, restored files, transient verification errors, and old terminal reads settling after a save.
| ) { | ||
| return res; | ||
| } | ||
| const existingFile = await deps.getSkillFileByPath(skillId, relativePath); |
There was a problem hiding this comment.
For a user with the resource-wide skill capability, canAccessResource bypasses its ID resolver and leaves resourceAccess.resourceInfo unset, so this handler first awaits getSkillById and only afterward starts this independent getSkillFileByPath Mongo read. That adds a full database round trip to every privileged file save; start both scoped reads together and still gate storage writes on the skill/source result.
AGENTS.md reference: AGENTS.md:L51-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid and reproduced against d7f7120 before the fix. Fixed in fe145ce.
The handler now starts getSkillById and getSkillFileByPath in one Promise.all, reusing resourceAccess.resourceInfo when ACL middleware already resolved the skill. The lookups keep their existing skill/path/tenant scope. Storage resolution and writes remain after the source, content-policy, and revision checks.
New upload.spec.ts regressions assert that both scoped reads are called before either deferred promise settles, cover either completion order, prove ACL-loaded skill data is reused, and assert zero storage writes for missing skills, managed sources, stale revisions, and lookup failure. The real MongoDB route/tenant suites also pass. This proves removal of the serialized lookup dependency, not a measured Lighthouse latency improvement.
|
Final-pass handoff for exact head Both new P2 findings were valid and reproduced before the fix. The terminal-response cache fix covers deleted or unavailable files, draft preservation, cancel/reopen, and later recovery while retaining confirmed content for transient failures. The upload fix overlaps the independent privileged lookups and reuses ACL-loaded skill data without moving writes ahead of validation. Each inline finding has the fix commit and regression coverage. The subsystem pass also checked query/cache writers, delayed read completion, revision checks, storage cleanup, authorization/tenant scope, externally managed files, and mixed-version behavior. Older in-flight reads cannot clobber a confirmed save, and confirmed save content can repopulate a cache cleared during the request. The existing unconditional legacy/agent write contract is unchanged. Local results: 41 client tests, 340 related API tests (including seven upload-ordering cases), 59 real MongoDB route/tenant tests, and 14 focused SkillFile database tests passed. CI checks are registered for this exact head; results are still being collected. A maintainer must trigger Codex for this new head; this comment is a handoff, not a trigger. |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe145cee8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (status === 404 || status === 410 || status === 403) { | ||
| return null; |
There was a problem hiding this comment.
When a file read temporarily returns 403, or a deleted path is later recreated, returning null records a successful query result; with staleTime: Infinity and mount/focus/reconnect refetching disabled below, revisiting the file never issues another GET, so the error view persists until a full reload or an unrelated explicit invalidation. The fresh issue introduced by the prior cache-eviction fix is this permanent success state; keep terminal responses retryable or explicitly invalidate them when access/files can be restored.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in c21600e. Returning null still clears stale bytes on 403/404/410; a null cache entry now forces a GET on the next mount despite infinite staleTime. A Retry button also recovers without leaving the file, and confirmed content remains cached. Coverage: real query/viewer regressions for unavailable revisits, repeated 403 retries, conflict/deletion/cancel/reopen/recreation, and transient failures. All 46 related client tests passed.
| // A parent-version update can fail after the file row committed. Never | ||
| // delete its live blob on that ambiguous failure; retain it for reread. | ||
| const persisted = await deps.getSkillFileByPath(skillId, relativePath); | ||
| if (persisted?.file_id !== fileId) { |
There was a problem hiding this comment.
When replacing an existing file, if the SkillFile update commits but the subsequent parent-version update fails, this reread confirms that the new fileId is live and deliberately retains its blob, but rethrowing then skips the normal old-blob cleanup at lines 148–158. The previous storage object is no longer referenced and remains permanently orphaned; after confirming that the replacement committed, also delete existingFile while retaining the new blob.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in c21600e. When a reread confirms the new file revision committed despite a parent-version update failure, the handler retains its live blob and invokes the same best-effort cleanup for the superseded blob as on successful replacements. Losing uploads still delete only their own new blob. Coverage: real MongoDB route failure injection after metadata replacement verifies old blob and original owner are targeted, the committed blob remains, and the response stays 500. All 61 route/tenant tests passed.
| if (skill.source !== 'inline') { | ||
| return res.status(403).json({ error: 'Externally managed skill files are read-only' }); |
There was a problem hiding this comment.
For a stored skill created before the source field was populated, getSkillById returns a lean Mongo row with source absent—the schema default does not backfill existing documents—and this strict comparison classifies it as externally managed and returns 403. The previous upload route accepted these skills, so existing inline skills can no longer add or replace files; treat an absent source as inline or backfill it before enforcing the managed-source restriction.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in c21600e. A nullish stored source is now interpreted as inline in the upload check and normalized to inline in both full and list API responses, so existing inline skills remain editable. Explicit GitHub/Notion/deployment sources remain blocked. Coverage: real MongoDB route test unsets the source on the raw stored document, verifies GET detail/list, legacy upload and revision-checked replacement; existing managed-source tests assert rejection before storage. All 61 route/tenant tests passed.
|
The three new findings have been assessed and fixed at c21600e. Recovery on revisit and Retry, old-blob cleanup after a committed replacement, and legacy inline-source compatibility now have regression coverage. Focused client, API and real-MongoDB tests, affected TypeScript typechecks, scoped static checks and production builds pass. Local Lighthouse browser audit was blocked by missing Chrome; CI runs independently. Please review this exact PR head when a maintainer can trigger review. |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21600e9b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| skillId: row.skillId, | ||
| relativePath: row.relativePath, | ||
| ...(row.expectedFileId != null ? { file_id: row.expectedFileId } : {}), |
There was a problem hiding this comment.
When an agent edit_file overlaps a browser edit, the agent path in api/server/services/Endpoints/agents/skillDeps.js still calls upsertSkillFile without expectedFileId after only a separate parent-version check in writeBundledSkillFile; if the browser save lands between that check and the agent write, this optional branch performs an unconditional upsert and silently replaces the newer browser revision. Fresh evidence at this head is that the agent save call still omits the newly introduced revision field, so pass the captured file revision through that path and use the conditional replacement for existing files.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid and reproduced at a775439: after the agent read an old fileId, a browser save completed and the old agent saver silently overwrote it in the real MongoDB route regression. Fixed in a775439. Both edit_file and create_file overwrite now carry the file revision captured with cached, streamed, or large-file metadata to an injected typed saver; Mongo atomically matches file_id and refuses stale/deleted replacements. New subfiles use insert-only writes and a competing creator receives an actionable conflict. A losing agent upload cleans only its own blob; an ambiguous committed write keeps its live blob. Managed sources remain read-only. Coverage: real MongoDB browser/agent preflight and post-storage overlaps, 15 focused SkillFile method tests including concurrent creators, 355 related API tests including agent/tool and saver failure paths, and 77 initialization/wiring tests. The repository static checks and affected workspace typechecks pass.
There was a problem hiding this comment.
CI follow-up at 5f69712. The earlier head failed the real-MongoDB OpenAPI skill-file HTTP contract: its content-only machine-management PUT had been wired to the agent tool saver, which correctly rejects a missing captured revision. The fix gives management PUT a separate saver that reads the current revision once, then uses insert-only creation or atomic conditional replacement; overlapping writes produce the documented 409. Agent edit_file and create_file still require their captured revision and fail closed. The formerly failing OpenAPI suite now passes with MongoDB and disk readback. Coverage: 149 related real-MongoDB API tests, 392 related TypeScript API tests, API typecheck, production builds, and staged/PR-diff static checks.
danny-avila
added
the
🗺️ Custom Skills
|
PR #16362 new exact head: a775439. The reviewed P1 agent/browser subfile race is fixed with captured revisions, atomic replace/insert-only create, loser-only cleanup, and regression tests at the real MongoDB boundary. Related tests, schema/API typechecks, static checks and builds pass. Local Lighthouse browser audit could not run without Chrome; CI is independent. A maintainer can request an exact-head Codex review when available. |
|
PR #16362 exact pushed head: 5f69712. The sole failed CI check on the previous head was the machine-management OpenAPI file-write contract; this head fixes its content-only PUT without weakening the agent tool revision requirement. The failing OpenAPI case and 149 related real-MongoDB API tests, 392 TypeScript API tests, API typecheck, production builds, and scoped static checks pass locally. Local Lighthouse reached the browser launch but Chrome is unavailable here. CI will run at this new head. A maintainer can request Codex review of this exact SHA when available. |
2 participants
Summary
Skill sub-files appear in the sidebar but open in a read-only viewer. The unused tree editor does not reach that UI. This PR lets authorized users edit inline text sub-files in the live viewer and persist changes through the existing storage pipeline. Saves match the file revision atomically; conflicts preserve drafts instead of overwriting newer content.
SKILL.mdcontinues to use the skill-body form. Binary, oversized, and externally managed files remain read-only.Confirmed 404/410/403 reads clear cached file bytes without discarding an open draft. Unavailable files retry when revisited and have a Retry action while open. Transient network/5xx failures retain the last confirmed content. Skills stored before the
sourcefield existed are treated as inline by both the API and upload handler, so their files remain editable.Fixes #16316
How it works
The new conditional POST route requires a matching path and revision. The legacy collection upload route retains its unconditional contract for existing clients. In a mixed deployment, an older server rejects the conditional route instead of silently ignoring the revision. No
/treeroute is introduced.Upload and agent file-save behavior lives in
packages/apiwith injected storage and database methods; legacy JavaScript only wires it. Privileged skill and file reads start together, while validation gates storage writes. A 409 cannot recreate deleted files. Losing uploads clean up only their own blob. When a file replacement commits but the separate parent version update fails, the new live blob is retained, best-effort cleanup targets the superseded blob, and the request still reports the error. GitHub and Notion files are rejected before storage writes. Existing executable-file metadata is preserved.Agent
edit_fileandcreate_fileoverwrites now carry the file revision read with the content through the typed saver into Mongo’s atomic file-id match. Missing revisions fail closed; new-file creation uses the existing unique (skillId, path) index with an insert-only write. If a browser save wins during agent storage, the agent’s upload is cleaned up without deleting the winner. Ambiguous parent-version failures keep the committed blob and clean the superseded blob. Externally managed subfiles remain read-only for agent writes. Legacy client uploads and external sync/import keep their existing contracts.The existing machine-management PUT accepts complete content without a caller revision. Its separate saver reads the current file once after authorization, then uses insert-only creation or an atomic revision-checked replacement. A concurrent change returns the documented 409 response. Agent
edit_fileandcreate_fileremain fail-closed when their captured revision is missing.The editor keeps its draft through refreshes, save errors, and permission changes. Conflicts require the user to copy the draft and reopen the current file rather than blindly retry. Successful saves cancel older content reads and install their confirmed revision. List-cache updates avoid partial singleton responses and refresh the complete list. Delayed HTTP and agent content-cache writes match the file revision they read, including through the deployment adapter.
Type of change
Testing
npx tsc --noEmit: client, packages/api, packages/data-schemas, packages/data-providernpm run static-checks: staged change and PR diffnpm run lighthouseReal MongoDB tests used a temporary TCP-only
--nounixsocketpreload and the cached mongod binary. No harness files are committed. Full suites, config migration, unused-i18n-key, and unused-dependency checks were not run locally for this patch. CI and review status should be evaluated at the exact PR head.