You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
devenv is used to load test.env, which caused test to write their logs into a test.log file. However, I suspect that no-one uses this file (I know I never do).
It can be enabled on a per-need basis with LOG_ENABLED=true LOG_DESTINATION=test.log pnpm test.
Similarly, the test:log command only allows printing the last 50 lines of logs, which might be find in some use cases but is very non-universal. It's as easy to use pnpm exec pino-pretty.
Checklist
pnpm build --force && pnpm verify passes
Tests pass, and new behavior is covered by tests
A changeset is included for every package this touches
Written with the help of an LLM or coding agent? Say so here, and name the tooling.
Looks good — no blocking findings. This is a clean removal of an unused test-only mechanism.
What I checked:
No remaining dotenv imports. Grep confirms the only surviving dotenv in the tree is the transitive dotenv@8.6.0 pulled in by @changesets/changelog-github (pnpm-lock.yaml:11737) — unrelated to the ^17.4.2 dep being dropped. All jest.setup.ts / env.ts importers are deleted, and dev-env/src/bin.ts no longer imports the removed ./env.js. The env.js hits elsewhere in the grep are the unrelated config modules in common/pds/ozone.
Default logging behavior is silent, as intended.packages/common/src/logger.ts:3 reads LOG_ENABLED ?? '0', so with the LOG_ENABLED=true/LOG_DESTINATION=test.log env removed from bsky/vitest.config.ts and the deleted test.env files, test logging defaults to silent rather than spamming stdout. The escape hatch described in the PR body (LOG_ENABLED=true LOG_DESTINATION=test.log pnpm test) works because the logger reads process.env directly.
No dangling references to the removed pieces. No workflow under .github/, README, or Makefile references test.log / test:log / test.env. The one LOG_ENABLED=true in Makefile:27 is the dev-env REPL start command, which passes the var directly and is unaffected.
Changeset.identity, dev-env, ozone, pds are covered at patch, which fits — these are devDep/dep removals with no public-API impact.
One minor, non-blocking note:bsky and bsync are also touched (a test:log script removed from each, plus the env block in bsky/vitest.config.ts) but aren't in the changeset. CLAUDE.md's "every package touched needs a changeset" is strict, but since these edits are purely test scripts/config with no effect on the published artifact, omitting them is defensible — flagging only so it's a conscious choice, not an oversight.
Looks good — no blocking findings. Clean removal of a test-only mechanism that only ever toggled logging.
What I checked:
No dangling dotenv. No package.json in the workspace still lists dotenv (grep over packages/*, services/*, root came back empty), and no .ts/.cjs still imports it. The three jest.setup.ts files and dev-env/src/env.ts are deleted, and dev-env/src/bin.ts:1 no longer imports ./env.js. The only surviving test.log references are the new, intentional docs in SKILL.md.
Default logging stays silent. All three deleted test.env files (and bsky/vitest.config.ts's env block) only set LOG_ENABLED=true / LOG_DESTINATION=test.log. With them gone, tests default to silent (common/src/logger.ts reads LOG_ENABLED ?? '0'), and the escape hatch from the PR body works since the logger reads process.env directly. No test asserts on log output or the test.log file.
Lost capability is unused.dev-env/src/env.ts also supported process.env.ENV → loading ./.${ENV}.env. grep finds no remaining process.env.ENV consumer, so dropping it is safe. The LOG_ENABLED=true make run-dev-env path in Makefile:27 passes the var directly and is unaffected.
Agent files updated in the same PR. The new "Logs" section in .agents/skills/testing/SKILL.md documents the LOG_ENABLED/LOG_DESTINATION + pino-pretty workflow that replaces the removed test:log scripts — exactly what the review guidance asks for when a documented pattern is removed.
One non-blocking note (changeset scope):bsky and bsync are touched (each loses a test:log script; bsky also loses the vitest.config.ts env block) but aren't in .changeset/warm-sloths-bake.md, which lists only identity, dev-env, ozone, pds (the packages that actually had the dotenv devDep). CLAUDE.md's "every package touched needs a changeset" is strict, but since these are pure test-script/config edits with zero effect on the published artifact, omitting them is defensible — flagging only so it's a conscious call.
· msi/drop-dotenv-dep
Looks good — no blocking findings. Clean removal of a test-only dotenv mechanism whose sole effect was toggling pino logging to a test.log file.
What I checked:
No dangling dotenv. After the change, the only dotenv reference in the tree (outside pnpm-lock.yaml) is the changeset title itself. All four dotenv devDeps are gone from package.json, the three jest.setup.ts files + dev-env/src/env.ts are deleted, and dev-env/src/bin.ts:1 no longer imports ./env.js. pnpm-lock.yaml correctly drops dotenv@17.4.2 while leaving the unrelated transitive dotenv@8.6.0 (from @changesets/changelog-github) in place.
Default logging stays silent.packages/common/src/logger.ts:3 reads LOG_ENABLED ?? '0', so with the test.env files and bsky/vitest.config.ts's env block removed, tests default to silent rather than writing test.log. The escape hatch from the PR body (LOG_ENABLED=true LOG_DESTINATION=test.log pnpm test) works because the logger reads process.env directly at load — no dotenv bootstrap needed. No test asserts on log output or the test.log file.
Lost ENV-file capability is unused.dev-env/src/env.ts also supported process.env.ENV → loading ./.${ENV}.env; grep finds no remaining consumer of process.env.ENV, so dropping it is safe. Makefile:27's LOG_ENABLED=true make run-dev-env passes the var directly and is unaffected.
Agent files updated in the same PR. The new "Logs" section in .agents/skills/testing/SKILL.md documents the LOG_ENABLED/LOG_DESTINATION + pino-pretty workflow that replaces the removed test:log scripts — exactly what the review guidance asks for when a documented pattern is removed.
One non-blocking note (changeset scope):bsky and bsync are touched (each loses a test:log script; bsky also loses its vitest.config.ts env block) but aren't in .changeset/warm-sloths-bake.md, which lists only the four packages that carried the dotenv devDep (identity, dev-env, ozone, pds). CLAUDE.md's "every package touched needs a changeset" is strict, but these are pure test-script/config edits with zero effect on the published artifact, so omitting them is defensible — flagging only so it's a conscious call.
· msi/drop-dotenv-dep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
devenvis used to loadtest.env, which caused test to write their logs into atest.logfile. However, I suspect that no-one uses this file (I know I never do).It can be enabled on a per-need basis with
LOG_ENABLED=true LOG_DESTINATION=test.log pnpm test.Similarly, the
test:logcommand only allows printing the last 50 lines of logs, which might be find in some use cases but is very non-universal. It's as easy to usepnpm exec pino-pretty.Checklist
pnpm build --force && pnpm verifypasses