fix: context-aware command visibility for agent executor and planner - #56
fix: context-aware command visibility for agent executor and planner#56sanchit056 wants to merge 1 commit into
Conversation
- Executor: re-scope the ReAct agent's available_commands to the current context whenever it changes (set_current_*, go_up, reset_context) via a context-change observer on the Workflow, so the agent never acts on a stale command list after a mid-trajectory switch. - Planner: build_query_with_next_steps now uses get_all_contexts_command_display_text so the planner sees every available context's commands
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The context change listener mechanism on
Workflowonly ever appends to_context_change_listenersand doesn’t expose any way to deregister or use weak references, which could cause long‑lived references to execution contexts/agents; consider adding a remove/unsubscribe API or using weakrefs to avoid leaks for short‑lived workflows. - In
_notify_context_change, you currently log only the exception message ({exc}); capturingexc_info=Trueor logging the full traceback would make diagnosing listener failures much easier while still honoring the “must never break context switching” requirement.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The context change listener mechanism on `Workflow` only ever appends to `_context_change_listeners` and doesn’t expose any way to deregister or use weak references, which could cause long‑lived references to execution contexts/agents; consider adding a remove/unsubscribe API or using weakrefs to avoid leaks for short‑lived workflows.
- In `_notify_context_change`, you currently log only the exception message (`{exc}`); capturing `exc_info=True` or logging the full traceback would make diagnosing listener failures much easier while still honoring the “must never break context switching” requirement.
## Individual Comments
### Comment 1
<location path="fastworkflow/command_metadata_api.py" line_range="680-681" />
<code_context>
+ sections.append("\n".join(parts))
+
+ return "\n\n".join(sections)
+ except Exception as e: # callers must never break on metadata assembly
+ return base_text
+
@staticmethod
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Log or surface minimal details when falling back after an exception
Catching all exceptions and returning `base_text` protects callers, but it also hides routing/CRD problems entirely. Please add at least a debug or warning log (with sensitive details redacted) so misconfigurations can be detected while still maintaining the "never break" behavior for metadata assembly.
Suggested implementation:
```python
return "\n\n".join(sections)
except Exception as e: # callers must never break on metadata assembly
logger.warning(
"Failed to assemble command metadata; falling back to base_text.",
exc_info=True,
)
return base_text
```
To make this compile and follow existing logging conventions, ensure that:
1. A module-level logger is defined, for example:
`logger = logging.getLogger(__name__)`
2. The `logging` module is imported at the top of `fastworkflow/command_metadata_api.py` if it is not already:
`import logging`
If this file already uses a different logging helper or framework (e.g. a shared `logger` from your codebase), replace the `logger` usage in this patch with that existing logger instance instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| except Exception as e: # callers must never break on metadata assembly | ||
| return base_text |
There was a problem hiding this comment.
suggestion (bug_risk): Log or surface minimal details when falling back after an exception
Catching all exceptions and returning base_text protects callers, but it also hides routing/CRD problems entirely. Please add at least a debug or warning log (with sensitive details redacted) so misconfigurations can be detected while still maintaining the "never break" behavior for metadata assembly.
Suggested implementation:
return "\n\n".join(sections)
except Exception as e: # callers must never break on metadata assembly
logger.warning(
"Failed to assemble command metadata; falling back to base_text.",
exc_info=True,
)
return base_textTo make this compile and follow existing logging conventions, ensure that:
- A module-level logger is defined, for example:
logger = logging.getLogger(__name__) - The
loggingmodule is imported at the top offastworkflow/command_metadata_api.pyif it is not already:
import logging
If this file already uses a different logging helper or framework (e.g. a sharedloggerfrom your codebase), replace theloggerusage in this patch with that existing logger instance instead.
|
closed. I made the recommended change to the PR and resubmitted as 2.30.1 |
…#59) * fix: v2.30.1 — context-aware command visibility for agent and planner (fix-ry7) Re-scope the ReAct executor's available_commands on context switches and give the planner an all-contexts command map. Addresses Sourcery review on upstream PR #56 (listener unsubscribe/WeakMethod, exc_info logging). Co-authored-by: Cursor <cursoragent@cursor.com> * fix: harden context-change agent refresh for WEC hosts Resolve the ReAct agent via workflow_tool_agent or _workflow_tool_agent, and fall back to app_workflow when the active stack is empty so WEC context switches still re-scope available_commands. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Dhar Rawal <drawal@radiantlogic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Two related fixes that make the command lists shown to the agent and the planner context-aware:
Summary by Sourcery
Make agent and planner command visibility context-aware to avoid stale or incomplete command lists.
Enhancements: