Repository navigation
fix: exclude expanded tools from prompt diffs - #957
Conversation
| for _, key := range []string{"parameters", "custom"} { | ||
| if value, ok := model[key].(map[string]any); ok && len(value) == 0 { | ||
| delete(model, key) | ||
| if parameters, ok := model["parameters"].(map[string]any); ok { |
There was a problem hiding this comment.
We should validate user's input parameters as an object here I think.
In case someone does:
key: default
name: Default
model:
modelName: gpt-5
parameters: temperature=0.2
---
There was a problem hiding this comment.
Agreed. Your example synced without an error, and ldcli sent "parameters": "temperature=0.2" as a string. LaunchDarkly does not keep a non-object value there, so the setting was lost. Variation.Validate now rejects a non-object model.parameters or model.custom with model.parameters must be an object (or model.custom ...) before any write. TestPromptRejectsModelValuesThatAreNotObjects covers your example, an empty parameters:, and a list in custom, and it failed before the fix. Added in f001a10.
| requireOnlyReads(t, api.requests) | ||
| } | ||
|
|
||
| func TestPromptDiffExcludesExpandedToolsFromModelParameters(t *testing.T) { |
There was a problem hiding this comment.
This seems to cover only pull. think it would be worth it for a push test case?
There was a problem hiding this comment.
Good call, added TestPromptPushExcludesExpandedToolsFromModelParameters. The fake API can now add the expanded tools to model.parameters.tools on every read, as LaunchDarkly does. The test changes only the local instructions and checks that the plan says instructions changed with no model change. It then pushes, checks that LaunchDarkly gets the local model without the expanded tools, and checks that the next sync reports No changes.. Without this PR's fix, the same test fails with a conflict, because the expanded tools look like a model change in LaunchDarkly. Added in 4e0e87b.
nkester-ld
left a comment
nkester-ld
left a comment
There was a problem hiding this comment.
Might be worth the extra validation and test cases
LaunchDarkly keeps model.parameters and model.custom only as objects. Sync now rejects any other value before it writes.
The test of expanded tools covered only a pull. The fake API can now add the tool content to model.parameters.tools on each read, as LaunchDarkly does.
Context
LaunchDarkly can return one attached tool in two forms. The response has a top-level
toolsreference and an expandedmodel.parameters.toolsvalue.ldcli treated the expanded copy as a model setting. The plan reported a model change and a pull wrote the expanded tool into the local file.
What changes
The review output now changes as follows:
Design notes
CanonicalModelremoves only the API-expandedmodel.parameters.toolsvalue, and only when the variation has tool references. A legitimatemodel.parameters.skillsvalue remains.toolsmodel parameter remains unchanged when the variation has no tool attachment.Review focus
Verification
go test ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.9.0 runmake buildTestPromptDiffExcludesExpandedToolsFromModelParametersfailed onmainand passes with this change.TestCatalogClientConfigRemovesExpandedToolsFromModelParametersandTestFingerprintVariationExcludesExpandedToolParametersmake sure that amodel.parameters.skillsvalue stays in the decoded model and changes the fingerprint.Related changes
This stack builds on #937 to #941, which are merged.
ctawiah/fix-prompt-tool-diff-5b1c)ctawiah/plan-referenced-attachments-1e8a)ctawiah/keep-unused-attachments-1e8a)ctawiah/add-attachments-1e8a)ctawiah/route-scopes-1e8a)ctawiah/scope-commands-1e8a)ctawiah/create-attachments-1e8a)ctawiah/create-attachment-flags-1e8a)ctawiah/share-variation-selection-1e8a)ctawiah/create-variations-1e8a)ctawiah/create-variation-flags-1e8a)Note
Overview
LaunchDarkly often returns the same tools both as top-level
toolsrefs and as expandedmodel.parameters.tools. ldcli was treating that duplicate as a real model change, so prompt sync showed false model diffs and pulls could persist expanded tool JSON locally.This PR introduces shared
CanonicalModelnormalization: when a variation has tool attachments, it strips API-expandedparameters.toolswhile keeping other parameters (includingparameters.skills), still dropping emptyparameters/customobjects. That runs when decoding catalog config responses, when fingerprinting variations, and when rendering variation JSON for diffs—so planning, pull/push, and manifest fingerprints agree.Variation validation now rejects non-object
model.parametersandmodel.custombefore writes, matching LaunchDarkly’s object-only semantics.Regression tests cover catalog decode, fingerprints, plan/diff output, and end-to-end prompt sync (including simulated
expandToolsAPI behavior).Reviewed by Cursor Bugbot for commit 4e0e87b. Bugbot is set up for automated code reviews on this repo. Configure here.