src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)

123-136: The SetAccessControl call appears ineffective; consider removing or fixing.

The FileSecurity object created on line 123 has no access rules added to it. Calling SetAccessControl(fileSecurity) on line 136 with this empty security descriptor doesn't actually inherit permissions from the parent directory—it does nothing meaningful.

If the intent is to inherit parent ACLs, the file should already inherit them automatically when created on NTFS (via the fileSecurity parameter passed to Create). The SetAccessRuleProtection(false, false) call ensures inheritance is not blocked, but calling SetAccessControl afterward with the same empty object is redundant.

Consider either:

1. Removing line 136 and the misleading comment on line 135, or
2. Actually retrieving and applying the parent directory's ACL if explicit inheritance copying is needed.

♻️ Suggested simplification

             fileStream = fileInfo.Create(
                 FileMode.CreateNew,
                 systemRights,
                 fileShare,
                 bufferSize: 1,
                 FileOptions.None,
                 fileSecurity: fileSecurity);
-
-            // inherit permissions from parent directory
-            fileInfo.SetAccessControl(fileSecurity);
         }

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 123 - 136, The
call to fileInfo.SetAccessControl(fileSecurity) in AtomicFileTarget.cs is
redundant because the FileSecurity instance has no rules and
SetAccessRuleProtection(false, false) merely allows inheritance; either remove
the SetAccessControl call and the misleading "inherit permissions" comment, or
if explicit copying is required, fetch the parent directory's FileSecurity
(e.g., DirectoryInfo.GetAccessControl()) and apply that object to the file
instead; update the code around fileInfo.Create(...) and the comment near
SetAccessControl to reflect the chosen approach.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs (1)

44-45: Consider making the enum internal to match its namespace intent.

The Win32FileAttributes enum is declared as public but resides in the NLog.Internal namespace. Since this is an implementation detail for the Win32 interop layer, marking it internal would prevent it from being part of the public API surface.

♻️ Suggested change

     [Flags]
-    public enum Win32FileAttributes : int
+    internal enum Win32FileAttributes : int

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs` around lines 44
- 45, The Win32FileAttributes enum is declared public but lives in the
NLog.Internal namespace and should be an implementation detail; change its
accessibility from public to internal by updating the Win32FileAttributes enum
declaration to internal (ensure no external code relies on its public visibility
and update any unit tests or callers if needed).

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj (1)

37-42: ⚠️ Potential issue | 🟠 Major

Prevent dual resolution of NLog.Targets.AtomicFile in this mode.

With this explicit <Reference> enabled, the unconditional <ProjectReference> at Line 45 can still be included, so MSBuild may resolve the same assembly from two paths. Gate the ProjectReference to the complementary condition.

🔧 Proposed fix

-  <ItemGroup>
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net8.0' OR '$(TestTargetFramework)' == '' ">
     <ProjectReference Include="..\..\src\NLog.Targets.AtomicFile\NLog.Targets.AtomicFile.csproj" />
   </ItemGroup>

#!/bin/bash
# Verify whether ProjectReference is still unconditional while explicit Reference is conditional.
python - <<'PY'
import xml.etree.ElementTree as ET
p = "tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj"
root = ET.parse(p).getroot()

def local(tag): return tag.split('}', 1)[-1]

project_ref_conditions = []
explicit_ref_conditions = []

for itemgroup in root:
    if local(itemgroup.tag) != "ItemGroup":
        continue
    cond = itemgroup.attrib.get("Condition", "").strip()
    for child in itemgroup:
        name = local(child.tag)
        include = child.attrib.get("Include", "")
        if name == "ProjectReference" and include.endswith("NLog.Targets.AtomicFile.csproj"):
            project_ref_conditions.append(cond)
        if name == "Reference" and include == "NLog.Targets.AtomicFile":
            explicit_ref_conditions.append(cond)

print("Explicit <Reference> conditions:", explicit_ref_conditions)
print("ProjectReference conditions:", project_ref_conditions)
print("Has unconditional ProjectReference:", any(c == "" for c in project_ref_conditions))
PY

🤖 Prompt for AI Agents

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

In `@tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj`
around lines 37 - 42, The explicit <Reference Include="NLog.Targets.AtomicFile">
is conditionally added when Condition=" '$(TargetFramework)' != 'net8.0' AND
'$(TestTargetFramework)' != '' ", but the corresponding <ProjectReference> for
NLog.Targets.AtomicFile remains unconditional and can cause dual resolution;
update the <ProjectReference Include="...NLog.Targets.AtomicFile.csproj"> block
so it is only present when the explicit Reference is not used (i.e., add the
complementary Condition to that ItemGroup or ProjectReference—e.g., the inverse
of "'$(TargetFramework)' != 'net8.0' AND '$(TestTargetFramework)' != ''") to
ensure only one resolution path is active.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs (1)

82-84: Enum values are GENERIC_ constants, not FILE_GENERIC_*.*

The values 0x80000000, 0x40000000, and 0x20000000 correspond to GENERIC_READ, GENERIC_WRITE, and GENERIC_EXECUTE respectively. The actual FILE_GENERIC_* constants are composite values combining multiple rights. While these members aren't used in the current implementation, the naming could cause confusion when searching Win32 documentation.

This aligns with the reviewer feedback in PR comments suggesting renaming to match Win32 defines for easier searching.

♻️ Suggested rename to match Win32 defines

-            FILE_GENERIC_READ = 0x80000000,
-            FILE_GENERIC_WRITE = 0x40000000,
-            FILE_GENERIC_EXECUTE = 0x20000000,
+            GENERIC_READ = 0x80000000,
+            GENERIC_WRITE = 0x40000000,
+            GENERIC_EXECUTE = 0x20000000,

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs` around lines 82 - 84,
Rename the enum members FILE_GENERIC_READ, FILE_GENERIC_WRITE, and
FILE_GENERIC_EXECUTE in NativeMethods.cs to GENERIC_READ, GENERIC_WRITE, and
GENERIC_EXECUTE respectively, since their hex values (0x80000000, 0x40000000,
0x20000000) map to the Win32 GENERIC_* constants; update any references (e.g.,
usages in methods within the same file or other files) to the new member names
to keep semantics identical and make the names searchable against Win32 docs.

src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)

184-189: Consider using Dispose() or a using statement for cleaner resource management.

The current cleanup logic works, but the pattern could be simplified. Since SafeFileHandle implements IDisposable, calling Dispose() is more idiomatic than checking IsClosed and calling Close().

♻️ Simplified cleanup using Dispose

             catch
             {
-                if ((handle != null) && (!handle.IsClosed))
-                    handle.Close();
+                handle?.Dispose();
                 throw;
             }

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 184 - 189,
Replace the manual SafeFileHandle cleanup in the catch block with Dispose() (or
use a using statement where the SafeFileHandle is created) to rely on
IDisposable instead of checking IsClosed and calling Close(); specifically
update the catch that references the local variable handle (SafeFileHandle) in
AtomicFileTarget.cs so it calls handle.Dispose() (or refactor the handle
acquisition into a using block) before rethrowing to simplify and make resource
management idiomatic.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)

175-177: Minor style inconsistency with brace placement.

Line 175 uses K&R style (opening brace on same line), while the rest of the file uses Allman style (opening brace on new line). Consider aligning for consistency.

♻️ Suggested style fix

-                if (handle.IsInvalid) {
+                if (handle.IsInvalid)
+                {
                     System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(System.Runtime.InteropServices.Marshal.GetHRForLastWin32Error());
                 }

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 175 - 177, The
if statement checking handle.IsInvalid uses K&R brace placement; change it to
Allman style to match the file by placing the opening brace on the next line so
the block for the condition "if (handle.IsInvalid)" matches the rest of the
file's brace convention (the block that calls
System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(...)).

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs (1)

87-96: Consider documenting the FileMode parameter constraint.

The P/Invoke accepts FileMode directly for dwCreationDisposition. While .NET FileMode values 1-5 align with Win32 creation disposition constants, FileMode.Append (6) is invalid for Win32 and must be converted by callers. A comment clarifying this constraint would improve maintainability.

📝 Suggested documentation

+        /// <summary>
+        /// Creates or opens a file or I/O device.
+        /// </summary>
+        /// <remarks>
+        /// Note: FileMode.Append is not a valid Win32 creation disposition.
+        /// Callers must convert Append to OpenOrCreate and use FILE_APPEND_DATA access rights.
+        /// </remarks>
         [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
         [DefaultDllImportSearchPaths(DllImportSearchPath.System32)]
         public static extern Microsoft.Win32.SafeHandles.SafeFileHandle CreateFile(

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs` around lines 87 - 96,
The CreateFile P/Invoke signature currently accepts a FileMode for the
dwCreationDisposition parameter; add a brief XML comment above the CreateFile
method explaining that .NET FileMode values 1–5 map to Win32 creation
dispositions but FileMode.Append (value 6) is not a valid Win32 disposition and
must be converted by callers (e.g., treat Append as OpenOrCreate with an
explicit file pointer move) before calling CreateFile to avoid invalid behavior.
Reference CreateFile and the FileMode dwCreationDisposition parameter in the
comment so maintainers know the constraint and conversion responsibility.

src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)

136-157: Method is specialized for FileMode.Append despite accepting generic FileMode parameter.

The method accepts FileMode fileMode but only adds write access (FILE_APPEND_DATA) when fileMode == FileMode.Append. Other file modes would result in a handle without write permissions. Since this is currently only called with FileMode.Append, this works correctly, but the signature is misleading.

Consider either:

1. Removing the fileMode parameter and hardcoding FileMode.Append behavior, or
2. Adding validation/support for other modes if future callers might use them.

♻️ Option 1: Simplify to Append-only

-        private static FileStream CreateWindowsStream(string filePath, FileMode fileMode, FileShare fileShare, FileOptions fileOptions)
+        private static FileStream CreateWindowsStream(string filePath, FileShare fileShare, FileOptions fileOptions)
         {
             Microsoft.Win32.SafeHandles.SafeFileHandle? handle = null;
             try
             {

-                NativeMethods.Win32FileAccess dwDesiredAccess = NativeMethods.Win32FileAccess.SYNCHRONIZE;
-                if (fileMode == FileMode.Append)
-                {
-                    dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA;
-                }
+                NativeMethods.Win32FileAccess dwDesiredAccess = NativeMethods.Win32FileAccess.SYNCHRONIZE | NativeMethods.Win32FileAccess.FILE_APPEND_DATA;
                 if ((fileOptions & FileOptions.DeleteOnClose) != 0)
                 {
                     dwDesiredAccess |= NativeMethods.Win32FileAccess.DELETE; // required by FILE_DELETE_ON_CLOSE
                 }

-                // Must use a valid Win32 constant here...
-                if (fileMode == FileMode.Append)
-                {
-                    fileMode = FileMode.OpenOrCreate;
-                }
+                FileMode fileMode = FileMode.OpenOrCreate;

🤖 Prompt for AI Agents

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

In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 136 - 157, The
CreateWindowsStream method currently accepts a FileMode parameter but only
implements append semantics (adds FILE_APPEND_DATA and forces
FileMode.OpenOrCreate), which is misleading; change CreateWindowsStream to be
append-only by removing the FileMode parameter and hardcoding append behavior:
update the method signature (remove the fileMode argument), always include
NativeMethods.Win32FileAccess.FILE_APPEND_DATA in dwDesiredAccess, set the
effective fileMode to FileMode.OpenOrCreate inside the method, remove the
conditional branch that mutates fileMode, and update all callers to invoke
CreateWindowsStream without a FileMode argument; alternatively, if you want to
support other modes, add validation and branch logic to grant appropriate access
bits for FileMode.Create, FileMode.Truncate, FileMode.Open, etc., but prefer the
simpler append-only refactor given current usage in CreateWindowsStream.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro