Skip to content

Revise PR #239: the verify-wake section teaches that seeing nothing proves the path works - #260

Closed
jaylfc wants to merge 2 commits into
masterfrom
exec/tsk-pg7p4b
Closed

Revise PR #239: the verify-wake section teaches that seeing nothing proves the path works#260
jaylfc wants to merge 2 commits into
masterfrom
exec/tsk-pg7p4b

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 13, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): Revise PR #239: the verify-wake section teaches that seeing nothing proves the path works

Autonomous build of board card tsk-pg7p4b.

Files:
taosmd/docs/a2a-comms.md | 52 +++++++++++++++++++++++++++++++++++++++
taosmd/skills/taosmd-a2a/SKILL.md | 10 +++-----
tests/test_a2a_watch.py | 48 ++++++++++++++++++++++++++++++++++++
3 files changed, 103 insertions(+), 7 deletions(-)

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7a52098e-cf3e-4d3d-a91f-1f9ca28c644c

📥 Commits

Reviewing files that changed from the base of the PR and between 051cf0a and e97f24d.

📒 Files selected for processing (3)
  • taosmd/docs/a2a-comms.md
  • taosmd/skills/taosmd-a2a/SKILL.md
  • tests/test_a2a_watch.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@kilo-code-bot

kilo-code-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • taosmd/docs/a2a-comms.md
  • taosmd/skills/taosmd-a2a/SKILL.md
  • tests/test_a2a_watch.py

Reviewed by step-3.7-flash · Input: 34.4K · Output: 5K · Cached: 153.2K

@jaylfc

jaylfc commented Aug 14, 2026

Copy link
Copy Markdown
Owner Author

This is the fix I wanted and it is better than what I asked for. Approving.

Reviewed on head e97f24d. 7 tests pass in tests/test_a2a_watch.py.

The blocker is fully addressed

#239's verify-wake section told a joining agent to start a watcher, observe nothing, and treat
that as proof the path worked. That is the ambiguous-probe failure exactly: silence is what a
working wake and a dead wake both produce.

The rewrite replaces it with a procedure that runs both halves:

  • Positive control: send from a sender that is not the excluded one; the trigger MUST fire
    and you MUST observe WOKE. Stated as "this is what proves the query can produce a presence".
  • Negative control: send from the excluded sender; the wake MUST NOT fire, which is what
    proves --exclude is holding rather than the path being silent because it is broken.
  • The closing line is the right one: "Only after the positive control fires AND the negative
    control holds may you treat the wake path as proven. Until then, silence is a coin flip, not
    evidence."

It also cites the fleet rule from bus 2445 in place, so a reader gets the general principle and
not just this one procedure.

Every flag the doc teaches actually exists

A runbook that teaches a flag the CLI does not have fails the same way the old section did, so I
checked rather than assumed:

a2a-watch  : --channel  --exclude  --count            all present
a2a-bridge : --channel  --trigger  --exclude  --count  all present
CONTROL    : --nonexistent-flag -> "taosmd: error: unrecognized arguments"

The control matters: it shows the help output would have told me if a documented flag were
missing.

The --count 1 framing is a good touch, since it makes the verification bounded and
self-terminating instead of leaving a watcher running that the reader then has to reason about.

Two other things in here that are worth having

  1. The floor-separation rule ("a watcher MUST NOT advance the session's READ watermark") with
    the concrete incident attached: a watermark advanced by something that was not a read hid 155
    bus messages, including a direct design question, for three days. That is the kind of rule
    that only survives if the incident travels with it.
  2. BOTH realtime wake AND the hourly cron floor, with the reason stated (the bridge only
    fires while running, the cron survives session death). The old text made the cron sound
    optional once a watcher was configured.

No blockers. Merge it.

This was referenced Aug 14, 2026
@jaylfc

jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

Reviewed by running the documented procedure, not by reading it. Blocked on one thing, and it is a reordering. Everything else here is right, and the section is a real improvement on what it replaces.

What is fixed, confirmed

  • The absence-as-proof bullet is gone, and the replacement forces a presence. That was the blocker on A2A onboarding: realtime wake is shipped but the skill never tells agents it exists #239 and it is properly discharged.
  • The bridge bullet no longer hangs. The reader is now told to generate the message they are waiting for, which was the second half of the same defect.
  • The stray "Before answering any questions about the user's history..." clause is gone from both files (0 hits in each).
  • Both the BOTH-mechanisms standard and the floor-separation rule survive, correctly.
  • Every flag the section documents actually exists. I checked each against the subparsers rather than assuming: a2a-watch has --channel/--server/--exclude/--count, a2a-bridge has those plus --trigger/--debounce/--max-concurrency. A verification doc citing a flag that argparse rejects would be its own joke.
  • The drift-guard test genuinely goes red. I ran it against A2A onboarding: realtime wake is shipped but the skill never tells agents it exists #239's text and against this branch:
#239 doc  -> FAILED  assert 'positive control' in "### verify your wake path\n\nbefore answering any questions..."
this branch -> 1 passed

BLOCKER: the negative control inspects a process that has already exited

Step 1 starts the watcher with --count 1 and says so explicitly: "so it exits on its own once it fires". Step 2 fires it. Step 3 then sends the excluded-sender message and asks the reader to "observe no trigger run".

By step 3 there is nothing left to observe. cli.py breaks out of the stream loop on if count and fired >= count, and the command returns. I ran the section exactly as written against a live taosmd serve:

bridge started            alive=YES
POSITIVE control sent  -> output='WOKE'   alive=NO     <- exited here
NEGATIVE control sent  -> output='WOKE'   alive=NO     <- nothing was listening

So "no trigger run" at step 3 is produced identically by the --exclude gate holding and by the watcher being dead. That is an ambiguous probe, in the section written to abolish ambiguous probes, and the section convicts itself two paragraphs earlier: "if the same output could mean two things, it means nothing until you have run the other one." It is the fourth sighting on this repo of a check whose PASS is reachable from zero data.

The fix is to swap steps 2 and 3. Run the negative control while the watcher is still armed, then let the positive control fire and exit. I ran that ordering too, so this is a tested suggestion and not a guess:

bridge started            alive=YES
NEGATIVE control sent  -> output=''       alive=YES    <- silence now MEANS something
POSITIVE control sent  -> output='WOKE'   alive=NO

In that order the negative control's silence is evidence, because the same process demonstrably fires one message later. Whatever wording you choose, the doc must state that the watcher has to still be running when the negative control is judged.

The test needs to move with it

test_verify_wake_doc_teaches_positive_and_negative_control asserts both phrases are present, not that the procedure is sound, so it passes on the broken ordering and would lock it in. Assert the order as well: the negative-control block must appear before the positive-control block in that section. The two existing asserts are the right guard against the original defect and should stay.

Also worth a look while you are in there: the third assert targets "proves the wake path is configured", but #239's shipped wording was "it only means the wake path is configured and listening". That assert would not have caught the text it was written against. The first two asserts are what actually make it go red. Tighten it or drop it, but do not leave it looking like protection it does not provide.

Mechanics

Per the standing ruling, this closes now and the work carries forward on a revision card rather than holding a throttle slot it cannot use. The branch is not deleted and the card carries git fetch origin exec/tsk-pg7p4b.

@jaylfc

jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

Closing under the close-on-block ruling. Revision card: tsk-dvfhq3.

The blocker is one ordering swap (the negative control runs against a watcher that --count 1 has already exited, so it cannot fail), and the card carries the measured before/after plus the test change. A blocked PR can never be revised in place here: next_card.py:32 excludes cards with an open exec/* PR, so leaving this open would hold a throttle slot it could not use and freeze the card that fixes it.

Nothing is lost. The branch exec/tsk-pg7p4b is not deleted, this review comment stays and the card links to it, and the card's STEP 0 squash-merges the branch so every good part of this PR carries forward. Most of it was good.

Reopen if the block is wrong.

jaylfc added a commit that referenced this pull request Aug 17, 2026
…er has already exited, so it cannot fail (#299)

* carry forward exec/tsk-pg7p4b

* fix wake-verify negative control ordering and doc-gate test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant