feat(skills): add Kiro local account importer#17
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the kiro-local-account-importer skill, which enables safe local importing of Kiro CLI accounts into StaticFlow. It includes a Python script to discover and parse local SQLite databases, apply scheduling and proxy configurations, and validate accounts against active US proxies. The feedback highlights several critical areas to improve the script's robustness and correctness, such as fixing a loop-control bug during proxy validation, handling potential database and network exceptions, removing dead code, and preventing crashes from null database values.
| if not isinstance(balance, dict) or not balance_has_minimum_remaining( | ||
| balance, args.minimum_remaining_credits | ||
| ): | ||
| attempt["error"] = "remaining credits below minimum" | ||
| break |
There was a problem hiding this comment.
If balance is not a dictionary (for example, if the API returns an empty response or None), the current check will set the error to 'remaining credits below minimum' and break out of the proxy validation loop entirely. This prevents the script from trying other active proxies. Instead, an invalid/non-dict response should be treated as a proxy/network failure (continuing to the next proxy), and the loop should only be broken if we successfully retrieved a valid balance dictionary and confirmed that the credits are below the minimum.
| if not isinstance(balance, dict) or not balance_has_minimum_remaining( | |
| balance, args.minimum_remaining_credits | |
| ): | |
| attempt["error"] = "remaining credits below minimum" | |
| break | |
| if not isinstance(balance, dict): | |
| attempt["error"] = "invalid balance response" | |
| continue | |
| if not balance_has_minimum_remaining( | |
| balance, args.minimum_remaining_credits | |
| ): | |
| attempt["error"] = "remaining credits below minimum" | |
| break |
| created = request_json( | ||
| "POST", args.admin_base_url, "/admin/kiro-gateway/accounts", args.admin_token, body | ||
| ) | ||
| result["created_name"] = created.get("name") if isinstance(created, dict) else auth.name |
There was a problem hiding this comment.
To ensure robustness against any server-side normalization or modification of the account name during creation, we should use the created_name returned by the server for all subsequent operations (such as PATCH, POST /balance, and DELETE) instead of assuming it is exactly auth.name.
| created = request_json( | |
| "POST", args.admin_base_url, "/admin/kiro-gateway/accounts", args.admin_token, body | |
| ) | |
| result["created_name"] = created.get("name") if isinstance(created, dict) else auth.name | |
| created = request_json( | |
| "POST", args.admin_base_url, "/admin/kiro-gateway/accounts", args.admin_token, body | |
| ) | |
| created_name = created.get("name") if isinstance(created, dict) else auth.name | |
| result["created_name"] = created_name |
| def choose_proxy( | ||
| proxies: list[dict[str, Any]], | ||
| account_counts: dict[str, int], | ||
| latencies: dict[str, float], | ||
| balance_penalty_ms: float, | ||
| ) -> dict[str, Any] | None: | ||
| ranked = rank_proxies(proxies, account_counts, latencies, balance_penalty_ms) | ||
| if not ranked: | ||
| return None | ||
| selected = ranked[0] | ||
| account_counts[str(selected["id"])] = account_counts.get(str(selected["id"]), 0) + 1 | ||
| return selected |
There was a problem hiding this comment.
The function choose_proxy is defined but never called anywhere in the script or tests. If it is dead code, it should be removed to keep the codebase clean and maintainable.
| conn = sqlite3.connect(str(path)) | ||
| try: | ||
| profile_arn = load_profile_arn(conn) |
There was a problem hiding this comment.
If the SQLite file is corrupted, locked, or not a valid database, sqlite3.connect or subsequent queries can raise sqlite3.Error (such as sqlite3.DatabaseError or sqlite3.OperationalError). Since these are not caught, they will crash the entire import script. Wrapping these database operations in a try...except sqlite3.Error block and raising a descriptive ValueError will make the script much more robust.
try:
conn = sqlite3.connect(str(path))
except sqlite3.Error as exc:
raise ValueError(f"Failed to open SQLite database {path}: {exc}") from exc
try:
profile_arn = load_profile_arn(conn)| def query_auth_kv(conn: sqlite3.Connection, keys: tuple[str, ...]) -> str | None: | ||
| for key in keys: | ||
| row = conn.execute("SELECT value FROM auth_kv WHERE key = ? LIMIT 1", (key,)).fetchone() | ||
| if row: | ||
| return str(row[0]) | ||
| return None |
There was a problem hiding this comment.
If row[0] is None (which represents a NULL value in the database), str(row[0]) will return the string "None". Then, load_json("None", ...) will be called, which raises a ValueError because "None" is not a valid JSON object. To prevent this, we should check if row[0] is not None before converting it to a string.
| def query_auth_kv(conn: sqlite3.Connection, keys: tuple[str, ...]) -> str | None: | |
| for key in keys: | |
| row = conn.execute("SELECT value FROM auth_kv WHERE key = ? LIMIT 1", (key,)).fetchone() | |
| if row: | |
| return str(row[0]) | |
| return None | |
| def query_auth_kv(conn: sqlite3.Connection, keys: tuple[str, ...]) -> str | None: | |
| for key in keys: | |
| row = conn.execute("SELECT value FROM auth_kv WHERE key = ? LIMIT 1", (key,)).fetchone() | |
| if row and row[0] is not None: | |
| return str(row[0]) | |
| return None |
| for item in dirs: | ||
| root = Path(item).expanduser() | ||
| for path in root.rglob("*"): | ||
| if path.is_file() and path.suffix.lower() in {".sqlite", ".sqlite3", ".db"}: | ||
| discovered.append(path) |
There was a problem hiding this comment.
Iterating over root.rglob("*") can raise OSError (such as PermissionError) if the script encounters directories with restricted permissions. This will crash the entire discovery process. Wrapping the iteration in a try...except OSError block ensures that permission or other filesystem errors in one directory do not crash the script.
| for item in dirs: | |
| root = Path(item).expanduser() | |
| for path in root.rglob("*"): | |
| if path.is_file() and path.suffix.lower() in {".sqlite", ".sqlite3", ".db"}: | |
| discovered.append(path) | |
| for item in dirs: | |
| root = Path(item).expanduser() | |
| try: | |
| for path in root.rglob("*"): | |
| try: | |
| if path.is_file() and path.suffix.lower() in {".sqlite", ".sqlite3", ".db"}: | |
| discovered.append(path) | |
| except OSError: | |
| continue | |
| except OSError: | |
| pass |
| except urllib.error.HTTPError as exc: | ||
| text = exc.read().decode("utf-8", errors="replace") | ||
| raise RuntimeError(f"{method} {url} failed: HTTP {exc.code}: {text[:500]}") from exc |
There was a problem hiding this comment.
The current exception handling only catches urllib.error.HTTPError. However, general network failures (like connection refused, DNS resolution failure, or timeouts) raise urllib.error.URLError, which is not caught here and will result in a raw traceback. Catching urllib.error.URLError as well and raising a clean RuntimeError will improve the CLI user experience.
| except urllib.error.HTTPError as exc: | |
| text = exc.read().decode("utf-8", errors="replace") | |
| raise RuntimeError(f"{method} {url} failed: HTTP {exc.code}: {text[:500]}") from exc | |
| except urllib.error.HTTPError as exc: | |
| text = exc.read().decode("utf-8", errors="replace") | |
| raise RuntimeError(f"{method} {url} failed: HTTP {exc.code}: {text[:500]}") from exc | |
| except urllib.error.URLError as exc: | |
| raise RuntimeError(f"{method} {url} failed: {exc.reason}") from exc |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 77e76f172d
ℹ️ 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".
| except Exception as exc: | ||
| attempt["error"] = str(exc) | ||
|
|
||
| result["deleted"] = delete_account(args, auth.name) |
There was a problem hiding this comment.
When the generated/imported name already exists (for example reusing --account-name, --name-prefix, or the default default name), the earlier POST /admin/kiro-gateway/accounts replaces the existing row via create_or_replace_kiro_account/upsert_kiro_account; if the newly imported credentials then fail balance validation or duplicate-user checks, this unconditional delete removes the account that existed before the import. Please preflight for name collisions or create under a temporary unique name before deleting failures.
Useful? React with 👍 / 👎.
| created = request_json( | ||
| "POST", args.admin_base_url, "/admin/kiro-gateway/accounts", args.admin_token, body | ||
| ) |
There was a problem hiding this comment.
In environments where the Kiro provider binding is unset or not the chosen US proxy, creating the account as active here lets create_or_replace_kiro_account immediately run sync_kiro_status_after_account_update before the later PATCH assigns the fixed proxy. That first status refresh can go through inherited/direct routing, which violates this importer’s no-direct/no-proxy policy and can cache a failure before validation. Create the account disabled (then patch proxy and enable it) or otherwise avoid the pre-patch refresh.
Useful? React with 👍 / 👎.
|
|
||
| def fetch_kiro_accounts(base_url: str, token: str | None) -> list[dict[str, Any]]: | ||
| payload = request_json( | ||
| "GET", base_url, "/admin/kiro-gateway/accounts?limit=10000&offset=0", token |
There was a problem hiding this comment.
When there are more than 200 Kiro accounts, this request still only sees the first page because the admin handler clamps limit to MAX_ADMIN_LIST_LIMIT (200) and returns has_more. That means proxy counts and existing_user_ids ignore later accounts, so the importer can place new accounts on overloaded proxies and miss duplicate upstream user_ids already present beyond the first page. Please follow pagination until has_more is false.
Useful? React with 👍 / 👎.
| if args.min_interval_min_ms < 0 or args.min_interval_max_ms < args.min_interval_min_ms: | ||
| raise SystemExit("invalid min interval range") |
There was a problem hiding this comment.
If an operator passes --min-interval-min-ms 0 --min-interval-max-ms 0 or a value above 1000, this validation accepts it and the real import writes an interval outside the stated standard policy, even though the script hard-enforces the other policy knobs such as concurrency and minimum credits. Please reject interval bounds outside 200..1000 so applied accounts cannot accidentally route with no pacing or excessive pacing.
Useful? React with 👍 / 👎.
1 participant
Summary
Add a local Kiro account import skill that reads Kiro CLI auth SQLite files on the operator machine and calls the locally mapped
llm-accessadmin API directly, without involving the StaticFlow backend.What changed
kiro-local-account-importerskill docs and agent metadata.scripts/import_kiro_accounts.pyto discover SQLite files, parse Kiroauth_kvrecords, create accounts through/admin/kiro-gateway/accounts, assign fixed United States proxies, and refresh balances before accepting imports.3, randomized start interval in200..1000ms, minimum remaining credits>= 10, no direct/no-proxy imports, and duplicate upstreamuser_iddeletion.Test plan
python3 -m unittest skills/kiro-local-account-importer/tests/test_import_kiro_accounts.pypython3 -m py_compile skills/kiro-local-account-importer/scripts/import_kiro_accounts.pypython3 /home/ts_user/.codex/skills/.system/skill-creator/scripts/quick_validate.py skills/kiro-local-account-importerNotes
No Rust crates are affected by this PR, so no Cargo build or clippy run was needed.