feat!: rename Java packages from gov.nist.secauto.metaschema to dev.metaschema #618
Conversation
📝 WalkthroughWalkthroughGlobal rename of Java packages/modules from gov.nist.secauto.metaschema.* → dev.metaschema.*, plus related refactors and small feature additions: CallingContext extraction, type-aware ExtraArgument for shell completion, path formatters & CLI wiring, ValidationContext/PathTracker, ParallelValidationConfig, schemagen DOM writers, and regenerated databind/testing artifacts. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"noteBkg":"#f8f9fa","noteBorder":"#c6cbd1","actorBkg":"#f0f7ff"}}}%%
sequenceDiagram
autonumber
actor User
participant CLI as metaschema-cli
participant Proc as CLIProcessor/CallingContext
participant Validator as DefaultConstraintValidator
participant Config as ParallelValidationConfig
participant Exec as ExecutorService
participant Handler as IConstraintValidationHandler
Note over CLI,Validator: Parallel validation flow (new config + executor)
User->>CLI: validate --threads N <input>
CLI->>Proc: parse options, build CallingContext
Proc->>Validator: instantiate with Handler and Config(withThreads N)
Validator->>Config: isParallel? / getExecutor()
alt parallel enabled
Validator->>Exec: submit validation tasks
par worker threads
Exec->>Validator: run validation task
Validator->>Handler: report findings (thread-safe)
end
else sequential
Validator->>Handler: validate sequentially
end
Validator-->>CLI: aggregated findings / exit code
CLI-->>User: exit
%%{init: {"themeVariables":{"noteBkg":"#fff8f0","noteBorder":"#ffd6a5"}}}%%
sequenceDiagram
autonumber
participant Reader as MetaschemaJsonReader/MetaschemaXmlReader
participant PT as PathTracker
participant VC as ValidationContext
participant PH as IProblemHandler
Note over Reader,PH: Context-rich error reporting with ValidationContext
Reader->>PT: update current path during parse
Reader->>VC: create ValidationContext(resource, location, PT.path, format)
Reader->>PH: handleMissingInstances(parent,target,unhandled, VC)
PH-->>Reader: produce/record contextual diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/src/main/java/dev/metaschema/core/metapath/format/IPathSegment.java (1)
67-67: Fix incomplete Javadoc tag.The Javadoc contains an incomplete
{@code}tag. It should be{@code null}to properly document that null may be returned.🔎 Proposed fix
* Get the value associated with the path segment. * - * @return the value or {@code} if no value is associated with this path segment + * @return the value or {@code null} if no value is associated with this path segment */PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (1)
13-33: TDD requirement is well-structured but lacks explicit phase sequencing constraints.The mandatory TDD section clearly distinguishes new components (write tests first) from replacements (characterization tests first). However, the plan doesn't explicitly state whether phases can be parallelized or if Phase 1 must complete before Phase 2 begins. Given that Phase 3 depends on both Phases 1 & 2, clarify:
- Can test files for Phase 1 & 2 coexist during development?
- When are JDOM2 dependencies removed from pom.xml—before or after Phase 2 tests pass?
This matters because premature dependency removal could cause Phase 1 tests to fail to compile.
core/src/main/java/dev/metaschema/core/metapath/format/PathFormatSelection.java (1)
15-15: Fix typo in @see tag.The @see tag references
METAPATH_PATH_FORMATERbut should beMETAPATH_PATH_FORMATTER(note the double 'T' in FORMATTER) to match the naming convention used in lines 16-17 and to correctly reference the constant inIPathFormatter.🔎 Proposed fix
- * @see IPathFormatter#METAPATH_PATH_FORMATER + * @see IPathFormatter#METAPATH_PATH_FORMATTER
🧹 Nitpick comments (12)
PRDs/20251224-codegen-quality/implementation-plan.md (1)
43-46: Minor: Inconsistent path format in documentation.The document uses full paths in lines 43–46 (e.g.,
databind/src/main/java/dev/metaschema/databind/codegen/typeinfo/INamedInstanceTypeInfo.java) but placeholder paths in lines 172–179 (e.g.,databind/src/main/java/.../config/binding/MetaschemaBindings.java). For clarity and consistency, standardize on one format throughout the document.🔎 Suggestion: Use consistent path format
For readability, prefer either:
- Option A (full paths):
databind/src/main/java/dev/metaschema/databind/codegen/config/binding/MetaschemaBindings.java- Option B (abbreviated):
databind/src/main/java/.../codegen/config/binding/MetaschemaBindings.javaApply chosen format uniformly across all file path references.
Also applies to: 115-119, 172-179
PRDs/20251208-module-builder/implementation-plan.md (1)
1-10: Clarify prerequisite dependency on PR #618.The implementation plan uses the new
dev/metaschemapackage paths throughout, which assumes PR #618 (package rename) is merged. Consider adding a brief header note or dependency statement (e.g., "Prerequisite: PR #618 package rename must be merged") to make this clear to future readers and link this plan to the broader context.core/src/main/java/dev/metaschema/core/metapath/cst/NamedFunctionReference.java (1)
19-23: Pre-existing documentation inconsistency: "variable reference" should be "named function reference".The class Javadoc says "variable reference" but links to
#id-named-function-refand this class isNamedFunctionReference. The same inconsistency exists in thegetName()method description (line 49). This is a pre-existing issue not introduced by this PR, but worth fixing in a follow-up.🔎 Suggested documentation fix
/** * The CST node for a Metapath - * <a href="https://www.w3.org/TR/xpath-31/#id-named-function-ref">variable - * reference</a>. + * <a href="https://www.w3.org/TR/xpath-31/#id-named-function-ref">named + * function reference</a>. */And for the
getName()method:/** - * Get the variable name. + * Get the function name. * * @return the name of the referenced function */PRDs/20251228-validation-errors/implementation-plan.md (4)
15-35: Clarify theFormattype source and definition.Line 24 references a
Formatfield inValidationContext, but the type and its origin are not specified in this plan. Before implementation, confirm whereFormatis defined (existing enum, new type, etc.) and whether it's already available in the codebase or needs to be created as part of this work.If
Formatdoes not exist, would you like me to help draft its definition?
37-57: EnsurePathTrackerJavadoc explicitly documents non-thread-safety.The acceptance criteria correctly acknowledge thread-safety considerations (line 56). When implementing, add explicit Javadoc to
PathTrackerstating it is not thread-safe and requires external synchronization if shared across threads, or clarify that each thread should maintain its own instance during parsing.
239-250: Specify the synthetic URI format for null input fallback.Task 3.6 mentions using a "synthetic URI" when
nullis passed, but does not define its format. Before implementation, clarify what this URI should be (e.g.,unknown://unknown, a fixed constant, a UUID-based URI). This is important for consistency in error messages tested in Task 2.3 (testErrorWithoutSourceUri).Would you like me to propose a convention (e.g.,
URI.create("unknown://unknown")) or is there an existing pattern in the codebase for this?
180-196: Clarify the property type grouping strategy in error messages.Line 187 mentions "Group missing properties by type" and line 195 specifies distinguishing attributes vs. elements for XML. Before implementation, detail the exact grouping criteria: are you grouping by structural role (flags, fields, assemblies) or by serialization type (attributes, elements, properties)? Provide a brief example of the expected error message format for multiple missing properties of different types.
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
63-66: Fix typo in toString() output (pre-existing).Line 65 contains a typo:
"(abiguous)"should be"(ambiguous)". This affects the string representation used in debugging and logging.🔎 Proposed fix
@Override public String toString() { - return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)"); }PRDs/20251215-shell-completion/implementation-plan.md (1)
490-490: Clarify enum completion type registration timing and location.Phase 5 proposes adding
.type(Format.class)to option definitions, and the acceptance criteria (lines 559-560, 573) mention registering enum types withCompletionTypeRegistry.registerEnum(). However, the plan doesn't specify:
- When these registrations occur (application startup, static initializer, lazy on first use?)
- Where they're called (main method, command initialization, registry static block?)
- Who registers them (framework vs. user code for custom enums?)
For example, is there a designated place in
MetaschemaCommandsor a startup hook whereCompletionTypeRegistry.registerEnum(Format.class)would be called before shell completion is invoked?Would you like me to propose a registration mechanism (e.g., static initializer in a registry loader class, or a command initialization hook)?
Also applies to: 559-560, 573-573
core/src/main/java/dev/metaschema/core/mdm/IDMAssemblyNodeItem.java (1)
28-28: Package reference updated correctly; pre-existing documentation issue noted.The FQN update to
dev.metaschema.core.model.IFlagInstanceis correct. However, this line references anewFlagmethod that does not exist in this interface—onlynewFieldandnewAssemblyare defined. This appears to be a pre-existing documentation error.🔎 Suggested documentation fix
- * {@link #newFlag(dev.metaschema.core.model.IFlagInstance, IAnyAtomicItem)}, + * {@link IDMModelNodeItem#newFlag(dev.metaschema.core.model.IFlagInstance, IAnyAtomicItem)},Or if
newFlagis inherited from a parent interface, verify the correct reference. If the method doesn't exist anywhere, consider removing this line.PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)
87-101: Acceptance criteria for IndentingXMLStreamWriter are comprehensive but lack edge-case coverage.The criteria correctly emphasize text preservation and mixed content tests (lines 63–68, 70–75), especially for XHTML in
xs:documentation. However, consider adding test cases for:
- Empty CDATA sections and nested CDATA (not just "not indented internally")
- Processor instructions with attributes
- Namespace prefix declarations on nested elements
- Very deeply nested structures (nesting stress test)
These additions would improve confidence that the indenter handles real-world XSD schema documentation safely.
123-150: Characterization test approach for Phase 2 is sound but raises integration risk.The plan (lines 125–149) correctly uses characterization tests to verify behavioral equivalence before deleting JDOM2 classes. However, line 143 suggests parameterizing tests to cover both old and new implementations:
Update tests to use new implementations (or parameterize to test both)
This is vague. If tests are parameterized, the old JDOM2 implementation classes must remain available during Phase 2 verification. Clarify:
- Will JDOM2 classes be deleted only after all parameterized tests pass?
- If test classes are in separate files (XmlSchemaLoaderTest.java, DomDatatypeContentTest.java), how is equivalence verified without side-by-side comparison?
A clearer approach: Use a parameterized test that accepts a supplier/factory for the implementation, allowing Phase 2 to prove equivalence before deletion.
core/src/main/java-templates/dev/metaschema/core/MetaschemaJavaVersion.java
Show resolved
Hide resolved
core/src/main/java-templates/dev/metaschema/core/MetaschemaJavaVersion.java
Show resolved
Hide resolved
david-waltermire
force-pushed
the
feature/616-package-rename
branch
from
d8b00c7 to
db1b0d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
64-66: Fix typo in string literal.The string literal on line 65 contains a typo:
"(abiguous)"should be"(ambiguous)".🔎 Proposed fix
@Override public String toString() { - return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)"); }core/src/main/java/dev/metaschema/core/datatype/adapter/DayTimeAdapter.java (1)
54-54: Fix typo in comment.The comment contains a grammatical error: "value in immutable" should be "value is immutable".
🔎 Proposed fix
- // value in immutable + // value is immutablecore/src/main/java/dev/metaschema/core/mdm/impl/AbstractDMNodeItem.java (1)
14-17: Fix typo in Javadoc.Line 15 contains a typo: "assmebly" should be "assembly".
🔎 Proposed fix
/** - * This abstract Metapath assmebly node item implementation supports creating a + * This abstract Metapath assembly node item implementation supports creating a * Metaschema module-based data model. */
🧹 Nitpick comments (7)
PRDs/20251217-parallel-constraint-validation/implementation-plan.md (2)
140-247: ParallelValidationConfig API design is sound, but clarify the threshold rationale.The class correctly distinguishes between internal (owned) and external (unowned) executors in close(), uses appropriate null-checking, and provides sensible factory methods. The SEQUENTIAL constant as a singleton is a good pattern for avoiding unnecessary object allocation.
However, consider adding a brief Javadoc note explaining why PARALLEL_THRESHOLD = 4 was chosen—whether based on empirical testing, heuristics, or a conservative estimate. If this value should be tunable in the future, consider noting that as a potential enhancement.
642-669: Sorting findings on every call may have performance implications.The
getFindings()method (lines 642–646) sorts the queue every time it is called. For documents with many constraint violations, repeated calls to this method will repeatedly sort the findings list. Consider documenting this behavior or, if performance becomes a concern, caching the sorted list with invalidation on new findings.If sorting performance is not a bottleneck in practice, document the design decision in the Javadoc so future maintainers understand why sorting occurs on every call rather than on-demand or incrementally.
PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)
123-165: Characterization testing approach is sound; consider parameterized test guidance.The two-step approach (characterization tests against JDOM2, then verification against new DOM implementation) follows best practices for behavioral equivalence verification. Line 143 mentions "parameterize to test both," which is wise for confidence; clarifying in the acceptance criteria that the goal is behavioral equivalence (not dual-implementation testing) would help implementers understand the expected test lifecycle.
396-412: XPath namespace handling example is clear; may need multi-namespace guidance during implementation.The example covers the
xsnamespace (XSD) correctly. If XSD schemas in the codebase use other namespaces (XHTML in documentation, etc.), ensure theNamespaceContextimplementation handles multiple prefixes. This can be deferred to implementation but may be worth calling out in phase 2 acceptance criteria if known namespaces are identified.core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)
102-107: Consider removing redundant function equality check.The
equalsmethod checks function equality twice: Line 102 verifies!getFunction().equals(other.getFunction())and returns false if not equal, but then Line 105 checksObjects.equals(function, other.function)again as part of the final return statement. Since the first check already returned false when functions don't match, the second check is redundant.🔎 Proposed fix to remove redundancy
CalledContext other = (CalledContext) obj; - if (!getFunction().equals(other.getFunction())) { - return false; - } - return Objects.equals(function, other.function) + return Objects.equals(function, other.function) && Objects.equals(arguments, other.arguments) && Objects.equals(contextItem, other.contextItem);core/src/main/java/dev/metaschema/core/datatype/markup/flexmark/impl/FixedEmphasisDelimiterProcessor.java (2)
26-26: Track upstream fix for potential cleanup.The TODO indicates this class is a temporary workaround pending the merge of flexmark-java#580.
Would you like me to open an issue to track monitoring the upstream PR and removing this workaround once it's merged?
41-67: Consider adding Javadoc for implementation-specific behavior.The override methods
getDelimiterUseandprocesscontain non-trivial implementation-specific logic (e.g., the "multiple of 3" rule, emphasis wrapping decisions) that differs from standard delimiter processing. While inherited Javadoc covers the interface contract, documenting these implementation details would improve maintainability, especially given this is a temporary workaround for upstream behavior.Example Javadoc enhancement for getDelimiterUse
+ /** + * {@inheritDoc} + * <p> + * This implementation applies the "multiple of 3" rule for internal delimiter runs + * and adjusts delimiter usage based on opener/closer lengths to fix flexmark-java + * issue #580. + */ @Override public int getDelimiterUse(DelimiterRun opener, DelimiterRun closer) {
|
@coderabbitai Yes, please remove the outdated learning (ID c77ba4a9-f817-49f9-b7b0-2a1fb05c02d2) that references the old gov.nist.secauto.metaschema.* package convention. The new canonical convention is dev.metaschema.* as of this PR. Regarding follow-up issues in external repos - no, that's not necessary. The core/metaschema submodule is owned by this project's maintainers and will be updated in coordination with this change. |
|
Tip For best results, initiate chat on the files or code changes. @david-waltermire Thanks — I’ve removed the outdated learning and will continue to use the dev.metaschema.* convention going forward. Noted on the core/metaschema submodule: no external follow-up issues will be opened. ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
david-waltermire
force-pushed
the
feature/616-package-rename
branch
2 times, most recently
from
0db795c to
cc4aac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @.github/workflows/build.yml:
- Line 276: The workflow currently treats lychee exit code 1 as the “broken
links” failure; update the exit-code branching so any checks that currently
compare exit_code == 1 (at the logic handling lines around the TIMEOUTS/TIMEOUT
detection and the subsequent branching referenced) instead use exit_code == 2 to
detect real broken-link failures and trigger issue creation/job failure, and
treat exit_code == 1 as a transient/runtime error (timeout) path (e.g.,
increment TIMEOUTS or retry/skip); update the conditions around the TIMEOUTS
assignment and the two places noted (near the TIMEOUTS line and the two decision
blocks at 300–301 and 310–312) so they correctly differentiate exit_code == 2
(broken links) vs exit_code == 1 (timeouts/transient).
In @core/src/main/java/dev/metaschema/core/datatype/package-info.java:
- Line 52: Update the coding guidelines text that still references the old
package convention "gov.nist.secauto.metaschema.*" to the new canonical
"dev.metaschema.*" convention; specifically replace any guideline wording that
mandates gov.nist.secauto.metaschema.* with dev.metaschema.* so it matches the
updated package declarations (e.g., dev.metaschema.core.datatype) and avoid
leaving the outdated namespace in documentation or package-info comments.
In @core/src/main/java/dev/metaschema/core/metapath/cst/package-info.java:
- Around line 18-27: Update the Javadoc tags for the two subpackage references
that use {@code}: replace {@code dev.metaschema.core.metapath.cst.logic} and
{@code dev.metaschema.core.metapath.cst.items} with {@link
dev.metaschema.core.metapath.cst.logic} and {@link
dev.metaschema.core.metapath.cst.items} so all subpackage references (math,
path, type, logic, items) consistently use {@link} for proper cross-referencing.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
65-65: Fix typo: "abiguous" should be "ambiguous".The suffix string contains a spelling error.
🔎 Proposed fix
- return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)");PRDs/20251217-cli-processor-refactor/implementation-plan.md (1)
738-750: Add null-check foroption.getArgName()result.The codebase confirms that
Option.getArgName()can return null. InCompletionScriptGenerator.java, this is already handled with a defensive check:String argName = opt.getArgName(); if (argName == null) { argName = "arg"; }Apply the same pattern here to avoid a potential NPE when appending the argument name to the builder.
🧹 Nitpick comments (17)
CONTRIBUTING.md (1)
110-110: Update specification link to use canonical documentation URL.The package path update to
dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.javais correct and aligns with the PR's renaming objectives. However, the specification link still useshttps://pages.nist.gov/metaschema/when the project's preferred and canonical documentation URL ishttps://framework.metaschema.dev/.Per prior learnings, pages.nist.gov URLs should not be used in documentation; consider updating the specification link to use the canonical framework.metaschema.dev URL.
🔎 Proposed fix to align with canonical documentation URL
-The Metaschema [specification](https://pages.nist.gov/metaschema/specification/syntax/metapath/) and [this conformant library](https://github.com/metaschema-framework/metaschema-java) library implement the Metapath functions required by the specification. See the library's [registry class with an inventory listing of the functions](https://github.com/metaschema-framework/metaschema-java/blob/main/core/src/main/java/dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java) supported in the most current release. +The Metaschema [specification](https://framework.metaschema.dev/specification/syntax/metapath/) and [this conformant library](https://github.com/metaschema-framework/metaschema-java) library implement the Metapath functions required by the specification. See the library's [registry class with an inventory listing of the functions](https://github.com/metaschema-framework/metaschema-java/blob/main/core/src/main/java/dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java) supported in the most current release.Based on learnings from PR #605, framework.metaschema.dev is the canonical documentation URL for this project.
PRDs/20251215-shell-completion/PRD.md (2)
248-250: Demonstrate the new ExtraArgument type feature in ShellCompletionCommand example.The PRD introduces type-based completion via
ExtraArgument.getType()(lines 215–224), but the example at line 248–250 uses the old 2-argument factory method. To showcase the new capability and align with the design goals, the shell argument itself should specify a completion type:// Shell enum would be defined as shown on lines 243–246 public enum Shell { BASH, ZSH } // Then register it: static { CompletionTypeRegistry.registerEnum(Shell.class); } // And use it in EXTRA_ARGUMENTS: ExtraArgument.newInstance("shell", true, Shell.class)This demonstrates that the feature applies not just to Options but also to ExtraArguments, making the design more cohesive.
184-188: Consider documenting shell built-in function availability.The PRD references shell built-in completion functions (
_filedir,_files,_urls) without noting version or environment requirements. These are typically available in bash-completion package (v2.8+) and zsh completion system, but availability varies by system.Consider adding a compatibility note or fallback behavior (e.g., graceful degradation for older shells). Alternatively, document minimum shell versions or suggest that end users install bash-completion package if these functions are not available.
PRDs/20251228-validation-errors/implementation-plan.md (1)
254-275: Verification phase covers implementation but may miss broader integration testing.The verification plan runs databind tests (
mvn -pl databind teston line 257) and full CI build. However, since validation contexts flow through JSON and XML readers that are used across the codebase, consider adding integration tests in the core or testing modules to ensure ValidationContext propagation works correctly end-to-end. The full build will catch compilation errors but not all functional integration issues.core/src/main/java/dev/metaschema/core/datatype/adapter/TimeWithTZAdapter.java (1)
6-6: Update coding guidelines to reflect new package naming convention.The coding guidelines currently state: "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this PR establishes
dev.metaschema.*as the new canonical convention. The guidelines should be updated to avoid confusion in future reviews.Based on learnings, the canonical package naming convention is
dev.metaschema.*as of PR #618.Would you like me to help draft updated coding guidelines that reflect the new
dev.metaschema.*package naming convention?core/src/main/java/dev/metaschema/core/datatype/markup/package-info.java (1)
50-50: LGTM! Package declaration correctly updated to new namespace.The package declaration has been properly updated from
gov.nist.secauto.metaschema.core.datatype.markuptodev.metaschema.core.datatype.markup, correctly implementing the package rename objective.Note: The coding guidelines currently state "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this guideline is outdated. Based on learnings, the canonical convention is
dev.metaschema.*as of this PR (#618). The coding guidelines should be updated to reflect this new convention.core/src/main/java/dev/metaschema/core/metapath/function/FunctionService.java (1)
35-35: Address the FIXME comment.The FIXME suggests renaming
getInstance()toinstance()for consistency.Would you like me to open a follow-up issue to track this naming improvement, or would you prefer to address it in this PR?
core/src/main/java/dev/metaschema/core/metapath/cst/type/Castable.java (1)
6-6: Update coding guidelines to reflect new package convention.The coding guidelines currently specify:
Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages
This is now outdated as of PR #618, which establishes
dev.metaschema.*as the canonical naming convention. The guidelines document should be updated to reflect this breaking change.Would you like me to generate a script to locate and update the coding guidelines file?
core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)
93-108: Optional: Simplify redundant equality check inequals().The method checks function equality twice:
- Line 102-104: early return if functions aren't equal
- Line 105: re-checks
Objects.equals(function, other.function)Since line 105 is only reached when functions are already known to be equal, the check is redundant. Consider simplifying:
🔎 Proposed simplification
@Override public boolean equals(Object obj) { if (this == obj) { return true; } if (obj == null || getClass() != obj.getClass()) { return false; } CalledContext other = (CalledContext) obj; - if (!getFunction().equals(other.getFunction())) { - return false; - } - return Objects.equals(function, other.function) - && Objects.equals(arguments, other.arguments) - && Objects.equals(contextItem, other.contextItem); + return Objects.equals(function, other.function) + && Objects.equals(arguments, other.arguments) + && Objects.equals(contextItem, other.contextItem); }PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (4)
1-35: Clear TDD mandate, but phase dependencies need explicit documentation.The plan correctly enforces TDD as mandatory (line 32), which aligns with project learnings. However, the phase sequencing (line 9: "tightly coupled, cannot function independently") implies hidden prerequisites—e.g., Phase 3 depends on Phase 1 completion, but this is not explicitly stated in Phase 3's requirements. Consider adding a "Prerequisites" subsection to each phase listing prior phases that must be complete.
54-101: Phase 1 design is sound; clarify "whitespace-only" text detection in implementation.The test scenarios for
IndentingXMLStreamWriterare comprehensive and correctly identify the critical case: preserving text content without spurious indentation. The stack-based state machine (lines 320–363) elegantly handles nested mixed-content. However, the acceptance criteria should clarify what "whitespace-only" means during implementation. For example:
- Is a text node of
"\n "(newline + indent) considered whitespace-only?- What about
"\t"?This distinction affects whether a parent's
hasTextflag is set and thus whether child elements receive indentation. Consider adding a test case for this edge case (e.g., "Test that whitespace-only text nodes do not trigger mixed-content mode") to the Phase 1 acceptance criteria.
104-165: Characterization-test-first strategy is excellent; clarify DocumentBuilderFactory namespace configuration.The Phase 2 approach of writing characterization tests against JDOM2, verifying they pass, then replacing with new implementation is a strong TDD strategy for behavioral equivalence. However, a critical detail is missing: DocumentBuilderFactory must be configured with
setNamespaceAware(true)to correctly handle XPath namespace queries (line 132). Without this, XPath queries like/xs:schema/xs:simpleTypewill fail because the default namespace context is not populated.Recommendation: Add an explicit acceptance criterion:
- "Ensure
DocumentBuilderFactory.setNamespaceAware(true)is set inXmlSchemaLoaderto support namespace-aware XPath queries."Additionally, the test scenario at line 129 should specify what assertions are expected (e.g., "verify element count matches JDOM2 result", "verify element names in order", etc.) to ensure characterization tests capture all relevant behavior.
243-272: Phases 4–6 are clear; minor clarification on Phase 5 test redundancy.Phases 4 (configuration), 5 (test updates), and 6 (verification) are well-structured. However, Phase 5 mentions updating
XmlSuiteTestto replaceStAXEventBuilderwithDocumentBuilder(line 256), but Phase 2 already defines characterization tests that test both JDOM2 and new DOM implementations. Clarify whether:
- Phase 5 updates are separate integration tests, or
- Phase 5 reuses/refactors Phase 2 characterization tests to cover the broader XmlSuiteTest suite.
This avoids duplicate test-writing effort and keeps the plan efficient.
cli-processor/src/main/java/dev/metaschema/cli/processor/CallingContext.java (1)
125-127: Consider adding Javadoc to package-private methods for consistency.While the coding guidelines only mandate Javadoc for public/protected members (which are fully covered), some package-private methods have Javadoc (e.g.,
getOptionsList(),getExtraArgs()) while others don't (getCalledCommands(),toOptions()). Adding brief documentation to these methods would improve consistency and maintainability.Also applies to: 144-150
PRDs/20251217-parallel-constraint-validation/implementation-plan.md (3)
1041-1043: PARALLEL_THRESHOLD constant placement.The
PARALLEL_THRESHOLD = 4constant is defined at line 1012 but the decision to parallelize based on sibling count (line 1042:item.modelItems().count() >= PARALLEL_THRESHOLD) is good. However, confirm:
- This threshold is appropriate for typical document structures
- The threshold makes sense relative to JVM overhead for task submission
Consider making the threshold configurable via
ParallelValidationConfigfor future tuning:private int getParallelThreshold() { return parallelConfig.getParallelThreshold(); }
1315-1330: Integration tests are well-planned but marked as placeholders.Task 7 shows integration test scenarios (lines 1315-1330) but the test bodies are incomplete. Recommend:
- Implementing these as soon as the infrastructure changes are complete
- Using test fixtures from Task 1-6 to create realistic validation scenarios
- Adding performance assertions (e.g., parallel faster than sequential on large documents)
Would you like me to help generate complete integration test implementations that validate end-to-end parallel constraint validation behavior?
807-807: Reconsider the necessity of alternative toCollections.synchronizedListforkeyRefItems.The code at line 709 uses
Collections.synchronizedList(new ArrayList<>())to store key references during parallel validation. While the concern about synchronizedList's locking behavior is generally valid, the actual usage pattern mitigates this concern:
- Write phase:
keyRefItems.add()is called concurrently during parallel validation- Read phase: Sequential iteration occurs in
finalizeValidation()(lines 977-989) only after all parallel validation completes- No concurrent access: Reads and writes never overlap
Since iteration happens exclusively in a sequential finalization phase,
Collections.synchronizedListis acceptable here. However,CopyOnWriteArrayListwould be slightly more appropriate for this write-before-read pattern, as it would eliminate lock contention during the sequential finalization phase without the overhead of synchronization on every operation.The current choice is not suboptimal given the actual access pattern, though a minor optimization is possible.
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In
@core/src/main/java/dev/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java:
- Line 6: The coding guidelines still reference the old package prefix
gov.nist.secauto.metaschema.*, so update the document to use the new
dev.metaschema.* convention everywhere (including example package declarations
and import examples) and ensure all examples match the actual package
declarations such as the package line in AbstractCSTVisitorBase (package
dev.metaschema.core.metapath.cst); search the repo/docs for any remaining
gov.nist.secauto.metaschema occurrences and replace them, and optionally open an
issue to track the guidelines update if you want a changelog entry.
In
@core/src/main/java/dev/metaschema/core/metapath/cst/items/DecimalLiteral.java:
- Line 6: The coding guidelines still reference the old package prefix
"gov.nist.secauto.metaschema.*"; update the guideline text to the new canonical
package naming convention "dev.metaschema.*". Locate references to the old
prefix in the project's coding standards/documentation and replace them with
"dev.metaschema.*" (mentioning the new convention and optionally referencing PR
#618), and ensure examples and any associated package declarations (e.g.,
classes like DecimalLiteral with package declarations) reflect the updated
prefix.
In
@core/src/main/java/dev/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java:
- Line 6: The coding guidelines still reference the old package prefix
gov.nist.secauto.metaschema.*; update all documentation and guideline files to
use the new canonical package convention dev.metaschema.* (search for
occurrences of "gov.nist.secauto.metaschema" and replace with "dev.metaschema"),
and add a brief note in the guidelines (referencing examples like
AbstractRootPathExpression.java and other core classes under dev.metaschema.*)
explaining the change; if you prefer, open a follow-up issue to track any
remaining docs to review.
In @core/src/main/java/dev/metaschema/core/metapath/format/IPathSegment.java:
- Line 67: The Javadoc says getNodeItem() on IPathSegment can return null but
the signature lacks a SpotBugs nullable annotation; add the SpotBugs @Nullable
annotation (edu.umd.cs.findbugs.annotations.Nullable) to the getNodeItem()
return type in interface IPathSegment and add the corresponding import so static
analysis recognizes the null contract; repeat the same change for the other
method noted in the comment (the second nullable-returning method at the
referenced location).
In @PRDs/20251208-module-builder/implementation-plan.md:
- Line 10: PRD conflict: two PRDs target IModuleMockFactory at different package
paths; choose a single canonical location (prefer
dev.metaschema/core/testsupport/builder/IModuleMockFactory.java to match
constraint-builder), update both PRDs to reference that path, ensure the
interface IModuleMockFactory declares both methods constraintSet() and module(),
and coordinate implementation by having one PR add both method signatures (or
have the first PR add one signature and the second immediately add the other in
the same branch) to avoid merge conflicts—reference the interface name
IModuleMockFactory and the method names constraintSet() and module() when
updating docs and code.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
PRDs/20251215-shell-completion/PRD.md (4)
95-98: Provide implementations or detailed specs forFileCompletionTypeandUriCompletionType.The PRD references
new FileCompletionType()andnew UriCompletionType()in the registry initialization but does not specify their implementation. While theICompletionTypeinterface is clearly defined, providing concrete implementations (or at minimum, detailed behavior specs) for these built-in types would strengthen the specification.Example: Show how
FileCompletionType.getBashCompletion()returns_filedirandgetZshCompletion()returns_files(as indicated in the table at lines 183–187).
45-51: Add JPMS module export requirements for new public APIs.The PRD introduces new public interfaces and classes in
dev.metaschema.cli.processor.completionand modifications todev.metaschema.cli.processor.command. Since metaschema-java uses JPMS (Java Platform Module System), thecli-processormodule'smodule-info.javamust explicitly export these packages for consuming modules (e.g.,metaschema-cli) to access them.Add a section specifying:
- Export of
dev.metaschema.cli.processor.completionpackage (containsICompletionType,CompletionTypeRegistry,CompletionScriptGenerator)- Confirmation that existing
dev.metaschema.cli.processor.commandexport includes the extendedExtraArgumentinterfaceAlso applies to: 612-619
237-269: ExpandShellCompletionCommandspecification with execution logic and integration details.The outline is skeletal and omits critical implementation details:
- Constructor/initialization: How does the command access the
CLIProcessoror command registry? Is it passed as constructor parameter or retrieved via static method?- Shell argument parsing: How is the
shellextra argument (line 248) parsed and validated? What happens if an invalid shell type (e.g.,tcsh) is passed?- Execution flow: Show the
execute()orhandle()method signature and logic flow (parse args → get commands → generate script → write output)- Integration point: How does
metaschema-cliregister and initialize this command? Does it require changes toMetaschemaCommandsinitialization?These details are essential for developers to implement the command correctly.
590-606: Expand testing strategy to include file I/O and edge cases.The testing strategy (lines 590–606) covers core generation logic but omits several important scenarios:
Missing unit/integration tests:
--tooption: writing script to file, handling write failures, parent directory validation- Permission errors: insufficient write permissions → IO_ERROR exit code
- Edge cases: empty command registry, commands with no options, nested subcommands
- Type registration: lookup of unregistered types returns null/freeform completion
- Enum registration: enum constant name conversion to lowercase works correctly
Additional verification:
- Generated bash scripts pass
bash -nsyntax check- Generated zsh scripts pass
zsh -nsyntax check- Completion script is valid UTF-8 with no byte-order mark
Add these to the testing strategy section.
core/src/main/java/dev/metaschema/core/datatype/IDataTypeProvider.java (1)
23-29: Fix typo in Javadoc.Line 25 contains a typo: "adpater" should be "adapter".
🔎 Proposed fix
* Get the type information for abstract item types that do not have an - * associated data type adpater. + * associated data type adapter. *
🧹 Nitpick comments (11)
PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (1)
396-412: Verify XPath namespace handling tests cover schema-relevant namespaces.The
NamespaceContextexample is helpful, but Phase 2 acceptance criteria (lines 152–165) should explicitly require tests for namespace-qualified XPath queries commonly used in schema processing (e.g.,/xs:schema/xs:simpleTypewith the XSD namespace registered). This prevents runtime namespace-resolution failures when deployed.PRDs/20251206-build-cleanup/implementation-plan.md (1)
274-421: Javadoc PRs (7-12) lack concrete deliverables and definition of done.PRs 7-12 define scope directories and estimated file counts ("~40-50") but lack:
- Exact file lists to document
- Clear Javadoc coverage targets (zero warnings? 100% method docs?)
- Guidance on what qualifies as "complete" for acceptance
Recommend creating a Javadoc audit or tool output listing:
- Current warnings per module
- Target state (e.g., zero warnings)
- Specific files requiring attention
This will make PRs actionable and measurable before implementation.
core/src/main/java/dev/metaschema/core/datatype/markup/MarkupLine.java (1)
6-6: Update coding guidelines to reflect the new package naming convention.The coding guidelines currently state: "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this PR establishes
dev.metaschema.*as the new canonical convention. Please update the coding guidelines document (docs/javadoc-style-guide.md or wherever the guideline is maintained) to reflect this breaking change.As per coding guidelines (which need updating), the old convention was
gov.nist.secauto.metaschema.*.Do you want me to help identify which documentation files need to be updated to reflect the new package naming convention?
PRDs/20251208-constraint-builder/implementation-plan.md (1)
1-20: Add explicit inter-PR dependency documentation.The PR strategy section should document dependencies on the companion module-builder PRD (specifically that both PRDs will add methods to
IModuleMockFactory). This will clarify sequencing and merge order for reviewers and maintainers.PRDs/20251208-module-builder/implementation-plan.md (1)
35-35: Clarify module definition wiring in acceptance criteria.PR 2 acceptance criteria (lines 35, 39–41) state that
toDefinition(IModule)methods setgetContainingModule()and thattoModule()"builds all definitions with proper module wiring." Specify the exact mechanism: Do builders call a setter on the definition object, or does the module constructor wire the reference? This will help reviewers validate implementation completeness.Also applies to: 39-41
core/src/main/java-templates/dev/metaschema/core/MetaschemaJavaVersion.java (1)
22-55: Consider adding @nonnull annotations for null-safety guarantees.The getter methods return non-null String constants, but lack explicit
@NonNullannotations. Adding these annotations would strengthen the API contract and enable static analysis tools to catch potential issues in calling code.🔎 Proposed addition of @nonnull annotations
Import the annotation at the top of the file:
package dev.metaschema.core; import dev.metaschema.core.util.IVersionInfo; + +import edu.umd.cs.findbugs.annotations.NonNull;Then annotate the return types:
@Override + @NonNull public String getName() { return NAME; } @Override + @NonNull public String getVersion() { return VERSION; } @Override + @NonNull public String getBuildTimestamp() { return BUILD_TIMESTAMP; } @Override + @NonNull public String getGitOriginUrl() { return ORIGIN; } @Override + @NonNull public String getGitCommit() { return COMMIT; } @Override + @NonNull public String getGitBranch() { return BRANCH; } @Override + @NonNull public String getGitClosestTag() { return CLOSEST_TAG; }core/src/main/java/dev/metaschema/core/metapath/cst/type/Treat.java (1)
22-25: Pre-existing Javadoc reference may be incorrect.The class Javadoc references the XPath 3.1 "cast as" operator (
#id-cast), but this is theTreatclass which implements the "treat as" expression. The correct XPath 3.1 reference would be#id-treat. This appears to be a pre-existing issue not introduced by this PR's namespace changes.🔎 Proposed fix
/** * A compact syntax tree node that supports the Metapath - * <a href="https://www.w3.org/TR/xpath-31/#id-cast">"cast as" operator</a>. + * <a href="https://www.w3.org/TR/xpath-31/#id-treat">"treat as" expression</a>. */core/src/main/java/dev/metaschema/core/datatype/adapter/DateTimeWithTZAdapter.java (1)
26-26: Consider using the preferred documentation URL.The Javadoc references
https://pages.nist.gov/metaschema/..., but the project's preferred and canonical documentation domain ishttps://framework.metaschema.dev/. Consider updating to use the canonical URL for consistency with project documentation standards.Based on learnings, framework.metaschema.dev is the official reference domain for Metaschema framework documentation.
PRDs/20251215-shell-completion/PRD.md (2)
122-138: Document enum constant name handling inCompletionTypeRegistry.forEnum().Line 137 converts enum constant names to lowercase for completion generation:
.map(String::toLowerCase)This is a sensible default, but should be documented explicitly in the Javadoc or in the PRD's type mapping guide to clarify that users can expect lowercase enum values in completions (e.g.,
Format.XML→ completion offersxml). This prevents surprises if a tool or script relies on exact case matching.
56-78: Add security/escaping guidance toICompletionTypeimplementers.Custom
ICompletionTypeimplementations (like user-defined completion types) should properly escape or validate their bash/zsh output to prevent injection. Add to theICompletionTypeJavadoc:Security Note: Implementations must ensure returned bash/zsh completion code is properly escaped and safe. Avoid embedding unsanitized strings directly into shell syntax. Use shell-safe values (alphanumeric,
-,_) or properly quote complex values.This prevents a third-party completion type from accidentally or maliciously generating unsafe shell code.
Also applies to: 140-150
PRDs/20251217-parallel-constraint-validation/implementation-plan.md (1)
140-247: ParallelValidationConfig: solid design with proper ownership semantics.The class correctly distinguishes between:
- Internal executor (created via
withThreads(int)): owned by config, shut down byclose()- External executor (provided via
withExecutor(ExecutorService)): not shut down, caller retains ownershipImplementation notes:
withThreads(1)correctly delegates toSEQUENTIALto avoid unnecessary object creation (line 190-191)- Lazy initialization of internal executor with double-checked locking (lines 218-224) is appropriate for the single-threaded validation context
close()properly handles thread interruption and usesawaitTermination(60, TimeUnit.SECONDS)(line 238)NullPointerExceptionfromObjects.requireNonNull()is standard for null-safety (line 172)Potential improvement: Consider whether 60-second termination timeout is appropriate for all use cases, or if it should be configurable. This is a minor design consideration, not a defect.
core/src/main/java/dev/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java
Show resolved
Hide resolved
core/src/main/java/dev/metaschema/core/metapath/cst/items/DecimalLiteral.java
Show resolved
Hide resolved
core/src/main/java/dev/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java
Show resolved
Hide resolved
core/src/main/java/dev/metaschema/core/metapath/format/IPathSegment.java
Outdated
Show resolved
Hide resolved
david-waltermire
force-pushed
the
feature/616-package-rename
branch
2 times, most recently
from
650bcd2 to
dcb3732
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.github/workflows/build.yml:
- Line 276: The grep pattern used to count timeouts in the TIMEOUTS assignment
is wrong: change the pattern in the TIMEOUTS command from matching an
asterisk-prefixed entry to match lychee's actual output by using the same style
as the ERRORS grep (i.e., use the pattern that looks for "^[TIMEOUT]" entries in
html-link-report.md), so the TIMEOUTS variable correctly counts lines starting
with "[TIMEOUT]".
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
65-65: Fix typo in string literal.The word "abiguous" should be "ambiguous" in the toString() output.
🔎 Proposed fix
- return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)");
♻️ Duplicate comments (2)
PRDs/20251208-constraint-builder/implementation-plan.md (1)
56-57: Clarify IModuleMockFactory package location with module-builder PRD.A previous review flagged a critical conflict: this plan places
IModuleMockFactory.javaatdev/metaschema/core/testsupport/builder/(line 56), but the sibling module-builder PRD places it atdev/metaschema/core/testing/model/. Both plans claim to modify the same class with different methods (constraintSet()vs.module()).Before proceeding, confirm:
- Which location is canonical?
- Should both methods coexist on the same class or be split?
- Is this PRD's location decision coordinated with the module-builder PRD?
.github/workflows/build.yml (1)
300-301: Critical: Exit code logic may still be inverted (past review issue appears unresolved).A past review comment (marked "Addressed in commit cc4aac8") flagged that lychee uses:
- Exit code 2 for broken links (actual link-check failures)
- Exit code 1 for runtime/config errors (including timeouts)
However, the current code still checks
exit_code == 1to trigger issue creation and job failure, with comments stating1=errors found, 2=timeouts only. If the past review's documentation reference is correct, this logic is inverted:
- Broken links (exit 2) will NOT create issues or fail the job
- Timeouts (exit 1) WILL create issues and fail the job
This defeats the purpose of separating timeout handling from genuine link failures. Please verify the current lychee exit code semantics and correct the branching logic if needed.
What are the exit codes used by lychee link checker version 0.14 or later?Based on learnings: The prior review at this location cited lychee documentation confirming exit code semantics, and this issue was marked resolved. If the current code is correct, the lychee behavior may have changed or the prior review was mistaken—verification is needed.
Also applies to: 310-312
Also applies to: 310-312
🧹 Nitpick comments (6)
core/src/main/java/dev/metaschema/core/mdm/impl/AbstractDMNodeItem.java (1)
15-15: Optional: Fix typo in Javadoc.The word "assmebly" should be "assembly". This is a pre-existing typo, not introduced by this PR, but could be corrected for clarity.
🔎 Proposed fix
- * This abstract Metapath assmebly node item implementation supports creating a + * This abstract Metapath assembly node item implementation supports creating aPRDs/20251224-codegen-quality/implementation-plan.md (2)
115-119: Clarify path documentation for consistency.The "Files to Modify" section (lines 115–119) explicitly lists paths using the
dev/metaschema/package convention, but the "Files Actually Modified" section (lines 167–180) uses ellipsis abbreviations (e.g.,databind/src/main/java/.../config/binding/) that obscure the actual package namespace.For readers unfamiliar with the repository structure and to ensure consistency with the canonical
dev.metaschema.*convention, consider either:
- Expanding all abbreviated paths to match the explicitness of lines 115–119, or
- Adding a clear pattern explanation at the document start (e.g., "Ellipses represent
dev/metaschema/databind/codegen/...").Also applies to: 167-180
1-10: Add context statement about package naming pre-condition.This implementation plan documents work that assumes Java packages have been (or are being) renamed from
gov.nist.secauto.metaschema.*todev.metaschema.*(PR #618). Adding a brief statement at the beginning (e.g., in the Prerequisites section) that explicitly names this pre-condition would help readers unfamiliar with PR #618 understand the full context.Example addition to Prerequisites (after line 11):
- Package rename to
dev.metaschema.*(PR #618) must be merged before this plan is executedThis is optional and primarily a documentation clarity improvement.
PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)
125-137: Add verification step for characterization tests against XSD specification.The characterization test approach (test JDOM2 behavior, then verify new implementation matches) is sound, but risks perpetuating JDOM2 bugs if they exist.
Recommend adding an explicit step to validate both implementations against the W3C XSD specification for correctness (namespace handling, XPath 1.0 semantics for
/xs:schema/xs:simpleType, etc.), rather than relying solely on behavioral equivalence with the original.
414-422: DOM element serialization approach is valid; consider adding error handling discussion.The
Transformerapproach for serializing DOM elements toXMLStreamWriteris correct, but the plan should note potential error conditions:
TransformerFactory.newInstance()can raiseTransformerFactoryConfigurationErrortransform()can raiseTransformerExceptionif the transformation failsStAXResultcompatibility depends on the Transformer implementationRecommend adding a note in Phase 2 or 5 about error handling strategy (retry logic, logging, fallback behavior).
PRDs/20251215-shell-completion/implementation-plan.md (1)
485-533: Clarify type assignments and enum registration in Phase 5.Lines 516-533 list files to update with
.type()calls and provide type assignments (table at lines 523-532). However, the plan does not specify where enum completion types should be registered.For example,
TO_OPTIONshould useFormat.classtype, but the plan should clarify:
- Where
Format.classcompletion type is registered (e.g., in command initialization, static block, or SPI)- Whether other custom enum types need similar registration
- If registration happens in MetaschemaCommands or elsewhere
Add explicit registration locations to Phase 5 to ensure completion behavior is wired up for all custom types.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
PRDs/20251208-module-builder/implementation-plan.md (1)
1-142: Add cross-reference to constraint-builder PRD in Dependencies section.The implementation plan depends on coordination with the companion constraint-builder PRD (which also modifies
IModuleMockFactory), but there is no explicit Dependencies section documenting this relationship. Add a Dependencies section near the beginning to clearly state:
- Constraint-builder PRD (path/issue reference)
- Coordination needed on
IModuleMockFactory- Recommended merge/implementation sequence
This ensures future readers understand the inter-PRD dependencies and avoid accidental conflicts.
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
65-65: Fix typo in toString() output.The string literal contains a typo: "abiguous" should be "ambiguous". This affects the user-facing output of the
toString()method.🔎 Proposed fix
- return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)");core/src/main/java/dev/metaschema/core/datatype/adapter/HexBinaryAdapter.java (1)
24-31: Pre-existing Javadoc error: references base64 instead of hex binary.The class-level Javadoc incorrectly describes this adapter as handling "base64 encoded string" and links to the base64 datatype specification. This appears to be a copy-paste error from
Base64Adapter.The documentation should reference "hexadecimal binary" or "hex binary" encoding and link to the hex-binary datatype specification instead.
🔎 Suggested correction
/** - * Maintains a byte buffer backed representation of a byte stream parsed from a - * base64 encoded string. + * Maintains a byte buffer backed representation of a byte stream parsed from a + * hexadecimal encoded string. * <p> * Provides support for the Metaschema <a href= - * "https://pages.nist.gov/metaschema/specification/datatypes/#base64">base64</a> + * "https://pages.nist.gov/metaschema/specification/datatypes/#hexbinary">hexBinary</a> * data type. */Note: This is a pre-existing issue not introduced by this PR, but worth correcting while the file is under review.
PRDs/20251217-cli-processor-refactor/implementation-plan.md (1)
1011-1029: Missing import for @nonnull annotation in CallingContextTest.The CallingContextTest code snippet uses
@NonNullannotation (line 1048) but does not include the required import statement. This will cause a compilation error.Add this import to the imports section:
import edu.umd.cs.findbugs.annotations.NonNull;
♻️ Duplicate comments (2)
PRDs/20251208-module-builder/implementation-plan.md (1)
10-10: Unresolved: IModuleMockFactory path conflict with constraint-builder PRD requires coordination.This plan places
IModuleMockFactoryatdev/metaschema/core/testing/model/IModuleMockFactory.java(lines 10, 56, 75), but the companion constraint-builder PRD specifies the path asdev/metaschema/core/testsupport/builder/IModuleMockFactory.java. Both PRDs must add methods to this class:
- constraint-builder adds
constraintSet()- module-builder adds
module()(line 10, 16)Action required:
- Confirm the canonical location for
IModuleMockFactory(suggestdev/metaschema/core/testsupport/builder/to align with naming intent)- Update this plan to reference the agreed-upon path
- Ensure both methods coexist on the same interface
- Coordinate implementation sequence across both PRs to avoid merge conflicts
Also applies to: 56-56, 75-75
.github/workflows/build.yml (1)
276-276: Fix the TIMEOUT grep pattern - issue from previous review still present.The pattern
^\* \[TIMEOUT\]is incorrect and was flagged in the previous review. Lychee outputs[TIMEOUT]without an asterisk prefix (consistent with the[ERR]pattern on line 275). The current pattern will never match, causing the timeout warning at lines 283-284 to never display.🔎 Proposed fix
- TIMEOUTS=$(grep -c "^\* \[TIMEOUT\]" html-link-report.md 2>/dev/null || echo "0") + TIMEOUTS=$(grep -c "^\[TIMEOUT\]" html-link-report.md 2>/dev/null || echo "0")
🧹 Nitpick comments (2)
PRDs/20251217-context-functions/PRD.md (1)
122-124: Clarify size computation strategy: eager vs. lazy evaluation.Line 124 (NFR-2) states size computation should be "lazy/efficient (avoid computing sequence size unless
last()is called)", but the Phase 3 design (line 198) computes size eagerly:int size = retval.size().Consider documenting whether upfront size computation is acceptable or if deferred evaluation is required. This affects both performance and the FocusContext API design (e.g., whether size is computed on-demand or stored).
Also applies to: 196-210
core/src/main/java/dev/metaschema/core/datatype/markup/flexmark/impl/IMarkupVisitor.java (1)
6-6: LGTM! Package rename aligns with PR objectives.The package declaration correctly uses the new
dev.metaschema.*convention. Note that the coding guidelines still reference the oldgov.nist.secauto.metaschema.*convention and should be updated.Based on learnings, the canonical convention is now
dev.metaschema.*as of this PR.
david-waltermire
force-pushed
the
feature/616-package-rename
branch
from
dcb3732 to
ad56332
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
65-65: Fix typo in string literal.The string contains
"(abiguous)"which should be"(ambiguous)".🔎 Proposed fix
- return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)"); + return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)");
🧹 Nitpick comments (23)
PRDs/20251221-xmlbeans-removal/implementation-plan.md (1)
16-17: Clarify the reference material path for external users.The reference path
databind-modules/target/generated-resources/schema/json/metaschema-model_schema.jsonpoints to a Maven build artifact, which requires running a full build to generate. Consider documenting the build step required to generate this file, or providing a stable URL or alternative reference for IDE validation.PRDs/20251217-context-functions/PRD.md (1)
126-190: Solution design is sound and backward-compatible.The phased approach (FocusContext → DynamicContext → PredicateExpression → StaticContext) is logical and maintains API compatibility. The distinction between
subContext()(preserves focus) andsubContext(FocusContext)(sets new focus) is important for correctness, but the table at lines 186–189 combined with the code example at lines 200–210 adequately conveys the intent.Minor suggestion: When implementing, ensure the Javadoc for
subContext()andsubContext(FocusContext)clearly distinguishes their behaviors to avoid confusion during integration with FLWOR expressions and other sub-context scenarios.PRDs/20251215-shell-completion/implementation-plan.md (1)
486-533: Task 5.1: Enum Registration Wiring Left Implicit.The option type assignments table (lines 522–531) specifies that
TO_OPTION,AS_FORMAT_OPTION, andAS_SCHEMA_FORMAT_OPTIONshould use custom enum types (e.g.,Format.class). However, the task does not explicitly document where and when these enum types are registered withCompletionTypeRegistry.registerEnum().Add a paragraph clarifying the registration strategy: for example, whether enum registration happens at module initialization, in a static block within command classes, or via a centralized configuration method. This ensures implementations don't miss the registration step and enum completions work end-to-end.
PRDs/20251228-validation-errors/implementation-plan.md (4)
44-50: PathTracker implementation is straightforward; clarify thread-safety requirement.The acceptance criteria (line 56) mention "thread-safe or documented as not thread-safe," but the code example uses
ArrayDeque<String>without synchronization. Since PathTracker is used during single-threaded parsing (lines 204, 219), document explicitly that it is not thread-safe and intended for single-threaded parsing contexts only.Add a Javadoc note to PathTracker explicitly stating:
This class is not thread-safe and should only be used within a single parsing thread.
185-195: AbstractProblemHandler updates require careful implementation of error message formatting.Task 3.2 specifies using format-appropriate terminology (attribute/element for XML, property for JSON) and grouping by type. The acceptance criteria are clear, but implementation complexity is higher than suggested. Ensure:
- Error messages are human-readable and include all context (location, path, type)
- Null field values (Issue #205, Task 2.4) don't cause NPE or incomplete messages
- Default values are correctly distinguished from missing required values
Consider extracting error message formatting logic into a separate formatter class (e.g.,
ValidationErrorFormatter) to keepAbstractProblemHandlerfocused on exception handling rather than string formatting.
242-250: Null URI handling in deserializers is important for robustness.Task 3.6 specifies using a "synthetic URI" for null cases. Define what a synthetic URI looks like (e.g.,
file:///unknown,memory:///synthetic?) and ensure:
- Error messages remain informative even with synthetic URI
- Synthetic URIs don't pollute production logs or error reports
- The choice of synthetic URI is documented in code comments
Use a descriptive synthetic URI like
synthetic:///deserializationorunknown://sourceand document the choice inDefaultXmlDeserializerandDefaultJsonDeserializerJavadoc for clarity.
302-309: Risk assessment is thorough and mitigations are practical.The four identified risks (backward compatibility, performance, edge cases, IResourceLocation integration) are relevant. Mitigations are sound. One additional risk to consider: Test data management — ensure that Metaschema test modules (referenced in Phase 2.1) are version-controlled and not generated dynamically, to keep tests reproducible and maintainable.
Add a note to the Risk Assessment clarifying that test Metaschema modules and fixtures are committed to source control and documented for maintainability.
PRDs/20251224-codegen-quality/implementation-plan.md (2)
1-10: Document context: Consider adding a note clarifying relationship to PR #618.This implementation plan documents phases 1–5 of code generator quality improvements across multiple historical PRs. Since this file is being updated as part of PR #618 (package rename), readers may benefit from a brief introductory note explaining that all file paths and package names herein reflect the new
dev.metaschema.*namespace.For example, add a note after line 3:
> **Note:** This document was updated in PR #618 to reflect the package rename > from `gov.nist.secauto.metaschema.*` to `dev.metaschema.*`. All file paths > and package names herein use the current canonical namespace.
1-1: Minor framing clarification: Document reads as retrospective, not forward-looking plan.The document is titled "Implementation Plan" but comprehensively documents completed work across five PRs with all acceptance criteria marked
[x]and PR status marked✅ Completed. Readers may be clearer if the title or opening reflects this is a historical reference/retrospective document. For example:
- Current: "Implementation Plan: Code Generator Quality Improvements"
- Suggested: "Implementation Summary: Code Generator Quality Improvements"
or- Add a subtitle: "Completed phases 1–5 across PRs #577, #584, #593"
This helps readers immediately understand the document is a record of completed work, not a forward-facing roadmap.
Also applies to: 26-26, 98-98, 184-184, 252-252, 312-312, 391-402
PRDs/20251217-parallel-constraint-validation/implementation-plan.md (9)
1041-1043: Verify PARALLEL_THRESHOLD constant tuning and documentation.The threshold of 4 sibling nodes (line 1041–1043) determines when to switch from sequential to parallel traversal. This is a tuning parameter that should be validated:
- Is 4 optimal for typical document structures and JVM thread pools?
- Should this be configurable or left as a constant?
- Add an inline comment explaining why 4 was chosen (CPU cores, typical list sizes, etc.).
Consider documenting this threshold in ParallelValidationConfig Javadoc or in a separate tuning guide for operators.
1206-1231: Validate thread count argument parsing and error messages.The
getParallelConfig()helper (lines 1206–1231) parses--threadsand validates the count. Ensure:
- Error messages are actionable (line 1222: "Invalid thread count: " + value should include the reason).
- The validation at line 1224 rejects 0 and negative values with a clear message (currently done, good).
- Non-numeric input is caught and reported clearly (line 1221 catches
NumberFormatException).Consider unifying error message formatting with the rest of the CLI for consistency. Also verify that invalid arguments throw
InvalidArgumentExceptionas expected by the CLI framework.
476-509: FindingCollectingHandler concurrency test helper methods are incomplete.The test
testConcurrentAddFindings()(lines 476–509) callshandler.handleExpectViolation(...)with mock objects created by unimplemented helper methods (lines 493–495:createMockConstraint(),createMockNode(),createMockContext()). Before finalizing this task:
- Implement the mock helper methods with realistic objects.
- Verify that the mocks correctly implement the required interfaces.
- Ensure the test actually exercises the thread-safe code paths in the real handler.
Without these helpers, the test cannot run. Consider using
Mockitoor similar framework if the real objects are too heavyweight.I can help generate the mock helper implementations if you provide details on the constraint and node item interfaces. Would you like me to create those?
588-594: Incomplete helper methods in FindingCollectingHandler test block further test execution.Lines 588–594 define stub helper methods
addFinding()andaddFindingWithPath()with "Implementation using mocks" comments. These methods must be fully implemented before the tests can run. Provide:
- Real or mocked implementations of the helper methods.
- Proper mock setup for
ConstraintValidationFindingand related interfaces.- Test fixtures for consistent mock data across test methods.
Suggest creating a separate
TestFixturesorConstraintTestHelperutility class if mocking becomes complex.I can generate the full test fixture and helper implementations if you clarify the structure of
ConstraintValidationFinding,INodeItem, and related interfaces.
720-751: DefaultConstraintValidator thread-safety test is a skeleton without executable code.The test
testConcurrentValueMapAccess()(lines 720–751) creates a validator but does not invoke any actual validation methods. The comment at lines 734–737 says "Call methods that access valueMap" but provides no implementation. This test will pass vacuously and provide no value.Complete this test with:
- Real calls to constraint validation methods that exercise
valueMapmutations.- Assertions that verify no
ConcurrentModificationExceptionis thrown.- Proper setup of mock node items and constraints to trigger validation.
Same issue exists for
testConcurrentIndexKeyRefAccess()(lines 754–784).I can generate complete implementations of these tests with proper mock setup and actual validator method calls. Would you like me to do that?
883-915: ParallelValidationTest is incomplete with commented-out core test logic.The key test
testSequentialAndParallelProduceSameResults()(lines 883–915) has the critical assertions commented out (lines 902–913). Without these assertions, the test does nothing and will pass trivially.Uncomment and fully implement:
- The test document creation (line 902).
- The dynamic context initialization (line 904).
- The actual validation calls (lines 906–907).
- The findings comparison and assertions (lines 909–912).
This is essential to validate the PR's core promise: that parallel and sequential modes produce identical results.
I can generate the complete test using a realistic sample Metaschema document if you provide a reference or test fixture path.
933-945: Verify ParallelValidationConfig backward-compatibility constructor behavior.The test at lines 933–945 validates that both the new constructor (with
ParallelValidationConfig) and the old constructor (handler only) work. However, it does not verify:
- That the old constructor correctly defaults to
SEQUENTIALconfig.- That sequential and parallel validators produce identical findings on simple documents.
- That exception handling is consistent between the two constructors.
Add assertions to verify the sequential default and consider adding a comparative validation test.
1315-1331: Integration tests are stubs and lack concrete implementations.The integration tests in Task 7 (lines 1315–1331) are mostly empty placeholders:
- Line 1315–1318: No actual CLI invocation or assertion.
- Line 1321–1324: Comments describe intent but no test code.
- Line 1327–1330: Performance test lacks baseline comparison.
Provide full implementations including:
- Actual CLI command invocation (e.g., using
ProcessBuilderor a test CLI utility).- Assertion of SARIF output correctness.
- Performance baseline comparison (if this is a requirement).
- Sample document creation or reference to existing test fixtures.
Without these, the integration test layer is not functional.
I can generate complete integration tests with CLI invocation and SARIF output validation if you provide paths to sample documents or test fixtures.
1353-1364: Verification checklist should include performance and correctness metrics.The checklist (lines 1353–1364) covers compilation and option parsing but lacks:
- Quantitative performance metrics: How much faster should parallel be on typical documents? (2×, 4×, etc.)
- Correctness validation: Explicit statement that SARIF outputs from sequential and parallel modes are byte-for-byte identical or semantically equivalent.
- Scalability limits: What is the maximum thread count tested? Does it scale linearly?
- Stress testing: Large document validation (1000+ sibling nodes) to trigger parallel threshold.
- Regression testing: Validation that existing non-parallel tests still pass.
Recommend adding these metrics to the checklist to ensure quality gates are clear.
PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (3)
143-143: Provide concrete guidance for parameterized test approach.The plan mentions parameterizing tests to validate both JDOM2 and new implementations (line 143: "or parameterize to test both"), but doesn't detail the expected test structure (e.g., parameterized fixture, abstract test base, test inheritance pattern). Provide an example or reference to clarify the expected test organization for reviewers.
264-272: Add performance regression testing to Phase 6 verification.The Phase 6 final verification includes semantic equivalence checks (comparing generated schema output before/after) but doesn't address performance implications of migrating from JDOM2/Saxon to standard DOM/XPath APIs. Consider adding guidance for benchmarking or profiling schema generation performance (execution time, memory usage) to detect regressions.
290-301: Clarify public API visibility for new implementations.The plan lists new classes (IndentingXMLStreamWriter, XmlSchemaLoader, DomDatatypeContent) under the .impl package, which conventionally indicates internal APIs. Explicitly state in the acceptance criteria for each new class whether it is:
- Package-private: Available only within dev.metaschema.schemagen.xml.impl (recommended for internal wrappers)
- Public: Part of the stable public API (requires API contract documentation)
core/src/main/java/dev/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (1)
84-93: Minor typo in Javadoc.Line 89: "whoose" should be "whose"
🔎 Proposed fix
* Visit each child expression of the provided {@code expr}, aggregating the * results. * * @param expr - * the expression whoose children should be visited + * the expression whose children should be visited * @param context * used to pass additional state * @return the aggegated result.github/workflows/build.yml (1)
276-276: LGTM! TIMEOUT pattern correctly fixed.The grep pattern now matches
^\[TIMEOUT\], consistent with the ERRORS pattern on line 275. This addresses the previous review feedback and correctly matches lychee's markdown output format.The elif logic prioritizes errors over timeouts, which is reasonable since broken links are more critical. Note that if both errors and timeouts exist, the timeout count won't appear in the summary (though it remains in the report artifact).
Optional: Consider showing both metrics
If you want to display both errors and timeouts in the summary when both occur, you could replace the elif with separate if statements:
- elif [ "$TIMEOUTS" -gt 0 ]; then + fi + if [ "$TIMEOUTS" -gt 0 ]; then echo ":warning: **$TIMEOUTS link(s) timed out** (external sites may be slow)" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY - else + fi + if [ "$ERRORS" -eq 0 ] && [ "$TIMEOUTS" -eq 0 ]; then echo ":white_check_mark: **All links valid**" >> $GITHUB_STEP_SUMMARY fiThis would show both the error count and timeout count when both are present. The current approach is also valid if you prefer to highlight only the most critical issue.
Also applies to: 283-284
…etaschema BREAKING CHANGE: All Java packages have been renamed to align with the Maven groupId (dev.metaschema.java). Changes: - Renamed all Java packages from gov.nist.secauto.metaschema.* to dev.metaschema.* - Updated JPMS module names accordingly - Renamed SPI service files to match new package names - Updated all pom.xml path references - Moved java-templates to correct package paths - Regenerated bootstrap binding classes - Updated documentation, .claude files, and skills - Updated CI workflow to build maven-plugin first (avoids stale remote SNAPSHOT) - Fixed CI to treat link checker timeouts as warnings, not errors - Fixed Javadoc issues (incomplete tags, typos, redundant code) Migration required for downstream users: - Update all import statements - Update JPMS requires directives - Update SPI service file references - Update any reflection-based code using FQCNs Closes metaschema-framework#616
david-waltermire
force-pushed
the
feature/616-package-rename
branch
from
ad56332 to
1820c4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)
46-53: Defensive copy needed to ensure immutability.The class documentation claims thread-safety through immutability (line 23), but the constructor assigns the
argumentslist directly without a defensive copy. If the caller retains a reference to a mutable list and modifies it after construction, theCalledContextstate could change, violating the immutability guarantee.🔎 Proposed fix
public CalledContext( @NonNull IFunction function, @NonNull List<ISequence<?>> arguments, @Nullable IItem contextItem) { this.function = function; this.contextItem = contextItem; - this.arguments = arguments; + this.arguments = List.copyOf(arguments); }core/src/main/java/dev/metaschema/core/datatype/adapter/Base64Adapter.java (1)
28-28: Update documentation URL to use the canonical domain.The Javadoc references
https://pages.nist.gov/metaschema/specification/datatypes/#BASE64. Update this tohttps://framework.metaschema.dev/specification/datatypes/#BASE64to align with the project's canonical documentation URL.
🧹 Nitpick comments (13)
PRDs/20251231-choice-required-fix/implementation-plan.md (3)
51-63: Javadoc updates should clarify null-safety contract.The plan to enhance setter Javadoc with nullability guidance is good. Ensure the updated getter Javadoc for
isRequired()explicitly states: "Returns false for properties inside choice blocks, as their requirement is conditional on the choice branch being selected." This prevents future misinterpretations.
84-105: Acceptance criteria are well-defined but could be more specific on verification.The criteria correctly require testing of null-safety annotations on generated code and choice property handling. Add explicit checkpoints: (1) verify regenerated classes have
@Nullableon all choice property getters, and (2) run integration tests with the regenerated bootstrap classes to catch any annotation-driven behavior changes.
106-118: Files changed summary uses correct dev/metaschema paths.All file paths reflect the PR #618 package rename to
dev.metaschema.*. The summary correctly identifies modified, new, and regenerated files. Consider adding a note that the regenerated files (lines 116-118) will be automatically updated and should not be manually edited—improvements should flow through the code generator.PRDs/20251217-parallel-constraint-validation/implementation-plan.md (1)
1015-1088: Parallel traversal strategy with threshold-based dispatch is sound.The plan:
- Uses a
PARALLEL_THRESHOLDconstant (4) to decide when to parallelize (line 1041–1042).- Avoids parallelization overhead for small child counts (sequential for < 4).
- Submits each child as independent task (lines 1054–1059).
- Collects futures and waits with
future.get(), propagating exceptions (lines 1063–1078).- Handles
ExecutionException,InterruptedException, and cancels remaining futures on failure (lines 1067–1087).The threshold of 4 is reasonable (avoids overhead for tiny sibling groups). The exception handling correctly distinguishes
RuntimeException(re-throws) from checked exceptions (wraps inConstraintValidationException). Good defensive coding on interruption (line 1076 restores interrupt flag).One note: Consider whether
context.subContext()is idempotent and lightweight (line 1056)—the plan mentions it copiesexecutionStackandletVariableMap, which is O(n). For deeply nested documents with many siblings, this could add overhead. Monitor performance in practice; if problematic, consider a more efficient copy strategy or copy-on-write semantics in a future iteration.PRDs/20251217-context-functions/PRD.md (1)
349-350: Clarify DefaultFunctionLibrary updates split between PR 2 and PR 3.Line 349 lists
DefaultFunctionLibrary.javaas modified in PR 2 (forFnPosition/FnLast), but line 369 also lists the same file for PR 3 (forFnDefaultLanguage). This is likely intentional (both functions register in the same library), but the phasing should clarify this shared dependency to prevent integration surprises.Consider either:
- Consolidating both function registrations in PR 2 with a forward reference, or
- Explicitly noting that PR 3 adds the
FnDefaultLanguageregistration to the library created in PR 2.Also applies to: 369-369
PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (3)
123-150: Clarify test parameterization strategy for JDOM2 → DOM migration.The Phase 2 approach (write characterization tests against JDOM2, then verify new DOM implementation passes same tests) is sound, but the plan should clarify how tests will handle namespace-context differences between the two APIs. Mixed content documents can be polluted with indentation characters if the indentation logic is not carefully designed, which applies equally to XPath query results that may vary in whitespace handling.
For example, JDOM2 uses Namespace objects while javax.xml.xpath requires a NamespaceContext implementation (lines 396-411). Consider:
- Will tests use a single parameterized suite with adapter classes, or separate test classes?
- How will whitespace-sensitive assertions be handled?
- Are there XPath queries (e.g., text node selection) that behave differently between the APIs?
This clarification will prevent implementation surprises.
318-364: Minor pseudocode clarity: document the push/reset sequence for state tracking.The stack-based state tracking design is sound, but the order of operations in the
writeStartElementpseudocode (lines 334-340) should explicitly document when parent state is saved relative to delegation and depth increment. The current structure (save, delegate, increment, reset) is correct, but a simple comment clarifying the sequence would prevent implementation misinterpretation:writeStartElement: if (!hasText) write newline + indent // indent before tag hasTextStack.push(hasText) // SAVE parent state first delegate.writeStartElement(...) // delegate to underlying writer depth++ // track new level hasText = false // reset for child contextThis ensures the stack correctly preserves each ancestor's mixed-content state.
264-272: Expand Phase 6 Final Verification with more concrete checks.Phase 6 acceptance criteria (lines 266-272) are minimal. Add explicit verification steps such as:
- Verify no remaining JDOM2/Saxon imports in compiled bytecode (class files)
- Compare generated XSD output byte-for-byte or semantically before/after to confirm indentation equivalence
- Run integration tests (if any) that depend on the schemagen module
- Verify Maven plugin invocations work end-to-end with new implementation
Additionally, the PR objectives mention "migration guidance" for downstream projects—consider adding a note in Phase 6 about updating README or migration docs (if applicable to this PR scope).
PRDs/20251217-cli-processor-refactor/implementation-plan.md (1)
1048-1051: Add@NonNullannotations to test utility method signature.Line 1048 defines
createContext()without@NonNullannotations, while the method always returns a non-nullCallingContextand accepts non-null varargs. For consistency with other code in the project and to support static analysis, add annotations:@NonNull private CallingContext createContext(@NonNull String... args) { return new CallingContext(processor, Arrays.asList(args)); }As per coding guidelines, based on learnings, this aligns with the project's null-safety practices.
PRDs/20251215-path-formatters/plan.md (2)
294-294: Clarify task deferral status for JSON value key resolution helper.Line 294 marks the JSON value key resolution helper as deferred (
[ ]unchecked) with note "not needed for basic path formatting." However, the Phase 4 task list (lines 305–315) does not explicitly call out this deferral. If this task is intentionally skipped, confirm it won't be needed for Phase 4 implementation or clarify whether it should be added as a future Phase 5 task.Consider adding a note in the Phase 4 task list (after line 315) clarifying which Phase 2 tasks are deferred and why, to prevent confusion during implementation.
Also applies to: 305-315
1-476: PRD is well-structured and properly aligned with package rename.Overall assessment: This is a comprehensive and well-organized PRD that correctly applies the new
dev.metaschema.*package naming convention throughout (per PR #618). The document clearly separates four implementation phases, provides detailed technical specifications with code examples, includes a robust testing strategy with ~50 test cases across phases, and identifies risks with mitigations. The phase-based task breakdown with checkmark completion tracking is clear.Strengths:
- Clear problem statement and non-goals that prevent scope creep
- Detailed technical specifications for both XPathFormatter and JsonPointerFormatter with examples
- Comprehensive test matrix covering normal cases, edge cases, and special characters
- Risk/mitigation analysis shows forward-thinking about potential issues
- Proper use of
dev.metaschema.*package namespace throughoutMinor areas for improvement (optional, per "Chill" review mode):
- Deferral of the JSON value key resolution helper (line 294) is noted but not explicitly cross-referenced in the Phase 4 task list
- No explicit mention of integration test approach for CLI in Phase 4 (test infrastructure section at lines 470–475 references Maven Invoker but Phase 4 tasks don't detail this)
core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)
86-91: Consider consistent field access style.The
hashCodemethod usesgetFunction().hashCode()whileequals(line 102) accesses thefunctionfield directly. While functionally equivalent, consider using consistent field access for stylistic uniformity.🔎 Optional refactor
@Override public int hashCode() { final int prime = 31; int result = 1; - result = prime * result + getFunction().hashCode(); + result = prime * result + function.hashCode(); return prime * result + Objects.hash(contextItem, arguments); }.github/workflows/build.yml (1)
237-241: LGTM! Consistent with the Code workflow.This pre-build step mirrors the one in the Code workflow and correctly ensures the local plugin is available for the website build.
Optional: Extract to a reusable composite action
To reduce duplication, consider extracting this pre-build step into a reusable composite action that both workflows can reference. This would centralize the logic and make future maintenance easier. However, given the simplicity of the step, the current duplication is acceptable.
aj-stein
left a comment
aj-stein
left a comment
There was a problem hiding this comment.
Prodigious work and a wonderful milestone, great work, @david-waltermire!
david-waltermire
merged commit 99ec065
into
metaschema-framework:develop
2 participants
Summary
gov.nist.secauto.metaschema.*todev.metaschema.*to align with the Maven groupId (dev.metaschema.java)BREAKING CHANGE: All Java packages have been renamed. Downstream projects must update their imports.
Test plan
mvn clean install -PCI -Prelease)Closes #616
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.