Remove Strategy interface abstraction and add commit_linking setting

[evisdren]

Summary

• Removes the Strategy interface (~140 lines) that only had one implementation (ManualCommitStrategy), inlining the concrete type throughout the codebase
• Adds [Y/n/a] to the commit linking prompt so users can opt into auto-linking at commit time
• When a user picks a (always), the preference is saved to settings.local.json — future commits auto-link without prompting
• All users default to "prompt" behavior (via GetCommitLinking() returning "prompt" when unset). Users discover "always" through the prompt itself, eliminating setup-time complexity
• Removes isFirstSetup detection, migrateProjectSettings(), and duplicated save blocks from setup.go

Migration behavior

User type	Behavior
New user	Sees [Y/n/a] on first commit with active session, can pick a to auto-link
Existing user (no commit_linking set)	Same prompt, now with a option
User who already has commit_linking: "always"	Auto-links without prompt (unchanged)
User who picks a at prompt	settings.local.json gets commit_linking: "always", future commits auto-link

Test plan

• mise run fmt passes
• mise run lint passes (0 issues)
• mise run test:ci passes (all unit + integration tests)
• Manual: fresh entire enable → no commit_linking in settings (clean)
• Manual: git commit -m "test" with active session → prompt shows [Y/n/a]
• Manual: press a → commit links, settings.local.json now has commit_linking: "always"
• Manual: next git commit -m "test" → auto-links without prompt

🤖 Generated with Claude Code

[cursor]

PR Summary

Medium Risk
Touches git hook behavior and settings parsing/merging, which can affect commit message modification and user workflows if the new commit_linking option is misread or persisted incorrectly. Changes are mostly mechanical type inlining but spread across multiple commands and hooks.

Overview
Simplifies strategy plumbing by removing the Strategy interface and switching call sites to use *strategy.ManualCommitStrategy directly (including GetStrategy, hook contexts, and reset/rewind helpers), with logging now using the manual-commit constant rather than Strategy.Name().

Adds commit_linking to settings ("prompt" default; "always" supported) with validation and local override merge behavior, and updates prepare-commit-msg to optionally auto-link commits or prompt via TTY with a new Always choice that persists commit_linking="always" to settings.local.json.

Removes deprecated strategy field warning output and associated tests, and updates strategy/tests to match the concrete-type API changes.

^{Written by Cursor Bugbot for commit 6096b8d. Configure here.}

[Copilot]

Pull request overview

This pull request simplifies the codebase architecture by removing an unnecessary abstraction layer and adds a new user-configurable setting for commit linking behavior.

Changes:

• Removed the Strategy interface abstraction since only ManualCommitStrategy ever existed, replacing all interface references with the concrete type
• Added a commit_linking setting ("always"/"prompt") to control whether commits are automatically linked to agent sessions or prompt the user each time
• Set new users to default to "always" (auto-link) while existing users retain "prompt" behavior for backward compatibility
• Removed deprecated strategy field warnings from status and doctor commands

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
cmd/entire/cli/strategy/strategy.go	Removed 146-line Strategy interface definition and all its method signatures
cmd/entire/cli/strategy/manual_commit.go	Changed NewManualCommitStrategy to return concrete type, removed Name/Description methods and interface check
cmd/entire/cli/strategy/manual_commit_hooks.go	Integrated commit_linking setting into PrepareCommitMsg to control auto-linking vs prompting behavior, added nolint comments
cmd/entire/cli/strategy/manual_commit_logs.go	Added nolint comment to suppress false positive unparam warning
cmd/entire/cli/strategy/session_test.go	Updated test name and implementation to reflect concrete type usage
cmd/entire/cli/strategy/manual_commit_test.go	Removed unnecessary type casting since NewManualCommitStrategy returns concrete type
cmd/entire/cli/settings/settings.go	Added CommitLinking field, constants, and GetCommitLinking() method; removed deprecated strategy warning functions
cmd/entire/cli/settings/settings_test.go	Added tests for commit_linking field loading, merging, and default behavior; removed deprecated strategy tests
cmd/entire/cli/config.go	Changed GetStrategy return type from interface to concrete type
cmd/entire/cli/setup.go	Set commit_linking to "always" for new installations in both interactive and non-interactive modes
cmd/entire/cli/status.go	Removed calls to WriteDeprecatedStrategyWarnings
cmd/entire/cli/status_test.go	Removed tests for deprecated strategy warnings
cmd/entire/cli/doctor.go	Removed deprecated strategy warning display from sessions fix command
cmd/entire/cli/doctor_test.go	Removed test for deprecated strategy warnings and cleaned up unused imports
cmd/entire/cli/rewind.go	Updated function signatures to accept concrete type instead of interface
cmd/entire/cli/reset.go	Updated function signature to accept concrete type instead of interface
cmd/entire/cli/hooks_git_cmd.go	Removed strategyName field from gitHookContext, use constant directly for logging
cmd/entire/cli/hook_registry.go	Use strategy.StrategyNameManualCommit constant directly instead of calling strategy.Name()
cmd/entire/cli/explain_test.go	Updated test name and implementation to reflect concrete type usage
cmd/entire/cli/checkpoint/temporary.go	Removed nolint:wrapcheck comment (minor/unrelated cleanup)

Comments suppressed due to low confidence (1)

cmd/entire/cli/checkpoint/temporary.go:541

• This lint comment removal appears unrelated to the PR's purpose of removing the Strategy interface and adding commit_linking. The original comment "Propagating context cancellation" was actually correct - context errors should typically be propagated without wrapping to preserve their semantic meaning for context.Canceled and context.DeadlineExceeded checks.

This change should either be reverted or explained in the PR description if it's intentional cleanup.

				return nil, branchErr

[Copilot]

The commit_linking field accepts any string value without validation. Invalid values like "never", "sometimes", or typos like "allways" will be silently accepted and treated as "prompt" by GetCommitLinking(), which could lead to unexpected behavior.

Consider adding validation to reject invalid values and provide clear error messages. For example, after unmarshaling the value, check if it's one of the allowed constants (CommitLinkingAlways or CommitLinkingPrompt) and return an error if not.

Suggested change

	settings.CommitLinking = cl
	switch cl {
	case "always", "prompt":
	settings.CommitLinking = cl
	default:
	return fmt.Errorf("invalid commit_linking value %q: must be \"always\" or \"prompt\"", cl)
	}

[evisdren]

bugbot run

[khaong]

Two thoughts on the commit_linking approach:

1. I remember we discussed having an "always" option on the commit prompt itself — did that get dropped for a reason? A [Y/n/a] where a writes commit_linking: "always" to settings.local.json feels like the natural place for this. Users discover it when they're actually committing, and since it's a personal preference (one dev likes prompts, another doesn't), local settings is the right home.

2. If we go that route, commit_linking can just be a normal setting — no need for migrateProjectSettings, the struct-copy-and-strip, or the duplicated save blocks. determineSettingsTarget and settings.Load() already handle the merge.

[evisdren]

Two thoughts on the commit_linking approach:

1. I remember we discussed having an "always" option on the commit prompt itself — did that get dropped for a reason? A [Y/n/a] where a writes commit_linking: "always" to settings.local.json feels like the natural place for this. Users discover it when they're actually committing, and since it's a personal preference (one dev likes prompts, another doesn't), local settings is the right home.
2. If we go that route, commit_linking can just be a normal setting — no need for migrateProjectSettings, the struct-copy-and-strip, or the duplicated save blocks. determineSettingsTarget and settings.Load() already handle the merge.

1. Yes, you're right. Will add that in.
2. Good point, will update

[evisdren]

bugbot run

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

[cursor]

New users never get "always" commit linking default

High Severity

The PR description states new users (via entire enable) should default to commit_linking: "always" (auto-link), while existing users without the setting default to "prompt". However, neither runEnableInteractive nor setupAgentHooksNonInteractive ever sets CommitLinking on the settings object. Since GetCommitLinking() returns CommitLinkingPrompt when the field is empty, all users — including new ones — get "prompt" behavior instead of the intended "always".

Additional Locations (1)

• cmd/entire/cli/settings/settings.go#L68-L74

 

[evisdren]

this was based on outdataed PR description - resolved now

[cursor]

Saving commit linking creates unintended enabled override

Medium Severity

saveCommitLinkingAlways calls settings.LoadFromFile on a potentially nonexistent settings.local.json, which returns a default struct with Enabled: true. It then saves the full struct, writing "enabled": true into local settings as a side effect. This pins an explicit enabled: true in the local file that wasn't user-intended, which could prevent a later project-level enabled: false from taking effect since local settings override project settings during merge.

 

[evisdren]

cursor bugbot