cleveragents/cleveragents-core

Fork 3

`agents tool` does not register when tools are added. #621

New issue

Closed

opened 2026-03-07 01:39:20 +00:00 by brent.edwards · 3 comments

brent.edwards commented

2026-03-07 01:39:20 +00:00

Member

Metadata

Commit Message: fix(tool): persist tool registration to database after add
Branch: fix/tool-add-not-registering
Type: Bug
Priority: Critical
MoSCoW: Must have
Points: 3
Milestone: v3.2.0
Assignee: @freemo

Parent

Epic: #401 (E2E Integration Testing — bug discovered during M3 acceptance testing)

Background and Context

Bug reported by @brent.edwards on Day 27. After agents init and agents tool add -c /app/examples/tools/mcp-tool.yaml, running agents tool list shows "No tools found." The tool add command appears to succeed silently but the tool is not persisted/registered.

Investigation reveals that ToolRegistryRepository.create() in repositories.py calls session.flush() but never session.commit(). This is the exact same pattern as bug #589 (project persistence). The sessionmaker creates sessions with autocommit=False (default), so flush() sends SQL to the engine's transaction buffer but never persists it. When the session is garbage-collected, SQLAlchemy performs an implicit rollback. The same bug exists in update() and delete() methods.

The _get_tool_registry_service() factory in tool.py creates a raw sessionmaker without using a UnitOfWork, so nobody ever calls commit().

Reproduction Steps

cd ~
mkdir data
uv venv
source .venv/bin/activate
uv pip install -e /app
agents init
agents tool add -c /app/examples/tools/mcp-tool.yaml

Test ("When")

agents tool list

Expected

To see the tool among the list of tools.

Actual

(devuser) ➜  ~ agents tool list
2026-03-07 01:17:41 [debug    ] Starting attempt               attempt_number=1 outcome=None
No tools found.
Register one with 'agents tool add --config <file>'

Acceptance Criteria

ToolRegistryRepository.create() calls session.commit() after session.flush()
ToolRegistryRepository.update() calls session.commit() after session.flush()
ToolRegistryRepository.delete() calls session.commit() after session.flush()
All three methods have finally: session.close() guards
agents tool add followed by agents tool list in separate processes shows the tool
Existing tests pass (BDD, Robot, ASV)
New Behave BDD regression tests for tool add/list persistence
New Robot Framework integration smoke tests for tool lifecycle
New ASV benchmark for tool add/list round-trip

Subtasks

Fix ToolRegistryRepository.create() — add session.commit() + finally: session.close()
Fix ToolRegistryRepository.update() — add session.commit() + finally: session.close()
Fix ToolRegistryRepository.delete() — add session.commit() + finally: session.close()
Update class docstring to reflect commit-per-method pattern
Write Behave BDD regression scenarios for tool add/list persistence
Write Robot Framework integration smoke tests for tool lifecycle
Write ASV benchmark for tool add/list round-trip
Verify coverage >=97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `fix(tool): persist tool registration to database after add` - **Branch**: `fix/tool-add-not-registering` - **Type**: Bug - **Priority**: Critical - **MoSCoW**: Must have - **Points**: 3 - **Milestone**: v3.2.0 - **Assignee**: @freemo ## Parent Epic: #401 (E2E Integration Testing — bug discovered during M3 acceptance testing) ## Background and Context Bug reported by @brent.edwards on Day 27. After `agents init` and `agents tool add -c /app/examples/tools/mcp-tool.yaml`, running `agents tool list` shows "No tools found." The tool add command appears to succeed silently but the tool is not persisted/registered. Investigation reveals that `ToolRegistryRepository.create()` in `repositories.py` calls `session.flush()` but never `session.commit()`. This is the exact same pattern as bug #589 (project persistence). The `sessionmaker` creates sessions with `autocommit=False` (default), so `flush()` sends SQL to the engine's transaction buffer but never persists it. When the session is garbage-collected, SQLAlchemy performs an implicit rollback. The same bug exists in `update()` and `delete()` methods. The `_get_tool_registry_service()` factory in `tool.py` creates a raw `sessionmaker` without using a `UnitOfWork`, so nobody ever calls `commit()`. ## Reproduction Steps ``` cd ~ mkdir data uv venv source .venv/bin/activate uv pip install -e /app agents init agents tool add -c /app/examples/tools/mcp-tool.yaml ``` ### Test ("When") ``` agents tool list ``` ### Expected To see the tool among the list of tools. ### Actual ``` (devuser) ➜ ~ agents tool list 2026-03-07 01:17:41 [debug ] Starting attempt attempt_number=1 outcome=None No tools found. Register one with 'agents tool add --config <file>' ``` ## Acceptance Criteria - [x] `ToolRegistryRepository.create()` calls `session.commit()` after `session.flush()` - [x] `ToolRegistryRepository.update()` calls `session.commit()` after `session.flush()` - [x] `ToolRegistryRepository.delete()` calls `session.commit()` after `session.flush()` - [x] All three methods have `finally: session.close()` guards - [x] `agents tool add` followed by `agents tool list` in separate processes shows the tool - [x] Existing tests pass (BDD, Robot, ASV) - [x] New Behave BDD regression tests for tool add/list persistence - [x] New Robot Framework integration smoke tests for tool lifecycle - [x] New ASV benchmark for tool add/list round-trip ## Subtasks - [x] Fix `ToolRegistryRepository.create()` — add `session.commit()` + `finally: session.close()` - [x] Fix `ToolRegistryRepository.update()` — add `session.commit()` + `finally: session.close()` - [x] Fix `ToolRegistryRepository.delete()` — add `session.commit()` + `finally: session.close()` - [x] Update class docstring to reflect commit-per-method pattern - [x] Write Behave BDD regression scenarios for tool add/list persistence - [x] Write Robot Framework integration smoke tests for tool lifecycle - [x] Write ASV benchmark for tool add/list round-trip - [x] Verify coverage >=97% via `nox -s coverage_report` - [x] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

freemo referenced this issue

2026-03-07 23:06:08 +00:00

TDD: agents tool add does not register tool test #638

freemo added the

labels

2026-03-07 23:06:19 +00:00

freemo self-assigned this

2026-03-07 23:06:36 +00:00

freemo added this to the v3.2.0 milestone

2026-03-07 23:06:36 +00:00

freemo referenced this issue

2026-03-07 23:07:33 +00:00

agents skill does not register when skills are added. #620

freemo commented

2026-03-07 23:07:39 +00:00

Owner

Triage — Day 27

Status: Confirmed bug. Triaged and labeled.

Actions Taken

Added labels: Type/Bug, Priority/Critical, MoSCoW/Must have, State/Unverified, Points/3
Assigned to: @freemo (CTO — best positioned to diagnose persistence/registry path)
Milestone: v3.2.0 (M3 — all bugs are top priority)
Removed redundant bug label (repo-level default; use org-level Type/Bug)
Created TDD counterpart: #638

Issue Body Compliance Note

Same as #620 — the issue body is missing CONTRIBUTING.md template sections (Metadata table, Background, Acceptance Criteria, Subtasks, Definition of Done). The Given/When/Expected/Actual format is good for reproduction, but the structural sections will need to be added by the assignee.

Root Cause Hypothesis

Likely shares root cause with #620 (skill add not registering). Both skill add and tool add go through similar registration flows. Suspect a common persistence issue in the resource add path — possibly session.flush() without session.commit(), or the in-memory registry not being updated after the DB write.

Dependencies

TDD tests (#638) must be written first per CONTRIBUTING.md Bug Fix Workflow
TDD infrastructure (#627, #628) blocks TDD test execution but not creation
Likely shares root cause with #620 (skill add not registering) — fix one, fix both

## Triage — Day 27 **Status**: Confirmed bug. Triaged and labeled. ### Actions Taken - Added labels: `Type/Bug`, `Priority/Critical`, `MoSCoW/Must have`, `State/Unverified`, `Points/3` - Assigned to: @freemo (CTO — best positioned to diagnose persistence/registry path) - Milestone: v3.2.0 (M3 — all bugs are top priority) - Removed redundant `bug` label (repo-level default; use org-level `Type/Bug`) - Created TDD counterpart: #638 ### Issue Body Compliance Note Same as #620 — the issue body is missing CONTRIBUTING.md template sections (Metadata table, Background, Acceptance Criteria, Subtasks, Definition of Done). The Given/When/Expected/Actual format is good for reproduction, but the structural sections will need to be added by the assignee. ### Root Cause Hypothesis Likely shares root cause with #620 (skill add not registering). Both `skill add` and `tool add` go through similar registration flows. Suspect a common persistence issue in the resource add path — possibly `session.flush()` without `session.commit()`, or the in-memory registry not being updated after the DB write. ### Dependencies - TDD tests (#638) must be written first per CONTRIBUTING.md Bug Fix Workflow - TDD infrastructure (#627, #628) blocks TDD test execution but not creation - Likely shares root cause with #620 (skill add not registering) — fix one, fix both

freemo added a new dependency

2026-03-07 23:57:21 +00:00

#638 TDD: agents tool add does not register tool test

freemo added

and removed

labels

2026-03-08 00:26:58 +00:00

freemo commented

2026-03-08 00:27:16 +00:00

Owner

Verification — Day 28

Status: Verified. Bug confirmed and root cause identified.

Root Cause

ToolRegistryRepository.create() in repositories.py (line ~3247) calls session.flush() but never calls session.commit(). This is identical to bug #589 (project persistence).

The _get_tool_registry_service() factory in tool.py creates a raw sessionmaker without a UnitOfWork, so no caller ever commits the transaction. When the session is garbage-collected (or the process exits), SQLAlchemy performs an implicit rollback.

The same flush-without-commit bug exists in:

ToolRegistryRepository.update() (~line 3377)
ToolRegistryRepository.delete() (~line 3414)

Actions Taken

Updated issue body with CONTRIBUTING.md-compliant template (Metadata, Subtasks, DoD)
Transitioned from State/Unverified → State/Verified
Branch assigned: fix/tool-add-not-registering
Commit message: fix(tool): persist tool registration to database after add

Fix Plan

Add session.commit() after session.flush() in all three methods
Add finally: session.close() guard to all three methods
Update class docstring to reflect commit-per-method pattern
Write Behave BDD + Robot + ASV tests

## Verification — Day 28 **Status**: Verified. Bug confirmed and root cause identified. ### Root Cause `ToolRegistryRepository.create()` in `repositories.py` (line ~3247) calls `session.flush()` but **never calls `session.commit()`**. This is identical to bug #589 (project persistence). The `_get_tool_registry_service()` factory in `tool.py` creates a raw `sessionmaker` without a `UnitOfWork`, so no caller ever commits the transaction. When the session is garbage-collected (or the process exits), SQLAlchemy performs an implicit rollback. The same flush-without-commit bug exists in: - `ToolRegistryRepository.update()` (~line 3377) - `ToolRegistryRepository.delete()` (~line 3414) ### Actions Taken - Updated issue body with CONTRIBUTING.md-compliant template (Metadata, Subtasks, DoD) - Transitioned from `State/Unverified` → `State/Verified` - Branch assigned: `fix/tool-add-not-registering` - Commit message: `fix(tool): persist tool registration to database after add` ### Fix Plan 1. Add `session.commit()` after `session.flush()` in all three methods 2. Add `finally: session.close()` guard to all three methods 3. Update class docstring to reflect commit-per-method pattern 4. Write Behave BDD + Robot + ASV tests

freemo added

and removed

labels

2026-03-08 00:27:35 +00:00

freemo referenced this issue from a commit

2026-03-08 03:14:56 +00:00

fix(tool): persist tool registration to database after add

freemo referenced this issue from a pull request that will close it,

2026-03-08 03:15:35 +00:00

fix(tool): persist tool registration to database after add #641

freemo added

and removed

labels

2026-03-08 03:16:01 +00:00

freemo commented

2026-03-08 03:16:25 +00:00

Owner

Implementation Report — PR #641

Root Cause Analysis

The ToolRegistryRepository.create() method (and similarly update() / delete()) in src/cleveragents/infrastructure/database/repositories.py called session.flush() but never session.commit(). The CLI's _get_tool_registry_service() factory in src/cleveragents/cli/commands/tool.py:75-96 creates a raw sessionmaker without a UnitOfWork wrapper, so no caller ever committed the transaction. When the session was garbage-collected, SQLAlchemy performed an implicit rollback.

The identical bug also existed in ValidationAttachmentRepository.attach() and .detach().

Changes Made

Bug Fix — `repositories.py`

Method	Line (approx)	Change
`ToolRegistryRepository.create()`	~3247	Added `session.commit()` after `session.flush()`, `finally: session.close()`
`ToolRegistryRepository.update()`	~3377	Same
`ToolRegistryRepository.delete()`	~3414	Same
`ValidationAttachmentRepository.attach()`	~3699	Same
`ValidationAttachmentRepository.detach()`	~3745	Same

Both class docstrings updated from "flush but do NOT commit" to "flush and commit".

New Tests

features/tool_add_persist.feature — 3 Behave scenarios (single round-trip, multi-tool, duplicate rejection) using file-based SQLite
features/steps/tool_add_persist_steps.py — Step definitions with tool-persist prefix to avoid AmbiguousStep collisions
robot/tool_add_persist.robot + robot/helper_tool_add_persist.py — 2 Robot Framework integration tests
benchmarks/tool_add_persist_bench.py — ASV benchmark with track_list_after_add_count

Key Design Decisions

Commit-in-repository rather than adding UnitOfWork to the CLI factory — CLI commands are simple CRUD that should auto-persist. The existing in-memory test suite already shared a single session (so flush was effectively committed), meaning this change is backward-compatible.
File-based SQLite in tests — in-memory databases wouldn't catch the bug since flush() is sufficient within a single persistent session; the bug only manifests when the engine/session is fully disposed between operations.
finally: session.close() — ensures deterministic session cleanup even when exceptions propagate, preventing connection pool exhaustion.

Quality Gate Results

Gate	Result
`nox -s lint`	PASSED
`nox -s typecheck`	PASSED (0 errors)
`nox -s unit_tests`	9109 scenarios passed, 0 failed
`nox -s integration_tests`	New tests PASSED; pre-existing failures in `cli_plan_context_commands` and `core_cli_commands` are unrelated (FileNotFoundError in `agents init`)
`nox -s coverage_report`	97% (meets ≥97% threshold)

Files Modified/Created

File	Action
`src/cleveragents/infrastructure/database/repositories.py`	Modified (5 methods fixed, 2 docstrings updated)
`features/tool_add_persist.feature`	Created
`features/steps/tool_add_persist_steps.py`	Created
`robot/tool_add_persist.robot`	Created
`robot/helper_tool_add_persist.py`	Created
`benchmarks/tool_add_persist_bench.py`	Created

## Implementation Report — PR #641 ### Root Cause Analysis The `ToolRegistryRepository.create()` method (and similarly `update()` / `delete()`) in `src/cleveragents/infrastructure/database/repositories.py` called `session.flush()` but **never** `session.commit()`. The CLI's `_get_tool_registry_service()` factory in `src/cleveragents/cli/commands/tool.py:75-96` creates a raw `sessionmaker` without a `UnitOfWork` wrapper, so no caller ever committed the transaction. When the session was garbage-collected, SQLAlchemy performed an implicit rollback. The identical bug also existed in `ValidationAttachmentRepository.attach()` and `.detach()`. ### Changes Made #### Bug Fix — `repositories.py` | Method | Line (approx) | Change | |--------|--------------|--------| | `ToolRegistryRepository.create()` | ~3247 | Added `session.commit()` after `session.flush()`, `finally: session.close()` | | `ToolRegistryRepository.update()` | ~3377 | Same | | `ToolRegistryRepository.delete()` | ~3414 | Same | | `ValidationAttachmentRepository.attach()` | ~3699 | Same | | `ValidationAttachmentRepository.detach()` | ~3745 | Same | Both class docstrings updated from "flush but do NOT commit" to "flush **and commit**". #### New Tests - **`features/tool_add_persist.feature`** — 3 Behave scenarios (single round-trip, multi-tool, duplicate rejection) using file-based SQLite - **`features/steps/tool_add_persist_steps.py`** — Step definitions with `tool-persist` prefix to avoid AmbiguousStep collisions - **`robot/tool_add_persist.robot`** + **`robot/helper_tool_add_persist.py`** — 2 Robot Framework integration tests - **`benchmarks/tool_add_persist_bench.py`** — ASV benchmark with `track_list_after_add_count` ### Key Design Decisions 1. **Commit-in-repository** rather than adding UnitOfWork to the CLI factory — CLI commands are simple CRUD that should auto-persist. The existing in-memory test suite already shared a single session (so flush was effectively committed), meaning this change is backward-compatible. 2. **File-based SQLite** in tests — in-memory databases wouldn't catch the bug since `flush()` is sufficient within a single persistent session; the bug only manifests when the engine/session is fully disposed between operations. 3. **`finally: session.close()`** — ensures deterministic session cleanup even when exceptions propagate, preventing connection pool exhaustion. ### Quality Gate Results | Gate | Result | |------|--------| | `nox -s lint` | PASSED | | `nox -s typecheck` | PASSED (0 errors) | | `nox -s unit_tests` | **9109 scenarios passed**, 0 failed | | `nox -s integration_tests` | New tests PASSED; pre-existing failures in `cli_plan_context_commands` and `core_cli_commands` are unrelated (FileNotFoundError in `agents init`) | | `nox -s coverage_report` | **97%** (meets ≥97% threshold) | ### Files Modified/Created | File | Action | |------|--------| | `src/cleveragents/infrastructure/database/repositories.py` | Modified (5 methods fixed, 2 docstrings updated) | | `features/tool_add_persist.feature` | Created | | `features/steps/tool_add_persist_steps.py` | Created | | `robot/tool_add_persist.robot` | Created | | `robot/helper_tool_add_persist.py` | Created | | `benchmarks/tool_add_persist_bench.py` | Created |

freemo added a new dependency

2026-03-08 22:11:30 +00:00

#641 fix(tool): persist tool registration to database after add

freemo referenced this issue from a commit

2026-03-08 22:11:51 +00:00

fix(tool): persist tool registration to database after add

freemo closed this issue

2026-03-08 22:17:10 +00:00