feat: report which user-agent the verdicts came from (RP-1198) - #7
Conversation
RobotsMachine already had the matched token in hand at the match site in HandleUserAgent, but kept only the booleans (_seenSpecificAgent, _everSeenSpecificAgent). MatchedUserAgent surfaces it so a caller can record WHICH agent a robots.txt was interpreted for. pocketCrawler needs this for GoogleRobotsTxtMode.Only, where it writes the agent into the crawl manifest that the NEXT crawl reads back to decide whether it may reuse the stored robots.txt. Computing it on the caller's side would mean a second, hand-rolled scan whose tokenisation could disagree with this one - "User-agent: botify/2.0" is a botify group here, because ExtractUserAgent truncates at the first character outside [a-zA-Z_-], and is not one under a whole-value compare. Reporting only. _specificStates, _globalStates and PathAllowedByRobots are untouched, so no verdict moves; the 161-test suite, Google conformance included, passes unchanged on net8.0 and net10.0. Two semantics worth naming, since neither is forced by the name: - null means "no specific group was obeyed", the same condition PathAllowedByRobots falls back to the global rules on. A caller that speaks robots.txt renders that as "*". - when several groups match, the lowest index in the caller's list wins, not the first in the file. This under-reports on purpose: the rules obeyed are the UNION of every matching group, and one token cannot describe a two-group file. Callers pass their agents most-specific-first, so the token returned is the most specific one present. The min is what File_order_does_not_decide_the_tie pins: replacing it with first-wins fails that test and only that test - the same-order tie-break case passes either way, so the pair is needed, not just the one.
There was a problem hiding this comment.
Pull request overview
This PR adds an observable to RobotsMachine so callers can record which caller-supplied user-agent token the robots.txt verdicts were computed against, without changing allow/disallow behavior.
Changes:
- Track the lowest-index matching caller user-agent during parsing and expose it via a new
MatchedUserAgentproperty. - Update
HandleUserAgentto iterate user agents by index (to support “lowest index wins” semantics). - Add a dedicated test suite covering null/global-only semantics, tie-break behavior, token extraction, and “verdicts unchanged”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
RobotsTxt/RobotsMachine.cs |
Adds MatchedUserAgent and matched-agent tracking during parse to report which caller agent applied. |
TestRobotsTxt/TestMatchedUserAgent.cs |
Adds tests pinning MatchedUserAgent semantics and asserting verdicts remain unchanged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public RobotsMachine(byte[] robotsBody, List<byte[]> userAgents) | ||
| { | ||
| _userAgents = userAgents; | ||
| ParseRobotsTxt(robotsBody, this); | ||
| // Decoded here rather than in the property: the parse is done, so the value can be readonly and | ||
| // costs one allocation per file instead of one per read. | ||
| if (_matchedAgentIdx >= 0) | ||
| { | ||
| MatchedUserAgent = Encoding.UTF8.GetString(_userAgents[_matchedAgentIdx]); | ||
| } |
There was a problem hiding this comment.
The observation is correct — one Encoding.UTF8.GetString per RobotsMachine that matched a specific agent — but I'd like to keep it eager, for a reason that isn't just "it's small".
It isn't a hot path. RobotsMachine is constructed once per robots.txt file, in BuildFromRobotsTxtFile. The per-URL hot path is PathAllowedByRobots, which this PR does not touch. And the decode is gated on _matchedAgentIdx >= 0, so it only happens for files that actually carry a group we obey — about 30% of them in pocketCrawler's production logs, against ~86k parses/24h fleet-wide. So the change is on the order of tens of thousands of ~8-byte gen0 strings per day, fleet-wide, none of them per-URL.
Lazy-with-caching would cost something real. The caller shares these objects across threads — pocketCrawler's boost workers read robots rules concurrently, which was the subject of RP-1172 — so a cache field would be shared mutable state on a concurrently-read object. The race is benign (reference assignment is atomic, both threads would decode the same value), but "benign race" is a claim someone has to re-verify every time they touch the class. Decoding in the constructor, after the parse has finished, makes the property immutable and removes the question entirely. That seems the better trade for an allocation this size.
If the allocation does matter to a future caller, the zero-cost version is to expose the matched token as ReadOnlyMemory<byte> and let the caller decode — same cost when used, none when not, and still no shared mutable state. I didn't do that because it makes the API awkward for the only consumer today, which wants a string to compare against its own configured list. Happy to switch if you'd rather have that shape.
RobotsMachinealready had the matched token in hand at the match site inHandleUserAgent, but kept only the booleans (_seenSpecificAgent,_everSeenSpecificAgent).MatchedUserAgentsurfaces it, so a caller can record which agent a robots.txt was interpreted for.Why
pocketCrawler needs it for
GoogleRobotsTxtMode.Only(RP-1198), where the agent goes into the crawl manifest that the next crawl reads back to decide whether it may reuse the stored robots.txt and send a conditional refetch. Only mode was writing"", which matches neither arm of that gate, so one such crawl degraded every later crawl of the project.Computing it on the caller's side would mean a second, hand-rolled scan whose tokenisation could disagree with this one.
User-agent: botify/2.0is a botify group here, becauseExtractUserAgenttruncates at the first character outside[a-zA-Z_-]; it is not one under a whole-value compare. That divergence is asserted on the pocketCrawler side.Reporting only
_specificStates,_globalStatesandPathAllowedByRobotsare untouched, and the 161-test suite — Google conformance included — passes unchanged on net8.0 and net10.0. That is the evidence for "no verdict moved"; it is not a proof, and the PR does not claim more.Two semantics, neither forced by the name
nullmeans "no specific group was obeyed" — the same conditionPathAllowedByRobotsfalls back to the global rules on. A caller that speaks robots.txt renders it as*.The min is what
File_order_does_not_decide_the_tiepins. Replacing it with first-wins fails that test and only that test — the same-order tie-break case passes either way, so the swapped-order pair is what discriminates, not the obvious single case. Verified by making that mutation and running it.Tests
New
TestMatchedUserAgent(12 cases): global-only, empty file, an agent we do not carry, each single group, both tie-break orders, Google's truncation, the not-a-prefix-match counter-case, case-insensitivity, a rule-less group, and a verdicts-unchanged check.