Skip to content

fix: plan a tool or skill row when a variation newly references it - #945

Open
ctawiah wants to merge 4 commits into
ctawiah/fix-prompt-tool-diff-5b1cfrom
ctawiah/plan-referenced-attachments-1e8a
Open

ctawiah wants to merge 4 commits into
ctawiah/fix-prompt-tool-diff-5b1cfrom
ctawiah/plan-referenced-attachments-1e8a

Conversation

@ctawiah

@ctawiah ctawiah commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Context

A sync plans one row for each variation. In the tools, skills, and all scopes, it also plans one row for each tool and skill. A variation row also writes the content of each tool and skill that the variation uses. A push publishes the local copy, and a pull writes the LaunchDarkly copy to the local file.

checkAttachmentBaselines (#939) stops a variation row from overwriting a tool that changed on the other side. But it skips a tool that the overwritten side of the variation does not use yet. So in a sync prompts plan, which has no tool rows, two overwrites happen on main:

  • A variation push that newly attaches a tool publishes the old local copy over a change in LaunchDarkly.
  • A variation pull that newly references a tool writes the old LaunchDarkly copy over a local edit.

#941 already prevents both overwrites when the plan has a row for the tool (attachmentResolver.settledRow). This PR plans that row.

What changes

  • After the variation rows, planReferencedAttachments collects each tool and skill that a variation row writes to a side that does not use it yet. Only a push or a pull counts. A conflict writes no side until the user chooses one, and its choice covers only the variation, so a conflict gets no extra row.
  • It plans those tools and skills with the existing planAttachmentRows, and keeps only the rows where both sides exist and differ. Only then can the variation row overwrite something.
  • The existing apply order (tools and skills first), settledRow, and groupConflicts do the rest. When both sides of the tool changed, the tool row is a conflict, and the variations that share the tool get one choice.
  • A newly referenced tool with equal sides, or one that LaunchDarkly does not have, gets no row. Those plans do not change.

The push case against a stand-in for the LaunchDarkly API. The search tool changed in LaunchDarkly, and attach adds it to a second variation:

# main
Prompts
  pushed   demo/support/concise   pins search v3
  pulled   demo/support/default   pins search v3, summarize v1
LaunchDarkly search: v3 'Search the help center'      (the LaunchDarkly change is gone)

# this PR
Tools
  pulled   demo/search            version 2
Prompts
  pushed   demo/support/concise   pins search v2
  pulled   demo/support/default   pins search v2, summarize v1
LaunchDarkly search: v2 'Changed in LaunchDarkly'

Design notes

  • The fix reuses the planning and resolver code of feat: plan standalone tool and skill rows by sync scope #939 and feat: apply tool and skill rows in a sync #941: planAttachmentRows, planAttachment, the shared attachmentCache, settledRow, and groupConflicts. There is no second guard.
  • The other option was a guard that turns the whole variation row into a conflict. That asks the user to choose a side for variation content that did not change. The tool row lets the plan keep both changes.
  • checkAttachmentBaselines now uses the new writtenSides(), the same helper that finds the new references. Both read the direction of a row in one place.
  • The new rows come from the variation rows, not from the filter. So a --config-key sync is safe too, and it reads only tools of the projects that it already reads.
  • This PR does not depend on the rest of the stack. It can merge and ship on its own.

Review focus

  • Can a plan now get a tool row that changes a plan that was safe before, for example the sync after attach?
  • Is the conflict case right? A tool that changed on both sides must become one conflict with the variations that use it.

Verification

  • go test ./... and golangci-lint run (v2.9.0) pass.
  • These acceptance tests run sync prompts through the command. They fail on main and pass here:
    • TestPromptPushOfANewToolReferenceKeepsTheLaunchDarklyChange
    • TestPromptPullOfANewToolReferenceKeepsTheLocalEdit, whose next dry run plans only the repin of the variation
    • TestPromptPullOfANewSkillReferenceKeepsTheLocalEdit
    • TestPromptNewToolReferenceWithTwoChangesIsAConflict
  • TestPromptNewToolReferenceWithEqualSidesPlansNoToolRow checks that a safe plan does not change.
  • TestPromptConflictChoiceDoesNotPullAToolThatOnlyTheLocalVersionUses checks that a choice of LaunchDarkly for a conflicted variation does not pull a tool that only the local version newly uses. It fails without 0a8d6d07.
  • I built ldcli from main and from this branch, and ran both flows against a stand-in for the LaunchDarkly API. main overwrites in both flows. This branch keeps both changes.

Related changes

This stack builds on #937 to #941, which are merged.

  1. fix: exclude expanded tools from prompt diffs #957: Exclude expanded tools from prompt diffs (ctawiah/fix-prompt-tool-diff-5b1c)
  2. This PR: plan a tool or skill row when a variation newly references it (ctawiah/plan-referenced-attachments-1e8a)
  3. feat: keep unused tools and skills, and detach any resource kind #946: Keep unused tools and skills, and detach any resource kind (ctawiah/keep-unused-attachments-1e8a)
  4. feat: add tools and skills from LaunchDarkly #947: Add tools and skills from LaunchDarkly (ctawiah/add-attachments-1e8a)
  5. feat: route sync, add, and detach by scope #948: Route sync, add, and detach by scope (ctawiah/route-scopes-1e8a)
  6. feat: add the sync all, sync tools, and sync skills commands #949: Add the sync all, sync tools, and sync skills commands (ctawiah/scope-commands-1e8a)
  7. feat: create a tool or a skill from the content of an add #950: Create a tool or a skill from the content of an add (ctawiah/create-attachments-1e8a)
  8. feat: add the create flags of sync tools add and sync skills add #951: Add the create flags of sync tools add and sync skills add (ctawiah/create-attachment-flags-1e8a)
  9. refactor: share the selection of a new variation, and check any kind before an add #952: Share the selection of a new variation, and check any kind before an add (ctawiah/share-variation-selection-1e8a)
  10. feat: create a prompt variation from the content of an add #953: Create a prompt variation from the content of an add (ctawiah/create-variations-1e8a)
  11. feat: add the create flags of sync prompts add #954: Add the create flags of sync prompts add (ctawiah/create-variation-flags-1e8a)

Note

Overview
Fixes sync prompts so attaching a tool or skill to a variation no longer silently overwrites the other side’s attachment content.

When a variation push/pull newly references a tracked tool or skill that differs locally and in LaunchDarkly, planning now adds an explicit tool/skill row (via planReferencedAttachments) so content is reconciled before the variation pins a version. Equal content or missing server copies still get no extra row.

Conflict handling is extended: conflictUses tracks tools/skills only needed for one side of a conflicted variation; applyConflictResolutions drops or keeps those rows based on the user’s choice, and conflict grouping no longer absorbs them incorrectly.

Broad acceptance tests cover push/pull, skills, dual-side edits, conflict flags, and repin-only follow-up syncs.

Reviewed by Cursor Bugbot for commit cf6adb5. Bugbot is set up for automated code reviews on this repo. Configure here.

@ctawiah
ctawiah added this pull request to stack #955 October 22, 2026 22:19
A variation row writes the content of each tool and skill that it uses.
The guard for tool and skill changes skips one that the overwritten
variation does not use yet. So in a sync of prompts, two overwrites
happened:

- A push of a variation that newly attaches a tool published the old
  local copy over a change in LaunchDarkly.
- A pull of a variation that newly references a tool wrote the old
  LaunchDarkly copy over a local edit.

The plan now has a row for such a tool or skill when it exists on both
sides with different content. The row decides the content, as in a sync
of every kind: it pulls, pushes, or is a conflict. The variation row then
uses that content and pins its version. A tool or skill with equal sides
gets no row, so other plans do not change.
Comment thread internal/sync/prompt/plan.go Outdated
Comment on lines +90 to +93
// writtenSides returns the variation that a variation row writes, and the
// variation that the write replaces. A push writes the local variation to
// LaunchDarkly, and a pull writes the LaunchDarkly variation to the local
// file. Both are nil for a row that writes no variation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of a confusing comment. either remove are make more succinct imo

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, shortened it to two lines in 0a8d6d0.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread internal/sync/prompt/state.go
A conflicted variation planned a row for each tool and skill that one
of its sides newly used. The conflict choice of the variation then also
covered that row. So a choice of LaunchDarkly, whose version does not
use the tool, pulled the tool over a local edit.

A conflict writes no side until the user chooses one. So only a push or
a pull of a variation now plans these rows. A conflict plans no extra
rows. The comment of writtenSides is shorter.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0a8d6d0. Configure here.

Comment thread internal/sync/prompt/state.go
…ly uses

A conflicted variation planned no row for a tool or skill that one of its
sides newly used. So a choice of local published the local copy over a
LaunchDarkly change of the tool, and a tool that changed on both sides
never became part of the conflict.

Each side of a conflict now counts. A row that only conflicts need joins
the conflict when the tool or skill changed on both sides. Otherwise it
keeps its own action, and stays in the plan only when the chosen side
uses the tool or skill. So a choice of local pulls the LaunchDarkly change
first, and a choice of LaunchDarkly still leaves a tool that it does not
use as it is.
// A push of a variation that newly attaches a tracked tool must not publish
// the local copy over a change in LaunchDarkly. The plan pulls the tool, and
// the variation pins the pulled version.
func TestPromptPushOfANewToolReferenceKeepsTheLaunchDarklyChange(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no change needed - calling out the push with the dry run flag doesn't change anything might be helpful for the test name/descriptive comment. It immediately gives a good understanding of the test. same with the pull using the flag.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done in cf6adb5. Both comments now say the dry run plans the tool row and changes nothing, and the tests check that right after the dry run.

The push and pull tests of a new tool reference start with a dry run.
Their comments now say so, and each test checks after the dry run that
it sent only reads and left the tool file as it was.
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