🐛 fix(cli): resolve rule ids before submitting a standard

[987poiuytrewq]

Explanation

Removing or editing a rule in a standard and submitting it with --no-review reports success and changes nothing on the server. The rule is still there afterwards, and nothing retries it.

compareStandardFields stamps every deleteRule and updateRule proposal with a placeholder id:

for (const rule of remainingDeleted) {
  const ruleId = createRuleId('unresolved');
  changes.push({
    type: ChangeProposalType.deleteRule,
    payload: { targetId: ruleId, item: { id: ruleId, content: rule } },
  });
}

createRuleId is brandedIdFactory(), which returns (id) => id, so targetId is the literal string 'unresolved'. The local Markdown carries no rule ids, so there was nothing else to put there.

StandardChangeProposalApplier resolves both change types by id:

const filteredRules = rules.filter(
  (rule) => rule.id !== changeProposal.payload.targetId,
);

The placeholder matches no rule, so the filter removes nothing and updateRule's map rewrites nothing. On the --no-review path, where playbookSubmitHandler sends proposals straight to batchApply, that is the whole story:

1. batchApply returns success, because the standard itself was touched.
2. The handler prints 1 standard updated from collectParts(response.updated).
3. It then clears the staged change, so the local playbook forgets the deletion.

The rule survives, the operator is told it did not, and there is nothing left locally to retry. Rule additions are unaffected, which is why this reads as "some edits don't stick" rather than as a broken command.

The fix

The CLI already had the missing piece. StandardsGateway.getRules returns { id, content }[] for a standard and was called from nowhere in the codebase. buildProposals now takes an optional fetcher, reads a standard's rules once, and hands compareStandardFields a content-to-id map. No server change and no new endpoint.

Two things left deliberately alone:

• StandardDiffStrategy keeps the placeholder. It feeds playbook diff, which only prints; no id is ever dereferenced there. Adding a network call to a display command seemed a poor trade.
• An unmatched rule stops the submit rather than going out as a placeholder. If the lookup fails, or comes back without one of the rules being removed or edited, that standard is reported through the skipped path the builder already uses for stale deployed content: the handler keeps the staged change and exits 1, so the operator can retry. Additions are exempt, since they name no existing rule — a standard that only gained rules still submits. (This replaces an earlier fallback to the placeholder, which review correctly pointed out would recreate the data loss by another route.)

Type of Change

• Bug fix
• New feature
• Improvement/Enhancement
• Refactoring
• Documentation
• Breaking change

Affected Components

• Domain packages affected: apps/cli only (application/utils/artifactComparison.ts, infra/commands/playbook/submit/proposalBuilder.ts, infra/commands/playbook/submitHandler.ts)
• Frontend / Backend / Both: neither — CLI
• Breaking changes (if any): none. The new parameters are optional and every existing call site keeps its current behaviour.

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing completed
• Test coverage maintained or improved

Test Details:

Thirteen tests added across the two specs.

artifactComparison.spec.ts — compareStandardFields:

• targets a deleted rule by its server id
• carries the server id on the deleted rule item
• targets an updated rule by the id of the content it replaces
• falls back to the placeholder when the deleted rule is absent from the supplied ids
• falls back to the placeholder when no ids are supplied

proposalBuilder.spec.ts — buildProposals:

• targets a deleted rule by the id fetched from the standard
• fetches the rules with the space and artifact of the standard
• fetches a standard's rules once when several agents render it
• skips the entry rather than submitting an unresolved removal when the lookup fails
• submits no proposal for the skipped standard
• submits a standard that only gained rules anyway, since an addition names no existing rule
• skips the entry when a changed rule is missing from the fetched ids
• falls back to the placeholder when no fetcher is provided

The six that assert the new behaviour were confirmed to fail against main with only the specs applied, so they pin the behaviour rather than the implementation. The fallback tests pass either way, by design.

Verified against a live instance. Both CLIs were run against the same standard in a throwaway space, deleting the same rule with --no-review, pulling afterwards to read the server's own state back:

	reported	rule after pull
0.35.1 as published	1 standard updated	still there
this branch	1 standard updated	gone

Editing a rule was checked the same way and also lands on this branch, which matters because matchUpdatedRules turns a reworded rule into an updateRule carrying the same placeholder. The fixture was restored to its published state afterwards — by another --no-review deletion, which is its own small proof.

nx test packmind-cli — 126 suites, 3490 tests, all passing. nx typecheck packmind-cli clean. nx lint packmind-cli reports the same 13 pre-existing warnings as main, none in the changed files. prettier --check clean.

TODO List

• CHANGELOG Updated
• Documentation Updated

Reviewer Notes

This is not scoped to --no-review. buildProposals runs before the noReview branch, so the resolved ids now reach batchCreate as well as batchApply. That is deliberate, but it is the part I would most like a second opinion on, because I could not establish what the review path does with targetId today.

What the public code shows:

• StandardChangesApplier extends StandardChangeProposalApplier, so the review path ends in the same rules.filter((rule) => rule.id !== changeProposal.payload.targetId).
• That branch reads changeProposal.payload directly rather than through getEffectivePayload, so a reviewer's decision does not supply the id either.
• There is no content-based rule lookup anywhere in this repository, and StandardChangeProposalApplier.spec.ts supplies a real ruleId in its deleteRule fixtures.

Taken alone that suggests a CLI-originated rule deletion could never have applied on either path, which does not match what we observe — deletions do land through review. So batchCreate most likely resolves targetId when the proposal is created. That code is in the closed edition (the OSS PlaybookChangeManagementAdapter throws Method not implemented for every method), so this is inference, not something I could read.

Either way the change should be safe: if creation resolves the id, a correct id arriving pre-resolved is at worst redundant; if it does not, this repairs the review path too. One argument in favour of ids over content matching on this path specifically: a review proposal can sit pending for days, and saveNewVersion passes rule ids through to updateStandard, so an id survives someone rewording that rule in the UI where a content key would not.

The check I would ask of someone with access: submit a rule removal through review and look at the stored proposal's payload.targetId. If it is the literal string unresolved, the review path has the same defect and this fixes both.

Two smaller notes:

• matchUpdatedRules pairs a delete with an add into an updateRule, so a merely reworded rule takes the same path and was equally affected.
• The fetcher is keyed on spaceId/standardId and cached for the lifetime of one buildProposals call, so a standard rendered for several agents costs one request.

🤖 Generated with Claude Code

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the previously reported lookup-failure data-loss path is now fully guarded.

Summary

This PR fixes CLI standard submissions by resolving server-side rule IDs before constructing update and deletion proposals.

• Fetches and caches rule IDs once per standard during proposal construction.
• Skips the entire submission safely when a required rule ID cannot be resolved, preserving staged changes for retry.
• Adds focused coverage for successful resolution, failed and incomplete lookups, caching, and addition-only submissions.
• Updates the changelog for the corrected behavior.

Diagram

sequenceDiagram
    participant CLI as playbook submit
    participant Gateway as StandardsGateway
    participant Builder as Proposal Builder
    participant Server as Proposal API

    CLI->>Gateway: getRules(spaceId, standardId)
    Gateway-->>CLI: rule IDs and contents
    CLI->>Builder: buildProposals(changes, rule map)
    Builder->>Builder: Resolve update/delete target IDs
    alt Every required rule ID resolves
        Builder-->>CLI: Prepared proposals
        CLI->>Server: batchApply or batchCreate
        Server-->>CLI: Success
        CLI->>CLI: Clear staged changes
    else Lookup fails or target is missing
        Builder-->>CLI: Skipped entry
        CLI->>CLI: Abort batch and retain staging
    end

Loading

_{Reviews (2) · Last reviewed commit: "🐛 fix(cli): keep the staged change when..."}

[quentinlebourles-packmind]

Hi @987poiuytrewq, thank you for this PR and for the detailed write-up. You are right on your diagnostic, the --no-review rule removals and edits were silently dropped.

We're not going to merge it, though, because the fix belongs on the server. Fixing it in the CLI only helps people who upgrade, and every CLI already installed would keep losing changes.

If you can rectify it, else we will open a ticket and do it later.

Again, thanks you for your efforts.

[987poiuytrewq]

Hi @quentinlebourles-packmind - yeah you're right this fix should be done server-side. I did this to unblock my workflow in the meantime.

Had a go at doing the server-side fix in #527 - it's got some design decisions to be made in there though before you merge.

No worries - happy to help!