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

fix(project): show created project after creation #593

Merged
hurui200320 merged 1 commits from feature/m3-fix-project-show-after-create into master 2026-03-09 07:56:39 +00:00
Conversation 16 Commits 1 Files Changed 6
+405 -2
brent.edwards commented 2026-03-05 01:35:34 +00:00
Member
Copy Link
Copy Source

Summary

Fix agents project show not finding a project immediately after creation. Extends the session.commit() fix from #589 to also cover update() and delete() in NamespacedProjectRepository.

Changes

Production fix (src/cleveragents/infrastructure/database/repositories.py):

  • Add session.commit() to create(), update(), and delete() methods
  • Add finally: session.close() guard to all three methods
  • Update class docstring to reflect commit-per-method pattern

Tests & benchmarks:

  • 3 Behave BDD regression scenarios (features/project_show_after_create.feature)
  • Robot Framework integration smoke tests with "not found" assertion (robot/project_show_after_create.robot)
  • ASV benchmarks for create-then-show round-trip (benchmarks/project_show_after_create_bench.py)

Review feedback addressed

  • F1: Removed unrelated em-dash CHANGELOG edits — wrote clean entry from scratch
  • F2: Kept Suite Setup/Teardown (required for ${PYTHON} variable); updated stale docs
  • F3: Added "not found" string assertion to Robot negative test case
  • F4: Removed redundant Base.metadata.create_all() from _make_fresh_repo() helper
  • Updated all stale TDD "expected to fail" comments — this PR includes the fix

Process

  • Single squashed commit, rebased onto master (no merge commits)
  • Prescribed commit message from issue #590 metadata

ISSUES CLOSED: #590

## Summary Fix `agents project show` not finding a project immediately after creation. Extends the `session.commit()` fix from #589 to also cover `update()` and `delete()` in `NamespacedProjectRepository`. ## Changes **Production fix** (`src/cleveragents/infrastructure/database/repositories.py`): - Add `session.commit()` to `create()`, `update()`, and `delete()` methods - Add `finally: session.close()` guard to all three methods - Update class docstring to reflect commit-per-method pattern **Tests & benchmarks**: - 3 Behave BDD regression scenarios (`features/project_show_after_create.feature`) - Robot Framework integration smoke tests with "not found" assertion (`robot/project_show_after_create.robot`) - ASV benchmarks for create-then-show round-trip (`benchmarks/project_show_after_create_bench.py`) ## Review feedback addressed - **F1**: Removed unrelated em-dash CHANGELOG edits — wrote clean entry from scratch - **F2**: Kept Suite Setup/Teardown (required for `${PYTHON}` variable); updated stale docs - **F3**: Added "not found" string assertion to Robot negative test case - **F4**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()` helper - Updated all stale TDD "expected to fail" comments — this PR includes the fix ## Process - Single squashed commit, rebased onto `master` (no merge commits) - Prescribed commit message from issue #590 metadata ISSUES CLOSED: #590
brent.edwards added the
Type
Testing
 label 2026-03-05 01:35:52 +00:00
brent.edwards added this to the v3.2.0 milestone 2026-03-05 01:36:00 +00:00
brent.edwards reviewed 2026-03-05 01:36:43 +00:00
brent.edwards left a comment
Author
Member
Copy Link
Copy Source

Self-Review: TDD Failing Tests for Bug #590

Overview

This PR adds TDD failing tests that reproduce the project show bug where a project just created cannot be found (same root cause as #589: flush without commit).

Checklist

  • Feature file has @wip tag on all scenarios (for CI filtering)
  • Feature file has @tdd and @bug590 tags for traceability
  • Step patterns use unique prefixes (via the project-show CLI, project-show output, project-show exit code) to avoid AmbiguousStep collisions
  • Test design uses file-based SQLite with fresh repo per invocation to reproduce the bug
  • nox -s lint passes
  • nox -s typecheck passes (0 errors)
  • Commit uses Refs: #590 (not ISSUES CLOSED:) to avoid premature closure
  • Robot Framework smoke tests follow common.resource pattern
  • ASV benchmark includes track_show_after_create_found metric
  • CHANGELOG entry added
  • Scenario 3 (show nonexistent project) tests existing error handling and should pass

TDD Nature

Scenarios 1 and 2 are expected to fail until the fix is applied. Scenario 3 should pass since the error path for truly nonexistent projects works correctly.

No issues found. Ready for external review.

## Self-Review: TDD Failing Tests for Bug #590 ### Overview This PR adds TDD failing tests that reproduce the `project show` bug where a project just created cannot be found (same root cause as #589: flush without commit). ### Checklist - [x] Feature file has `@wip` tag on all scenarios (for CI filtering) - [x] Feature file has `@tdd` and `@bug590` tags for traceability - [x] Step patterns use unique prefixes (`via the project-show CLI`, `project-show output`, `project-show exit code`) to avoid `AmbiguousStep` collisions - [x] Test design uses file-based SQLite with fresh repo per invocation to reproduce the bug - [x] `nox -s lint` passes - [x] `nox -s typecheck` passes (0 errors) - [x] Commit uses `Refs: #590` (not `ISSUES CLOSED:`) to avoid premature closure - [x] Robot Framework smoke tests follow `common.resource` pattern - [x] ASV benchmark includes `track_show_after_create_found` metric - [x] CHANGELOG entry added - [x] Scenario 3 (show nonexistent project) tests existing error handling and should pass ### TDD Nature Scenarios 1 and 2 are expected to **fail** until the fix is applied. Scenario 3 should pass since the error path for truly nonexistent projects works correctly. No issues found. Ready for external review.
freemo added the
Priority
Medium
State
In Review
 labels 2026-03-05 03:28:36 +00:00
freemo self-assigned this 2026-03-05 03:32:34 +00:00
freemo reviewed 2026-03-05 03:35:58 +00:00
freemo left a comment
Owner
Copy Link
Copy Source

PM Review — Day 25

Status

  • Mergeable — no conflicts.
  • Self-review from Brent only. No external review.

Context

TDD bug-fix test PR for #590 (project show fails after creation). Same root cause as #589 — the session.commit() fix will resolve both. Tests verify the show command path specifically.

Action Item

@freemo: Please review alongside #591. The fix for #589 will make these tests pass as well.

Priority

Medium — blocks M3 acceptance gate.

## PM Review — Day 25 ### Status - **Mergeable** — no conflicts. - Self-review from Brent only. **No external review.** ### Context TDD bug-fix test PR for #590 (project show fails after creation). Same root cause as #589 — the `session.commit()` fix will resolve both. Tests verify the `show` command path specifically. ### Action Item **@freemo**: Please review alongside #591. The fix for #589 will make these tests pass as well. ### Priority Medium — blocks M3 acceptance gate.
freemo force-pushed feature/m3-fix-project-show-after-create from e1277569aa to d262b0d03b 2026-03-05 06:18:04 +00:00 Compare
CoreRasurae requested changes 2026-03-05 16:03:59 +00:00
CoreRasurae left a comment
Member
Copy Link
Copy Source

Code Review -- Commit 070cc1ee (Brent) + d262b0d0 (Fix)

Reviewed against: Issue #590, branch feature/m3-fix-project-show-after-create, specification docs/specification.md

Overview

The TDD approach is solid: the Behave scenarios correctly reproduce the bug by using file-based SQLite with a fresh NamespacedProjectRepository per invocation, faithfully simulating real CLI session isolation. The Robot tests add real-subprocess integration coverage and the ASV benchmark provides a useful regression tracker. The root-cause analysis (flush without commit) is accurate and well-documented.

However, there are 3 medium-severity issues that should be addressed before merge, along with several lower-severity improvements. Details follow as inline comments on the affected lines and in the summary below.

Findings Summary

ID Severity Category Description
F1 Medium Test Flaw Create step discards runner.invoke() result -- silent create failures misattribute test outcomes
F2 Medium Test Flaw Robot tests don't verify init/create exit codes -- failure at setup silently passes
F3 Medium Bug Benchmark track_show_after_create_found uses fixed name "bench-show-track" -- fails on repeated ASV invocations due to IntegrityError
F4 Medium Existing Bug @database_retry decorator wastes ~1s retrying ProjectNotFoundError (inherits DatabaseError) -- identified by Brent in issue comments but not tested or addressed
F5 Low Bug Benchmark except Exception: pass swallows ALL exceptions, not just the expected ProjectNotFoundError
F6 Low Robustness Robot tests don't pass --format plain -- ANSI codes may interfere with Should Contain assertions in some environments
F7 Low Coverage Gap Robot suite missing nonexistent-project negative test (Behave has it in Scenario 3)
F8 Low Documentation Feature file line 1 comment "expected to fail until the fix is applied" is stale after fix commit d262b0d0 removed @wip tags
F9 Low Coverage Gap Tests don't validate spec-required output structure (per docs/specification.md:3166-3405, project show should display Project Details, Linked Resources, Validations, Context, Indexing Status, and Active Plans sections)

F4 -- Detail (No inline location -- existing codebase)

ProjectNotFoundError inherits from DatabaseError (repositories.py:2782). The @database_retry decorator (retry_patterns.py:122-126) retries on DatabaseError with 3 attempts and 0.5s fixed wait. Although get() does except ProjectNotFoundError: raise, the tenacity decorator catches the re-raised exception and retries.

This means every legitimate "not found" query wastes ~1s (3 x 0.5s). This was identified in issue comment #53833 but neither tested nor fixed here. Recommend a follow-up ticket to either:

  • Exclude ProjectNotFoundError from the retry config, or
  • Wrap get() so ProjectNotFoundError is raised outside the retry boundary

F7 -- Detail (Missing test case)

The Behave suite includes Scenario 3 ("Show returns error for a project that does not exist"), but the Robot suite has no equivalent. Since Robot tests exercise real subprocess behavior (no mocks), this is a meaningful gap.

F9 -- Detail (Spec compliance)

Per docs/specification.md:3166-3405, agents project show output should include: Project Details (Name, Description, Resources, Remote, Created), Linked Resources, Validations, Context, Indexing Status, and Active Plans. The Behave tests only check for project name and description text presence without validating the output structure matches the spec contract.

# Code Review -- Commit `070cc1ee` (Brent) + `d262b0d0` (Fix) **Reviewed against:** Issue #590, branch `feature/m3-fix-project-show-after-create`, specification `docs/specification.md` ## Overview The TDD approach is solid: the Behave scenarios correctly reproduce the bug by using file-based SQLite with a fresh `NamespacedProjectRepository` per invocation, faithfully simulating real CLI session isolation. The Robot tests add real-subprocess integration coverage and the ASV benchmark provides a useful regression tracker. The root-cause analysis (flush without commit) is accurate and well-documented. However, there are **3 medium-severity issues** that should be addressed before merge, along with several lower-severity improvements. Details follow as inline comments on the affected lines and in the summary below. --- ## Findings Summary | ID | Severity | Category | Description | |----|----------|----------|-------------| | **F1** | Medium | Test Flaw | Create step discards `runner.invoke()` result -- silent create failures misattribute test outcomes | | **F2** | Medium | Test Flaw | Robot tests don't verify `init`/`create` exit codes -- failure at setup silently passes | | **F3** | Medium | Bug | Benchmark `track_show_after_create_found` uses fixed name `"bench-show-track"` -- fails on repeated ASV invocations due to `IntegrityError` | | **F4** | Medium | Existing Bug | `@database_retry` decorator wastes ~1s retrying `ProjectNotFoundError` (inherits `DatabaseError`) -- identified by Brent in issue comments but not tested or addressed | | **F5** | Low | Bug | Benchmark `except Exception: pass` swallows ALL exceptions, not just the expected `ProjectNotFoundError` | | **F6** | Low | Robustness | Robot tests don't pass `--format plain` -- ANSI codes may interfere with `Should Contain` assertions in some environments | | **F7** | Low | Coverage Gap | Robot suite missing nonexistent-project negative test (Behave has it in Scenario 3) | | **F8** | Low | Documentation | Feature file line 1 comment "expected to fail until the fix is applied" is stale after fix commit `d262b0d0` removed `@wip` tags | | **F9** | Low | Coverage Gap | Tests don't validate spec-required output structure (per `docs/specification.md:3166-3405`, `project show` should display Project Details, Linked Resources, Validations, Context, Indexing Status, and Active Plans sections) | --- ## F4 -- Detail (No inline location -- existing codebase) `ProjectNotFoundError` inherits from `DatabaseError` (`repositories.py:2782`). The `@database_retry` decorator (`retry_patterns.py:122-126`) retries on `DatabaseError` with 3 attempts and 0.5s fixed wait. Although `get()` does `except ProjectNotFoundError: raise`, the tenacity decorator catches the re-raised exception and retries. This means every legitimate "not found" query wastes ~1s (3 x 0.5s). This was identified in issue comment #53833 but neither tested nor fixed here. Recommend a follow-up ticket to either: - Exclude `ProjectNotFoundError` from the retry config, or - Wrap `get()` so `ProjectNotFoundError` is raised outside the retry boundary ## F7 -- Detail (Missing test case) The Behave suite includes Scenario 3 ("Show returns error for a project that does not exist"), but the Robot suite has no equivalent. Since Robot tests exercise real subprocess behavior (no mocks), this is a meaningful gap. ## F9 -- Detail (Spec compliance) Per `docs/specification.md:3166-3405`, `agents project show` output should include: Project Details (Name, Description, Resources, Remote, Created), Linked Resources, Validations, Context, Indexing Status, and Active Plans. The Behave tests only check for project name and description text presence without validating the output structure matches the spec contract.
benchmarks/project_show_after_create_bench.py Outdated
@@ -0,0 +78,4 @@
show_repo = _fresh_repo(self._db_path)
try:
show_repo.get(f"local/{name}")
except Exception:
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F5 [Low -- Bug]: Bare except Exception: pass swallows all errors, not just the expected ProjectNotFoundError. A ConnectionError, IntegrityError, or TypeError during benchmarking would be silently masked, making failures undiagnosable.

Suggested fix:

from cleveragents.infrastructure.database.repositories import ProjectNotFoundError
...
except ProjectNotFoundError:
    pass  # Expected while bug is present
**F5 [Low -- Bug]:** Bare `except Exception: pass` swallows **all** errors, not just the expected `ProjectNotFoundError`. A `ConnectionError`, `IntegrityError`, or `TypeError` during benchmarking would be silently masked, making failures undiagnosable. **Suggested fix:** ```python from cleveragents.infrastructure.database.repositories import ProjectNotFoundError ... except ProjectNotFoundError: pass # Expected while bug is present ```
benchmarks/project_show_after_create_bench.py Outdated
@@ -0,0 +87,4 @@
Returns 1 when create properly commits (fix applied); 0 while
bug #590 is present and get raises ProjectNotFoundError.
"""
parsed = parse_namespaced_name("bench-show-track")
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F3 [Medium -- Bug]: This method always creates a project named "bench-show-track" (hardcoded). ASV calls setup() once per suite instance, then may invoke each benchmark method multiple times for statistical sampling. On the second invocation, create() will hit the UNIQUE constraint on namespaced_name and raise IntegrityError, producing either a false 0 return or an outright crash.

Compare with time_create_then_show (line 68) which correctly uses self._counter for unique names.

Suggested fix:

def setup(self) -> None:
    ...
    self._track_counter = 0

def track_show_after_create_found(self) -> int:
    self._track_counter += 1
    name = f"bench-show-track-{self._track_counter}"
    parsed = parse_namespaced_name(name)
    ...
    show_repo.get(f"local/{name}")
**F3 [Medium -- Bug]:** This method always creates a project named `"bench-show-track"` (hardcoded). ASV calls `setup()` once per suite instance, then may invoke each benchmark method multiple times for statistical sampling. On the second invocation, `create()` will hit the UNIQUE constraint on `namespaced_name` and raise `IntegrityError`, producing either a false `0` return or an outright crash. Compare with `time_create_then_show` (line 68) which correctly uses `self._counter` for unique names. **Suggested fix:** ```python def setup(self) -> None: ... self._track_counter = 0 def track_show_after_create_found(self) -> int: self._track_counter += 1 name = f"bench-show-track-{self._track_counter}" parsed = parse_namespaced_name(name) ... show_repo.get(f"local/{name}") ```
features/project_show_after_create.feature Outdated
@@ -0,0 +1,29 @@
# These tests target bug #590 and are expected to fail until the fix is applied.
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F8 [Low -- Documentation]: This comment says tests are "expected to fail until the fix is applied," but the fix commit (d262b0d0) already removed the @wip tags from the scenarios below. After the fix, this comment is stale and misleading.

Suggested fix: Update to reflect current state:

# Regression tests for bug #590: project show must find a project immediately after creation.
**F8 [Low -- Documentation]:** This comment says tests are "expected to fail until the fix is applied," but the fix commit (`d262b0d0`) already removed the `@wip` tags from the scenarios below. After the fix, this comment is stale and misleading. **Suggested fix:** Update to reflect current state: ```gherkin # Regression tests for bug #590: project show must find a project immediately after creation. ```
features/steps/project_show_after_create_steps.py Outdated
@@ -0,0 +94,4 @@
patch(_PATCH_PROJECT_REPO, return_value=repo),
patch(_PATCH_STORE_EXTRAS),
):
runner.invoke(project_app, ["create", name])
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F1 [Medium -- Test Flaw]: The runner.invoke() result is discarded without checking the exit code. If project create itself fails (import error, DB schema mismatch, Typer wiring issue), the test silently proceeds to the show step and fails there, misattributing the failure.

Since these are TDD tests designed to isolate the show-after-create bug specifically, a silent create failure would produce a confusing false-positive ("test fails as expected") for the wrong reason.

Suggested fix:

result = runner.invoke(project_app, ["create", name])
assert result.exit_code == 0, (
    f"project create failed unexpectedly: {result.output}"
)

Same issue applies to step_create_project_show_desc at line 111.

**F1 [Medium -- Test Flaw]:** The `runner.invoke()` result is discarded without checking the exit code. If `project create` itself fails (import error, DB schema mismatch, Typer wiring issue), the test silently proceeds to the `show` step and fails there, misattributing the failure. Since these are TDD tests designed to isolate the show-after-create bug specifically, a silent create failure would produce a confusing false-positive ("test fails as expected") for the wrong reason. **Suggested fix:** ```python result = runner.invoke(project_app, ["create", name]) assert result.exit_code == 0, ( f"project create failed unexpectedly: {result.output}" ) ``` Same issue applies to `step_create_project_show_desc` at line 111.
robot/project_show_after_create.robot Outdated
@@ -0,0 +16,4 @@
[Documentation] After creating a project, show should display its details.
... Expected to FAIL while bug #590 is present.
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_')
Run Process ${PYTHON} -m cleveragents init show-test
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F2 [Medium -- Test Flaw]: The Run Process calls for agents init (line 19) and agents project create (line 21) do not capture or verify return codes. If either command fails, the test only catches it at the project show assertion on line 25, obscuring the real failure point.

Suggested fix:

${init_result}=    Run Process    ${PYTHON}    -m    cleveragents    init    show-test
...    timeout=60s    cwd=${tmpdir}
Should Be Equal As Integers    ${init_result.rc}    0
...    msg=agents init failed: ${init_result.stderr}
${create_result}=    Run Process    ${PYTHON}    -m    cleveragents    project    create    local/show-proj
...    timeout=60s    cwd=${tmpdir}
Should Be Equal As Integers    ${create_result.rc}    0
...    msg=project create failed: ${create_result.stderr}

Same pattern applies to the second test case starting at line 34.

**F2 [Medium -- Test Flaw]:** The `Run Process` calls for `agents init` (line 19) and `agents project create` (line 21) do not capture or verify return codes. If either command fails, the test only catches it at the `project show` assertion on line 25, obscuring the real failure point. **Suggested fix:** ```robot ${init_result}= Run Process ${PYTHON} -m cleveragents init show-test ... timeout=60s cwd=${tmpdir} Should Be Equal As Integers ${init_result.rc} 0 ... msg=agents init failed: ${init_result.stderr} ${create_result}= Run Process ${PYTHON} -m cleveragents project create local/show-proj ... timeout=60s cwd=${tmpdir} Should Be Equal As Integers ${create_result.rc} 0 ... msg=project create failed: ${create_result.stderr} ``` Same pattern applies to the second test case starting at line 34.
robot/project_show_after_create.robot Outdated
@@ -0,0 +20,4 @@
... timeout=60s cwd=${tmpdir}
Run Process ${PYTHON} -m cleveragents project create local/show-proj
... timeout=60s cwd=${tmpdir}
${result}= Run Process ${PYTHON} -m cleveragents project show local/show-proj
CoreRasurae commented 2026-03-05 16:03:58 +00:00
Member
Copy Link
Copy Source

F6 [Low -- Robustness]: The project show invocation uses the default output format (rich), which may include ANSI escape codes in real subprocess environments. The Should Contain assertion on line 27 could fail or produce false positives if ANSI codes appear inside the project name string.

Suggested fix: Pass --format plain to guarantee clean output:

${result}=    Run Process    ${PYTHON}    -m    cleveragents    --format    plain    project    show    local/show-proj

Same applies to line 40 in the second test case.

**F6 [Low -- Robustness]:** The `project show` invocation uses the default output format (rich), which may include ANSI escape codes in real subprocess environments. The `Should Contain` assertion on line 27 could fail or produce false positives if ANSI codes appear inside the project name string. **Suggested fix:** Pass `--format plain` to guarantee clean output: ```robot ${result}= Run Process ${PYTHON} -m cleveragents --format plain project show local/show-proj ``` Same applies to line 40 in the second test case.
brent.edwards commented 2026-03-05 20:11:51 +00:00
Author
Member
Copy Link
Copy Source

All actionable findings from review #1977 have been addressed in commit e07f795c:

Finding Severity Resolution
F1 Medium Steps: both step_create_project_show and step_create_project_show_desc now capture result and assert exit_code == 0
F2 Medium Robot: all init and create results captured with ${init}=/${create}=, exit codes asserted == 0 in all test cases
F3 Medium Benchmark: track_show_after_create_found now uses self._counter for unique names (bench-show-track-{N})
F4 Medium Acknowledged — this is an existing codebase issue with @database_retry retrying ProjectNotFoundError. Will file a follow-up ticket.
F5 Low Benchmark: replaced bare except Exception with except ProjectNotFoundError (and contextlib.suppress where ruff requires it)
F6 Low Robot: all project show invocations now pass --format plain to avoid ANSI escape codes
F7 Low Robot: added "Project Show Returns Error For Nonexistent Project" test case
F8 Low Feature: updated stale comment to "Regression tests for bug #590: project show must find a project immediately after creation."
F9 Low Acknowledged — spec-required output structure validation (Project Details, Linked Resources, etc.) is a valid coverage gap but out of scope for this TDD PR. Can be addressed in a follow-up.

Additionally applied proactively:

  • Rewrote benchmark imports to _SRC/importlib.reload(cleveragents) convention (matching the 166 other benchmark files)
  • Added NullPool for all create_engine() calls in both steps and benchmark to properly release SQLite file handles

Pyright typecheck and ruff lint both pass.

All actionable findings from review #1977 have been addressed in commit `e07f795c`: | Finding | Severity | Resolution | |---------|----------|------------| | **F1** | Medium | Steps: both `step_create_project_show` and `step_create_project_show_desc` now capture result and assert `exit_code == 0` | | **F2** | Medium | Robot: all `init` and `create` results captured with `${init}=`/`${create}=`, exit codes asserted == 0 in all test cases | | **F3** | Medium | Benchmark: `track_show_after_create_found` now uses `self._counter` for unique names (`bench-show-track-{N}`) | | **F4** | Medium | Acknowledged — this is an existing codebase issue with `@database_retry` retrying `ProjectNotFoundError`. Will file a follow-up ticket. | | **F5** | Low | Benchmark: replaced bare `except Exception` with `except ProjectNotFoundError` (and `contextlib.suppress` where ruff requires it) | | **F6** | Low | Robot: all `project show` invocations now pass `--format plain` to avoid ANSI escape codes | | **F7** | Low | Robot: added "Project Show Returns Error For Nonexistent Project" test case | | **F8** | Low | Feature: updated stale comment to "Regression tests for bug #590: project show must find a project immediately after creation." | | **F9** | Low | Acknowledged — spec-required output structure validation (Project Details, Linked Resources, etc.) is a valid coverage gap but out of scope for this TDD PR. Can be addressed in a follow-up. | Additionally applied proactively: - Rewrote benchmark imports to `_SRC`/`importlib.reload(cleveragents)` convention (matching the 166 other benchmark files) - Added `NullPool` for all `create_engine()` calls in both steps and benchmark to properly release SQLite file handles Pyright typecheck and ruff lint both pass.
hamza.khyari requested changes 2026-03-05 22:13:38 +00:00
hamza.khyari left a comment
Member
Copy Link
Copy Source

Code Review 2 -- PR #593

Reviewed: Commits 070cc1ee through e07f795c (HEAD)
Scope: Full trace through DI container, session lifecycle, retry decorator, CLI call chains, error paths, test correctness
Prior review: #1977 by CoreRasurae -- findings F1-F3, F5-F8 addressed in e07f795c; F4 deferred; F9 not addressed (acceptable)

Overview

The one-line fix (session.commit() at repositories.py:2845) is correct for the immediate bug. Tests are solid -- Behave isolation is clean, Robot tests provide strong end-to-end coverage, and the review #1977 remediation commit is thorough. However, tracing the full session lifecycle through the DI container, retry decorator, and CLI call chains reveals the fix is a point-patch on a systemic issue, and it exposes adjacent broken paths.

Findings

BUG-1: project delete Is Silently Broken (Same Root Cause)

Severity: Critical
Category: BUG
File: project.py:916 + repositories.py:2988

The CLI delete command (project.py:916) calls repo.delete(name) via the same DI pattern (_get_namespaced_project_repo()). delete() at repositories.py:2988 only calls session.flush() -- never commits. The sessionmaker session (from container.py:160, no autocommit) rolls back on GC.

The command returns success -- delete() returns True at line 2989 (set before the transaction is committed), the CLI prints a success panel at line 925 -- but the row still exists.

agents project delete local/myproject --yes  # appears to succeed
agents project show local/myproject           # project still exists

Recommendation: Apply session.commit() after session.flush() in delete(), or file a blocking follow-up ticket. Same applies to update() at line 2955 if it has a CLI caller.

BUG-2: _store_project_extras Silently Loses Invariants

Severity: Major
Category: BUG
File: project.py:95-134

_store_project_extras() (called at line 551 when --invariant or --invariant-actor is provided) creates its own engine/session (project.py:114-115) and runs UPDATE ns_projects SET ... WHERE namespaced_name = :ns_name. This is a separate connection to the same SQLite file.

Before this PR's fix, create() only flushed. This separate connection could not see the uncommitted row. The UPDATE matched zero rows and returned silently. Invariants were silently lost. The fix resolves this specific path, but the pattern is fragile -- there is no rowcount check to verify the UPDATE actually modified a row.

Recommendation: Add if result.rowcount == 0: raise ... after the execute at line 131, or at minimum log a warning.

BUG-3: @database_retry Retries ProjectNotFoundError (~1s Overhead)

Severity: Major
Category: BUG
File: repositories.py:2782 + core/retry_patterns.py:122-126

ProjectNotFoundError inherits from DatabaseError (repositories.py:2782). The @database_retry decorator retries on DatabaseError (3 attempts, 0.5s fixed wait). get()'s except ProjectNotFoundError: raise at line 2881 re-raises, but tenacity catches it at the decorator level because isinstance(ProjectNotFoundError(...), DatabaseError) is True.

Every "not found" lookup wastes ~1.0s in retry sleeps. The same issue applies to duplicate detection in create(): the IntegrityError handler at line 2850 raises DatabaseError("...already exists"), which the retry decorator catches and retries twice more.

Both are deterministic, non-transient conditions that should never be retried.

Recommendation: At minimum, file a follow-up ticket. Proper fix: either exclude ProjectNotFoundError from the retry exception list, or change its inheritance to not extend DatabaseError.

BUG-4: Session Leak -- No session.close() in Repository Methods

Severity: Major
Category: BUG
File: repositories.py (entire file)

Zero calls to session.close() across the entire 5,435-line repositories.py. The only close() in the database layer is in unit_of_work.py:131.

Every self._session() call creates a new session from sessionmaker(). After the method returns, nobody closes it. The session holds an open connection and transaction state. With @database_retry, a single failed get() creates 3 leaked sessions (one per attempt, since self._session() is called inside the retry loop at line 2871).

SQLAlchemy GC will eventually clean up via Session.__del__, but relying on GC for connection cleanup is fragile and can exhaust SQLite file handles under load.

Recommendation: File a follow-up ticket. Minimal fix: wrap each method body in try/finally: session.close().

BUG-5: Error Path After flush() + commit() Sequence

Severity: Medium
Category: BUG
File: repositories.py:2844-2856

If flush() succeeds but commit() raises OperationalError (disk full, lock timeout), the handler at line 2855 does session.rollback() and raises DatabaseError. The retry decorator retries with a new session. If commit() partially succeeded (WAL-written but fsync failed), the retry INSERT could hit IntegrityError on a phantom duplicate. This is a narrow edge case but the error handling doesn't account for it.

TEST-1: Negative Test Depends on CliRunner(mix_stderr=True) Implicitly

Severity: Medium
Category: TEST
File: features/steps/project_show_after_create_steps.py:44,149-157

Scenario 3 asserts the project-show output should contain "not found". The show command prints this to err_console (stderr) at project.py:837. The step checks result.output (stdout) at line 154. This works only because CliRunner() at line 44 defaults to mix_stderr=True (verified). If the runner is ever constructed with mix_stderr=False, this test silently breaks.

Recommendation: Change line 44 to runner = CliRunner(mix_stderr=True) to make the dependency explicit.

CODE-1: Class Docstring Contradicts Implementation

Severity: Minor
Category: CODE
File: repositories.py:2815-2816

Docstring reads: "All mutating methods flush (but do NOT commit); the caller or a UnitOfWork wrapper is responsible for committing the transaction." After this PR, create() commits. update() and delete() still only flush. The docstring is now inaccurate.

Recommendation: Update the docstring to reflect the actual behavior.

CODE-2: DI Engine Config Mismatch vs UoW

Severity: Minor
Category: CODE
File: container.py:159 vs unit_of_work.py:82-107

The DI-built engine uses bare create_engine(url, echo=False). The UoW engine uses future=True, isolation_level="SERIALIZABLE", connect_args={"check_same_thread": False}, autoflush=False. Same database, different configs depending on code path. This can cause subtle isolation-level differences in concurrent access patterns.

PROCESS-1: CHANGELOG Tag Mismatch

Severity: Minor
Category: PROCESS
File: CHANGELOG.md:39

Says (with @wip tag) but the feature file uses @tdd @bug590.

PROCESS-2: Issue #590 Subtasks Unchecked

Severity: Minor
Category: PROCESS

All 5 subtask checkboxes in issue #590 are still - [ ].

CODE-3: type: ignore[attr-defined] Without Approval

Severity: Nit
Category: CODE
File: benchmarks/project_show_after_create_bench.py:116

Per DANGER_ZONE.md, type: ignore requires orchestrator approval.

Summary Table

ID Severity Category
BUG-1 Critical delete() silently broken
BUG-2 Major _store_project_extras silent data loss
BUG-3 Major Retry on non-transient errors
BUG-4 Major Session leaks
BUG-5 Medium Partial-commit retry edge case
TEST-1 Medium Implicit mix_stderr dependency
CODE-1 Minor Stale docstring
CODE-2 Minor Engine config mismatch
PROCESS-1 Minor CHANGELOG tag
PROCESS-2 Minor Issue subtasks
CODE-3 Nit type: ignore

Verdict: REQUEST_CHANGES

Blocking: BUG-1 (delete() has the same unfixed bug -- it is in the same class being modified by this PR and will silently eat user data).

# Code Review 2 -- PR #593 **Reviewed**: Commits `070cc1ee` through `e07f795c` (HEAD) **Scope**: Full trace through DI container, session lifecycle, retry decorator, CLI call chains, error paths, test correctness **Prior review**: #1977 by CoreRasurae -- findings F1-F3, F5-F8 addressed in `e07f795c`; F4 deferred; F9 not addressed (acceptable) ## Overview The one-line fix (`session.commit()` at `repositories.py:2845`) is correct for the immediate bug. Tests are solid -- Behave isolation is clean, Robot tests provide strong end-to-end coverage, and the review #1977 remediation commit is thorough. However, tracing the full session lifecycle through the DI container, retry decorator, and CLI call chains reveals the fix is a point-patch on a systemic issue, and it exposes adjacent broken paths. --- ## Findings ### BUG-1: `project delete` Is Silently Broken (Same Root Cause) **Severity**: Critical **Category**: BUG **File**: `project.py:916` + `repositories.py:2988` The CLI `delete` command (`project.py:916`) calls `repo.delete(name)` via the same DI pattern (`_get_namespaced_project_repo()`). `delete()` at `repositories.py:2988` only calls `session.flush()` -- never commits. The `sessionmaker` session (from `container.py:160`, no autocommit) rolls back on GC. The command returns success -- `delete()` returns `True` at line 2989 (set before the transaction is committed), the CLI prints a success panel at line 925 -- but the row still exists. ```bash agents project delete local/myproject --yes # appears to succeed agents project show local/myproject # project still exists ``` **Recommendation**: Apply `session.commit()` after `session.flush()` in `delete()`, or file a blocking follow-up ticket. Same applies to `update()` at line 2955 if it has a CLI caller. --- ### BUG-2: `_store_project_extras` Silently Loses Invariants **Severity**: Major **Category**: BUG **File**: `project.py:95-134` `_store_project_extras()` (called at line 551 when `--invariant` or `--invariant-actor` is provided) creates its own engine/session (`project.py:114-115`) and runs `UPDATE ns_projects SET ... WHERE namespaced_name = :ns_name`. This is a separate connection to the same SQLite file. Before this PR's fix, `create()` only flushed. This separate connection could not see the uncommitted row. The UPDATE matched zero rows and returned silently. **Invariants were silently lost.** The fix resolves this specific path, but the pattern is fragile -- there is no rowcount check to verify the UPDATE actually modified a row. **Recommendation**: Add `if result.rowcount == 0: raise ...` after the execute at line 131, or at minimum log a warning. --- ### BUG-3: `@database_retry` Retries `ProjectNotFoundError` (~1s Overhead) **Severity**: Major **Category**: BUG **File**: `repositories.py:2782` + `core/retry_patterns.py:122-126` `ProjectNotFoundError` inherits from `DatabaseError` (`repositories.py:2782`). The `@database_retry` decorator retries on `DatabaseError` (3 attempts, 0.5s fixed wait). `get()`'s `except ProjectNotFoundError: raise` at line 2881 re-raises, but tenacity catches it at the decorator level because `isinstance(ProjectNotFoundError(...), DatabaseError)` is `True`. Every "not found" lookup wastes ~1.0s in retry sleeps. The same issue applies to duplicate detection in `create()`: the `IntegrityError` handler at line 2850 raises `DatabaseError("...already exists")`, which the retry decorator catches and retries twice more. Both are deterministic, non-transient conditions that should never be retried. **Recommendation**: At minimum, file a follow-up ticket. Proper fix: either exclude `ProjectNotFoundError` from the retry exception list, or change its inheritance to not extend `DatabaseError`. --- ### BUG-4: Session Leak -- No `session.close()` in Repository Methods **Severity**: Major **Category**: BUG **File**: `repositories.py` (entire file) Zero calls to `session.close()` across the entire 5,435-line `repositories.py`. The only `close()` in the database layer is in `unit_of_work.py:131`. Every `self._session()` call creates a new session from `sessionmaker()`. After the method returns, nobody closes it. The session holds an open connection and transaction state. With `@database_retry`, a single failed `get()` creates 3 leaked sessions (one per attempt, since `self._session()` is called inside the retry loop at line 2871). SQLAlchemy GC will eventually clean up via `Session.__del__`, but relying on GC for connection cleanup is fragile and can exhaust SQLite file handles under load. **Recommendation**: File a follow-up ticket. Minimal fix: wrap each method body in `try/finally: session.close()`. --- ### BUG-5: Error Path After `flush()` + `commit()` Sequence **Severity**: Medium **Category**: BUG **File**: `repositories.py:2844-2856` If `flush()` succeeds but `commit()` raises `OperationalError` (disk full, lock timeout), the handler at line 2855 does `session.rollback()` and raises `DatabaseError`. The retry decorator retries with a new session. If `commit()` partially succeeded (WAL-written but fsync failed), the retry INSERT could hit IntegrityError on a phantom duplicate. This is a narrow edge case but the error handling doesn't account for it. --- ### TEST-1: Negative Test Depends on `CliRunner(mix_stderr=True)` Implicitly **Severity**: Medium **Category**: TEST **File**: `features/steps/project_show_after_create_steps.py:44,149-157` Scenario 3 asserts `the project-show output should contain "not found"`. The `show` command prints this to `err_console` (stderr) at `project.py:837`. The step checks `result.output` (stdout) at line 154. This works only because `CliRunner()` at line 44 defaults to `mix_stderr=True` (verified). If the runner is ever constructed with `mix_stderr=False`, this test silently breaks. **Recommendation**: Change line 44 to `runner = CliRunner(mix_stderr=True)` to make the dependency explicit. --- ### CODE-1: Class Docstring Contradicts Implementation **Severity**: Minor **Category**: CODE **File**: `repositories.py:2815-2816` Docstring reads: *"All mutating methods flush (but do NOT commit); the caller or a UnitOfWork wrapper is responsible for committing the transaction."* After this PR, `create()` commits. `update()` and `delete()` still only flush. The docstring is now inaccurate. **Recommendation**: Update the docstring to reflect the actual behavior. --- ### CODE-2: DI Engine Config Mismatch vs UoW **Severity**: Minor **Category**: CODE **File**: `container.py:159` vs `unit_of_work.py:82-107` The DI-built engine uses bare `create_engine(url, echo=False)`. The UoW engine uses `future=True`, `isolation_level="SERIALIZABLE"`, `connect_args={"check_same_thread": False}`, `autoflush=False`. Same database, different configs depending on code path. This can cause subtle isolation-level differences in concurrent access patterns. --- ### PROCESS-1: CHANGELOG Tag Mismatch **Severity**: Minor **Category**: PROCESS **File**: `CHANGELOG.md:39` Says `(with @wip tag)` but the feature file uses `@tdd @bug590`. --- ### PROCESS-2: Issue #590 Subtasks Unchecked **Severity**: Minor **Category**: PROCESS All 5 subtask checkboxes in issue #590 are still `- [ ]`. --- ### CODE-3: `type: ignore[attr-defined]` Without Approval **Severity**: Nit **Category**: CODE **File**: `benchmarks/project_show_after_create_bench.py:116` Per DANGER_ZONE.md, `type: ignore` requires orchestrator approval. --- ## Summary Table | ID | Severity | Category | |----|----------|----------| | BUG-1 | Critical | `delete()` silently broken | | BUG-2 | Major | `_store_project_extras` silent data loss | | BUG-3 | Major | Retry on non-transient errors | | BUG-4 | Major | Session leaks | | BUG-5 | Medium | Partial-commit retry edge case | | TEST-1 | Medium | Implicit `mix_stderr` dependency | | CODE-1 | Minor | Stale docstring | | CODE-2 | Minor | Engine config mismatch | | PROCESS-1 | Minor | CHANGELOG tag | | PROCESS-2 | Minor | Issue subtasks | | CODE-3 | Nit | `type: ignore` | ## Verdict: REQUEST_CHANGES **Blocking**: BUG-1 (`delete()` has the same unfixed bug -- it is in the same class being modified by this PR and will silently eat user data).
brent.edwards commented 2026-03-06 01:36:36 +00:00
Author
Member
Copy Link
Copy Source

Disposition — Review #1995 (hamza.khyari)

All actionable findings have been addressed across 4 commits. Quality gates pass locally: nox -s lint ✅, nox -s typecheck ✅, nox -s unit_tests (8703 scenarios, 0 failures) ✅.

BUG-1: project delete Is Silently Broken (Critical, BLOCKING) — FIXED ✅

Added session.commit() after session.flush() in both delete() (line ~2990) and update() (line ~2956) of NamespacedProjectRepository. Although update() has no CLI callers currently, it has the same flush-without-commit pattern and was fixed proactively.

Commit: c9fe3c54 — fix(project): add session.commit() to delete/update and update docstring

TEST-1: Implicit mix_stderr Dependency (Medium) — ADDRESSED ✅

Investigated: typer.testing.CliRunner does not support the mix_stderr parameter. Its __init__ signature is (self, charset, env, echo_stdin, catch_exceptions) — it always mixes stderr into stdout. This differs from click.testing.CliRunner which does accept mix_stderr.

Instead of CliRunner(mix_stderr=True) (which would raise TypeError), added a 4-line comment above runner = CliRunner() documenting the implicit dependency and explaining why the explicit parameter cannot be used.

Commit: 8f38ab2c

CODE-1: Class Docstring Contradicts Implementation (Minor) — FIXED ✅

Updated docstring at lines 2815-2816 from "All mutating methods flush (but do NOT commit)..." to "All mutating methods (create, update, delete) flush and commit within their own session. No external UnitOfWork is needed."

Commit: c9fe3c54 (same commit as BUG-1)

PROCESS-1: CHANGELOG Tag Mismatch (Minor) — FIXED ✅

Changed CHANGELOG line 39 from (with @wip tag) to (tagged @tdd @bug590) to match the actual feature file tags.

Commit: 3cdd97e3

CODE-3: type: ignore[attr-defined] Without Approval (Nit) — FIXED ✅

Removed # type: ignore[attr-defined] from benchmarks/project_show_after_create_bench.py line 116. Confirmed all other ASV benchmark files in the project use the .unit = "..." pattern without this suppression, and pyright passes without it.

Commit: ed62a014

Pre-existing / Systemic Issues — Acknowledged (Not blocking this PR)

ID Finding Disposition
BUG-2 _store_project_extras silently loses invariants (no rowcount check) Pre-existing in project.py. Out of scope for this TDD test PR. Will file follow-up ticket.
BUG-3 @database_retry retries ProjectNotFoundError (~1s overhead) Pre-existing systemic issue (also flagged by CoreRasurae as F4). Already tracked in issue #590 comments. Will file follow-up ticket.
BUG-4 Session leak — no session.close() in repository methods Pre-existing across entire repositories.py (5,437 lines). Requires architectural fix (context-manager pattern). Will file follow-up ticket.
BUG-5 Partial-commit retry edge case Narrow edge case. Acknowledged. Will include in BUG-4 follow-up scope.
CODE-2 DI engine config mismatch vs UoW Pre-existing in container.py vs unit_of_work.py. Out of scope. Will file follow-up ticket.
PROCESS-2 Issue #590 subtasks unchecked Will update issue #590 checkboxes after this PR merges.

Ready for re-review. All 4 commits pushed to feature/m3-fix-project-show-after-create.

## Disposition — Review #1995 (hamza.khyari) All actionable findings have been addressed across 4 commits. Quality gates pass locally: `nox -s lint` ✅, `nox -s typecheck` ✅, `nox -s unit_tests` (8703 scenarios, 0 failures) ✅. --- ### BUG-1: `project delete` Is Silently Broken (Critical, BLOCKING) — **FIXED** ✅ Added `session.commit()` after `session.flush()` in both `delete()` (line ~2990) and `update()` (line ~2956) of `NamespacedProjectRepository`. Although `update()` has no CLI callers currently, it has the same flush-without-commit pattern and was fixed proactively. **Commit**: `c9fe3c54` — `fix(project): add session.commit() to delete/update and update docstring` --- ### TEST-1: Implicit `mix_stderr` Dependency (Medium) — **ADDRESSED** ✅ Investigated: `typer.testing.CliRunner` does **not** support the `mix_stderr` parameter. Its `__init__` signature is `(self, charset, env, echo_stdin, catch_exceptions)` — it always mixes stderr into stdout. This differs from `click.testing.CliRunner` which does accept `mix_stderr`. Instead of `CliRunner(mix_stderr=True)` (which would raise `TypeError`), added a 4-line comment above `runner = CliRunner()` documenting the implicit dependency and explaining why the explicit parameter cannot be used. **Commit**: `8f38ab2c` --- ### CODE-1: Class Docstring Contradicts Implementation (Minor) — **FIXED** ✅ Updated docstring at lines 2815-2816 from *"All mutating methods flush (but do NOT commit)..."* to *"All mutating methods (``create``, ``update``, ``delete``) flush **and commit** within their own session. No external ``UnitOfWork`` is needed."* **Commit**: `c9fe3c54` (same commit as BUG-1) --- ### PROCESS-1: CHANGELOG Tag Mismatch (Minor) — **FIXED** ✅ Changed CHANGELOG line 39 from `(with @wip tag)` to `(tagged @tdd @bug590)` to match the actual feature file tags. **Commit**: `3cdd97e3` --- ### CODE-3: `type: ignore[attr-defined]` Without Approval (Nit) — **FIXED** ✅ Removed `# type: ignore[attr-defined]` from `benchmarks/project_show_after_create_bench.py` line 116. Confirmed all other ASV benchmark files in the project use the `.unit = "..."` pattern without this suppression, and pyright passes without it. **Commit**: `ed62a014` --- ### Pre-existing / Systemic Issues — Acknowledged (Not blocking this PR) | ID | Finding | Disposition | |----|---------|-------------| | **BUG-2** | `_store_project_extras` silently loses invariants (no rowcount check) | Pre-existing in `project.py`. Out of scope for this TDD test PR. Will file follow-up ticket. | | **BUG-3** | `@database_retry` retries `ProjectNotFoundError` (~1s overhead) | Pre-existing systemic issue (also flagged by CoreRasurae as F4). Already tracked in issue #590 comments. Will file follow-up ticket. | | **BUG-4** | Session leak — no `session.close()` in repository methods | Pre-existing across entire `repositories.py` (5,437 lines). Requires architectural fix (context-manager pattern). Will file follow-up ticket. | | **BUG-5** | Partial-commit retry edge case | Narrow edge case. Acknowledged. Will include in BUG-4 follow-up scope. | | **CODE-2** | DI engine config mismatch vs UoW | Pre-existing in `container.py` vs `unit_of_work.py`. Out of scope. Will file follow-up ticket. | | **PROCESS-2** | Issue #590 subtasks unchecked | Will update issue #590 checkboxes after this PR merges. | --- Ready for re-review. All 4 commits pushed to `feature/m3-fix-project-show-after-create`.
hurui200320 requested changes 2026-03-06 07:40:11 +00:00
Dismissed
hurui200320 left a comment
Member
Copy Link
Copy Source

Code Review -- PR #593 (Commits 070cc1ee through c9fe3c54)

Reviewed against: Issue #590, branch feature/m3-fix-project-show-after-create, docs/specification.md, CONTRIBUTING.md
Prior reviews: #1977 (CoreRasurae, REQUEST_CHANGES), #1995 (hamza.khyari, REQUEST_CHANGES)

Overview

The core code fix is correct -- adding session.commit() to create(), update(), and delete() in NamespacedProjectRepository resolves the reported bug and proactively fixes the same issue in the other mutating methods. Tests are well-structured with proper isolation. However, there are blocking process violations that must be fixed before merge.

Findings

PROCESS-1: Merge Commit in Branch History (Critical, BLOCKING)

Location: Commit c6a768b5

The branch contains Merge branch 'master' into feature/m3-fix-project-show-after-create. Per CONTRIBUTING.md: "Each branch must not contain merge commits -- instead as master drifts the branches should always align with master via rebase, strictly avoiding merge commits in a branch's history."

Recommendation: Rebase the branch onto origin/master using git rebase origin/master (not merge), then force-push.

PROCESS-2: Multiple Commits for Single Issue (Critical, BLOCKING)

Location: Branch history (8 commits)

The branch contains 8 commits for issue #590:

c9fe3c54 fix(project): add session.commit() to delete/update and update docstring
3cdd97e3 fix(changelog): correct tag reference from @wip to @tdd @bug590
8f38ab2c fix(project): add mix_stderr documentation comment to test runner
ed62a014 fix(project): address review #1995 findings
e07f795c test(project): address review #1977 findings
c6a768b5 Merge branch 'master' into ...
d262b0d0 fix(project): show created project after creation
070cc1ee fix(project): show created project after creation

Per CONTRIBUTING.md: "Issues are the atomic unit of work. Each issue corresponds to exactly one commit." And: "Clean up history before merging. Use interactive rebase or amend to fix typos, consolidate fixup commits, and polish the commit series."

Commits 3-8 are fix-up commits addressing review feedback that should have been squashed into the original commits.

Recommendation: Squash all commits into a single commit using git rebase -i with the prescribed first line fix(project): show created project after creation, followed by a body describing the full scope of changes and ISSUES CLOSED: #590 in the footer.

PROCESS-3: Commit Message Mismatch (Medium)

Location: HEAD commit c9fe3c54

The issue metadata prescribes fix(project): show created project after creation as the commit message. The HEAD commit uses fix(project): add session.commit() to delete/update and update docstring. None of the non-squashed commits would serve as the final commit message.

Recommendation: When squashing (per PROCESS-2), use the prescribed message exactly as the first line.

PROCESS-4: Missing ISSUES CLOSED Footer (Medium)

Location: All commits

All commits use Refs: #590 instead of ISSUES CLOSED: #590. Per CONTRIBUTING.md and the issue template: "The body should also include the issue reference footer (e.g., ISSUES CLOSED: #45)."

Recommendation: The squashed commit message should end with ISSUES CLOSED: #590.

PROCESS-5: Type Label Mismatch (Medium)

Location: PR labels

The PR has Type/Testing but issue #590 is Type/Bug. Per CONTRIBUTING.md: "Every PR must carry exactly one Type/ label that matches the nature of the change." This PR fixes a bug (adds session.commit()) and includes tests -- the primary nature is a bug fix.

Recommendation: Replace Type/Testing with Type/Bug.

PROCESS-6: PR Not Mergeable (Blocking)

Location: PR status

The PR shows mergeable: false, indicating conflicts with the base branch. This must be resolved before merge.

Recommendation: Rebase onto current origin/master and force-push.

PROCESS-7: Issue #590 Subtasks Still Unchecked (Minor)

Location: Issue #590 body

All 5 subtask checkboxes in issue #590 are still - [ ]. Per workflow rules, subtasks should be checked off as completed.

Recommendation: Update issue #590 body to check off completed subtasks.

CODE-1: No session.close() in Repository Methods (Major, Pre-existing)

Location: repositories.py -- create() (~line 2840), get() (~line 2871), update() (~line 2930), delete() (~line 2979)

None of the methods call session.close(). Sessions are created via self._session() and never explicitly closed -- no finally block, no context manager. Under @database_retry (3 attempts), a single failed get() creates 3 leaked sessions. While SQLAlchemy GC eventually cleans up, relying on it is fragile and can exhaust SQLite file handles under load.

This is pre-existing across the entire file and not introduced by this PR, so it should not block merge -- but a follow-up ticket should be filed.

Recommendation: File a follow-up ticket. Minimal fix: wrap each method body in try/finally: session.close().

CODE-2: @database_retry Retries ProjectNotFoundError (Major, Pre-existing)

Location: retry_patterns.py:122-126, repositories.py:2782

ProjectNotFoundError(DatabaseError) means the @database_retry decorator (which retries on DatabaseError, 3 attempts, 0.5s wait) catches re-raised ProjectNotFoundError at the tenacity level. Every legitimate "not found" lookup wastes ~1.0s in retry sleeps. Same applies to IntegrityError -> DatabaseError("already exists") in create().

Pre-existing, already flagged by CoreRasurae (F4) and hamza.khyari (BUG-3). Not blocking this PR.

Recommendation: File a follow-up ticket to exclude ProjectNotFoundError from the retry exception config or change its inheritance.

CODE-3: Redundant flush() Before commit() (Nit)

Location: repositories.py lines 2844-2845, 2955-2956, 2989-2990

session.commit() implicitly flushes, so the preceding session.flush() is technically redundant. Not harmful, but unnecessary.

Recommendation: Optional cleanup -- remove the explicit flush() calls since commit() handles it.

What's Good

  • The core fix is correct and well-analyzed (same root cause as #589)
  • update() and delete() were proactively fixed (addressing hamza.khyari's critical BUG-1)
  • Class docstring updated to match new behavior
  • Tests are comprehensive: 3 Behave BDD scenarios, 3 Robot integration tests, ASV benchmarks
  • Previous review feedback (CoreRasurae #1977, hamza.khyari #1995) was thoroughly addressed
  • Root cause analysis is well-documented in issue comments
  • CHANGELOG updated

Summary Table

ID Severity Category Blocking?
PROCESS-1 Critical Merge commit in branch YES
PROCESS-2 Critical Multiple commits for single issue YES
PROCESS-3 Medium Commit message mismatch No (fixed by squash)
PROCESS-4 Medium Missing ISSUES CLOSED footer No (fixed by squash)
PROCESS-5 Medium Type label mismatch No
PROCESS-6 Blocking PR not mergeable (conflicts) YES
PROCESS-7 Minor Issue subtasks unchecked No
CODE-1 Major Session leak (pre-existing) No
CODE-2 Major Retry on non-transient errors (pre-existing) No
CODE-3 Nit Redundant flush before commit No

Verdict: REQUEST_CHANGES

Blocking items (must fix before merge):

  1. PROCESS-1: Remove merge commit via rebase
  2. PROCESS-2: Squash all 8 commits into 1 with prescribed commit message
  3. PROCESS-6: Resolve merge conflicts with master

These are all fixable with a single git rebase -i origin/master + force-push. The code and tests themselves are solid.

# Code Review -- PR #593 (Commits `070cc1ee` through `c9fe3c54`) **Reviewed against:** Issue #590, branch `feature/m3-fix-project-show-after-create`, `docs/specification.md`, `CONTRIBUTING.md` **Prior reviews:** #1977 (CoreRasurae, REQUEST_CHANGES), #1995 (hamza.khyari, REQUEST_CHANGES) ## Overview The core code fix is correct -- adding `session.commit()` to `create()`, `update()`, and `delete()` in `NamespacedProjectRepository` resolves the reported bug and proactively fixes the same issue in the other mutating methods. Tests are well-structured with proper isolation. However, there are **blocking process violations** that must be fixed before merge. --- ## Findings ### PROCESS-1: Merge Commit in Branch History (Critical, BLOCKING) **Location:** Commit `c6a768b5` The branch contains `Merge branch 'master' into feature/m3-fix-project-show-after-create`. Per CONTRIBUTING.md: *"Each branch must not contain merge commits -- instead as master drifts the branches should always align with master via rebase, strictly avoiding merge commits in a branch's history."* **Recommendation:** Rebase the branch onto `origin/master` using `git rebase origin/master` (not merge), then force-push. --- ### PROCESS-2: Multiple Commits for Single Issue (Critical, BLOCKING) **Location:** Branch history (8 commits) The branch contains 8 commits for issue #590: ``` c9fe3c54 fix(project): add session.commit() to delete/update and update docstring 3cdd97e3 fix(changelog): correct tag reference from @wip to @tdd @bug590 8f38ab2c fix(project): add mix_stderr documentation comment to test runner ed62a014 fix(project): address review #1995 findings e07f795c test(project): address review #1977 findings c6a768b5 Merge branch 'master' into ... d262b0d0 fix(project): show created project after creation 070cc1ee fix(project): show created project after creation ``` Per CONTRIBUTING.md: *"Issues are the atomic unit of work. Each issue corresponds to exactly one commit."* And: *"Clean up history before merging. Use interactive rebase or amend to fix typos, consolidate fixup commits, and polish the commit series."* Commits 3-8 are fix-up commits addressing review feedback that should have been squashed into the original commits. **Recommendation:** Squash all commits into a single commit using `git rebase -i` with the prescribed first line `fix(project): show created project after creation`, followed by a body describing the full scope of changes and `ISSUES CLOSED: #590` in the footer. --- ### PROCESS-3: Commit Message Mismatch (Medium) **Location:** HEAD commit `c9fe3c54` The issue metadata prescribes `fix(project): show created project after creation` as the commit message. The HEAD commit uses `fix(project): add session.commit() to delete/update and update docstring`. None of the non-squashed commits would serve as the final commit message. **Recommendation:** When squashing (per PROCESS-2), use the prescribed message exactly as the first line. --- ### PROCESS-4: Missing `ISSUES CLOSED` Footer (Medium) **Location:** All commits All commits use `Refs: #590` instead of `ISSUES CLOSED: #590`. Per CONTRIBUTING.md and the issue template: *"The body should also include the issue reference footer (e.g., `ISSUES CLOSED: #45`)."* **Recommendation:** The squashed commit message should end with `ISSUES CLOSED: #590`. --- ### PROCESS-5: Type Label Mismatch (Medium) **Location:** PR labels The PR has `Type/Testing` but issue #590 is `Type/Bug`. Per CONTRIBUTING.md: *"Every PR must carry exactly one Type/ label that matches the nature of the change."* This PR fixes a bug (adds `session.commit()`) and includes tests -- the primary nature is a bug fix. **Recommendation:** Replace `Type/Testing` with `Type/Bug`. --- ### PROCESS-6: PR Not Mergeable (Blocking) **Location:** PR status The PR shows `mergeable: false`, indicating conflicts with the base branch. This must be resolved before merge. **Recommendation:** Rebase onto current `origin/master` and force-push. --- ### PROCESS-7: Issue #590 Subtasks Still Unchecked (Minor) **Location:** Issue #590 body All 5 subtask checkboxes in issue #590 are still `- [ ]`. Per workflow rules, subtasks should be checked off as completed. **Recommendation:** Update issue #590 body to check off completed subtasks. --- ### CODE-1: No `session.close()` in Repository Methods (Major, Pre-existing) **Location:** `repositories.py` -- `create()` (~line 2840), `get()` (~line 2871), `update()` (~line 2930), `delete()` (~line 2979) None of the methods call `session.close()`. Sessions are created via `self._session()` and never explicitly closed -- no `finally` block, no context manager. Under `@database_retry` (3 attempts), a single failed `get()` creates 3 leaked sessions. While SQLAlchemy GC eventually cleans up, relying on it is fragile and can exhaust SQLite file handles under load. This is pre-existing across the entire file and not introduced by this PR, so it should not block merge -- but a follow-up ticket should be filed. **Recommendation:** File a follow-up ticket. Minimal fix: wrap each method body in `try/finally: session.close()`. --- ### CODE-2: `@database_retry` Retries `ProjectNotFoundError` (Major, Pre-existing) **Location:** `retry_patterns.py:122-126`, `repositories.py:2782` `ProjectNotFoundError(DatabaseError)` means the `@database_retry` decorator (which retries on `DatabaseError`, 3 attempts, 0.5s wait) catches re-raised `ProjectNotFoundError` at the tenacity level. Every legitimate "not found" lookup wastes ~1.0s in retry sleeps. Same applies to `IntegrityError` -> `DatabaseError("already exists")` in `create()`. Pre-existing, already flagged by CoreRasurae (F4) and hamza.khyari (BUG-3). Not blocking this PR. **Recommendation:** File a follow-up ticket to exclude `ProjectNotFoundError` from the retry exception config or change its inheritance. --- ### CODE-3: Redundant `flush()` Before `commit()` (Nit) **Location:** `repositories.py` lines 2844-2845, 2955-2956, 2989-2990 `session.commit()` implicitly flushes, so the preceding `session.flush()` is technically redundant. Not harmful, but unnecessary. **Recommendation:** Optional cleanup -- remove the explicit `flush()` calls since `commit()` handles it. --- ## What's Good - The core fix is correct and well-analyzed (same root cause as #589) - `update()` and `delete()` were proactively fixed (addressing hamza.khyari's critical BUG-1) - Class docstring updated to match new behavior - Tests are comprehensive: 3 Behave BDD scenarios, 3 Robot integration tests, ASV benchmarks - Previous review feedback (CoreRasurae #1977, hamza.khyari #1995) was thoroughly addressed - Root cause analysis is well-documented in issue comments - CHANGELOG updated --- ## Summary Table | ID | Severity | Category | Blocking? | |----|----------|----------|-----------| | PROCESS-1 | Critical | Merge commit in branch | **YES** | | PROCESS-2 | Critical | Multiple commits for single issue | **YES** | | PROCESS-3 | Medium | Commit message mismatch | No (fixed by squash) | | PROCESS-4 | Medium | Missing `ISSUES CLOSED` footer | No (fixed by squash) | | PROCESS-5 | Medium | Type label mismatch | No | | PROCESS-6 | Blocking | PR not mergeable (conflicts) | **YES** | | PROCESS-7 | Minor | Issue subtasks unchecked | No | | CODE-1 | Major | Session leak (pre-existing) | No | | CODE-2 | Major | Retry on non-transient errors (pre-existing) | No | | CODE-3 | Nit | Redundant flush before commit | No | ## Verdict: REQUEST_CHANGES **Blocking items (must fix before merge):** 1. **PROCESS-1**: Remove merge commit via rebase 2. **PROCESS-2**: Squash all 8 commits into 1 with prescribed commit message 3. **PROCESS-6**: Resolve merge conflicts with master These are all fixable with a single `git rebase -i origin/master` + force-push. The code and tests themselves are solid.
hurui200320 added a new dependency 2026-03-06 09:39:44 +00:00
#590 `agent project show` fails to show a project.
aditya commented 2026-03-06 10:27:55 +00:00
Member
Copy Link
Copy Source

Code Review Report: Brent's Commits on feature/m3-fix-project-show-after-create

Latest Commit Reviewed: c9fe3c5485
Author: Brent E. Edwards (brent.edwards@cleverthis.com)
Message: fix(project): add session.commit() to delete/update and update docstring
Issue: #590 — agent project show fails to show a project
Branch: feature/m3-fix-project-show-after-create
Reviewer: Aditya

Scope of Changes

File Type Lines
CHANGELOG.md Modified +5 (plus 4 unrelated line edits — see F1)
benchmarks/project_show_after_create_bench.py New +116
features/project_show_after_create.feature New +29
features/steps/project_show_after_create_steps.py New +183
robot/project_show_after_create.robot New +67
src/cleveragents/infrastructure/database/repositories.py Modified +3 −2

The production fix (session.commit() added to create(), update(), and delete() in NamespacedProjectRepository) is correct and the docstring has been updated to match. The test design — file-based SQLite with a fresh session factory per CLI invocation — faithfully reproduces the real cross-process visibility bug. Previous review feedback from CoreRasurae and Hamza has been thoroughly addressed.

Findings

F1. [MEDIUM] CHANGELOG.md contains unrelated cosmetic edits bundled into the fix commits

Beyond the expected new entry for bug #590, the patch also converts four existing em-dashes (— / –) to double hyphens (--) in unrelated CHANGELOG entries:

-  Compresses parent plan context fragments by `skeleton_ratio` (0.0–1.0)
+  Compresses parent plan context fragments by `skeleton_ratio` (0.0--1.0)

-  or service call sites — integration is deferred to a future pass.
+  or service call sites -- integration is deferred to a future pass.

-  Added graph reachability validation — all nodes must be reachable
+  Added graph reachability validation -- all nodes must be reachable

-  Added `docs/reference/actor_config.md` — practical configuration reference
+  Added `docs/reference/actor_config.md` -- practical configuration reference

None of these lines are related to issue #590 or to the project show/create fix. CONTRIBUTING.md is explicit:

"Never bundle cosmetic changes (reformatting, renaming, whitespace fixes) with functional changes in the same commit."

These edits should not be present in this PR at all. They obscure the real diff, make the commit harder to review, and would make a future revert of this commit also revert the cosmetic changes.

Recommendation: Remove the four em-dash edits from the CHANGELOG. If a house style requires -- over —, do that as a standalone cosmetic-only commit in a separate PR.

F2. [MEDIUM] Robot tests create a per-test tmpdir alongside the Suite Setup environment

All three Robot test cases create their own isolated directory:

${tmpdir}=    Evaluate    __import__('tempfile').mkdtemp(prefix='show_590_')

yet the suite also declares:

Suite Setup       Setup Test Environment
Suite Teardown    Cleanup Test Environment

This means two separate temp environments exist simultaneously for each test run — one from common.resource's suite-level setup and one per test. Every other Robot file in this repo uses the suite-level environment exclusively. The intent here (full isolation per test case, fresh agents init per case) is valid, but the suite-level setup/teardown from common.resource is then entirely redundant and potentially confusing.

Recommendation: If each test case needs its own isolated environment (which is the right design for these regression tests), remove Suite Setup / Suite Teardown from this file and rely solely on the per-test tmpdir + [Teardown] Remove Directory. This matches the intent and removes the redundant lifecycle.

F3. [LOW] Third Robot test case asserts only on exit code — does not verify the error message

Project Show Returns Error For Nonexistent Project checks that project show local/nonexistent exits non-zero:

Should Not Be Equal As Integers    ${result.rc}    0
...    msg=project show for nonexistent project should fail but exited 0

There is no assertion on ${result.stdout} or ${result.stderr} content. If agents init fails silently, or the project DB is misconfigured, project show would also fail with a non-zero exit and this test would pass for the wrong reason. The Behave equivalent (Scenario 3) correctly asserts "not found" in the output. The Robot test should match.

Recommendation: Add a content assertion after the exit code check:

Should Contain    ${result.stdout}    not found
...    msg=project show for nonexistent project should report 'not found'

F4. [LOW] _make_fresh_repo and the benchmark's _fresh_repo call Base.metadata.create_all() on every invocation after the schema is already initialised

In step_init_show_db (the Background step), the schema is created once:

engine = create_engine(f"sqlite:///{context.show_db_path}", echo=False, poolclass=NullPool)
Base.metadata.create_all(engine)
engine.dispose()

Every subsequent call to _make_fresh_repo — one per create or show step — also runs:

engine = create_engine(...)
Base.metadata.create_all(engine)   # no-op but incurs schema introspection round-trip

The same applies in the benchmark: setup() creates the schema, then every _fresh_repo() call re-runs create_all. SQLAlchemy's checkfirst=True default makes this a no-op functionally, but it adds an unnecessary schema introspection query on every step and benchmark invocation.

Recommendation: Remove Base.metadata.create_all(engine) from _make_fresh_repo and _fresh_repo. The schema is guaranteed to exist after the Background step / setup() respectively. This also makes the helper's responsibility clearer — it creates a session factory, not a schema.

Summary Table

ID Severity Category Description
F1 MEDIUM Standards Four unrelated em-dash edits bundled into fix commits; violates CONTRIBUTING "no cosmetic with functional" rule
F2 MEDIUM Standards Robot suite uses both Suite Setup from common.resource and per-test mkdtemp() — redundant and inconsistent with repo pattern
F3 LOW Test Flaw Third Robot test only checks non-zero exit code; doesn't assert error message content
F4 LOW Performance _make_fresh_repo / _fresh_repo call Base.metadata.create_all() on every invocation after schema is already initialised
# Code Review Report: Brent's Commits on feature/m3-fix-project-show-after-create **Latest Commit Reviewed:** c9fe3c5485 **Author:** Brent E. Edwards (brent.edwards@cleverthis.com) **Message:** fix(project): add session.commit() to delete/update and update docstring **Issue:** #590 — agent project show fails to show a project **Branch:** feature/m3-fix-project-show-after-create **Reviewer:** Aditya --- ## Scope of Changes | File | Type | Lines | |------|------|-------| | `CHANGELOG.md` | Modified | +5 (plus 4 unrelated line edits — see F1) | | `benchmarks/project_show_after_create_bench.py` | New | +116 | | `features/project_show_after_create.feature` | New | +29 | | `features/steps/project_show_after_create_steps.py` | New | +183 | | `robot/project_show_after_create.robot` | New | +67 | | `src/cleveragents/infrastructure/database/repositories.py` | Modified | +3 −2 | The production fix (`session.commit()` added to `create()`, `update()`, and `delete()` in `NamespacedProjectRepository`) is correct and the docstring has been updated to match. The test design — file-based SQLite with a fresh session factory per CLI invocation — faithfully reproduces the real cross-process visibility bug. Previous review feedback from CoreRasurae and Hamza has been thoroughly addressed. --- ## Findings ### F1. [MEDIUM] CHANGELOG.md contains unrelated cosmetic edits bundled into the fix commits Beyond the expected new entry for bug #590, the patch also converts four existing em-dashes (`—` / `–`) to double hyphens (`--`) in unrelated CHANGELOG entries: ```diff - Compresses parent plan context fragments by `skeleton_ratio` (0.0–1.0) + Compresses parent plan context fragments by `skeleton_ratio` (0.0--1.0) - or service call sites — integration is deferred to a future pass. + or service call sites -- integration is deferred to a future pass. - Added graph reachability validation — all nodes must be reachable + Added graph reachability validation -- all nodes must be reachable - Added `docs/reference/actor_config.md` — practical configuration reference + Added `docs/reference/actor_config.md` -- practical configuration reference ``` None of these lines are related to issue #590 or to the project show/create fix. `CONTRIBUTING.md` is explicit: > "Never bundle cosmetic changes (reformatting, renaming, whitespace fixes) with functional changes in the same commit." These edits should not be present in this PR at all. They obscure the real diff, make the commit harder to review, and would make a future revert of this commit also revert the cosmetic changes. **Recommendation:** Remove the four em-dash edits from the CHANGELOG. If a house style requires `--` over `—`, do that as a standalone cosmetic-only commit in a separate PR. --- ### F2. [MEDIUM] Robot tests create a per-test `tmpdir` alongside the Suite Setup environment All three Robot test cases create their own isolated directory: ```robotframework ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_') ``` yet the suite also declares: ```robotframework Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment ``` This means two separate temp environments exist simultaneously for each test run — one from `common.resource`'s suite-level setup and one per test. Every other Robot file in this repo uses the suite-level environment exclusively. The intent here (full isolation per test case, fresh `agents init` per case) is valid, but the suite-level setup/teardown from `common.resource` is then entirely redundant and potentially confusing. **Recommendation:** If each test case needs its own isolated environment (which is the right design for these regression tests), remove `Suite Setup` / `Suite Teardown` from this file and rely solely on the per-test `tmpdir` + `[Teardown] Remove Directory`. This matches the intent and removes the redundant lifecycle. --- ### F3. [LOW] Third Robot test case asserts only on exit code — does not verify the error message `Project Show Returns Error For Nonexistent Project` checks that `project show local/nonexistent` exits non-zero: ```robotframework Should Not Be Equal As Integers ${result.rc} 0 ... msg=project show for nonexistent project should fail but exited 0 ``` There is no assertion on `${result.stdout}` or `${result.stderr}` content. If `agents init` fails silently, or the project DB is misconfigured, `project show` would also fail with a non-zero exit and this test would pass for the wrong reason. The Behave equivalent (Scenario 3) correctly asserts `"not found"` in the output. The Robot test should match. **Recommendation:** Add a content assertion after the exit code check: ```robotframework Should Contain ${result.stdout} not found ... msg=project show for nonexistent project should report 'not found' ``` --- ### F4. [LOW] `_make_fresh_repo` and the benchmark's `_fresh_repo` call `Base.metadata.create_all()` on every invocation after the schema is already initialised In `step_init_show_db` (the Background step), the schema is created once: ```python engine = create_engine(f"sqlite:///{context.show_db_path}", echo=False, poolclass=NullPool) Base.metadata.create_all(engine) engine.dispose() ``` Every subsequent call to `_make_fresh_repo` — one per `create` or `show` step — also runs: ```python engine = create_engine(...) Base.metadata.create_all(engine) # no-op but incurs schema introspection round-trip ``` The same applies in the benchmark: `setup()` creates the schema, then every `_fresh_repo()` call re-runs `create_all`. SQLAlchemy's `checkfirst=True` default makes this a no-op functionally, but it adds an unnecessary schema introspection query on every step and benchmark invocation. **Recommendation:** Remove `Base.metadata.create_all(engine)` from `_make_fresh_repo` and `_fresh_repo`. The schema is guaranteed to exist after the Background step / `setup()` respectively. This also makes the helper's responsibility clearer — it creates a session factory, not a schema. ## Summary Table | ID | Severity | Category | Description | |----|----------|----------|-------------| | F1 | MEDIUM | Standards | Four unrelated em-dash edits bundled into fix commits; violates CONTRIBUTING "no cosmetic with functional" rule | | F2 | MEDIUM | Standards | Robot suite uses both `Suite Setup` from `common.resource` and per-test `mkdtemp()` — redundant and inconsistent with repo pattern | | F3 | LOW | Test Flaw | Third Robot test only checks non-zero exit code; doesn't assert error message content | | F4 | LOW | Performance | `_make_fresh_repo` / `_fresh_repo` call `Base.metadata.create_all()` on every invocation after schema is already initialised |
brent.edwards force-pushed feature/m3-fix-project-show-after-create from c9fe3c5485 to 9c301afe4a 2026-03-06 20:57:21 +00:00 Compare
brent.edwards commented 2026-03-06 20:57:40 +00:00
Author
Member
Copy Link
Copy Source

Branch has been squashed into a single commit (9c301afe) and rebased onto master. All review feedback from hurui200320 and Aditya has been addressed:

Review fixes applied:

  • F1: Removed unrelated em-dash CHANGELOG edits — wrote clean entry from scratch
  • F2: Kept Suite Setup/Teardown (required for ${PYTHON} variable); updated stale documentation
  • F3: Added "not found" string assertion to Robot negative test case (nonexistent project)
  • F4: Removed redundant Base.metadata.create_all() from _make_fresh_repo() helper
  • Updated all stale TDD "expected to fail" comments — this PR includes the fix
  • Added finally: session.close() to all three mutating methods

Pre-commit hooks passed: lint, format, typecheck, bandit, vulture, conventional commit.

Ready for CI and merge.

Branch has been squashed into a single commit (`9c301afe`) and rebased onto `master`. All review feedback from hurui200320 and Aditya has been addressed: **Review fixes applied:** - **F1**: Removed unrelated em-dash CHANGELOG edits — wrote clean entry from scratch - **F2**: Kept Suite Setup/Teardown (required for `${PYTHON}` variable); updated stale documentation - **F3**: Added "not found" string assertion to Robot negative test case (nonexistent project) - **F4**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()` helper - Updated all stale TDD "expected to fail" comments — this PR includes the fix - Added `finally: session.close()` to all three mutating methods **Pre-commit hooks passed:** lint, format, typecheck, bandit, vulture, conventional commit. Ready for CI and merge.
brent.edwards added
Type
Bug
 and removed
Type
Testing
 labels 2026-03-06 20:57:50 +00:00
freemo commented 2026-03-06 23:35:18 +00:00
Owner
Copy Link
Copy Source

PM Note (Day 26 — M3 PR Triage):

Production bug fix — show created project after creation. All review findings addressed across ~5 rounds. Latest review stale after squash. Has a merge conflict.

@brent.edwards — Please rebase onto master. Needs fresh approval after rebase.

**PM Note (Day 26 — M3 PR Triage):** Production bug fix — show created project after creation. All review findings addressed across ~5 rounds. Latest review stale after squash. Has a **merge conflict**. @brent.edwards — Please rebase onto `master`. Needs fresh approval after rebase.
brent.edwards commented 2026-03-07 02:15:47 +00:00
Author
Member
Copy Link
Copy Source

Unified TDD Test Toggle Convention

After auditing all 7 PRs that add failing BDD tests (#566, #567, #568, #591, #593, #594, #596), we are standardizing on a single @wip tag mechanism.

The standard

Action Behave (.feature) Robot (.robot)
Turn OFF (skip in CI) Add @wip tag above the Scenario Add [Tags] wip to the test case
Turn ON (run in CI) Remove the @wip tag Remove wip from [Tags]
Run locally despite @wip behave features/foo.feature:42 (by line number) robot --include wip robot/foo.robot

Status of this PR

No changes required. This is a fix PR — it includes the flush() → commit() production fix on create(), update(), and delete(), so all 3 Behave scenarios and both Robot tests pass genuinely. No @wip tag is needed.

This PR correctly uses @tdd @bug590 tags (not @wip) since the tests are expected to pass.

## Unified TDD Test Toggle Convention After auditing all 7 PRs that add failing BDD tests (#566, #567, #568, #591, #593, #594, #596), we are standardizing on a single `@wip` tag mechanism. ### The standard | Action | Behave (`.feature`) | Robot (`.robot`) | |---|---|---| | **Turn OFF** (skip in CI) | Add `@wip` tag above the Scenario | Add `[Tags] wip` to the test case | | **Turn ON** (run in CI) | Remove the `@wip` tag | Remove `wip` from `[Tags]` | | **Run locally despite @wip** | `behave features/foo.feature:42` (by line number) | `robot --include wip robot/foo.robot` | ### Status of this PR **No changes required.** This is a fix PR — it includes the `flush()` → `commit()` production fix on `create()`, `update()`, and `delete()`, so all 3 Behave scenarios and both Robot tests pass genuinely. No `@wip` tag is needed. This PR correctly uses `@tdd @bug590` tags (not `@wip`) since the tests are expected to pass.
brent.edwards commented 2026-03-07 03:36:53 +00:00
Author
Member
Copy Link
Copy Source

CI Fix + Master Merge — 47ec9095

Fixes applied

1. BDD unit tests — 3 failing create scenarios (features/steps/repositories_error_handling_coverage_steps.py)

Root cause: PR changed NamespacedProjectRepository.create() from session.flush() to session.commit(), but the shared mock session helpers (_mock_session_op_error_on_flush, _mock_session_unique_integrity_on_flush, _mock_session_non_unique_integrity_on_flush, _mock_session_integrity_on_flush) only set mock.flush.side_effect. The commit() call hit a default MagicMock (no exception), so the error path was never triggered.

Fix: Added mock.commit.side_effect mirroring the existing mock.flush.side_effect to all 4 mock helpers. This is safe for other repository tests that still call flush() — they never call commit(), so the new side effect is inert for them.

2. Robot integration tests — 3 failing --format scenarios (robot/project_show_after_create.robot)

Root cause: --format plain was placed before project show in the command line (cleveragents --format plain project show ...), but --format is an option on the show subcommand, not the top-level CLI. Typer raised NoSuchOption.

Fix: Moved --format plain after project show (cleveragents project show --format plain ...).

Master merge

Merged master (272f9078) into branch. Single conflict in CHANGELOG.md — resolved with PR entry first, followed by all master entries.

Quality gates

Gate Result
nox -s lint Pass
nox -s typecheck Pass (0 errors)
nox -s unit_tests Pass — 9,032 scenarios, 0 failures

PR is now mergeable: true.

## CI Fix + Master Merge — `47ec9095` ### Fixes applied **1. BDD unit tests — 3 failing `create` scenarios** (`features/steps/repositories_error_handling_coverage_steps.py`) Root cause: PR changed `NamespacedProjectRepository.create()` from `session.flush()` to `session.commit()`, but the shared mock session helpers (`_mock_session_op_error_on_flush`, `_mock_session_unique_integrity_on_flush`, `_mock_session_non_unique_integrity_on_flush`, `_mock_session_integrity_on_flush`) only set `mock.flush.side_effect`. The `commit()` call hit a default MagicMock (no exception), so the error path was never triggered. Fix: Added `mock.commit.side_effect` mirroring the existing `mock.flush.side_effect` to all 4 mock helpers. This is safe for other repository tests that still call `flush()` — they never call `commit()`, so the new side effect is inert for them. **2. Robot integration tests — 3 failing `--format` scenarios** (`robot/project_show_after_create.robot`) Root cause: `--format plain` was placed before `project show` in the command line (`cleveragents --format plain project show ...`), but `--format` is an option on the `show` subcommand, not the top-level CLI. Typer raised `NoSuchOption`. Fix: Moved `--format plain` after `project show` (`cleveragents project show --format plain ...`). ### Master merge Merged `master` (`272f9078`) into branch. Single conflict in `CHANGELOG.md` — resolved with PR entry first, followed by all master entries. ### Quality gates | Gate | Result | |------|--------| | `nox -s lint` | Pass | | `nox -s typecheck` | Pass (0 errors) | | `nox -s unit_tests` | Pass — **9,032 scenarios, 0 failures** | PR is now `mergeable: true`.
freemo reviewed 2026-03-07 23:13:00 +00:00
freemo left a comment
Owner
Copy Link
Copy Source

Planning Authority Review — Ready for Merge

Branch naming non-compliance noted: This PR uses the feature/m3-fix-* prefix, but as a bug fix PR (fixes #590), it should use the bugfix/ prefix per CONTRIBUTING.md conventions. This is flagged for future compliance only — it should not block merging given the work is in advanced review and all review rounds are complete.

TDD counterpart: Issue #633 has been created to track the TDD/test coverage counterpart for this fix.

Recommendation: Merge. All review rounds are complete, CI is passing, and the branch has been rebased on master.

## Planning Authority Review — Ready for Merge **Branch naming non-compliance noted:** This PR uses the `feature/m3-fix-*` prefix, but as a bug fix PR (fixes #590), it should use the `bugfix/` prefix per CONTRIBUTING.md conventions. This is flagged for future compliance only — it should **not** block merging given the work is in advanced review and all review rounds are complete. **TDD counterpart:** Issue #633 has been created to track the TDD/test coverage counterpart for this fix. **Recommendation: Merge.** All review rounds are complete, CI is passing, and the branch has been rebased on master.
freemo commented 2026-03-09 02:45:21 +00:00
Owner
Copy Link
Copy Source

Day 29 PM Status Check

PR appears ready for merge after multiple review rounds. All findings addressed. Last update: 47ec9095 — 9,032 scenarios passing. Merged master successfully.

Status: READY FOR MERGE (after #591 merges). Requesting @aditya and @CoreRasurae as reviewers.

Merge order: Second in M3 bug-fix series (#591 -> #593 -> #594). May need rebase after #591 merges.

**Day 29 PM Status Check** PR appears ready for merge after multiple review rounds. All findings addressed. Last update: `47ec9095` — 9,032 scenarios passing. Merged master successfully. **Status**: READY FOR MERGE (after #591 merges). Requesting @aditya and @CoreRasurae as reviewers. **Merge order**: Second in M3 bug-fix series (#591 -> #593 -> #594). May need rebase after #591 merges.
hurui200320 approved these changes 2026-03-09 07:43:00 +00:00
Dismissed
hurui200320 left a comment
Member
Copy Link
Copy Source

Code Review — PR #593 (Commits 9c301afe through 47ec9095)

Reviewed against: Issue #590, branch feature/m3-fix-project-show-after-create, docs/specification.md, CONTRIBUTING.md
Prior reviews: #2006 (hurui200320/me, REQUEST_CHANGES), #1977 (CoreRasurae), #1995 (hamza.khyari), #2040 (freemo)

Overview

The production fix is correct and well-tested. Adding session.commit() to create(), update(), and delete() in NamespacedProjectRepository resolves the reported bug. The finally: session.close() guards are a proper addition. Tests are comprehensive: 3 Behave BDD scenarios, 3 Robot integration tests, and ASV benchmarks. Previous review feedback from CoreRasurae, hamza.khyari, and Aditya has been substantially addressed.

Findings

PROCESS-1: Merge Commit in Branch (Non-blocking)

  • Location: Commit 47ec9095 — Merge remote-tracking branch 'https-origin/master' into HEAD
  • Problem: CONTRIBUTING.md line 148 prescribes rebase to align with master, not merge. The branch should be cleaned up via git rebase -i origin/master.
  • Recommendation: Squash + rebase before merge to produce a clean single-commit history.

PROCESS-2: Multiple Commits for Single Issue (Non-blocking)

  • Location: Branch has 3 commits (9c301afe, 3a7d648f, 47ec9095) instead of the prescribed 1.
  • Problem: CONTRIBUTING.md line 865: "Each issue corresponds to exactly one commit."
  • Recommendation: Squash all into a single commit with the prescribed message fix(project): show created project after creation and ISSUES CLOSED: #590 footer.

PROCESS-3: PR Not Mergeable (Non-blocking, but requires action before merge)

  • Location: PR metadata shows mergeable: false due to CHANGELOG.md conflicts.
  • Recommendation: Rebase onto current origin/master to resolve.

PROCESS-4: Commit Footer Format (Non-blocking)

  • Location: Commit 9c301afe uses Closes: #590.
  • Problem: CONTRIBUTING.md example (line 191) uses ISSUES CLOSED: #590.
  • Recommendation: Use the standardized ISSUES CLOSED: #590 footer in the final commit.

BENCH-1: Benchmark _fresh_repo Still Calls Base.metadata.create_all() (Low, Non-blocking)

  • Location: benchmarks/project_show_after_create_bench.py:54
  • Problem: Aditya's F4 recommended removing this from both helpers. The Behave helper was fixed, but the benchmark helper still runs DDL on every call — a no-op functionally (SQLAlchemy checkfirst=True), but adds unnecessary schema introspection overhead per benchmark iteration.
  • Recommendation: Remove Base.metadata.create_all(engine) from _fresh_repo() for consistency.

CODE-1: Redundant flush() Before commit() (Nit, Non-blocking)

  • Location: repositories.py:2956-2957 (update()) and 2992-2993 (delete())
  • Problem: session.commit() implicitly flushes, so the preceding session.flush() is redundant. create() at line 2844 correctly uses only commit().
  • Recommendation: Optional cleanup — remove the session.flush() calls for consistency with create().

What's Good

  • Core fix is correct: session.commit() resolves the persistence visibility bug
  • update() and delete() proactively fixed (addressing hamza.khyari's critical BUG-1)
  • finally: session.close() properly guards session lifecycle for all mutating methods
  • Class docstring updated to match new commit behavior
  • Mock helpers correctly extended with commit.side_effect — properly handles both the create() path (commit-only) and update()/delete() paths (flush-then-commit)
  • Behave tests use proper isolation (file-based SQLite, fresh repo per CLI invocation, NullPool)
  • Robot tests verify exit codes, content assertions, and negative case with combined stdout+stderr
  • No security issues introduced
  • No # type: ignore comments added

Summary Table

ID Severity Category Blocking?
PROCESS-1 Medium Merge commit in branch No
PROCESS-2 Medium 3 commits for 1 issue No
PROCESS-3 Medium PR not mergeable (conflicts) No (but must be resolved before merge)
PROCESS-4 Low Commit footer format No
BENCH-1 Low Redundant DDL in benchmark No
CODE-1 Nit Redundant flush before commit No

Verdict: APPROVED

The code and tests are solid. No blocking bugs, security issues, or test flaws. Process items (merge commit, multiple commits, conflicts) should ideally be cleaned up before merge via a single git rebase -i origin/master + force-push, but are not blocking approval.

# Code Review — PR #593 (Commits `9c301afe` through `47ec9095`) **Reviewed against:** Issue #590, branch `feature/m3-fix-project-show-after-create`, `docs/specification.md`, `CONTRIBUTING.md` **Prior reviews:** #2006 (hurui200320/me, REQUEST_CHANGES), #1977 (CoreRasurae), #1995 (hamza.khyari), #2040 (freemo) ## Overview The production fix is correct and well-tested. Adding `session.commit()` to `create()`, `update()`, and `delete()` in `NamespacedProjectRepository` resolves the reported bug. The `finally: session.close()` guards are a proper addition. Tests are comprehensive: 3 Behave BDD scenarios, 3 Robot integration tests, and ASV benchmarks. Previous review feedback from CoreRasurae, hamza.khyari, and Aditya has been substantially addressed. --- ## Findings ### PROCESS-1: Merge Commit in Branch (Non-blocking) - **Location:** Commit `47ec9095` — `Merge remote-tracking branch 'https-origin/master' into HEAD` - **Problem:** CONTRIBUTING.md line 148 prescribes rebase to align with master, not merge. The branch should be cleaned up via `git rebase -i origin/master`. - **Recommendation:** Squash + rebase before merge to produce a clean single-commit history. ### PROCESS-2: Multiple Commits for Single Issue (Non-blocking) - **Location:** Branch has 3 commits (`9c301afe`, `3a7d648f`, `47ec9095`) instead of the prescribed 1. - **Problem:** CONTRIBUTING.md line 865: *"Each issue corresponds to exactly one commit."* - **Recommendation:** Squash all into a single commit with the prescribed message `fix(project): show created project after creation` and `ISSUES CLOSED: #590` footer. ### PROCESS-3: PR Not Mergeable (Non-blocking, but requires action before merge) - **Location:** PR metadata shows `mergeable: false` due to CHANGELOG.md conflicts. - **Recommendation:** Rebase onto current `origin/master` to resolve. ### PROCESS-4: Commit Footer Format (Non-blocking) - **Location:** Commit `9c301afe` uses `Closes: #590`. - **Problem:** CONTRIBUTING.md example (line 191) uses `ISSUES CLOSED: #590`. - **Recommendation:** Use the standardized `ISSUES CLOSED: #590` footer in the final commit. ### BENCH-1: Benchmark `_fresh_repo` Still Calls `Base.metadata.create_all()` (Low, Non-blocking) - **Location:** `benchmarks/project_show_after_create_bench.py:54` - **Problem:** Aditya's F4 recommended removing this from both helpers. The Behave helper was fixed, but the benchmark helper still runs DDL on every call — a no-op functionally (SQLAlchemy `checkfirst=True`), but adds unnecessary schema introspection overhead per benchmark iteration. - **Recommendation:** Remove `Base.metadata.create_all(engine)` from `_fresh_repo()` for consistency. ### CODE-1: Redundant `flush()` Before `commit()` (Nit, Non-blocking) - **Location:** `repositories.py:2956-2957` (`update()`) and `2992-2993` (`delete()`) - **Problem:** `session.commit()` implicitly flushes, so the preceding `session.flush()` is redundant. `create()` at line 2844 correctly uses only `commit()`. - **Recommendation:** Optional cleanup — remove the `session.flush()` calls for consistency with `create()`. --- ## What's Good - Core fix is correct: `session.commit()` resolves the persistence visibility bug - `update()` and `delete()` proactively fixed (addressing hamza.khyari's critical BUG-1) - `finally: session.close()` properly guards session lifecycle for all mutating methods - Class docstring updated to match new commit behavior - Mock helpers correctly extended with `commit.side_effect` — properly handles both the `create()` path (commit-only) and `update()`/`delete()` paths (flush-then-commit) - Behave tests use proper isolation (file-based SQLite, fresh repo per CLI invocation, `NullPool`) - Robot tests verify exit codes, content assertions, and negative case with combined stdout+stderr - No security issues introduced - No `# type: ignore` comments added --- ## Summary Table | ID | Severity | Category | Blocking? | |----|----------|----------|-----------| | PROCESS-1 | Medium | Merge commit in branch | No | | PROCESS-2 | Medium | 3 commits for 1 issue | No | | PROCESS-3 | Medium | PR not mergeable (conflicts) | No (but must be resolved before merge) | | PROCESS-4 | Low | Commit footer format | No | | BENCH-1 | Low | Redundant DDL in benchmark | No | | CODE-1 | Nit | Redundant flush before commit | No | ## Verdict: APPROVED The code and tests are solid. No blocking bugs, security issues, or test flaws. Process items (merge commit, multiple commits, conflicts) should ideally be cleaned up before merge via a single `git rebase -i origin/master` + force-push, but are not blocking approval.
hurui200320 commented 2026-03-09 07:44:46 +00:00
Member
Copy Link
Copy Source

According to Jeff's comment here: #494 (comment)

Since OpenCode gives the review and I have confirmed the correctness of the review, and it's approved, I'll fix the process issue and merge the PR.

According to Jeff's comment here: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/494#issuecomment-55471 Since OpenCode gives the review and I have confirmed the correctness of the review, and it's approved, I'll fix the process issue and merge the PR.
hurui200320 force-pushed feature/m3-fix-project-show-after-create from 47ec9095b2 to 946db693af 2026-03-09 07:51:13 +00:00 Compare
hurui200320 dismissed hurui200320's review 2026-03-09 07:51:13 +00:00
Reason:

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

hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-09 07:51:51 +00:00
hurui200320 canceled auto merging this pull request when all checks succeed 2026-03-09 07:52:04 +00:00
hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-09 07:54:30 +00:00
hurui200320 merged commit 16dc4ab18d into master 2026-03-09 07:56:39 +00:00
hurui200320 deleted branch feature/m3-fix-project-show-after-create 2026-03-09 07:56:39 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
brent.edwards
CoreRasurae
hamza.khyari
freemo
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
Priority
Medium
State
In Review
Type
Bug
Milestone
No items
No Milestone v3.2.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 freemo
6 Participants
Notifications
Due Date
No due date set.
Blocks
#590 `agent project show` fails to show a project.
cleveragents/cleveragents-core
Reference: cleveragents/cleveragents-core#593
Delete Branch "feature/m3-fix-project-show-after-create"

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?