fix(project): persist project to database during creation #591
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
6 participants
Notifications
Due date
No due date set.
Blocks
#589
agents project doesn't show created projects.
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!591
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m3-fix-project-create-persist"
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?
Summary
Fix
agents project createnot persisting projects to the database.NamespacedProjectRepository.create()calledsession.flush()but neversession.commit(), so projects were invisible to subsequentagents project listinvocations that open a separate session.Changes
Production fix (
src/cleveragents/infrastructure/database/repositories.py):session.flush()withsession.commit()inNamespacedProjectRepository.create()finally: session.close()guard for proper session lifecycleTests & benchmarks:
features/project_create_persist.feature)robot/project_create_persist.robot)benchmarks/project_create_persist_bench.py)Review feedback addressed
finally: session.close()tocreate()methodsession.flush()beforesession.commit()${PYTHON})"my-app"to"local/my-app"in namespace scenarioBase.metadata.create_all()from_make_fresh_repo()Process
master(no merge commits)ISSUES CLOSED: #589
agents projectdoesn't show created projects. #589Self-Review: TDD Failing Tests for Bug #589
Overview
This PR adds TDD failing tests that reproduce the persistence bug where
agents project createflushes but never commits, making projects invisible toagents project list.Checklist
@wiptag on all scenarios (for CI filtering)@tddand@bug589tags for traceabilityvia the persist CLI,persist project list,persist duplicate creation) to avoidAmbiguousStepcollisions with existing step filesnox -s lintpassesnox -s typecheckpasses (0 errors)Refs: #589(notISSUES CLOSED:) to avoid premature closurecommon.resourcepatterntrack_list_after_create_countmetric that returns 0 (bug present) or 1 (fix applied)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.
brent.edwards referenced this pull request2026-03-05 01:35:34 +00:00
agents projectdoesn't show created projects. #589PM Review — Day 25
Status
Context
This is a TDD bug-fix test PR for #589 (project persistence bug —
flush()withoutcommit()inNamespacedProjectRepository.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 inrepositories.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).
156c70bce9bd7953f0a5agents projectdoesn't show created projects. #589Code 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
robot/project_create_persist.robotinitorcreaterobot/project_create_persist.robotagents initexit code unchecked in both test casesfeatures/steps/project_create_persist_steps.pyexit_code == 0; failure surfaces as confusing list assertionbenchmarks/project_create_persist_bench.py_SRC/importlib.reloadconvention used in 166 other benchmarksbenchmarks/project_create_persist_bench.pytrack_list_after_create_countuses hardcoded name"bench-track"-- not idempotent; UNIQUE violation on repeated callscreate_engine()called but neverengine.dispose()-- resource/file-lock leaks across scenariosrobot/project_create_persist.robot${list.rc} == 0(first test does)fix(project)prefix implies a bug fix, but commit only adds tests;test(project)would be more accuratefeatures/project_create_persist.featurelocal/...); no bare-name test matching the issue's own repro stepsfeatures/steps/project_create_persist_steps.py_PATCH_STORE_EXTRASmock is unnecessary since--invariantflag is never passedAcceptance Criteria Coverage (Issue #589)
project createpersists to DBproject listshows created projectInline comments below provide details and recommendations for each finding.
@ -0,0 +19,4 @@from sqlalchemy import create_enginefrom sqlalchemy.orm import sessionmakerF4 -- MEDIUM | Convention: This try/except
ModuleNotFoundErrorimport 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:The try/except approach has two practical downsides:
importlib.reload()-- ASV may load a stale installed package instead of the source tree.ModuleNotFoundErrorfailures -- a renamed class or circular import would trigger theexceptbranch, 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)F6 -- MEDIUM | Performance: Same engine leak issue as the Behave steps.
create_engine()is called but neverengine.dispose()-d. Consider usingpoolclass=NullPoolor adding explicitengine.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 whilebug #589 is present and list returns empty.F5 -- MEDIUM | Bug Detection: This method always creates a project named
"bench-track". If ASV runs it more than once persetup()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 usesself._counterto generate unique names per invocation.Recommendation: Use
self._counteroruuid4()for the project name:@ -0,0 +8,4 @@Given a fresh project-persist database is initialised@tdd @bug589Scenario: Created project appears in project listF9 -- 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 onparse_namespaced_namedefaulting to thelocal/namespace. Adding one scenario with a bare name (e.g.,project1instead oflocal/project1) would more faithfully reproduce the reported bug and exercise the default-namespace resolution path end-to-end.This is minor since
parse_namespaced_namehas 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:F6 -- MEDIUM | Performance:
create_engine()is called here but the engine is neverdispose()-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_dbattempts toshutil.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 usepoolclass=NullPool(fromsqlalchemy.pool) so connections are not pooled:@ -0,0 +102,4 @@repo = _make_fresh_repo(context.persist_db_path)with (patch(_PATCH_PROJECT_REPO, return_value=repo),patch(_PATCH_STORE_EXTRAS),F10 -- LOW | Convention:
_store_project_extrasis only called in thecreatecommand when--invariantor--invariant-actorflags 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 thecreatecommand'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)F3 -- MEDIUM | Test Flaws: The result is appended to
context.persist_create_resultsbut no scenario step ever assertsresult.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:
or add a new Gherkin
Thenstep likeThen the persist creation should succeedthat checkscontext.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-testF2 -- HIGH | Test Flaws:
agents initis 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 addShould 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-multiF1 -- 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 initfails (permissions, disk, migration error), both creates silently fail, and the finalShould Containon 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} 0after 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/firstF7 -- LOW | Test Flaws: Unlike the first test (line 27) which asserts
Should Be Equal As Integers ${list.rc} 0, this second test jumps directly toShould Containon stdout. Ifproject listexits non-zero,${list.stdout}could be empty while the actual error is in${list.stderr}.Recommendation: Add
Should Be Equal As Integers ${list.rc} 0before theShould Containassertions, consistent with the first test case.All 10 findings from review #1976 have been addressed in commit
35634253:${create_first}=/${create_second}=, exit codes assertedagents initcaptured as${init}=,${init.rc}checked == 0assert result.exit_code == 0added afterrunner.invoke(project_app, ["create", name])_SRC/importlib.reload(cleveragents)conventiontrack_list_after_create_countnow usesself._counterfor unique namescreate_engine()calls now usepoolclass=NullPool${list.rc}checked == 0 beforeShould Containtest(project):prefix"my-app"_PATCH_STORE_EXTRASis patchedNote: 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@wiptags. The new bare-name scenario follows the same convention (no@wip).Pyright typecheck and ruff lint both pass.
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
NamespacedProjectRepository.delete()andupdate()have the same flush-without-commit bugProjectResourceLinkRepository.create_link()and.remove_link()same bug -- called directly from project CLI without UoW@database_retryretriesIntegrityError3 times (permanent error) -- duplicate project creation wastes ~1.5s before failingcreate()opens a session, commits, returns, but nofinally: session.close(). All 42 session-factory repo methods leak sessions.exit_code != 0-- only checks output textstep_list_projects_persistdoesn't assertexit_code == 0-- list failure surfaces as confusing "name not in output"@wiptag)" but there are 4 scenarios and no@wiptagsBUG-1:
delete()andupdate()in Same Class Have the Same BugSeverity: Major
File:
repositories.py:2915(update),repositories.py:2964(delete)delete()is called directly fromproject.py:916without UoW. Sameflush()withoutcommit()pattern.agents project deletewill appear to succeed but the deletion won't persist.Recommendation: Follow-up issue to add
session.commit()to both methods.BUG-2:
ProjectResourceLinkRepositorySame PatternSeverity: Major
File:
repositories.py:3018(create_link),repositories.py:3120(remove_link)Called directly from
project.py:564andproject.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-126IntegrityError(UNIQUE constraint) is caught, wrapped asDatabaseError, and re-raised.@database_retryretries onDatabaseError-- including permanent errors that will never succeed: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-2856The method calls
self._session()to create a session but has nofinally: 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
ResourceRegistryServicemethods which all havefinally: 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-174step_persist_dup_failsonly checks output text. Doesn't assertresult.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-154step_list_projects_persiststores the result but never checksexit_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 == 0to the list step.DOC-1: CHANGELOG Is Stale
Severity: Minor
File:
CHANGELOG.md:35Says "3 Behave BDD scenarios (with
@wiptag)". After the review fix commit there are 4 scenarios and no@wiptags.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.
update
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
exit_code != 0b1682999assert result.exit_code != 0instep_persist_dup_failsexit_code == 0b1682999assert result.exit_code == 0instep_list_projects_persist@wiptag)"063c9318@tdd @bug589)"Acknowledged — Pre-existing Systemic Issues (Not Introduced by This PR)
delete()andupdate()flush without commitc9fe3c54) which addssession.commit()to both methods.ProjectResourceLinkRepositorysame pattern@database_retryretries permanentIntegrityErrorfinally: 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
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)
142424b9isMerge 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.b1682999(exit_code assertions),cce381b9(CHANGELOG fix), and063c9318(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.CHANGELOG.md(commitcce381b9)cce381b9accidentally deleted the entire CHANGELOG.md (328 lines -> 1 line), then063c9318restored 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
Type/Testingbut the issue isType/Bugand 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 beType/Bug.mergeable: false-- conflicts with master need resolution via rebase (not merge).repositories.py:2840-2856session.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, addingfinally: session.close()to the method being modified would be a minimal, low-risk improvement. Recommendation: Wrap the try block in atry/finallythat callssession.close().MEDIUM
repositories.py:2844flush()beforecommit().session.flush()on line 2844 is now unnecessary becausesession.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 theflush()call.features/project_create_persist.feature:1benchmarks/project_create_persist_bench.py:96-109track_list_after_create_countreturns cumulative count. Each call creates a new project, butlist_projects()returns ALL projects in the database. Sincesetup()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.CHANGELOG.md:36-41agents project createnot persisting projects to the database").LOW
- [ ]). Per project guidelines, subtasks should be checked off as they are completed.Acceptance Criteria Coverage
project createpersists to DBproject listshows created projectWhat's Good
session.commit()) is the correct, minimal solution.AmbiguousStepcollisions.context.add_cleanup()and[Teardown]in Robot.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 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
CHANGELOG.mdbenchmarks/project_create_persist_bench.pyfeatures/project_create_persist.featurefeatures/steps/project_create_persist_steps.pyrobot/project_create_persist.robotsrc/cleveragents/infrastructure/database/repositories.pyThe production fix is a correct, minimal one-liner: adding
session.commit()aftersession.flush()inNamespacedProjectRepository.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
tmpdirdespite Suite Setup already provisioning oneBoth Robot test cases call
Evaluate __import__('tempfile').mkdtemp(...)inline:yet the suite already declares:
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 callsagents initwithin it. If the intent is an isolated fresh environment per test case (which is valid here), the suite-levelSetup Test Environment/Cleanup Test Environmentshould be removed from this file so the lifecycle is clear and unambiguous.Recommendation: Remove
Suite Setup/Suite Teardownfrom this file and rely solely on the per-testtmpdir+[Teardown] Remove Directory, consistent with the isolation intent. Or conversely, use the suite-level environment and drop the per-testmkdtemp(), matching the repo-wide pattern.F2. [MEDIUM] Scenario 3 bare-name assertion is a loose substring match — namespace resolution unverified
The assertion
"my-app"is a substring of"local/my-app". The step implementation uses a plainassert name in outputcheck, 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 thatparse_namespaced_nameresolves the bare name to thelocal/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:
This directly verifies the namespace resolution that the scenario is designed to test.
F3. [LOW]
_make_fresh_repocallsBase.metadata.create_all()redundantly on every step invocationstep_init_persist_db(the Background step) already creates the full schema once:However,
_make_fresh_repo— called on everycreateandliststep — also calls:SQLAlchemy's
create_allwith the defaultcheckfirst=Trueonly 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 inbenchmarks/project_create_persist_bench.pywheresetup()creates the schema and_fresh_repo()recreates it on every benchmark call.Recommendation: Remove the
Base.metadata.create_all(engine)call from_make_fresh_reposince the schema is guaranteed to exist after the Background step. The benchmark'ssetup()already handles schema creation independently, so_fresh_repothere can equally be simplified.Summary Table
tmpdiralongside Suite Setup fromcommon.resource; ambiguous lifecycle"my-app"instead of"local/my-app"; namespace resolution never verified_make_fresh_repocallsBase.metadata.create_all()on every step/benchmark invocation despite schema being pre-createdagents projectdoesn't show created projects.063c93181dd04884c715Branch has been squashed into a single commit (
d04884c7) and rebased ontomaster. All review feedback from hurui200320 and Aditya has been addressed:Review fixes applied:
finally: session.close()toNamespacedProjectRepository.create()session.flush()beforesession.commit()${PYTHON}variable)"my-app"to"local/my-app"Base.metadata.create_all()from_make_fresh_repo()helperPre-commit hooks passed: lint, format, typecheck, bandit, vulture, conventional commit.
Ready for CI and merge.
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 (addssession.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).
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
@wiptag mechanism.The standard
.feature).robot)@wiptag above the Scenario[Tags] wipto the test case@wiptagwipfrom[Tags]behave features/foo.feature:42(by line number)robot --include wip robot/foo.robotStatus of this PR
No changes required. This is a fix PR — it includes the actual
flush()→commit()production fix inNamespacedProjectRepository.create(), so all 4 Behave scenarios and both Robot tests pass genuinely. No@wiptag is needed.This PR correctly uses
@tdd @bug589tags (not@wip) since the tests are expected to pass.Test failures fixed + merge conflict resolved
Test failure fix (commit
f336b4e9)Three scenarios in
repositories_error_handling_coverage.featurewere failing:Root cause: This PR changed
NamespacedProjectRepository.create()fromsession.flush()tosession.commit(). The error-handling test mocks only setmock.flush.side_effect, socommit()succeeded silently and the expectedDatabaseErrorwas never raised.Fix: Added
mock.commit.side_effectmirroringmock.flush.side_effectin 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 callsflush()orcommit().Merge conflict resolved (commit
0b4fe163)Merged master (
272f9078) into the branch. One conflict inCHANGELOG.md— resolved with our#589entry first.Quality gates verified locally
nox -s lintnox -s typechecknox -s unit_testsPR is now
mergeable: trueand ready to merge.agents project createpersistence commit test #632Planning 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 thebugfix/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.
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.
Code Review — PR #591 (Issue #589)
Reviewer: @hurui200320
Reviewed commit:
0b4fe163a84b261e9519e47cd6232fae92392188Reviewed 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()) inNamespacedProjectRepository.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 callsflush()orcommit().What's Good
repositories.py:2840-2857):session.commit()replacessession.flush(), andfinally: session.close()ensures proper session lifecycle. Correct and minimal.NullPoolprevents file handle leaks. Schema pre-created once in Background step.init,create, andlist.repositories_error_handling_coverage_steps.py):mock.commit.side_effectadded alongsidemock.flush.side_effectin all 4 flush-error helpers — makes tests work regardless of flush vs commit.Disposition of My Previous Review (#2005)
0b4fe163introduced when resolving conflictType/Bugmergeable: truefinally: session.close()addedsession.flush()removed, onlysession.commit()remainsAll 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
0b4fe163Description: 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,0b4fe163Description: 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 commitd04884c7. 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
project createpersists to DBproject listshows created projectBottom 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.
0b4fe163a8f4f1eb3f3aNew commits pushed, approval review dismissed automatically according to repository settings
f4f1eb3f3ae1e0652af3Re-approve the PR after rebase
agents projectdoesn't show created projects. #589agents project createpersistence commit test #632agents projectdoesn't show created projects. #589