mirror of
https://github.com/daymade/claude-code-skills.git
synced 2026-09-14 16:15:21 +08:00
feat(github-contributor): add push-time gotchas and PR #1624 case study
- Add Step 3.4 push-time verification checklist to SKILL.md - Add references/push_time_gotchas.md - Extend PR #2634 case study with merge-ready maintenance arc - Add PR #1624 case study (frontend rebase, async init guards, test coupling) - Bump marketplace version to 1.2.0 and refresh description Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
daymade
@@ -364,10 +364,10 @@
|
||||
},
|
||||
{
|
||||
"name": "github-contributor",
|
||||
"description": "Strategic guide for becoming an effective GitHub contributor. Covers opportunity discovery, project selection, high-quality PR creation, and reputation building. Use when looking to contribute to open-source projects, building GitHub presence, or learning contribution best practices",
|
||||
"description": "End-to-end playbook for shipping high-quality pull requests to open-source projects you don't maintain — discovery, CONTRIBUTING compliance, PR-size check, minimal-diff implementation, PR description with AI-assisted disclosure, conflict resolution, and post-submission maintainer interaction. Use whenever creating, editing, or pushing a PR to a third-party GitHub repo — submit a PR, open a PR, fix this upstream, rebase against main, respond to the bot review, an owner/repo target, or 提 PR / 上游 PR / 贡献代码 / rebase 冲突 / 回应维护者.",
|
||||
"source": "./github-contributor",
|
||||
"strict": false,
|
||||
"version": "1.1.0",
|
||||
"version": "1.2.0",
|
||||
"category": "developer-tools",
|
||||
"keywords": [
|
||||
"github",
|
||||
@@ -376,7 +376,12 @@
|
||||
"pull-request",
|
||||
"reputation",
|
||||
"contributor",
|
||||
"oss"
|
||||
"oss",
|
||||
"rebase",
|
||||
"conflict-resolution",
|
||||
"pr-description",
|
||||
"ai-assisted-disclosure",
|
||||
"bot-review"
|
||||
]
|
||||
},
|
||||
{
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
Security scan passed
|
||||
Scanned at: 2026-05-17T16:03:14.036474
|
||||
Scanned at: 2026-06-23T18:33:14.138886
|
||||
Tool: gitleaks + pattern-based validation
|
||||
Content hash: 1c10f77d562155b1c1cbda8e3ff066c1652d6afb9afe1f4908ac2811203d5879
|
||||
Content hash: 932f003ea20cb7d1255b9f3791721c813cb8457adf6aeb0715d36201469083a6
|
||||
|
||||
@@ -110,6 +110,23 @@ Before writing the PR description, list every "I tested…" / "I verified…" /
|
||||
|
||||
This rule prevents the most damaging trust failure: a maintainer running your "tested" command and finding it doesn't work.
|
||||
|
||||
### Step 3.4 — Push-time verification
|
||||
|
||||
Local tests passing is not the finish line. Before you call the PR merge-ready, run the push-time checklist:
|
||||
|
||||
1. **Visibility check** — confirm the target repo is actually public/private as you assume:
|
||||
```bash
|
||||
gh repo view <owner>/<repo> --json visibility,isPrivate,defaultBranchRef
|
||||
```
|
||||
2. **Security hooks** — if pre-push fails, fix the rule or the content; do not `--no-verify`.
|
||||
3. **Push succeeds** — if it fails with 503/auth errors, check `git config --global --get-regexp url` for stale URL rewrites.
|
||||
4. **Mergeability check** — `git push` succeeding does not mean GitHub can merge:
|
||||
```bash
|
||||
gh pr view <pr-number> --repo <owner>/<repo> --json mergeable,mergeStateStatus
|
||||
```
|
||||
|
||||
Full details (URL rewrites, PII-hook false positives, `--force-with-lease` caveats) are in [`references/push_time_gotchas.md`](references/push_time_gotchas.md).
|
||||
|
||||
## Phase 4 — PR Description Writing
|
||||
|
||||
A great PR description does three jobs: (1) lets the maintainer decide in 30 seconds whether to merge, (2) gives reviewers everything they need to verify without DM'ing you, (3) creates a written record that survives team turnover.
|
||||
@@ -240,7 +257,9 @@ The point of counter-review is to surface things you didn't think of, not to man
|
||||
| [`references/phase3_quality_gates_and_e2e.md`](references/phase3_quality_gates_and_e2e.md) | Isolated-home pattern, single-instance forward, SQLite verification, screencapture + window focus |
|
||||
| [`references/phase4_pr_description.md`](references/phase4_pr_description.md) | Body skeleton, test-coverage-matrix, AI disclosure templates, screenshot placeholder pattern |
|
||||
| [`references/phase5_post_submission.md`](references/phase5_post_submission.md) | `gh api in_reply_to` recipe, `--force-with-lease` semantics, counter-review filtering |
|
||||
| [`references/push_time_gotchas.md`](references/push_time_gotchas.md) | Git remote URL rewrites, PII-hook false positives, mergeability verification, force-push caveats |
|
||||
| [`references/case_study_cc-switch_pr_2634.md`](references/case_study_cc-switch_pr_2634.md) | Full real-world walkthrough including dev log, SQLite dump, screenshots |
|
||||
| [`references/case_study_cc-switch_pr_1624.md`](references/case_study_cc-switch_pr_1624.md) | Frontend/state-management rebase case study — async init guards, test coupling, i18n conflicts |
|
||||
| [`references/pr_checklist.md`](references/pr_checklist.md) | Original consolidated checklist (legacy; phase docs supersede the workflow sections) |
|
||||
| [`references/project_evaluation.md`](references/project_evaluation.md) | Project health rubric for the discovery step |
|
||||
| [`references/communication_templates.md`](references/communication_templates.md) | Issue-claim, review-response, and after-merge templates |
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
# Case Study: cc-switch PR #1624 (protect conversation history)
|
||||
|
||||
A companion to the PR #2634 case study. This PR adds a "keep conversation history" setting to `farion1231/cc-switch` that, when enabled, applies transcript-protection settings in Claude's `settings_config.json`. Like PR #2634, it required a long rebase to stay merge-ready; unlike #2634, the hardest problems were in frontend state management and test coupling rather than in Rust parsing.
|
||||
|
||||
PR URL: https://github.com/farion1231/cc-switch/pull/1624
|
||||
|
||||
## Phase 1 — Scope and baseline
|
||||
|
||||
The project's `CONTRIBUTING.md` AI-Assisted clause applied (same five rules as PR #2634). The PR-size baseline was already known from PR #2634; this change was intentionally smaller (+~300/-~50 lines), mostly TypeScript and two small Rust commands.
|
||||
|
||||
## Phase 2 — Implementation notes
|
||||
|
||||
### Scope contract
|
||||
|
||||
> Goal: add a UI toggle that protects or unprotects Claude conversation history by writing/removing `cleanupPeriod: 99999` in `~/.claude/settings.json`.
|
||||
>
|
||||
> In scope: the settings toggle, Tauri commands to read/apply/clear protection, frontend state sync, i18n strings.
|
||||
>
|
||||
> Explicitly out of scope: changing the default cleanup period for new users; migrating existing history; UI beyond the settings dialog.
|
||||
|
||||
### Two-language split
|
||||
|
||||
The feature touches both Rust backend commands and React frontend state:
|
||||
|
||||
- Rust (`src-tauri/src/claude_mcp.rs`, `src-tauri/src/commands/plugin.rs`) adds `get_cleanup_period_days`, `set_cleanup_period_days`, `clear_cleanup_period_days` and wraps them as Tauri invoke handlers.
|
||||
- Frontend (`src/hooks/useSettingsForm.ts`, `src/hooks/useSettings.ts`) adds the toggle and syncs it with the actual persisted `settings.json` state.
|
||||
|
||||
This split is common in Tauri/Electron apps. The merge gate is that **both sides must stay consistent**: if the UI shows "protected" but `settings.json` says otherwise, the user will lose trust in the toggle.
|
||||
|
||||
## Phase 3 — Quality gates and frontend-specific failures
|
||||
|
||||
### Clippy
|
||||
|
||||
Two `unnecessary_map_or` warnings appeared after the rebase:
|
||||
|
||||
```rust
|
||||
// Before
|
||||
.map_or(false, |d| d == 99999)
|
||||
.map_or(false, |days| days > 365)
|
||||
|
||||
// After
|
||||
== Some(99999)
|
||||
.is_some_and(|days| days > 365)
|
||||
```
|
||||
|
||||
These are small but worth fixing because maintainers notice red CI more than they notice code style.
|
||||
|
||||
### React race condition: async init vs. user toggle
|
||||
|
||||
`useSettingsForm` loads server settings via React Query, then asynchronously reads the real transcript-protection state from `~/.claude/settings.json`. If the user toggles the switch before that async read returns, the async result must not overwrite the user's explicit choice.
|
||||
|
||||
The fix uses two refs:
|
||||
|
||||
1. `hasSyncedTranscriptProtectionRef` — ensures the async read happens only once (on initial data load), not on every refetch.
|
||||
2. `userTouchedTranscriptRef` — if the user has manually changed the toggle, the async result is ignored.
|
||||
|
||||
This pattern generalizes: **any async initialization that can return after user interaction needs a "user touched" guard**.
|
||||
|
||||
### Test coupling after refactoring
|
||||
|
||||
`useSettings.ts` was refactored to extract a shared `syncTranscriptProtection` helper used by both auto-save and explicit save. After the refactor, three SettingsDialog tests failed because the helper called `settingsApi.applyTranscriptProtection()` / `clearTranscriptProtection()` on every save, but the tests only mocked `settingsApi.save()`.
|
||||
|
||||
The fix was not to add more mocks. It was to make the helper compare the new value against the **last known persisted value** and only call the protection API when the value actually changed:
|
||||
|
||||
```ts
|
||||
const baseline = lastSyncedKeepConversationHistoryRef.current ?? persistedValue;
|
||||
if (baseline === nextValue) return;
|
||||
```
|
||||
|
||||
This made the tests pass because most test cases did not change the toggle, so the API was never invoked. More importantly, it made the production behavior correct: no spurious writes to `settings.json`.
|
||||
|
||||
**Lesson**: when a refactor breaks tests, first ask whether the refactor introduced unnecessary side effects. Fixing the side effect is usually better than adding mocks.
|
||||
|
||||
### i18n conflict resolution
|
||||
|
||||
Upstream had added a new "Codex Auth" section to the locale files while the PR added a "keepConversationHistory" section. The rebase conflict was resolved by keeping **both** blocks in the correct order. This is the standard resolution for unrelated additions to the same file: preserve both, in the order the file uses.
|
||||
|
||||
## Phase 5 — Post-submission maintenance
|
||||
|
||||
The PR sat long enough that upstream `main` advanced. The rebase conflicts were in:
|
||||
|
||||
- `src/hooks/useSettingsForm.ts` — upstream added new Codex settings that had to coexist with `keepConversationHistory`.
|
||||
- `src/i18n/locales/*.json` — upstream added the Codex Auth section.
|
||||
- `src-tauri/src/deeplink/provider.rs` — upstream refactored Claude env extraction; the transcript-protection PR did not touch this, but a parallel PR (#2634) did, so the combined working tree needed both to compile.
|
||||
|
||||
After resolving, the full suite was re-run:
|
||||
|
||||
```bash
|
||||
pnpm typecheck && pnpm test:unit
|
||||
cargo clippy --all-targets && cargo test
|
||||
```
|
||||
|
||||
## Lessons that became skill rules
|
||||
|
||||
1. **Async initialization needs a "user touched" guard** to prevent late-arriving state from overwriting user input.
|
||||
2. **Refactor-induced test failures often signal a real bug** — prefer removing the spurious side effect over adding mocks.
|
||||
3. **i18n conflicts from unrelated additions** resolve by keeping both blocks in file order.
|
||||
4. **Re-run the full suite after every rebase**, even when the conflicts are "just" in TypeScript locale files.
|
||||
5. **Multi-language PRs** must keep frontend state and backend persisted state consistent; the bug usually appears at the boundary.
|
||||
@@ -211,3 +211,86 @@ Replying as a comment (not in PR body) means the resolution appears next to the
|
||||
8. **`code-simplifier` cleanups should be a separate commit** (or a separate PR) — bundling them into a fix commit makes review harder and risks scope creep.
|
||||
9. **`--force-with-lease`, never plain `--force`** — review threads are too easy to destroy.
|
||||
10. **Self-audit "what's my evidence?" pass** before publishing the PR body catches fabrication-by-default.
|
||||
|
||||
## Phase 6 — Keeping the PR merge-ready over time
|
||||
|
||||
After the initial push, upstream `main` continued to move. The PR sat open long enough that a second rebase was required. This phase is where many PRs quietly rot; the work below is what kept it in a merge-ready state.
|
||||
|
||||
### Rebase against latest upstream `main`
|
||||
|
||||
```bash
|
||||
git fetch origin
|
||||
git rebase origin/main
|
||||
```
|
||||
|
||||
Two new conflicts appeared:
|
||||
|
||||
- **`src-tauri/src/deeplink/provider.rs`** — upstream had extracted a helper `extract_claude_config_env(request)` in the same region where the PR added `build_claude_settings(request, extra_env)`. The resolution kept both: use upstream's extraction for the standard fields, then layer the PR's `extra_env` merge on top.
|
||||
- **`src-tauri/src/services/provider/usage.rs`** — upstream had refactored credential resolution into `resolve_script_credentials`. The PR's older `extract_api_key_from_provider` changes were obsolete; dropping them was the correct resolution.
|
||||
|
||||
After resolving, two upstream test initializers lacked the new `extra_env` field. The fix was minimal:
|
||||
|
||||
```rust
|
||||
DeepLinkImportRequest {
|
||||
// ... existing fields ...
|
||||
extra_env: None,
|
||||
}
|
||||
```
|
||||
|
||||
**Lesson**: when rebasing, prefer the upstream's architectural change and re-apply your feature as a thin layer on top. Do not re-litigate upstream refactors inside your PR.
|
||||
|
||||
### Quality gates re-run
|
||||
|
||||
Every rebase invalidates your previous "tests pass" claim. Re-run the full suite:
|
||||
|
||||
```bash
|
||||
pnpm typecheck
|
||||
pnpm test:unit
|
||||
cargo clippy --all-targets
|
||||
cargo test
|
||||
```
|
||||
|
||||
This surfaced two clippy warnings in unrelated transcript-protection code that had been added in a parallel PR:
|
||||
|
||||
- `.map_or(false, |d| d == 99999)` → `== Some(99999)`
|
||||
- `.map_or(false, |days| days > 365)` → `.is_some_and(|days| days > 365)`
|
||||
|
||||
Both were fixed before push. The key point is not the specific warnings but the rule: **a rebase means re-running every gate**.
|
||||
|
||||
### Push-time failures
|
||||
|
||||
#### 1. Gitconfig URL rewrite (`503` on push)
|
||||
|
||||
`git push fork feat/deeplink-extra-env` failed with a 503. The cause was a stale global rewrite that mapped SSH-style GitHub URLs to HTTPS:
|
||||
|
||||
```bash
|
||||
git config --global --get-regexp url
|
||||
# output showed url.https://github.com/.insteadOf=<ssh-git-url-prefix>
|
||||
```
|
||||
|
||||
Removing the rewrite restored normal SSH push.
|
||||
|
||||
#### 2. Local PII hook false positives
|
||||
|
||||
The local pre-push hook (gitleaks + bash fallback) rejected several upstream commits because they contained sample home-directory paths used as test fixture data (e.g., macOS `/Users/…` and Linux `/home/…` examples) and a public upstream maintainer email address in `README_DE.md`.
|
||||
|
||||
The correct response was **not** `--no-verify`. Instead, the allowlist in the local guard config was updated to recognize these as public fixture content. This is a one-time local config change; it was not committed to the upstream repo.
|
||||
|
||||
### Counter-review with filtering
|
||||
|
||||
A Dynamic Workflow review was run over the combined changes. It produced a list of findings. The majority were discarded using the skill's probability / cost / reality filter. Two survived:
|
||||
|
||||
1. A clippy warning in the transcript-protection code (fixed).
|
||||
2. A unit-test mocking issue caused by a new helper calling transcript-protection APIs; the fix was to pass a `persistedValue` baseline so the API is only invoked when the toggle actually changes.
|
||||
|
||||
Both fixes were amended into the tip commit and force-pushed with `--force-with-lease`.
|
||||
|
||||
### Final mergeability check
|
||||
|
||||
After push:
|
||||
|
||||
```bash
|
||||
gh pr view 2634 --repo farion1231/cc-switch --json mergeable,mergeStateStatus
|
||||
```
|
||||
|
||||
Returned `mergeable: MERGEABLE`. That is the only state that justifies calling the PR "ready to merge".
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
# Push-Time Gotchas
|
||||
|
||||
A PR can look perfect locally and still fail at the last meter. These are the failures that happen **after** the code is written and tested — when you push, when GitHub evaluates mergeability, or when your own security hooks reject the commit.
|
||||
|
||||
## Git remote URL rewrites can silently break push
|
||||
|
||||
A global `.gitconfig` URL rewrite can turn an SSH push into HTTPS, or route it through a stale endpoint, producing errors like `503` or `unable to access` even though `git remote -v` looks correct.
|
||||
|
||||
**Detection:**
|
||||
|
||||
```bash
|
||||
git config --global --get-regexp url
|
||||
```
|
||||
|
||||
**If you see a rewrite that affects this repo**, remove it for the current push or globally:
|
||||
|
||||
```bash
|
||||
git config --global --unset url."https://github.com/".insteadOf
|
||||
git config --global --unset url."https://".insteadOf
|
||||
```
|
||||
|
||||
Then verify with `git remote -v` and a test fetch:
|
||||
|
||||
```bash
|
||||
git fetch origin
|
||||
git push fork <branch>
|
||||
```
|
||||
|
||||
## Local PII / secret hooks may reject upstream commits
|
||||
|
||||
If the project uses local pre-commit/pre-push hooks (gitleaks, custom PII guards), an innocent rebase can surface test fixtures, sample paths, or documentation emails from upstream history that the hooks flag.
|
||||
|
||||
**Do not bypass with `--no-verify`.** The correct fixes are, in order:
|
||||
|
||||
1. **Add an allowlist entry** for obviously public fixture content (sample paths, upstream maintainer email in `README_DE.md`, etc.).
|
||||
2. **Tune the rule** if it is producing false positives (e.g., adding word boundaries to a phone-number regex).
|
||||
3. **Sanitize the commit** only if the flagged content is actually yours and should not be public.
|
||||
|
||||
Keep the allowlist change in a separate commit or in your local guard config — do not commit `.pii-patterns` to the upstream repo unless the project specifically supports it.
|
||||
|
||||
## Verify mergeability before declaring "ready"
|
||||
|
||||
`git push` succeeding does not mean the PR is mergeable. Always check:
|
||||
|
||||
```bash
|
||||
gh pr view <pr-number> --repo <owner>/<repo> --json mergeable,mergeStateStatus
|
||||
```
|
||||
|
||||
A `mergeable` value of `CONFLICTING` means the GitHub merge algorithm still sees a conflict even if local `git merge` appeared clean (this can happen with rename/add conflicts or protected-file checks). Fix locally and push again.
|
||||
|
||||
## Use `--force-with-lease`, but know what it does not protect
|
||||
|
||||
`--force-with-lease` aborts if the remote ref has moved since you last fetched. It does **not** protect:
|
||||
|
||||
- Review threads on lines you are about to rewrite (they will be marked outdated).
|
||||
- Bot comments that reference a specific commit hash.
|
||||
- CI runs that are in progress on the old tip.
|
||||
|
||||
So only force-push after you have addressed the feedback that caused the rewrite, and mention the rewrite in a PR comment if maintainers are mid-review.
|
||||
|
||||
## Checklist before every push
|
||||
|
||||
```bash
|
||||
# 1. Visibility and target correctness
|
||||
gh repo view <owner>/<repo> --json visibility,isPrivate,defaultBranchRef
|
||||
|
||||
# 2. Local checks (use the project's own commands)
|
||||
<project test/lint commands from CONTRIBUTING.md>
|
||||
|
||||
# 3. Mergeability after push
|
||||
gh pr view <pr-number> --repo <owner>/<repo> --json mergeable,mergeStateStatus
|
||||
```
|
||||
Reference in New Issue
Block a user