From c52f2b6e0517cbc57cb7e604cf6b5e8a51f20aff Mon Sep 17 00:00:00 2001 From: chichuan Date: Thu, 6 Aug 2026 10:41:23 +0800 Subject: [PATCH 01/13] release: use isolated changelog fragments --- .changes/README.md | 29 +++++ .github/PULL_REQUEST_TEMPLATE.md | 6 +- .github/workflows/ci.yml | 6 + .github/workflows/release.yml | 2 +- CONTRIBUTING.md | 5 +- docs/ci-pr-gates.md | 22 ++-- docs/releasing.md | 15 ++- scripts/policy/check-changelog-pr.sh | 11 ++ scripts/policy/check-release-fragments.sh | 95 +++++++++++++++ scripts/release/prepare-changelog.sh | 27 +++-- scripts/release/render-release-fragments.sh | 73 +++++++++++ test/scripts/changelog_pr_gate_test.go | 127 +++++++++++++++++--- test/scripts/release_script_test.go | 66 +++++----- 13 files changed, 409 insertions(+), 75 deletions(-) create mode 100644 .changes/README.md create mode 100755 scripts/policy/check-release-fragments.sh create mode 100755 scripts/release/render-release-fragments.sh diff --git a/.changes/README.md b/.changes/README.md new file mode 100644 index 00000000..86c31701 --- /dev/null +++ b/.changes/README.md @@ -0,0 +1,29 @@ +# Release fragments + +普通功能、修复和面向用户的行为变更不要再修改根目录 `CHANGELOG.md` 的 +`Unreleased` 区域。每个 PR 在本目录新增一个独立的 Markdown fragment，避免 +并行 PR 争用同一文件。 + +文件名使用能唯一定位变更的短名，通常是 PR 号，例如 +`1234-chat-reply-mentions.md`。文件格式严格如下： + +```markdown +--- +category: Added +--- + +- **Chat reply mentions** (#1234) — supports mentioning selected members. +``` + +`category` 只能是 `Added`、`Changed`、`Deprecated`、`Removed`、`Fixed` 或 +`Security`。正文至少包含一个 Markdown 列表项，且不得包含 `TODO` 或 `TBD`。 + +发布 beta 时，`scripts/release/prepare-changelog.sh` 会按分类和文件名稳定排序， +将未归档 fragments 汇总为唯一的版本章节，并移动到 +`.changes/released//`。因此 release-seal PR 是唯一会修改 +`CHANGELOG.md` 的 PR；它同时归档已消费的 fragments，供审计追溯。 +归档只能在同一个 release-seal PR 中以原样移动完成；CI 会拒绝直接修改、 +删除或重写已归档文件。 + +无需面向用户发布说明的改动不添加 fragment。评审者根据改动是否可见来判断该 +例外是否成立。 diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 391364b6..8b54e4f2 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -19,8 +19,10 @@ repeat the entire CI suite locally only to fill this checklist: CI expands the selected tier from documentation checks, through affected-package tests, to the complete high-risk suite. -- [ ] Exact in-place `CHANGELOG.md`-only check (otherwise `N/A`): - `./scripts/policy/check-changelog-pr.sh --fast-path "$(git merge-base HEAD origin/main)" HEAD` +- [ ] Release fragment added for a user-visible behavior/interface change (otherwise `N/A`): + `.changes/.md`; ordinary PRs must not edit `CHANGELOG.md`. +- [ ] Release-seal validation (otherwise `N/A`): + `./scripts/policy/check-changelog-pr.sh --content-only "$(git merge-base HEAD origin/main)" HEAD` - [ ] Targeted test/check commands and results: - [ ] Behavior evidence (test name, CLI output shape, or before/after result): - [ ] Documentation links/content/rendering checked (documentation-only, otherwise diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bada42c2..06eb7ed5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1283,6 +1283,12 @@ jobs: ./scripts/policy/check-changelog-pr.sh \ "$mode" "$PR_BASE_SHA" HEAD + - name: Validate release fragment lifecycle + if: github.event_name == 'pull_request' + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: ./scripts/policy/check-release-fragments.sh "$PR_BASE_SHA" HEAD + - name: Validate trusted main CHANGELOG-only push if: github.event_name == 'push' && needs.lint.outputs.changelog_only == 'true' env: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 96944724..08e83fd8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2793,7 +2793,7 @@ jobs: fi if test "${{ needs.dispatch-contract.outputs.mode }}" = plan_release; then echo - echo "Plan only: no tag or package was created. Add the exact \`CHANGELOG.md\` section, merge it to main, then run publish." + echo "Plan only: no tag or package was created. Render pending \`.changes/*.md\` fragments into the exact \`CHANGELOG.md\` section, merge the release-seal PR to main, then run publish." fi } >> "$GITHUB_STEP_SUMMARY" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a17dcee..dc4ce719 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,7 +75,10 @@ coverage is additionally selected for platform-sensitive code. change. 6. Run `./scripts/release/verify-package-managers.sh` when packaging or installer surfaces change (run `make package` first). -7. Update docs and `CHANGELOG.md` for behavior/interface changes. +7. Update docs and add one `.changes/.md` release fragment for + behavior/interface changes. Do not edit `CHANGELOG.md` in an ordinary PR; + the release-seal workflow renders and archives fragments into the versioned + changelog section. ## Submission Flow diff --git a/docs/ci-pr-gates.md b/docs/ci-pr-gates.md index 69f9a6e3..86ccd69d 100644 --- a/docs/ci-pr-gates.md +++ b/docs/ci-pr-gates.md @@ -48,8 +48,12 @@ It then runs: --fast-path "$PR_BASE_SHA" HEAD ``` -Because the verified PR diff contains only `CHANGELOG.md`, the validator and -its policy dependencies in that merge tree are byte-for-byte the current base +The exact fast path remains limited to historic one-file maintenance. A +release-seal PR uses `--content-only`, which permits the generated +`CHANGELOG.md` change together with archival moves from `.changes/` to +`.changes/released/`; it receives the normal scoped admission instead of this +fast path. Ordinary PRs must not modify `CHANGELOG.md`; they add a standalone +release fragment instead. The validator and its policy dependencies in that merge tree are byte-for-byte the current base versions. Validation targets the synthetic merge tree, not the feature-branch tree, so a stale branch cannot supply an older validator or combine with newer base notes into an invalid final CHANGELOG. @@ -79,10 +83,12 @@ to the complete main admission suite. A source change can therefore never inherit the CHANGELOG-only result. Any PR that touches `CHANGELOG.md` but also changes another file runs the same -content contract in `Policy` with `--content-only`. That mode permits the -second file but still rejects invalid dates or versions, missing bullets, -placeholder `TODO`/`TBD`, unmanaged-section changes, and unsafe tree modes. -Adding a second file therefore cannot bypass CHANGELOG validation. +content contract in `Policy` with `--content-only`. That mode accepts only +fragment archival moves (`.changes/.md` to +`.changes/released//.md`) alongside the changelog; source and +documentation changes are rejected. It still rejects invalid dates or +versions, missing bullets, placeholder `TODO`/`TBD`, unmanaged-section +changes, and unsafe tree modes. ## Risk tiers and downstream boundaries @@ -166,11 +172,11 @@ make mock-mcp-smoke go test -v -count=1 ./pkg/editiontest/... ``` -For an exact CHANGELOG-only branch: +For a release-seal branch that archives rendered fragments: ```sh base_ref=$(git merge-base HEAD origin/main) -./scripts/policy/check-changelog-pr.sh --fast-path "$base_ref" HEAD +./scripts/policy/check-changelog-pr.sh --content-only "$base_ref" HEAD ``` `make coverage-gate` is an enforcement step, not a profile generator. For a diff --git a/docs/releasing.md b/docs/releasing.md index db12b52f..428cdbcc 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -17,7 +17,8 @@ 1. 在上述 `Release` 页面选择 `Run workflow`，分支必须是默认分支 `main`。 2. `release_operation=plan`，选择 `release_channel=beta|stable`；仅在开始新 beta 线时选择 `release_bump=patch|minor|major`。 -3. workflow summary 会给出唯一的下一版本。把对应的精确 `CHANGELOG.md` 章节通过 PR 合入 `main`。 +3. workflow summary 会给出唯一的下一版本。运行 `prepare-changelog.sh` 将已合入的 + release fragments 汇总成对应的精确 `CHANGELOG.md` 章节，并通过唯一的 release-seal PR 合入 `main`。 4. 再次运行，改为 `release_operation=publish`。beta 会直接进入自动化发布；stable 会在封 tag 前等待管理员签收。 `plan` 是纯只读操作，不创建 tag、预留版本号或生成包。CHANGELOG 合入期间若另一个发布先占用了该版本，`publish` 会重新分配并因 CHANGELOG 章节不匹配而拒绝，需要重新 plan。`publish` 会先再次确认 dispatch SHA 仍是当前 `main`、Code Admission 和平台治理均通过，再由唯一的 write job 使用 GitHub API 原子创建 annotated tag；同一次 run 随即进入既有的跨平台构建、GitHub/npm、可选 OSS/Gitee 发布和 Homebrew 直交付 DAG。内置 `GITHUB_TOKEN` 创建的 tag 不依赖第二条 workflow 被再次触发。 @@ -94,7 +95,8 @@ main 上的候选代码 + beta CHANGELOG dws-release v1.2.3-beta.1 ``` -如果 CHANGELOG 尚不存在，该命令只生成模板并停止。补全内容、删除所有 `TODO`，提交后通过 PR 合入 `main`；然后重新运行完全相同的命令，它会执行完整预检： +如果 CHANGELOG 尚不存在，该命令会从 `.changes/*.md` 生成 beta 章节并归档已消费的 +fragments，然后停止。审阅生成内容并通过唯一的 release-seal PR 合入 `main`；然后重新运行完全相同的命令，它会执行完整预检： ```bash dws-release v1.2.3-beta.1 @@ -132,6 +134,15 @@ dws-release v1.2.3 --from-beta v1.2.3-beta.1 正式版使用 `## [1.2.3] - YYYY-MM-DD`。该章节会直接成为 GitHub Release Notes。 +### Release fragments + +普通 PR 不修改 `CHANGELOG.md` 的 `Unreleased` 区域。需要面向用户发布说明的改动在 +`.changes/.md` 中增加一个独立 fragment；格式和允许的分类见 +[`.changes/README.md`](../.changes/README.md)。预发封板时 +`scripts/release/prepare-changelog.sh prerelease ` 会稳定排序并汇总所有未归档 +fragment，写入唯一版本章节后移动到 `.changes/released//`。因此并发 PR 不会争用 +`CHANGELOG.md`；唯一的 release-seal PR 同时提交生成的章节与归档移动，供审计复核。 + ## CI/CD 保证 - 只接受 `vX.Y.Z-beta.N` 和 `vX.Y.Z`，且新版本必须高于上一正式版。这里的“上一正式版”必须同时具备公开非草稿 GitHub Release 和同 tag/commit 的成功 Release workflow；只有 tag、没有交付成功的孤儿版本会阻断后续发布，要求走机器核验恢复补齐。云端 tag 会固定 `Release-Run`、requester、commit 和版本分配指纹，交付验证按该精确 run/attempt 及完整 job graph 取证，不接受任意 `workflow_dispatch`。历史版本若曾通过专用 recovery workflow 完成交付，只能使用仓库内 `delivered-stable-recoveries.json` 中精确到 tag、commit、run、workflow SHA 与 attempt 的 reviewed 证据。 diff --git a/scripts/policy/check-changelog-pr.sh b/scripts/policy/check-changelog-pr.sh index b31af5cf..a88d4045 100755 --- a/scripts/policy/check-changelog-pr.sh +++ b/scripts/policy/check-changelog-pr.sh @@ -119,6 +119,17 @@ content-only) fi exit 1 fi + if ! awk -F ' ' ' + $1 == "M" && $2 == "CHANGELOG.md" && NF == 2 { next } + $1 == "D" && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ && NF == 2 { next } + $1 == "A" && $2 ~ /^\.changes\/released\/[0-9]+\.[0-9]+\.[0-9]+(-beta\.[1-9][0-9]*)?\/[a-z0-9][a-z0-9._-]*\.md$/ && NF == 2 { next } + $1 ~ /^R[0-9]+$/ && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ && $3 ~ /^\.changes\/released\/[0-9]+\.[0-9]+\.[0-9]+(-beta\.[1-9][0-9]*)?\/[a-z0-9][a-z0-9._-]*\.md$/ && NF == 3 { next } + { invalid = 1 } + END { exit invalid } + ' "$TMP_ROOT/name-status"; then + printf '%s\n' 'error: CHANGELOG.md may accompany only release-fragment archival; ordinary PRs must add .changes/.md without editing CHANGELOG.md' >&2 + exit 1 + fi ;; esac diff --git a/scripts/policy/check-release-fragments.sh b/scripts/policy/check-release-fragments.sh new file mode 100755 index 00000000..2610f32b --- /dev/null +++ b/scripts/policy/check-release-fragments.sh @@ -0,0 +1,95 @@ +#!/bin/sh +set -eu + +ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd)" + +usage() { printf '%s\n' 'usage: check-release-fragments.sh BASE HEAD' >&2; } + +[ "$#" -eq 2 ] || { usage; exit 2; } +base="$(git -C "$ROOT" rev-parse --verify --quiet "$1^{commit}")" || { usage; exit 2; } +head="$(git -C "$ROOT" rev-parse --verify --quiet "$2^{commit}")" || { usage; exit 2; } +merge_base="$(git -C "$ROOT" merge-base "$base" "$head")" +tmp_root="$(mktemp -d "${TMPDIR:-/tmp}/dws-release-fragment-policy.XXXXXX")" +cleanup() { rm -rf "$tmp_root"; } +trap cleanup EXIT HUP INT TERM + +git -C "$ROOT" diff --no-ext-diff --find-renames --name-status "$merge_base" "$head" >"$tmp_root/status" + +archive_changed=false +if awk -F '\t' '{ for (field = 2; field <= NF; field++) if ($field ~ /^\.changes\/released\//) found = 1 } END { exit !found }' "$tmp_root/status"; then + archive_changed=true +fi + +if [ "$archive_changed" = true ]; then + release_version="$(git -C "$ROOT" diff --no-ext-diff --unified=0 "$merge_base" "$head" -- CHANGELOG.md | sed -n 's/^+## \[\([0-9][0-9.]*\(-beta\.[1-9][0-9]*\)\{0,1\}\)\] - .*/\1/p')" + [ "$(printf '%s\n' "$release_version" | sed '/^$/d' | wc -l | tr -d '[:space:]')" -eq 1 ] || { + printf '%s\n' 'error: release-fragment archival requires exactly one newly added versioned CHANGELOG section' >&2 + exit 1 + } + if ! awk -F '\t' -v version="$release_version" ' + $1 == "M" && $2 == "CHANGELOG.md" && NF == 2 { changelog = 1; next } + $1 ~ /^R100$/ && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ && $3 ~ ("^\\.changes/released/" version "/[a-z0-9][a-z0-9._-]*\\.md$") && NF == 3 { + source = $2; target = $3; sub(/^.*\//, "", source); sub(/^.*\//, "", target) + if (source != target) invalid = 1 + moved++; next + } + { invalid = 1 } + END { exit !(changelog && moved > 0 && !invalid) } + ' "$tmp_root/status"; then + printf '%s\n' 'error: release fragments must be unchanged R100 moves from .changes/.md to .changes/released//.md in the matching release-seal PR' >&2 + exit 1 + fi + source_changes="$tmp_root/source-changes" + mkdir -p "$source_changes" + git -C "$ROOT" ls-tree -r --name-only "$merge_base" -- .changes | + while IFS= read -r path; do + case "$path" in + .changes/*.md) + name="${path#.changes/}" + mkdir -p "$(dirname "$source_changes/$name")" + git -C "$ROOT" show "$merge_base:$path" >"$source_changes/$name" + ;; + esac + done + "$ROOT/scripts/release/render-release-fragments.sh" "$source_changes" >"$tmp_root/expected-notes" + git -C "$ROOT" show "$head:CHANGELOG.md" | + awk -v heading="## [$release_version] - " ' + index($0, heading) == 1 { found = 1; next } + found && /^## / { exit } + found { print } + ' >"$tmp_root/actual-notes" + normalize_notes() { + awk ' + /^[[:space:]]*$/ && !started { next } + { started = 1; lines[++count] = $0 } + END { + while (count > 0 && lines[count] ~ /^[[:space:]]*$/) count-- + for (line_no = 1; line_no <= count; line_no++) print lines[line_no] + } + ' "$1" + } + normalize_notes "$tmp_root/expected-notes" >"$tmp_root/expected-notes.normalized" + normalize_notes "$tmp_root/actual-notes" >"$tmp_root/actual-notes.normalized" + if ! cmp -s "$tmp_root/expected-notes.normalized" "$tmp_root/actual-notes.normalized"; then + printf '%s\n' 'error: release-seal CHANGELOG section does not exactly match the rendered active release fragments' >&2 + diff -u "$tmp_root/expected-notes.normalized" "$tmp_root/actual-notes.normalized" >&2 || true + exit 1 + fi +else + if awk -F '\t' '{ for (field = 2; field <= NF; field++) if ($field ~ /^\.changes\/released\//) invalid = 1 } END { exit !invalid }' "$tmp_root/status"; then + printf '%s\n' 'error: archived release fragments are immutable outside their release-seal PR' >&2 + exit 1 + fi + if awk -F '\t' ' + $1 == "D" && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ { invalid = 1 } + $1 ~ /^R[0-9]+$/ && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ { invalid = 1 } + END { exit !invalid } + ' "$tmp_root/status"; then + printf '%s\n' 'error: active release fragments may be deleted or renamed only by the matching release-seal archival move' >&2 + exit 1 + fi +fi + +if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | grep -Eq '^\.changes/[a-z0-9][a-z0-9._-]*\.md$'; then + "$ROOT/scripts/release/render-release-fragments.sh" "$ROOT/.changes" >/dev/null +fi diff --git a/scripts/release/prepare-changelog.sh b/scripts/release/prepare-changelog.sh index 0c718fa3..053a46b7 100755 --- a/scripts/release/prepare-changelog.sh +++ b/scripts/release/prepare-changelog.sh @@ -10,6 +10,7 @@ VERSION="${2:-}" FROM_BETA="" FROM_REF="" CHANGELOG="$ROOT/CHANGELOG.md" +CHANGES_DIR="$ROOT/.changes" usage() { cat >&2 <<'EOF' @@ -19,6 +20,7 @@ Options: --from-beta Required for stable release notes --from-ref Commit-list baseline for prerelease notes --changelog Override CHANGELOG.md path + --changes-dir Override release fragment directory EOF } @@ -29,6 +31,7 @@ while [ "$#" -gt 0 ]; do --from-beta) [ "$#" -ge 2 ] || { usage; exit 2; }; FROM_BETA="$2"; shift 2 ;; --from-ref) [ "$#" -ge 2 ] || { usage; exit 2; }; FROM_REF="$2"; shift 2 ;; --changelog) [ "$#" -ge 2 ] || { usage; exit 2; }; CHANGELOG="$2"; shift 2 ;; + --changes-dir) [ "$#" -ge 2 ] || { usage; exit 2; }; CHANGES_DIR="$2"; shift 2 ;; -h|--help) usage; exit 0 ;; *) printf 'unknown argument: %s\n' "$1" >&2; usage; exit 2 ;; esac @@ -60,21 +63,19 @@ fi release_date="${DWS_RELEASE_DATE:-$(TZ=Asia/Shanghai date +%F)}" section="$(mktemp "${TMPDIR:-/tmp}/dws-changelog-section.XXXXXX")" output="$(mktemp "${TMPDIR:-/tmp}/dws-changelog-output.XXXXXX")" -cleanup() { rm -f "$section" "$output"; } +fragments="$(mktemp "${TMPDIR:-/tmp}/dws-changelog-fragments.XXXXXX")" +cleanup() { rm -f "$section" "$output" "$fragments"; } trap cleanup EXIT HUP INT TERM { printf '## [%s] - %s\n\n' "$semver" "$release_date" if [ "$CHANNEL" = "stable" ]; then printf 'This release promotes the sealed `%s` contents to stable.\n\n' "$FROM_BETA" - else - printf '\n\n' - fi - printf '### Changed\n\n' - if [ "$CHANNEL" = "stable" ]; then + printf '### Changed\n\n' printf -- '- TODO: summarize the complete user-visible release promoted from `%s`.\n' "$FROM_BETA" else - printf -- '- TODO: summarize this beta candidate and its validation scope.\n' + "$SCRIPT_DIR/render-release-fragments.sh" "$CHANGES_DIR" >"$fragments" + cat "$fragments" fi } > "$section" @@ -105,7 +106,17 @@ fi [ "$inserted" -eq 1 ] || { printf 'CHANGELOG is missing ## [Unreleased]\n' >&2; exit 1; } cp "$output" "$CHANGELOG" -printf 'Prepared CHANGELOG template for %s. Replace TODO, review, commit, and merge it before release.\n' "$VERSION" +if [ "$CHANNEL" = "prerelease" ]; then + archive_dir="$CHANGES_DIR/released/$semver" + mkdir -p "$archive_dir" + find "$CHANGES_DIR" -mindepth 1 -maxdepth 1 -type f -name '*.md' ! -name 'README.md' -exec mv {} "$archive_dir"/ \; +fi + +if [ "$CHANNEL" = "stable" ]; then + printf 'Prepared CHANGELOG template for %s. Replace TODO, review, commit, and merge it before release.\n' "$VERSION" +else + printf 'Prepared CHANGELOG and archived release fragments for %s. Review, commit, and merge the release-seal PR before release.\n' "$VERSION" +fi if [ -n "$FROM_REF" ] && git rev-parse --verify --quiet "$FROM_REF^{commit}" >/dev/null; then printf '\nCommits since %s:\n' "$FROM_REF" git log --oneline "$FROM_REF..HEAD" diff --git a/scripts/release/render-release-fragments.sh b/scripts/release/render-release-fragments.sh new file mode 100755 index 00000000..c6ed6796 --- /dev/null +++ b/scripts/release/render-release-fragments.sh @@ -0,0 +1,73 @@ +#!/bin/sh +set -eu + +CHANGES_DIR="${1:-.changes}" + +usage() { + printf '%s\n' 'usage: render-release-fragments.sh [changes-dir]' >&2 +} + +[ "$#" -le 1 ] || { usage; exit 2; } +[ -d "$CHANGES_DIR" ] || { printf 'release fragments directory not found: %s\n' "$CHANGES_DIR" >&2; exit 1; } + +tmp_root="$(mktemp -d "${TMPDIR:-/tmp}/dws-release-fragments.XXXXXX")" +cleanup() { rm -rf "$tmp_root"; } +trap cleanup EXIT HUP INT TERM + +find "$CHANGES_DIR" -mindepth 1 -maxdepth 1 -type f -name '*.md' ! -name 'README.md' -print | + LC_ALL=C sort >"$tmp_root/files" + +[ -s "$tmp_root/files" ] || { + printf '%s\n' 'no release fragments found; add .changes/.md before preparing a prerelease changelog' >&2 + exit 1 +} + +validate_fragment() { + fragment="$1" + base="$(basename "$fragment")" + case "$base" in + [a-z0-9]*.md) ;; + *) printf 'invalid release fragment filename: %s\n' "$fragment" >&2; return 1 ;; + esac + + [ "$(sed -n '1p' "$fragment")" = '---' ] && + [ "$(sed -n '3p' "$fragment")" = '---' ] || { + printf 'invalid release fragment header: %s\n' "$fragment" >&2 + return 1 + } + case "$(sed -n '2p' "$fragment")" in + 'category: Added'|'category: Changed'|'category: Deprecated'|'category: Removed'|'category: Fixed'|'category: Security') ;; + *) printf 'invalid release fragment category: %s\n' "$fragment" >&2; return 1 ;; + esac + + body="$(sed -n '4,$p' "$fragment")" + printf '%s\n' "$body" | grep -Eq '^- [^[:space:]].*' || { + printf 'release fragment must contain a non-empty Markdown list item: %s\n' "$fragment" >&2 + return 1 + } + if printf '%s\n' "$body" | grep -Eqi '(^|[^[:alnum:]_])(TODO|TBD)([^[:alnum:]_]|$)'; then + printf 'release fragment must not contain TODO/TBD: %s\n' "$fragment" >&2 + return 1 + fi +} + +while IFS= read -r fragment; do + validate_fragment "$fragment" +done <"$tmp_root/files" + +for category in Added Changed Deprecated Removed Fixed Security; do + category_files="$tmp_root/$category" + : >"$category_files" + while IFS= read -r fragment; do + if [ "$(sed -n '2p' "$fragment")" = "category: $category" ]; then + printf '%s\n' "$fragment" >>"$category_files" + fi + done <"$tmp_root/files" + [ -s "$category_files" ] || continue + + printf '### %s\n\n' "$category" + while IFS= read -r fragment; do + awk 'NR >= 4 { if ($0 ~ /[^[:space:]]/) started = 1; if (started) print }' "$fragment" + printf '\n' + done <"$category_files" +done diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 4f485999..00fea1ea 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -9,9 +9,10 @@ import ( ) type changelogGateRepo struct { - root string - gate string - base string + root string + gate string + fragmentPolicy string + base string } const changelogGateBase = `# Changelog @@ -69,8 +70,10 @@ func newChangelogGateRepo(t *testing.T) *changelogGateRepo { } for _, path := range []string{ "scripts/policy/check-changelog-pr.sh", + "scripts/policy/check-release-fragments.sh", "scripts/policy/open-source-audit.sh", "scripts/release/release-lib.sh", + "scripts/release/render-release-fragments.sh", } { data, err := os.ReadFile(filepath.Join(sourceRoot, path)) if err != nil { @@ -91,12 +94,21 @@ func newChangelogGateRepo(t *testing.T) *changelogGateRepo { changelogGateGit(t, root, "commit", "-m", "seed repository") return &changelogGateRepo{ - root: root, - gate: filepath.Join(root, "scripts", "policy", "check-changelog-pr.sh"), - base: strings.TrimSpace(changelogGateGit(t, root, "rev-parse", "HEAD")), + root: root, + gate: filepath.Join(root, "scripts", "policy", "check-changelog-pr.sh"), + fragmentPolicy: filepath.Join(root, "scripts", "policy", "check-release-fragments.sh"), + base: strings.TrimSpace(changelogGateGit(t, root, "rev-parse", "HEAD")), } } +func (r *changelogGateRepo) runFragmentPolicy(t *testing.T, base, head string) (string, error) { + t.Helper() + cmd := exec.Command("sh", r.fragmentPolicy, base, head) + cmd.Dir = r.root + output, err := cmd.CombinedOutput() + return string(output), err +} + func changelogGateWrite(t *testing.T, root, path, content string, mode os.FileMode) { t.Helper() full := filepath.Join(root, path) @@ -203,22 +215,110 @@ func TestChangelogPRGateAcceptsTargetedChanges(t *testing.T) { } } -func TestChangelogPRContentOnlyAllowsOtherFiles(t *testing.T) { +func TestChangelogPRContentOnlyRejectsOrdinaryChangesAlongsideChangelog(t *testing.T) { repo := newChangelogGateRepo(t) changelogGateWrite(t, repo.root, "CHANGELOG.md", changelogGateValidRelease, 0o644) changelogGateWrite(t, repo.root, "internal/change.go", "package internal\n", 0o644) repo.commit(t, "change code with release notes") output, err := repo.runMode(t, "--content-only") - if err != nil { - t.Fatalf("content-only gate error = %v\noutput:\n%s", err, output) - } - if !strings.Contains(output, "CHANGELOG PR check: ok (mode=content-only") { - t.Fatalf("content-only gate output missing success marker:\n%s", output) + if err == nil || !strings.Contains(output, "may accompany only release-fragment archival") { + t.Fatalf("content-only gate accepted ordinary source change: err=%v\noutput:\n%s", err, output) } } -func TestChangelogPRContentOnlyStillValidatesContentWithOtherFiles(t *testing.T) { +func TestChangelogPRContentOnlyAcceptsReleaseFragmentArchival(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/1234-chat.md", "---\ncategory: Added\n---\n\n- Chat reply mentions.\n", 0o644) + repo.commit(t, "add release fragment") + sealBase := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + changelogGateWrite(t, repo.root, "CHANGELOG.md", `# Changelog + +## [Unreleased] + +## [1.0.1-beta.1] - 2026-07-17 + +### Added + +- Chat reply mentions. + +## [1.0.0] - 2026-07-01 + +### Added + +- Initial release. +`, 0o644) + if err := os.MkdirAll(filepath.Join(repo.root, ".changes", "released", "1.0.1-beta.1"), 0o755); err != nil { + t.Fatalf("MkdirAll archive: %v", err) + } + if err := os.Rename(filepath.Join(repo.root, ".changes", "1234-chat.md"), filepath.Join(repo.root, ".changes", "released", "1.0.1-beta.1", "1234-chat.md")); err != nil { + t.Fatalf("Rename release fragment: %v", err) + } + repo.commit(t, "seal release notes") + + output, err := repo.runMode(t, "--content-only") + if err != nil { + t.Fatalf("content-only release seal error = %v\noutput:\n%s", err, output) + } + if !strings.Contains(output, "CHANGELOG PR check: ok (mode=content-only") { + t.Fatalf("content-only release seal output missing success marker:\n%s", output) + } + if output, err := repo.runFragmentPolicy(t, sealBase, "HEAD"); err != nil { + t.Fatalf("release fragment policy rejected matching seal: %v\noutput:\n%s", err, output) + } +} + +func TestReleaseFragmentPolicyRejectsInvalidActiveFragmentAndWrongArchiveVersion(t *testing.T) { + t.Run("invalid active fragment", func(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/1234-invalid.md", "---\ncategory: Added\n---\n\n- TODO: fill this in.\n", 0o644) + repo.commit(t, "add invalid fragment") + output, err := repo.runFragmentPolicy(t, repo.base, "HEAD") + if err == nil || !strings.Contains(output, "must not contain TODO/TBD") { + t.Fatalf("invalid active fragment passed: err=%v\noutput:\n%s", err, output) + } + }) + + t.Run("archive version differs from changelog", func(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/1234-chat.md", "---\ncategory: Added\n---\n\n- Chat reply mentions.\n", 0o644) + repo.commit(t, "add release fragment") + sealBase := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + changelogGateWrite(t, repo.root, "CHANGELOG.md", changelogGateValidRelease, 0o644) + if err := os.MkdirAll(filepath.Join(repo.root, ".changes", "released", "1.0.2-beta.1"), 0o755); err != nil { + t.Fatalf("MkdirAll archive: %v", err) + } + if err := os.Rename(filepath.Join(repo.root, ".changes", "1234-chat.md"), filepath.Join(repo.root, ".changes", "released", "1.0.2-beta.1", "1234-chat.md")); err != nil { + t.Fatalf("Rename release fragment: %v", err) + } + repo.commit(t, "archive under wrong release") + output, err := repo.runFragmentPolicy(t, sealBase, "HEAD") + if err == nil || !strings.Contains(output, "unchanged R100 moves") { + t.Fatalf("wrong archive version passed: err=%v\noutput:\n%s", err, output) + } + }) + + t.Run("archive notes differ from rendered fragment", func(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/1234-chat.md", "---\ncategory: Added\n---\n\n- Chat reply mentions.\n", 0o644) + repo.commit(t, "add release fragment") + sealBase := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + changelogGateWrite(t, repo.root, "CHANGELOG.md", changelogGateValidRelease, 0o644) + if err := os.MkdirAll(filepath.Join(repo.root, ".changes", "released", "1.0.1-beta.1"), 0o755); err != nil { + t.Fatalf("MkdirAll archive: %v", err) + } + if err := os.Rename(filepath.Join(repo.root, ".changes", "1234-chat.md"), filepath.Join(repo.root, ".changes", "released", "1.0.1-beta.1", "1234-chat.md")); err != nil { + t.Fatalf("Rename release fragment: %v", err) + } + repo.commit(t, "seal mismatched release notes") + output, err := repo.runFragmentPolicy(t, sealBase, "HEAD") + if err == nil || !strings.Contains(output, "does not exactly match") { + t.Fatalf("mismatched release notes passed: err=%v\noutput:\n%s", err, output) + } + }) +} + +func TestChangelogPRContentOnlyStillValidatesReleaseNotes(t *testing.T) { tests := []struct { name string changelog string @@ -250,7 +350,6 @@ func TestChangelogPRContentOnlyStillValidatesContentWithOtherFiles(t *testing.T) t.Run(test.name, func(t *testing.T) { repo := newChangelogGateRepo(t) changelogGateWrite(t, repo.root, "CHANGELOG.md", test.changelog, 0o644) - changelogGateWrite(t, repo.root, "internal/change.go", "package internal\n", 0o644) repo.commit(t, test.name) output, err := repo.runMode(t, "--content-only") diff --git a/test/scripts/release_script_test.go b/test/scripts/release_script_test.go index 3d3a43de..164b3ae6 100644 --- a/test/scripts/release_script_test.go +++ b/test/scripts/release_script_test.go @@ -73,6 +73,7 @@ type releaseTestRepo struct { remote string contract string prepare string + render string releaseCmd string lib string verify string @@ -107,6 +108,7 @@ func newReleaseTestRepo(t *testing.T) *releaseTestRepo { remote: remote, contract: filepath.Join(sourceRoot, "scripts", "release", "release-contract.sh"), prepare: filepath.Join(sourceRoot, "scripts", "release", "prepare-changelog.sh"), + render: filepath.Join(sourceRoot, "scripts", "release", "render-release-fragments.sh"), releaseCmd: filepath.Join(sourceRoot, "scripts", "release", "release.sh"), lib: filepath.Join(sourceRoot, "scripts", "release", "release-lib.sh"), verify: filepath.Join(sourceRoot, "scripts", "release", "verify-release-artifacts.sh"), @@ -1654,10 +1656,13 @@ func TestReleaseContractCIAllowsMainToAdvanceAfterTagSeal(t *testing.T) { } } -func TestReleasePrepareChangelogCreatesGuardedTemplate(t *testing.T) { +func TestReleasePrepareChangelogRendersAndArchivesPrereleaseFragments(t *testing.T) { r := newReleaseTestRepo(t) releaseCopyFile(t, r.lib, filepath.Join(r.root, "scripts", "release", "release-lib.sh"), 0o644) releaseCopyFile(t, r.prepare, filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), 0o755) + releaseCopyFile(t, r.render, filepath.Join(r.root, "scripts", "release", "render-release-fragments.sh"), 0o755) + mustWriteFile(t, filepath.Join(r.root, ".changes", "1234-chat-reply.md"), []byte("---\ncategory: Added\n---\n\n- **Chat reply mentions** (#1234) — supports selected member mentions.\n"), 0o644) + mustWriteFile(t, filepath.Join(r.root, ".changes", "1235-chat-fix.md"), []byte("---\ncategory: Fixed\n---\n\n- **Chat reply fallback** (#1235) — keeps replies compatible with older clients.\n"), 0o644) r.commitAndPush(t, "install changelog preparation") cmd := exec.Command("sh", filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), "prerelease", "v1.0.1-beta.1") @@ -1671,55 +1676,38 @@ func TestReleasePrepareChangelogCreatesGuardedTemplate(t *testing.T) { if err != nil { t.Fatalf("ReadFile(CHANGELOG.md) error = %v", err) } - if !strings.Contains(string(changelog), "## [1.0.1-beta.1] - 2026-07-11") || !strings.Contains(string(changelog), "TODO") { - t.Fatalf("prepared changelog missing guarded template:\n%s", changelog) + content := string(changelog) + if !strings.Contains(content, "## [1.0.1-beta.1] - 2026-07-11") || + !strings.Contains(content, "### Added") || + !strings.Contains(content, "Chat reply mentions") || + !strings.Contains(content, "### Fixed") || + !strings.Contains(content, "Chat reply fallback") || + strings.Contains(content, "TODO") { + t.Fatalf("prepared changelog did not render release fragments:\n%s", changelog) } - - r.commitAndPush(t, "commit unfinished release notes") - contractOutput, contractErr := runReleaseScript(t, r.root, r.contract, - "--repo-root", r.root, - "--channel", "prerelease", - "--version", "v1.0.1-beta.1", - "--remote", "origin", - ) - if contractErr == nil || !strings.Contains(contractOutput, "TODO/TBD") { - t.Fatalf("unfinished template was not blocked: err=%v\noutput:\n%s", contractErr, contractOutput) + for _, name := range []string{"1234-chat-reply.md", "1235-chat-fix.md"} { + if _, err := os.Stat(filepath.Join(r.root, ".changes", name)); !os.IsNotExist(err) { + t.Fatalf("unconsumed release fragment %s still present: %v", name, err) + } + if _, err := os.Stat(filepath.Join(r.root, ".changes", "released", "1.0.1-beta.1", name)); err != nil { + t.Fatalf("archived release fragment %s missing: %v", name, err) + } } } -func TestReleasePrepareChangelogKeepsUnreleasedContentAboveNewVersion(t *testing.T) { +func TestReleasePrepareChangelogRejectsInvalidReleaseFragment(t *testing.T) { r := newReleaseTestRepo(t) releaseCopyFile(t, r.lib, filepath.Join(r.root, "scripts", "release", "release-lib.sh"), 0o644) releaseCopyFile(t, r.prepare, filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), 0o755) - mustWriteFile(t, filepath.Join(r.root, "CHANGELOG.md"), []byte("# Changelog\n\n## [Unreleased]\n\n### Changed\n\n- Keep this unreleased note.\n\n## [1.0.0] - 2026-07-01\n\n### Changed\n\n- Initial release.\n"), 0o644) - r.commitAndPush(t, "add unreleased changelog note") + releaseCopyFile(t, r.render, filepath.Join(r.root, "scripts", "release", "render-release-fragments.sh"), 0o755) + mustWriteFile(t, filepath.Join(r.root, ".changes", "invalid.md"), []byte("---\ncategory: Added\n---\n\n- TODO: write this later.\n"), 0o644) + r.commitAndPush(t, "add invalid release fragment") cmd := exec.Command("sh", filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), "prerelease", "v1.0.1-beta.1") cmd.Dir = r.root cmd.Env = append(os.Environ(), "DWS_RELEASE_DATE=2026-07-11") - if output, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("prepare changelog error = %v\noutput:\n%s", err, output) - } - data, err := os.ReadFile(filepath.Join(r.root, "CHANGELOG.md")) - if err != nil { - t.Fatalf("ReadFile(CHANGELOG.md) error = %v", err) - } - content := string(data) - positions := []int{ - strings.Index(content, "## [Unreleased]"), - strings.Index(content, "- Keep this unreleased note."), - strings.Index(content, "## [1.0.1-beta.1] - 2026-07-11"), - strings.Index(content, "## [1.0.0] - 2026-07-01"), - } - for index, position := range positions { - if position < 0 { - t.Fatalf("prepared changelog is missing expected marker %d:\n%s", index, content) - } - } - for index := 1; index < len(positions); index++ { - if positions[index-1] >= positions[index] { - t.Fatalf("prepared changelog order is invalid: %v\n%s", positions, content) - } + if output, err := cmd.CombinedOutput(); err == nil || !strings.Contains(string(output), "must not contain TODO/TBD") { + t.Fatalf("invalid release fragment was accepted: err=%v\noutput:\n%s", err, output) } } From 39d6caa24df2120e6159f9e28b9a6ea7ad76acd6 Mon Sep 17 00:00:00 2001 From: chichuan Date: Tue, 11 Aug 2026 21:07:12 +0800 Subject: [PATCH 02/13] fix: validate every top-level .changes entry in the fragment gate The fragment gate only ran validation when the changed path matched the legal fragment name pattern, so `.changes/Foo.md`, `.changes/notes.txt` and a symlinked fragment slipped through untouched and then broke the next PR that added a legal fragment. The trigger now fires on any top-level `.changes/` change other than README.md and rejects every entry that is not README.md, released/, or a 100644 blob named ^[a-z0-9][a-z0-9._-]*\.md$. The renderer had the same hole from the other side: `find -type f` is false for symlinks, so a symlinked fragment was silently dropped from the rendered notes, and the `[a-z0-9]*.md` glob only constrained the first character so `chat reply.md` passed. It now walks every top-level entry and fails on symlinks, unexpected directories, non-regular files and illegal names. Both scripts pin LC_ALL=C so the ASCII ranges cannot match uppercase under a different collation. Adds regression coverage for illegal names, non-markdown entries, symlinks and executable modes on both the gate and the renderer. --- scripts/policy/check-release-fragments.sh | 61 ++++++++++++- scripts/release/render-release-fragments.sh | 57 +++++++++++-- test/scripts/changelog_pr_gate_test.go | 94 +++++++++++++++++++++ test/scripts/release_script_test.go | 67 +++++++++++++++ 4 files changed, 270 insertions(+), 9 deletions(-) diff --git a/scripts/policy/check-release-fragments.sh b/scripts/policy/check-release-fragments.sh index 2610f32b..95c24f3c 100755 --- a/scripts/policy/check-release-fragments.sh +++ b/scripts/policy/check-release-fragments.sh @@ -1,6 +1,10 @@ #!/bin/sh set -eu +# Fragment names are matched with ASCII ranges, so keep collation deterministic. +LC_ALL=C +export LC_ALL + ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd)" usage() { printf '%s\n' 'usage: check-release-fragments.sh BASE HEAD' >&2; } @@ -90,6 +94,59 @@ else fi fi -if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | grep -Eq '^\.changes/[a-z0-9][a-z0-9._-]*\.md$'; then - "$ROOT/scripts/release/render-release-fragments.sh" "$ROOT/.changes" >/dev/null +# Any change to a top-level `.changes/` entry other than README.md must be +# validated. Filtering this trigger by the legal fragment name pattern would let +# an illegally named or non-regular entry skip validation entirely, and it would +# then break the next unrelated PR that adds a legal fragment. +if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | + awk ' + $0 !~ /^\.changes\/[^\/]+$/ { next } + $0 == ".changes/README.md" { next } + { found = 1 } + END { exit !found } + '; then + git -C "$ROOT" ls-tree "$head" -- .changes/ >"$tmp_root/head-entries" + awk ' + { + mode = $1 + type = $2 + path = $0 + sub(/^[^\t]*\t/, "", path) + name = path + sub(/^.*\//, "", name) + if (path == ".changes/README.md") { + if (mode == "100644" && type == "blob") next + print path + next + } + if (path == ".changes/released") { + if (type == "tree") next + print path + next + } + if (mode != "100644" || type != "blob") { print path; next } + if (name !~ /^[a-z0-9][a-z0-9._-]*\.md$/) { print path; next } + } + ' "$tmp_root/head-entries" >"$tmp_root/invalid-entries" + if [ -s "$tmp_root/invalid-entries" ]; then + printf '%s\n' 'error: .changes/ accepts only README.md, released/, and release fragments named .md matching ^[a-z0-9][a-z0-9._-]*\.md$ stored as regular 100644 files' >&2 + sed 's/^/ /' "$tmp_root/invalid-entries" >&2 + exit 1 + fi + + # A release-seal branch is already held to the stricter rendered-notes + # comparison above, and its fragments have moved into the archive, so only an + # ordinary fragment change is re-rendered here. + if [ "$archive_changed" = false ]; then + head_changes="$tmp_root/head-changes" + mkdir -p "$head_changes" + awk '{ path = $0; sub(/^[^\t]*\t/, "", path); print path }' "$tmp_root/head-entries" | + while IFS= read -r path; do + case "$path" in + .changes/released) continue ;; + esac + git -C "$ROOT" show "$head:$path" >"$head_changes/${path#.changes/}" + done + "$ROOT/scripts/release/render-release-fragments.sh" "$head_changes" >/dev/null + fi fi diff --git a/scripts/release/render-release-fragments.sh b/scripts/release/render-release-fragments.sh index c6ed6796..1734a399 100755 --- a/scripts/release/render-release-fragments.sh +++ b/scripts/release/render-release-fragments.sh @@ -1,6 +1,10 @@ #!/bin/sh set -eu +# Fragment names are matched with ASCII ranges, so keep collation deterministic. +LC_ALL=C +export LC_ALL + CHANGES_DIR="${1:-.changes}" usage() { @@ -14,8 +18,51 @@ tmp_root="$(mktemp -d "${TMPDIR:-/tmp}/dws-release-fragments.XXXXXX")" cleanup() { rm -rf "$tmp_root"; } trap cleanup EXIT HUP INT TERM -find "$CHANGES_DIR" -mindepth 1 -maxdepth 1 -type f -name '*.md' ! -name 'README.md' -print | - LC_ALL=C sort >"$tmp_root/files" +# A fragment must be a regular file named ^[a-z0-9][a-z0-9._-]*\.md$. Anything +# else is rejected rather than skipped, so a symlink or an illegally named +# fragment can never be silently dropped from the rendered notes. +validate_fragment_name() { + name="$1" + path="$2" + case "$name" in + *.md) ;; + *) printf 'invalid release fragment filename: %s\n' "$path" >&2; return 1 ;; + esac + case "$name" in + [a-z0-9]*) ;; + *) printf 'invalid release fragment filename: %s\n' "$path" >&2; return 1 ;; + esac + case "$name" in + *[!a-z0-9._-]*) printf 'invalid release fragment filename: %s\n' "$path" >&2; return 1 ;; + esac +} + +find "$CHANGES_DIR" -mindepth 1 -maxdepth 1 -print | sort >"$tmp_root/entries" + +: >"$tmp_root/files" +while IFS= read -r entry; do + base="${entry##*/}" + if [ "$base" = 'README.md' ]; then + continue + fi + if [ -L "$entry" ]; then + printf 'release fragment must be a regular file, not a symbolic link: %s\n' "$entry" >&2 + exit 1 + fi + if [ -d "$entry" ]; then + if [ "$base" = 'released' ]; then + continue + fi + printf 'unexpected directory in release fragments directory: %s\n' "$entry" >&2 + exit 1 + fi + if [ ! -f "$entry" ]; then + printf 'release fragment must be a regular file: %s\n' "$entry" >&2 + exit 1 + fi + validate_fragment_name "$base" "$entry" + printf '%s\n' "$entry" >>"$tmp_root/files" +done <"$tmp_root/entries" [ -s "$tmp_root/files" ] || { printf '%s\n' 'no release fragments found; add .changes/.md before preparing a prerelease changelog' >&2 @@ -24,11 +71,7 @@ find "$CHANGES_DIR" -mindepth 1 -maxdepth 1 -type f -name '*.md' ! -name 'README validate_fragment() { fragment="$1" - base="$(basename "$fragment")" - case "$base" in - [a-z0-9]*.md) ;; - *) printf 'invalid release fragment filename: %s\n' "$fragment" >&2; return 1 ;; - esac + validate_fragment_name "${fragment##*/}" "$fragment" [ "$(sed -n '1p' "$fragment")" = '---' ] && [ "$(sed -n '3p' "$fragment")" = '---' ] || { diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index fb9a25a8..95fb4926 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -318,6 +318,100 @@ func TestReleaseFragmentPolicyRejectsInvalidActiveFragmentAndWrongArchiveVersion }) } +const changelogGateValidFragment = "---\ncategory: Added\n---\n\n- Chat reply mentions.\n" + +// A top-level `.changes/` entry that violates the naming rule or is not a +// regular 100644 blob must fail the gate. Selecting the trigger by the legal +// name pattern used to let these entries through untouched, which both bypassed +// validation here and broke the next PR that added a legal fragment. +func TestReleaseFragmentPolicyRejectsIllegalFragmentNamesAndModes(t *testing.T) { + tests := []struct { + name string + seed func(t *testing.T, root string) + wantPath string + }{ + { + name: "uppercase fragment name", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/Chat.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/Chat.md", + }, + { + name: "fragment name with a space", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/chat reply.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/chat reply.md", + }, + { + name: "fragment name starting with a dash", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/-chat.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/-chat.md", + }, + { + name: "non-markdown entry", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/notes.txt", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/notes.txt", + }, + { + name: "symlinked fragment", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/1234-chat.md", changelogGateValidFragment, 0o644) + if err := os.Symlink("1234-chat.md", filepath.Join(root, ".changes", "5678-alias.md")); err != nil { + t.Fatalf("Symlink release fragment: %v", err) + } + }, + wantPath: ".changes/5678-alias.md", + }, + { + name: "executable fragment", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/1234-chat.md", changelogGateValidFragment, 0o755) + }, + wantPath: ".changes/1234-chat.md", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + repo := newChangelogGateRepo(t) + test.seed(t, repo.root) + repo.commit(t, test.name) + + output, err := repo.runFragmentPolicy(t, repo.base, "HEAD") + if err == nil { + t.Fatalf("illegal release fragment entry passed:\n%s", output) + } + if !strings.Contains(output, ".changes/ accepts only") { + t.Fatalf("gate output missing the entry contract:\n%s", output) + } + if !strings.Contains(output, test.wantPath) { + t.Fatalf("gate output missing offending entry %q:\n%s", test.wantPath, output) + } + }) + } +} + +func TestReleaseFragmentPolicyAcceptsLegalFragmentAlongsideReadmeAndArchive(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n", 0o644) + changelogGateWrite(t, repo.root, ".changes/released/1.0.0/0001-seed.md", changelogGateValidFragment, 0o644) + repo.commit(t, "seed fragment directory") + base := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + + changelogGateWrite(t, repo.root, ".changes/1234-chat.md", changelogGateValidFragment, 0o644) + repo.commit(t, "add release fragment") + + if output, err := repo.runFragmentPolicy(t, base, "HEAD"); err != nil { + t.Fatalf("legal release fragment rejected: %v\noutput:\n%s", err, output) + } +} + func TestChangelogPRContentOnlyStillValidatesReleaseNotes(t *testing.T) { tests := []struct { name string diff --git a/test/scripts/release_script_test.go b/test/scripts/release_script_test.go index d3c969b7..5b372335 100644 --- a/test/scripts/release_script_test.go +++ b/test/scripts/release_script_test.go @@ -1715,6 +1715,73 @@ func TestReleasePrepareChangelogRejectsInvalidReleaseFragment(t *testing.T) { } } +const releaseValidFragment = "---\ncategory: Added\n---\n\n- **Chat reply mentions** (#1234) — supports selected member mentions.\n" + +// The renderer used to skip anything `find -type f -name '*.md'` did not match, +// so a symlinked or illegally named fragment was dropped from the notes without +// a word. Every unexpected top-level entry must fail the release instead. +func TestReleasePrepareChangelogRejectsUnsafeReleaseFragmentEntries(t *testing.T) { + tests := []struct { + name string + seed func(t *testing.T, changesDir string) + wantMsg string + }{ + { + name: "symlinked fragment", + seed: func(t *testing.T, changesDir string) { + mustWriteFile(t, filepath.Join(changesDir, "1234-chat.md"), []byte(releaseValidFragment), 0o644) + if err := os.Symlink("1234-chat.md", filepath.Join(changesDir, "5678-alias.md")); err != nil { + t.Fatalf("Symlink release fragment: %v", err) + } + }, + wantMsg: "not a symbolic link", + }, + { + name: "fragment name with a space", + seed: func(t *testing.T, changesDir string) { + mustWriteFile(t, filepath.Join(changesDir, "chat reply.md"), []byte(releaseValidFragment), 0o644) + }, + wantMsg: "invalid release fragment filename", + }, + { + name: "uppercase fragment name", + seed: func(t *testing.T, changesDir string) { + mustWriteFile(t, filepath.Join(changesDir, "Chat.md"), []byte(releaseValidFragment), 0o644) + }, + wantMsg: "invalid release fragment filename", + }, + { + name: "non-markdown entry", + seed: func(t *testing.T, changesDir string) { + mustWriteFile(t, filepath.Join(changesDir, "notes.txt"), []byte(releaseValidFragment), 0o644) + }, + wantMsg: "invalid release fragment filename", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + r := newReleaseTestRepo(t) + releaseCopyFile(t, r.lib, filepath.Join(r.root, "scripts", "release", "release-lib.sh"), 0o644) + releaseCopyFile(t, r.prepare, filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), 0o755) + releaseCopyFile(t, r.render, filepath.Join(r.root, "scripts", "release", "render-release-fragments.sh"), 0o755) + test.seed(t, filepath.Join(r.root, ".changes")) + r.commitAndPush(t, "add unsafe release fragment entry") + + cmd := exec.Command("sh", filepath.Join(r.root, "scripts", "release", "prepare-changelog.sh"), "prerelease", "v1.0.1-beta.1") + cmd.Dir = r.root + cmd.Env = append(os.Environ(), "DWS_RELEASE_DATE=2026-07-11") + output, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("unsafe release fragment entry was accepted:\n%s", output) + } + if !strings.Contains(string(output), test.wantMsg) { + t.Fatalf("renderer output missing %q:\n%s", test.wantMsg, output) + } + }) + } +} + func TestReleaseMirrorUsesChannelSpecificPointer(t *testing.T) { sourceRoot, err := filepath.Abs(filepath.Join("..", "..")) if err != nil { From e0dd8003782a4177f4334c5317c3bd735285e9dc Mon Sep 17 00:00:00 2001 From: chichuan Date: Tue, 11 Aug 2026 21:23:34 +0800 Subject: [PATCH 03/13] docs: state the release fragment filename and file-kind contract --- .changes/README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.changes/README.md b/.changes/README.md index 86c31701..9d747e3d 100644 --- a/.changes/README.md +++ b/.changes/README.md @@ -5,7 +5,10 @@ 并行 PR 争用同一文件。 文件名使用能唯一定位变更的短名，通常是 PR 号，例如 -`1234-chat-reply-mentions.md`。文件格式严格如下： +`1234-chat-reply-mentions.md`。文件名必须匹配 +`^[a-z0-9][a-z0-9._-]*\.md$`，且必须是普通文件，不能是符号链接。本目录顶层 +只接受 `README.md`、`released/` 和符合该规则的 fragment；其余条目会被 CI 直接 +拒绝而不是忽略，以免非法条目跳过校验后拖垮下一个 PR。文件格式严格如下： ```markdown --- From bdf3048773e5a83b476034231264481c8cdcee24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B9=8B=E6=A1=90?= <1241839690@qq.com> Date: Wed, 12 Aug 2026 11:29:21 +0800 Subject: [PATCH 04/13] feat(drive): add file comment commands --- internal/helpers/drive.go | 1 + internal/helpers/file_comment.go | 547 ++++++++++++++++++++++++++ internal/helpers/file_comment_test.go | 413 +++++++++++++++++++ 3 files changed, 961 insertions(+) create mode 100644 internal/helpers/file_comment.go create mode 100644 internal/helpers/file_comment_test.go diff --git a/internal/helpers/drive.go b/internal/helpers/drive.go index 752ab25d..f2fc9c35 100644 --- a/internal/helpers/drive.go +++ b/internal/helpers/drive.go @@ -3204,6 +3204,7 @@ func newDriveCommand() *cobra.Command { driveListCmd, driveListSpacesCmd, driveInfoCmd, + newDriveFileCommentCmd(), driveDownloadCmd, driveDownloadVersionCmd, driveMkdirCmd, diff --git a/internal/helpers/file_comment.go b/internal/helpers/file_comment.go new file mode 100644 index 00000000..a0e8fe91 --- /dev/null +++ b/internal/helpers/file_comment.go @@ -0,0 +1,547 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package helpers + +import ( + "context" + "encoding/json" + "fmt" + "strconv" + "strings" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/corecmd/contract" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/output" + "github.com/spf13/cobra" +) + +const ( + fileCommentServer = "doc-comment" + listFileCommentsTool = "list_file_comments" + createFileCommentTool = "create_file_comment" + fileCommentMaxPageSize = 200 + fileCommentMaxAutoPages = 10 + fileCommentMaxContentLength = 2099 +) + +type fileCommentPage struct { + nodeID string + total any + hasMore bool + nextCursor string + comments []map[string]any +} + +// newDriveFileCommentCmd follows the existing resource-first comment surface: +// doc comment, sheet comment, and drive comment. The public command belongs to +// Drive, while the implementation routes to the shared doc-comment MCP server. +func newDriveFileCommentCmd() *cobra.Command { + commentCmd := &cobra.Command{ + Use: "comment", + Short: "普通文件评论管理", + Long: "管理钉盘普通预览文件的评论：查询评论列表或创建全文纯文本评论。", + RunE: groupRunE, + } + + listCmd := NewLeafCommand(LeafSpec{ + Use: "list", + Short: "查询普通文件评论列表", + Long: `查询钉盘普通预览文件的评论。支持 dentryUuid、钉盘文件 URL，以及配合 +--space-id 使用的数字 dentry ID。支持的文件类型由服务端判定。 + +默认返回一页；--all 固定按每页 200 条自动翻页，最多 10 页。--scope 在 CLI +侧按服务端返回的统一 anchor 过滤；total 始终表示文件的全部有效评论数，count +表示本次输出且符合 scope 的评论数。`, + Example: ` dws drive comment list --node --format json + dws drive comment list --node --all --format json`, + Tool: listFileCommentsTool, + Flags: []LeafFlag{ + {Name: "node", Usage: "文件 ID (dentryUuid)、数字 dentry ID 或钉盘文件 URL", Required: true, Aliases: []string{"url", "id", "node-id", "file-id"}, Bind: "fileId", Trim: true}, + {Name: "space-id", Usage: "钉盘空间 ID；仅数字 dentry ID 必填", Bind: "spaceId", OmitEmpty: true, Trim: true}, + {Name: "limit", Usage: "每页评论数，范围 1-200", Kind: LeafInt, Default: "200", Aliases: []string{"page-size"}, Bind: "maxResults"}, + {Name: "cursor", Usage: "分页游标，取自上页 nextCursor", Bind: "nextToken", OmitEmpty: true, Trim: true}, + {Name: "all", Usage: "自动拉取全部评论，最多 10 页 / 2000 条", Kind: LeafBool, Bind: "all"}, + {Name: "scope", Usage: "评论范围: all(全部) / whole(全文) / partial(历史局部)", Default: "all", Bind: "scope", Trim: true, Enum: []string{"all", "whole", "partial"}}, + }, + Constraints: []LeafConstraint{ + {Kind: LeafMutuallyExclusive, Flags: []string{"all", "cursor"}, Description: "--all 与 --cursor 互斥"}, + {Kind: LeafMutuallyExclusive, Flags: []string{"all", "limit"}, Description: "--all 与显式 --limit/--page-size 互斥"}, + {Kind: "custom", Flags: []string{"limit"}, Description: "--limit/--page-size 必须在 1-200 之间"}, + {Kind: "custom", Flags: []string{"cursor"}, Description: "--cursor 必须是服务端返回的非负数字游标"}, + }, + Safety: contract.SafetySpec{ + Effect: "read", Risk: "low", + Confirmation: "not_required", Idempotency: "idempotent", + }, + Contract: LeafContract{ + Identity: contract.ToolIdentitySpec{ + ProductID: "drive", + Name: "list_file_comments", + CanonicalPath: "drive.list_file_comments", + CLIPath: "drive comment list", + PrimaryCLIPath: "drive comment list", + }, + Description: "查询钉盘普通预览文件评论，支持安全分页聚合和全文/局部范围过滤", + DryRun: &contract.DryRunSpec{PreviewKind: contract.DryRunPreviewRequest}, + Interface: &contract.InterfaceSpec{ + Mode: "composite", + Availability: "available", + Reason: "The CLI wraps doc-comment/list_file_comments with bounded auto-pagination, cursor anomaly guards, local anchor-scope filtering, and a stable output projection, so no single direct MCP interface represents the complete command contract.", + }, + Selection: contract.SelectionSpec{ + AgentSummary: "查询 PDF、DOCX、XLSX 等钉盘普通预览文件上的评论", + UseWhen: []string{ + "用户要查看普通文件上的全文评论或历史高亮/矩形评论时", + "需要从评论列表取得 commentId、作者、时间和统一 anchor，或用 --all 拉取完整列表时", + }, + AvoidWhen: []string{ + "在线文字文档评论使用 dws doc comment list", + "在线表格单元格评论使用 dws sheet comment list", + }, + Examples: []string{ + "dws drive comment list --node --format json", + "dws drive comment list --node --all --format json", + }, + }, + }, + Validate: validateFileCommentList, + Call: runFileCommentList, + }) + + createCmd := NewLeafCommand(LeafSpec{ + Use: "create", + Short: "创建普通文件全文评论", + Long: `在钉盘普通预览文件上创建一条全文纯文本评论。当前不支持 @人、通知选项或 +局部锚点；content 按服务端规则使用 UTF-16 长度计数，最多 2099。`, + Example: ` dws drive comment create --node --content "请补充最终结论" --format json`, + Tool: createFileCommentTool, + Flags: []LeafFlag{ + {Name: "node", Usage: "文件 ID (dentryUuid)、数字 dentry ID 或钉盘文件 URL", Required: true, Aliases: []string{"url", "id", "node-id", "file-id"}, Bind: "fileId", Trim: true}, + {Name: "space-id", Usage: "钉盘空间 ID；仅数字 dentry ID 必填", Bind: "spaceId", OmitEmpty: true, Trim: true}, + {Name: "content", Usage: "全文评论内容，纯文本且 UTF-16 长度不超过 2099", Required: true, Bind: "content"}, + }, + Constraints: []LeafConstraint{ + {Kind: "custom", Flags: []string{"content"}, Description: "--content 去除首尾空白后必须非空，且 UTF-16 长度不超过 2099"}, + }, + Safety: contract.SafetySpec{ + Effect: "write", Risk: "medium", + Confirmation: "user_required", Idempotency: "unknown", + }, + Contract: LeafContract{ + Identity: contract.ToolIdentitySpec{ + ProductID: "drive", + Name: "create_file_comment", + CanonicalPath: "drive.create_file_comment", + CLIPath: "drive comment create", + PrimaryCLIPath: "drive comment create", + }, + Description: "在钉盘普通预览文件上创建一条全文纯文本评论", + DryRun: &contract.DryRunSpec{PreviewKind: contract.DryRunPreviewRequest}, + Interface: &contract.InterfaceSpec{ + Mode: "composite", + Availability: "available", + Reason: "The CLI wraps doc-comment/create_file_comment with exact local content validation, runtime confirmation, and a stable flattened output projection, so no single direct MCP interface represents the complete command contract.", + }, + Selection: contract.SelectionSpec{ + AgentSummary: "在 PDF、DOCX、XLSX 等钉盘普通预览文件上创建全文纯文本评论", + UseWhen: []string{ + "用户明确要求在普通文件上留下不绑定具体位置的评论时", + }, + AvoidWhen: []string{ + "在线文字文档评论使用 dws doc comment create", + "在线表格单元格评论使用 dws sheet comment create；当前普通文件评论不支持 @人或局部锚点", + }, + Examples: []string{ + "dws drive comment create --node --content \"请补充最终结论\" --format json", + }, + }, + }, + Validate: validateFileCommentCreate, + Call: runFileCommentCreate, + }) + + commentCmd.AddCommand(listCmd, createCmd) + return commentCmd +} + +func validateFileCommentList(cmd *cobra.Command, _ []string) error { + limit, _ := cmd.Flags().GetInt("limit") + if cmd.Flags().Changed("page-size") { + limit, _ = cmd.Flags().GetInt("page-size") + } + if limit < 1 || limit > fileCommentMaxPageSize { + return &CLIError{ + Code: CodeInvalidParam, + Message: fmt.Sprintf("--limit/--page-size 必须在 1-%d 之间", fileCommentMaxPageSize), + } + } + if cursor, _ := cmd.Flags().GetString("cursor"); strings.TrimSpace(cursor) != "" { + cursor = strings.TrimSpace(cursor) + if !allASCIIDigits(cursor) { + return &CLIError{Code: CodeInvalidParam, Message: "--cursor 必须是服务端返回的非负数字游标"} + } + if _, err := strconv.ParseInt(cursor, 10, 64); err != nil { + return &CLIError{Code: CodeInvalidParam, Message: "--cursor 超出 64 位整数范围"} + } + } + return nil +} + +func validateFileCommentCreate(cmd *cobra.Command, _ []string) error { + content, _ := cmd.Flags().GetString("content") + if strings.TrimSpace(content) == "" { + return &CLIError{Code: CodeInvalidParam, Message: "--content 去除首尾空白后不能为空"} + } + length := fileCommentUTF16Length(content) + if length > fileCommentMaxContentLength { + return &CLIError{ + Code: CodeInputTooLarge, + Message: fmt.Sprintf("--content 最多 %d 个 UTF-16 代码单元，当前为 %d", fileCommentMaxContentLength, length), + } + } + return nil +} + +func fileCommentUTF16Length(value string) int { + length := 0 + for _, r := range value { + length++ + if r > 0xffff { + length++ + } + } + return length +} + +func allASCIIDigits(value string) bool { + if value == "" { + return false + } + for _, r := range value { + if r < '0' || r > '9' { + return false + } + } + return true +} + +func runFileCommentList(cmd *cobra.Command, tool string, args map[string]any) error { + fetchAll, _ := args["all"].(bool) + scope, _ := args["scope"].(string) + if scope == "" { + scope = "all" + } + request := fileCommentMCPArgs(args) + if fetchAll { + request["maxResults"] = fileCommentMaxPageSize + delete(request, "nextToken") + } + if deps != nil && deps.Caller != nil && deps.Caller.DryRun() { + return callMCPToolOnServer(fileCommentServer, tool, request) + } + + if !fetchAll { + page, err := fetchFileCommentPage(tool, request) + if err != nil { + return err + } + if err := validateFileCommentNextCursor(page, stringArg(request, "nextToken")); err != nil { + return err + } + page.comments = filterFileCommentsByScope(page.comments, scope) + return output.WriteCommandPayload(cmd, fileCommentListPayload(page, scope), output.FormatJSON) + } + + var aggregate fileCommentPage + aggregate.comments = make([]map[string]any, 0) + seenCursors := map[string]bool{} + currentCursor := "" + for pageNumber := 0; pageNumber < fileCommentMaxAutoPages; pageNumber++ { + if currentCursor == "" { + delete(request, "nextToken") + } else { + request["nextToken"] = currentCursor + } + page, err := fetchFileCommentPage(tool, request) + if err != nil { + return err + } + if pageNumber == 0 { + aggregate.nodeID = page.nodeID + aggregate.total = page.total + } + aggregate.comments = append(aggregate.comments, filterFileCommentsByScope(page.comments, scope)...) + if !page.hasMore { + aggregate.hasMore = false + aggregate.nextCursor = "" + return output.WriteCommandPayload(cmd, fileCommentListPayload(aggregate, scope), output.FormatJSON) + } + if err := validateFileCommentNextCursor(page, currentCursor); err != nil { + return err + } + if seenCursors[page.nextCursor] { + return fileCommentPaginationError("分页游标发生循环，结果可能不完整") + } + seenCursors[page.nextCursor] = true + currentCursor = page.nextCursor + } + return fileCommentPaginationError(fmt.Sprintf("自动翻页达到 %d 页上限但服务端仍返回 hasMore=true，结果可能不完整", fileCommentMaxAutoPages)) +} + +func runFileCommentCreate(cmd *cobra.Command, tool string, args map[string]any) error { + request := fileCommentMCPArgs(args) + if deps != nil && deps.Caller != nil && deps.Caller.DryRun() { + return callMCPToolOnServer(fileCommentServer, tool, request) + } + raw, err := callMCPToolReturnTextOnServer(context.Background(), fileCommentServer, tool, request) + if err != nil { + return err + } + payload, err := decodeFileCommentPayload(tool, raw) + if err != nil { + return err + } + nodeID, ok := nonEmptyStringField(payload, "fileId") + if !ok { + return invalidFileCommentResponse(tool, "缺少 fileId", nil) + } + comment, ok := payload["comment"].(map[string]any) + if !ok { + return invalidFileCommentResponse(tool, "缺少 comment 对象", nil) + } + if err := validateFileCommentItem(tool, "comment", comment); err != nil { + return err + } + out := map[string]any{"nodeId": nodeID} + for key, value := range projectFileComment(comment) { + out[key] = value + } + return output.WriteCommandPayload(cmd, out, output.FormatJSON) +} + +func fileCommentMCPArgs(args map[string]any) map[string]any { + out := make(map[string]any, len(args)) + for key, value := range args { + switch key { + case "all", "scope": + continue + default: + out[key] = value + } + } + return out +} + +func fetchFileCommentPage(tool string, request map[string]any) (fileCommentPage, error) { + raw, err := callMCPToolReturnTextOnServer(context.Background(), fileCommentServer, tool, request) + if err != nil { + return fileCommentPage{}, err + } + payload, err := decodeFileCommentPayload(tool, raw) + if err != nil { + return fileCommentPage{}, err + } + nodeID, ok := nonEmptyStringField(payload, "fileId") + if !ok { + return fileCommentPage{}, invalidFileCommentResponse(tool, "缺少 fileId", nil) + } + total, ok := payload["total"] + if !ok { + return fileCommentPage{}, invalidFileCommentResponse(tool, "缺少 total", nil) + } + hasMore, ok := payload["hasMore"].(bool) + if !ok { + return fileCommentPage{}, invalidFileCommentResponse(tool, "缺少布尔字段 hasMore", nil) + } + nextCursor := "" + if value, exists := payload["nextToken"]; exists && value != nil { + var stringValue bool + nextCursor, stringValue = value.(string) + if !stringValue { + return fileCommentPage{}, invalidFileCommentResponse(tool, "nextToken 不是字符串", nil) + } + } + rawItems, exists := payload["items"] + if !exists { + return fileCommentPage{}, invalidFileCommentResponse(tool, "缺少 items", nil) + } + items, ok := rawItems.([]any) + if rawItems == nil { + items = []any{} + ok = true + } + if !ok { + return fileCommentPage{}, invalidFileCommentResponse(tool, "items 不是数组", nil) + } + comments := make([]map[string]any, 0, len(items)) + for index, item := range items { + comment, ok := item.(map[string]any) + if !ok { + return fileCommentPage{}, invalidFileCommentResponse(tool, fmt.Sprintf("items[%d] 不是对象", index), nil) + } + if err := validateFileCommentItem(tool, fmt.Sprintf("items[%d]", index), comment); err != nil { + return fileCommentPage{}, err + } + comments = append(comments, projectFileComment(comment)) + } + return fileCommentPage{ + nodeID: nodeID, total: total, hasMore: hasMore, + nextCursor: strings.TrimSpace(nextCursor), comments: comments, + }, nil +} + +func decodeFileCommentPayload(tool, raw string) (map[string]any, error) { + if strings.TrimSpace(raw) == "" { + return nil, invalidFileCommentResponse(tool, "返回为空", nil) + } + var payload map[string]any + if err := json.Unmarshal([]byte(raw), &payload); err != nil { + return nil, invalidFileCommentResponse(tool, "返回不是有效 JSON", err) + } + for depth := 0; depth < 2; depth++ { + if _, ok := payload["fileId"]; ok { + break + } + var nested map[string]any + for _, key := range []string{"result", "data"} { + if value, ok := payload[key].(map[string]any); ok { + nested = value + break + } + } + if nested == nil { + break + } + payload = nested + } + return payload, nil +} + +func projectFileComment(comment map[string]any) map[string]any { + out := map[string]any{} + for _, key := range []string{"commentId", "parentCommentId", "content", "createdAt", "updatedAt", "options", "anchor"} { + if value, ok := comment[key]; ok { + out[key] = value + } + } + if value, ok := comment["commentCustomType"]; ok { + out["customType"] = value + } + creator := map[string]any{} + for source, target := range map[string]string{ + "creatorId": "userId", "creatorName": "name", "creatorAvatar": "avatar", + } { + if value, ok := comment[source]; ok { + creator[target] = value + } + } + if len(creator) > 0 { + out["creator"] = creator + } + return out +} + +func validateFileCommentItem(tool, path string, comment map[string]any) error { + if _, ok := nonEmptyStringField(comment, "commentId"); !ok { + return invalidFileCommentResponse(tool, path+" 缺少 commentId", nil) + } + if _, ok := comment["anchor"].(map[string]any); !ok { + return invalidFileCommentResponse(tool, path+" 缺少 anchor 对象", nil) + } + return nil +} + +func filterFileCommentsByScope(comments []map[string]any, scope string) []map[string]any { + if scope == "" || scope == "all" { + return comments + } + out := make([]map[string]any, 0, len(comments)) + for _, comment := range comments { + anchor, _ := comment["anchor"].(map[string]any) + commentScope, _ := anchor["scope"].(string) + if commentScope == scope { + out = append(out, comment) + } + } + return out +} + +func fileCommentListPayload(page fileCommentPage, scope string) map[string]any { + comments := page.comments + if comments == nil { + comments = make([]map[string]any, 0) + } + nextCursor := any(nil) + if page.nextCursor != "" { + nextCursor = page.nextCursor + } + return map[string]any{ + "nodeId": page.nodeID, + "total": page.total, + "count": len(page.comments), + "hasMore": page.hasMore, + "nextCursor": nextCursor, + "complete": !page.hasMore, + "scope": scope, + "comments": comments, + } +} + +func validateFileCommentNextCursor(page fileCommentPage, currentCursor string) error { + if !page.hasMore { + return nil + } + if page.nextCursor == "" { + return fileCommentPaginationError("服务端返回 hasMore=true 但 nextCursor 为空，结果可能不完整") + } + if !allASCIIDigits(page.nextCursor) { + return fileCommentPaginationError("服务端返回的 nextCursor 不是非负数字游标，结果可能不完整") + } + if _, err := strconv.ParseInt(page.nextCursor, 10, 64); err != nil { + return fileCommentPaginationError("服务端返回的 nextCursor 超出 64 位整数范围，结果可能不完整") + } + if page.nextCursor == currentCursor { + return fileCommentPaginationError("服务端分页游标未前进，结果可能不完整") + } + return nil +} + +func fileCommentPaginationError(message string) error { + return &CLIError{ + Code: CodeContentTruncated, + Message: message, + Suggestion: "请稍后重试，或去掉 --all 后使用服务端返回的 --cursor 分页读取", + Operation: fileCommentServer + "/" + listFileCommentsTool, + } +} + +func invalidFileCommentResponse(tool, reason string, cause error) error { + return &CLIError{ + Code: CodeMCPToolError, + Message: fmt.Sprintf("%s 返回结构异常：%s", tool, reason), + Suggestion: "请确认 doc-comment MCP 与当前 DWS 契约一致", + Operation: fileCommentServer + "/" + tool, + Cause: cause, + } +} + +func nonEmptyStringField(payload map[string]any, key string) (string, bool) { + value, ok := payload[key].(string) + value = strings.TrimSpace(value) + return value, ok && value != "" +} + +func stringArg(args map[string]any, key string) string { + value, _ := args[key].(string) + return strings.TrimSpace(value) +} diff --git a/internal/helpers/file_comment_test.go b/internal/helpers/file_comment_test.go new file mode 100644 index 00000000..1af29a06 --- /dev/null +++ b/internal/helpers/file_comment_test.go @@ -0,0 +1,413 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package helpers + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "reflect" + "strings" + "testing" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/corecmd/contractfinal" + apperrors "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/errors" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/pkg/edition" + "github.com/spf13/cobra" +) + +type fileCommentTestCall struct { + server string + tool string + args map[string]any +} + +type fileCommentTestCaller struct { + calls []fileCommentTestCall + responses []string + err error + dryRun bool +} + +func (c *fileCommentTestCaller) CallTool(_ context.Context, server, tool string, args map[string]any) (*edition.ToolResult, error) { + copied := make(map[string]any, len(args)) + for key, value := range args { + copied[key] = value + } + c.calls = append(c.calls, fileCommentTestCall{server: server, tool: tool, args: copied}) + if c.err != nil { + return nil, c.err + } + response := `{"result":{"fileId":"file-1","comment":{"commentId":"1","content":"ok","anchor":{"version":"v1","surface":"file","scope":"whole"}}}}` + if len(c.responses) > 0 { + response = c.responses[0] + c.responses = c.responses[1:] + } + return &edition.ToolResult{Content: []edition.ContentBlock{{Type: "text", Text: response}}}, nil +} + +func (c *fileCommentTestCaller) Format() string { return "json" } +func (c *fileCommentTestCaller) DryRun() bool { return c.dryRun } +func (c *fileCommentTestCaller) Fields() string { return "" } +func (c *fileCommentTestCaller) JQ() string { return "" } + +func executeFileCommentCommand(t *testing.T, caller *fileCommentTestCaller, stdin string, args ...string) ([]byte, error) { + t.Helper() + testseam.Protect(t, &deps) + InitDeps(caller) + var stdout bytes.Buffer + deps.Out.w = &stdout + deps.Out.errW = io.Discard + + root := &cobra.Command{Use: "dws", SilenceErrors: true, SilenceUsage: true} + root.SetOut(&stdout) + root.SetErr(io.Discard) + root.SetIn(strings.NewReader(stdin)) + root.PersistentFlags().Bool("yes", false, "") + root.PersistentFlags().Bool("dry-run", false, "") + root.PersistentFlags().String("format", "json", "") + drive := &cobra.Command{Use: "drive"} + drive.AddCommand(newDriveFileCommentCmd()) + root.AddCommand(drive) + var setOutput func(*cobra.Command) + setOutput = func(command *cobra.Command) { + command.SetOut(&stdout) + command.SetErr(io.Discard) + for _, child := range command.Commands() { + setOutput(child) + } + } + setOutput(root) + root.SetArgs(args) + err := root.Execute() + return stdout.Bytes(), err +} + +func decodeFileCommentTestOutput(t *testing.T, raw []byte) map[string]any { + t.Helper() + var payload map[string]any + if err := json.Unmarshal(raw, &payload); err != nil { + t.Fatalf("decode output %q: %v", raw, err) + } + return payload +} + +func TestDriveFileCommentListMapsFiltersAndProjects(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{`{ + "result": { + "fileId": "resolved-file", + "total": 2, + "count": 2, + "hasMore": false, + "nextToken": null, + "items": [ + { + "commentId": "101", + "parentCommentId": null, + "content": "whole", + "creatorId": "user-1", + "creatorName": "Alice", + "creatorAvatar": "https://avatar/1", + "createdAt": 1785920000000, + "updatedAt": 1785920000100, + "commentCustomType": "common", + "anchor": {"version":"v1","surface":"file","scope":"whole"} + }, + { + "commentId": "102", + "content": "partial", + "commentCustomType": "highlight", + "options": {"page":"1"}, + "anchor": {"version":"v1","surface":"file","scope":"partial","selector":{"kind":"legacy-highlight"}} + } + ] + } + }`}} + + out, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "list", + "--node", "https://alidocs.dingtalk.com/i/drive/file", + "--space-id", "123", "--limit", "20", "--scope", "whole", + ) + if err != nil { + t.Fatal(err) + } + if len(caller.calls) != 1 { + t.Fatalf("calls = %#v", caller.calls) + } + call := caller.calls[0] + if call.server != fileCommentServer || call.tool != listFileCommentsTool { + t.Fatalf("target = %s/%s", call.server, call.tool) + } + wantArgs := map[string]any{ + "fileId": "https://alidocs.dingtalk.com/i/drive/file", + "spaceId": "123", + "maxResults": 20, + } + if !reflect.DeepEqual(call.args, wantArgs) { + t.Fatalf("args = %#v, want %#v", call.args, wantArgs) + } + + payload := decodeFileCommentTestOutput(t, out) + if payload["nodeId"] != "resolved-file" || payload["total"] != float64(2) || + payload["count"] != float64(1) || payload["complete"] != true || payload["scope"] != "whole" { + t.Fatalf("list output = %#v", payload) + } + if payload["nextCursor"] != nil || payload["hasMore"] != false { + t.Fatalf("pagination output = %#v", payload) + } + comments := payload["comments"].([]any) + comment := comments[0].(map[string]any) + if comment["commentId"] != "101" || comment["customType"] != "common" { + t.Fatalf("comment projection = %#v", comment) + } + creator := comment["creator"].(map[string]any) + if creator["userId"] != "user-1" || creator["name"] != "Alice" || creator["avatar"] != "https://avatar/1" { + t.Fatalf("creator projection = %#v", creator) + } + if _, leaked := comment["creatorId"]; leaked { + t.Fatalf("raw creator fields leaked: %#v", comment) + } +} + +func TestDriveFileCommentListKeepsEmptyCommentsAsArray(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{`{ + "fileId":"file-1","total":0,"count":0,"hasMore":false,"nextToken":null,"items":[] + }`}} + + out, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "list", "--node", "file-1", + ) + if err != nil { + t.Fatal(err) + } + payload := decodeFileCommentTestOutput(t, out) + comments, ok := payload["comments"].([]any) + if !ok || len(comments) != 0 { + t.Fatalf("comments = %#v, want an empty JSON array", payload["comments"]) + } + if payload["total"] != float64(0) || payload["count"] != float64(0) || + payload["hasMore"] != false || payload["complete"] != true || payload["nextCursor"] != nil { + t.Fatalf("empty list output = %#v", payload) + } +} + +func TestDriveFileCommentListAllAggregatesPages(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{ + `{"fileId":"file-1","total":3,"count":2,"hasMore":true,"nextToken":"2","items":[ + {"commentId":"1","anchor":{"scope":"whole"}}, + {"commentId":"2","anchor":{"scope":"partial"}} + ]}`, + `{"fileId":"file-1","total":3,"count":1,"hasMore":false,"nextToken":null,"items":[ + {"commentId":"3","anchor":{"scope":"partial"}} + ]}`, + }} + + out, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "list", "--node", "file-1", "--all", "--scope", "partial", + ) + if err != nil { + t.Fatal(err) + } + if len(caller.calls) != 2 { + t.Fatalf("calls = %#v", caller.calls) + } + if caller.calls[0].args["maxResults"] != fileCommentMaxPageSize { + t.Fatalf("first args = %#v", caller.calls[0].args) + } + if _, ok := caller.calls[0].args["nextToken"]; ok { + t.Fatalf("first page unexpectedly has cursor: %#v", caller.calls[0].args) + } + if caller.calls[1].args["nextToken"] != "2" { + t.Fatalf("second args = %#v", caller.calls[1].args) + } + for _, call := range caller.calls { + if _, ok := call.args["all"]; ok { + t.Fatalf("local --all leaked to MCP: %#v", call.args) + } + if _, ok := call.args["scope"]; ok { + t.Fatalf("local --scope leaked to MCP: %#v", call.args) + } + } + + payload := decodeFileCommentTestOutput(t, out) + if payload["total"] != float64(3) || payload["count"] != float64(2) || + payload["hasMore"] != false || payload["complete"] != true || payload["nextCursor"] != nil { + t.Fatalf("aggregate output = %#v", payload) + } + comments := payload["comments"].([]any) + if comments[0].(map[string]any)["commentId"] != "2" || comments[1].(map[string]any)["commentId"] != "3" { + t.Fatalf("scope-filtered comments = %#v", comments) + } +} + +func TestDriveFileCommentListRejectsPaginationAnomalies(t *testing.T) { + tests := []struct { + name string + args []string + response string + message string + }{ + { + name: "missing cursor", + args: []string{"drive", "comment", "list", "--node", "file-1", "--all"}, + response: `{"fileId":"file-1","total":1,"count":1,"hasMore":true,"nextToken":null,"items":[]}`, + message: "nextCursor 为空", + }, + { + name: "stalled cursor", + args: []string{"drive", "comment", "list", "--node", "file-1", "--cursor", "2"}, + response: `{"fileId":"file-1","total":2,"count":1,"hasMore":true,"nextToken":"2","items":[]}`, + message: "游标未前进", + }, + { + name: "invalid server cursor", + args: []string{"drive", "comment", "list", "--node", "file-1"}, + response: `{"fileId":"file-1","total":2,"count":1,"hasMore":true,"nextToken":"next","items":[]}`, + message: "不是非负数字游标", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{tt.response}} + _, err := executeFileCommentCommand(t, caller, "", tt.args...) + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeContentTruncated || !strings.Contains(cliErr.Message, tt.message) { + t.Fatalf("error = %#v", err) + } + if len(caller.calls) != 1 { + t.Fatalf("calls = %#v", caller.calls) + } + }) + } +} + +func TestDriveFileCommentListValidatesLocalParameters(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {"all with cursor", []string{"--all", "--cursor", "1"}}, + {"all with limit", []string{"--all", "--limit", "20"}}, + {"zero limit", []string{"--limit", "0"}}, + {"large limit", []string{"--page-size", "201"}}, + {"nonnumeric cursor", []string{"--cursor", "next"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{} + args := []string{"drive", "comment", "list", "--node", "file-1"} + args = append(args, tt.args...) + if _, err := executeFileCommentCommand(t, caller, "", args...); err == nil { + t.Fatal("invalid arguments unexpectedly succeeded") + } + if len(caller.calls) != 0 { + t.Fatalf("invalid arguments reached MCP: %#v", caller.calls) + } + }) + } +} + +func TestDriveFileCommentCreateValidatesMapsAndProjects(t *testing.T) { + validContent := strings.Repeat("a", fileCommentMaxContentLength) + caller := &fileCommentTestCaller{responses: []string{`{"result":{"fileId":"resolved-file","comment":{ + "commentId":"912345","parentCommentId":null,"content":"created","creatorId":"user-1", + "createdAt":1785920000000,"commentCustomType":"common", + "anchor":{"version":"v1","surface":"file","scope":"whole","selector":null} + }}}`}} + out, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "create", "--node", "file-1", "--space-id", "123", + "--content", validContent, "--yes", + ) + if err != nil { + t.Fatal(err) + } + if len(caller.calls) != 1 || caller.calls[0].server != fileCommentServer || caller.calls[0].tool != createFileCommentTool { + t.Fatalf("calls = %#v", caller.calls) + } + wantArgs := map[string]any{"fileId": "file-1", "spaceId": "123", "content": validContent} + if !reflect.DeepEqual(caller.calls[0].args, wantArgs) { + t.Fatalf("args = %#v, want %#v", caller.calls[0].args, wantArgs) + } + payload := decodeFileCommentTestOutput(t, out) + if payload["nodeId"] != "resolved-file" || payload["commentId"] != "912345" || + payload["content"] != "created" || payload["customType"] != "common" { + t.Fatalf("create output = %#v", payload) + } + if _, nested := payload["comment"]; nested { + t.Fatalf("create output was not flattened: %#v", payload) + } + + for _, tc := range []struct { + name string + content string + }{ + {"blank", " \t"}, + {"ascii over limit", strings.Repeat("a", fileCommentMaxContentLength+1)}, + {"utf16 over limit", strings.Repeat("😀", 1050)}, + } { + t.Run(tc.name, func(t *testing.T) { + invalidCaller := &fileCommentTestCaller{} + _, err := executeFileCommentCommand(t, invalidCaller, "", + "drive", "comment", "create", "--node", "file-1", "--content", tc.content, "--yes", + ) + if err == nil { + t.Fatal("invalid content unexpectedly succeeded") + } + if len(invalidCaller.calls) != 0 { + t.Fatalf("invalid content reached MCP: %#v", invalidCaller.calls) + } + }) + } +} + +func TestDriveFileCommentCreatePublishesAndEnforcesConfirmation(t *testing.T) { + group := newDriveFileCommentCmd() + create, remaining, err := group.Find([]string{"create"}) + if err != nil || len(remaining) != 0 { + t.Fatalf("find create: remaining=%v err=%v", remaining, err) + } + final, ok := contractfinal.RuntimeContractFinal(create) + if !ok || final.Safety == nil { + t.Fatal("create command is missing ContractFinal Safety") + } + if safety := *final.Safety; safety.Effect != "write" || safety.Risk != "medium" || + safety.Confirmation != "user_required" || safety.Idempotency != "unknown" { + t.Fatalf("create safety = %#v", safety) + } + + caller := &fileCommentTestCaller{} + _, err = executeFileCommentCommand(t, caller, "", + "drive", "comment", "create", "--node", "file-1", "--content", "需要确认", + ) + var appErr *apperrors.Error + if !errors.As(err, &appErr) || appErr.Reason != "confirmation_required" { + t.Fatalf("closed-stdin error = %#v", err) + } + if len(caller.calls) != 0 { + t.Fatalf("unconfirmed create reached MCP: %#v", caller.calls) + } +} + +func TestDriveFileCommentRejectsMalformedResponseInsteadOfReturningEmpty(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{`{"result":{"fileId":"file-1","total":0,"hasMore":false}}`}} + _, err := executeFileCommentCommand(t, caller, "", "drive", "comment", "list", "--node", "file-1") + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeMCPToolError || !strings.Contains(cliErr.Message, "缺少 items") { + t.Fatalf("error = %#v", err) + } +} From 4243676739892eb69c7b603af572ac66707f6ac9 Mon Sep 17 00:00:00 2001 From: chichuan Date: Wed, 12 Aug 2026 12:00:42 +0800 Subject: [PATCH 05/13] fix: trigger release fragment tree validation on nested .changes paths Git records no diff entry for a directory itself, so adding .changes/foo/bar.md only surfaced the nested path, which the single-level trigger regex skipped. The entry validation and the renderer were both bypassed, letting a nested directory reach main and break every later fragment render with 'unexpected directory'. Trigger the top-level tree validation on any .changes change outside .changes/released/ (which keeps its own immutability and release-seal checks), and assert .changes itself is still a tree so replacing it with a blob or symlink cannot empty the child listing unnoticed. Re-rendering stays keyed on fragment changes so a README-only edit does not fail on an empty fragment set. --- .changes/README.md | 6 +- scripts/policy/check-release-fragments.sh | 43 +++++++- test/scripts/changelog_pr_gate_test.go | 128 ++++++++++++++++++++++ test/scripts/release_script_test.go | 10 ++ 4 files changed, 179 insertions(+), 8 deletions(-) diff --git a/.changes/README.md b/.changes/README.md index 9d747e3d..79b5868f 100644 --- a/.changes/README.md +++ b/.changes/README.md @@ -7,8 +7,10 @@ 文件名使用能唯一定位变更的短名，通常是 PR 号，例如 `1234-chat-reply-mentions.md`。文件名必须匹配 `^[a-z0-9][a-z0-9._-]*\.md$`，且必须是普通文件，不能是符号链接。本目录顶层 -只接受 `README.md`、`released/` 和符合该规则的 fragment；其余条目会被 CI 直接 -拒绝而不是忽略，以免非法条目跳过校验后拖垮下一个 PR。文件格式严格如下： +只接受 `README.md`、`released/` 和符合该规则的 fragment：fragment 一律平铺在 +顶层，不接受任何其它子目录，本目录自身也不能被替换成文件或符号链接。其余条目 +会被 CI 直接拒绝而不是忽略，以免非法条目跳过校验后拖垮下一个 PR。文件格式 +严格如下： ```markdown --- diff --git a/scripts/policy/check-release-fragments.sh b/scripts/policy/check-release-fragments.sh index 95c24f3c..f10e3d32 100755 --- a/scripts/policy/check-release-fragments.sh +++ b/scripts/policy/check-release-fragments.sh @@ -94,17 +94,48 @@ else fi fi -# Any change to a top-level `.changes/` entry other than README.md must be -# validated. Filtering this trigger by the legal fragment name pattern would let -# an illegally named or non-regular entry skip validation entirely, and it would -# then break the next unrelated PR that adds a legal fragment. +# `.changes/released/**` carries its own immutability and release-seal checks +# above, so every other `.changes` change must revalidate the top-level tree. +# This trigger must not be narrowed to single-level paths or to the legal +# fragment name pattern: git records no diff entry for a directory itself, so +# adding `.changes/foo/bar.md` only ever shows the nested path, and an illegally +# named or non-regular entry only ever shows its own path. Either one would skip +# validation here and then break the next unrelated PR that adds a legal +# fragment. +changes_tree_changed=false if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | awk ' - $0 !~ /^\.changes\/[^\/]+$/ { next } + $0 ~ /^\.changes\/released\// { next } + { found = 1 } + END { exit !found } + '; then + changes_tree_changed=true +fi + +# Re-rendering is driven by fragment changes only. `.changes/README.md` is the +# contributor contract rather than release content, and it may be edited while no +# fragment is pending, which the renderer would reject as an empty fragment set. +fragment_changed=false +if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | + awk ' + $0 ~ /^\.changes\/released\// { next } $0 == ".changes/README.md" { next } { found = 1 } END { exit !found } '; then + fragment_changed=true +fi + +if [ "$changes_tree_changed" = true ]; then + # `.changes` itself must stay a directory: replacing it with a blob or a + # symlink leaves the child listing below empty, which would report no invalid + # entries while silently discarding every fragment. + changes_root_type="$(git -C "$ROOT" ls-tree "$head" -- .changes | awk 'NR == 1 { print $2 }')" + if [ "$changes_root_type" != tree ]; then + printf '%s\n' 'error: .changes must remain a directory holding README.md, released/, and release fragments' >&2 + exit 1 + fi + git -C "$ROOT" ls-tree "$head" -- .changes/ >"$tmp_root/head-entries" awk ' { @@ -137,7 +168,7 @@ if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .chang # A release-seal branch is already held to the stricter rendered-notes # comparison above, and its fragments have moved into the archive, so only an # ordinary fragment change is re-rendered here. - if [ "$archive_changed" = false ]; then + if [ "$fragment_changed" = true ] && [ "$archive_changed" = false ]; then head_changes="$tmp_root/head-changes" mkdir -p "$head_changes" awk '{ path = $0; sub(/^[^\t]*\t/, "", path); print path }' "$tmp_root/head-entries" | diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 95fb4926..25fc18c1 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -412,6 +412,134 @@ func TestReleaseFragmentPolicyAcceptsLegalFragmentAlongsideReadmeAndArchive(t *t } } +// Git records no diff entry for a directory itself, so adding +// `.changes/foo/bar.md` only ever surfaces the nested path. Triggering the +// top-level validation on single-level paths let such a directory reach main +// untouched and then broke every later fragment render with +// `unexpected directory`. +func TestReleaseFragmentPolicyRejectsNestedChangesEntries(t *testing.T) { + tests := []struct { + name string + seed func(t *testing.T, root string) + wantPath string + }{ + { + name: "nested directory only", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/foo/bar.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/foo", + }, + { + name: "nested directory alongside a legal fragment", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/1234-chat.md", changelogGateValidFragment, 0o644) + changelogGateWrite(t, root, ".changes/foo/bar.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/foo", + }, + { + name: "nested directory shadowing the archive name", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/released.d/bar.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/released.d", + }, + { + name: "deeply nested directory", + seed: func(t *testing.T, root string) { + changelogGateWrite(t, root, ".changes/a/b/c/note.md", changelogGateValidFragment, 0o644) + }, + wantPath: ".changes/a", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + repo := newChangelogGateRepo(t) + test.seed(t, repo.root) + repo.commit(t, test.name) + + output, err := repo.runFragmentPolicy(t, repo.base, "HEAD") + if err == nil { + t.Fatalf("nested .changes entry passed:\n%s", output) + } + if !strings.Contains(output, ".changes/ accepts only") { + t.Fatalf("gate output missing the entry contract:\n%s", output) + } + if !strings.Contains(output, test.wantPath) { + t.Fatalf("gate output missing offending entry %q:\n%s", test.wantPath, output) + } + }) + } +} + +// Replacing `.changes` itself empties the child listing, so scanning entries +// alone would report nothing invalid while the whole fragment directory +// disappears. Seeded without an active fragment so the archival-move guard +// cannot mask the directory contract being asserted here. +func TestReleaseFragmentPolicyRejectsReplacedChangesDirectory(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n", 0o644) + repo.commit(t, "seed fragment directory") + base := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + + if err := os.RemoveAll(filepath.Join(repo.root, ".changes")); err != nil { + t.Fatalf("RemoveAll(.changes) error = %v", err) + } + if err := os.Symlink("docs", filepath.Join(repo.root, ".changes")); err != nil { + t.Fatalf("Symlink(.changes) error = %v", err) + } + repo.commit(t, "replace .changes with a symlink") + + output, err := repo.runFragmentPolicy(t, base, "HEAD") + if err == nil { + t.Fatalf("replaced .changes directory passed:\n%s", output) + } + if !strings.Contains(output, ".changes must remain a directory") { + t.Fatalf("gate output missing the directory contract:\n%s", output) + } +} + +// `.changes/README.md` is the contributor contract, not release content, so a +// documentation-only edit must pass even though no fragment is pending — the +// renderer rejects an empty fragment set. +func TestReleaseFragmentPolicyAcceptsReadmeOnlyChangeWithoutPendingFragments(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n", 0o644) + repo.commit(t, "seed fragment directory") + base := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n\nName each fragment `.md`.\n", 0o644) + repo.commit(t, "document the fragment naming rule") + + if output, err := repo.runFragmentPolicy(t, base, "HEAD"); err != nil { + t.Fatalf("README-only change rejected: %v\noutput:\n%s", err, output) + } +} + +// A README-only edit still revalidates the tree, so pollution that somehow +// reached the branch is reported by the PR that touches `.changes/` rather than +// deferred to whichever unrelated PR next adds a fragment. +func TestReleaseFragmentPolicyReadmeOnlyChangeStillValidatesTree(t *testing.T) { + repo := newChangelogGateRepo(t) + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n", 0o644) + changelogGateWrite(t, repo.root, ".changes/Foo.md", changelogGateValidFragment, 0o644) + repo.commit(t, "seed polluted fragment directory") + base := strings.TrimSpace(changelogGateGit(t, repo.root, "rev-parse", "HEAD")) + + changelogGateWrite(t, repo.root, ".changes/README.md", "# Release fragments\n\nUpdated contract.\n", 0o644) + repo.commit(t, "edit only the fragment readme") + + output, err := repo.runFragmentPolicy(t, base, "HEAD") + if err == nil { + t.Fatalf("README-only change skipped tree validation:\n%s", output) + } + if !strings.Contains(output, ".changes/Foo.md") { + t.Fatalf("gate output missing the pre-existing illegal entry:\n%s", output) + } +} + func TestChangelogPRContentOnlyStillValidatesReleaseNotes(t *testing.T) { tests := []struct { name string diff --git a/test/scripts/release_script_test.go b/test/scripts/release_script_test.go index 5b372335..6ad03b77 100644 --- a/test/scripts/release_script_test.go +++ b/test/scripts/release_script_test.go @@ -1757,6 +1757,16 @@ func TestReleasePrepareChangelogRejectsUnsafeReleaseFragmentEntries(t *testing.T }, wantMsg: "invalid release fragment filename", }, + { + // The PR gate is what keeps a nested directory out of `.changes/`; + // this covers the release-side backstop that reports it instead of + // rendering notes from an unexpected tree shape. + name: "nested fragment directory", + seed: func(t *testing.T, changesDir string) { + mustWriteFile(t, filepath.Join(changesDir, "foo", "bar.md"), []byte(releaseValidFragment), 0o644) + }, + wantMsg: "unexpected directory in release fragments directory", + }, } for _, test := range tests { From 12435e6e54fe1f8b7835e9d779c3904d739136d9 Mon Sep 17 00:00:00 2001 From: chichuan Date: Wed, 12 Aug 2026 12:04:43 +0800 Subject: [PATCH 06/13] refactor: stage the .changes diff once for both fragment triggers Both trigger predicates ran the same git diff, which the script already avoids elsewhere by staging --name-status into $tmp_root/status. Write the path list once and let each awk predicate read it, matching that idiom. --- scripts/policy/check-release-fragments.sh | 33 ++++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/scripts/policy/check-release-fragments.sh b/scripts/policy/check-release-fragments.sh index f10e3d32..ec88f951 100755 --- a/scripts/policy/check-release-fragments.sh +++ b/scripts/policy/check-release-fragments.sh @@ -102,13 +102,14 @@ fi # named or non-regular entry only ever shows its own path. Either one would skip # validation here and then break the next unrelated PR that adds a legal # fragment. +git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes >"$tmp_root/changes-paths" + changes_tree_changed=false -if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | - awk ' - $0 ~ /^\.changes\/released\// { next } - { found = 1 } - END { exit !found } - '; then +if awk ' + $0 ~ /^\.changes\/released\// { next } + { found = 1 } + END { exit !found } +' "$tmp_root/changes-paths"; then changes_tree_changed=true fi @@ -116,13 +117,12 @@ fi # contributor contract rather than release content, and it may be edited while no # fragment is pending, which the renderer would reject as an empty fragment set. fragment_changed=false -if git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes | - awk ' - $0 ~ /^\.changes\/released\// { next } - $0 == ".changes/README.md" { next } - { found = 1 } - END { exit !found } - '; then +if awk ' + $0 ~ /^\.changes\/released\// { next } + $0 == ".changes/README.md" { next } + { found = 1 } + END { exit !found } +' "$tmp_root/changes-paths"; then fragment_changed=true fi @@ -165,9 +165,10 @@ if [ "$changes_tree_changed" = true ]; then exit 1 fi - # A release-seal branch is already held to the stricter rendered-notes - # comparison above, and its fragments have moved into the archive, so only an - # ordinary fragment change is re-rendered here. + # Only an ordinary fragment change is re-rendered here: a release-seal branch + # is already held to the stricter rendered-notes comparison above and its + # fragments have moved into the archive, and a README-only edit carries no + # release content to render. if [ "$fragment_changed" = true ] && [ "$archive_changed" = false ]; then head_changes="$tmp_root/head-changes" mkdir -p "$head_changes" From 5c68e4d9ccb89781ffb6d2677524230516b6e07d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Wed, 12 Aug 2026 12:32:53 +0800 Subject: [PATCH 07/13] fix(skill): avoid duplicate Agent skill roots --- README.md | 4 +- README_zh.md | 6 +- build/npm/install.js | 66 +++++++++++++++ docs/rfc-skill-installation-and-upgrade.md | 3 + internal/app/root.go | 5 ++ internal/app/skill_setup.go | 95 +++++++++++++++++++-- internal/app/skill_setup_test.go | 18 ++++ internal/app/skill_startup_repair.go | 64 ++++++++++++++ internal/app/skill_startup_repair_test.go | 90 ++++++++++++++++++++ internal/upgrade/coverage_edges_test.go | 2 +- internal/upgrade/paths.go | 82 +++++++++++++++--- internal/upgrade/paths_multi_test.go | 64 +++++++++++--- scripts/install-skills.sh | 54 ++++++++++++ scripts/install.ps1 | 46 ++++++++++ scripts/install.sh | 55 ++++++++++++ test/scripts/install_js_smoke.mjs | 32 +++++++ test/scripts/install_script_test.go | 99 ++++++++++++++++++++++ 17 files changed, 747 insertions(+), 38 deletions(-) create mode 100644 internal/app/skill_startup_repair.go create mode 100644 internal/app/skill_startup_repair_test.go diff --git a/README.md b/README.md index 14f6b11a..e55d3104 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ The verifier uses isolated directories and does not replace the `dws` on the cur The upgrade process follows a two-phase atomic flow to ensure consistency: 1. **Prepare** — downloads the platform-specific binary and skill packages to a temporary directory, verifies SHA256 checksums, and extracts/validates all files. If any step fails, the upgrade aborts without modifying the existing installation. -2. **Apply** — only after all preparations succeed, the binary is replaced and skill packages are installed to all detected agent directories (`~/.agents/skills/dws`, `~/.claude/skills/dws`, `~/.cursor/skills/dws`, etc.). +2. **Apply** — only after all preparations succeed, the binary is replaced and skills are flattened into detected agent-specific roots (for example `~/.codex/skills/dingtalk-chat`). `~/.agents/skills` is used only when no specific Agent is detected; once a specific root is active, older DWS-managed generic copies are backed up and retired so the same Skill is not discovered twice. A backup of the current version is automatically created before each upgrade. Use `dws upgrade --rollback` to restore the previous version if needed. @@ -405,7 +405,7 @@ After installing, AI tools like Claude Code / Cursor can operate DingTalk direct curl -fsSL https://raw.githubusercontent.com/DingTalk-Real-AI/dingtalk-workspace-cli/main/scripts/install-skills.sh | sh ``` -> `install.sh` installs under `$HOME/.agents/skills/` (global; multi layout is per-product siblings, mono is the `dws/` subdirectory); `install-skills.sh` installs under `./.agents/skills/` (current project). +> Installers prefer detected agent-specific roots such as `$HOME/.codex/skills/`. They use `.agents/skills/` only as the generic fallback when no specific Agent is detected; multi layout is per-product siblings, while mono uses the `dws/` subdirectory. > > China users: prefix `DWS_GITEE_REPO` to use the Gitee mirror — see [China mirror](#china-mirror). diff --git a/README_zh.md b/README_zh.md index dcda0671..c82a9228 100644 --- a/README_zh.md +++ b/README_zh.md @@ -207,7 +207,7 @@ bash verify-all-channels.sh 升级过程采用两阶段原子流程，确保一致性： 1. **准备阶段** — 将平台对应的二进制文件和技能包下载到临时目录，校验 SHA256 校验和，解压并验证所有文件。任何步骤失败则立即中止，不会修改现有安装。 -2. **执行阶段** — 仅在所有准备工作成功后，替换二进制文件并将技能包安装到所有已检测到的 Agent 目录（`~/.agents/skills/dws`、`~/.claude/skills/dws`、`~/.cursor/skills/dws` 等）。 +2. **执行阶段** — 仅在所有准备工作成功后，替换二进制文件并将技能包平铺到已检测到的具体 Agent 目录（例如 `~/.codex/skills/dingtalk-chat`、`~/.claude/skills/dingtalk-chat`）。只有未检测到具体 Agent 时才使用 `~/.agents/skills`；检测到具体 Agent 后会备份迁走旧的 DWS 通用副本，避免同一 Skill 被重复发现。 每次升级前自动备份当前版本，可通过 `dws upgrade --rollback` 随时回滚。 @@ -399,7 +399,7 @@ Schema 生成的叶子 safety/参数/选型文案由 Go 中的 ProductDecl / Con curl -fsSL https://raw.githubusercontent.com/DingTalk-Real-AI/dingtalk-workspace-cli/main/scripts/install-skills.sh | sh ``` -> `install.sh` 安装到 `$HOME/.agents/skills/`（全局，multi 为按产品平铺，mono 为 `dws/` 子目录）；`install-skills.sh` 安装到 `./.agents/skills/`（当前项目）。 +> 安装器优先使用检测到的具体 Agent 根目录（如 `$HOME/.codex/skills/`）；仅在未检测到具体 Agent 时回退到 `.agents/skills/`。multi 为按产品平铺，mono 为 `dws/` 子目录。 > > 国内用户加 `DWS_GITEE_REPO` 走 Gitee 镜像，见 [国内加速安装](#国内加速安装)。 @@ -427,7 +427,7 @@ DWS_SKILL_SOURCE=/path/to/skills dws skill setup --mode multi | 参数 | 取值 | 说明 | |------|------|------| | `--mode` | `mono` \| `multi` | skill 布局，不指定则交互式询问 | -| `--target` | `all` \| `claude` \| `cursor` \| `codex` \| `opencode` \| `qoder` | 安装目标，`all` 表示铺到所有检测到的 Agent home | +| `--target` | `all` \| `claude` \| `cursor` \| `codex` \| `opencode` \| `qoder` | 安装目标；`all` 表示铺到检测到的具体 Agent home，仅在未检测到具体 Agent 时回退到 `~/.agents/skills` | | `--source` | 路径 | 本地源目录（覆盖内置 skills） | | `--yes` | — | 仅供脚本使用：跳过确认提示。删除操作仍会先备份到 `~/.dws/skill-backups/` | diff --git a/build/npm/install.js b/build/npm/install.js index 0c7bda6e..364982c0 100755 --- a/build/npm/install.js +++ b/build/npm/install.js @@ -233,6 +233,10 @@ function installSkillsToHomes(skillRoot) { let attempted = 0; let failed = 0; + const specificAgentDirs = AGENT_DIRS.slice(1).filter((agentDir) => + fs.existsSync(path.dirname(path.join(homeDir, agentDir))), + ); + const installToBase = (baseDir) => { const victims = [path.join(baseDir, "dws")]; if (fs.existsSync(baseDir)) { @@ -252,6 +256,9 @@ function installSkillsToHomes(skillRoot) { }; AGENT_DIRS.forEach((agentDir, index) => { + if (index === 0 && specificAgentDirs.length > 0) { + return; + } const baseDir = path.join(homeDir, agentDir); const parentGate = path.dirname(baseDir); if (index > 0 && !fs.existsSync(parentGate)) { @@ -265,6 +272,15 @@ function installSkillsToHomes(skillRoot) { } }); + if (specificAgentDirs.length > 0 && installed > 0) { + try { + retireGenericSkillRoot(homeDir, managedNames); + } catch (err) { + console.warn(`⚠️ 通用 Skill 副本迁移失败: ${err.message}`); + failed += 1; + } + } + if (attempted === 0) { if (installToBase(path.join(homeDir, ".agents", "skills"))) { installed += 1; @@ -329,6 +345,40 @@ function isManagedMultiSkillDir(dir, managedNames) { return LEGACY_OFFICIAL_MULTI_SKILLS.has(name) || managedNames.has(name); } +function retireGenericSkillRoot(homeDir, managedNames) { + const baseDir = path.join(homeDir, ".agents", "skills"); + const victims = [path.join(baseDir, "dws")]; + if (fs.existsSync(baseDir)) { + for (const entry of fs.readdirSync(baseDir, { withFileTypes: true })) { + if (entry.isDirectory() && isManagedMultiSkillDir(path.join(baseDir, entry.name), managedNames)) { + victims.push(path.join(baseDir, entry.name)); + } + } + } + const backups = []; + try { + for (const victim of victims) { + if (!backupAndRemoveSkillDir(homeDir, victim, backups)) { + throw new Error(`failed to back up Skill directory ${victim}`); + } + } + } catch (err) { + const restoreErrors = []; + for (let i = backups.length - 1; i >= 0; i -= 1) { + try { + fs.mkdirSync(path.dirname(backups[i].original), { recursive: true }); + fs.renameSync(backups[i].backup, backups[i].original); + } catch (restoreErr) { + restoreErrors.push(`${backups[i].original}: ${restoreErr.message}`); + } + } + if (restoreErrors.length > 0) { + throw new Error(`${err.message}; generic-root rollback failed: ${restoreErrors.join("; ")}`); + } + throw err; + } +} + function skillDirectoryDigest(dir) { const files = []; const visit = (current, prefix) => { @@ -577,6 +627,10 @@ function installMultiSkillsToHomes(multiRoot) { let attempted = 0; let failed = 0; + const specificAgentDirs = AGENT_DIRS.slice(1).filter((agentDir) => + fs.existsSync(path.dirname(path.join(homeDir, agentDir))), + ); + const installToBase = (baseDir) => { fs.mkdirSync(baseDir, { recursive: true }); const victims = [path.join(baseDir, "dws")]; @@ -604,6 +658,9 @@ function installMultiSkillsToHomes(multiRoot) { }; AGENT_DIRS.forEach((agentDir, index) => { + if (index === 0 && specificAgentDirs.length > 0) { + return; + } const baseDir = path.join(homeDir, agentDir); const parentGate = path.dirname(baseDir); if (index > 0 && !fs.existsSync(parentGate)) { @@ -617,6 +674,15 @@ function installMultiSkillsToHomes(multiRoot) { } }); + if (specificAgentDirs.length > 0 && installed > 0) { + try { + retireGenericSkillRoot(homeDir, managedNames); + } catch (err) { + console.warn(`⚠️ 通用 Skill 副本迁移失败: ${err.message}`); + failed += 1; + } + } + if (attempted === 0) { if (installToBase(path.join(homeDir, ".agents", "skills"))) { installed += 1; diff --git a/docs/rfc-skill-installation-and-upgrade.md b/docs/rfc-skill-installation-and-upgrade.md index 5f068fab..a33bc95f 100644 --- a/docs/rfc-skill-installation-and-upgrade.md +++ b/docs/rfc-skill-installation-and-upgrade.md @@ -142,6 +142,9 @@ Agent 仍只需以 `SKILL.md` 发现和加载 Skill；统一元数据位于 Agen 升级器对每个 Agent 目标执行： +- 先探测具体 Agent home；只在没有任何具体 Agent 时使用 `~/.agents/skills` 通用 fallback； +- 具体 Agent 安装成功后，将 `~/.agents/skills` 中旧的 DWS 受管副本可恢复地迁入备份，避免 Codex 等同时扫描两个根目录时重复发现同名 Skill； + 1. 只读计算对面布局、过期受管 Skill 和同名官方 Skill； 2. 在目标文件系统的 staging 中复制完整新集合； 3. staging 全部成功后，才将旧集合移入备份目录； diff --git a/internal/app/root.go b/internal/app/root.go index f24b4cf1..ef0998d1 100644 --- a/internal/app/root.go +++ b/internal/app/root.go @@ -681,6 +681,11 @@ func newRootCommandWithEngine(rootCtx context.Context, engine *pipeline.Engine, if _, err := parseAgentProduct(os.Getenv(agentproduct.EnvName)); err != nil { return err } + if shouldRepairNestedSkillLayout(cmd) { + if err := repairNestedMultiSkillLayout(); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "⚠️ 检测到旧升级器留下的嵌套 Skill，但自动修复失败: %v；请运行 dws skill setup --mode multi --yes\n", err) + } + } authpkg.SetRuntimeProfile(flags.Profile) // Apply OAuth credential overrides from CLI flags (highest priority). diff --git a/internal/app/skill_setup.go b/internal/app/skill_setup.go index c970b093..8e42a428 100644 --- a/internal/app/skill_setup.go +++ b/internal/app/skill_setup.go @@ -92,6 +92,7 @@ type skillSetupBackup struct { type skillSetupTargetPlan struct { Destination string Backups []skillSetupBackup + CleanupOnly bool } type skillSetupPlan struct { @@ -147,7 +148,8 @@ multi 模式支持按产品挑选： 备份失败时保留原目录并跳过该目标，绝不静默删除。 · 所有将被移除的目录都会在确认前逐条列出。 -不带 --mode 时进入交互式询问；不带 --target 时铺到所有检测到的 Agent 目录。 +不带 --mode 时进入交互式询问；不带 --target 时铺到检测到的具体 Agent 目录； +未检测到具体 Agent 时才回退到 ~/.agents/skills，避免同一 Agent 扫描两份 Skill。 skill 源默认取二进制内嵌的版本（升级二进制即升级 skill）；--source / DWS_SKILL_SOURCE 可显式覆盖。`, Example: ` dws skill setup --mode multi --target claude --dry-run dws skill setup --mode multi --target claude`, @@ -940,18 +942,67 @@ func agentHomeForMode(base, mode string) string { } func detectExistingAgentHomes(home, mode string) []string { - var out []string + var specific []string for i, rel := range skillSetupAgentHomes { + if i == 0 { + continue + } base := filepath.Join(home, rel) parent := filepath.Dir(base) - if i > 0 { - if _, err := skillSetupStat(parent); errors.Is(err, os.ErrNotExist) { - continue - } + if info, err := skillSetupStat(parent); err != nil || !info.IsDir() { + continue } - out = append(out, agentHomeForMode(base, mode)) + specific = append(specific, agentHomeForMode(base, mode)) } - return out + if len(specific) > 0 { + return specific + } + return []string{agentHomeForMode(filepath.Join(home, skillSetupAgentHomes[0]), mode)} +} + +func genericSkillCleanupTarget(dests []string, managed map[string]bool) (*skillSetupTargetPlan, error) { + home, err := skillSetupUserHomeDir() + if err != nil { + return nil, fmt.Errorf("无法解析 HOME 以计算通用 Skill 迁移: %w", err) + } + genericBase := filepath.Join(home, ".agents", "skills") + usesSpecificRoot := false + for _, dest := range dests { + base := dest + if filepath.Base(dest) == "dws" { + base = filepath.Dir(dest) + } + if filepath.Clean(base) != filepath.Clean(genericBase) { + usesSpecificRoot = true + break + } + } + if !usesSpecificRoot { + return nil, nil + } + + target := &skillSetupTargetPlan{Destination: genericBase, CleanupOnly: true} + add := func(path, reason string) { + if info, statErr := skillSetupStat(path); statErr == nil && info.IsDir() { + target.Backups = append(target.Backups, skillSetupBackup{Path: path, Reason: reason}) + } + } + add(filepath.Join(genericBase, "dws"), skillSetupBackupMutual) + entries, readErr := skillSetupReadDir(genericBase) + if readErr != nil && !errors.Is(readErr, os.ErrNotExist) { + return nil, fmt.Errorf("扫描通用 Skill 根目录失败 %s: %w", genericBase, readErr) + } + for _, entry := range entries { + path := filepath.Join(genericBase, entry.Name()) + if entry.IsDir() && isManagedDWSMultiSkillDir(path, managed) { + target.Backups = append(target.Backups, skillSetupBackup{Path: path, Reason: skillSetupBackupStale}) + } + } + if len(target.Backups) == 0 { + return nil, nil + } + sort.Slice(target.Backups, func(i, j int) bool { return target.Backups[i].Path < target.Backups[j].Path }) + return target, nil } func buildSkillSetupPlan(mode, src string, dests, multiSkillNames []string, filtered bool) (*skillSetupPlan, error) { @@ -1027,6 +1078,13 @@ func buildSkillSetupPlan(mode, src string, dests, multiSkillNames []string, filt sort.Slice(target.Backups, func(i, j int) bool { return target.Backups[i].Path < target.Backups[j].Path }) plan.Targets = append(plan.Targets, target) } + cleanupTarget, cleanupErr := genericSkillCleanupTarget(sortedDests, managedNames) + if cleanupErr != nil { + return nil, cleanupErr + } + if cleanupTarget != nil { + plan.Targets = append(plan.Targets, *cleanupTarget) + } return plan, nil } @@ -1079,7 +1137,11 @@ func renderSkillSetupPlan(out io.Writer, plan *skillSetupPlan) { } fmt.Fprintln(out, " destinations:") for _, target := range plan.Targets { - fmt.Fprintf(out, " - %s\n", target.Destination) + if target.CleanupOnly { + fmt.Fprintf(out, " - %s (仅迁移旧的通用 DWS 副本)\n", target.Destination) + } else { + fmt.Fprintf(out, " - %s\n", target.Destination) + } } fmt.Fprintln(out, " 将备份并移除（先保存到 ~/.dws/skill-backups/）：") count := 0 @@ -1676,6 +1738,21 @@ func executeSkillSetupPlan(plan *skillSetupPlan, out, errOut io.Writer) (install perTarget = len(plan.MultiSkillNames) } for _, target := range plan.Targets { + if target.CleanupOnly { + if skipped > 0 { + continue + } + if homeErr != nil { + fmt.Fprintf(errOut, " ✗ 无法解析 HOME，保留通用 Skill 副本 %s: %v\n", target.Destination, homeErr) + skipped++ + continue + } + if _, cleanupErr := backupSkillSetupTarget(home, target.Backups, out); cleanupErr != nil { + fmt.Fprintf(errOut, " ✗ 通用 Skill 副本迁移失败，已回滚 %s: %v\n", target.Destination, cleanupErr) + skipped++ + } + continue + } if len(target.Backups) > 0 && homeErr != nil { if plan.Mode == skillSetupModeMono { fmt.Fprintf(errOut, " ✗ 无法解析 HOME，跳过刷新（保留原目录） %s: %v\n", target.Destination, homeErr) diff --git a/internal/app/skill_setup_test.go b/internal/app/skill_setup_test.go index f92c2dd3..297b6274 100644 --- a/internal/app/skill_setup_test.go +++ b/internal/app/skill_setup_test.go @@ -330,6 +330,24 @@ func TestResolveSkillSetupTargetsMultiOmitsDwsTail(t *testing.T) { } } +func TestCrossPlatformCoverageResolveSkillSetupTargetsPrefersSpecificAgentRoot(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills", ".codex/skills"}) + if err := os.MkdirAll(filepath.Join(home, ".codex"), 0o755); err != nil { + t.Fatal(err) + } + + got, err := resolveSkillSetupTargets("all", skillSetupModeMulti) + if err != nil { + t.Fatal(err) + } + want := filepath.Join(home, ".codex", "skills") + if len(got) != 1 || filepath.Clean(got[0]) != filepath.Clean(want) { + t.Fatalf("targets = %v, want [%s]", got, want) + } +} + func TestCrossPlatformCoverageInstallSkillToHomesEndToEnd(t *testing.T) { src := t.TempDir() if err := os.WriteFile(filepath.Join(src, "SKILL.md"), []byte("# test"), 0o644); err != nil { diff --git a/internal/app/skill_startup_repair.go b/internal/app/skill_startup_repair.go new file mode 100644 index 00000000..f045b1a6 --- /dev/null +++ b/internal/app/skill_startup_repair.go @@ -0,0 +1,64 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 + +package app + +import ( + "fmt" + "path/filepath" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" + "github.com/spf13/cobra" +) + +var repairNestedSkillUpgrade = upgrade.UpgradeSkillLocationsWithOptions + +// repairNestedMultiSkillLayout closes the compatibility gap where an older +// running dws process replaces itself with a newer binary, but then installs +// the new release bundle with its legacy mono copier. That produces the +// impossible layout /dws/multi//SKILL.md. The new binary cannot +// change the already-running old process, so its next invocation repairs this +// exact malformed layout from the bundle embedded in the new binary. +// +// A legitimate mono install has no dws/multi product tree and is untouched. +func repairNestedMultiSkillLayout() error { + home, err := skillSetupUserHomeDir() + if err != nil { + return err + } + found := false + for _, rel := range skillSetupAgentHomes { + nested := filepath.Join(home, rel, "dws", "multi") + if isSkillSourceRoot(nested, skillSetupModeMulti) { + found = true + break + } + } + if !found { + return nil + } + + source, cleanup, err := materializeEmbeddedSkillSource(skillSetupModeMulti) + if err != nil { + return fmt.Errorf("提取当前版本内嵌 multi Skill 失败: %w", err) + } + defer cleanup() + result, err := repairNestedSkillUpgrade(source, upgrade.SkillUpgradeOptions{Version: RawVersion()}) + if err != nil { + return err + } + if failed := result.Failed(); len(failed) > 0 { + return fmt.Errorf("%d 个 Agent 目录修复失败", len(failed)) + } + return nil +} + +func shouldRepairNestedSkillLayout(cmd *cobra.Command) bool { + if cmd == nil { + return true + } + if cmd.Name() == "upgrade" { + return false + } + return !(cmd.Name() == "setup" && cmd.Parent() != nil && cmd.Parent().Name() == "skill") +} diff --git a/internal/app/skill_startup_repair_test.go b/internal/app/skill_startup_repair_test.go new file mode 100644 index 00000000..c07598ad --- /dev/null +++ b/internal/app/skill_startup_repair_test.go @@ -0,0 +1,90 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 + +package app + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + upgradepkg "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" + "github.com/spf13/cobra" +) + +func TestCrossPlatformCoverageStartupRepairsNestedUpgradeLayout(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + upgradepkg.SwapUserHomeDirForTest(t, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills", ".codex/skills"}) + if err := os.MkdirAll(filepath.Join(home, ".codex"), 0o755); err != nil { + t.Fatal(err) + } + nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("old nested"), 0o644); err != nil { + t.Fatal(err) + } + + if err := repairNestedMultiSkillLayout(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(home, ".codex", "skills", "dingtalk-chat", "SKILL.md")); err != nil { + t.Fatalf("canonical Codex Skill missing: %v", err) + } + if _, err := os.Stat(filepath.Join(home, ".agents", "skills", "dws")); !os.IsNotExist(err) { + t.Fatalf("nested generic layout remains: %v", err) + } +} + +func TestCrossPlatformCoverageStartupRepairIgnoresLegitimateMonoAndReportsFailure(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) + mono := filepath.Join(home, ".agents", "skills", "dws") + if err := os.MkdirAll(mono, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(mono, "SKILL.md"), []byte("valid mono"), 0o644); err != nil { + t.Fatal(err) + } + called := false + testseam.Swap(t, &repairNestedSkillUpgrade, func(string, upgradepkg.SkillUpgradeOptions) (*upgradepkg.SkillUpgradeResult, error) { + called = true + return nil, errors.New("unexpected") + }) + if err := repairNestedMultiSkillLayout(); err != nil || called { + t.Fatalf("valid mono repair = %v, called=%v", err, called) + } + + nested := filepath.Join(mono, "multi", "dingtalk-chat") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { + t.Fatal(err) + } + if err := repairNestedMultiSkillLayout(); err == nil { + t.Fatal("repair failure was not surfaced") + } +} + +func TestCrossPlatformCoverageStartupRepairSkipsExplicitSkillManagers(t *testing.T) { + upgradeCmd := &cobra.Command{Use: "upgrade"} + if shouldRepairNestedSkillLayout(upgradeCmd) { + t.Fatal("upgrade must manage its own Skill lifecycle") + } + skillCmd := &cobra.Command{Use: "skill"} + setupCmd := &cobra.Command{Use: "setup"} + skillCmd.AddCommand(setupCmd) + if shouldRepairNestedSkillLayout(setupCmd) { + t.Fatal("skill setup must manage its own Skill lifecycle") + } + if !shouldRepairNestedSkillLayout(&cobra.Command{Use: "version"}) || !shouldRepairNestedSkillLayout(nil) { + t.Fatal("ordinary commands must trigger repair detection") + } +} diff --git a/internal/upgrade/coverage_edges_test.go b/internal/upgrade/coverage_edges_test.go index 9333e95e..960d81f7 100644 --- a/internal/upgrade/coverage_edges_test.go +++ b/internal/upgrade/coverage_edges_test.go @@ -264,7 +264,7 @@ func TestCrossPlatformCoverageUpgradePathsAndSkillsEdges(t *testing.T) { } knownSkillDirs = []string{".agents/skills", ".real/skills", ".missing/skills", ".present/skills"} result, err := UpgradeSkillLocations(source) - if err != nil || len(result.Succeeded()) != 2 || len(result.Failed()) != 0 { + if err != nil || len(result.Succeeded()) != 1 || len(result.Failed()) != 0 { t.Fatalf("skill upgrade = %#v, %v", result, err) } knownSkillDirs = []string{".real/skills"} diff --git a/internal/upgrade/paths.go b/internal/upgrade/paths.go index 0f90d998..16c4d74b 100644 --- a/internal/upgrade/paths.go +++ b/internal/upgrade/paths.go @@ -35,8 +35,10 @@ const ( // - test/scripts/package_script_test.go expectedPackagedSkillTargets // - scripts/release/verify-package-managers.sh HOME_AGENT_PARENTS / HOME_SKILL_TARGETS // -// The first entry (.agents/skills) is always updated; subsequent entries are -// only updated when their parent directory already exists. +// The first entry (.agents/skills) is a generic fallback. It is used only +// when no concrete Agent home is detected; otherwise publishing there would +// duplicate every Skill for Agents (including Codex) that scan both roots. +// Subsequent entries are updated when their parent directory already exists. var knownSkillDirs = []string{ ".agents/skills", ".claude/skills", @@ -201,9 +203,9 @@ func (r *SkillUpgradeResult) Failed() []SkillDirResult { // `dws skill setup --mode mono` flow. // // Strategy (matches npm install.js installSkillsToHomes): -// - ~/.agents/skills/ is ALWAYS updated (primary install location) -// - Other agent dirs (claude, cursor, ...) are updated only when the parent -// directory exists (e.g. ~/.claude/ exists => user has Claude) +// - Concrete agent dirs (claude, cursor, codex, ...) are updated when their +// parent directory exists (e.g. ~/.codex/ exists => user has Codex) +// - ~/.agents/skills/ is used only when no concrete Agent is detected // - ~/.real/ and other blacklisted paths are NEVER touched // - If no location was updated at all, fall back to ~/.agents/skills/ // @@ -514,23 +516,57 @@ func publishMultiUpgradeTarget(homeDir, destBase, multiRoot string, skills []str return publishStagedSkillSet(homeDir, staged, victims) } +func hasDetectedSpecificSkillRoot(homeDir string) bool { + for _, agentDir := range knownSkillDirs { + if isGenericSkillRoot(agentDir) || isBlacklisted(agentDir) { + continue + } + parentGate := filepath.Dir(filepath.Join(homeDir, agentDir)) + if info, err := upgradeStat(parentGate); err == nil && info.IsDir() { + return true + } + } + return false +} + +func isGenericSkillRoot(agentDir string) bool { + return filepath.Clean(agentDir) == filepath.Clean(".agents/skills") +} + +func retireGenericSkillRoot(homeDir string, managed map[string]bool) error { + base := filepath.Join(homeDir, ".agents", "skills") + victims, err := managedMultiSkillVictims(base, managed) + if err != nil { + return err + } + victims = append(victims, filepath.Join(base, "dws")) + if _, err := backupSkillSet(homeDir, victims); err != nil { + return fmt.Errorf("迁移通用 Skill 根目录失败: %w", err) + } + return nil +} + // upgradeMonoSkillLocations is the legacy mono behavior: one dws/ directory // per agent home. func upgradeMonoSkillLocations(homeDir, skillSrc string) (*SkillUpgradeResult, error) { result := &SkillUpgradeResult{} managedNames := readManagedSkillNames(homeDir) + hasSpecificRoot := hasDetectedSpecificSkillRoot(homeDir) - for i, agentDir := range knownSkillDirs { + for _, agentDir := range knownSkillDirs { destDir := filepath.Join(homeDir, agentDir, "dws") if isBlacklisted(agentDir) { result.Results = append(result.Results, SkillDirResult{Dir: destDir, Status: SkillDirBlacklisted}) continue } + if isGenericSkillRoot(agentDir) && hasSpecificRoot { + continue + } - if i > 0 { + if !isGenericSkillRoot(agentDir) { parentGate := filepath.Dir(filepath.Join(homeDir, agentDir)) - if _, err := os.Stat(parentGate); os.IsNotExist(err) { + if _, err := upgradeStat(parentGate); os.IsNotExist(err) { result.Results = append(result.Results, SkillDirResult{Dir: destDir, Status: SkillDirSkipped}) continue } @@ -542,12 +578,20 @@ func upgradeMonoSkillLocations(homeDir, skillSrc string) (*SkillUpgradeResult, e } result.Results = append(result.Results, SkillDirResult{Dir: destDir, Status: SkillDirOK}) } + if hasSpecificRoot && len(result.Succeeded()) > 0 { + genericBase := filepath.Join(homeDir, ".agents", "skills") + if err := retireGenericSkillRoot(homeDir, managedNames); err != nil { + result.Results = append(result.Results, SkillDirResult{Dir: genericBase, Status: SkillDirFailed, Err: err}) + } else { + result.Results = append(result.Results, SkillDirResult{Dir: genericBase, Status: SkillDirSkipped}) + } + } // Fallback: if nothing succeeded, force the primary location. The multi // leftovers under the primary base are the usual reason the primary // install failed, so clean them first — failing loud like the multi // fallback — instead of letting mono and multi co-exist marked OK. - if len(result.Succeeded()) == 0 { + if len(result.Succeeded()) == 0 && !hasSpecificRoot { destBase := filepath.Join(homeDir, ".agents", "skills") dest := filepath.Join(destBase, "dws") if err := publishMonoUpgradeTarget(homeDir, destBase, skillSrc, managedNames); err != nil { @@ -588,18 +632,22 @@ func upgradeMultiSkillLocations(homeDir, multiRoot string, skills []string) (*Sk result := &SkillUpgradeResult{} managedNames := readManagedSkillNames(homeDir) + hasSpecificRoot := hasDetectedSpecificSkillRoot(homeDir) - for i, agentDir := range knownSkillDirs { + for _, agentDir := range knownSkillDirs { destBase := filepath.Join(homeDir, agentDir) if isBlacklisted(agentDir) { result.Results = append(result.Results, SkillDirResult{Dir: destBase, Status: SkillDirBlacklisted}) continue } + if isGenericSkillRoot(agentDir) && hasSpecificRoot { + continue + } - if i > 0 { + if !isGenericSkillRoot(agentDir) { parentGate := filepath.Dir(destBase) - if _, err := os.Stat(parentGate); os.IsNotExist(err) { + if _, err := upgradeStat(parentGate); os.IsNotExist(err) { result.Results = append(result.Results, SkillDirResult{Dir: destBase, Status: SkillDirSkipped}) continue } @@ -611,9 +659,17 @@ func upgradeMultiSkillLocations(homeDir, multiRoot string, skills []string) (*Sk } result.Results = append(result.Results, SkillDirResult{Dir: destBase, Status: SkillDirOK}) } + if hasSpecificRoot && len(result.Succeeded()) > 0 { + genericBase := filepath.Join(homeDir, ".agents", "skills") + if err := retireGenericSkillRoot(homeDir, managedNames); err != nil { + result.Results = append(result.Results, SkillDirResult{Dir: genericBase, Status: SkillDirFailed, Err: err}) + } else { + result.Results = append(result.Results, SkillDirResult{Dir: genericBase, Status: SkillDirSkipped}) + } + } // Fallback: if nothing succeeded, force the primary location - if len(result.Succeeded()) == 0 { + if len(result.Succeeded()) == 0 && !hasSpecificRoot { destBase := filepath.Join(homeDir, ".agents", "skills") if err := publishMultiUpgradeTarget(homeDir, destBase, multiRoot, skills, skillSet, managedNames); err != nil { return result, fmt.Errorf("所有技能目录安装失败，回退到主目录也失败: %w", err) diff --git a/internal/upgrade/paths_multi_test.go b/internal/upgrade/paths_multi_test.go index ad791144..f76fb8a2 100644 --- a/internal/upgrade/paths_multi_test.go +++ b/internal/upgrade/paths_multi_test.go @@ -124,7 +124,8 @@ func TestCrossPlatformCoverageBundleSkillNamesLayouts(t *testing.T) { func TestCrossPlatformCoverageUpgradeSkillLocationsMulti(t *testing.T) { home := withFakeHome(t) - // .agents always installs; .claude installs (parent exists); .cursor skipped. + // A concrete Agent root wins over the generic .agents fallback; .claude + // installs and .cursor is skipped. agentsBase := filepath.Join(home, ".agents", "skills") claudeBase := filepath.Join(home, ".claude", "skills") for _, base := range []string{agentsBase, claudeBase} { @@ -164,7 +165,7 @@ func TestCrossPlatformCoverageUpgradeSkillLocationsMulti(t *testing.T) { t.Fatalf("expected 0 failures, got %v", failed) } - for _, base := range []string{agentsBase, claudeBase} { + for _, base := range []string{claudeBase} { if _, err := os.Stat(filepath.Join(base, "dws")); !os.IsNotExist(err) { t.Errorf("mono leftover still present: %s", filepath.Join(base, "dws")) } @@ -192,15 +193,18 @@ func TestCrossPlatformCoverageUpgradeSkillLocationsMulti(t *testing.T) { // Succeeded entries report the agent home base in multi mode. succeeded := result.Succeeded() - if len(succeeded) != 2 { - t.Fatalf("Succeeded() len = %d, want 2 (%v)", len(succeeded), result.Results) + if len(succeeded) != 1 { + t.Fatalf("Succeeded() len = %d, want 1 (%v)", len(succeeded), result.Results) } - wantDirs := map[string]bool{agentsBase: true, claudeBase: true} + wantDirs := map[string]bool{claudeBase: true} for _, d := range succeeded { if !wantDirs[d.Dir] { t.Errorf("unexpected succeeded dir %q", d.Dir) } } + if _, err := os.Stat(filepath.Join(agentsBase, "dws")); !os.IsNotExist(err) { + t.Fatalf("generic mono duplicate still visible: %v", err) + } // Multi cache refreshed under the fake home. if _, err := os.Stat(filepath.Join(home, ".dws", "skills", "multi", "dingtalk-chat", "SKILL.md")); err != nil { @@ -212,6 +216,45 @@ func TestCrossPlatformCoverageUpgradeSkillLocationsMulti(t *testing.T) { } } +func TestCrossPlatformCoverageUpgradeUsesAgentSpecificRootWithoutGenericDuplicate(t *testing.T) { + home := withFakeHome(t) + testseam.Swap(t, &knownSkillDirs, []string{".agents/skills", ".codex/skills"}) + + genericBase := filepath.Join(home, ".agents", "skills") + codexBase := filepath.Join(home, ".codex", "skills") + for _, base := range []string{genericBase, codexBase} { + if err := os.MkdirAll(base, 0o755); err != nil { + t.Fatal(err) + } + } + legacyNested := filepath.Join(genericBase, "dws", "multi", "dingtalk-chat") + if err := os.MkdirAll(legacyNested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(legacyNested, "SKILL.md"), []byte("old nested"), 0o644); err != nil { + t.Fatal(err) + } + + multiRoot := writeMultiBundle(t, t.TempDir(), "dingtalk-chat", "dingtalk-shared") + result, err := UpgradeSkillLocationsWithOptions(multiRoot, SkillUpgradeOptions{Version: "1.2.3"}) + if err != nil || len(result.Failed()) != 0 { + t.Fatalf("UpgradeSkillLocationsWithOptions() = %#v, %v", result, err) + } + + want := filepath.Join(codexBase, "dingtalk-chat", "SKILL.md") + if _, err := os.Stat(want); err != nil { + t.Fatalf("canonical Codex Skill missing at %s: %v", want, err) + } + for _, duplicate := range []string{ + filepath.Join(genericBase, "dingtalk-chat", "SKILL.md"), + filepath.Join(genericBase, "dws", "multi", "dingtalk-chat", "SKILL.md"), + } { + if _, err := os.Stat(duplicate); !os.IsNotExist(err) { + t.Fatalf("generic duplicate still visible at %s: %v", duplicate, err) + } + } +} + func TestCrossPlatformCoverageUpgradeSkillLocationsMultiFallbackPrimary(t *testing.T) { home := withFakeHome(t) // No agent parent dirs at all: only .agents (index 0) is attempted and the @@ -508,7 +551,7 @@ func TestCrossPlatformCoverageUpgradeSkillLocationsMonoReadDirErrorFailsHome(t * if len(failed) != 1 { t.Fatalf("Failed() len = %d, want 1 (%v)", len(failed), result.Results) } - wantDir := filepath.Join(agentsBase, "dws") + wantDir := agentsBase if failed[0].Dir != wantDir || failed[0].Err == nil { t.Fatalf("failed entry = %#v, want dir %q with non-nil err", failed[0], wantDir) } @@ -931,8 +974,8 @@ func TestCrossPlatformCoverageMultiUpgradeBackupAndFallbackEdges(t *testing.T) { t.Fatalf("blacklisted home must never be touched, stat err=%v", err) } - // Per-skill backup failure fails the home; a second home still succeeds so - // the fallback never runs. + // A detected concrete Agent root wins over .agents. A failure while retiring + // the old generic copy is surfaced after the concrete root succeeds. home2 := t.TempDir() testseam.Swap(t, &upgradeUserHomeDir, func() (string, error) { return home2, nil }) knownSkillDirs = []string{".agents/skills", ".claude/skills"} @@ -956,12 +999,13 @@ func TestCrossPlatformCoverageMultiUpgradeBackupAndFallbackEdges(t *testing.T) { } testseam.Swap(t, &upgradeRename, os.Rename) - // Per-skill copy failure fails the home the same way. + // Per-skill copy failure on the concrete home fails that home; with no + // successful concrete target, the generic copy is left untouched. home3 := t.TempDir() testseam.Swap(t, &upgradeUserHomeDir, func() (string, error) { return home3, nil }) os.MkdirAll(filepath.Join(home3, ".claude"), 0o755) testseam.Swap(t, &upgradeCopyDir, func(src, dst string) error { - if strings.Contains(dst, ".agents") { + if strings.Contains(dst, ".claude") { return errors.New("copy denied") } return copyDir(src, dst) diff --git a/scripts/install-skills.sh b/scripts/install-skills.sh index 2eab3cc0..a4563dff 100755 --- a/scripts/install-skills.sh +++ b/scripts/install-skills.sh @@ -408,6 +408,28 @@ multi_tree_has_skills() { return 1 } +# Move DWS-owned copies out of the generic root once a concrete Agent root is +# active. This prevents Agents such as Codex from discovering duplicates. +retire_generic_skill_root() { + _rgs_root="$1" + _rgs_base="$_rgs_root/.agents/skills" + _rgs_stage="$(mktemp -d "${TMPDIR:-/tmp}/dws-retire-generic.XXXXXX")" || return 1 + _rgs_backups="$_rgs_stage/backups" + : > "$_rgs_backups" || { rm -rf "$_rgs_stage"; return 1; } + for _rgs_victim in "$_rgs_base/dws" "$_rgs_base"/*; do + [ -d "$_rgs_victim" ] || continue + if [ "$(basename "$_rgs_victim")" != "dws" ] && ! is_managed_multi_skill_dir "$_rgs_victim"; then + continue + fi + if ! backup_and_record_skill_dir "$_rgs_victim" "$_rgs_backups"; then + restore_multi_skill_set /dev/null "$_rgs_backups" || true + rm -rf "$_rgs_stage" + return 1 + fi + done + rm -rf "$_rgs_stage" +} + # Same semantics as build/npm/install.js installMultiSkillsToHomes (root = DWS_SKILLS_ROOT or PWD). install_multi_skills_to_root() { multi_src="$1" @@ -416,6 +438,15 @@ install_multi_skills_to_root() { attempted=0 failed=0 idx=0 + specific_agents=0 + for specific_dir in \ + ".claude/skills" ".cursor/skills" ".qoder/skills" ".qoderwork/skills" \ + ".gemini/skills" ".codex/skills" ".github/skills" ".windsurf/skills" \ + ".augment/skills" ".cline/skills" ".amp/skills" ".kiro/skills" \ + ".trae/skills" ".openclaw/skills" ".hermes/skills" + do + [ -e "$root/$(dirname "$specific_dir")" ] && specific_agents=$((specific_agents + 1)) + done for agent_dir in \ ".agents/skills" \ ".claude/skills" \ @@ -434,6 +465,10 @@ install_multi_skills_to_root() { ".openclaw/skills" \ ".hermes/skills" do + if [ "$idx" -eq 0 ] && [ "$specific_agents" -gt 0 ]; then + idx=$((idx + 1)) + continue + fi base_dir="$root/$agent_dir" parent_gate="$(dirname "$base_dir")" if [ "$idx" -gt 0 ] && [ ! -e "$parent_gate" ]; then @@ -449,6 +484,9 @@ install_multi_skills_to_root() { fi idx=$((idx + 1)) done + if [ "$specific_agents" -gt 0 ] && [ "$installed" -gt 0 ]; then + retire_generic_skill_root "$root" || failed=$((failed + 1)) + fi if [ "$attempted" -eq 0 ] && _install_multi_to_base "$multi_src" "$root/.agents/skills" "$root" ".agents/skills"; then installed=$((installed + 1)) fi @@ -618,6 +656,15 @@ install_skills_to_root() { attempted=0 failed=0 idx=0 + specific_agents=0 + for specific_dir in \ + ".claude/skills" ".cursor/skills" ".qoder/skills" ".qoderwork/skills" \ + ".gemini/skills" ".codex/skills" ".github/skills" ".windsurf/skills" \ + ".augment/skills" ".cline/skills" ".amp/skills" ".kiro/skills" \ + ".trae/skills" ".openclaw/skills" ".hermes/skills" + do + [ -e "$root/$(dirname "$specific_dir")" ] && specific_agents=$((specific_agents + 1)) + done for agent_dir in \ ".agents/skills" \ ".claude/skills" \ @@ -636,6 +683,10 @@ install_skills_to_root() { ".openclaw/skills" \ ".hermes/skills" do + if [ "$idx" -eq 0 ] && [ "$specific_agents" -gt 0 ]; then + idx=$((idx + 1)) + continue + fi base_dir="$root/$agent_dir" parent_gate="$(dirname "$base_dir")" if [ "$idx" -gt 0 ] && [ ! -e "$parent_gate" ]; then @@ -655,6 +706,9 @@ install_skills_to_root() { fi idx=$((idx + 1)) done + if [ "$specific_agents" -gt 0 ] && [ "$installed" -gt 0 ]; then + retire_generic_skill_root "$root" || failed=$((failed + 1)) + fi if [ "$attempted" -eq 0 ]; then if [ "$root" = "$HOME" ]; then flabel="~/.agents/skills/$SKILL_NAME" diff --git a/scripts/install.ps1 b/scripts/install.ps1 index e9edbeb9..8bcd5d80 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -457,6 +457,38 @@ function Restore-MultiSkillSet { return $ok } +function Move-GenericSkillRootToBackup { + param([string]$Root) + + $baseDir = Join-Path $Root ".agents\skills" + $victims = [System.Collections.Generic.List[string]]::new() + $victims.Add((Join-Path $baseDir $SkillName)) + foreach ($existing in Get-ChildItem -Path $baseDir -Directory -ErrorAction SilentlyContinue) { + if (Test-ManagedMultiSkillDir -Dir $existing.FullName) { + $victims.Add($existing.FullName) + } + } + $backups = @() + try { + $seen = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($victim in $victims) { + if (!$seen.Add($victim)) { continue } + $backupPath = "" + if (!(Backup-SkillDir -Dir $victim -BackupPath ([ref]$backupPath))) { + throw "通用 Skill 副本备份失败: $victim" + } + if ($backupPath) { + $backups += [pscustomobject]@{ Original = $victim; Backup = $backupPath } + } + } + return $true + } catch { + Restore-MultiSkillSet -Published @() -Backups $backups | Out-Null + Write-Say "⚠️ 通用 Skill 副本迁移失败，已回滚: $_" + return $false + } +} + function Copy-SkillToDir { param([string]$SkillSrc, [string]$Dest, [string]$Label) @@ -780,7 +812,11 @@ function Install-SkillsToHomes { $installed = 0 $attempted = 0 $failed = 0 + $specificAgents = @($AgentDirs | Select-Object -Skip 1 | Where-Object { + Test-Path (Split-Path (Join-Path $Root $_) -Parent) + }) for ($i = 0; $i -lt $AgentDirs.Count; $i++) { + if ($i -eq 0 -and $specificAgents.Count -gt 0) { continue } $agentDir = $AgentDirs[$i] $baseDir = Join-Path $Root $agentDir $parentGate = Split-Path $baseDir -Parent @@ -800,6 +836,9 @@ function Install-SkillsToHomes { $failed++ } } + if ($specificAgents.Count -gt 0 -and $installed -gt 0) { + if (!(Move-GenericSkillRootToBackup -Root $Root)) { $failed++ } + } if ($attempted -eq 0) { $fallback = Join-Path (Join-Path $Root ".agents\skills") $SkillName if ($Root -eq $HOME) { @@ -851,7 +890,11 @@ function Install-MultiSkillsToHomes { $installed = 0 $attempted = 0 $failed = 0 + $specificAgents = @($AgentDirs | Select-Object -Skip 1 | Where-Object { + Test-Path (Split-Path (Join-Path $Root $_) -Parent) + }) for ($i = 0; $i -lt $AgentDirs.Count; $i++) { + if ($i -eq 0 -and $specificAgents.Count -gt 0) { continue } $agentDir = $AgentDirs[$i] $baseDir = Join-Path $Root $agentDir $parentGate = Split-Path $baseDir -Parent @@ -866,6 +909,9 @@ function Install-MultiSkillsToHomes { $failed++ } } + if ($specificAgents.Count -gt 0 -and $installed -gt 0) { + if (!(Move-GenericSkillRootToBackup -Root $Root)) { $failed++ } + } if ($attempted -eq 0) { if (Install-MultiToBase -MultiSrc $MultiSrc -BaseDir (Join-Path $Root ".agents\skills") -Root $Root -AgentDir ".agents\skills") { $installed++ diff --git a/scripts/install.sh b/scripts/install.sh index 18ca3a6a..f7ef7fea 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -643,6 +643,29 @@ _install_mono_to_base() { say "✅ Skills → ${_mono_label} (${_mono_count} files)" } +# Move DWS-owned copies out of the generic root once a concrete Agent root is +# active. This prevents Agents such as Codex from discovering the same Skill +# through both ~/.agents/skills and ~/.codex/skills. +retire_generic_skill_root() { + _rgs_root="$1" + _rgs_base="$_rgs_root/.agents/skills" + _rgs_stage="$(mktemp -d "${TMPDIR:-/tmp}/dws-retire-generic.XXXXXX")" || return 1 + _rgs_backups="$_rgs_stage/backups" + : > "$_rgs_backups" || { rm -rf "$_rgs_stage"; return 1; } + for _rgs_victim in "$_rgs_base/dws" "$_rgs_base"/*; do + [ -d "$_rgs_victim" ] || continue + if [ "$(basename "$_rgs_victim")" != "dws" ] && ! is_managed_multi_skill_dir "$_rgs_victim"; then + continue + fi + if ! backup_and_record_skill_dir "$_rgs_victim" "$_rgs_backups"; then + restore_multi_skill_set /dev/null "$_rgs_backups" || true + rm -rf "$_rgs_stage" + return 1 + fi + done + rm -rf "$_rgs_stage" +} + # Install skill tree into all agent homes (same rules as build/npm/install.js installSkillsToHomes). # Installing mono removes proven DWS-managed multi leftovers for mutual exclusion, # mirroring `dws skill setup --mode mono`. @@ -653,6 +676,15 @@ install_skills_to_homes() { attempted=0 failed=0 idx=0 + specific_agents=0 + for specific_dir in \ + ".claude/skills" ".cursor/skills" ".qoder/skills" ".qoderwork/skills" \ + ".gemini/skills" ".codex/skills" ".github/skills" ".windsurf/skills" \ + ".augment/skills" ".cline/skills" ".amp/skills" ".kiro/skills" \ + ".trae/skills" ".openclaw/skills" ".hermes/skills" + do + [ -e "$root/$(dirname "$specific_dir")" ] && specific_agents=$((specific_agents + 1)) + done for agent_dir in \ ".agents/skills" \ ".claude/skills" \ @@ -671,6 +703,10 @@ install_skills_to_homes() { ".openclaw/skills" \ ".hermes/skills" do + if [ "$idx" -eq 0 ] && [ "$specific_agents" -gt 0 ]; then + idx=$((idx + 1)) + continue + fi base_dir="$root/$agent_dir" parent_gate="$(dirname "$base_dir")" if [ "$idx" -gt 0 ] && [ ! -e "$parent_gate" ]; then @@ -693,6 +729,9 @@ install_skills_to_homes() { fi idx=$((idx + 1)) done + if [ "$specific_agents" -gt 0 ] && [ "$installed" -gt 0 ]; then + retire_generic_skill_root "$root" || failed=$((failed + 1)) + fi if [ "$attempted" -eq 0 ]; then case "$root" in "$HOME") @@ -746,6 +785,15 @@ install_multi_skills_to_homes() { attempted=0 failed=0 idx=0 + specific_agents=0 + for specific_dir in \ + ".claude/skills" ".cursor/skills" ".qoder/skills" ".qoderwork/skills" \ + ".gemini/skills" ".codex/skills" ".github/skills" ".windsurf/skills" \ + ".augment/skills" ".cline/skills" ".amp/skills" ".kiro/skills" \ + ".trae/skills" ".openclaw/skills" ".hermes/skills" + do + [ -e "$root/$(dirname "$specific_dir")" ] && specific_agents=$((specific_agents + 1)) + done for agent_dir in \ ".agents/skills" \ ".claude/skills" \ @@ -764,6 +812,10 @@ install_multi_skills_to_homes() { ".openclaw/skills" \ ".hermes/skills" do + if [ "$idx" -eq 0 ] && [ "$specific_agents" -gt 0 ]; then + idx=$((idx + 1)) + continue + fi base_dir="$root/$agent_dir" parent_gate="$(dirname "$base_dir")" if [ "$idx" -gt 0 ] && [ ! -e "$parent_gate" ]; then @@ -779,6 +831,9 @@ install_multi_skills_to_homes() { fi idx=$((idx + 1)) done + if [ "$specific_agents" -gt 0 ] && [ "$installed" -gt 0 ]; then + retire_generic_skill_root "$root" || failed=$((failed + 1)) + fi if [ "$attempted" -eq 0 ] && _install_multi_to_base "$multi_src" "$root/.agents/skills" "$root" ".agents/skills"; then installed=$((installed + 1)) fi diff --git a/test/scripts/install_js_smoke.mjs b/test/scripts/install_js_smoke.mjs index be3fb627..6bf29d82 100644 --- a/test/scripts/install_js_smoke.mjs +++ b/test/scripts/install_js_smoke.mjs @@ -193,6 +193,38 @@ scenario("multi install lays out sibling skills and caches", () => { } }); +scenario("Codex uses its canonical root without a generic duplicate", () => { + const { tmp, pkg, home } = stagePkg({ + "mono/SKILL.md": "# mono fixture\n", + "multi/dingtalk-chat/SKILL.md": "# dingtalk-chat\n", + "multi/dws-shared/SKILL.md": "# dws-shared\n", + }); + try { + writeFile(path.join(home, ".codex", "config.toml"), "model = \"test\"\n"); + writeFile( + path.join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat", "SKILL.md"), + "old nested duplicate\n", + ); + + const res = runInstall(pkg, home, "multi"); + assert.equal(res.status, 0, `exit=${res.status}\nstdout=${res.stdout}\nstderr=${res.stderr}`); + assert.ok( + fs.existsSync(path.join(home, ".codex", "skills", "dingtalk-chat", "SKILL.md")), + "Codex canonical Skill installed", + ); + assert.ok( + !fs.existsSync(path.join(home, ".agents", "skills", "dingtalk-chat", "SKILL.md")), + "generic flat duplicate not installed", + ); + assert.ok( + !fs.existsSync(path.join(home, ".agents", "skills", "dws")), + "legacy nested duplicate retired", + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } +}); + scenario("empty multi/ tree falls back to mono and keeps the old multi cache", () => { const { tmp, pkg, home } = stagePkg({ "SKILL.md": "# mono root copy\n", diff --git a/test/scripts/install_script_test.go b/test/scripts/install_script_test.go index 5b5fc6c5..5a890f75 100644 --- a/test/scripts/install_script_test.go +++ b/test/scripts/install_script_test.go @@ -1245,6 +1245,105 @@ install_multi_skills_to_root "$DWS_TEST_MULTI" "$DWS_TEST_ROOT" assertSkillProvenance(t, home, filepath.Join(base, "dingtalk-test"), "dingtalk-test", "install-skills.sh") } +func TestInstallerShellPrefersCodexRootWithoutGenericDuplicate(t *testing.T) { + for _, scriptName := range []string{"install.sh", "install-skills.sh"} { + t.Run(scriptName, func(t *testing.T) { + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", scriptName)) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.LastIndex(string(data), "\nmain\n") + if cut < 0 { + t.Fatalf("%s final main invocation not found", scriptName) + } + library := filepath.Join(t.TempDir(), scriptName+"-lib.sh") + mustWriteFile(t, library, data[:cut], 0o755) + + home := t.TempDir() + source := filepath.Join(t.TempDir(), "multi") + mustWriteFile(t, filepath.Join(home, ".codex", "config.toml"), []byte("model=test\n"), 0o644) + mustWriteFile(t, filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat", "SKILL.md"), []byte("old nested\n"), 0o644) + mustWriteFile(t, filepath.Join(source, "dingtalk-chat", "SKILL.md"), []byte("new chat\n"), 0o644) + + installCall := `install_multi_skills_to_homes "$DWS_TEST_SOURCE"` + if scriptName == "install-skills.sh" { + installCall = `install_multi_skills_to_root "$DWS_TEST_SOURCE" "$HOME"` + } + cmd := exec.Command("sh", "-c", `. "$DWS_TEST_LIBRARY" +`+installCall+` +`) + cmd.Env = append(os.Environ(), "HOME="+home, "DWS_TEST_LIBRARY="+library, "DWS_TEST_SOURCE="+source) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%s Codex-root harness failed: %v\n%s", scriptName, err, output) + } + if _, err := os.Stat(filepath.Join(home, ".codex", "skills", "dingtalk-chat", "SKILL.md")); err != nil { + t.Fatalf("%s canonical Codex Skill missing: %v", scriptName, err) + } + for _, duplicate := range []string{ + filepath.Join(home, ".agents", "skills", "dingtalk-chat", "SKILL.md"), + filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat", "SKILL.md"), + } { + if _, err := os.Stat(duplicate); !os.IsNotExist(err) { + t.Fatalf("%s generic duplicate remains at %s: %v", scriptName, duplicate, err) + } + } + }) + } +} + +func TestInstallPowerShellPrefersCodexRootWithoutGenericDuplicate(t *testing.T) { + pwsh, err := exec.LookPath("pwsh") + if err != nil { + if runtime.GOOS == "windows" { + pwsh, err = exec.LookPath("powershell") + } + if err != nil { + t.Skip("PowerShell is not available") + } + } + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", "install.ps1")) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.LastIndex(string(data), "# ── Main") + if cut < 0 { + t.Fatal("install.ps1 main section not found") + } + prefix := strings.ReplaceAll(string(data[:cut]), "$HOME", "$env:DWS_TEST_HOME") + prefix += ` +if (!(Install-MultiSkillsToHomes -MultiSrc $env:DWS_TEST_MULTI -Root $env:DWS_TEST_HOME)) { exit 2 } +exit 0 +` + harnessPath := filepath.Join(t.TempDir(), "install-codex-root.ps1") + mustWriteFile(t, harnessPath, []byte(prefix), 0o644) + + home := t.TempDir() + multi := filepath.Join(t.TempDir(), "multi") + mustWriteFile(t, filepath.Join(home, ".codex", "config.toml"), []byte("model=test\n"), 0o644) + mustWriteFile(t, filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat", "SKILL.md"), []byte("old nested\n"), 0o644) + mustWriteFile(t, filepath.Join(multi, "dingtalk-chat", "SKILL.md"), []byte("new chat\n"), 0o644) + + cmd := exec.Command(pwsh, "-NoProfile", "-NonInteractive", "-File", harnessPath) + cmd.Env = append(os.Environ(), "DWS_TEST_HOME="+home, "DWS_TEST_MULTI="+multi) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("PowerShell Codex-root harness failed: %v\n%s", err, output) + } + if _, err := os.Stat(filepath.Join(home, ".codex", "skills", "dingtalk-chat", "SKILL.md")); err != nil { + t.Fatalf("PowerShell canonical Codex Skill missing: %v", err) + } + if _, err := os.Stat(filepath.Join(home, ".agents", "skills", "dws")); !os.IsNotExist(err) { + t.Fatalf("PowerShell generic duplicate remains: %v", err) + } +} + func TestInstallPowerShellBackupFailureWritesNoMultiSkills(t *testing.T) { pwsh, err := exec.LookPath("pwsh") if err != nil { From 566e94a31e61b7064338079a75b3a2c178b5d361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Wed, 12 Aug 2026 13:19:52 +0800 Subject: [PATCH 08/13] fix(skill): make generic cleanup deterministic --- internal/app/skill_setup.go | 30 +++++++++++++++++++-------- internal/app/skill_setup_plan_test.go | 23 ++++++++++++++++++++ test/scripts/install_script_test.go | 17 +++++++-------- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/internal/app/skill_setup.go b/internal/app/skill_setup.go index 8e42a428..f3391b90 100644 --- a/internal/app/skill_setup.go +++ b/internal/app/skill_setup.go @@ -961,25 +961,37 @@ func detectExistingAgentHomes(home, mode string) []string { } func genericSkillCleanupTarget(dests []string, managed map[string]bool) (*skillSetupTargetPlan, error) { - home, err := skillSetupUserHomeDir() - if err != nil { - return nil, fmt.Errorf("无法解析 HOME 以计算通用 Skill 迁移: %w", err) - } - genericBase := filepath.Join(home, ".agents", "skills") - usesSpecificRoot := false + // Derive HOME from a concrete Agent destination instead of resolving it a + // second time. The destinations were already resolved from HOME by the + // caller, and a later/transient UserHomeDir failure must not turn an + // otherwise valid setup plan into an error. Direct/custom destinations that + // do not match a known concrete Agent root have no generic-root migration. + home := "" for _, dest := range dests { base := dest if filepath.Base(dest) == "dws" { base = filepath.Dir(dest) } - if filepath.Clean(base) != filepath.Clean(genericBase) { - usesSpecificRoot = true + base = filepath.Clean(base) + for i, rel := range skillSetupAgentHomes { + if i == 0 { + continue + } + suffix := filepath.Clean(filepath.FromSlash(rel)) + needle := string(filepath.Separator) + suffix + if strings.HasSuffix(base, needle) { + home = strings.TrimSuffix(base, needle) + break + } + } + if home != "" { break } } - if !usesSpecificRoot { + if home == "" { return nil, nil } + genericBase := filepath.Join(home, ".agents", "skills") target := &skillSetupTargetPlan{Destination: genericBase, CleanupOnly: true} add := func(path, reason string) { diff --git a/internal/app/skill_setup_plan_test.go b/internal/app/skill_setup_plan_test.go index f911ce9d..74885c44 100644 --- a/internal/app/skill_setup_plan_test.go +++ b/internal/app/skill_setup_plan_test.go @@ -146,6 +146,29 @@ func TestCrossPlatformCoverageSkillSetupMonoPlanIncludesSameNameTarget(t *testin } } +func TestCrossPlatformCoverageSkillSetupGenericCleanupDerivesHomeFromConcreteTarget(t *testing.T) { + home := t.TempDir() + dest := filepath.Join(home, ".codex", "skills") + genericMono := filepath.Join(home, ".agents", "skills", "dws") + if err := os.MkdirAll(genericMono, 0o755); err != nil { + t.Fatal(err) + } + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { + return "", errors.New("transient HOME failure") + }) + + plan, err := buildSkillSetupPlan(skillSetupModeMulti, "source", []string{dest}, []string{"dingtalk-chat"}, true) + if err != nil { + t.Fatal(err) + } + if len(plan.Targets) != 2 || !plan.Targets[1].CleanupOnly || plan.Targets[1].Destination != filepath.Dir(genericMono) { + t.Fatalf("generic cleanup target = %#v", plan.Targets) + } + if len(plan.Targets[1].Backups) != 1 || plan.Targets[1].Backups[0].Path != genericMono { + t.Fatalf("generic cleanup backups = %#v", plan.Targets[1].Backups) + } +} + func TestCrossPlatformCoverageSkillSetupPlanDeduplicatesAndFailsClosed(t *testing.T) { dest := filepath.Join(t.TempDir(), "skills") if err := os.MkdirAll(filepath.Join(dest, "dws"), 0o755); err != nil { diff --git a/test/scripts/install_script_test.go b/test/scripts/install_script_test.go index 5a890f75..c6295d06 100644 --- a/test/scripts/install_script_test.go +++ b/test/scripts/install_script_test.go @@ -15,11 +15,6 @@ import ( "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/skillstate" ) -var expectedHomeSkillTargets = []string{ - ".agents/skills/dws", - ".cursor/skills/dws", -} - func assertSkillProvenance(t *testing.T, home, skillDir, name, source string) { t.Helper() state, readable, err := skillstate.Read(home) @@ -187,11 +182,13 @@ func TestInstallScriptSourceModeInstallsSkillsIntoAgentsDir(t *testing.T) { t.Fatalf("install.sh error = %v\noutput:\n%s", err, string(output)) } - for _, rel := range expectedHomeSkillTargets { - skillPath := filepath.Join(fixture.fakeHome, filepath.FromSlash(rel), "SKILL.md") - if _, err := os.Stat(skillPath); err != nil { - t.Fatalf("Stat(%s) error = %v\noutput:\n%s", skillPath, err, string(output)) - } + skillPath := filepath.Join(fixture.fakeHome, ".cursor", "skills", "dws", "SKILL.md") + if _, err := os.Stat(skillPath); err != nil { + t.Fatalf("Stat(%s) error = %v\noutput:\n%s", skillPath, err, string(output)) + } + genericPath := filepath.Join(fixture.fakeHome, ".agents", "skills", "dws") + if _, err := os.Stat(genericPath); !os.IsNotExist(err) { + t.Fatalf("generic Skill root must not duplicate detected Cursor: Stat(%s) = %v\noutput:\n%s", genericPath, err, string(output)) } } From 24ea2505a5d9999e592ccbf7f4169d28810a31ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Wed, 12 Aug 2026 13:44:14 +0800 Subject: [PATCH 09/13] test(skill): cover upgrade migration branches --- internal/app/skill_setup_plan_test.go | 57 ++++++++++++++++++++ internal/app/skill_startup_repair_test.go | 64 +++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/internal/app/skill_setup_plan_test.go b/internal/app/skill_setup_plan_test.go index 74885c44..fcc5ceb0 100644 --- a/internal/app/skill_setup_plan_test.go +++ b/internal/app/skill_setup_plan_test.go @@ -167,6 +167,63 @@ func TestCrossPlatformCoverageSkillSetupGenericCleanupDerivesHomeFromConcreteTar if len(plan.Targets[1].Backups) != 1 || plan.Targets[1].Backups[0].Path != genericMono { t.Fatalf("generic cleanup backups = %#v", plan.Targets[1].Backups) } + var preview bytes.Buffer + renderSkillSetupPlan(&preview, plan) + if !strings.Contains(preview.String(), "仅迁移旧的通用 DWS 副本") { + t.Fatalf("generic cleanup preview missing: %s", preview.String()) + } + + t.Run("managed multi and scan failure", func(t *testing.T) { + managedDir := filepath.Join(home, ".agents", "skills", "dingtalk-chat") + if err := os.MkdirAll(managedDir, 0o755); err != nil { + t.Fatal(err) + } + target, targetErr := genericSkillCleanupTarget([]string{dest}, map[string]bool{"dingtalk-chat": true}) + if targetErr != nil || target == nil || len(target.Backups) != 2 { + t.Fatalf("managed generic cleanup = %#v, %v", target, targetErr) + } + failure := errors.New("generic scan failure") + testseam.Swap(t, &skillSetupReadDir, func(string) ([]os.DirEntry, error) { return nil, failure }) + if _, targetErr := genericSkillCleanupTarget([]string{dest}, nil); !errors.Is(targetErr, failure) { + t.Fatalf("generic scan error = %v", targetErr) + } + if _, planErr := buildSkillSetupPlan(skillSetupModeMulti, "source", []string{dest}, []string{"dingtalk-chat"}, true); !errors.Is(planErr, failure) { + t.Fatalf("generic cleanup plan error = %v", planErr) + } + }) +} + +func TestCrossPlatformCoverageSkillSetupCleanupOnlyExecutionBranches(t *testing.T) { + failure := errors.New("cleanup failure") + cleanup := skillSetupTargetPlan{Destination: "generic", CleanupOnly: true, Backups: []skillSetupBackup{{Path: "old"}}} + + t.Run("prior skip suppresses cleanup", func(t *testing.T) { + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return t.TempDir(), nil }) + plan := &skillSetupPlan{Mode: skillSetupModeMono, Source: "missing", Targets: []skillSetupTargetPlan{{Destination: "install"}, cleanup}} + installed, skipped, err := executeSkillSetupPlan(plan, io.Discard, io.Discard) + if err != nil || installed != 0 || skipped != 1 { + t.Fatalf("cleanup after skip = (%d, %d, %v)", installed, skipped, err) + } + }) + + t.Run("home failure keeps generic copy", func(t *testing.T) { + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) + var stderr bytes.Buffer + _, skipped, err := executeSkillSetupPlan(&skillSetupPlan{Mode: skillSetupModeMono, Targets: []skillSetupTargetPlan{cleanup}}, io.Discard, &stderr) + if err != nil || skipped != 1 || !strings.Contains(stderr.String(), "保留通用 Skill 副本") { + t.Fatalf("cleanup HOME failure = (%d, %v, %q)", skipped, err, stderr.String()) + } + }) + + t.Run("backup failure is reported", func(t *testing.T) { + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return t.TempDir(), nil }) + testseam.Swap(t, &skillSetupBackupAndRemove, func(string, string) (string, error) { return "", failure }) + var stderr bytes.Buffer + _, skipped, err := executeSkillSetupPlan(&skillSetupPlan{Mode: skillSetupModeMono, Targets: []skillSetupTargetPlan{cleanup}}, io.Discard, &stderr) + if err != nil || skipped != 1 || !strings.Contains(stderr.String(), "迁移失败") { + t.Fatalf("cleanup backup failure = (%d, %v, %q)", skipped, err, stderr.String()) + } + }) } func TestCrossPlatformCoverageSkillSetupPlanDeduplicatesAndFailsClosed(t *testing.T) { diff --git a/internal/app/skill_startup_repair_test.go b/internal/app/skill_startup_repair_test.go index c07598ad..47f7f751 100644 --- a/internal/app/skill_startup_repair_test.go +++ b/internal/app/skill_startup_repair_test.go @@ -4,9 +4,12 @@ package app import ( + "bytes" + "context" "errors" "os" "path/filepath" + "strings" "testing" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" @@ -88,3 +91,64 @@ func TestCrossPlatformCoverageStartupRepairSkipsExplicitSkillManagers(t *testing t.Fatal("ordinary commands must trigger repair detection") } } + +func TestCrossPlatformCoverageStartupRepairErrorBranchesAndRootWarning(t *testing.T) { + failure := errors.New("repair failure") + t.Run("HOME failure", func(t *testing.T) { + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) + if err := repairNestedMultiSkillLayout(); !errors.Is(err, failure) { + t.Fatalf("HOME error = %v", err) + } + }) + + t.Run("embedded extraction failure", func(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) + nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { + t.Fatal(err) + } + testseam.Swap(t, &embeddedSkillStat, func(string) (os.FileInfo, error) { return nil, failure }) + if err := repairNestedMultiSkillLayout(); !errors.Is(err, failure) { + t.Fatalf("embedded extraction error = %v", err) + } + }) + + t.Run("failed Agent result", func(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) + nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { + t.Fatal(err) + } + testseam.Swap(t, &repairNestedSkillUpgrade, func(string, upgradepkg.SkillUpgradeOptions) (*upgradepkg.SkillUpgradeResult, error) { + return &upgradepkg.SkillUpgradeResult{Results: []upgradepkg.SkillDirResult{{Status: upgradepkg.SkillDirFailed, Err: failure}}}, nil + }) + if err := repairNestedMultiSkillLayout(); err == nil { + t.Fatal("failed Agent repair succeeded") + } + }) + + t.Run("root warning", func(t *testing.T) { + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) + root := newRootCommandWithEngine(context.Background(), nil, false, true) + cmd := &cobra.Command{Use: "version"} + cmd.SetContext(context.Background()) + var stderr bytes.Buffer + cmd.SetErr(&stderr) + if err := root.PersistentPreRunE(cmd, nil); err != nil { + t.Fatal(err) + } + if !strings.Contains(stderr.String(), "自动修复失败") { + t.Fatalf("root repair warning missing: %q", stderr.String()) + } + }) +} From c2c11310792a6c23681eaf6975aa6405df2c9824 Mon Sep 17 00:00:00 2001 From: chichuan Date: Wed, 12 Aug 2026 13:52:25 +0800 Subject: [PATCH 10/13] fix: match the release archive directory literally, not as a regex release_version was interpolated into an awk regex, where '.' matches any character. Version 1.0.1-beta.1 therefore also admitted .changes/released/1x0x1-betaX1/, letting the archive drift from the CHANGELOG version while every other seal assertion still passed and breaking the documented audit trail. Compare the archive prefix with index() and split the basename off with substr(), matching the literal-comparison idiom already used throughout check-changelog-pr.sh. Only the basename, whose character class is fixed, stays a pattern. --- scripts/policy/check-release-fragments.sh | 13 +++- test/scripts/changelog_pr_gate_test.go | 74 +++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/scripts/policy/check-release-fragments.sh b/scripts/policy/check-release-fragments.sh index ec88f951..a93f113a 100755 --- a/scripts/policy/check-release-fragments.sh +++ b/scripts/policy/check-release-fragments.sh @@ -30,10 +30,17 @@ if [ "$archive_changed" = true ]; then printf '%s\n' 'error: release-fragment archival requires exactly one newly added versioned CHANGELOG section' >&2 exit 1 } - if ! awk -F '\t' -v version="$release_version" ' + # The archive directory is matched as a literal prefix, never as a regex: + # interpolating the version into one would make `.` match any character, so + # `1.0.1-beta.1` would also admit `.changes/released/1x0x1-betaX1/` and break + # the documented audit trail. Only the fragment basename, whose character class + # is fixed, stays a pattern. + if ! awk -F '\t' -v prefix=".changes/released/$release_version/" ' $1 == "M" && $2 == "CHANGELOG.md" && NF == 2 { changelog = 1; next } - $1 ~ /^R100$/ && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ && $3 ~ ("^\\.changes/released/" version "/[a-z0-9][a-z0-9._-]*\\.md$") && NF == 3 { - source = $2; target = $3; sub(/^.*\//, "", source); sub(/^.*\//, "", target) + $1 == "R100" && NF == 3 && $2 ~ /^\.changes\/[a-z0-9][a-z0-9._-]*\.md$/ && index($3, prefix) == 1 { + target = substr($3, length(prefix) + 1) + if (target !~ /^[a-z0-9][a-z0-9._-]*\.md$/) { invalid = 1; next } + source = $2; sub(/^.*\//, "", source) if (source != target) invalid = 1 moved++; next } diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 25fc18c1..bf05855b 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -43,6 +43,26 @@ const changelogGateValidRelease = `# Changelog - Initial release. ` +// A release-seal section whose 1.0.1-beta.1 notes are exactly the rendered form +// of changelogGateValidFragment. A seal PR built from this therefore satisfies +// the rendered-notes comparison, which isolates the archive-path assertions. +const changelogGateSealedRelease = `# Changelog + +## [Unreleased] + +## [1.0.1-beta.1] - 2026-07-17 + +### Added + +- Chat reply mentions. + +## [1.0.0] - 2026-07-01 + +### Added + +- Initial release. +` + func newChangelogGateRepo(t *testing.T) *changelogGateRepo { t.Helper() @@ -320,6 +340,60 @@ func TestReleaseFragmentPolicyRejectsInvalidActiveFragmentAndWrongArchiveVersion const changelogGateValidFragment = "---\ncategory: Added\n---\n\n- Chat reply mentions.\n" +// sealFragmentInto stages a release-seal PR that archives the single active +// fragment into .changes/released//, with a CHANGELOG section whose +// rendered notes already match. Only archiveDir varies, so a rejection can only +// come from the archive-path contract. +func (r *changelogGateRepo) sealFragmentInto(t *testing.T, archiveDir string) string { + t.Helper() + changelogGateWrite(t, r.root, ".changes/1234-chat.md", changelogGateValidFragment, 0o644) + r.commit(t, "add release fragment") + sealBase := strings.TrimSpace(changelogGateGit(t, r.root, "rev-parse", "HEAD")) + + changelogGateWrite(t, r.root, "CHANGELOG.md", changelogGateSealedRelease, 0o644) + target := filepath.Join(r.root, ".changes", "released", archiveDir) + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatalf("MkdirAll(%s) error = %v", target, err) + } + if err := os.Rename(filepath.Join(r.root, ".changes", "1234-chat.md"), filepath.Join(target, "1234-chat.md")); err != nil { + t.Fatalf("Rename release fragment error = %v", err) + } + r.commit(t, "seal release into "+archiveDir) + return sealBase +} + +// The archive directory used to be matched by interpolating the version into an +// awk regex, where `.` matches any character. Version `1.0.1-beta.1` therefore +// also admitted `1x0x1-betaX1`, letting the archive drift from the CHANGELOG +// version while every other seal assertion still passed. +func TestReleaseFragmentPolicyRejectsArchiveDirectoryMatchedAsRegex(t *testing.T) { + for _, archiveDir := range []string{"1x0x1-betaX1", "1.0.1-betaX1", "1x0.1-beta.1"} { + t.Run(archiveDir, func(t *testing.T) { + repo := newChangelogGateRepo(t) + sealBase := repo.sealFragmentInto(t, archiveDir) + + output, err := repo.runFragmentPolicy(t, sealBase, "HEAD") + if err == nil { + t.Fatalf("archive directory %q passed:\n%s", archiveDir, output) + } + if !strings.Contains(output, "unchanged R100 moves") { + t.Fatalf("gate output missing the archive-move contract:\n%s", output) + } + }) + } +} + +// Guards the fix against over-blocking: the archive directory that exactly +// equals the CHANGELOG version must still seal cleanly. +func TestReleaseFragmentPolicyAcceptsArchiveDirectoryMatchingChangelogVersion(t *testing.T) { + repo := newChangelogGateRepo(t) + sealBase := repo.sealFragmentInto(t, "1.0.1-beta.1") + + if output, err := repo.runFragmentPolicy(t, sealBase, "HEAD"); err != nil { + t.Fatalf("matching archive directory rejected: %v\noutput:\n%s", err, output) + } +} + // A top-level `.changes/` entry that violates the naming rule or is not a // regular 100644 blob must fail the gate. Selecting the trigger by the legal // name pattern used to let these entries through untouched, which both bypassed From fcbbc0bd9a538556f91275d3b800cd4c3c2976da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Wed, 12 Aug 2026 14:21:47 +0800 Subject: [PATCH 11/13] fix(skill): require explicit nested layout migration --- internal/app/root.go | 6 +- internal/app/skill_startup_repair.go | 42 ++---- internal/app/skill_startup_repair_test.go | 159 +++++++++------------- 3 files changed, 81 insertions(+), 126 deletions(-) diff --git a/internal/app/root.go b/internal/app/root.go index ef0998d1..f9ad0a9f 100644 --- a/internal/app/root.go +++ b/internal/app/root.go @@ -681,9 +681,9 @@ func newRootCommandWithEngine(rootCtx context.Context, engine *pipeline.Engine, if _, err := parseAgentProduct(os.Getenv(agentproduct.EnvName)); err != nil { return err } - if shouldRepairNestedSkillLayout(cmd) { - if err := repairNestedMultiSkillLayout(); err != nil { - fmt.Fprintf(cmd.ErrOrStderr(), "⚠️ 检测到旧升级器留下的嵌套 Skill，但自动修复失败: %v；请运行 dws skill setup --mode multi --yes\n", err) + if shouldDetectNestedSkillLayout(cmd) { + if found, err := detectNestedMultiSkillLayout(); err == nil && found { + fmt.Fprintln(cmd.ErrOrStderr(), "⚠️ 检测到旧升级器留下的嵌套 Skill；请运行 dws skill setup --mode multi 查看迁移计划并确认") } } diff --git a/internal/app/skill_startup_repair.go b/internal/app/skill_startup_repair.go index f045b1a6..ee854f62 100644 --- a/internal/app/skill_startup_repair.go +++ b/internal/app/skill_startup_repair.go @@ -4,56 +4,34 @@ package app import ( - "fmt" "path/filepath" - "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" "github.com/spf13/cobra" ) -var repairNestedSkillUpgrade = upgrade.UpgradeSkillLocationsWithOptions - -// repairNestedMultiSkillLayout closes the compatibility gap where an older +// detectNestedMultiSkillLayout detects the compatibility gap where an older // running dws process replaces itself with a newer binary, but then installs // the new release bundle with its legacy mono copier. That produces the -// impossible layout /dws/multi//SKILL.md. The new binary cannot -// change the already-running old process, so its next invocation repairs this -// exact malformed layout from the bundle embedded in the new binary. +// impossible layout /dws/multi//SKILL.md. // -// A legitimate mono install has no dws/multi product tree and is untouched. -func repairNestedMultiSkillLayout() error { +// Detection is deliberately read-only. Ordinary commands must not turn a +// compatibility warning into an unconfirmed, cross-Agent Skill refresh. +// A legitimate mono install has no dws/multi product tree and is ignored. +func detectNestedMultiSkillLayout() (bool, error) { home, err := skillSetupUserHomeDir() if err != nil { - return err + return false, err } - found := false for _, rel := range skillSetupAgentHomes { nested := filepath.Join(home, rel, "dws", "multi") if isSkillSourceRoot(nested, skillSetupModeMulti) { - found = true - break + return true, nil } } - if !found { - return nil - } - - source, cleanup, err := materializeEmbeddedSkillSource(skillSetupModeMulti) - if err != nil { - return fmt.Errorf("提取当前版本内嵌 multi Skill 失败: %w", err) - } - defer cleanup() - result, err := repairNestedSkillUpgrade(source, upgrade.SkillUpgradeOptions{Version: RawVersion()}) - if err != nil { - return err - } - if failed := result.Failed(); len(failed) > 0 { - return fmt.Errorf("%d 个 Agent 目录修复失败", len(failed)) - } - return nil + return false, nil } -func shouldRepairNestedSkillLayout(cmd *cobra.Command) bool { +func shouldDetectNestedSkillLayout(cmd *cobra.Command) bool { if cmd == nil { return true } diff --git a/internal/app/skill_startup_repair_test.go b/internal/app/skill_startup_repair_test.go index 47f7f751..bebca931 100644 --- a/internal/app/skill_startup_repair_test.go +++ b/internal/app/skill_startup_repair_test.go @@ -12,19 +12,15 @@ import ( "strings" "testing" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/skillstate" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" - upgradepkg "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" "github.com/spf13/cobra" ) -func TestCrossPlatformCoverageStartupRepairsNestedUpgradeLayout(t *testing.T) { +func TestCrossPlatformCoverageStartupDetectsNestedUpgradeLayoutWithoutMutation(t *testing.T) { home := t.TempDir() testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) - upgradepkg.SwapUserHomeDirForTest(t, func() (string, error) { return home, nil }) testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills", ".codex/skills"}) - if err := os.MkdirAll(filepath.Join(home, ".codex"), 0o755); err != nil { - t.Fatal(err) - } nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") if err := os.MkdirAll(nested, 0o755); err != nil { t.Fatal(err) @@ -33,18 +29,20 @@ func TestCrossPlatformCoverageStartupRepairsNestedUpgradeLayout(t *testing.T) { t.Fatal(err) } - if err := repairNestedMultiSkillLayout(); err != nil { - t.Fatal(err) + found, err := detectNestedMultiSkillLayout() + if err != nil || !found { + t.Fatalf("detect nested layout = (%v, %v), want (true, nil)", found, err) } - if _, err := os.Stat(filepath.Join(home, ".codex", "skills", "dingtalk-chat", "SKILL.md")); err != nil { - t.Fatalf("canonical Codex Skill missing: %v", err) + data, err := os.ReadFile(filepath.Join(nested, "SKILL.md")) + if err != nil || string(data) != "old nested" { + t.Fatalf("detection changed nested Skill: data=%q err=%v", data, err) } - if _, err := os.Stat(filepath.Join(home, ".agents", "skills", "dws")); !os.IsNotExist(err) { - t.Fatalf("nested generic layout remains: %v", err) + if _, err := os.Stat(filepath.Join(home, ".codex", "skills", "dingtalk-chat")); !os.IsNotExist(err) { + t.Fatalf("detection unexpectedly installed a canonical Skill: %v", err) } } -func TestCrossPlatformCoverageStartupRepairIgnoresLegitimateMonoAndReportsFailure(t *testing.T) { +func TestCrossPlatformCoverageStartupDetectionIgnoresMonoAndReportsReadFailure(t *testing.T) { home := t.TempDir() testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) @@ -55,100 +53,79 @@ func TestCrossPlatformCoverageStartupRepairIgnoresLegitimateMonoAndReportsFailur if err := os.WriteFile(filepath.Join(mono, "SKILL.md"), []byte("valid mono"), 0o644); err != nil { t.Fatal(err) } - called := false - testseam.Swap(t, &repairNestedSkillUpgrade, func(string, upgradepkg.SkillUpgradeOptions) (*upgradepkg.SkillUpgradeResult, error) { - called = true - return nil, errors.New("unexpected") - }) - if err := repairNestedMultiSkillLayout(); err != nil || called { - t.Fatalf("valid mono repair = %v, called=%v", err, called) + found, err := detectNestedMultiSkillLayout() + if err != nil || found { + t.Fatalf("valid mono detection = (%v, %v), want (false, nil)", found, err) } - nested := filepath.Join(mono, "multi", "dingtalk-chat") - if err := os.MkdirAll(nested, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { - t.Fatal(err) - } - if err := repairNestedMultiSkillLayout(); err == nil { - t.Fatal("repair failure was not surfaced") + failure := errors.New("HOME failure") + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) + found, err = detectNestedMultiSkillLayout() + if found || !errors.Is(err, failure) { + t.Fatalf("HOME failure detection = (%v, %v)", found, err) } } -func TestCrossPlatformCoverageStartupRepairSkipsExplicitSkillManagers(t *testing.T) { +func TestCrossPlatformCoverageStartupDetectionSkipsExplicitSkillManagers(t *testing.T) { upgradeCmd := &cobra.Command{Use: "upgrade"} - if shouldRepairNestedSkillLayout(upgradeCmd) { + if shouldDetectNestedSkillLayout(upgradeCmd) { t.Fatal("upgrade must manage its own Skill lifecycle") } skillCmd := &cobra.Command{Use: "skill"} setupCmd := &cobra.Command{Use: "setup"} skillCmd.AddCommand(setupCmd) - if shouldRepairNestedSkillLayout(setupCmd) { + if shouldDetectNestedSkillLayout(setupCmd) { t.Fatal("skill setup must manage its own Skill lifecycle") } - if !shouldRepairNestedSkillLayout(&cobra.Command{Use: "version"}) || !shouldRepairNestedSkillLayout(nil) { - t.Fatal("ordinary commands must trigger repair detection") + if !shouldDetectNestedSkillLayout(&cobra.Command{Use: "version"}) || !shouldDetectNestedSkillLayout(nil) { + t.Fatal("ordinary commands must trigger read-only detection") } } -func TestCrossPlatformCoverageStartupRepairErrorBranchesAndRootWarning(t *testing.T) { - failure := errors.New("repair failure") - t.Run("HOME failure", func(t *testing.T) { - testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) - if err := repairNestedMultiSkillLayout(); !errors.Is(err, failure) { - t.Fatalf("HOME error = %v", err) - } +func TestCrossPlatformCoverageStartupWarningIsReadOnlyAndDoesNotBypassConfirmation(t *testing.T) { + home := t.TempDir() + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + testseam.Swap(t, &skillSetupAgentHomes, []string{".codex/skills"}) + nested := filepath.Join(home, ".codex", "skills", "dws", "multi", "dingtalk-chat") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("old nested"), 0o644); err != nil { + t.Fatal(err) + } + + // Any accidental return to startup mutation must fail this test before it + // can touch the fixture. + testseam.Swap(t, &skillSetupCopyDir, func(string, string) error { + t.Fatal("ordinary command attempted to copy Skills") + return nil + }) + testseam.Swap(t, &skillSetupBackupAndRemove, func(string, string) (string, error) { + t.Fatal("ordinary command attempted to back up or remove Skills") + return "", nil + }) + testseam.Swap(t, &skillSetupWriteState, func(string, skillstate.State) error { + t.Fatal("ordinary command attempted to write Skill state") + return nil }) - t.Run("embedded extraction failure", func(t *testing.T) { - home := t.TempDir() - testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) - testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) - nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") - if err := os.MkdirAll(nested, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { - t.Fatal(err) - } - testseam.Swap(t, &embeddedSkillStat, func(string) (os.FileInfo, error) { return nil, failure }) - if err := repairNestedMultiSkillLayout(); !errors.Is(err, failure) { - t.Fatalf("embedded extraction error = %v", err) - } - }) - - t.Run("failed Agent result", func(t *testing.T) { - home := t.TempDir() - testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) - testseam.Swap(t, &skillSetupAgentHomes, []string{".agents/skills"}) - nested := filepath.Join(home, ".agents", "skills", "dws", "multi", "dingtalk-chat") - if err := os.MkdirAll(nested, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(nested, "SKILL.md"), []byte("nested"), 0o644); err != nil { - t.Fatal(err) - } - testseam.Swap(t, &repairNestedSkillUpgrade, func(string, upgradepkg.SkillUpgradeOptions) (*upgradepkg.SkillUpgradeResult, error) { - return &upgradepkg.SkillUpgradeResult{Results: []upgradepkg.SkillDirResult{{Status: upgradepkg.SkillDirFailed, Err: failure}}}, nil - }) - if err := repairNestedMultiSkillLayout(); err == nil { - t.Fatal("failed Agent repair succeeded") - } - }) - - t.Run("root warning", func(t *testing.T) { - testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return "", failure }) - root := newRootCommandWithEngine(context.Background(), nil, false, true) - cmd := &cobra.Command{Use: "version"} - cmd.SetContext(context.Background()) - var stderr bytes.Buffer - cmd.SetErr(&stderr) - if err := root.PersistentPreRunE(cmd, nil); err != nil { - t.Fatal(err) - } - if !strings.Contains(stderr.String(), "自动修复失败") { - t.Fatalf("root repair warning missing: %q", stderr.String()) - } - }) + root := newRootCommandWithEngine(context.Background(), nil, false, true) + cmd := &cobra.Command{Use: "version"} + cmd.SetContext(context.Background()) + var stderr bytes.Buffer + cmd.SetErr(&stderr) + if err := root.PersistentPreRunE(cmd, nil); err != nil { + t.Fatal(err) + } + warning := stderr.String() + if !strings.Contains(warning, "dws skill setup --mode multi") { + t.Fatalf("safe migration hint missing: %q", warning) + } + if strings.Contains(warning, "--yes") { + t.Fatalf("migration hint bypasses confirmation: %q", warning) + } + data, err := os.ReadFile(filepath.Join(nested, "SKILL.md")) + if err != nil || string(data) != "old nested" { + t.Fatalf("ordinary command changed nested Skill: data=%q err=%v", data, err) + } } From 76a6980244c8302754df0770158c56ad1ec9cdaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B9=8B=E6=A1=90?= <1241839690@qq.com> Date: Wed, 12 Aug 2026 14:24:50 +0800 Subject: [PATCH 12/13] fix(drive): validate numeric file comment IDs --- internal/helpers/file_comment.go | 37 +++- internal/helpers/file_comment_test.go | 234 ++++++++++++++++++++++++++ 2 files changed, 267 insertions(+), 4 deletions(-) diff --git a/internal/helpers/file_comment.go b/internal/helpers/file_comment.go index a0e8fe91..f573de94 100644 --- a/internal/helpers/file_comment.go +++ b/internal/helpers/file_comment.go @@ -20,6 +20,7 @@ import ( "strconv" "strings" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/corecmd" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/corecmd/contract" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/output" "github.com/spf13/cobra" @@ -42,6 +43,14 @@ type fileCommentPage struct { comments []map[string]any } +func fileCommentNodeFlag() LeafFlag { + return LeafFlag{Name: "node", Usage: "文件 ID (dentryUuid)、数字 dentry ID 或钉盘文件 URL", Required: true, Aliases: []string{"url", "id", "node-id", "file-id"}, Bind: "fileId", Trim: true} +} + +func fileCommentSpaceIDFlag() LeafFlag { + return LeafFlag{Name: "space-id", Usage: "钉盘空间 ID；仅数字 dentry ID 必填", Bind: "spaceId", OmitEmpty: true, Trim: true, RequiredWhen: "--node is a numeric dentry ID"} +} + // newDriveFileCommentCmd follows the existing resource-first comment surface: // doc comment, sheet comment, and drive comment. The public command belongs to // Drive, while the implementation routes to the shared doc-comment MCP server. @@ -66,8 +75,8 @@ func newDriveFileCommentCmd() *cobra.Command { dws drive comment list --node --all --format json`, Tool: listFileCommentsTool, Flags: []LeafFlag{ - {Name: "node", Usage: "文件 ID (dentryUuid)、数字 dentry ID 或钉盘文件 URL", Required: true, Aliases: []string{"url", "id", "node-id", "file-id"}, Bind: "fileId", Trim: true}, - {Name: "space-id", Usage: "钉盘空间 ID；仅数字 dentry ID 必填", Bind: "spaceId", OmitEmpty: true, Trim: true}, + fileCommentNodeFlag(), + fileCommentSpaceIDFlag(), {Name: "limit", Usage: "每页评论数，范围 1-200", Kind: LeafInt, Default: "200", Aliases: []string{"page-size"}, Bind: "maxResults"}, {Name: "cursor", Usage: "分页游标，取自上页 nextCursor", Bind: "nextToken", OmitEmpty: true, Trim: true}, {Name: "all", Usage: "自动拉取全部评论，最多 10 页 / 2000 条", Kind: LeafBool, Bind: "all"}, @@ -126,8 +135,8 @@ func newDriveFileCommentCmd() *cobra.Command { Example: ` dws drive comment create --node --content "请补充最终结论" --format json`, Tool: createFileCommentTool, Flags: []LeafFlag{ - {Name: "node", Usage: "文件 ID (dentryUuid)、数字 dentry ID 或钉盘文件 URL", Required: true, Aliases: []string{"url", "id", "node-id", "file-id"}, Bind: "fileId", Trim: true}, - {Name: "space-id", Usage: "钉盘空间 ID；仅数字 dentry ID 必填", Bind: "spaceId", OmitEmpty: true, Trim: true}, + fileCommentNodeFlag(), + fileCommentSpaceIDFlag(), {Name: "content", Usage: "全文评论内容，纯文本且 UTF-16 长度不超过 2099", Required: true, Bind: "content"}, }, Constraints: []LeafConstraint{ @@ -175,6 +184,9 @@ func newDriveFileCommentCmd() *cobra.Command { } func validateFileCommentList(cmd *cobra.Command, _ []string) error { + if err := validateFileCommentNodeSpace(cmd); err != nil { + return err + } limit, _ := cmd.Flags().GetInt("limit") if cmd.Flags().Changed("page-size") { limit, _ = cmd.Flags().GetInt("page-size") @@ -198,6 +210,9 @@ func validateFileCommentList(cmd *cobra.Command, _ []string) error { } func validateFileCommentCreate(cmd *cobra.Command, _ []string) error { + if err := validateFileCommentNodeSpace(cmd); err != nil { + return err + } content, _ := cmd.Flags().GetString("content") if strings.TrimSpace(content) == "" { return &CLIError{Code: CodeInvalidParam, Message: "--content 去除首尾空白后不能为空"} @@ -212,6 +227,20 @@ func validateFileCommentCreate(cmd *cobra.Command, _ []string) error { return nil } +func validateFileCommentNodeSpace(cmd *cobra.Command) error { + node := corecmd.EffectiveValue(cmd, fileCommentNodeFlag()) + if !allASCIIDigits(node) { + return nil + } + if spaceID := corecmd.EffectiveValue(cmd, fileCommentSpaceIDFlag()); spaceID == "" { + return &CLIError{ + Code: CodeInvalidParam, + Message: "--node 为数字 dentry ID 时必须同时提供 --space-id", + } + } + return nil +} + func fileCommentUTF16Length(value string) int { length := 0 for _, r := range value { diff --git a/internal/helpers/file_comment_test.go b/internal/helpers/file_comment_test.go index 1af29a06..0f2dd795 100644 --- a/internal/helpers/file_comment_test.go +++ b/internal/helpers/file_comment_test.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "reflect" "strings" @@ -306,6 +307,7 @@ func TestDriveFileCommentListValidatesLocalParameters(t *testing.T) { {"zero limit", []string{"--limit", "0"}}, {"large limit", []string{"--page-size", "201"}}, {"nonnumeric cursor", []string{"--cursor", "next"}}, + {"overflow cursor", []string{"--cursor", "999999999999999999999999999999"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -322,6 +324,50 @@ func TestDriveFileCommentListValidatesLocalParameters(t *testing.T) { } } +func TestDriveFileCommentNumericNodeRequiresSpaceIDForListAndCreateAliases(t *testing.T) { + tests := []struct { + name string + args []string + }{ + { + name: "list id alias", + args: []string{"drive", "comment", "list", "--id", "231773999335"}, + }, + { + name: "create file-id alias", + args: []string{"drive", "comment", "create", "--file-id", "231773999335", "--content", "test", "--yes"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{} + _, err := executeFileCommentCommand(t, caller, "", tt.args...) + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeInvalidParam || !strings.Contains(cliErr.Message, "--space-id") { + t.Fatalf("error = %#v", err) + } + if len(caller.calls) != 0 { + t.Fatalf("numeric node without space-id reached MCP: %#v", caller.calls) + } + }) + } + + caller := &fileCommentTestCaller{responses: []string{`{ + "fileId":"resolved-file","total":0,"count":0,"hasMore":false,"nextToken":null,"items":[] + }`}} + if _, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "list", "--url", "231773999335", "--space-id", "2402756201", + ); err != nil { + t.Fatal(err) + } + if len(caller.calls) != 1 || caller.calls[0].args["fileId"] != "231773999335" || caller.calls[0].args["spaceId"] != "2402756201" { + t.Fatalf("numeric node with space-id args = %#v", caller.calls) + } + if allASCIIDigits("") { + t.Fatal("empty string unexpectedly accepted as numeric") + } +} + func TestDriveFileCommentCreateValidatesMapsAndProjects(t *testing.T) { validContent := strings.Repeat("a", fileCommentMaxContentLength) caller := &fileCommentTestCaller{responses: []string{`{"result":{"fileId":"resolved-file","comment":{ @@ -411,3 +457,191 @@ func TestDriveFileCommentRejectsMalformedResponseInsteadOfReturningEmpty(t *test t.Fatalf("error = %#v", err) } } + +func TestDriveFileCommentDryRunAndCallErrors(t *testing.T) { + for _, tt := range []struct { + name string + args []string + tool string + }{ + { + name: "list dry run", + args: []string{"drive", "comment", "list", "--node", "file-1"}, + tool: listFileCommentsTool, + }, + { + name: "create dry run", + args: []string{"drive", "comment", "create", "--node", "file-1", "--content", "test", "--yes"}, + tool: createFileCommentTool, + }, + } { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{dryRun: true} + out, err := executeFileCommentCommand(t, caller, "", tt.args...) + if err != nil { + t.Fatal(err) + } + if len(caller.calls) != 0 || !strings.Contains(string(out), `"tool": "`+tt.tool+`"`) { + t.Fatalf("calls = %#v, output = %s", caller.calls, out) + } + }) + } + + sentinel := errors.New("mcp unavailable") + for _, tt := range []struct { + name string + args []string + }{ + { + name: "list call error", + args: []string{"drive", "comment", "list", "--node", "file-1", "--all"}, + }, + { + name: "create call error", + args: []string{"drive", "comment", "create", "--node", "file-1", "--content", "test", "--yes"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{err: sentinel} + _, err := executeFileCommentCommand(t, caller, "", tt.args...) + if !errors.Is(err, sentinel) { + t.Fatalf("error = %#v", err) + } + }) + } +} + +func TestDriveFileCommentListResponseValidationBranches(t *testing.T) { + tests := []struct { + name string + response string + message string + }{ + {name: "empty", response: "", message: "返回为空"}, + {name: "invalid json", response: "{", message: "不是有效 JSON"}, + {name: "missing file id", response: `{}`, message: "缺少 fileId"}, + {name: "missing total", response: `{"fileId":"file-1","hasMore":false,"items":[]}`, message: "缺少 total"}, + {name: "missing has more", response: `{"fileId":"file-1","total":0,"items":[]}`, message: "缺少布尔字段 hasMore"}, + {name: "next token type", response: `{"fileId":"file-1","total":1,"hasMore":true,"nextToken":2,"items":[]}`, message: "nextToken 不是字符串"}, + {name: "items type", response: `{"fileId":"file-1","total":0,"hasMore":false,"items":{}}`, message: "items 不是数组"}, + {name: "item type", response: `{"fileId":"file-1","total":1,"hasMore":false,"items":[1]}`, message: "items[0] 不是对象"}, + {name: "missing comment id", response: `{"fileId":"file-1","total":1,"hasMore":false,"items":[{"anchor":{}}]}`, message: "缺少 commentId"}, + {name: "missing anchor", response: `{"fileId":"file-1","total":1,"hasMore":false,"items":[{"commentId":"1"}]}`, message: "缺少 anchor 对象"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{tt.response}} + _, err := executeFileCommentCommand(t, caller, "", "drive", "comment", "list", "--node", "file-1") + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeMCPToolError || !strings.Contains(cliErr.Message, tt.message) { + t.Fatalf("error = %#v", err) + } + }) + } + + caller := &fileCommentTestCaller{responses: []string{`{ + "data":{"fileId":"file-1","total":0,"hasMore":false,"nextToken":null,"items":null} + }`}} + out, err := executeFileCommentCommand(t, caller, "", "drive", "comment", "list", "--node", "file-1") + if err != nil { + t.Fatal(err) + } + if comments := decodeFileCommentTestOutput(t, out)["comments"].([]any); len(comments) != 0 { + t.Fatalf("comments = %#v", comments) + } +} + +func TestDriveFileCommentCreateResponseValidationBranches(t *testing.T) { + tests := []struct { + name string + response string + message string + }{ + {name: "invalid json", response: "{", message: "不是有效 JSON"}, + {name: "missing file id", response: `{"comment":{"commentId":"1","anchor":{}}}`, message: "缺少 fileId"}, + {name: "missing comment", response: `{"fileId":"file-1"}`, message: "缺少 comment 对象"}, + {name: "missing comment id", response: `{"fileId":"file-1","comment":{"anchor":{}}}`, message: "缺少 commentId"}, + {name: "missing anchor", response: `{"fileId":"file-1","comment":{"commentId":"1"}}`, message: "缺少 anchor 对象"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{tt.response}} + _, err := executeFileCommentCommand(t, caller, "", + "drive", "comment", "create", "--node", "file-1", "--content", "test", "--yes", + ) + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeMCPToolError || !strings.Contains(cliErr.Message, tt.message) { + t.Fatalf("error = %#v", err) + } + }) + } +} + +func TestDriveFileCommentPaginationCycleLimitAndPartialPage(t *testing.T) { + cycleCaller := &fileCommentTestCaller{responses: []string{ + `{"fileId":"file-1","total":0,"hasMore":true,"nextToken":"2","items":[]}`, + `{"fileId":"file-1","total":0,"hasMore":true,"nextToken":"3","items":[]}`, + `{"fileId":"file-1","total":0,"hasMore":true,"nextToken":"2","items":[]}`, + }} + _, err := executeFileCommentCommand(t, cycleCaller, "", "drive", "comment", "list", "--node", "file-1", "--all") + var cliErr *CLIError + if !errors.As(err, &cliErr) || cliErr.Code != CodeContentTruncated || !strings.Contains(cliErr.Message, "发生循环") { + t.Fatalf("cycle error = %#v", err) + } + + limitResponses := make([]string, 0, fileCommentMaxAutoPages) + for index := 0; index < fileCommentMaxAutoPages; index++ { + limitResponses = append(limitResponses, fmt.Sprintf( + `{"fileId":"file-1","total":0,"hasMore":true,"nextToken":"%d","items":[]}`, + index+1, + )) + } + limitCaller := &fileCommentTestCaller{responses: limitResponses} + _, err = executeFileCommentCommand(t, limitCaller, "", "drive", "comment", "list", "--node", "file-1", "--all") + if !errors.As(err, &cliErr) || cliErr.Code != CodeContentTruncated || !strings.Contains(cliErr.Message, "10 页上限") { + t.Fatalf("page limit error = %#v", err) + } + + partialCaller := &fileCommentTestCaller{responses: []string{`{ + "fileId":"file-1","total":1,"hasMore":true,"nextToken":"2", + "items":[{"commentId":"1","anchor":{"scope":"whole"}}] + }`}} + out, err := executeFileCommentCommand(t, partialCaller, "", "drive", "comment", "list", "--node", "file-1") + if err != nil { + t.Fatal(err) + } + payload := decodeFileCommentTestOutput(t, out) + if payload["nextCursor"] != "2" || payload["complete"] != false || payload["hasMore"] != true { + t.Fatalf("partial page = %#v", payload) + } + + overflowCaller := &fileCommentTestCaller{responses: []string{`{ + "fileId":"file-1","total":0,"hasMore":true, + "nextToken":"999999999999999999999999999999","items":[] + }`}} + _, err = executeFileCommentCommand(t, overflowCaller, "", "drive", "comment", "list", "--node", "file-1") + if !errors.As(err, &cliErr) || cliErr.Code != CodeContentTruncated || !strings.Contains(cliErr.Message, "超出 64 位整数范围") { + t.Fatalf("overflow cursor error = %#v", err) + } +} + +func TestDriveFileCommentInternalDefaults(t *testing.T) { + caller := &fileCommentTestCaller{responses: []string{`{ + "fileId":"file-1","total":0,"hasMore":false,"nextToken":null,"items":[] + }`}} + testseam.Protect(t, &deps) + InitDeps(caller) + var stdout bytes.Buffer + cmd := &cobra.Command{Use: "list"} + cmd.SetOut(&stdout) + if err := runFileCommentList(cmd, listFileCommentsTool, map[string]any{"fileId": "file-1", "maxResults": 200}); err != nil { + t.Fatal(err) + } + payload := decodeFileCommentTestOutput(t, stdout.Bytes()) + if payload["scope"] != "all" { + t.Fatalf("scope = %#v", payload["scope"]) + } + if comments := fileCommentListPayload(fileCommentPage{}, "all")["comments"].([]map[string]any); len(comments) != 0 { + t.Fatalf("nil comments projection = %#v", comments) + } +} From b1317264971e6e2e16c9eb5e84d4b9667fe8b294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B9=8B=E6=A1=90?= <1241839690@qq.com> Date: Wed, 12 Aug 2026 14:36:26 +0800 Subject: [PATCH 13/13] docs: add drive file comment release fragment --- .changes/961-drive-file-comments.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/961-drive-file-comments.md diff --git a/.changes/961-drive-file-comments.md b/.changes/961-drive-file-comments.md new file mode 100644 index 00000000..ee4823ad --- /dev/null +++ b/.changes/961-drive-file-comments.md @@ -0,0 +1,5 @@ +--- +category: Added +--- + +- **Drive file comments** (#961) — adds `dws drive comment list` and `dws drive comment create` for comments on ordinary preview files.