pinact: fix SARIF fixes corrupting lines on apply - #1154
Merged
Conversation
pinact emits each fix's deletedRegion with only startLine (no endLine or columns). Trunk's fix applier reads that as a zero-width insertion at column 1, so it prepends the pinned `uses:` and never deletes the original line — concatenating both onto one line (the leftover falls past the `#` into a comment). Every pinned action was mangled on apply. Normalize pinact's SARIF in pinact_run.py before emitting it: widen each fix's deletedRegion to span the whole original line (startColumn 1 -> endColumn len+1, endLine = startLine), matching the fully-specified region convention the ruff/sqlfluff converters already rely on. No-op for any region pinact fully specifies. Add a regression test that applies a real fix (the existing snapshot tests never do — runCheck forces --no-fix) and asserts the line is pinned to a SHA without the concatenation corruption. Fails without the fix. Co-authored-by: Cursor <cursoragent@cursor.com>
The end-to-end fix test is token-gated (pinact only pins online, resolving tags to SHAs via the GitHub API), so it is skipped in CI without a token. Add offline tests that drive normalize_fix_regions directly via python3 — covering the widen case plus the two guards (already-specified region, out-of-range line) — so the invariant is locked on every run with no token or network. Also assert the online fix leaves exactly one `uses:` (the corruption doubled it). Co-authored-by: Cursor <cursoragent@cursor.com>
Only backfill what a deletedRegion is missing instead of overwriting it with a full-line span: preserve an explicit startColumn/endLine and infer endColumn to the end of the region's end line. Skip regions that already carry an endColumn or an offset-based span. Adds partial-region tests. Co-authored-by: Cursor <cursoragent@cursor.com>
CI's eslint-plugin-jest flags every expect() inside a conditionalTest() callback as jest/no-standalone-expect, since the rule doesn't know the repo's it()/it.skip() wrapper. Register it via additionalTestBlockFunctions so expects in conditionalTest bodies are valid. Co-authored-by: Cursor <cursoragent@cursor.com>
|
😎 Merged manually by @EliSchleifer - details. |
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.
Summary
The
pinactlinter's autofix corrupts everyuses:line it pins. Runningtrunk check --fix(or fmt-on-save) turns:into:
The pinned ref is written, but the original
uses: …@v4is left dangling on the same line (falling past the#into a comment). Every pinned action is affected.Root cause
The plugin passes pinact's SARIF straight to Trunk's fix applier. pinact describes each fix with a line-only region:
Trunk reads a
deletedRegionwith noendLine/columns as a zero-width insertion point at column 1, so it inserts the pinned line and never deletes the original — concatenating both. Theruff/sqlfluff/etc. converters in this repo don't hit this because they always emit a fully-specified region (startLine+startColumn+endLine+endColumn).Fix
pinact_run.pynow post-processes pinact's SARIF before emitting it (normalize_fix_regions): each fix'sdeletedRegionis widened to span the whole original line (startColumn: 1→endColumn: len(line)+1,endLine = startLine), matching the convention Trunk applies correctly. It's a no-op for any region pinact ever fully specifies (guards onendColumn/endLine/charLength/charOffset).Coverage gap this exposes
None of the existing snapshot tests ever applied a pinact fix — the driver's
runCheckforces-n(--no-fix), so every snapshot only covered parse-errors. Added a regression test that applies a real fix and asserts the line is pinned to a SHA without the concatenation corruption. Verified it fails without thepinact_run.pychange and passes with it. It's online-gated (a resolvable SHA is required) and version-independent (does not snapshot the volatile SHA).Test plan
trunk fmt+trunk checkon both changed files — no new issuesuses:lines — 0 mangled, byte-for-byte identical to a directpinact runMade with Cursor