tests/NLog.Targets.GZipFile.Tests/GZipFileTests.cs (1)

43-46: Global static setting may affect test isolation.

LogManager.ThrowExceptions is a global static property. Setting it in the constructor affects all tests in this class, but may also bleed into other test classes if tests run in parallel within the same process. Consider resetting it in a Dispose method (implementing IDisposable) or using IClassFixture to manage the setting's lifecycle.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/NLog.Targets.GZipFile.Tests/GZipFileTests.cs` around lines 43 - 46, The
constructor for GZipFileTests sets the global static LogManager.ThrowExceptions
which can leak across tests; modify GZipFileTests to capture the previous value,
implement IDisposable (or use an IClassFixture) and in Dispose restore
LogManager.ThrowExceptions to the saved previousValue so the global state is
reset after the test class finishes; reference the GZipFileTests constructor,
LogManager.ThrowExceptions and Dispose/IDisposable (or IClassFixture) when
making the change.

src/NLog/Targets/FileTarget.cs (1)

1313-1313: Consider extracting conditions to reduce cognitive complexity.

This line now has 5 conditional operators, exceeding SonarCloud's maximum of 4. While functionally correct, extracting to named locals would improve readability and satisfy the static analysis rule.

♻️ Proposed refactor to reduce complexity

                 if (Header != null)
                 {
-                    bool writeHeader = (initialFileOpen && WriteHeaderWhenInitialFileNotEmpty) || ReplaceFileContentsOnEachWrite || (initialFileOpen && ArchiveOldFileOnStartup) || (fileWasEmpty ?? fileStream.Length == 0);
+                    bool treatAsNewFile = ReplaceFileContentsOnEachWrite || (initialFileOpen && ArchiveOldFileOnStartup);
+                    bool forceHeaderOnStart = initialFileOpen && WriteHeaderWhenInitialFileNotEmpty;
+                    bool writeHeader = forceHeaderOnStart || treatAsNewFile || (fileWasEmpty ?? fileStream.Length == 0);
                     if (writeHeader)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Targets/FileTarget.cs` at line 1313, The complex boolean for
writeHeader should be split into named local booleans to reduce cognitive
complexity: compute separate locals such as bool isInitialOpen =
initialFileOpen, bool initialNotEmptyHeader = isInitialOpen &&
WriteHeaderWhenInitialFileNotEmpty, bool replaceOnEachWrite =
ReplaceFileContentsOnEachWrite, bool archiveOnStartup = isInitialOpen &&
ArchiveOldFileOnStartup, and bool isFileEmpty = fileWasEmpty ??
fileStream.Length == 0, then set writeHeader = initialNotEmptyHeader ||
replaceOnEachWrite || archiveOnStartup || isFileEmpty; update the assignment
site (writeHeader and the referenced symbols initialFileOpen,
WriteHeaderWhenInitialFileNotEmpty, ReplaceFileContentsOnEachWrite,
ArchiveOldFileOnStartup, fileWasEmpty, fileStream.Length) accordingly to improve
readability and satisfy SonarCloud.

tests/NLog.Targets.GZipFile.Tests/GZipFileTests.cs (1)

120-128: Make regression preconditions explicit in target setup.

To avoid this test silently drifting with future default changes, set the key preconditions explicitly (especially the startup/archive/open mode that exercises this path).

🧪 Suggested test setup hardening

                     cfg.ForLogger().WriteTo(new GZipFileTarget()
                     {
                         FileName = logFileName,
                         Layout = "${message}",
                         LineEnding = LineEndingMode.LF,
+                        KeepFileOpen = true,
+                        ArchiveOldFileOnStartup = true,
+                        CompressionLevel = CompressionLevel.Optimal,
                         Header = "Hello World",
                         Footer = "Goodbye World",
                         WriteBom = true,
                     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/NLog.Targets.GZipFile.Tests/GZipFileTests.cs` around lines 120 - 128,
The test relies on specific target startup/archive/open behavior but doesn't
lock those preconditions; update the GZipFileTarget setup (the new
GZipFileTarget instance used in the test where FileName, Layout, LineEnding,
Header, Footer, WriteBom are set) to explicitly set the startup/archive/open
mode properties so the exercised path can't change with defaults—for example set
StartMode (or equivalent startup property), ArchiveMode (or archive rotation
property), and file-open behavior properties such as KeepFileOpen and
ConcurrentWrite (or OpenFileShare/OpenFileCache settings) to the values required
by the test; ensure those properties are set on the same GZipFileTarget instance
so the test remains deterministic.

src/NLog/Targets/FileTarget.cs (1)

1331-1338: Harden IsNewFileEmpty against non-seekable streams.

Line 1335's exact-type matching with GetType() is brittle for wrapped streams. More critically, line 1337's unconditional access to fileStream.Length can throw NotSupportedException for non-seekable streams (e.g., GZipStream from GZipFileTarget) when initialFileOpen=false during file reopens, since the type guard at line 1335 only applies when initialFileOpen=true. Use capability checks (CanSeek) instead of runtime type matching.

♻️ Proposed refactor

 private bool IsNewFileEmpty(bool initialFileOpen, Stream fileStream)
 {
     if (ReplaceFileContentsOnEachWrite)
         return true;
-    if (initialFileOpen && ArchiveOldFileOnStartup && !typeof(FileStream).Equals(fileStream.GetType()))
+    if (initialFileOpen && ArchiveOldFileOnStartup && !fileStream.CanSeek)
         return true;
+    if (!fileStream.CanSeek)
+        return false;
     return fileStream.Length == 0;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Targets/FileTarget.cs` around lines 1331 - 1338, The IsNewFileEmpty
method currently uses exact type comparison and unguarded access to
fileStream.Length which can throw for non-seekable or wrapped streams; update
IsNewFileEmpty to return true when ReplaceFileContentsOnEachWrite is set, and
when initialFileOpen is true and ArchiveOldFileOnStartup is set but the provided
stream cannot seek (use fileStream.CanSeek instead of
typeof(FileStream).Equals(fileStream.GetType())), and before reading Length
ensure fileStream.CanSeek is true (otherwise treat as non-empty/return true as
appropriate for your logic) to avoid NotSupportedException on streams like
GZipStream.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/NLog/Targets/FileTarget.cs (1)

1331-1338: Consider a more defensive check for non-seekable streams.

The current logic only avoids fileStream.Length when both ArchiveOldFileOnStartup is true AND the stream is non-seekable. If a future subclass overrides CreateFileStream to return a non-seekable stream without setting ArchiveOldFileOnStartup=true, this would throw NotSupportedException.

The implementation works correctly for GZipFileTarget since it falls back to a regular FileStream when ArchiveOldFileOnStartup=false. However, a more defensive approach would check CanSeek first:

🛡️ Optional defensive refactor

 private bool IsNewFileEmpty(bool initialFileOpen, Stream fileStream)
 {
     if (ReplaceFileContentsOnEachWrite)
         return true;
-    if (ArchiveOldFileOnStartup && !fileStream.CanSeek)
-        return initialFileOpen;    // NLog.Targets.GZipFile expects newly opened files to be empty
+    if (!fileStream.CanSeek)
+        return initialFileOpen && ArchiveOldFileOnStartup;    // NLog.Targets.GZipFile expects newly opened files to be empty
     return fileStream.Length == 0;
 }

This ensures fileStream.Length is never accessed on non-seekable streams, returning false (non-empty) as a safe fallback when conditions aren't met.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Targets/FileTarget.cs` around lines 1331 - 1338, The IsNewFileEmpty
method must avoid accessing fileStream.Length on non-seekable streams; update
the logic in IsNewFileEmpty to first check ReplaceFileContentsOnEachWrite
(return true), then check fileStream.CanSeek and if false return
ArchiveOldFileOnStartup ? initialFileOpen : false, and only if the stream is
seekable evaluate ArchiveOldFileOnStartup or fileStream.Length == 0; this
ensures fileStream.Length is never called for non-seekable streams (refer to
method IsNewFileEmpty and flags ReplaceFileContentsOnEachWrite and
ArchiveOldFileOnStartup).

tests/NLog.Targets.AtomicFile.Tests/AtomFileTests.cs (1)

42-45: Global state mutation may affect test isolation.

Setting LogManager.ThrowExceptions = true in the constructor modifies global state, which could interfere with parallel test execution. Since the test creates its own LogFactory instance (line 55), consider setting ThrowExceptions on that specific factory instead:

♻️ Preferred: Configure the specific LogFactory instance

 public AtomFileTests()
 {
-    LogManager.ThrowExceptions = true;
 }

 [Fact]
 public void SimpleAtomicFileStream_HeaderFooterBom()
 {
     // ...
     try
     {
-        var logFactory = new LogFactory().Setup().LoadConfiguration(cfg =>
+        var logFactory = new LogFactory();
+        logFactory.ThrowExceptions = true;
+        logFactory.Setup().LoadConfiguration(cfg =>

Alternatively, if LogManager.ThrowExceptions is intentional for all tests, consider implementing IDisposable to restore the original value after tests complete.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/NLog.Targets.AtomicFile.Tests/AtomFileTests.cs` around lines 42 - 45,
The test constructor currently sets the global LogManager.ThrowExceptions = true
which mutates global state; instead, remove that global assignment in
AtomFileTests() and set the ThrowExceptions flag on the specific LogFactory
instance you create for the tests (the LogFactory created in the test setup) by
assigning factory.ThrowExceptions = true right after constructing the
LogFactory; if the global change was intentional for all tests, implement
IDisposable on AtomFileTests to save the original LogManager.ThrowExceptions
value in the constructor and restore it in Dispose() to avoid leaking global
state across tests.