fix(cli): scan/diff --pr stamp the PR number in the diff manifest (#668) - #675
Merged
Merged
Conversation
The scan path's prepareDiffManifest never fired its PR branch (the modeDecision struct carried only Base/Scope — the PR number that drove FetchPR was dropped at the struct boundary), so scan --pr and diff --pr emitted diff_manifest.json with no pr_number while parse --pr (which sets it via resolveStepDiffOpts) stamped it correctly. Downstream: reporter.py's PR banner never printed and pipeline_output.json.diff.pr_number was null. Fix: modeDecision carries PR; scan.go passes it to manifestOpts. The manifest's PR branch now fires on the scan path exactly as it does on the parse path — one field, one assignment. Fixes #668 Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
gadievron
requested review from
dgeyshis,
shahar-davidson and
sounil
as code owners
September 22, 2026 00:57
Collaborator
Author
|
Status: ON HOLD — do not merge. The adversarial review (2026-09-22) found defects in this PR that need rewriting before it can merge. The specific finding is in the review record. The PR is being rewritten; the current branch and all evidence are preserved. Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d |
added 2 commits
September 22, 2026 14:16
…668) Rewrite of the previous attempt (which set BOTH opts.pr and opts.base — triggering the mutual-exclusivity validation and making scan --pr exit 2). The correct fix: modeDecision carries PR (the resolved number); manifestOpts carries it as prNumber — a STAMP-ONLY field excluded from validate()/isSet() (which mean 'the user requested a fetch'), and excluded from the fetch branch (which re-fetches). BuildManifest receives firstNonZero(opts.pr, opts.prNumber): the user-facing --pr sets opts.pr (the fetch path), the resolved decision sets prNumber (the stamp path). Fixes #668 Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
Collaborator
Author
|
Hold lifted at 9f795ac — with a body correction from the verification pass: the shipped fix is a stamp-only |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
openant scan --pr Nandopenant diff --pr Nnow emitpr_number: Nindiff_manifest.json, matchingopenant parse --pr Nwhich already did. Closes #668.The bug
selectModeresolved the PR but returned amodeDecisioncarrying onlyBase/Scope— the PR number was dropped at the struct boundary.scan.go'smanifestOptsnever setpr, soprepareDiffManifest's PR branch never fired on the scan path. Downstream:reporter.py's PR banner never printed andpipeline_output.json.diff.pr_numberwas null.The fix
Two lines:
modeDecisiongainsPR int;scan.gopasses it tomanifestOpts. Theomitemptyon the JSON field keeps non-PR runs unchanged.Fixes #668
Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
How (corrected 2026-09-22)
The shipped mechanism is a stamp-only
prNumber:diff_shared.gorecords the PR number into the diff manifest at construction (:94-98) andscan.go:242threads it — no second fetch, nomanifestOpts.prre-resolution. (An earlier revision of this body described settingmanifestOpts.pr; that was not the shipped fix.)