Repository navigation
Conversation
Keep consumer-owned mapping out of domain abstractions. Add enforcement support for branded class requirements.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors extraction and connection-detection to use domain value-object classes, strengthens role-enforcement rules to block consumer-specific mappings in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs`:
- Around line 459-484: The code currently treats PrivateIdentifier names as bare
identifiers (readMemberName returns 'brand' for both `private brand` and
`#brand`), but this is undocumented and untested; update
hasRequiredPrivateMember/readMemberName to normalize incoming
requiredPrivateMembers entries (strip a leading '#' if present) before
comparison with readMemberName, add/adjust tests to include a fixture using the
`#brand` PrivateIdentifier syntax to exercise the PrivateIdentifier branch, and
add a short note in the role/config docs or builder explaining that
requiredPrivateMembers should be provided as bare names (or that leading '#'
will be normalized) so users aren’t surprised by silent no-matches; reference
functions: hasRequiredPrivateMember, readMemberName, isPrivateMember and the
config key role.requiredPrivateMembers when making these changes.
- Around line 433-448: The helper validateClassMethodOutputs currently only runs
when role.allowedOutputs is an array, which incorrectly skips input validation
for class methods; change the gate to run when either role.allowedInputs or
role.allowedOutputs is present, rename the helper to something like
validateClassMethodContracts to reflect it enforces both axes, and ensure it
still iterates class MethodDefinition members and calls
validateFunctionContract(member.value, role, `${name}.${methodName}`) for each
public/non-constructor method so both input and output constraints are applied.
In
`@packages/riviere-role-enforcement/src/features/enforcement/commands/run-role-enforcement.spec.ts`:
- Around line 261-277: Add a parallel unit test to cover ECMAScript private
fields using the "#name" syntax: in run-role-enforcement.spec.ts create a test
similar to the existing one that writes a domain class declaring "private `#brand`
= 'Beta'" (or using the "#brand" private identifier) and then call
runWith(configWithGenericRequiredPrivateMembers(['brand']), workspaceDir)
asserting exitCode 0 and empty stderr; this verifies the plugin's
isPrivateMember check (which tests member.accessibility === 'private' and
member.key?.type === 'PrivateIdentifier') correctly accepts
PrivateIdentifier-style fields.
In `@tools/dev-workflow-v2/agents/architecture-review.md`:
- Line 23: The three consecutive sentences that start with "Read" (e.g., "Read
`docs/conventions/review-feedback-checks.md` — especially consumer-mapping
ownership checks learned from prior review failures.") should be smoothed into a
single intro; replace the repeated "Read" lines with a single sentence like
"Read the following:" and convert the referenced items into a compact bulleted
list to match the style used in code-review.md; update the paragraph in
architecture-review.md accordingly so the bullets list each doc/reference and
the consumer-mapping note as an item.
- Around line 50-52: Add the new "Consumer-Mapping Ownership Check" to the
Pre-Response Checklist by inserting a bullet equivalent to the existing
"External-Client Domain-Leak Check" wording: call out that any port, presenter,
formatter, bridge, translator, or adapter under domain/ that solely maps domain
results to a specific consumer API (CLI, workflow, builder) must FAIL because it
is not a genuine domain concept; update the checklist entry labeled
"Pre-Response Checklist" to include this new consumer-mapping rule and ensure
the phrasing mirrors the External-Client Domain-Leak Check for symmetry and
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5313452-3839-4269-8db4-87de88b268bc
⛔ Files ignored due to path filters (25)
apps/docs/reference/api/generated/riviere-builder/README.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/BuildValidationError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/ComponentNotFoundError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/ComponentTypeMismatchError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/CustomTypeAlreadyDefinedError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/CustomTypeNotFoundError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/DomainNotFoundError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/DuplicateComponentError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/InvalidGraphError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/MissingDomainsError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/MissingRequiredPropertiesError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/MissingSourcesError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/RiviereBuilder.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/classes/SourceConflictError.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/APIInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/BuilderWarning.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/CustomInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/CustomTypeInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/DomainOpInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/EventHandlerInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/EventInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/UIInput.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/UpsertOptions.mdis excluded by!**/generated/**apps/docs/reference/api/generated/riviere-builder/interfaces/UseCaseInput.mdis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.claude/automatic-code-review/rules.md.coderabbit.yaml.riviere/role-definitions/domain-service.md.riviere/role-definitions/index.md.riviere/role-definitions/value-object.mddocs/conventions/review-feedback-checks.mdpackages/riviere-role-enforcement/role-enforcement-plugin.mjspackages/riviere-role-enforcement/skills/plan-review.mdpackages/riviere-role-enforcement/src/features/enforcement/commands/run-role-enforcement.spec.tspackages/riviere-role-enforcement/src/features/enforcement/commands/test-fixture-config.tspackages/riviere-role-enforcement/src/features/enforcement/domain/role-enforcement-builder.spec.tspackages/riviere-role-enforcement/src/features/enforcement/domain/role-enforcement-builder.tstools/dev-workflow-v2/.claude-plugin/plugin.jsontools/dev-workflow-v2/agents/architecture-review.mdtools/dev-workflow-v2/agents/code-review.mdtools/dev-workflow-v2/docs/factory/factory-map.mdtools/dev-workflow-v2/package.json
Normalize branded private-member requirements and apply class method contracts consistently. Keep consumer-mapping ownership in architecture review only and align supporting docs.
Deploying living-architecture with
|
| Latest commit: |
7e1472a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7e5c6663.living-architecture.pages.dev |
| Branch Preview URL: | https://issue-361.living-architecture.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/riviere-role-enforcement/role-enforcement-plugin.mjs (1)
518-524:⚠️ Potential issue | 🟠 MajorConstructor parameter properties still fail the new private-brand rule.
hasRequiredPrivateMember()only scans class-body members, soconstructor(private readonly brand: string) {}still gets rejected even though it creates the same private instance member. That leavesrequiredPrivateMemberswith a false positive on a valid TypeScript branding pattern, and the current specs only cover body fields.🔍 One possible fix
function hasRequiredPrivateMember(classNode, privateMemberName) { const normalizedPrivateMemberName = normalizeRequiredPrivateMemberName(privateMemberName) - return classNode.body.body.some( - (member) => - isPrivateMember(member) && - readMemberName(member.key) === normalizedPrivateMemberName, - ) + return ( + classNode.body.body.some( + (member) => + isPrivateMember(member) && + readMemberName(member.key) === normalizedPrivateMemberName, + ) || + classNode.body.body.some( + (member) => + member.type === 'MethodDefinition' && + member.kind === 'constructor' && + member.value.params.some((param) => + hasMatchingPrivateParameterProperty(param, normalizedPrivateMemberName), + ), + ) + ) } +function hasMatchingPrivateParameterProperty(param, privateMemberName) { + if (param.type !== 'TSParameterProperty' || param.accessibility !== 'private') { + return false + } + + const parameter = + param.parameter?.type === 'AssignmentPattern' ? param.parameter.left : param.parameter + + return parameter?.type === 'Identifier' && parameter.name === privateMemberName +}Run this read-only check to confirm the gap and find impacted patterns:
#!/bin/bash rg -n "hasRequiredPrivateMember|validateRequiredPrivateMembers|TSParameterProperty" packages/riviere-role-enforcement/role-enforcement-plugin.mjs rg -nP "@riviere-role\\s+value-object[\\s\\S]{0,220}constructor\\(\\s*private(?:\\s+readonly)?\\s+\\w+" --type ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs` around lines 518 - 524, hasRequiredPrivateMember currently only checks class body members so constructor parameter properties like `constructor(private readonly brand: string){}` are missed; update hasRequiredPrivateMember to also scan the class constructor parameters for TSParameterProperty/parameter nodes with accessibility (private/protected) and treat them as private members: when walking classNode.body.body, if you encounter a ClassMethod with kind "constructor" inspect its params, detect TSParameterProperty or params with accessibility keywords, extract the param name via readMemberName logic (apply normalizeRequiredPrivateMemberName) and compare to normalizedPrivateMemberName; keep existing checks using isPrivateMember for body fields and add this constructor-param branch so validateRequiredPrivateMembers accepts TS constructor parameter properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.riviere/roles.ts:
- Around line 76-81: The change narrowed role('value-object') to only targets:
['class'], which breaks existing exported value-object interface/type-alias
declarations (e.g., BuiltRole, BuiltLocation, LocationBuilder in
role-enforcement-builder.ts); either restore interface and type-alias to the
role targets or remove/migrate those exported annotations. Fix by updating the
role declaration to include 'interface' and 'type-alias' (e.g., targets:
['class','interface','type-alias']) until you migrate those exported
declarations, and ensure any exported BuiltRole/BuiltLocation/LocationBuilder
declarations keep the required /** riviere-role value-object */ annotation if
they remain as value-object types.
---
Duplicate comments:
In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs`:
- Around line 518-524: hasRequiredPrivateMember currently only checks class body
members so constructor parameter properties like `constructor(private readonly
brand: string){}` are missed; update hasRequiredPrivateMember to also scan the
class constructor parameters for TSParameterProperty/parameter nodes with
accessibility (private/protected) and treat them as private members: when
walking classNode.body.body, if you encounter a ClassMethod with kind
"constructor" inspect its params, detect TSParameterProperty or params with
accessibility keywords, extract the param name via readMemberName logic (apply
normalizeRequiredPrivateMemberName) and compare to normalizedPrivateMemberName;
keep existing checks using isPrivateMember for body fields and add this
constructor-param branch so validateRequiredPrivateMembers accepts TS
constructor parameter properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fffa4e1c-4717-4aac-8995-41c74a394720
📒 Files selected for processing (7)
.riviere/roles.tspackages/riviere-role-enforcement/role-enforcement-plugin.mjspackages/riviere-role-enforcement/src/features/enforcement/commands/run-role-enforcement.spec.tspackages/riviere-role-enforcement/src/features/enforcement/commands/test-fixture-config.tspackages/riviere-role-enforcement/src/features/enforcement/domain/role-enforcement-builder.tstools/dev-workflow-v2/agents/architecture-review.mdtools/dev-workflow-v2/docs/factory/factory-map.md
Convert extractor value objects to branded classes and keep CLI and role checks working.
Superseded by commit 2f215d8; checks are green.
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/build-call-graph.coverage.spec.ts (1)
150-158: 🧹 Nitpick | 🔵 TrivialAvoid invoking
buildCallGraphtwice for the same scenario.Lines 150 and 152 call
buildCallGraph(sharedProject, [compCaller], index, strictOptions)with identical inputs. The second invocation duplicates work; capture the result once and assert both that it doesn't throw and on its shape.♻️ Suggested change
- expect(() => buildCallGraph(sharedProject, [compCaller], index, strictOptions)).not.toThrow() - - const result = buildCallGraph(sharedProject, [compCaller], index, strictOptions) - expect(result).toStrictEqual([ + const result = buildCallGraph(sharedProject, [compCaller], index, strictOptions) + expect(result).toStrictEqual([ expect.objectContaining({ source: expect.stringContaining('strictcaller'), target: expect.stringContaining('stricttarget'), }), ])If the explicit "does not throw" assertion is desired (as the test title suggests), keep it but use
.not.toThrow()directly on the already-evaluated value pattern by wrapping the single call:- expect(() => buildCallGraph(sharedProject, [compCaller], index, strictOptions)).not.toThrow() - - const result = buildCallGraph(sharedProject, [compCaller], index, strictOptions) + let result: ReturnType<typeof buildCallGraph> = [] + expect(() => { result = buildCallGraph(sharedProject, [compCaller], index, strictOptions) }).not.toThrow()🤖 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/connection-detection/call-graph/build-call-graph.coverage.spec.ts` around lines 150 - 158, The test currently calls buildCallGraph(sharedProject, [compCaller], index, strictOptions) twice; instead, invoke it once and reuse the result: wrap the single call in an expect(() => { result = buildCallGraph(sharedProject, [compCaller], index, strictOptions) }).not.toThrow() to assert it doesn't throw while capturing the return value into a const/let (result), then run the existing expect(result).toStrictEqual(...) assertion; reference buildCallGraph, sharedProject, compCaller, index, and strictOptions to locate where to change.packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor-method-extraction.spec.ts (1)
46-57: 🧹 Nitpick | 🔵 TrivialTest strictness lowered:
toMatchObjectwon't catch unexpected extra fields.These full-shape assertions previously used
toStrictEqual, which would fail ifextractComponentsever emits extra/unknown properties on a component. WithtoMatchObject, regressions that introduce stray fields will silently pass. If the change is to tolerate non-enumerable/internal fields added by the new class-based value objects, consider keeping strict equality for the public shape (e.g., destructure to a plain object, or assert specific allowed keys) so this suite still defends against accidental schema drift. Same applies to lines 77 and 97-108.🤖 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/component-extraction/extractor-method-extraction.spec.ts` around lines 46 - 57, The test has been relaxed from strict equality to toMatchObject which will hide accidental extra fields; change the assertions in extractor-method-extraction.spec.ts (the expect(result) checks around the shown block and the other assertions at the locations called out — lines near 77 and 97-108) to assert a strict public shape: either restore toStrictEqual against a plain POJO (by mapping/destructuring each extracted component to an object with only the expected keys: type, name, location, domain, module) or explicitly assert allowed keys (e.g., check Object.keys and values) so unknown properties on the extracted components will fail the test. Ensure you update all three assertion sites consistently.packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/deduplicate-links.ts (1)
31-50: 🛠️ Refactor suggestion | 🟠 MajorDefensive post-construction check is inconsistent with
requireSourceLocation.Lines 44–47 only check
sourceLocation === undefinedafter construction, whilerequireSourceLocation(lines 113–133) additionally requireslineNumberandmethodName. IfExtractedLink's constructor can produce a partialsourceLocation, that partial state passes here and only blows up later in deduplication viarequireSourceLocation.Either:
- have
ExtractedLinkitself enforce the invariant (preferred — it owns the value object) and drop both runtime guards here, or- make this guard match
requireSourceLocationso any inconsistency fails at construction.Also note
buildUncertainLink(lines 52–70) has no equivalent guard, so the protection is asymmetric.🤖 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/connection-detection/call-graph/deduplicate-links.ts` around lines 31 - 50, The current post-construction check only tests sourceLocation !== undefined in the function that builds links, which is inconsistent with requireSourceLocation (which also enforces lineNumber and methodName) and leaves buildUncertainLink unguarded; fix by one of two options: (1) Preferred — move/implement the full invariant check into ExtractedLink's constructor so it always validates that sourceLocation includes lineNumber and methodName (then remove the ad-hoc guards in the factory functions), or (2) If you prefer minimal change, extend the guard in the factory functions (the function creating the link here and buildUncertainLink) to mirror requireSourceLocation’s checks (verify sourceLocation exists and has non-null lineNumber and methodName) so construction fails consistently before deduplication; reference the ExtractedLink class, requireSourceLocation helper, and buildUncertainLink factory when applying the change.packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.ts (1)
44-48: 🧹 Nitpick | 🔵 Trivial
@riviere-role value-objecton a non-branded internal interface.Per this PR, the
value-objectrole is being tightened to require a privatebrandmember.ComponentContextis annotated@riviere-role value-objectbut is a plain interface (no brand) and is not exported — so it both cannot satisfy the new contract and isn't required to carry the annotation. Either drop the annotation or convert it to a branded class like the other value objects introduced in this PR (e.g.DraftComponent,InterfaceResolutionOutcome).🤖 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/component-extraction/extractor.ts` around lines 44 - 48, The ComponentContext interface is incorrectly annotated with `@riviere-role` value-object but lacks the required private brand; either remove the annotation or convert ComponentContext into a branded value-object class similar to DraftComponent and InterfaceResolutionOutcome: replace the interface ComponentContext with a non-exported class ComponentContext that has a private readonly brand (e.g. private readonly __brand = Symbol('ComponentContext')), explicit constructor accepting domain and module, public readonly properties or getters for domain and module, and preserve any usage sites; update the `@riviere-role` value-object annotation to sit on this new class if you choose the branded approach.tools/dev-workflow-v2/src/features/workflow/domain/workflow.ts (1)
97-103: 🧹 Nitpick | 🔵 Trivial
query-modelis semantically incorrect for a dependency-injection port type.
WorkflowDepsis a DI bag (callable membersgetGitInfo,getPrFeedback,sleepMs,now) — not a CQRS query result/model. The retag fromvalue-objectis necessary because the current role constraints forbid callable members in value objects, butquery-modelconflates two distinct DDD concepts: dependency-inversion boundaries (ports/adapters) and read-side representations (query models). This is an anti-pattern per DDD hexagonal architecture guidance. The role taxonomy is missing a dedicatedportoradapterrole to capture this pattern without semantic drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow.ts` around lines 97 - 103, The role tag on the DI bag is wrong: replace the comment /** `@riviere-role` query-model */ above the WorkflowDeps type with a port/adapter role (e.g., /** `@riviere-role` port */) because WorkflowDeps represents a dependency-inversion port containing callable members (getGitInfo, getPrFeedback, sleepMs, now) rather than a read-side query model or value object; update any project role taxonomy references or tests that validate role names to accept the new "port" (or "adapter") role so the DI interface WorkflowDeps is correctly classified.
♻️ Duplicate comments (4)
packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.spec.ts (1)
33-346:⚠️ Potential issue | 🟡 Minor
toMatchObjectis still used throughout — switch all assertions back totoStrictEqual.Every assertion at Lines 33, 40, 58, 84, 110, 136, 161, 180, 208, 228, 257, 289, 313, and 346 still uses
toMatchObject, which permits extra unexpected fields on the result. Restore strict equality so the tests guard against accidental field additions and shape drift now thatextractComponentsreturnsDraftComponentinstances.💚 Example diff (apply same change to all listed lines)
- expect(result).toMatchObject([]) + expect(result).toStrictEqual([])- expect(result).toMatchObject([ + expect(result).toStrictEqual([ { type: 'useCase', name: 'CreateOrder', ... }, ])🤖 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/component-extraction/extractor.spec.ts` around lines 33 - 346, The tests in extractor.spec.ts use toMatchObject in multiple assertions; change all those assertions to use toStrictEqual to enforce exact shape equality (replace every expect(...).toMatchObject(...) with expect(...).toStrictEqual(...)). Look for usages in tests that call extract(project, ...), and for helper-created projects/configs such as createTestProject, extract, createOrdersUseCaseConfig, createConfigWithRule, and createConfigWithCustomTypes; update the assertion calls in those test cases (including the edge cases, function/class/custom type extraction blocks) so they assert strict equality.packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/connection-detection-values.spec.ts (1)
35-91:⚠️ Potential issue | 🟡 MinorUse
toStrictEqual(nottoMatchObject) and keep assertion style consistent across the three suites.
- Lines 35–41 and 64–70 use
toMatchObject, which would silently pass ifConnectionDetectionOptions/PerModuleConnectionOptionsever stored additional unexpected fields on the instance — exactly what these "stores all constructor fields" tests should be guarding against.- The
CrossModuleConnectionOptionsblock (Lines 89–91) checks each property in isolation while the other two blocks shape-check the whole instance. Pick one style and apply it uniformly so a regression is caught the same way in every options class.💚 Suggested change
- expect(result).toMatchObject({ + expect(result).toStrictEqual({ allowIncomplete: true, sourceFilePaths: ['/src/orders/order.ts'], eventPublishers, httpLinks, repository: 'test-repo', })- expect(result).toMatchObject({ + expect(result).toStrictEqual({ allComponents: [component], allowIncomplete: true, sourceFilePaths: ['/src/orders/order-service.ts'], httpLinks, repository: 'test-repo', })- expect(result.allowIncomplete).toBe(false) - expect(result.eventPublishers).toStrictEqual(eventPublishers) - expect(result.repository).toBe('test-repo') + expect(result).toStrictEqual({ + allowIncomplete: false, + eventPublishers, + repository: 'test-repo', + })🤖 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/connection-detection/connection-detection-values.spec.ts` around lines 35 - 91, Tests for ConnectionDetectionOptions, PerModuleConnectionOptions, and CrossModuleConnectionOptions use inconsistent assertion styles; replace the toMatchObject assertions in the ConnectionDetectionOptions and PerModuleConnectionOptions specs with toStrictEqual (or update the CrossModuleConnectionOptions to match, but prefer toStrictEqual for exact shape checks) so all three suites assert full deep equality and will fail on unexpected extra fields; update the assertions around the result variables in those tests (look for the expect(result).toMatchObject calls and change them to expect(result).toStrictEqual with the same expected object) to keep assertion style consistent across the three suites.packages/riviere-builder/src/features/building/domain/construction/construction-types.ts (1)
14-115:⚠️ Potential issue | 🟠 MajorDo not reclassify builder construction inputs as
query-model.
BuilderOptions,UIInput,APIInput, and the rest are the builder’s write-side construction contract insrc/features/building/domain/construction/. Retagging them asquery-modelgives a read-side boundary role ownership of core domain inputs and reintroduces the same ownership mistake this PR is meant to block.As per coding guidelines,
**/*.{ts,tsx}must follow the audit checklist in thedevelopment-skills:separation-of-concernsskill.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riviere-builder/src/features/building/domain/construction/construction-types.ts` around lines 14 - 115, The interfaces BuilderOptions, UIInput, APIInput, UseCaseInput, DomainOpInput, EventInput, EventHandlerInput, CustomTypeInput, CustomInput (and any other builder construction inputs in this file) are incorrectly tagged as `@riviere-role` query-model; revert those annotations so the write-side construction contract retains its correct ownership (remove the query-model tag or replace it with the appropriate write-side/command-model tag per project conventions), and re-run the development-skills:separation-of-concerns audit checklist to ensure all src/features/building/domain/construction/* types remain classified as construction/write-side types.packages/riviere-role-enforcement/role-enforcement-plugin.mjs (1)
555-561:⚠️ Potential issue | 🟠 Major
requiredPrivateMembersstill misses private constructor parameter properties.
constructor(private readonly brand: string)is aTSParameterProperty, not a class-body member withkey, sohasRequiredPrivateMember()still rejects a valid branded-class pattern. That leaves the new branding rule incomplete.Suggested direction
function hasRequiredPrivateMember(classNode, privateMemberName) { const normalizedPrivateMemberName = normalizeRequiredPrivateMemberName(privateMemberName) - return classNode.body.body.some( - (member) => - isPrivateMember(member) && - readMemberName(member.key) === normalizedPrivateMemberName, - ) + return classNode.body.body.some((member) => { + if (isPrivateMember(member)) { + return readMemberName(member.key) === normalizedPrivateMemberName + } + + if (!isConstructorDefinition(member)) { + return false + } + + return member.value.params.some( + (param) => + param.type === 'TSParameterProperty' && + param.accessibility === 'private' && + unwrapParameterProperty(param.parameter)?.type === 'Identifier' && + unwrapParameterProperty(param.parameter).name === normalizedPrivateMemberName, + ) + }) }#!/bin/bash # Verify that requiredPrivateMembers checks class-body members only and does not cover TSParameterProperty. sed -n '555,646p' packages/riviere-role-enforcement/role-enforcement-plugin.mjs rg -n -C2 'constructor\(private|constructor\(public|constructor\(protected' packages/riviere-role-enforcement/src/features/enforcement -g '*.spec.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs` around lines 555 - 561, hasRequiredPrivateMember misses private constructor parameter properties (TSParameterProperty) because it only checks class body members' keys; update hasRequiredPrivateMember to also inspect the class's constructor parameters for TSParameterProperty/parameter properties and treat their identifier names (after normalizeRequiredPrivateMemberName) as valid private members. Specifically, keep the existing check over classNode.body.body using isPrivateMember and readMemberName, and add a secondary check that finds the ClassMethod with kind 'constructor' in classNode.body.body, iterates its value.params to detect TSParameterProperty nodes or params with accessibility modifier, extracts their identifier names, normalizes them via normalizeRequiredPrivateMemberName, and returns true if any match the required privateMemberName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.riviere/roles.ts:
- Around line 76-83: The JSDoc annotation marking the internal interface
ComponentContext as a value-object is now invalid because value-object targets
classes only; open the file containing the interface named ComponentContext (in
extractor.ts) and remove the /** `@riviere-role` value-object */ annotation
immediately above the interface declaration since ComponentContext is an
internal, non-exported interface and should not be role-annotated.
In `@packages/riviere-cli/src/features/extract/domain/extraction-outcome.ts`:
- Around line 28-29: The union type ExtractionOutcome (and its constituent types
DraftOnlyOutcome, FullExtractionOutcome, FieldFailureOutcome) is a domain result
model and must not be labeled as a query projection; change the JSDoc role tag
from "@riviere-role query-model" to "@riviere-role domain-model" (or remove the
query-model tag entirely) so the file correctly asserts domain ownership and
aligns with the separation-of-concerns audit; update the comment immediately
above the export of ExtractionOutcome accordingly.
In
`@packages/riviere-cli/src/features/extract/infra/external-clients/extraction-config/load-extended-module.spec.ts`:
- Around line 14-19: The cleanup in the test harness that calls rmSync(dir, {
recursive: true }) can fail and leave temp dirs behind; update the finally block
that wraps fn(dir) (created via mkdtempSync) to call rmSync(dir, { recursive:
true, force: true }) so removal is idempotent and resilient to ENOENT/EACCES
during test failures.
In
`@packages/riviere-cli/src/features/extract/infra/external-clients/extraction-config/load-extended-module.ts`:
- Around line 145-156: The current catch in loadExtendedModule swallows all
errors and rewraps them as ConfigSchemaValidationError, obscuring genuine errors
from resolveConfigWithExtends; change the catch to re-throw known project error
types (e.g., ConfigFileNotFoundError, PackageResolveError,
InvalidConfigFormatError, and already ConfigSchemaValidationError) and only wrap
unknown errors as a ConfigSchemaValidationError with the original error stored
as the cause; update the catch in loadExtendedModule to check instanceof for
those types (and preserve the original error in the thrown
ConfigSchemaValidationError's cause when wrapping).
- Around line 96-106: The catch block around require.resolve currently swallows
the original resolution error; update the try/catch in load-extended-module (the
block that calls require.resolve(`${packageName}/package.json`) and throws
PackageResolveError) to catch the error as e and rethrow new
PackageResolveError(packageName, { cause: e }) so the original MODULE_NOT_FOUND
diagnostic is preserved, and modify PackageResolveError's constructor to accept
and forward options (e.g., options?: ErrorOptions) to super(...) so the cause is
attached to the thrown error.
In
`@packages/riviere-cli/src/features/extract/infra/persistence/extraction-project/extraction-project-repository.spec.ts`:
- Around line 264-318: The test title claims customTypes from the base and local
extends config are merged, but it only asserts the project is defined; update
the spec to actually assert the merge by calling new
ExtractionProjectRepository().loadFromFullProject(...) and inspecting the
resolved module's customTypes (or extraction result) to ensure it contains both
"service" (from extended.yml) and "repository" (from extract.yml); reference the
resolved object's modules array (e.g., result.modules[0].customTypes or the
extraction components produced) and add assertions that keys for both custom
types exist and/or that components for a class named OrderService and
OrderRepository are present to prove both were merged.
In `@packages/riviere-cli/src/platform/domain/add-component.ts`:
- Around line 12-47: The Add*Input interfaces (e.g., AddUIInput, AddAPIInput,
AddUseCaseInput, AddDomainOpInput, AddEventInput, AddEventHandlerInput,
AddCustomInput and the AddComponentInput type) are write-side DTOs and should
not be annotated as query-model; update their `@riviere-role` tags to the
appropriate write/command role (for example `@riviere-role` command-model or
remove the query-model tag and add the command-model tag) so they are treated as
write-side inputs that drive addComponentToBuilder(), ensuring separation of
concerns and compliance with the separation-of-concerns audit.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/draft-component.ts`:
- Around line 6-28: The DraftComponent class repeats the inline location shape
and stores the constructor's location object by reference; extract a shared type
(e.g., SourceLocation from `@living-architecture/riviere-schema` or create a local
SourceLocation type) and use it for the readonly location property and
constructor params, and when assigning inside the constructor (in
DraftComponent's constructor and params.location) clone or freeze the value
(e.g., assign a shallow copy or Object.freeze({...params.location})) so external
mutations can't change the stored location.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/build-call-graph.guard.spec.ts`:
- Around line 24-31: Replace the magic numbers in the Project compilerOptions
with the ts-morph enums: import ScriptTarget and ModuleKind from 'ts-morph' and
set target: ScriptTarget.ESNext and module: ModuleKind.ESNext in the Project({
... }) initializer (the instance created in this spec file); update the imports
accordingly so build-call-graph.guard.spec.ts uses the named enums instead of 99
for clarity and consistency with detect-connections-fixtures.ts.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/build-call-graph.ts`:
- Around line 124-133: The runtime guard function requireResolvedTypeName exists
to compensate for lost type narrowing on the TypeResolution type; fix by
restoring a compile-time discriminated union or adding an explicit type guard on
the TypeResolution class so callers can rely on the resolved=true invariant
instead of throwing at runtime. Concretely, update the TypeResolution definition
in type-resolver.ts to either (a) split into two types (e.g.,
ResolvedTypeResolution { resolved: true; typeName: string } and
UnresolvedTypeResolution { resolved: false; typeName?: undefined }) or (b) add a
method/isResolved(): this is ResolvedTypeResolution that narrows the instance
when true; then remove requireResolvedTypeName and replace its uses so code
checks the discriminant or calls the type guard (e.g.,
myResolution.isResolved()) and directly accesses typeName without runtime
TypeError checks.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/call-graph-outcomes.ts`:
- Around line 28-31: The two constructors are inconsistent: update the
MethodLookup constructor signature to match InterfaceResolutionOutcome by using
comma separators (not semicolons) and remove the stray trailing whitespace in
the parameter list; specifically edit the MethodLookup constructor (the params
object containing method: MethodDeclaration | undefined and classFound: boolean)
to use the same comma-separated style as InterfaceResolutionOutcome so both
constructors follow the same formatting.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/call-graph-shared.guard.spec.ts`:
- Around line 24-40: The test title expects a TypeError but currently only
asserts the error message string; update the assertion to check the error
constructor as well (e.g., use toThrow(TypeError) or toThrowError(TypeError,
'Expected interface resolution type name') ) when calling
resolveTypeThroughInterface with the Project, ComponentIndex, and
CallGraphOptions setup so the spec verifies a TypeError is thrown (refer to
resolveTypeThroughInterface, CallGraphOptions, ComponentIndex, and Project to
locate the test).
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/call-graph-shared.ts`:
- Around line 55-64: Replace the inline type import in requireInterfaceTypeName
with a top-level import: add InterfaceResolution to the file's import list
(import { InterfaceResolution } from
'../interface-resolution/resolve-interface') and change the function signature
to use that named type (interfaceResolution: InterfaceResolution). Keep the
existing runtime guard/throw in requireInterfaceTypeName unchanged; only move
the type reference to the top-level import to match the file style.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/call-graph-types.ts`:
- Around line 4-19: The CallGraphOptions VO currently declares sourceFilePaths
as mutable string[]; change the property to an immutable type (e.g., readonly
string[] or ReadonlyArray<string>) and defensively copy/freeze the input in the
constructor (e.g., assign Object.freeze([...params.sourceFilePaths]) cast to the
readonly type) so callers cannot mutate contents; update CallGraphOptions
(property and constructor assignment) and apply the same pattern to any other
branded classes that accept array fields to preserve true VO immutability.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/deduplicate-links.guard.spec.ts`:
- Around line 49-88: Add an explicit branch handling extractedLinkState.mode ===
'missingMethodName' so it sets this.sourceLocation to an object containing
repository, filePath and lineNumber but omitting methodName (mirroring the
intended "missing method" shape) instead of relying on the current unconditional
fallthrough; update the dispatch in the constructor/initializer that reads
extractedLinkState.mode (and/or the ExtractedLinkMockMode enum) to include this
case (or make the switch/exhaustive if/else throw for unknown modes) so future
modes cannot accidentally inherit the fallthrough behavior—refer to the existing
this.sourceLocation assignments and the extractedLinkState.mode checks to place
the new branch.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/deduplicate-links.ts`:
- Line 88: The deduplication uses only the callSite.lineNumber to pick the
earliest link but currently calls requireSourceLocation(existing) which enforces
methodName; change the guard to validate/unwrap only the fields used (e.g.,
callSite and callSite.lineNumber) instead of calling requireSourceLocation;
specifically, replace usages of requireSourceLocation(existing) in the
deduplicate-links flow (including the comparison at raw.callSite.lineNumber <
requireSourceLocation(existing).lineNumber and the surrounding 113-133 block)
with a lightweight check/unwrap that ensures existing.callSite and
existing.callSite.lineNumber are present (or fall back safely) so links with
undefined methodName don't throw, and keep any full-object invariant enforcement
inside the ExtractedLink creation/validator instead of here.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/trace-calls.ts`:
- Line 144: The guard in requireResolvedTypeName currently throws a bare
TypeError; change it to throw a ConnectionDetectionError so the domain invariant
violation carries call-site/file/line context (use the existing
ConnectionDetectionError class), and update requireResolvedTypeName's signature
to accept the offending CallExpression (or a source location/file/line tuple) so
you can populate the ConnectionDetectionError with the expression/context when
constructing the error and include a clear message like "Expected resolved type
name" plus the resolved location info.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/type-resolver.ts`:
- Around line 7-39: TypeResolution currently exposes a boolean resolved plus
optional fields which prevents TypeScript narrowing; remove the resolved field,
change the static factories TypeResolution.resolved(typeName: string) and
TypeResolution.unresolved(reason: string) to only accept the appropriate param,
and add an isResolved(): this is TypeResolution & { typeName: string } method
that returns typeName !== undefined; make the identical change to
InterfaceResolution (remove resolved field, add isResolved type-guard, and
tighten its factories). Then update all callers (e.g., build-call-graph.ts,
call-graph-shared.ts, trace-calls.ts) to replace checks of if
(!typeResult.resolved) with if (!typeResult.isResolved()) and remove any
requireResolvedTypeName()/requireInterfaceTypeName() runtime-throw helpers so
callers rely on the narrowed typeName string.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/connection-detection-values.ts`:
- Around line 15-26: The helper assignLinkCollections<TTimings> currently
accepts a mutable target and bypasses readonly fields declared in
ConnectionDetectionResult and PerModuleDetectionResult; to fix this, change
assignLinkCollections to accept a writable-shaped target (e.g. Writable<{ links:
ExtractedLink[]; externalLinks: ExternalLink[]; timings: TTimings }>) so it no
longer silently defeats readonly, then update CrossModuleDetectionResult to add
externalLinks (so the helper can be reused) and call assignLinkCollections from
CrossModuleDetectionResult, ConnectionDetectionResult, and
PerModuleDetectionResult rather than inlining or mutating readonly fields
directly; ensure you reference the function assignLinkCollections and the
classes ConnectionDetectionResult, PerModuleDetectionResult, and
CrossModuleDetectionResult when making these edits.
- Around line 73-87: The classes use `declare readonly` for fields that are
actually initialized in the constructor via assignLinkCollections; replace
`declare readonly` with plain `readonly` on the fields in
ConnectionDetectionResult (brand, links, externalLinks, timings) and do the same
for PerModuleDetectionResult so the declarations match how assignLinkCollections
initializes them; keep the constructor calling assignLinkCollections(this,
params) and mirror the style used by ConnectionDetectionOptions and
CrossModuleDetectionResult for consistency with project tsconfig.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/detect-connections.spec.ts`:
- Around line 342-345: Replace the use of the structural matcher toMatchObject
with the precise equality matcher toEqual for the array assertion: update the
expectation on project.getSourceFiles().map((f) => f.getFilePath()) in the
detect-connections.spec test to use toEqual([...]) so the test more clearly
asserts exact string-array equality instead of structural-subset semantics.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/detect-connections.ts`:
- Around line 11-26: The file imports classes twice (as types and as renamed
value aliases like ConnectionDetectionResultRecord,
CrossModuleDetectionResultRecord, PerModuleDetectionResultRecord); consolidate
by importing each Result class once (e.g., import { ConnectionDetectionResult,
CrossModuleDetectionResult, PerModuleDetectionResult, ConnectionTimings,
CrossModuleTimings, PerModuleTimings } from './connection-detection-values') and
remove the *Record aliases, then update construction sites to use the classes
directly (e.g., new PerModuleDetectionResult(...), new
CrossModuleDetectionResult(...), new ConnectionDetectionResult(...)) while
keeping separate type-only imports only where truly needed.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/interface-resolution/resolve-interface.ts`:
- Around line 7-46: The refactor flattened the previous discriminated union into
one class (InterfaceResolution) with optional fields, losing type-narrowing;
restore a discriminated-union shape by replacing the single class with two
value-object classes (e.g. InterfaceResolved and InterfaceUnresolved) each with
a branded private field and a precise readonly resolved literal (resolved: true
and resolved: false), move the specific properties onto the appropriate class
(InterfaceResolved has typeName: string; InterfaceUnresolved has reason: string
and typeDefinedInSource: boolean), and export type InterfaceResolution =
InterfaceResolved | InterfaceUnresolved (or keep InterfaceResolution as a
factory namespace that returns those types) so callers using the resolved
boolean can statically narrow without runtime guards like
requireInterfaceTypeName.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/enrich-components-http-call.spec.ts`:
- Line 178: The test uses toMatchObject on the array at
result.components[0]?._missing which permits extra entries; change these
assertions to use toStrictEqual(['method']) or add an explicit length/assertion
so the test fails if spurious entries are added (update the occurrences around
the existing expect at result.components[0]?._missing and the other similar
assertions noted at the other occurrences - lines referencing
result.components[…]._missing at the later checks). Ensure each replaced
assertion exactly matches the expected array and/or asserts _missing.length ===
1 to prevent regressions.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/enrich-components.spec.ts`:
- Line 207: The test uses toMatchObject on _missing arrays which allows extra
elements to pass; change those assertions to use toEqual (or toStrictEqual) so
the array must exactly match; locate the assertions like
expect(result.components[0]?._missing).toMatchObject(['path']) and the other
similar expect(... _missing).toMatchObject(...) occurrences (also present in the
same file for the other test cases) and replace toMatchObject([...]) with
toEqual([...]) to ensure any added unexpected missing fields fail the test.
- Around line 78-84: The helper enrich currently accepts modules: Module[] but
only uses modules[0]; change its signature to enrich(drafts: DraftComponent[],
module: Module) and update its body to call enrichComponents(drafts, module,
sharedProject) without the runtime TypeError guard; then update all test call
sites to pass a Module (module) instead of a single-element array ([module]) so
calls become enrich([...], module). Ensure references to DraftComponent, Module,
enrichComponents and sharedProject are preserved.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/enriched-component.ts`:
- Line 17: EnrichedComponent's private field `_missing` is mutable while all
other fields are readonly; change the `_missing` declaration to `readonly
_missing: string[] | undefined` and update any constructor parameter/assignment
in the EnrichedComponent class so the value is set once at construction (or
wrapped/copied if necessary) to preserve immutability; also search for and
remove or replace any later mutations of `_missing` (e.g., pushes or
reassignments) so the value-object contract for EnrichedComponent is upheld.
- Line 3: The type alias MetadataValue is declared separately and then
re-exported; inline it by changing the standalone declaration to an exported
alias (make it export type MetadataValue = string | number | boolean | string[])
and remove the separate re-export statement for MetadataValue so there's a
single declaration-and-export for that type.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/evaluate-extraction-rule.ts`:
- Line 17: Move the lone import of ExtractionResult so it sits with the other
imports instead of between the export re-exports; specifically relocate the line
importing ExtractionResult to the import block above (near the other import
statements) and keep the export { … } from … blocks grouped together thereafter
to maintain consistent ordering.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/extraction-result.spec.ts`:
- Around line 8-22: Update the spec to more fully exercise the branded
value-objects: rename the variable in the ExtractionContext test from result to
context and assert context.filePath, add at least one new ExtractionResult test
that constructs an ExtractionResult with a non-trivial value (e.g., an array or
nested object) and asserts the stored shape, and add a compile-time check using
a call site with // `@ts-expect-error` attempting to pass a plain object where
ExtractionResult (or ExtractionContext) is required to prove the brand prevents
structural substitution; optionally add a simple identity/equality check (===)
between two ExtractionResult instances to document expected semantics.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/value-extraction/method-extraction-result.ts`:
- Around line 19-29: The parameters array is currently mutable because its type
is ParameterInfo[]; update the type to a readonly array to enforce immutability:
change the property declaration readonly parameters: ParameterInfo[] to readonly
parameters: readonly ParameterInfo[] (or ReadonlyArray<ParameterInfo>), change
the constructor param type from parameters: ParameterInfo[] to parameters:
readonly ParameterInfo[], and update the MethodExtractionValue union entry (the
union type at MethodExtractionValue) from ParameterInfo[] to readonly
ParameterInfo[] so callers cannot mutate the contained arrays; keep assignments
in the MethodExtractionResult constructor unchanged (types will now enforce
immutability).
In
`@packages/riviere-extract-ts/src/platform/domain/ast-literals/literal-detection.ts`:
- Around line 54-66: The LiteralResult class currently decouples kind and value
types causing loss of discriminated-union narrowing; change the class to restore
the correlation by making it generic or using a mapped type: introduce a union
type alias for the literal kinds (e.g. LiteralKind =
'string'|'number'|'boolean'|'string[]') and a mapping type (e.g. KindToValue)
that maps each kind to its exact value type, then update LiteralResult to be
generic over K extends LiteralKind (or have its constructor take params: { kind:
K; value: KindToValue[K] }) so that new LiteralResult({ kind: 'string', value:
... }) enforces and preserves the correct value type and lets consumers (and
functions like buildExtractionResult and extractLiteralValue) benefit from
discriminated narrowing.
In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs`:
- Around line 201-252: validateForbiddenSupertypes can be bypassed by aliased
imports because readDeclaredSupertypes currently returns only local identifier
names; change readDeclaredSupertypes to accept the existing filename parameter
and for each identifier resolved via readNamedTypeReference call
readImportedReference(filename, localName) (or similar existing
import-resolution helper) to map the local identifier back to its
exported/original name, then return or compare against those resolved names so
validateForbiddenSupertypes checks role.forbiddenSupertypes against the resolved
export names rather than raw local identifiers.
In
`@packages/riviere-role-enforcement/src/features/enforcement/commands/test-fixture-config.ts`:
- Around line 104-137: Both helpers configWithGenericRepositoryMethodInputs and
configWithGenericRepositoryMethodInputsOnly duplicate the same
roleEnforcement(...) payload except for the presence of allowedOutputs; extract
a small factory function (e.g., makeRepositoryConfig(allowedInputs: string[],
allowedOutputs?: string[])) that builds and returns the roleEnforcement call and
have both configWithGenericRepositoryMethodInputs and
configWithGenericRepositoryMethodInputsOnly call that factory, preserving the
same filter on genericTestRoles (removing role-b-repository), targets ['class'],
and using genericTestLocations and the existing
canonicalConfigurationsFile/ignorePatterns/roleDefinitionsDir/package list;
ensure the factory sets allowedOutputs only when provided so role-b-repository
behaves identically to the originals.
---
Outside diff comments:
In
`@packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor-method-extraction.spec.ts`:
- Around line 46-57: The test has been relaxed from strict equality to
toMatchObject which will hide accidental extra fields; change the assertions in
extractor-method-extraction.spec.ts (the expect(result) checks around the shown
block and the other assertions at the locations called out — lines near 77 and
97-108) to assert a strict public shape: either restore toStrictEqual against a
plain POJO (by mapping/destructuring each extracted component to an object with
only the expected keys: type, name, location, domain, module) or explicitly
assert allowed keys (e.g., check Object.keys and values) so unknown properties
on the extracted components will fail the test. Ensure you update all three
assertion sites consistently.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.ts`:
- Around line 44-48: The ComponentContext interface is incorrectly annotated
with `@riviere-role` value-object but lacks the required private brand; either
remove the annotation or convert ComponentContext into a branded value-object
class similar to DraftComponent and InterfaceResolutionOutcome: replace the
interface ComponentContext with a non-exported class ComponentContext that has a
private readonly brand (e.g. private readonly __brand =
Symbol('ComponentContext')), explicit constructor accepting domain and module,
public readonly properties or getters for domain and module, and preserve any
usage sites; update the `@riviere-role` value-object annotation to sit on this new
class if you choose the branded approach.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/build-call-graph.coverage.spec.ts`:
- Around line 150-158: The test currently calls buildCallGraph(sharedProject,
[compCaller], index, strictOptions) twice; instead, invoke it once and reuse the
result: wrap the single call in an expect(() => { result =
buildCallGraph(sharedProject, [compCaller], index, strictOptions)
}).not.toThrow() to assert it doesn't throw while capturing the return value
into a const/let (result), then run the existing
expect(result).toStrictEqual(...) assertion; reference buildCallGraph,
sharedProject, compCaller, index, and strictOptions to locate where to change.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/call-graph/deduplicate-links.ts`:
- Around line 31-50: The current post-construction check only tests
sourceLocation !== undefined in the function that builds links, which is
inconsistent with requireSourceLocation (which also enforces lineNumber and
methodName) and leaves buildUncertainLink unguarded; fix by one of two options:
(1) Preferred — move/implement the full invariant check into ExtractedLink's
constructor so it always validates that sourceLocation includes lineNumber and
methodName (then remove the ad-hoc guards in the factory functions), or (2) If
you prefer minimal change, extend the guard in the factory functions (the
function creating the link here and buildUncertainLink) to mirror
requireSourceLocation’s checks (verify sourceLocation exists and has non-null
lineNumber and methodName) so construction fails consistently before
deduplication; reference the ExtractedLink class, requireSourceLocation helper,
and buildUncertainLink factory when applying the change.
In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow.ts`:
- Around line 97-103: The role tag on the DI bag is wrong: replace the comment
/** `@riviere-role` query-model */ above the WorkflowDeps type with a port/adapter
role (e.g., /** `@riviere-role` port */) because WorkflowDeps represents a
dependency-inversion port containing callable members (getGitInfo,
getPrFeedback, sleepMs, now) rather than a read-side query model or value
object; update any project role taxonomy references or tests that validate role
names to accept the new "port" (or "adapter") role so the DI interface
WorkflowDeps is correctly classified.
---
Duplicate comments:
In
`@packages/riviere-builder/src/features/building/domain/construction/construction-types.ts`:
- Around line 14-115: The interfaces BuilderOptions, UIInput, APIInput,
UseCaseInput, DomainOpInput, EventInput, EventHandlerInput, CustomTypeInput,
CustomInput (and any other builder construction inputs in this file) are
incorrectly tagged as `@riviere-role` query-model; revert those annotations so the
write-side construction contract retains its correct ownership (remove the
query-model tag or replace it with the appropriate write-side/command-model tag
per project conventions), and re-run the
development-skills:separation-of-concerns audit checklist to ensure all
src/features/building/domain/construction/* types remain classified as
construction/write-side types.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.spec.ts`:
- Around line 33-346: The tests in extractor.spec.ts use toMatchObject in
multiple assertions; change all those assertions to use toStrictEqual to enforce
exact shape equality (replace every expect(...).toMatchObject(...) with
expect(...).toStrictEqual(...)). Look for usages in tests that call
extract(project, ...), and for helper-created projects/configs such as
createTestProject, extract, createOrdersUseCaseConfig, createConfigWithRule, and
createConfigWithCustomTypes; update the assertion calls in those test cases
(including the edge cases, function/class/custom type extraction blocks) so they
assert strict equality.
In
`@packages/riviere-extract-ts/src/features/extraction/domain/connection-detection/connection-detection-values.spec.ts`:
- Around line 35-91: Tests for ConnectionDetectionOptions,
PerModuleConnectionOptions, and CrossModuleConnectionOptions use inconsistent
assertion styles; replace the toMatchObject assertions in the
ConnectionDetectionOptions and PerModuleConnectionOptions specs with
toStrictEqual (or update the CrossModuleConnectionOptions to match, but prefer
toStrictEqual for exact shape checks) so all three suites assert full deep
equality and will fail on unexpected extra fields; update the assertions around
the result variables in those tests (look for the expect(result).toMatchObject
calls and change them to expect(result).toStrictEqual with the same expected
object) to keep assertion style consistent across the three suites.
In `@packages/riviere-role-enforcement/role-enforcement-plugin.mjs`:
- Around line 555-561: hasRequiredPrivateMember misses private constructor
parameter properties (TSParameterProperty) because it only checks class body
members' keys; update hasRequiredPrivateMember to also inspect the class's
constructor parameters for TSParameterProperty/parameter properties and treat
their identifier names (after normalizeRequiredPrivateMemberName) as valid
private members. Specifically, keep the existing check over classNode.body.body
using isPrivateMember and readMemberName, and add a secondary check that finds
the ClassMethod with kind 'constructor' in classNode.body.body, iterates its
value.params to detect TSParameterProperty nodes or params with accessibility
modifier, extracts their identifier names, normalizes them via
normalizeRequiredPrivateMemberName, and returns true if any match the required
privateMemberName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.spec.ts (1)
99-123: 🧹 Nitpick | 🔵 TrivialStale test name and likely redundant case.
After dropping
configDir/glob selection fromextractComponents, this test no longer exercises any "outside configDir" behaviour — there is noconfigDirin the helper anymore. Functionally it now duplicates the preceding'extracts components when source file paths are absolute and module paths are relative'case (only the absolute-path prefix differs). Either remove it or rename it to a distinct intent (e.g. "extracts components regardless of where the absolute path is rooted") so the test title still matches what the assertion verifies.🤖 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/component-extraction/extractor.spec.ts` around lines 99 - 123, The test titled 'extracts components when absolute file path is outside configDir' is now redundant because configDir/glob logic was removed; either delete this test or rename it to reflect current behavior (e.g., 'extracts components regardless of absolute path root') and ensure assertions still match; locate the case in extractor.spec.ts where extract(project, [absolutePath], createOrdersUseCaseConfig()) is invoked and either remove that it(...) block or update its description (and optionally add a comment referencing extract and DraftComponent) so the test intent matches the assertion using extract and createOrdersUseCaseConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/riviere-extract-ts/src/features/extraction/domain/config-resolution/resolve-config.spec.ts`:
- Line 8: The nested describe('modules without extends', ...) is now redundant;
move the four it(...) test cases currently inside that inner block up one level
so they are direct children of describe('resolveConfig', ...), and remove the
describe('modules without extends', ...) wrapper to flatten the suite while
keeping each it(...) test unchanged.
---
Outside diff comments:
In
`@packages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.spec.ts`:
- Around line 99-123: The test titled 'extracts components when absolute file
path is outside configDir' is now redundant because configDir/glob logic was
removed; either delete this test or rename it to reflect current behavior (e.g.,
'extracts components regardless of absolute path root') and ensure assertions
still match; locate the case in extractor.spec.ts where extract(project,
[absolutePath], createOrdersUseCaseConfig()) is invoked and either remove that
it(...) block or update its description (and optionally add a comment
referencing extract and DraftComponent) so the test intent matches the assertion
using extract and createOrdersUseCaseConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c45207d-056e-46be-9c3b-4a97d9adbf5b
📒 Files selected for processing (13)
.riviere/role-enforcement.config.ts.riviere/roles.tspackages/riviere-extract-ts/src/features/extraction/domain/component-extraction/extractor.spec.tspackages/riviere-extract-ts/src/features/extraction/domain/config-resolution/resolve-config.spec.tspackages/riviere-role-enforcement/src/features/enforcement/domain/role-enforcement-builder.tstools/dev-workflow-v2/src/features/workflow/domain/fixtures/workflow-test-fixtures.tstools/dev-workflow-v2/src/features/workflow/domain/workflow-events.tstools/dev-workflow-v2/src/features/workflow/domain/workflow-types.tstools/dev-workflow-v2/src/features/workflow/domain/workflow.tstools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.tstools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.spec.tstools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.tstools/dev-workflow-v2/src/shell/opencode-plugin.ts
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dev-workflow-v2/agents/architecture-review.md`:
- Around line 52-54: Update the wording in the "Consumer-Mapping Ownership
Check" section to use the more precise term "write port" instead of the generic
"port" wherever the list reads "port, presenter, formatter, bridge, translator,
or adapter"; change the list item to "write port, presenter, formatter, bridge,
translator, or adapter" and ensure the phrasing aligns with the source skill
wording used in plan-review (the anti-pattern targets output/write ports mapping
domain results to consumer APIs).
In `@tools/dev-workflow-v2/src/features/workflow/domain/fold.ts`:
- Line 131: Replace the boolean short-circuit spread for transcriptPath with the
explicit ternary-object spread to match the codebase idiom: change the
expression that currently uses ...(event.transcriptPath !== undefined && {
transcriptPath: event.transcriptPath }) to use a ternary that spreads either {
transcriptPath: event.transcriptPath } or an empty object when
event.transcriptPath is undefined; update the occurrence in fold.ts where
event.transcriptPath is used so it mirrors the ternary-style conditional spreads
used elsewhere (e.g., workflow.ts).
In
`@tools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 15-22: The import of Workflow should be type-only since this file
only uses it for the type of Parameters<typeof Workflow.rehydrate>; change the
import statement to "import type { Workflow } ..." so the runtime value isn't
retained and the module graph stays clean—update the import that currently reads
"import { Workflow } from '../../domain/workflow'" and leave the rest of the
file (including the use of Workflow.rehydrate in the type alias WorkflowDeps)
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84e3cb48-ca3c-4730-a0a0-cf9da12b1d89
📒 Files selected for processing (10)
tools/dev-workflow-v2/agents/architecture-review.mdtools/dev-workflow-v2/src/features/workflow/domain/fixtures/workflow-test-fixtures.tstools/dev-workflow-v2/src/features/workflow/domain/fold.tstools/dev-workflow-v2/src/features/workflow/domain/workflow-events.tstools/dev-workflow-v2/src/features/workflow/domain/workflow-types.tstools/dev-workflow-v2/src/features/workflow/domain/workflow.tstools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.tstools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.spec.tstools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.tstools/dev-workflow-v2/src/shell/opencode-plugin.ts



Summary
Closes #361
Summary by CodeRabbit
New Features
Documentation
Refactor
Coverage (first 10 changed source files)
packages/riviere-builderpackages/riviere-clipackages/riviere-extract-tstools/dev-workflow-v2