Skip to content

fix(patch): report no-op edits clearly - #25612

Closed
alaamohanad169-ship-it wants to merge 3 commits into
NousResearch:mainfrom
alaamohanad169-ship-it:fix/patch-noop-reporting
Closed

fix(patch): report no-op edits clearly#25612
alaamohanad169-ship-it wants to merge 3 commits into
NousResearch:mainfrom
alaamohanad169-ship-it:fix/patch-noop-reporting

Conversation

@alaamohanad169-ship-it

Copy link
Copy Markdown
Contributor

Summary

  • Add noop/message fields to PatchResult
  • Report identical replace-mode edits as successful no-ops without writing
  • Treat V4A pure-context/no-effective-change updates as successful no-ops with guidance

The _approval_callback method in HermesCLI (cli.py) hardcoded timeout = 60
instead of reading the approvals.timeout config value. This meant the config
setting was silently ignored for CLI interactive approval prompts.

One-line change: replace timeout = 60 with
timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 60)).

Other approval paths (tools/approval.py and hermes_cli/callbacks.py) already
read the config correctly.
…g multiple-choice

When choices were provided, awaiting_text was set to False, which meant
get_pending_for_session() would never match the entry for text-fallback
adapters like Mattermost. The user's typed reply was never intercepted,
causing the agent to block until timeout.

Button-based adapters (e.g. Telegram) resolve via resolve_gateway_clarify
directly and don't check awaiting_text, so they're unaffected.

Now awaiting_text is always True so both button and text-fallback paths work.
- Add explicit noop/message fields to PatchResult
- Report identical replace-mode edits (old_string == new_string) as
  successful no-ops without writing to disk
- Treat V4A pure-context/no-effective-change updates as successful no-ops
  with guidance message
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists labels May 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #25564 — identical fix (PatchResult.noop field for no-op edits). Same title and approach.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the contribution — closing because of scope creep that obscures the real fix:

  1. The legitimate piece is the no-op reporting in patch (recognizing identical strings + V4A empty-modify). That's worth landing on its own.

  2. The PR also bundles an unrelated cli.py approvals.timeout change (duplicate of fix(cli): read approvals.timeout from config in CLI approval callback #25609, which is itself a duplicate of fix: read approvals.timeout from config in CLI approval callback #25559) and a clarify_gateway.py change (duplicate of your own fix(clarify): set awaiting_text=True for all clarify entries including multiple-choice #25610).

If you want to land the no-op reporting piece, please re-submit it as a focused PR touching only the patch tool — drop the cli.py and clarify_gateway.py hunks. Happy to look at the focused version.

@teknium1 teknium1 closed this May 14, 2026
@alaamohanad169-ship-it
alaamohanad169-ship-it deleted the fix/patch-noop-reporting branch June 6, 2026 14:55
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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants