Skip to content

Commit 1d4e978

Browse files
dougborgclaude
andcommitted
docs+chore: address third Copilot review on #716 + fix misleading regen comment
Three valid fixes — two from Copilot's third review pass on the First-Push Safety rewording, plus the script-comment cleanup that was deferred from the previous round. ## COMMIT_STANDARDS.md "First-Push Safety" example labels The previous fix updated the rationale to acknowledge that `git push -u origin <branch-name>` is safe by itself, but the example block still labeled that form "Risky" — contradicting the rationale above it. Reworked the example block to show three distinct cases: DANGEROUS (under-specified pushes that genuinely resolve to the tracked upstream), DISCOURAGED (the bare-branch-name form — safe today but relies on push.default config staying consistent forever), and REQUIRED (the explicit `HEAD:refs/heads/<name>` form). ## COMMIT_STANDARDS.md and CONTRIBUTING.md hook claim Both files claimed "a pre-push hook enforces this" / "enforces the explicit form". Read the actual hook (scripts/pre-push-guard.sh): it rejects pushes that land any non-`main` local ref on `refs/heads/main`. It does NOT reject the bare-branch-name push form (which targets `refs/heads/<name>`, not main). Reworded both files to describe what the hook actually does — block the specific originating-incident path — and explain that the explicit refspec is the contributor-facing rule, not a hook-enforced constraint. ## scripts/regenerate_client.py inline comment The comment at line 347 said "Update main __init__.py with flattened imports (preserve any custom content)" but the code immediately overwrites the file via `.write_text(init_content)` with no merge step. The comment misled the reader (and tripped the Copilot review on this PR's first round). Rewrote the comment to match actual behavior: overwrite verbatim, no merge, with a pointer for where to add custom re-exports instead. Refs #569. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent a4669e8 commit 1d4e978

3 files changed

Lines changed: 25 additions & 8 deletions

File tree

.github/agents/guides/shared/COMMIT_STANDARDS.md

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -314,16 +314,27 @@ muscle-memory bare `git push` is all it takes.
314314
**Always use the fully explicit destination ref for first-time pushes:**
315315

316316
```bash
317-
# Risky — depends on push.default, branch tracking, and contributor habits
317+
# DANGEROUS — under-specified pushes resolve to the tracked upstream (`main`)
318+
git push # no remote, no refspec
319+
git push -u origin # no refspec
320+
git push origin HEAD # depends on push.default
321+
322+
# DISCOURAGED — safe today, but relies on push.default staying `simple` /
323+
# `current` and on the branch name being spelled correctly. One stray
324+
# `git config --global push.default upstream` defeats it.
318325
git push -u origin chore/foo
319326

320-
# Safe — names both source (HEAD) and destination ref explicitly
327+
# REQUIRED — names both source (HEAD) and destination ref explicitly.
328+
# Immune to push.default, branch-tracking config, and bare-push habits.
321329
git push -u origin HEAD:refs/heads/chore/foo
322330
```
323331

324332
This actually happened: commit `30f3fd86` reached main + tagged `mcp-v0.44.1` +
325-
published to PyPI before the pipeline could be cancelled. A `pre-push` hook now enforces
326-
the explicit form on every contributor's machine; **do not bypass with `--no-verify`**.
333+
published to PyPI before the pipeline could be cancelled. A `pre-push` hook at
334+
`scripts/pre-push-guard.sh` now blocks any push that lands a non-`main` local ref on
335+
`refs/heads/main`, so the exact incident can't recur — but the hook only catches pushes
336+
that actually target main, *not* pushes that omit the explicit refspec. **Always use the
337+
explicit form, and do not bypass the hook with `--no-verify`.**
327338

328339
## Multi-Package Changes
329340

docs/CONTRIBUTING.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,12 @@ All formatting is automated via `uv run poe format`.
133133
destination ref explicitly, so it's immune to git configs (e.g.
134134
`push.default = upstream`) that can reroute an under-specified push to whatever the
135135
branch tracks — which, if you created the branch via
136-
`git checkout -b <name> origin/main`, is `main`. A pre-push hook enforces the
137-
explicit form; never bypass with `--no-verify`. Full rationale + the incident that
138-
prompted the rule live in
136+
`git checkout -b <name> origin/main`, is `main`. A pre-push hook
137+
(`scripts/pre-push-guard.sh`) blocks the specific case where a non-`main` local ref
138+
would land on `refs/heads/main`, so the originating incident can't recur — but the
139+
hook doesn't reject every under-specified push, so the explicit refspec is the
140+
contributor-facing rule. Never bypass with `--no-verify`. Full rationale + the
141+
incident that prompted the rule live in
139142
[COMMIT_STANDARDS "First-Push Safety"](../.github/agents/guides/shared/COMMIT_STANDARDS.md#first-push-safety).
140143

141144
### Commit Message Format

scripts/regenerate_client.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,10 @@ def move_client_to_workspace(workspace_path: Path) -> bool:
344344
shutil.copy2(source_item, target_item)
345345
print(f" 📄 Updated file: {item_name}")
346346

347-
# Update main __init__.py with flattened imports (preserve any custom content)
347+
# Overwrite main __init__.py with the flattened-import template below.
348+
# Note: the file is rewritten verbatim, not merged — any hand-added exports
349+
# will be lost on regen. Add custom re-exports to `katana_client.py` or to a
350+
# new editable module instead.
348351
main_init = target_client_path / "__init__.py"
349352
init_content = '''"""Katana Public API Client - Python client for Katana Manufacturing ERP."""
350353

0 commit comments

Comments
 (0)