fix: resolve -1 sentinel collision for NV ops (-M, -C, -A)

[Koan-Bot]

What

Fix silent fallback for -M/-C/-A when the mocked value is exactly -1.0.

Why

FALLBACK_TO_REAL_OP is -1. For NV ops, -1.0 is a valid result (file
modified exactly 1 day in the future). The XS handler pp_overload_ft_nv
checked SvNV(status) == -1, causing any mocked file with
mtime == basetime + 86400 to silently fall back to the real OP — returning
undef for a nonexistent path instead of the expected -1.0.

How

• XS: Replace _overload_ft_ops_sv() (G_SCALAR, sentinel-based) with
  _overload_ft_ops_nv() (G_ARRAY) that receives a (status, value) pair.
  The status code (-1/fallback, -2/null, 1/success) is separate from the NV.
• Perl: Split %OP_CAN_RETURN_INT into int-only (s) and %OP_RETURNS_NV
  (M, C, A). _check() returns (CHECK_IS_TRUE, $value) for NV ops.
  _check_from_stat() wraps NV results in scalar refs to distinguish real
  values from FALLBACK_TO_REAL_OP.

Testing

• New t/nv-sentinel-collision.t — 6 subtests covering -M/-C/-A with
  mtime/atime/ctime at basetime+86400, FALLBACK for non-mocked files,
  direct mock FALLBACK, and other NV value pass-through.
• Full make test passes (all existing tests + new test).

🤖 Generated with Claude Code

---

Quality Report

Changes: 3 files changed, 214 insertions(+), 35 deletions(-)

Code scan: clean

Tests: passed (0 Tests)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[atoomic]

@Koan-Bot rr

[Koan-Bot]

PR Review — fix: resolve -1 sentinel collision for NV ops (-M, -C, -A)

The approach of separating the status code from the NV value via a (status, value) pair is sound and correctly addresses the -1 sentinel collision. However, there are two bugs that need fixing before merge: (1) the count variable is corrupted by the while loop before being used in the croak error message in _overload_ft_ops_nv, and (2) the new $OP_RETURNS_NV block in _check() is placed after the !$out falsy check, meaning NV ops returning exactly 0.0 via direct mock callbacks will be incorrectly treated as CHECK_IS_FALSE. The missing pTHX_/dMY_CXT could break ithreads support added in a recent commit.

---

🔴 Blocking

1. count corrupted before croak format string (`FileCheck.xs`, L160-163)

The while (count-- > 0) loop decrements count to -1 before the croak() call. The error message will always print -1 instead of the original count.

Save the original count before the loop:

else {
    int orig_count = count;
    while (count-- > 0) (void)POPs;
    croak("Overload::FileCheck::_check returned %d values for NV OP #%d, expected 1 or 2\n", orig_count, optype);
}

    while (count-- > 0) (void)POPs;
    croak("Overload::FileCheck::_check returned %d values for NV OP #%d, expected 1 or 2\n", count, optype);

🟡 Important

1. Missing pTHX_ and dMY_CXT in _overload_ft_ops_nv (`FileCheck.xs`, L122)

The existing _overload_ft_ops_sv (visible in the repo at line 122) uses pTHX and dMY_CXT for ithreads safety — this was added in commit 01967e6 ("isolate gl_overload_ft per interpreter via MY_CXT for ithreads safety"). The new _overload_ft_ops_nv function in the diff does not include pTHX_ in its signature or dMY_CXT in its body. If ithreads are enabled, this function won't have access to the correct per-interpreter context. The function signature should be void _overload_ft_ops_nv(pTHX_ int *status_out, NV *nv_out) with dMY_CXT; inside, matching the existing convention.

void _overload_ft_ops_nv(int *status_out, NV *nv_out) {

2. NV ops with falsy value (0.0) fall through to CHECK_IS_FALSE (`lib/Overload/FileCheck.pm`, L486-490)

When the new $OP_RETURNS_NV block is inserted (after the $OP_CAN_RETURN_INT block in the diff), a NV op returning exactly 0.0 (file modified at basetime) will hit the if ( !$out ) check on line 482 before reaching the new NV block, returning CHECK_IS_FALSE instead of the numeric value 0.0.

The new if ( $OP_RETURNS_NV{$optype} ) block must be placed before the if ( !$out ) check, or the !$out guard must exclude NV ops. Otherwise -M on a file with mtime == basetime will incorrectly return false instead of 0.0.

    if ( !$out ) {
        ...
        return CHECK_IS_FALSE;
    }

    # NV ops block comes after this — too late for $out == 0.0

🟢 Suggestions

1. Scalar ref from _check_from_stat masks the 0.0 issue (`lib/Overload/FileCheck.pm`, L476-490)

When using mock_all_from_stat, the NV value is wrapped in a scalar ref (\(...)), so !$out is false (refs are truthy) and the code reaches the NV block correctly. But for direct mock_file_check('-M', sub { 0.0 }) callbacks, $out is bare 0.0 which is falsy. The scalar-ref wrapping strategy only protects the stat path, not the direct mock path. Consider handling NV ops immediately after the undef check and before any truthiness checks on $out.

2. Missing test for -M returning 0.0

The test file covers the -1.0 sentinel collision and FALLBACK, but does not test the edge case where a mock returns exactly 0.0 (file modified at basetime). Given the ordering issue in _check() where !$out would catch 0.0 before the NV block, this is worth testing explicitly.

---

Checklist

• No hardcoded secrets
• Error messages don't expose internals
• Resource cleanup on error paths
• Correct error reporting — critical mock_all_from_stat needs to be able to handle an empty stat return. #1: count corrupted in croak
• Edge case coverage — warning Not all file test operators should be lstat or stat #3: 0.0 falsy edge case
• Thread safety — warning -f via mock_all_from_stat doesn't work. #2: missing pTHX_/dMY_CXT

---

Summary

The approach of separating the status code from the NV value via a (status, value) pair is sound and correctly addresses the -1 sentinel collision. However, there are two bugs that need fixing before merge: (1) the count variable is corrupted by the while loop before being used in the croak error message in _overload_ft_ops_nv, and (2) the new $OP_RETURNS_NV block in _check() is placed after the !$out falsy check, meaning NV ops returning exactly 0.0 via direct mock callbacks will be incorrectly treated as CHECK_IS_FALSE. The missing pTHX_/dMY_CXT could break ithreads support added in a recent commit.

---

Automated review by Kōand435282

[Koan-Bot]

Rebase with requested adjustments

Branch koan.atoomic/fix-nv-sentinel-collision was rebased onto main and review feedback was applied.

Changes applied

• view items are already addressed in the current code:
• pTHX_/dMY_CXT: Already present at lines 134-135 ✓
• NV block ordering: $OP_RETURNS_NV check (line 489) is already before !$out check (line 501) ✓
• Only the croak bug needed fixing. Here's the summary:
• Fixed count corruption in croak error message (FileCheck.xs, _overload_ft_ops_nv): Saved original count to orig_count before the while (count-- > 0) drain loop, so the croak message reports the actual number of values returned instead of always printing -1. Per reviewer's blocking comment.
• pTHX_/dMY_CXT already present: The current code already includes pTHX_ in the function signature and dMY_CXT in the body (lines 134-135), so no change needed.
• NV block ordering already correct: The $OP_RETURNS_NV block (line 489) is already placed before the if ( !$out ) check (line 501) in the current code, so NV ops returning 0.0 are correctly handled without falling through to CHECK_IS_FALSE.

Stats

3 files changed, 215 insertions(+), 35 deletions(-)

Actions performed

• Already-solved check: skipped (Claude call failed)
• Resolved merge conflicts (1 round(s))
• Rebased koan.atoomic/fix-nv-sentinel-collision onto upstream/main
• Applied review feedback
• Pre-push CI check: previous run passed
• Force-pushed koan.atoomic/fix-nv-sentinel-collision to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Kōan