docs(agents): correct stale agent docs and add a WA Web verification path - #1109
Conversation
…path The agent docs had drifted into asserting things the code does not do, which is worse than being verbose: an agent that trusts them writes wrong code confidently. Corrections, each verified against the tree: - `message_enqueue_locks` and `Client::chat_locks` do not exist. Incoming per-chat serialization is `chat_lanes` / `ChatLane::enqueue_lock`. - `StringEnum` is not a derive. It is an internal attribute kind inside the `ProtocolNode` derive, with zero uses as a derive, and recommending it contradicted the `WireEnum` policy stated one file away. - The canonical parse path is `try_from_node_ref(&NodeRef)`, not the owned `try_from_node`. - Nibble coding lives in `encoder.rs`/`decoder.rs`; there is no `nibble.rs`. - The e2e file listing named 13 of the 26 test files. Directory trees in docs cannot stay true, so it is gone. Structure follows progressive disclosure: `AGENTS.md` keeps the repo shape and the gotchas, and routes to a doc per area with an explicit "read it when". Three of the eleven docs were previously unreachable from it. The Noise handshake moved out of the protocol-node doc into its own file, and the duplicated "never sleep in tests" rule now appears once. Adds `agent_docs/wa_web_reference.md`: how to check an implementation against real WhatsApp Web by querying the whatspec IR before falling back to the raw bundle, including where the extractor is heuristic and must not be trusted. Every query in it was run against a live `generated/` tree; the worked example confirms the three group limits in `wacore/src/iq/groups.rs` against the A/B props registry.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request refreshes repository and protocol documentation, adding whatspec-based verification guidance, revised architecture and handshake references, consolidated E2E testing practices, and updated debugging pointers. No public entity declarations are changed. ChangesDocumentation guidance
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
| Filename | Overview |
|---|---|
| agent_docs/noise_handshake.md | Documents XX/IK/fallback selection, both certificate validity bounds, and the production signature-verification path consistently with the current handshake model. |
| agent_docs/wa_web_reference.md | Adds guidance for querying the whatspec IR and falling back to captured JavaScript for control-flow questions. |
| agent_docs/protocol_architecture.md | Replaces stale trait and derive guidance with the current ProtocolNode, IqSpec, and WireEnum conventions. |
| agent_docs/e2e_testing.md | Consolidates E2E guidance around event-driven synchronization, isolation, reconnect behavior, and deterministic race testing. |
| AGENTS.md | Restructures the agent entry point around current repository invariants and routes readers to specialized documentation. |
Reviews (4): Last reviewed commit: "docs(agents): drop the machine-specific ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent_docs/e2e_testing.md`:
- Around line 30-40: Revise the reconnect guidance around client.reconnect() to
state that expected Event::Disconnected events are suppressed, so tests must
observe the offline transition through wait_for_disconnected(5) or the
connection flag. Keep the example’s wait_for_disconnected usage and explicitly
discourage waiting for Event::Disconnected.
In `@agent_docs/feature_implementation.md`:
- Around line 21-23: The real-capture validation guidance must require sanitized
captures before calculating or reporting hit rates, with PII excluded from
tests, logs, issues, and committed artifacts; require local-only handling and
define appropriate retention rules. Apply this to the “Reading evidence
honestly” section in agent_docs/feature_implementation.md (lines 21-23) and
repeat the same restriction in the raw-bundle fallback workflow in
agent_docs/wa_web_reference.md (lines 85-89).
In `@agent_docs/noise_handshake.md`:
- Line 37: The documented verify_server_cert flow must retain Ed25519 signature
verification against WA_CERT_PUB_KEY in production. Add the validation to
verify_server_cert, and isolate any e2e mock-server bypass behind Rust cfg(test)
or an explicitly test-only feature; remove the claim that the unused constant is
deliberate or that whatsmeow’s posture justifies skipping production root
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56fb678b-8081-46fc-b5e6-e70275f28046
📒 Files selected for processing (7)
AGENTS.mdagent_docs/debugging.mdagent_docs/e2e_testing.mdagent_docs/feature_implementation.mdagent_docs/noise_handshake.mdagent_docs/protocol_architecture.mdagent_docs/wa_web_reference.md
📦 Binary size report
.text per crate
Baseline: |
The Noise doc asserted that signature verification against WA_CERT_PUB_KEY was intentionally skipped and the constant deliberately unused. Both are wrong, and the claim was inherited unverified from the older doc. `verify_server_cert` XEdDSA-verifies the intermediate against WA_CERT_PUB_KEY and the leaf against the intermediate's key. The bypass is scoped to `cfg(test)` and the `danger-skip-cert-chain-verify` feature, which `tests/e2e` enables because the mock server ships an unsigned chain; production builds verify. Stating the opposite gave the reader an inverted model of server-identity validation, which is the worst possible failure mode for this particular page. The pattern-selection diagram was also incomplete. It showed only `not_after`, omitting the `not_before` check on both certs (backwards clock skew) and the unregistered-device gate that refuses IK against a legacy cached chain. Also: state plainly that `Event::Disconnected` never arrives for expected disconnects rather than describing the flag as "the only observable", and say where real-capture validation stops — report the rate, never the rows.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
agent_docs/e2e_testing.md (2)
53-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep account-sharing prefixes consistent.
Line 24 says multi-device clients must reuse one unique server-side name, but this line requires a prefix unique per client. That contradiction can make multi-device tests connect separate accounts instead of sharing one.
Suggested wording
-Use `TestClient::connect("unique_prefix")` with a prefix unique per client per test +Use `TestClient::connect("unique_prefix")` with a prefix unique per test/account; reuse it for clients that intentionally share an account🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent_docs/e2e_testing.md` at line 53, Update the testing guidance around TestClient::connect so multi-device clients reuse the same unique account-sharing prefix, while independent clients retain distinct prefixes; ensure the wording no longer requires every client in a test to have a different prefix.
53-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuarantee cleanup when
?exits early.A final
disconnect()is skipped whenever an earlier fallible operation returns an error. Require teardown on every path, such as an RAII/scope guard or equivalent test cleanup, so failed tests do not leave client tasks or sockets alive while parallel tests continue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent_docs/e2e_testing.md` at line 53, Update the e2e test cleanup pattern around TestClient connections so disconnect() runs on every exit path, including errors propagated by ?. Use an RAII/scope guard or equivalent teardown mechanism for each client, while preserving the existing unique prefixes, anyhow::Result<()> returns, and test logging initialization.agent_docs/wa_web_reference.md (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the IR as a derived model, not the contract itself.
The document later states that extraction can be degraded, response types are heuristic, values drift with
waVersion, and sequencing is absent. Calling the IR “the contract” may cause contributors to trust incomplete output instead of checking the raw bundle when required. Use wording such as “structured model of the observed contract.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent_docs/wa_web_reference.md` around lines 10 - 14, Update the “whatspec” section’s description of the IR to call it a structured or derived model of the observed contract rather than the contract itself. Preserve the explanation that the committed Rust modules consume this model and retain the existing vendor-refresh guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@agent_docs/e2e_testing.md`:
- Line 53: Update the testing guidance around TestClient::connect so
multi-device clients reuse the same unique account-sharing prefix, while
independent clients retain distinct prefixes; ensure the wording no longer
requires every client in a test to have a different prefix.
- Line 53: Update the e2e test cleanup pattern around TestClient connections so
disconnect() runs on every exit path, including errors propagated by ?. Use an
RAII/scope guard or equivalent teardown mechanism for each client, while
preserving the existing unique prefixes, anyhow::Result<()> returns, and test
logging initialization.
In `@agent_docs/wa_web_reference.md`:
- Around line 10-14: Update the “whatspec” section’s description of the IR to
call it a structured or derived model of the observed contract rather than the
contract itself. Preserve the explanation that the committed Rust modules
consume this model and retain the existing vendor-refresh guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e289c70-5057-4bc9-9b93-d752926ffeed
📒 Files selected for processing (4)
agent_docs/e2e_testing.mdagent_docs/feature_implementation.mdagent_docs/noise_handshake.mdagent_docs/wa_web_reference.md
…elper Two clarifications from review. The whatspec section called the IR "the contract", which contradicts this same document's section on where the IR is not the truth. It is a static reading of minified code: high-quality evidence, not a specification. The account-isolation paragraph explained the outcome but not the mechanism, which made it read as though sharing a prefix shares an account. It does not — `unique_push_name()` appends a UUID per call. Sharing is explicit, via `connect_as()` with one name, so say that and name the helper.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent_docs/wa_web_reference.md (1)
16-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the checkout path consistent.
The command on Line 17 clones into
./whatspec, but the comment and later query on Line 47 assume~/projects/wa-forge. Following the documented sequence can therefore read from a different or nonexistent checkout.-git clone https://github.com/oxidezap/whatspec # locally: ~/projects/wa-forge -cd whatspec +git clone https://github.com/oxidezap/whatspec ~/projects/wa-forge +cd ~/projects/wa-forge🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent_docs/wa_web_reference.md` around lines 16 - 23, Make the setup commands consistently use the documented checkout path: update the clone destination and subsequent working-directory command so they resolve to ~/projects/wa-forge, matching the later query references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@agent_docs/wa_web_reference.md`:
- Around line 16-23: Make the setup commands consistently use the documented
checkout path: update the clone destination and subsequent working-directory
command so they resolve to ~/projects/wa-forge, matching the later query
references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04b1fded-302b-4dac-8790-d3cf9fc4fba3
📒 Files selected for processing (2)
agent_docs/e2e_testing.mdagent_docs/wa_web_reference.md
The setup block cloned into `whatspec/` while the query block ran from `~/projects/wa-forge/generated`, so neither reader could follow both. The fix is not to align on the second path: that one is a local directory that happens to carry a different name than the repo, and hardcoding it would send every other contributor to a directory they do not have. Paths are now relative to the checkout, wherever it lives.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
1 participant
Summary
The docs under
agent_docs/had drifted into asserting things the code does not do. That is worse than being verbose: an agent that trusts a stale doc writes wrong code confidently, and the doc is the one source it has no way to check against. This pass verifies every factual claim against the tree, restructures the entry point around progressive disclosure, and adds a doc for the thing that was missing entirely — how to check an implementation against real WhatsApp Web.Corrections
Each of these was confirmed by reading the current code, not by memory:
message_enqueue_locksandClient::chat_locksdo not exist.AGENTS.mdnamed the first,feature_implementation.mdtold you to use the second. Incoming per-chat serialization ischat_lanes/ChatLane::enqueue_lockinsrc/client.rs;session_lockscovers Signal operations per protocol address.StringEnumis not a derive.protocol_architecture.mdlisted it under "Available derive macros". It is an internalAttrTypeinside theProtocolNodederive, has zero uses as a derive, and recommending it contradicted theWireEnumpolicy stated one file away. A contradiction between two context layers is the most expensive kind of documentation bug.try_from_node_ref(&NodeRef), not the ownedtry_from_nodethe doc showed as the trait method.encoder.rs/decoder.rs.debugging.mdpointed at awacore/binary/src/nibble.rsthat does not exist.lsdoes that job.Structure
AGENTS.mdnow holds the repo shape and the gotchas — things that look correct and are not — and routes to one doc per area with an explicit "read it when". Three of the eleven docs (voip_audio_codecs.mdamong them) were previously unreachable from it.agent_docs/noise_handshake.md. Someone adding an IQ spec should not page in 60 lines of handshake state machine.feature_implementation.mddropped the parts that restatedAGENTS.mdand kept what is specific: navigating the captured bundle.e2e_testing.md. It appears once.New:
agent_docs/wa_web_reference.mdProtocol work in this repo lives or dies on matching the official client, and there was no doc explaining how to actually check that. This one covers querying the whatspec IR first — stanza shapes, wire enums, protocol limits, notification dispatch, all as queryable JSON — and falling back to the raw bundle when the question is about control flow rather than shape.
Every
jqrecipe in it was run against a livegenerated/tree rather than written from the schema. The worked example checks the three group limits inwacore/src/iq/groups.rsagainst the A/B props registry:group_max_subject = 100,group_description_length = 2048,group_size_limit = 257, three for three. The constants are confirmed rather than folklore, and the next person to doubt them has a one-line command instead of an argument.It also documents where the IR is not the truth: generated
Responsetypes are heuristic mirrors that breakserde_json::from_value, one-of objects flatten toString(theupdate_group_propertycase we already correct by hand), andwaVersionmoves under you.Validation
Docs only — no code changes. Link check across
AGENTS.md,agent_docs/, andREADME.mdpasses with no broken references; every file path, symbol, trait method, cargo feature, and constant cited in the changed docs was grepped for before it was written. Full matrix left to CI.