From 5279ff910632579f48d319e918c73e9327856761 Mon Sep 17 00:00:00 2001 From: Jean-Sebastien Carle <29762210+jscarle@users.noreply.github.com> Date: Sun, 8 Mar 2026 21:35:24 -0400 Subject: [PATCH] Fixed change tracking and failure-state regressions. --- AGENTS.md | 5 + .../OnePasswordManagerCommandTests.cs | 132 +++++++++++++++++- OnePassword.NET/Common/TrackedList.cs | 7 +- OnePassword.NET/Items/Field.cs | 1 + OnePassword.NET/OnePasswordManager.Items.cs | 10 +- 5 files changed, 147 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9920634..07c5407 100644 --- 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 index a90742d..34c1234 100644 --- 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(() => 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(() => 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,6 +532,10 @@ 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); } @@ -461,6 +543,54 @@ exit 0 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 index 58f0667..2e6e004 100644 --- 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(_list); } @@ -215,4 +220,4 @@ void IList.RemoveAt(int index) _list.RemoveAt(index); _changed = true; } -} \ No newline at end of file +} diff --git a/OnePassword.NET/Items/Field.cs b/OnePassword.NET/Items/Field.cs index bc15c4d..5e94d88 100644 --- 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 /// void ITracked.AcceptChanges() { + TypeChanged = false; ValueChanged = false; } } diff --git a/OnePassword.NET/OnePasswordManager.Items.cs b/OnePassword.NET/OnePasswordManager.Items.cs index 7e94466..01391a8 100644 --- 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; } /// @@ -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; } ///