🖊️ fix: Persist Skill Subfile Edits - #16362
lia-by-librechat[bot] wants to merge 4 commits into
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. |
This branch has not been deployed
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 behavior lives in
packages/apiwith injected storage and database methods; the legacy API route 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.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 and packages/apinpm 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. The full test suites, schema-only SkillFile methods, 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.