From 17407b636ff8e19403d079cc79259e2b03d2f115 Mon Sep 17 00:00:00 2001 From: Hugo Pernet Date: Fri, 28 Aug 2026 14:39:09 +0200 Subject: [PATCH 1/7] Expose Xero leave balance import and status to CLI and MCP TimePro added POST /api/leave/balances/import and GET /api/leave/balances/status (PR 3493). Surface both as CLI commands under a new "leave balances" branch, and as MCP tools on the default leave surface. The import endpoint reads the request stream as the raw Xero CSV export, so it needs a PostRawAsync helper: every existing POST goes through JsonContent, which would send an escaped string literal and fail the server-side parser. An integration test pins the body and text/csv content type against that regression. Both surfaces take a path to the CSV rather than its contents. MCP tool arguments travel through an agent's context, where a large CSV is expensive and liable to be silently truncated into a partial import that still reports success. LeaveBalanceImportService owns reading and validating the file so the CLI and MCP behave identically, and translates the endpoint's 403 and 422 into readable messages. The import replaces balances company-wide with no server-side dry-run, so the CLI confirms unless --yes is passed. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 26 +++ README.md | 23 ++- .../Features/Leave/BalancesImportCommand.cs | 116 +++++++++++ .../Features/Leave/BalancesStatusCommand.cs | 80 ++++++++ .../Leave/LeaveBalanceImportService.cs | 136 +++++++++++++ .../Features/Mcp/McpHostCommand.cs | 1 + .../Features/Mcp/Tools/LeaveMcpTools.cs | 52 ++++- .../ApiClient/TimeProApiClient.cs | 42 ++++ src/SSW.TimePro.Cli/Program.cs | 14 ++ .../Shared/Models/LeaveModels.cs | 37 ++++ .../Features/LeaveBalancesApiTests.cs | 146 ++++++++++++++ .../Features/Leave/BalancesCommandsTests.cs | 155 +++++++++++++++ .../Leave/LeaveBalanceImportServiceTests.cs | 183 ++++++++++++++++++ .../Features/Mcp/LeaveMcpToolsTests.cs | 110 ++++++++++- 14 files changed, 1117 insertions(+), 4 deletions(-) create mode 100644 src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs create mode 100644 src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs create mode 100644 src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs create mode 100644 tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs create mode 100644 tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs create mode 100644 tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs diff --git a/AGENTS.md b/AGENTS.md index 6b92974..3abef26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -53,6 +53,8 @@ Run `tp --help` for full command list. Key commands: - `tp leave cancel ID --reason "..." --yes` - Cancel leave - `tp leave list --filter UPCOMING --json` - List leave - `tp leave balance --emp-id JEK` - Leave-usage signal (days since last leave + hours taken in last 12 months) +- `tp leave balances status` - When leave balances were last imported from Xero, and whether they are stale +- `tp leave balances import ./LeaveBalances.csv --yes` - Import company-wide leave balances from a Xero CSV export (leave admins only) - `tp feature accounting enable` - Enable accounting skills and accounting MCP tools - `tp feature developer enable` - Enable developer diagnostics/environment comparison skills and timesheet/finance bug diagnostic skills - `tp mcp [--tenant NAME]` - Start MCP server (optional per-session tenant binding) @@ -98,6 +100,30 @@ validation and payload preparation, returns the proposed request, and must not c The cancel endpoint (`PUT /api/leave/{id}/cancel`) requires `LeaveId` (Guid) and `CancellationReason` in the request body. +## Leave Balances (Xero CSV Sync) + +`POST /api/leave/balances/import` takes the raw Xero "Leave Balances" CSV export as the +request body with content type `text/csv`. It is **not** a JSON endpoint and **not** a +multipart upload, so it must go through `PostRawAsync`, never the `PostAsync`/`PutAsync` +JSON helpers - `JsonContent` would send an escaped string literal and the server-side +parser would reject it. `LeaveBalancesApiTests` asserts the body and content type for +exactly this reason. + +The endpoint is leave-admin only (`403` otherwise) and returns `422` with the CSV parser's +message as a bare JSON string when the file cannot be read. Both are translated into +readable messages by `LeaveBalanceImportService`, which owns file reading and validation +for the CLI and MCP alike. Import replaces stored balances for every matched employee; +there is no server-side dry-run, so the CLI confirms unless `--yes` is passed. + +CLI and MCP surfaces take a **path** to the CSV, not its contents. Tool arguments travel +through an agent's context, where a large CSV is expensive and liable to be silently +truncated into a partial import that still looks successful. `GET /api/leave/balances/status` +is the cheap read used to decide whether a re-import is due. + +Rows whose Xero employee name matches no TimePro employee, or matches several, are returned +in `unmatchedEmployees` and skipped rather than guessed at; implausible balances are returned +in `warnings`. The import succeeds regardless, so both lists must be surfaced to the user. + The list endpoint (`GET /api/leave/`) returns per-entry `daysAway`, `updatedAt`, `optionalEmp`, `timeLessOverride`, `cancellationReason` (all bound on `LeaveEntry`) plus a top-level `cancelledCount` on the list envelope. These surface in `tp leave list --json`. ## Testing diff --git a/README.md b/README.md index 18a3d91..f87860f 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ SSW TimePro is a time tracking and invoicing system. This CLI makes it fast to v - **Timesheet CRUD** — Create, update, delete timesheets with rate checking and lock detection - **Suggested Timesheets** — View and accept suggested timesheets to keep accuracy stats high - **CRM Bookings** — See your appointments from the CRM calendar -- **Leave Management** — Create, list, update, and cancel EasyLeave requests +- **Leave Management** — Create, list, update, and cancel EasyLeave requests; leave admins can sync company-wide leave balances from a Xero CSV export - **Repo Mapping** — Map git repos to clients/projects; auto-detects via path or remote URL, with git worktree support; optional `--issues-repo` for projects whose issues live in a different GitHub repo than the code - **Daily Scrum** — Generate an SSW-format daily scrum email from timesheets, CRM bookings and GitHub activity, with AutoScrum-inspired `--smart` selection, overridable per-tenant/client templates, rich-text / markdown / plain clipboard support and an interactive copy mode - **Location Defaults** — Set WFH days so location is auto-applied when creating timesheets @@ -132,6 +132,8 @@ tp ts get 2026-03-12 # Specific date | `tp leave create` | Create a leave request (see options and `--dry-run` below) | | `tp leave update ID` | Update a leave request while preserving unspecified API-returned fields; supports `--dry-run` | | `tp leave cancel ID` | Cancel a leave request (`--reason`) | +| `tp leave balances status` | Show when leave balances were last imported from Xero and whether they are stale | +| `tp leave balances import PATH` | Import leave balances for all employees from a Xero CSV export (leave admins only) | | `tp cl search QUERY` | Search for clients | | `tp proj list --client ID` | List projects for a client | | `tp proj recent` | Surface projects you've recently logged time against (likely picks for new entries) | @@ -244,6 +246,12 @@ tp leave update --start 2026-04-01 --end 2026-04-01 \ # Cancel a leave request tp leave cancel --reason "Plans changed" --yes + +# Check whether the stored leave balances are still current +tp leave balances status --json + +# Import the latest Xero leave balances export (leave admins only) +tp leave balances import ~/Downloads/LeaveBalances.csv --yes ``` Leave create options: @@ -266,6 +274,16 @@ explicit. Use `--clear-approved-by` or `--clear-cc` to remove those values, and `--dry-run`; combine it with `--json` to inspect the exact API payload without creating or changing leave. +**Leave balances (Xero sync).** `tp leave balances import` uploads the raw Xero "Leave +Balances" CSV export and replaces the balances stored in TimePro for every employee it can +match. It requires leave admin rights, has no dry-run and no undo, so it prompts unless you +pass `--yes`. Rows whose Xero name matches no TimePro employee — or matches more than one — +are reported as skipped rather than guessed at, and implausibly large balances are flagged +as warnings; the import still succeeds, so check both lists afterwards. Run +`tp leave balances status` first to see whether a re-import is actually due. The MCP tool +takes the path to the CSV rather than its contents, so the file never has to pass through +an agent's context. + ### Week View Compact view shows one line per timesheet with totals: @@ -569,7 +587,7 @@ Current default tool groups include: |-------|----------| | Timesheets | Get, create, update, delete, suggested timesheets, accept suggestions, list iterations, `check_week` (leave-aware weekly coverage) | | Lookup | Search clients, list projects, get client rate, CRM bookings, location and repo mapping | -| Leave | List EasyLeave entries (optionally filtered by `empId`), create and safely update EasyLeave requests with dry-run previews, `get_leave_balance` (days since last leave + 12-month hours) | +| Leave | List EasyLeave entries (optionally filtered by `empId`), create and safely update EasyLeave requests with dry-run previews, `get_leave_balance` (days since last leave + 12-month hours), `get_leave_balance_status` and `import_leave_balances` (Xero balance sync, leave admins only) | Optional accounting MCP tools are enabled with: @@ -608,6 +626,7 @@ Then ask Claude things like: - "Accept the suggested timesheet for Monday" - "What's my billing rate for Northwind?" - "Move my upcoming leave to Wednesday and keep its other details" +- "Are the leave balances up to date? If not, import ~/Downloads/LeaveBalances.csv" ### VS Code (Copilot / Continue) diff --git a/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs b/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs new file mode 100644 index 0000000..0efb347 --- /dev/null +++ b/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs @@ -0,0 +1,116 @@ +using System.ComponentModel; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Infrastructure.Config; +using SSW.TimePro.Cli.Infrastructure.Output; +using Spectre.Console; +using Spectre.Console.Cli; + +namespace SSW.TimePro.Cli.Features.Leave; + +[Description("Import leave balances for all employees from a Xero leave balances CSV export")] +public class BalancesImportCommand : AsyncCommand +{ + private readonly LeaveBalanceImportService _importService; + private readonly IConfigService _config; + + public class Settings : CommandSettings + { + [CommandArgument(0, "")] + [Description("Path to the Xero 'Leave Balances' CSV export")] + public string CsvPath { get; set; } = string.Empty; + + [CommandOption("--yes")] + [Description("Skip confirmation")] + public bool Yes { get; set; } + + [CommandOption("--json")] + [Description("Output as JSON")] + public bool Json { get; set; } + } + + public BalancesImportCommand(LeaveBalanceImportService importService, IConfigService config) + { + _importService = importService; + _config = config; + } + + protected override async Task ExecuteAsync(CommandContext context, Settings settings, CancellationToken cancellationToken) + { + if (_config.LoadActiveTenantConfig() is null) + { + WriteError(settings.Json, "Not logged in. Run 'tp login --tenant ' first."); + return 1; + } + + // Validate the file before prompting so an unusable path fails immediately. + try + { + _importService.ReadCsv(settings.CsvPath); + } + catch (LeaveBalanceImportValidationException ex) + { + WriteError(settings.Json, ex.Message); + return 1; + } + + // The import replaces stored balances for every matched employee and TimePro offers no + // dry-run, so confirm unless the caller has opted out. + if (!settings.Yes && !settings.Json) + { + AnsiConsole.MarkupLine( + $"About to import leave balances for [bold]all employees[/] from {Markup.Escape(settings.CsvPath)}."); + if (!AnsiConsole.Confirm("This replaces the balances currently stored in TimePro. Continue?", false)) + return 1; + } + + try + { + var result = await _importService.ImportAsync(settings.CsvPath, cancellationToken); + + OutputHelper.Render(result, settings.Json, r => + { + var table = new Table().NoBorder().HideHeaders().AddColumn("Key").AddColumn("Value"); + table.AddRow("[bold]Balances as at[/]", r.AsAtDate.ToString("yyyy-MM-dd")); + table.AddRow("[bold]Created[/]", r.Created.ToString()); + table.AddRow("[bold]Updated[/]", r.Updated.ToString()); + AnsiConsole.Write(table); + + foreach (var warning in r.Warnings) + OutputHelper.WriteWarning(warning); + + if (r.UnmatchedEmployees.Count > 0) + { + OutputHelper.WriteWarning( + $"{r.UnmatchedEmployees.Count} row(s) were skipped because the name did not match exactly one TimePro employee:"); + foreach (var name in r.UnmatchedEmployees) + AnsiConsole.MarkupLine($" - {Markup.Escape(name)}"); + } + + OutputHelper.WriteSuccess($"Imported leave balances ({r.Created} created, {r.Updated} updated)"); + }); + + return 0; + } + catch (LeaveBalanceImportValidationException ex) + { + WriteError(settings.Json, ex.Message); + return 1; + } + catch (ApiException ex) + { + var detail = ApiErrorParser.ExtractDetail(ex.ResponseBody); + if (settings.Json) + OutputHelper.WriteJsonError($"API error: {ex.Message}", ex.StatusCode, detail); + OutputHelper.WriteError($"API error ({ex.StatusCode}): {ex.Message}" + + (detail is not null ? $" — {detail}" : "")); + return 1; + } + } + + private static void WriteError(bool json, string message) + { + if (json) + OutputHelper.WriteJsonError(message); + OutputHelper.WriteError(message); + } +} diff --git a/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs b/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs new file mode 100644 index 0000000..e88723c --- /dev/null +++ b/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs @@ -0,0 +1,80 @@ +using System.ComponentModel; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Infrastructure.Config; +using SSW.TimePro.Cli.Infrastructure.Output; +using Spectre.Console; +using Spectre.Console.Cli; + +namespace SSW.TimePro.Cli.Features.Leave; + +[Description("Show when leave balances were last imported from Xero and whether they are stale")] +public class BalancesStatusCommand : AsyncCommand +{ + private readonly ITimeProApiClient _api; + private readonly IConfigService _config; + + public class Settings : CommandSettings + { + [CommandOption("--json")] + [Description("Output as JSON")] + public bool Json { get; set; } + } + + public BalancesStatusCommand(ITimeProApiClient api, IConfigService config) + { + _api = api; + _config = config; + } + + protected override async Task ExecuteAsync(CommandContext context, Settings settings, CancellationToken cancellationToken) + { + if (_config.LoadActiveTenantConfig() is null) + { + const string message = "Not logged in. Run 'tp login --tenant ' first."; + if (settings.Json) + OutputHelper.WriteJsonError(message); + else + OutputHelper.WriteError(message); + return 1; + } + + try + { + var status = await _api.GetLeaveBalanceStatusAsync(cancellationToken); + if (status is null) + { + // Nothing imported yet is a valid state, not a failure. + if (settings.Json) + OutputHelper.WriteJson(new { imported = false }); + else + OutputHelper.WriteWarning("No leave balances have been imported yet."); + return 0; + } + + OutputHelper.Render(status, settings.Json, s => + { + var table = new Table().NoBorder().HideHeaders().AddColumn("Key").AddColumn("Value"); + table.AddRow("[bold]Balances as at[/]", s.AsAtDate?.ToString("yyyy-MM-dd") ?? "-"); + table.AddRow("[bold]Last imported[/]", + s.LastImportedAt?.ToLocalTime().ToString("yyyy-MM-dd HH:mm") ?? "never"); + table.AddRow("[bold]Employees[/]", s.EmployeeCount.ToString()); + table.AddRow("[bold]Stale[/]", s.IsStale ? "[yellow]yes[/]" : "no"); + AnsiConsole.Write(table); + + if (s.IsStale) + OutputHelper.WriteWarning("Balances are stale. Re-import the latest Xero export with 'tp leave balances import '."); + }); + + return 0; + } + catch (ApiException ex) + { + var detail = ApiErrorParser.ExtractDetail(ex.ResponseBody); + if (settings.Json) + OutputHelper.WriteJsonError($"API error: {ex.Message}", ex.StatusCode, detail); + OutputHelper.WriteError($"API error ({ex.StatusCode}): {ex.Message}" + + (detail is not null ? $" — {detail}" : "")); + return 1; + } + } +} diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs new file mode 100644 index 0000000..5023980 --- /dev/null +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs @@ -0,0 +1,136 @@ +using System.Text; +using System.Text.Json; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Shared.Models; + +namespace SSW.TimePro.Cli.Features.Leave; + +public sealed class LeaveBalanceImportValidationException(string message) : Exception(message); + +/// +/// Reads a Xero "Leave Balances" CSV export from disk and imports it into TimePro. +/// +/// The file is read here rather than passed around as text so the MCP tool can take a path: +/// tool arguments travel through the model's context, where a large CSV is both expensive and +/// liable to be silently truncated. The CSV itself is parsed server-side and never stored. +/// +public sealed class LeaveBalanceImportService +{ + /// + /// Guard against a caller pointing at the wrong file entirely (a database dump, a video). + /// A real Xero balance export for a company of any plausible size is far below this. + /// + internal const int MaxCsvBytes = 5 * 1024 * 1024; + + private readonly ITimeProApiClient _api; + + public LeaveBalanceImportService(ITimeProApiClient api) => _api = api; + + /// + /// Reads and sanity-checks the CSV at , throwing + /// before any network call if it is + /// missing, empty, oversized, or plainly not a CSV. + /// + public string ReadCsv(string csvPath) + { + if (string.IsNullOrWhiteSpace(csvPath)) + throw new LeaveBalanceImportValidationException("A path to the Xero leave balances CSV export is required."); + + string fullPath; + try + { + fullPath = Path.GetFullPath(csvPath.Trim()); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) + { + throw new LeaveBalanceImportValidationException($"'{csvPath}' is not a valid file path."); + } + + if (Directory.Exists(fullPath)) + throw new LeaveBalanceImportValidationException($"'{fullPath}' is a directory. Provide the path to the CSV file itself."); + + if (!File.Exists(fullPath)) + throw new LeaveBalanceImportValidationException($"File not found: {fullPath}"); + + var length = new FileInfo(fullPath).Length; + if (length == 0) + throw new LeaveBalanceImportValidationException($"File is empty: {fullPath}"); + + if (length > MaxCsvBytes) + { + throw new LeaveBalanceImportValidationException( + $"File is {length / 1024 / 1024}MB, which is larger than the {MaxCsvBytes / 1024 / 1024}MB limit for a leave balances CSV. Check that '{fullPath}' is the Xero export."); + } + + string content; + try + { + content = File.ReadAllText(fullPath, Encoding.UTF8); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + throw new LeaveBalanceImportValidationException($"Could not read {fullPath}: {ex.Message}"); + } + + if (string.IsNullOrWhiteSpace(content)) + throw new LeaveBalanceImportValidationException($"File contains no data: {fullPath}"); + + if (content.Contains('\0')) + { + throw new LeaveBalanceImportValidationException( + $"'{fullPath}' looks like a binary file, not a CSV. Export the Xero leave balances report as CSV first."); + } + + return content; + } + + /// + /// Reads the CSV and sends it to TimePro. The server owns parsing and matching, so its + /// rejection messages are surfaced verbatim rather than second-guessed here. + /// + public async Task ImportAsync(string csvPath, CancellationToken ct = default) + { + var csv = ReadCsv(csvPath); + + try + { + return await _api.ImportLeaveBalancesAsync(csv, ct) + ?? throw new LeaveBalanceImportValidationException( + "TimePro accepted the import but returned no result. Run 'tp leave balances status' to confirm what was stored."); + } + catch (ApiException ex) when (ex.StatusCode is 401 or 403) + { + throw new LeaveBalanceImportValidationException( + "Importing leave balances requires leave admin rights in TimePro, and this account does not have them."); + } + catch (ApiException ex) when (ex.StatusCode == 422) + { + // The endpoint returns the CSV parser's own message as a bare JSON string. + throw new LeaveBalanceImportValidationException( + $"TimePro could not read the CSV: {DescribeUnprocessable(ex.ResponseBody)}"); + } + } + + /// + /// Unwraps the bare JSON string body returned by the endpoint's 422, falling back to the + /// shared problem+json parsing for anything else. + /// + internal static string DescribeUnprocessable(string? responseBody) + { + if (string.IsNullOrWhiteSpace(responseBody)) + return "the file was rejected without a reason."; + + try + { + using var doc = JsonDocument.Parse(responseBody); + if (doc.RootElement.ValueKind == JsonValueKind.String) + return doc.RootElement.GetString() ?? responseBody; + } + catch (JsonException) + { + // Not JSON at all — fall through to the shared parser. + } + + return ApiErrorParser.ExtractDetail(responseBody) ?? responseBody; + } +} diff --git a/src/SSW.TimePro.Cli/Features/Mcp/McpHostCommand.cs b/src/SSW.TimePro.Cli/Features/Mcp/McpHostCommand.cs index 6b03ea0..8687d59 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/McpHostCommand.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/McpHostCommand.cs @@ -39,6 +39,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings builder.Services.AddHttpClient(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); + builder.Services.AddSingleton(); var mcpServer = builder.Services .AddMcpServer() diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs index 2cad8e0..ca159f3 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs @@ -14,6 +14,7 @@ public class LeaveMcpTools private readonly IConfigService _config; private readonly LeaveCreateService _leaveCreateService; private readonly LeaveUpdateService _leaveUpdateService; + private readonly LeaveBalanceImportService _leaveBalanceImportService; private static readonly JsonSerializerOptions JsonOpts = new() { @@ -25,12 +26,14 @@ public LeaveMcpTools( ITimeProApiClient api, IConfigService config, LeaveCreateService leaveCreateService, - LeaveUpdateService leaveUpdateService) + LeaveUpdateService leaveUpdateService, + LeaveBalanceImportService leaveBalanceImportService) { _api = api; _config = config; _leaveCreateService = leaveCreateService; _leaveUpdateService = leaveUpdateService; + _leaveBalanceImportService = leaveBalanceImportService; } [McpServerTool] @@ -199,6 +202,53 @@ public async Task UpdateLeave( } } + [McpServerTool] + [Description("Report when TimePro's leave balances were last imported from Xero, how many employees have a stored balance, and whether the data is stale. Read-only. Check this before importing so you can tell the user whether a re-import is actually needed.")] + public async Task GetLeaveBalanceStatus(CancellationToken ct = default) + { + if (_config.LoadActiveTenantConfig() is null) + return """{"error": "Not logged in. Run 'tp login --tenant ' first."}"""; + + var status = await _api.GetLeaveBalanceStatusAsync(ct); + if (status is null) + return JsonSerializer.Serialize(new { imported = false }, JsonOpts); + + return JsonSerializer.Serialize(status, JsonOpts); + } + + [McpServerTool] + [Description( + "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing the balances currently stored in TimePro. " + + "Requires leave admin rights; other accounts get a permission error. There is no dry run and no undo, so confirm with the user before calling this. " + + "Pass the path to the CSV file on this machine - do not paste the file contents. " + + "Call GetLeaveBalanceStatus first to check whether an import is due. " + + "Report the returned unmatchedEmployees (rows skipped because the Xero name matched no TimePro employee, or matched several) and warnings back to the user - the import succeeds despite them.")] + public async Task ImportLeaveBalances( + [Description("Path to the Xero 'Leave Balances' CSV export on this machine")] string csvPath, + CancellationToken ct = default) + { + if (_config.LoadActiveTenantConfig() is null) + return """{"error": "Not logged in. Run 'tp login --tenant ' first."}"""; + + try + { + var result = await _leaveBalanceImportService.ImportAsync(csvPath, ct); + return JsonSerializer.Serialize(new + { + success = true, + result.AsAtDate, + result.Created, + result.Updated, + result.UnmatchedEmployees, + result.Warnings + }, JsonOpts); + } + catch (LeaveBalanceImportValidationException ex) + { + return JsonSerializer.Serialize(new { error = ex.Message }, JsonOpts); + } + } + private static string? ResolveEmpId(string? empId, string? employeeId) { var requestedEmpId = !string.IsNullOrWhiteSpace(empId) ? empId : employeeId; diff --git a/src/SSW.TimePro.Cli/Infrastructure/ApiClient/TimeProApiClient.cs b/src/SSW.TimePro.Cli/Infrastructure/ApiClient/TimeProApiClient.cs index cd487a1..9a0831a 100644 --- a/src/SSW.TimePro.Cli/Infrastructure/ApiClient/TimeProApiClient.cs +++ b/src/SSW.TimePro.Cli/Infrastructure/ApiClient/TimeProApiClient.cs @@ -1,4 +1,5 @@ using System.Net.Http.Json; +using System.Text; using System.Text.Json; using SSW.TimePro.Cli.Infrastructure.Config; using SSW.TimePro.Cli.Shared.Models; @@ -47,6 +48,11 @@ public interface ITimeProApiClient Task CreateLeaveAsync(CreateLeaveRequest request, CancellationToken ct = default); Task UpdateLeaveAsync(UpdateLeaveRequest request, CancellationToken ct = default); Task CancelLeaveAsync(string leaveId, CancelLeaveRequest request, CancellationToken ct = default); + + // Leave balances (Xero CSV sync). Import is leave-admin only server-side and replaces + // stored balances; status is a cheap read used to decide whether a re-import is due. + Task GetLeaveBalanceStatusAsync(CancellationToken ct = default); + Task ImportLeaveBalancesAsync(string csvContent, CancellationToken ct = default); Task ExportTimesheetsCsvAsync(DateOnly? startDate, DateOnly? endDate, CancellationToken ct = default); Task> GetBlogsAsync(bool includeFormerEmployees = false, CancellationToken ct = default); Task> GetProjectsSummaryAsync(string employeeId, DateOnly startDate, DateOnly endDate, CancellationToken ct = default); @@ -384,6 +390,20 @@ public async Task CancelLeaveAsync(string leaveId, CancelLeaveRequest request, C await PutAsync($"/api/leave/{Uri.EscapeDataString(leaveId)}/cancel", request, ct); } + public async Task GetLeaveBalanceStatusAsync(CancellationToken ct = default) + { + return await GetAsync("/api/leave/balances/status", ct); + } + + public async Task ImportLeaveBalancesAsync( + string csvContent, CancellationToken ct = default) + { + // The endpoint reads the request body as the raw Xero CSV export, so this must not go + // through the JSON helpers — JsonContent would send an escaped string literal. + return await PostRawAsync( + "/api/leave/balances/import", csvContent, "text/csv", ct); + } + // ───────────────────────── Export ───────────────────────── public async Task ExportTimesheetsCsvAsync( @@ -802,6 +822,28 @@ private async Task GetBytesAsync(string relativeUrl, CancellationToken c return System.Text.Json.JsonSerializer.Deserialize(content, ReadJsonOptions); } + /// + /// POSTs a body verbatim under an explicit content type, for endpoints that read the raw + /// request stream rather than a JSON payload (currently the Xero leave balance CSV import). + /// + private async Task PostRawAsync(string relativeUrl, string content, string contentType, CancellationToken ct) + { + using var request = new HttpRequestMessage(HttpMethod.Post, relativeUrl) + { + Content = new StringContent(content, Encoding.UTF8, contentType) + }; + ConfigureRequest(request); + + using var response = await _http.SendAsync(request, ct); + await EnsureSuccessAsync(response, ct); + + var body = await response.Content.ReadAsStringAsync(ct); + if (string.IsNullOrWhiteSpace(body)) + return default; + + return JsonSerializer.Deserialize(body, ReadJsonOptions); + } + private async Task PutAsync(string relativeUrl, object body, CancellationToken ct) { using var request = new HttpRequestMessage(HttpMethod.Put, relativeUrl) diff --git a/src/SSW.TimePro.Cli/Program.cs b/src/SSW.TimePro.Cli/Program.cs index f6ea7b4..6c7834f 100644 --- a/src/SSW.TimePro.Cli/Program.cs +++ b/src/SSW.TimePro.Cli/Program.cs @@ -29,6 +29,8 @@ using LeaveUpdate = SSW.TimePro.Cli.Features.Leave.UpdateCommand; using LeaveCancel = SSW.TimePro.Cli.Features.Leave.CancelCommand; using LeaveBalance = SSW.TimePro.Cli.Features.Leave.BalanceCommand; +using LeaveBalancesStatus = SSW.TimePro.Cli.Features.Leave.BalancesStatusCommand; +using LeaveBalancesImport = SSW.TimePro.Cli.Features.Leave.BalancesImportCommand; using InvList = SSW.TimePro.Cli.Features.Invoices.ListCommand; using InvGet = SSW.TimePro.Cli.Features.Invoices.GetCommand; using InvLines = SSW.TimePro.Cli.Features.Invoices.LinesCommand; @@ -113,6 +115,7 @@ services.AddHttpClient(); services.AddSingleton(); services.AddSingleton(); +services.AddSingleton(); var registrar = new TypeRegistrar(services); @@ -215,6 +218,17 @@ void RegisterLeaveCommands(IConfigurator branch) .WithDescription("Cancel a leave request"); branch.AddCommand("balance") .WithDescription("Show leave stats (days since last leave, leave taken in last 12 months)"); + + // "balances" (plural) is the company-wide Xero balance sync, distinct from the + // per-employee "balance" stats command above. + branch.AddBranch("balances", balances => + { + balances.SetDescription("Manage imported leave balances (Xero sync)"); + balances.AddCommand("status") + .WithDescription("Show when leave balances were last imported and whether they are stale"); + balances.AddCommand("import") + .WithDescription("Import leave balances from a Xero CSV export"); + }); } config.AddBranch("leave", lv => diff --git a/src/SSW.TimePro.Cli/Shared/Models/LeaveModels.cs b/src/SSW.TimePro.Cli/Shared/Models/LeaveModels.cs index 8c05809..4916d68 100644 --- a/src/SSW.TimePro.Cli/Shared/Models/LeaveModels.cs +++ b/src/SSW.TimePro.Cli/Shared/Models/LeaveModels.cs @@ -137,3 +137,40 @@ public class CancelLeaveRequest public string LeaveId { get; set; } = string.Empty; public string CancellationReason { get; set; } = string.Empty; } + +/// +/// Current state of the leave balance sync (API "GET /api/leave/balances/status"). +/// Null dates mean nothing has been imported yet. +/// +public class LeaveBalanceStatus +{ + /// Date the imported balances are accurate as at, taken from the Xero export. + public DateOnly? AsAtDate { get; set; } + + public DateTimeOffset? LastImportedAt { get; set; } + + /// Number of employees with a stored balance. + public int EmployeeCount { get; set; } + + /// Server's own judgement that the stored balances are too old to rely on. + public bool IsStale { get; set; } +} + +/// +/// Outcome of a leave balance import (API "POST /api/leave/balances/import"). +/// +public class ImportLeaveBalancesResult +{ + public DateOnly AsAtDate { get; set; } + public int Created { get; set; } + public int Updated { get; set; } + + /// + /// Xero rows whose employee name matched no TimePro employee, or matched more than one. + /// These rows were skipped, not imported. + /// + public List UnmatchedEmployees { get; set; } = []; + + /// Rows that were imported but look wrong (e.g. an implausibly large balance). + public List Warnings { get; set; } = []; +} diff --git a/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs b/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs new file mode 100644 index 0000000..34dfaa6 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs @@ -0,0 +1,146 @@ +using FluentAssertions; +using WireMock.RequestBuilders; +using WireMock.ResponseBuilders; +using Xunit; +using SSW.TimePro.Cli.Infrastructure.ApiClient; + +namespace SSW.TimePro.Cli.Integration.Features; + +public class LeaveBalancesApiTests : TestBase +{ + private const string Csv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\nJohn Smith,Annual Leave,12.50\n"; + + [Fact] + public async Task ImportLeaveBalances_SendsRawCsvBodyWithCsvContentType() + { + WireMock.Given( + Request.Create() + .WithPath("/api/leave/balances/import") + .UsingPost() + ).RespondWith( + Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(""" + { + "asAtDate": "2026-08-01", + "created": 1, + "updated": 2, + "unmatchedEmployees": ["Nobody Here"], + "warnings": ["Jane Doe has an unusually large balance (2500.00 hours) - please verify."] + } + """) + ); + + var result = await ApiClient.ImportLeaveBalancesAsync(Csv, CancellationToken.None); + + result.Should().NotBeNull(); + result!.AsAtDate.Should().Be(new DateOnly(2026, 8, 1)); + result.Created.Should().Be(1); + result.Updated.Should().Be(2); + result.UnmatchedEmployees.Should().Equal("Nobody Here"); + result.Warnings.Should().ContainSingle(); + + // The endpoint reads the request stream as CSV. If this ever regresses to the JSON + // helpers the body becomes an escaped string literal and the server-side parser fails. + var request = WireMock.LogEntries.Should().ContainSingle().Subject.RequestMessage; + request.Body.Should().Be(Csv); + request.Headers!["Content-Type"].ToString().Should().Contain("text/csv"); + } + + [Fact] + public async Task ImportLeaveBalances_WhenForbidden_ThrowsApiExceptionWith403() + { + WireMock.Given( + Request.Create() + .WithPath("/api/leave/balances/import") + .UsingPost() + ).RespondWith( + Response.Create().WithStatusCode(403) + ); + + var act = () => ApiClient.ImportLeaveBalancesAsync(Csv, CancellationToken.None); + + (await act.Should().ThrowAsync()).Which.StatusCode.Should().Be(403); + } + + [Fact] + public async Task ImportLeaveBalances_WhenCsvRejected_ThrowsApiExceptionCarryingServerMessage() + { + WireMock.Given( + Request.Create() + .WithPath("/api/leave/balances/import") + .UsingPost() + ).RespondWith( + Response.Create() + .WithStatusCode(422) + .WithHeader("Content-Type", "application/json") + .WithBody("\"Missing required column 'Units'.\"") + ); + + var act = () => ApiClient.ImportLeaveBalancesAsync(Csv, CancellationToken.None); + + var exception = (await act.Should().ThrowAsync()).Which; + exception.StatusCode.Should().Be(422); + exception.ResponseBody.Should().Contain("Missing required column"); + } + + [Fact] + public async Task GetLeaveBalanceStatus_WithStoredBalances_ReturnsStatus() + { + WireMock.Given( + Request.Create() + .WithPath("/api/leave/balances/status") + .UsingGet() + ).RespondWith( + Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(""" + { + "asAtDate": "2026-08-01", + "lastImportedAt": "2026-08-02T09:00:00+00:00", + "employeeCount": 42, + "isStale": true + } + """) + ); + + var status = await ApiClient.GetLeaveBalanceStatusAsync(CancellationToken.None); + + status.Should().NotBeNull(); + status!.AsAtDate.Should().Be(new DateOnly(2026, 8, 1)); + status.LastImportedAt.Should().Be(new DateTimeOffset(2026, 8, 2, 9, 0, 0, TimeSpan.Zero)); + status.EmployeeCount.Should().Be(42); + status.IsStale.Should().BeTrue(); + } + + [Fact] + public async Task GetLeaveBalanceStatus_WhenNothingImported_ReturnsNullDates() + { + WireMock.Given( + Request.Create() + .WithPath("/api/leave/balances/status") + .UsingGet() + ).RespondWith( + Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(""" + { + "asAtDate": null, + "lastImportedAt": null, + "employeeCount": 0, + "isStale": false + } + """) + ); + + var status = await ApiClient.GetLeaveBalanceStatusAsync(CancellationToken.None); + + status.Should().NotBeNull(); + status!.AsAtDate.Should().BeNull(); + status.LastImportedAt.Should().BeNull(); + status.EmployeeCount.Should().Be(0); + } +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs new file mode 100644 index 0000000..afe37e2 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs @@ -0,0 +1,155 @@ +using System.Text; +using FluentAssertions; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; +using SSW.TimePro.Cli.Features.Leave; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Infrastructure.Config; +using SSW.TimePro.Cli.Infrastructure.DependencyInjection; +using SSW.TimePro.Cli.Shared.Models; +using Spectre.Console.Cli; +using Xunit; + +namespace SSW.TimePro.Cli.Tests.Features.Leave; + +public class BalancesCommandsTests : IDisposable +{ + private const string ValidCsv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n"; + + private readonly string _tempDir = Directory.CreateDirectory( + Path.Combine(Path.GetTempPath(), "tp-balance-cmd-tests", Guid.NewGuid().ToString("N"))).FullName; + + [Fact] + public async Task Import_WithValidCsvAndJson_CallsApiWithoutPrompting() + { + var api = Substitute.For(); + string? sent = null; + api.ImportLeaveBalancesAsync(Arg.Do(v => sent = v), Arg.Any()) + .Returns(new ImportLeaveBalancesResult { AsAtDate = new DateOnly(2026, 8, 1), Created = 3, Updated = 4 }); + var app = CreateApp(api); + var path = WriteFile("balances.csv", ValidCsv); + + // --json implies non-interactive, so no confirmation should be required. + var exitCode = await app.RunAsync(["import", path, "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(0); + sent.Should().Be(ValidCsv); + } + + [Fact] + public async Task Import_WhenFileMissing_FailsBeforeCallingApi() + { + var api = Substitute.For(); + var app = CreateApp(api); + + var exitCode = await app.RunAsync( + ["import", Path.Combine(_tempDir, "nope.csv"), "--json"], + TestContext.Current.CancellationToken); + + exitCode.Should().Be(1); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenNotLoggedIn_FailsBeforeReadingFile() + { + var api = Substitute.For(); + var app = CreateApp(api, loggedIn: false); + var path = WriteFile("balances.csv", ValidCsv); + + var exitCode = await app.RunAsync(["import", path, "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(1); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenApiRejectsCsv_ReturnsFailureExitCode() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException( + 422, "Unprocessable Entity", "\"Missing required column 'Units'.\"")); + var app = CreateApp(api); + var path = WriteFile("balances.csv", ValidCsv); + + var exitCode = await app.RunAsync(["import", path, "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task Status_WhenNothingImported_SucceedsWithoutError() + { + var api = Substitute.For(); + api.GetLeaveBalanceStatusAsync(Arg.Any()) + .Returns((LeaveBalanceStatus?)null); + var app = CreateApp(api); + + var exitCode = await app.RunAsync(["status", "--json"], TestContext.Current.CancellationToken); + + // "Never imported" is a valid state to report, not a failure. + exitCode.Should().Be(0); + } + + [Fact] + public async Task Status_WhenBalancesStored_Succeeds() + { + var api = Substitute.For(); + api.GetLeaveBalanceStatusAsync(Arg.Any()) + .Returns(new LeaveBalanceStatus + { + AsAtDate = new DateOnly(2026, 8, 1), + LastImportedAt = new DateTimeOffset(2026, 8, 2, 9, 0, 0, TimeSpan.Zero), + EmployeeCount = 42, + IsStale = true + }); + var app = CreateApp(api); + + var exitCode = await app.RunAsync(["status", "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(0); + await api.Received(1).GetLeaveBalanceStatusAsync(Arg.Any()); + } + + private string WriteFile(string name, string content) + { + var path = Path.Combine(_tempDir, name); + File.WriteAllText(path, content, new UTF8Encoding(false)); + return path; + } + + private static CommandApp CreateApp(ITimeProApiClient api, bool loggedIn = true) + { + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns(loggedIn + ? new TenantConfig + { + TenantId = "test", + ApiUrl = "https://timepro.example", + ApiKey = "test-api-key", + EmployeeId = "TST" + } + : null); + + var services = new ServiceCollection(); + services.AddSingleton(api); + services.AddSingleton(config); + services.AddSingleton(); + + var app = new CommandApp(new TypeRegistrar(services)); + app.Configure(configurator => + { + configurator.AddCommand("import"); + configurator.AddCommand("status"); + }); + return app; + } + + public void Dispose() + { + if (Directory.Exists(_tempDir)) + Directory.Delete(_tempDir, recursive: true); + GC.SuppressFinalize(this); + } +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs new file mode 100644 index 0000000..da71e55 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs @@ -0,0 +1,183 @@ +using System.Text; +using FluentAssertions; +using NSubstitute; +using SSW.TimePro.Cli.Features.Leave; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Shared.Models; +using Xunit; + +namespace SSW.TimePro.Cli.Tests.Features.Leave; + +public class LeaveBalanceImportServiceTests : IDisposable +{ + private const string ValidCsv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n"; + + private readonly string _tempDir = Directory.CreateDirectory( + Path.Combine(Path.GetTempPath(), "tp-balance-import-tests", Guid.NewGuid().ToString("N"))).FullName; + + [Fact] + public async Task Import_WithValidCsv_SendsFileContentsVerbatim() + { + var api = Substitute.For(); + string? sent = null; + api.ImportLeaveBalancesAsync(Arg.Do(value => sent = value), Arg.Any()) + .Returns(new ImportLeaveBalancesResult + { + AsAtDate = new DateOnly(2026, 8, 1), + Created = 1, + Updated = 2, + UnmatchedEmployees = ["Nobody Here"], + Warnings = ["Jane Doe has an unusually large balance"] + }); + + var path = WriteFile("balances.csv", ValidCsv); + var service = new LeaveBalanceImportService(api); + + var result = await service.ImportAsync(path, TestContext.Current.CancellationToken); + + // The server parses the CSV, so it must arrive exactly as written — no re-encoding. + sent.Should().Be(ValidCsv); + result.Created.Should().Be(1); + result.Updated.Should().Be(2); + result.UnmatchedEmployees.Should().Equal("Nobody Here"); + result.Warnings.Should().ContainSingle(); + } + + [Fact] + public async Task Import_WhenFileMissing_DoesNotCallApi() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + + var act = () => service.ImportAsync(Path.Combine(_tempDir, "nope.csv"), TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("File not found:*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenPathIsDirectory_DoesNotCallApi() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + + var act = () => service.ImportAsync(_tempDir, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*is a directory*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenFileEmpty_DoesNotCallApi() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("empty.csv", string.Empty); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("File is empty:*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenFileIsWhitespaceOnly_DoesNotCallApi() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("blank.csv", " \n\n"); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("File contains no data:*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenFileIsBinary_DoesNotCallApi() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + var path = Path.Combine(_tempDir, "balances.xlsx"); + File.WriteAllBytes(path, [0x50, 0x4B, 0x03, 0x04, 0x00, 0x00, 0x01]); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*looks like a binary file*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenApiReturnsForbidden_ExplainsAdminRequirement() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException(403, "Forbidden", null)); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*leave admin rights*"); + } + + [Fact] + public async Task Import_WhenApiRejectsCsv_SurfacesServerMessage() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException( + 422, "Unprocessable Entity", "\"Missing required column 'Units'.\"")); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*Missing required column 'Units'.*"); + } + + [Fact] + public async Task Import_WhenApiFailsForAnotherReason_LeavesApiExceptionUnwrapped() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException(500, "Server Error", null)); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + // 5xx should reach the caller as-is so the response body stays diagnosable. + await act.Should().ThrowAsync(); + } + + [Theory] + [InlineData("\"Missing required column 'Units'.\"", "Missing required column 'Units'.")] + [InlineData("Not json at all", "Not json at all")] + [InlineData("", "the file was rejected without a reason.")] + public void DescribeUnprocessable_UnwrapsServerBody(string body, string expected) + { + LeaveBalanceImportService.DescribeUnprocessable(body).Should().Be(expected); + } + + private string WriteFile(string name, string content) + { + var path = Path.Combine(_tempDir, name); + File.WriteAllText(path, content, new UTF8Encoding(false)); + return path; + } + + public void Dispose() + { + if (Directory.Exists(_tempDir)) + Directory.Delete(_tempDir, recursive: true); + GC.SuppressFinalize(this); + } +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs index 8a1496c..6e2c289 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs @@ -265,8 +265,116 @@ public async Task UpdateLeave_WhenChangingNote_PreservesExistingFields() .NotContain(call => call.GetMethodInfo().Name == nameof(ITimeProApiClient.UpdateLeaveAsync)); } + [Fact] + public async Task ImportLeaveBalances_WithCsvPath_SendsFileContentsAndReportsSkippedRows() + { + var api = Substitute.For(); + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns(new TenantConfig + { + TenantId = "test", + ApiUrl = "https://timepro.example", + ApiKey = "test-api-key", + EmployeeId = "TST" + }); + + const string csv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n"; + string? sent = null; + api.ImportLeaveBalancesAsync(Arg.Do(value => sent = value), Arg.Any()) + .Returns(new ImportLeaveBalancesResult + { + AsAtDate = new DateOnly(2026, 8, 1), + Created = 1, + Updated = 2, + UnmatchedEmployees = ["Nobody Here"], + Warnings = ["Jane Doe has an unusually large balance"] + }); + + var path = Path.Combine(Path.GetTempPath(), $"tp-mcp-balances-{Guid.NewGuid():N}.csv"); + await File.WriteAllTextAsync(path, csv, TestContext.Current.CancellationToken); + + try + { + var tools = CreateTools(api, config); + var json = await tools.ImportLeaveBalances(path, TestContext.Current.CancellationToken); + + sent.Should().Be(csv); + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("success").GetBoolean().Should().BeTrue(); + doc.RootElement.GetProperty("created").GetInt32().Should().Be(1); + doc.RootElement.GetProperty("updated").GetInt32().Should().Be(2); + // Skipped rows must reach the agent so it can tell the user who was missed. + doc.RootElement.GetProperty("unmatchedEmployees").EnumerateArray() + .Select(e => e.GetString()).Should().Equal("Nobody Here"); + doc.RootElement.GetProperty("warnings").GetArrayLength().Should().Be(1); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public async Task ImportLeaveBalances_WhenFileMissing_ReturnsErrorWithoutCallingApi() + { + var api = Substitute.For(); + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns(new TenantConfig + { + TenantId = "test", + ApiUrl = "https://timepro.example", + ApiKey = "test-api-key", + EmployeeId = "TST" + }); + var tools = CreateTools(api, config); + + var json = await tools.ImportLeaveBalances( + Path.Combine(Path.GetTempPath(), $"tp-missing-{Guid.NewGuid():N}.csv"), + TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("error").GetString().Should().StartWith("File not found"); + await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ImportLeaveBalances_WhenNotLoggedIn_ReturnsErrorWithoutCallingApi() + { + var api = Substitute.For(); + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns((TenantConfig?)null); + var tools = CreateTools(api, config); + + var json = await tools.ImportLeaveBalances("balances.csv", TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("error").GetString().Should().Contain("Not logged in"); + await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task GetLeaveBalanceStatus_WhenNothingImported_ReportsNotImported() + { + var api = Substitute.For(); + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns(new TenantConfig + { + TenantId = "test", + ApiUrl = "https://timepro.example", + ApiKey = "test-api-key", + EmployeeId = "TST" + }); + api.GetLeaveBalanceStatusAsync(Arg.Any()).Returns((LeaveBalanceStatus?)null); + var tools = CreateTools(api, config); + + var json = await tools.GetLeaveBalanceStatus(TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("imported").GetBoolean().Should().BeFalse(); + } + private static LeaveMcpTools CreateTools(ITimeProApiClient api, IConfigService config) => - new(api, config, new LeaveCreateService(api), new LeaveUpdateService(api)); + new(api, config, new LeaveCreateService(api), new LeaveUpdateService(api), new LeaveBalanceImportService(api)); private static (string id, TimeZoneInfo timeZone) FindTimeZone(params string[] ids) { From caa85267a4dfaa87c1f2d3896462d64cdfcd16a4 Mon Sep 17 00:00:00 2001 From: Hugo Pernet Date: Fri, 28 Aug 2026 16:08:20 +0200 Subject: [PATCH 2/7] Show the original path when a balances CSV path resolves elsewhere A shell that strips backslashes turns an absolute Windows path into a drive-relative one ("C:\Users\me\x.csv" -> "C:Usersmex.csv"), which resolves against the working directory. The resolved path alone gives no hint that this happened, so report the original alongside it when the two differ. Co-Authored-By: Claude Opus 5 (1M context) --- .../Features/Leave/LeaveBalanceImportService.cs | 15 ++++++++++++++- .../Leave/LeaveBalanceImportServiceTests.cs | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs index 5023980..c98949c 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs @@ -50,7 +50,7 @@ public string ReadCsv(string csvPath) throw new LeaveBalanceImportValidationException($"'{fullPath}' is a directory. Provide the path to the CSV file itself."); if (!File.Exists(fullPath)) - throw new LeaveBalanceImportValidationException($"File not found: {fullPath}"); + throw new LeaveBalanceImportValidationException($"File not found: {Describe(fullPath, csvPath)}"); var length = new FileInfo(fullPath).Length; if (length == 0) @@ -111,6 +111,19 @@ public async Task ImportAsync(string csvPath, Cancell } } + /// + /// Names the resolved path, and the original alongside it when the two differ. A shell that + /// strips backslashes turns an absolute Windows path into a drive-relative one that resolves + /// somewhere unexpected, which is impossible to spot from the resolved path alone. + /// + private static string Describe(string fullPath, string originalPath) + { + var trimmedOriginal = originalPath.Trim(); + return string.Equals(fullPath, trimmedOriginal, StringComparison.Ordinal) + ? fullPath + : $"{fullPath} (resolved from '{trimmedOriginal}')"; + } + /// /// Unwraps the bare JSON string body returned by the endpoint's 422, falling back to the /// shared problem+json parsing for anything else. diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs index da71e55..b428927 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs @@ -56,6 +56,21 @@ public async Task Import_WhenFileMissing_DoesNotCallApi() api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); } + [Fact] + public async Task Import_WhenRelativePathResolvesElsewhere_ReportsBothPaths() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + + // A shell that strips backslashes turns "C:\Users\me\x.csv" into a drive-relative + // "C:Usersmex.csv", which silently resolves against the working directory. + var act = () => service.ImportAsync("Usersmexero.csv", TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*resolved from 'Usersmexero.csv'*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + [Fact] public async Task Import_WhenPathIsDirectory_DoesNotCallApi() { From 4ab3a6b3383202ce6c6ebfe2524ca30ba819d065 Mon Sep 17 00:00:00 2001 From: Hugo Pernet Date: Fri, 28 Aug 2026 16:37:56 +0200 Subject: [PATCH 3/7] Trim the import_leave_balances tool description At 618 characters it was 2.4x the next longest tool description in the server and 7x the median. Dropped the leave-admin pre-warning (the service already returns a clear message when the API refuses), the pointer to get_leave_balance_status (that tool's own description already says to check it before importing), and the gloss on what unmatchedEmployees means. Kept the parts a model gets wrong without them: the blast radius, that there is no undo, that it takes a path rather than file contents, and that skipped rows must be surfaced. 337 characters. Co-Authored-By: Claude Opus 5 (1M context) --- src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs index ca159f3..023f3d1 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs @@ -218,11 +218,9 @@ public async Task GetLeaveBalanceStatus(CancellationToken ct = default) [McpServerTool] [Description( - "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing the balances currently stored in TimePro. " - + "Requires leave admin rights; other accounts get a permission error. There is no dry run and no undo, so confirm with the user before calling this. " - + "Pass the path to the CSV file on this machine - do not paste the file contents. " - + "Call GetLeaveBalanceStatus first to check whether an import is due. " - + "Report the returned unmatchedEmployees (rows skipped because the Xero name matched no TimePro employee, or matched several) and warnings back to the user - the import succeeds despite them.")] + "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing what TimePro currently stores. " + + "No dry run and no undo, so confirm with the user first. Pass the path to the CSV file, not its contents. " + + "Always report the returned unmatchedEmployees (rows skipped) and warnings - the import succeeds despite them.")] public async Task ImportLeaveBalances( [Description("Path to the Xero 'Leave Balances' CSV export on this machine")] string csvPath, CancellationToken ct = default) From 9eb1cb24823e8941da473d9ab92419cbebfc7d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jernej=20Kavka=20=28JK=29=20=5BSSW=20=C2=B7=20Microsoft=20?= =?UTF-8?q?MVP=5D?= Date: Sat, 29 Aug 2026 10:30:46 +1000 Subject: [PATCH 4/7] feat(leave): refine balance import surfaces --- AGENTS.md | 3 + README.md | 10 +- docs/skill-generation.md | 3 +- .../Features/Leave/BalancesStatusCommand.cs | 2 +- .../Leave/LeaveBalanceImportService.cs | 29 ++++- .../Features/Mcp/Tools/AccountingMcpTools.cs | 42 ++++++- .../Features/Mcp/Tools/LeaveMcpTools.cs | 38 +----- .../Features/Skills/SkillModelBuilder.cs | 4 +- .../Templates/timepro-accounting-cli.md | 33 +++++- .../Infrastructure/Config/FeatureCatalog.cs | 2 +- .../Features/AccountingApiTests.cs | 4 +- .../Features/LeaveBalancesApiTests.cs | 4 +- .../Features/LeaveTests.cs | 37 +++--- .../Features/TimesheetGetTests.cs | 8 +- .../Features/Leave/BalancesCommandsTests.cs | 8 +- .../Leave/LeaveBalanceImportServiceTests.cs | 23 ++++ .../Features/Mcp/AccountingMcpToolsTests.cs | 108 ++++++++++++++++++ .../Features/Mcp/LeaveMcpToolsTests.cs | 95 ++------------- .../Features/Skills/SkillGenerationTests.cs | 7 +- 19 files changed, 299 insertions(+), 161 deletions(-) create mode 100644 tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs diff --git a/AGENTS.md b/AGENTS.md index 3abef26..42831c9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,6 +120,9 @@ through an agent's context, where a large CSV is expensive and liable to be sile truncated into a partial import that still looks successful. `GET /api/leave/balances/status` is the cheap read used to decide whether a re-import is due. +The read-only MCP status tool is on the default leave surface. The destructive MCP import +tool is available only when the accounting feature pack is enabled. + Rows whose Xero employee name matches no TimePro employee, or matches several, are returned in `unmatchedEmployees` and skipped rather than guessed at; implausible balances are returned in `warnings`. The import succeeds regardless, so both lists must be surfaced to the user. diff --git a/README.md b/README.md index f87860f..8fd407a 100644 --- a/README.md +++ b/README.md @@ -280,9 +280,9 @@ match. It requires leave admin rights, has no dry-run and no undo, so it prompts pass `--yes`. Rows whose Xero name matches no TimePro employee — or matches more than one — are reported as skipped rather than guessed at, and implausibly large balances are flagged as warnings; the import still succeeds, so check both lists afterwards. Run -`tp leave balances status` first to see whether a re-import is actually due. The MCP tool -takes the path to the CSV rather than its contents, so the file never has to pass through -an agent's context. +`tp leave balances status` first to see whether a re-import is actually due. The accounting +MCP tool takes the path to the CSV rather than its contents, so the file never has to pass +through an agent's context. ### Week View @@ -587,7 +587,7 @@ Current default tool groups include: |-------|----------| | Timesheets | Get, create, update, delete, suggested timesheets, accept suggestions, list iterations, `check_week` (leave-aware weekly coverage) | | Lookup | Search clients, list projects, get client rate, CRM bookings, location and repo mapping | -| Leave | List EasyLeave entries (optionally filtered by `empId`), create and safely update EasyLeave requests with dry-run previews, `get_leave_balance` (days since last leave + 12-month hours), `get_leave_balance_status` and `import_leave_balances` (Xero balance sync, leave admins only) | +| Leave | List EasyLeave entries (optionally filtered by `empId`), create and safely update EasyLeave requests with dry-run previews, `get_leave_balance` (days since last leave + 12-month hours), and `get_leave_balance_status` (Xero balance sync status) | Optional accounting MCP tools are enabled with: @@ -595,7 +595,7 @@ Optional accounting MCP tools are enabled with: tp feature accounting enable ``` -That adds invoices, receipts, credit notes, products/SKUs, client rates, unbilled time, timesheet queries, current user/reference-code reporting, recurring invoices, and prepaid drawdown status. More complex accounting diagnostics live in guide-backed Markdown skills so teams can extend the collection without adding a dedicated command for every report. +That adds invoices, receipts, credit notes, products/SKUs, client rates, unbilled time, timesheet queries, current user/reference-code reporting, recurring invoices, prepaid drawdown status, and the leave-admin-only `import_leave_balances` tool. More complex accounting diagnostics live in guide-backed Markdown skills so teams can extend the collection without adding a dedicated command for every report. Developer diagnostics are CLI/skill workflows. Enable the generated developer skills with: diff --git a/docs/skill-generation.md b/docs/skill-generation.md index c7d39d7..b1e95b7 100644 --- a/docs/skill-generation.md +++ b/docs/skill-generation.md @@ -119,7 +119,8 @@ The tenant setup skill keeps: The accounting skill keeps: - `allowed-tools: Bash(tp *)` -- instruction-only read-only accounting workflows +- instruction-only accounting workflows, read-only except for explicitly approved Xero leave-balance imports +- default-MCP balance status plus accounting-gated MCP import guidance - client billable-work threshold report guidance, including the `.rows` JSON envelope shape - deeper reconciliation diagnostics for Excel, CSV, Xero MCP, bank-feed MCP, or another external source - guidance to check `tp accounting guide` first, then use specific recipes under `guides/accounting/` diff --git a/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs b/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs index e88723c..961f6b0 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/BalancesStatusCommand.cs @@ -41,7 +41,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings try { var status = await _api.GetLeaveBalanceStatusAsync(cancellationToken); - if (status is null) + if (status?.LastImportedAt is null) { // Nothing imported yet is a valid state, not a failure. if (settings.Json) diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs index c98949c..60ffc9b 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs @@ -39,7 +39,7 @@ public string ReadCsv(string csvPath) string fullPath; try { - fullPath = Path.GetFullPath(csvPath.Trim()); + fullPath = Path.GetFullPath(ExpandHomeDirectory(csvPath)); } catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) { @@ -84,6 +84,33 @@ public string ReadCsv(string csvPath) return content; } + /// + /// Expands the shell-style current-user home shorthand because MCP arguments are passed + /// directly to the process and do not go through shell expansion. + /// + internal static string ExpandHomeDirectory(string path) + { + var trimmedPath = path.Trim(); + if (trimmedPath != "~" + && !trimmedPath.StartsWith("~/", StringComparison.Ordinal) + && !trimmedPath.StartsWith(@"~\", StringComparison.Ordinal)) + { + return trimmedPath; + } + + var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); + if (string.IsNullOrWhiteSpace(userProfile)) + return trimmedPath; + + if (trimmedPath.Length == 1) + return userProfile; + + var relativePath = trimmedPath[2..] + .Replace('/', Path.DirectorySeparatorChar) + .Replace('\\', Path.DirectorySeparatorChar); + return Path.Combine(userProfile, relativePath); + } + /// /// Reads the CSV and sends it to TimePro. The server owns parsing and matching, so its /// rejection messages are surfaced verbatim rather than second-guessed here. diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs index e89e666..de132f9 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs @@ -1,6 +1,7 @@ using System.ComponentModel; using System.Text.Json; using ModelContextProtocol.Server; +using SSW.TimePro.Cli.Features.Leave; using SSW.TimePro.Cli.Infrastructure.ApiClient; using SSW.TimePro.Cli.Infrastructure.Config; using SSW.TimePro.Cli.Shared.Models; @@ -8,7 +9,7 @@ namespace SSW.TimePro.Cli.Features.Mcp.Tools; /// -/// MCP tools for accountant-focused read-only operations. Also exposes cross-domain read +/// MCP tools for accountant-focused operations. Also exposes cross-domain read /// tools useful to accountants (timesheet queries, product lists, rate tables, etc.) that /// aren't already on or . /// @@ -20,6 +21,7 @@ public class AccountingMcpTools { private readonly ITimeProApiClient _api; private readonly IConfigService _config; + private readonly LeaveBalanceImportService _leaveBalanceImportService; private static readonly JsonSerializerOptions JsonOpts = new() { @@ -28,10 +30,14 @@ public class AccountingMcpTools DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull }; - public AccountingMcpTools(ITimeProApiClient api, IConfigService config) + public AccountingMcpTools( + ITimeProApiClient api, + IConfigService config, + LeaveBalanceImportService leaveBalanceImportService) { _api = api; _config = config; + _leaveBalanceImportService = leaveBalanceImportService; } private bool NotAuthed(out string error) @@ -60,6 +66,38 @@ private static List ResolveEmpIds(string[]? empIds, string[]? employeeId .ToList() ?? []; } + // ─── Leave balance import ─────────────────────────────────────────────── + + [McpServerTool] + [Description( + "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing what TimePro currently stores. " + + "No dry run and no undo, so confirm with the user first. Pass the path to the CSV file, not its contents. " + + "Always report the returned unmatchedEmployees (rows skipped) and warnings - the import succeeds despite them.")] + public async Task ImportLeaveBalances( + [Description("Path to the Xero 'Leave Balances' CSV export on this machine")] string csvPath, + CancellationToken ct = default) + { + if (NotAuthed(out var err)) return err; + + try + { + var result = await _leaveBalanceImportService.ImportAsync(csvPath, ct); + return JsonSerializer.Serialize(new + { + success = true, + result.AsAtDate, + result.Created, + result.Updated, + result.UnmatchedEmployees, + result.Warnings + }, JsonOpts); + } + catch (LeaveBalanceImportValidationException ex) + { + return JsonSerializer.Serialize(new { error = ex.Message }, JsonOpts); + } + } + // ─── Invoices ─────────────────────────────────────────────────────────── [McpServerTool] diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs index 023f3d1..ab9700a 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs @@ -14,7 +14,6 @@ public class LeaveMcpTools private readonly IConfigService _config; private readonly LeaveCreateService _leaveCreateService; private readonly LeaveUpdateService _leaveUpdateService; - private readonly LeaveBalanceImportService _leaveBalanceImportService; private static readonly JsonSerializerOptions JsonOpts = new() { @@ -26,14 +25,12 @@ public LeaveMcpTools( ITimeProApiClient api, IConfigService config, LeaveCreateService leaveCreateService, - LeaveUpdateService leaveUpdateService, - LeaveBalanceImportService leaveBalanceImportService) + LeaveUpdateService leaveUpdateService) { _api = api; _config = config; _leaveCreateService = leaveCreateService; _leaveUpdateService = leaveUpdateService; - _leaveBalanceImportService = leaveBalanceImportService; } [McpServerTool] @@ -210,43 +207,12 @@ public async Task GetLeaveBalanceStatus(CancellationToken ct = default) return """{"error": "Not logged in. Run 'tp login --tenant ' first."}"""; var status = await _api.GetLeaveBalanceStatusAsync(ct); - if (status is null) + if (status?.LastImportedAt is null) return JsonSerializer.Serialize(new { imported = false }, JsonOpts); return JsonSerializer.Serialize(status, JsonOpts); } - [McpServerTool] - [Description( - "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing what TimePro currently stores. " - + "No dry run and no undo, so confirm with the user first. Pass the path to the CSV file, not its contents. " - + "Always report the returned unmatchedEmployees (rows skipped) and warnings - the import succeeds despite them.")] - public async Task ImportLeaveBalances( - [Description("Path to the Xero 'Leave Balances' CSV export on this machine")] string csvPath, - CancellationToken ct = default) - { - if (_config.LoadActiveTenantConfig() is null) - return """{"error": "Not logged in. Run 'tp login --tenant ' first."}"""; - - try - { - var result = await _leaveBalanceImportService.ImportAsync(csvPath, ct); - return JsonSerializer.Serialize(new - { - success = true, - result.AsAtDate, - result.Created, - result.Updated, - result.UnmatchedEmployees, - result.Warnings - }, JsonOpts); - } - catch (LeaveBalanceImportValidationException ex) - { - return JsonSerializer.Serialize(new { error = ex.Message }, JsonOpts); - } - } - private static string? ResolveEmpId(string? empId, string? employeeId) { var requestedEmpId = !string.IsNullOrWhiteSpace(empId) ? empId : employeeId; diff --git a/src/SSW.TimePro.Cli/Features/Skills/SkillModelBuilder.cs b/src/SSW.TimePro.Cli/Features/Skills/SkillModelBuilder.cs index 87cbeb3..4a12d23 100644 --- a/src/SSW.TimePro.Cli/Features/Skills/SkillModelBuilder.cs +++ b/src/SSW.TimePro.Cli/Features/Skills/SkillModelBuilder.cs @@ -16,13 +16,13 @@ public static class SkillModelBuilder public const string DeveloperTimesheetDiagnosticsName = "timepro-dev-timesheet-diagnostics"; public const string DeveloperFinanceDiagnosticsName = "timepro-dev-finance-diagnostics"; public const string EnvironmentCompareName = "timepro-env-compare"; - public const int CurrentSkillVersion = 1; + public const int CurrentSkillVersion = 2; private const string TimesheetsDescription = "Manage SSW TimePro timesheets with the tp CLI — view/accept/create entries, repo mappings, bookings, leave, and daily scrum. Use when entering, fixing, or reviewing timesheets."; private const string AccountingDescription = - "Explore SSW TimePro financial data via the tp CLI (read-only) — invoices with line items, billed timesheets, credit notes, receipts, sale products, client rates, aged debtors, unbilled time, recurring invoices, prepaid drawdowns and client billable-work threshold reports. Use for accountant-style questions. For raw HTTP/curl access (when tp isn't installed), use the timepro-accounting skill instead."; + "Explore SSW TimePro financial data via the tp CLI — read-only invoices, receipts, rates and reconciliation reports, plus explicitly approved Xero leave-balance imports. Use for accountant-style questions. For raw HTTP/curl access (when tp isn't installed), use the timepro-accounting skill instead."; private const string TenantSetupDescription = "Set up and switch TimePro tenant profiles with the tp CLI, including switching the active session to ssw-staging and using process-local --tenant/--env overrides without changing the active tenant."; diff --git a/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-accounting-cli.md b/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-accounting-cli.md index dc24ed8..bd0789e 100644 --- a/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-accounting-cli.md +++ b/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-accounting-cli.md @@ -6,7 +6,7 @@ Do not install this file directly as an agent skill. # TimePro Accounting (CLI) -Accountant-facing read-only access to SSW TimePro via the `tp` CLI. Pipe `--json` output into `jq` or Python to calculate totals, compare against Xero, or audit historical data. +Accountant-facing access to SSW TimePro via the `tp` CLI. The financial exploration workflows are read-only; the Xero leave-balance import is the one explicitly documented write and requires direct user approval. Pipe `--json` output into `jq` or Python to calculate totals, compare against Xero, or audit historical data. ## Setup This skill reuses the tenant config already configured for `tp`. If `tp login` has been run, nothing else is needed. Otherwise run `tp login --tenant ` first. @@ -72,10 +72,36 @@ tp prepaid status --output /tmp/prepaid.pdf # Cross-employee/client/project timesheet query tp query --from 2026-03-01 --to 2026-03-31 --json tp query --from 2026-03-01 --to 2026-03-31 --client --json + +# Xero leave balances +tp leave balances status --json +tp leave balances import ./LeaveBalances.csv --yes --json ``` ## Common workflows +### Import Xero leave balances (destructive) +This import replaces stored leave balances company-wide, has no dry-run, and cannot be +undone through TimePro. Do not run it from an inferred request. Obtain explicit user +approval for the exact file and tenant immediately before importing. + +1. Run `tp info --json` and report the selected tenant. +2. Run `tp leave balances status --json` and report the current as-at/import dates, + employee count, and staleness. +3. Confirm the local CSV path with the user. Do not paste the CSV into an agent prompt. +4. After approval, run: + +```bash +tp leave balances import ./LeaveBalances.csv --yes --json +``` + +Always report `created`, `updated`, `unmatchedEmployees`, and `warnings`. A successful +request may still skip unmatched rows. + +For MCP, `get_leave_balance_status` is on the default TimePro surface, while the destructive +`import_leave_balances` tool is available only after `tp feature accounting enable`. The +same explicit approval and result-reporting rules apply. + ### Drill into an invoice ```bash INV=142 @@ -220,8 +246,9 @@ Prefer the accounting guide before manually stitching primitives: - `guides/accounting/invoice-evidence-pack.md` assembles an invoice evidence pack from header, lines, allocated/write-off timesheets, receipts, and credit notes. - `guides/accounting/client-accounting-position.md` assembles client-level invoice, debt, unbilled, credit note, rate, and external comparison evidence. -If using `tp mcp` with accounting enabled, MCP exposes primitive read-only tools. -Use skills or guide-backed markdown to compose multi-step diagnostics locally. +If using `tp mcp` with accounting enabled, MCP exposes primitive read-only tools plus the +explicitly approved `import_leave_balances` write. Use skills or guide-backed markdown to +compose multi-step diagnostics locally. Enable this MCP surface once with: diff --git a/src/SSW.TimePro.Cli/Infrastructure/Config/FeatureCatalog.cs b/src/SSW.TimePro.Cli/Infrastructure/Config/FeatureCatalog.cs index 331c263..8c5b9ea 100644 --- a/src/SSW.TimePro.Cli/Infrastructure/Config/FeatureCatalog.cs +++ b/src/SSW.TimePro.Cli/Infrastructure/Config/FeatureCatalog.cs @@ -17,7 +17,7 @@ public static class FeatureCatalog new( Accounting, "Accounting", - "Accounting skills, guide topics, and read-only accounting MCP tools.", + "Accounting skills, guide topics, and accounting MCP tools including guarded leave-balance import.", Version: 1, Aliases: ["accounts", "accountant"]), new( diff --git a/tests/SSW.TimePro.Cli.Integration/Features/AccountingApiTests.cs b/tests/SSW.TimePro.Cli.Integration/Features/AccountingApiTests.cs index 7470398..3edcf52 100644 --- a/tests/SSW.TimePro.Cli.Integration/Features/AccountingApiTests.cs +++ b/tests/SSW.TimePro.Cli.Integration/Features/AccountingApiTests.cs @@ -118,7 +118,9 @@ public async Task GetInvoiceTimesheets_WriteOff_HitsWriteOffPath() var rows = await ApiClient.GetInvoiceTimesheetsAsync(142, "writeoff", CancellationToken.None); rows.Should().BeEmpty(); - WireMock.LogEntries.First().RequestMessage.AbsolutePath.Should().EndWith("/WriteOff"); + var requestMessage = WireMock.LogEntries.First().RequestMessage; + requestMessage.Should().NotBeNull(); + requestMessage!.AbsolutePath.Should().EndWith("/WriteOff"); } [Fact] diff --git a/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs b/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs index 34dfaa6..b24c52d 100644 --- a/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs +++ b/tests/SSW.TimePro.Cli.Integration/Features/LeaveBalancesApiTests.cs @@ -44,7 +44,9 @@ public async Task ImportLeaveBalances_SendsRawCsvBodyWithCsvContentType() // The endpoint reads the request stream as CSV. If this ever regresses to the JSON // helpers the body becomes an escaped string literal and the server-side parser fails. var request = WireMock.LogEntries.Should().ContainSingle().Subject.RequestMessage; - request.Body.Should().Be(Csv); + request.Should().NotBeNull(); + request!.Body.Should().Be(Csv); + request.Headers.Should().NotBeNull(); request.Headers!["Content-Type"].ToString().Should().Contain("text/csv"); } diff --git a/tests/SSW.TimePro.Cli.Integration/Features/LeaveTests.cs b/tests/SSW.TimePro.Cli.Integration/Features/LeaveTests.cs index 428572e..d5f7bbf 100644 --- a/tests/SSW.TimePro.Cli.Integration/Features/LeaveTests.cs +++ b/tests/SSW.TimePro.Cli.Integration/Features/LeaveTests.cs @@ -197,17 +197,20 @@ public async Task CreateLeave_WithValidRequest_SendsCorrectPayload() entries.Should().HaveCount(1); var logEntry = entries.First(); - logEntry.RequestMessage.Method.Should().Be("POST"); - logEntry.RequestMessage.Path.Should().Be("/api/leave/"); + var requestMessage = logEntry.RequestMessage; + requestMessage.Should().NotBeNull(); + requestMessage!.Method.Should().Be("POST"); + requestMessage.Path.Should().Be("/api/leave/"); // Verify headers - logEntry.RequestMessage.Headers.Should().ContainKey("x-timepro-tenant-id"); - logEntry.RequestMessage.Headers!["x-timepro-tenant-id"].Should().Contain("test"); - logEntry.RequestMessage.Headers.Should().ContainKey("x-timepro-api-key"); - logEntry.RequestMessage.Headers!["x-timepro-api-key"].Should().Contain("test-api-key"); + requestMessage.Headers.Should().NotBeNull(); + requestMessage.Headers!.Should().ContainKey("x-timepro-tenant-id"); + requestMessage.Headers["x-timepro-tenant-id"].Should().Contain("test"); + requestMessage.Headers.Should().ContainKey("x-timepro-api-key"); + requestMessage.Headers["x-timepro-api-key"].Should().Contain("test-api-key"); // Verify body contains required fields - var body = logEntry.RequestMessage.Body; + var body = requestMessage.Body; body.Should().NotBeNullOrEmpty(); var doc = JsonDocument.Parse(body!); var root = doc.RootElement; @@ -250,7 +253,9 @@ public async Task CreateLeave_WithMinimalRequest_OmitsNullFields() await ApiClient.CreateLeaveAsync(request, CancellationToken.None); // Assert - var body = WireMock.LogEntries.First().RequestMessage.Body; + var requestMessage = WireMock.LogEntries.First().RequestMessage; + requestMessage.Should().NotBeNull(); + var body = requestMessage!.Body; var doc = JsonDocument.Parse(body!); var root = doc.RootElement; @@ -410,11 +415,13 @@ public async Task CancelLeave_WithValidRequest_SendsCorrectPayload() entries.Should().HaveCount(1); var logEntry = entries.First(); - logEntry.RequestMessage.Method.Should().Be("PUT"); - logEntry.RequestMessage.Path.Should().Be($"/api/leave/{leaveId}/cancel"); + var requestMessage = logEntry.RequestMessage; + requestMessage.Should().NotBeNull(); + requestMessage!.Method.Should().Be("PUT"); + requestMessage.Path.Should().Be($"/api/leave/{leaveId}/cancel"); // Verify body contains LeaveId and CancellationReason - var body = logEntry.RequestMessage.Body; + var body = requestMessage.Body; body.Should().NotBeNullOrEmpty(); var doc = JsonDocument.Parse(body!); var root = doc.RootElement; @@ -500,9 +507,11 @@ public async Task CancelLeave_SetsAuthHeaders() // Assert var req = WireMock.LogEntries.First().RequestMessage; - req.Headers.Should().ContainKey("x-timepro-tenant-id"); - req.Headers!["x-timepro-tenant-id"].Should().Contain("test"); + req.Should().NotBeNull(); + req!.Headers.Should().NotBeNull(); + req.Headers!.Should().ContainKey("x-timepro-tenant-id"); + req.Headers["x-timepro-tenant-id"].Should().Contain("test"); req.Headers.Should().ContainKey("x-timepro-api-key"); - req.Headers!["x-timepro-api-key"].Should().Contain("test-api-key"); + req.Headers["x-timepro-api-key"].Should().Contain("test-api-key"); } } diff --git a/tests/SSW.TimePro.Cli.Integration/Features/TimesheetGetTests.cs b/tests/SSW.TimePro.Cli.Integration/Features/TimesheetGetTests.cs index f5acf94..7ce9203 100644 --- a/tests/SSW.TimePro.Cli.Integration/Features/TimesheetGetTests.cs +++ b/tests/SSW.TimePro.Cli.Integration/Features/TimesheetGetTests.cs @@ -78,10 +78,12 @@ public async Task GetTimesheets_SetsAuthHeaders() var entries = WireMock.LogEntries; entries.Should().HaveCount(1); var req = entries.First().RequestMessage; - req.Headers.Should().ContainKey("x-timepro-tenant-id"); - req.Headers!["x-timepro-tenant-id"].Should().Contain("test"); + req.Should().NotBeNull(); + req!.Headers.Should().NotBeNull(); + req.Headers!.Should().ContainKey("x-timepro-tenant-id"); + req.Headers["x-timepro-tenant-id"].Should().Contain("test"); req.Headers.Should().ContainKey("x-timepro-api-key"); - req.Headers!["x-timepro-api-key"].Should().Contain("test-api-key"); + req.Headers["x-timepro-api-key"].Should().Contain("test-api-key"); } [Fact] diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs index afe37e2..e0bcafe 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs @@ -83,7 +83,13 @@ public async Task Status_WhenNothingImported_SucceedsWithoutError() { var api = Substitute.For(); api.GetLeaveBalanceStatusAsync(Arg.Any()) - .Returns((LeaveBalanceStatus?)null); + .Returns(new LeaveBalanceStatus + { + AsAtDate = null, + LastImportedAt = null, + EmployeeCount = 0, + IsStale = false + }); var app = CreateApp(api); var exitCode = await app.RunAsync(["status", "--json"], TestContext.Current.CancellationToken); diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs index b428927..dfad56f 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs @@ -56,6 +56,29 @@ public async Task Import_WhenFileMissing_DoesNotCallApi() api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); } + [Theory] + [InlineData("~/Downloads/LeaveBalances.csv")] + [InlineData(@"~\Downloads\LeaveBalances.csv")] + public void ExpandHomeDirectory_WithTildePrefix_UsesCurrentUserProfile(string path) + { + var expected = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + "Downloads", + "LeaveBalances.csv"); + + var expanded = LeaveBalanceImportService.ExpandHomeDirectory(path); + + expanded.Should().Be(expected); + } + + [Theory] + [InlineData("~northwind/LeaveBalances.csv")] + [InlineData("exports/~/LeaveBalances.csv")] + public void ExpandHomeDirectory_WhenTildeIsNotTheFirstSegment_LeavesPathUnchanged(string path) + { + LeaveBalanceImportService.ExpandHomeDirectory(path).Should().Be(path); + } + [Fact] public async Task Import_WhenRelativePathResolvesElsewhere_ReportsBothPaths() { diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs new file mode 100644 index 0000000..72ffe63 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs @@ -0,0 +1,108 @@ +using System.Text.Json; +using FluentAssertions; +using NSubstitute; +using SSW.TimePro.Cli.Features.Leave; +using SSW.TimePro.Cli.Features.Mcp.Tools; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Infrastructure.Config; +using SSW.TimePro.Cli.Shared.Models; +using Xunit; + +namespace SSW.TimePro.Cli.Tests.Features.Mcp; + +public class AccountingMcpToolsTests +{ + [Fact] + public void BalanceTools_AreSeparatedBetweenDefaultAndAccountingSurfaces() + { + typeof(LeaveMcpTools).GetMethod(nameof(LeaveMcpTools.GetLeaveBalanceStatus)).Should().NotBeNull(); + typeof(LeaveMcpTools).GetMethod(nameof(AccountingMcpTools.ImportLeaveBalances)).Should().BeNull(); + typeof(AccountingMcpTools).GetMethod(nameof(AccountingMcpTools.ImportLeaveBalances)).Should().NotBeNull(); + } + + [Fact] + public async Task ImportLeaveBalances_WithCsvPath_SendsFileContentsAndReportsSkippedRows() + { + var api = Substitute.For(); + var config = CreateConfig(); + const string csv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n"; + string? sent = null; + api.ImportLeaveBalancesAsync(Arg.Do(value => sent = value), Arg.Any()) + .Returns(new ImportLeaveBalancesResult + { + AsAtDate = new DateOnly(2026, 8, 1), + Created = 1, + Updated = 2, + UnmatchedEmployees = ["Nobody Here"], + Warnings = ["Jane Doe has an unusually large balance"] + }); + + var path = Path.Combine(Path.GetTempPath(), $"tp-mcp-balances-{Guid.NewGuid():N}.csv"); + await File.WriteAllTextAsync(path, csv, TestContext.Current.CancellationToken); + + try + { + var tools = CreateTools(api, config); + var json = await tools.ImportLeaveBalances(path, TestContext.Current.CancellationToken); + + sent.Should().Be(csv); + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("success").GetBoolean().Should().BeTrue(); + doc.RootElement.GetProperty("created").GetInt32().Should().Be(1); + doc.RootElement.GetProperty("updated").GetInt32().Should().Be(2); + doc.RootElement.GetProperty("unmatchedEmployees").EnumerateArray() + .Select(e => e.GetString()).Should().Equal("Nobody Here"); + doc.RootElement.GetProperty("warnings").GetArrayLength().Should().Be(1); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public async Task ImportLeaveBalances_WhenFileMissing_ReturnsErrorWithoutCallingApi() + { + var api = Substitute.For(); + var tools = CreateTools(api, CreateConfig()); + + var json = await tools.ImportLeaveBalances( + Path.Combine(Path.GetTempPath(), $"tp-missing-{Guid.NewGuid():N}.csv"), + TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("error").GetString().Should().StartWith("File not found"); + await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ImportLeaveBalances_WhenNotLoggedIn_ReturnsErrorWithoutCallingApi() + { + var api = Substitute.For(); + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns((TenantConfig?)null); + var tools = CreateTools(api, config); + + var json = await tools.ImportLeaveBalances("balances.csv", TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("error").GetString().Should().Contain("Not logged in"); + await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); + } + + private static IConfigService CreateConfig() + { + var config = Substitute.For(); + config.LoadActiveTenantConfig().Returns(new TenantConfig + { + TenantId = "test", + ApiUrl = "https://timepro.example", + ApiKey = "test-api-key", + EmployeeId = "TST" + }); + return config; + } + + private static AccountingMcpTools CreateTools(ITimeProApiClient api, IConfigService config) => + new(api, config, new LeaveBalanceImportService(api)); +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs index 6e2c289..e250cff 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs @@ -266,7 +266,7 @@ public async Task UpdateLeave_WhenChangingNote_PreservesExistingFields() } [Fact] - public async Task ImportLeaveBalances_WithCsvPath_SendsFileContentsAndReportsSkippedRows() + public async Task GetLeaveBalanceStatus_WhenNothingImported_ReportsNotImported() { var api = Substitute.For(); var config = Substitute.For(); @@ -277,94 +277,13 @@ public async Task ImportLeaveBalances_WithCsvPath_SendsFileContentsAndReportsSki ApiKey = "test-api-key", EmployeeId = "TST" }); - - const string csv = "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n"; - string? sent = null; - api.ImportLeaveBalancesAsync(Arg.Do(value => sent = value), Arg.Any()) - .Returns(new ImportLeaveBalancesResult - { - AsAtDate = new DateOnly(2026, 8, 1), - Created = 1, - Updated = 2, - UnmatchedEmployees = ["Nobody Here"], - Warnings = ["Jane Doe has an unusually large balance"] - }); - - var path = Path.Combine(Path.GetTempPath(), $"tp-mcp-balances-{Guid.NewGuid():N}.csv"); - await File.WriteAllTextAsync(path, csv, TestContext.Current.CancellationToken); - - try + api.GetLeaveBalanceStatusAsync(Arg.Any()).Returns(new LeaveBalanceStatus { - var tools = CreateTools(api, config); - var json = await tools.ImportLeaveBalances(path, TestContext.Current.CancellationToken); - - sent.Should().Be(csv); - using var doc = JsonDocument.Parse(json); - doc.RootElement.GetProperty("success").GetBoolean().Should().BeTrue(); - doc.RootElement.GetProperty("created").GetInt32().Should().Be(1); - doc.RootElement.GetProperty("updated").GetInt32().Should().Be(2); - // Skipped rows must reach the agent so it can tell the user who was missed. - doc.RootElement.GetProperty("unmatchedEmployees").EnumerateArray() - .Select(e => e.GetString()).Should().Equal("Nobody Here"); - doc.RootElement.GetProperty("warnings").GetArrayLength().Should().Be(1); - } - finally - { - File.Delete(path); - } - } - - [Fact] - public async Task ImportLeaveBalances_WhenFileMissing_ReturnsErrorWithoutCallingApi() - { - var api = Substitute.For(); - var config = Substitute.For(); - config.LoadActiveTenantConfig().Returns(new TenantConfig - { - TenantId = "test", - ApiUrl = "https://timepro.example", - ApiKey = "test-api-key", - EmployeeId = "TST" - }); - var tools = CreateTools(api, config); - - var json = await tools.ImportLeaveBalances( - Path.Combine(Path.GetTempPath(), $"tp-missing-{Guid.NewGuid():N}.csv"), - TestContext.Current.CancellationToken); - - using var doc = JsonDocument.Parse(json); - doc.RootElement.GetProperty("error").GetString().Should().StartWith("File not found"); - await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); - } - - [Fact] - public async Task ImportLeaveBalances_WhenNotLoggedIn_ReturnsErrorWithoutCallingApi() - { - var api = Substitute.For(); - var config = Substitute.For(); - config.LoadActiveTenantConfig().Returns((TenantConfig?)null); - var tools = CreateTools(api, config); - - var json = await tools.ImportLeaveBalances("balances.csv", TestContext.Current.CancellationToken); - - using var doc = JsonDocument.Parse(json); - doc.RootElement.GetProperty("error").GetString().Should().Contain("Not logged in"); - await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); - } - - [Fact] - public async Task GetLeaveBalanceStatus_WhenNothingImported_ReportsNotImported() - { - var api = Substitute.For(); - var config = Substitute.For(); - config.LoadActiveTenantConfig().Returns(new TenantConfig - { - TenantId = "test", - ApiUrl = "https://timepro.example", - ApiKey = "test-api-key", - EmployeeId = "TST" + AsAtDate = null, + LastImportedAt = null, + EmployeeCount = 0, + IsStale = false }); - api.GetLeaveBalanceStatusAsync(Arg.Any()).Returns((LeaveBalanceStatus?)null); var tools = CreateTools(api, config); var json = await tools.GetLeaveBalanceStatus(TestContext.Current.CancellationToken); @@ -374,7 +293,7 @@ public async Task GetLeaveBalanceStatus_WhenNothingImported_ReportsNotImported() } private static LeaveMcpTools CreateTools(ITimeProApiClient api, IConfigService config) => - new(api, config, new LeaveCreateService(api), new LeaveUpdateService(api), new LeaveBalanceImportService(api)); + new(api, config, new LeaveCreateService(api), new LeaveUpdateService(api)); private static (string id, TimeZoneInfo timeZone) FindTimeZone(params string[] ids) { diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs index 5f3d278..b821a6d 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs @@ -121,7 +121,12 @@ public void Accounting_ProducesAccountingSkillWithoutPrefetchBlock() output.Should().Contain("guides/accounting/tax-mismatch.md"); output.Should().Contain("guides/accounting/invoice-evidence-pack.md"); output.Should().Contain("guides/accounting/client-accounting-position.md"); - output.Should().Contain("MCP exposes primitive read-only tools"); + output.Should().Contain("Import Xero leave balances (destructive)"); + output.Should().Contain("tp leave balances status --json"); + output.Should().Contain("tp leave balances import ./LeaveBalances.csv --yes --json"); + output.Should().Contain("`get_leave_balance_status` is on the default TimePro surface"); + output.Should().Contain("`import_leave_balances` tool is available only after `tp feature accounting enable`"); + output.Should().Contain("explicitly approved `import_leave_balances` write"); output.Should().Contain("With another MCP such as Xero"); output.Should().Contain("tp feature accounting enable"); output.Should().NotContain("## Run these first"); From 077e57a0d7cc083a1ecea0bc51ac6faff72679f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jernej=20Kavka=20=28JK=29=20=5BSSW=20=C2=B7=20Microsoft=20?= =?UTF-8?q?MVP=5D?= Date: Sat, 29 Aug 2026 10:34:21 +1000 Subject: [PATCH 5/7] refactor(paths): centralize home expansion --- .../Leave/LeaveBalanceImportService.cs | 30 ++-------------- .../Features/Mcp/Tools/LookupMcpTools.cs | 4 +-- .../Features/RepoMap/RemoveCommand.cs | 7 ++-- .../Features/RepoMap/SetCommand.cs | 10 +++--- .../Infrastructure/Config/RepoDetector.cs | 9 +++-- .../Infrastructure/Paths/PathExpander.cs | 34 ++++++++++++++++++ .../Leave/LeaveBalanceImportServiceTests.cs | 23 ------------ .../Infrastructure/Paths/PathExpanderTests.cs | 36 +++++++++++++++++++ 8 files changed, 86 insertions(+), 67 deletions(-) create mode 100644 src/SSW.TimePro.Cli/Infrastructure/Paths/PathExpander.cs create mode 100644 tests/SSW.TimePro.Cli.Tests/Infrastructure/Paths/PathExpanderTests.cs diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs index 60ffc9b..68d47ac 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs @@ -1,6 +1,7 @@ using System.Text; using System.Text.Json; using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Infrastructure.Paths; using SSW.TimePro.Cli.Shared.Models; namespace SSW.TimePro.Cli.Features.Leave; @@ -39,7 +40,7 @@ public string ReadCsv(string csvPath) string fullPath; try { - fullPath = Path.GetFullPath(ExpandHomeDirectory(csvPath)); + fullPath = Path.GetFullPath(PathExpander.ExpandHomeDirectory(csvPath)); } catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) { @@ -84,33 +85,6 @@ public string ReadCsv(string csvPath) return content; } - /// - /// Expands the shell-style current-user home shorthand because MCP arguments are passed - /// directly to the process and do not go through shell expansion. - /// - internal static string ExpandHomeDirectory(string path) - { - var trimmedPath = path.Trim(); - if (trimmedPath != "~" - && !trimmedPath.StartsWith("~/", StringComparison.Ordinal) - && !trimmedPath.StartsWith(@"~\", StringComparison.Ordinal)) - { - return trimmedPath; - } - - var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - if (string.IsNullOrWhiteSpace(userProfile)) - return trimmedPath; - - if (trimmedPath.Length == 1) - return userProfile; - - var relativePath = trimmedPath[2..] - .Replace('/', Path.DirectorySeparatorChar) - .Replace('\\', Path.DirectorySeparatorChar); - return Path.Combine(userProfile, relativePath); - } - /// /// Reads the CSV and sends it to TimePro. The server owns parsing and matching, so its /// rejection messages are surfaced verbatim rather than second-guessed here. diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LookupMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LookupMcpTools.cs index a06ef72..0109499 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LookupMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LookupMcpTools.cs @@ -3,6 +3,7 @@ using ModelContextProtocol.Server; using SSW.TimePro.Cli.Infrastructure.ApiClient; using SSW.TimePro.Cli.Infrastructure.Config; +using SSW.TimePro.Cli.Infrastructure.Paths; namespace SSW.TimePro.Cli.Features.Mcp.Tools; @@ -100,8 +101,7 @@ public string GetLocationAndMapping( RepoMappingEntry? match = null; if (repoPath is not null) { - var normalized = repoPath.Replace("~", - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile)); + var normalized = PathExpander.ExpandHomeDirectory(repoPath); match = RepoDetector.Detect(normalized, mappings); } diff --git a/src/SSW.TimePro.Cli/Features/RepoMap/RemoveCommand.cs b/src/SSW.TimePro.Cli/Features/RepoMap/RemoveCommand.cs index 3f4a9cb..e1c2119 100644 --- a/src/SSW.TimePro.Cli/Features/RepoMap/RemoveCommand.cs +++ b/src/SSW.TimePro.Cli/Features/RepoMap/RemoveCommand.cs @@ -1,6 +1,7 @@ using System.ComponentModel; using SSW.TimePro.Cli.Infrastructure.Config; using SSW.TimePro.Cli.Infrastructure.Output; +using SSW.TimePro.Cli.Infrastructure.Paths; using Spectre.Console.Cli; namespace SSW.TimePro.Cli.Features.RepoMap; @@ -22,11 +23,11 @@ public class Settings : CommandSettings protected override int Execute(CommandContext context, Settings settings, CancellationToken cancellationToken) { var mappings = _config.LoadRepoMappings(); - var normalizedPath = settings.Path.Replace("~", Environment.GetFolderPath(Environment.SpecialFolder.UserProfile)); + var normalizedPath = PathExpander.ExpandHomeDirectory(settings.Path); var removed = mappings.RemoveAll(m => - m.PathPattern.Equals(settings.Path, StringComparison.OrdinalIgnoreCase) || - m.PathPattern.Equals(normalizedPath, StringComparison.OrdinalIgnoreCase)); + PathExpander.ExpandHomeDirectory(m.PathPattern) + .Equals(normalizedPath, StringComparison.OrdinalIgnoreCase)); if (removed == 0) { diff --git a/src/SSW.TimePro.Cli/Features/RepoMap/SetCommand.cs b/src/SSW.TimePro.Cli/Features/RepoMap/SetCommand.cs index 4f26551..7836b08 100644 --- a/src/SSW.TimePro.Cli/Features/RepoMap/SetCommand.cs +++ b/src/SSW.TimePro.Cli/Features/RepoMap/SetCommand.cs @@ -1,6 +1,7 @@ using System.ComponentModel; using SSW.TimePro.Cli.Infrastructure.Config; using SSW.TimePro.Cli.Infrastructure.Output; +using SSW.TimePro.Cli.Infrastructure.Paths; using Spectre.Console.Cli; namespace SSW.TimePro.Cli.Features.RepoMap; @@ -52,15 +53,12 @@ protected override int Execute(CommandContext context, Settings settings, Cancel } var mappings = _config.LoadRepoMappings(); - var home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - string Expand(string p) => p.StartsWith("~") - ? home + p.AsSpan(1).ToString() - : p; - var normalizedInput = Expand(settings.Path); + var normalizedInput = PathExpander.ExpandHomeDirectory(settings.Path); // Match by expanded path so "~/foo" and "/Users/me/foo" dedupe. var existing = mappings.FirstOrDefault(m => - Expand(m.PathPattern).Equals(normalizedInput, StringComparison.OrdinalIgnoreCase)); + PathExpander.ExpandHomeDirectory(m.PathPattern) + .Equals(normalizedInput, StringComparison.OrdinalIgnoreCase)); if (existing is not null) { diff --git a/src/SSW.TimePro.Cli/Infrastructure/Config/RepoDetector.cs b/src/SSW.TimePro.Cli/Infrastructure/Config/RepoDetector.cs index 7f540f6..f713821 100644 --- a/src/SSW.TimePro.Cli/Infrastructure/Config/RepoDetector.cs +++ b/src/SSW.TimePro.Cli/Infrastructure/Config/RepoDetector.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using SSW.TimePro.Cli.Infrastructure.Paths; namespace SSW.TimePro.Cli.Infrastructure.Config; @@ -17,8 +18,6 @@ public static class RepoDetector /// public static RepoMappingEntry? Detect(string directory, List mappings) { - var home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - // Collect candidate paths: cwd + main worktree var candidatePaths = new List { directory }; var mainRepoPath = ResolveMainRepoPath(directory); @@ -34,7 +33,7 @@ public static class RepoDetector foreach (var m in mappings) { - var score = ScoreMapping(m, candidatePaths, remoteUrl, home); + var score = ScoreMapping(m, candidatePaths, remoteUrl); if (score > bestScore) { bestScore = score; @@ -46,14 +45,14 @@ public static class RepoDetector } private static int ScoreMapping( - RepoMappingEntry mapping, List paths, string? remoteUrl, string home) + RepoMappingEntry mapping, List paths, string? remoteUrl) { int bestScore = -1; // Check path-based matching if (!string.IsNullOrEmpty(mapping.PathPattern)) { - var pattern = mapping.PathPattern.Replace("~", home); + var pattern = PathExpander.ExpandHomeDirectory(mapping.PathPattern); foreach (var path in paths) { diff --git a/src/SSW.TimePro.Cli/Infrastructure/Paths/PathExpander.cs b/src/SSW.TimePro.Cli/Infrastructure/Paths/PathExpander.cs new file mode 100644 index 0000000..9fef420 --- /dev/null +++ b/src/SSW.TimePro.Cli/Infrastructure/Paths/PathExpander.cs @@ -0,0 +1,34 @@ +namespace SSW.TimePro.Cli.Infrastructure.Paths; + +/// +/// Expands portable path shorthand before paths are resolved or compared. +/// +public static class PathExpander +{ + /// + /// Expands a leading current-user home segment (~, ~/, or ~\). + /// Embedded tildes and named-user forms such as ~someone are left unchanged. + /// + public static string ExpandHomeDirectory(string path) + { + var trimmedPath = path.Trim(); + if (trimmedPath != "~" + && !trimmedPath.StartsWith("~/", StringComparison.Ordinal) + && !trimmedPath.StartsWith(@"~\", StringComparison.Ordinal)) + { + return trimmedPath; + } + + var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); + if (string.IsNullOrWhiteSpace(userProfile)) + return trimmedPath; + + if (trimmedPath.Length == 1) + return userProfile; + + var relativePath = trimmedPath[2..] + .Replace('/', Path.DirectorySeparatorChar) + .Replace('\\', Path.DirectorySeparatorChar); + return Path.Combine(userProfile, relativePath); + } +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs index dfad56f..b428927 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs @@ -56,29 +56,6 @@ public async Task Import_WhenFileMissing_DoesNotCallApi() api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); } - [Theory] - [InlineData("~/Downloads/LeaveBalances.csv")] - [InlineData(@"~\Downloads\LeaveBalances.csv")] - public void ExpandHomeDirectory_WithTildePrefix_UsesCurrentUserProfile(string path) - { - var expected = Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - "Downloads", - "LeaveBalances.csv"); - - var expanded = LeaveBalanceImportService.ExpandHomeDirectory(path); - - expanded.Should().Be(expected); - } - - [Theory] - [InlineData("~northwind/LeaveBalances.csv")] - [InlineData("exports/~/LeaveBalances.csv")] - public void ExpandHomeDirectory_WhenTildeIsNotTheFirstSegment_LeavesPathUnchanged(string path) - { - LeaveBalanceImportService.ExpandHomeDirectory(path).Should().Be(path); - } - [Fact] public async Task Import_WhenRelativePathResolvesElsewhere_ReportsBothPaths() { diff --git a/tests/SSW.TimePro.Cli.Tests/Infrastructure/Paths/PathExpanderTests.cs b/tests/SSW.TimePro.Cli.Tests/Infrastructure/Paths/PathExpanderTests.cs new file mode 100644 index 0000000..80a938f --- /dev/null +++ b/tests/SSW.TimePro.Cli.Tests/Infrastructure/Paths/PathExpanderTests.cs @@ -0,0 +1,36 @@ +using FluentAssertions; +using SSW.TimePro.Cli.Infrastructure.Paths; +using Xunit; + +namespace SSW.TimePro.Cli.Tests.Infrastructure.Paths; + +public class PathExpanderTests +{ + [Theory] + [InlineData("~/Downloads/LeaveBalances.csv")] + [InlineData(@"~\Downloads\LeaveBalances.csv")] + public void ExpandHomeDirectory_WithTildePrefix_UsesCurrentUserProfile(string path) + { + var expected = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + "Downloads", + "LeaveBalances.csv"); + + PathExpander.ExpandHomeDirectory(path).Should().Be(expected); + } + + [Fact] + public void ExpandHomeDirectory_WithTildeOnly_ReturnsCurrentUserProfile() + { + PathExpander.ExpandHomeDirectory("~").Should().Be( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile)); + } + + [Theory] + [InlineData("~northwind/LeaveBalances.csv")] + [InlineData("exports/~/LeaveBalances.csv")] + public void ExpandHomeDirectory_WhenTildeIsNotTheFirstSegment_LeavesPathUnchanged(string path) + { + PathExpander.ExpandHomeDirectory(path).Should().Be(path); + } +} From ccfbc47cb355fece9bcc25c931f790aadbde1017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jernej=20Kavka=20=28JK=29=20=5BSSW=20=C2=B7=20Microsoft=20?= =?UTF-8?q?MVP=5D?= Date: Sat, 29 Aug 2026 10:40:50 +1000 Subject: [PATCH 6/7] docs(skills): expose leave balance status --- docs/skill-generation.md | 1 + .../Features/Skills/Templates/timepro-timesheets.md | 7 +++++++ .../Features/Skills/SkillGenerationTests.cs | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/docs/skill-generation.md b/docs/skill-generation.md index b1e95b7..5cd4fb9 100644 --- a/docs/skill-generation.md +++ b/docs/skill-generation.md @@ -105,6 +105,7 @@ The timesheets skill keeps: - `tp info --json` as the first health/update check, preferred over `tp --version` - `tp project recent --json` as the first project-selection step - the existing timesheet, booking, leave, repo mapping, scrum, and troubleshooting guidance +- read-only EasyLeave balance freshness via CLI and the default MCP status tool - Northwind-only examples (`NWIND`, `1I776Q`, `Northwind/traders-app`) The tenant setup skill keeps: diff --git a/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md b/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md index 917803d..0e48e7b 100644 --- a/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md +++ b/src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md @@ -60,6 +60,7 @@ tp bk list --week --json # Leave tp leave list --filter UPCOMING --json +tp leave balances status --json tp leave create --start 2026-03-30 --end 2026-03-30 --type 1 \ --note "Reason" --approved-by "approver@northwind.example" \ --cc "notify1@northwind.example,notify2@northwind.example" --yes @@ -70,6 +71,12 @@ tp leave update --note "Updated reason" --dry-run --json tp leave cancel --reason "Plans changed" --yes ``` +### Check EasyLeave balance freshness +Use `tp leave balances status --json` for read-only questions about when company-wide +EasyLeave balances were last imported and whether they are stale. The equivalent default +MCP tool is `get_leave_balance_status`. Do not import balances from this skill; the +destructive `import_leave_balances` workflow is accounting-gated. + ## Workflow: Enter Timesheets for the Week 1. Pick the project: `tp project recent --json`. 2. Verify repo mapping: `tp map detect`. If category is missing, run the repo mapping setup workflow below. diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs index b821a6d..df4bb3c 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Skills/SkillGenerationTests.cs @@ -64,6 +64,10 @@ public void Frontmatter_ContainsNameDescriptionAndAllowedTools() output.Should().Contain("name: timepro-timesheets"); output.Should().Contain("description:"); output.Should().Contain("allowed-tools: Bash(tp *), Bash(sl *)"); + output.Should().Contain("tp leave balances status --json"); + output.Should().Contain("The equivalent default\nMCP tool is `get_leave_balance_status`"); + output.Should().Contain("`import_leave_balances` workflow is accounting-gated"); + output.Should().NotContain("tp leave balances import"); } [Fact] From a1e9e9bc18b019956c9c6f1f891e49e112a5e038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jernej=20Kavka=20=28JK=29=20=5BSSW=20=C2=B7=20Microsoft=20?= =?UTF-8?q?MVP=5D?= Date: Sat, 29 Aug 2026 11:23:27 +1000 Subject: [PATCH 7/7] fix(leave): harden balance import recovery --- .../Features/Leave/BalancesImportCommand.cs | 14 +++- .../Leave/LeaveBalanceImportService.cs | 45 +++++++++-- .../Features/Mcp/Tools/AccountingMcpTools.cs | 10 ++- .../Features/Mcp/Tools/LeaveMcpTools.cs | 2 +- .../Features/Leave/BalancesCommandsTests.cs | 33 ++++++++ .../Leave/LeaveBalanceImportServiceTests.cs | 77 ++++++++++++++++++- .../Features/Mcp/AccountingMcpToolsTests.cs | 41 ++++++++++ .../Features/Mcp/LeaveMcpToolsTests.cs | 14 ++++ 8 files changed, 222 insertions(+), 14 deletions(-) diff --git a/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs b/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs index 0efb347..eca8c64 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/BalancesImportCommand.cs @@ -75,14 +75,15 @@ protected override async Task ExecuteAsync(CommandContext context, Settings table.AddRow("[bold]Updated[/]", r.Updated.ToString()); AnsiConsole.Write(table); - foreach (var warning in r.Warnings) + foreach (var warning in r.Warnings ?? []) OutputHelper.WriteWarning(warning); - if (r.UnmatchedEmployees.Count > 0) + var unmatchedEmployees = r.UnmatchedEmployees ?? []; + if (unmatchedEmployees.Count > 0) { OutputHelper.WriteWarning( - $"{r.UnmatchedEmployees.Count} row(s) were skipped because the name did not match exactly one TimePro employee:"); - foreach (var name in r.UnmatchedEmployees) + $"{unmatchedEmployees.Count} row(s) were skipped because the name did not match exactly one TimePro employee:"); + foreach (var name in unmatchedEmployees) AnsiConsole.MarkupLine($" - {Markup.Escape(name)}"); } @@ -96,6 +97,11 @@ protected override async Task ExecuteAsync(CommandContext context, Settings WriteError(settings.Json, ex.Message); return 1; } + catch (LeaveBalanceImportUncertainException ex) + { + WriteError(settings.Json, ex.Message); + return 1; + } catch (ApiException ex) { var detail = ApiErrorParser.ExtractDetail(ex.ResponseBody); diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs index 68d47ac..358aeae 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveBalanceImportService.cs @@ -8,6 +8,9 @@ namespace SSW.TimePro.Cli.Features.Leave; public sealed class LeaveBalanceImportValidationException(string message) : Exception(message); +public sealed class LeaveBalanceImportUncertainException(string message, Exception? innerException = null) + : Exception(message, innerException); + /// /// Reads a Xero "Leave Balances" CSV export from disk and imports it into TimePro. /// @@ -17,6 +20,14 @@ public sealed class LeaveBalanceImportValidationException(string message) : Exce /// public sealed class LeaveBalanceImportService { + private const string UncertainImportMessage = + "The import request was sent and may have been applied, but TimePro did not return a complete result. " + + "Run 'tp leave balances status' before retrying."; + + private static readonly Encoding StrictUtf8 = new UTF8Encoding( + encoderShouldEmitUTF8Identifier: false, + throwOnInvalidBytes: true); + /// /// Guard against a caller pointing at the wrong file entirely (a database dump, a video). /// A real Xero balance export for a company of any plausible size is far below this. @@ -66,7 +77,12 @@ public string ReadCsv(string csvPath) string content; try { - content = File.ReadAllText(fullPath, Encoding.UTF8); + content = File.ReadAllText(fullPath, StrictUtf8); + } + catch (DecoderFallbackException) + { + throw new LeaveBalanceImportValidationException( + $"'{fullPath}' is not valid UTF-8. Export the Xero leave balances report as a UTF-8 CSV and try again."); } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { @@ -95,11 +111,22 @@ public async Task ImportAsync(string csvPath, Cancell try { - return await _api.ImportLeaveBalancesAsync(csv, ct) - ?? throw new LeaveBalanceImportValidationException( - "TimePro accepted the import but returned no result. Run 'tp leave balances status' to confirm what was stored."); + var result = await _api.ImportLeaveBalancesAsync(csv, ct); + if (result is null || result.AsAtDate == default) + throw new LeaveBalanceImportUncertainException(UncertainImportMessage); + + // System.Text.Json can replace collection initializers with null when the API sends + // explicit nulls. Normalize here so every CLI/MCP caller receives the same safe shape. + result.Warnings ??= []; + result.UnmatchedEmployees ??= []; + return result; } - catch (ApiException ex) when (ex.StatusCode is 401 or 403) + catch (ApiException ex) when (ex.StatusCode == 401) + { + throw new LeaveBalanceImportValidationException( + "TimePro authentication failed. Run 'tp login --tenant ' again."); + } + catch (ApiException ex) when (ex.StatusCode == 403) { throw new LeaveBalanceImportValidationException( "Importing leave balances requires leave admin rights in TimePro, and this account does not have them."); @@ -110,6 +137,14 @@ public async Task ImportAsync(string csvPath, Cancell throw new LeaveBalanceImportValidationException( $"TimePro could not read the CSV: {DescribeUnprocessable(ex.ResponseBody)}"); } + catch (LeaveBalanceImportUncertainException) + { + throw; + } + catch (Exception ex) + { + throw new LeaveBalanceImportUncertainException(UncertainImportMessage, ex); + } } /// diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs index de132f9..c2bfe68 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/AccountingMcpTools.cs @@ -68,7 +68,7 @@ private static List ResolveEmpIds(string[]? empIds, string[]? employeeId // ─── Leave balance import ─────────────────────────────────────────────── - [McpServerTool] + [McpServerTool(Destructive = true, Idempotent = true, ReadOnly = false)] [Description( "Import leave balances for EVERY employee from a Xero 'Leave Balances' CSV export, replacing what TimePro currently stores. " + "No dry run and no undo, so confirm with the user first. Pass the path to the CSV file, not its contents. " @@ -96,6 +96,14 @@ public async Task ImportLeaveBalances( { return JsonSerializer.Serialize(new { error = ex.Message }, JsonOpts); } + catch (LeaveBalanceImportUncertainException ex) + { + return JsonSerializer.Serialize(new + { + error = ex.Message, + mayHaveBeenApplied = true + }, JsonOpts); + } } // ─── Invoices ─────────────────────────────────────────────────────────── diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs index ab9700a..df5e199 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs @@ -199,7 +199,7 @@ public async Task UpdateLeave( } } - [McpServerTool] + [McpServerTool(ReadOnly = true, Destructive = false)] [Description("Report when TimePro's leave balances were last imported from Xero, how many employees have a stored balance, and whether the data is stale. Read-only. Check this before importing so you can tell the user whether a re-import is actually needed.")] public async Task GetLeaveBalanceStatus(CancellationToken ct = default) { diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs index e0bcafe..fc97572 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/BalancesCommandsTests.cs @@ -36,6 +36,39 @@ public async Task Import_WithValidCsvAndJson_CallsApiWithoutPrompting() sent.Should().Be(ValidCsv); } + [Fact] + public async Task Import_WhenApiReturnsNullCollections_StillSucceeds() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns(new ImportLeaveBalancesResult + { + AsAtDate = new DateOnly(2026, 8, 1), + UnmatchedEmployees = null!, + Warnings = null! + }); + var app = CreateApp(api); + var path = WriteFile("balances.csv", ValidCsv); + + var exitCode = await app.RunAsync(["import", path, "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(0); + } + + [Fact] + public async Task Import_WhenSubmissionOutcomeIsUncertain_ReturnsFailureExitCode() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new HttpRequestException("Connection dropped")); + var app = CreateApp(api); + var path = WriteFile("balances.csv", ValidCsv); + + var exitCode = await app.RunAsync(["import", path, "--json"], TestContext.Current.CancellationToken); + + exitCode.Should().Be(1); + } + [Fact] public async Task Import_WhenFileMissing_FailsBeforeCallingApi() { diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs index b428927..6af8d3b 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveBalanceImportServiceTests.cs @@ -43,6 +43,26 @@ public async Task Import_WithValidCsv_SendsFileContentsVerbatim() result.Warnings.Should().ContainSingle(); } + [Fact] + public async Task Import_WhenApiReturnsNullCollections_NormalizesThemToEmpty() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns(new ImportLeaveBalancesResult + { + AsAtDate = new DateOnly(2026, 8, 1), + UnmatchedEmployees = null!, + Warnings = null! + }); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var result = await service.ImportAsync(path, TestContext.Current.CancellationToken); + + result.UnmatchedEmployees.Should().BeEmpty(); + result.Warnings.Should().BeEmpty(); + } + [Fact] public async Task Import_WhenFileMissing_DoesNotCallApi() { @@ -127,6 +147,41 @@ public async Task Import_WhenFileIsBinary_DoesNotCallApi() api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); } + [Fact] + public async Task Import_WhenFileIsNotUtf8_ExplainsHowToExportIt() + { + var api = Substitute.For(); + var service = new LeaveBalanceImportService(api); + var path = Path.Combine(_tempDir, "windows-1252.csv"); + File.WriteAllBytes(path, + [ + .. Encoding.ASCII.GetBytes("Employee,Leave Type,Units\nJos"), + 0xE9, + .. Encoding.ASCII.GetBytes(",Annual Leave,76.00\n") + ]); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*not valid UTF-8*Export*UTF-8 CSV*"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.ImportLeaveBalancesAsync)); + } + + [Fact] + public async Task Import_WhenApiReturnsUnauthorized_DirectsUserToLogin() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException(401, "Unauthorized", null)); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*authentication failed*tp login*"); + } + [Fact] public async Task Import_WhenApiReturnsForbidden_ExplainsAdminRequirement() { @@ -159,7 +214,7 @@ public async Task Import_WhenApiRejectsCsv_SurfacesServerMessage() } [Fact] - public async Task Import_WhenApiFailsForAnotherReason_LeavesApiExceptionUnwrapped() + public async Task Import_WhenApiFailsAfterSubmission_WarnsBeforeRetrying() { var api = Substitute.For(); api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) @@ -169,8 +224,24 @@ public async Task Import_WhenApiFailsForAnotherReason_LeavesApiExceptionUnwrappe var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); - // 5xx should reach the caller as-is so the response body stays diagnosable. - await act.Should().ThrowAsync(); + var exception = await act.Should().ThrowAsync(); + exception.WithMessage("*may have been applied*tp leave balances status*before retrying*"); + exception.Which.InnerException.Should().BeOfType(); + } + + [Fact] + public async Task Import_WhenSuccessResponseIsIncomplete_WarnsBeforeRetrying() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns(new ImportLeaveBalancesResult()); + var service = new LeaveBalanceImportService(api); + var path = WriteFile("balances.csv", ValidCsv); + + var act = () => service.ImportAsync(path, TestContext.Current.CancellationToken); + + (await act.Should().ThrowAsync()) + .WithMessage("*may have been applied*tp leave balances status*before retrying*"); } [Theory] diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs index 72ffe63..ebec19e 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/AccountingMcpToolsTests.cs @@ -1,5 +1,6 @@ using System.Text.Json; using FluentAssertions; +using ModelContextProtocol.Server; using NSubstitute; using SSW.TimePro.Cli.Features.Leave; using SSW.TimePro.Cli.Features.Mcp.Tools; @@ -20,6 +21,20 @@ public void BalanceTools_AreSeparatedBetweenDefaultAndAccountingSurfaces() typeof(AccountingMcpTools).GetMethod(nameof(AccountingMcpTools.ImportLeaveBalances)).Should().NotBeNull(); } + [Fact] + public void ImportLeaveBalances_IsMarkedDestructiveAndIdempotent() + { + var attribute = typeof(AccountingMcpTools) + .GetMethod(nameof(AccountingMcpTools.ImportLeaveBalances))! + .GetCustomAttributes(typeof(McpServerToolAttribute), inherit: false) + .Cast() + .Single(); + + attribute.Destructive.Should().BeTrue(); + attribute.Idempotent.Should().BeTrue(); + attribute.ReadOnly.Should().BeFalse(); + } + [Fact] public async Task ImportLeaveBalances_WithCsvPath_SendsFileContentsAndReportsSkippedRows() { @@ -90,6 +105,32 @@ public async Task ImportLeaveBalances_WhenNotLoggedIn_ReturnsErrorWithoutCalling await api.DidNotReceive().ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()); } + [Fact] + public async Task ImportLeaveBalances_WhenSubmissionOutcomeIsUncertain_ReturnsSafeRetryGuidance() + { + var api = Substitute.For(); + api.ImportLeaveBalancesAsync(Arg.Any(), Arg.Any()) + .Returns>(_ => throw new HttpRequestException("Connection dropped")); + var tools = CreateTools(api, CreateConfig()); + var path = Path.Combine(Path.GetTempPath(), $"tp-mcp-balances-{Guid.NewGuid():N}.csv"); + await File.WriteAllTextAsync(path, "Employee,Leave Type,Units\nJane Doe,Annual Leave,76.00\n", + TestContext.Current.CancellationToken); + + try + { + var json = await tools.ImportLeaveBalances(path, TestContext.Current.CancellationToken); + + using var doc = JsonDocument.Parse(json); + doc.RootElement.GetProperty("mayHaveBeenApplied").GetBoolean().Should().BeTrue(); + doc.RootElement.GetProperty("error").GetString() + .Should().Contain("tp leave balances status").And.Contain("before retrying"); + } + finally + { + File.Delete(path); + } + } + private static IConfigService CreateConfig() { var config = Substitute.For(); diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs index e250cff..ac2c262 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs @@ -1,5 +1,6 @@ using System.Text.Json; using FluentAssertions; +using ModelContextProtocol.Server; using NSubstitute; using SSW.TimePro.Cli.Features.Leave; using SSW.TimePro.Cli.Features.Mcp.Tools; @@ -12,6 +13,19 @@ namespace SSW.TimePro.Cli.Tests.Features.Mcp; public class LeaveMcpToolsTests { + [Fact] + public void GetLeaveBalanceStatus_IsMarkedReadOnly() + { + var attribute = typeof(LeaveMcpTools) + .GetMethod(nameof(LeaveMcpTools.GetLeaveBalanceStatus))! + .GetCustomAttributes(typeof(McpServerToolAttribute), inherit: false) + .Cast() + .Single(); + + attribute.ReadOnly.Should().BeTrue(); + attribute.Destructive.Should().BeFalse(); + } + [Fact] public async Task CreateLeave_WhenProfileTimezoneAvailable_SendsDateOffsetsFromProfileTimezone() {