[thomasluizon]

Closes #599.

Every MCP tool whose capability carries a confirmation requirement was refused forever, and the retry that should clear the refusal could not. The selective-auth middleware in src/Orbit.Api/Extensions/WebApplicationExtensions.cs evaluated the agent policy a second time, in front of IAgentOperationExecutor, and it never received the caller's confirmation token.

Why carrying the token into the middleware does not work

The ticket asks for ConfirmationToken to be carried into the middleware's AgentPolicyEvaluationContext. Reading the two paths field by field shows that cannot deliver the expected behaviour, so this change removes the duplicate gate instead of duplicating the token plumbing into it.

1. A confirmation token is single use. PendingAgentOperationStore.TryConsumeFreshConfirmation calls MarkConsumed() on success. Whichever gate consumes it first leaves nothing for the second gate.
2. The two gates compute different fingerprints for the same call. The middleware computes Compute(toolName, rawMcpArgumentsJson) over the camelCase MCP arguments. The executor computes Compute(operationId, bridgeArgumentsJson) over the snake_case object McpExecutorBridge builds. For manage_api_keys those differ (keyId against key_id, and so on), so the middleware's pending operation and the executor's pending operation are two different rows. A token minted against one never matches the other.

So the middleware now steps aside for a confirmation-gated capability, exactly as it already steps aside for execute_agent_operation_v2 at the top of the same method. Both kinds of call reach IAgentOperationExecutor, which evaluates access and confirmation together, holds the caller's token, and writes the AgentAuditLogs row.

Blast radius: every StepUp capability reachable from MCP

Three capabilities declare AgentConfirmationRequirement.StepUp. All three were broken on main, and all three route through McpExecutorBridge, so all three are fixed here.

Capability	MCP tool	State before this change
subscriptions.manage	manage_subscription	Broken. Refused with step_up_required, no retry possible.
api-keys.manage	manage_api_keys	Broken. Refused with step_up_required, no retry possible.
account.manage	manage_account	Broken. Refused with step_up_required, no retry possible.

This fixes the JWT and OAuth door only. The API-key door stays closed by design: AgentTools.cs:56, :73 and :89 throw UnauthorizedAccessException for AgentAuthMethod.ApiKey, so an MCP client on an API key still cannot mint a confirmation token for any of the 15 gated tools. That behaviour is correct, it fails closed, and this change does not touch it. TheRecoveryTools_RefuseAnApiKeyCredentialSoThatDoorStaysClosed now pins it.

The same defect also blocked all twelve FreshConfirmation MCP tools, refused with confirmation_required and equally unable to retry: delete_habit, bulk_delete_habits, bulk_create_habits, bulk_update_habits, bulk_reschedule_habits, bulk_log_habits, bulk_skip_habits, delete_goal, delete_tag, delete_notification, delete_user_fact, manage_calendar_sync. AgentPlatform:ShadowModeEnabled is absent from every appsettings*.json, so it defaults to false and the refusals were live in production.

Ten of those twelve are fixed by removing the duplicate gate alone, because each already declares a confirmationToken parameter and forwards it to McpExecutorBridge.ExecuteAsync. bulk_log_habits and bulk_skip_habits were not. They declared no such parameter, and their shared helper ExecuteBulkHabitOperationAsync hardcoded confirmationToken: null, so HasFreshConfirmation could never return true and both stayed permanently refused after the first round's change. Both now declare the parameter with the same [Description(...)] shape as their HabitsBulkWrite siblings, and the helper forwards it: HabitTools.cs:458 (the bulk_log_habits parameter), :468 (the bulk_skip_habits parameter), :478 (the helper parameter) and :489 (the bridge call). Those four are read at head f3766d18 and are unchanged by round 3; an earlier revision of this body cited :469 and :479, which are the two CancellationToken lines.

Every hardcoded confirmationToken: null in HabitTools.cs, one verdict each

null is correct wherever the forwarded operation's capability declares AgentConfirmationRequirement.None, because no confirmation is ever evaluated for it. Line numbers are the review's, at head 9c7d8d56.

Site	Operation forwarded	Capability	Requirement	Verdict
:132	create_habit	habits.write	None	null correct
:166	update_habit	habits.write	None	null correct
:197	log_habit	habits.write	None	null correct
:236	skip_habit	habits.write	None	null correct
:254	update_checklist	habits.write	None	null correct
:341	create_sub_habit	habits.write	None	null correct
:357	duplicate_habit	habits.write	None	null correct
:486	bulk_log_habits, bulk_skip_habits	habits.bulk.write	FreshConfirmation	Defect. Fixed here (now :489).
:502	reorder_habits	habits.write	None	null correct
:518	move_habit_parent	habits.write	None	null correct
:541	link_goals_to_habit	habits.write	None	null correct

The last row was not among the review's ten; it carries the same verdict as the other habits.write sites. The same sweep over the other eleven files in src/Orbit.Api/Mcp/Tools/ found no further instance. Every one of the remaining fourteen gated MCP tools already threads the caller's parameter, and every other confirmationToken: null in that folder sits on a capability declaring None. Guard 5 below now enforces that instead of leaving it to a reader.

Field by field against the chat path

Field	Verdict
OperationFingerprint	Carried already, and left as is. The middleware needs its own value because EvaluateAccessRequirements denies a mutation with a blank fingerprint.
OperationArgumentsJson	Correctly omitted. It is read in exactly one place, EvaluateConfirmationRequirement, to seed a pending operation's arguments. The middleware no longer reaches that branch, so passing it would be dead.
ConfirmationToken	Correctly omitted, for the two reasons above. The executor receives it from the tool's own confirmationToken parameter.
StepUpSatisfied	Deleted from the record. Nothing ever set it and nothing ever read it. The real step-up state is PendingAgentOperationState.StepUpSatisfiedAtUtc, which AgentStepUpService.VerifyChallengeAsync writes and IsUsable reads. The chat surface does not set this field either, so there was no second mechanism to copy.
ConfirmationRequirementOverride	Does not exist. The ticket lists it as one of six optional parameters on AgentPolicyEvaluationContext; the record declares five, and this is not among them. Nothing to carry or omit.

What the MCP surface does when the token is absent

It refuses and names the recovery path, through McpExecutorBridge.BuildPendingMessage. That message and the three tools' confirmationToken descriptions all named verify_step_up_agent_operation_v2 as the source of the token. It does not return one: it returns a PendingAgentOperation and marks StepUpSatisfiedAtUtc. Only confirm_agent_operation_v2 mints a token, via PendingAgentOperationStore.Confirm. All four strings now name the real four-step recovery: step_up_agent_operation_v2, then verify_step_up_agent_operation_v2, then confirm_agent_operation_v2 for the token, then retry the tool with it. StepUpTool_AfterTheFourRecoveryToolsRun_RunsInsteadOfBeingRefused drives that exact sequence through the real AgentTools methods rather than through the services under them, and asserts the retry runs.

Five guard tests hold the deferral safe

Skipping the middleware gate is only safe while every confirmation-gated MCP tool actually reaches the executor and arrives there carrying a gate the executor can enforce. tests/Orbit.Infrastructure.Tests/Mcp/ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs pins all of it.

1. EveryConfirmationRequirement_SitsOnAMutation. Only a mutation is guaranteed to reach the executor.

2. TheSourceScan_ReachesEveryConfirmationGatedMcpToolInTheCatalog. The scan's own tripwire. It asserts the set of tools the source scan found equals the set of gated mcpTools in the catalog, so a scan that quietly stopped finding anything cannot turn the three guards below green while enforcing nothing.

3. EveryConfirmationGatedMcpTool_RoutesThroughTheExecutorBridge. Every gated tool reaches executorBridge.ExecuteAsync, following one level into a same-file helper so the shared bulk-habit wrapper counts.

4. EveryConfirmationGatedMcpTool_ForwardsAnOperationIdCarryingTheSameConfirmationRequirement. The hole the first round left open. The old guard only asked whether the text executorBridge.ExecuteAsync( appeared, never which operation id was passed, and the executor gates on catalogService.GetCapability(operation.CapabilityId), not on the tool name. The two can diverge. Swap delete_notification's forwarded "delete_notifications" for the file's own UpdateNotificationsTool const and three things happen together: the tool name still resolves to a gated capability so the middleware still steps aside, a bridge call is still present so the old guard stays green, and the executor gates on NotificationsWrite, which declares None. The tool would then run with no confirmation at all. The new guard resolves the forwarded operation id out of the source (a string literal, a same-file const string, or a helper parameter bound positionally from the caller's argument) and asserts the capability it resolves to is the tool's own: same Id and same ConfirmationRequirement. Round 3 added the Id half. Matching the requirement alone was not enough, because a gated tool could forward a different gated capability's operation id and stay green: swapping TagTools.cs:82 from "delete_tag" to "delete_goal" leaves both at FreshConfirmation, so confirmation still fires, but the executor then enforces goals.delete in place of tags.delete.

5. EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken. Pins the bulk_log_habits/bulk_skip_habits defect so it cannot return: each gated tool must declare a string? confirmationToken parameter, and that bare identifier must be what reaches the bridge's token slot. The resolver is a text scan over one file, not a compiler. It accepts the identifier written directly in the bridge call, or carried through exactly one same-file helper, and it refuses a tool that writes to the parameter with =, ??= or += before the call. A conditional expression, an omitted optional argument, default, null!, (string?)null, "" and an unrelated local all fail, because none of them is that identifier. The helper hop pairs a caller argument with a helper parameter by position alone, and it strips a named-argument prefix without reading the name. Several members can share one name, so the scan reads every member whose parameter count can admit the call: an ambiguous name reddens the tool instead of resolving to one of them. Two or more helper hops read as no bridge call at all and fail guard 3 above; that direction is safe, so it stands. Aliasing, reflection and a call reached through an interface are outside what the scan models, and an author who sets out to defeat a text scan can spell the bypass another way. What this guard closes is every shape an ordinary refactor produces.

   Round 3 replaced a weaker rule here. The guard first flagged a call only when the token argument was absent or the literal text null, which whitelisted one spelling: confirmationToken: default, null!, (string?)null and "" are all null for a string? and all passed, leaving both bulk tools permanently refused with the guard silent. Adding default to the literal list would not close it, because the next spelling is not on the list either. Asserting the structural property instead - the forwarded expression is the tool's own parameter - refuses every spelling in one rule, including an unrelated local. The declaration check also now reads the tool's parameter list rather than its whole body, so a local named confirmationToken no longer satisfies it. This is the structural guarantee that made the guard preferable to hand-enumerating each confirmationToken: null site, so a one-spelling whitelist was weaker than the claim above it. Round 4 then fixed the parser under that rule. The rule is right; four defects in its helpers let five mutations compile, restore the Reduce daily summary log egress for ticket 763 #599 defect and leave all five guards green. Section 9 shows each one.

Two scanner defects are fixed in the same file. Directory.EnumerateFiles used SearchOption.TopDirectoryOnly, so any subfolder added under Mcp/Tools was skipped silently; it now walks AllDirectories. BuildMemberBodies keyed a member by the first \w+( token in its chunk, which is an invocation pattern, not a declaration. That mis-keys any member the boundary regex does not split on, including one written without an explicit access modifier, and it dropped a generic method outright: McpToolHelpers.DeserializeJson<T> was absent from the lookup at the previous head, because > sits between the name and the (. Members are now keyed from the declaration itself. This one produces a false red rather than a false green, so it was the lower risk of the two, but it was still wrong.

Together with the existing McpToolsDoNotDispatchCommands guard, a future tool that skipped the bridge, forwarded the wrong operation id, or dropped its token turns one of these red instead of silently losing its gate.

An unknown operation leaves an audit row again

AgentOperationExecutor.ExecuteAsync returned UnknownOperation before any TryAuditAsync, so once the middleware stepped aside a gated tool naming a missing operation left no AgentAuditLogs row where the legacy middleware path used to write one. A trail gap, not a gate gap, but it is this change that opened it. DenyUnknownOperationAsync now writes the row first: capability id unknown, risk Low, policy decision Denied, outcome UnsupportedByPolicy, error unsupported_by_policy, with the arguments redacted the same way every other row is. AuditContext carries a capability id and a risk class instead of an AgentCapability, because an unknown operation has no capability to carry.

Test evidence

Commands run from the worktree root. Sections 1 to 4 are round 1 and still hold. Sections 5 and 6 are round 2. Sections 7 and 8 are round 3, at head db92bcec. Sections 9 and 10 are round 4, at head 2a455864. Sections 11 and 12 are round 5, at this head.

1. Existing tests, unchanged, with the defect present. Source reverted to b6a92960, both new test files removed.

dotnet test tests/Orbit.Infrastructure.Tests --filter "FullyQualifiedName~Mcp|FullyQualifiedName~AgentPolicyEvaluatorTests|FullyQualifiedName~AgentStepUpServiceTests"

Result: Com falha: 0, Aprovado: 263. All 263 pass. No existing test caught the defect, including the MCP middleware tests, the policy evaluator tests and the step-up service tests.

2. New tests with the defect still present. Full fix restored, then only the capability skip removed from HandleMcpToolCallAsync.

dotnet test tests/Orbit.Infrastructure.Tests --filter "FullyQualifiedName~McpConfirmationGateTests|FullyQualifiedName~ConfirmationGatedMcpToolsRouteThroughExecutorTests"

Result: Com falha: 3, Aprovado: 3. The three that fail are the three that drive the gate, and each fails for the intended reason:

McpConfirmationGateTests.StepUpTool_WithoutToken_IsRefusedByTheExecutorAndNamesTheRecoveryPath [FAIL]
  Expected outcome.PolicyError to be <null>, but found "step_up_required".
McpConfirmationGateTests.StepUpTool_WithConfirmedStepUpToken_RunsInsteadOfBeingRefused [FAIL]
  Expected outcome.PolicyError to be <null>, but found "step_up_required".
McpConfirmationGateTests.StepUpTool_WithAnUnknownToken_StaysRefused [FAIL]
  Expected outcome.PolicyError to be <null>, but found "step_up_required".

The middleware wrote the JSON-RPC policy error and the tool never ran, which is the defect exactly. ToolWithoutConfirmationRequirement_IsStillGatedByTheMiddleware and both guard tests pass in this state, so the red is specific to the gate.

3. The same tests after the fix. Same command: Com falha: 0, Aprovado: 6.

4. Whole suite after the round 1 fix. dotnet test Orbit.slnx

Orbit.Domain.Tests           Com falha: 0, Aprovado:  585
Orbit.Analyzers.Tests        Com falha: 0, Aprovado:   32
Orbit.Application.Tests      Com falha: 0, Aprovado: 3393
Orbit.Infrastructure.Tests   Com falha: 0, Aprovado: 2241

McpConfirmationGateTests drives the production path rather than a stand-in: the real WebApplicationExtensions.HandleMcpToolCallAsync, the real McpExecutorBridge, AgentOperationExecutor, AgentPolicyEvaluator, PendingAgentOperationStore and AgentStepUpService over the real capability catalog, with the six-digit code read out of the email service the way a user reads it. Only the backing IAiTool and the ownership lookup are stubs, because the subject is authorization, not the revoke itself.

Round 1 ran before the rename, so its output names StepUpTool_WithConfirmedStepUpToken_RunsInsteadOfBeingRefused. That test is now StepUpTool_AfterTheFourRecoveryToolsRun_RunsInsteadOfBeingRefused and drives the four AgentTools methods directly.

5. Round 2. Each new assertion was broken on purpose and watched go red, then restored and watched go green. The two guards read the tool sources off disk, so a mutation to src/ turns them red with no rebuild.

5a. The P2 hole, driven exactly as the review describes it. NotificationTools.cs:130 changed from "delete_notifications" to the file's own UpdateNotificationsTool const, which is "update_notifications".

dotnet test tests/Orbit.Infrastructure.Tests --no-build --filter "FullyQualifiedName~ConfirmationGatedMcpToolsRouteThroughExecutorTests"

EveryConfirmationGatedMcpTool_ForwardsAnOperationIdCarryingTheSameConfirmationRequirement [FAIL]
  Expected offenders to be empty because the executor gates on the capability behind the
  forwarded operation id, not on the MCP tool name, so a gated tool forwarding an ungated
  operation id runs with no confirmation while the middleware still steps aside.
  Offending tool(s):
NotificationTools.cs: delete_notification (FreshConfirmation) forwards operation
  'UpdateNotificationsTool' resolving to notifications.write (None)

Com falha: 1, Aprovado: 4

The other four stayed green, including EveryConfirmationGatedMcpTool_RoutesThroughTheExecutorBridge, which is the old guard: a bridge call was still present, so it saw nothing wrong. That is the hole, reproduced. After git checkout -- src/Orbit.Api/Mcp/Tools/NotificationTools.cs, same command: Com falha: 0, Aprovado: 5.

5b. The P1, with the fix reverted. HabitTools.cs restored to head 9c7d8d56, same command:

EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken [FAIL]
  Offending tool(s):
HabitTools.cs: bulk_log_habits declares no confirmationToken parameter
HabitTools.cs: bulk_log_habits forwards confirmationToken 'null' to the executor
HabitTools.cs: bulk_skip_habits declares no confirmationToken parameter
HabitTools.cs: bulk_skip_habits forwards confirmationToken 'null' to the executor

Com falha: 1, Aprovado: 4

It names both broken tools and nothing else. With the fix restored: Com falha: 0, Aprovado: 5. Round 3 rewrote this guard's failure message, so re-running this mutation today prints the round 3 wording shown in section 7 rather than the text quoted here.

5c. The restored audit row. DenyUnknownOperationAsync swapped back for the bare return AgentOperationResponseFactory.UnknownOperation(...).

dotnet test tests/Orbit.Infrastructure.Tests --no-build --filter "FullyQualifiedName~AgentOperationExecutor_ReturnsUnsupportedWhenOperationIsMissing"

AgentOperationExecutor_ReturnsUnsupportedWhenOperationIsMissing [FAIL]
  NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
    RecordAsync(entry => ... entry.Error == "unsupported_by_policy"), any CancellationToken)
  Actually received no matching calls.

Com falha: 1, Aprovado: 0

Restored: Com falha: 0, Aprovado: 1.

6. Whole suite at round 2's head, f3766d18. dotnet build Orbit.slnx -v q --nologo succeeds with 0 errors, then dotnet test Orbit.slnx --no-build:

Orbit.Domain.Tests           Com falha: 0, Aprovado:  585
Orbit.Analyzers.Tests        Com falha: 0, Aprovado:   32
Orbit.Application.Tests      Com falha: 0, Aprovado: 3393
Orbit.Infrastructure.Tests   Com falha: 0, Aprovado: 2245

6255 tests, zero failures, up 4 from round 1's 6251: three new guards plus the API-key refusal test. CSC : warning CS9057 confirms the local SDK still skips Orbit.Analyzers, so ORBIT0001 to ORBIT0005 did not run locally; every added line was grepped by hand and carries no // comment that is not an XML doc. node tools/check-suppression-allowlist.mjs reports all 101 declared sites accounted for, and this round adds no suppression. node tools/arch-map.mjs regenerates architecture.json and architecture.html with no drift.

7. Round 3. Six mutations, each red on its own, real tree green before every one of them. Command throughout, run in the foreground:

dotnet test tests/Orbit.Infrastructure.Tests/Orbit.Infrastructure.Tests.csproj --no-build --filter "FullyQualifiedName~ConfirmationGatedMcpToolsRouteThroughExecutorTests"

Each mutation was also compiled (dotnet build src/Orbit.Api/Orbit.Api.csproj -v q --nologo, 0 Erro(s) every time), so none of the six is a proof against code that would not build. Each was reverted with git checkout -- <file> and the tree re-run green before the next.

7a to 7d. The P2 hole, one spelling of null at a time. HabitTools.cs:489, the shared bulk helper's bridge call. Real tree before each: Com falha: 0, Aprovado: 5.

Mutation at :489	Result	Offenders named
}, confirmationToken: default, cancellationToken);	Com falha: 1, Aprovado: 4	bulk_log_habits, bulk_skip_habits forward 'default'
}, confirmationToken: null!, cancellationToken);	Com falha: 1, Aprovado: 4	both forward 'null!'
}, confirmationToken: (string?)null, cancellationToken);	Com falha: 1, Aprovado: 4	both forward '(string?)null'
}, confirmationToken: "", cancellationToken);	Com falha: 1, Aprovado: 4	both forward '""'

All four fail EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken and nothing else. Full text of the first:

EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken [FAIL]
  Offending tool(s):
HabitTools.cs: bulk_log_habits forwards 'default' to the executor instead of its own
  confirmationToken parameter
HabitTools.cs: bulk_skip_habits forwards 'default' to the executor instead of its own
  confirmationToken parameter

Com falha: 1, Aprovado: 4

At the previous head all four were Com falha: 0, Aprovado: 5. That is the defect this round closes.

7e. A gated tool that declares the parameter and then forwards a different local. TagTools.DeleteTag given var token = confirmationToken; and passing token to the bridge. This is the direct-call path, not the helper path, so it also shows the rule holds without a hop.

EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken [FAIL]
  Offending tool(s):
TagTools.cs: delete_tag forwards 'token' to the executor instead of its own
  confirmationToken parameter

Com falha: 1, Aprovado: 4

7f. The P3, the operation-id swap. TagTools.cs:82, "delete_tag" changed to "delete_goal". Both capabilities are FreshConfirmation, so this was green before round 3.

EveryConfirmationGatedMcpTool_ForwardsAnOperationIdCarryingTheSameConfirmationRequirement [FAIL]
  Offending tool(s):
TagTools.cs: delete_tag (tags.delete, FreshConfirmation) forwards operation '"delete_goal"'
  resolving to goals.delete (FreshConfirmation)

Com falha: 1, Aprovado: 4

8. Whole suite at round 3's head, db92bcec. dotnet build Orbit.slnx -v q --nologo succeeds with 0 Erro(s), then dotnet test Orbit.slnx --no-build:

Orbit.Analyzers.Tests        Com falha: 0, Aprovado:   32
Orbit.Domain.Tests           Com falha: 0, Aprovado:  585
Orbit.Application.Tests      Com falha: 0, Aprovado: 3393
Orbit.Infrastructure.Tests   Com falha: 0, Aprovado: 2245

6255 tests, zero failures, unchanged from round 2: this round tightens three existing assertions and adds no test. Round 3 touches one file, tests/Orbit.Infrastructure.Tests/Mcp/ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs, so src/ is untouched, no suppression is added and architecture.json reports no drift. CSC : warning CS9057 still confirms the local SDK skips Orbit.Analyzers, so ORBIT0001 to ORBIT0005 did not run locally; every added line was grepped by hand for a bare // or /* */ outside /// and none is present.

One assertion deleted. McpConfirmationGateTests.cs:116 held outcome.ToolOutput.Should().NotBeNull();, which cannot fail under a real defect because the four Should().Contain(...) calls after it already throw on a null string. It is gone.

9. Round 4. Five parser bypasses, each red on its own, the real tree green before and after every one of them. Round 3's rule is the right shape, but the parser helpers under it were not load-bearing until round 3 made them so. Five mutations compiled, restored the #599 defect and left all five guards green. Same foreground command as section 7, with dotnet build Orbit.slnx -v q --nologo reporting 0 Erro(s) before every run, and git checkout -- <file> plus a green re-run between them.

Mutation	Before round 4	After round 4	Offender named
HabitTools.cs:489 as }, ids.Count > 0 ? null : confirmationToken, cancellationToken);	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	bulk_log_habits and bulk_skip_habits forward 'ids.Count > 0 ? null : confirmationToken'
TagTools.cs:82 as ExecuteAsync(user, tagId.Length > 0 ? "delete_goal" : "delete_tag", ...)	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	delete_tag (tags.delete, FreshConfirmation) forwards an operation resolving to an unknown capability
ExecuteBulkHabitOperationAsync given string? confirmationToken = null, called from bulk_skip_habits without that argument	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	bulk_skip_habits forwards '<omitted>'
A correct bridge call kept in a dead if (tagId.Length == 0) branch, with the real path sent to a same-file helper forwarding null	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	delete_tag forwards 'null'
confirmationToken = null; above the TagTools.DeleteTag bridge call	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	delete_tag assigns to confirmationToken before the executor reads it

Four root causes, one commit each, all in the guard's own parser.

• ArgumentAt read any : as a named-argument separator, so a conditional in either slot collapsed to its else-branch. One defect defeated two guards, because the token slot and the operation-id slot share that method. It now strips a ^\w+\s*:(?!:) prefix only, which leaves a colon inside a string literal and a :: qualifier alone.
• ResolveExpression gave one answer to two questions. "Not a parameter" correctly returned the expression; "the caller passed nothing at that index" returned the helper's own parameter name, which then compared equal to confirmationToken. The second case now returns an <omitted> sentinel, which can never equal the parameter name and resolves to no capability.
• CollectBridgeCalls returned as soon as the tool body held one bridge call, so a decoy hid every helper call in the same tool. It now reads the direct and helper sets together.
• The rule never looked for an assignment to the parameter. \bconfirmationToken\s*=[^=>] over the tool's statements, and over a same-file helper it reaches, now adds an offender. The parameter list is excluded so the declaration's own = null default does not match, and ==, !=, >=, <=, ??= and a lambda arrow do not trip it.

9a. The six earlier mutations still redden. Each applied to TagTools.DeleteTag, compiled with 0 Erro(s), run, then reverted: confirmationToken: default, null!, (string?)null, "", a forwarded var token local, and "delete_tag" swapped for "delete_goal". All six print Com falha: 1, Aprovado: 4, the first five on the token guard and the last on the operation-id guard. A fix that broke an existing guard would not be a fix.

9b. Both legitimate shapes still pass, and the two-hop shape is disclosed. A tool whose only bridge call is direct is the real delete_tag, green. delete_tag refactored onto one same-file helper that receives and forwards confirmationToken compiles and stays green, so reading direct and helper calls together did not cost the one-hop refactor. Two helper hops go red on EveryConfirmationGatedMcpTool_RoutesThroughTheExecutorBridge: the resolver follows exactly one hop, so it sees no bridge call at all. That is a false red, not a false green, so it is left as is.

10. Whole suite at round 4's head, 2a455864. dotnet build Orbit.slnx -v q --nologo succeeds with 0 Erro(s), then dotnet test Orbit.slnx --no-build:

Orbit.Analyzers.Tests        Com falha: 0, Aprovado:   32
Orbit.Domain.Tests           Com falha: 0, Aprovado:  585
Orbit.Application.Tests      Com falha: 0, Aprovado: 3393
Orbit.Infrastructure.Tests   Com falha: 0, Aprovado: 2245

6255 tests, zero failures, unchanged from rounds 2 and 3: round 4 fixes four parser helpers and one doc block, and adds no test. It touches the same single file, tests/Orbit.Infrastructure.Tests/Mcp/ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs, so src/ is untouched, no suppression is added and architecture.json reports no drift. CSC : warning CS9057 still confirms the local SDK skips Orbit.Analyzers, so ORBIT0001 to ORBIT0005 did not run locally; every added line was grepped for a bare // or /* */ outside /// and none is present.

11. Round 5. Two last bypasses, one false red removed, and the prose cut back to what the code does. Round 4's thirteen fresh attacks found nine already closed, including a colon and a comma inside a string literal, global:: in the call, a confirmationTokenBackup local, == and >= near the assignment check, three direct bridge calls, and a helper shared by a gated and an ungated tool. Two got through. Every run below is a git archive extract of the head under test, with dotnet build src/Orbit.Api/Orbit.Api.csproj reporting 0 Erro(s) before each guard run and the tool sources restored from a pristine copy between runs.

Mutation	At 2a455864	At this head	Offender named
confirmationToken ??= tagId; above the TagTools.DeleteTag bridge call	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	delete_tag assigns to confirmationToken before the executor reads it
confirmationToken += string.Empty; above the same bridge call	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	the same offender
A same-name ExecuteBulkHabitOperationAsync overload holding its own null token forward, declared after the real helper	Com falha: 0, Aprovado: 5	Com falha: 1, Aprovado: 4	bulk_log_habits and bulk_skip_habits forward 'null'
The identical overload declared before the real helper	Com falha: 1, Aprovado: 4	Com falha: 1, Aprovado: 4	the same two tools
A correct 1-parameter ExecuteBulkHabitOperationAsync message helper declared before the real helper, and called by it	Com falha: 1, Aprovado: 4, a false red on guard 3	Com falha: 0, Aprovado: 5	none

Two root causes, one commit each, both in the guard's own parser.

• The assignment rule missed every compound assignment. \bconfirmationToken\s*=[^=>] anchors on \s*, which cannot cross the ?? of ??= or the + of +=. confirmationToken ??= tagId; compiled green with all five guards green, and that is not a contrived line: a later change writing confirmationToken ??= await ResolveStoredTokenAsync(user, cancellationToken); hands the executor a token the MCP caller never sent, so HasFreshConfirmation is satisfied from state the caller does not control while the middleware has already stepped aside. The rule is now \bconfirmationToken\s*(?:\?\?|\+)?=[^=>], which admits the only two compound operators a string? can carry. == still fails on the second =, !=, >= and <= still fail because those characters break \s* and are not in the alternation, => still fails on the trailing character class, and a bare ?? with no = still fails.

• The member index collapsed overloads. It was a dictionary keyed on the bare name and built with group.First(), so two same-named members became whichever was declared first, and the scan then analysed a method the tool never calls. It broke in both directions, and declaration order was the only thing that decided it.

  The lookup now holds every member under its name, and each call site reads every candidate whose parameter count can admit that many arguments. One candidate resolves the call; more than one is ambiguous, so all of them are read and any bad one reddens the tool. That is the fail-closed option rather than the key-on-arity-alone option, chosen because the false red is the nastier half of this defect and arity alone would still have to pick one member when two overloads share an argument count. The cheap arity filter is what removes the false red in the table's last row, where the same-named sibling cannot take seven arguments. A name that admits no candidate stays unresolved, and that hides nothing: the bridge call inside it is not collected either, so guard 3 reddens the tool. Arity, not the parameter type, decides a candidate, because a text scan cannot type-check an argument.

11a. The four ordered negatives still pass, because a fix that reddens correct code is not a fix. Each compiles with 0 Erro(s) and prints Com falha: 0, Aprovado: 5: if (confirmationToken == null && tagId.Length >= 1 && confirmationToken != tagId) in DeleteTag; a string? confirmationTokenBackup = null; local beside a correct confirmationToken: confirmationToken named argument; a DescribeTokens helper reached from DeleteTag whose body holds the lambda confirmationToken => confirmationToken ?? "none"; and the correct-overload row in the table above, where the ambiguity is resolved by arity rather than guessed.

11b. All eleven earlier mutations still redden. Round 4's five - the conditional in the token slot, the conditional in the operation-id slot, the optional-helper-parameter refactor, the decoy direct call beside a helper forwarding null, and confirmationToken = null; - plus confirmationToken: default, null!, (string?)null, "", a forwarded var token local and the delete_tag to delete_goal swap. All eleven compile and all eleven print Com falha: 1, Aprovado: 4.

11c. Three limits are known and deliberately left. A named argument is stripped without checking its name, so a helper call written ..., confirmationToken: null, filterJson: confirmationToken, ... stays green while C# binds the token into filterJson; nobody reorders two named arguments by accident, and it is confined to the helper hop because the bridge signature has one string? slot. A correct call reddens when its trailing named arguments are reordered. SplitTopLevelArguments does not track < and >, so a new Dictionary<string, string> payload garbles the failure message. The last two fail closed, and the first needs a name-aware lookup that the arity filter did not make cheap. The doc comment now states the first two rather than claiming more than the code does.

12. Whole suite at this head. dotnet build Orbit.slnx -v q --nologo succeeds with 0 Erro(s), then dotnet test Orbit.slnx --no-build:

Orbit.Analyzers.Tests        Com falha: 0, Aprovado:   32
Orbit.Domain.Tests           Com falha: 0, Aprovado:  585
Orbit.Application.Tests      Com falha: 0, Aprovado: 3393
Orbit.Infrastructure.Tests   Com falha: 0, Aprovado: 2245

6255 tests, zero failures, unchanged from rounds 2, 3 and 4: round 5 fixes one regex, one lookup and one doc block, and adds no test. It touches the same single file, tests/Orbit.Infrastructure.Tests/Mcp/ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs, so src/ is untouched, no suppression is added and architecture.json reports no drift. CSC : warning CS9057 still confirms the local SDK skips Orbit.Analyzers, so ORBIT0001 to ORBIT0005 did not run locally; every added line was grepped for a bare // or /* */ outside /// and none is present.

Assumptions

• The deferral is keyed on capability.ConfirmationRequirement, not on a list of executor-routed tool names. Rejected alternative: a hand-maintained set of the 40 bridge-routed tools, like AiBearingMcpTools. That set would drift, and the two guard tests make the requirement-based rule self-enforcing instead.
• StepUpSatisfied is deleted rather than wired up. Rejected alternative: setting it in both call sites and reading it in AgentPolicyEvaluator. That would be a second step-up mechanism beside StepUpSatisfiedAtUtc, which the ticket explicitly warns against.
• The new gate test lives in tests/Orbit.Infrastructure.Tests/Extensions/ beside WebApplicationExtensionsMcpTests, and the architecture guard in tests/Orbit.Infrastructure.Tests/Mcp/ beside McpToolsDoNotDispatchCommandsTests. Rejected alternative: one file holding both, which would mix a behaviour test with a source-scanning guard.
• HandleMcpToolCallAsync and McpToolCallRequest widen from private to internal so the test can drive the real entry point. Orbit.Api already grants InternalsVisibleTo("Orbit.Infrastructure.Tests"). Rejected alternative: reaching it over a hosted ASP.NET pipeline, which the unit-only suite does not host.
• architecture.json and architecture.html are regenerated by node tools/arch-map.mjs and shipped in the same commit, because the generator picked up the new test class. Round 2 adds no new test class, so the generator reports no drift and neither file changes again.
• The token guard demands the tool forward the confirmationToken identifier itself, not merely a value derived from it. A tool writing var token = confirmationToken; and forwarding token fails, even though it is behaviourally correct. That strictness is the point: the alternative is deciding, from source text, whether an arbitrary expression happens to evaluate to the caller's token, which is the enumeration problem that let default through. Forwarding the parameter is what every one of the fifteen gated tools already does, and the failure message says exactly what to write.
• The guard resolves a forwarded operation id from the source text rather than from a running catalog, because an operation id is only bound to a capability through GetCapabilityByChatTool(tool.Name) at startup, and the unit suite instantiates no IAiTool set. Rejected alternative: asserting over every string literal in the tool's body. Three gated tools name confirm_agent_operation_v2 in a [Description(...)], and that is itself a chat tool on ChatInteract with None, so the loose rule would have gone red on tools that are correct.

Follow-up found but not fixed here

An allowed MCP mutation now writes one accurate AgentAuditLogs row from the executor. Before this change it wrote two: the executor's row plus a lower-fidelity legacy row from ExecuteAuthorizedMcpToolCallAsync, which reports Allowed/Succeeded without seeing the tool's real outcome. That duplicate still happens for every non-confirmation MCP mutation, and predates this ticket. It is a separate defect in a separate concern and is not widened into this change.

A gated tool that reaches McpExecutorBridge through two or more same-file helper hops is refused by EveryConfirmationGatedMcpTool_RoutesThroughTheExecutorBridge, because the resolver follows exactly one hop and then finds no bridge call. No tool in src/Orbit.Api/Mcp/Tools/ has that shape today. It fails closed, so deepening the resolver is a separate change rather than part of this one.

Three narrower limits of the same text scan are listed in section 11c and left for the same reason: the named-argument prefix is stripped without reading the name, a correct call reddens when its trailing named arguments are reordered, and SplitTopLevelArguments does not track < and >. Two of the three fail closed. The guard's doc comment now states these limits and says plainly that a source-text scan is defeatable by an author who sets out to defeat it, so the next reader does not mistake it for an airtight rule.

🤖 Generated with Claude Code

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

REQUEST CHANGES, head 9c7d8d56756e33d21670b567784db3311cacb73e.

Pullfrog could not run this review. Codex and Pullfrog share one OpenAI meter, exhausted until 2026-09-22. A separate agent reviewed in the worktree carrying this exact head, and the orchestrator re-checked the blocking finding by hand. The substitution is named so nobody later reads this as having had the usual reviewer.

git rev-parse HEAD printed 9c7d8d56756e33d21670b567784db3311cacb73e and gh pr view 534 --json headRefOid printed the same. git status --porcelain was empty before and after. No file in the worktree changed; every destructive experiment ran in a separate git archive extract.

The core change is right. The executor does own the gate now, the gate fails closed, and no second path reaches a confirmed operation without it. This blocks on one false load-bearing claim in the body and on a hole in the guard test that carries the whole safety argument for the removed gate.

What ran

dotnet build Orbit.slnx -v q --nologo succeeded. CSC : warning CS9057 confirms the local SDK skips Orbit.Analyzers, so ORBIT0001..0005 did not run locally. The added lines were grepped by hand instead: every added comment is an XML-doc comment, which NoCommentsAnalyzer.IsAllowed permits, so CI will not fail on one.

Whole suite at this head, dotnet test Orbit.slnx --no-build:

Orbit.Analyzers.Tests        0 failed, 32 passed
Orbit.Domain.Tests           0 failed, 585 passed
Orbit.Application.Tests      0 failed, 3393 passed
Orbit.Infrastructure.Tests   0 failed, 2241 passed

6251 tests, zero failures. The body's numbers are exact.

The fix was broken on purpose and the tests went red. Deleting only the capability skip at WebApplicationExtensions.cs:306-312 in a scratch copy turns 3 of the 6 new tests red, each with Expected outcome.PolicyError to be null, but found "step_up_required". The fourth test and both guards stay green. The body's evidence reproduces exactly.

CI: 23 checks, all pass except auto-merge (skipping). drift passes, so architecture.json and architecture.html match the generator. All three mutation jobs pass. pullfrog-approval is absent, as expected.

The five security questions, answered by driving them

Does the executor own the gate? Yes. Every confirmation-gated capability was enumerated from AgentCatalogService.Capabilities.cs: three StepUp at :829, :873, :1000, and nine FreshConfirmation at :221, :241, :255, :366, :414, :575, :615, :751, :933. :933 is SyncWrite, which declares no mcpTools, so 15 MCP tools are gated, not 16. All 15 call executorBridge.ExecuteAsync. Two use a different operation id from the tool name, delete_notification to delete_notifications and delete_user_fact to delete_user_facts, and both map back to the same gated capability. The second MCP door, execute_agent_operation_v2, forwards its own token at AgentTools.cs:41-49. MCP rate limits run before the skip, so they are unaffected.

Flag OFF, which is production today: the change is NOT inert, and the body does not claim it is. RequireApiKeyCreationStepUp does not gate this code. All 15 gated MCP tools change behaviour on deploy, from a permanent refusal to a completable flow. That is the fix.

Flag ON: AppConfig.Create("RequireApiKeyCreationStepUp", "true", ...) was seeded and manage_api_keys revoke driven through the real middleware, bridge, executor, evaluator, pending store and step-up service. Without step-up it refuses. With a valid confirmation token but no verified step-up it still refuses. After IssueChallengeAsync and VerifyChallengeAsync the same token runs the tool. The flag is orthogonal, and the tree says why: that key is read in exactly one place, CreateApiKeyCommand.cs:40 and :72, while AgentPolicyEvaluator reads only Users and AppFeatureFlags.

Replay, reuse, expiry: none. The confirmed retry run twice with one token returns the result, then Step-up verification required. TryConsumeFreshConfirmation calls MarkConsumed() and IsUsable then rejects the row. A token is bound to (UserId, CapabilityId, OperationFingerprint, ConfirmationTokenHash), so it cannot move to another capability or to different arguments. Expiry is the pending-operation TTL, 10 minutes by default.

Failure mode: fail closed. HasFreshConfirmation needs StepUpSatisfiedAtUtc, which only VerifyChallengeAsync sets. IssueChallengeAsync awaits SendVerificationCodeAsync before saving the challenge row, so an email failure leaves no challenge and the operation stays refused. There is no fail-open path.

P1, and it is why this is REQUEST CHANGES

The body says twelve FreshConfirmation MCP tools are fixed. Two of them are not.

src/Orbit.Api/Mcp/Tools/HabitTools.cs:452-489. BulkLogHabits and BulkSkipHabits declare no confirmationToken parameter, and their shared helper hardcodes it:

        }, confirmationToken: null, cancellationToken);

Re-checked by hand at this head: confirmationToken: null is hardcoded at :132, :166, :197, :236, :254, :341, :357, :486, :502 and :518, while the single-habit destructive tools thread the real parameter at :181, :374, :407, :429 and :447. The bulk helper is :486.

Their capability is HabitsBulkWrite, FreshConfirmation. The token is always null, so HasFreshConfirmation always returns false. Both tools stay permanently refused after this change, proven by driving the tool rather than by reading it.

Failure scenario: a person asks an MCP client to log ten habits. The client calls bulk_log_habits and gets Confirmation required before this action runs. Confirm pending operation <id> via confirm_agent_operation_v2, then retry with the returned confirmation token. It calls confirm_agent_operation_v2, gets a token, and finds no parameter on the tool to carry it. The loop never closes, and this change makes the refusal worse, because it now names a recovery path these two tools cannot use.

The body states "They share the one call site, so they are fixed by the same change." That is false for these two. The body lands in history on squash, and a later reader will cite it as proof that MCP confirmation is complete.

Fix it here, under "Maximum implementation": add the parameter to both methods and thread it through ExecuteBulkHabitOperationAsync, four lines in the file this pull request's own guard test scans. Then correct the sentence.

P2, both worth fixing in this pull request

The guard test does not pin the operation the tool forwards. tests/Orbit.Infrastructure.Tests/Mcp/ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs:48-88. ReachesBridge only asks whether the text executorBridge.ExecuteAsync( appears, never which operation id is passed. The executor gates on catalogService.GetCapability(operation.CapabilityId), not on the tool name, so the two can diverge.

Concrete: NotificationTools.cs:123-137 passes "delete_notifications", which maps to NotificationsDelete (FreshConfirmation). The same file already holds UpdateNotificationsTool = "update_notifications" at :24, which maps to NotificationsWrite with AgentConfirmationRequirement.None (AgentCatalogService.Capabilities.cs:546-555). Change that one argument and three things happen together: the tool name still resolves to a gated capability so the middleware still steps aside, the guard stays green because a bridge call is still present, and the executor gates on None. delete_notification then runs with no confirmation at all, which is exactly what the guard's own doc comment promises to prevent. Assert that each gated tool's operation id resolves to a capability whose ConfirmationRequirement matches.

Two smaller notes on that file: SearchOption.TopDirectoryOnly skips any subfolder added under Mcp/Tools, and BuildMemberBodies keys a member by the first \w+( token in its chunk, so a helper carrying an attribute is keyed by the attribute name and disappears from the lookup. The second produces a false red rather than a false green.

A second route into api-keys.manage carries no gate, ApiKeysController.cs:93 and :31. RevokeApiKey and CreateApiKey go through MediatR with no policy evaluation, no confirmation and no step-up. AgentCapabilityIds.ApiKeysManage lists both in controllerActions, but IsMappedControllerAction has exactly one consumer, a coverage test at AgentCatalogServiceTests.cs:28. Nothing enforces it at request time, so a session JWT revokes a key today with no second factor. Pre-existing at b6a92960, and PR 528 closes part of it. The rest needs its own ticket, not this pull request.

P3

• API-key credentials still cannot complete any confirmation, AgentTools.cs:56, :73, :89. All three confirmation tools throw UnauthorizedAccessException for AgentAuthMethod.ApiKey, so an MCP client on an API key cannot mint a token for any of the 15 gated tools. The behaviour is correct and fails closed. Pre-existing. The body's table says "Broken" and then implies "fixed here" without the qualifier. Add one sentence: this fixes the JWT and OAuth door, the API-key door stays closed by design.
• No audit row when the executor rejects an unknown operation, AgentOperationExecutor.cs:27-29. GetOperation returns null and the method returns UnknownOperation before any TryAuditAsync. Before this change the middleware wrote a legacy AgentAuditLogs row. A trail gap, not a gate gap.
• The body overstates what the new test drives. It says "A test drives that exact sequence and asserts the retry runs." StepUpTool_WithConfirmedStepUpToken_RunsInsteadOfBeingRefused calls _stepUpService and _pendingOperationStore directly and never calls AgentTools.StepUpAgentOperation, VerifyStepUpAgentOperation or ConfirmAgentOperation. It drives the services under the four tools, not the four tools, and nothing covers their ApiKey refusals. Reword it or drive the real methods.
• One assertion cannot fail under a real defect, McpConfirmationGateTests.cs:116. outcome.ToolOutput.Should().NotBeNull(); adds no signal, because the four Should().Contain(...) calls on the next lines already throw on a null string. Delete it. Every other new assertion can go red, and three were watched doing it.

Body claims checked and found true

Three capabilities declare StepUp. The twelve FreshConfirmation tool names are complete and correct; only the "fixed by the same change" verdict is wrong, for two of them. TryConsumeFreshConfirmation calls MarkConsumed() at PendingAgentOperationStore.cs:116. The two gates compute different fingerprints, Compute(toolName, camelCase) at WebApplicationExtensions.cs:531 against Compute(operationId, snake_case) at AgentOperationExecutor.cs:114, so keyId against key_id makes them differ. ConfirmationRequirementOverride does not exist anywhere in the tree, so the ticket was wrong about it. StepUpSatisfied was genuinely dead at b6a92960, appearing once at its own declaration, and all four call sites use the named IsReadOnlyCredential: argument. AgentPlatform:ShadowModeEnabled is absent from both appsettings.json and appsettings.Production.json so it defaults to false, though the Render environment variables could not be read, so an override there is not ruled out. InternalsVisibleTo("Orbit.Infrastructure.Tests") already exists at Orbit.Api.csproj:15 and this diff does not touch it.

Deploy order

Safe to deploy before RequireApiKeyCreationStepUp is flipped, and the reverse order breaks nothing either, because the two are orthogonal. No file in this diff reads that key. It has exactly one reader, CreateApiKeyCommand.cs:40 and :72, gating an EmailChallengeService authorization. The step-up on api-keys.manage is declared at AgentCatalogService.Capabilities.cs:873, is already live, and no config row controls it. Proven rather than assumed: the row was seeded to "true" and the full refuse, step-up, verify, confirm and retry sequence ran identically.

One consequence a human must know before the flip, independent of the order. Once the row is true, MCP manage_api_keys with action=create reaches CreateApiKeyCommand and fails with ApiKeyCreationChallengeRequired, because the EmailChallengeOperation.ApiKeyCreation authorization comes only from ApiKeysController.RequestCreationChallenge and ConfirmCreationChallenge, and no MCP tool exposes either. This pull request does not cause that, it exposes it, because before this change the call never got past the middleware. The failure is closed, so nothing is unsafe, but MCP key creation stops working the moment the row flips. action=revoke is unaffected.

One more note on the flip itself, which belongs to #529 rather than here: EmailChallengeService stores its authorizations in IMemoryCache, so the challenge and the create call must land on the same instance.

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

APPROVE, head f3766d182a41bc9a065aa15e8565624b2852394e.

Pullfrog could not run this review. Codex and Pullfrog share one OpenAI meter, exhausted until 2026-09-22. A separate agent reviewed in the worktree carrying this exact head. The substitution is named so nobody later reads this as having had the usual reviewer.

git rev-parse HEAD and gh pr view 534 --json headRefOid both return f3766d182a41bc9a065aa15e8565624b2852394e. git status --porcelain printed nothing at the start and nothing at the end. All mutation work ran in a git archive f3766d18 extract, whose seven relevant files were checksummed against the worktree before and after every mutation. This is a narrow review of round 2; round 1 was not re-opened.

The P1 is fixed, driven rather than read

A driver in the extract stands up the real graph: AgentCatalogService, PendingAgentOperationStore, AgentPolicyEvaluator, AgentOperationExecutor, the real AgentAuditService over an OrbitDbContext, McpExecutorBridge, the real HabitTools, the real AgentTools, and the real middleware entry HandleMcpToolCallAsync. Only the backing IAiTool and the ownership lookup are stubs.

bulk_log_habits  first call -> Confirmation required before this action runs. Confirm pending
                               operation 0539a566... via confirm_agent_operation_v2, then retry...
                 minted token len=36
                 retry with token -> bulk_log_habits-ran
                 replay same token -> Confirmation required ... (fresh pending operation)

bulk_skip_habits first call -> Confirmation required ...
                 retry with token -> bulk_skip_habits-ran
                 replay same token -> Confirmation required ...

The middleware stepped aside on every call, asserted on the JSON-RPC body carrying no error. The token came from AgentTools.ConfirmAgentOperation, not from the store underneath it. The replay line is a free extra: the token is single use, so a third call mints a fresh pending operation instead of running.

The new guard is real, and every named mutation reddens it

The guard reads the tool sources off disk, so no rebuild is needed between mutations.

mutation	result
none, baseline	0 failed, 5 passed
NotificationTools.cs:130 "delete_notifications" to "update_notifications"	1 failed
same, via the UpdateNotificationsTool const rather than a literal	1 failed
HabitTools.cs:489 back to confirmationToken: null	1 failed
whole HabitTools.cs reverted to 9c7d8d56	1 failed
NotificationTools.cs moved into a Sub/ subfolder and mutated	1 failed
[McpServerTool(Name = "delete_tag")] renamed so the scan loses it	1 failed

The messages are specific rather than generic:

EveryConfirmationGatedMcpTool_ForwardsAnOperationIdCarryingTheSameConfirmationRequirement [FAIL]
NotificationTools.cs: delete_notification (FreshConfirmation) forwards operation '"update_notifications"'
  resolving to notifications.write (None)

EveryConfirmationGatedMcpTool_AcceptsAndForwardsAConfirmationToken [FAIL]
HabitTools.cs: bulk_log_habits declares no confirmationToken parameter
HabitTools.cs: bulk_log_habits forwards confirmationToken 'null' to the executor

Two things worth naming. On the operation-id mutation, EveryConfirmationGatedMcpTool_RoutesThroughTheExecutorBridge, the round 1 guard, stayed green, which reproduces the hole exactly as reported. The subfolder run proves SearchOption.AllDirectories is load-bearing rather than decorative, and the renamed-attribute run proves the scan cannot go quiet and take the other guards with it.

The guard also resolves the same thing the executor gates on, checked rather than assumed: AgentCatalogService.Operations.cs:34 builds every operation as GetCapabilityByChatTool(tool.Name) with operation.Id == tool.Name, so the guard's GetCapabilityByChatTool(forwardedId) and the executor's GetCapability(GetOperation(id).CapabilityId) are the same lookup. It is not asserting against a parallel map.

The real tree still passes

dotnet build Orbit.slnx -v q --nologo exits 0 with no error CS.

0 failed,   32 passed  Orbit.Analyzers.Tests
0 failed,  585 passed  Orbit.Domain.Tests
0 failed, 3393 passed  Orbit.Application.Tests
0 failed, 2245 passed  Orbit.Infrastructure.Tests

6255 total, zero failures, up 4 on Infrastructure from round 1's 2241, matching three new guards plus TheRecoveryTools_RefuseAnApiKeyCredentialSoThatDoorStaysClosed. The body's count is accurate.

CSC : warning CS9057 on five projects means the local SDK skipped Orbit.Analyzers and ORBIT0001..0005 did not run, so every added line of f3766d18 was grepped by hand for a bare // or /* */ outside ///. None.

The ten remaining confirmationToken: null sites are all correct

Every one forwards an operation owned by AgentCapabilityIds.HabitsWrite ("habits.write", AgentCatalogService.Capabilities.cs:172-181), which declares AgentConfirmationRequirement.None. None of the ten is gated, so null is right in all of them: create_habit (:132), update_habit (:166), log_habit (:197), skip_habit (:236), update_checklist (:254), create_sub_habit (:341), duplicate_habit (:357), reorder_habits (:505), move_habit_parent (:521), link_goals_to_habit (:544).

The audit row, driven

A second harness whose catalog carries no IAiTool, so GetOperation("bulk_log_habits") returns null while GetCapabilityByMcpTool still returns habits.bulk.write. That is the real shape: a gated MCP tool naming an operation the catalog does not carry.

missing-operation call -> Error: unsupported_by_policy
audit row capability=unknown source=bulk_log_habits surface=Mcp decision=Denied
          outcome=UnsupportedByPolicy risk=Low error=unsupported_by_policy
          args={"habit_ids":["d5afde16-..."]}

Exactly one row, with arguments redacted the same way every other row is. The hoist of arguments above the operation is null check at AgentOperationExecutor.cs:26-31 is what makes RedactArguments(arguments) safe here, because GetRawText() on an Undefined element would throw. Correct.

The AgentTools tests are not vacuous

McpConfirmationGateTests now holds AgentTools _agentTools and calls the real StepUpAgentOperation, VerifyStepUpAgentOperation and ConfirmAgentOperation, and no longer reaches _stepUpService or _pendingOperationStore directly for that path.

Checked rather than assumed: replacing the throw in AgentTools.ConfirmAgentOperation with { } gives Expected a <System.UnauthorizedAccessException> to be thrown, but no exception was thrown. Restored, green again.

The body is corrected

The false claim is gone. It now reads "Ten of those twelve are fixed by removing the duplicate gate alone ... bulk_log_habits and bulk_skip_habits were not." The API-key qualifier is present: "This fixes the JWT and OAuth door only. The API-key door stays closed by design ... an MCP client on an API key still cannot mint a confirmation token for any of the 15 gated tools."

Load-bearing counts check out: the scan names exactly 15 gated tools, matching 12 FreshConfirmation plus 3 StepUp, and "the other eleven files in src/Orbit.Api/Mcp/Tools/" is correct read as the eleven other files calling executorBridge.ExecuteAsync. The capability ids match AgentContracts.cs:243,245,266,267.

Findings

P2, a proven bypass on a second spelling of null, introduced in round 2

ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs:35 sets NullLiteral = "null", and :146 flags a call only when call.ConfirmationTokenExpression is null or NullLiteral. That is a whitelist of one spelling.

Proven rather than reasoned. Changing HabitTools.cs:489 to }, confirmationToken: default, cancellationToken); gives 0 failed, 5 passed. All five guards stay green. default for string? is null, so both bulk tools would be permanently refused again and the guard would say nothing, which is the exact defect this round exists to prevent. (string?)null and null! take the same path.

Failure scenario: a worker adds a gated MCP tool, writes confirmationToken: default because the parameter is optional, and ships a tool no client can ever call. CI is green.

The fix is one rule instead of a literal list: assert the forwarded expression resolves to the tool's own confirmationToken parameter, rather than asserting it is not the string "null". That closes null, default, null!, (string?)null and "" together.

This does not block the verdict. Nothing in the tree is wrong today and both mutations the round was ordered to redden do redden. It matters because the structural guarantee is the reason this guard was accepted over a hand enumeration, and a one-spelling whitelist is weaker than that guarantee.

P3, the operation-id guard compares the requirement rather than the capability

:117 compares forwarded?.ConfirmationRequirement == tool.Capability.ConfirmationRequirement. Proven: changing TagTools.cs:82 from "delete_tag" to "delete_goal", both FreshConfirmation but different capabilities, leaves the guard green at 0 failed, 5 passed.

A gated tool could forward another gated capability's operation id. Confirmation still fires, but the executor enforces the wrong scope, goals.delete in place of tags.delete. Low practical severity, because the swap also changes which tool runs, so behaviour tests would catch it. Adding forwarded.Id == tool.Capability.Id is free. The commit message does not over-claim: it says "a capability with the same ConfirmationRequirement", which is what the code does.

P3, two of four line citations in the body are off by one

The body cites HabitTools.cs:458, :469, :479, :489. At this head :458 and :489 are right, but the bulk_skip_habits parameter is at :468 and the helper parameter at :478; :469 and :479 are both CancellationToken lines. The body squashes into history, so it is worth one edit.

P3, the guard file's own doc comment is out of date

:13 says "That deferral is safe only while four invariants hold, and each one is pinned by a test here" and lists four. The file now holds five [Fact]s, and the missing one is TheSourceScan_ReachesEveryConfirmationGatedMcpToolInTheCatalog, which is the one the other guards depend on.

Pre-existing and correctly left out

The body's own "Follow-up found but not fixed here": an allowed non-confirmation MCP mutation still writes a second, lower-fidelity legacy audit row from ExecuteAuthorizedMcpToolCallAsync. It predates this ticket.

What could not be verified

Two CI checks were still IN_PROGRESS: Analyze (csharp) and Mutation (infrastructure). Every other check on the head is SUCCESS. Those two were not waited for, so their result is not stated here. There is no pullfrog-approval check on this head, as expected.

Round 1's deploy-order conclusion stands. Round 2 changes no DTO, no contract and no migration, so nothing in it bears on that.

---

This targets protected main, cannot merge before 2026-09-22 while the shared meter is exhausted, and is merged and deployed by hand, so this verdict is advice for that moment rather than a merge trigger.

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

REQUEST CHANGES, head db92bcec545dbde3e8a4b19ae9bbbe86fdcca4c5.

Pullfrog could not run this review. Codex and Pullfrog share one OpenAI meter, exhausted until 2026-09-22. A separate agent reviewed in the worktree carrying this exact head. The substitution is named so nobody later reads this as having had the usual reviewer.

git rev-parse HEAD and gh pr view 534 --json headRefOid both give db92bcec545dbde3e8a4b19ae9bbbe86fdcca4c5. git status --porcelain was empty before and after; every mutation ran in a git archive db92bcec extract, never in the worktree.

Every item round 3 was ordered to fix is fixed. All five named mutations redden, the P3 swap reddens, and the four body and doc items are correct at this head. This is REQUEST CHANGES for a different reason: five further mutations each compile, each restore the exact #599 defect, and each leave all five guards green. Two of them defeat the round-3 P3 fix and the round-3 token rule through one shared root cause.

The ordered mutations, and the P3 re-check

Guard command throughout, in the extract: dotnet test tests/Orbit.Infrastructure.Tests/... --filter "FullyQualifiedName~ConfirmationGatedMcpToolsRouteThroughExecutorTests". Real tree green at 0 failed, 5 passed before and after every one.

#	mutation	result
M1	HabitTools.cs:489 to confirmationToken: default	RED
M2	:489 to confirmationToken: null!	RED
M3	:489 to confirmationToken: (string?)null	RED
M4	:489 to confirmationToken: ""	RED
M5	TagTools.DeleteTag declares the parameter, adds var token = confirmationToken;, forwards token	RED
M6	TagTools.cs:82 to "delete_goal", the round-3 P3	RED

M1 to M4 name both bulk tools and print the exact offending expression. M6 prints delete_tag (tags.delete, FreshConfirmation) forwards operation '"delete_goal"' resolving to goals.delete (FreshConfirmation). None leaves the suite green, so the P1 trigger in the order is not met.

Five that do leave it green

Every one of these also builds: dotnet build src/Orbit.Api/Orbit.Api.csproj -v q --nologo gives 0 errors for each. None is a proof against code that would not compile.

#	mutation	result
A1	:489 to }, ids.Count > 0 ? null : confirmationToken, cancellationToken);	GREEN, bypass
A2	helper's string? confirmationToken made optional; bulk_skip_habits drops the argument	GREEN, bypass
A4	TagTools.DeleteTag adds confirmationToken = null; before the bridge call	GREEN, bypass
A7	DeleteTag keeps a correct bridge call in a dead if (tagId.Length == 0) branch and sends the real work to a same-file helper forwarding null	GREEN, bypass
A8	TagTools.cs:82 to ExecuteAsync(user, tagId.Length > 0 ? "delete_goal" : "delete_tag", ...)	GREEN, bypass

And the four that failed to break it, which is the evidence the rule is general rather than a list of five: confirmationToken ?? null reddens; an all-named bridge call with cancellationToken at index 3 reddens; dropping the parameter and declaring a local string? confirmationToken = null; reddens with delete_tag declares no 'string? confirmationToken' parameter; and two helper hops redden on the routing guard, which fails closed, although a legitimate two-hop refactor reddens the same way.

P2, any : in an argument is read as a named-argument separator

ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs:276:

var namedSeparator = argument.IndexOf(':', StringComparison.Ordinal);
return namedSeparator < 0 ? argument : argument[(namedSeparator + 1)..].Trim();

A conditional expression carries a colon, so ArgumentAt returns only its else-branch. Both the token slot and the operation-id slot go through this method, so one defect defeats two guards.

A1, HabitTools.cs:489: write }, ids.Count > 0 ? null : confirmationToken, cancellationToken);. The guard reads confirmationToken and passes. In production every bulk_log_habits and bulk_skip_habits call that names habit ids forwards null, so HasFreshConfirmation never sees the caller's token and both tools are refused forever. That is #599, returned, with five green guards.

A8, TagTools.cs:82: write ExecuteAsync(user, tagId.Length > 0 ? "delete_goal" : "delete_tag", ...). The guard reads "delete_tag", resolves it to tags.delete, matches, and passes. In production tagId is never empty, so the executor gates goals.delete on every call. That is the same defect the round-3 P3 fix was ordered to close, reached by a different spelling.

Fix: strip the prefix only when it is a real named argument, matching ^\w+\s*:(?!:) before taking the tail and leaving the expression whole otherwise. One line, and it closes both.

P2, an omitted optional argument resolves to the callee's own parameter name

:288:

var parameterIndex = calleeParameters.IndexOf(expression);
if (parameterIndex < 0 || parameterIndex >= callerArguments.Count)
    return expression;

The two branches mean different things and share one result. "The expression is not a parameter" correctly returns the expression. "The caller passed nothing at that index" returns the helper's own parameter name, which then compares equal to ConfirmationTokenParameterName and passes.

A2, HabitTools.cs:470 and :478: give the helper string? confirmationToken = null, CancellationToken cancellationToken = default, then call it as ExecuteBulkHabitOperationAsync(user, "bulk_skip_habits", habitIds, date, filterJson);. bulk_skip_habits keeps its own parameter at :468, so the declaration half passes, and it never passes the value on. The helper forwards its default null. bulk_skip_habits is refused forever.

This is the closest neighbour of the original defect and the most plausible refactor of the five. Fix: split the branches, returning a sentinel such as "<omitted>" when the caller supplied no argument at that index, which can never equal the parameter name.

P3, a direct bridge call hides every helper bridge call in the same tool

:221:

var direct = ReadBridgeCalls(member.Body, constants, [], []);
if (direct.Count > 0)
    return direct;

The helper hop runs only when the tool has no direct call, so a tool with both paths has the helper path unread.

A7, TagTools.cs:81: keep a correct bridge call inside a guard branch and send the real path to DeleteTagCoreAsync, which forwards null. Every real delete_tag call loses its token and the guard reads only the decoy. Fix: return direct.Concat(indirect) rather than short-circuiting.

P3, the rule never sees an assignment to the parameter

:159 compares the forwarded expression's text only. A4, TagTools.cs:81: add confirmationToken = null; above the bridge call. The forwarded text is still confirmationToken, so the guard passes and delete_tag is refused forever. Fix: add an offender when the tool body assigns to confirmationToken, matching \bconfirmationToken\s*=[^=] over member.Body excluding member.ParameterList.

P3, the doc comment and the body claim more than the code checks

:28-33 states "Any other expression in the token slot refuses the tool forever" and "forwards that identifier to the executor, directly or through a helper it calls". A1, A2, A4 and A7 each falsify the first, and A7 falsifies the second, because only one of a tool's two paths is read. The pull request body's guard-5 paragraph carries the same wording and the same overclaim. Introduced in round 3.

Where these come from

Every root cause sits in code round 3 did not touch, in the parser helpers, pre-existing from round 2. What round 3 introduces is the claim that the rule is structural rather than enumerable, and that claim is what these falsify. The mechanisms were not load-bearing before round 3, because the guard did not previously promise to resolve anything.

Confirmations

dotnet build Orbit.slnx -v q --nologo gives 0 errors. Full suite:

0 failed,   32 passed  Orbit.Analyzers.Tests
0 failed,  585 passed  Orbit.Domain.Tests
0 failed, 3393 passed  Orbit.Application.Tests
0 failed, 2245 passed  Orbit.Infrastructure.Tests

6255 tests, zero failures, identical to round 2 on all four projects. The guard does not refuse the real tree.

The four body and doc items, read at this head rather than from the commit message: bulk_log_habits at :458 cited as :458, correct; bulk_skip_habits at :468 cited as :468, corrected from :469; the helper at :478 cited as :478, corrected from :479; the bridge call at :489, correct. The doc comment at :13 now reads "five invariants", the <list> holds five <item> entries, the file holds five [Fact] methods, and item 2 names the source-scan invariant. All correct.

CSC : warning CS9057 on all nine projects means the local SDK skips Orbit.Analyzers, so the Roslyn comment rules did not run. Every line this commit adds was grepped for a bare // or /* */ outside ///. None.

What was not verified

The round 1 and round 2 conclusions, which the order put out of scope. node tools/arch-map.mjs drift and node tools/check-suppression-allowlist.mjs were not re-run, because round 3 changes one test file and adds no test class, so the body's "no drift" was taken at its word.

Whether a conditional expression in a bridge argument would survive a human reviewer is not the question here. The finding is about what the guard proves, not about what is plausible, except A2, which is flagged as the plausible one.

---

Round 3 touches one test file, so nothing in it bears on deploy order. The round 1 conclusion stands. This targets protected main, cannot merge before 2026-09-22, and Thomas merges and deploys it by hand, so this verdict is advice for that moment rather than a merge trigger.

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

REQUEST CHANGES, head 2a4558644c445b452b98b5657f67ca5c172be49f.

Pullfrog could not run this review. Codex and Pullfrog share one OpenAI meter, exhausted until 2026-09-22. A separate agent reviewed in the worktree carrying this exact head. The substitution is named so nobody later reads this as having had the usual reviewer.

git rev-parse HEAD and gh pr view 534 --json headRefOid both return 2a455864. git status --porcelain empty at the start and the end; the worktree was never touched. Every mutation ran in a git archive 2a455864 extract, with every scanned path restored from the committed blob before each one, so exactly one mutation was live at a time, and each was compiled with 0 errors before the guard ran. So every row below is a shape that really compiles.

CSC : warning CS9057 confirms Orbit.Analyzers is skipped locally. All 57 added lines across the five round-4 commits were grepped for a bare // or /* */ outside ///. None.

The five ordered bypasses all redden

#	mutation	guard
A1	}, ids.Count > 0 ? null : confirmationToken, cancellationToken);	RED, both bulk tools named, forwards 'ids.Count > 0 ? null : confirmationToken'
A2	ExecuteAsync(user, tagId.Length > 0 ? "delete_goal" : "delete_tag", ...)	RED, resolving to an unknown capability
A3	helper's token made optional, BulkSkipHabits drops the argument	RED, bulk_skip_habits forwards '<omitted>', and bulk_log_habits correctly stays clean
A4	decoy direct call in a dead branch plus a helper forwarding null	RED, delete_tag forwards 'null'
A5	confirmationToken = null; above the bridge call	RED, assigns to confirmationToken before the executor reads it

All six earlier mutations still redden too: default, null!, (string?)null, "", a var token local, and the delete_tag to delete_goal swap. No fix broke an existing guard.

The real tree is unchanged at 32 / 585 / 3393 / 2245, 6255 passed, zero failures, so the guard does not refuse everything. The three legitimate shapes behave as documented: a direct-only call is green, exactly one same-file helper hop is green, and two hops redden on the routing guard, which fails closed.

Nine of thirteen new attacks failed against the new code

Worth recording, because these are the evidence the fixes are general rather than a list:

• A colon and a comma inside a string literal in the payload argument: green, SkipStringLiteral keeps the indices intact.
• global::System.Guid.Parse(...) in the call: green, the (?!:) lookahead holds. Structurally :: can never produce a false green, because a stripped tail would have to read exactly confirmationToken and X::confirmationToken is not legal C#.
• A local named confirmationTokenBackup = null; alongside a correct named argument: green, \b stops the assignment rule firing on a longer name.
• confirmationToken == null && tagId.Length >= 1 && confirmationToken != tagId: green, so ==, != and >= do not false-fire.
• Three direct bridge calls with the third forwarding null: red, so direct.Concat(indirect) scales past two.
• One helper shared by gated delete_tag and ungated update_tag: green, the hop resolves against the gated tool's own arguments.
• The helper moved to a different file in the same directory: red on the routing guard, and the prose already says "same-file".
• The parameter renamed confirmationTokenValue: red, declares no 'string? confirmationToken' parameter.

P2. The assignment rule misses every compound assignment

ConfirmationGatedMcpToolsRouteThroughExecutorTests.cs:63:

[GeneratedRegex(@"\b" + ConfirmationTokenParameterName + @"\s*=[^=>]")]

\s* cannot cross the ?? or the + of ??= and +=, so the pattern only ever sees a plain =. Proven green by confirmationToken ??= tagId; and by confirmationToken += string.Empty;, both above the bridge call in TagTools.DeleteTag, with all five guards green.

Failure scenario, and it is not sabotage. A later change to HabitTools.BulkDeleteHabits adds confirmationToken ??= await ResolveStoredTokenAsync(user, cancellationToken); so a caller that "forgot" the token still works. The executor then receives a token the MCP caller never sent, HasFreshConfirmation is satisfied from state the caller does not control, the middleware has already stepped aside, and the guard added specifically to catch this is green. ??= on a nullable parameter is an ordinary line of C#.

The doc comment at :32-33 states the opposite in as many words: "it refuses a tool that assigns to the parameter before the call."

One alternation fixes it, checked against every operator in the ordered list:

[GeneratedRegex(@"\b" + ConfirmationTokenParameterName + @"\s*(?:\?\?|\+)?=[^=>]")]

== still fails because [^=>] rejects the second =; !=, >=, <= still fail because those characters break \s* and are not in the alternation; => still fails; a bare ?? with no = still fails. string? admits no other compound operator.

P2. membersByName collapses overloads, so the scan reads a method the tool never calls

:199-201:

.ToDictionary(group => group.Key, group => group.First(), StringComparer.Ordinal);

ReadHelperCalls resolves a callee by bare name, so with two same-named members the scan always analyses whichever is declared first. Proven both ways: an overload declared after the real helper, forwarding null, called by BulkSkipHabits, is green; the identical overload declared before it is red. Declaration order, nothing else.

It breaks in both directions. The mirror case is a false red: a gated tool calls a perfectly correct overload B while an unrelated same-named overload A is declared first and holds no bridge call, so the routing guard reds a tool that is fine and nobody will be able to see why from the message. Adding an overload is an ordinary refactor and the guard's answer to it is arbitrary.

Key the lookup on name plus argument count, or add every ambiguous name to the offender list so ambiguity fails closed rather than silently picking one.

P3, none blocking

• A named argument is stripped without checking its name, :300-308 and :310-326. ArgumentAt strips any ^\w+\s*: prefix and ResolveExpression then indexes positionally, so a helper call written ..., confirmationToken: null, filterJson: confirmationToken, cancellationToken: cancellationToken) is green while C# binds the token into filterJson. This is sabotage-shaped rather than a refactor, nobody reorders two named arguments by accident, and it is structurally confined to the helper hop because the bridge signature has exactly one string? slot.
• A correct call reddens when its trailing named arguments are reordered. cancellationToken: cancellationToken, confirmationToken: confirmationToken is semantically identical and reports forwards 'cancellationToken'. Fails closed, so it is noise, but noise a reviewer has to decode. The same name-aware lookup fixes both.
• SplitTopLevelArguments does not track < and >, :389-423. A new Dictionary<string, string> { ... } payload splits at the comma inside the generic argument list and reports forwards 'string>\n }'. Fails closed, but names no real defect, and that payload shape is plausible.
• Latent, not a finding today: ReadParameterNames at :382 does parameter.Split('=')[0], so an = inside a [Description("...")] on a helper parameter would truncate the name. No Description string under Mcp/Tools/ currently contains = and no helper carries the attribute.
• The doc comment still overclaims on one point, :32-33. The assignment clause is false for ??= and +=, and "carried through exactly one same-file helper that receives it in the matching argument position" reads stronger than the code, which matches by position and never by name. It does not mention overloads at all.

The prose is otherwise a large improvement

Both round-3 overclaims are gone. These were verified against behaviour and are accurate: "The resolver is a text scan over one file, not a compiler", which is the right thing to say first; the seven shapes listed as failing, every one of which was verified red; "Two or more helper hops read as no bridge call at all and fail the routing guard above"; and "Aliasing, reflection and a call reached through an interface are outside what the scan models."

Is this good enough to stop?

No, and a fifth round should be small.

The bar set for this pull request is that every bypass a normal refactor could produce is closed and the prose claims no more than that. P2-a fails both halves at once: confirmationToken ??= ... is an ordinary line, it defeats the rule round 4 was ordered to add, and the doc comment asserts in plain words that it is caught. That is not a concession that source scans are defeatable in principle. It is a rule that does not do the single thing it was written to do.

The overload collapse belongs in the same round, because a scan silently analysing a different method than the one called is a correctness bug in the scan rather than a coverage limit, and it produces both false greens and unexplainable false reds.

The two parser P3s and the named-argument binding do not block. Fold them in only if the name-aware lookup is cheap once the file is open.

What round 4 delivered is real and is not being asked back: five ordered bypasses red, six earlier mutations still red, three legitimate shapes correct, nine of thirteen new attacks failed, and 6255 green. The remaining work is one regex and one lookup.

---

This targets protected main, cannot merge before 2026-09-22, and is merged and deployed by hand, so this verdict is advice for that moment rather than a merge trigger.

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using GPT Sol ｜ 𝕏}

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[thomasluizon]

@pullfrog review

[pullfrog]

✅ No new issues found.

Reviewed changes Reviewed the full confirmation-gate change across the MCP middleware, executor, affected tools, audit flow, and regression tests.

• Confirmation ownership: Confirmation-gated MCP calls now bypass the legacy middleware policy pass and rely on the token-aware IAgentOperationExecutor evaluation.

• Token recovery: The bulk log and skip tools now accept and forward confirmation tokens, while step-up guidance names the complete recovery sequence.

• Audit continuity: Unknown executor operations now emit a denied audit entry before returning the unsupported response.

• Safety coverage: Behavioral tests exercise refusal, step-up recovery, token rejection, and API-key closure, while catalog-backed guards cover every gated MCP tool's route, capability, and token forwarding.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}