From 5d4f8f4f416317b11219d2b7527e547729965a2b Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Thu, 26 Mar 2026 16:24:56 -0400 Subject: [PATCH 01/20] (fix(recipient)): harden Outlook recipient fallback resolution - catch COM failures when reading Exchange sender and recipient properties - fall back to mail item, address entry, and property accessor values when Exchange data is unavailable - add regression tests covering sender and recipient fallback behavior for unreadable Exchange users --- .../Recipient/RecipientStaticTests.cs | 106 ++++- .../Recipient/RecipientStatic.cs | 444 +++++++++++------- 2 files changed, 372 insertions(+), 178 deletions(-) diff --git a/UtilitiesCS.Test/OutlookObjects/Recipient/RecipientStaticTests.cs b/UtilitiesCS.Test/OutlookObjects/Recipient/RecipientStaticTests.cs index 358053c2..6a6b226f 100644 --- a/UtilitiesCS.Test/OutlookObjects/Recipient/RecipientStaticTests.cs +++ b/UtilitiesCS.Test/OutlookObjects/Recipient/RecipientStaticTests.cs @@ -161,6 +161,48 @@ public void GetSenderInfo_WithNameSpaceAndUnresolvedSender_FallsBackToMailSender .Be("Ada Lovelace <ada@example.com>"); } + [TestMethod] + public void GetSenderInfo_WhenExchangeUserPropertiesThrowComException_FallsBackToMailValues() + { + // Arrange + var exchangeUser = new Mock(); + var sender = new Mock(); + var mail = new Mock(); + + exchangeUser + .SetupGet(x => x.FirstName) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + exchangeUser + .SetupGet(x => x.LastName) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + exchangeUser + .SetupGet(x => x.PrimarySmtpAddress) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + + sender + .SetupGet(x => x.AddressEntryUserType) + .Returns(OlAddressEntryUserType.olExchangeUserAddressEntry); + sender.Setup(x => x.GetExchangeUser()).Returns(exchangeUser.Object); + sender.SetupGet(x => x.Address).Returns("mdlz@jobalerts.mdlz.com"); + sender.SetupGet(x => x.Name).Returns("Mondelēz International, Inc."); + + mail.SetupGet(x => x.Sender).Returns(sender.Object); + mail.SetupGet(x => x.SenderName).Returns("Mondelēz International, Inc."); + mail.SetupGet(x => x.SenderEmailAddress).Returns("mdlz@jobalerts.mdlz.com"); + + // Act + var result = mail.Object.GetSenderInfo(); + + // Assert + result.Name.Should().Be("Mondelēz International, Inc."); + result.Address.Should().Be("mdlz@jobalerts.mdlz.com"); + result + .Html.Should() + .Be( + "Mondelēz International, Inc. <mdlz@jobalerts.mdlz.com>" + ); + } + [TestMethod] public void ToResolvedRecipient_WhenRecipientDoesNotResolve_ReturnsOriginalRecipientAfterResolveAttempt() { @@ -389,6 +431,33 @@ public void GetInfo_WithStoresWrapper_UsesExchangeNameAndPropertyAccessorFallbac .Be("Ada Lovelace <ada@example.com>"); } + [TestMethod] + public void GetInfo_WithStoresWrapper_WhenExchangePropertiesThrowComException_FallsBackToRecipientValues() + { + // Arrange + var recipient = CreateRecipientMock( + name: "Mondelēz International, Inc.", + address: "mdlz@jobalerts.mdlz.com", + type: (int)OlMailRecipientType.olTo, + userType: OlAddressEntryUserType.olExchangeUserAddressEntry, + hasExchangeUser: true, + exchangeNameThrowsComException: true, + exchangePrimarySmtpThrowsComException: true + ); + + // Act + var result = RecipientStatic.GetInfo(new[] { recipient.Object }, null).Single(); + + // Assert + result.Name.Should().Be("Mondelēz International, Inc."); + result.Address.Should().Be("mdlz@jobalerts.mdlz.com"); + result + .Html.Should() + .Be( + "Mondelēz International, Inc. <mdlz@jobalerts.mdlz.com>" + ); + } + [TestMethod] public void GetInfo_ForRecipientSequence_ProjectsEachRecipient() { @@ -525,7 +594,9 @@ params Microsoft.Office.Interop.Outlook.Recipient[] recipients bool hasExchangeUser = false, string exchangeFirstName = "", string exchangeLastName = "", - string exchangePrimarySmtpAddress = "" + string exchangePrimarySmtpAddress = "", + bool exchangeNameThrowsComException = false, + bool exchangePrimarySmtpThrowsComException = false ) { var propertyAccessor = new Mock(); @@ -562,11 +633,34 @@ params Microsoft.Office.Interop.Outlook.Recipient[] recipients else { var exchangeUser = new Mock(); - exchangeUser.SetupGet(x => x.FirstName).Returns(exchangeFirstName); - exchangeUser.SetupGet(x => x.LastName).Returns(exchangeLastName); - exchangeUser - .SetupGet(x => x.PrimarySmtpAddress) - .Returns(exchangePrimarySmtpAddress); + if (exchangeNameThrowsComException) + { + exchangeUser + .SetupGet(x => x.FirstName) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + exchangeUser + .SetupGet(x => x.LastName) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + } + else + { + exchangeUser.SetupGet(x => x.FirstName).Returns(exchangeFirstName); + exchangeUser.SetupGet(x => x.LastName).Returns(exchangeLastName); + } + + if (exchangePrimarySmtpThrowsComException) + { + exchangeUser + .SetupGet(x => x.PrimarySmtpAddress) + .Throws(new System.Runtime.InteropServices.COMException("Boom")); + } + else + { + exchangeUser + .SetupGet(x => x.PrimarySmtpAddress) + .Returns(exchangePrimarySmtpAddress); + } + addressEntry.Setup(x => x.GetExchangeUser()).Returns(exchangeUser.Object); } } diff --git a/UtilitiesCS/OutlookObjects/Recipient/RecipientStatic.cs b/UtilitiesCS/OutlookObjects/Recipient/RecipientStatic.cs index a7272877..495a49a8 100644 --- a/UtilitiesCS/OutlookObjects/Recipient/RecipientStatic.cs +++ b/UtilitiesCS/OutlookObjects/Recipient/RecipientStatic.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.InteropServices; using System.Text.RegularExpressions; using Microsoft.Office.Interop.Outlook; using UtilitiesCS.Extensions; @@ -21,6 +22,165 @@ public static class RecipientStatic private const string PR_SMTP_ADDRESS = "http://schemas.microsoft.com/mapi/proptag/0x39FE001E"; + private static string FirstNonEmptyValue(params Func[] valueFactories) + { + foreach (var valueFactory in valueFactories) + { + var value = NormalizeRecipientValue(valueFactory(), rejectLegacyExchangeDn: false); + if (!value.IsNullOrEmpty()) + { + return value; + } + } + + return string.Empty; + } + + private static string FirstValidAddressValue(params Func[] valueFactories) + { + foreach (var valueFactory in valueFactories) + { + var value = NormalizeRecipientValue(valueFactory(), rejectLegacyExchangeDn: true); + if (!value.IsNullOrEmpty()) + { + return value; + } + } + + return string.Empty; + } + + private static string NormalizeRecipientValue(string value, bool rejectLegacyExchangeDn) + { + if (string.IsNullOrWhiteSpace(value)) + { + return null; + } + + if ( + rejectLegacyExchangeDn + && value.StartsWith("/o=ExchangeLabs", StringComparison.OrdinalIgnoreCase) + ) + { + return null; + } + + return value; + } + + private static T TryGetComValue(Func valueFactory, string context) + where T : class + { + try + { + return valueFactory(); + } + catch (COMException ex) + { + logger.Warn($"Failed to read {context}. {ex.Message}"); + return null; + } + } + + private static string TryGetPropertyValueAsString( + Func propertyAccessorFactory, + string propertyName, + string context + ) + { + var propertyAccessor = TryGetComValue( + propertyAccessorFactory, + $"{context} property accessor" + ); + if (propertyAccessor is null) + { + return null; + } + + try + { + return propertyAccessor.GetProperty(propertyName) as string; + } + catch (COMException ex) + { + logger.Warn($"Failed to read {context} property {propertyName}. {ex.Message}"); + return null; + } + catch (InvalidOperationException) + { + return null; + } + } + + // Outlook can report an Exchange-flavored address entry type even when + // GetExchangeUser or its directory-backed properties are unreadable. + private static ExchangeUser TryGetExchangeUser(AddressEntry addressEntry, string context) + { + if (addressEntry is null) + { + return null; + } + + return TryGetComValue(() => addressEntry.GetExchangeUser(), $"{context} exchange user"); + } + + private static string TryGetExchangeDisplayName(AddressEntry addressEntry, string context) + { + var exchangeUser = TryGetExchangeUser(addressEntry, context); + if (exchangeUser is null) + { + return null; + } + + var firstName = TryGetComValue(() => exchangeUser.FirstName, $"{context} first name"); + var lastName = TryGetComValue(() => exchangeUser.LastName, $"{context} last name"); + var nameParts = new[] { firstName, lastName }.Where(part => + !string.IsNullOrWhiteSpace(part) + ); + + return string.Join(" ", nameParts); + } + + private static string TryGetExchangePrimarySmtpAddress( + AddressEntry addressEntry, + string context + ) + { + var exchangeUser = TryGetExchangeUser(addressEntry, context); + if (exchangeUser is null) + { + return null; + } + + return TryGetComValue( + () => exchangeUser.PrimarySmtpAddress, + $"{context} primary SMTP address" + ); + } + + private static AddressEntry TryGetRecipientAddressEntry(Recipient recipient) + { + if (recipient is null) + { + return null; + } + + try + { + if (!recipient.Resolved) + { + recipient.Resolve(); + } + + return recipient.AddressEntry; + } + catch (COMException ex) + { + logger.Warn($"Failed to resolve recipient address entry. {ex.Message}"); + return null; + } + } + public static Outlook.AddressList GetGlobalAddressList( this Outlook.Store store, Outlook.Application olApp @@ -60,28 +220,13 @@ public static string ConvertRecipientToHtml(string name, string address) public static string GetSenderName(this MailItem olMail) { - var name = olMail.SenderName; - return name; - //AddressEntry sender = olMail.Sender; - //string senderName = ""; + var sender = TryGetComValue(() => olMail.Sender, "mail sender"); - //if (sender?.AddressEntryUserType == OlAddressEntryUserType.olExchangeUserAddressEntry || sender?.AddressEntryUserType == OlAddressEntryUserType.olExchangeRemoteUserAddressEntry) - //{ - // ExchangeUser exchUser = sender.GetExchangeUser(); - // if (exchUser != null) - // { - // senderName = $"{exchUser.FirstName} {exchUser.LastName}"; - // } - // else - // { - // senderName = sender.Name; - // } - //} - //else - //{ - // senderName = olMail.SenderName; - //} - //return senderName; + return FirstNonEmptyValue( + () => TryGetExchangeDisplayName(sender, "mail sender"), + () => TryGetComValue(() => olMail.SenderName, "mail sender name"), + () => TryGetComValue(() => sender?.Name, "mail sender display name") + ); } public static string GetSenderName(this MeetingItem olMeeting) @@ -96,51 +241,21 @@ public static string GetSenderAddress(this MeetingItem olMeeting) public static string GetSenderAddress(this MailItem olMail) { - return olMail.SenderEmailAddress; - //AddressEntry sender = olMail.Sender; - //string senderAddress = ""; - - //if (sender?.AddressEntryUserType == OlAddressEntryUserType.olExchangeUserAddressEntry || sender?.AddressEntryUserType == OlAddressEntryUserType.olExchangeRemoteUserAddressEntry) - //{ - // ExchangeUser exchUser = sender.GetExchangeUser(); - // if (exchUser != null) - // { - // senderAddress = exchUser.PrimarySmtpAddress; - // } - // else - // { - // senderAddress = sender.Address; - // } - //} - //else - //{ - // senderAddress = olMail.SenderEmailAddress; - //} - //if (senderAddress.IsNullOrEmpty()) - //{ - // var olPA = sender.PropertyAccessor; - // try - // { - // senderAddress = olPA.GetProperty(PR_SMTP_ADDRESS) as string; - // if (senderAddress.IsNullOrEmpty()) - // throw new InvalidOperationException("Sender address is null or empty"); - // } - // catch - // { - // try - // { - // senderAddress = olMail.SenderName; - // if (senderAddress.IsNullOrEmpty() || senderAddress.StartsWith("/o=ExchangeLabs")) - // throw new InvalidOperationException("Sender address and name are null or empty"); - // } - // catch - // { - // senderAddress = ""; - // } - // } - //} - - //return senderAddress; + var sender = TryGetComValue(() => olMail.Sender, "mail sender"); + + return FirstValidAddressValue( + () => TryGetExchangePrimarySmtpAddress(sender, "mail sender"), + () => TryGetComValue(() => olMail.SenderEmailAddress, "mail sender email address"), + () => TryGetComValue(() => sender?.Address, "mail sender address"), + () => + TryGetPropertyValueAsString( + () => sender?.PropertyAccessor, + PR_SMTP_ADDRESS, + "mail sender" + ), + () => TryGetComValue(() => olMail.SenderName, "mail sender fallback name"), + () => TryGetComValue(() => sender?.Name, "mail sender fallback display name") + ); } public static IRecipientInfo GetSenderInfo(this MeetingItem olMeeting) @@ -170,7 +285,6 @@ public static IRecipientInfo GetSenderInfo(this MailItem olMail) { var name = olMail.GetSenderName(); var address = olMail.GetSenderAddress(); - var pa = olMail.Sender.PropertyAccessor; var html = ConvertRecipientToHtml(name, address); return new RecipientInfo(name, address, html); } @@ -413,57 +527,19 @@ public static IEnumerable GetCcRecipients(this MeetingItem olMeeting) private static string GetRecipientAddress(Recipient olRecipient) { - string smtpAddress; - - if ( - olRecipient.AddressEntry.AddressEntryUserType - == OlAddressEntryUserType.olExchangeUserAddressEntry - || olRecipient.AddressEntry.AddressEntryUserType - == OlAddressEntryUserType.olExchangeRemoteUserAddressEntry - ) - { - ExchangeUser exchUser = olRecipient.AddressEntry.GetExchangeUser(); - if (exchUser != null) - { - smtpAddress = exchUser.PrimarySmtpAddress; - } - else - { - smtpAddress = olRecipient.Address; - } - } - else - { - smtpAddress = olRecipient.Address; - } - if (smtpAddress.IsNullOrEmpty()) - { - var olPA = olRecipient.PropertyAccessor; - try - { - smtpAddress = (string)olPA.GetProperty(PR_SMTP_ADDRESS); - if (smtpAddress.IsNullOrEmpty()) - throw new InvalidOperationException("SMTP address is null or empty"); - } - catch - { - try - { - smtpAddress = olRecipient.Name; - if ( - smtpAddress.IsNullOrEmpty() || smtpAddress.StartsWith("/o=ExchangeLabs") - ) - throw new InvalidOperationException( - "SMTP address and name are null, empty, or malformed" - ); - } - catch (System.Exception) - { - smtpAddress = ""; - } - } - } - return smtpAddress; + var addressEntry = TryGetRecipientAddressEntry(olRecipient); + + return FirstValidAddressValue( + () => TryGetExchangePrimarySmtpAddress(addressEntry, "recipient"), + () => TryGetComValue(() => olRecipient?.Address, "recipient address"), + () => + TryGetPropertyValueAsString( + () => olRecipient?.PropertyAccessor, + PR_SMTP_ADDRESS, + "recipient" + ), + () => TryGetComValue(() => olRecipient?.Name, "recipient fallback name") + ); //var OlPA = OlRecipient.PropertyAccessor; //string StrSMTPAddress; //try @@ -524,76 +600,100 @@ string DomainName internal static (string Name, string Address) GetRecipientInfo(Recipient recipient) { - string name, - address; - name = recipient.Name; - address = recipient.Address; - //try - //{ - // if (recipient.AddressEntry.AddressEntryUserType == OlAddressEntryUserType.olExchangeUserAddressEntry || recipient.AddressEntry.AddressEntryUserType == OlAddressEntryUserType.olExchangeRemoteUserAddressEntry) - // { - // ExchangeUser exchUser = recipient.AddressEntry.GetExchangeUser(); - // if (exchUser != null) - // { - // var firstNameExch = exchUser.FirstName; - // address = exchUser.PrimarySmtpAddress; - // var rx = new Regex(@"^(.+)@([^@]+)$"); - // name = $"{exchUser.FirstName} {exchUser.LastName}"; - // } - // else - // { - // name = recipient.Name; - // address = recipient.Address; - // } - // } - // else - // { - // name = recipient.Name; - // address = recipient.Address; - // } - //} - //catch (System.Exception) - //{ - // name = recipient.Name; - // address = recipient.Address; - //} + if (recipient is null) + { + return (null, null); + } - return (name, address); + return (GetRecipientName(recipient), GetRecipientAddress(recipient)); + } + + internal static (string Name, string Address) GetExchangeSenderInfo(AddressEntry sender) + { + if (sender is null) + { + return (null, null); + } + + return ( + TryGetExchangeDisplayName(sender, "sender"), + TryGetExchangePrimarySmtpAddress(sender, "sender") + ); } private static string GetRecipientName(Recipient olRecipient) { - string recipientName; - if ( - olRecipient.AddressEntry.AddressEntryUserType - == OlAddressEntryUserType.olExchangeUserAddressEntry - || olRecipient.AddressEntry.AddressEntryUserType - == OlAddressEntryUserType.olExchangeRemoteUserAddressEntry - ) + var addressEntry = TryGetRecipientAddressEntry(olRecipient); + + return FirstNonEmptyValue( + () => TryGetExchangeDisplayName(addressEntry, "recipient"), + () => TryGetComValue(() => olRecipient?.Name, "recipient name") + ); + } + + private static string GetRecipientHtml(Recipient olRecipient) + { + return ConvertRecipientToHtml( + GetRecipientName(olRecipient), + GetRecipientAddress(olRecipient) + ); + } + + internal static bool TryGetExchangeRecipientType( + Outlook.Recipient recipient, + out Outlook.OlAddressEntryUserType userType + ) + { + userType = default; + + if (recipient is null) + { + return false; + } + + try { - ExchangeUser exchUser = olRecipient.AddressEntry.GetExchangeUser(); - if (exchUser != null) + if (!recipient.Resolved && !recipient.Resolve()) { - recipientName = $"{exchUser.FirstName} {exchUser.LastName}"; + return false; } - else + + var addressEntry = recipient.AddressEntry; + if (addressEntry is null) { - recipientName = olRecipient.Name; + return false; } + + userType = addressEntry.AddressEntryUserType; + return true; } - else + catch (COMException) { - recipientName = olRecipient.Name; + return false; } - return recipientName; } - private static string GetRecipientHtml(Recipient olRecipient) + internal static bool TryGetExchangeAddressEntryType( + Outlook.AddressEntry addressEntry, + out Outlook.OlAddressEntryUserType userType + ) { - return ConvertRecipientToHtml( - GetRecipientName(olRecipient), - GetRecipientAddress(olRecipient) - ); + userType = default; + + if (addressEntry is null) + { + return false; + } + + try + { + userType = addressEntry.AddressEntryUserType; + return true; + } + catch (COMException) + { + return false; + } } } } From 87d9d40246ea21c7ce2a92e40a2f62753772199f Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Sat, 21 Mar 2026 22:54:07 -0400 Subject: [PATCH 02/20] fix: align codex web workflow with linux setup --- .codex/codex-web-setup.plan.md | 2 ++ .codex/codex-web-setup.sh | 2 +- .github/workflows/codex-web-setup-test.yml | 13 +++---------- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/.codex/codex-web-setup.plan.md b/.codex/codex-web-setup.plan.md index ffc23afe..c35e88d6 100644 --- a/.codex/codex-web-setup.plan.md +++ b/.codex/codex-web-setup.plan.md @@ -15,9 +15,11 @@ Fix `.codex/codex-web-setup.sh` so a Linux-based Codex Web bootstrap does not fa 1. Refactor the verification path in `.codex/codex-web-setup.sh` so general prerequisite checks remain mandatory. 2. Detect non-Windows PowerShell hosts and downgrade the Visual Studio-specific task verification from a hard failure to a warning. 3. Update the script's repo notes so they describe partial verification on Linux instead of an expected non-zero exit. +4. Update `.github/workflows/codex-web-setup-test.yml` so CI expects the new Linux warning-and-success behavior rather than the previous hard failure. ## Verification 1. Run a syntax check for `.codex/codex-web-setup.sh`. 2. Run the repository PowerShell quality commands required by policy. 3. If practical, run a focused command path that exercises the non-Windows verification branch without reinstalling dependencies. +4. Run `actionlint` against the updated workflow definition. diff --git a/.codex/codex-web-setup.sh b/.codex/codex-web-setup.sh index 82eaeb06..5d0e553f 100755 --- a/.codex/codex-web-setup.sh +++ b/.codex/codex-web-setup.sh @@ -196,7 +196,7 @@ install_dotnet_coverage() { mkdir -p "${dotnet_tools_dir}" export PATH="${dotnet_tools_dir}:${PATH}" - append_if_missing "${HOME}/.bashrc" 'export PATH="$HOME/.dotnet/tools:$PATH"' + append_if_missing "${HOME}/.bashrc" "export PATH=\"\$HOME/.dotnet/tools:\$PATH\"" if command -v dotnet-coverage >/dev/null 2>&1; then log "dotnet-coverage is already available; skipping." diff --git a/.github/workflows/codex-web-setup-test.yml b/.github/workflows/codex-web-setup-test.yml index 804629fb..b08ff267 100644 --- a/.github/workflows/codex-web-setup-test.yml +++ b/.github/workflows/codex-web-setup-test.yml @@ -79,26 +79,19 @@ jobs: EOF chmod +x "${HOME}/.dotnet/tools/dotnet-coverage" - - name: Execute script and assert expected Linux failure + - name: Execute script and assert expected Linux warning shell: bash run: | set -euo pipefail export PATH="${GITHUB_WORKSPACE}/.github/test-bin:${PATH}" - set +e bash .codex/codex-web-setup.sh > codex-web-setup.log 2>&1 - exit_code=$? - set -e cat codex-web-setup.log - if [ "${exit_code}" -eq 0 ]; then - echo "Expected .codex/codex-web-setup.sh to fail on Linux verification, but it succeeded." - exit 1 - fi - - grep -F "Windows-only Visual Studio task requirements" codex-web-setup.log + grep -F "Skipping Windows-only Visual Studio task verification because this host is not Windows." codex-web-setup.log + grep -F "[codex-web-setup] Setup complete." codex-web-setup.log - name: Upload script log if: always() From 625a5904fe3ccd41a884e46f0ed7b9cbed0e198f Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Sat, 21 Mar 2026 22:54:42 -0400 Subject: [PATCH 03/20] ci: run codex web setup test on branch updates --- .codex/codex-web-setup.plan.md | 1 + .github/workflows/codex-web-setup-test.yml | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/.codex/codex-web-setup.plan.md b/.codex/codex-web-setup.plan.md index c35e88d6..8a02603a 100644 --- a/.codex/codex-web-setup.plan.md +++ b/.codex/codex-web-setup.plan.md @@ -16,6 +16,7 @@ Fix `.codex/codex-web-setup.sh` so a Linux-based Codex Web bootstrap does not fa 2. Detect non-Windows PowerShell hosts and downgrade the Visual Studio-specific task verification from a hard failure to a warning. 3. Update the script's repo notes so they describe partial verification on Linux instead of an expected non-zero exit. 4. Update `.github/workflows/codex-web-setup-test.yml` so CI expects the new Linux warning-and-success behavior rather than the previous hard failure. +5. Add non-default-branch CI triggers so the setup workflow can run from branch pushes and pull requests, not only from `workflow_dispatch` on the default branch. ## Verification diff --git a/.github/workflows/codex-web-setup-test.yml b/.github/workflows/codex-web-setup-test.yml index b08ff267..b372229d 100644 --- a/.github/workflows/codex-web-setup-test.yml +++ b/.github/workflows/codex-web-setup-test.yml @@ -2,6 +2,14 @@ name: Codex Web Setup Test on: workflow_dispatch: + push: + paths: + - ".codex/codex-web-setup.sh" + - ".github/workflows/codex-web-setup-test.yml" + pull_request: + paths: + - ".codex/codex-web-setup.sh" + - ".github/workflows/codex-web-setup-test.yml" permissions: contents: read From e2f6b56dd043346e7ea4597e11ad9767e4ed2145 Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 23 Mar 2026 10:14:30 -0400 Subject: [PATCH 04/20] (fix(concurrent-observable)): relay wrapper as CollectionChanged sender - Update ConcurrentObservableBase to raise CollectionChanged with the wrapper instance for relayed view-model and base-collection events - Add regression tests covering add, remove, and concrete wrapper casting scenarios for ConcurrentObservableCollection subscribers - Include the new sender regression test file in the UtilitiesCS.Test project Refs: #87 --- ...ncurrentObservableCollectionSenderTests.cs | 111 ++++++++++++++++++ UtilitiesCS.Test/UtilitiesCS.Test.csproj | 1 + .../Collections/ConcurrentObservableBase.cs | 12 +- 3 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 UtilitiesCS.Test/ReusableTypeClasses/Concurrent/Observable/Collection/ConcurrentObservableCollectionSenderTests.cs diff --git a/UtilitiesCS.Test/ReusableTypeClasses/Concurrent/Observable/Collection/ConcurrentObservableCollectionSenderTests.cs b/UtilitiesCS.Test/ReusableTypeClasses/Concurrent/Observable/Collection/ConcurrentObservableCollectionSenderTests.cs new file mode 100644 index 00000000..7d48066c --- /dev/null +++ b/UtilitiesCS.Test/ReusableTypeClasses/Concurrent/Observable/Collection/ConcurrentObservableCollectionSenderTests.cs @@ -0,0 +1,111 @@ +using System.Collections.ObjectModel; +using System.Collections.Specialized; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Swordfish.NET.Collections; + +namespace ConcurrentObservableCollection.Tests +{ + /// + /// Regression tests for the CollectionChanged sender identity bug. + /// + /// ConcurrentObservableBase relays CollectionChanged from its internal + /// ObservableCollection, and the original code forwarded the inner + /// collection as the sender instead of the wrapper (this). Any subscriber + /// that casts sender to the wrapper type (e.g. SubjectMapSco) hits + /// InvalidCastException. These tests verify the fix. + /// + [TestClass] + public class ConcurrentObservableCollectionSenderTests + { + /// + /// Verify that the sender passed to CollectionChanged on Add is the + /// ConcurrentObservableCollection wrapper, not the inner + /// ObservableCollection. + /// + [TestMethod] + public void CollectionChanged_Add_SenderIsWrapperNotInnerCollection() + { + // Arrange + var collection = new ConcurrentObservableCollection(); + object capturedSender = null; + + collection.CollectionChanged += (sender, _) => + { + capturedSender = sender; + }; + + // Act + collection.Add(42); + + // Assert — sender must be the wrapper, not ObservableCollection + capturedSender.Should().NotBeNull("CollectionChanged must fire on Add"); + capturedSender + .Should() + .BeSameAs( + collection, + "sender should be the ConcurrentObservableCollection wrapper, " + + "not the internal ObservableCollection" + ); + capturedSender + .Should() + .NotBeOfType>( + "sender must not be the raw ObservableCollection" + ); + } + + /// + /// Verify that sender is the wrapper when an item is removed. + /// + [TestMethod] + public void CollectionChanged_Remove_SenderIsWrapperNotInnerCollection() + { + // Arrange + var collection = new ConcurrentObservableCollection(); + collection.Add("item"); + object capturedSender = null; + + collection.CollectionChanged += (sender, _) => + { + capturedSender = sender; + }; + + // Act + collection.Remove("item"); + + // Assert + capturedSender.Should().NotBeNull("CollectionChanged must fire on Remove"); + capturedSender.Should().BeSameAs(collection); + } + + /// + /// Verify that a subscriber can safely cast sender to the concrete + /// wrapper type — the pattern used by SubjectMap_CollectionChanged. + /// + [TestMethod] + public void CollectionChanged_SenderCanBeCastToConcreteWrapperType() + { + // Arrange + var collection = new ConcurrentObservableCollection(); + ConcurrentObservableCollection castSender = null; + + collection.CollectionChanged += (sender, _) => + { + // This cast mirrors the real-world SubjectMapSco cast pattern. + // Before the fix, this throws InvalidCastException. + castSender = (ConcurrentObservableCollection)sender; + }; + + // Act + collection.Add(1); + + // Assert + castSender + .Should() + .BeSameAs( + collection, + "casting sender to the wrapper type must succeed and return the same instance" + ); + } + } +} diff --git a/UtilitiesCS.Test/UtilitiesCS.Test.csproj b/UtilitiesCS.Test/UtilitiesCS.Test.csproj index cc60caca..2e589a17 100644 --- a/UtilitiesCS.Test/UtilitiesCS.Test.csproj +++ b/UtilitiesCS.Test/UtilitiesCS.Test.csproj @@ -293,6 +293,7 @@ + diff --git a/UtilitiesSwordfish/Collections/ConcurrentObservableBase.cs b/UtilitiesSwordfish/Collections/ConcurrentObservableBase.cs index 17f72e6a..6c5bf3ca 100644 --- a/UtilitiesSwordfish/Collections/ConcurrentObservableBase.cs +++ b/UtilitiesSwordfish/Collections/ConcurrentObservableBase.cs @@ -128,22 +128,26 @@ protected ConcurrentObservableBase(IEnumerable enumerable) _baseCollection.CollectionChanged += HandleBaseCollectionChanged; // ***DRM Comment: Doesn't appear to be firing*** - // Bubble up the notify collection changed event from the view model + // Bubble up the notify collection changed event from the view model. + // Use 'this' as sender so subscribers receive the wrapper collection, + // not the internal ObservableCollectionViewModel. _viewModel.CollectionChanged += (sender, e) => { if (CollectionChanged != null) { - CollectionChanged(sender, e); + CollectionChanged(this, e); } }; // DRM Hack -> Bubble up the notify collection changed event from the base collection. - // Could create duplicate events, but this is the only way I can currently get it to fire + // Could create duplicate events, but this is the only way I can currently get it to fire. + // Use 'this' as sender so subscribers receive the wrapper collection, + // not the internal ObservableCollection. _baseCollection.CollectionChanged += (sender, e) => { if (CollectionChanged != null) { - CollectionChanged(sender, e); + CollectionChanged(this, e); } }; } From affcdb9ee6c5db5ffe0fc53fe77bd143aba717a8 Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 23 Mar 2026 17:56:49 -0400 Subject: [PATCH 05/20] (fix(QfcItemController)): propagate OperationCanceledException from conversation load - Add catch (OperationCanceledException) { throw; } before the broad catch (System.Exception) so mid-load cancellation is not suppressed - Extract ConversationResolver.LoadAsync into protected virtual DoLoadConversationResolverCoreAsync to enable seam-based testing without WinForms infrastructure - Guard PopulateConversationAsync against null ConversationResolver after a silently-swallowed load failure; add token check after load - Add QfcItemControllerTests with four regression tests covering cancellation propagation and null-guard paths --- .../Controllers/QfcItemControllerTests.cs | 165 ++++++++++++++++++ QuickFiler.Test/QuickFiler.Test.csproj | 1 + QuickFiler/Controllers/QfcItemController.cs | 35 +++- 3 files changed, 195 insertions(+), 6 deletions(-) create mode 100644 QuickFiler.Test/Controllers/QfcItemControllerTests.cs diff --git a/QuickFiler.Test/Controllers/QfcItemControllerTests.cs b/QuickFiler.Test/Controllers/QfcItemControllerTests.cs new file mode 100644 index 00000000..02ff66cb --- /dev/null +++ b/QuickFiler.Test/Controllers/QfcItemControllerTests.cs @@ -0,0 +1,165 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using QuickFiler.Controllers; +using QuickFiler.Helper_Classes; + +namespace QuickFiler.Controllers.Tests +{ + /// + /// Regression tests for the cancellation-flow bug in QfcItemController. + /// + /// Root cause: LoadConversationResolverAsync caught all exceptions including + /// OperationCanceledException and suppressed them, leaving ConversationResolver null. + /// PopulateConversationAsync then dereferenced the null property and crashed with + /// NullReferenceException instead of propagating the OperationCanceledException. + /// + [TestClass] + public class QfcItemControllerTests + { + // --------------------------------------------------------------------------- + // Test double: subclass that overrides the static-call seam so tests do not + // require WinForms infrastructure (ItemViewer, MailItem, etc.). + // --------------------------------------------------------------------------- + private sealed class TestableQfcItemController : QfcItemController + { + private readonly Func> _loadCore; + + /// + /// Delegate executed in place of ConversationResolver.LoadAsync. + /// Throw OperationCanceledException to simulate a mid-load cancellation. + /// Throw any other exception to simulate a non-cancellation load failure. + /// + internal TestableQfcItemController(Func> loadCore) + : base() + { + _loadCore = loadCore; + } + + protected override Task DoLoadConversationResolverCoreAsync( + CancellationTokenSource tokenSource, + CancellationToken token, + bool loadAll + ) => _loadCore(); + } + + // --------------------------------------------------------------------------- + // LoadConversationResolverAsync tests + // --------------------------------------------------------------------------- + + [TestMethod] + public async Task LoadConversationResolverAsync_WhenLoadThrowsOperationCanceled_PropagatesCancellation() + { + // Arrange + // Simulates OperationCanceledException thrown from inside ConversationResolver.LoadAsync + // (e.g., from TimeOutTask.RunWithTimeout -> GetConversationDfAsync) while the token + // was canceled during the async operation. + // A separate CTS is used for the method call so the pre-guard passes; the seam + // throws unconditionally to reproduce the bug scenario. + var callCts = new CancellationTokenSource(); + var callToken = callCts.Token; // not canceled; pre-guard passes + + var controller = new TestableQfcItemController(() => + throw new OperationCanceledException(callToken) + ); + + // Act + Func act = () => + controller.LoadConversationResolverAsync(callCts, callToken, false); + + // Assert — before the fix this call completed silently (exception was suppressed) + await act.Should() + .ThrowAsync( + because: "cancellation during load must propagate, not be swallowed" + ); + + callCts.Dispose(); + } + + [TestMethod] + public async Task LoadConversationResolverAsync_WhenLoadThrowsNonCancellation_DoesNotThrow() + { + // Arrange — non-cancellation exceptions (e.g., COM errors) must still be + // suppressed and logged so the overall populate flow can continue gracefully. + var callCts = new CancellationTokenSource(); + var callToken = callCts.Token; + + var controller = new TestableQfcItemController(() => + throw new InvalidOperationException("simulated non-cancel load failure") + ); + + // Act + Func act = () => + controller.LoadConversationResolverAsync(callCts, callToken, false); + + // Assert — non-OCE must still be swallowed; behaviour is unchanged from before fix + await act.Should() + .NotThrowAsync( + because: "non-cancellation load failures must be suppressed and logged" + ); + + callCts.Dispose(); + } + + // --------------------------------------------------------------------------- + // PopulateConversationAsync tests + // --------------------------------------------------------------------------- + + [TestMethod] + public async Task PopulateConversationAsync_WhenLoadCanceledDuringAsync_ThrowsOperationCanceledNotNullRef() + { + // Arrange + // This is the exact regression scenario: token cancelled during LoadAsync, + // OCE was swallowed, ConversationResolver was null, and PopulateConversationAsync + // crashed with NullReferenceException on ConversationResolver.Count.SameFolder. + var callCts = new CancellationTokenSource(); + var callToken = callCts.Token; + + var controller = new TestableQfcItemController(() => + throw new OperationCanceledException(callToken) + ); + + // Act + Func act = () => controller.PopulateConversationAsync(callCts, callToken, false); + + // Assert — before the fix this threw NullReferenceException; + // after the fix it propagates OperationCanceledException. + await act.Should() + .ThrowAsync( + because: "a mid-load cancellation must surface as OperationCanceledException, " + + "not crash with NullReferenceException on the null ConversationResolver" + ); + + callCts.Dispose(); + } + + [TestMethod] + public async Task PopulateConversationAsync_WhenLoadFailsWithNonCancellation_ReturnsWithoutCrash() + { + // Arrange — verifies the null guard added to PopulateConversationAsync: + // if a non-cancellation exception causes the resolver to remain null, + // the method must return cleanly rather than dereference null. + var callCts = new CancellationTokenSource(); + var callToken = callCts.Token; + + var controller = new TestableQfcItemController(() => + throw new InvalidOperationException("simulated non-cancel load failure") + ); + + // Act + Func act = () => controller.PopulateConversationAsync(callCts, callToken, false); + + // Assert — ConversationResolver is null after the suppressed load failure; + // the null guard must prevent a NullReferenceException. + await act.Should() + .NotThrowAsync( + because: "when load fails silently and ConversationResolver is null, " + + "PopulateConversationAsync must return without crashing" + ); + + callCts.Dispose(); + } + } +} diff --git a/QuickFiler.Test/QuickFiler.Test.csproj b/QuickFiler.Test/QuickFiler.Test.csproj index d5320f4d..c2674f4a 100644 --- a/QuickFiler.Test/QuickFiler.Test.csproj +++ b/QuickFiler.Test/QuickFiler.Test.csproj @@ -77,6 +77,7 @@ + Form diff --git a/QuickFiler/Controllers/QfcItemController.cs b/QuickFiler/Controllers/QfcItemController.cs index 0ddbba54..7a0c7921 100644 --- a/QuickFiler/Controllers/QfcItemController.cs +++ b/QuickFiler/Controllers/QfcItemController.cs @@ -30,7 +30,7 @@ internal class QfcItemController : IQfcItemController, INotifyPropertyChanged, I #region ctor - private QfcItemController() { } + protected QfcItemController() { } public QfcItemController( IApplicationGlobals appGlobals, @@ -597,15 +597,17 @@ bool loadAll try { - ConversationResolver = await ConversationResolver.LoadAsync( - _globals, - ItemHelper, + ConversationResolver = await DoLoadConversationResolverCoreAsync( tokenSource, token, - loadAll, - SetTopicThread + loadAll ); } + catch (OperationCanceledException) + { + // Cancellation is an expected flow; propagate so callers can observe it. + throw; + } catch (System.Exception e) { logger.Error($"Error in PopulateConversationAsync: {e.Message}", e); @@ -613,6 +615,24 @@ bool loadAll } } + /// + /// Seam for the static ConversationResolver.LoadAsync call. Override in tests to + /// inject controlled behaviour without requiring WinForms infrastructure. + /// + protected virtual Task DoLoadConversationResolverCoreAsync( + CancellationTokenSource tokenSource, + CancellationToken token, + bool loadAll + ) => + ConversationResolver.LoadAsync( + _globals, + ItemHelper, + tokenSource, + token, + loadAll, + SetTopicThread + ); + public async Task PopulateConversationAsync( CancellationTokenSource tokenSource, CancellationToken token, @@ -620,6 +640,9 @@ bool loadAll ) { await LoadConversationResolverAsync(tokenSource, token, loadAll); + token.ThrowIfCancellationRequested(); + if (ConversationResolver is null) + return; await RenderConversationCountAsync( ConversationResolver.Count.SameFolder, token, From 239351859a24e510249b3e97a477ddb9f8e73ffb Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 23 Mar 2026 18:58:45 -0400 Subject: [PATCH 06/20] (fix(EfcHomeController)): guard ExecuteMovesAsync against re-entrant cleanup and fix metrics predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `_isExecuting` volatile flag to drop re-entrant calls before any field access, preventing NullReferenceException when Cleanup() nulls _globals mid-await - Capture _globals into a local before MoveToFolderAsync as defense-in-depth against concurrent Cleanup() racing the post-await continuation - Fix inverted predicate in QuickFileMetrics_WRITE: `Count == 0` → `Count > 0` to write metrics for non-empty move results instead of empty ones - Add EfcHomeControllerTests with three regression tests covering the re-entrancy guard and both empty-list and null-list paths through QuickFileMetrics_WRITE --- .../Controllers/EfcHomeControllerTests.cs | 110 ++++++++++++++++++ QuickFiler.Test/QuickFiler.Test.csproj | 1 + QuickFiler/Controllers/EfcHomeController.cs | 66 +++++++---- 3 files changed, 152 insertions(+), 25 deletions(-) create mode 100644 QuickFiler.Test/Controllers/EfcHomeControllerTests.cs diff --git a/QuickFiler.Test/Controllers/EfcHomeControllerTests.cs b/QuickFiler.Test/Controllers/EfcHomeControllerTests.cs new file mode 100644 index 00000000..852d2805 --- /dev/null +++ b/QuickFiler.Test/Controllers/EfcHomeControllerTests.cs @@ -0,0 +1,110 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using QuickFiler.Interfaces; +using UtilitiesCS; + +namespace QuickFiler.Controllers.Tests +{ + [TestClass] + public class EfcHomeControllerTests + { + private Mock _mockGlobals; + private Mock _mockParentCleanup; + + [TestInitialize] + public void Setup() + { + _mockGlobals = new Mock(MockBehavior.Loose); + _mockParentCleanup = new Mock(); + } + + /// + /// Creates an EfcHomeController via the private (globals, parentCleanup) constructor, + /// which does not allocate sub-components such as the data model or stop-watch. + /// + private EfcHomeController CreateMinimalController() + { + var ctor = typeof(EfcHomeController).GetConstructor( + BindingFlags.NonPublic | BindingFlags.Instance, + null, + new[] { typeof(IApplicationGlobals), typeof(System.Action) }, + null + ); + ctor.Should().NotBeNull("private (globals, parentCleanup) constructor must exist"); + return (EfcHomeController) + ctor.Invoke(new object[] { _mockGlobals.Object, _mockParentCleanup.Object }); + } + + private static void SetField(object target, string fieldName, object value) + { + var field = target + .GetType() + .GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance); + field.Should().NotBeNull($"field '{fieldName}' must exist on EfcHomeController"); + field.SetValue(target, value); + } + + // Regression test for: + // System.NullReferenceException at EfcHomeController.ExecuteMovesAsync line 346 + // Root cause: re-entrant invocation could call Cleanup() while MoveToFolderAsync was + // awaited, nulling _globals. The second continuation then dereferenced null _globals. + [TestMethod] + public async Task ExecuteMovesAsync_WhenAlreadyExecuting_ReturnsWithoutAccessingNullFields() + { + // Arrange + var controller = CreateMinimalController(); + + // Simulate a concurrent invocation already in progress. + SetField(controller, "_isExecuting", true); + + // _formController is null from the private constructor. + // If the guard were absent, the very first line of ExecuteMovesAsync would throw + // NullReferenceException on _formController.SelectedFolder. + + // Act & Assert: must complete without exception. + Func act = () => controller.ExecuteMovesAsync(); + await act.Should() + .NotThrowAsync("a re-entrant call must be dropped via the _isExecuting guard"); + } + + // Regression test for the inverted guard in QuickFileMetrics_WRITE. + // The original condition was `moved.Count == 0`, which entered the metrics-writing + // block only when the list was empty and would immediately throw DivideByZeroException + // on `Duration /= moved.Count`. The fix changes the condition to `moved.Count > 0`. + [TestMethod] + public void QuickFileMetrics_WRITE_WithEmptyList_SkipsBodyAndDoesNotThrow() + { + // Arrange + var controller = CreateMinimalController(); + // _stopWatch is null in a controller created via the private constructor. + // If the body were entered (old bug: Count == 0), accessing _stopWatch.Elapsed + // would throw NullReferenceException before even reaching DivideByZeroException. + var emptyMoved = new List(); + + // Act & Assert + Action act = () => + controller.QuickFileMetrics_WRITE("session.csv", @"Inbox\Projects", emptyMoved); + act.Should() + .NotThrow( + "an empty moved list must cause the metrics body to be skipped (Count > 0 guard)" + ); + } + + [TestMethod] + public void QuickFileMetrics_WRITE_WithNullList_SkipsBodyAndDoesNotThrow() + { + // Arrange + var controller = CreateMinimalController(); + + // Act & Assert + Action act = () => + controller.QuickFileMetrics_WRITE("session.csv", @"Inbox\Projects", null); + act.Should().NotThrow("a null moved list must be handled by the null guard"); + } + } +} diff --git a/QuickFiler.Test/QuickFiler.Test.csproj b/QuickFiler.Test/QuickFiler.Test.csproj index c2674f4a..0524bb22 100644 --- a/QuickFiler.Test/QuickFiler.Test.csproj +++ b/QuickFiler.Test/QuickFiler.Test.csproj @@ -75,6 +75,7 @@ bin\x86\Release\ + diff --git a/QuickFiler/Controllers/EfcHomeController.cs b/QuickFiler/Controllers/EfcHomeController.cs index 348675b5..8a24cad6 100644 --- a/QuickFiler/Controllers/EfcHomeController.cs +++ b/QuickFiler/Controllers/EfcHomeController.cs @@ -285,6 +285,8 @@ public System.Diagnostics.Stopwatch StopWatch get => _stopWatch; } + private volatile bool _isExecuting; + public bool Loaded => throw new NotImplementedException(); internal void CreateCancellationToken() @@ -319,35 +321,49 @@ public SynchronizationContext UiSyncContext async public Task ExecuteMovesAsync() { - var selectedFolder = _formController.SelectedFolder; - var moveConversation = _formController.MoveConversation; - var convInfo = DataModel.ConversationResolver.ConversationInfo.SameFolder; - if (!moveConversation) + if (_isExecuting) + return; + + _isExecuting = true; + try { - convInfo = convInfo - .Where(itemInfo => itemInfo.EntryId == DataModel.Mail.EntryID) - .ToList(); - } + var selectedFolder = _formController.SelectedFolder; + var moveConversation = _formController.MoveConversation; + var convInfo = DataModel.ConversationResolver.ConversationInfo.SameFolder; + if (!moveConversation) + { + convInfo = convInfo + .Where(itemInfo => itemInfo.EntryId == DataModel.Mail.EntryID) + .ToList(); + } - var result = await _dataModel.MoveToFolderAsync( - selectedFolder, - _formController.SaveAttachments, - _formController.SaveEmail, - _formController.SavePictures, - moveConversation - ); + // Capture _globals before the await: Cleanup() may null the field while + // MoveToFolderAsync is in flight, causing NullReferenceException on resume. + var globals = _globals; + var result = await _dataModel.MoveToFolderAsync( + selectedFolder, + _formController.SaveAttachments, + _formController.SaveEmail, + _formController.SavePictures, + moveConversation + ); - if (!result) - { - MessageBox.Show($"Cannot move to folderpath {selectedFolder}"); + if (!result) + { + MessageBox.Show($"Cannot move to folderpath {selectedFolder}"); + } + else + { + QuickFileMetrics_WRITE( + globals.FS.Filenames.EmailSession, + selectedFolder, + convInfo + ); + } } - else + finally { - QuickFileMetrics_WRITE( - _globals.FS.Filenames.EmailSession, - selectedFolder, - convInfo - ); + _isExecuting = false; } } @@ -367,7 +383,7 @@ public void QuickFileMetrics_WRITE( List moved ) { - if (moved is not null && moved.Count == 0) + if (moved is not null && moved.Count > 0) { var curDateText = DateTime.Now.ToString("MM/dd/yyyy"); var curTimeText = DateTime.Now.ToString("hh:mm"); From fd29a8c1fba2dd75dbdd057dbd6ceb3c95c5f521 Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Wed, 25 Mar 2026 14:31:52 -0400 Subject: [PATCH 07/20] (codex): convert feature review to codex skill and sub-agents --- .codex/agents/atomic-executor.toml | 53 ++++++++++++++++++++ .codex/agents/atomic-planner.toml | 56 ++++++++++++++++++++++ .codex/agents/feature-reviewer.toml | 55 +++++++++++++++++++++ .codex/prompts/feature-review-remediate.md | 10 ++++ 4 files changed, 174 insertions(+) create mode 100644 .codex/agents/atomic-executor.toml create mode 100644 .codex/agents/atomic-planner.toml create mode 100644 .codex/agents/feature-reviewer.toml create mode 100644 .codex/prompts/feature-review-remediate.md diff --git a/.codex/agents/atomic-executor.toml b/.codex/agents/atomic-executor.toml new file mode 100644 index 00000000..d6bb4b7f --- /dev/null +++ b/.codex/agents/atomic-executor.toml @@ -0,0 +1,53 @@ +name = "atomic-executor" +description = "Execute an atomic plan exactly as written, verify acceptance criteria task-by-task, and stop only for preflight-blocking issues." + +developer_instructions = """ +You are an execution-only agent. + +Primary role: +- Execute a plan produced by the atomic-planner exactly as written. +- Preserve phase headings, task IDs, checkbox format, and task order. +- Verify each task’s acceptance criteria before checking it off. +- Do not re-plan. + +Use the following repo-local skills as the canonical workflow source: +- atomic-executor +- policy-compliance-order +- atomic-plan-contract +- acceptance-criteria-tracking + +Hard constraints: +- Do not invent new phases or tasks. +- Do not reorder tasks. +- Do not replace the plan with a different approach. +- Do not use an in-session todo list as a substitute for the plan file. +- The plan file on disk is the source of truth. + +Policy and preflight rules: +- Repository policy files are authoritative over this agent. +- Before execution, perform preflight validation against repo policy and plan-format requirements. +- Blocking is allowed only during preflight, before the first task executes. +- If the plan is invalid, non-atomic, non-verifiable, or conflicts with repo policy, stop at preflight and provide a precise plan delta for correction. +- After execution begins, do not block mid-plan; continue to completion within the allowed scope of the current tasks. + +Execution behavior: +- Execute tasks one by one, in order. +- Only perform micro-actions necessary to complete the current task. +- If completing the current task would require a new independent outcome not described in the plan, stop and request a plan revision only if still in preflight; otherwise stay within the task’s scope. +- After a task passes verification, immediately check it off in the canonical plan file on disk. +- If the completed work satisfies acceptance criteria in the resolved requirements source files, check those off as well per repo rules. + +Verification discipline: +- Prefer repo-defined commands and the canonical toolchain loop. +- Never claim success without verification. +- If the plan affects code or tests, ensure the final QA phase runs the repo-standard toolchain loop for all impacted languages. +- Treat expect-fail tasks according to the plan’s explicit acceptance criteria. + +Resume behavior: +- On resume or continue, load the plan-of-record, find the next unchecked task, and continue from there without replanning. + +Communication discipline: +- Be concise and exact. +- Report commands or tasks run and summarize results. +- End progress updates with the updated checklist or the current phase plus upcoming tasks. +""" \ No newline at end of file diff --git a/.codex/agents/atomic-planner.toml b/.codex/agents/atomic-planner.toml new file mode 100644 index 00000000..a7ecd759 --- /dev/null +++ b/.codex/agents/atomic-planner.toml @@ -0,0 +1,56 @@ +name = "atomic-planner" +description = "Generate deterministic phased implementation plans with atomic checkbox tasks and mandatory atomic-executor preflight clearance before finalization." + +developer_instructions = """ +You are a planning-only agent. + +Primary role: +- Produce phased implementation plans composed of atomic checkbox tasks. +- Design work that can be executed deterministically by an executor without replanning. +- You may read code, docs, plans, and repo policy for context, but you do not implement changes. + +Use the following repo-local skills as the canonical workflow source: +- atomic-planner +- policy-compliance-order +- atomic-plan-contract + +Hard constraints: +- Do not implement code, tests, configuration, CI, or docs beyond plan files. +- Do not execute the plan. +- Your only write scope is creating or updating a Markdown plan document when explicitly requested or when the calling workflow provides an authoritative plan target path. + +Plan requirements: +- Output a brief overview followed by phases and atomic tasks. +- Every task must be binary, atomic, and independently verifiable. +- Every task must use stable checkbox IDs in the canonical form [P#-T#]. +- If the plan changes code or tests, include Phase 0 baseline capture tasks and a final QA phase. +- Use executor-compatible formatting exactly. +- Reject placeholders, vague bucket tasks, and multi-outcome tasks. +- Prefer machine-verifiable acceptance criteria only. + +Mode and policy behavior: +- Repository policy files are authoritative over this agent. +- Resolve work mode from issue.md using repo rules. +- Fail closed when the mode marker is missing or malformed if repo policy requires that behavior. +- For minor-audit mode, require explicit evidence-capture tasks, targeted verification tasks, and end-state evidence tasks. + +Mandatory orchestration rule: +- Before finalizing any plan, explicitly spawn the `atomic-executor` subagent in preflight-validation-only mode. +- Pass the current draft plan file as the plan of record for validation. +- Wait for one of exactly two outcomes: + - `PREFLIGHT: ALL CLEAR` + - `PREFLIGHT: REVISIONS REQUIRED` +- If `atomic-executor` returns `PREFLIGHT: REVISIONS REQUIRED`, revise the same plan file and then explicitly spawn `atomic-executor` again in preflight-validation-only mode. +- Repeat this planner → executor-preflight loop until `atomic-executor` returns `PREFLIGHT: ALL CLEAR`. +- Do not finalize the plan before that result is obtained. + +Preflight loop behavior: +- Preserve the caller-provided target path when one is supplied. +- Do not create extra sibling plan files when an authoritative target file exists. +- Treat executor preflight findings as binding plan defects to be corrected before finalization. + +Self-checking: +- Before each executor preflight pass, confirm zero placeholders, atomic tasks only, canonical phase headings, canonical checkbox syntax, machine-verifiable acceptance criteria, and policy compatibility. +- Before final completion, confirm that `atomic-executor` has returned `PREFLIGHT: ALL CLEAR`. +- If the request is outside planning, refuse execution and direct the workflow to the executor. +""" \ No newline at end of file diff --git a/.codex/agents/feature-reviewer.toml b/.codex/agents/feature-reviewer.toml new file mode 100644 index 00000000..52cfaa41 --- /dev/null +++ b/.codex/agents/feature-reviewer.toml @@ -0,0 +1,55 @@ +name = "feature-reviewer" +description = "Review a feature branch relative to a base branch, generate policy/code/feature audit artifacts, and trigger remediation planning when required." + +developer_instructions = """ +You are a feature-branch reviewer. + +Primary role: +- Review the current feature branch relative to a specified base branch. +- Produce audit-grade review artifacts, not code changes. +- Do not ask clarifying questions unless execution is impossible; make best-effort assumptions and record them explicitly. + +Use the following repo-local skills as the canonical workflow source: +- feature-review +- policy-compliance-order +- evidence-and-timestamp-conventions +- policy-audit-template-usage +- pr-context-artifacts +- acceptance-criteria-tracking +- remediation-handoff-atomic-planner + +Core behavior: +- Treat repository policy files as authoritative over this agent. +- Use PR context artifacts as the primary source of truth for scope and baseline evidence. +- Read artifacts/pr_context.summary.txt thoroughly first. +- Use artifacts/pr_context.appendix.txt as secondary evidence for exact diff anchoring. +- If PR context artifacts are missing or stale, refresh them using the repo’s canonical mechanism before continuing. +- Determine the active feature folder deterministically from the repo’s scoping docs and PR context. +- Write timestamped review artifacts into the active feature folder. + +Required outputs: +- policy-audit..md +- code-review..md +- feature-audit..md +- remediation-inputs..md when remediation is required +- remediation-plan..md when remediation is required + +Review constraints: +- Do not modify policy documents. +- Prefer check-only / no-mutation verification commands. +- Do not silently fix code during review. +- If tooling cannot be run, mark the relevant sections UNVERIFIED or PARTIAL with a concrete reason. +- Continue until all required review artifacts exist on disk. + +Acceptance and remediation rules: +- Trigger remediation if policy audit has FAIL or meaningful PARTIAL findings, toolchain checks fail, blockers exist in code review, or required acceptance criteria are not fully met. +- When remediation is triggered, generate remediation-inputs first. +- Then delegate remediation planning to the atomic-planner agent or equivalent workflow. +- Remediation planning must treat remediation-inputs as the primary requirements source. + +Final response contract: +- Report every artifact path created or updated. +- Provide a one-paragraph go/no-go recommendation for PR readiness. +- If remediation was triggered, confirm the remediation planning handoff occurred. +- Do not claim completion unless all required reported artifacts exist on disk. +""" \ No newline at end of file diff --git a/.codex/prompts/feature-review-remediate.md b/.codex/prompts/feature-review-remediate.md new file mode 100644 index 00000000..3f03896b --- /dev/null +++ b/.codex/prompts/feature-review-remediate.md @@ -0,0 +1,10 @@ +--- +description: Run feature review and, if needed, remediation planning with executor preflight +--- + +$feature-review +Use pr_context as authoritative if present and valid. Only fall back to an explicit base branch if pr_context is missing, stale, or ambiguous. +If remediation is required, explicitly spawn atomic-planner to create or revise the remediation plan. +Before atomic-planner finalizes the plan, it must explicitly spawn atomic-executor in preflight-validation-only mode against that same file. +If atomic-executor returns PREFLIGHT: REVISIONS REQUIRED, atomic-planner must revise the same file and repeat until PREFLIGHT: ALL CLEAR. +Only then finalize the remediation plan and report all generated artifacts. \ No newline at end of file From 06bfd3009924f184dbc4623c9b7abbcfbd54921a Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Tue, 24 Mar 2026 19:49:39 -0400 Subject: [PATCH 08/20] (chore): upgrade ribbon --- TaskMaster/Ribbon/RibbonExplorer.xml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/TaskMaster/Ribbon/RibbonExplorer.xml b/TaskMaster/Ribbon/RibbonExplorer.xml index a385557d..98f766b4 100644 --- a/TaskMaster/Ribbon/RibbonExplorer.xml +++ b/TaskMaster/Ribbon/RibbonExplorer.xml @@ -1,5 +1,5 @@  - + @@ -167,7 +167,7 @@ /> - +