Conversation
…ired legacy-combo entries - check:side-effects: 'time' now means Date.now() or new Date() with no arguments. new Date(value) is a pure conversion, not a side effect. Every allowlisted 'time' entry was a conversion; none of the ten legacy-combo modules reads the clock. - check:side-effects:diff: an entry may drop expiresBy only when it also leaves the legacy-combo tier; expiry against HEAD date, growth rules and the legacy-reduction rule are unchanged. - Allowlist: ten legacy-combo entries -> persistence-only; three conversion-only boundary-time entries removed (39 -> 36 legacy, 0 combos). - Regression test with fixtures: clock reads flagged, conversions not, and expiry evaluated against commits dated after 2026-06-01 in a temp repo. - docs/guardrails.md Guardrail 10; config snapshot regenerated.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const ROOT = process.cwd(); | ||
| const ROOT = process.env['CHERRY_SIDE_EFFECTS_ROOT'] ?? process.cwd(); |
There was a problem hiding this comment.
If CHERRY_SIDE_EFFECTS_ROOT is inherited from a previous local task or runner configuration, normal guardrail invocations use it without validation. This redirects both the lib scan and allowlist lookup; the diff checker also reads Git history from that root. As a result, npm run check can report success without examining the current checkout. Please scope or validate this fixture override so normal guardrail entry points cannot silently use it.
| const unlisted = path.join(repoRoot, 'tests', 'fixtures', 'guardrails', 'repo', 'side-effects-unlisted'); | ||
| fs.rmSync(unlisted, { recursive: true, force: true }); | ||
| fs.mkdirSync(path.join(unlisted, 'lib'), { recursive: true }); | ||
| fs.mkdirSync(path.join(unlisted, 'scripts'), { recursive: true }); | ||
| fs.copyFileSync(path.join(fixtureRoot, 'lib', 'clock-read.ts'), path.join(unlisted, 'lib', 'clock-read.ts')); | ||
| fs.writeFileSync(path.join(unlisted, 'scripts', 'side-effects.allowlist.json'), '{}\n'); | ||
| try { | ||
| const missing = runGuardrail('scripts/check-side-effects.mts', unlisted); | ||
| assert.notEqual(missing.status, 0, 'expected an unlisted clock read to fail'); | ||
| assert.equal(missing.output.includes('lib/clock-read.ts: time'), true, missing.output); | ||
| } finally { | ||
| fs.rmSync(unlisted, { recursive: true, force: true }); |
There was a problem hiding this comment.
This test creates and recursively deletes a fixed directory inside the checkout. Two overlapping test runs can remove or partly rewrite each other's fixture, causing intermittent copy or read failures and unreliable guardrail results. Setup failures before the try block can also leave an untracked directory behind. Use a unique temporary directory, as the Git fixture later in this test already does.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63362f9ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
|
|
||
| const ROOT = process.cwd(); | ||
| const ROOT = process.env['CHERRY_SIDE_EFFECTS_ROOT'] ?? process.cwd(); |
There was a problem hiding this comment.
Reading CHERRY_SIDE_EFFECTS_ROOT in the production guardrail exposes a complete bypass: setting it to a directory containing an empty lib/ and {} allowlist makes npm run check:side-effects report success without inspecting this repository. The same override also redirects the diff guardrail. Keep registered execution rooted at process.cwd() and expose fixture-root injection through a test-only helper or otherwise reject this variable in normal runs.
AGENTS.md reference: AGENTS.md:L66-L70
Useful? React with 👍 / 👎.
| - `check:side-effects` scans `lib/**` (excluding `lib/adapters/runtime/**`) for effects: Prisma access, clock reads (`Date.now()` or `new Date()` with no arguments; `new Date(value)` is a pure conversion, not an effect), `process.env`, fs, fetch, crypto, randomness, Next request helpers. Every hit must be declared in `scripts/side-effects.allowlist.json` with `source: legacy`. | ||
| - Tiers: `persistence-only` (Prisma, no clock), `boundary-time` (clock or other effects, no Prisma), `legacy-combo` (Prisma and a clock read in the same module). | ||
| - `legacy-combo` entries require `expiresBy: YYYY-MM-DD`. `check:side-effects:diff` fails when an entry is expired, when entries or effects grow, and when an allowlist change does not reduce the number of legacy entries. | ||
| - The expiry epoch is the HEAD commit date (guardrails may not read the wall clock), so run `npm run check:side-effects:diff` after committing, not only on the working tree. | ||
| - Guardrail checks: `check:side-effects`, `check:side-effects:diff`. Tests: `tests/node/guardrails/side-effects-time-detection.test.ts`. |
There was a problem hiding this comment.
These additions name scripts/side-effects.allowlist.json and tests/node/guardrails/side-effects-time-detection.test.ts, although docs/doctrine.md §5.1 requires documentation to refer only to guardrail names rather than implementation paths. Describe the policy through check:side-effects and check:side-effects:diff without embedding paths that can drift.
AGENTS.md reference: AGENTS.md:L6-L7
Useful? React with 👍 / 👎.
| @@ -0,0 +1,108 @@ | |||
| import * as assert from 'node:assert/strict'; | |||
There was a problem hiding this comment.
This commit combines scripts/**, tests/**, and docs/**, contrary to the active docs/doctrine.md §8 requirement that guardrail, test, and documentation concerns be isolated into their respective commit categories. Split these companion changes into separately categorized commits before merging.
AGENTS.md reference: AGENTS.md:L6-L7
Useful? React with 👍 / 👎.
| cwd: repoRoot, | ||
| encoding: 'utf8', | ||
| env: { ...process.env, CHERRY_SIDE_EFFECTS_ROOT: root }, | ||
| }); |
There was a problem hiding this comment.
The spawned guardrail inherits SOURCE_DATE_EPOCH and CHERRY_GUARDRAIL_EPOCH, but resolveEpochMs() gives those variables precedence over the temporary repository's HEAD date. In reproducible-build environments that set SOURCE_DATE_EPOCH, the nominal 2026-05-31 pre-expiry assertion can therefore fail; clear both overrides from the child environment, or explicitly set the intended epoch for each case.
Useful? React with 👍 / 👎.
| // A clock read is `Date.now()` or `new Date()` with no arguments. `new Date(value)` is a pure | ||
| // conversion of a supplied timestamp and is not a side effect. | ||
| time: /\bnew Date\s*\(\s*\)|\bDate\.now\s*\(/, |
There was a problem hiding this comment.
The narrowed expression misses real zero-argument clock reads written with a spread, such as new Date(...[]) or a typed const args: [] = []; new Date(...args). The previous expression detected these, but they now pass without a time effect even though they read the wall clock at runtime; distinguish conversions only when an actual argument is guaranteed rather than treating every syntactically nonempty argument list as pure.
Useful? React with 👍 / 👎.
| function runGuardrail(script: string, root: string): { status: number | null; output: string } { | ||
| const result = spawnSync('npm', ['run', 'ts:esm', '--', script], { | ||
| cwd: repoRoot, | ||
| encoding: 'utf8', | ||
| env: { ...process.env, CHERRY_SIDE_EFFECTS_ROOT: root }, | ||
| }); |
There was a problem hiding this comment.
The test invokes each guardrail by passing its script path directly to ts:esm, bypassing the registered check:<name> command and scripts/guardrails/run.mts. Consequently the regression test can stay green if registry routing or runner behavior breaks, and it violates the repository rule that guardrails are unaddressable by path; invoke npm run check:side-effects and npm run check:side-effects:diff instead.
AGENTS.md reference: AGENTS.md:L66-L70
Useful? React with 👍 / 👎.
| const unlisted = path.join(repoRoot, 'tests', 'fixtures', 'guardrails', 'repo', 'side-effects-unlisted'); | ||
| fs.rmSync(unlisted, { recursive: true, force: true }); | ||
| fs.mkdirSync(path.join(unlisted, 'lib'), { recursive: true }); | ||
| fs.mkdirSync(path.join(unlisted, 'scripts'), { recursive: true }); | ||
| fs.copyFileSync(path.join(fixtureRoot, 'lib', 'clock-read.ts'), path.join(unlisted, 'lib', 'clock-read.ts')); | ||
| fs.writeFileSync(path.join(unlisted, 'scripts', 'side-effects.allowlist.json'), '{}\n'); |
There was a problem hiding this comment.
This scratch fixture is created at a fixed path inside the source tree after first recursively deleting that path. Concurrent test runs can delete or overwrite one another's fixture, and an interruption before the finally block leaves the worktree dirty; allocate it with mkdtempSync under CHERRY_TMP_ROOT as the later temporary Git repository already does.
Useful? React with 👍 / 👎.
| export function nowMs(): number { | ||
| return Date.now(); | ||
| } | ||
| export function today(): Date { | ||
| return new Date(); |
There was a problem hiding this comment.
Both Date.now() and new Date() occur in the same allowlisted file, while the assertion only checks that the file has a single time effect. If either half of the new alternation is accidentally removed later, the other call still produces the identical successful result, so this regression test does not actually protect both promised syntaxes; place them in separate fixtures or run independent negative cases.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Summary
Resolves the expired
legacy-comboside-effects allowlist that failsnpm run checkon every commit dated after 2026-06-01, by fixing what the guardrail measures rather than by extending the deadline.What the rule protects.
check:side-effectsrequires every module underlib/(outsidelib/adapters/runtime) that touches Prisma, the clock, env, fs, fetch, crypto, or randomness to be declared inscripts/side-effects.allowlist.json. Modules that mix Prisma access with a current-time read are tierlegacy-comboand must carry anexpiresBydate;check:side-effects:difffails on expiry, growth, or any allowlist change that does not reduce legacy entries. The purpose is that persistence-touching legacy modules must not also read wall-clock time, so financial behaviour stays replayable with injectednow.What was wrong with the ten files. Nothing in behaviour. Each of the ten (
lib/autopilot/{engineDecisionId,service}.ts,lib/bank/ingest.ts,lib/buckets/regimes.ts,lib/daily-state/runDailyForUser.ts,lib/dashboard.ts,lib/demo-seeder.ts,lib/income/monthly.ts,lib/unified-activity.ts,lib/vine/run-recommendation.ts) already receivesnowfrom its caller and only constructs Date objects from supplied values (new Date(ms),new Date(Date.UTC(...)),new Date(options.now.getTime() + ...)). None callsDate.now()ornew Date(). The detector's pattern\bnew Date\s*\(could not distinguish a conversion from a clock read, so the files were classified as clock readers in 2025-12, given a March 2026 deadline, extended to June 2026, and expired. The same imprecision put four other modules (lib/buckets/periods.ts,lib/evaluator/regime-buckets.ts,lib/schemas/bank-ingest.ts,lib/bank/csv-dev-provider.ts) in theboundary-timetier for conversions only.Changes.
check:side-effects:timenow meansDate.now()ornew Date()with no arguments. Real clock reads are still caught (regression test). A root override env var enables fixture testing.check:side-effects:diff: an entry may dropexpiresByonly when it also leaves thelegacy-combotier (the tier is verified against detected effects bycheck:side-effects). Previously the rule fired even after the clock read was gone, which made remediation impossible without deleting the whole entry. Expiry against the HEAD commit date, growth rules, and the "must reduce legacy entries" rule are unchanged. Root override for fixture testing.persistence-only(no expiry, same as the other 20 Prisma-using service modules); three conversion-only entries are deleted;csv-dev-providerkeepsfs. Legacy entries 39 → 36, legacy-combo 10 → 0.tests/node/guardrails/side-effects-time-detection.test.ts: clock reads flagged, conversions not, unlisted clock read fails; and in a throwaway git repo, a commit dated 2026-07-01 with an entry expiring 2026-06-01 fails while remediation on a 2026-09 commit passes (and a tier change alone does not satisfy the reduction rule).docs/guardrails.mdGuardrail 10 documents the tiers, the precisetimedefinition, and that the epoch is the HEAD commit date (run the diff check after committing).docs/config-snapshot.mdblock regenerated.Why not the alternative. Routing every
new Date(value)through a helper would only relocate pure conversions to satisfy a regex; moving all Prisma access in these modules behind runtime adapters is the documented target for the whole service layer (66lib/prismaimporters) and is out of scope here.No runtime code under
lib/,app/, orcomponents/changed. No engine, accounting, session, or API semantics changed.Testing
Node 24.15.0,
npm ci,CHERRY_TMP_ROOTset,CHERRY_VINE_SIGNATURE_MODE=enforce.npm run checknpm testnpm run buildnpm run check:side-effects:diffafter committing (HEAD dated 2026-09-22)tests/node/guardrails/side-effects-time-detection.test.tsRisk
Dateconstructions; everyDate.now()/new Date()inlib/remains an error unless allowlisted, and the allowlist cannot grow.check:side-effects:diffcompares againstHEAD~1, so on a squash merge tomainit compares the previousmainallowlist (10 combos) with this one (0): reduction satisfied.Engine Impact
The PR appears safe to merge, with two non-blocking hardening improvements recommended for guardrail root isolation and concurrent test reliability.
Findings
Summary
This PR narrows the side-effect guardrail’s clock detection to actual zero-argument Date reads, reclassifies conversion-only allowlist entries, and adds regression coverage for detection and expiry remediation.
Date.now()and zero-argumentnew Date()remain classified as clock reads.legacy-combo.legacy-combo.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Guardrail invocation] --> B{CHERRY_SIDE_EFFECTS_ROOT set?} B -->|No| C[Use current checkout] B -->|Yes| D[Use overridden root] C --> E[Scan lib source] D --> E E --> F[Detect Prisma and clock effects] F --> G[Validate allowlist effects and tier] C --> H[Read current and previous allowlists] D --> H H --> I[Check expiry, growth, and legacy reduction]Reviews (1) · Last reviewed commit: "guardrails: detect real clock reads in s..."