Skip to content

fix(report): keep the delete credential when the record cannot be written - #633

Merged
theCodeDrift merged 2 commits into
mainfrom
fix/622-keep-report-delete-credential
Nov 8, 2026
Merged

theCodeDrift merged 2 commits into
mainfrom
fix/622-keep-report-delete-credential

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

taskless report --from publishes a report, then writes .taskless/reports/<stamp>.json, the only place the report's delete URL is kept. The stamp has one-second resolution and the write refuses to overwrite, so two publishes in one second made the second write throw EEXIST after its report was already live. An unwritable reports directory failed the same way. Either way the raw fs error reached the top-level handler, which printed only its message: the report stayed live for 30 days with no way to delete it, and the user never saw its URL.

What changed

  • Collision-proof record names (report/records.ts). A taken name moves to <stamp>_2.json, <stamp>_3.json, … (up to 100 tries), still with wx, so nothing is overwritten. _ sorts after ., so a suffixed record sorts after the one it collided with. --delete already accepts any .json directly in .taskless/reports/, so suffixed names resolve unchanged.
  • New code REPORT_RECORD_FAILED. When the record still cannot be written, writeRecord throws a CLIError whose message says the report is live, gives its URL, expiry and delete URL, and shows how to delete it. Printing the credential is the lesser harm compared with a page nobody can take down.
  • --json envelope carries the credential. CLIError gains optional details, and CLIErrorEnvelope gains optional url, deleteUrl and expiresAt (CLIErrorDetails), present only for this code. report's fail() passes error.details to writeJsonError. That one-line change is the only edit outside the publish path in commands/report.ts, so this merges cleanly with fix(onboard): refuse a behind .taskless/ in onboard and report instead of migrating #630 in either order.
  • Recipe: report lists the new code (topic v8).
  • OpenSpec: cli-report's "A published report is recorded and summarized" restated with the suffix rule and the failure case, plus two scenarios. Archived in this PR, and all 53 prior scenarios are still present.
  • Changeset: patch.

Testing

  • writeRecord unit tests: a taken stamp yields _2 and then _3, the existing record is unchanged, readRecord resolves the suffixed name, and it sorts after the original. A read-only reports directory throws REPORT_RECORD_FAILED with the URL and delete URL in the message and in details.
  • End to end through the built CLI against a fake tskl.es: with every stamp the run could land on pre-taken, the publish succeeds and records under _2 with the right delete URL. With .taskless/reports/ read-only, the publish exits non-zero, stderr carries the code, URL and delete URL, and the --json envelope carries url, deleteUrl and expiresAt.
  • pnpm typecheck, pnpm lint and pnpm --filter @taskless/cli test (2970 passed) all pass.

Fixes #622

@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 1m 19s —— View job


Review complete ✅

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviews, review threads, or top-level comments beyond the trigger comment itself, so this is the first substantive review of this PR — everything below is assessed fresh, not reclassified against prior findings.

  • Read prior review data (.prior-review.json) — empty, first review
  • Fetch PR diff and changed files
  • Review report/records.ts collision logic
  • Review types/errors.ts / cli-error.ts changes
  • Review report.ts wiring and tests
  • Review OpenSpec/changeset docs
  • Post final review

Summary

This is a clean, well-scoped fix. The collision handling in writeRecord (packages/cli/src/report/records.ts:55-109) is correct: the wx flag keeps every write non-overwriting, the retry loop only advances past a name on EEXIST, and any other failure (including a read-only directory) falls through to the single REPORT_RECORD_FAILED throw carrying the URL/delete URL/expiry both in the message and in details. The plumbing through CLIError.details → CLIErrorEnvelope/makeErrorEnvelope/writeJsonError (packages/cli/src/types/errors.ts, packages/cli/src/util/cli-error.ts) is additive and backward compatible — all other call sites pass no details and are unaffected. commands/report.ts's one-line change (passing error.details through fail()) is the only touch outside the publish path, matching the PR's claim about merging cleanly with #630.

recordPath's validation (records.ts:117-130) correctly accepts suffixed names like <stamp>_2.json since it only requires the resolved path to live directly in the reports directory and end in .json — --delete works unchanged, confirmed by the new unit test.

Tests are thorough: unit coverage for both the suffix-assignment and read-only-directory paths, plus end-to-end coverage through the built CLI for both --json and human output, including a reasonably clever test that pre-occupies every stamp name the real run could land on to force a real collision deterministically.

One minor/informational point (inline)

  • packages/cli/src/report/records.ts:34-38 — the comment acknowledges that suffixes past _9 sort before _2.._9 lexicographically, calling this cost-free since "order is not publish order anyway." That's true for the stated purpose, but it's a slight gap against the PR description's more general claim that "a suffixed record sorts after the one it collided with" — true for the common one-collision case, not guaranteed once ≥10 same-second collisions occur. Extremely unlikely in practice (would need 10+ publishes landing on the exact same UTC second), so not blocking — just flagging the discrepancy between the code comment's honest caveat and the PR description's broader claim.

No correctness, security, or test-adequacy issues found beyond that nit. Docs (openspec/specs/cli-report/spec.md, the archived change, agent/report.md's v8 recipe bump, and the changeset) are all consistent with the implementation.

Comment thread packages/cli/src/report/records.ts Resolved
@theCodeDrift

Copy link
Copy Markdown
Member Author

Re: @github-actions — "Claude finished @theCodeDrift's task in 1m 19s"
#633 (comment)

Thanks. The one inline point, about suffixes past _9, is declined with the reasoning in its thread: same-second order is not publish order, and every suffix still sorts after the bare stamp.

— AI Coding Agent

…tten

Two publishes in one second collided on the record name, and an
unwritable reports directory failed the same way, after the report was
already live. A taken name now moves to a _N suffix, and a record that
still cannot be written exits with REPORT_RECORD_FAILED carrying the URL
and delete URL in the message and the --json envelope.
@theCodeDrift
theCodeDrift force-pushed the fix/622-keep-report-delete-credential branch from 942420a to a9c30dd Compare November 8, 2026 05:44
@theCodeDrift
theCodeDrift merged commit 4ec8a41 into main Nov 8, 2026
4 checks passed
@theCodeDrift
theCodeDrift deleted the fix/622-keep-report-delete-credential branch November 8, 2026 05:48
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.

report --from loses the only delete credential when the local record cannot be written

1 participant