Skip to content

fix(cli): honor safe-mode isolation in one-shot - #73005

Open
AlambritoDito wants to merge 1 commit into
NousResearch:mainfrom
AlambritoDito:fix/oneshot-safe-mode-isolation
Open

fix(cli): honor safe-mode isolation in one-shot#73005
AlambritoDito wants to merge 1 commit into
NousResearch:mainfrom
AlambritoDito:fix/oneshot-safe-mode-isolation

Conversation

@AlambritoDito

Copy link
Copy Markdown

Summary

Fixes one-shot mode so the existing --safe-mode / --ignore-user-config / --ignore-rules gates reach the embedded agent runtime.

Previously, top-level CLI parsing set the isolation environment variables, but hermes_cli.oneshot still loaded user config and constructed AIAgent without skip_context_files or skip_memory. A superficially successful hermes -z ... --safe-mode invocation could therefore inherit user configuration, rules/context files, and memory.

Changes

  • Skip user config in load_config() when HERMES_IGNORE_USER_CONFIG=1, including cached config paths.
  • Forward HERMES_IGNORE_RULES=1 to one-shot AIAgent as:
    • skip_context_files=True
    • skip_memory=True
  • Preserve explicit model, provider and toolsets under isolation.
  • Add behavioral tests using a temporary HERMES_HOME, sentinel config values, and a capturing agent.
  • Keep managed administrator policy active.

Verification

  • 35 passed — focused one-shot/flag tests
  • 250 passed — config, managed-config and one-shot slice
  • Ruff check green
  • Real isolated invocation with explicit provider/model returned the requested JSON with exit 0
  • Independent security/correctness review: PASS

Notes

This does not add new user-facing configuration or environment variables; it makes existing safe-mode internals work consistently in the one-shot path.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 28, 2026

@teknium1 teknium1 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.

Thanks for the focused isolation fix. The safe-mode premise is confirmed on current main: one-shot still loads user config (hermes_cli/oneshot.py:330; hermes_cli/config.py:3268-3326) and omits the agent skip flags (hermes_cli/oneshot.py:410-435).

Problems

  • The PR does not wire explicit top-level --ignore-user-config / --ignore-rules into the one-shot path. One-shot dispatch bypasses cmd_chat() (hermes_cli/main.py:10759-10766), while those variables are set only in cmd_chat() (hermes_cli/main.py:2641-2648). Thus the proposed downstream checks work for --safe-mode, but not for the two explicit flags described in the PR.
  • The new tests set environment variables directly, so they do not cover that missing argument-to-environment boundary.

Suggested changes

  • Share the isolation-env setup between cmd_chat() and top-level one-shot startup, before discovery/loading.
  • Add a top-level -z regression for each explicit isolation flag, then adapt the config hunk to current hermes_cli/config.py:3262-3406 after the loader refactor.

Automated hermes-sweeper review.

Comment thread hermes_cli/oneshot.py
toolsets_list = sorted(_get_platform_tools(cfg, "cli"))

session_db = _create_session_db_for_oneshot()
ignore_rules = os.environ.get("HERMES_IGNORE_RULES") == "1"

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 consumes the environment flag, but top-level -z --ignore-rules never sets it: one-shot bypasses cmd_chat(), where explicit ignore flags are currently exported. Please wire those arguments before one-shot startup and add a top-level dispatch regression.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Two PRs address the same one-shot isolation failure across user configuration and rules/memory loading. #51797 covers the top-level dispatch, shared config loader, and agent-construction boundaries, while #73005 covers only the downstream config-loader and agent-construction boundaries.

Related pull requests

  • #51797 duplicate — (+315/-38) — keep open with a salvage path: consistent with the automated keep-open verdict, its diff adds the missing shared isolation setup in hermes_cli/main.py, propagates ignore_rules through one-shot dispatch, isolates the config cache, and sets skip_context_files/skip_memory on AIAgent. The concrete salvage path is its main.py dispatch wiring and top-level safe-mode regression coverage, which #73005 lacks.
  • #73005 related — (+216/-5) — close as duplicate of #51797: its config.py and oneshot.py changes overlap the downstream portions of #51797, but its diff does not modify main.py, so explicit top-level --ignore-user-config and --ignore-rules still fail to reach one-shot startup. Despite the keep_open review on #73005, that review itself identifies this missing dispatch boundary and requests the shared startup wiring and top-level regressions already present in #51797.

Duplicates

#73005 duplicates the config-loader and AIAgent-isolation portions of #51797; the explicit duplicate chain is #73005#51797, with #51797 containing the additional top-level dispatch fix.

Suggested consolidation

Keep #51797 open with a salvage path focused on its shared main.py isolation setup, explicit one-shot argument propagation, isolated config-cache handling, and top-level dispatch regressions. Close #73005 as duplicate of #51797: although its visible review says keep_open with medium salvageability, its complete diff still omits the contributor-identified main.py boundary, while #51797 implements that boundary in addition to the overlapping config.py and oneshot.py changes.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup51797 ["PRs duplicating each other"]
        P51797["PR #51797 (open)"]
        P73005["PR #73005 (open)"]
    end
    class P51797 open
    class P73005 open
    class P73005 target
    click P51797 "https://github.com/NousResearch/hermes-agent/pull/51797"
    click P73005 "https://github.com/NousResearch/hermes-agent/pull/73005"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 32 kB of PR diffs, 4 kB of issue/PR text, 1 kB of discussion (2 comments), 0 verify verdicts. 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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants