From ad863a86e250eb37c1785323155f4a8ecf74db07 Mon Sep 17 00:00:00 2001 From: Jason Mulligan Date: Thu, 25 Jun 2026 21:36:58 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat:=20fix=20audit=20findings=20=E2=80=94?= =?UTF-8?q?=20padding=20logic,=20isNaN=20clarity,=20structuredClone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../.openspec.yaml | 2 ++ .../fix-audit-findings-filesize-js/design.md | 32 +++++++++++++++++++ .../proposal.md | 30 +++++++++++++++++ .../specs/number-formatting/spec.md | 28 ++++++++++++++++ .../specs/partial-application/spec.md | 25 +++++++++++++++ .../fix-audit-findings-filesize-js/tasks.md | 27 ++++++++++++++++ 6 files changed, 144 insertions(+) create mode 100644 openspec/changes/fix-audit-findings-filesize-js/.openspec.yaml create mode 100644 openspec/changes/fix-audit-findings-filesize-js/design.md create mode 100644 openspec/changes/fix-audit-findings-filesize-js/proposal.md create mode 100644 openspec/changes/fix-audit-findings-filesize-js/specs/number-formatting/spec.md create mode 100644 openspec/changes/fix-audit-findings-filesize-js/specs/partial-application/spec.md create mode 100644 openspec/changes/fix-audit-findings-filesize-js/tasks.md diff --git a/openspec/changes/fix-audit-findings-filesize-js/.openspec.yaml b/openspec/changes/fix-audit-findings-filesize-js/.openspec.yaml new file mode 100644 index 0000000..578bd54 --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-26 diff --git a/openspec/changes/fix-audit-findings-filesize-js/design.md b/openspec/changes/fix-audit-findings-filesize-js/design.md new file mode 100644 index 0000000..ba8ef9f --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/design.md @@ -0,0 +1,32 @@ +## Context + +filesize.js is a lightweight, zero-dependency JavaScript utility for converting bytes to human-readable strings. The codebase has been optimized for performance with cached lookups and pre-computed values. An audit identified three issues: a padding logic bug in helpers.js, an isNaN clarity issue, and a JSON deep clone fragility in the partial function. + +## Goals / Non-Goals + +**Goals:** +- Fix the padding logic bug so excess decimal places are truncated when separator and pad options are combined +- Improve code clarity with isNaN(num) instead of isNaN(arg) +- Replace JSON deep clone with structuredClone for robustness + +**Non-Goals:** +- No changes to the public API +- No changes to performance-critical paths beyond the bug fix +- No new dependencies or features + +## Decisions + +1. **Round before separator replacement** — The value must be rounded to `round` decimal places before the decimal point is replaced with the separator. This ensures the separator character doesn't interfere with truncation. Alternative (truncate after separator) was rejected because it would require distinguishing the decimal separator from grouping separators. + +2. **structuredClone with JSON fallback** — `structuredClone` is available in Node 17+, but the package supports Node 10.8.0+. We'll use `structuredClone` with a fallback to `JSON.parse(JSON.stringify())` for older versions. This preserves compatibility while using the modern API when available. + +3. **No breaking changes** — All fixes are backward compatible. The padding bug fix changes behavior only for the specific edge case where separator and pad are both set with excess decimal places. + +## Risks / Trade-offs + +- [Risk] Rounding before separator replacement could affect values that rely on the current (buggy) behavior → [Mitigation] The current behavior is incorrect per the documented `round` parameter, so fixing it is the right call. Tests will catch any regressions. +- [Risk] structuredClone fallback adds code complexity → [Mitigation] The fallback is a simple ternary: `typeof structuredClone === 'function' ? structuredClone(obj) : JSON.parse(JSON.stringify(obj))` + +## Open Questions + +- None. The audit findings are clear and the fixes are straightforward. diff --git a/openspec/changes/fix-audit-findings-filesize-js/proposal.md b/openspec/changes/fix-audit-findings-filesize-js/proposal.md new file mode 100644 index 0000000..ebef353 --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/proposal.md @@ -0,0 +1,30 @@ +## Why + +An audit of the filesize.js source code identified three issues that affect correctness and code quality: + +1. A medium-severity bug in `helpers.js` where the padding logic fails to truncate excess decimal places when both `separator` and `pad` options are set, producing output with more decimal digits than the `round` parameter specifies. +2. A low-severity clarity issue in `filesize.js` where `isNaN(arg)` is used after `num = Number(arg)` was already computed — `isNaN(num)` would be more intentional. +3. A low-severity fragility issue in the `partial` function where `JSON.parse(JSON.stringify())` is used for deep cloning, which silently drops functions, undefined, Dates, and RegExps. + +These fixes improve correctness (the padding bug), code clarity (isNaN), and future-proofing (structuredClone). + +## What Changes + +- Fix `applyNumberFormatting` in `helpers.js` to round the value to `round` decimal places BEFORE applying separator replacement and padding, ensuring excess decimal places are truncated. +- Replace `isNaN(arg)` with `isNaN(num)` in the `filesize` function for clarity. +- Replace `JSON.parse(JSON.stringify())` with `structuredClone()` in the `partial` function for robust deep cloning, with a fallback for older Node versions. + +## Capabilities + +### New Capabilities +- `number-formatting`: Correct truncation of decimal places when separator and pad options are combined + +### Modified Capabilities +- `partial-application`: Use structuredClone instead of JSON serialization for deep cloning options + +## Impact + +- `src/helpers.js` — `applyNumberFormatting` function (rounding applied before separator/padding) +- `src/filesize.js` — `filesize` function (isNaN clarity), `partial` function (structuredClone) +- `tests/unit/filesize-helpers.test.js` — New tests for padding + separator edge case +- `tests/unit/filesize.test.js` — New tests for partial function with structuredClone diff --git a/openspec/changes/fix-audit-findings-filesize-js/specs/number-formatting/spec.md b/openspec/changes/fix-audit-findings-filesize-js/specs/number-formatting/spec.md new file mode 100644 index 0000000..4372864 --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/specs/number-formatting/spec.md @@ -0,0 +1,28 @@ +## ADDED Requirements + +### Requirement: Padding truncates excess decimal places with separator +When both `separator` and `pad` options are set, the formatted value MUST be truncated to the specified `round` decimal places before the separator is applied, ensuring the output never exceeds the requested precision. + +#### Scenario: Truncate excess decimals with separator and pad +- **WHEN** `filesize(1234.567, {separator: ",", pad: true, round: 2})` is called +- **THEN** the result is `"1,234.57"` (not `"1,234.567"`) + +#### Scenario: Truncate multiple excess decimals with separator and pad +- **WHEN** `filesize(1234.5678, {separator: ",", pad: true, round: 2})` is called +- **THEN** the result is `"1,234.57"` (not `"1,234.5678"`) + +#### Scenario: Pad with fewer decimals than round +- **WHEN** `filesize(1234.5, {separator: ",", pad: true, round: 2})` is called +- **THEN** the result is `"1,234.50"` (padded with trailing zero) + +#### Scenario: Separator and pad without excess decimals +- **WHEN** `filesize(1234.56, {separator: ",", pad: true, round: 2})` is called +- **THEN** the result is `"1,234.56"` (no change needed) + +#### Scenario: Pad without separator still works +- **WHEN** `filesize(1234.5, {pad: true, round: 2})` is called +- **THEN** the result is `"1234.50"` (existing behavior preserved) + +#### Scenario: Separator without pad still works +- **WHEN** `filesize(1234.567, {separator: ",", round: 2})` is called +- **THEN** the result is `"1,234.57"` (existing behavior preserved) diff --git a/openspec/changes/fix-audit-findings-filesize-js/specs/partial-application/spec.md b/openspec/changes/fix-audit-findings-filesize-js/specs/partial-application/spec.md new file mode 100644 index 0000000..10b23dd --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/specs/partial-application/spec.md @@ -0,0 +1,25 @@ +## MODIFIED Requirements + +### Requirement: Partial function uses structuredClone for deep cloning +The `partial` function MUST use `structuredClone` for deep cloning `localeOptions`, `symbols`, and `fullforms` options, with a fallback to `JSON.parse(JSON.stringify())` for environments where `structuredClone` is not available. + +#### Scenario: Partial clones plain objects correctly +- **WHEN** `partial({ localeOptions: { minimumFractionDigits: 2 }, symbols: {}, fullforms: [] })` is called +- **THEN** the returned function has independently cloned copies of each option + +#### Scenario: Partial clones with structuredClone when available +- **WHEN** `structuredClone` is available in the runtime +- **THEN** `partial` uses `structuredClone` for deep cloning + +#### Scenario: Partial falls back to JSON clone when structuredClone unavailable +- **WHEN** `structuredClone` is not available in the runtime +- **THEN** `partial` falls back to `JSON.parse(JSON.stringify())` for deep cloning + +## ADDED Requirements + +### Requirement: Partial function preserves non-cloneable values with fallback +When `structuredClone` is not available, the `partial` function MUST maintain backward compatibility by using `JSON.parse(JSON.stringify())` which preserves plain objects, arrays, strings, numbers, booleans, and null. + +#### Scenario: Partial works with plain object options +- **WHEN** `partial({ localeOptions: { localeMatcher: "best fit" } })` is called +- **THEN** the returned function correctly uses the cloned localeOptions diff --git a/openspec/changes/fix-audit-findings-filesize-js/tasks.md b/openspec/changes/fix-audit-findings-filesize-js/tasks.md new file mode 100644 index 0000000..e410b0d --- /dev/null +++ b/openspec/changes/fix-audit-findings-filesize-js/tasks.md @@ -0,0 +1,27 @@ +## 1. Fix padding logic in helpers.js + +- [ ] 1.1 Add rounding step in applyNumberFormatting before separator replacement when separator and pad are both set +- [ ] 1.2 Ensure rounding uses Math.round(value * p) / p where p = Math.pow(10, round) +- [ ] 1.3 Verify the fix doesn't break existing separator-only or pad-only behavior + +## 2. Fix isNaN clarity in filesize.js + +- [ ] 2.1 Replace isNaN(arg) with isNaN(num) in the filesize function + +## 3. Replace JSON deep clone with structuredClone in partial function + +- [ ] 3.1 Add typeof structuredClone check and use structuredClone when available, falling back to JSON.parse(JSON.stringify()) +- [ ] 3.2 Apply the change to localeOptions, symbols, and fullforms cloning + +## 4. Add tests + +- [ ] 4.1 Add test for filesize(1234.567, {separator: ",", pad: true, round: 2}) returning "1,234.57" +- [ ] 4.2 Add test for filesize(1234.5678, {separator: ",", pad: true, round: 2}) returning "1,234.57" +- [ ] 4.3 Add test for partial function with structuredClone availability +- [ ] 4.4 Add test for partial function fallback behavior + +## 5. Verify and commit + +- [ ] 5.1 Run npm test to verify all tests pass +- [ ] 5.2 Run npm run lint to verify lint passes +- [ ] 5.3 Commit and push changes From a123df5beebafec137f8e81bffd7a4f3bf1960bf Mon Sep 17 00:00:00 2001 From: Jason Mulligan Date: Thu, 25 Jun 2026 21:37:49 -0400 Subject: [PATCH 2/5] =?UTF-8?q?feat:=20propose=20fix=20for=20audit=20findi?= =?UTF-8?q?ngs=20#296=20=E2=80=94=20padding=20truncation,=20isNaN=20clarit?= =?UTF-8?q?y,=20deep=20clone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- memory/398eda1062e85f52-feature-goals.md | 82 +++++++++++++++++++ memory/398eda1062e85f52-feature-prompt.md | 29 +++++++ .../.openspec.yaml | 2 + .../design.md | 48 +++++++++++ .../proposal.md | 24 ++++++ .../specs/number-formatting/spec.md | 24 ++++++ .../specs/partial-application/spec.md | 23 ++++++ .../tasks.md | 26 ++++++ 8 files changed, 258 insertions(+) create mode 100644 memory/398eda1062e85f52-feature-goals.md create mode 100644 memory/398eda1062e85f52-feature-prompt.md create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/.openspec.yaml create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/design.md create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/proposal.md create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/number-formatting/spec.md create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/partial-application/spec.md create mode 100644 openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/tasks.md diff --git a/memory/398eda1062e85f52-feature-goals.md b/memory/398eda1062e85f52-feature-goals.md new file mode 100644 index 0000000..e960a54 --- /dev/null +++ b/memory/398eda1062e85f52-feature-goals.md @@ -0,0 +1,82 @@ +# Feature Goals — Fix Audit Findings (Issue #296) + +## Goal 1: Fix padding logic in helpers.js (medium severity) + +**Scope:** +- **Included:** Fix `applyNumberFormatting` in `src/helpers.js` to truncate excess decimal places when both `separator` and `pad` options are set +- **Excluded:** Changes to locale formatting paths (locale=true or locale string), changes to rounding logic, changes to other helper functions + +**Key Requirements:** +1. When `separator` is set and `pad` is true, the function must truncate excess decimal digits to match `round` before applying padding +2. The fix must handle the case where the value has more decimal places than `round` specifies (e.g., `1234.567` with `round: 2` → `1234.57`, then pad if needed) +3. The fix must not affect locale-formatted paths (locale=true or locale string) — those are handled separately +4. The fix must preserve existing behavior when `separator` is not set + +**Acceptance Criteria:** +- `filesize(1234.567, {separator: ",", pad: true, round: 2})` returns `"1,234.57"` (not `"1,234.567"`) +- `filesize(1234.5, {separator: ",", pad: true, round: 2})` returns `"1,234.50"` (padding still works) +- `filesize(1234.567, {separator: ",", pad: false, round: 2})` returns `"1,234.57"` (no padding, but truncation still applies) +- Existing tests for padding and separator continue to pass + +**Dependencies:** +- `src/helpers.js` — `applyNumberFormatting` function +- Existing test suite for padding and separator behavior + +**Risks / Edge Cases:** +- Values with many decimal places (e.g., floating point artifacts like `1.0000000001`) +- Negative numbers with padding and separator +- Round values of 0 (should truncate all decimals) +- Interaction with `locale` option (locale formatting should remain unaffected) + +--- + +## Goal 2: Clarify isNaN check in filesize.js (low severity) + +**Scope:** +- **Included:** Change `isNaN(arg)` to `isNaN(num)` in the `filesize` function for clarity +- **Excluded:** Any logic changes, no behavioral modification + +**Key Requirements:** +1. Replace `isNaN(arg)` with `isNaN(num)` after `num = Number(arg)` has been computed +2. The change is purely cosmetic — both are functionally equivalent in this context +3. Add a brief comment explaining why `num` is used instead of `arg` + +**Acceptance Criteria:** +- Code reads `isNaN(num)` instead of `isNaN(arg)` +- No behavioral change — all existing tests pass +- Linting passes (oxlint/oxfmt) + +**Dependencies:** +- `src/filesize.js` — `filesize` function + +**Risks / Edge Cases:** +- None — this is a pure clarity improvement with no behavioral impact + +--- + +## Goal 3: Replace JSON deep clone in partial function (low severity) + +**Scope:** +- **Included:** Replace `JSON.parse(JSON.stringify())` with a safer deep clone approach in the `partial` function +- **Excluded:** Changes to other functions, changes to the partial function's API or behavior + +**Key Requirements:** +1. Replace `JSON.parse(JSON.stringify(localeOptions))` with a safe deep clone that handles plain objects and arrays +2. Apply the same fix to `symbols` and `fullforms` cloning +3. The replacement must handle the current use case (plain objects/arrays from user options) +4. Consider using structuredClone (available in Node.js 17+) or a simple recursive clone for plain objects + +**Acceptance Criteria:** +- `partial` function no longer uses `JSON.parse(JSON.stringify())` +- Deep cloned values are independent of the original options object +- All existing tests for `partial` continue to pass +- Linting passes (oxlint/oxfmt) + +**Dependencies:** +- `src/filesize.js` — `partial` function +- Node.js 17+ has `structuredClone` available (filesize.js requires Node.js >= 10.8.0, so need a fallback) + +**Risks / Edge Cases:** +- `structuredClone` is not available in Node.js < 17 — need a fallback for older versions +- The fallback should handle plain objects and arrays (current use case) +- Must not break existing behavior for any option type diff --git a/memory/398eda1062e85f52-feature-prompt.md b/memory/398eda1062e85f52-feature-prompt.md new file mode 100644 index 0000000..9aa1205 --- /dev/null +++ b/memory/398eda1062e85f52-feature-prompt.md @@ -0,0 +1,29 @@ +CHANGE_NAME: fix-audit-findings-helpers-padding-clarify-isnan + +## Summary + +Fix three audit findings from issue #296 in the filesize.js codebase: a medium-severity bug in helpers.js where padding with a custom separator fails to truncate excess decimal places, a low-severity clarity issue in filesize.js where `isNaN(arg)` should reference the already-coerced `num` variable, and a low-severity fragility issue where `JSON.parse(JSON.stringify())` is used for deep cloning in the `partial` function. + +## Technical Approach + +### Finding 1: Padding with separator (helpers.js — medium) + +The `applyNumberFormatting` function in `src/helpers.js` handles three formatting paths: locale formatting (when `locale` is true or a string), separator replacement (when `separator` is set), and padding (when `pad` is true). The bug occurs when both `separator` and `pad` are set: the separator replacement runs first, converting `.` to the custom separator (e.g., `,`), then the padding logic runs. However, the padding logic only calls `padEnd(round, ZERO)` on the decimal portion — it does not truncate excess digits. So `filesize(1234.567, {separator: ",", pad: true, round: 2})` produces `"1,234.567"` instead of `"1,234.57"`. + +The fix: before applying `padEnd`, truncate the decimal portion to `round` digits. This should happen in the non-locale padding block (where `locale !== true && locale.length === 0`), after the separator replacement but before the `padEnd` call. The truncation logic should split on the separator, take the decimal portion, truncate it to `round` characters (or to `round` digits if it's numeric), and rejoin. + +### Finding 2: isNaN clarity (filesize.js — low) + +In the `filesize` function, after `num = Number(arg)`, the code checks `isNaN(arg)`. While functionally equivalent (both coerce to number and check for NaN), using `isNaN(num)` is clearer and more intentional — it checks the already-computed numeric value rather than the original argument. This is a one-line change with no behavioral impact. + +### Finding 3: JSON deep clone fragility (filesize.js — low) + +The `partial` function uses `JSON.parse(JSON.stringify())` to deep clone `localeOptions`, `symbols`, and `fullforms`. This works for plain objects and arrays but silently drops functions, undefined, Dates, RegExps, and circular references. Since the current use case only involves plain objects/arrays from user options, the risk is low, but a more robust approach would be preferable. + +The fix: use `structuredClone` if available (Node.js 17+), with a fallback to a simple recursive clone for plain objects and arrays. Since filesize.js targets Node.js >= 10.8.0, the fallback is necessary. The fallback should handle the current use case (plain objects/arrays) without attempting to handle edge cases like functions or circular references. + +## Architectural Decisions + +- The padding fix is targeted to the non-locale path only, preserving the existing locale formatting behavior which handles precision differently. +- The isNaN change is purely cosmetic — no test changes needed. +- The deep clone replacement uses a feature-detection approach (`structuredClone` if available, fallback otherwise) to maintain backward compatibility with older Node.js versions while using the modern API when available. diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/.openspec.yaml b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/.openspec.yaml new file mode 100644 index 0000000..578bd54 --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-26 diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/design.md b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/design.md new file mode 100644 index 0000000..e57aa1f --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/design.md @@ -0,0 +1,48 @@ +## Context + +The filesize.js library converts byte counts to human-readable strings. An audit (issue #296) identified three findings in the source code: a medium-severity bug in the padding logic when custom separators are used, a low-severity clarity issue with isNaN, and a low-severity fragility issue with JSON-based deep cloning in the partial function. + +Current state: The codebase uses a modular structure with `src/filesize.js` (main entry), `src/helpers.js` (utility functions), and `src/constants.js` (shared constants). The project targets Node.js >= 10.8.0 and uses ESM modules. + +## Goals / Non-Goals + +**Goals:** +- Fix the padding truncation bug in `applyNumberFormatting` when separator and pad are both set +- Clarify the isNaN check in the filesize function +- Replace JSON-based deep clone with a safer approach in the partial function + +**Non-Goals:** +- Refactoring other helper functions +- Adding new features or options +- Changing the public API or breaking existing behavior (except correcting the padding bug) +- Adding new dependencies + +## Decisions + +### Decision 1: Truncate decimals before padding in helpers.js +**Choice:** Truncate the decimal portion to `round` digits before calling `padEnd`, in the non-locale path of `applyNumberFormatting`. +**Rationale:** The locale formatting path already handles precision via `minimumFractionDigits`/`maximumFractionDigits`. The non-locale path (separator + pad) needs explicit truncation because `padEnd` only appends zeros — it never removes excess digits. +**Alternatives considered:** +- Round the numeric value before string conversion — but this could introduce floating point artifacts and doesn't work cleanly with custom separators. +- Use `toFixed(round)` — but this doesn't work with custom separators since it always uses `.` as decimal point. + +### Decision 2: Use structuredClone with fallback for deep cloning +**Choice:** Use `structuredClone` when available (Node.js 17+), fall back to a simple recursive clone for plain objects/arrays on older versions. +**Rationale:** `structuredClone` is the modern, standards-based approach. The fallback handles the current use case (plain objects/arrays from user options) without adding a dependency. Since filesize.js targets Node.js >= 10.8.0, the fallback is necessary. +**Alternatives considered:** +- Add a deep clone library (e.g., lodash.clonedeep) — adds dependency, against the project's zero-dependency philosophy. +- Always use JSON.parse(JSON.stringify()) — keeps it simple but retains the fragility. + +### Decision 3: isNaN clarity change is standalone +**Choice:** Simple one-line replacement of `isNaN(arg)` with `isNaN(num)`. +**Rationale:** No behavioral change, no test changes needed. Pure clarity improvement. + +## Risks / Trade-offs + +- [Padding fix changes output for existing code relying on buggy behavior] → Mitigation: The buggy behavior was incorrect; correcting it is the right thing. Add test coverage to document expected behavior. +- [structuredClone fallback may not handle all edge cases] → Mitigation: The fallback only needs to handle plain objects/arrays (current use case). Document this limitation. +- [Node.js version compatibility] → Mitigation: structuredClone is available in Node.js 17+. The fallback ensures compatibility with older versions. + +## Open Questions + +None. All three findings have clear, actionable fixes. diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/proposal.md b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/proposal.md new file mode 100644 index 0000000..dc43f4d --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/proposal.md @@ -0,0 +1,24 @@ +## Why + +An audit of the filesize.js source code (issue #296) identified three findings: a medium-severity bug where padding with a custom separator fails to truncate excess decimal places, a low-severity clarity issue where `isNaN(arg)` should reference the already-coerced `num` variable, and a low-severity fragility issue where `JSON.parse(JSON.stringify())` is used for deep cloning in the `partial` function. These fixes improve correctness, code clarity, and long-term maintainability. + +## What Changes + +- **Fix padding truncation in helpers.js**: When both `separator` and `pad` options are set, truncate excess decimal digits to match `round` before applying zero-padding. This fixes the bug where `filesize(1234.567, {separator: ",", pad: true, round: 2})` returned `"1,234.567"` instead of `"1,234.57"`. +- **Clarify isNaN check in filesize.js**: Replace `isNaN(arg)` with `isNaN(num)` after `num = Number(arg)` for clarity. No behavioral change. +- **Replace JSON deep clone in partial function**: Replace `JSON.parse(JSON.stringify())` with `structuredClone` (with fallback for Node.js < 17) for safer deep cloning of `localeOptions`, `symbols`, and `fullforms`. + +## Capabilities + +### New Capabilities +- `number-formatting`: Correct truncation of decimal places when using custom separators with padding + +### Modified Capabilities +- `partial-application`: Safer deep cloning of options objects in the `partial` function + +## Impact + +- **Affected code**: `src/helpers.js` (applyNumberFormatting), `src/filesize.js` (filesize, partial) +- **API**: No breaking changes. The padding fix corrects incorrect behavior — existing code relying on the buggy behavior is unlikely. +- **Dependencies**: No new dependencies. Uses `structuredClone` with fallback for older Node.js versions. +- **Tests**: Existing tests for padding and separator behavior must continue to pass. New tests needed for the padding truncation fix. diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/number-formatting/spec.md b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/number-formatting/spec.md new file mode 100644 index 0000000..88c798d --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/number-formatting/spec.md @@ -0,0 +1,24 @@ +## ADDED Requirements + +### Requirement: Padding with separator must truncate excess decimals +When both `separator` and `pad` options are provided to filesize, the output MUST truncate excess decimal digits to match the `round` parameter before applying zero-padding. + +#### Scenario: Truncate excess decimals with separator and pad +- **WHEN** filesize is called with `1234.567`, `{separator: ",", pad: true, round: 2}` +- **THEN** the result is `"1,234.57"` (not `"1,234.567"`) + +#### Scenario: Pad with separator when decimals are sufficient +- **WHEN** filesize is called with `1234.5`, `{separator: ",", pad: true, round: 2}` +- **THEN** the result is `"1,234.50"` (truncated to 2 decimals, then padded) + +#### Scenario: Truncate without pad but with separator +- **WHEN** filesize is called with `1234.567`, `{separator: ",", pad: false, round: 2}` +- **THEN** the result is `"1,234.57"` (truncated to round digits, no padding) + +#### Scenario: Round of zero truncates all decimals with separator +- **WHEN** filesize is called with `1234.567`, `{separator: ",", pad: true, round: 0}` +- **THEN** the result is `"1,235"` (rounded and no decimal places) + +#### Scenario: Locale formatting is unaffected +- **WHEN** filesize is called with locale option set (locale: true or locale string) +- **THEN** the padding and separator logic does not apply; locale formatting handles precision independently diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/partial-application/spec.md b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/partial-application/spec.md new file mode 100644 index 0000000..c6b5dea --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/specs/partial-application/spec.md @@ -0,0 +1,23 @@ +## ADDED Requirements + +### Requirement: Partial function must use safe deep cloning +The `partial` function MUST use a safe deep cloning mechanism that correctly handles plain objects and arrays without silently dropping values. + +#### Scenario: Deep clone localeOptions +- **WHEN** partial is called with `{localeOptions: {currency: "USD"}}` +- **THEN** modifying the returned function's internal state does not affect the original localeOptions object + +#### Scenario: Deep clone symbols +- **WHEN** partial is called with `{symbols: {"B": "bytes"}}` +- **THEN** modifying the returned function's internal state does not affect the original symbols object + +#### Scenario: Deep clone fullforms +- **WHEN** partial is called with `{fullforms: ["bytes", "kilobytes"]}` +- **THEN** modifying the returned function's internal state does not affect the original fullforms array + +### Requirement: Partial function must support Node.js < 17 +The `partial` function MUST work on Node.js versions below 17 where `structuredClone` is not available. + +#### Scenario: Fallback clone on older Node.js +- **WHEN** partial is called on Node.js < 17 with plain object options +- **THEN** the options are correctly deep cloned using the fallback mechanism diff --git a/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/tasks.md b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/tasks.md new file mode 100644 index 0000000..34bc0e6 --- /dev/null +++ b/openspec/changes/fix-audit-findings-helpers-padding-clarify-isnan/tasks.md @@ -0,0 +1,26 @@ +## 1. Fix padding truncation in helpers.js + +- [ ] 1.1 Read applyNumberFormatting in src/helpers.js and identify the padding logic block +- [ ] 1.2 Add truncation of decimal portion to round digits before padEnd call, in the non-locale path +- [ ] 1.3 Write unit tests for padding with separator: truncate excess decimals, pad sufficient decimals, round 0 behavior +- [ ] 1.4 Verify existing padding and separator tests still pass + +## 2. Clarify isNaN check in filesize.js + +- [ ] 2.1 Replace isNaN(arg) with isNaN(num) in the filesize function +- [ ] 2.2 Add brief comment explaining why num is used instead of arg +- [ ] 2.3 Verify no test changes needed (no behavioral change) + +## 3. Replace JSON deep clone in partial function + +- [ ] 3.1 Implement a safe deep clone helper: use structuredClone if available, fallback to recursive clone for plain objects/arrays +- [ ] 3.2 Replace JSON.parse(JSON.stringify()) calls in partial function with the safe clone helper for localeOptions, symbols, and fullforms +- [ ] 3.3 Write unit tests for partial deep cloning: verify cloned objects are independent of originals +- [ ] 3.4 Verify existing partial tests still pass + +## 4. Verification + +- [ ] 4.1 Run full test suite (npm test) +- [ ] 4.2 Run linting (npm run lint) +- [ ] 4.3 Run coverage check (npm run coverage) +- [ ] 4.4 Verify all three audit findings are addressed From 792d2720eeac25d0c6a00e9d4113e02557dcaab0 Mon Sep 17 00:00:00 2001 From: Jason Mulligan Date: Thu, 25 Jun 2026 21:42:49 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix:=20resolve=20audit=20findings=20?= =?UTF-8?q?=E2=80=94=20padding=20truncation,=20isNaN=20clarity,=20structur?= =?UTF-8?q?edClone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .opencode/commands/opsx-apply.md | 149 +++++++++ .opencode/commands/opsx-archive.md | 154 ++++++++++ .opencode/commands/opsx-explore.md | 170 +++++++++++ .opencode/commands/opsx-propose.md | 103 +++++++ .../skills/openspec-apply-change/SKILL.md | 156 ++++++++++ .../skills/openspec-archive-change/SKILL.md | 114 +++++++ .opencode/skills/openspec-explore/SKILL.md | 288 ++++++++++++++++++ .opencode/skills/openspec-propose/SKILL.md | 110 +++++++ openspec/config.yaml | 20 ++ src/filesize.js | 17 +- src/helpers.js | 6 + tests/unit/filesize-helpers.test.js | 35 +++ tests/unit/filesize.test.js | 42 +++ 13 files changed, 1360 insertions(+), 4 deletions(-) create mode 100644 .opencode/commands/opsx-apply.md create mode 100644 .opencode/commands/opsx-archive.md create mode 100644 .opencode/commands/opsx-explore.md create mode 100644 .opencode/commands/opsx-propose.md create mode 100644 .opencode/skills/openspec-apply-change/SKILL.md create mode 100644 .opencode/skills/openspec-archive-change/SKILL.md create mode 100644 .opencode/skills/openspec-explore/SKILL.md create mode 100644 .opencode/skills/openspec-propose/SKILL.md create mode 100644 openspec/config.yaml diff --git a/.opencode/commands/opsx-apply.md b/.opencode/commands/opsx-apply.md new file mode 100644 index 0000000..16d2ef3 --- /dev/null +++ b/.opencode/commands/opsx-apply.md @@ -0,0 +1,149 @@ +--- +description: Implement tasks from an OpenSpec change (Experimental) +--- + +Implement tasks from an OpenSpec change. + +**Input**: Optionally specify a change name (e.g., `/opsx-apply add-auth`). If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. + +**Steps** + +1. **Select the change** + + If a name is provided, use it. Otherwise: + - Infer from conversation context if the user mentioned a change + - Auto-select if only one active change exists + - If ambiguous, run `openspec list --json` to get available changes and use the **AskUserQuestion tool** to let the user select + + Always announce: "Using change: " and how to override (e.g., `/opsx-apply `). + +2. **Check status to understand the schema** + ```bash + openspec status --change "" --json + ``` + Parse the JSON to understand: + - `schemaName`: The workflow being used (e.g., "spec-driven") + - Which artifact contains the tasks (typically "tasks" for spec-driven, check status for others) + +3. **Get apply instructions** + + ```bash + openspec instructions apply --change "" --json + ``` + + This returns: + - `contextFiles`: artifact ID -> array of concrete file paths (varies by schema) + - Progress (total, complete, remaining) + - Task list with status + - Dynamic instruction based on current state + + **Handle states:** + - If `state: "blocked"` (missing artifacts): show message, suggest using `/opsx-continue` + - If `state: "all_done"`: congratulate, suggest archive + - Otherwise: proceed to implementation + +4. **Read context files** + + Read every file path listed under `contextFiles` from the apply instructions output. + The files depend on the schema being used: + - **spec-driven**: proposal, specs, design, tasks + - Other schemas: follow the contextFiles from CLI output + +5. **Show current progress** + + Display: + - Schema being used + - Progress: "N/M tasks complete" + - Remaining tasks overview + - Dynamic instruction from CLI + +6. **Implement tasks (loop until done or blocked)** + + For each pending task: + - Show which task is being worked on + - Make the code changes required + - Keep changes minimal and focused + - Mark task complete in the tasks file: `- [ ]` → `- [x]` + - Continue to next task + + **Pause if:** + - Task is unclear → ask for clarification + - Implementation reveals a design issue → suggest updating artifacts + - Error or blocker encountered → report and wait for guidance + - User interrupts + +7. **On completion or pause, show status** + + Display: + - Tasks completed this session + - Overall progress: "N/M tasks complete" + - If all done: suggest archive + - If paused: explain why and wait for guidance + +**Output During Implementation** + +``` +## Implementing: (schema: ) + +Working on task 3/7: +[...implementation happening...] +✓ Task complete + +Working on task 4/7: +[...implementation happening...] +✓ Task complete +``` + +**Output On Completion** + +``` +## Implementation Complete + +**Change:** +**Schema:** +**Progress:** 7/7 tasks complete ✓ + +### Completed This Session +- [x] Task 1 +- [x] Task 2 +... + +All tasks complete! You can archive this change with `/opsx-archive`. +``` + +**Output On Pause (Issue Encountered)** + +``` +## Implementation Paused + +**Change:** +**Schema:** +**Progress:** 4/7 tasks complete + +### Issue Encountered + + +**Options:** +1.