Skip to content

fix(memory): enforce per-user authority for turn observation - #1139

Merged
mergify[bot] merged 14 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/moi-provider-memory-authority-20261026
Oct 28, 2026
Merged

mergify[bot] merged 14 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/moi-provider-memory-authority-20261026

Conversation

@loveRhythm1990

@loveRhythm1990 loveRhythm1990 commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Capability assessment: Unchanged: observation remains the existing Server background memory capability, governed by the canonical per-user authority shared with recall and session memory.

Summary

Turn observation previously sent the deployment master key directly, allowing background writes when the user's normal memory paths were denied. Server composition now shares one UserScopedMemoriaPort template across observation, prompt recall and session memory. Every observation binds the authenticated Astra owner, checks write admission, and resolves fresh transport immediately before /v1/observe.

Related issue

Closes #1138.

Change type

  • Bug fix
  • Documentation
  • Test
  • Build, CI, or maintenance

Architecture and complexity delta

Canonical owners remain UserScopedMemoriaPort / MemoriaCredentialResolver, the shared loop input owner, and SessionCapture. Existing observer dispatch, recall/session-memory consumers, transport clients, capture loaders/scopers and CI consumers were checked. The separate observer URL/master-key path and discarded HTTP client were removed; no parallel authorization policy, credential cache or persistent lifecycle state was added.

  • The resolved transport supplies the Memoria owner and authentication scheme, with exactly one X-User-Id. Disabled/read-only access sends no observation; lookup errors, owner mismatches and revoked grants fail closed. The worker has no independent master-key fallback.
  • Admission and send-time resolution retain two existing authority reads. Resolving transport no longer constructs a discarded HTTP client. The observer preserves its 10-second connect / 30-second request limits, redirect rejection, single serialization and queue-byte accounting.
  • Observer outcomes distinguish accepted requests from not_configured, access_disabled, empty and errors. The SQL-budget probe waits for exact scheduled and successful counts and rejects skipped, failed, dropped or additional pending observations. Its strict local mock binds both endpoint and test credentials; every captured statement still counts toward the raw 20-statement budget.
  • Empty parent responses check outstanding children, pending replies and newly accepted input before spending the existing one-shot final-answer recovery. Cancellation fixtures use their owned Session and drain before deletion, with bounded replay and safe phase diagnostics. Existing execution budgets, tool authority and success assertions remain in force.

Diagnostic correctness

The producer and consumer share astra.harness_progress.v7. Safe progress retains closed terminal/error tags, event counters, provider-failure stages and run-scoped loop/child boundaries. It publishes no raw responses, stderr, run/user identities, tool arguments or arbitrary trace fields.

Step journals require a matching session and exact run, including canonically linked descendants. Latest provider failure uses comparable producer UTC timestamps rather than artifact merge order; conflicting latest projections remain unknown. Failed-event tags remain failure observations when optional error-kind metadata is absent; successful completed events with a null error kind do not replace failures.

Commit 4c9934c8 fixes the complete loader → invocation scope → progress path: unrepresentable typed timestamps are rejected with corruption metadata, and reliably owned but unorderable events excluded during scoping leave an integrity marker. Further scopes retain that uncertainty. Incomplete captures (skipped/truncated rows or integrity conflicts) cannot claim a latest failure. Retained-event and corruption counters remain visible; invalid rows never become certifiable evidence. No new diagnostic state/schema or weaker acceptance criterion is added.

User and compatibility impact

  • Hosted scoped credentials retain their existing consent and owner. Memories previously written under other owners are not migrated.
  • Standalone self-hosted deployments do not require MOI authentication. Local-account master fallback requires MEMORIA_SELF_HOSTED_MASTER_ACCESS=1, a configured master credential, no MEMORIA_WEB_URL, and the existing active/local/unbound-account checks. Master transports require Memoria's owner-scoped authentication support (fix: behavioral audit round 6 — security, correctness, resource governance #250).
  • Missing authority templates report not_configured. Success ratios should use success / (success + error) and display skipped outcomes separately; the bundled dashboard already separates outcomes.
  • Rust observer implementations now return TurnObserverOutcome, and the constructor accepts an authority port. Repository callers are updated. No migration, HTTP API, persistent lifecycle state, schema, new SQL definition or new Astra database-write path is introduced.
  • This PR does not establish MOI cross-session recall: provider authentication alone does not establish a memory identity/grant. Live passes do not prove that the parent-recovery ordering explains every historical intermittent empty_completion.

Verification

  • cargo test -p astra-test-harness --lib -- --test-threads=1: 639 passed, 1 existing ignored, with serial test execution and unchanged test deadlines.
  • New progress probes reproduced three diagnostic defects before the fix. Regressions cover real typed/raw and enveloped journal loading, root and step scoping, repeated scope, malformed time, incomplete captures, missing/null error metadata, and safe CI summary preservation.
  • python3 -m unittest discover -s scripts/ci -p 'test_manual_harness*.py': 25 passed, 6 platform-dependent skips on macOS (31 total).
  • cargo clippy -p astra-test-harness --lib --tests -- -D warnings, cargo fmt --all --check, and git diff --check: passed.
  • Systematic review covered 25/25 changed files, including documentation and test files excluded by OCR's default selection. No further blocking finding was identified after these fixes.
  • The branch contains upstream main 715a0f7b92f75b3ccea900c625133684c1eadf7c. The preceding 83f51863 head passed all 39 CI checks, including the live harness; this follow-up requires fresh CI on its own commit.

MatrixOne and the real Memoria smoke were not rerun for this diagnostic-only follow-up. Earlier backend/owner-isolation validation is not presented as a fresh result. No QA or production changes were made. Human review and current-head CI remain required before merge.

Public diagnostic path exercised: real typed journal loading → invocation scoping → safe root/step progress, followed by CI summary publication. Unhappy paths include overflow/missing/invalid timestamps, repeated scoping, incomplete capture, absent error metadata, and unrelated runs. The observer's /v1/observe entrypoint and denial/revocation behavior are covered by the existing authority/backend tests and preceding-head CI; the latest patch changes harness capture and diagnostics only.

Final checklist

  • Tests exercise the owning boundaries and production call order.
  • Memory/loop contracts and harness/testing documentation are updated.
  • No credentials, private endpoints, customer data or generated files are included.
  • Conventional Commit PR title and feature-branch delivery.

CI follow-up (b5799a3e)

The completed CI for 4c9934c8 was not green: online integration passed 294/295 with one fixture-cleanup timeout, and the live suite passed 14/18. The online-core failure is the aggregate of the integration failure. Cancellation, all three durable child terminals, and replay completed in about 2 seconds; the timeout occurred after entering fixture cleanup. The exact blocked cleanup operation was not retained by that run.

This follow-up fails cleanup immediately if forced background shutdown reports unfinished tasks, and records safe phase/elapsed markers around every cleanup operation. Two unavailable-model negative controls now provide complete valid spawn arguments, so an unrelated message action field is less likely to prevent reaching model admission. Their original rejection, no-substitution, call-count and zero-child assertions remain unchanged. Journal tool-outcome assertions now publish expected/actual counts through the existing safe numeric diagnostic, sharing the grader's deduplicated count.

Validation of this follow-up: 640 harness unit tests passed, 1 existing ignored; 25 Python tests passed, 6 platform skips; harness Clippy, formatting and diff checks passed. The current-source cancellation journey passed against local MatrixOne (including cleanup); this local run took 69.9s, of which 52.9s was bootstrap and 4.9s cleanup, so it does not establish the CI 60s deadline. A separate fresh local DB probe exceeded its 90s bootstrap allowance and is not counted as a pass. No production behavior, timeout, pass criterion or required CI gate was relaxed.

Still under investigation: the intermittent workspace-evidence parent empty_completion, the actual file-write count in the failed live run, and the precise CI cleanup stall. New-head CI is required; earlier green runs are not evidence that these are fixed.

A fixed-head live repetition at 4c9934c8 (run 40635240667) finished 2/3 passed. The failing repetition exited 0 and recorded results_adopted plus finalization_accepted; only the final text_contains("2024") check failed. This is distinct from the original empty-completion failure and does not establish whether the final-answer mismatch came from model content, file evidence, or output projection. Raw private outputs were not published. The follow-up automatic CI runs against b5799a3e, not this baseline.

Merge and follow-up status

The repository merge queue merged this PR at b5799a3e on 2026-10-28 at 05:29:50 UTC. The assistant did not execute a merge. That head passed Test Suite, static checks and 18/18 live regressions. The cancellation case passed in 3.485s; the exact historical cleanup stall remains unproven.

The subsequent two-file supplement did not enter this merge and is being reviewed separately in draft #1154. Its first three-repetition live run correctly failed: all three answers and original criteria passed, but an added global read-count assertion counted two records. The original causal-child criterion already requires successful completion, so that redundant broader assertion was a mistake in the supplement. #1154 removes it and adds deterministic regression coverage for failed, unknown, unlinked and same-run reads. See that draft for current-head verification; this merged PR does not claim the supplement is validated.

@loveRhythm1990
loveRhythm1990 marked this pull request as ready for review October 26, 2026 06:50
@loveRhythm1990 loveRhythm1990 changed the title fix(memory): 后台 observer 统一使用用户记忆授权 fix(memory): enforce per-user authority for turn observation Oct 26, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed commit 273871418bdeca771d2dfaebebbb6a7a6b94a47d. I found no actionable correctness or design issues in this change. The observer now uses the shared UserScopedMemoriaPort authority path and removes the direct deployment-master-key request path; the new outcomes are per-call results, not persisted state.

One operational cost to keep explicit: an enabled observation resolves existing write authority twice (admission, then fresh transport), typically adding two existing authority snapshot reads per observation. There is no new schema, persistent state, SQL definition, or database write. The extra reads support early denial and a fresh revocation check before sending.

The focused offline authority target passed locally (9/9). Static Checks, PR Title, and the live harness passed; the Test Suite workflow was still in progress at review time. No existing PR comments or review threads were present.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the rebased head and its SQL-budget and mock-credential follow-ups. The original authority boundary remains shared with recall/session memory, and the previously reported live harness failure did not recur on this head (18/18 passed). I found two remaining issues inline: the SQL-budget test can count skipped observations as completion, and authorized observations construct an extra unused HTTP client while resolving transport.

The focused authority tests passed locally (9/9), as did the strict mock credential test. An isolated test confirmed that the budget helper accepts not_configured, access_disabled, and empty outcomes. I did not rerun the full database or real Memoria smoke. Enabled observations still perform two existing authority-snapshot reads; no new schema, SQL definition, or database write was introduced.

Comment thread crates/runtime/tests/chat_sql_budget.rs Resolved
Comment thread crates/runtime/src/turn/services.rs Resolved

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes for the two issues in my preceding review: the SQL-budget test must verify successful observer execution rather than accepting skipped outcomes, and the observer should avoid constructing an unused HTTP client during fresh authority resolution. Please retain the existing owner and revocation checks.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The two previous inline findings are addressed on this head: skipped observer outcomes no longer count as success, and transport resolution no longer constructs an unused HTTP client. One additional SQL-capture boundary remains unresolved, as described inline.

Local verification passed 12 existing offline tests and one focused reproduction; five database tests were ignored. The current live harness reports 17/18: flash_missing_child_model_fail_closed was rejected during argument validation because /prompt was missing and /message was not valid for the action. That evidence does not establish a causal link to this memory change.

Comment thread crates/runtime/tests/chat_sql_budget.rs Resolved

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed this head, including all three prior inline findings. The authorized SQL-budget probe now requires exact scheduled and successful observer counts and rejects skipped/failed outcomes, with regression coverage for an extra scheduled-but-pending observer. Fresh authority transport resolution no longer constructs the unused HTTP client. I found no new actionable code issues in this review.

Local verification: chat_sql_budget 4 offline tests passed (5 database tests ignored), and turn_observer_authority 9/9 passed. The current CI SQL-budget test passed and the live harness passed 18/18. I did not independently rerun the database or real Memoria smoke.

CI is not green: online integration timed out in e2e_matrix_stream_root_cancel_settles_slow_fanout_without_late_synthesis, and the online-core aggregate failed as a result. The prior head passed that same test; the timeout log has no useful location, so its cause and relationship to this PR remain unconfirmed. Please resolve or verify the failed CI before merge. This review does not approve the PR.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The three prior code findings are addressed, and I found no new actionable code issue. I am requesting changes because the current-head online integration gate remains failed: e2e_matrix_stream_root_cancel_settles_slow_fanout_without_late_synthesis timed out after 60 seconds, and the online-core aggregate failed as a consequence. Please investigate or verify the timeout and obtain a passing current-head CI run before merge. The available log does not establish whether this PR caused the timeout.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the current head and the three prior inline findings. The skipped-observation and extra-pending-dispatch SQL-budget gaps are fixed, and fresh authority transport no longer constructs a discarded HTTP client. The latest test-only cancellation-journey change uses the existing fixture lifecycle and preserves the cancellation and replay assertions; I found no new actionable code issue in it.

The earlier online integration timeout did not recur: the current-head online integration job passed. The current live harness, however, failed 2 of 18 cases: agent_workspace_evidence_chain exited 3 with failure class Unknown, and flash_child_question_parent_answer hit its 90-second limit (exit 124, EfficiencyBoundsExceeded). The safe diagnostic reports readiness 200 and a registered, active model, but does not establish the underlying causes or a causal link to this PR. Please investigate or verify these failures and obtain a passing current-head live harness before merge. The macOS resolution job is still pending at review time.

Local validation on this head: 9/9 authority tests and 4/4 offline SQL-budget/mock tests passed; 5 database tests were ignored. I did not independently rerun the real-Memoria or MatrixOne tests. No new database schema, persistent state, SQL definition, or database write was found; an enabled observation still performs the two existing authority reads for admission and fresh send-time resolution.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes for the two diagnostic correctness issues below. They can hide the failing step or misidentify the latest provider failure even when the underlying run has settled correctly.

Validation: I reviewed the current head and traced the per-step progress projection, session capture merge, and diagnostic consumer. Two isolated Rust probes on the earlier head d2e1f0681412688df4e1003aed8942c010f21e9c reproduced the outputs described below; I checked that the affected paths remain unchanged at this head. The current-head GitHub Static Checks, Test Suite, and live harness passed. I did not rebuild those Rust probes against this head: the isolated build required a full dependency rebuild and was stopped. The local Python progress suite had 19/21 passing; the two unchanged process-supervisor tests failed identically on the earlier head in the same environment, and their cause was not established.

@loveRhythm1990
loveRhythm1990 force-pushed the fix/moi-provider-memory-authority-20261026 branch from 4214e83 to 83f5186 Compare October 28, 2026 01:05

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

已重新审查当前 head,包含原来的 observer 权限/SQL 预算、父子等待恢复、harness 诊断以及全部历史 review 线程。之前的权限、额外 HTTP client、观察完成计数和按步骤关联 journal 等修复已核对;但最后一次 provider failure 的无效时间处理在真实 capture → scope → projection 路径仍有遗漏,详见行内评论。

当前 head 的 GitHub 检查已全部完成并通过,live harness 为 18/18。独立运行 Python progress 测试 22 项,18 通过、4 项因平台跳过;git diff --check 通过。本次未独立重跑 Rust、MatrixOne 或真实 Memoria 测试。该发现根据加载器及生产调用链静态推导,尚未在 Rust 中运行复现。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

仍需修改:当前 head 83f51863 与上次审查相同,上次关于无效时间戳的评论虽被标记 resolved,但代码没有变化。现已在隔离工作树通过真实加载器、invocation 过滤和 progress 输出的定向 Rust 测试复现:未过滤的控制组返回 unknown,过滤后却错误报告较早的 auth 失败,且损坏计数为零。复现细节已追加到重新打开的线程。

其余此前评论的修复和当前 head CI 已重新核对;本次没有独立重跑完整 Rust、MatrixOne 或真实 Memoria 验证。请先修复该诊断边界并补充覆盖生产调用顺序的回归。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

已复核此前所有 review 问题及本次修复。上次复现的无效时间戳问题现在从真实 journal 加载、invocation scope 到最终 progress 输出均能保留不确定性;异常记录不再成为可认证证据。原有用户权限 owner、SQL 预算和步骤归属修复也已核对,未发现新的阻塞问题。

本地验证:此前的真实加载/过滤/输出回归在本 head 通过;progress 测试 23/23、session capture 测试 48/48 通过;Python manual harness 25 通过、6 项平台跳过;diff 检查通过。测试回归仅临时添加于隔离工作树。未独立重跑 MatrixOne 或真实 Memoria。

当前 head 的 GitHub CI 尚在运行,以上批准不代表 CI 已通过;合并前仍需检查最终结果。

@mergify mergify Bot added the queued label Oct 28, 2026
@mergify
mergify Bot merged commit 0fe324c into matrixorigin:main Oct 28, 2026
42 of 43 checks passed
@mergify

mergify Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

✅ Merged into main as 0fe324c at 05:29 UTC

Less than a minute in the queue, with no time running CI.

So far

  • ✅ Entered the queue Wed 28 Oct, 05:29 · rule main · triggered by rule Automatic queue on approval for main
  • ✅ Checks skipped · PR is already up-to-date
  • ✅ Merged 05:29 · 0fe324c · squash
Merge conditions: 17 of 17 met

This pull request

Condition Required by
✅ #review-threads-unresolved = 0 GitHub branch protection
✅ github-review-approved GitHub branch protection

The batch, tested in place

Condition Required by
✅ any of: check-success = Check PR title, check-neutral = Check PR title, check-skipped = Check PR title GitHub branch protection
✅ any of: check-success = check, check-neutral = check, check-skipped = check GitHub branch protection
✅ any of: check-success = Test: core crates + bridge hooks, check-neutral = Test: core crates + bridge hooks, check-skipped = Test: core crates + bridge hooks GitHub branch protection
✅ any of: check-success = Test: astra-runtime, check-neutral = Test: astra-runtime, check-skipped = Test: astra-runtime GitHub branch protection
✅ any of: check-success = Test: astra-cli (edge-fs-tools), check-neutral = Test: astra-cli (edge-fs-tools), check-skipped = Test: astra-cli (edge-fs-tools) GitHub branch protection
✅ any of: check-success = Test: astra-cli (edge-git-gix), check-neutral = Test: astra-cli (edge-git-gix), check-skipped = Test: astra-cli (edge-git-gix) GitHub branch protection
✅ any of: check-success = Test: astra-cli (edge-rest), check-neutral = Test: astra-cli (edge-rest), check-skipped = Test: astra-cli (edge-rest) GitHub branch protection
✅ any of: check-success = Test: astra-cli (edge-shell), check-neutral = Test: astra-cli (edge-shell), check-skipped = Test: astra-cli (edge-shell) GitHub branch protection
✅ any of: check-success = Test: astra-cli (non-edge), check-neutral = Test: astra-cli (non-edge), check-skipped = Test: astra-cli (non-edge) GitHub branch protection
✅ any of: check-success = Test: online (core), check-neutral = Test: online (core), check-skipped = Test: online (core) GitHub branch protection
✅ any of: check-success = Test: online (integration), check-neutral = Test: online (integration), check-skipped = Test: online (integration) GitHub branch protection
✅ any of: check-success = Test: turn-core + services + plan, check-neutral = Test: turn-core + services + plan, check-skipped = Test: turn-core + services + plan GitHub branch protection
✅ any of: check-success = web (typecheck, test, build), check-neutral = web (typecheck, test, build), check-skipped = web (typecheck, test, build) GitHub branch protection
✅ any of: check-success = Astra SDK (typecheck, test+coverage, build), check-neutral = Astra SDK (typecheck, test+coverage, build), check-skipped = Astra SDK (typecheck, test+coverage, build) GitHub branch protection
✅ any of: check-success = Test: macOS workspace coordination, check-neutral = Test: macOS workspace coordination, check-skipped = Test: macOS workspace coordination GitHub branch protection

Times in UTC · dashboard · how the merge queue works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(memory): 后台 observer 绕过用户记忆授权并使用错误 owner

2 participants