feat(extract): auto-discover module tsconfig and extract per-module - #267

packages/riviere-extract-ts/README.md (2)

9-9: ⚠️ Potential issue | 🟡 Minor

Stale "Skeleton implementation" status note.

This note predates the current PR's work. The implementation is now substantive (per-module extraction, connection detection, enrichment). The status banner should be removed or updated to reflect the current state.

✏️ Proposed fix

-**Current Status:** Skeleton implementation. Predicate logic and output generation coming in subsequent tasks (see PRD phase-10-typescript-extraction).
-
 ## Installation

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/riviere-extract-ts/README.md` at line 9, Update the README status
line that currently reads "Skeleton implementation. Predicate logic and output
generation coming in subsequent tasks (see PRD phase-10-typescript-extraction)"
to reflect the current substantive implementation: either remove the "Skeleton
implementation" sentence entirely or replace it with a short, accurate status
summary (e.g., "Per-module extraction, connection detection, and enrichment
implemented — see PRD phase-10-typescript-extraction for remaining tasks") so
the README no longer claims the code is a skeleton.

---

42-43: ⚠️ Potential issue | 🟠 Major

README example is non-functional: missing required parameters and imports.

The usage example on lines 42–43 calls extractComponents(sourceFiles, config) with only 2 arguments, but the actual function signature requires:

• project: Project (first parameter)
• sourceFilePaths: string[] (second parameter)
• config: ResolvedExtractionConfig (third parameter—note the type differs from the example's ExtractionConfig)
• globMatcher: GlobMatcher (fourth parameter)
• configDir?: string (optional fifth parameter)

The example also lacks the required imports for Project and matchesGlob. Update the example to match the actual CLI pattern: extractComponents(project, sourceFiles, resolvedConfig, matchesGlob, configDir).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/riviere-extract-ts/README.md` around lines 42 - 43, The README
example calls extractComponents with the wrong signature and is missing imports;
update the snippet to import Project and matchesGlob and call extractComponents
with the proper parameters: create or obtain a Project instance (type Project)
and pass it as the first arg, pass sourceFiles as the second, pass a
ResolvedExtractionConfig (not raw ExtractionConfig) as the third, pass
matchesGlob (type GlobMatcher) as the fourth, and include the optional configDir
string as the fifth if applicable; ensure the example shows importing Project
and matchesGlob and constructing/obtaining resolvedConfig before calling
extractComponents(project, sourceFiles, resolvedConfig, matchesGlob, configDir).

packages/riviere-extract-conventions/src/default-config.spec.ts (1)

77-91: ⚠️ Potential issue | 🟡 Minor

Add glob to requiredKeys for an explicit assertion.

The new 10th key (glob) is verified only by the toHaveLength(10) count — any other extra key would also satisfy the test. Listing glob in requiredKeys makes the assertion targeted and would catch regressions where glob is missing or renamed.

🛡️ Proposed fix

     const requiredKeys = [
       'name',
       'path',
+      'glob',
       'api',
       'useCase',
       'domainOp',
       'event',
       'eventHandler',
       'eventPublisher',
       'ui',
     ]
     const moduleKeys = Object.keys(module)
     expect(moduleKeys).toStrictEqual(expect.arrayContaining(requiredKeys))
     expect(moduleKeys).toHaveLength(10)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/riviere-extract-conventions/src/default-config.spec.ts` around lines
77 - 91, The test builds requiredKeys and asserts moduleKeys contains them and
has length 10; add the missing 'glob' string to the requiredKeys array (in the
test that defines requiredKeys/moduleKeys) so the explicit assertion checks for
'glob' rather than relying solely on the toHaveLength(10) check; update the
requiredKeys array (the variable named requiredKeys in default-config.spec.ts)
to include 'glob'.

packages/riviere-extract-config/extraction-config.schema.json (1)

44-63: ⚠️ Potential issue | 🟡 Minor

Stale module description — "path pattern" now refers to glob, not path.

Line 46 still reads "A module defines extraction rules for a path pattern". With this PR, path is a directory (not a pattern) and the pattern lives in glob. The description should be updated to reflect the new two-field semantics.

📝 Suggested fix

-      "description": "A module defines extraction rules for a path pattern",
+      "description": "A module defines extraction rules for a directory path and source file glob pattern",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/riviere-extract-config/extraction-config.schema.json` around lines
44 - 63, Update the schema description for the "module" object to reflect the
new semantics: change the sentence "A module defines extraction rules for a path
pattern" to indicate that a module defines extraction rules for a module
directory ("path") combined with a file pattern ("glob"); reference the "module"
object and its "path" (module root directory) and "glob" (glob pattern for
source files) properties so the description clearly states that path is a
directory and glob is the pattern used to match files.

packages/riviere-extract-config/src/validation-extends.spec.ts (1)

85-96: ⚠️ Potential issue | 🟡 Minor

Test "no path" case now also omits glob, obscuring the intent

After making glob required, this test exercises "neither path nor glob present" rather than isolating a missing-path scenario. The assertion still passes, but the test no longer proves the validator specifically rejects a missing path. Adding glob here would make this test precise.

🔧 Proposed fix to isolate the missing-path case

    it('returns error when module has extends but no path', () => {
      const result = validateExtractionConfig({
        modules: [
          {
            name: 'orders',
+           glob: '**',
            extends: '@living-architecture/riviere-extract-conventions',
          },
        ],
      })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/riviere-extract-config/src/validation-extends.spec.ts` around lines
85 - 96, The test "returns error when module has extends but no path" now also
omits glob, so it actually tests "neither path nor glob present"; update the
test case in validation-extends.spec.ts to include a valid glob on the module
(e.g., add a glob property) while leaving path absent so
validateExtractionConfig is exercised specifically for missing path; keep the
same assertions (expect result.valid to be false and the error path to include
'/modules/0') and reference the validateExtractionConfig call and the module
object with name 'orders' and extends
'@living-architecture/riviere-extract-conventions' so the intent is clear.

tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts (2)

247-295: 🧹 Nitpick | 🔵 Trivial

Identical mock setup/teardown duplicated across two tests — extract a shared helper.

The Object.defineProperty + console.log spy setup (and matching teardown) is copied verbatim into both "default IO" tests. Regardless of whether vi.spyOn is adopted (see comment above), this should be extracted into a helper or beforeEach/afterEach scoped to a nested describe block to eliminate duplication and reduce maintenance surface.

♻️ Suggested structure

describe('uses default IO (no io option)', () => {
  beforeEach(() => {
    vi.spyOn(process, 'exit').mockImplementation(vi.fn() as never)
    vi.spyOn(console, 'log').mockImplementation(vi.fn())
  })
  // afterEach handled automatically by vi.restoreAllMocks() in outer beforeEach

  it('exits with 0 on success', async () => { ... })
  it('writeFile is a noop when resolveTimingsFilePath provided', async () => { ... })
})

Also applies to: 298-346

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts`
around lines 247 - 295, The test duplicates the mock/teardown setup for
process.exit, process.stdout.write and console.log across two "uses default IO"
tests; extract this into a shared setup/teardown helper (or a nested describe
with beforeEach/afterEach) so both specs reuse it. Move the
Object.defineProperty mocks (or replace with vi.spyOn(process,
'exit').mockImplementation(...) and vi.spyOn(process.stdout,
'write').mockImplementation(...)) and the console.log spy (mockLog) into the
shared beforeEach, and restore/restoreAllMocks in afterEach (or rely on outer
restoreAllMocks) so individual tests only declare steps, buildContext and call
runWorkflow; keep references to runWorkflow, mockLog, process.exit and
process.stdout.write to locate the code to change.

---

247-295: ⚠️ Potential issue | 🟠 Major

process.exit / process.stdout.write mocks are not cleaned up if assertions fail.

Both tests mock these globals via Object.defineProperty and restore them at the end of the test body. If vi.waitFor(...) times out or throws, the restore code never runs. Because these mocks are not created via vi.spyOn, vi.restoreAllMocks() in beforeEach won't clean them up either, leaving subsequent tests with a stubbed process.exit.

Per testing conventions, prefer vi.spyOn so that beforeEach's vi.restoreAllMocks() handles cleanup automatically. If Object.defineProperty is truly required for process.stdout.write, at minimum wrap the assertion in try/finally.

🔒 Proposed fix — switch to `vi.spyOn` and remove manual teardown

  it('uses default IO when io option not provided', async () => {
-   const originalExit = process.exit
-   const originalWrite = process.stdout.write
-   const mockExit = vi.fn()
-   const mockWrite = vi.fn(
-     (
-       _data: string | Uint8Array,
-       encodingOrCb?: BufferEncoding | ((error?: Error | null) => void),
-       cb?: (error?: Error | null) => void,
-     ): boolean => {
-       const callback = typeof encodingOrCb === 'function' ? encodingOrCb : cb
-       if (callback) callback()
-       return true
-     },
-   )
-   Object.defineProperty(process, 'exit', {
-     value: mockExit,
-     configurable: true,
-   })
-   Object.defineProperty(process.stdout, 'write', {
-     value: mockWrite,
-     configurable: true,
-   })
-   const mockLog = vi.spyOn(console, 'log').mockImplementation(vi.fn())
+   const mockExit = vi.spyOn(process, 'exit').mockImplementation(vi.fn() as never)
+   const mockLog = vi.spyOn(console, 'log').mockImplementation(vi.fn())
    // ... test body ...
    await vi.waitFor(() => {
      expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('result'))
      expect(mockExit).toHaveBeenCalledWith(0)
    })
-   Object.defineProperty(process, 'exit', { value: originalExit, configurable: true })
-   Object.defineProperty(process.stdout, 'write', { value: originalWrite, configurable: true })
-   mockLog.mockRestore()
  })

Also applies to: 298-346

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts`
around lines 247 - 295, The test stubs process.exit and process.stdout.write
with Object.defineProperty which are not restored if vi.waitFor throws; change
these to use vi.spyOn(process, 'exit') and vi.spyOn(process.stdout, 'write') (or
wrap the test assertions in a try/finally) so vi.restoreAllMocks() in beforeEach
cleans them up automatically; update both test cases that call runWorkflow and
use mockLog/vi.waitFor to create spies via vi.spyOn(...) for process.exit and
process.stdout.write and remove the manual Object.defineProperty teardown (or
add try/finally around the assertions if Object.defineProperty must remain).
Ensure references to process.exit, process.stdout.write, runWorkflow, mockLog,
and vi.waitFor are updated accordingly.

packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/enrich-components.spec.ts (1)

86-87: ⚠️ Potential issue | 🟡 Minor

notUsedModule calls still pass old glob-embedded paths, leaving fixtures in an inconsistent state.

notUsedModule now always appends glob: '**', so calling it with '/src/orders/**' produces a module with path: '/src/orders/**' and glob: '**'. If enrich-components.ts resolves the glob by joining module.path and module.glob (as stated in the PR description), the effective pattern would be /src/orders/**/**.

The tests still pass because alwaysMatch/neverMatch bypass glob evaluation, but the fixtures are inconsistent with the new convention and will mislead readers.

🐛 Proposed fix

-        [notUsedModule('orders', '/src/orders/**')],  // line 86
+        [notUsedModule('orders', '/src/orders')],
 
-      const result = enrich([], [notUsedModule('orders', '/src/orders/**')])  // line 107
+      const result = enrich([], [notUsedModule('orders', '/src/orders')])
 
-      const config: ResolvedExtractionConfig = {modules: [notUsedModule('other', '/src/other/**')],}  // line 217
+      const config: ResolvedExtractionConfig = {modules: [notUsedModule('other', '/src/other')],}

Also applies to: 107-107, 217-217

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/enrich-components.spec.ts`
around lines 86 - 87, The fixtures call notUsedModule with a glob-embedded path
(e.g., '/src/orders/**') but notUsedModule now appends module.glob = '**'
itself, causing duplicate globs; update the test fixtures to pass base
directories without the trailing '/**' (e.g., '/src/orders') wherever
notUsedModule is used (references: notUsedModule in the spec and the glob
resolution behavior in enrich-components.ts) so module.path holds the base path
and module.glob remains '**', keeping fixture expectations consistent.

tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts (3)

228-243: ⚠️ Potential issue | 🟡 Minor

"Does not write" tests pass before the workflow finishes — false positive risk.

vi.waitFor(() => expect(...).toHaveLength(0)) resolves immediately on the first poll because the collection is empty before the workflow has had a chance to execute. If a regression were introduced that causes a delayed write, these tests would still pass.

Contrast with the "writes" tests (e.g., line 220), which correctly block until a positive signal appears.

The fix is to first confirm workflow completion via mockIO.exitCode, then assert the absence:

-    await vi.waitFor(() => {
-      expect(mockIO.writeFileCalls).toHaveLength(0)
-    })
+    await vi.waitFor(() => expect(mockIO.exitCode).toBeDefined())
+    expect(mockIO.writeFileCalls).toHaveLength(0)

Apply the same pattern to the filter-based assertion at line 280:

-    await vi.waitFor(() => {
-      expect(mockIO.writeFileCalls.filter((c) => c.path.endsWith('output.json'))).toHaveLength(0)
-    })
+    await vi.waitFor(() => expect(mockIO.exitCode).toBeDefined())
+    expect(mockIO.writeFileCalls.filter((c) => c.path.endsWith('output.json'))).toHaveLength(0)

Also applies to: 267-282

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts`
around lines 228 - 243, The test currently uses vi.waitFor(() =>
expect(mockIO.writeFileCalls).toHaveLength(0)) which can pass before the
workflow runs; update the test to wait for workflow completion by awaiting the
mockIO.exitCode (e.g., await vi.waitFor(() =>
expect(mockIO.exitCode).toBeDefined() or a specific code) after calling
runWorkflow(…) and only then assert
expect(mockIO.writeFileCalls).toHaveLength(0); apply the same change to the
filter-based assertion that inspects mockIO.writeFileCalls (replace its
immediate waitFor check with first waiting for mockIO.exitCode and then
asserting the filtered result is empty).

---

19-31: ⚠️ Potential issue | 🟡 Minor

Add Error.captureStackTrace to both test error classes.

Both TestExitSignal and ContextBuildError are missing Error.captureStackTrace, which is required by the coding guidelines for this path.

🛠️ Proposed fix

 class TestExitSignal extends Error {
   constructor() {
     super('process.exit called')
     this.name = 'TestExitSignal'
+    Error.captureStackTrace(this, TestExitSignal)
   }
 }

 class ContextBuildError extends Error {
   constructor() {
     super('context error')
     this.name = 'ContextBuildError'
+    Error.captureStackTrace(this, ContextBuildError)
   }
 }

Based on learnings: "Custom error classes must use Error.captureStackTrace for cleaner stack traces." (applies to tools/dev-workflow/**/*.{ts,tsx})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts`
around lines 19 - 31, Both custom error classes TestExitSignal and
ContextBuildError need Error.captureStackTrace called in their constructors to
produce proper stack traces; update each constructor to call
Error.captureStackTrace(this, TestExitSignal) and Error.captureStackTrace(this,
ContextBuildError) respectively (after super(...)) and keep the name assignments
as-is so they comply with the project's coding guidelines for custom errors.

---

304-403: ⚠️ Potential issue | 🟠 Major

Resource leak: process.exit / process.stdout.write not cleaned up on assertion failure, and setup is duplicated.

Both tests restore process.exit and process.stdout.write after await vi.waitFor(...). If vi.waitFor times out or an assertion inside it throws, the Object.defineProperty restores on lines 344–351 and 394–401 are never reached. vi.restoreAllMocks() in beforeEach does not restore Object.defineProperty overrides (it only restores vi.spyOn spies), so a failing test leaves the global mocks active for subsequent tests.

The same 20-line Object.defineProperty boilerplate is duplicated verbatim across both tests, making future maintenance error-prone.

Recommended fix: Use vi.spyOn (which vi.restoreAllMocks() handles automatically) and move the spy setup into a nested describe with beforeEach/afterEach to eliminate the duplication:

♻️ Proposed refactor

+  describe('default IO behavior', () => {
+    let mockExit: ReturnType<typeof vi.fn>
+    let mockWrite: ReturnType<typeof vi.fn>
+    let mockLog: ReturnType<typeof vi.spyOn>
+
+    beforeEach(() => {
+      mockExit = vi.fn()
+      mockWrite = vi.fn(
+        (
+          _data: string | Uint8Array,
+          encodingOrCb?: BufferEncoding | ((error?: Error | null) => void),
+          cb?: (error?: Error | null) => void,
+        ): boolean => {
+          const callback = typeof encodingOrCb === 'function' ? encodingOrCb : cb
+          if (callback) callback()
+          return true
+        },
+      )
+      vi.spyOn(process, 'exit').mockImplementation(mockExit)
+      vi.spyOn(process.stdout, 'write').mockImplementation(mockWrite)
+      mockLog = vi.spyOn(console, 'log').mockImplementation(vi.fn())
+    })
+
+    // vi.restoreAllMocks() in the outer beforeEach handles cleanup automatically
+
     it('uses default IO when io option not provided', async () => {
-      const originalExit = process.exit
-      const originalWrite = process.stdout.write
-      const mockExit = vi.fn()
-      const mockWrite = vi.fn(...)
-      Object.defineProperty(process, 'exit', { value: mockExit, configurable: true })
-      Object.defineProperty(process.stdout, 'write', { value: mockWrite, configurable: true })
-      const mockLog = vi.spyOn(console, 'log').mockImplementation(vi.fn())
       const steps: Step<BaseContext>[] = [...]
       const buildContext = async () => ({ branch: 'test' })

       runWorkflow(steps, buildContext)

       await vi.waitFor(() => {
         expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('result'))
         expect(mockExit).toHaveBeenCalledWith(0)
       })
-
-      Object.defineProperty(process, 'exit', { value: originalExit, configurable: true })
-      Object.defineProperty(process.stdout, 'write', { value: originalWrite, configurable: true })
-      mockLog.mockRestore()
     })

     it('uses default IO writeFile (noop) when resolveTimingsFilePath provided without io', async () => {
-      // same Object.defineProperty boilerplate
       const steps: Step<BaseContext>[] = [...]
       const buildContext = async () => ({ branch: 'test' })

       runWorkflow(steps, buildContext, undefined, { resolveTimingsFilePath: () => 'timings.md' })

       await vi.waitFor(() => {
         expect(mockExit).toHaveBeenCalledWith(0)
       })
-
-      // same restore boilerplate
     })
+  })

Based on learnings: "When mocking globals, use vi.spyOn instead of direct property reassignment to prevent mock leaks if tests throw before restore is reached." (applies via **/*.{test,spec}.{ts,tsx} → docs/conventions/testing.md)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/dev-workflow/platform/infra/workflow-execution/run-workflow.spec.ts`
around lines 304 - 403, Replace the direct Object.defineProperty overrides for
process.exit and process.stdout.write with vi.spyOn-based mocks (e.g.,
vi.spyOn(process, 'exit').mockImplementation(mockExit) and
vi.spyOn(process.stdout, 'write').mockImplementation(mockWrite)) and move that
setup/teardown into a nested describe with beforeEach to create the mocks and
afterEach to call vi.restoreAllMocks() (or explicitly mockRestore) so mocks are
always cleaned even if vi.waitFor/assertions throw; update both tests to use the
shared describe and remove the duplicated Object.defineProperty blocks
(referencing process.exit and process.stdout.write in the changes).