Skip to content

feat(extensions): carry filesystem mtime through Source entity (swamp-club#271)#1348

Merged
stack72 merged 1 commit into
mainfrom
issue-271/source-mtime
May 8, 2026
Merged

feat(extensions): carry filesystem mtime through Source entity (swamp-club#271)#1348
stack72 merged 1 commit into
mainfrom
issue-271/source-mtime

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented May 8, 2026

Summary

  • Add sourceMtime: string to the Source domain entity and thread it through the entire lifecycle — from Deno.stat in reconcile/install services, through domain transitions (observeFreshSource, withState, withFingerprintAndState), to catalog persistence via sourceToRow().
  • withFingerprintAndState takes sourceMtime as a required parameter (not optional) so the compiler catches every call site that forgets to supply mtime — preventing the class of bugs this issue was filed to fix.
  • withState preserves existing mtime (correct asymmetry: state-only transitions like tombstoning don't re-observe the source).
  • Deno.stat in reconcile happens before the try block so failed builds still record the real filesystem mtime (the file is on disk and readable; only bundling failed).

Closes swamp-club#271

Test Plan

  • deno check — 0 type errors (compiler catches all missing sourceMtime call sites)
  • deno lint — clean
  • deno fmt --check — clean
  • deno run test — 5694 passed, 0 failed
  • deno run compile — binary compiles successfully
  • Unit tests verify sourceMtime round-trip through makeSource, withState (preserves), withFingerprintAndState (updates), and observeFreshSource (both new and existing source paths)
  • ExtensionRepository tests verify mtime survives catalog write → read cycle

🤖 Generated with Claude Code

…-club#271)

sourceToRow() hardcoded source_mtime to empty string because the Source
domain entity didn't carry mtime. The ExtensionLoader wrote mtime to the
catalog correctly, but aggregate-level saves via ExtensionRepository
overwrote it with empty string.

Add sourceMtime: string to Source, thread it through makeSource,
withState (preserves), withFingerprintAndState (required param per ADV-1),
and observeFreshSource. Capture mtime via Deno.stat in reconcile and
install services — stat happens before the try block in reconcile so
failed builds still record the real mtime.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — this PR makes no user-visible changes. All 8 modified files are internal: domain entity (source.ts, extension.ts), persistence (extension_repository.ts), and services (install_extension_service.ts, reconcile_from_disk_service.ts) plus their tests. No commands, renderers, output formatters, error messages, flags, or JSON shapes were changed.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

No critical or high severity issues found.

Medium

  1. src/libswamp/extensions/install_extension_service.ts:246Deno.stat not inside try/catch; TOCTOU window
    The Deno.stat(absolutePath) call is unguarded. If the file is deleted between bundleAndIndexOne completing and Deno.stat executing, this throws an unhandled Deno.errors.NotFound that propagates up and aborts the entire install loop for all remaining sources. This is a narrow TOCTOU window and unlikely in practice (the file was just read by the bundler), but a concurrent editor or git checkout could trigger it. The reconcile service correctly handles this by placing Deno.stat outside the try block intentionally (per the PR description), but the install service has no such design intention documented and no catch around it.
    Suggested fix: Wrap it in a try/catch that falls back to "" or skip the source if stat fails.

Low

  1. src/domain/extensions/extension_test.ts:393observeFreshSource: adds new Source in transient Bundled state doesn't assert mtime round-trip
    The test passes sourceMtime: "2026-01-15T10:00:00.000Z" but never asserts that the resulting source carries it through. This is the only test that exercises the new-source (non-existing) path of observeFreshSource with a non-empty mtime, so there's no coverage proving the field survives the new-source branch. Not a code bug — the implementation is clearly correct — just a gap in the test net.

  2. sourceMtime typed as string rather than a branded/opaque type
    The rest of the domain uses branded types for identity fields (SourceFingerprint, SourceLocation, etc.). sourceMtime is a bare string, which means any caller can pass arbitrary non-ISO strings without a type error. Given that the field is persisted to the catalog and may be compared later, a SourceMtime branded type would be more consistent. This is a minor consistency concern, not a correctness issue — the two producers (Deno.stat().mtime?.toISOString() and row.source_mtime ?? "") are both correct.

Verdict

PASS — This is a clean, well-structured change. The sourceMtime field is threaded correctly through all domain transitions. withState preserves mtime (state-only transitions like tombstoning don't re-observe), withFingerprintAndState requires it as a parameter (compiler-enforced), and the persistence layer round-trips it faithfully. All callers of makeSource, withFingerprintAndState, observeFreshSource, and makeExtensionWithNewSource supply the new field. The recordBundleBuildFailed path for existing sources correctly goes through updateSourceStatewithState, which preserves the existing mtime. The reconcile service intentionally places Deno.stat before the try block so that failed builds still get the real mtime — this is documented in the PR description and is correct.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Explicit mtime assertion in repository round-trip test (src/infrastructure/persistence/extension_repository_test.ts): Test #1 (round-trip save→load) and Test #2 (diff-save INSERT) both set sourceMtime values but don't explicitly assert the loaded extensions carry the correct mtime after persistence. The TypeScript type system guarantees the field flows through, but an explicit assertEquals(source.sourceMtime, "2026-01-15T10:00:00.000Z") in the round-trip test would make the claim in the PR description fully provable at the test level.

  2. Explicit mtime assertion in observeFreshSource test (src/domain/extensions/extension_test.ts:377): The "adds new Source in transient Bundled state" test passes sourceMtime: "2026-01-15T10:00:00.000Z" but only asserts on state.tag and sources.size. Adding assertEquals(s.sourceMtime, "2026-01-15T10:00:00.000Z") would close the loop.

Summary

Well-designed change that correctly threads sourceMtime through the Source entity lifecycle following DDD principles:

  • Domain design: sourceMtime is correctly placed on the Source entity (not the aggregate root), and the transition semantics are right — withState preserves existing mtime (state-only transitions like tombstoning don't re-observe the file), while withFingerprintAndState accepts a new mtime (re-observation gets a fresh stat). The field is readonly and immutable, consistent with the existing Source pattern.
  • Required parameter on withFingerprintAndState: Making sourceMtime required (not optional) is the right call — the compiler catches every forgotten call site.
  • Stat placement in reconcile: Moving Deno.stat before the try block so failed builds still record filesystem mtime is a correct design choice.
  • Import boundaries: No violations — domain changes in src/domain/, persistence in src/infrastructure/, services in src/libswamp/.
  • Test coverage: Unit tests cover makeSource, withState (preserves mtime), withFingerprintAndState (updates mtime), observeFreshSource (both new and existing paths), and repository persistence. All existing tests updated with the new required field.
  • No security concerns: mtime comes from trusted Deno.stat, stored via parameterized SQLite queries.

@stack72 stack72 merged commit 0e46f9a into main May 8, 2026
11 checks passed
@stack72 stack72 deleted the issue-271/source-mtime branch May 8, 2026 13:28
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