Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8497 +/- ##
==========================================
+ Coverage 76.88% 77.07% +0.18%
==========================================
Files 460 466 +6
Lines 24778 24975 +197
Branches 6609 6650 +41
==========================================
+ Hits 19051 19249 +198
+ Misses 5727 5726 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
andypalmi
left a comment
There was a problem hiding this comment.
All four tools line up with the routes and controllers, and the tricky bits are handled well: the partial update with the empty-name guard, the settings.env normalization that dodges the unguarded Object.keys in uploadSnapshot, and making the device target snapshot required so the route (which never replies when it is missing) can't hang. Tests cover them nicely.
One optional cleanup, same theme as the pipeline stage tools: platform_export_snapshot and platform_import_snapshot carry near-identical components schemas. The differences are just the direction wording and the "exposes hidden values" caution, and that caution already lives in the export tool's description, so repeating it in the arg is the kind of duplication worth dropping. Could this be a single shared schema in schemas.js (next to snapshotId/hostedInstanceId), spread into both?
// schemas.js
const snapshotComponents = z.object({
flows: z.boolean().optional().describe('Include flows (default true). Excluding flows also excludes credentials'),
credentials: z.boolean().optional().describe('Include the flow credentials (default true)'),
envVars: z.union([z.literal('all'), z.literal('keys'), z.literal(false)]).optional().describe('Environment variables: "all" keeps keys and values (default), "keys" keeps only the names, false removes them')
}).optional()And a smaller one: a couple of descriptions restate mechanics that already live in the args, for example the non-empty name and empty-string-to-clear notes on platform_update_snapshot. Could those stay only in the arg .describe() that owns them, keeping the description tool-level? The owner-resolution, partial-update, and immediate-deploy caution are genuinely tool-level and read well where they are.
None of this is blocking, happy for it to be a follow-up if you would rather keep this PR focused.
…s, enforce upfront validation for encrypted data, and handle excluded components properly. Add tests for various encrypted scenarios.
|
Pushed a follow-up to the import tool after a closer read of the controller. Hidden env vars are also exported encrypted (the env entry gets a
Since both live in the route/controller, the tool handles them client-side for now:
Tests cover the strip, both rejection paths, and the two cases that should still go through. Guarding the decrypt loop in |
Mark platform_set_instance_device_target destructive: it overwrites the target on every device assigned to the instance, so it belongs behind destructive tool access rather than plain write. Guard platform_update_snapshot: tool input is not validated platform-side, so a blank name reached the controller and surfaced as a 500, and an update with no fields got a 200 with the snapshot unchanged. Reduce env vars to their names up front on a keys-only import. The route discards the values anyway, but decrypts the hidden ones first, which forced a credentialSecret the caller does not need. Share one components schema between export and import, use the toolError helper for the tool's own 400, and keep arg-level mechanics in the arg descriptions rather than repeating them in the tool description.
Closes #7688
Adds four snapshot write tools:
platform_update_snapshotwrapsPUT /api/v1/snapshots/:id. The controller does proper partial updates, so the tool only sends the fields it was given. A blank name is not cleanly rejected: the controller throws a sequelizeValidationErrorand nothing maps that to a status code, so the route answers 500. The tool catches it first (whitespace only counts as blank, since the controller trims) and returns a 400. An update with no fields at all gets a 200 back with the snapshot unchanged, which reads as a successful edit that never happened, so the tool rejects that too.platform_export_snapshotwrapsPOST /api/v1/snapshots/:id/export.credentialSecretis unconditionally required by the route (400 without it), and the description warns that the default export includes hidden env var values, plus a reminder that the same secret is needed at import time.platform_import_snapshotwrapsPOST /api/v1/snapshots/import. One real quirk surfaced while reading the controller:uploadSnapshotcallsObject.keys(snapshot.settings.env)unguarded, so a snapshot withoutsettings.env500s. The handler normalises an omitted env to{}so agents don't hit that. Same story for hidden env values, which get decrypted before the component filtering runs, so a keys-only or env-excluded import would 500 without a secret it does not actually need; the tool reduces env up front in both cases and the result is identical to what the route produces on its happy path. One rough edge left: the export response carries six extra fields (id,createdAt,updatedAt,ownerType,user,exportedBy) that this tool'ssnapshotargument does not accept, so pasting an export straight back in fails validation withunrecognized key. The description spells out which four fields to copy, but it might be worth letting the schema ignore the extras so the obvious export then import flow just works.platform_set_instance_device_targetwrapsPOST /api/v1/projects/:id/devices/settings. The description carries a caution that setting the target deploys immediately to every assigned device, and notes the route can only set a target, not clear one. The tool makessnapshotIdrequired because the route's onlyreply.sendsits inside theif (request.body.targetSnapshot)block: omit it and you get a 200 with an empty body and nothing changed, a silent no-op rather than an error. It is also annotateddestructiveHint: true, since it overwrites what every assigned device is running rather than adding to it, which puts it behind destructive tool access instead of plain write.Descriptions were written from the actual route/controller behavior rather than assumptions, including the owner resolution (instance or device) happening from the snapshot itself. The behaviors above were checked against a running platform, calling each route directly and invoking the matching tool with the same arguments.