mirror of
https://github.com/HKUDS/CLI-Anything.git
synced 2026-09-18 23:42:43 +08:00
richardhxwang
d71d4a240b
* fix(n8n): send only the fields the workflow API accepts
n8n's workflow schema sets additionalProperties:false and marks active, tags,
meta, isArchived, triggerCount and the id/timestamp fields readOnly. The payload
reduction in _clean_for_api was a blacklist of five fields, which is incomplete
against that schema: on n8n 2.16.1 a GET returns 20 keys, five are removed, and
eight of the remaining fifteen are rejected. Every command that builds a payload
from a fetched workflow therefore fails with
400 - request/body must NOT have additional properties
Separate the two reductions the helper was serving. Writes now keep only the
schema's non-readOnly properties; backup-all and diff keep the previous
behaviour under _strip_server_fields, because a backup has to record active and
tags and a diff has to compare them - exactly what the API refuses. versions
diff had a fifth inline copy of the blacklist, now routed through the same
helper.
nodeGroups and parentFolderId are in the writable set although they only exist
from n8n 2.33.3, so an edit made against a current instance does not silently
delete a workflow's node groups.
Two further layers are handled:
- Nulls are dropped. description is a plain string in the schema, so a workflow
without one reads back as null and sending it returns
"request/body/description must be string". Omitting a field leaves the stored
value untouched, so dropping is the safe direction.
- The nested settings object is reduced too. It is additionalProperties:false as
well and n8n's own editor writes binaryMode into it, so every workflow created
in the UI was rejected even with a clean top level.
Adds tests/test_api_payload_fields.py, which declares the accepted set itself
rather than importing it, so it asserts the API contract instead of tracking
what the module currently sends. Five of its cases fail on main.
* fix(n8n): route import, restore-all and template deploy through the same payload helper
These three built their request bodies with their own inline copy of the field
blacklist and then set data["active"] = False. active is readOnly on the
workflow endpoint, so all three failed unconditionally with 400, independently
of the field-count problem:
workflow import <file> 400
workflow backup-all -> workflow restore-all 400, printed as
"OK: Restored 0 workflows (1 failed)"
template deploy <id> 400
Route them through _clean_for_api instead. Workflows still land inactive, since
active cannot be set through this endpoint at all.
template deploy additionally needs settings defaulted: n8n.io templates
frequently omit it while the API requires it, which surfaced as
"400 - request/body must have required property 'settings'".
* fix(n8n): keep nulls for nullable properties, and always supply settings
Addresses the two review comments on this PR.
Dropping every null was wrong for versions rollback. staticData, pinData and
parentFolderId are nullable in the schema, and for them null is meaningful: it
clears the stored value, while omitting the key leaves it alone. So a snapshot
taken before any pinned data existed could not clear pinned data added later,
and the command reported a successful rollback regardless. The non-nullable
properties still have their nulls dropped, since echoing back the null a GET
returned is what draws "request/body/description must be string".
Verified against a live instance: a PUT with pinData: null empties it, and
rollback from a snapshot that predates the pinned data now restores the empty
state.
workflow import and workflow restore-all also did not guarantee the required
settings object, so a handwritten or older export lacking it still failed with
"must have required property 'settings'". Rather than repeat the default at
each call site, _clean_for_api now always supplies it, and the copy in
template_deploy is dropped.
* fix(n8n): restore tag assignments after recreating or rewinding a workflow
tags is readOnly on the workflow body, so reducing the payload correctly means
import, restore-all and versions rollback all leave the tags behind - and do it
silently, because the create or update itself succeeds. A restored workflow was
counted as restored while dropping out of every tag-filtered search.
Put them back through update_workflow_tags, which the harness already wraps.
Passing None (an older export with no tags key) leaves the current assignment
alone; an empty list clears it, so rewinding to a snapshot taken before any tag
existed removes them, matching the null-clears semantics used for pinData.
Tag ids are per-instance, so restoring into a different n8n answers 404 Some
tags not found. That is reported as a warning instead of raised: the workflow is
already created at that point, and failing the whole restore over its tags would
lose more than it protects.
Also completes the payload tests per review: parentFolderId is now in the
fixture and asserted both as writable and as null-preserving, and `shared` is
pinned explicitly - it is the one field that does not draw a 400 (the API
answers 200 and ignores it), so only an assertion catches it being forwarded.
* fix(n8n): keep export local-shaped, and reduce every write at the call to the API
Reducing `export` to the writable set was a regression of my own making: the
file it writes lost its tags, so `export` -> `import` restored a workflow
without them, while `backup-all` -> `restore-all` kept them. Two paths meant to
do the same thing behaved differently.
Split it along one rule instead. Local artefacts keep the full picture, and
every body that goes to the API is reduced at the call:
local form (_strip_server_fields): export, backup-all, diff, versions diff
wire form (_clean_for_api): create, update, import, restore-all,
template deploy, autofix, patch, rollback
`workflow create` and `workflow update` now reduce their input too, which is
what makes a file straight out of `export` usable in both. Hand-written JSON is
unaffected in practice: the fields this removes are exactly the ones that would
have produced a 400. `create` also reattaches tags from the file, the same way
`import` does.
Verified against a live instance: an exported workflow keeps its tags, imports
with tags and pinData intact, and the same file can be fed back through
`workflow update` without a 400.
* fix(n8n): degrade instead of failing on cross-version writes and unusable tags
Three failure modes where a write that had already partly succeeded was reported
as a plain failure, or silently did the wrong thing.
Cross-version imports: `nodeGroups` and `parentFolderId` exist only from n8n
2.33.3, so a file exported from a newer instance was rejected wholesale by an
older one. The target version is not cheap to query, so the write now reacts to
the rejection - retry once without those properties and say so. Losing a canvas
grouping beats losing the import. Every workflow create/update goes through the
one wrapper, `scaffold --deploy` included.
Tags with no id: filtering ids out of a non-empty list yields an empty list, and
sending that cleared every tag on the workflow - the opposite of what an
unreadable record should do. It now warns and leaves the assignment alone. An
explicitly empty list still clears, since that is a real state.
Tag request failures: only HTTPError was caught, so a timeout or dropped
connection escaped and made the caller count an already-created workflow as
failed - and a retry of `restore-all` would then duplicate it. Catches
RequestException instead.
Verified against a live n8n 2.16.1, which is old enough to reject both newer
properties: a file carrying them imports with
"WARN: Target n8n rejected nodeGroups, parentFolderId - retrying without", and a
file whose tags carry no ids imports with the assignment left untouched.
* fix(n8n): state what the retry knows, not what it assumes
The fallback warning claimed the target had rejected those specific properties,
but the retry fires on any 400 that carries them - it is a guess, not a
diagnosis. If the 400 came from something else the retry fails and the real
error surfaces, so the message should not have asserted a cause.
before: Target n8n rejected nodeGroups, parentFolderId - retrying without
after: Write rejected with 400 - retrying without nodeGroups, parentFolderId
* fix(n8n): carry the cross-version fallback into settings, and keep diagnostics off stdout
The nested settings object gained five properties in n8n 2.33.3 - binaryMode,
credentialResolverId, customTelemetryTags, redactionPolicy, timeSavedMode - and
it is additionalProperties:false in its own right. Two consequences, both fixed
here:
Writing to a current instance was resetting them, because the whitelist was
taken from the 2.16.1 schema. binaryMode matters most: the editor writes it into
every workflow it creates, so every edit was quietly changing it.
The cross-version retry only stripped top-level properties, so a payload whose
only incompatibility was in settings got no retry, and one that had both failed
again on the second request. It now reduces settings the same way and names what
it dropped as `settings.<key>`.
The trade is one failed request against silently changing a setting: on an older
instance a workflow carrying binaryMode now costs a 400 and a retry rather than
being stripped up front.
Diagnostics from these fallbacks go to stderr. warn() prints to stdout, which
would corrupt the documented --json output exactly when a fallback fires;
repl_skin.py is meant to stay a verbatim copy of the plugin's, so this goes
around it instead of changing it.
Verified against a live n8n 2.16.1: a file carrying nodeGroups and
settings.binaryMode imports with "Write rejected with 400 - retrying without
nodeGroups, settings.binaryMode" on stderr.
* fix(n8n): retry only on unknown-property errors, not on any 400
The compatibility retry fired on any 400 whose payload happened to carry a
newer-schema field. A rejected *value* - a malformed nodeGroups object, an
unsupported settings value - produces a 400 too, and the retry would then drop
the property and succeed, reporting success while silently discarding
configuration the caller asked for. That is the failure mode this PR exists to
remove, reintroduced in the fallback itself.
n8n's validator separates the two cases, so the retry now requires the first:
unknown key request/body must NOT have additional properties
request/body/settings must NOT have additional properties
bad value request/body/description must be string
request/body/settings/executionTimeout must be number
request/body/settings/callerPolicy must be equal to one of ...
An unreadable error body counts as no evidence, and does not retry either.
Verified against a live n8n 2.16.1: a payload with an unknown property still
degrades and imports; one that also carries an invalid settings value surfaces
`request/body/settings/callerPolicy must be equal to one of the allowed values`
and creates nothing. A payload whose only problem is a bad value never reaches
the retry at all, covered by unit tests since it needs a newer instance to
reproduce end to end.
* fix(n8n): match the rejected path, not just the wording
The previous predicate accepted any message ending in "must NOT have additional
properties", but the validator uses that same wording for an unexpected member
nested anywhere:
retryable request/body must NOT have additional properties
request/body/settings must NOT have additional properties
malformed request/body/nodes/0 must NOT have additional properties
request/body/nodeGroups/0 must NOT have additional properties
The last one is the dangerous case: a grouping whose item carries an unexpected
member would have been read as a version mismatch, and dropping the entire
grouping could then succeed - turning invalid input into a success that quietly
lost the grouping. Only the two paths the retry actually reduces now qualify.
Verified against a live n8n 2.16.1 that the deep form is real - a node with an
extra member reports request/body/nodes/0 - and that a payload combining it with
an unknown top-level property now surfaces that error and creates nothing, while
a plain version mismatch still degrades and imports. The nodeGroups/0 case needs
an instance that defines nodeGroups, so it is covered by unit tests.
* fix(n8n): treat malformed tags and settings as input, not as something to swallow
Two shape assumptions, both of which turned bad input into the wrong outcome.
A scalar `tags` value raised TypeError on the comprehension, which happens after
the workflow has been created - so the caller reported a failure for something
that exists, and a retry of restore-all would duplicate it. That is the partial
success this helper already guarded against for request failures, just reached
by a different route. A non-list is now reported as an unreadable record and the
assignment is left alone.
A non-object `settings` was silently replaced with `{}`. The value is invalid,
but that is the server's verdict to give by name, not something to quietly
substitute - the caller would otherwise see a success with their input gone. It
is now passed through; only a missing one is defaulted.
Verified against a live instance: importing a workflow with "tags": 1 creates it
and reports `Tags ... are not a list — left unchanged`, and one with
"settings": "nope" fails with `request/body/settings must be object` and creates
nothing.
---------
Co-authored-by: richardhxwang <225731730+richardhxwang@users.noreply.github.com>