fix: read a harness tool call as tooling, not as an undeclared action - #419
Draft
rita-aga wants to merge 3 commits into
Draft
fix: read a harness tool call as tooling, not as an undeclared action#419rita-aga wants to merge 3 commits into
rita-aga wants to merge 3 commits into
Conversation
A conformance check of a clean Claude Code run returned `fail` with one `unknown_action` violation per harness tool call — 81 in the run that found it — each reporting that the agent "decided on `Bash`, which the kernel never recorded a row for". The rows were clean, the evidence complete, the spec pinned; the verdict was noise. `is_action_name` accepted any bare `[A-Za-z0-9_.-]+` token, so `Bash`, `Read`, `Edit`, `Agent` and `mcp__linear__*` all classified as claims about governed actions. The existing `HarnessTool` escape only fired for the Temper MCP envelope, whose `choice.action` carries a colon and a line of Python. A decision is now judged as a governed-action claim only when something positively identifies it as one: 1. the names under `choice.arguments.trajectory_actions` — that key means "the governed actions this code called", so they are judged whether or not the kernel recognises them, and an envelope naming an undeclared action is still the fault to report; 2. a `choice.action` the new `ActionVocabulary` contains: declared by the actor spec, defined by the platform (`KERNEL_PLATFORM_ACTIONS`), or dispatched by the kernel somewhere in this session, on any entity. Anything else is counted in the new `stats.ots_decisions_skipped_as_unrecognized_name` and never reported. The existing `ots_decisions_skipped_as_harness_tool` keeps its meaning exactly — an envelope whose name is not even shaped like an action name — so the two skip reasons stay distinguishable in a report. Rows are untouched: an undeclared action, an illegal source state, or a move after a terminal state is still caught from the kernel's own record, which is where the governance check lives. The change can only remove decision-side violations, never add one. What it cannot do is separate a bare `Bash` from a bare `Frobnicate` — same token, same producer path, nothing else in the record to go on. So this is the conservative half, and the module doc says so: what is given up is the record of an attempt that never reached the platform, not any action that occurred. Narrowing that gap means adding positive identification (reading a governed target out of the arguments, or admitting the tenant's other registered specs), which is additive to `ActionVocabulary` and can only move decisions from skipped to judged. Only kernel-chosen names widen the vocabulary. Rows marked `spec_governed = false` — `POST /api/audit` and the unmet-trajectory endpoint, both caller-named — and capture-loss markers are excluded, or one audit record called `Bash` would turn every `Bash` decision in that session into a violation in somebody else's report. Verified: 783 tests pass (`cargo test -p temper-server --lib --features observe`), 60 of them conformance. Mutation-tested — restoring the pre-fix rule fails 7 of the new tests, including the Bash/Read/Edit/Agent/ToolSearch regression fixture. Live against a local kernel with the katagami-curation app: a clean four-row CuratorAgent run plus a 13-decision Claude Code trajectory now returns `verdict: pass, evidence_complete: true, spec_resolution: pinned` with `ots_decisions_skipped_as_unrecognized_name: 12` and no violations, while a second run proves a `Delete` decision the spec never declares is still reported as `forbidden_action`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SF1Xhjcg7zG38WjmfC239C
Review of the previous commit found a caller-writable path into `ActionVocabulary` that the `spec_governed` exclusion did not cover. `names_a_kernel_dispatch` accepted any row not marked `spec_governed = false`. `authz::helpers::record_authz_denial` writes its row with `spec_governed` unset, and `POST /api/authorize` reaches it with a caller-supplied action name, resource type and `X-Temper-Ctx-SessionId`. `None != Some(false)`, so the row passed the filter and its caller-chosen name entered the vocabulary. The attack that opens, which flips a clean run to `fail`: 1. call `POST /api/authorize` with somebody else's session id and action `Bash`; the denial is recorded as a row on that session, entity type `Widget`, `spec_governed` unset; 2. their conformance check reads the session's rows, skips that one as another entity's — no violation, no visible trace — but takes `Bash` into the vocabulary; 3. every `Bash` decision in their trajectory is now placeable, so it is judged, so it reports as `unknown_action` against a spec they followed. A verdict anyone can flip is not a verdict, and it is the exact regression this branch exists to remove, reintroduced against a targeted session. The filter is now an allowlist of one source: `TrajectorySource::Entity`, the entity dispatch path, which is the only writer whose action name the kernel chose. `Authz` rows are an authorization question a caller asked, not an action the kernel dispatched. `Platform` rows are kernel-named but every one of those names is already in `KERNEL_PLATFORM_ACTIONS`, so admitting them adds nothing and is what would let a capture-loss marker's `CaptureLost` in. The `spec_governed = false` test stays on top, since nothing stops a caller-named audit row claiming `Entity` as its source. A row that cannot be placed simply does not widen the vocabulary — the safe direction, since it can only cause a decision to be counted rather than judged. Also from review: - The `ActionVocabulary` doc said "every action name the kernel dispatched in this session", which described the intent rather than the filter that had been written. It now points at `names_a_kernel_dispatch` and says it is narrower than "appears on a row". - The platform-verb collision is documented instead of left to be discovered: a harness tool called `Delete`, `Put` or `Patch` is placeable, so it is judged, and against an actor that does not declare it that is a `forbidden_action`. Nothing in the record separates the two readings; the tie-break deliberately runs toward reporting for the platform's own write verbs and toward counting everywhere else. `a_harness_tool_named_after_a_platform_verb_is_reported` pins it so it stays a decision. Three new tests, all mutation-checked: restoring the flag-only filter fails the authz-denial and platform-bookkeeping cases. 786 tests pass (`cargo test -p temper-server --lib --features observe`), 63 of them conformance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SF1Xhjcg7zG38WjmfC239C
…es not close Re-review of the previous commit confirmed the injection fixed and found two claims in its own documentation that the code does not support, plus one residual the commit's framing implied was closed. `Entity` is not "the only writer whose action name the kernel chose". It is where the kernel ROUTED the name: a Cedar-permitted dispatch of an action no spec declares is refused inside the actor and still recorded on that source, so a caller who may dispatch at all can put a name of their choosing into the vocabulary. Reworded to say routed rather than chose, and the residual is named. "Every one of those Platform names is already in KERNEL_PLATFORM_ACTIONS" is false — `policy_saved` and `repl_execution` are platform-source names the list does not carry. Platform stays excluded, but for the reason that actually holds (it is what would admit a capture-loss marker), with the cost stated exactly: a decision naming one of those two is counted rather than judged. And the part that matters most for anyone reading this filter as a fix: the same unauthenticated `POST /api/authorize` still reaches the ROW walk. Aim its `resource_type` at the entity type under check and the denial row is judged as the actor's own execution, so its caller-chosen name reports as `unknown_action` with no vocabulary involved. That is older than this module and is deliberately not repaired here — denial rows are judged on purpose, because that is how `denied_then_retried` sees a denial, so changing it is a decision about denial-row provenance rather than about how a decision is classified. The two ways to close it (bind the session on that endpoint to an authenticated principal, or teach the walk that an `Authz` row records a question rather than an act) are written down beside the filter so the next reader inherits the choice instead of the impression that the class is handled. Comments only; no behaviour change. 63 conformance tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SF1Xhjcg7zG38WjmfC239C
Collaborator
Author
|
@greptile review |
Comment on lines
+213
to
215
| if vocabulary.contains(action) { | ||
| return DecisionActions::Actions(vec![action]); | ||
| } |
There was a problem hiding this comment.
Nested action claims are bypassed
When a decision has a vocabulary-recognized outer action and also carries trajectory_actions, this early return ignores every nested governed-action claim. An undeclared nested action is therefore omitted from the report, contrary to the documented contract that every trajectory_actions entry is judged.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/conformance/decisions.rs
Line: 213-215
Comment:
**Nested action claims are bypassed**
When a decision has a vocabulary-recognized outer action and also carries `trajectory_actions`, this early return ignores every nested governed-action claim. An undeclared nested action is therefore omitted from the report, contrary to the documented contract that every `trajectory_actions` entry is judged.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A conformance check of a clean Claude Code run returned
failwith oneunknown_actionviolation per harness tool call — 81 in the run that found it — each reporting that the agent "decided onBash, which the kernel never recorded a row for; no[[action]]in theCuratorAgentspec defines this name". The rows were clean,evidence_completewas true, the spec was pinned. The verdict was noise, and layer 1 could not returnpassfor any harness-driven run however conformant.is_action_nameaccepted any bare[A-Za-z0-9_.-]+token, soBash,Read,Write,Edit,Agent,ToolSearch,SendMessageand everymcp__*tool classified as a claim about a governed action. The existingHarnessToolescape only fired for the Temper MCP envelope, whosechoice.actioncarries a colon and a line of Python.The rule
A decision is judged as a governed-action claim only when something positively identifies it as one. Two things can:
choice.arguments.trajectory_actions. That key means "the governed actions this code called", so every name in it is judged whether or not the kernel recognises it — an envelope namingFrobnicateis exactly the fault to report.choice.actionthe newActionVocabularycontains — a name the actor spec declares, a name the platform defines (KERNEL_PLATFORM_ACTIONS), or a name the kernel dispatched somewhere in this session, on any entity.Everything else is the agent's own tooling: skipped from the action walk, counted, never reported.
Only rows the kernel wrote while dispatching on an entity (
TrajectorySource::Entity) may widen the vocabulary, and caller-named rows on that source (spec_governed = false, fromPOST /api/auditand the unmet-trajectory endpoint) plus capture-loss markers are excluded on top. Otherwise one record namedBashwritten into somebody else's session turns everyBashin their run into a violation of a spec they followed. See the review round below — the first version of this filter did not check the source, and that hole was real.What is not weakened
Kernel rows are untouched. An undeclared action, an illegal source state, a move after a terminal state, a denied-then-retried action — all still caught from the platform's own record, which is where the governance check lives.
On the decision side specifically, these survive and have tests: a platform verb the actor does not declare (
forbidden_action), an action the session dispatched on another entity, and every name inside an MCPexecuteenvelope.Blast radius, precisely: every decision the old rule judged and the new rule skips is one it can no longer report. The only shape where the new rule judges something the old one did not is a bare, action-shaped
choice.actionthat also carries atrajectory_actionslist — the old rule took the bare name and ignored the list. No current producer emits that shape.What this cannot do, and what that costs
A bare
Bashand a bareFrobnicateare the same token from the same producer path. Nothing in the record distinguishes a harness tool from an action name the kernel has never seen — a transcript converter writes both aschoice.action = <the tool the model called>. So the checker cannot have both "no false violations for harness tools" and "every unplaceable name reported". This is the conservative half, and the module doc says so.What is given up is one candour signal: an agent that reports reaching for an action which never reached the platform, whose name no spec in the walk declares and which nothing in the session dispatched, is counted rather than reported. The attempt failed before the kernel saw it; what is lost is the record that it was made — not any action that occurred.
Two existing tests encoded that lost case with an invented name; both are rewritten to use a name the kernel can place (
Delete), so "decisions still fail a run" stays pinned.The way to narrow the gap is to add positive identification, never to widen what counts as a name: reading a governed target out of a decision's arguments (as katagami's offline
conformance_check.pyalready does), or givingActionVocabularythe tenant's other registered specs. Both are additive — they can only move decisions from skipped to judged — which is why the vocabulary is one type with one constructor.Stats
ots_decisions_skipped_as_harness_toolkeeps its exact meaning: an envelope whose name is not even shaped like an action name. The newots_decisions_skipped_as_unrecognized_namecounts the new class, so the two skip reasons stay distinguishable and the checker's blind spot is visible in every report rather than folded into a line that reads as certainty.Tests
cargo test -p temper-server --lib --features observe— 786 pass, 0 fail, 63 of them conformance.New coverage: the Bash/Read/Write/Edit/Agent/ToolSearch/SendMessage/
mcp__*regression fixture; harness tools interleaved with real declared actions; an undeclared action reached through the harness still reported; an action the session dispatched on another entity still judgeable; the caller-supplied audit row that must not widen the vocabulary; the capture-loss marker that must not either.Mutation-tested. Restoring the pre-fix rule (bare token wins) fails 7 of the new tests, including the regression fixture — so they pin the behaviour rather than merely accompanying it.
Live verification
A local kernel with the katagami-curation app loaded, driving a real
CuratorAgentthroughReceiveBrief → BeginDrafting → RecordDraft → SelfReview, then posting a 13-decision Claude Code trajectory (12 harness tools + one declared action) for the same session and callingPOST /api/conformance/check:A second live run confirms the check was not blunted — a
Deletedecision the spec never declares is still reported:Relationship to katagami PR #204
#204 is the producer-side half of the same symptom: the converter writes harness calls as
"<harness> tool: <name>"envelopes. It states it does not depend on this change, and the two compose correctly. With #204, those decisions carry a colon and land inots_decisions_skipped_as_harness_tool— exactly what its evidence table expects; without it they land in the new counter. All five source fragments #204 pins inkernel_decision_contract.json(TRAJECTORY_ACTIONS_KEY,NESTED_ACTION_KEY, theis_action_namesignature, and its two character predicates) survive this refactor verbatim, so its kernel contract test still passes.Known remaining defect, neither PR's scope: a genuine cross-entity governed call (
Workspaces('x')/Temper.ResolvePathfrom a curator run) still reads asunknown_actionagainstCuratorAgent, because the decision path attributes every named action to the actor's own entity while the row path does not. #204 now records the entity set each call drove, so the kernel could read it and skip entries belonging to another actor — a follow-up that depends on #204 landing first.Review round (three fresh-context adversarial reviews, findings closed)
A vocabulary-injection hole, found and fixed (commit 2). The first commit's filter accepted any row not marked
spec_governed = false.record_authz_denialwrites rows with that flag UNSET, and the unauthenticatedPOST /api/authorizereaches it with a caller-supplied action name, resource type andX-Temper-Ctx-SessionId. So one call naming actionBashagainst somebody else's session putBashin that session's vocabulary and turned everyBashin their clean run into a violation — the exact regression this branch removes, reintroduced against a targeted session.The filter is now an allowlist of one source,
TrajectorySource::Entity, with thespec_governedtest kept on top (nothing stops a caller-named audit row claimingEntity). Two new tests, both mutation-checked: reverting to the flag-only filter fails them.Two claims corrected, one residual named (commit 3).
Entityis where the kernel routed a name, not always where it chose one — a Cedar-permitted dispatch of an undeclared action is refused inside the actor and still recorded there. And "every Platform name is already inKERNEL_PLATFORM_ACTIONS" was false (policy_saved,repl_execution).Residual, deliberately not fixed here and documented beside the filter: the same unauthenticated
POST /api/authorizestill reaches the ROW walk. Aim itsresource_typeat the entity type under check and the denial row is judged as the actor's own execution, so its caller-chosen name reports asunknown_actiondirectly, with no vocabulary involved. That is older than this module and untouched by these commits. It is not repaired here because denial rows are judged on purpose — it is howdenied_then_retriedsees a denial — so changing it is a decision about denial-row provenance, not about how a decision is classified. The two ways to close it (bind that endpoint's session to an authenticated principal, or teach the walk that anAuthzrow records a question rather than an act) are written down indecisions.rsso the next reader inherits the choice rather than the impression that the class is handled. Worth its own issue.Final state: 786 tests pass in
cargo test -p temper-server --lib --features observe, 63 of them conformance. The push ran the repo's pre-push gates — rustfmt,cargo clippy --workspace -- -D warnings, the readability ratchet, andcargo test --workspaceacross 99 test binaries — all green.🤖 Generated with Claude Code
https://claude.ai/code/session_01SF1Xhjcg7zG38WjmfC239C
Greptile Summary
This PR distinguishes harness tool calls from governed-action decisions by introducing a deployment-aware action vocabulary and a separate counter for unrecognized names.
Confidence Score: 4/5
The PR appears safe to merge, with one non-blocking classification inconsistency for a valid but currently unproduced decision shape.
The harness-tool regression is addressed with narrow vocabulary sources and broad tests, but recognized outer actions bypass nested
trajectory_actionsdespite the module’s contract that all nested governed-action claims are judged.Files Needing Attention: crates/temper-server/src/conformance/decisions.rs
Important Files Changed
ActionVocabularyand the new decision-classification rules; recognized outer actions currently suppress nested governed-action claims.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD D[OTS invocation decision] --> T{Invocation type?} T -->|Thought| ST[Count as thinking] T -->|Callable| V{Outer action in vocabulary?} V -->|Yes| O[Judge outer action] V -->|No| N{trajectory_actions present?} N -->|Yes| G[Judge nested governed actions] N -->|No| S{Outer token action-shaped?} S -->|Yes| U[Count as unrecognized name] S -->|No| H[Count as harness envelope] O --> C[Compare against actor spec and recorded rows] G --> CPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: only an entity dispatch may widen t..." | Re-trigger Greptile