Add warning at delete contents for Linux users on @fedify/init - #992
Add warning at delete contents for Linux users on @fedify/init#992lego37yoon wants to merge 2039 commits into
@fedify/init#992Conversation
The deduplication section used bold for label and warning phrases where italics carry enough emphasis, and applied the markdown-it-jsr-ref `~` prefix to standalone type names. The `~` prefix is only for member references such as `~KvStore.cas` that hide the owner type; a standalone type name should not use it. Lower the four labels/warnings to italics and drop the `~` from the two standalone `KvStore` and `ParallelMessageQueue` references, leaving member references untouched. fedify-dev#806 (comment) fedify-dev#806 (comment) fedify-dev#806 (comment) fedify-dev#806 (comment) fedify-dev#806 (comment) fedify-dev#806 (comment) Assisted-by: Claude Code:claude-opus-4-8
The introduction to the deduplication section read as if a `deduplicationKey` always drops a duplicate enqueue. That is not true under the default "open" fallback, where a queue without native deduplication and a key–value store without `cas` proceed without deduplication after a debug log. Reword the sentence so the key is described as requesting at-most-once-per-key enqueue, with whether the drop actually happens deferred to the fallback rules that follow. fedify-dev#806 (comment) Assisted-by: Claude Code:claude-opus-4-8
On the cas fallback path, planDeduplication only rejected a multi-item deduplicated batch when `queue.enqueueMany` was absent. A `ParallelMessageQueue` always exposes `enqueueMany()`, so the guard was skipped even when the wrapped queue lacked it; the cas plan then claimed one marker and `ParallelMessageQueue.enqueueMany()` fanned the batch out to individual `enqueue()` calls (the wrapper only throws when a `deduplicationKey` is forwarded, which the cas path never does). That silently fanned out a batch the docs promise to reject and left it non-atomic, so a partial failure could duplicate the messages that already succeeded. Look through the wrapper: a `ParallelMessageQueue` whose wrapped queue has no `enqueueMany()` cannot enqueue a batch atomically, so the batch is now rejected before any marker is claimed. The native path was already covered because it forwards the key and the wrapper rejects it there. Add a regression test for a deduplicated multi-item batch over a `ParallelMessageQueue` wrapping a non-native, no-`enqueueMany` backend on the cas path. fedify-dev#806 (comment) Assisted-by: Claude Code:claude-opus-4-8
Context.enqueueTask() and enqueueTaskMany() now accept a
deduplicationKey requesting at-most-once enqueue for tasks that share
it (new TaskEnqueueOptions.deduplicationKey).
Resolution follows the queue and key-value store capabilities:
- A queue declaring the new MessageQueue.nativeDeduplication owns the
check; the key is forwarded through the new
MessageQueueEnqueueOptions.deduplicationKey.
- Otherwise Fedify applies a best-effort guard through the optional
KvStore.cas primitive under a new taskDeduplication key prefix,
tunable with the new FederationOptions.taskDeduplicationTtl and
taskDeduplicationFallback options.
For enqueueTaskMany(), a single key governs the whole batch. A native
queue that does not implement enqueueMany() cannot express batch-level
at-most-once with a per-message key, so such a multi-item enqueue is
rejected with a TypeError instead of silently leaking duplicates.
Configuration errors that are decidable without a payload (a native
queue lacking enqueueMany, or a closed fallback without cas) are
checked before payloads are validated and encoded, so they reject
before any user schema runs or any key is reserved.
fedify-dev#798
Assisted-by: Claude Code:claude-opus-4-8
Layer task-specific telemetry onto the custom background task dispatch path, reusing the queue-task metric pattern and mirroring the existing `http_signatures.failure_reason` enum in metrics.ts. Each dequeued task now runs in a `fedify.task` span that inherits the enqueue site's trace context and carries `fedify.task.name`, `fedify.task.attempt`, and, on a terminal failure, `fedify.task.failure_reason`. The `fedify.queue.task.*` metrics report task runs under the new `"task"` role with the task name and, on failure, a bounded `fedify.task.failure_reason`. To tell the failure reasons apart, `#listenTaskMessage` splits the former `decode()` call into its deserialize and validate phases and returns the decision point that failed: `deserialization`, `validation`, `unknown_task`, or `handler`. A swallowed abort is reported as a graceful interruption, not a failure. The reported `fedify.queue.backend` reflects the resolved queue so it stays accurate under the outbox fallback. Public surface: `QueueTaskRole` gains `"task"`, `QueueTaskCommonAttributes` gains `taskName`, and a new `QueueTaskFailureReason` type plus an optional trailing `failureReason` parameter on `recordQueueTaskOutcome()` carry the reason. `TaskCodec` exposes an instance `validate()` wrapper so the dispatch site can split decoding without importing the class. fedify-dev#799 Assisted-by: Claude Code:claude-opus-4-8
Deno executes the TypeScript sources directly, so `test:deno` spent most of its time on a `build` it never needed: with no dist/ output present, the whole Deno suite passes except the npm packaging regression tests added for fedify-dev#655, which assert that the built package.json entry points of `@fedify/cli`, `@fedify/create`, and `@fedify/init` exist. Those checks guard the npm artifacts, not the Deno runtime, and still run under `test:node` and `test:bun`, which build first—so skip them under Deno and drop the `build` dependency from `test:deno`. `@fedify/lint`'s oxlint integration test already skips itself when *dist/oxlint.js* is absent. Update AGENTS.md to match: document `mise run build`/`prepare-each` for building, `check-each` and `test-each` for scoping work to specific packages, recommend the now build-free `test:deno` as the default test loop during development, and add a section directing agents to consult `mise tasks`. Assisted-by: Claude Code:claude-opus-4-8 Assisted-by: Claude Code:claude-fable-5
The fedify.task span and the fedify.queue.task.* metrics promised that
fedify.task.failure_reason is set only on a terminal failure, but the
task worker attributed a handler failure to every thrown attempt, even
when the retry policy had just scheduled a re-enqueue. A transient
error that later succeeded thus produced a failed measurement and an
ERROR span, inflating failure alerts and diverging from the
inbox/outbox convention where an attempt folded into a retry records
result=completed.
To fix this at the decision point instead of patching each call site,
the task listener now returns a dispatch result that distinguishes the
outcome:
- A handler error folded into a scheduled retry records completed.
- Only a terminal give-up records failed with the handler reason.
- An aborted attempt that the retry policy abandons records aborted
rather than completed, so tasks dropped during a graceful shutdown
are no longer invisible to telemetry.
TaskCodec.decode() now reports which phase failed (deserialization or
validation) instead of throwing, so the worker no longer re-composes
the codec's deserialize/validate pipeline inline to tell the two
apart; the redundant instance validate() wrapper is removed.
The retry-path test now pins the decided semantics (no failed
measurement, span status UNSET), a new test covers the abandoned-abort
case, and the manual's failed/aborted definitions are updated to match.
fedify-dev#812 (comment)
fedify-dev#812 (comment)
Assisted-by: Claude Code:claude-fable-5
Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Codex:gpt-5.5
The fedify.queue.task.enqueued counter was recorded only after the whole dispatch resolved, so on a queue without enqueueMany a batch fanned out through Promise.all could lose measurements: one rejected enqueue aborted the batch before any recording, leaving messages that had already reached the backend uncounted and skewing the enqueued-versus-completed reconciliation. The fan-out path now uses Promise.allSettled and records each message right after its individual enqueue succeeds, mirroring how the outbox delivery path counts partial successes, and rethrows the first rejection afterwards. The single-message and enqueueMany paths record the whole batch with one counter add carrying the batch size, so recordQueueTaskEnqueued() gains an optional count parameter instead of being called once per identical-attribute message. FederationImpl also gains a metrics getter wrapping the memoized getFederationMetrics() lookup, replacing the scattered per-call-site invocations. fedify-dev#812 (review) Assisted-by: Claude Code:claude-fable-5 Assisted-by: Codex:gpt-5.5
Version errors are found by `Claude Code:fable-5` fedify-dev#812 (comment)
Addresses maintainer review feedback on the custom-task
observability additions. All four changes clarify existing
semantics; none change telemetry behavior.
- The `fedify.queue.task.failed` metric summary said tasks are
counted as failed "because processing threw," which excluded the
dispatch-stage drops (`deserialization`, `validation`,
`unknown_task`) that are recorded as failed without the handler
throwing. The summary now covers both a processing throw and a
dispatch drop.
- The `fedify.queue.task.enqueued` counter is emitted at the enqueue
site, not during the worker run, so grouping it under "task run
measurement" was misleading. The docs now state that `enqueued`
is recorded at the enqueue site while `started`, `completed`,
`failed`, and `duration` are recorded during the worker run.
- A short operational example now precedes the OpenTelemetry
cross-reference, showing how to split schema drift from handler
give-ups via `fedify.task.failure_reason` and how to spot retry
re-enqueues via `fedify.queue.task.attempt > 0`.
- The `completed` outcome returned after scheduling a retry now
carries a comment explaining it means "the attempt was folded
into a retry," not "the handler succeeded," to keep a future
change from regressing the terminal-failure-only task telemetry.
fedify-dev#812 (comment)
fedify-dev#812 (comment)
fedify-dev#812 (comment)
fedify-dev#812 (comment)
Assisted-by: Claude Code:claude-opus-4-8
When a custom task's handler failed and a retry was scheduled, but
re-enqueuing the retry message to the queue backend itself threw, the
worker boundary recorded `fedify.task.failure_reason` as `handler` and
re-threw the enqueue error, shadowing the original handler error. An
infrastructure failure was thus indistinguishable from a handler fault,
and the documented failure reasons map to dispatch decision points, none
of which is a re-enqueue failure.
Because the failure-reason set is a small bounded set that is open to
refinement, this adds a distinct `retry_enqueue` value rather than
folding the case into `handler`. One extra value keeps the metric
cardinality bounded (`taskName x |failure_reason| x backend`).
- `QueueTaskFailureReason` gains `retry_enqueue`.
- The retry re-enqueue is now wrapped narrowly; on failure the worker
logs it and re-throws a `TaskRetryEnqueueError` that preserves the
original handler error as its `cause`. The throw keeps the message
nacked rather than dropped, and the worker boundary records
`retry_enqueue` with the handler error surfaced on the span.
- `TaskRetryEnqueueError` and `QueueTaskDispatchResult` move out of
middleware.ts into dedicated `tasks/error.ts` and `tasks/types.ts`
modules, re-exported through the task barrel.
- The tasks and OpenTelemetry manuals document the new value, and a
worker test covers the re-enqueue-failure path.
Assisted-by: Claude Code:claude-opus-4-8
The fanout, outbox, inbox, and task branches of FederationImpl.processQueuedTask() each repeated the same consumer-span and metric boilerplate: open a CONSUMER span, scope the extracted trace context, record the started/outcome/duration metrics, pair the in-flight increment with its decrement, set the span status, and end the span. The task branch had also drifted from the other three, recording its outcome outside the finally block that guarantees it. Collapse the four copies onto two helpers. #runWorkerSpan() opens the span and scopes the trace context; #instrumentWorkerBody() wraps a worker body with the shared boundary telemetry. Each branch now supplies only its span name, span attributes, common metric attributes, body, and error classifier. The outcome record lives in one finally block again, so a "failed" outcome that arrives through the body's return value (a dropped or given-up task) is instrumented the same way as one that arrives through a thrown error. Unify the former WorkerSpanOutcome type into QueueTaskDispatchResult (failureReason and error are now optional on the failed variant), so the task dispatch result and the worker-boundary outcome share one type, and move the classifyAbortableError and classifyTaskError classifiers into tasks/error.ts. fedify-dev#812 Assisted-by: Claude Code:claude-opus-4-8
Running only `mise run test:deno` by itself is not a problem. However, when running `mise run test`, `test:deno` and `build` would run simultaneously, often causing errors. To prevent this, the `wait_for` attributes are added in the `test:deno` task with `build` item.
The test drove the first marker's expiry through a real 1 ms taskDeduplicationTtl, but that TTL equally applied to the marker the second enqueue re-claimed. Every assertion after the re-claim therefore had to run within 1 ms of wall clock before MemoryKvStore's lazy expiry check judged the marker expired. Local runs finish those microtask hops in time, but under CI load—especially `mise run test:bun`, which runs every package's tests in parallel on a Temporal polyfill—the window was routinely blown, failing the test intermittently. Simulate the first marker's expiry deterministically by deleting the key instead, and stretch the TTL to one minute so the second marker comfortably outlives the assertions. This removes the wall-clock dependence entirely and also drops the 20 ms delay. Assisted-by: Claude Code:claude-fable-5
…d the final PR number in changelog
By [JSR #1473](jsr-io/jsr#1473), the docs building task broken. For that, @dahlia updates [`markdown-it-jsr-ref`](dahlia/markdown-it-jsr-ref#1). Assisted-by: Claude Code:claude-opus-4-8
Add custom background tasks in worker
Add vocab-runtime helpers for canonicalizing and comparing FEP-ef61 portable URI identifiers. The comparison form accepts both ap: and ap+ef61:, decodes DID authorities, strips query hints, and preserves path and fragment identity. Document the helper behavior and cover decoded, encoded, query, fragment, casing, and rejection cases in the URL runtime tests. Closes fedify-dev#828 Assisted-by: Codex:gpt-5.5
Keep the comparison canonicalizer from using URL-normalized path components, because FEP-ef61 treats portable URI paths as opaque object identity strings. Also normalize percent-encoding case in DID authorities so equivalent authority spellings compare consistently. fedify-dev#924 (comment) fedify-dev#924 (comment) fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Make the portable URI comparison helpers accept raw strings only, since URL objects may already have normalized opaque path segments before the helpers can compare identity. Keep the redundant pattern check out of the validated path and document the string-only comparison contract. fedify-dev#924 (comment) fedify-dev#924 (comment) fedify-dev#924 (comment) fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Clarify that portable URI comparison normalizes percent-encoding case only for the DID authority. Path and fragment components remain byte-for-byte from the raw match because FEP-ef61 treats portable object paths as opaque identity strings. fedify-dev#924 (review) Assisted-by: Codex:gpt-5.5
Normalize percent-escape hex casing in preserved path and fragment components, while still reading those components from the raw URI string so URL dot-segment normalization cannot collapse portable object identity. Also make arePortableUrisEqual() fall back to direct string comparison for non-portable strings. Portable-looking inputs still use the strict portable URI parser, so malformed portable authorities continue to fail instead of being silently accepted. fedify-dev#924 (comment) fedify-dev#924 (comment) fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Clarify the public helper contracts for portable URI canonicalization and comparison, including the strict fallback used for non-portable inputs and the parser errors that remain visible for malformed portable URI strings. Add regression coverage for multiple percent-encoded triplets in one URI segment so normalization is checked across every escape match. fedify-dev#924 (review) Assisted-by: Codex:gpt-5.5
Make the equality helper robust for malformed portable URI strings while keeping canonicalization strict. Identical strings can still compare equal, but malformed or mixed portable inputs no longer escape as TypeError from the comparison path. Encode raw path and fragment characters before comparing canonical portable URI forms, while continuing to preserve dot segments from the original raw string and normalize existing percent escapes. fedify-dev#924 (comment) fedify-dev#924 (comment) fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Validate path and fragment percent escapes before canonicalizing portable URI components. This keeps malformed inputs from being normalized into the same canonical form as a different valid URI string during equality checks. fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Normalize over-encoded unreserved characters in portable DID authorities so URL-safe spellings compare equal to their decoded form. Reserved DID-internal escapes such as encoded slashes and colons remain percent-encoded to preserve existing authority identity semantics. fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Normalize percent-encoded unreserved characters in portable URI paths and fragments so escaped and decoded spellings compare equal. Reserved escapes remain percent-encoded, and raw dot segments are still preserved instead of being collapsed through URL pathname normalization. fedify-dev#924 (comment) fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Remove the redundant bare-percent branch from portable component normalization. Invalid percent escapes are already rejected before matching, so percent matches are always complete escape triplets. fedify-dev#924 (comment) Assisted-by: Codex:gpt-5.5
Vite does not inject `.env` variables into `process.env`, so non-Deno SvelteKit projects failed during `mise test:init` runs that need KV/MQ connection settings (e.g. with `mysql`). Add `@dotenvx/dotenvx` as a dev dependency and wrap the `dev`/`build`/`preview` tasks with `dotenvx run --` for every non-Deno package manager. fedify-dev#892 Assisted-by: Claude Code:claude-sonnet-5
Rebasing onto main replayed this branch's original commit that hand-edited CHANGES.md before Sacho was adopted, instead of the changes.d/init/sveltekit.md fragment created while resolving the earlier merge. Rebase discards merge commits, so that fragment, which only ever existed inside the abandoned merge commit, was lost. Recreated changes.d/init/sveltekit.md and let `sacho sync --force` regenerate the Unreleased @fedify/init section from it alongside the existing hydration-test-fix fragment. Also restored the [SvelteKit] link definition in the released Version 1.3.0 section, which the formatter relocated into the Unreleased region again, to keep it from tripping Sacho's "outside the unreleased region" check. fedify-dev#892 Assisted-by: Claude Code:claude-sonnet-5
…it-fedify-init Add SvelteKit support to `fedify init`
- add next as dependency and deno.json - skip Deno tests for packages without deno.json - add test scripts in next/package.json Assisted-by: OpenCode:gpt-5.6-terra
Assisted-by: OpenCode:gpt-5.6-terra
Fix fedify/next test-each failure, add fedify/next tests
✅ Deploy Preview for fedify-json-schema canceled.
|
📝 WalkthroughWalkthrough
ChangesProject directory cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FedifyInit
participant ActionPlanner
participant CommandExecutor
FedifyInit->>ActionPlanner: Select cleanup mode by operating system
ActionPlanner->>FedifyInit: Request mode-specific confirmation
FedifyInit->>CommandExecutor: Execute trash or permanent-delete command
CommandExecutor-->>FedifyInit: Return success or mode-specific failure
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changes.d/init/permanent-delete.md`:
- Around line 1-2: Correct the release-note grammar in
changes.d/init/permanent-delete.md lines 1-2 and CHANGES.md lines 178-179 by
changing “before delete” to “before deleting” and referring to the existing
contents.
In `@packages/init/src/ask/dir.ts`:
- Around line 123-127: Update the command builder around
getPowershellTrashCommand to escape apostrophes in dir by doubling them before
interpolation into the PowerShell single-quoted literal. Preserve existing path
behavior and add a Windows test covering a directory path containing an
apostrophe.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 593d4915-bb1e-4486-9907-cc88fd8527b3
📒 Files selected for processing (3)
CHANGES.mdchanges.d/init/permanent-delete.mdpackages/init/src/ask/dir.ts
| command: (dir: string) => [ | ||
| "powershell", | ||
| "-Command", | ||
| getPowershellTrashCommand(dir), | ||
| ], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching dir.ts:"
fd -a 'dir\.ts$' . || true
file="$(fd 'dir\.ts$' . | grep 'packages/init/src/ask/dir.ts' | head -n 1 || true)"
echo "Target file: $file"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo
echo "Search PowerShell command construction:"
rg -n "getPowershellTrashCommand|powershell|DeleteDirectory|Trash|trash" packages/init/src -S || true
echo
echo "Search dir input paths/usages in init ask flow:"
rg -n "command:\s*\(dir: string\) => \[|getPowershellTrashCommand|selectDirectory|ask.*dir|dir" packages/init/src -S || trueRepository: fedify-dev/fedify
Length of output: 40650
🌐 Web query:
PowerShell single-quoted string escaping single quote doubled two single quotes
💡 Result:
To include a single quotation mark within a PowerShell single-quoted string, you must use two consecutive single quotation marks ('') [1][2]. This effectively escapes the quote by "doubling" it, which tells the PowerShell parser to treat the pair as a single literal quote character [3][4]. For example, to represent the word don't in a single-quoted string, you would write: 'don''t' Because single-quoted strings are treated as verbatim literals, other common escape methods—such as the backtick character (`)—do not function as escape sequences inside them [1][5][3]. The backtick is treated as a literal character when placed within single quotes [1][6]. If you require complex quoting or find doubling single quotes difficult to manage, you can alternatively use a here-string (@'... '@), which preserves the content literally without requiring special escaping for single quotes [2][4].
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.6
- 2: https://ss64.com/ps/syntax-esc.html
- 3: https://www.red-gate.com/simple-talk/sysadmin/powershell/when-to-quote-in-powershell/
- 4: https://stackoverflow.com/questions/22525381/escaping-single-quote-sign-in-powershell
- 5: https://stackoverflow.com/questions/11231410/can-i-use-a-single-quote-in-a-powershell-string
- 6: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.5
Escape dir in the PowerShell trash command.
getPowershellTrashCommand() inserts dir into a PowerShell single-quoted literal, so a name like C:\O'Brien terminates the string. In PowerShell single quotes, escape an apostrophe by doubling it (''). Add a Windows test with an apostrophe-containing directory path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/init/src/ask/dir.ts` around lines 123 - 127, Update the command
builder around getPowershellTrashCommand to escape apostrophes in dir by
doubling them before interpolation into the PowerShell single-quoted literal.
Preserve existing path behavior and add a Windows test covering a directory path
containing an apostrophe.
dahlia
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
Since it's not a new feature, but a bug fix, could you change the target branch of this pull request to 2.0-maintenance (instead of main)?
|
I changed the target branch now, but without rebase it pulls the whole changes in |
|
I will review this after the commit history is cleaned up. Consider opening a new PR with a clean history like this:
|
Summary
@fedify/initproject initialization asks users to use non-empty directory, and confirm to move existing contents to Trash. However, its behavior actually permanently delete all of existing contents without back up for Linux or UNIX-like system users.This pull request notices users who are using Linux or UNIX-like systems that this action deletes existing data permanently. Furthermore, refactored messages and action structure in
dir.tsfile to easily add conditions or supported systems.Fixes #989
Test Plan
mise run check-each initmise run test-each init