Skip to content

fix(bench): remove hardcoded credential paths from benchmark runners#177

Merged
bensig merged 1 commit intoMemPalace:developfrom
travisbreaks:fix/remove-hardcoded-credential-paths
Apr 12, 2026
Merged

fix(bench): remove hardcoded credential paths from benchmark runners#177
bensig merged 1 commit intoMemPalace:developfrom
travisbreaks:fix/remove-hardcoded-credential-paths

Conversation

@travisbreaks
Copy link
Copy Markdown
Contributor

@travisbreaks travisbreaks commented Apr 8, 2026

Summary

  • Removes _load_api_key() fallback to ~/.config/lu/keys.json with personal key names (anthropic_milla, anthropic_claude_code_main) from longmemeval_bench.py and locomo_bench.py
  • Simplifies API key loading to: --llm-key flag > ANTHROPIC_API_KEY env var
  • Updates help text and HYBRID_MODE.md docs to match

The hardcoded path and personal key names leak internal infrastructure details. The standard ANTHROPIC_API_KEY env var is the expected pattern for Anthropic API consumers.

Context

Identified while reviewing the security fixes in PR #34, which bundled this with several other changes. Per reviewer feedback on #34, submitting as a focused standalone fix.

Test plan

  • pytest tests/ -v passes (benchmarks are not part of the test suite)
  • python benchmarks/longmemeval_bench.py --help shows updated help text
  • ANTHROPIC_API_KEY=sk-ant-... python benchmarks/longmemeval_bench.py ... --mode diary still works

🤖 Generated with Claude Code

@travisbreaks
fix(bench): remove hardcoded credential paths from benchmark runners
7d78570 
The `_load_api_key()` function in longmemeval_bench.py and locomo_bench.py
searched for API keys in a fixed path (`~/.config/lu/keys.json`) using
personal key names (`anthropic_milla`, `anthropic_claude_code_main`).

This leaks internal infrastructure details into the public codebase and
trains contributors to store credentials in a non-standard location
rather than using the standard ANTHROPIC_API_KEY env var.

Simplified to: CLI flag > env var > empty string. Updated help text
and HYBRID_MODE.md docs to match.

Co-Authored-By: Tadao <[email protected]>
@bgauryy
Copy link
Copy Markdown

bgauryy commented Apr 8, 2026

PR Review: fix(bench): remove hardcoded credential paths from benchmark runners

Executive Summary

Aspect Value
PR Goal Remove personal credential lookup paths (~/.config/lu/keys.json with key names like anthropic_milla) from benchmark runners
Files Changed 3
Risk Level 🟢 LOW - Pure cleanup, removes code, no new logic
Review Effort 1 - Trivial deletion with consistent doc updates
Recommendation ✅ APPROVE

Affected Areas: benchmarks/locomo_bench.py, benchmarks/longmemeval_bench.py, benchmarks/HYBRID_MODE.md

Business Impact: None — removes dead infrastructure-specific code that external contributors could never use anyway.

Flow Changes: _load_api_key() in both benchmark files now has 2 resolution steps instead of 3. The removed 3rd step (~/.config/lu/keys.json lookup) was personal infrastructure and non-portable.

Ratings

Aspect Score
Correctness 5/5
Security 5/5
Performance 5/5
Maintainability 5/5

PR Health

  • Has clear description
  • References ticket/issue (if applicable)
  • Appropriate size (or justified if large)
  • Has relevant tests (N/A — no testable logic added)

Analysis

What Changed

benchmarks/longmemeval_bench.py (-25/+3)

  • _load_api_key(): Removed 18-line block that read ~/.config/lu/keys.json and iterated personal key names (lu_key, anthropic_milla, anthropic_claude_code_main)
  • Updated docstring to reflect simplified resolution: --llm-key arg > ANTHROPIC_API_KEY env var
  • Updated error message when key is missing (removed ~/.config/lu/keys.json mention)
  • Updated --llm-key argparse help text to match

benchmarks/locomo_bench.py (-18/+1)

  • _load_api_key(): Same 17-line removal of personal key file lookup
  • Added docstring matching the simplified resolution chain

benchmarks/HYBRID_MODE.md (-1/+0)

  • Removed step 3 from "Key loading priority" list (~/.config/lu/keys.json)

Verification

Check Result
Function signature unchanged _load_api_key(key_arg) — no change, callers safe
All callers verified 2 call sites total (1 per file), both pass llm_key arg unchanged
json import still needed Yes — json.dumps/json.loads used extensively in both files
No remaining keys.json refs Only in these 3 files (confirmed via codebase search)
No remaining personal key names Only in these 3 files (confirmed via codebase search)
Docs consistent with code Yes — HYBRID_MODE.md updated to match

Issues

No issues found. This is a clean removal of personal infrastructure details that leaked into the public codebase. The change is complete and consistent across all 3 files.

Created by Octocode MCP https://octocode.ai 🔍🐙

web3guru888
web3guru888 approved these changes Apr 11, 2026
View reviewed changes
Copy link
Copy Markdown

@web3guru888 web3guru888 left a comment

Choose a reason for hiding this comment

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

🔧 Review of #177 — fix(bench): remove hardcoded credential paths from benchmark runners

Scope: +4/−44 · 3 file(s)

  • benchmarks/HYBRID_MODE.md (modified: +0/−1)
  • benchmarks/locomo_bench.py (modified: +1/−18)
  • benchmarks/longmemeval_bench.py (modified: +3/−25)

🟢 Approved — clean, well-structured PR. Good work @travisbreaks!

🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig bensig changed the base branch from main to develop April 11, 2026 22:23
bensig
bensig approved these changes Apr 12, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bensig bensig left a comment

Choose a reason for hiding this comment

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

Code review + security audit clean.

@bensig bensig merged commit 8920610 into MemPalace:develop Apr 12, 2026
jphein added a commit to jphein/mempalace that referenced this pull request Apr 12, 2026
@jphein @claude
fix: merge upstream (develop+main), resolve MemPalace#666 block reaso…
df12ea5 
…n conflicts

Merge upstream/develop: MemPalace#177, MemPalace#361, MemPalace#449, MemPalace#450 (benchmarks, tilde expand,
duplicate cache vars, test fixture cleanup).

Merge upstream/main: MemPalace#666 (z3tz3r0's block reason disambiguation).
Conflict resolution: keep our scoped "For THIS save" phrasing instead of
MemPalace#666's absolute "Do NOT write to auto-memory" — both memory systems are
used in tandem. Also drop "AAAK-compressed" from diary instructions since
diary_write is plain text, not AAAK dialect.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@igorls igorls mentioned this pull request Apr 13, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bensig bensig bensig approved these changes

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner

@igorls igorls Awaiting requested review from igorls igorls is a code owner

+1 more reviewer

@web3guru888 web3guru888 web3guru888 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@travisbreaks @bgauryy @bensig @web3guru888