Repository navigation
test: close Astra coverage gaps and remove the duplicate quota check - #806
Merged
thomasluizon merged 4 commits intoOct 19, 2026
Merged
Conversation
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes across the quota cleanup, Astra goal boundaries, MCP middleware coverage, and dependent fixtures.
- Single quota gate: Removed the duplicate allowance check and interface method while preserving atomic reservation, transactional claim rollback, and crisis/pick-list quota bypasses.
- MCP regression coverage: Exercised the actual selective-auth middleware for both rate-limit policies, endpoint short-circuiting, per-key isolation, and direct
get_retrospectiveclassification. - Goal and quota tests: Covered goal habit counts at and above the cap, bidirectional links, rejected writes, Free/Pro smoke-account rules, daily quota consumption, and updated gating-matrix fixtures.
- Validation: Passed 113 focused application tests, 60 focused infrastructure tests, and all 15 gating-matrix tests. The full solution suite was not rerun during this review.
gpt-6.1-sol | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the previous review at a6a09952, with the full PR diff checked for context.
- Preserved goal-cap error codes: Routed the existing rejection through
ToolResult.FromFailureand strengthened the test to requireMAX_HABITS_PER_GOAL, while retaining the message and no-write assertions. - Covered pipeline registration: Added an in-memory test of
ConfigureOrbitPipelinethat verifies an exhausted MCP budget returns 429 before the terminal delegate and uses the expected API-key partition. - Simplified quota fixtures: Renamed the block-answer test to describe its mocked claim attempt and removed redundant pick-list setup while retaining the no-consumption assertion.
- Verified affected paths: Passed 31 focused application tests and 46 focused infrastructure tests, including the new pipeline test and quota rollback coverage. The full solution suite was not rerun.
gpt-6.1-sol | 𝕏
|
This was referenced Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




An authenticated MCP POST now has regression coverage through
UseMcpSelectiveAuth: exhausting either the general or AI budget returns 429 before the endpoint delegate runs, while a second API key for the same user still reaches the endpoint. The direct AI-tool theory also coversget_retrospective.CreateGoalToolTests.csnow exercises the production constructor with real habit entities at the cap and one above it. Rejection preserves the existing error result and writes neither a goal nor links; the accepted boundary links every habit.The Astra block-answer pre-check duplicated the atomic reservation. Remove that pre-check and the obsolete allowance interface/implementation, migrate dependent fixtures and feature-explanation quota checks to
TryConsumeAiMessage, and preserve smoke-account bypass coverage for both Free and Pro users.TryConsumeAiMessageitself is unchanged. Its existing transactional rollback test still proves that quota rejection leaves the choice unclaimed.Closes thomasluizon/orbit-tickets#264 (ORB-258). Includes the folded work from thomasluizon/orbit-tickets#259, thomasluizon/orbit-tickets#260 and thomasluizon/orbit-tickets#265.
Approach
tests/Orbit.Infrastructure.Tests/Extensions/WebApplicationExtensionsMcpTests.cs: build the actual middleware delegate withApplicationBuilder, substituting authentication, rate-limit and policy ports. This covers the missing composition without hosting a server. SDK request types produce the MCP body.tests/Orbit.Application.Tests/Chat/Tools/CreateGoalToolTests.cs: test the Astra tool directly, including repository writes and both sides of habit links, without duplicating command-handler coverage.src/Orbit.Domain/Interfaces/IPayGateService.cs,src/Orbit.Application/Common/PayGateService.csandsrc/Orbit.Application/Chat/AstraBlockAnswerService.cs: remove the duplicate allowance gate. Update the dependent application/infrastructure tests,paygate.mdprovenance andtools/__tests__/gating-matrix.test.mjsto follow the reservation path.Test evidence
The following fault injections were temporary and were restored before committing. These are observed local runs, not inferred outcomes.
MCP middleware and retrospective classification
Temporarily removed the
TryApplyMcpRateLimitsAsynccall fromHandleMcpRequestAsyncand removed only the directget_retrospectiveentry fromAiBearingMcpTools, leaving the wrapped-agent operation set intact.Command for all three observations:
dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~WebApplicationExtensionsMcpTestsUseMcpSelectiveAuth_ExhaustedKey_RejectsBeforeNextAndLeavesSecondKeyUnaffectedfailed for bothmcpandmcp-aiwithExpected limited.Response.StatusCode to be 429, but found 200.TryApplyMcpRateLimitsAsync_AiBearingTool_HitsAiLimitBeforeGeneralLimitfailed forget_retrospectivebecause the second call returned true instead of false.The middleware cases use
get_daily_summary, so their failures are independent of the retrospective classification injection.Astra goal habit cap
Temporarily removed the cap guard from
CreateGoalTool.PrepareAsync.dotnet test tests/Orbit.Application.Tests --filter FullyQualifiedName~CreateGoalToolTestsHabitIds_AboveCap_ReturnsExistingErrorWithoutWritingGoalOrLinksfailed withExpected result.Success to be False, but found True. The at-cap success case passed.Reservation cleanup
Before cleanup, the unchanged focused application selection (the application command below with
--no-build) passed 106 tests. After migrating it to the reservation path it passed 104 tests; eight obsolete allowance cases were replaced with six smoke-environment/plan cases. The focused infrastructure selection passed 26 tests and the gating-matrix suite passed all 15 tests. The reservation method was compared with its original source and is unchanged.dotnet test tests/Orbit.Application.Tests --filter FullyQualifiedName~HabitIds_AboveCap_ReturnsExistingErrorWithoutWritingGoalOrLinks: unchanged test passed. Strengthened assertion failed before the fix, exit 1:Expected result.ErrorCode to be "MAX_HABITS_PER_GOAL", but found <null>at line 106. After the fix, rerun with--no-buildpassed, exit 0.dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~UseMcpSelectiveAuth_ExhaustedKey_RejectsBeforeNextAndLeavesSecondKeyUnaffected: both existing cases passed with the pipeline registration temporarily removed, confirming the gap.dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~ConfigureOrbitPipeline_LimitedMcpPost_RejectsBeforeNext: new test failed with registration removed, exit 1:Expected context.Response.StatusCode to be 429, but found 200at line 372. After restoring registration, rerun with--no-buildpassed, exit 0.dotnet build Orbit.slnx: exit 0, zero errors. Regenerated OpenAPI files were unchanged.dotnet test tests/Orbit.Domain.Tests --no-build: exit 0, 797 passed.dotnet test tests/Orbit.Application.Tests --no-build: exit 0, 5,458 passed.dotnet test tests/Orbit.Infrastructure.Tests --no-build: exit 0, 3,955 passed.node --test tools/__tests__/gating-matrix.test.mjs: exit 0, 15 passed.Validation
All commands exited 0:
dotnet build Orbit.slnxdotnet test tests/Orbit.Domain.Testsdotnet test tests/Orbit.Application.Testsdotnet test tests/Orbit.Infrastructure.Testsdotnet test tests/Orbit.Analyzers.Testsnode --test tools/__tests__/gating-matrix.test.mjsAfter the solution build, the focused MCP and goal selections passed again with 34 and 9 tests respectively. OpenAPI v1/v2 regenerated without a contract diff. Commit hooks, the timeless check, suppression allowlist and diff whitespace checks passed; the changed C# files contain no bare narration comments.
Assumptions
ToolResult.ErrorCode; preserve and assert that observed value plus the exact existing message, rather than introducing a code in a test-only change to this tool.ApplicationBuilderand mock the authentication port, rather than adding a hosted integration harness; repository conventions require unit tests and the order prohibits starting a server.gating-matrix.jsonas local generated output asCLAUDE.mdinstructs, rather than committing its unrelated existing drift even though the snapshot is tracked.Invoked the real
ConfigureOrbitPipelinein memory instead of starting an HTTP server, honoring the work order’s server prohibition.Branch applicability
Re-verified the affected code on
redesign/mainat45548c59. The localorigin/mainsnapshot at4b1c9d7ehas the same middleware limiter call, retrospective classifier, goal cap and obsolete allowance definitions. The additional Astra block-answer caller is specific to the redesign branch and is removed here.External interface evidence
MCP bodies are produced by installed ModelContextProtocol 1.4.1 types (
JsonRpcRequest,CallToolRequestParams,RequestId) usingMcpJsonUtilities.DefaultOptions, rather than handwritten protocol fixtures. The existing SDK serialization test and new middleware cases passed against that producer. The authentication substitute returns the real ASP.NETAuthenticateResult.Success(AuthenticationTicket)type; its constructor and authentication-service signatures were checked against the installed .NET reference package and exercised by the passing middleware cases. No production external response fields were added.Manual steps
None.