docs: fork workflow + CONTRIBUTING.md#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds contributor documentation for CTMobi’s veo-tools fork, defining how the fork tracks upstream while supporting CTMobi-specific development.
Changes:
- Adds
CONTRIBUTING.mdwith branch strategy formainandupstream-sync. - Documents upstream sync, feature branch, PR, and branch naming workflows.
- Adds a decision tree for routing changes to CTMobi or upstream.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Never push directly to `main` or `upstream-sync`**. Always go through a PR. | ||
| - **`upstream-sync` PRs**: only the sync procedure (force-push reset). No code reviews needed for the reset itself; if a sync introduces conflicts when merging into `main`, those conflicts are reviewed in the follow-up `main` PR. |
| ```bash | ||
| git checkout main | ||
| git fetch origin | ||
| git merge upstream-sync # or: git rebase upstream-sync |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md file to establish the workflow for the CTMobi/veo-tools fork, detailing branch management and synchronization with the upstream repository. Review feedback highlighted several areas for improvement, including resolving a contradiction regarding direct pushes to the upstream-sync branch, clarifying the git cherry-pick syntax, and refining the decision-making process for merging urgent fixes into the fork's main branch while simultaneously proposing them upstream.
| ```bash | ||
| git checkout main | ||
| git checkout -b feat/<short-name>-fork | ||
| git cherry-pick <commit-sha>... |
There was a problem hiding this comment.
The ellipsis ... in the command might be misinterpreted as part of the syntax or a specific git range operator. It is clearer to use a standard placeholder for the commit(s) to be picked.
| git cherry-pick <commit-sha>... | |
| git cherry-pick <commit-sha> |
| - **Never push directly to `main` or `upstream-sync`**. Always go through a PR. | ||
| - **`upstream-sync` PRs**: only the sync procedure (force-push reset). No code reviews needed for the reset itself; if a sync introduces conflicts when merging into `main`, those conflicts are reviewed in the follow-up `main` PR. |
There was a problem hiding this comment.
There is a contradiction between the rule forbidding direct pushes to upstream-sync (line 103) and the documented sync procedure (line 36) which uses a direct force-push. Additionally, the mention of "upstream-sync PRs" is confusing if the branch is a mirror updated via reset --hard. If upstream-sync is a mirror, it is standard to allow direct force-pushes for the sync process.
| - **Never push directly to `main` or `upstream-sync`**. Always go through a PR. | |
| - **`upstream-sync` PRs**: only the sync procedure (force-push reset). No code reviews needed for the reset itself; if a sync introduces conflicts when merging into `main`, those conflicts are reviewed in the follow-up `main` PR. | |
| - **Never push directly to `main`**. Always go through a PR. `upstream-sync` is an exception as it is a mirror updated via the sync procedure. | |
| - **Sync PRs**: When merging `upstream-sync` into `main`, use a PR (e.g., `sync/<date>`) if the merge is non-trivial. No code reviews are needed for the `upstream-sync` reset itself; conflicts are reviewed in the follow-up `main` PR. |
|
|
||
| ``` | ||
| Is the change something kdowswell would likely accept? | ||
| ├── Yes → base from upstream-sync, PR to kdowswell. Mirror it to CTMobi only if blocked or rejected. |
There was a problem hiding this comment.
The decision tree suggests mirroring changes to the CTMobi fork only if they are blocked or rejected by upstream. This could delay getting important fixes into the fork's main branch. It is often better to allow merging into main immediately if the change is needed for internal work while simultaneously proposing it upstream.
| ├── Yes → base from upstream-sync, PR to kdowswell. Mirror it to CTMobi only if blocked or rejected. | |
| ├── Yes → base from upstream-sync, PR to kdowswell. Also PR to CTMobi's main if the change is needed immediately. |
| └── base from main, PR to CTMobi. | ||
| ``` | ||
|
|
||
| When in doubt, base from `main` and PR to CTMobi — it's the safer default. Promoting fork-internal commits to an upstream PR is straightforward via cherry-pick from `upstream-sync`. |
There was a problem hiding this comment.
The phrase "via cherry-pick from upstream-sync" is technically incorrect for the described workflow. You would cherry-pick from a fork-internal branch onto a new branch based on upstream-sync to prepare an upstream PR.
| When in doubt, base from `main` and PR to CTMobi — it's the safer default. Promoting fork-internal commits to an upstream PR is straightforward via cherry-pick from `upstream-sync`. | |
| When in doubt, base from `main` and PR to CTMobi — it's the safer default. Promoting fork-internal commits to an upstream PR is straightforward: create a branch from `upstream-sync` and cherry-pick the relevant commits onto it. |
Resolved 7 findings from Copilot (5) and gemini-code-assist (2).
Architectural:
- registerRule() global registry replaced with createValidator()
factory pattern. FOUNDATION_RULES exported as array; each skill
creates its own validator with baseRules + extraRules. Avoids
module-level singleton state, cross-skill leakage, test-order
dependence. Foundation owns FOUNDATION_RULES + createValidator;
sub-projects ship their own rule arrays.
- New rule #9: outputPath required when storageUri is unset.
Catches missing output destination at validation time instead
of failing late in the download phase.
Cleanup:
- VeoConfig.durationSeconds comment updated to reflect model-aware
validation (Veo 3.x → {4,6,8}; Veo 2 → {5,6,7,8}), not a fixed set
- New Open Question #4: Veo 2 allowed durations verification
(rule #1 referenced it but the OQ didn't exist; previously cross-
referenced sampleCount's OQ by mistake)
- Open Question #1 (Default model ID) moved to new "Resolved
decisions" subsection; Open Questions list renumbered 1-4
- tsconfig-paths moved from devDependencies to dependencies (used
at runtime by scripts via -r tsconfig-paths/register)
- negativePrompt "no/don't" guidance moved out of "Valid values"
column into descriptive Notes, since it's non-enforceable
Cross-reference renumbering propagated:
- Rule #1: Open Question #5 → #4 (Veo 2 durations)
- Rule #5: Open Question #3 → #2 (token counting heuristic)
- Rule #6: Open Question #2 → #1 (region detection)
- Rule #7: Open Question #4 → #3 (sampleCount)
- Resolved decisions: Open Question #4 → #2 (token counting)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bot review applied (commit
|
| Source | Finding | Resolution |
|---|---|---|
| Copilot + gemini | "Never push directly to main or upstream-sync" contradicts the sync procedure (which force-pushes to upstream-sync) |
Rule rewritten: upstream-sync is exempt because it's a mirror, not a working branch. No PR / no review for the verbatim upstream replication. Added "main sync PRs" guidance for the merge into main. |
| Copilot | git merge upstream-sync after git fetch origin uses a possibly-stale local branch |
Sync procedure updated: pull upstream-sync first to fast-forward the local copy. Alternative noted: merge origin/upstream-sync directly. |
| gemini #5 | Decision tree's "Mirror to CTMobi only if blocked or rejected" can delay urgent fixes | Tree expanded to allow parallel PRs (upstream + fork) when a change is needed urgently in the fork. Avoids blocking internal work on upstream's review cadence. |
Cleanup (2)
| Source | Finding | Resolution |
|---|---|---|
| gemini #3 | git cherry-pick <commit-sha>... — ellipsis could be parsed as git range |
Replaced with single <commit-sha> placeholder + comment that multiple SHAs are space-separated |
| gemini #6 | Phrase "via cherry-pick from upstream-sync" was technically inverted |
Reworded to describe the actual flow: create branch from upstream-sync and cherry-pick fork-internal commits onto it |
Diff summary
b5faf53: 24 insertions, 10 deletions. All contradictions internal to the doc resolved; sync procedure now safe against stale local branches; decision tree handles the urgent-fix case.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md file that outlines the branching strategy and contribution workflows for the CTMobi fork of veo-tools. The documentation covers branch synchronization, feature development, and decision-making for upstream contributions. Feedback focused on making the git command examples more robust; specifically, using git checkout -B instead of git pull for the upstream-sync mirror branch to handle remote history rewrites. Additionally, a correction was noted for a summarized sync command to include a necessary checkout step, preventing accidental destructive resets on the main branch.
| git checkout upstream-sync | ||
| git pull --ff-only origin upstream-sync # ensure local upstream-sync matches the latest refresh on origin |
There was a problem hiding this comment.
Since upstream-sync is a mirror that is updated via force-push on the remote (as described in the sync procedure), a standard git pull --ff-only will fail for other contributors if the history has been rewritten. Using git checkout -B with the remote-tracking branch is a more robust way to synchronize the local mirror to match the remote state.
| git checkout upstream-sync | |
| git pull --ff-only origin upstream-sync # ensure local upstream-sync matches the latest refresh on origin | |
| git checkout -B upstream-sync origin/upstream-sync |
| git checkout upstream-sync | ||
| git pull origin upstream-sync # ensure latest sync |
There was a problem hiding this comment.
Similar to the previous section, git pull will fail if the upstream-sync branch was force-pushed on the remote. It is safer to fetch and then reset the local branch to match the remote state exactly to ensure a clean base for upstream PRs.
| git checkout upstream-sync | |
| git pull origin upstream-sync # ensure latest sync | |
| git fetch origin | |
| git checkout -B upstream-sync origin/upstream-sync |
| ## Pull request rules | ||
|
|
||
| - **Never push directly to `main`**. Always go through a PR. | ||
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git reset --hard upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. |
There was a problem hiding this comment.
The summarized sync command sequence is missing a git checkout upstream-sync step. If a user runs git reset --hard upstream/main while on the main branch, they will lose all their local customizations and overwrite them with the upstream state.
Suggested sequence:
git fetch upstream && git checkout upstream-sync && git reset --hard upstream/main && git push --force-with-lease origin upstream-sync
Second-round bot review applied (commit
|
| # | Finding | Resolution |
|---|---|---|
| 1 | Sync procedure TL;DR was missing git checkout upstream-sync — running git reset --hard upstream/main while on main would silently overwrite main |
Added explicit git checkout upstream-sync step with bold inline comment warning about the consequence of skipping it |
| 2 | git pull --ff-only upstream-sync fails after a force-push (history rewrite breaks fast-forward) |
Replaced with git checkout -B upstream-sync origin/upstream-sync — unconditionally aligns local branch to remote state. Correct semantics for a mirror branch. |
| 3 | Same problem in the "Feature branches from upstream-sync" section | Same fix applied with reference back to the rationale |
Diff summary
b1d1c23: 8 insertions, 9 deletions. The doc now describes a sync workflow that works correctly for all contributors regardless of when they last fetched.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md guide for the CTMobi fork of veo-tools, detailing the branching strategy and workflows for syncing with upstream and managing internal features. The reviewer provided several actionable suggestions to improve the safety and consistency of the documented Git commands. Key feedback includes replacing risky git reset sequences with atomic git checkout -B commands to prevent accidental data loss and ensuring that the workflow for merging upstream changes into main correctly follows the project's pull request policy instead of suggesting direct pushes.
| git checkout upstream-sync # IMPORTANT: switch to upstream-sync before reset | ||
| git reset --hard upstream/main |
There was a problem hiding this comment.
Using git checkout followed by git reset --hard is risky. If the checkout fails (e.g., if the branch doesn't exist locally), the reset will execute on the current branch (likely main), potentially leading to data loss. Using git checkout -B is a safer, atomic way to create or reset the local branch to the desired upstream state regardless of the current branch. The warning in line 30 should also be updated to reflect this change.
| git checkout upstream-sync # IMPORTANT: switch to upstream-sync before reset | |
| git reset --hard upstream/main | |
| git checkout -B upstream-sync upstream/main |
| | Branch | Purpose | Modification rules | | ||
| |---|---|---| | ||
| | `main` | Default branch of the fork. Reflects CTMobi's evolved state: includes upstream changes that have been synced **plus** customizations not in upstream (yet or ever). | Only via PR. Default base for fork-internal feature branches. | | ||
| | `upstream-sync` | Clean mirror of `kdowswell/veo-tools:main`. Never modified by hand. | Updated only via the sync procedure below (`git reset --hard upstream/main` + force push). | |
There was a problem hiding this comment.
Update the description of the sync procedure to match the safer git checkout -B command suggested in the workflows section.
| | `upstream-sync` | Clean mirror of `kdowswell/veo-tools:main`. Never modified by hand. | Updated only via the sync procedure below (`git reset --hard upstream/main` + force push). | | |
| | `upstream-sync` | Clean mirror of `kdowswell/veo-tools:main`. Never modified by hand. | Updated only via the sync procedure below (`git checkout -B upstream-sync upstream/main` + force push). | |
| git checkout main | ||
| git pull --ff-only origin main | ||
| git merge upstream-sync # or: git rebase upstream-sync | ||
| # resolve conflicts if customizations clash with upstream changes | ||
| git push origin main |
There was a problem hiding this comment.
This workflow snippet suggests pushing directly to main, which contradicts the 'Never push directly to main' rule (line 107). Since merging upstream-sync into main typically involves merge commits or conflict resolution, it should be done via a PR to maintain visibility and history as per line 109.
| git checkout main | |
| git pull --ff-only origin main | |
| git merge upstream-sync # or: git rebase upstream-sync | |
| # resolve conflicts if customizations clash with upstream changes | |
| git push origin main | |
| git checkout main | |
| git pull --ff-only origin main | |
| git checkout -b sync/$(date +%Y-%m-%d) | |
| git merge upstream-sync # or: git rebase upstream-sync | |
| # resolve conflicts if customizations clash with upstream changes | |
| git push -u origin sync/$(date +%Y-%m-%d) | |
| gh pr create --repo CTMobi/veo-tools --base main --fill |
| ## Pull request rules | ||
|
|
||
| - **Never push directly to `main`**. Always go through a PR. | ||
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git reset --hard upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. |
There was a problem hiding this comment.
Update the command summary to match the safer git checkout -B procedure.
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git reset --hard upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. | |
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git checkout -B upstream-sync upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. |
Third-round bot review applied (commit
|
| Source | Severity | Finding | Resolution |
|---|---|---|---|
| gemini #1 | High | git checkout upstream-sync + git reset --hard upstream/main is unsafe — if the checkout fails (branch missing locally), the reset executes on the current branch, potentially overwriting main |
Switched to atomic git checkout -B upstream-sync upstream/main — creates-or-rewrites in a single op regardless of starting branch. Rationale documented inline. |
| gemini #2 | Medium | Repo structure table still describes the old git reset --hard pattern |
Updated to reflect the new git checkout -B sync command |
| gemini #4 | Medium | Pull request rules inline-command summary still uses old pattern | Updated to the new -B form |
Sync via PR (1)
| Source | Severity | Finding | Resolution |
|---|---|---|---|
| gemini #3 | Medium | "Bringing upstream changes into main" instructed direct git push origin main, contradicting the "Never push directly to main" rule |
Workflow rewritten to use a sync/<date> PR branch. gh pr create --base main --fill is the documented default. Direct fast-forward by maintainer kept as the explicit exception when there's literally nothing to review. |
Diff summary
6b9b37b: 11 insertions, 10 deletions. All commands now use the safe atomic -B pattern. The sync flow now matches the "always PR" rule. Document is internally consistent for the first time across both branches' rules.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md file that defines the branching strategy, synchronization workflows, and contribution guidelines for the CTMobi fork of veo-tools. The reviewer identified several opportunities to improve the documentation's consistency and robustness. Specifically, feedback was provided to align the branch modification rules with the documented exceptions for trivial upstream syncs and to use shell variables in command examples to avoid potential date mismatches during branch operations.
|
|
||
| | Branch | Purpose | Modification rules | | ||
| |---|---|---| | ||
| | `main` | Default branch of the fork. Reflects CTMobi's evolved state: includes upstream changes that have been synced **plus** customizations not in upstream (yet or ever). | Only via PR. Default base for fork-internal feature branches. | |
There was a problem hiding this comment.
The modification rules for the main branch should reflect the exception for trivial syncs mentioned later in the document (lines 56 and 110) to ensure the documentation is consistent throughout.
| | `main` | Default branch of the fork. Reflects CTMobi's evolved state: includes upstream changes that have been synced **plus** customizations not in upstream (yet or ever). | Only via PR. Default base for fork-internal feature branches. | | |
| | `main` | Default branch of the fork. Reflects CTMobi's evolved state: includes upstream changes that have been synced **plus** customizations not in upstream (yet or ever). | Only via PR (except for trivial upstream syncs). Default base for fork-internal feature branches. | |
| git checkout -b sync/$(date +%Y-%m-%d) # PR head branch | ||
| git merge upstream-sync # or: git rebase upstream-sync | ||
| # resolve conflicts if customizations clash with upstream changes | ||
| git push -u origin sync/$(date +%Y-%m-%d) |
There was a problem hiding this comment.
Using $(date +%Y-%m-%d) twice in the same sequence of commands can lead to a mismatch if the date changes (e.g., at midnight) between the branch creation and the push. Capturing the date in a variable makes the workflow more robust.
| git checkout -b sync/$(date +%Y-%m-%d) # PR head branch | |
| git merge upstream-sync # or: git rebase upstream-sync | |
| # resolve conflicts if customizations clash with upstream changes | |
| git push -u origin sync/$(date +%Y-%m-%d) | |
| SYNC_DATE=$(date +%Y-%m-%d) | |
| git checkout -b sync/$SYNC_DATE # PR head branch | |
| git merge upstream-sync # or: git rebase upstream-sync | |
| # resolve conflicts if customizations clash with upstream changes | |
| git push -u origin sync/$SYNC_DATE |
| - **Never push directly to `main`**. Always go through a PR. | ||
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git checkout -B upstream-sync upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. | ||
| - **`main` sync PRs**: when `upstream-sync` is merged into `main`, do it via a PR (e.g., `sync/<date>` branch) — especially if the merge has conflicts that resolved against customizations. Trivial fast-forward merges may be done directly by a maintainer, but a PR is preferred for visibility. |
There was a problem hiding this comment.
There is a contradiction between the rule "Never push directly to main" and the exception allowed for maintainers during trivial syncs. Clarifying this in the rules section prevents confusion for contributors regarding when direct pushes are acceptable.
| - **Never push directly to `main`**. Always go through a PR. | |
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git checkout -B upstream-sync upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. | |
| - **`main` sync PRs**: when `upstream-sync` is merged into `main`, do it via a PR (e.g., `sync/<date>` branch) — especially if the merge has conflicts that resolved against customizations. Trivial fast-forward merges may be done directly by a maintainer, but a PR is preferred for visibility. | |
| - **Never push directly to `main`** for feature work. Always go through a PR. | |
| - **`upstream-sync` is exempt from the PR rule** because it's a mirror, not a working branch. It is refreshed via the documented sync procedure (`git fetch upstream && git checkout -B upstream-sync upstream/main && git push --force-with-lease origin upstream-sync`). No PR, no code review for the reset itself — there's nothing reviewable about replicating upstream verbatim. | |
| - **`main` sync PRs**: when `upstream-sync` is merged into `main`, do it via a PR (e.g., `sync/<date>` branch) — especially if the merge has conflicts that resolved against customizations. Trivial fast-forward merges (no conflicts, no affected customizations) are the exception and may be done directly by a maintainer, though a PR is still preferred for visibility. |
Resolved 1 superpower review finding (Important). The previous "use --fill then edit before publishing" instruction was misleading: gh pr create --fill skips the prompt and publishes the PR immediately using the last commit message verbatim. For a sync branch, that commit is typically a merge commit titled "Merge origin/upstream-sync into sync/<date>" — useless for reviewers and there's no "before publishing" window to edit. Replaced with the interactive form (gh pr create without --fill): gh prompts for title and body, the maintainer writes a real summary of what upstream changes landed, and only then publishes. The inline comment now explicitly warns against --fill for this workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Superpower review applied (commit
|
| # | Finding | Resolution |
|---|---|---|
| 1 | --fill skips the prompt and publishes immediately using the last commit message verbatim — "edit before publishing" doesn't work |
Dropped --fill. Workflow now uses interactive gh pr create (gh prompts for title and body). Inline comment explicitly warns against --fill for sync PRs |
The agent's two Minor findings (rebase command not shown, recovery/direct-merge paragraph transition) were noted but not applied — they're true polish, not operational blockers.
Diff summary
87a13eb: 6 insertions, 3 deletions. The fix closes a workflow trap where a maintainer following the doc literally would end up with a published PR titled "Merge origin/upstream-sync" — exactly the outcome the previous comment was meant to prevent.
|
@gemini review |
|
@claude review |
|
Claude finished @GiuseppeIuculano's task in 2m 14s —— View job Code Review —
|
| Prior finding | Status |
|---|---|
git fetch origin missing before direct merge — origin/upstream-sync could be stale |
✅ Applied (a2c9e4e) |
--fill auto-publishes sync PR with terse merge commit title |
✅ Dropped (87a13eb), interactive gh pr create documented |
git pull --ff-only origin main missing from feature branch workflow |
✅ Applied (6980ce4) |
Cherry-pick section branching from stale local main |
✅ Applied (6980ce4) |
| Preflight scope "all workflows" contradicted atomic sync exemption | ✅ Applied (b44ecf6) |
| Lease-failure wording "your changes already landed" misleading | ✅ Applied (b44ecf6) |
Trivial sync exception using stale local upstream-sync ref |
✅ Applied (30b61ca) |
Informational — Decision tree "deduplicates" may overstate automaticity (line 157)
"When upstream merges, the next sync deduplicates the commit (or we resolve the conflict in the sync PR)"
Git doesn't automatically deduplicate commits — it depends on merge strategy and whether the diffs are identical. The or we resolve the conflict clause correctly hedges, but "deduplicates" may give contributors false confidence. Worth rewording to "the next sync will either merge cleanly (if git detects the change as already applied) or produce a conflict to resolve in the sync PR". Not a blocker.
Overall verdict
Ready to merge. The one Low item is a formatting change — no command text needs to change, just presentation. Everything operational is correct and consistent: atomic fetch, --force-with-lease, SYNC_DATE capture, origin/upstream-sync throughout consumer workflows, --ff-only on all pulls, git checkout -B for idempotent retry, git fetch origin before direct merge, preflight with correct scope, gh CLI prerequisite, diverged-main recovery, declined-PR cherry-pick path, and the parallel-PR urgent-fix case. Solid contribution.
| git checkout -B sync/$SYNC_DATE # `-B` so a retry on the same day overwrites the previous attempt | ||
| git merge origin/upstream-sync # merge the remote-tracking branch directly, no local checkout needed | ||
| # If conflicts: edit files, then `git add <resolved-files>` and `git merge --continue` | ||
| # (or `git rebase --continue` if you rebased). Confirm `git status` shows a clean | ||
| # state with no in-progress merge before pushing. | ||
| git push -u origin sync/$SYNC_DATE |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md file for the veo-tools fork, detailing the repository structure, sync workflows, and branch naming conventions. The documentation covers the use of main and upstream-sync branches, providing specific git commands for various contribution scenarios. Review feedback suggests enhancing the documented one-liner for trivial syncs by adding the --no-edit flag to the merge command to prevent interactive prompts and notes potential restrictions from GitHub branch protection rules.
|
|
||
| **If `main` has diverged** (the `--ff-only` fails because you have commits on local `main` that aren't on `origin/main`): you've broken the "never push to main" rule, but recovery is straightforward. Confirm you have no irreplaceable uncommitted work, then reset: `git checkout main && git reset --hard origin/main`. If those local commits represent real work, branch them off first (`git checkout -b recovery/local-main`) before resetting, then port them via a normal feature PR. | ||
|
|
||
| For a trivial sync with no conflicts and no customizations affected, the maintainer may merge directly with an explicit branch switch and a pull-first to stay consistent with the other workflows: `git checkout main && git pull --ff-only origin main && git fetch origin && git merge origin/upstream-sync && git push origin main`. The `git pull` refreshes `main`; `git fetch origin` refreshes `origin/upstream-sync` (the pull only fetches `main`, so without this step the merge could pick up a stale mirror state). Merging `origin/upstream-sync` (rather than the local `upstream-sync` ref) ensures the merge takes the latest state pushed to the remote mirror. Note this is **not** a fast-forward in the strict git sense — once `main` has any customization (including this `CONTRIBUTING.md`), `main` diverges from `upstream/main` and `git merge --ff-only` would fail. We just mean "merge without conflicts or PR". The PR path is the default; the direct merge is the documented exception when there's literally nothing to review. |
There was a problem hiding this comment.
The git merge command in this one-liner will open the default system editor for the merge commit message, which interrupts the flow of a 'trivial' sync. Adding the --no-edit flag makes this truly non-interactive. Additionally, note that git push origin main may be blocked by branch protection rules on GitHub (e.g., 'Require a pull request before merging'), even for maintainers, unless they have bypass permissions.
| For a trivial sync with no conflicts and no customizations affected, the maintainer may merge directly with an explicit branch switch and a pull-first to stay consistent with the other workflows: `git checkout main && git pull --ff-only origin main && git fetch origin && git merge origin/upstream-sync && git push origin main`. The `git pull` refreshes `main`; `git fetch origin` refreshes `origin/upstream-sync` (the pull only fetches `main`, so without this step the merge could pick up a stale mirror state). Merging `origin/upstream-sync` (rather than the local `upstream-sync` ref) ensures the merge takes the latest state pushed to the remote mirror. Note this is **not** a fast-forward in the strict git sense — once `main` has any customization (including this `CONTRIBUTING.md`), `main` diverges from `upstream/main` and `git merge --ff-only` would fail. We just mean "merge without conflicts or PR". The PR path is the default; the direct merge is the documented exception when there's literally nothing to review. | |
| For a trivial sync with no conflicts and no customizations affected, the maintainer may merge directly with an explicit branch switch and a pull-first to stay consistent with the other workflows: 'git checkout main && git pull --ff-only origin main && git fetch origin && git merge --no-edit origin/upstream-sync && git push origin main'. The git pull refreshes main; git fetch origin refreshes origin/upstream-sync (the pull only fetches main, so without this step the merge could pick up a stale mirror state). Merging origin/upstream-sync (rather than the local upstream-sync ref) ensures the merge takes the latest state pushed to the remote mirror. Note this is not a fast-forward in the strict git sense — once main has any customization (including this CONTRIBUTING.md), main diverges from upstream/main and git merge --ff-only would fail. We just mean "merge without conflicts or PR". The PR path is the default; the direct merge is the documented exception when there's literally nothing to review. |
Resolved 3 review findings (1 gemini + 1 Copilot + 1 claude bot). - git merge in the sync workflow now uses --no-edit to skip the default editor for the merge commit message. The previous form would have interrupted the otherwise-non-interactive script with an editor session. - `git checkout -B sync/$SYNC_DATE` retry semantics clarified: -B overwrites the local branch on retry, but if the branch was already pushed in a prior attempt, the subsequent git push will need --force-with-lease since the remote ref has moved. Previously the comment only described local-branch overwrite, not the push consequence. - Direct merge exception moved from inline prose into a multi-line code block matching the other workflows. Since this is the workflow that bypasses the PR rule, it deserves at least the same presentation as the safer paths. Added a branch-protection warning: git push origin main may be blocked if GitHub branch protection requires PRs even for maintainers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fourteenth-round review applied (commit
|
| # | Source | Finding | Resolution |
|---|---|---|---|
| 1 | gemini | git merge origin/upstream-sync opens default editor for merge commit message, interrupting non-interactive workflow |
Added --no-edit to both occurrences (sync PR + direct merge exception). Workflow now truly non-interactive |
| 2 | Copilot | Same-day retry: -B overwrites local sync branch, but push semantics on already-pushed branch unclear |
Inline comment now explains: subsequent push needs --force-with-lease when the remote ref has moved from a prior attempt |
| 3 | claude | Direct merge exception buried in inline prose despite being the workflow that bypasses PR rule | Moved to multi-line code block with explicit comments + safety lead-in. Same presentation as the safer paths |
Plus gemini's note about branch protection: added warning that git push origin main may be blocked by GitHub branch protection rules, requiring maintainers without bypass permissions to use the PR workflow instead.
Diff summary
ba0ad6c: 18 insertions, 3 deletions. The direct-merge workflow now has the same operational clarity as the PR-based paths.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CONTRIBUTING.md file for the veo-tools fork, detailing the repository structure, synchronization workflows with the upstream repository, and branch management policies. The review feedback identifies a technical oversight in the preflight instructions regarding the reconciliation of a diverged local main branch; specifically, it suggests ensuring that main is reset to the remote state after backing up local work to prevent future fast-forward merge failures.
|
|
||
| > **Prerequisites** (all workflows): the [GitHub CLI (`gh`)](https://cli.github.com/) installed and authenticated against `github.com` (`gh auth login`). The PR-creation steps below all use `gh pr create`. | ||
| > | ||
| > **Preflight for workflows that touch your working tree** (feature branches and sync-to-main; the atomic `upstream-sync` refresh in the next section is exempt because it doesn't checkout or modify any working files): start from a clean working tree (`git status` should report no modified or untracked files). Stash (`git stash -u`) or commit your local work first — the `git checkout` and `git merge` steps assume the working tree won't get contaminated. If you've been working on `main` directly (despite the rule), reconcile that first: either reset (`git checkout main && git reset --hard origin/main` after backing up your work) or branch it off (`git checkout -b backup/local-main`). |
There was a problem hiding this comment.
The instruction to 'branch it off' is incomplete for reconciling the main branch. If a user simply creates a new branch, their local main branch will still contain the commits that shouldn't be there, which will cause subsequent steps like git pull --ff-only (line 59) to fail. It is more effective to create the backup branch and then reset main to the remote state in a single operation.
| > **Preflight for workflows that touch your working tree** (feature branches and sync-to-main; the atomic `upstream-sync` refresh in the next section is exempt because it doesn't checkout or modify any working files): start from a clean working tree (`git status` should report no modified or untracked files). Stash (`git stash -u`) or commit your local work first — the `git checkout` and `git merge` steps assume the working tree won't get contaminated. If you've been working on `main` directly (despite the rule), reconcile that first: either reset (`git checkout main && git reset --hard origin/main` after backing up your work) or branch it off (`git checkout -b backup/local-main`). | |
| > **Preflight for workflows that touch your working tree** (feature branches and sync-to-main; the atomic `upstream-sync` refresh in the next section is exempt because it doesn't checkout or modify any working files): start from a clean working tree (`git status` should report no modified or untracked files). Stash (`git stash -u`) or commit your local work first — the `git checkout` and `git merge` steps assume the working tree won't get contaminated. If you've been working on `main` directly (despite the rule), reconcile that first: either reset (`git checkout main && git reset --hard origin/main` after backing up your work) or branch it off and reset main (`git branch backup/local-main && git reset --hard origin/main`). |
|
@claude review |
|
Claude finished @GiuseppeIuculano's task in 2m 12s —— View job Code Review —
|
| Prior finding | Status |
|---|---|
Direct merge missing git fetch origin — origin/upstream-sync could be stale |
✅ Applied (a2c9e4e) |
| Direct merge buried in inline prose — harder to copy, safety notes invisible | ✅ Applied (ba0ad6c) |
git merge opens editor — interrupts non-interactive sync workflow |
✅ Applied (ba0ad6c, --no-edit added to both occurrences) |
--fill auto-publishes terse merge-commit title for sync PRs |
✅ Dropped (87a13eb), interactive gh pr create documented |
git pull --ff-only origin main missing from feature branch workflow |
✅ Applied (6980ce4) |
Cherry-pick section branching from stale local main |
✅ Applied (6980ce4) |
| Preflight scope "all workflows" contradicted atomic sync exemption | ✅ Applied (b44ecf6) |
Trivial sync exception using stale local upstream-sync ref |
✅ Applied (30b61ca) |
Informational — Decision tree "deduplicates" overstates git automaticity (line 172)
"When upstream merges, the next sync deduplicates the commit (or we resolve the conflict in the sync PR)"
Git doesn't automatically deduplicate commits — the outcome depends on the merge strategy and whether the diff is byte-for-byte identical. The "or we resolve the conflict" clause correctly hedges, but "deduplicates" remains misleading. Consider: "the next sync will either merge cleanly (if the changes have no overlap) or produce a conflict to resolve in the sync PR." Not a blocker.
Overall verdict
Ready to merge with the two fixes applied. Both are small and the Medium one closes a real footgun in the preflight. Everything else is correct: atomic fetch, --force-with-lease on upstream-sync, SYNC_DATE capture, origin/upstream-sync throughout consumer workflows, --ff-only on all pulls, git checkout -B for sync branch, --no-edit on merges, git fetch origin before direct merge, gh prerequisite, diverged-main recovery (consistent with the fix above), declined-PR cherry-pick path, and parallel-PR urgent-fix case. Solid contribution.
"deduplicates" wording Resolved 3 review findings (1 gemini + 2 claude bot). - Preflight recovery for diverged local main: the previous "git checkout -b backup/local-main" alone creates the backup but leaves main still diverged, so subsequent git pull --ff-only steps fail with the same error. Replaced with the atomic form: "git branch backup/local-main && git reset --hard origin/main". This creates the backup ref without switching branches and then resets main in place — the contributor is ready to continue on main immediately. - Sync push: added --force-with-lease unconditionally. The push is on a contributor-owned sync/<date> branch, so the lease is safe. First push is a no-op (remote ref doesn't exist); same-day retries succeed without confusing the contributor with a rejected-non-fast-forward error. Inline comment explains. - Decision tree "deduplicates the commit" reworded — git doesn't automatically deduplicate; the outcome depends on merge strategy and whether the diff is byte-for-byte identical. New wording: "either merges cleanly (if the change has no overlap with what landed) or produces a conflict to resolve in the sync PR". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fifteenth-round review applied (commit
|
| # | Source | Finding | Resolution |
|---|---|---|---|
| 1 | gemini + claude | Preflight "branch it off" with git checkout -b backup/local-main alone leaves main still diverged — subsequent pulls fail |
Atomic form: git branch backup/local-main && git reset --hard origin/main. Creates backup ref without switching, then resets main in place |
| 2 | claude | Sync push git push -u origin sync/$SYNC_DATE fails on same-day retry (the -B above can have moved local ref past origin) |
Added --force-with-lease unconditionally (safe on contributor-owned throwaway PR branch; no-op on first push; handles retry cleanly) |
| 3 | claude (info) | Decision tree "deduplicates the commit" overstates git automaticity | Reworded: "either merges cleanly (if the change has no overlap with what landed) or produces a conflict to resolve in the sync PR" |
Diff summary
65ad489: 4 insertions, 3 deletions. Both Medium-level operational footguns now closed; decision tree wording is honest about merge semantics.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
@claude review |
|
Claude finished @GiuseppeIuculano's task in 2m 38s —— View job Code Review —
|
| Prior finding | Status |
|---|---|
Preflight "branch it off" left main still diverged |
✅ Applied (65ad489) — git branch approach with reset in place |
Sync push git push -u origin sync/$SYNC_DATE fails on same-day retry |
✅ Applied (65ad489) — --force-with-lease added unconditionally |
| Decision tree "deduplicates the commit" overstated git automaticity | ✅ Applied (65ad489) — reworded to "merges cleanly or produces a conflict" |
Direct merge missing git fetch origin before merge |
✅ Applied (a2c9e4e) |
| Direct merge buried in inline prose | ✅ Applied (ba0ad6c) — multi-line code block |
git merge opens editor in non-interactive sync |
✅ Applied (ba0ad6c) — --no-edit on both occurrences |
--fill auto-publishes terse merge commit title |
✅ Dropped (87a13eb) — interactive gh pr create |
git pull --ff-only origin main missing from feature branch workflow |
✅ Applied (6980ce4) |
Cherry-pick section branching from stale local main |
✅ Applied (6980ce4) |
Trivial sync exception using stale local upstream-sync ref |
✅ Applied (30b61ca) |
Overall verdict
Ready to merge with two small fixes. The Medium item is a one-liner (swap git checkout -b for git branch ... && git reset --hard) and directly closes a self-contradiction the Preflight section explicitly warns against. The Low item removes a stale │ artifact. Everything operational is sound: atomic fetch, --force-with-lease on both sync push and PR branch push, SYNC_DATE capture, origin/upstream-sync throughout consumer workflows, --ff-only on all pulls, git checkout -B for idempotent sync branch, --no-edit on merges, git fetch origin before direct merge, gh prerequisite, and parallel-PR urgent-fix case. Solid contribution.
Resolved 6 review findings (2 gemini + 4 claude bot). Atomic write robustness: - gemini #1: gs:// branch now uses the same atomic-write pattern as HTTPS. The @google-cloud/storage downloads also go through temp file + rename, not direct to outputPath. Consistent crash safety. - gemini #2: temp file naming now includes a random suffix: ${outputPath}.${randomSuffix}.tmp instead of fixed .tmp. Prevents collision if two processes target the same output path concurrently (low likelihood in practice but trivially correct). Coherence cleanup: - claude #3: data flow diagram updated to "max 10" matching the module boundary (was still showing the pre-round-21 "max 5"). Diagram also now shows the cross-origin auth strip, HTTPS→HTTP rejection, and atomic write — full alignment with the module boundary description. - claude #4: orphan "#### `REGIONS` constant" heading removed. Round 21 inserted MODEL_DURATIONS shapes between the heading and its content, leaving an empty section visible in the rendered outline. - claude #5: rule #10 row moved to its correct position after rules #8 and #9 in the validation table. Numeric sequence is now #1-#10 followed by F1-F5. - claude #6: DURATION_SUGGESTIONS test coverage added to audio-default.test.ts (co-located since both are pure lookup constants with identical test shape — avoids trivial file proliferation). Test plan now explicitly references both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolved 2 claude bot review findings. - Diverged-main recovery: replaced `git checkout -b recovery/local-main` with the same `git branch X && git reset --hard origin/main` pattern used in the Preflight block. The previous form contradicted the Preflight's own explicit warning: "Avoid `git checkout -b backup/local-main` alone — it creates the backup but leaves main still diverged, so the subsequent `git pull --ff-only` steps will fail." Now consistent. - Decision tree continuation line had a stray `│` character left over from a previous tree-structure refactor. Replaced with spaces to match the adjacent indentation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sixteenth-round review applied (commit
|
| # | Severity | Finding | Resolution |
|---|---|---|---|
| 1 | Medium | "If main has diverged" used git checkout -b recovery/local-main — directly contradicted the Preflight block's own warning against git checkout -b alone |
Replaced with git branch recovery/local-main && git reset --hard origin/main (same atomic pattern as Preflight). Now internally consistent |
| 2 | Low | Stray │ character on the decision tree continuation line (artifact from a previous tree-structure refactor) |
Replaced with spaces to match adjacent indentation |
Diff summary
efce7e8: 2 insertions, 2 deletions. The Medium fix closed a real internal contradiction (the doc warned against a command while using it elsewhere); the Low fix removed a rendering artifact.
Summary
Documents the fork's branching strategy and contribution workflow. Pairs with the new
upstream-syncbranch (also pushed) to establish a clean separation between CTMobi's evolved state and upstream's mainline.What ships in this PR
CONTRIBUTING.mdcovering:main+upstream-sync)origin= CTMobi,upstream= kdowswell)git reset --hard upstream/main+ force-push toupstream-sync)mainfor fork-internal work,upstream-syncfor changes intended for upstreamWhy now
The fork was just created and hadn't diverged from upstream yet — perfect moment to establish the structure before customizations make it harder to retrofit.
Companion artifacts (not in this PR)
upstream-syncalready created and pushed; tracksupstream/mainatf87f3a9(current upstream HEAD).origin↔upstream).Test plan
upstream-syncexists atorigin/upstream-syncand points tof87f3a9kdowswellmerges anything) validates the documented procedure end-to-end🤖 Generated with Claude Code