fix(media): validate upload paths before generation#5615
Open
j15z wants to merge 44 commits into
Open
Conversation
…KB tool handlers Mirrors mothership dev f90f9b05: - regenerated tool-catalog/tool-schemas mirrors (search trigger replaces research + scout; QueryUserTable / SearchKnowledgeBase entries) - queryUserTableServerTool / searchKnowledgeBaseServerTool: read-only wrappers delegating to the full user_table / knowledge_base handlers with hard operation allowlists (and outputPath export rejection on query_user_table) - display maps: 'search' agent label/title/icon added; research + scout entries retained so historical transcripts keep rendering - Search.id replaces Research.id in LONG_RUNNING_TOOL_IDS (it inherits research's long crawls) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors mothership dev db60da94: run_code is the compute-only variant of function_execute for the search agent — same sandbox and inputs, no outputs.files / outputTable, so it cannot create or overwrite workspace resources. Wrapper handler hard-rejects the write vectors and delegates to executeFunctionExecute; run_code is deliberately absent from OUTPUT_PATH_TOOLS and the table output post-processor, so the name gating blocks writes even for leaked args. Added to LONG_RUNNING_TOOL_IDS, display title/icon maps, and the regenerated catalog/schema mirrors. Also removes two ineffective biome suppression comments in the docs workflow-preview (the rule doesn't fire in the docs app config). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…data
A failed handler result that carried a defined-but-empty output (the
app-tool executor's 'Tool not found' ships output: {}) won the priority
race in getToolCallTerminalData, so the resume payload's data — the only
thing the model reads — was a bare {} with the error text dropped. The
search agent retried run_code 20+ times blind against a stale server
because every failure rendered as empty instead of 'Tool not found'.
Failed calls now always carry error in their terminal data: merged into
object outputs, wrapped alongside non-object outputs, preserved when the
output already has an error field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…splay Companion to mothership 8ae32e97 (user_memory tool removed — the feature no longer exists). Regenerates the mothership contract mirrors via generate-mship-contracts.ts, which also picks up the pending telemetry contract additions (gen_ai.agent.name labels, llm.client.context_tokens, llm.client.compactions, llm.request.compaction_trigger, llm.compaction.pause, gen_ai.usage.context_tokens), and removes the user_memory display title. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…select types only
UI ordering: answering a question card no longer echoes a duplicate user
bubble. The combined answer still goes on the wire as a user message, but the
chat pairs it back to its card (strict 'Prompt — Answer' match, now uniform
for single questions too) and renders the card as the answered recap — the
card IS the user turn, and the next assistant message streams below it. The
pairing is derived from the transcript, so live and reloaded renders are
identical; a dismissed card followed by an unrelated typed message does not
match and renders normally. Messages ending with a question card also drop
the copy/thumbs actions row — the card is an input surface, not a reactable
assistant turn.
Question types are now single_select and multi_select only: text is removed
(the free-text 'Something else' row covers it) and confirm collapses into
single_select with Yes/No options. multi_select rows toggle with a check and
the free-text row's arrow submits the step; answers are comma-joined labels
plus any typed entry. Agent-supplied catch-all options ('Other', 'Something
else', 'None of the above') are stripped at parse — the card always provides
its own free-text row; a question left with no real options is invalid.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removes multi_select (and its toggle/check UI). The card is one shape: pick one option or type into the always-present 'Something else' row. Catch-all stripping and the transcript pairing/recap behavior are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-adds multi_select with a reworked interaction: option rows carry real checkboxes (emcn Checkbox chrome) instead of numbers and arrows, an option-styled Submit row confirms the step, and the "Something else" row reads as a plain option until clicked — then it becomes the focused text box, auto-checks, and can be unchecked without losing the typed text (blur with nothing typed reverts it). single_select behavior, catch-all stripping, and the transcript pairing/recap format are unchanged; multi_select answers are the checked labels comma-joined.
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
Stop the mothership from adopting a workspace user-skill on its own: - Remove the load_user_skill tool and its three payload callers (chat payload, mothership execute route, inbox executor); delete lib/mothership/skills.ts + its test. Skills no longer autoload as the agent's own instructions. - Rename the workspace "## Skills" inventory to "## Agent Block Skills — NOT FOR YOU" with a one-line guardrail so a skill's description (e.g. "respond like a pirate") is not treated as an instruction. Skills reach the model as behavior only via explicit /-attach. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…top it clearing them (#5546) * fix(copilot): persist KB tag subblocks as JSON strings from edit_workflow The edit_workflow tool normalizes array-with-id subblocks (via normalizeArrayWithIds) but only re-stringifies the keys listed in JSON_STRING_SUBBLOCK_KEYS. `tagFilters` (knowledge-tag-filters) and `documentTags` (document-tag-entry) were missing, so agent-authored tag filters were stored as raw JSON arrays while those UI components read their value with JSON.parse (expecting a string). The result: an agent edit to a Knowledge block's tag filter persisted correctly but rendered as an empty filter in the editor (JSON.parse on an array throws -> []). - Add `tagFilters` and `documentTags` to JSON_STRING_SUBBLOCK_KEYS so edit_workflow stores them in the same shape the UI writes. - Make both components' parsers tolerate an already-parsed array on read, self-healing values already persisted in the broken (array) shape. Search execution was unaffected (parseTagFilters accepts arrays), so the value was never lost — only the editor render and round-trip were broken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): expose KB tag definitions in VFS meta.json Surface each knowledge base's defined tags (displayName -> tagSlot) inline in its meta.json via serializeKBMeta, loaded in one batched query (loadKbTagDefinitions), so the agent can bind a knowledge-tag filter to a real tag slot instead of guessing a tag name it cannot otherwise see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stringify KB tag subblocks on the nested-node edit path The nested-node merge path normalized array-with-id subblocks but never re-serialized the JSON_STRING_SUBBLOCK_KEYS, so editing a block nested in a loop/parallel container still persisted tagFilters/documentTags (and conditions/routes) as raw arrays -- the exact shape the subblock components cannot JSON.parse. Route all four write paths through a single normalizeSubblockValue helper so the normalize and re-stringify steps cannot drift apart again, and extract the duplicated string-or-array read logic into parseJsonArrayValue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): tighten subblock serialization helpers Derive KbTagDefinitionSummary from the canonical TagDefinition instead of restating its fields, make parseJsonArrayValue generic so callers drop their `as T[]` casts, and unexport the three builders helpers that no longer have consumers outside the module now that normalizeSubblockValue fronts them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stop stripping tagFilters/documentTags from the agent's workflow view sanitizeForCopilot dropped `tagFilters` and `documentTags` from the workflow state the agent reads (workflows/{name}/state.json), while edit_workflow is allowed to write both. The field was therefore write-only: on a follow-up edit the agent read back an absent field, concluded no filter was set, and cleared the user's tag filter. The redaction was introduced for workflow *export* (#1628) and is already enforced there by sanitizeWorkflowForSharing's key list. The duplicate in the copilot-only sanitizeSubBlocks was redundant for export and destructive for the agent. Removes it and pins the contract with a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): reject malformed KB tag values instead of clearing the filter `knowledge-tag-filters` and `document-tag-entry` had no arm in the `edit_workflow` input validator, so they fell through to the pass-through default. Any non-array value the agent supplied -- a double-encoded JSON string, an object, an unparseable string -- reached `normalizeSubblockValue`, where `normalizeArrayWithIds` coerces unparseable input to `[]`. The write path then persisted `"[]"` over the tag filter the user had configured. `condition-input` and `router-input` already guard against exactly this and return an actionable error to the model. Extend that arm to cover the two KB subblock types. It keys on subblock type, so the unrelated `tagFilters` short-input on the Algolia block is unaffected. `null`/`undefined` and empty arrays still clear the field, so intentional clears keep working. Also wrap `loadKbTagDefinitions` in try/catch. Tag definitions are an optional meta.json enrichment, but the query ran inside the top-level `Promise.all`, so a transient failure would reject the entire workspace VFS materialize and leave the agent unable to read any file. Now it degrades to a meta.json without tag definitions, matching the sibling materializers. Adds regression tests for both, plus the first tests for `parseJsonArrayValue`, the helper that keeps pre-fix raw-array rows readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): collapse duplicate JSON-array parsing in edit-workflow builders `normalizeArrayWithIds` and `normalizeConditionRouterIds` each hand-rolled the same "accept a raw array or the JSON string these subblocks persist" parse. Extract `parseJsonArray`, which returns null when the value is neither, so each caller keeps its own distinct fallback: `[]` for the former, the untouched original value for the latter. Behavior-preserving. An empty array is truthy, so `[]` and `"[]"` still parse through rather than hitting either fallback. `validation.ts` has a third copy, but `builders.ts` already imports from it, so sharing the helper across the two would introduce an import cycle. Left as is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): specify tag name and legal operators in KB meta.json `tagDefinitions` exposed `displayName`, but a `tagFilters` entry must carry the key `tagName`. An entry written with `displayName` passes validation and persists, then filters nothing -- a silent failure. Rename the field at the serializer boundary; the DB column is untouched. Also emit the operators legal for each tag's `fieldType`, reusing `getOperatorsForFieldType`. `between` is valid for number and date but not for text or boolean, and the agent has no way to infer that. An unrecognized fieldType yields an empty list rather than throwing. Still unspecified, and deliberately out of scope: a filter entry's value key is `tagValue` (but `value` on documentTags), and `between` needs `valueTo`. Those describe the subblock entry shape, not the knowledge base, so meta.json is the wrong place for them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): pass a nullish subblock clear through instead of serializing "[]" `validateValueForSubBlockType` accepts null as an explicit clear, but `normalizeSubblockValue` then ran it through `normalizeArrayWithIds`, which coerces any non-array to `[]`, and persisted the string "[]". No data is lost either way -- "[]" and an absent field both mean "no filters". But it left the field present when the caller asked for it to be unset, so `sanitizeForCopilot` showed the agent an empty filter rather than an absent one, contradicting the absent-means-unset invariant the sanitizer documents. It also made Algolia's `if (params.tagFilters)` see a set value, since "[]" is truthy. An explicitly empty array still serializes to "[]" -- clearing with a value is distinct from clearing by omission. Reported by Cursor Bugbot on #5546. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit 7c321ca. Bugbot is set up for automated code reviews on this repo. Configure here. |
Contributor
Greptile SummaryThis PR tightens media tool path handling for generated outputs and uploaded inputs. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(media): preflight all writable outpu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Allow media tools to consume chat-scoped uploads directly while keeping outputs under files/. Fail malformed or unresolved declarations before provider work and regenerate the Mothership-derived tool schemas.
j15z
force-pushed
the
feat/media-upload-path-validation
branch
from
6bccb71 to
9c234ad
Compare
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7c321ca. Configure here.
Sg312
force-pushed
the
dev
branch
2 times, most recently
from
2eaf4ce to
e424e2d
Compare
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
3 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.
Summary
Media generation now accepts chat-scoped
uploads/...paths as inputs, keeps outputs restricted tofiles/..., and reports invalid or missing paths before starting provider work. Previously, an uploaded reference could be silently dropped and leave the user with a generic generated subject.The shared path boundary applies consistently to image, video, audio, and FFmpeg tools. Image generation also lets reference-loading failures reach the caller instead of catching and skipping them locally.
Related: https://github.com/simstudioai/mothership/pull/352
Type of Change
Testing
bun run scripts/sync-tool-catalog.ts --check --input=../mothership/copilot/contracts/tool-catalog-v1.jsonapps/sim/providers/meta/index.ts:122.Reviewers should focus on the input/output path boundary and the guarantee that validation occurs before paid Gemini, Fal, or FFmpeg work begins.
Checklist
Screenshots/Videos
Not applicable; this is server-side validation and generated tool-contract behavior.