Add --timeout flag to run-sql, document env var configuration - #1
Merged
Conversation
- cli.py: new --timeout <seconds> option on run-sql, overriding sql_guard.DEFAULT_TIMEOUT_SECONDS for that call only. Rejects non-positive values via a validation callback (same pattern as --format). Lets the Skill (or a human) retry a query that's already minimal but legitimately slow, instead of only being able to narrow the query or accept the default ~45s limit. - tests: pass-through (custom value and default-when-omitted, via a mocked commands.run_sql), validation rejection, and a real end-to-end confirmation that a short --timeout genuinely cuts execution short (view wrapping range() outside the guard, same pattern used for the existing timeout tests). 168 -> 172 tests. - SKILL.md: documents --timeout on run-sql, updates the timeout error-type guidance and behavioral rule 5 to distinguish "narrow the query" from "retry with --timeout" depending on whether the query is already minimal. - README.md / SKILL.md: added a consolidated environment-variable configuration section (OTAI_CACHE_DIR, OTAI_BASE_URI, OTAI_LOG_LEVEL) in both, replacing the previously scattered mentions. Verified end-to-end against the real S3 bucket: a real slow cross join times out early with a short --timeout, and a real query succeeds with an explicit override. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Typer/Click's error panel wraps its message text differently depending on the terminal width Rich detects, which differs between a local terminal and the CI runner - splitting "positive number" across the box border there. Matches this file's own established convention for invalid-option tests (test_unknown_format_is_rejected): check result.exit_code only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
--timeout <seconds>flag torun-sql, letting a caller (typically the Skill, on behalf of an LLM) override the default ~45s query timeout for one specific call — for a query that's already minimal but legitimately slow (e.g. a full aggregate over a very large dataset), rather than a mistake that needs narrowing.cli.py: new--timeoutoption, validated to be positive, passed through ascommands.run_sql's existingtimeout_secondsparameter (already plumbed throughsql_guard.run_guarded_query, just not previously exposed at the CLI layer).commands.run_sql), validation rejection, and a real end-to-end confirmation that a short--timeoutgenuinely cuts execution short (a view wrappingrange()created outside the guard, same pattern as the existing timeout tests). 168 → 172 tests.SKILL.md: documents--timeoutonrun-sql, and updates thetimeouterror-type guidance / behavioral rule 5 to distinguish "narrow the query" from "retry with--timeout" depending on whether the query is already minimal.README.md/SKILL.md: added a consolidated environment-variable configuration section (OTAI_CACHE_DIR,OTAI_BASE_URI,OTAI_LOG_LEVEL), replacing previously scattered mentions.Branched from this repo's current
main(e136e64), so the diff is scoped to just this feature and preserves theUpdate SKILL.mdinvocation-URL change.Test plan
make lintpassesmake test(172 tests) passesprek run --all-filespasses locally--timeout, and a real query succeeds with an explicit override