Add GitIgnoreEnsurer to proactively protect managed agent folders - #7
Merged
Merged
Conversation
Adds a new GitIgnoreEnsurer unit (RepoSync subsystem) that ensures a repo's .gitignore covers the four managed agent-package folders (.github/agents, .github/standards, .github/templates, .github/skills) immediately after a successful package extraction, before any commit can occur. - Idempotency is a marker-comment scan only (no git check-ignore, no glob analysis) - purely additive, never edits/removes existing .gitignore lines. - Wired into RepoCardViewModel right after PackageZipExtractor.Extract in both the "Select Package..." and "Upgrade" flows, wrapped in a non-blocking try/catch so I/O failures never abort extraction. - Complements (does not replace) the existing advisory "Committed agent files" badge, which remains unchanged. - Adds companion tests, design/verification docs, reqstream requirements, and SysML2 model updates for the new unit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is additive, well-covered by tests and companion artifacts, and introduces no blocking correctness or safety issues.
Pull request overview
This PR adds a proactive .gitignore protection step to the RepoSync flow so the four managed agent-package folders (.github/agents, .github/standards, .github/templates, .github/skills) are less likely to be accidentally committed immediately after extraction.
Changes:
- Introduces
GitIgnoreEnsurer(RepoSync) to append a marker-delimited.gitignoreblock for the managed folders (additive-only, marker-scan idempotency). - Wires
.gitignoreensuring intoRepoCardViewModelright after successful package extraction (non-blocking warning on failure). - Adds unit tests and updates requirements, SysML2 model, design/verification docs, and review metadata to cover the new unit and behavior.
File summaries
| File | Description |
|---|---|
| test/DemaConsulting.AgentControl.Tests/RepoSync/GitIgnoreEnsurerTests.cs | Adds unit tests for marker-scan idempotency, file creation/appending, and I/O failure wrapping. |
| test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs | Adds coverage ensuring .gitignore is updated after Upgrade/Select Package, and that .gitignore failures are non-blocking. |
| src/DemaConsulting.AgentControl/RepoSync/GitIgnoreEnsurer.cs | Implements additive .gitignore update with marker-based idempotency and I/O exception wrapping. |
| src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs | Calls GitIgnoreEnsurer after extraction via a dedicated non-blocking wrapper. |
| requirements.yaml | Includes the new GitIgnoreEnsurer requirements file in the root requirements set. |
| README.md | Documents the new proactive .gitignore protection alongside existing badges/behavior. |
| docs/verification/agent-control/repo-sync/git-ignore-ensurer.md | Adds unit-level verification design for GitIgnoreEnsurer and its acceptance criteria. |
| docs/verification/agent-control/repo-sync.md | Updates RepoSync verification approach/criteria to include .gitignore prevention behavior and tests. |
| docs/user_guide/introduction.md | Updates user guide to describe proactive .gitignore ensuring in Select Package/Upgrade flows. |
| docs/sysml2/model/agent-control/repo-sync/git-ignore-ensurer.sysml | Adds SysML2 element tying source/test/design/verification/requirements for GitIgnoreEnsurer. |
| docs/sysml2/model/agent-control/repo-sync.sysml | Updates RepoSync model to include GitIgnoreEnsurer as a subsystem part and describe behavior. |
| docs/reqstream/agent-control/repo-sync/git-ignore-ensurer.yaml | Adds unit requirements for marker-based additive .gitignore enforcement. |
| docs/reqstream/agent-control/repo-sync.yaml | Adds subsystem-level requirement for post-sync .gitignore coverage and links to unit requirement/tests. |
| docs/design/agent-control/repo-sync/git-ignore-ensurer.md | Adds design doc describing purpose, data model, behavior, error handling, and callers. |
| docs/design/agent-control/repo-sync.md | Updates subsystem design to include GitIgnoreEnsurer interface/contract and rationale for placement. |
| docs/design/agent-control/launcher-ui/repo-card-view-model.md | Documents the new non-blocking ensure step in the shared extract-and-pin sequence and error handling. |
| architecture.md | Notes the new proactive .gitignore ensure step as additive to the advisory committed-files badge. |
| .reviewmark.yaml | Adds a ReviewMark review-set for the new GitIgnoreEnsurer unit and companion artifacts. |
| .cspell.yaml | Adds “unreordered” to the repository dictionary to support new documentation/test wording. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ped error message - GitIgnoreEnsurer now detects and reuses the existing .gitignore file's newline style (CRLF vs LF) instead of always using Environment.NewLine, avoiding mixed line endings when appending on a different OS than the file was originally created on. - RepoCardViewModel.EnsureGitIgnoreCoversManagedFolders now surfaces GitIgnoreEnsurer's exception message directly instead of re-wrapping it with a redundant "Failed to update .gitignore..." prefix. - Updated GitIgnoreEnsurerTests to assert the new newline-preservation behavior for LF-only existing content. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
GitIgnoreEnsurer rewrites .gitignore with WriteAllText, which can change a user-owned file’s original encoding/BOM despite the “purely additive” intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
- GitIgnoreEnsurer now detects and preserves the existing .gitignore file's encoding (including a UTF-8 BOM if present) when rewriting it, instead of always writing back as UTF-8 without a BOM - a user-owned file should not have its encoding silently changed by a purely additive update. New files default to UTF-8 without a BOM. - Added GitIgnoreEnsurerTests coverage for CRLF-only existing content (protecting DetectNewLine/BuildSeparator from Windows-created .gitignore regressions) and for UTF-8 BOM preservation on rewrite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
GitIgnoreEnsurer currently overwrites .gitignore in-place (risking corruption on mid-write failures) and contains a couple of inaccurate documentation claims that should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/DemaConsulting.AgentControl/RepoSync/GitIgnoreEnsurer.cs:41
- The remarks claim this class is "thread-safe", but
Ensuredoes an unsynchronized read-modify-write of a shared file. Concurrent calls can race and write duplicate blocks, so the doc is currently incorrect (and could hide a real concurrency edge case).
This issue also appears on line 113 of the same file.
src/DemaConsulting.AgentControl/RepoSync/GitIgnoreEnsurer.cs:116
ReadExistingFileonly preserves encodings that can be detected via a BOM (because theStreamReaderis constructed with UTF-8 + BOM detection). The current doc mentions preserving "non-UTF8 encoding" generally, which isn't accurate for non-BOM legacy encodings.
/// <see cref="Ensure"/> can write the updated content back using the same encoding the
/// file already had (preserving a BOM/non-UTF8 encoding rather than silently normalizing
/// it to UTF-8) - a user-owned file should not have its encoding changed as a side effect
/// of a purely additive update.
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
…racy, reqstream orphan fix - Write .gitignore atomically (temp file + File.Replace/File.Move) so a mid-write failure (disk full, crash) can never leave a truncated or corrupted .gitignore behind. - Correct XML doc comments: remove inaccurate 'thread-safe' claim (Ensure performs an unsynchronized read-modify-write) and clarify that only BOM-detectable encodings are preserved, not non-UTF8 encodings generally. - Fix orphaned-requirements reqstream lint error by linking AgentControl-RepoSync-GitIgnorePrevention into AgentControl-System-Upgrade's children. - Update design/verification docs and reqstream test lists accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation includes a concrete inaccuracy about Ensure “never” throwing (it can still throw on read I/O failures), and there is also a small punctuation fix needed in the user guide.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/user_guide/introduction.md:66
- This bullet ends without a period, which makes the sentence grammatically incomplete compared to the surrounding documentation text.
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
- Correct git-ignore-ensurer.md: the marker-already-present no-op path still performs an initial read and can still throw InvalidOperationException if that read fails; it simply never writes. - Add missing period to a user guide bullet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is narrowly scoped, consistent with the stated design/requirements, and is backed by targeted unit tests plus updated companion documentation/model artifacts.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new
GitIgnoreEnsurerunit (RepoSync subsystem) that proactively protects a repo's.gitignoreso the four managed agent-package folders (.github/agents,.github/standards,.github/templates,.github/skills) can't be accidentally committed by an agentic coding tool before AgentControl's existing advisory badge would ever catch it.Problem
The existing "Committed agent files" badge only detects the problem after files are already committed - too late if an agentic tool has already run
git add/git commiton a developer's behalf. The fix needs to happen at the moment the folders are created on disk (package extraction), before any commit can occur.Change
GitIgnoreEnsurer: scans the repo-root.gitignorefor a fixed marker comment (# Added by AgentControl - agent package folders). If absent, appends a block covering the four managed folders (creating the file if missing). Purely additive - never edits/removes existing lines; nogit check-ignorecall and no gitignore glob-pattern analysis (marker presence is the sole idempotency gate, an intentional simplicity-over-precision tradeoff).RepoCardViewModelimmediately after every successfulPackageZipExtractor.Extractcall, in both the "Select Package..." and "Upgrade" flows, wrapped in its own non-blocking try/catch so.gitignoreI/O failures never abort extraction or the pin-file write..reviewmark.yamlentry, and documentation updates (architecture.md,docs/user_guide/introduction.md,README.md).Validation
pwsh ./fix.ps1/pwsh ./build.ps1: build succeeded, 199/199 tests passing.pwsh ./lint.ps1: passes cleanly (yamllint, cspell, markdownlint-cli2, reqstream, versionmark, reviewmark, sysml2tools, dotnet format --verify-no-changes).AgentControl-LauncherUI-RepoCardViewModelreview-set: Pass, 0 findings.AgentControl-RepoSync-GitIgnoreEnsurerreview-set: one flagged item (missing SysML2 path in review-set) verified as a false positive - matches the existing convention used by the siblingReleaseNotesViewerViewModelreview-set.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com