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

87-93: ⚡ Quick win

Add XML documentation for the return value.

The <returns> XML documentation tag is missing for the CreateLinkedHooks method.

📝 Suggested documentation addition

 /// Creates a linked hook combining the current and given <see cref="FileLifecycleHooks"/>.
 /// </summary>
 /// <param name="hooks">Hooks to link with current <see cref="FileLifecycleHooks"/>. Methods on <paramref name="hooks"/> will be invoked after the current <see cref="FileLifecycleHooks"/> methods.</param>
-/// <returns></returns>
+/// <returns>A new <see cref="LinkedFileLifecycleHooks"/> that invokes both hook instances in sequence.</returns>
 public FileLifecycleHooks CreateLinkedHooks(FileLifecycleHooks hooks) =>
     new LinkedFileLifecycleHooks(this, hooks);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/NLog/Targets/FileLifecycleHooks.cs` around lines 87 - 93, The
CreateLinkedHooks method lacks a <returns> XML doc; update the XML documentation
for the CreateLinkedHooks(FileLifecycleHooks hooks) method to include a
<returns> tag that clearly states it returns a new LinkedFileLifecycleHooks
instance that links the current FileLifecycleHooks with the provided hooks
(methods on the provided hooks will be invoked after the current ones),
referencing LinkedFileLifecycleHooks and FileLifecycleHooks in the description.

tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)

150-184: ⚡ Quick win

Strengthen error-path verification by asserting log output was still written.

Hooks_ErrorHandlingTest validates callback recording, but it should also assert that writing still succeeded when hooks throw.

Suggested test addition

 LogManager.Configuration = null;    // Flush

+Assert.True(new FileInfo(logFile).Length > 0);
+
 var expected = new List<string>
 {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/NLog.UnitTests/Targets/FileTargetTests.cs` around lines 150 - 184, The
test Hooks_ErrorHandlingTest currently only asserts the hook callback sequence;
enhance it to also verify that the log entry was actually written despite hooks
throwing by reading the temporary logFile (after the existing flush:
LogManager.Configuration = null) and asserting the file contains the expected
logged text (e.g., "ccc" or the formatted log line); update the method to use
the existing logFile and callRecording variables and add a final assertion like
Assert.Contains("ccc", File.ReadAllText(logFile)) before cleanup.

tests/NLog.UnitTests/Targets/LinkedFileLifecycleHooksTests.cs (1)

98-139: ⚡ Quick win

Extract shared test logic to eliminate duplication.

CreateLinkedHooks and VerifyChainedCalls share nearly identical test logic (lines 107-138 duplicate lines 64-95). The only difference is how the target hook chain is constructed (line 105 vs line 62).

Consider extracting the shared assertion logic into a helper method:

private void VerifyHookChaining(FileLifecycleHooks target, List<string> callRecording)
{
    target.OnFileDeleting("del.log");
    target.OnFileClosed("close.log");
    using (var stream = new MemoryStream())
    {
        var actualStream = target.OnFileOpened("open.log", stream);
        Assert.IsType<MockStream>(actualStream);
        Assert.Equal("hook_2", ((MockStream)actualStream).Name);
    }

    var expectedTarget = new FileTarget("my-file_1");
    target.OnTargetInitialize(expectedTarget);

    expectedTarget = new FileTarget("my-file_2");
    expectedTarget.Dispose();
    target.OnTargetClose(expectedTarget);

    var expected = new List<string>
    {
        "hook_1_OnFileDeleting_del.log",
        "hook_2_OnFileDeleting_del.log",
        "hook_1_OnFileClosed_close.log",
        "hook_2_OnFileClosed_close.log",
        "hook_1_OnFileOpened_open.log_stream",
        "hook_2_OnFileOpened_open.log_hook_1",
        "hook_1_OnTargetInitialize_my-file_1",
        "hook_2_OnTargetInitialize_my-file_1",
        "hook_1_OnTargetClose_my-file_2",
        "hook_2_OnTargetClose_my-file_2"
    };

    Assert.Equal(expected, callRecording);
}

Then both tests become:

[Fact]
public void VerifyChainedCalls()
{
    var callRecording = new List<string>();
    var first = new MockFileLifecycleHooks("hook_1", callRecording);
    var second = new MockFileLifecycleHooks("hook_2", callRecording);
    var target = new LinkedFileLifecycleHooks(first, second);
    
    VerifyHookChaining(target, callRecording);
}

[Fact]
public void CreateLinkedHooks()
{
    var callRecording = new List<string>();
    var first = new MockFileLifecycleHooks("hook_1", callRecording);
    var second = new MockFileLifecycleHooks("hook_2", callRecording);
    var target = first.CreateLinkedHooks(second);
    
    VerifyHookChaining(target, callRecording);
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/NLog.UnitTests/Targets/LinkedFileLifecycleHooksTests.cs` around lines
98 - 139, Tests CreateLinkedHooks and VerifyChainedCalls duplicate the same
sequence of actions and assertions; extract that shared logic into a private
helper (e.g., VerifyHookChaining(FileLifecycleHooks target, List<string>
callRecording)) that performs the
OnFileDeleting/OnFileClosed/OnFileOpened/OnTargetInitialize/OnTargetClose calls
and asserts the expected callRecording; then simplify both tests to construct
MockFileLifecycleHooks("hook_1", callRecording) and
MockFileLifecycleHooks("hook_2", callRecording) and call VerifyHookChaining with
either new LinkedFileLifecycleHooks(first, second) or
first.CreateLinkedHooks(second).

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)

607-610: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve fatal-exception behavior in lifecycle hook catch blocks.

Line 607, Line 1015, Line 1039, and Line 1346 catch Exception and always continue. Add ExceptionMustBeRethrown(ex) guards before logging so fatal exceptions are not swallowed.

🛠️ Proposed fix pattern (apply to all four hook catch blocks)

 catch (Exception ex)
 {
+    if (ExceptionMustBeRethrown(ex))
+        throw;
     InternalLogger.Error(ex, "{0}: {1} hook threw an unexpected error", this, nameof(FileLifecycleHooks.OnTargetClose));
 }

 catch (Exception ex)
 {
+    if (ExceptionMustBeRethrown(ex))
+        throw;
     InternalLogger.Error(ex, "{0}: {1} hook threw an unexpected error for file '{2}'", this, nameof(FileLifecycleHooks.OnFileDeleting), filepath);
 }

 catch (Exception ex)
 {
+    if (ExceptionMustBeRethrown(ex))
+        throw;
     InternalLogger.Error(ex, "{0}: {1} hook threw an unexpected error for file '{2}'", this, nameof(FileLifecycleHooks.OnFileClosed), openFile.FileAppender.FilePath);
 }

 catch (Exception ex)
 {
+    if (ExceptionMustBeRethrown(ex))
+        throw;
     InternalLogger.Error(ex, "{0}: {1} hook threw an unexpected error for file '{2}'", this, nameof(FileLifecycleHooks.OnFileOpened), filePath);
 }

Also applies to: 1015-1018, 1039-1042, 1346-1349

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/NLog/Targets/FileTarget.cs` around lines 607 - 610, In the FileTarget.cs
file, add `ExceptionMustBeRethrown(ex)` guard checks before logging in all four
lifecycle hook catch blocks (the OnTargetClose hook catch block at line 607, and
the other three hook catch blocks at lines 1015, 1039, and 1346). Each catch
block should check if the exception is fatal using `ExceptionMustBeRethrown(ex)`
before calling InternalLogger.Error, and rethrow the exception if it is fatal,
ensuring fatal exceptions are not swallowed by these catch blocks.

Fix all unresolved CodeRabbit comments on this PR:

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