Repository navigation
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
![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 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/command_llm.cc">
<violation number="1" location="src/command_llm.cc:788">
P2: `--json` exposes the timeout duration only inside the prose `error` field, so callers cannot consume it structurally. Store the duration on `LLMTimeoutError` and emit a dedicated JSON field instead of interpolating it into `what()`.
(Based on your team's feedback about structured exception data.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // What the backend says about a failure is its own business, so whether the | ||
| // time ran out is read off the clock rather than off the message | ||
| if (std::chrono::steady_clock::now() - started >= timeout) { | ||
| throw LLMTimeoutError{"The completion request did not answer within " + |
There was a problem hiding this comment.
P2: --json exposes the timeout duration only inside the prose error field, so callers cannot consume it structurally. Store the duration on LLMTimeoutError and emit a dedicated JSON field instead of interpolating it into what().
(Based on your team's feedback about structured exception data.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/command_llm.cc, line 788:
<comment>`--json` exposes the timeout duration only inside the prose `error` field, so callers cannot consume it structurally. Store the duration on `LLMTimeoutError` and emit a dedicated JSON field instead of interpolating it into `what()`.
(Based on your team's feedback about structured exception data.) </comment>
<file context>
@@ -777,7 +777,21 @@ auto sourcemeta::jsonschema::llm(const sourcemeta::core::Options &options)
+ // What the backend says about a failure is its own business, so whether the
+ // time ran out is read off the clock rather than off the message
+ if (std::chrono::steady_clock::now() - started >= timeout) {
+ throw LLMTimeoutError{"The completion request did not answer within " +
+ std::to_string(timeout.count()) + " seconds",
+ std::string{url}};
</file context>
| sourcemeta::core::HTTPResponse response; | ||
| try { | ||
| response = request.send(); | ||
| } catch (const sourcemeta::core::HTTPError &) { |
There was a problem hiding this comment.
TODO: We probably need a better upstream error for this...
🤖 Augment PR SummaryImproves timeout reporting for the 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } catch (const sourcemeta::core::HTTPError &) { | ||
| // What the backend says about a failure is its own business, so whether the | ||
| // time ran out is read off the clock rather than off the message | ||
| if (std::chrono::steady_clock::now() - started >= timeout) { |
There was a problem hiding this comment.
Could you check the silent-endpoint case on macOS at src/command_llm.cc:787? The Darwin backend leaves timeoutIntervalForRequest at its 60-second default, so the request fails before the default 120-second timeout and bypasses both LLMTimeoutError and the new guidance.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // What the backend says about a failure is its own business, so whether the | ||
| // time ran out is read off the clock rather than off the message | ||
| if (std::chrono::steady_clock::now() - started >= timeout) { | ||
| throw LLMTimeoutError{"The completion request did not answer within " + |
There was a problem hiding this comment.
Could you check the late response-size failure case on Windows at src/command_llm.cc:788? WinHTTP uses per-operation timeouts, so a slowly arriving response can exceed the 16 MiB limit after timeout; this discards the actual response-size HTTPError and incorrectly advises waiting longer.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
This branch has not been deployed
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com