From 75977addc8686d6b9834a7b230d7e476e0c01e2a Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 01:55:29 +1000 Subject: [PATCH 1/3] fix(airlock): stop enable/disable rewriting network.json Toggling airlock read the whole file into NetworkConfig, flipped one bool and serialized it back over the top, so comments, key order and any key the class doesn't model were destroyed. A file containing JSON comments threw instead. The enabled value is now spliced in place using Utf8JsonReader offsets, with a depth check so an "enabled" nested inside a rule can't be matched, and the document is validated up front so a file broken further down isn't rewritten. Separately, --disable-airlock created an empty local network.json when only a global one existed. Local config replaces global entirely, so that silently dropped the project out of its ruleset. Creating a local file now seeds it from the global one and the command says it did. Bad JSON reports the path and exits 1 instead of throwing a stack trace, and the reader accepts comments and trailing commas to match how people edit it. Closes #129 --- README.md | 6 +- app/Commands/Airlock/DisableAirlock.cs | 11 +- app/Commands/Airlock/DisableGlobalAirlock.cs | 11 +- app/Commands/Airlock/EnableAirlock.cs | 12 +- app/Commands/Airlock/EnableGlobalAirlock.cs | 12 +- app/Commands/Airlock/NetworkConfig.cs | 7 +- app/Commands/Airlock/_AirlockCommands.cs | 34 +++ app/Commands/Airlock/_AirlockConfig.cs | 223 +++++++++++++++-- .../AirlockConfigTests.cs | 225 ++++++++++++++++++ 9 files changed, 485 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 9a331c8..57e1476 100644 --- a/README.md +++ b/README.md @@ -252,6 +252,10 @@ Airlock provides an additional layer of security by routing all network traffic - Local: `.copilot_here/network.json` - Default Rules: `~/.config/copilot_here/default-airlock-rules.json` (updated with script updates) +A local `network.json` replaces the global one outright, so the two are never merged. That's why the first time `--enable-airlock` or `--disable-airlock` creates a local file in a project, it copies your global config across rather than starting empty. The command tells you when it does this. + +Enabling and disabling only ever changes the `enabled` value. The commands leave your comments, key order, indentation, and any keys `copilot_here` doesn't know about exactly as you wrote them. + **Example Configuration:** ```json { @@ -276,7 +280,7 @@ Airlock provides an additional layer of security by routing all network traffic - **enforce** (`e`): Blocks requests not matching the allowlist - **monitor** (`m`): Allows all requests but logs them for review -When enabling Airlock for the first time, you'll be prompted to choose between enforce and monitor mode. +New configs start in enforce mode. Switch to monitor by setting `"mode": "monitor"` in `network.json`. **Logging:** When `enable_logging` is true (or in monitor mode), request logs are saved to `.copilot_here/logs/` (excluded from git by default). diff --git a/app/Commands/Airlock/DisableAirlock.cs b/app/Commands/Airlock/DisableAirlock.cs index 578fb03..c6e4a61 100644 --- a/app/Commands/Airlock/DisableAirlock.cs +++ b/app/Commands/Airlock/DisableAirlock.cs @@ -1,5 +1,4 @@ using System.CommandLine; -using CopilotHere.Infrastructure; namespace CopilotHere.Commands.Airlock; @@ -8,13 +7,9 @@ public sealed partial class AirlockCommands private static Command SetDisableAirlockCommand() { var command = new Command("--disable-airlock", "Disable Airlock for local config"); - command.SetAction(_ => - { - var paths = AppPaths.Resolve(); - AirlockConfig.DisableLocal(paths); - Console.WriteLine("✅ Airlock disabled (local)"); - return 0; - }); + command.SetAction(_ => RunToggle( + "✅ Airlock disabled (local)", + paths => (AirlockConfig.DisableLocal(paths), AirlockConfig.GetLocalRulesPath(paths)))); return command; } } diff --git a/app/Commands/Airlock/DisableGlobalAirlock.cs b/app/Commands/Airlock/DisableGlobalAirlock.cs index 6e36159..2cf2ab9 100644 --- a/app/Commands/Airlock/DisableGlobalAirlock.cs +++ b/app/Commands/Airlock/DisableGlobalAirlock.cs @@ -1,5 +1,4 @@ using System.CommandLine; -using CopilotHere.Infrastructure; namespace CopilotHere.Commands.Airlock; @@ -8,13 +7,9 @@ public sealed partial class AirlockCommands private static Command SetDisableGlobalAirlockCommand() { var command = new Command("--disable-global-airlock", "Disable Airlock for global config"); - command.SetAction(_ => - { - var paths = AppPaths.Resolve(); - AirlockConfig.DisableGlobal(paths); - Console.WriteLine("✅ Airlock disabled (global)"); - return 0; - }); + command.SetAction(_ => RunToggle( + "✅ Airlock disabled (global)", + paths => (AirlockConfig.DisableGlobal(paths), AirlockConfig.GetGlobalRulesPath(paths)))); return command; } } diff --git a/app/Commands/Airlock/EnableAirlock.cs b/app/Commands/Airlock/EnableAirlock.cs index 5ec537f..e56b51b 100644 --- a/app/Commands/Airlock/EnableAirlock.cs +++ b/app/Commands/Airlock/EnableAirlock.cs @@ -1,5 +1,4 @@ using System.CommandLine; -using CopilotHere.Infrastructure; namespace CopilotHere.Commands.Airlock; @@ -8,14 +7,9 @@ public sealed partial class AirlockCommands private static Command SetEnableAirlockCommand() { var command = new Command("--enable-airlock", "Enable Airlock with local rules (.copilot_here/network.json)"); - command.SetAction(_ => - { - var paths = AppPaths.Resolve(); - AirlockConfig.EnableLocal(paths); - Console.WriteLine("✅ Airlock enabled (local)"); - Console.WriteLine($" 📁 Rules: {AirlockConfig.GetLocalRulesPath(paths)}"); - return 0; - }); + command.SetAction(_ => RunToggle( + "✅ Airlock enabled (local)", + paths => (AirlockConfig.EnableLocal(paths), AirlockConfig.GetLocalRulesPath(paths)))); return command; } } diff --git a/app/Commands/Airlock/EnableGlobalAirlock.cs b/app/Commands/Airlock/EnableGlobalAirlock.cs index 451b197..ab91db5 100644 --- a/app/Commands/Airlock/EnableGlobalAirlock.cs +++ b/app/Commands/Airlock/EnableGlobalAirlock.cs @@ -1,5 +1,4 @@ using System.CommandLine; -using CopilotHere.Infrastructure; namespace CopilotHere.Commands.Airlock; @@ -8,14 +7,9 @@ public sealed partial class AirlockCommands private static Command SetEnableGlobalAirlockCommand() { var command = new Command("--enable-global-airlock", "Enable Airlock with global rules (~/.config/copilot_here/network.json)"); - command.SetAction(_ => - { - var paths = AppPaths.Resolve(); - AirlockConfig.EnableGlobal(paths); - Console.WriteLine("✅ Airlock enabled (global)"); - Console.WriteLine($" 🌍 Rules: {AirlockConfig.GetGlobalRulesPath(paths)}"); - return 0; - }); + command.SetAction(_ => RunToggle( + "✅ Airlock enabled (global)", + paths => (AirlockConfig.EnableGlobal(paths), AirlockConfig.GetGlobalRulesPath(paths)))); return command; } } diff --git a/app/Commands/Airlock/NetworkConfig.cs b/app/Commands/Airlock/NetworkConfig.cs index d69ccea..5e8b3a6 100644 --- a/app/Commands/Airlock/NetworkConfig.cs +++ b/app/Commands/Airlock/NetworkConfig.cs @@ -51,10 +51,15 @@ public sealed class NetworkRule /// /// JSON source generator context for AOT-compatible serialization. /// +// network.json is a hand-edited file, so the reader tolerates comments and trailing +// commas. Anything that loads here must also survive a toggle, and the toggle's +// Utf8JsonReader is configured to match. [JsonSourceGenerationOptions( WriteIndented = true, PropertyNamingPolicy = JsonKnownNamingPolicy.SnakeCaseLower, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)] + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + ReadCommentHandling = JsonCommentHandling.Skip, + AllowTrailingCommas = true)] [JsonSerializable(typeof(NetworkConfig))] [JsonSerializable(typeof(NetworkRule))] [JsonSerializable(typeof(List))] diff --git a/app/Commands/Airlock/_AirlockCommands.cs b/app/Commands/Airlock/_AirlockCommands.cs index 465123a..0195411 100644 --- a/app/Commands/Airlock/_AirlockCommands.cs +++ b/app/Commands/Airlock/_AirlockCommands.cs @@ -1,4 +1,6 @@ using System.CommandLine; +using System.Text.Json; +using CopilotHere.Infrastructure; namespace CopilotHere.Commands.Airlock; @@ -17,4 +19,36 @@ public void Configure(RootCommand root) root.Add(SetEditAirlockRulesCommand()); root.Add(SetEditGlobalAirlockRulesCommand()); } + + /// + /// Runs an Airlock toggle and reports which file it wrote and how. + /// + /// Returns the outcome and the path it wrote. + private static int RunToggle(string successMessage, Func toggle) + { + var paths = AppPaths.Resolve(); + + try + { + var (outcome, path) = toggle(paths); + + Console.WriteLine(successMessage); + Console.WriteLine($" 📁 Rules: {path}"); + + if (outcome == AirlockToggleOutcome.SeededFromGlobal) + { + Console.WriteLine($" ↳ seeded from global config ({paths.GetGlobalPath("network.json")})"); + Console.WriteLine(" Local config replaces global entirely, so its rules were copied across."); + } + + return 0; + } + catch (JsonException ex) + { + Console.Error.WriteLine("❌ Could not update the Airlock config, because it isn't valid JSON."); + Console.Error.WriteLine($" {ex.Message}"); + Console.Error.WriteLine(" No changes were written."); + return 1; + } + } } diff --git a/app/Commands/Airlock/_AirlockConfig.cs b/app/Commands/Airlock/_AirlockConfig.cs index 18da7be..4f939e7 100644 --- a/app/Commands/Airlock/_AirlockConfig.cs +++ b/app/Commands/Airlock/_AirlockConfig.cs @@ -1,3 +1,4 @@ +using System.Text; using System.Text.Json; using CopilotHere.Infrastructure; @@ -24,6 +25,18 @@ public sealed record AirlockConfig private const string RulesFileName = "network.json"; + /// UTF-8 BOM. Some Windows editors add one; it must survive a rewrite. + private static ReadOnlySpan Utf8Bom => [0xEF, 0xBB, 0xBF]; + + /// + /// Matches the deserializer's leniency, so any file that loads can also be toggled. + /// + private static readonly JsonReaderOptions ReaderOptions = new() + { + CommentHandling = JsonCommentHandling.Skip, + AllowTrailingCommas = true + }; + /// /// Loads Airlock configuration from config files. /// The enabled flag is read from within the network.json file. @@ -93,45 +106,202 @@ internal static void WriteNetworkConfig(string path, NetworkConfig config) } /// Enables Airlock in local config by setting enabled:true in network.json. - public static void EnableLocal(AppPaths paths) + public static AirlockToggleOutcome EnableLocal(AppPaths paths) => SetEnabledLocal(paths, true); + + /// Enables Airlock in global config by setting enabled:true in network.json. + public static AirlockToggleOutcome EnableGlobal(AppPaths paths) => SetEnabledGlobal(paths, true); + + /// Disables Airlock in local config by setting enabled:false in network.json. + public static AirlockToggleOutcome DisableLocal(AppPaths paths) => SetEnabledLocal(paths, false); + + /// Disables Airlock in global config by setting enabled:false in network.json. + public static AirlockToggleOutcome DisableGlobal(AppPaths paths) => SetEnabledGlobal(paths, false); + + private static AirlockToggleOutcome SetEnabledLocal(AppPaths paths, bool enabled) => + SetEnabledInJson(paths.GetLocalPath(RulesFileName), enabled, seedFrom: paths.GetGlobalPath(RulesFileName)); + + private static AirlockToggleOutcome SetEnabledGlobal(AppPaths paths, bool enabled) => + SetEnabledInJson(paths.GetGlobalPath(RulesFileName), enabled, seedFrom: null); + + /// + /// Sets the enabled flag in a network.json file, leaving every other byte of the + /// file alone. Comments, key order, formatting and keys this app doesn't model all + /// survive, because a rules file people hand-edit shouldn't be reformatted under them. + /// + /// + /// Config file to copy when doesn't exist yet. A local file + /// shadows the global one entirely (see ), so creating an empty + /// local file would silently drop the user out of their global ruleset. + /// + private static AirlockToggleOutcome SetEnabledInJson(string path, bool enabled, string? seedFrom) { - SetEnabledInJson(paths.GetLocalPath(RulesFileName), true); + var dir = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(dir)) + Directory.CreateDirectory(dir); + + if (File.Exists(path)) + { + WriteEnabled(path, readFrom: path, enabled); + return AirlockToggleOutcome.UpdatedExisting; + } + + if (!string.IsNullOrEmpty(seedFrom) && File.Exists(seedFrom)) + { + WriteEnabled(path, readFrom: seedFrom, enabled); + return AirlockToggleOutcome.SeededFromGlobal; + } + + WriteNetworkConfig(path, NetworkConfig.CreateDefault(enabled)); + return AirlockToggleOutcome.CreatedDefault; } - /// Enables Airlock in global config by setting enabled:true in network.json. - public static void EnableGlobal(AppPaths paths) + /// + /// Rewrites 's bytes with the new enabled flag and saves + /// them to . The whole edit happens in memory first, so a file + /// that fails to parse is never partially written. When the two paths differ, a broken + /// source leaves no new file behind at all. + /// + private static void WriteEnabled(string path, string readFrom, bool enabled) { - SetEnabledInJson(paths.GetGlobalPath(RulesFileName), true); + byte[] updated; + try + { + updated = SetEnabledInJsonBytes(File.ReadAllBytes(readFrom), enabled); + } + catch (JsonException ex) + { + throw new JsonException($"{readFrom}: {ex.Message}", ex); + } + + File.WriteAllBytes(path, updated); } - /// Disables Airlock in local config by setting enabled:false in network.json. - public static void DisableLocal(AppPaths paths) + /// + /// Returns with the root object's "enabled" value replaced, + /// inserting the property if it isn't there. Every other byte is copied verbatim. + /// + internal static byte[] SetEnabledInJsonBytes(byte[] json, bool enabled) + { + var bomLength = json.AsSpan().StartsWith(Utf8Bom) ? Utf8Bom.Length : 0; + var body = json.AsSpan(bomLength); + + // The search below stops as soon as it finds "enabled". Without a full pass first, + // a file that is broken further down would still get rewritten, handing the user + // back a config that won't load. + ValidateJson(body); + + ReadOnlySpan replacement = enabled ? "true"u8 : "false"u8; + + var existing = FindRootEnabledValue(body); + if (existing is var (start, length)) + return Splice(json, bomLength + start, length, replacement); + + return InsertRootEnabled(json, bomLength, replacement); + } + + /// Throws JsonException if the document isn't well-formed. + private static void ValidateJson(ReadOnlySpan body) { - SetEnabledInJson(paths.GetLocalPath(RulesFileName), false); + var reader = new Utf8JsonReader(body, ReaderOptions); + while (reader.Read()) + { + } } - /// Disables Airlock in global config by setting enabled:false in network.json. - public static void DisableGlobal(AppPaths paths) + /// + /// Locates the value of the root object's "enabled" property. + /// Offsets are relative to . Returns null when absent. + /// + private static (int Start, int Length)? FindRootEnabledValue(ReadOnlySpan body) { - SetEnabledInJson(paths.GetGlobalPath(RulesFileName), false); + var reader = new Utf8JsonReader(body, ReaderOptions); + + while (reader.Read()) + { + // Depth 1 is a property of the root object. Without this check, a host or + // path containing "enabled" inside allowed_rules would match instead. + if (reader.TokenType != JsonTokenType.PropertyName || reader.CurrentDepth != 1) + continue; + + if (!reader.ValueTextEquals("enabled"u8)) + continue; + + if (!reader.Read()) + break; + + if (reader.TokenType is JsonTokenType.StartObject or JsonTokenType.StartArray) + { + throw new JsonException( + "The \"enabled\" property must be a boolean, but it holds an object or array."); + } + + var start = (int)reader.TokenStartIndex; + return (start, (int)reader.BytesConsumed - start); + } + + return null; } - /// Sets the enabled flag in a network.json file. - private static void SetEnabledInJson(string path, bool enabled) + /// + /// Adds an "enabled" property as the first entry of the root object, matching the + /// file's existing newline style and indentation. + /// + private static byte[] InsertRootEnabled(byte[] json, int bomLength, ReadOnlySpan value) { - NetworkConfig config; + var body = json.AsSpan(bomLength); + var reader = new Utf8JsonReader(body, ReaderOptions); - if (File.Exists(path)) + if (!reader.Read() || reader.TokenType != JsonTokenType.StartObject) + throw new JsonException("network.json must have a JSON object at its root."); + + var afterBrace = (int)reader.BytesConsumed; + + if (!reader.Read()) + throw new JsonException("network.json must have a JSON object at its root."); + + var newline = body.IndexOf((byte)'\r') >= 0 ? "\r\n" : "\n"; + var nextTokenStart = (int)reader.TokenStartIndex; + var indent = DetectIndent(body, nextTokenStart); + + // An empty root object has no property to sit above, so the insert has to + // supply the closing brace's own line as well. + if (reader.TokenType == JsonTokenType.EndObject) { - config = ReadNetworkConfig(path) ?? NetworkConfig.CreateDefault(enabled); - config.Enabled = enabled; + var text = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)}{newline}"; + return Splice(json, bomLength + afterBrace, nextTokenStart - afterBrace, Encoding.UTF8.GetBytes(text)); } - else + + var inserted = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)},"; + return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(inserted)); + } + + /// + /// Reads the whitespace at the start of the line holding , + /// so an inserted property lines up with its siblings. Falls back to two spaces. + /// + private static string DetectIndent(ReadOnlySpan body, int tokenStart) + { + var lineStart = tokenStart; + while (lineStart > 0 && body[lineStart - 1] is not ((byte)'\n' or (byte)'\r')) + lineStart--; + + var indent = body[lineStart..tokenStart]; + foreach (var b in indent) { - config = NetworkConfig.CreateDefault(enabled); + if (b is not ((byte)' ' or (byte)'\t')) + return " "; } - WriteNetworkConfig(path, config); + return indent.IsEmpty ? " " : Encoding.UTF8.GetString(indent); + } + + private static byte[] Splice(byte[] source, int start, int length, ReadOnlySpan replacement) + { + var result = new byte[source.Length - length + replacement.Length]; + source.AsSpan(0, start).CopyTo(result); + replacement.CopyTo(result.AsSpan(start)); + source.AsSpan(start + length).CopyTo(result.AsSpan(start + replacement.Length)); + return result; } /// Gets the path to the local rules file (creates dir if needed). @@ -161,3 +331,16 @@ public enum AirlockConfigSource Global, Local } + +/// What a toggle did to the config file, so commands can say so. +public enum AirlockToggleOutcome +{ + /// The file already existed and only its enabled flag changed. + UpdatedExisting, + + /// A new local file was created from the global one, carrying its rules across. + SeededFromGlobal, + + /// No config existed anywhere, so a default one was written. + CreatedDefault +} diff --git a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs index eea2327..2c5a99b 100644 --- a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs +++ b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using CopilotHere.Commands.Airlock; using CopilotHere.Infrastructure; using TUnit.Core; @@ -182,4 +183,228 @@ public async Task ReadNetworkConfig_NonexistentFile_ReturnsNull() // Assert await Assert.That(config).IsNull(); } + + [Test] + public async Task Toggle_KeepsUnknownKeysAndFormatting() + { + // Arrange + var localRulesPath = _paths.GetLocalPath("network.json"); + const string original = """ + { + "enabled": true, + "mode": "monitor", + "allowed_rules": [ + { + "host": "api.nuget.org", + "allowed_paths": ["/v3/*"], + "note": "a key copilot_here doesn't model" + } + ], + "my_custom_top_level": { "a": 1 } + } + """; + File.WriteAllText(localRulesPath, original); + + // Act + AirlockConfig.DisableLocal(_paths); + AirlockConfig.EnableLocal(_paths); + + // Assert - back to the original byte for byte + await Assert.That(File.ReadAllText(localRulesPath)).IsEqualTo(original); + } + + [Test] + public async Task Toggle_KeepsComments() + { + // Arrange + var localRulesPath = _paths.GetLocalPath("network.json"); + const string original = """ + { + // nuget is needed for restore + "enabled": true, + "allowed_rules": [ { "host": "api.nuget.org", "allowed_paths": ["*"] } ] + } + """; + File.WriteAllText(localRulesPath, original); + + // Act + AirlockConfig.DisableLocal(_paths); + + // Assert + await Assert.That(File.ReadAllText(localRulesPath)) + .IsEqualTo(original.Replace("\"enabled\": true", "\"enabled\": false")); + } + + [Test] + public async Task Toggle_IgnoresEnabledNestedInsideARule() + { + // Arrange + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """ + { + "allowed_rules": [ + { "host": "a.com", "enabled": false, "allowed_paths": ["*"] } + ], + "enabled": false + } + """); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert - only the root flag flipped + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).Contains("\"host\": \"a.com\", \"enabled\": false"); + await Assert.That(updated).Contains("\"enabled\": true"); + } + + [Test] + public async Task Toggle_InsertsEnabledWhenMissing() + { + // Arrange + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """ + { + "mode": "monitor", + "allowed_rules": [] + } + """); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).Contains("\"enabled\": true"); + await Assert.That(updated).Contains("\"mode\": \"monitor\""); + await Assert.That(AirlockConfig.Load(_paths).Enabled).IsTrue(); + } + + [Test] + public async Task Toggle_MalformedJson_ThrowsAndLeavesFileAlone() + { + // Arrange + var localRulesPath = _paths.GetLocalPath("network.json"); + const string broken = """{ "enabled": true, "allowed_rules": [ }"""; + File.WriteAllText(localRulesPath, broken); + + // Act & Assert + await Assert.That(() => AirlockConfig.DisableLocal(_paths)).Throws(); + await Assert.That(File.ReadAllText(localRulesPath)).IsEqualTo(broken); + } + + [Test] + public async Task DisableLocal_NoLocalFile_SeedsFromGlobalRules() + { + // Arrange - rules live globally only, with nothing in the project yet + var globalRulesPath = _paths.GetGlobalPath("network.json"); + File.WriteAllText(globalRulesPath, """ + { + "enabled": true, + "mode": "monitor", + "allowed_rules": [ + { "host": "api.nuget.org", "allowed_paths": ["*"] }, + { "host": "registry.npmjs.org", "allowed_paths": ["*"] } + ] + } + """); + + // Act + var outcome = AirlockConfig.DisableLocal(_paths); + + // Assert - the global rules came across instead of being shadowed by an empty file + await Assert.That(outcome).IsEqualTo(AirlockToggleOutcome.SeededFromGlobal); + + var local = AirlockConfig.ReadNetworkConfig(_paths.GetLocalPath("network.json")); + await Assert.That(local!.Enabled).IsFalse(); + await Assert.That(local.Mode).IsEqualTo("monitor"); + await Assert.That(local.AllowedRules.Count).IsEqualTo(2); + await Assert.That(local.AllowedRules[0].Host).IsEqualTo("api.nuget.org"); + } + + [Test] + public async Task EnableLocal_NoLocalFile_SeedsFromGlobalRules() + { + // Arrange + var globalRulesPath = _paths.GetGlobalPath("network.json"); + File.WriteAllText(globalRulesPath, """ + { + "enabled": false, + "allowed_rules": [ { "host": "api.nuget.org", "allowed_paths": ["*"] } ] + } + """); + + // Act + var outcome = AirlockConfig.EnableLocal(_paths); + + // Assert + await Assert.That(outcome).IsEqualTo(AirlockToggleOutcome.SeededFromGlobal); + + var config = AirlockConfig.Load(_paths); + await Assert.That(config.Enabled).IsTrue(); + await Assert.That(config.EnabledSource).IsEqualTo(AirlockConfigSource.Local); + + var local = AirlockConfig.ReadNetworkConfig(_paths.GetLocalPath("network.json")); + await Assert.That(local!.AllowedRules.Count).IsEqualTo(1); + } + + [Test] + public async Task DisableLocal_BrokenGlobalFile_CreatesNoLocalFile() + { + // Arrange + File.WriteAllText(_paths.GetGlobalPath("network.json"), """{ "enabled": true, ]"""); + + // Act & Assert - better to fail loudly than seed a project from a broken source + await Assert.That(() => AirlockConfig.DisableLocal(_paths)).Throws(); + await Assert.That(File.Exists(_paths.GetLocalPath("network.json"))).IsFalse(); + } + + [Test] + public async Task DisableLocal_NoConfigAnywhere_WritesDefault() + { + // Act + var outcome = AirlockConfig.DisableLocal(_paths); + + // Assert + await Assert.That(outcome).IsEqualTo(AirlockToggleOutcome.CreatedDefault); + + var local = AirlockConfig.ReadNetworkConfig(_paths.GetLocalPath("network.json")); + await Assert.That(local!.Enabled).IsFalse(); + await Assert.That(local.AllowedRules.Count).IsEqualTo(0); + } + + [Test] + public async Task EnableLocal_ExistingLocalFile_ReportsUpdatedExisting() + { + // Arrange + File.WriteAllText(_paths.GetLocalPath("network.json"), """{ "enabled": false }"""); + + // Act + var outcome = AirlockConfig.EnableLocal(_paths); + + // Assert + await Assert.That(outcome).IsEqualTo(AirlockToggleOutcome.UpdatedExisting); + await Assert.That(AirlockConfig.Load(_paths).Enabled).IsTrue(); + } + + [Test] + public async Task Load_FileWithCommentsAndTrailingCommas_Reads() + { + // Arrange + File.WriteAllText(_paths.GetLocalPath("network.json"), """ + { + // hand-edited configs happen + "enabled": true, + "allowed_rules": [ + { "host": "api.nuget.org", "allowed_paths": ["*"] }, + ], + } + """); + + // Act + var config = AirlockConfig.Load(_paths); + + // Assert + await Assert.That(config.Enabled).IsTrue(); + } } From 1dd61f991b5e9d57af0b183e8c10fac018209849 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 02:08:29 +1000 Subject: [PATCH 2/3] fix(airlock): address round-1 bot review findings Address Copilot + Codex feedback on PR #133 round 1: - FindRootEnabledValue returned the first root "enabled" occurrence, but System.Text.Json's deserializer (used by Load) resolves duplicate keys to the last one. A hand-edited file with a duplicate root "enabled" could report success while Load() still saw the stale value. Keep scanning and splice the occurrence Load() will actually read. - InsertRootEnabled's empty-object branch replaced every byte between the braces, including a comment-only object's comment, since JsonCommentHandling.Skip doesn't surface comments as tokens. Only use the whole-range replace for a truly empty `{}`; otherwise insert ahead of the existing content so comments (or any other whitespace) survive. - Same method always inserted a newline+indent even when the file had no newline anywhere, reformatting a deliberately single-line network.json onto multiple lines. Detect the no-newline case and insert inline instead. - RunToggle's JsonException handler claimed the file "isn't valid JSON" even when the JSON was syntactically valid but shaped wrong (e.g. "enabled" holding an object). Drop the specific claim and let ex.Message carry the actual reason. --- app/Commands/Airlock/_AirlockCommands.cs | 2 +- app/Commands/Airlock/_AirlockConfig.cs | 36 ++++++++++--- .../AirlockConfigTests.cs | 53 +++++++++++++++++++ 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/app/Commands/Airlock/_AirlockCommands.cs b/app/Commands/Airlock/_AirlockCommands.cs index 0195411..bf0dba6 100644 --- a/app/Commands/Airlock/_AirlockCommands.cs +++ b/app/Commands/Airlock/_AirlockCommands.cs @@ -45,7 +45,7 @@ private static int RunToggle(string successMessage, Func body) private static (int Start, int Length)? FindRootEnabledValue(ReadOnlySpan body) { var reader = new Utf8JsonReader(body, ReaderOptions); + (int Start, int Length)? match = null; while (reader.Read()) { @@ -236,10 +237,15 @@ private static (int Start, int Length)? FindRootEnabledValue(ReadOnlySpan } var start = (int)reader.TokenStartIndex; - return (start, (int)reader.BytesConsumed - start); + match = (start, (int)reader.BytesConsumed - start); + + // Keep scanning instead of returning here: a hand-edited file can carry a + // duplicate root "enabled" key, and System.Text.Json's deserializer resolves + // that to the last occurrence, so the splice has to target the same one + // Load() will actually read - otherwise the two disagree after a toggle. } - return null; + return match; } /// @@ -259,16 +265,32 @@ private static byte[] InsertRootEnabled(byte[] json, int bomLength, ReadOnlySpan if (!reader.Read()) throw new JsonException("network.json must have a JSON object at its root."); - var newline = body.IndexOf((byte)'\r') >= 0 ? "\r\n" : "\n"; var nextTokenStart = (int)reader.TokenStartIndex; - var indent = DetectIndent(body, nextTokenStart); + + // A file with no newline anywhere is deliberately single-line; inserting our + // usual newline+indent would reformat it onto multiple lines for no reason. + string newline; + string indent; + if (body.IndexOf((byte)'\n') >= 0 || body.IndexOf((byte)'\r') >= 0) + { + newline = body.IndexOf((byte)'\r') >= 0 ? "\r\n" : "\n"; + indent = DetectIndent(body, nextTokenStart); + } + else + { + newline = ""; + indent = " "; + } // An empty root object has no property to sit above, so the insert has to - // supply the closing brace's own line as well. - if (reader.TokenType == JsonTokenType.EndObject) + // supply the closing brace's own line as well - but only when it's truly + // empty. A root object holding nothing but comments still lands here (the + // comments aren't tokens), and replacing the byte range between the braces + // would delete them; inserting ahead of that range instead keeps them intact. + if (reader.TokenType == JsonTokenType.EndObject && nextTokenStart == afterBrace) { var text = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)}{newline}"; - return Splice(json, bomLength + afterBrace, nextTokenStart - afterBrace, Encoding.UTF8.GetBytes(text)); + return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(text)); } var inserted = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)},"; diff --git a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs index 2c5a99b..6efafe8 100644 --- a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs +++ b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs @@ -387,6 +387,59 @@ public async Task EnableLocal_ExistingLocalFile_ReportsUpdatedExisting() await Assert.That(AirlockConfig.Load(_paths).Enabled).IsTrue(); } + [Test] + public async Task Toggle_InsertsEnabledPreservesCommentInOtherwiseEmptyObject() + { + // Arrange - the root object has no real properties, only a comment + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """ + { + // explanation + } + """); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert - the comment survives alongside the inserted property + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).Contains("// explanation"); + await Assert.That(updated).Contains("\"enabled\": true"); + } + + [Test] + public async Task Toggle_InsertsEnabledOnSingleLineFile_StaysSingleLine() + { + // Arrange - a file with no newlines anywhere is deliberately single-line + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """{ "mode": "monitor" }"""); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert - stays on one line instead of being reformatted + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).DoesNotContain("\n"); + await Assert.That(updated).Contains("\"enabled\": true"); + await Assert.That(updated).Contains("\"mode\": \"monitor\""); + } + + [Test] + public async Task Toggle_DuplicateRootEnabled_UpdatesTheOccurrenceLoadWillRead() + { + // Arrange - a hand-edited duplicate key. System.Text.Json's deserializer + // resolves duplicates to the last occurrence, so the splice must target + // that one or Load() would disagree with what the toggle just reported. + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """{ "enabled": false, "enabled": false }"""); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert + await Assert.That(AirlockConfig.Load(_paths).Enabled).IsTrue(); + } + [Test] public async Task Load_FileWithCommentsAndTrailingCommas_Reads() { From 402cd22891b6be46bc7f908ad08926d956ea33b3 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 02:13:47 +1000 Subject: [PATCH 3/3] fix(airlock): stop the empty-object insert from emitting a trailing comma The round-1 fix for comment/whitespace preservation in InsertRootEnabled's EndObject branch narrowed the "replace the whole gap" path to only the truly-empty {} case, but let every other EndObject case fall through to the insert-with-comma path. That path is only correct when a real property follows the insertion point - an EndObject means nothing does, so the comma made the file strict-JSON-invalid (our own AllowTrailingCommas=true reader tolerated it, which is exactly why the suite didn't catch it). Split the EndObject branch three ways instead: truly empty (synthesize the whole line, no comma), whitespace/comments only (insert ahead of the content, no comma), and a real property follows (insert with comma, unchanged). Tests now parse every insert-path result with AllowTrailingCommas=false so a malformed splice can't hide behind our own reader's leniency again. Added cases for a truly-empty {} and a whitespace-only {\n} object - the latter is the one that actually regressed and had no prior coverage. Also added a test distinguishing the shape-error path (enabled holding an object) from a syntax error, closing the gap the error-message fix didn't have a test for yet. --- app/Commands/Airlock/_AirlockConfig.cs | 28 +++++-- .../AirlockConfigTests.cs | 76 ++++++++++++++++++- 2 files changed, 95 insertions(+), 9 deletions(-) diff --git a/app/Commands/Airlock/_AirlockConfig.cs b/app/Commands/Airlock/_AirlockConfig.cs index bd05bb1..8a9e3c7 100644 --- a/app/Commands/Airlock/_AirlockConfig.cs +++ b/app/Commands/Airlock/_AirlockConfig.cs @@ -282,17 +282,29 @@ private static byte[] InsertRootEnabled(byte[] json, int bomLength, ReadOnlySpan indent = " "; } - // An empty root object has no property to sit above, so the insert has to - // supply the closing brace's own line as well - but only when it's truly - // empty. A root object holding nothing but comments still lands here (the - // comments aren't tokens), and replacing the byte range between the braces - // would delete them; inserting ahead of that range instead keeps them intact. - if (reader.TokenType == JsonTokenType.EndObject && nextTokenStart == afterBrace) + // EndObject means no property follows, so there's never a comma to add. + // A root object holding nothing but comments still lands here too (the + // comments aren't tokens), which is why the two sub-cases below insert + // ahead of any existing content rather than replacing it outright. + if (reader.TokenType == JsonTokenType.EndObject) { - var text = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)}{newline}"; - return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(text)); + if (nextTokenStart == afterBrace) + { + // Truly empty root object - nothing at all between the braces, not even + // whitespace - so synthesize the whole line ourselves. + var text = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)}{newline}"; + return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(text)); + } + + // The object holds only whitespace or comments - insert ahead of that + // content so it survives. No trailing comma: nothing follows the new + // property, and one here would make the file strict-JSON-invalid even + // though our own lenient reader would tolerate it. + var noFollowingProperty = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)}"; + return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(noFollowingProperty)); } + // A real property follows, so the comma is required. var inserted = $"{newline}{indent}\"enabled\": {Encoding.UTF8.GetString(value)},"; return Splice(json, bomLength + afterBrace, 0, Encoding.UTF8.GetBytes(inserted)); } diff --git a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs index 6efafe8..cb1657f 100644 --- a/tests/CopilotHere.UnitTests/AirlockConfigTests.cs +++ b/tests/CopilotHere.UnitTests/AirlockConfigTests.cs @@ -293,6 +293,31 @@ public async Task Toggle_MalformedJson_ThrowsAndLeavesFileAlone() await Assert.That(File.ReadAllText(localRulesPath)).IsEqualTo(broken); } + [Test] + public async Task Toggle_EnabledHoldsObject_ThrowsShapeError_NotSyntaxError() + { + // Arrange - structurally valid JSON, but "enabled" holds the wrong shape. + // Distinguishing this from a syntax error is what lets RunToggle's message + // avoid the wrong claim that the file "isn't valid JSON". + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, """{ "enabled": {} }"""); + + // Act + JsonException? caught = null; + try + { + AirlockConfig.EnableLocal(_paths); + } + catch (JsonException ex) + { + caught = ex; + } + + // Assert + await Assert.That(caught).IsNotNull(); + await Assert.That(caught!.Message).Contains("must be a boolean"); + } + [Test] public async Task DisableLocal_NoLocalFile_SeedsFromGlobalRules() { @@ -401,10 +426,48 @@ public async Task Toggle_InsertsEnabledPreservesCommentInOtherwiseEmptyObject() // Act AirlockConfig.EnableLocal(_paths); - // Assert - the comment survives alongside the inserted property + // Assert - the comment survives alongside the inserted property, and the + // result is well-formed strict JSON (no trailing comma left dangling + // ahead of a comment with nothing else following it) var updated = File.ReadAllText(localRulesPath); await Assert.That(updated).Contains("// explanation"); await Assert.That(updated).Contains("\"enabled\": true"); + AssertWellFormedStrictJson(updated); + } + + [Test] + public async Task Toggle_InsertsEnabledOnEmptyObject_StaysWellFormed() + { + // Arrange - truly empty: nothing at all between the braces + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, "{}"); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).Contains("\"enabled\": true"); + AssertWellFormedStrictJson(updated); + } + + [Test] + public async Task Toggle_InsertsEnabledOnWhitespaceOnlyEmptyObject_StaysWellFormed() + { + // Arrange - empty object spread across lines, no comment, just whitespace. + // This is the case that regressed: the insert has nothing following it, + // so a trailing comma here would make the file strict-JSON-invalid even + // though our own lenient reader tolerates it. + var localRulesPath = _paths.GetLocalPath("network.json"); + File.WriteAllText(localRulesPath, "{\n}"); + + // Act + AirlockConfig.EnableLocal(_paths); + + // Assert + var updated = File.ReadAllText(localRulesPath); + await Assert.That(updated).Contains("\"enabled\": true"); + AssertWellFormedStrictJson(updated); } [Test] @@ -422,6 +485,17 @@ public async Task Toggle_InsertsEnabledOnSingleLineFile_StaysSingleLine() await Assert.That(updated).DoesNotContain("\n"); await Assert.That(updated).Contains("\"enabled\": true"); await Assert.That(updated).Contains("\"mode\": \"monitor\""); + AssertWellFormedStrictJson(updated); + } + + /// + /// Parses with comments allowed but trailing commas rejected, so a splice that + /// only happens to be readable by our own lenient reader still fails the test. + /// + private static void AssertWellFormedStrictJson(string json) + { + var options = new JsonDocumentOptions { CommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = false }; + using var _ = JsonDocument.Parse(json, options); } [Test]