protographic/src/sdl-to-proto-visitor.ts (1)

224-250: Simplify proto option wiring and double‑check go_package default behavior

You’re manually reconstructing the ProtoOptions bag before calling buildProtoOptions. Since GraphQLToProtoTextVisitorOptions already extends ProtoOptions, you can avoid drift by passing options directly:

-    const protoOptionsFromLanguageProps = buildProtoOptions(
-      {
-        goPackage: options.goPackage,
-        javaPackage: options.javaPackage,
-        javaOuterClassname: options.javaOuterClassname,
-        javaMultipleFiles: options.javaMultipleFiles,
-        csharpNamespace: options.csharpNamespace,
-        rubyPackage: options.rubyPackage,
-        phpNamespace: options.phpNamespace,
-        phpMetadataNamespace: options.phpMetadataNamespace,
-        objcClassPrefix: options.objcClassPrefix,
-        swiftPrefix: options.swiftPrefix,
-      },
-      packageName,
-    );
+    const protoOptionsFromLanguageProps = buildProtoOptions(options, packageName);

Also note that buildProtoOptions currently only emits go_package when options.goPackage is truthy, so the “Generate default go_package if not provided” behavior isn’t actually triggered when goPackage is omitted. If you intended to derive a default from packageName, consider relaxing that guard in buildProtoOptions in a follow‑up.

protographic/tests/operations/field-numbering.test.ts (1)

1-224: FieldNumberManager behavior is well covered

These tests exercise all the important behaviors of createFieldNumberManager (per‑message sequencing, manual assignment, resets, and mixed usage), which should make regressions obvious. If you ever wire a ProtoLockManager into this flow, consider an additional test that passes a stub lock manager and asserts reconcileFieldOrder / getLockManager interactions, but that’s optional for now.

protographic/src/types.ts (1)

1-14: Use a type‑only namespace import for protobuf types

Since protobuf is only used for typing and the rest of the codebase uses a namespace import, it’s cleaner and safer to switch to a type‑only namespace import:

-import protobuf from 'protobufjs';
+import type * as protobuf from 'protobufjs';

This avoids relying on a synthetic default export, prevents an unnecessary runtime import when compiling with importsNotUsedAsValues = 'remove', and keeps the style consistent with other modules using protobufjs.

protographic/tests/operations/request-builder.test.ts (1)

241-299: Prefer GraphQL type guards over constructor name checks

In multiple places you validate the looked‑up type with:

if (!inputType || inputType.constructor.name !== 'GraphQLInputObjectType') {
  throw new Error('Invalid input type');
}

Since you already import GraphQLInputObjectType, or could import isInputObjectType, this can be made more robust and intention‑revealing:

import { /* ..., */ isInputObjectType } from 'graphql';

const inputType = schema.getType('UserInput');
if (!inputType || !isInputObjectType(inputType)) {
  throw new Error('Invalid input type');
}

This avoids relying on constructor.name (which can be brittle under minification or across multiple GraphQL instances) and keeps the tests aligned with GraphQL’s own helpers.

protographic/tests/operations/fragments.test.ts (1)

324-353: Tighten fragment test naming and expectations

Two small nits:

• The test should handle fragments that reference non-existent fragments actually uses a single defined fragment (UserFields) and doesn’t exercise missing/undefined fragments. If the intent is simply “basic fragment usage without cycles”, consider renaming to avoid confusion for future readers.
• In should handle __typename field in fragments, the snapshot implicitly asserts that __typename is dropped, but nothing explicitly checks for its absence. You could make the intent clearer and guard against regressions by also asserting that proto does not contain __typename (or the corresponding field name) in addition to the snapshot.

These are purely clarity/coverage tweaks; the existing assertions already exercise the core behavior.

Also applies to: 1849-1905

protographic/tests/operations/proto-text-generator.test.ts (1)

6-11: Reuse the exported MethodWithIdempotency type to avoid drift

You’re redefining a local MethodWithIdempotency interface that matches the production type:

interface MethodWithIdempotency extends protobuf.Method {
  idempotencyLevel?: 'NO_SIDE_EFFECTS' | 'DEFAULT';
}

Now that MethodWithIdempotency (and IdempotencyLevel) are exported from src/types.ts / src/index.ts, you could import and use that instead:

import type { MethodWithIdempotency } from '../../src';

This keeps the tests in sync if you ever extend the allowed idempotency levels or adjust the interface.

protographic/tests/operations/recursion-protection.test.ts (1)

294-392: Clarify edge‑case test names and consider softening the timing assertion

Two minor suggestions:

• should handle empty fragments gracefully and should handle fragments that reference non-existent fragments don’t match their implementations: the former’s fragment isn’t empty, and the latter doesn’t reference any missing fragment. Renaming them to reflect what they actually test (e.g., “simple fragment spread without cycles”) would make the suite easier to read.
• In should handle reasonable depth efficiently, asserting endTime - startTime < 1000 is probably fine, but it does introduce a small risk of flakiness on slow or contended CI runners. If this ever becomes noisy, you could either relax the threshold further or drop the timing assertion and rely on the fact that the test will naturally fail if compilation becomes pathologically slow.

Functionally everything here looks correct; these are just resilience/clarity tweaks.

Also applies to: 395-431

protographic/src/operation-to-proto.ts (3)

181-212: Type fieldNumberManager explicitly to align with FieldNumberManager interface

fieldNumberManager is declared without a type annotation and only assigned in the constructor via createFieldNumberManager(this.lockManager). This effectively makes it any under typical TypeScript settings, which loses the structural contract (e.g., reconcileFieldOrder, getLockManager, etc.) and weakens IDE support.

Consider importing the type and annotating the field:

import type { FieldNumberManager } from './operations/field-numbering.js';

class OperationsToProtoVisitor {
  // ...
  private readonly fieldNumberManager: FieldNumberManager;
  // ...
}

This keeps the implementation unchanged while tightening type-safety around the field-number integration.

---

281-291: Reuse createOperationMethodName to keep RPC naming conventions centralized

processOperation currently derives the method name via upperFirst(camelCase(operationName)), even though a dedicated createOperationMethodName helper exists and is already imported. Duplicating the naming logic here can lead to drift if naming rules evolve (e.g., handling of punctuation, acronyms, or legacy conventions used elsewhere in protographic).

You can delegate to the shared helper and then apply the optional operation-type prefix on top, e.g.:

let methodName = createOperationMethodName(operationName);

if (this.prefixOperationType) {
  const operationTypePrefix = upperFirst(node.operation.toLowerCase());
  methodName = `${operationTypePrefix}${methodName}`;
}

This keeps operations-based generation aligned with the rest of the codebase’s RPC naming semantics.

---

439-448: Consider a clearer error when the schema lacks the root operation type

getRootType uses non-null assertions on schema.getQueryType(), getMutationType(), and getSubscriptionType(). If a caller passes a schema without a matching root type for the operation (e.g., a mutation operation against a schema without mutation root), this will blow up with a generic runtime error rather than a targeted message.

Since you already validate the operations, this is likely rare, but you could improve diagnostics by explicitly checking for null and throwing a more descriptive error:

private getRootType(operationType: OperationTypeNode): GraphQLObjectType {
  switch (operationType) {
    case OperationTypeNode.QUERY: {
      const type = this.schema.getQueryType();
      if (!type) throw new Error('Schema does not define a query root type.');
      return type;
    }
    // similar for MUTATION/SUBSCRIPTION
  }
}

This is non-blocking but makes failures easier to debug when the schema and operations get out of sync.

protographic/src/operations/proto-text-generator.ts (1)

148-188: Avoid duplicating method formatting logic between serviceToProtoText and formatMethod

Right now serviceToProtoText hand-rolls RPC formatting (including idempotency handling), while formatMethod independently formats a single rpc line without idempotency support. This creates two slightly different code paths for essentially the same concern.

Consider either:

• Making serviceToProtoText delegate to an enhanced formatMethod that also understands MethodWithIdempotency, or
• Updating formatMethod to mirror the idempotency/streaming logic so callers using it directly get behaviour consistent with serviceToProtoText.

This would reduce drift if you later tweak RPC formatting (e.g., adding more per-method options).

Also applies to: 425-440

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.

Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.

Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.

protographic/src/operations/message-builder.ts (1)

526-552: LGTM!

The function correctly builds nested messages from a field map. The field numbering logic appropriately uses the manager when provided (lines 538-541) and falls back to simple incrementing otherwise (lines 546-548).

Note: Unlike buildMessageFromSelectionSet, this function doesn't reconcile field order with lock data. This appears intentional for this simpler utility, but if lock-stable ordering is needed, consider adding reconcileFieldOrder support similar to lines 172-174 in buildMessageFromSelectionSet.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.

Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.

protographic/OPERATIONS_TO_PROTO.md (1)

59-59: Fix remaining MD036 linting violations: bold emphasis at line start.

Lines 59 and 68 still use bold emphasis markers at the start of the line, triggering the MD036 linting rule. This was flagged in the previous review.

Apply this diff to move the emoji outside the bold emphasis:

-**✅ Correct: Named operation**
+✅ **Correct: Named operation**
 ```graphql

-**❌ Incorrect: Anonymous operation**
+❌ **Incorrect: Anonymous operation**
 ```graphql

Also applies to: 68-68

cli/src/commands/grpc-service/commands/generate.ts (2)

278-328: Handle or explicitly guard against duplicate proto symbols during AST merge

Merging via protobuf.Root and a single synthetic Service is a good direction, but the current logic silently dedupes messages/enums by name (first one wins) and unconditionally adds all methods to mergedService:

if (!seenMessages.has(message.name)) { ... }
...
if (!seenEnums.has(enumType.name)) { ... }
...
for (const method of Object.values(service.methods)) {
  mergedService.add(method);
}

If two operation documents accidentally produce:

• different message/enum definitions with the same name, or
• RPC methods with the same name,

you may end up with either confusing behavior (wrong type chosen) or a less-than-obvious error from protobuf.Service.add. To make failures clearer and avoid silent conflicts, consider explicitly detecting duplicates and failing fast with a targeted error, for example:

-  const mergedService = new protobuf.Service(serviceName);
+  const mergedService = new protobuf.Service(serviceName);
+  const seenMethods = new Set<string>();
   ...
       } else if (nested instanceof protobuf.Service) {
         const service = nested as protobuf.Service;
         for (const method of Object.values(service.methods)) {
-          mergedService.add(method);
+          if (seenMethods.has(method.name)) {
+            throw new Error(
+              `Duplicate RPC method "${method.name}" detected while merging operations. ` +
+                'Ensure operation names (and thus RPC names) are unique across files.',
+            );
+          }
+          seenMethods.add(method.name);
+          mergedService.add(method);
         }
       }

Similarly, if compile-time guarantees do not ensure that same-named messages/enums are structurally identical, it may be worth checking and throwing on incompatible duplicates rather than silently picking the first.

---

541-557: Custom scalar mapping parsing covers both inline JSON and @file; consider light validation

The parseCustomScalarMapping helper correctly supports both:

• inline JSON strings, and
• @path/to/file.json syntax with a clear error when the file is missing,

which matches the CLI help text. One small optional improvement would be to validate the parsed value before returning, to fail fast on unexpected shapes:

-  if (input.startsWith('@')) {
-    ...
-    const fileContent = await readFile(filePath, 'utf8');
-    return JSON.parse(fileContent);
-  }
-  // Otherwise, treat as inline JSON
-  return JSON.parse(input);
+  const raw = input.startsWith('@')
+    ? JSON.parse(await readFile(filePath, 'utf8'))
+    : JSON.parse(input);
+
+  if (!raw || typeof raw !== 'object' || Array.isArray(raw)) {
+    throw new Error('Custom scalar mapping must be a JSON object of { "GraphQLScalar": "ProtoType" } pairs.');
+  }
+  return raw as Record<string, string>;

Not critical, but it would give users a clearer error if they accidentally pass an array or some other invalid JSON shape.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.

Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.

protographic/OPERATIONS_TO_PROTO.md (1)

59-68: Resolve remaining MD036 linting violations on example labels.

Lines 59 and 68 still begin with bold emphasis (**), which markdownlint flags as MD036 violations despite prior review comments indicating these were addressed. Reposition the emoji to the line start and apply bold to the label text only.

-**✅ Correct: Named operation**
+✅ **Correct: Named operation**

-**❌ Incorrect: Anonymous operation**
+❌ **Incorrect: Anonymous operation**

cli/src/commands/grpc-service/commands/generate.ts (1)

258-261: Sort operation files for deterministic output.

fs.promises.readdir() does not guarantee entry ordering across platforms. To ensure deterministic proto generation and consistent RPC/method ordering, the filtered operation files should be sorted before processing.

Apply this diff:

-  const operationFiles = files.filter((file) => {
-    const ext = extname(file).toLowerCase();
-    return validExtensions.includes(ext);
-  });
+  const operationFiles = files
+    .filter((file) => validExtensions.includes(extname(file).toLowerCase()))
+    .sort();

cli/src/commands/grpc-service/commands/generate.ts (1)

434-464: Consider consolidating language options handling.

The manual construction of the protoOptions array from languageOptions (lines 434-464) and the similar pattern in lines 171-182 creates duplication. While the current implementation is correct and functional, consolidating this logic into a helper function or using the protographic library's built-in options handling would improve maintainability.

Based on learnings

Example refactor:

function buildProtoOptions(languageOptions: ProtoOptions): ProtoOption[] {
  const options: ProtoOption[] = [];
  
  if (languageOptions.goPackage) {
    options.push(getGoModulePathProtoOption(languageOptions.goPackage));
  }
  if (languageOptions.javaPackage) {
    options.push({ name: 'java_package', constant: `"${languageOptions.javaPackage}"` });
  }
  // ... etc
  
  return options;
}

Then use: const protoOptions = buildProtoOptions(languageOptions);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.

Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.

protographic/OPERATIONS_TO_PROTO.md (1)

59-59: MD036 violations persist: emphasis still used instead of headings.

The format **✅ Correct: Named operation** and **❌ Incorrect: Anonymous operation** starts with bold emphasis, which still violates markdownlint rule MD036. Previous review comments flagged this issue with suggested fixes, but the violations remain in the current code.

Apply this diff to move the emoji outside the emphasis:

-**✅ Correct: Named operation**
+✅ **Correct: Named operation**

-**❌ Incorrect: Anonymous operation**
+❌ **Incorrect: Anonymous operation**

Also applies to: 68-68

cli/src/commands/grpc-service/commands/generate.ts (2)

223-225: Remove or conditionalize the hardcoded query idempotency display.

Since queryIdempotency is intentionally hardcoded to 'NO_SIDE_EFFECTS' (per line 376's comment) and not exposed as a CLI option, unconditionally displaying it in the results may confuse users who expect to configure it. Either omit this line or add a comment explaining it's a fixed default for operations mode.

   if (result.isOperationsMode) {
-    resultInfo['query idempotency'] = 'NO_SIDE_EFFECTS';
+    // queryIdempotency is hardcoded to NO_SIDE_EFFECTS for all Query operations in operations mode
+    // and is not user-configurable in this release
+    resultInfo['query idempotency'] = 'NO_SIDE_EFFECTS (default)';
   }

---

255-275: Sort operation files for deterministic output.

The filtered operation files are not sorted before processing, which can lead to non-deterministic RPC method ordering in the merged proto across different platforms or filesystem types.

   const operationFiles = files.filter((file) => {
     const ext = extname(file).toLowerCase();
     return validExtensions.includes(ext);
-  });
+  }).sort();

cli/src/commands/grpc-service/commands/generate.ts (2)

277-327: Consider validating structural equality of duplicate messages/enums.

The merge logic silently skips duplicate messages and enums by name (lines 302-304, 309-311) without verifying structural equality. While duplicates should be identical when all operations reference the same schema, adding a structural comparison or warning would make silent conflicts more visible during development.

Example validation at line 303:

         if (!seenMessages.has(message.name)) {
           mergedRoot.add(message);
           seenMessages.add(message.name);
+        } else {
+          // Optional: validate that duplicate messages are structurally identical
+          const existing = mergedRoot.lookup(message.name);
+          // Compare field names, types, and numbers - implementation omitted for brevity
         }

---

436-466: Consider refactoring verbose protoOptions construction.

The protoOptions array is built with repetitive if-statements checking each language option. While functional, this could be simplified with a mapping structure or helper function to reduce duplication and improve maintainability.

Example approach:

const optionMappings: Array<{ key: keyof ProtoOptions; name: string; needsQuotes?: boolean }> = [
  { key: 'javaPackage', name: 'java_package', needsQuotes: true },
  { key: 'javaOuterClassname', name: 'java_outer_classname', needsQuotes: true },
  // ... etc
];

const protoOptions: ProtoOption[] = [];
for (const { key, name, needsQuotes } of optionMappings) {
  const value = languageOptions[key];
  if (value !== undefined) {
    const constant = needsQuotes ? `"${value}"` : String(value);
    protoOptions.push({ name, constant });
  }
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.

Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.