馃搵 Pre-flight Checks
馃攳 Problem Description
The native review of PR #1074 (lineage review-c5d66f2dc210046d, reliability lens) approved the change with three advisory suggestions recorded as separate later work. They are small quality gaps in the new mem_context budget surface, none blocking:
- Min-floor boundary untested:
memContextMaxBytes now falls back to the 16 KiB default for sub-integer positives, but the smallest valid explicit budget, max_bytes=1, has no test. A future refactor of the n < 1 guard could regress the boundary silently (internal/mcp/mcp.go, the resolver and its guard).
- Schema description omits the fallback: the tool description for
max_bytes documents the default and the 64 KiB clamp but not that invalid input (mistyped, non-positive, NaN, sub-integer) falls back to the default budget. A caller reading only the schema has no way to know a bad value is safe rather than an error.
- Pinned-order tiebreak assumption undocumented: the budget tests assert newest-first pinned ordering (
pinned-119 down to pinned-100) based on pinnedObservationsLimit ordering by datetime(created_at) DESC, id DESC. The test comments explain it, but the assumption depends on insert-order IDs; a future ordering change in the store would break the budget tests in a way that points at the wrong layer.
馃挕 Proposed Solution
One small follow-up PR touching only internal/mcp:
- Add a test case asserting
max_bytes=1.0 renders an exactly 1-byte budget (or the closest renderable prefix) and stays valid UTF-8.
- Extend the
max_bytes schema description with one sentence documenting the invalid-input fallback.
- Add a comment in
internal/mcp/context_budget_test.go (or a store-level note) naming the ordering dependency explicitly, so a store ordering change explains the failure at the right layer.
馃摝 Affected Area
MCP Server (tools, transport)
馃攧 Alternatives Considered
馃搸 Additional Context
馃搵 Pre-flight Checks
status:approvedbefore a PR can be opened馃攳 Problem Description
The native review of PR #1074 (lineage
review-c5d66f2dc210046d, reliability lens) approved the change with three advisory suggestions recorded as separate later work. They are small quality gaps in the new mem_context budget surface, none blocking:memContextMaxBytesnow falls back to the 16 KiB default for sub-integer positives, but the smallest valid explicit budget,max_bytes=1, has no test. A future refactor of then < 1guard could regress the boundary silently (internal/mcp/mcp.go, the resolver and its guard).max_bytesdocuments the default and the 64 KiB clamp but not that invalid input (mistyped, non-positive, NaN, sub-integer) falls back to the default budget. A caller reading only the schema has no way to know a bad value is safe rather than an error.pinned-119down topinned-100) based onpinnedObservationsLimitordering bydatetime(created_at) DESC, id DESC. The test comments explain it, but the assumption depends on insert-order IDs; a future ordering change in the store would break the budget tests in a way that points at the wrong layer.馃挕 Proposed Solution
One small follow-up PR touching only
internal/mcp:max_bytes=1.0renders an exactly 1-byte budget (or the closest renderable prefix) and stays valid UTF-8.max_bytesschema description with one sentence documenting the invalid-input fallback.internal/mcp/context_budget_test.go(or a store-level note) naming the ordering dependency explicitly, so a store ordering change explains the failure at the right layer.馃摝 Affected Area
MCP Server (tools, transport)
馃攧 Alternatives Considered
馃搸 Additional Context
R3-maxbytes-min-floor-untested,R3-maxbytes-schema-doc-fallback,R3-pinned-order-tiebreak-assumed), allSUGGESTION/ informational, non-blocking by design.