fix(riviere-cli): enforce entrypoint layering - #372

packages/riviere-cli/src/features/extract/entrypoint/extract/extract.pr-extraction.spec.ts (1)

311-331: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

errorSpy.mockRestore() is not guarded — spy leaks if the await rejects.

If parseCommandWithErrorHandling rejects unexpectedly (e.g., a misconfiguration in CI), mockRestore() on line 328 is never reached and the console.error spy persists into the next test.

🛡️ Proposed fix: wrap in `try/finally`

-  await parseCommandWithErrorHandling([
-    'node',
-    'riviere',
-    'extract',
-    '--config',
-    configPath,
-    '--pr',
-    '--base',
-    'main',
-    '--components-only',
-  ])
-
-  errorSpy.mockRestore()
-
-  expect(stderrOutput.some((msg) => msg.includes('untracked'))).toBe(true)
+  try {
+    await parseCommandWithErrorHandling([
+      'node',
+      'riviere',
+      'extract',
+      '--config',
+      configPath,
+      '--pr',
+      '--base',
+      'main',
+      '--components-only',
+    ])
+  } finally {
+    errorSpy.mockRestore()
+  }
+
+  expect(stderrOutput.some((msg) => msg.includes('untracked'))).toBe(true)

🤖 Prompt for AI Agents

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

In
`@packages/riviere-cli/src/features/extract/entrypoint/extract/extract.pr-extraction.spec.ts`
around lines 311 - 331, The test currently sets up errorSpy (vi.spyOn(console,
'error')) then awaits parseCommandWithErrorHandling but calls
errorSpy.mockRestore() unguarded so the spy can leak if the await rejects; wrap
the invocation of parseCommandWithErrorHandling and the subsequent mockRestore
in a try/finally block so that errorSpy.mockRestore() always runs, referencing
the existing errorSpy and parseCommandWithErrorHandling identifiers to locate
where to apply the try/finally.

packages/riviere-cli/src/features/builder/entrypoint/validate/validate.spec.ts (1)

256-289: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing test case for the GRAPH_CORRUPTED error path — violates mandatory 100% coverage.

The entrypoint.ts else-branch maps any !result.success code that is not 'GRAPH_NOT_FOUND' to CliErrorCode.GraphCorrupted. This branch is never exercised in this spec; only the GRAPH_NOT_FOUND path is covered. Given mandatory 100% coverage, add a case that writes a malformed graph file and asserts CliErrorCode.GraphCorrupted appears in the output.

✅ Proposed additional test

+    it('returns GRAPH_CORRUPTED when graph file is malformed', async () => {
+      const graphPath = join(ctx.testDir, '.riviere/graph.json')
+      await mkdir(dirname(graphPath), { recursive: true })
+      await writeFile(graphPath, 'not valid json')
+      await createProgram().parseAsync(['node', 'riviere', 'builder', 'validate'])
+      expect(ctx.consoleOutput.join('\n')).toContain(CliErrorCode.GraphCorrupted)
+    })

🤖 Prompt for AI Agents

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

In
`@packages/riviere-cli/src/features/builder/entrypoint/validate/validate.spec.ts`
around lines 256 - 289, Add a test in validate.spec.ts under the "error
handling" describe that exercises the entrypoint.ts branch mapping non-success
results to CliErrorCode.GraphCorrupted: use createGraph or directly write a
malformed graph JSON file into ctx.testDir (e.g., invalid JSON or missing
required fields), then run
createProgram().parseAsync(['node','riviere','builder','validate','--graph',
malformedPath]) and assert that ctx.consoleOutput contains
CliErrorCode.GraphCorrupted; reference the existing tests and utilities
(createGraph, createProgram, parseOutput, CliErrorCode) to mirror style and
ensure the corrupted-graph branch is executed.

packages/riviere-cli/src/features/builder/entrypoint/component-checklist/entrypoint.ts (1)

32-50: 🧹 Nitpick | 🔵 Trivial

Remove the async keyword on line 32 — componentChecklist.execute() is synchronous and does not require await.

The ComponentChecklist.execute() method returns ComponentChecklistResult directly, not a Promise. The method is synchronous, so the async keyword on the action handler is unnecessary. The code works correctly as-is, but the async declaration should be removed for clarity and to avoid false implications about async operations.

Proposed fix

-    .action(async (options: ComponentChecklistOptions) => {
+    .action((options: ComponentChecklistOptions) => {
       if (options.type !== undefined && !isValidComponentType(options.type)) {

🤖 Prompt for AI Agents

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

In
`@packages/riviere-cli/src/features/builder/entrypoint/component-checklist/entrypoint.ts`
around lines 32 - 50, Remove the unnecessary async on the action handler since
ComponentChecklist.execute is synchronous: change the handler signature from
.action(async (options: ComponentChecklistOptions) => { ... }) to
.action((options: ComponentChecklistOptions) => { ... }); ensure there are no
await expressions inside the handler (referencing componentChecklist.execute and
ComponentChecklistResult) and leave the logic and error handling unchanged.

packages/riviere-cli/src/features/query/entrypoint/entry-points/entry-points.spec.ts (1)

155-163: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a test for error output without --json.

The graph-load failure path in entrypoint.ts outputs unconditionally (not gated on --json), but the only error test (line 159) passes --json. This means the test passes whether or not the error output is gated on --json — a future accidental regression where --json gating is applied to the error path would go undetected.

✅ Suggested additional test

  it('returns GRAPH_NOT_FOUND when no graph exists', async () => {
    await createProgram().parseAsync(['node', 'riviere', 'query', 'entry-points', '--json'])
    expect(ctx.consoleOutput.join('\n')).toContain(CliErrorCode.GraphNotFound)
  })
+
+  it('returns GRAPH_NOT_FOUND without --json flag (errors are always surfaced)', async () => {
+    await createProgram().parseAsync(['node', 'riviere', 'query', 'entry-points'])
+    expect(ctx.consoleOutput.join('\n')).toContain(CliErrorCode.GraphNotFound)
+  })

🤖 Prompt for AI Agents

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

In
`@packages/riviere-cli/src/features/query/entrypoint/entry-points/entry-points.spec.ts`
around lines 155 - 163, Add a second test that verifies the non-JSON error path:
duplicate the existing spec that calls createProgram().parseAsync(['node',
'riviere', 'query', 'entry-points']) without the '--json' flag and assert that
ctx.consoleOutput.join('\n') contains CliErrorCode.GraphNotFound; this ensures
the graph-load failure message printed by the entry-points command (see
entry-points.spec.ts using createProgram, parseAsync, ctx.consoleOutput, and
CliErrorCode.GraphNotFound) is covered for the non-JSON output case.

packages/riviere-cli/src/features/query/entrypoint/trace/trace.spec.ts (2)

171-183: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Error output without --json is untested.

The entrypoint outputs graph-load failures unconditionally (no --json check), but both error tests pass --json. If error output were accidentally gated on --json in the future, these tests would still pass.

Add a companion test without --json to lock in the unconditional-output contract, mirroring the same gap in entry-points.spec.ts.

🤖 Prompt for AI Agents

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

In `@packages/riviere-cli/src/features/query/entrypoint/trace/trace.spec.ts`
around lines 171 - 183, Add a companion non-JSON test in trace.spec.ts that
asserts the same unconditional error output when no graph exists: duplicate the
existing "returns GRAPH_NOT_FOUND when no graph exists" test but remove the
'--json' flag, call createProgram().parseAsync([...
'query','trace','any:mod:type:component']), capture console output via
parseOutput(ctx.consoleOutput) and assert via expectErrorOutput(...) that
output.error.code equals CliErrorCode.GraphNotFound (mirroring the gap fixed in
entry-points.spec.ts) to lock in the unconditional error-emission contract.

---

217-238: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Prefer vi.spyOn over direct prototype mutation.

Directly assigning to queryClass.prototype.traceFlow bypasses TypeScript's type-checking on the replacement function and requires manual try/finally cleanup. vi.spyOn is type-safe, self-documenting, and lets vi.restoreAllMocks() handle teardown automatically — which setupCommandTest likely already calls in afterEach.

♻️ Proposed refactor

-      const originalTraceFlow = queryClass.prototype.traceFlow
-
-      queryClass.prototype.traceFlow = () => {
-        throw new TestAssertionError('Unexpected internal error')
-      }
-
-      try {
-        await expect(
-          createProgram().parseAsync([
-            'node',
-            'riviere',
-            'query',
-            'trace',
-            'orders:checkout:api:place-order',
-            '--json',
-          ]),
-        ).rejects.toThrow('Unexpected internal error')
-      } finally {
-        queryClass.prototype.traceFlow = originalTraceFlow
-      }
+      vi.spyOn(queryClass.prototype, 'traceFlow').mockImplementation(() => {
+        throw new TestAssertionError('Unexpected internal error')
+      })
+
+      await expect(
+        createProgram().parseAsync([
+          'node',
+          'riviere',
+          'query',
+          'trace',
+          'orders:checkout:api:place-order',
+          '--json',
+        ]),
+      ).rejects.toThrow('Unexpected internal error')

🤖 Prompt for AI Agents

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

In `@packages/riviere-cli/src/features/query/entrypoint/trace/trace.spec.ts`
around lines 217 - 238, Replace the direct prototype mutation of
queryClass.prototype.traceFlow with a vi.spyOn on the RiviereQuery class method:
import the module as queryModule, get queryModule.RiviereQuery, and call
vi.spyOn(queryClass.prototype, 'traceFlow').mockImplementation(() => { throw new
TestAssertionError('Unexpected internal error') }); remove the manual
try/finally that restores originalTraceFlow and rely on the test harness (e.g.
setupCommandTest/vi.restoreAllMocks) to teardown mocks; keep the
createProgram().parseAsync(...) expect(...).rejects.toThrow assertion unchanged.

packages/riviere-cli/src/platform/infra/cli/presentation/output.ts (1)

3-18: ⚠️ Potential issue | 🔴 Critical

Add missing export keywords and @riviere-role annotations to SuccessOutput and ErrorOutput interfaces.

output-writer.ts line 3 imports SuccessOutput (import { type SuccessOutput }), but the interfaces in output.ts are not exported. This is a TypeScript compilation error. Both interfaces must be exported and must include the required @riviere-role annotation per the coding guidelines.

Proposed fix

+/** `@riviere-role` cli-response-formatter */
+export interface SuccessOutput<T> {
   success: true
   data: T
   warnings: string[]
 }
 
+/** `@riviere-role` cli-response-formatter */
+export interface ErrorOutput {
   success: false
   error: {
     code: string
     message: string
     suggestions: string[]
   }
 }

🤖 Prompt for AI Agents

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

In `@packages/riviere-cli/src/platform/infra/cli/presentation/output.ts` around
lines 3 - 18, The interfaces SuccessOutput and ErrorOutput are not exported and
lack the required `@riviere-role` annotation; update the declarations for
SuccessOutput<T> and ErrorOutput to include the export keyword and add the
`@riviere-role` annotation above each interface declaration so they are publicly
available and comply with coding guidelines (locate the interface declarations
SuccessOutput and ErrorOutput in this file and add the export modifier and the
annotation).

packages/riviere-cli/src/features/query/entrypoint/components/components.errors.spec.ts (1)

67-82: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

console.error spy not restored on assertion failure.

If either assertion at lines 80–81 fails, errorSpy.mockRestore() is skipped and the console.error mock leaks into subsequent tests, potentially suppressing real errors or causing false results. Explicitly scope teardown outside the assertion path using a beforeEach/afterEach pair or check whether restoreMocks: true is set in the Vitest config.

🛡️ Proposed fix using a scoped afterEach

-  it('outputs error to stderr when --type is invalid and --json not provided', async () => {
-    await createGraph(ctx.testDir, {
+  describe('outputs error to stderr when --type is invalid and --json not provided', () => {
+    const errorOutput: string[] = []
+    let errorSpy: ReturnType<typeof vi.spyOn>
+
+    beforeEach(() => {
+      errorSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation((msg: string) => errorOutput.push(msg))
+    })
+
+    afterEach(() => {
+      errorSpy.mockRestore()
+      errorOutput.length = 0
+    })
+
+    it('outputs error message', async () => {
+      await createGraph(ctx.testDir, {
         // ... graph setup unchanged ...
-    })
-
-    const errorOutput: string[] = []
-    const errorSpy = vi
-      .spyOn(console, 'error')
-      .mockImplementation((msg: string) => errorOutput.push(msg))
-
-    await createProgram().parseAsync([
-      // ...
-    ])
-
-    expect(errorOutput).toHaveLength(1)
-    expect(errorOutput[0]).toContain('Invalid component type: InvalidType')
-    errorSpy.mockRestore()
-  })
+      })
+
+      await createProgram().parseAsync([
+        // ...
+      ])
+
+      expect(errorOutput).toHaveLength(1)
+      expect(errorOutput[0]).toContain('Invalid component type: InvalidType')
+    })
+  })

🤖 Prompt for AI Agents

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

In
`@packages/riviere-cli/src/features/query/entrypoint/components/components.errors.spec.ts`
around lines 67 - 82, The test creates a console.error spy (errorSpy) but calls
errorSpy.mockRestore() only after assertions, so if an expectation throws the
mock is not restored and leaks; fix by restoring the spy in a teardown path
(e.g., move the mocking into beforeEach and call errorSpy.mockRestore() in
afterEach, or wrap the parse/assert block in a try/finally and restore errorSpy
in finally) — update the test that calls createProgram().parseAsync([...]) and
the errorSpy setup/teardown so console.error is always restored regardless of
test failures.

.riviere/role-definitions/cli-output-formatter.md (1)

15-22: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider updating the canonical example to reflect delegation guidance.

The behavioral contract (lines 9-10) states that output formatters should delegate writing to cli-response-writer, but the canonical example directly calls console.log. This inconsistency may confuse implementers about whether direct output is acceptable within this role.

🤖 Prompt for AI Agents

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

In @.riviere/role-definitions/cli-output-formatter.md around lines 15 - 22, The
canonical example violates the role contract by calling console.log directly;
update the presentExtractionResult example to delegate all output to the
cli-response-writer interface instead of printing directly (i.e., replace
console.log usage with a call to the CLI response writer provided by the
cli-response-writer role so both the formatted table
(formatTable(result.components)) and the extraction outcome are written via that
writer). Ensure the example references presentExtractionResult and formatTable
and shows using the cli-response-writer API to emit both lines.