Topic9：DSL错误提示美化器设计文档和开发文档

[mahiru114514]

Summary

• add a design document for integrating structured diagnostics into the DSL parsers
• add a development guide covering implementation stages, testing, error recovery, and acceptance criteria
• distinguish current repository behavior from proposed functionality

Scope

Documentation only. No compiler source code or tests are changed.

Validation

• Markdown rendering passed
• local links and code fences checked
• independent documentation review: 0 critical and 0 important issues
• project L2 tests were not run because the local harness and Python test dependencies were unavailable

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

🔴 Artifact upload fails on early-exit paths — Lines 59, 179: if: always() means upload-artifact runs even when checkout/test steps fail. If benchmark_reports/ was never created, actions/upload-artifact errors with "Path not found", masking the real failure.
Suggestion: Guard with if: always() && hashFiles('benchmark_reports/**') != ''.

🔴 git fetch by SHA is server-dependent — Line 92: git fetch origin "$BASE_SHA" relies on uploadpack.allowReachableSHA1InWant. Works on GitHub, but will fail on many self-hosted git servers (GitLab CE, etc.). Also, if BASE_SHA is the PR base head, it may not be reachable from any ref in the shallow clone.
Suggestion: Fetch the base ref instead (git fetch origin "$BASE_REF" --depth=1), or at minimum add --no-tags and an explicit error message:

git fetch origin "$BASE_SHA" --depth=1 2>&1 || {
  echo "::error::Cannot fetch baseline $BASE_SHA; is it reachable from a ref?";
  exit 1;
}

🟡 GITHUB_REF mismatch on PR-triggered runs — Lines 34, 84: For PR events, github.ref is refs/pull/N/merge (a synthetic ref). If the workflow uses a mirror/checkout strategy, this ref may not exist on the configured origin. The old code fetched main (always exists); the new code may silently miss commits if the ref doesn't resolve.
Suggestion: Use git fetch origin "$GITHUB_SHA" directly (like the BASE_SHA pattern) to avoid ref-resolution ambiguity, or verify the ref exists with a git ls-remote check first.

🟡 Trap registered too late — Line 97: trap ... EXIT is set after git worktree add. If worktree add itself fails mid-execution, the partially-created worktree is never cleaned up.
Suggestion: Register the trap immediately after mktemp, before the worktree is created:

BASELINE=$(mktemp -d "$RUNNER_TEMP/dsl-baseline.XXXXXX")
trap 'git worktree remove --force "$BASELINE" 2>/dev/null || true' EXIT
git worktree add --detach "$BASELINE" "$BASE_SHA"

🟡 Missing set -euo pipefail — All shell steps: Without pipefail, a failed command in a pipe (e.g., git fetch ... | tee) still exits 0.
Suggestion: Add set -euo pipefail as the first line of every multi-line run: block.

💭 Redundant pytest install — Line 45: pytest>=7,<10 is pinned again after pip install -e ".[all]". If the project's pyproject.toml already declares pytest as a dependency (which the .[all] extra likely includes), this line is dead weight and creates a version-conflict risk if the bounds diverge.
Suggestion: Move the pytest pin into the project's dependency spec instead.

---

📁 benchmarks/bench_dsl_diagnostics.py

🔴 Bug: Division by zero risk — compare_parsing (line ~181):

ratio = current["median_s"] / baseline["median_s"]

If a pathological checkout parses in 0 µs (e.g. empty corpus edge case), this raises ZeroDivisionError. Guard with a minimum floor or early-exit when baseline["median_s"] <= 0.

🟡 Unformatted floats in markdown table — markdown_report (line ~240):

lines.append(f"| {label} | {parsing['baseline'][key]} | {parsing['current'][key]} |")

median_s renders as a raw float (e.g. 0.000123456789) while ratio uses :.3f. Apply :.6f for the seconds column so the table stays readable.

🟡 Incomplete thread-limiting env — run_worker (line ~168):
Only OMP_NUM_THREADS and OPENBLAS_NUM_THREADS are pinned. MKL_NUM_THREADS, VECLIB_MAXIMUM_THREADS, NUMEXPR_NUM_THREADS are not, so NumPy/ONNX threads can still vary across machines, inflating the performance ratio and causing false "target not met" results.

🟡 importlib.metadata.version crashes on missing dep — build_report (line ~220):
If onnx or numpy is absent, PackageNotFoundError is raised. It's caught by the outer try/except, but the report only says "PackageNotFoundError: No package metadata was found for onnx" — no hint that this is an environment issue. Wrap each call in try/except and record "unavailable" instead.

💭 Path.is_relative_to requires Python 3.9+ — line ~72:
The module uses from __future__ import annotations and str | None hints (fine on 3.7+), but is_relative_to is a runtime call that requires 3.9+. Document the minimum version in the docstring or argparse.ArgumentParser description.

💭 revision renders as literal "None" in markdown — line ~240:
When a checkout is an unpacked archive (no git), revision returns None. The table shows | Revision | None | None |. Replace with "(unknown)" for clarity.

---

📁 docs/superpowers/plans/2026-08-12-dsl-diagnostics-implementation.md

Code Review: DSL Diagnostics Implementation Plan

🔴 Blockers

Contradictory render_error signature — Task 1 Step 3 defines render_error(err, *, stream: TextIO) -> str, but a function that writes to a stream should return None (or vice versa). This will cause confusion during implementation.

Missing error code in verification — Task 5 Step 4 lists "E100, E101, E110, E111, E200 and E201" but omits E112 (duplicate else), which is tested in Task 3 Step 1. Either add E112 to the requirements or remove the test.

🟡 Suggestions

Ambiguous error limit semantics — Task 1 Step 4: "stop at exactly max_errors" is unclear. Does the collector stop after collecting max_errors, or before reaching max_errors? This is an off-by-one risk. Recommend clarifying: "collect at most max_errors errors; set limit_reached=True if more would have been emitted."

CompileResult field addition risk — Task 4 Step 3 appends defaulted fields to CompileResult. If any code constructs CompileResult positionally (not by keyword), this will break silently. Consider making the new fields keyword-only or verifying no positional usage exists first.

No DoS protection for malicious input — The plan doesn't address pathological inputs: extremely long lines, deeply nested blocks (potential RecursionError), or files with thousands of errors. Consider adding max-depth or max-line-length guards in SourceBuffer/validator.

Complex block recovery may hide errors — Task 3 Step 3: "recover by popping through an outer matching block or treating the closer as recovery for the top frame" — this dual recovery strategy can produce confusing messages where a stray endif silently "fixes" an unrelated unclosed block. Recommend preferring conservative recovery (emit E110, skip the closer, continue validation) over speculative recovery.

Undefined "normal mode" — Task 4 Step 4: "exit 2 without traceback in normal mode" — what triggers "non-normal" mode? A --debug flag? An environment variable? This needs a concrete definition or the implementation will be inconsistent.

💭 Nits

Plan includes execution instructions — Task 5 Step 6 (git add, commit, push) and Step 5 (memory.md merge) are operational steps, not implementation requirements. Consider moving them to a separate "release checklist" to keep the plan focused on what to build.

Good: typo test case — retrun x (Task 2 Step 1) is a smart test for catch-all/typo detection; it ensures the validator doesn't silently accept misspelled keywords.

Good: backward compatibility emphasis — "Keep format_error() available", "retains their previous Program.dump() structure", and "Do not modify ONNX parsing" show careful attention to not breaking existing users.

Minor: error code numbering — The scheme (E100/E101 syntax, E110/E111/E112 blocks, E200/E201 operators) is logical but the plan never defines the full namespace. Consider adding a one-line legend early (e.g., "E1xx = syntax, E2xx = semantic, E3xx = internal").

---

📁 docs/topics/09-DSL诊断-CI与Benchmark.md

Review: docs/topics/09-DSL诊断-CI与Benchmark.md

🟡 建议：明确 CI 中的测试去重机制 — 第 30 行："原有 pytest tests/ 会自动发现这些文件，不重复执行专项测试。"

含义模糊：CI 到底是只跑 pytest tests/（自动发现），还是同时有独立专项步骤？如果两者都跑，就是重复执行。建议明确二选一。

🟡 建议：新分支首推硬失败需要 workaround — 第 47 行：新分支首次 push 的全零 before 会明确失败。

这会导致 git push -u origin new-branch 后 benchmark step 必然 red，开发体验差。建议在文档中注明规避方式（例如先 rebase 到已有分支，或直接开 PR 后用 PR base）。

🟡 建议：硬编码 SHA 997d2aa 需更醒目提醒 — 第 35 行：虽然下一段已说明 rebase 后应换 SHA，但建议直接在命令块前用 > ⚠️ 引用块标注"此 SHA 仅适用于当前 PR，rebase 后必须更新"，避免 copy-paste 直接执行过时基线。

💭 nit：措辞一致性 — 第 54 行 "硬门禁" 与第 58 行 "默认明确显示…但不因…阻止合并" 形成对比，逻辑正确但"硬门禁"一词容易让读者误解为 CI 会 block。建议改为 "必失败项" 或 "阻断性检查"，与后文软性目标区分更清晰。

---

📁 docs/topics/09-DSL错误提示美化器-开发文档.md

🔴 Bug: 12周计划 W2/W3 顺序颠倒 — 第19节：W2 "锁定输出契约" 需要先有 SourceBuffer 才能可靠测试行号和 caret 对齐。建议 W3→W2 或 W2 仅锁定错误码/文本结构，W3 再锁定精确渲染。

🔴 Bug: for 块在 BlockFrame 中出现但不在项目范围内 — 第11.1节 Literal["if","while","for"]，但第2.1节当前代码只支持 if/while，全文无 for 块验证描述。实现者会困惑是否要支持 for，还是 Literal 应只保留 "if","while"。

🔴 Bug: 错误格式示例与错误码体系可能矛盾 — 第7.1节示例 retrun → error[E100]: cannot parse statement，但 retrun 是关键字拼写错误，应该是 E101/E103 等更具体的码，而非通用 E100。设计文档的错误码分配需要确认与开发文档一致。

🟡 设计缺口: OP_SIGNATURES 与 OP_HANDLERS 一致性检查不足 — 第10.4节 set(OP_SIGNATURES) == set(OP_HANDLERS) 只比对键名，不比对签名值。新增 handler 若参数数写错，测试不会失败。建议增加断言每个 signature 与对应 handler 的 *args 解包数量匹配。

🟡 设计缺口: CompileResult 兼容策略不够明确 — 第13.2节同时保留 diagnostics 和 errors，说"CLI 只渲染 diagnostics"。但现有调用方（第2.1节 compiler.py）直接读 errors 拼接输出。需要明确：errors 在新实现中是否仍为多行文本列表？格式是否变化？

🟡 设计缺口: Parser 状态重置无具体机制 — 第12.3节要求"确认 builder、变量表、循环栈和标签计数器处于干净状态"，但未指定实现方式（parse() 开头调 reset()？每次 __init__ 重建？）。缺少具体方案，实现者可能各自为政导致二次解析状态泄漏。

🟡 测试缺口: 缺少 strip() 残留风险的回归测试 — 第8.3节指出"保存原始行之前调用 strip()"是常见错误，但第16.4节测试用例清单中没有"带前导/尾随空白的合法语句"用例。建议在"行列定位"类别中增加此用例。

🟡 测试缺口: 缺少嵌套块恢复后后续语句定位的用例 — 第11.2节提到"嵌套块错误不能破坏后续独立语句的位置"，但第16.4节"块结构"用例没有明确覆盖"嵌套块错误后紧跟独立算子行"的场景。

💭 文档一致性 — 第9节列了两个方案处理循环导入但没标记推荐方案，实现者需自行决定。建议明确标注推荐方案1（移至 dsl_errors.py），减少分支决策成本。

💭 措辞精度 — 第10.2节第5步"检查算子是否存在"与第10.4节 OP_SIGNATURES 重复；第10.2节可改为"检查算子是否在共享签名表中"以体现 DRY 意图。

💭 第15.1节"精确拼写表" 措辞模糊 — 建议明确是硬编码映射（"ad" → "add"）还是 Levenshtein 距离 ≤2 的通用算法，避免实现方向分歧。

---

📁 docs/topics/09-DSL错误提示美化器-设计文档.md

🔴 Bug: DSLSyntaxError 数据类与 Exception 基类冲突 — §7.2

@dataclass
class DSLSyntaxError(DSLParseError):

@dataclass 自动生成的 __init__ 不会正确调用 Exception.__init__，导致 args 为空、__str__() 行为未定义。建议不要直接加 @dataclass 装饰器，而是手写 __init__ 并显式调用 super().__init__(message)，或用 @dataclass(init=False) 手写初始化。

🔴 Bug: 恢复规则歧义 — §9.2 第 4 条

"若该结束符能匹配更外层块，则弹出到该外层块（含它）"

当栈中存在多个同类型外层块时，应匹配最近的还是最远的？例如 if → while → if → endif，endif 应匹配最近的 if（栈顶以下第二个）还是最外的 if？需明确为最近的匹配帧。

🟡 Suggestion: 命名不一致 — §7.4 / §9.2 / §11.3

文档中同时出现 collector.limit_reached、diagnostic_limit_reached、diagnostic_limit。建议统一：ErrorCollector 用 limit_reached: bool 和 limit: int，CompileResult 用 diagnostic_limit_reached 和 diagnostic_limit，并在文档中显式标注映射关系。

🟡 Suggestion: 缺少宽字符显示对齐说明 — §7.1 / §11.1

§7.1 说明列号按 Unicode code point 计数，但未提及 CJK 等双宽字符在终端中的实际占用。若用户使用中文变量名或注释，caret 位置会错位。建议：要么声明"不支持双宽字符对齐"（可接受），要么提供 east_asian_width 换算策略。

🟡 Suggestion: 性能目标缺乏依据 — §15

1.5 倍目标合理但缺少基线数据。建议在开发文档中附上当前解析流程的实测中位数，否则验收时无法判断是否达标。

🟡 Suggestion: 验收标准覆盖不足 — §16 第 3 条

仅要求覆盖 E100/E101/E110/E111/E200/E201 六个错误码，但 §8 定义了 12 个。若 E112（重复 else）或 E202/E203 未被覆盖，测试可能遗漏边界。建议明确这是最小覆盖还是完整覆盖。

💭 Nit: 缺少扩展指南 — §16 第 10 条要求"文档说明如何增加新的错误码、验证规则和修复建议"，但文档正文未给出操作步骤。建议在 §8 或 §10 末尾加一段"如何新增错误码"的模板。

💭 Nit: 多行构造声明 — 未明确说明 DSL 不支持多行语句或字符串续行。若未来语法扩展引入这些，当前"物理行为同步边界"的策略（§9.1）会失效，建议在 §4.2 非目标中补充。

---

📁 memory/memory.md

Code Review: memory/memory.md

🟡 Missing purpose/context header — Lines 1-2: File jumps straight into entries. Add a brief header explaining what this file is, who maintains it, and when to add vs. remove entries (e.g., "Engineering decisions and constraints for this repo. Append-only log; entries older than N months may be archived.").

🟡 Dense unstructured prose — Lines 3-5: Each entry is a single run-on paragraph mixing decision, rationale, scope, and caveats. Consider structured fields per entry:

- **Decision**: benchmark 复用 ci.yml 的 test/benchmark jobs，不新建 pipeline
- **Rationale**: pytest 已自动发现 DSL 测试；报告复用 artifacts
- **Scope**: 本仓库课题 PR 的 CI 接入
- **Caveat**: ...

This makes it scannable and prevents information from being lost in a wall of text.

🟡 Insufficient cross-reference — Line 5: References "课题 9" without a link to the issue/PR that defines it. Any team member encountering this later has no path to verify the decision. Consider: (see #1234) or 课题 9 (issue #XXX).

🟡 Future dates (2026-09-13) — If this is intentional (project timeline), fine. If it's a typo, fix it. At minimum, the date format should be ISO 8601 with timezone (2026-09-13T00:00:00Z) to avoid ambiguity for future automation.

🟡 No expiry or verification mechanism — Entries state requirements as present-tense facts ("维护者要求", "benchmark 应用"). Over time these may go stale. Consider adding a status: field (active | superseded | archived) or a review date per entry so stale decisions get pruned.

💭 Inconsistent scope wording — Line 3 says "适用于本仓库课题 PR 的 CI 接入" (applicable scope) while Line 5 has a longer applicability clause. Standardize a consistent Applies to: or Scope: label at the end of each entry.

💭 Consider front-matter metadata — If tooling will ever parse this file (e.g., for dashboards or staleness checks), YAML front-matter with version, owner, last-audited would help. If purely human-facing, the current format is acceptable but the structural issues above still apply.

---

Summary: The content is valuable and the decisions are well-reasoned. The main risk is readability and maintainability — these entries will become harder to navigate as more are added. Structuring each entry with explicit fields (decision / rationale / scope / caveat) would significantly improve long-term utility.

---

📁 scratchv/compiler.py

🟡 Double file read — The use_dsl block reads input_path into source for validation, then _parse opens the same file again because it doesn't receive the already-read content. Pass source explicitly to _parse or refactor to avoid the redundant I/O.

🟡 Dropped DSLParser fallback is a silent breaking change — Previously _parse fell back to DSLParser if ExtendedDSLParser failed. Now any DSL file that only the old parser handles will hard-fail. If ExtendedDSLParser is a true superset, add a comment saying so; otherwise this is a regression.

🟡 Inconsistent exception handling — Inside except Exception, only DSLSyntaxError is caught when use_dsl is true. Other DSL-related exceptions (e.g. DSLValidationError, MemoryError from large input) are re-raised instead of being wrapped in a CompileResult. Consider catching a broader DSL exception base class for consistent behavior.

🟡 source or "" conflates None and "" — An empty file ("") and a missing source (None when input_path is also None) both produce "" passed to validate(). An empty DSL file should probably produce a clear error, not silently validate as valid.

💭 diagnostic_limit default vs actual — The dataclass field defaults to 20, but when validation fails the actual limit comes from collector.max_errors, which may differ. Consumers reading result.diagnostic_limit will see 20 on success (meaningless) and the real value on failure. Consider leaving it as 0 (unset) when not applicable, or making it a computed property.

💭 Import inside except block — from scratchv.frontend.dsl_errors import DSLSyntaxError inside the exception handler means an import error there produces a confusing traceback. A top-level import (or module-level __all__ check) would be clearer.

---

📁 scratchv/frontend/__init__.py

🟡 OP_SIGNATURES exported as public API — 这是一个内部数据常量，暴露在顶层 __all__ 中意味着下游使用者可能依赖它的内部结构。如果它只是给 DSLValidator 用的，考虑不导出或加注释说明其稳定性契约。

其余无问题。

---

📁 scratchv/frontend/dsl_errors.py

🟡 Marker misaligned for line numbers ≥ 10 — format_error: marker_padding = 6 + display_start hardcodes the prefix length. The prefix f" {err.line} | " is 5 + len(str(err.line)) chars, so for err.line=10 the caret shifts left by 1, for err.line=100 by 2, etc.

Suggestion: prefix_len = len(f" {err.line} | "), then marker_padding = prefix_len + display_start.

🟡 .errors re-sorts on every access — ErrorCollector.errors calls sorted() each time. If a caller accesses .errors in a loop or multiple times, this is redundant O(n log n) work.

Suggestion: Sort once in report() or cache the sorted result, invalidated on add()/clear().

💭 render_error signature is misleading — It accepts stream: TextIO but never writes to it, only returns a string. The stream is used solely for isatty() color detection.

Consider: either write to stream and return None, or rename to detect_color(stream) and keep format_error as the renderer.

💭 NO_COLOR check is strict-presence — "NO_COLOR" not in os.environ disables color even when NO_COLOR="". Some tools treat empty as "allow color". Worth a comment noting the chosen convention.

---

---

⚠️ 未审查的文件

• scratchv/frontend/dsl_extended.py
• scratchv/frontend/dsl_parser.py
• scratchv/frontend/dsl_validator.py
• scratchv/main.py
• tests/test_dsl_diagnostics_benchmark.py
• tests/test_dsl_diagnostics_cli.py
• tests/test_dsl_errors.py
• tests/test_dsl_validator.py