Skip to content

feat: Add install-server and install-client Makefile targets#94

Merged
evansenter merged 3 commits into
mainfrom
install-server-client
Jan 25, 2026
Merged

feat: Add install-server and install-client Makefile targets#94
evansenter merged 3 commits into
mainfrom
install-server-client

Conversation

@evansenter

Copy link
Copy Markdown
Owner

Summary

  • Adds install-server target for machines hosting the database
  • Adds install-client target for connecting to remote servers (requires REMOTE_URL)
  • Adds backwards-compatible install: install-server alias

Mirrors the agent-event-bus pattern for multi-machine Tailscale setups.

Test plan

  • make check passes
  • make install-server works (existing behavior)
  • make install-client REMOTE_URL=http://example.com/mcp configures MCP correctly

Part of #93

🤖 Generated with Claude Code

Mirrors the agent-event-bus pattern for multi-machine setups:
- install-server: Full local installation with LaunchAgent/systemd + MCP
- install-client: CLI + remote MCP config (requires REMOTE_URL)
- install: Backwards-compatible alias for install-server

Part of #93 (deploy to speck-vm)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude

claude Bot commented Jan 25, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Adds install-server and install-client Makefile targets for multi-machine Tailscale setups, with a backwards-compatible install alias pointing to install-server.

Issues Found

Critical

None

Important

None

Suggestions

  • Makefile:57-61 - The PATH check in install-server is conditional (@if ! echo ...) which is an improvement, but the original install target always showed the PATH message. While this is a reasonable enhancement, it changes observable behavior that users may rely on. Consider documenting this change in the PR description or ensuring it's intentional.

  • Makefile:80 - The install-client target explicitly removes the existing MCP config before adding (mcp remove ... || true), but install-server doesn't do this - it relies on the 2>/dev/null suppression of the "already configured" case. For consistency, consider either: (a) having install-server also do remove-then-add, or (b) documenting why the behavior differs between targets.

  • Makefile:90-91 - The instruction to add AGENT_SESSION_ANALYTICS_URL to shell profile is good, but this environment variable doesn't appear to be used anywhere in the codebase yet (per issue Deploy agent-session-analytics to speck-vm with Tailscale #93, the push CLI command that would use it hasn't been implemented). Consider either: (a) adding a note that this is for future functionality, or (b) deferring this instruction until the push command is implemented.

Verdict

REQUEST_CHANGES - A few minor consistency and documentation suggestions to improve clarity.


Automated review by Claude Code

- Remove reinstall target (install-server is idempotent and restarts)
- Document that install-server/install-client are idempotent
- Add logs target for tailing server logs
- Fix restart message to say "make install-server"

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude

claude Bot commented Jan 25, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Adds install-server and install-client Makefile targets for multi-machine Tailscale setups, with install as a backwards-compatible alias to install-server. Also adds a logs target for convenience.

Issues Found

Critical

None

Important

  • CLAUDE.md:50,59 - The PR removes the reinstall target from the Makefile, but CLAUDE.md still references it in two places:

    • Line 50: make reinstall # pip install -e . + restart (for pyproject.toml)
    • Line 59: | \pyproject.toml` | `make reinstall` |`

    This will cause documentation to be out of sync with actual functionality. Either restore the reinstall target or update CLAUDE.md to remove these references (perhaps recommending make install-server or uv sync && make restart as the replacement workflow).

Suggestions

  • Makefile:82-84 - The install-client target does mcp remove then mcp add, while install-server relies on the 2>/dev/null pattern to handle the "already configured" case. For idempotency consistency, consider using the same pattern in both targets. The current approach works, but the difference in behavior (silent no-op vs explicit remove-then-add) could be surprising.

  • Makefile:92-93 - The instruction to set AGENT_SESSION_ANALYTICS_URL references functionality (push CLI command) that is not implemented yet per issue Deploy agent-session-analytics to speck-vm with Tailscale #93 task list. Consider adding a brief note like "(for future push command)" to clarify this is forward-looking setup.

Previously Addressed (Filtered)

  • Makefile:57-61 - PATH check conditional behavior (noted as intentional enhancement)
  • Makefile:80 - MCP remove/add consistency question (similar to current suggestion, acknowledging the design choice)
  • Makefile:90-91 - AGENT_SESSION_ANALYTICS_URL env var for future use (similar to current suggestion)

3 items from prior feedback rounds were not re-raised but informed the current suggestions.

Verdict

REQUEST_CHANGES - The reinstall target removal requires corresponding CLAUDE.md updates to maintain documentation accuracy.


Automated review by Claude Code

- Replace reinstall with install-server (idempotent)
- Add logs target to command list
- Update "When to Restart" table

Addresses reviewer feedback.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@evansenter

Copy link
Copy Markdown
Owner Author

Feedback Addressed

Implemented

  • [Important] CLAUDE.md references to make reinstall - Updated to recommend make install-server (idempotent, restarts service automatically)

Skipped

  • [Suggestion] MCP remove/add consistency - Intentional difference: install-server uses static localhost URL (just suppress "already exists"), install-client uses dynamic REMOTE_URL that may change (needs remove-then-add)
  • [Suggestion] AGENT_SESSION_ANALYTICS_URL note - Implementation of push command is coming next, so the scaffolding is appropriate

@claude

claude Bot commented Jan 25, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Adds install-server and install-client Makefile targets for multi-machine Tailscale setups, replacing the old reinstall target with a cleaner idempotent install-server workflow. Includes backwards-compatible install alias and a new logs target.

Issues Found

Critical

None

Important

None

Suggestions

None

Previously Addressed (Filtered)

  • CLAUDE.md:50,59 - References to make reinstall (Implemented: updated to show make install-server)
  • Makefile:82-84 - MCP remove/add consistency between targets (Skipped: intentional difference for static vs dynamic URLs)
  • Makefile:92-93 - AGENT_SESSION_ANALYTICS_URL env var for future use (Skipped: scaffolding for upcoming push command per Deploy agent-session-analytics to speck-vm with Tailscale #93)

3 items from prior feedback rounds were not re-raised.

Verdict

APPROVE - Code looks good, no issues found. The CLAUDE.md documentation has been updated to match the new Makefile targets, the implementation is clean and idempotent, and the previously raised suggestions were addressed or explained with reasonable justifications.


Automated review by Claude Code

@evansenter evansenter merged commit fc0aa62 into main Jan 25, 2026
3 checks passed
@evansenter evansenter deleted the install-server-client branch January 25, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant