fix(project): persist project to database during creation #591

Merged
hurui200320 merged 1 commit from feature/m3-fix-project-create-persist into master 2026-03-09 06:59:16 +00:00
Member

Summary

Fix agents project create not persisting projects to the database. NamespacedProjectRepository.create() called session.flush() but never session.commit(), so projects were invisible to subsequent agents project list invocations that open a separate session.

Changes

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

  • Replace session.flush() with session.commit() in NamespacedProjectRepository.create()
  • Add finally: session.close() guard for proper session lifecycle

Tests & benchmarks:

  • 4 Behave BDD regression scenarios (features/project_create_persist.feature)
  • Robot Framework integration smoke tests (robot/project_create_persist.robot)
  • ASV benchmarks for create-then-list round-trip (benchmarks/project_create_persist_bench.py)

Review feedback addressed

  • H3: Added finally: session.close() to create() method
  • M1: Removed redundant session.flush() before session.commit()
  • M2: Updated stale TDD "expected to fail" comments — this PR includes the fix
  • M4: Rewrote CHANGELOG entry to describe the fix, not just tests
  • F1: Updated Robot documentation (Suite Setup/Teardown kept — needed for ${PYTHON})
  • F2: Fixed bare assertion "my-app" to "local/my-app" in namespace scenario
  • F3: Removed redundant Base.metadata.create_all() from _make_fresh_repo()

Process

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

ISSUES CLOSED: #589

## Summary Fix `agents project create` not persisting projects to the database. `NamespacedProjectRepository.create()` called `session.flush()` but never `session.commit()`, so projects were invisible to subsequent `agents project list` invocations that open a separate session. ## Changes **Production fix** (`src/cleveragents/infrastructure/database/repositories.py`): - Replace `session.flush()` with `session.commit()` in `NamespacedProjectRepository.create()` - Add `finally: session.close()` guard for proper session lifecycle **Tests & benchmarks**: - 4 Behave BDD regression scenarios (`features/project_create_persist.feature`) - Robot Framework integration smoke tests (`robot/project_create_persist.robot`) - ASV benchmarks for create-then-list round-trip (`benchmarks/project_create_persist_bench.py`) ## Review feedback addressed - **H3**: Added `finally: session.close()` to `create()` method - **M1**: Removed redundant `session.flush()` before `session.commit()` - **M2**: Updated stale TDD "expected to fail" comments — this PR includes the fix - **M4**: Rewrote CHANGELOG entry to describe the fix, not just tests - **F1**: Updated Robot documentation (Suite Setup/Teardown kept — needed for `${PYTHON}`) - **F2**: Fixed bare assertion `"my-app"` to `"local/my-app"` in namespace scenario - **F3**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()` ## Process - Single squashed commit, rebased onto `master` (no merge commits) - Prescribed commit message from issue #589 metadata ISSUES CLOSED: #589
brent.edwards added this to the v3.2.0 milestone 2026-03-05 01:20:44 +00:00
brent.edwards left a comment

Self-Review: TDD Failing Tests for Bug #589

Overview

This PR adds TDD failing tests that reproduce the persistence bug where agents project create flushes but never commits, making projects invisible to agents project list.

Checklist

  • Feature file has @wip tag on all scenarios (for CI filtering)
  • Feature file has @tdd and @bug589 tags for traceability
  • Step patterns use unique prefixes (via the persist CLI, persist project list, persist duplicate creation) to avoid AmbiguousStep collisions with existing step files
  • Test design uses file-based SQLite with fresh repo per invocation (not shared session) to faithfully reproduce the bug
  • nox -s lint passes
  • nox -s typecheck passes (0 errors)
  • Commit message matches issue metadata exactly
  • Commit uses Refs: #589 (not ISSUES CLOSED:) to avoid premature closure
  • Robot Framework smoke tests follow common.resource pattern
  • ASV benchmark includes track_list_after_create_count metric that returns 0 (bug present) or 1 (fix applied)
  • CHANGELOG entry added

TDD Nature

All 3 BDD scenarios and both Robot test cases are expected to fail until the bug fix is applied. This is by design — the tests define correct behaviour that the code does not yet exhibit.

No Issues Found

The implementation looks correct for a TDD test PR. Ready for external review.

## Self-Review: TDD Failing Tests for Bug #589 ### Overview This PR adds TDD failing tests that reproduce the persistence bug where `agents project create` flushes but never commits, making projects invisible to `agents project list`. ### Checklist - [x] Feature file has `@wip` tag on all scenarios (for CI filtering) - [x] Feature file has `@tdd` and `@bug589` tags for traceability - [x] Step patterns use unique prefixes (`via the persist CLI`, `persist project list`, `persist duplicate creation`) to avoid `AmbiguousStep` collisions with existing step files - [x] Test design uses file-based SQLite with fresh repo per invocation (not shared session) to faithfully reproduce the bug - [x] `nox -s lint` passes - [x] `nox -s typecheck` passes (0 errors) - [x] Commit message matches issue metadata exactly - [x] Commit uses `Refs: #589` (not `ISSUES CLOSED:`) to avoid premature closure - [x] Robot Framework smoke tests follow `common.resource` pattern - [x] ASV benchmark includes `track_list_after_create_count` metric that returns 0 (bug present) or 1 (fix applied) - [x] CHANGELOG entry added ### TDD Nature All 3 BDD scenarios and both Robot test cases are **expected to fail** until the bug fix is applied. This is by design — the tests define correct behaviour that the code does not yet exhibit. ### No Issues Found The implementation looks correct for a TDD test PR. Ready for external review.
freemo self-assigned this 2026-03-05 03:32:33 +00:00
freemo left a comment

PM Review — Day 25

Status

  • Mergeable — no conflicts.
  • Self-review from Brent only. No external review.
  • Empty PR body — the self-review comment and issue #589 comment serve as documentation.

Context

This is a TDD bug-fix test PR for #589 (project persistence bug — flush() without commit() in NamespacedProjectRepository.create()). Tests use file-based SQLite to faithfully reproduce the cross-session visibility problem.

Action Item

@freemo: Please review this PR and implement the fix (add session.commit() to the create path in repositories.py:2828-2855). The test suite is designed to fail until the fix is applied.

Priority

Medium — blocks M3 acceptance gate. Related to #590 (same root cause).

## PM Review — Day 25 ### Status - **Mergeable** — no conflicts. - Self-review from Brent only. **No external review.** - Empty PR body — the self-review comment and issue #589 comment serve as documentation. ### Context This is a TDD bug-fix test PR for #589 (project persistence bug — `flush()` without `commit()` in `NamespacedProjectRepository.create()`). Tests use file-based SQLite to faithfully reproduce the cross-session visibility problem. ### Action Item **@freemo**: Please review this PR and implement the fix (add `session.commit()` to the create path in `repositories.py:2828-2855`). The test suite is designed to fail until the fix is applied. ### Priority Medium — blocks M3 acceptance gate. Related to #590 (same root cause).
freemo force-pushed feature/m3-fix-project-create-persist from 156c70bce9
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / build (pull_request) Successful in 17s
CI / quality (pull_request) Successful in 19s
CI / security (pull_request) Successful in 34s
CI / typecheck (pull_request) Successful in 35s
CI / unit_tests (pull_request) Failing after 2m20s
CI / docker (pull_request) Has been skipped
CI / integration_tests (pull_request) Failing after 2m58s
CI / coverage (pull_request) Successful in 5m27s
CI / benchmark-regression (pull_request) Successful in 28m57s
to bd7953f0a5
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 18s
CI / typecheck (pull_request) Successful in 36s
CI / security (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 2m9s
CI / integration_tests (pull_request) Successful in 2m56s
CI / docker (pull_request) Successful in 55s
CI / coverage (pull_request) Successful in 4m25s
CI / benchmark-regression (pull_request) Successful in 28m56s
2026-03-05 05:55:01 +00:00
Compare
CoreRasurae left a comment

Code Review -- Commit 178a315b (Brent Edwards)

Reviewed against: Issue #589 acceptance criteria, docs/specification.md, and codebase conventions.

The test design is sound -- file-based SQLite with separate sessions per invocation correctly reproduces the cross-process persistence bug. The Behave scenario logic, the Robot integration approach, and the ASV benchmark concept are all appropriate.

That said, there are 10 findings (2 HIGH, 4 MEDIUM, 4 LOW) across the five files. The highest-priority items are in the Robot Framework tests where multiple commands silently swallow failures, which would produce misleading CI errors that hide the actual root cause.

Findings Summary

ID Severity Category File Description
F1 HIGH Test Flaws robot/project_create_persist.robot Second test case doesn't verify any exit codes for init or create
F2 HIGH Test Flaws robot/project_create_persist.robot agents init exit code unchecked in both test cases
F3 MEDIUM Test Flaws features/steps/project_create_persist_steps.py Create step never asserts exit_code == 0; failure surfaces as confusing list assertion
F4 MEDIUM Convention benchmarks/project_create_persist_bench.py Import pattern deviates from the established _SRC / importlib.reload convention used in 166 other benchmarks
F5 MEDIUM Bug Detection benchmarks/project_create_persist_bench.py track_list_after_create_count uses hardcoded name "bench-track" -- not idempotent; UNIQUE violation on repeated calls
F6 MEDIUM Performance Steps + Benchmark create_engine() called but never engine.dispose() -- resource/file-lock leaks across scenarios
F7 LOW Test Flaws robot/project_create_persist.robot Second test doesn't check ${list.rc} == 0 (first test does)
F8 LOW Convention Commit message fix(project) prefix implies a bug fix, but commit only adds tests; test(project) would be more accurate
F9 LOW Coverage features/project_create_persist.feature All scenarios use fully-qualified names (local/...); no bare-name test matching the issue's own repro steps
F10 LOW Convention features/steps/project_create_persist_steps.py _PATCH_STORE_EXTRAS mock is unnecessary since --invariant flag is never passed

Acceptance Criteria Coverage (Issue #589)

Criterion Behave Robot Status
project create persists to DB Scenario 1 Test 1 Covered
project list shows created project Scenario 1 Test 1 Covered
Multiple projects created and listed Scenario 2 Test 2 Covered
Duplicate name produces clear error Scenario 3 -- Partial -- Robot has no duplicate test

Inline comments below provide details and recommendations for each finding.

## Code Review -- Commit `178a315b` (Brent Edwards) **Reviewed against:** Issue #589 acceptance criteria, `docs/specification.md`, and codebase conventions. The test design is sound -- file-based SQLite with separate sessions per invocation correctly reproduces the cross-process persistence bug. The Behave scenario logic, the Robot integration approach, and the ASV benchmark concept are all appropriate. That said, there are **10 findings** (2 HIGH, 4 MEDIUM, 4 LOW) across the five files. The highest-priority items are in the Robot Framework tests where multiple commands silently swallow failures, which would produce misleading CI errors that hide the actual root cause. ### Findings Summary | ID | Severity | Category | File | Description | |:---|:---------|:---------|:-----|:------------| | F1 | **HIGH** | Test Flaws | `robot/project_create_persist.robot` | Second test case doesn't verify any exit codes for `init` or `create` | | F2 | **HIGH** | Test Flaws | `robot/project_create_persist.robot` | `agents init` exit code unchecked in **both** test cases | | F3 | MEDIUM | Test Flaws | `features/steps/project_create_persist_steps.py` | Create step never asserts `exit_code == 0`; failure surfaces as confusing list assertion | | F4 | MEDIUM | Convention | `benchmarks/project_create_persist_bench.py` | Import pattern deviates from the established `_SRC` / `importlib.reload` convention used in 166 other benchmarks | | F5 | MEDIUM | Bug Detection | `benchmarks/project_create_persist_bench.py` | `track_list_after_create_count` uses hardcoded name `"bench-track"` -- not idempotent; UNIQUE violation on repeated calls | | F6 | MEDIUM | Performance | Steps + Benchmark | `create_engine()` called but never `engine.dispose()` -- resource/file-lock leaks across scenarios | | F7 | LOW | Test Flaws | `robot/project_create_persist.robot` | Second test doesn't check `${list.rc} == 0` (first test does) | | F8 | LOW | Convention | Commit message | `fix(project)` prefix implies a bug fix, but commit only adds tests; `test(project)` would be more accurate | | F9 | LOW | Coverage | `features/project_create_persist.feature` | All scenarios use fully-qualified names (`local/...`); no bare-name test matching the issue's own repro steps | | F10 | LOW | Convention | `features/steps/project_create_persist_steps.py` | `_PATCH_STORE_EXTRAS` mock is unnecessary since `--invariant` flag is never passed | ### Acceptance Criteria Coverage (Issue #589) | Criterion | Behave | Robot | Status | |:---|:---:|:---:|:---| | `project create` persists to DB | Scenario 1 | Test 1 | Covered | | `project list` shows created project | Scenario 1 | Test 1 | Covered | | Multiple projects created and listed | Scenario 2 | Test 2 | Covered | | Duplicate name produces clear error | Scenario 3 | -- | **Partial** -- Robot has no duplicate test | Inline comments below provide details and recommendations for each finding.
@ -0,0 +19,4 @@
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker
Member

F4 -- MEDIUM | Convention: This try/except ModuleNotFoundError import pattern deviates from the established convention used across the other 166 benchmark files in this repo (e.g., acms_backends_bench.py, action_cli_bench.py). The standard pattern is:

_SRC = str(Path(__file__).resolve().parents[1] / "src")
if _SRC not in sys.path:
    sys.path.insert(0, _SRC)
import cleveragents
importlib.reload(cleveragents)

The try/except approach has two practical downsides:

  1. Skips importlib.reload() -- ASV may load a stale installed package instead of the source tree.
  2. Masks non-ModuleNotFoundError failures -- a renamed class or circular import would trigger the except branch, insert the path, and attempt the import again with a confusing double traceback.

Recommendation: Align with the codebase convention.

**F4 -- MEDIUM | Convention**: This try/except `ModuleNotFoundError` import pattern deviates from the established convention used across the other 166 benchmark files in this repo (e.g., `acms_backends_bench.py`, `action_cli_bench.py`). The standard pattern is: ```python _SRC = str(Path(__file__).resolve().parents[1] / "src") if _SRC not in sys.path: sys.path.insert(0, _SRC) import cleveragents importlib.reload(cleveragents) ``` The try/except approach has two practical downsides: 1. **Skips `importlib.reload()`** -- ASV may load a stale installed package instead of the source tree. 2. **Masks non-`ModuleNotFoundError` failures** -- a renamed class or circular import would trigger the `except` branch, insert the path, and attempt the import again with a confusing double traceback. **Recommendation:** Align with the codebase convention.
@ -0,0 +46,4 @@
engine = create_engine(f"sqlite:///{db_path}", echo=False)
Base.metadata.create_all(engine)
factory = sessionmaker(bind=engine, expire_on_commit=False)
return NamespacedProjectRepository(session_factory=factory)
Member

F6 -- MEDIUM | Performance: Same engine leak issue as the Behave steps. create_engine() is called but never engine.dispose()-d. Consider using poolclass=NullPool or adding explicit engine.dispose() after the repo is used.

**F6 -- MEDIUM | Performance**: Same engine leak issue as the Behave steps. `create_engine()` is called but never `engine.dispose()`-d. Consider using `poolclass=NullPool` or adding explicit `engine.dispose()` after the repo is used.
@ -0,0 +83,4 @@
"""Track whether list sees the created project.
Returns 1 when create properly commits (fix applied); 0 while
bug #589 is present and list returns empty.
Member

F5 -- MEDIUM | Bug Detection: This method always creates a project named "bench-track". If ASV runs it more than once per setup() invocation (which can happen under certain ASV configurations or when comparing across commits), the second call will hit a UNIQUE constraint violation once the persistence fix is applied, crashing the benchmark.

Compare to time_create_then_list (line 74) which correctly uses self._counter to generate unique names per invocation.

Recommendation: Use self._counter or uuid4() for the project name:

self._counter += 1
parsed = parse_namespaced_name(f"bench-track-{self._counter}")
**F5 -- MEDIUM | Bug Detection**: This method always creates a project named `"bench-track"`. If ASV runs it more than once per `setup()` invocation (which can happen under certain ASV configurations or when comparing across commits), the second call will hit a UNIQUE constraint violation once the persistence fix is applied, crashing the benchmark. Compare to `time_create_then_list` (line 74) which correctly uses `self._counter` to generate unique names per invocation. **Recommendation:** Use `self._counter` or `uuid4()` for the project name: ```python self._counter += 1 parsed = parse_namespaced_name(f"bench-track-{self._counter}") ```
@ -0,0 +8,4 @@
Given a fresh project-persist database is initialised
@tdd @bug589
Scenario: Created project appears in project list
Member

F9 -- LOW | Coverage: All three scenarios use fully-qualified names (local/my-app, local/alpha, local/beta). The issue's own reproduction steps use a bare name (agents project create project1) which relies on parse_namespaced_name defaulting to the local/ namespace. Adding one scenario with a bare name (e.g., project1 instead of local/project1) would more faithfully reproduce the reported bug and exercise the default-namespace resolution path end-to-end.

This is minor since parse_namespaced_name has its own test suite, but worth noting for completeness.

**F9 -- LOW | Coverage**: All three scenarios use fully-qualified names (`local/my-app`, `local/alpha`, `local/beta`). The issue's own reproduction steps use a **bare name** (`agents project create project1`) which relies on `parse_namespaced_name` defaulting to the `local/` namespace. Adding one scenario with a bare name (e.g., `project1` instead of `local/project1`) would more faithfully reproduce the reported bug and exercise the default-namespace resolution path end-to-end. This is minor since `parse_namespaced_name` has its own test suite, but worth noting for completeness.
@ -0,0 +62,4 @@
return NamespacedProjectRepository(session_factory=factory)
def _cleanup_persist_db(context: Any) -> None:
Member

F6 -- MEDIUM | Performance: create_engine() is called here but the engine is never dispose()-d. Each call opens a connection pool to the SQLite file. Over a test suite run with multiple scenarios, this accumulates unclosed connections and file handles. With SQLite specifically, this can cause file-lock contention when _cleanup_persist_db attempts to shutil.rmtree() the temp directory (dangling connections hold the file open).

Recommendation: Either store the engine on the context for disposal in _cleanup_persist_db, or use poolclass=NullPool (from sqlalchemy.pool) so connections are not pooled:

from sqlalchemy.pool import NullPool
engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool)
**F6 -- MEDIUM | Performance**: `create_engine()` is called here but the engine is never `dispose()`-d. Each call opens a connection pool to the SQLite file. Over a test suite run with multiple scenarios, this accumulates unclosed connections and file handles. With SQLite specifically, this can cause file-lock contention when `_cleanup_persist_db` attempts to `shutil.rmtree()` the temp directory (dangling connections hold the file open). **Recommendation:** Either store the engine on the context for disposal in `_cleanup_persist_db`, or use `poolclass=NullPool` (from `sqlalchemy.pool`) so connections are not pooled: ```python from sqlalchemy.pool import NullPool engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool) ```
@ -0,0 +102,4 @@
repo = _make_fresh_repo(context.persist_db_path)
with (
patch(_PATCH_PROJECT_REPO, return_value=repo),
patch(_PATCH_STORE_EXTRAS),
Member

F10 -- LOW | Convention: _store_project_extras is only called in the create command when --invariant or --invariant-actor flags are passed. Since the tests pass neither flag, this function is never invoked and the patch is unnecessary. While harmless, it adds noise about test intent and could silently mask future bugs if the create command's control flow is refactored to always call this function.

Recommendation: Remove the patch(_PATCH_STORE_EXTRAS) context manager, or add a comment explaining it's defensive.

**F10 -- LOW | Convention**: `_store_project_extras` is only called in the `create` command when `--invariant` or `--invariant-actor` flags are passed. Since the tests pass neither flag, this function is never invoked and the patch is unnecessary. While harmless, it adds noise about test intent and could silently mask future bugs if the `create` command's control flow is refactored to always call this function. **Recommendation:** Remove the `patch(_PATCH_STORE_EXTRAS)` context manager, or add a comment explaining it's defensive.
@ -0,0 +105,4 @@
patch(_PATCH_STORE_EXTRAS),
):
result = runner.invoke(project_app, ["create", name])
context.persist_create_results.append(result)
Member

F3 -- MEDIUM | Test Flaws: The result is appended to context.persist_create_results but no scenario step ever asserts result.exit_code == 0. If the CLI create fails (DI container error, import error), the test silently continues to the list step, which then fails with a confusing assertion like "Expected 'local/my-app' in project list output" rather than identifying the create failure.

Recommendation: Either add an inline assertion here:

assert result.exit_code == 0, (
    f"project create failed (exit {result.exit_code}): {result.output}"
)

or add a new Gherkin Then step like Then the persist creation should succeed that checks context.persist_create_results[-1].exit_code.

**F3 -- MEDIUM | Test Flaws**: The result is appended to `context.persist_create_results` but **no scenario step ever asserts `result.exit_code == 0`**. If the CLI create fails (DI container error, import error), the test silently continues to the list step, which then fails with a confusing assertion like _"Expected 'local/my-app' in project list output"_ rather than identifying the create failure. **Recommendation:** Either add an inline assertion here: ```python assert result.exit_code == 0, ( f"project create failed (exit {result.exit_code}): {result.output}" ) ``` or add a new Gherkin `Then` step like `Then the persist creation should succeed` that checks `context.persist_create_results[-1].exit_code`.
@ -0,0 +16,4 @@
[Documentation] After creating a project, listing projects should include it.
... Expected to FAIL while bug #589 is present.
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='persist_589_')
Run Process ${PYTHON} -m cleveragents init persist-test
Member

F2 -- HIGH | Test Flaws: agents init is not captured into a variable in either test case (here and line 37). Its exit code and stderr are never verified. If initialization fails, all downstream operations fail with misleading assertion errors instead of a clear "init failed" signal.

Recommendation: Change to ${init}= Run Process ... and add Should Be Equal As Integers ${init.rc} 0 msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr} in both test cases.

**F2 -- HIGH | Test Flaws**: `agents init` is not captured into a variable in either test case (here and line 37). Its exit code and stderr are never verified. If initialization fails, all downstream operations fail with misleading assertion errors instead of a clear "init failed" signal. **Recommendation:** Change to `${init}= Run Process ...` and add `Should Be Equal As Integers ${init.rc} 0 msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr}` in both test cases.
@ -0,0 +34,4 @@
[Documentation] Creating two projects should result in both appearing in list.
... Expected to FAIL while bug #589 is present.
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='persist_589_multi_')
Run Process ${PYTHON} -m cleveragents init persist-multi
Member

F1 -- HIGH | Test Flaws: This entire second test case fires three commands (init, create local/first, create local/second) without capturing their process result or checking exit codes. Compare to the first test case (lines 19-24) which properly captures ${create} and asserts ${create.rc} == 0.

If agents init fails (permissions, disk, migration error), both creates silently fail, and the final Should Contain on the list output produces a misleading failure about missing project names instead of surfacing the real root cause.

Recommendation: Capture all three process results and add Should Be Equal As Integers ${result.rc} 0 after each, mirroring the pattern already used in the first test case.

**F1 -- HIGH | Test Flaws**: This entire second test case fires three commands (`init`, `create local/first`, `create local/second`) without capturing their process result or checking exit codes. Compare to the first test case (lines 19-24) which properly captures `${create}` and asserts `${create.rc} == 0`. If `agents init` fails (permissions, disk, migration error), both creates silently fail, and the final `Should Contain` on the list output produces a misleading failure about missing project names instead of surfacing the real root cause. **Recommendation:** Capture all three process results and add `Should Be Equal As Integers ${result.rc} 0` after each, mirroring the pattern already used in the first test case.
@ -0,0 +42,4 @@
... timeout=60s cwd=${tmpdir}
${list}= Run Process ${PYTHON} -m cleveragents project list
... timeout=60s cwd=${tmpdir}
Should Contain ${list.stdout} local/first
Member

F7 -- LOW | Test Flaws: Unlike the first test (line 27) which asserts Should Be Equal As Integers ${list.rc} 0, this second test jumps directly to Should Contain on stdout. If project list exits non-zero, ${list.stdout} could be empty while the actual error is in ${list.stderr}.

Recommendation: Add Should Be Equal As Integers ${list.rc} 0 before the Should Contain assertions, consistent with the first test case.

**F7 -- LOW | Test Flaws**: Unlike the first test (line 27) which asserts `Should Be Equal As Integers ${list.rc} 0`, this second test jumps directly to `Should Contain` on stdout. If `project list` exits non-zero, `${list.stdout}` could be empty while the actual error is in `${list.stderr}`. **Recommendation:** Add `Should Be Equal As Integers ${list.rc} 0` before the `Should Contain` assertions, consistent with the first test case.
Author
Member

All 10 findings from review #1976 have been addressed in commit 35634253:

Finding Severity Resolution
F1 HIGH Robot test 2: all process results captured with ${create_first}=/${create_second}=, exit codes asserted
F2 HIGH Robot tests 1+2: agents init captured as ${init}=, ${init.rc} checked == 0
F3 MEDIUM Steps: assert result.exit_code == 0 added after runner.invoke(project_app, ["create", name])
F4 MEDIUM Benchmark: imports rewritten to _SRC/importlib.reload(cleveragents) convention
F5 MEDIUM Benchmark: track_list_after_create_count now uses self._counter for unique names
F6 MEDIUM Steps + Benchmark: all create_engine() calls now use poolclass=NullPool
F7 LOW Robot test 2: ${list.rc} checked == 0 before Should Contain
F8 LOW Commit uses test(project): prefix
F9 LOW Feature: added "Bare project name uses default namespace and persists" scenario with "my-app"
F10 LOW Steps: added defensive comment explaining why _PATCH_STORE_EXTRAS is patched

Note: During rebase, I noticed the bug-fix author's commits (fix(project): persist project to database during creation) already landed on this branch and removed the @wip tags. The new bare-name scenario follows the same convention (no @wip).

Pyright typecheck and ruff lint both pass.

All 10 findings from review #1976 have been addressed in commit `35634253`: | Finding | Severity | Resolution | |---------|----------|------------| | **F1** | HIGH | Robot test 2: all process results captured with `${create_first}=`/`${create_second}=`, exit codes asserted | | **F2** | HIGH | Robot tests 1+2: `agents init` captured as `${init}=`, `${init.rc}` checked == 0 | | **F3** | MEDIUM | Steps: `assert result.exit_code == 0` added after `runner.invoke(project_app, ["create", name])` | | **F4** | MEDIUM | Benchmark: imports rewritten to `_SRC`/`importlib.reload(cleveragents)` convention | | **F5** | MEDIUM | Benchmark: `track_list_after_create_count` now uses `self._counter` for unique names | | **F6** | MEDIUM | Steps + Benchmark: all `create_engine()` calls now use `poolclass=NullPool` | | **F7** | LOW | Robot test 2: `${list.rc}` checked == 0 before `Should Contain` | | **F8** | LOW | Commit uses `test(project):` prefix | | **F9** | LOW | Feature: added "Bare project name uses default namespace and persists" scenario with `"my-app"` | | **F10** | LOW | Steps: added defensive comment explaining why `_PATCH_STORE_EXTRAS` is patched | Note: During rebase, I noticed the bug-fix author's commits (`fix(project): persist project to database during creation`) already landed on this branch and removed the `@wip` tags. The new bare-name scenario follows the same convention (no `@wip`). Pyright typecheck and ruff lint both pass.
hamza.khyari approved these changes 2026-03-05 22:09:27 +00:00
Dismissed
hamza.khyari left a comment

Code Review -- PR #591 (Issue #589)

Scope: 3 non-merge commits on feature/m3-fix-project-create-persist. 1-line production fix + 384 lines of tests/benchmarks.


Summary Table

ID Severity Category Description
BUG-1 Major BUG NamespacedProjectRepository.delete() and update() have the same flush-without-commit bug
BUG-2 Major BUG ProjectResourceLinkRepository.create_link() and .remove_link() same bug -- called directly from project CLI without UoW
BUG-3 High BUG @database_retry retries IntegrityError 3 times (permanent error) -- duplicate project creation wastes ~1.5s before failing
BUG-4 High BUG Session never closed -- create() opens a session, commits, returns, but no finally: session.close(). All 42 session-factory repo methods leak sessions.
TEST-1 Minor TEST Scenario 4 (duplicate) doesn't assert exit_code != 0 -- only checks output text
TEST-2 Minor TEST step_list_projects_persist doesn't assert exit_code == 0 -- list failure surfaces as confusing "name not in output"
DOC-1 Minor DOC CHANGELOG says "3 Behave BDD scenarios (with @wip tag)" but there are 4 scenarios and no @wip tags

BUG-1: delete() and update() in Same Class Have the Same Bug

Severity: Major
File: repositories.py:2915 (update), repositories.py:2964 (delete)

delete() is called directly from project.py:916 without UoW. Same flush() without commit() pattern. agents project delete will appear to succeed but the deletion won't persist.

Recommendation: Follow-up issue to add session.commit() to both methods.


BUG-2: ProjectResourceLinkRepository Same Pattern

Severity: Major
File: repositories.py:3018 (create_link), repositories.py:3120 (remove_link)

Called directly from project.py:564 and project.py:732. Both flush without commit. agents project create --resource <name> will appear to link the resource but the link won't persist.

Recommendation: Follow-up issue.


BUG-3: Retry Fires on IntegrityError (Permanent Error)

Severity: High
File: repositories.py:2847-2853 + core/retry_patterns.py:122-126

IntegrityError (UNIQUE constraint) is caught, wrapped as DatabaseError, and re-raised. @database_retry retries on DatabaseError -- including permanent errors that will never succeed:

attempt 1: flush -> IntegrityError -> rollback -> DatabaseError -> retry (wait 0.5s)
attempt 2: flush -> IntegrityError -> rollback -> DatabaseError -> retry (wait 0.5s)
attempt 3: flush -> IntegrityError -> rollback -> DatabaseError -> raise
Total: ~1.5s for what should be instant

Confirmed by instrumenting the duplicate creation path.

Recommendation: Pre-existing pattern, not introduced by this PR. Follow-up issue to either (a) not wrap IntegrityError as DatabaseError, or (b) add IntegrityError to a no-retry exception list.


BUG-4: Sessions Never Closed in Session-Factory Repositories

Severity: High
File: repositories.py:2840-2856

The method calls self._session() to create a session but has no finally: session.close(). On the success path, the session is committed but left open. On the error path, the session is rolled back but left open. Instrumented 3 operations (2 creates + 1 list): 3 sessions created, 0 closed.

This applies to all 42 mutating methods across 14 session-factory repositories. With NullPool (tests) it's benign. With real connection pooling the pool will exhaust under load.

Compare with ResourceRegistryService methods which all have finally: session.close().

Recommendation: Pre-existing, not introduced by this PR. Follow-up issue to add finally: session.close() to all session-factory repo methods.


TEST-1: Duplicate Scenario Doesn't Assert Exit Code

Severity: Minor
File: features/steps/project_create_persist_steps.py:163-174

step_persist_dup_fails only checks output text. Doesn't assert result.exit_code != 0. A command that accidentally succeeds but logs "already exists" would pass.

Recommendation: Add assert result.exit_code != 0.


TEST-2: List Step Doesn't Assert Exit Code

Severity: Minor
File: features/steps/project_create_persist_steps.py:148-154

step_list_projects_persist stores the result but never checks exit_code == 0. If list fails, the error surfaces as the confusing assertion "Expected 'local/my-app' in project list output but got: [error text]".

Recommendation: Add assert result.exit_code == 0 to the list step.


DOC-1: CHANGELOG Is Stale

Severity: Minor
File: CHANGELOG.md:35

Says "3 Behave BDD scenarios (with @wip tag)". After the review fix commit there are 4 scenarios and no @wip tags.


Verdict: REQUEST_CHANGES. The 1-line fix is correct and solves #589. BUG-1 through BUG-4 are pre-existing systemic issues that warrant follow-up issues but shouldn't block this targeted fix. TEST-1/2 and DOC-1 are minor.

# Code Review -- PR #591 (Issue #589) **Scope**: 3 non-merge commits on `feature/m3-fix-project-create-persist`. 1-line production fix + 384 lines of tests/benchmarks. --- ## Summary Table | ID | Severity | Category | Description | |----|----------|----------|-------------| | BUG-1 | **Major** | BUG | `NamespacedProjectRepository.delete()` and `update()` have the same flush-without-commit bug | | BUG-2 | **Major** | BUG | `ProjectResourceLinkRepository.create_link()` and `.remove_link()` same bug -- called directly from project CLI without UoW | | BUG-3 | **High** | BUG | `@database_retry` retries `IntegrityError` 3 times (permanent error) -- duplicate project creation wastes ~1.5s before failing | | BUG-4 | **High** | BUG | Session never closed -- `create()` opens a session, commits, returns, but no `finally: session.close()`. All 42 session-factory repo methods leak sessions. | | TEST-1 | Minor | TEST | Scenario 4 (duplicate) doesn't assert `exit_code != 0` -- only checks output text | | TEST-2 | Minor | TEST | `step_list_projects_persist` doesn't assert `exit_code == 0` -- list failure surfaces as confusing "name not in output" | | DOC-1 | Minor | DOC | CHANGELOG says "3 Behave BDD scenarios (with `@wip` tag)" but there are 4 scenarios and no `@wip` tags | --- ## BUG-1: `delete()` and `update()` in Same Class Have the Same Bug **Severity**: Major **File**: `repositories.py:2915` (update), `repositories.py:2964` (delete) `delete()` is called directly from `project.py:916` without UoW. Same `flush()` without `commit()` pattern. `agents project delete` will appear to succeed but the deletion won't persist. **Recommendation**: Follow-up issue to add `session.commit()` to both methods. --- ## BUG-2: `ProjectResourceLinkRepository` Same Pattern **Severity**: Major **File**: `repositories.py:3018` (create_link), `repositories.py:3120` (remove_link) Called directly from `project.py:564` and `project.py:732`. Both flush without commit. `agents project create --resource <name>` will appear to link the resource but the link won't persist. **Recommendation**: Follow-up issue. --- ## BUG-3: Retry Fires on IntegrityError (Permanent Error) **Severity**: High **File**: `repositories.py:2847-2853` + `core/retry_patterns.py:122-126` `IntegrityError` (UNIQUE constraint) is caught, wrapped as `DatabaseError`, and re-raised. `@database_retry` retries on `DatabaseError` -- including permanent errors that will never succeed: ``` attempt 1: flush -> IntegrityError -> rollback -> DatabaseError -> retry (wait 0.5s) attempt 2: flush -> IntegrityError -> rollback -> DatabaseError -> retry (wait 0.5s) attempt 3: flush -> IntegrityError -> rollback -> DatabaseError -> raise Total: ~1.5s for what should be instant ``` Confirmed by instrumenting the duplicate creation path. **Recommendation**: Pre-existing pattern, not introduced by this PR. Follow-up issue to either (a) not wrap IntegrityError as DatabaseError, or (b) add IntegrityError to a no-retry exception list. --- ## BUG-4: Sessions Never Closed in Session-Factory Repositories **Severity**: High **File**: `repositories.py:2840-2856` The method calls `self._session()` to create a session but has no `finally: session.close()`. On the success path, the session is committed but left open. On the error path, the session is rolled back but left open. Instrumented 3 operations (2 creates + 1 list): 3 sessions created, 0 closed. This applies to all 42 mutating methods across 14 session-factory repositories. With `NullPool` (tests) it's benign. With real connection pooling the pool will exhaust under load. Compare with `ResourceRegistryService` methods which all have `finally: session.close()`. **Recommendation**: Pre-existing, not introduced by this PR. Follow-up issue to add `finally: session.close()` to all session-factory repo methods. --- ## TEST-1: Duplicate Scenario Doesn't Assert Exit Code **Severity**: Minor **File**: `features/steps/project_create_persist_steps.py:163-174` `step_persist_dup_fails` only checks output text. Doesn't assert `result.exit_code != 0`. A command that accidentally succeeds but logs "already exists" would pass. **Recommendation**: Add `assert result.exit_code != 0`. --- ## TEST-2: List Step Doesn't Assert Exit Code **Severity**: Minor **File**: `features/steps/project_create_persist_steps.py:148-154` `step_list_projects_persist` stores the result but never checks `exit_code == 0`. If list fails, the error surfaces as the confusing assertion "Expected 'local/my-app' in project list output but got: [error text]". **Recommendation**: Add `assert result.exit_code == 0` to the list step. --- ## DOC-1: CHANGELOG Is Stale **Severity**: Minor **File**: `CHANGELOG.md:35` Says "3 Behave BDD scenarios (with `@wip` tag)". After the review fix commit there are 4 scenarios and no `@wip` tags. --- **Verdict**: REQUEST_CHANGES. The 1-line fix is correct and solves #589. BUG-1 through BUG-4 are pre-existing systemic issues that warrant follow-up issues but shouldn't block this targeted fix. TEST-1/2 and DOC-1 are minor.
hamza.khyari left a comment

update

update
Author
Member

Disposition — Review #1993 / #1994 (hamza.khyari)

All actionable findings from Hamza's review have been addressed. Quality gates pass: nox -s lint , nox -s typecheck , nox -s unit_tests (4 scenarios, 0 failures) .


Fixes Pushed

ID Finding Commit Resolution
TEST-1 Duplicate scenario doesn't assert exit_code != 0 b1682999 Added assert result.exit_code != 0 in step_persist_dup_fails
TEST-2 List step doesn't assert exit_code == 0 b1682999 Added assert result.exit_code == 0 in step_list_projects_persist
DOC-1 CHANGELOG says "3 scenarios (with @wip tag)" 063c9318 Updated to "4 Behave BDD scenarios (tagged @tdd @bug589)"

Acknowledged — Pre-existing Systemic Issues (Not Introduced by This PR)

ID Finding Disposition
BUG-1 delete() and update() flush without commit Pre-existing. Already fixed in PR #593 (c9fe3c54) which adds session.commit() to both methods.
BUG-2 ProjectResourceLinkRepository same pattern Pre-existing. Follow-up issue warranted — not in scope of this TDD test PR.
BUG-3 @database_retry retries permanent IntegrityError Pre-existing retry pattern. Follow-up issue to add IntegrityError to no-retry list.
BUG-4 Sessions never closed in session-factory repos Pre-existing across all 42 mutating methods. Follow-up issue to add finally: session.close().

Hamza correctly identified these as pre-existing and not blocking for this PR. BUG-1 is already resolved in the companion PR #593.


Ready for re-review. @hamza.khyari

## Disposition — Review #1993 / #1994 (hamza.khyari) All actionable findings from Hamza's review have been addressed. Quality gates pass: `nox -s lint` ✅, `nox -s typecheck` ✅, `nox -s unit_tests` (4 scenarios, 0 failures) ✅. --- ### Fixes Pushed | ID | Finding | Commit | Resolution | |:---|:--------|:-------|:-----------| | **TEST-1** | Duplicate scenario doesn't assert `exit_code != 0` | `b1682999` | Added `assert result.exit_code != 0` in `step_persist_dup_fails` | | **TEST-2** | List step doesn't assert `exit_code == 0` | `b1682999` | Added `assert result.exit_code == 0` in `step_list_projects_persist` | | **DOC-1** | CHANGELOG says "3 scenarios (with `@wip` tag)" | `063c9318` | Updated to "4 Behave BDD scenarios (tagged `@tdd @bug589`)" | --- ### Acknowledged — Pre-existing Systemic Issues (Not Introduced by This PR) | ID | Finding | Disposition | |:---|:--------|:-----------| | **BUG-1** | `delete()` and `update()` flush without commit | Pre-existing. Already fixed in PR #593 (`c9fe3c54`) which adds `session.commit()` to both methods. | | **BUG-2** | `ProjectResourceLinkRepository` same pattern | Pre-existing. Follow-up issue warranted — not in scope of this TDD test PR. | | **BUG-3** | `@database_retry` retries permanent `IntegrityError` | Pre-existing retry pattern. Follow-up issue to add IntegrityError to no-retry list. | | **BUG-4** | Sessions never closed in session-factory repos | Pre-existing across all 42 mutating methods. Follow-up issue to add `finally: session.close()`. | Hamza correctly identified these as pre-existing and not blocking for this PR. BUG-1 is already resolved in the companion PR #593. --- Ready for re-review. @hamza.khyari
hurui200320 requested changes 2026-03-06 07:27:46 +00:00
Dismissed
hurui200320 left a comment

Code Review -- PR #591 (Issue #589)

Reviewer: @hurui200320
Reviewed against: Issue #589 acceptance criteria, docs/specification.md, CONTRIBUTING.md, and codebase conventions.

Verdict: REQUEST CHANGES

The 1-line production fix (session.commit()) is correct and resolves the bug. The test design is sound -- using file-based SQLite with separate sessions per invocation properly reproduces real CLI behavior. However, the branch has serious structural/process issues that violate the project's commit quality standards per CONTRIBUTING.md.


Findings

CRITICAL (Must Fix Before Merge)

ID Location Description
C1 Branch history Merge commit in branch. Commit 142424b9 is Merge branch 'master' into feature/m3-fix-project-create-persist. Per project rules, branches must align with master via rebase, strictly avoiding merge commits. The branch needs to be rebased onto master.
C2 Branch history Multiple fixup commits violate the "one commit per issue" rule. The branch has 6 non-merge commits for issue #589. Commits b1682999 (exit_code assertions), cce381b9 (CHANGELOG fix), and 063c9318 (CHANGELOG fix retry) are clearly fixup commits patching earlier work. Per CONTRIBUTING.md: "Every commit must completely implement an issue and close it. At no point should a branch contain multiple commits addressing the same issue, nor should it have commits that fix earlier commits present in the same branch." These should be squashed into clean, independently-valid commits via interactive rebase.
C3 CHANGELOG.md (commit cce381b9) Destructive intermediate commit. Commit cce381b9 accidentally deleted the entire CHANGELOG.md (328 lines -> 1 line), then 063c9318 restored it. This intermediate broken state violates "Each commit must build and pass all tests" and "the repository should be in a working state at every commit."

HIGH

ID Location Description
H1 PR metadata Type label mismatch. PR carries Type/Testing but the issue is Type/Bug and the PR includes the actual bug fix (session.commit()). Per CONTRIBUTING.md: "Every PR must carry exactly one Type/ label that matches the nature of the change." This should be Type/Bug.
H2 PR status PR is not mergeable. The PR shows mergeable: false -- conflicts with master need resolution via rebase (not merge).
H3 repositories.py:2840-2856 Session never closed after commit. The fix adds session.commit() at line 2845, but the session created at line 2840 is never closed on any code path. The success path commits but leaves the session open; error paths rollback but don't close. While this is a pre-existing pattern across the class, adding finally: session.close() to the method being modified would be a minimal, low-risk improvement. Recommendation: Wrap the try block in a try/finally that calls session.close().

MEDIUM

ID Location Description
M1 repositories.py:2844 Redundant flush() before commit(). session.flush() on line 2844 is now unnecessary because session.commit() on line 2845 implicitly flushes. Not a bug, but it makes the intent unclear and could mislead future developers into thinking flush is doing something extra. Recommendation: Remove the flush() call.
M2 features/project_create_persist.feature:1 Stale TDD comment. File comment says "These tests target bug #589 and are expected to fail until the fix is applied." The fix IS included in this PR, so this comment is misleading. Recommendation: Remove or update the comment.
M3 benchmarks/project_create_persist_bench.py:96-109 track_list_after_create_count returns cumulative count. Each call creates a new project, but list_projects() returns ALL projects in the database. Since setup() only runs once per ASV session, the return value increases monotonically (1, 2, 3, ...) instead of consistently returning 1. This makes the tracking metric unreliable as an indicator of "persistence works." Recommendation: Filter the list result to just the created project, or use a unique DB per call.
M4 CHANGELOG.md:36-41 CHANGELOG entry describes tests, not the fix. The entry says "Added TDD failing tests for..." but this PR also includes the actual bug fix. The CHANGELOG should describe the fix from the user's perspective (e.g., "Fixed agents project create not persisting projects to the database").

LOW

ID Location Description
L1 Issue #589 body Issue subtasks not checked off. All subtasks in the issue body remain unchecked (- [ ]). Per project guidelines, subtasks should be checked off as they are completed.
L2 PR description PR description is sparse. While it has the required closing keyword, the description is minimal. CONTRIBUTING.md requires "a clear, descriptive body that explains the purpose of the change, summarizes what was done and why, and provides enough context for a reviewer."

Acceptance Criteria Coverage

Criterion Behave Robot Status
project create persists to DB Scenario 1 Test 1 Covered
project list shows created project Scenario 1 Test 1 Covered
Multiple projects created and listed Scenario 2 Test 2 Covered
Duplicate name produces clear error Scenario 4 -- Covered (Behave only)

What's Good

  • The 1-line fix (session.commit()) is the correct, minimal solution.
  • Test design with file-based SQLite + fresh session per invocation accurately reproduces the real cross-process persistence bug.
  • Step definitions use unique prefixes to avoid AmbiguousStep collisions.
  • Cleanup is handled properly via context.add_cleanup() and [Teardown] in Robot.
  • NullPool usage prevents SQLite file handle leaks across test scenarios.
  • All previous review findings (Luis/CoreRasurae #1976 and Hamza #1993/#1994) have been addressed.

Bottom line: The code fix is correct. The blocking issues are all process/hygiene: the branch needs an interactive rebase to eliminate the merge commit and squash fixup commits into clean, independently-valid commits. The PR also needs its Type label corrected and conflicts resolved.

## Code Review -- PR #591 (Issue #589) **Reviewer:** @hurui200320 **Reviewed against:** Issue #589 acceptance criteria, `docs/specification.md`, `CONTRIBUTING.md`, and codebase conventions. **Verdict: REQUEST CHANGES** The 1-line production fix (`session.commit()`) is correct and resolves the bug. The test design is sound -- using file-based SQLite with separate sessions per invocation properly reproduces real CLI behavior. However, the branch has serious structural/process issues that violate the project's commit quality standards per CONTRIBUTING.md. --- ## Findings ### CRITICAL (Must Fix Before Merge) | ID | Location | Description | |----|----------|-------------| | **C1** | Branch history | **Merge commit in branch.** Commit `142424b9` is `Merge branch 'master' into feature/m3-fix-project-create-persist`. Per project rules, branches must align with master via rebase, strictly avoiding merge commits. The branch needs to be rebased onto master. | | **C2** | Branch history | **Multiple fixup commits violate the "one commit per issue" rule.** The branch has 6 non-merge commits for issue #589. Commits `b1682999` (exit_code assertions), `cce381b9` (CHANGELOG fix), and `063c9318` (CHANGELOG fix retry) are clearly fixup commits patching earlier work. Per CONTRIBUTING.md: "Every commit must completely implement an issue and close it. At no point should a branch contain multiple commits addressing the same issue, nor should it have commits that fix earlier commits present in the same branch." These should be squashed into clean, independently-valid commits via interactive rebase. | | **C3** | `CHANGELOG.md` (commit `cce381b9`) | **Destructive intermediate commit.** Commit `cce381b9` accidentally deleted the entire CHANGELOG.md (328 lines -> 1 line), then `063c9318` restored it. This intermediate broken state violates "Each commit must build and pass all tests" and "the repository should be in a working state at every commit." | ### HIGH | ID | Location | Description | |----|----------|-------------| | **H1** | PR metadata | **Type label mismatch.** PR carries `Type/Testing` but the issue is `Type/Bug` and the PR includes the actual bug fix (`session.commit()`). Per CONTRIBUTING.md: "Every PR must carry exactly one Type/ label that matches the nature of the change." This should be `Type/Bug`. | | **H2** | PR status | **PR is not mergeable.** The PR shows `mergeable: false` -- conflicts with master need resolution via rebase (not merge). | | **H3** | `repositories.py:2840-2856` | **Session never closed after commit.** The fix adds `session.commit()` at line 2845, but the session created at line 2840 is never closed on any code path. The success path commits but leaves the session open; error paths rollback but don't close. While this is a pre-existing pattern across the class, adding `finally: session.close()` to the method being modified would be a minimal, low-risk improvement. **Recommendation:** Wrap the try block in a `try/finally` that calls `session.close()`. | ### MEDIUM | ID | Location | Description | |----|----------|-------------| | **M1** | `repositories.py:2844` | **Redundant `flush()` before `commit()`.** `session.flush()` on line 2844 is now unnecessary because `session.commit()` on line 2845 implicitly flushes. Not a bug, but it makes the intent unclear and could mislead future developers into thinking flush is doing something extra. **Recommendation:** Remove the `flush()` call. | | **M2** | `features/project_create_persist.feature:1` | **Stale TDD comment.** File comment says "These tests target bug #589 and are expected to fail until the fix is applied." The fix IS included in this PR, so this comment is misleading. **Recommendation:** Remove or update the comment. | | **M3** | `benchmarks/project_create_persist_bench.py:96-109` | **`track_list_after_create_count` returns cumulative count.** Each call creates a new project, but `list_projects()` returns ALL projects in the database. Since `setup()` only runs once per ASV session, the return value increases monotonically (1, 2, 3, ...) instead of consistently returning 1. This makes the tracking metric unreliable as an indicator of "persistence works." **Recommendation:** Filter the list result to just the created project, or use a unique DB per call. | | **M4** | `CHANGELOG.md:36-41` | **CHANGELOG entry describes tests, not the fix.** The entry says "Added TDD failing tests for..." but this PR also includes the actual bug fix. The CHANGELOG should describe the fix from the user's perspective (e.g., "Fixed `agents project create` not persisting projects to the database"). | ### LOW | ID | Location | Description | |----|----------|-------------| | **L1** | Issue #589 body | **Issue subtasks not checked off.** All subtasks in the issue body remain unchecked (`- [ ]`). Per project guidelines, subtasks should be checked off as they are completed. | | **L2** | PR description | **PR description is sparse.** While it has the required closing keyword, the description is minimal. CONTRIBUTING.md requires "a clear, descriptive body that explains the purpose of the change, summarizes what was done and why, and provides enough context for a reviewer." | --- ## Acceptance Criteria Coverage | Criterion | Behave | Robot | Status | |-----------|--------|-------|--------| | `project create` persists to DB | Scenario 1 | Test 1 | Covered | | `project list` shows created project | Scenario 1 | Test 1 | Covered | | Multiple projects created and listed | Scenario 2 | Test 2 | Covered | | Duplicate name produces clear error | Scenario 4 | -- | Covered (Behave only) | --- ## What's Good - The 1-line fix (`session.commit()`) is the correct, minimal solution. - Test design with file-based SQLite + fresh session per invocation accurately reproduces the real cross-process persistence bug. - Step definitions use unique prefixes to avoid `AmbiguousStep` collisions. - Cleanup is handled properly via `context.add_cleanup()` and `[Teardown]` in Robot. - NullPool usage prevents SQLite file handle leaks across test scenarios. - All previous review findings (Luis/CoreRasurae #1976 and Hamza #1993/#1994) have been addressed. --- **Bottom line:** The code fix is correct. The blocking issues are all process/hygiene: the branch needs an interactive rebase to eliminate the merge commit and squash fixup commits into clean, independently-valid commits. The PR also needs its Type label corrected and conflicts resolved.
Member

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

Latest Commit Reviewed: 063c93181d
Author: Brent E. Edwards (brent.edwards@cleverthis.com)
Message: docs(changelog): fix scenario count and tag reference (content fix)
Issue: #589 — agents project doesn't show created projects
Branch: feature/m3-fix-project-create-persist
Reviewer: Aditya

Scope of Changes

File Type Lines
CHANGELOG.md Modified +6
benchmarks/project_create_persist_bench.py New +111
features/project_create_persist.feature New +34
features/steps/project_create_persist_steps.py New +182
robot/project_create_persist.robot New +59
src/cleveragents/infrastructure/database/repositories.py Modified +1

The production fix is a correct, minimal one-liner: adding session.commit() after session.flush() in NamespacedProjectRepository.create(). The test scaffolding using file-based SQLite with a fresh session factory per CLI invocation is the right design for faithfully reproducing this cross-session visibility bug.


Findings

F1. [MEDIUM] Robot tests create per-test tmpdir despite Suite Setup already provisioning one

Both Robot test cases call Evaluate __import__('tempfile').mkdtemp(...) inline:

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

yet the suite already declares:

Suite Setup       Setup Test Environment
Suite Teardown    Cleanup Test Environment

This means two separate temp directories are live simultaneously per test run — one from common.resource's suite setup and one per test case. Every other Robot file in the repo uses the suite-level environment exclusively and calls agents init within it. If the intent is an isolated fresh environment per test case (which is valid here), the suite-level Setup Test Environment / Cleanup Test Environment should be removed from this file so the lifecycle is clear and unambiguous.

Recommendation: Remove Suite Setup / Suite Teardown from this file and rely solely on the per-test tmpdir + [Teardown] Remove Directory, consistent with the isolation intent. Or conversely, use the suite-level environment and drop the per-test mkdtemp(), matching the repo-wide pattern.


F2. [MEDIUM] Scenario 3 bare-name assertion is a loose substring match — namespace resolution unverified

Scenario: Bare project name uses default namespace and persists
    When I create a project named "my-app" via the persist CLI
    And I list projects via the persist CLI
    Then the persist project list should contain "my-app"

The assertion "my-app" is a substring of "local/my-app". The step implementation uses a plain assert name in output check, which will pass whether the output contains "local/my-app" or literally just "my-app". The intent of this scenario — added to address CoreRasurae's F9 — is specifically to verify that parse_namespaced_name resolves the bare name to the local/ namespace. That resolution is never actually asserted. A project stored incorrectly as "my-app" without a namespace would also pass this test.

Recommendation: Change the assertion to the fully-qualified name:

Then the persist project list should contain "local/my-app"

This directly verifies the namespace resolution that the scenario is designed to test.


F3. [LOW] _make_fresh_repo calls Base.metadata.create_all() redundantly on every step invocation

step_init_persist_db (the Background step) already creates the full schema once:

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

However, _make_fresh_repo — called on every create and list step — also calls:

engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool)
Base.metadata.create_all(engine)  # redundant after Background step

SQLAlchemy's create_all with the default checkfirst=True only creates missing tables, so this is functionally harmless. But it adds an unnecessary schema introspection round-trip to every step invocation. The same redundancy exists in benchmarks/project_create_persist_bench.py where setup() creates the schema and _fresh_repo() recreates it on every benchmark call.

Recommendation: Remove the Base.metadata.create_all(engine) call from _make_fresh_repo since the schema is guaranteed to exist after the Background step. The benchmark's setup() already handles schema creation independently, so _fresh_repo there can equally be simplified.

Summary Table

ID Severity Category Description
F1 MEDIUM Standards Robot tests create per-test tmpdir alongside Suite Setup from common.resource; ambiguous lifecycle
F2 MEDIUM Test Flaw Scenario 3 asserts bare "my-app" instead of "local/my-app"; namespace resolution never verified
F3 LOW Performance _make_fresh_repo calls Base.metadata.create_all() on every step/benchmark invocation despite schema being pre-created
# Code Review Report: Brent's Commits on feature/m3-fix-project-create-persist **Latest Commit Reviewed:** 063c93181d **Author:** Brent E. Edwards (brent.edwards@cleverthis.com) **Message:** docs(changelog): fix scenario count and tag reference (content fix) **Issue:** #589 — agents project doesn't show created projects **Branch:** feature/m3-fix-project-create-persist **Reviewer:** Aditya ## Scope of Changes | File | Type | Lines | |------|------|-------| | `CHANGELOG.md` | Modified | +6 | | `benchmarks/project_create_persist_bench.py` | New | +111 | | `features/project_create_persist.feature` | New | +34 | | `features/steps/project_create_persist_steps.py` | New | +182 | | `robot/project_create_persist.robot` | New | +59 | | `src/cleveragents/infrastructure/database/repositories.py` | Modified | +1 | The production fix is a correct, minimal one-liner: adding `session.commit()` after `session.flush()` in `NamespacedProjectRepository.create()`. The test scaffolding using file-based SQLite with a fresh session factory per CLI invocation is the right design for faithfully reproducing this cross-session visibility bug. --- ## Findings ### F1. [MEDIUM] Robot tests create per-test `tmpdir` despite Suite Setup already provisioning one Both Robot test cases call `Evaluate __import__('tempfile').mkdtemp(...)` inline: ```robotframework ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='persist_589_') ``` yet the suite already declares: ```robotframework Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment ``` This means two separate temp directories are live simultaneously per test run — one from `common.resource`'s suite setup and one per test case. Every other Robot file in the repo uses the suite-level environment exclusively and calls `agents init` within it. If the intent is an isolated fresh environment per test case (which is valid here), the suite-level `Setup Test Environment` / `Cleanup Test Environment` should be removed from this file so the lifecycle is clear and unambiguous. **Recommendation:** Remove `Suite Setup` / `Suite Teardown` from this file and rely solely on the per-test `tmpdir` + `[Teardown] Remove Directory`, consistent with the isolation intent. Or conversely, use the suite-level environment and drop the per-test `mkdtemp()`, matching the repo-wide pattern. --- ### F2. [MEDIUM] Scenario 3 bare-name assertion is a loose substring match — namespace resolution unverified ```gherkin Scenario: Bare project name uses default namespace and persists When I create a project named "my-app" via the persist CLI And I list projects via the persist CLI Then the persist project list should contain "my-app" ``` The assertion `"my-app"` is a substring of `"local/my-app"`. The step implementation uses a plain `assert name in output` check, which will pass whether the output contains `"local/my-app"` or literally just `"my-app"`. The intent of this scenario — added to address CoreRasurae's F9 — is specifically to verify that `parse_namespaced_name` resolves the bare name to the `local/` namespace. That resolution is never actually asserted. A project stored incorrectly as `"my-app"` without a namespace would also pass this test. **Recommendation:** Change the assertion to the fully-qualified name: ```gherkin Then the persist project list should contain "local/my-app" ``` This directly verifies the namespace resolution that the scenario is designed to test. --- ### F3. [LOW] `_make_fresh_repo` calls `Base.metadata.create_all()` redundantly on every step invocation `step_init_persist_db` (the Background step) already creates the full schema once: ```python engine = create_engine(f"sqlite:///{context.persist_db_path}", echo=False, poolclass=NullPool) Base.metadata.create_all(engine) engine.dispose() ``` However, `_make_fresh_repo` — called on every `create` and `list` step — also calls: ```python engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool) Base.metadata.create_all(engine) # redundant after Background step ``` SQLAlchemy's `create_all` with the default `checkfirst=True` only creates missing tables, so this is functionally harmless. But it adds an unnecessary schema introspection round-trip to every step invocation. The same redundancy exists in `benchmarks/project_create_persist_bench.py` where `setup()` creates the schema and `_fresh_repo()` recreates it on every benchmark call. **Recommendation:** Remove the `Base.metadata.create_all(engine)` call from `_make_fresh_repo` since the schema is guaranteed to exist after the Background step. The benchmark's `setup()` already handles schema creation independently, so `_fresh_repo` there can equally be simplified. ## Summary Table | ID | Severity | Category | Description | |----|----------|----------|-------------| | F1 | MEDIUM | Standards | Robot tests create per-test `tmpdir` alongside Suite Setup from `common.resource`; ambiguous lifecycle | | F2 | MEDIUM | Test Flaw | Scenario 3 asserts bare `"my-app"` instead of `"local/my-app"`; namespace resolution never verified | | F3 | LOW | Performance | `_make_fresh_repo` calls `Base.metadata.create_all()` on every step/benchmark invocation despite schema being pre-created |
brent.edwards force-pushed feature/m3-fix-project-create-persist from 063c93181d
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 16s
CI / lint (pull_request) Successful in 19s
CI / quality (pull_request) Successful in 23s
CI / security (pull_request) Successful in 36s
CI / typecheck (pull_request) Successful in 50s
CI / unit_tests (pull_request) Successful in 2m15s
CI / docker (pull_request) Successful in 9s
CI / integration_tests (pull_request) Successful in 3m7s
CI / coverage (pull_request) Successful in 4m31s
CI / benchmark-regression (pull_request) Successful in 29m1s
to d04884c715
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / build (pull_request) Successful in 17s
CI / quality (pull_request) Successful in 22s
CI / security (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 35s
CI / unit_tests (pull_request) Failing after 2m18s
CI / docker (pull_request) Has been skipped
CI / integration_tests (pull_request) Successful in 3m6s
CI / coverage (pull_request) Successful in 4m28s
CI / benchmark-regression (pull_request) Successful in 29m23s
2026-03-06 20:52:48 +00:00
Compare
Author
Member

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

Review fixes applied:

  • H3: Added finally: session.close() to NamespacedProjectRepository.create()
  • M1: Removed redundant session.flush() before session.commit()
  • M2: Updated stale TDD "expected to fail" comments throughout — this PR includes the fix
  • M4: Rewrote CHANGELOG entry to describe the fix, not just tests
  • F1: Updated Robot documentation (Suite Setup/Teardown retained — required for ${PYTHON} variable)
  • F2: Fixed bare-name scenario assertion from "my-app" to "local/my-app"
  • F3: Removed redundant Base.metadata.create_all() from _make_fresh_repo() helper

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

Ready for CI and merge.

Branch has been squashed into a single commit (`d04884c7`) and rebased onto `master`. All review feedback from hurui200320 and Aditya has been addressed: **Review fixes applied:** - **H3**: Added `finally: session.close()` to `NamespacedProjectRepository.create()` - **M1**: Removed redundant `session.flush()` before `session.commit()` - **M2**: Updated stale TDD "expected to fail" comments throughout — this PR includes the fix - **M4**: Rewrote CHANGELOG entry to describe the fix, not just tests - **F1**: Updated Robot documentation (Suite Setup/Teardown retained — required for `${PYTHON}` variable) - **F2**: Fixed bare-name scenario assertion from `"my-app"` to `"local/my-app"` - **F3**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()` helper **Pre-commit hooks passed:** lint, format, typecheck, bandit, vulture, conventional commit. Ready for CI and merge.
Owner

PM Note (Day 26 — M3 PR Triage):

This is a production bug fix (persist project to DB during creation). All review findings have been addressed across ~5 rounds. The latest review is stale after squash. Has a merge conflict.

@brent.edwards — Please rebase onto master. After rebase, this needs a fresh approval. The fix is confirmed correct (adds session.commit() to the repository method) and the TDD approach with failing tests is solid.

Merge order: This should merge after #568 and #566 (approved PRs first), then before #593/#594 (which may depend on this).

**PM Note (Day 26 — M3 PR Triage):** This is a **production bug fix** (persist project to DB during creation). All review findings have been addressed across ~5 rounds. The latest review is stale after squash. Has a **merge conflict**. @brent.edwards — Please rebase onto `master`. After rebase, this needs a fresh approval. The fix is confirmed correct (adds `session.commit()` to the repository method) and the TDD approach with failing tests is solid. **Merge order**: This should merge after #568 and #566 (approved PRs first), then before #593/#594 (which may depend on this).
Author
Member

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 actual flush()commit() production fix in NamespacedProjectRepository.create(), so all 4 Behave scenarios and both Robot tests pass genuinely. No @wip tag is needed.

This PR correctly uses @tdd @bug589 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 actual `flush()` → `commit()` production fix in `NamespacedProjectRepository.create()`, so all 4 Behave scenarios and both Robot tests pass genuinely. No `@wip` tag is needed. This PR correctly uses `@tdd @bug589` tags (not `@wip`) since the tests are expected to pass.
Author
Member

Test failures fixed + merge conflict resolved

Test failure fix (commit f336b4e9)

Three scenarios in repositories_error_handling_coverage.feature were failing:

NamespacedProjectRepository create raises DatabaseError on UNIQUE IntegrityError
NamespacedProjectRepository create raises DatabaseError on non-UNIQUE IntegrityError
NamespacedProjectRepository create raises DatabaseError on OperationalError

Root cause: This PR changed NamespacedProjectRepository.create() from session.flush() to session.commit(). The error-handling test mocks only set mock.flush.side_effect, so commit() succeeded silently and the expected DatabaseError was never raised.

Fix: Added mock.commit.side_effect mirroring mock.flush.side_effect in all 4 flush-error mock helper functions (_mock_session_op_error_on_flush, _mock_session_unique_integrity_on_flush, _mock_session_non_unique_integrity_on_flush, _mock_session_integrity_on_flush). This makes the mocks work regardless of whether the production code calls flush() or commit().

Merge conflict resolved (commit 0b4fe163)

Merged master (272f9078) into the branch. One conflict in CHANGELOG.md — resolved with our #589 entry first.

Quality gates verified locally

Gate Result
nox -s lint All checks passed
nox -s typecheck 0 errors, 0 warnings
nox -s unit_tests 9,033 scenarios passed, 0 failed, 6 skipped

PR is now mergeable: true and ready to merge.

## Test failures fixed + merge conflict resolved ### Test failure fix (commit `f336b4e9`) Three scenarios in `repositories_error_handling_coverage.feature` were failing: ``` NamespacedProjectRepository create raises DatabaseError on UNIQUE IntegrityError NamespacedProjectRepository create raises DatabaseError on non-UNIQUE IntegrityError NamespacedProjectRepository create raises DatabaseError on OperationalError ``` **Root cause:** This PR changed `NamespacedProjectRepository.create()` from `session.flush()` to `session.commit()`. The error-handling test mocks only set `mock.flush.side_effect`, so `commit()` succeeded silently and the expected `DatabaseError` was never raised. **Fix:** Added `mock.commit.side_effect` mirroring `mock.flush.side_effect` in all 4 flush-error mock helper functions (`_mock_session_op_error_on_flush`, `_mock_session_unique_integrity_on_flush`, `_mock_session_non_unique_integrity_on_flush`, `_mock_session_integrity_on_flush`). This makes the mocks work regardless of whether the production code calls `flush()` or `commit()`. ### Merge conflict resolved (commit `0b4fe163`) Merged master (`272f9078`) into the branch. One conflict in `CHANGELOG.md` — resolved with our `#589` entry first. ### Quality gates verified locally | Gate | Result | |------|--------| | `nox -s lint` | All checks passed | | `nox -s typecheck` | 0 errors, 0 warnings | | `nox -s unit_tests` | 9,033 scenarios passed, 0 failed, 6 skipped | PR is now `mergeable: true` and ready to merge.
freemo left a comment

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 #589), 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 #632 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 #589), 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 #632 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.
Owner

Day 29 PM Status Check

PR appears ready for merge after multiple review rounds. All findings addressed across 7 review cycles. Last update: f336b4e9 — 9,033 scenarios passing, 0 failed. Merged master successfully.

Status: READY FOR MERGE. Waiting on 2 approving reviews per CONTRIBUTING.md. Requesting @aditya and @CoreRasurae as reviewers.

Merge order: This PR should be merged first among the M3 bug-fix series (#591 -> #593 -> #594), as later PRs may need rebase after this merges.

**Day 29 PM Status Check** PR appears ready for merge after multiple review rounds. All findings addressed across 7 review cycles. Last update: `f336b4e9` — 9,033 scenarios passing, 0 failed. Merged master successfully. **Status**: READY FOR MERGE. Waiting on 2 approving reviews per CONTRIBUTING.md. Requesting @aditya and @CoreRasurae as reviewers. **Merge order**: This PR should be merged first among the M3 bug-fix series (#591 -> #593 -> #594), as later PRs may need rebase after this merges.
hurui200320 approved these changes 2026-03-09 06:48:50 +00:00
Dismissed
hurui200320 left a comment

Code Review — PR #591 (Issue #589)

Reviewer: @hurui200320
Reviewed commit: 0b4fe163a84b261e9519e47cd6232fae92392188
Reviewed against: Issue #589 acceptance criteria, docs/specification.md, CONTRIBUTING.md, and codebase conventions.

Verdict: APPROVED (with non-blocking notes — please fix before merge)


Code Assessment

The 1-line production fix (session.flush()session.commit() + finally: session.close()) in NamespacedProjectRepository.create() is correct, minimal, and directly resolves the persistence bug. The test scaffolding — file-based SQLite with a fresh session factory per CLI invocation — faithfully reproduces the cross-session visibility problem that was the root cause. The error handling mock updates correctly ensure compatibility regardless of whether production code calls flush() or commit().

What's Good

  1. Production fix (repositories.py:2840-2857): session.commit() replaces session.flush(), and finally: session.close() ensures proper session lifecycle. Correct and minimal.
  2. Test design: File-based SQLite + fresh engine/session per step invocation accurately simulates separate CLI processes. NullPool prevents file handle leaks. Schema pre-created once in Background step.
  3. All 4 acceptance criteria covered by 4 Behave scenarios: create-then-list, multiple projects, bare-name namespace resolution, and duplicate detection.
  4. Exit code assertions on all relevant steps (create, list, duplicate).
  5. Robot Framework tests properly capture all process results and assert exit codes for init, create, and list.
  6. Error handling mocks (repositories_error_handling_coverage_steps.py): mock.commit.side_effect added alongside mock.flush.side_effect in all 4 flush-error helpers — makes tests work regardless of flush vs commit.
  7. CHANGELOG accurately describes the fix from a user perspective.

Disposition of My Previous Review (#2005)

Previous ID Status Notes
C1 (merge commit) See N1 below New merge commit 0b4fe163 introduced when resolving conflict
C2 (multiple commits) See N2 below 2 real commits + 1 merge; still violates single-commit rule
C3 (destructive commit) Resolved Squash eliminated the intermediate broken state
H1 (type label) Resolved PR now carries Type/Bug
H2 (not mergeable) Resolved PR shows mergeable: true
H3 (session.close) Resolved finally: session.close() added
M1 (redundant flush) Resolved session.flush() removed, only session.commit() remains
M2 (stale TDD comment) Resolved Feature file comment updated to "Regression tests"
M3 (cumulative count) Acknowledged Documented in docstring; acceptable
M4 (CHANGELOG) Resolved Now describes the fix, not just tests

All previous review findings from CoreRasurae (#1976), Hamza (#1993/#1994), and Aditya (comment #55074) have also been addressed in the current code.


Non-Blocking Notes (Fix Before Merge)

N1. Merge commit in branch (re-raised from #2005 C1)

Location: Branch HEAD 0b4fe163
Description: The branch contains a merge commit (Merge remote-tracking branch 'https-origin/master' into HEAD). Per CONTRIBUTING.md, branches must align with master via rebase, not merge.
Recommendation: Rebase onto master instead of merging.

N2. Multiple commits for one issue (re-raised from #2005 C2)

Location: Branch has 3 commits: d04884c7, f336b4e9, 0b4fe163
Description: Per CONTRIBUTING.md: "Every commit must completely implement an issue and close it." Commit f336b4e9 (fix(test): set commit.side_effect on flush-error mock sessions) is a fixup for the main commit d04884c7. These should be a single commit.
Recommendation: Interactive rebase to squash all work into a single commit using the prescribed message fix(project): persist project to database during creation.

N3. PR description stale

Location: PR body
Description: The Process section states "Single squashed commit, rebased onto master (no merge commits)" but the current branch has 3 commits including a merge. This will become accurate after N1/N2 are addressed.


Acceptance Criteria Coverage

Criterion Behave Robot Status
project create persists to DB Scenario 1 Test 1 Covered
project list shows created project Scenario 1 Test 1 Covered
Multiple projects created and listed Scenario 2 Test 2 Covered
Duplicate name produces clear error Scenario 4 Covered (Behave only)

Bottom line: The code fix is correct, tests are thorough, and all prior review feedback has been addressed. Approving on code quality. N1/N2 (branch hygiene) should be resolved with a squash + rebase before the actual merge — this is a 5-minute fix.

@brent.edwards — Please squash and rebase before merge.

## Code Review — PR #591 (Issue #589) **Reviewer:** @hurui200320 **Reviewed commit:** `0b4fe163a84b261e9519e47cd6232fae92392188` **Reviewed against:** Issue #589 acceptance criteria, `docs/specification.md`, `CONTRIBUTING.md`, and codebase conventions. **Verdict: APPROVED** (with non-blocking notes — please fix before merge) --- ## Code Assessment The 1-line production fix (`session.flush()` → `session.commit()` + `finally: session.close()`) in `NamespacedProjectRepository.create()` is correct, minimal, and directly resolves the persistence bug. The test scaffolding — file-based SQLite with a fresh session factory per CLI invocation — faithfully reproduces the cross-session visibility problem that was the root cause. The error handling mock updates correctly ensure compatibility regardless of whether production code calls `flush()` or `commit()`. ### What's Good 1. **Production fix** (`repositories.py:2840-2857`): `session.commit()` replaces `session.flush()`, and `finally: session.close()` ensures proper session lifecycle. Correct and minimal. 2. **Test design**: File-based SQLite + fresh engine/session per step invocation accurately simulates separate CLI processes. `NullPool` prevents file handle leaks. Schema pre-created once in Background step. 3. **All 4 acceptance criteria covered** by 4 Behave scenarios: create-then-list, multiple projects, bare-name namespace resolution, and duplicate detection. 4. **Exit code assertions** on all relevant steps (create, list, duplicate). 5. **Robot Framework tests** properly capture all process results and assert exit codes for `init`, `create`, and `list`. 6. **Error handling mocks** (`repositories_error_handling_coverage_steps.py`): `mock.commit.side_effect` added alongside `mock.flush.side_effect` in all 4 flush-error helpers — makes tests work regardless of flush vs commit. 7. **CHANGELOG** accurately describes the fix from a user perspective. --- ## Disposition of My Previous Review (#2005) | Previous ID | Status | Notes | |-------------|--------|-------| | C1 (merge commit) | **See N1 below** | New merge commit `0b4fe163` introduced when resolving conflict | | C2 (multiple commits) | **See N2 below** | 2 real commits + 1 merge; still violates single-commit rule | | C3 (destructive commit) | **Resolved** | Squash eliminated the intermediate broken state | | H1 (type label) | **Resolved** | PR now carries `Type/Bug` | | H2 (not mergeable) | **Resolved** | PR shows `mergeable: true` | | H3 (session.close) | **Resolved** | `finally: session.close()` added | | M1 (redundant flush) | **Resolved** | `session.flush()` removed, only `session.commit()` remains | | M2 (stale TDD comment) | **Resolved** | Feature file comment updated to "Regression tests" | | M3 (cumulative count) | **Acknowledged** | Documented in docstring; acceptable | | M4 (CHANGELOG) | **Resolved** | Now describes the fix, not just tests | All previous review findings from CoreRasurae (#1976), Hamza (#1993/#1994), and Aditya (comment #55074) have also been addressed in the current code. --- ## Non-Blocking Notes (Fix Before Merge) ### N1. Merge commit in branch (re-raised from #2005 C1) **Location:** Branch HEAD `0b4fe163` **Description:** The branch contains a merge commit (`Merge remote-tracking branch 'https-origin/master' into HEAD`). Per CONTRIBUTING.md, branches must align with master via rebase, not merge. **Recommendation:** Rebase onto master instead of merging. ### N2. Multiple commits for one issue (re-raised from #2005 C2) **Location:** Branch has 3 commits: `d04884c7`, `f336b4e9`, `0b4fe163` **Description:** Per CONTRIBUTING.md: "Every commit must completely implement an issue and close it." Commit `f336b4e9` (`fix(test): set commit.side_effect on flush-error mock sessions`) is a fixup for the main commit `d04884c7`. These should be a single commit. **Recommendation:** Interactive rebase to squash all work into a single commit using the prescribed message `fix(project): persist project to database during creation`. ### N3. PR description stale **Location:** PR body **Description:** The Process section states "Single squashed commit, rebased onto master (no merge commits)" but the current branch has 3 commits including a merge. This will become accurate after N1/N2 are addressed. --- ## Acceptance Criteria Coverage | Criterion | Behave | Robot | Status | |-----------|--------|-------|--------| | `project create` persists to DB | Scenario 1 | Test 1 | Covered | | `project list` shows created project | Scenario 1 | Test 1 | Covered | | Multiple projects created and listed | Scenario 2 | Test 2 | Covered | | Duplicate name produces clear error | Scenario 4 | — | Covered (Behave only) | --- **Bottom line:** The code fix is correct, tests are thorough, and all prior review feedback has been addressed. Approving on code quality. N1/N2 (branch hygiene) should be resolved with a squash + rebase before the actual merge — this is a 5-minute fix. @brent.edwards — Please squash and rebase before merge.
hurui200320 force-pushed feature/m3-fix-project-create-persist from 0b4fe163a8
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / build (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 17s
CI / security (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 2m28s
CI / integration_tests (pull_request) Successful in 3m3s
CI / docker (pull_request) Successful in 39s
CI / coverage (pull_request) Successful in 4m37s
CI / benchmark-regression (pull_request) Successful in 29m38s
to f4f1eb3f3a
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 17s
CI / build (pull_request) Successful in 17s
CI / unit_tests (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / typecheck (pull_request) Has been cancelled
CI / coverage (pull_request) Has been cancelled
CI / benchmark-regression (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
CI / security (pull_request) Has been cancelled
2026-03-09 06:53:28 +00:00
Compare
hurui200320 dismissed hurui200320's review 2026-03-09 06:53:28 +00:00
Reason:

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

hurui200320 force-pushed feature/m3-fix-project-create-persist from f4f1eb3f3a
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 17s
CI / build (pull_request) Successful in 17s
CI / unit_tests (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / typecheck (pull_request) Has been cancelled
CI / coverage (pull_request) Has been cancelled
CI / benchmark-regression (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
CI / security (pull_request) Has been cancelled
to e1e0652af3
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / build (pull_request) Successful in 17s
CI / quality (pull_request) Successful in 19s
CI / security (pull_request) Successful in 36s
CI / typecheck (pull_request) Successful in 42s
CI / unit_tests (pull_request) Successful in 2m16s
CI / docker (pull_request) Successful in 42s
CI / integration_tests (pull_request) Successful in 3m7s
CI / coverage (pull_request) Successful in 4m33s
CI / benchmark-regression (pull_request) Successful in 30m6s
2026-03-09 06:53:57 +00:00
Compare
hurui200320 left a comment

Re-approve the PR after rebase

Re-approve the PR after rebase
hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-09 06:55:22 +00:00
hurui200320 deleted branch feature/m3-fix-project-create-persist 2026-03-09 06:59:16 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
Reference
cleveragents/cleveragents-core!591
No description provided.