-
Notifications
You must be signed in to change notification settings - Fork 0
✨ feat(prompt): add parent commit options to resolve 409 conflicts #723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
codekiln
wants to merge
9
commits into
main
Choose a base branch
from
m16-i719-prompt-push-fails-with-409-conflict-when-updating-
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
00a6af3
✨ feat(prompt): add parent commit options to resolve 409 conflicts
codekiln 474b70d
🧪 test(prompt): address Copilot review comments
codekiln 7877a9c
✨ feat(prompt): make auto-parent default behavior for push updates
codekiln e33cd3e
📚 docs: add implementation plan for auto-parent default behavior
codekiln b25a8c2
📚 docs(prompts): fix doc comment for examples field
codekiln 94cccb9
🧪 test(sdk): add commit_hash field to deserialization error test mock
codekiln d518bae
🔄 ci: trigger re-run with rotated API credentials
codekiln 5cd3539
🔒 security(deps): update bytes from 1.10.1 to 1.11.1
codekiln daa8623
🔒 security(deps): update time from 0.3.44 to 0.3.47
codekiln File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
262 changes: 262 additions & 0 deletions
262
docs/implementation/719-prompt-push-auto-parent-default.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,262 @@ | ||
| # Plan: Fix Issue #719 - Auto-Parent Commit as Default Behavior | ||
|
|
||
| **Issue**: #719 - "prompt push fails with 409 conflict when updating existing prompt" | ||
| **Milestone**: #16 (ls-prompt-ux) | ||
| **Date**: 2026-01-23 | ||
|
|
||
| ## Context | ||
|
|
||
| **Root Cause**: LangSmith API requires parent commit when updating existing prompts | ||
| **Current State**: PR #723 added three flags (`--parent-commit`, `--auto-parent`, `--force`) but this is over-engineered | ||
|
|
||
| **Key Insight from User**: | ||
| > "It's my expectation that 'The push should automatically fetch the latest commit hash and use it as the parent commit' as per the description of #719. That's what happens in git and it's what should happen here." | ||
|
|
||
| **Design Document Context**: | ||
| The `docs/implementation/ls-prompt-ux-command-redesign.md` outlines a future CRUD redesign (Phase 2, Issue #668.2) where: | ||
| - `push` will be split into `create` (new prompts) and `update` (existing prompts) | ||
| - This is part of a broader AI-first UX redesign | ||
| - Phase 2 is planned but not yet implemented | ||
|
|
||
| ## Current State Analysis | ||
|
|
||
| ### What Exists Now | ||
| - **PR #723 implementation**: Three flags for parent commit handling | ||
| - **Uncommitted changes**: Removed all three flags, made auto-parent the default | ||
| - **Code location**: `cli/src/commands/prompt.rs` lines 100-800 | ||
| - **SDK support**: `get_commit()` method exists and works (added in PR #723) | ||
|
|
||
| ### Current Behavior After Changes | ||
| ```rust | ||
| // If repo exists → auto-fetch latest commit as parent | ||
| // If new repo → no parent needed | ||
| let final_parent_commit = if repo_exists { | ||
| // Fetch latest commit automatically | ||
| match client.prompts().get_commit(owner, repo, "latest").await { | ||
| Ok(commit_info) => Some(commit_info.commit_hash), | ||
| Err(e) => None // Graceful fallback | ||
| } | ||
| } else { | ||
| None // New repo, no parent needed | ||
| }; | ||
| ``` | ||
|
|
||
| ## Decision Point: Minimal Fix vs CRUD Redesign Now | ||
|
|
||
| ### Option A: Minimal Fix (RECOMMENDED) | ||
| Keep the current `push` command, make auto-parent the default. | ||
|
|
||
| **Pros:** | ||
| - Solves #719 immediately and completely | ||
| - Simple, Git-like UX (just works™) | ||
| - No breaking changes | ||
| - Aligns with future CRUD redesign (can be implemented later in Phase 2) | ||
| - Minimal code changes (flags removed, auto-fetch as default) | ||
|
|
||
| **Cons:** | ||
| - `push` still does both create AND update (slightly confusing) | ||
| - Doesn't implement the CRUD redesign yet | ||
|
|
||
| ### Option B: Implement CRUD Commands Now | ||
| Split `push` into `create` and `update` commands as outlined in redesign doc. | ||
|
|
||
| **Pros:** | ||
| - Implements Phase 2 of redesign immediately | ||
| - Clear separation: `create` for new prompts, `update` for existing | ||
| - More explicit intent | ||
|
|
||
| **Cons:** | ||
| - Much larger scope than issue #719 | ||
| - Requires additional planning and testing | ||
| - Breaking change (deprecating `push`) | ||
| - Out of scope for current milestone | ||
|
|
||
| ## Recommended Approach: Option A (Minimal Fix) | ||
|
|
||
| **Rationale:** | ||
| 1. Issue #719 is about fixing 409 conflicts, not redesigning commands | ||
| 2. The CRUD redesign is planned for Phase 2 (Issue #668.2) - separate work | ||
| 3. Auto-parent as default solves the problem completely and elegantly | ||
| 4. Keeps changes focused and testable | ||
| 5. Doesn't block future CRUD implementation | ||
|
|
||
| ## Implementation Plan | ||
|
|
||
| ### Files to Modify | ||
|
|
||
| 1. **`cli/src/commands/prompt.rs`** (primary changes) | ||
| - Lines 100-148: Remove three flag definitions | ||
| - Lines 588-593: Remove flag variables from pattern match | ||
| - Lines 656-678: Simplify parent commit logic (already done in uncommitted changes) | ||
|
|
||
| 2. **Tests to Update** | ||
| - Any CLI tests that use `--parent-commit`, `--auto-parent`, or `--force` flags | ||
| - Verify auto-parent behavior with integration tests | ||
|
|
||
| 3. **Documentation to Update** | ||
| - PR #723 description needs updating | ||
| - CHANGELOG.md entry should reflect "auto-parent as default" not "added flags" | ||
|
|
||
| ### Detailed Changes | ||
|
|
||
| #### 1. Remove Flag Definitions (DONE in uncommitted changes) | ||
| ```rust | ||
| // REMOVE these lines from Push struct: | ||
| /// Parent commit hash for updates (resolves 409 conflicts) | ||
| #[arg(long, value_name = "HASH", conflicts_with = "auto_parent")] | ||
| parent_commit: Option<String>, | ||
|
|
||
| /// Automatically fetch and use latest commit as parent | ||
| #[arg(long, conflicts_with = "parent_commit")] | ||
| auto_parent: bool, | ||
|
|
||
| /// Force push without parent commit validation (use with caution) | ||
| #[arg(long, conflicts_with_all = ["parent_commit", "auto_parent"])] | ||
| force: bool, | ||
| ``` | ||
|
|
||
| #### 2. Simplify Parent Commit Logic (DONE in uncommitted changes) | ||
| ```rust | ||
| // Replace complex if/else with simple auto-fetch logic | ||
| let final_parent_commit = if repo_exists { | ||
| formatter.info("Fetching latest commit as parent..."); | ||
| match client.prompts().get_commit(owner, repo, "latest").await { | ||
| Ok(commit_info) => { | ||
| println!("✓ Latest commit: {}", commit_info.commit_hash); | ||
| Some(commit_info.commit_hash) | ||
| } | ||
| Err(e) => { | ||
| eprintln!("⚠ Warning: Could not fetch latest commit: {}", e); | ||
| eprintln!(" Proceeding without parent commit (assuming first commit)"); | ||
| None | ||
| } | ||
| } | ||
| } else { | ||
| formatter.info("New repository, no parent commit needed"); | ||
| None | ||
| }; | ||
| ``` | ||
|
|
||
| #### 3. Update Tests | ||
| Search for any tests using the removed flags and update them: | ||
| ```bash | ||
| # Find tests that might use the flags | ||
| grep -r "parent-commit\|auto-parent\|force" sdk/tests/ cli/tests/ | ||
| ``` | ||
|
|
||
| ### Behavior Specification | ||
|
|
||
| **For new prompts:** | ||
| ```bash | ||
| $ langstar prompt push -o owner -r new-prompt -t "template" | ||
| ℹ Checking if repository owner/new-prompt exists... | ||
| ℹ Repository not found, creating owner/new-prompt... | ||
| ✓ Repository created successfully | ||
| ℹ New repository, no parent commit needed | ||
| ℹ Pushing prompt to owner/new-prompt... | ||
| ✓ Pushed successfully | ||
| ``` | ||
|
|
||
| **For existing prompts:** | ||
| ```bash | ||
| $ langstar prompt push -o owner -r existing-prompt -t "updated template" | ||
| ℹ Checking if repository owner/existing-prompt exists... | ||
| ✓ Repository exists | ||
| ℹ Fetching latest commit as parent... | ||
| ✓ Latest commit: abc123def456 | ||
| ℹ Pushing prompt to owner/existing-prompt... | ||
| ✓ Pushed successfully (commit: def789ghi012) | ||
| ``` | ||
|
|
||
| **Error handling (graceful fallback):** | ||
| ```bash | ||
| $ langstar prompt push -o owner -r existing-prompt -t "template" | ||
| ℹ Checking if repository owner/existing-prompt exists... | ||
| ✓ Repository exists | ||
| ℹ Fetching latest commit as parent... | ||
| ⚠ Warning: Could not fetch latest commit: API error: 404 | ||
| Proceeding without parent commit (assuming first commit) | ||
| ℹ Pushing prompt to owner/existing-prompt... | ||
| ✓ Pushed successfully | ||
| ``` | ||
|
|
||
| ## Verification Plan | ||
|
|
||
| ### 1. Manual Testing | ||
| ```bash | ||
| # Test 1: Create new prompt (should work without parent) | ||
| langstar prompt push -o "-" -r test-new-prompt -t "Hello {name}" | ||
|
|
||
| # Test 2: Update existing prompt (should auto-fetch parent) | ||
| langstar prompt push -o "-" -r test-new-prompt -t "Hi {name}, how are you?" | ||
|
|
||
| # Test 3: Verify no 409 errors on update | ||
| langstar prompt push -o "-" -r test-new-prompt -t "Greetings {name}" | ||
| ``` | ||
|
|
||
| ### 2. Automated Tests | ||
| ```bash | ||
| # Run full test suite | ||
| cargo nextest run --profile ci --all-features --workspace | ||
|
|
||
| # Focus on prompt tests | ||
| cargo nextest run --profile ci test_push test_get_commit | ||
| ``` | ||
|
|
||
| ### 3. Validation Criteria | ||
| - ✅ No 409 errors when updating existing prompts | ||
| - ✅ New prompts created without parent commit | ||
| - ✅ Existing prompts updated with auto-fetched parent | ||
| - ✅ Graceful fallback if parent fetch fails | ||
| - ✅ All existing tests pass | ||
| - ✅ Help text updated (no mention of removed flags) | ||
|
|
||
| ## Commit Strategy | ||
|
|
||
| ### Commit Message | ||
| ``` | ||
| ✨ feat(prompt): make auto-parent default behavior for push updates | ||
|
|
||
| Resolves 409 conflicts by automatically fetching latest commit as parent | ||
| when updating existing prompts, following Git-like UX expectations. | ||
|
|
||
| Changes: | ||
| - Remove --parent-commit, --auto-parent, and --force flags | ||
| - Make parent commit fetching automatic and transparent | ||
| - New repos: no parent needed (first commit) | ||
| - Existing repos: auto-fetch latest commit as parent | ||
| - Graceful error handling with fallback to no parent | ||
|
|
||
| Fixes #719 | ||
|
|
||
| Co-Authored-By: Claude <noreply@anthropic.com> | ||
| ``` | ||
|
|
||
| ### PR Updates | ||
| 1. Update PR #723 description to reflect simplified approach | ||
| 2. Remove validation analysis about flags (no longer applicable) | ||
| 3. Update test plan to focus on auto-parent behavior | ||
| 4. Emphasize Git-like UX and simplicity | ||
|
|
||
| ## Future Work (Out of Scope) | ||
|
|
||
| The following are planned but NOT part of this fix: | ||
|
|
||
| 1. **Phase 2 CRUD Redesign** (Issue #668.2): | ||
| - Split `push` into `create` and `update` commands | ||
| - Implement progressive disclosure help system | ||
| - Add `get` command with modifiers | ||
|
|
||
| 2. **Advanced Features** (if needed): | ||
| - `--force` flag for emergency overrides (only if use case emerges) | ||
| - `--parent-commit` for advanced scenarios (only if use case emerges) | ||
|
|
||
| ## Summary | ||
|
|
||
| **Approach**: Minimal fix - make auto-parent the default behavior | ||
| **Scope**: Issue #719 only (409 conflict resolution) | ||
| **Changes**: Remove flags, simplify logic (already done in uncommitted changes) | ||
| **Testing**: Manual + automated verification | ||
| **Outcome**: Git-like UX where "push just works" for both create and update | ||
|
|
||
| This plan solves #719 completely while staying focused and not over-engineering the solution. |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
repo_existsis true, the fallback message “assuming first commit” is misleading (it’s explicitly not the first commit if the repo exists). Also, proceeding without a parent for an existing repo is likely to recreate the original 409 conflict. Suggest changing the warning text to reflect reality (e.g., “Proceeding without parent commit; update may fail with 409 (retry recommended)”) so users understand the consequence and next action.