fix(timesheets): ts create takes the iteration by name or ID and validates it locally - #88
Conversation
…dates it locally `ts update` and `ts accept` resolve `--iteration` by name or ID, but `ts create` and the MCP `create_timesheet` tool still took only an integer ID, and a missing or wrong one reached the API, which answers with a bare "Please select an iteration". `TimesheetCreateService` now resolves the iteration through the same project lookup and fails before the write with the available iterations listed: unknown name or ID, no iteration on a project that uses them, or an iteration on a project that does not. The MCP tool gains an optional `iteration` string argument; `iterationId` stays for existing callers. The tools/list goldens change additively. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A missing rate sends ts create into its rate-creation prompt, so an unknown iteration must be refused before a rate can be written for a create that is going to fail anyway. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Astra review (independent lane) raised one finding, fixed in the follow-up commit: iteration validation ran after the no-rate early return, so the interactive rate prompt could create a rate for a create that was then refused. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved validation-ordering, MCP precedence, and documentation findings remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds name-or-ID iteration resolution and local validation for timesheet creation across the CLI and MCP, while preserving iterationId compatibility.
Changes:
- Resolves and validates iterations before saving.
- Updates CLI/MCP inputs, schemas, goldens, and tests.
- Updates skill guidance, versioning, and agent documentation.
File summaries
| File | Summary |
|---|---|
tests/SSW.TimePro.Cli.Tests/Features/Timesheets/TimesheetCreateServiceTests.cs |
Tests iteration resolution and validation scenarios. |
tests/SSW.TimePro.Cli.Integration/Mcp/CliMcpCreatePayloadTests.cs |
Verifies CLI/MCP payload and error parity. |
tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Discovery/tools-list.default.json |
Updates the default MCP schema golden. |
tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Discovery/tools-list.accounting.json |
Updates the accounting MCP schema golden. |
src/SSW.TimePro.Cli/Features/Timesheets/TimesheetCreateService.cs |
Resolves and validates iterations; validation ordering requires correction. |
src/SSW.TimePro.Cli/Features/Timesheets/CreateCommand.cs |
Accepts iteration names or IDs for CLI creation. |
src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md |
Updates iteration guidance; validation wording needs clarification. |
src/SSW.TimePro.Cli/Features/Skills/SkillModelBuilder.cs |
Bumps the skill version. |
src/SSW.TimePro.Cli/Features/Mcp/Tools/TimesheetMcpTools.cs |
Adds MCP iteration support; discovery wording and precedence coverage need updates. |
AGENTS.md |
Documents write-surface behavior; guidance scope needs narrowing. |
Review details
Suppressed comments (3)
AGENTS.md:119
- This newly added “every write surface” claim is not true:
ts copystill has its own iteration resolver and request construction, and an unmatched source iteration is reduced to a null ID before the write (src/SSW.TimePro.Cli/Features/Timesheets/CopyCommand.cs:113-142). Since this PR explicitly leaves copy out of scope, narrow the guidance to the create/update/accept services or document the exception so agents do not assume copy has the new fail-fast validation.
elsewhere either. The iteration is taken by name or ID on every write surface and checked locally
against the project's iterations: the API answers a missing or unknown one with a bare "Please
select an iteration", so the service fails first and lists the available ones.
src/SSW.TimePro.Cli/Features/Skills/Templates/timepro-timesheets.md:174
ResolveIterationAsyncmakes aGetIterationsAsyncrequest to enumerate valid options, so this says the command fails before an API call even though it still performs that read. Clarify that validation happens before theSaveTimesheetwrite, otherwise agents may incorrectly assume this path is network-free.
`ts accept` all take `--iteration <name-or-id>`; a missing or unknown iteration fails before the
src/SSW.TimePro.Cli/Features/Timesheets/TimesheetCreateService.cs:78
- Because iteration validation is after the sell-price lookup, an interactive
ts createwith an invalid iteration and no active client rate entersResolveMissingRateAsync; that path can callSaveClientRateAsyncbefore the secondPrepareAsyncfinally rejects the iteration. Validate the iteration before missing-rate recovery/writes so invalid input cannot create a client rate as a side effect.
var iterationId = await ResolveIterationAsync(options.ProjectId, options.Iteration, ct);
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Location: location, | ||
| Category: categoryId, | ||
| IterationId: iterationId, | ||
| Iteration: iteration ?? iterationId?.ToString(), |
| [Description("Iteration/sprint ID. Prefer 'iteration', which also takes the name.")] int? iterationId = null, | ||
| [Description("Iteration/sprint, by name or ID. Required for projects that use iterations (e.g., 1I776Q).")] string? iteration = null, |
Summary
ts updateandts accepthave taken--iteration <name-or-id>since 0.3.1, butts createand the MCPcreate_timesheettool still accepted only an integer ID. A missing or wrong iteration went to the API, which answersIterationId: Please select an iterationwithout saying what the options are.TimesheetCreateService.PrepareAsyncnow resolves the iteration through the same project lookup the update path uses and fails before the write in three cases, each listing the available iterations where that helps:Unknown iteration 'Sprint 99' for project '1I776Q'. Available iterations: Checkout API (3402), Order history (3403).Project '1I776Q' requires an iteration. Available iterations: ...Project 'NW0002' does not use iterations.Both surfaces are adapters over the service, so CLI and MCP produce the same message and neither calls
SaveTimesheeton failure.What changed
TimesheetCreateOptions.IterationId(int) becomesIteration(string);ts create --iteration <NAME_OR_ID>.create_timesheetgains an optionaliterationstring argument.iterationIdis kept for existing callers and is used wheniterationis not given. Thetools/listgoldens change additively (one new optional property, two description strings).CurrentSkillVersion5 → 6 so installed skills report as outdated.Cost: one
GetIterationsForAddTimesheetGET per create. The update path already pays it.Not in scope:
ts copystill carries the source row's iteration over by name and has no override, and it still builds its ownTimesheetRequest. That is a separate slice of #38.How verified
By execution:
dotnet test tests/SSW.TimePro.Cli.Tests/— 596 passed. New: 7TimesheetCreateServiceTestscases (name, case-insensitive name, ID, unknown, missing on an iteration project, none on a plain project, requested on a plain project).dotnet test tests/SSW.TimePro.Cli.Integration/— 301 passed. New inCliMcpCreatePayloadTests: CLI and MCP send a byte-identical payload withiterationID: 3403when given the name, and both return the same error without aSaveTimesheetcall when given an unknown one.ssw-stagingtenant, clientNWIND, Release build from this branch). Project codes and iteration names below are Northwind placeholders; everything else is verbatim.The MCP path was driven over stdio (
initialize→notifications/initialized→tools/call create_timesheet) against the same tenant:iteration: "Checkout API"returned the saved entry with that iteration;"Sprint 99"and a missing iteration returned the same two messages as the CLI with no write. All verification rows were deleted afterwards (tp ts get 2026-09-13 --json→[]).By inspection only: the
iterationIdfallback when an MCP caller passes both arguments (iterationwins).🤖 Generated with Claude Code