Repository navigation
docs: document the live tool rail and add capped execution examples - #12
Conversation
|
Claude finished @epanonymous's task in 1m 35s —— View job PR #12 review: live tool rail docs and capped execution examples
SummaryThis is a careful PR. Paid steps and free steps are kept apart, the API key is only sent on run and poll requests, and redirects are rejected so the key can't be forwarded. There is never more than one run POST, and the docs say clearly which responses were captured live and which are templates. Credits and USD are treated as equal, which matches Findings1. Medium: unexpected poll statuses abort a run that is still going.
2. Low: Python drops the reason for a 402 error.
3. Low: errors from the run POST don't say "do not resubmit".
4. Test coverage: the TS suite has gaps the Python suite doesn't.
5. Docs: hosted MCP names were not checked live.
Smaller points
About the earlier fix-round commentThe "three-model Result" comment mentioned above still isn't on this PR. This review can serve as the findings list instead: items 1–4 are confirmed by reading the code, and item 5 is a documentation-consistency point. I made no code changes or commits. |
|
Fix round blocked: the referenced three-model Result comment is not present. Checked the PR comments, issue-comment API, submitted reviews, and inline review comments; only the Claude error comment is available. Please post the Confirmed and Disputed findings so each fix can be mapped to its commit and regression test. Baseline validation: Python unittest and Node test suites pass (20 passed, 2 live smoke tests skipped). No lint or typecheck commands are configured in this repository. No code changes or commits were made, and nothing was pushed or merged. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if result['status'] == 'succeeded': | ||
| return result | ||
| if result['status'] != 'running': | ||
| raise RuntimeError(f'Run {run_id}: {result["status"]}; do not resubmit') |
There was a problem hiding this comment.
Non-terminal statuses are handled as failures, and the resume hint is lost. The poll vocabulary was never captured live (the PR says so). If the server returns queued/pending/ready, or uses uppercase like RUNNING (discovery uses SYNC/DEVA), the loop raises Run …: queued; do not resubmit while the run is still going and still spending. That message also leaves out the resume GET /runs/{id} path that every other poll-exit error includes.
Also, result['status'] raises a bare KeyError if the field is missing, so the run ID is lost here too. The TS version doesn't have this problem.
Suggestion: only treat failed (plus any other documented terminal states) as terminal, keep polling on unknown statuses, and include the resume path in every error raised after on_run:
status = result.get('status')
if status == 'succeeded':
return result
if status == 'failed':
raise RuntimeError(f'Run {run_id} failed; do not resubmit')
# Unknown or non-terminal status: keep polling until max_polls.Same applies to examples/typescript/tools_loop.ts:60-61.
| req = Request(BASE + path, data=None if body is None else json.dumps(body).encode(), | ||
| headers=headers, method=method) | ||
| # Never follow redirects or retry POSTs; credentials stay on the original request. | ||
| with _opener.open(req, timeout=60) as response: |
There was a problem hiding this comment.
The 402 reason is dropped in Python. tools.md tells users to "Read the error reason" for 402 (cap_preflight with max_allowed_cap_credits, or cap_floor_above_balance). Here urllib raises HTTPError and never reads the body, so the CLI only prints HTTP Error 402: Payment Required. The TS request includes await response.text(), so the two examples behave differently.
Consider catching HTTPError, reading error.read(), and re-raising with the body. Close the response either way.
| # One-call task budget: reserve the entire cap before this single POST. | ||
| # Do not release that reservation or submit again on an ambiguous failure. | ||
| code, result = http('POST', '/run/' + path_slug, body, key) |
There was a problem hiding this comment.
Nit: this comment says the cap is reserved, but nothing in run_loop reserves anything. The one-run limit only holds because the function makes a single POST. A caller who loops over run_loop gets no protection. I'd reword it as a caller responsibility (e.g. "Callers must deduct budget from their task budget before calling, and keep it deducted on any exception"), or add an optional ledger/reserve hook that the tests can check.
Related: a transport error on this POST (timeout after the request was sent, or a 503) propagates as a raw TimeoutError/HTTPError with no "do not resubmit" wording, unlike every other failure path. Wrapping it the same way would make the no-retry contract show up in the error itself. The same applies to tools_loop.ts:44.
| const { error, calls } = await execute(t, [[202, { run_id: 'r1' }], [200, { status: 'running' }]], undefined, 0.01, 1); | ||
| assert.match(String(error), /resume GET \/runs\/r1/); | ||
| assert.equal(calls.length, 5); | ||
| }); |
There was a problem hiding this comment.
The TS suite is missing tests the Python suite has:
- No redirect test.
redirect: 'error'intools_loop.ts:15is the only thing keeping the Bearer key off a redirected request, and nothing would catch a regression. A smallnode:httpserver returning 301/302/307/308 (likeredirect_serverin the Python tests) would cover it. - No
maxPollsvalidation test (Python hastest_invalid_poll_limits_do_not_call_http). - The "no retry" cases only check
calls.lengthandassert.ok(error). Addingassert.equal(calls.filter(c => c.method === 'POST' && c.url.includes('/run/')).length, 1)would state the main invariant directly.
|
|
||
| Use `list_resources` → `inspect_resource` → `estimate_resource` → `run_resource` → `get_run`. Discovery, inspection, and estimation are free. Poll an asynchronous run until `succeeded` or `failed`; read synchronous output directly. Check MCP `result.isError` as well as HTTP status. Never automatically retry `run_resource`. | ||
|
|
||
| Hosted MCP uses `max_total_charge_credits` for the cap and `min_cap_credits` in inspection; REST uses `maxTotalChargeUsd` and `min_cap_usd`. One credit equals one USD. Read the [tool loop guide](tools.md) for budget guards and failure handling. |
There was a problem hiding this comment.
The PR description says an authenticated hosted tools/list call was not available, so the tool names and the argument names (max_total_charge_credits, min_cap_credits) come from the registry source, not from a live response. api-reference.md already labels the run/poll contracts as "not captured". A matching one-line caveat here ("Argument names from the server registry; confirm with tools/list") would make the doc's evidence standard consistent.
Minor: deva_resources_catalog was removed from the stdio table (line 125), but line 47 still calls it "the existing … legacy discovery" tool. Keeping it in the table with a "(legacy)" label would avoid the mismatch.
|
Addressed the supplied review in commit 033e01a (pushed to
Validation:
There are no repository lint/typecheck scripts, so those tools/configuration were installed outside the repository for local validation. Local HTTP test servers are shut down and joined by the tests. PR remains open; no merge or force-push. At the pushed head, the |
Replace legacy catalog guidance with the live discover → inspect → estimate → run → poll loop, with curl-captured responses and capped Python/TypeScript examples.
Implementation mapping:
docs/tools.md, and link it from the README. Discovery, inspect, and estimate responses were captured live on 2026-10-30. Catalog still returns 200 and is labeled legacy.https://api.deva.me/v1/mcp, and five upcoming npm stdiodeva_tools_*tools, explicitly marked for the next release.Live route correction:
POST /run/{slug}is the working endpoint; bare/runreturned 404.POST /estimate/{slug}is preferred; the body-slug/estimatealias still works.Validation results: 20 mocked tests passed. Two live smoke tests skipped because
DEVA_API_KEYwas not set. Unauthenticated run/poll probes returned 401; successful authenticated execution/poll responses could not be captured and are not represented as live captures. No paid execution was started. Hosted names were checked against the current tool registry; an authenticated hostedtools/listcall was not available. npm publishing and server changes are outside this documentation PR.