diff --git a/.codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py b/.codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py index a2eafdbb..017f2424 100644 --- a/.codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py +++ b/.codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py @@ -234,10 +234,10 @@ def parse_comment_cards(comment_block: str) -> list[dict[str, str]]: comments: list[dict[str, str]] = [] pattern = re.compile( r"" - # CodeRabbit can fold nitpick cards for repo tooling files such as .js/.ts. - # Keep the matcher broad enough for common source/config files while still - # requiring a path-like summary header instead of arbitrary review text. - r"((?:[^<\n]+/)*[^<\n]+\.(?:cs|md|csproj|yaml|yml|json|txt|props|targets|js|jsx|mjs|cjs|ts|tsx)|AGENTS\.md|CLAUDE\.md|README\.md|\.gitignore)" + # CodeRabbit can fold cards for source, docs, scripts, and repo config files. + # Keep the matcher path-like, but do not hardcode a tiny extension allow-list + # or we will silently drop valid findings such as .py skill files. + r"((?:[^<\n]+/)*[^<\n/]+(?:\.[A-Za-z0-9._-]+)+|AGENTS\.md|CLAUDE\.md|README\.md|\.gitignore)" r" \((\d+)\)
\s*(.*?)\s*(?:(?:
)|(?:))", re.S, ) @@ -266,6 +266,27 @@ def parse_comment_cards(comment_block: str) -> list[dict[str, str]]: return comments +def normalize_review_body_for_parsing(review_body: str) -> str: + # CodeRabbit sometimes wraps structured HTML sections in markdown blockquotes, + # such as the CAUTION block used for outside-diff comments. Remove the quote + # prefixes for parsing while leaving the original raw body unchanged for output. + return re.sub(r"(?m)^>\s?", "", review_body) + + +def find_section_block_end(review_body: str, block_start: int) -> int: + depth = 1 + for tag_match in re.finditer(r"
|
", review_body[block_start:]): + tag = tag_match.group(0) + if tag == "
": + depth += 1 + else: + depth -= 1 + if depth == 0: + return block_start + tag_match.start() + + return len(review_body) + + def parse_review_comment_group(review_body: str, section_name: str) -> dict[str, Any]: section_match = re.search( rf"[^<]*{re.escape(section_name)} \((?P\d+)\)
\s*", @@ -275,16 +296,9 @@ def parse_review_comment_group(review_body: str, section_name: str) -> dict[str, if section_match is None: return {"count": 0, "comments": [], "raw": ""} - remaining_body = review_body[section_match.end() :] - end_markers = [ - "\n
\n\n
\n🤖 Prompt for all review comments with AI agents", - "\n
\n\n
\n🪄 Autofix (Beta)", - "\n
\n\n
\nℹ️ Review info", - "\n
\n\n---", - ] - end_positions = [remaining_body.find(marker) for marker in end_markers if remaining_body.find(marker) >= 0] - end_index = min(end_positions) if end_positions else len(remaining_body) - comment_block = remaining_body[:end_index].strip() + block_end = find_section_block_end(review_body, section_match.end()) + comment_block = review_body[section_match.end() : block_end].strip() + comment_block = re.sub(r"\s*\s*$", "", comment_block, flags=re.S) return { "count": int(section_match.group("count")), "comments": parse_comment_cards(comment_block), @@ -293,15 +307,19 @@ def parse_review_comment_group(review_body: str, section_name: str) -> dict[str, def parse_latest_review_body(review_body: str) -> dict[str, Any]: - actionable_count_match = re.search(r"\*\*Actionable comments posted:\s*(\d+)\*\*", review_body) + normalized_review_body = normalize_review_body_for_parsing(review_body) + actionable_count_match = re.search(r"\*\*Actionable comments posted:\s*(\d+)\*\*", normalized_review_body) prompt_match = re.search( r"🤖 Prompt for all review comments with AI agents\s*```(.*?)```", - review_body, + normalized_review_body, re.S, ) - nitpick_group = parse_review_comment_group(review_body, "Nitpick comments") + outside_diff_group = parse_review_comment_group(normalized_review_body, "Outside diff range comments") + nitpick_group = parse_review_comment_group(normalized_review_body, "Nitpick comments") return { "actionable_count": int(actionable_count_match.group(1)) if actionable_count_match else 0, + "outside_diff_count": outside_diff_group["count"], + "outside_diff_comments": outside_diff_group["comments"], "nitpick_count": nitpick_group["count"], "nitpick_comments": nitpick_group["comments"], "all_comments_prompt": prompt_match.group(1).strip() if prompt_match else "", @@ -607,8 +625,17 @@ def build_result(pr_number: int, branch: str) -> dict[str, Any]: latest_review_body = str(latest_review.get("body") or "") if latest_review.get("user") == CODERABBIT_LOGIN and latest_review_body: coderabbit_review = parse_latest_review_body(latest_review_body) + outside_diff_count = int(coderabbit_review.get("outside_diff_count") or 0) + parsed_outside_diff_count = len(coderabbit_review.get("outside_diff_comments", [])) nitpick_count = int(coderabbit_review.get("nitpick_count") or 0) parsed_nitpick_count = len(coderabbit_review.get("nitpick_comments", [])) + if "Outside diff range comments" in latest_review_body and not parsed_outside_diff_count: + warnings.append("CodeRabbit outside-diff comments block could not be parsed from the latest review body.") + elif outside_diff_count and parsed_outside_diff_count != outside_diff_count: + warnings.append( + "CodeRabbit outside-diff comments were only partially parsed from the latest review body: " + f"declared={outside_diff_count}, parsed={parsed_outside_diff_count}." + ) if "Nitpick comments" in latest_review_body and not parsed_nitpick_count: warnings.append("CodeRabbit nitpick comments block could not be parsed from the latest review body.") elif nitpick_count and parsed_nitpick_count != nitpick_count: @@ -680,6 +707,17 @@ def format_text(result: dict[str, Any]) -> str: if actionable_count and not comments: lines.append(" Details: see latest-commit review threads below.") + outside_diff_comments = review_feedback.get("outside_diff_comments", []) + outside_diff_count = review_feedback.get("outside_diff_count") or len(outside_diff_comments) + lines.append("") + lines.append(f"CodeRabbit outside-diff comments: {outside_diff_count} declared, {len(outside_diff_comments)} parsed") + for comment in outside_diff_comments: + lines.append(f"- {comment['path']} {comment['range']}".rstrip()) + if comment["title"]: + lines.append(f" Title: {comment['title']}") + if comment["description"]: + lines.append(f" Description: {comment['description']}") + nitpick_comments = review_feedback.get("nitpick_comments", []) nitpick_count = review_feedback.get("nitpick_count") or len(nitpick_comments) lines.append("") diff --git a/ai-plan/public/ai-first-config-system/todos/ai-first-config-system-tracking.md b/ai-plan/public/ai-first-config-system/todos/ai-first-config-system-tracking.md index 0c39d776..aca43ecc 100644 --- a/ai-plan/public/ai-first-config-system/todos/ai-first-config-system-tracking.md +++ b/ai-plan/public/ai-first-config-system/todos/ai-first-config-system-tracking.md @@ -24,7 +24,7 @@ - PR review 信号漂移风险：CodeRabbit 可能把建议折叠在 latest review body，而不是 issue comments - 缓解措施：`gframework-pr-review` 现已同时解析 latest review body，并输出 declared / parsed 数量以便快速识别解析缺口 - PR follow-up 残留风险：PR `#262` 最新 review thread 仍有少量 open comments，且 nitpick body 解析仍存在 declared / parsed 缺口 - - 缓解措施：先以 latest unresolved thread 为准逐条本地核验；已确认并补齐运行时诊断路径与 `else without if` 回归测试，剩余解析缺口单独留在 skill 后续处理 + - 缓解措施：先以 latest unresolved thread 为准逐条本地核验；已确认并补齐运行时诊断路径与 `else without if` 回归测试，skill 现已补齐 `.py` nitpick 与 outside-diff comment 解析，剩余项只需等待本地修复推送后再复抓确认 - 非阻塞项回退风险：将 VS Code 功能标为非阻塞但导致主线回退的风险 - 缓解措施：C# 主线补齐新关键字时仍需在 `configValidation.js` 与 `extension.js` 中同步落地，只是不让复杂表单控件阻塞发布 @@ -52,11 +52,14 @@ - text 输出会显示 `CodeRabbit nitpick comments: X declared, Y parsed`，避免再次静默遗漏 - 已按 5 条 nitpick 更新 VS Code tool hints、shared validation helper，以及对称分支测试覆盖 - PR `#262` 最新 follow-up： - - 最新抓取结果显示仍有 2 条 actionable comments 与 1 条已解析 nitpick 需要本地核验 + - 最新抓取结果显示 latest review body 里有 2 条 nitpick 与 1 条 outside-diff actionable comment - `SchemaConfigGenerator` 的分支级诊断定位已在当前分支，无需重复修改 - `YamlConfigSchemaValidator` 已补齐 `conditionalSchemaPath` 诊断路径，避免 `reward[then]` / `reward[else]` 坏形状误报到父路径 - `YamlConfigLoaderIfThenElseTests` 已新增运行时 `else` 缺失 `if` 回归，避免 Runtime / Generator 覆盖漂移 - active trace 已将重复的 `### 验证` 标题改为专用 PR follow-up 标题，消除 `MD024` + - `gframework-pr-review` 现已在 latest review body 中同时解析 `Outside diff range comments` 与 `Nitpick comments` + - `parse_comment_cards` 已不再遗漏 `.codex/.../*.py` 这类 skill 文件评论卡片 + - `tools/gframework-config-tool/src/configValidation.js` 已按 outside-diff 建议收紧条件分支坏形状拒绝规则，并补齐 JS 回归测试 - 分支同步状态： - `feat/ai-first-config` 已 rebase 到 `origin/feat/ai-first-config` - 当前已解决“ahead / behind 同时存在”的分支差异，不再 behind 远端 @@ -82,14 +85,14 @@ - `2026-04-17` 之前的详细实现记录与定向验证命令已归档到历史 tracking / trace - active 跟踪文件只保留当前恢复点、当前状态和下一步，不再重复堆积已完成阶段的完整历史 - `2026-04-20` 当前恢复点验证： - - `python3 .codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py`：通过（`CodeRabbit actionable comments: 2`，`CodeRabbit nitpick comments: 2 declared, 1 parsed`） - - `bun run test`（`tools/gframework-config-tool`）：通过 + - `python3 .codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py --pr 262 --format json`：通过（`CodeRabbit outside-diff comments: 1 declared, 1 parsed`，`CodeRabbit nitpick comments: 2 declared, 2 parsed`） + - `bun run test`（`tools/gframework-config-tool`）：通过（122 tests；包含条件分支坏形状回归） - `dotnet test GFramework.SourceGenerators.Tests/GFramework.SourceGenerators.Tests.csproj -c Release --filter "FullyQualifiedName~SchemaConfigGeneratorTests"`：通过 - `dotnet test GFramework.Game.Tests/GFramework.Game.Tests.csproj -c Release --filter "FullyQualifiedName~YamlConfigLoaderIfThenElseTests"`：通过（8 tests；新增 `else without if` 运行时回归） - `dotnet build GFramework.sln -c Release`：通过（存在仓库既有 analyzer warning，无新增错误） ## 下一步 -1. 提交并推送当前 PR `#262` follow-up 修复后，重新抓取一次 PR review，确认 open thread 是否已清空或只剩 parser gap +1. 提交并推送当前 PR `#262` follow-up 修复后，重新抓取一次 PR review，确认 outside-diff comment 与 open thread 是否都已收口 2. 若 PR review 已收口，再回到 `GFramework.Game/Config/YamlConfigSchemaValidator.cs`、`GFramework.Game.SourceGenerators/Config/SchemaConfigGenerator.cs`、`tools/gframework-config-tool/src/configValidation.js` 盘点下一批候选关键字 3. 优先判断 `oneOf` / `anyOf` 是否存在可接受的 object-focused 子集；若仍会引入生成类型形状漂移，就直接跳过 diff --git a/ai-plan/public/ai-first-config-system/traces/ai-first-config-system-trace.md b/ai-plan/public/ai-first-config-system/traces/ai-first-config-system-trace.md index 4693c3c0..e59b4c93 100644 --- a/ai-plan/public/ai-first-config-system/traces/ai-first-config-system-trace.md +++ b/ai-plan/public/ai-first-config-system/traces/ai-first-config-system-trace.md @@ -83,11 +83,18 @@ - 2026-04-20：`python3 .codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py` - 结果：通过 - 备注：输出 `CodeRabbit actionable comments: 2`、`CodeRabbit nitpick comments: 2 declared, 1 parsed`，并暴露剩余 review follow-up +- 2026-04-20：skill parser follow-up + - 结果：已补齐 + - 备注：`gframework-pr-review` 现可解析 latest review body 中的 `Outside diff range comments`，并且不再遗漏 `.codex/.../*.py` nitpick cards +- 2026-04-20：`python3 .codex/skills/gframework-pr-review/scripts/fetch_current_pr_review.py --pr 262 --format json` + - 结果：通过 + - 备注：输出 `CodeRabbit outside-diff comments: 1 declared, 1 parsed`、`CodeRabbit nitpick comments: 2 declared, 2 parsed`，parser warning 清零 - 2026-04-20：运行时条件分支 follow-up - 结果：已补齐 - 备注：`YamlConfigSchemaValidator` 现对非 object 的 `if` / `then` / `else` 使用分支级诊断路径；运行时测试新增 `else` 缺失 `if` 回归 - 2026-04-20：`bun run test`（`tools/gframework-config-tool`） - - 结果：通过（118 tests） + - 结果：通过（122 tests） + - 备注：新增条件分支坏形状回归后，tooling 现在会拒绝缺失 `type: "object"`、坏形状 `properties`、坏形状 `required` 与空白 required 成员 - 2026-04-20：`dotnet test GFramework.SourceGenerators.Tests/GFramework.SourceGenerators.Tests.csproj -c Release --filter "FullyQualifiedName~SchemaConfigGeneratorTests"` - 结果：通过（46 tests） - 2026-04-20：`dotnet test GFramework.Game.Tests/GFramework.Game.Tests.csproj -c Release --filter "FullyQualifiedName~YamlConfigLoaderIfThenElseTests"` diff --git a/tools/gframework-config-tool/src/configValidation.js b/tools/gframework-config-tool/src/configValidation.js index 50aae63e..6c7b193c 100644 --- a/tools/gframework-config-tool/src/configValidation.js +++ b/tools/gframework-config-tool/src/configValidation.js @@ -1485,6 +1485,10 @@ function parseConditionalObjectSchema(rawSchema, displayPath, keywordName, prope throw new Error(`Schema property '${displayPath}' must declare '${keywordName}' as an object-valued schema.`); } + if (rawSchema.type !== "object") { + throw new Error(`Schema property '${displayPath}' must declare an object-typed '${keywordName}' schema.`); + } + validateConditionalSchemaTargets(rawSchema, displayPath, keywordName, properties); const conditionalSchema = parseSchemaNode(rawSchema, `${displayPath}[${keywordName}]`); if (conditionalSchema.type !== "object") { @@ -1534,9 +1538,14 @@ function validateDeclaredTargetReferences(rawSchema, displayPath, contextLabel, return; } - if (rawSchema.properties && - typeof rawSchema.properties === "object" && - !Array.isArray(rawSchema.properties)) { + if (rawSchema.properties !== undefined) { + if (!rawSchema.properties || + typeof rawSchema.properties !== "object" || + Array.isArray(rawSchema.properties)) { + throw new Error( + `Schema property '${displayPath}' must declare 'properties' in ${contextLabel} as an object-valued map.`); + } + for (const propertyName of Object.keys(rawSchema.properties)) { if (Object.prototype.hasOwnProperty.call(properties, propertyName)) { continue; @@ -1548,13 +1557,24 @@ function validateDeclaredTargetReferences(rawSchema, displayPath, contextLabel, } } - if (!Array.isArray(rawSchema.required)) { + if (rawSchema.required === undefined) { return; } + if (!Array.isArray(rawSchema.required)) { + throw new Error( + `Schema property '${displayPath}' must declare 'required' in ${contextLabel} as an array of property names.`); + } + for (const requiredProperty of rawSchema.required) { - if (typeof requiredProperty !== "string" || requiredProperty.trim().length === 0) { - continue; + if (typeof requiredProperty !== "string") { + throw new Error( + `Schema property '${displayPath}' must declare 'required' entries in ${contextLabel} as property-name strings.`); + } + + if (requiredProperty.trim().length === 0) { + throw new Error( + `Schema property '${displayPath}' cannot declare blank property names in 'required' for ${contextLabel}.`); } if (Object.prototype.hasOwnProperty.call(properties, requiredProperty)) { diff --git a/tools/gframework-config-tool/test/configValidation.test.js b/tools/gframework-config-tool/test/configValidation.test.js index ddc2e1c2..dcfe00ab 100644 --- a/tools/gframework-config-tool/test/configValidation.test.js +++ b/tools/gframework-config-tool/test/configValidation.test.js @@ -2846,6 +2846,125 @@ test("parseSchemaContent should reject then declarations without if", () => { /must declare 'if' when using 'then' or 'else'/u); }); +test("parseSchemaContent should require explicit object type for conditional branches", () => { + assert.throws( + () => parseSchemaContent(` + { + "type": "object", + "properties": { + "reward": { + "type": "object", + "properties": { + "itemId": { "type": "string" } + }, + "if": { + "properties": { + "itemId": { "type": "string", "const": "potion" } + } + }, + "then": { + "type": "object", + "properties": { + "itemId": { "type": "string" } + } + } + } + } + } + `), + /must declare an object-typed 'if' schema/u); +}); + +test("parseSchemaContent should reject conditional branches with non-object properties metadata", () => { + assert.throws( + () => parseSchemaContent(` + { + "type": "object", + "properties": { + "reward": { + "type": "object", + "properties": { + "itemId": { "type": "string" } + }, + "if": { + "type": "object", + "properties": [] + }, + "then": { + "type": "object", + "properties": { + "itemId": { "type": "string" } + } + } + } + } + } + `), + /must declare 'properties' in 'if' as an object-valued map/u); +}); + +test("parseSchemaContent should reject conditional branches with non-array required metadata", () => { + assert.throws( + () => parseSchemaContent(` + { + "type": "object", + "properties": { + "reward": { + "type": "object", + "properties": { + "itemCount": { "type": "integer" } + }, + "if": { + "type": "object", + "properties": { + "itemCount": { "type": "integer" } + } + }, + "then": { + "type": "object", + "required": "itemCount", + "properties": { + "itemCount": { "type": "integer" } + } + } + } + } + } + `), + /must declare 'required' in 'then' as an array of property names/u); +}); + +test("parseSchemaContent should reject conditional branches with invalid required entries", () => { + assert.throws( + () => parseSchemaContent(` + { + "type": "object", + "properties": { + "reward": { + "type": "object", + "properties": { + "bonus": { "type": "integer" } + }, + "if": { + "type": "object", + "properties": { + "bonus": { "type": "integer" } + } + }, + "else": { + "type": "object", + "required": [" "], + "properties": { + "bonus": { "type": "integer" } + } + } + } + } + } + `), + /cannot declare blank property names in 'required' for 'else'/u); +}); + test("validateParsedConfig should report then violations", () => { const schema = parseSchemaContent(` {