Skip to content

fix(config,skills): strip 'export' prefix when parsing .env files - #51163

Open
wukai424 wants to merge 1 commit into
NousResearch:mainfrom
wukai424:fix/env-export-prefix
Open

fix(config,skills): strip 'export' prefix when parsing .env files#51163
wukai424 wants to merge 1 commit into
NousResearch:mainfrom
wukai424:fix/env-export-prefix

Conversation

@wukai424

Copy link
Copy Markdown

Problem

The load_env() functions in hermes_cli/config.py and tools/skills_tool.py parsed lines with line.partition('=') directly. On .env files that use bash-compatible export KEY=VALUE syntax, this produced keys like export NOTION_API_KEY instead of NOTION_API_KEY.

On Windows the Hermes desktop app does not source .env into os.environ, so every lookup fell through to load_env(), which never found the real key. This caused the NOTION_API_KEY (and any other export-prefixed skill env var) setup popup to reappear endlessly — even after the user entered a valid token through the UI.

The same bug existed in the write paths: save_env_value() and remove_env_value() matched startswith('KEY=') which missed export KEY=... lines, creating duplicate entries on every UI save.

Fix

  • Strip export (bash) and set (cmd) shell prefixes before parsing the key/value pair in load_env() in config.py and skills_tool.py
  • Match shell-prefixed lines in save_env_value() and remove_env_value() write paths
  • Matches the existing behavior in agent/secret_scope.py (load_env_file()) which already handled the export prefix correctly

Testing

  • Confirmed get_env_value('NOTION_API_KEY') returns the correct value after the fix (previously returned None)
  • Verified both with and without export prefix work correctly

The load_env() functions in hermes_cli/config.py and
tools/skills_tool.py used line.partition('=') directly,
producing keys like 'export NOTION_API_KEY' when .env files
used bash-compatible 'export KEY=VALUE' syntax.

On Windows the desktop app doesn't source .env into os.environ,
so every lookup fell back to load_env(), which never found the
real key — making the setup popup reappear endlessly even after
the user entered a valid token.

Fix: strip 'export ' (bash) and 'set ' (cmd) prefixes before
parsing, matching what agent/secret_scope.py (load_env_file)
already did. Also harden both write paths (save_env_value and
remove_env_value) to match existing lines regardless of shell
prefixes.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #6659 — both strip the export prefix in the .env parsers (load_env() in hermes_cli/config.py and tools/skills_tool.py). #6659 is the earlier still-open PR and additionally covers hermes_cli/main.py. Marking this as duplicate for tracking; maintainer to pick the canonical fix.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean, targeted fix for .env file parsing. The change strips 'export ' (bash) and 'set ' (cmd) prefixes from keys before parsing, so .env files written with export syntax are handled correctly. Implementation is correct and well-scoped to two files.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the write-path gap. The parser portion has already landed on current main in commit 5c1ac6c70 (hermes_cli/config.py:7301, tools/skills_tool.py:217, and hermes_cli/main.py:849), but the writer defect remains.

Problems

  • hermes_cli/config.py:7577 and hermes_cli/config.py:7656 still only match bare KEY= lines, so the proposed writer direction is needed; however, the PR's parser hunks are now superseded by the current parser/sanitizer path.
  • hermes_cli/memory_setup.py:393 independently writes .env files using the same pre-= key extraction, leaving the same duplicate-entry behavior for export KEY=value.
  • tests/hermes_cli/test_env_export_prefix.py tests parser behavior but has no save_env_value or remove_env_value coverage.

Suggested changes

  • Salvage the writer matching logic onto current hermes_cli/config.py without replacing the existing parser behavior, cover the memory setup writer too, and add update/remove regression tests for export-prefixed lines.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026

@GottZ GottZ left a comment

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.

This was generated by AI during triage.

Summary

Three PRs address the mishandling of shell-prefixed entries in hand-rolled .env parsing. #6659 and the merged #54482 fix export-prefixed reads across all three parsers, while #51163 overlaps those parser changes but additionally fixes save matching that can append a duplicate and remove matching that otherwise leaves an existing prefixed entry untouched.

Related pull requests

  • #6659 [closed] related — (+6/-0) — superseded: This original parser-only fix strips export in config loading, provider detection, and skill loading; it remains relevant because its change was cherry-picked with authorship into merged #54482.
  • #51163 related — (+36/-4) — keep open for salvage, not merge as-is: Its parser hunks overlap the implementation already merged in #54482, but its save_env_value() change prevents appending a duplicate when an export-prefixed key is missed, and its remove_env_value() change allows that prefixed entry to be removed. Consistent with the visible keep_open review on #51163, the writer logic should be rebased onto current main, extended to the analogous memory_setup.py path, and covered by save/remove regression tests.
  • #54482 [merged] related — (+130/-0) — merged reference implementation: It applies #6659's export-prefix parser fix to config loading, provider detection, and skill loading on the modern code path, with four parser regression tests; it does not address the writer defects identified in #51163.

Duplicates

#6659 and #54482 implement essentially the same three-parser export fix, with #54482 being the merged, tested application of #6659. The parser portion of #51163 is also duplicate/superseded, but its save/remove writer changes are distinct.

Suggested consolidation

Merge #51163 only after rebasing it to retain the distinct writer fix, dropping the parser hunks already covered by #54482, extending matching to the analogous hermes_cli/memory_setup.py writer, and adding update/remove regression tests, as required by the contributor keep_open review. #6659 can remain closed as superseded by merged #54482; #54482 is the canonical parser implementation rather than a candidate for closure.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 12 kB of PR diffs, 5 kB of issue/PR text, 14 kB of discussion (6 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants