feat(runtime): add channel-neutral execution context

[chitcommit]

Closes #181.

Changes

• adds a request-scoped execution context after actor and tenant authorization
• uses existing X-Source-Service plus W3C traceparent/baggage for provenance
• keeps source/channel/workspace/session metadata audit-only; it cannot change financial tenant scope
• classifies HTTP operations as read / suggest / preview / execute
• lets MCP replace generic HTTP metadata with canonical MCP capability + intent
• automatically enriches ChittyLedger audit entries with execution provenance
• no database/schema changes

Portable projection

The same runtime context can be populated by ChatGPT, Claude, or ChittyClaw/OpenClaw adapters without provider-specific finance logic. ChittyAuth remains actor authority; tenant_users remains financial authority.

Verification

Adds focused execution-context tests. Full repository CI is the execution proof.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-03T13:28:36.096484Z	dffa4d5	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5b5a6ee6-57cc-4f5d-ad9c-360b028cc2cb

📥 Commits

Reviewing files that changed from the base of the PR and between 5f4939d and b21d4c4.

📒 Files selected for processing (12)

• AGENTS.md
• CHARTER.md
• CHITTY.md
• SECURITY.md
• server/__tests__/execution-context.test.ts
• server/__tests__/ledger-client.test.ts
• server/__tests__/mcp.test.ts
• server/app.ts
• server/env.ts
• server/lib/ledger-client.ts
• server/middleware/execution-context.ts
• server/routes/mcp.ts

 ____________________________________
< Zero-day? Zero chance on my watch. >
 ------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

Clean, small, no schema changes, and the tests cover the main paths. The scope handling is right: tenantId comes only from c.get('tenantId'), and the test shows baggage can't change it. Points below, most important first.

Concerns

1. Unvalidated caller-controlled values go into the audit trail. X-Source-Service, baggage (channel/workspace/session) and traceparent are stored verbatim in ChittyLedger metadata. They have no length cap, no format check and no allowlist. traceparent should match the W3C format (00-<32 hex>-<16 hex>-<2 hex>), and be dropped if it doesn't. Cap the other values (e.g. 128 chars, printable only). Also consider whether X-Source-Service should be trusted as provenance when authMethod is session/chittyauth, as opposed to service. Today a user can claim to be chittyclaw. The PR text says the metadata is audit-only, but audit readers may still trust it, so I'd mark it as claimed or only honor it for service auth.
2. Intent inference is a substring match. pathname.includes('/preview') and '/advice' match anywhere in the path, so something like /api/x/preview-and-commit would be recorded as preview even though it mutates. The failure that matters is a mutating route being labelled non-mutating. Match on whole path segments, or let routes declare their intent with setExecutionOperation.
3. MCP intent is hardcoded. toolName === 'get-property-advice' ? 'suggest' : 'execute' records every other tool, including read-only ones, as execute. That will cause false positives if anything later gates on execute. A per-tool intent field in TOOLS would keep this in one place.
4. capability can have unbounded cardinality. finance.mcp.resources.read:${uri} embeds the raw client URI. Cap or normalize it. For HTTP paths, normalizePath doesn't handle non-UUID/non-numeric ids (slugs, ChittyIDs).

Minor

• new URL(c.req.url).pathname is parsed twice; use c.req.path once.
• requestId is always a new UUID. If a valid traceparent is present, reuse its trace-id. Also consider echoing the id in an X-Request-ID response header.
• executionAuditMetadata is exported but unused, and ledgerLog reimplements it. Use it there, or delete it. ledgerLog also types get as returning any; use FinanceExecutionContext | undefined.
• The middleware sits only in protectedRoute. Routes that use authAndContext without the tenant middleware get no context, and ledgerLog silently skips enrichment there. Worth a comment.
• If entry metadata already contains an execution key, the spread overwrites it. That's probably fine, but note it.

Tests

• Missing: malformed or oversize baggage and traceparent, ; properties in baggage, the chitty.source fallback, and a ledgerLog enrichment test that asserts the execution metadata reaches logToLedger.
• There's no integration test that /mcp sets the capability or intent. The test uses a stub route rather than the real handler.

Nothing here blocks the design. I'd fix 1 and 2 before merge.

🤖 Generated with Claude Code

[claude]

Review: channel-neutral execution context

Overall this is a clean, small change. It adds no schema changes, it keeps authorization separate from provenance, and the middleware runs after tenantMiddleware, so tenantId can't be influenced by headers. The tests cover that invariant.

Issues / suggestions

1. Provenance values are unbounded and unvalidated, and they land in the audit ledger. X-Source-Service, baggage values and traceparent are copied verbatim into metadata.execution by ledgerLog. I'd add a length cap per field (say 128 characters), a charset check, and a W3C format check for traceparent (^00-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$), dropping the value on mismatch. X-Source-Service is also spoofable. The docs say "untrusted", but the ledger should make clear that source.service is a claim and not an attestation.

2. MCP tools/call defaults to execute. In server/routes/mcp.ts only get-property-advice is suggest. Any other tool, including read-only ones, is labeled execute. That mislabels audit data, and it becomes a bug if intent is ever enforced. Derive intent from per-tool metadata in TOOLS instead of a hard-coded name check.

3. Intent inference is path substring matching. includes('/preview') and includes('/advice') match anywhere in the path, and every other non-GET is execute. That is fine as a label, but please state in the docs that intent is advisory and not enforced. Anchoring on path segments would be more precise.

4. Dead and duplicated code. executionAuditMetadata is exported but unused, and ledgerLog re-implements it inline. new URL(c.req.url).pathname is parsed twice, so use c.req.path. ledgerLog types get as returning any. Type it as FinanceExecutionContext | undefined.

5. baggage parsing. The W3C spec limits baggage to 180 members and 8192 bytes. Bail out early on oversized input. The decodeURIComponent fallback is untested.

6. Test coverage gaps.

   • There is no test that ledgerLog attaches metadata.execution, which is the main behavioral change.
   • The MCP test uses a stand-in route, so the setExecutionOperation calls in mcp.ts are untested.
   • parseBaggage has no tests for malformed, oversized or ;prop-suffixed members.
   • The no-context path in ledgerLog is untested.

None of these look blocking, though items 1 and 2 are worth fixing before merge.

🤖 Generated with Claude Code

[claude]

Review: channel-neutral execution context

Clean, small change. No schema changes, and provenance stays separate from authorization (the test showing tenant-attacker in baggage doesn't change scope is good). Findings, most important first.

Issues

1. Unbounded, client-controlled values go into audit records. X-Source-Service, baggage members, traceparent and the MCP uri (via finance.mcp.resources.read:${uri}) are copied verbatim into executionContext, and ledgerLog ships them to ChittyLedger. Cap lengths (e.g. 128 chars per field, plus a limit on baggage members), strip control characters, and validate traceparent against the W3C format. Otherwise a caller can bloat ledger entries or inject misleading values. X-Source-Service is freely settable, so ledger consumers should treat source.* as unverified; worth a comment in ledger-client.
2. Intent inference can mislabel mutating requests. inferExecutionIntent uses pathname.includes('/preview' | '/suggest' | '/advice') on any non-GET. A route like POST /api/x/advice/apply or /api/suggestions/:id/accept would be audited as suggest/preview even though it writes. If intent later feeds policy (L0-L4 trust), this fails open. Match the last path segment or use an explicit allowlist; unmatched stays execute.
3. MCP tools/call intent. Everything except get-property-advice is execute, with the name hard-coded in the switch. Put intent on the TOOLS definitions so new read-only tools aren't mislabelled. Unknown tool names are also recorded as capabilities before callTool rejects them (see 1).
4. MCP coverage depends on the mount. setExecutionOperation spreads c.get('executionContext'). If /mcp isn't mounted with protectedRoute, current is undefined and you get a context with no actor or scope. Confirm the mount in app.ts, and make setExecutionOperation throw or no-op when the context is missing.

Minor

• new URL(c.req.url).pathname is computed twice; use c.req.path.
• executionAuditMetadata is exported but unused, and ledgerLog re-implements it. Use it or remove it, and type get as returning FinanceExecutionContext | undefined instead of any.
• ledgerLog silently overwrites any caller-supplied metadata.execution. Probably intended, but add a comment.
• parseBaggage has no limits and keys aren't validated.
• requestId is generated but never returned (e.g. an X-Request-Id header), which limits its usefulness for audit linkage.

Tests
The middleware unit tests are good. Missing:

• ledgerLog enrichment (with and without a context, and with executionCtx throwing).
• Malformed or oversized baggage, malformed traceparent, and the ; property syntax.
• Misleading paths for item 2 (e.g. /api/x/advice/apply).
• An integration test through createApp showing /mcp gets a context with the right capability per method.

No blocking security problem found, since authorization is unaffected. I'd fix 1, 2 and 4 before merge.

🤖 Generated with Claude Code

[claude]

Review: channel-neutral execution context

Overall this is a clean, small, additive change. The security posture is right: the context is built after tenantMiddleware, scope.tenantId comes only from the authorized c.var.tenantId, provenance is sanitized and size-capped, and claimed: true marks source as untrusted. There are no schema changes. I read the diff only and did not run CI or the tests.

Issues / suggestions

1. Capability silently collapses to finance.unknown. setExecutionOperation runs sanitizeValue on the capability, and that rejects anything over 128 chars or non-printable. finance.mcp.resources.read:${uri} embeds a caller-supplied URI, so a long URI turns the audit label into finance.unknown. That loses the very thing the label is for. Truncate instead of dropping, or use a separate capability sanitizer with a larger limit. The uri also goes into the label unvalidated.
2. Intent inference is heuristic, and the default is execute. Any non-GET that lacks a preview/suggest/advice path segment is labelled execute. That fails safe for auditing, but the test name says "mechanically", and if anyone later gates behaviour on intent it will misclassify (for example, a POST search endpoint, or a GET with side effects). Please add a comment that intent is audit-only and must not be used for authorization.
3. normalizePath only handles UUIDs and numeric IDs. Slugs, hashes and account codes stay in capability. That gives high-cardinality labels and can put identifiers into ledger metadata.
4. MCP behaviour change. tools/call now returns a 400 Unknown tool before callTool. This looks fine, but check that it matches the previous error code/status for clients, and add a test.
5. requestId is a fresh randomUUID(). Consider reusing the trace id from traceparent, or cf-ray, when present, so ledger entries correlate with edge logs.
6. Ledger enrichment spreads execution over entry.metadata. This overwrites any existing metadata.execution key. That's probably intended, but worth a comment. The get?: optional typing on ledgerLog also means callers that pass a non-Hono context silently skip enrichment. Passing the real Context would be sturdier.
7. Source claims in the ledger. workspace/session values come from an untrusted header and are persisted in an immutable audit trail. Length and charset are bounded, so this is acceptable. Make sure the ledger consumers treat them as claims, which claimed: true conveys.

Test coverage

Good unit coverage of intent inference, baggage parsing and the scope-isolation case. Missing:

• ledgerLog enrichment (the headline feature) has no test.
• Malformed input: bad traceparent, oversize baggage (>8 KB), invalid percent-encoding, non-printable values, and a ; property suffix.
• An integration test through createApp showing /mcp actually sets canonical capability/intent, and that unknown tool returns 400.
• The finance.unknown fallback from point 1.

No blocking issues. I'd address points 1 and 4, and add the ledger and malformed-header tests, before merge.

[claude]

Review: channel-neutral execution context

Overall this is a clean, small, additive change. The middleware runs after tenantMiddleware, scope comes only from c.get('tenantId'), there are no schema changes, and the provenance parsing is defensive. Findings below, roughly by importance.

Worth addressing

1. No test covers the ledger enrichment or the MCP wiring. The tests exercise the middleware and a fake /mcp-test route. Nothing tests that ledgerLog merges metadata.execution, or that /mcp tools/call and resources/read set the expected capability and intent. Add a ledgerLog test with a stub c.get and a stub waitUntil. Add one real /mcp request test too.
2. setExecutionOperation can silently lose the capability. sanitizeValue caps values at 128 characters and printable ASCII. finance.mcp.resources.read:${uri} embeds a caller-supplied URI, so a long or non-ASCII URI becomes finance.unknown, and the audit trail loses which resource was read. Consider truncating, or recording only the URI scheme and path template.
3. Behavior change in tools/call. An unknown tool now returns -32602 with HTTP 400 from the route, before callTool. Check that this matches what callTool returned before (error code and status), and that any existing MCP tests or clients don't depend on the old shape.
4. The execution key in ledger metadata is overwritten. { ...entry.metadata, execution } replaces any execution field a caller already set. This is unlikely to matter, but a namespaced key would be safer.

Minor / design notes

• X-Source-Service is spoofable. It is marked claimed: true, which is good, but ledger readers will see source.service: "chittyauth" for any caller who sends that header. For authMethod: 'service', consider binding the source to the authenticated service identity so audit consumers can tell the two apart.
• Intent inference is path-based. A POST that only reads (search or query endpoints) is labeled execute. A mutating route with preview or advice in its path would be labeled non-executing. Since the label is audit-only this is acceptable, but keep it in mind before anyone uses intent for gating. Documenting it as non-authoritative would help.
• Workspace and session IDs go into the immutable ledger. They are caller-supplied values stored in an append-only log. Confirm that is acceptable from a privacy and retention standpoint.
• Baggage parsing: duplicate keys resolve as last-wins, and a malformed member is skipped rather than rejecting the whole header, which is good.
• Coverage gap: the middleware is only on protectedRoute. Routes that use authAndContext alone get no executionContext, and ledgerLog silently skips enrichment for them. A one-line comment would make that explicit.
• Type: claimed: true as a literal type is slightly odd. It's fine now, but it will need changing if a verified source is ever added.

No security blockers found: provenance never feeds authorization, and the tests assert that. I couldn't run CI here, so I haven't verified the tests pass.

[claude]

Review: channel-neutral execution context

The design is sound. The context is built after auth and tenant checks, and scope comes only from c.get('tenantId'). Provenance is sanitized and size-capped, and there is no schema change. The comments below are mostly small.

Issues / suggestions

1. Missing tests for the MCP wiring and ledger enrichment. The tests cover the middleware in isolation using a fake /mcp-test route. Nothing exercises these:

   • mcpRoutes setting capability and intent per method, including tools/call using TOOL_DEFINITIONS[].intent.
   • ledgerLog merging metadata.execution.
   • The protectedRoute ordering in app.ts, meaning executionContext is present after tenantMiddleware.

   A small ledgerLog test with a stub c.get and a stubbed logToLedger or fetch would cover the main selling point of the PR.

2. tools/call behavior change for unknown tools. The handler now returns -32602 with HTTP 400 before reaching callTool. Please check this matches what callTool returned for unknown tools before (probably -32601 or a thrown error). If it differs, clients could notice. Keeping the old error path and only labeling known tools would be the safer option.

3. setExecutionOperation can hide the real capability. sanitizeValue caps values at 128 characters and rejects non-printable characters. A long finance.mcp.resources.read:<uri> therefore collapses to finance.unknown in the audit log. Truncate instead, or log the resource type without the full URI. The URI can also hold identifiers you may not want in every audit row.

4. Path-based intent inference may under-classify. Any non-GET request with a suggest or advice segment is labeled suggest, even if that route also persists data. The fallback to execute is the right fail-safe, but a route like POST /api/.../advice/apply would be mislabeled as non-mutating. Consider restricting suggest and preview to the last path segment, or letting routes opt in explicitly through setExecutionOperation. Also, HEAD/OPTIONS are not in the CORS allowMethods, which is harmless but worth knowing.

5. Minor points:

   • claimed: true as a literal type is always true. Either drop it or add a verified state later, for example when the service-token lane authenticates the source. At the moment it adds noise to every audit row.
   • trace.requestId is a fresh random UUID. Consider reusing an existing request ID (such as cf-ray, if available) or deriving it from the traceparent trace-id so logs correlate.
   • In ledgerLog, metadata.execution overwrites any caller-supplied execution key. That is probably intended, but a comment would help.
   • .get('chitty.channel') || undefined is redundant, because the map values are already non-empty.
   • The workspace-vs-tenant test asserts source.workspace equals tenant-attacker. That is correct, but the intent reads better if it also checks the value is not used for authorization (for example by asserting scope only).

Security

There are no concerns that block merge. Header and baggage inputs are length-bounded, restricted to printable ASCII and percent-decoded safely. No provenance field feeds authorization. Stored audit values are attacker-controlled strings, so downstream ledger consumers should treat them as untrusted.

Style

This matches the repo conventions: Hono middleware, types in HonoEnv, and no raw DB access. The docs updates are consistent.

Overall this is good to merge once the MCP/ledger tests are added and the tools/call error-code change is confirmed.

[claude]

Review: channel-neutral execution context

Overall this is a clean, small, well-documented change. It adds no schema changes, and the middleware is correctly placed after tenantMiddleware. Provenance is treated as untrusted and kept separate from authorization, and the sanitization is sensible (length cap, printable ASCII, baggage size cap, strict traceparent regex). I haven't run the tests or typecheck locally. This review comes from reading the diff.

Potential issues

1. Intent inference can mislabel mutating routes as suggest. inferExecutionIntent returns suggest for any non-GET whose path contains an advice segment (or suggest). If a mutating POST/PUT is ever mounted under such a path, the audit trail will say "suggest" for a write. Consider defaulting non-GET to execute unless a route opts in explicitly via setExecutionOperation, or at least drop advice and comment that the inference is advisory.

2. setExecutionOperation silently degrades to finance.unknown. sanitizeValue caps values at 128 chars. resources/read:${uri} interpolates a caller-supplied URI, so a long or non-printable URI turns the capability into finance.unknown, which loses the canonical capability the PR is meant to record. Validate the URI against the known RESOURCES and use the canonical name, or truncate instead of dropping.

3. Unknown-tool behavior change in MCP. tools/call now returns a 400 with -32602 for an unknown tool before reaching callTool. If callTool previously returned a different error shape, this changes behavior for existing clients. Please confirm that's intended. Also, ping, notifications/* and the default branch never call setExecutionOperation, so they keep the generic finance.http.post:/mcp capability.

4. ledgerLog enrichment. { ...entry.metadata, execution } overwrites any existing metadata.execution; a namespaced key or collision check is safer. The caller-claimed workspace/session/channel is now forwarded to the external ledger, so note in SECURITY.md that these are claimed, not verified. claimed: true as a literal type carries no information; compute it from whether a header was actually present.

5. trace.requestId is always a fresh UUID. It ignores an inbound X-Request-ID or the trace-id from a valid traceparent, which would help correlation (optional).

Nits

• setExecutionOperation spreads current, which would throw if the middleware isn't mounted on that route. A clearer guard would help.
• baggage.get(...) || undefined is redundant.
• The 'finance.chitty.cc' fallback for source.service makes "no header" indistinguishable from a self-claim by that name.

Test coverage

• Good unit coverage of the middleware, including the key property that tenant scope isn't derived from baggage.
• Missing: a ledgerLog test (metadata attached, existing metadata preserved, no-op without c.get).
• Missing: an integration test through createApp on a real protected route.
• Missing: an MCP test against the real /mcp route in mcp.ts (tools/call suggest vs execute, resources/read, unknown tool). The current MCP test uses a stub route in the test file, so it doesn't exercise mcp.ts.
• inferExecutionIntent should also be tested with PUT/PATCH/DELETE.

Performance and security

• Performance cost is negligible (one baggage parse and one randomUUID per request).
• I found no security blockers. The provenance-versus-authority separation is sound.

Items 1–3 are the ones I'd address before merge.

[claude]

Review: channel-neutral execution context

Overall this is a clean, small, well-scoped change. It adds no schema changes, it keeps provenance audit-only, and the tests cover the main sanitization paths. Notes below, roughly by priority.

Issues / suggestions

1. Ledger enrichment is untested. ledgerLog now merges execution into metadata, but no test covers it. Please add a test for three cases: a context is present, no context is present, and an entry that already has metadata. The spread order (execution is written last) also means a caller-supplied metadata.execution is silently overwritten. That is probably what you want, but it should be explicit.
2. source.claimed: true is a literal type, and the default is misleading. When no header or baggage is supplied, service falls back to finance.chitty.cc but is still marked claimed: true. Make claimed a real boolean, false for the fallback. Otherwise auditors can't tell a self-attributed request from a caller-asserted one.
3. setExecutionOperation silently degrades. sanitizeValue(capability) ?? 'finance.unknown' applies the 128-char cap to finance.mcp.resources.read:${uri}. A long resource URI collapses to finance.unknown and loses the audit signal. Truncate instead, or leave the URI out of the capability and put it in the audit metadata. If it is called when no context exists, it spreads undefined, so a guard would be safer.
4. Capability cardinality. normalizePath only collapses UUIDs and numeric segments. Slug-style IDs produce unbounded distinct finance.http.* capabilities. That is a problem if these values are ever aggregated or used for policy.
5. Intent heuristic defaults to execute. For non-GET methods this is the safe default. But the segment match means a mutating POST to /api/foo/advice is labelled suggest. If intent may later feed trust-level (L0-L4) decisions, document that HTTP intent is a heuristic and not enforcement, and preferably declare it explicitly per route for mutating endpoints.
6. Behavior change in MCP. An unknown tool now returns 400 / -32602 instead of 500 / -32000. This is more correct and the test was updated. Please mention it in the PR description in case clients depend on the old code.
7. Minor items. TRACEPARENT_RE accepts all-zero trace and span IDs, which the W3C spec treats as invalid. requestId is always a fresh UUID, so consider returning it in a response header for correlation. The || undefined on the baggage lookups is redundant.

Security

• The design is sound. Provenance is not used for authorization, the context is built after the tenant check, and the length, printable-ASCII and baggage-size limits are sensible.
• The chitty.source baggage fallback lets any caller, including a service-token one, assert an arbitrary source.service. This is documented as claimed. Item 2 would make that distinction machine-readable.
• The workspace=tenant-attacker test is a nice guard against scope confusion.

Performance

• The cost is negligible: a few header parses and one randomUUID per request.

Tests

• Good coverage of the middleware. The gaps are ledger enrichment (item 1) and an integration check that protectedRoute mounts the middleware on /mcp and that MCP overrides reach the ledger entry.

No blocking issues. I'd address items 1-3 before merge.

🤖 Generated with Claude Code

[claude]

Review: channel-neutral execution context

Overall this is a clean, small, no-schema change. Provenance is sanitized and kept audit-only, and the middleware runs after tenantMiddleware, which is the right place. I read the diff but did not run the tests. Notes below, roughly by priority.

Issues / suggestions

1. trace.requestId is not unique per request (execution-context.ts). It is set to the trace-id from traceparent. One distributed trace usually spans many requests, so ledger entries from different requests will share a requestId. Always generate crypto.randomUUID() for requestId and keep the trace-id separately (for example trace.traceId). A client can also pick the requestId by sending a crafted traceparent.

2. Path-suffix intent heuristics can mislabel mutating routes. inferExecutionIntent treats any POST ending in /preview, /suggest or /advice as non-executing. That is only correct if those handlers never write. The fail-safe default of execute is good. Since the label lands in the audit trail, I'd either allowlist exact known routes or have routes declare their intent through setExecutionOperation. The preview-and-commit test only covers one naming pattern. Add a test for /api/x/preview/commit, which should be execute because the terminal segment is commit.

3. ledgerLog enrichment can clobber and is untested (ledger-client.ts). { ...entry.metadata, execution } silently overwrites any caller-supplied metadata.execution. Also, ledgerLog is called from the unauthenticated webhook handlers (server/books/webhooks.ts), where executionContext is unset. That falls through safely, but there is no test covering enriched versus un-enriched entries. Please add a small test for both cases and for the key collision.

4. Unverified claims in audit metadata. source.service comes from the caller-controlled X-Source-Service header. The claimed: true flag helps. Consider also recording a verified-versus-claimed distinction when authMethod === 'service', so auditors can tell a trusted internal caller from an arbitrary JWT user claiming chittyclaw. The workspace and session values are written verbatim to an immutable ledger. That is acceptable, but confirm they are not PII.

5. Minor

   • setExecutionOperation accepts a free-form capability and silently turns an invalid one into finance.unknown. Typing it as a template-literal union or constant map would catch typos at compile time.
   • RESOURCE_CAPABILITIES[uri] ?? '...unknown' uses a caller-controlled key on a plain object. Use Object.hasOwn or a Map so __proto__ or constructor can't resolve to inherited properties. The ?? 'unknown' fallback does not cover that.
   • baggage parsing splits on , before handling percent-encoding. That is correct per the W3C spec, but a decoded value with a comma or = is still accepted by sanitizeValue. Fine, just noting it.
   • The ReadonlyMap and OPTIONS handling is fine. The HEAD/OPTIONS read classification is OK.

Tests

Good coverage of parsing, oversized input and the MCP overrides. Gaps: ledger enrichment (3), requestId uniqueness (1), and a test that the middleware is actually wired into protectedRoute with the real tenant middleware, so scope.tenantId is checked end to end.

Docs

The docs updates are consistent with CLAUDE.md conventions. No mocks or schema changes.

Nothing blocking apart from (1), which I'd fix before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dffa4d5aff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Mount execution context on allocation routes

The new middleware only runs through protectedRoute, but protectedPrefixes does not include /api/allocations. Consequently, real requests to /api/allocations/preview and /api/allocations/execute never receive an executionContext, despite the new test asserting those intents using an isolated app that mounts the middleware globally; in particular, the allocation.executed ledger entry remains unenriched. Add the allocation prefix to the production middleware registration rather than relying on the test-only setup.

Useful? React with 👍 / 👎.

[claude]

Review: channel-neutral execution context

Clean, small, additive change. Authorization is not derived from provenance (tenantId/userId come from earlier middleware), and the tests cover the main paths.

Should address

1. requestId is taken from the caller's trace-id (execution-context.ts). A client can send the same traceparent on every request, so requestId stops being unique, and it is written to the immutable ledger. Always generate requestId server-side and keep the trace-id as a separate field.
2. Caller-claimed source.service/channel/workspace/session go into the ledger. Sanitization is good, but claimed: true only means the header was present. A caller can claim finance.chitty.cc or chittyclaw. Add a comment on the ledgerLog enrichment, and make sure audit consumers don't treat source.service as trusted.
3. Generic HTTP capability uses the raw path. normalizePath only strips UUIDs and numeric segments, so slugs, Wave/Stripe IDs or emails can leak into the capability string and inflate cardinality. Prefer c.req.routePath (the matched pattern).

Minor

• Intent inference only looks at the terminal path segment. Document the convention that preview/suggest/advice routes must end in that segment. Fail-safe to execute is the right default.
• ledgerLog silently overrides any existing metadata.execution key. Add a one-line comment.
• Confirm ChittyLedger has no metadata size limit, since every audit entry now carries more data.
• setExecutionOperation throws if the context is missing. Confirm mcpRoutes is only mounted behind protectedRoute.

Tests
Good coverage of sanitization, tenant-spoofing and the MCP override. Missing: a test that requestId isn't caller-controlled, a percent-encoded baggage value test, and a test of middleware ordering after tenantMiddleware.

Only item 1 is worth fixing before merge.

[claude]

Review

Overall: clean, small, well-scoped, and the security stance is right. Provenance is applied after tenant auth and never feeds back into tenantId/userId. Inputs are length-capped and sanitized, and the tests cover the attacker-workspace case. Notes below, roughly by priority.

Issues / suggestions

1. trace.requestId is caller-controlled (execution-context.ts). It is set to the traceparent trace-id when present, so a client can reuse one trace-id across many requests, or replay a victim's trace-id. Every audit row then shares a "request id", and the ledger can't tell requests apart. Keep requestId = crypto.randomUUID() and store the trace-id separately, e.g. traceId. traceparent is already stored, so correlation is preserved.
2. Unbounded capability cardinality. normalizePath only collapses UUIDs and numeric segments. Slugs, account codes, or other non-UUID ids pass through, so finance.http.post:/api/... becomes high-cardinality if anyone aggregates on it. Consider using Hono's c.req.routePath after next(), or a post-routing hook, for the canonical pattern.
3. Intent inference is a heuristic that writes to the audit log. Terminal-segment matching on preview/suggest/advice is fine as fail-safe (default execute). But a POST ending in /preview that actually persists would be recorded as preview. The docs say intent is non-authoritative, which is good. I'd add a note on the route-owner side that routes labeled preview must be side-effect-free.
4. ledgerLog overwrite semantics. execution is spread last, so a caller-supplied metadata.execution is silently replaced. Probably intended, but add a test or comment. Also, source.workspace/session are caller-claimed free text going into an immutable ledger. Capped at 128 printable chars, which is OK, but worth confirming the ledger treats metadata as untrusted.
5. Coverage gap. Auth/tenant-failure ordering isn't tested. Nothing checks that executionContextMiddleware only runs after tenantMiddleware, or that it throws sensibly if userId/tenantId is unset. Also nothing tests that ledgerLog still works when c.get throws or there's no context (public/webhook routes), though the code handles it. A test that the real createApp protected routes populate the context would help.
6. Minor: the baggage parser splits on ,/; without handling quoted values, which is acceptable given it's lenient. setExecutionOperation for unknown MCP tools records intent: 'execute' with the generic capability, which is a good fail-safe. The resources/read unknown-URI path logs ...:unknown as read before the handler rejects it, which is fine.
7. Docs: CHITTY.md edit is long for an Architecture paragraph; consider moving the detail to the new SECURITY section and linking.

No blocking security concerns beyond #1, which I'd fix before merge since it undermines the audit-correlation purpose.

[claude]

Review

Overall: a clean, well-scoped change. Context is built after auth and tenant resolution. Provenance is sanitized and size-bounded. scope.tenantId comes only from the authorized c.var.tenantId, and a test proves a spoofed workspace can't change it. No schema change.

Issues / suggestions

1. trace.requestId is not unique per request (execution-context.ts, ~line 588). It is set to the trace-id from traceparent whenever that header is present. Several requests in one trace then share a requestId, and a caller can choose the value. For an audit trail, requestId should always be a fresh crypto.randomUUID() (or a server-side id such as cf-ray). trace.traceparent already carries the trace correlation.
2. Caller-claimed values go into the immutable ledger. ledgerLog embeds source, channel, workspace and session (all caller-controlled) in metadata.execution. The claimed flag and the SECURITY.md note help, but consumers must treat these as untrusted. For example, X-Source-Service: chittyledger-admin is accepted verbatim. Consider namespacing them under claimed.*, apart from the server-derived fields (actor, scope, capability).
3. Intent inference is a label only, and it is lenient. Unmatched POSTs default to execute, which fails safe. A mutating route ending in /advice would be recorded as suggest. That is audit-only, but worth a comment or a route-level override.
4. Silent override in ledgerLog. { ...entry.metadata, execution } overwrites any execution key the caller already set. This is probably intended, but a one-line comment would say so.
5. setExecutionOperation throws execution_context_unavailable if the middleware isn't mounted. /mcp is protected, so it works today. /api/tenants uses authAndContext without the execution middleware, so a future call there would 500. Consider a no-op fallback.
6. Unknown MCP resource URIs record finance.mcp.resources.read:unknown. Consider including the sanitized URI.

Tests

• The unit tests are good: sanitization, oversized baggage, the tenant-spoof case, and MCP overrides.
• mcp.test.ts injects executionContext by hand, so the real wiring isn't exercised. Nothing covers the middleware order in app.ts (tenant, then execution context). Nothing checks that a real route calling ledgerLog emits metadata.execution. One integration-style test through createApp would cover both.
• Missing edge cases: duplicate baggage keys, percent-encoded values, and an uppercase traceparent.

Other

• Allowing traceparent and baggage in CORS is appropriate.
• No performance concerns.
• CLAUDE.md conventions (tenant scoping, no mocks) are respected.

Nice work. Item 1 is the one I'd fix before merge.

🤖 Generated with Claude Code

[claude]

Review of #182: channel-neutral execution context

The middleware runs after auth and tenant checks. Provenance is audit-only, and there are no schema changes. Tests cover the main paths.

Issues / risks

1. Claimed provenance goes into an immutable ledger. source.service, channel, workspace and session come from the caller. claimed: true marks this, but ledger consumers should treat them as untrusted. ledgerLog also overwrites any existing metadata.execution, so check whether a caller already sets it.
2. Intent heuristic. Mutating methods default to execute, which fails safe. POST routes that only read are mislabeled execute, and GET routes with side effects (OAuth callbacks, sync triggers) are labeled read. Please inventory those routes, or document HTTP intent as best-effort.
3. normalizePath only handles UUIDs and numeric ids. Slugs, ChittyIDs, emails and codes go into capability raw. Use Hono's matched route pattern (c.req.routePath), read after next(), or finalize the capability there.
4. The default capability skips the 128-character limit. sanitizeValue is applied only in setExecutionOperation.
5. requestId is the traceparent trace-id. Requests in the same trace share it, so it is not unique per request. Use crypto.randomUUID() and keep traceparent separately.
6. Baggage duplicate keys are last-wins. Decide whether first-wins is preferable.
7. setExecutionOperation throws if the middleware is absent. Confirm /mcp is always mounted under protectedRoute.

Tests
Sanitization, scope isolation, intent inference and the MCP override are covered. Gaps:

• No integration test with createApp for the middleware ordering.
• ledgerLog with no execution context is not tested.
• No tests for duplicate or percent-encoded baggage.

Security
I found no authorization bypass. Tenant scope comes only from c.get('tenantId'), and a test pins that. Items 3 and 5 are the ones I would fix before merge.