Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/appkit/src/context/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,3 @@ export {
runInUserContext,
} from "./execution-context";
export { ServiceContext } from "./service-context";
export type { UserContext } from "./user-context";
57 changes: 8 additions & 49 deletions packages/appkit/src/core/appkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ import type {
} from "shared";
import { version as productVersion } from "../../package.json";
import { CacheManager } from "../cache";
import { runInUserContext, ServiceContext } from "../context";
import type { UserContext } from "../context/user-context";
import { ServiceContext } from "../context";
import {
isInternalTelemetryEnabled,
TelemetryReporter,
} from "../internal-telemetry";
import { createLogger } from "../logging/logger";
import { USER_CONTEXT_SYMBOL } from "../plugin/plugin";
import { ResourceRegistry, ResourceType } from "../registry";
import type { TelemetryConfig } from "../telemetry";
import { TelemetryManager } from "../telemetry";
Expand Down Expand Up @@ -134,39 +132,18 @@ export class AppKit<TPlugins extends InputPluginMap> {
}
}

/**
* Wraps all function properties in an exports object so they run
* inside the given user context (via AsyncLocalStorage).
* This ensures RoutingPool and other context-aware code sees the
* user identity even though the function was obtained outside the proxy.
*/
private wrapExportsInUserContext(
exports: Record<string, unknown>,
userContext: UserContext,
) {
for (const key in exports) {
if (!Object.hasOwn(exports, key)) continue;
const val = exports[key];
if (typeof val === "function") {
const fn = val as (...args: unknown[]) => unknown;
exports[key] = (...args: unknown[]) =>
runInUserContext(userContext, () => fn(...args));
} else if (AppKit.isPlainObject(val)) {
this.wrapExportsInUserContext(
val as Record<string, unknown>,
userContext,
);
}
}
}

/**
* Wraps a plugin's exports with an `asUser` method that returns
* a user-scoped version of the exports.
*
* When `exports()` returns a callable (function), it is returned as-is
* since the plugin manages its own `asUser` per-call (e.g. files plugin).
* When it returns a plain object, the standard `asUser` wrapper is added.
*
* The OBO-side wrapping lives inside `Plugin.asUser` — calling
* `plugin.asUser(req).exports()` returns exports whose functions already
* run inside the user's AsyncLocalStorage scope. AppKit only adapts the
* shape; it does not own the user-context concept.
*/
private wrapWithAsUser<T extends BasePlugin>(plugin: T) {
// If plugin doesn't implement exports(), return empty object
Expand All @@ -192,26 +169,8 @@ export class AppKit<TPlugins extends InputPluginMap> {
* Returns user-scoped exports where all methods execute with the
* user's Databricks credentials instead of the service principal.
*/
asUser: (req: import("express").Request) => {
const userPlugin = (plugin as any).asUser(req);
const userContext = (userPlugin as any)[
USER_CONTEXT_SYMBOL
] as UserContext;
const userExports = (plugin.exports?.() ?? {}) as Record<
string,
unknown
>;
// Wrap each export in runInUserContext instead of bind.
// bind() bypasses the Proxy get trap, so methods called via bind
// would not run inside the user's AsyncLocalStorage context.
if (userContext) {
this.wrapExportsInUserContext(userExports, userContext);
} else {
// Fallback for dev mode proxy (no userContext symbol)
this.bindExportMethods(userExports, userPlugin);
}
return userExports;
},
asUser: (req: import("express").Request) =>
(plugin as any).asUser(req).exports() as Record<string, unknown>,
};
}

Expand Down
127 changes: 79 additions & 48 deletions packages/appkit/src/plugin/plugin.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code Review: PR #385 — refactor(appkit): own asUser OBO wrapping in Plugin proxy

Context

PR #385 moves OBO-side exports() wrapping out of AppKit#wrapWithAsUser and into Plugin.asUser's proxy. The cross-file USER_CONTEXT_SYMBOL bridge is removed, and two inline Proxy literals (real OBO + dev fallback) collapse into a single _createAsUserProxy(wrapCall) factory. A 26-test file covering proxy mechanics is added.

Scope: 4 files changed in packages/appkit/ — context/index.ts, core/appkit.ts, plugin/plugin.ts, plugin/tests/asUser-proxy.test.ts (new)
Reviewers dispatched: correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, reliability, api-contract


Review Findings

P2 — Moderate

# File Issue Reviewer(s) Confidence Route
1 packages/appkit/src/plugin/plugin.ts:52 Duplicate isPlainObject implementation. Identical logic exists at plugin.ts:52 (module function) and appkit.ts:180 (static method). Two sources of truth for the same check — if logic needs to change, both must be updated independently. maintainability 0.95 manual -> human
2 packages/appkit/src/plugin/plugin.ts:65 wrapExportFunctions mutates input object in-place. Currently safe because exports() returns a fresh object per call, but fragile if a plugin ever caches the result. The docstring says "Mutates and returns" but the wrap naming suggests a non-mutating operation. maintainability 0.80 advisory -> human
3 packages/appkit/src/plugin/plugin.ts:75 No circular reference protection in wrapExportFunctions. Recursive descent into nested plain objects has no cycle detection. Extremely unlikely with real plugin exports (shallow structures, isPlainObject rejects arrays/class instances), but undocumented limitation. testing 0.70 advisory -> human

P3 — Low

# File Issue Reviewer(s) Confidence Route
4 packages/appkit/src/plugin/plugin.ts:70 Redundant Object.hasOwn() check. Object.keys() on line 69 already returns only own enumerable keys, so the hasOwn guard on line 70 can never be true — dead code. testing 0.95 safe_auto -> review-fixer

Pre-existing (not introduced by this PR)

# File Issue Reviewer(s) Confidence
packages/appkit/src/core/appkit.ts:172 (plugin as any).asUser(req).exports() cast loses type safety. Pre-existing pattern — the old code also used (plugin as any) with USER_CONTEXT_SYMBOL. kieran-typescript 0.92

Testing Gaps

  • No test for exports() returning non-plain, non-function values (Map, Set, Array as top-level return) — the fallthrough at plugin.ts:502 is uncovered
  • No test for deeply nested exports (5+ levels) to confirm recursion terminates cleanly
  • No integration test at AppKit layer for when plugin.exports() throws during wrapWithAsUser
  • wrapExportFunctions and isPlainObject have no isolated unit tests (only tested indirectly through proxy integration)

Coverage Notes

  • Suppressed: 0 findings below confidence threshold
  • Learnings: No docs/solutions/ directory exists — no institutional learnings to surface
  • Agent-Native: No gaps — this is an internal refactor with no new user-facing features
  • Schema Drift / Deployment: N/A (no migrations)
  • Failed reviewers: 0 of 9

Verdict: Ready to merge (with optional improvements)

The core refactoring is sound — consolidating two Proxy literals into _createAsUserProxy(wrapCall) is a clear improvement that eliminates the cross-file USER_CONTEXT_SYMBOL bridge. The 26-test file is thorough and well-organized, covering real OBO, dev fallback, exports interception, async propagation, concurrent user isolation, and boundary cases.

No P0 or P1 issues found in the PR's committed changes. The findings below are improvements, not blockers.


Improvement Plan

Fix 1: Remove redundant Object.hasOwn() check (P3)

File: packages/appkit/src/plugin/plugin.ts:70

Remove the dead if (!Object.hasOwn(exports, key)) continue; guard since Object.keys() on line 69 already returns only own enumerable keys.

Optional improvements (not blocking merge)

  1. Deduplicate isPlainObject — Extract to a shared utility (e.g. packages/appkit/src/utils.ts) or have plugin.ts import AppKit.isPlainObject from appkit.ts. Both files (plugin.ts:52 and appkit.ts:180) have identical implementations.

  2. Add test for non-plain-object top-level exports — Add a test case where exports() returns a Map or Array as the top-level value, verifying it's returned as-is without wrapping (the plugin.ts:502 fallthrough path).

  3. Add test for mixed async/sync exports — Verify that an exports object with both async and sync functions preserves their semantics when wrapped.


Verification

After making changes:

pnpm build && pnpm test && pnpm check:fix && pnpm -r typecheck

Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,7 @@ import type {
} from "shared";
import { AppManager } from "../app";
import { CacheManager } from "../cache";
import {
getCurrentUserId,
runInUserContext,
ServiceContext,
type UserContext,
} from "../context";
import { getCurrentUserId, runInUserContext, ServiceContext } from "../context";
import type { PluginContext } from "../core/plugin-context";
import { AppKitError, AuthenticationError } from "../errors";
import { createLogger } from "../logging/logger";
Expand All @@ -43,19 +38,46 @@ import type {

const logger = createLogger("plugin");

/**
* Symbol used to expose the UserContext from an asUser() proxy.
* Allows wrapWithAsUser in appkit.ts to retrieve the context and
* wrap export methods in runInUserContext().
*/
export const USER_CONTEXT_SYMBOL = Symbol("appkit.userContext");

/**
* OTel context key for marking OBO dev mode fallback.
* Set when asUser() is called in development mode without a user token.
*/
const DEV_OBO_FALLBACK_KEY = createContextKey("appkit.devOboFallback");

/**
* Returns true if `value` is a plain object literal (not an array, Date,
* class instance, etc.). Used to decide whether to recurse into nested
* export shapes when wrapping functions.
*/
function isPlainObject(value: unknown): value is Record<string, unknown> {
if (typeof value !== "object" || value === null) return false;
const proto = Object.getPrototypeOf(value);
return proto === Object.prototype || proto === null;
}

/**
* Recursively replaces every function in `exports` with `wrap(fn)`,
* walking into nested plain objects. Mutates and returns `exports`.
*
* Used by the asUser proxy to make the user context follow function
* references that escape the proxy via `exports()`.
*/
function wrapExportFunctions(
exports: Record<string, unknown>,
wrap: (fn: (...a: unknown[]) => unknown) => (...a: unknown[]) => unknown,
): Record<string, unknown> {
for (const key of Object.keys(exports)) {
if (!Object.hasOwn(exports, key)) continue;
const val = exports[key];
if (typeof val === "function") {
exports[key] = wrap(val as (...a: unknown[]) => unknown);
} else if (isPlainObject(val)) {
wrapExportFunctions(val as Record<string, unknown>, wrap);
}
}
return exports;
}

/**
* Returns true if the current execution is an OBO dev mode fallback
* (asUser() was called but fell back to service principal due to missing token).
Expand Down Expand Up @@ -403,30 +425,18 @@ export abstract class Plugin<
const userEmail = req.header("x-forwarded-email");
const isDev = process.env.NODE_ENV === "development";

// In local development, skip user impersonation
// since there's no user token available
// In local development, skip user impersonation since there's no user
// token available. Mark execution as OBO dev fallback via OTel context
// so telemetry can distinguish intended OBO calls from regular SP calls.
if (!token && isDev) {
logger.warn(
"asUser() called without user token in development mode. Skipping user impersonation.",
);

// Return a proxy that marks execution as OBO dev fallback via OTel context,
// so telemetry spans can distinguish intended OBO calls from regular SP calls
return new Proxy(this, {
get: (target, prop, receiver) => {
const value = Reflect.get(target, prop, receiver);
if (typeof value !== "function") return value;
if (typeof prop === "string" && EXCLUDED_FROM_PROXY.has(prop))
return value;

return (...args: unknown[]) => {
const ctx = otelContext
.active()
.setValue(DEV_OBO_FALLBACK_KEY, true);
return otelContext.with(ctx, () => value.apply(target, args));
};
},
}) as this;
return this._createAsUserProxy((fn) => (...args) => {
const ctx = otelContext.active().setValue(DEV_OBO_FALLBACK_KEY, true);
return otelContext.with(ctx, () => fn(...args));
});
}

if (!token) {
Expand All @@ -446,34 +456,55 @@ export abstract class Plugin<
userEmail ?? undefined,
);

// Return a proxy that wraps method calls in user context
return this._createUserContextProxy(userContext);
return this._createAsUserProxy(
(fn) =>
(...args) =>
runInUserContext(userContext, () => fn(...args)),
);
}

/**
* Creates a proxy that wraps method calls in a user context.
* This allows all plugin methods to automatically use the user's
* Databricks credentials.
* Creates a proxy of `this` where every method call — and every function
* in the result of `exports()` — runs inside `wrapCall`.
*
* `wrapCall` decides the per-call scope. Two strategies are used today:
* - real OBO: fn => (...args) => runInUserContext(userContext, () => fn(...args))
* - dev fallback: fn => (...args) => otelContext.with(DEV_OBO_FALLBACK_KEY=true, () => fn(...args))
*
* `exports` is intercepted because methods captured in the returned
* exports object never re-enter the proxy's `get` trap. Wrapping them
* here is the only way to make the user context follow function
* references back out of the plugin.
*/
private _createUserContextProxy(userContext: UserContext): this {
private _createAsUserProxy(
wrapCall: (
fn: (...a: unknown[]) => unknown,
) => (...a: unknown[]) => unknown,
): this {
return new Proxy(this, {
get: (target, prop, receiver) => {
// Expose userContext via symbol so wrapWithAsUser can wrap exports
if (prop === USER_CONTEXT_SYMBOL) return userContext;

const value = Reflect.get(target, prop, receiver);

if (typeof value !== "function") {
if (typeof value !== "function") return value;
if (typeof prop === "string" && EXCLUDED_FROM_PROXY.has(prop))
return value;
}

if (typeof prop === "string" && EXCLUDED_FROM_PROXY.has(prop)) {
return value;
if (prop === "exports") {
return () => {
const raw = (value as () => unknown).call(target);
if (raw == null) return {};
// Callable exports (e.g. files, jobs) manage per-call asUser
// themselves; leave them untouched.
if (typeof raw === "function") return raw;
if (isPlainObject(raw)) {
return wrapExportFunctions(raw, wrapCall);
}
return raw;
};
}

return (...args: unknown[]) => {
return runInUserContext(userContext, () => value.apply(target, args));
};
const fn = (value as (...a: unknown[]) => unknown).bind(target);
return wrapCall(fn);
},
}) as this;
}
Expand Down
Loading
Loading