Non-interactive AskUserQuestion auto-reply and --json-stream#87
Non-interactive AskUserQuestion auto-reply and --json-stream#87kalil0321 wants to merge 1 commit into
Conversation
When interactive=False, AskUserQuestion returns a fixed message instructing the model to assume reasonable answers instead of blocking on questionary. Add --json-stream on agent and engineer for NDJSON progress on stdout plus a terminal result event. Default Cursor model is composer-2.5. Co-authored-by: kalil0321 <kalil0321@users.noreply.github.com>
|
|
5 similar comments
|
|
|
|
|
|
|
|
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/reverse_api/cli.py">
<violation number="1" location="src/reverse_api/cli.py:1966">
P2: `engineer --json-stream` does not return machine-readable output for missing `RUN_ID`; it still checks only `as_json` in the early validation path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| is_flag=True, | ||
| help="Emit NDJSON progress events on stdout during the run, then a final {\"event\":\"result\",...} line. Implies --no-interactive.", | ||
| ) | ||
| def engineer(run_id, prompt, fresh, model, output_dir, no_interactive, as_json, json_stream): |
There was a problem hiding this comment.
P2: engineer --json-stream does not return machine-readable output for missing RUN_ID; it still checks only as_json in the early validation path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/reverse_api/cli.py, line 1966:
<comment>`engineer --json-stream` does not return machine-readable output for missing `RUN_ID`; it still checks only `as_json` in the early validation path.</comment>
<file context>
@@ -1918,7 +1957,13 @@ def run_auto_capture(
+ is_flag=True,
+ help="Emit NDJSON progress events on stdout during the run, then a final {\"event\":\"result\",...} line. Implies --no-interactive.",
+)
+def engineer(run_id, prompt, fresh, model, output_dir, no_interactive, as_json, json_stream):
"""Run reverse engineering on a previous run."""
# `run_id` is declared optional at the click level so that wrappers using
</file context>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
reverse-api-engineer/src/reverse_api/cli.py
Line 1973 in 8840ced
Emit machine-readable error for missing RUN_ID in --json-stream
When engineer is invoked with --json-stream but no RUN_ID, this branch only checks as_json and falls back to plain-text usage output, so automation expecting NDJSON/JSON gets non-parseable stderr text and no terminal result event. Since this commit adds --json-stream as a scripted output mode, the missing-run-id validation path should treat json_stream the same as --json (including the event wrapper) to keep machine-output behavior consistent.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from .json_stream import attach_json_stream_to_engineer | ||
|
|
||
| attach_json_stream_to_engineer(engineer, json_event_sink) | ||
| json_event_sink({"event": "agent_started", "mode": mode_label, "url": url}) |
There was a problem hiding this comment.
agent_started event is undocumented and creates an asymmetric event stream. In the agent path, attach_json_stream_to_engineer emits run_started and then this line immediately emits agent_started, so consumers see two startup events. In the engineer path (via run_reverse_engineering), only run_started is emitted. The documentation and PR description list run_started as a common event type but make no mention of agent_started, which means any consumer parsing the agent stream will receive an unexpected extra event before tool_start.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/reverse_api/cli.py
Line: 1852
Comment:
**`agent_started` event is undocumented and creates an asymmetric event stream.** In the `agent` path, `attach_json_stream_to_engineer` emits `run_started` and then this line immediately emits `agent_started`, so consumers see two startup events. In the `engineer` path (via `run_reverse_engineering`), only `run_started` is emitted. The documentation and PR description list `run_started` as a common event type but make no mention of `agent_started`, which means any consumer parsing the `agent` stream will receive an unexpected extra event before `tool_start`.
How can I resolve this? If you propose a fix, please make it concise.| if hasattr(self._inner, "thinking"): | ||
| try: | ||
| self._inner.thinking(text, max_length=max_length) | ||
| except TypeError: | ||
| self._inner.thinking(text) | ||
| else: | ||
| self._inner.thinking(text) |
There was a problem hiding this comment.
else branch always raises AttributeError. The else block is entered only when hasattr(self._inner, "thinking") is False — meaning the inner UI does not have a thinking attribute — yet the very next line calls self._inner.thinking(text), which will always raise AttributeError. The intent was clearly to skip silently when the inner UI lacks the method; the call should be removed.
| if hasattr(self._inner, "thinking"): | |
| try: | |
| self._inner.thinking(text, max_length=max_length) | |
| except TypeError: | |
| self._inner.thinking(text) | |
| else: | |
| self._inner.thinking(text) | |
| if hasattr(self._inner, "thinking"): | |
| try: | |
| self._inner.thinking(text, max_length=max_length) | |
| except TypeError: | |
| self._inner.thinking(text) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/reverse_api/json_stream.py
Line: 75-81
Comment:
**Unreachable `else` branch always raises `AttributeError`.** The `else` block is entered only when `hasattr(self._inner, "thinking")` is `False` — meaning the inner UI does *not* have a `thinking` attribute — yet the very next line calls `self._inner.thinking(text)`, which will always raise `AttributeError`. The intent was clearly to skip silently when the inner UI lacks the method; the call should be removed.
```suggestion
if hasattr(self._inner, "thinking"):
try:
self._inner.thinking(text, max_length=max_length)
except TypeError:
self._inner.thinking(text)
```
How can I resolve this? If you propose a fix, please make it concise.| try: | ||
| self._inner.thinking(text, max_length=max_length) | ||
| except TypeError: | ||
| self._inner.thinking(text) |
There was a problem hiding this comment.
TypeError catch could silently swallow a real type error from inside thinking(). If the inner thinking() body raises TypeError for any reason other than the unexpected max_length kwarg, the handler will catch it and call self._inner.thinking(text) as a fallback, hiding the real error. Consider inspecting the signature once rather than catching by exception.
| try: | |
| self._inner.thinking(text, max_length=max_length) | |
| except TypeError: | |
| self._inner.thinking(text) | |
| import inspect | |
| sig = inspect.signature(self._inner.thinking) | |
| if "max_length" in sig.parameters: | |
| self._inner.thinking(text, max_length=max_length) | |
| else: | |
| self._inner.thinking(text) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/reverse_api/json_stream.py
Line: 76-79
Comment:
**Broad `TypeError` catch could silently swallow a real type error from inside `thinking()`.** If the inner `thinking()` body raises `TypeError` for any reason other than the unexpected `max_length` kwarg, the handler will catch it and call `self._inner.thinking(text)` as a fallback, hiding the real error. Consider inspecting the signature once rather than catching by exception.
```suggestion
import inspect
sig = inspect.signature(self._inner.thinking)
if "max_length" in sig.parameters:
self._inner.thinking(text, max_length=max_length)
else:
self._inner.thinking(text)
```
How can I resolve this? If you propose a fix, please make it concise.2 participants
Summary
--json,--no-interactive, or--json-streamis used,AskUserQuestionno longer opens questionary prompts. Each question receives a fixed auto-reply telling the model to assume reasonable answers or stop and ask the caller to rerun with a clearer prompt.--json-stream: New flag onagentandengineeremits NDJSON progress events on stdout (run_started,tool_start,tool_end,thinking,ask_user_skipped, etc.) and ends with{"event":"result",...}using the same schema as--json. Logs stay on stderr.cursor_modelfromcomposer-2tocomposer-2.5(bridge fallback included).SDK coverage
can_use_tool→_ask_user_questionsask_user_questiontool →_ask_user_questionsask_user_skippedwhen an ask-user-like tool starts (no bridge injection yet)Tests
tests/test_engineer.py— non-interactive stub + permission handlertests/test_cli_json_stream.py— NDJSON wiring for agent/engineerDocs
website/content/docs/cli/scripted-usage.mdxfor--json-stream(no headless doc changes).Summary by cubic
Make AskUserQuestion non-interactive by auto-replying when running with --json, --no-interactive, or --json-stream. Add --json-stream to agent and engineer for NDJSON progress on stdout, and update the default Cursor model to composer-2.5.
New Features
ask_user_skippedevent with a count. Applied across Claude/auto and Copilot; Cursor emits the skip event when an ask-user-like tool starts.--json-streamtoagentandengineer. Streams NDJSON events on stdout (e.g.,run_started,tool_start,tool_end,thinking,ask_user_skipped) and ends with{"event":"result",...}using the same schema as--json. Logs remain on stderr. Implies non-interactive mode.Migration
--json,--no-interactive, or--json-stream. Remove these flags to enable prompts.--json-streamshould parse NDJSON and read the finalresultevent for the outcome.cursor_modelis now composer-2.5. Overridecursor_modeltocomposer-2if you need the previous default.Written for commit 8840ced. Summary will update on new commits. Review in cubic
Greptile Summary
This PR adds
--json-stream(NDJSON progress events) to theagentandengineercommands, makesAskUserQuestionnon-interactive in scripted runs by auto-replying with a fixed message, and bumps the default Cursor model tocomposer-2.5._ask_user_questionsinBaseEngineernow routes to either the interactive questionary UI or a fixed stub reply depending onself.interactive; the same dispatcher is wired into Claude, Auto, and Copilot paths.--json-stream:json_stream.pyintroducesStreamingUIWrapperandattach_json_stream_to_engineerto intercept UI lifecycle calls and emit NDJSON events; theagentpath also emits an undocumentedagent_startedevent afterrun_started, creating a slightly asymmetric schema versus theengineerpath.composer-2.5default: Updated consistently acrossconfig.py,cli.py,cursor_engineer.py,run.mjs, docs, and tests.Confidence Score: 3/5
Safe to merge after fixing the
thinkingelse-branch crash; the rest of the change is well-scoped.The
StreamingUIWrapper.thinkingmethod contains an else branch that callsself._inner.thinking(text)immediately afterhasattrconfirmed the attribute does not exist — this will always raiseAttributeErrorif any inner UI lacks athinkingmethod. It won't crash in current usage if every concrete UI does exposethinking, but the logic is demonstrably wrong and the code path is untested.src/reverse_api/json_stream.py — the
thinkingmethod's else branch and TypeError catch both need attention.Important Files Changed
StreamingUIWrapper.thinkingwhere theelsebranch calls a method thathasattrjust confirmed doesn't exist, and uses broadTypeErrorcatching to probe method signatures._ask_user_questionsdispatcher that routes to interactive UI or a fixed non-interactive stub; logic is clean and both paths (interactive/non-interactive) are correctly handled.--json-streamflag toagentandengineer, wires the sink through call chains; theagentpath emits an undocumentedagent_startedevent in addition torun_started, creating an asymmetric event schema versus theengineerpath.json_event_sinkparameter torun_reverse_engineeringand attaches the stream wrapper after engineer construction; ordering is correct relative tostart_syncandgenerate.ask_user_skippedevent when a Cursor ask-user-like tool fires in non-interactive mode; note-only (no bridge injection) and clearly documented as such.AskUserQuestionhandling from_ask_user_interactiveto the new_ask_user_questionsdispatcher; straightforward and correct._ask_user_questionsswitch as auto_engineer; change is minimal and correct.run_agent_capture/run_engineerdirectly and do not exercise the realattach_json_stream_to_engineercall, so theagent_startedvsrun_startedinconsistency in the real code path is not caught._ask_user_questionsrouting tests; coverage is appropriate for the new branch.cursor_modelbumped fromcomposer-2tocomposer-2.5; consistent across all affected files.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add non-interactive AskUserQuestion auto..." | Re-trigger Greptile