Impl/topic07

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 1 个文件超过上限（最多 10 个）未审查

📁 .github/workflows/ci.yml

No blockers. One nit:

💭 Nit: Naming inconsistency — Step names use "topic07 logger regressions" (lowercase, no space) but "Topic 07 logger case report" (title case, with space). Pick one convention.

The rest looks clean and follows existing patterns correctly — shell syntax is valid, YAML structure is consistent, summary block mirrors the conditional pattern used for other reports.

---

📁 benchmarks/cases/topic07_logger_feature.dsl

🔴 Undefined variables acc and x — Lines 6–8: Both are referenced but never declared or initialized. acc must start at 0 before add(acc, x); x has no binding at all.

x = 3          # declare input
acc = 0        # initialize accumulator
for i = 0, 4
  acc = add(acc, x)
endfor
t = mul(acc, x)
return t

🔴 No expected output / assertion — The comment promises "byte-identical output" but nothing actually validates it. If this is a benchmark regression gate, it will silently pass even when the emitted artifact changes.

💭 Loop bound ambiguity — for i = 0, 4 — inclusive or exclusive upper bound? 5 iterations vs 4 produces t = 15*x vs 12*x. Pin the semantics in the comment or use a different syntax so the intent is unambiguous.

💭 No function boundary — The file is bare statements. If other cases in topic07 wrap logic in func ... endfunc, this should match for consistency (and so the parser's function-scope codegen path actually gets exercised).

---

📁 benchmarks/run_topic07_logger_case.py

🔴 Bug: render_markdown called twice in main — Lines 537-539: render_markdown(report) is invoked once for stdout and again for file write, duplicating work on every run.
Suggestion: md = render_markdown(report) once, then use md for both print(md) and args.markdown.write_text(md + "\n").

🟡 Fragile log parsing — _count_levels (line 87) splits on whitespace and indexes parts[2] to find the level. Any change to the log format (timestamp width, extra columns) silently breaks level counting with no error signal. _debug_markers and _phase_coverage are similarly brittle substring matches against hardcoded log templates.
Suggestion: Add a module-level constant or docstring documenting the exact log line format these parsers depend on, so future log-format changes trigger an obvious update here.

🟡 _sha256_file returns "" for missing files — Line 70. If both output files are absent, "" == "" passes the byte-identical check while nothing was actually compared. The output_bytes and success guards mostly mitigate this, but the asymmetry between _sha256_file ("") and _sha256_text("") (valid hex) is confusing.
Suggestion: Return None for missing files and skip the comparison when either side is None, or assert both exist before hashing.

🟡 overhead_pct division by near-zero — Line 385: if plain["compile_ms_median"] is tiny (sub-microsecond), the percentage explodes.
Suggestion: Guard with a minimum base (e.g., max(plain["compile_ms_median"], 0.1)), or report "n/a" when the base is below a threshold.

🟡 measure_logged_run discards all but the last result — Line 120: the loop overwrites result every iteration. With repeats > 1, only the last compile's errors are reported. If earlier runs produced different error output, it's invisible.
Suggestion: If per-repeat error diversity matters, collect all errors lists; otherwise document that only the final run is inspected.

🟡 _handler_count only inspects "scratchv" logger — Line 75: if init_logger attaches handlers to root or other named loggers, the count underreports. The measure_handler_lifecycle check (max(counts) > 2) silently passes if handlers land elsewhere.
Suggestion: Consider walking the entire logger tree, or document that only the "scratchv" logger is in scope.

💭 FAILURE_SOURCE magic string — Line 58: "add(a, b\n" is a single unterminated argument. The comment explains it, but a named constant like _SYNTAX_ERROR_CASE with a brief docstring would be clearer than a module-level constant with a trailing \n that looks like a typo.

💭 datetime.now(timezone.utc) — Line 391: prefer datetime.now(timezone.utc).replace(microsecond=0) for more readable report timestamps, or use isoformat(timespec="seconds").

---

📁 docs/topics/07-编译器日志增强器-开发文档.md

Code Review: docs/topics/07-编译器日志增强器-开发文档.md

---

🟡 文档一致性问题

1. 代码示例 Python 版本不兼容 — 全文使用 str | None / logging.Logger | None 联合类型语法，但文档自身声明 venv 为 Python 3.8.10（§五注意事项），该语法需 3.10+。实现者直接复制代码会报语法错误。建议在示例前统一加 from __future__ import annotations 或改用 Optional[str]。

2. F9 未定义 — §2.5 用例描述引用"评审修复轮补充（F1–F6/F9）"，但 §2.1–§2.5 的所有变更表中均无 F9 条目。建议补充定义或删除引用。

3. C7 条件逻辑自相矛盾 — 正文描述判定改为 self._log is None or not is_initialized()，但参考实现仅写了 self._log is None。后者无法覆盖 shutdown() 后 _log 仍非 None 的场景（F5 的 shutdown 重建用例会失败）。参考实现应与正文一致。

4. C11 与 §1.5 logger 命名矛盾 — 文档"实现结果"节承认此矛盾但未修正正文。建议直接更新 §1.5 或 C11 使之一致，避免实现者无所适从。

---

🟡 结构与可维护性

5. "实现结果"节不属于开发文档 — 末尾 638 行追加的集成结果、commit hash、测试数字是事后记录，放在指导实现的文档中会模糊"设计 vs 实际"的边界。建议移至 docs/topics/07-开发文档-实施记录.md 或在文档顶部加明确的分隔标注。

6. 同一内容三处重复 — 阶段埋点信息在 §1.5（常量表）、§2.2（变更表 C8）、§3.1（埋点清单）分别描述。§1.5 与 §3.1 列名/消息模板不完全一致（如 §3.1 多了 FAILED 后缀），后续修改易遗漏。建议 §1.5 定义单一 source of truth，§3.1 引用而非重复。

---

💭 小问题

7. §2.5 夹具路径硬编码 benchmarks/cases/001_simple_add.dsl，该文件不存在时所有 7 个行为级用例级联失败。建议在文档中注明该文件的创建方式或 fallback。

8. §3.5 控制台示例的时间戳 01:16:44 无年份/日期，文件示例有 2026-09-14 01:16:44。若 _ColorFormatter 使用 %H:%M:%S 格式，调试时会缺少日期上下文。

---

📁 docs/topics/07-编译器日志增强器-设计文档.md

🔴 Bug: str | None 类型语法与 Python 3.8 不兼容 — 文档多处使用 str | None（如 CompilerConfig.log_file: str | None = None），但 §4.6 明确 venv 为 Python 3.8.10。X | Y 联合类型需 3.10+，3.8 下 dataclass 字段会抛 TypeError。需改为 Optional[str] 或加 from __future__ import annotations。

🟡 use_logger 与 IR 校验耦合被 --log-file 放大 — §4.7 已记录，但新增 --log-file 也置 use_logger=True，导致仅想写文件的用户也会触发 note: IR... 输出。建议在 CompilerConfig 中新增独立 verify_ir: bool 字段解耦，而非推迟到后续课题——当前设计将日志开关与校验开关永久绑定，--log-file 的语义变得不纯。

🟡 FileHandler(mode="w") 截断 + 多次 init 的静默数据丢失 — §4.7 承认"既定语义"，但若测试 harness 或 REPL 场景在同一进程内调用两次 compile()（或 init_logger），第二次会静默截断第一次的日志。建议至少提供 mode 可配置参数，或默认 "a"（追加），让调用方显式选择截断。

🟡 self._log 与外部 shutdown() 的生命周期脱钩 — §4.3.2 中 self._log 初始化后永不清除。若用户代码在 compile() 之外调用 shutdown()（F5 场景），self._log 仍持有引用但底层 handler 已清空，后续调用 self._log.debug(...) 写入静默丢弃。文档提到 F5 修复了 init_logger 重初始化的问题，但未说明 self._log 是否同步刷新——若不清除，_logger() 会返回旧引用而非重新 get_logger。建议 _logger() 每次调用时检测 is_initialized()，失效则重建。

🟡 模块级可变状态无线程保护 — _root_logger、_initialized、_console_handler、_file_handler、_config 均为模块级全局变量，init_logger/shutdown/set_level 无锁保护。CLI 单线程无碍，但文档作为库 API（get_logger 的"自动初始化"）可能被多线程测试或插件场景触发竞态。建议至少在文档中明确标注"非线程安全，单线程使用"。

💭 命名不一致：log_color vs use_color — CompilerConfig.log_color 映射到 init_logger(use_color=...)，两个名字语义相同但拼写不同。建议在文档中加注映射关系，或统一为 use_color。

💭 LogFileError 未在公共 API 表中列出 — §2.3.1 的异常描述提到了 LogFileError，但 API 表只列了函数签名，未将 LogFileError(OSError) 作为独立可导入符号登记。建议补充到 §2.3.1 表格中，方便调用方做 except LogFileError 精确捕获。

💭 --json-log 当前行为未说明 — 文档说"保留命名，本期不引入"，但未说明传入 --json-log 时 argparse 的行为（当前会 SystemExit(2) 报 unrecognized arguments）。建议在 §2.3.2 加一句"当前传入将报参数错误"以免用户困惑。

---

📁 scratchv/compiler.py

🔴 Bug: PassManager.run logs exception twice — Line ~160: self._log.error("pass '%s' failed: %s", p.name, exc, exc_info=True) includes both %s of exc and the full traceback via exc_info=True. The exception's string gets duplicated in the log line plus another full traceback.
Suggestion: Drop %s/exc and keep only exc_info=True, or vice-versa.

🔴 Bug: Logger re-init may double-register handlers — Line ~325: if self.config.use_logger and (self._log is None or not is_initialized()) triggers init_logger on every fresh driver. If init_logger is not idempotent (i.e., appends handlers without clearing), a second driver in the same process will stack console/file handlers and every log line is emitted twice.
Suggestion: Verify init_logger clears handlers before adding; otherwise guard with a "config changed" check instead of always re-initing.

🟡 Bug: Inconsistent stage identity in PassManager.run — Line ~155 uses self._log.name (e.g. "scratchv.compiler.passes") as the stage name for log_progress, but line ~195 uses self._name ("optimizer") in the summary. Progress lines and the summary disagree about which pipeline they belong to.
Suggestion: Use self._name everywhere (it's the human-readable label the caller supplied).

🟡 Bug: Warnings collected before a hard failure are silently dropped — compile() accumulates warnings from parse/verify/opt, but only flushes them to the log at the very end (line ~480). If codegen raises, earlier warnings (e.g. IR verifier messages) never reach the log file, even though they're useful context for diagnosing the failure.
Suggestion: On each failure return path, log the collected warnings before returning, or wrap the compile body in a try/finally that flushes pending warnings.

🟡 Bug: _one_line returns the original multi-line text on all-blank input — Line ~40: if every line is whitespace, the loop body never returns and we fall through to return text, leaking a multi-line blank blob into a log line (defeating the function's stated purpose in R9).
Suggestion: return "" (or a sentinel like "<blank>") in the fallback branch.

🟡 Bug: _phase context entered before imports in cycle-estimation block — Line ~445 wraps the from scratchv.backend.cycle_estimator import ... inside log_phase("compiler.cycle", ...). The phase timer/duration will include import time, and if the import raises, the phase is reported as a "cycle estimation" failure rather than an import/dependency error. Minor, but it skews timing stats and misattributes the error.
Suggestion: Move imports outside the _phase context, or split into a narrower phase.

💭 Nit: self._log.warning("%s", w) — Line ~478: passing a plain string through %s is the same as self._log.warning(w) for the common case, but it disables lazy formatting for interpolated messages. Prefer self._log.warning(w) and let the caller pre-format if needed.

💭 Nit: Blank line between import time and from contextlib — Minor PEP8 nit (one blank line between import and from blocks of the same module group).

💭 Nit: _pass_logger() called on every _run_optimizations — Line ~543: PassManager("optimizer", log=self._pass_logger()) re-fetches the child logger each time. Cheap (logger factory is cached) but the PassManager holds the reference, so this is fine functionally — just noting the pattern.

---

📁 scratchv/main.py

Code Review

---

🔴 config may be unbound in finally — Line ~265:
If an exception occurs before config is assigned (e.g., inside args_to_config()), the finally block will raise NameError, masking the original error.

# Guard or initialize before try:
config = None
try:
    config = args_to_config(args)
    ...
finally:
    if config and config.use_logger:
        shutdown()

🔴 shutdown() in finally can mask the original exception — Line ~267:
If shutdown() itself raises, it propagates through finally and hides whatever caused the exit. Wrap defensively:

finally:
    if config and config.use_logger:
        try:
            shutdown()
        except Exception:
            pass

🟡 Help text is self-contradictory — Line ~77:
"Write plain-text DEBUG log to FILE (implies logging at INFO+)" — "DEBUG" and "INFO+" contradict each other. If the file always captures DEBUG regardless of --log-level, say so clearly:

help=("Write plain-text DEBUG log to FILE; "
      "must differ from input/output paths")

🟡 No visible validation for --log-file path collision — The help promises "must differ from input/output paths", but this diff doesn't show where that check lives. If it's in the logger module raising LogFileError, confirm that args_to_config or the logger init validates this before any file is opened (not after). A missing check means a user's .onnx input gets silently overwritten.

🟡 --log-file alone enables logging but log_color uses stderr — Line ~154:
log_color=sys.stderr.isatty() is fine for --log-level, but when only --log-file is given (no --log-level), the user gets INFO-level stderr output and a file. The help says "implies logging at INFO+", which could surprise users who just want a file. Consider making --log-file silent on stderr unless --log-level is also passed.

💭 sys.stderr.isatty() is a point-in-time snapshot — Minor, but if a subprocess changes stderr between args_to_config() and actual logging, color detection is stale. Acceptable for a CLI tool.

---

Summary: Two 🔴 fixes required (unbound config, shutdown() exception masking). The 🟡 items are about correctness of user-facing behavior and missing validation.

---

📁 scratchv/utils/logger.py

🟡 **Thread safety** — `init_logger`, `set_level`, `shutdown` all mutate
   shared globals (`_root_logger`, `_console_handler`, `_file_handler`,
   `_initialized`) without locking. Concurrent calls can interleave
   (e.g., `shutdown` clears handlers mid-`init_logger`). Consider a
   module-level `threading.Lock`.

🟡 **Broad exception swallow in `shutdown()`** — Lines ~250–253:
   `except Exception: pass` silently discards every failure type,
   including `PermissionError` or bugs in handler implementations.
   Consider logging the swallowed exception at DEBUG, or at minimum
   restricting to `OSError`/`ValueError` (the known-fail cases from
   closed streams).

🟡 **Unused `_config`** — Written in `init_logger` and `set_level`
   but never read anywhere visible. If no external code consumes it,
   it's dead state. If it is consumed elsewhere, add a brief comment
   noting which module reads it.

🟡 **New public API `is_initialized()` has no test coverage shown** —
   Add a unit test verifying it returns `False` after `shutdown()`
   and `True` after `init_logger()`.

💭 **Traceback indentation** — `super().format(record)` appends the
   traceback to the message body, so in the output only the first
   line gets the `{asctime} {level} [{name}]` prefix; subsequent
   traceback lines are unindented. This is standard but worth noting
   if anyone is grepping log files by prefix.

---

📁 tests/test_logger.py

🟡 Misleading test name — test_log_file_only_contains_debug: you assert "info-line" in text, so the file holds more than debug. Rename to test_file_captures_debug_below_console_level.

🟡 Coupling to private state — Tests read _initialized, _root_logger, _console_handler, _file_handler, _config. These are implementation details; a refactor breaks them without any behavioral change. Prefer observable behavior (handler levels via the logging API, capsys/file content).

🟡 Brittle handler count — assert len(new_handlers) == 2 hardcodes console+file. If a third handler is ever added (journald, audit), this fails spuriously. Assert the specific handlers you care about by type/identity instead.

🟡 Double shutdown — teardown_method calls shutdown() and most tests also call it inline (test_shutdown_resets_state calls it twice). Keep inline shutdown only where it's needed to flush before reading capsys/the file; let teardown handle cleanup.

🟡 Unverified line — test_shutdown_resets_state: log.info("revived logger") is never observed. Capture it (caplog/capsys) and assert, or remove it so the test doesn't imply a check it doesn't perform.

💭 test_log_file_error_resets_state — nice that you assert err == ""; consider also asserting _config == {} for parity with test_shutdown_resets_state.

💭 test_reinit_closes_old_handlers — the spy pattern works, but move assert old_file.stream is None to right after init_logger(...) (before stopping patches) so it's clear the assertion proves close() ran.

Otherwise solid: the wraps=h.close spy, the LogFileError reset check, and verifying actual routing in test_set_level_updates_console_only rather than just handler levels are all good regression coverage.

---

📁 tests/test_logger_wiring.py

Code Review: tests/test_logger_wiring.py

🟡 Fragile test input — test_parse_failure_logged (L148): uses "model.onnx" (nonexistent relative path) as input. If CompilerDriver or main gains a file-existence check before _parse is called, this test silently breaks or tests the wrong path. Use str(DSL) (which is known to exist) or a tmp file:

rc = main([str(DSL), "-o", ...])

🟡 DSL path never existence-checked — Module-level DSL (L23–24) is used unconditionally in 7+ tests. If the file is missing, every test fails with a cryptic FileNotFoundError. Consider a guard:

if not DSL.exists():
    pytest.skip(f"DSL fixture not found: {DSL}")

🟡 Private state assertions — Tests at L193–197 and L207–208 reach into logger_mod._file_handler, logger_mod._config, logger_mod._initialized, logger_mod._root_logger. These are implementation details that will break silently on refactor. Prefer asserting observable behavior (log file contents, stderr format) instead. If you must check internal state, wrap in a dedicated integration test or mark with # fragile: internal state.

💭 Inconsistent ONNX guard — test_log_file_bad_path_error conditionally uses if ONNX.exists() (L204), but DSL has no equivalent guard. Pick one policy: guard all fixture paths or none.

---

---

⚠️ 未审查的文件

• tests/test_topic07_logger_case_report.py