fix(airlock): stop enable/disable rewriting network.json - #133
Conversation
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
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
@coderabbitai review |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75977addc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes Airlock enable/disable behavior so network.json is no longer round-tripped through a typed model (which previously dropped comments/formatting/unknown keys), and prevents --disable-airlock from accidentally shadowing global rules with an empty local file.
Changes:
- Update Airlock toggling to splice/insert the root
"enabled"value in-place usingUtf8JsonReaderoffsets (preserving comments, ordering, indentation, unknown keys, and BOM; validating JSON first). - Seed newly-created local
.copilot_here/network.jsonfrom global~/.config/copilot_here/network.jsonand surface an outcome so the CLI can report what happened. - Add targeted unit tests for preservation, comment handling, nested
enabled, missingenabled, malformed JSON, and global-to-local seeding; update README to document the behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CopilotHere.UnitTests/AirlockConfigTests.cs | Adds new unit tests covering toggle preservation, malformed JSON handling, and global-to-local seeding outcomes. |
| README.md | Documents non-merge behavior, seeding from global on first local creation, and preservation guarantees during toggles. |
| app/Commands/Airlock/NetworkConfig.cs | Aligns JSON source-gen options with hand-edited config reality (comments + trailing commas). |
| app/Commands/Airlock/_AirlockConfig.cs | Implements byte-preserving toggle logic, seeding behavior, and introduces AirlockToggleOutcome. |
| app/Commands/Airlock/_AirlockCommands.cs | Adds shared toggle runner with consistent success/error reporting and seeded-from-global messaging. |
| app/Commands/Airlock/EnableAirlock.cs | Switches to shared toggle runner and reports rules path. |
| app/Commands/Airlock/DisableAirlock.cs | Switches to shared toggle runner and reports rules path. |
| app/Commands/Airlock/EnableGlobalAirlock.cs | Switches to shared toggle runner and reports rules path. |
| app/Commands/Airlock/DisableGlobalAirlock.cs | Switches to shared toggle runner and reports rules path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dd61f991b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…omma
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
app/Commands/Airlock/_AirlockConfig.cs:277
- Newline detection assumes that the presence of any '\r' implies CRLF ("\r\n"). For files using lone CR line endings (legacy Mac style), this will insert "\r\n" and won’t preserve the existing newline style as intended. Consider detecting CRLF vs CR vs LF based on the first newline sequence encountered.
if (body.IndexOf((byte)'\n') >= 0 || body.IndexOf((byte)'\r') >= 0)
{
newline = body.IndexOf((byte)'\r') >= 0 ? "\r\n" : "\n";
indent = DetectIndent(body, nextTokenStart);
app/Commands/Airlock/_AirlockConfig.cs:197
if (existing is var (start, length))is a subtle pattern to read correctly with a nullable tuple return type; it can look like it would always match. Using an explicit nullable tuple pattern (orexisting.HasValue) would make the null behavior obvious and reduce the chance of future regressions during refactors.
var existing = FindRootEnabledValue(body);
if (existing is var (start, length))
return Splice(json, bomLength + start, length, replacement);
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
app/Commands/Airlock/_AirlockConfig.cs:140
- SetEnabledInJson creates the target directory before attempting to parse the source JSON. If the operation fails (e.g., seeding from a malformed global config), this can still create a new
.copilot_here/directory even though the command reports that no changes were written. Consider deferring directory creation until after JSON parsing succeeds (e.g., create the directory immediately before writing the output file).
var dir = Path.GetDirectoryName(path);
if (!string.IsNullOrEmpty(dir))
Directory.CreateDirectory(dir);
app/Commands/Airlock/_AirlockConfig.cs:199
- The nullable tuple check
if (existing is var (start, length))is hard to read and can be misinterpreted as an always-truevarpattern. Using an explicit nullable-tuple positional pattern (or an explicit null check) makes it clearer that the splice only happens when an existing rootenabledvalue was found.
var existing = FindRootEnabledValue(body);
if (existing is var (start, length))
return Splice(json, bomLength + start, length, replacement);
Summary
Toggling airlock destroyed config in two separate ways, both reported in #129.
The toggle rewrote the whole file.
SetEnabledInJsondeserializednetwork.jsonintoNetworkConfig, flippedEnabled, and serialized it back over the top, so comments, key order, indentation and any key the class doesn't model were lost. A file containing JSON comments threw an unhandledJsonExceptioninstead. This came in with c0c1a83, which replaced a surgicaltrue/falseswap with a typed round-trip.The
enabledvalue is now spliced in place usingUtf8JsonReadertoken offsets. ACurrentDepth == 1check stops anenablednested inside a rule from matching, which the old string-scanning code couldn't do, and the document is validated up front so a file broken further down isn't rewritten into a half-fixed state.Separately,
--disable-airlocksilently emptied a project's ruleset. With rules only in~/.config/copilot_here/network.json, running it inside a project created.copilot_here/network.jsonwithallowed_rules: []. Local config replaces global outright, so re-enabling left the project running with no rules at all and nothing in the output said so. Creating a local file now seeds it from the global one, and the command reports that it did.Bad JSON also names the file and exits 1 rather than throwing a stack trace, and the reader accepts comments and trailing commas to match how people actually edit the file.
Test plan
origin/mainplus this branch. 12 are new: round-trip preservation, comments, anenablednested inside a rule, a missingenabledkey, an empty root object, malformed JSON, and the global-to-local seeding.--disable-airlockfollowed by--enable-airlock:diffreports it byte-identical to the original.--disable-airlockinside a project. The new local file carries both global rules instead of[].dotnet publish -c Release -r osx-arm64clean, no trim or AOT warnings from the changed code.Closes #129