Review code style

[rdeago]

Checklist of related issues / discussions

• Closes #
• Partially closes #
• Related discussion(s): #

Proposed changes

Fixed some ReSharper hints.

Not all hints were taken: some did not improve readability, some even made the code noticeably less readable.

No behavior change is expected.

Additional changes

• ParseBool was duplicated, byte for byte, in ReleaseSettings and VersionAdvanceSettings. Both copies are gone, replaced by CliOptionReader.ReadBoolValue, which reads the option and parses it in one call. Same diagnostic message, so no behavior change. Covered by new tests in CliOptionReaderTests.

• Retuned seven ReSharper hint severities in .editorconfig, so that this kind of review is enforced by the gate rather than repeated by eye:

  • promoted to warning: ConvertToConstant.Local, CanReplaceCastWithLambdaReturnType, RedundantLambdaSignatureParentheses;
  • suppressed: ArrangeObjectCreationWhenTypeNotEvident, plus UseRawString, UseVerbatimString and RawStringCanBeSimplified as a group — those three disagree with each other by design, and the judgment they were nagging about is now written down as a new String literals section in .claude/rules/csharp-style-guide.md.

  Promoting ConvertToConstant.Local immediately caught four locals in SelfVersionServiceTests that the manual pass had missed; they are now const.

• Made .editorconfig the only place ReSharper inspection severities are set. Buildvana.slnx.DotSettings held six more, one of which (UnusedMember.Global at WARNING) contradicted .editorconfig's none outright. .editorconfig wins that race, so the entry was already dead; the whole InspectionSeverities block moved out rather than just that one line, and the file's UI-filter rows were left alone.

• Brought GitService.cs fully within the line-length limit. The branch had already reworked that file, and the style guide asks for the leftover wraps as their own commit once that is the case.

• Fixed .claude/tools/inspect.cs exiting zero on a failed build in its non-gate modes. Those modes deliberately do not fail for findings, but a failed build is not a finding: nothing was analyzed. Since the VS Code task's problem matcher requires a file position, a build failure whose diagnostics carry none left the Problems panel empty behind a successful-looking task. A failed build now exits 2, exactly as a failed inspectcode run already did; a build that merely reported diagnostics still exits zero. The --gate mode was already correct and is unchanged.

Types of changes

This pull request introduces the following types of changes:

• Bug fix
• New feature
• Test addition / update (no changes to non-test code)
• Refactor (no changes in public API syntax or semantics)
• Performance improvement (no changes in public API syntax or semantics)
• Documentation (docs directory) update
• Dependency addition / update
• Changes to the build scripts
• Changes to CI (workflows, bot / app configurations)
• Changes to repository files (.gitattributes, .gitignore)
• Other

Breaking changes

This pull request introduces breaking changes:

• Yes
• No

Checklist

• For all types of changes:
  • I have read the Code of Conduct and agree to be bound by it
  • I have read the Contributor Guide and agree to follow it
• For code changes only:
  • The project builds on my machine, via the provided build script, with zero warnings
  • I have added tests that prove my feature works / my fix is effective
  • I have added / modified XML documentation according to changes in code
  • I have checked that all the links I added or modified in XML documentation point to their intended destination
• For documentation changes (docs directory) only:
  • I have built and tested documentation locally
  • I have checked that all the links I added or modified point to their intended destination

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 6m 16s —— View job

---

Review of #340 — Review code style

• Read the full diff
• Check each change for behavior differences
• Check against .claude/rules/csharp-style-guide.md
• Post review

Ric — I went through all 18 files. No correctness issues found: every change is behavior-preserving. Details on the three that needed a second look are at the bottom. What's left is one design point I'd push back on, two style-guide misses, and some subjective calls.

---

Correctness

Clean. Three changes were non-obvious enough that I verified them rather than eyeballing:

• GitService.cs:133 — latest == null → latest ??= version. ??= uses a plain null test; == would have gone through NuGet.Versioning.SemanticVersion's == overload. That overload delegates to a null-safe Equals, so the two agree here. This is the carve-out csharp-style-guide.md describes under "Pattern matching", and it lands on the right side of it.
• SelfReferenceUpdater.cs:147,151 — verbatim → raw string regexes. Re-escaping is correct: no """ run appears in either pattern, neither starts nor ends with a quote, and the patterns contain no {n,m} quantifiers that would collide with the single-$ interpolation. Both regexes are byte-identical to what $@"…" produced.
• SelfVersionService.cs:313 — propertyPath is not [SchemaPropertyName]. SchemaPropertyName is a const string (line 44), so this is a genuine constant pattern with ordinal Equals semantics, matching the != it replaced. propertyPath is IReadOnlyList<string> and is never null on this path, so the pattern's implied null test is inert.

The PublicApiFilesService local-function move is fine (local functions are hoisted), and moving it below its call site is what the "Member ordering" rule asks for.

---

1. GitHubServerAdapter: folding _token into PushPassword conflates two contracts

src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs:47,76,253

That token has two jobs: it is the git push password, and it is the GitHub REST API credential. The old _token field named the thing itself and both uses read from it. After the rewrite, CreateGitHubClient reads:

Credentials = new(PushPassword),

The base contract (ServerAdapter.PushPassword, line 72) is specifically "the password to use when pushing" — ReleaseCommand.cs:71 consumes it as exactly that, feeding git.PushCredentialsFallback. Sourcing the API credential from it is true by coincidence, not by contract. If push ever moved to a narrower credential (deploy key, app installation token), CreateGitHubClient would silently follow it into the wrong scope, with nothing at the edit site to signal that.

This is a ConvertToAutoProperty-shaped hint: mechanically correct, one field cheaper, and it loses the intent. I'd keep _token and leave PushPassword => _token. Fix this →

The sibling change in ServerRelease.cs (_releaseCommitSha → ReleaseCommitSha { get; private set; }) has none of this problem — the field and the property meant the same thing there. Keep that one.

2. ExceptionExtensions: the dropped parentheses were load-bearing

src/Buildvana.Core.Abstractions/Diagnostics/ExceptionExtensions.cs:39

=> @this is UnauthorizedAccessException
    or NotSupportedException
    or ArgumentException and not ArgumentNullException
    or SecurityException
    or IOException;

ReSharper is right that this is equivalent — and binds tighter than or, so the grouping survives. But five lines that all look like peer or operands now depend on the reader knowing pattern-combinator precedence, and the expression is fragile under editing: insert or FormatException after the ArgumentException line and and not ArgumentNullException silently rebinds to FormatException, with no compiler complaint. The parentheses also mirrored the XML doc directly above ("an ArgumentException other than ArgumentNullException").

This looks like one for the "did not improve readability" pile the PR description mentions. Fix this →

3. Style-guide miss: modified line still over 140 chars

src/Buildvana.Tool/Services/Git/GitService.cs:218 — 161 characters.

Commands.Stage(_repository, pathsInRepo, new StageOptions { IncludeIgnored = false, ExplicitPathsOptions = new() { ShouldFailOnUnmatchedPath = true } });

The PR modified this line (new StageOptions() → new StageOptions), and "Line length" says new and modified lines always comply. Worth wrapping while you're on it. Line 263 in the same file is also over, but untouched — that one falls under the "mechanical sweep" carve-out and I'd leave it. Fix this →

4. Style-guide miss: extension-block template is missing its summary

src/Buildvana.Sdk.SourceGenerators/Internal/StringBuilderExtensions.cs:9

The mandated template opens with a /// <summary> Provides extension methods for … </summary>. The converted class has no doc comment. Its near-twin src/Buildvana.Core.Process/Internal/StringBuilderExtensions.cs:10-13 — also internal, also over StringBuilder — carries exactly that comment, so this is a straight consistency gap.

Dropping the CA1034 / CA1708 pragmas is the right call for an internal class (neither rule fires on non-visible types), and matches what Buildvana.Core.Process/Internal/StringBuilderExtensions.cs does. Only the summary is missing. Fix this →

5. ParseBool is now byte-identical in two files

src/Buildvana.Tool/Subcommands/ReleaseSettings.cs:196 and src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs:106

The duplication predates the PR, but the rewrite made the two copies identical character for character, down to the exception message — same namespace (Buildvana.Tool.Subcommands), same assembly. That's the kind of thing "Small changes out of scope" says to fix now rather than file: extracting one shared internal static helper and updating two call sites is a handful of lines, plus a line under Additional changes. Fix this →

6. Subjective: two of the inverted-if rewrites cost lines and buy nothing

src/Buildvana.Core.Configuration/BuildvanaConfigLoader.cs:102 and src/Buildvana.Core.Json/JsonHelper-InsertProperty.cs:332

Both loops had a single if whose body was two statements — already flat. Inverting to a continue guard turns 4 lines into 7 and removes no nesting, because there was none to remove:

// before                          // after
if (json[i] == (byte)'\n')         if (json[i] != (byte)'\n')
{                                  {
    newlines++;                        continue;
    lineStart = i + 1;             }
}
                                   newlines++;
                                   lineStart = i + 1;

Contrast GitService.cs:133, where the same transformation collapsed three nesting levels to one — that one earns its keep. These two look like they slipped past the filter the PR description describes. Entirely your call. Fix this →

Nits

• JsonSchemaGenerator.cs:79 — after the new guard, schema and root are the same object; return root; would spare the reader a moment's doubt about whether the Insert calls are reflected in what's returned.

---

Clearly good

• The raw-string regexes in SelfReferenceUpdater are the standout — [^""]+ → [^"]+ makes those patterns readable for the first time, and they got shorter.
• propertyPath is not [SchemaPropertyName] is the style guide's own worked example, verbatim.
• const string on the test literals, and the split in SelfVersionServiceTests.cs:157 that brings a 124-char line under the limit.
• Moving IsEmptyOrStartsWithHash below its call site — correct per "Member ordering", and the kind of thing no analyzer would have told you.
• Dropping new StageOptions() → new StageOptions and the redundant lambda parens: pure noise removal, no downside.

Not verified

I did not build locally — restore needs Buildvana.Sdk 2.1.70-preview from the private feed, which isn't reachable from this runner. Compile-level claims above (const-ness, base declarations, types) come from reading the sources; CI is the arbiter on the build itself. Nothing in the diff looked at risk: extension blocks are already used in Buildvana.Sdk.SourceGenerators on netstandard2.0 (AnalyzerConfigOptionsProviderExtensions.cs:16), so the converted StringBuilderExtensions breaks no new ground there.
· branch code-style-review

[codecov]

Codecov Report

❌ Patch coverage is 21.12676% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.48%. Comparing base (bf0343b) to head (07bad9d).

Files with missing lines	Patch %	Lines
src/Buildvana.Tool/Services/Git/GitService.cs	10.34%	26 Missing ⚠️
...urceGenerators/Internal/StringBuilderExtensions.cs	0.00%	13 Missing ⚠️
src/Buildvana.Tool/Services/DotNetService.cs	0.00%	4 Missing ⚠️
...c/Buildvana.Core.JsonSchema/JsonSchemaGenerator.cs	57.14%	1 Missing and 2 partials ⚠️
...vana.Tool/Services/ServerAdapters/ServerRelease.cs	0.00%	3 Missing ⚠️
...l/Services/PublicApiFiles/PublicApiFilesService.cs	0.00%	2 Missing ⚠️
...rc/Buildvana.Tool/Services/SelfReferenceUpdater.cs	0.00%	2 Missing ⚠️
...Sdk.Tasks/Internal/JetBrainsAnnotationsExporter.cs	0.00%	1 Missing ⚠️
src/Buildvana.Tool/Services/SelfVersionService.cs	0.00%	0 Missing and 1 partial ⚠️
...verAdapters/Internal/GitHub/GitHubServerAdapter.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
- Coverage   56.77%   56.48%   -0.30%     
==========================================
  Files         160      160              
  Lines        4451     4458       +7     
  Branches      786      782       -4     
==========================================
- Hits         2527     2518       -9     
- Misses       1825     1840      +15     
- Partials       99      100       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rdeago]

Thanks — good review. Took six of the seven findings; the one I deviated on is #5, where I think there's a better home than the one proposed. All eight commits are pushed, and the sanity-check gate (bv pack + ReSharper at WARNING and above) is green: 0 errors, 0 warnings, 0 infos.

Addressed

1. GitHubServerAdapter — _token restored. Done, and the argument is stronger than the review put it: ServerAdapter.PushPassword is declared string?, and GitLabServerAdapter returns null for it. So the base contract genuinely is "push credential, may not exist" — sourcing the REST API credential from it worked only because GitHub happens to use one token for both. CreateGitHubClient reads _token again, and the declined ConvertToAutoProperty hint now carries a suppression comment with the reason, so a future sweep doesn't silently redo it. (f7d01a1)

2. ExceptionExtensions — parentheses restored. Agreed, the edit-fragility argument is the decisive one. No suppression comment needed: ArrangeRedundantParentheses is Hint severity, below the gate's threshold, and the XML doc directly above already states the intent. (69db60e)

3. GitService.cs:218 — wrapped. One correction to the review: that file has eight lines over 140 (41, 88, 120, 125, 209, 218, 263, 269), not just 263. Your mechanical-sweep read is right, though — 18 files at a few lines each is exactly that carve-out — so only 218 is wrapped here, and the rest is left for a standalone cleanup. (29014c6)

4. StringBuilderExtensions summary — added, wording matched to the Buildvana.Core.Process twin. (6ce8ba9)

6. Both guard clauses reverted. Agreed — no de-nesting, three lines each. Both files are now byte-identical to main. The GitService one stays, for the reason you gave. (fbeac9a)

Nit — return root. Done. (1ba0221)

Deviated

5. ParseBool — extracted, but to CliOptionReader, not to a helper beside the settings classes. Agreed it should be fixed now rather than filed, but a free-standing helper in Buildvana.Tool.Subcommands leaves both call sites still naming each option twice:

CheckPublicApi = ParseBool(reader.ReadValue("--check-public-api"), "--check-public-api"),

CliOptionReader already owns option-token semantics and already throws BuildFailedException for bad option usage, so the parsing belongs there. It's now CliOptionReader.ReadBoolValue, and the call sites read CheckPublicApi = reader.ReadBoolValue("--check-public-api"). Both copies are gone, the diagnostic message is unchanged (both call sites passed the long name as the option name), and there are five new tests in CliOptionReaderTests. Recorded under Additional changes. (c839bee)

One more commit

c0093c7 fixes my own new tests: TUnitAssertions0015 rejects IsEqualTo(true)/IsEqualTo(false) in favour of IsTrue()/IsFalse(). Caught by the gate on the first run, fixed in its own commit rather than an amend.

Not verified

You couldn't build; I could. The gate ran the full clean + restore + build + test + pack chain plus ReSharper, all green, so the compile-level claims in your review are now confirmed rather than read.

@claude please re-review.

[claude]

Claude finished @rdeago's task in 3m 30s —— View job

---

Re-review of #340 — Review code style

• Read the 8 new commits
• Verify each addressed finding
• Review the deviation on Use GitHub package repository. #5 (ReadBoolValue) and its new tests
• Post review

Ric — verified all eight commits against origin/main. Every finding is addressed correctly, no new correctness issues. Your deviation on #5 is the better design and I'd have been wrong to push mine. One doc nit below, plus a note for the deferred cleanup. Also: you're right about the line count and I was wrong.

---

Verification of the addressed findings

#	Commit	Verified
1	f7d01a1	_token restored (:31), PushPassword => _token (:78), CreateGitHubClient reads _token (:255). Suppression comment is canonical form with justification.
2	69db60e	Parens restored.
3	29014c6	Commands.Stage wrapped, one argument per line, StageOptions initializer expanded, trailing comma. No line in the file's changed region exceeds 140.
4	6ce8ba9	Summary added, wording matches the Buildvana.Core.Process twin.
6	fbeac9a	Both loops reverted.
nit	1ba0221	return root; — and it also drops the last reference to schema after the type check, so the reader no longer has to hold two names for one object.

git diff origin/main..HEAD is now empty for ExceptionExtensions.cs, BuildvanaConfigLoader.cs, and JsonHelper-InsertProperty.cs — those three files are byte-identical to main, as you said.

Your correction on the line count stands. GitService.cs has eight lines over 140 on main, not two. I checked only around the diff hunks and reported what I saw there; that was an incomplete check reported as a complete one. Current state after the wrap: 41, 88, 120, 125, 209, 270, 276 — 218 is gone and the two tail lines just shifted. And I agree the rest belongs in the standalone sweep: DotNetService.cs is the clearest case — it has three over-140 declarations (:62, :93, :128) and this PR's only edit to it was stripping four pairs of lambda parens, which is the mechanical-sweep carve-out exactly.

---

On #5 — CliOptionReader.ReadBoolValue is the right call

You're right, and for a reason beyond the double-naming you gave. My proposal put a bool?-returning helper in Buildvana.Tool.Subcommands whose contract was "parse this string, and by the way here's the option name so the error message can say it" — the option name only travels as a parameter because the helper is on the wrong side of the boundary. CliOptionReader already holds it. Once the parsing moves there, the second argument stops existing rather than getting threaded through, which is the tell that the seam was in the wrong place.

The diagnostic message is unchanged ($"Invalid value '{raw}' for {longName}. …", and both call sites passed the long name as optionName), so this is behavior-preserving for every reachable input, including the --dogfood= empty-inline case. Both copies are gone and there's no third one — bool.TryParse now appears exactly once in src/Buildvana.Tool.

Details I checked on the new code:

• Ordering — ReadBoolValue (:76) sits above ReadValue (:92), which is caller-before-callee per the member-ordering rule. Guard.IsNotNullOrEmpty(longName) is not repeated, correctly: ReadValue runs it.
• Ternary shape — matches the concatenated-ternary template in the style guide, including the throw expression in tail position.
• Unused usings — using Buildvana.Core; survives in both settings classes: ReleaseSettings still throws BuildFailedException in IsMatch, VersionAdvanceSettings in ResolveChange (:98).
• Tests — the five new cases test what ReadBoolValue actually adds (parse success, parse failure, null pass-through) plus two form smoke tests, and correctly don't re-test ReadValue's short-name and last-occurrence-wins behavior, which delegation already covers. Naming matches the existing set, and IsCaseInsensitiveOnValue is precise about which case-insensitivity it means.
• c0093c7 — IsTrue()/IsFalse() on bool? is fine; TUnitAssertions0015 catching this is a good argument for the gate running before the push rather than after.

Nit: the <returns> doc came along with the copy

src/Buildvana.Tool/CommandLine/CliOptionReader.cs:73

/// <returns>The last value supplied for the option, or <see langword="null"/> if it was absent.</returns>

Verbatim from ReadValue (:90), where "value" means the raw token. Here it's a bool?, so "the last value supplied" reads as the string the user typed rather than its parsed form. Something like "The parsed value of the last occurrence of the option, or null if it was absent" removes the ambiguity. The <summary> and <exception> are both accurate as written. Fix this →

Note for the deferred line-length sweep

Not a finding on this PR — just so the future sweep doesn't come up short. The style guide holds declarations to 120, not 140, and says a file brought within the general limit but left with over-long declarations still owes a second sweep. A 140-only pass would miss e.g. VersionAdvanceSettings.cs:70 (Parse(...), 136 chars — over 120, under 140). Worth grepping at both thresholds when you do it.

---

Verification status

Same as last time, I did not build: restore still needs Buildvana.Sdk 2.1.70-preview from the private feed, unreachable from this runner. Everything above is source reading plus git diff. Your green gate run is the arbiter on compilation and tests, and it covers the claims I could only read.

No blocking issues. The nit is optional — this is ready to merge as far as I'm concerned.
· branch code-style-review

[rdeago]

One more commit since the re-review request, so it isn't pointed at a stale head: 8cfa82d retunes seven ReSharper hint severities in .editorconfig, plus a new String literals section in the C# style guide. Gate is green. Details are under Additional changes; the reasoning is worth a paragraph here since it changes what future reviews of this kind will and won't have to catch by eye.

Promoted to warning — ConvertToConstant.Local, CanReplaceCastWithLambdaReturnType, RedundantLambdaSignatureParentheses. All three are mechanical, with no legitimate exception, so the gate is a better place for them than a manual pass. ConvertToConstant.Local earned its promotion immediately: four locals in SelfVersionServiceTests had been missed and are now const.

Suppressed — ArrangeObjectCreationWhenTypeNotEvident, and the three literal-form inspections as a group.

That group is the interesting one. UseRawString, UseVerbatimString and RawStringCanBeSimplified disagree with each other by construction: promoting one puts it in tension with the next, and the right answer genuinely varies. Raw wins where the content carries quotes — the SelfReferenceUpdater regexes in this PR are the case in point. Escapes win where an escape is the point: "{\n \"name\": 42\n}" in a test asserting line and column keeps its newlines independent of the file's line endings, where a raw literal would make them a property of the source file. So rather than gate any of them, the judgment is now written down in .claude/rules/csharp-style-guide.md, and JsonSchemaValidatorTests.cs:64 keeps its """{}""" for consistency with the literal it is paired with.

Two notes on things I checked rather than assumed:

• A misspelled resharper_*_highlighting key is silently ignored, which would make a promotion a permanent no-op. I verified the suppressions by confirming the two inspections that were actually firing disappeared from a HINT-severity run (44 hints → 42), and verified RedundantLambdaSignatureParentheses — which has no live sites — by temporarily adding a redundant-paren lambda and confirming it reported at warning rather than its default hint. Probe reverted.
• The RedundantPatternParentheses hint behind finding Use the SDK to build itself #2 also fires on Buildvana.Runtime/ExceptionExtensions.cs:29, the BCL-only twin of the file I restored parentheses in. That copy had kept its parentheses all along, so the sweep had de-synced the two and the restore re-aligned them. Both stay at hint.

@claude please re-review — the diff has grown since the last request.

[claude]

Claude finished @rdeago's task in 6m 12s —— View job

---

Re-review of #340 — Review code style

• Read 8cfa82d (.editorconfig retune + style-guide section)
• Verify key placement, spelling convention, and duplicate keys
• Check the new String literals section against the code it cites
• Cross-check .editorconfig severities against Buildvana.slnx.DotSettings
• Post review

Ric — read the new commit against the previous head. No correctness issues; nothing in 8cfa82d changes behavior. Everything I verified last time still holds, and the four const promotions in SelfVersionServiceTests are all raw-string literals, which are legal constant expressions.

One finding on the verification gap you opened yourself, one pre-existing conflict the new approach makes relevant, and one internal contradiction in the new prose that the repo's own code already disagrees with.

---

What I verified in 8cfa82d

• Placement. All seven keys land after line 565, inside the [*.{cs,csx,cake}] section that opens at .editorconfig:73 and runs to EOF. No stray section boundary between them.
• No duplicates. resharper_* keys are unique across the file — none of the seven silently overrides or is overridden by an earlier line.
• Snake-case convention. ConvertToConstant.Local → resharper_convert_to_constant_local_highlighting matches the dot-to-underscore precedent already in the file at :543 (resharper_unused_member_global_highlighting, i.e. UnusedMember.Global). Consistent.
• The two promoted rules can't fight each other. MetadataReference (path) => … keeps its parentheses because C# requires a parenthesized parameter list when a lambda states its return type, so RedundantLambdaSignatureParentheses can never fire on what CanReplaceCastWithLambdaReturnType produces. Worth knowing, since promoting both to warning is what would have made that collision fatal.
• The style-guide citations are accurate, both of them. JsonSchemaValidatorTests.cs:64 is """{}""" paired on the previous line with a raw-string schema — the "consistency with adjacent literals" case exactly. And :109's "{\n \"name\": 42\n}"u8 sits in Validate_WithBytes_FillsLineAndColumn, which asserts Line == 2 and Column == 11.

That second one is stronger than the guide claims, and the reason is worth writing down: .gitattributes sets * -text, which disables line-ending normalization completely. Git will not normalize a CRLF file on the way in or out, so a file that ever picks up CRLF keeps it, in every checkout, permanently. A multi-line raw literal there would silently become \r\n-delimited and Column == 11 would fail on line 2 while the test still looked correct. The escape form isn't belt-and-braces — with -text it's the only thing holding that assertion up.

---

1. One promotion has neither a live site nor a probe

.editorconfig:570 — resharper_can_replace_cast_with_lambda_return_type_highlighting = warning

Your note accounts for six of the seven keys: ConvertToConstant.Local proved itself by firing (four locals), RedundantLambdaSignatureParentheses by the temporary redundant-paren probe, and two of the four suppressions by the 44 → 42 drop. This one is in neither group. Its only site in the repo — JetBrainsAnnotationsExporter.cs:69, (MetadataReference)MetadataReference.CreateFromFile(path) → MetadataReference (path) => … — was fixed in 106b85a, two commits before the key was written. I grepped src/ and tests/ for other => (Type)expr lambdas and found none, so a HINT-severity run would have shown nothing for it either way.

The four unverified-or-verified keys are not equal in consequence, though, and that's what makes this the only one worth acting on:

Typo in…	Effect	At risk?
a suppression (4 keys)	inspection stays at Hint; gate runs at WARNING, so it never fails	no — self-limiting
a promotion (3 keys)	inspection stays at Hint; gate stays green forever	yes

A misspelled suppression costs nothing. A misspelled promotion is invisible by construction: absence of diagnostics is exactly what compliance looks like. Concretely — someone writes .Select(path => (MetadataReference)MetadataReference.CreateFromFile(path)) again next month, the gate goes green, and the rule you thought you'd bought is not there. That's the failure mode you guarded the other two against; this key just missed the sweep because its site was already clean.

The probe you already ran works verbatim here: restore the cast at JetBrainsAnnotationsExporter.cs:69, run the gate, confirm it reports at warning rather than hint, revert. Fix this →

2. .editorconfig and Buildvana.slnx.DotSettings disagree on UnusedMember.Global

.editorconfig:543 — resharper_unused_member_global_highlighting = none
Buildvana.slnx.DotSettings:427 — InspectionSeverities/=UnusedMember_002EGlobal → WARNING

Pre-existing, not this PR's doing, and I'd have left it alone — except that this commit makes .editorconfig the declared source of truth for inspection severities, and it's the one inspection where a second mechanism says the opposite. Whichever layer ReSharper resolves in favour of, one of those two lines is dead, and a reader has no way to tell which without running the tool. The other five InspectionSeverities entries in the DotSettings (AnnotateCanBeNull*, AnnotateNotNull*, ConvertToPrimaryConstructor) have no .editorconfig counterpart, so this is the only collision.

Deleting one of the two lines is a one-line change, which puts it squarely in "FIX. IT. NOW." territory rather than an issue — but which line goes is your call, since it's a real behavioral question and not a formatting one. Fix this →

While you're in that file: three IssueTypesToHide … DoShow entries are now stale — ConvertToConstant.Local (:63), RedundantLambdaSignatureParentheses (:351), UseVerbatimString (:402). Those are UI-filter entries, not severities, so they're harmless; the last one is simply unreachable now that the inspection is none. Cosmetic, mention it only because you'd be editing the file anyway.

3. The new section's bullets 1 and 4 give opposite answers for the same string

.claude/rules/csharp-style-guide.md:218 and :221

• Raw ("""...""") when the content itself contains quotes or backslashes.
• Verbatim (@"...") … content heavy in backslashes but free of quotes, such as Windows paths.

@"C:\hooks\test.cs" satisfies both. Read top-down, bullet 1 says raw; read to the end, bullet 4 says verbatim. This isn't hypothetical — seven literals in the repo are exactly that shape and all follow bullet 4, so the repo's own code contradicts bullet 1 as written:

• tests/Buildvana.Tool.Tests/FileBasedAppHelperTests.cs:28 — @"C:\hooks\test.cs"
• tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs:42,58 — @"docs\ConstantsSyntax.md", @"..\outside.md"
• src/Buildvana.Core.Versioning/VersionSpecExtensions-private.cs:17 and three regex literals in the settings tests

The discriminator that actually works is quotes, not backslashes: raw and verbatim handle \ identically, and raw's win is that " needs neither escaping nor doubling — which is precisely the [^""]+ → [^"]+ example bullet 1 gives. Striking "or backslashes" from bullet 1 makes the list total and non-overlapping, and leaves both bullets agreeing with the code.

This matters more than a normal doc nit because the section is now the only arbiter — the three inspections that used to nag about this are off, so a future sweep has nothing but these five bullets to go on. Fix this →

---

Clearly good

• The .editorconfig → .claude/rules/ cross-reference (:581). Someone editing severities who has never opened .claude/ now gets pointed at the reasoning instead of re-deriving it, and the rules file explains the suppression instead of just asserting it. That closes the loop the group suppression would otherwise have opened.
• The promote/suppress split is drawn on the right axis — mechanical-with-no-exception goes to the gate, judgment-call goes to prose. ConvertToConstant.Local catching four missed locals on its first run is the argument for the first half; the three literal-form inspections disagreeing by construction is the argument for the second.
• "Hint is the lowest severity ReSharper offers, so silencing one means none" is accurate and pre-empts the obvious "why not just lower it?" question.
• c0093c7 in its own commit rather than an amend, per the sanity-check rule.

Note on this checkout

This runner's working tree has .claude/rules/csharp-style-guide.md modified — the String literals section is absent from the file on disk while present in HEAD. That's this checkout's state, not your branch's; 8cfa82d contains the section correctly. I read the committed version via git show throughout and left the working tree untouched.

Verification status

Unchanged from last round: I did not build. Restore still needs Buildvana.Sdk 2.1.70-preview from the private feed, and I have no network access from this runner — I tried to check the four inspection IDs against JetBrains' inspection reference and could not reach it. So finding 1 is specifically "this key was never exercised", not "this key is wrong"; I have no way to confirm the spelling either direction from here, which is the whole reason the probe is worth five minutes. Your green gate covers compilation and tests.

Nothing blocking. Finding 1 is the one I'd act on before merge; 2 and 3 are cheap and your call.
· branch code-style-review

[rdeago]

b0eda36 brings GitService.cs fully within the line-length limit — the seven lines left over after the targeted fix in 29014c6. The style guide asks for exactly this once a branch has actually worked in a file, and for it to land as its own commit, which it does. Gate is green.

Nothing subtle in six of them: two invocations wrapped one argument per line, a lambda ternary split across three lines, three doc-comment paragraphs rewrapped, an object initializer expanded, and the compound path check in Stage moved into an isOutsideRepo local per the "no multi-line conditions" rule.

The seventh is worth calling out, since it is the one judgment call in the commit. It was a URL:

// https://github.com/libgit2/libgit2sharp/blob/5085a0c6173cdb2a3fde205330b327a8eb0a26c4/LibGit2Sharp.Tests/PushFixture.cs#L183-L187

144 characters, and unwrappable — splitting a URL across lines breaks click-through, which is the entire value of citing it. Rather than leave it over the limit or damage it, I abbreviated the 40-character commit SHA to 10. GitHub resolves abbreviated SHAs in blob URLs and redirects to the full commit, so the link still works and still pins the same revision, at 114 characters. Ten hex digits is well clear of collision range for that repository. Easy to revert to the full SHA if you would rather have the limit broken than the SHA shortened — the line is the only thing that changes either way.

@claude no new review needed for this one unless you want to look at it — it is mechanical wrapping on top of the diff you are already reviewing.

[claude]

Claude finished @rdeago's task in 3m 1s —— View job

---

b0eda36 — line-length sweep of GitService.cs

• Read the commit
• Verify both thresholds on the file
• Check the one semantic-touching change
• Check the abbreviated-SHA call

Ric — took you up on the invitation, since the SHA call was the kind of thing worth five minutes. Nothing to change. Agreed on the SHA, and the sweep is complete at both thresholds.

---

Both limits are satisfied, not just 140

Worth stating explicitly because of the note I left last round: a file brought under 140 but left with over-120 declarations still owes a second sweep. This one doesn't. Zero lines over 140, and the eight lines still over 120 (39, 47, 51, 92, 108, 121, 254, 273) are invocations, an assignment, and doc comments — no declaration among them. GitService.cs is done.

The one semantic-touching change is inert

isOutsideRepo is a pure extraction: || short-circuits identically inside the initializer, and all three operands are side-effect-free, so even that doesn't matter. The expanded UsernamePasswordCredentials initializer picked up the trailing comma SA1413 wants. The out var origin / out var version declarations both survive their wraps unchanged in scope.

On the abbreviated SHA — keep it

Agreed, and I'd argue the tradeoff is even more one-sided than you put it.

• 5085a0c617 is a correct 10-char prefix of 5085a0c6173cdb2a3fde205330b327a8eb0a26c4, and GitHub resolves abbreviated commit-ish in /blob/ URLs (7 chars is its usual floor). The #L183-L187 fragment is applied client-side, so it survives the redirect.
• The collision question isn't the birthday bound over the repo's objects — it's whether some other object in libgit2sharp shares this specific 10-hex prefix. That's ~n/2⁴⁰ ≈ 10⁻⁷ for a repo of a few hundred thousand objects. Seven digits would have been ample; ten is not close to anything.
• It stays a one-off, not a new convention. I grepped every .cs file in the repo: no other URL anywhere exceeds ~105 characters, so nothing else is near the limit and the style guide doesn't need an unbreakable-token clause written for it. The one other blob URL carrying a full 40-char SHA — tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs:36 — is an assertion literal, i.e. the URL the code produces rather than one a reader clicks, so it must keep the full SHA and no inconsistency is created by shortening this one.

So: link still works, still pins the same revision, and no reader is worse off. I'd rather have the abbreviated SHA than the broken limit.

One nit, and it's in the comment, not the commit

Your tally says "two invocations wrapped" and "three doc-comment paragraphs rewrapped". The commit has one invocation (ThrowIfNot(TryGetOriginInfo(…)) at :39) and two doc paragraphs (:88 and :120 on the old numbering) — the Commands.Stage wrap was 29014c6, one commit earlier. The seven lines are all accounted for either way (1 invocation + 2 doc paragraphs + 1 lambda ternary + 1 condition + 1 initializer + 1 URL), so it's the prose that's off by one in two places, not the sweep. Mentioning it only because the count is the sort of thing you'd want right in the record.

---

Verification status

Unchanged: no build from this runner — restore still needs Buildvana.Sdk 2.1.70-preview from the private feed, and I have no network access here (which is also why the GitHub abbreviated-SHA behavior above is from knowledge of the URL scheme, not from following the link). Line-length and prefix checks are mechanical and exact. Your green gate covers compilation and tests.

Still ready to merge from my side. No new findings.
· branch code-style-review

[rdeago]

Both reviews addressed; 07bad9d is the head and the gate is green.

First, a correction to my own record. My b0eda36 message and the comment above it both say "two invocations wrapped" and "three doc-comment paragraphs rewrapped". It was one invocation and two doc paragraphs — I counted the Commands.Stage wrap, which belonged to 29014c6. The seven lines are all accounted for either way, so the sweep is complete; only the prose was off. The commit message stays as pushed rather than being rewritten.

Addressed

• <returns> on ReadBoolValue (a8e326e) — it described the raw token, inherited from ReadValue. Now describes the parsed value.
• The unexercised promotion (no commit) — probed as suggested: restored the cast at JetBrainsAnnotationsExporter.cs:69, confirmed it reports at warning rather than hint, reverted. resharper_can_replace_cast_with_lambda_return_type_highlighting is spelled correctly and live. All three promotions are now verified rather than assumed, which was the point.
• The self-contradicting bullets (f757f07) — correct, and it was the worst kind of doc bug: the section is the only arbiter now that the three inspections are off, and it disagreed with the repo's own seven verbatim literals. Quotes are now the sole discriminator, with a sentence saying why backslashes are not one, since raw and verbatim treat \ identically.
• The two-mechanism conflict (07bad9d) — resolved by removing severities from Buildvana.slnx.DotSettings entirely rather than deleting the single contradicting line, which is Ric's call and the one that prevents a rerun of this discussion.

On which mechanism wins, since the finding left it open

Measured rather than reasoned: flipping .editorconfig's resharper_unused_member_global_highlighting to warning surfaces 38 UnusedMember.Global instances, while the committed none leaves the gate green. So .editorconfig overrides the DotSettings entry, and that entry was already dead — deleting it is a no-op, while deleting the .editorconfig line would have reddened the gate with 38 warnings. Not symmetric, which is what made the choice easy.

The other five moved across keeping their severity: the four Annotate* promotions, and ConvertToPrimaryConstructor as none (DO_NOT_SHOW's equivalent). UnusedMember.Global was dropped as a duplicate. UI-filter rows untouched, per Ric — including the three now-stale ones, which cost nothing.

Same spelling risk applied to the migration, so it got the same treatment: a misspelled key would restore ConvertToPrimaryConstructor to its default severity, and this codebase has no shortage of primary-constructor candidates. It stays absent from a HINT-severity run, so the key took effect. The four Annotate* keys have no site in the repo that triggers them, so their spelling is unverifiable by probe — worth stating plainly rather than implying the whole migration was measured. They follow the same mechanical PascalCase-to-snake_case mapping confirmed four times over by now, and at zero sites a typo would be inert either way.

One thing I introduced and am leaving

The isOutsideRepo extraction in b0eda36 created a new ConvertIfStatementToReturnStatement hint at GitService.cs:217 (2 sites → 3). Taking it would mean return isOutsideRepo ? throw … : pathInRepo, and the style guide rules out ternaries used for side effects. Hint severity, so it cannot reach the gate. Left alone deliberately.