LearnNewsExamplesServices
Frontmatter
id10231
titlePermissionService.grantPermission must not lazy-stub missing targets
stateClosed
labels
bugaiarchitecture
assigneesneo-gemini-3-1-pro
createdAtApr 23, 2026, 2:01 PM
updatedAtApr 23, 2026, 3:47 PM
githubUrlhttps://github.com/neomjs/neo/issues/10231
authorneo-opus-4-7
commentsCount0
parentIssue10139
subIssues[]
subIssuesCompleted0
subIssuesTotal0
blockedBy[]
blocking[]
closedAtApr 23, 2026, 3:47 PM

PermissionService.grantPermission must not lazy-stub missing targets

Closedbugaiarchitecture
neo-opus-4-7
neo-opus-4-7 commented on Apr 23, 2026, 2:01 PM

Context

Same session as the upsertNode root-cause ticket (filed as sibling). This ticket closes the symptom-layer hazard that the upsertNode fix enables full elimination of; filed independently so each can land and be reviewed in isolation.

The Problem

ai/mcp/server/memory-core/services/PermissionService.mjs:65-68 fabricates a minimal AGENT:<x> stub if the target isn't in the in-memory cache when grantPermission is called:

if (!db.nodes.get(to)) {
    // It's technically fine if the target doesn't exist yet, but linkNodes might cull it if not found in SQLite.
    // Let's lazily ensure the node exists if we're granting permission to it.
    GraphService.upsertNode({ id: to, type: 'AGENT', name: to.replace('AGENT:', ''), properties: {} });
}

Two independent hazards compound here:

  1. In-memory check only (addressed by sibling upsertNode ticket at the upsertNode substrate layer): if to is a seeded identity ('AGENT:*', '@neo-opus-4-7', etc.) that exists in SQLite but isn't yet cache-loaded, the condition returns falsy and the stub is upserted. Upstream fix prevents metadata destruction but doesn't fix #2.
  2. Stub shape is architecturally wrong: even with correct cache behavior, the intent is wrong. Identity creation is a privileged operation via the seed script; it should NOT be an implicit side-effect of a permission grant. The lazy-stub pattern creates identity nodes with type: 'AGENT' and stripped metadata — neither AgentIdentity nor BroadcastSentinel. Any downstream consumer reading type-specific metadata gets garbage.

Empirical evidence: AGENT:* observed with type 'AGENT' (lazy-stub shape) instead of 'BroadcastSentinel' (seed shape from #10174). Attribution: grantPermission({to: 'AGENT:*', ...}) called before AGENT:* was in cache → stub upserted → SQLite overwritten.

The Architectural Reality

  • ai/mcp/server/memory-core/services/PermissionService.mjs:65-68 — lazy-stub path
  • Precedent for strict check: GraphService.linkNodes:179-185 queries SQLite directly and CULLS the edge if the target doesn't exist, logging a warning
  • Identity creation substrate: ai/scripts/seedAgentIdentities.mjs — the ONE place that should upsert AgentIdentity / BroadcastSentinel nodes
  • Existing specs (MailboxService.spec.mjs, PermissionService.spec.mjs post-#10229) already pre-seed their test fixtures; this ticket's stricter behavior aligns with the established spec pattern

The Fix

Replace the lazy-stub path with SQLite direct existence check, throwing a clear error for non-existent targets:

async grantPermission({ to, scope }) {
    const owner = RequestContextService.getAgentIdentityNodeId();
    if (!owner) throw new Error("Cannot grant permission: no agent identity context bound.");
    if (!to) throw new Error("Missing 'to' parameter.");
    if (!scope) throw new Error("Missing 'scope' parameter.");

    if (!this.validScopes.includes(scope)) {
        throw new Error(`Invalid scope. Must be one of: ${this.validScopes.join(', ')}`);
    }

    // Verify target exists in SQLite directly (not in-memory cache, per sibling ticket's
    // lazy-load root-cause fix). Identity creation is a privileged operation (seedAgentIdentities.mjs);
    // it should NOT be an implicit side-effect of a permission grant. Stubbing with type 'AGENT'
    // + stripped metadata destroys seed data and creates type-inconsistent nodes.
    // Pattern mirrors linkNodes:179-185 (FK-style existence guard).
    const verifyStmt = GraphService.db.storage.db.prepare('SELECT count(*) as count FROM Nodes WHERE id = ?');
    if (verifyStmt.get(to).count === 0) {
        throw new Error(`Cannot grant ${scope} to ${to}: target does not exist. Identity nodes must be pre-seeded via ai/scripts/seedAgentIdentities.mjs.`);
    }

    GraphService.linkNodes(to, owner, scope, 1.0);
    return { success: true, message: `Granted ${scope} to ${to}` };
}

Acceptance Criteria

  • grantPermission({to: '<seeded-identity>', ...}) succeeds AND preserves the existing node's type/properties (no stub overwrite)
  • grantPermission({to: '<non-existent-id>', ...}) throws a clear error citing the seed script as the correct path
  • Regression test: seed AGENT:* as BroadcastSentinel, call grantPermission({to: 'AGENT:*', scope: 'CAN_REPLY_TO'}), assert the node's type remains BroadcastSentinel post-grant
  • Regression test: call grantPermission({to: 'AGENT:phantom', ...}) on a fresh DB, assert throws with "must be pre-seeded" message
  • Existing PermissionService.spec.mjs test fixtures updated: any previously relying on the lazy-stub must now pre-seed nodes explicitly (symmetric with MailboxService.spec pattern)
  • Anchor & Echo inline comment at the new existence-check block

Out of Scope

  • Changes to revokePermission (edge-only deletion, no upsert hazard)
  • Changes to the AGENT:* sentinel semantics
  • Role-based addressing (role:<name>) and human addressing (human:<login>) — these are write-permissive by design; only the AGENT:/@-prefixed identity path has the stub hazard

Avoided Traps

  • "Just use linkNodes' cull-if-missing behavior": rejected. Silent culling obscures the "target is mis-typed" class of errors. Throwing surfaces the misuse loudly, aligning with the "surface, don't obscure" discipline from PR #10227.
  • "Create full AgentIdentity stub if missing": rejected. Identity metadata (githubLogin, modelFamily, accountType) is seed-provisioned; permission grant has no knowledge of these. Any stub would be wrong metadata.
  • "Wait for sibling upsertNode fix to eliminate the hazard at the substrate layer": rejected in part. The upsertNode fix eliminates the cold-cache corruption of existing nodes, but does NOT fix the architectural misuse of upsertNode-from-grantPermission-with-wrong-type. Both tickets needed.

Related

  • Root cause (substrate layer): sibling ticket "upsertNode must lazy-load from SQLite before cache decision"
  • Existing correct pattern: GraphService.linkNodes:179-185 (SQLite FK-style check)
  • Seed source of truth: ai/scripts/seedAgentIdentities.mjs
  • Parent epic: #10139 Mailbox A2A primitive

Origin Session ID: 24aa1fa1-9a22-498e-97f2-760c12e5a79d

Handoff Retrieval Hints

  • query_raw_memories(query="PermissionService grantPermission lazy stub AGENT type")
  • query_raw_memories(query="AgentIdentity wipe partial state overwrite")
tobiu closed this issue on Apr 23, 2026, 3:47 PM
tobiu referenced in commit 5631b48 - "fix(memory-core): replace lazy-stubbing in grantPermission with SQLite existence guard (#10231) (#10235) on Apr 23, 2026, 3:47 PM