From 5a9d156f3fcb7ad3b42ab3bb819f5add512004c5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 28 Sep 2026 19:53:25 +0000 Subject: [PATCH] fix(publish): Clean up release branches for revisions --- .lore.md | 546 +++++++++++++------- src/commands/__tests__/publish-main.test.ts | 237 +++++++++ src/commands/__tests__/publish.test.ts | 336 ++++++++++-- src/commands/publish.ts | 135 +++-- 4 files changed, 990 insertions(+), 264 deletions(-) create mode 100644 src/commands/__tests__/publish-main.test.ts diff --git a/.lore.md b/.lore.md index 9f6a2813..4b92ea6e 100644 --- a/.lore.md +++ b/.lore.md @@ -12,10 +12,6 @@ - **Craft Cloudflare target: deployType defaults to worker, account ID optional**: Cloudflare target's \`deployType\` config default flipped from \`pages\` to \`worker\` (Cloudflare is steering new projects to Workers; Pages is positioned as legacy but still supported via \`deployType: pages\`). \`CLOUDFLARE_ACCOUNT_ID\` is optional — only \`CLOUDFLARE_API_TOKEN\` is required/secret; account ID is a non-secret identifier, forwarded to wrangler only when set, else wrangler auto-discovers it (single-account tokens) or errors listing account IDs in non-interactive/CI multi-account cases. No Cloudflare SDK added — raw global \`fetch\` used (Node 24.18.0 baseline). \`CLOUDFLARE_API_TOKEN\` always passed via env vars, never CLI argv/logs. Wrangler v4 has no built-in OIDC/keyless auth (unlike craft's npm target OIDC path) — auth is via API token, legacy Global API Key (deprecated), or \`wrangler login\` OAuth only. Introduced as a PR #843 follow-up; adversarial review verdict: solid, no CRITICAL/MAJOR issues. - - -- **Craft config schema: top-level CraftProjectConfigSchema NOT passthrough, TargetConfigSchema IS**: In getsentry/craft's \`schemas/project_config.ts\`, the top-level \`CraftProjectConfigSchema\` is a plain \`z.object\` — NOT \`.passthrough()\` — so Zod strips any unknown top-level key during \`validateConfiguration()\` (config.ts:130). Any new top-level feature (e.g. a \`workspaces:\` key) must be added explicitly to this schema; it cannot be smuggled in via passthrough. By contrast, \`TargetConfigSchema\` IS \`.passthrough()\`, so target-specific fields (tagPrefix, deployType, workspaces bool) flow through freely and are narrowed via \`TypedTargetConfig\\`. Provider schemas (\`BaseStatusProviderSchema\`, \`BaseArtifactProviderSchema\`) use permissive \`z.record(z.any())\`. When designing new top-level config, follow existing structured nested-object/union patterns (\`VersioningConfigSchema\`, \`ChangelogConfigSchema\`) rather than assuming passthrough behavior applies globally. - - **Craft npm target auth: temp .npmrc via npm_config_userconfig bypasses all default config**: Craft's npm target creates a temporary \`.npmrc\` file containing \`//registry.npmjs.org/:\_authToken=${NPM_TOKEN}\` and sets the \`npm_config_userconfig\` env var to point to it. This completely overrides npm's default config file lookup chain — the user's home \`.npmrc\` and project \`.npmrc\` are both bypassed. This is why OIDC (which relies on \`setup-node\` creating a properly configured project \`.npmrc\`) requires a separate code path that skips the temp file entirely. The pattern is used in both \`publishPackage()\` and \`getLatestVersion()\`. The \`npm_config_userconfig\` approach (instead of \`--userconfig\` CLI flag) was chosen for yarn compatibility. @@ -44,22 +40,70 @@ - **getsentry/publish poller: active-polling via self-dispatch, cron as fallback**: GitHub Actions \`\*/5\` cron drifts to 30-40 min under load, stalling releases with fast CI. \`ci-poller.yml\` uses self-dispatch: when pending issues remain, it \`gh workflow run\`s itself (cap 60 attempts ~30min). Concurrency group \`ci-status-poller\` with \`cancel-in-progress: false\` queues runs — GHA startup gives ~30-60s between checks. Chain starts via \`waiting-for-ci\` job in \`publish.yml\` when \`accepted\` label is added. Cron is safety net (also skipped when \`CI_POLLER_HAS_PENDING == 'false'\`). Zero idle cost — no chain running unless human action occurred. Self-dispatch step must guard on \`steps.token.outcome == 'success'\` and \`steps.remaining.outcome == 'success'\` to avoid dispatching with empty token. - + -- **Publish repo event-driven CI gate replaces polling in craft publish**: The \`getsentry/publish\` repo uses a label-based state machine instead of having \`craft publish\` poll CI status for up to 60 minutes. Labels: \`ci-pending\` (set at issue creation), \`ci-ready\` (set by poller or dispatch when CI passes). The \`publish.yml\` job requires \`accepted && !ci-pending\`. A \`ci-poller.yml\` cron (every 5 min) checks CI via GitHub API and swaps labels when ready. A \`ci-ready.yml\` handles \`repository_dispatch\` for repos that signal directly. This eliminates wasted runner time from idle polling and preserves the GitHub App token's 1-hour lifetime for the actual publish step. +- **handleReleaseBranch non-blocking cleanup**: Chose non-blocking \`handleReleaseBranch()\` housekeeping over failing an otherwise completed publish because all targets have already shipped before the release-branch merge and deletion run. It attempts \`--no-ff\` with Git’s default \`ort\`, retries conflicts with \`-s resolve\` for criss-cross histories, and distinguishes authentication failures through \`isAuthError()\`; remaining failures go to Sentry and warning logs without changing publish success. ### Decision + + +- **CalVer publish issues suppress @mentions**: For CalVer releases, transform changelog \`by @author in\` text to bold author text only in the publish-issue payload via \`disableChangelogMentions\`; keep committed \`CHANGELOG.md\` entries unchanged. Chose issue-only suppression over rewriting the generated changelog because regular-cadence issue bodies would mass-notify contributors, while repository markdown does not notify them. + + + +- **cloudflare.md token permissions**: Document Cloudflare API permissions in Craft's existing \`docs/src/content/docs/targets/cloudflare.md\` page, not in \`getsentry/publish\` or a duplicate page. Chose mode-specific account permissions over a broad combined permission set because \`deployType: worker\` needs \`Account → Workers Scripts → Edit\`, while \`deployType: pages\` needs \`Account → Cloudflare Pages → Edit\`; a token serving both modes needs both. User, zone, billing, account-settings, and unrelated read permissions are unnecessary. + + + +- **Craft publish state never reads repo-local files**: Craft never reads repository-local publish state. Chose workflow-provided XDG state paths over deriving \`.craft-publish-\.json\` in the checkout because checkout contents are attacker-controlled and duplicate state-key logic drifts from Craft. The publish workflow computes the canonical state path, passes it as \`CRAFT_STATE_FILE_PATH\` for issue recovery, and Craft uses \`CRAFT_PUBLISH_STATE_GITHUB_REPO\` only to identify its secure state filename. + - **Craft workspaces redesign: PR sequence status**: Multi-product workspaces redesign for getsentry/craft. PR A (#847): merged. PR B (#848, schema/resolver/selector): all Bugbot findings fixed (42d17f0), adversarial review SOLID/MERGE, CI green — HELD pending Burak Yigit Kaya's manual review/merge. PR C (publish-state workspace threading): branched as feat/workspaces-threading off feat/workspaces-schema (not master) so PR B's resolver is available; will rebase if #848 changes. Research found only ONE functional gap — getPublishStatePath()/getPublishStateFilename() (src/utils/publishState.ts) lacked workspace in its key, risking same-repo/cwd/version workspaces overwriting each other's publish state (see \[\[NEW_PUBLISHSTATE_ENTRY]]). Everything else (version resolution, changelog, release branch naming, target providers) already flows through the workspace-resolved getConfiguration(), needing no code changes. Implemented + tested (5 new tests, 1093 passed, tsc/lint/prettier clean). PR C review/opening HELD until #848 settles to avoid basing on a moving foundation. Sequence: A✅ → B (awaiting merge) → C (implemented, held) → D (action-layer) → E (docs). + + +- **createReleaseBranch remote guard**: Chose an exact \`git ls-remote --heads \ refs/heads/\\` check before any preparation over checking local branches or silently advancing the version, because fresh clones can lack a pending remote release branch and later hit a non-fast-forward push. If the selected remote already has the exact ref, preserve it and fail with guidance to resume and publish that release. + + + +- **discover-location.js workflow discovery**: Chose runnable \`src/publish/discover-location.js\` over embedded JavaScript in \`publish.yml\` Bash/YAML for workspace discovery. Inline workflow code looks local and convenient, but is difficult to test, reuse, validate, and review; the Node entry point centralizes JSON validation and yields focused unit coverage. + + + +- **docs-preview.yml removed: defer previews to Cloudflare-side setup**: Chose to REMOVE the \`docs-preview.yml\` workflow and defer previews to Cloudflare-side configuration instead of maintaining CI-driven previews (\`wrangler versions upload\`). CI previews looked attractive because they integrate with PRs, but they introduce lifecycle complexity (cleanup, fork guards, artifact handling) and ongoing maintenance burden. By deferring to Cloudflare-native preview mechanisms, the system avoids CI orchestration entirely and keeps the pipeline focused on producing the \`cloudflare.zip\` artifact. The release path remains unchanged (CI builds + uploads artifact; Craft deploys). Rejected keeping the workflow because even the Cloudflare-based CI preview still duplicated logic better handled by the platform itself. + + + +- **getWorkspaceGlobMatches keeps globSync**: Chose synchronous, sorted \`globSync()\` workspace expansion over reusing the CLI async parallel walker because configuration loading is deterministic and release-safety-critical, while workspace discovery does relatively little I/O. The walker overlaps directory reads but yields completion-order results and would force async propagation through configuration resolution; retain lexical and realpath containment plus sorted output. Revisit bounded concurrency only after profiling demonstrates expansion is a bottleneck. + + + +- **publish-issue-title.peggy generated docs**: Chose \`src/modules/publish-issue-title.peggy\` as the single source of truth and generate the grammar block in \`docs/publish-issue-format.md\` from it. Maintaining hand-copied grammar in both files looks readable, but parser and public-format documentation drift independently; generation makes the documented language match the executable parser. + + + +- **publish-issue-validation shared identity predicates**: Chose shared \`publish-issue-validation.js\` repository/version/path predicates over trusting PEG title parsing independently in the controller and CI poller, because grammar acceptance is not a semantic safety boundary before authenticated API calls. Both paths must reject unsafe repository identities, non-Craft SemVer versions, and paths failing \`isPublishPath('.' + parsedTitle.path)\` before checkout, revision/check-run lookup, state creation, Craft invocation, or interpolated release-bot endpoints. Cover traversal, \`.\`, \`\_\_proto\_\_\`, and option-like paths; retain valid build metadata. + - **Rejected Alchemy IaC framework for craft's Cloudflare target**: Considered adopting Alchemy (alchemy.run/cloudflare, TS IaC framework, Terraform/Pulumi alternative for Cloudflare) for craft's Cloudflare target — rejected. Reasons: (1) state ownership mismatch — Alchemy is stateful IaC expecting to own the whole resource stack (Workers/D1/KV/R2/DNS), but craft targets are stateless publish steps shipping a prebuilt \`\*-cloudflare.zip\` artifact; (2) reverses craft's deliberate 'no new runtime dependency' choice to shell out to Wrangler \[\[019f664e-b8f0-7322-b858-e243a203b289]] — Alchemy is heavier and bun-first while craft runs on Node; (3) Alchemy's Workers-first/Pages-de-emphasized stance adds nothing Wrangler doesn't already give craft; (4) wrong layer — Alchemy belongs upstream, in a user's own build step, not inside craft's publish target. Only relevant if a downstream project (e.g. getsentry/toolkit docs site) wants IaC to manage its own CF resources — orthogonal to craft, which still just deploys built output either way. + + +- **tar-fs pinned at 1.16.6 via override (tar-stream@1.x preserved)**: craft pins tar-fs at 1.16.6 via root pnpm override \`tar-fs@<1.16.4: 1.16.6\` because \`@vercel/client\` exact-pins vulnerable 1.16.3 (GHSA-pq67-2wwv-3xjx / CVE-2024-12905 link-following + path traversal, GHSA-8cj5-5rvv-wf4v, GHSA-vj76-c3g6-qr5v). 1.16.6 (latest patched 1.x) keeps the tar-stream@1.x dependency tree; jumping to 2.x would switch tar-stream from 1.x to 2.x. In craft's usage the vulnerability is unreachable — @vercel/client only calls \`pack()\` (archive.js createTgzFiles); the advisories are in \`extract\` — but dependency-review gates the merge anyway. Verified via npm audit API: 1.16.4 and 1.16.6 report 'advisories: none'. + + + +- **WorkspaceSchema partial github**: Chose \`GitHubGlobalConfigSchema.partial()\` for workspace-level \`github\` overrides over reusing the required top-level schema because workspaces inherit omitted \`owner\` and \`repo\` fields and may override only supported fields. Reusing the full schema looks consistent, but rejects valid inherited configurations with \`workspaces.\.github.owner: Required\`. The top-level schema remains required; workspace \`projectPath\` remains explicitly unsupported. + ### Gotcha + + +- **action.yml PATH_INPUT validation and ambient workspace**: Validate \`inputs.path\` as a safe concrete relative checkout path before every Action side effect, under \`LC_ALL=C\`: each slash-separated segment must match ASCII \`\[A-Za-z0-9\_.-]+\` and must not be empty, \`.\`, \`..\`, \`\_\_proto\_\_\`, or start with \`-\`. This preserves paths such as \`packages/CLI\` while blocking traversal, prototype, and option-like values. Clear \`CRAFT_WORKSPACE\` for both Craft invocations unless an explicit Action workspace input is supplied, so inherited ambient selection cannot diverge from controller state. + - **actions/create-github-app-token v3 deprecated app-id input**: \*\*actions/create-github-app-token v3 required + app-id deprecated\*\*: v3 is needed for Node.js 24 runners (v2 uses Node 20, deprecated). v3 also deprecated the \`app-id\` input in favor of \`client-id\`. When the input references a variable named \`\*\_APP_ID\`, the variable value may already be a client ID (check \`vars.SENTRY_INTERNAL_APP_ID\` vs \`vars.SENTRY_RELEASE_BOT_CLIENT_ID\`). Replace both the action version and the input name. Affects all workflow files using the action. @@ -68,10 +112,6 @@ - **astro@7 docs build fails: root vite ^7.3.5 override downgrades astro's vite 8**: Trap: \`astro build\` in \`docs/\` fails with \`rollupOptions.input should not be an html file when building for SSR\` after rebasing onto master. Looks like a vite 7.x SSR regression. Root cause: master's root \`pnpm.overrides\` has \`vite: ^7.3.5\` (root CVE fix); \`astro@7.1.4\` requires \`vite ^8.0.13\`, so the root override DOWNGRADES astro's vite to 7.x, which astro 7's static-build cannot consume. Fix: add docs-local \`pnpm.overrides\` \`vite: ^8.0.0\` (or \`^8.0.13\`) — pnpm's most-specific workspace-local override wins for the docs subtree, leaving root on vite 7. Verified: docs build succeeds with vite@8.1.5. - - -- **auto-approve race: can fire before ci-pending.yml adds its label**: \*\*Resolved in getsentry/publish PR #7881\*\*: Eliminated the race by restructuring the label flow — \`ci-pending\` is now added by \`waiting-for-ci\` in \`publish.yml\` only AFTER \`accepted\` is present, not by a separate \`ci-pending.yml\` on \`issues:opened\`. The \`ci-pending.yml\` workflow was deleted. New flow: craft creates issue → (auto-approve or human) adds \`accepted\` → \`waiting-for-ci\` job adds \`ci-pending\`, comments, and triggers the poller → poller swaps to \`ci-ready\` when CI passes → \`publish.yml\` fires. The publish gate also requires \`label == accepted || label == ci-ready\`, so publish can never fire from the initial \`accepted\` label event alone — it must wait for the poller's \`ci-ready\` transition. This closes the race regardless of label ordering. - - **bun pm pack reads workspace versions from bun.lock, not package.json**: \`bun pm pack\` rewrites \`workspace:\*\` specifiers to concrete versions at pack time, reading them from \`bun.lock\` — NOT from the workspace \`package.json\`. If craft bumps package.json but leaves bun.lock stale, published tarballs point at old versions (ETARGET on install). Fix in \`NpmTarget.patchBunLock\` (src/targets/npm.ts): regex-patch bun.lock per workspace path-key. CRITICAL regex gotcha: use \`\[^{}]\*?\` (not \`\[\s\S]\*?\`) to bound the match to the workspace's own block — \`\[\s\S]\*?\` walks past a versionless workspace's closing \`}\` and silently rewrites the NEXT workspace's version. Normalize \`\\\` → \`/\` for Windows; escape regex chars (reuse \`escapeRegex\` from \`src/utils/filters.ts\`, don't duplicate). Only call when \`isWorkspace\` is true — otherwise non-workspace bun repos get a spurious 'lockfile out of sync' warning. Idempotent; uses \`safeFs.writeFileSync\`. @@ -80,10 +120,22 @@ - **CI poller needs release-bot token for private repo CI status checks**: The \`getsentry/publish\` CI poller's cross-repo API calls (commit status, check suites, git refs) require the \`sentry-release-bot\` app token with \`owner: getsentry\` scope — not the \`sentry-internal-app\` token. The internal app isn't installed on all getsentry repos (e.g. \`sentry-xbox\`, \`sentry-playstation\`, \`sentry-switch\`, \`service-registry\`), causing 404s on all CI status API calls. Combined with the \`gh api --jq\` stdout error leak, this silently leaves issues stuck in \`ci-pending\` indefinitely. Fix (PR #7843): added a separate \`release-bot-token\` step and a \`gh_api_release\` helper that uses \`GH_TOKEN="$RELEASE_TOKEN"\` for all cross-repo calls. + + +- **ci-poller body trailing newlines**: Trap: decoding GitHub issue bodies into Bash variables or \`updated_body=$(...)\` looks lossless, but command substitution strips every trailing newline and \`jq -r\` adds one on output. Fix: write \`.body\` with \`jq -jr\` to a temporary file, pass \`PUBLISH_ISSUE_BODY_FILE\` to the resolver, write rewritten \`.issueBody\` with \`jq -jr\`, and call \`gh issue edit --body-file\`. This preserves every byte except the indexed canonical SHA replacement. + + + +- **ci-poller subshell continue fallthrough**: Trap: wrapping each CI-poller iteration in a subshell looks ideal for per-issue \`EXIT\` cleanup, but \`continue\` is no longer inside the enclosing \`while\`; Bash errors, then later commands can edit with invalid data. Fix: use \`exit 0\` for intentional per-issue skips inside the subshell, and reserve \`continue\` for the outer loop. Keep the per-issue trap and regression-test that every resolver/decode failure skips \`gh issue edit\`. + - **Cloudflare Pages: use top-level production_branch, not source.config.production_branch**: Trap: Cloudflare Pages \`GET /accounts/{id}/pages/projects/{name}\` returns both a top-level \`result.production_branch\` and a nested \`result.source.config.production_branch\` — the nested one looks authoritative but only describes the linked git repo's branch setting. Fix: wrangler's \`deploy2()\` reads the top-level field, comparing it to \`--branch\` to compute isProduction; any mismatch silently produces a \*preview\* deploy (no error). Craft's cloudflare target replicates this in \`resolveProductionBranch()\`: (1) use configured \`productionBranch\` if set; (2) else call the same GET via raw \`fetch\` to infer it — this needs no extra token scope beyond the \`Account → Cloudflare Pages → Edit\` scope deploy already requires (wrangler makes the same GET internally); (3) else omit \`--branch\` — a bare deploy from a non-git temp dir defaults to production. 404 (project not found) is a hard-fail via \`reportError\`; transient 5xx/network errors soft-fail (warn + omit \`--branch\`). + + +- **core.setOutput JSON result**: Trap: passing a plain object to \`core.setOutput()\` looks natural because downstream workflows use \`fromJSON()\`, but GitHub Actions coerces it to \`"\[object Object]"\`. Fix: \`JSON.stringify()\` every structured Action output, including \`inputs.js\` and \`resolve-location.js\` \`result\` values, because \`fromJSON()\` requires JSON text; scalar outputs such as a revision remain plain strings. + - **Craft changelog: commits without PRs were forced into leftovers regardless of category match**: In \`src/utils/changelog.ts\`, the leftovers guard at the categorization step had \`if (!categoryTitle || !raw.pr)\` — the \`|| !raw.pr\` condition forced ALL commits without associated PRs into the "Other" leftovers section, even when they matched a category via \`commit_patterns\` or labels. This meant direct pushes (no PR) like \`feat(auth): add SSO\` would correctly contribute to version bump calculation (which runs before the leftovers check) but would appear under "Other" in the changelog instead of their matched category. Fix: remove \`|| !raw.pr\` so only \`!categoryTitle\` controls leftover placement. The downstream rendering already handles PR-less commits gracefully — \`createPREntriesFromRaw\` uses \`raw.pr ?? ''\`, and \`formatChangelogEntry\` falls back to commit-hash links when \`prNumber\` is falsy (empty string). @@ -100,33 +152,17 @@ - **Craft cloudflare target: reportError() inside its own try block gets swallowed by that block's catch**: Trap: calling \`reportError()\` (which throws, meant to hard-fail on real misconfig) from inside a try block whose catch is designed to soft-fail transient errors looks correct — reportError should propagate as a rejection. But the surrounding catch can't distinguish 'intentional hard-fail' from 'unexpected error' and swallows it into a warning instead, silently degrading a hard-fail into a soft-fail. Fix: restructure so the try/catch only wraps the network fetch + JSON parse and returns a result; check the result and call \`reportError()\` OUTSIDE that try block for real misconfig (e.g. 404 project-not-found), keeping soft-fail warn+continue for transient 5xx/network errors inside. Applied in \`resolveProductionBranch()\`, src/targets/cloudflare.ts. - + -- **craft docs Astro upgrade: Starlight + content.config.ts + vite constraints**: The \`docs/\` workspace uses \`@astrojs/starlight\`. Bumping Astro needs lockstep Starlight + content-collection migration. Astro 6: Starlight ≥0.38.0 (0.37.x fails \`processedDirs is not iterable\`). Astro 7 (e.g. 7.1.4): (1) move \`src/content/config.ts\`→\`src/content.config.ts\`, use \`glob({ pattern: '\*\*/\*.{md,mdx}', base: './src/content/docs' })\` from \`astro/loaders\` (legacy collections removed); (2) glob MUST include \`.mdx\` or \`index.mdx\` drops and sidebar \`slug:'index'\` 404s; (3) Starlight ≥0.39 removed \`autogenerate\`-with-\`label\` — use \`{ label:'Targets', items:\[{ autogenerate:{ directory:'targets' } }] }\`; (4) astro@7.1.4 needs \`vite ^8.0.13\` — a root \`vite: ^7.3.5\` override breaks docs build (see root-vite-override gotcha). Dependabot PRs bumping only \`astro\` fail \`Build Docs\` until all applied. +- **craft dependency-review: @vercel/client exact pins need pnpm overrides**: craft's dependency-review workflow runs \`fail-on-severity: high\` with no config file, so any high-severity transitive vuln BLOCKS merges. \`@vercel/client@18.2.5\` and friends exact-pin vulnerable versions with no newer release dropping them: js-yaml@4.1.1 (via @vercel/python-analysis), path-to-regexp@6.1.0 (@vercel/routing-utils@6.4.1, latest — it also carries the fixed one aliased \`path-to-regexp-updated\`), tar-fs@1.16.3 (direct, exact pin). Fixes are root pnpm.overrides: \`js-yaml: ^4.3.0\`, \`path-to-regexp@<6.3.0: ^6.3.0\`, \`tar-fs@<1.16.4: 1.16.6\` (1.16.6 chosen over 2.x to keep tar-stream@1.x; @vercel/client only uses pack, never extract, but CI gates anyway). Root overrides do NOT cascade to docs/ (separate lockfile) — check both. See \[\[019fa896-e1de-7551-bdb9-bce5ad328264]]. - **Craft docs/src/content/docs/configuration.md duplicates DEFAULT_RELEASE_CONFIG**: The \`Default Configuration\` YAML snippet in \`docs/src/content/docs/configuration.md\` (~lines 230-257) enumerates the default changelog categories verbatim and must be kept in sync with \`DEFAULT_RELEASE_CONFIG\` in \`src/utils/changelog.ts\`. When adding/removing/reordering categories in code, update this doc snippet in the same PR — otherwise users copy-pasting to customize get a silently-outdated config. No automated check exists. Related: \[\[019db138-9e4d-79f5-bf25-7379510c2b45]]. - - -- **Craft extractWorkspaceSelection: greedy token + relocated to helpers.ts**: Trap: \`--workspace \\` naively took the next argv token as the workspace value, so \`--workspace --dry-run\` returned '--dry-run' as the workspace name, and a bare trailing \`--workspace\` returned undefined and suppressed an already-set CRAFT_WORKSPACE env var instead of falling through to it. Fix (PR #848, committed 42d17f0): only accept the next token if \`next !== undefined && !next.startsWith('-')\`; otherwise break and fall through to \`process.env.CRAFT_WORKSPACE\`. Also relocated \`extractWorkspaceSelection\` from src/index.ts to src/utils/helpers.ts (an existing side-effect-free CLI helpers module) so it's unit-testable without importing index.ts, which has a top-level \`withTracing(main,...)()\` side effect — see \[\[019f8985-3aa7-7638-b114-dcab725c307c]]. Cursor Bugbot finding (Medium, bug f1f9647b) confirmed fixed. - - - -- **Craft index.ts redundant middleware clobbers pre-parse --workspace selection**: Trap: src/index.ts had BOTH a pre-parse \`setActiveWorkspace(extractWorkspaceSelection(argv))\` call (before yargs \`.parse()\`) AND a leftover post-parse \`.middleware(argv => setActiveWorkspace(argv.workspace as string | undefined))\` — the middleware looks like harmless redundancy/backup. But yargs sets \`argv.workspace\` to a falsy value for a bare/valueless \`--workspace\` flag, so the middleware re-ran AFTER the correct pre-parse selection and overwrote it, breaking \`CRAFT_WORKSPACE=cli\` + bare trailing \`--workspace\` (should resolve to 'cli', instead threw ConfigurationError). Found only via end-to-end verification against the rebuilt binary (dist/craft) — tsc/unit tests didn't catch it. Fix: delete the post-parse middleware entirely; the pre-parse extraction+setActiveWorkspace call is the single source of truth. getsentry/craft PR #848, commit 42d17f0. - - - -- **Craft PR review: check git status, not just committed diff — working tree can hide the real fix**: Trap: \`git diff origin/master...HEAD\` only shows committed content, so it looks like 'the diff' of a branch under review. But intended review fixes can exist only as uncommitted working-tree changes (visible via \`git status\`, invisible to that diff) — reviewing the committed diff alone gives a stale picture. Fix: always run \`git status\` first; if modified/untracked files exist beyond the commit, read actual working-tree file contents to review the true intended state, and flag as CRITICAL that they must be \`git add\`+committed (or amended into the existing commit) before push, since CI only sees committed state. Confirmed twice in getsentry/craft PR A review (July 21 2026): first at commit \`412b796\` (old segment-count logic, no try/catch), then reconfirmed as CRITICAL-0 in final adversarial pass before the fixes were amended into commit 1afce59 and shipped as PR getsentry/craft#847. - - - -- **Craft publish-issue pipeline disambiguates products only by version+subdirectory, not workspace id**: Mechanism confirmed via source read: \`action.yml:241\` builds issue title as \`publish: ${GITHUB\_REPOSITORY}${SUBDIRECTORY}@${RESOLVED\_VERSION}\`; SUBDIRECTORY is empty when \`inputs.path == '.'\`. Existing-issue lookup matches purely by title string (\`gh issue list --json title | jq select(.title==$t) | first\`) — no workspace/product id involved. NOT a live bug today: the convention of one \`.craft.yml\` per product, each with a distinct \`path\` subdir, makes titles differ by construction, sidestepping collision. It becomes actively reachable under a single-file multi-product \`workspaces:\` model: two products released from repo root at the same version, both with open publish issues simultaneously, would collide on title, and the action would edit the WRONG issue — merging/overwriting the \`### Targets\` checklist (\`action.yml:257-316\`) into a scrambled union, risking a mixed publish. This is why the workspaces redesign's action-layer fix (additive title change embedding workspace name) is non-negotiable and must ship together with the \`--workspace\` selector, never separately. See \[\[019f85aa-3d3d-702b-a703-b36adf189438]]. - -- **Craft publishState.ts: filename missing workspace key causes cross-workspace collision**: Trap: getPublishStateFilename()/getPublishStatePath() (src/utils/publishState.ts) key the publish-state filename on owner/repo/cwd-hash/version only — this looked complete since it already handles monorepo subpath disambiguation via githubConfig.projectPath. But under the \`workspaces:\` model, two different products in the same repo/cwd releasing the same version produce an IDENTICAL filename and silently overwrite each other's publish state. Fix (getsentry/craft, branch feat/workspaces-threading): added an optional \`workspace\` param, threaded via \`getActiveWorkspace()\` at the publish.ts call site, appended into the filename key: \`publish-state-\-\-\-\-\.json\`. Omitting workspace (base/no-workspace projects) yields the byte-identical old filename — fully backward compatible. Note: the legacy pre-XDG state file path (\`.craft-publish-\.json\`) is only read to emit a migration warning, never actually read for state — not a collision risk. +- **Craft publishState.ts: filename missing workspace key causes cross-workspace collision**: Trap: keying publish-resume state by sanitized version looks stable because filenames need safe characters, but distinct valid SemVer build metadata such as \`4.2.6+sentry1\` and \`4.2.6+Sentry1\` collapse to one state file and silently skip targets. Fix: preserve legacy filenames for versions unchanged by sanitization; when sanitization changes a version, use a lossless Base64URL version component. Apply the identical rule in Craft \`publishState.ts\` and Publish \`Set targets\`; retain lossless Base64URL workspace isolation and never read repository-local state. @@ -140,37 +176,69 @@ - **Craft src/index.ts has top-level side effect — never import it in tests**: Trap: extracting a testable helper (e.g. --workspace flag parsing) into src/index.ts and importing it directly in a test file looks convenient. But index.ts ends with \`withTracing(main, {...})()\` executed at module load — importing it anywhere runs the actual CLI as a side effect. Fix: don't export helpers from index.ts for direct unit testing; instead verify behavior through a lower-level module (e.g. assert getConfiguration().targets resolves correctly post-setActiveWorkspace in config.test.ts) or verify end-to-end via the built binary (\`node build.mjs\` then run \`dist/craft ...\`). Found during getsentry/craft PR #848 (feat/workspaces-schema) review. + + +- **Craft workspace selection must be parsed before yargs builders and never overwritten**: Parse Craft workspace selection from raw argv before yargs builders and set it once: builders run before middleware, so builder-time configuration otherwise misses \`--workspace\`. Use \`parseArgs({ tokens: true })\` and explicitly narrow \`token.kind === 'option'\` before reading option-only fields such as \`value\` or \`inlineValue\`. For separated \`--workspace\`, accept only a following non-flag string; accept inline \`--workspace=-foo\`; empty, bare, or flag-followed forms fall back to \`CRAFT_WORKSPACE\`. Repeated options use the last value, but a final valueless option clears earlier CLI selection. Do not let middleware overwrite this selection with a falsy bare option. Builders needing target choices should tolerate configuration failure and fall back to all target names. + - **Craft workspace-level github override needs partial schema, not GitHubGlobalConfigSchema**: Trap: reusing GitHubGlobalConfigSchema (requires owner+repo) for a workspace's per-workspace \`github\` override field looks natural — it's the same shape as top-level \`github\`. But a workspace override must allow specifying ONLY projectPath while inheriting owner/repo from the base config; requiring owner/repo again broke validateConfiguration with 'workspaces.\.github.owner: Required' for every workspace test config. Fix: give the workspace-level \`github\` field its own partial schema (all fields \`.optional()\`), while the top-level \`github\` schema stays required (non-passthrough per \[\[019f85aa-3d99-71cd-95ec-afc4cb7f065f]]). Found in getsentry/craft PR #848 (feat/workspaces-schema). - + -- **Craft yargs CLI: builder() runs before middleware() — getConfiguration() in a builder misses --workspace**: Trap: a yargs command builder calling getConfiguration() (e.g. publish.ts:60, for --target CLI choices) looks safe since builders run 'at parse time'. But execution order is BUILDER → MIDDLEWARE → HANDLER — the setActiveWorkspace middleware runs AFTER builders, so builder-time getConfiguration() sees no active workspace and throws, breaking \`craft publish\` entirely whenever \`workspaces:\` is configured. Fix shipped in PR #848 (dual approach): (1) primary — manually parse --workspace/CRAFT_WORKSPACE from raw argv/env and call setActiveWorkspace() in src/index.ts BEFORE yargs \`.parse()\` (existing middleware kept as backup); (2) secondary — wrap publish.ts builder's getConfiguration() in try/catch, falling back to getAllTargetNames() (Object.keys(TARGET_MAP)) on failure, also fixing --target choices being scoped to base config instead of the selected workspace. Verified end-to-end via built dist/craft: correct errors for no-workspace/unknown-workspace, correct resolution for --workspace, --workspace=X, and CRAFT_WORKSPACE env. +- **CRAFT_PUBLISH_STATE_GITHUB_REPO identity override**: Trap: using the workspace-resolved \`getGlobalGitHubConfig()\` for publish-state identity appears consistent, but a workspace may publish through a release repository different from the controller’s checkout repository. The controller then cannot locate Craft’s retry state. Fix: \`getPublishStateGitHubConfig()\` accepts only \`owner/repo\` from \`CRAFT_PUBLISH_STATE_GITHUB_REPO\` and applies it only to \`getPublishStatePath()\`; target construction and publishing retain the resolved workspace GitHub config. Reject malformed overrides with \`ConfigurationError\`. Chose a narrow state-key override over replacing global GitHub configuration because replacing it silently redirects publish behavior. - **Craft: local \`pnpm lint\` doesn't catch CI's prettier check — use \`pnpm format:check\`**: Trap: \`pnpm lint\` (ESLint only) passing locally looks like a full pre-push check, but CI's 'Lint fixes' job runs \`pnpm format:check\` (prettier --check .) separately — a file can pass ESLint yet fail CI on pure formatting (e.g. line-wrapping in \`src/targets/\_\_tests\_\_/cloudflare.test.ts\`). Fix: run \`pnpm format:check\` (or \`prettier --write \\`) locally before pushing, not just \`pnpm lint\`. Confirmed in getsentry/craft PR #846. + + +- **Dependabot alerts stay open right after merge — re-scan auto-closes**: Trap: right after a security-fix PR merges, its Dependabot alerts still show OPEN — looks like the fix didn't take, which invites an immediate re-fix. Dependabot must re-scan the default branch post-merge and typically auto-closes fixed alerts within minutes. Fix: do NOT re-trace chains or open a new fix PR immediately after merge; schedule a ~5-minute re-check, then reconcile which alerts actually closed vs. remained. Remaining ones are genuinely unfixed (e.g. the still-open js-yaml/smol-toml/cookie/brace-expansion/svgo set on getsentry/craft \[\[019fdd1b-e108-7cf6-b74e-64921a4750a1]]). Complements the merge pipeline \[\[019fdd20-3623-7c80-9987-2ee77f26354f]], which says investigate NEW alerts — this clarifies timing for pre-existing ones. + - **Dependabot phantom alert: lockfile version outside vulnerable range**: Trap: a GitHub Dependabot alert stays OPEN even though the lockfile already resolves to a version OUTSIDE the vulnerable range. Looks like a real vuln needing a fix. Concrete case: svgo GHSA-2p49-hgcm-8545 (range >= 1.0.0, < 2.8.3) alert #197 on getsentry/craft \`docs/pnpm-lock.yaml\`, but the lockfile resolves svgo@4.0.1 (4.0.1 > 2.8.3 = patched). GitHub's alert state was stale and never reconciled. Fix: grep the lockfile for the resolved version; if outside the vulnerable range, the alert is phantom — no code change needed. Don't chase it. - + + +- **detailsFromContext repo version validation**: Trap: validating only the parsed publish path looks sufficient because the PEG grammar limits repository and version characters, but option-like/prototype repository names can reach checkout and non-SemVer versions can reach state creation and \`craft publish\`. Fix: fail closed in \`detailsFromContext()\` before workflow side effects: reject unsafe repository identities and require Craft-compatible \`isValidVersion\` SemVer validation. Preserve valid build metadata such as \`4.2.6+sentry1\`; grammar parse failure alone is not a field-specific safety boundary. + + + +- **detailsFromContext workspaceJson escapes**: Trap: extracting \`\[workspace: "..."]\` with a permissive regex looks sufficient because ordinary quoted names match, but invalid JSON escapes such as \`cli\qnext\` still match and make \`JSON.parse(workspaceJson)\` throw an unhandled \`SyntaxError\`. Fix: wrap that parse in \`try/catch\` and convert failures into the module’s contextual validation error, because issue content is untrusted and malformed metadata must fail cleanly. + + -- **Docker Hub CDN propagation lag causes transient 'not found' on freshly-published Node tags**: Trap: immediately after a new \`node:X.Y.Z-bookworm-slim\` tag is published to Docker Hub, CDN propagation can lag by several minutes — the image job fails with \`failed to resolve source metadata … not found\` even though the tag exists. Looks like the tag was never published, but it's a transient CDN issue. Fix: simply re-run the failed image job (don't roll back the Dockerfile pin). Confirmed during PR #838 (Node 24.18.0): image job failed 20s after flip-to-ready, rerun succeeded ~8 minutes later. +- **discover-location empty Craft JSON**: Trap: \`JSON.parse()\` of Craft workspace output looks adequate because malformed JSON throws, but empty output leaks raw \`Unexpected end of JSON input\` instead of the controller’s contextual discovery failure. Fix: catch all JSON parse errors and throw \`Craft workspace discovery returned an invalid workspace list.\`, then require an array; workflow diagnostics must identify failed workspace discovery rather than JSON internals. + + + +- **discoverLocation root bypasses workspace discovery**: Chose returning \`{ path: "." }\` immediately for an exact root input over validating \`workspaceNames\` first, because root publishing has no possible workspace classification and must work when Craft workspace discovery is unavailable. Trap: routing root through \`getWorkspaceNames()\` looks uniformly fail-closed, but a broken or unavailable \`craft workspace list\` blocks valid root releases unnecessarily. Fix: only exact \`"."\` bypasses discovery; every non-root path must still discover and validate workspace names, then fail closed on discovery errors. Input path validation remains upstream in \`detailsFromContext()\` before workflow location resolution. - **Edit tool triggers Prettier reformatting on the entire file**: When using the Edit tool to make a targeted change to a file in a repo with Prettier configured, the tool may reformat the entire file (e.g., aligning markdown table columns, changing quote styles). This causes the git diff to show cosmetic changes far beyond the intended edit. To keep commits clean: either accept the Prettier reformatting as a net improvement (if the file passes \`prettier --check\`), or use \`git checkout -- \\` to restore the original and re-apply the change via bash/sed for surgical precision. In this codebase, Prettier is configured (\`.prettierrc.yml\`) but the lint CI workflow does NOT run \`prettier --check\`, only ESLint and typecheck. + + +- **findConfigFile \_configPathCache temp tests**: Trap: changing \`process.cwd()\` to a new temporary fixture appears to make \`getWorkspaceNames()\` discover its \`.craft.yml\`, but \`findConfigFile()\` returns the module-level \`\_configPathCache\` first. A prior fixture cleanup then produces ENOENT for its deleted config, which looks like glob-resolution failure. Fix: reset discovery through the public cache-clearing path between fixture directories; do not test workspace glob behavior against a stale cached config path. + + + +- **generate-publish parser docs markers**: Trap: counting only exact generated-doc markers looks sufficient because a valid BEGIN/END pair is unique, but duplicate or malformed marker-like fragments (such as \`\ + +- **getRevisionBranchName detached SHA fallback**: Trap: \`git name-rev --name-only --no-undefined \\` looks required before checkout because named release branches are preferred, but a CI-approved detached SHA has no name and must remain publishable. Fix: \`getRevisionBranchName()\` catches the lookup failure and returns \`''\`; \`publishMain()\` checks out \`branchName || rev\`. This preserves named-ref behavior while allowing exact immutable CI revisions. + - **getsentry/craft bootstrap deadlock: gcs target runs before docker in .craft.yml — gcs failure blocks Docker Hub update**: Trap: craft's \`.craft.yml\` target order is npm → gcs → registry → docker → github → gh-pages. The publish loop is sequential + fail-fast. If gcs fails (e.g., due to Node regression), docker targets never run, so \`getsentry/craft:latest\` on Docker Hub never gets the fixed image. The fixed image IS built and pushed to \`ghcr.io/getsentry/craft:latest\` by the \`image\` workflow on master merge, but Docker Hub remains stale. Recovery: manually mirror \`ghcr.io/getsentry/craft:\\` → Docker Hub, or retry publish after gcs is unblocked. Reordering \`.craft.yml\` to put docker before gcs would require two separate runs (npm already published on first run). Node 24.18.0 upgrade (PR #838) merged 2026-06-26; new release cut immediately after merge. -- **getsentry/craft Dockerfile uses floating Node base image — no automated bump guardrail**: Trap: craft's Dockerfile uses \`node:22-bookworm-slim\` / \`node:22-bookworm\` (floating tags), so a new Node minor/patch release is silently picked up on the next Docker build. No Dependabot or Renovate is configured for the Dockerfile. Fix: pin to an exact version and update both builder (line 1) and runtime (line 20) stages together. Also align \`volta.node\` in \`package.json\` to the same version. PR #837 pinned to 22.23.1; PR #838 upgraded to Node 24.18.0 (LTS Krypton), squash-merged 2026-06-26. Docker Hub CDN propagation lag (~minutes) can cause \`not found\` on freshly-published tags — retry the image job rather than rolling back. +- **getsentry/craft Dockerfile uses floating Node base image — no automated bump guardrail**: Craft's Dockerfile must not use floating Node tags such as \`node:22-bookworm-slim\`: pin an exact Node version in both builder and runtime stages, and align \`volta.node\` in package.json. No automated Dockerfile bump guardrail exists. Newly published exact Docker Hub Node tags can briefly return \`not found\` while CDN propagation completes; retry the failed image job after a few minutes rather than reverting a valid pin. @@ -180,18 +248,54 @@ - **getsentry/craft: tar is a pinned devDep (no caret) — must bump the pin directly, not via override**: Trap: \`tar\` looks like a transitive dep that should be fixed via \`pnpm.overrides\`, because most security fixes use overrides. Fix: \`tar@7.5.11\` is a direct devDependency pinned without a range operator at \`package.json:65\` — bump the pin directly to \`7.5.16\` and run \`pnpm install\`. An override would be redundant and confusing. Vulnerable range: \`<= 7.5.15\` (GHSA-vmf3-w455-68vh, Alerts #180/#181). Confirmed tar@7.5.16 exists on npm. Applied in branch \`byk/fix/dependabot-security-alerts\`. - + + +- **getsentry/craft:latest Publish image contract**: Chose \`getsentry/craft:latest\` over an immutable controller image such as \`getsentry/craft:2.31.0\` because Publish always uses the latest Craft release; Craft must release required features before controller rollout. This does not weaken revision integrity: Publish checks out and runs \`craft publish --rev\` against the exact CI-approved SHA. Trap: pinning the controller image looks reproducible, but it violates the Publish/Craft release contract and stalls newly released functionality. + + + +- **getsentry/publish prettier script rewrites src**: Trap: running \`yarn prettier \\` looks like targeted formatting, but the Yarn Classic script hard-codes \`prettier --write src\`; appended paths do not constrain it. It rewrites unrelated baseline files and then fails on \`src/modules/publish-issue-title.peggy\` because Prettier has no parser. Fix: do not use that script for scoped changes; restore formatter-only churn, format only supported files with a direct command if needed, then use ESLint, generated-parser check, and Vitest for Publish verification. + + + +- **getWorkspaceGlobMatches brace expansion**: Trap: validating only nonempty outer brace expansion looks sound because each returned alternative is checked, but \`flatMap\` silently drops malformed nested branches: \`packages/{cli,{mcp}}\` yields \`cli\` and passes. Fix: make brace expansion propagate an explicit invalid result through every recursive branch; require a valid, nonempty complete expansion before validating each expanded POSIX glob/path. Apply identically in \`src/schemas/project_config.ts\` and \`src/config.ts\`. Permit balanced nested braces; reject malformed, empty, absolute, traversal, prototype, option-like, or unsafe alternatives. Do not recurse expanded strings through \`hasMagic()\`, which can stack-overflow. + + + +- **getWorkspaceGlobMatches symlink containment and realpath errors**: Trap: lexical workspace-root checks after \`globSync()\` look sufficient, but glob emits broken-symlink candidates and symlinks can physically escape the repository. Fix: validate each match as a safe path and lexically contained, then \`realpathSync()\` it and require containment under the real root plus \`lstatSync(...).isDirectory()\`. Skip only \`ENOENT\` from \`realpathSync()\` (a vanished or broken link); rethrow \`ELOOP\`, \`EACCES\`, \`EPERM\`, I/O, and programming errors so unsafe filesystem state is never masked. Keep sorted results. Regression tests must prove installed glob emits the broken link, exclude it, and prove an \`ELOOP\` link propagates. -- **gh pr merge fails: master checked out in another worktree**: Trap: \`gh pr merge --squash --admin\` fails with \`fatal: 'master' is already used by worktree at /home/byk/.local/share/opencode/worktree/...\` when \`master\` is checked out in another opencode worktree. Looks like a CLI auth or branch-protection error. Root cause: the gh CLI does a local \`git checkout master\` to perform the merge, which conflicts with the existing worktree's checkout of master. Fix: use the GitHub API admin merge — \`gh api PUT /repos/getsentry/craft/pulls/{n}/merge -f merge_method=squash\` (repo admins bypass branch protection via API). This is the standard merge path for self-reviewed craft PRs in this environment. Happened on both PR #854 and #855 merges. + - +- **getWorkspaceNames overlapping globs**: Trap: expanding workspace globs into a deduplicated list looks sufficient because output has no duplicates. Fix: map each concrete directory to every matching configured key and reject any directory with multiple matches; otherwise release configuration is ambiguous. Expand only safe, repository-contained globs relative to the explicit configuration directory, keep directories only, ignore \`node_modules\`, sort results, and preserve exact spelling. -- **git rebase --continue fails in dumb terminal: set GIT_EDITOR**: Trap: \`git rebase --continue\` fails with \`Terminal is dumb, but EDITOR unset\` / \`could not commit staged changes\` in agent shells (dumb TERM, no EDITOR). Looks like a rebase conflict but is just git needing an editor for the commit message. Fix: set \`GIT_EDITOR=true\` (or \`:\`) before continuing — the original commit message is already present, so a no-op editor suffices. \`git rebase --continue -m\` is INVALID (no \`-m\` for \`--continue\`). Same EDITOR/dumb-terminal root cause as the prepare-dry-run e2e test failures. + + +- **gh api Bash backticks**: Trap: embedding Markdown code spans in a double-quoted Bash payload for \`gh api\` looks like ordinary text, but backticks execute command substitution before GitHub receives the reply, silently stripping text and emitting \`command not found\`. Fix: send plain text or a safely quoted JSON body/file; inspect the posted comment, delete malformed replies, and repost corrections. + + + +- **gh api review replies/reviewers: form strings 422 — use JSON body**: \`gh api\` review endpoints reject form-encoded strings: \`gh api -f in_reply_to=3730403846 -f commit_id=...\` returns HTTP 422 (oneOf subschema) because \`-f\` sends strings and \`in_reply_to\` must be an INTEGER; positioning (\`commit_id\`/\`path\`/\`line\`, or \`position\`) is also required. Fix: POST a raw JSON body via \`-d\`/\`--input\`, passing \`in_reply_to\` as a number plus valid positioning. Same class of bug: \`POST /pulls/{n}/review-requests\` requires \`reviewers\` as a REAL JSON array (\`{"reviewers": \["BYK"]}\`) — a JSON-encoded string \`"\[\\"BYK\\"]"\` also 422s. Both hit during PR #865 review cleanup. + + + +- **git stash pop without ref grabs another branch's stash**: Trap: after making changes, running \`git stash\` then bare \`git stash pop\` is natural — but it pops the LATEST entry in the repository-wide stash list, which may belong to ANOTHER branch. In getsentry/craft this popped stash@{0} 'lore-churn-pre-rebase' from fix/dependabot-security-alerts onto a PR worktree, flooding it with 72 staged + 5 deleted unrelated files and 7 merge conflicts — looks like lore-daemon churn, but is a mis-targeted stash. Fix: never bare-pop; inspect \`git stash list\` first and use \`git stash pop stash@{N}\`. Recovery: \`git reset --hard \\` clears the staged churn/conflicts; LEAVE the foreign stash in place (dropping it loses another branch's work) and don't commit its untracked leftovers. + + + +- **Git worktree conflict when master is checked out elsewhere**: If \`master\` is checked out in another worktree, any local operation that checks it out fails with \`fatal: 'master' is already used by worktree ...\`; this is not an auth or branch-protection failure. For PR merges, use the GitHub API admin merge, e.g. \`gh api PUT /repos/getsentry/craft/pulls/{n}/merge -f merge_method=squash\`, which avoids local checkout. For post-merge cleanup, detach at the merge commit (\`git checkout \\`) before deleting the branch rather than checking out \`master\`. Verify merge and remote-branch state separately. - **GitHub App tokens expire after 1 hour — breaks long-running CI publishes**: GitHub App installation tokens expire after 1 hour (non-configurable). For publish jobs exceeding this (e.g., sentry-native's ~1h 23m symbol upload), the token expires before Craft's post-publish \`git push\` for the release branch merge. Git fails with \`could not read Username for 'https://github.com': No such device or address\` — which looks like a credential config issue but is actually token expiration. No code change in Craft alone can fix this — the \`GITHUB_TOKEN\` env var, git \`http.extraheader\`, and Octokit all use the same expired token. The real fix requires the CI workflow (\`getsentry/publish\`) to generate a fresh token after the Docker container exits, before the merge step. + + +- **GitHub stale CHANGES_REQUESTED: only new review or dismissal clears**: GitHub does not clear a \`CHANGES_REQUESTED\` reviewDecision just because every thread is resolved. Re-requesting the reviewer only TRANSIENTLY clears it — observed CHANGES_REQUESTED → null → CHANGES_REQUESTED within ~1 minute (the cached decision re-appears because the stale review still exists). Durable clears are only: (a) the reviewer submits a NEW review on the latest commit (approve/comment), or (b) an admin dismisses the stale review. On craft PR #865, BYK chose (a): the assistant submitted a fresh APPROVED review on the new head on BYK's behalf, flipping reviewDecision to APPROVED and mergeStateStatus to CLEAN. Related: GraphQL \`resolveReviewThread\` DOES accept outdated threads (isOutdated: true) — GitHub allowed the mutation despite the assumption it would reject them. + + + +- **loadConfigurationFromString workspaceDirectory**: Trap: \`loadConfigurationFromString()\` appears equivalent to loading \`.craft.yml\` from disk, but workspace glob expansion historically called \`getConfigFilePath()\` and thus failed without a local config or used an unrelated one. Fix: thread an explicit workspace directory through workspace selection and glob resolution; \`prepare --config-from\` supplies Git’s repository top-level directory. This keeps remote configuration globs relative to the checked-out repository, not caller CWD discovery. + - **Lore tool seeds generic entries unrelated to the project — clean before committing**: The opencode-lore tool (https://github.com/BYK/opencode-lore) can seed AGENTS.md with generic/template lore entries that are unrelated to the actual project. These are identifiable by: (1) shared UUID prefix like \`019c9aa1-\*\` suggesting batch creation, (2) content referencing technologies not in the codebase (e.g., React useState, Kubernetes helm charts, TypeScript strict mode boilerplate in a Node CLI project). These mislead AI assistants about the project's tech stack. Always review lore-managed sections in AGENTS.md before committing and remove entries that don't apply to the actual codebase. Cursor BugBot will flag these as "Irrelevant lore entries." @@ -208,22 +312,50 @@ - **pnpm gotchas: lockfile conflicts, overrides, workspace cascade**: pnpm gotchas: (1) Lock file conflicts: never manually resolve — \`git checkout --theirs pnpm-lock.yaml\` then \`pnpm install\` to regenerate. \`git stash pop\` after merge can re-conflict; drop the stash and re-run instead. (2) Overrides: \`>=\` crosses major versions — use \`^\` to stay in-major. Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when all consumers are on same major. (3) Overrides go stale on tree changes — audit with \`pnpm why\` and remove orphans. (4) Root \`pnpm.overrides\` does NOT cascade into workspace sub-projects with their own lockfile (e.g. \`docs/\`). Concrete case: root \`postcss: ^8.5.10\` resolved to 8.5.15 (vulnerable, <= 8.5.17, GHSA-r28c-9q8g-f849) and left \`docs/\` on 8.5.15 too. Fix required adding \`postcss: ^8.5.18\` to BOTH root and \`docs/package.json\` (resolves 8.5.24). When fixing a transitive vuln, check EVERY workspace package.json + lockfile, not just root. - + -- **PR cleanup: 'master' worktree conflict blocks checkout — detach HEAD instead**: Trap: after merging a PR, checking out \`master\` locally (to delete the merged branch) looks like the natural cleanup step. But in this environment \`master\` is already checked out by another opencode worktree (e.g. \`.local/share/opencode/worktree/\/\\`), so \`git checkout master\` fails with \`fatal: 'master' is already used by worktree at ...\`. This recurred identically in PR #843 and #846 cleanups. Fix: instead of checking out master, \`git checkout \\` (detached HEAD) to leave the current branch, then \`git branch -D \\` to delete it. Verify remote branch deletion via \`gh pr view --json mergeCommit,mergedAt,state\` and a remote branch listing. - - - -- **prepare-dry-run e2e tests fail without EDITOR in dumb terminals**: The 7 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`TERM=dumb\` and \`EDITOR\` is unset (e.g., inside agent shells or minimal CI containers). The error is \`Terminal is dumb, but EDITOR unset\` from git commit. This is a pre-existing environment issue, not a code defect. These tests pass in normal CI (Node.js 20/22 runners) where terminal capabilities are available. +- **prepare explicit staging**: Trap: broadly staging the working tree before a Craft release commit looks convenient because preparation may modify several files, but it can sweep unrelated user or agent changes into the release. Fix: when no pre-release command ran, stage and commit only Craft-owned outputs such as the changelog; never include stray \`.lore.md\`, \`.opencode\`, or unrelated edits. -- **prepare-dry-run e2e tests require EDITOR env var for git commit**: The 6 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`EDITOR\` is unset and the terminal is non-interactive (e.g., headless CI agents, worktrees). The error is \`Terminal is dumb, but EDITOR unset\` from git refusing to commit without a message editor. These are environment-dependent failures, not code bugs. They pass in environments with \`EDITOR=vi\` or similar set. +- **prepare-dry-run e2e tests require EDITOR env var for git commit**: In dumb/non-interactive terminals, Git operations that need to retain or create a commit message can fail with \`Terminal is dumb, but EDITOR unset\` (including prepare dry-run e2e commits and \`git rebase --continue\`). This is an environment issue, not a code or conflict failure. Set \`EDITOR\` or \`GIT_EDITOR\` to a usable editor; \`true\` or \`:\` is sufficient when the existing message should be retained. \`git rebase --continue -m\` is invalid. - **Publish issue title monorepo suffix breaks owner/repo parsing**: Craft titles like \`publish: getsentry/relay/py@0.9.26\` have a subdirectory path suffix. Naive \`sed 's/^publish: \\(.\*\\)@.\*/\1/p'\` extracts \`getsentry/relay/py\` — causes 404 on every GitHub API call (\`repos/getsentry/relay/py/...\` isn't a valid repo). Fix: match only first two path segments: \`sed -n 's|^publish: \\(\[^/]\*/\[^/@]\*\\).\*@.\*|\1|p'\`. Applies to any workflow parsing publish issue titles (ci-poller.yml, auto-approve.yml, craft-action.yml's request-publish step). + + +- **PUBLISH_ARGS JSON fallback**: Trap: \`JSON.parse(process.env.PUBLISH_ARGS || '')\` looks like a harmless missing-value default, but an unset variable becomes an unhandled \`SyntaxError\` before deliberate validation runs. Fix: use a valid JSON fallback such as \`'{}'\` in runnable Publish entry points, because \`JSON.parse()\` must receive JSON and subsequent required-field validation can emit the intended contextual error. + + + +- **publish-issue-format Merge target contract**: \`Merge target\` is a required field in the canonical body-start request header, not a separately searchable issue-body property. Chose extracting it from the Peggy-parsed header over \`/^Merge target:/m\` scanning because a later decoy line can otherwise control Craft’s merge target while release revision remains canonical. Trap: requiring the field in grammar looks sufficient because the header validates, but discarding its value lets another parser read attacker-controlled content. Fix: return and validate the parsed header value as the sole authority; \`(default)\` maps to the repository default branch. + + + +- **publish.yml canonical cwd state hash**: Trap: hashing the textual publish path appears equivalent to Craft’s cwd hash, but \`packages/../cli\` and \`cli\` name the same directory with different bytes. Fix: \`publish.yml\` canonicalizes \`/github/workspace/\_\_repo\_\_/$CRAFT_PUBLISH_PATH\` with \`realpath -m\`, rejects paths outside the repository root, then SHA-1 hashes that canonical cwd exactly as Craft hashes \`process.cwd()\`. Test the actual workflow \`Set targets\` shell with root and workspace fixtures; unit-testing a reimplementation misses drift in title repository parsing, cwd hashing, Base64URL workspace encoding, and emitted state payload. Compare parsed JSON rather than literal formatting, because pretty-printing is not state semantics. + + + +- **publish.yml checkout@v7 parity**: Trap: upgrading only \`ci-poller.yml\` to \`actions/checkout@v7\` looks scoped because it is under direct review, but Publish also checks out its controller and CI-approved target repository. Fix: use \`actions/checkout@v7\` for all required Publish checkouts and assert versions in workflow tests; preserve unrelated workflow versions unless their scope independently requires an upgrade. + + + +- **publish.yml default-branch title inference**: Trap: resolving an ambiguous Publish title against the default-branch checkout looks authoritative because it contains Craft config, but release-branch workspace definitions may differ. Fix: new requests must carry dedicated versioned metadata binding the exact materialized release branch and SHA; Publish checks out that ref and requires CI success for that exact head before interpreting a trailing segment. Requests lacking the marker retain legacy path, checkout, and state semantics byte-for-byte. + + + +- **ReleaseRevision Peggy check-runs authority**: Use generated Peggy start rules, including \`ReleaseRevision\`, as the sole executable authority for Publish title, body-header, release-revision, and generated-doc grammar. Reject inline/blockquoted decoys and grammar drift by globally requiring exactly one exact \`View check runs\` marker, parsing only the body-start header, and requiring the supplied \`getsentry/\\`, a lowercase 40-hex SHA, and the required \`/checks\` path followed by an optional final \`/\`. Update only parser-indexed SHA offsets; do not use standalone regexes or bullet-prefix counting for authority or uniqueness checks. + + + +- **resolveProductionBranch expansion guard**: Trap: checking \`${VAR}\` expansion only in the configured Cloudflare \`productionBranch\` looks sufficient, but a branch inferred through the Pages API can contain the same unresolved syntax and reach Wrangler via \`--branch\`. Fix: apply the shared \`ENV_EXPANSION_REGEX\` inside \`resolveProductionBranch()\` after resolution; return \`undefined\` and omit \`--branch\` for both configured and API-inferred unresolved values. + + + +- **resolveWorkspaceConfig Object.hasOwn prototype names**: Trap: \`workspaces\[workspaceName]\` plus a truthiness check looks sufficient for a \`z.record\` config, but inherited names such as \`constructor\`, \`toString\`, and \`\_\_proto\_\_\` resolve from \`Object.prototype\` and can silently select the base release config. Fix: reject unless \`Object.hasOwn(workspaces, workspaceName)\` before indexing; this preserves explicitly configured own keys. Regression-test all three inherited names as unknown workspaces. + - **semgrep-code-getsentry\[bot] flags lockfile deps by CVE history, not version range**: Trap: \`semgrep-code-getsentry\[bot]\` posts inline review comments on \`pnpm-lock.yaml\` flagging packages (vite, vitest, fast-xml-parser) as High/Critical vulns. Looks like a real fix is needed. Root cause: the bot scans the lockfile for ANY version of a package with a CVE in its history, without checking whether the resolved version falls in the vulnerable range. In craft PR #854 it flagged vite@7.3.5, vitest@4.1.8, fast-xml-parser@5.8.0 — all already patched (confirmed via GHSA vulnerable ranges). Fix: cross-check against Dependabot (range-aware, authoritative for the repo) and verify the resolved version is actually in the vulnerable range before bumping. Bumping patched deps can regress builds (e.g. a vite bump would break docs' astro 7 requirement for vite 8). @@ -232,31 +364,51 @@ - **spawnProcess no-ops in dry-run — breaks post-check logic**: \`src/utils/system.ts\` \`spawnProcess\` returns \`undefined\` immediately in dry-run mode (unless \`enableInDryRunMode\` or worktree mode is set) — it never actually spawns the child. Any logic that relies on observable side effects (files written, state changed) after a spawn will see unchanged state in dry-run. Example trap: post-checking package.json \`version\` after \`npm version\` to detect successful-but-errored bumps would incorrectly trigger fallback paths in dry-run because the spawn is skipped and files stay stale. Mitigation: guard post-check / fallback logic with \`isDryRun()\` and short-circuit to the existing dry-run success behavior. File writes don't have this problem if routed through \`safeFs.writeFileSync\` which handles dry-run natively. + + +- **updateReleaseRevision validates SHA**: Trap: \`updateReleaseRevision()\` can trust its caller because the existing canonical link was validated, but an arbitrary replacement corrupts release authority. Fix: require exactly 40 lowercase hex characters and replace only parser-recorded SHA offsets. Regression-test strict byte-for-byte preservation outside those offsets, including CRLF, trailing newlines, and appended decoy text. + - **Vite plugin delegates deployment to wrangler**: Trap: Vite plugin delegates deployment to wrangler. Fix: Use wrangler directly for deployment. + + +- **walkFiles maxResults concurrency ordering**: Trap: using parallel \`walkFiles()\` with a caller that exits after a result limit looks faster, but concurrent workers may already have queued or begun extra traversal and yield completion-order results. Fix: callers requiring deterministic early-stop behavior use \`concurrency: 1\`; parallel mode is for broad scans where overlapping directory reads outweigh nondeterminism. Excluded directories such as \`node_modules\` are never opened, and followed symlinks still use visited-directory tracking to prevent re-entry. + + + +- **WorkspaceNameSchema literal glob magic**: Trap: allowing glob-character segments in every configured workspace key looks necessary for valid patterns such as \`packages/\[!a]\*\`, but \`hasMagic()\` treats literals like \`packages/foo]\`, \`packages/foo!\`, and \`packages/foo^\` as non-patterns. They then bypass concrete-path validation. Fix: permit glob-only characters only when the key is an actual supported Glob pattern; otherwise require \`isSafeWorkspacePath()\` in both schema validation and config resolution. Test malformed literals alongside valid character-class patterns. + + + +- **WorkspaceSchema \_\_proto\_\_ Zod record loss**: Trap: allowing \`\_\_proto\_\_\` under \`workspaces\` looks safe because it matches the ASCII key regex and passes Zod validation, but Zod record output silently drops it. \`getWorkspaceNames()\` then returns no such workspace, and \`Object.keys(workspaces)\` makes the root \`github.projectPath\` guard think no workspaces exist. Fix: explicitly reject prototype-sensitive workspace keys (\`\_\_proto\_\_\`, and defensively other inherited names) before relying on record output; test selection and root-projectPath validation with each key. + ### Pattern + + +- **action.yml workspace propagation**: Craft workspace keys are literal relative paths or safe globs expanded relative to \`.craft.yml\`; Action identity uses the exact concrete path. Chose path-based identity over symbolic names because monorepos need independent units such as \`packages/cli\`. A named-workspace selector and its additive publish-issue title component must ship together: adding either alone looks incremental, but root-level workspaces at the same version can collide and merge target checklists. Reject unsafe or overlapping matches and simultaneous Action \`path\` plus \`workspace\`; omit workspace identity when absent to preserve legacy titles. + - **AGENTS.md lore section: only include project-relevant entries**: The \`\\` in AGENTS.md is auto-maintained by the lore tool and can accumulate cross-project entries irrelevant to craft. When committing, strip entries that don't pertain to this repo. ADDITIONALLY: the lore daemon re-modifies AGENTS.md in the working tree between sessions. If a branch was committed while churn was present, it lands in an ANCESTOR commit and ships in the PR diff (not just working tree). Fix: squash the branch (\`git reset --soft master\` + re-commit only intended files, or rebase) to drop AGENTS.md from history before force-pushing. Always \`git diff master...HEAD --name-only\` to confirm AGENTS.md is absent from the PR. -- **CI poller variable gate with dedicated app token in getsentry/publish**: \*\*CI poller variable gate with dedicated app token in getsentry/publish\*\*: The \`ci-poller.yml\` cron uses repo variable \`CI_POLLER_HAS_PENDING\` as a fast gate — \`'true'\` runs, otherwise skips. \`workflow_dispatch\` bypasses the gate. \*\*Self-dispatch for fast re-checking\*\*: when pending issues remain, the poller dispatches itself (up to 60 attempts, ~30 min cap) for ~30-60s intervals instead of relying on unreliable cron. \*\*Two tokens required\*\*: (1) \`sentry-release-bot\` (\`SENTRY_RELEASE_BOT_CLIENT_ID\`/\`SENTRY_RELEASE_BOT_PRIVATE_KEY\` with \`owner: getsentry\`) for cross-repo API calls (CI status, check suites, git refs) since sentry-internal-app isn't installed on all repos. (2) \`CI_POLLER_APP\` token for writing repo variables. \`gh issue list/edit/comment\` uses sentry-internal-app token so label changes trigger \`publish.yml\`. Key: cleanup steps use \`if: always()\`; disable steps guard on \`steps.poller-token.outcome == 'success'\`. +- **CI poller variable gate with dedicated app token in getsentry/publish**: Trap: deduplicating only a default issue-list page looks sufficient for exact Publish titles, but older duplicate requests can be omitted and compact titles can collide with legacy path titles. Fix: query all relevant open requests before deduplicating. Preserve field-less legacy issue semantics; compact suffix meaning is resolved only from exact workspace membership in the CI-approved checkout, never by mutating an existing issue. - **CLI UX: auto-correct common user mistakes with stderr warnings instead of hard errors**: CLI UX: auto-correct common user mistakes with stderr warnings instead of hard errors. Safe when: (1) input is already invalid, (2) no ambiguity in correction, (3) warning goes to stderr (doesn't interfere with JSON/stdout). Normalize inputs at command level before passing to pure parsing functions. The \`gh\` CLI is the UX model. - + -- **Craft action.yml opt-in ci_ready input + signal-ready composite action**: In getsentry/craft, the \`ci_ready: 'true'\` input on \`action.yml\` makes \`craft prepare\` add a \`ci-pending\` label at issue creation (atomic with \`gh issue create --label\`), opting the repo into event-driven publishing. A separate composite action \`signal-ready/action.yml\` lets target repo CI send a \`repository_dispatch\` event of type \`ci-ready\` with \`{repo, version, sha}\` payload to the publish repo when CI passes. The publish repo's \`ci-ready.yml\` handler finds the issue by exact title match (\`publish: {repo}@{version}\`), verifies \`ci-pending\` is present, swaps labels. Auth: the dispatch sender needs a token with write access to the publish repo (sentry-release-bot app token, not \`GITHUB_TOKEN\`). +- **Cloudflare artifact layout wrangler.jsonc + dist at zip root**: When deploying docs via Cloudflare Workers from CI artifacts, package \`wrangler.jsonc\` and \`dist/\` at the archive ROOT (not nested). This matches Wrangler’s expectation for project root during \`versions upload\`. A nested layout looks reasonable (artifact subdir) but causes misresolution of config/assets at deploy time. Fix: \`cd apps/cli-docs && zip -r $OUT wrangler.jsonc dist\` so both sit at top-level. -- **Craft checkMinimalConfigVersion: strip pre-release only from CURRENT version, never from minVersion**: checkMinimalConfigVersion in src/config.ts strips \`.pre\`/\`.build\` ONLY from the effectiveCurrentVersion side, never from the configured minVersion side, so a local dev build (e.g. 2.27.0-dev.0) can satisfy \`minVersion: 2.27.0\` for dogfooding a feature before release, while a genuinely-too-new minVersion (e.g. 2.28.0) is still correctly rejected. Implemented in getsentry/craft PR #848 (feat/workspaces-schema) after the user explicitly suggested special-casing dev versions rather than guessing/hardcoding a future release version for WORKSPACES_MIN_VERSION. Final gate value: WORKSPACES_MIN_VERSION = '2.27.0' (package.json version at the time was '2.27.0-dev.0'). Pre-existing edge case unaffected: if minVersion itself carries a pre/build suffix, versionGreaterOrEqualThan's throw path can still fire. +- **Craft checkMinimalConfigVersion: strip pre-release only from CURRENT version, never from minVersion**: \`checkMinimalConfigVersion\` strips prerelease/build fields only from the current Craft version, never configured \`minVersion\`, so \`2.29.0-dev.0\` can dogfood features gated at stable \`2.29.0\` without relaxing a user-required prerelease. \`WORKSPACES_MIN_VERSION\` is \`2.29.0\`: top-level workspaces did not ship in 2.27.x or 2.28.0. Ignore SemVer build metadata for precedence comparisons, because \`+build\` must not affect ordering; preserve configured prerelease semantics. @@ -286,10 +438,18 @@ - **Craft PR workflow: branch/cherry-pick/notes; amend pre-push, new commit once pushed**: Branch naming: \`{user}/{type}/{description}\`. From a feature-branch worktree, create fix branches off master: \`git checkout -b fix/name --track origin/master\`, then \`git cherry-pick \\`. Commit titles follow conventional commits: \`fix:\`, \`feat:\`, \`refactor:\`, \`meta:\`, \`docs:\`. When adversarial-review fixes surface as uncommitted working-tree changes on top of a NOT-YET-PUSHED commit, amend them in (\`git commit --amend\`) to keep history coherent. Once a branch is already pushed and under active review (e.g. Bugbot/reviewers have commented on specific lines), push follow-up fixes as a NEW commit instead of amending — amending would orphan/hide the reviewed diff and break comment threading (done in PR #848, commit 42d17f0 on top of already-pushed 4e2d8f4). Attach plans as git notes (\`git notes add -m ...\`, push via \`git push origin refs/notes/commits\`). Create draft PRs with \`gh pr create --draft\`. + + +- **Craft publish recovery**: Steps: 1. Check npm, GCS, and the release-registry repository — partial failures may follow successful targets. 2. Mark every completed target in the publish issue — retries skip checked targets. 3. Re-add \`accepted\` — this retriggers the Publish workflow. Gotchas: - Retrying before verifying npm can cause duplicate-publish 403. - Docker Hub 400 during blob upload is a known transient flake; retry after preserving completed state. Verify: - \[ ] Every completed artifact exists remotely. - \[ ] The issue checklist matches remote state. + - **Craft publish_repo 'self' sentinel resolves to GITHUB_REPOSITORY at runtime**: The composite action's \`publish_repo\` input supports a special sentinel value \`"self"\` which resolves to \`$GITHUB_REPOSITORY\` at runtime in the bash script of the 'Request publish' step. This allows repos to create publish request issues in themselves rather than in a separate \`{owner}/publish\` repo. The resolution happens in bash (not in the GitHub Actions expression) because the expression layer sets \`PUBLISH_REPO\` via \`inputs.publish_repo || format('{0}/publish', github.repository_owner)\` — the string \`"self"\` passes through as-is and gets resolved to the actual repo name in the shell. Useful for personal/small repos where the default GITHUB_TOKEN already has write access to the repo itself. + + +- **Craft runtime release follow-up**: Steps: 1. Confirm \`package.json\` contains a development prerelease version — prevents dispatching from an already released state. 2. Identify the latest published tag — establishes the release baseline. 3. Dispatch \`.github/workflows/release.yml\` with the \`version\` input, normally \`auto\` — publishes the current master state. Gotchas: - Delaying after a significant runtime or Docker-image change leaves users on obsolete infrastructure despite master being fixed. Verify: - \[ ] Release workflow succeeds. - \[ ] The updated image is published and available to users. + - **Craft uses home-grown SemVer utils — don't add semver package for version comparisons**: Despite \`semver\` being a dependency (used in \`src/utils/autoVersion.ts\` for \`semver.inc()\`), the codebase has its own \`SemVer\` interface and utilities in \`src/utils/version.ts\`: \`parseVersion()\`, \`versionGreaterOrEqualThan()\`, \`isPreviewRelease()\`, etc. These are used throughout the codebase (npm target, publish tag logic, etc.). When adding version comparison logic, use these existing utilities rather than introducing new custom comparison functions or reaching for the \`semver\` package. Example: the OIDC minimum npm version check was initially implemented with 3 separate constants and a custom comparison helper, then refactored to a single \`SemVer\` constant + \`versionGreaterOrEqualThan()\`. @@ -302,9 +462,9 @@ - **Craft: split unrelated work into separate PRs off master (except when depending on unmerged work)**: When a feature or fix is logically independent from existing work on the current branch, don't append it to that branch. Create a new branch off master (\`git fetch origin master && git checkout -b \ origin/master\`), stash changes, pop on the new branch, commit, push with \`-u\`, open PR. This keeps PRs self-contained, independently reviewable, and revertable. Applies even when the current branch name thematically overlaps (e.g., a branch named \`fix/dependabot-security-alerts\` is not the right home for a feature adding \`security:\` prefix recognition). Exception: if the new work genuinely DEPENDS on code only present in an unmerged sibling PR (e.g. PR C's publish-state threading needs PR B's workspace resolver), branch off that sibling's branch instead of master, and plan to rebase onto master once the sibling merges. Hold the dependent PR's review/opening until the sibling settles, to avoid reviewing against a moving base. - + -- **getsentry/craft publish recovery: check completed targets before retry to avoid duplicate-publish 403**: When a craft publish run fails mid-way (e.g., docker\[release] 400 Bad Request), some targets (npm, gcs, registry) may have already succeeded. Recovery steps: (1) verify which targets completed (check npm registry, GCS bucket, release-registry repo); (2) update the publish issue body to check off completed targets; (3) re-add the 'accepted' label to re-trigger the publish workflow — it will skip already-checked targets. Always verify npm publish before retry to avoid duplicate-publish 403. Docker Hub 400 during blob upload is a known transient infrastructure flake — retry resolves it. +- **getsentry/craft open Dependabot alerts: smol-toml, cookie, brace-expansion, svgo**: Snapshot after PR #866 merge (Aug 2026): 7 Dependabot alerts still open on getsentry/craft default branch: js-yaml ×3 (alerts #215/#216/#219, GHSA-5p4m-2wfm-xmqj — same advisory, multiple vulnerable copies), smol-toml (#218, GHSA-v3rj-xjv7-4jmq), cookie (#217, GHSA-pxg6-pf52-xh8x), brace-expansion (#214, GHSA-rgw5-rvv9-x895), svgo (#197, GHSA-2p49-hgcm-8545). All are transitive. Before fixing each: trace the full chain in BOTH root and docs/ lockfiles (root pnpm.overrides don't cascade \[\[019fa896-e1de-7551-bdb9-bce5ad328264]]), follow the deep-chain verification rule \[\[019fdd20-369f-740b-95af-7487ca5d076a]], and check whether the package is a direct-pin devDep (like tar \[\[019ef40b-f60a-7a76-81c9-cc1efbc24430]]) before reaching for overrides. @@ -314,9 +474,9 @@ - **getsentry/craft: form-data has two transitive copies requiring separate overrides**: Two copies of \`form-data\` in root \`pnpm-lock.yaml\` via different chains: (1) \`form-data@4.0.4\` (High, GHSA-hmw2-7cc7-3qxx, Alert #178): \`@types/node-fetch@2.6.13\` → \`form-data@4.0.4\`. Fix: \`"form-data@>=4": "^4.0.6"\` in pnpm.overrides. (2) \`form-data@2.5.5\` (High, Alert #179): \`@google-cloud/storage@7.18.0\` → \`retry-request@7.0.2\` → \`@types/request@2.48.13\` → \`form-data@2.5.5\`. Fix: \`"form-data@<3": "^2.5.6"\` in pnpm.overrides. Both overrides needed; one alone leaves the other copy vulnerable. form-data@4.0.6 and @2.5.6 confirmed on npm. Both overrides applied in branch \`byk/fix/dependabot-security-alerts\`. - + -- **getsentry/publish: label-based state machine for publish gating**: getsentry/publish label state machine (post-PR #7881 + retry-race fix): \`accepted\` (human/auto-approve, added first), \`ci-pending\` (added by \`waiting-for-ci\` in publish.yml AFTER \`accepted\`), \`ci-ready\` (added by poller when CI passes), \`ci-failed\` (added by poller on failure, also removes \`accepted\`). \*\*Publish gate fires ONLY on \`ci-ready\` label event\*\* — not \`accepted\` — to prevent race where \`publish\` and \`waiting-for-ci\` run in parallel on the same \`labeled: accepted\` event when \`ci-ready\` is stale from a previous failed publish. \`waiting-for-ci\` job (on \`accepted\` labeled, when \`ci-pending\` or \`ci-failed\` present): removes \`ci-failed\` AND \`ci-ready\` (critical: stale \`ci-ready\` must be cleared so poller's re-add generates a fresh \`labeled\` event), adds \`ci-pending\`, comments, triggers poller. Concurrency group uses \`github.event.issue.title\` to lock on repo@version. +- **getsentry/publish Yarn Classic tooling**: Use Volta-pinned Yarn Classic 1.22.22 for getsentry/publish: \`yarn install\`, \`yarn test\`, \`yarn lint\`, and \`yarn prettier\`; never use pnpm or npm. Chose Yarn over the repository-wide pnpm habit because Publish has \`yarn.lock\` and CI installs/tests with Yarn; another package manager creates unrelated resolution churn. In particular, do not commit \`pnpm-lock.yaml\`. Source is CommonJS; tests are Vitest ES modules under matching \`\_\_tests\_\_/\` paths. @@ -328,258 +488,282 @@ ### Preference - + -- **Always add dry-run awareness to createDryRunIsolation()**: Always add dry-run awareness to createDryRunIsolation() — enables worktree mode for local operations. - - - -- **Always add dry-run logging with consistent formatting via logDryRun()**: Always add dry-run logging with consistent formatting via logDryRun(). +- **Always add paired fail-closed regression tests**: When changing behavior to bypass a dependency or discovery step for a narrow special case, add regression coverage for both sides: prove the special case succeeds without the dependency and prove ordinary/non-special-case inputs still invoke it and propagate failures. Do not rely only on helper-level invalid-output tests; include a public workflow-level test using the relevant non-root input and a throwing dependency. Treat missing paired coverage as a blocking issue before merge, then rerun focused tests, lint, and a final diff audit. -- **Always analyze and understand configuration and testing patterns before making changes**: The user consistently demonstrates a behavior of seeking to understand the configuration schema, testing patterns, and documentation conventions before making changes or implementing new features. This is evident from their requests to analyze and understand various configuration files such as \`.craft.yml\`, \`package.json\`, and \`pnpm-lock.yaml\`, as well as their interest in testing patterns and documentation conventions. The user also appears to prioritize understanding the existing setup and potential risks, such as bootstrap deadlock risks, before proceeding with changes. - - - -- **Always be blocked in dry-run**: User stated ALWAYS be blocked in dry-run. +- **Always analyze and understand configuration and testing patterns before making changes**: Before modifying utility, configuration, workspace behavior, or responding to related review feedback, inspect the current implementation, schema, call sites, loading and validation paths, related tests, documentation, compatibility conventions, and relevant recent commits or review context. For configuration, verify file-based and string/remote loading, validation, inheritance/override semantics, security constraints, error messages, and backward compatibility, including slash-containing workspace paths. Distinguish actual legacy behavior from deliberately supported current contracts when intent is ambiguous. Preserve established error messages and semantics unless the requested change requires otherwise, then add focused regression coverage for required distinctions and edge cases. - **Always be consistent**: User stated always be consistent. - + -- **Always call plan_exit to indicate done planning**: User stated always call plan_exit to indicate to the user that you are done planning. +- **Always cut a new release immediately after merging a fix or upgrade PR**: After merging a PR into master (whether a bug fix or dependency upgrade), the user consistently decides to cut a new release right away. This applies to both hotfix scenarios (e.g., Node version pin to fix a regression) and feature/upgrade scenarios (e.g., Node 24 upgrade). The user does not wait for additional changes to batch into a release — each significant merge is followed immediately by a release cut. When assisting, proactively prepare for or suggest the release step as the natural next action after a successful merge. This was confirmed after merging PR #843. - + -- **Always clean up merged branches and scratch directories after completing a task**: After merging PRs and completing a work cycle, the user consistently performs cleanup: deletes local branches (using \`-D\` for squash merges), confirms remote branches are gone, and removes temporary scratch directories (e.g., \`temp\_\*/\` dirs). The user approves deletion of large scratch dirs when prompted but intentionally retains certain directories (e.g., \`.opencode/\`). The assistant should proactively identify stale local branches and temp directories at end-of-task, present a summary of what would be deleted with sizes, and wait for explicit user approval before deleting anything. +- **Always enforce strict fail-closed publish-issue parsing**: For Publish issue metadata and revision updates, accept only one canonical, tightly structured issue-body format. Anchor authority-bearing content at the start; require the expected Quick links structure with exactly one View check runs link; validate the requested repository and a 40-character lowercase SHA exactly; and reject missing, duplicate, malformed, forged, ambiguous, or repository-mismatched fields with explicit errors. Reuse validated extraction before updates and modify only the matched check-runs section. Add regressions for discovered bypasses and keep unrelated changes out of the branch. - + -- **Always create a branch before making changes**: Always create a branch before making changes. +- **Always enforce strict publish-controller contracts and review hygiene**: For publish-controller changes, require implementation and tests to honor documented canonical formats and security boundaries exactly. Validate that values consumed by workflows come only from the authoritative canonical header, reject malformed inputs rather than silently continuing, preserve unrelated issue-body bytes during narrow updates, and cover failure paths with regressions. Check workflow details such as required action versions, cleanup of temporary files, and shell error propagation. Treat documentation/code parity, generated-artifact freshness, and absence of unrelated diff churn as merge requirements; do not accept passing happy-path tests as sufficient when contract violations or missing failure-path coverage remain. - + -- **Always cut a new release immediately after merging a fix or upgrade PR**: After merging a PR into master (whether a bug fix or dependency upgrade), the user consistently decides to cut a new release right away. This applies to both hotfix scenarios (e.g., Node version pin to fix a regression) and feature/upgrade scenarios (e.g., Node 24 upgrade). The user does not wait for additional changes to batch into a release — each significant merge is followed immediately by a release cut. When assisting, proactively prepare for or suggest the release step as the natural next action after a successful merge. This was confirmed after merging PR #843. +- **Always follow documented plans and processes**: Follow approved plans and the repository’s established branch, PR, review, and merge processes. Verify requirements before proceeding, including relevant CI checks, documentation updates, reviews, approvals, and final validation. Explicitly note and justify any deviation. Confirm every requirement is met before marking work complete or merging. - + -- **Always demand adversarial, thorough code review for deploy-target changes**: When implementing or following up on a release-automation deploy target (e.g., craft's cloudflare target), the user consistently requires deep, skeptical review before/after code changes: (1) research actual underlying tool/API behavior (wrangler CLI source, Cloudflare docs) rather than assuming; (2) explicitly enumerate concrete scrutiny areas—dry-run safety (must NEVER make network calls or side effects in dry-run/worktree mode), secret/token leakage into logs or argv, config validation edge cases (env-var expansion guards, optional vs required secrets), and test coverage of each behavior; (3) request findings as prioritized CRITICAL/MAJOR/MINOR lists with file:line references and concrete fixes; (4) forbid the assistant from modifying files during review or rubber-stamping—must find real bugs; (5) verify claims via actual commands (tsc, test runs, git diff) rather than trusting code reading alone. Apply this rigor whenever reviewing or extending sensitive deploy/release automation code. +- **Always gather dependency-chain context, verify target versions, and choose the minimal safe fix**: Before dependency or vulnerability fixes, inspect repository history, manifests, and every independent lockfile; trace each reachable vulnerable copy through exact resolutions, parents, ranges, and consumers. Verify patched versions and engine requirements, and explicitly report \`Confirmed \@\ exists on npm\`. Check for downgrades and peer conflicts. Prefer the smallest compatible patched upgrade—usually a same-major transitive \`^\` or \`~\` override—over new direct dependencies, workarounds, rollback pins, broad \`>=\` ranges, or major upgrades. Regenerate lockfiles with pnpm rather than editing them manually, and verify all vulnerable nodes are removed. - + -- **Always enable worktree mode using enableWorktreeMode()**: Always enable worktree mode using enableWorktreeMode() and disable using disableWorktreeMode(). +- **Always investigate the full code path before making changes**: Before implementing repository changes, comprehensively trace the relevant configuration fields, schemas, commands, utilities, targets, tests, documentation, and external issue or Git behavior. Search all references and inspect surrounding code to understand defaults, fallbacks, validation, logging, dry-run behavior, and edge cases. Report findings with precise file paths, line numbers, and pertinent code snippets or exact content. Use existing tests and contracts to guide the implementation rather than changing behavior based on a single call site. - + -- **Always fix transitive dependency vulnerabilities using pnpm.overrides before adding direct dependencies**: When addressing Dependabot security alerts for transitive dependencies, the user first attempts to resolve them via pnpm.overrides in package.json. Only when overrides prove insufficient (e.g., pnpm doesn't apply overrides to auto-installed peer deps) does the user escalate to adding a direct devDependency. The user also handles multiple vulnerable version ranges of the same package with separate override keys (e.g., 'form-data@>=4' and 'form-data@<3'). Always analyze the full dependency chain in the lockfile before proposing fixes, and prefer the least-invasive solution (override) over adding direct deps. +- **Always load repository setup before situation-specific skills**: At the start of coding sessions, load the \`repo-setup\` skill before loading or applying any situation-specific skills. Use repository setup first to establish project instructions, workflow, paths, and context, then proceed to task-specific exploration, planning, implementation, review, or shipping. - + -- **Always follow documented plans and processes**: The user consistently expects adherence to established plans and processes. When working on tasks, they reference approved plans (.opencode/plans/), follow standard procedures for PR reviews and merges, and expect verification of each step. The user wants confirmation that all requirements are met before proceeding, including CI checks, documentation updates, and proper approvals. Any deviations from the plan should be explicitly noted and justified. This was reinforced during the PR #843 merge process, where the user authorized admin merge only after confirming all adversarial review comments were addressed and bots ran clean. +- **Always perform read-only, evidence-based reviews before making changes**: When reviewing security, compatibility, or proposed repository changes, first inspect the current worktree, relevant branches, manifests, lockfiles, workflows, and tests without modifying files or system state. Treat the review adversarially rather than accepting existing claims, trace each issue through its actual dependency or execution path, and account for baseline divergence. Report precise file-and-line evidence, prioritize blockers and compatibility risks, identify required regression tests and validation commands, and provide a concrete, minimal remediation plan. Preserve pre-existing changes and clearly state the final verdict or planning status. - + -- **Always follow structured PR workflow with verification and cleanup**: The user consistently follows a structured PR workflow: 1) Requires all CI checks to pass (including security scans and docs preview), 2) Verifies bot findings are addressed, 3) Ensures adversarial review comments are resolved with tests, 4) Uses admin merge when appropriate, 5) Confirms successful merge with commit verification, 6) Performs thorough cleanup (deleting remote/local branches, removing scratch files while preserving intentional untracked files like .opencode/). The user expects detailed verification of each step before proceeding. +- **Always push completed work when requested**: When the user explicitly says the agent needs a push, treat pushing the relevant completed changes to the remote as a required final delivery step. Ensure the intended commits and branch are ready, preserve any stated exclusions or hygiene constraints, and report substantive push confirmation rather than an empty response. - + -- **Always gather deep dependency chain context before proposing fixes**: When investigating vulnerabilities or dependency issues, Burak Yigit Kaya systematically traces the full transitive dependency chain — identifying lockfile line numbers for each package, which packages pull them in, and which version ranges are affected — before proposing any fix. He reads raw lockfile lines, cross-references multiple lockfiles (e.g., root vs. docs/), and documents every intermediate dependency. When proposing fixes, he expects solutions that account for the complete chain (e.g., pnpm overrides targeting the correct ancestor). Do not suggest fixes without first confirming the full dependency path and all affected copies of a package. Applied in getsentry/craft Dependabot analysis: traced form-data through @google-cloud/storage → retry-request → @types/request → form-data@2.5.5. +- **Always remove unrelated diff churn before merging**: Keep the working tree, staging area, commits, and final diff strictly limited to the approved request. Treat unrelated formatter-, whitespace-, newline-, quote-, lockfile-, generated-, or baseline-only changes as merge blockers: inspect their provenance, preserve pre-existing unrelated changes exactly, exclude them from staging, and revert incidental churn. Use non-destructive checks when broad commands could rewrite files. Before finalizing, inspect the exact modified-file list and diff; retain only intentional implementation, documentation, and focused test changes, with no unintended artifacts. - + -- **Always implement isInWorktreeMode() to check current mode**: Always implement isInWorktreeMode() to check current mode. +- **Always revert back to using the GitHub**: User stated always revert back to using the GitHub. - + -- **Always investigate and remediate Dependabot/security alerts in the getsentry/craft repository before implementing fixes**: The user consistently directs dependency and security work in the getsentry/craft repo, including analyzing open Dependabot alerts and fetching GitHub security advisories/alerts to plan remediation. When the user mentions security reports, advisories, or Dependabot alerts without specifying a repo, assume getsentry/craft. Gather the current alert data (e.g. via gh api for security-advisories and dependabot/alerts), map vulnerable packages to required version bumps, and present a remediation plan prior to editing files or running upgrades. +- **Always sets repo_url from githubRepo config**: Always sets repo_url from githubRepo config: User stated always sets repo_url from githubRepo config. - + -- **Always plan and document implementation details before coding**: The user consistently creates detailed plans and documentation before implementing features. They analyze existing code, identify gaps, and document their approach in markdown files. The user expects assistants to follow these plans precisely, including specific file paths, line numbers, and implementation details. They prefer comprehensive planning to ensure correct implementation, especially for complex features like prefixed tags and Cloudflare deployments. +- **Always specify a \[script_name]\(https://developers**: User asserted always specify a \[script_name]\(https://developers. - + -- **Always prefer upgrading to a fixed version over applying workarounds**: When a bug or vulnerability is caused by an upstream dependency or runtime (e.g., a Node.js version, a vulnerable package), the user consistently prefers upgrading to the patched/fixed version rather than applying workarounds, monkey-patches, or configuration hacks. Applies to Node.js runtime pins (upgrade to the version containing the fix), npm/pnpm dependencies (bump to the fixed version or add a direct dependency), and transitive vulnerabilities (use overrides to force the fixed version). When proposing solutions, always lead with the clean upgrade path and only mention workarounds as a fallback if no fixed version exists yet. Confirmed again: Node 22.23.0 regression → upgrade to 22.23.1 (not pin back to 22.22.x). +- **Always update tests for workspace behavior changes**: When changing workspace parsing, resolution, configuration, discovery, CLI output, or publish workflows, inspect the existing resolution flow and add focused regression tests with the implementation. Cover exact values and errors for absent configuration, invalid external output, legacy-compatible names, empty lists, listing and active-workspace selection, valid/unknown/invalid/ambiguous/overlapping glob inputs, version gates, and concrete paths resolved relative to the config file. Test traversal/prototype-key rejection where relevant. Mock dependencies explicitly, restore mocks after each test, preserve unrelated working-tree changes, place tests in corresponding existing test files, and run focused Craft tests after each change. - + -- **Always prep a draft PR immediately and set a reminder to merge once blocking dependencies are resolved**: When a desired change is blocked by an external dependency (e.g., a Docker image not yet published, a upstream PR not yet merged), the user does not wait — they immediately prepare the branch and PR with all code changes ready, then explicitly note the blocker and plan to merge once it clears. Follow this pattern: make all code changes now, commit and push the branch, open the PR (draft if needed), and clearly state what external condition must be met before merging. Do not delay the code prep work waiting for the blocker to resolve. +- **Always use pnpm for package management**: In Craft repository work, use pnpm for all package-management operations and never use npm or yarn. Follow the repository’s prescribed installation command, \`pnpm install --frozen-lockfile\`, and respect the Volta-managed Node.js version (currently v22.12.0) when installing dependencies or running checks. - + -- **Always provide comprehensive context for code reviews and testing**: The user consistently provides extensive context when requesting code reviews or discussing testing. This includes: detailed file contents, specific line numbers, test results, implementation logic, design intent, and areas requiring special scrutiny. The user expects reviewers to have all necessary information to understand changes, identify edge cases, and verify correctness without needing to ask follow-up questions about context. +- **Always use the latest compatibility date for Cloudflare Pages Functions**: Always use the latest available Cloudflare compatibility date for Pages Functions and Workers deployments. Do not pin an older date for stability: compatibility dates define the runtime behavior, so an old date silently preserves outdated semantics and security fixes. Verify the current date against Cloudflare's primary documentation before changing configuration or docs. - + -- **Always request adversarial (non-rubber-stamp) review of dependency/security PRs with structured verification questions**: When a PR addresses Dependabot/security alerts or pnpm overrides, the user requires a rigorous adversarial code review (explicitly NOT a rubber-stamp) before merge. They provide (or expect) a fixed set of yes/no verification questions: (1) confirm the vulnerable package resolves to the patched version in ALL workspace lockfiles (root + docs); (2) confirm no regression/downgrade of other previously-fixed packages; (3) confirm no peer/version conflicts introduced; (4) confirm the commit touches ONLY intended files and excludes lore/agent files (AGENTS.md, .lore.md, .opencode); (5) confirm override syntax/version range is valid (use ^/~ not >=). Apply this review process to every security/dependency PR before merging. +- **Always verify Cloudflare/Wrangler/third-party behavior against primary sources before answering or changing**: Always verify behavior against primary sources for Cloudflare/Wrangler or any third-party tool/API before answering technical/design questions or making changes. Check the exact installed tool version, its --help output and bundled source, plus official vendor documentation; never guess. Verify defaults, precedence, endpoint behavior, authentication and least-privilege token scopes, account/environment and production semantics, compatibility dates, and network/deployment side effects. For Wrangler investigations, use an isolated scratch dir like /tmp/opencode, install the exact version, inspect its bundled CLI source, and cross-check official Cloudflare docs. Pass API tokens only via environment variables, never in args/logs. Ensure dry-run paths have no network or deployment side effects. Cite exact source locations/URLs and document required permissions; report prioritized CRITICAL/MAJOR/MINOR findings with file:line references and command-based verification. - + -- **Always request adversarial no-rubber-stamp code reviews with prioritized file:line findings and explicit verification**: When asking for a code review (especially PRs or security fixes), the user expects a NO-RUBBER-STAMP adversarial senior review. The reviewer must NOT modify any files (read-only). Findings must be prioritized CRITICAL/MAJOR/MINOR with concrete file:line citations. The user provides specific verification questions that must be answered with direct yes/no responses. Always verify the committed HEAD matches intended working-tree changes and contains only intended files (exclude lore/agent churn like AGENTS.md/.lore.md/.opencode). Flag any unverifiable claim as unverified. This is part of the user's 'regular rigor' process: tsc, tests, lint, prettier, then adversarial review before merge. +- **Always write tests alongside implementation**: Behavioral pattern detected across 9 sessions (action: requested-tests). The user consistently demonstrates this behavior. - + -- **Always request comprehensive code reviews with specific scrutiny areas**: The user consistently requests detailed code reviews for complex changes, providing extensive context about the implementation. They specify exact files and functions to examine, define particular areas that need scrutiny (like edge cases, regressions, and inconsistencies), and request prioritized findings with concrete fixes. The user also provides verification steps to ensure the review covers all critical aspects of the change. +- **Avoids guesses**: user does not want guesses. - + -- **Always request tests after implementing new features**: The user consistently requests or adds tests after implementing new changes across multiple sessions. This includes instances where the user asks for tests after implementing Cloudflare target, dry-run features, and other functionality. The user also plans and adds tests for new features like dry-run mode deployments. This behavior was confirmed during the PR #843 merge, where accompanying tests for adversarial review fixes were verified as present. +- **Check release workflow changes end to end**: Before modifying Craft release or publish behavior, inspect the live repository and search all related references across \`action.yml\`, reusable workflows, scripts, and tests. Trace inputs and outputs—especially version, workspace, revision, changelog, and issue metadata—through every caller and consumer, then update all affected propagation points consistently. Preserve established safeguards such as file-based changelogs, exact-title issue deduplication, checked-target retention, and current-only prerelease compatibility. Do not rely on stale assumptions or patch context; use exact current lines and type definitions. Add or update focused tests for the affected contract and run broader verification afterward. - + -- **Always requests detailed analysis and planning before implementation**: The user consistently requests detailed analysis and verification of code, configurations, and plans. They ask for confirmation on intent, analysis of existing test files, and review of relevant source files. The user also tends to provide file contents and grep results to support their requests. They expect thorough investigation and planning, including checking for specific details such as architecture, testing patterns, and documentation conventions. This was demonstrated during the PR #843 process, where empirical testing was used to verify false alarms. +- **CI poller resolver edit gate**: Never edit a Publish issue unless \`resolve-ci-poller-input.js\` succeeds and emits a JSON object with a nonempty string \`.issueBody\`. Missing, scalar, malformed, non-string, or empty responses can appear usable after \`jq\` extraction yet make \`gh issue edit --body-file\` erase the canonical body. Require resolver success plus \`jq -e\` object/type/nonempty validation and a nonempty output file; otherwise warn, skip the issue, and clean every temporary file. Regression-test resolver failure, malformed JSON, scalar/non-string, missing, and empty bodies. - + -- **Always requests thorough code reviews and scrutiny of new targets**: The user consistently requests thorough code reviews of new targets added to the getsentry/craft repo. This includes scrutinizing dry-run correctness, secret leakage, config validation edge cases, and test quality. The user also tends to analyze existing test files, review relevant source files (such as index, base, schema, and documentation), and examine directory contents and test files in the repo. This behavior was reinforced during the PR #843 review process, where adversarial review findings were addressed and verified. +- **Clarify ambiguous requirements before finalizing implementation decisions**: When a technical term or proposed change is ambiguous, ask for or provide clarification tied to the actual workflow before treating an earlier approval or rejection as final. The user may reverse an initial decision after learning the behavior’s real purpose. Preserve useful behavior when it supports internal handoffs and validation, but distinguish it from unrelated concerns such as issue-title syntax. Confirm the intended semantics and add focused regression tests once the clarified decision is made. - + -- **Always revert back to using the GitHub**: User stated always revert back to using the GitHub. +- **Continue high-priority Publish work from the existing state**: When resuming a multi-session task, continue the existing Publish worktree and its established high-priority plan rather than restarting or broadening scope. First inspect the current implementation and contracts, then complete parser/documentation/workflow changes, add regressions, run focused and cross-repository verification, and finish with a strict read-only final review. Treat independent adversarial review as a merge gate: do not modify reviewed files while it is pending, resolve every finding after it returns, and rerun verification before committing. Preserve known unrelated dirty changes and honor explicit no-edit review requests. - + -- **Always run full test suite and lint/format checks before considering work complete**: After implementing or fixing code, the user expects the assistant to run the full verification pipeline: \`pnpm test\` (full suite, not just affected files), \`tsc --noEmit\`, \`pnpm lint\`, and \`pnpm exec prettier --check\` on changed files. The assistant should report pass/fail counts precisely (e.g., '58 test files passed, 1067 tests passed, 1 skipped'), investigate any warnings or failures to determine if they are pre-existing/flaky vs. caused by the current change, and only proceed (e.g., to merge or mark work done) once the full suite is confirmed green. When failures appear in CI (Node version matrix, lint fixes, etc.), diagnose root cause explicitly (e.g., flaky test vs. real formatting issue) before dismissing them as unrelated. Use pnpm exclusively for package/test operations, never npm or yarn. +- **Enforce Craft dry-run isolation through centralized wrappers**: Use Craft’s centralized dry-run infrastructure: \`getGitClient()\`/\`createGitClient()\`, \`getGitHubClient()\`, \`safeFs\`, and \`safeExec\`. Make \`createDryRunIsolation()\` dry-run-aware and run permitted local Git operations in a temporary worktree, using \`enableWorktreeMode()\`, \`disableWorktreeMode()\`, and \`isInWorktreeMode()\` consistently. Block every remote mutation in the wrappers, leave the original checkout unchanged, and clean up isolation afterward. Format simulations through \`logDryRun()\`; use explicit guards only for native dry-run flags, mock data, or post-checks skipped with their side effects. Add regressions proving remote operations are blocked, local simulation is isolated, cleanup succeeds, and the original repository remains untouched. - + -- **Always sets repo_url from githubRepo config**: Always sets repo_url from githubRepo config: User stated always sets repo_url from githubRepo config. +- **Enforce explicit edge-case invariants**: Enforce narrow configuration and CLI invariants with targeted tests. Prerelease compatibility relaxation applies only to the running/current Craft version: current \`X.Y.Z-dev.0\` may satisfy \`minVersion\` through \`X.Y.Z\`, but never normalize or weaken the configured minimum. CLI parsing must never consume a following flag as an option value. Treat bare, empty, flag-followed, or otherwise invalid workspace options as invalid and preserve environment fallback where applicable; distinguish valid inline values. Test malformed and repeated options, \`--\` terminators, fallback behavior, and valid/invalid configuration paths. - + -- **Always specify a \[script_name]\(https://developers**: User asserted always specify a \[script_name]\(https://developers. +- **Enforce preview deployment lifecycle, fork safety, and cleanup invariants**: PR preview/deployment workflows must: trigger cleanup when a PR closes so preview storage never grows unbounded; skip deployment and cleanup for fork PRs because they lack write permissions; delete previews only based on authoritative closure or verified inactivity, never age/count heuristics that can remove live previews; and use metadata-only/API/blobless cleanup rather than downloading large artifacts. Handle missing events and cleanup timing, validate these safeguards in every workflow change, and prefer designs that eliminate the failure class, such as auto-expiring previews. - + -- **Always squash-merge getsentry/craft PRs via GitHub API only when CI is green and mergeable**: When merging PRs on getsentry/craft, the user directs using \`gh api PUT /repos/getsentry/craft/pulls/\/merge\` with \`merge_method=squash\` (admin squash-merge via API) instead of \`gh pr merge\` CLI, because the CLI fails when master is checked out in another worktree. The user imposes a strict conditional: merge ONLY if ALL CI checks are green AND mergeable status is clean; if any check is red, report which failed and stop (never merge). After a successful merge, re-query open Dependabot alerts to confirm they auto-close. Follow this exact flow for getsentry/craft PR merges. +- **getsentry/publish CommonJS Vitest conventions**: Use getsentry/publish conventions: kebab-case source filenames, CommonJS \`require\`/\`module.exports\`, camelCase variables/functions, UPPER_SNAKE_CASE constants and regexes, and imports ordered Node built-ins, external packages, then local modules. Tests use Vitest globals and ES modules; mock external dependencies before importing the module under test because import-time dependency capture otherwise bypasses mocks. Use descriptive errors and early validation guards; throw on unexpected states rather than silently defaulting. - + -- **Always switch to a specific branch before exploring the repository**: The user consistently switches to a specific branch before exploring the repository structure and files. This is observed in the current session where the user stated they will switch to branch "${branch}" at 10:02. This behavior is also implied in prior sessions where the user is working with specific branches and targets, such as 'test/strengthen-idf-cascade-tests' on Jun 10, 2026. The assistant should expect the user to specify a branch before proceeding with repository exploration. +- **Ground Git and release changes in authoritative repository-wide evidence**: Before changing Git or release behavior, verify assumptions against official Git documentation, installed dependency APIs, repository source, configuration flow, docs, changelogs, and existing tests. Preserve exact documented semantics and user-specified wording rather than relying on simplified assumptions. Trace all relevant callers and configuration paths, especially for remotes, refspecs, release branches, tag prefixes, workspaces, dry runs, and monorepo isolation. Account for edge cases such as push URLs, mirrored remotes, remote-only branches, slashed prefixes, and per-product version history. Add focused regression coverage and documentation updates when behavior changes. - + -- **Always targets the production environment for Cloudflare deployments**: Always targets the production environment for Cloudflare deployments. +- **Inspect canonical parsing and validation paths before changes**: When modifying publish-issue behavior, first examine the canonical PEG grammar, its generated-parser workflow, and every downstream consumer that validates or resolves parsed fields. Track exact title syntax, repository/path/version validation, CI checkout timing, and relevant tests so changes preserve the established contract. Pay particular attention to migration-related behavior: confirm obsolete legacy syntax has been removed, and ensure path/workspace resolution follows the documented full-path, CI-approved-revision flow. - + -- **Always use the latest compatibility date for Cloudflare Pages Functions**: Always use the latest available compatibility_date when configuring Cloudflare Pages Functions / Workers deploys (via wrangler or the Craft Cloudflare target). Seen as a recurring directive across 9+ prior sessions. Do not pin to an older date for stability — always bump to current. +- **Maintain strict canonical contracts with regression coverage**: When continuing Publish-related work, preserve the user’s explicitly stated authority, parsing, checkout, and workspace-resolution contracts exactly rather than broadening behavior. Treat documentation, grammar, generated artifacts, and implementation as one canonical system that must remain aligned. Before declaring work complete, conduct a read-only merge-blocking audit, add focused fail-first tests for uncovered edge cases, regenerate derived parser artifacts, and run the full verification suite. Reject ambiguous or duplicate release-authority inputs, preserve exact repository/title spelling without normalization, and ensure contextual failures are tested. - + -- **Always verify merge conditions before merging and report blockers instead of merging**: User authorizes merges (including admin squash-merges for Dependabot/security PRs) only via explicit conditional directives. Before executing, confirm all stated gates pass: CI checks green, mergeable status clean, no bot/adversarial review findings, and verification (build, tsc, tests, lint) complete. If any condition fails, report exactly which check is red and stop — never merge. Once merged, confirm specified downstream effects (e.g., Dependabot alerts auto-closing). +- **Make release-branch creation fail closed before side effects**: When preparing a release, detect an exact existing release branch at every effective push destination before creating branches, writing outputs, prompting, entering dry-run isolation, or modifying files. Resolve push URLs using actual Git push semantics, including pushurl, pushInsteadOf, multiple destinations, and mirror configuration; do not rely on fetch URLs or cached refs. Preserve any existing remote branch and instruct the user to resume/publish the pending release. Treat lookup failures as fatal. Keep the early preflight for UX, but also make the final push race-safe with an atomic expected-absent condition such as an empty force-with-lease expectation. - + -- **Always verify npm package existence before recommending version bumps**: When analyzing dependency vulnerabilities and recommending fixes, the user consistently confirms that the target patched version actually exists on npm before finalizing the recommendation. This applies to both direct dependency bumps (e.g., tar@7.5.16) and override targets (e.g., form-data@4.0.6, form-data@2.5.6). Always check npm registry availability for the exact recommended version as part of the fix analysis, and explicitly note the confirmation (e.g., 'Confirmed X@Y.Z exists on npm') alongside the fix action. +- **Make sure to add wrangler binary to the omnibus docker image**: Steps: 1. Update Dockerfile to include global npm install of wrangler in the runtime stage. Gotchas: - Ensure wrangler binary is properly verified after installation. Verify: - \[ ] wrangler binary exists in the Docker image. - \[ ] wrangler --version returns the expected version. - + -- **Always verify resolved lockfile versions to classify Dependabot alerts as genuine or stale before fixing**: When remediating Dependabot alerts in the getsentry/craft repo, the user first cross-references each alert's vulnerable range against actual resolved versions in both root and docs pnpm-lock.yaml files. Alerts with resolved versions outside the vulnerable range are flagged as stale/phantom and dismissed without changes. For genuine alerts, the user determines fix strategy: direct version bump in package.json for direct deps, or pnpm.overrides for transitive deps, then runs pnpm install. Always analyze root and docs projects separately as they maintain independent lockfiles. +- **Never be marked as latest**: User stated never be marked as latest. - + -- **Always write tests alongside implementation**: Always write tests alongside implementation. +- **Never return: it terminates the process with the**: User stated never return: it terminates the process with the. - + -- **Demand adversarial, skeptical code reviews with explicit no-modify and no-rubber-stamp instructions**: When requesting a code review (especially on the getsentry/craft cloudflare target or similar security/infra-sensitive code), the user consistently: (1) frames the review as 'adversarial senior code review', explicitly instructing the assistant NOT to modify files and NOT to rubber-stamp; (2) supplies precise commands to run first (git log, git diff against origin/master excluding .lore.md, full file reads) to establish current state; (3) provides a detailed summary of the change under review; (4) enumerates specific, numbered scrutiny points (e.g., dry-run must make zero network/spawn calls, no secret/token leakage into argv/logs/errors, control-flow tracing of try/catch boundaries, verification that error-reporting functions actually throw/propagate, test quality checks for vacuous passes, regression checks on defaults). Assistant should proactively verify each named concern with code-path tracing and cite file:line evidence, output findings prioritized (CRITICAL/MAJOR/MINOR), and avoid superficial approval — treat every review as a chance to find real bugs, not confirm correctness. +- **Never swept in by accident**: User stated never swept in by accident. - + -- **Do not commit lore-daemon AGENTS.md churn in feature PRs**: The user consistently requires that lore daemon-generated changes — modifications to AGENTS.md and untracked .lore.md/.opencode/ directories — are excluded from committed or staged file sets for fix/feature branches. When preparing, squashing, or reviewing a PR, verify the diff contains only intended changes and drop any lore churn via git restore, reset, or rebase. The user treats this as a hard rule: lore churn must not ship in fix PRs. Always confirm the final file list excludes these paths (e.g., via git diff master...HEAD --name-only) before force-pushing or merging, and include this check in any review verification checklist. +- **Never use the @typescript-eslint/no-unused-vars ESLint rule**: Never use \`@typescript-eslint/no-unused-vars\`: do not add, enable, suppress, treat as a merge blocker, or clean up solely for this rule; it is outside this project’s quality policy. Distinguish diagnostics introduced by the current change from confirmed baseline warnings using history or baseline output. The seven known warnings in \`src/commands/publish.ts\` and \`src/utils/git.ts\`, including underscore-prefixed catch variables, are pre-existing non-blocking hygiene debt. Report lint as passing only as 0 errors with the unchanged seven warnings noted when relevant. - + -- **Enforce full verification pipeline before merge and require explicit merge go-ahead**: User runs a rigorous, repeated workflow around PRs: after implementation, always run tsc typecheck, lint, prettier --check/--write, and the full test suite (plus docs build when relevant) before considering work done. Before merging, require an adversarial/code review pass (background subagent or bot review) and check CI status, mergeable state, and Bugbot/Seer findings explicitly. Do NOT merge without the user's explicit go-ahead ('run the merge process for PR #X') even if CI is green — the assistant should present status and wait for approval. When merging, use the same admin/squash-merge process consistently across PRs. After merge, clean up: delete the merged local branch, but leave other stale branches for a later 'end-of-task cleanup' step rather than deleting them immediately. Also proactively fix formatting/prettier issues before they cause CI failures, learning from prior review feedback (e.g., bot/user comments) rather than repeating past mistakes. +- **Peggy extra-options-file keys**: Peggy extra-options-file keys must use long-option names without leading dashes, with internally dashed CLI options written in camelCase. Copying CLI syntax into the config file looks natural, but its option-object keys are names such as \`grammarSource\`, not \`--grammar-source\`. - + -- **Follow the established git workflow (branch, PR, review)**: Behavioral pattern detected across 3 sessions (action: enforced-workflow). The user consistently demonstrates this behavior. +- **Prefer explicit, fail-closed publish workflow design**: For publish-controller changes, use explicit runnable code and consolidated parser/documentation sources rather than inline YAML/Bash JavaScript or duplicated parsing logic. Treat JSON boundaries deliberately: pass valid JSON strings to JSON.parse and serialize structured GitHub Action outputs for fromJSON consumers. Preserve safety over permissive fallback—when workspace discovery is required, failures must stop execution rather than fall back to a potentially misrouted checkout path. Address validated review findings, update requested dependencies, and avoid unrelated formatter churn. - + -- **Maintain minimal, stateless, dependency-light design for craft's Cloudflare target**: When evaluating changes or new tools for craft's Cloudflare deploy target, the user consistently favors a minimal, stateless design that shells out to \`wrangler\` rather than adopting heavier frameworks, SDKs, or new runtime dependencies. The user has explicitly rejected adding new dependencies (e.g., Cloudflare SDKs, IaC tools like Alchemy) in favor of using the existing wrangler CLI and Node's native \`fetch\`. The user prioritizes: (1) security scrutiny — no secret leakage into logs/argv, use of env vars over CLI args; (2) strict dry-run safety — Cloudflare deploys must NEVER run in dry-run mode since they're irreversible remote operations; (3) treating the target as a stateless publish step (consuming a prebuilt zip artifact) rather than owning infrastructure state; (4) aligning defaults with Cloudflare's own platform direction (e.g., defaulting to \`worker\` over \`pages\` since Pages is being deprecated). When proposing changes, verify these constraints are preserved and prefer the simplest solution using existing tooling over introducing new frameworks/dependencies. +- **Prefer fail-closed validation over implicit fallback**: When inputs, workspace discovery, parsing, or configuration are invalid or unavailable, preserve strict boundaries and fail explicitly rather than guessing, normalizing, or silently falling back to another behavior. Treat deployment prerequisites as intentional when they prevent ambiguous routing. Ensure parsing receives valid structured input and produces deliberate validation errors. Add focused regressions for boundary cases such as malformed JSON, unsafe paths, invalid identities/versions, and discovery failures. - + -- **Make sure to add wrangler binary to the omnibus docker image**: Steps: 1. Update Dockerfile to include global npm install of wrangler in the runtime stage. Gotchas: - Ensure wrangler binary is properly verified after installation. Verify: - \[ ] wrangler binary exists in the Docker image. - \[ ] wrangler --version returns the expected version. +- **Prefer fail-closed, race-safe implementations that avoid unintended side effects**: When implementing Git or release workflows, validate conditions before making changes, propagate unexpected query failures, and use exact matching rather than fuzzy or ambiguous checks. Treat early checks as user-facing preflight only; enforce critical invariants atomically at the final operation to prevent races, such as requiring a destination ref to remain absent during push. Preserve existing remote branches and unrelated working-tree files, and never silently overwrite, advance, or sweep them into commits. Follow documented Git semantics precisely, including effective push destinations, exact ref patterns, and meaningful exit statuses, while providing clear, actionable errors. - + -- **Never be marked as latest**: User stated never be marked as latest. +- **Prefer minimal, fail-closed fixes backed by direct inspection**: When reviewing or fixing PR feedback, first inspect the relevant source, review threads, branch state, and recent changes rather than making broad assumptions. Implement only the narrowly justified correction, preserving valid existing behavior unless evidence shows it is obsolete. For parsing, discovery, documentation, and workflow boundaries, prefer fail-closed handling: treat missing markers, malformed or blank output, and absent required paths as explicit contextual errors. Add focused regression coverage before or alongside fixes, then run verification and a final review. - + -- **Never return: it terminates the process with the**: User stated never return: it terminates the process with the. +- **Prefer minimal, stateless Wrangler-based Cloudflare deployments**: Craft’s Cloudflare target defaults to Workers while retaining explicit \`deployType: pages\`. Chose the existing Wrangler CLI and raw Node \`fetch\` over Alchemy, SDKs, or other IaC frameworks because Craft performs a stateless deployment of a prebuilt artifact; Alchemy expects state ownership, is heavier and Bun-first, and belongs in a downstream project’s build layer. Pass \`CLOUDFLARE_API_TOKEN\` only through environment variables. Dry-run must make no network or deployment calls. - + -- **Never swept in by accident**: User stated never swept in by accident. +- **Prefer Peggy as the canonical single source for Publish parsing**: Prefer Peggy as the canonical single source for Publish title parsing and format documentation. Chose one grammar with generated parser and synchronized documentation over regexes, inline parsers, or manually duplicated docs because format drift creates incompatible accepted inputs. Generate the parser reproducibly with a checked-in generator and stale-generation check; generate or synchronize \`docs/publish-issue-format.md\` from the grammar. Keep complex workflow JavaScript in runnable Node modules where practical. - + -- **Never use the @typescript-eslint/no-unused-vars ESLint rule**: The user has repeatedly and explicitly stated across multiple sessions that they never use the \`@typescript-eslint/no-unused-vars\` ESLint rule. When lint output reports warnings from this specific rule (e.g., '\_err defined but never used'), the user does not consider them real issues or merge blockers. Assistants should not treat \`@typescript-eslint/no-unused-vars\` warnings as errors to fix, should not add this rule to ESLint configs, and should not flag its violations during verification or commit readiness checks. +- **Preserve configured values and relax only derived inputs**: When compatibility or location logic needs flexibility, keep configured or externally supplied values authoritative and unchanged. Apply any normalization, prerelease relaxation, or interpretation only to the current/derived side. In particular, never relax a configured minVersion, never normalize controller-provided names, and use exact identity/path matching for repositories and workspaces. Treat missing configuration and unmatched paths according to their explicit fallback semantics rather than coercing them into a different identity. - + -- **Prefers using wrangler for Pages deployments due over complexity of manual implementation**: prefers using wrangler for Pages deployments due to complexity of manual implementation. +- **Preserve exact release-workflow contracts and repository hygiene**: Treat the user’s stated release and Git conventions as strict requirements. Never stage, commit, or modify unrelated working-tree changes; inspect the exact intended file set and repository hygiene before pushing. Preserve specified wording and behavior verbatim, including branch-switch log messages, documented process-termination semantics, and release-formatting rules such as converting CalVer \`@\`-mentions to bold text. During reviews, verify these contracts precisely, report prioritized \`file:line\` findings, and provide a clear merge verdict without making edits when a read-only review is requested. - + -- **Provide precise, file:line-cited findings when requesting research or review**: When the user (BYK) asks the assistant to review PR feedback, investigate architecture, or perform adversarial code review, they expect (and themselves model) extremely specific, evidence-based communication: exact file paths, line numbers, function names, and quoted code/comments rather than vague summaries. They frequently reference bot-reported bugs (Bugbot) and their own GitHub review comments by ID, and expect the assistant to ground responses in actual source code (reading files, running commands, checking library internals like wrangler's bundled source) rather than speculating. When asked to investigate/design (e.g., workspace support), the user explicitly forbids writing code until a thorough, file:line-cited map of all touchpoints is produced first. When asked to review, they want skeptical, adversarial scrutiny with concrete CRITICAL/MAJOR/MINOR findings, not rubber-stamping. Always cite exact locations, verify claims against real code/library behavior, and separate research/investigation from implementation unless explicitly told to proceed to build mode. +- **Publish JSON.parse valid fallbacks**: Always supply valid JSON text to \`JSON.parse()\` in Publish workflow handoffs, using \`{}\` or \`\[]\` fallbacks followed by contextual shape/required-field validation. \`process.env.VALUE || ''\` looks like a harmless missing-value default, but throws an unhelpful parser \`SyntaxError\` before deliberate validation can explain the failed contract. - + -- **Request tests after implementing new Cloudflare target**: Always request tests after implementing new features or targets. +- **release branch switch log**: Always log a successful switch to a newly created Craft release branch exactly as \`Switched to branch "${branchName}"\`. Alternative wording looks harmless, but breaks consistent CLI UX and output assertions. - + -- **Require design-first investigation and explicit sign-off before implementing cross-cutting architectural changes**: When a change touches multiple subsystems (e.g., config schema, CLI, action-layer, publish pipeline) rather than a single file, the user wants investigation and design decided before any code is written. Pattern: (1) explicitly instruct 'do not write code, only read/investigate and report findings' with file:line citations; (2) request a structured deliverable covering current data flow, every touchpoint needing changes, existing precedents/naming collisions, and schema/extension points; (3) require the assistant to surface open design questions (naming, schema shape, migration/back-compat) and pause for the user's decision rather than assuming; (4) write findings/decisions into a durable, version-controlled design doc (e.g. .opencode/plans/) before implementation; (5) split unrelated fixes into separate small PRs vs. the larger redesign, and defer/park in-flight PRs whose scope overlaps the redesign until design is agreed. Always confirm branch/working directory and produce a todo list tracking design→review→implementation phases. +- **Require adversarial, source-verified reviews for deployment changes**: For Craft deployment-target work, inspect the actual working tree and implementation rather than relying only on commit diffs or assumptions. Verify CLI/API behavior against primary documentation or exact dependency source, then test security, dry-run side effects, configuration validation, fallback paths, runtime compatibility, and documentation accuracy. Ensure secrets such as API tokens are passed only through environment variables and never exposed in argv or logs. Prefer minimal, stateless integrations without unnecessary SDKs or infrastructure ownership. Report concrete CRITICAL/MAJOR/MINOR findings with file:line references and command-based evidence; do not rubber-stamp solid-looking changes. Treat uncommitted fixes as critical because CI only sees committed state. - + -- **Require design-first, incrementally-reviewed development with explicit PR splitting and adversarial verification**: Before implementation, user wants thorough codebase research and a written design doc covering all touchpoints (schema, resolver, CLI, caching, backward-compat), with explicit open questions posed back for user decision rather than assistant assumptions. User splits large features into multiple sequential, independently-mergeable PRs (e.g., PR A/B/C/D/E) rather than one big change, parking unrelated/controversial work into separate redesign efforts. After each implementation phase, user demands an adversarial senior-code-reviewer-style review — explicitly instructing not to rubber-stamp, to find real bugs/edge-cases/backward-compat breaks, with concrete file:line citations, prioritized (CRITICAL/MAJOR/MINOR) findings, and direct answers to specific verification questions. User also insists on backward compatibility being verified explicitly (feature must be fully inert when unused) and cross-checks naming/terminology against existing code concepts to avoid collisions. When investigating, user often forbids code changes first ('do NOT write code, only read/report') to separate research from implementation. +- **Require audit-driven fixes with focused regression coverage**: When addressing repository or PR issues, perform a thorough audit rather than a narrow edit, tracing findings through related runtime code, workflows, documentation, and tests. Treat correctness and safety findings as merge blockers until fixed and covered by focused regression tests, especially validation and failure paths. Preserve exact contracts, including formatting or trailing bytes where relevant; do not weaken validation merely to satisfy fixtures or permit unsafe fallbacks. Keep code, generated artifacts, and documentation aligned, remove unrelated churn, verify affected paths and the final diff, rerun full verification, and obtain a fresh read-only audit before declaring the work mergeable. - + -- **Require doc-cited, source-verified answers for CLI/API behavior questions**: When investigating wrangler/Cloudflare CLI or API behavior, Burak Yigit Kaya requires answers grounded in official Cloudflare docs (cited URLs) or the tool's own source — never guessed. Standard procedure: install the exact wrangler version in a scratch dir under /tmp/opencode/ (never the real repo — see \[\[019f7135-8af9-78ae-833c-8decd37d78b1]]), e.g. \`cd /tmp/opencode && mkdir wr2 && cd wr2 && npm init -y && npm install wrangler@4.111.0\`, then grep/read \`node_modules/wrangler/wrangler-dist/cli.js\` for exact endpoint calls, default values, and precedence order; cross-check against Cloudflare's official docs (API token permissions reference, CI/CD guides). Applies to default branch/env resolution, auth precedence, flag semantics, token scopes, non-interactive/CI behavior. When existing code made assumptions, re-verify against source/docs and cite exact lines or doc URLs before trusting or changing behavior. +- **Require end-to-end compatibility for workspace release changes**: Treat workspace-aware release behavior as an end-to-end, cross-repository contract. Propagate selected workspaces losslessly through configuration resolution, CLI parsing, actions, input forwarding, title/issue identity, CI-label assumptions, target selection, publish-state keys, documentation, and tests; update all producers and consumers together. Validate untrusted workspace values before side effects and fail closed for empty, option-like, ambiguous, control-character, Unicode-control, separator, or otherwise unsafe input. Preserve unscoped paths byte-for-byte, prevent workspace/version collisions, use authoritative identity/state algorithms, and do not ship producer changes before acceptance-side support. Test scoped and unscoped paths, defaults, invalid inputs, and security/correctness boundaries; require adversarial evidence-based review and use pnpm for project dependency and test commands. -- **Require exhaustive file:line-cited research before design or code changes**: For architecture-level changes (e.g. multi-product/workspaces support, config schema changes, new deploy targets), Burak Yigit Kaya requires a read-only investigation phase first: read all relevant source/tests/docs/schemas across the codebase, cite exact file:line locations, track progress via a todo list, and identify existing precedents/naming collisions/extension points before proposing any design. He explicitly forbids writing or modifying code during this phase ('do NOT write code — only read/investigate'). Deliverable is a structured, numbered map of touchpoints, current behavior, and open questions — with the user (not the assistant) making explicit design-decision calls before implementation proceeds. Confirmed repeatedly: Cloudflare target design, prefixed-tags design, and the workspaces redesign (multi-product config, publish-issue action layer). Apply whenever a change spans multiple interacting subsystems. +- **Require exhaustive file:line-cited research before design or code changes**: Before architecture, cross-cutting, migration, compatibility, unfamiliar-implementation, or release-readiness decisions, perform a non-mutating, evidence-based investigation. Preserve the worktree; inspect repository state, relevant source, tests, docs, schemas, workflows, complete diffs, and upstream/PR context. Produce a file:line- and commit-cited map of current behavior, contracts, touchpoints, dependencies, risks, compatibility/security concerns, test coverage/gaps, precedents, and open questions. Verify claims against source, track work with a todo list, ask explicit design questions, record decisions durably, and obtain user sign-off before implementation when required. Isolate risky shared-infrastructure changes into small reviewable PRs. + + + +- **Require full verification, resolved reviews, and explicit authorization before merging**: Before marking work done or merging, inspect the exact remote PR head and complete diff; run tsc, lint, Prettier, full tests, and relevant docs build/preview. Conduct an adversarial review, resolve every human and bot thread, and confirm CI, security scans, approvals, draft status, and GitHub mergeability are clean. Treat pending checks, blocked states, stale approvals, or head changes as reasons to pause. Ensure only intended committed files are pushed. Wait for explicit user authorization, then use GitHub API admin squash-merge, never \`gh pr merge\`. Verify the post-merge state and investigate any new Dependabot alerts separately. + + + +- **Require rigorous read-only adversarial reviews with definitive verdicts**: Before approving, merging, or declaring changes ready, conduct an independent, adversarial, read-only audit of the exact current head against its explicit base. Unless authorized, do not edit, format, generate, install, build, test, stage, commit, push, or otherwise mutate the workspace. Inspect the complete diff, relevant unchanged source and callers, tests, docs, workflows, and generated artifacts. Verify requested behavior, security, compatibility, fail-closed and dry-run behavior, side-effect ordering, race safety, regression coverage, CI/review requirements, and diff hygiene. Report severity-ranked findings first with precise file:line evidence and practical fixes, explicitly assess each requested contract, list residual risks or testing gaps, and end with an unambiguous merge verdict. Re-audit after every fix or head change. + + + +- **Require strict read-only, fail-closed release-workflow audits**: When reviewing publish and CI-poller changes, perform an adversarial read-only audit of the exact uncommitted diff and relevant unchanged callers. Verify parser/documentation parity, canonical input authority, exact validation rules, byte preservation, safe cleanup, workflow triggers and gates, checkout/version requirements, JSON serialization, tests, and unrelated-diff hygiene. Treat destructive workflow paths as fail-closed: invalid or empty resolver/rewrite output must never edit issue content, and temporary files must be cleaned on every exit path. Preserve \`workflow_dispatch\` for CI-poller recovery. Report severity-ranked findings with precise file:line citations, explicit PASS/FAIL coverage for requested invariants, testing gaps, and end exactly with \`MERGE\` or \`DO-NOT-MERGE\`. - + + +- **Require strict validation without normalization**: For Publish workflow inputs from issue titles/bodies, paths, repositories, versions, revisions, and workspace names, use explicit narrow validation and fail closed on malformed or ambiguous values. Preserve identities, paths, and casing exactly; never normalize them. Enforce the canonical title grammar through a shared resolver before CI/API side effects, reject unsafe segments such as \`.\`, \`..\`, \`\_\_proto\_\_\`, and leading \`-\`, and require canonical, uniquely recognized structured links for revisions. + + + +- **Review code before committing**: Behavioral pattern detected across 24 sessions (action: requested-review). The user consistently demonstrates this behavior. + + + +- **State and enforce critical behavioral contracts explicitly**: When implementing or reviewing changes, explicitly state non-negotiable behavioral invariants—especially for release safety, parsing, validation, event ordering, identity, and compatibility—and enforce them at the earliest relevant boundary with focused regression tests. Preserve established behavior rather than silently normalizing or broadening semantics. Treat user-stated constraints as authoritative: for example, option-looking tokens must not become workspace names; prerelease relaxation applies only to the running version, never configured \`minVersion\`; the CI poller adds \`ci-ready\`; and unused-variable rules must not be mischaracterized. Require focused and full verification, distinguish new failures from known baseline warnings, and audit final diffs, documentation, workflows, and tests against the contract. + + + +- **Treat explicit user and repository invariants as binding contracts**: Treat behavior stated as “always,” “never,” “intentional,” or with exact wording as an authoritative contract, not a suggestion. Preserve exact spelling, identity, formatting, tool choices, staging boundaries, fallback behavior, and compatibility semantics across all modes and edge cases. Enforce the contract at the earliest reliable boundary and fail closed where safety or release correctness requires it; do not weaken it through normalization, cached assumptions, fallbacks, or mode-specific exceptions. Preserve unrelated working-tree changes and add focused regressions covering the invariant, failure path, and absence of unintended side effects. If implementation evidence conflicts with the stated contract, surface the discrepancy rather than silently choosing another convention. + + + +- **Trust stated system invariants**: Treat the user’s explicit domain guarantees as authoritative. Do not add defensive handling for states the user says cannot occur, such as wrapping \`JSON.parse()\` for input guaranteed to be valid JSON. Preserve stated invariants exactly, including required repository identity, checkout-path behavior, and no name normalization; use explicit errors only for genuinely unexpected values. Prefer requested architectural cleanup, such as consolidating parsing under Peggy. + + -- **Require primary-source verification before answering technical/design questions**: When the user raises questions about third-party tool behavior (e.g., Cloudflare wrangler CLI semantics, defaults, auth mechanisms), they expect answers grounded in verified primary sources — actual installed tool source code, \`--help\` output, and official vendor documentation — not assumptions or prior knowledge. The user explicitly states not to guess and often specifies concrete verification steps (e.g., install the exact tool version locally, run help commands, grep/read bundled source, cross-reference with official docs, cite URLs). This pattern recurs around code review: after leaving review comments on a PR/doc raising design questions (e.g., 'is this really the default?', 'can this be auto-inferred?', 'is this actually a secret?'), the user expects the assistant to research each question thoroughly using the codebase/installed dependencies before proposing doc or code changes. Always verify claims against actual source/docs before answering, and cite exact file/line locations or doc URLs as evidence. +- **Use a dedicated, up-to-date branch worktree for changes**: Before making changes, create or use a dedicated branch and worktree rather than modifying the primary checkout. Start new work from the appropriate current base branch; check the branch's remote state and fast-forward/update the worktree first so remote changes are preserved. Keep unrelated local, untracked planning, and agent files intact and out of implementation commits. - + -- **Require rigorous, honest adversarial code review before merging**: Before merging any PR, the user expects an explicit, no-shortcuts adversarial review pass: verify committed HEAD state matches intended working-tree changes (don't assume uncommitted edits are already in the commit), run full verification suite (tsc --noEmit, targeted tests, full test suite, prettier --check on changed files, docs build), enumerate any behavior divergences from prior logic and judge whether each is an intended fix or a regression, check edge cases explicitly (e.g., HEAD refs, trailing/leading slashes, empty values), and produce a prioritized CRITICAL/MAJOR/MINOR findings list with file:line references and concrete fixes. The user explicitly forbids rubber-stamping and requires the assistant to ground claims in actual source code rather than summaries — when the assistant makes an unverified claim (e.g., 'data-corruption bug'), the user pushes back and demands the assistant re-verify against real code before restating it. Also proactively run prettier before finishing, since past PRs failed CI on formatting. +- **Use an isolated, test-backed workflow for issue fixes**: When implementing a GitHub issue fix, first fetch the latest \`origin/master\`, create a dedicated fix branch from it, and preserve unrelated or untracked worktree changes. Inspect the issue, relevant callers, contracts, recent history, and existing tests before making the smallest targeted change. Add focused regression coverage for the reported failure while preserving ordinary behavior, then run focused tests and the project’s full validation suite, audit the final diff for unrelated files, and only then commit and push. For review-only requests, strictly honor read-only constraints and do not rerun checks the user says were already completed. - + -- **Require rigorous, structured multi-phase design-then-adversarial-review workflow for architectural changes**: When working on non-trivial features (e.g., the craft 'workspaces' redesign), the user consistently: (1) requires investigation/design-doc phases before any code is written, explicitly forbidding code changes during research; (2) demands file:line-cited, exhaustive inventories of every touchpoint affected by a design change; (3) splits large efforts into small mergeable PRs (salvage/fix PR first, then schema, resolver, selector, action-layer, docs as separate follow-up PRs); (4) insists on explicit backward-compatibility guarantees (new features must be fully inert/no-op when unused); (5) after implementation, requests an adversarial senior-code-reviewer style review with specific prioritized focus areas (backward compat, merge/resolution semantics, caching correctness, edge cases in version/gate logic, CLI wiring) rather than a rubber-stamp; (6) requires findings prioritized as CRITICAL/MAJOR/MINOR with concrete file:line fixes and direct yes/no answers to specific risk questions; (7) always wants full verification (tsc, full test suite, lint/prettier) run before considering work done. Follow this same design→review→verify cadence for future architectural or schema-changing tasks. +- **Use concrete workspace paths for glob-based configuration**: When workspace keys are directory glob patterns, expand them into the exact discovered workspace paths rather than treating patterns as workspace identities. Validate the expanded mapping so that a concrete path matched by more than one configured glob is rejected with a clear ambiguity error. Include regression coverage for overlapping patterns and verify representative glob syntaxes resolve to the expected concrete workspace path. - + -- **Respect explicitly rejected approaches**: Behavioral pattern detected across 3 sessions (action: rejected-approach). The user consistently demonstrates this behavior. +- **Use exact CI-approved identities and authoritative state for Publish decisions**: Use exact, lossless CI-approved checkout and workspace identities throughout Publish parsing, resolution, validation, discovery, titles, and state keys. New Craft requests include the checkout repository identity; preserve POSIX \`/\` paths and exact case. After checking out the CI-approved revision, classify a suffix as a workspace only by exact \`craft workspace list\` membership; retain nonmatches as checkout paths. A missing root \`.craft.yml\` means checkout-path behavior; fail closed when discovery is unreliable. Base decisions on authoritative runtime state, not normalization or mutable issue metadata, and account explicitly for GitHub Actions event semantics such as idempotent label updates. - + -- **Review code before committing**: Behavioral pattern detected across 3 sessions (action: requested-review). The user consistently demonstrates this behavior. +- **Use precise and safe Git remote-operation guidance**: When discussing Git refspecs or rewritten published history, state the exact semantics and safety implications. Explain that pattern refspecs require one \`\*\` in both source and destination and substitute the source match into the destination, such as \`refs/heads/\*:refs/heads/\*\` for all branches. For rebased commits already pushed, explain that replacing remote history requires bypassing fast-forward protection, but never recommend blind \`--force\`; account for possible concurrent remote updates and prefer a guarded approach such as \`--force-with-lease\`. - + -- **Use pnpm for package management**: Always use \`pnpm\` for package management, never use \`npm\` or \`yarn\`. +- **Verify exact implementation details before making changes**: Inspect the repository’s current source, tests, CLI options, lockfile, and—when behavior depends on a library—the exact installed dependency implementation rather than relying on assumptions or generic documentation. Trace call sites and argument ordering, preserve documented invariants and recovery paths, and account for edge cases such as parsing ambiguity, remote Git behavior, failure handling, and security boundaries. Support conclusions with concrete file locations and validate changes using targeted regression tests and relevant broader test suites. - + -- **Verify architectural claims against actual source code before accepting them, and drive design decisions via explicit Q\&A before any implementation**: When the assistant makes an architectural or risk claim (e.g., 'latent data-corruption bug', 'workspace term already used'), the user pushes back and demands the assistant re-verify against actual source code/line citations rather than repeating a summary — leading the assistant to self-correct or confirm with precise evidence. The user also consistently drives multi-step technical redesigns through iterative, explicit design questions (e.g., issue-title format, term reuse, config shape, migration strategy) rather than letting the assistant proceed on assumptions, answering each with a clear decision before coding starts. The user explicitly forbids writing code during investigation/design phases, requires file:line-cited evidence for claims, wants decisions recorded in a durable design doc, and expects any risky/coupled change (e.g., touching shared infra like getsentry/publish) to be isolated into its own reviewable, revertable PR. Always ground claims in code, ask clarifying design questions before implementing, and document agreed decisions before proceeding. +- **Verify live workflow configuration before dispatching releases**: Before triggering or changing a release workflow, inspect the repository’s current GitHub Actions YAML, workflow inputs, permissions, checkout/authentication behavior, and recent runs. Use the documented normal Release workflow and its supported inputs rather than assuming versioning or invocation details. When modifying related publish/release workflow code, validate the live workflow state and run focused contract tests before broader test suites. - + -- **Verify version-gate constants against actual package.json version before hardcoding them**: When implementing or reviewing minimum-version gates (e.g., WORKSPACES_MIN_VERSION, AUTO_VERSION), always cross-check the chosen constant against the current package.json version string before finalizing. Since checkMinimalConfigVersion requires the running package version to be >= a config's declared minVersion, setting a gate to a future/unreleased version (e.g., '2.28.0' when package.json is '2.27.0-dev.0') will break tests and block usage until release. Do not guess or hardcode a future release version — instead: (1) read package.json's actual version, (2) reason about how pre-release/dev suffixes compare in SemVer, and (3) if the correct value affects feature usability at release or requires a product decision, explicitly flag it to the user for confirmation rather than silently picking a value. Also verify whether the gate is even necessary given schema opt-in status. +- **Wait for CI approval before publishing releases**: For release publishing, follow the label-driven workflow rather than attempting to publish early. Prepare the release and approve its publish request, then wait for all required CI workflows to finish. Treat \`ci-pending\` as a block; the poller adds \`ci-ready\` once CI passes, which triggers publishing when the request is also \`accepted\`. Monitor the publish workflow through completion and verify that the release artifact exists only after the run succeeds. diff --git a/src/commands/__tests__/publish-main.test.ts b/src/commands/__tests__/publish-main.test.ts new file mode 100644 index 00000000..d5b34e1a --- /dev/null +++ b/src/commands/__tests__/publish-main.test.ts @@ -0,0 +1,237 @@ +import { captureException } from '@sentry/node'; +import { mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join as pathJoin } from 'path'; +import type { SimpleGit } from 'simple-git'; +import { + afterEach, + beforeEach, + describe, + expect, + test, + vi, + type Mock, +} from 'vitest'; + +import { + expandWorkspaceTargets, + getActiveWorkspace, + getArtifactProviderFromConfig, + getConfiguration, + getGlobalGitHubConfig, + getNoMergeConfig, + getStatusProviderFromConfig, +} from '../../config'; +import { logger } from '../../logger'; +import { getTargetByName } from '../../targets'; +import { BaseTarget } from '../../targets/base'; +import { safeFs } from '../../utils/dryRun'; +import { promptConfirmation } from '../../utils/helpers'; +import { getPublishStatePath } from '../../utils/publishState'; +import { + findReleaseBranches, + getDefaultBranch, + getGitClient, + isRepoDirty, +} from '../../utils/git'; +import { BranchCleanupError, publishMain } from '../publish'; + +vi.mock('@sentry/node', () => ({ + captureException: vi.fn(), + startSpan: vi.fn((_options: unknown, callback: () => unknown): unknown => + callback(), + ), +})); +vi.mock('../../config', async importOriginal => ({ + ...(await importOriginal()), + expandWorkspaceTargets: vi.fn(), + getActiveWorkspace: vi.fn(), + getArtifactProviderFromConfig: vi.fn(), + getConfiguration: vi.fn(), + getGlobalGitHubConfig: vi.fn(), + getNoMergeConfig: vi.fn(), + getStatusProviderFromConfig: vi.fn(), +})); +vi.mock('../../utils/git', () => ({ + findReleaseBranches: vi.fn(), + getDefaultBranch: vi.fn(), + getGitClient: vi.fn(), + isRepoDirty: vi.fn(), +})); +vi.mock('../../targets', async importOriginal => ({ + ...(await importOriginal()), + getTargetByName: vi.fn(), +})); +vi.mock('../../utils/helpers', async importOriginal => ({ + ...(await importOriginal()), + promptConfirmation: vi.fn(), +})); +vi.mock('../../utils/publishState', async importOriginal => ({ + ...(await importOriginal()), + getPublishStatePath: vi.fn(), +})); + +function createMockGit(revision: string): SimpleGit { + return { + branch: vi.fn().mockResolvedValue(undefined), + branchLocal: vi.fn().mockResolvedValue({ all: [] }), + checkout: vi.fn().mockResolvedValue(undefined), + merge: vi.fn().mockResolvedValue(undefined), + pull: vi.fn().mockResolvedValue(undefined), + push: vi.fn().mockResolvedValue(undefined), + raw: vi.fn().mockResolvedValue(''), + revparse: vi.fn().mockResolvedValue(revision), + status: vi.fn().mockResolvedValue({ files: [] }), + } as unknown as SimpleGit; +} + +const defaultOptions = { + remote: 'origin', + mergeTarget: 'main', + target: 'all', + newVersion: '1.2.3', + noMerge: false, + keepDownloads: false, + noStatusCheck: true, + keepBranch: false, + noGitChecks: true, +}; + +const publishTarget = vi.fn(); +const testState = { directory: '' }; + +class TestTarget extends BaseTarget { + public override publish(version: string, revision: string): Promise { + return publishTarget(version, revision); + } +} + +describe('publishMain release branch handling', () => { + beforeEach(async () => { + vi.clearAllMocks(); + testState.directory = await mkdtemp( + pathJoin(tmpdir(), 'craft-publish-main-'), + ); + vi.mocked(getPublishStatePath).mockReturnValue( + pathJoin(testState.directory, 'publish-state.json'), + ); + vi.mocked(getConfiguration).mockReturnValue({ + releaseBranchPrefix: 'stable', + targets: [{ name: 'test' }], + postReleaseCommand: '', + }); + vi.mocked(getStatusProviderFromConfig).mockResolvedValue({} as never); + vi.mocked(getArtifactProviderFromConfig).mockResolvedValue({ + listArtifactsForRevision: vi.fn().mockResolvedValue([]), + setDownloadDirectory: vi.fn(), + } as never); + vi.mocked(getGlobalGitHubConfig).mockResolvedValue({ + owner: 'getsentry', + repo: 'craft', + }); + vi.mocked(expandWorkspaceTargets).mockResolvedValue([{ name: 'test' }]); + vi.mocked(getTargetByName).mockReturnValue(TestTarget); + vi.mocked(promptConfirmation).mockResolvedValue(undefined); + vi.mocked(getNoMergeConfig).mockReturnValue({ + noMerge: false, + source: 'config', + }); + vi.mocked(getActiveWorkspace).mockReturnValue(undefined); + vi.mocked(isRepoDirty).mockReturnValue(false); + vi.mocked(findReleaseBranches).mockResolvedValue({ + exactMatches: [], + fuzzyMatches: [], + }); + vi.mocked(getDefaultBranch).mockResolvedValue('main'); + vi.spyOn(safeFs, 'unlink').mockResolvedValue(undefined); + vi.spyOn(safeFs, 'writeFileSync').mockImplementation(() => undefined); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await rm(testState.directory, { recursive: true, force: true }); + }); + + test('publishes and merges an approved revision while deleting the canonical branch', async () => { + const requestedRevision = 'v1.2.3'; + const approvedRevision = 'a'.repeat(40); + const git = createMockGit(approvedRevision); + (git.raw as Mock).mockImplementation((...args: string[]) => + Promise.resolve( + args[0] === 'name-rev' ? 'remotes/origin/stable/1.2.3\n' : '', + ), + ); + vi.mocked(getGitClient).mockResolvedValue(git); + + await publishMain({ ...defaultOptions, rev: requestedRevision }); + + expect(git.checkout).toHaveBeenNthCalledWith(1, requestedRevision); + expect(publishTarget).toHaveBeenCalledWith('1.2.3', approvedRevision); + expect(git.raw).not.toHaveBeenCalledWith( + 'name-rev', + '--name-only', + '--no-undefined', + requestedRevision, + ); + expect(git.merge).toHaveBeenCalledWith([ + '--no-ff', + '--no-edit', + approvedRevision, + ]); + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/stable/1.2.3', + [`--force-with-lease=refs/heads/stable/1.2.3:${approvedRevision}`], + ); + expect(git.branch).not.toHaveBeenCalled(); + }); + + test('keeps the ordinary release branch checkout and merge path', async () => { + const revision = 'b'.repeat(40); + const git = createMockGit(revision); + vi.mocked(getGitClient).mockResolvedValue(git); + + await publishMain(defaultOptions); + + expect(git.checkout).toHaveBeenNthCalledWith(1, 'stable/1.2.3'); + expect(publishTarget).toHaveBeenCalledWith('1.2.3', revision); + expect(git.merge).toHaveBeenCalledWith([ + '--no-ff', + '--no-edit', + 'stable/1.2.3', + ]); + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/stable/1.2.3', + [`--force-with-lease=refs/heads/stable/1.2.3:${revision}`], + ); + }); + + test('reports remote deletion failures as cleanup failures', async () => { + const requestedRevision = 'release-candidate'; + const approvedRevision = 'c'.repeat(40); + const git = createMockGit(approvedRevision); + vi.mocked(git.push) + .mockResolvedValueOnce({} as never) + .mockRejectedValueOnce(new Error('remote delete failed')); + vi.mocked(getGitClient).mockResolvedValue(git); + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + + await publishMain({ ...defaultOptions, rev: requestedRevision }); + + expect(captureException).toHaveBeenCalledWith( + expect.any(BranchCleanupError), + ); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('Failed to clean up release branch'), + ); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining( + `--force-with-lease=refs/heads/stable/1.2.3:${approvedRevision}`, + ), + ); + expect(warn).not.toHaveBeenCalledWith( + expect.stringContaining('Failed to merge release branch'), + ); + }); +}); diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index 3e866f72..b4191f50 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -1,11 +1,14 @@ +import { mkdir, mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; import { vi, describe, test, expect, beforeEach, type Mock } from 'vitest'; import { join as pathJoin } from 'path'; import { spawnProcess, hasExecutable } from '../../utils/system'; +import { createGitClient } from '../../utils/git'; import { getPublishStateGitHubConfig, - getRevisionBranchName, runPostReleaseCommand, handleReleaseBranch, + BranchCleanupError, MergeConflictError, PushError, } from '../publish'; @@ -13,7 +16,8 @@ import { getPublishStateFilename } from '../../utils/publishState'; import type { SimpleGit } from 'simple-git'; vi.mock('../../utils/system'); -vi.mock('../../utils/git', () => ({ +vi.mock('../../utils/git', async importOriginal => ({ + ...(await importOriginal()), getDefaultBranch: vi.fn().mockResolvedValue('main'), getGitClient: vi.fn(), isRepoDirty: vi.fn(), @@ -210,33 +214,9 @@ describe('getPublishStateGitHubConfig', () => { }); }); -describe('getRevisionBranchName', () => { - test('returns the named ref for a revision when available', async () => { - const git = { - raw: vi.fn().mockResolvedValue('release/1.2.3\n'), - } as unknown as SimpleGit; - - await expect(getRevisionBranchName(git, 'abc123')).resolves.toBe( - 'release/1.2.3', - ); - expect(git.raw).toHaveBeenCalledWith( - 'name-rev', - '--name-only', - '--no-undefined', - 'abc123', - ); - }); - - test('allows a detached CI-approved revision', async () => { - const git = { - raw: vi.fn().mockRejectedValue(new Error('Could not get ref name')), - } as unknown as SimpleGit; - - await expect(getRevisionBranchName(git, 'abc123')).resolves.toBe(''); - }); -}); - describe('handleReleaseBranch', () => { + const releaseRevision = 'abc123'; + /** * Creates a mock SimpleGit instance where each method returns * a chainable object (SimpleGit & Promise), matching simple-git's API. @@ -264,8 +244,9 @@ describe('handleReleaseBranch', () => { mockGit.merge = makeChainable(); mockGit.push = makeChainable(); mockGit.branch = makeChainable(); + mockGit.branchLocal = makeChainable({ all: ['release/1.0.0'] }); mockGit.remote = makeChainable(); - mockGit.revparse = makeChainable('main'); + mockGit.revparse = makeChainable(releaseRevision); mockGit.raw = makeChainable(''); mockGit.status = makeChainable({ conflicted: [] }); mockGit.diff = makeChainable(''); @@ -273,6 +254,50 @@ describe('handleReleaseBranch', () => { return mockGit as unknown as SimpleGit & Record; } + async function createReleaseRepository( + pushAdvanced: boolean, + advanceLocal = true, + ) { + const directory = await mkdtemp( + pathJoin(tmpdir(), 'craft-publish-cleanup-'), + ); + const repository = pathJoin(directory, 'repository'); + const remote = pathJoin(directory, 'remote.git'); + const branch = 'release/1.0.0'; + const branchRef = `refs/heads/${branch}`; + + await createGitClient(directory).raw('init', '--bare', remote); + await mkdir(repository); + const git = createGitClient(repository); + await git.raw('init', '-b', 'main'); + await git.addConfig('user.name', 'Craft Test'); + await git.addConfig('user.email', 'craft@example.com'); + await git.addConfig('commit.gpgsign', 'false'); + await git.raw('commit', '--allow-empty', '-m', 'approved release'); + const approvedRevision = (await git.revparse('HEAD')).trim(); + await git.raw('branch', branch); + await git.addRemote('origin', remote); + await git.raw('push', 'origin', 'main', branch); + await git.checkout(branch); + if (advanceLocal) { + await git.raw('commit', '--allow-empty', '-m', 'advanced release'); + } + const advancedRevision = (await git.revparse('HEAD')).trim(); + if (pushAdvanced) { + await git.push('origin', branch); + } + await git.checkout('main'); + + return { + directory, + git, + branch, + branchRef, + approvedRevision, + advancedRevision, + }; + } + beforeEach(() => { vi.clearAllMocks(); }); @@ -280,7 +305,13 @@ describe('handleReleaseBranch', () => { test('successful merge with default strategy', async () => { const git = createMockGit(); - await handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main'); + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ); expect(git.checkout).toHaveBeenCalledWith('main'); expect(git.pull).toHaveBeenCalledWith('origin', 'main', ['--rebase']); @@ -311,7 +342,13 @@ describe('handleReleaseBranch', () => { return Promise.resolve(); }); - await handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main'); + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ); expect(git.merge).toHaveBeenCalledTimes(3); // First attempt: default strategy @@ -344,7 +381,13 @@ describe('handleReleaseBranch', () => { .mockImplementationOnce(() => Promise.reject(abortError)) .mockImplementationOnce(() => Promise.resolve()); - await handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main'); + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ); expect(git.merge).toHaveBeenCalledTimes(3); expect(git.merge).toHaveBeenNthCalledWith(2, ['--abort']); @@ -382,6 +425,7 @@ describe('handleReleaseBranch', () => { git, 'origin', 'release/1.0.0', + releaseRevision, 'main', ).catch((e: unknown) => e); @@ -415,6 +459,7 @@ describe('handleReleaseBranch', () => { git, 'origin', 'release/1.0.0', + releaseRevision, 'main', ).catch((e: unknown) => e); @@ -431,7 +476,13 @@ describe('handleReleaseBranch', () => { (git.pull as Mock).mockImplementationOnce(() => Promise.reject(pullError)); await expect( - handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main'), + handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ), ).rejects.toThrow('CONFLICT during rebase'); // rebase --abort should have been called to clean up @@ -441,26 +492,214 @@ describe('handleReleaseBranch', () => { expect(git.push).not.toHaveBeenCalledWith('origin', 'main'); }); - test('deletes branch after successful merge', async () => { + test('deletes the remote and local branches after successful merge', async () => { const git = createMockGit(); - await handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main', false); + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + false, + ); - expect(git.branch).toHaveBeenCalledWith(['-D', 'release/1.0.0']); - // push --delete is chained from branch() - expect(git.push).toHaveBeenCalledWith([ + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/release/1.0.0', + ['--force-with-lease=refs/heads/release/1.0.0:abc123'], + ); + expect(git.branchLocal).toHaveBeenCalledOnce(); + expect(git.branch).toHaveBeenCalledWith(['-d', '--', 'release/1.0.0']); + }); + + test('merges an approved revision and deletes its canonical remote branch', async () => { + const git = createMockGit(); + await handleReleaseBranch( + git, 'origin', - '--delete', 'release/1.0.0', - ]); + releaseRevision, + 'main', + false, + 'abc123', + ); + + expect(git.merge).toHaveBeenCalledWith(['--no-ff', '--no-edit', 'abc123']); + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/release/1.0.0', + ['--force-with-lease=refs/heads/release/1.0.0:abc123'], + ); + expect(git.branch).toHaveBeenCalledWith(['-d', '--', 'release/1.0.0']); + }); + + test('preserves an advanced remote branch when the deletion lease fails', async () => { + const git = createMockGit(); + const cleanupError = new Error('stale info'); + (git.push as Mock) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(cleanupError); + + const error = await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(BranchCleanupError); + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/release/1.0.0', + ['--force-with-lease=refs/heads/release/1.0.0:abc123'], + ); + expect(git.branchLocal).not.toHaveBeenCalled(); + }); + + test('preserves an advanced remote branch when Git rejects the deletion lease', async () => { + const { + directory, + git, + branch, + branchRef, + approvedRevision, + advancedRevision, + } = await createReleaseRepository(true); + + try { + const error = await handleReleaseBranch( + git, + 'origin', + branch, + approvedRevision, + 'main', + false, + approvedRevision, + ).catch((cleanupError: unknown) => cleanupError); + + expect(error).toBeInstanceOf(BranchCleanupError); + expect((error as BranchCleanupError).remoteDeleted).toBe(false); + expect((await git.revparse(branchRef)).trim()).toBe(advancedRevision); + expect( + (await git.raw('ls-remote', '--heads', 'origin', branchRef)).split( + '\t', + )[0], + ).toBe(advancedRevision); + } finally { + await rm(directory, { recursive: true, force: true }); + } + }); + + test('preserves an advanced local branch after deleting the remote branch', async () => { + const { + directory, + git, + branch, + branchRef, + approvedRevision, + advancedRevision, + } = await createReleaseRepository(false); + + try { + const error = await handleReleaseBranch( + git, + 'origin', + branch, + approvedRevision, + 'main', + false, + approvedRevision, + ).catch((cleanupError: unknown) => cleanupError); + + expect(error).toBeInstanceOf(BranchCleanupError); + expect((error as BranchCleanupError).remoteDeleted).toBe(true); + expect((await git.revparse(branchRef)).trim()).toBe(advancedRevision); + await expect( + git.raw('ls-remote', '--heads', 'origin', branchRef), + ).resolves.toBe(''); + } finally { + await rm(directory, { recursive: true, force: true }); + } + }); + + test('deletes matching local and remote branches with Git porcelain', async () => { + const { directory, git, branch, branchRef, approvedRevision } = + await createReleaseRepository(false, false); + + try { + await handleReleaseBranch( + git, + 'origin', + branch, + approvedRevision, + 'main', + false, + approvedRevision, + ); + + await expect(git.revparse(branchRef)).rejects.toThrow(); + await expect( + git.raw('ls-remote', '--heads', 'origin', branchRef), + ).resolves.toBe(''); + } finally { + await rm(directory, { recursive: true, force: true }); + } + }); + + test('reports local cleanup failure after deleting the remote branch', async () => { + const git = createMockGit(); + (git.branch as Mock).mockRejectedValueOnce( + new Error('branch is checked out'), + ); + + const error = await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ).catch((cleanupError: unknown) => cleanupError); + + expect(error).toBeInstanceOf(BranchCleanupError); + expect((error as BranchCleanupError).remoteDeleted).toBe(true); + expect(git.push).toHaveBeenCalledWith( + 'origin', + ':refs/heads/release/1.0.0', + ['--force-with-lease=refs/heads/release/1.0.0:abc123'], + ); + }); + + test('skips local deletion when the canonical branch does not exist', async () => { + const git = createMockGit(); + (git.branchLocal as Mock).mockResolvedValueOnce({ all: [] }); + + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + ); + + expect(git.branch).not.toHaveBeenCalled(); }); test('does not delete branch when keepBranch is true', async () => { const git = createMockGit(); - await handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main', true); + await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + releaseRevision, + 'main', + true, + ); - expect(git.branch).not.toHaveBeenCalledWith(['-D', 'release/1.0.0']); + expect(git.branchLocal).not.toHaveBeenCalled(); + expect(git.branch).not.toHaveBeenCalled(); }); test('resolves default branch when mergeTarget is not provided', async () => { @@ -469,7 +708,7 @@ describe('handleReleaseBranch', () => { const git = createMockGit(); - await handleReleaseBranch(git, 'origin', 'release/1.0.0'); + await handleReleaseBranch(git, 'origin', 'release/1.0.0', releaseRevision); expect(getDefaultBranch).toHaveBeenCalledWith(git, 'origin'); expect(git.checkout).toHaveBeenCalledWith('master'); @@ -516,3 +755,14 @@ describe('PushError', () => { expect(err.message).toBe('could not read Username'); }); }); + +describe('BranchCleanupError', () => { + test('records whether remote cleanup completed', () => { + const err = new BranchCleanupError('cleanup failed', true); + + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(BranchCleanupError); + expect(err.message).toBe('cleanup failed'); + expect(err.remoteDeleted).toBe(true); + }); +}); diff --git a/src/commands/publish.ts b/src/commands/publish.ts index dad43f31..07d2a6fc 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -447,24 +447,43 @@ export class PushError extends Error { } } +/** + * Error thrown when release branch cleanup fails after the merge was pushed. + */ +export class BranchCleanupError extends Error { + public __proto__: Error; + public readonly remoteDeleted: boolean; + + public constructor(message: string, remoteDeleted: boolean) { + const trueProto = new.target.prototype; + super(message); + this.__proto__ = trueProto; + this.remoteDeleted = remoteDeleted; + } +} + /** * Deals with the release branch after publishing is done * - * Leave the release branch unmerged, or merge it but not delete it if the + * Leave the release branch unmerged, or merge it but preserve it if the * corresponding flags are set. * * @param git Git client * @param remoteName The git remote name to interact with * @param branch Name of the release branch + * @param releaseRevision Expected release branch revision * @param [mergeTarget] Branch name to merge the release branch into * @param keepBranch If set to "true", the branch will not be deleted + * @param mergeSource Revision to merge, defaults to the release branch */ export async function handleReleaseBranch( git: SimpleGit, remoteName: string, branch: string, + releaseRevision: string, mergeTarget?: string, keepBranch = false, + mergeSource = branch, ): Promise { if (!mergeTarget) { mergeTarget = await getDefaultBranch(git, remoteName); @@ -486,9 +505,9 @@ export async function handleReleaseBranch( } // Stage 1: Merge — if this fails, it's a merge conflict - logger.debug(`Merging ${branch} into: ${mergeTarget}`); + logger.debug(`Merging ${mergeSource} into: ${mergeTarget}`); try { - await git.merge(['--no-ff', '--no-edit', branch]); + await git.merge(['--no-ff', '--no-edit', mergeSource]); } catch (mergeError) { // Default strategy (ort) failed — abort and retry with resolve strategy logger.warn( @@ -504,7 +523,7 @@ export async function handleReleaseBranch( // Retry with the resolve strategy which handles criss-cross ambiguities // differently and often succeeds where ort fails on files like CHANGELOG.md try { - await git.merge(['-s', 'resolve', '--no-ff', '--no-edit', branch]); + await git.merge(['-s', 'resolve', '--no-ff', '--no-edit', mergeSource]); } catch (resolveError) { // Resolve also failed — capture conflict details before aborting let conflictedFiles: string[] = []; @@ -549,9 +568,42 @@ export async function handleReleaseBranch( if (keepBranch) { logger.info('Not deleting the release branch.'); } else { - logger.debug(`Deleting the release branch: ${branch}`); - await git.branch(['-D', branch]).push([remoteName, '--delete', branch]); + logger.debug(`Deleting the remote release branch: ${branch}`); + const branchRef = `refs/heads/${branch}`; + try { + await git.push(remoteName, `:${branchRef}`, [ + `--force-with-lease=${branchRef}:${releaseRevision}`, + ]); + } catch (cleanupError) { + throw new BranchCleanupError( + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError), + false, + ); + } logger.info(`Removed the remote branch: "${branch}"`); + + try { + const localBranches = await git.branchLocal(); + if (localBranches.all.includes(branch)) { + const localRevision = (await git.revparse(branchRef)).trim(); + if (localRevision !== releaseRevision) { + throw new Error( + `Local release branch is at ${localRevision}, expected ${releaseRevision}`, + ); + } + await git.branch(['-d', '--', branch]); + logger.info(`Removed the local branch: "${branch}"`); + } + } catch (cleanupError) { + throw new BranchCleanupError( + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError), + true, + ); + } } } @@ -627,22 +679,15 @@ export async function publishMain(argv: PublishOptions): Promise { config.releaseBranchPrefix || DEFAULT_RELEASE_BRANCH_NAME; const rev = argv.rev; - let checkoutTarget; - let branchName; + const branchName = `${branchPrefix}/${newVersion}`; if (rev) { - logger.debug(`Trying to get branch name for provided revision: "${rev}"`); - branchName = await getRevisionBranchName(git, rev); - checkoutTarget = branchName || rev; - logger.debug('Checking out revision', checkoutTarget); - await git.checkout(checkoutTarget); + logger.debug('Checking out revision', rev); + await git.checkout(rev); } else { // Find the remote branch - branchName = `${branchPrefix}/${newVersion}`; - checkoutTarget = branchName; - try { logger.debug('Checking out release branch', branchName); - await git.checkout(checkoutTarget); + await git.checkout(branchName); } catch (err) { const { exactMatches, fuzzyMatches } = await findReleaseBranches( git, @@ -677,7 +722,7 @@ export async function publishMain(argv: PublishOptions): Promise { } } - const revision = await git.revparse('HEAD'); + const revision = (await git.revparse('HEAD')).trim(); logger.debug('Revision to publish: ', revision); const statusProvider = await getStatusProviderFromConfig(); @@ -830,10 +875,6 @@ export async function publishMain(argv: PublishOptions): Promise { ? 'auto-detection (compiled GitHub Action with dist/ folder)' : 'config'; logger.info(`Not merging the release branch (${source}).`); - } else if (!branchName) { - logger.info( - 'Not merging because cannot determine a branch name to merge from.', - ); } else if ( targetsToPublish.has(SpecialTarget.All) || targetsToPublish.has(SpecialTarget.None) || @@ -845,8 +886,10 @@ export async function publishMain(argv: PublishOptions): Promise { git, argv.remote, branchName, + revision, argv.mergeTarget, argv.keepBranch, + rev ? revision : branchName, ); } catch (mergeError) { // The merge is a housekeeping step — it must not block the success @@ -854,10 +897,30 @@ export async function publishMain(argv: PublishOptions): Promise { // observability but don't fail the command. captureException(mergeError); - const lines = [ - `Failed to merge release branch "${branchName}" into the target branch.`, - ]; - if (mergeError instanceof MergeConflictError) { + const lines: string[] = []; + if (mergeError instanceof BranchCleanupError) { + lines.push( + `Failed to clean up release branch "${branchName}" after merging it into the target branch.`, + ); + if (mergeError.remoteDeleted) { + lines.push( + `The merge was pushed and the remote release branch was deleted, but deleting the local branch failed.`, + `Inspect the local branch before deleting it.`, + ``, + `To retry safely: git branch -d -- ${branchName}`, + ); + } else { + lines.push( + `The merge was pushed, but deleting the remote release branch failed.`, + `The branch may have advanced beyond the expected release revision ${revision}. Inspect it before cleanup.`, + ``, + `To retry safely: git push --force-with-lease=refs/heads/${branchName}:${revision} ${argv.remote} :refs/heads/${branchName}`, + ); + } + } else if (mergeError instanceof MergeConflictError) { + lines.push( + `Failed to merge release branch "${branchName}" into the target branch.`, + ); lines.push(`Merge conflict — both ort and resolve strategies failed.`); if (mergeError.conflictedFiles.length > 0) { lines.push(``); @@ -878,6 +941,9 @@ export async function publishMain(argv: PublishOptions): Promise { ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, ); } else if (mergeError instanceof PushError) { + lines.push( + `Failed to merge release branch "${branchName}" into the target branch.`, + ); lines.push( `The merge succeeded locally but pushing to the remote failed.`, `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).`, @@ -890,6 +956,9 @@ export async function publishMain(argv: PublishOptions): Promise { ` 3. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, ); } else { + lines.push( + `Failed to merge release branch "${branchName}" into the target branch.`, + ); lines.push( `All publish targets completed successfully — only the post-publish merge failed.`, ``, @@ -930,20 +999,6 @@ export async function publishMain(argv: PublishOptions): Promise { await runPostReleaseCommand(newVersion, config.postReleaseCommand); } -export async function getRevisionBranchName( - git: SimpleGit, - revision: string, -): Promise { - try { - return ( - await git.raw('name-rev', '--name-only', '--no-undefined', revision) - ).trim(); - } catch { - // A CI-approved SHA can be checked out detached without a named ref. - return ''; - } -} - export const handler = async (args: { [argName: string]: any; }): Promise => {