Repository navigation
Conversation
hidb4ai
left a comment
There was a problem hiding this comment.
Design note (non-blocking): please trim the duplicate report artifacts.
The synthetic fixtures, configuration, tests, and curated EXPERIMENT.md belong in the repository. Keeping report.json and deterministic-support-audit.json as a frozen evidence snapshot is also reasonable, given the aggregate-report deliverables in #2069.
For evaluation/memory/capture_ownership/reports/qwen-flash-20261020/, I suggest removing:
report.md: the per-row table is generated from the same data inreport.jsonand can be regenerated on demand.follow-up-issue.md: the production follow-up is already tracked in #2119. Link to that issue from the experiment report and update the accompanying references instead of maintaining a second copy of its text.
Future routine run outputs should stay in an ignored results directory or an external artifact archive, consistent with evaluation/README.md. Additional committed snapshots should have an explicit purpose as maintained reference evidence.
|
Thanks for the suggestion. Addressed in commit 11ea32d. Removed the generated report.md and duplicate follow-up-issue.md. The CLA is now signed. Could a maintainer please approve the pending |
| assert evidence["content"]["content"] == cases[row["case_id"]].prompt | ||
| # Runtime compaction retains the body once and removes its metadata copy. | ||
| assert "content" not in evidence["source_metadata"] | ||
| assert row["occupancy"]["text_carriers"] == 1 |
There was a problem hiding this comment.
[P2] 这个断言改成 1 之后,本 PR 提交的 report.json(该 arm 全部为 2)在 head 上已无法复现
冲突解析把 joined-source-v1 的期望从 text_carriers == 2 改成 1,理由是 master 的证据压缩只保留一份正文。这部分与 head 一致:CI 的 evaluation-tests(make evaluation-unit-test,显式包含 evaluation/memory/capture_ownership/tests、-m "not live",并 uv sync --extra capture-ownership)在 head f71f6955 上 success,说明 head 重放确实得到 1。
问题在本 PR 同时提交的度量产物没有跟着更新:
evaluation/memory/capture_ownership/reports/qwen-flash-20261020/report.json中joined-source-v1的 38 行occupancy.text_carriers全部是 2(host-turn-candidate-v1是 28 行 1 / 10 行 0),而同文件code_sha仍是2c21132ba5dd1caa986484b8d71e1504f0dca3e2,即 PR 的第一个提交,早于本次 merge 引入的运行时压缩。- 该 harness 的
README.md自己规定 lock 绑定 "production and evaluation source file hashes … code commit","A changed dataset/code/configuration is rejected before replay","do not reuse a reviewed output under a different lock"。按这条契约,仓库里这份报告已经不属于当前代码:读者会把 38×2 当成 head 的载体占用度量,但 head 只会得到 1,且没有任何字段说明差异来自 master 的压缩。
影响范围是证据artifact 的可追溯性,不是产品行为;decision(projection_follow_up_required)由 critical_candidates 计数推出、不由 occupancy 推出,所以结论本身不因此反转。
建议二选一:
- 在 head 上重新
freeze+run+report,更新reports/qwen-flash-20261020/report.json与对应 markdown(code_sha随之前移到合入点); - 或在报告与
README.md明确标注:2c21132b之后 master 的证据压缩使joined-source-v1的text_carriers由 2 变 1,正文数字只描述该提交,不描述当前 head。
另:本次仅核对已提交产物与 CI 选择集,未在本机重跑 --mode live(需要真实 provider 凭据),live 与 offline 的 occupancy 是否同样为 1 未实测。
There was a problem hiding this comment.
Thanks for catching this. Addressed in commit 55713ea using the historical-snapshot clarification option.
The suite README and EXPERIMENT.md now explicitly distinguish the live experiment frozen at 2c21132 (two text carriers) from the post-compaction offline regression at f71f695 (one carrier).
The original report.json, code_sha, run/lock hashes, and measurements remain unchanged. No post-compaction live evaluation is claimed.
Could you please take another look? Thank you.
There was a problem hiding this comment.
按历史快照方案处理,55713ead 核对结论:本条闭合。
reports/qwen-flash-20261020/report.json未被改写:76 行,joined-source-v138/38 行occupancy.text_carriers == 2,code_sha仍为2c21132b;与EXPERIMENT.md新增 "All 38joined-source-v1rows haveoccupancy.text_carriers == 2" 一致。- head 的离线回归仍按当前运行时断言一次携带:
tests/test_replay.py对joined-source-v1断言"content" not in evidence["source_metadata"]且text_carriers == 1,与 README 新句 "The offline regression at mergef71f6955checks exactly one text carrier" 一致。 README.md与EXPERIMENT.md的交叉引用锚点reports/qwen-flash-20261020/EXPERIMENT.md#revision-scope与## Revision scope标题对应,可解析;"No post-compaction live evaluation is claimed" 已明确排除把旧度量当作当前 checkout 的读数。
非阻断提示:EXPERIMENT.md 与 README.md 里两处历史提交链接指向 tmt-123/powercontext 的 commit。合入上游后改为 oceanbase/powercontext/commit/... 更耐久,fork 删除后现有链接会失效。
Zxf-xufeng
left a comment
There was a problem hiding this comment.
Reviewed commit 55713ea. I found two issues in the evaluation harness: automatic continuations can bypass the advertised request/cost guard, and the experiment lock can accept changes to the inference implementation without changing its provenance hashes. Please address the inline comments before merging.
Validation: 24 local tests passed; 2 were skipped because Alembic was unavailable in the local environment. Both findings were reproduced offline, with no live provider calls. The request-limit reproduction used Pydantic AI 2.29.0. GitHub checks had no failures at review time.
The clarification that the committed live report describes the historical pre-compaction experiment is sufficient; these findings concern future execution of the harness, not a claim that the historical measurements are invalid.
| input_type=AtomicMemoryExtractionInput, | ||
| output_type=AtomicMemoryExtractionOutput, | ||
| model_settings=cast(ModelSettings, config.model_settings), | ||
| limits=InferenceLimits(timeout_seconds=config.timeout_seconds, max_requests=config.max_requests), |
There was a problem hiding this comment.
[P2] Disable automatic continuations to enforce the request budget
This constructs InferenceLimits without setting allow_continuations, whose default is True. Pydantic AI handles a suspended response by issuing continuation requests inside the same agent request step, so max_requests does not cap the number of provider calls. In an offline reproduction using this adapter configuration and Pydantic AI 2.29.0, a FunctionModel returning suspended and then complete was called twice with max_requests=1, while the returned usage still reported requests=1. For a provider that returns suspended responses, these additional calls bypass the harness's per-extraction reservation and request pacing, so the configured cost ceiling is not guaranteed. Please set allow_continuations=False here, or explicitly account for and bound every continuation before issuing it. This does not imply that the historical Qwen run used continuations.
There was a problem hiding this comment.
Independently reproduced at 55713ead with the evaluation lock's Pydantic AI 2.57.0: a local FunctionModel returning suspended and then complete was called twice with max_requests=1, while usage reported requests=1 (202 input / 14 output tokens). With allow_continuations=False, the control stopped after one call with InvalidInferenceOutputError. Please disable automatic continuations here or explicitly reserve, pace, and bound each actual call. No provider calls were made.
There was a problem hiding this comment.
Addressed in commit 17ecf1d.
The evaluation extractor now explicitly sets allow_continuations=False. SDK retries remain disabled.
The regression uses a local FunctionModel with max_requests=1 to verify that a suspended response fails the extraction without a second model invocation.
| paths = [ | ||
| "integrations/workbuddy/plugins/powercontext/hooks/workbuddy_powercontext_hook.py", | ||
| "src/powercontext/builtin/sources/content.py", | ||
| "src/powercontext/builtin/artifacts/atomic_memory/extraction.py", | ||
| "src/powercontext/builtin/artifacts/prompt/builtin.py", |
There was a problem hiding this comment.
[P2] Include the inference implementation in the experiment freeze
The fixed production-file list omits code that directly controls the generated request, including src/powercontext/builtin/inference/pydantic_ai.py and builtin/runtime/composition.py. code_sha only records HEAD, so it does not detect uncommitted changes to those files. In a temporary copy, I froze the experiment, changed the instructions passed to Agent in pydantic_ai.py, and verified that freeze() still returned an identical lock and verify_lock() accepted it. A replay can therefore execute different inference logic under the same code, prompt, and harness provenance hashes, contrary to the documented rejection of code changes after freezing. Please either reject uncommitted changes to the relevant production sources at freeze/replay time, or include their contents in a sufficiently complete source fingerprint.
There was a problem hiding this comment.
Independently reproduced at 55713ead: in a temporary Git repository containing copies of the reviewed files, I froze a configuration with max_requests=2, then changed retries=self._limits.max_requests - 1 to retries=0 in the tracked src/powercontext/builtin/inference/pydantic_ai.py without committing. verify_lock() still accepted the original lock. This confirms that retry behavior, as well as the request instructions described above, can change without changing the recorded provenance. Relevant runtime edits need to invalidate replay before any generation occurs.
There was a problem hiding this comment.
Independently confirmed at 55713ead in an isolated Git copy. After freezing with max_requests=2, I changed the tracked adapter from retries=self._limits.max_requests - 1 to retries=0 without committing. The regenerated lock was identical and verify_lock() accepted the old lock. Generation-affecting runtime edits need to invalidate replay before any model call, either through a sufficiently complete source fingerprint or an explicit check for relevant uncommitted changes.
There was a problem hiding this comment.
Addressed in commit 17ecf1d.
The freeze now includes the complete Runtime source/resource tree, WorkBuddy Python helpers, evaluation sources and audit scripts, package configuration, and dependency locks.
It hashes working-file contents and the file inventory. Regression cases verify that inference/runtime/host edits and source additions or deletions invalidate the lock even when HEAD is unchanged.
knqiufan
left a comment
There was a problem hiding this comment.
Reviewed 55713ea. Requesting changes for the reproduced canary-audit and usage-recording defects detailed inline. The freeze-integrity defect was also independently reproduced and is already tracked in the existing thread.
Validation used local synthetic inputs and the actual HTTP/SQLite/extraction path with a FunctionModel; no live provider evaluation or native WorkBuddy acceptance is claimed.
| for index, candidate in enumerate(row["candidates"]): | ||
| if literal not in candidate["text"]: | ||
| continue | ||
| entries.append( |
There was a problem hiding this comment.
[P2] Match complete canary values before attesting non-current support
The repeated-history anchor is 37, so this substring check also accepts a candidate saying "The legacy archive must retain backups for 370 days" as a witness for the frozen 37-day claim. In a local simulated run, I truthfully annotated that changed value as unsupported and retained the required current facts in the paired arm; audit() still emitted two archive-retention witnesses. This incorrectly presents an unsupported value as deterministic evidence of the historical claim. Please require an unambiguous match for the complete canary value, including appropriate boundaries, before emitting a witness. This reproduction does not establish that the private outputs behind the committed snapshot contain this error.
There was a problem hiding this comment.
Independently reproduced at 55713ead: replacing the historical candidate's 37 days with 370 days, marking that candidate as unsupported, and retaining the required current fact in the paired output still made audit() emit two archive-retention witnesses. The substring test therefore attests a different numeric value as support for the frozen claim. Please require an unambiguous match for the complete canary value before emitting a witness. This used synthetic local outputs and does not establish an error in the private outputs behind the committed snapshot.
There was a problem hiding this comment.
Addressed in commit 17ecf1d.
Ground-truth validation, required-current literal retention, and canary auditing now share complete-value matching with identifier/path and numeric boundaries. The audit also requires the output-bound annotation to reference the corresponding frozen claim.
Extended values such as 370 cannot witness the 37-day claim, including when a support annotation is mistaken. All original 12 historical witnesses still reproduce under the stricter audit.
| candidates = await pipeline.extract(request) | ||
| row["candidates"] = [item.model_dump(mode="json") for item in candidates] | ||
| row["provider_usage"] = measured.usage if mode == "live" else None |
There was a problem hiding this comment.
[P2] Preserve known usage when candidate validation fails
MeasuredGenerator.generate() records usage after successful structured generation, but pipeline.extract() can subsequently raise InvalidInferenceOutputError for an unavailable evidence ID. Because usage is copied only after extraction succeeds, that known usage is discarded. A local FunctionModel returning 101 input and 7 output tokens plus an invalid evidence ID produced four generation_failed rows with provider_usage=null, despite the adapter returning usage each time. Please persist known usage independently of candidate validity and include it in the report's known-usage totals (which currently select only completed rows), while leaving genuinely unavailable usage unknown. This was reproduced through the real HTTP/SQLite/extraction path with a local model, without provider requests.
There was a problem hiding this comment.
Independently reproduced at 55713ead through the actual HTTP/SQLite/extraction path with a local FunctionModel. Four calls each returned 101 input / 7 output tokens; only the first candidate cited an unavailable evidence ID. That row became generation_failed with provider_usage=null, so the report counted 303/21 tokens instead of the known 404/28. Restoring the failed row's usage still left the aggregate at 303/21 because it filters to completed rows. Please fix both usage preservation after structured generation and aggregation of known failed-call usage. No external provider calls were made.
There was a problem hiding this comment.
Addressed in commit 17ecf1d.
Usage returned by the adapter is now retained in a finally block, including when candidate evidence-ID validation subsequently fails. Aggregation includes both completed and generation-failed rows.
Known subtotals are preserved, unavailable usage remains unknown, and measured zero remains zero. Regression cases check persisted rows and aggregate totals across invalid evidence IDs, successful responses, and failures without a response.
hidb4ai
left a comment
There was a problem hiding this comment.
Reviewed 55713ead35be922b30c33f4cbf9f6019ff814a6b. I independently reproduced the four open P2 findings and added the evidence to their existing threads. Please address request/cost limits, experiment-freeze integrity, exact canary matching, and failed-call usage accounting before merging.
The historical-snapshot clarification is sufficient. These findings concern execution and evidence integrity of the evaluation harness; they do not establish that the historical Qwen measurements are wrong. All reproduction probes used local synthetic inputs, with no external provider calls.
|
@Zxf-xufeng @knqiufan @hidb4ai Thank you for the reviews and independent reproductions. The four reported issues are addressed in commit 17ecf1d, and I have added details in the corresponding review threads. The committed historical report and deterministic audit JSON remain unchanged. No new live provider evaluation or native WorkBuddy acceptance is claimed. Could you please re-review the current head, 17ecf1d? Thank you. |
hireview001
left a comment
There was a problem hiding this comment.
- CI failed: windows (failure).
- CI failed: macos (macos-15-intel) (failure).
- CI failed: macos (macos-15) (failure).
- CI failed: Desktop validation (failure). Failed steps: macos (macos-15) / Run cargo test --locked --manifest-path src-tauri/Cargo.toml; macos (macos-15-intel) / Run cargo test --locked --manifest-path src-tauri/Cargo.toml; windows / Run cargo test --locked --manifest-path src-tauri/Cargo.toml.
knqiufan
left a comment
There was a problem hiding this comment.
Reviewed e522d49e. One P2 remains in the nested evaluation lock; please refresh it before merging.
| { name = "tzdata", marker = "extra == 'server'", specifier = ">=2026.3" }, | ||
| { name = "uvicorn", marker = "extra == 'server'", specifier = ">=0.34,<1" }, | ||
| ] | ||
| provides-extras = ["builtin", "cli", "client", "code", "seekdb", "server", "tracing-otlp"] |
There was a problem hiding this comment.
[P2] Refresh the nested evaluation lock for the current package metadata
At this head, uv lock --locked --offline --project evaluation fails with "The lockfile at uv.lock needs to be updated", while the master evaluation lock passes. This local powercontext entry still omits ops and expanded dependency metadata from the current root pyproject.toml, so the documented uv sync --project evaluation --extra capture-ownership --locked setup cannot use the committed lock. In an isolated checkout, refreshing only this package's metadata makes the locked check pass without changing dependency versions. Please refresh the nested lock against the current checkout and verify it in locked mode.
There was a problem hiding this comment.
Addressed in commit f08ef54.
Refreshed evaluation/uv.lock against the current root package metadata, including ops and the expanded dependency declarations.
Only the local powercontext package metadata changed. All 130 package versions, sources, and dependency relationships remain unchanged.
Verified that uv lock --locked --offline --project evaluation passes, and a fresh installation with uv sync --project evaluation --extra capture-ownership --locked --offline succeeds.
Could you please re-review? Thank you.
Which issue or RFC does this PR close?
Refs #2069.
Production design follow-up: #2119.
This PR delivers the capture-ownership evaluation implementation and report. The separate production issue tracks the follow-up design required by the evaluation findings.
Rationale for this change
WorkBuddy preserves the full host-assembled prompt for Source auditability. That text can include historical user turns, previously injected PreparedContext, host/Skill instructions, and conversation summaries.
The captured text reaches Atomic Memory extraction through both content and Source metadata. This evaluation measures whether non-current context produces new memory candidates and compares an evaluation-only current-turn view while preserving production behavior.
What changes are included in this PR?
capture-ownershipevaluation CLI for manifest validation, experiment freezing, replay, candidate review, and aggregate reporting.joined-source-v1with an evaluation-onlyhost-turn-candidate-v1, keeping Source identity, evidence ordering, extraction instructions, model settings, and generation limits consistent.Experiment results
The completed experiment used pinned
qwen-flash-2025-07-28, temperature 0, thinking disabled, a 1024-token output cap, and two repetitions per arm.All 64 planned model calls completed successfully. Another 12 candidate-arm rows were skipped because the current-turn boundary was empty or unknown.
Critical candidates include non-current-only support, false current-user attribution, or unsupported assertions.
Six cases reproduced contamination in both repetitions while their paired experimental outputs retained all required current facts without critical contamination. Twelve deterministic literal witnesses supplement the support annotations.
The resulting decision is
projection_follow_up_required. The production design work is tracked separately in #2119.Reports are under:
evaluation/memory/capture_ownership/reports/qwen-flash-20261020/These results describe the synthetic corpus and configured model, not production incidence. The experimental view still produces quoted-input contamination and current-fact omissions, so it is not deployment-ready.
Are there any user-facing changes?
Adds an evaluation CLI, synthetic fixtures, documentation, and reports.
Production capture, Source registration, public APIs, extraction instructions, Artifact publication, search/ranking, citations, and PreparedContext behavior remain unchanged.
Raw run inputs, generated candidate text, and credentials remain outside Git. Committed experiment reports contain aggregate measurements and content-free evidence checks.
How was this change tested?
uv run pytest tests/workbuddy_plugin tests/test_source_observations.py tests/test_sources.py tests/e2e/test_atomic_memory_generation_merge.py tests/builtin/artifacts/prompt tests/builtin/artifacts/memory/test_prompts.py evaluation/memory/capture_ownership/tests -q— 163 passed.make evaluation-unit-test— 773 passed, including the offline real HTTP/persistence/pipeline replay.uv run --project evaluation --extra capture-ownership ty check --project evaluation— passed.uv run ty check— passed.uv run prek run -a— passed.The test suites overlap; their pass counts should not be added together.
I manually reviewed the supplied review materials. Stored candidate annotations retain their Codex-assisted provenance.
Native WorkBuddy client acceptance, other models/providers, other extraction profiles, and other database backends were not evaluated. The replay uses the runtime's native ContentSource materialization branch and independently checks the registered text-evidence projection.
AI usage statement
OpenAI Codex assisted with implementation, tests, and candidate support annotation.
Qwen Flash was used as the measured generation model, not as a judge. Critical reproduction findings are supplemented by the committed deterministic support audit. I also manually reviewed the review materials.