Skip to content

fix(approvals): accept allow-always source metadata#3

Draft
iamlukethedev wants to merge 1 commit into
mainfrom
cursor/fix-approval-source-schema-297d
Draft

fix(approvals): accept allow-always source metadata#3
iamlukethedev wants to merge 1 commit into
mainfrom
cursor/fix-approval-source-schema-297d

Conversation

@iamlukethedev

Copy link
Copy Markdown
Owner

Summary

  • Problem: Telegram/native allow-always approvals already persist allowlist entries with source: "allow-always", but the gateway exec.approvals.set schema rejected that field as unexpected.
  • Why it matters: local approvals files and the gateway push path disagree on the allowlist contract, so gateway sync can fail even when the on-disk approvals file is valid for the runtime.
  • What changed: src/gateway/protocol/schema/exec-approvals.ts now accepts optional source: "allow-always" on allowlist entries, matching the existing persisted approvals type.
  • What did NOT change (scope boundary): no approval decision logic, persistence behavior, or Telegram/native reply handling changed in this PR; it only aligns the gateway protocol schema with existing stored data.
  • AI-assisted: Yes. I implemented the protocol fix and verified it locally with targeted tests and changed-scope checks.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: the durable approvals store type and normalization path already preserved source: "allow-always", but the gateway protocol schema for exec.approvals.set and exec.approvals.node.set did not allow that field.
  • Missing detection / guardrail: there was no validator regression test covering a real allowlist payload that included source: "allow-always".
  • Contributing context (if known): the issue report shows this surfaced through Telegram "Allow always", but the mismatch is fundamentally between persisted approvals data and the gateway wire contract.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: src/gateway/protocol/index.test.ts
  • Scenario the test should lock in: validateExecApprovalsSetParams and validateExecApprovalsNodeSetParams accept allowlist entries that include source: "allow-always".
  • Why this is the smallest reliable guardrail: the bug is a protocol/schema rejection, so validator tests hit the exact contract boundary that failed without needing a live Telegram or gateway sync flow.
  • Existing test that already covers this (if any): none.
  • If no new test is added, why not: N/A.

User-visible / Behavior Changes

  • Gateway approval set payloads may now include persisted allow-always source metadata without being rejected as invalid.

Diagram (if applicable)

Before:
[allowlist entry with source="allow-always"] -> [exec.approvals.set validator] -> [reject unexpected property]

After:
[allowlist entry with source="allow-always"] -> [exec.approvals.set validator] -> [accept payload]

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: Ubuntu Linux in Cursor Cloud
  • Runtime/container: Node 22 + pnpm workspace
  • Model/provider: N/A
  • Integration/channel (if any): Gateway exec approvals protocol validation
  • Relevant config (redacted): approvals payload with allowlist entry { pattern: "/usr/bin/curl", source: "allow-always" }

Steps

  1. Construct an approvals file payload containing an allowlist entry with source: "allow-always".
  2. Validate it through the gateway protocol path for exec.approvals.set or exec.approvals.node.set.
  3. Observe whether the validator accepts or rejects the payload.

Expected

  • The gateway protocol should accept allowlist entries that match the already-persisted approvals type, including source: "allow-always".

Actual

  • Before this fix, the schema rejected the payload because source was not part of ExecApprovalsAllowlistEntrySchema.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

  • Verified scenarios: ran pnpm test src/gateway/protocol/index.test.ts; ran pnpm check:changed.
  • Edge cases checked: both gateway set and node set validators accept allow-always source metadata; no unrelated schema fields were broadened.
  • What you did not verify: live Telegram approval flow or a real openclaw approvals set --gateway round-trip against a running gateway.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps:

Risks and Mitigations

  • Risk: this only fixes the schema mismatch slice of the issue; if another runtime path strips or mishandles source, that would need follow-up.
    • Mitigation: this PR is intentionally scoped to the concrete validator rejection, and the new tests lock the contract in place.
Open in Web Open in Cursor 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants