-
Notifications
You must be signed in to change notification settings - Fork 1k
Closed
Description
Problem
internal/merge/merge.go defines its own Issue struct (lines 43-66) that duplicates types.Issue. When fields are added to types.Issue, developers must remember to update the merge driver's copy. This has caused silent data loss:
- fix: merge driver silently drops issue fields (metadata, labels, assignee, etc.) #1480 — Metadata, labels, assignee, and many other fields are dropped during merge
Root Cause: Struct Duplication Drift
Go's JSON unmarshaler ignores unknown fields by default. When types.Issue gained new fields (like Metadata), the merge driver's local struct wasn't updated. Tests passed because they didn't check field preservation, and the compiler didn't complain.
Current State
The merge driver already depends on internal/types:
import "github.com/steveyegge/beads/internal/types"
const (
StatusTombstone = string(types.StatusTombstone)
StatusClosed = string(types.StatusClosed)
)So "vendored code purity" is not a constraint.
Proposed Fix: Use types.Issue Directly
Refactor internal/merge/merge.go to use types.Issue instead of its local struct.
Changes Required
- Remove local
Issuestruct definition - Use
types.Issuethroughout merge logic - Handle merge-specific fields:
RawLine— Add totypes.Issuewithjson:"-"tag, or use a wrapper struct- Time fields —
types.Issueusestime.Time, merge usesstring(needs adapter)
Wrapper Approach (if needed)
type MergeIssue struct {
types.Issue
RawLine string `json:"-"` // Merge-specific: original JSON line for conflict output
}Alternatives Considered
| Option | Pros | Cons |
|---|---|---|
| A. Use types.Issue | Guaranteed sync, compiler enforces | May need wrapper for merge-specific fields |
| B. map[string]json.RawMessage | Auto-preserves unknown fields | Complex custom marshal/unmarshal |
| C. Struct parity test | Low risk, catches drift | Doesn't fix architecture |
Recommendation: Option A. The dependency already exists, and this prevents future data loss.
Acceptance Criteria
-
internal/merge/merge.goimports and usestypes.Issue(or a thin wrapper) - All fields from
types.Issueare preserved during merge - Existing merge tests pass
- Add test: round-trip an issue with metadata through merge, verify preservation
Related
- fix: merge driver silently drops issue fields (metadata, labels, assignee, etc.) #1480 — Immediate fix: add missing fields manually (triage)
- feat: add optional JSON metadata field to issues #1406 — Metadata feature that exposed this bug
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels