also looking at this
fix(deps-file): anchor the dependency regex to a key, and rewrite each match from its own captures
#1454
3 files
+216
-41
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -125,6 +125,22 @@ async function parseSimpleYamlDependencies(content: string, filePath: string): P | ||
| 125 | 125 | return dependencies |
| 126 | 126 | } |
| 127 | 127 | |
| 128 | /** | |
| 129 | * Does this content parse as YAML? | |
| 130 | * | |
| 131 | * Used as a guard around rewrites rather than as a parser: the result is only | |
| 132 | * ever compared before and after, so a file that never parsed is not penalised. | |
| 133 | */ | |
| 134 | function parsesAsYaml(content: string): boolean { | |
| 135 | try { | |
| 136 | Bun.YAML.parse(content) | |
| 137 | return true | |
| 138 | } | |
| 139 | catch { | |
| 140 | return false | |
| 141 | } | |
| 142 | } | |
| 143 | ||
| 128 | 144 | /** |
| 129 | 145 | * Update dependency file content with new package versions |
| 130 | 146 | */ |
| @@ -148,44 +164,53 @@ export async function updateDependencyFile(filePath: string, content: string, up | ||
| 148 | 164 | for (const update of updates) { |
| 149 | 165 | // Clean package name (remove dependency type info like "(dev)") |
| 150 | 166 | const cleanPackageName = update.name.replace(/\s*\(dev\)$/, '').replace(/\s*\(peer\)$/, '').replace(/\s*\(optional\)$/, '') |
| 151 | ||
| 152 | // Create regex to find the package line and update its version | |
| 153 | // Handle various YAML formats: "package: version", "package:version", " package: ^version" | |
| 154 | // Use word boundaries to prevent partial matches (e.g., "zip" matching "unzip") | |
| 155 | // Capture: (1) package name and colon, (2) version part, (3) optional comment part | |
| 167 | const escapedName = cleanPackageName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') | |
| 168 | ||
| 169 | // Anchor to a key at the start of a line. | |
| 170 | // | |
| 171 | // The previous pattern was `(\s*\bNAME\b\s*:\s*)(...)`, which matched the | |
| 172 | // package name anywhere in the file - including inside a prose comment - | |
| 173 | // and whose leading `\s*` reached back across the newline before the real | |
| 174 | // key. Both halves were needed to corrupt a file, and both are gone: `^` | |
| 175 | // with the `m` flag pins the match to a key position, and the indent is | |
| 176 | // `[ \t]*`, which cannot cross a line. A commented-out key (`# redis: ^7`) | |
| 177 | // and a quoted neighbour (`"@types/node"` when updating `node`) no longer | |
| 178 | // match either, because neither starts with the name. stacksjs/buddy#1453 | |
| 179 | // | |
| 180 | // Deliberately not anchored at the end, so a CRLF file still matches. | |
| 181 | // | |
| 182 | // Groups: 1 indent, 2 key including any quotes, 3 colon and its spacing, | |
| 183 | // 4 quote around the value, 5 version, 6 inline comment. | |
| 156 | 184 | const packageRegex = new RegExp( |
| 157 | `(\\s*\\b${cleanPackageName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b\\s*:\\s*)([^\\s#\\n\\r]+)(\\s*#.*)?`, | |
| 158 | 'g', | |
| 185 | `^([ \\t]*)("${escapedName}"|'${escapedName}'|${escapedName})([ \\t]*:[ \\t]*)(["']?)([^\\s#"']+)\\4([ \\t]*#[^\\r\\n]*)?`, | |
| 186 | 'gm', | |
| 159 | 187 | ) |
| 160 | 188 | |
| 161 | // Extract the original version prefix (^, ~, >=, etc.) or lack thereof | |
| 162 | const currentMatch = updatedContent.match(packageRegex) | |
| 163 | if (currentMatch) { | |
| 164 | const fullMatch = currentMatch[0] | |
| 165 | // Parse the match into parts: package+colon, version, comment | |
| 166 | const matchParts = fullMatch.match(new RegExp( | |
| 167 | `(\\s*\\b${cleanPackageName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b\\s*:\\s*)([^\\s#\\n\\r]+)(\\s*#.*)?`, | |
| 168 | )) | |
| 169 | ||
| 170 | if (matchParts) { | |
| 171 | const packageAndColon = matchParts[1] // " package: " | |
| 172 | const currentVersionInFile = matchParts[2] // "^1.0.0" | |
| 173 | const commentPart = matchParts[3] || '' // " # comment" or empty | |
| 174 | ||
| 189 | // A replacer function, so every match is rewritten from its own captures. | |
| 190 | // | |
| 191 | // The previous code read the first match, built one replacement string | |
| 192 | // from it, and handed that literal to `String.replace` with a `g` regex - | |
| 193 | // writing the first occurrence's version over every other occurrence. | |
| 194 | updatedContent = updatedContent.replace( | |
| 195 | packageRegex, | |
| 196 | ( | |
| 197 | fullMatch: string, | |
| 198 | indent: string, | |
| 199 | key: string, | |
| 200 | separator: string, | |
| 201 | quote: string, | |
| 202 | currentVersionInFile: string, | |
| 203 | commentPart?: string, | |
| 204 | ) => { | |
| 175 | 205 | // Check if current version should be respected (like "*", "latest", etc.) |
| 176 | const shouldRespectVersion = (version: string): boolean => { | |
| 177 | const dynamicIndicators = ['latest', '*', 'main', 'master', 'develop', 'dev'] | |
| 178 | const cleanVersion = version.toLowerCase().trim() | |
| 179 | return dynamicIndicators.includes(cleanVersion) | |
| 180 | } | |
| 181 | ||
| 182 | if (shouldRespectVersion(currentVersionInFile)) { | |
| 206 | const dynamicIndicators = ['latest', '*', 'main', 'master', 'develop', 'dev'] | |
| 207 | if (dynamicIndicators.includes(currentVersionInFile.toLowerCase().trim())) { | |
| 183 | 208 | getDefaultLogger().info(`⚠️ Skipping update for ${cleanPackageName} - version "${currentVersionInFile}" should be respected`) |
| 184 | continue | |
| 209 | return fullMatch | |
| 185 | 210 | } |
| 186 | 211 | |
| 187 | const versionPrefixMatch = currentVersionInFile.match(/^(\D*)/) | |
| 188 | const originalPrefix = versionPrefixMatch ? versionPrefixMatch[1] : '' | |
| 212 | // Extract the original version prefix (^, ~, >=, etc.) or lack thereof | |
| 213 | const originalPrefix = currentVersionInFile.match(/^(\D*)/)?.[1] ?? '' | |
| 189 | 214 | |
| 190 | 215 | // Check if newVersion already has a prefix (to avoid double prefixes) |
| 191 | 216 | const newVersionHasPrefix = /^[\^~>=<]/.test(update.newVersion) |
| @@ -193,11 +218,20 @@ export async function updateDependencyFile(filePath: string, content: string, up | ||
| 193 | 218 | // Use newVersion as-is if it already has a prefix, otherwise preserve original prefix |
| 194 | 219 | const finalVersion = newVersionHasPrefix ? update.newVersion : `${originalPrefix}${update.newVersion}` |
| 195 | 220 | |
| 196 | // Replace with: package+colon + new version + preserved comment | |
| 197 | const replacement = `${packageAndColon}${finalVersion}${commentPart}` | |
| 198 | updatedContent = updatedContent.replace(packageRegex, replacement) | |
| 199 | } | |
| 200 | } | |
| 221 | return `${indent}${key}${separator}${quote}${finalVersion}${quote}${commentPart ?? ''}` | |
| 222 | }, | |
| 223 | ) | |
| 224 | } | |
| 225 | ||
| 226 | // A corrupted write is worse than a missed update. These files are committed | |
| 227 | // by a bot, and a deps.yaml that no longer parses fails `Setup Pantry` before | |
| 228 | // any job in the workflow runs - so nothing downstream is left to catch it. | |
| 229 | // | |
| 230 | // Only bail when this function is what broke it: a file that already did not | |
| 231 | // parse is left to whatever it was doing before. stacksjs/buddy#1453 | |
| 232 | if (updatedContent !== content && parsesAsYaml(content) && !parsesAsYaml(updatedContent)) { | |
| 233 | getDefaultLogger().warn(`⚠️ Skipping updates for ${filePath}: the rewrite no longer parses as YAML, so the file is left unchanged`) | |
| 234 | return content | |
| 201 | 235 | } |
| 202 | 236 | |
| 203 | 237 | return updatedContent |
| @@ -192,6 +192,140 @@ dependencies: | ||
| 192 | 192 | }) |
| 193 | 193 | }) |
| 194 | 194 | |
| 195 | describe('comment and multi-occurrence corruption (stacksjs/buddy#1453)', () => { | |
| 196 | const mk = (name: string, currentVersion: string, newVersion: string): PackageUpdate => ({ | |
| 197 | name, | |
| 198 | currentVersion, | |
| 199 | newVersion, | |
| 200 | updateType: 'minor', | |
| 201 | dependencyType: 'dependencies', | |
| 202 | file: 'deps.yaml', | |
| 203 | metadata: undefined, | |
| 204 | }) | |
| 205 | ||
| 206 | it('should not take its replacement text from a package name inside a comment', async () => { | |
| 207 | // Verbatim from the PR that produced a deps.yaml which no longer parsed: | |
| 208 | // the comment mentions `sqlite: latest`, and because the old regex was | |
| 209 | // unanchored that prose was the FIRST match, so its captures drove the | |
| 210 | // replacement for the real key - and its greedy leading `\s*` swallowed | |
| 211 | // the newline, collapsing `sqlite` onto the `postgres` line. | |
| 212 | const content = `# That is not hypothetical: \`sqlite: latest\` was fine only because the SDK did | |
| 213 | # not support sqlite.org and skipped it with a warning. | |
| 214 | dependencies: | |
| 215 | bun: ^1.3.14 | |
| 216 | mysql: ^8.0.45 | |
| 217 | postgres: ^18.0.0 | |
| 218 | sqlite: ^3.52.0 | |
| 219 | ` | |
| 220 | ||
| 221 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 222 | mk('postgres', '^18.0.0', '18.6'), | |
| 223 | mk('sqlite', '^3.52.0', '3.53.4'), | |
| 224 | ]) | |
| 225 | ||
| 226 | // The whole point: the file still parses. | |
| 227 | expect(() => Bun.YAML.parse(result)).not.toThrow() | |
| 228 | ||
| 229 | // Both keys survive, on their own lines, with their own versions. | |
| 230 | expect(result).toMatch(/^ {2}postgres: \^18\.6$/m) | |
| 231 | expect(result).toMatch(/^ {2}sqlite: \^3\.53\.4$/m) | |
| 232 | ||
| 233 | // The comment is prose and must come out byte-identical. | |
| 234 | expect(result).toContain('# That is not hypothetical: `sqlite: latest` was fine only because the SDK did') | |
| 235 | }) | |
| 236 | ||
| 237 | it('should rewrite each occurrence from its own captures, not the first match', async () => { | |
| 238 | // A `g` regex was used with a single replacement string built from the | |
| 239 | // first match, so the first occurrence's rendered text was written over | |
| 240 | // every other occurrence. Two occurrences with DIFFERENT prefixes are | |
| 241 | // what exposes it: with the same prefix the wrong answer and the right | |
| 242 | // answer are identical. | |
| 243 | const content = `dependencies: | |
| 244 | typescript: ^5.0.0 | |
| 245 | ||
| 246 | devDependencies: | |
| 247 | typescript: ~4.9.0 | |
| 248 | ` | |
| 249 | ||
| 250 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 251 | mk('typescript', '^5.0.0', '5.6.2'), | |
| 252 | ]) | |
| 253 | ||
| 254 | expect(() => Bun.YAML.parse(result)).not.toThrow() | |
| 255 | // Each line keeps the prefix it was written with. | |
| 256 | expect(result).toMatch(/^ {2}typescript: \^5\.6\.2$/m) | |
| 257 | expect(result).toMatch(/^ {2}typescript: ~5\.6\.2$/m) | |
| 258 | }) | |
| 259 | ||
| 260 | it('should leave a commented-out dependency alone', async () => { | |
| 261 | const content = `dependencies: | |
| 262 | # redis: ^7.4.1 # disabled for now | |
| 263 | sqlite3: ^3.50.3 | |
| 264 | ` | |
| 265 | ||
| 266 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 267 | mk('redis', '^7.4.1', '7.5.0'), | |
| 268 | ]) | |
| 269 | ||
| 270 | expect(result).toContain('# redis: ^7.4.1 # disabled for now') | |
| 271 | expect(result).not.toContain('7.5.0') | |
| 272 | }) | |
| 273 | ||
| 274 | it('should respect a dynamic version even when it is quoted', async () => { | |
| 275 | // `"*"` captured with its quotes missed the `['latest', '*', ...]` test, | |
| 276 | // so the guard never fired and the prefix logic produced `"*"3.13.5`. | |
| 277 | const content = `dependencies: | |
| 278 | python.org: "*" | |
| 279 | node: "latest" | |
| 280 | ` | |
| 281 | ||
| 282 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 283 | mk('python.org', '*', '3.13.5'), | |
| 284 | mk('node', 'latest', '22.17.1'), | |
| 285 | ]) | |
| 286 | ||
| 287 | expect(() => Bun.YAML.parse(result)).not.toThrow() | |
| 288 | expect(result).toContain('python.org: "*"') | |
| 289 | expect(result).toContain('node: "latest"') | |
| 290 | expect(result).not.toContain('3.13.5') | |
| 291 | expect(result).not.toContain('22.17.1') | |
| 292 | }) | |
| 293 | ||
| 294 | it('should preserve the quoting style of a version it does update', async () => { | |
| 295 | const content = `dependencies: | |
| 296 | typescript: "^5.0.0" | |
| 297 | node: '^22.12.0' | |
| 298 | ` | |
| 299 | ||
| 300 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 301 | mk('typescript', '^5.0.0', '5.6.2'), | |
| 302 | mk('node', '^22.12.0', '22.17.1'), | |
| 303 | ]) | |
| 304 | ||
| 305 | expect(() => Bun.YAML.parse(result)).not.toThrow() | |
| 306 | expect(result).toContain('typescript: "^5.6.2"') | |
| 307 | expect(result).toContain('node: \'^22.17.1\'') | |
| 308 | }) | |
| 309 | ||
| 310 | it('should still update a file that did not parse as YAML to begin with', async () => { | |
| 311 | // The parse guard exists to stop THIS function corrupting a file. A file | |
| 312 | // that was already broken is not its business, and bailing there would | |
| 313 | // silently stop updating it. | |
| 314 | const content = `dependencies: | |
| 315 | bun: ^1.3.14 | |
| 316 | : this line is not valid yaml | |
| 317 | ` | |
| 318 | ||
| 319 | expect(() => Bun.YAML.parse(content)).toThrow() | |
| 320 | ||
| 321 | const result = await updateDependencyFile('deps.yaml', content, [ | |
| 322 | mk('bun', '^1.3.14', '1.4.2'), | |
| 323 | ]) | |
| 324 | ||
| 325 | expect(result).toContain('bun: ^1.4.2') | |
| 326 | }) | |
| 327 | }) | |
| 328 | ||
| 195 | 329 | describe('YAML comment preservation', () => { |
| 196 | 330 | it('should preserve inline comments when updating dependencies', async () => { |
| 197 | 331 | const content = `dependencies: |
| @@ -126,13 +126,20 @@ devDependencies: | ||
| 126 | 126 | // Pass the content directly to avoid file system corruption |
| 127 | 127 | const updatedContent = await updateDependencyFile(depsPath, depsYaml, updates) |
| 128 | 128 | |
| 129 | // python.org should not be updated because it uses "*" | |
| 129 | // The result has to be YAML before any of the assertions below mean | |
| 130 | // anything. Without this, `toContain('python.org: "*"')` was satisfied by | |
| 131 | // the corrupt line `python.org: "*"3.13.5`, so this test passed for months | |
| 132 | // against output that `Bun.YAML.parse` rejects (stacksjs/buddy#1453). | |
| 133 | expect(() => Bun.YAML.parse(updatedContent)).not.toThrow() | |
| 134 | ||
| 135 | // python.org should not be updated because it uses "*", quoted or not | |
| 130 | 136 | expect(updatedContent).toContain('python.org: "*"') |
| 131 | expect(updatedContent).not.toContain('python.org: "3.13.5"') | |
| 137 | expect(updatedContent).not.toContain('3.13.5') | |
| 132 | 138 | |
| 133 | // typescript should be updated because it uses a specific version | |
| 134 | expect(updatedContent).toContain('typescript: ^5.1.0') | |
| 135 | expect(updatedContent).not.toContain('typescript: ^5.0.0') | |
| 139 | // typescript should be updated because it uses a specific version, and the | |
| 140 | // author's quoting survives the rewrite | |
| 141 | expect(updatedContent).toContain('typescript: "^5.1.0"') | |
| 142 | expect(updatedContent).not.toContain('5.0.0') | |
| 136 | 143 | }) |
| 137 | 144 | |
| 138 | 145 | it('should test CLI respectLatest flag override', async () => { |