Skip to content

fix: allow converting string literal block with list#3050

Open
Yuan325 wants to merge 1 commit intomainfrom
migrate-fix
Open

fix: allow converting string literal block with list#3050
Yuan325 wants to merge 1 commit intomainfrom
migrate-fix

Conversation

@Yuan325
Copy link
Copy Markdown
Contributor

@Yuan325 Yuan325 commented Apr 14, 2026

This PR updates the following:

  • Fix YAML String Block Conversion: Resolved an issue where multiline strings (like tool descriptions) containing list syntax were being re-encoded as double-quoted strings with explicit \n characters. They now correctly use the | literal block style as expected.
description: |
  this is the description
  this tool uses the following parameter:
  - param_1
  - param_2

# will turn into 

description: "this is the description\nthis tool uses the following parameter:\n-param_1\n-param_2"
  • Updated the converter to identify and retain initial comment lines/license headers at the top of configuration files.
  • Updated migration completion status to reflect "ended" state.
  • Update to use "v1" -> "nested format" and "v2" -> "flat format"
  • Remove "authSources" when checking for keys. We had previously removed support for "authSources".

Fixes #3023

@Yuan325 Yuan325 requested a review from a team as a code owner April 14, 2026 02:37
@Yuan325 Yuan325 force-pushed the migrate-fix branch 2 times, most recently from 4cf9718 to 2a1728c Compare April 14, 2026 02:39
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the configuration conversion logic to preserve top-level comments and empty lines, while transitioning terminology from 'v1/v2' to 'nested/flat' formats. It also enables literal style for multiline strings in YAML encoding. Review feedback suggests renaming variables for consistency, adding the missing 'authSources' key to the migration list, correcting comment numbering, and updating related informational logs that have become inaccurate due to these changes.

I am having trouble creating individual review comments. Click here to see my feedback.

cmd/internal/config.go (147)

high

To align with the PR's goal of moving away from "v1/v2" terminology, this variable should be renamed (e.g., to nestedFormatKeys). Additionally, authSources is missing from this list, which prevents the migration tool from detecting and converting documents that use the deprecated authSources key, even though it is explicitly handled in the switch statement at line 170.

	nestedFormatKeys := []string{"sources", "authSources", "authServices", "embeddingModels", "tools", "toolsets", "prompts"}

cmd/internal/config.go (126)

medium

The comment starts with "2.", but there is no preceding "1." step in this function. It is better to use a descriptive comment without numbering to avoid confusion.

	// Manually copy top-level comments and empty lines from the source

cmd/internal/config.go (163)

medium

Update the variable name to match the suggested rename at line 147.

			if slices.Contains(nestedFormatKeys, key) {

cmd/internal/migrate/command.go (130)

medium

While updating this message, please note that the informational log at line 68 ("any comments... will not be preserved") is now incorrect because of the changes in ConvertConfig that preserve top-level comments. Since line 68 is outside the current diff hunks, it should be updated in a separate commit or by expanding the PR scope to ensure users are not given misleading information.

@Yuan325 Yuan325 force-pushed the migrate-fix branch 2 times, most recently from 93a516a to efe5e17 Compare April 14, 2026 02:45
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.

migration add \n instead of separating lines for description

2 participants