review: aaif-goose/goose#8868 merge-after-nits, QwenLM/qwen-code#3683 merge-after-nits; docs: INDEX +8 PRs (drip-123)

Author: Bojun-Vvibe

reviews/2026-W17/drip-123/QwenLM-qwen-code-pr-3683.md

@@ -0,0 +1,48 @@
+# QwenLM/qwen-code#3683 — Upgrade GitHub Actions to latest versions
+
+- **PR**: https://github.com/QwenLM/qwen-code/pull/3683
+- **Author**: salmanmkc (Salman Chishti)
+- **Head SHA**: `4c7994dd51b119ba4053e7d7536192fb96823e77`
+- **Verdict**: `merge-after-nits`
+
+## Context
+
+The qwen-code repository pins all third-party GitHub Actions to specific commit SHAs with a trailing `# ratchet:owner/repo@vN` comment — the standard `ratchet`-tool pattern that lets a bot detect when a major version has shipped and propose a SHA bump. This PR is a bulk-update sweep across roughly a dozen workflow files: `actions/configure-pages` v5→v6, `actions/upload-pages-artifact` v3→v5, `actions/deploy-pages` v4→v5, `actions/create-github-app-token` v2→v3.1.1, `docker/setup-qemu-action` v3→v4, `docker/setup-buildx-action` v3→v4, `docker/metadata-action` v5→v6, `docker/login-action` v3→v4.1.0, `docker/build-push-action` v6→v7, `dorny/test-reporter` v2→v3.0.0, `thollander/actions-comment-pull-request` v3→v3.0.1, `google-github-actions/run-gemini-cli` v0→v0.1.22.
+
+## What it changes
+
+Mechanical SHA bumps across 12+ workflow files. The pattern is consistent throughout: each `uses: 'owner/repo@<old-sha>' # ratchet:owner/repo@vN` line is rewritten to `uses: 'owner/repo@<new-sha>'  # vN.M.P` (note the formatting drift — see nit 1 below). Spot-checking a few of the bumps against upstream changelogs:
+
+- **`docker/setup-buildx-action` v3 → v4** is a major bump that drops Node 16 runner support and requires Node 20+ — qwen-code's CI runs on `ubuntu-latest` which has Node 20+ since mid-2024, so safe.
+- **`docker/build-push-action` v6 → v7** is a major bump that changes the default Buildx version requirement and the cache-export shape — should be safe for the existing build matrix but worth a single test run before merge.
+- **`actions/upload-pages-artifact` v3 → v5** skips v4 entirely; v4 introduced the `id-token: write` permission requirement for some flows, v5 stabilized it. The repository's `docs-page-action.yml` at `:36` uses this action — needs a check that the calling workflow has `permissions: id-token: write` set.
+- **`google-github-actions/run-gemini-cli` v0 → v0.1.22** is a patch bump within the v0 line, low risk.
+- **`actions/create-github-app-token` v2 → v3.1.1** is a major bump — v3 changed the input schema for some less-common knobs; the diff at `:75-78` and `:155-158` shows the existing `app-id` / `private-key` input shape, which is unchanged across v2 and v3, so the call sites are compatible.
+
+## Design analysis
+
+The right cadence for this kind of sweep is "all the ratchet-tracked actions in one PR" exactly as done here — splitting into one-PR-per-action would 12x the review overhead with no safety gain because the actions are independent of each other. The SHA-pinning convention (`owner/repo@<full-40-char-SHA>`) is preserved throughout, which is the security-relevant invariant — a `uses: 'owner/repo@v4'` would silently float to whatever the maintainer tags as `v4` next, and the explicit SHA is the supply-chain mitigation. Downside: the diff is hard to audit for "did the SHA actually correspond to the claimed version?" without checking each one against the upstream tag — which is exactly what the `ratchet` tool is *supposed* to do automatically, but a reviewer should still spot-check 2-3 of the major bumps.
+
+## Concerns / nits
+
+1. **Comment-format drift** — the existing convention is `# ratchet:owner/repo@vN` (single hash, single space, the literal string `ratchet:`). The new lines use `  # vN.M.P` (double space, no `ratchet:` prefix, exact version string). This breaks the ratchet-tool's ability to parse the file and propose future bumps — the next time `actions/checkout@v5` ships v6, the tool won't know to bump `actions/configure-pages` because its line no longer matches the parser regex. **This is the only blocking nit.** Either preserve the `# ratchet:owner/repo@vN` format on the bumped lines, or update the project's ratchet config to also recognize the new format. Inconsistency is the worst outcome — half the lines parse, half don't.
+
+2. **No CI run referenced in the PR description** — for a sweep that includes two major-version bumps on Docker actions (`setup-buildx-action` v3→v4, `build-push-action` v6→v7), a successful end-to-end build-and-publish run on a test branch would close the regression-detection gap. The other bumps (Pages, run-gemini-cli, dorny/test-reporter) are lower-risk patch/minor bumps where ratchet's own version-resolution proves the SHA→version mapping.
+
+3. **`actions/deploy-pages` v4 → v5** at `:115-116` — v5 requires `permissions: pages: write, id-token: write` on the calling job. The diff doesn't show the surrounding `permissions:` block; the reviewer should verify `docs-page-action.yml`'s deploy job already has both permissions set (it almost certainly does since v4 also required `pages: write`, but `id-token: write` might be new).
+
+4. **`thollander/actions-comment-pull-request` v3 → v3.0.1** at `:9-10` — the diff shows a SHA bump with the comment `# v3.0.1`. v3.0.0 of this action introduced a breaking change to the `message` input shape (now requires the `message:` key instead of accepting the message body directly in some configurations). The call site at `post-coverage-comment/action.yml` should be re-checked for the new input shape. Worth a CI run.
+
+5. **Mixed comment hygiene** — some bumped lines have `# v4.0.0`, some have `# v0.1.22`, some have `# v3.1.1`. The version-string format is inconsistent (some use full semver, some use partial). Not blocking but worth normalizing.
+
+## Risks
+
+Medium. Two major Docker-action bumps that change runtime requirements, one Pages action (`upload-pages-artifact`) that skips a major version. All three need a successful CI run to verify, and the comment-format drift (nit 1) breaks future ratchet automation if not fixed.
+
+## Suggestions
+
+Block on nit 1 (comment-format consistency — either preserve `# ratchet:owner/repo@vN` or update ratchet config) and ask for a successful end-to-end CI run on a test branch covering at least the build-and-publish-image, e2e, and docs-page-action workflows. The other nits are nice-to-have.
+
+## What I learned
+
+The `# ratchet:owner/repo@vN` comment convention is load-bearing for any project using the ratchet tool — it's not just human documentation, it's the parser anchor for "is this action up to date?" detection. Any bulk SHA-bump PR that doesn't preserve the comment format silently turns off the automation that prompted the PR in the first place. Worth grepping for `# ratchet:` on any future SHA-pinned-actions PR to make sure the comment shape survives.

reviews/2026-W17/drip-123/block-goose-pr-8868.md

@@ -0,0 +1,42 @@
+# block/goose#8868 — redesign skills library
+
+- **PR**: https://github.com/block/goose/pull/8868
+- **Author**: morgmart
+- **Head SHA**: `3f6fb45b38dd82787ea5e3b7007378c4683ee097`
+- **Verdict**: `merge-after-nits`
+
+## Context
+
+`goose` ships a skills system (markdown-formatted reusable instruction packs) discovered from a chain of project-scoped and home-scoped directories. Historically the project-scoped writable location was `<project>/.goose/skills` — a goose-specific path that doesn't compose with other agent ecosystems (Claude / `.claude/skills`, the broader `.agents/skills` convention used by spec-kitty and other tooling). This PR migrates the canonical project-scoped writable location to `<project>/.agents/skills`, makes it the *highest-priority* project source so it shadows the legacy `.goose/skills` on collision, and ships a UI redesign of the SkillsView that adds project-aware skill hydration plus a category/source filter chip set. The diff visible at lines 1-200 covers the Rust-side discovery + sources changes; the UI side is referenced but not in the visible window.
+
+## What it changes
+
+**Rust core**: `crates/goose/src/skills/mod.rs:8-13` rewrites `project_skills_dir` to return `<project>/.agents/skills` instead of `<project>/.goose/skills`. The `all_skill_dirs` walker at `:20-25` reorders the project-scoped directory list so `.agents/skills` is searched first, then `.goose/skills`, then `.claude/skills` — meaning a skill named `shared-skill` defined in both `.agents/skills/shared-skill/` and `.goose/skills/shared-skill/` resolves to the `.agents/skills` version. **Sources tests**: existing tests at `crates/goose/src/sources.rs:380` and `:541` are migrated from `.goose/skills` to `.agents/skills` paths. Two new tests pin the new behavior: `list_sources_reads_project_agents_skills` at `:54-73` proves the `.agents/skills` path is discovered with the right description, and `project_sources_prefer_agents_directory_over_legacy_goose` at `:75-113` constructs a collision (same skill name in both `.agents/skills` and `.goose/skills`) and asserts the listed result has length 1, points to `.agents/skills`, has the "preferred" description (not "legacy"), and the exported source contains the agents-directory content. **UI**: `ui/goose2/scripts/check-file-sizes.mjs:83-87` adds an exception entry for `SkillsView.tsx` raising the limit to 620 lines with a justification naming the centralized list/detail state, project-aware hydration, filter UX, and import/export flows. `ui/goose2/src/features/agents/ui/PersonaDetails.tsx:7` imports the new `DetailField` shared component.
+
+## Design analysis
+
+The discovery-order rewrite at `mod.rs:20-25` is the load-bearing decision and it's the right one. Putting `.agents/skills` *first* in the project-scoped list at `:21` means new skills written via the redesigned UI (which I assume targets the new canonical path) shadow any legacy `.goose/skills` entry with the same name. That's the correct migration semantic — users who copy their old skills over to `.agents/skills` get the new version, users who don't see no behavior change because `.goose/skills` is still discovered. The collision test at `:75-113` is the strongest piece of the diff: it proves the precedence contract end-to-end through the public `list_sources` API plus `export_source`, not just through the directory-walker function in isolation. That means a future refactor that accidentally swaps `.agents/skills` and `.goose/skills` in the search order will fail this test, not just a unit-level walker test that someone might forget to update.
+
+The `project_skills_dir` *write* path at `:11-13` correctly returns the new canonical path, so any UI flow that calls `project_skills_dir(...)` to compute "where do I write a new skill?" will write to `.agents/skills`. Combined with the discovery-order change, this means the system is internally consistent: writes go to `.agents/skills`, reads prefer `.agents/skills`, no skill is silently shadowed by an older copy of itself.
+
+## Concerns / nits
+
+1. **No migration story for legacy `.goose/skills` users** — the diff doesn't move existing `.goose/skills` content, doesn't print a deprecation warning when reading from `.goose/skills`, and doesn't document the migration in the PR description visible portion. Users with skills in `.goose/skills` will continue to see them work (because `.goose/skills` is still in the search order), but won't know the canonical path has moved. A startup-time deprecation log when `.goose/skills` is non-empty AND `.agents/skills` is empty (i.e., "you have legacy skills, please move them") would close the doc gap. Not blocking but worth mentioning.
+
+2. **`SkillsView.tsx` file-size exception at 620 lines** is a code-smell escape hatch — the justification ("centralizes list/detail state, project-aware skill hydration, category/source filtering, import/export flows, and detail-page action wiring pending a later decomposition") names five distinct concerns in one component. The "pending a later decomposition" hedge suggests the author knows this — if there's an existing tracking issue for the decomposition, the justification should link it; if not, one should be filed. Not blocking but the file-size exception list is meant to be an exit ramp, not a parking lot.
+
+3. **`PersonaDetails.tsx` import of `DetailField`** at `:7` is one of presumably many UI changes not visible in the lines 1-200 window — out-of-scope for this review but worth noting that the PR appears to bundle the skills-library redesign with broader shared-component refactors. A reviewer should verify the PR description names this scope explicitly.
+
+4. **The legacy-search-order test name `project_sources_prefer_agents_directory_over_legacy_goose` is good** — it documents the intent inline. But the test only checks the case where both directories have the *same skill name*. A symmetric test for "skills in both directories with different names" (which should both appear in the listing) would close the only remaining ambiguity in the precedence contract: does `.goose/skills` get *suppressed* when `.agents/skills` exists, or merely *shadowed on name collision*? The diff implements the latter (correct), and a test that proves it would be reviewer-friendly.
+
+## Risks
+
+Medium. The discovery-order change is strictly additive for the read path (`.agents/skills` is *added* to the search order, not substituted for `.goose/skills`), so no user loses access to existing skills. The write path *does* change — any future skill-write flow that calls `project_skills_dir` will write to the new location. If there's any UI flow that hard-codes `.goose/skills` for skill-writing rather than going through `project_skills_dir`, this PR would split writes between two directories silently. The diff visible window doesn't cover the UI write side, so this is a verification step for the maintainer.
+
+## Suggestions
+
+Land after: (a) adding a startup-time deprecation log for non-empty `.goose/skills` directories (or at minimum a one-line CHANGELOG note about the canonical path move), (b) filing or linking a tracking issue for the `SkillsView.tsx` decomposition the file-size justification references, and (c) verifying no UI write path hard-codes `.goose/skills`.
+
+## What I learned
+
+The "introduce new canonical path, search-order-prefer it, leave legacy in the chain" migration shape is the right pattern for any directory-convention rename — strictly additive on reads, write-redirected on writes, no user-data motion required. The collision-precedence test at `:75-113` is the highest-value test in the diff because it pins the *precedence semantic*, not just the *path-walking mechanic* — that's the pattern to copy for any future "search order matters" refactor.