Timesheets: model the entry destroy; retire five phantom coverage gaps (#581, #582)

[jeremy]

🛑 HELD — draft on purpose, do not merge

Spec-train order is enforced: #628 → ScheduleEntries triad (#546/#547) → #626 → #629 → recurrence absorption.

#626, #628 and #629 all touch spec/basecamp.smithy and the six generated trees, and all branched from the same base. Merging any two in parallel conflicts across every generated tree. Each needs a rebase and a full make generate and counts re-derived from behavior-model.json (never hand-incremented) after the one ahead of it lands.

This PR returns to ready when its turn arrives and it has been rebased.

---

Lane B2a of the v0.13.0 coverage work: #581 (gauge needle show/update/destroy) and #582 (recording timesheet read + entry create/delete). Six routes were carried as bc3_routes_not_modeled dispositions. One was a real gap. Five were not.

The one real gap: DELETE /timesheet_entries/:id

Now DestroyTimesheetEntry — 204, @idempotent, natural: true, retryOn: [429, 503], maxAttempts: 3 to match its timesheet siblings. Surfaces as timesheets.destroy(entryId) in all six SDKs.

Verified at the pinned provenance revision 2c0dafba13:

	evidence
drawn	config/routes.rb:187 — resources :timesheet_entries, only: %i[ show update destroy ], controller: "timesheets/entries", flat, no bucket scope
renderable	Timesheets::EntriesController#destroy answers format.json { head :no_content }; the legitimate template-less escape hatch, not the GetRecording trap
403 path	the same action answers head :forbidden when Current.person.can_archive_or_trash? is false — hence ForbiddenError in the error set, and an empty-body 403 test in every SDK that has one
documented	doc/api/sections/timesheets.md:688, "will return 204 No Content"
API-tested	test/api/timesheets/entries_controller_api_test.rb — "destroy entry via flat route" asserts :no_content + reload.deleted?, plus an assert_recognizes on the unscoped route

TrashRecording did not already cover this: it trashes recoverably, #destroy deletes outright. Go keeps Trash and gains Destroy, each documented against the other.

The five that were not gaps

bc3 draws each of these actions twice, and the SDK already models the flat spelling of every one:

bc3 route (ledger)	already modeled as
GET /projects/:id/gauge/needles/:id	GetGaugeNeedle → GET /gauge_needles/{needleId}
PUT /projects/:id/gauge/needles/:id	UpdateGaugeNeedle
DELETE /projects/:id/gauge/needles/:id	DestroyGaugeNeedle
GET /projects/:id/recordings/:id/timesheet	GetRecordingTimesheet
POST /projects/:id/recordings/:id/timesheet/entries	CreateTimesheetEntry

Both draws in each pair name the same controller action (routes.rb:186+:737 for needles; :266/:745 and :268/:747 for timesheets). Both controllers branch on @bucket being nil and fall back to authorize_recording, so the flat form is the written path, not an accident. bc3 documents both spellings, its API tests exercise both — "unscoped and scoped routes return identical data for needle" asserts it outright — and the flat needle GET/PUT plus both flat timesheet routes carry live example-markers.

They surfaced in the direction-2 ledger only because bucket_insensitive_key collapses a leading /buckets/:id and nothing else. A tracking_issue on them was a standing promise to ship five duplicate operations across six SDKs for no reachable capability, so they get a truthful disposition instead.

modeled_as, and why it is checked

"We already model that at the other spelling" is exactly the claim that produced ListForwards and RepositionTodolistGroup, so the new disposition is verified, not asserted. The gate requires the named operationId to exist in openapi.json, share the verb, sit at a different path (an identical one means the entry is stale), have bc3's own route table document that path, and have that path be this route with a leading scope removed.

That last rule is a suffix test over _-flattened segments, not a subset test. Subset was the first version and it let GetProjectTimesheet (/projects/:id/timesheet) discharge the recording timesheet route by dropping recordings/:id from the middle — a different endpoint, not a spelling. Flattening is what lets it survive bc3's renames: /gauge_needles/:id → [gauge, needles, :id] is a suffix of [projects, :id, gauge, needles, :id].

Six adversarial substitutions were run against the finished check; all six are rejected with distinct messages:

substitution	rejected because
NoSuchOperation	names no operation
ListGaugeNeedles for needle show	collection cannot stand in for member
GetTimesheetEntry for needle show	different resource
GetProjectTimesheet for recording timesheet	interior segment dropped
ListGauges for needle show	different noun
CreateGaugeNeedle for entry create	different terminal

Verification

make check green at this head — REAL exit code captured to a log and grepped back, REAL_EXIT=0, with PRE_SHA == POST_SHA == e75d69de1 and matching tree hashes, ending on a clean working tree.

All six runners dispatch the new conformance fixture and pass it: Go, Kotlin, Ruby, Python and Swift print PASS: Timesheet entry destroy…; TypeScript's default reporter hides case names, so it was re-run with --reporter=verbose to confirm ✓ … Timesheet entry destroy uses /timesheet_entries/{entryId} path.

Nothing here is asserted without a red proof:

• The conformance fixture is not vacuous — mutating its expected path to a bucket-scoped spelling makes conformance-go exit 2 with Expected request path "/999/buckets/456/timesheet_entries/999" … got "/999/timesheet_entries/999".
• The Go unit tests discriminate Destroy from Trash — routing Destroy through TrashRecordingWithResponse fails them with expected method DELETE, got PUT and expected path /12345/timesheet_entries/… got /12345/recordings/…/status/trashed.json.

Execution, not cache: Swift produced real Test Suite output; Ruby went 1241 → 1243 runs, 29713 assertions, 0 failures, 0 errors, 0 skips; TS SDK 1285 passed across 80 files; Python 1050 passed. Kotlin's jvmTest reported UP-TO-DATE in the final pass, so it was re-run with --rerun-tasks — :basecamp-sdk:jvmTest and :generator:test both executed, BUILD SUCCESSFUL, exit 0.

make bc3-route-parity passes and now reports 15 dispositions (6 tracking_issue, 2 registry, 2 out_of_scope, 5 modeled_as). Both #581 and #582 tracking_issue entries are gone.

Counts were re-derived from behavior-model.json, not hand-edited: total 240→241, idempotent 78→79, union 201→202, DELETEs 23→24, and SPEC.md's retry-ceiling paragraph 198→199 / 190→191 / 201→202. SPEC.md, SECURITY.md, AGENTS.md and scripts/check-idempotency-parity updated to match.

Two smaller things worth calling out in review:

• The Smithy doc is deliberately one complete sentence on one line. Generators truncate method docs to the first line, so a two-line comment shipped "Permanently delete a timesheet entry. Returns 403 when the caller may not" into the TS/Ruby/Kotlin service docs.
• DestroyTimesheetEntry needed a RESOURCE_TYPE_OVERRIDES entry in the TS, Kotlin and Swift generators. Destroy is not a verb pattern, so resource-type inference falls through to a generic "resource" and would have filed this delete away from the get/create/update siblings that report timesheet_entry. Ruby and Python emit no resourceType, so they need nothing.

Notes

• Additive only. No existing operation, path or signature changes.
• Not train-blocked: Todolist polymorphic read repair: one truthful flat shape replaces Todolist/TodolistGroup/TodolistOrGroup #544 is an open issue with no PR, and no open PR touches spec/ or openapi.json. Both halves of this are now false and it is superseded by the hold banner above. Todolist polymorphic read repair: one truthful flat shape replaces Todolist/TodolistGroup/TodolistOrGroup #544 shipped as One flat Todolist replaces Todolist, TodolistGroup and TodolistOrGroup #628, and Model the bare field-map error bodies, then cloud_files and google_documents (#550, #551) #629 also touches spec/. All three branched from the same base and must land in order (One flat Todolist replaces Todolist, TodolistGroup and TodolistOrGroup #628 → triad → Timesheets: model the entry destroy; retire five phantom coverage gaps (#581, #582) #626 → Model the bare field-map error bodies, then cloud_files and google_documents (#550, #551) #629), each with a rebase, a full make generate, and counts re-derived from behavior-model.json.
• Reporting, not closing: Spec: model the gauge needle show/update/destroy operations #581 is not a defect and needs a maintainer decision on whether to close it as already-covered — deliberately no closing keyword for it. Spec: model the recording timesheet read + timesheet entry create/delete operations #582 is genuinely closed by the new operation.

Separate finding — pre-existing, deliberately not fixed here

The same RESOURCE_TYPE_OVERRIDES gap already ships in the gauge service, inside #581's own resource family:

GetGaugeNeedle      → resourceType: "gauge_needle"
UpdateGaugeNeedle   → resourceType: "gauge_needle"
DestroyGaugeNeedle  → resourceType: "resource"   ← inconsistent
ToggleGauge         → resourceType: "resource"   ← inconsistent
ListGaugeNeedles    → resourceType: "gauge_needle"
CreateGaugeNeedle   → resourceType: "gauge_needle"

Destroy and Toggle are both absent from the verb-pattern list. The fix is two more override lines in each of the three generators. I left it out because it changes an observability string on already-shipped operations — that is a bug, and folding it into an enhancement PR would file it wrongly in the release notes. Happy to open it as a follow-up. Note also that nothing in make check catches this class; a lint asserting every operation in a service family agrees on resourceType would.

Also unfixed and unrelated: several per-SDK README service tables list stale method rosters — typescript/README.md:479 shows forRecording, forProject, report (missing get/create/update), go/README.md:460 shows MyEntries, ProjectEntries for methods actually named Report/ProjectReport/RecordingReport/Get/Create/Update/Trash. Adding only destroy would have made them half-true.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6c2668ef90

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

This PR is Lane B2a of the v0.13.0 coverage work. It models one genuinely missing endpoint — DELETE /timesheet_entries/{entryId} as the new DestroyTimesheetEntry operation — across all six SDKs (Go, TypeScript, Ruby, Swift, Kotlin, Python), following the Smithy-first generation pipeline. It also retires five bc3_routes_not_modeled dispositions from issues #581/#582 that were false gaps (bc3 draws those routes under both a project-scoped and a flat spelling, and the SDK already models the flat form), introducing a new checked modeled_as disposition in the route-parity gate to prove that claim rather than assert it.

Changes:

• Adds DestroyTimesheetEntry (DELETE, 204, @idempotent, natural: true, retry [429,503] max 3) to the Smithy spec and regenerates all downstream artifacts (openapi, schema, metadata, per-SDK services, Go generated client), plus a hand-written Go Timesheet.Destroy wrapper mirroring Trash.
• Adds a modeled_as disposition to scripts/check-bc3-route-parity (suffix-over-flattened-segments verification) and marks the five #581/#582 routes as covered by their flat spellings.
• Adds a conformance test enforcing the flat /timesheet_entries/{entryId} path, wires it into all six runners, and updates operation counts (240→241, idempotent 78→79, DELETEs 23→24) in SPEC.md, SECURITY.md, AGENTS.md, and the idempotency-parity script.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 25 out of 39 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
spec/basecamp.smithy	Defines the new DestroyTimesheetEntry operation; first-line of /// doc trails off mid-sentence (see comment)
spec/overlays/tags.smithy	Tags the operation into the Schedule service group
openapi.json / typescript/src/generated/openapi-stripped.json / schema.d.ts / path-mapping.ts / metadata.ts	Regenerated OpenAPI + TS artifacts for the new op
go/pkg/generated/client.gen.go / url-routes.json / go/pkg/basecamp/timesheet.go	Generated Go client method + route entry; hand-written Destroy wrapper mirroring Trash
ruby / python / kotlin / swift generated services, metadata, generator configs	Per-SDK destroy methods, retry/idempotency metadata, service-split + method-name overrides
conformance/tests/paths.json + 6 runners	New flat-path conformance test dispatched by all six SDK runners
scripts/check-bc3-route-parity	New modeled_as disposition with suffix-check verification and disposition breakdown output
spec/bc3-route-allowlist.yml	Retires 5 phantom gaps as modeled_as, removes the real DELETE gap
SPEC.md / SECURITY.md / AGENTS.md / scripts/check-idempotency-parity	Operation-count updates; SPEC.md §7 counts were not updated (see comment)
behavior-model.json	Retry/idempotency entry for the new op

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d89dc6fd4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 25 out of 39 changed files in this pull request and generated no new comments.

Suppressed comments (1)

spec/basecamp.smithy:3082

• The first line of this operation's description ends mid-clause ("...Returns 403 when the caller may not"). The TypeScript, Ruby, and Kotlin service generators emit only the first line of the description as the method doc comment (op.description.split("\n")[0] in typescript/scripts/generate-services.ts:1156, operation['description']&.lines&.first in ruby/scripts/generate-services.rb:395, and op.description.lines().first() in kotlin/generator/.../ServiceEmitter.kt:67). As a result, the generated destroy docstrings in typescript/src/generated/services/timesheets.ts:299, ruby/lib/basecamp/generated/services/timesheets_service.rb:86, and kotlin/.../services/timesheets.kt:167 all read "Permanently delete a timesheet entry. Returns 403 when the caller may not" and drop "archive or trash the entry", leaving an incomplete sentence in the public API docs. Reflowing so the first line is a complete sentence fixes all three (the full text is still preserved in openapi.json/schema.d.ts). Regenerate the downstream artifacts after editing the spec.

/// Permanently delete a timesheet entry. Returns 403 when the caller may not
/// archive or trash the entry.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 44 changed files in this pull request and generated no new comments.

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: e75d69de1c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

• .github/workflows/test.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.