From 6463ab13c94cc8ccee91c1afcfa9b5281d682ed0 Mon Sep 17 00:00:00 2001 From: kongenpei Date: Wed, 1 Apr 2026 13:57:28 +0800 Subject: [PATCH 1/8] ci: make pkg.pr.new comment flow fork-safe (#170) * ci: make pkg.pr.new comment flow fork-safe * ci: harden trusted comment workflow inputs * ci: skip comment steps when payload artifact is missing * ci: use artifact PR number when workflow_run pull_requests is empty * ci: allow PR comment workflow to write pull requests --------- Co-authored-by: kongenpei --- .github/workflows/pkg-pr-new-comment.yml | 149 +++++++++++++++++++++++ .github/workflows/pkg-pr-new.yml | 101 ++++++--------- 2 files changed, 183 insertions(+), 67 deletions(-) create mode 100644 .github/workflows/pkg-pr-new-comment.yml diff --git a/.github/workflows/pkg-pr-new-comment.yml b/.github/workflows/pkg-pr-new-comment.yml new file mode 100644 index 00000000..56725fc6 --- /dev/null +++ b/.github/workflows/pkg-pr-new-comment.yml @@ -0,0 +1,149 @@ +name: PR Preview Package Comment + +on: + workflow_run: + workflows: ["PR Preview Package"] + types: [completed] + +permissions: + actions: read + contents: read + issues: write + pull-requests: write + +jobs: + comment: + if: github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.event == 'pull_request' + runs-on: ubuntu-latest + + steps: + - name: Check comment payload artifact + id: payload + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const runId = context.payload.workflow_run?.id; + const { data } = await github.rest.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: runId, + per_page: 100, + }); + const found = Boolean( + data.artifacts?.some((artifact) => artifact.name === "pkg-pr-new-comment-payload") + ); + core.setOutput("found", found ? "true" : "false"); + if (!found) { + core.notice("No comment payload artifact found for this run; skipping comment."); + } + + - name: Download comment payload + if: steps.payload.outputs.found == 'true' + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 + with: + name: pkg-pr-new-comment-payload + repository: ${{ github.repository }} + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Comment install command + if: steps.payload.outputs.found == 'true' + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const fs = require("fs"); + const payload = JSON.parse(fs.readFileSync("pkg-pr-new-comment-payload.json", "utf8")); + const url = payload?.url; + const payloadPr = payload?.pr; + const sourceRepo = payload?.sourceRepo; + const sourceBranch = payload?.sourceBranch; + if (!Number.isInteger(payloadPr)) { + throw new Error(`Invalid PR number in artifact payload: ${payloadPr}`); + } + if (payloadPr <= 0) { + throw new Error(`Invalid PR number in artifact payload: ${payloadPr}`); + } + const issueNumber = payloadPr; + const runPrNumber = context.payload.workflow_run?.pull_requests?.[0]?.number; + if (Number.isInteger(runPrNumber) && runPrNumber !== issueNumber) { + throw new Error( + `PR number mismatch between workflow_run (${runPrNumber}) and artifact payload (${issueNumber})`, + ); + } + + if (typeof url !== "string" || url.trim() !== url || /[\u0000-\u001F\u007F]/.test(url)) { + throw new Error(`Invalid package URL in payload: ${url}`); + } + let parsedUrl; + try { + parsedUrl = new URL(url); + } catch { + throw new Error(`Invalid package URL in payload: ${url}`); + } + if (parsedUrl.protocol !== "https:" || parsedUrl.hostname !== "pkg.pr.new") { + throw new Error(`Invalid package URL in payload: ${url}`); + } + + const safeRepoPattern = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + const safeBranchPattern = /^[A-Za-z0-9._\/-]+$/; + const hasSkillSource = + typeof sourceRepo === "string" && + typeof sourceBranch === "string" && + safeRepoPattern.test(sourceRepo) && + safeBranchPattern.test(sourceBranch); + const skillSection = hasSkillSource + ? [ + "", + "### ๐Ÿงฉ Skill update", + "", + "```bash", + `npx skills add ${sourceRepo}#${sourceBranch} -y -g`, + "```", + ] + : [ + "", + "### ๐Ÿงฉ Skill update", + "", + "_Unavailable for this PR because source repo/branch metadata is missing._", + ]; + + const body = [ + "", + "## ๐Ÿš€ PR Preview Install Guide", + "", + "### ๐Ÿงฐ CLI update", + "", + "```bash", + `npm i -g ${url}`, + "```", + ...skillSection, + ].join("\n"); + + const comments = await github.paginate(github.rest.issues.listComments, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueNumber, + per_page: 100, + }); + + const existing = comments.find((comment) => + comment.user?.login === "github-actions[bot]" && + typeof comment.body === "string" && + comment.body.includes("") + ); + + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueNumber, + body, + }); + } diff --git a/.github/workflows/pkg-pr-new.yml b/.github/workflows/pkg-pr-new.yml index ee07e71f..a6582fa5 100644 --- a/.github/workflows/pkg-pr-new.yml +++ b/.github/workflows/pkg-pr-new.yml @@ -7,7 +7,6 @@ on: permissions: contents: read - pull-requests: write jobs: publish: @@ -31,74 +30,42 @@ jobs: - name: Publish to pkg.pr.new run: npx pkg-pr-new publish --no-compact --json output.json --comment=off ./.pkg-pr-new - - name: Comment install command - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - const fs = require("fs"); - const output = JSON.parse(fs.readFileSync("output.json", "utf8")); - const url = output?.packages?.[0]?.url; - if (!url) { - throw new Error("No package URL found in output.json"); - } - const sourceRepo = context.payload.pull_request?.head?.repo?.full_name; - const sourceBranch = context.payload.pull_request?.head?.ref; - const hasSkillSource = Boolean(sourceRepo && sourceBranch); + - name: Build comment payload + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + SOURCE_REPO: ${{ github.event.pull_request.head.repo.full_name }} + SOURCE_BRANCH: ${{ github.event.pull_request.head.ref }} + run: | + node <<'NODE' + const fs = require("fs"); - const skillSection = hasSkillSource - ? [ - "", - "### ๐Ÿงฉ Skill update", - "", - "```bash", - `npx skills add ${sourceRepo}#${sourceBranch} -y -g`, - "```", - ] - : [ - "", - "### ๐Ÿงฉ Skill update", - "", - "_Unavailable for this PR because source repo/branch metadata is missing._", - ]; + const output = JSON.parse(fs.readFileSync("output.json", "utf8")); + const url = output?.packages?.[0]?.url; + if (!url) throw new Error("No package URL found in output.json"); + if (!url.startsWith("https://pkg.pr.new/")) { + throw new Error(`Unexpected package URL: ${url}`); + } - const body = [ - "", - "## ๐Ÿš€ PR Preview Install Guide", - "", - "### ๐Ÿงฐ CLI update", - "", - "```bash", - `npm i -g ${url}`, - "```", - ...skillSection, - ].join("\n"); - const issueNumber = context.issue.number; + const pr = Number(process.env.PR_NUMBER); + if (!Number.isInteger(pr) || pr <= 0) { + throw new Error(`Invalid PR_NUMBER: ${process.env.PR_NUMBER}`); + } - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: issueNumber, - per_page: 100, - }); + const payload = { + pr, + url, + sourceRepo: process.env.SOURCE_REPO || "", + sourceBranch: process.env.SOURCE_BRANCH || "", + }; - const existing = comments.find((comment) => - comment.user?.login === "github-actions[bot]" && - typeof comment.body === "string" && - comment.body.includes("") - ); + fs.writeFileSync( + "pkg-pr-new-comment-payload.json", + JSON.stringify(payload), + ); + NODE - if (existing) { - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - body, - }); - } else { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: issueNumber, - body, - }); - } + - name: Upload comment payload + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: pkg-pr-new-comment-payload + path: pkg-pr-new-comment-payload.json From 4c51a9874dc9fdc594f9d9db402a95211bc03357 Mon Sep 17 00:00:00 2001 From: JackZhao10086 Date: Wed, 1 Apr 2026 13:58:47 +0800 Subject: [PATCH 2/8] fix: correct URL formatting in login --no-wait output (#169) * fix: Fix the issue where the URL returned by the "lark-cli auth login --no-wait" command contains \u0026 * style: fix indentation and whitespace in error handling code * fix(auth): handle JSON encoding errors in login output * docs(cmd/auth): add comment for authLoginRun function --- cmd/auth/login.go | 21 +++++++++++++++------ cmd/root.go | 11 +++++++++-- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 572965d3..ebd744f2 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -90,6 +90,7 @@ func completeDomain(toComplete string) []string { return completions } +// authLoginRun executes the login command logic. func authLoginRun(opts *LoginOptions) error { f := opts.Factory @@ -225,26 +226,34 @@ func authLoginRun(opts *LoginOptions) error { // --no-wait: return immediately with device code and URL if opts.NoWait { - b, _ := json.Marshal(map[string]interface{}{ + data := map[string]interface{}{ "verification_url": authResp.VerificationUriComplete, "device_code": authResp.DeviceCode, "expires_in": authResp.ExpiresIn, "hint": fmt.Sprintf("Show verification_url to user, then immediately execute: lark-cli auth login --device-code %s (blocks until authorized or timeout). Do not instruct the user to run this command themselves.", authResp.DeviceCode), - }) - fmt.Fprintln(f.IOStreams.Out, string(b)) + } + encoder := json.NewEncoder(f.IOStreams.Out) + encoder.SetEscapeHTML(false) + if err := encoder.Encode(data); err != nil { + fmt.Fprintf(f.IOStreams.ErrOut, "error: failed to write JSON output: %v\n", err) + } return nil } // Step 2: Show user code and verification URL if opts.JSON { - b, _ := json.Marshal(map[string]interface{}{ + data := map[string]interface{}{ "event": "device_authorization", "verification_uri": authResp.VerificationUri, "verification_uri_complete": authResp.VerificationUriComplete, "user_code": authResp.UserCode, "expires_in": authResp.ExpiresIn, - }) - fmt.Fprintln(f.IOStreams.Out, string(b)) + } + encoder := json.NewEncoder(f.IOStreams.Out) + encoder.SetEscapeHTML(false) + if err := encoder.Encode(data); err != nil { + fmt.Fprintf(f.IOStreams.ErrOut, "error: failed to write JSON output: %v\n", err) + } } else { fmt.Fprintf(f.IOStreams.ErrOut, msg.OpenURL) fmt.Fprintf(f.IOStreams.ErrOut, " %s\n\n", authResp.VerificationUriComplete) diff --git a/cmd/root.go b/cmd/root.go index 4a6daf9f..6168dbde 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -4,6 +4,7 @@ package cmd import ( + "bytes" "encoding/json" "errors" "fmt" @@ -241,12 +242,18 @@ func writeSecurityPolicyError(w io.Writer, spErr *internalauth.SecurityPolicyErr } env := map[string]interface{}{"ok": false, "error": errData} - b, err := json.MarshalIndent(env, "", " ") + + buffer := &bytes.Buffer{} + encoder := json.NewEncoder(buffer) + encoder.SetEscapeHTML(false) + encoder.SetIndent("", " ") + err := encoder.Encode(env) + if err != nil { fmt.Fprintln(w, `{"ok":false,"error":{"type":"internal_error","code":"marshal_error","message":"failed to marshal error"}}`) return } - fmt.Fprintln(w, string(b)) + fmt.Fprint(w, buffer.String()) } // installTipsHelpFunc wraps the default help function to append a TIPS section From d4e83df22c3dd51153b1d7af37c509bf278fc645 Mon Sep 17 00:00:00 2001 From: kongenpei Date: Wed, 1 Apr 2026 15:27:08 +0800 Subject: [PATCH 3/8] chore: add pull request template (#176) * add pull request template * fix: use safe related issue placeholder in PR template --------- Co-authored-by: kongenpei --- .github/pull_request_template.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 00000000..2ba1ecad --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,16 @@ +## Summary + + +## Changes + +- Change 1 +- Change 2 + +## Test Plan + +- [ ] Unit tests pass +- [ ] Manual local verification confirms the `lark xxx` command works as expected + +## Related Issues + +- None From 70c72a2c028e2c254ca741c11cf741422ab6849d Mon Sep 17 00:00:00 2001 From: feng zhi hao Date: Wed, 1 Apr 2026 15:47:20 +0800 Subject: [PATCH 4/8] feat(mail): auto-resolve local image paths in draft body HTML (#81) (#139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(mail): auto-resolve local image paths in draft body HTML (#81) Allow in set_body/set_reply_body HTML. Local file paths are automatically resolved into inline MIME parts with generated CIDs, eliminating the need to manually pair add_inline with set_body. Removing or replacing an tag in the body automatically cleans up or replaces the corresponding MIME inline part. - Add postProcessInlineImages to unify resolve, validate, and orphan cleanup into a single post-processing step - Extract loadAndAttachInline shared helper to deduplicate addInline and resolveLocalImgSrc logic - Cache resolved paths so the same file is only attached once - Use whitelist URI scheme detection instead of blacklist - Remove dead validateInlineCIDAfterApply and validateOrphanedInlineCIDAfterApply functions Closes #81 * fix(mail): harden inline image CID handling 1. Fix imgSrcRegexp to skip attribute names like data-src/x-src that contain "src" as a suffix โ€” only match the real src attribute. 2. Sanitize cidFromFileName to replace whitespace with hyphens, producing RFC-safe CID tokens (e.g. "my logo.png" โ†’ "my-logo"). 3. Add CID validation in newInlinePart to reject spaces, tabs, angle brackets, and parentheses โ€” fail fast instead of silently producing broken inline images in the sent email. * refactor(mail): use UUID for auto-generated inline CIDs Replace filename-derived CID generation (cidFromFileName + uniqueCID) with UUID-based generation. UUIDs contain only [0-9a-f-] characters, eliminating all RFC compliance risks from special characters, Unicode, or filename collisions. Same-file deduplication via pathToCID cache is preserved โ€” multiple tags referencing the same file still share one MIME part and one CID. * fix(mail): avoid panic in generateCID by using uuid.NewRandom uuid.New() calls Must(NewRandom()) which panics if the random source fails. Replace with uuid.NewRandom() and propagate the error through resolveLocalImgSrc, so the CLI returns a clear error instead of crashing in extreme environments. * fix(mail): restore quote block hint in set_reply_body template description The auto-resolve PR accidentally dropped "the quote block is re-appended automatically" from the set_reply_body shape description. Restore it alongside the new local-path support note. * fix(mail): add orphan invariant comment and expand regex test coverage - Add comment in postProcessInlineImages explaining that partially attached inline parts on error are cleaned up by the next Apply. - Add regex test cases: single-quoted src, multiple spaces before src, and newline before src. * fix(mail): use consistent inline predicate and safer HTML part lookup 1. removeOrphanedInlineParts: change condition from ContentDisposition=="inline" && ContentID!="" to isInlinePart(child) && ContentID!="", matching the predicate used elsewhere โ€” parts with only a ContentID (no Content-Disposition) are now correctly cleaned up. 2. postProcessInlineImages: use findPrimaryBodyPart instead of findPart(snapshot.Body, PrimaryHTMLPartID) to avoid stale PartID after ops restructure the MIME tree. * fix(mail): revert orphan cleanup to ContentDisposition check to protect HTML body The previous change (d3d1982) broadened the orphan cleanup predicate to isInlinePart(), which treats any part with a ContentID as inline. This deletes the primary HTML body when it carries a Content-ID header (valid in multipart/related), even on metadata-only edits like set_subject. Revert to the original ContentDisposition=="inline" && ContentID!="" condition โ€” only parts explicitly marked as inline attachments are candidates for orphan removal. Add regression test covering multipart/related with a Content-ID-bearing HTML body. --- shortcuts/mail/draft/patch.go | 216 +++-- .../mail/draft/patch_inline_resolve_test.go | 773 ++++++++++++++++++ shortcuts/mail/draft/patch_test.go | 14 +- shortcuts/mail/mail_draft_edit.go | 15 +- .../references/lark-mail-draft-edit.md | 28 +- 5 files changed, 969 insertions(+), 77 deletions(-) create mode 100644 shortcuts/mail/draft/patch_inline_resolve_test.go diff --git a/shortcuts/mail/draft/patch.go b/shortcuts/mail/draft/patch.go index 3fc6d3dd..e2c57024 100644 --- a/shortcuts/mail/draft/patch.go +++ b/shortcuts/mail/draft/patch.go @@ -8,12 +8,18 @@ import ( "mime" "os" "path/filepath" + "regexp" "strings" + "github.com/google/uuid" "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/shortcuts/mail/filecheck" ) +// imgSrcRegexp matches and captures the src value. +// It handles both single and double quotes. +var imgSrcRegexp = regexp.MustCompile(`(?i)]*?\s)?src\s*=\s*["']([^"']+)["']`) + var protectedHeaders = map[string]bool{ "message-id": true, "mime-version": true, @@ -33,13 +39,10 @@ func Apply(snapshot *DraftSnapshot, patch Patch) error { return err } } - if err := refreshSnapshot(snapshot); err != nil { - return err - } - if err := validateInlineCIDAfterApply(snapshot); err != nil { + if err := postProcessInlineImages(snapshot); err != nil { return err } - return validateOrphanedInlineCIDAfterApply(snapshot) + return refreshSnapshot(snapshot) } func applyOp(snapshot *DraftSnapshot, op PatchOp, options PatchOptions) error { @@ -523,21 +526,25 @@ func addAttachment(snapshot *DraftSnapshot, path string) error { return nil } -func addInline(snapshot *DraftSnapshot, path, cid, fileName, contentType string) error { +// loadAndAttachInline reads a local image file, validates its format, +// creates a MIME inline part, and attaches it to the snapshot's +// multipart/related container. If container is non-nil it is reused; +// otherwise the container is resolved from the snapshot. +func loadAndAttachInline(snapshot *DraftSnapshot, path, cid, fileName string, container *Part) (*Part, error) { safePath, err := validate.SafeInputPath(path) if err != nil { - return fmt.Errorf("inline image %q: %w", path, err) + return nil, fmt.Errorf("inline image %q: %w", path, err) } info, err := os.Stat(safePath) if err != nil { - return err + return nil, fmt.Errorf("inline image %q: %w", path, err) } if err := checkSnapshotAttachmentLimit(snapshot, info.Size(), nil); err != nil { - return err + return nil, err } content, err := os.ReadFile(safePath) if err != nil { - return err + return nil, fmt.Errorf("inline image %q: %w", path, err) } name := fileName if strings.TrimSpace(name) == "" { @@ -545,23 +552,30 @@ func addInline(snapshot *DraftSnapshot, path, cid, fileName, contentType string) } detectedCT, err := filecheck.CheckInlineImageFormat(name, content) if err != nil { - return err + return nil, fmt.Errorf("inline image %q: %w", path, err) } - inline, err := newInlinePart(path, content, cid, fileName, detectedCT) + inline, err := newInlinePart(safePath, content, cid, name, detectedCT) if err != nil { - return err + return nil, fmt.Errorf("inline image %q: %w", path, err) } - containerRef := primaryBodyRootRef(&snapshot.Body) - if containerRef == nil || *containerRef == nil { - return fmt.Errorf("draft has no primary body container") - } - container, err := ensureInlineContainerRef(containerRef) - if err != nil { - return err + if container == nil { + containerRef := primaryBodyRootRef(&snapshot.Body) + if containerRef == nil || *containerRef == nil { + return nil, fmt.Errorf("draft has no primary body container") + } + container, err = ensureInlineContainerRef(containerRef) + if err != nil { + return nil, fmt.Errorf("inline image %q: %w", path, err) + } } container.Children = append(container.Children, inline) container.Dirty = true - return nil + return container, nil +} + +func addInline(snapshot *DraftSnapshot, path, cid, fileName, contentType string) error { + _, err := loadAndAttachInline(snapshot, path, cid, fileName, nil) + return err } func replaceInline(snapshot *DraftSnapshot, partID, path, cid, fileName, contentType string) error { @@ -762,6 +776,9 @@ func newInlinePart(path string, content []byte, cid, fileName, contentType strin if err := validate.RejectCRLF(cid, "inline cid"); err != nil { return nil, err } + if strings.ContainsAny(cid, " \t<>()") { + return nil, fmt.Errorf("inline cid %q contains invalid characters (spaces, tabs, angle brackets, or parentheses are not allowed)", cid) + } if err := validate.RejectCRLF(fileName, "inline filename"); err != nil { return nil, err } @@ -857,59 +874,152 @@ func removeHeader(headers *[]Header, name string) { *headers = next } -// validateInlineCIDAfterApply checks that all CID references in the HTML body -// resolve to actual inline MIME parts. This is called after Apply (editing) to -// prevent broken CID references, but NOT during Parse (where broken CIDs -// should not block opening the draft). -func validateInlineCIDAfterApply(snapshot *DraftSnapshot) error { - htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) - if htmlPart == nil { - return nil +// uriSchemeRegexp matches a URI scheme (RFC 3986: ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) ":"). +var uriSchemeRegexp = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9+.\-]*:`) + +// isLocalFileSrc returns true if src is a local file path. +// Any URI with a scheme (http:, cid:, data:, ftp:, blob:, file:, etc.) +// or protocol-relative URL (//host/...) is rejected. +func isLocalFileSrc(src string) bool { + trimmed := strings.TrimSpace(src) + if trimmed == "" { + return false } - refs := extractCIDRefs(string(htmlPart.Body)) - if len(refs) == 0 { - return nil + if strings.HasPrefix(trimmed, "//") { + return false } - cids := make(map[string]bool) - for _, part := range flattenParts(snapshot.Body) { - if part == nil || part.ContentID == "" { + return !uriSchemeRegexp.MatchString(trimmed) +} + +// generateCID returns a random UUID string suitable for use as a Content-ID. +// UUIDs contain only [0-9a-f-], which is inherently RFC-safe and unique, +// avoiding all filename-derived encoding/collision issues. +func generateCID() (string, error) { + id, err := uuid.NewRandom() + if err != nil { + return "", fmt.Errorf("failed to generate CID: %w", err) + } + return id.String(), nil +} + +// resolveLocalImgSrc scans HTML for references, +// creates MIME inline parts for each local file, and returns the HTML +// with those src attributes replaced by cid: URIs. +func resolveLocalImgSrc(snapshot *DraftSnapshot, html string) (string, error) { + matches := imgSrcRegexp.FindAllStringSubmatchIndex(html, -1) + if len(matches) == 0 { + return html, nil + } + + var container *Part + // Cache resolved paths so the same file is only attached once. + pathToCID := make(map[string]string) + + // Iterate in reverse so that index offsets remain valid after replacement. + for i := len(matches) - 1; i >= 0; i-- { + srcStart, srcEnd := matches[i][2], matches[i][3] + src := html[srcStart:srcEnd] + if !isLocalFileSrc(src) { continue } - cids[strings.ToLower(part.ContentID)] = true + + resolvedPath, err := validate.SafeInputPath(src) + if err != nil { + return "", fmt.Errorf("inline image %q: %w", src, err) + } + + cid, ok := pathToCID[resolvedPath] + if !ok { + fileName := filepath.Base(src) + cid, err = generateCID() + if err != nil { + return "", err + } + pathToCID[resolvedPath] = cid + + container, err = loadAndAttachInline(snapshot, src, cid, fileName, container) + if err != nil { + return "", err + } + } + + html = html[:srcStart] + "cid:" + cid + html[srcEnd:] } - for _, ref := range refs { - if !cids[strings.ToLower(ref)] { - return fmt.Errorf("html body references missing inline cid %q", ref) + + return html, nil +} + +// removeOrphanedInlineParts removes inline MIME parts whose ContentID +// is not in the referencedCIDs set from all multipart/related containers. +func removeOrphanedInlineParts(root *Part, referencedCIDs map[string]bool) { + if root == nil { + return + } + if !strings.EqualFold(root.MediaType, "multipart/related") { + for _, child := range root.Children { + removeOrphanedInlineParts(child, referencedCIDs) } + return } - return nil + kept := make([]*Part, 0, len(root.Children)) + for _, child := range root.Children { + if child == nil { + continue + } + if strings.EqualFold(child.ContentDisposition, "inline") && child.ContentID != "" { + if !referencedCIDs[strings.ToLower(child.ContentID)] { + root.Dirty = true + continue + } + } + kept = append(kept, child) + } + root.Children = kept } -// validateOrphanedInlineCIDAfterApply checks the reverse direction: every -// inline MIME part with a ContentID must be referenced by the HTML body. -// An orphaned inline part (CID exists but HTML has no ) will -// be displayed as an unexpected attachment by most mail clients. -func validateOrphanedInlineCIDAfterApply(snapshot *DraftSnapshot) error { - htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) +// postProcessInlineImages is the unified post-processing step that: +// 1. Resolves local to inline CID parts. +// 2. Validates all CID references in HTML resolve to MIME parts. +// 3. Removes orphaned inline MIME parts no longer referenced by HTML. +func postProcessInlineImages(snapshot *DraftSnapshot) error { + htmlPart := findPrimaryBodyPart(snapshot.Body, "text/html") if htmlPart == nil { return nil } - refs := extractCIDRefs(string(htmlPart.Body)) + + origHTML := string(htmlPart.Body) + // Note: if resolveLocalImgSrc returns an error after partially attaching + // inline parts to the snapshot, those parts are orphaned but will be + // cleaned up by removeOrphanedInlineParts on the next successful Apply. + html, err := resolveLocalImgSrc(snapshot, origHTML) + if err != nil { + return err + } + if html != origHTML { + htmlPart.Body = []byte(html) + htmlPart.Dirty = true + } + + refs := extractCIDRefs(html) refSet := make(map[string]bool, len(refs)) for _, ref := range refs { refSet[strings.ToLower(ref)] = true } - var orphaned []string + + cidParts := make(map[string]bool) for _, part := range flattenParts(snapshot.Body) { if part == nil || part.ContentID == "" { continue } - if !refSet[strings.ToLower(part.ContentID)] { - orphaned = append(orphaned, part.ContentID) - } + cidParts[strings.ToLower(part.ContentID)] = true } - if len(orphaned) > 0 { - return fmt.Errorf("inline MIME parts have no reference in the HTML body and will appear as unexpected attachments: orphaned cids %v; if you used set_body, make sure the new body preserves all existing cid:... references", orphaned) + + for _, ref := range refs { + if !cidParts[strings.ToLower(ref)] { + return fmt.Errorf("html body references missing inline cid %q", ref) + } } + + removeOrphanedInlineParts(snapshot.Body, refSet) return nil } diff --git a/shortcuts/mail/draft/patch_inline_resolve_test.go b/shortcuts/mail/draft/patch_inline_resolve_test.go new file mode 100644 index 00000000..7c43886a --- /dev/null +++ b/shortcuts/mail/draft/patch_inline_resolve_test.go @@ -0,0 +1,773 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package draft + +import ( + "os" + "regexp" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” basic auto-resolve +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcBasic(t *testing.T) { + chdirTemp(t) + os.WriteFile("logo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
Hello
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `
Hello
`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + if htmlPart == nil { + t.Fatal("HTML part not found") + } + body := string(htmlPart.Body) + if strings.Contains(body, "./logo.png") { + t.Fatal("local path should have been replaced") + } + // Extract the generated CID from the HTML body. + cidRe := regexp.MustCompile(`src="cid:([^"]+)"`) + m := cidRe.FindStringSubmatch(body) + if m == nil { + t.Fatalf("expected src to contain a cid: reference, got: %s", body) + } + cid := m[1] + // Verify MIME inline part was created with the matching CID. + found := false + for _, part := range flattenParts(snapshot.Body) { + if part != nil && part.ContentID == cid { + found = true + if part.MediaType != "image/png" { + t.Fatalf("expected image/png, got %q", part.MediaType) + } + } + } + if !found { + t.Fatalf("expected inline MIME part with CID %q to be created", cid) + } +} + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” multiple images +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcMultipleImages(t *testing.T) { + chdirTemp(t) + os.WriteFile("a.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + os.WriteFile("b.jpg", []byte{0xFF, 0xD8, 0xFF, 0xE0}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `
`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + body := string(htmlPart.Body) + cidRe := regexp.MustCompile(`src="cid:([^"]+)"`) + matches := cidRe.FindAllStringSubmatch(body, -1) + if len(matches) != 2 { + t.Fatalf("expected 2 cid: references, got %d in: %s", len(matches), body) + } + if matches[0][1] == matches[1][1] { + t.Fatalf("expected different CIDs for different files, both got: %s", matches[0][1]) + } +} + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” skips cid/http/data URIs +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcSkipsNonLocalSrc(t *testing.T) { + chdirTemp(t) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 + +
+--rel +Content-Type: image/png; name=existing.png +Content-Disposition: inline; filename=existing.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + originalBody := string(htmlPart.Body) + + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: originalBody}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart = findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + if string(htmlPart.Body) != originalBody { + t.Fatalf("body should be unchanged, got: %s", string(htmlPart.Body)) + } +} + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” duplicate file names get unique CIDs +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcDuplicateCID(t *testing.T) { + chdirTemp(t) + os.MkdirAll("sub", 0o755) + os.WriteFile("logo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + os.WriteFile("sub/logo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `
`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + body := string(htmlPart.Body) + cidRe := regexp.MustCompile(`src="cid:([^"]+)"`) + matches := cidRe.FindAllStringSubmatch(body, -1) + if len(matches) != 2 { + t.Fatalf("expected 2 cid: references, got %d in: %s", len(matches), body) + } + if matches[0][1] == matches[1][1] { + t.Fatalf("expected different CIDs for different files, both got: %s", matches[0][1]) + } +} + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” same file referenced multiple times reuses one CID +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcSameFileReused(t *testing.T) { + chdirTemp(t) + os.WriteFile("logo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `

text

`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + body := string(htmlPart.Body) + // Both references should resolve to the same CID. + cidRe := regexp.MustCompile(`src="cid:([^"]+)"`) + matches := cidRe.FindAllStringSubmatch(body, -1) + if len(matches) != 2 { + t.Fatalf("expected 2 cid: references, got %d in: %s", len(matches), body) + } + if matches[0][1] != matches[1][1] { + t.Fatalf("expected same CID reused, got %q and %q", matches[0][1], matches[1][1]) + } + // Count inline MIME parts โ€” should be exactly 1. + var count int + for _, part := range flattenParts(snapshot.Body) { + if part != nil && strings.EqualFold(part.ContentDisposition, "inline") { + count++ + } + } + if count != 1 { + t.Fatalf("expected 1 inline part (reused), got %d", count) + } +} + +// --------------------------------------------------------------------------- +// resolveLocalImgSrc โ€” non-image format rejected +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcRejectsNonImage(t *testing.T) { + chdirTemp(t) + os.WriteFile("doc.txt", []byte("not an image"), 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `
`}}, + }) + if err == nil { + t.Fatal("expected error for non-image file") + } +} + +// --------------------------------------------------------------------------- +// orphan cleanup โ€” delete inline image by removing from body +// --------------------------------------------------------------------------- + +func TestOrphanCleanupOnImgRemoval(t *testing.T) { + snapshot := mustParseFixtureDraft(t, `Subject: Inline +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 + +
hello
+--rel +Content-Type: image/png; name=logo.png +Content-Disposition: inline; filename=logo.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + // Remove the tag from body. + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: "
hello
"}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + for _, part := range flattenParts(snapshot.Body) { + if part != nil && part.ContentID == "logo" { + t.Fatal("expected orphaned inline part 'logo' to be removed") + } + } +} + +// --------------------------------------------------------------------------- +// orphan cleanup โ€” replace inline image +// --------------------------------------------------------------------------- + +func TestOrphanCleanupOnImgReplace(t *testing.T) { + chdirTemp(t) + os.WriteFile("new.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Inline +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 + +
+--rel +Content-Type: image/png; name=old.png +Content-Disposition: inline; filename=old.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + // Replace old image reference with a new local file. + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: `
`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + var foundOld bool + var newInlineCount int + for _, part := range flattenParts(snapshot.Body) { + if part == nil { + continue + } + if part.ContentID == "old" { + foundOld = true + } + if strings.EqualFold(part.ContentDisposition, "inline") && part.ContentID != "" && part.ContentID != "old" { + newInlineCount++ + } + } + if foundOld { + t.Fatal("expected old inline part to be removed") + } + if newInlineCount != 1 { + t.Fatalf("expected 1 new inline part, got %d", newInlineCount) + } +} + +// --------------------------------------------------------------------------- +// set_reply_body โ€” local path resolved, quote block preserved +// --------------------------------------------------------------------------- + +func TestSetReplyBodyResolvesLocalImgSrc(t *testing.T) { + chdirTemp(t) + os.WriteFile("photo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Re: Hello +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
original reply
quoted text
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_reply_body", Value: `
new reply
`}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + if htmlPart == nil { + t.Fatal("HTML part not found") + } + body := string(htmlPart.Body) + if strings.Contains(body, "./photo.png") { + t.Fatal("local path should have been replaced") + } + cidRe := regexp.MustCompile(`src="cid:([^"]+)"`) + m := cidRe.FindStringSubmatch(body) + if m == nil { + t.Fatalf("expected cid: reference in body, got: %s", body) + } + if !strings.Contains(body, "history-quote-wrapper") { + t.Fatalf("expected quote block preserved, got: %s", body) + } + found := false + for _, part := range flattenParts(snapshot.Body) { + if part != nil && part.ContentID == m[1] { + found = true + } + } + if !found { + t.Fatalf("expected inline MIME part with CID %q to be created", m[1]) + } +} + +// --------------------------------------------------------------------------- +// mixed usage โ€” add_inline + local path in body +// --------------------------------------------------------------------------- + +func TestMixedAddInlineAndLocalPath(t *testing.T) { + chdirTemp(t) + os.WriteFile("a.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + os.WriteFile("b.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{ + {Op: "add_inline", Path: "a.png", CID: "a"}, + {Op: "set_body", Value: `
`}, + }, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + var foundA bool + var autoResolvedCount int + for _, part := range flattenParts(snapshot.Body) { + if part == nil { + continue + } + if part.ContentID == "a" { + foundA = true + } else if strings.EqualFold(part.ContentDisposition, "inline") && part.ContentID != "" { + autoResolvedCount++ + } + } + if !foundA { + t.Fatal("expected inline part 'a' from add_inline") + } + if autoResolvedCount != 1 { + t.Fatalf("expected 1 auto-resolved inline part for b.png, got %d", autoResolvedCount) + } +} + +// --------------------------------------------------------------------------- +// conflict: add_inline same file + body local path โ†’ redundant part cleaned +// --------------------------------------------------------------------------- + +func TestAddInlineSameFileAsLocalPath(t *testing.T) { + chdirTemp(t) + os.WriteFile("logo.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/html; charset=UTF-8 + +
empty
+`) + // add_inline creates CID "logo", but body uses local path instead of cid:logo. + // resolve generates a UUID CID, orphan cleanup removes the unused "logo". + err := Apply(snapshot, Patch{ + Ops: []PatchOp{ + {Op: "add_inline", Path: "logo.png", CID: "logo"}, + {Op: "set_body", Value: `
`}, + }, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + // The explicitly added "logo" CID is orphaned (not referenced in HTML) + // and should be auto-removed. Only the auto-generated CID remains. + var foundLogo bool + var count int + for _, part := range flattenParts(snapshot.Body) { + if part != nil && strings.EqualFold(part.ContentDisposition, "inline") { + count++ + if part.ContentID == "logo" { + foundLogo = true + } + } + } + if foundLogo { + t.Fatal("expected orphaned 'logo' inline part to be removed") + } + if count != 1 { + t.Fatalf("expected 1 inline part after orphan cleanup, got %d", count) + } +} + +// --------------------------------------------------------------------------- +// conflict: remove_inline but body still references its CID โ†’ error +// --------------------------------------------------------------------------- + +func TestRemoveInlineButBodyStillReferencesCID(t *testing.T) { + snapshot := mustParseFixtureDraft(t, `Subject: Inline +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 + +
+--rel +Content-Type: image/png; name=logo.png +Content-Disposition: inline; filename=logo.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + // remove_inline removes the MIME part, but set_body still references cid:logo. + err := Apply(snapshot, Patch{ + Ops: []PatchOp{ + {Op: "remove_inline", Target: AttachmentTarget{CID: "logo"}}, + {Op: "set_body", Value: `
`}, + }, + }) + if err == nil || !strings.Contains(err.Error(), "missing inline cid") { + t.Fatalf("expected missing cid error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// conflict: remove_inline + body replaces with local path โ†’ works +// --------------------------------------------------------------------------- + +func TestRemoveInlineAndReplaceWithLocalPath(t *testing.T) { + chdirTemp(t) + os.WriteFile("new.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644) + + snapshot := mustParseFixtureDraft(t, `Subject: Inline +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 + +
+--rel +Content-Type: image/png; name=old.png +Content-Disposition: inline; filename=old.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{ + {Op: "remove_inline", Target: AttachmentTarget{CID: "old"}}, + {Op: "set_body", Value: `
`}, + }, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + var foundOld bool + var newInlineCount int + for _, part := range flattenParts(snapshot.Body) { + if part == nil { + continue + } + if part.ContentID == "old" { + foundOld = true + } + if strings.EqualFold(part.ContentDisposition, "inline") && part.ContentID != "" && part.ContentID != "old" { + newInlineCount++ + } + } + if foundOld { + t.Fatal("expected old inline part to be removed") + } + if newInlineCount != 1 { + t.Fatalf("expected 1 new inline part from local path resolve, got %d", newInlineCount) + } +} + +// --------------------------------------------------------------------------- +// no HTML body โ€” text/plain only draft +// --------------------------------------------------------------------------- + +func TestResolveLocalImgSrcNoHTMLBody(t *testing.T) { + snapshot := mustParseFixtureDraft(t, `Subject: Plain +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 + +Just plain text. +`) + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_body", Value: "Updated plain text."}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } +} + +// --------------------------------------------------------------------------- +// regression: HTML body with Content-ID must not be removed by orphan cleanup +// --------------------------------------------------------------------------- + +func TestOrphanCleanupPreservesHTMLBodyWithContentID(t *testing.T) { + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice +To: Bob +MIME-Version: 1.0 +Content-Type: multipart/related; boundary="rel" + +--rel +Content-Type: text/html; charset=UTF-8 +Content-ID: + +
hello world
+--rel +Content-Type: image/png; name=logo.png +Content-Disposition: inline; filename=logo.png +Content-ID: +Content-Transfer-Encoding: base64 + +cG5n +--rel-- +`) + // A metadata-only edit should not destroy the HTML body part even though + // its Content-ID is not referenced by any . + err := Apply(snapshot, Patch{ + Ops: []PatchOp{{Op: "set_subject", Value: "Updated subject"}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + htmlPart := findPrimaryBodyPart(snapshot.Body, "text/html") + if htmlPart == nil { + t.Fatal("HTML body part was deleted by orphan cleanup") + } + if !strings.Contains(string(htmlPart.Body), "hello world") { + t.Fatalf("HTML body content changed unexpectedly: %s", string(htmlPart.Body)) + } +} + +// --------------------------------------------------------------------------- +// helper unit tests +// --------------------------------------------------------------------------- + +func TestIsLocalFileSrc(t *testing.T) { + tests := []struct { + src string + want bool + }{ + {"./logo.png", true}, + {"../images/logo.png", true}, + {"logo.png", true}, + {"/absolute/path/logo.png", true}, + {"cid:logo", false}, + {"CID:logo", false}, + {"http://example.com/img.png", false}, + {"https://example.com/img.png", false}, + {"data:image/png;base64,abc", false}, + {"//cdn.example.com/a.png", false}, + {"blob:https://example.com/uuid", false}, + {"ftp://example.com/file.png", false}, + {"file:///local/file.png", false}, + {"mailto:test@example.com", false}, + {"", false}, + } + for _, tt := range tests { + if got := isLocalFileSrc(tt.src); got != tt.want { + t.Errorf("isLocalFileSrc(%q) = %v, want %v", tt.src, got, tt.want) + } + } +} + +func TestGenerateCID(t *testing.T) { + seen := make(map[string]bool) + for i := 0; i < 100; i++ { + cid, err := generateCID() + if err != nil { + t.Fatalf("generateCID() error = %v", err) + } + if cid == "" { + t.Fatal("generateCID() returned empty string") + } + if strings.ContainsAny(cid, " \t\r\n<>()") { + t.Fatalf("generateCID() returned CID with invalid characters: %q", cid) + } + if seen[cid] { + t.Fatalf("generateCID() returned duplicate CID: %q", cid) + } + seen[cid] = true + } +} + +// --------------------------------------------------------------------------- +// imgSrcRegexp โ€” must not match data-src or similar attribute names +// --------------------------------------------------------------------------- + +func TestImgSrcRegexpSkipsDataSrc(t *testing.T) { + tests := []struct { + name string + html string + want string // expected captured src value, empty if no match + }{ + { + name: "plain src", + html: ``, + want: "./logo.png", + }, + { + name: "src with alt before", + html: `pic`, + want: "./logo.png", + }, + { + name: "data-src before real src", + html: ``, + want: "./logo.png", + }, + { + name: "only data-src, no src", + html: ``, + want: "", + }, + { + name: "x-src before real src", + html: ``, + want: "./real.png", + }, + { + name: "single-quoted src", + html: ``, + want: "./logo.png", + }, + { + name: "multiple spaces before src", + html: ``, + want: "./logo.png", + }, + { + name: "newline before src", + html: "", + want: "./logo.png", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := imgSrcRegexp.FindStringSubmatch(tt.html) + got := "" + if len(matches) > 1 { + got = matches[1] + } + if got != tt.want { + t.Errorf("imgSrcRegexp on %q: got %q, want %q", tt.html, got, tt.want) + } + }) + } +} + +// --------------------------------------------------------------------------- +// newInlinePart โ€” rejects CIDs with spaces or other invalid characters +// --------------------------------------------------------------------------- + +func TestNewInlinePartRejectsInvalidCIDChars(t *testing.T) { + content := []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A} + for _, bad := range []string{"my logo", "a\tb", "cid", "cid(x)"} { + _, err := newInlinePart("test.png", content, bad, "test.png", "image/png") + if err == nil { + t.Errorf("expected error for CID %q, got nil", bad) + } + } + // Valid CIDs should pass. + for _, good := range []string{"logo", "my-logo", "img_01", "photo.2"} { + _, err := newInlinePart("test.png", content, good, "test.png", "image/png") + if err != nil { + t.Errorf("unexpected error for CID %q: %v", good, err) + } + } +} diff --git a/shortcuts/mail/draft/patch_test.go b/shortcuts/mail/draft/patch_test.go index 8572f503..e3c55a9e 100644 --- a/shortcuts/mail/draft/patch_test.go +++ b/shortcuts/mail/draft/patch_test.go @@ -460,7 +460,7 @@ func TestRemoveInlineFailsWhenHTMLStillReferencesCID(t *testing.T) { } } -func TestApplySetBodyOrphanedInlineCIDIsRejected(t *testing.T) { +func TestApplySetBodyOrphanedInlineCIDIsAutoRemoved(t *testing.T) { snapshot := mustParseFixtureDraft(t, `Subject: Inline From: Alice To: Bob @@ -480,12 +480,18 @@ Content-Transfer-Encoding: base64 cG5n --rel-- `) - // set_body that drops the existing cid:logo reference โ†’ logo becomes orphaned + // set_body that drops the existing cid:logo reference โ†’ logo is auto-removed err := Apply(snapshot, Patch{ Ops: []PatchOp{{Op: "set_body", Value: "
replaced body without cid reference
"}}, }) - if err == nil || !strings.Contains(err.Error(), "orphaned cids") { - t.Fatalf("expected orphaned cid error, got: %v", err) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + // The orphaned inline part should be removed from the MIME tree. + for _, part := range flattenParts(snapshot.Body) { + if part != nil && part.ContentID == "logo" { + t.Fatal("expected orphaned inline part 'logo' to be removed") + } } } diff --git a/shortcuts/mail/mail_draft_edit.go b/shortcuts/mail/mail_draft_edit.go index 99061b8b..17a38372 100644 --- a/shortcuts/mail/mail_draft_edit.go +++ b/shortcuts/mail/mail_draft_edit.go @@ -303,13 +303,13 @@ func buildDraftEditPatchTemplate() map[string]interface{} { {"op": "set_recipients", "shape": map[string]interface{}{"field": "to|cc|bcc", "addresses": []map[string]interface{}{{"address": "string", "name": "string(optional)"}}}}, {"op": "add_recipient", "shape": map[string]interface{}{"field": "to|cc|bcc", "address": "string", "name": "string(optional)"}}, {"op": "remove_recipient", "shape": map[string]interface{}{"field": "to|cc|bcc", "address": "string"}}, - {"op": "set_body", "shape": map[string]interface{}{"value": "string"}}, - {"op": "set_reply_body", "shape": map[string]interface{}{"value": "string (user-authored content only, WITHOUT the quote block; the quote block is re-appended automatically)"}}, + {"op": "set_body", "shape": map[string]interface{}{"value": "string (supports โ€” local paths auto-resolved to inline MIME parts)"}}, + {"op": "set_reply_body", "shape": map[string]interface{}{"value": "string (user-authored content only, WITHOUT the quote block; the quote block is re-appended automatically; supports โ€” local paths auto-resolved to inline MIME parts)"}}, {"op": "set_header", "shape": map[string]interface{}{"name": "string", "value": "string"}}, {"op": "remove_header", "shape": map[string]interface{}{"name": "string"}}, {"op": "add_attachment", "shape": map[string]interface{}{"path": "string(relative path)"}}, {"op": "remove_attachment", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}}}, - {"op": "add_inline", "shape": map[string]interface{}{"path": "string(relative path)", "cid": "string", "filename": "string(optional)", "content_type": "string(optional)"}}, + {"op": "add_inline", "shape": map[string]interface{}{"path": "string(relative path)", "cid": "string", "filename": "string(optional)", "content_type": "string(optional)"}, "note": "advanced: prefer in set_body/set_reply_body instead"}, {"op": "replace_inline", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}, "path": "string(relative path)", "cid": "string(optional)", "filename": "string(optional)", "content_type": "string(optional)"}}, {"op": "remove_inline", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}}}, }, @@ -318,8 +318,8 @@ func buildDraftEditPatchTemplate() map[string]interface{} { "group": "subject_and_body", "ops": []map[string]interface{}{ {"op": "set_subject", "shape": map[string]interface{}{"value": "string"}}, - {"op": "set_body", "shape": map[string]interface{}{"value": "string"}}, - {"op": "set_reply_body", "shape": map[string]interface{}{"value": "string (user-authored content only, WITHOUT the quote block; the quote block is re-appended automatically)"}}, + {"op": "set_body", "shape": map[string]interface{}{"value": "string (supports โ€” local paths auto-resolved to inline MIME parts)"}}, + {"op": "set_reply_body", "shape": map[string]interface{}{"value": "string (user-authored content only, WITHOUT the quote block; the quote block is re-appended automatically; supports โ€” local paths auto-resolved to inline MIME parts)"}}, }, }, { @@ -342,7 +342,7 @@ func buildDraftEditPatchTemplate() map[string]interface{} { "ops": []map[string]interface{}{ {"op": "add_attachment", "shape": map[string]interface{}{"path": "string(relative path)"}}, {"op": "remove_attachment", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}}}, - {"op": "add_inline", "shape": map[string]interface{}{"path": "string(relative path)", "cid": "string", "filename": "string(optional)", "content_type": "string(optional)"}}, + {"op": "add_inline", "shape": map[string]interface{}{"path": "string(relative path)", "cid": "string", "filename": "string(optional)", "content_type": "string(optional)"}, "note": "advanced: prefer in set_body/set_reply_body instead"}, {"op": "replace_inline", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}, "path": "string(relative path)", "cid": "string(optional)", "filename": "string(optional)", "content_type": "string(optional)"}}, {"op": "remove_inline", "shape": map[string]interface{}{"target": map[string]interface{}{"part_id": "string(optional)", "cid": "string(optional)"}}}, }, @@ -359,12 +359,13 @@ func buildDraftEditPatchTemplate() map[string]interface{} { {"situation": "draft created by +reply or +forward (has_quoted_content=true)", "recommended_op": "set_reply_body โ€” replaces only the user-authored portion and automatically preserves the quoted original message; if user explicitly wants to remove the quote, use set_body instead"}, }, "notes": []string{ + "`set_body`/`set_reply_body` support inline images via local file paths: use in the HTML value โ€” the local path is automatically resolved into an inline MIME part with a generated CID; removing or replacing an tag automatically cleans up or replaces the corresponding MIME part; do NOT use `add_inline` for this; example: {\"op\":\"set_body\",\"value\":\"
Hello
\"}", + "`add_inline` is an advanced op for precise CID control only โ€” in most cases, use in `set_body`/`set_reply_body` instead", "`ops` is executed in order", "all file paths (--patch-file and `path` fields in ops) must be relative โ€” no absolute paths or .. traversal", "all body edits MUST go through --patch-file; there is no --set-body flag", "`set_body` replaces the ENTIRE body including any reply/forward quote block; when the draft has both text/plain and text/html, it updates the HTML body and regenerates the plain-text summary, so the input should be HTML", "`set_reply_body` replaces only the user-authored portion of the body and automatically re-appends the trailing reply/forward quote block (generated by +reply or +forward); the value you pass should contain ONLY the new user-authored content WITHOUT the quote block โ€” the quote block will be re-inserted automatically; if the user wants to modify content INSIDE the quote block, use `set_body` instead for full replacement; if the draft has no quote block, it behaves identically to `set_body`", - "`add_inline` only adds the MIME binary part; it does NOT insert an tag into the HTML body; to display the image in the body, you must ALSO use set_body/set_reply_body to insert into the body content; forgetting this causes the inline part to become an orphaned attachment when sent", "`body_kind` only supports text/plain and text/html", "`selector` currently only supports primary", "`remove_attachment` target supports part_id or cid; priority: part_id > cid", diff --git a/skills/lark-mail/references/lark-mail-draft-edit.md b/skills/lark-mail/references/lark-mail-draft-edit.md index 6831700c..5cb58d29 100644 --- a/skills/lark-mail/references/lark-mail-draft-edit.md +++ b/skills/lark-mail/references/lark-mail-draft-edit.md @@ -198,9 +198,9 @@ lark-cli mail +draft-edit --draft-id --inspect { "op": "add_inline", "path": "./logo.png", "cid": "logo" } ``` -> **้‡่ฆ๏ผš`add_inline` ไป…ๆทปๅŠ  MIME ไบŒ่ฟ›ๅˆถ้ƒจๅˆ†๏ผŒไธไผšๅœจ HTML ๆญฃๆ–‡ไธญๆ’ๅ…ฅ `` ๆ ‡็ญพใ€‚** -> ๅฆ‚้œ€ๅ›พ็‰‡ๅœจ้‚ฎไปถๆญฃๆ–‡ไธญๅฏ่ง๏ผŒ**ๅฟ…้กป**ๅŒๆ—ถไฝฟ็”จ `set_body` ๆˆ– `set_reply_body` ๆ›ดๆ–ฐ HTML ๆญฃๆ–‡ๅนถๅŠ ๅ…ฅ `` ๆ ‡็ญพใ€‚ๅ‚่ง[ๅœจๆญฃๆ–‡ไธญๆ’ๅ…ฅๅ†…ๅตŒๅ›พ็‰‡](#ๅœจๆญฃๆ–‡ไธญๆ’ๅ…ฅๅ†…ๅตŒๅ›พ็‰‡)็š„ๅฎŒๆ•ดๆต็จ‹ใ€‚ -> ๅฆ‚ๆžœๅฟ˜่ฎฐๆทปๅŠ  `` ๅผ•็”จ๏ผŒ่ฏฅๅ†…ๅตŒ้ƒจๅˆ†ๅœจๅ‘้€ๆ—ถไผšๅ˜ๆˆๅญค็ซ‹้™„ไปถใ€‚ +> **ๆŽจ่ๆ–นๅผ๏ผš** ็›ดๆŽฅๅœจ `set_body`/`set_reply_body` ็š„ HTML ไธญไฝฟ็”จ ``๏ผˆๆœฌๅœฐๆ–‡ไปถ่ทฏๅพ„๏ผ‰๏ผŒ็ณป็ปŸไผš่‡ชๅŠจๅˆ›ๅปบ MIME ๅ†…ๅตŒ้ƒจๅˆ†ใ€็”Ÿๆˆ CID ๅนถๆ›ฟๆขไธบ `cid:` ๅผ•็”จใ€‚ๅˆ ้™คๆˆ–ๆ›ฟๆข `` ๆ ‡็ญพๆ—ถ๏ผŒๅฏนๅบ”็š„ MIME ้ƒจๅˆ†ไผš่‡ชๅŠจๆธ…็†ใ€‚่ฏฆ่ง[ๅœจๆญฃๆ–‡ไธญๆ’ๅ…ฅๅ†…ๅตŒๅ›พ็‰‡](#ๅœจๆญฃๆ–‡ไธญๆ’ๅ…ฅๅ†…ๅตŒๅ›พ็‰‡)ใ€‚ +> +> `add_inline` ไป…ๅœจ้œ€่ฆ็ฒพ็กฎๆŽงๅˆถ CID ๅ‘ฝๅๆ—ถไฝฟ็”จใ€‚ไฝฟ็”จๆ—ถไป้œ€ๅœจ HTML ๆญฃๆ–‡ไธญๅŠ ๅ…ฅ `` ๅผ•็”จใ€‚ `replace_inline` @@ -304,23 +304,18 @@ lark-cli mail +draft-edit --draft-id --patch-file ./patch.json ### ๅœจๆญฃๆ–‡ไธญๆ’ๅ…ฅๅ†…ๅตŒๅ›พ็‰‡ -ๆทปๅŠ ๅ†…ๅตŒๅ›พ็‰‡้œ€่ฆ**ไธคไธชๅๅŒ็ผ–่พ‘**๏ผš๏ผˆ1๏ผ‰้€š่ฟ‡ `add_inline` ๆทปๅŠ  MIME ้ƒจๅˆ†๏ผŒ๏ผˆ2๏ผ‰้€š่ฟ‡ `set_body` ๆˆ– `set_reply_body` ๅœจ HTML ๆญฃๆ–‡ไธญๆ’ๅ…ฅ `` ๆ ‡็ญพใ€‚ +็›ดๆŽฅๅœจ `set_body`/`set_reply_body` ็š„ HTML ไธญไฝฟ็”จๆœฌๅœฐๆ–‡ไปถ่ทฏๅพ„ๅณๅฏใ€‚็ณป็ปŸไผš่‡ชๅŠจๅˆ›ๅปบ MIME ๅ†…ๅตŒ้ƒจๅˆ†ๅนถๆ›ฟๆขไธบ `cid:` ๅผ•็”จใ€‚ ```bash -# 1. ๆŸฅ็œ‹่‰็จฟไปฅ่Žทๅ–ๅฝ“ๅ‰ HTML ๆญฃๆ–‡ๅ’Œๅทฒๆœ‰็š„ๅ†…ๅตŒ้ƒจๅˆ† +# 1. ๆŸฅ็œ‹่‰็จฟไปฅ่Žทๅ–ๅฝ“ๅ‰ HTML ๆญฃๆ–‡ lark-cli mail +draft-edit --draft-id --inspect -# ่ฟ”ๅ›žๅŒ…ๅซ๏ผš -# projection.body_html_summary: "
ๅŽŸๆœ‰ๅ†…ๅฎน
" -# projection.inline_summary: [{"part_id":"1.1.2","cid":"existing.png", ...}] -# 2. ็ผ–ๅ†™่กฅไธ๏ผˆๆณจๆ„๏ผšๅ›žๅค่‰็จฟ็”จ set_reply_body๏ผŒๆ™ฎ้€š่‰็จฟ็”จ set_body๏ผ‰ +# 2. ็ผ–ๅ†™่กฅไธ โ€” ็›ดๆŽฅไฝฟ็”จๆœฌๅœฐๆ–‡ไปถ่ทฏๅพ„๏ผˆๆณจๆ„๏ผšๅ›žๅค่‰็จฟ็”จ set_reply_body๏ผŒๆ™ฎ้€š่‰็จฟ็”จ set_body๏ผ‰ cat > ./patch.json << 'EOF' { "ops": [ - { "op": "set_body", "value": "
ๅŽŸๆœ‰ๅ†…ๅฎน
" }, - { "op": "add_inline", "path": "./new-image.png", "cid": "new-image" } - ], - "options": {} + { "op": "set_body", "value": "
ๅ†…ๅฎน
" } + ] } EOF @@ -328,6 +323,13 @@ EOF lark-cli mail +draft-edit --draft-id --patch-file ./patch.json ``` +ๅ†…ๅตŒๅ›พ็‰‡็š„ๅขžๅˆ ๆ”น้€š่ฟ‡ HTML ๆญฃๆ–‡่‡ชๅŠจ่”ๅŠจ๏ผš +- **ๆทปๅŠ **๏ผšๅœจ HTML ไธญๅ†™ ``๏ผŒ่‡ชๅŠจๅˆ›ๅปบ MIME ้ƒจๅˆ† +- **ๅˆ ้™ค**๏ผšไปŽ HTML ไธญ็งป้™ค `` ๆ ‡็ญพ๏ผŒๅฏนๅบ” MIME ้ƒจๅˆ†่‡ชๅŠจๆธ…็† +- **ๆ›ฟๆข**๏ผšๅฐ† `src` ๆ”นไธบๆ–ฐ็š„ๆœฌๅœฐ่ทฏๅพ„๏ผŒๆ—ง MIME ้ƒจๅˆ†่‡ชๅŠจ็งป้™คใ€ๆ–ฐ้ƒจๅˆ†่‡ชๅŠจๅˆ›ๅปบ + +> **้ซ˜็บง็”จๆณ•๏ผš** ้œ€่ฆ็ฒพ็กฎๆŽงๅˆถ CID ๅ‘ฝๅๆ—ถ๏ผŒไปๅฏไฝฟ็”จ `add_inline` ๆ‰‹ๅŠจๆทปๅŠ  MIME ้ƒจๅˆ†๏ผŒๅนถๅœจ HTML ไธญ็”จ `` ๅผ•็”จใ€‚ + ### ไฝฟ็”จ patch-file ่ฟ›่กŒ้ซ˜็บง็ผ–่พ‘ ```bash From 17698d5c6a9b0943eb0ef1cf9235b44834653415 Mon Sep 17 00:00:00 2001 From: kongenpei Date: Wed, 1 Apr 2026 16:00:55 +0800 Subject: [PATCH 5/8] docs: add concise AGENTS development guide (#178) * docs: add concise AGENTS development guide * docs: align AGENTS with toolchain and CI license checks * docs: remove toolchain prerequisite section --------- Co-authored-by: kongenpei --- AGENTS.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..e594a81c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,33 @@ +# AGENTS.md +Concise maintainer/developer guide for building, testing, and opening high-quality PRs in this repo. + +## Goal (pick one per PR) +- Make CLI better: improve UX, error messages, help text, flags, and output clarity. +- Improve reliability: fix bugs, edge cases, and regressions with tests. +- Improve developer velocity: simplify code paths, reduce complexity, keep behavior explicit. +- Improve quality gates: strengthen tests/lint/checks without adding heavy process. + +## Fast Dev Loop +1. `make build` (runs `python3 scripts/fetch_meta.py` first) +2. `make unit-test` (required before PR) +3. Run changed command(s) manually via `./lark-cli ...` + +## Pre-PR Checks (match CI gates) +1. `make unit-test` +2. `go mod tidy` (must not change `go.mod`/`go.sum`) +3. `go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main` +4. If dependencies changed: `go run github.com/google/go-licenses/v2@v2.0.1 check ./... --disallowed_types=forbidden,restricted,reciprocal,unknown` +5. Optional full local suite: `make test` (vet + unit + integration) + +## Test/Check Commands +- Unit: `make unit-test` +- Integration: `make integration-test` +- Full: `make test` +- Vet only: `make vet` +- Coverage (local): `go test -race -coverprofile=coverage.txt -covermode=atomic ./...` + +## Commit/PR Rules +- Use Conventional Commits in English: `feat: ...`, `fix: ...`, `docs: ...`, `ci: ...`, `test: ...`, `chore: ...`, `refactor: ...` +- Keep PR title in the same Conventional Commit format (squash merge keeps it). +- Before opening a real PR, draft/fill description from `.github/pull_request_template.md` and ensure Summary/Changes/Test Plan are complete. +- Never commit secrets/tokens/internal sensitive data. From 5621d2e555907b83e802b994319d16643c762fc6 Mon Sep 17 00:00:00 2001 From: williamfzc <178894043@qq.com> Date: Wed, 1 Apr 2026 17:45:39 +0800 Subject: [PATCH 6/8] feat(ci): refine PR business area labels and introduce skill format check (#148) * feat(ci): add PR size label pipeline * chore(ci): make PR label sync non-blocking * feat(ci): add dry-run mode for PR label sync * feat(ci): add PR label dry-run samples * test(ci): update PR label samples with real historical merged PRs Replaced synthetic or open PR samples with actual merged/closed PRs from the repository to provide a more accurate reflection of the size label categorization. Added 4 samples each for sizes S, M, and L covering docs, fixes, ci, and features. * feat(ci): add high-level area tags for PRs Based on user feedback, fine-grained domain labels (like `domain/base`) are too detailed for the early stages. This change adds support for applying `area/*` tags to indicate which important top-level modules a PR touches. Currently tracked areas: - `area/shortcuts` - `area/skills` - `area/cmd` Minor modules like docs, ci, and tests are intentionally excluded to keep tags focused on critical architectural components. * refactor(ci): extract pr-label-sync logic to a dedicated directory To avoid polluting the root `scripts/` directory, moved `sync_pr_labels.js` and `sync_pr_labels.samples.json` into a new `scripts/sync-pr-labels/` folder. Added a dedicated README to document its usage and behavior. Updated `.github/workflows/pr-labels.yml` to reflect the new path. * refactor(ci): rename pr label script directory for simplicity Renamed `scripts/sync-pr-labels/` to `scripts/pr-labels/` to keep directory names concise. Updated internal references and GitHub workflow files to point to the new path. * ci: add GitHub Actions workflow to check skill format * test(ci): update sample json to include expected_areas Added `expected_areas` lists to each sample in `samples.json` to reflect the newly added `area/*` high-level module tagging logic. Allows testing to accurately check both `size/*` and `area/*` outputs. * refactor(scripts): move skill format check to isolated directory and add README * test(scripts): add positive and negative tests for skill format check * fix(scripts): revert skill changes and downgrade version/metadata checks to warnings * fix(scripts): completely remove version check and skip lark-shared * refactor(ci): improve pr-labels script readability and maintainability - Reorganized code into logical sections with clear comments - Encapsulated GitHub API interactions into a reusable `GitHubClient` class - Extracted and centralized classification logic into a pure `evaluateRules` function - Replaced magic numbers with named constants (`THRESHOLD_L`, `THRESHOLD_XL`) - Fixed `ROOT` path resolution logic - Simplified conditional statements and control flow * ci: fix setup-node version in pr-labels workflow * tmp * refactor(ci): replace generic area labels with business-specific ones - Add PATH_TO_AREA_MAP to map shortcuts/skills paths to business areas (im, vc, ccm, base, mail, calendar, task, contact) - Replace importantAreas with businessAreas throughout the codebase - Remove area/shortcuts, area/skills, area/cmd generic labels - Now generates specific labels like area/im, area/vc, area/ccm, etc. - Update samples.json expected_areas to match new behavior Co-Authored-By: Claude Opus 4.6 * fix(ci): address PR review feedback for label scripts and workflows - Add `edited` event to PR labels workflow to trigger on title changes - Add security warning comment in pr-labels.yml workflow - Update pr-labels README with latest business area labels - Exclude `skills/lark-*` paths from low risk doc classification - Handle renamed files properly in PR path classification - Fix YAML frontmatter extraction to handle CRLF line endings - Use precise regex for YAML key validation instead of substring match - Fix exit code checking logic in skill-format-check test script - Translate Chinese comments in skill-format-check to English * fix(skill-format-check): address CodeRabbit review feedback - Fix frontmatter closing delimiter detection to strictly match '---' using regex, preventing invalid closing tags like '----' from passing. - Improve test fixture reliability by failing tests immediately if fixture preparation fails, avoiding false positives. * fix: address review comments from PR 148 - ci: warn when PR label sync fails in job summary - test(skill-format-check): capture validator output for negative tests - fix(skill-format-check): catch errors when reading SKILL.md to avoid hard crashes * fix: add error handling for directory enumeration in skill-format-check - refactor: use `fs.readdirSync` with `{ withFileTypes: true }` to avoid extra stat calls - fix: catch and report errors gracefully during skills directory enumeration instead of crashing * docs(skill-format-check): clarify `metadata` requirement in README test(pr-labels): add edge case samples for skills paths, CCM multi-paths, and renames * test(pr-labels): add real PR edge case samples - use PR #134 to test skill path behaviors - use PR #57 to test multi-path CCM resolution - use PR #11 to test track renames cross domains * refactor(ci): migrate pr labels from area to domain prefix - Replaced `area/` prefix with `domain/` for PR labeling to align with existing GitHub labels - Renamed internal constants and variables from `area` to `domain` (e.g. `PATH_TO_AREA_MAP` to `PATH_TO_DOMAIN_MAP`) - Updated `samples.json` test data to use new `domain/` format and `expected_domains` key - Added `scripts/pr-labels/test.js` runner script for continuous validation of labeling logic against PR samples - Corrected expected size label for PR #134 test sample * test: use execFileSync instead of execSync in pr-labels test script * fix: resolve target path against process.cwd() instead of __dirname in skill-format-check * docs: correct label prefix in PR label workflow README - Updated README.md to reflect the new `domain/` label prefix instead of `area/` * fix(ci): fix dry-run console output formatting and enforce auth in tests - Removed duplicate domain array interpolation in printDryRunResult - Added process.env.GITHUB_TOKEN guard in test.js to prevent ambiguous failures from API rate limits * fix(ci): ensure PR labels can be applied reliably - Added `issues: write` permission to pr-labels workflow, which is strictly required by the GitHub REST API to modify labels on pull requests - Reordered script execution in `index.js` to apply/remove labels on the PR *before* attempting to sync repository-level label definitions (colors/descriptions). The definition sync is now a trailing best-effort step with error catching so transient repo-level API failures don't abort the critical path. * fix(ci): fix edge cases in pr-label index script - Added missing `skills/lark-task/` to `PATH_TO_DOMAIN_MAP` to properly detect task domain modifications - Updated GitHub REST API error checking in `syncLabelDefinition` to reliably match `error.status === 422` rather than loosely checking substring - Moved token presence check in `main()` to happen before `resolveContext` to avoid triggering unauthenticated 401 API limits when GITHUB_TOKEN is omitted locally * test(ci): clean up PR label test samples - Removed duplicate PR entries (#11 and #57) to reduce redundant API calls during testing - Renamed sample test cases to correctly reflect their expected labels (e.g. `size-l-skill-format-check` -> `size-m-skill-format-check`) * fix(ci): bootstrap new labels before applying to PRs - Prior changes correctly made full label sync best-effort, but broke the flow for brand new domains - GitHub API returns a 422 error if you attempt to attach a label to an Issue/PR that does not exist in the repository - Added a targeted bootstrap loop to create/sync specifically the labels in `toAdd` before attempting `client.addLabels()` - Left the remaining global label synchronization as a best-effort trailing action * test(ci): automate PR label regression testing - Added a dedicated GitHub Actions workflow (`pr-labels-test.yml`) to automatically run `test.js` against `samples.json` whenever the labeling logic is updated - Documented local testing instructions in `scripts/pr-labels/README.md` --------- Co-authored-by: Claude Opus 4.6 --- .github/workflows/pr-labels-test.yml | 31 + .github/workflows/pr-labels.yml | 43 + .github/workflows/skill-format-check.yml | 32 + scripts/pr-labels/README.md | 58 ++ scripts/pr-labels/index.js | 747 ++++++++++++++++++ scripts/pr-labels/samples.json | 145 ++++ scripts/pr-labels/test.js | 52 ++ scripts/skill-format-check/README.md | 36 + scripts/skill-format-check/index.js | 96 +++ scripts/skill-format-check/test.sh | 82 ++ .../tests/bad-skill-no-frontmatter/SKILL.md | 3 + .../bad-skill-unclosed-frontmatter/SKILL.md | 9 + .../tests/bad-skill/SKILL.md | 8 + .../tests/good-skill-complex/SKILL.md | 17 + .../tests/good-skill-minimal/SKILL.md | 10 + .../tests/good-skill/SKILL.md | 12 + 16 files changed, 1381 insertions(+) create mode 100644 .github/workflows/pr-labels-test.yml create mode 100644 .github/workflows/pr-labels.yml create mode 100644 .github/workflows/skill-format-check.yml create mode 100644 scripts/pr-labels/README.md create mode 100755 scripts/pr-labels/index.js create mode 100644 scripts/pr-labels/samples.json create mode 100644 scripts/pr-labels/test.js create mode 100644 scripts/skill-format-check/README.md create mode 100644 scripts/skill-format-check/index.js create mode 100755 scripts/skill-format-check/test.sh create mode 100644 scripts/skill-format-check/tests/bad-skill-no-frontmatter/SKILL.md create mode 100644 scripts/skill-format-check/tests/bad-skill-unclosed-frontmatter/SKILL.md create mode 100644 scripts/skill-format-check/tests/bad-skill/SKILL.md create mode 100644 scripts/skill-format-check/tests/good-skill-complex/SKILL.md create mode 100644 scripts/skill-format-check/tests/good-skill-minimal/SKILL.md create mode 100644 scripts/skill-format-check/tests/good-skill/SKILL.md diff --git a/.github/workflows/pr-labels-test.yml b/.github/workflows/pr-labels-test.yml new file mode 100644 index 00000000..df8dd891 --- /dev/null +++ b/.github/workflows/pr-labels-test.yml @@ -0,0 +1,31 @@ +name: Test PR Label Logic + +on: + push: + branches: [main] + paths: + - "scripts/pr-labels/**" + - ".github/workflows/pr-labels-test.yml" + pull_request: + branches: [main] + paths: + - "scripts/pr-labels/**" + - ".github/workflows/pr-labels-test.yml" + +permissions: + contents: read + +jobs: + test-pr-labels: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Run PR label regression tests + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: node scripts/pr-labels/test.js diff --git a/.github/workflows/pr-labels.yml b/.github/workflows/pr-labels.yml new file mode 100644 index 00000000..096b4125 --- /dev/null +++ b/.github/workflows/pr-labels.yml @@ -0,0 +1,43 @@ +name: PR Labels + +on: + pull_request_target: + # NOTE: This event runs with base-branch code and write permissions. + # Do NOT add `ref: github.event.pull_request.head.sha` to the checkout step, + # as that would execute untrusted PR code with elevated access. + types: + - opened + - edited + - reopened + - synchronize + - ready_for_review + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + sync-pr-labels: + if: ${{ github.event.pull_request.state == 'open' }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Sync managed PR labels + id: sync_pr_labels + # Labeling is best-effort and must not block PR merges. + continue-on-error: true + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: node scripts/pr-labels/index.js + + - name: Warn when label sync fails + if: ${{ always() && steps.sync_pr_labels.outcome == 'failure' }} + run: | + echo "::warning::PR label sync failed; labels may be stale." + echo "โš ๏ธ PR label sync failed; labels may be stale." >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/skill-format-check.yml b/.github/workflows/skill-format-check.yml new file mode 100644 index 00000000..7c8b8fc5 --- /dev/null +++ b/.github/workflows/skill-format-check.yml @@ -0,0 +1,32 @@ +name: Skill Format Check + +on: + push: + branches: [main] + paths: + - "skills/**" + - "scripts/skill-format-check/**" + - ".github/workflows/skill-format-check.yml" + pull_request: + branches: [main] + paths: + - "skills/**" + - "scripts/skill-format-check/**" + - ".github/workflows/skill-format-check.yml" + +permissions: + contents: read + +jobs: + check-format: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Run Skill Format Check + run: node scripts/skill-format-check/index.js diff --git a/scripts/pr-labels/README.md b/scripts/pr-labels/README.md new file mode 100644 index 00000000..a996c8c2 --- /dev/null +++ b/scripts/pr-labels/README.md @@ -0,0 +1,58 @@ +# PR Label Sync + +This directory contains scripts and sample data for automatically classifying and labeling GitHub Pull Requests based on the files they modify. + +## Files + +- `index.js`: The main Node.js script. It fetches PR files, evaluates their risk level, calculates business impact, and uses GitHub APIs to add appropriate `size/*` and `domain/*` labels. +- `samples.json`: A collection of historical PRs used as test cases to verify the labeling logic (especially for regression testing the S/M/L thresholds). + +## Features + +### Size Labels (`size/*`) +The script evaluates the "effective" lines of code changed (ignoring tests, docs, and ci files) to classify the PR: +- **`size/S`**: Low-risk changes involving only docs, tests, CI workflows, or chores. +- **`size/M`**: Small-to-medium changes affecting a single business domain, with effective lines under 300. +- **`size/L`**: Large features (>= 300 lines), cross-domain changes, or any changes touching core architecture paths (like `cmd/`). +- **`size/XL`**: Architectural overhauls, extremely large PRs (>1200 lines), or sensitive refactors. + +### Domain Tags (`domain/*`) +The script also identifies which business domains a PR touches to give reviewers an immediate sense of the impact scope. Currently tracked domains include: +- `domain/im` +- `domain/vc` +- `domain/ccm` +- `domain/base` +- `domain/mail` +- `domain/calendar` +- `domain/task` +- `domain/contact` + +Minor modules like docs and tests are omitted to keep PR tags clean and focused on structural changes. + +## Usage + +### In GitHub Actions +This script is designed to run in CI workflows. It automatically reads the `GITHUB_EVENT_PATH` payload to get the PR context. + +```bash +node scripts/pr-labels/index.js +``` + +### Local Dry Run +You can test the labeling logic against an existing GitHub PR without actually applying labels by using the `--dry-run` flag. + +```bash +# Requires GITHUB_TOKEN environment variable or passing --token +node scripts/pr-labels/index.js --dry-run --repo larksuite/cli --pr-number 123 +``` + +## Testing + +A regression test suite is available in `test.js` which verifies the output of the classification logic against historical PRs configured in `samples.json`. + +```bash +# Requires GITHUB_TOKEN environment variable to avoid rate limits +GITHUB_TOKEN=$(gh auth token) node scripts/pr-labels/test.js +``` + +This test suite also runs automatically in CI via `.github/workflows/pr-labels-test.yml` when changes are made to this directory. \ No newline at end of file diff --git a/scripts/pr-labels/index.js b/scripts/pr-labels/index.js new file mode 100755 index 00000000..5897d0c2 --- /dev/null +++ b/scripts/pr-labels/index.js @@ -0,0 +1,747 @@ +#!/usr/bin/env node +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +const fs = require("node:fs/promises"); +const path = require("node:path"); + +// ============================================================================ +// Constants & Configuration +// ============================================================================ + +const API_BASE = "https://api.github.com"; +const SCRIPT_DIR = __dirname; +const ROOT = path.join(SCRIPT_DIR, "..", ".."); + +const THRESHOLD_L = 300; +const THRESHOLD_XL = 1200; + +const LABEL_DEFINITIONS = { + "size/S": { color: "77bb00", description: "Low-risk docs, CI, test, or chore only changes" }, + "size/M": { color: "eebb00", description: "Single-domain feat or fix with limited business impact" }, + "size/L": { color: "ff8800", description: "Large or sensitive change across domains or core paths" }, + "size/XL": { color: "ee0000", description: "Architecture-level or global-impact change" }, +}; + +const MANAGED_LABELS = new Set(Object.keys(LABEL_DEFINITIONS)); + +// File path matching configurations +const DOC_SUFFIXES = [".md", ".mdx", ".txt", ".rst"]; +const LOW_RISK_PREFIXES = [".github/", "docs/", ".changeset/", "testdata/", "tests/", "skill-template/"]; +const LOW_RISK_FILENAMES = new Set(["readme.md", "readme.zh.md", "changelog.md", "license", "cla.md"]); +const LOW_RISK_TEST_SUFFIXES = ["_test.go", ".snap"]; + +const CORE_PREFIXES = ["internal/auth/", "internal/engine/", "internal/config/", "cmd/"]; +const HEAD_BUSINESS_DOMAINS = new Set(["im", "contact", "ccm", "base", "docx"]); +const LOW_RISK_TYPES = new Set(["docs", "ci", "test", "chore"]); + +// CODEOWNERS-based path to domain label mapping +// Maps shortcuts and skills paths to business domain labels +const PATH_TO_DOMAIN_MAP = { + // shortcuts + "shortcuts/im/": "im", + "shortcuts/vc/": "vc", + "shortcuts/calendar/": "calendar", + "shortcuts/doc/": "ccm", + "shortcuts/sheets/": "ccm", + "shortcuts/drive/": "ccm", + "shortcuts/base/": "base", + "shortcuts/mail/": "mail", + "shortcuts/task/": "task", + "shortcuts/contact/": "contact", + // skills + "skills/lark-im/": "im", + "skills/lark-vc/": "vc", + "skills/lark-doc/": "ccm", + "skills/lark-base/": "base", + "skills/lark-mail/": "mail", + "skills/lark-calendar/": "calendar", + "skills/lark-task/": "task", + "skills/lark-contact/": "contact", +}; + +const SENSITIVE_PATTERN = /(^|\/)(auth|permission|permissions|security)(\/|_|\.|$)/; + +const CLASS_STANDARDS = { + "size/S": { + channel: "Fast track (S)", + gates: [ + "Code quality: AI code review passed", + "Dependency and configuration security checks passed", + ], + }, + "size/M": { + channel: "Fast track (M)", + gates: [ + "Code quality: AI code review passed", + "Dependency and configuration security checks passed", + "Skill format validation: added or modified Skills load successfully", + "CLI automation tests: all required business-line tests passed", + ], + }, + "size/L": { + channel: "Standard track (L)", + gates: [ + "Code quality: AI code review passed", + "Dependency and configuration security checks passed", + "Skill format validation: added or modified Skills load successfully", + "CLI automation tests: all required business-line tests passed", + "Domain evaluation passed: reported success rate is greater than 95%", + ], + }, + "size/XL": { + channel: "Strict track (XL)", + gates: [ + "Code quality: AI code review passed", + "Dependency and configuration security checks passed", + "Skill format validation: added or modified Skills load successfully", + "CLI automation tests: all required business-line tests passed", + "Domain evaluation passed: reported success rate is greater than 95%", + "Cross-domain release gate: all domains and full integration evaluations passed", + ], + }, +}; + +// ============================================================================ +// Utilities +// ============================================================================ + +function log(message) { + console.error(`sync-pr-labels: ${message}`); +} + +function normalizePath(input) { + return String(input || "").trim().toLowerCase(); +} + +function envValue(name) { + return (process.env[name] || "").trim(); +} + +function envOrFail(name) { + const value = envValue(name); + if (!value) { + throw new Error(`missing required environment variable: ${name}`); + } + return value; +} + +// ============================================================================ +// GitHub API Client +// ============================================================================ + +class GitHubClient { + constructor(token, repo, prNumber) { + this.token = token; + this.repo = repo; + this.prNumber = prNumber; + } + + buildHeaders(hasBody = false) { + const headers = { + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; + if (this.token) { + headers.Authorization = `Bearer ${this.token}`; + } + if (hasBody) { + headers["Content-Type"] = "application/json"; + } + return headers; + } + + async request(endpoint, options = {}) { + const { method = "GET", payload, allow404 = false } = options; + const hasBody = payload !== undefined; + const url = endpoint.startsWith("http") ? endpoint : `${API_BASE}${endpoint}`; + + const response = await fetch(url, { + method, + headers: this.buildHeaders(hasBody), + body: hasBody ? JSON.stringify(payload) : undefined, + }); + + if (allow404 && response.status === 404) { + return null; + } + + if (!response.ok) { + const detail = await response.text(); + const error = new Error(`GitHub API ${method} ${url} failed: ${response.status} ${detail}`); + error.status = response.status; + throw error; + } + + const text = await response.text(); + return text ? JSON.parse(text) : null; + } + + async getPullRequest() { + return this.request(`/repos/${this.repo}/pulls/${this.prNumber}`); + } + + async listPrFiles() { + const files = []; + for (let page = 1; ; page += 1) { + const params = new URLSearchParams({ per_page: "100", page: String(page) }); + const batch = await this.request(`/repos/${this.repo}/pulls/${this.prNumber}/files?${params}`); + if (!batch || batch.length === 0) { + break; + } + files.push(...batch); + if (batch.length < 100) { + break; + } + } + return files; + } + + async listIssueLabels() { + const labels = await this.request(`/repos/${this.repo}/issues/${this.prNumber}/labels`); + return new Set(labels.map((item) => item.name)); + } + + async syncLabelDefinition(name) { + const label = LABEL_DEFINITIONS[name]; + const createUrl = `/repos/${this.repo}/labels`; + const updateUrl = `/repos/${this.repo}/labels/${encodeURIComponent(name)}`; + + try { + await this.request(createUrl, { + method: "POST", + payload: { name, color: label.color, description: label.description }, + }); + log(`created label ${name}`); + } catch (error) { + if (error.status !== 422) { + throw error; + } + await this.request(updateUrl, { + method: "PATCH", + payload: { new_name: name, color: label.color, description: label.description }, + }); + log(`updated label ${name}`); + } + } + + async addLabels(labels) { + if (labels.length === 0) return; + await this.request(`/repos/${this.repo}/issues/${this.prNumber}/labels`, { + method: "POST", + payload: { labels }, + }); + log(`added labels: ${labels.join(", ")}`); + } + + async removeLabel(name) { + await this.request(`/repos/${this.repo}/issues/${this.prNumber}/labels/${encodeURIComponent(name)}`, { + method: "DELETE", + allow404: true, + }); + log(`removed label: ${name}`); + } +} + +// ============================================================================ +// Path & Domain Heuristics +// ============================================================================ + +function parsePrType(title) { + const match = String(title || "").trim().match(/^([a-z]+)(?:\([^)]+\))?!?:/i); + return match ? match[1].toLowerCase() : ""; +} + +function isLowRiskPath(filePath) { + const normalized = normalizePath(filePath); + const basename = path.posix.basename(normalized); + + if (normalized.startsWith("skills/lark-")) return false; + if (DOC_SUFFIXES.some((suffix) => normalized.endsWith(suffix))) return true; + if (LOW_RISK_FILENAMES.has(basename)) return true; + if (LOW_RISK_PREFIXES.some((prefix) => normalized.startsWith(prefix))) return true; + if (LOW_RISK_TEST_SUFFIXES.some((suffix) => normalized.endsWith(suffix))) return true; + return normalized.includes("/testdata/"); +} + +function isBusinessSkillPath(filePath) { + const normalized = normalizePath(filePath); + return normalized.startsWith("shortcuts/") || normalized.startsWith("skills/lark-"); +} + +function shortcutDomainForPath(filePath) { + const parts = normalizePath(filePath).split("/"); + return parts.length >= 2 && parts[0] === "shortcuts" ? parts[1] : ""; +} + +function skillDomainForPath(filePath) { + const parts = normalizePath(filePath).split("/"); + return parts.length >= 2 && parts[0] === "skills" && parts[1].startsWith("lark-") + ? parts[1].slice("lark-".length) + : ""; +} + +// Get business domain label based on CODEOWNERS path mapping +function getBusinessDomain(filePath) { + const normalized = normalizePath(filePath); + for (const [prefix, domain] of Object.entries(PATH_TO_DOMAIN_MAP)) { + if (normalized.startsWith(prefix)) { + return domain; + } + } + return ""; +} + +async function detectNewShortcutDomain(files) { + for (const item of files) { + if (item.status !== "added") continue; + const domain = shortcutDomainForPath(item.filename); + if (!domain) continue; + try { + await fs.access(path.join(ROOT, "shortcuts", domain)); + } catch { + return domain; + } + } + return ""; +} + +function collectCoreAreas(filenames) { + const areas = new Set(); + for (const name of filenames) { + const normalized = normalizePath(name); + for (const prefix of CORE_PREFIXES) { + if (normalized.startsWith(prefix)) { + // remove trailing slash for area name + areas.add(prefix.slice(0, -1)); + } + } + } + return areas; +} + +function collectSensitiveKeywords(filenames) { + const hits = new Set(); + for (const name of filenames) { + const match = normalizePath(name).match(SENSITIVE_PATTERN); + if (match && match[2]) { + hits.add(match[2]); + } + } + return [...hits].sort(); +} + +// ============================================================================ +// Classification Logic +// ============================================================================ + +function evaluateRules(context) { + const { + prType, effectiveChanges, lowRiskOnly, + domains, headDomains, coreAreas, coreSignals, + sensitiveKeywords, sensitive, newShortcutDomain, + singleDomain, multiDomain, filenames + } = context; + + const reasons = []; + let label; + + if (lowRiskOnly && (LOW_RISK_TYPES.has(prType) || effectiveChanges === 0)) { + reasons.push("Only low-risk docs, CI, test, or chore paths were changed, with no effective business code or Skill changes"); + label = "size/S"; + return { label, reasons }; + } + + // XL is reserved for architecture-level or global-impact changes. + const isXL = + effectiveChanges > THRESHOLD_XL || + (prType === "refactor" && sensitive && effectiveChanges >= THRESHOLD_L) || + (coreAreas.size >= 2 && (multiDomain || effectiveChanges >= THRESHOLD_L)) || + (headDomains.length >= 2 && sensitive); + + if (isXL) { + if (effectiveChanges > THRESHOLD_XL) reasons.push("Effective business code or Skill changes are far beyond the L threshold"); + if (prType === "refactor" && sensitive && effectiveChanges >= THRESHOLD_L) reasons.push("Refactor PR touches core or sensitive paths"); + if (coreAreas.size >= 2) reasons.push("Touches multiple core areas at the same time"); + if (headDomains.length >= 2) reasons.push("Impacts multiple major business domains"); + coreSignals.forEach((signal) => reasons.push(`Core area hit: ${signal}`)); + sensitiveKeywords.forEach((keyword) => reasons.push(`Sensitive keyword hit: ${keyword}`)); + label = "size/XL"; + } else if ( + prType === "refactor" || + effectiveChanges >= THRESHOLD_L || + Boolean(newShortcutDomain) || + multiDomain || + sensitive + ) { + if (prType === "refactor") reasons.push("PR type is refactor"); + if (effectiveChanges >= THRESHOLD_L) reasons.push(`Effective business code or Skill changes exceed ${THRESHOLD_L} lines`); + if (newShortcutDomain) reasons.push(`Introduces a new business domain directory: shortcuts/${newShortcutDomain}/`); + if (multiDomain) reasons.push("Touches multiple business domains"); + coreSignals.forEach((signal) => reasons.push(`Core area hit: ${signal}`)); + sensitiveKeywords.forEach((keyword) => reasons.push(`Sensitive keyword hit: ${keyword}`)); + label = "size/L"; + } else { + if (filenames.some(isBusinessSkillPath) || effectiveChanges > 0) { + reasons.push("Regular feat, fix, or Skill change within a single business domain"); + } + if (singleDomain && domains.size > 0) { + reasons.push(`Impact is limited to a single business domain: ${[...domains].sort().join(", ")}`); + } + if (effectiveChanges < THRESHOLD_L) { + reasons.push(`Effective business code or Skill changes are below ${THRESHOLD_L} lines`); + } + label = "size/M"; + } + + return { label, reasons }; +} + +async function classifyPr(payload, files) { + const pr = payload.pull_request; + const title = pr.title || ""; + const prType = parsePrType(title); + const filenames = files.map((item) => item.filename || ""); + const impactedPaths = files.flatMap((item) => { + const paths = [item.filename || ""]; + if (item.status === "renamed" && item.previous_filename) { + paths.push(item.previous_filename); + } + return paths.filter(Boolean); + }); + + // Filter out docs, tests, and other low-risk paths so the size label tracks business impact. + const effectiveChanges = files.reduce( + (sum, item) => sum + (isLowRiskPath(item.filename) ? 0 : (item.changes || 0)), + 0, + ); + const totalChanges = files.reduce((sum, item) => sum + (item.changes || 0), 0); + + const domains = new Set(); + const businessDomains = new Set(); + + for (const name of impactedPaths) { + const businessDomain = getBusinessDomain(name); + if (businessDomain) { + businessDomains.add(businessDomain); + domains.add(businessDomain); + continue; + } + + const shortcutDomain = shortcutDomainForPath(name); + if (shortcutDomain) domains.add(shortcutDomain); + + const skillDomain = skillDomainForPath(name); + if (skillDomain) domains.add(skillDomain); + } + + const coreAreas = collectCoreAreas(impactedPaths); + const newShortcutDomain = await detectNewShortcutDomain(files); + + const lowRiskOnly = impactedPaths.length > 0 && impactedPaths.every(isLowRiskPath); + const singleDomain = domains.size <= 1; + const multiDomain = domains.size >= 2; + const headDomains = [...domains].filter((domain) => HEAD_BUSINESS_DOMAINS.has(domain)); + const coreSignals = [...coreAreas].sort(); + const sensitiveKeywords = collectSensitiveKeywords(impactedPaths); + const sensitive = coreSignals.length > 0 || sensitiveKeywords.length > 0; + + const context = { + prType, effectiveChanges, lowRiskOnly, + domains, headDomains, coreAreas, coreSignals, + sensitiveKeywords, sensitive, newShortcutDomain, + singleDomain, multiDomain, filenames: impactedPaths + }; + + const { label, reasons } = evaluateRules(context); + + return { + label, + title, + prType: prType || "unknown", + totalChanges, + effectiveChanges, + domains: [...domains].sort(), + businessDomains: [...businessDomains].sort(), + coreAreas: [...coreAreas].sort(), + coreSignals, + sensitiveKeywords, + newShortcutDomain, + reasons, + lowRiskOnly, + filenames, + }; +} + +// ============================================================================ +// Output & Formatting +// ============================================================================ + +async function writeStepSummary(prNumber, classification) { + const summaryPath = (process.env.GITHUB_STEP_SUMMARY || "").trim(); + if (!summaryPath) return; + + const standard = CLASS_STANDARDS[classification.label]; + const domains = classification.domains.join(", ") || "-"; + const bDomains = classification.businessDomains.join(", ") || "-"; + const coreAreas = classification.coreAreas.join(", ") || "-"; + const reasons = classification.reasons.length > 0 + ? classification.reasons + : ["No higher-severity rule matched, so the PR defaults to medium classification"]; + + const lines = [ + "## PR Size Classification", + "", + `- PR: #${prNumber}`, + `- Label: \`${classification.label}\``, + `- PR Type: \`${classification.prType}\``, + `- Total Changes: \`${classification.totalChanges}\``, + `- Effective Business/SKILL Changes: \`${classification.effectiveChanges}\``, + `- Business Domains: \`${domains}\``, + `- Impacted Domains: \`${bDomains}\``, + `- Core Areas: \`${coreAreas}\``, + `- CI/CD Channel: \`${standard.channel}\``, + `- Low Risk Only: \`${classification.lowRiskOnly}\``, + "", + "### Reasons", + "", + ...reasons.map((reason) => `- ${reason}`), + "", + "### Pipeline Gates", + "", + ...standard.gates.map((gate) => `- ${gate}`), + "", + ]; + + await fs.appendFile(summaryPath, `${lines.join("\n")}\n`, "utf8"); +} + +function formatDryRunResult(repo, prNumber, classification) { + const standard = CLASS_STANDARDS[classification.label]; + return { + repo, + prNumber, + label: classification.label, + prType: classification.prType, + totalChanges: classification.totalChanges, + effectiveChanges: classification.effectiveChanges, + lowRiskOnly: classification.lowRiskOnly, + domains: classification.domains, + businessDomains: classification.businessDomains, + coreAreas: classification.coreAreas, + coreSignals: classification.coreSignals, + sensitiveKeywords: classification.sensitiveKeywords, + reasons: classification.reasons, + channel: standard.channel, + gates: standard.gates, + }; +} + +function printDryRunResult(result, options) { + if (options.json) { + console.log(JSON.stringify(result, null, 2)); + return; + } + + const signalParts = [ + ...result.coreSignals.map((signal) => `core:${signal}`), + ...result.sensitiveKeywords.map((keyword) => `keyword:${keyword}`), + ...(result.domains.length > 0 ? [`domains:${result.domains.join(",")}`] : []), + ]; + const reasonParts = result.reasons.length > 0 + ? result.reasons + : ["No higher-severity rule matched, so the PR defaults to medium classification"]; + + console.log( + `${result.label} | #${result.prNumber} | type:${result.prType} | eff:${result.effectiveChanges} | ` + + `sig:${signalParts.join(";") || "-"} | reason:${reasonParts.join("; ")}`, + ); +} + +function printHelp() { + const lines = [ + "Usage:", + " node scripts/pr-labels/index.js", + " node scripts/pr-labels/index.js --dry-run --pr-url [--token ] [--json]", + " node scripts/pr-labels/index.js --dry-run --repo --pr-number [--token ] [--json]", + "", + "Modes:", + " default Read the GitHub Actions event payload and apply labels", + " --dry-run Fetch the PR, compute the managed label, and print the result without writing labels", + "", + "Options:", + " --pr-url GitHub pull request URL, for example https://github.com/larksuite/cli/pull/123", + " --repo Repository name, used with --pr-number", + " --pr-number Pull request number, used with --repo", + " --token GitHub token override; falls back to GITHUB_TOKEN", + " --json Print dry-run output as JSON instead of the default one-line summary", + " --help Show this message", + ]; + console.log(lines.join("\n")); +} + +function parseArgs(argv) { + const options = { + dryRun: false, + json: false, + help: false, + prUrl: "", + repo: "", + prNumber: "", + token: "", + }; + + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i]; + if (arg === "--dry-run") options.dryRun = true; + else if (arg === "--json") options.json = true; + else if (arg === "--help" || arg === "-h") options.help = true; + else if (arg === "--pr-url") options.prUrl = argv[++i] || ""; + else if (arg === "--repo") options.repo = argv[++i] || ""; + else if (arg === "--pr-number") options.prNumber = argv[++i] || ""; + else if (arg === "--token") options.token = argv[++i] || ""; + else throw new Error(`unknown argument: ${arg}`); + } + + return options; +} + +function parsePrUrl(prUrl) { + let parsed; + try { + parsed = new URL(prUrl); + } catch { + throw new Error(`invalid PR URL: ${prUrl}`); + } + + const match = parsed.pathname.match(/^\/([^/]+)\/([^/]+)\/pull\/(\d+)\/?$/); + if (!match) throw new Error(`unsupported PR URL format: ${prUrl}`); + + return { repo: `${match[1]}/${match[2]}`, prNumber: Number(match[3]) }; +} + +async function loadEventPayload(filePath) { + return JSON.parse(await fs.readFile(filePath, "utf8")); +} + +async function resolveContext(options) { + const token = options.token; + + if (options.prUrl) { + const { repo, prNumber } = parsePrUrl(options.prUrl); + const client = new GitHubClient(token, repo, prNumber); + const payload = { + repository: { full_name: repo }, + pull_request: await client.getPullRequest(), + }; + return { repo, prNumber, payload, client }; + } + + if (options.repo || options.prNumber) { + if (!options.repo || !options.prNumber) throw new Error("--repo and --pr-number must be provided together"); + const prNumber = Number(options.prNumber); + if (!Number.isInteger(prNumber) || prNumber <= 0) throw new Error(`invalid PR number: ${options.prNumber}`); + + const client = new GitHubClient(token, options.repo, prNumber); + const payload = { + repository: { full_name: options.repo }, + pull_request: await client.getPullRequest(), + }; + return { repo: options.repo, prNumber, payload, client }; + } + + const eventPath = envOrFail("GITHUB_EVENT_PATH"); + const payload = await loadEventPayload(eventPath); + const repo = payload.repository.full_name; + const prNumber = payload.pull_request.number; + const client = new GitHubClient(token, repo, prNumber); + + return { repo, prNumber, payload, client }; +} + +// ============================================================================ +// Main Execution +// ============================================================================ + +async function main() { + const options = parseArgs(process.argv.slice(2)); + if (options.help) { + printHelp(); + return; + } + + options.token = options.token || envValue("GITHUB_TOKEN"); + + if (!options.dryRun && !options.token) { + throw new Error("missing required GitHub token; set GITHUB_TOKEN or pass --token"); + } + + const { repo, prNumber, payload, client } = await resolveContext(options); + + const files = await client.listPrFiles(); + const classification = await classifyPr(payload, files); + + if (options.dryRun) { + printDryRunResult(formatDryRunResult(repo, prNumber, classification), options); + return; + } + + const desired = new Set([classification.label]); + for (const domain of classification.businessDomains) { + desired.add(`domain/${domain}`); + } + + const current = await client.listIssueLabels(); + const managedCurrent = [...current].filter((label) => MANAGED_LABELS.has(label) || label.startsWith("domain/")); + const toAdd = [...desired].filter((label) => !current.has(label)).sort(); + const toRemove = managedCurrent.filter((label) => !desired.has(label)).sort(); + + for (const domain of classification.businessDomains) { + const labelName = `domain/${domain}`; + if (!LABEL_DEFINITIONS[labelName]) { + LABEL_DEFINITIONS[labelName] = { color: "1d76db", description: `PR touches the ${domain} domain` }; + } + } + + // Ensure labels to be added actually exist in the repository first + // If the label doesn't exist, GitHub API will return 422 Unprocessable Entity when trying to add it to a PR. + for (const label of toAdd) { + if (LABEL_DEFINITIONS[label]) { + try { + await client.syncLabelDefinition(label); + } catch (e) { + log(`Warning: Failed to bootstrap new label ${label}: ${e.message}`); + } + } + } + + await client.addLabels(toAdd); + + for (const label of toRemove) { + await client.removeLabel(label); + } + + // Keep other label metadata consistent. This is best-effort trailing work. + for (const label of Object.keys(LABEL_DEFINITIONS)) { + if (toAdd.includes(label)) continue; // Already synced above + try { + await client.syncLabelDefinition(label); + } catch (e) { + log(`Warning: Failed to sync label definition for ${label}: ${e.message}`); + } + } + + await writeStepSummary(prNumber, classification); + + log( + `pr #${prNumber} type=${classification.prType} total_changes=${classification.totalChanges} ` + + `effective_changes=${classification.effectiveChanges} files=${files.length} ` + + `desired=${[...desired].sort().join(",") || "-"} current_managed=${managedCurrent.sort().join(",") || "-"} ` + + `reasons=${classification.reasons.join(" | ") || "-"}`, + ); +} + +main().catch((error) => { + log(error.message || String(error)); + process.exit(1); +}); diff --git a/scripts/pr-labels/samples.json b/scripts/pr-labels/samples.json new file mode 100644 index 00000000..76dd7291 --- /dev/null +++ b/scripts/pr-labels/samples.json @@ -0,0 +1,145 @@ +[ + { + "name": "size-s-docs-badge", + "number": 103, + "title": "docs: add official badge to distinguish from third-party Lark CLI tools", + "pr_url": "https://github.com/larksuite/cli/pull/103", + "status": "merged", + "merged_at": "2026-03-30T12:15:45Z", + "expected_label": "size/S", + "expected_domains": [], + "review_note": "Pure docs sample. Useful to confirm low-risk paths stay in S even when total changed lines are not tiny." + }, + { + "name": "size-s-docs-simplify", + "number": 26, + "title": "docs: simplify installation steps by merging CLI and Skills into one โ€ฆ", + "pr_url": "https://github.com/larksuite/cli/pull/26", + "status": "merged", + "merged_at": "2026-03-28T09:33:24Z", + "expected_label": "size/S", + "expected_domains": [], + "review_note": "Docs sample, verifying docs changes remain in S." + }, + { + "name": "size-s-docs-star-history", + "number": 12, + "title": "docs: add Star History chart to readmes", + "pr_url": "https://github.com/larksuite/cli/pull/12", + "status": "merged", + "merged_at": "2026-03-28T16:00:15Z", + "expected_label": "size/S", + "expected_domains": [], + "review_note": "Docs sample, no effective business code changes." + }, + { + "name": "size-s-docs-clarify-install", + "number": 3, + "title": "docs: clarify install methods and add source build steps", + "pr_url": "https://github.com/larksuite/cli/pull/3", + "status": "merged", + "merged_at": "2026-03-28T03:43:44Z", + "expected_label": "size/S", + "expected_domains": [], + "review_note": "Docs sample, pure documentation clarification." + }, + { + "name": "size-m-fix-base-scope", + "number": 96, + "title": "fix(base): correct scope for record history list shortcut", + "pr_url": "https://github.com/larksuite/cli/pull/96", + "status": "merged", + "merged_at": "2026-03-30T11:40:18Z", + "expected_label": "size/M", + "expected_domains": ["domain/base"], + "review_note": "Small fix sample. Verify the lower edge of the M bucket within a single domain." + }, + { + "name": "size-m-fix-mail-sensitive", + "number": 92, + "title": "fix: remove sensitive send scope from reply and forward shortcuts", + "pr_url": "https://github.com/larksuite/cli/pull/92", + "status": "merged", + "merged_at": "2026-03-30T10:19:11Z", + "expected_label": "size/M", + "expected_domains": ["domain/mail"], + "review_note": "Security-like wording in the title but stays in one business domain (mail)." + }, + { + "name": "size-m-ci-improve", + "number": 71, + "title": "ci: improve CI workflows and add golangci-lint config", + "pr_url": "https://github.com/larksuite/cli/pull/71", + "status": "merged", + "merged_at": "2026-03-30T03:09:31Z", + "expected_label": "size/M", + "expected_domains": [], + "review_note": "CI workflow change that goes beyond S threshold." + }, + { + "name": "size-m-feat-im-pagination", + "number": 30, + "title": "feat: add auto-pagination to messages search and update lark-im docs", + "pr_url": "https://github.com/larksuite/cli/pull/30", + "status": "merged", + "merged_at": "2026-03-30T15:00:41Z", + "expected_label": "size/M", + "expected_domains": ["domain/im"], + "review_note": "Single-domain feature with larger diff but effective changes stay in M." + }, + { + "name": "size-l-fix-api-silent", + "number": 85, + "title": "fix: resolve silent failure in `lark-cli api` error output (#39)", + "pr_url": "https://github.com/larksuite/cli/pull/85", + "status": "merged", + "merged_at": "2026-03-30T09:19:24Z", + "expected_label": "size/L", + "expected_domains": [], + "review_note": "Touches core area (cmd), bumping the size to L." + }, + { + "name": "size-l-fix-cli", + "number": 91, + "title": "fix: correct CLI examples in root help and READMEs (closes #48)", + "pr_url": "https://github.com/larksuite/cli/pull/91", + "status": "closed", + "merged_at": null, + "expected_label": "size/L", + "expected_domains": [], + "review_note": "Closed PR touching core area (cmd)." + }, + { + "name": "size-m-skill-format-check", + "number": 134, + "title": "feat(ci): add skill format check workflow to ensure SKILL.md compliance", + "pr_url": "https://github.com/larksuite/cli/pull/134", + "status": "closed", + "merged_at": null, + "expected_label": "size/M", + "expected_domains": [], + "review_note": "Includes updates to tests/bad-skill/SKILL.md inside skills-like paths, testing how skill mock files and test scripts are handled." + }, + { + "name": "size-l-ccm-multi-path", + "number": 57, + "title": "feat(docs): support local image upload in docs +create", + "pr_url": "https://github.com/larksuite/cli/pull/57", + "status": "closed", + "merged_at": null, + "expected_label": "size/L", + "expected_domains": ["domain/ccm"], + "review_note": "Touches docs_create_images.go and table_auto_width.go, representing multiple CCM sub-paths but resolving to a single ccm domain." + }, + { + "name": "size-l-domain-rename", + "number": 11, + "title": "docs: rename user-facing Bitable references to Base", + "pr_url": "https://github.com/larksuite/cli/pull/11", + "status": "merged", + "merged_at": "2026-03-28T16:00:52Z", + "expected_label": "size/L", + "expected_domains": ["domain/base", "domain/ccm"], + "review_note": "A rename across paths. Since we track previous_filename to evaluate domains, this should properly capture the base domain." + } +] \ No newline at end of file diff --git a/scripts/pr-labels/test.js b/scripts/pr-labels/test.js new file mode 100644 index 00000000..db08ddc1 --- /dev/null +++ b/scripts/pr-labels/test.js @@ -0,0 +1,52 @@ +const fs = require('fs'); +const { execFileSync } = require('child_process'); +const path = require('path'); + +const samplesPath = path.join(__dirname, 'samples.json'); +const indexPath = path.join(__dirname, 'index.js'); +const samples = JSON.parse(fs.readFileSync(samplesPath, 'utf8')); + +if (!process.env.GITHUB_TOKEN) { + console.error("โŒ Error: GITHUB_TOKEN environment variable is required to run tests without hitting API rate limits."); + console.error("Please run: GITHUB_TOKEN=$(gh auth token) node scripts/pr-labels/test.js"); + process.exit(1); +} + +let passed = 0; +let failed = 0; + +for (const sample of samples) { + try { + const output = execFileSync( + process.execPath, + [indexPath, '--dry-run', '--json', '--pr-url', sample.pr_url], + { encoding: 'utf8', env: process.env } + ); + const result = JSON.parse(output); + + const matchLabel = result.label === sample.expected_label; + + // Sort before comparing to ignore order + const actualDomains = (result.businessDomains || []).sort(); + const expectedDomains = (sample.expected_domains || []).map(d => d.replace('domain/', '')).sort(); + + const matchDomains = JSON.stringify(actualDomains) === JSON.stringify(expectedDomains); + + if (matchLabel && matchDomains) { + console.log(`โœ… Passed: ${sample.name}`); + passed++; + } else { + console.log(`โŒ Failed: ${sample.name}`); + console.log(` Label expected: ${sample.expected_label}, got: ${result.label}`); + console.log(` Domains expected: ${expectedDomains}, got: ${actualDomains}`); + failed++; + } + } catch (e) { + console.log(`โŒ Failed: ${sample.name} (Execution error)`); + console.error(e.message); + failed++; + } +} + +console.log(`\nTest Summary: ${passed} passed, ${failed} failed`); +if (failed > 0) process.exit(1); diff --git a/scripts/skill-format-check/README.md b/scripts/skill-format-check/README.md new file mode 100644 index 00000000..04a00b4e --- /dev/null +++ b/scripts/skill-format-check/README.md @@ -0,0 +1,36 @@ +# Skill Format Check + +This directory contains a script to validate the format of `SKILL.md` files located in the `../../skills` directory. + +## Purpose + +The `index.js` script ensures that all `SKILL.md` files conform to the standard template defined in `skill-template/skill-template.md`. Specifically, it checks that the YAML frontmatter includes the following fields: +- `name` (required) +- `description` (required) +- `metadata` (outputs a warning if missing, does not fail the build) + +> **Note:** The `lark-shared` skill is explicitly excluded from these format checks. + +## Usage + +This script is executed automatically via GitHub Actions (`.github/workflows/skill-format-check.yml`) on pull requests and pushes that modify the `skills/` directory. + +To run the check manually from the root of the repository, execute: + +```bash +node scripts/skill-format-check/index.js +``` + +You can also specify a custom target directory as the first argument: + +```bash +node scripts/skill-format-check/index.js ./path/to/my/skills +``` + +## Testing + +This tool comes with a quick validation script to ensure it correctly identifies good and bad skill formats. To run the tests, execute: + +```bash +./scripts/skill-format-check/test.sh +``` diff --git a/scripts/skill-format-check/index.js b/scripts/skill-format-check/index.js new file mode 100644 index 00000000..71b14f00 --- /dev/null +++ b/scripts/skill-format-check/index.js @@ -0,0 +1,96 @@ +const fs = require('fs'); +const path = require('path'); + +// Allow passing a target directory as the first argument. +// If provided, resolve against process.cwd() so it behaves as the user expects. +// If not provided, default to '../../skills' relative to this script's directory. +const targetDirArg = process.argv[2]; +const SKILLS_DIR = targetDirArg + ? path.resolve(process.cwd(), targetDirArg) + : path.resolve(__dirname, '../../skills'); + +function checkSkillFormat() { + console.log(`Checking skill format in ${SKILLS_DIR}...`); + + if (!fs.existsSync(SKILLS_DIR)) { + console.error('Skills directory not found:', SKILLS_DIR); + process.exit(1); + } + + let skills; + try { + skills = fs + .readdirSync(SKILLS_DIR, { withFileTypes: true }) + .filter(entry => entry.isDirectory()) + .map(entry => entry.name); + } catch (err) { + console.error(`Failed to enumerate skills directory: ${err.message}`); + process.exit(1); + } + + let hasErrors = false; + + skills.forEach(skill => { + // Skip lark-shared skill completely + if (skill === 'lark-shared') { + console.log(`โญ๏ธ Skipping check for ${skill}`); + return; + } + + const skillPath = path.join(SKILLS_DIR, skill); + const skillFile = path.join(skillPath, 'SKILL.md'); + + if (!fs.existsSync(skillFile)) { + console.error(`โŒ [${skill}] Missing SKILL.md`); + hasErrors = true; + return; + } + + let content; + try { + content = fs.readFileSync(skillFile, 'utf-8'); + } catch (err) { + console.error(`โŒ [${skill}] Failed to read SKILL.md: ${err.message}`); + hasErrors = true; + return; + } + + // Normalize line endings to simplify parsing + const normalizedContent = content.replace(/\r\n/g, '\n'); + + // Check YAML Frontmatter + if (!normalizedContent.startsWith('---\n')) { + console.error(`โŒ [${skill}] SKILL.md must start with YAML frontmatter (---)`); + hasErrors = true; + } else { + const frontmatterMatch = normalizedContent.match(/^---\n([\s\S]*?)\n---(?:\n|$)/); + if (!frontmatterMatch) { + console.error(`โŒ [${skill}] SKILL.md has unclosed or invalid YAML frontmatter`); + hasErrors = true; + } else { + const frontmatter = frontmatterMatch[1]; + if (!/^name:/m.test(frontmatter)) { + console.error(`โŒ [${skill}] YAML frontmatter missing 'name'`); + hasErrors = true; + } + if (!/^description:/m.test(frontmatter)) { + console.error(`โŒ [${skill}] YAML frontmatter missing 'description'`); + hasErrors = true; + } + if (!/^metadata:/m.test(frontmatter)) { + console.warn(`โš ๏ธ [${skill}] YAML frontmatter missing 'metadata' (Warning only)`); + // hasErrors = true; // Downgrade to warning to not fail on existing skills + } + } + } + }); + + if (hasErrors) { + console.error('\nโŒ Skill format check failed. Please fix the errors above.'); + process.exit(1); + } else { + console.log('\nโœ… Skill format check passed!'); + } +} + +checkSkillFormat(); diff --git a/scripts/skill-format-check/test.sh b/scripts/skill-format-check/test.sh new file mode 100755 index 00000000..1a5faf82 --- /dev/null +++ b/scripts/skill-format-check/test.sh @@ -0,0 +1,82 @@ +#!/bin/bash + +# Get the directory of this script +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" +INDEX_JS="$DIR/index.js" +TEMP_DIR="$DIR/tests/temp_test_dir" + +echo "=== Running tests for skill-format-check ===" +echo "Index script: $INDEX_JS" + +prepare_fixture() { + local test_name=$1 + rm -rf "$TEMP_DIR" + mkdir -p "$TEMP_DIR" + if [ ! -d "$DIR/tests/$test_name" ]; then + echo "โŒ Missing fixture directory: $DIR/tests/$test_name" + exit 1 + fi + cp -r "$DIR/tests/$test_name" "$TEMP_DIR/" || { + echo "โŒ Failed to copy fixture: $test_name" + exit 1 + } +} + +# Function to run a positive test +run_positive_test() { + local test_name=$1 + echo -e "\n--- [Positive] $test_name ---" + + prepare_fixture "$test_name" + + node "$INDEX_JS" "$TEMP_DIR" + + if [ $? -eq 0 ]; then + echo "โœ… Passed! (Correctly validated $test_name)" + rm -rf "$TEMP_DIR" + return 0 + else + echo "โŒ Failed! Expected $test_name to pass but it failed." + rm -rf "$TEMP_DIR" + exit 1 + fi +} + +# Function to run a negative test +run_negative_test() { + local test_name=$1 + echo -e "\n--- [Negative] $test_name ---" + + prepare_fixture "$test_name" + + # Capture output for diagnostics while still treating non-zero as expected + local log_file="$TEMP_DIR/.validator.log" + node "$INDEX_JS" "$TEMP_DIR" > "$log_file" 2>&1 + local exit_code=$? + + if [ $exit_code -ne 0 ]; then + echo "โœ… Passed! (Correctly rejected $test_name)" + rm -rf "$TEMP_DIR" + return 0 + else + echo "โŒ Failed! Expected $test_name to fail but it passed." + if [ -s "$log_file" ]; then + echo "--- Validator output ---" + cat "$log_file" + fi + rm -rf "$TEMP_DIR" + exit 1 + fi +} + +# Run positive tests +run_positive_test "good-skill" +run_positive_test "good-skill-minimal" +run_positive_test "good-skill-complex" + +# Run negative tests +run_negative_test "bad-skill" +run_negative_test "bad-skill-no-frontmatter" +run_negative_test "bad-skill-unclosed-frontmatter" + +echo -e "\n๐ŸŽ‰ All tests passed successfully!" diff --git a/scripts/skill-format-check/tests/bad-skill-no-frontmatter/SKILL.md b/scripts/skill-format-check/tests/bad-skill-no-frontmatter/SKILL.md new file mode 100644 index 00000000..5a7afa3e --- /dev/null +++ b/scripts/skill-format-check/tests/bad-skill-no-frontmatter/SKILL.md @@ -0,0 +1,3 @@ +# No Frontmatter Skill + +This skill completely lacks a YAML frontmatter. diff --git a/scripts/skill-format-check/tests/bad-skill-unclosed-frontmatter/SKILL.md b/scripts/skill-format-check/tests/bad-skill-unclosed-frontmatter/SKILL.md new file mode 100644 index 00000000..189d6253 --- /dev/null +++ b/scripts/skill-format-check/tests/bad-skill-unclosed-frontmatter/SKILL.md @@ -0,0 +1,9 @@ +--- +name: bad-skill-unclosed +version: 1.0.0 +description: "This skill has an unclosed frontmatter block." +metadata: {} + +# Unclosed Frontmatter Skill + +This frontmatter does not have a closing `---` block. \ No newline at end of file diff --git a/scripts/skill-format-check/tests/bad-skill/SKILL.md b/scripts/skill-format-check/tests/bad-skill/SKILL.md new file mode 100644 index 00000000..465a05da --- /dev/null +++ b/scripts/skill-format-check/tests/bad-skill/SKILL.md @@ -0,0 +1,8 @@ +--- +version: 1.0.0 +metadata: {} +--- + +# Bad Skill + +This skill is missing required fields like name and description. diff --git a/scripts/skill-format-check/tests/good-skill-complex/SKILL.md b/scripts/skill-format-check/tests/good-skill-complex/SKILL.md new file mode 100644 index 00000000..0f7b5183 --- /dev/null +++ b/scripts/skill-format-check/tests/good-skill-complex/SKILL.md @@ -0,0 +1,17 @@ +--- +name: good-skill-complex +version: 2.5.1-beta +description: > + A very complex description + that spans multiple lines + and contains weird chars: !@#$%^&*() +metadata: + requires: + bins: ["lark-cli", "node"] + cliHelp: "lark-cli something --help" + customField: "customValue" +--- + +# Complex Skill + +This skill has a complex frontmatter block. diff --git a/scripts/skill-format-check/tests/good-skill-minimal/SKILL.md b/scripts/skill-format-check/tests/good-skill-minimal/SKILL.md new file mode 100644 index 00000000..ca3f481c --- /dev/null +++ b/scripts/skill-format-check/tests/good-skill-minimal/SKILL.md @@ -0,0 +1,10 @@ +--- +name: good-skill-minimal +version: 0.1.0 +description: Minimal valid description +metadata: {} +--- + +# Minimal Skill + +This has the bare minimum required fields. diff --git a/scripts/skill-format-check/tests/good-skill/SKILL.md b/scripts/skill-format-check/tests/good-skill/SKILL.md new file mode 100644 index 00000000..8c2e7b40 --- /dev/null +++ b/scripts/skill-format-check/tests/good-skill/SKILL.md @@ -0,0 +1,12 @@ +--- +name: good-skill +version: 1.0.0 +description: "This is a properly formatted skill." +metadata: + requires: + bins: ["lark-cli"] +--- + +# Good Skill + +This skill follows all the formatting rules. From d4c051d2112c30ba44a757d8e34e797db5b2fa07 Mon Sep 17 00:00:00 2001 From: JackZhao10086 Date: Wed, 1 Apr 2026 17:58:52 +0800 Subject: [PATCH 7/8] feat: improve OS keychain/DPAPI access error handling for sandbox environments (#173) * refactor(keychain): improve error handling and consistency across platforms - Change platformGet to return error instead of empty string - Add proper error wrapping for keychain operations - Make master key creation conditional in getMasterKey - Improve error messages and handling for keychain access - Update dependent code to handle new error returns * docs(keychain): improve function documentation and error message Add detailed doc comments for all platform-specific keychain functions to clarify their purpose and behavior. Also enhance the error hint message to include a suggestion for reconfiguring the CLI when keychain access fails. * refactor(keychain): reorder operations in platformGet for better error handling Check for file existence before attempting to read and get master key * fix(keychain): improve error handling and consistency across platforms. * fix(keychain): handle corrupted master key case * fix(keychain): handle I/O errors when reading master key --- internal/auth/token_store.go | 4 +- internal/core/config.go | 8 ++++ internal/keychain/default.go | 11 ++--- internal/keychain/keychain.go | 34 +++++++++++++-- internal/keychain/keychain_darwin.go | 60 ++++++++++++++++++++------- internal/keychain/keychain_other.go | 46 +++++++++++++++----- internal/keychain/keychain_windows.go | 21 ++++++++-- 7 files changed, 142 insertions(+), 42 deletions(-) diff --git a/internal/auth/token_store.go b/internal/auth/token_store.go index 80883a64..7e52f670 100644 --- a/internal/auth/token_store.go +++ b/internal/auth/token_store.go @@ -39,8 +39,8 @@ func MaskToken(token string) string { // GetStoredToken reads the stored UAT for a given (appId, userOpenId) pair. func GetStoredToken(appId, userOpenId string) *StoredUAToken { - jsonStr := keychain.Get(keychain.LarkCliService, accountKey(appId, userOpenId)) - if jsonStr == "" { + jsonStr, err := keychain.Get(keychain.LarkCliService, accountKey(appId, userOpenId)) + if err != nil || jsonStr == "" { return nil } var token StoredUAToken diff --git a/internal/core/config.go b/internal/core/config.go index ced1e27b..18a2aa4e 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -5,11 +5,13 @@ package core import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" "github.com/larksuite/cli/internal/keychain" + "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/validate" ) @@ -113,6 +115,12 @@ func RequireConfig(kc keychain.KeychainAccess) (*CliConfig, error) { app := raw.Apps[0] secret, err := ResolveSecretInput(app.AppSecret, kc) if err != nil { + // If the error comes from the keychain, it will already be wrapped as an ExitError. + // For other errors (e.g. file read errors, unknown sources), wrap them as ConfigError. + var exitErr *output.ExitError + if errors.As(err, &exitErr) { + return nil, exitErr + } return nil, &ConfigError{Code: 2, Type: "config", Message: err.Error()} } cfg := &CliConfig{ diff --git a/internal/keychain/default.go b/internal/keychain/default.go index 5d9e3d10..59d99ebf 100644 --- a/internal/keychain/default.go +++ b/internal/keychain/default.go @@ -3,17 +3,12 @@ package keychain -import "fmt" - -// defaultKeychain implements KeychainAccess using the real platform keychain. +// defaultKeychain is the default implementation of KeychainAccess +// that uses the package-level functions. type defaultKeychain struct{} func (d *defaultKeychain) Get(service, account string) (string, error) { - val := Get(service, account) - if val == "" { - return "", fmt.Errorf("keychain entry not found: %s/%s", service, account) - } - return val, nil + return Get(service, account) } func (d *defaultKeychain) Set(service, account, value string) error { diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index c225db8b..a5dc74b5 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -5,6 +5,15 @@ // macOS uses the system Keychain; Linux uses AES-256-GCM encrypted files; Windows uses DPAPI + registry. package keychain +import ( + "errors" + "fmt" + + "github.com/larksuite/cli/internal/output" +) + +var errNotInitialized = errors.New("keychain not initialized") + const ( // LarkCliService is the unified keychain service name for all secrets // (both AppSecret and UAT). Entries are distinguished by account key format: @@ -13,6 +22,22 @@ const ( LarkCliService = "lark-cli" ) +// wrapError is a helper to wrap underlying errors into output.ExitError. +// It formats the error message and provides a hint for troubleshooting keychain access issues. +func wrapError(op string, err error) error { + if err == nil { + return nil + } + msg := fmt.Sprintf("keychain %s failed: %v", op, err) + hint := "Check if the OS keychain/credential manager is locked or accessible. If running inside a sandbox or CI environment, please ensure the process has the necessary permissions to access the keychain." + + if errors.Is(err, errNotInitialized) { + hint = "The keychain master key may have been cleaned up or deleted. Please reconfigure the CLI by running `lark-cli config init`." + } + + return output.ErrWithHint(output.ExitAPI, "config", msg, hint) +} + // KeychainAccess abstracts keychain Get/Set/Remove for dependency injection. // Used by AppSecret operations (ForStorage, ResolveSecretInput, RemoveSecretStore). // UAT operations in token_store.go use the package-level Get/Set/Remove directly. @@ -24,16 +49,17 @@ type KeychainAccess interface { // Get retrieves a value from the keychain. // Returns empty string if the entry does not exist. -func Get(service, account string) string { - return platformGet(service, account) +func Get(service, account string) (string, error) { + val, err := platformGet(service, account) + return val, wrapError("Get", err) } // Set stores a value in the keychain, overwriting any existing entry. func Set(service, account, data string) error { - return platformSet(service, account, data) + return wrapError("Set", platformSet(service, account, data)) } // Remove deletes an entry from the keychain. No error if not found. func Remove(service, account string) error { - return platformRemove(service, account) + return wrapError("Remove", platformRemove(service, account)) } diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index fe71583d..a49633f7 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -11,6 +11,7 @@ import ( "crypto/cipher" "crypto/rand" "encoding/base64" + "errors" "os" "path/filepath" "regexp" @@ -36,11 +37,14 @@ func StorageDir(service string) string { var safeFileNameRe = regexp.MustCompile(`[^a-zA-Z0-9._-]`) +// safeFileName sanitizes an account name to be used as a safe file name. func safeFileName(account string) string { return safeFileNameRe.ReplaceAllString(account, "_") + ".enc" } -func getMasterKey(service string) ([]byte, error) { +// getMasterKey retrieves the master key from the system keychain. +// If allowCreate is true, it generates and stores a new master key if one doesn't exist. +func getMasterKey(service string, allowCreate bool) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), keychainTimeout) defer cancel() @@ -59,28 +63,48 @@ func getMasterKey(service string) ([]byte, error) { resCh <- result{key: key, err: nil} return } + // Key is found but invalid or corrupted + resCh <- result{key: nil, err: errors.New("keychain is corrupted")} + return + } else if !errors.Is(err, keyring.ErrNotFound) { + // Not ErrNotFound, which means access was denied or blocked by the system + resCh <- result{key: nil, err: errors.New("keychain access blocked")} + return + } + + // If ErrNotFound, check if we are allowed to create a new key + if !allowCreate { + // Creation not allowed (e.g., during Get operation), return error + resCh <- result{key: nil, err: errNotInitialized} + return } - // Generate new master key if not found or invalid + // It's the first time and creation is allowed (Set operation), generate a new key key := make([]byte, masterKeyBytes) if _, randErr := rand.Read(key); randErr != nil { resCh <- result{key: nil, err: randErr} return } - encodedKey = base64.StdEncoding.EncodeToString(key) - setErr := keyring.Set(service, "master.key", encodedKey) - resCh <- result{key: key, err: setErr} + encodedKeyStr := base64.StdEncoding.EncodeToString(key) + setErr := keyring.Set(service, "master.key", encodedKeyStr) + if setErr != nil { + resCh <- result{key: nil, err: setErr} + return + } + resCh <- result{key: key, err: nil} }() select { case res := <-resCh: return res.key, res.err case <-ctx.Done(): - return nil, ctx.Err() + // Timeout is usually caused by ignored/blocked permission prompts + return nil, errors.New("keychain access blocked") } } +// encryptData encrypts data using AES-GCM. func encryptData(plaintext string, key []byte) ([]byte, error) { block, err := aes.NewCipher(key) if err != nil { @@ -103,6 +127,7 @@ func encryptData(plaintext string, key []byte) ([]byte, error) { return result, nil } +// decryptData decrypts data using AES-GCM. func decryptData(data []byte, key []byte) (string, error) { if len(data) < ivBytes+tagBytes { return "", os.ErrInvalid @@ -125,24 +150,30 @@ func decryptData(data []byte, key []byte) (string, error) { return string(plaintext), nil } -func platformGet(service, account string) string { - key, err := getMasterKey(service) +// platformGet retrieves a value from the macOS keychain. +func platformGet(service, account string) (string, error) { + path := filepath.Join(StorageDir(service), safeFileName(account)) + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } if err != nil { - return "" + return "", err } - data, err := os.ReadFile(filepath.Join(StorageDir(service), safeFileName(account))) + key, err := getMasterKey(service, false) if err != nil { - return "" + return "", err } plaintext, err := decryptData(data, key) if err != nil { - return "" + return "", err } - return plaintext + return plaintext, nil } +// platformSet stores a value in the macOS keychain. func platformSet(service, account, data string) error { - key, err := getMasterKey(service) + key, err := getMasterKey(service, true) if err != nil { return err } @@ -170,6 +201,7 @@ func platformSet(service, account, data string) error { return nil } +// platformRemove deletes a value from the macOS keychain. func platformRemove(service, account string) error { err := os.Remove(filepath.Join(StorageDir(service), safeFileName(account))) if err != nil && !os.IsNotExist(err) { diff --git a/internal/keychain/keychain_other.go b/internal/keychain/keychain_other.go index 631a9fb0..55192d46 100644 --- a/internal/keychain/keychain_other.go +++ b/internal/keychain/keychain_other.go @@ -9,6 +9,7 @@ import ( "crypto/aes" "crypto/cipher" "crypto/rand" + "errors" "fmt" "os" "path/filepath" @@ -21,8 +22,7 @@ const masterKeyBytes = 32 const ivBytes = 12 const tagBytes = 16 -// StorageDir returns the storage directory for a given service name. -// Each service gets its own directory for physical isolation. +// StorageDir returns the directory where encrypted files are stored. func StorageDir(service string) string { home, err := os.UserHomeDir() if err != nil || home == "" { @@ -36,11 +36,14 @@ func StorageDir(service string) string { var safeFileNameRe = regexp.MustCompile(`[^a-zA-Z0-9._-]`) +// safeFileName sanitizes an account name to be used as a safe file name. func safeFileName(account string) string { return safeFileNameRe.ReplaceAllString(account, "_") + ".enc" } -func getMasterKey(service string) ([]byte, error) { +// getMasterKey retrieves the master key from the file system. +// If allowCreate is true, it generates and stores a new master key if one doesn't exist. +func getMasterKey(service string, allowCreate bool) ([]byte, error) { dir := StorageDir(service) keyPath := filepath.Join(dir, "master.key") @@ -48,6 +51,18 @@ func getMasterKey(service string) ([]byte, error) { if err == nil && len(key) == masterKeyBytes { return key, nil } + if err == nil && len(key) != masterKeyBytes { + // Key file exists but is corrupted + return nil, errors.New("keychain is corrupted") + } + if err != nil && !errors.Is(err, os.ErrNotExist) { + // Real I/O error (permission denied, etc.) - propagate it + return nil, err + } + + if !allowCreate { + return nil, errNotInitialized + } if err := os.MkdirAll(dir, 0700); err != nil { return nil, err @@ -78,6 +93,7 @@ func getMasterKey(service string) ([]byte, error) { return key, nil } +// encryptData encrypts data using AES-GCM. func encryptData(plaintext string, key []byte) ([]byte, error) { block, err := aes.NewCipher(key) if err != nil { @@ -100,6 +116,7 @@ func encryptData(plaintext string, key []byte) ([]byte, error) { return result, nil } +// decryptData decrypts data using AES-GCM. func decryptData(data []byte, key []byte) (string, error) { if len(data) < ivBytes+tagBytes { return "", os.ErrInvalid @@ -122,24 +139,30 @@ func decryptData(data []byte, key []byte) (string, error) { return string(plaintext), nil } -func platformGet(service, account string) string { - key, err := getMasterKey(service) +// platformGet retrieves a value from the file system. +func platformGet(service, account string) (string, error) { + path := filepath.Join(StorageDir(service), safeFileName(account)) + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } if err != nil { - return "" + return "", err } - data, err := os.ReadFile(filepath.Join(StorageDir(service), safeFileName(account))) + key, err := getMasterKey(service, false) if err != nil { - return "" + return "", err } plaintext, err := decryptData(data, key) if err != nil { - return "" + return "", err } - return plaintext + return plaintext, nil } +// platformSet stores a value in the file system. func platformSet(service, account, data string) error { - key, err := getMasterKey(service) + key, err := getMasterKey(service, true) if err != nil { return err } @@ -167,6 +190,7 @@ func platformSet(service, account, data string) error { return nil } +// platformRemove deletes a value from the file system. func platformRemove(service, account string) error { err := os.Remove(filepath.Join(StorageDir(service), safeFileName(account))) if err != nil && !os.IsNotExist(err) { diff --git a/internal/keychain/keychain_windows.go b/internal/keychain/keychain_windows.go index 8830e8ac..f0e3f9f8 100644 --- a/internal/keychain/keychain_windows.go +++ b/internal/keychain/keychain_windows.go @@ -22,12 +22,14 @@ import ( const regRootPath = `Software\LarkCli\keychain` +// registryPathForService returns the registry path for a given service. func registryPathForService(service string) string { return regRootPath + `\` + safeRegistryComponent(service) } var safeRegRe = regexp.MustCompile(`[^a-zA-Z0-9._-]`) +// safeRegistryComponent sanitizes a string to be used as a registry key component. func safeRegistryComponent(s string) string { // Registry key path uses '\\' separators; avoid accidental nesting and odd chars. s = strings.ReplaceAll(s, "\\", "_") @@ -39,6 +41,7 @@ func valueNameForAccount(account string) string { return base64.RawURLEncoding.EncodeToString([]byte(account)) } +// dpapiEntropy generates entropy for DPAPI encryption based on the service and account names. func dpapiEntropy(service, account string) *windows.DataBlob { // Bind ciphertext to (service, account) to reduce swap/replay risks. // Note: empty entropy is allowed, but we intentionally use deterministic entropy. @@ -49,6 +52,7 @@ func dpapiEntropy(service, account string) *windows.DataBlob { return &windows.DataBlob{Size: uint32(len(data)), Data: &data[0]} } +// dpapiProtect encrypts data using Windows DPAPI. func dpapiProtect(plaintext []byte, entropy *windows.DataBlob) ([]byte, error) { var in windows.DataBlob if len(plaintext) > 0 { @@ -70,6 +74,7 @@ func dpapiProtect(plaintext []byte, entropy *windows.DataBlob) ([]byte, error) { return res, nil } +// dpapiUnprotect decrypts data using Windows DPAPI. func dpapiUnprotect(ciphertext []byte, entropy *windows.DataBlob) ([]byte, error) { var in windows.DataBlob if len(ciphertext) > 0 { @@ -91,6 +96,7 @@ func dpapiUnprotect(ciphertext []byte, entropy *windows.DataBlob) ([]byte, error return res, nil } +// freeDataBlob frees the memory allocated for a DataBlob. func freeDataBlob(b *windows.DataBlob) { if b == nil || b.Data == nil { return @@ -101,11 +107,16 @@ func freeDataBlob(b *windows.DataBlob) { b.Size = 0 } -func platformGet(service, account string) string { - v, _ := registryGet(service, account) - return v +// platformGet retrieves a value from the Windows registry. +func platformGet(service, account string) (string, error) { + v, ok := registryGet(service, account) + if !ok { + return "", nil + } + return v, nil } +// platformSet stores a value in the Windows registry. func platformSet(service, account, data string) error { entropy := dpapiEntropy(service, account) protected, err := dpapiProtect([]byte(data), entropy) @@ -115,10 +126,12 @@ func platformSet(service, account, data string) error { return registrySet(service, account, protected) } +// platformRemove deletes a value from the Windows registry. func platformRemove(service, account string) error { return registryRemove(service, account) } +// registryGet retrieves a string value from the registry under the given service and account. func registryGet(service, account string) (string, bool) { keyPath := registryPathForService(service) k, err := registry.OpenKey(registry.CURRENT_USER, keyPath, registry.QUERY_VALUE) @@ -143,6 +156,7 @@ func registryGet(service, account string) (string, bool) { return string(plain), true } +// registrySet stores a string value in the registry under the given service and account. func registrySet(service, account string, protected []byte) error { keyPath := registryPathForService(service) k, _, err := registry.CreateKey(registry.CURRENT_USER, keyPath, registry.SET_VALUE) @@ -158,6 +172,7 @@ func registrySet(service, account string, protected []byte) error { return nil } +// registryRemove deletes a value from the registry under the given service and account. func registryRemove(service, account string) error { keyPath := registryPathForService(service) k, err := registry.OpenKey(registry.CURRENT_USER, keyPath, registry.SET_VALUE) From eb8b542f42508b38afc0f518d9171fd05c6f91a4 Mon Sep 17 00:00:00 2001 From: MaxHuang22 Date: Wed, 1 Apr 2026 20:14:19 +0800 Subject: [PATCH 8/8] feat: add TestGenerateShortcutsJSON and skip redundant meta fetch (#179) * feat: add TestGenerateShortcutsJSON for registry shortcut export Add a test that exports all shortcuts as JSON when SHORTCUTS_OUTPUT env var is set, enabling the registry repo to extract shortcut metadata without depending on a dump-shortcuts CLI command. --- scripts/fetch_meta.py | 16 +++++++++++++++ shortcuts/register_test.go | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/scripts/fetch_meta.py b/scripts/fetch_meta.py index 4c0145b4..b7ab0908 100644 --- a/scripts/fetch_meta.py +++ b/scripts/fetch_meta.py @@ -67,8 +67,24 @@ def main(): parser = argparse.ArgumentParser(description="Fetch meta_data.json for build-time embedding") parser.add_argument("--brand", default="feishu", choices=["feishu", "lark"], help="API brand (default: feishu)") + parser.add_argument("--force", action="store_true", + help="force refresh from remote even if local file exists") args = parser.parse_args() + if os.path.exists(OUT_PATH) and not args.force: + if os.path.isfile(OUT_PATH): + try: + with open(OUT_PATH, "r", encoding="utf-8") as fp: + local = json.load(fp) + if local.get("services"): + print(f"fetch-meta: {OUT_PATH} already exists, skipping (use --force to re-fetch)", file=sys.stderr) + return + print(f"fetch-meta: {OUT_PATH} has no services, re-fetching", file=sys.stderr) + except (OSError, json.JSONDecodeError): + print(f"fetch-meta: {OUT_PATH} is invalid JSON, re-fetching", file=sys.stderr) + else: + print(f"fetch-meta: {OUT_PATH} is not a file, re-fetching", file=sys.stderr) + data = fetch_remote(args.brand) count = len(data.get("services", [])) print(f"fetch-meta: OK, {count} services from remote API", file=sys.stderr) diff --git a/shortcuts/register_test.go b/shortcuts/register_test.go index 48b72c39..2d169617 100644 --- a/shortcuts/register_test.go +++ b/shortcuts/register_test.go @@ -4,6 +4,10 @@ package shortcuts import ( + "encoding/json" + "os" + "path/filepath" + "strings" "testing" "github.com/larksuite/cli/internal/cmdutil" @@ -88,3 +92,39 @@ func TestRegisterShortcutsReusesExistingServiceCommand(t *testing.T) { t.Fatal("base workspace shortcut not mounted on existing service command") } } + +func TestGenerateShortcutsJSON(t *testing.T) { + output := os.Getenv("SHORTCUTS_OUTPUT") + if output == "" { + t.Skip("set SHORTCUTS_OUTPUT env to generate shortcuts.json") + } + + shortcuts := AllShortcuts() + + type entry struct { + Verb string `json:"verb"` + Description string `json:"description"` + Scopes []string `json:"scopes"` + } + grouped := make(map[string][]entry) + for _, s := range shortcuts { + verb := strings.TrimPrefix(s.Command, "+") + grouped[s.Service] = append(grouped[s.Service], entry{ + Verb: verb, + Description: s.Description, + Scopes: s.ScopesForIdentity("user"), + }) + } + + data, err := json.MarshalIndent(grouped, "", " ") + if err != nil { + t.Fatalf("marshal shortcuts: %v", err) + } + if err := os.MkdirAll(filepath.Dir(output), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(output, data, 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + t.Logf("wrote %d bytes to %s", len(data), output) +}