Skip to content

fix(agent): allow background review to read files - #40007

Closed
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix-background-review-read-file
Closed

fix(agent): allow background review to read files#40007
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix-background-review-read-file

Conversation

@yinkev

@yinkev yinkev commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #40003
Supersedes stale #27422, which targeted the old run_agent.py path and did not include tests.

Summary

  • allow the background review runtime whitelist to call the read-only read_file tool
  • keep write/search/terminal/web/delegation tools denied
  • update review guidance to say read_file is only for read-only inspection when available, with a privacy guard against copying local file contents into durable skills
  • add regression coverage for the whitelist and review prompt

Compatibility notes

Tests

  • uv run --extra dev python -m pytest -q tests/run_agent/test_background_review_toolset_restriction.py
  • uv run --extra dev python -m ruff check agent/background_review.py tests/run_agent/test_background_review_toolset_restriction.py
  • uv run --extra dev python -m py_compile agent/background_review.py
  • git diff --check

@daimon-nous daimon-nous Bot added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jun 5, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused implementation and regression coverage. The current code still has the deny-wall this patch targets (agent/background_review.py:806-844), but this change revives an intentionally rejected design direction.

Problems

  • The maintainer decision on #27422 says the review fork's memory+skills-only whitelist is intentional: skill_view is the supported read path for SKILL.md and its linked skill assets, while arbitrary-file reads require a broader file-operations safety-boundary design.
  • read_file accepts absolute, relative, and home-relative paths (tools/file_tools.py:2043-2053), so the proposed prompt-only privacy instruction does not provide that requested boundary.

Suggested changes

  • Retain skill_view for skill-owned content. If arbitrary external-file inspection is reconsidered, propose it as a broader, explicitly bounded design with end-to-end safety coverage rather than a one-off whitelist exception.

This is an automated hermes-sweeper review.

@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 labels Jul 14, 2026
@yinkev

yinkev commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing because the maintainer review confirmed that arbitrary read_file access in the background-review fork conflicts with the intentionally narrow memory+skills safety boundary. A future reconsideration would need a broader bounded file-access design rather than this whitelist exception.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: background-review fork can't read an external file — model calls skill_manage(action="read_file") and gets "Unknown action"

2 participants