Preserve restored Terraform root identifiers

[thomhurst]

Summary

• keep restored nested command facades on their root sub-domain identifier
• preserve current explicit identifiers and baseline root casing
• prevent Terraform stacks restoration from producing conflicting Stacks, StacksConfiguration, StacksDeploymentGroup, and StacksDeploymentRun identifiers
• add a nested-facade restoration regression

Validation

• focused regression: 1/1 passed
• full OptionsGenerator tests: 987/987 passed with coverage
• OptionsGenerator Release build: 0 warnings, 0 errors
• diff check clean

This is stacked on #4006 because Terraform reaches this path only after repeatable-option generation succeeds. Rebase onto main after #4006 merges.

Refs #3996

Summary by CodeRabbit

• Bug Fixes

  • Improved compatibility when restoring nested or aliased commands.
  • Preserved established command identifiers, including custom names and historical casing.
  • Correctly distinguishes literal execute commands from parent command facades.
  • Consolidates removed parent and child commands under a consistent restored root.
  • Rejects conflicting command identifiers to prevent ambiguous command hierarchies.

• Tests

  • Added coverage for command restoration scenarios to help prevent future compatibility regressions.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 782f7518-7496-482d-9901-e1315f0de23f

📥 Commits

Reviewing files that changed from the base of the PR and between 74b6dc4 and 86d1969.

📒 Files selected for processing (4)

• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GeneratorHardeningTests.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratedApiCompatibilityPreserver.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CliCommandDefinition.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CommandTreeNode.cs

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

GeneratedApiCompatibilityPreserver now restores command roots, groups, casing, and command-part identifiers from current groups, baseline commands, facade metadata, and implementation types. CommandTreeNode applies these overrides and rejects conflicts. Tests cover nested and aliased command facades.

Changes

Command facade restoration

Layer / File(s)	Summary
Command definition and tree population tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CliCommandDefinition.cs, tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CommandTreeNode.cs	CliCommandDefinition stores command-part identifier overrides. CommandTreeNode groups segments case-insensitively, applies compatible overrides, and throws InvalidOperationException for conflicting overrides.
Restored command group resolution tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratedApiCompatibilityPreserver.cs	Restored commands resolve unique facade-derived or current group identifiers. Facade suffixes provide command-part overrides when the split is unambiguous.
Facade restoration hardening tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GeneratorHardeningTests.cs	Tests cover historical casing, nested roots, custom identifiers, literal execute, missing groups, live siblings and children, length-changing command paths, and conflicting identifiers.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 86d19

The PR preserves restored Terraform identifiers and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Preserver as GeneratedApiCompatibilityPreserver
  participant Baseline as Baseline command
  participant Current as Current command groups
  participant Facades as Facade implementation types
  participant Tree as CommandTreeNode
  Preserver->>Baseline: Read restored command metadata
  Preserver->>Current: Resolve a unique group identifier
  Current-->>Preserver: Return live group or fallback identifier
  Preserver->>Facades: Normalize types and parse facade suffixes
  Facades-->>Preserver: Return command-part identifier overrides
  Preserver->>Tree: Submit command definition
  Tree-->>Tree: Apply overrides or reject conflicts

Loading

Poem

A rabbit checks each command trail,
Root names stand and do not fail.
Casing follows the history,
execute keeps its place in mystery.
Conflicts stop at the gate.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: preserving Terraform root identifiers during restoration.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/terraform-restored-subdomain-identifiers-20260824

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The PR preserves historical root and nested identifiers when restoring removed generated command facades.

• Recovers root identifiers from historical facades, current command groups, and baseline option types.
• Carries historical nested casing into command-tree generation and rejects conflicting explicit identifiers.
• Adds regressions for custom roots, parent/child restoration, mixed facades, literal execute commands, and live siblings.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratedApiCompatibilityPreserver.cs	Restores historical root and nested identifiers from facade, current-command, and baseline naming information.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CommandTreeNode.cs	Applies explicit nested identifiers while recursively grouping commands and rejects irreconcilable casing conflicts.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CliCommandDefinition.cs	Adds immutable per-command metadata for historical nested identifier overrides.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GeneratorHardeningTests.cs	Adds comprehensive restoration regressions covering the previously reported root and mixed-facade failures.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Generated API baseline] --> B[Restore removed command]
    C[Current command definitions] --> B
    B --> D[Recover root identifier]
    B --> E[Recover nested identifiers]
    D --> F[Resolve sub-domain group]
    E --> G[Build command tree]
    F --> G
    G --> H[Preserved generated facade]

Loading

_{Reviews (17): Last reviewed commit: "refactor(generator): unify fallback reco..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4ebdb2a9a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #4045

Summary: This PR fixes RestoreRemovedCommand to derive the restored command group identifier from the baseline definition rather than an arbitrary facade method (facadeMethods[0]), and adds a regression test for the multi-facade case. Good fix for the original bug — using the first facade method's declaring type as a proxy for the group identifier was fragile.

Issue found

GeneratedApiCompatibilityPreserver.cs:146-177 — the length-truncation fallback silently discards tool-specific identifier overrides that differ in length from the mechanical default.

When the restored command's whole group has vanished from the current scrape (currentIdentifiers.Length == 0, i.e. no live command shares CommandParts[0]), the code falls back to reconstructing the identifier from baseline.ClassName:

if (baseline.ClassName.StartsWith(tool.NamespacePrefix, StringComparison.Ordinal)
    && baseline.ClassName.AsSpan(tool.NamespacePrefix.Length)
        .StartsWith(defaultIdentifier, StringComparison.OrdinalIgnoreCase))
{
    return baseline.ClassName.Substring(tool.NamespacePrefix.Length, defaultIdentifier.Length);
}

return defaultIdentifier;

This assumes the true identifier is always the same length as defaultIdentifier = ToPascalCase(rootCommand). That assumption breaks for any scraper that overrides NormalizeCommandIdentifier to a longer/shorter string than the mechanical Pascal-case conversion — which is exactly what ArgoCdCliScraper does:

// ArgoCdCliScraper.cs
protected override string NormalizeCommandIdentifier(string commandPart) =>
    commandPart.Equals("appset", StringComparison.OrdinalIgnoreCase)
        ? "ApplicationSet"
        : base.NormalizeCommandIdentifier(commandPart);

Concrete failure scenario: if the entire appset ... command group disappears from a fresh CLI scrape (e.g. ArgoCD removes/renames it) while old generated facades for it still exist, RestoreRemovedCommand needs to regenerate appset create. currentIdentifiers is empty (no live appset commands), defaultIdentifier = "Appset", and baseline.ClassName = "ArgoCdApplicationSetCreateOptions". AsSpan(6) = "ApplicationSetCreateOptions", which does not start with "Appset" (mismatch at index 3: l vs s), so the StartsWith guard fails and the method falls through to return defaultIdentifier = "Appset" — instead of the correct "ApplicationSet". This reintroduces the exact class-name collision with the two-word app set command that the override exists to prevent (per the comment on NormalizeCommandIdentifier).

Even in the case where the StartsWith check does pass, truncating to defaultIdentifier.Length is still wrong in general — it can only ever reproduce defaultIdentifier itself (since a same-prefix, same-length substring starting with defaultIdentifier case-insensitively is defaultIdentifier when they're the same length), so the "success" branch is effectively dead code that never returns anything other than what the "fail" branch also returns. Either way, this fallback can never recover a genuinely different override string once no live command remains for the root — it can only recover the mechanical default.

Suggested fix: rather than reconstructing the identifier from a length-sliced substring of the baseline class name, prefer preserving the actual override text. Two options:

1. Store CommandGroupIdentifierOverride (or an equivalent "the identifier this baseline command actually used") directly on GeneratedApiBaseline when it's captured, so restoration never needs to reverse-engineer it from a mangled class name.
2. If baseline doesn't carry that field, search sibling baseline entries under the same root for their group identifier consistently, or fall back to stripping the known command-part suffixes (CreateOptions, etc.) from ClassName rather than assuming a fixed length match with defaultIdentifier.

The added test (ApiCompatibilityPreserver_Uses_Root_Identifier_For_Restored_Nested_Facades) only covers the case where defaultIdentifier and the true override happen to be textually identical ("group" → "Group" either way), so it doesn't exercise this path — worth adding a case that mirrors the ArgoCD appset/ApplicationSet override to lock in the correct behavior.

---

Reviewed with the code-review skill against the current diff (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratedApiCompatibilityPreserver.cs), cross-checked against ArgoCdCliScraper.cs and CobraCliScraper.cs to confirm the override mechanism this fallback needs to preserve.

[thomhurst]

Fixed the Greptile custom-normalization finding at exact head ead0cb8. Baseline option types now recover the historical root identifier by stripping the tool prefix and non-root command suffix, preserving ArgoCdApplicationSetCreateOptions as ApplicationSet while nested facades still share one root. Added an appset regression. Validation: OptionsGenerator tests 988/988; Release build 0 warnings/errors; diff check clean.

[claude]

Code Review — PR #4045

Summary: Good incremental fix. RestoreRemovedCommand now derives the group identifier from baseline instead of facadeMethods[0].DeclaringType, and correctly recovers custom-normalized identifiers (e.g. ArgoCD's appset → ApplicationSet) by stripping the namespace prefix and command suffix off baseline.ClassName when no live sibling remains. The new regression tests (ApiCompatibilityPreserver_Uses_Root_Identifier_For_Restored_Nested_Facades, ApiCompatibilityPreserver_Preserves_Custom_Root_Identifier_For_Restored_Facades) correctly lock in both the shared-root and custom-override cases raised in earlier review rounds.

I verified the diff against the PR head (ead0cb8) and found a few things worth addressing, roughly in order of severity:

1. SubDomainGroup for restored commands isn't normalized the same way live-scraped commands are

RestoreRemovedCommand (line 136, unchanged by this diff) sets:

SubDomainGroup = commandParts.Length > 1 ? commandParts[0] : null,

i.e. the raw command-part text (e.g. "stacks"). Every live scraper (TerraformCliScraper.cs:162, CobraCliScraper.cs:167, etc.) instead sets SubDomainGroup to the PascalCase/override-normalized identifier (e.g. "Stacks").

CliToolDefinition.SubDomainGroups (CliToolDefinition.cs:109-114) dedups these with a case-sensitive .Distinct(). So if a tool's stacks group is only partially removed — some terraform stacks * commands still exist, one was removed and needs restoring — you get two distinct SubDomainGroups entries, "Stacks" and "stacks", even though GetSubDomainIdentifier (used everywhere downstream, including SubDomainClassGenerator.cs:53) matches them case-insensitively and would resolve both to the same identifier. That means the same sub-domain class/tree gets generated twice — a duplicate-output bug of the exact same shape this PR is fixing for CommandGroupIdentifierOverride, just on the sibling SubDomainGroup field. Worth normalizing this the same way (GeneratorUtils.ToPascalCase(commandParts[0]), or better, reuse GetRestoredCommandGroupIdentifier's result) while you're in this code.

2. currentIdentifiers duplicates GetSubDomainIdentifier's logic with subtly different semantics

GetRestoredCommandGroupIdentifier (line ~157) reimplements "resolve the override for a group" inline:

.Select(command => command.CommandGroupIdentifierOverride ?? defaultIdentifier)
.Distinct(StringComparer.OrdinalIgnoreCase)

but GeneratorUtils.GetSubDomainIdentifier (GeneratorUtils.cs:1134-1157), which is the canonical version of this concept and the thing that actually validates/throws on genuine conflicts at generation time, filters out null overrides before deduping and matches on SubDomainGroup rather than raw CommandParts[0]. Two parallel implementations of the same idea will drift — a future edge case fixed in one won't automatically apply to the other. Given point 1 above (restored commands have a differently-cased SubDomainGroup than CommandParts[0]), these two functions can genuinely disagree on what the "current" identifiers for a group are. Consider calling GetSubDomainIdentifier directly (after fixing SubDomainGroup per point 1) instead of re-deriving the same answer a second way.

3. Suffix-reconstruction fallback is override-unaware for non-root command parts

The fallback that recovers the identifier from baseline.ClassName builds commandSuffix from every part after the root using the generic GeneratorUtils.ToPascalCase:

var commandSuffix = string.Concat(
    baseline.CommandParts!.Skip(1).Select(GeneratorUtils.ToPascalCase)) + "Options";

This ignores any scraper's NormalizeCommandIdentifier override for a non-root segment (the same mechanism ArgoCdCliScraper/EksctlCliScraper use for the root segment, which this PR now handles). No current scraper happens to apply such an override to a non-root part, so this won't fire today, but it's a latent trap: if one ever does, EndsWith(commandSuffix) silently fails to match and the code falls through to the unqualified defaultIdentifier, producing a wrong-but-plausible restored identifier with no error. Given point 1's suggestion to centralize identifier recovery, it'd be worth having this reconstruction go through the same override-aware path rather than hand-rolling ToPascalCase again.

None of these are regressions introduced by this diff except where noted — they're pre-existing gaps adjacent to the code this PR touches, surfaced because the PR's whole point is fixing identifier-conflict bugs in this exact function. Given that, I'd lean toward folding at least #1 into this PR (it's a one-line change and directly undermines the guarantee the PR is trying to provide), and leaving #2/#3 as a follow-up if you'd rather keep this PR scoped to the reported bug.

---

Reviewed with the code-review skill; cross-checked against CliToolDefinition.cs, GeneratorUtils.cs, TerraformCliScraper.cs, CobraCliScraper.cs, and ArgoCdCliScraper.cs/EksctlCliScraper.cs to confirm the normalization mechanisms this diff needs to stay consistent with.

[thomhurst]

Fixed the restored SubDomainGroup normalization finding at exact head bdfe2f6708e32b271bee8273e699f56585ca2259. Restored commands now use the recovered group identifier, matching live scrapers (Stacks, ApplicationSet) and preventing casing-variant duplicate sub-domain generation. The regression now covers a partially removed root alongside a live normalized sibling and verifies one deduplicated group; custom ArgoCD normalization is also asserted.

Validation: GeneratorHardeningTests 101/101; full OptionsGenerator suite 988/988; Release build 0 warnings/errors; diff check clean. Format verification remains blocked only by pre-existing IHelpTextCache.cs whitespace and unrelated info diagnostics.

@claude Please re-review exact head bdfe2f6708e32b271bee8273e699f56585ca2259, specifically confirming finding #1 is resolved.

[claude]

Code Review — PR #4045 (re-review at bdfe2f67)

Finding #1 confirmed resolved. The last commit (bdfe2f67) changes:

-            SubDomainGroup = commandParts.Length > 1 ? commandParts[0] : null,
+            SubDomainGroup = commandParts.Length > 1 ? groupIdentifier : null,

Restored commands now set SubDomainGroup from the same normalized groupIdentifier used for CommandGroupIdentifierOverride, instead of the raw lowercase commandParts[0]. That closes the casing-variant duplicate-group gap I flagged: a partially-removed root (some group * commands still live, one restored) previously could yield both "Group" and "group" in CliToolDefinition.SubDomainGroups, which — despite GetSubDomainIdentifier matching them case-insensitively downstream — meant the same sub-domain class/tree could be generated twice.

The updated ApiCompatibilityPreserver_Uses_Root_Identifier_For_Restored_Nested_Facades test now exercises exactly that scenario (a live ToolGroupCurrentOptions sibling with subDomainGroup: "Group" alongside two restored facades under the same root) and asserts preserved.SubDomainGroups dedups to a single ["Group"] entry — this is the right regression to lock in the fix. ApiCompatibilityPreserver_Preserves_Custom_Root_Identifier_For_Restored_Facades was updated consistently (SubDomainGroup == "ApplicationSet" instead of "appset").

I verified this against the current head (bdfe2f6708e32b271bee8273e699f56585ca2259, which matches the PR's current head SHA) — nothing else changed in this commit beyond the one-line fix and the two test assertions.

Still open, not blocking: findings #2 (GetRestoredCommandGroupIdentifier's inline currentIdentifiers resolution duplicating GeneratorUtils.GetSubDomainIdentifier's logic) and #3 (the baseline.ClassName suffix-reconstruction fallback not being override-aware for non-root command parts) from the previous review round remain as described — both are pre-existing, non-regressed, and reasonable to leave as a follow-up given this PR is now correctly scoped to the reported bug.

No new issues introduced by this commit. Good to merge from my side.

---

Reviewed with the code-review skill against the current diff, cross-checked against the previous two review rounds on this PR to confirm finding #1 was addressed as requested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdfe2f6708

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Rebased the three-commit stacked diff onto the rewritten #4006 parent at exact head c77b027. Diff remains limited to GeneratedApiCompatibilityPreserver plus its hardening tests. Validation: full OptionsGenerator suite 1013/1013; Release build 0 warnings/errors; diff check clean. @claude Please re-review the exact current head.

[thomhurst]

Rebased the unchanged three-commit stacked diff onto #4006 exact parent 6ff7908fd8bb573478361f810100a92e57f253b9. Current exact head: b4c25781cd92f7bb76ccbc15bd061312a116748c. Diff remains limited to GeneratedApiCompatibilityPreserver and its hardening tests.

Validation: full OptionsGenerator suite 1016/1016; Release build 0 warnings/errors; diff check clean.

@claude Please re-review the exact current head.

[thomhurst]

Fixed the historical facade-casing finding at exact head 34c14cea8aa3b8c2bb0a7c12d82582d3e33ad7f9. When no live sibling identifies a restored root, compatibility restoration now derives a unique root identifier from all matching baseline facade declarations, stripping interface/tool prefixes and intermediate command suffixes. This preserves GcloudCloudshell casing while avoiding the prior arbitrary-first-facade bug for nested groups. Added focused regression. Validation: focused 1/1; full OptionsGenerator suite 1017/1017 with coverage; Release build 0 warnings/errors; scoped format and diff checks clean. @claude Please review this exact head. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34c14cea8a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #4045 (re-review at 34c14cea)

Summary: This commit reworks GetRestoredCommandGroupIdentifier into a three-tier resolver, adding a new middle tier (GetRootIdentifierFromFacade) that extracts the root identifier from a nested facade's declaring type name by stripping the tool's namespace prefix and the middle command-part suffix. This directly targets the PR's stated goal: restoring terraform stacks * no longer produces conflicting Stacks/StacksConfiguration/StacksDeploymentGroup/StacksDeploymentRun identifiers, because the extraction now correctly isolates the root (Stacks) instead of using the raw nested-facade class name segment.

I traced the new logic by hand against 2-part, 3-part, and custom-override command shapes, and checked it against the three new regression tests (ApiCompatibilityPreserver_Uses_Root_Identifier_For_Restored_Nested_Facades, _Preserves_Custom_Root_Identifier_For_Restored_Facades, _Preserves_Historical_Facade_Casing). I didn't find a case that produces a wrong result. Finding #1 from the earlier review rounds (raw commandParts[0] leaking into SubDomainGroup) is confirmed still fixed at this commit.

One architecture note, building on findings #2/#3 from the earlier rounds (both still open, still non-blocking):

The resolver now has four independent strategies for the same underlying question, two of which duplicate identical string-extraction logic

GetRootIdentifierFromFacade and the class-name reconstruction fallback inside GetRestoredCommandGroupIdentifier both do the identical "does this name start with the namespace prefix and end with a computed suffix — if so, return the middle substring" check, just against two different strings (declaringType vs. baseline.ClassName) with two different suffix computations (Skip(1).SkipLast(1) vs. Skip(1) + "Options"). Combined with tier 1 (currentIdentifiers, which independently reimplements GeneratorUtils.GetSubDomainIdentifier's override-resolution — finding #2 from the prior round), this method has grown from 5 lines to roughly 80 across two helpers, with 4 different strategies for "the identifier for this root," each keyed on a slightly different piece of data (live CommandParts[0], live SubDomainGroup via GetSubDomainIdentifier, facade DeclaringType, baseline ClassName).

Since this PR is adding a third tier to a resolver that already had known duplication, the case for consolidating rather than accreting is stronger now than when it was first raised. Concretely: extracting a shared StripPrefixAndSuffix(string value, string prefix, string suffix) helper for the two identical substring-extraction tiers would mean a future fix to the extraction rule (e.g., a namespace prefix that overlaps with a suffix) only needs to be made once, instead of drifting between two copies in the same file.

Not blocking — the new tests demonstrate the fix works for the reported cases, and this is additive complexity rather than a regression. Given this method has now been touched across three PRs in a row for closely related identifier-restoration bugs, it may be worth converting the standing follow-up (#2/#3 plus this note) into a tracked issue so the consolidation doesn't keep getting deferred piecemeal.

No new correctness issues found in this commit. CLAUDE.md compliance: no applicable nested CLAUDE.md under tools/ModularPipelines.OptionsGenerator/, and this diff doesn't touch build/test tooling covered by the root CLAUDE.md.

---

Reviewed with the code-review skill; compared against the two earlier review rounds on this PR and re-verified finding #1 remains fixed at the current head (34c14cea8aa3b8c2bb0a7c12d82582d3e33ad7f9).

[thomhurst]

Fixed the live-sibling group-key finding at exact head ef2e258. Restored commands now reuse the unique current SubDomainGroup for their CLI root while keeping the recovered identifier as CommandGroupIdentifierOverride, preventing appset/ApplicationSet duplicate groups. Validation: full OptionsGenerator suite 1017/1017; Release build 0 warnings/errors; diff check clean. @claude Please re-review exact current head.

[thomhurst]

Fixed the wholly-removed parent/child root split at exact head 3da0fc22e844b4b18a09f90d820346380f85416d. Historical facade inference is now method-aware: ExecuteAsync declaring types include the full group path, while named methods include only the parent path, so both strip the correct suffix and recover one root identifier. Added a removed group nested parent plus group nested child regression. This commit is layered atop ef2e2584fb, retaining the concurrent raw SubDomainGroup key fix. Validation on the combined tree: focused 1/1; full OptionsGenerator suite 1018/1018 with coverage; Release build 0 warnings/errors; scoped format and diff checks clean. @claude Please review exact head. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3da0fc22e8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d9a19888c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Resolved the CodeFactor complexity finding at exact head 5996dcf. Per-facade identifier recovery is now isolated from cross-facade conflict aggregation with behavior preserved. Validation: GeneratorHardeningTests 109/109; Release build 0 warnings/errors; scoped format and diff checks clean. The full coverage run hit the mandatory 2 GB agent guard at 2,150 MB, so CI owns that full check. @claude Please review exact head. @codex review

[thomhurst]

Addressed exact-head Codex P2 in 78d3143: compatible facade prefix dictionaries now merge; only same-index identifier conflicts fail closed. Added regression for named facade + parent ExecuteAsync + child facade with historical intermediate casing. Validation: GeneratorHardeningTests 110/110, Release build 0 warnings/errors, scoped format and diff checks clean. @claude Please review exact head. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 78d3143dc9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #4045 (re-review at 78d3143d)

This covers the three commits since the last review round (3e8edcbe3b, 5996dcffa8, 78d3143dc9), which add CommandPartIdentifierOverrides (recovering historical casing for intermediate command-part segments, not just the root) and a new conflict guard in CommandTreeNode.InsertCommand.

The new CommandTreeNode conflict check can throw on a legitimate, common merge case

CommandTreeNode.InsertCommand (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CommandTreeNode.cs:96-110) now throws InvalidOperationException whenever two commands sharing the same intermediate segment resolve to different PascalSegment values:

else if (!child.PascalSegment.Equals(pascalSegment, StringComparison.Ordinal))
{
    throw new InvalidOperationException(
        $"Command part '{segment}' has conflicting generated identifiers: "
        + $"{child.PascalSegment}, {pascalSegment}.");
}

pascalSegment comes from command.CommandPartIdentifierOverrides when present, otherwise the default ToPascalCase(segment). The problem: only restored commands populate CommandPartIdentifierOverrides (set in RestoreRemovedCommand → GetRestoredCommandPartIdentifierOverrides, GeneratedApiCompatibilityPreserver.cs:141-145). Live/current commands always fall through to plain ToPascalCase(segment) — there's no equivalent of GetRestoredSubDomainGroup's "prefer the live sibling's casing" reconciliation for non-root segments.

SubDomainClassGenerator.cs:56 calls CommandTreeNode.BuildTree with all commands sharing a SubDomainGroup (live and restored, mixed indiscriminately), and InsertCommand starts at partIndex = 1 — i.e. it hits this check on the very first segment after the root. So the trigger condition is: a 3+ part command group (root + at least two more segments — exactly the terraform stacks configuration * / stacks deployment-group * shape this PR's own description is about) where one sub-command under a shared intermediate segment is still live (→ default ToPascalCase) and a sibling under that same segment was removed and is being restored with a historical/actual casing that differs from the default (e.g. the new test's own "cloud-shell" → "Cloudshell" recovery, vs. a live sibling that would default to "CloudShell").

That's not a hypothetical: it's the exact "partially removed group" scenario GetRestoredSubDomainGroup was written to handle at the root level. The gap is that reconciliation never got extended to intermediate segments when CommandPartIdentifierOverrides was introduced, and the new fail-fast throw makes that gap a hard generator crash instead of a silent wrong-but-plausible identifier. There's no test covering this interaction — a repo-wide search turns up zero references to CommandTreeNode or the "conflicting generated identifiers" message in the test project, so this path (throw or otherwise) isn't exercised at all, including for the case where the throw is correct.

Suggested approach: before assigning a part-level override for a restored command, check whether any live current command shares that root + segment prefix and already has a resolved casing for that position (mirroring GetRestoredSubDomainGroup's pattern), and prefer that. That keeps the fail-fast guard for genuine ambiguity (e.g. two restored siblings disagreeing) while not crashing on the live/restored split this PR exists to handle.

Related architecture note (building on prior rounds' feedback)

There are now three different places resolving "do these identifiers for the same command part agree," each with a different failure philosophy:

• GetRestoredCommandPartIdentifierOverrides (GeneratedApiCompatibilityPreserver.cs:255-280): facades disagree → silently drop to no override (default casing for everyone).
• CommandTreeNode.InsertCommand: nodes disagree → hard throw.
• GetRestoredCommandGroupIdentifier's root-level tiers: ambiguous → falls through to the next weaker heuristic.

This is the same duplication the 08:08 review round flagged (three-plus independent strategies for "the identifier for this position," each keyed on different data). Given this round adds a fourth mechanism and a new hard-failure mode to the mix, it reinforces that suggestion: consolidating into one identifier-resolution helper — reused for root and intermediate parts alike, with one consistent conflict policy — would prevent exactly this kind of gap (a policy added in one tier, like fail-fast, not applying consistently to the others).

Nice regression coverage on ApiCompatibilityPreserver_Merges_Compatible_Facade_Casing and ApiCompatibilityPreserver_Infers_Root_Through_Historical_Intermediate_Casing for the cases they do cover — the gap above is specifically the live+restored mixed-casing case at a non-root segment, which isn't yet represented in either new test.

---

Reviewed with the code-review skill; traced CommandTreeNode.BuildTree's caller in SubDomainClassGenerator.cs to confirm live and restored commands are merged into the same tree before the new conflict check runs, and grepped the test project to confirm the new throw path is untested.

[thomhurst]

Addressed Claude's exact-head mixed live/restored finding in 45a2707. Command trees now resolve each shared raw prefix once: a single explicit historical identifier applies to live and restored siblings; genuinely conflicting explicit identifiers still fail fast. Added regressions for both paths. Validation: GeneratorHardeningTests 112/112, Release build 0 warnings/errors, scoped format and diff checks clean. @claude Please review exact head. @codex review

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45a270727d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #4045 (re-review at 45a27072)

This round covers the single new commit since the last review (78d3143d): 45a27072 "reconcile mixed facade casing", which touches CommandTreeNode.cs and adds two tests to GeneratorHardeningTests.cs.

The gap flagged in the previous round is resolved. The 09:38 review noted that CommandTreeNode.InsertCommand hard-crashed when a live sibling (default ToPascalCase(segment)) and a restored sibling (explicit CommandPartIdentifierOverrides) shared an intermediate segment with differing casing — a scenario not covered by any existing test.

The rewrite (InsertCommand → grouped PopulateNode) fixes this at the root cause rather than special-casing it: children are now grouped once per segment, and only explicit overrides among a group's commands are considered when deriving/validating the identifier (identifierOverrides = ... .OfType<string>().Distinct(...)). A live command with no override no longer participates in the conflict check — the group's single explicit override (if any) wins, and the fail-fast InvalidOperationException is now reserved for genuinely conflicting explicit overrides.

Both new tests target exactly this:

• ApiCompatibilityPreserver_Applies_Restored_Casing_To_Live_Siblings — a live command sharing a segment with a restored/override'd sibling now merges into one class instead of throwing.
• CommandTreeNode_Rejects_Conflicting_Explicit_Identifiers — two explicit overrides that still disagree correctly continue to throw.

This is also a nice simplification vs. the incremental casing patches from the last several commits (74b6dc47e…5996dcffa) — one grouping pass replaces several rounds of ad hoc reconciliation.

Still open (not new, carried from the 09:38 round): the broader observation that identifier-agreement is resolved in three separate places (GetRestoredCommandPartIdentifierOverrides, CommandTreeNode, GetRestoredCommandGroupIdentifier's root tiers) with different conflict policies wasn't addressed in this commit — GeneratedApiCompatibilityPreserver.cs is untouched here. Not blocking given the concrete crash is fixed, but consolidating into a single identifier-resolution helper would still reduce the risk of the next mixed-casing scenario needing its own bespoke fix.

No new bugs found in this delta.

---

Reviewed with the code-review skill; commit 45a27072 is the only commit on the PR since the last review at 78d3143d.

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45a270727d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed both late exact-head Codex P2 findings in 86d1969: restored parent casing now flows into colliding live child nodes, and length-changing nested identifiers are recovered without fixed-length assumptions. Ambiguous recovery still fails closed. Validation: GeneratorHardeningTests 114/114, Release build 0 warnings/errors, scoped format and diff checks clean. @claude Please review exact head. @codex review

[thomhurst]

Follow-up simplification at b9d6418: root and nested facade recovery now share the same ambiguity-safe identifier partitioner, removing the remaining fixed-suffix root assumption. GeneratorHardeningTests still pass 114/114; Release build 0 warnings/errors; format/diff clean. @claude Please review exact head. @codex review

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #4045 (re-review at b9d64183)

This round covers the two commits since the last review (45a27072): 86d19696 "recover nested facade identifiers" and b9d64183 "reuse identifier recovery" (author explicitly requested re-review on both). Together they replace the fixed-length suffix matching in GetRootIdentifierFromFacade / GetRecoveredCommandPartIdentifierOverrides with a shared SplitRecoveredIdentifiers helper, plus a small but important fix in CommandTreeNode.PopulateNode.

SplitRecoveredIdentifiers (GeneratedApiCompatibilityPreserver.cs:340-380) — a DP that partitions a concatenated PascalCase suffix into N substrings, minimizing the count that don't case-insensitively match the expected default identifier, and returns null (fail closed) if the minimum isn't unique. This correctly handles the length-changing rename case that broke the old fixed-suffix-length assumption (e.g. appset → ApplicationSet), and it's now genuinely shared between the root-identifier recovery and the per-part override recovery — a real fix for the "same identifier-recovery logic implemented per call site" gap flagged in the previous round. Traced through the new ApiCompatibilityPreserver_Recovers_Length_Changing_Nested_Identifier test by hand and the DP does land on the unique minimum-cost split it expects.

Still open, not new: GetRestoredCommandGroupIdentifier's own fallback tier (the commandSuffix/EndsWith branch used when neither facadeIdentifiers nor currentIdentifiers resolve to a single candidate) was not migrated to SplitRecoveredIdentifiers and still does the old fixed-length-suffix Substring — the exact pattern this PR just proved is unsafe for renamed/length-changing identifiers. It's a narrower fallback path (only reached when the facade-based and live-sibling-based lookups are both ambiguous), so not blocking, but it's the same bug class reintroduced one layer up in the same function, and now inconsistent with the sibling code that was just hardened.

CommandTreeNode.cs:88-93 — widening identifierOverrides from childCommands (only commands continuing past partIndex) to commands filtered by CommandParts[partIndex] == segment (all commands at this level, including leaves terminating exactly at this segment) is correct: a leaf command ending at this segment can carry a restored CommandPartIdentifierOverrides[partIndex] that previously wasn't considered when deriving the shared node's casing. Backed by the new ApiCompatibilityPreserver_Applies_Restored_Parent_Casing_To_Live_Child test.

Broader observation (not blocking, carried forward and reinforced by this delta): this PR is the latest in a chain of 15 commits, each patching a new edge case in reverse-engineering CommandPartIdentifierOverrides/casing from concatenated generated class names (ToolFooBarOptions → infer Foo/Bar boundaries) because the generator has no persisted record of the identifier decisions it made last time — only the emitted source to parse back. SplitRecoveredIdentifiers is a genuine improvement over the previous ad hoc casing patches, but it's still fundamentally guessing string boundaries from a concatenated name, which is why a length-changing rename needed its own algorithm change today and a future one (e.g. an identifier substring that happens to collide with an adjacent default identifier) could still produce an ambiguous null and silently fall back to the default name. A more durable fix than continuing to harden the parser would be to persist the actual identifier decisions at generation time — e.g. a small per-tool JSON manifest ({ClassName: {CommandPartIdentifierOverrides, CommandGroupIdentifierOverride, ...}}) written alongside the generated sources and read back directly on the next run — turning "restoration" into a lookup instead of a fuzzy-matching problem, and eliminating this entire class of edge case going forward.

No new bugs found in this delta; the two changes are correct fixes for real gaps, backed by targeted regression tests.

---

Reviewed with the code-review skill; commits 86d19696 and b9d64183 are the delta since the last review at 45a27072.

[thomhurst]

Closed Claude's carried fallback inconsistency at 0162e92: baseline class-name fallback now uses the same ambiguity-safe identifier partitioner as facade root/nested recovery. Validation unchanged: GeneratorHardeningTests 114/114, Release build 0 warnings/errors, format/diff clean. @claude Please review exact head. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 0162e92711

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".