Skip to content

fix(timesheets): ts create takes the iteration by name or ID and validates it locally - #88

Merged
jernejk merged 2 commits into
mainfrom
fix/create-iteration-name
Sep 13, 2026
Merged

fix(timesheets): ts create takes the iteration by name or ID and validates it locally#88
jernejk merged 2 commits into
mainfrom
fix/create-iteration-name

Conversation

@jernejk

@jernejk jernejk commented Sep 13, 2026

Copy link
Copy Markdown
Member

Summary

ts update and ts accept have taken --iteration <name-or-id> since 0.3.1, but ts create and the MCP create_timesheet tool still accepted only an integer ID. A missing or wrong iteration went to the API, which answers IterationId: Please select an iteration without saying what the options are.

TimesheetCreateService.PrepareAsync now 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 name or ID: Unknown iteration 'Sprint 99' for project '1I776Q'. Available iterations: Checkout API (3402), Order history (3403).
  • no iteration on a project that uses them: Project '1I776Q' requires an iteration. Available iterations: ...
  • an iteration on a project that does not use them: Project 'NW0002' does not use iterations.

Both surfaces are adapters over the service, so CLI and MCP produce the same message and neither calls SaveTimesheet on failure.

What changed

  • TimesheetCreateOptions.IterationId (int) becomes Iteration (string); ts create --iteration <NAME_OR_ID>.
  • MCP create_timesheet gains an optional iteration string argument. iterationId is kept for existing callers and is used when iteration is not given. The tools/list goldens change additively (one new optional property, two description strings).
  • Skill template: the create example and the Iterations section now say name-or-ID; CurrentSkillVersion 5 → 6 so installed skills report as outdated.
  • AGENTS.md: one paragraph under timesheet writes.

Cost: one GetIterationsForAddTimesheet GET per create. The update path already pays it.

Not in scope: ts copy still carries the source row's iteration over by name and has no override, and it still builds its own TimesheetRequest. That is a separate slice of #38.

How verified

By execution:

  • dotnet test tests/SSW.TimePro.Cli.Tests/ — 596 passed. New: 7 TimesheetCreateServiceTests cases (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 in CliMcpCreatePayloadTests: CLI and MCP send a byte-identical payload with iterationID: 3403 when given the name, and both return the same error without a SaveTimesheet call when given an unknown one.
  • Forced failure: removing the missing-iteration check fails 1 unit test.
  • Staging (ssw-staging tenant, client NWIND, Release build from this branch). Project codes and iteration names below are Northwind placeholders; everything else is verbatim.
$ tp ts create --client NWIND --project 1I776Q --category WEBDEV --date 2026-09-13 \
    --start 22:00 --end 22:30 --iteration "checkout api" --description "CLI verification, safe to delete" --json --yes
{ "success": true, "timesheetId": <id>, "timesheet": { "projectId": "1I776Q", "iteration": "Checkout API", "totalTime": 0.5, ... } }

$ tp ts create ... --iteration 3403 ...
  "iteration": "Order history"

$ tp ts create ... --iteration "Sprint 99" ... ; echo $?
{"error":{"code":null,"message":"Unknown iteration 'Sprint 99' for project '1I776Q'. Available iterations: Checkout API (3402), Order history (3403), Product search (3404).","detail":null}}
1

$ tp ts create ... (no --iteration) ... ; echo $?
{"error":{"code":null,"message":"Project '1I776Q' requires an iteration. Available iterations: Checkout API (3402), Order history (3403), Product search (3404).","detail":null}}
1

$ tp ts create --client NWIND --project NW0002 ... --iteration "Sprint 1" ... ; echo $?
{"error":{"code":null,"message":"Project 'NW0002' does not use iterations.","detail":null}}
1

The MCP path was driven over stdio (initializenotifications/initializedtools/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 iterationId fallback when an MCP caller passes both arguments (iteration wins).

🤖 Generated with Claude Code

…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>
Copilot AI lite review requested due to automatic review settings September 13, 2026 00:06
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>
@jernejk

jernejk commented Sep 13, 2026

Copy link
Copy Markdown
Member Author

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. PrepareAsync now resolves the iteration first; Prepare_ChecksTheIterationBeforeReportingAMissingRate fails without the reorder. Everything else in the review was clean (callers, empty iteration lists, envelope parity, test coverage).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 copy still 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

  • ResolveIterationAsync makes a GetIterationsAsync request 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 the SaveTimesheet write, 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 create with an invalid iteration and no active client rate enters ResolveMissingRateAsync; that path can call SaveClientRateAsync before the second PrepareAsync finally 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(),
Comment on lines +92 to +93
[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,
@jernejk
jernejk merged commit eb8394e into main Sep 13, 2026
1 check passed
@jernejk
jernejk deleted the fix/create-iteration-name branch September 13, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants