fix(custody): treat an undefined watch filename as a directory-level change

Closed
IC
1mo ago
+37-2

filenameMatches in the custody manifest watcher guards filename === null, but Node's fs.watch listener can also be invoked with undefined. When it is, the guard falls through and .toString() throws.

# Unhandled error between tests
TypeError: undefined is not an object (evaluating 'filename.toString')
  at filenameMatches (packages/opencode/src/custody-manifest-watcher.ts:51:31)
  at <anonymous> (packages/opencode/src/custody-manifest-watcher.ts:142:16)
  at #onEvent (node:fs:49:70)

The declared parameter type is string | Buffer | null, so TypeScript never flags the case — the value simply isn't supposed to exist, and at runtime it does.

Why it is worth fixing rather than tolerating

This throws inside an fs.watch callback, not on a request path, so there is no caller to catch it. In CI it surfaces as 1932 pass, 0 fail, 1 error — a red gate with no failing test to point at, which is a genuinely confusing signal to land on someone's PR. In production the same event kills the watcher callback, and the manifest watcher is one half of the pair that keeps custody bindings reconciled (the low-frequency metadata poll is the other). Losing it degrades silently: reconciliation just gets slower, with nothing to say why.

undefined here carries the same meaning null already does — Node could not attribute the change to a specific entry — so the fix routes it to the existing behaviour: reconcile the whole directory.

-  filename: string | Buffer | null,
+  filename: string | Buffer | null | undefined,
   expected: string,
 ): boolean {
-  return filename === null || filename.toString() === expected
+  // Node may omit the filename (undefined) when it cannot attribute the change
+  // to a specific entry; treat that like null and reconcile the whole directory.
+  return filename == null || filename.toString() === expected
 }

Widening the type matters as much as the guard: it makes the case visible to the next reader instead of leaving a runtime possibility the signature denies.

Verification

The test drives the production callback through the module's existing watchImpl seam rather than calling the helper directly — a helper-level test would keep passing if the caller stopped using it.

Red-first, against unfixed code — the real TypeError, not a tidy assertion failure, confirming the test reaches the production path:

error: expect(received).not.toThrow()
Error message: "undefined is not an object (evaluating 'filename.toString')"
(fail) custody manifest watcher registry > treats an undefined filename as a directory-level change

Two mutations, both re-run independently of the implementation:

mutationresult
revert guard + typeTypeError: undefined is not an object
keep the widened type, but make undefined mean no match instead of reconcileExpected: > 1 / Received: 1

The second is the one that matters: it proves the test asserts the reconcile actually happens, not merely that nothing threw. A fix that swallowed the event silently — which would have looked fine and quietly disabled the watcher — does not pass.

Gates: core 295/0 · opencode 1927/0 · typecheck clean. Clean main measures 1926 (1924 pass + 2 known intermittents, #220); the +1 is this test.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the custody manifest watcher crashing when Node invokes the fs.watch callback with an undefined filename, which previously threw a TypeError and silently degraded reconciliation.

  • Treats an undefined filename like null and reconciles the whole directory instead of throwing.
  • Widens the parameter type to include undefined so the runtime case is visible in the signature.
  • Adds a test that drives the production callback to verify the reconcile happens, not merely that nothing throws.

RetriggerConfidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or repository-rule issues identified.

Summary

This PR safely handles undefined filenames from fs.watch as directory-level changes, preventing an exception and preserving custody-manifest reconciliation.

  • Widens the watcher filename type to reflect observed runtime behavior.
  • Treats both null and undefined as unattributed directory changes.
  • Adds a production-path regression test that verifies reconciliation occurs rather than merely suppressing the exception.

Reviews (1) · Last reviewed commit: "fix(opencode): handle undefined filename..."