fix(engine): small correctness fixes found in the 1.1.1 review #152

2026-11-10T21:08:14Z

james.maes commented

2026-11-10 21:08:14 +00:00

What

Eight small, independent engine fixes from the 1.1.1 review, each with a regression test:

Drive root as target: C:\ (or /) rejected every repository. The root now ends in exactly one separator.
Windows-invalid characters: the set is written out instead of asked of the running platform, so the rules answer the same everywhere.
SyncSummary.WasCanceled: true exactly when a repository was canceled. A cancellation that arrives after the last repository finished leaves a complete run (the CLI then exits 0, not 1).
Concurrency read from disk: Account.MaxConcurrency is kept between 1 and 64 however it is set, so a hand-edited accounts.json cannot hand the engine 0 or 5000. The engine's own handling of a value below one has a test.
ProcessRunner: every way out ends a child that is still alive, and a child that exits before reading its input answers with its exit code and error output instead of an I/O error.
Gateway cache: logic moved to GatewayCache (under the coverage gate): four clients, keyed by a hash of the token, least recently used dropped first.
Activity log: timestamps carry their UTC offset; the clock is read once per entry; daily files older than 30 days are removed; a write that finds the file held by another process waits briefly and retries instead of dropping the entry.
Invalid-paths message: ends with "No files were written." instead of "Nothing was checked out.", which was wrong for a repository that already has a working tree.

Why

Closes #151

How it was tested

New tests: GatewayCacheTests, ProcessRunnerTests (real local child processes, Windows only), and additions to ActivityLogTests, OrgSyncEngineTests, RepositoryPathResolverTests, WindowsPathValidationTests, AccountsStoreTests.
Local gates: locked zero-warning x64 build, 835 engine and 164 CLI tests, 100% line coverage on the three gated assemblies (branch: Engine 94.65%, ViewModels 94.55%, CLI 98.20%), dotnet format --verify-no-changes.

Notes for the reviewer

User-visible: the log line format gains the offset (2026-11-10 14:03:22.123 -06:00 [INFO] ...), and log files older than 30 days are removed. docs/CLI.md and README.md say so; docs/CLI.md also named the wrong log folder, corrected here.
FileActivityLog.RetentionDays is a new public constant on the engine package; nothing else in the public surface changed shape.
A dropped gateway is not disposed on purpose: a request that started before it was dropped may still be using it.

Checklist

Commits are signed and signed off (git commit -S -s)
Zero build warnings; format and lint are clean
Tests added or updated; the 100% coverage gate still passes
Docs updated for user-visible changes; threat model updated if the attack surface changed (no change to the attack surface)
No secrets, credentials, or personal data
New dependencies are justified below, or none were added (none)
Substantial AI assistance, if any, is noted here: written with Claude Code, reviewed by an independent read-only agent before merge

🤖 Generated with Claude Code

## What Eight small, independent engine fixes from the 1.1.1 review, each with a regression test: - **Drive root as target**: `C:\` (or `/`) rejected every repository. The root now ends in exactly one separator. - **Windows-invalid characters**: the set is written out instead of asked of the running platform, so the rules answer the same everywhere. - **`SyncSummary.WasCanceled`**: true exactly when a repository was canceled. A cancellation that arrives after the last repository finished leaves a complete run (the CLI then exits 0, not 1). - **Concurrency read from disk**: `Account.MaxConcurrency` is kept between 1 and 64 however it is set, so a hand-edited `accounts.json` cannot hand the engine 0 or 5000. The engine's own handling of a value below one has a test. - **`ProcessRunner`**: every way out ends a child that is still alive, and a child that exits before reading its input answers with its exit code and error output instead of an I/O error. - **Gateway cache**: logic moved to `GatewayCache` (under the coverage gate): four clients, keyed by a hash of the token, least recently used dropped first. - **Activity log**: timestamps carry their UTC offset; the clock is read once per entry; daily files older than 30 days are removed; a write that finds the file held by another process waits briefly and retries instead of dropping the entry. - **Invalid-paths message**: ends with "No files were written." instead of "Nothing was checked out.", which was wrong for a repository that already has a working tree. ## Why Closes #151 ## How it was tested - New tests: `GatewayCacheTests`, `ProcessRunnerTests` (real local child processes, Windows only), and additions to `ActivityLogTests`, `OrgSyncEngineTests`, `RepositoryPathResolverTests`, `WindowsPathValidationTests`, `AccountsStoreTests`. - Local gates: locked zero-warning x64 build, 835 engine and 164 CLI tests, 100% line coverage on the three gated assemblies (branch: Engine 94.65%, ViewModels 94.55%, CLI 98.20%), `dotnet format --verify-no-changes`. ## Notes for the reviewer - User-visible: the log line format gains the offset (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`), and log files older than 30 days are removed. `docs/CLI.md` and `README.md` say so; `docs/CLI.md` also named the wrong log folder, corrected here. - `FileActivityLog.RetentionDays` is a new public constant on the engine package; nothing else in the public surface changed shape. - A dropped gateway is not disposed on purpose: a request that started before it was dropped may still be using it. ## Checklist - [x] Commits are signed and signed off (`git commit -S -s`) - [x] Zero build warnings; format and lint are clean - [x] Tests added or updated; the 100% coverage gate still passes - [x] Docs updated for user-visible changes; threat model updated if the attack surface changed (no change to the attack surface) - [x] No secrets, credentials, or personal data - [x] New dependencies are justified below, or none were added (none) - [x] Substantial AI assistance, if any, is noted here: written with Claude Code, reviewed by an independent read-only agent before merge 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 1 commit 2026-11-10 21:08:14 +00:00

fix(engine): small correctness fixes found in the 1.1.1 review b592898fcb

- A target folder that is a drive root ('C:\', '/') rejected every
  repository: the root keeps its separator when trimmed, and a second one
  was appended. The root now ends in exactly one.
- The Windows-invalid character set was asked of the running platform, so
  the Windows rules answered differently elsewhere. It is written out.
- SyncSummary.WasCanceled reported the token at the end of the run, so a
  cancellation that arrived after the last repository finished marked a
  complete run as canceled (and the CLI exited 1). It is now true exactly
  when a repository was canceled.
- A concurrency read from accounts.json was used as stored. Account now
  keeps it between 1 and 64 however it is set. The engine's handling of a
  value below one has a test.
- ProcessRunner wrote the child's input outside the block that ends the
  child, and a child that exited before reading its input failed the call
  with an I/O error. Every way out now ends a child that is still alive,
  and such a child answers with its exit code and error output.
- The gateway cache kept up to eight clients, keyed by the token, and
  emptied itself when full. GatewayCache keeps four, keyed by a hash of
  the token, and drops only the one unused the longest.
- The activity log: timestamps carry their offset from UTC; the clock is
  read once per entry, so the timestamp and the daily file agree at
  midnight; daily files older than 30 days are removed; a write that finds
  the file held by another process (the app and the CLI share it) waits
  briefly and tries again instead of dropping the entry.
- The invalid-paths message ended with "Nothing was checked out.", which
  is wrong for a repository that already has a working tree. It now ends
  with "No files were written.", which holds for a clone and a pull.

docs/CLI.md named the wrong log folder; corrected with the above.

Closes #151

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

james.maes added 1 commit 2026-11-10 21:32:34 +00:00

fix(engine): address the independent review of the small fixes 61fb8c253b

An absent concurrency is the default, not the minimum; the invalid-paths
sentence is about the working tree; a log that gave up on a held file does
not wait again before a cool-down; the cache keeps eight clients and hashes
the token as it is; the CLI marks a run as canceled when cancellation kept
a path recovery from happening; tests no longer depend on the wall clock.

Refs #151

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

james.maes commented

2026-11-10 21:32:34 +00:00

Independent review (read-only agent): merge after fixes

The reviewer built and ran the branch, including throwaway experiments against real child processes and four concurrent log writers. No data loss, hang, or escape from the target root. Findings and what was done, in the second commit:

#	Finding	Resolution
F1	An account whose JSON has no `MaxConcurrency` field is read as 0 by the source-generated reader, and the new clamp turned that into 1 (confirmed by running).	A value below 1 becomes the default (8); above 64 becomes 64. A missing `Description` is read as empty, not null. Tests for the absent fields, including a save round-trip.
F2	"No files were written." is not true of the kept `.git`.	"Nothing was written to the working tree."
F3	A log file held and released again and again stalled writers 200 ms per cycle, because every successful write re-armed the wait (confirmed by running).	After giving up once, no write waits again until a 30-second cool-down is over. Waits are counted in tests instead of timed.
F4	`docs/CLI.md` over-claimed "without losing entries" and "the same file".	Reworded to what the code does.
F5	`WasCanceled`'s doc did not match the CLI, and Ctrl+C before a `--sanitize-paths` recovery now printed "Finished".	Doc says what the engine guarantees; the CLI marks the run as canceled when cancellation kept a recovery from happening. Test added.
F6	Cache size 4 misses every lookup for 5 to 8 accounts with their own tokens.	Back to 8.
F7	Two malformed tokens could share a key after UTF-8 encoding.	The hash is taken over the UTF-16 code units. Test added.
F8	A failing test could leave a PowerShell child behind for ten minutes; three log tests depended on the wall clock or on midnight.	`try/finally` cancel; fixed clock for file names; counted waits.
F9	Output buffers were disposed before the child was ended on a failure.	Declared before the child starts, so they outlive it.

Checked and found sound by the reviewer: 28 root shapes × 10 names in the resolver (drive roots, UNC, \\?\), the character set against the platform's on Windows, every cancellation path for WasCanceled, ProcessRunner with 8 MB of output and a child that never reads its input, 16,000 log entries from four processes with none lost, retention boundaries, and the public surface of the engine package (one constant added).

Gates after the fixes: 839 engine and 164 CLI tests, 100% line coverage on the three gated assemblies, zero warnings, format clean.

Left for later work packages: the CLI logs to the default folder even when GCLO_DATA_DIR is set (CLI package).

## Independent review (read-only agent): merge after fixes The reviewer built and ran the branch, including throwaway experiments against real child processes and four concurrent log writers. No data loss, hang, or escape from the target root. Findings and what was done, in the second commit: | # | Finding | Resolution | | --- | --- | --- | | F1 | An account whose JSON has no `MaxConcurrency` field is read as 0 by the source-generated reader, and the new clamp turned that into 1 (confirmed by running). | A value below 1 becomes the default (8); above 64 becomes 64. A missing `Description` is read as empty, not null. Tests for the absent fields, including a save round-trip. | | F2 | "No files were written." is not true of the kept `.git`. | "Nothing was written to the working tree." | | F3 | A log file held and released again and again stalled writers 200 ms per cycle, because every successful write re-armed the wait (confirmed by running). | After giving up once, no write waits again until a 30-second cool-down is over. Waits are counted in tests instead of timed. | | F4 | `docs/CLI.md` over-claimed "without losing entries" and "the same file". | Reworded to what the code does. | | F5 | `WasCanceled`'s doc did not match the CLI, and Ctrl+C before a `--sanitize-paths` recovery now printed "Finished". | Doc says what the engine guarantees; the CLI marks the run as canceled when cancellation kept a recovery from happening. Test added. | | F6 | Cache size 4 misses every lookup for 5 to 8 accounts with their own tokens. | Back to 8. | | F7 | Two malformed tokens could share a key after UTF-8 encoding. | The hash is taken over the UTF-16 code units. Test added. | | F8 | A failing test could leave a PowerShell child behind for ten minutes; three log tests depended on the wall clock or on midnight. | `try/finally` cancel; fixed clock for file names; counted waits. | | F9 | Output buffers were disposed before the child was ended on a failure. | Declared before the child starts, so they outlive it. | Checked and found sound by the reviewer: 28 root shapes × 10 names in the resolver (drive roots, UNC, `\\?\`), the character set against the platform's on Windows, every cancellation path for `WasCanceled`, `ProcessRunner` with 8 MB of output and a child that never reads its input, 16,000 log entries from four processes with none lost, retention boundaries, and the public surface of the engine package (one constant added). Gates after the fixes: 839 engine and 164 CLI tests, 100% line coverage on the three gated assemblies, zero warnings, format clean. Left for later work packages: the CLI logs to the default folder even when `GCLO_DATA_DIR` is set (CLI package).

james.maes merged commit 7bb44cd132 into main

2026-11-10 21:32:36 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/gclo#152