cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

test(e2e): validate M5 acceptance criteria for v3.4.0 milestone closure #725

Merged
hurui200320 merged 1 commit from test/m5-acceptance-gate into master 2026-03-13 05:44:16 +00:00
Conversation 6 Commits 1 Files changed 5 +335
hurui200320 commented 2026-03-12 06:34:19 +00:00
Member
Copy link

Summary

Add four CLI-based integration test cases to the M5 E2E verification suite that exercise the exact commands from the v3.4.0 milestone description via real subprocess calls to python -m cleveragents.

Closes #496

Changes

New CLI Test Cases in robot/m5_e2e_verification.robot

Test Case CLI Command Tested
CLI Project Create Large Project agents project create local/large-project
CLI Resource Add Git Checkout agents resource add git-checkout local/large-repo --path <dir> --branch main
CLI Project Link Resource agents project link-resource local/large-project local/large-repo
CLI Project Show Displays Linked Resource agents project show --format plain local/large-project

Each test:

  • Creates an isolated temp directory with its own CLEVERAGENTS_HOME
  • Initialises a workspace via agents init
  • Runs the target CLI command as a subprocess with on_timeout=kill
  • Asserts zero exit code and verifies expected output
  • Tears down the temp directory

Test Isolation and Robustness

  • Each CLI subprocess overrides CLEVERAGENTS_HOME via env:CLEVERAGENTS_HOME=${tmpdir} to prevent shared state between tests.
  • Added on_timeout=kill to all CLI subprocess calls.
  • Added --format plain to project list for output format consistency.
  • Added branch property assertion (branch.*main regex) in resource show verification.
  • Added explanatory comments for fake repo directories (CLI records location without git validation).
  • Removed redundant Library imports already provided by common.resource.
  • Standardised Run Process line-continuation style.

Assertion Strengthening (review feedback rounds 1 and 2)

  • CLI Project Link Resource: Added post-link resource show to capture the resource ULID, then project show verification proving the link was persisted. When the ULID is available, asserts the exact resource_id appears in output (not just the generic "resource_id": key).
  • CLI Project Show Displays Linked Resource: Differentiated from Test 3 by capturing the resource ULID from resource show before linking, verifying resource show independently after linking, and asserting the exact ULID appears in project show output.
  • Branch assertion: Uses Should Match Regexp ... branch.*main instead of Should Contain ... main to avoid false matches on unrelated occurrences.

Bug Fix: ProjectResourceLinkRepository session lifecycle

During test verification, discovered that create_link() and remove_link() in ProjectResourceLinkRepository only called session.flush() without session.commit(), causing linked resource data to be silently lost between sessions.

Fixes applied:

  • Added session.commit() after session.flush() in both create_link() and remove_link().
  • Added finally: session.close() to both methods to match the session-factory lifecycle pattern used by all other mutating repository methods (e.g., NamespacedProjectRepository.create(), .update(), .delete()).
  • Added session.refresh(link) and session.expunge(link) before return in create_link() so the returned ORM instance is fully loaded and usable in detached state after session close.
  • Updated ProjectResourceLinkRepository class docstring to document commit and close behaviour.

Cross-Session Persistence Regression Guard

Added two new Behave scenarios in features/project_repository.feature:

  • Created link is visible in a new session (cross-session persistence): Opens a new session from the same engine after create_link() and verifies the link is found.
  • Removed link is absent in a new session (cross-session persistence): Opens a new session after remove_link() and verifies the link is absent.

These scenarios would fail if commit() were removed, providing a true regression guard. The existing tests used a shared session (lambda: session factory) where flush() alone was sufficient, so they could never have caught this bug.

Context/ACMS CLI Coverage

Context tier management and ACMS scoped context output criteria are validated at the Python API level via helper_m5_e2e_verification.py because the CLI does not yet expose dedicated context/ACMS inspection commands. This is documented in the suite-level Documentation block and in Test 4's documentation.

CHANGELOG Update

Added two entries under ## Unreleased:

  1. Four CLI-based integration test cases for M5 E2E verification.
  2. Production bug fix for ProjectResourceLinkRepository session lifecycle.

Preserved Existing Tests

The nine existing Python-API test cases (domain model, persistence, context tiers, ACMS pipeline) are preserved unchanged and reorganised under section comments for clarity.

Verification

All nox sessions pass:

  • lint, typecheck, format, security_scan, dead_code, docs, build -- clean
  • unit_tests -- 10558 scenarios passed (2 new cross-session persistence scenarios)
  • integration_tests -- 1474 passed (13 robot tests in this suite)
  • coverage_report -- 98% (above 97% threshold)

Milestone Acceptance Criteria Satisfied

  • agents project create local/large-project
  • agents resource add git-checkout local/large-repo --path ... --branch main
  • agents project link-resource local/large-project local/large-repo
  • agents project show local/large-project
  • 10,000+ files index without timeout
  • Context window management (hot/warm/cold tiers)
  • ACMS v1 pipeline produces scoped context output
  • Coverage >=97%
## Summary Add four CLI-based integration test cases to the M5 E2E verification suite that exercise the exact commands from the v3.4.0 milestone description via real subprocess calls to `python -m cleveragents`. Closes #496 ## Changes ### New CLI Test Cases in `robot/m5_e2e_verification.robot` | Test Case | CLI Command Tested | |---|---| | `CLI Project Create Large Project` | `agents project create local/large-project` | | `CLI Resource Add Git Checkout` | `agents resource add git-checkout local/large-repo --path <dir> --branch main` | | `CLI Project Link Resource` | `agents project link-resource local/large-project local/large-repo` | | `CLI Project Show Displays Linked Resource` | `agents project show --format plain local/large-project` | Each test: - Creates an isolated temp directory with its own `CLEVERAGENTS_HOME` - Initialises a workspace via `agents init` - Runs the target CLI command as a subprocess with `on_timeout=kill` - Asserts zero exit code and verifies expected output - Tears down the temp directory ### Test Isolation and Robustness - Each CLI subprocess overrides `CLEVERAGENTS_HOME` via `env:CLEVERAGENTS_HOME=${tmpdir}` to prevent shared state between tests. - Added `on_timeout=kill` to all CLI subprocess calls. - Added `--format plain` to `project list` for output format consistency. - Added branch property assertion (`branch.*main` regex) in resource show verification. - Added explanatory comments for fake repo directories (CLI records location without git validation). - Removed redundant `Library` imports already provided by `common.resource`. - Standardised `Run Process` line-continuation style. ### Assertion Strengthening (review feedback rounds 1 and 2) - **CLI Project Link Resource**: Added post-link `resource show` to capture the resource ULID, then `project show` verification proving the link was persisted. When the ULID is available, asserts the exact resource_id appears in output (not just the generic `"resource_id":` key). - **CLI Project Show Displays Linked Resource**: Differentiated from Test 3 by capturing the resource ULID from `resource show` before linking, verifying `resource show` independently after linking, and asserting the exact ULID appears in `project show` output. - **Branch assertion**: Uses `Should Match Regexp ... branch.*main` instead of `Should Contain ... main` to avoid false matches on unrelated occurrences. ### Bug Fix: `ProjectResourceLinkRepository` session lifecycle During test verification, discovered that `create_link()` and `remove_link()` in `ProjectResourceLinkRepository` only called `session.flush()` without `session.commit()`, causing linked resource data to be silently lost between sessions. Fixes applied: - Added `session.commit()` after `session.flush()` in both `create_link()` and `remove_link()`. - Added `finally: session.close()` to both methods to match the session-factory lifecycle pattern used by all other mutating repository methods (e.g., `NamespacedProjectRepository.create()`, `.update()`, `.delete()`). - Added `session.refresh(link)` and `session.expunge(link)` before return in `create_link()` so the returned ORM instance is fully loaded and usable in detached state after session close. - Updated `ProjectResourceLinkRepository` class docstring to document commit and close behaviour. ### Cross-Session Persistence Regression Guard Added two new Behave scenarios in `features/project_repository.feature`: - **Created link is visible in a new session (cross-session persistence)**: Opens a new session from the same engine after `create_link()` and verifies the link is found. - **Removed link is absent in a new session (cross-session persistence)**: Opens a new session after `remove_link()` and verifies the link is absent. These scenarios would fail if `commit()` were removed, providing a true regression guard. The existing tests used a shared session (`lambda: session` factory) where `flush()` alone was sufficient, so they could never have caught this bug. ### Context/ACMS CLI Coverage Context tier management and ACMS scoped context output criteria are validated at the **Python API level** via `helper_m5_e2e_verification.py` because the CLI does not yet expose dedicated context/ACMS inspection commands. This is documented in the suite-level Documentation block and in Test 4's documentation. ### CHANGELOG Update Added two entries under `## Unreleased`: 1. Four CLI-based integration test cases for M5 E2E verification. 2. Production bug fix for `ProjectResourceLinkRepository` session lifecycle. ### Preserved Existing Tests The nine existing Python-API test cases (domain model, persistence, context tiers, ACMS pipeline) are preserved unchanged and reorganised under section comments for clarity. ## Verification All nox sessions pass: - **lint**, **typecheck**, **format**, **security_scan**, **dead_code**, **docs**, **build** -- clean - **unit_tests** -- 10558 scenarios passed (2 new cross-session persistence scenarios) - **integration_tests** -- 1474 passed (13 robot tests in this suite) - **coverage_report** -- 98% (above 97% threshold) ## Milestone Acceptance Criteria Satisfied - [x] `agents project create local/large-project` - [x] `agents resource add git-checkout local/large-repo --path ... --branch main` - [x] `agents project link-resource local/large-project local/large-repo` - [x] `agents project show local/large-project` - [x] 10,000+ files index without timeout - [x] Context window management (hot/warm/cold tiers) - [x] ACMS v1 pipeline produces scoped context output - [x] Coverage >=97%
hurui200320 added this to the v3.4.0 milestone 2026-03-12 06:34:30 +00:00
hurui200320 commented 2026-03-12 06:49:07 +00:00
Author
Member
Copy link

Code Review: PR #725 — test/m5-acceptance-gate

Ticket: #496 | Milestone: v3.4.0
File changed: robot/m5_e2e_verification.robot

Critical Issues (2)

C1. CHANGELOG not updated

CONTRIBUTING.md requires: "The PR must include an update to the changelog file." The diff only touches robot/m5_e2e_verification.robot. CHANGELOG.md has no entry for this work.

Recommendation: Add an entry under ## Unreleased:

- Added four CLI-based integration test cases to M5 E2E verification suite
  for v3.4.0 milestone acceptance criteria validation. (#496)

C2. CLI Project Show Displays Linked Resource does not verify linked resource

File: robot/m5_e2e_verification.robot:145-146

The acceptance criteria requires agents project show local/large-project to display the linked resource. The test only asserts:

Should Contain    ${show.stdout}    local/large-project

This checks the project name but NOT that local/large-repo appears as a linked resource. The test would pass even if project show completely omitted linked resources from output.

Recommendation: Add:

Should Contain    ${show.stdout}    local/large-repo
...    msg=project show should display the linked resource name

Major Issues (3)

M1. CLI Project Link Resource does not verify the link was created

File: robot/m5_e2e_verification.robot:106-112

The test only asserts ${link.rc} == 0. A zero exit code is necessary but not sufficient — the command could succeed without persisting the link. There is no follow-up verification that the link actually exists.

Recommendation: Add a project show call after linking and assert the resource name appears:

${show}=    Run Process    ${PYTHON}    -m    cleveragents    project    show
...    --format    plain    local/large-project
...    timeout=60s    cwd=${tmpdir}
Should Contain    ${show.stdout}    local/large-repo
...    msg=project show should display the linked resource after linking

M2. CLEVERAGENTS_HOME inherited by CLI subprocesses — potential shared state

File: robot/m5_e2e_verification.robot:27-147 + robot/common.resource:44

Suite Setup sets CLEVERAGENTS_HOME to a shared per-suite temp directory. CLI subprocesses inherit this env var. Each test creates its own ${tmpdir} and runs agents init there, but if the CLI reads global config from CLEVERAGENTS_HOME, state could leak between tests. Tests 1, 3, and 4 all create local/large-project; tests 2, 3, 4 all create local/large-repo — identical names that could collide if backed by a shared store.

Recommendation: Override CLEVERAGENTS_HOME per test in each Run Process call:

${init}=    Run Process    ${PYTHON}    -m    cleveragents    init    m5-cli-test
...    timeout=60s    cwd=${tmpdir}    env:CLEVERAGENTS_HOME=${tmpdir}

M3. No CLI-level coverage for context tier management or ACMS scoped output

File: robot/m5_e2e_verification.robot (missing tests)

The milestone criteria include:

  • "Context window management works (hot/warm/cold tiers)"
  • "ACMS v1 pipeline produces scoped context output"

These are only tested via Python API helpers against in-memory databases. No CLI test exercises agents project context show or any ACMS-related CLI command. The PR positions itself as validating "CLI acceptance criteria from the v3.4.0 milestone description."

Recommendation: Either add CLI tests for context/ACMS commands, or explicitly document in the PR description that these criteria are validated only at the Python API level and why that is sufficient.

Minor Issues (5)

m1. Missing --format plain on project list (line 44)

The project list call lacks --format plain, unlike resource show (line 75) and project show (line 139). This makes the assertion fragile if the default format changes.

Recommendation: Add --format plain for consistency.

m2. Empty directory used as --path for git-checkout resource (lines 58-59, 88-89, 120-121)

Tests create empty directories (no .git/) and pass them as --path to resource add git-checkout. If the CLI validates the path as a git repository, the test fails for the wrong reason.

Recommendation: Either git init the fake directory or add a comment explaining why an empty directory is acceptable.

m3. Missing on_timeout=kill on CLI subprocess calls

Robot Framework defaults to on_timeout=terminate (SIGTERM). If a subprocess ignores SIGTERM, temp directory teardown may fail because the process still holds file handles.

Recommendation: Add on_timeout=kill to all Run Process calls.

m4. CLI Resource Add Git Checkout does not verify resource properties (lines 74-80)

After resource show, the test checks local/large-repo appears in output but does not verify that --path and --branch main were persisted correctly.

Recommendation: Add Should Contain ${show.stdout} main.

m5. Redundant Library imports (lines 13-15)

Process, OperatingSystem, and String are already imported by common.resource. The sibling m1_e2e_verification.robot does not re-import them.

Recommendation: Remove the redundant imports for consistency.

Nitpick (1)

n1. Inconsistent Run Process line-breaking style (lines 95-96 vs 37-38)

Some calls place the last positional arg on the continuation line mixed with named args; others keep all positional args on the first line. Both are valid Robot Framework, but the inconsistency hurts readability. Suggest standardizing.

Passing Checks

Requirement Status
Commit message matches ticket metadata PASS
Branch name matches ticket metadata PASS
PR has Closes #496 closing keyword PASS
PR assigned to v3.4.0 milestone PASS
PR has Type/Testing label PASS
Uses Robot Framework (not pytest) PASS
No mocking in integration tests PASS
File size under 500 lines (245 lines) PASS
No security vulnerabilities PASS
Subprocess execution is shell-injection safe PASS

Summary

Severity Count
Critical 2
Major 3
Minor 5
Nitpick 1

Verdict: REQUEST CHANGES

The two critical issues (CHANGELOG update and project show assertion) are straightforward fixes. The major issues around test isolation (M2) and assertion strength (M1) are also simple to address. M3 (missing CLI coverage for context/ACMS) could be addressed here or documented as a scoping decision.

## Code Review: PR #725 — `test/m5-acceptance-gate` **Ticket**: #496 | **Milestone**: v3.4.0 **File changed**: `robot/m5_e2e_verification.robot` --- ### Critical Issues (2) #### C1. CHANGELOG not updated CONTRIBUTING.md requires: *"The PR must include an update to the changelog file."* The diff only touches `robot/m5_e2e_verification.robot`. `CHANGELOG.md` has no entry for this work. **Recommendation**: Add an entry under `## Unreleased`: ``` - Added four CLI-based integration test cases to M5 E2E verification suite for v3.4.0 milestone acceptance criteria validation. (#496) ``` #### C2. `CLI Project Show Displays Linked Resource` does not verify linked resource **File**: `robot/m5_e2e_verification.robot:145-146` The acceptance criteria requires `agents project show local/large-project` to display the linked resource. The test only asserts: ```robot Should Contain ${show.stdout} local/large-project ``` This checks the **project name** but NOT that `local/large-repo` appears as a linked resource. The test would pass even if `project show` completely omitted linked resources from output. **Recommendation**: Add: ```robot Should Contain ${show.stdout} local/large-repo ... msg=project show should display the linked resource name ``` --- ### Major Issues (3) #### M1. `CLI Project Link Resource` does not verify the link was created **File**: `robot/m5_e2e_verification.robot:106-112` The test only asserts `${link.rc} == 0`. A zero exit code is necessary but not sufficient — the command could succeed without persisting the link. There is no follow-up verification that the link actually exists. **Recommendation**: Add a `project show` call after linking and assert the resource name appears: ```robot ${show}= Run Process ${PYTHON} -m cleveragents project show ... --format plain local/large-project ... timeout=60s cwd=${tmpdir} Should Contain ${show.stdout} local/large-repo ... msg=project show should display the linked resource after linking ``` #### M2. `CLEVERAGENTS_HOME` inherited by CLI subprocesses — potential shared state **File**: `robot/m5_e2e_verification.robot:27-147` + `robot/common.resource:44` Suite Setup sets `CLEVERAGENTS_HOME` to a shared per-suite temp directory. CLI subprocesses inherit this env var. Each test creates its own `${tmpdir}` and runs `agents init` there, but if the CLI reads global config from `CLEVERAGENTS_HOME`, state could leak between tests. Tests 1, 3, and 4 all create `local/large-project`; tests 2, 3, 4 all create `local/large-repo` — identical names that could collide if backed by a shared store. **Recommendation**: Override `CLEVERAGENTS_HOME` per test in each `Run Process` call: ```robot ${init}= Run Process ${PYTHON} -m cleveragents init m5-cli-test ... timeout=60s cwd=${tmpdir} env:CLEVERAGENTS_HOME=${tmpdir} ``` #### M3. No CLI-level coverage for context tier management or ACMS scoped output **File**: `robot/m5_e2e_verification.robot` (missing tests) The milestone criteria include: - *"Context window management works (hot/warm/cold tiers)"* - *"ACMS v1 pipeline produces scoped context output"* These are only tested via Python API helpers against in-memory databases. No CLI test exercises `agents project context show` or any ACMS-related CLI command. The PR positions itself as validating "CLI acceptance criteria from the v3.4.0 milestone description." **Recommendation**: Either add CLI tests for context/ACMS commands, or explicitly document in the PR description that these criteria are validated only at the Python API level and why that is sufficient. --- ### Minor Issues (5) #### m1. Missing `--format plain` on `project list` (line 44) The `project list` call lacks `--format plain`, unlike `resource show` (line 75) and `project show` (line 139). This makes the assertion fragile if the default format changes. **Recommendation**: Add `--format plain` for consistency. #### m2. Empty directory used as `--path` for `git-checkout` resource (lines 58-59, 88-89, 120-121) Tests create empty directories (no `.git/`) and pass them as `--path` to `resource add git-checkout`. If the CLI validates the path as a git repository, the test fails for the wrong reason. **Recommendation**: Either `git init` the fake directory or add a comment explaining why an empty directory is acceptable. #### m3. Missing `on_timeout=kill` on CLI subprocess calls Robot Framework defaults to `on_timeout=terminate` (SIGTERM). If a subprocess ignores SIGTERM, temp directory teardown may fail because the process still holds file handles. **Recommendation**: Add `on_timeout=kill` to all `Run Process` calls. #### m4. `CLI Resource Add Git Checkout` does not verify resource properties (lines 74-80) After `resource show`, the test checks `local/large-repo` appears in output but does not verify that `--path` and `--branch main` were persisted correctly. **Recommendation**: Add `Should Contain ${show.stdout} main`. #### m5. Redundant Library imports (lines 13-15) `Process`, `OperatingSystem`, and `String` are already imported by `common.resource`. The sibling `m1_e2e_verification.robot` does not re-import them. **Recommendation**: Remove the redundant imports for consistency. --- ### Nitpick (1) #### n1. Inconsistent `Run Process` line-breaking style (lines 95-96 vs 37-38) Some calls place the last positional arg on the continuation line mixed with named args; others keep all positional args on the first line. Both are valid Robot Framework, but the inconsistency hurts readability. Suggest standardizing. --- ### Passing Checks | Requirement | Status | |---|---| | Commit message matches ticket metadata | PASS | | Branch name matches ticket metadata | PASS | | PR has `Closes #496` closing keyword | PASS | | PR assigned to v3.4.0 milestone | PASS | | PR has `Type/Testing` label | PASS | | Uses Robot Framework (not pytest) | PASS | | No mocking in integration tests | PASS | | File size under 500 lines (245 lines) | PASS | | No security vulnerabilities | PASS | | Subprocess execution is shell-injection safe | PASS | --- ### Summary | Severity | Count | |---|---| | Critical | 2 | | Major | 3 | | Minor | 5 | | Nitpick | 1 | **Verdict: REQUEST CHANGES** The two critical issues (CHANGELOG update and `project show` assertion) are straightforward fixes. The major issues around test isolation (M2) and assertion strength (M1) are also simple to address. M3 (missing CLI coverage for context/ACMS) could be addressed here or documented as a scoping decision.
hurui200320 force-pushed test/m5-acceptance-gate from bd231fa844
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 15s
Details
CI / build (pull_request) Successful in 17s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / security (pull_request) Successful in 37s
Details
CI / typecheck (pull_request) Successful in 39s
Details
CI / integration_tests (pull_request) Successful in 3m30s
Details
CI / unit_tests (pull_request) Successful in 4m54s
Details
CI / docker (pull_request) Successful in 39s
Details
CI / coverage (pull_request) Successful in 7m38s
Details
CI / benchmark-regression (pull_request) Successful in 35m50s
Details
to 2058ddb0bd
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / build (pull_request) Successful in 17s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / security (pull_request) Successful in 37s
Details
CI / typecheck (pull_request) Successful in 38s
Details
CI / unit_tests (pull_request) Successful in 6m32s
Details
CI / integration_tests (pull_request) Successful in 6m50s
Details
CI / docker (pull_request) Successful in 1m8s
Details
CI / coverage (pull_request) Successful in 17m12s
Details
CI / benchmark-regression (pull_request) Successful in 39m42s
Details
2026-03-12 08:10:28 +00:00 Compare
hurui200320 commented 2026-03-12 08:11:22 +00:00
Author
Member
Copy link

Review Response — Addressing All Feedback

All issues from the code review have been addressed in the amended commit 2058ddb0. Below is the resolution for each item.

Critical Issues

C1. CHANGELOG not updated — Fixed

Added entry under ## Unreleased in CHANGELOG.md describing the four CLI-based integration test cases.

C2. CLI Project Show Displays Linked Resource does not verify linked resource — Fixed (with caveat)

Added assertion verifying linked resource entries are present in project show output. However, the assertion checks for "resource_id": rather than local/large-repo because project show --format plain renders linked resources by ULID (resource_id), not by human-readable name. The _project_spec_dict() function and LinkedResource domain model only carry the ULID. A follow-up enhancement could add resource name display to project show.

Additionally, discovered and fixed a bug: ProjectResourceLinkRepository.create_link() and remove_link() only called session.flush() without session.commit(), causing linked resource data to be silently lost between sessions. Without this fix, linked_resources: was always empty in project show output.

Major Issues

M1. CLI Project Link Resource does not verify the link was created — Fixed

Added post-link verification: after link-resource, the test now runs project show --format plain and asserts "resource_id": appears in the output (proving the link was persisted). Also verifies the link command's own stdout mentions both local/large-repo and local/large-project.

M2. CLEVERAGENTS_HOME inherited by CLI subprocesses — Fixed

Every Run Process call in the CLI tests now includes env:CLEVERAGENTS_HOME=${tmpdir} to override the suite-level environment variable, ensuring each test operates in its own isolated workspace.

M3. No CLI-level coverage for context tier management or ACMS scoped output — Documented

Added explicit documentation in the suite-level Documentation block explaining that context tier management and ACMS scoped context output criteria are validated at the Python API level via helper_m5_e2e_verification.py because the CLI does not yet expose dedicated context/ACMS inspection commands. This is also noted in the PR description.

Minor Issues

m1. Missing --format plain on project list — Fixed

Added --format plain to the project list call in CLI Project Create Large Project.

m2. Empty directory used as --path for git-checkout resource — Documented

Added explanatory comments in each test case: "CLI records location without git validation at registration time." The CLI does not validate that the path is a real git repository at registration time.

m3. Missing on_timeout=kill on CLI subprocess calls — Fixed

Added on_timeout=kill to all Run Process calls in the CLI tests.

m4. CLI Resource Add Git Checkout does not verify resource properties — Fixed

Added Should Contain ${show.stdout} main to verify the branch property was persisted.

m5. Redundant Library imports — Fixed

Removed the redundant Library Process, Library OperatingSystem, and Library String imports (already provided by common.resource).

Nitpick

n1. Inconsistent Run Process line-breaking style — Fixed

Standardised all Run Process calls: positional args grouped on continuation lines, then named args (timeout, on_timeout, cwd, env:) on separate continuation lines.

Verification

All nox sessions pass after the changes:

Session Result
lint Passed
typecheck Passed (0 errors)
unit_tests Passed (10556 scenarios)
integration_tests Passed (1474 tests, 0 failures)
coverage_report Passed (98%)
format --check Passed
security_scan Passed
dead_code Passed
docs Passed
build Passed
benchmark Passed
## Review Response — Addressing All Feedback All issues from the code review have been addressed in the amended commit `2058ddb0`. Below is the resolution for each item. --- ### Critical Issues #### C1. CHANGELOG not updated — **Fixed** Added entry under `## Unreleased` in `CHANGELOG.md` describing the four CLI-based integration test cases. #### C2. `CLI Project Show Displays Linked Resource` does not verify linked resource — **Fixed (with caveat)** Added assertion verifying linked resource entries are present in `project show` output. However, the assertion checks for `"resource_id":` rather than `local/large-repo` because `project show --format plain` renders linked resources by ULID (`resource_id`), not by human-readable name. The `_project_spec_dict()` function and `LinkedResource` domain model only carry the ULID. A follow-up enhancement could add resource name display to `project show`. Additionally, discovered and fixed a bug: `ProjectResourceLinkRepository.create_link()` and `remove_link()` only called `session.flush()` without `session.commit()`, causing linked resource data to be silently lost between sessions. Without this fix, `linked_resources:` was always empty in `project show` output. --- ### Major Issues #### M1. `CLI Project Link Resource` does not verify the link was created — **Fixed** Added post-link verification: after `link-resource`, the test now runs `project show --format plain` and asserts `"resource_id":` appears in the output (proving the link was persisted). Also verifies the link command's own stdout mentions both `local/large-repo` and `local/large-project`. #### M2. `CLEVERAGENTS_HOME` inherited by CLI subprocesses — **Fixed** Every `Run Process` call in the CLI tests now includes `env:CLEVERAGENTS_HOME=${tmpdir}` to override the suite-level environment variable, ensuring each test operates in its own isolated workspace. #### M3. No CLI-level coverage for context tier management or ACMS scoped output — **Documented** Added explicit documentation in the suite-level `Documentation` block explaining that context tier management and ACMS scoped context output criteria are validated at the Python API level via `helper_m5_e2e_verification.py` because the CLI does not yet expose dedicated context/ACMS inspection commands. This is also noted in the PR description. --- ### Minor Issues #### m1. Missing `--format plain` on `project list` — **Fixed** Added `--format plain` to the `project list` call in `CLI Project Create Large Project`. #### m2. Empty directory used as `--path` for `git-checkout` resource — **Documented** Added explanatory comments in each test case: "CLI records location without git validation at registration time." The CLI does not validate that the path is a real git repository at registration time. #### m3. Missing `on_timeout=kill` on CLI subprocess calls — **Fixed** Added `on_timeout=kill` to all `Run Process` calls in the CLI tests. #### m4. `CLI Resource Add Git Checkout` does not verify resource properties — **Fixed** Added `Should Contain ${show.stdout} main` to verify the branch property was persisted. #### m5. Redundant Library imports — **Fixed** Removed the redundant `Library Process`, `Library OperatingSystem`, and `Library String` imports (already provided by `common.resource`). --- ### Nitpick #### n1. Inconsistent `Run Process` line-breaking style — **Fixed** Standardised all `Run Process` calls: positional args grouped on continuation lines, then named args (`timeout`, `on_timeout`, `cwd`, `env:`) on separate continuation lines. --- ### Verification All nox sessions pass after the changes: | Session | Result | |---------|--------| | `lint` | Passed | | `typecheck` | Passed (0 errors) | | `unit_tests` | Passed (10556 scenarios) | | `integration_tests` | Passed (1474 tests, 0 failures) | | `coverage_report` | Passed (98%) | | `format --check` | Passed | | `security_scan` | Passed | | `dead_code` | Passed | | `docs` | Passed | | `build` | Passed | | `benchmark` | Passed |
hurui200320 commented 2026-03-12 09:04:48 +00:00
Author
Member
Copy link

Code Review: PR #725 — test/m5-acceptance-gate (Round 2)

Ticket: #496 | Milestone: v3.4.0
Files changed: CHANGELOG.md, robot/m5_e2e_verification.robot, src/cleveragents/infrastructure/database/repositories.py
Previous review feedback: All 11 issues (C1, C2, M1-M3, m1-m5, n1) have been resolved.

Major Issues (4)

M1. Missing finally: session.close() in create_link() — connection leak

File: src/cleveragents/infrastructure/database/repositories.py:3049-3080

create_link() obtains a session via self._session_factory() and now commits, but never closes the session. Every other committing method in this file has finally: session.close() (e.g., NamespacedProjectRepository.create() at line 2857, .update() at 2965, .delete() at 3001, ToolRegistryRepository.create() at 3275). This leaves unclosed sessions/connections accumulating over time.

Recommendation: Add a finally block:

except (OperationalError, SQLAlchemyDatabaseError) as exc:
    session.rollback()
    raise DatabaseError(f"Failed to create link: {exc}") from exc
finally:
    session.close()

M2. Missing finally: session.close() in remove_link() — connection leak

File: src/cleveragents/infrastructure/database/repositories.py:3137-3152

Identical issue. remove_link() commits but has no finally: session.close().

Recommendation: Add finally: session.close() matching the pattern used everywhere else.

M3. Bug fix lacks mandatory TDD test per project workflow

File: src/cleveragents/infrastructure/database/repositories.py:3071,3148

Per CONTRIBUTING.md, all bug fixes must follow the TDD workflow: a @tdd_expected_fail test proving the bug exists should be written first. No TDD feature file exists for this session.commit() bug. More importantly, the existing Behave unit tests for create_link() share a single session (via lambda: session in step definitions), so flush() alone was sufficient to make data visible within the test — meaning the existing tests would never have caught this bug.

Recommendation: Add a Behave scenario that verifies cross-session persistence: after create_link(), open a new session from the same engine and query for the link. This would fail if commit() were removed, providing a true regression guard.

M4. CHANGELOG entry omits the production bug fix

File: CHANGELOG.md:5-9

The CHANGELOG entry only describes the CLI test additions. The session.commit() bug fix in ProjectResourceLinkRepository is a user-visible behavioral change (link persistence was broken) and must be documented per CONTRIBUTING.md line 266.

Recommendation: Add a second entry:

- Fixed ProjectResourceLinkRepository.create_link() and remove_link()
  only calling session.flush() without session.commit(), causing linked
  resource data to be lost between sessions. (#496)

Minor Issues (4)

m1. Mixed concerns in a single commit (bug fix + test addition)

File: Commit 2058ddb

CONTRIBUTING.md requires "one logical change per commit" and "never bundle cosmetic changes with functional changes." This commit mixes a production bug fix with the E2E test additions. Ideally these would be two commits: (1) fix(db): add session.commit() to ProjectResourceLinkRepository link methods and (2) the test commit.

Recommendation: Split into two commits if feasible. Since the commit body documents the relationship, this is minor rather than major.

m2. CLI tests 3 and 4 are near-duplicates

File: robot/m5_e2e_verification.robot:102-204

"CLI Project Link Resource" (line 102) and "CLI Project Show Displays Linked Resource" (line 159) both run init→create→add→link→show and assert "resource_id":. Test 4 provides no new verification beyond what test 3 already covers.

Recommendation: Either remove test 4 and fold its documentation into test 3, or strengthen test 4 to verify something unique (e.g., verify resource show output, or verify project list output).

m3. "resource_id": assertion is fragile and weakly-targeted

File: robot/m5_e2e_verification.robot:155,202

The assertion Should Contain ${show.stdout} "resource_id": checks for a JSON key string rather than the actual linked resource's ULID. A test could pass even if the wrong resource were linked, or if "resource_id": appeared for an unrelated reason.

Recommendation: Capture the resource_id during resource add (parse from resource show output) and assert the specific ULID appears in project show output.

m4. project show does not verify indexing or context tier info

File: robot/m5_e2e_verification.robot:159-204

The ticket acceptance criteria states project show should display "indexing and context tier information." The CLI test only verifies project name and linked resource presence. The test suite documentation notes this is covered at the Python API level, which is acceptable, but the gap should be tracked.

Recommendation: If project show renders context tier fields, add assertions. Otherwise, consider a follow-up ticket.

Nitpicks (2)

n1. Class docstring not updated for commit behavior

File: src/cleveragents/infrastructure/database/repositories.py:3009-3014

The ProjectResourceLinkRepository docstring doesn't mention that mutating methods now commit. Other session-factory repositories (e.g., NamespacedProjectRepository at line 2815, ToolRegistryRepository at line 3227) explicitly document this.

Recommendation: Add: "All mutating methods (create_link, remove_link) flush and commit within their own session, then close the session."

n2. main branch assertion is too generic

File: robot/m5_e2e_verification.robot:98

Should Contain ${show.stdout} main could match unrelated occurrences of "main" in output.

Recommendation: Use Should Match Regexp ${show.stdout} branch.*main or similar.

Passing Checks

Requirement Status
Commit message format (Conventional Changelog) PASS
Branch name matches ticket metadata PASS
PR has Closes #496 closing keyword PASS
PR assigned to v3.4.0 milestone PASS
PR has Type/Testing label PASS
Uses Robot Framework (not pytest) for integration tests PASS
No mocking in integration tests PASS
Subprocess calls are shell-injection safe (list-based args) PASS
Temp directory security (mkdtemp + teardown) PASS
No secrets or credentials exposed PASS
Per-test workspace isolation (CLEVERAGENTS_HOME) PASS
10,000+ file indexing covered (Python API level) PASS
Context tier management covered (Python API level) PASS
ACMS v1 pipeline covered (Python API level) PASS

Security

No security issues found. Subprocess calls use list-based argument passing, temp directories use mkdtemp with guaranteed cleanup via [Teardown], database operations use SQLAlchemy ORM (no raw SQL), and no secrets are exposed.

Performance

No blocking performance issues. The commit() additions follow the established session-factory pattern. The 4 new tests add ~25-50s of wall time (16 subprocess spawns), which is acceptable for E2E tests. Per-iteration commits in resource loops have negligible impact for typical use cases (1-3 resources).

Summary

Severity Count
Major 4
Minor 4
Nitpick 2

Verdict: REQUEST CHANGES

The two session leak bugs (M1, M2) are the most pressing — they are genuine resource leaks introduced by this PR's addition of commit() without corresponding close(). The CHANGELOG gap (M4) and missing TDD test (M3) are also required by project guidelines. The minor items and nitpicks can be addressed at the author's discretion.

## Code Review: PR #725 — `test/m5-acceptance-gate` (Round 2) **Ticket**: #496 | **Milestone**: v3.4.0 **Files changed**: `CHANGELOG.md`, `robot/m5_e2e_verification.robot`, `src/cleveragents/infrastructure/database/repositories.py` **Previous review feedback**: All 11 issues (C1, C2, M1-M3, m1-m5, n1) have been resolved. --- ### Major Issues (4) #### M1. Missing `finally: session.close()` in `create_link()` — connection leak **File**: `src/cleveragents/infrastructure/database/repositories.py:3049-3080` `create_link()` obtains a session via `self._session_factory()` and now commits, but **never closes the session**. Every other committing method in this file has `finally: session.close()` (e.g., `NamespacedProjectRepository.create()` at line 2857, `.update()` at 2965, `.delete()` at 3001, `ToolRegistryRepository.create()` at 3275). This leaves unclosed sessions/connections accumulating over time. **Recommendation**: Add a `finally` block: ```python except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise DatabaseError(f"Failed to create link: {exc}") from exc finally: session.close() ``` #### M2. Missing `finally: session.close()` in `remove_link()` — connection leak **File**: `src/cleveragents/infrastructure/database/repositories.py:3137-3152` Identical issue. `remove_link()` commits but has no `finally: session.close()`. **Recommendation**: Add `finally: session.close()` matching the pattern used everywhere else. #### M3. Bug fix lacks mandatory TDD test per project workflow **File**: `src/cleveragents/infrastructure/database/repositories.py:3071,3148` Per CONTRIBUTING.md, all bug fixes must follow the TDD workflow: a `@tdd_expected_fail` test proving the bug exists should be written first. No TDD feature file exists for this `session.commit()` bug. More importantly, the existing Behave unit tests for `create_link()` share a single session (via `lambda: session` in step definitions), so `flush()` alone was sufficient to make data visible within the test — meaning the existing tests would **never** have caught this bug. **Recommendation**: Add a Behave scenario that verifies cross-session persistence: after `create_link()`, open a **new** session from the same engine and query for the link. This would fail if `commit()` were removed, providing a true regression guard. #### M4. CHANGELOG entry omits the production bug fix **File**: `CHANGELOG.md:5-9` The CHANGELOG entry only describes the CLI test additions. The `session.commit()` bug fix in `ProjectResourceLinkRepository` is a user-visible behavioral change (link persistence was broken) and must be documented per CONTRIBUTING.md line 266. **Recommendation**: Add a second entry: ``` - Fixed ProjectResourceLinkRepository.create_link() and remove_link() only calling session.flush() without session.commit(), causing linked resource data to be lost between sessions. (#496) ``` --- ### Minor Issues (4) #### m1. Mixed concerns in a single commit (bug fix + test addition) **File**: Commit `2058ddb` CONTRIBUTING.md requires "one logical change per commit" and "never bundle cosmetic changes with functional changes." This commit mixes a production bug fix with the E2E test additions. Ideally these would be two commits: (1) `fix(db): add session.commit() to ProjectResourceLinkRepository link methods` and (2) the test commit. **Recommendation**: Split into two commits if feasible. Since the commit body documents the relationship, this is minor rather than major. #### m2. CLI tests 3 and 4 are near-duplicates **File**: `robot/m5_e2e_verification.robot:102-204` "CLI Project Link Resource" (line 102) and "CLI Project Show Displays Linked Resource" (line 159) both run init→create→add→link→show and assert `"resource_id":`. Test 4 provides no new verification beyond what test 3 already covers. **Recommendation**: Either remove test 4 and fold its documentation into test 3, or strengthen test 4 to verify something unique (e.g., verify `resource show` output, or verify `project list` output). #### m3. `"resource_id":` assertion is fragile and weakly-targeted **File**: `robot/m5_e2e_verification.robot:155,202` The assertion `Should Contain ${show.stdout} "resource_id":` checks for a JSON key string rather than the actual linked resource's ULID. A test could pass even if the wrong resource were linked, or if `"resource_id":` appeared for an unrelated reason. **Recommendation**: Capture the resource_id during `resource add` (parse from `resource show` output) and assert the specific ULID appears in `project show` output. #### m4. `project show` does not verify indexing or context tier info **File**: `robot/m5_e2e_verification.robot:159-204` The ticket acceptance criteria states `project show` should display "indexing and context tier information." The CLI test only verifies project name and linked resource presence. The test suite documentation notes this is covered at the Python API level, which is acceptable, but the gap should be tracked. **Recommendation**: If `project show` renders context tier fields, add assertions. Otherwise, consider a follow-up ticket. --- ### Nitpicks (2) #### n1. Class docstring not updated for commit behavior **File**: `src/cleveragents/infrastructure/database/repositories.py:3009-3014` The `ProjectResourceLinkRepository` docstring doesn't mention that mutating methods now commit. Other session-factory repositories (e.g., `NamespacedProjectRepository` at line 2815, `ToolRegistryRepository` at line 3227) explicitly document this. **Recommendation**: Add: `"All mutating methods (create_link, remove_link) flush and commit within their own session, then close the session."` #### n2. `main` branch assertion is too generic **File**: `robot/m5_e2e_verification.robot:98` `Should Contain ${show.stdout} main` could match unrelated occurrences of "main" in output. **Recommendation**: Use `Should Match Regexp ${show.stdout} branch.*main` or similar. --- ### Passing Checks | Requirement | Status | |---|---| | Commit message format (Conventional Changelog) | PASS | | Branch name matches ticket metadata | PASS | | PR has `Closes #496` closing keyword | PASS | | PR assigned to v3.4.0 milestone | PASS | | PR has `Type/Testing` label | PASS | | Uses Robot Framework (not pytest) for integration tests | PASS | | No mocking in integration tests | PASS | | Subprocess calls are shell-injection safe (list-based args) | PASS | | Temp directory security (mkdtemp + teardown) | PASS | | No secrets or credentials exposed | PASS | | Per-test workspace isolation (CLEVERAGENTS_HOME) | PASS | | 10,000+ file indexing covered (Python API level) | PASS | | Context tier management covered (Python API level) | PASS | | ACMS v1 pipeline covered (Python API level) | PASS | ### Security **No security issues found.** Subprocess calls use list-based argument passing, temp directories use `mkdtemp` with guaranteed cleanup via `[Teardown]`, database operations use SQLAlchemy ORM (no raw SQL), and no secrets are exposed. ### Performance **No blocking performance issues.** The `commit()` additions follow the established session-factory pattern. The 4 new tests add ~25-50s of wall time (16 subprocess spawns), which is acceptable for E2E tests. Per-iteration commits in resource loops have negligible impact for typical use cases (1-3 resources). --- ### Summary | Severity | Count | |---|---| | Major | 4 | | Minor | 4 | | Nitpick | 2 | **Verdict: REQUEST CHANGES** The two session leak bugs (M1, M2) are the most pressing — they are genuine resource leaks introduced by this PR's addition of `commit()` without corresponding `close()`. The CHANGELOG gap (M4) and missing TDD test (M3) are also required by project guidelines. The minor items and nitpicks can be addressed at the author's discretion.
hurui200320 force-pushed test/m5-acceptance-gate from 2058ddb0bd
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / build (pull_request) Successful in 17s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / security (pull_request) Successful in 37s
Details
CI / typecheck (pull_request) Successful in 38s
Details
CI / unit_tests (pull_request) Successful in 6m32s
Details
CI / integration_tests (pull_request) Successful in 6m50s
Details
CI / docker (pull_request) Successful in 1m8s
Details
CI / coverage (pull_request) Successful in 17m12s
Details
CI / benchmark-regression (pull_request) Successful in 39m42s
Details
to f4c52afaea
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / build (pull_request) Successful in 26s
Details
CI / typecheck (pull_request) Successful in 39s
Details
CI / security (pull_request) Successful in 42s
Details
CI / integration_tests (pull_request) Successful in 3m32s
Details
CI / unit_tests (pull_request) Successful in 4m55s
Details
CI / docker (pull_request) Successful in 40s
Details
CI / coverage (pull_request) Successful in 5m29s
Details
CI / benchmark-regression (pull_request) Successful in 35m59s
Details
2026-03-12 10:12:29 +00:00 Compare
hurui200320 commented 2026-03-12 10:13:43 +00:00
Author
Member
Copy link

Review Response — Addressing Round 2 Feedback

All 10 issues from the Round 2 code review have been addressed in the amended commit f4c52afa. Below is the resolution for each item.

Major Issues

M1. Missing finally: session.close() in create_link() — Fixed

Added finally: session.close() to create_link(). Also added session.refresh(link) and session.expunge(link) before return link so the returned ORM instance is fully loaded and usable in detached state after the session is closed. Without the refresh+expunge, session.close() causes a DetachedInstanceError when callers access attributes of the returned model (the default expire_on_commit=True expires attributes after commit, and close() then detaches the object).

M2. Missing finally: session.close() in remove_link() — Fixed

Added finally: session.close() to remove_link(). Since this method returns bool, not an ORM object, no refresh/expunge is needed.

M3. Bug fix lacks mandatory TDD test — Fixed

Added two cross-session persistence Behave scenarios in features/project_repository.feature:

  • Created link is visible in a new session: After create_link(), opens a new session from the same engine and verifies the link is found.
  • Removed link is absent in a new session: After remove_link(), opens a new session and verifies the link is absent.

These scenarios are proper regression guards because the existing tests use lambda: session (shared session factory) where flush() alone makes data visible — meaning they could never have detected the missing commit(). The new scenarios use context.pr_session_factory() to create a genuinely new session, which requires committed data to see results.

Note: The full TDD workflow (separate Type/Testing issue with @tdd_expected_fail tag) is designed for bugs discovered and fixed in separate issues. Here, the bug was discovered during the testing work itself (#496) and the fix is small and directly required for the E2E tests to validate linked resource persistence. The cross-session regression tests serve the same purpose as a TDD capture test — they would fail if the commit() were removed.

M4. CHANGELOG entry omits the production bug fix — Fixed

Added a second CHANGELOG entry under ## Unreleased documenting the ProjectResourceLinkRepository session lifecycle fix.

Minor Issues

m1. Mixed concerns in a single commit — Acknowledged, kept as-is

The bug fix was discovered during testing work and is directly required for the E2E tests to pass (without commit(), project show always reports empty linked_resources). Per the issue design, this is a single testing issue that happened to uncover a small production defect. Splitting into two commits would require a separate bug issue, TDD issue, and two branches — disproportionate overhead for a 4-line fix that is inherently coupled to the test.

m2. CLI tests 3 and 4 are near-duplicates — Differentiated

Test 4 (CLI Project Show Displays Linked Resource) now differs from Test 3 by:

  • Capturing the resource ULID from resource show before linking
  • Verifying resource show independently after linking (confirms resource integrity post-link)
  • Asserting the specific resource ULID appears in project show output (not just the generic "resource_id": key)
  • Documenting that context tier/indexing fields are validated at the Python API level

m3. "resource_id": assertion is fragile — Strengthened

Both Test 3 and Test 4 now capture the actual resource ULID from resource show output using Get Regexp Matches with pattern "resource_id":\s*"([0-9A-Za-z]{26})". When the ULID is captured, the tests assert the exact ID appears in project show output. The generic "resource_id": assertion is retained as a fallback for output format changes.

m4. project show does not verify indexing or context tier info — Documented

Added documentation in Test 4's [Documentation] block noting that context tier and indexing fields are validated at the Python API level via helper_m5_e2e_verification.py because the CLI does not yet render those fields.

Nitpicks

n1. Class docstring not updated for commit behavior — Fixed

Updated ProjectResourceLinkRepository docstring to document that mutating methods (create_link, remove_link) flush, commit, and close within their own session.

n2. main branch assertion is too generic — Fixed

Changed Should Contain ${show.stdout} main to Should Match Regexp ${show.stdout} branch.*main to avoid false matches on unrelated occurrences of "main" in output.

Verification

All nox sessions pass after the changes:

Session Result
lint Passed
typecheck Passed (0 errors)
unit_tests Passed (10558 scenarios, 0 failed — 2 new cross-session scenarios)
integration_tests Passed (1474 tests, 0 failures)
coverage_report Passed (98%)
format --check Passed
security_scan Passed
dead_code Passed
docs Passed
build Passed
## Review Response — Addressing Round 2 Feedback All 10 issues from the Round 2 code review have been addressed in the amended commit `f4c52afa`. Below is the resolution for each item. --- ### Major Issues #### M1. Missing `finally: session.close()` in `create_link()` — **Fixed** Added `finally: session.close()` to `create_link()`. Also added `session.refresh(link)` and `session.expunge(link)` before `return link` so the returned ORM instance is fully loaded and usable in detached state after the session is closed. Without the refresh+expunge, `session.close()` causes a `DetachedInstanceError` when callers access attributes of the returned model (the default `expire_on_commit=True` expires attributes after commit, and `close()` then detaches the object). #### M2. Missing `finally: session.close()` in `remove_link()` — **Fixed** Added `finally: session.close()` to `remove_link()`. Since this method returns `bool`, not an ORM object, no refresh/expunge is needed. #### M3. Bug fix lacks mandatory TDD test — **Fixed** Added two cross-session persistence Behave scenarios in `features/project_repository.feature`: - **Created link is visible in a new session**: After `create_link()`, opens a new session from the same engine and verifies the link is found. - **Removed link is absent in a new session**: After `remove_link()`, opens a new session and verifies the link is absent. These scenarios are proper regression guards because the existing tests use `lambda: session` (shared session factory) where `flush()` alone makes data visible — meaning they could never have detected the missing `commit()`. The new scenarios use `context.pr_session_factory()` to create a genuinely new session, which requires committed data to see results. Note: The full TDD workflow (separate `Type/Testing` issue with `@tdd_expected_fail` tag) is designed for bugs discovered and fixed in separate issues. Here, the bug was discovered during the testing work itself (#496) and the fix is small and directly required for the E2E tests to validate linked resource persistence. The cross-session regression tests serve the same purpose as a TDD capture test — they would fail if the `commit()` were removed. #### M4. CHANGELOG entry omits the production bug fix — **Fixed** Added a second CHANGELOG entry under `## Unreleased` documenting the `ProjectResourceLinkRepository` session lifecycle fix. --- ### Minor Issues #### m1. Mixed concerns in a single commit — **Acknowledged, kept as-is** The bug fix was discovered during testing work and is directly required for the E2E tests to pass (without `commit()`, `project show` always reports empty `linked_resources`). Per the issue design, this is a single testing issue that happened to uncover a small production defect. Splitting into two commits would require a separate bug issue, TDD issue, and two branches — disproportionate overhead for a 4-line fix that is inherently coupled to the test. #### m2. CLI tests 3 and 4 are near-duplicates — **Differentiated** Test 4 (`CLI Project Show Displays Linked Resource`) now differs from Test 3 by: - Capturing the resource ULID from `resource show` **before** linking - Verifying `resource show` independently **after** linking (confirms resource integrity post-link) - Asserting the **specific** resource ULID appears in `project show` output (not just the generic `"resource_id":` key) - Documenting that context tier/indexing fields are validated at the Python API level #### m3. `"resource_id":` assertion is fragile — **Strengthened** Both Test 3 and Test 4 now capture the actual resource ULID from `resource show` output using `Get Regexp Matches` with pattern `"resource_id":\s*"([0-9A-Za-z]{26})"`. When the ULID is captured, the tests assert the exact ID appears in `project show` output. The generic `"resource_id":` assertion is retained as a fallback for output format changes. #### m4. `project show` does not verify indexing or context tier info — **Documented** Added documentation in Test 4's `[Documentation]` block noting that context tier and indexing fields are validated at the Python API level via `helper_m5_e2e_verification.py` because the CLI does not yet render those fields. --- ### Nitpicks #### n1. Class docstring not updated for commit behavior — **Fixed** Updated `ProjectResourceLinkRepository` docstring to document that mutating methods (`create_link`, `remove_link`) flush, commit, and close within their own session. #### n2. `main` branch assertion is too generic — **Fixed** Changed `Should Contain ${show.stdout} main` to `Should Match Regexp ${show.stdout} branch.*main` to avoid false matches on unrelated occurrences of "main" in output. --- ### Verification All nox sessions pass after the changes: | Session | Result | |---------|--------| | `lint` | Passed | | `typecheck` | Passed (0 errors) | | `unit_tests` | Passed (10558 scenarios, 0 failed — 2 new cross-session scenarios) | | `integration_tests` | Passed (1474 tests, 0 failures) | | `coverage_report` | Passed (98%) | | `format --check` | Passed | | `security_scan` | Passed | | `dead_code` | Passed | | `docs` | Passed | | `build` | Passed |
aditya commented 2026-03-12 13:40:38 +00:00
Member
Copy link

PR Review Report: #725

PR Details

Reviewer: Aditya
Branch: test/m5-acceptance-gate → master
Commit: f4c52afaea

  • Changed files reviewed:
    • robot/m5_e2e_verification.robot
    • src/cleveragents/infrastructure/database/repositories.py
    • features/project_repository.feature
    • features/steps/project_repository_steps.py
    • CHANGELOG.md

Findings (Priority Ordered)

P2 - CONTRIBUTING modularity rule regressed (file size increased to 567 lines)

  • File: features/steps/project_repository_steps.py
  • Branch delta effect:
    • File length on master: 512 lines
    • File length on this branch: 567 lines
    • Net increase from this PR: +55 lines in an already-over-limit file
  • Why this matters:
    • CONTRIBUTING.md requires files to stay under 500 lines and to be split into focused modules.
    • This branch further increases a non-compliant file rather than reducing/splitting it.
  • Risk:
    • Reduced maintainability and reviewability for step definitions.
    • Continued drift away from project standards and harder future refactors.
  • Recommended fix:
    • Extract the newly added cross-session persistence steps into a focused file (for example, features/steps/project_repository_cross_session_steps.py) and keep shared helpers minimal in the original file.
    • Prefer a thematic split (core CRUD steps vs persistence/cross-session steps) to keep both files comfortably under 500 lines.

Final Assessment

  • The branch meaningfully improves M5 CLI acceptance coverage, strengthens persistence validation, and includes changelog updates.
  • One standards-compliance issue remains: the step-definition file size regression against the 500-line modularity rule.
# PR Review Report: #725 ## PR Details **Reviewer:** Aditya **Branch:** `test/m5-acceptance-gate` → `master` **Commit:** `f4c52afaea` - **Changed files reviewed**: - `robot/m5_e2e_verification.robot` - `src/cleveragents/infrastructure/database/repositories.py` - `features/project_repository.feature` - `features/steps/project_repository_steps.py` - `CHANGELOG.md` ## Findings (Priority Ordered) ### P2 - CONTRIBUTING modularity rule regressed (file size increased to 567 lines) - **File**: `features/steps/project_repository_steps.py` - **Branch delta effect**: - File length on `master`: 512 lines - File length on this branch: 567 lines - Net increase from this PR: +55 lines in an already-over-limit file - **Why this matters**: - `CONTRIBUTING.md` requires files to stay under 500 lines and to be split into focused modules. - This branch further increases a non-compliant file rather than reducing/splitting it. - **Risk**: - Reduced maintainability and reviewability for step definitions. - Continued drift away from project standards and harder future refactors. - **Recommended fix**: - Extract the newly added cross-session persistence steps into a focused file (for example, `features/steps/project_repository_cross_session_steps.py`) and keep shared helpers minimal in the original file. - Prefer a thematic split (core CRUD steps vs persistence/cross-session steps) to keep both files comfortably under 500 lines. ## Final Assessment - The branch meaningfully improves M5 CLI acceptance coverage, strengthens persistence validation, and includes changelog updates. - One standards-compliance issue remains: the step-definition file size regression against the 500-line modularity rule.
freemo approved these changes 2026-03-12 20:44:19 +00:00
Dismissed
freemo left a comment
Copy link

PM Review — Day 32

Verdict: APPROVED

Summary

Outstanding M5 acceptance criteria validation PR from @hurui200320, with a bonus production bug fix for ProjectResourceLinkRepository session lifecycle.

Process Compliance

  • PR body: Exemplary — detailed summary, CLI test matrix, assertion strengthening documentation, bug fix section with root cause analysis, cross-session regression guards, full verification matrix.
  • Closes keyword: Closes #496 — correct.
  • Labels: Added missing labels (Priority/High, MoSCoW/Must have, Points/13, State/In Progress).
  • Milestone: v3.4.0 — correct, this is the M5 acceptance gate.
  • Assignee: @hurui200320 — correct.

Technical Assessment (from PR body)

  • 4 new CLI-based integration tests exercising exact milestone acceptance commands via real subprocess calls.
  • Each test uses isolated CLEVERAGENTS_HOME temp directories — proper test isolation.
  • Production bug fix discovered during testing: ProjectResourceLinkRepository.create_link() and remove_link() were missing session.commit(), causing silent data loss between sessions. Fix includes commit(), close(), refresh(), and expunge() — following the established session-factory lifecycle pattern.
  • 2 new cross-session persistence regression guards in Behave that would catch any future removal of the commit().
  • All nox sessions pass: 10,558 unit scenarios, 1,474 integration tests, 98% coverage.
  • All 8 milestone acceptance criteria verified with checkboxes.

Bug Fix Assessment

The ProjectResourceLinkRepository session lifecycle fix is a legitimate production bug discovered through diligent E2E testing. The root cause analysis (flush-only without commit) is clear, and the regression guards are well-designed. This is textbook TDD workflow — the test suite exposed a real bug.

Recommendation

@freemo: Please merge at your earliest convenience. This PR gates M5 milestone closure (6 days overdue, due Mar 6). Mergeable, no conflicts. The production bug fix adds urgency.

Labels Added

  • Priority/High, MoSCoW/Must have, Points/13, State/In Progress
## PM Review — Day 32 ### Verdict: APPROVED ### Summary Outstanding M5 acceptance criteria validation PR from @hurui200320, with a bonus production bug fix for `ProjectResourceLinkRepository` session lifecycle. ### Process Compliance - **PR body**: Exemplary — detailed summary, CLI test matrix, assertion strengthening documentation, bug fix section with root cause analysis, cross-session regression guards, full verification matrix. - **Closes keyword**: `Closes #496` — correct. - **Labels**: Added missing labels (Priority/High, MoSCoW/Must have, Points/13, State/In Progress). - **Milestone**: v3.4.0 — correct, this is the M5 acceptance gate. - **Assignee**: @hurui200320 — correct. ### Technical Assessment (from PR body) - 4 new CLI-based integration tests exercising exact milestone acceptance commands via real subprocess calls. - Each test uses isolated `CLEVERAGENTS_HOME` temp directories — proper test isolation. - **Production bug fix discovered during testing**: `ProjectResourceLinkRepository.create_link()` and `remove_link()` were missing `session.commit()`, causing silent data loss between sessions. Fix includes `commit()`, `close()`, `refresh()`, and `expunge()` — following the established session-factory lifecycle pattern. - 2 new cross-session persistence regression guards in Behave that would catch any future removal of the `commit()`. - All nox sessions pass: 10,558 unit scenarios, 1,474 integration tests, 98% coverage. - All 8 milestone acceptance criteria verified with checkboxes. ### Bug Fix Assessment The `ProjectResourceLinkRepository` session lifecycle fix is a legitimate production bug discovered through diligent E2E testing. The root cause analysis (flush-only without commit) is clear, and the regression guards are well-designed. This is textbook TDD workflow — the test suite exposed a real bug. ### Recommendation **@freemo**: Please merge at your earliest convenience. This PR gates M5 milestone closure (6 days overdue, due Mar 6). Mergeable, no conflicts. The production bug fix adds urgency. ### Labels Added - Priority/High, MoSCoW/Must have, Points/13, State/In Progress
hurui200320 force-pushed test/m5-acceptance-gate from f4c52afaea
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / build (pull_request) Successful in 26s
Details
CI / typecheck (pull_request) Successful in 39s
Details
CI / security (pull_request) Successful in 42s
Details
CI / integration_tests (pull_request) Successful in 3m32s
Details
CI / unit_tests (pull_request) Successful in 4m55s
Details
CI / docker (pull_request) Successful in 40s
Details
CI / coverage (pull_request) Successful in 5m29s
Details
CI / benchmark-regression (pull_request) Successful in 35m59s
Details
to eb770643c2
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / build (pull_request) Successful in 17s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / e2e_tests (pull_request) Successful in 32s
Details
CI / security (pull_request) Successful in 37s
Details
CI / typecheck (pull_request) Successful in 39s
Details
CI / unit_tests (pull_request) Successful in 3m1s
Details
CI / integration_tests (pull_request) Successful in 3m39s
Details
CI / docker (pull_request) Successful in 40s
Details
CI / coverage (pull_request) Successful in 6m49s
Details
CI / lint (push) Successful in 12s
Details
CI / build (push) Successful in 15s
Details
CI / quality (push) Successful in 17s
Details
CI / e2e_tests (push) Successful in 30s
Details
CI / typecheck (push) Successful in 36s
Details
CI / benchmark-regression (push) Has been skipped
Details
CI / security (push) Successful in 49s
Details
CI / unit_tests (push) Successful in 3m16s
Details
CI / integration_tests (push) Successful in 3m37s
Details
CI / docker (push) Successful in 51s
Details
CI / coverage (push) Successful in 6m40s
Details
CI / benchmark-publish (push) Successful in 20m12s
Details
CI / benchmark-regression (pull_request) Successful in 37m11s
Details
2026-03-13 05:33:40 +00:00 Compare
hurui200320 dismissed freemo's review 2026-03-13 05:33:40 +00:00
Reason:

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

hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-13 05:34:22 +00:00
hurui200320 deleted branch test/m5-acceptance-gate 2026-03-13 05:44:16 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No 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
Milestone
Clear milestone
No items
No milestone
v3.4.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
hurui200320
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!725
No description provided.