Skip to content

Merge every .env candidate instead of stopping at the first - #53

Open
kliebensteinchr wants to merge 1 commit into
inkbox-ai:mainfrom
kliebensteinchr:fix/env-file-merge-all-candidates
Open

Merge every .env candidate instead of stopping at the first#53
kliebensteinchr wants to merge 1 commit into
inkbox-ai:mainfrom
kliebensteinchr:fix/env-file-merge-all-candidates

Conversation

@kliebensteinchr

Copy link
Copy Markdown

The bug

_maybe_load_env_file() picks the first existing candidate and ignores the rest:

path = next((p for p in candidates if p.exists()), None)

The candidates are $INKBOX_CLAUDE_ENV_FILE, ./.env, ~/.inkbox-claude/.env. So any unrelated ./.env in the cwd shadows the install's config entirely.

Concretely: a ~/.env holding nothing but SLACK_BOT_TOKEN means inkbox-claude start/restart run from $HOME fail with

INKBOX_API_KEY and INKBOX_IDENTITY are not set — run `inkbox-claude setup` first.

even though both are sitting in ~/.inkbox-claude/.env. The failure points at setup, so the natural next step is re-running the wizard — which rewrites a config that was never the problem.

The systemd/launchd units set INKBOX_CLAUDE_ENV_FILE explicitly, so autostart is unaffected. This only hits manual CLI runs, which is what makes it confusing: the service is up, but the CLI insists you are not configured.

The fix

Read every candidate in the same priority order, still via setdefault:

  • real env beats every file (unchanged)
  • an earlier candidate beats a later one, per key (unchanged in spirit — ./.env still wins over the state dir)
  • a later candidate now fills only what the earlier ones left out

Also dedupes resolved paths so a cwd that is the state dir is not read twice, and treats an unreadable file as absent rather than raising.

No precedence change for anyone whose ./.env already carried the Inkbox keys — that file still wins every key it sets.

Tests

Two regression tests in tests/test_daemon.py:

  • test_maybe_load_env_file_merges_state_dir_under_an_unrelated_cwd_env — the reported failure: unrelated ./.env present, keys still resolve from the state dir, and the cwd file's own var still applies
  • test_maybe_load_env_file_earlier_candidate_wins_per_key — pins the precedence so a future change cannot silently invert it

Full suite locally on 3.12: 346 passed, 22 skipped.

🤖 Generated with Claude Code

_maybe_load_env_file() picked the first existing candidate of
$INKBOX_CLAUDE_ENV_FILE, ./.env, ~/.inkbox-claude/.env and ignored the
rest. An unrelated ./.env in the cwd -- a project's own secrets, or a
stray one in $HOME -- therefore shadowed the install's config entirely,
so `start`/`restart` from that directory reported "INKBOX_API_KEY and
INKBOX_IDENTITY are not set" while both sat in the state dir. The
systemd/launchd units set INKBOX_CLAUDE_ENV_FILE explicitly, so this
only hit manual CLI runs.

Read all candidates in the same priority order instead, still via
setdefault: real env beats every file, an earlier file beats a later
one, and a later file fills only what the earlier ones left out.
Resolved paths are deduped so a cwd that is the state dir is not read
twice, and an unreadable file is treated as absent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread inkbox_claude/daemon.py
if stripped.startswith("export "):
stripped = stripped[len("export "):]
key, value = stripped.split("=", 1)
os.environ.setdefault(key.strip(), value.strip().strip('"').strip("'"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because every later file is merged per key, omission in a higher-priority config no longer restores its documented default. A complete explicit config can silently inherit stale optional values from the state-dir file, and a partial cwd config can be combined with credentials or identity from a different install, producing a configuration no single file defines. Please keep the explicit file authoritative and only fall through from cwd when it has zero bridge-owned keys (or otherwise select one Inkbox config atomically) rather than merging independent configs per key.

Comment thread inkbox_claude/daemon.py
if key_path in seen: # cwd == state dir, or a symlink to it
continue
seen.add(key_path)
lines = path.read_text().splitlines()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The unreadable-file fallback is incomplete: read_text() can raise UnicodeDecodeError, and malformed assignments such as an empty key raise while updating os.environ outside this try. Because lower-priority files are now always read, an unrelated malformed file can crash startup even when a higher-priority config is complete. Please skip decode and malformed-assignment failures per candidate and add regression coverage for both cases.

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.

2 participants