Skip to content

fix(approval): accept "auto" as alias for "smart" approval mode#60567

Open
whichguy wants to merge 1 commit into
NousResearch:mainfrom
whichguy:fix/approval-mode-auto-alias-clean
Open

fix(approval): accept "auto" as alias for "smart" approval mode#60567
whichguy wants to merge 1 commit into
NousResearch:mainfrom
whichguy:fix/approval-mode-auto-alias-clean

Conversation

@whichguy

@whichguy whichguy commented Jul 7, 2026

Copy link
Copy Markdown

fix(approval): accept "auto" as alias for "smart" approval mode

THESIS: approvals.mode: auto was silently rejected by _normalize_approval_mode
(valid: manual/smart/off only) and fell back to "manual", causing every
DANGEROUS_PATTERN match to prompt the user — the exact opposite of what
"auto" implies. Users writing "auto" expect automatic approval, not more
prompts.

LEARNINGS:

  • Silent fallback to "manual" is the worst possible default for a user who
    wrote "auto" — it produces the opposite behavior from what the value name
    implies. A loud error would have been better than a quiet wrong default.
  • "auto" is the most intuitive value name for users who want LLM-based
    auto-approval without knowing Hermes internal mode names. Rejecting it
    forces users to discover "smart" through documentation or error logs.
  • The web_server.py config schema had stale options that did not match the
    actual valid values, compounding the confusion.

REFERENCES:

  • _normalize_approval_mode in tools/approval.py
  • Test: test_auto_is_alias_for_smart in tests/tools/test_approval.py
  • Config schema: hermes_cli/web_server.py approvals.mode options
  • Tips: hermes_cli/tips.py approval mode hint

THESIS: approvals.mode: auto was silently rejected by _normalize_approval_mode
(valid: manual/smart/off only) and fell back to "manual", causing every
DANGEROUS_PATTERN match to prompt the user — the exact opposite of what
"auto" implies. Users writing "auto" expect automatic approval, not more
prompts.

LEARNINGS:
- Silent fallback to "manual" is the worst possible default for a user who
  wrote "auto" — it produces the opposite behavior from what the value name
  implies. A loud error would have been better than a quiet wrong default.
- "auto" is the most intuitive value name for users who want LLM-based
  auto-approval without knowing Hermes internal mode names. Rejecting it
  forces users to discover "smart" through documentation or error logs.
- The web_server.py config schema had stale options that did not match the
  actual valid values, compounding the confusion.

REFERENCES:
- _normalize_approval_mode in tools/approval.py
- Test: test_auto_is_alias_for_smart in tests/tools/test_approval.py
- Config schema: hermes_cli/web_server.py approvals.mode options
- Tips: hermes_cli/tips.py approval mode hint
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/tools Tool registry, model_tools, toolsets sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists labels Jul 8, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

This PR is titled as a one-line approval-mode fix but its diff is +18/-2085 across 20 files: the tools/approval.py change is ~18 lines, while the branch deletes all 16 .github/workflows/*.yml files (ci.yml, tests.yml, typecheck.yml, lint.yml, osv-scanner.yml, supply-chain-audit.yml, docker.yml, upload_to_pypi.yml, uv-lockfile-check.yml, etc.), all of which currently exist on main. Merging as-is would remove CI, OSV, and supply-chain security scanning. This looks like a stale/botched branch rather than an intentional change. Please rebase onto current main and resubmit with ONLY the approvals.mode alias change scoped in. Note the approval-alias portion also overlaps the already-merged #54469 (which validates modes and maps unknown values to manual) and issue #4261 — worth reconciling the intended auto behavior with that. Flagging needs-decision for a maintainer.

@egilewski

Copy link
Copy Markdown
Contributor

too large to review safely

This PR changes 2097 production lines before tests and docs. Please split it or add a focused justification if it should stay together.

Signed: GPT-5.5-low in Codex

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

Labels

comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants