Repository navigation
feat(extract): add EventPublisherDef convention and async connection detection (#238) - #251
Conversation
…detection Add EventPublisherDef marker interface and EventPublisherContainer decorator to riviere-extract-conventions. Implement publish-side async detection (Publisher → Event via method parameter type matching) and subscribe-side async detection (Event → EventHandler via subscribedEvents metadata). Wire both async detectors into detectConnections() alongside the existing sync call graph. Co-Authored-By: Claude Opus 4.5 <[email protected]>
… detection In strict mode, throw ConnectionDetectionError when a parameter type matches multiple Event components. In lenient mode, return an _uncertain link instead of multiple concrete links. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds EventPublisher component type across extraction schema and decorators, implements publish/subscribe async connection detection, refactors CLI architecture for better separation of concerns, and enhances dev-workflow with architecture-review agent and improved command orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant Publisher as EventPublisher
participant Event as Event Component
participant Detector as Async Detector
participant ProjectAnalysis as Project Analysis
Publisher->>Detector: detectPublishConnections(project, components)
Detector->>ProjectAnalysis: Find publisher class in AST
ProjectAnalysis-->>Detector: Publisher class found
Detector->>PublisherClass: Extract public methods
loop For each method with parameter
PublisherClass-->>Detector: Method parameter type
Detector->>Event: Find Event with matching eventName
alt Exact match found
Event-->>Detector: Single Event matched
Detector-->>Detector: Create ExtractedLink (async)
else No match (strict mode)
Detector-->>Detector: Throw ConnectionDetectionError
else No match (lenient mode)
Detector-->>Detector: Return _uncertain link
else Multiple matches (strict mode)
Detector-->>Detector: Throw ConnectionDetectionError
else Multiple matches (lenient mode)
Detector-->>Detector: Return _uncertain link
end
end
Detector-->>Publisher: Return ExtractedLink[] (async)
sequenceDiagram
participant EventHandler as EventHandler
participant MetaData as Component Metadata
participant Detector as Async Detector
participant Event as Event Component
EventHandler->>Detector: detectSubscribeConnections(components, options)
Detector->>MetaData: Read subscribedEvents metadata
MetaData-->>Detector: Event names array
loop For each subscribedEvent name
Detector->>Event: Find Event with matching eventName
alt Exact match found
Event-->>Detector: Single Event matched
Detector-->>Detector: Create ExtractedLink (async)
else No match (strict mode)
Detector-->>Detector: Throw ConnectionDetectionError
else No match (lenient mode)
Detector-->>Detector: Return _uncertain link
else Multiple matches (strict mode)
Detector-->>Detector: Throw ConnectionDetectionError
else Multiple matches (lenient mode)
Detector-->>Detector: Return _uncertain link
end
end
Detector-->>EventHandler: Return ExtractedLink[] (async)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying living-architecture with
|
| Latest commit: |
dedb63a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b2370e92.living-architecture.pages.dev |
| Branch Preview URL: | https://issue-238-m1-d1-7-d1-5-d1-6.living-architecture.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/riviere-extract-conventions/src/default-config.spec.ts (1)
73-90:⚠️ Potential issue | 🟡 MinorUpdate the test description to match the new component count.
The test now includes
eventPublisher, so “6 required component types” is no longer accurate.✏️ Proposed fix
- it('declares all 6 required component types', () => { + it('declares all 7 required component types', () => {packages/riviere-extract-ts/src/domain/value-extraction/enrich-components.ts (1)
75-104:⚠️ Potential issue | 🟠 MajorAdd
eventPublisherto the built-in type list or enrichment will skip its rule.
getBuiltInRule()now supportseventPublisher, butfindDetectionRule()only recognizes built-ins listed inbuiltInTypes. That makeseventPublisherfall through tocustomTypes, leaving metadata empty for publisher components.✅ Proposed fix
const builtInTypes: readonly string[] = [ 'api', 'useCase', 'domainOp', 'event', 'eventHandler', + 'eventPublisher', 'ui', ]apps/docs/reference/extraction-config/schema.md (1)
45-53:⚠️ Potential issue | 🟡 MinorClarify explicit
notUsedguidance for the new component typeLine 50 adds
eventPublisher, but the module docs still imply component rules are optional. Please add a short note that all standard component types (includingeventPublisher) must be explicitly declared and unused ones set to{ notUsed: true }, to prevent misconfigured modules.📄 Suggested doc tweak
| `eventHandler` | `componentRule` | No | Detection rule for EventHandler components | | `eventPublisher` | `componentRule` | No | Detection rule for EventPublisher components | | `ui` | `componentRule` | No | Detection rule for UI components | | `customTypes` | `Record<string, detectionRule>` | No | User-defined component types with their detection rules | + +Note: Modules should explicitly declare all standard component types (api, useCase, domainOp, event, eventHandler, eventPublisher, ui). For unused types, set `{ notUsed: true }` instead of omitting the field.Based on learnings: Extraction config schemas must define all six required component types per module: api, useCase, domainOp, event, eventHandler, and ui. Unused component types must be explicitly marked with
notUsed: truerather than omitted.
🤖 Fix all issues with AI agents
In @.claude/commands/next-task.md:
- Around line 103-104: The two bullets "Ask the user to rename the current
session using `/rename issue-<number>`" and "Ask the user to restart the
conversation in the worktree directory to avoid permissions issues" are
ambiguous; update them to explicitly tell the user to (1) run the `/rename
issue-<number>` command as shown, and (2) start a new chat/conversation while
their shell/IDE is pointed at the repository worktree (e.g., open a new terminal
or reopen the IDE with the project root set to the worktree directory) and
include a brief one-sentence explanation of the permissions problem (files or
git refs in other worktrees can be locked or inaccessible to background
processes unless the tool is run from the correct worktree), so readers know
exactly what action to take and why.
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-publish-connections.spec.ts`:
- Around line 67-76: Add a new unit test in detect-publish-connections.spec.ts
to cover the edge case where the components array is empty: call
detectPublishConnections(sharedProject, [], { strict: false }) (and also
consider strict: true if relevant) and assert it returns an empty array;
reference the existing test pattern (e.g., the test named 'returns empty array
when no eventPublisher components exist') and reuse buildComponent/sharedProject
setup to keep consistency while ensuring the components collection edge case is
explicitly validated.
- Around line 1-7: The test currently imports sharedProject from
call-graph-fixtures which shares a single ts-morph Project across many specs and
pollutes symbol resolution; replace that usage by creating a fresh ts-morph
Project instance for this spec (or use a factory that returns a new Project per
file) and pass that new Project into the same helpers used here (e.g.,
buildComponent, nextFile) so detectPublishConnections in this file operates on
an isolated Project; update imports to remove sharedProject and adjust setup in
detect-publish-connections.spec.ts to construct and dispose the Project for this
test file.
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-publish-connections.ts`:
- Around line 43-55: The code currently uses a coverage-ignore comment and a
defensive undefined check for firstParam; remove the comment and make lineNumber
required by ensuring firstParam cannot be undefined: update the caller or the
local function signature so params is typed as a non-empty array (or validate
earlier and throw/return before reaching this block), then drop the "if
(firstParam === undefined) return []" guard and compute paramType/paramTypeName
and SourceLocation.lineNumber directly using method.getStartLineNumber(); keep
repository as '' and filePath using publisher.location.file to locate the block
(references: params, firstParam, method.getStartLineNumber(), SourceLocation).
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-subscribe-connections.spec.ts`:
- Around line 29-161: Add TS-008 edge-case tests for detectSubscribeConnections:
add a test where an eventHandler has duplicated subscribedEvents (e.g.,
['OrderPlaced','OrderPlaced']) and assert only one link is produced; add a test
with mixed-type subscribedEvents (e.g., ['OrderPlaced', 123, null]) and assert
non-string entries are filtered and only valid string subscriptions are
considered; add tests for ambiguous multi-event matches by creating two Event
components sharing the same metadata.eventName and assert that in strict mode
detectSubscribeConnections throws ConnectionDetectionError while in lenient mode
it returns an uncertain link (_uncertain containing the eventName). Use
buildComponent to construct components and reference detectSubscribeConnections
and ConnectionDetectionError in assertions.
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-subscribe-connections.ts`:
- Around line 37-48: Add handling for ambiguous event-name matches: after
computing matchingEvents, if matchingEvents.length > 1 then if options.strict is
true return a single failure link (implement or reuse a helper, e.g.,
handleAmbiguous or extend handleNoMatch) so strict mode fails on ambiguity;
otherwise (lenient) emit one link marked as uncertain by returning a single
connection object using componentIdentity(matchingEvents[0]) as source,
componentIdentity(handler) as target, include sourceLocation:
toSourceLocation(handler) and add an indicator (e.g., uncertain: true or set
type to 'async_uncertain') so callers can detect the ambiguous/uncertain match.
Ensure the existing no-match branch (handleNoMatch) and the normal
matchingEvents.map path remain unchanged.
- Around line 43-47: The returned mapping in matchingEvents.map currently sets
the link type using a redundant assertion "type: 'async' as const"; remove the
unnecessary "as const" so the property is simply "type: 'async'" (the Link type
union and contextual typing will infer the correct LinkType). Update the mapping
returned in detect-subscribe-connections (the matchingEvents.map callback that
uses componentIdentity(event), componentIdentity(handler),
toSourceLocation(handler)) to drop the type assertion.
- Add RequiredLineLocation type alias to avoid v8 ignore comments - Add ambiguous match handling to subscribe-side detection - Add edge case tests: duplicates, mixed types, ambiguous matches - Remove unnecessary `as const` assertions - Clarify worktree restart instructions in next-task command Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-subscribe-connections.spec.ts`:
- Around line 243-245: The helper buildMixedSubscribedEvents currently uses
JSON.parse to produce a runtime array containing non-string values while
declaring string[]; replace the parse with an explicit literal and correct the
type: change the function signature to return (string | number | null)[] and
return a literal like ["OrderPlaced", 123, null] so the test data is type-safe
and honest; update any callers/tests expecting string[] to accept the union type
if necessary.
- Around line 8-241: The test suite for detectSubscribeConnections is missing
string edge-case coverage for empty and whitespace-only subscribed event names;
add new test cases in detect-subscribe-connections.spec.ts that create
eventHandler components (use unique names like EmptyNameHandler and
BlankNameHandler via buildComponent with metadata.subscribedEvents containing ''
and ' '), then assert strict mode throws ConnectionDetectionError for each and
lenient mode returns an uncertain async link (source '_unresolved', target
'orders:eventHandler:<HandlerName>' and _uncertain containing the provided
empty/whitespace string or a descriptive message); place these alongside
existing tests so they validate filtering/behavior for subscribedEvents in
detectSubscribeConnections.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…olations Extract private functions from entrypoint files in riviere-cli and dev-workflow to respect thin-layer and separation-of-concerns rules: - riviere-cli: move 47 private functions to commands/, domain/, and platform/infra/ - dev-workflow: extract stdin helpers, CLI parsers, and push-reflection logic - Fix dependency-cruiser violations with dependency injection (CliReader interface) - Add eclair query file exclusion from max-lines rule - Move misplaced files to correct layers (CS-010 violations) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/riviere-cli/src/features/extract/entrypoint/extract.pr-extraction.spec.ts (1)
356-376: 🧹 Nitpick | 🔵 TrivialConsider wrapping spy cleanup in
try/finallyfor robustness.The
vi.spyOnusage is correct per project conventions, but if theawait createProgram().parseAsync()call or subsequent assertions throw,mockRestore()won't execute. Usingtry/finallyensures cleanup even on test failure.🔧 Proposed defensive cleanup pattern
const stderrOutput: string[] = [] const errorSpy = vi.spyOn(console, 'error').mockImplementation((msg: string) => { stderrOutput.push(String(msg)) }) - await createProgram().parseAsync([ - 'node', - 'riviere', - 'extract', - '--config', - configPath, - '--pr', - '--base', - 'main', - '--components-only', - ]) - - errorSpy.mockRestore() - - expect(stderrOutput.some((msg) => msg.includes('untracked'))).toBe(true) + try { + await createProgram().parseAsync([ + 'node', + 'riviere', + 'extract', + '--config', + configPath, + '--pr', + '--base', + 'main', + '--components-only', + ]) + + expect(stderrOutput.some((msg) => msg.includes('untracked'))).toBe(true) + } finally { + errorSpy.mockRestore() + }Based on learnings: "When mocking globals (
console.error,console.log,window.fetch, etc.), usevi.spyOninstead of direct property reassignment to prevent leaks if the test throws before restore."packages/riviere-cli/src/features/query/entrypoint/orphans.ts (1)
26-33:⚠️ Potential issue | 🟠 MajorAdd non-JSON output handling to all query entrypoints.
The default output is silently dropped across all query commands (orphans, search, entry-points, domains, trace, components). Help text examples demonstrate running without
--json(e.g.,$ riviere query orphans), but no output is printed unless the flag is provided. Each entrypoint needs to output results in human-readable format by default, not just when--jsonis specified.packages/riviere-cli/src/platform/infra/graph-persistence/builder-graph-loader.spec.ts (1)
16-17:⚠️ Potential issue | 🟡 MinorUpdate describe block name to match module.
The describe block is still named
'link-infrastructure'but the tests now coverbuilder-graph-loader. This inconsistency may cause confusion when reviewing test output.Proposed fix
-describe('link-infrastructure', () => { +describe('builder-graph-loader', () => { describe('withGraphBuilder', () => {
🤖 Fix all issues with AI agents
In `@apps/eclair/eslint.config.mjs`:
- Around line 56-61: The new override object (the block containing the files:
['**/queries/**/*.ts'] and rules: { 'max-lines': 'off' }) is misformatted and
triggers `@stylistic/indent` and `@stylistic/object-curly-newline`; reformat that
override so the object curly braces and property lines follow the repo style
(consistent indentation and each property placement/newline rules) — ensure the
files array and rules object are indented to match surrounding overrides and
that the opening/closing { } for the override and the rules object obey
object-curly-newline (properties on their own lines as required) and keep the
'max-lines' rule key exactly as shown.
In `@docs/conventions/anti-patterns.md`:
- Around line 199-201: Add BAD/GOOD examples to the AP-008 section to match
other anti-pattern entries: under the "AP-008: Unused Variables in Shell
Scripts" header add a "✗ Bad" example that shows capturing curl output into
RESPONSE and assigning STATUS with "$?" but never using STATUS, and a "✓ Good"
example that removes the unused STATUS (or uses it) and only echoes RESPONSE;
ensure examples are formatted as bash code blocks and use the same variable
names (RESPONSE, STATUS) as shown in the review suggestion.
In `@eslint.config.mjs`:
- Around line 263-295: This change modifies the root ESLint config (adding the
max-lines rule and new entrypoint-specific 'no-restricted-syntax' selectors:
VariableDeclaration[kind="let"], NewExpression[callee.name="Error"],
FunctionDeclaration:not([parent.type="ExportNamedDeclaration"]), and
VariableDeclarator > ArrowFunctionExpression) and must not be applied without
explicit approval; before committing, obtain documented approval from the repo
owners or architecture/infra team and either revert these rule changes or add a
review-approved sign-off (e.g., PR description note or approval ticket
reference) that is committed alongside the change so reviewers can verify
authorization.
In
`@packages/riviere-cli/src/features/builder/commands/add-component-to-builder.ts`:
- Around line 56-58: The current check conflates missing and invalid API type:
when evaluating "if (!options.apiType || !isValidApiType(options.apiType))" it
always throws MissingRequiredOptionError('api-type', 'API'), which misleads
users who supplied an invalid value. Change the logic in the
add-component-to-builder handler to first check for absence of options.apiType
and throw MissingRequiredOptionError('api-type', 'API'), and then separately
validate with isValidApiType(options.apiType) and throw a validation-specific
error (e.g., InvalidOptionError or a new descriptive Error) including the actual
provided value; update references to options.apiType, isValidApiType, and
MissingRequiredOptionError accordingly.
In `@packages/riviere-cli/src/features/builder/commands/component-adders.ts`:
- Around line 118-134: The current addEventHandlerComponent uses
options.subscribedEvents but only checks for presence, so strings like " , "
become an empty array and produce an invalid EventHandler; update
addEventHandlerComponent to parse subscribedEvents first (split by ',', trim
each entry, filter out empty strings), then validate that the resulting array
has at least one entry and throw MissingRequiredOptionError('subscribed-events',
'EventHandler') if empty before calling builder.addEventHandler; reference the
options.subscribedEvents input handling and the builder.addEventHandler call so
the validation happens on the sanitized array.
- Around line 54-74: The addAPIComponent function currently ignores invalid
options.httpMethod values; change the logic so that when options.httpMethod is
provided you call validateHttpMethod(options.httpMethod) (instead of only
checking isValidHttpMethod) and if validation fails surface/throw the returned
error (or wrap it similarly to MissingRequiredOptionError) so invalid HTTP
methods are rejected consistently before calling RiviereBuilder.addApi; only set
input.httpMethod when validation passes.
In `@packages/riviere-cli/src/features/builder/entrypoint/add-component.ts`:
- Around line 102-137: The current try/catch around addComponentToBuilder and
writeFile masks unexpected I/O errors by converting them into ValidationError;
narrow the scope so only addComponentToBuilder is inside the try (or perform
writeFile after the try) and keep the catch handling only the specific
DomainNotFoundError, CustomTypeNotFoundError and DuplicateComponentError cases;
any other thrown error should be rethrown (or allowed to bubble) so writeFile
failures aren’t reported via formatError/CliErrorCode.ValidationError — update
the block around addComponentToBuilder, writeFile, formatError, CliErrorCode and
getErrorMessage accordingly.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/custom-type-parser.ts`:
- Around line 18-47: parsePropertySpec fails to trim whitespace for the parsed
type and description, causing valid CLI input like "foo: string" to be rejected;
update parsePropertySpec to trim parts after splitting (e.g., assign const
[name, type, description] = parts.map(p => p.trim()) or individually trim type
and description) before validating with isValidPropertyType and before adding
description to the CustomPropertyDefinition, ensuring validation uses the
trimmed type and the stored definition uses trimmed values; keep existing error
messages and VALID_PROPERTY_TYPES reference.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/domain-input-parser.ts`:
- Around line 28-34: parseDomainJson currently lets JSON.parse throw SyntaxError
for malformed JSON while schema mismatches throw InvalidDomainJsonError; wrap
the JSON.parse call in a try/catch inside parseDomainJson, catch any parse
errors and rethrow an InvalidDomainJsonError (including the original error
message/details) so consumers receive a consistent error type; keep the
subsequent isDomainInputParsed check unchanged and include the original parse
error when constructing InvalidDomainJsonError to aid debugging.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/enrichment-error-handler.ts`:
- Around line 2-4: The import statements use overly long relative paths; update
the three imports so they reference sibling modules with simpler paths: change
the import for formatError to import from "./output", change CliErrorCode to
import from "./error-codes", and change handleComponentNotFoundError to import
from "../graph-persistence/builder-graph-loader"; keep the same imported symbols
(handleComponentNotFoundError, formatError, CliErrorCode) and update only the
module specifiers.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/enrichment-parser.ts`:
- Around line 3-12: The parseStateChange function currently accepts empty state
names (e.g., ":" or "foo:") by splitting input into [from, to, ...rest]; update
it to validate that both from and to are non-empty (after trimming) and return
undefined if either is an empty string or contains only whitespace; keep the
existing guard for extra segments (rest.length > 0) and reference
parseStateChange and the destructuring of [from, to, ...rest] when making the
change.
- Around line 49-70: The return type of buildBehavior is too broad (uses object)
— update its signature to a precise shape reflecting possible keys so callers
get correct types: replace the return type with either an explicit typed union
such as { behavior: { reads?: TypeOfReads[]; validates?: TypeOfValidates[];
modifies?: TypeOfModifies[]; emits?: TypeOfEmits[] } } | Record<string, never>
(or {}), using the actual element types from BehaviorOptions (reference
BehaviorOptions.reads/validates/modifies/emits) so the returned behavior object
is strongly typed; keep the existing runtime logic in buildBehavior unchanged
and only refine the function signature to reference those specific types.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/link-http-errors.ts`:
- Around line 1-8: Move the import of CliErrorCode so all imports are grouped at
the top of the file: place "import { CliErrorCode } from './error-codes'" above
the ApiComponentSummary interface (alongside "import type { HttpMethod }" and
"import { formatError }"), ensuring no imports appear after the interface and
preserving the existing interface name ApiComponentSummary and any references to
CliErrorCode in this module.
In `@packages/riviere-cli/src/platform/infra/cli-presentation/output-writer.ts`:
- Around line 13-25: The current try/catch around writeFileSync swallows system
I/O errors and mislabels them as CliErrorCode.ValidationError; remove the
try/catch so writeFileSync(options.output, JSON.stringify(data)) can throw and
fail loudly (preferred), or if you must catch, rethrow the original error after
logging and use the correct error code (e.g., CliErrorCode.RuntimeError) and
include the original error.message/stack when calling formatError; update
references to writeFileSync, options.output, CliErrorCode.ValidationError,
ExitCode.RuntimeError, and formatError accordingly.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/signature-parser.ts`:
- Around line 97-114: The parser currently treats "param:type ->" as valid
because an empty returnType is ignored; after computing arrowIndex and
returnType in signature-parser.ts (variables: arrowIndex, returnType,
paramsPart, trimmed) add a validation that if arrowIndex !== -1 and returnType
=== '' (empty after trim) you return a failure result (same format as the
existing error) indicating an invalid signature; do this before calling
parseParameters/buildSignatureObject so signatures with a present arrow but no
return type are rejected.
- Around line 41-52: The current parseParameters function uses
paramsPart.split(',') (and parseParameter which splits on ':') which breaks
complex TypeScript types containing commas/colons (generics, object/function
types); update parseParameters to use a bracket/quote-aware tokenizer (or call
the existing custom-type parser) that tracks matching <> [] {} and quoted
strings to split top-level parameters only, then pass each top-level param
string to parseParameter; if you decide to restrict supported types instead, add
explicit validation in parseParameters to detect nested delimiters and return a
failure result with a clear error message and add tests to cover generics,
object and function type cases.
- Around line 10-18: Trim the split parts before validating so whitespace-only
values are rejected: in the signature parsing logic (the variables name, type,
description derived from parts), call .trim() on each part first, then check
that trimmed name and type are non-empty and return undefined if they are; only
include description in the returned object when the trimmed description is
non-empty. Ensure you update the validation that currently checks name === '' or
type === '' to operate on the trimmed values (and reference the same trimmed
variables when building the returned object).
In `@packages/riviere-cli/src/platform/infra/extraction-config/config-loader.ts`:
- Around line 59-67: Move the inline PackageConfigNotFoundError class into the
centralized errors module (../errors/errors.ts), implement it there as an
exported class named PackageConfigNotFoundError that extends Error and includes
a readonly packageName: string property set from the constructor (matching the
pattern used by PackageResolveError), preserve the custom message and this.name
override, then remove the inline definition from config-loader.ts and import {
PackageConfigNotFoundError } from '../errors/errors' where it’s used.
In
`@packages/riviere-cli/src/platform/infra/source-filtering/filter-source-files.ts`:
- Around line 65-69: ResolveFilterOptions duplicates the existing FilterOptions
interface; remove the duplicate by replacing all uses of ResolveFilterOptions
with the shared FilterOptions type and delete the ResolveFilterOptions
declaration. Update any function signatures and variable annotations that
referenced ResolveFilterOptions (e.g., ResolveFilterOptions in the resolve*
functions or parameters) to use FilterOptions so the code remains DRY and types
stay consistent.
In
`@packages/riviere-extract-ts/src/domain/connection-detection/async-detection/detect-subscribe-connections.spec.ts`:
- Around line 124-145: Add tests in detect-subscribe-connections.spec.ts that
cover the remaining TS-008 string edge-cases by creating events via
buildComponent and handlers with subscribedEvents and metadata.eventName using
very long strings (e.g., >1k chars), Unicode/special characters (emojis, RTL
scripts, accented letters), injection-like patterns (e.g., newline/NULL/SQL-like
payloads), and cases with surrounding/tricky whitespace to ensure
trimming/inexact matches; call detectSubscribeConnections for each scenario
(both strict: true and lenient: false variants where relevant) and assert
expected behavior (exact match yields connection, mismatches yield
no-match/unresolved like '_unresolved' or _uncertain) using the same expectation
style (expect.arrayContaining / expect.objectContaining) as the existing tests
and reusing helper names like WhitespaceHandler/buildComponent to locate test
scaffolding.
In
`@tools/dev-workflow/features/complete-task/domain/complete-task-cli-parser.ts`:
- Around line 64-74: resolveSkipReview currently re-parses the CLI for pr mode
causing duplicate work; change resolveSkipReview to accept a prMode parameter
(e.g., resolveSkipReview(cliReader: CliReader, prMode: PrMode)) and remove the
internal call to parsePRMode; update the caller in buildCompleteTaskContext (and
its upstream call site in complete-task.ts) to pass the already-parsed prMode
returned by parsePRMode(cliReader), and keep the existing behavior of throwing
WorkflowError if prMode !== 'update' when the --reject-review-feedback flag is
present.
In
`@tools/dev-workflow/features/push-reflection/commands/push-reflection.spec.ts`:
- Around line 19-45: Add a new test case to cover the single-element array edge
case: mock mockGit.lastCommitFiles to resolve to an array with exactly one
reflection file (e.g.,
'docs/continuous-improvement/post-merge-reflections/reflection-1.md'), mock
mockGit.push to resolve, call executePushReflection(), and assert the returned
result equals { pushedFiles: [thatFile] } and that mockGit.push was called once;
place this new "it" alongside the existing tests that use
mockGit.lastCommitFiles and mockGit.push so it mirrors the all-files reflection
test but with a single-item array.
In `@tools/dev-workflow/features/push-reflection/commands/push-reflection.ts`:
- Around line 5-20: The two error classes used by the exported function
executePushReflection (EmptyCommitError and NonReflectionFilesError) must be
exported so callers can catch/narrow them; update the class declarations for
EmptyCommitError and NonReflectionFilesError to be exported (export class
EmptyCommitError / export class NonReflectionFilesError) and keep their existing
constructors/messages unchanged so consumers can import these symbols and
perform instanceof checks when executePushReflection throws them.
In `@tools/dev-workflow/features/push-reflection/entrypoint/cli.ts`:
- Around line 3-23: The CLI currently calls executePushReflection() directly;
refactor it to use runWorkflow and the command's context builder instead: import
runWorkflow from workflow-runner and import the command's context builder (e.g.,
buildPushReflectionContext) from the same command directory, then call
runWorkflow(buildPushReflectionContext(...), { steps: [ /* list each step on its
own line referencing executePushReflection or its step functions */ ] });
Declare each step on its own line for readability and remove the direct
executePushReflection() invocation and custom promise handling so runWorkflow
manages execution and error/reporting.
In `@tools/dev-workflow/platform/infra/stdin-reader.ts`:
- Around line 3-8: The JsonParseResult type is currently internal but is the
return type of the exported tryParseJson function; export it so consumers can
use the discriminated union. Update the declaration of JsonParseResult to export
(e.g., export type JsonParseResult = ...) and ensure any references in
tryParseJson's signature use the exported name so external modules can import
and annotate results.
Delete component-adders.ts (unused file) and unexport internal-only functions that were incorrectly exported during extraction. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/signature-parser.ts`:
- Line 84: The arrow detection is inconsistent: the check using
trimmed.startsWith('->') and the later check for ' -> ' should use a unified
pattern; update the logic in signature-parser.ts (the code around the trimmed
variable and the parsing branch that handles return-only signatures and the
branch that handles parameters) to detect arrows with optional surrounding
whitespace (e.g., use a single regex like /\s*->\s*/ or normalize the string and
then split on '->'), and apply that same detection in both the return-type-only
path and the parameters+return path so signatures like "a:string->number" parse
correctly as parameter a:string with return number. Ensure you modify both
occurrences referenced (the startsWith('->') check and the later ' -> ' check)
and adjust subsequent splitting/trimming logic accordingly.
In `@packages/riviere-cli/src/platform/infra/extraction-config/config-loader.ts`:
- Around line 113-115: The isPackageReference function incorrectly treats
Windows absolute paths and UNC paths as package references; import
path.isAbsolute (or destructure isAbsolute from 'path') at the top of the module
and change isPackageReference(source: string) to return !source.startsWith('.')
&& !source.startsWith('/') && !isAbsolute(source); update references to the
isPackageReference function accordingly; add unit tests that assert false for
Windows absolute paths like "C:\\foo\\bar" and for UNC paths like
"\\\\server\\share\\file" (and keep existing POSIX and relative tests) to reach
full coverage.
Add file-based debug logging throughout the complete-task workflow (context building, step execution, code review agents) to diagnose hangs. Add permissionMode to Claude agent SDK calls. Remove v8 ignore comments from untested files and add proper tests to restore 100% coverage. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@tools/dev-workflow/features/complete-task/domain/steps/run-code-review.ts`:
- Line 1: The code currently returns reportPath but never writes parsed.report
to disk; update the step in run-code-review.ts to persist parsed.report to
reportPath (use node:fs/promises writeFile after importing it) by
JSON.stringify(parsed.report, null, 2) (or an appropriate encoding), await the
write, and ensure any directory creation/permissions are handled or errors are
caught and rethrown so the returned reportPath is valid and meaningful; modify
the code that sets/returns reportPath and the parsed.report handling to perform
this write (references: reportPath and parsed.report).
- Around line 52-57: Remove the "as const" assertion on the inline
['task-check'] and instead give the returned array a proper typed literal: drop
the "as const" and annotate the expression (or the function's return type) as
ReadonlyArray<'architecture-review'|'code-review'|'bug-scanner'|'task-check'>
(or readonly
('architecture-review'|'code-review'|'bug-scanner'|'task-check')[]). Keep the
conditional using shouldRunTaskCheck but return shouldRunTaskCheck ?
['task-check'] : [] without "as const" and ensure the function (or the return
expression) is typed to include 'task-check' so the types remain narrow without
using a type assertion.
In
`@tools/dev-workflow/features/merge-and-cleanup/commands/merge-and-cleanup.spec.ts`:
- Around line 61-74: Remove the redundant mock setup in the test: delete the
duplicate mockResolvedValue calls for mockGit.currentBranch and
mockGithub.findPRForBranch inside the 'it' block (they are already configured in
the beforeEach), leaving the test to call executeMergeAndCleanup() and assert
the context; reference mockGit.currentBranch and mockGithub.findPRForBranch to
locate the duplicated lines and ensure only the beforeEach config remains.
In
`@tools/dev-workflow/features/merge-and-cleanup/domain/merge-cleanup-context.spec.ts`:
- Around line 23-25: Add edge-case tests to the existing
mergeCleanupContextSchema spec: add three new it blocks that call
mergeCleanupContextSchema.parse with null, with undefined, and with a payload
where prNumber is a string (e.g., 'not-a-number') to assert validation throws;
use expect(() => mergeCleanupContextSchema.parse(...)).toThrow() for
null/undefined and expect(...).toThrow('invalid_type') for the wrong-type
prNumber case so the schema rejection is covered for null, undefined, and type
mismatches.
- Around line 17-20: The test only asserts branch and prNumber after calling
mergeCleanupContextSchema.parse; update the test to also assert the other parsed
fields (reflectionFilePath, worktreePath, mainRepoPath) so the parse behavior is
fully exercised: after calling mergeCleanupContextSchema.parse(input) add
expects for result.reflectionFilePath, result.worktreePath, and
result.mainRepoPath matching the input values (or expected normalized values) to
catch regressions in mergeCleanupContextSchema parsing logic.
In `@tools/dev-workflow/features/push-reflection/entrypoint/cli.spec.ts`:
- Around line 52-64: The test "handles non-Error rejection" currently verifies
JSON output but omits asserting the CLI sets process.exitCode on failure; after
triggering the rejection via mockExecute.mockRejectedValue('string error') and
importing './cli', add an assertion that process.exitCode === 1 (e.g.
expect(process.exitCode).toBe(1)) to ensure the CLI's failure path sets the exit
code; use the same test setup around mockExecute and console.log expectations
and reset process.exitCode in teardown if necessary.
In `@tools/dev-workflow/platform/domain/workflow-execution/run-workflow.ts`:
- Around line 14-17: The WorkflowOptions interface is exported unnecessarily;
make it internal by removing the export keyword from the WorkflowOptions
declaration and update any local references (e.g., function signatures that use
WorkflowOptions<T extends BaseContext>, usages of resolveTimingsFilePath and
debugLog) to use the now-unexported type; ensure no external modules import
WorkflowOptions—if any do, either re-export a minimal public type or update
those call sites to stop importing it.
In `@tools/dev-workflow/platform/infra/external-clients/claude-agent.spec.ts`:
- Around line 268-289: The test currently asserts internal call shape using
expect(mockSdkQuery).toHaveBeenCalledWith(...) which couples to implementation;
instead modify the spec for claude.query to be behavior-driven: update the
mockSdkQuery to validate incoming args and throw or return an error when
options.permissionMode is missing or incorrect (use the existing mockSdkQuery
function to inspect its received params), then call claude.query and assert on
the observable/returned result (e.g., that the structured_output/result and
score are as expected) rather than asserting the exact SDK call; apply the same
pattern for the other test around lines 362-373 so both specs validate behavior
via mock failure/response rather than toHaveBeenCalledWith on permissionMode.
Interface and noop stay in platform/domain/ (no I/O), filesystem implementation moves to platform/infra/ to satisfy CS-010 layer responsibility rules. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The code review agents were hanging because they were instructed to use the Write tool to save reports, but the SDK query() call had no permission configuration. In default mode, Write requires approval which cannot happen in headless mode, causing a deadlock. Adding permissionMode: 'acceptEdits' allows agents to write files without manual permission approval. Also fixes lint error in extract.pr-extraction.spec.ts. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Extract shared test fixtures to __fixtures__/extraction-test-fixtures.ts - Move api-component-queries.ts to queries/ (CS-010 layer fix) - Rename types.ts → extraction-config-schema.ts (CS-009) - Rename interfaces.ts → component-contracts.ts (CS-009) - Fix run-workflow.ts domain I/O violation with WorkflowIO injection - Add workflow-io interface (domain) and implementation (infra) - Fix decorator test names to outcome-based (TS-001) - Fix toBeDefined() assertions to behavior assertions (TS-003) - Use it.each for parameterized decorator tests (SD-023) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/riviere-cli/src/features/builder/entrypoint/link-http.ts (3)
66-79: 🧹 Nitpick | 🔵 TrivialConsider using a reporter for validation errors.
Line 69 uses
console.logdirectly for validation errors, while other error paths use dedicated reporter functions (reportGraphNotFound,reportNoApiFoundForPath,reportAmbiguousApiMatch). For consistency, consider havingvalidateOptionsreturn a structured error or using a dedicated reporter.
121-138:⚠️ Potential issue | 🟡 MinorMissing error handling and user feedback.
Two concerns:
No error handling for
writeFile(Line 123): If the write fails, the error will propagate as an unhandled rejection without user-friendly messaging.No success feedback in non-JSON mode: When
--jsonis not provided, the command completes silently. Users won't know if the link was created successfully.Suggested fix
- const link = builder.link(linkInput) - - await writeFile(graphPath, builder.serialize(), 'utf-8') - - if (options.json) { + const link = builder.link(linkInput) + + try { + await writeFile(graphPath, builder.serialize(), 'utf-8') + } catch (error) { + console.error(`Failed to write graph to ${graphPath}`) + return + } + + if (options.json) { console.log( JSON.stringify( formatSuccess({ link, matchedApi: { id: matchedApi.id, path: matchedApi.path, method: matchedApi.httpMethod, }, }), ), ) + } else { + console.log(`Link created: ${matchedApi.id} → ${targetId}`) }
108-119: 🧹 Nitpick | 🔵 TrivialConsider initializing
typein the object literal.The conditional assignment pattern works but could be simplified by initializing the
typeproperty conditionally in the object literal, avoiding the intermediate mutation.Suggested refactor
- const linkInput: { - from: string - to: string - type?: 'sync' | 'async' - } = { + const linkType = options.linkType !== undefined && isValidLinkType(options.linkType) + ? options.linkType + : undefined + + const linkInput = { from: matchedApi.id, to: targetId, + ...(linkType && { type: linkType }), } - - if (options.linkType !== undefined && isValidLinkType(options.linkType)) { - linkInput.type = options.linkType - }
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/queries/api-component-queries.ts`:
- Around line 15-17: The type guard isRestApiWithPath currently returns
"component is Component & ApiComponent" which is redundant; change its return
type to "component is ApiComponent" and keep the same predicate (component.type
=== 'API' && 'path' in component && 'httpMethod' in component) so the guard
still narrows correctly to an API component with path and httpMethod; update the
function signature accordingly (isRestApiWithPath) and ensure imports/types
Component and ApiComponent remain used where needed.
In
`@packages/riviere-cli/src/features/extract/__fixtures__/extraction-test-fixtures.ts`:
- Around line 7-20: Replace the locally declared DraftComponent interface with
the canonical type from `@living-architecture/riviere-extract-ts`: remove the
local DraftComponent declaration and add an import for DraftComponent, then
update the ExtractionOutput interface to use the imported DraftComponent[] (keep
ExtractionOutput export as-is) so the fixture uses the shared, authoritative
type rather than a duplicate.
In
`@packages/riviere-cli/src/platform/infra/cli-presentation/extract-validator.ts`:
- Around line 48-61: The validateFormatOption function currently treats an empty
--files value as present; update the markdown check in validateFormatOption
(with ExtractOptions/options) so it requires either options.pr or at least one
file: replace the check `options.format === 'markdown' && !options.pr &&
options.files === undefined` with a test that treats files as missing unless
they are a non-empty array (e.g. `!(Array.isArray(options.files) &&
options.files.length > 0)`), so --format markdown fails when files is undefined
or an empty array.
In `@tools/dev-workflow/features/complete-task/commands/complete-task.ts`:
- Around line 9-70: The entry point should follow workflow-runner conventions:
import runWorkflow from workflow-runner (instead of run-workflow), move the
command-local context builder into this command (use a local
buildCompleteTaskContext wrapper) and extract the steps before calling
runWorkflow (create a steps const = buildSteps(stepDeps) and pass steps into
runWorkflow), and declare stepDeps/contextDeps as top-level variables for
readability; keep the result formatter (formatCompleteTaskResult) and options
(resolveTimingsFilePath, debugLog, io) unchanged but pass the extracted steps
and local context builder into executeCompleteTask when invoking runWorkflow.
In `@tools/dev-workflow/platform/domain/workflow-execution/run-workflow.spec.ts`:
- Around line 269-287: The test name claims the default IO writeFile is a noop
but only checks process exit; update the test to either (A) assert the writeFile
behavior by injecting a mock IO and spying on writeFile when calling runWorkflow
with resolveTimingsFilePath (verify it was not called or called with expected
args to prove noop), referencing runWorkflow and resolveTimingsFilePath, or (B)
if you prefer minimal change, rename the test to reflect that it only asserts
process exit (e.g., "exits process when resolveTimingsFilePath provided without
io"); pick one approach and apply it in the test around the existing runWorkflow
invocation.
In `@tools/dev-workflow/platform/infra/workflow-io.spec.ts`:
- Around line 21-47: Add TS-008 edge-case tests for IO by extending the existing
specs that use createDefaultWorkflowIO: add a writeFile test that calls
io.writeFile with an empty string and expects writeFileSync to have been called
with '' and 'utf-8'; add a log test that calls io.log with an empty string and
expects console.log to have been called with ''; and add an exit test that calls
io.exit(0) (using the same process.exit spy/MockExitSignal pattern) and expects
process.exit to have been called with 0. Ensure you reference the same
spies/mocks (writeFileSync, console.log spy, process.exit spy) and restore mocks
as in the existing tests.
Prevents lazy dismissals like "out of scope" without explanation. When action is 'rejected', message must be ≥300 characters with detailed reasoning about WHY the feedback doesn't apply. Error message explains: - The limit exists to prevent lazy dismissals - Must explain why feedback doesn't apply - If claiming out of scope: which file was NOT modified - Reminder that modified files are probably in scope Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace partial matchers (stringContaining, toContain) with exact value assertions per TS-003 rule. Rename test for clarity per TS-002. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tools/dev-workflow/features/respond-to-feedback/domain/feedback-response.spec.ts`:
- Around line 57-67: Add a boundary unit test that verifies a message of exactly
300 characters is accepted by the input validator: create a test (e.g.,
it('accepts rejected feedback with exactly minimum length')) that builds a
message via 'a'.repeat(300), uses respondToFeedbackInputSchema.parse(...) with
action 'rejected' and asserts the parsed result.action === 'rejected'; place the
new test alongside the existing specs in feedback-response.spec.ts to cover the
off-by-one boundary.
Verifies that messages with exactly 300 characters pass validation, catching potential off-by-one errors in the minimum length check. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Moves add-component-to-builder.ts from commands/ to cli-presentation/ with better name (component-builder-input.ts). File contains CLI input transformation logic that imports from infra layer, so it belongs in infra not domain. Per CS-010 layer responsibility rules. Also adds boundary test for 300-char rejection minimum in feedback. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…t command Moves component-builder-input.ts to commands/add-component.ts following vertical slice naming convention. Entrypoint and command share the same name because they are different layers of the same slice. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Moves load→mutate→persist logic from entrypoint to commands/add-component.ts. Entrypoint is now thin: defines CLI options, calls command. component-builder-input.ts remains in infra as dispatcher helper. Vertical slice: entrypoint → command → infra helper Co-Authored-By: Claude Opus 4.5 <[email protected]>
Each layer maps its input to the next layer's types: - Entrypoint: CLI options → command input - Command: command input → domain types - Domain: pure logic, no infra imports Mapper moved to platform/infra/component-mapping to avoid peer import in commands. Updated CLAUDE.md with layer pattern. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/entrypoint/add-component.ts`:
- Around line 64-87: The CLI currently does parseInt(options.lineNumber, 10)
which can produce NaN and be silently dropped; add explicit validation so a
non-empty --line-number is parsed to an integer and checked with
Number.isFinite/Number.isInteger (or equivalent) and if invalid return a clear
validation error; preferably implement this check inside addComponent
(referencing the addComponent function) so all callers get consistent error
formatting, and only pass a validated number (or undefined when no input) from
the action handler to addComponent.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.spec.ts`:
- Around line 7-27: The spec currently only tests the unknown componentType
path; add focused edge-case tests for buildDomainInput to cover required-option
failures and invalid/empty inputs: add tests that assert
MissingRequiredOptionError (and specific messages) when required fields are
missing (e.g., missing route for UI components), tests that validate invalid
apiType/httpMethod values produce the appropriate validation error, tests for
empty/whitespace subscribed-events and malformed custom properties (null,
non-object, or bad key/value types), and tests for null/undefined/empty overall
input; implement these as small, isolated it() cases (or a parameterized table)
referencing buildDomainInput and asserting exact thrown error types/messages so
each category from the TS-008 checklist is covered.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.ts`:
- Around line 58-112: In the API mapper (the "API" function) stop silently
dropping invalid httpMethod: if input.httpMethod is provided but
!isValidHttpMethod(input.httpMethod) throw a MissingRequiredOptionError (or an
InvalidOptionError) referencing 'http-method' for API instead of omitting it; if
valid, keep the existing inclusion logic using isValidHttpMethod. In the
EventHandler mapper (the "EventHandler" function) after building
subscribedEvents (input.subscribedEvents.split(...).map(...).filter(...)) check
the resulting array and if its length is 0 throw
MissingRequiredOptionError('subscribed-events', 'EventHandler') so
empty/whitespace-only values are rejected rather than accepted. Ensure you use
the existing MissingRequiredOptionError and isValidHttpMethod symbols to locate
the code.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/commands/add-component.ts`:
- Around line 96-100: The current catch block converts all unknown exceptions
into a ValidationError JSON which masks system-level failures; update the error
handling in add-component.ts so you only format expected domain errors (handle
DuplicateComponentError via formatError with CliErrorCode.DuplicateComponent and
known validation errors via CliErrorCode.ValidationError using getErrorMessage),
but for any other/unknown errors re-throw the original error (or at minimum log
its full stack with console.error(error.stack)) instead of converting it to a
ValidationError; modify the branch that currently prints
CliErrorCode.ValidationError to either re-throw error or log the stack before
formatting so system exceptions surface with full stack traces.
- Around line 55-57: Wrap the JSON parsing and graph parsing in a try/catch
around the lines that call JSON.parse(content) and parseRiviereGraph, so
malformed graph JSON (SyntaxError) is caught instead of bubbling up; on catch
detect SyntaxError and emit a structured CLI error (use the project's CLI error/
logger API or throw a well-typed CLI error) with a clear message that the graph
file is corrupted, then exit/return gracefully before calling
RiviereBuilder.resume(graph); ensure you reference the same variables (content,
graph) and keep RiviereBuilder.resume only inside the successful path.
- Add line number validation (NaN, Infinity checks) - Add malformed JSON error handling - Add httpMethod validation for API components - Add empty/whitespace checks for subscribedEvents - Add v8 ignore for defensive error re-throw Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/commands/add-component.spec.ts`:
- Around line 40-71: Add tests in add-component.spec.ts covering numeric edge
cases for addComponent: assert that fractional numbers (e.g., 3.14) produce
CliErrorCode.ValidationError, assert that zero and negative integers (e.g., 0,
-1) produce CliErrorCode.ValidationError if the contract requires positive
integers, and add a very large integer (e.g., Number.MAX_SAFE_INTEGER or a large
finite value) to verify it is treated as a valid lineNumber and proceeds to the
graph check (expect CliErrorCode.GraphNotFound); use the same helpers
(inputWithGraphPath, parseErrorOutput) and existing assertion patterns to
implement these cases.
In `@packages/riviere-cli/src/features/builder/commands/add-component.ts`:
- Around line 44-55: Replace the current Number.isFinite check for
input.lineNumber in add-component.ts with an integer check and a non-negative
bound: validate using Number.isInteger(input.lineNumber) and also ensure the
value meets the expected bound (e.g., input.lineNumber >= 0 or input.lineNumber
> 0 depending on whether lines are 0- or 1-based) before accepting it; if the
check fails, keep the existing formatError/CliErrorCode.ValidationError message
and return.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.ts`:
- Around line 48-121: The current presence checks (e.g., in the UI, DomainOp,
Event, and Custom mappers) allow whitespace-only strings like ' '; update each
guard to trim the incoming value and reject empty trimmed strings (e.g., const
route = input.route?.trim(); if (!route) throw ...), then use the trimmed value
in the returned input (route, operationName, eventName, customType). Keep
EventHandler's subscribedEvents handling as-is (it already trims/filters). Also
ensure when calling parseCustomProperties you pass/retain the trimmed customType
where applicable.
In `@packages/riviere-cli/vitest.config.mts`:
- Line 21: The coverage exclusion for '**/extract-output-formatter.ts' in
vitest.config.mts removes production code
(src/platform/infra/cli-presentation/extract-output-formatter.ts) from enforced
100% coverage; either remove this glob so the file is covered by tests (add
unit/integration tests for functions in extract-output-formatter), or if you
intentionally want to exclude it, add an explicit justification comment next to
the exclusion explaining why it’s a thin wrapper covered by integration tests
and replace the broad glob with a specific path pattern (e.g., the exact
relative path or narrower pattern matching only that file) so you don’t
accidentally exclude other files.
- Use Number.isInteger with positive check for lineNumber (must be >= 1) - Add boundary tests for line numbers (fractional, negative, zero, large) - Add isBlank helper to reject whitespace-only required fields - Add whitespace validation for route/operationName/eventName/customType - Add justification for extract-output-formatter.ts coverage exclusion Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/commands/add-component.spec.ts`:
- Around line 17-87: The spec is missing a command-level test for invalid
componentType; add a test in add-component.spec.ts that calls addComponent with
inputFrom inputWithGraphPath() but overrides componentType with an invalid value
(e.g., 'INVALID'), then parseErrorOutput(ctx.consoleOutput) and assert
output.error.code === CliErrorCode.ValidationError and output.error.message
contains an 'invalid component type' (or similar) message; mirror the existing
line number tests' structure (it.each or a single it) so the new test exercises
the addComponent componentType validation path.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.spec.ts`:
- Around line 7-16: Add happy-path unit tests for buildDomainInput to verify
successful mappings: create a "successful mappings" describe block and add tests
that (1) pass componentType 'UI' with a route containing surrounding whitespace
and assert the returned object has type 'UI' and input.route trimmed to '/home';
(2) pass componentType 'UseCase' and assert the returned type is 'UseCase' (no
extra required fields); (3) pass componentType 'API' with apiType 'REST',
httpMethod 'GET' and httpPath '/users' and assert result.input contains apiType
'REST', httpMethod 'GET' and path '/users'; and (4) pass componentType
'EventHandler' with subscribedEvents 'EventA, EventB , EventC' and assert
result.input.subscribedEvents equals ['EventA','EventB','EventC']; use baseInput
from the spec, buildDomainInput to produce results, and matcher helpers like
toEqual/toMatchObject/expect.objectContaining to validate structure and
trimming.
- Around line 161-181: Add a test that ensures malformed customProperty values
cause the Custom mapper to throw InvalidCustomPropertyError: create an it block
similar to the suggestion that calls buildDomainInput with componentType:
'Custom', customType: 'MyType', and customProperty: ['invalid-no-colon'] and
assert expect(() =>
buildDomainInput(input)).toThrow(InvalidCustomPropertyError); also add the
required import for InvalidCustomPropertyError from the errors module so the
test can reference the error class; this targets the code path where
buildDomainInput calls parseCustomProperties.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.ts`:
- Around line 8-10: The type predicate on isBlank is incorrect because it claims
"value is undefined" while the function actually checks for undefined, empty, or
whitespace-only strings; change its signature to a plain boolean return (e.g.,
function isBlank(value: string | undefined): boolean) or rename the function to
something explicit like isBlankOrMissing and return boolean, keeping the same
implementation and updating any call sites that relied on the previous
type-guard narrowing.
- Add componentType validation test at command level - Add success-path tests for mapper (UI, UseCase, API, EventHandler) - Add malformed customProperty test for Custom component - Fix isBlank type guard to return boolean instead of misleading predicate Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@docs/continuous-improvement/post-merge-reflections/2025-02-03-issue-238-m1-d1-7-d1-5-d1-6-define-event.md`:
- Line 72: The table cell text uses "Low priority nitpick" and should hyphenate
the compound adjective; update the markdown row containing "Strengthen
buildBehavior return type" (the table entry with "buildBehavior") to change "Low
priority nitpick" to "Low-priority nitpick" so the compound adjective is
correct.
- Line 71: The table cell containing "Low priority nitpick" should hyphenate the
compound adjective; update the table row (the string "| 14 | Simplify import
paths | ❌ | Low priority nitpick |") to read "| 14 | Simplify import paths | ❌ |
Low-priority nitpick |" so the compound adjective is clear and grammatically
correct.
In `@packages/riviere-cli/src/features/builder/commands/add-component.spec.ts`:
- Around line 17-100: Add two tests: one "successful component addition" that
creates a valid .riviere/graph.json (minimal valid JSON for parseRiviereGraph),
calls addComponent(inputWithGraphPath()) and asserts the CLI prints a success
JSON (use parse/inspect ctx.consoleOutput) and that returned data contains a
componentId (exercises parseRiviereGraph, RiviereBuilder.resume,
buildDomainInput, addComponentToBuilder, writeFile, formatSuccess); and one
"builder error handling" test that stubs/mocks addComponentToBuilder to throw,
then calls addComponent(...) with a valid graph and asserts the CLI error output
matches handleError(error) behavior (checks error.code/message), ensuring the
catch block path is exercised. Use the existing helpers (ctx,
inputWithGraphPath) and create the graph file via writeFile before invoking
addComponent.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.spec.ts`:
- Around line 29-75: Add missing happy-path tests to the successful mappings
suite by adding three specs that call buildDomainInput with componentType
'DomainOp', 'Event', and 'Custom' respectively; for DomainOp pass operationName
with surrounding whitespace and entity and assert result.type === 'DomainOp' and
result.input matches trimmed operationName and entity (operationName, entity),
for Event pass eventName and eventSchema and assert result.type === 'Event' and
result.input matches trimmed eventName and eventSchema (eventName, eventSchema),
and for Custom pass customType (with whitespace) and a metadata property and
assert result.type === 'Custom' and result.input contains the trimmed custom
type field (customTypeName or the mapper's output key for customType) and any
mapped metadata; use buildDomainInput as the invocation point to verify trimming
and correct output structure.
RFC-011: Numeric input edge case testing RFC-012: Whitespace-only string validation RFC-013: Success-path tests for mappers RFC-014: Systematic TS-008 edge case verification RFC-015: Zod read-modify-write preservation Updated TS-008 with Critical vs Extended numeric categories. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix "low priority" → "low-priority" hyphenation in reflection - Add success path test for add-component command - Add domain not found error test - Add success path tests for DomainOp, Event, Custom mappers Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/riviere-cli/src/features/builder/commands/add-component.spec.ts`:
- Around line 75-87: The test titles in the it.each block (testing addComponent)
contain "and" which mixes behaviors; change each test name to a single-concept
phrase (e.g., "accepts valid lineNumber" or "proceeds to graph check when
lineNumber is valid") or split the assertions into two separate tests so each
verifies only one behavior (one test asserting acceptance of the valid
lineNumber input for addComponent and another asserting that it triggers the
GraphNotFound error/graph check). Apply the same rename/split pattern to the
other failing cases in the same spec (the other it/it.each around the
add-component tests) so each test name describes exactly one behavior.
- Around line 43-53: Add systematic string edge-case tests to the "component
type validation" spec by calling addComponent (with inputWithGraphPath()) using
a table/loop of invalid componentType values (e.g., '', ' ' (whitespace-only),
very long string, uppercase/lowercase variants like 'invalid'/'InVaLiD' if
validation is case-sensitive, strings with special characters or emojis, and
typical injection patterns) and assert via parseErrorOutput(ctx.consoleOutput)
that output.error.code === CliErrorCode.ValidationError and the message contains
'Invalid component type' (also include a test ensuring trimming behavior if
validation should trim input). Ensure each case is a separate it/test or a
parameterized it.each so failures report the specific offending string.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.spec.ts`:
- Around line 106-115: The test for Custom input currently verifies trimming of
customType but omits asserting successful parsing of customProperty; update the
'maps valid Custom input with trimmed customType' test (or add a new happy-path
test) to include a valid customProperty value and assert that
buildDomainInput(...) returns the expected parsed structure (e.g.,
result.input.customProperty or the parsed fields created from customProperty) so
the successful customProperty handling is covered alongside the existing
malformed-case tests; locate the test using the spec file and the
buildDomainInput helper to add the additional expectation(s).
- Add string edge cases for componentType validation (empty, whitespace, special chars, typo) - Rename test names containing "and" to single-concept phrases - Add customProperty happy-path test for Custom mapper Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@docs/continuous-improvement/post-merge-reflections/2026-02-04-issue-238-m1-d1-7-d1-5-d1-6-define-event.md`:
- Line 1: The document heading "Pre-Merge Reflection:
issue-238-m1-d1-7-d1-5-d1-6-define-event" is inconsistent with its directory;
update the top-level heading to "Post-Merge Reflection:
issue-238-m1-d1-7-d1-5-d1-6-define-event" so the title matches the post-merge
reflections context and any references using that header (search for the exact
header string) remain correct.
- Around line 25-27: The bullet currently attributes multiple CodeRabbit rounds
to "add-component"; update that phrasing to match the PR scope by replacing
"add-component" with the correct feature reference (e.g., "async detection" or
"async detection and related dev-workflow and layer separation changes") so the
note aligns with the PR summary and timeline; find the line under "Multiple
CodeRabbit rounds" that mentions "add-component" and adjust the wording
accordingly to reference the async detection/dev-workflow/layer separation work.
In `@packages/riviere-cli/src/features/builder/commands/add-component.spec.ts`:
- Around line 62-94: Add tests covering null, explicit undefined, and the
missing-optional case for lineNumber: add three new it.each cases in the same
"line number validation" describe block that call addComponent({
...inputWithGraphPath(), lineNumber: value }) for values null and undefined, and
a separate call that omits the lineNumber property entirely; use
parseErrorOutput(ctx.consoleOutput) to assert the intended behavior: expect null
to produce CliErrorCode.ValidationError with message containing 'Invalid line
number', and expect explicit undefined and the omitted/missing property to be
treated as absent and reach the graph check (expect CliErrorCode.GraphNotFound).
Reference the existing helpers addComponent, inputWithGraphPath,
parseErrorOutput and ctx.consoleOutput when locating where to add these tests.
In
`@packages/riviere-cli/src/platform/infra/component-mapping/add-component-mapper.spec.ts`:
- Around line 50-74: The API and EventHandler tests currently only assert
result.input; update those two specs to also assert result.type to match the
expected component type: in the 'includes optional httpPath and httpMethod for
API' test assert result.type === 'API' (alongside buildDomainInput usage and
existing input checks), and in the 'parses comma-separated subscribedEvents'
test assert result.type === 'EventHandler' (alongside the subscribedEvents
check) so the mapper returns both the correct input shape and component type.
Prevents manually typing wrong date by requiring `date +%Y-%m-%d` command. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|



Closes #238
Deliverable: EventPublisherDef interface pattern, async publish-side detection, and async subscribe-side detection
Context
PRD: `docs/project/PRD/active/PRD-phase-12-connection-detection.md` — M1 D1.7, D1.5, D1.6
Async connection detection requires a convention for typed publish methods (D1.7) before detection can work (D1.5). Subscribe-side detection (D1.6) uses subscribedEvents metadata from Phase 11. These three deliverables form a complete vertical slice for async connection detection.
Traceability
Acceptance Criteria
Edge Case Scenarios
Implementation Guidelines
Embedded Reasoning
Why: Async connections (event publish/subscribe) are a core connection type alongside sync calls. The convention must exist before detection can work.
What: EventPublisherDef interface (Zod branded), publish-side detector (parameter type → Event matching), subscribe-side detector (subscribedEvents → Event matching).
How: Define EventPublisherDef in conventions package. In extract-ts, find classes implementing it, match method parameter types to Event components. For subscribe side, match subscribedEvents metadata to Event components. Both produce ExtractedLink[] with type "async".
Testing Strategy
Dependencies
Depends on #237
Verification
```bash
pnpm nx test riviere-extract-conventions -- --testNamePattern "EventPublisherDef"
pnpm nx test riviere-extract-ts -- --testNamePattern "async-detection"
pnpm nx test riviere-extract-ts -- --coverage
```
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation