Skip to content

fix(file-safety): extend cross-profile guard to cover top-level profile state files - #35902

Open
uzunkuyruk wants to merge 2 commits into
NousResearch:mainfrom
uzunkuyruk:fix/file-safety-profile-state-guard
Open

fix(file-safety): extend cross-profile guard to cover top-level profile state files#35902
uzunkuyruk wants to merge 2 commits into
NousResearch:mainfrom
uzunkuyruk:fix/file-safety-profile-state-guard

Conversation

@uzunkuyruk

Copy link
Copy Markdown
Contributor

What does this PR do?

classify_cross_profile_target guarded profile-scoped directories (skills, plugins, cron, memories) but not top-level profile state files (SOUL.md, config.yaml, .env, auth.json).

This meant an agent running under one profile could silently write to another profile's SOUL.md or config.yaml without triggering the cross-profile warning — the exact failure mode reported in #32049 (Docker sandbox mirror writes).

Fix:

  • Add PROFILE_STATE_FILES = ("SOUL.md", "config.yaml", ".env", "auth.json") constant
  • Extend classify_cross_profile_target to detect cross-profile writes targeting these files at both <root>/SOUL.md (default profile) and <root>/profiles/<name>/SOUL.md (named profile) paths

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

References

Fixes #32049

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • My PR contains only changes related to this fix

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/file File tools (read, write, patch, search) labels May 31, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

One issue that looks worth fixing before merge.

agent/file_safety.py — the diff replaces the final else: return None branch with two new elif branches, but does not add a catch-all else at the end.

Original code (on main):

if parts[0] in PROFILE_SCOPED_AREAS:
    target_profile = "default"
    area = parts[0]
elif parts[0] == "profiles" and len(parts) >= 3 and parts[2] in PROFILE_SCOPED_AREAS:
    target_profile = parts[1]
    area = parts[2]
else:
    return None          # ← catch-all for unrecognized paths

After this PR:

if parts[0] in PROFILE_SCOPED_AREAS:
    ...
elif parts[0] == "profiles" and ...:
    ...
elif parts[0] in PROFILE_STATE_FILES:      # new
    ...
elif parts[0] == "profiles" and ...:       # new
    ...
# ← no else — falls through

If parts[0] is not in PROFILE_SCOPED_AREAS, not "profiles", and not in PROFILE_STATE_FILES (e.g. a file like README.md or scripts/foo), the code falls through to active_profile = _resolve_active_profile_name() and then if target_profile == active_profile — but target_profile was never assigned, so this raises NameError.

Suggested fix: add else: return None after the last elif block, restoring the catch-all for paths that aren't profile-scoped or profile state files.

…le state files (SOUL.md, config.yaml, .env, auth.json)

@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 narrowing the cross-profile guard to the existing classifier path. The premise remains valid on current main: classify_cross_profile_target() recognizes only skills/plugins/cron/memories at agent/file_safety.py:419-432, and the current test explicitly expects root config.yaml not to classify at tests/agent/test_file_safety_cross_profile.py:170-179.

Problems

  • The final PR commit restores the catch-all else: return None, resolving the prior review comment, but it adds no regression coverage for the four new state-file paths or both profile directions.
  • get_cross_profile_warning() renders every area as a directory at agent/file_safety.py:467; this change would make state-file warnings say config.yaml/ or SOUL.md/.

Suggested changes

  • Add focused parameterized classifier and write/patch guard tests for all four files.
  • Distinguish a state file from a scoped directory in the warning text and update the related tool-facing wording.

This is an automated hermes-sweeper review.

Comment thread agent/file_safety.py
area = parts[2]
elif parts[0] in PROFILE_STATE_FILES:
target_profile = "default"
area = parts[0]

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.

area is later rendered as a directory ({info['area']}/ in get_cross_profile_warning), so this produces config.yaml/ or SOUL.md/. Please distinguish state files from directory-scoped areas in the warning text.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Docker terminal backend lets file tools write to a sandbox-mirror copy of authoritative profile state (SOUL.md, etc.) — follow-up to #31290

4 participants