Skip to content

fix: scope terminal approval exemptions - #69152

Open
trac3r00 wants to merge 1 commit into
NousResearch:mainfrom
trac3r00:fix/terminal-approval-scoped-workspace
Open

fix: scope terminal approval exemptions#69152
trac3r00 wants to merge 1 commit into
NousResearch:mainfrom
trac3r00:fix/terminal-approval-scoped-workspace

Conversation

@trac3r00

Copy link
Copy Markdown
Contributor

Summary

  • allow only strict read-only active-profile config inspection commands through terminal approval
  • allow only single-target recursive cleanup under the current Kanban task workspace
  • preserve hardline, sensitive-write, and ambiguous-command protections; clarify that a denied destructive action does not block read-only discovery

Verification

  • python -m pytest tests/tools/test_approval.py -q -o "addopts=" -k "KanbanWorkspaceRecursiveCleanup or ReadOnlyProfileConfigInspection or ApprovalTimeoutIsNotConsent"
  • python -m pytest tests/tools/test_tirith_security.py -q -o "addopts="
  • python -m pytest tests/agent/test_tool_guardrails.py -q -o "addopts="
  • python -m py_compile tools/approval.py

Known baseline: the full tests/tools/test_approval.py run has one pre-existing macOS tempdir canonicalization failure in test_nonrecursive_verification_artifact_cleanup; this PR does not touch that path.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 22, 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 narrowing the proposed command shapes and adding negative cases. The recursive-cleanup exemption needs one safety boundary before it can be salvaged.

Problems

  • tools/approval.py:2009 treats every descendant of HERMES_KANBAN_WORKSPACE as disposable. Kanban explicitly supports preserved dir: and worktree workspaces (website/docs/user-guide/features/kanban.md:66-68), so this would auto-allow recursive deletion inside a user source tree. Existing Kanban cleanup deliberately refuses paths outside managed scratch roots (tests/hermes_cli/test_kanban_db.py:2535-2563).
  • The new cleanup tests exercise detect_dangerous_command, not the terminal gate that executes the full policy ordering (tools/approval.py:3335).

Suggested changes

  • Gate the exemption on verified managed-scratch containment/workspace kind, rather than the task workspace environment variable alone.
  • Add check_all_command_guards coverage for scratch versus preserved workspaces and for a matching approvals.deny rule.

Automated hermes-sweeper review.

Comment thread tools/approval.py
try:
return os.path.commonpath((workspace, target)) == workspace
except ValueError:
return False

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.

HERMES_KANBAN_WORKSPACE is also used for preserved dir: and worktree tasks, not just disposable scratch tasks (website/docs/user-guide/features/kanban.md:66-68). A descendant-only check would auto-allow rm -rf inside a user source tree. Please require the same managed-scratch containment/workspace-kind proof used by Kanban's automatic cleanup.

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

Labels

area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants