Impl/topic24

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 9 个变更文件

📁 .github/workflows/ci.yml

This change is clean and follows existing patterns well. No blockers found.

🟡 Observation — Unconditional CI runs

The new test and benchmark steps run on every PR without an if: guard. This is consistent with the adjacent steps, so not a bug — just noting that if these are expensive to run, consider gating on main only like the visualization step below does.

💭 Nit — Summary ordering

The spike_tools_report.md block is inserted before the existing echo "", which means the spacing between reports is consistent. Good.

Overall: Straightforward addition, properly structured, no correctness or security concerns.

---

📁 benchmarks/run_topic24_spike_case.py

🔴 Non-hermetic subprocess probe — run_probe_subprocess() (line ~437) passes the host os.environ (minus SCRATCHV_SPIKE_*). If CI has a real Spike on PATH, --probe-spike will resolve it and probe["backend"] may not equal {"kind": "emulator", "spike_style": True}, violating the probe_backend_marked_emulator check. The docstring claims the case is hermetic — it isn't for this sub-case.
Suggestion: set PATH to an empty/minimal directory in the subprocess env, or inject a fake spike that satisfies the probe without a real installation.

🟡 Unhandled subprocess.TimeoutExpired — line ~457. A hung run_spike_bench.py raises an uncaught exception, crashing the entire benchmark with no report written. Wrap in try/except subprocess.TimeoutExpired and return a failure dict.

🟡 Fragile subprocess.run monkey-patch — line ~479: spike_sim.subprocess.run = lambda ... in run_parser_tolerance. If spike_sim re-imports subprocess internally or another test module patches subprocess.run, this breaks silently. Prefer unittest.mock.patch("scratchv.standalone.spike_sim.subprocess.run").

🟡 Typo inconsistency in canned fixture — CANNED_STDERR uses "Commited" (misspelled) while POISONED_ENV_STDERR uses "Committed". If this is deliberate parser-tolerance testing, add a comment. If accidental, fix one — a silent parser mismatch here would mask a real regression.

🟡 POSIX-only shell stubs — make_fake_tool writes #!/bin/sh scripts. If CI ever runs on Windows, chmod +x and #!/bin/sh are no-ops. Either guard with sys.platform or document the Linux-only requirement.

💭 _patched_module which parameter (line ~99) has no type annotation — add Callable[[str], str | None] | None.

💭 Dual control of PATH resolution — _patched_module patches spike_sim.shutil.which to always return None, while _scenario also passes which= to resolve_spike_tools. One mechanism is dead code in every resolution-matrix scenario. Pick one and remove the other to reduce confusion.

💭 677 lines in a single benchmark file — consider splitting build_resolution_matrix, run_cli_probe, run_degradation_probe, run_probe_subprocess, run_parser_tolerance into a benchmark_cases/ package for maintainability.

---

📁 docs/ARCHITECTURE.md

🟡 Resolution priority chain hardcoded — If resolve_spike_tools() is refactored (e.g., SCRATCHV_SPIKE_HOME renamed, new fallback added), this doc silently goes stale with no drift detection. Consider either (a) generating this line from a constant in the source, or (b) adding a doc-consistency test that asserts the chain in the doc matches the code.

🟡 Dangling reference — "详见课题 24 设计文档" has no path or link. A reader can't jump to it. Add a relative link like [课题 24](../design/24-spike-tool-resolution.md).

💭 Vague degradation description — "skip / 告警分层降级" — which tool skips, which warns? If the behavior differs per tool, a one-line table (tool → behavior on missing) would be more actionable than prose.

---

📁 docs/topics/24-Spike仿真-开发文档.md

Review: docs/topics/24-Spike仿真-开发文档.md

🔴 Blockers

1. is_executable 未捕获 OSError — §2.4 代码片段中 os.access(path, os.X_OK) 在某些系统（root 访问其他用户文件、mount 点）会抛 PermissionError/OSError，导致整个 resolver 崩溃而非跳过候选项。应 try/except OSError: return False。

2. SpikeTools frozen dataclass 内含可变 dict — sources: dict 和 candidates: dict 用了 field(default_factory=dict)。frozen=True 只锁顶层赋值，内部 dict 仍可被外部 tools.sources["spike"] = "x" 篡改。建议改为 Mapping[str, str] 类型或构造后转为 types.MappingProxyType，或在 as_dict 中返回 dict(...) 的防御性拷贝。

🟡 Suggestions

3. missing 属性名 vs as_dict key 不一致 — missing 返回 "spike-dasm" / "spike-log-parser"（连字符），as_dict() 用 "spike_dasm" / "spike_log_parser"（下划线）。消费方（尤其 run_spike_bench.py 的 probe 输出）容易混用。建议统一为下划线或连字符，并在 §1.2 注明规范名映射。

4. parse_commit_stats MIPS 正则未定义多核场景 — r"([\d.]+)\s*MIPS" 只匹配第一个匹配。若 spike 输出多核行（core 0: ... 1.5 MIPS / core 1: ... 2.0 MIPS），行为不确定。应在 §2.6 明确：取首行？取最大值？取总和？

5. 缺少"exit 0 但无可解析统计段 → failed/-2"的测试用例 — §1.5 定义了此防呆逻辑（疑似非 Spike 可执行文件），但 §5.2 用例表中无对应用例。建议新增 test_exit_zero_no_stats_becomes_failed，用 mock 子进程返回 returncode=0、stdout/stderr 不含任何统计段头。

6. §2.6 告警职责模糊 — 推荐方案说"缺失段头时 run_spike 自行 append warning"，但 parse_commit_stats 缺段返回 (0, 0.0)。run_spike 如何判断"段头不存在"与"值确实为 0"？若 stderr 有 Commited 0 instructions 也算正常 0，两者无法区分。建议：解析函数额外返回 (values, warnings) 或返回一个带 found: bool 的结构，让 run_spike 无需猜测。

7. parse_pc_histogram 未定义 EOF 行为 — §2.6 说"空行结束"，但若 PC histogram 段在输出末尾且无尾随空行，解析应到 EOF 结束。正则 r"^(0x[0-9a-fA-F]+):\s*(\d+)$" 逐行匹配天然容忍 EOF，但文档未明确，实现者可能提前 break。

💭 Nits

8. §2.10 最终返回值 — EXIT_OK if result.status == "ok" else EXIT_RUN_FAIL 在 skip 路径已提前 return，所以不会误触。但若未来有人把 skip 分支移到后面，此处会错误返回 EXIT_RUN_FAIL。可改为显式白名单：if result.status in ("ok", "skipped"): return EXIT_OK。

9. _resolve_one 层级间告警不对称 — CLI/env 层无效时告警，home/PATH/common/legacy 层无效时静默跳过。这是合理的（显式输入才值得告警），但建议在 §2.4 docstring 中注明此设计意图，减少后续审查者困惑。

10. 用例 14 仅列在表格中 — test_report_status_fields 在 §5.2 表格中列出但 §5.3 无示例代码。考虑到报告格式（文本/JSON）是本次改动的高风险区域，建议补一段实现示例或至少注明断言哪些具体键。

---

📁 docs/topics/24-Spike仿真-设计文档.md

🔴 Bug: exit_code=-2 哨兵值语义冲突 — 场景 8b（spike 退出 0 但无可解析段）复用 -2，但 -2 在本设计和旧代码中均表示"工具缺失"。下游如果依赖 -2 判断是否需要跳过，会误判一次真实执行失败为"环境不具备"。
建议：为"执行成功但无有效输出"分配独立哨兵值（如 -3），或将该场景统一映射为 exit_code=0 + status=failed + parse_warnings，不污染既有语义。

🔴 缺失：--require-spike + --json 交互未定义 — 场景 4（CLI 路径无效）和场景 1（--require-spike 缺 spike）均写"无报告（仅 stderr）"。但用户同时传 --json 时是静默丢弃 JSON 输出，还是仍输出含 status=failed 的 JSON？应明确契约，避免 CI 脚本因无 JSON 而解析空输出。
建议：规定 --json 时无论退出码如何，stdout 始终输出含 status/skip_reason 的 JSON，stderr 打印 ERROR/SKIP 日志；或在设计文档中明确 "严格模式下 JSON 不输出"。

🟡 JSON exit_code 字段 vs 进程退出码混淆 — skip 示例中进程退出码为 0，但 JSON 内 "exit_code": -2。文档未在契约层显式区分这两个概念。建议在 §2.3 退出码契约旁加一句："exit_code" 字段为 Spike 结果层哨兵值，与进程退出码独立；进程退出码见 EXIT_OK/EXIT_RUN_FAIL/EXIT_CONFIG。

🟡 BNF 语义歧义 — resolve(tool) ::= cli | env | spike_home | ... 用 | 表达的是"有序回退"而非"择一"。读者可能误解为并行竞争。建议改为 ::= cli(tool) ?? env(tool) ?? spike_home(tool) ?? ...（?? 表示顺序回退），或在下方注明 "| 此处表示优先级回退，非并择"。

🟡 SpikeResult 默认 status 未定义 — 文档说"既有字段与拼写保持不变"，但新增的 status 字段在旧消费方（ci_benchmark.py 等）未传入时默认值是什么？若默认空串，generate_spike_report() 是否打印 Status: 空行？建议指定 status 默认值为 "ok"，并在 SpikeResult 数据类 docstring 中注明。

🟡 COMMON_SPIKE_DIRS 含 ~ 路径但未声明展开时机 — /opt/riscv/bin 等无问题，但 ~/riscv/bin、~/.local/bin 需要 os.path.expanduser()。§2.1 只在 env 值处理中提及 expanduser，未在 common 层说明。建议在 2.1 关键规则中补一条：common 层候选路径在匹配前执行 expanduser。

🟡 测试用例 5 应拆分 — 该用例混合了报告字段验证、commit 解析、cache 解析、PC 直方图解析四个独立关注点。任一断言失败无法定位根因。建议拆为 5a（报告字段）、5b（commit/cache 解析）、5c（PC 直方图），共享同一组固定样本 fixture。

💭 JSON 字段命名不一致 — 工具名用连字符（spike-dasm），JSON key 用下划线（spike_dasm）。建议在 §2.2 末尾加一条命名约定说明，避免实现时随意选择。

💭 行号引用会腐化 — 全文多处 spike_sim.py:37-39、:348-423 等行号，实现后必然偏移。建议使用锚点注释（如 # MARK: legacy-constants）替代行号，或在首次引用时注明"行号基于基线 commit SHA"。

💭 场景 8b 报告状态列歧义 — 表格中 failed + parse_warnings 看起来像两个并列状态，实际是 status=failed 且 parse_warnings 非空。建议改写为 failed（parse_warnings 非空），与其他行格式对齐。

---

📁 docs/topics/24-Spike仿真.md

🟡 Inconsistent env var documentation — Usage section documents SCRATCHV_SPIKE_HOME but pitfalls table adds SCRATCHV_SPIKE_BIN to the resolution chain. The SCRATCHV_SPIKE_BIN variable is never explained in the "运行方式" section.
Suggestion: Add SCRATCHV_SPIKE_BIN (direct path to binary) to the env-var example, or remove it from the resolution order if it's internal-only.

🟡 Ambiguous $HOME in comment — Line # 约定 $HOME/bin/spike is easily misread as the shell's $HOME (/root, /home/user), when it means $SCRATCHV_SPIKE_HOME.
Suggestion: Change to # 约定 $SCRATCHV_SPIKE_HOME/bin/spike.

💭 Minor: orphan / line break — The --json fields list wraps with a dangling / at end of line before spike_tools. Harmless but slightly unusual formatting; consider using a bullet list or putting all fields on one line for clarity.

-  / `spike_tools`
-  / `parse_warnings` / `tool_warnings` 字段
+  字段：`status`, `skip_reason`, `spike_binary`, `spike_tools`,
+  `parse_warnings`, `tool_warnings`

---

📁 scratchv/standalone/run_spike_bench.py

🔴 Import path may fail — probe_spike_tools(): from scratchv.standalone.spike_sim import ...
When run as python scratchv/standalone/run_spike_bench.py, sys.path[0] is the script's directory (scratchv/standalone/), so the scratchv package isn't on the path and this raises ModuleNotFoundError (caught as ImportError, but only prints a probe failure instead of revealing the real issue).
Suggestion: match the import style used elsewhere in this file, or fall back to from spike_sim import ... since both files live in the same directory:

try:
    from scratchv.standalone.spike_sim import (SpikeConfigError, resolve_spike_tools)
except ImportError:
    from spike_sim import (SpikeConfigError, resolve_spike_tools)

🟡 tools.sources may be None — parts.append(f"{name}=… ({tools.sources.get(name, 'missing')})")
If resolve_spike_tools() returns a tools object with sources=None, this crashes with AttributeError after the success path has already been reached — turning a successful probe into a hard failure.
Suggestion: src = tools.sources or {}, then src.get(name, 'unknown').

🟡 Probe result is silently dropped when the flag is omitted, but attached when set — asymmetric contract
--probe-spike prints to stderr but has no effect on stdout output unless the user also passes --json. Consider stating this in the --help text, e.g. "…and (with --json) embeds resolved paths in the report."

🟡 Narrow exception handling may mask real failures
Only ImportError and SpikeConfigError are caught. If resolve_spike_tools() raises anything else (e.g. OSError from a shell probe, FileNotFoundError from a stat call), the whole script crashes on an opt-in flag that's meant to be best-effort.
Suggestion: either widen to except Exception as e: with a stderr note, or document that only those two error types are handled.

🟡 spike_tools.as_dict() output shape is opaque to report consumers
report["spike_tools"] is a free-form dict. Since this file emphasizes that the backend is the emulator (per the new docstring and "backend" field), future readers may conflate the presence of spike_tools with "spike was used for numbers." Consider adding a "used_for_stats": false marker inside spike_tools or renaming to "spike_tools_probe" to make the distinction explicit.

💭 Consistency nit — "spike_style": True in the backend dict is a bit ambiguous (does it mean "Spike-like ISA" or "emulates Spike behavior"?). A short key like "isa": "rv64im" or a comment in the docstring would help future readers.

💭 Missing test coverage for probe_spike_tools() success/failure/missing-source branches. Given the file appears to be a standalone script, a small inline self-test or unit test in a nearby tests/ module would catch the sources=None case above.

---

📁 tests/test_spike_sim_paths.py

🔴 None — no blockers found.

🟡 Fragile string assertion — test_main_optional_invalid_cli_still_runs:
assert "Committed insns:" in captured.out couples the test to a text-report format string. If the wording changes (e.g. "Committed instructions"), the test breaks even though behavior is correct. Consider running with --json and asserting report["committed_insns"] == 1234 instead.

🟡 Autouse fixture hides baseline state — hermetic_env:
The fixture globally monkeypatches shutil.which → None on every test. Tests that need real which behavior (e.g. test_resolution_env_over_path) must re-patch, but a new test author won't know to check this. Add a one-line docstring note: """Also: shutil.which returns None by default — re-patch if needed."""

🟡 Overly strict empty-list assertion — test_run_spike_canned_output_is_ok:
assert result.parse_warnings == [] will break if a benign, non-error warning is ever added (e.g. "optional tools not configured"). Consider asserting not any("error" in w.lower() for w in result.parse_warnings) or checking a specific absence instead of requiring the list to be empty.

🟡 Missing success-path test for run_spike_with_log —
Only the OSError path is tested (test_run_spike_with_log_oserror_maps_to_failed). The success path (stdout + log content both returned) is untested, so regressions there would be silent.

💭 Nit: CANNED_STDERR uses "Commited" (Spike's actual misspelling) but the comma-separated test case in test_parse_commit_stats uses "Committed". A brief comment noting that the parser must accept both spellings would help future readers.

---

📁 tests/test_topic24_spike_case_report.py

🔴 Bug: cache_miss_rate expectation doesn't match the inputs — Line 152: cache_hits=10_000, cache_misses=25, but cache_miss_rate == 0.25. No standard formula (misses/(hits+misses)=0.00249, misses/hits=0.0025) yields 0.25. Either the parser is mis-parsing (e.g., reading "25" as "25%" of some other field) or the test expectation is wrong — if the latter, the parser bug it would catch is now hidden. Verify the parser formula and fix whichever is broken.

🟡 Float equality is brittle — Lines 151, 152: tolerance["mips"] == 42.0 and cache_miss_rate == 0.25. Use pytest.approx(...). If the parser ever emits 42.0000000001 from float division, this test flakes and teaches nothing.

🟡 Subset assertion is weaker than intended — Line 39: ALL_SOURCES <= {row["observed_source"] for row in matrix} only proves each source appears somewhere. It doesn't prove each source is wired to the right scenario. A swap between env and path rows would pass. The per-scenario assertions below partially cover this, but consider also asserting {row["observed_source"] for row in matrix} == ALL_SOURCES (equality, not subset) to catch spurious extra sources, and keying by_scenario defensively so a duplicate scenario key fails loudly rather than silently overwriting.

🟡 test_hard_check_gate_is_not_vacuous trusts the monkeypatch target — Lines 178-181 patch benchmarks.run_topic24_spike_case.build_resolution_matrix. This only works because evaluate/main resolve the name from module globals at call time. If anyone later refactors to from .run_topic24_spike_case import build_resolution_matrix inside the target module (or passes it as a closure arg), the patch silently becomes a no-op and this test turns into a fake green. Add a sanity assertion right after monkeypatch.setattr — e.g. call build_resolution_matrix via the module attribute and assert it returns the broken row — so a future refactor fails the test for the right reason.

💭 Hardcoded scenario names are duplicated — by_scenario["clean_missing"], ["cli_beats_env"], etc. are string literals repeated across the file. If the production scenario name changes, only the KeyError reveals it. Consider a SCENARIOS = {...} constant next to ALL_SOURCES to make the contract explicit in one place.

💭 test_main_writes_json_and_markdown mixes two concerns — It validates the happy-path artifacts and the unknown-case SystemExit. Split into two tests so a failure in one doesn't hide the other, and so capsys.readouterr().out (which only asserts non-empty — see line 148) can be replaced with an actual content assertion in its own test.

💭 assert capsys.readouterr().out — Line 148: asserting truthiness is near-vacuous (a single stray print passes). Either assert on a specific substring or drop it.

---