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. diff --git a/.changes/README.md b/.changes/README.md new file mode 100644 index 00000000..79b5868f --- /dev/null +++ b/.changes/README.md @@ -0,0 +1,34 @@ +# Release fragments + +普通功能、修复和面向用户的行为变更不要再修改根目录 `CHANGELOG.md` 的 +`Unreleased` 区域。每个 PR 在本目录新增一个独立的 Markdown fragment,避免 +并行 PR 争用同一文件。 + +文件名使用能唯一定位变更的短名,通常是 PR 号,例如 +`1234-chat-reply-mentions.md`。文件名必须匹配 +`^[a-z0-9][a-z0-9._-]*\.md$`,且必须是普通文件,不能是符号链接。本目录顶层 +只接受 `README.md`、`released/` 和符合该规则的 fragment:fragment 一律平铺在 +顶层,不接受任何其它子目录,本目录自身也不能被替换成文件或符号链接。其余条目 +会被 CI 直接拒绝而不是忽略,以免非法条目跳过校验后拖垮下一个 PR。文件格式 +严格如下: + +```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 dfafcb02..eace8f78 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1288,6 +1288,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 55b6e41a..ce9d6372 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -83,7 +83,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/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/ci-pr-gates.md b/docs/ci-pr-gates.md index 9d731a2a..5244b214 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 @@ -184,11 +190,11 @@ Schema,并让 candidate 对两份历史 contract 独立执行检查;它只 lifecycle 的 exact rename 规范化到当前历史副本,不会维护第二份 allowlist,也不会 放宽其他 Schema 历史字段。 -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/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..f9ad0a9f 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 shouldDetectNestedSkillLayout(cmd) { + if found, err := detectNestedMultiSkillLayout(); err == nil && found { + fmt.Fprintln(cmd.ErrOrStderr(), "⚠️ 检测到旧升级器留下的嵌套 Skill;请运行 dws skill setup --mode multi 查看迁移计划并确认") + } + } 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..f3391b90 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,79 @@ 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) { + if info, err := skillSetupStat(parent); err != nil || !info.IsDir() { + continue + } + specific = append(specific, agentHomeForMode(base, mode)) + } + 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) { + // 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) + } + 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 } - out = append(out, agentHomeForMode(base, mode)) } - return out + if home == "" { + return nil, nil + } + genericBase := filepath.Join(home, ".agents", "skills") + + 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 +1090,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 +1149,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 +1750,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_plan_test.go b/internal/app/skill_setup_plan_test.go index f911ce9d..fcc5ceb0 100644 --- a/internal/app/skill_setup_plan_test.go +++ b/internal/app/skill_setup_plan_test.go @@ -146,6 +146,86 @@ 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) + } + 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) { dest := filepath.Join(t.TempDir(), "skills") if err := os.MkdirAll(filepath.Join(dest, "dws"), 0o755); err != nil { 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..ee854f62 --- /dev/null +++ b/internal/app/skill_startup_repair.go @@ -0,0 +1,42 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 + +package app + +import ( + "path/filepath" + + "github.com/spf13/cobra" +) + +// 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. +// +// 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 false, err + } + for _, rel := range skillSetupAgentHomes { + nested := filepath.Join(home, rel, "dws", "multi") + if isSkillSourceRoot(nested, skillSetupModeMulti) { + return true, nil + } + } + return false, nil +} + +func shouldDetectNestedSkillLayout(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..bebca931 --- /dev/null +++ b/internal/app/skill_startup_repair_test.go @@ -0,0 +1,131 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 + +package app + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/skillstate" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/spf13/cobra" +) + +func TestCrossPlatformCoverageStartupDetectsNestedUpgradeLayoutWithoutMutation(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"}) + 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) + } + + found, err := detectNestedMultiSkillLayout() + if err != nil || !found { + t.Fatalf("detect nested layout = (%v, %v), want (true, nil)", found, 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, ".codex", "skills", "dingtalk-chat")); !os.IsNotExist(err) { + t.Fatalf("detection unexpectedly installed a canonical Skill: %v", err) + } +} + +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"}) + 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) + } + found, err := detectNestedMultiSkillLayout() + if err != nil || found { + t.Fatalf("valid mono detection = (%v, %v), want (false, nil)", found, err) + } + + 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 TestCrossPlatformCoverageStartupDetectionSkipsExplicitSkillManagers(t *testing.T) { + upgradeCmd := &cobra.Command{Use: "upgrade"} + 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 shouldDetectNestedSkillLayout(setupCmd) { + t.Fatal("skill setup must manage its own Skill lifecycle") + } + if !shouldDetectNestedSkillLayout(&cobra.Command{Use: "version"}) || !shouldDetectNestedSkillLayout(nil) { + t.Fatal("ordinary commands must trigger read-only detection") + } +} + +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 + }) + + 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) + } +} diff --git a/internal/helpers/drive.go b/internal/helpers/drive.go index 04087d0d..ccceafbf 100644 --- a/internal/helpers/drive.go +++ b/internal/helpers/drive.go @@ -3283,6 +3283,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..f573de94 --- /dev/null +++ b/internal/helpers/file_comment.go @@ -0,0 +1,576 @@ +// 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" + "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 +} + +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. +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{ + 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"}, + {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{ + fileCommentNodeFlag(), + fileCommentSpaceIDFlag(), + {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 { + 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") + } + 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 { + if err := validateFileCommentNodeSpace(cmd); err != nil { + return err + } + 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 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 { + 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..0f2dd795 --- /dev/null +++ b/internal/helpers/file_comment_test.go @@ -0,0 +1,647 @@ +// 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" + "fmt" + "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"}}, + {"overflow cursor", []string{"--cursor", "999999999999999999999999999999"}}, + } + 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 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":{ + "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) + } +} + +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) + } +} 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/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..a93f113a --- /dev/null +++ b/scripts/policy/check-release-fragments.sh @@ -0,0 +1,191 @@ +#!/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; } + +[ "$#" -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 + } + # 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" && 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 + } + { 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 + +# `.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. +git -C "$ROOT" diff --no-ext-diff --name-only "$merge_base" "$head" -- .changes >"$tmp_root/changes-paths" + +changes_tree_changed=false +if awk ' + $0 ~ /^\.changes\/released\// { next } + { found = 1 } + END { exit !found } +' "$tmp_root/changes-paths"; 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 awk ' + $0 ~ /^\.changes\/released\// { next } + $0 == ".changes/README.md" { next } + { found = 1 } + END { exit !found } +' "$tmp_root/changes-paths"; 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 ' + { + 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 + + # 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" + 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/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..1734a399 --- /dev/null +++ b/scripts/release/render-release-fragments.sh @@ -0,0 +1,116 @@ +#!/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() { + 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 + +# 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 + exit 1 +} + +validate_fragment() { + fragment="$1" + validate_fragment_name "${fragment##*/}" "$fragment" + + [ "$(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 92b6ef62..bf05855b 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 @@ -42,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() @@ -69,8 +90,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 +114,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 +235,386 @@ 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) + } + }) +} + +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 +// 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) + } +} + +// 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 changelog string @@ -250,7 +646,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/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..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)) } } @@ -1245,6 +1242,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 { diff --git a/test/scripts/release_script_test.go b/test/scripts/release_script_test.go index 1fe83120..6ad03b77 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"), @@ -1658,10 +1660,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") @@ -1675,55 +1680,115 @@ 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) + 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) } - data, err := os.ReadFile(filepath.Join(r.root, "CHANGELOG.md")) - if err != nil { - t.Fatalf("ReadFile(CHANGELOG.md) error = %v", err) +} + +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", + }, + { + // 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", + }, } - 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) - } + + 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) + } + }) } }