fix(installer): remove double-escaping of quotes in CSV manifest pipeline#1746
fix(installer): remove double-escaping of quotes in CSV manifest pipeline#1746
Conversation
🤖 Augment PR SummarySummary: Removes redundant pre-escaping of double quotes in manifest text cleaning to prevent quote duplication after CSV write/parse round-trips. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| .replaceAll('{{module}}', artifact.module || 'core') | ||
| .replaceAll('{{path}}', pathToUse) | ||
| .replaceAll('{{description}}', artifact.description || `${artifact.name} ${artifact.type || ''}`) | ||
| .replaceAll('{{description}}', (artifact.description || `${artifact.name} ${artifact.type || ''}`).replaceAll('"', "'")) |
There was a problem hiding this comment.
renderTemplate() now replaces " with ' for {{description}} unconditionally, but this method is used for non-TOML templates too (MD/YAML), so it will silently alter descriptions/trigger-phrases outside Gemini TOML generation as well. Also, if artifact.description is ever non-string, the .replaceAll() call will throw at runtime.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| @@ -1,4 +1,4 @@ | |||
| description = """{{description}}""" | |||
| description = "{{description}}" | |||
There was a problem hiding this comment.
Switching to description = "{{description}}" means the description must be valid TOML “basic string” content (no raw newlines; backslashes/control chars need escaping), but the current sanitization only handles ". If any workflow description includes those characters, the generated .toml will still be invalid.
Severity: medium
Other Locations
tools/cli/installers/lib/ide/templates/combined/gemini-workflow-yaml.toml:1
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/installers/lib/ide/_config-driven.js (1)
393-398:⚠️ Potential issue | 🟠 MajorDescription sanitization is both over-broad and incomplete, leaving backslash-containing descriptions still broken.
Three issues with the current approach:
Over-broad:
replaceAll('"', "'")runs unconditionally insiderenderTemplate(), which is called for every output format —.md,.toml,.yaml,.yml. For Markdown output, double quotes in descriptions are valid and semantically meaningful. This universally corrupts descriptions likeThe "recommended" approachin all non-TOML outputs.Wrong escape strategy: The TOML-correct escape for a double quote inside a basic string is
\", not'. Replacing with'is a lossy transformation — the stored description content is permanently altered in all generated files.Incomplete: In a TOML basic string, any Unicode character may be used except those that must be escaped: quotation mark, backslash, and the control characters other than tab. The sanitization only handles
"but not\. A description containing a backslash (e.g., a regex example like\d+, or a Windows path) still produces invalid TOML.The fix should be applied in a format-aware manner — only when rendering a TOML template — and should properly escape all TOML-unsafe characters:
🔧 Proposed fix: TOML-aware sanitization helper
renderTemplate(template, artifact) { + // Determine if the template produces TOML (used to apply correct escaping) + const isToml = template.includes('description = "{{description}}"'); + + const sanitizeForFormat = (str) => { + if (!isToml) return str; + // Must escape backslash first, then double-quote, then control chars + return str.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n').replace(/\r/g, '\\r'); + }; let rendered = template .replaceAll('{{name}}', artifact.name || '') .replaceAll('{{module}}', artifact.module || 'core') .replaceAll('{{path}}', pathToUse) - .replaceAll('{{description}}', (artifact.description || `${artifact.name} ${artifact.type || ''}`).replaceAll('"', "'")) + .replaceAll('{{description}}', sanitizeForFormat(artifact.description || `${artifact.name} ${artifact.type || ''}`)) .replaceAll('{{workflow_path}}', pathToUse);Alternatively, pass the detected extension from
loadTemplateintorenderTemplateto make format detection explicit and reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` around lines 393 - 398, The description replacement currently unconditionally replaces double quotes with single quotes in the renderTemplate block (the chained replaceAll on template that replaces '{{description}}'), which corrupts non-TOML outputs and fails to escape backslashes for TOML; change this to be format-aware (pass the template extension from loadTemplate into renderTemplate or detect the format from the template name) and only apply TOML-safe escaping when rendering TOML: for '{{description}}' replace backslash with '\\\\' and double-quote with '\\"' (and escape other TOML control characters like newline/tab as needed) before inserting into template, leaving descriptions untouched for markdown/yaml outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Line 397: The replacement for '{{description}}' can produce a trailing space
when artifact.type is falsy; update the expression used in the replaceAll call
that builds the fallback description so it only inserts a space when
artifact.type is present (for example by joining [artifact.name,
artifact.type].filter(Boolean).join(' ') or using a conditional: artifact.type ?
`${artifact.name} ${artifact.type}` : artifact.name) while preserving the
subsequent .replaceAll('"', "'") behavior; locate the
replaceAll('{{description}}', ...) invocation to make this change.
In `@tools/cli/installers/lib/ide/templates/combined/gemini-workflow-yaml.toml`:
- Line 1: The template changed description from a TOML multi-line string to a
basic single-line string causing failures when descriptions contain backslashes
because renderTemplate() doesn't escape backslashes; revert the template in
gemini-workflow-yaml.toml to use a TOML literal multi-line string (description =
"""{{description}}""") so backslashes and quotes don't need escaping, and if you
prefer keeping single-line strings instead, update renderTemplate() to also
escape backslashes and quotes before insertion (refer to renderTemplate() for
where substitution happens).
In `@tools/cli/installers/lib/ide/templates/combined/gemini-workflow.toml`:
- Line 1: The description TOML field was downgraded from a multi-line basic
string to a single-line string causing backslashes in descriptions to produce
invalid TOML; update the template so the description field uses a TOML
multi-line basic string like the existing prompt field (i.e., change the
description mapping from description = "{{description}}" to the triple-quoted
multi-line form), and ensure the renderer still sanitizes or escapes any literal
""" sequences in the description value before insertion to avoid breaking the
delimiter.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 393-398: The description replacement currently unconditionally
replaces double quotes with single quotes in the renderTemplate block (the
chained replaceAll on template that replaces '{{description}}'), which corrupts
non-TOML outputs and fails to escape backslashes for TOML; change this to be
format-aware (pass the template extension from loadTemplate into renderTemplate
or detect the format from the template name) and only apply TOML-safe escaping
when rendering TOML: for '{{description}}' replace backslash with '\\\\' and
double-quote with '\\"' (and escape other TOML control characters like
newline/tab as needed) before inserting into template, leaving descriptions
untouched for markdown/yaml outputs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/templates/combined/gemini-workflow-yaml.tomltools/cli/installers/lib/ide/templates/combined/gemini-workflow.toml
| .replaceAll('{{module}}', artifact.module || 'core') | ||
| .replaceAll('{{path}}', pathToUse) | ||
| .replaceAll('{{description}}', artifact.description || `${artifact.name} ${artifact.type || ''}`) | ||
| .replaceAll('{{description}}', (artifact.description || `${artifact.name} ${artifact.type || ''}`).replaceAll('"', "'")) |
There was a problem hiding this comment.
Trailing space in fallback description when artifact.type is falsy.
The expression `${artifact.name} ${artifact.type || ''}` produces "name " (trailing space) when artifact.type is undefined, null, or an empty string.
🐛 Proposed fix
- .replaceAll('{{description}}', (artifact.description || `${artifact.name} ${artifact.type || ''}`).replaceAll('"', "'"))
+ .replaceAll('{{description}}', (artifact.description || [artifact.name, artifact.type].filter(Boolean).join(' ')))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .replaceAll('{{description}}', (artifact.description || `${artifact.name} ${artifact.type || ''}`).replaceAll('"', "'")) | |
| .replaceAll('{{description}}', (artifact.description || [artifact.name, artifact.type].filter(Boolean).join(' ')).replaceAll('"', "'")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/ide/_config-driven.js` at line 397, The replacement
for '{{description}}' can produce a trailing space when artifact.type is falsy;
update the expression used in the replaceAll call that builds the fallback
description so it only inserts a space when artifact.type is present (for
example by joining [artifact.name, artifact.type].filter(Boolean).join(' ') or
using a conditional: artifact.type ? `${artifact.name} ${artifact.type}` :
artifact.name) while preserving the subsequent .replaceAll('"', "'") behavior;
locate the replaceAll('{{description}}', ...) invocation to make this change.
| @@ -1,4 +1,4 @@ | |||
| description = """{{description}}""" | |||
| description = "{{description}}" | |||
There was a problem hiding this comment.
Same fragility regression as gemini-workflow.toml — description changed from multi-line to single-line basic string.
Identical analysis applies: the multi-line """...""" format tolerates unescaped double quotes natively per TOML spec, while the new single-line "..." format requires both " and \ to be escaped. The incomplete sanitization in renderTemplate() (backslashes unhandled) makes this template breakable by descriptions containing \. Revert to """{{description}}""" consistent with the fix suggested for gemini-workflow.toml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/ide/templates/combined/gemini-workflow-yaml.toml` at
line 1, The template changed description from a TOML multi-line string to a
basic single-line string causing failures when descriptions contain backslashes
because renderTemplate() doesn't escape backslashes; revert the template in
gemini-workflow-yaml.toml to use a TOML literal multi-line string (description =
"""{{description}}""") so backslashes and quotes don't need escaping, and if you
prefer keeping single-line strings instead, update renderTemplate() to also
escape backslashes and quotes before insertion (refer to renderTemplate() for
where substitution happens).
| @@ -1,4 +1,4 @@ | |||
| description = """{{description}}""" | |||
| description = "{{description}}" | |||
There was a problem hiding this comment.
Downgrade from """...""" to "..." makes the description field strictly less robust.
The previous multi-line basic string ("""...""") permitted unescaped double quotes per the TOML spec: you can write a quotation mark, or two adjacent quotation marks, anywhere inside a multi-line basic string — they can also be written just inside the delimiters. Only a """ sequence inside the value would require special handling (extremely unlikely in a workflow description).
The new single-line "..." format requires both " AND \ to be escaped. Since the renderer's sanitization only handles " (and via ' replacement, not \"), any description containing a backslash (e.g., a regex or path example) still generates invalid TOML — and now there is also no buffer from the multi-line format.
The prompt field in this same file correctly uses """...""". Aligning description to also use a multi-line basic string would be more robust and consistent:
🔧 Proposed revert to multi-line basic string
-description = "{{description}}"
+description = """{{description}}"""This renders valid TOML for any real-world description (those containing """ are astronomically unlikely), and the renderer-level sanitization could be kept as a safety net for the """ case only.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description = "{{description}}" | |
| description = """{{description}}""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/ide/templates/combined/gemini-workflow.toml` at line
1, The description TOML field was downgraded from a multi-line basic string to a
single-line string causing backslashes in descriptions to produce invalid TOML;
update the template so the description field uses a TOML multi-line basic string
like the existing prompt field (i.e., change the description mapping from
description = "{{description}}" to the triple-quoted multi-line form), and
ensure the renderer still sanitizes or escapes any literal """ sequences in the
description value before insertion to avoid breaking the delimiter.
pbean
force-pushed
the
escape-fix-cis
branch
from
fe390ca to
275f81a
Compare
pbean
changed the title
|
augment review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tools/cli/installers/lib/core/manifest-generator.js (7)
661-667:⚠️ Potential issue | 🟡 Minor
trim()on CSV content can drop significant whitespace/empty trailing fields.
Trimming the full file can remove leading/trailing spaces that are part of quoted fields and can drop trailing empty rows/fields. Handle empty content explicitly without trimming the whole payload.Proposed fix
- const content = await fs.readFile(csvPath, 'utf8'); - const lines = content.trim().split('\n'); + const content = await fs.readFile(csvPath, 'utf8'); + if (!content || content.replaceAll('\r', '') === '') return []; + const lines = content.split('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 661 - 667, The code currently calls content.trim() after reading csvPath which can remove significant leading/trailing whitespace and drop trailing empty fields/rows; instead, stop trimming the entire payload: after fs.readFile(csvPath, 'utf8') check if content === '' (or content.length === 0) and return [] for an empty file, then split the raw content with content.split('\n') into lines and keep the existing lines.length < 2 guard (or adjust it to account for a single-header-with-empty-row case) so quoted-field whitespace and trailing empty fields are preserved.
1035-1038:⚠️ Potential issue | 🟠 MajorFiles-manifest CSV writes raw values without escaping.
File names/paths can contain commas or quotes; current output becomes invalid CSV. Use the same escaping as other manifests.Proposed fix
- // Add all files - for (const file of allFiles) { - csv += `"${file.type}","${file.name}","${file.module}","${file.path}","${file.hash}"\n`; - } + const escapeCsv = (value) => `"${String(value ?? '').replaceAll('"', '""')}"`; + // Add all files + for (const file of allFiles) { + csv += [ + escapeCsv(file.type), + escapeCsv(file.name), + escapeCsv(file.module), + escapeCsv(file.path), + escapeCsv(file.hash), + ].join(',') + '\n'; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 1035 - 1038, The CSV writer loop that builds the manifest (the for...of over allFiles producing csv variable) writes raw fields and breaks when names/paths contain commas or quotes; update the loop to escape each field the same way as other manifests by replacing any " characters with "" and wrapping fields in double quotes (handle null/undefined by converting to empty string) before concatenation, using the existing allFiles entries (file.type, file.name, file.module, file.path, file.hash) so the resulting line is a valid CSV.
708-728:⚠️ Potential issue | 🟠 MajorPreserved rows are re-emitted without escaping quotes.
Wrapping with"is not enough; internal quotes must be doubled. This becomes a correctness bug now that quotes aren’t pre-escaped.Proposed fix
- return upgradedValues.map((v) => `"${v}"`).join(','); + return upgradedValues + .map((v) => `"${String(v ?? '').replaceAll('"', '""')}"`) + .join(',');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 708 - 728, The upgradeRowToSchema function currently wraps values with double quotes but does not escape internal quotes, causing malformed CSV; update upgradeRowToSchema so that before quoting each value (in the map over upgradedValues) you replace any existing " characters with "" (CSV doubling) and then wrap the result in quotes; ensure you apply this transformation to every value produced (including defaults and empty strings) so preserved rows are emitted with internal quotes escaped.
213-216:⚠️ Potential issue | 🟡 Minor
installPathcan contain double slashes at root.
WhenrelativePathis empty, you generate.../workflows//workflow.yaml. This breaks path comparisons and hash lookups.Proposed fix
- const installPath = - moduleName === 'core' - ? `${this.bmadFolderName}/core/workflows/${relativePath}/${entry.name}` - : `${this.bmadFolderName}/${moduleName}/workflows/${relativePath}/${entry.name}`; + const rel = relativePath ? `${relativePath}/` : ''; + const installPath = + moduleName === 'core' + ? `${this.bmadFolderName}/core/workflows/${rel}${entry.name}` + : `${this.bmadFolderName}/${moduleName}/workflows/${rel}${entry.name}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 213 - 216, The installPath assignment in manifest-generator.js can produce double slashes when relativePath is empty; update the logic where installPath is computed (the ternary that uses moduleName, this.bmadFolderName, relativePath, and entry.name) to join path segments while filtering out empty segments (or branch on empty relativePath) so you never insert an extra "/" before entry.name; ensure both the 'core' and non-core branches use the same safe join approach to avoid paths like ".../workflows//workflow.yaml".
970-975:⚠️ Potential issue | 🟠 MajorHashing failures are silently swallowed.
Returning''hides IO failures and makes the manifest look valid when it isn’t. At least log a warning (or propagate).Proposed fix
- } catch { - return ''; - } + } catch (error) { + await prompts.log.warn(`Failed to hash ${filePath}: ${error.message}`); + return ''; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 970 - 975, The calculateFileHash function currently swallows all errors (catch { return '' }) making IO failures invisible; change the catch to capture the error (e.g. catch (err)) and either log a clear warning including the filePath and err (using the module's logger or console.warn) before returning a sentinel (or keep returning '' if callers expect that), or rethrow the error so callers can handle it — ensure the updated catch references calculateFileHash and filePath so the log message includes which file failed and the actual error details from fs.readFile.
669-682:⚠️ Potential issue | 🟠 MajorUse
csv.parse()instead of regex for CSV parsing to handle escaped quotes and complex data.The regex pattern
/(".*?"|[^",\s]+)(?=\s*,|\s*$)/gfails on standard CSV escaping (e.g.,""for escaped quotes). This causes silent data corruption when reading preserved CSV rows. Sincecsv-parse/syncis already imported and used correctly inwriteAgentManifest()(line 778) andwriteTaskManifest()(line 850), apply the same approach here:const records = csv.parse(content, { columns: true, skip_empty_lines: true, });Then extract only the preserved module rows instead of splitting on
\nand parsing manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 669 - 682, The current manual CSV parsing using header/headerColumns/oldColumns and the regex in the loop over dataRows is brittle and breaks on escaped quotes; replace the manual split+regex logic with the same csv-parse/sync call used in writeAgentManifest() and writeTaskManifest(): parse the full CSV content with csv.parse(content, { columns: true, skip_empty_lines: true }) to get records, then build preservedRows by selecting the preserved module rows from those parsed records (mapping back to the original column order using oldColumns if needed) instead of using the regex-based columns/cleanColumns extraction.
992-1001:⚠️ Potential issue | 🟠 MajorPath handling is unsafe; fix with proper path normalization and add validation. CSV escaping is also broken.
The
replace()approach fails whenfilePathis not underthis.bmadDir— it corrupts the path by removing only the first occurrence. The proposedpath.relative()fix is sound for computing relative paths, but code must validate that files are actually underbmadDirfirst.More critically, CSV escaping is broken: fields containing quotes create invalid CSV. A filename like
test"file.yamlproduces"test"file.yaml"with unmatched quotes, corrupting the manifest thatreadFilesManifest()parses back. Proper RFC 4180 escaping requires doubling internal quotes:"test""file.yaml".Additional concerns:
- No input validation: code assumes all
filePathentries are underbmadDirwithout verifying.- Silent errors:
calculateFileHash()returns empty string on failure, masking file access issues silently.Validate paths against
bmadDirand escape CSV fields properly by doubling quotes.Suggested fix
for (const filePath of this.allInstalledFiles) { - // Store paths relative to bmadDir (no folder prefix) - const relativePath = filePath.replace(this.bmadDir, '').replaceAll('\\', '/').replace(/^\//, ''); + // Validate and compute relative path safely + const normalizedFile = path.normalize(filePath); + const normalizedDir = path.normalize(this.bmadDir); + if (!normalizedFile.startsWith(normalizedDir + path.sep) && normalizedFile !== normalizedDir) { + console.warn(`File not under bmadDir: ${filePath}`); + continue; + } + const relativePath = path.relative(normalizedDir, normalizedFile).replaceAll('\\', '/');And for CSV escaping:
- csv += `"${file.type}","${file.name}","${file.module}","${file.path}","${file.hash}"\n`; + const escape = (s) => `"${String(s).replaceAll('"', '""')}"`; + csv += `${escape(file.type)},${escape(file.name)},${escape(file.module)},${escape(file.path)},${escape(file.hash)}\n`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 992 - 1001, The manifest generator currently uses unsafe string replaces on file paths and broken CSV quoting; update the loop that iterates this.allInstalledFiles to compute relative paths with path.relative(this.bmadDir, filePath), validate that the result does not start with '..' (reject or log and skip files not under this.bmadDir), normalize separators with path.posix.join/replace as needed, and derive module from the validated relative path parts; ensure calculateFileHash surfaces errors (throw or log and skip) instead of returning empty string so failures are not silent; and fix CSV output for the manifest by RFC 4180 escaping—wrap fields containing commas/newlines/quotes in double quotes and double any internal double-quote characters (e.g., turn " into "") so readFilesManifest can parse returned lines correctly.
🧹 Nitpick comments (1)
tools/cli/installers/lib/core/manifest-generator.js (1)
734-760: Workflow manifest order is nondeterministic.
Relying on filesystem traversal means manifest rows can reorder across runs, creating noisy diffs. Sort by module/name before writing.Proposed fix
- // Write all workflows - for (const [, value] of allWorkflows) { + const sorted = [...allWorkflows.values()].sort( + (a, b) => a.module.localeCompare(b.module) || a.name.localeCompare(b.name) + ); + // Write all workflows + for (const value of sorted) { const row = [escapeCsv(value.name), escapeCsv(value.description), escapeCsv(value.module), escapeCsv(value.path)].join(','); csv += row + '\n'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 734 - 760, The workflow manifest is written in nondeterministic order because writeWorkflowManifest builds allWorkflows from this.workflows and iterates the Map without sorting; change the logic in writeWorkflowManifest so after populating allWorkflows (or instead of a Map) you produce a deterministic array of entries and sort it by module then name (e.g., sort by value.module then value.name) before the loop that writes rows; use the same escapeCsv and row construction but iterate the sorted array so the CSV output is stable across runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 32-34: The cleanForCSV method currently uses a falsy check and
calls .trim() directly, which incorrectly drops valid values like 0/false and
can throw for non-string inputs; update cleanForCSV to use a nullish check
(e.g., text == null) and coerce the input to a string before trimming/replacing
(e.g., String(text).trim().replaceAll(...)) so 0/false are preserved and
non-strings are handled safely; locate the cleanForCSV function to apply this
change.
---
Outside diff comments:
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 661-667: The code currently calls content.trim() after reading
csvPath which can remove significant leading/trailing whitespace and drop
trailing empty fields/rows; instead, stop trimming the entire payload: after
fs.readFile(csvPath, 'utf8') check if content === '' (or content.length === 0)
and return [] for an empty file, then split the raw content with
content.split('\n') into lines and keep the existing lines.length < 2 guard (or
adjust it to account for a single-header-with-empty-row case) so quoted-field
whitespace and trailing empty fields are preserved.
- Around line 1035-1038: The CSV writer loop that builds the manifest (the
for...of over allFiles producing csv variable) writes raw fields and breaks when
names/paths contain commas or quotes; update the loop to escape each field the
same way as other manifests by replacing any " characters with "" and wrapping
fields in double quotes (handle null/undefined by converting to empty string)
before concatenation, using the existing allFiles entries (file.type, file.name,
file.module, file.path, file.hash) so the resulting line is a valid CSV.
- Around line 708-728: The upgradeRowToSchema function currently wraps values
with double quotes but does not escape internal quotes, causing malformed CSV;
update upgradeRowToSchema so that before quoting each value (in the map over
upgradedValues) you replace any existing " characters with "" (CSV doubling) and
then wrap the result in quotes; ensure you apply this transformation to every
value produced (including defaults and empty strings) so preserved rows are
emitted with internal quotes escaped.
- Around line 213-216: The installPath assignment in manifest-generator.js can
produce double slashes when relativePath is empty; update the logic where
installPath is computed (the ternary that uses moduleName, this.bmadFolderName,
relativePath, and entry.name) to join path segments while filtering out empty
segments (or branch on empty relativePath) so you never insert an extra "/"
before entry.name; ensure both the 'core' and non-core branches use the same
safe join approach to avoid paths like ".../workflows//workflow.yaml".
- Around line 970-975: The calculateFileHash function currently swallows all
errors (catch { return '' }) making IO failures invisible; change the catch to
capture the error (e.g. catch (err)) and either log a clear warning including
the filePath and err (using the module's logger or console.warn) before
returning a sentinel (or keep returning '' if callers expect that), or rethrow
the error so callers can handle it — ensure the updated catch references
calculateFileHash and filePath so the log message includes which file failed and
the actual error details from fs.readFile.
- Around line 669-682: The current manual CSV parsing using
header/headerColumns/oldColumns and the regex in the loop over dataRows is
brittle and breaks on escaped quotes; replace the manual split+regex logic with
the same csv-parse/sync call used in writeAgentManifest() and
writeTaskManifest(): parse the full CSV content with csv.parse(content, {
columns: true, skip_empty_lines: true }) to get records, then build
preservedRows by selecting the preserved module rows from those parsed records
(mapping back to the original column order using oldColumns if needed) instead
of using the regex-based columns/cleanColumns extraction.
- Around line 992-1001: The manifest generator currently uses unsafe string
replaces on file paths and broken CSV quoting; update the loop that iterates
this.allInstalledFiles to compute relative paths with
path.relative(this.bmadDir, filePath), validate that the result does not start
with '..' (reject or log and skip files not under this.bmadDir), normalize
separators with path.posix.join/replace as needed, and derive module from the
validated relative path parts; ensure calculateFileHash surfaces errors (throw
or log and skip) instead of returning empty string so failures are not silent;
and fix CSV output for the manifest by RFC 4180 escaping—wrap fields containing
commas/newlines/quotes in double quotes and double any internal double-quote
characters (e.g., turn " into "") so readFilesManifest can parse returned lines
correctly.
---
Nitpick comments:
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 734-760: The workflow manifest is written in nondeterministic
order because writeWorkflowManifest builds allWorkflows from this.workflows and
iterates the Map without sorting; change the logic in writeWorkflowManifest so
after populating allWorkflows (or instead of a Map) you produce a deterministic
array of entries and sort it by module then name (e.g., sort by value.module
then value.name) before the loop that writes rows; use the same escapeCsv and
row construction but iterate the sorted array so the CSV output is stable across
runs.
| cleanForCSV(text) { | ||
| if (!text) return ''; | ||
| return text | ||
| .trim() | ||
| .replaceAll(/\s+/g, ' ') // Normalize all whitespace (including newlines) to single space | ||
| .replaceAll('"', '""'); // Escape quotes for CSV | ||
| return text.trim().replaceAll(/\s+/g, ' '); // Normalize all whitespace (including newlines) to single space |
There was a problem hiding this comment.
Falsy check drops valid values and .trim() can throw.
if (!text) discards legitimate 0/false, and .trim() will throw on non-string values parsed from YAML/XML. Use nullish checks and coerce to string.
Proposed fix
- if (!text) return '';
- return text.trim().replaceAll(/\s+/g, ' '); // Normalize all whitespace (including newlines) to single space
+ if (text === null || text === undefined) return '';
+ return String(text).trim().replaceAll(/\s+/g, ' '); // Normalize all whitespace (including newlines) to single space📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cleanForCSV(text) { | |
| if (!text) return ''; | |
| return text | |
| .trim() | |
| .replaceAll(/\s+/g, ' ') // Normalize all whitespace (including newlines) to single space | |
| .replaceAll('"', '""'); // Escape quotes for CSV | |
| return text.trim().replaceAll(/\s+/g, ' '); // Normalize all whitespace (including newlines) to single space | |
| cleanForCSV(text) { | |
| if (text === null || text === undefined) return ''; | |
| return String(text).trim().replaceAll(/\s+/g, ' '); // Normalize all whitespace (including newlines) to single space |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 32 - 34,
The cleanForCSV method currently uses a falsy check and calls .trim() directly,
which incorrectly drops valid values like 0/false and can throw for non-string
inputs; update cleanForCSV to use a nullish check (e.g., text == null) and
coerce the input to a string before trimming/replacing (e.g.,
String(text).trim().replaceAll(...)) so 0/false are preserved and non-strings
are handled safely; locate the cleanForCSV function to apply this change.
There was a problem hiding this comment.
Dismiss. This is pre-existing code that wasn't changed by this PR — the if (!text) guard and .trim() call were already there before. The only thing this PR removed was the .replaceAll('"', '""') line. Beyond that, the function only processes description, capabilities, role, etc. — these
are always strings from YAML parsing or regex captures, never 0 or false. It's a theoretical concern on code we didn't touch
There was a problem hiding this comment.
@pbean, understood — you're right that this is pre-existing code outside the scope of your change. I'll focus on the actual modifications in this PR. Thank you for the clarification.
🧠 Learnings used
Learnt from: wsmoak
Repo: bmad-code-org/BMAD-METHOD PR: 1729
File: tools/cli/installers/lib/ide/codex.js:265-265
Timestamp: 2026-02-21T13:18:49.398Z
Learning: In tools/cli/installers/lib/ide/codex.js transformToSkillFormat method, the description field is required in the Agent Skills format. Empty strings should be replaced with the default value `${skillName} skill`, so the logical OR operator (||) is intentionally used instead of nullish coalescing (??).
275f81a to
0090f15
Compare
pbean
changed the title
…line cleanForCSV() pre-escaped " to "" before storing in memory, then escapeCsv() escaped again at CSV write time. After csv-parse round-trip (which only un-escapes once), descriptions retained doubled quotes instead of originals, corrupting generated output files. Fix: remove the redundant quote escaping from cleanForCSV() since escapeCsv() already handles CSV quoting correctly at write time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pbean
force-pushed
the
escape-fix-cis
branch
from
0090f15 to
f09dcd4
Compare
pbean
changed the title
…templates
Replace triple-quoted """{{description}}""" with single-quoted '{{description}}'
to avoid TOML escaping issues in Gemini workflow templates.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…line (bmad-code-org#1746) * fix(installer): remove double-escaping of quotes in CSV manifest pipeline cleanForCSV() pre-escaped " to "" before storing in memory, then escapeCsv() escaped again at CSV write time. After csv-parse round-trip (which only un-escapes once), descriptions retained doubled quotes instead of originals, corrupting generated output files. Fix: remove the redundant quote escaping from cleanForCSV() since escapeCsv() already handles CSV quoting correctly at write time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(installer): use single quotes for description in Gemini workflow templates Replace triple-quoted """{{description}}""" with single-quoted '{{description}}' to avoid TOML escaping issues in Gemini workflow templates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
cleanForCSV()pre-escaped"to""before storing descriptions in memory, thenescapeCsv()escaped again when writing to CSVcsv-parseround-trip (which only un-escapes once), descriptions retained doubled quotes""instead of originals, corrupting generated output files.replaceAll('"', '""')fromcleanForCSV()—escapeCsv()already handles CSV quoting correctly at write time"""{{description}}"""with single-quoted'{{description}}'in Gemini workflow TOML templates to avoid TOML escaping issuesTest plan
🤖 Generated with Claude Code