You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
269fe3a 当时观测:Linux build、x64/arm64 install-smoke、Windows validation 全通过;upload 按条件跳过,GitHub 显示 All checks have passed。
旧 head 的唯一失败为 云效同步:找不到配置的项目 memmy(1832b179386e24414d3891e244)。pull_request_target 使用受信任 main 的同步脚本;需管理员核实项目 ID/名称或令牌项目权限。新 head 未重新触发该同步,不代表云效配置已经修复;未修改或绕过工作流。
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.
// 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.
Nested ternary (explicitly disallowed by the review checklist) that encodes several independent business rules; it is hard to verify and easy to break.
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.
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.
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.
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.
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.
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.
Two issues here (same pattern at line ~322 for installPlugin):
The ternary is redundant — passing beforeInstall through is already undefined-safe, so both branches are identical in effect.
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.
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.
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.
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.
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.
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.
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.
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.
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.:
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.
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).
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.
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;
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.
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.
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.
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.
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.
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).
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
最新冲突解决与架构复核(2026-10-29)
合入
v1.2.7@03f915d6,提交78463685,当前净差异 28 个文件。保持普通 merge 历史,未合并 PR。本轮验证及限制
--maxWorkers=2运行:2,661 通过 / 2 跳过;默认高并发下两个队列用例超时,单文件 14 项及降低并发全量复跑均通过。问题与修复
移植 #732 到 1.2.7。原逻辑先推进引导,再分别保存权限和自动扫描开关;保存失败或立即退出可能留下旧开关,后台仍扫描。现在原子保存授权和开关,成功后推进;失败提示并可重试,重启恢复已保存授权。快速开关写入串行化,避免旧响应覆盖最终选择。
早期提交 269fe3a 曾为旧基线补齐桌面扫描子进程取消链路。最新 v1.2.7 已统一由 Memory 执行扫描,本轮合并按新架构重新适配,未保留旧后端扫描器;详见顶部最新验证。原 #732 保留。
历史自动验证与审查(269fe3aa)
隔离桌面验收(269fe3aa,2026-10-27)
完整 macOS arm64 unsigned DMG 构建成功,使用本机同型号模型缓存。合成 HOME/app-data/会话与固定响应模型,不使用个人会话。
此前“卡启动页”包含旧自动化窗口观察状态的误判:同一次首次启动、不重启应用,重新绑定窗口后即显示授权页;本轮未修改产品启动代码。GUI 观察中也重新校验实际窗口,未只用后台状态替代界面验收。
取消在 scan 阶段触发,在途写入排空由 HTTP/执行器回归另行覆盖。保存失败恢复重试及快速开关 GUI 已补验如下;不将旧 v1.2.6 GUI 结果冒充本轮结果。未执行真实 Hook/插件安装。
追加隔离 GUI 验收:保存重试与快速开关
同一修复提交 269fe3a、全新合成配置及 1,000 个合成会话;本次未修改源码或新增提交。
历史 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 未重新触发该同步,不代表云效配置已经修复;未修改或绕过工作流。