cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

feat(acms): implement UKO Layer 2 Paradigm Vocabularies (uko-oo, uko-func, uko-proc) #657

Merged
hamza.khyari merged 1 commits from feature/m6-uko-layer2-paradigm-vocabularies into master 2026-03-13 18:28:12 +00:00
Conversation 14 Commits 1 Files Changed 20
+3894 -130
hamza.khyari commented 2026-03-10 00:27:54 +00:00
Member
Copy Link
Copy Source

Summary

Implements Forgejo issue #575 -- UKO Layer 2 Paradigm Vocabularies (uko-oo, uko-func, uko-proc) for the ACMS subsystem.

Motivation

Layer 2 paradigm vocabularies specialize the Layer 1 uko-code: domain ontology with paradigm-specific semantic classes. Object-oriented code needs class/interface/method/attribute hierarchy concepts, functional code needs pure functions/type classes/monads, and procedural code needs function declarations/global variables/headers/structs/macros. Each paradigm also extends the DetailLevelMap with paradigm-specific named levels (e.g., CLASS_HIERARCHY for OO). Without these, the ACMS context pipeline cannot produce paradigm-aware code summaries.

Changes

Ontology (docs/ontology/uko.ttl)

  • Added uko-func: and uko-proc: namespace prefixes
  • Added 8 new OWL classes: PureFunction, TypeClass, Monad (functional); ProceduralFunction, GlobalVariable, HeaderFile, StructDefinition, Macro (procedural)

Python Models (src/cleveragents/acms/uko/)

  • vocabularies.py (494 lines) -- VocabularyClass, VocabularyProperty, ParadigmVocabulary, VocabularyRegistry (frozen Pydantic models with prefix/IRI lookup and duplicate protection)
  • detail_level_maps.py (277 lines) -- DetailLevelMapBuilder with insertion/reassignment logic; pre-built maps: CODE_DETAIL_LEVEL_MAP, OO_DETAIL_LEVEL_MAP, FUNC_DETAIL_LEVEL_MAP, PROC_DETAIL_LEVEL_MAP
  • __init__.py / acms/__init__.py -- Package re-exports with sorted __all__

DetailLevelMap Inheritance

  • OO map inserts CLASS_HIERARCHY at depth 3 and VISIBILITY_ANNOTATED at depth 7, shifting FULL_SOURCE from 9 to 11 (12 total levels) -- matches spec lines 24991-25004
  • Functional and procedural maps insert paradigm-specific levels per spec

Tests

  • Behave: features/acms/uko_layer2_paradigm_vocabularies.feature -- 81 scenarios, 286 steps, 0 failures
  • Robot helper: robot/helper_uko_layer2_paradigm.py -- 5 subcommands, all pass
  • CHANGELOG.md updated

Documentation

  • docs/reference/uko_layer2_paradigm_vocabularies.md -- Reference documentation

Automated Check Results

Check Result
Lint (ruff) PASS -- 0 errors
Typecheck (pyright) PASS -- 0 errors, 0 warnings
Banned patterns (type:ignore, print) PASS -- none found
File size (all src < 500 lines) PASS -- max 494 lines
Security (no eval/exec/secrets/unbounded) PASS
Behave tests PASS -- 81 scenarios, 286 steps, 0 failures
Robot helpers PASS -- all 5 subcommands ok
Module exports PASS -- all symbols importable

Diff Coverage

Source File New Lines Covered Coverage
vocabularies.py 139 139 100%
detail_level_maps.py 78 78 100%
acms/uko/__init__.py 5 5 100%
acms/__init__.py 4 4 100%
domain/models/acms/crp.py (delta) 14 14 100%
TOTAL 240 240 100%

Spec Reference

docs/specification.md lines ~42333-42422 (Layer 2: Paradigm / Format Specializations)

Closes #575

## Summary Implements Forgejo issue #575 -- UKO Layer 2 Paradigm Vocabularies (`uko-oo`, `uko-func`, `uko-proc`) for the ACMS subsystem. ## Motivation Layer 2 paradigm vocabularies specialize the Layer 1 `uko-code:` domain ontology with paradigm-specific semantic classes. Object-oriented code needs class/interface/method/attribute hierarchy concepts, functional code needs pure functions/type classes/monads, and procedural code needs function declarations/global variables/headers/structs/macros. Each paradigm also extends the DetailLevelMap with paradigm-specific named levels (e.g., CLASS_HIERARCHY for OO). Without these, the ACMS context pipeline cannot produce paradigm-aware code summaries. ## Changes ### Ontology (`docs/ontology/uko.ttl`) - Added `uko-func:` and `uko-proc:` namespace prefixes - Added 8 new OWL classes: PureFunction, TypeClass, Monad (functional); ProceduralFunction, GlobalVariable, HeaderFile, StructDefinition, Macro (procedural) ### Python Models (`src/cleveragents/acms/uko/`) - **`vocabularies.py`** (494 lines) -- VocabularyClass, VocabularyProperty, ParadigmVocabulary, VocabularyRegistry (frozen Pydantic models with prefix/IRI lookup and duplicate protection) - **`detail_level_maps.py`** (277 lines) -- DetailLevelMapBuilder with insertion/reassignment logic; pre-built maps: CODE_DETAIL_LEVEL_MAP, OO_DETAIL_LEVEL_MAP, FUNC_DETAIL_LEVEL_MAP, PROC_DETAIL_LEVEL_MAP - **`__init__.py`** / **`acms/__init__.py`** -- Package re-exports with sorted `__all__` ### DetailLevelMap Inheritance - OO map inserts CLASS_HIERARCHY at depth 3 and VISIBILITY_ANNOTATED at depth 7, shifting FULL_SOURCE from 9 to 11 (12 total levels) -- matches spec lines 24991-25004 - Functional and procedural maps insert paradigm-specific levels per spec ### Tests - **Behave**: `features/acms/uko_layer2_paradigm_vocabularies.feature` -- 81 scenarios, 286 steps, 0 failures - **Robot helper**: `robot/helper_uko_layer2_paradigm.py` -- 5 subcommands, all pass - **CHANGELOG.md** updated ### Documentation - `docs/reference/uko_layer2_paradigm_vocabularies.md` -- Reference documentation ## Automated Check Results | Check | Result | |-------|--------| | Lint (ruff) | PASS -- 0 errors | | Typecheck (pyright) | PASS -- 0 errors, 0 warnings | | Banned patterns (type:ignore, print) | PASS -- none found | | File size (all src < 500 lines) | PASS -- max 494 lines | | Security (no eval/exec/secrets/unbounded) | PASS | | Behave tests | PASS -- 81 scenarios, 286 steps, 0 failures | | Robot helpers | PASS -- all 5 subcommands ok | | Module exports | PASS -- all symbols importable | ## Diff Coverage | Source File | New Lines | Covered | Coverage | |-------------|-----------|---------|----------| | `vocabularies.py` | 139 | 139 | 100% | | `detail_level_maps.py` | 78 | 78 | 100% | | `acms/uko/__init__.py` | 5 | 5 | 100% | | `acms/__init__.py` | 4 | 4 | 100% | | `domain/models/acms/crp.py` (delta) | 14 | 14 | 100% | | **TOTAL** | **240** | **240** | **100%** | ## Spec Reference `docs/specification.md` lines ~42333-42422 (Layer 2: Paradigm / Format Specializations) Closes #575
hamza.khyari self-assigned this 2026-03-10 00:45:39 +00:00
hamza.khyari added this to the v3.5.0 milestone 2026-03-10 00:46:44 +00:00
hamza.khyari added the
Type
Feature
Priority
Low
 labels 2026-03-10 00:47:33 +00:00
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from eee1c01fd0 to b0a7c1b8a6 2026-03-10 01:17:16 +00:00 Compare
hamza.khyari removed the
Priority
Low
 label 2026-03-10 01:21:53 +00:00
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from db04d3a114 to 995ad1daf3 2026-03-10 14:55:56 +00:00 Compare
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from 995ad1daf3 to 91efb508e0 2026-03-10 16:05:47 +00:00 Compare
brent.edwards requested changes 2026-03-10 23:51:13 +00:00
Dismissed
brent.edwards left a comment
Member
Copy Link
Copy Source

Exhaustive Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies

Reviewer: Brent Edwards
PR Size: XL (~3,118 lines across 18 files)
Review method: 10 parallel review passes (architecture, security, test quality, ontology, code quality, backward compatibility, CRP deep-dive, Semgrep scan, Pyright analysis, Ruff lint)

Overall Assessment

This is a well-structured PR with thorough test coverage (81 Behave scenarios, 286 steps), clean Semgrep results, and correct ontology modeling. The Python models are properly frozen, docstrings are thorough, and the DetailLevelMapBuilder pattern is sound. However, there are 1 P0 blocker, 4 P1 must-fix, 13 P2 should-fix, and 10 P3 nit findings that need attention before merge.

Per playbook escalation rules: The P0 requires a fix before merge. The multiple P1 findings in the CRP domain subsystem warrant escalation to the domain model owner (Luis).

P0:blocker (1) — Must fix before merge

H1. Robot helper imports VocabularyRegistry from wrong module
robot/helper_uko_layer2_paradigm.py:28 — VocabularyRegistry is imported from cleveragents.acms.uko.vocabularies, but it is defined in cleveragents.acms.uko.vocabulary_registry and is NOT exported from vocabularies.py. Every robot subcommand will crash at runtime with ImportError.

Fix:

from cleveragents.acms.uko.vocabularies import (
    get_func_vocabulary,
    get_oo_vocabulary,
    get_proc_vocabulary,
)
from cleveragents.acms.uko.vocabulary_registry import VocabularyRegistry

The same wrong import path appears in docs/reference/uko_layer2_paradigm_vocabularies.md:124-127 — users following the documentation will also get ImportError.

P1:must-fix (4) — Must fix before merge

H2. _freeze_levels model_validator does not re-run on field assignment, breaking the immutability invariant
crp.py:108-112 — DetailLevelMap uses validate_assignment=True (not frozen=True). The _freeze_levels logic is a model_validator(mode="after"), which runs at construction but does NOT re-run on field assignment in Pydantic v2. This means map.levels = {"FOO": 1} would bypass the freeze and set a plain dict, silently breaking the immutability contract.

Fix: Either (a) make DetailLevelMap frozen=True and keep register() using object.__setattr__, or (b) convert the freeze to a @field_validator("levels", mode="after") so it runs on every assignment trigger.

H3. MappingProxyType serialization is untested — model_dump_json() may fail
crp.py:86,111 — The levels field is annotated as dict[str, int] but holds MappingProxyType at runtime. Pydantic v2's Rust-based JSON serializer (pydantic-core) may not handle MappingProxyType correctly. No test in this PR calls model_dump() or model_dump_json() on a DetailLevelMap.

Fix: Add a test that roundtrips DetailLevelMap(...).model_dump_json() to confirm compatibility. If it fails, add a @field_serializer("levels") that converts to dict.

H4. levels field type annotation lies to the type checker
crp.py:86 — Field is dict[str, int] but runtime is MappingProxyType[str, int]. Pyright/mypy will believe .levels supports .update(), .pop(), [key]=val — all of which raise TypeError at runtime. Downstream code passing map.levels to functions expecting dict gets false confidence.

Fix: Annotate as Mapping[str, int] (from typing) which is truthful for both construction input and runtime type. Pydantic accepts dict input during construction either way.

H5. Unbounded list fields in ContextRequest allow memory exhaustion
crp.py:330-385 — Fields entities, uko_types, focus, preferred_strategies, and required_backends are all list[str] with no max_length. Since ContextRequest is the primary user-facing input model, a malformed request with millions of items could exhaust memory. Also applies to query: str (no max_length).

Fix: Add max_length constraints tuned to expected workloads (e.g., max_length=1000 for lists, max_length=10_000 for query).

P2:should-fix (13) — Fix in follow-up PR within 3 days

M1. Duplicated _validate_uri across VocabularyClass and VocabularyProperty
vocabularies.py:94-100 and vocabularies.py:144-150 — Character-for-character identical validators. Extract to a shared module-level helper _validate_http_uri(v: str) -> str.

M2. Inconsistent URI scheme validation — parent_uris, domain_uri, range_uri not validated
vocabularies.py:89,129-141 — Only the primary uri field enforces http(s):// scheme. Secondary URI fields (parent_uris, domain_uri, range_uri, sub_property_of) have no scheme check. If these reach an RDF serializer or HTTP client, malicious values like javascript: or file:/// could be injected.

M3. Near-identical _build_func_map and _build_proc_map + hardcoded max_depth=9
detail_level_maps.py:261-292 — These differ only in the domain string. Extract to _build_passthrough_map(domain: str). Also, max_depth=9 is hardcoded in 3 places instead of deriving from CODE_DETAIL_LEVEL_MAP.max_depth.

M4. No domain-specific exception types
crp.py:136, detail_level_maps.py:142-148, vocabulary_registry.py:67-72 — All error conditions raise bare ValueError. Define domain exceptions (UnknownDetailLevelError, DuplicateVocabularyError, etc.) as subclasses of ValueError for backward compatibility.

M5. resolve() has no cycle guard for circular parent chains
crp.py:129-136 — If a circular parent reference is introduced (e.g., A.parent=B, B.parent=A), resolve() infinite-recurses to RecursionError. Add a visited-set guard or max-depth parameter.

M6. register() is racy under concurrent access
crp.py:138-158 — Two threads calling register() concurrently both read dict(self.levels), add their entry, and write back — last-writer-wins race. Consider returning a new instance or adding a note that the method is not thread-safe.

M7. after_level not stripped in DetailLevelMapBuilder.insert_after
detail_level_maps.py:120-150 — new_level is stripped but after_level is not. If whitespace is present, the lookup fails with a confusing error.

M8. Missing test: VocabularyProperty with empty label/domain/range
Feature file has no scenario testing the min_length=1 validators on VocabularyProperty.label, domain_uri, and range_uri (only tests empty uri).

M9. Missing test: invalid URI scheme (non-HTTP)
No scenario tests that ftp:// or urn: URIs are rejected by _validate_uri. The validator code at vocabularies.py:94-100 would go undetected if removed.

M10. Missing test: insert_after at the tail of the level list
All insertion tests insert after middle levels. No test inserts after FULL_SOURCE (the last level), which exercises the boundary of ordered_names.insert(idx + 1, ...) at the end of the list.

M11. scoped_session change alters session-sharing semantics for R2 tests
features/steps/repositories_coverage_r2_steps.py:237-241 — Changing from sessionmaker to scoped_session(sessionmaker(...)) means repeated calls return the same session within a thread. Add .remove() in teardown and verify no existing R2 steps rely on independent sessions.

M12. Ontology: uko-oo:Class inherits contradictory archetypes (Atom + Container)
uko.ttl:150-153,556-559 — uko-code:TypeDefinition subclasses uko:Atom. Then uko-oo:Class subclasses both TypeDefinition (→Atom) and Container. If disjointness axioms are ever added to Layer 0, every uko-oo:Class individual becomes unsatisfiable. Add a comment documenting this design decision, or change TypeDefinition to subclass InformationUnit directly.

M13. CHANGELOG references issue #575 but not PR #657
CHANGELOG.md:17 — Clarify: (issue #575, PR #657) or just (#657) per project convention.

P3:nit (10) — Optional, author discretion

L1. vocabularies.py:139-142 — VocabularyProperty.sub_property_of uses "" default instead of None. Makes truthiness checks ambiguous.

L2. vocabularies.py:346,351 — Unnecessary parentheses around string literals in comment=("...").

L3. vocabularies.py:22-29 — __all__ omits public namespace constants (UKO_OO_PREFIX, UKO_OO_IRI, etc.). Either add them or prefix with _.

L4. vocabularies.py:270-271,333-334,372-373 — layer=2 and parent_domain="uko-code:" explicitly passed but are already model defaults. Add a comment # explicit for spec traceability or remove.

L5. detail_level_maps.py:64,251 + vocabularies.py:188,271,334,373 — String literal "uko-code:" repeated 6 times. Consider a named constant.

L6. crp.py:349-356 — ContextRequest.depth: int | str union makes every consumer do isinstance dispatch. Consider adding a resolve_depth(map) -> int convenience method.

L7. uko.ttl:9 — Unused uko-py: prefix declared but never used. Flagged as warning by TTL validators.

L8. Feature file — Inconsistent @frozen vs @vocab_frozen tagging (lines 40,67 vs 555). Running --tags=@frozen misses the ParadigmVocabulary test.

L9. acms/__init__.py:28-42 — __all__ is a manual copy of uko/__init__.py:38-52. Consider re-exporting programmatically or adding a sync comment.

L10. Step definitions — The try/except error-capture pattern is repeated 18 times across 4 step files. Consider extracting a capture_error(ctx, *exc_types) context manager.

Static Analysis Results

Tool Result
Semgrep (auto + p/python + p/security-audit) 0 findings — clean
Pyright 0 genuine errors (17 env-only errors from missing pydantic in /tmp)
Ruff 9 violations: 6x I001 (unsorted imports), 3x E402 (benchmark sys.path) — all auto-fixable

Checklist Verification

Architecture Check Result
Domain model invariants preserved PARTIAL — _freeze_levels has bypass risk (H2)
No circular dependencies PASS — verified one-way chain
New abstractions have clear SRP PASS
Public interfaces documented PASS — thorough docstrings
Type annotations complete PARTIAL — levels annotation lies (H4)
Error types domain-specific FAIL — bare ValueError everywhere (M4)
Backward-compatible PARTIAL — effective_levels() return type changed (H4), levels runtime type changed

Summary

Severity Count Action Required
P0:blocker 1 Fix before merge
P1:must-fix 4 Fix before merge
P2:should-fix 13 Fix in follow-up PR (3 day SLA)
P3:nit 10 Author discretion

Verdict: REQUEST_CHANGES — The P0 (wrong import) and P1 items (freeze invariant, serialization, type annotation, unbounded inputs) must be resolved before merge. The P2/P3 items can be addressed in a follow-up.

## Exhaustive Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies **Reviewer:** Brent Edwards **PR Size:** XL (~3,118 lines across 18 files) **Review method:** 10 parallel review passes (architecture, security, test quality, ontology, code quality, backward compatibility, CRP deep-dive, Semgrep scan, Pyright analysis, Ruff lint) --- ### Overall Assessment This is a well-structured PR with thorough test coverage (81 Behave scenarios, 286 steps), clean Semgrep results, and correct ontology modeling. The Python models are properly frozen, docstrings are thorough, and the `DetailLevelMapBuilder` pattern is sound. However, there are **1 P0 blocker**, **4 P1 must-fix**, **13 P2 should-fix**, and **10 P3 nit** findings that need attention before merge. **Per playbook escalation rules:** The P0 requires a fix before merge. The multiple P1 findings in the CRP domain subsystem warrant escalation to the domain model owner (Luis). --- ### P0:blocker (1) — Must fix before merge **H1. Robot helper imports `VocabularyRegistry` from wrong module** `robot/helper_uko_layer2_paradigm.py:28` — `VocabularyRegistry` is imported from `cleveragents.acms.uko.vocabularies`, but it is defined in `cleveragents.acms.uko.vocabulary_registry` and is NOT exported from `vocabularies.py`. Every robot subcommand will crash at runtime with `ImportError`. Fix: ```python from cleveragents.acms.uko.vocabularies import ( get_func_vocabulary, get_oo_vocabulary, get_proc_vocabulary, ) from cleveragents.acms.uko.vocabulary_registry import VocabularyRegistry ``` The same wrong import path appears in `docs/reference/uko_layer2_paradigm_vocabularies.md:124-127` — users following the documentation will also get `ImportError`. --- ### P1:must-fix (4) — Must fix before merge **H2. `_freeze_levels` model_validator does not re-run on field assignment, breaking the immutability invariant** `crp.py:108-112` — `DetailLevelMap` uses `validate_assignment=True` (not `frozen=True`). The `_freeze_levels` logic is a `model_validator(mode="after")`, which runs at construction but does NOT re-run on field assignment in Pydantic v2. This means `map.levels = {"FOO": 1}` would bypass the freeze and set a plain `dict`, silently breaking the immutability contract. Fix: Either (a) make `DetailLevelMap` `frozen=True` and keep `register()` using `object.__setattr__`, or (b) convert the freeze to a `@field_validator("levels", mode="after")` so it runs on every assignment trigger. **H3. `MappingProxyType` serialization is untested — `model_dump_json()` may fail** `crp.py:86,111` — The `levels` field is annotated as `dict[str, int]` but holds `MappingProxyType` at runtime. Pydantic v2's Rust-based JSON serializer (`pydantic-core`) may not handle `MappingProxyType` correctly. No test in this PR calls `model_dump()` or `model_dump_json()` on a `DetailLevelMap`. Fix: Add a test that roundtrips `DetailLevelMap(...).model_dump_json()` to confirm compatibility. If it fails, add a `@field_serializer("levels")` that converts to `dict`. **H4. `levels` field type annotation lies to the type checker** `crp.py:86` — Field is `dict[str, int]` but runtime is `MappingProxyType[str, int]`. Pyright/mypy will believe `.levels` supports `.update()`, `.pop()`, `[key]=val` — all of which raise `TypeError` at runtime. Downstream code passing `map.levels` to functions expecting `dict` gets false confidence. Fix: Annotate as `Mapping[str, int]` (from `typing`) which is truthful for both construction input and runtime type. Pydantic accepts `dict` input during construction either way. **H5. Unbounded list fields in `ContextRequest` allow memory exhaustion** `crp.py:330-385` — Fields `entities`, `uko_types`, `focus`, `preferred_strategies`, and `required_backends` are all `list[str]` with no `max_length`. Since `ContextRequest` is the primary user-facing input model, a malformed request with millions of items could exhaust memory. Also applies to `query: str` (no `max_length`). Fix: Add `max_length` constraints tuned to expected workloads (e.g., `max_length=1000` for lists, `max_length=10_000` for query). --- ### P2:should-fix (13) — Fix in follow-up PR within 3 days **M1. Duplicated `_validate_uri` across `VocabularyClass` and `VocabularyProperty`** `vocabularies.py:94-100` and `vocabularies.py:144-150` — Character-for-character identical validators. Extract to a shared module-level helper `_validate_http_uri(v: str) -> str`. **M2. Inconsistent URI scheme validation — `parent_uris`, `domain_uri`, `range_uri` not validated** `vocabularies.py:89,129-141` — Only the primary `uri` field enforces `http(s)://` scheme. Secondary URI fields (`parent_uris`, `domain_uri`, `range_uri`, `sub_property_of`) have no scheme check. If these reach an RDF serializer or HTTP client, malicious values like `javascript:` or `file:///` could be injected. **M3. Near-identical `_build_func_map` and `_build_proc_map` + hardcoded `max_depth=9`** `detail_level_maps.py:261-292` — These differ only in the domain string. Extract to `_build_passthrough_map(domain: str)`. Also, `max_depth=9` is hardcoded in 3 places instead of deriving from `CODE_DETAIL_LEVEL_MAP.max_depth`. **M4. No domain-specific exception types** `crp.py:136`, `detail_level_maps.py:142-148`, `vocabulary_registry.py:67-72` — All error conditions raise bare `ValueError`. Define domain exceptions (`UnknownDetailLevelError`, `DuplicateVocabularyError`, etc.) as subclasses of `ValueError` for backward compatibility. **M5. `resolve()` has no cycle guard for circular parent chains** `crp.py:129-136` — If a circular parent reference is introduced (e.g., `A.parent=B, B.parent=A`), `resolve()` infinite-recurses to `RecursionError`. Add a visited-set guard or max-depth parameter. **M6. `register()` is racy under concurrent access** `crp.py:138-158` — Two threads calling `register()` concurrently both read `dict(self.levels)`, add their entry, and write back — last-writer-wins race. Consider returning a new instance or adding a note that the method is not thread-safe. **M7. `after_level` not stripped in `DetailLevelMapBuilder.insert_after`** `detail_level_maps.py:120-150` — `new_level` is stripped but `after_level` is not. If whitespace is present, the lookup fails with a confusing error. **M8. Missing test: VocabularyProperty with empty label/domain/range** Feature file has no scenario testing the `min_length=1` validators on `VocabularyProperty.label`, `domain_uri`, and `range_uri` (only tests empty `uri`). **M9. Missing test: invalid URI scheme (non-HTTP)** No scenario tests that `ftp://` or `urn:` URIs are rejected by `_validate_uri`. The validator code at `vocabularies.py:94-100` would go undetected if removed. **M10. Missing test: `insert_after` at the tail of the level list** All insertion tests insert after middle levels. No test inserts after `FULL_SOURCE` (the last level), which exercises the boundary of `ordered_names.insert(idx + 1, ...)` at the end of the list. **M11. `scoped_session` change alters session-sharing semantics for R2 tests** `features/steps/repositories_coverage_r2_steps.py:237-241` — Changing from `sessionmaker` to `scoped_session(sessionmaker(...))` means repeated calls return the same session within a thread. Add `.remove()` in teardown and verify no existing R2 steps rely on independent sessions. **M12. Ontology: `uko-oo:Class` inherits contradictory archetypes (Atom + Container)** `uko.ttl:150-153,556-559` — `uko-code:TypeDefinition` subclasses `uko:Atom`. Then `uko-oo:Class` subclasses both `TypeDefinition` (→Atom) and `Container`. If disjointness axioms are ever added to Layer 0, every `uko-oo:Class` individual becomes unsatisfiable. Add a comment documenting this design decision, or change `TypeDefinition` to subclass `InformationUnit` directly. **M13. CHANGELOG references issue `#575` but not PR `#657`** `CHANGELOG.md:17` — Clarify: `(issue #575, PR #657)` or just `(#657)` per project convention. --- ### P3:nit (10) — Optional, author discretion **L1.** `vocabularies.py:139-142` — `VocabularyProperty.sub_property_of` uses `""` default instead of `None`. Makes truthiness checks ambiguous. **L2.** `vocabularies.py:346,351` — Unnecessary parentheses around string literals in `comment=("...")`. **L3.** `vocabularies.py:22-29` — `__all__` omits public namespace constants (`UKO_OO_PREFIX`, `UKO_OO_IRI`, etc.). Either add them or prefix with `_`. **L4.** `vocabularies.py:270-271,333-334,372-373` — `layer=2` and `parent_domain="uko-code:"` explicitly passed but are already model defaults. Add a comment `# explicit for spec traceability` or remove. **L5.** `detail_level_maps.py:64,251` + `vocabularies.py:188,271,334,373` — String literal `"uko-code:"` repeated 6 times. Consider a named constant. **L6.** `crp.py:349-356` — `ContextRequest.depth: int | str` union makes every consumer do isinstance dispatch. Consider adding a `resolve_depth(map) -> int` convenience method. **L7.** `uko.ttl:9` — Unused `uko-py:` prefix declared but never used. Flagged as warning by TTL validators. **L8.** Feature file — Inconsistent `@frozen` vs `@vocab_frozen` tagging (lines 40,67 vs 555). Running `--tags=@frozen` misses the ParadigmVocabulary test. **L9.** `acms/__init__.py:28-42` — `__all__` is a manual copy of `uko/__init__.py:38-52`. Consider re-exporting programmatically or adding a sync comment. **L10.** Step definitions — The try/except error-capture pattern is repeated 18 times across 4 step files. Consider extracting a `capture_error(ctx, *exc_types)` context manager. --- ### Static Analysis Results | Tool | Result | |------|--------| | **Semgrep** (auto + p/python + p/security-audit) | 0 findings — clean | | **Pyright** | 0 genuine errors (17 env-only errors from missing pydantic in /tmp) | | **Ruff** | 9 violations: 6x I001 (unsorted imports), 3x E402 (benchmark sys.path) — all auto-fixable | --- ### Checklist Verification | Architecture Check | Result | |---|---| | Domain model invariants preserved | **PARTIAL** — `_freeze_levels` has bypass risk (H2) | | No circular dependencies | **PASS** — verified one-way chain | | New abstractions have clear SRP | **PASS** | | Public interfaces documented | **PASS** — thorough docstrings | | Type annotations complete | **PARTIAL** — `levels` annotation lies (H4) | | Error types domain-specific | **FAIL** — bare ValueError everywhere (M4) | | Backward-compatible | **PARTIAL** — `effective_levels()` return type changed (H4), `levels` runtime type changed | --- ### Summary | Severity | Count | Action Required | |----------|-------|-----------------| | P0:blocker | 1 | Fix before merge | | P1:must-fix | 4 | Fix before merge | | P2:should-fix | 13 | Fix in follow-up PR (3 day SLA) | | P3:nit | 10 | Author discretion | **Verdict: REQUEST_CHANGES** — The P0 (wrong import) and P1 items (freeze invariant, serialization, type annotation, unbounded inputs) must be resolved before merge. The P2/P3 items can be addressed in a follow-up.
robot/helper_uko_layer2_paradigm.py Outdated
@@ -0,0 +25,4 @@
PROC_DETAIL_LEVEL_MAP,
)
from cleveragents.acms.uko.vocabularies import ( # noqa: E402
VocabularyRegistry,
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P0:blocker — VocabularyRegistry is imported from cleveragents.acms.uko.vocabularies, but it is defined in cleveragents.acms.uko.vocabulary_registry and is NOT exported from vocabularies.py. Every robot subcommand will crash at runtime with ImportError.

Fix:

from cleveragents.acms.uko.vocabularies import (
    get_func_vocabulary,
    get_oo_vocabulary,
    get_proc_vocabulary,
)
from cleveragents.acms.uko.vocabulary_registry import VocabularyRegistry
P0:blocker — `VocabularyRegistry` is imported from `cleveragents.acms.uko.vocabularies`, but it is defined in `cleveragents.acms.uko.vocabulary_registry` and is NOT exported from `vocabularies.py`. Every robot subcommand will crash at runtime with `ImportError`. Fix: ```python from cleveragents.acms.uko.vocabularies import ( get_func_vocabulary, get_oo_vocabulary, get_proc_vocabulary, ) from cleveragents.acms.uko.vocabulary_registry import VocabularyRegistry ```
src/cleveragents/acms/uko/detail_level_maps.py Outdated
@@ -0,0 +258,4 @@
return builder.build()
def _build_func_map() -> DetailLevelMap:
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P2:should-fix — _build_func_map (line 261) and _build_proc_map (line 278) are near-identical — they differ only in the domain string. Also, max_depth=9 is hardcoded instead of derived from CODE_DETAIL_LEVEL_MAP.max_depth. If the parent map ever changes, child maps silently drift.

Fix:

def _build_passthrough_map(domain: str) -> DetailLevelMap:
    return DetailLevelMap(
        domain=domain,
        parent=CODE_DETAIL_LEVEL_MAP,
        levels={},
        max_depth=CODE_DETAIL_LEVEL_MAP.max_depth,
    )
P2:should-fix — `_build_func_map` (line 261) and `_build_proc_map` (line 278) are near-identical — they differ only in the domain string. Also, `max_depth=9` is hardcoded instead of derived from `CODE_DETAIL_LEVEL_MAP.max_depth`. If the parent map ever changes, child maps silently drift. Fix: ```python def _build_passthrough_map(domain: str) -> DetailLevelMap: return DetailLevelMap( domain=domain, parent=CODE_DETAIL_LEVEL_MAP, levels={}, max_depth=CODE_DETAIL_LEVEL_MAP.max_depth, ) ```
src/cleveragents/acms/uko/vocabularies.py Outdated
@@ -0,0 +86,4 @@
default="",
description="rdfs:comment describing this class",
)
parent_uris: tuple[str, ...] = Field(
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P2:should-fix — Only the primary uri field validates the HTTP(S) scheme. The parent_uris, domain_uri, range_uri, and sub_property_of fields have no scheme validation. If these reach an RDF serializer or HTTP client downstream, non-HTTP URIs (e.g., javascript:, file:///) could be injected. Apply the shared validator to all URI-typed fields.

P2:should-fix — Only the primary `uri` field validates the HTTP(S) scheme. The `parent_uris`, `domain_uri`, `range_uri`, and `sub_property_of` fields have no scheme validation. If these reach an RDF serializer or HTTP client downstream, non-HTTP URIs (e.g., `javascript:`, `file:///`) could be injected. Apply the shared validator to all URI-typed fields.
src/cleveragents/acms/uko/vocabularies.py Outdated
@@ -0,0 +91,4 @@
description="rdfs:subClassOf parent IRIs",
)
@field_validator("uri")
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P2:should-fix — This _validate_uri method is character-for-character identical to the one in VocabularyProperty (line 144-150). Extract to a shared module-level helper:

def _validate_http_uri(v: str) -> str:
    if not v.startswith(("http://", "https://")):
        raise ValueError(f"URI must use http(s) scheme, got: {v!r}")
    return v
P2:should-fix — This `_validate_uri` method is character-for-character identical to the one in `VocabularyProperty` (line 144-150). Extract to a shared module-level helper: ```python def _validate_http_uri(v: str) -> str: if not v.startswith(("http://", "https://")): raise ValueError(f"URI must use http(s) scheme, got: {v!r}") return v ```
src/cleveragents/domain/models/acms/crp.py
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P1:must-fix — The levels field is annotated as dict[str, int] but holds MappingProxyType[str, int] at runtime after _freeze_levels runs. This means:

  • isinstance(map.levels, dict) → False at runtime
  • Type checkers believe .levels supports .update(), .pop(), [key]=val — all raise TypeError
  • model_dump_json() with Pydantic's Rust serializer is untested and may fail

Fix: Annotate as Mapping[str, int] (from typing). Add a test for model_dump() and model_dump_json() roundtrip.

P1:must-fix — The `levels` field is annotated as `dict[str, int]` but holds `MappingProxyType[str, int]` at runtime after `_freeze_levels` runs. This means: - `isinstance(map.levels, dict)` → `False` at runtime - Type checkers believe `.levels` supports `.update()`, `.pop()`, `[key]=val` — all raise `TypeError` - `model_dump_json()` with Pydantic's Rust serializer is untested and may fail Fix: Annotate as `Mapping[str, int]` (from `typing`). Add a test for `model_dump()` and `model_dump_json()` roundtrip.
src/cleveragents/domain/models/acms/crp.py
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P1:must-fix — _freeze_levels is a model_validator(mode="after") which runs at construction but does NOT re-run on field assignment in Pydantic v2. Since this model uses validate_assignment=True (not frozen=True), doing map.levels = {"FOO": 1} bypasses the freeze and sets a plain dict, silently breaking the immutability contract.

Fix: Either (a) make DetailLevelMap frozen=True and keep register() using object.__setattr__, or (b) convert to @field_validator("levels", mode="after") so it runs on every assignment.

P1:must-fix — `_freeze_levels` is a `model_validator(mode="after")` which runs at construction but does NOT re-run on field assignment in Pydantic v2. Since this model uses `validate_assignment=True` (not `frozen=True`), doing `map.levels = {"FOO": 1}` bypasses the freeze and sets a plain `dict`, silently breaking the immutability contract. Fix: Either (a) make `DetailLevelMap` `frozen=True` and keep `register()` using `object.__setattr__`, or (b) convert to `@field_validator("levels", mode="after")` so it runs on every assignment.
src/cleveragents/domain/models/acms/crp.py Outdated
brent.edwards commented 2026-03-10 23:51:13 +00:00
Member
Copy Link
Copy Source

P2:should-fix — resolve() recurses through self.parent.resolve(depth) with no cycle guard. If a circular parent reference is introduced (e.g., via DetailLevelMapBuilder mis-wiring), this causes RecursionError. Add a visited-set guard or max-recursion parameter.

P2:should-fix — `resolve()` recurses through `self.parent.resolve(depth)` with no cycle guard. If a circular parent reference is introduced (e.g., via `DetailLevelMapBuilder` mis-wiring), this causes `RecursionError`. Add a visited-set guard or max-recursion parameter.
freemo commented 2026-03-11 05:50:11 +00:00
Owner
Copy Link
Copy Source

PM Status (Day 31):

Merge conflict detected. The 17 PR merges from Days 30-31 (including your own 5 merges) significantly updated develop.

Action required: @hamza.khyari — please rebase this PR against current develop and push.

Priority: M6 work — continue at pace. Your Days 30-31 velocity was excellent (5 merges).

**PM Status (Day 31)**: Merge conflict detected. The 17 PR merges from Days 30-31 (including your own 5 merges) significantly updated `develop`. **Action required**: @hamza.khyari — please rebase this PR against current `develop` and push. **Priority**: M6 work — continue at pace. Your Days 30-31 velocity was excellent (5 merges).
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from 69fed8a925 to b7aff82d6f 2026-03-11 14:27:53 +00:00 Compare
hamza.khyari reviewed 2026-03-11 14:57:33 +00:00
hamza.khyari left a comment
Author
Member
Copy Link
Copy Source

Response to Review #2126 (Brent Edwards)

All findings have been validated against the current source (e6304300) and addressed in commit e6304300.

P0 (1 finding)

ID Finding Resolution
H1 Robot helper imports VocabularyRegistry from wrong module FIXED -- Updated robot/helper_uko_layer2_paradigm.py and docs/reference/uko_layer2_paradigm_vocabularies.md to import from vocabulary_registry

P1 (4 findings)

ID Finding Resolution
H2 _freeze_levels model_validator bypass on assignment FIXED -- Added __setattr__ override that re-freezes levels as MappingProxyType after Pydantic stores the validated value. Also changed validate_levels to mode="before" to handle MappingProxyType input on round-trip. BDD scenario added.
H3 MappingProxyType serialization untested / fails FIXED -- Added @field_serializer("levels") that converts MappingProxyType back to plain dict for JSON serialization. Added 2 BDD scenarios for model_dump_json() and model_dump().
H4 levels annotated as dict[str, int] but runtime is MappingProxyType DOCUMENTED -- Added field description noting the runtime wrapping behavior. Keeping dict[str, int] annotation for Pydantic deserialization compatibility.
H5 Unbounded list fields in ContextRequest OUT OF SCOPE -- ContextRequest is pre-existing code not modified in this PR. Filed as separate concern.

P2 (13 findings)

ID Finding Resolution
M1 Duplicated _validate_uri FIXED -- Extracted _validate_http_uri() shared helper, both classes now delegate to it.
M2 Secondary URIs not validated FIXED -- Added field_validator for parent_uris, domain_uri, range_uri, sub_property_of -- all now check HTTP(S) scheme.
M3 Near-identical func/proc builders FIXED -- Consolidated into _build_passthrough_map(domain). Extracted _CODE_MAX_DEPTH and _CODE_DOMAIN constants.
M4 No domain-specific exceptions FIXED -- Added DetailLevelError(ValueError) and DetailLevelCycleError(DetailLevelError).
M5 No cycle guard in resolve() FIXED -- Iterative parent-chain walk with id()-based visited set. BDD scenario added.
M6 register() race under concurrent access ACKNOWLEDGED -- Thread-safety documented in detail_level_maps.py module docstring (lines 14-19). Module constants are read-only after init; register() is not called at runtime.
M7 after_level not stripped in insert_after FIXED -- Now stripped before lookup and recording.
M8 Missing test: empty label/domain/range FIXED -- Added 3 BDD scenarios.
M9 Missing test: non-HTTP URI rejected FIXED -- Added 2 BDD scenarios.
M10 Missing test: insert_after at tail FIXED -- Added BDD scenario inserting after FULL_SOURCE.
M11 scoped_session semantics change INTENTIONAL -- Required to fix GC-induced SQLAlchemy session rollback with StaticPool. Documented.
M12 uko-oo:Class contradictory archetypes BY SPEC -- Per specification, uko-oo:Class is both a TypeDefinition (Atom) and Container. This is intentional OWL multiple inheritance.
M13 CHANGELOG missing PR reference FIXED -- Added PR #657.

P3 (selected fixes)

ID Finding Resolution
L1 sub_property_of default "" -> None FIXED
L2 Unnecessary parentheses in comment=() FIXED
L3 __all__ missing namespace constants FIXED
L5 "uko-code:" repeated 6 times FIXED -- Extracted as _CODE_DOMAIN constant
L4, L6-L10 Various style suggestions DEFERRED -- Low priority, not blocking

Quality Gates

  • lint: PASS
  • typecheck: PASS
  • L2 feature: 98 scenarios PASS (88 original + 10 new)
  • R2 coverage: 37 scenarios PASS
  • UKO ontology: 33 scenarios PASS
## Response to Review #2126 (Brent Edwards) All findings have been validated against the current source (`e6304300`) and addressed in commit `e6304300`. ### P0 (1 finding) | ID | Finding | Resolution | |----|---------|------------| | H1 | Robot helper imports VocabularyRegistry from wrong module | **FIXED** -- Updated `robot/helper_uko_layer2_paradigm.py` and `docs/reference/uko_layer2_paradigm_vocabularies.md` to import from `vocabulary_registry` | ### P1 (4 findings) | ID | Finding | Resolution | |----|---------|------------| | H2 | `_freeze_levels` model_validator bypass on assignment | **FIXED** -- Added `__setattr__` override that re-freezes `levels` as `MappingProxyType` after Pydantic stores the validated value. Also changed `validate_levels` to `mode="before"` to handle `MappingProxyType` input on round-trip. BDD scenario added. | | H3 | `MappingProxyType` serialization untested / fails | **FIXED** -- Added `@field_serializer("levels")` that converts `MappingProxyType` back to plain `dict` for JSON serialization. Added 2 BDD scenarios for `model_dump_json()` and `model_dump()`. | | H4 | `levels` annotated as `dict[str, int]` but runtime is `MappingProxyType` | **DOCUMENTED** -- Added field description noting the runtime wrapping behavior. Keeping `dict[str, int]` annotation for Pydantic deserialization compatibility. | | H5 | Unbounded list fields in `ContextRequest` | **OUT OF SCOPE** -- `ContextRequest` is pre-existing code not modified in this PR. Filed as separate concern. | ### P2 (13 findings) | ID | Finding | Resolution | |----|---------|------------| | M1 | Duplicated `_validate_uri` | **FIXED** -- Extracted `_validate_http_uri()` shared helper, both classes now delegate to it. | | M2 | Secondary URIs not validated | **FIXED** -- Added `field_validator` for `parent_uris`, `domain_uri`, `range_uri`, `sub_property_of` -- all now check HTTP(S) scheme. | | M3 | Near-identical func/proc builders | **FIXED** -- Consolidated into `_build_passthrough_map(domain)`. Extracted `_CODE_MAX_DEPTH` and `_CODE_DOMAIN` constants. | | M4 | No domain-specific exceptions | **FIXED** -- Added `DetailLevelError(ValueError)` and `DetailLevelCycleError(DetailLevelError)`. | | M5 | No cycle guard in `resolve()` | **FIXED** -- Iterative parent-chain walk with `id()`-based visited set. BDD scenario added. | | M6 | `register()` race under concurrent access | **ACKNOWLEDGED** -- Thread-safety documented in `detail_level_maps.py` module docstring (lines 14-19). Module constants are read-only after init; `register()` is not called at runtime. | | M7 | `after_level` not stripped in `insert_after` | **FIXED** -- Now stripped before lookup and recording. | | M8 | Missing test: empty label/domain/range | **FIXED** -- Added 3 BDD scenarios. | | M9 | Missing test: non-HTTP URI rejected | **FIXED** -- Added 2 BDD scenarios. | | M10 | Missing test: insert_after at tail | **FIXED** -- Added BDD scenario inserting after `FULL_SOURCE`. | | M11 | `scoped_session` semantics change | **INTENTIONAL** -- Required to fix GC-induced SQLAlchemy session rollback with `StaticPool`. Documented. | | M12 | `uko-oo:Class` contradictory archetypes | **BY SPEC** -- Per specification, `uko-oo:Class` is both a `TypeDefinition` (Atom) and `Container`. This is intentional OWL multiple inheritance. | | M13 | CHANGELOG missing PR reference | **FIXED** -- Added `PR #657`. | ### P3 (selected fixes) | ID | Finding | Resolution | |----|---------|------------| | L1 | `sub_property_of` default `""` -> `None` | **FIXED** | | L2 | Unnecessary parentheses in `comment=()` | **FIXED** | | L3 | `__all__` missing namespace constants | **FIXED** | | L5 | `"uko-code:"` repeated 6 times | **FIXED** -- Extracted as `_CODE_DOMAIN` constant | | L4, L6-L10 | Various style suggestions | **DEFERRED** -- Low priority, not blocking | ### Quality Gates - lint: PASS - typecheck: PASS - L2 feature: 98 scenarios PASS (88 original + 10 new) - R2 coverage: 37 scenarios PASS - UKO ontology: 33 scenarios PASS
hamza.khyari commented 2026-03-11 17:59:47 +00:00
Author
Member
Copy Link
Copy Source

Review Response -- Second Fix Pass (commit 339f71db)

All remaining valid-but-unfixed findings from Brent Edwards' review (ID 2126) have been addressed in this commit. Combined with the first fix pass (e6304300), every valid finding is now resolved.

Findings Addressed in Second Pass

ID Severity Finding Resolution
H5 P1 Unbounded list fields in ContextRequest Added max_length=500 to entities, uko_types, focus, preferred_strategies, required_backends via module-level _MAX_LIST_ITEMS constant
M6 P2 register() race condition Added PrivateAttr(default_factory=threading.Lock) field with with self._register_lock: guard around read-modify-write in register()
L4 P3 Implicit layer=2 and parent_domain defaults Changed both to Field(...) (required). Updated all 4 test callsites in uko_l2_coverage_ttl_steps.py
L6 P3 depth: int | str union dispatch Introduced DetailDepth = int | str type alias at module level; used in ContextRequest.depth field, DetailLevelMap.resolve() signature, and exported from acms.__init__
L7 P3 Unused uko-py: prefix in uko.ttl Removed @prefix uko-py: line
L8 P3 Inconsistent @frozen/@vocab_frozen tags Normalized to @vocab_frozen throughout
L9 P3 acms/__init__.py __all__ duplication Changed to __all__: list[str] = list(_uko.__all__)
L10 P3 try/except error-capture boilerplate (~29 occurrences) Extracted capture_error() context-manager helper in _uko_l2_test_helpers.py; applied to all 29 occurrences across 4 step files

Findings Confirmed Not Bugs (unchanged)

ID Finding Reason
M11 scoped_session semantics Intentional fix for GC-induced SQLAlchemy session rollback in test infrastructure
M12 uko-oo:Class dual inheritance Explicitly per spec line 44340; OWL allows multiple inheritance; Atom and Container are not disjoint

Quality Gates

Check Result
nox -s lint PASS
nox -s typecheck PASS
behave (L2 feature) 98 scenarios, 333 steps -- all PASS
behave (UKO ontology) PASS

All findings from the original review are now addressed. Ready for re-review.

## Review Response -- Second Fix Pass (commit 339f71db) All remaining valid-but-unfixed findings from Brent Edwards' review (ID 2126) have been addressed in this commit. Combined with the first fix pass (e6304300), every valid finding is now resolved. ### Findings Addressed in Second Pass | ID | Severity | Finding | Resolution | |----|----------|---------|------------| | H5 | P1 | Unbounded list fields in ContextRequest | Added `max_length=500` to `entities`, `uko_types`, `focus`, `preferred_strategies`, `required_backends` via module-level `_MAX_LIST_ITEMS` constant | | M6 | P2 | `register()` race condition | Added `PrivateAttr(default_factory=threading.Lock)` field with `with self._register_lock:` guard around read-modify-write in `register()` | | L4 | P3 | Implicit `layer=2` and `parent_domain` defaults | Changed both to `Field(...)` (required). Updated all 4 test callsites in `uko_l2_coverage_ttl_steps.py` | | L6 | P3 | `depth: int \| str` union dispatch | Introduced `DetailDepth = int \| str` type alias at module level; used in `ContextRequest.depth` field, `DetailLevelMap.resolve()` signature, and exported from `acms.__init__` | | L7 | P3 | Unused `uko-py:` prefix in uko.ttl | Removed `@prefix uko-py:` line | | L8 | P3 | Inconsistent `@frozen`/`@vocab_frozen` tags | Normalized to `@vocab_frozen` throughout | | L9 | P3 | `acms/__init__.py` `__all__` duplication | Changed to `__all__: list[str] = list(_uko.__all__)` | | L10 | P3 | try/except error-capture boilerplate (~29 occurrences) | Extracted `capture_error()` context-manager helper in `_uko_l2_test_helpers.py`; applied to all 29 occurrences across 4 step files | ### Findings Confirmed Not Bugs (unchanged) | ID | Finding | Reason | |----|---------|--------| | M11 | scoped_session semantics | Intentional fix for GC-induced SQLAlchemy session rollback in test infrastructure | | M12 | uko-oo:Class dual inheritance | Explicitly per spec line 44340; OWL allows multiple inheritance; Atom and Container are not disjoint | ### Quality Gates | Check | Result | |-------|--------| | `nox -s lint` | PASS | | `nox -s typecheck` | PASS | | behave (L2 feature) | 98 scenarios, 333 steps -- all PASS | | behave (UKO ontology) | PASS | All findings from the original review are now addressed. Ready for re-review.
freemo commented 2026-03-11 18:17:40 +00:00
Owner
Copy Link
Copy Source

PM Review — Day 31 (Specification Update)

This PR is mergeable — one of only 5 clean PRs in the repository.

Spec Alignment Check

UKO Layer 2 paradigm vocabularies (OO, functional, procedural) are NOT impacted by the ACP→A2A or TUI changes. The ACMS knowledge layer is orthogonal to the protocol layer.

Action Required

@hamza.khyari — This is ready for merge review. Excellent velocity (5 PRs merged recently). Please ensure CHANGELOG is updated if not already done.

## PM Review — Day 31 (Specification Update) This PR is **mergeable** — one of only 5 clean PRs in the repository. ### Spec Alignment Check UKO Layer 2 paradigm vocabularies (OO, functional, procedural) are NOT impacted by the ACP→A2A or TUI changes. The ACMS knowledge layer is orthogonal to the protocol layer. ### Action Required @hamza.khyari — This is ready for merge review. Excellent velocity (5 PRs merged recently). Please ensure CHANGELOG is updated if not already done.
hamza.khyari commented 2026-03-11 18:33:10 +00:00
Author
Member
Copy Link
Copy Source

H4 Fix (commit 6363106d)

Missed one: H4 (P1) -- levels field type annotation lies to the type checker.

Changed DetailLevelMap.levels annotation from dict[str, int] to Mapping[str, int] (from collections.abc). This truthfully reflects the MappingProxyType stored at runtime -- type checkers now see read-only methods only (__getitem__, keys(), values(), items()) and will correctly flag mutation attempts.

All quality gates still pass (lint, typecheck, 98 scenarios / 333 steps).

All 28 findings from the review are now addressed:

  • 26 fixed (H1-H5, M1-M10, M13, L1-L10)
  • 2 confirmed not bugs with justification (M11, M12)
## H4 Fix (commit 6363106d) Missed one: **H4 (P1) -- `levels` field type annotation lies to the type checker.** Changed `DetailLevelMap.levels` annotation from `dict[str, int]` to `Mapping[str, int]` (from `collections.abc`). This truthfully reflects the `MappingProxyType` stored at runtime -- type checkers now see read-only methods only (`__getitem__`, `keys()`, `values()`, `items()`) and will correctly flag mutation attempts. All quality gates still pass (lint, typecheck, 98 scenarios / 333 steps). **All 28 findings from the review are now addressed:** - 26 fixed (H1-H5, M1-M10, M13, L1-L10) - 2 confirmed not bugs with justification (M11, M12)
brent.edwards requested changes 2026-03-11 18:53:52 +00:00
Dismissed
brent.edwards left a comment
Member
Copy Link
Copy Source

Second-Round Review — Export chains, backward compatibility, __all__ sorting

I verified the export chains, searched for existing callers of changed APIs, and checked __all__ sorting across all new/modified files. Four issues found; one P1, three P2.

P1 — DetailLevelError / DetailLevelCycleError not exported from domain/models/acms/__init__.py

File: src/cleveragents/domain/models/acms/__init__.py

The PR adds two new public exception classes to crp.py:

class DetailLevelError(ValueError): ...
class DetailLevelCycleError(DetailLevelError): ...

Neither is imported nor listed in __all__ of domain/models/acms/__init__.py. The only current consumer (features/steps/uko_l2_detail_level_steps.py:310) works around this by importing directly from the internal module:

from cleveragents.domain.models.acms.crp import DetailLevelCycleError

This breaks the established encapsulation pattern in this codebase — every other public symbol in crp.py is re-exported through the package __init__.py. Downstream consumers who want to catch these specific exceptions (rather than the generic ValueError) would need to reach into the internal crp module.

Fix: Add both to the import block and to __all__ in domain/models/acms/__init__.py.

P2 — __all__ sorting violations in 3 files

CONTRIBUTING.md requires sorted __all__. Three files have incorrect ordering:

1. src/cleveragents/acms/uko/vocabularies.py — UKO_* constants are placed before ParadigmVocabulary, but in ASCII/lexicographic sort, P (0x50) < U (0x55), so ParadigmVocabulary must come first. Correct order:

ParadigmVocabulary, UKO_ATOM, UKO_BOUNDARY, ..., UKO_PROC_PREFIX,
VocabularyClass, VocabularyProperty, get_func_vocabulary, ...

2. src/cleveragents/acms/uko/detail_level_maps.py — DetailLevelMapBuilder (D) sorts before FUNC_DETAIL_LEVEL_MAP (F), but currently appears after PROC_DETAIL_LEVEL_MAP. Correct order:

CODE_DETAIL_LEVEL_MAP, DetailLevelMapBuilder, FUNC_DETAIL_LEVEL_MAP,
OO_DETAIL_LEVEL_MAP, PROC_DETAIL_LEVEL_MAP, build_effective_map

3. src/cleveragents/acms/uko/__init__.py — Same issue inherited from detail_level_maps.py. DetailLevelMapBuilder must come after CODE_DETAIL_LEVEL_MAP and before FUNC_DETAIL_LEVEL_MAP.

4. src/cleveragents/acms/__init__.py — Uses list(_uko.__all__) so it inherits whatever order uko/__init__.py has. Once (3) is fixed this auto-fixes, but worth noting that the explicit imports at the top of the file are also in the unsorted order and should match the corrected __all__.

P2 — effective_levels() return type change is a breaking API change

File: src/cleveragents/domain/models/acms/crp.py:246

The return type of effective_levels() changed from dict[str, int] to MappingProxyType[str, int]. This is a behavioral breaking change: any caller that mutates the returned mapping (e.g., levels["FOO"] = 5) will now get a TypeError at runtime.

There is one existing caller in features/steps/crp_models_steps.py:148 that annotates the return as dict[str, int]:

levels: dict[str, int] = context.child_detail_level_map.effective_levels()

This is technically a type error now (the annotation lies), though it works at runtime since the caller only reads. However, the asymmetry should be cleaned up — the test annotation should use Mapping[str, int] or MappingProxyType[str, int].

More importantly: since effective_levels() previously returned a mutable dict, any caller who mutated the result (even unintentionally via merged.update() patterns in their own code) will now break. I searched the codebase and found no current callers that mutate the return value, so the practical risk is low. But this should be documented in the changelog entry as a breaking change.

Informational — Items verified as correct

  • DetailDepth export: Correctly added to domain/models/acms/__init__.py imports and __all__. ✓
  • _MAX_LIST_ITEMS privacy: Underscore-prefixed, not exported. ✓
  • acms/__init__.py re-exports: list(_uko.__all__) correctly picks up VocabularyRegistry and all 13 symbols. ✓
  • _build_passthrough_map correctness: levels={} with parent=CODE_DETAIL_LEVEL_MAP is correct. resolve("SIGNATURES") walks the parent chain and finds it. effective_levels() merges parent levels (all 10 code levels) with the empty child dict, returning the full parent set. The max_depth=9 matches the parent. ✓
  • resolve() exception subclassing: DetailLevelError(ValueError) and DetailLevelCycleError(DetailLevelError) are both subclasses of ValueError, so existing except ValueError handlers (e.g., in crp_models_steps.py, depth_breadth_projection.py) continue to catch them. ✓ Backward compatible.
  • ContextRequest max_length=500: All existing construction sites pass small lists (typically 0-5 items). No breakage risk. ✓
  • domain/models/acms/__init__.py __all__: Sorted correctly. ✓
  • vocabulary_registry.py __all__: Single entry, sorted. ✓
## Second-Round Review — Export chains, backward compatibility, `__all__` sorting I verified the export chains, searched for existing callers of changed APIs, and checked `__all__` sorting across all new/modified files. Four issues found; one P1, three P2. --- ### P1 — `DetailLevelError` / `DetailLevelCycleError` not exported from `domain/models/acms/__init__.py` **File:** `src/cleveragents/domain/models/acms/__init__.py` The PR adds two new public exception classes to `crp.py`: ```python class DetailLevelError(ValueError): ... class DetailLevelCycleError(DetailLevelError): ... ``` Neither is imported nor listed in `__all__` of `domain/models/acms/__init__.py`. The only current consumer (`features/steps/uko_l2_detail_level_steps.py:310`) works around this by importing directly from the internal module: ```python from cleveragents.domain.models.acms.crp import DetailLevelCycleError ``` This breaks the established encapsulation pattern in this codebase — every other public symbol in `crp.py` is re-exported through the package `__init__.py`. Downstream consumers who want to catch these specific exceptions (rather than the generic `ValueError`) would need to reach into the internal `crp` module. **Fix:** Add both to the import block and to `__all__` in `domain/models/acms/__init__.py`. --- ### P2 — `__all__` sorting violations in 3 files CONTRIBUTING.md requires sorted `__all__`. Three files have incorrect ordering: **1. `src/cleveragents/acms/uko/vocabularies.py`** — `UKO_*` constants are placed before `ParadigmVocabulary`, but in ASCII/lexicographic sort, `P` (0x50) < `U` (0x55), so `ParadigmVocabulary` must come first. Correct order: ``` ParadigmVocabulary, UKO_ATOM, UKO_BOUNDARY, ..., UKO_PROC_PREFIX, VocabularyClass, VocabularyProperty, get_func_vocabulary, ... ``` **2. `src/cleveragents/acms/uko/detail_level_maps.py`** — `DetailLevelMapBuilder` (D) sorts before `FUNC_DETAIL_LEVEL_MAP` (F), but currently appears after `PROC_DETAIL_LEVEL_MAP`. Correct order: ``` CODE_DETAIL_LEVEL_MAP, DetailLevelMapBuilder, FUNC_DETAIL_LEVEL_MAP, OO_DETAIL_LEVEL_MAP, PROC_DETAIL_LEVEL_MAP, build_effective_map ``` **3. `src/cleveragents/acms/uko/__init__.py`** — Same issue inherited from `detail_level_maps.py`. `DetailLevelMapBuilder` must come after `CODE_DETAIL_LEVEL_MAP` and before `FUNC_DETAIL_LEVEL_MAP`. **4. `src/cleveragents/acms/__init__.py`** — Uses `list(_uko.__all__)` so it inherits whatever order `uko/__init__.py` has. Once (3) is fixed this auto-fixes, but worth noting that the explicit imports at the top of the file are also in the unsorted order and should match the corrected `__all__`. --- ### P2 — `effective_levels()` return type change is a breaking API change **File:** `src/cleveragents/domain/models/acms/crp.py:246` The return type of `effective_levels()` changed from `dict[str, int]` to `MappingProxyType[str, int]`. This is a **behavioral** breaking change: any caller that mutates the returned mapping (e.g., `levels["FOO"] = 5`) will now get a `TypeError` at runtime. There is one existing caller in `features/steps/crp_models_steps.py:148` that annotates the return as `dict[str, int]`: ```python levels: dict[str, int] = context.child_detail_level_map.effective_levels() ``` This is technically a type error now (the annotation lies), though it works at runtime since the caller only reads. However, the asymmetry should be cleaned up — the test annotation should use `Mapping[str, int]` or `MappingProxyType[str, int]`. More importantly: since `effective_levels()` previously returned a mutable `dict`, any caller who mutated the result (even unintentionally via `merged.update()` patterns in their own code) will now break. I searched the codebase and found no current callers that mutate the return value, so the practical risk is low. But this should be documented in the changelog entry as a breaking change. --- ### Informational — Items verified as correct - **`DetailDepth` export:** Correctly added to `domain/models/acms/__init__.py` imports and `__all__`. ✓ - **`_MAX_LIST_ITEMS` privacy:** Underscore-prefixed, not exported. ✓ - **`acms/__init__.py` re-exports:** `list(_uko.__all__)` correctly picks up `VocabularyRegistry` and all 13 symbols. ✓ - **`_build_passthrough_map` correctness:** `levels={}` with `parent=CODE_DETAIL_LEVEL_MAP` is correct. `resolve("SIGNATURES")` walks the parent chain and finds it. `effective_levels()` merges parent levels (all 10 code levels) with the empty child dict, returning the full parent set. The `max_depth=9` matches the parent. ✓ - **`resolve()` exception subclassing:** `DetailLevelError(ValueError)` and `DetailLevelCycleError(DetailLevelError)` are both subclasses of `ValueError`, so existing `except ValueError` handlers (e.g., in `crp_models_steps.py`, `depth_breadth_projection.py`) continue to catch them. ✓ Backward compatible. - **`ContextRequest` max_length=500:** All existing construction sites pass small lists (typically 0-5 items). No breakage risk. ✓ - **`domain/models/acms/__init__.py` `__all__`:** Sorted correctly. ✓ - **`vocabulary_registry.py` `__all__`:** Single entry, sorted. ✓
src/cleveragents/acms/uko/__init__.py Outdated
@@ -0,0 +36,4 @@
from cleveragents.acms.uko.vocabulary_registry import VocabularyRegistry
__all__: list[str] = [
"CODE_DETAIL_LEVEL_MAP",
brent.edwards commented 2026-03-11 18:53:52 +00:00
Member
Copy Link
Copy Source

P2: Same __all__ sorting issue inherited from the submodules. DetailLevelMapBuilder must come after CODE_DETAIL_LEVEL_MAP and before FUNC_DETAIL_LEVEL_MAP. The explicit imports at the top should also be reordered to match the corrected sort.

**P2**: Same `__all__` sorting issue inherited from the submodules. `DetailLevelMapBuilder` must come after `CODE_DETAIL_LEVEL_MAP` and before `FUNC_DETAIL_LEVEL_MAP`. The explicit imports at the top should also be reordered to match the corrected sort.
src/cleveragents/acms/uko/detail_level_maps.py Outdated
@@ -0,0 +32,4 @@
__all__: list[str] = [
"CODE_DETAIL_LEVEL_MAP",
"FUNC_DETAIL_LEVEL_MAP",
"OO_DETAIL_LEVEL_MAP",
brent.edwards commented 2026-03-11 18:53:52 +00:00
Member
Copy Link
Copy Source

P2: __all__ is not sorted. DetailLevelMapBuilder (D) sorts before FUNC_DETAIL_LEVEL_MAP (F). Correct order:

__all__: list[str] = [
    "CODE_DETAIL_LEVEL_MAP",
    "DetailLevelMapBuilder",
    "FUNC_DETAIL_LEVEL_MAP",
    "OO_DETAIL_LEVEL_MAP",
    "PROC_DETAIL_LEVEL_MAP",
    "build_effective_map",
]
**P2**: `__all__` is not sorted. `DetailLevelMapBuilder` (D) sorts before `FUNC_DETAIL_LEVEL_MAP` (F). Correct order: ```python __all__: list[str] = [ "CODE_DETAIL_LEVEL_MAP", "DetailLevelMapBuilder", "FUNC_DETAIL_LEVEL_MAP", "OO_DETAIL_LEVEL_MAP", "PROC_DETAIL_LEVEL_MAP", "build_effective_map", ] ```
src/cleveragents/acms/uko/vocabularies.py Outdated
@@ -0,0 +20,4 @@
from pydantic import BaseModel, ConfigDict, Field, field_validator
__all__: list[str] = [
"UKO_ATOM",
brent.edwards commented 2026-03-11 18:53:52 +00:00
Member
Copy Link
Copy Source

P2: __all__ is not sorted per CONTRIBUTING.md. In ASCII/lexicographic order, P < U < V < g, so ParadigmVocabulary must come before the UKO_* constants. Correct order:

__all__: list[str] = [
    "ParadigmVocabulary",
    "UKO_ATOM",
    "UKO_BOUNDARY",
    "UKO_CODE_CALLABLE",
    "UKO_CODE_TYPE_DEFINITION",
    "UKO_CONTAINER",
    "UKO_DEPENDS_ON",
    "UKO_FUNC_IRI",
    "UKO_FUNC_PREFIX",
    "UKO_OO_IRI",
    "UKO_OO_PREFIX",
    "UKO_PROC_IRI",
    "UKO_PROC_PREFIX",
    "VocabularyClass",
    "VocabularyProperty",
    "get_func_vocabulary",
    "get_oo_vocabulary",
    "get_proc_vocabulary",
]
**P2**: `__all__` is not sorted per CONTRIBUTING.md. In ASCII/lexicographic order, `P` < `U` < `V` < `g`, so `ParadigmVocabulary` must come before the `UKO_*` constants. Correct order: ```python __all__: list[str] = [ "ParadigmVocabulary", "UKO_ATOM", "UKO_BOUNDARY", "UKO_CODE_CALLABLE", "UKO_CODE_TYPE_DEFINITION", "UKO_CONTAINER", "UKO_DEPENDS_ON", "UKO_FUNC_IRI", "UKO_FUNC_PREFIX", "UKO_OO_IRI", "UKO_OO_PREFIX", "UKO_PROC_IRI", "UKO_PROC_PREFIX", "VocabularyClass", "VocabularyProperty", "get_func_vocabulary", "get_oo_vocabulary", "get_proc_vocabulary", ] ```
src/cleveragents/domain/models/acms/__init__.py Outdated
brent.edwards commented 2026-03-11 18:53:52 +00:00
Member
Copy Link
Copy Source

P1: DetailLevelError and DetailLevelCycleError are defined in crp.py but not imported here and not listed in __all__. This breaks the encapsulation pattern — every other public CRP symbol is re-exported through this __init__.py.

Add to the crp import block:

from cleveragents.domain.models.acms.crp import (
    ...
    DetailLevelCycleError,
    DetailLevelError,
    ...
)

And add both to __all__ (between DetailDepth and DetailLevelMap).

**P1**: `DetailLevelError` and `DetailLevelCycleError` are defined in `crp.py` but not imported here and not listed in `__all__`. This breaks the encapsulation pattern — every other public CRP symbol is re-exported through this `__init__.py`. Add to the `crp` import block: ```python from cleveragents.domain.models.acms.crp import ( ... DetailLevelCycleError, DetailLevelError, ... ) ``` And add both to `__all__` (between `DetailDepth` and `DetailLevelMap`).
src/cleveragents/domain/models/acms/crp.py Outdated
@@ -139,2 +244,3 @@
object.__setattr__(self, "levels", MappingProxyType(updated))
def effective_levels(self) -> dict[str, int]:
def effective_levels(self) -> MappingProxyType[str, int]:
brent.edwards commented 2026-03-11 18:53:52 +00:00
Member
Copy Link
Copy Source

P2: Return type changed from dict[str, int] → MappingProxyType[str, int]. This is a behavioral breaking change (callers who mutated the old return value will now get TypeError). No current callers are affected, but this should be noted in the CHANGELOG entry as a breaking change. Also, features/steps/crp_models_steps.py:148 annotates the return as dict[str, int] which is now incorrect.

**P2**: Return type changed from `dict[str, int]` → `MappingProxyType[str, int]`. This is a behavioral breaking change (callers who mutated the old return value will now get `TypeError`). No current callers are affected, but this should be noted in the CHANGELOG entry as a breaking change. Also, `features/steps/crp_models_steps.py:148` annotates the return as `dict[str, int]` which is now incorrect.
brent.edwards requested changes 2026-03-11 18:57:53 +00:00
brent.edwards left a comment
Member
Copy Link
Copy Source

Second-Round Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies

Reviewer: Brent Edwards
Review method: 6 parallel review threads (fix verification, domain model deep-dive, test quality, ontology/TTL alignment, security/type safety, backward compatibility/API surface)

First-Round Fix Verification

16 of 17 original findings fully resolved. Excellent fix work by @hamza.khyari — the two fix-pass commits address the vast majority of issues thoroughly and correctly. The only partial fix is M4 (domain-specific exceptions): DetailLevelError and DetailLevelCycleError are present but DuplicateVocabularyError for VocabularyRegistry was not added. This is low-risk and acceptable as-is.

New Findings — P1:must-fix (5)

N1. DetailLevelError / DetailLevelCycleError not exported from package __init__.py
src/cleveragents/domain/models/acms/__init__.py — Both exceptions are defined in crp.py:68-73 and raised by resolve() at lines 207 and 215, but neither is imported into __init__.py nor listed in __all__. Consumers must use the internal path from cleveragents.domain.models.acms.crp import DetailLevelError — breaking the package encapsulation pattern. The existing caller in depth_breadth_projection.py:309 lets errors propagate as ValueError, which still works since both subclass ValueError, but the public API contract is incomplete.

Fix: Add to the import block and __all__:

from cleveragents.domain.models.acms.crp import (
    ...
    DetailLevelCycleError,
    DetailLevelError,
    ...
)

N2. deepcopy / pickle / model_copy(deep=True) crash on DetailLevelMap
src/cleveragents/domain/models/acms/crp.py — The combination of MappingProxyType in levels and threading.Lock in _register_lock makes the model unpicklable and un-deepcopyable. copy.deepcopy(model) raises TypeError: cannot pickle 'mappingproxy' object. This matters because LangGraph's state management (src/cleveragents/langgraph/nodes.py:100) uses deepcopy — any state containing a DetailLevelMap would crash at runtime.

Fix: Add __deepcopy__ and __copy__ methods:

def __deepcopy__(self, memo: dict) -> DetailLevelMap:
    return self.model_copy(update={"levels": dict(self.levels)})

N3. query field on ContextRequest still has no max_length
src/cleveragents/domain/models/acms/crp.py:416-419 — The original H5 finding explicitly flagged query: str alongside the list fields. The list fields all received max_length=500, but query was missed. A 100MB query string passes validation, which is the single largest memory-exhaustion vector on the input model.

Fix:

query: str | None = Field(
    default=None,
    max_length=10_000,
    description="Natural language query",
)

N4. Inner-function imports in uko_l2_detail_level_steps.py
features/steps/uko_l2_detail_level_steps.py:310,318,331 — Three imports are inside function bodies:

  • Line 310: from cleveragents.domain.models.acms.crp import DetailLevelCycleError
  • Line 318: Same import duplicated
  • Line 331: from types import MappingProxyType

CONTRIBUTING.md lines 1289-1294 is absolute: "Ensure all imports are at the top of the Python file. Never encapsulate imports inside an indented code block." The only exception is if TYPE_CHECKING:.

N5. Robot helper uses except Exception without re-raising in all 5 commands
robot/helper_uko_layer2_paradigm.py:58,77,102,159,200 — Every command function catches Exception, prints a message, and returns 1 without re-raising. CONTRIBUTING.md lines 496, 1382: "Do not suppress errors. Do not use bare except: or except Exception: without re-raising." The exception type and traceback are lost.

Fix: Either narrow the caught types to specific expected exceptions, or add traceback.print_exc() and re-raise.

New Findings — P2:should-fix (7)

N6. __setattr__ re-freezes from original pre-validation value, not validated self.levels
crp.py:170 — After super().__setattr__(name, value), Pydantic has validated and stored the result on self.levels. But the re-freeze line uses MappingProxyType(dict(value)) — the original pre-validation input — not MappingProxyType(dict(self.levels)). Currently safe because validate_levels doesn't modify keys/values, but if the validator is ever extended to normalize keys (strip whitespace, case-fold), the __setattr__ would silently revert that normalization.

Fix: object.__setattr__(self, "levels", MappingProxyType(dict(self.levels))) instead of dict(value).

N7. Factually incorrect comments about model_validator behavior
crp.py:148-155,161-165 — The docstrings state "model_validator does not re-run on validate_assignment". In Pydantic 2.12.5 (the project's version), model_validator(mode='after') does re-run on validate_assignment. The __setattr__ override is therefore redundant (but harmless as defensive code). The comments will mislead future maintainers.

Fix: Update comments to say the __setattr__ is a defensive measure, not a fix for a current Pydantic limitation.

N8. capture_error() context manager suppresses exceptions
features/steps/_uko_l2_test_helpers.py:42-49 — The helper catches exc_types, stores the exception on ctx.error, but never re-raises. This is a hand-rolled contextlib.suppress() equivalent. While the intent (negative-test capture) is sound, CONTRIBUTING.md prohibits suppression. Add a code comment citing the test-infrastructure exemption rationale.

N9. __all__ lists not sorted in 3 files
src/cleveragents/acms/uko/vocabularies.py, src/cleveragents/acms/uko/detail_level_maps.py, src/cleveragents/acms/uko/__init__.py — The UKO_* namespace constants are placed before alphabetically earlier entries. acms/__init__.py inherits the issue via list(_uko.__all__). CONTRIBUTING.md requires sorted __all__.

N10. effective_levels() return type change not documented as breaking
crp.py — Return type changed from dict[str, int] to MappingProxyType[str, int]. Any downstream code doing levels = map.effective_levels(); levels["FOO"] = 1 would now get TypeError. No current callers are affected, but this is a public API change. Should be noted in CHANGELOG.

N11. Missing test coverage for diverse bad URI schemes on secondary fields
features/acms/uko_layer2_paradigm_vocabularies.feature — Tests only cover ftp:// on VocabularyClass.uri and urn: on VocabularyProperty.uri. No tests verify scheme rejection on parent_uris, domain_uri, range_uri, or sub_property_of — all of which have validators.

N12. Incomplete H5 fix — M4 domain exceptions partial
VocabularyRegistry.register() in vocabulary_registry.py:67-72 still raises bare ValueError for duplicate prefix/IRI. The M4 finding asked for DuplicateVocabularyError. Low priority since the detail-level exceptions (the critical ones) are present.

New Findings — P3:nit (8)

N13. crp.py:155 — __setattr__ parameter value: object vs Pydantic's value: Any. No functional impact but inconsistent with parent signature.

N14. crp.py:226-241 — register() doesn't enforce value <= max_depth. A registered named level can map to a depth exceeding max_depth, while resolve(99) clamps integers. Inconsistent capping behavior.

N15. crp.py:405 — List items in ContextRequest lack per-string max_length. 500 items of 1MB strings each = 500MB.

N16. vocabularies.py:82 — _validate_http_uri is scheme-only. No SSRF protection (low risk: identifiers not fetched, but worth a code comment).

N17. docs/reference/uko_layer2_paradigm_vocabularies.md — Description columns paraphrase rdfs:comment rather than quoting verbatim.

N18. docs/ontology/uko.ttl:555-558 — No inline comment documenting the intentional dual Atom+Container inheritance design decision (M12 follow-up).

N19. docs/reference/uko_layer2_paradigm_vocabularies.md — "Layer 1 Dependencies" section omits Layer 0 URIs that are directly referenced.

N20. features/steps/uko_l2_detail_level_steps.py:339-345 — Manual try/except TypeError: pass instead of using capture_error(), inconsistent with rest of test suite.

Static Analysis

Tool Result
Pyright 0 errors on all src/ files
Semgrep (auto + p/python + p/security-audit) 0 findings
Ruff Clean (verified by author's CI)

Ontology Consistency

All 12 Layer 2 OWL classes and 2 OWL properties verified field-by-field across TTL, Python models, and reference docs. No mismatches in URIs, labels, comments, or parent chains. Namespace IRIs consistent across all sources.

Summary

Severity Count Action Required
P0:blocker 0 —
P1:must-fix 5 Fix before merge
P2:should-fix 7 Fix in this PR or follow-up (3 day SLA)
P3:nit 8 Author discretion

Verdict: REQUEST_CHANGES — The P1 items (missing exports, deepcopy crash, query max_length, CONTRIBUTING.md violations) must be addressed. All are straightforward fixes. The bulk of the first-round issues were resolved excellently — this is close to merge-ready.

Note: Per playbook escalation rules, N2 (deepcopy crash affecting LangGraph) may warrant a quick check with the LangGraph subsystem owner to confirm whether DetailLevelMap objects appear in graph state.

## Second-Round Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies **Reviewer:** Brent Edwards **Review method:** 6 parallel review threads (fix verification, domain model deep-dive, test quality, ontology/TTL alignment, security/type safety, backward compatibility/API surface) --- ### First-Round Fix Verification **16 of 17 original findings fully resolved.** Excellent fix work by @hamza.khyari — the two fix-pass commits address the vast majority of issues thoroughly and correctly. The only partial fix is M4 (domain-specific exceptions): `DetailLevelError` and `DetailLevelCycleError` are present but `DuplicateVocabularyError` for `VocabularyRegistry` was not added. This is low-risk and acceptable as-is. --- ### New Findings — P1:must-fix (5) **N1. `DetailLevelError` / `DetailLevelCycleError` not exported from package `__init__.py`** `src/cleveragents/domain/models/acms/__init__.py` — Both exceptions are defined in `crp.py:68-73` and raised by `resolve()` at lines 207 and 215, but neither is imported into `__init__.py` nor listed in `__all__`. Consumers must use the internal path `from cleveragents.domain.models.acms.crp import DetailLevelError` — breaking the package encapsulation pattern. The existing caller in `depth_breadth_projection.py:309` lets errors propagate as `ValueError`, which still works since both subclass `ValueError`, but the public API contract is incomplete. Fix: Add to the import block and `__all__`: ```python from cleveragents.domain.models.acms.crp import ( ... DetailLevelCycleError, DetailLevelError, ... ) ``` **N2. `deepcopy` / `pickle` / `model_copy(deep=True)` crash on `DetailLevelMap`** `src/cleveragents/domain/models/acms/crp.py` — The combination of `MappingProxyType` in `levels` and `threading.Lock` in `_register_lock` makes the model unpicklable and un-deepcopyable. `copy.deepcopy(model)` raises `TypeError: cannot pickle 'mappingproxy' object`. This matters because LangGraph's state management (`src/cleveragents/langgraph/nodes.py:100`) uses `deepcopy` — any state containing a `DetailLevelMap` would crash at runtime. Fix: Add `__deepcopy__` and `__copy__` methods: ```python def __deepcopy__(self, memo: dict) -> DetailLevelMap: return self.model_copy(update={"levels": dict(self.levels)}) ``` **N3. `query` field on `ContextRequest` still has no `max_length`** `src/cleveragents/domain/models/acms/crp.py:416-419` — The original H5 finding explicitly flagged `query: str` alongside the list fields. The list fields all received `max_length=500`, but `query` was missed. A 100MB query string passes validation, which is the single largest memory-exhaustion vector on the input model. Fix: ```python query: str | None = Field( default=None, max_length=10_000, description="Natural language query", ) ``` **N4. Inner-function imports in `uko_l2_detail_level_steps.py`** `features/steps/uko_l2_detail_level_steps.py:310,318,331` — Three imports are inside function bodies: - Line 310: `from cleveragents.domain.models.acms.crp import DetailLevelCycleError` - Line 318: Same import duplicated - Line 331: `from types import MappingProxyType` CONTRIBUTING.md lines 1289-1294 is absolute: *"Ensure all imports are at the top of the Python file. Never encapsulate imports inside an indented code block."* The only exception is `if TYPE_CHECKING:`. **N5. Robot helper uses `except Exception` without re-raising in all 5 commands** `robot/helper_uko_layer2_paradigm.py:58,77,102,159,200` — Every command function catches `Exception`, prints a message, and returns 1 without re-raising. CONTRIBUTING.md lines 496, 1382: *"Do not suppress errors. Do not use bare `except:` or `except Exception:` without re-raising."* The exception type and traceback are lost. Fix: Either narrow the caught types to specific expected exceptions, or add `traceback.print_exc()` and re-raise. --- ### New Findings — P2:should-fix (7) **N6. `__setattr__` re-freezes from original pre-validation `value`, not validated `self.levels`** `crp.py:170` — After `super().__setattr__(name, value)`, Pydantic has validated and stored the result on `self.levels`. But the re-freeze line uses `MappingProxyType(dict(value))` — the original pre-validation input — not `MappingProxyType(dict(self.levels))`. Currently safe because `validate_levels` doesn't modify keys/values, but if the validator is ever extended to normalize keys (strip whitespace, case-fold), the `__setattr__` would silently revert that normalization. Fix: `object.__setattr__(self, "levels", MappingProxyType(dict(self.levels)))` instead of `dict(value)`. **N7. Factually incorrect comments about model_validator behavior** `crp.py:148-155,161-165` — The docstrings state *"model_validator does not re-run on validate_assignment"*. In Pydantic 2.12.5 (the project's version), `model_validator(mode='after')` **does** re-run on `validate_assignment`. The `__setattr__` override is therefore redundant (but harmless as defensive code). The comments will mislead future maintainers. Fix: Update comments to say the `__setattr__` is a defensive measure, not a fix for a current Pydantic limitation. **N8. `capture_error()` context manager suppresses exceptions** `features/steps/_uko_l2_test_helpers.py:42-49` — The helper catches `exc_types`, stores the exception on `ctx.error`, but never re-raises. This is a hand-rolled `contextlib.suppress()` equivalent. While the intent (negative-test capture) is sound, CONTRIBUTING.md prohibits suppression. Add a code comment citing the test-infrastructure exemption rationale. **N9. `__all__` lists not sorted in 3 files** `src/cleveragents/acms/uko/vocabularies.py`, `src/cleveragents/acms/uko/detail_level_maps.py`, `src/cleveragents/acms/uko/__init__.py` — The `UKO_*` namespace constants are placed before alphabetically earlier entries. `acms/__init__.py` inherits the issue via `list(_uko.__all__)`. CONTRIBUTING.md requires sorted `__all__`. **N10. `effective_levels()` return type change not documented as breaking** `crp.py` — Return type changed from `dict[str, int]` to `MappingProxyType[str, int]`. Any downstream code doing `levels = map.effective_levels(); levels["FOO"] = 1` would now get `TypeError`. No current callers are affected, but this is a public API change. Should be noted in CHANGELOG. **N11. Missing test coverage for diverse bad URI schemes on secondary fields** `features/acms/uko_layer2_paradigm_vocabularies.feature` — Tests only cover `ftp://` on `VocabularyClass.uri` and `urn:` on `VocabularyProperty.uri`. No tests verify scheme rejection on `parent_uris`, `domain_uri`, `range_uri`, or `sub_property_of` — all of which have validators. **N12. Incomplete H5 fix — M4 domain exceptions partial** `VocabularyRegistry.register()` in `vocabulary_registry.py:67-72` still raises bare `ValueError` for duplicate prefix/IRI. The M4 finding asked for `DuplicateVocabularyError`. Low priority since the detail-level exceptions (the critical ones) are present. --- ### New Findings — P3:nit (8) **N13.** `crp.py:155` — `__setattr__` parameter `value: object` vs Pydantic's `value: Any`. No functional impact but inconsistent with parent signature. **N14.** `crp.py:226-241` — `register()` doesn't enforce `value <= max_depth`. A registered named level can map to a depth exceeding `max_depth`, while `resolve(99)` clamps integers. Inconsistent capping behavior. **N15.** `crp.py:405` — List items in `ContextRequest` lack per-string `max_length`. 500 items of 1MB strings each = 500MB. **N16.** `vocabularies.py:82` — `_validate_http_uri` is scheme-only. No SSRF protection (low risk: identifiers not fetched, but worth a code comment). **N17.** `docs/reference/uko_layer2_paradigm_vocabularies.md` — Description columns paraphrase `rdfs:comment` rather than quoting verbatim. **N18.** `docs/ontology/uko.ttl:555-558` — No inline comment documenting the intentional dual Atom+Container inheritance design decision (M12 follow-up). **N19.** `docs/reference/uko_layer2_paradigm_vocabularies.md` — "Layer 1 Dependencies" section omits Layer 0 URIs that are directly referenced. **N20.** `features/steps/uko_l2_detail_level_steps.py:339-345` — Manual `try/except TypeError: pass` instead of using `capture_error()`, inconsistent with rest of test suite. --- ### Static Analysis | Tool | Result | |------|--------| | **Pyright** | 0 errors on all src/ files | | **Semgrep** (auto + p/python + p/security-audit) | 0 findings | | **Ruff** | Clean (verified by author's CI) | --- ### Ontology Consistency All 12 Layer 2 OWL classes and 2 OWL properties verified field-by-field across TTL, Python models, and reference docs. **No mismatches** in URIs, labels, comments, or parent chains. Namespace IRIs consistent across all sources. --- ### Summary | Severity | Count | Action Required | |----------|-------|-----------------| | P0:blocker | 0 | — | | P1:must-fix | 5 | Fix before merge | | P2:should-fix | 7 | Fix in this PR or follow-up (3 day SLA) | | P3:nit | 8 | Author discretion | **Verdict: REQUEST_CHANGES** — The P1 items (missing exports, deepcopy crash, query max_length, CONTRIBUTING.md violations) must be addressed. All are straightforward fixes. The bulk of the first-round issues were resolved excellently — this is close to merge-ready. **Note:** Per playbook escalation rules, N2 (deepcopy crash affecting LangGraph) may warrant a quick check with the LangGraph subsystem owner to confirm whether `DetailLevelMap` objects appear in graph state.
hurui200320 requested changes 2026-03-12 12:41:11 +00:00
hurui200320 left a comment
Member
Copy Link
Copy Source

Consolidated Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies

Reviewer: Rui Hu (hurui200320)
PR Size: XL (~3,100+ lines across 19 files)
Review method: 5 parallel review agents (bug detection, security, test quality, type safety/performance, spec compliance + prior review verification)

Specification Compliance

All three paradigm vocabularies (uko-oo, uko-func, uko-proc) match the specification exactly:

  • OWL classes, properties, and rdfs:subClassOf hierarchies in uko.ttl align with spec lines ~42333-42422
  • DetailLevelMap insertions (CLASS_HIERARCHY at depth 3, VISIBILITY_ANNOTATED at depth 7, FULL_SOURCE shifted to 11) match spec lines ~24991-25004
  • Namespace URIs are correct
  • Functional and procedural maps correctly inherit uko-code levels without insertions

The ontology modeling, vocabulary definitions, and builder logic are well-implemented overall. Credit to @hamza.khyari for addressing the bulk of the first-round review findings thoroughly.

Prior Review Status

Brent Edwards' first-round findings (review ID 2126): All resolved. The two fix-pass commits addressed all P0/P1 items correctly.

Brent Edwards' second-round findings (review ID 2141): 12 of 19 remain unresolved (5 P1, 5 P2, 4 P3 — details below with N-prefixed IDs from that review). One finding (N7 — comment about model_validator behavior) was evaluated and found to be a non-issue (the comment is factually correct for Pydantic v2).

P1: Must Fix Before Merge (5 unresolved + 1 new)

F1. DetailLevelError / DetailLevelCycleError not exported from domain/models/acms/__init__.py (unresolved N1)
src/cleveragents/domain/models/acms/__init__.py — Both exceptions are defined in crp.py:68-73 and raised by resolve(), but neither is imported or listed in __all__. Consumers must reach into the private crp module path.
Fix: Add both to the import block and __all__ in domain/models/acms/__init__.py.

F2. deepcopy / pickle / model_copy(deep=True) crash on DetailLevelMap (unresolved N2)
src/cleveragents/domain/models/acms/crp.py:220 — The _register_lock: threading.Lock private attribute cannot be pickled or deep-copied. copy.deepcopy(map) raises TypeError: cannot pickle '_thread.lock' object. This affects any deep-copy use case including test fixtures and LangGraph state management.
Fix: Add __deepcopy__ method that creates a new instance with a fresh lock:

def __deepcopy__(self, memo: dict) -> DetailLevelMap:
    import copy
    return DetailLevelMap(
        domain=copy.deepcopy(self.domain, memo),
        parent=copy.deepcopy(self.parent, memo),
        levels=dict(self.levels),
        max_depth=self.max_depth,
    )

F3. ContextRequest.query has no max_length (unresolved N3)
src/cleveragents/domain/models/acms/crp.py:416-419 — The H5 fix added max_length=500 to list fields, but the query string (primary user-facing free-text input) was missed. A multi-gigabyte string can exhaust memory. Same applies to purpose field (line ~489).
Fix: Add max_length=10_000 (or appropriate limit) to query and max_length=2_000 to purpose.

F4. Inner-function imports violate CONTRIBUTING.md (unresolved N4)
features/steps/uko_l2_detail_level_steps.py:310, 318, 331 — Three imports inside function bodies. CONTRIBUTING.md (lines 1289-1294) absolutely forbids this (only if TYPE_CHECKING: exception). Lines 310 and 318 are also duplicates of each other.
Fix: Move all three imports to the top of the file; deduplicate DetailLevelCycleError import.

F5. Robot helper uses bare except Exception without re-raising (unresolved N5)
robot/helper_uko_layer2_paradigm.py:58, 77, 102, 159, 200 — All 5 command functions catch broad Exception, print a terse message, and return 1 without traceback. CONTRIBUTING.md (lines 496, 1382) explicitly prohibits this pattern.
Fix: Either let exceptions propagate (centralize error handling in main() only), or at minimum add traceback.print_exc() and narrow the caught exception types.

F6. __setattr__ re-freezes from raw input value, not validated self.levels (unresolved N6, elevated from P2 due to correctness risk)
src/cleveragents/domain/models/acms/crp.py:167-172 — After super().__setattr__(), Pydantic's validate_assignment may transform value before storing it. The re-freeze uses MappingProxyType(dict(value)) (the raw input) instead of MappingProxyType(dict(self.levels)) (the validated result). When a caller passes a MappingProxyType, the isinstance check fails to detect the need for re-freezing, leaving self.levels as a mutable dict from the Pydantic validator.
Fix: Check self.levels after super call:

super().__setattr__(name, value)
if name == "levels" and not isinstance(self.levels, MappingProxyType):
    object.__setattr__(self, "levels", MappingProxyType(dict(self.levels)))

P2: Should Fix (7)

F7. effective_levels() has no cycle guard — RecursionError on circular parents
src/cleveragents/domain/models/acms/crp.py:246-257 — resolve() has a visited set for cycle detection, but effective_levels() recurses through self.parent.effective_levels() with no guard. A circular parent chain causes RecursionError.
Fix: Add a visited-set guard mirroring resolve(), raising DetailLevelCycleError.

F8. __all__ sorting violations in 3 files (unresolved N9)

  • src/cleveragents/acms/uko/vocabularies.py:22-41 — ParadigmVocabulary (P) should come before UKO_* (U) in ASCII order
  • src/cleveragents/acms/uko/detail_level_maps.py:32-39 — DetailLevelMapBuilder (D) should come after CODE_* (C) and before FUNC_* (F)
  • src/cleveragents/acms/uko/__init__.py:38-52 — Same cascaded issue
    Fix: Sort all __all__ lists by ASCII order per CONTRIBUTING.md.

F9. Per-item string lengths in list fields unbounded (unresolved N15, elevated to P2)
src/cleveragents/domain/models/acms/crp.py:420-479 — entities, uko_types, focus, preferred_strategies, required_backends have max_length=500 (list cap), but individual strings have no length limit. 500 items of 10 MB each = 5 GB allocation.
Fix: Use Annotated[str, StringConstraints(max_length=2_000)] for list items.

F10. effective_levels() return type change not documented as breaking (unresolved N10)
CHANGELOG.md:17-26 — The return type changed from dict[str, int] to MappingProxyType[str, int]. Callers relying on mutation will get TypeError. Not mentioned in CHANGELOG.
Fix: Add a note: "Breaking: DetailLevelMap.effective_levels() now returns MappingProxyType (read-only) instead of dict."

F11. Missing test coverage for bad URI schemes on secondary fields (unresolved N11)
features/acms/uko_layer2_paradigm_vocabularies.feature — Tests cover non-HTTP rejection on VocabularyClass.uri and VocabularyProperty.uri, but not on parent_uris, domain_uri, range_uri, or sub_property_of.
Fix: Add 4 scenarios testing non-HTTP rejection on each secondary URI field.

F12. VocabularyRegistry.register() raises bare ValueError (unresolved N12)
src/cleveragents/acms/uko/vocabulary_registry.py:67-72 — No domain-specific exception. Per CONTRIBUTING.md's fail-fast principles, domain operations should raise specific error types.
Fix: Define DuplicateVocabularyError(ValueError) and raise it instead.

F13. Missing deepcopy and JSON roundtrip tests
No test verifies copy.deepcopy(DetailLevelMap(...)) succeeds, and no test roundtrips model_dump_json() through model_validate_json(). Both are important for the MappingProxyType + Lock combination.
Fix: Add scenarios for both deep-copy and JSON roundtrip.

P3: Nits (6)

F14. crp.py:199-200 — resolve() doesn't clamp negative integer depths. resolve(-5) returns -5. Fix: return max(0, min(depth, self.max_depth)).

F15. crp.py:158 — __setattr__ uses value: object instead of Pydantic's value: Any. Minor type inconsistency.

F16. crp.py:222-244 — register() accepts value > max_depth without warning. A level registered at depth 100 with max_depth=9 would never be resolved.

F17. vocabularies.py:241-244 — ParadigmVocabulary.iri has no URI scheme validation, unlike all other URI fields. Inconsistent.

F18. detail_level_maps.py:44,73 — _CODE_MAX_DEPTH = 9 is hardcoded separately from _CODE_LEVELS. Should derive: max(_CODE_LEVELS.values()).

F19. vocabularies.py:336,399,442 — "uko-code:" magic string repeated 3 times without referencing the _CODE_DOMAIN constant from detail_level_maps.py.

Static Analysis

Tool Result
Semgrep (auto + security) 0 findings
Pyright 0 errors
Ruff Clean (per author CI)

Summary

Severity Count Action
P1: must-fix 6 Fix before merge
P2: should-fix 7 Fix in this PR or follow-up (3 day SLA)
P3: nit 6 Author discretion

Verdict: REQUEST_CHANGES — The 6 P1 items (missing exports, deepcopy crash, unbounded query, CONTRIBUTING.md violations, __setattr__ correctness) must be resolved. Most are straightforward fixes. The spec compliance and overall code quality are strong — this is close to merge-ready.

## Consolidated Code Review — PR #657: UKO Layer 2 Paradigm Vocabularies **Reviewer:** Rui Hu (`hurui200320`) **PR Size:** XL (~3,100+ lines across 19 files) **Review method:** 5 parallel review agents (bug detection, security, test quality, type safety/performance, spec compliance + prior review verification) --- ### Specification Compliance All three paradigm vocabularies (uko-oo, uko-func, uko-proc) **match the specification exactly**: - OWL classes, properties, and `rdfs:subClassOf` hierarchies in `uko.ttl` align with spec lines ~42333-42422 - DetailLevelMap insertions (CLASS_HIERARCHY at depth 3, VISIBILITY_ANNOTATED at depth 7, FULL_SOURCE shifted to 11) match spec lines ~24991-25004 - Namespace URIs are correct - Functional and procedural maps correctly inherit uko-code levels without insertions The ontology modeling, vocabulary definitions, and builder logic are well-implemented overall. Credit to @hamza.khyari for addressing the bulk of the first-round review findings thoroughly. --- ### Prior Review Status Brent Edwards' first-round findings (review ID 2126): **All resolved.** The two fix-pass commits addressed all P0/P1 items correctly. Brent Edwards' second-round findings (review ID 2141): **12 of 19 remain unresolved** (5 P1, 5 P2, 4 P3 — details below with N-prefixed IDs from that review). One finding (N7 — comment about model_validator behavior) was evaluated and found to be a non-issue (the comment is factually correct for Pydantic v2). --- ### P1: Must Fix Before Merge (5 unresolved + 1 new) **F1. `DetailLevelError` / `DetailLevelCycleError` not exported from `domain/models/acms/__init__.py`** (unresolved N1) `src/cleveragents/domain/models/acms/__init__.py` — Both exceptions are defined in `crp.py:68-73` and raised by `resolve()`, but neither is imported or listed in `__all__`. Consumers must reach into the private `crp` module path. **Fix:** Add both to the import block and `__all__` in `domain/models/acms/__init__.py`. **F2. `deepcopy` / `pickle` / `model_copy(deep=True)` crash on `DetailLevelMap`** (unresolved N2) `src/cleveragents/domain/models/acms/crp.py:220` — The `_register_lock: threading.Lock` private attribute cannot be pickled or deep-copied. `copy.deepcopy(map)` raises `TypeError: cannot pickle '_thread.lock' object`. This affects any deep-copy use case including test fixtures and LangGraph state management. **Fix:** Add `__deepcopy__` method that creates a new instance with a fresh lock: ```python def __deepcopy__(self, memo: dict) -> DetailLevelMap: import copy return DetailLevelMap( domain=copy.deepcopy(self.domain, memo), parent=copy.deepcopy(self.parent, memo), levels=dict(self.levels), max_depth=self.max_depth, ) ``` **F3. `ContextRequest.query` has no `max_length`** (unresolved N3) `src/cleveragents/domain/models/acms/crp.py:416-419` — The H5 fix added `max_length=500` to list fields, but the `query` string (primary user-facing free-text input) was missed. A multi-gigabyte string can exhaust memory. Same applies to `purpose` field (line ~489). **Fix:** Add `max_length=10_000` (or appropriate limit) to `query` and `max_length=2_000` to `purpose`. **F4. Inner-function imports violate CONTRIBUTING.md** (unresolved N4) `features/steps/uko_l2_detail_level_steps.py:310, 318, 331` — Three imports inside function bodies. CONTRIBUTING.md (lines 1289-1294) absolutely forbids this (only `if TYPE_CHECKING:` exception). Lines 310 and 318 are also duplicates of each other. **Fix:** Move all three imports to the top of the file; deduplicate `DetailLevelCycleError` import. **F5. Robot helper uses bare `except Exception` without re-raising** (unresolved N5) `robot/helper_uko_layer2_paradigm.py:58, 77, 102, 159, 200` — All 5 command functions catch broad `Exception`, print a terse message, and return 1 without traceback. CONTRIBUTING.md (lines 496, 1382) explicitly prohibits this pattern. **Fix:** Either let exceptions propagate (centralize error handling in `main()` only), or at minimum add `traceback.print_exc()` and narrow the caught exception types. **F6. `__setattr__` re-freezes from raw input `value`, not validated `self.levels`** (unresolved N6, elevated from P2 due to correctness risk) `src/cleveragents/domain/models/acms/crp.py:167-172` — After `super().__setattr__()`, Pydantic's `validate_assignment` may transform `value` before storing it. The re-freeze uses `MappingProxyType(dict(value))` (the raw input) instead of `MappingProxyType(dict(self.levels))` (the validated result). When a caller passes a `MappingProxyType`, the `isinstance` check fails to detect the need for re-freezing, leaving `self.levels` as a mutable `dict` from the Pydantic validator. **Fix:** Check `self.levels` after super call: ```python super().__setattr__(name, value) if name == "levels" and not isinstance(self.levels, MappingProxyType): object.__setattr__(self, "levels", MappingProxyType(dict(self.levels))) ``` --- ### P2: Should Fix (7) **F7. `effective_levels()` has no cycle guard — `RecursionError` on circular parents** `src/cleveragents/domain/models/acms/crp.py:246-257` — `resolve()` has a `visited` set for cycle detection, but `effective_levels()` recurses through `self.parent.effective_levels()` with no guard. A circular parent chain causes `RecursionError`. **Fix:** Add a visited-set guard mirroring `resolve()`, raising `DetailLevelCycleError`. **F8. `__all__` sorting violations in 3 files** (unresolved N9) - `src/cleveragents/acms/uko/vocabularies.py:22-41` — `ParadigmVocabulary` (P) should come before `UKO_*` (U) in ASCII order - `src/cleveragents/acms/uko/detail_level_maps.py:32-39` — `DetailLevelMapBuilder` (D) should come after `CODE_*` (C) and before `FUNC_*` (F) - `src/cleveragents/acms/uko/__init__.py:38-52` — Same cascaded issue **Fix:** Sort all `__all__` lists by ASCII order per CONTRIBUTING.md. **F9. Per-item string lengths in list fields unbounded** (unresolved N15, elevated to P2) `src/cleveragents/domain/models/acms/crp.py:420-479` — `entities`, `uko_types`, `focus`, `preferred_strategies`, `required_backends` have `max_length=500` (list cap), but individual strings have no length limit. 500 items of 10 MB each = 5 GB allocation. **Fix:** Use `Annotated[str, StringConstraints(max_length=2_000)]` for list items. **F10. `effective_levels()` return type change not documented as breaking** (unresolved N10) `CHANGELOG.md:17-26` — The return type changed from `dict[str, int]` to `MappingProxyType[str, int]`. Callers relying on mutation will get `TypeError`. Not mentioned in CHANGELOG. **Fix:** Add a note: "**Breaking:** `DetailLevelMap.effective_levels()` now returns `MappingProxyType` (read-only) instead of `dict`." **F11. Missing test coverage for bad URI schemes on secondary fields** (unresolved N11) `features/acms/uko_layer2_paradigm_vocabularies.feature` — Tests cover non-HTTP rejection on `VocabularyClass.uri` and `VocabularyProperty.uri`, but not on `parent_uris`, `domain_uri`, `range_uri`, or `sub_property_of`. **Fix:** Add 4 scenarios testing non-HTTP rejection on each secondary URI field. **F12. `VocabularyRegistry.register()` raises bare `ValueError`** (unresolved N12) `src/cleveragents/acms/uko/vocabulary_registry.py:67-72` — No domain-specific exception. Per CONTRIBUTING.md's fail-fast principles, domain operations should raise specific error types. **Fix:** Define `DuplicateVocabularyError(ValueError)` and raise it instead. **F13. Missing `deepcopy` and JSON roundtrip tests** No test verifies `copy.deepcopy(DetailLevelMap(...))` succeeds, and no test roundtrips `model_dump_json()` through `model_validate_json()`. Both are important for the `MappingProxyType` + Lock combination. **Fix:** Add scenarios for both deep-copy and JSON roundtrip. --- ### P3: Nits (6) **F14.** `crp.py:199-200` — `resolve()` doesn't clamp negative integer depths. `resolve(-5)` returns `-5`. Fix: `return max(0, min(depth, self.max_depth))`. **F15.** `crp.py:158` — `__setattr__` uses `value: object` instead of Pydantic's `value: Any`. Minor type inconsistency. **F16.** `crp.py:222-244` — `register()` accepts `value > max_depth` without warning. A level registered at depth 100 with `max_depth=9` would never be resolved. **F17.** `vocabularies.py:241-244` — `ParadigmVocabulary.iri` has no URI scheme validation, unlike all other URI fields. Inconsistent. **F18.** `detail_level_maps.py:44,73` — `_CODE_MAX_DEPTH = 9` is hardcoded separately from `_CODE_LEVELS`. Should derive: `max(_CODE_LEVELS.values())`. **F19.** `vocabularies.py:336,399,442` — `"uko-code:"` magic string repeated 3 times without referencing the `_CODE_DOMAIN` constant from `detail_level_maps.py`. --- ### Static Analysis | Tool | Result | |------|--------| | Semgrep (auto + security) | 0 findings | | Pyright | 0 errors | | Ruff | Clean (per author CI) | --- ### Summary | Severity | Count | Action | |----------|-------|--------| | P1: must-fix | 6 | Fix before merge | | P2: should-fix | 7 | Fix in this PR or follow-up (3 day SLA) | | P3: nit | 6 | Author discretion | **Verdict: REQUEST_CHANGES** — The 6 P1 items (missing exports, deepcopy crash, unbounded query, CONTRIBUTING.md violations, `__setattr__` correctness) must be resolved. Most are straightforward fixes. The spec compliance and overall code quality are strong — this is close to merge-ready.
hamza.khyari commented 2026-03-12 13:08:18 +00:00
Author
Member
Copy Link
Copy Source

Third-Round Review Response (Reviews #2140 + #2141)

All 20 findings from Brent Edwards' third-round review have been addressed in commit 2f34170e.

P1 Fixes

ID Finding Status
N1 DetailLevelError/DetailLevelCycleError not exported from __init__.py Fixed -- added to imports, __all__, and docstring
N2 deepcopy/pickle/model_copy(deep=True) crash on DetailLevelMap Fixed -- added __copy__ and __deepcopy__ overrides
N3 query field has no max_length Fixed -- added max_length=10_000 constant
N4 Inner-function imports in uko_l2_detail_level_steps.py Fixed -- moved to top-level imports
N5 Robot helper except Exception without re-raising Fixed -- added traceback.print_exc() for diagnostic output

P2 Fixes

ID Finding Status
N6 __setattr__ re-freezes from value not self.levels Fixed -- now uses self.levels
N7 Factually incorrect comments about model_validator Fixed -- updated to reflect Pydantic 2.12+ behavior
N8 capture_error() suppresses exceptions Fixed -- added Note section documenting test-infrastructure exemption
N9 __all__ not sorted in 3 files Fixed -- applied ruff isort-style sorting
N10 effective_levels() return type change not documented Fixed -- added Breaking note to CHANGELOG
N11 Missing test coverage for bad URI schemes on secondary fields Fixed -- added 4 scenarios and 4 step definitions
N12 VocabularyRegistry raises bare ValueError Fixed -- added DuplicateVocabularyError(ValueError) subclass

P3 Fixes

ID Finding Status
N13 __setattr__ value: object vs value: Any Fixed -- changed to Any
N14 register() doesn't enforce value <= max_depth Fixed -- added check
N15 List items lack per-string max_length Fixed -- _BoundedStr = Annotated[str, StringConstraints(max_length=2000)] on all 5 list fields
N16 _validate_http_uri lacks SSRF note Fixed -- added docstring note
N17 Docs paraphrase instead of quoting rdfs:comment Fixed -- verbatim rdfs:comment values in all description columns
N18 No inline comment in uko.ttl for dual inheritance Fixed -- added 4-line comment block
N19 Docs "Layer 1 Dependencies" omits Layer 0 URIs Fixed -- renamed section to "Parent-Layer Dependencies", added 4 Layer 0 URIs
N20 Manual try/except inconsistent with capture_error() Fixed -- added inline comment explaining different pattern

Additional Improvement

  • Extracted DetailLevelMap, DetailDepth, DetailLevelError, and DetailLevelCycleError from crp.py (601 lines) into new detail_level.py (267 lines), bringing crp.py down to 364 lines. All existing import paths preserved via re-exports. This addresses the CONTRIBUTING.md 500-line file limit.

Quality Gates

  • nox -s lint -- PASS
  • nox -s typecheck -- PASS
  • behave features/acms/uko_layer2_paradigm_vocabularies.feature -- PASS
  • behave features/uko_ontology.feature -- PASS
  • behave features/crp_models.feature -- PASS
  • behave features/unified_context_models.feature -- PASS
  • behave features/depth_breadth_projection.feature -- PASS
  • behave features/context_strategy_registry.feature -- PASS
  • behave features/uko_ontology_registry.feature -- PASS
## Third-Round Review Response (Reviews #2140 + #2141) All 20 findings from Brent Edwards' third-round review have been addressed in commit `2f34170e`. ### P1 Fixes | ID | Finding | Status | |----|---------|--------| | N1 | `DetailLevelError`/`DetailLevelCycleError` not exported from `__init__.py` | Fixed -- added to imports, `__all__`, and docstring | | N2 | `deepcopy`/`pickle`/`model_copy(deep=True)` crash on `DetailLevelMap` | Fixed -- added `__copy__` and `__deepcopy__` overrides | | N3 | `query` field has no `max_length` | Fixed -- added `max_length=10_000` constant | | N4 | Inner-function imports in `uko_l2_detail_level_steps.py` | Fixed -- moved to top-level imports | | N5 | Robot helper `except Exception` without re-raising | Fixed -- added `traceback.print_exc()` for diagnostic output | ### P2 Fixes | ID | Finding | Status | |----|---------|--------| | N6 | `__setattr__` re-freezes from `value` not `self.levels` | Fixed -- now uses `self.levels` | | N7 | Factually incorrect comments about model_validator | Fixed -- updated to reflect Pydantic 2.12+ behavior | | N8 | `capture_error()` suppresses exceptions | Fixed -- added Note section documenting test-infrastructure exemption | | N9 | `__all__` not sorted in 3 files | Fixed -- applied ruff isort-style sorting | | N10 | `effective_levels()` return type change not documented | Fixed -- added Breaking note to CHANGELOG | | N11 | Missing test coverage for bad URI schemes on secondary fields | Fixed -- added 4 scenarios and 4 step definitions | | N12 | `VocabularyRegistry` raises bare `ValueError` | Fixed -- added `DuplicateVocabularyError(ValueError)` subclass | ### P3 Fixes | ID | Finding | Status | |----|---------|--------| | N13 | `__setattr__` `value: object` vs `value: Any` | Fixed -- changed to `Any` | | N14 | `register()` doesn't enforce `value <= max_depth` | Fixed -- added check | | N15 | List items lack per-string `max_length` | Fixed -- `_BoundedStr = Annotated[str, StringConstraints(max_length=2000)]` on all 5 list fields | | N16 | `_validate_http_uri` lacks SSRF note | Fixed -- added docstring note | | N17 | Docs paraphrase instead of quoting `rdfs:comment` | Fixed -- verbatim rdfs:comment values in all description columns | | N18 | No inline comment in `uko.ttl` for dual inheritance | Fixed -- added 4-line comment block | | N19 | Docs "Layer 1 Dependencies" omits Layer 0 URIs | Fixed -- renamed section to "Parent-Layer Dependencies", added 4 Layer 0 URIs | | N20 | Manual `try/except` inconsistent with `capture_error()` | Fixed -- added inline comment explaining different pattern | ### Additional Improvement - Extracted `DetailLevelMap`, `DetailDepth`, `DetailLevelError`, and `DetailLevelCycleError` from `crp.py` (601 lines) into new `detail_level.py` (267 lines), bringing `crp.py` down to 364 lines. All existing import paths preserved via re-exports. This addresses the CONTRIBUTING.md 500-line file limit. ### Quality Gates - `nox -s lint` -- PASS - `nox -s typecheck` -- PASS - `behave features/acms/uko_layer2_paradigm_vocabularies.feature` -- PASS - `behave features/uko_ontology.feature` -- PASS - `behave features/crp_models.feature` -- PASS - `behave features/unified_context_models.feature` -- PASS - `behave features/depth_breadth_projection.feature` -- PASS - `behave features/context_strategy_registry.feature` -- PASS - `behave features/uko_ontology_registry.feature` -- PASS
hamza.khyari commented 2026-03-12 13:34:41 +00:00
Author
Member
Copy Link
Copy Source

Response to Rui Hu Review #2152 (commit 49f24c9d)

Thanks for the thorough review, @hurui200320. 12 of the 19 findings overlap with Brent Edwards' reviews #2140/#2141 and were already addressed in commit 2f34170e. The remaining findings have been evaluated and resolved below.

Findings Already Fixed (12 -- overlap with Brent Edwards N1-N20)

These were resolved in commit 2f34170e (pushed before review #2152):

Rui ID Brent ID Finding Status
F1 N1 Exceptions not exported from __init__.py Fixed
F2 N2 deepcopy/pickle crash Fixed (__copy__, __deepcopy__)
F4 N4 Inner-function imports Fixed
F5 N5 Robot helper bare except Exception Fixed (traceback.print_exc())
F6 N6 __setattr__ re-freezes from raw value Fixed (uses self.levels)
F8 N9 __all__ sorting violations Fixed (ruff --fix)
F9 N15 Per-item string lengths unbounded Fixed (_BoundedStr)
F10 N10 effective_levels() return type not documented Fixed (CHANGELOG)
F11 N11 Missing URI scheme tests on secondary fields Fixed (4 scenarios)
F12 N12 Bare ValueError in registry Fixed (DuplicateVocabularyError)
F15 N13 value: object vs value: Any Fixed
F16 N14 register() accepts value > max_depth Fixed

New Findings Fixed in 49f24c9d (5)

Rui ID Sev Finding Resolution
F3 (gap) P1 purpose field missing max_length Fixed -- added max_length=_MAX_ITEM_LENGTH (2,000). query was already fixed to 10,000 in the prior commit.
F7 P2 effective_levels() has no cycle guard Fixed -- added _visited: set[int] parameter with DetailLevelCycleError on cycle detection, mirroring resolve().
F13 P2 Missing deepcopy test Fixed -- added scenario and step. Also fixed infinite recursion bug in __copy__/__deepcopy__: model_copy(deep=False) internally calls __copy__(), so we now construct via DetailLevelMap(...) directly.
F14 P3 resolve() doesn't clamp negative ints Fixed -- max(0, min(depth, self.max_depth)).
F17 P3 ParadigmVocabulary.iri no URI validation Fixed -- added _validate_http_uri field validator on iri.

Findings Kept As-Is With Rationale (2)

F18 (P3): _CODE_MAX_DEPTH = 9 hardcoded separately from _CODE_LEVELS

Kept as-is. The max_depth=9 value comes directly from the specification (spec lines 24970-24985) and is an independent semantic property of the domain -- it means "the maximum meaningful detail depth for the uko-code domain is 9." While it happens to equal max(_CODE_LEVELS.values()) today, these are conceptually distinct: max_depth is a domain constraint from the spec, not a derived property of the current level set. Deriving it would invert that relationship and silently change semantics if a level were added or removed. Hardcoding the spec value makes the spec dependency explicit.

F19 (P3): "uko-code:" string repeated 3 times in vocabularies.py

Kept as-is. _CODE_DOMAIN exists in detail_level_maps.py, but importing it into vocabularies.py would create a cross-module coupling between two sibling modules that are currently independent (vocabularies.py defines vocabulary data models; detail_level_maps.py defines builder/map logic). The string "uko-code:" is a stable specification constant (the Layer 1 domain prefix), not a value that drifts. Introducing a shared constants module for a single 10-character string adds indirection without meaningful safety gain.

Quality Gates

  • nox -s lint -- PASS
  • nox -s typecheck -- PASS
  • behave features/acms/uko_layer2_paradigm_vocabularies.feature -- PASS (105 scenarios)
  • behave features/crp_models.feature -- PASS
  • behave features/uko_ontology.feature -- PASS
  • behave features/unified_context_models.feature -- PASS
## Response to Rui Hu Review #2152 (commit `49f24c9d`) Thanks for the thorough review, @hurui200320. 12 of the 19 findings overlap with Brent Edwards' reviews #2140/#2141 and were already addressed in commit `2f34170e`. The remaining findings have been evaluated and resolved below. ### Findings Already Fixed (12 -- overlap with Brent Edwards N1-N20) These were resolved in commit `2f34170e` (pushed before review #2152): | Rui ID | Brent ID | Finding | Status | |--------|----------|---------|--------| | F1 | N1 | Exceptions not exported from `__init__.py` | Fixed | | F2 | N2 | `deepcopy`/`pickle` crash | Fixed (`__copy__`, `__deepcopy__`) | | F4 | N4 | Inner-function imports | Fixed | | F5 | N5 | Robot helper bare `except Exception` | Fixed (`traceback.print_exc()`) | | F6 | N6 | `__setattr__` re-freezes from raw value | Fixed (uses `self.levels`) | | F8 | N9 | `__all__` sorting violations | Fixed (ruff --fix) | | F9 | N15 | Per-item string lengths unbounded | Fixed (`_BoundedStr`) | | F10 | N10 | `effective_levels()` return type not documented | Fixed (CHANGELOG) | | F11 | N11 | Missing URI scheme tests on secondary fields | Fixed (4 scenarios) | | F12 | N12 | Bare `ValueError` in registry | Fixed (`DuplicateVocabularyError`) | | F15 | N13 | `value: object` vs `value: Any` | Fixed | | F16 | N14 | `register()` accepts `value > max_depth` | Fixed | ### New Findings Fixed in `49f24c9d` (5) | Rui ID | Sev | Finding | Resolution | |--------|-----|---------|------------| | F3 (gap) | P1 | `purpose` field missing `max_length` | Fixed -- added `max_length=_MAX_ITEM_LENGTH` (2,000). `query` was already fixed to 10,000 in the prior commit. | | F7 | P2 | `effective_levels()` has no cycle guard | Fixed -- added `_visited: set[int]` parameter with `DetailLevelCycleError` on cycle detection, mirroring `resolve()`. | | F13 | P2 | Missing `deepcopy` test | Fixed -- added scenario and step. Also fixed infinite recursion bug in `__copy__`/`__deepcopy__`: `model_copy(deep=False)` internally calls `__copy__()`, so we now construct via `DetailLevelMap(...)` directly. | | F14 | P3 | `resolve()` doesn't clamp negative ints | Fixed -- `max(0, min(depth, self.max_depth))`. | | F17 | P3 | `ParadigmVocabulary.iri` no URI validation | Fixed -- added `_validate_http_uri` field validator on `iri`. | ### Findings Kept As-Is With Rationale (2) **F18 (P3): `_CODE_MAX_DEPTH = 9` hardcoded separately from `_CODE_LEVELS`** Kept as-is. The `max_depth=9` value comes directly from the specification (spec lines 24970-24985) and is an independent semantic property of the domain -- it means "the maximum meaningful detail depth for the uko-code domain is 9." While it happens to equal `max(_CODE_LEVELS.values())` today, these are conceptually distinct: `max_depth` is a domain constraint from the spec, not a derived property of the current level set. Deriving it would invert that relationship and silently change semantics if a level were added or removed. Hardcoding the spec value makes the spec dependency explicit. **F19 (P3): `"uko-code:"` string repeated 3 times in `vocabularies.py`** Kept as-is. `_CODE_DOMAIN` exists in `detail_level_maps.py`, but importing it into `vocabularies.py` would create a cross-module coupling between two sibling modules that are currently independent (`vocabularies.py` defines vocabulary data models; `detail_level_maps.py` defines builder/map logic). The string `"uko-code:"` is a stable specification constant (the Layer 1 domain prefix), not a value that drifts. Introducing a shared constants module for a single 10-character string adds indirection without meaningful safety gain. ### Quality Gates - `nox -s lint` -- PASS - `nox -s typecheck` -- PASS - `behave features/acms/uko_layer2_paradigm_vocabularies.feature` -- PASS (105 scenarios) - `behave features/crp_models.feature` -- PASS - `behave features/uko_ontology.feature` -- PASS - `behave features/unified_context_models.feature` -- PASS
freemo added the
MoSCoW
Must have
Points
13
Priority
High
State
In Progress
 labels 2026-03-12 20:43:48 +00:00
freemo commented 2026-03-12 20:45:24 +00:00
Owner
Copy Link
Copy Source

PM Status — Day 32

Review Progress

This PR has been through 3 review rounds with thorough findings from @brent.edwards (2 rounds) and @hurui200320 (1 round):

Round Reviewer Verdict P0 P1 P2 P3
1 Brent REQUEST_CHANGES 1 4 13 10
2 Brent REQUEST_CHANGES 0 5 7 8
3 Rui REQUEST_CHANGES 0 6 7 6

Current Blocking Items (consolidated from Rui's review)

6 P1 findings remain unresolved:

  1. F1: DetailLevelError/DetailLevelCycleError not exported from __init__.py
  2. F2: deepcopy/pickle crash on DetailLevelMap (threading.Lock)
  3. F3: ContextRequest.query has no max_length
  4. F4: Inner-function imports violate CONTRIBUTING.md
  5. F5: Robot helper uses bare except Exception without re-raising
  6. F6: __setattr__ re-freezes from raw input, not validated value

Labels Added

  • Priority/High, MoSCoW/Must have, Points/13, State/In Progress

Action Required

@hamza.khyari: Please address the 6 P1 findings from Rui's review (#2152). Most are straightforward fixes (import moves, max_length addition, exception export). This PR is in an overdue milestone (M6, due Mar 10) and blocks ACMS pipeline completeness. The sooner these are resolved, the sooner we can approve and merge.

## PM Status — Day 32 ### Review Progress This PR has been through **3 review rounds** with thorough findings from @brent.edwards (2 rounds) and @hurui200320 (1 round): | Round | Reviewer | Verdict | P0 | P1 | P2 | P3 | |-------|----------|---------|----|----|----|----| | 1 | Brent | REQUEST_CHANGES | 1 | 4 | 13 | 10 | | 2 | Brent | REQUEST_CHANGES | 0 | 5 | 7 | 8 | | 3 | Rui | REQUEST_CHANGES | 0 | 6 | 7 | 6 | ### Current Blocking Items (consolidated from Rui's review) 6 P1 findings remain unresolved: 1. **F1**: `DetailLevelError`/`DetailLevelCycleError` not exported from `__init__.py` 2. **F2**: `deepcopy`/`pickle` crash on `DetailLevelMap` (threading.Lock) 3. **F3**: `ContextRequest.query` has no `max_length` 4. **F4**: Inner-function imports violate CONTRIBUTING.md 5. **F5**: Robot helper uses bare `except Exception` without re-raising 6. **F6**: `__setattr__` re-freezes from raw input, not validated value ### Labels Added - Priority/High, MoSCoW/Must have, Points/13, State/In Progress ### Action Required **@hamza.khyari**: Please address the 6 P1 findings from Rui's review (#2152). Most are straightforward fixes (import moves, max_length addition, exception export). This PR is in an overdue milestone (M6, due Mar 10) and blocks ACMS pipeline completeness. The sooner these are resolved, the sooner we can approve and merge.
aditya approved these changes 2026-03-13 15:58:03 +00:00
Dismissed
hamza.khyari commented 2026-03-13 18:13:33 +00:00
Author
Member
Copy Link
Copy Source

Blockers Status -- All 6 P1 Items Already Resolved

@freemo -- all 6 P1 findings listed in your Day 32 status are already fixed. They were addressed in commits 2f34170e (pushed 13:08 UTC) and 49f24c9d (pushed 13:34 UTC), both before your comment.

Details in response comments #59807 (Brent's N1-N20) and #59861 (Rui's F1-F19).

Blocker Fix Commit Resolution
F1: Exceptions not exported 2f34170e Added to imports, __all__, docstring in acms/__init__.py
F2: deepcopy/pickle crash 2f34170e + 49f24c9d Added __copy__/__deepcopy__; fixed model_copy recursion
F3: query no max_length 2f34170e + 49f24c9d query: max_length=10_000; purpose: max_length=2_000
F4: Inner-function imports 2f34170e Moved to top-level imports, deduplicated
F5: Bare except Exception 2f34170e Added traceback.print_exc() to all 5 handlers
F6: setattr raw value 2f34170e Now re-freezes from self.levels (validated) not raw value

All quality gates pass (lint, typecheck, 105 behave scenarios). Ready for re-review or merge approval.

## Blockers Status -- All 6 P1 Items Already Resolved @freemo -- all 6 P1 findings listed in your Day 32 status are already fixed. They were addressed in commits `2f34170e` (pushed 13:08 UTC) and `49f24c9d` (pushed 13:34 UTC), both before your comment. Details in response comments #59807 (Brent's N1-N20) and #59861 (Rui's F1-F19). | Blocker | Fix Commit | Resolution | |---------|------------|------------| | F1: Exceptions not exported | `2f34170e` | Added to imports, `__all__`, docstring in `acms/__init__.py` | | F2: deepcopy/pickle crash | `2f34170e` + `49f24c9d` | Added `__copy__`/`__deepcopy__`; fixed model_copy recursion | | F3: query no max_length | `2f34170e` + `49f24c9d` | query: `max_length=10_000`; purpose: `max_length=2_000` | | F4: Inner-function imports | `2f34170e` | Moved to top-level imports, deduplicated | | F5: Bare except Exception | `2f34170e` | Added `traceback.print_exc()` to all 5 handlers | | F6: __setattr__ raw value | `2f34170e` | Now re-freezes from `self.levels` (validated) not raw `value` | All quality gates pass (lint, typecheck, 105 behave scenarios). Ready for re-review or merge approval.
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from 49f24c9d9c to ad76dcdc8d 2026-03-13 18:19:24 +00:00 Compare
hamza.khyari dismissed aditya's review 2026-03-13 18:19:24 +00:00
Reason:

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

hamza.khyari scheduled this pull request to auto merge when all checks succeed 2026-03-13 18:19:54 +00:00
hamza.khyari canceled auto merging this pull request when all checks succeed 2026-03-13 18:20:01 +00:00
hamza.khyari force-pushed feature/m6-uko-layer2-paradigm-vocabularies from ad76dcdc8d to 3c014a9565 2026-03-13 18:22:03 +00:00 Compare
hamza.khyari scheduled this pull request to auto merge when all checks succeed 2026-03-13 18:22:12 +00:00
hamza.khyari merged commit 232ef965f5 into master 2026-03-13 18:28:12 +00:00
hamza.khyari deleted branch feature/m6-uko-layer2-paradigm-vocabularies 2026-03-13 19:21:25 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
brent.edwards
hurui200320
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
 controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
 overdue
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
MoSCoW
Must have
Points
13
Priority
High
State
In Progress
Type
Feature
Milestone
No items
No Milestone v3.5.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
No Assignees hamza.khyari
5 Participants
Notifications
Due Date
No due date set.
Blocks
#575 feat(acms): implement UKO Layer 2 Paradigm Vocabularies (uko-oo, uko-func, uko-proc)
cleveragents/cleveragents-core
Reference: cleveragents/cleveragents-core#657
Delete Branch "feature/m6-uko-layer2-paradigm-vocabularies"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?