Merge pull request #101 from jscarle/feature/fix-change-tracking

jscarle · web-flow · commit 24a2f34fdd18 · 2026-03-08T20:42:40.000-05:00
diff --git a/AGENTS.md b/AGENTS.md
@@ -27,6 +27,11 @@ This repository contains `OnePassword.NET`, a .NET wrapper for the 1Password CLI
 - Do not read, search, or summarize generated documentation/site assets unless the user explicitly asks for them.
 - In particular, avoid generated docfx output and bundled vendor assets such as minified JavaScript, CSS, or copied third-party files; prefer the markdown and source files under `docfx/` instead.
 
+## Tests
+
+- In this repository, skipped live tests usually mean the configured test account does not support the required operations for those scenarios.
+- When reporting test results, call out that capability limitation explicitly instead of treating those skips as product failures by default.
+
 ## API Abstraction
 
 - Never expose or leak raw 1Password CLI responses through the public API unless the user explicitly asks for that exact behavior.
diff --git a/OnePassword.NET.Tests/OnePasswordManagerCommandTests.cs b/OnePassword.NET.Tests/OnePasswordManagerCommandTests.cs
@@ -4,6 +4,8 @@
 using System.Runtime.InteropServices;
 using OnePassword.Common;
 using OnePassword.Documents;
+using OnePassword.Items;
+using OnePassword.Templates;
 using OnePassword.Vaults;
 
 namespace OnePassword;
@@ -211,6 +213,74 @@ public void UpdateExtractsCurrentPlatformExecutablePayload()
         });
     }
 
+    [Test]
+    public void AcceptChangesOnItemClearsNestedUrlChanges()
+    {
+        var item = CreateTrackedItem("item-id", "Original Title");
+        item.Urls.Add(new Url { Href = "https://initial.example.com" });
+
+        AcceptChanges(item);
+
+        Assert.That(IsTrackedChanged(item), Is.False);
+
+        item.Urls[0].Href = "https://updated.example.com";
+        Assert.That(IsTrackedChanged(item), Is.True);
+
+        AcceptChanges(item);
+
+        Assert.That(IsTrackedChanged(item), Is.False);
+    }
+
+    [Test]
+    public void AcceptChangesOnFieldClearsTypeChanges()
+    {
+        var field = new Field("Environment", FieldType.String, "Production");
+
+        AcceptChanges(field);
+
+        Assert.That(IsTrackedChanged(field), Is.False);
+
+        field.Type = FieldType.Concealed;
+        Assert.That(IsTrackedChanged(field), Is.True);
+
+        AcceptChanges(field);
+
+        Assert.That(IsTrackedChanged(field), Is.False);
+    }
+
+    [Test]
+    public void CreateItemFailureKeepsTemplateDirty()
+    {
+        using var fakeCli = new FakeCli(errorOutput: "[ERROR] create failed");
+        var manager = fakeCli.CreateManager();
+        var template = new Template
+        {
+            Title = "Original Title"
+        };
+
+        AcceptChanges(template);
+
+        template.Title = "Updated Title";
+
+        Assert.Throws<InvalidOperationException>(() => manager.CreateItem(template, "vault-id"));
+        Assert.That(IsTrackedChanged(template), Is.True);
+    }
+
+    [Test]
+    public void EditItemFailureKeepsItemDirty()
+    {
+        using var fakeCli = new FakeCli(errorOutput: "[ERROR] edit failed");
+        var manager = fakeCli.CreateManager();
+        var item = CreateTrackedItem("item-id", "Original Title");
+
+        AcceptChanges(item);
+
+        item.Title = "Updated Title";
+
+        Assert.Throws<InvalidOperationException>(() => manager.EditItem(item, "vault-id"));
+        Assert.That(IsTrackedChanged(item), Is.True);
+    }
+
     [Test]
     public void ShareItemWithoutEmailsOmitsEmailsFlag()
     {
@@ -345,16 +415,18 @@ private sealed class FakeCli : IDisposable
     {
         private readonly string _argumentsPath;
         private readonly string _directoryPath;
+        private readonly string _errorOutputPath;
         private readonly string _nextOutputPath;
         private readonly string _updateMessagePath;
         private readonly string _updatePayloadPath;
         private readonly string _updatedVersionOutputPath;
         private readonly string _versionOutputPath;
 
-        public FakeCli(string versionOutput = "2.32.1\n", string nextOutput = "{}", string? updateVersionOutput = null)
+        public FakeCli(string versionOutput = "2.32.1\n", string nextOutput = "{}", string? updateVersionOutput = null, string? errorOutput = null)
         {
             _directoryPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
             _argumentsPath = Path.Combine(_directoryPath, "last-arguments.txt");
+            _errorOutputPath = Path.Combine(_directoryPath, "error-output.txt");
             _nextOutputPath = Path.Combine(_directoryPath, "next-output.txt");
             _updateMessagePath = Path.Combine(_directoryPath, "update-output.txt");
             _updatePayloadPath = Path.Combine(_directoryPath, "update-payload.zip");
@@ -364,6 +436,8 @@ public FakeCli(string versionOutput = "2.32.1\n", string nextOutput = "{}", stri
             Directory.CreateDirectory(_directoryPath);
             File.WriteAllText(_nextOutputPath, nextOutput);
             File.WriteAllText(_versionOutputPath, versionOutput);
+            if (errorOutput is not null)
+                File.WriteAllText(_errorOutputPath, errorOutput);
             if (updateVersionOutput is not null)
             {
                 File.WriteAllText(_updateMessagePath, $"Version {updateVersionOutput.Trim()} is now available.");
@@ -435,6 +509,10 @@ @echo off
                     type "%~dp0VERSION_OUTPUT_PLACEHOLDER"
                     exit /b 0
                   )
+                  if exist "%~dp0error-output.txt" (
+                    type "%~dp0error-output.txt" 1>&2
+                    exit /b 0
+                  )
                   type "%~dp0next-output.txt"
                   """.Replace("VERSION_OUTPUT_PLACEHOLDER", versionOutputFileName)
                 : """
@@ -454,13 +532,65 @@ exit 0
                     cat "$script_dir/VERSION_OUTPUT_PLACEHOLDER"
                     exit 0
                   fi
+                  if [ -f "$script_dir/error-output.txt" ]; then
+                    cat "$script_dir/error-output.txt" >&2
+                    exit 0
+                  fi
                   cat "$script_dir/next-output.txt"
                   """.Replace("VERSION_OUTPUT_PLACEHOLDER", versionOutputFileName);
         }
 
         private static string PackagedExecutableName => RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "op.exe" : "op";
     }
 
+    private static Item CreateTrackedItem(string itemId, string title)
+    {
+        var item = new Item
+        {
+            Title = title
+        };
+        SetNonPublicProperty(item, nameof(Item.Id), itemId);
+        return item;
+    }
+
+    private static void SetNonPublicProperty(object target, string propertyName, object? value)
+    {
+        var property = target.GetType().GetProperty(propertyName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)
+            ?? throw new InvalidOperationException($"Could not find property '{propertyName}' on type {target.GetType().Name}.");
+        property.SetValue(target, value);
+    }
+
+    private static bool IsTrackedChanged(object tracked)
+    {
+        var interfaceType = GetTrackedInterface(tracked.GetType());
+        var changedProperty = interfaceType.GetProperty("Changed", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)
+            ?? throw new InvalidOperationException("Could not find ITracked.Changed.");
+        var interfaceMap = tracked.GetType().GetInterfaceMap(interfaceType);
+        var methodIndex = Array.IndexOf(interfaceMap.InterfaceMethods, changedProperty.GetMethod);
+        return methodIndex >= 0
+            ? (bool)(interfaceMap.TargetMethods[methodIndex].Invoke(tracked, null) ?? false)
+            : throw new InvalidOperationException("Could not resolve the ITracked.Changed implementation.");
+    }
+
+    private static void AcceptChanges(object tracked)
+    {
+        var interfaceType = GetTrackedInterface(tracked.GetType());
+        var acceptChangesMethod = interfaceType.GetMethod("AcceptChanges", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)
+            ?? throw new InvalidOperationException("Could not find ITracked.AcceptChanges.");
+        var interfaceMap = tracked.GetType().GetInterfaceMap(interfaceType);
+        var methodIndex = Array.IndexOf(interfaceMap.InterfaceMethods, acceptChangesMethod);
+        if (methodIndex < 0)
+            throw new InvalidOperationException("Could not resolve the ITracked.AcceptChanges implementation.");
+
+        interfaceMap.TargetMethods[methodIndex].Invoke(tracked, null);
+    }
+
+    private static Type GetTrackedInterface(Type type)
+    {
+        return type.GetInterfaces().FirstOrDefault(x => x.FullName == "OnePassword.Common.ITracked")
+            ?? throw new InvalidOperationException($"Type {type.Name} does not implement OnePassword.Common.ITracked.");
+    }
+
     private sealed class TestDocument(string id) : IDocument
     {
         public string Id { get; } = id;
diff --git a/OnePassword.NET/Common/TrackedList.cs b/OnePassword.NET/Common/TrackedList.cs
@@ -93,6 +93,11 @@ public T this[int index]
     void ITracked.AcceptChanges()
     {
         _changed = false;
+        foreach (var item in _list)
+        {
+            if (item is ITracked tracked)
+                tracked.AcceptChanges();
+        }
         _initialList = new List<T>(_list);
     }
 
@@ -215,4 +220,4 @@ void IList<T>.RemoveAt(int index)
         _list.RemoveAt(index);
         _changed = true;
     }
-}
+}
diff --git a/OnePassword.NET/Items/Field.cs b/OnePassword.NET/Items/Field.cs
@@ -99,6 +99,7 @@ public Field(string label, FieldType type, string value, Section? section = null
     /// <inheritdoc />
     void ITracked.AcceptChanges()
     {
+        TypeChanged = false;
         ValueChanged = false;
     }
 }
diff --git a/OnePassword.NET/OnePasswordManager.Items.cs b/OnePassword.NET/OnePasswordManager.Items.cs
@@ -130,12 +130,9 @@ public Item CreateItem(Template template, string vaultId)
             command += $" {QuoteArgument(assignmentStatement)}";
 
         var json = SerializeTemplateForItemCommand(template);
+        var createdItem = Op(JsonContext.Default.Item, command, json);
         ((ITracked)template).AcceptChanges();
-        if (template.TitleChanged)
-            command += $" --title \"{template.Title}\"";
-        if (((ITracked)template.Tags).Changed)
-            command += $" --tags \"{template.Tags.ToCommaSeparated()}\"";
-        return Op(JsonContext.Default.Item, command, json);
+        return createdItem;
     }
 
     /// <inheritdoc />
@@ -172,8 +169,9 @@ public Item EditItem(Item item, string vaultId)
             var changedUrl = item.Urls.FirstOrDefault(url => url.Primary && ((ITracked)url).Changed);
             command += $" --url \"{changedUrl}\"";
         }
+        var editedItem = Op(JsonContext.Default.Item, command, json);
         ((ITracked)item).AcceptChanges();
-        return Op(JsonContext.Default.Item, command, json);
+        return editedItem;
     }
 
     /// <inheritdoc />

Original file line number	Diff line number	Diff line change
`@@ -93,6 +93,11 @@ public T this[int index]`
`93`	`93`	`void ITracked.AcceptChanges()`
`94`	`94`	`{`
`95`	`95`	`_changed = false;`
	`96`	`+ foreach (var item in _list)`
	`97`	`+ {`
	`98`	`+ if (item is ITracked tracked)`
	`99`	`+ tracked.AcceptChanges();`
	`100`	`+ }`
`96`	`101`	`_initialList = new List<T>(_list);`
`97`	`102`	`}`
`98`	`103`
`@@ -215,4 +220,4 @@ void IList<T>.RemoveAt(int index)`
`215`	`220`	`_list.RemoveAt(index);`
`216`	`221`	`_changed = true;`
`217`	`222`	`}`
`218`		`-}`
	`223`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -99,6 +99,7 @@ public Field(string label, FieldType type, string value, Section? section = null`
`99`	`99`	`/// <inheritdoc />`
`100`	`100`	`void ITracked.AcceptChanges()`
`101`	`101`	`{`
	`102`	`+ TypeChanged = false;`
`102`	`103`	`ValueChanged = false;`
`103`	`104`	`}`
`104`	`105`	`}`