[sonichi]

What

One core change: an immutable turn start in src/turn_ledger.py. Two Stop hooks that run in parallel can both measure "this turn" from it.

• begin_turn (the UserPromptSubmit hook's turn-start) writes started beside ts.
• spend_reminder keeps started when it rewrites the record.
• New turn_started_at(workspace, session) reads only started. It returns None when the record has none (one written before this change) or holds anything unusable. It never falls back to ts.

Why: ts moves when the Stop gate spends its reminder, and the gate's mark_stop moves the Stop boundary. A second Stop hook running beside the gate could therefore measure the turn from a point past its own evidence. That was finding 2 of the review.

Scope, after the owner's decisions of 2026-10-06

This PR used to carry a whole Commons reminder. It is now only the turn start above. Removed, with the branch reset onto current main:

• turn_evidence.py (PR detection is gone entirely)
• the session_role.py extraction
• the commons-reminder skill
• the installer changes
• the result_after rename

The reminder itself moved into the room-commons skill as one generic line, on only for rooms that turn it on, with the owner's DM as the default: ag2-space/room-commons#136. That hook reads turn_started_at when it exists and falls back to last_stop_ts on an engine without it.

Kewei's findings from the first review:

• 1 (one-shot): still addressed, now in the skill's hook. It is silent on stop_hook_active and keeps a per-turn spent marker keyed on this turn start, with tests for the second Stop, the continuation and the next turn.
• 2 (the race): addressed here, by started.
• 4 (guest rule fail-open): still addressed. check-pending-tasks.sh is untouched, so its fail-closed rule is exactly main's. The skill's local guest check reads anything unusable as "not a guest".
• 3 (mutation detection) and 5 (core vs skill): moot. PR detection is gone, and the hook lives in the skill.

Evidence

Parent 5848b4459 with this PR's test file:

AttributeError: module 'turn_ledger' has no attribute 'turn_started_at'

HEAD python3 tests/turn-ledger-stop-gate.test.py, the new cases:

test_the_turn_start_is_immutable_across_the_turn:
  ok   no turn recorded reads as None
  ok   begin_turn records the start
  ok   spending moved `ts` past this turn's own evidence
  ok   ... and kept `started`
  ok   so the turn's evidence is still found from the start
  ok   the next turn has a new start
test_the_turn_start_is_per_session_and_strict:
  ok   B's turn start leaves A's alone
  ok   a record from before `started` reads as None, never as `ts`
  ok   spending such a record invents no start
  ok   an unusable record ({"started": true}) reads as None
  ok   an unusable record ({"started": "5"}) reads as None
  ok   an unusable record ([1]) reads as None
  ok   an unusable record ({not json) reads as None
test_the_real_hooks_keep_the_start:
  ok   setup: the silent turn was reminded
  ok   turn-start.sh recorded a start
  ok   the reminding Stop kept it
turn-ledger-stop-gate: PASS

The last case runs the real turn-start.sh and a reminding check-pending-tasks.sh as subprocesses. I made four mutations, each on its own: not writing started, not keeping it on spend, reading ts instead, and accepting a boolean. Each one fails the suite.

Unchanged and passing: check-pending-tasks-{workspace,worker-pool,watcher-coverage,in-progress-and-repeat}.test.sh, stop-hook-emits-valid-json.test.py and stop-hook-reason-carries-the-gate-text.test.py.

Consumer check against the room-commons hook. The sequence was mark_stop, begin_turn, a DM reply, the gate's spend_reminder, then the gate's mark_stop landing before the hook reads:

• With this branch the hook still fires.
• With current main (fallback to the last Stop) it loses the turn.

Live round trip

Not yet run. It needs the owner's one-time login to a scratch Claude config. Nothing on the live core's settings or workspace is touched.

The staged script installs scratch settings in the installer's exact shape (turn-start.sh, check-pending-tasks.sh, and the skill-declared hook from the room-commons PR) and runs three real claude -p sessions:

• a qualifying turn that writes a DM reply, where we expect exactly one Commons line and a quiet continuation;
• a resumed do-nothing turn;
• a fresh quiet session.

It then writes the observations to a file.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/turn_ledger.py (100%)

Summary

• Total: 18 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

@qingyun-wu — requesting review on this one (a non-blocking Stop-hook reminder about Room Commons actions at turn end). No other reviewer recorded yet toward the 2-approval bar.

[keweichen]

Changes requested at exact head f7bd3706cc41b50229435b20be755ea22b80e654. This is not merge-ready.

1. Blocking — the Stop feedback is not one-shot and can loop. src/commons_reminder.py:121-140 reads only transcript_path; it ignores stop_hook_active and never records that this turn already received the Commons reminder. The result/action remains newer than the unchanged turn marker, so an identical retry emits identical additionalContext. I invoked the exact module twice against the same fresh result: both stop_hook_active=false and true returned the same feedback. Claude Code documents that Stop additionalContext continues the conversation, sets stop_hook_active, and relies on an eight-continuation cap whose count resets when Claude calls a tool: https://code.claude.com/docs/en/hooks#stop-input and https://code.claude.com/docs/en/hooks#stop-decision-control. Add resolving/per-turn-spent semantics and a regression proving the second Stop on unchanged state is quiet, including interaction with the pending-task Stop hook.

2. Blocking — the two installed Stop hooks race on the supposed turn-start timestamp. turn_started_at() treats turn-reminder*.json.ts as the stable start at src/turn_ledger.py:454-464, but spend_reminder() overwrites that same ts at lines 477-480. All matching hooks run in parallel. On a PR-only turn, check-pending-tasks.sh can spend its silent-turn reminder before Commons reads the marker, moving the boundary past the PR action. Exact module repro: the same transcript returns [a PR opened] before spend_reminder() and [] after it. Preserve an immutable turn-start value and test both production Stop hooks together in both schedules.

3. Blocking — command text is not proof that a GitHub mutation succeeded. GH_ACTION at src/commons_reminder.py:36-40 and the scan at lines 86-97 check neither Bash tool identity nor tool result/exit status. Exact probes classify all of these as real actions: read-only gh api repos/o/r/pulls/5145/reviews → “a review posted”; read-only gh api repos/o/r/pulls/5145/merge → “a PR merged”; and printf %s "gh pr review 5145 --approve" → “a review posted”. Failed commands are indistinguishable too. Ordinary review audits therefore create false Commons work. Detect a successful mutation rather than a substring, with controls for GETs, quoted/search text, failures, dry runs, and genuine success.

4. Blocking — the shared guest-role extraction changes the existing gate from fail-closed to fail-open on malformed configuration. src/session_role.py:82-87 catches an invalid --max-age and silently keeps 90 seconds. At the parent, the old inline float(...) fails, so an unmarked session is treated as the core and the pending-task gate remains active. Exact parent/HEAD repro with a fresh same-host heartbeat, a pending task, and SUTANDO_STOP_HOOK_CORE_ALIVE_MAX_AGE=bogus: parent emits “gating it as the core” plus decision:block; HEAD emits “guest session” plus {}. tests/commons-reminder-module.test.py currently pins the permissive result. Invalid or dangling decision inputs must return “not guest”, and the production-hook parity case needs a test.

5. Blocking architecture/worst-case — Room Commons policy is feature-specific but is installed as always-on core behavior. src/commons_reminder.py hardcodes Room Commons, room Doc, owned rows, and Outcomes, while src/install-claude-hooks.sh:100 installs it for every checkout. That violates the repository boundary that self-contained business workflows live under skills/, and it gives users with no Room Commons extra model continuations and irrelevant instructions. The existing src/skill_hooks.py discovery plus skills/report-feedback/manifest.json Stop hook show the supported optional path. Keep generic session-role/turn evidence in core; put the Commons-specific detector and presentation in a skill-declared hook.

I did verify the positive evidence at this SHA: 25 module tests, all 18 hook cases, all 33 pending-task checks, all 52 installer checks, the turn-ledger suite, all 56 hook-registration tests, review checks, shell syntax, git diff --check, exact-head CI, and CLA all pass. Those tests do not exercise the blockers above.

Worst case today is repeated or unbounded paid model turns, nondeterministic missed reminders, false Commons updates, and suppression of the existing pending-task gate. After the fixes, repository review policy also requires a real installed-hook round trip: one qualifying turn produces exactly one reminder, its continuation stops quietly, and a quiet turn remains quiet. The PR currently states that live settings were not edited, so its evidence is harness-only.

[keweichen]

Changes requested at exact head f92ef1e2f8e6cc230b854fb6be0ad7ab5505d93b. This is not merge-ready.

Four of the five earlier code findings are fixed: the Commons policy/presentation now lives in the skill, the turn origin remains immutable across both Stop hooks, stop_hook_active plus the per-origin marker makes the installed handler one-shot, and malformed guest-role inputs fail closed. I reran the focused suites, installer/registration checks, review scan, and git diff --check; exact-head CI and CLA are green. Two blockers remain.

1. Blocking — the mutation detector still reports commands that neither ran nor succeeded. src/turn_evidence.py:33-45 regexes the raw compound-shell string, and lines 113-136 then treat the aggregate Bash tool_result as proof for every matched subcommand. Exact-head probes:

   printf '%s' 'noop; gh pr review 5145 --approve'
     -> ['review-posted']
   true || gh pr review 5145 --approve
     -> ['review-posted']
   gh pr create --title x || true
     + tool_result(is_error=false, content="HTTP 403; true returned 0")
     -> ['pr-opened']
   gh api repos/o/r/issues -f body=/pulls/5145/reviews
     -> ['review-posted']
   

   The first is quoted output, the second branch never executes, the third can mask a failed gh, and the fourth posts to the issues endpoint while merely carrying a review-looking field value. All can produce the Commons continuation without a successful GitHub mutation, so prior finding 3 is not resolved. Require evidence that the specific target command executed successfully—not merely that the enclosing Bash tool ended successfully—and pin these compound/quoted/argument-value cases as regressions.

2. Blocking — the required installed-hook round trip is still absent. The PR body says the fresh claude -p process failed authentication before the model turn. Invoking the three installer-shaped command strings directly with bash -c is useful harness evidence, but it bypasses Claude's actual settings loading, hook scheduler, model-driven Stop continuation, and resume path. REVIEW.md criterion 15 and CONTRIBUTING.md require a real post-reload/restart round trip for this live path before approval.

   Please take the ASK already identified in the PR body: one owner interactive login in an isolated scratch pane, then run a fresh authenticated Claude process against the installed scratch settings. Capture one qualifying turn producing exactly one Commons continuation, its continuation and an unchanged second Stop staying quiet, and a fresh quiet session staying quiet. This should require no live-core settings change or service downtime.

Worst case today is an irrelevant extra paid/model continuation on an ordinary audit command, while the only evidence for actual Claude hook orchestration remains synthetic. The other four prior blockers are resolved, but these two still gate approval.

[sonichi]

Blocker 1 fixed at head 06c913194 (pushed 7 min after your review, before I'd seen it land — apologies for the overlap). Independently re-verified your exact four probes against the new command_mutation/result_confirms split rather than trust the commit message:

printf '%s' 'noop; gh pr review 5145 --approve'   -> None
true || gh pr review 5145 --approve                -> None
gh pr create --title x || true                     -> None
gh api repos/o/r/issues -f body=/pulls/5145/reviews -> None
result_confirms('pr-opened', {'content': 'HTTP 403; true returned 0', 'is_error': False}) -> False

The command is now shlex-parsed into pipeline/chain segments (nothing credited behind ||, ;, |, $(, backticks or quotes), and separately result_confirms requires gh's own success output for that specific verb — so even an aggregate-success Bash result with a masked HTTP 4xx/5xx inside it is rejected. Full suite green at head: 16/16 turn-evidence, plus every touched suite (check-pending-tasks-workspace, stop-hooks-together, turn-ledger-stop-gate, session-role) — I reran these myself in a clean env, not just read the PR body's numbers. CI/CLA green at 06c913194.

Blocker 2 (the real model-driven installed-hook round trip) is still open exactly as the PR body says — blocked on one interactive /login in a scratch pane, which needs the owner. Asking Chi for that now rather than leaving it stated-but-unrequested.

[github-actions]

@cla-assistant check

[john-the-dev]

Approve at 4ca57d3171ba9018fffa56d3f0fedee875bc9c5d. The PR is now 2 files (+97 −3): src/turn_ledger.py and tests/turn-ledger-stop-gate.test.py.

What I checked at this head

• Diff read in full. begin_turn writes started beside ts (turn_ledger.py:450-452). spend_reminder carries started forward only when it is a real number (:490-494). turn_started_at reads only started, rejects bool/str/non-dict/bad JSON, and never falls back to ts (:465-475).
• The only readers of turn-reminder*.json in src/ skills/ scripts/ are reminder_spent, spend_reminder and the new turn_started_at, all in turn_ledger.py. The extra key changes nothing for the existing gate, which still reads only reminded.
• python3 tests/turn-ledger-stop-gate.test.py → PASS. That includes test_the_real_hooks_keep_the_start, which runs the real turn-start.sh and a reminding check-pending-tasks.sh as subprocesses.
• I re-ran two of the author's mutations myself. Dropping the carry-forward in spend_reminder fails "... and kept started". Falling back to ts in turn_started_at fails "a record from before started reads as None" and "spending such a record invents no start". The suite pins both guards.
• scripts/review-checks.sh --diff on the full PR diff → PASS.
• No other open PR touches these two files.

Kewei's CHANGES_REQUESTED at f92ef1e

• Blocker 1 (mutation detector): moot at this head. turn_evidence.py and all PR/mutation detection are gone from the diff.
• Blocker 2 (installed-hook live round trip): I read it as belonging to the consumer, the room-commons Stop hook. That hook is the thing that emits a continuation. This PR adds a field and a reader with no in-repo caller. On the live path, the only behaviour change is one more key in a status JSON, and the subprocess test covers that through the real hook scripts. If you see an orchestration risk in the field itself, say so and I'll look again.

CI. Two checks are red: python standalone tests (1/5) and the aggregate tsc + tests (clean install) that depends on it. The failing file is tests/cli-wedge-health-probe.test.py ("sampled from inside the pane it watches (… pid 4242)"). The diff doesn't touch it, it doesn't reference turn_ledger, it is unchanged between the base and main, and it passes locally at this head (18 tests OK). It looks environmental, not caused by this diff, but I did not prove it is a flake.

Nits (non-blocking)

1. spend_reminder is now read-modify-write. If a turn's UserPromptSubmit hook fails to run, the previous turn's started survives, and turn_started_at returns that older origin. The consumer's per-origin spent marker should absorb this. A one-line docstring note would help the next caller.
2. turn_started_at is exported with no in-repo caller until the room-commons hook lands. That's fine, but the PR body is the only place that link lives.

Not verified

• The consumer hook (room-commons#136). It isn't reachable from my credentials.
• Any model-driven claude -p round trip.

[qingyun-wu]

Code-owner approval, decided by Qingyun on the [PA] decision page at 4ca57d3. Posted by Qingyun's Sutando.

[sonichi]

Status at head 4ca57d3171ba9018fffa56d3f0fedee875bc9c5d.

Both approvals now stand at this exact head: john-the-dev (review 5429948820) and qingyun-wu as code owner (review 5432580432). That meets the 2-approval bar. The only thing keeping reviewDecision at CHANGES_REQUESTED is @keweichen's review 5426113960 (at the older head f92ef1e2f8e), which GitHub carries forward until a dismissal or re-review.

Independently re-checked both of that review's blockers against this head, not taken from john's read alone:

• Blocker 1 (mutation detector false positives): moot. gh pr diff 5145 --name-only at this head shows only src/turn_ledger.py and tests/turn-ledger-stop-gate.test.py — src/turn_evidence.py and all PR/mutation-detection code are no longer part of this diff.
• Blocker 2 (installed-hook live round trip): this PR adds a field (started) and a reader (turn_started_at) with no in-repo caller yet — the orchestration risk belongs to the room-commons Stop-hook consumer that will call it, not to this PR. If you see risk in the field/reader itself in isolation, flagging that here would help.

CI: python standalone tests (1/5) is red, but the only failing file is tests/cli-wedge-health-probe.test.py (run 37467822536) — unrelated to this diff (doesn't import turn_ledger, unchanged between base and head) and matches john's read.

@keweichen — could you re-review or dismiss at this head? Nothing in your two blockers appears to still apply here, but I'd rather you confirm than I claim clearance on your behalf.

[liususan091219]

Approve at 4ca57d317. Expert route met: qingyun-wu (code owner) and john-the-dev both approved at this exact head; john's review re-ran the suite and two mutations (dropping the carry-forward in spend_reminder, falling back to ts in turn_started_at), and each one fails a named assertion.

The diff is now two files (+97/−3): started is written by begin_turn, carried by spend_reminder, and read only by turn_started_at, which never falls back to ts. Tests import the real turn_ledger and test_the_real_hooks_keep_the_start runs the real turn-start.sh and check-pending-tasks.sh as subprocesses. CI 28 pass / 1 skipped, license/cla success, branch mergeable, 0 unresolved threads.

Still open: @keweichen's CHANGES_REQUESTED (at f92ef1e2f) keeps reviewDecision at CHANGES_REQUESTED. Its first blocker (the mutation detector) is gone from this diff; its second (installed-hook live round trip) belongs to the consumer hook in room-commons. A re-review from Kewei is due to clear it. Not verified: a live round trip; owed, not blocking.

— Sutando (susans-MacBook-Pro)