🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

const candidates: Record<string, string[]> = {};
...
for (const [providerId, keys] of Object.entries(plugin.providerAuthEnvVars) ...) {
  appendUniqueEnvVarCandidates(candidates, providerId, keys);
}
...
const bucket = (target[normalizedProviderId] ??= []);

const candidates: Record<string, string[]> = Object.create(null);

function isSafeKey(k: string): boolean {
  return k !== "__proto__" && k !== "prototype" && k !== "constructor";
}

function appendUniqueEnvVarCandidates(
  target: Record<string, string[]>,
  providerId: string,
  keys: readonly string[],
) {
  const normalizedProviderId = providerId.trim();
  if (!normalizedProviderId || !isSafeKey(normalizedProviderId) || keys.length === 0) return;
  const bucket = (target[normalizedProviderId] ??= []);
  ...
}

Analyzed PR: #68130 at commit 3cae1ff

_{Last updated on: 2026-04-17T13:48:00Z}

Greptile Summary

This PR bundles three independent fixes: (1) reduces the dreaming narrative cleanup log from warn to debug when deleteSession fails due to missing operator.admin scope (expected for the cron context); (2) guards the Ollama think parameter injection behind isReasoningModelHeuristic so non-reasoning models like qwen2.5 don't receive a param that causes a 400 error, expanding the heuristic regex to cover qwen3 and qwq; and (3) changes PROVIDER_ENV_API_KEY_CANDIDATES in model-auth-env-vars.ts to delegate to the existing lazy Proxy rather than eagerly resolving at import time.

• The lazy-loading test in model-auth-env-vars.test.ts has a misplaced assertion: expect(loadPluginManifestRegistry).not.toHaveBeenCalled() runs synchronously before the dynamic import() resolves, so it cannot catch an eager-loading regression. The assertion should be moved to after await modPromise.
• The scope-error check in dreaming-narrative.ts uses errMsg.includes("missing scope: operator.admin") — consider preferring a typed error field if one is available from the auth layer.

Confidence Score: 5/5

Safe to merge; all findings are non-blocking style/quality suggestions.

Both inline findings are P2: the lazy-loading test has a misplaced assertion (doesn't block functionality, only weakens the regression guard), and the string-based scope check is fragile but the worst-case degradation is higher log verbosity. The implementation changes themselves are correct.

src/agents/model-auth-env-vars.test.ts — the "is lazy-loaded" test assertion needs to be repositioned after await modPromise to actually validate the lazy-loading property.

This is a comment left during a code review.
Path: src/agents/model-auth-env-vars.test.ts
Line: 42

Comment:
**Lazy-load assertion runs before module initializes**

`import()` always resolves asynchronously (Promise microtask), so the module's top-level code hasn't executed yet when this synchronous assertion runs. This means the test passes even if `model-auth-env-vars.ts` eagerly called `loadPluginManifestRegistry` at import time — it wouldn't be caught because the module hasn't been evaluated by the time this check executes.

Move the "not called" assertion to after the `await modPromise` to actually verify the lazy-loading property:

```suggestion
    const mod = await modPromise;

    // Module is now fully initialized; registry should NOT have been called yet
    expect(loadPluginManifestRegistry).not.toHaveBeenCalled();

    // Now access the lazy export
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 938

Comment:
**Fragile string-based scope detection**

Matching on `errMsg.includes("missing scope: operator.admin")` couples the suppression logic to the exact error message format. If the auth layer ever changes that string (e.g. to `"scope missing: operator.admin"` or adds localization), this silently degrades back to the warn-level noise the PR is trying to eliminate.

If the error type that `deleteSession` throws has a structured code or scope field (e.g. `err.code`, `err.scope`, or an `isScopeError()` helper), prefer checking that instead. If no typed path exists today, a narrow inline comment explaining the message format's stability helps future readers understand the coupling.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(memory-core): reduce log noise for n..." | Re-trigger Greptile}

Related work from PRtags group ultimate-chow-4ei0

Title: Open PR duplicate: [Bug]: memory-core: narrative session cleanup fails with "missing scope: operator.admin"

* This PR

Closing this as duplicate or superseded after Codex automated review.

Close #68130 as duplicate/superseded. It targets the same memory-core missing scope: operator.admin narrative cleanup warning already grouped with #68020, #68087, #68312, and #68681; #68020 is the canonical maintainer PR for the remaining focused cleanup policy. #68130 is also not a clean landing vehicle because its memory-core patch calls logger.debug outside the current local logger contract and the branch bundles unrelated Ollama/model-auth changes.

Best possible solution:

Close #68130 as a duplicate/superseded contributor PR. Keep the remaining cleanup-warning policy on the canonical maintainer item #68020 or a new focused PR that preserves the local logger contract, keeps genuine cleanup failures visible, and splits unrelated Ollama/model-auth fixes into separate reviewed PRs.

What I checked:

• Duplicate group: Provided PRtags context groups fix: reduce log level for narrative session cleanup scope failures #68130 with duplicate memory-core cleanup PRs fix(memory-core): silence expected operator.admin scope miss in dreaming cleanup #68020, fix(memory-core): downgrade cleanup warning to debug when missing operator.admin scope #68087, fix(memory-core): downgrade narrative cleanup WARN to debug for missing-scope errors #68312, and fix(memory-core): suppress expected dreaming cleanup scope warnings #68681. fix(memory-core): silence expected operator.admin scope miss in dreaming cleanup #68020 is open, maintainer-authored, maintainer-labeled, and describes the same subagent.deleteSession() / missing scope: operator.admin warning path with focused tests.
• Current cleanup call still warns on all delete failures: generateAndAppendDreamNarrative() still calls params.subagent.deleteSession({ sessionKey }) in finally and logs any cleanup error through params.logger.warn(...), which is the exact surface this PR tries to change. (extensions/memory-core/src/dreaming-narrative.ts:923, d54d2d6b9b8a)
• Gateway fallback scope invariant remains: Current tests explicitly preserve that fallback plugin subagent sessions.delete does not mint operator.admin and rejects with missing scope: operator.admin, matching the root cause described by fix(memory-core): silence expected operator.admin scope miss in dreaming cleanup #68020 and fix: reduce log level for narrative session cleanup scope failures #68130. (src/gateway/server-plugins.test.ts:822, d54d2d6b9b8a)
• PR patch conflicts with logger contract: The local Logger type in current main only requires info, warn, and error; fix: reduce log level for narrative session cleanup scope failures #68130's provided diff adds params.logger.debug(...), matching the unresolved P1 review concern that minimal loggers would throw in cleanup. (extensions/memory-core/src/dreaming-narrative.ts:55, d54d2d6b9b8a)
• Branch scope is dirty: Provided PR file context shows fix: reduce log level for narrative session cleanup scope failures #68130 changes extensions/ollama/* and src/agents/model-auth-env-vars* in addition to extensions/memory-core/src/dreaming-narrative.ts, so it bundles unrelated Ollama thinking and model-auth env-var work into a memory-core log-level fix.

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a.