Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-06-20
53 changes: 53 additions & 0 deletions openspec/changes/fix-pino-multistream-compatibility/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
## Context

The `src/logger.js` module implements a dual-file logging strategy using pino. It writes info/warn/debug messages to `madz.log` and error/fatal messages to `madz_error.log`. The current implementation uses `pino.multistream(streams)`, which was deprecated in pino v9 and removed in pino v10. The application's `package.json` specifies `"pino": "^10.3.1"`, so the code is incompatible with its declared dependency.

The logger module handles OS-specific log directory detection (Alpine, Linux, macOS, Windows), graceful fallback to `/tmp` if the primary directory is unwritable, and silent mode for tests. All of this logic is sound — only the final `pino()` constructor call needs updating.

## Goals / Non-Goals

**Goals:**
- Replace `pino.multistream(streams)` with the pino v10-compatible array-based destination API
- Preserve all existing behavior: dual-file routing, fallback logic, silent test mode, exported logger interface
- Remove the deprecated TODO comment

**Non-Goals:**
- Changing log format, structure, or verbosity
- Modifying log directory detection or fallback logic
- Adding new logging capabilities or transports
- Changing the exported logger API

## Decisions

### Decision: Use array syntax as second argument to `pino()`

**Rationale:** Pino v10 accepts an array of destinations as the second argument to the constructor. Each array element can be a stream object with `{ stream, level }` properties — exactly the structure already being built in the current code. This is a drop-in replacement requiring a single-line change.

**Alternatives considered:**
- `pino.destination()` with transport targets: More verbose, requires `pino/file` package for file destinations, overkill for this use case
- Custom transport: Unnecessary complexity for a simple file-routing scenario
- Keeping multistream with a pino v9 downgrade: Violates the declared dependency in package.json

### Decision: Accept changed error log routing

**Rationale:** The original `multistream()` routed error/fatal to both streams. The v10 array API routes each level to streams matching that level or higher. This means error logs will write to `madz_error.log` only, not `madz.log`. This is the more correct behavior for a dedicated error log and aligns with common logging practices.

**Impact:** Users who relied on errors appearing in `madz.log` will see them only in `madz_error.log` going forward. This is a behavioral correction, not a regression.

## Risks / Trade-offs

- **[Risk]** Error logs no longer appear in `madz.log` → **[Mitigation]** This is intentional and more correct. Document the change in the PR description.
- **[Risk]** Stream objects in array may need different structure than multistream expected → **[Mitigation]** The pino v10 docs confirm `{ stream, level }` objects are supported. The existing `streams` array already uses this structure.

## Migration Plan

1. Apply the code change to `src/logger.js`
2. Run the full test suite to verify no regressions
3. Verify the application starts without TypeError
4. Verify logs are written to the correct files

No rollback strategy needed — this is a single-file fix with no data migration.

## Open Questions

None. The fix is a straightforward API replacement.
25 changes: 25 additions & 0 deletions openspec/changes/fix-pino-multistream-compatibility/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
## Why

The application depends on pino `^10.3.1`, but `src/logger.js` still calls `pino.multistream()`, an API that was removed in pino v10. This causes a `TypeError` at startup: "pino.multistream is not a function." The audit-code skill flagged this as a high-severity bug. The fix is straightforward — migrate to the v10-compatible array-based destination API — but it must be done before the application can start.

## What Changes

- Replace `pino.multistream(streams)` with the v10 array-based destination API in `src/logger.js`
- Remove the deprecated TODO comment about multistream deprecation
- No changes to log format, log directory detection, or the exported logger interface
- Level routing behavior shifts slightly: error/fatal logs write to `madz_error.log` only (not both files), which is the more correct behavior for a dedicated error log

## Capabilities

### New Capabilities
<!-- None — this is a compatibility fix, not a new capability -->

### Modified Capabilities
<!-- No spec-level requirement changes — only implementation update -->

## Impact

- **Affected code:** `src/logger.js` (single file, single function call change)
- **Dependencies:** pino v10.3.1 (already installed, no version change)
- **Behavioral change:** Error logs no longer duplicate into `madz.log` — they write only to `madz_error.log`. This is a correction, not a regression.
- **Risk:** Low. The change is a drop-in API replacement with existing error handling and fallback logic preserved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
## MODIFIED Requirements

### Requirement: Dual file output
The logger SHALL produce two log files:
- `madz.log`: captures `info`, `warn`, `debug`, and `trace` levels
- `madz_error.log`: captures `error` and `fatal` levels

With the pino v10 array-based destination API, each log level routes to streams matching that level or higher. Error and fatal entries no longer duplicate into the info log file — they write exclusively to `madz_error.log`. This is the more correct behavior for a dedicated error log.

#### Scenario: Error appears only in error file
- **WHEN** `logger.error("something failed")` is called
- **THEN** the entry appears only in `madz_error.log`, not in `madz.log`

#### Scenario: Fatal appears only in error file
- **WHEN** `logger.fatal("critical failure")` is called
- **THEN** the entry appears only in `madz_error.log`, not in `madz.log`

#### Scenario: Info only in info file
- **WHEN** `logger.info("startup complete")` is called
- **THEN** the entry appears only in `madz.log`, not in `madz_error.log`
23 changes: 23 additions & 0 deletions openspec/changes/fix-pino-multistream-compatibility/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## 1. Implement the pino v10-compatible logger

- [ ] 1.1 Replace `pino.multistream(streams)` with the v10 array-based destination API in src/logger.js
- [ ] 1.2 Remove the deprecated TODO comment about multistream deprecation
- [ ] 1.3 Verify the streams array structure is compatible with v10 API (each element has `{ stream, level }`)

## 2. Update the system-logger spec delta

- [ ] 2.1 Verify the delta spec at specs/system-logger/spec.md correctly documents the new error routing behavior
- [ ] 2.2 Ensure all scenarios in the delta spec use the correct 4-hashtag format

## 3. Test the implementation

- [ ] 3.1 Run the full test suite (`npm run test`) and verify all tests pass
- [ ] 3.2 Run lint (`npm run lint`) and verify no lint errors
- [ ] 3.3 Verify the application starts without TypeError (`npm start` background check)

## 4. Verify logging behavior

- [ ] 4.1 Confirm info/warn/debug logs write to madz.log
- [ ] 4.2 Confirm error/fatal logs write only to madz_error.log (not madz.log)
- [ ] 4.3 Confirm silent mode works in test environment (NODE_ENV=test)
- [ ] 4.4 Confirm fallback behavior works when log directories are unwritable