Repository navigation
Conversation
Reviewer's GuideThe PR replaces the two remediation execution modes with one edit-site-aware pipeline that returns conflict-resolved groups and isolated content, while preserving a flat remediation view and strengthening behavior across Maven, TOML, exclusions, transitive dependencies, and multi-manifest workspaces. Sequence diagram for conflict-aware remediation orchestrationsequenceDiagram
participant Caller
participant runRemediation
participant Extractor as extractRemediations
participant Updater
participant Resolver as conflictResolution
participant Filesystem
Caller->>runRemediation: runRemediation(targetPath, options)
runRemediation->>Extractor: extractRemediations(manifest)
Extractor-->>runRemediation: remediations
loop Each remediation
runRemediation->>Updater: updater(originalContent, dependency fix)
Updater-->>runRemediation: isolated content and applied edit
runRemediation->>Updater: changeKey(manifestPath, applied edit)
end
runRemediation->>Resolver: resolve versions within each changeKey group
Resolver-->>runRemediation: appliedVersion
runRemediation->>Updater: updater(originalContent, grouped fixes)
Updater-->>runRemediation: isolated group content
opt Not dry run
runRemediation->>Updater: updater(originalContent, all grouped fixes)
Updater-->>runRemediation: combined content
runRemediation->>Filesystem: writeFileSync(manifestPath, combined content)
end
runRemediation-->>Caller: groups and flat remediations
Flow diagram for edit-site grouping and conflict resolutionflowchart TD
A[Extract remediations] --> B[Run updater once per remediation]
B --> C{Updater located dependency?}
C -- No --> D[Drop unlocatable remediation]
C -- Yes --> E[Derive changeKey]
E --> F{Excluded dependency shares edit site?}
F -- Yes --> G[Record skipped remediation]
F -- No --> H[Group by changeKey]
H --> I{Group has conflicting versions?}
I -- No --> J[Use shared target version]
I -- Yes --> K[Resolve with conflictResolution]
J --> L[Produce group content and divergent flag]
K --> L
L --> M[Flatten groups into remediations]
M --> N{dryRun?}
N -- Yes --> O[Return groups without writing]
N -- No --> P[Apply combined changes and write manifest]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/remediate.js" line_range="216" />
<code_context>
+ if (resolve) {
+ return resolve(versions)
+ }
+ return versions.sort((a, b) => compareVersions(b, a))[0]
+}
+
</code_context>
<issue_to_address>
**Prerelease fixes are selected over stable releases**
When a shared edit site has both a prerelease fix and its corresponding stable release, `resolveConflict` sorts candidates with `compareVersions`, whose lexical comparison of qualified dot-separated segments ranks the prerelease above the stable release. It applies the prerelease version to the manifest, so users see the prerelease reported as the selected fix.
Use version ordering that ranks a prerelease below its corresponding stable release.
</issue_to_address>
### Comment 2
<location path="src/remediate.js" line_range="394" />
<code_context>
- fs.writeFileSync(manifestPath, result.content, 'utf-8')
+ const combined = manifestType.updater(originalContent, allChanges)
+ if (combined.applied.length > 0) {
+ fs.writeFileSync(manifestPath, combined.content, 'utf-8')
appliedFiles.push(manifestPath)
}
</code_context>
<issue_to_address>
**Failed runs leave partial manifest updates**
When a later analysis request or conflict resolver throws after an earlier manifest has been updated, `runRemediation` writes earlier manifests before processing all manifests succeeds, then rejects without returning `appliedFiles`. The workspace is partially updated and the caller does not know which files changed.
Defer manifest writes until all analysis and conflict resolution for the run succeeds.
Also at `src/remediate.js:373`.
</issue_to_address>
### Comment 3
<location path="src/remediate.js" line_range="394" />
<code_context>
- fs.writeFileSync(manifestPath, result.content, 'utf-8')
+ const combined = manifestType.updater(originalContent, allChanges)
+ if (combined.applied.length > 0) {
+ fs.writeFileSync(manifestPath, combined.content, 'utf-8')
appliedFiles.push(manifestPath)
}
</code_context>
<issue_to_address>
**Concurrent runs lose updates**
When two runs target the same manifest and both scan before either writes, each run reads the manifest at `src/remediate.js:315`, then writes a full-file result at `src/remediate.js:394`; the last writer overwrites edits made by the other run, so one remediation disappears.
Serialize writes per manifest or re-read and merge/check the file before writing.
</issue_to_address>
### Comment 4
<location path="src/remediate.js" line_range="383" />
<code_context>
+ allGroups.push({
+ changeKey,
+ appliedVersion,
+ remediations: members,
+ content: { path: manifestPath, after: result.content },
+ divergent: members.some(r => r.fixedInVersion !== appliedVersion),
</code_context>
<issue_to_address>
**Reports show versions that were not applied**
When a conflict resolver selects a version different from one or more member remediations, `runRemediation` retains each member's original `fixedInVersion` while recording the selected `appliedVersion` on the group. The CLI passes the flat remediations to `generateReport`, which prints `fixedInVersion`, so users see targets that were not written to the manifest.
Set each returned remediation's `fixedInVersion` to its group's `appliedVersion` before generating the report.
Also at `src/remediate.js:384`, `src/remediate.js:400`.
</issue_to_address>
### Comment 5
<location path="test/remediate.test.js" line_range="1047" />
<code_context>
const pomPath = path.join(dir, 'pom.xml')
matchStub.returns({ provideStack: stub().resolves({ content: '{}', contentType: 'application/json', ecosystem: 'maven' }) })
- requestStackStub.resolves(buildExcludeReport([COMMONS_TEXT, JACKSON_CORE]))
+ requestStackStub.resolves(buildMultiDepReport([COMMONS_TEXT, JACKSON_CORE]))
const result = await runRemediation(pomPath, { dryRun: true, exclude: ['pkg:maven/org.apache.commons/commons-text'] })
</code_context>
<issue_to_address>
**Percent-encoding remains untested**
When purl component decoding is broken, when purl component decoding is broken, the `percent-encoded purls match decoded exclude patterns` test still passes: its fixture and exclude pattern use unencoded components, so the assertions only exercise case-insensitive type matching.
Use a fixture containing a percent-encoded purl component and an exclude pattern using its decoded form.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and when dependencies share an edit site, the selected conflict version is written into the manifest and may be consumed by later builds; a mistaken selection can therefore persist as a wrong dependency version. The impact is bounded and can be repaired by restoring the manifest or rerunning remediation.
Blocking findings: src/remediate.js:216, src/remediate.js:394, src/remediate.js:394, src/remediate.js:383, test/remediate.test.js:1047
Strum355
changed the title
acd1f0b to
59a4eb4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ruromero
left a comment
ruromero
left a comment
There was a problem hiding this comment.
Requesting changes for two small items; everything else is non-blocking (inline). This follows up on my earlier review. Findings were verified locally against 59a4eb4.
Blocking
- Mark the breaking change. CONTRIBUTING.md says PR titles must follow Conventional Commits.
perDependencyChangesandremediation.changes/DependencyFixshipped inv0.4.0(the currentlateston npm, via #630), so removing them needsfix!:or aBREAKING CHANGE:footer. Releases are bumped by hand, so the main purpose is to make sure whoever cuts the next release picks the right bump and the changelog flags it. The GH action currently passesperDependencyChangesand readsremediation.changes, so it must move together with this (TC-7118). - Add a test for the TC-6891 fix. See the inline comment on
test/remediate.test.js.
No critical correctness issues found. In local probe runs, grouping, conflict resolution, exclude co-location and multi-manifest handling behave as intended. The TOML shared-ref conflict now writes the resolved version, where main overclaims.
| matchStub.returns({ provideStack: stub().resolves({ content: '{}', contentType: 'application/json', ecosystem: 'gradle' }) }) | ||
| requestStackStub.resolves(buildMultiDepReport([ | ||
| { depRef: 'pkg:maven/com.fasterxml.jackson.core/jackson-core@2.14.0', fixedIn: ['pkg:maven/com.fasterxml.jackson.core/jackson-core@2.15.0'], issueId: 'CVE-2020-1000' }, | ||
| { depRef: 'pkg:maven/com.fasterxml.jackson.core/jackson-databind@2.14.0', fixedIn: ['pkg:maven/com.fasterxml.jackson.core/jackson-databind@2.15.0'], issueId: 'CVE-2020-2000' }, |
There was a problem hiding this comment.
Blocking: both deps here target 2.15.0, so this test never reaches the TC-6891 case this PR fixes. Please add a variant with conflicting targets (e.g. 2.15.0 / 2.17.0) that asserts:
group.appliedVersion === '2.17.0'andgroup.divergent === truegroup.content.aftercontainsjackson = "2.17.0"- in apply mode, the file on disk contains
2.17.0
I checked locally: this branch already gets all of these right, and main writes 2.15.0 while reporting both as applied. So the test only locks in behaviour that already works.
| artifactId: remediation.artifactId, | ||
| newVersion: remediation.fixedInVersion, | ||
| }]) | ||
| if (isolated.applied.length === 0) { |
There was a problem hiding this comment.
Transitive deps (not declared in this manifest) are now dropped without a trace. On main, a dry-run whose only finding is a transitive dep returns exit 2 with 1 remediation. On this branch it returns exit 0 with 0, and the CLI report loses those entries.
Keeping them out of remediations fits the "truthful result" goal. Could they go into skipped instead? Both updaters already give a reason in isolated.skipped (e.g. "Dependency not found in pom.xml or has no <version> element", "No library entry found for module …"), so consumers and the CLI would still see them. Either way, the behaviour change is worth calling out in the PR description.
| * sources?: string, | ||
| * backendUrl?: string, | ||
| * perDependencyChanges?: boolean, | ||
| * conflictResolution?: (versions: string[]) => string, |
There was a problem hiding this comment.
TC-7116: "The default should be 'highest'. Consider also supporting 'first' and a function form." Only the function form works today: conflictResolution: 'highest' throws resolve is not a function. Please either accept 'highest' | 'first' | (versions) => string, or at least fail with a clear error when the value isn't a function.
There was a problem hiding this comment.
Intentional deviation from the ticket. We discussed this during implementation and decided string dispatch adds indirection for no real value — the default is highest (no option needed), and any custom policy is a one-liner function like v => v[0]. Accepting strings means maintaining a lookup table and validating unknown strings, which is more code for the same result.
If a consumer passes "highest" today it will be falsy and fall through to the default (highest), so it happens to work — but we are not documenting it as supported.
| }))) | ||
| if (result.applied.length > 0) { | ||
| fs.writeFileSync(manifestPath, result.content, 'utf-8') | ||
| const combined = manifestType.updater(originalContent, allChanges) |
There was a problem hiding this comment.
Question: TC-7116 step 4 is "Produce combined content (all groups applied together) for consumers that want a single atomic write." Here the combined content is only computed when !dryRun, and it's written to disk but never returned. Is that intentional, i.e. apply mode is the only consumer? If a dry-run consumer should be able to get it, consider computing it regardless of dryRun and returning it per manifest (e.g. [{ path, after }]).
There was a problem hiding this comment.
Intentional. We discussed this with the TC-6891 agent who confirmed how the GH action works today: bundle mode runs with dryRun: false and uses the working tree as the combined content (stages changed files via git diff --name-only). Dependency mode also runs with dryRun: false, then discards the working tree and rebuilds branches from group.edit.after.
No consumer needs combined content as a return value — dryRun controls the side effect (disk write), groups is the return data. They are orthogonal. Computing combined content on dry-run would add an extra updater pass for a value nobody reads.
| if (resolve) { | ||
| return resolve(versions) | ||
| } | ||
| return versions.sort((a, b) => compareVersions(b, a))[0] |
There was a problem hiding this comment.
Suggestion (not a regression): compareVersions ranks pre-releases above the release (2.0.0-rc1 > 2.0.0), so with candidates {2.0.0, 2.0.0-rc1} the default policy picks 2.0.0-rc1. The action's compareVersions behaves the same, so parity is preserved. Since this is becoming the canonical policy, consider handling qualifiers, or at least adding a test that documents the current behaviour.
There was a problem hiding this comment.
Added a test documenting the current behavior (compareVersions ranks 2.0.0-rc1 above 2.0.0).
Question for you: does the backend ever emit pre-release versions in fixedIn? If it does not today and is not expected to, this is a theoretical concern we can defer. If it does or might, we should handle qualifiers properly — but that would be a change to compareVersions in remediation.js, which is pre-existing and shared with the version selection strategies. Either way, not a regression in this PR.
|
|
||
| // Dry-run signals "changes available but not written" via exit code 2. | ||
| return { exitCode: dryRun ? 2 : 0, remediations: allRemediations, manifests: manifestPaths, appliedFiles, skipped: allSkipped } | ||
| return { exitCode: dryRun ? 2 : 0, groups: allGroups, remediations: flatRemediations, manifests: manifestPaths, appliedFiles, skipped: allSkipped } |
There was a problem hiding this comment.
nit: the result object { exitCode, groups, remediations, manifests, appliedFiles, skipped } is built in three places (:264, :403 and here). A small helper would keep the three copies from drifting apart.
There was a problem hiding this comment.
The project has tsc set up with JSDoc types — the @returns typedef on runRemediation covers all three return sites, so tsc would catch a missing or misspelled field. Given that, the helper would add indirection without catching anything the type checker does not already catch. Leaving as-is.
The orchestrator had two divergent code paths controlled by a perDependencyChanges flag: one that ran the updater per-dep to discover edit sites, and one that ran it once with everything bundled. Neither detected shared-ref version conflicts — two deps sharing a Maven property or Gradle version.ref could end up with different target versions, silently applying only one while reporting both as fixed. The per-dep path left conflict resolution to the GH action, and the bundle path picked whichever version the iteration order landed on first. Now there is one flow. The orchestrator always discovers edit sites, groups inseparable remediations by changeKey, resolves conflicts (highest version wins by default, overridable), and returns structured groups alongside the existing flat remediations list. Every consumer gets the same truthful, conflict-resolved data regardless of whether it plans to create one PR or many. Implements: https://issues.redhat.com/browse/TC-7116 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous test only exercised case-insensitive type matching. Now uses a percent-encoded exclude pattern (commons%2Dtext) to verify normalizeExcludePattern decodes components before matching. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Rename RemediationGroup.content to .edit (clearer for { path, after })
- Add transitive deps to skipped with reason instead of dropping silently
- Guard conflictResolution output against non-candidate versions
- Add TOML conflicting-versions test (dry-run + apply mode)
- Add prerelease sorting behavior test (documents current semantics)
- Add exclude matcher unit tests (percent-encoding, ** vs *, case)
- Fix percent-encoding test comment (artifactId, not groupId)
- Export normalizeExcludePattern and compileExcludeMatcher for testing
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7e0c53b to
ed50ee7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #681 +/- ##
==========================================
+ Coverage 92.03% 92.06% +0.02%
==========================================
Files 43 43
Lines 11305 11312 +7
Branches 2265 2271 +6
==========================================
+ Hits 10405 10414 +9
+ Misses 900 898 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary
The orchestrator had two divergent code paths: one that ran the updater per-dep to discover edit sites (
perDependencyChanges: true), and one that ran it once with everything bundled. Neither detected shared-ref version conflicts — two deps sharing a Maven${property}or Gradleversion.refcould end up with different target versions, silently applying only one while reporting both as fixed (TC-6891).Now every consumer — CLI, GH action, future tooling — receives the same truthful, conflict-resolved data regardless of how they plan to use it.
Test plan
conflictResolutionfunction overrides default policyremediationsequalsgroups.flatMap(g => g.remediations)Implements: https://issues.redhat.com/browse/TC-7116
🤖 Generated with Claude Code
Summary by Sourcery
Unify remediation orchestration around conflict-aware edit-site groups so all consumers receive consistent, isolated, and truthful remediation results.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests: