Skip to content

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

Closed
iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:fix/watcher-undefined-filename
Closed

iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:fix/watcher-undefined-filename

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

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:

mutation result
revert guard + type TypeError: undefined is not an object
keep the widened type, but make undefined mean no match instead of reconcile Expected: > 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.

Written for commit 58422fe. Summary will update on new commits.

Review in cubic

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..."

Node's fs.watch listener can be invoked with an undefined filename, not
only null. The guard fell through to filename.toString() on undefined and
threw a TypeError from the watch callback, surfacing as an unhandled error
between tests. Treat a missing filename like null and reconcile the
directory, and widen the parameter type so the possibility is visible.
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Swept the class rather than just the instance — there are two fs.watch listeners in the tree, and the other one is already safe:

tui-preferences.ts:333-339

(_event, filename) => {
  const isOurs =
    filename === name ||
    (filename?.startsWith(`${name}.`) && filename.endsWith('.tmp'))
  if (filename != null && !isOurs) return

Optional chaining plus != null already covers undefined: an omitted filename yields a falsy isOurs, the != null guard does not fire, and the handler falls through to reconcile — the same semantics this PR gives the manifest watcher. No second fix needed.

So the difference between the two sites is only that one used === null and the other != null. That is the whole defect: == null / != null covers both, === null covers one, and the declared type made the gap invisible in the only place it mattered.

@iceteaSA

Copy link
Copy Markdown
Contributor Author

No longer applies: custody-manifest-watcher.ts was deleted on main (c2f3428). Closing.

@iceteaSA iceteaSA closed this Sep 24, 2026
@iceteaSA
iceteaSA deleted the fix/watcher-undefined-filename branch October 17, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant