add AGENTS.md

[plebhash]

No description provided.

[plebhash]

example within the context of #2319

it turns this (which would have been blindly copy-pasted as a big "clanker review" comment):

Findings

1. Interop: mining.extranonce.subscribe now rejects params it used to ignore. I verified that "params": null fails with ValueNotAnArray and ["x"] with WrongArgs. Before the PR the method accepted anything. The companion turns that parse error into a disconnect. Known ASIC firmware sends [], so it is probably fine in practice, but strictness buys nothing here. Suggest ignoring params for this method.
2. Monitorability side effect: rejections decided inside the trait are invisible to the implementer. Unauthorized workers get code 24 and extranonce2-size or version-bits failures get code 20 straight from the default method, and handle_submit is never called for them. Before, these surfaced as Err(InvalidSubmission) the app could log or count, and that variant is no longer produced by the trait. Worth a hook or an explicit note in the trait docs. The same change means unauthenticated submit spam now gets a response per message instead of an error the app could act on. That matches pool behavior, and rate limiting belongs to the app, but it moves the decision.
3. Low: JsonRpcError still requires exactly three elements. [20, "msg"] and string codes fail the whole Message parse. Not a regression, since no array form parsed before, and only IsClient users are affected. The in-tree example is the only one.
4. Low: bit-aloo's finding 2 remains. An IsClient that sends mining.extranonce.subscribe gets UnknownID on the ack. Nothing in-tree sends it.
5. Nits. The len - upstream_prefix_len subtraction is repeated inline in three channels, a local_len() on the prefix would remove it. AllocatedExtranoncePrefix::set_upstream_prefix is pub but only allocator tests use it, the companion goes through the channel setters. Client StandardChannel::set_upstream_extranonce_prefix has no test. The allocator setter takes Vec<u8> while the prefix setters take &[u8].

into a more refined list of review findings, with file/line and severity, along with the clanker comment:

Findings

1. mining.extranonce.subscribe rejects params it used to ignore.
Severity: low. Blocker: no. Verified failure modes, but no known firmware sends anything other than [], so the impact is speculative. The fix is one line and strictly safer.
Anchor: sv1/src/methods/client_to_server.rs:144

this tightens `mining.extranonce.subscribe` parsing: before this PR the method was accepted regardless of `params`, now `"params": null` fails with `ValueNotAnArray` and any non-empty array fails with `WrongArgs`. in the companion (sv2-apps#823, `sv1_server/mod.rs`) a `Client2Server::try_from` error becomes a `TproxyError::disconnect`, so a miner that sends anything other than `[]` here gets dropped instead of subscribed.

the NiceHash extension defines no params and cgminer-derived firmware sends `[]`, so this is probably fine in practice, but strictness buys nothing on a method whose only payload is the id. suggestion: keep the id and ignore `params` entirely, matching the pre-PR behavior.

2. Trait-level submit rejections are invisible to the implementer.
Severity: medium. Blocker: no, but decide before merge. It removes observability the implementer had and leaves a dead error variant, and the pending major bump is the cheap moment to add a hook.
Anchor: sv1/src/lib.rs:157, range 157 to 173

side effect worth a deliberate call: the unauthorized-worker (24) and extranonce2-size / version-bits (20) rejections are now decided and answered inside the default method, and `handle_submit` is never invoked for them. before this PR they surfaced as `Err(Error::InvalidSubmission)`, which the implementer could log, count, or use to disconnect. now the implementer has no visibility at all, so tProxy cannot include these in its share metrics. `Error::InvalidSubmission` is also no longer produced anywhere in the crate, only its definition and `Display` arm remain in `error.rs`.

answering with a proper sv1 response instead of a transport error is the right call, the observability just should not go away silently. suggestion: at minimum say in the `handle_submit` rustdoc which rejections never reach it, and drop the dead `Error::InvalidSubmission` variant. if tProxy wants to count them, a `on_submit_rejected(client_id, &submit, SubmitError)` hook with a no-op default is the smallest addition, and this major bump is the cheap moment to add it.

3. JsonRpcError accepts only 3-element arrays.
Severity: low. Blocker: no. Not a regression, since the old struct form parsed no array at all, and only IsClient users are affected.
Anchor: sv1/src/json_rpc.rs:140

`Legacy` only matches a 3-tuple, so `"error": [20, "Other/Unknown"]` fails the whole `Message` parse with "did not match any variant of untagged enum MessageWire" (a string code like `["20", ...]` fails the same way). not a regression, the derived struct form never parsed arrays at all, and only `IsClient` users are affected, of which the in-tree example is the only one. flagging it because the module docs at the top of `lib.rs` describe the error as a "list (error code, error message)", i.e. two elements.

suggestion (fine as a follow-up): deserialize the legacy form as `Vec<serde_json::Value>` and accept 2 or 3 elements, with `data` as `None` when absent.

4. IsClient cannot pair the new subscribe ack.
Severity: low. Blocker: no. Latent, nothing in-tree sends the request.
Anchor: sv1/src/methods/client_to_server.rs:129

minor asymmetry: this `From` impl gives a client a way to send `mining.extranonce.subscribe`, but the `result: true` ack the server now returns parses as a `GeneralResponse`, and `IsClient::update_response` maps an id that is neither authorize nor submit to `Error::UnknownID`. nothing in-tree sends this request, so it is latent rather than a bug. suggestion: a one-line note on this struct that `IsClient` does not track the id, or leave it until a client needs it.

5a. len - upstream_prefix_len repeated in three channels.
Severity: nit. Blocker: no.
Anchor: sv2/channels-sv2/src/extranonce_manager/prefix.rs:204

nit: `len - upstream_prefix_len` is the locally owned tail, and the same subtraction is repeated inline in the three channel setters (`client/extended.rs:222`, `server/extended.rs:371`, `server/standard.rs:332`). a `local_len()` accessor next to `upstream_prefix_len()` would let the channels write `upstream_prefix.len() + self.extranonce_prefix.local_len() + rollable` and keep the invariant in one place.

5b. AllocatedExtranoncePrefix::set_upstream_prefix is pub with no caller.
Severity: nit. Blocker: no.
Anchor: sv2/channels-sv2/src/extranonce_manager/prefix.rs:293

nit: this is `pub`, but the only callers are the allocator tests. the companion updates prefixes through the channel setters, and server channels take ownership of the `AllocatedExtranoncePrefix` at construction, so there is no window to call it afterwards. `pub(crate)` unless there is a planned external user.

5c. Client standard setter has no test.
Severity: low. Blocker: no. The method is a one-line delegate with an error mapping, so the risk is small.
Anchor: sv2/channels-sv2/src/client/standard.rs:161

nit: this is the only one of the four new channel setters without a test. the other three assert the preserved suffix, the allocation count and the transactional failure; a copy of `set_upstream_extranonce_prefix_enforces_full_extranonce_size_transactionally` from `client/extended.rs`, adapted to `allocate_standard`, would close the gap.

5d. Allocator setter takes Vec<u8>, siblings take &[u8].
Severity: nit. Blocker: no.
Anchor: sv2/channels-sv2/src/extranonce_manager/allocator.rs:232

nit: the allocator takes `Vec<u8>` while every other `set_upstream_prefix` / `set_upstream_extranonce_prefix` takes `&[u8]`, so the companion clones the same bytes once for the allocator and borrows them for each channel. taking `&[u8]` here too and copying internally keeps the four signatures uniform.

6. Two prefix-update paths with different lease guarantees, undocumented.
Severity: low. Blocker: no, optional. Documentation only, and #2330 is agreed to be a separate PR. Skip it if you consider your #2330 comment enough.
Anchor: sv2/channels-sv2/src/client/extended.rs:191

not asking to solve #2330 here (agreed it is a separate PR), but with this PR the crate ships two prefix-update paths with different guarantees: this one keeps the promise that pre-rotation jobs stay valid while dropping the old lease on line 206 (the #2330 part-1 hazard), and `set_upstream_extranonce_prefix` below keeps the lease attached. suggestion: a one-line pointer in this doc saying that callers with live jobs under an allocator-minted prefix should prefer `set_upstream_extranonce_prefix`, so the two are not picked interchangeably until #2330 lands.

[plebhash]

I think this is good because it forces me to stay on the loop during agentic reviews.

It's arguably slower than the lazy alternative of blindly copy-pasting a big "clanker review" text. And not necessarily a silver bullet, because I'm still going to copypaste a bunch of smaller "clanker review" comments and I'm not necessarily understand 100% of what they mean.

But at least I'm forced to think about it a little bit as a human reviewer, and potentially catch some hallucinations and blindspots, or refine low-quality clanker findings before they're fed back into the PR author.

[plebhash]

example where this paid off: https://github.com/stratum-mining/stratum/pull/2319/changes#r3963740631

[GitGab19]

Completely agree with this direction 👍

[bit-aloo]

LGTM