Skip to content

Read the two line readers as projections - #376

Merged
iderex merged 1 commit into
masterfrom
scan/the-two-line-readers-are-projections
Sep 7, 2026
Merged

iderex merged 1 commit into
masterfrom
scan/the-two-line-readers-are-projections

Conversation

@iderex

@iderex iderex commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Refs #373, the class cs/linq/missed-select: two alerts, both in the readers ReleaseVersionAgreementTests walks a workflow file with.

Finishes: #373

The two sites

Read from the platform rather than from the issue, at the head this branch starts from:

git rev-parse origin/master
1c877ccaaae7708d9d4ef71068769cdeec11be12

for a in 238 239; do gh api repos/Flowfin/jellyfin-plugin-metadata-sync/code-scanning/alerts/$a --jq '"#\(.number) \(.state) \(.rule.id) \(.most_recent_instance.location.path):\(.most_recent_instance.location.start_line)-\(.most_recent_instance.location.end_line)"'; done
#238 open cs/linq/missed-select Jellyfin.Plugin.MetadataSync.Tests/ReleaseVersionAgreementTests.cs:616-653
#239 open cs/linq/missed-select Jellyfin.Plugin.MetadataSync.Tests/ReleaseVersionAgreementTests.cs:686-702

The first is JobLines, the second is Script. Each is a foreach whose first statement trims the raw line into the variable the rest of the body reads, which is the shape the query asks about.

What changed

The mapping moves into the sequence in both, as a Select over the split lines, and the loop bodies start where they always did.

In Script the filter that drops blank lines and comments moves with it, into a Where, and the reason is on a sibling board rather than in this tree: a loop whose body opens with one test and a continue is what cs/linq/missed-where asks about, and the board that rewrote eleven of these sites as projections had six alerts of that class minted at the same sites by the next scan. So the two rewrites are one change here rather than one and its correction a scan later. The loop holds only the join of continuation lines now.

What proving it found

The suite is what says the rewrite kept the readers' meaning, and it does, on both lines:

dotnet build Jellyfin.Plugin.MetadataSync.sln --nologo
VSTEST_CONNECTION_TIMEOUT=600 DOTNET_CLI_UI_LANGUAGE=en dotnet test Jellyfin.Plugin.MetadataSync.Tests/Jellyfin.Plugin.MetadataSync.Tests.csproj --nologo --no-build -f net9.0
Failed: 0, Passed: 1709, Skipped: 0, Total: 1709
... -f net10.0
Failed: 0, Passed: 1709, Skipped: 0, Total: 1709

That count is one higher than the mainline's, and the extra leg is the thing worth reading here. A rewrite that moves a filter is a rewrite that can drop it, so I deleted the comment filter and ran the two route classes:

the comment filter deleted from Script, before this change   -> Failed: 0, Passed: 18

Green. Nothing held it. Every refusal pattern is anchored to the start of a line and a comment never starts with exit, so the existing leg ARefusalInsideACommentIsNotARefusal passes whether or not comments are stripped. The one reading a dropped filter does change is the assignment pattern, which is deliberately unanchored so an assignment after a semicolon or a case pattern is read. A comment such as # want="0.1.1.0" is then an assignment, the compared variable's origin becomes the literal, and the join is refused for a comment.

OriginReassignedInAComment is that fixture, the control with one commented-out assignment added after the real one, and AnAssignmentInsideACommentIsNotAnOrigin asserts the join still reads as one. With the filter deleted again:

the comment filter deleted from Script, with the new leg      -> Failed: 1, Passed: 18: AnAssignmentInsideACommentIsNotAnOrigin
JobLines trimming both ends instead of the end                -> Failed: 10, Passed: 8
restored                                                     -> Failed: 0, Passed: 19

The second line is the one-character mistake this rewrite could have made, Trim for TrimEnd, which loses the indentation every reading of the job depends on.

The means

C#, in the file the alerts name, and the fixture in the same shape as the five beside it. It adds no language, runtime or dependency. The alternative was a dismissal, and the sibling boards have taken both routes for this class; here the query read the two sites correctly, the projection is the plainer form, and a dismissal would have left the filter unheld.

What this does not say

The alerts close when the analysis next runs over master, not on this merge, so #373 stays open until that reading is pasted into it. Whether this class arrives again is operations#2176's question and not this change's.

No second reader looked at this. The runs above are the evidence in place of one.

Code scanning reported the two loops in ReleaseVersionAgreementTests that
map their iteration variable in their first statement, alerts 238 and 239
of the class cs/linq/missed-select. The mapping moves into the sequence
in both. In the script reader the comment filter moves with it, because a
body that is one test and a continue is the shape the next scan reads as
a missed Where, and a sibling board paid for that rewrite with a second
class minted at the same sites one scan later.

Deleting the comment filter left the suite green, which I found while
proving the rewrite. Every refusal pattern is anchored to the start of a
line, so a commented-out exit was never read as one, and the assignment
pattern is the one unanchored reading that a dropped filter changes. A
fixture holds it now: an assignment inside a comment that would otherwise
become the compared variable's origin and refuse the route's join for a
comment. With the filter deleted that leg is the one that goes red.

Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com>
@iderex iderex added the security label Sep 7, 2026
@iderex
iderex merged commit e941800 into master Sep 7, 2026
14 of 15 checks passed
@iderex
iderex deleted the scan/the-two-line-readers-are-projections branch September 7, 2026 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant