Skip to content

Tie an investigation claim's later calls to the named token that made it - #6738

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/bugbash-journal-claims
Oct 25, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/bugbash-journal-claims

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 25, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Review fixes

Round 1 fixes are folded into the sections below: the legacy token can no longer renew, submit, read evidence or act (Level 2 and server actions) under a served named token's claim or name; store-level submit refusals journal as the caller; invalid and unclaimed are reserved token names; renew answers claim_not_token like submit and evidence; the status rosters in the three tool descriptions are updated, with the Darling-only statuses after the <<GUIDE>> marker (fixes the two contract tests CI failed on); tool-entry and real-host tests cover the served policy and names wiring.

What was wrong

With named agent tokens served, a claim's later calls were checked against the claim_id only, not against the token that made the claim:

  1. submit_investigation_report, the claim renewal (claim_investigation_request with a claim_id) and get_investigation_evidence accepted any named token that presented another token's claim_id. Refusals on submit were journaled under the claim's holder (or "unclaimed"), not the caller.
  2. While a deployment moves from the legacy token to named tokens, both are served. A legacy-token caller could pass an agent_id equal to a named agent's name and take the claim, so the journal credited that named agent with the claim and with plan changes made under it.
  3. (Review fixes) The legacy token could still act in a named agent's journal through a claim that agent's name already held (a claim made before the upgrade, or before the operator added that named token): it kept renewing, submitted, read evidence, and ran Level 2 actions with agent_action.agent_id set to that name.

What changed

A claim's later calls must come from the token that claimed it, and the legacy token may not act under a served named agent's name or claim.

  • Mcp/DarlingMcpInvestigationTools.cs
    • Submit, renew and evidence take the calling token's name. A named token that presents a claim_id another token made is refused with status claim_not_token and journaled under the caller's name.
    • The legacy token (no name), with named tokens served, is refused with status agent_id_is_named_token when it claims with an agent_id that is a served name, and when it renews, submits or reads evidence under a claim whose holder is a served name. These rows journal as agent:invalid, never as the named agent.
    • Every submit refusal now journals under the caller (journalAs), including the store-level ones (lease_lost, already_reported, closed); they used to journal as agent:unclaimed.
    • Every one of these refusals answers through InvestigationToolCore.Status(<status>, message), so the renew arm answers claim_not_token like submit and evidence (it answered invalid).
    • The statuses that only Darling answers are listed after the <<GUIDE>> marker of claim_investigation_request (claim_not_token, agent_id_is_named_token), get_investigation_evidence and submit_investigation_report (same two), so the heads shared with Lite stay byte-identical and keep the shared status contract (this was the CI failure on the previous head).
  • DarlingAgentActionGate.cs and ServerActionGate.cs: a legacy-token agent_action whose agent_id is a served name is refused with agent_id_is_named_token and journaled as agent:invalid. The names reach the gates through DarlingAgentActionSeat, DarlingMcpServedPolicy (mute, custom rule, signature merge and threshold override tools) and a new namedAgentNames argument on the plan force, evict, kill, plan guide and recompile tools.
  • DarlingMcpTokenRoles.cs: IsValidName reserves invalid and unclaimed (any case), the two placeholder actors refusal rows use. The config check names the reserved words and does not serve that token.
  • DarlingMcpHostService.cs: the host passes the served agent names (tokens.AgentNames) to the policy and the seat. Operator-role token names are not in the set (their journal actor is operator:<name>).

Named tokens have not shipped: the commit that added them (#6556, 2026-10-22 23:35) is after the v3.11.0 tag (11:26 the same day) and git tag --contains lists no release. So no released config can carry a token named invalid or unclaimed, and the reserved names need no migration note beyond the CHANGELOG line.

A legacy caller that passes an agent_id equal to a served name is refused even when it means to renew a claim it holds under that name. After this change that renew is refused anyway (the claim's holder is a served name), so the two rules agree. The named token takes the claim over by claiming again under its own name (status renewed, reclaimed: true, same claim_id).

Tests

  • InvestigationClaimTokenLiveTests (live PostgreSQL, now 12 tests): the renew test asserts claim_not_token; the legacy-claim test asserts agent_id_is_named_token; new tests for the legacy token refused on renew, submit and evidence under a named token's claim (rows journal as agent:invalid), the named token reclaiming under its own name and then submitting, and a lease_lost refusal journaled as the calling token and not agent:unclaimed.
  • New InvestigationNamedTokenHostLiveTests (live PostgreSQL, 5 tests) through the real host (BuildServer with two agent tokens plus the legacy token, ToolsCallAsync): the second token's submit_investigation_report and get_investigation_evidence on the first token's claim answer claim_not_token with the row naming the second token; the legacy bearer's claim_investigation_request under a served name, and its submit_investigation_report and get_investigation_evidence under a named token's claim, answer agent_id_is_named_token; its create_mute_rule (the seat) and kill_session (the server action gate) as a served name are refused. BuildServer gained an optional owner argument so the server gate can run.
  • DarlingMcpNamedTokenTests: two new bad-entry cases (invalid, Unclaimed) for the reserved names.
  • Revert proof, each on its own build: dropping tokens!.AgentNames at the policy registration fails 3 host tests; dropping served?.NamedAgentNames at the claim, the evidence and the submit tool entries each fails one host test; dropping the served policy from the evidence tool entry fails the second-token evidence test; dropping the seat's names or the kill tool's names fails the Level 2 host test; returning agent instead of journalAs to the store fails the lease_lost test; disabling the legacy-holder check fails the legacy renew/submit/evidence test.
  • Run on the rig (DARLING_TEST_PG): *Investigation*, *Claim*, *AgentAction*, *ServerActionGate*, *TokenRoles*, *NamedToken*, *AgentToken*, *DarlingMcpTokenSet* (before the last merge of origin/dev): Total 670, Failed 0, Skipped 0. Earlier on the same build: InvestigationToolContractTests and McpToolGuideTests (the two classes CI failed) pass. The Census, Ratchet, *Contract*, *ToolGuide* and *McpPage* classes were run on the merged head (after the final merge of origin/dev), together with the two live classes above: Total 759, Failed 0, Skipped 0.
  • Build: 0 warnings, 0 errors. No full suite (CI decides). Lite is untouched.

Considered, not added

  • claim_not_token tells a caller that a GUID it already holds is another token's claim on this request. claim_ids are never listed, so the caller learns nothing it could not already use.
  • Pre-gate argument refusals (a bad argument, a PostgreSQL target, a bad session id) in the plan, mute and custom rule tools still journal a legacy caller under the agent_id it passed (agentTokenName ?? action.AgentId). That row is a refusal only (no claim or change is credited), so it was left; threading the names there is a larger change.

CHANGELOG entry

A claim's later calls must now come from the token that claimed it. With named agent tokens, submitting a report, renewing a claim or reading evidence with another token's claim_id is refused with claim_not_token (renew included) and journaled under the calling token. The legacy token, when named tokens are served, is refused with agent_id_is_named_token when it claims under a named token's name, acts under a claim held by a named token, or passes a named token's name as the agent_id of an agent_action, so the journal no longer credits a named agent with what the legacy token did. A submit refused by the store (lease_lost, already_reported, closed) now journals as the calling token and not agent:unclaimed. The names invalid and unclaimed are reserved: a mcp.tokens entry with either name (any case) is not served and the config check names the reserved words (named tokens are not in a release yet, so no released config is affected).

Submit, renew and evidence now refuse a named token that presents another token's claim_id (claim_not_token), journaled under the caller. A legacy-token claim whose agent_id is a served named agent token's name is refused.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 25, 2026 18:29
&& !string.Equals(holder, tokenName, StringComparison.Ordinal))
{
await store.RecordRefusalAsync(requestId, tokenName, "claim", ClaimNotToken, null, cancellationToken);
return McpHelpers.Refusal("claim_id", ClaimNotTokenMessage(tokenName));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renew arm does not answer claim_not_token — it answers status: "invalid".

McpHelpers.Refusal is Status("invalid", sentence, new { parameter }) (PerformanceMonitor.Common/Mcp/McpHelpers.cs:946-949), so the only place claim_not_token appears on this path is the journal reason (claim:claim_not_token, line 439). Submit (line 681) and evidence (line 555) return InvestigationToolCore.Status(ClaimNotToken, …), so the same violation answers with two different statuses depending on which tool the caller reached.

Three places say otherwise:

  • The PR body: "Submit, renew and evidence … the call is refused with the new status claim_not_token".
  • The CHANGELOG entry, which ships: "submitting a report, renewing a claim or reading evidence with another token's claim_id is refused with claim_not_token".
  • claim_investigation_request's own description (line 362-374) is untouched by this PR: its status roster is still "claimed, renewed, already_claimed, not_open, not_found or unsupported_format" and says nothing about either invalid or claim_not_token. submit_investigation_report's roster was updated in this same diff (line 585), so an agent can discover the refusal there and not here.

The test encodes the actual behaviour (Assert.Equal("invalid", StatusOf(refused)), InvestigationClaimTokenLiveTests.cs:162), so this is the body and the changelog overstating the code, not the test being wrong.

Either make the three arms uniform:

Suggested change
return McpHelpers.Refusal("claim_id", ClaimNotTokenMessage(tokenName));
return InvestigationToolCore.Status(ClaimNotToken, ClaimNotTokenMessage(tokenName));

(which is what the changelog already promises, and what an agent keying retry logic off status needs), or keep Refusal for symmetry with the sibling AgentIdNotToken guard above and instead correct the body/changelog and add the refusal to the claim tool's description. What should not ship is the current three-way disagreement.

"after a later evidence_ready. sections (optional) names the sections to return; omit it for all. A request that " +
"another agent holds a live claim on is refused (already_claimed) unless you pass its claim_id; an unclaimed or " +
"finished request is served to any caller.")]
"finished request is served to any caller. A named agent token that passes another token's claim_id is refused (claim_not_token).")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guide sentence lands here, but get_investigation_evidence's status roster three lines up (line 515) is unchanged: "status ok, empty, not_found or already_claimed". EvidenceAsync now returns a fourth, claim_not_token (line 555), and the roster is the sentence an agent reads to know what the answers can be.

The same hunk updated submit's roster for exactly this (line 585: "already_reported, claim_not_token or not_found"), so the omission here looks like a miss rather than a choice.

Suggested, on line 515: … status ok, empty, not_found, already_claimed or claim_not_token. <<GUIDE>> .

@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Non-blocking: nothing reaches the four new wirings

Every one of the nine new facts calls the internal helper and hands it the identity by hand — tokenName: Second, namedAgentNames: named. None goes through a token. So the four lines that actually connect a bearer token to the new rule are not exercised by anything:

Line What it does Reached by a test?
DarlingMcpInvestigationTools.cs:385 served?.NamedAgentNames → ClaimAsync no
DarlingMcpInvestigationTools.cs:533 served?.Policy.AgentTokenName → EvidenceAsync no
DarlingMcpInvestigationTools.cs:627 served?.Policy.AgentTokenName → SubmitAsync no
DarlingMcpHostService.cs:1337 tokens!.AgentNames → DarlingMcpServedPolicy no

Delete any one of those four arguments and all nine facts still pass green, because each fact supplies the value the dropped argument was meant to carry. That is the exact failure mode #6593 round 3 already filed against this feature, and the suite already has the shape that catches it — AgentTokenIdentityGateLiveTests.AKillCallThroughTheHost_WithANamedAgentToken_JournalsTheTokensName (AgentTokenIdentityGateLiveTests.cs:232), whose own doc comment says "It fails when the wrapper drops served?.Policy.AgentTokenName". It builds a real server with DarlingMcpWritePolicyTests.BuildServer(mcp, …, tokens: tokens) and calls ToolsCallAsync(server, tool, arguments, agentBearer).

One fact in that shape would cover all four rows: build a host serving triage-bot and other-bot as agent tokens plus a legacy token, tools/call submit_investigation_report with other-bot's bearer and triage-bot's claim_id, and assert claim_not_token in the body and 0|1|0 on the journal — plus the legacy bearer claiming as agent_id: triage-bot for the AgentNames row.

Not blocking on its own; raising it because the feature it guards is an identity boundary and the repo has already paid for this shape once.

Things I checked that are fine

  • tokens! at DarlingMcpHostService.cs:1337 is safe: perRequest is tokens is { HasNamed: true } (line 1263), so the null-forgiving operator is guarded by the branch it sits in. The non-named arm passes no names, which is right — there are none.
  • AgentNames excluding operator-role tokens is correct, and the body's reason checks out: an operator's journal actor is operator:<name> (DarlingMcpWritePolicy.cs:289) and AgentTokenName is null for a non-investigations policy (DarlingMcpWritePolicy.cs:279), so an operator name never appears as agent:.
  • "Considered, not added" — a legacy token presenting a named agent's claim_id holds up under reading. claim_id is projected by neither ListSql (line 408-410) nor ReportsRequestsSelect (line 501-511) nor ReportsSql (line 541), and SweepLapsedSql puts it in once_key, which ReportsJournalSql does not select (line 559). So no MCP read hands one caller another's claim_id, and with the new guard at line 411 a claim under a named name can no longer come from the legacy token. The residual needs a leaked claim_id; scoping it out is reasonable and the body says so.
  • The #1776 hygiene guard passes: the own-store marker sits 9 lines above the class declaration, inside LivePostgresCollectionHygieneTests.HeaderLookbackLines (25).
  • Harness parity: InvestigationClaimTokenLiveTests.RunAsync mirrors InvestigationToolsLiveTests.RunAsync (line 631-646), including NpgsqlDataSource.Create(scratch.ConnectionString) with no SearchPath — the investigation SQL is schema-qualified, and that existing class composes real briefs the same way.
  • Parity rule: Lite has no named-token concept at all (no hit for AgentTokenName, McpTokenEntry, mcp.tokens anywhere under Lite/), so there is no counterpart to keep in step. "Lite is untouched" is the right answer here, not a parity gap.
  • ReportContractRequired_IsExactlyTheSubmitToolsRequiredParameters (InvestigationBriefComposerTests.cs:292) still holds: it filters on !p.IsOptional && has DescriptionAttribute, and served is optional with no description.
  • Guide budgets: both touched descriptions grow the head by well under ConvertedHeadCap (1,000) and the tail by well under McpToolGuide.MaxGuideCharacters (24,000).

claude[bot]
claude Bot previously requested changes Oct 25, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — Before this change submit, the renewal and evidence were checked against the claim_id only — the diff's removed lines: submit read the holder only to name the journal actor, renew went straight to RenewAsync, evidence tested ClaimHolderAsync(...) is not null.
  • VERIFIED — A legacy caller could claim under a named agent's name — pre-change ClaimAsync had only the tokenName is not null arm (DarlingMcpInvestigationTools.cs:403); a legacy agent_id met IsValidAgentId alone (:420).
  • VERIFIED — Submit, renew and evidence now take served?.Policy.AgentTokenName — DarlingMcpInvestigationTools.cs:384, :533, :627.
  • VERIFIED — The refusal journals under the caller, not the holder — tokenName is the actor at :439, :554, :680, and journalAs = tokenName ?? agent (:676) carries it to the three later submit refusals (:688, :701, :719); pinned by ANamedTokens_OtherSubmitRefusals_AreJournaledAsItself_NotAsUnclaimed and TheHoldersOwnRefusals_StillJournalAsTheHolder.
  • VERIFIED — Evidence checks the holder only while the request is claimed, as before — the guard is inside if (request.Value.Row.State == "claimed") (:548).
  • VERIFIED — A legacy claim under a served named name is refused agent_id_is_named_token under the invalid actor — :411-417, InvestigationRules.cs:40, InvestigationRequestStore.cs:1102; pinned by ALegacyClaim_UnderAServedNamedTokensName_IsRefused_AndOtherNamesClaim asserting 0|0|1.
  • VERIFIED — Legacy single-token mode behaves as before — no named tokens means perRequest false (DarlingMcpHostService.cs:1263), null AgentTokenName (DarlingMcpWritePolicy.cs:279) and an empty NamedAgentNames (DarlingMcpThresholdOverrideTools.cs:36), so all three guards are inert; pinned by ALegacyClaim_WithNoNamedTokensServed_AcceptsAnyValidName.
  • VERIFIED — Operator names are excluded and their actor is operator:<name>, never agent: — DarlingMcpTokenRoles.cs:61, DarlingMcpWritePolicy.cs:289, :279. The tokens! at DarlingMcpHostService.cs:1337 is guarded by its perRequest branch.
  • VERIFIED — Lite is untouched and owes no counterpart — only Darling/ paths in the diff, and AgentTokenName, McpTokenEntry and mcp.tokens have no occurrence under Lite/.
  • UNVERIFIED — "New class on the rig: Total 9, Failed 0" and the revert proof (5 of 9 fail with the checks disabled) — the nine [Fact]s exist, but all self-skip on Assert.SkipWhen without DARLING_TEST_PG (InvestigationClaimTokenLiveTests.cs:55); a live store and two builds were needed.
  • UNVERIFIED — The *Investigation* / *McpTool* / Stage=Guard sweeps at Total 1422 and Total 450, Failed 0 — needed a run.
  • UNVERIFIED — "Build: 0 warnings, 0 errors" — needed a Windows net10.0 build; the new types and usings do resolve on a read.
  • REFUTED — "Submit, renew and evidence … refused with the new status claim_not_token", and the CHANGELOG's "submitting a report, renewing a claim or reading evidence … is refused with claim_not_token" — renew returns McpHelpers.Refusal("claim_id", …) (:440), and Refusal is Status("invalid", …) (McpHelpers.cs:946). claim_not_token reaches that caller only as the journal reason. The PR's own test asserts "invalid" (InvestigationClaimTokenLiveTests.cs:162), and claim_investigation_request's roster (:362) lists neither status while submit's was updated in the same diff (:585). Commented inline.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs:677 — the submit guard: get the holder is not null conjunct wrong and either a named token reports on a stranger's claim, or a holder of a lapsed claim is refused instead of answering lease_lost — covered by ASecondNamedToken_CannotSubmitOnTheFirstTokensClaim_AndTheRefusalJournalsTheSecond and ANamedTokens_OtherSubmitRefusals_AreJournaledAsItself_NotAsUnclaimed.
  2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs:436 — the renew guard, now ahead of RenewAsync: a dropped tokenName is not null breaks the legacy renewal — covered by ASecondNamedToken_CannotRenewTheFirstTokensClaim_ButTheHolderCan and OnTheLegacyToken_ARenewalByClaimIdStillWorks.
  3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs:1337 — tokens!.AgentNames, the only thing making the legacy guard live in production — UNCOVERED. Drop the argument and all nine facts still pass, because each supplies namedAgentNames by hand. Same gap for DarlingMcpInvestigationTools.cs:385, :533, :627; the suite already has the catching shape at AgentTokenIdentityGateLiveTests.cs:232. Commented at top level.
  4. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs:411 — the legacy guard is an else if ahead of the renewal block, so a legacy caller passing agent_id equal to a served named name is refused even when it only meant to renew a claim it already holds under that name. Claim path covered by ALegacyClaim_UnderAServedNamedTokensName_IsRefused_AndOtherNamesClaim; the renew-under-that-name path UNCOVERED. Noted, not raised as a defect — the claim can only lapse.

Traps near this diff

  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit — the census in McpPageContractTests compares the two SKUs' payload keys by tool name for exactly this (#3594)." — does not apply — named tokens are Darling-only: Lite/Mcp/McpInvestigationTools.cs:113 and :252 take no tokenName.
  • "Ruling (#5833): tests are load-bearing. … never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage)." — does not apply — all nine facts drive the store and assert answers and journal rows (InvestigationClaimTokenLiveTests.cs:109, :219).
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — ClaimNotToken and AgentIdIsNamedToken are single internal const declarations (DarlingMcpInvestigationTools.cs:880, :886) that the messages and the tests both read.

Divergence (blind read vs PR body)

Two places, both on the renew arm.

  1. The body and the CHANGELOG name claim_not_token as the status for all three arms. :440 → McpHelpers.cs:946 says renew answers invalid, with claim_not_token only in the journal. The author's own test agrees with the code, so the account is ahead of the code rather than the code being wrong — but the CHANGELOG is what ships, and claim_investigation_request's description documents neither status, so an agent cannot learn this refusal from the surface it reads.

  2. The Tests section reads as end-to-end coverage. Every fact calls the internal helper with the identity passed as an argument, so nothing exercises served?.Policy.AgentTokenName or served?.NamedAgentNames — the four lines where a bearer token becomes an identity. A silence rather than a contradiction, but it is the difference between "the rule is tested" and "the rule is wired".

The rest matched. "Considered, not added" holds up: claim_id is projected by neither ListSql (InvestigationRequestStore.cs:408) nor ReportsRequestsSelect (:501) nor ReportsSql (:541), and SweepLapsedSql (:568) puts it in once_key, which ReportsJournalSql (:559) does not select.

Reviewer checklist

  • Run the new class on the rig with DARLING_TEST_PG set and confirm 9/9 — it self-skips without it, so a green CI leg proves nothing about these facts.
  • Re-run the revert proof after deciding the status question on line 440: if renew moves to Status(ClaimNotToken, …), InvestigationClaimTokenLiveTests.cs:162 changes with it.
  • On a real host with two agent tokens plus a legacy token in mcp.tokens: tools/call submit_investigation_report with the second bearer and the first's claim_id, and claim_investigation_request on the legacy bearer with agent_id = the first token's name. Read the refusals and the journal actor from the store — the only thing that exercises DarlingMcpHostService.cs:1337 and the three served?. call sites.
  • Confirm the build is clean on Windows/net10.0.

…r claim; renew answers claim_not_token; reserved token names; host tests

A legacy-token caller is refused (agent_id_is_named_token) when it renews, submits or reads evidence under a claim a served named token holds, and when an agent_action carries a served name as its agent_id. Store-level submit refusals journal as the caller. The names invalid and unclaimed are reserved for token names. The Darling-only statuses are listed after the GUIDE marker so the shared heads stay identical to Lite.
@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Follow-up review of 8ac1d3ea0..f484b4edb (the real new work is commit 1ad885cd2; the two merges on either side of it just bring in origin/dev and don't touch any file this commit touches).

The code is correct and well covered. I traced the new "a served named token's identity can't be hijacked" check through both gates:

  • ServerActionGate.AuthorizeAsync and DarlingAgentActionGate.RunAsync both gained the same agentTokenName is null && namedAgentNames.Contains(action.AgentId) block, refusing a legacy-token agent_action whose agent_id is a served named token's name.
  • DarlingMcpInvestigationTools.ClaimTokenRefusal now unifies the "claim/renew/submit/evidence must come from the claim's actual holder" check for both named tokens (claim_not_token) and the legacy token hitting a named token's claim (agent_id_is_named_token), and is wired into all three call sites (ClaimAsync, EvidenceAsync, SubmitAsync).
  • SubmitAsync's journalAs = tokenName ?? agent fix (a stale/lease-lost submit now journals under the calling token, not the old holder or "unclaimed") is exercised directly by AStoreLevelSubmitRefusal_IsJournaledAsTheCaller_NotAsUnclaimed.
  • namedAgentNames is threaded through every ServerActionGate.AuthorizeAsync caller I could find (DarlingMcpPlanForceTools.cs, .PlanGuide.cs, .RecompileModule.cs — all 7 call sites) and every DarlingAgentActionGate.RunAsync caller (DarlingMcpAgentActions.cs's shared private RunAsync, DarlingMcpSignatureMergeTools.cs, DarlingMcpThresholdOverrideTools.cs). The one gap, ServerActionGate.CheckAsync, which still defaults namedAgentNames to null, has zero callers anywhere in the tree — dead code, not a live hole.
  • The docstring restructuring (moving Darling-only statuses after <<GUIDE>>) actually fixes a pre-existing parity break: before this commit, submit_investigation_report's pre-<<GUIDE>> head literally listed claim_not_token, which Lite's identical tool does not have; after this commit the pre-marker text is byte-identical to Lite/Mcp/McpInvestigationTools.cs's again.

One thing worth flagging: the PR body is now stale against this commit. Its "Considered, not added" section says, of the current PR:

Journaling those legacy pre-gate argument refusals under invalid: it needs the served names threaded through about 35 agentTokenName call sites in the plan tools... Not added.
Refusing a legacy-token call that presents a named agent's claim_id: ... Not added.

Commit 1ad885cd2 (this follow-up range) adds exactly both of these: the agentTokenName/namedAgentNames threading through the plan-tool call sites (journaling under invalid via InvestigationRules.InvalidAgentId), and the legacy-token-vs-named-claim_id refusal via ClaimTokenRefusal's tokenName is null branch. The commit message itself describes this accurately ("A legacy-token caller is refused (agent_id_is_named_token) when it renews, submits or reads evidence under a claim a served named token holds, and when an agent_action carries a served name as its agent_id"), but the top-level PR description was never updated to match — a reader of the PR body alone would believe these paths are still open. Worth editing the body before merge so it doesn't misdescribe the final state.

claude[bot]
claude Bot previously requested changes Oct 25, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — "Submit, renew and evidence take the calling token's name... refused with the new status claim_not_token" — DarlingMcpInvestigationTools.ClaimTokenRefusal (new in this commit) is wired into ClaimAsync, EvidenceAsync and SubmitAsync; InvestigationNamedTokenHostLiveTests.TheSecondToken_SubmittingOnTheFirstTokensClaim_IsRefusedAndJournaledAsTheSecond exercises it end to end.
  • VERIFIED — "Submit refusals now journal under tokenName ?? holder ?? "unclaimed"" — DarlingMcpInvestigationTools.cs SubmitAsync's journalAs = tokenName ?? agent (where agent = holder ?? UnclaimedAgent), covered by AStoreLevelSubmitRefusal_IsJournaledAsTheCaller_NotAsUnclaimed.
  • VERIFIED (commit message, not PR body) — "A legacy-token caller is refused (agent_id_is_named_token) when it renews, submits or reads evidence under a claim a served named token holds, and when an agent_action carries a served name as its agent_id" — ClaimTokenRefusal's tokenName is null branch, plus the matching new block in both DarlingAgentActionGate.RunAsync and ServerActionGate.AuthorizeAsync; covered by InvestigationClaimTokenLiveTests.TheLegacyToken_CannotRenewSubmitOrReadEvidence_OnAServedNamedTokensClaim_... and InvestigationNamedTokenHostLiveTests.TheLegacyTokens_Level2Actions_AsAServedTokensName_AreRefused_OnBothGates.
  • UNVERIFIED — the PR body's cited rig run counts ("New class on the rig: Total 9, Failed 0...", "Total 1422, Failed 0...", "Build: 0 warnings, 0 errors") — these describe live test-rig/build output from before this commit; a read-only tree has no way to re-run them.
  • REFUTED — PR body, "Considered, not added: Journaling those legacy pre-gate argument refusals under invalid... Not added" and "Refusing a legacy-token call that presents a named agent's claim_id... Not added" — this follow-up commit (1ad885cd2) adds both: namedAgentNames is threaded through every ServerActionGate.AuthorizeAsync/DarlingAgentActionGate.RunAsync call site and journals the new refusal under InvestigationRules.InvalidAgentId (DarlingAgentActionGate.cs, ServerActionGate.cs), and ClaimTokenRefusal's tokenName is null branch refuses a legacy call on a named token's held claim_id (DarlingMcpInvestigationTools.cs). The commit message correctly describes this; the top-level PR body was not updated to match.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs (new ClaimTokenRefusal, ~line 895) — the single chokepoint deciding whether a claim/submit/evidence call may act under a claim_id it doesn't own — covered by InvestigationNamedTokenHostLiveTests.TheSecondToken_SubmittingOnTheFirstTokensClaim_IsRefusedAndJournaledAsTheSecond and TheLegacyToken_UsingAServedTokensClaim_IsRefused_ForSubmitAndEvidence.
  2. Darling/PerformanceMonitor.Darling.Service/Mcp/ServerActionGate.cs (new block, ~line 88) — the legacy-token-vs-named-token-name collision check for Level 3 server actions (kill/force/evict/recompile/plan-guide) — covered by InvestigationNamedTokenHostLiveTests.TheLegacyTokens_Level2Actions_AsAServedTokensName_AreRefused_OnBothGates (its kill_session arm).
  3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs (SubmitAsync, journalAs = tokenName ?? agent) — changes who a store-level submit refusal (e.g. lease_lost) is journaled under — covered by AStoreLevelSubmitRefusal_IsJournaledAsTheCaller_NotAsUnclaimed.

Traps near this diff

  • "Lite and Darling ship in parity... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: named agent tokens (#6542) are a Darling-only MCP feature (no NamedAgentNames/AgentIdIsNamedToken anywhere outside Darling/), and the docstring restructuring in this commit actually restores byte-identical shared heads with Lite/Mcp/McpInvestigationTools.cs (compared claim_investigation_request and submit_investigation_report's text up to <<GUIDE>>) rather than breaking parity.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted" — does not apply: the near-identical named-token-collision blocks added to DarlingAgentActionGate.RunAsync and ServerActionGate.AuthorizeAsync are two gates with different outcome types (AgentActionGateOutcome vs ServerActionDecision), following the pre-existing duplication pattern already in both files (e.g. the AgentIdNotToken check), not a new pinned-constant pair.

Divergence (blind read vs PR body)

The PR body's "Considered, not added" section describes two mitigations as deliberately deferred; this follow-up commit implements both of them (see the REFUTED claims above). The commit message is accurate about this; the PR body is not and should be updated before merge so it doesn't misdescribe the PR's final state.

Reviewer checklist

  • Update the PR body's "Considered, not added" section (or add a new section) to reflect that commit 1ad885cd2 implemented both previously-deferred mitigations, so the merged description matches the shipped behavior.
  • Re-run the live-Postgres Darling.Tests suite (DARLING_TEST_PG set) to confirm InvestigationNamedTokenHostLiveTests and the new InvestigationClaimTokenLiveTests facts pass on CI's rig — a read-only tree can't execute them.

review-pass: follow-up head=f484b4edbfc67089e5ccf470a75bd14d73b39c8e

claude[bot]
claude Bot previously requested changes Oct 25, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — "Submit, renew and evidence take the calling token's name... refused with the new status claim_not_token" — DarlingMcpInvestigationTools.ClaimTokenRefusal (new in this commit) is wired into ClaimAsync, EvidenceAsync and SubmitAsync; InvestigationNamedTokenHostLiveTests.TheSecondToken_SubmittingOnTheFirstTokensClaim_IsRefusedAndJournaledAsTheSecond exercises it end to end.
  • VERIFIED — "Submit refusals now journal under tokenName ?? holder ?? "unclaimed"" — DarlingMcpInvestigationTools.cs SubmitAsync's journalAs = tokenName ?? agent (where agent = holder ?? UnclaimedAgent), covered by AStoreLevelSubmitRefusal_IsJournaledAsTheCaller_NotAsUnclaimed.
  • VERIFIED (commit message, not PR body) — "A legacy-token caller is refused (agent_id_is_named_token) when it renews, submits or reads evidence under a claim a served named token holds, and when an agent_action carries a served name as its agent_id" — ClaimTokenRefusal's tokenName is null branch, plus the matching new block in both DarlingAgentActionGate.RunAsync and ServerActionGate.AuthorizeAsync; covered by InvestigationClaimTokenLiveTests.TheLegacyToken_CannotRenewSubmitOrReadEvidence_OnAServedNamedTokensClaim_... and InvestigationNamedTokenHostLiveTests.TheLegacyTokens_Level2Actions_AsAServedTokensName_AreRefused_OnBothGates.
  • UNVERIFIED — the PR body's cited rig run counts ("New class on the rig: Total 9, Failed 0...", "Total 1422, Failed 0...", "Build: 0 warnings, 0 errors") — these describe live test-rig/build output from before this commit; a read-only tree has no way to re-run them.
  • REFUTED — PR body, "Considered, not added: Journaling those legacy pre-gate argument refusals under invalid... Not added" and "Refusing a legacy-token call that presents a named agent's claim_id... Not added" — this follow-up commit (1ad885cd2) adds both: namedAgentNames is threaded through every ServerActionGate.AuthorizeAsync/DarlingAgentActionGate.RunAsync call site and journals the new refusal under InvestigationRules.InvalidAgentId (DarlingAgentActionGate.cs, ServerActionGate.cs), and ClaimTokenRefusal's tokenName is null branch refuses a legacy call on a named token's held claim_id (DarlingMcpInvestigationTools.cs). The commit message correctly describes this; the top-level PR body was not updated to match.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs (new ClaimTokenRefusal, ~line 895) — the single chokepoint deciding whether a claim/submit/evidence call may act under a claim_id it doesn't own — covered by InvestigationNamedTokenHostLiveTests.TheSecondToken_SubmittingOnTheFirstTokensClaim_IsRefusedAndJournaledAsTheSecond and TheLegacyToken_UsingAServedTokensClaim_IsRefused_ForSubmitAndEvidence.
  2. Darling/PerformanceMonitor.Darling.Service/Mcp/ServerActionGate.cs (new block, ~line 88) — the legacy-token-vs-named-token-name collision check for Level 3 server actions (kill/force/evict/recompile/plan-guide) — covered by InvestigationNamedTokenHostLiveTests.TheLegacyTokens_Level2Actions_AsAServedTokensName_AreRefused_OnBothGates (its kill_session arm).
  3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs (SubmitAsync, journalAs = tokenName ?? agent) — changes who a store-level submit refusal (e.g. lease_lost) is journaled under — covered by AStoreLevelSubmitRefusal_IsJournaledAsTheCaller_NotAsUnclaimed.

Traps near this diff

  • "Lite and Darling ship in parity... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: named agent tokens (#6542) are a Darling-only MCP feature (no NamedAgentNames/AgentIdIsNamedToken anywhere outside Darling/), and the docstring restructuring in this commit actually restores byte-identical shared heads with Lite/Mcp/McpInvestigationTools.cs (compared claim_investigation_request and submit_investigation_report's text up to <<GUIDE>>) rather than breaking parity.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted" — does not apply: the near-identical named-token-collision blocks added to DarlingAgentActionGate.RunAsync and ServerActionGate.AuthorizeAsync are two gates with different outcome types (AgentActionGateOutcome vs ServerActionDecision), following the pre-existing duplication pattern already in both files (e.g. the AgentIdNotToken check), not a new pinned-constant pair.

Divergence (blind read vs PR body)

The PR body's "Considered, not added" section describes two mitigations as deliberately deferred; this follow-up commit implements both of them (see the REFUTED claims above). The commit message is accurate about this; the PR body is not and should be updated before merge so it doesn't misdescribe the PR's final state.

Reviewer checklist

  • Update the PR body's "Considered, not added" section (or add a new section) to reflect that commit 1ad885cd2 implemented both previously-deferred mitigations, so the merged description matches the shipped behavior.
  • Re-run the live-Postgres Darling.Tests suite (DARLING_TEST_PG set) to confirm InvestigationNamedTokenHostLiveTests and the new InvestigationClaimTokenLiveTests facts pass on CI's rig — a read-only tree can't execute them.

@erikdarlingdata
erikdarlingdata dismissed stale reviews from claude[bot], claude[bot], and claude[bot] October 25, 2026 22:42

Claude posting for Erik Darling. Superseded: the round-1 items were fixed in f484b4e.

@erikdarlingdata
erikdarlingdata merged commit f74de02 into dev Oct 25, 2026
42 of 43 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/bugbash-journal-claims branch October 25, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant