cleveragents/cleveragents-core

Fork 3

BUG-HUNT: [error-handling] Broad exception in deactivate #1993

New issue

Open

opened 2026-04-03 00:32:35 +00:00 by freemo · 3 comments

freemo commented

2026-04-03 00:32:35 +00:00

Owner

Metadata

Branch: fix/error-handling-tool-lifecycle-deactivate
Commit Message: fix(tool): replace broad Exception catch in ToolRuntime.deactivate and deactivate_plan with specific exception types
Milestone: v3.2.0
Parent Epic: #1669

Bug Report: [error-handling] — Broad exception in deactivate

Severity Assessment

Impact: Potential to swallow and hide bugs, making debugging difficult.
Likelihood: Low, but possible if other unexpected errors occur.
Priority: Low

Location

File: src/cleveragents/tool/lifecycle.py
Function/Class: ToolRuntime.deactivate and ToolRuntime.deactivate_plan
Lines: 650-657 and 668-678

Description

The deactivate and deactivate_plan methods in ToolRuntime use a broad except Exception as exc: block. This can catch and obscure unexpected errors, making it difficult to debug issues that are not related to the deactivation process itself.

Per CONTRIBUTING.md exception propagation guidelines: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic." and "Only catch exceptions when you can meaningfully handle them (e.g., retry logic, resource cleanup, adding context). Otherwise, let them propagate."

Evidence

        try:
            instance.deactivate(ctx)
        except Exception as exc:
            logger.warning(
                "Tool deactivation failed (non-fatal)",
                extra={"tool": tool_name, "plan_id": ctx.plan_id, "error": str(exc)},
            )

Expected Behavior

The exception handling should be more specific, catching only the expected exceptions. If the intention is to catch all exceptions, it should be logged and re-raised or handled in a way that doesn't hide the original error.

Suggested Fix

Catch more specific exceptions if possible (e.g., ToolDeactivationError, RuntimeError, OSError). If a broad catch is truly necessary for non-fatal cleanup, at minimum log the full traceback (not just str(exc)) to preserve the original stack trace for debugging.

Subtasks

Write a TDD issue-capture Behave test that proves the bug exists (unexpected exceptions are silently swallowed in deactivate and deactivate_plan)
Identify the specific exception types that instance.deactivate(ctx) is expected to raise
Replace the broad except Exception with specific exception types in ToolRuntime.deactivate
Replace the broad except Exception with specific exception types in ToolRuntime.deactivate_plan
If broad catch is unavoidable, log the full traceback using logger.exception(...) or exc_info=True to preserve stack trace
Update the TDD test to verify the fix (test should now pass)
Tests (Behave): Add/update scenarios for tool deactivation error handling
Tests (Robot): Add integration test for tool lifecycle deactivation error paths
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.
All nox stages pass.
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: ca-new-issue-creator

## Metadata - **Branch**: `fix/error-handling-tool-lifecycle-deactivate` - **Commit Message**: `fix(tool): replace broad Exception catch in ToolRuntime.deactivate and deactivate_plan with specific exception types` - **Milestone**: v3.2.0 - **Parent Epic**: #1669 ## Bug Report: [error-handling] — Broad exception in deactivate ### Severity Assessment - **Impact**: Potential to swallow and hide bugs, making debugging difficult. - **Likelihood**: Low, but possible if other unexpected errors occur. - **Priority**: Low ### Location - **File**: `src/cleveragents/tool/lifecycle.py` - **Function/Class**: `ToolRuntime.deactivate` and `ToolRuntime.deactivate_plan` - **Lines**: `650-657` and `668-678` ### Description The `deactivate` and `deactivate_plan` methods in `ToolRuntime` use a broad `except Exception as exc:` block. This can catch and obscure unexpected errors, making it difficult to debug issues that are not related to the deactivation process itself. Per CONTRIBUTING.md exception propagation guidelines: *"Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic."* and *"Only catch exceptions when you can meaningfully handle them (e.g., retry logic, resource cleanup, adding context). Otherwise, let them propagate."* ### Evidence ```python try: instance.deactivate(ctx) except Exception as exc: logger.warning( "Tool deactivation failed (non-fatal)", extra={"tool": tool_name, "plan_id": ctx.plan_id, "error": str(exc)}, ) ``` ### Expected Behavior The exception handling should be more specific, catching only the expected exceptions. If the intention is to catch all exceptions, it should be logged and re-raised or handled in a way that doesn't hide the original error. ### Suggested Fix Catch more specific exceptions if possible (e.g., `ToolDeactivationError`, `RuntimeError`, `OSError`). If a broad catch is truly necessary for non-fatal cleanup, at minimum log the full traceback (not just `str(exc)`) to preserve the original stack trace for debugging. ### Category error-handling ## Subtasks - [ ] Write a TDD issue-capture Behave test that proves the bug exists (unexpected exceptions are silently swallowed in `deactivate` and `deactivate_plan`) - [ ] Identify the specific exception types that `instance.deactivate(ctx)` is expected to raise - [ ] Replace the broad `except Exception` with specific exception types in `ToolRuntime.deactivate` - [ ] Replace the broad `except Exception` with specific exception types in `ToolRuntime.deactivate_plan` - [ ] If broad catch is unavoidable, log the full traceback using `logger.exception(...)` or `exc_info=True` to preserve stack trace - [ ] Update the TDD test to verify the fix (test should now pass) - [ ] Tests (Behave): Add/update scenarios for tool deactivation error handling - [ ] Tests (Robot): Add integration test for tool lifecycle deactivation error paths - [ ] 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. - All nox stages pass. - Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: ca-new-issue-creator

freemo added this to the v3.2.0 milestone

2026-04-03 00:32:52 +00:00

freemo commented

2026-04-03 02:42:32 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Low — The broad exception catch in deactivate and deactivate_plan is a code quality concern, not a functional bug. The current behavior (logging and continuing) is intentionally non-fatal for cleanup operations. The risk of silently swallowing unexpected errors is real but low-likelihood.
Milestone: v3.2.0 (Decisions + Validations — already assigned, though this is a tool lifecycle issue, not a decisions/validation issue. The milestone assignment is acceptable since v3.2.0 covers general code quality improvements.)
MoSCoW: Could Have — This is a code quality improvement per CONTRIBUTING.md exception handling guidelines. It does not affect functional behavior and is not blocking any other work. Should be addressed when time permits.
Parent Epic: #1669 (already linked, confirmed correct)

The suggested fix is reasonable: narrowing the exception types and logging full tracebacks.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: ca-project-owner

Issue triaged by project owner: - **State**: Verified - **Priority**: Low — The broad exception catch in `deactivate` and `deactivate_plan` is a code quality concern, not a functional bug. The current behavior (logging and continuing) is intentionally non-fatal for cleanup operations. The risk of silently swallowing unexpected errors is real but low-likelihood. - **Milestone**: v3.2.0 (Decisions + Validations — already assigned, though this is a tool lifecycle issue, not a decisions/validation issue. The milestone assignment is acceptable since v3.2.0 covers general code quality improvements.) - **MoSCoW**: Could Have — This is a code quality improvement per CONTRIBUTING.md exception handling guidelines. It does not affect functional behavior and is not blocking any other work. Should be addressed when time permits. - **Parent Epic**: #1669 (already linked, confirmed correct) The suggested fix is reasonable: narrowing the exception types and logging full tracebacks. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo added the

MoSCoW

Could have

label

2026-04-03 02:43:11 +00:00

freemo commented

2026-04-05 06:48:06 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Low — broad exception handling is a code quality issue, not a functional bug
Milestone: v3.2.0
MoSCoW: Could Have — error handling improvements are desirable but not blocking milestone completion. Per CONTRIBUTING.md, exceptions should propagate unless there is meaningful recovery logic, so this is a valid concern but low urgency
Parent Epic: #392 (Actor YAML & Compiler) — the deactivate method is part of the actor lifecycle

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: ca-project-owner

Issue triaged by project owner: - **State**: Verified - **Priority**: Low — broad exception handling is a code quality issue, not a functional bug - **Milestone**: v3.2.0 - **MoSCoW**: Could Have — error handling improvements are desirable but not blocking milestone completion. Per CONTRIBUTING.md, exceptions should propagate unless there is meaningful recovery logic, so this is a valid concern but low urgency - **Parent Epic**: #392 (Actor YAML & Compiler) — the `deactivate` method is part of the actor lifecycle --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo removed this from the v3.2.0 milestone

2026-04-06 22:30:59 +00:00

freemo commented

2026-04-06 22:31:12 +00:00

Author

Owner

This issue has been moved to the backlog as part of an aggressive grooming of the v3.2.0 milestone. It has been deemed non-critical for the minimal viability of the milestone and will be addressed in a future release.