Skip to content

test(approval): cover approval orchestration paths - #47563

Open
wgu9 wants to merge 3 commits into
NousResearch:mainfrom
wgu9:test/approval-coverage
Open

test(approval): cover approval orchestration paths#47563
wgu9 wants to merge 3 commits into
NousResearch:mainfrom
wgu9:test/approval-coverage

Conversation

@wgu9

@wgu9 wgu9 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add behavior tests for approval session/yolo state, gateway queue resolution, and session cleanup.
  • Cover approval config fallback/persistence helpers and callback fail-closed behavior.
  • Cover check_dangerous_command orchestration for containers, hardline blocks, cron handling, gateway approval payloads, and CLI deny/session/always choices.

Coverage

  • tools/approval.py: 57% -> 72% in the targeted coverage run.

Tests

  • scripts/run_tests.sh tests/tools/test_approval.py
  • .venv/bin/python -m pytest tests/tools/test_approval.py --cov=tools.approval --cov-report=term-missing
  • .venv/bin/ruff check tests/tools/test_approval.py

Closes #36523

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have labels Jun 17, 2026
@wgu9
wgu9 force-pushed the test/approval-coverage branch from cc7426c to 3f00dc7 Compare July 3, 2026 10:29
@wgu9

wgu9 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this on current main and resolved the import conflict in tests/tools/test_approval.py by keeping both upstream approval-mode helpers and this PR's cron/yolo/gateway coverage helpers.

Validation:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/tools/test_approval.py -q -> 318 passed
  • python -m ruff check tests/tools/test_approval.py
  • python -m py_compile tests/tools/test_approval.py
  • git diff --check upstream/main...HEAD

@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 adding focused approval coverage. One assertion needs updating for current main.

Problems

  • tests/tools/test_approval.py:321 expects _await_gateway_decision() to return exactly two keys. Current main returns {"resolved": ..., "choice": ..., "reason": ...} at tools/approval.py:2556 (the reason field came from cb6c47af08), so this test would fail after salvage.

Suggested changes

  • Assert the required fields individually at tests/tools/test_approval.py:321resolved and choice—and optionally assert reason is None, rather than asserting the entire return dict.

Automated hermes-sweeper review.

Comment thread tests/tools/test_approval.py Outdated
@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@wgu9
wgu9 force-pushed the test/approval-coverage branch from 8a41145 to c4518cf Compare July 20, 2026 07:04
@wgu9

wgu9 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. The gateway-decision assertion now checks resolved, choice, and reason individually, matching the current return contract. The 20 tests added by this PR pass, and git diff --check upstream/main...HEAD passes. The full approval test file has one unrelated existing macOS /tmp path-classification failure; all 331 other tests pass.

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

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve test coverage: tools/approval.py

3 participants