feat(uko): add UKO ontology scaffolding #466

2026-02-27T11:47:50Z

hamza.khyari commented

2026-02-27 11:47:50 +00:00

Summary

Add UKO Layer 0-3 ontology skeleton as RDF/TTL (docs/ontology/uko.ttl) with base URI, version IRI, and prefix conventions
Define Layer 0 foundation nodes (Resource, Artifact, CodeArtifact, Document), Layer 1 structure (Module, Function, Class), Layer 2 relationships (imports, defines, inherits), Layer 3 Python-specific nodes
Implement lightweight TTL parser (no external RDF dependencies) with UKOLoader service for URI parsing, inheritance resolution, and validation
Add UKONode, UKOPrefix, UKOVersion, UKOOntology frozen Pydantic v2 models
Validation rejects undefined prefixes and missing rdf:type for nodes

Testing

15 Behave BDD scenarios / 60 steps (all pass)
Robot Framework smoke tests (robot/uko_ontology.robot)
ASV benchmarks (benchmarks/uko_load_bench.py)
Lint clean (ruff)

Files Changed

docs/ontology/uko.ttl — RDF/TTL ontology skeleton
src/cleveragents/domain/models/core/uko.py — UKO Pydantic v2 models
src/cleveragents/application/services/uko_loader.py — TTL parser + validator + inheritance resolver
features/uko_ontology.feature + features/steps/uko_ontology_steps.py — BDD tests
robot/uko_ontology.robot + robot/helper_uko_ontology.py — Integration tests
benchmarks/uko_load_bench.py — ASV benchmarks
docs/reference/uko.md — Reference documentation
CHANGELOG.md — Updated

Closes #189

## Summary - Add UKO Layer 0-3 ontology skeleton as RDF/TTL (`docs/ontology/uko.ttl`) with base URI, version IRI, and prefix conventions - Define Layer 0 foundation nodes (Resource, Artifact, CodeArtifact, Document), Layer 1 structure (Module, Function, Class), Layer 2 relationships (imports, defines, inherits), Layer 3 Python-specific nodes - Implement lightweight TTL parser (no external RDF dependencies) with `UKOLoader` service for URI parsing, inheritance resolution, and validation - Add `UKONode`, `UKOPrefix`, `UKOVersion`, `UKOOntology` frozen Pydantic v2 models - Validation rejects undefined prefixes and missing `rdf:type` for nodes ## Testing - 15 Behave BDD scenarios / 60 steps (all pass) - Robot Framework smoke tests (`robot/uko_ontology.robot`) - ASV benchmarks (`benchmarks/uko_load_bench.py`) - Lint clean (ruff) ## Files Changed - `docs/ontology/uko.ttl` — RDF/TTL ontology skeleton - `src/cleveragents/domain/models/core/uko.py` — UKO Pydantic v2 models - `src/cleveragents/application/services/uko_loader.py` — TTL parser + validator + inheritance resolver - `features/uko_ontology.feature` + `features/steps/uko_ontology_steps.py` — BDD tests - `robot/uko_ontology.robot` + `robot/helper_uko_ontology.py` — Integration tests - `benchmarks/uko_load_bench.py` — ASV benchmarks - `docs/reference/uko.md` — Reference documentation - `CHANGELOG.md` — Updated Closes #189

hamza.khyari added this to the v3.4.0 milestone 2026-02-27 11:47:50 +00:00

hamza.khyari added the

Type

Feature

label 2026-02-27 11:47:50 +00:00

hamza.khyari self-assigned this 2026-02-27 11:48:42 +00:00

CoreRasurae commented

2026-02-27 15:11:41 +00:00

Code Review Report

Commit: 8206fad — fix(uko): address review findings for UKO ontology scaffolding
Author: khyari hamza (Hamza Khyari)
Branch: feature/m6-acms-uko-schema
Issue: #189 — feat(uko): add UKO ontology scaffolding
Files changed: 5 files, +96 / -4

Severity Summary

Severity	Count
Critical (Spec Mismatch)	3
Bug	1
Test Flaw	4
Design Issue	2
Minor / Style	2

CRITICAL — Specification Mismatches

C-1. Ontology URI Namespace Diverges from Specification

Files: docs/ontology/uko.ttl, src/cleveragents/application/services/uko_loader.py:42-47

The specification (docs/specification.md, ~line 40785-40800) defines the UKO namespace prefixes as domain-based names:

Spec Prefix	Spec IRI
`uko:`	`https://cleveragents.ai/ontology/uko#`
`uko-code:`	`https://cleveragents.ai/ontology/uko/code#`
`uko-oo:`	`https://cleveragents.ai/ontology/uko/oo#`
`uko-py:`	`https://cleveragents.ai/ontology/uko/py#`

The implementation uses a completely different naming convention — numeric layer IDs:

Impl Prefix	Impl IRI
`uko0:`	`https://ontology.cleverthis.com/uko/layer0/`
`uko1:`	`https://ontology.cleverthis.com/uko/layer1/`
`uko2:`	`https://ontology.cleverthis.com/uko/layer2/`
`uko3:`	`https://ontology.cleverthis.com/uko/layer3/`

Both the base domain (cleveragents.ai vs ontology.cleverthis.com) and the naming strategy (semantic domain names vs numeric indices) differ. The _LAYER_PREFIXES dict in uko_loader.py:42-47 hardcodes the numeric scheme. This will require a complete rewrite when aligning with the specification.

C-2. Layer 0 Class Hierarchy Does Not Match Specification

File: docs/ontology/uko.ttl:17-35

The specification defines five Layer 0 base classes following an information-theory model:

uko:InformationUnit (root)
uko:Container (subClassOf InformationUnit)
uko:Atom (subClassOf InformationUnit)
uko:Annotation (subClassOf InformationUnit)
uko:Boundary (subClassOf InformationUnit)

The implementation defines four classes following an artifact-centric model:

uko0:Resource (root)
uko0:Artifact (subClassOf Resource)
uko0:CodeArtifact (subClassOf Artifact)
uko0:Document (subClassOf Artifact)

These are fundamentally different ontologies. The spec's Layer 0 is a general-purpose information classification system, while the implementation's Layer 0 is a software-artifact taxonomy. The test in features/uko_ontology.feature:167 hardcodes count: 4 for Layer 0, cementing this divergence.

C-3. Layers 1-3 Structure Does Not Match Specification

File: docs/ontology/uko.ttl:37-80

Layer	Specification	Implementation
1	Four domains: Software (`Module`, `Callable`, `TypeDefinition`), Documents, Data, Infrastructure	Three code-only classes: `Module`, `Function`, `Class`
2	Paradigm specializations: OO (`Class`, `Interface`, `Method`), Functional, Procedural	Three relationship properties: `imports`, `defines`, `inherits`
3	Language-specific extensions: Python, TypeScript, Rust, Java	Three Python classes only: `PythonModule`, `PythonClass`, `PythonFunction`

Notable: the spec places Class at Layer 2 (OO paradigm) but the implementation puts it at Layer 1. The spec's Layer 2 contains classes (OO specializations), while the implementation's Layer 2 contains only ObjectProperty relationships. The spec uses Callable (Layer 1) and Method (Layer 2); the implementation uses Function (Layer 1) which has no spec equivalent at that layer.

BUG

B-1. Validation Produces False-Positive "Undefined Prefix" for Resolved HTTP URIs

File: src/cleveragents/application/services/uko_loader.py:186-201

if node.parent_uri:
    resolved_parent = node.parent_uri
    if resolved_parent not in node_uris and ":" in resolved_parent:  # line 188
        prefix_part = resolved_parent.split(":")[0]
        if prefix_part not in known_prefixes:
            errors.append(...)  # "undefined prefix"

    # NEW CHECK (this commit)
    if resolved_parent not in node_uris:  # line 197
        errors.append(...)  # "non-existent parent"

Two problems:

False prefix error on external URIs. If parent_uri points to an external ontology class (e.g., owl:Thing which resolves to http://www.w3.org/2002/07/owl#Thing), then ":" in resolved_parent is True (because of http:), prefix_part becomes "http", and "http" not in known_prefixes is True. This produces a spurious "undefined prefix 'http:'" error. While the current TTL avoids this scenario, it violates standard RDF practice where cross-ontology references are common.
Duplicate errors. The new existence check at line 197 is not guarded by elif, so both the prefix error and the non-existent parent error fire for the same node. This clutters the error output with redundant messages for a single root cause.

TEST FLAWS

T-1. No Test for `rdfs:domain`/`rdfs:range` URI Resolution

Impact: The primary code fix in this commit

The commit message states "Resolve rdfs:domain and rdfs:range values to full URIs in parser" (uko_loader.py:125-135), yet no Behave scenario or step verifies this behavior. There is no assertion checking that node.properties["rdfs:domain"] contains a fully resolved URI rather than a prefixed name. If this resolution logic regresses, no test will catch it.

T-2. No Test for Parent-URI Existence Validation

Impact: New validation rule at uko_loader.py:196-201

The commit adds a new validation check that rejects nodes whose parent_uri references a non-existent node. But no test scenario exercises this path. The new "Validate rejects ontology with typeless node" test only verifies missing rdf:type, not missing parent references.

T-3. Duplicate/Inconsistent Context Attributes in Step Definitions

File: features/steps/uko_ontology_steps.py

The commit introduces parallel naming conventions for the same concepts:

Pattern	Original	New (this commit)
Layer nodes	`context.layer_nodes` (line 117)	`context.uko_layer_nodes` (line 123)
Validation errors	`context.validation_errors` (line 129)	`context.uko_validation_errors` (line 163)
Ontology	`context.ontology` (line 38)	`context.uko_ontology` (line 155)

The new step step_when_get_all_layer_nodes (line 120-123) is functionally identical to step_when_get_layer (line 114-117) — both call loader.get_layer_nodes() — but store results in different attributes. This creates maintenance confusion and increases the risk of reading the wrong attribute in a future step.

T-4. Hardcoded Node Counts Coupled to Non-Spec TTL

File: features/uko_ontology.feature:165-170

Examples:
  | layer | count |
  | 0     | 4     |
  | 1     | 3     |
  | 2     | 3     |
  | 3     | 3     |

These exact counts are tightly coupled to the current TTL file that diverges from the spec (see C-2, C-3). When the TTL is corrected to match the specification (Layer 0 = 5 classes, Layer 1 = many more domain classes), every row in this table will break. The older there should be at least 4 nodes assertion (line 157) is more resilient to ontology evolution.

DESIGN ISSUES

D-1. Validator Assumes Closed-World (All Parents Internal)

File: uko_loader.py:196-201

Standard RDF ontologies commonly reference classes from external vocabularies (owl:Thing, rdfs:Resource, etc.). The new parent-existence check treats any reference to a class not defined within the same TTL file as an error. This closed-world assumption is incompatible with RDF best practices and will need to be relaxed when the ontology grows to reference standard vocabularies.

D-2. `rdfs:subPropertyOf` Not Handled

File: uko_loader.py

The spec's TTL definitions use rdfs:subPropertyOf for property inheritance (e.g., uko-oo:inheritsFrom rdfs:subPropertyOf uko:dependsOn). The parser extracts rdfs:domain, rdfs:range, and rdfs:subClassOf but ignores rdfs:subPropertyOf. When the ontology adds property hierarchies per the specification, this will need to be addressed.

MINOR / STYLE

M-1. Inconsistent Quoting in Error Messages

File: uko_loader.py:183-209

Older error messages omit quotes around URIs (f"Node {node.uri} missing rdf:type"), while the new messages added in this commit use quotes (f"Node '{node.uri}' references ..."). This should be consistent across all error strings.

M-2. Premature `ISSUES CLOSED: #189`

Both commits on this branch include ISSUES CLOSED: #189, but at least two subtasks from the issue's acceptance criteria remain unverified:

"Verify coverage >=97% via nox -s coverage_report"
"Run nox (all default sessions, including benchmark), fix any errors"

The issue should not be auto-closed until these are confirmed passing.

Recommendations

Align the TTL with the spec before merging — or document a deliberate deviation in an ADR. The current ontology structure is so different from the spec that it will require a near-total rewrite later.
Fix the validation logic at uko_loader.py:188-201 — add elif to avoid duplicate errors and exclude fully-resolved HTTP URIs from the prefix check.
Add test scenarios for rdfs:domain/range resolution and parent-URI existence validation — these are the two main code changes in this commit and neither has test coverage.
Consolidate step definitions — remove the duplicate step_when_get_all_layer_nodes / uko_layer_nodes and reuse the existing step_when_get_layer / layer_nodes pattern.
Do not close #189 until nox and coverage verification are completed per the issue's acceptance criteria.

# Code Review Report **Commit:** `8206fad` — `fix(uko): address review findings for UKO ontology scaffolding` **Author:** khyari hamza (Hamza Khyari) **Branch:** `feature/m6-acms-uko-schema` **Issue:** #189 — feat(uko): add UKO ontology scaffolding **Files changed:** 5 files, +96 / -4 --- ## Severity Summary | Severity | Count | |----------|-------| | Critical (Spec Mismatch) | 3 | | Bug | 1 | | Test Flaw | 4 | | Design Issue | 2 | | Minor / Style | 2 | --- ## CRITICAL — Specification Mismatches ### C-1. Ontology URI Namespace Diverges from Specification **Files:** `docs/ontology/uko.ttl`, `src/cleveragents/application/services/uko_loader.py:42-47` The specification (`docs/specification.md`, ~line 40785-40800) defines the UKO namespace prefixes as **domain-based names**: | Spec Prefix | Spec IRI | |---|---| | `uko:` | `https://cleveragents.ai/ontology/uko#` | | `uko-code:` | `https://cleveragents.ai/ontology/uko/code#` | | `uko-oo:` | `https://cleveragents.ai/ontology/uko/oo#` | | `uko-py:` | `https://cleveragents.ai/ontology/uko/py#` | The implementation uses a completely different naming convention — **numeric layer IDs**: | Impl Prefix | Impl IRI | |---|---| | `uko0:` | `https://ontology.cleverthis.com/uko/layer0/` | | `uko1:` | `https://ontology.cleverthis.com/uko/layer1/` | | `uko2:` | `https://ontology.cleverthis.com/uko/layer2/` | | `uko3:` | `https://ontology.cleverthis.com/uko/layer3/` | Both the base domain (`cleveragents.ai` vs `ontology.cleverthis.com`) and the naming strategy (semantic domain names vs numeric indices) differ. The `_LAYER_PREFIXES` dict in `uko_loader.py:42-47` hardcodes the numeric scheme. This will require a complete rewrite when aligning with the specification. ### C-2. Layer 0 Class Hierarchy Does Not Match Specification **File:** `docs/ontology/uko.ttl:17-35` The specification defines **five** Layer 0 base classes following an information-theory model: - `uko:InformationUnit` (root) - `uko:Container` (subClassOf InformationUnit) - `uko:Atom` (subClassOf InformationUnit) - `uko:Annotation` (subClassOf InformationUnit) - `uko:Boundary` (subClassOf InformationUnit) The implementation defines **four** classes following an artifact-centric model: - `uko0:Resource` (root) - `uko0:Artifact` (subClassOf Resource) - `uko0:CodeArtifact` (subClassOf Artifact) - `uko0:Document` (subClassOf Artifact) These are fundamentally different ontologies. The spec's Layer 0 is a general-purpose information classification system, while the implementation's Layer 0 is a software-artifact taxonomy. The test in `features/uko_ontology.feature:167` hardcodes `count: 4` for Layer 0, cementing this divergence. ### C-3. Layers 1-3 Structure Does Not Match Specification **File:** `docs/ontology/uko.ttl:37-80` | Layer | Specification | Implementation | |---|---|---| | **1** | Four domains: Software (`Module`, `Callable`, `TypeDefinition`), Documents, Data, Infrastructure | Three code-only classes: `Module`, `Function`, `Class` | | **2** | Paradigm specializations: OO (`Class`, `Interface`, `Method`), Functional, Procedural | Three relationship properties: `imports`, `defines`, `inherits` | | **3** | Language-specific extensions: Python, TypeScript, Rust, Java | Three Python classes only: `PythonModule`, `PythonClass`, `PythonFunction` | Notable: the spec places `Class` at Layer 2 (OO paradigm) but the implementation puts it at Layer 1. The spec's Layer 2 contains **classes** (OO specializations), while the implementation's Layer 2 contains only **ObjectProperty** relationships. The spec uses `Callable` (Layer 1) and `Method` (Layer 2); the implementation uses `Function` (Layer 1) which has no spec equivalent at that layer. --- ## BUG ### B-1. Validation Produces False-Positive "Undefined Prefix" for Resolved HTTP URIs **File:** `src/cleveragents/application/services/uko_loader.py:186-201` ```python if node.parent_uri: resolved_parent = node.parent_uri if resolved_parent not in node_uris and ":" in resolved_parent: # line 188 prefix_part = resolved_parent.split(":")[0] if prefix_part not in known_prefixes: errors.append(...) # "undefined prefix" # NEW CHECK (this commit) if resolved_parent not in node_uris: # line 197 errors.append(...) # "non-existent parent" ``` Two problems: 1. **False prefix error on external URIs.** If `parent_uri` points to an external ontology class (e.g., `owl:Thing` which resolves to `http://www.w3.org/2002/07/owl#Thing`), then `":" in resolved_parent` is `True` (because of `http:`), `prefix_part` becomes `"http"`, and `"http" not in known_prefixes` is `True`. This produces a spurious *"undefined prefix 'http:'"* error. While the current TTL avoids this scenario, it violates standard RDF practice where cross-ontology references are common. 2. **Duplicate errors.** The new existence check at line 197 is *not* guarded by `elif`, so both the prefix error *and* the non-existent parent error fire for the same node. This clutters the error output with redundant messages for a single root cause. --- ## TEST FLAWS ### T-1. No Test for `rdfs:domain`/`rdfs:range` URI Resolution **Impact:** The primary code fix in this commit The commit message states *"Resolve rdfs:domain and rdfs:range values to full URIs in parser"* (`uko_loader.py:125-135`), yet **no Behave scenario or step verifies this behavior**. There is no assertion checking that `node.properties["rdfs:domain"]` contains a fully resolved URI rather than a prefixed name. If this resolution logic regresses, no test will catch it. ### T-2. No Test for Parent-URI Existence Validation **Impact:** New validation rule at `uko_loader.py:196-201` The commit adds a new validation check that rejects nodes whose `parent_uri` references a non-existent node. But no test scenario exercises this path. The new "Validate rejects ontology with typeless node" test only verifies missing `rdf:type`, not missing parent references. ### T-3. Duplicate/Inconsistent Context Attributes in Step Definitions **File:** `features/steps/uko_ontology_steps.py` The commit introduces parallel naming conventions for the same concepts: | Pattern | Original | New (this commit) | |---|---|---| | Layer nodes | `context.layer_nodes` (line 117) | `context.uko_layer_nodes` (line 123) | | Validation errors | `context.validation_errors` (line 129) | `context.uko_validation_errors` (line 163) | | Ontology | `context.ontology` (line 38) | `context.uko_ontology` (line 155) | The new step `step_when_get_all_layer_nodes` (line 120-123) is functionally identical to `step_when_get_layer` (line 114-117) — both call `loader.get_layer_nodes()` — but store results in different attributes. This creates maintenance confusion and increases the risk of reading the wrong attribute in a future step. ### T-4. Hardcoded Node Counts Coupled to Non-Spec TTL **File:** `features/uko_ontology.feature:165-170` ```gherkin Examples: | layer | count | | 0 | 4 | | 1 | 3 | | 2 | 3 | | 3 | 3 | ``` These exact counts are tightly coupled to the current TTL file that diverges from the spec (see C-2, C-3). When the TTL is corrected to match the specification (Layer 0 = 5 classes, Layer 1 = many more domain classes), every row in this table will break. The older `there should be at least 4 nodes` assertion (line 157) is more resilient to ontology evolution. --- ## DESIGN ISSUES ### D-1. Validator Assumes Closed-World (All Parents Internal) **File:** `uko_loader.py:196-201` Standard RDF ontologies commonly reference classes from external vocabularies (`owl:Thing`, `rdfs:Resource`, etc.). The new parent-existence check treats any reference to a class not defined within the same TTL file as an error. This closed-world assumption is incompatible with RDF best practices and will need to be relaxed when the ontology grows to reference standard vocabularies. ### D-2. `rdfs:subPropertyOf` Not Handled **File:** `uko_loader.py` The spec's TTL definitions use `rdfs:subPropertyOf` for property inheritance (e.g., `uko-oo:inheritsFrom rdfs:subPropertyOf uko:dependsOn`). The parser extracts `rdfs:domain`, `rdfs:range`, and `rdfs:subClassOf` but ignores `rdfs:subPropertyOf`. When the ontology adds property hierarchies per the specification, this will need to be addressed. --- ## MINOR / STYLE ### M-1. Inconsistent Quoting in Error Messages **File:** `uko_loader.py:183-209` Older error messages omit quotes around URIs (`f"Node {node.uri} missing rdf:type"`), while the new messages added in this commit use quotes (`f"Node '{node.uri}' references ..."`). This should be consistent across all error strings. ### M-2. Premature `ISSUES CLOSED: #189` Both commits on this branch include `ISSUES CLOSED: #189`, but at least two subtasks from the issue's acceptance criteria remain unverified: - *"Verify coverage >=97% via `nox -s coverage_report`"* - *"Run `nox` (all default sessions, including benchmark), fix any errors"* The issue should not be auto-closed until these are confirmed passing. --- ## Recommendations 1. **Align the TTL with the spec** before merging — or document a deliberate deviation in an ADR. The current ontology structure is so different from the spec that it will require a near-total rewrite later. 2. **Fix the validation logic** at `uko_loader.py:188-201` — add `elif` to avoid duplicate errors and exclude fully-resolved HTTP URIs from the prefix check. 3. **Add test scenarios** for rdfs:domain/range resolution and parent-URI existence validation — these are the two main code changes in this commit and neither has test coverage. 4. **Consolidate step definitions** — remove the duplicate `step_when_get_all_layer_nodes` / `uko_layer_nodes` and reuse the existing `step_when_get_layer` / `layer_nodes` pattern. 5. **Do not close #189** until `nox` and coverage verification are completed per the issue's acceptance criteria.

freemo added the

labels 2026-03-01 04:36:07 +00:00

freemo referenced this pull request

2026-03-04 01:03:12 +00:00

feat(uko): add analyzer plugin framework and initial domain analyzers (Python, Markdown) #551

CoreRasurae commented

2026-03-04 15:48:11 +00:00

Code Review — Commit `c8df7e5` (fix(uko): add missing Layer 0 properties, fix validation and immutability)

Reviewed against: Issue #189 acceptance criteria, docs/specification.md (Section 14 / ACMS Architecture / UKO), and general code quality standards.

Scope: test coverage, test flaws, performance, bug detection, security, spec fidelity.

Findings

BUG-1 (Medium): UKO namespace prefix match is overly broad

File: src/cleveragents/application/services/uko_loader.py:227-245

The internal-URI validation check uses a prefix string without a trailing delimiter:

_uko_ns = "https://cleveragents.ai/ontology/uko"
...
if resolved_parent.startswith(_uko_ns) and resolved_parent not in node_uris:

"https://cleveragents.ai/ontology/ukobogus#Something".startswith(_uko_ns) evaluates to True. Any URI whose path happens to start with .../uko (e.g., a hypothetical ukobogus:, a typo like ukoo:, or any future external ontology whose IRI shares that prefix) would be incorrectly treated as an internal UKO URI and then rejected as non-existent — a false-positive validation error.

Suggested fix — guard with a delimiter check:

if (
    (resolved_parent.startswith(_uko_ns + "#") or resolved_parent.startswith(_uko_ns + "/"))
    and resolved_parent not in node_uris
):

BUG-2 (Low): `_extract_version_string` fix has no dedicated test

File: src/cleveragents/application/services/uko_loader.py:515-527

The old code returned the raw last path segment for non-semver IRIs (e.g., https://cleveragents.ai/ontology/uko would return "uko"). The fix now correctly returns "".

However, the only new test for version rejection uses version IRI 99.0.0, which is a valid semver string — it just isn't in SUPPORTED_VERSIONS. That test exercises the validate() version-check, not the _extract_version_string code path. There is no test that sends a version IRI with a non-semver last segment (e.g., <https://cleveragents.ai/ontology/uko> or <https://example.com/foo/bar>) and verifies the loader rejects it with "Unsupported version" and version "".

SPEC-1 (Medium): `rdfs:comment` values truncated relative to specification

File: docs/ontology/uko.ttl:69-87

Three content property comments are shortened compared to the specification (docs/specification.md lines ~40847-40861):

Property	Missing from TTL comment
`hasRendering`	"Multiple renderings at different depths may exist for the same node. The depth is indicated by the associated uko:renderingDepth value."
`renderingDepth`	"Paired with uko:hasRendering via a blank node or reification."
`hasFullContent`	"Equivalent to uko:hasRendering at the domain's maximum depth."

The truncated portions carry important semantic constraints (multiplicity, pairing via blank node/reification, equivalence to max-depth rendering) that downstream consumers of the ontology may rely on. The TTL should mirror the specification comments verbatim.

TEST-1 (Medium): No BDD test for `rdfs:domain`/`rdfs:range` correctness of new properties

File: features/uko_ontology.feature:230-257

The three new scenarios verify that the 10 new properties exist as nodes with the correct labels. However, none of them verify:

That rdfs:domain is correctly resolved to uko:InformationUnit for each property.
That rdfs:range is correctly set (e.g., xsd:string for hasRendering, xsd:dateTime for validFrom, xsd:boolean for isCurrent, uko:InformationUnit for isRevisionOf).
That sourceResource (an owl:ObjectProperty) has no rdfs:range — matching the spec.

The existing @uko_domain_range scenarios only test contains and inheritsFrom. A similar pattern should cover the 10 new properties to ensure they match the specification.

TEST-2 (Low): No test for duplicate-key handling after `properties` type change

File: src/cleveragents/domain/models/core/uko.py:31, features/steps/uko_ontology_steps.py:355

The properties field was changed from dict[str, str] to tuple[tuple[str, str], ...]. The step code now does prop_dict = dict(node.properties) which silently drops duplicates (keeps last value). While the current loader code cannot produce duplicates (it populates from a dict), a future TTL extension with multi-valued predicates (valid in RDF) could introduce them. A test asserting the expected lookup behavior for duplicate keys would prevent silent data loss.

SPEC-2 (Low): Commit message references wrong specification line numbers

The commit body says "per specification lines 41909-41956", but the actual content/provenance/temporal property definitions are at approximately lines 40847-40894 in docs/specification.md. Minor traceability issue that could confuse future reviewers.

PERF-1 (Informational): `get_layer_nodes` does a linear scan on every call

File: src/cleveragents/application/services/uko_loader.py:317-323

def get_layer_nodes(self, ontology: UKOOntology, layer: int) -> list[UKONode]:
    return [n for n in ontology.nodes if n.layer == layer]

This is O(n) per call. For the current 32-node ontology it is negligible. As the spec envisions projects with 10,000+ indexed files (milestone v3.4.0 success criteria), precomputing a dict[int, list[UKONode]] index during load would be more efficient. Not a blocker now but worth a # TODO for the scaling milestone.

Positive Notes

The dict → tuple[tuple[str,str],...] change for properties correctly enforces deep immutability on a frozen=True model.
The parent-URI validation fix closes a real gap where fully-resolved internal URIs were silently accepted.
The _extract_version_string returning "" for non-semver segments prevents confusing error messages.
Ruff clean, Pyright 0 errors on the loader.
BDD scenario count (28), Robot tests (4), and benchmark suites are all present per issue subtasks.

Severity Summary

ID	Severity	Category	Action needed
BUG-1	Medium	Bug	Fix namespace prefix to include delimiter
BUG-2	Low	Test gap	Add test for non-semver version IRI
SPEC-1	Medium	Spec fidelity	Update TTL comments to match spec verbatim
TEST-1	Medium	Test gap	Add domain/range tests for new properties
TEST-2	Low	Test gap	Add duplicate-key behavior test
SPEC-2	Low	Documentation	Fix spec line numbers in commit body
PERF-1	Info	Performance	Consider layer index for future scaling

## Code Review — Commit `c8df7e5` (fix(uko): add missing Layer 0 properties, fix validation and immutability) **Reviewed against**: Issue #189 acceptance criteria, `docs/specification.md` (Section 14 / ACMS Architecture / UKO), and general code quality standards. **Scope**: test coverage, test flaws, performance, bug detection, security, spec fidelity. --- ### Findings #### BUG-1 (Medium): UKO namespace prefix match is overly broad **File**: `src/cleveragents/application/services/uko_loader.py:227-245` The internal-URI validation check uses a prefix string without a trailing delimiter: ```python _uko_ns = "https://cleveragents.ai/ontology/uko" ... if resolved_parent.startswith(_uko_ns) and resolved_parent not in node_uris: ``` `"https://cleveragents.ai/ontology/ukobogus#Something".startswith(_uko_ns)` evaluates to **`True`**. Any URI whose path happens to start with `.../uko` (e.g., a hypothetical `ukobogus:`, a typo like `ukoo:`, or any future external ontology whose IRI shares that prefix) would be incorrectly treated as an internal UKO URI and then rejected as non-existent — a false-positive validation error. **Suggested fix** — guard with a delimiter check: ```python if ( (resolved_parent.startswith(_uko_ns + "#") or resolved_parent.startswith(_uko_ns + "/")) and resolved_parent not in node_uris ): ``` --- #### BUG-2 (Low): `_extract_version_string` fix has no dedicated test **File**: `src/cleveragents/application/services/uko_loader.py:515-527` The old code returned the raw last path segment for non-semver IRIs (e.g., `https://cleveragents.ai/ontology/uko` would return `"uko"`). The fix now correctly returns `""`. However, the only new test for version rejection uses version IRI `99.0.0`, which **is** a valid semver string — it just isn't in `SUPPORTED_VERSIONS`. That test exercises the `validate()` version-check, not the `_extract_version_string` code path. There is **no test** that sends a version IRI with a non-semver last segment (e.g., `<https://cleveragents.ai/ontology/uko>` or `<https://example.com/foo/bar>`) and verifies the loader rejects it with "Unsupported version" and version `""`. --- #### SPEC-1 (Medium): `rdfs:comment` values truncated relative to specification **File**: `docs/ontology/uko.ttl:69-87` Three content property comments are shortened compared to the specification (`docs/specification.md` lines ~40847-40861): | Property | Missing from TTL comment | |---|---| | `hasRendering` | "Multiple renderings at different depths may exist for the same node. The depth is indicated by the associated uko:renderingDepth value." | | `renderingDepth` | "Paired with uko:hasRendering via a blank node or reification." | | `hasFullContent` | "Equivalent to uko:hasRendering at the domain's maximum depth." | The truncated portions carry important semantic constraints (multiplicity, pairing via blank node/reification, equivalence to max-depth rendering) that downstream consumers of the ontology may rely on. The TTL should mirror the specification comments verbatim. --- #### TEST-1 (Medium): No BDD test for `rdfs:domain`/`rdfs:range` correctness of new properties **File**: `features/uko_ontology.feature:230-257` The three new scenarios verify that the 10 new properties exist as nodes with the correct labels. However, none of them verify: - That `rdfs:domain` is correctly resolved to `uko:InformationUnit` for each property. - That `rdfs:range` is correctly set (e.g., `xsd:string` for `hasRendering`, `xsd:dateTime` for `validFrom`, `xsd:boolean` for `isCurrent`, `uko:InformationUnit` for `isRevisionOf`). - That `sourceResource` (an `owl:ObjectProperty`) has no `rdfs:range` — matching the spec. The existing `@uko_domain_range` scenarios only test `contains` and `inheritsFrom`. A similar pattern should cover the 10 new properties to ensure they match the specification. --- #### TEST-2 (Low): No test for duplicate-key handling after `properties` type change **File**: `src/cleveragents/domain/models/core/uko.py:31`, `features/steps/uko_ontology_steps.py:355` The `properties` field was changed from `dict[str, str]` to `tuple[tuple[str, str], ...]`. The step code now does `prop_dict = dict(node.properties)` which silently drops duplicates (keeps last value). While the current loader code cannot produce duplicates (it populates from a `dict`), a future TTL extension with multi-valued predicates (valid in RDF) could introduce them. A test asserting the expected lookup behavior for duplicate keys would prevent silent data loss. --- #### SPEC-2 (Low): Commit message references wrong specification line numbers The commit body says "per specification lines 41909-41956", but the actual content/provenance/temporal property definitions are at approximately lines 40847-40894 in `docs/specification.md`. Minor traceability issue that could confuse future reviewers. --- #### PERF-1 (Informational): `get_layer_nodes` does a linear scan on every call **File**: `src/cleveragents/application/services/uko_loader.py:317-323` ```python def get_layer_nodes(self, ontology: UKOOntology, layer: int) -> list[UKONode]: return [n for n in ontology.nodes if n.layer == layer] ``` This is O(n) per call. For the current 32-node ontology it is negligible. As the spec envisions projects with 10,000+ indexed files (milestone v3.4.0 success criteria), precomputing a `dict[int, list[UKONode]]` index during load would be more efficient. Not a blocker now but worth a `# TODO` for the scaling milestone. --- ### Positive Notes - The `dict` → `tuple[tuple[str,str],...]` change for `properties` correctly enforces deep immutability on a `frozen=True` model. - The parent-URI validation fix closes a real gap where fully-resolved internal URIs were silently accepted. - The `_extract_version_string` returning `""` for non-semver segments prevents confusing error messages. - Ruff clean, Pyright 0 errors on the loader. - BDD scenario count (28), Robot tests (4), and benchmark suites are all present per issue subtasks. --- ### Severity Summary | ID | Severity | Category | Action needed | |----|----------|----------|---------------| | BUG-1 | Medium | Bug | Fix namespace prefix to include delimiter | | BUG-2 | Low | Test gap | Add test for non-semver version IRI | | SPEC-1 | Medium | Spec fidelity | Update TTL comments to match spec verbatim | | TEST-1 | Medium | Test gap | Add domain/range tests for new properties | | TEST-2 | Low | Test gap | Add duplicate-key behavior test | | SPEC-2 | Low | Documentation | Fix spec line numbers in commit body | | PERF-1 | Info | Performance | Consider layer index for future scaling |

CoreRasurae force-pushed feature/m6-acms-uko-schema from c8df7e58c6 to 41254ccffc

2026-03-04 20:03:38 +00:00

Compare

brent.edwards requested changes 2026-03-04 20:06:26 +00:00

Dismissed

brent.edwards left a comment

Thanks for the UKO scaffolding work. I found several blocking issues before this can merge.

Good

UKO TTL skeleton is clearly layered with prefixes and class/property scaffolding (docs/ontology/uko.ttl).
UKO loader provides prefix parsing, inheritance resolution, and validation with cycle detection (src/cleveragents/application/services/uko_loader.py).
UKO domain models are clean, frozen Pydantic v2 value objects (src/cleveragents/domain/models/core/uko.py).

Needs attention

P1:must-fix – PR scope mismatch: this branch deletes/rewrites a very large portion of the repo (585 files changed, ~100k deletions), far beyond the UKO feature described. Examples include removals of migrations (alembic/versions/m6_001_checkpoint_metadata_table.py), services (src/cleveragents/application/services/checkpoint_service.py), specs/docs (docs/specification.md), tests (features/*, robot/*), and CI config. Please revert unrelated deletions and keep this PR focused on UKO scaffolding only.
P1:must-fix – Spec-first violation: docs/specification.md is massively reduced/edited in this PR without an ADR or clear alignment to the stated architecture. If this is intentional, it requires a dedicated ADR and a separate PR. Otherwise, revert the spec changes.
P1:must-fix – PR process violation: CONTRIBUTING requires PRs to have exactly one Type/ label and no State/, Priority/, or MoSCoW/ labels. This PR currently has extra labels (e.g., State/Verified, Priority/Medium, MoSCoW/Should have). Please fix the labels before merge.
P1:must-fix – Removal of benchmarks/tests/ADR docs appears to break required quality gates (BDD/Robot/ASV coverage) and violates the “tests at multiple levels” requirement. If those removals are deliberate, they need a separate, scoped justification and replacement coverage in a dedicated PR.

Given the P1 items, I’m marking this as request changes. Happy to re-review once the PR is reduced to the UKO changes and the process issues are corrected.

Thanks for the UKO scaffolding work. I found several blocking issues before this can merge. Good - UKO TTL skeleton is clearly layered with prefixes and class/property scaffolding (`docs/ontology/uko.ttl`). - UKO loader provides prefix parsing, inheritance resolution, and validation with cycle detection (`src/cleveragents/application/services/uko_loader.py`). - UKO domain models are clean, frozen Pydantic v2 value objects (`src/cleveragents/domain/models/core/uko.py`). Needs attention - P1:must-fix – PR scope mismatch: this branch deletes/rewrites a very large portion of the repo (585 files changed, ~100k deletions), far beyond the UKO feature described. Examples include removals of migrations (`alembic/versions/m6_001_checkpoint_metadata_table.py`), services (`src/cleveragents/application/services/checkpoint_service.py`), specs/docs (`docs/specification.md`), tests (`features/*`, `robot/*`), and CI config. Please revert unrelated deletions and keep this PR focused on UKO scaffolding only. - P1:must-fix – Spec-first violation: `docs/specification.md` is massively reduced/edited in this PR without an ADR or clear alignment to the stated architecture. If this is intentional, it requires a dedicated ADR and a separate PR. Otherwise, revert the spec changes. - P1:must-fix – PR process violation: CONTRIBUTING requires PRs to have exactly one `Type/` label and no `State/`, `Priority/`, or `MoSCoW/` labels. This PR currently has extra labels (e.g., `State/Verified`, `Priority/Medium`, `MoSCoW/Should have`). Please fix the labels before merge. - P1:must-fix – Removal of benchmarks/tests/ADR docs appears to break required quality gates (BDD/Robot/ASV coverage) and violates the “tests at multiple levels” requirement. If those removals are deliberate, they need a separate, scoped justification and replacement coverage in a dedicated PR. Given the P1 items, I’m marking this as request changes. Happy to re-review once the PR is reduced to the UKO changes and the process issues are corrected.

CoreRasurae removed the

labels 2026-03-04 20:39:56 +00:00

CoreRasurae force-pushed feature/m6-acms-uko-schema from 41254ccffc to 9f685ee9bb

2026-03-04 20:55:10 +00:00

Compare

brent.edwards approved these changes 2026-03-04 22:14:34 +00:00

Dismissed

brent.edwards left a comment

Approve.

CoreRasurae force-pushed feature/m6-acms-uko-schema from 9f685ee9bb to ad53a659de

2026-03-04 22:40:45 +00:00

Compare

CoreRasurae dismissed brent.edwards's review 2026-03-04 22:40:46 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-03-04 22:41:06 +00:00

CoreRasurae merged commit ad53a659de into master

2026-03-04 23:00:37 +00:00

CoreRasurae deleted branch feature/m6-acms-uko-schema

2026-03-04 23:00:37 +00:00

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: cleveragents/cleveragents-core#466

feat(uko): add UKO ontology scaffolding #466

Summary

Testing

Files Changed

Code Review Report

Severity Summary

CRITICAL — Specification Mismatches

C-1. Ontology URI Namespace Diverges from Specification

C-2. Layer 0 Class Hierarchy Does Not Match Specification

C-3. Layers 1-3 Structure Does Not Match Specification

BUG

B-1. Validation Produces False-Positive "Undefined Prefix" for Resolved HTTP URIs

TEST FLAWS

T-1. No Test for rdfs:domain/rdfs:range URI Resolution

T-2. No Test for Parent-URI Existence Validation

T-3. Duplicate/Inconsistent Context Attributes in Step Definitions

T-4. Hardcoded Node Counts Coupled to Non-Spec TTL

DESIGN ISSUES

D-1. Validator Assumes Closed-World (All Parents Internal)

D-2. rdfs:subPropertyOf Not Handled

MINOR / STYLE

M-1. Inconsistent Quoting in Error Messages

M-2. Premature ISSUES CLOSED: #189

Recommendations

Code Review — Commit c8df7e5 (fix(uko): add missing Layer 0 properties, fix validation and immutability)

Findings

BUG-1 (Medium): UKO namespace prefix match is overly broad

BUG-2 (Low): _extract_version_string fix has no dedicated test

SPEC-1 (Medium): rdfs:comment values truncated relative to specification

TEST-1 (Medium): No BDD test for rdfs:domain/rdfs:range correctness of new properties

TEST-2 (Low): No test for duplicate-key handling after properties type change

SPEC-2 (Low): Commit message references wrong specification line numbers

PERF-1 (Informational): get_layer_nodes does a linear scan on every call

Positive Notes

Severity Summary

T-1. No Test for `rdfs:domain`/`rdfs:range` URI Resolution

D-2. `rdfs:subPropertyOf` Not Handled

M-2. Premature `ISSUES CLOSED: #189`

Code Review — Commit `c8df7e5` (fix(uko): add missing Layer 0 properties, fix validation and immutability)

BUG-2 (Low): `_extract_version_string` fix has no dedicated test

SPEC-1 (Medium): `rdfs:comment` values truncated relative to specification

TEST-1 (Medium): No BDD test for `rdfs:domain`/`rdfs:range` correctness of new properties

TEST-2 (Low): No test for duplicate-key handling after `properties` type change

PERF-1 (Informational): `get_layer_nodes` does a linear scan on every call