Skip to content

benchmark: split the legacy run/evaluate chain into B, H, and Apps legacy (B01) - #422

Merged
Dong1017 merged 4 commits into
mainfrom
impl/b01-run-evaluate
Nov 15, 2026
Merged

Dong1017 merged 4 commits into
mainfrom
impl/b01-run-evaluate

Conversation

@Dong1017

@Dong1017 Dong1017 commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

benchmark: split the legacy run/evaluate chain into B, H, and Apps legacy (B01)

Governing Issue and delivery slice

Refs #414

Cross-issue architecture adoption: This PR carries B01 implementation plus the shared five-entry D008 decision metadata for #414 and #413. The delegated architecture adoption is recorded on Issue #414 and Issue #413. The OBS ownership sections remain in the joint record; OBS implementation remains in PR #425, not this PR. These records document architecture adoption, not independent implementation acceptance or reviewer APPROVE; both issues retain their evidence requirements.

The #371 design was accepted by the user in issue comment 6841315306. Design acceptance is not implementation approval.

@amy-why-3459 @zengchuang-hw: this update is for re-review of the B01 implementation plus shared D008 architecture metadata in the three-dot diff against current main 8c321005d50c2ecef61bf20132409d88474a6450 at PR head e427c42c535ddb49fce0d6943b44044e03f11cf2. Both D008 choices are adopted and linked to the decision record; this is architecture adoption, not reviewer APPROVE. The public Harness signature question is recorded below with the D018/D022/FINAL quotations, line numbers, and in-tree call-site evidence; H defaults that would create an H→B edge were not restored. None of these are self-approved.

Current head (ordinary merge, read back 2026-11-15 07:42 Asia/Shanghai after the remote update): e427c42c535ddb49fce0d6943b44044e03f11cf2. Parents are previous B01 candidate 0c056b930ac1900298dfa4c71285b57837b3d9cf and current main 8c321005d50c2ecef61bf20132409d88474a6450 (includes #437 engine/sandbox). Tree 7f4986ff18e313e364ed95815317c1e1199ebdd9. Current main is an ancestor. Unique three-dot diff versus current main: 51 files, +1301 / −110. Commit count versus main is 4. Author GUOGUO <xwdong1998@163.com> with the existing DCO and Agent-assisted-by trailers. This is an ordinary merge that preserves B01 legacy-run boundaries, not a force-with-lease rewrite of the current tip and not a merge of the PR into main. Previous official remote tip d5f6e02bc48ae22d2fbb2d9ce718265e5395708c remains an ancestor of this head. The current tree is not identical to d5f6e02 / db924ea.

Hosted CI that completed on earlier heads db924ea165fbf69a8240745ee2eed1cb3c044eea or d5f6e02bc48ae22d2fbb2d9ce718265e5395708c ran on those earlier trees and must not be treated as tests of current head e427c42. The decision-record headings ## Benchmark sampling and ## Snapshot port remain in the current tree (blob links above now point at e427c42c535ddb49fce0d6943b44044e03f11cf2). The shared D008 record is unchanged and still also cites the OBS entries; that cross-issue adoption question remains a reviewer item and is not claimed resolved by this body update.

A new hosted run started after the 2026-11-15 07:42 Asia/Shanghai ordinary push of this merge onto current main. First read-back 2026-11-15 07:46 Asia/Shanghai: lint-and-smoke (job), declarations (job), pr-references (job), installed-package 3.10/3.11 (jobs, 3.11), and knowledge-model-policy (success, Not applicable, run) already success; full-check 3.10 (job), full-check 3.11 (job), and dashboard-browser (job) were still in_progress and are not reported as passed. GitHub mergeable=true; mergeable_state=blocked (reviews and required checks outstanding). auto_merge is null. No amy-why-3459 or zengchuang-hw Approve exists on this head.

Local Linux evidence on the exact current head, from an exact git archive of e427c42 / tree 7f4986ff on the shared Debian 13.7 / Python 3.13.5 host (completed 2026-11-15 07:15 Asia/Shanghai, before this push): 89 passed + 56 subtests, 0 failed/errors/skips, covering the four exact codex/workswarm export and knowledge-reads nodes plus the B01-impacted legacy-run / forwarder / gate modules. Native 45-second timeouts and integrity checks were not relaxed. This is candidate-host evidence, not hosted CI and not a full-suite pass. Prior Windows timeout records are retained and not re-characterized.

#397 and the main tip this branch already merged (not a pending prerequisite)

#397 is already squash-merged to main. Do not treat it as an unmerged prerequisite of this PR, and do not describe this PR as a vehicle that would land #397.

Ref Role
bf9aeeb528b5354e53b8f9e9476d3c5f853e7a74 H02, already on main.
8150ee048af6503070fa71768fbf64dffcc1a77e Squash-merge of #397, "Publish standardized Apps run output (#397)", parent bf9aeeb, 2026-11-13 17:05:44 Asia/Shanghai (UTC+8). The unsquashed review-branch SHAs (f9a2336, d8d86387, 0f4c22d, …) are not ancestors of this squash.
fe16b29388de29708d30ea388b589355540f3172 Then-current main at the 1db7d6b1 integration. #400, "Use user acceptance criteria, with ceiling-analysis fallback (#400)", parent 8150ee04, 2026-11-13 17:56:06 Asia/Shanghai (UTC+8). This was the tip merged into this branch at 1db7d6b1, not 8150ee04 alone. It is not the current main tip.
57f8e8a4915b38201d9fa58d4d05acbe3c4199ee Then-current main as of the 2026-11-14 19:36 Asia/Shanghai history rewrite, still the squash-merge of #428 (committer date 2026-11-14 14:07:08 Asia/Shanghai). It was the single parent of then-head d5f6e02bc48ae22d2fbb2d9ce718265e5395708c. Current main is now 8c321005d50c2ecef61bf20132409d88474a6450 and is a parent of current head e427c42.

This branch was started earlier by stacking a local merge of main at K01 d7de7f3 plus the then-head of #397 (f9a2336), then cherry-picking later #397 docs d8d86387, and adding a test-only NEW_H_CLOSURE row for execution_spec when that helper was still missing from main. Those unsquashed commits remain in this branch's history, but they are historical. After the ordinary merge of then-current main (1db7d6b1, parents f0dd3ec3 + fe16b293), the three-dot diff versus that tip no longer includes the #397 Apps tree (apps/api.py, apps/archive.py, apps/result.py, docs/apps-output.md, harness/execution_spec.py, run_store.py, tests/test_apps_*.py). That exclusion still holds versus current main 57f8e8a. Merging this PR would land the B01 split, not #397 and not #400.

The #397 squash also added inferrsi.harness.execution_spec to main's H02 NEW_H_CLOSURE. The previous "gap not cleared on main" / "execution_spec is not on main" comments are obsolete and have been removed. The only conflict on the main merge was tests/test_harness_execute.py: both sides already listed the same closure set; this branch kept that set, took main's wrapping, and replaced the stale comment. No #397 source was edited.

At the 1db7d6b1 / f6371b5c integration, git diff versus then-current main (fe16b293) was recorded as 51 files, +1281 / −111. At previous official head db924ea, and again at d5f6e02 versus then-main 57f8e8a, the unique three-dot B01 diff was 52 files, +1335 / −112. Current head e427c42c535ddb49fce0d6943b44044e03f11cf2 versus current main 8c321005d50c2ecef61bf20132409d88474a6450 is 51 files, +1301 / −110. This is not a claim that the current tree equals the raw db924ea or d5f6e02 trees; later ordinary merges integrated subsequent main, including #437. Git recorded the B01 moves as harness/evaluation.py → benchmark/legacy_run_v0.py, harness/scenarios/media.py → benchmark/sampling.py, and harness/reporting.py → apps/legacy/reporting.py.

This addresses the whole-PR blocker in zengchuang-hw's COMMENT review on head f0dd3ec3 (review 5888255798, 2026-11-13 16:39:42 Asia/Shanghai (UTC+8)) and amy-why-3459's item 1 in issue comment 6848125700 (16:36:32 Asia/Shanghai (UTC+8)): merge accepted main (with #397) so the three-dot diff contains only B01, and drop the stack-specific closure comment.

Commits (B01 work and integration)

The PR branch now contains four unique commits versus current main, listed at the end of the table. Earlier SHAs below remain provenance; d5f6e02 and the later ordinary merges are ancestors of the current tip. The unsquashed #397 / H02 review-stack commits were not replayed.

Commit Role
85f4521333c451358ace7c233d34e1ada191f68c Historical local merge of main at K01 d7de7f3 and then-#397 f9a2336. Local base only.
4380add202fb3a8feeff7b5d60c11e1f17a53ab5 DCO freeze of the B01 implementation on that base. Author GUOGUO; implementer trailer Agent-assisted-by: Claude Code / claude-fable-5-2.
77144a6741cfaaee4072b22bd186fa3d9778ad07 Ordinary merge of main / H02 squash bf9aeeb (before #397 landed). execute_trial / execute_command still accept environment.
0788ffa6540a9c2fdbda83b8084cc0f87d376c76 Historical cherry-pick -x of then-#397 docs d8d86387.
f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5 Historical test-only NEW_H_CLOSURE addition, made when execution_spec was not yet on main. This was the previously reviewed and CI-green head.
1db7d6b10b4d7eb159874b68c44962514df7fd38 Ordinary merge of then-current main (fe16b293, includes #397 squash 8150ee04 and #400). Conflict only in tests/test_harness_execute.py, resolved as described above. No reset, rebase, or dropped B01 work.
f6371b5c6adae56173bb2a1ab4d09a9457ad2d62 Then-current pushed head after the sha256 pin (amy-why-3459 item 4): test_sampling_sha256_matches_the_infra_hasher pins benchmark.sampling._sha256 to infra.artifacts.sha256. Historical; no longer the branch tip.
ac69ce491b4966f20fc492bfaee89d0d3db98d2b Docs: record RSI-master D008 B01 ownership decisions in the contract table.
db924ea165fbf69a8240745ee2eed1cb3c044eea Previous official head before the 2026-11-14 19:36 Asia/Shanghai history rewrite (then 17 commits vs main). Rollback point; no longer the branch tip. Docs: cite the delegated D008 decision record and clear review nits.
d5f6e02bc48ae22d2fbb2d9ce718265e5395708c Previous official remote tip before later main merges (single-parent presentation on then-main 57f8e8a; tree 268da5969fd28bb535dc4bd8e0225e7ac3637108). Remains an ancestor of the current tip.
a663fcc71c0c5f6b897bf0adc014d7eee265ada4 Ordinary merge of main after the K02 resource move.
0c056b930ac1900298dfa4c71285b57837b3d9cf Ordinary merge preserving the B01 run split with then-current main read routing.
e427c42c535ddb49fce0d6943b44044e03f11cf2 Current HEAD (read back 2026-11-15 07:42 Asia/Shanghai: PR #422 OPEN, head.sha this SHA, commits=4, changed_files=51, +1301 / −110, mergeable=true). Parents 0c056b93 and current main 8c321005d50c2ecef61bf20132409d88474a6450; tree 7f4986ff18e313e364ed95815317c1e1199ebdd9. Author GUOGUO; Signed-off-by: GUOGUO <xwdong1998@163.com>; Refs #414.

Review items (citable D008 decisions and Markdown/nit fixes recorded)

These are listed for independent reviewers. Previous official head db924ea165fbf69a8240745ee2eed1cb3c044eea added the citable record, which remains in current head e427c42c535ddb49fce0d6943b44044e03f11cf2, correct per-row/header status, a blank line after the ownership table, and the test whitespace nit. No Harness defaults or gate-policy approval are added.

  1. D008 owner rows now cite the decision record and its per-row anchors. RSI-master means the architecture-coordination role delegated by requesting maintainer Dong1017 (xinwen dong), not an additional GitHub reviewer/approver or a claim of legal repository ownership.

    • Benchmark sampling: collect_samples belongs to B; transport remains caller-injected.
    • Snapshot port: H owns the required keyword-only (repo, exclude) -> dict contract, Apps legacy binds it, infra/targets implements it; H has no default.
      The five record decisions are adopted under delegation; other baseline table entries are not blanket-accepted. Independent implementation review remains separate. Authority text is now separated from the table by a blank line.
  2. Public Harness signatures vs D018/D022 (amy-why-3459 item 2; cursor[bot] review 5889339860 blocker 2). This PR does change the Harness ports relative to pre-B01 main (bf9aeeb). That is not left as an uncited "in-scope decision"; the contract text, the old call surface, and the in-tree compatibility path are recorded here.

    Contract text (exact, with line numbers on this head unless noted):

    • docs/contracts/function-owners.md L25: "Existing external public composed entries keep their original semantics and paths per D018/D022, and an owner change cannot be used to delete an old promise." The same file L15 already says of harness/runner: "old run unchanged; migrating old evaluate decouples the runner first; new H scoring cannot be retained by attaching a legacy label." L19 assigns read_metric / validate_metric to B and execute_command / retention to H, "no whole-file ownership."
    • D013, docs/contracts/current-design.md L138–L140: "old entry points, arguments, Path, stdout, stderr, and old run 0/1/2 stay byte-for-byte compatible. New Apps naming/envelope/exit mapping is fixed by D022, but K01/H01 do not implement a CLI."
    • D022 §1, docs/contracts/isolation-errors-and-cli.md L113 / L117–L119: "Path-by-path retention of old entries"; "K01 / H01 do not implement the new CLI, and they do not touch the console inferrsi=inferrsi.cli:main, __main__, arguments.parser / Path / defaults, old root / harness evaluation, old run 0 / 1 / 2 / stdout / stderr, plan / --dry-run, existing grade behavior, or plugin entries. Old run judgment still gives incomplete priority over failed, and the arrangement of reasons does not change meaning. Old evaluate migration follows D018-B4: B.legacy_run_v0 keeps the rules, and Apps.legacy.evaluation assembles. First the runner detaches from the internal evaluate, then the old evaluation / harness.evaluation compatibility edges are reviewed leaf by leaf. New H must not score through an old shell." D023's preface at L115 says it "does not change the compatibility commitments in section 1."
    • The route that D022 L119 names "D018-B4" is the later legacy-evaluation entry in the current gate-policy file, which is headed ### B3 (there is no ### B4 heading in that 118-line file). docs/contracts/gate-policy-runstore-and-first-prs.md L52–L56: "B legacy_run_v0.py keeps the only old evaluate rules; Apps legacy/evaluation.py adapts old RunSpec/args to B-local DTOs and returns the original dict"; "new H delivers only fixed execution facts, and evaluation is composed by the EvaluationPort that Apps injects; the old run composition path keeps the original call behavior in Apps legacy." "Re-exporting old harness.evaluation is then a compatibility endpoint, not an H core dependency"; "This is not exemption by labelling business code legacy."
    • PR-DIVISION-FINAL.md B01 L74–L75 (accepted 2026-11-12 11:35–11:36 Asia/Shanghai): "规则进 B,执行事实留 H,组合进 Apps legacy"; "runner 不再内嵌判分与写报告,改由 Apps legacy 组合;scenarios/external 的执行留 H、metric 校验进 B"; "旧 harness 路径不留会形成 H→B/Apps 的壳;根 evaluation.py / execution.py / reporting.py 可暂留精确转发,到 R01 再删。" Acceptance at L77 includes "旧 run 退出码 0/1/2、incomplete 优先、同一夹具下 evaluation.json 字节不变." B01 explicitly does not retire those root forwarders or migrate the CLI (L78). FINAL's only other D022 mention (A-entry L124) is that A-entry will not deliver D022's new evaluate subcommands or a first-wave DeprecationWarning; that is not a B01 deliverable.

    Old call surface at H02 bf9aeeb (the pre-B01 public H / root APIs):

    • harness/scenarios/external.py L12–L20: execute_trial(..., metric_key, *, metric_reader: Callable[..., float] = read_metric, environment=None). The default imported read_metric and validate_metric from inferrsi.model.protocols.metrics (L8) and applied validate_metric(metric_key, metric_reader(...)) (L30). That is an H→model/B metric-rule edge.
    • harness/runner.py L19–L25: run_experiment(spec, environment, *, execute=execute_trial, snapshot=repository_identity) — defaults imported execute_trial from H and repository_identity from infra.targets (L12, L16); the body also imported evaluate and write_report from harness.evaluation / harness.reporting (L14–L15), which is the inlined scoring/report path B01 must detach.
    • Root execution.py L18–L19 was already def run_experiment(spec: RunSpec, environment: dict) -> Path: and forwarded to H with execute= / snapshot= filled in. CLI run imported this root entry (cli.py still does, L498–L500).
    • The documented compose example at docs/architecture.md L251–L264 on bf9aeeb imported run_experiment / execute_trial from inferrsi.harness.runner and inferrsi.harness.scenarios.external and used execute=partial(execute_trial, metric_reader=read_engine_metric).
    • In-tree importers of those H modules on bf9aeeb: tests/test_dashboard_workflow.py, tests/test_flare_adapter.py, tests/test_module_boundaries.py, tests/test_runtime.py (run_experiment as run_with_adapters plus H execute_trial), tests/test_trial_checks.py (from inferrsi.harness.runner import run_experiment and run_experiment(spec, {"backend": "cpu"}) with no ports), plus docs as above. optimization_session.py already called H execute_trial with metric_key=None (execution facts only).

    What this head keeps, and what it does not restore:

    • metric= on H execute_trial is optional (None by default, external.py L18). TypeError is raised only when metric_key is not None and metric is None (L27–L28). H no longer imports read_metric / validate_metric. Restoring a default metric_reader=read_metric or default execute=execute_trial / assess=evaluate / report=write_report / snapshot=repository_identity on the H callables would re-introduce the H→model / H→B / H→Apps / H→infra-targets edges that FINAL L75 forbids ("旧 harness 路径不留会形成 H→B/Apps 的壳") and that D018-B3 L56 and D022 L119 require to be composed by Apps, not kept as an H core default. Those defaults were not restored.
    • Compatibility for the composed old run is in apps.legacy.run (L19–L57): execute_trial still accepts metric_reader= (default read_metric) and wraps H with metric=partial(trial_metric, reader=metric_reader); run_experiment still defaults execute=execute_trial and snapshot=repository_identity and supplies assess / report. Root execution.py L18–L19 is unchanged in signature vs bf9aeeb and now forwards to that Apps composition. docs/architecture.md L257–L269 retargets the compose example to from inferrsi.apps.legacy.run import execute_trial, run_experiment and keeps metric_reader=. CHANGELOG L15–L21 discloses the H port change and points callers at apps.legacy.run.

    In-tree call-site verification on this head (no extra code fix required):

    • Supported composed / root entries, still with the old signatures: cli.py L498–L500 → inferrsi.execution.run_experiment(spec, environment); tests/test_runtime.py L19 / L104 / L576 same root entry; tests/test_trial_checks.py L11 / L105 / L114, tests/test_flare_adapter.py L10 / L303 / L310, tests/test_dashboard_workflow.py L8 / L29–L31, and tests/test_module_boundaries.py L13 / L62–L65 all use inferrsi.apps.legacy.run (run_experiment(spec, env) or partial(execute_trial, metric_reader=...)).
    • In-tree H execute_trial callers that remain on the H module are execution-fact paths, not the old metric_reader composition: harness/optimization_session.py L43 and L89–L90 pass metric_key=None; tests/architecture_real_ports.py L82–L88 and tests/test_moe_benchmark.py L420–L421 do the same; tests/test_legacy_run_split.py asserts the new TypeError and the metric= path. The MoE task (benchmark/tasks/moe-forward-npu/benchmark.py L258–L261) receives an injected executor and itself passes metric=read_metric with "metrics.suite_latency_ms", which is also what docs/moe-forward-benchmark.md L50–L52 describes. Session injection of the bare H executor at optimization_session.py L234 is therefore still valid for that task.
    • Residual unsupported call, out of tree only (disclosed, not hidden): a caller that still does from inferrsi.harness.scenarios.external import execute_trial and passes metric_reader= or a non-None metric_key without metric=, or from inferrsi.harness.runner import run_experiment and calls run_experiment(spec, env) without the four ports, will get TypeError. No such remaining in-tree production, test, or documented compose path was found on this head. The supported replacement is inferrsi.apps.legacy.run (or the unchanged root inferrsi.execution.run_experiment(spec, environment)). This is not a request to restore H defaults.

Independent verification and test evidence

All times below are Asia/Shanghai (UTC+8). Machine-wide cap of 2 pytest workers was respected.

  1. Previously reviewed head f0dd3ec3. Remote CI on that head: all listed checks passed (declarations, full-check 3.10 and 3.11, installed-package 3.10 and 3.11, lint-and-smoke, pr-references, and knowledge-model-policy = Not applicable). The earlier PR-body line that "CI lanes have not run" / "POSIX / Linux CI pending" is obsolete.
  2. Linux byte-equality of evaluation.json is tested, not only inferred. tests/test_legacy_run_v0_frozen.py::test_old_run_keeps_exit_code_and_evaluation_bytes compares CLI evaluation.json bytes to the frozen fixtures, normalizing \r\n to \n. Those tests are part of the full-check lanes that passed on f0dd3ec3. zengchuang-hw independently reported the 12 frozen CLI scenarios passing on Linux, including byte comparisons, plus 99 passed in the focused B01 modules, in review 5888255798.
  3. Windows-local evidence on the earlier stack (before this main merge; not repeated as a full suite): focused 17-file recheck at 0788ffa, 15:43:16–15:56:05, -n 2, then a test-only closure fix and exact-node retest 16:03:10–16:04:11 (1 passed). That work predates Publish standardized Apps run output #397 landing on main.
  4. Historical focused regression and SSH push of then-head f6371b5c, after then-current main (fe16b293) was merged. Waiter PID 42268 started the focused set at 18:32:01 once no other pytest was live, and finished at 18:37:33, exit 0. Command (Python 3.13.5, pytest 9.0.2, -n 2) on exact then-head f6371b5c: tests/test_legacy_run_v0_frozen.py, tests/test_legacy_run_split.py (includes the sha256 pin), tests/test_legacy_run_forwarder_gate.py, tests/test_target_module_import_gate.py, tests/test_moe_benchmark.py, tests/test_harness_execute.py::test_h02_6_no_device_model_or_network_fallback, and tests/test_harness_run_store.py::test_h01_6_new_h_closure_reaches_no_legacy_k_b_or_apps_module. Result: 100 passed, 2 skipped in 301.39s (0:05:01). Log: wave2-prep/b01-focused-after-main397-t248.log. SSH push 18:37:33–18:37:43, PUSH_EXIT=0. Result file wave2-prep/b01-wait-recheck-push-t248.result.txt: tests_exit=0 push_exit=0 local=f6371b5c… remote=f6371b5c…. At that time, git ls-remote upstream refs/heads/impl/b01-run-evaluate and gh pr view 422 (headRefOid, mergeable=MERGEABLE; BLOCKED was the outstanding CHANGES_REQUESTED review, not a merge conflict) confirmed f6371b5c. That SHA is no longer the branch tip. No full-suite repeat locally. Hosted CI on db924ea and the new run on d5f6e02 are described under Current head above; the db924ea Ready run is not evidence for the new merge-tree.
  5. Historical remote CI on previous head f6371b5c (read back 18:38:40 Asia/Shanghai (UTC+8), immediately after the push): declarations (16s), lint-and-smoke (14s), pr-references (7s), and installed-package 3.10/3.11 already success; knowledge-model-policy is Not applicable; full-check 3.10 and 3.11 were still pending. That pending status applied only to f6371b5c at that read-back and does not describe previous official head db924ea, later d5f6e02, or current head e427c42 (see Current head above). Ready CI on the previous reviewed head f0dd3ec3 was already fully green, including the frozen evaluation.json byte comparison.

Out of scope / not claimed

Copy link
Copy Markdown
Collaborator Author

@amy-why-3459 @zengchuang-hw Please review B01 at f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5, especially the gate-policy changes, D008 ownership rows, and frozen MoE task-pack adjustment listed in the PR body. This branch includes the unmerged #397 Apps dependency and must not land it indirectly. The body distinguishes implementer results, independent checks, the reproduced H02 closure mismatch and its test-only fix, and checks still pending. Refs #414; no Issue closure or merge is requested.

@Dong1017 Dong1017 added area:harness Harness module: profile, A/B loop, quality gates, evidence reports kind:task Bounded delivery task with a delegated slice labels Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Review of f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5 against main bf9aeeb528b5354e53b8f9e9476d3c5f853e7a74. This is a full first review. I separated out the files that come from #397 and reviewed the B01 split itself: benchmark/legacy_run_v0.py, benchmark/sampling.py, apps/legacy/*, harness/runner.py, harness/scenarios/external.py, harness/atomic_json.py, the root forwarders, cli.py, the gate policy, the docs and the tests. I'm not approving yet. Items below.

Checked, no change needed

  • legacy_run_v0.evaluate(..., run_error=) keeps the old behaviour. Before, runner appended error to reasons after evaluate and forced INCONCLUSIVE. Now run_error is the last item of incomplete, so reasons = failed + incomplete comes out in the same order with the same verdict. The string labels give the same values as the old Verdict.*.value.
  • MoE task-pack change (benchmark/tasks/moe-forward-npu/benchmark.py L554, L563-564): model/protocols/metrics.read_metric already ends in validate_metric, so metric=read_metric gives the same behaviour. The runtime injection in harness/optimization_session.py L232-236 passes harness.scenarios.external.execute_trial, which accepts metric=. The other execute_trial callers in that file (L45, L91) pass metric_key=None, so the new TypeError guard does not affect them.
  • LEGACY_RUN_FORWARDERS (tests/_gate_policy_support.py) only tightens the gate. Each root file maps to one exact target, the Apps closure may not reach the root forwarders, and the blocker keeps isolation_complete false. tests/test_legacy_run_forwarder_gate.py covers the adjacent module, second importer and loop-back cases.
  • CI at f0dd3ec3: all 7 checks pass, including lint-and-smoke and full-check 3.10/3.11.

Needs a change or an answer

  1. Stacked Publish standardized Apps run output #397 code. Merging this PR would also land Publish standardized Apps run output #397. Publish standardized Apps run output #397 is now approved at d497762152c7a05f593e9d836fb6b54519a567f0, but this branch carries a different history for it: the f9a2336b merge plus the cherry-pick 0788ffa6. Please merge Publish standardized Apps run output #397 first. Then merge main back in so the three-dot diff contains only B01. When you do that, please also drop the stack-specific comment in tests/test_harness_execute.py L31-35 ("execution_spec is not on main ... does not clear the main gap"), because it goes stale as soon as Publish standardized Apps run output #397 is on main. I'll re-review the B01-only diff after that.
  2. Public Harness signatures change. harness/scenarios/external.py L18 replaces metric_reader= with metric=, and L27-28 now raise TypeError when metric_key is set without metric. harness/runner.py L20-23 removes the execute / snapshot defaults and makes assess / report required. On main, harness/README.md told integrators to "Compose scenarios/external.py with a metric reader for a new engine", so these look like documented extension points. docs/contracts/function-owners.md L25 says that "existing external public composed entries keep their original semantics and paths per D018/D022". Are harness.scenarios.external.execute_trial and harness.runner.run_experiment outside that promise? If they are, please cite the decision in the PR body. If they are not, keep a compatible signature, or a forwarder under apps.legacy, until R01. The CHANGELOG entry (L17-23) is good either way.
  3. D008 rows (docs/contracts/function-owners.md L22-23). On the substance I have no objection to either row. collect_samples does seed ordering, timing and artifact hashing, which is measurement work and belongs in B. The snapshot port is bound by Apps legacy without a default in H. Who formally accepts D008 rows? If it's the architecture owner rather than PR reviewers, please record that acceptance. Then update the rows so they no longer say "owner pending formal review". Otherwise they will be stale on main after merge, the same problem we fixed in docs/apps-output.md on Publish standardized Apps run output #397.
  4. Duplicated helpers (small). harness/atomic_json.py duplicates infra.artifacts.atomic_write_json, and benchmark/sampling.py L521-526 duplicates infra.artifacts.sha256. The writer has the byte-equality test test_harness_writer_bytes_equal_the_infra_writer. Is there an equivalent test pinning sampling._sha256 to infra.artifacts.sha256? If not, please add a one-line equality assertion so the two copies can't drift before R01.
  5. PR body is out of date. It still says "CI lanes ... have not run on this branch" and "POSIX / Linux CI pending". Linux CI has now run and passed at f0dd3ec3. Please update those lines. Linux byte-equality of evaluation.json is still "inferred" in the table. Does any CI test compare the frozen fixtures byte-for-byte? If so, cite it; if not, keep it listed as a gap.

@zengchuang-hw zengchuang-hw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR review verdict — DO NOT PASS for the whole stacked PR yet; the focused B01 review found no new blocking implementation defect.

Reviewed head f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5 against base bf9aeeb528b5354e53b8f9e9476d3c5f853e7a74. This is my first review of #422, responding to #422 (comment). Acting reviewer: OpenAI dot for zengchuang-hw, under the owner's direct request to publish the completed review. This is a non-approving COMMENT, not Request changes or merge authorization.

B01 findings

  • The gate registers only the three exact root-to-Apps forwarder pairs; adjacent targets and another importer remain rejected. Existing forwarders keep overall isolation false, and Apps cannot loop back through them.
  • The D008 ownership rows match the code: Benchmark owns sampling order/timing/artifact hashes, while the callback owns transport; Apps supplies repository_identity to the required Harness snapshot port. Sampling adds no H/infra import.
  • The frozen MoE entry supplies metric=read_metric; that reader already applies the positive-finite metric validator. Harness retains execution facts rather than semantic metric rules.
  • The legacy run is composed in Apps with four required H ports and B-owned rules/labels. The focused Linux checks preserve exit codes 0/1/2, incomplete precedence and golden evaluation.json bytes. H-owned JSON writer byte equality and the integrated H02 execution_spec closure assertion pass.

Before whole-PR approval

The current main-target diff still includes the unmerged #397 Apps dependency, as the PR explicitly states. A focused B01 result must not approve or land that dependency indirectly. Resolve the dependency through its separate independent acceptance, or isolate B01 on an accepted base, then re-review the resulting head/base integration. This review neither advances nor approves #397.

Independently executed

Exact-head source was materialized in an isolated workspace and verified against the Git blob SHAs.

  • Linux / Python 3.12: test_legacy_run_v0_frozen.py, test_legacy_run_split.py, test_legacy_run_forwarder_gate.py, test_target_module_import_gate.py, test_moe_benchmark.py: 99 passed, 1 warning, 10.84 s.
  • test_image_modules.py plus test_harness_execute.py::test_h02_6_no_device_model_or_network_fallback: 6 passed, 4 skipped, 1 warning. Skips are missing optional skimage.metrics.
  • The 12 frozen CLI scenarios passed on Linux, including byte comparisons. The MoE fake-runtime/process checks are not a real NPU benchmark.
  • Remote evidence for this exact head: seven completed-success check runs and a successful knowledge-model-policy status (Not applicable). This is distinct from my local focused checks.

Not independently executed: full suite, wheel/sdist/install validation, Windows, or real NPU/service measurements. No campaign PASS claimed. Initial local materialization/PYTHONPATH setup errors were corrected before the exact-source final runs and are not attributed to this PR.

Optional documentation follow-up: the PR body's statement that CI has not run is now stale; distinguish the current remote successful checks from the original Windows-local evidence.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR review verdict — DO NOT PASS
Reviewed base bf9aeeb528b5354e53b8f9e9476d3c5f853e7a74 and head f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5 against current origin/main fe16b29388de29708d30ea388b589355540f3172. Governing Issue #414 (B01). Independent reviewer; not the PR author.

Blockers (must clear before re-review)

  1. Head is not mergeable onto current main; three-dot is still the #397 stack. mergeable_state=dirty. Merging origin/main into this head conflicts in tests/test_harness_execute.py at NEW_H_CLOSURE (lines ~39–45: formatting/execution_spec membership vs main after #397). Main already contains #397 (8150ee04) and #400 (fe16b293). Three-dot vs merge-base bf9aeeb is still 63 files / +2891/−118, including apps/api.py, apps/archive.py, apps/result.py, docs/apps-output.md, and tests/test_apps_* — even though those blobs already match main. #414 / Amy / zengchuang all require a B01-only integration after #397 lands. Expected: merge or rebase onto current main, resolve the conflict, drop the stack-only comment at tests/test_harness_execute.py L31–35, push a new head whose three-dot is B01-scoped, then re-request review.

  2. Public Harness signature break vs D018/D022 is unanswered. harness/scenarios/external.py L18–28 replaces metric_reader= with metric= and raises TypeError when metric_key is set without metric. harness/runner.py L20–23 removes execute/snapshot defaults and requires assess/report. docs/contracts/function-owners.md L25 still says existing external public composed entries keep original semantics and paths per D018/D022. Compat wrappers exist under apps/legacy/run.py, but the H entry points themselves changed. Expected: cite an accepted decision that these H paths are outside that promise, or keep compatible H signatures / H-side forwarders until R01; record the choice in the PR body.

  3. D008 ownership still marked unapproved while the move already landed. docs/contracts/function-owners.md L22–23 still say owner pending formal review for collect_samples → B sampling and runner snapshot → Apps legacy. #414 required owner rows before the move; merging would publish stale Proposed text. Expected: record formal D008 acceptance (architecture owner or delegated acceptance on the Issue/PR) and update the rows so they no longer say pending, or do not merge the ownership claim until accepted.

Verified on this head (not enough for PASS)

  • Ready CI on f0dd3ec: declarations, full-check 3.10/3.11, installed-package 3.10/3.11, pr-references, lint-and-smoke — all success.
  • Local Linux exact-head: focused B01 set 100 passed in 9.05s (test_legacy_run_v0_frozen, test_legacy_run_split, test_legacy_run_forwarder_gate, test_target_module_import_gate, test_moe_benchmark, test_h02_6_no_device_model_or_network_fallback).
  • first_wave_report(): isolation_complete remains false; blockers include legacy run forwarders: 3 as required by #414.
  • #397 Apps blobs on this head match current main; residual issue is integration/scope presentation, not divergent Apps source.

Notes (non-blocking)

  • Add a one-line pin that benchmark.sampling._sha256 equals infra.artifacts.sha256 (writer already has test_harness_writer_bytes_equal_the_infra_writer; local Linux bytes currently match).
  • PR body still claims CI has not run / Linux CI pending; Ready CI is green on this head — refresh before the next review.
  • Prior peer feedback: amy-why-3459 comment and zengchuang-hw COMMENT both withheld whole-PR approval for the same stack reason; this review agrees and does not clear #397 through this PR.

No merge.

@zengchuang-hw

Copy link
Copy Markdown
Contributor

#397 已随 8150ee04 合入,当前 #422 仍以 H02 为共同基线,GitHub 显示存在冲突,diff 里还包含 Apps 标准输出文件。请同步最新 main,清理历史堆叠说明,再发一次仅含 B01 的复核请求。Apps 侧会按 #414 对照旧 run 的 0/1/2 退出码、incomplete 优先和固定样例的 evaluation.json 字节。

Amy 提到的旧 Harness 扩展入口 也请一并处理。旧 run 的保真与 metric_reader=、runner 默认参数这两个调用面的兼容性需要分别核对。

@zengchuang-hw

Copy link
Copy Markdown
Contributor

已复核同步 main 后的 f6371b5c。当前 diff 已去掉 #397 的 Apps 标准输出代码。Windows 49 项定向测试通过,覆盖旧 run 的 0/1/2、incomplete 优先、evaluation.json 规范换行后的固定样例字节,以及转发和导入边界。apps.legacy.run.execute_trial 提供 metric_reader=,原 Harness 导入路径的同名函数只接受 metric=。请在 PR 说明里明确原导入路径的兼容策略和 D008 接受记录,并更新已过时的 #397 堆叠说明。

Copy link
Copy Markdown
Collaborator

Incremental review of f0dd3ec3ac8a154f9ff47ecdcad5d69fcf12f5c5..f6371b5c6adae56173bb2a1ab4d09a9457ad2d62. Current main is fe16b29388de29708d30ea388b589355540f3172 and the merge-base is now fe16b293. I checked it against my items in 6848125700 and the blockers in review 5889339860. Not approving yet: one item is still open (D008, below).

Resolved

  • Item 1, the Publish standardized Apps run output #397 stack. 1db7d6b1 merges current main. Local git merge-tree onto fe16b293 is clean. The three-dot diff no longer contains apps/api.py, apps/archive.py, apps/result.py, docs/apps-output.md or tests/test_apps_*. The stale comment in tests/test_harness_execute.py L31–32 now correctly says execution_spec landed on main with 8150ee04. NEW_H_CLOSURE matches main.
  • Item 2, the Harness signatures. Answered with in-repo contract text. docs/contracts/gate-policy-runstore-and-first-prs.md B3 (L54–56) says "new H delivers only fixed execution facts … the old run composition path keeps the original call behavior in Apps legacy", and D022 L119 says "New H must not score through an old shell". apps/legacy/run.py keeps execute_trial(..., metric_reader=read_metric) and run_experiment(spec, environment) with the old execute / snapshot defaults. Root execution.run_experiment(spec, environment) is unchanged. docs/architecture.md L269 and CHANGELOG L15–21 point callers to apps.legacy.run. The residual risk, an out-of-tree direct caller of the H modules, is disclosed in the PR body. I accept this answer.
  • Item 4. test_sampling_sha256_matches_the_infra_hasher in tests/test_legacy_run_split.py L163–166 pins sampling._sha256 to infra.artifacts.sha256. Nit: L167–169 leaves three blank lines before the next test.
  • Item 5. The PR body is refreshed. Byte equality of evaluation.json is cited as tested (test_old_run_keeps_exit_code_and_evaluation_bytes).
  • Hosted checks at f6371b5c are all successful: declarations, lint-and-smoke, pr-references, full-check 3.10/3.11 and installed-package 3.10/3.11.

Still open

  • Item 3 / review 5889339860 blocker 3. docs/contracts/function-owners.md L22–23 (collect_samples → B sampling, runner snapshot port → Apps legacy) still read "owner pending formal review". The PR body says no one has recorded who formally accepts these rows. I have no objection to their substance. But merging as is would publish two pending ownership claims on main for code that has already moved. Please get the D008 acceptance recorded by whoever owns it (on task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 or this PR). Then update L22–23 so they no longer say pending, citing that acceptance. If the acceptance has to come after merge, state that explicitly as an accepted exception. Once that lands I will re-review the new head and approve if nothing else changes.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR review verdict — DO NOT PASS

Reviewed base fe16b29388de29708d30ea388b589355540f3172 and head f6371b5c6adae56173bb2a1ab4d09a9457ad2d62; merge-base fe16b293. Current main has advanced to f7b640f2; a local git merge-tree against it is clean. This is an incremental review: I agree with the earlier incremental review in issuecomment-6850804160 and do not repeat the items it records as resolved.

Blockers

  • docs/contracts/function-owners.md:22-23 still says "owner pending formal review" for both D008 rows, while the moves are already in this diff. #414 requires the owner to be recorded before collect_samples moves. Record the acceptance (or an explicitly accepted exception) and cite it in those rows; no code change is needed. This is the only remaining blocker.

Issue completion
#414 is partially complete: the split, frozen old-run behavior, exit codes and byte-identical evaluation.json are met; the D008 acceptance is still missing, so the issue requirements should not be closed yet.

Check scope
Read #414/#371 and the existing reviews/comments, the head diff, ownership and gate files; ran the focused Linux/Python 3.12 set (102 passed in the review, 51 passed in the issue check). Did not run the full suite, Windows, wheel/sdist, three-host or NPU/service checks.

Comment thread docs/contracts/function-owners.md Outdated
| `harness/scenarios/external` `read_metric`/`validate_metric` | B semantic validation; `execute_command` and retention → H | no whole-file ownership; offline assess executes nothing |
| `campaign_records` `STAGES`/claim capture/reconciliation | H records; dashboard/report presentation → Apps | first remove the `campaign_records → dashboard_data` reverse dependency; #333 is Proposed and no claim is made that a unified event envelope is accepted |
| `catalog.repository_pack`/`REPO_TYPES` | D020 neutral raw-data technical leaf; target interpretation stays H | K does not expose the pack reader publicly; `infra` changes only its same-leaf import; seven K bodies/root entries/tests move together |
| `harness/scenarios/media` `collect_samples` (**added with the old-run split; owner pending formal review**) | B sampling: seed ordering, timing records and artifact hashes | request transport stays with the caller's callback; B hashes artifacts itself and imports no H or `infra` module |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both rows 22–23 still say owner pending formal review, but #414 requires the D008 owner to be recorded before collect_samples moves. This is the only remaining blocker on this head: record the acceptance (or an explicitly accepted exception) and update these rows to cite it. Same standard as #425.

Copy link
Copy Markdown
Collaborator

Incremental review of f6371b5c6adae56173bb2a1ab4d09a9457ad2d62..ac69ce491b4966f20fc492bfaee89d0d3db98d2b. That is one commit, ac69ce49 "docs: record RSI-master D008 B01 ownership decisions", and it only changes docs/contracts/function-owners.md (+3/−2). Merge-base is unchanged. This checks the one item I left open in 6850804160 (D008), which is also the remaining blocker in review 5891934882. Not approving yet.

What changed

  • L22–23 no longer say "owner pending formal review". They now state the dispositions: collect_samples → B sampling, and the snapshot port → H contract, Apps legacy binding and infra/targets implementation. I still have no objection to the substance. The _sha256 pin test cited in L22 exists (tests/test_legacy_run_split.py::test_sampling_sha256_matches_the_infra_hasher).

Still open

  1. The decision record can't be found. L22–23 cite "RSI-master architecture decision 2026-11-14 Asia/Shanghai". The new L24 says "the rows cite that decision record". But neither row links to a record. task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 has no comment recording it, and neither does this PR's conversation. The only trace is this commit, authored on the PR branch (Agent-assisted-by: ... (architecture decision integration)). I asked for the acceptance to be recorded by whoever owns D008, on task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 or here, and for the rows to cite it. Please add a link from L22–23 to the actual record: an issue/PR comment or a decision-record file, made by the owner. Please also say who "RSI-master" is in the repository's terms. Without that, a reviewer can't verify the adoption L24 describes. I'm not judging who the owner should be. I'm asking for a citable record.
  2. The PR body is stale relative to this head. L7, L12 ("D008 row proposed, not approved"), L19, L55–58 and L97 still say the D008 rows are pending formal review and were deliberately not rewritten. Please update the body to match ac69ce49 and cite the record from item 1.
  3. Markdown nit, function-owners.md L24. The new paragraph comes right after the last table row with no blank line, so GitHub renders it as an extra table row. Please add a blank line before L24.
  4. Question, L3. The file header still says Status: **Proposed**. If L22–23 are now adopted decisions, please clarify how adopted rows sit inside a Proposed table (for example, a per-row status or a note in L3). Otherwise readers will get conflicting signals.

Checks at ac69ce49 (read just now): declarations, lint-and-smoke, pr-references and installed-package 3.10/3.11 succeeded. full-check 3.10/3.11 were still in progress.

Once item 1 is citable (and 2–3 are fixed), I'll re-review the new head and expect to approve if nothing else changes.

Copy link
Copy Markdown
Collaborator Author

@zengchuang-hw @amy-why-3459 @david6666666 refreshed head: ac69ce491b4966f20fc492bfaee89d0d3db98d2b. The two remaining D008 ownership entries now record RSI-master's architecture decision (2026-11-14 Asia/Shanghai): sampling belongs to Benchmark, with transport injected through the callback; snapshot has a Harness-owned port contract, Apps legacy binding, and infra/targets implementation, with no Harness default dependency. This docs-only commit records the architectural choice; it does not claim another reviewer's approval or complete implementation acceptance. Please assess concrete boundary/correctness issues against this head rather than treating ownership as awaiting a choice. New-head hosted CI and other Issue #414 acceptance evidence remain separate.

Copy link
Copy Markdown
Collaborator

@Dong1017 I re-checked ac69ce491b4966f20fc492bfaee89d0d3db98d2b after your 6872763392. The head is unchanged, so my 6872720512 still applies. I'm not disputing the ownership choice itself. My review of the boundary and correctness side of these two rows found nothing new beyond that comment. What's still open:

  1. No citable record yet. task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 has no new comment since then, and this PR conversation has none either. The decision is still stated only by the PR branch (the commit and your comment). Please link docs/contracts/function-owners.md L22–23 to a record made by whoever decides D008, and say who "RSI-master" is in repository terms.
  2. PR body is still stale. L12, L19, L55–58 and L97 still say the D008 rows are proposed or pending formal review.
  3. Missing blank line before L24. Without it, the paragraph renders as a table row.
  4. L3 Status: **Proposed**. Still needs a clarification.

Once 1–3 are in a new head, I'll re-review.

Copy link
Copy Markdown
Collaborator

Incremental review of ac69ce491b4966f20fc492bfaee89d0d3db98d2b...db924ea165fbf69a8240745ee2eed1cb3c044eea. That is one commit, db924ea1 "docs: cite delegated D008 decisions and clear review nits". It adds docs/contracts/d008-wave2-ownership-decision.md (+45), edits function-owners.md (+4/−3), and removes one blank line in tests/test_legacy_run_split.py. The merge-base is unchanged. Not approving yet.

My items from 6872720512 / 6872896898

  • 3. Blank line before the authority paragraph: resolved. The paragraph is now separated from the table.
  • 4. L3 status: resolved. "Proposed baseline; individually adopted entries are identified by their linked decision records" removes the conflicting signal.
  • 1. Citable record: mostly resolved. L22–23 now link to anchors in the new decision file, and its L7 defines RSI-master. One point to make explicit: per L7, RSI-master is an assistant acting under the delegation of Dong1017, who is also this PR's author. So the adoption is recorded by the author side. That matches the delegation model that function-owners.md L3 already describes. I'm not blocking on it, but please confirm in task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 that this delegation is the intended authority for D008 adoption. A maintainer comment there would also work.
  • 2. PR body: still stale. At db924ea1, body L12 ("D008 row proposed, not approved"), L19 ("Formal acceptance … still needed and is not claimed"), L51/L53/L55–58 ("still marked … owner pending formal review", "does not change the D008 wording") and L97 ("remain pending formal review") contradict the new head. Please update the body to cite d008-wave2-ownership-decision.md and the current head.

New in this increment (question)

Checks at db924ea1 (read just now): declarations, lint-and-smoke, pr-references, installed-package 3.10/3.11 and dashboard-browser succeeded. full-check 3.10/3.11 were still in progress.

Once the body is updated and full-check is green, I'll re-review and expect to approve if nothing else changes.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR review verdict — DO NOT PASS

Reviewed base fe16b29388de29708d30ea388b589355540f3172 and head db924ea165fbf69a8240745ee2eed1cb3c044eea.

Incremental re-review after head moved past cursor[bot] CR on f6371b5c… (ac69ce49 + db924ea1). Independent reviewer; not the PR author. Governing Issue #414.

Resolved since prior CR

  • D008 rows no longer say “owner pending formal review”. docs/contracts/d008-wave2-ownership-decision.md is a citable adoption record; it defines RSI-master as the architecture-coordination role delegated by Dong1017, links #414/#413, and separates architecture adoption from PR Approve / merge.
  • function-owners.md links the two B01 rows to that record, clarifies Proposed baseline vs adopted entries, and separates the authority paragraph from the table (blank line fixed).
  • Extra blank line before test_harness_writer_rejects_nan… removed.
  • Local focused B01 set on this head: 101 passed (legacy_run_v0_frozen, legacy_run_split, forwarder/import gates, moe_benchmark, test_h02_6…). Ready declarations / lint-and-smoke / pr-references / installed-package are success; full-check 3.10/3.11 were still in progress at review time on this docs-only increment.

Blockers (must clear before Approve)

  1. PR body is still stale relative to this head. It still says D008 is “proposed / pending formal review”, cites head f6371b5c, and claims the rows were deliberately not rewritten (e.g. body L7, L12, L19, L51–58, L97). That contradicts the adopted decision record and the current function-owners.md rows. Expected: refresh the PR body to cite d008-wave2-ownership-decision.md, drop the pending wording, and name the current head before re-requesting review.

Notes

  • Architecture adoption here is not a substitute for independent implementation Approve; this CR withholds Approve only for the stale body. Prior stack / signature / sha256 items remain resolved as previously recorded.
  • Confirm full-check 3.10/3.11 reach success on db924ea1… before merge.

No merge.

@Dong1017

Copy link
Copy Markdown
Collaborator Author

Posted by an agent on behalf of @Dong1017 (incremental review only; points already raised by Amy and cursor[bot] are not repeated).

Incremental note on head db924ea1 (merges cleanly into current main 57f8e8a4):

The new docs/contracts/d008-wave2-ownership-decision.md also adopts #413 / #425 ownership entries. L9 describes it as the citable record for both #414 (B01 / #422) and #413 (OBS / #425), and L25-45 adopt the Dashboard workflow, Profile records and Pre-profile searches entries. The PR body L99, however, says "K02, S01, and OBS (#425) are separate slices and are not in this PR", and #414 has no acceptance record for this cross-issue adoption. Merging #422 would land #425's ownership decisions in main without #425's reviewers seeing them.

Suggest either moving L25-45 to #425 (or a separate decision PR), or stating the cross-issue adoption explicitly in this body and recording acceptance in #414/#413 before merge. (Amy raised this as a question; on re-check I think it should be treated as blocking.)

History-only rewrite for PR 422. This commit has a single parent (current main)
and does not rebase or replay unsquashed #397 / H02 review-stack commits.

Method: merge-tree(current main, db924ea); not the raw db924ea tree
Official head at preparation: db924ea
Current main at preparation: 57f8e8a

No file-content change relative to that method's resulting tree.
Local candidate only; no push.

Refs #414

Agent-assisted-by: Grok Bot (decision record and comment fixes)
Agent-assisted-by: Claude Code / claude-fable-5-2 [effort=xhigh] (prior architecture review)
Agent-assisted-by: Claude Code / claude-fable-5-2 [effort=xhigh] (architecture review and draft)
Agent-assisted-by: Grok Bot (architecture decision integration)
Agent-assisted-by: Grok Bot
Agent-assisted-by: Codex (author)
Agent-assisted-by: Claude Code / claude-fable-5-2 effort=xhigh (author)
Signed-off-by: GUOGUO <xwdong1998@163.com>
@Dong1017
Dong1017 force-pushed the impl/b01-run-evaluate branch from db924ea to d5f6e02 Compare November 14, 2026 11:36

Copy link
Copy Markdown
Collaborator

Re-checked db924ea165fbf69a8240745ee2eed1cb3c044eea...d5f6e02bc48ae22d2fbb2d9ce718265e5395708c. d5f6e02b is a single-parent squash onto main 57f8e8a4. I ran git merge-tree --write-tree 57f8e8a4 db924ea1 locally. It merges with no conflicts and gives tree 268da5969fd28bb535dc4bd8e0225e7ac3637108, which is exactly the tree of d5f6e02b. So the only content change from my last review (6873299097) is incoming main. Not approving yet.

Resolved since 6873299097

  • The body now cites d008-wave2-ownership-decision.md and drops the "pending formal review" wording (L12, L19, L60-62).

Still open

  1. Cross-issue adoption (blocking). The decision file is unchanged (blob bc6036cb, the same as observability: dissolve the transition package into H records, Apps views, and K (OBS) #425), so L25-45 still adopt task: OBS — 解散 observability 过渡包并归位记录与展示 #413 / observability: dissolve the transition package into H records, Apps views, and K (OBS) #425 entries: Dashboard workflow, Profile records and Pre-profile searches. Body L104 still says "OBS (observability: dissolve the transition package into H records, Apps views, and K (OBS) #425) [is a] separate slice and [is] not in this PR". This is the point Dong1017's note 6874272179 marked as blocking. Please either move those sections out of benchmark: split the legacy run/evaluate chain into B, H, and Apps legacy (B01) #422, or state the cross-issue adoption in the body and record acceptance on task: B01 — 旧 run/evaluate 整链迁入最终归属 #414/task: OBS — 解散 observability 过渡包并归位记录与展示 #413.
  2. The body is stale again for the new head. L19, L21, L54 and L58 still give db924ea1 as the current HEAD, and L12/L60-62 link to blobs at db924ea1. Please update the body for the squash d5f6e02b and its history method.
  3. The D008 delegation confirmation on task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 that I asked for earlier is still non-blocking.

Checks on d5f6e02b (read just now): pr-references, declarations, lint-and-smoke, installed-package 3.10/3.11 and dashboard-browser succeeded. full-check 3.10/3.11 are still in progress.

I did not run tests this round.

cursor[bot]
cursor Bot previously approved these changes Nov 14, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

PR review verdict — PASS
Reviewed base 57f8e8a4915b38201d9fa58d4d05acbe3c4199ee and head d5f6e02bc48ae22d2fbb2d9ce718265e5395708c. Governing Issue #414 (B01). Independent reviewer via cursor[bot]; not the PR author.

Prior sole blocker (CR 5902400218 on db924ea) — resolved

  • PR body no longer claims D008 pending / cites wrong head. It cites docs/contracts/d008-wave2-ownership-decision.md and current head d5f6e02….
  • function-owners.md L22–23 link the adopted decision record (no “owner pending formal review”).
  • History rewrite: single commit on parent 57f8e8a…; three-dot vs that base is 52 files, +1335/−112, B01-only (no #397 Apps tree).

Verified on this head

  • Spot-check: apps/legacy/run.py composition, harness/atomic_json.py, benchmark/sampling.py move, root evaluation/execution/reporting forwarders remain; sha256 pin test present; first_wave_report() still reports legacy run forwarders: 3 and isolation_complete=False.
  • Local Linux focused set: 101 passed in 9.05s (test_legacy_run_v0_frozen, test_legacy_run_split, forwarder/import gates, test_moe_benchmark, test_h02_6…).
  • Ready CI on d5f6e02: lint-and-smoke, declarations, pr-references, installed-package 3.10/3.11, full-check 3.10/3.11, dashboard-browser, knowledge-model-policy — all success.
  • Earlier stack / H-signature disclosure / D008 adoption / sha256 pin remain resolved as previously recorded.

Notes (non-blocking)

  • Body’s timed “full-check … in_progress” read-back is superseded by green Ready CI on this head.
  • origin/main has advanced past the PR base (e.g. #426); GitHub still mergeable=true and a local merge-tree write succeeded.

No campaign PASS claimed. No merge by this review.

Ordinary merge of main at acc881f, which
moved the packaged Knowledge corpus to knowledge/data and the repository
pack descriptions to harness/data. No rebase, cherry-pick or history
rewrite.

The merge had no textual conflict. Git merged docs/architecture.md,
docs/module-layout.md and src/inferrsi/cli.py automatically; each keeps
the K02 text and paths together with the old-run split changes.

Agent-assisted-by: Claude Code / claude-fable-5-2 [effort=xhigh] (merge integration)
Signed-off-by: GUOGUO <xwdong1998@163.com>
Ordinary merge of accepted main 37220b1. Preserve the original PR ancestry and canonical D008 record, matching function-owner headers, and package payload bytes.

Keep the deferred execution/run suffix AST unchanged and import-isolation regression passing.

Verification: changed CLI Ruff E/F/I, diff check, byte/AST invariants and 1 focused import-isolation test passed on this exact staged tree. Prior broader evidence remains tied to its recorded tree.

Assisted-by: Grok Bot (deterministic integration and validation; no coding-model call).
Signed-off-by: GUOGUO <xwdong1998@163.com>
Local candidate only: bounded Windows regression validation remains incomplete.

Signed-off-by: GUOGUO <xwdong1998@163.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

PR review verdict — PASS
Reviewed base 8c321005d50c2ecef61bf20132409d88474a6450 and head e427c42c535ddb49fce0d6943b44044e03f11cf2. Governing Issue #414 (B01). Independent reviewer via cursor[bot]; not the PR author.

Prior blockers re-checked on this head — resolved

  • Stack / #397: three-dot vs current main is B01-only (51 files, +1301/−110); no apps/api|archive|result / Apps-output stack residue. mergeable=true; local merge-tree clean. #397 already on main.
  • H signatures: CHANGELOG + PR body disclose metric= / required ports; compat remains in apps.legacy.run and root execution.py. In-tree callers updated; H defaults that would recreate H→B/Apps/infra edges were not restored.
  • D008 adoption: docs/contracts/d008-wave2-ownership-decision.md is citable; function-owners.md rows link adopted decisions; no “owner pending formal review”.
  • Stale body (prior CR on db924ea): body cites current head e427c42…, the decision record, and B01 scope. Timed “full-check in_progress” read-back is superseded by green Ready CI on this exact head (non-blocking).

Verified on this head

  • Ready CI on e427c42: lint-and-smoke, declarations, pr-references, installed-package 3.10/3.11, full-check 3.10/3.11, dashboard-browser, knowledge-model-policy (N/A success) — all success.
  • Local Linux focused set: 102 passed, 4 subtests (test_legacy_run_v0_frozen, test_legacy_run_split incl. sha256/writer pins, forwarder/import gates, test_moe_benchmark, test_h02_6…).
  • first_wave_report(): isolation_complete=False; blockers include legacy run forwarders: 3; no absent: benchmark/.
  • Spot-check: Apps legacy composition, H atomic_json, B sampling (no H/infra imports), deleted H evaluation/reporting/media shells, root forwarders retained for R01.

No campaign PASS claimed. No merge by this review.

Copy link
Copy Markdown
Collaborator

Incremental review: d5f6e02bc48ae22d2fbb2d9ce718265e5395708c...e427c42c535ddb49fce0d6943b44044e03f11cf2. That's three merge commits: a663fcc7 (main K02), 0c056b93 (main read routing) and e427c42c (main 8c321005, #437, still current main). Not approving yet.

How I checked the merges: I compared the net diff against the base before (57f8e8a4...d5f6e02b: 52 files, +1335/−112) and after (8c321005...e427c42c: 51 files, +1301/−110). 44 files have identical patches. The seven that differ (docs/architecture.md, docs/module-layout.md, docs/contracts/function-owners.md, src/inferrsi/cli.py, src/inferrsi/harness/README.md, tests/_gate_policy_support.py, tests/test_target_module_import_gate.py) differ only in S01/K02 context that came from main. The +/− counts per file are unchanged, apart from one line in architecture.md and the README re-wrap. The missing file is src/inferrsi/harness/atomic_json.py. #437 (8c321005) already put it on main, byte-identical to this branch's earlier version (blob 056b7ee2), so it's no longer in the diff. I found no unexpected changes beyond the merges.

Resolved since my last comment (6876763192)

  • The body is current for e427c42c: head, parents, tree, history and blob links (L19-27, L63-66).
  • Hosted checks on this head are green, per cursor[bot] review 5908339081 and body L25. I didn't run tests locally this round.

Still open

  1. Cross-issue adoption (blocking). docs/contracts/d008-wave2-ownership-decision.md has the same patch as on d5f6e02b. At this head, L9 still calls it the record for both task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 and task: OBS — 解散 observability 过渡包并归位记录与展示 #413 (OBS / observability: dissolve the transition package into H records, Apps views, and K (OBS) #425). The "Dashboard workflow", "Profile records and overview" and "Pre-profile searches" sections still adopt task: OBS — 解散 observability 过渡包并归位记录与展示 #413 / observability: dissolve the transition package into H records, Apps views, and K (OBS) #425 entries. Meanwhile body L116 still says "OBS (observability: dissolve the transition package into H records, Apps views, and K (OBS) #425) [is a] separate slice and [is] not in this PR". task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 and task: OBS — 解散 observability 过渡包并归位记录与展示 #413 have no new comments since 2026-11-14, so no cross-issue acceptance is recorded yet. Please either move those sections out of benchmark: split the legacy run/evaluate chain into B, H, and Apps legacy (B01) #422 (observability: dissolve the transition package into H records, Apps views, and K (OBS) #425 has the same file), or state the cross-issue adoption in this body and record acceptance on task: B01 — 旧 run/evaluate 整链迁入最终归属 #414/task: OBS — 解散 observability 过渡包并归位记录与展示 #413.

Non-blocking

Copy link
Copy Markdown
Collaborator Author

@amy-why-3459 @zengchuang-hw — scope clarification for e427c42: this PR delivers B01 implementation plus the shared five-entry D008 architecture metadata for Issue 414 and Issue 413. I retain the OBS ownership sections in that joint record; OBS implementation remains in PR 425. This is an architecture decision, not independent implementation acceptance. Please review correctness and boundaries on this current head.

Copy link
Copy Markdown
Collaborator

Re-check on the same head e427c42c535ddb49fce0d6943b44044e03f11cf2, in reply to 6887367230. No new commits since my last comment (6887178900), so I didn't re-read the diff. Still not approving, but only body wording is left.

Resolved: the cross-issue adoption is now on record on both governing issues: #414 6887366262 and #413 6887365381. Both adopt the same five-entry D008 record at this head. So the substance of my blocking item 1 is resolved, and the D008 sections can stay in #422.

Still open: the PR body contradicts this.

  1. Body L116 still says "K02, S01, and OBS (observability: dissolve the transition package into H records, Apps views, and K (OBS) #425) are separate slices and are not in this PR", and L15 lists OBS under "What it does not deliver". Neither line says that d008-wave2-ownership-decision.md also carries the OBS / task: OBS — 解散 observability 过渡包并归位记录与展示 #413 ownership entries. Please add one line to the body with the scope you put in 6887367230: B01 implementation plus the shared five-entry D008 metadata for task: B01 — 旧 run/evaluate 整链迁入最终归属 #414 and task: OBS — 解散 observability 过渡包并归位记录与展示 #413, with OBS implementation staying in observability: dissolve the transition package into H records, Apps views, and K (OBS) #425. Link the two adoption comments above.
  2. Non-blocking, same as last time: L7 ("add H-owned atomic JSON writes") and L11 (inferrsi.harness.atomic_json) still list the atomic JSON writer as delivered here. It landed on main with skill-eval: split the four-file family by function owner and drop the residual (S01) #437 and isn't in this diff any more.

Once the body is updated, I'll approve if nothing else changes.

@amy-why-3459 amy-why-3459 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.

LGTM

Same head e427c42c535ddb49fce0d6943b44044e03f11cf2. No new commits since my code review in 6887178900. The only change since my last comment (6887410296) is the PR body edit at 01:40Z.

What I checked:

  • The body now says it carries the shared five-entry D008 metadata for #414/#413, links the adoption comments on #414 (6887366262) and #413 (6887365381), and keeps the OBS implementation in #425. L15 and L116 no longer contradict d008-wave2-ownership-decision.md, so the blocking item is resolved.
  • The non-blocking atomic_json wording is fixed: the body now says it already landed with #437 and is unchanged here.
  • Reviews on this head: cursor[bot] approved (5908339081). No unresolved CHANGES_REQUESTED or other blocking review on e427c42c.

@Dong1017
Dong1017 merged commit 9ede183 into main Nov 15, 2026
12 checks passed
@Dong1017
Dong1017 deleted the impl/b01-run-evaluate branch November 15, 2026 02:55
Dong1017 added a commit that referenced this pull request Nov 15, 2026
Merge current main9ede1835 (#422) into OBS. Preserve both changelog sections and all five D008 owner rows. Adapt workflow test imports to B01 Apps legacy composition and OBS Apps views. No #428 layout decision is made.

Local merge preserved before exact-new-head Linux validation; prior5ccb2b7 test results do not validate this head. Windows isolated controls remain inconclusive.

Agent-assisted-by: Grok Bot (deterministic merge resolution; no new coding-model request)
Signed-off-by: GUOGUO <xwdong1998@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:harness Harness module: profile, A/B loop, quality gates, evidence reports kind:task Bounded delivery task with a delegated slice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants