Skip to content

chore(cli): deprecate soul remember in favor of soul note (phase 2 #231)#236

Open
AnshulPatil2005 wants to merge 2 commits into
qbtrix:devfrom
AnshulPatil2005:feat/issue-231-phase2-clean
Open

chore(cli): deprecate soul remember in favor of soul note (phase 2 #231)#236
AnshulPatil2005 wants to merge 2 commits into
qbtrix:devfrom
AnshulPatil2005:feat/issue-231-phase2-clean

Conversation

@AnshulPatil2005
Copy link
Copy Markdown

Summary

This PR covers only Phase 2 of #231 and targets dev (where Phase 1 was already merged in #233).

Changes

  • Add DeprecationWarning when soul remember is invoked, pointing to soul note <path> <fact>.
  • Print a CLI deprecation notice with --no-dedup guidance for callers that need raw append behavior.
  • Update CLI reference text for soul remember to mark it deprecated and point to soul note.
  • Add a README quickstart example using soul note.
  • Add a CLI test assertion that remember output includes deprecation messaging.

Out of scope

Validation

  • PYTHONPATH=src pytest tests/test_cli/test_cli.py -k remember_command -q
  • PYTHONPATH=src python -m py_compile src/soul_protocol/cli/main.py tests/test_cli/test_cli.py

Refs #231

@AnshulPatil2005
Copy link
Copy Markdown
Author

once this is done i can move to phase 3

Copy link
Copy Markdown
Contributor

@prakashUXtech prakashUXtech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase 2 is scoped tightly and the doc updates landed in both places (cli-reference + README) so nothing drifted.

Four small things to land before this merges:

  1. CHANGELOG entry. Add a line under ## Deprecated in CHANGELOG.md so 0.4.x → 0.5.0 upgraders see this called out.

  2. Removal horizon in the deprecation message. The warning says "use soul note instead" but doesn't say when remember actually goes away. Pick a target version (something like "scheduled for removal in 0.7.0") and put it in both the warning text and the docs section. Without a horizon, deprecations linger indefinitely.

  3. Docs section header. Change ### \soul remember`to### `soul remember` (deprecated)indocs/cli-reference.md` so the TOC reflects the state, not just the body text.

  4. Test sharpening (optional). The existing test asserts the deprecation text appears. Also assert result.exit_code == 0 and that the memory actually got stored, so the test verifies "warns AND still works" rather than just "warns."

Two style notes for your own reference:

  • stacklevel=2 is correct, but inside a Click command it doesn't point anywhere useful (the caller is Click internals, not user code). It's a no-op here, not wrong.
  • The console.print + warnings.warn double-notification looks redundant but is actually right. Python silences DeprecationWarning from __main__ by default, so the console.print is what most CLI users will see.

Approving once the CHANGELOG + horizon are in. Good Phase 2 close.

@AnshulPatil2005
Copy link
Copy Markdown
Author

Phase 2 is scoped tightly and the doc updates landed in both places (cli-reference + README) so nothing drifted.

Four small things to land before this merges:

  1. CHANGELOG entry. Add a line under ## Deprecated in CHANGELOG.md so 0.4.x → 0.5.0 upgraders see this called out.
  2. Removal horizon in the deprecation message. The warning says "use soul note instead" but doesn't say when remember actually goes away. Pick a target version (something like "scheduled for removal in 0.7.0") and put it in both the warning text and the docs section. Without a horizon, deprecations linger indefinitely.
  3. Docs section header. Change ### \soul remember``to### soul remember` (deprecated)`in`docs/cli-reference.md` so the TOC reflects the state, not just the body text.
  4. Test sharpening (optional). The existing test asserts the deprecation text appears. Also assert result.exit_code == 0 and that the memory actually got stored, so the test verifies "warns AND still works" rather than just "warns."

Two style notes for your own reference:

  • stacklevel=2 is correct, but inside a Click command it doesn't point anywhere useful (the caller is Click internals, not user code). It's a no-op here, not wrong.
  • The console.print + warnings.warn double-notification looks redundant but is actually right. Python silences DeprecationWarning from __main__ by default, so the console.print is what most CLI users will see.

Approving once the CHANGELOG + horizon are in. Good Phase 2 close.

Phase 2 is scoped tightly and the doc updates landed in both places (cli-reference + README) so nothing drifted.

Four small things to land before this merges:

  1. CHANGELOG entry. Add a line under ## Deprecated in CHANGELOG.md so 0.4.x → 0.5.0 upgraders see this called out.
  2. Removal horizon in the deprecation message. The warning says "use soul note instead" but doesn't say when remember actually goes away. Pick a target version (something like "scheduled for removal in 0.7.0") and put it in both the warning text and the docs section. Without a horizon, deprecations linger indefinitely.
  3. Docs section header. Change ### \soul remember``to### soul remember` (deprecated)`in`docs/cli-reference.md` so the TOC reflects the state, not just the body text.
  4. Test sharpening (optional). The existing test asserts the deprecation text appears. Also assert result.exit_code == 0 and that the memory actually got stored, so the test verifies "warns AND still works" rather than just "warns."

Two style notes for your own reference:

  • stacklevel=2 is correct, but inside a Click command it doesn't point anywhere useful (the caller is Click internals, not user code). It's a no-op here, not wrong.
  • The console.print + warnings.warn double-notification looks redundant but is actually right. Python silences DeprecationWarning from __main__ by default, so the console.print is what most CLI users will see.

Approving once the CHANGELOG + horizon are in. Good Phase 2 close.

on it

@AnshulPatil2005
Copy link
Copy Markdown
Author

@prakashUXtech I think merging #242 first would be useful it adds pytest CI to dev-targeting PRs, which would give #236 real test signal instead of the current 'no checks' state. I've already addressed all four items from the review in the follow-up commit (CHANGELOG, removal horizon, doc header, memory-stored assertion) the one remaining gap is adding assert result.exit_code == 0 to the test, which I'll push. Let me know if #242 needs any changes on your end.

prakashUXtech added a commit that referenced this pull request May 23, 2026
… (#252)

Update the agent-facing memory guide so the MCP recommendation
matches the post-#231 reality:

- soul_observe is the dedup-aware path: it runs the full pipeline
  (extraction + reconcile_fact + self-model update), so facts pulled
  out of real user/agent turns collapse onto existing entries rather
  than appending duplicates.
- soul_remember is now scoped to blunt forced writes — short episodic
  events, or cases where dedup is explicitly wrong. The bullet calls
  out that the CLI sibling 'soul remember' is on a deprecation path
  toward 'soul note' (#231 phase 2 / PR #236) and that the dedup-aware
  MCP variant will follow once it ships.

Pure docs change. No runtime behavior or test impact.

Refs #231, #251.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

This PR has been automatically marked as stale because it has not had activity in the last 14 days. It will be closed in 7 days if no further activity occurs. If you're still working on this, please push an update or leave a comment.

@github-actions github-actions Bot added the stale label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants