✨ feat(prompt): add parent commit options to resolve 409 conflicts#723
✨ feat(prompt): add parent commit options to resolve 409 conflicts#723
Conversation
SDK Integration Test Results295 tests +1 294 ✅ ±0 42s ⏱️ +12s For more details on these failures, see this check. Results for commit daa8623. ± Comparison against base commit 1704f1e. ♻️ This comment has been updated with latest results. |
Integration Test Results316 tests ±0 315 ✅ - 1 6m 8s ⏱️ + 1m 0s For more details on these failures, see this check. Results for commit daa8623. ± Comparison against base commit 1704f1e. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR resolves 409 conflicts when updating existing prompts by adding three new CLI options to specify parent commits: --parent-commit, --auto-parent, and --force.
Changes:
- Added
CommitManifestResponsetype to SDK withcommit_hashandmanifestfields - Added
get_commit()method to fetch full commit metadata including hash - Refactored
pull()to useget_commit()internally to reduce code duplication - Added three CLI flags with conflict constraints to handle parent commit scenarios
- Updated test mocks to include
commit_hashfield in responses
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/src/prompts.rs | Added get_commit() method and CommitManifestResponse type; refactored pull() to use new method |
| sdk/src/lib.rs | Exported CommitManifestResponse in public API |
| cli/src/commands/prompt.rs | Added three parent commit CLI flags with logic to auto-fetch, force push, or use explicit hash |
| sdk/tests/structured_prompts_test.rs | Updated mock responses to include commit_hash field matching new API response structure |
Addressed Copilot Review CommentsComment 1: Test Coverage for get_commit()✅ Fixed - Added dedicated test The test validates:
Comment 2: Make --force Mutually Exclusive✅ Fixed - Updated This prevents ambiguous behavior when Comment 3: Fix repo_exists Logic✅ Fixed - Changed Updated error message to clarify that auto-parent will be disabled when creation fails. All changes committed in cccfda5 |
cccfda5 to
3c54b71
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
cli/src/commands/prompt.rs:647
repo_existsdetection treats any error fromclient.prompts().get(&repo_handle)as “not found” and proceeds tocreate_repo(). This can mis-handle non-404 failures (e.g., auth/scoping problems, transient 5xx), and for--auto-parentit can incorrectly disable parent resolution and then push withoutparent_commit(leading to another 409). Consider matching specifically onLangstarError::ApiError { status: 404, .. }for the create-repo path, and returning the original error for other statuses.
let repo_exists = match client.prompts().get(&repo_handle).await {
Ok(_) => {
println!("✓ Repository exists");
true
}
Err(_) => {
formatter.info(&format!(
"Repository not found, creating {}...",
repo_handle
));
match client
| async fn test_pull_structured_prompt_deserialization_error() { | ||
| let mut server = Server::new_async().await; |
There was a problem hiding this comment.
In test_pull_structured_prompt_deserialization_error, the mocked GET /api/v1/commits response body is missing the required commit_hash field. Since pull() now deserializes into CommitManifestResponse (which requires commit_hash), the test will fail for the wrong reason (missing field) instead of exercising StructuredPrompt manifest deserialization. Update the mock response to include a valid commit_hash while keeping the manifest malformed.
There was a problem hiding this comment.
Fixed in commit 94cccb9. Added commit_hash field to the mock response so the test correctly exercises StructuredPrompt manifest deserialization errors (not missing commit_hash).
cli/src/commands/prompt.rs
Outdated
| eprintln!("⚠ Warning: Using --force flag"); | ||
| eprintln!(" This will push without parent commit validation"); | ||
| eprintln!(" May overwrite concurrent changes to the prompt"); |
There was a problem hiding this comment.
--force currently just sets final_parent_commit to None, which is the same request behavior as the default (no --parent-commit/--auto-parent). Given the known 409 behavior when updating existing repos without parent_commit, this flag likely won’t do what the help text claims (“push without parent commit validation”). Either implement a real force mechanism supported by the API (if one exists), or rename/reword/remove the flag so it doesn’t promise behavior the request cannot provide.
| eprintln!("⚠ Warning: Using --force flag"); | |
| eprintln!(" This will push without parent commit validation"); | |
| eprintln!(" May overwrite concurrent changes to the prompt"); | |
| eprintln!("⚠ Note: Using --force flag"); | |
| eprintln!(" Parent commit will not be automatically set for this push."); | |
| eprintln!(" This behaves the same as omitting --parent-commit/--auto-parent; the server may still reject updates that require a parent commit."); |
There was a problem hiding this comment.
This comment is now obsolete. The --force flag (along with --parent-commit and --auto-parent) has been removed entirely from the CLI in commit 7877a9c.
The current implementation makes auto-parent the default behavior - it automatically fetches the latest commit as parent for existing repositories. This eliminates the need for any parent-related flags and provides Git-like UX where updates "just work".
Adds three new CLI options to `langstar prompt push`: - --parent-commit <HASH>: Specify parent commit explicitly - --auto-parent: Automatically fetch latest commit as parent - --force: Push without parent validation (with warnings) This resolves 409 conflicts when updating existing prompts by providing the parent commit reference that the LangSmith API requires. SDK Changes: - Add CommitManifestResponse type for full commit metadata - Add get_commit() method to fetch commit hash and manifest - Refactor pull() to use get_commit() internally CLI Changes: - Add new flags with conflicts_with constraint - Implement auto-fetch logic for --auto-parent - Add warnings for --force flag - Pass parent_commit to SDK for both regular and structured prompts Tests: - Update mock responses to include commit_hash field - All 609 tests passing Fixes #719 Co-Authored-By: Claude <noreply@anthropic.com>
- Add dedicated test coverage for get_commit() method - Make --force flag mutually exclusive with --parent-commit and --auto-parent - Fix repo_exists logic when repository creation fails All three changes improve code quality and prevent ambiguous behavior. Tests: All 610 tests passing. Co-Authored-By: Claude <noreply@anthropic.com>
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>
3c54b71 to
e33cd3e
Compare
Updated CommitManifestResponse.examples doc comment to correctly describe it as "example run records" rather than "example run IDs", per LangSmith schema specification. Addresses review comment: #723 (comment)
Updated test_pull_structured_prompt_deserialization_error to include valid commit_hash field in mock response. The test now correctly exercises StructuredPrompt manifest deserialization error handling without failing on missing commit_hash first. Addresses review comment: #723 (comment)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cli/src/commands/prompt.rs:1
get(&repo_handle)treats any error as “repo not found” and falls into the create path. That will mis-handle cases like 401/403, 5xx, or transient network errors (and can disable auto-parent incorrectly, reintroducing 409 conflicts). Consider branching specifically on “not found” (e.g., HTTP 404) and returning early for other errors; similarly, ifcreate()fails due to “already exists” (e.g., 409), treat it asrepo_exists = trueso auto-parent is still attempted.
use crate::config::Config;
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct CommitManifestResponse { | ||
| /// The commit hash | ||
| pub commit_hash: String, | ||
| /// The commit manifest (prompt template/schema) | ||
| pub manifest: serde_json::Value, | ||
| /// Optional example run records (serialized RepoExampleResponse objects) | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub examples: Option<Vec<serde_json::Value>>, | ||
| } |
There was a problem hiding this comment.
commit_hash is required for deserialization, and pull() now deserializes via CommitManifestResponse (through get_commit()), even though pull() only needs manifest. This makes pull() more brittle/less backwards-compatible if the server ever omits commit_hash (or if older deployments return only { manifest: ... }). Consider either (a) making commit_hash optional with #[serde(default)] so pull() remains tolerant, or (b) splitting response types so pull() uses a minimal manifest-only struct while get_commit() enforces presence of commit_hash (and returns a clear error if missing).
Fixes integer overflow vulnerability in BytesMut::reserve (RUSTSEC-2026-0011). Also adds explicit type annotations to org_workspace_scoping_test.rs to resolve type inference issues introduced by the bytes update. Security advisory: https://rustsec.org/advisories/RUSTSEC-2026-0011
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cli/src/commands/prompt.rs:1
- The
Err(_)branch treats any failure fromprompts().get()as “repo not found” (and then as “new repo” if create succeeds/fails). This can silently disable auto-parent and/or attempt creation when the real issue is auth, network, 5xx, or other non-404 errors—leading to confusing behavior and potentially reintroducing 409s on push. Recommend matching on the SDK error/status and only taking the create/new-repo path for a definite “not found”; otherwise, surface the error and exit (or at least keeprepo_exists = truefor non-404 so parent fetching is still attempted).
use crate::config::Config;
| } | ||
| Err(e) => { | ||
| eprintln!("⚠ Warning: Could not fetch latest commit: {}", e); | ||
| eprintln!(" Proceeding without parent commit (assuming first commit)"); |
There was a problem hiding this comment.
When repo_exists is 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.
| eprintln!(" Proceeding without parent commit (assuming first commit)"); | |
| eprintln!( | |
| " Proceeding without parent commit; update may fail with 409 (retry recommended)" | |
| ); |
Fixes Denial of Service via Stack Exhaustion vulnerability (RUSTSEC-2026-0009). Security advisory: https://rustsec.org/advisories/RUSTSEC-2026-0009
Summary
Resolves 409 conflicts when updating existing prompts by automatically fetching the latest commit as the parent, following Git-like UX expectations. No flags required - it just works.
Changes
SDK (
sdk/src/prompts.rs,sdk/src/lib.rs)CommitManifestResponsetype withcommit_hashandmanifestfieldsget_commit()method to fetch full commit metadatapull()to useget_commit()internallyCommitManifestResponsein public APICLI (
cli/src/commands/prompt.rs)--parent-commit,--auto-parent, and--forceflags (over-engineered)Tests
Behavior
For new prompts:
For existing prompts:
Error handling (graceful fallback):
Design Decision: Minimal Fix
Issue #719 asked for: "The push should automatically fetch the latest commit hash and use it as the parent commit"
This PR delivers exactly that - no flags, no complexity, just automatic parent fetching like Git.
Why Not Flags?
PR #723 originally added three flags (
--parent-commit,--auto-parent,--force), but this was over-engineered:Future Work (Out of Scope)
The
docs/implementation/ls-prompt-ux-command-redesign.mddesign doc outlines a future CRUD redesign (Phase 2, Issue #668.2) wherepushwill be split intocreateandupdatecommands. This minimal fix:Related Issues
Fixes #719
Test Plan
get_commit()SDK method with proper return type🤖 Generated with Claude Code