cleveragents/cleveragents-core

Fork 3

BUG-HUNT: [consistency] Inconsistent validation logic for project names and namespaces #1276

New issue

Open

opened 2026-04-02 08:31:57 +00:00 by freemo · 1 comment

freemo commented

2026-04-02 08:31:57 +00:00

Owner

Bug Report: [consistency] — Inconsistent validation logic for project names and namespaces

Severity Assessment

Impact: The system may have an inconsistent understanding of what constitutes a valid project name. A name that is successfully parsed by parse_namespaced_name could be rejected by the NamespacedProject model's validators, leading to unexpected errors.
Likelihood: High. Any code path that constructs a NamespacedProject directly without using the parse_namespaced_name function could trigger this inconsistency.
Priority: Medium

Location

File: src/cleveragents/domain/models/core/project.py
Function/Class: parse_namespaced_name and NamespacedProject
Lines: 113-161 and 397-428

Description

There are two different implementations of validation logic for project names and namespaces in the same file:

The parse_namespaced_name function uses a set of regular expressions (_SERVER_NS_NAME_RE, _BARE_NAME_RE) to parse and validate a full namespaced string.
The NamespacedProject Pydantic model has @field_validator decorators on its name and namespace fields, which use a different, simpler regex (_BARE_NAME_RE) to validate the individual parts of the name.

This leads to inconsistent behavior. For example, the parser function is designed to handle full strings like "local/my-project", while the model's field validators treat name and namespace as simple, non-compound strings. This violates the DRY (Don't Repeat Yourself) principle and can lead to subtle bugs.

Evidence

# Parser function regex
_SERVER_NS_NAME_RE = re.compile(
    r"^(?:(?P<server>[a-zA-Z][a-zA-Z0-9_-]*):)?"
    r"(?P<namespace>[a-zA-Z][a-zA-Z0-9_-]*)/"
    r"(?P<name>[a-zA-Z][a-zA-Z0-9_-]*)$"
)

# Model validator regex
_BARE_NAME_RE = re.compile(r"^[a-zA-Z][a-zA-Z0-9_-]*$")

class NamespacedProject(BaseModel):
    # ...
    @field_validator("name")
    @classmethod
    def validate_name(cls: type[NamespacedProject], v: str) -> str:
        if not _BARE_NAME_RE.match(v): # Uses the simpler regex
            raise ValueError(...)
        return v

    @field_validator("namespace")
    @classmethod
    def validate_namespace(cls: type[NamespacedProject], v: str) -> str:
        if not _BARE_NAME_RE.match(v): # Uses the simpler regex
            raise ValueError(...)
        # ...
        return v

Expected Behavior

There should be a single source of truth for validating project names and namespaces. The validation logic should be consolidated to ensure that a name is treated the same way throughout the system.

Actual Behavior

The validation logic is duplicated and inconsistent, creating the potential for bugs and making the code harder to maintain.

Suggested Fix

Refactor the validation logic to have a single source of truth. A good approach would be:

Remove the @field_validator decorators for name and namespace from the NamespacedProject model.
Add a @model_validator(mode='after') to the NamespacedProject model.
In this new model validator, construct the full namespaced name from the model's fields and then call the parse_namespaced_name function to perform the validation. This would ensure that the same logic is used everywhere.

Alternatively, make parse_namespaced_name the only way to construct a NamespacedProject.

Metadata

Branch: fix/consistency-namespaced-project-validation-single-source-of-truth
Commit Message: fix(consistency): consolidate NamespacedProject validation to use parse_namespaced_name as single source of truth
Milestone: v3.3.0
Parent Epic: #362

Subtasks

Write a TDD issue-capture Behave scenario tagged @tdd_issue, @tdd_expected_fail that demonstrates the inconsistency (e.g., a value accepted by parse_namespaced_name is rejected by NamespacedProject field validators, or vice versa)
Evaluate the two suggested fix approaches (model validator delegating to parse_namespaced_name, or factory-only construction) and select the most appropriate one
Remove the @field_validator decorators for name and namespace from NamespacedProject (lines 397–428 of project.py)
Add a @model_validator(mode='after') to NamespacedProject that reconstructs the full namespaced string and delegates to parse_namespaced_name for validation
Update all call sites that construct NamespacedProject directly to ensure they remain compatible with the new validation approach
Remove the @tdd_expected_fail tag from the TDD scenario once the fix is in place and verify it passes
Run nox -e typecheck to confirm no type regressions
Run nox -e unit_tests to confirm all Behave tests pass
Run nox -e coverage_report to confirm coverage remains ≥ 97%

Definition of Done

The @tdd_expected_fail TDD capture scenario is committed first and demonstrates the validation inconsistency
The @field_validator decorators for name and namespace are removed from NamespacedProject
A @model_validator(mode='after') (or equivalent) delegates to parse_namespaced_name as the single source of truth
The TDD scenario passes without @tdd_expected_fail after the fix
No regressions in existing project-related tests
All nox stages pass (nox -e lint, nox -e typecheck, nox -e unit_tests, nox -e integration_tests, nox -e coverage_report)
Coverage >= 97%
Commit is on branch fix/consistency-namespaced-project-validation-single-source-of-truth with message fix(consistency): consolidate NamespacedProject validation to use parse_namespaced_name as single source of truth
PR is merged and this issue is closed

## Bug Report: [consistency] — Inconsistent validation logic for project names and namespaces ### Severity Assessment - **Impact**: The system may have an inconsistent understanding of what constitutes a valid project name. A name that is successfully parsed by `parse_namespaced_name` could be rejected by the `NamespacedProject` model's validators, leading to unexpected errors. - **Likelihood**: High. Any code path that constructs a `NamespacedProject` directly without using the `parse_namespaced_name` function could trigger this inconsistency. - **Priority**: Medium ### Location - **File**: `src/cleveragents/domain/models/core/project.py` - **Function/Class**: `parse_namespaced_name` and `NamespacedProject` - **Lines**: 113-161 and 397-428 ### Description There are two different implementations of validation logic for project names and namespaces in the same file: 1. The `parse_namespaced_name` function uses a set of regular expressions (`_SERVER_NS_NAME_RE`, `_BARE_NAME_RE`) to parse and validate a full namespaced string. 2. The `NamespacedProject` Pydantic model has `@field_validator` decorators on its `name` and `namespace` fields, which use a different, simpler regex (`_BARE_NAME_RE`) to validate the individual parts of the name. This leads to inconsistent behavior. For example, the parser function is designed to handle full strings like `"local/my-project"`, while the model's field validators treat `name` and `namespace` as simple, non-compound strings. This violates the DRY (Don't Repeat Yourself) principle and can lead to subtle bugs. ### Evidence ```python # Parser function regex _SERVER_NS_NAME_RE = re.compile( r"^(?:(?P<server>[a-zA-Z][a-zA-Z0-9_-]*):)?" r"(?P<namespace>[a-zA-Z][a-zA-Z0-9_-]*)/" r"(?P<name>[a-zA-Z][a-zA-Z0-9_-]*)$" ) # Model validator regex _BARE_NAME_RE = re.compile(r"^[a-zA-Z][a-zA-Z0-9_-]*$") class NamespacedProject(BaseModel): # ... @field_validator("name") @classmethod def validate_name(cls: type[NamespacedProject], v: str) -> str: if not _BARE_NAME_RE.match(v): # Uses the simpler regex raise ValueError(...) return v @field_validator("namespace") @classmethod def validate_namespace(cls: type[NamespacedProject], v: str) -> str: if not _BARE_NAME_RE.match(v): # Uses the simpler regex raise ValueError(...) # ... return v ``` ### Expected Behavior There should be a single source of truth for validating project names and namespaces. The validation logic should be consolidated to ensure that a name is treated the same way throughout the system. ### Actual Behavior The validation logic is duplicated and inconsistent, creating the potential for bugs and making the code harder to maintain. ### Suggested Fix Refactor the validation logic to have a single source of truth. A good approach would be: 1. Remove the `@field_validator` decorators for `name` and `namespace` from the `NamespacedProject` model. 2. Add a `@model_validator(mode='after')` to the `NamespacedProject` model. 3. In this new model validator, construct the full namespaced name from the model's fields and then call the `parse_namespaced_name` function to perform the validation. This would ensure that the same logic is used everywhere. Alternatively, make `parse_namespaced_name` the only way to construct a `NamespacedProject`. ### Category consistency --- ## Metadata - **Branch**: `fix/consistency-namespaced-project-validation-single-source-of-truth` - **Commit Message**: `fix(consistency): consolidate NamespacedProject validation to use parse_namespaced_name as single source of truth` - **Milestone**: v3.3.0 - **Parent Epic**: #362 ## Subtasks - [ ] Write a TDD issue-capture Behave scenario tagged `@tdd_issue`, `@tdd_expected_fail` that demonstrates the inconsistency (e.g., a value accepted by `parse_namespaced_name` is rejected by `NamespacedProject` field validators, or vice versa) - [ ] Evaluate the two suggested fix approaches (model validator delegating to `parse_namespaced_name`, or factory-only construction) and select the most appropriate one - [ ] Remove the `@field_validator` decorators for `name` and `namespace` from `NamespacedProject` (lines 397–428 of `project.py`) - [ ] Add a `@model_validator(mode='after')` to `NamespacedProject` that reconstructs the full namespaced string and delegates to `parse_namespaced_name` for validation - [ ] Update all call sites that construct `NamespacedProject` directly to ensure they remain compatible with the new validation approach - [ ] Remove the `@tdd_expected_fail` tag from the TDD scenario once the fix is in place and verify it passes - [ ] Run `nox -e typecheck` to confirm no type regressions - [ ] Run `nox -e unit_tests` to confirm all Behave tests pass - [ ] Run `nox -e coverage_report` to confirm coverage remains ≥ 97% ## Definition of Done - [ ] The `@tdd_expected_fail` TDD capture scenario is committed first and demonstrates the validation inconsistency - [ ] The `@field_validator` decorators for `name` and `namespace` are removed from `NamespacedProject` - [ ] A `@model_validator(mode='after')` (or equivalent) delegates to `parse_namespaced_name` as the single source of truth - [ ] The TDD scenario passes without `@tdd_expected_fail` after the fix - [ ] No regressions in existing project-related tests - [ ] All nox stages pass (`nox -e lint`, `nox -e typecheck`, `nox -e unit_tests`, `nox -e integration_tests`, `nox -e coverage_report`) - [ ] Coverage >= 97% - [ ] Commit is on branch `fix/consistency-namespaced-project-validation-single-source-of-truth` with message `fix(consistency): consolidate NamespacedProject validation to use parse_namespaced_name as single source of truth` - [ ] PR is merged and this issue is closed

freemo added the

labels

2026-04-02 08:32:40 +00:00

freemo added this to the v3.3.0 milestone

2026-04-02 08:32:56 +00:00

freemo commented

2026-04-02 08:33:18 +00:00

Author

Owner

Dependency Note: This issue is a child of Epic #362 (Security & Safety Hardening). Per the project's dependency tracking convention, this issue blocks #362 — the Epic cannot be considered complete until this bug is resolved and merged.

This issue is independent of any currently open issue and does not block any other in-progress work.

**Dependency Note**: This issue is a child of Epic #362 (Security & Safety Hardening). Per the project's dependency tracking convention, this issue **blocks** #362 — the Epic cannot be considered complete until this bug is resolved and merged. This issue is **independent** of any currently open issue and does not block any other in-progress work.

freemo self-assigned this

2026-04-02 18:45:26 +00:00

freemo removed the