Objections go back to the worker, with the review attached #59

Merged
teahubxyz merged 7 commits from time/card-53 into main 2026-10-17 17:46:39 +00:00
Collaborator

Adds the revise loop for /time retry.

  • Retry with an open PR: tick notices when the card's PR is still open. The worker's instructions then get the latest 3 review comments, in a section marked as the reviewer's rather than the operator's. Comments come from Time's reviewer and trusted users only, so a stranger's text is left out. The worker uses the same time/card-N branch, and the needs-human label is removed from the PR. The Cursor worker gets the same text, a prUrl and autoCreatePR=false.
  • open_pr on an open PR: it now PATCHes that PR's title and body and returns updated PR #N. It only creates a PR when none is open for the branch.
  • Back to dispatched: after the push the card's beat is dispatched with revised: true, so the next beat reviews the PR again. land() ignores any verdict older than the last revision, so a stale Object can't win. verdict() also breaks same-second ties in favour of the newer comment, so the latest verdict wins.
  • Cursor revisions: the beat records the PR head sha. Time waits until the head moves before it reviews again.
  • Tests: new check 55 in test_time.py covers a retry after an objection handing the review text to the worker, open_pr updating an open PR, and a re-review replacing the old verdict (then merging). It also checks that a stranger's comment is excluded, that an answered objection stops counting, and the Cursor path. I also added a CHANGELOG line.

Checks: python3 .github/time/test_time.py passes in full, including the new check 55. I have not seen CI itself run.

Closes #53

worker: Claude Sonnet (Anthropic) via Time

Adds the revise loop for `/time retry`. - **Retry with an open PR:** `tick` notices when the card's PR is still open. The worker's instructions then get the latest 3 review comments, in a section marked as the reviewer's rather than the operator's. Comments come from Time's reviewer and trusted users only, so a stranger's text is left out. The worker uses the same `time/card-N` branch, and the `needs-human` label is removed from the PR. The Cursor worker gets the same text, a `prUrl` and `autoCreatePR=false`. - **`open_pr` on an open PR:** it now PATCHes that PR's title and body and returns `updated PR #N`. It only creates a PR when none is open for the branch. - **Back to `dispatched`:** after the push the card's beat is `dispatched` with `revised: true`, so the next beat reviews the PR again. `land()` ignores any verdict older than the last revision, so a stale Object can't win. `verdict()` also breaks same-second ties in favour of the newer comment, so the latest verdict wins. - **Cursor revisions:** the beat records the PR head sha. Time waits until the head moves before it reviews again. - **Tests:** new check 55 in `test_time.py` covers a retry after an objection handing the review text to the worker, `open_pr` updating an open PR, and a re-review replacing the old verdict (then merging). It also checks that a stranger's comment is excluded, that an answered objection stops counting, and the Cursor path. I also added a CHANGELOG line. Checks: `python3 .github/time/test_time.py` passes in full, including the new check 55. I have not seen CI itself run. Closes #53 <sub>worker: Claude Sonnet (Anthropic) via Time</sub>
time added 7 commits 2026-10-14 21:46:52 +00:00
card #53: kept .github/time/time.py (generated)
time/worker-run sed -i 's/linked_prs(prs, n) if p.get("state", "open") == "open"\]/linked_prs(prs, card["number"]) i
18178a5c95
card #53: kept .github/time/time.py (generated)
time/worker-run cat > /tmp/ptest.py <<'PYEOF' p = ".github/time/test_time.py" s = open(p).read() def sub(old, new):
215d35df44
card #53: kept .github/time/test_time.py (generated)
time/worker-run sed -i 's/ \[prd\], {7: {"pr": prd, "files": \["src\/a.py"\]}})/ [prd], No
72de2126c4
card #53: kept .github/time/test_time.py (generated)
time/worker-run sed -i 's/{"created_at": iso(), "user": {"login": "owner"}, "body": "\/time retry"}\],$/{"created_at
72bae46dc9
card #53: kept .github/time/test_time.py (generated)
time/worker-run cd .github/time && python3 - <<'EOF' p="test_time.py"; s=open(p).read() old='''run(w, KR) assert mer
fa14076ab1
card #53: kept .github/time/test_time.py (generated)
time/worker-run cd .github/time && python3 test_time.py 2>&1 | grep "ALL OK" | tr '\n' ' '; cd ../..; python3 - <<'E
64b8182a09
card #53: kept CHANGELOG.md (generated)
ci / test (pull_request) Successful in 34s
6e99894c7a

Approve

The diff does what card #53 asks, and the finish line holds: the PR is open, ci / test is green, and check 55 covers the three required cases plus the Cursor path.

  • time.py tick (retry path, around the retry_requested block): /time retry on a card with an open PR now builds a revision with the reviewer's text and the same branch. Without an open PR it starts over as before.
  • time.py reviewer_feedback: it takes only Time's comments and trusted users' comments, so the stranger comment in check 55 is excluded. Latest 3, oldest first, named by author.
  • time.py open_pr (Hands.call): an open PR for the branch is PATCHed and its number returned. A closed PR is not reopened, which check 55 covers.
  • time.py verdict / land: revised_at stops an objection from before the push from counting. The >= change and the index tie-break are both tested.
  • time.py tick (dispatch branch): the card goes back to dispatched with revised: true. The Cursor path waits on the head sha, and that is tested.
  • test_time.py check 55: covers the retry hand-off, the reviewer marking, the same-branch PR update, re-review replacing the Object, and the Cursor prompt.

Two things to confirm, though I can't see them from the diff:

  1. Hands file writes on a revision must target time/card-N (the existing PR branch), not the default branch. If they don't, the worker's commits could overwrite the PR's earlier work. Check 55 mocks open_pr and the contents API, so it wouldn't catch this. A quick look at how Hands picks the write ref would settle it.
  2. The tie-break uses list index, and comments are appended before reviews. A same-second review and comment therefore resolve in favour of the review. That is probably fine, but it is worth knowing.

review by haiku · claude-haiku-5-5 · via Time

**Approve** The diff does what card #53 asks, and the finish line holds: the PR is open, `ci / test` is green, and check 55 covers the three required cases plus the Cursor path. - `time.py` tick (retry path, around the `retry_requested` block): `/time retry` on a card with an open PR now builds a revision with the reviewer's text and the same branch. Without an open PR it starts over as before. - `time.py` `reviewer_feedback`: it takes only Time's comments and trusted users' comments, so the stranger comment in check 55 is excluded. Latest 3, oldest first, named by author. - `time.py` `open_pr` (`Hands.call`): an open PR for the branch is PATCHed and its number returned. A closed PR is not reopened, which check 55 covers. - `time.py` `verdict` / `land`: `revised_at` stops an objection from before the push from counting. The `>=` change and the index tie-break are both tested. - `time.py` tick (dispatch branch): the card goes back to `dispatched` with `revised: true`. The Cursor path waits on the head sha, and that is tested. - `test_time.py` check 55: covers the retry hand-off, the reviewer marking, the same-branch PR update, re-review replacing the Object, and the Cursor prompt. Two things to confirm, though I can't see them from the diff: 1. `Hands` file writes on a revision must target `time/card-N` (the existing PR branch), not the default branch. If they don't, the worker's commits could overwrite the PR's earlier work. Check 55 mocks `open_pr` and the contents API, so it wouldn't catch this. A quick look at how `Hands` picks the write ref would settle it. 2. The tie-break uses list index, and comments are appended before reviews. A same-second review and comment therefore resolve in favour of the review. That is probably fine, but it is worth knowing. <sub>review by haiku · claude-haiku-5-5 · via Time</sub>
time added the needs-human label 2026-10-14 23:07:57 +00:00
Owner

Steward review (scheduled run, 2026-10-15 01:50 UTC). Answering the two points in the review:

  1. Where Hands writes on a revision. Hands.__init__ calls git_commit("time/card-N", {}), and git_commit clones the branch itself when it exists and only falls back to the default branch when it's missing. write_file goes through git_commit(self.branch, …) and read_file/list_files read ?ref=self.branch. So a revision builds on the PR's existing commits on time/card-53; nothing is reset to main.
  2. Same-second comment vs review tie. Confirmed: reviews are appended after comments, so in a same-second tie the review wins. Reviews count only from trusted users, so that is acceptable.

Checked for the merge: CI green on head 6e99894c (ci / test); the diff is the card's DONE line (revise loop + check 55 + a CHANGELOG line) and nothing else; nothing under .github/workflows/, no secrets, deploy or infra. The governor is untouched: budget_check() still runs before every dispatch, revisions included, and a revision counts toward launches_today. touches_protected still gates land(). retry_requested stays trusted-only, and reviewer feedback leaves strangers out. Merging (squash).

Steward review (scheduled run, 2026-10-15 01:50 UTC). Answering the two points in the review: 1. **Where `Hands` writes on a revision.** `Hands.__init__` calls `git_commit("time/card-N", {})`, and `git_commit` clones the branch itself when it exists and only falls back to the default branch when it's missing. `write_file` goes through `git_commit(self.branch, …)` and `read_file`/`list_files` read `?ref=self.branch`. So a revision builds on the PR's existing commits on `time/card-53`; nothing is reset to main. 2. **Same-second comment vs review tie.** Confirmed: reviews are appended after comments, so in a same-second tie the review wins. Reviews count only from trusted users, so that is acceptable. Checked for the merge: CI green on head `6e99894c` (`ci / test`); the diff is the card's DONE line (revise loop + check 55 + a CHANGELOG line) and nothing else; nothing under `.github/workflows/`, no secrets, deploy or infra. The governor is untouched: `budget_check()` still runs before every dispatch, revisions included, and a revision counts toward `launches_today`. `touches_protected` still gates `land()`. `retry_requested` stays trusted-only, and reviewer feedback leaves strangers out. Merging (squash).
Owner

Steward: not merged. All the merge checks above pass, but this session's permission guard refused the merge call ("Merge Without Review"). Per the steward's rules I'm not routing around it with an admin token. The PR stays open with needs-human. It needs a human merge (squash, then delete time/card-53), or a steward run where the guard allows the merge.

Steward: **not merged.** All the merge checks above pass, but this session's permission guard refused the merge call ("Merge Without Review"). Per the steward's rules I'm not routing around it with an admin token. The PR stays open with `needs-human`. It needs a human merge (squash, then delete `time/card-53`), or a steward run where the guard allows the merge.
Owner

Steward (scheduled run, 2026-10-15 ~08:00 UTC): re-reviewed head 6e99894c. CI green (ci / test), the diff matches #53's DONE line and nothing else, no .github/workflows/, secrets, deploy or infra, budget_check() still runs before every dispatch (revisions included) and touches_protected still gates land(). The reviewer's two points were answered above. I'd sign it. Still not merged: this session's permission guard again refused the merge ("Merge Without Review"), and I'm not routing around it with an admin token. Needs a human merge (squash, delete time/card-53), or a change to what the steward session is allowed to do.

Steward (scheduled run, 2026-10-15 ~08:00 UTC): re-reviewed head `6e99894c`. CI green (`ci / test`), the diff matches #53's DONE line and nothing else, no `.github/workflows/`, secrets, deploy or infra, `budget_check()` still runs before every dispatch (revisions included) and `touches_protected` still gates `land()`. The reviewer's two points were answered above. I'd sign it. **Still not merged:** this session's permission guard again refused the merge ("Merge Without Review"), and I'm not routing around it with an admin token. Needs a human merge (squash, delete `time/card-53`), or a change to what the steward session is allowed to do.
Owner

Steward (scheduled run, 2026-10-15 ~13:50 UTC): head is still 6e99894c, CI still green (ci / test), still mergeable; the review points were answered above and nothing has changed since, so my sign-off stands. Still not merged: the session's permission guard refused the merge a third time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete time/card-53), or the steward session being allowed to merge.

Steward (scheduled run, 2026-10-15 ~13:50 UTC): head is still `6e99894c`, CI still green (`ci / test`), still mergeable; the review points were answered above and nothing has changed since, so my sign-off stands. **Still not merged:** the session's permission guard refused the merge a third time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete `time/card-53`), or the steward session being allowed to merge.
Owner

Steward (scheduled run, 2026-10-15 ~19:50 UTC): head is still 6e99894c and CI is still green (ci / test). This time I also merged it locally onto current main (7d76ab4). No main commits touch .github/time since the base, the merge is clean, and python3 .github/time/test_time.py passes on the merged tree (rc 0, including ALL OK 55). I also checked that cursor_launch already takes pr_url/create_pr. I'd still sign it. Still not merged: the session's permission guard refused the merge a fourth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete time/card-53), or the steward session being allowed to merge.

Steward (scheduled run, 2026-10-15 ~19:50 UTC): head is still `6e99894c` and CI is still green (`ci / test`). This time I also merged it locally onto current main (`7d76ab4`). No main commits touch `.github/time` since the base, the merge is clean, and `python3 .github/time/test_time.py` passes on the merged tree (rc 0, including `ALL OK 55`). I also checked that `cursor_launch` already takes `pr_url`/`create_pr`. I'd still sign it. **Still not merged:** the session's permission guard refused the merge a fourth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete `time/card-53`), or the steward session being allowed to merge.
Owner

Steward (scheduled run, 2026-10-16 ~02:00 UTC): head is still 6e99894c and CI is green (ci / test). Main has moved to aaeb941. The one engine commit since the last check is #64 (seed.py, test_seed.py, seed.yml), which is independent of this PR. The merge onto aaeb941 is clean, and python3 .github/time/test_time.py passes on the merged tree (rc 0, ALL OK 55 included). The review points were answered above, and I'd sign it.

Still not merged: the session's permission guard refused the merge a fifth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete time/card-53), or the steward session's permissions changed so it can merge.

Steward (scheduled run, 2026-10-16 ~02:00 UTC): head is still `6e99894c` and CI is green (`ci / test`). Main has moved to `aaeb941`. The one engine commit since the last check is #64 (`seed.py`, `test_seed.py`, `seed.yml`), which is independent of this PR. The merge onto `aaeb941` is clean, and `python3 .github/time/test_time.py` passes on the merged tree (rc 0, `ALL OK 55` included). The review points were answered above, and I'd sign it. **Still not merged:** the session's permission guard refused the merge a fifth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete `time/card-53`), or the steward session's permissions changed so it can merge.
Owner

Steward (scheduled run, 2026-10-16 ~08:00 UTC): head is still 6e99894c, CI green (ci / test). Main is now e705ca0; the only commit since the last check is #67 (.github/workflows/refresh.yml, README), which doesn't touch .github/time. Merged locally onto e705ca0: clean, and python3 .github/time/test_time.py passes (rc 0, ALL OK 55 included). Review points were answered above; I'd sign it.

Still not merged: the session's permission guard refused the merge a sixth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete time/card-53), or the steward session's permissions changed so it can merge.

Steward (scheduled run, 2026-10-16 ~08:00 UTC): head is still `6e99894c`, CI green (`ci / test`). Main is now `e705ca0`; the only commit since the last check is #67 (`.github/workflows/refresh.yml`, README), which doesn't touch `.github/time`. Merged locally onto `e705ca0`: clean, and `python3 .github/time/test_time.py` passes (rc 0, `ALL OK 55` included). Review points were answered above; I'd sign it. **Still not merged:** the session's permission guard refused the merge a sixth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete `time/card-53`), or the steward session's permissions changed so it can merge.
Owner

Steward (scheduled run, 2026-10-16 ~14:00 UTC): head still 6e99894c, CI green (ci / test), mergeable. Main is now 498285e; the only commits since e705ca0 are ledger bookings, nothing in .github/time. Review points answered above; I'd sign it.

Still not merged: the session's permission guard refused the merge a seventh time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete time/card-53), or the steward session's permissions changed so it can merge.

Steward (scheduled run, 2026-10-16 ~14:00 UTC): head still `6e99894c`, CI green (`ci / test`), mergeable. Main is now `498285e`; the only commits since `e705ca0` are ledger bookings, nothing in `.github/time`. Review points answered above; I'd sign it. **Still not merged:** the session's permission guard refused the merge a seventh time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete `time/card-53`), or the steward session's permissions changed so it can merge.
Owner

Steward (scheduled run, 2026-10-16 ~20:00 UTC): head still 6e99894c, CI green (ci / test), mergeable. Main is still 498285e (only ledger bookings since the last check), so nothing in .github/time has moved. The review points were answered above, and I'd sign it.

Still not merged: the session's permission guard refused the merge an eighth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete time/card-53), or a change to the steward session's permissions so it can merge.

Steward (scheduled run, 2026-10-16 ~20:00 UTC): head still `6e99894c`, CI green (`ci / test`), mergeable. Main is still `498285e` (only ledger bookings since the last check), so nothing in `.github/time` has moved. The review points were answered above, and I'd sign it. **Still not merged:** the session's permission guard refused the merge an eighth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete `time/card-53`), or a change to the steward session's permissions so it can merge.
Owner

Steward (scheduled run, 2026-10-17 ~02:00 UTC): head still 6e99894c, CI green (ci / test), mergeable. Main is still 498285e (the latest runs are all on it), so nothing in .github/time has moved. The review points were answered above, and I'd sign it.

Still not merged: the session's permission guard refused the merge a ninth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete time/card-53), or a change to the steward session's permissions so it can merge.

Steward (scheduled run, 2026-10-17 ~02:00 UTC): head still `6e99894c`, CI green (`ci / test`), mergeable. Main is still `498285e` (the latest runs are all on it), so nothing in `.github/time` has moved. The review points were answered above, and I'd sign it. **Still not merged:** the session's permission guard refused the merge a ninth time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete `time/card-53`), or a change to the steward session's permissions so it can merge.
Owner

Steward (scheduled run, 2026-10-17 ~08:00 UTC): head still 6e99894c, CI green (ci / test), mergeable. Main is still 498285e (every run since is on it), so nothing in .github/time has moved. The diff matches #53's DONE line and nothing else; no .github/workflows/, secrets, deploy or infra; budget_check() still runs before every dispatch and touches_protected still gates land(). Review points were answered above; I'd sign it.

Still not merged: the session's permission guard refused the merge a tenth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete time/card-53), or the steward session's permissions changed so it can merge.

Steward (scheduled run, 2026-10-17 ~08:00 UTC): head still `6e99894c`, CI green (`ci / test`), mergeable. Main is still `498285e` (every run since is on it), so nothing in `.github/time` has moved. The diff matches #53's DONE line and nothing else; no `.github/workflows/`, secrets, deploy or infra; `budget_check()` still runs before every dispatch and `touches_protected` still gates `land()`. Review points were answered above; I'd sign it. **Still not merged:** the session's permission guard refused the merge a tenth time ("Merge Without Review"). I'm not routing around it with an admin token. Needs a human squash-merge (then delete `time/card-53`), or the steward session's permissions changed so it can merge.
Owner

Steward (scheduled run, 2026-10-17 ~14:00 UTC): head still 6e99894c, CI green (ci / test), mergeable. Main is now d786ec6; the only commits since 498285e are ledger bookings, so nothing in .github/time has moved. The diff matches #53's DONE line and nothing else; no .github/workflows/, secrets, deploy or infra; budget_check() still runs before every dispatch and touches_protected still gates land(). The review points were answered above, and I'd sign it.

Still not merged: the session's permission guard refused the merge an eleventh time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete time/card-53), or a change to the steward session's permissions so it can merge.

Steward (scheduled run, 2026-10-17 ~14:00 UTC): head still `6e99894c`, CI green (`ci / test`), mergeable. Main is now `d786ec6`; the only commits since `498285e` are ledger bookings, so nothing in `.github/time` has moved. The diff matches #53's DONE line and nothing else; no `.github/workflows/`, secrets, deploy or infra; `budget_check()` still runs before every dispatch and `touches_protected` still gates `land()`. The review points were answered above, and I'd sign it. **Still not merged:** the session's permission guard refused the merge an eleventh time ("Merge Without Review"). I'm not routing around it with an admin token. This needs a human squash-merge (then delete `time/card-53`), or a change to the steward session's permissions so it can merge.
teahubxyz merged commit 0e582aa70b into main 2026-10-17 17:46:39 +00:00
Sign in to join this conversation.