fix: add --version flag, use --yes in agent init instructions, document init prerequisite for mine - #559
fix: add --version flag, use --yes in agent init instructions, document init prerequisite for mine#559adiKhan12 wants to merge 2 commits into
Conversation
…t prerequisite for mine Fixes MemPalace#534. Three issues that break agent-driven use of MemPalace: 1. `mempalace --version` did not exist — agents following SKILL.md prerequisites hit exit code 2. Added --version flag to the CLI argparser using the existing version.py. 2. `mempalace instructions init` Step 5 used `mempalace init <dir>` without --yes — agents hit EOFError when entity detection triggers interactive prompts (confirm_entities calls input() on closed stdin). Updated to `mempalace init --yes <dir>`. 3. `mempalace instructions mine` did not mention that `mempalace init` must run first — agents hit "No mempalace.yaml found" with no guidance. Added Step 0 documenting the prerequisite.
405f388 to
b6a1683
Compare
|
This directly addresses the three agent-init failure modes I diagnosed in #534 — all three fixes are correct and the test coverage is solid. Fix 1 ( Fix 2 ( Fix 3 ( Tests: All four new tests are behavioral, not structural — they verify the flag works and the instructions contain the right text. The One small observation: LGTM. We hit variant 2 of this in our own setup early on — had to trace the EOFError back to the MemPalace-AGI integration — 208 discoveries, 120 tests | dashboard |
|
Thanks @web3guru888 — good catch on the assertion. Just pushed a follow-up commit tightening |
|
Note: #562 also fixes the SKILL.md ( |
|
Thanks for flagging the overlap with #562. Looking at the diff, there are a couple of differences in approach:
Happy to coordinate — if #562 lands first, I can rebase and keep just the parts that don't overlap (--version flag, mine.md Step 0, tests). |
|
@adiKhan12 — the differentiation is well-argued. The three differences are real: On On the On rebasing: The rebase-and-keep approach you described is the right path. If #562 merges first, a rebased version that trims to just (1) The tightened test assertion (asserting |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Nice addition! We incorporated this |
web3guru888
left a comment
There was a problem hiding this comment.
Review: fix: add --version flag, use --yes in agent init instructions, document init prerequisite for mine
Three distinct fixes bundled together, all targeting the same pain: MemPalace failing silently (or crashingly) when driven by an agent. Small PR, high practical value.
What's done well
--version uses action="version" correctly. The argparse action="version" idiom prints the version string and exits 0 — exactly the right mechanism. It's wired to __version__ from version.py, not a hardcoded string, so it'll stay accurate as the version is bumped. The version string format mempalace {__version__} is conventional and machine-parseable. No complaints here.
--yes documentation fills a real gap. The --yes flag has existed since #8 but the agent instructions never told agents to use it. Agents hitting EOFError on input() get a confusing stack trace with no guidance. The fix is exactly right: update the instructions, add a test that verifies init --yes skips input(). The "Without it, the command will crash with EOFError when stdin is not a terminal" note in init.md is particularly good — it explains why, not just what.
Step 0 in mine.md is the right level of specificity. It gives the exact error message agents will see (ERROR: No mempalace.yaml found in <dir>), the exact fix, and even the check condition (if a mempalace.yaml file exists). An agent following this document won't need to ask for help.
Tests cover the content of the markdown files. test_init_instructions_use_yes_flag and test_mine_instructions_mention_init_prerequisite assert on the actual file content, not just code behavior. This prevents someone from "fixing" the code while silently breaking the agent instructions again.
Issues found
The test_main_version_flag test is referenced in the PR description but not visible in the diff. The PR lists four new tests; I see test_cmd_init_yes_skips_interactive_prompts, test_init_instructions_use_yes_flag, and test_mine_instructions_mention_init_prerequisite in the diff. The test_main_version_flag test that verifies --version prints the version and exits 0 should be in test_cli.py — worth confirming it's present (may be outside the visible diff window if the file is large).
init.md Step 5 says "The --yes flag is required in agent/non-interactive contexts." Technically it's not required — mempalace init without --yes works fine in an interactive terminal. The word "required" could confuse a human user reading the docs. Something like "Use --yes to skip interactive prompts in agent/non-interactive contexts" is more accurate.
No SKILL.md update. The PR description mentions SKILL.md prerequisites tell agents to run mempalace --version, implying SKILL.md documents the --version flag as a prerequisite check. If SKILL.md exists and is already correct, no issue. If it still documents the broken invocation (or lacks --version entirely), it should be updated in this PR.
mine.md Step 0 uses a bash code block with mempalace init --yes <dir>. This is good, but the step says "Check if a mempalace.yaml file exists in the target directory" without specifying how an agent should check (e.g., test -f <dir>/mempalace.yaml). A concrete shell command would make this step machine-executable without ambiguity.
Suggestions
- Confirm
test_main_version_flagis present (or add it if missing from the batch). - Change "required" to "recommended" or "necessary" in
init.mdto avoid confusing interactive users. - Add a
test -f <dir>/mempalace.yamlexample tomine.mdStep 0 for fully unambiguous agent instructions. - Verify SKILL.md is consistent with these changes.
Overall verdict
Approve. The three fixes are all correct: --version is wired properly to __version__, the --yes documentation fills a genuine gap that was causing real agent crashes, and the mine.md prerequisite note saves agents from a confusing dead-end. The issues noted are minor — none affect correctness. This is ready to merge.
Reviewed by MemPalace-AGI — autonomous research system with perfect memory
|
Hi, thanks for the contribution. This PR has merge conflicts with Could you rebase onto If this change is no longer relevant, feel free to close the PR. (This message is part of a periodic backlog pass, sent to all open PRs that match this state.) |
|
Thanks for this, and apologies for the delay. |
What does this PR do?
Fixes #534. Three issues that break agent-driven use of MemPalace:
1.
mempalace --versioncrashes (exit 2)SKILL.md prerequisites tell agents to run
mempalace --version, but the flag didn't exist:Fix: Added
--versionto the CLI argparser using existingversion.py.2.
mempalace initwithout--yescauses EOFError in agent contextWhen entities are detected,
confirm_entities()callsinput()which crashes agents (no stdin):Fix: Updated
instructions/init.mdStep 5 to usemempalace init --yes <dir>. The--yesflag already existed (#8) but the agent instructions didn't use it.3.
mempalace minefails with no guidance when init hasn't runAgents following
instructions/mine.mdhit this with no context.Fix: Added Step 0 to
instructions/mine.mddocumenting the init prerequisite with the exact error message.How to test
New tests:
test_main_version_flag— verifies--versionprints version and exits 0test_cmd_init_yes_skips_interactive_prompts— verifiesyes=Trueis passed through toconfirm_entitiestest_init_instructions_use_yes_flag— verifies init.md containsmempalace init --yestest_mine_instructions_mention_init_prerequisite— verifies mine.md mentionsmempalace initandmempalace.yamlChecklist
python -m pytest tests/ -v) — 538 passingruff check .)