Skip to content

Remove checkpoints v2 write paths#1249

Merged
computermode merged 10 commits into
mainfrom
disallow-checkpoints-v2-setting
May 22, 2026
Merged

Remove checkpoints v2 write paths#1249
computermode merged 10 commits into
mainfrom
disallow-checkpoints-v2-setting

Conversation

@computermode
Copy link
Copy Markdown
Contributor

@computermode computermode commented May 22, 2026

https://entire.io/gh/entireio/cli/trails/413

Summary

  • Disallow all checkpoints v2 write/push settings for write paths: strategy_options.checkpoints_version: 2, checkpoints_v2, push_v2, and push_v2_refs now fall back to v1 behavior.
  • Keep push-time feedback for stale v2 settings. Each push warns with [entire] strategy_options.checkpoints_version %v is no longer supported. Falling back to version 1 until the v2 setting is removed from settings.
  • Collapse write and push code to v1-only paths: attach, condensation/finalization, manual pre-push, and checkpoint-remote push setup no longer branch into v2 writes, v2 pushes, or v2 fetch-for-push.
  • Remove obsolete v2 write/push helpers and gates, including CheckpointsWriteVersion, IsCheckpointsV2WriteEnabled, IsPushV2RefsEnabled, and the private v2 push implementation.
  • Preserve read fallback behavior. Existing v2 checkpoint metadata remains readable/resumable/explainable through the existing v2 read/fetch paths and read fallback tests.
  • Remove obsolete v2 dual-write and v2 push tests, while keeping warning and v2 read/resume fallback coverage.

Test plan

  • Set checkpoints version to v2 in a test repo, observed the CLI output stating it was deprecated and saw v1 pushed up instead
  • Ran explain for v1 checkpoints in this PR, observed no regressions
  • go test ./cmd/entire/cli/settings ./cmd/entire/cli/strategy -run TestWarnIfCheckpointsV2Disallowed
  • go test ./cmd/entire/cli/strategy
  • go test ./cmd/entire/cli -run TestAttach_CheckpointsVersion2_FallsBackToV1
  • go test ./cmd/entire/cli -run TestListCommittedForExplain_MergesV1AndV2
  • go test ./cmd/entire/cli -run TestGetBranchCheckpoints_V2PromptFallbackWhenV1Deleted
  • go test -tags integration ./cmd/entire/cli/integration_test -run TestV2Resume
  • go test -tags integration ./cmd/entire/cli/integration_test -run TestCheckpointsVersion2DisallowedResume
  • mise run lint

When strategy_options.checkpoints_version is set to 2, emit a one-time
warning to stderr and fall back to version 1. The setting was the entry
point for v2-only mode (skipping v1 writes); ignoring it routes all
writes back to v1 while leaving the underlying v2 code paths intact for
read-side fallback. Tests that previously asserted v2-only behavior now
assert the new fallback-to-v1 behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: c996e57d6669
Copilot AI review requested due to automatic review settings May 22, 2026 03:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes settings handling so strategy_options.checkpoints_version: 2 is treated as disallowed: the code emits a one-time stderr warning and falls back to checkpoints version 1, while keeping other configuration behavior intact. The test suite is updated to reflect the new “v2 ignored → v1 behavior” across unit, integration, and attach surfaces.

Changes:

  • Update EntireSettings.CheckpointsVersion() to warn once when configured to 2 and return 1 instead.
  • Adjust unit tests to assert the new warning + fallback behavior (including IsCheckpointsV2Enabled / IsPushV2RefsEnabled expectations).
  • Update integration + attach tests that previously validated “strict v2-only” behavior to validate fallback-to-v1 behavior and absence of v2 refs when only checkpoints_version: 2 is set.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cmd/entire/cli/settings/settings.go Implements the one-time warning and fallback-to-v1 behavior when checkpoints_version is set to 2.
cmd/entire/cli/settings/settings_test.go Updates unit tests to validate fallback behavior and warning emission.
cmd/entire/cli/integration_test/v2_resume_test.go Changes resume integration test from “v2-only” expectations to “v2 disallowed → v1 restore” expectations.
cmd/entire/cli/integration_test/v2_push_test.go Updates push integration test to expect v1 metadata branch push and no v2 refs when checkpoints_version: 2 is configured.
cmd/entire/cli/integration_test/v2_dual_write_test.go Updates dual-write integration tests to expect v1 writes and no v2 refs when checkpoints_version: 2 is configured.
cmd/entire/cli/attach_test.go Updates attach tests to expect v1 metadata behavior and v1-oriented refuse hints when checkpoints_version: 2 is configured.
Comments suppressed due to low confidence (2)

cmd/entire/cli/settings/settings_test.go:893

  • The helper resetCheckpointsVersionWarningOnce assigns a new sync.Once{} into a package-level variable. If any parallel test is executing checkpointsVersionWarningOnce.Do(...) at the same time, this is a data race. Either avoid resetting the global entirely (structure warning assertions to run in one place), or ensure all tests in this package that might touch CheckpointsVersion() are fully serialized while this helper is in use.
// resetCheckpointsVersionWarningOnce zeroes the package-level warn-once so
// each test can independently assert the warning behavior. Cannot restore the
// prior Once value (sync.Once contains a noCopy field, and copying-then-
// assigning back triggers govet's copylocks). Tests using this helper must
// not run in parallel.
func resetCheckpointsVersionWarningOnce(t *testing.T) {
	t.Helper()
	checkpointsVersionWarningOnce = sync.Once{}
}

cmd/entire/cli/settings/settings_test.go:923

  • This test and its subtests are marked t.Parallel() but include a case that calls IsPushV2RefsEnabled() with checkpoints_version: 2, which now emits a global warn-once to stderr. Running in parallel increases the chance of interfering with stderr-capture/warn-once assertions elsewhere in this package and can introduce race-detector failures if tests reset the global sync.Once. Consider removing parallelization here (or at least for the subtest that uses checkpoints_version: 2).
func TestIsPushV2RefsEnabled_RequiresBothFlags(t *testing.T) {
	t.Parallel()

	tests := []struct {
		name     string
		opts     map[string]any
		expected bool
	}{
		{"checkpoints_version 2 ignored after disallow", map[string]any{"checkpoints_v2": false, "push_v2_refs": false, "checkpoints_version": 2}, false},
		{"both true", map[string]any{"checkpoints_v2": true, "push_v2_refs": true}, true},
		{"only checkpoints_v2", map[string]any{"checkpoints_v2": true}, false},
		{"only push_v2_refs", map[string]any{"push_v2_refs": true}, false},
		{"both false", map[string]any{"checkpoints_v2": false, "push_v2_refs": false}, false},
		{"push_v2_refs wrong type", map[string]any{"checkpoints_v2": true, "push_v2_refs": "yes"}, false},
		{"empty options", map[string]any{}, false},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			s := &EntireSettings{

Comment thread cmd/entire/cli/settings/settings.go Outdated
Comment thread cmd/entire/cli/settings/settings_test.go Outdated
Comment thread cmd/entire/cli/settings/settings_test.go Outdated
computermode and others added 9 commits May 22, 2026 11:05
These two strategy_options were the entry points for dual-write and v2
ref push. With checkpoints_version: 2 already disallowed, leaving them
honored is incoherent — treat them the same way:

- IsCheckpointsV2Enabled: always returns false. If checkpoints_v2: true
  is configured, emit a one-time stderr warning and ignore the setting.
- IsPushV2RefsEnabled: always returns false. If push_v2_refs: true is
  configured, same warning treatment.

Read-side fallback when the v2 /main ref exists locally is unchanged —
committed_reader_resolve.go's hasLocalV2MainRef path still selects the
DualCheckpointReader regardless of the (now-ignored) settings.

Tests that exercised the v2 write/push happy paths via these settings
are deleted; the disallow behavior is now covered by:
  - TestIsCheckpointsV2Enabled_TrueIgnoredAfterDisallow
  - TestIsCheckpointsV2Enabled_LoadFromFileIgnoresDisallowedSetting
  - TestIsCheckpointsV2Enabled_LocalOverrideIgnoresDisallowedSetting
  - TestIsPushV2RefsEnabled_AlwaysFalseAfterDisallow
  - TestRunSessionsFix_V2ChecksSkippedWhenSettingIgnored

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 51d8634b453b
Entire-Checkpoint: 9deadbe3849a
Entire-Checkpoint: ca7e7c49c125
Entire-Checkpoint: f678bfcfcfa0
Entire-Checkpoint: 7299217bf05d
Entire-Checkpoint: 749550d59b7d
Entire-Checkpoint: 58d96f1248ed
Entire-Checkpoint: 3a15f532e0cc
@computermode computermode changed the title Disallow checkpoints_version: 2 setting Remove checkpoints v2 write paths May 22, 2026
@computermode computermode marked this pull request as ready for review May 22, 2026 19:49
@computermode computermode requested a review from a team as a code owner May 22, 2026 19:49
@computermode computermode merged commit a14451d into main May 22, 2026
9 checks passed
@computermode computermode deleted the disallow-checkpoints-v2-setting branch May 22, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants