Skip to content

Agent UX: --remove-ac warns instead of failing while --ac replaces, so repairing one acceptance criterion silently drops the rest #956

Closed

Description

@unbraind

Summary

Repairing one acceptance criterion silently destroyed the other two. Three behaviours combine to make this the expected outcome of the obvious repair sequence:

  1. --remove-ac failing to match emits a warning with a zero exit, not an error.
  2. --ac replaces the entire acceptance_criteria field rather than appending.
  3. --dep, the neighbouring list field, appends — the opposite convention.

An agent that runs --remove-ac <old> --ac <new> in one invocation, which reads as "swap this one entry", ends up with an item holding only <new>.

Reproduction

Item pmc-o58h held three semicolon-separated criteria:

acceptance_criteria: The release workflow makes no request to any administration-scoped endpoint; A release pull request whose required checks are still running is waited on rather than merged; A release pull request that can never merge fails the run with the failing check names instead of waiting out the deadline

Attempting to replace only the third:

pm update pmc-o58h --author claude --message "..." \
  --remove-ac "A release pull request that can never merge fails the run with the failing check names instead of waiting out the deadline" \
  --ac "A release pull request that has not become mergeable within the bounded deadline fails the run and names the checks still unsettled and failing rather than merging"

Output:

warnings:
  - "remove_ac_unmatched:A release pull request that can never merge fails the run with the failing check names instead of waiting out the deadline"

Resulting field — two criteria gone:

acceptance_criteria: A release pull request that has not become mergeable within the bounded deadline fails the run and names the checks still unsettled and failing rather than merging

The text passed to --remove-ac was copied verbatim from the item file, so whatever normalization the matcher applies (the field is stored as one ; -joined string) did not agree with the stored form. That is worth fixing on its own, but it is the combination that loses data.

Expected

Any one of these would prevent the silent loss:

  • --remove-ac that does not match should be an error, not a zero-exit warning. A removal that removed nothing is a failed instruction, and the caller almost always intends to modify what it did not remove.
  • --ac should append, matching --dep, with --replace-ac / --clear-ac as the explicit destructive forms (--replace-deps and --clear-deps already exist for dependencies).
  • At minimum, --ac replacing a non-empty field should warn with the count it is discarding: replacing 3 acceptance criteria with 1.

The --tags family already models the right shape and documents it in --help: "Set comma-separated tags (replaces existing). Use --add-tags / --remove-tags to mutate additively." --ac has no such note, and its sibling --dep behaves the other way.

Why this matters for agents

Acceptance criteria are the field a reviewer checks a closure claim against, so silently dropping two of three converts a well-evidenced item into an under-evidenced one with no error anywhere. The write reports success; the loss is only visible by reading the item back.

The general pattern worth fixing: across pm update, list-valued flags do not share a convention (--dep appends, --ac replaces, --tags replaces but documents it and offers additive variants), and the removal flags fail soft. An agent cannot infer which is which from --help.

Related

Activity

  1. coderabbitai commented on Aug 9, 2026

    @coderabbitai
    ⚠️ Possible Duplicate Issue(s)
    🔗 Related PRs

    #200 - Add validation self-repair and agent ergonomics primitives [closed]
    #499 - Harden SDK schema-driven input fidelity and package preflight [merged]
    #545 - SDK contract integrity: workspace paths, dependency truth, validation, and MCP parity [merged]
    #593 - SDK-first agent UX graph, ownership, and recovery contracts [merged]
    #616 - SDK batch transactions and conflict-safe mutation integrity [merged]


    📝 Issue Planner

    Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.

    • Create Plan

    🧪 Issue enrichment is currently in open beta.

    You can configure auto-planning by selecting labels in the issue_enrichment configuration.

    To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

    issue_enrichment:
      auto_enrich:
        enabled: false

    💬 Have feedback or questions? Drop into our discord!

  2. unbraind commented on Aug 9, 2026

    @unbraind
    OwnerAuthor

    Tracked as pm-lppm6y. It is distinct follow-on work to pm-xh82: the new item owns failure-atomic unmatched removal and lossless one-entry replacement semantics.

  3. unbraind commented on Aug 10, 2026

    @unbraind
    OwnerAuthor

    Merged in PR #961. Acceptance-criteria repair is failure-atomic, replacement conflicts are rejected, unmatched selectors fail, and single/bulk contracts are aligned. Tracker evidence: pm-lppm6y. Merged commit: fa3f8d8.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions