COG-6341 feat: Add --memory-only flag to cognee-cli forget - #4697
COG-6341 feat: Add --memory-only flag to cognee-cli forget#4697goran-radonic wants to merge 4 commits into
Conversation
cognee.forget() and CogneeApiClient.forget() both accept a memory_only parameter, but cognee-cli forget never exposed it in either the local argparse configuration or the --api-url remote-dispatch path, making memory-only forget entirely unreachable from the CLI. Adds the flag to both paths. Also fixes an unrelated bug found in the same function: --api-url forget dispatch read args.dataset_name, a field the CLI parser never sets (its flag is --dataset), silently dropping --dataset in that mode. And adds a guard for --everything --memory-only, which previously silently ignored --memory-only and did a full destructive wipe instead (--everything has no confirmation prompt). Split out of #4694 per reviewer feedback that PR bundled unrelated concerns; extracted as its own change.
|
@goran-radonic check tests please |
…ee-cli-forget-local-api-url
…ee-cli-forget-local-api-url
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return | ||
| if not everything and not dataset and not dataset_id and not data_id: | ||
| fmt.error("Specify --dataset or --dataset-id, --data-id with dataset, or --everything.") | ||
| return |
There was a problem hiding this comment.
minor — Validation gap: --data-id without a dataset passes through here but fails in the SDK with "data_id requires dataset or dataset_id." For consistency with the other validations added in this PR, check data_id and not dataset and not dataset_id and error with "Specify --dataset or --dataset-id when using --data-id."
| @@ -56,13 +68,22 @@ def execute(self, args: argparse.Namespace) -> None: | |||
| ) | |||
| return | |||
There was a problem hiding this comment.
minor — Same validation gap as in api_dispatch.py: data_id without a dataset reference should error here for better UX. Add a check after line 63: if data_id and not dataset and not dataset_id: fmt.error("Specify --dataset or --dataset-id when using --data-id.")
|
A 90-line diff to add one CLI flag, plus 180 lines of tests to prove it works — the ratio suggests either paranoia or prior trauma with silent flag-dropping. 🧪 🟢 No blocking issues found. Top findings:
See inline comments for details. |
--data-id alone satisfied forget's "must specify something" check, so it reached cognee.forget()/CogneeApiClient.forget() with no dataset and failed there instead, with a less clear error message. Add the same guard to both the local execute() path and the --api-url dispatch path.
| memory_only=False, | ||
| ) | ||
| dispatch(args) | ||
|
|
There was a problem hiding this comment.
[blocker] Missing test for the main bug fix — --data-id without --dataset should error.
The PR title says "Require dataset when --data-id is used" and the commit message explains that --data-id alone used to pass validation and fail later. But there's no test exercising the new validation at api_dispatch.py:393-395.
Add a test:
@patch("cognee.cli.api_dispatch.CogneeApiClient")
def test_data_id_without_dataset_does_not_call_client(self, MockClient):
"""--data-id alone must error; requires --dataset or --dataset-id."""
mock_instance = MagicMock()
MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance)
MockClient.return_value.__exit__ = MagicMock(return_value=False)
args = argparse.Namespace(
api_url="http://localhost:8000",
command="forget",
user_id=None,
dataset=None,
dataset_id=None,
data_id="11111111-1111-1111-1111-111111111111",
everything=False,
memory_only=False,
)
dispatch(args)
mock_instance.forget.assert_not_called()
| ) | ||
|
|
||
| with pytest.raises(CliCommandException): | ||
| command.execute(args) |
There was a problem hiding this comment.
[blocker] Missing test for the main bug fix — --data-id without --dataset should error.
Add a test in this class to match the validation at forget_command.py:71-73:
def test_execute_data_id_without_dataset_errors(self):
"""--data-id alone must error; requires --dataset or --dataset-id."""
mock_cognee = MagicMock()
mock_cognee.forget = AsyncMock()
with patch.dict(sys.modules, {"cognee": mock_cognee}):
command = ForgetCommand()
args = argparse.Namespace(
dataset=None,
dataset_id=None,
data_id="11111111-1111-1111-1111-111111111111",
everything=False,
memory_only=False,
)
# Should not raise, just print an error and return without calling forget().
command.execute(args)
mock_cognee.forget.assert_not_awaited()
| @@ -56,13 +68,26 @@ def execute(self, args: argparse.Namespace) -> None: | |||
| ) | |||
There was a problem hiding this comment.
[minor] Error message inconsistency.
This says --everything/--all but the equivalent message in api_dispatch.py:391 only says --everything. For consistency, either both should mention the --all alias or neither should.
|
A PR titled "Require dataset when --data-id is used" that adds validation for exactly that... then forgets to test it. 🎯 🔴 2 blockers, 1 minor — changes requested
See inline comments for details. |
2 participants
Description
Added --memory-only to cognee-cli forget, was missing even though the SDK already supports it (both locally and through --api-url). Also fixed --dataset getting silently dropped in --api-url mode (wrong arg name) and made --everything --memory-only error instead of just doing a full wipe.
Acceptance Criteria
Type of Change
Screenshots
CLI only, no UI. 164 passed.
Pre-submission Checklist
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.