Skip to content

fix: 修复首次拒绝扫描后仍扫描及撤销竞态(1.2.7) - #755

Merged
origami-chen merged 3 commits into
MemTensor:v1.2.7from
Orsted290:fix-onboarding-deny-scan-v127
Oct 29, 2026
Merged

origami-chen merged 3 commits into
MemTensor:v1.2.7from
Orsted290:fix-onboarding-deny-scan-v127

Conversation

@Orsted290

@Orsted290 Orsted290 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

最新冲突解决与架构复核(2026-10-29)

合入 v1.2.7@03f915d6,提交 78463685,当前净差异 28 个文件。保持普通 merge 历史,未合并 PR。

  • 扫描唯一执行者仍为 Memory。 未恢复 backend 的 scanProcess、fork 子进程、scanAll、活动任务/存储清理代码,删除旧子进程专用测试。后端原有 scan bridge 保持与基线一致,仅转发 Memory API 和展示状态。
  • 拒绝流程为:原子保存权限/开关 → 后端等待自动接入结束 → Memory reload-config 撤销并等待扫描/自动任务结束 → 更新引导 DB。去掉后端额外 cancel 请求,避免新链路重复取消。后端 API 保留持久化权限拦截,拒绝发布后立即禁止新扫描。
  • Memory 保留新基线 expectedExecution/expectedVersion、cancelled 状态、暂停恢复、清空数据及写入准入保护;并发取消共享完成等待,撤销不提前确认。HTTP 回归已迁移到真实 Memory executor,覆盖活动/暂停后拒绝、保存失败与重启恢复。
  • 本轮复核完整净差异及 UI → 配置 → HTTP → Memory 执行/恢复入口,未发现旧扫描执行链被重新引入。撤销不删除已有记忆或回滚已完成的插件安装。

本轮验证及限制

  • typecheck、lint、diff check 通过。
  • Memory 全量以 --maxWorkers=2 运行:2,661 通过 / 2 跳过;默认高并发下两个队列用例超时,单文件 14 项及降低并发全量复跑均通过。
  • 后端扫描桥接、授权、自动接入、HTTP 集成:48 项通过;实际后端拒绝后重启恢复:1 项通过。重启测试已隔离新基线的远程插件目录查询。
  • 完整工作区扫描中:桌面 1,313 通过 / 126 跳过,TUI 5 项通过;前端扫描授权/快速开关相关用例通过。
  • 全仓并非全绿:后端仍遇到 Office release/catalog 缺失、插件预览断言失败和远程目录查询导致的启动用例超时;前端仍有英文覆盖、工具分组展开、录音归档三项失败。上述失败对应的产品逻辑与原有用例未在本 PR 中改动;未将其认定为已修复,也未绕过断言或修改工作流。
  • 7846368 尚未重新打包或执行桌面 GUI。 以下 GUI 记录仅对应 269fe3a,不能用作重构后提交的 GUI 结论。推送后 CI/外部审查需重新运行,旧 head 结果不代表本次结果。

问题与修复

移植 #732 到 1.2.7。原逻辑先推进引导,再分别保存权限和自动扫描开关;保存失败或立即退出可能留下旧开关,后台仍扫描。现在原子保存授权和开关,成功后推进;失败提示并可重试,重启恢复已保存授权。快速开关写入串行化,避免旧响应覆盖最终选择。

早期提交 269fe3a 曾为旧基线补齐桌面扫描子进程取消链路。最新 v1.2.7 已统一由 Memory 执行扫描,本轮合并按新架构重新适配,未保留旧后端扫描器;详见顶部最新验证。原 #732 保留。

历史自动验证与审查(269fe3aa)

  • 全量 npm test、lint、typecheck 通过。Memory 1444 通过/1 跳过,后端 1120 通过,前端 2627 通过,桌面 1296 通过/126 跳过,其余根目录及 workspace 测试通过。
  • 覆盖保存失败/重试、reload/DB 失败、重启恢复、快速开启再关闭、活动/暂停写入排空、并发取消、子进程 exit/close、迟到消息和撤销期间自动接入。
  • 完整 base..head 调用链复核未再发现阻断项。撤销等待已开始的接入结束,不等于回滚已完成安装或删除已有记忆。

隔离桌面验收(269fe3aa,2026-10-27)

完整 macOS arm64 unsigned DMG 构建成功,使用本机同型号模型缓存。合成 HOME/app-data/会话与固定响应模型,不使用个人会话。

场景 实测结果
首次不允许 1,000 个合成会话;进入引导/跨 Agent 页面,三个开关 off;扫描 inactive,记忆/采集/认领计数 0/0/0。
退出重启后不扫描 完全结束隔离进程后重启;两次观测间隔 61 秒,始终 inactive,计数保持 0/0/0,权限 none。
扫描中返回再拒绝 18,000 个合成会话及合成插件冲突;先确认 Codex scan active=true,再返回授权页、选择不允许。子进程退出,最终三个开关 off;拒绝后观察 101 秒,inactive,计数稳定 1/0/1,无新增采集。

此前“卡启动页”包含旧自动化窗口观察状态的误判:同一次首次启动、不重启应用,重新绑定窗口后即显示授权页;本轮未修改产品启动代码。GUI 观察中也重新校验实际窗口,未只用后台状态替代界面验收。

取消在 scan 阶段触发,在途写入排空由 HTTP/执行器回归另行覆盖。保存失败恢复重试及快速开关 GUI 已补验如下;不将旧 v1.2.6 GUI 结果冒充本轮结果。未执行真实 Hook/插件安装。

追加隔离 GUI 验收:保存重试与快速开关

同一修复提交 269fe3a、全新合成配置及 1,000 个合成会话;本次未修改源码或新增提交。

  • 保存失败后恢复重试:通过。 授权页就绪后将测试配置目录设为 0555;点击“不允许”显示“保存扫描设置失败,请重试。”,停留原页,DB 仍 unset/引导未完成,扫描 inactive,计数 0/0/0。恢复为 0700 后,不重启应用,在原页再次点击“不允许”成功进入跨 Agent 页面,权限 none、三个开关 false,仍无扫描或新增。
  • 快速开启再关闭:通过。 启动扫描开关两次连续 GUI 点击共 54ms,定时扫描共 33ms;只读配置采样均实际记录 false→true→false。界面最终 off,离开到概览再返回仍 off。两次后续观测相隔 75 秒,扫描 inactive,记忆/采集/认领计数保持 0/0/0。
  • 主动开启扫描开关会授予 scan_only,关闭自动开关不会撤销手动扫描授权;最终 scan_only + 三开关 false 符合此语义。自动接入始终关闭,未安装 Hook/插件。桌面短时观测不替代自动测试中的可控迟到响应验证。

历史 CI 结论(269fe3aa)

269fe3a 当时观测:Linux build、x64/arm64 install-smoke、Windows validation 全通过;upload 按条件跳过,GitHub 显示 All checks have passed。

旧 head 的唯一失败为 云效同步:找不到配置的项目 memmy(1832b179386e24414d3891e244)。pull_request_target 使用受信任 main 的同步脚本;需管理员核实项目 ID/名称或令牌项目权限。新 head 未重新触发该同步,不代表云效配置已经修复;未修改或绕过工作流。

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #755
Task: c5b72680b2806df8
Base: v1.2.7
Head: fix-onboarding-deny-scan-v127

🔍 OpenCodeReview found 29 issue(s) in this PR.

⚠️ Review warnings (6)

  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files

1. App/backend/src/services/index.ts (L391-L395)

This duplicates the exact permission-evaluation logic that was added at the same time to AppConfigService.isScanAllowed() (config permission with DB fallback, same scan_only/scan_and_write_skill check). server.ts already wires the agent-sources route through appConfig.isScanAllowed(), so the two copies can silently drift (e.g. if one later adds scanPermissionOverride or a new permission value). Since appConfig is created in the same returned object literal, hoist it into a const appConfig = createAppConfigService({...}) above the return and reuse the single implementation.

💡 Suggested Change

Before:

      isScanAllowed: () => {
        const permission = options.scanPreferencesStore?.getScanPermission()
          ?? options.appStateStore.repositories.bootstrap.getOnboardingState().scanPermission;
        return permission === "scan_only" || permission === "scan_and_write_skill";
      },

After:

      // Reuses the single permission evaluation in AppConfigService (hoist `appConfig` above the return).
      isScanAllowed: () => appConfig.isScanAllowed?.() ?? true,

2. App/backend/src/index.ts (L130-L131)

This startup recovery mutates the durable, installation-scoped consent decision based purely on the config file, and it does so silently. If the config is ever stale or carries a value getScanPermission() cannot recognise (it coerces anything unknown to "unset"), a previously granted permission is overwritten with no trace, which makes support/diagnosis very hard. Please log the transition and read the stored value once instead of querying getOnboardingState() twice.

💡 Suggested Change

Before:

    const scanPermission = scanPreferencesStore.getScanPermission();
    if (scanPermission !== undefined && scanPermission !== appStateStore.repositories.bootstrap.getOnboardingState().scanPermission) {

After:

    const scanPermission = scanPreferencesStore.getScanPermission();
    const storedScanPermission = appStateStore.repositories.bootstrap.getOnboardingState().scanPermission;
    if (scanPermission !== undefined && scanPermission !== storedScanPermission) {
      console.warn(`Recovered scan permission from config after an interrupted save: ${storedScanPermission} -> ${scanPermission}`);

3. App/backend/src/infrastructure/memmy-config/agent-access.ts (L50-L52)

Two problems in this branch:

  1. Nested ternary (explicitly disallowed by the review checklist) that encodes several independent business rules; it is hard to verify and easy to break.
  2. Only none/unset force autoInjectSkill: false. When the permission is downgraded to scan_only without enableAutomation (e.g. PATCH /api/app/onboarding { scanPermission: "scan_only" }, which is exactly what memory-sources-page's ensureScanPermission sends), a previously persisted autoInjectSkill: true is preserved. Memory's automation path (runtime.ts → canInstall) only checks agentAccess.autoInjectSkill, so skills could keep being installed although the user can no longer write skills. autoInjectSkill should be false whenever the permission is not scan_and_write_skill.
💡 Suggested Change

Before:

            ...(permission === "none" || permission === "unset"
              ? { autoScanKnownAgents: false, watchFileChanges: false, autoInjectSkill: false }
              : enableAutomation ? {

After:

            ...automationSwitches(permission, enableAutomation)

4. App/backend/src/infrastructure/memmy-config/agent-access.ts (L61-L63)

getScanPermission() re-reads and re-parses the whole YAML on every call, and it is wired into isScanAllowed(), which is invoked once per source in the auto-inject loops (agent-source-auto-inject-service) and per scan request — synchronous blocking I/O in a hot predicate. It also duplicates the read/parse logic that readMemoryScanPreferences already has. Consider a shared helper (or caching the parsed access record and invalidating on mutateRuntimeConfig) so the permission read does not re-parse the file each time.


5. App/backend/src/services/app-config-service.ts (L36-L37)

isScanAllowed and setScanPermissionReconciler are declared optional on the public interface even though createAppConfigService always implements them. Callers therefore need a fallback, and the one in server.ts is options.services.appConfig?.isScanAllowed?.() ?? true — i.e. scanning silently stays allowed if the method is ever missing. Making these members required (and requiring the reconciler to be set) would keep the enforcement fail-closed. Note also that the permission === "scan_only" || permission === "scan_and_write_skill" predicate is now duplicated in services/index.ts and could be exported once from agent-access.ts.


6. App/backend/src/adapters/inbound/local-api/routes/agent-sources.ts (L81)

The new config-file-based gate (isScanAllowed) is only applied to POST /api/agent-sources/scan. Other endpoints that also read/import local agent history remain guarded solely by permissionManager.canScanAgentSource, which reads bootstrapRepository.getOnboardingState().scanPermission (plus a process-local override) and can lag behind the Memory config file that appConfig.isScanAllowed() reads — e.g. POST /api/agent-sources/:sourceId/managed/sync at line ~117 still succeeds after a revocation published only to the config file. Consider applying the same isScanAllowed?.() === false check to syncManaged (and any other scan-triggering entry point such as /auto-inject/run) so authorization is enforced consistently.


7. App/backend/src/services/agent-source-auto-inject-service.ts (L64)

isCurrent() is false for two different causes (scan permission revoked vs. an invalidated older generation), but both are reported as reason: "auto_inject_disabled". For a run aborted by revocation the caller/UI will conclude that automatic integration was disabled, which is misleading. Consider distinguishing the reason (e.g. scan_not_permitted / superseded) so the returned result is actionable.


8. App/backend/src/services/agent-source-auto-inject-service.ts (L44)

This is the concurrency core of the revocation drain (generation invalidates the in-flight run, completion waits for it to settle) but the invariant is not documented. Please add a short comment explaining: (1) bumping generation makes every isCurrent()/beforeInstall assertion of the running pass throw so it stops installing, (2) awaiting completion is what lets the caller acknowledge the revocation only after in-flight installs settle, and (3) completion is only assigned while running is true, so awaiting it when no pass is active is a no-op. Also note that a pass aborted this way still returns skipped: false with a partial installed list, which callers cannot distinguish from a completed pass.


9. App/backend/src/services/agent-source-service.ts (L250)

Two issues here (same pattern at line ~322 for installPlugin):

  1. The ternary is redundant — passing beforeInstall through is already undefined-safe, so both branches are identical in effect.
  2. beforeInstall is invoked twice per install: once explicitly here and once inside skillDistributionService.install/installPlugin. For the current assertion callbacks that is harmless, but the parameter contract is surprising for any side-effecting callback. Pick one place to invoke it (the distribution service already runs it right before the write) and pass it through unconditionally.
💡 Suggested Change

Before:

        await (beforeInstall ? options.skillDistributionService.install(sourceId, beforeInstall) : options.skillDistributionService.install(sourceId));

After:

        await options.skillDistributionService.install(sourceId, beforeInstall);

10. App/backend/src/services/agent-source-auto-inject-service.ts (L118)

If autoInjectSkill is switched off while a run is in progress, assertInstallAllowed() throws for the current source and — because the loop-top guard only checks isCurrent() (generation/scan permission), not the preference — for every remaining source as well. The run then returns skipped: false with one "Automatic integration disabled" failure per source instead of stopping. Suggested: check the preference at the top of the loop and break (or return the existing auto_inject_disabled skipped result) so a preference change does not look like N install failures to the caller/UI.

💡 Suggested Change

Before:

            assertInstallAllowed();

After:

            if (!options.getScanPreferences().autoInjectSkill) {
              return { ok: true, skipped: true, reason: "auto_inject_disabled", installed, failed };
            }
            assertInstallAllowed();

11. App/backend/src/adapters/inbound/local-api/server.ts (L112)

This gate defaults to permissive (?? true) when appConfig/isScanAllowed is absent. Since the gate exists to enforce a user's privacy revocation, a missing/unwired implementation silently allows scanning (fail-open) instead of denying. Prefer failing closed (e.g. === true) or making the dependency required, so a wiring regression cannot bypass the revocation.


12. App/backend/src/services/skill-distribution-service.ts (L24)

The new beforeInstall hook has non-obvious semantics that are now part of a shared service contract: it is invoked immediately before the mutating call and is expected to throw to abort the install (in install() this happens after the resolveRootDirectory() await, i.e. after several awaits have already run). Please document that contract on the interface members (and/or rename to something like assertCanInstall) so implementers of this interface know the hook may be called more than once and that throwing is the cancellation mechanism.


13. App/backend/src/services/app-config-service.ts (L161)

enableAutomation is derived from input.currentStep === "scan_permission_required", i.e. from an unrelated walkthrough-progress field via a hardcoded magic string. This is an implicit contract no caller documents: any PATCH /api/app/onboarding that carries scanPermission while the guide happens to be parked on scan_permission_required will force autoScanKnownAgents/watchFileChanges (and autoInjectSkill for scan_and_write_skill) back to true, silently reverting an explicit opt-out the user made in the Viewer. Please make the intent explicit (a dedicated input flag, or at minimum a named constant + comment) so automation is only enabled by the onboarding grant action itself.


14. App/backend/src/services/app-config-service.ts (L107-L111)

The "permission grants scanning" rule now exists in three places: here, the inline isScanAllowed passed to createAgentSourceAutoInjectService in services/index.ts, and permission-manager.canScanAgentSource. Three copies of the same business predicate will drift (e.g. when a new permission level is added). Extract one shared helper (e.g. isScanPermissionGranted(permission)) and reuse it in all three call sites.


15. Memory/src/agent-source/runtime.ts (L670-L672)

The permission probe polls every 16 ms, and canScan() calls loadMemmyConfig(), which does a synchronous readFileSync + full YAML parse of config.yaml on every call (no caching). During a scan this means up to ~60 blocking full-file config reads per second in the scan hot loop, competing with the actual ingestion work. Consider a coarser interval (e.g. 500-1000 ms) or caching the parsed permission keyed on the file mtime. Also note canScan() is not wrapped in try/catch here, so a hand-edited/unparsable config throws out of waitWhilePaused and fails the whole scan instead of being treated as a denied permission.


16. Memory/src/agent-source/runtime.ts (L263-L270)

Two concerns with this reconcile:

  1. It has no upper bound: cancelScan() waits for the scan loop to observe the abort plus sqlite cleanup, and await automation waits for any in-flight auto-inject install (which can shell out to external tooling). Because this is awaited synchronously from POST /api/v1/admin/reload-config (Memory/src/server/http.ts:597), a slow install makes every config reload block, and the desktop client will surface a memory layer timeout even though the permission was already persisted. Consider bounding the wait (Promise.race with a timeout) or only awaiting cancellation of the scan, not the whole automation.

  2. It is deny-only. When the desktop later re-grants permission (none -> scan_only), nothing re-kicks automation; recovery depends on the next scheduleAutomation tick, which is SCHEDULED_SCAN_INTERVAL_MS (60 min). Consider scheduling an immediate runAutomation when permission becomes authorized so a re-grant takes effect without a relaunch/hour-long delay.


17. Memory/src/server/http.ts (L597)

reconcileScanPermission() is awaited before service.reloadConfig(...), so any rejection from the reconcile path (e.g. cancelScan() failing on sqlite rm — a real case on Windows when a handle is still open, or an install that throws) aborts the whole request and the config reload never happens, even though reloading the config is the operation the caller actually asked for. Suggest isolating the failure (try/catch + log) and/or running the reconcile after reloadConfig() so a reconciliation problem cannot turn a successful reload into a 5xx.


18. Memory/src/config/index.ts (L844-L845)

This is a nested ternary (project rules forbid it) and it inlines the "scan_only" / "scan_and_write_skill" / "none" / "unset" literals that are repeated in AgentAccessConfig and again in canScan() (Memory/src/agent-source/runtime.ts:258-261). The two locations can drift (e.g. if a new value such as "write_only" is added). Prefer a module-level helper with an explicit allow-list, e.g.:

const SCAN_PERMISSIONS = new Set<NonNullable<AgentAccessConfig["scanPermission"]>>(["unset", "none", "scan_only", "scan_and_write_skill"]);
function normalizeScanPermission(value: unknown) {
  return SCAN_PERMISSIONS.has(value as never) ? value as AgentAccessConfig["scanPermission"] : "unset";
}

and reuse it in canScan().


19. Memory/src/agent-source/runtime.ts (L706-L707)

This guard is duplicated three times inside mutateConnection with the same shape, and it returns bare undefined from a Promise<unknown>. For the callers in runAutomation (which then re-read state and may trigger the autoInjectSuppressed rollback at lines 782-786 / 803-806) a silent skip is indistinguishable from a completed mutation, so a revoked grant can leave the caller believing the install ran. Consider a single assertAutomaticInstallAllowed?.() that throws a typed/skip error (already caught and logged by runAutomation) or returning an explicit { skipped: true } result, instead of repeating the guard at three call sites.


20. App/frontend/desktop/src/api/scan-preferences-writer.ts (L51-L55)

Partial-failure leaves the server inconsistent: updateOnboarding can successfully escalate the persisted permission (e.g. scan_and_write_skill), and if the following updateScanPreferences fails, only the local preferences are rolled back via onConfirmed(confirmed). The server then keeps the escalated permission while the UI shows the previous (disabled) toggles, and nothing reconciles them. Please consider re-reading/reverting the onboarding permission when the preferences write fails (or perform the permission write last / after the preferences write succeeds).


21. App/frontend/desktop/src/api/scan-preferences-writer.ts (L41-L43)

This nested ternary relies on ?: right-associativity for the && chain, which is easy to misread and violates the project rule against nested ternaries. Extract the permission mapping into a small named helper (e.g. resolveGrant(patch, permission)) so the escalation rules are explicit and testable.

💡 Suggested Change

Before:

          const grant = input.patch.autoInjectSkill === true ? "scan_and_write_skill"
            : (input.patch.autoScanKnownAgents === true || input.patch.watchFileChanges === true)
              && permission !== "scan_only" && permission !== "scan_and_write_skill" ? "scan_only" : undefined;

After:

          const grant = resolveGrant(input.patch, permission);

22. App/frontend/desktop/src/api/scan-preferences-writer.ts (L20-L21)

confirmed and permission are declared without initializers and depend on an implicit invariant: the first write() for a client must be enqueued while pending === 0, otherwise both stay undefined for that queue. This is not enforced or documented. Please initialize them explicitly and/or add a comment describing the invariant (and the pending === 0 snapshot semantics) so future changes don't break it.

💡 Suggested Change

Before:

  let confirmed: ScanPreferences;
  let permission: ScanPermission;

After:

  // Set on the first enqueued write (pending === 0) and updated as writes settle.
  let confirmed: ScanPreferences | undefined;
  let permission: ScanPermission | undefined;

23. App/frontend/desktop/src/api/scan-preferences-writer.ts (L39)

If any callback invoked inside the queued task throws (e.g. dispatch/onConfirmed/onError throwing), the async callback rejects, tail becomes a rejected promise, and every subsequent write() reuses that rejected chain: later writes never run and the rejection surfaces as an unhandled rejection (void ...write() at the call sites). Make the queue resilient by swallowing rejections before extending the chain.

💡 Suggested Change

Before:

      tail = tail.then(async () => {

After:

      tail = tail.then(async () => {

24. App/frontend/desktop/src/pages/onboarding-page.tsx (L362-L363)

On failure this dispatches agentSourcesFailed, but the onboarding page never renders state.agentSources.error, so the user gets no feedback: the conflict modal stays open, permissionSaving is not set (so the modal's resolveMemoryPluginConflict buttons remain enabled and can start installs/scans while the permission is being reset to unset), and the Cancel button looks like a no-op. Surface the error via local state/modal prop and disable the modal while the reset is in flight.

💡 Suggested Change

Before:

      dispatch(appActions.agentSourcesFailed(error instanceof Error ? error.message : String(error)));
      return;

After:

      setPermissionError(t("onboarding.permission.saveFailed"));
      return;

25. App/frontend/desktop/src/pages/onboarding-page.tsx (L253)

Storing the already-translated text in state means the message stays in the old language after a language switch, and it is never cleared when leaving/returning to the permission step, so a stale error can be shown on a later visit. Prefer storing an error key/flag and translating at render time, and clear it on step transitions.

💡 Suggested Change

Before:

      setPermissionError(t("onboarding.permission.saveFailed"));

After:

      setPermissionError("onboarding.permission.saveFailed");

26. Memory/src/agent-source/runtime.ts (L794)

config is a snapshot taken at line 765, but canInstall re-reads the file on every call and line 813 reads it a third time. The autoInjectSkill gate here therefore disagrees with the guard used inside the loop: if the flag is switched off while this run is in flight, the loop still walks every source and only exits on the first canInstall() call; if it is switched on, the whole branch is skipped for this cycle. Read the access config once (or use the fresh current value) so the branch decision and the install guard come from the same snapshot.


27. Memory/src/agent-source/runtime.ts (L263-L264)

Reconcile only handles revocation: when permission is re-granted it returns at the first line and does not re-arm automation. Because startAutomation() is only called at server start and scheduleAutomation uses the 60-minute SCHEDULED_SCAN_INTERVAL_MS whenever autoScanKnownAgents was false at scheduling time, the first automatic scan after the user grants permission in onboarding can be delayed by up to an hour. Consider resetting the schedule (or kicking an immediate runAutomation) when the reconcile detects that scanning just became allowed.


28. App/frontend/desktop/src/api/scan-preferences-writer.ts (L44-L45)

The grant logic silently upgrades an explicit denial: when permission is "none" (the user chose "Deny" in onboarding) and they flip any toggle in Memory -> sources, grant becomes "scan_only" and updateOnboarding rewrites the stored permission without any confirmation or UI warning. Toggling a preference switch therefore silently overrides a privacy decision the user explicitly made. Consider only auto-granting when permission === "unset" and requiring explicit confirmation for "none" (e.g. surface needsGrant to the caller instead of escalating inside the queue).


29. App/frontend/desktop/src/pages/onboarding-page.tsx (L249-L250)

The permission -> preferences mapping is now duplicated: the backend's setScanPermission(permission, enableAutomation) already derives autoScanKnownAgents/watchFileChanges/autoInjectSkill from scanPermission, and here the same rule is re-implemented as a local literal. Worse, this dispatch publishes a client-side guess instead of the persisted state (the previous code dispatched the result of updateScanPreferences). If the backend rule ever differs (or setScanPermission is skipped, e.g. no scanPreferencesStore), the UI shows preferences that were never stored until the sources page polls them. Prefer reading back the authoritative values (await clients.config.getScanPreferences()) or reusing scanPreferencesWriter instead of hardcoding the mapping.

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: NO TEST SCOPE

Automated tests were not run because the changed files do not map to an executable test scope.

Details: No executable test scope maps to the changed files. Automated tests were not run; add env.yaml source_mapping + execution for this path family, then rerun. Changed files: App/backend/src/adapters/inbound/local-api/routes/agent-sources.ts, App/backend/src/adapters/inbound/local-api/server.ts, App/backend/src/index.ts, App/backend/src/infrastructure/memmy-config/agent-access.ts, App/backend/src/services/agent-source-auto-inject-service.ts, App/backend/src/services/agent-source-service.ts, App/backend/src/services/app-config-service.ts, App/backend/src/services/index.ts, App/backend/src/services/skill-distribution-service.ts, App/backend/src/services/tests/agent-source-auto-inject-service.test.ts
Manual review or env.yaml source_mapping/execution coverage is required before merge.

Branch: fix-onboarding-deny-scan-v127

@origami-chen
origami-chen merged commit 1ecc861 into MemTensor:v1.2.7 Oct 29, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants