Repository navigation
fix(memory): enforce per-user authority for turn observation - #1139
mergify[bot] merged 14 commits into
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
2738714 to
25cbe7e
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
XuPeng-SH
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
XuPeng-SH
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
4214e83 to
83f5186
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
已重新审查当前 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
left a comment
There was a problem hiding this comment.
仍需修改:当前 head 83f51863 与上次审查相同,上次关于无效时间戳的评论虽被标记 resolved,但代码没有变化。现已在隔离工作树通过真实加载器、invocation 过滤和 progress 输出的定向 Rust 测试复现:未过滤的控制组返回 unknown,过滤后却错误报告较早的 auth 失败,且损坏计数为零。复现细节已追加到重新打开的线程。
其余此前评论的修复和当前 head CI 已重新核对;本次没有独立重跑完整 Rust、MatrixOne 或真实 Memoria 验证。请先修复该诊断边界并补充覆盖生产调用顺序的回归。
XuPeng-SH
left a comment
There was a problem hiding this comment.
已复核此前所有 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 已通过;合并前仍需检查最终结果。
✅ Merged into
|
| 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
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
UserScopedMemoriaPorttemplate 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
Architecture and complexity delta
Canonical owners remain
UserScopedMemoriaPort/MemoriaCredentialResolver, the shared loop input owner, andSessionCapture. 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.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.not_configured,access_disabled,emptyand 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.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
4c9934c8fixes 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
MEMORIA_SELF_HOSTED_MASTER_ACCESS=1, a configured master credential, noMEMORIA_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).not_configured. Success ratios should usesuccess / (success + error)and display skipped outcomes separately; the bundled dashboard already separates outcomes.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.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.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, andgit diff --check: passed.715a0f7b92f75b3ccea900c625133684c1eadf7c. The preceding83f51863head 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/observeentrypoint 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
CI follow-up (
b5799a3e)The completed CI for
4c9934c8was 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
messageaction 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 recordedresults_adoptedplusfinalization_accepted; only the finaltext_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 againstb5799a3e, not this baseline.Merge and follow-up status
The repository merge queue merged this PR at
b5799a3eon 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.