docs: make proxy-security.md's MAX_STRING_LENGTH derivable (#562) - #572
Conversation
…-io#562) Follow-up from the agentrust-io#570 review. The document specs a per-string cap as a literal 1MB, which is the same number as the whole-body cap both implementations enforce. An implementer following the text literally produces a check that cannot fire, because a string at that size is already a request the body-size check rejects. That is worse than having no check. A missing control is visible as missing. A present one that cannot trigger passes review, passes an audit read of the source, and counts toward this Definition of Done. States the cap as a derivation of MAX_REQUEST_BYTES and writes down the invariant, so a later change to the body cap moves the string cap with it instead of silently recreating the unreachable condition at a new ratio. This matches what shipped in agentrust-io#570. Also records the open disagreement rather than resolving it: this document says 10MB and both implementations enforce 1MB, and nothing says whether that was deliberate tightening or drift. Left on agentrust-io#562 for a maintainer call, with a note telling implementers to follow the enforced cap meanwhile rather than raising a body cap to match the doc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
agentrust-io#562 was closed as completed by agentrust-io#570, so it is no longer a live tracker for the MAX_REQUEST_BYTES mismatch this section defers to. Filed agentrust-io#573 for that question specifically and points there instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. And you found something I missed, which is worth saying first.
My #570 review compared this document's 1MB MAX_STRING_LENGTH against the 1_000_000 body cap both implementations enforce. That was the right comparison for whether the check can fire, and it is how the dead-code case surfaced. What I did not notice is that this document's own MAX_REQUEST_BYTES says 10MB, so the spec disagrees with both implementations by a factor of ten, and I walked straight past it while reading the two constants next to each other.
Writing that up as an explicit "Open" section rather than quietly picking a side is the right treatment. Nothing on record says whether 1MB was a deliberate tightening or drift, and a docs PR is not the place to decide it. Filing #573 and pointing implementers at the enforced cap and the ratio until it settles, rather than at raising a body cap to match this document, is also the safe direction: the failure mode of guessing wrong here is a proxy that accepts ten times more than anyone intended.
Stating the cap as a derivation is the durable half. A literal is correct exactly once. MAX_REQUEST_BYTES // 2 plus the invariant written down means a later change to the body cap carries the string cap with it instead of silently recreating the unreachable condition at a new ratio, which is the specific way this would have come back.
The paragraph explaining why the invariant matters is the part I would keep even if the constant changed:
an absent control is visible as missing, while a present one passes review, passes an audit read of the source, and counts toward this Definition of Done.
That generalises well beyond this constant, and it is the reason a Definition of Done listing controls by name is not the same as having them.
One note, not a request. This adds MUST and MUST NOT to docs/spec/proxy-security.md. cmcp carries no organisational-sponsor rule for normative text the way trace-spec and agent-manifest do, so nothing blocks it and I am taking it as written. Flagging it only so you know the difference is a property of the repo rather than of the change.
Third follow-up from you in two days, and each one has gone further than the review asked. Merging.
Follow-up to the #570 review, where you said:
This is that fix. Docs only, no source touched.
What was wrong
MAX_STRING_LENGTH = 1 * 1024 * 1024is a literal, and it is the same number as the whole-body cap both implementations enforce. A string at that size is already a request the body-size check rejects, so a per-string check written at the spec's literal value is unreachable on every input that gets far enough to hit it.Someone implementing from this document faithfully ends up with dead code that reads like a control. That is the failure mode you described better than I did: a missing check is visible as missing, a present one that cannot fire passes review, passes an audit read of the source, and counts toward this Definition of Done.
What changed
The cap is stated as a derivation rather than a literal:
Plus the invariant written down, so the relationship survives someone changing the body cap later. The wording requires deriving from whatever body cap the implementation actually enforces, rather than mandating exactly half, since the ratio is a judgment call and the derivation is the part that matters.
That matches what shipped in #570 (
_MAX_ARG_STRING_LENGTH = _DEFAULT_MAX_REQUEST_BYTES // 2), so the document and the implementation now agree on shape even while they disagree on the body cap.What I did not change
The 10MB in this document versus 1MB in
scripts/mock_upstream.pyandMCPServer.__init__. I have flagged that twice now without picking a side, and it is still not mine to pick. #562 was closed as completed when #570 merged, so that question had no live tracker. Filed #573 for it specifically, and the document points there, along with a note telling implementers to follow the enforced cap meanwhile rather than raising a body cap to match the doc, since that would be the more dangerous way to resolve the mismatch unilaterally.Deriving the string cap removes the dead-code hazard at either value, so this does not block that decision either way.
I also left the DoD checklist and the Malformed Input Handling contract alone.
Verification
git diff --statis one file, +17 -1. No source changes, so the suite is unaffected. Line endings preserved as CRLF to keep the diff to the lines that actually changed.