Skip to content

feat: add update command for non-interactive field updates#59

Open
ipreuss wants to merge 2 commits intowedow:masterfrom
ipreuss:feature/update-command
Open

feat: add update command for non-interactive field updates#59
ipreuss wants to merge 2 commits intowedow:masterfrom
ipreuss:feature/update-command

Conversation

@ipreuss
Copy link
Copy Markdown

@ipreuss ipreuss commented Apr 14, 2026

Summary

Adds tk update <id> --field=value for modifying YAML frontmatter fields without opening an editor. Useful for AI agents and automation scripts.

  • Update single or multiple fields: tk update abc --priority=1 --assignee=alice
  • JSON array syntax (requires jq): --tags='["bug", "urgent"]'tags: [bug, urgent]
  • Graceful fallback when jq unavailable
  • Proper error handling for invalid arguments/JSON

Also fixes update_yaml_field to work on macOS/BSD by using awk instead of GNU-specific sed syntax for inserting new fields.

Test plan

  • All existing tests pass (133 scenarios)
  • New test file with 9 scenarios covering:
    • Single field update
    • Multiple field update
    • New custom field insertion
    • JSON array parsing
    • Invalid JSON error
    • Unknown argument error
    • Missing arguments error
    • Non-existent ticket error
    • Partial ID resolution

🤖 Generated with Claude Code

Adds `tk update <id> --field=value` for modifying YAML frontmatter
fields without opening an editor. Useful for AI agents and scripts.

Features:
- Update single or multiple fields in one command
- JSON array syntax with jq: --tags='["a", "b"]' → tags: [a, b]
- Graceful fallback when jq unavailable
- Proper error handling for invalid arguments/JSON

Also fixes update_yaml_field to work on macOS/BSD by using awk
instead of GNU-specific sed syntax for inserting new fields.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@strefethen
Copy link
Copy Markdown

Code review feedback

Disclosure: review by Claude (Anthropic's coding agent) on behalf of
@stevetrefethen during evaluation of this PR for local adoption. Every
finding below was verified against this PR's branch (cc5cef9) on macOS
Darwin 24.5.0. To reproduce: git fetch origin pull/59/head:pr-59 && git worktree add /tmp/ticket-pr59 pr-59.

Context: the update_yaml_field GNU-sed → awk replacement is genuinely
load-bearing for this PR. The original 0,/^---$/ { /^---$/a\\ ... }
is a silent no-op on BSD sed — verified, sed exit=0 and file unchanged.
Master's three callers (status/deps/links) never trigger that
branch because those fields always exist post-cmd_create, so the bug
has been dormant — but cmd_update activates it the moment it lands.

Findings on cmd_update, ordered by severity.

1. set -e + ((updated++)) — every successful update returns exit 1

ticket line 2 is set -euo pipefail. ((updated++)) returns exit 1
when updated transitions 0→1 (bash arithmetic returns non-zero when
the expression value is 0). set -e then aborts cmd_update after
update_yaml_field has modified the file but before the success
message prints.

$ tk update tps-3zfs --assignee=alice ; echo $?
1
$ grep ^assignee tps-3zfs.md
assignee: alice          # update happened, but caller sees exit 1

Any agent or script branching on exit status will treat every update as
failure. Fix: : $((updated++)) or updated=$((updated+1)).

2. & in value → silent data corruption

update_yaml_field's sed branch interpolates value into
s/^${field}:.*/${field}: ${value}/. The & in a sed replacement is a
back-reference to the matched pattern.

$ grep ^priority tps-3zfs.md
priority: 2
$ tk update tps-3zfs '--priority=&X'
$ grep ^priority tps-3zfs.md
priority: priority: 2X

No error reported. Pre-existing in update_yaml_field but newly exposed
because cmd_update accepts arbitrary values.

3. / in value → sed error, file unchanged

$ tk update tps-3zfs --description='https://example.com'
sed: 1: "s/^...": bad flag in substitute command: '/'

Common in real descriptions (URLs, paths). Loud failure rather than
silent corruption, but blocks legitimate input.

Findings 2 and 3 are both addressed by switching the existing-field
branch of update_yaml_field to the same awk pattern this PR introduces
for the new-field branch — keeping value handling consistent and side-
stepping sed metacharacter rules entirely.

4. No field-name allowlist — structural fields can be violated

  • --id=xyz writes id: xyz inside the file but the filename is
    unchanged, so tk show xyz returns "not found"
  • --status=anything bypasses validate_status() that cmd_status
    enforces
  • --deps='["nonexistent"]' bypasses the existence check that tk dep
    performs (verified: tk dep abc nonexistent correctly errors)

5. JSON-array items containing commas split on round-trip

$ tk update tps-3zfs '--tags=["urgent, internal"]'
$ grep ^tags tps-3zfs.md
tags: [urgent, internal]
$ ticket-query | jq .tags
["urgent","internal"]    # was 1 tag, now 2

The jq filter map(tostring) | join(", ") loses item boundaries.

6. Test coverage gaps

The 9 new scenarios cover happy path and basic argv errors but don't
exercise: special characters in value, updating id/created/status,
jq-array items with embedded commas, or the set -e interaction. Adding
coverage for these would catch regressions of fixes 1–5.

- Exit 0 on success (updated=$((updated+1)) instead of ((updated++)))
- awk-based value replacement in both branches of update_yaml_field
  (eliminates sed metacharacter hazards: & / \)
- Hybrid field policy via validate_field_update helper:
  - immutable: id, created
  - routed: status, deps, links
  - body-backed (rejected): title, description, design, acceptance
  - validated: priority (0-4), parent (must exist), type (known values)
  - freeform: everything else with [A-Za-z][A-Za-z0-9_-]* name
- Reject array items with embedded commas (jq + no-jq paths)
- Add 24 regression scenarios

Ticket: M3R-7iue
@ipreuss
Copy link
Copy Markdown
Author

ipreuss commented Apr 21, 2026

Thanks for the thorough review — all 6 findings addressed in f4774ea. External architecture review flagged additional issues worth calling out (title/description/design/acceptance being body content, regex-injection via field names, no-jq fallback bypassing validation), all folded into the fix.

  1. Fixed ((updated++)) exit-1 via updated=$((updated+1)).
    2+3. update_yaml_field now uses awk for both branches — sed metacharacter hazards (&, /, \) gone. First-match-wins (intentional improvement over global-replace: duplicate keys stay visible rather than silently mirrored).
  2. Replaced implicit "allow any field" with a hybrid policy via validate_field_update:
    • Immutable: id, created
    • Routed: status, deps, links → point to tk status / tk dep / tk link
    • Body-backed: title, description, design, acceptance → point to tk edit (these live in the markdown body, not YAML; writing them to YAML would desync with cmd_create's body emission at ticket:216,218)
    • Validated: priority (0-4), parent (must exist), type (bug|feature|task|epic|chore)
    • Freeform: everything else, guarded by [A-Za-z][A-Za-z0-9_-]* name regex to prevent regex-injection into update_yaml_field's "^" field ":" pattern
  3. Reject JSON array items containing commas (jq path: jq -e + error("array item contains comma"); no-jq path: grep -qE '"[^"]+,[^"]+"' guard). Matches the ecosystem's comma-separated array format used by ticket-query (plugins/ticket-query:33 split(val, items, ", *")), ticket-ls, and internal awk splits.
  4. 24 new regression scenarios covering all fixes plus the set -e interaction, regex-injection guard, body-vs-YAML field policy, and jq error-coupling. Full behave suite: 157/157 green. Manual no-jq verification (env -i with jq-less PATH) confirmed.

For #5 specifically, supporting comma-bearing items would require parser changes in 5+ consumers (ticket-query, ticket-ls, ticket's own array splits). Happy to open a follow-up if you'd like that fleshed out separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants