fix(project): show created project after creation #593
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
#590
agent project show fails to show a project.
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!593
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m3-fix-project-show-after-create"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fix
agents project shownot finding a project immediately after creation. Extends thesession.commit()fix from #589 to also coverupdate()anddelete()inNamespacedProjectRepository.Changes
Production fix (
src/cleveragents/infrastructure/database/repositories.py):session.commit()tocreate(),update(), anddelete()methodsfinally: session.close()guard to all three methodsTests & benchmarks:
features/project_show_after_create.feature)robot/project_show_after_create.robot)benchmarks/project_show_after_create_bench.py)Review feedback addressed
${PYTHON}variable); updated stale docsBase.metadata.create_all()from_make_fresh_repo()helperProcess
master(no merge commits)ISSUES CLOSED: #590
agent project showfails to show a project. #590Self-Review: TDD Failing Tests for Bug #590
Overview
This PR adds TDD failing tests that reproduce the
project showbug where a project just created cannot be found (same root cause as #589: flush without commit).Checklist
@wiptag on all scenarios (for CI filtering)@tddand@bug590tags for traceabilityvia the project-show CLI,project-show output,project-show exit code) to avoidAmbiguousStepcollisionsnox -s lintpassesnox -s typecheckpasses (0 errors)Refs: #590(notISSUES CLOSED:) to avoid premature closurecommon.resourcepatterntrack_show_after_create_foundmetricTDD Nature
Scenarios 1 and 2 are expected to fail until the fix is applied. Scenario 3 should pass since the error path for truly nonexistent projects works correctly.
No issues found. Ready for external review.
agent project showfails to show a project. #590PM Review — Day 25
Status
Context
TDD bug-fix test PR for #590 (project show fails after creation). Same root cause as #589 — the
session.commit()fix will resolve both. Tests verify theshowcommand path specifically.Action Item
@freemo: Please review alongside #591. The fix for #589 will make these tests pass as well.
Priority
Medium — blocks M3 acceptance gate.
agent project showfails to show a project. #590e1277569aad262b0d03bagent project showfails to show a project. #590Code Review -- Commit
070cc1ee(Brent) +d262b0d0(Fix)Reviewed against: Issue #590, branch
feature/m3-fix-project-show-after-create, specificationdocs/specification.mdOverview
The TDD approach is solid: the Behave scenarios correctly reproduce the bug by using file-based SQLite with a fresh
NamespacedProjectRepositoryper invocation, faithfully simulating real CLI session isolation. The Robot tests add real-subprocess integration coverage and the ASV benchmark provides a useful regression tracker. The root-cause analysis (flush without commit) is accurate and well-documented.However, there are 3 medium-severity issues that should be addressed before merge, along with several lower-severity improvements. Details follow as inline comments on the affected lines and in the summary below.
Findings Summary
runner.invoke()result -- silent create failures misattribute test outcomesinit/createexit codes -- failure at setup silently passestrack_show_after_create_founduses fixed name"bench-show-track"-- fails on repeated ASV invocations due toIntegrityError@database_retrydecorator wastes ~1s retryingProjectNotFoundError(inheritsDatabaseError) -- identified by Brent in issue comments but not tested or addressedexcept Exception: passswallows ALL exceptions, not just the expectedProjectNotFoundError--format plain-- ANSI codes may interfere withShould Containassertions in some environmentsd262b0d0removed@wiptagsdocs/specification.md:3166-3405,project showshould display Project Details, Linked Resources, Validations, Context, Indexing Status, and Active Plans sections)F4 -- Detail (No inline location -- existing codebase)
ProjectNotFoundErrorinherits fromDatabaseError(repositories.py:2782). The@database_retrydecorator (retry_patterns.py:122-126) retries onDatabaseErrorwith 3 attempts and 0.5s fixed wait. Althoughget()doesexcept ProjectNotFoundError: raise, the tenacity decorator catches the re-raised exception and retries.This means every legitimate "not found" query wastes ~1s (3 x 0.5s). This was identified in issue comment #53833 but neither tested nor fixed here. Recommend a follow-up ticket to either:
ProjectNotFoundErrorfrom the retry config, orget()soProjectNotFoundErroris raised outside the retry boundaryF7 -- Detail (Missing test case)
The Behave suite includes Scenario 3 ("Show returns error for a project that does not exist"), but the Robot suite has no equivalent. Since Robot tests exercise real subprocess behavior (no mocks), this is a meaningful gap.
F9 -- Detail (Spec compliance)
Per
docs/specification.md:3166-3405,agents project showoutput should include: Project Details (Name, Description, Resources, Remote, Created), Linked Resources, Validations, Context, Indexing Status, and Active Plans. The Behave tests only check for project name and description text presence without validating the output structure matches the spec contract.@ -0,0 +78,4 @@show_repo = _fresh_repo(self._db_path)try:show_repo.get(f"local/{name}")except Exception:F5 [Low -- Bug]: Bare
except Exception: passswallows all errors, not just the expectedProjectNotFoundError. AConnectionError,IntegrityError, orTypeErrorduring benchmarking would be silently masked, making failures undiagnosable.Suggested fix:
@ -0,0 +87,4 @@Returns 1 when create properly commits (fix applied); 0 whilebug #590 is present and get raises ProjectNotFoundError."""parsed = parse_namespaced_name("bench-show-track")F3 [Medium -- Bug]: This method always creates a project named
"bench-show-track"(hardcoded). ASV callssetup()once per suite instance, then may invoke each benchmark method multiple times for statistical sampling. On the second invocation,create()will hit the UNIQUE constraint onnamespaced_nameand raiseIntegrityError, producing either a false0return or an outright crash.Compare with
time_create_then_show(line 68) which correctly usesself._counterfor unique names.Suggested fix:
@ -0,0 +1,29 @@# These tests target bug #590 and are expected to fail until the fix is applied.F8 [Low -- Documentation]: This comment says tests are "expected to fail until the fix is applied," but the fix commit (
d262b0d0) already removed the@wiptags from the scenarios below. After the fix, this comment is stale and misleading.Suggested fix: Update to reflect current state:
@ -0,0 +94,4 @@patch(_PATCH_PROJECT_REPO, return_value=repo),patch(_PATCH_STORE_EXTRAS),):runner.invoke(project_app, ["create", name])F1 [Medium -- Test Flaw]: The
runner.invoke()result is discarded without checking the exit code. Ifproject createitself fails (import error, DB schema mismatch, Typer wiring issue), the test silently proceeds to theshowstep and fails there, misattributing the failure.Since these are TDD tests designed to isolate the show-after-create bug specifically, a silent create failure would produce a confusing false-positive ("test fails as expected") for the wrong reason.
Suggested fix:
Same issue applies to
step_create_project_show_descat line 111.@ -0,0 +16,4 @@[Documentation] After creating a project, show should display its details.... Expected to FAIL while bug #590 is present.${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_')Run Process ${PYTHON} -m cleveragents init show-testF2 [Medium -- Test Flaw]: The
Run Processcalls foragents init(line 19) andagents project create(line 21) do not capture or verify return codes. If either command fails, the test only catches it at theproject showassertion on line 25, obscuring the real failure point.Suggested fix:
Same pattern applies to the second test case starting at line 34.
@ -0,0 +20,4 @@... timeout=60s cwd=${tmpdir}Run Process ${PYTHON} -m cleveragents project create local/show-proj... timeout=60s cwd=${tmpdir}${result}= Run Process ${PYTHON} -m cleveragents project show local/show-projF6 [Low -- Robustness]: The
project showinvocation uses the default output format (rich), which may include ANSI escape codes in real subprocess environments. TheShould Containassertion on line 27 could fail or produce false positives if ANSI codes appear inside the project name string.Suggested fix: Pass
--format plainto guarantee clean output:Same applies to line 40 in the second test case.
All actionable findings from review #1977 have been addressed in commit
e07f795c:step_create_project_showandstep_create_project_show_descnow capture result and assertexit_code == 0initandcreateresults captured with${init}=/${create}=, exit codes asserted == 0 in all test casestrack_show_after_create_foundnow usesself._counterfor unique names (bench-show-track-{N})@database_retryretryingProjectNotFoundError. Will file a follow-up ticket.except Exceptionwithexcept ProjectNotFoundError(andcontextlib.suppresswhere ruff requires it)project showinvocations now pass--format plainto avoid ANSI escape codesAdditionally applied proactively:
_SRC/importlib.reload(cleveragents)convention (matching the 166 other benchmark files)NullPoolfor allcreate_engine()calls in both steps and benchmark to properly release SQLite file handlesPyright typecheck and ruff lint both pass.
Code Review 2 -- PR #593
Reviewed: Commits
070cc1eethroughe07f795c(HEAD)Scope: Full trace through DI container, session lifecycle, retry decorator, CLI call chains, error paths, test correctness
Prior review: #1977 by CoreRasurae -- findings F1-F3, F5-F8 addressed in
e07f795c; F4 deferred; F9 not addressed (acceptable)Overview
The one-line fix (
session.commit()atrepositories.py:2845) is correct for the immediate bug. Tests are solid -- Behave isolation is clean, Robot tests provide strong end-to-end coverage, and the review #1977 remediation commit is thorough. However, tracing the full session lifecycle through the DI container, retry decorator, and CLI call chains reveals the fix is a point-patch on a systemic issue, and it exposes adjacent broken paths.Findings
BUG-1:
project deleteIs Silently Broken (Same Root Cause)Severity: Critical
Category: BUG
File:
project.py:916+repositories.py:2988The CLI
deletecommand (project.py:916) callsrepo.delete(name)via the same DI pattern (_get_namespaced_project_repo()).delete()atrepositories.py:2988only callssession.flush()-- never commits. Thesessionmakersession (fromcontainer.py:160, no autocommit) rolls back on GC.The command returns success --
delete()returnsTrueat line 2989 (set before the transaction is committed), the CLI prints a success panel at line 925 -- but the row still exists.Recommendation: Apply
session.commit()aftersession.flush()indelete(), or file a blocking follow-up ticket. Same applies toupdate()at line 2955 if it has a CLI caller.BUG-2:
_store_project_extrasSilently Loses InvariantsSeverity: Major
Category: BUG
File:
project.py:95-134_store_project_extras()(called at line 551 when--invariantor--invariant-actoris provided) creates its own engine/session (project.py:114-115) and runsUPDATE ns_projects SET ... WHERE namespaced_name = :ns_name. This is a separate connection to the same SQLite file.Before this PR's fix,
create()only flushed. This separate connection could not see the uncommitted row. The UPDATE matched zero rows and returned silently. Invariants were silently lost. The fix resolves this specific path, but the pattern is fragile -- there is no rowcount check to verify the UPDATE actually modified a row.Recommendation: Add
if result.rowcount == 0: raise ...after the execute at line 131, or at minimum log a warning.BUG-3:
@database_retryRetriesProjectNotFoundError(~1s Overhead)Severity: Major
Category: BUG
File:
repositories.py:2782+core/retry_patterns.py:122-126ProjectNotFoundErrorinherits fromDatabaseError(repositories.py:2782). The@database_retrydecorator retries onDatabaseError(3 attempts, 0.5s fixed wait).get()'sexcept ProjectNotFoundError: raiseat line 2881 re-raises, but tenacity catches it at the decorator level becauseisinstance(ProjectNotFoundError(...), DatabaseError)isTrue.Every "not found" lookup wastes ~1.0s in retry sleeps. The same issue applies to duplicate detection in
create(): theIntegrityErrorhandler at line 2850 raisesDatabaseError("...already exists"), which the retry decorator catches and retries twice more.Both are deterministic, non-transient conditions that should never be retried.
Recommendation: At minimum, file a follow-up ticket. Proper fix: either exclude
ProjectNotFoundErrorfrom the retry exception list, or change its inheritance to not extendDatabaseError.BUG-4: Session Leak -- No
session.close()in Repository MethodsSeverity: Major
Category: BUG
File:
repositories.py(entire file)Zero calls to
session.close()across the entire 5,435-linerepositories.py. The onlyclose()in the database layer is inunit_of_work.py:131.Every
self._session()call creates a new session fromsessionmaker(). After the method returns, nobody closes it. The session holds an open connection and transaction state. With@database_retry, a single failedget()creates 3 leaked sessions (one per attempt, sinceself._session()is called inside the retry loop at line 2871).SQLAlchemy GC will eventually clean up via
Session.__del__, but relying on GC for connection cleanup is fragile and can exhaust SQLite file handles under load.Recommendation: File a follow-up ticket. Minimal fix: wrap each method body in
try/finally: session.close().BUG-5: Error Path After
flush()+commit()SequenceSeverity: Medium
Category: BUG
File:
repositories.py:2844-2856If
flush()succeeds butcommit()raisesOperationalError(disk full, lock timeout), the handler at line 2855 doessession.rollback()and raisesDatabaseError. The retry decorator retries with a new session. Ifcommit()partially succeeded (WAL-written but fsync failed), the retry INSERT could hit IntegrityError on a phantom duplicate. This is a narrow edge case but the error handling doesn't account for it.TEST-1: Negative Test Depends on
CliRunner(mix_stderr=True)ImplicitlySeverity: Medium
Category: TEST
File:
features/steps/project_show_after_create_steps.py:44,149-157Scenario 3 asserts
the project-show output should contain "not found". Theshowcommand prints this toerr_console(stderr) atproject.py:837. The step checksresult.output(stdout) at line 154. This works only becauseCliRunner()at line 44 defaults tomix_stderr=True(verified). If the runner is ever constructed withmix_stderr=False, this test silently breaks.Recommendation: Change line 44 to
runner = CliRunner(mix_stderr=True)to make the dependency explicit.CODE-1: Class Docstring Contradicts Implementation
Severity: Minor
Category: CODE
File:
repositories.py:2815-2816Docstring reads: "All mutating methods flush (but do NOT commit); the caller or a UnitOfWork wrapper is responsible for committing the transaction." After this PR,
create()commits.update()anddelete()still only flush. The docstring is now inaccurate.Recommendation: Update the docstring to reflect the actual behavior.
CODE-2: DI Engine Config Mismatch vs UoW
Severity: Minor
Category: CODE
File:
container.py:159vsunit_of_work.py:82-107The DI-built engine uses bare
create_engine(url, echo=False). The UoW engine usesfuture=True,isolation_level="SERIALIZABLE",connect_args={"check_same_thread": False},autoflush=False. Same database, different configs depending on code path. This can cause subtle isolation-level differences in concurrent access patterns.PROCESS-1: CHANGELOG Tag Mismatch
Severity: Minor
Category: PROCESS
File:
CHANGELOG.md:39Says
(with @wip tag)but the feature file uses@tdd @bug590.PROCESS-2: Issue #590 Subtasks Unchecked
Severity: Minor
Category: PROCESS
All 5 subtask checkboxes in issue #590 are still
- [ ].CODE-3:
type: ignore[attr-defined]Without ApprovalSeverity: Nit
Category: CODE
File:
benchmarks/project_show_after_create_bench.py:116Per DANGER_ZONE.md,
type: ignorerequires orchestrator approval.Summary Table
delete()silently broken_store_project_extrassilent data lossmix_stderrdependencytype: ignoreVerdict: REQUEST_CHANGES
Blocking: BUG-1 (
delete()has the same unfixed bug -- it is in the same class being modified by this PR and will silently eat user data).Disposition — Review #1995 (hamza.khyari)
All actionable findings have been addressed across 4 commits. Quality gates pass locally:
nox -s lint✅,nox -s typecheck✅,nox -s unit_tests(8703 scenarios, 0 failures) ✅.BUG-1:
project deleteIs Silently Broken (Critical, BLOCKING) — FIXED ✅Added
session.commit()aftersession.flush()in bothdelete()(line ~2990) andupdate()(line ~2956) ofNamespacedProjectRepository. Althoughupdate()has no CLI callers currently, it has the same flush-without-commit pattern and was fixed proactively.Commit:
c9fe3c54—fix(project): add session.commit() to delete/update and update docstringTEST-1: Implicit
mix_stderrDependency (Medium) — ADDRESSED ✅Investigated:
typer.testing.CliRunnerdoes not support themix_stderrparameter. Its__init__signature is(self, charset, env, echo_stdin, catch_exceptions)— it always mixes stderr into stdout. This differs fromclick.testing.CliRunnerwhich does acceptmix_stderr.Instead of
CliRunner(mix_stderr=True)(which would raiseTypeError), added a 4-line comment aboverunner = CliRunner()documenting the implicit dependency and explaining why the explicit parameter cannot be used.Commit:
8f38ab2cCODE-1: Class Docstring Contradicts Implementation (Minor) — FIXED ✅
Updated docstring at lines 2815-2816 from "All mutating methods flush (but do NOT commit)..." to "All mutating methods (
create,update,delete) flush and commit within their own session. No externalUnitOfWorkis needed."Commit:
c9fe3c54(same commit as BUG-1)PROCESS-1: CHANGELOG Tag Mismatch (Minor) — FIXED ✅
Changed CHANGELOG line 39 from
(with @wip tag)to(tagged @tdd @bug590)to match the actual feature file tags.Commit:
3cdd97e3CODE-3:
type: ignore[attr-defined]Without Approval (Nit) — FIXED ✅Removed
# type: ignore[attr-defined]frombenchmarks/project_show_after_create_bench.pyline 116. Confirmed all other ASV benchmark files in the project use the.unit = "..."pattern without this suppression, and pyright passes without it.Commit:
ed62a014Pre-existing / Systemic Issues — Acknowledged (Not blocking this PR)
_store_project_extrassilently loses invariants (no rowcount check)project.py. Out of scope for this TDD test PR. Will file follow-up ticket.@database_retryretriesProjectNotFoundError(~1s overhead)session.close()in repository methodsrepositories.py(5,437 lines). Requires architectural fix (context-manager pattern). Will file follow-up ticket.container.pyvsunit_of_work.py. Out of scope. Will file follow-up ticket.Ready for re-review. All 4 commits pushed to
feature/m3-fix-project-show-after-create.Code Review -- PR #593 (Commits
070cc1eethroughc9fe3c54)Reviewed against: Issue #590, branch
feature/m3-fix-project-show-after-create,docs/specification.md,CONTRIBUTING.mdPrior reviews: #1977 (CoreRasurae, REQUEST_CHANGES), #1995 (hamza.khyari, REQUEST_CHANGES)
Overview
The core code fix is correct -- adding
session.commit()tocreate(),update(), anddelete()inNamespacedProjectRepositoryresolves the reported bug and proactively fixes the same issue in the other mutating methods. Tests are well-structured with proper isolation. However, there are blocking process violations that must be fixed before merge.Findings
PROCESS-1: Merge Commit in Branch History (Critical, BLOCKING)
Location: Commit
c6a768b5The branch contains
Merge branch 'master' into feature/m3-fix-project-show-after-create. Per CONTRIBUTING.md: "Each branch must not contain merge commits -- instead as master drifts the branches should always align with master via rebase, strictly avoiding merge commits in a branch's history."Recommendation: Rebase the branch onto
origin/masterusinggit rebase origin/master(not merge), then force-push.PROCESS-2: Multiple Commits for Single Issue (Critical, BLOCKING)
Location: Branch history (8 commits)
The branch contains 8 commits for issue #590:
Per CONTRIBUTING.md: "Issues are the atomic unit of work. Each issue corresponds to exactly one commit." And: "Clean up history before merging. Use interactive rebase or amend to fix typos, consolidate fixup commits, and polish the commit series."
Commits 3-8 are fix-up commits addressing review feedback that should have been squashed into the original commits.
Recommendation: Squash all commits into a single commit using
git rebase -iwith the prescribed first linefix(project): show created project after creation, followed by a body describing the full scope of changes andISSUES CLOSED: #590in the footer.PROCESS-3: Commit Message Mismatch (Medium)
Location: HEAD commit
c9fe3c54The issue metadata prescribes
fix(project): show created project after creationas the commit message. The HEAD commit usesfix(project): add session.commit() to delete/update and update docstring. None of the non-squashed commits would serve as the final commit message.Recommendation: When squashing (per PROCESS-2), use the prescribed message exactly as the first line.
PROCESS-4: Missing
ISSUES CLOSEDFooter (Medium)Location: All commits
All commits use
Refs: #590instead ofISSUES CLOSED: #590. Per CONTRIBUTING.md and the issue template: "The body should also include the issue reference footer (e.g.,ISSUES CLOSED: #45)."Recommendation: The squashed commit message should end with
ISSUES CLOSED: #590.PROCESS-5: Type Label Mismatch (Medium)
Location: PR labels
The PR has
Type/Testingbut issue #590 isType/Bug. Per CONTRIBUTING.md: "Every PR must carry exactly one Type/ label that matches the nature of the change." This PR fixes a bug (addssession.commit()) and includes tests -- the primary nature is a bug fix.Recommendation: Replace
Type/TestingwithType/Bug.PROCESS-6: PR Not Mergeable (Blocking)
Location: PR status
The PR shows
mergeable: false, indicating conflicts with the base branch. This must be resolved before merge.Recommendation: Rebase onto current
origin/masterand force-push.PROCESS-7: Issue #590 Subtasks Still Unchecked (Minor)
Location: Issue #590 body
All 5 subtask checkboxes in issue #590 are still
- [ ]. Per workflow rules, subtasks should be checked off as completed.Recommendation: Update issue #590 body to check off completed subtasks.
CODE-1: No
session.close()in Repository Methods (Major, Pre-existing)Location:
repositories.py--create()(~line 2840),get()(~line 2871),update()(~line 2930),delete()(~line 2979)None of the methods call
session.close(). Sessions are created viaself._session()and never explicitly closed -- nofinallyblock, no context manager. Under@database_retry(3 attempts), a single failedget()creates 3 leaked sessions. While SQLAlchemy GC eventually cleans up, relying on it is fragile and can exhaust SQLite file handles under load.This is pre-existing across the entire file and not introduced by this PR, so it should not block merge -- but a follow-up ticket should be filed.
Recommendation: File a follow-up ticket. Minimal fix: wrap each method body in
try/finally: session.close().CODE-2:
@database_retryRetriesProjectNotFoundError(Major, Pre-existing)Location:
retry_patterns.py:122-126,repositories.py:2782ProjectNotFoundError(DatabaseError)means the@database_retrydecorator (which retries onDatabaseError, 3 attempts, 0.5s wait) catches re-raisedProjectNotFoundErrorat the tenacity level. Every legitimate "not found" lookup wastes ~1.0s in retry sleeps. Same applies toIntegrityError->DatabaseError("already exists")increate().Pre-existing, already flagged by CoreRasurae (F4) and hamza.khyari (BUG-3). Not blocking this PR.
Recommendation: File a follow-up ticket to exclude
ProjectNotFoundErrorfrom the retry exception config or change its inheritance.CODE-3: Redundant
flush()Beforecommit()(Nit)Location:
repositories.pylines 2844-2845, 2955-2956, 2989-2990session.commit()implicitly flushes, so the precedingsession.flush()is technically redundant. Not harmful, but unnecessary.Recommendation: Optional cleanup -- remove the explicit
flush()calls sincecommit()handles it.What's Good
update()anddelete()were proactively fixed (addressing hamza.khyari's critical BUG-1)Summary Table
ISSUES CLOSEDfooterVerdict: REQUEST_CHANGES
Blocking items (must fix before merge):
These are all fixable with a single
git rebase -i origin/master+ force-push. The code and tests themselves are solid.agent project showfails to show a project.Code Review Report: Brent's Commits on feature/m3-fix-project-show-after-create
Latest Commit Reviewed: c9fe3c5485
Author: Brent E. Edwards (brent.edwards@cleverthis.com)
Message: fix(project): add session.commit() to delete/update and update docstring
Issue: #590 — agent project show fails to show a project
Branch: feature/m3-fix-project-show-after-create
Reviewer: Aditya
Scope of Changes
CHANGELOG.mdbenchmarks/project_show_after_create_bench.pyfeatures/project_show_after_create.featurefeatures/steps/project_show_after_create_steps.pyrobot/project_show_after_create.robotsrc/cleveragents/infrastructure/database/repositories.pyThe production fix (
session.commit()added tocreate(),update(), anddelete()inNamespacedProjectRepository) is correct and the docstring has been updated to match. The test design — file-based SQLite with a fresh session factory per CLI invocation — faithfully reproduces the real cross-process visibility bug. Previous review feedback from CoreRasurae and Hamza has been thoroughly addressed.Findings
F1. [MEDIUM] CHANGELOG.md contains unrelated cosmetic edits bundled into the fix commits
Beyond the expected new entry for bug #590, the patch also converts four existing em-dashes (
—/–) to double hyphens (--) in unrelated CHANGELOG entries:None of these lines are related to issue #590 or to the project show/create fix.
CONTRIBUTING.mdis explicit:These edits should not be present in this PR at all. They obscure the real diff, make the commit harder to review, and would make a future revert of this commit also revert the cosmetic changes.
Recommendation: Remove the four em-dash edits from the CHANGELOG. If a house style requires
--over—, do that as a standalone cosmetic-only commit in a separate PR.F2. [MEDIUM] Robot tests create a per-test
tmpdiralongside the Suite Setup environmentAll three Robot test cases create their own isolated directory:
yet the suite also declares:
This means two separate temp environments exist simultaneously for each test run — one from
common.resource's suite-level setup and one per test. Every other Robot file in this repo uses the suite-level environment exclusively. The intent here (full isolation per test case, freshagents initper case) is valid, but the suite-level setup/teardown fromcommon.resourceis then entirely redundant and potentially confusing.Recommendation: If each test case needs its own isolated environment (which is the right design for these regression tests), remove
Suite Setup/Suite Teardownfrom this file and rely solely on the per-testtmpdir+[Teardown] Remove Directory. This matches the intent and removes the redundant lifecycle.F3. [LOW] Third Robot test case asserts only on exit code — does not verify the error message
Project Show Returns Error For Nonexistent Projectchecks thatproject show local/nonexistentexits non-zero:There is no assertion on
${result.stdout}or${result.stderr}content. Ifagents initfails silently, or the project DB is misconfigured,project showwould also fail with a non-zero exit and this test would pass for the wrong reason. The Behave equivalent (Scenario 3) correctly asserts"not found"in the output. The Robot test should match.Recommendation: Add a content assertion after the exit code check:
F4. [LOW]
_make_fresh_repoand the benchmark's_fresh_repocallBase.metadata.create_all()on every invocation after the schema is already initialisedIn
step_init_show_db(the Background step), the schema is created once:Every subsequent call to
_make_fresh_repo— one percreateorshowstep — also runs:The same applies in the benchmark:
setup()creates the schema, then every_fresh_repo()call re-runscreate_all. SQLAlchemy'scheckfirst=Truedefault makes this a no-op functionally, but it adds an unnecessary schema introspection query on every step and benchmark invocation.Recommendation: Remove
Base.metadata.create_all(engine)from_make_fresh_repoand_fresh_repo. The schema is guaranteed to exist after the Background step /setup()respectively. This also makes the helper's responsibility clearer — it creates a session factory, not a schema.Summary Table
Suite Setupfromcommon.resourceand per-testmkdtemp()— redundant and inconsistent with repo pattern_make_fresh_repo/_fresh_repocallBase.metadata.create_all()on every invocation after schema is already initialisedc9fe3c54859c301afe4aBranch has been squashed into a single commit (
9c301afe) and rebased ontomaster. All review feedback from hurui200320 and Aditya has been addressed:Review fixes applied:
${PYTHON}variable); updated stale documentationBase.metadata.create_all()from_make_fresh_repo()helperfinally: session.close()to all three mutating methodsPre-commit hooks passed: lint, format, typecheck, bandit, vulture, conventional commit.
Ready for CI and merge.
PM Note (Day 26 — M3 PR Triage):
Production bug fix — show created project after creation. All review findings addressed across ~5 rounds. Latest review stale after squash. Has a merge conflict.
@brent.edwards — Please rebase onto
master. Needs fresh approval after rebase.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
flush()→commit()production fix oncreate(),update(), anddelete(), so all 3 Behave scenarios and both Robot tests pass genuinely. No@wiptag is needed.This PR correctly uses
@tdd @bug590tags (not@wip) since the tests are expected to pass.CI Fix + Master Merge —
47ec9095Fixes applied
1. BDD unit tests — 3 failing
createscenarios (features/steps/repositories_error_handling_coverage_steps.py)Root cause: PR changed
NamespacedProjectRepository.create()fromsession.flush()tosession.commit(), but the shared mock session helpers (_mock_session_op_error_on_flush,_mock_session_unique_integrity_on_flush,_mock_session_non_unique_integrity_on_flush,_mock_session_integrity_on_flush) only setmock.flush.side_effect. Thecommit()call hit a default MagicMock (no exception), so the error path was never triggered.Fix: Added
mock.commit.side_effectmirroring the existingmock.flush.side_effectto all 4 mock helpers. This is safe for other repository tests that still callflush()— they never callcommit(), so the new side effect is inert for them.2. Robot integration tests — 3 failing
--formatscenarios (robot/project_show_after_create.robot)Root cause:
--format plainwas placed beforeproject showin the command line (cleveragents --format plain project show ...), but--formatis an option on theshowsubcommand, not the top-level CLI. Typer raisedNoSuchOption.Fix: Moved
--format plainafterproject show(cleveragents project show --format plain ...).Master merge
Merged
master(272f9078) into branch. Single conflict inCHANGELOG.md— resolved with PR entry first, followed by all master entries.Quality gates
nox -s lintnox -s typechecknox -s unit_testsPR is now
mergeable: true.agents project showfails after create test #633Planning Authority Review — Ready for Merge
Branch naming non-compliance noted: This PR uses the
feature/m3-fix-*prefix, but as a bug fix PR (fixes #590), it should use 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 #633 has been created to track the TDD/test coverage counterpart for this fix.
Recommendation: Merge. All review rounds are complete, CI is passing, and the branch has been rebased on master.
Day 29 PM Status Check
PR appears ready for merge after multiple review rounds. All findings addressed. Last update:
47ec9095— 9,032 scenarios passing. Merged master successfully.Status: READY FOR MERGE (after #591 merges). Requesting @aditya and @CoreRasurae as reviewers.
Merge order: Second in M3 bug-fix series (#591 -> #593 -> #594). May need rebase after #591 merges.
Code Review — PR #593 (Commits
9c301afethrough47ec9095)Reviewed against: Issue #590, branch
feature/m3-fix-project-show-after-create,docs/specification.md,CONTRIBUTING.mdPrior reviews: #2006 (hurui200320/me, REQUEST_CHANGES), #1977 (CoreRasurae), #1995 (hamza.khyari), #2040 (freemo)
Overview
The production fix is correct and well-tested. Adding
session.commit()tocreate(),update(), anddelete()inNamespacedProjectRepositoryresolves the reported bug. Thefinally: session.close()guards are a proper addition. Tests are comprehensive: 3 Behave BDD scenarios, 3 Robot integration tests, and ASV benchmarks. Previous review feedback from CoreRasurae, hamza.khyari, and Aditya has been substantially addressed.Findings
PROCESS-1: Merge Commit in Branch (Non-blocking)
47ec9095—Merge remote-tracking branch 'https-origin/master' into HEADgit rebase -i origin/master.PROCESS-2: Multiple Commits for Single Issue (Non-blocking)
9c301afe,3a7d648f,47ec9095) instead of the prescribed 1.fix(project): show created project after creationandISSUES CLOSED: #590footer.PROCESS-3: PR Not Mergeable (Non-blocking, but requires action before merge)
mergeable: falsedue to CHANGELOG.md conflicts.origin/masterto resolve.PROCESS-4: Commit Footer Format (Non-blocking)
9c301afeusesCloses: #590.ISSUES CLOSED: #590.ISSUES CLOSED: #590footer in the final commit.BENCH-1: Benchmark
_fresh_repoStill CallsBase.metadata.create_all()(Low, Non-blocking)benchmarks/project_show_after_create_bench.py:54checkfirst=True), but adds unnecessary schema introspection overhead per benchmark iteration.Base.metadata.create_all(engine)from_fresh_repo()for consistency.CODE-1: Redundant
flush()Beforecommit()(Nit, Non-blocking)repositories.py:2956-2957(update()) and2992-2993(delete())session.commit()implicitly flushes, so the precedingsession.flush()is redundant.create()at line 2844 correctly uses onlycommit().session.flush()calls for consistency withcreate().What's Good
session.commit()resolves the persistence visibility bugupdate()anddelete()proactively fixed (addressing hamza.khyari's critical BUG-1)finally: session.close()properly guards session lifecycle for all mutating methodscommit.side_effect— properly handles both thecreate()path (commit-only) andupdate()/delete()paths (flush-then-commit)NullPool)# type: ignorecomments addedSummary Table
Verdict: APPROVED
The code and tests are solid. No blocking bugs, security issues, or test flaws. Process items (merge commit, multiple commits, conflicts) should ideally be cleaned up before merge via a single
git rebase -i origin/master+ force-push, but are not blocking approval.According to Jeff's comment here: #494 (comment)
Since OpenCode gives the review and I have confirmed the correctness of the review, and it's approved, I'll fix the process issue and merge the PR.
47ec9095b2946db693afNew commits pushed, approval review dismissed automatically according to repository settings
agent project showfails to show a project. #590agents project showfails after create test #633agent project showfails to show a project. #590