Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ai/skills/go/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ python3 "$GO_SKILL_DIR/scripts/run-review.py" run --harness "$HARNESS" --pr-url

Resolve the script against this skill's directory and keep the working directory at the implementation worktree. The runner uses `claude -p` or `codex exec` with the installed `review-code` skill, `--fix`, and a fresh conversation. The installed skill must support `REVIEW_CODE_REVIEW_DIR`; the runner checks this before launching. Each attempt keeps review sessions, reports, temporary checkouts, and hook files under its own `.notes/go-reviews/<run-id>/` directory. The child uses these locations through environment overrides. After validating completion, the parent archives the report at the shared PR path and records `archive_review_file`. Removing the implementation worktree removes the temporary state and leaves that report available. It retains normal harness permission checks and does not fall back to another harness when one fails. Reviews can take several minutes: start the command with the harness's background or yielding execution support, then poll its process and the saved status. Do not impose a short tool timeout. The runner's default timeout is 30 minutes, adjustable with `--timeout <seconds>`.

docket's `config.toml` picks the accounts for a Claude review. The runner reads `claude_config_dir` and `claude_fallback_config_dirs` from `$DOCKET_HOME/config.toml`, which defaults to `~/.docket/config.toml`. It orders the accounts the way docket's rotation does: it starts after the account that most recently hit a usage limit in docket's `limits.jsonl`, passes every account signed in to the same login, and it skips every account that a limit there still applies to. A limit applies to every account signed in to the login (one account in one organization) that docket recorded it with. An older limit without a login applies to the directory it names. An entry that names claude's default config directory, the one claude uses when `CLAUDE_CONFIG_DIR` is unset, runs the child with `CLAUDE_CONFIG_DIR` unset. Any other entry sets `CLAUDE_CONFIG_DIR` to that directory. An empty or missing `claude_config_dir` starts on this session's account, as docket starts on the account it inherited. Without a docket config, the review child runs on this session's account. Before launching on any account other than this session's, the runner checks that the account is signed in and has the `review-code` skill. If either check fails, the runner stops without launching a child, and its `error` names `config.toml` and says how to fix the account. If the child hits its account's usage limit while the checkout is still unchanged, the runner starts another attempt on the next account that is signed in to a different login and records `fallback_from_run_id`. After the listed accounts, it tries this session's account when the list leaves that account out, unless a limit that applies to a listed account also applies to this session's account. It tries each login once. When every account is over a recorded limit, the runner refuses before launching and names the time the first limit resets. If the child edited files before it hit the limit, the runner stops instead. The runner reads docket's limit log but never writes to it. Codex reviews ignore docket's accounts.
docket's `config.toml` picks the accounts for a Claude review. The runner reads `claude_config_dir` and `claude_fallback_config_dirs` from `$DOCKET_HOME/config.toml`, which defaults to `~/.docket/config.toml`. It orders the accounts the way docket does: it keeps the order `config.toml` lists them in and skips every account that a usage limit in docket's `limits.jsonl` still applies to, so a review returns to an earlier account as soon as its limit resets. A limit applies to every account signed in to the login (one account in one organization) that docket recorded it with. An older limit without a login applies to the directory it names. An entry that names claude's default config directory, the one claude uses when `CLAUDE_CONFIG_DIR` is unset, runs the child with `CLAUDE_CONFIG_DIR` unset. Any other entry sets `CLAUDE_CONFIG_DIR` to that directory. An empty or missing `claude_config_dir` starts on this session's account, as docket starts on the account it inherited. Without a docket config, the review child runs on this session's account. Before launching on any account other than this session's, the runner checks that the account is signed in and has the `review-code` skill. If either check fails, the runner stops without launching a child, and its `error` names `config.toml` and says how to fix the account. If the child hits its account's usage limit while the checkout is still unchanged, the runner starts another attempt on the next account that is signed in to a different login and records `fallback_from_run_id`. After the listed accounts, it tries this session's account when the list leaves that account out, unless a limit that applies to a listed account also applies to this session's account. It tries each login once. When every account is over a recorded limit, the runner refuses before launching and names the time the first limit resets. If the child edited files before it hit the limit, the runner stops instead. The runner reads docket's limit log but never writes to it. Codex reviews ignore docket's accounts.

In Codex, request host execution approval for the parent `run-review.py` command before launching it. The parent writes the validated report to `~/.agents/skills/review-code/.reviews` after the child exits. The nested `codex exec --approve-for-me` keeps its own workspace-write sandbox and receives no write grant for shared review caches.

Expand Down
31 changes: 8 additions & 23 deletions ai/skills/go/scripts/run-review.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,22 +405,20 @@ def review_accounts():
rotation is docket's account list. order is the CLAUDE_CONFIG_DIR values to
review under, where "" leaves the variable unset. limits maps each rotation
account that a limit docket recorded still applies to, as blocks() decides,
to the time the limit resets. The order follows docket's account.Pick. The
rotation starts at the first account after the first one that the most
recent limit applies to, passing every account that limit applies to, and
leaves out every account in limits. This session's account comes last when
docket's list leaves it out, unless a limit that applies to a rotation
account also applies to it. The order holds only the first account of each
to the time the limit resets. The order follows docket's account.Pick: it
keeps the rotation's order and leaves out every account in limits, so a
review returns to an earlier account as soon as its limit resets. This
session's account comes last when docket's list leaves it out, unless a
limit that applies to a rotation account also applies to it. The order holds only the first account of each
known login, because claude refuses every account signed in to a login once
one of them hits its limit. `run-review.py accounts` prints this as JSON.
"""
session = session_account()
config_path = docket_config_path()
rotation = list(dict.fromkeys(docket_accounts(config_path, session)))
logins = {account: login(account) for account in [*rotation, session]}
start, latest, latest_blocked, resets = 0, None, [], {}
session_reset = None
for limited, at, until, limited_login in load_limits(
resets, session_reset = {}, None
for limited, _, until, limited_login in load_limits(
config_path.with_name("limits.jsonl")
):
blocked = [
Expand All @@ -436,22 +434,9 @@ def review_accounts():
resets[account] = max(until, resets.get(account, until))
if blocks(limited, limited_login, session, logins[session]):
session_reset = max(until, session_reset or until)
if latest is None or at > latest:
latest, latest_blocked = at, blocked
start = rotation.index(blocked[0]) + 1
for _ in rotation:
if rotation[start % len(rotation)] not in latest_blocked:
break
start += 1
if rotation:
start %= len(rotation)
now = datetime.now(timezone.utc)
limits = {account: until for account, until in resets.items() if until > now}
candidates = [
account
for account in rotation[start:] + rotation[:start]
if account not in limits
]
candidates = [account for account in rotation if account not in limits]
if session not in rotation and not (session_reset and session_reset > now):
candidates.append(session)
order, ordered_logins = [], set()
Expand Down
33 changes: 9 additions & 24 deletions ai/skills/go/scripts/tests/test_run_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -1327,15 +1327,15 @@ def test_account_over_a_recorded_limit_is_skipped(self):
self.assertEqual(self.child_config_dirs(), [str(backup)])
self.assertNotIn("fallback_from_run_id", state)

def test_rotation_stays_on_the_backup_after_the_primary_limit_resets(self):
def test_order_returns_to_the_primary_after_its_limit_resets(self):
primary, backup = self.use_accounts("primary", "backup")
self.record_limit(primary, -timedelta(days=2), -timedelta(hours=1))

self.assert_success(self.run_review("claude"))

self.assertEqual(self.child_config_dirs(), [str(backup)])
self.assertEqual(self.child_config_dirs(), [str(primary)])

def test_rotation_wraps_to_an_account_whose_limit_reset(self):
def test_order_takes_the_primary_while_a_later_account_is_limited(self):
primary, backup = self.use_accounts("primary", "backup")
self.record_limit(backup, -timedelta(days=1), timedelta(days=1))
self.record_limit(primary, -timedelta(days=3), -timedelta(days=2))
Expand Down Expand Up @@ -1537,21 +1537,7 @@ def test_dir_without_both_uuids_has_no_known_login(self):

self.assertEqual(list(report["limits"]), [str(primary)])

def test_rotation_starts_after_the_first_dir_the_latest_limit_applies_to(self):
primary = self.sign_in(self.root / "primary")
backup = self.sign_in(self.root / "backup", TEAM_LOGIN)
spare = self.sign_in(self.root / "spare", TEAM_LOGIN)
self.write_docket_config(primary, backup, spare)
self.record_limit(
spare, -timedelta(days=2), -timedelta(days=1), login=TEAM_LOGIN
)

report = self.output(self.invoke("accounts", cwd=self.root))

# The start passes every dir on the limited login.
self.assertEqual(report["order"], [str(primary), str(backup), ""])

def test_rotation_stays_off_a_login_after_its_limit_resets(self):
def test_order_returns_to_a_login_after_its_limit_resets(self):
first = self.sign_in(self.root / "first", TEAM_LOGIN)
second = self.sign_in(self.root / "second", TEAM_LOGIN)
other = self.sign_in(self.root / "other", PERSONAL_LOGIN)
Expand All @@ -1562,7 +1548,7 @@ def test_rotation_stays_off_a_login_after_its_limit_resets(self):

report = self.output(self.invoke("accounts", cwd=self.root))

self.assertEqual(report["order"][0], str(other))
self.assertEqual(report["order"], [str(first), str(other), ""])

def test_limit_on_a_rotation_login_leaves_out_the_session_account_on_it(self):
self.sign_in(None, TEAM_LOGIN)
Expand Down Expand Up @@ -1593,25 +1579,24 @@ def test_every_login_over_a_limit_refuses_instead_of_the_session_account(self):

self.assertIn("usage limit", stderr)

def test_latest_limit_that_applies_to_no_rotation_dir_leaves_the_start(self):
def test_limit_that_applies_to_no_rotation_dir_blocks_none(self):
primary = self.sign_in(self.root / "primary", PERSONAL_LOGIN)
backup = self.sign_in(self.root / "backup")
spare = self.sign_in(self.root / "spare")
self.write_docket_config(primary, backup, spare)
self.record_limit(backup, -timedelta(days=2), -timedelta(days=1))
self.record_limit(
primary, -timedelta(hours=3), -timedelta(hours=2), login=TEAM_LOGIN
primary, -timedelta(hours=3), timedelta(hours=2), login=TEAM_LOGIN
)
self.record_limit(
self.root / "unlisted",
-timedelta(hours=1),
-timedelta(minutes=30),
timedelta(hours=2),
login=TEAM_LOGIN,
)

report = self.output(self.invoke("accounts", cwd=self.root))

self.assertEqual(report["order"], [str(spare), str(primary), str(backup), ""])
self.assertEqual(report["order"], [str(primary), str(backup), str(spare), ""])

def test_usage_limit_skips_later_dirs_signed_in_to_the_same_login(self):
primary = self.sign_in(self.root / "primary", TEAM_LOGIN)
Expand Down
Loading