Uh oh!
There was an error while loading. Please reload this page.
docs(skill): template update replaces the whole object, not just passed fields - #161
Merged
Conversation
…ed fields
POST /template/update binds all 16 channel fields plus description as plain
strings and writes every one of them unconditionally, so a channel absent from
the request is stored as "". The template card claimed the opposite — that
omitted channel flags are left unchanged — which turns a one-channel edit into a
silent wipe of every other channel on the template, for every escalation rule
bound to it. The published OpenAPI description already states the correct
behavior ("Replace the content of every channel"); the card contradicted it.
- state the destructive semantics and name the only inputs that really are
patch-semantics: team_id, feishu_app_card_v2_table_enabled,
incident_card_hidden_fields, status
- replace the update hot flow with snapshot -> edit -> preview -> write-all ->
verify-field-set; checking only the edited field cannot detect the damage,
which always lands on the fields the caller did not touch
- fix the flag name in the pointer-semantics gotcha: the CLI spells it
--feishu-app-card-v2-table-enabled
- warn that `list` returns every channel's full source per row (it has no
--fields projection) and give the file+jq form instead
Guard test bans the retracted claim from returning and pins the corrected text.Review of the previous commit found the safe-write flow it introduced carried the same class of defect it exists to prevent. Three corrections: Silent truncation. The flow moved channel bodies with `"$(cat …)"` / `"$(jq -r …)"`. Bash command substitution strips every trailing newline, so a body that legitimately ends in a blank line was written back shortened — and the verification step compared only which fields were non-empty, so a truncated-but-still-non-empty channel reported clean. Measured: a 15-byte body round-trips as 11. The write step now builds the whole request with `jq --rawfile` (byte-exact) and posts it via `--data -`, and step 5 diffs per-channel byte lengths instead of the non-empty field set, which catches truncation and wipes alike. Rebuilding the body from the snapshot also removes the previous "one flag per non-empty key" instruction, whose jq filter was unscoped and surfaced template_id/status/created_at/updated_at — keys `update` has no flags for. Miscount. `buildTemplateUpdates` writes 14 channel-content fields, not 16. Category error. `status` was listed among the inputs that survive omission. It is not a field of `update`'s request at all — it moves only through the separate enable/disable endpoints, which the CLI does not expose. The survivors are exactly the pointer-typed inputs: team_id, feishu_app_card_v2_table_enabled, incident_card_hidden_fields. Guard extended and mutation-verified.
/template/update now writes only the fields the request carries: an omitted channel keeps its stored body, and only an explicit empty string clears one. This card was written against the previous behaviour, where a one-channel edit overwrote all fourteen channels, so it taught a snapshot-rebuild-write-all flow and warned that omission destroys. Both are wrong now, and the flow is wrong in the expensive direction: rebuilding the whole object from a snapshot re-sends thirteen channels the caller never meant to touch. Rewrite the semantics banner, the hot flow, and the gotcha: - The hot flow sends one channel. Read the current body, preview it against a real incident, write that one field, then `cmp` the round-trip. The all-channel byte-length diff it used to end with was there to catch the collateral damage of a full-object write; there is no collateral damage now, and `cmp` catches truncation of the body that was actually written. - Clearing is now an explicit act, and it has a sharp edge worth its own bullet: a flag set to the empty string is dropped before it reaches the wire in builds older than v1.4.2, so `--dingtalk-app ''` is a silent no-op there. The card says to check `fduty --version` and to spell the field out in `--data` when it is older. The command-substitution hazard is unchanged and stays: `"$(...)"` strips every trailing newline off a template body regardless of how the server writes it. The guard test flips with the card and keeps banning the two claims that are still false either way (the 16-field miscount, `status` surviving omission). Its comment now records that this fact has been backwards in both directions, so the correction does not outlive the behaviour that motivated it. Prose only — nothing inside the generated fence changed; `make check-cards` still reports `skilldoc: cards OK`.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Why
The template card states:
That is backwards.
POST /template/updatebinds all 16 channel fields plusdescriptionas plain strings and writes every one of them unconditionally, so a channel absent from
the request is stored as
"". The CLI only sends flags that wereChanged, so the twotogether turn a one-channel edit into a silent wipe of every other channel on that
template, for every escalation rule bound to it. The blanked channel just stops
rendering — nothing errors, and the caller who checks only the field they edited sees a
clean result.
The published OpenAPI description is already correct ("Replace the content of every
channel on an existing template"); the card contradicted it.
What changed
updateis a full-object replace, omitted fields are CLEARED. Namesthe only inputs that really are patch-semantics (
team_id,feishu_app_card_v2_table_enabled,incident_card_hidden_fields,status).Step 5 diffs the non-empty-key set before and after, because the damage always lands on
the fields the caller did not touch.
--feishu-app-card-table-enabled; theflag is
--feishu-app-card-v2-table-enabled.listreturns every channel's full source per row and has no--fieldsprojection, sothe card now gives the file+
jqform.Verification
go build ./...clean,go test ./internal/skilldoc/green. New guard test bans theretracted claim from returning in either phrasing and pins the corrected text.