Refactor processCommand method to reduce complexity by david-waltermire · Pull Request #563 · metaschema-framework/metaschema-java

david-waltermire · 2025-12-21T15:32:43Z

Summary

Extract CallingContext from inner class to top-level public class for better testability
Add phase methods with Optional<ExitStatus> return pattern for clean flow control:
- checkHelpAndVersion() - early exit for --help/--version
- parseOptions() - parse all command line options
- validateExtraArguments() - validate extra arguments
- validateCalledCommands() - validate command chain
- applyGlobalOptions() - apply --quiet/--no-color
Simplify processCommand() using Optional-based result chaining
Remove PMD GodClass suppression - complexity reduced through extraction
Update CallingContext imports across cli-processor and metaschema-cli modules

Test plan

All 56 cli-processor tests pass
Integration tests verify CLIProcessor public API behavior
Unit tests verify individual phase method behavior
TDD approach: tests written before implementation
Full build with -DskipTests succeeds

Closes #252

Summary by CodeRabbit

Refactor
- CLI processing reorganized into phased flow so help/version are handled earlier, validation is clearer, and global options (e.g., --no-color, --quiet) apply consistently.
Bug Fixes
- More consistent early-exit behavior and clearer error/help messaging for invalid commands, options, and argument validation.
Tests
- Added unit and integration tests covering phase flows, option parsing, help/version output, and argument/option validation.
Documentation
- Added design and implementation plan describing the refactor and rollout.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-21T15:32:54Z

📝 Walkthrough

Walkthrough

Extracts the nested CallingContext into a top-level class and refactors CLIProcessor.processCommand into six sequential phases (help/version, parse options, validate extra args, validate called commands, apply globals, invoke). Adds CallingContext implementation, updates imports/usages across commands/tests, and introduces design/implementation docs plus unit and integration tests.

Changes

Cohort / File(s)	Summary
Design & Plan `PRDs/20251217-cli-processor-refactor/design.md`, `PRDs/20251217-cli-processor-refactor/implementation-plan.md`	Adds design and implementation-plan documents describing the six-phase refactor, migration steps, testing strategy, coordination notes (shell completion), and a proposed multi-commit plan.
New CallingContext `cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java`	Adds a top-level CallingContext that builds CLI state from args and implements phased processing (checkHelpAndVersion, parseOptions, validateExtraArguments, validateCalledCommands, applyGlobalOptions, invokeCommand), help generation, error handling, and mapping command execution to ExitStatus.
CLIProcessor updates `cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java`	Removes nested CallingContext, constructs `new CallingContext(this, args)`, relaxes `OPTIONS` visibility to package-private, adds `getOutputStream()`, and relaxes visibility for `handleNoColor()`/`handleQuiet()`.
Command & executor import updates `cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/`, `metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/*`	Replaces imports of `CLIProcessor.CallingContext` with top-level `gov.nist.secauto.metaschema.cli.processor.CallingContext`; one method signature updated where a parameter referenced the nested type.
New/updated test fixtures & helpers `cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestCommand.java`, `.../TestVersionInfo.java`, `TestCommandWithRequiredArg.java`, `TestCommandWithRequiredOption.java`	Adds test command implementations and a test version provider to exercise phase logic and validation in unit and integration tests.
New tests — suites `cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java`, `.../CallingContextTest.java`	Adds integration tests for CLIProcessor.process and unit tests for CallingContext phases (help/version, parsing, validation, globals, invocation).
Test import update `cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java`	Adjusts CallingContext import to the new top-level class.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor User
    participant CLIProcessor
    participant CallingContext
    participant OptionParser
    participant Commands as CommandChain
    participant Executor

    User->>CLIProcessor: process(args)
    CLIProcessor->>CallingContext: new(this, args)
    CLIProcessor->>CallingContext: processCommand()
    CallingContext->>CallingContext: checkHelpAndVersion()
    alt help/version requested
        CallingContext-->>User: print help/version + ExitStatus
    else continue
        CallingContext->>OptionParser: parseOptions(all collected options)
        OptionParser-->>CallingContext: CommandLine
        CallingContext->>CallingContext: validateExtraArguments()
        CallingContext->>Commands: validateCalledCommands()
        CallingContext->>CallingContext: applyGlobalOptions()
        CallingContext->>Commands: prepare executor for target command
        CallingContext->>Executor: invokeCommand()
        Executor-->>CallingContext: ExitStatus
        CallingContext-->>User: return ExitStatus
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Files/areas to focus review on:
- CallingContext.java — state construction from args, phased orchestration, option aggregation, help formatting, and error → ExitStatus mappings.
- CLIProcessor.java — changes to OPTIONS visibility, construction of CallingContext, and modified helper visibilities.
- Cross-cutting import/signature changes where callers now reference top-level CallingContext (verify public API compatibility, e.g., ValidateModuleCommand.newExecutor).
- New tests (CLIProcessorTest, CallingContextTest) — ensure assertions match intended behaviors and cover edge cases (help/version short-circuiting, validation errors).

Possibly related PRs

Add shell completion command to cli-processor #551 — Shell completion work referenced in the design; overlaps on CallingContext relocation and mentions ExtraArgument.getType().
Feature/cleanup 20250331 #397 — Related extraction/migration of CallingContext from CLIProcessor to a top-level class and associated import/signature updates.
Support base64 encoding/decoding in Metapath #340 — Prior refactor activity touching CallingContext/import updates; may contain overlapping considerations.

Poem

🐇 I hopped out from a nested den to clearer ground,
Six tidy hops make the CLI flow sound.
Help first, parse, check args, validate calls, globals then run —
Tests twitch their noses when the phases are done.
🌿 A neater trail, a brighter run — refactor fun.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.95% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: refactoring processCommand to reduce complexity through method extraction and simplification.
Linked Issues check	✅ Passed	The PR fully addresses issue #252: extracts CallingContext to a top-level class, refactors processCommand into phase methods, removes PMD suppression, and includes comprehensive tests.
Out of Scope Changes check	✅ Passed	All changes are scoped to the refactoring objective: CallingContext extraction, phase method implementation, import updates, and test additions. No unrelated modifications present.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99c6914 and 3a1d0f6.

📒 Files selected for processing (1)

PRDs/20251217-cli-processor-refactor/implementation-plan.md (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-12-13T21:16:12.281Z

Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T21:16:12.281Z
Learning: Applies to website/content/specification/**/*.md : Use RFC 2119 keywords (MUST, SHOULD, MAY) consistently in uppercase in specification documentation

Applied to files:

PRDs/20251217-cli-processor-refactor/implementation-plan.md

📚 Learning: 2025-12-17T13:27:43.679Z

Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: For larger initiatives requiring multiple PRs, use structured PRD (Product Requirements Document) approach. PRDs stored in PRDs/<YYYYMMDD>-<name>/ with PRD.md and implementation-plan.md. Target ≤50 files per PR, maximum 100 files per PR.

Applied to files:

PRDs/20251217-cli-processor-refactor/implementation-plan.md

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Website
GitHub Check: Code

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

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (6)

PRDs/20251217-cli-processor-refactor/design.md (2)
33-43: Consider adding language specifier to fenced code blocks.

The file structure diagram uses a plain fenced code block. For consistency with markdownlint rules (MD040), consider adding a language identifier like text or plaintext.
🔎 Proposed fix
-```
+```text
 cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/
47-55: Consider adding language specifier for phase flow diagram.

Same markdownlint suggestion applies here for consistency.
🔎 Proposed fix
-```
+```text
 processCommand()
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1)

391-395: TODO comment for help header.

The buildHelpHeader() method currently returns null with a TODO. This appears to be a deliberate stub.

Would you like me to help implement a suitable help header, or should this remain as-is for now?
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java (1)
22-41: Well-structured integration test suite.

The test class is well-organized with clear Javadoc, nested test groups, and proper setup. The output capture mechanism using ByteArrayOutputStream allows effective validation of CLI output.
Optional: Consider adding null-safety annotations

For consistency with the project's use of SpotBugs annotations, consider adding @NonNull to the fields:
-  private CLIProcessor processor;
-  private ByteArrayOutputStream outputCapture;
+  @NonNull
+  private CLIProcessor processor;
+  @NonNull
+  private ByteArrayOutputStream outputCapture;
This is optional for test code but improves consistency across the codebase.
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CallingContextTest.java (2)
28-47: Good unit test structure for phase method validation.

The test setup is appropriate for unit testing the individual CallingContext phase methods. The createContext helper method provides a clean way to instantiate test contexts.
Optional: Add null-safety annotations to helper method

For consistency with the project's null-safety standards:
+  @NonNull
-  private CallingContext createContext(String... args) {
+  private CallingContext createContext(@NonNull String... args) {
     return new CallingContext(processor, Arrays.asList(args));
   }
This is optional for test code but improves consistency.
49-171: Comprehensive validation of phase method behavior.

The unit tests effectively validate the Optional-based phase method pattern introduced in the refactor. Each phase method (checkHelpAndVersion, parseOptions, validateExtraArguments, validateCalledCommands, applyGlobalOptions) is tested with key scenarios:

Success paths returning empty Optional

Early exit paths returning ExitStatus

Exception handling for invalid input

The tests confirm the refactored design works correctly and support the TDD approach mentioned in the PR objectives.

Optional: Consider expanding edge case coverage

While current coverage validates the refactor, you could optionally add tests for:

Multiple invalid options in ParseOptionsTests

Various argument validation scenarios in ValidateExtraArgumentsTests

Multiple command validation cases in ValidateCalledCommandsTests

Additional global options in ApplyGlobalOptionsTests

This would provide more comprehensive edge case coverage, but the current tests are sufficient for validating the refactored phase methods.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ddc25f6 and bbf4c80.

📒 Files selected for processing (14)

PRDs/20251217-cli-processor-refactor/design.md (1 hunks)
PRDs/20251217-cli-processor-refactor/implementation-plan.md (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java (3 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java (1 hunks)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ShellCompletionCommand.java (1 hunks)
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java (1 hunks)
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CallingContextTest.java (1 hunks)
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestCommand.java (1 hunks)
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestVersionInfo.java (1 hunks)
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.java

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.java: All code changes must follow the Javadoc style guide (docs/javadoc-style-guide.md). New code requires 100% Javadoc coverage on public/protected members. Modified code must add/update Javadoc on any members touched. All Javadoc must include @param, @return, @throws tags in the correct order (BLOCKING)
Java target version must be Java 11. Use SpotBugs annotations (@nonnull, @nullable) for null safety in code.
Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages
Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green

Files:

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ShellCompletionCommand.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestVersionInfo.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestCommand.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CallingContextTest.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java

**/*Test.java

📄 CodeRabbit inference engine (CLAUDE.md)

@test methods do not require Javadoc if they use descriptive method names. Use JUnit 5 for testing with parallel execution enabled.

Files:

cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CallingContextTest.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java

🧠 Learnings (2)

📚 Learning: 2025-12-19T04:01:37.408Z

Learnt from: david-waltermire
Repo: metaschema-framework/metaschema-java PR: 550
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java:56-100
Timestamp: 2025-12-19T04:01:37.408Z
Learning: When overriding Java interface methods, rely on inherited Javadoc from the interface. Do not duplicate documentation in the implementing class unless there is implementation-specific behavior that warrants additional notes beyond the interface contract.

Applied to files:

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ShellCompletionCommand.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestVersionInfo.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestCommand.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CallingContextTest.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessorTest.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java

📚 Learning: 2024-11-15T20:00:59.203Z

Learnt from: david-waltermire
Repo: metaschema-framework/metaschema-java PR: 245
File: cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java:15-15
Timestamp: 2024-11-15T20:00:59.203Z
Learning: The `DefaultExtraArgument` class in `cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java` is currently a stub for future exploration.

Applied to files:

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/TestCommand.java
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java

🧬 Code graph analysis (5)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ShellCompletionCommand.java (1)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1)

CallingContext (45-570)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java (1)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1)

CallingContext (45-570)

cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionScriptGeneratorTest.java (1)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1)

CallingContext (45-570)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java (1)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java (1)

CallingContext (45-570)

cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java (1)

core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)

ObjectUtils (18-135)

🪛 markdownlint-cli2 (0.18.1)

PRDs/20251217-cli-processor-refactor/design.md

33-33: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

47-47: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

PRDs/20251217-cli-processor-refactor/implementation-plan.md

19-19: Emphasis used instead of a heading