fix(fragment): guard guid/updates reads with JsonValueKind by tonythethompson · Pull Request #135 · tonythethompson/QuickShell

tonythethompson · 2026-07-29T02:59:29Z

Summary

Adds a JsonValueKind.String guard before reading guid and updates in TerminalFragmentDiscovery.MergeFile. Previously, a non-string value made JsonElement.GetString() throw InvalidOperationException, which LoadAll caught and treated as malformed JSON, causing the entire fragment file to be skipped. Now only the bad profile is skipped and the rest of the file is processed.

Changes

QuickShell.Core/Services/TerminalFragmentDiscovery.cs: check guidNode.ValueKind and updatesNode.ValueKind before GetString().
QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs: add LoadAll_SkipsNonStringGuid_AndProcessesRestOfFile.

Test plan

dotnet test QuickShell.Core.Tests/QuickShell.Core.Tests.csproj -c Debug -p:Platform=x64 passed: 917/917.

Generated with Devin

Summary by cubic

Guarded JSON type checks for guid/updates in TerminalFragmentDiscovery so one bad profile no longer skips the whole fragment file; only the invalid entry is ignored.

Bug Fixes
- Check JsonValueKind.String before GetString() for guid/updates in TerminalFragmentDiscovery.MergeFile; skip entries with non-string IDs.
- Added test load_all_skips_non_string_guid_and_processes_rest_of_file to verify behavior; added XML doc comments for MergeFile.

^{Written for commit 7ad9a69. Summary will update on new commits.}

A non-string "guid" or "updates" value caused JsonElement.GetString() to throw InvalidOperationException, which LoadAll treated as malformed and skipped the whole file. Now each profile entry is individually skipped when its GUID target is not a JSON string, so one bad profile in a fragment no longer discards the rest. Added TerminalFragmentDiscoveryTests.LoadAll_SkipsNonStringGuid_AndProcessesRestOfFile. All 917 tests pass. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

coderabbitai · 2026-07-29T02:59:48Z

📝 Walkthrough

Walkthrough

Changes

Terminal fragment loading

Layer / File(s)	Summary
Identifier parsing and validation `QuickShell.Core/Services/TerminalFragmentDiscovery.cs`, `QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs`	`MergeFile` now accepts only string-valued `guid` and `updates` fields, with coverage for mixed valid and invalid JSON entries.

Agent documentation formatting

Layer / File(s)	Summary
AGENTS.md formatting updates `AGENTS.md`	Adds a placeholder line and adjusts Markdown table separator spacing.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 60.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (7 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Pipeline Stage Enum Ordering	✅ Passed	No `SessionWorkflowStage` enum or comparisons exist in the touched files or repo, so the pipeline-order check is not applicable to this PR.
Gpu/Cpu Runtime Boundary	✅ Passed	Only QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs changed; no inference/, main.py, requirements, or diarization provider/registry files were touched.
Managed Host Restart Safety	✅ Passed	PASS: The PR only changes TerminalFragmentDiscovery parsing/tests; no ManagedVenvHostManager/Containerized* restart or readiness paths are touched.
Title check	✅ Passed	The title accurately summarizes the main fix: guarding guid/updates JSON reads by value kind.
Description check	✅ Passed	The description matches the changeset and clearly explains the behavior fix and added test.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
✅ Committed to branch successfully - (🔄 Check to regenerate)

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch devin/fragment-guid-valuekind

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch devin/fragment-guid-valuekind

_{Comment @coderabbitai help to get the list of available commands.}

sourcery-ai

Hey - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

qodo-code-review · 2026-07-29T03:01:23Z

Qodo Fixer

No findings are available for this PR yet. Findings appear here once Qodo has reviewed the PR.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs`:
- Around line 252-256: Strengthen the assertions in the LoadAll test around
TerminalFragmentDiscovery.LoadAll by verifying the earlier valid profile is
retained and profiles keyed by invalid numeric guid/updates identifiers are
absent, or by asserting the exact expected profile count. Keep the existing
later valid-profile assertions while ensuring invalid profiles are confirmed
skipped rather than merely tolerated.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7c0523ee-d81c-42c8-b6b1-f8d3c5216b36

📥 Commits

Reviewing files that changed from the base of the PR and between 2ed2b1a and 8c523ff.

📒 Files selected for processing (3)

AGENTS.md
QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs
QuickShell.Core/Services/TerminalFragmentDiscovery.cs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Trackdubllc/Trackdub (manual)
tonythethompson/QuickShell (manual)
tonythethompson/numan (manual)
tonythethompson/dependency-chain-substrate (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

GitHub Check: Analyze C# with CodeQL
GitHub Check: Performance harness (artifacts)
GitHub Check: .NET build and test

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{cs,py}

📄 CodeRabbit inference engine (Custom checks)

Keep SessionWorkflowStage members in strictly ascending order: Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.

Files:

QuickShell.Core/Services/TerminalFragmentDiscovery.cs
QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs

**/*.cs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.cs: Keep namespaces aligned with folders, use nullable and implicit usings, and generally define one type per file.
Leave existing #region agent log instrumentation blocks intact.

Files:

QuickShell.Core/Services/TerminalFragmentDiscovery.cs
QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs

QuickShell.Core/**/*.cs

📄 CodeRabbit inference engine (AGENTS.md)

QuickShell.Core/**/*.cs: Keep QuickShell.Core free of CmdPal SDK dependencies; expose swappable services through interfaces and dependency injection.
Async methods ending in Async should accept CancellationToken cancellationToken = default; synchronous wrappers should use .GetAwaiter().GetResult().

Files:

QuickShell.Core/Services/TerminalFragmentDiscovery.cs

**/*.{cs,csproj}

📄 CodeRabbit inference engine (AGENTS.md)

Use the established static-versus-DI split: pure logic should be internal static helpers, while swappable dependencies should use interfaces and DI.

Files:

QuickShell.Core/Services/TerminalFragmentDiscovery.cs
QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs

QuickShell.Core.Tests/**/*.cs

📄 CodeRabbit inference engine (AGENTS.md)

QuickShell.Core.Tests/**/*.cs: Use xUnit, underscore-separated test method names, real services and established seams instead of Moq or FluentAssertions.
Use process-wide test seams such as LaunchExecutorTestEnvironment, FakeShortcutRepository, and AgentCliCatalog overrides; group tests sharing static seams with [Collection].

Files:

QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs

🔍 Remote MCP GitHub Copilot

Relevant review context

TerminalFragmentDiscovery is an internal static class that scans Windows Terminal fragment roots, with later roots overriding earlier ones. It also uses a fingerprint of fragment file paths, mtimes, and sizes to avoid a full rescan.
WtProfilesService recomputes that fingerprint and then calls TerminalFragmentDiscovery.LoadAll(_fragmentRoots, out var hadReadFailures). A nearby comment says it only commits fragments and invalidates settings caches when every discovered file reads successfully, so failures in this path can block a refresh.
The repo already has QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs, so this PR’s regression test sits in the existing discovery test area.

🔇 Additional comments (2)

AGENTS.md (1)

24-24: LGTM!

Also applies to: 40-40

QuickShell.Core/Services/TerminalFragmentDiscovery.cs (1)

166-175: LGTM!

…ath.Combine' may silently drop its earlier arguments' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>

coderabbitai · 2026-07-29T05:35:41Z

Note

Docstrings generation - SUCCESS
Generated docstrings and committed to branch devin/fragment-guid-valuekind (commit: 7ad9a69d2cad5459798c761a38151ec7bced100a)

@tonythethompson

Docstrings generation was requested by @tonythethompson. The following files were modified: * `QuickShell.Core/Services/TerminalFragmentDiscovery.cs` These files were ignored: * `QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs` These file types are not supported: * `AGENTS.md`

linear-code · 2026-07-29T05:44:37Z

TS-72

sourcery-ai Bot reviewed Jul 29, 2026

View reviewed changes

Update AGENTS.md

8c523ff

coderabbitai Bot requested changes Jul 29, 2026

View reviewed changes

Comment thread QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs Outdated

qodo-code-review Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs Outdated

github-advanced-security Bot found potential problems Jul 29, 2026

View reviewed changes

Comment thread QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs Fixed

tonythethompson and others added 2 commits July 28, 2026 20:11

Update QuickShell.Core.Tests/TerminalFragmentDiscoveryTests.cs

1967651

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>

coderabbitai Bot approved these changes Jul 29, 2026

View reviewed changes

style(tests): rename test to lowercase snake_case

201e1da

tonythethompson merged commit 1996a48 into master Jul 29, 2026
11 checks passed

tonythethompson deleted the devin/fragment-guid-valuekind branch July 29, 2026 05:44

This was referenced Jul 30, 2026

Skip non-string guid/updates entries in fragment merge #141

Closed

Skip non-string guid/updates entries in fragment merge trackdubllc/Trackdub#8

Closed

Navigation Menu

Uh oh!

fix(fragment): guard guid/updates reads with JsonValueKind - #135

fix(fragment): guard guid/updates reads with JsonValueKind#135
tonythethompson merged 6 commits into
masterfrom
devin/fragment-guid-valuekind

tonythethompson commented Jul 29, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

qodo-code-review Bot commented Jul 29, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

linear-code Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tonythethompson commented Jul 29, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Summary by cubic

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot commented Jul 29, 2026

Qodo Fixer

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Relevant review context

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

linear-code Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tonythethompson commented Jul 29, 2026 •

edited by cubic-dev-ai Bot

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading