ai: distill the store migration lessons into the plugin skills#51
Merged
Conversation
Adds the Effect v4 SQL story to effect-v4-construct-map, which had no coverage of it at all. The standalone @effect/sql package is gone and the core now lives in effect/unstable/sql, and the sqlite driver peers on effect alone over Node's built-in node:sqlite. Four SQL behaviours are recorded with it. SqliteClient.layer carries no error channel, so construction failures are defects rather than typed errors. Calling sql with a plain string yields an identifier, which is how a table name is interpolated safely. Transactions commit only on a successful Exit, so a defect and an interrupt each roll back exactly as a typed failure does. The Migrator is forward-only, with no down migration, no rollback and no status. Corrects MakeOptions.disableChecks in the schema house style. Its docstring promises to skip validation, but it gates only the check phase. Type errors still throw and the structural re-parse still runs, so it is a semantic switch for trusted data and never a performance one. Records alongside it that constructing a recursive Schema.Class tree node by node re-validates each subtree and doubles in cost per level, reaching 2.7 seconds at depth 20. The faithful escape is an internal prototype-assign path, because the Data.Class constructor is exactly an object assign. Gives effect-v4-planning two carve-outs from the buy-in step. A delegated subagent has no user to wait on, so it emits the design summary and proceeds under its parent's instructions. A small pure-tier bugfix that changes no public API collapses that summary to one line naming the failing test. Neither carve-out waives the summary itself. Names the subtree drift in effect-v4-source-lookup. The catalog is a caret range, so pnpm floats forward while the vendored tree stays pinned, and the tree currently sits at beta.94 against an installed beta.97. The installed source is the tiebreak on existence and signature. Also states why a probe must be written inside the package rather than merely run from there, since Node resolves bare imports from the script's own path and not the working directory. Adds the positive control a behavioural probe needs, for when a dead harness and a passing result would print the same thing. Every claim was settled at the rung the improve skill requires: source for existence and signature, and a live probe against the installed beta for each semantic claim. Signed-off-by: C. Spencer Beggs <spencer@beggs.codes> Claude-Session: https://claude.ai/code/session_01EJct3ERQPubzXRgTLcQZq5
📝 WalkthroughWalkthroughThe pull request updates Effect v4 skill documentation with SQL migration mappings, planning workflow exceptions, schema validation and recursive construction guidance, and procedures for resolving source drift and running TypeScript probes. ChangesEffect v4 guidance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code ReviewCurrent Commit: 2fa9c83 SummaryDocumentation-only PR distilling the store-cycle migration lessons into five plugin skills. Verdict: LGTM — approving. Accurate, well-sourced, and internally consistent. What I checkedEvery substantive claim I could reach settled against
The semantic/perf claims ( Nits (non-blocking)
Validation
The prose carries its own evidence (line cites, probe controls, positive-control reasoning) — this is exactly the discipline the |
![spencerbeggs[bot]](https://avatars.githubusercontent.com/in/2527309?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@plugin/skills/effect-v4-construct-map/SKILL.md`:
- Around line 408-412: Clarify the `sql(name)` guidance in the SQL interpolation
section: state that `sql(table)` returns an `Identifier` whose name is quoted
and escaped by the compiler, not a bound value parameter. Explicitly distinguish
identifier escaping from parameterized value interpolation, while retaining the
warnings about `sql.unsafe` and `sql.literal`.
- Line 395: Update the `@effect/sql-sqlite-node` versus `node:sqlite` comparison
table to document the minimum supported Node.js version, note that Node
22.5.0–22.12.x requires the `--experimental-sqlite` flag, and state that older
runtimes cannot load `node:sqlite`.
In `@plugin/skills/effect-v4-schema/references/house-style.md`:
- Around line 74-93: Reword the performance discussion in the disableChecks
section to acknowledge that skipping .check(...) filters can reduce work for
check-heavy schemas. Clarify that the depth-20 recursive-build benchmark only
demonstrates that disableChecks is not a general-purpose performance switch,
rather than claiming it provides no speed benefit in all cases.
- Around line 113-129: Restrict the prototype-based construction described in
the internal node builder to data-only classes whose derived initialization is
empty; do not generalize it to arbitrary Schema.Class types. Explicitly document
that Object.create bypasses field initializers, private fields, and constructor
side effects, and add or retain a test verifying the recursive AST class has no
derived initialization and that bypass-built nodes match constructor-built
nodes.
In `@plugin/skills/effect-v4-source-lookup/SKILL.md`:
- Around line 47-58: Update the version-drift guidance in the skill
documentation to label beta.94 and beta.97 as the currently observed example
rather than fixed values. In the surrounding drift rule, direct users to the
provided diff command as the authoritative way to determine current
subtree-versus-lockfile differences.
- Around line 152-167: Keep probe creation and cleanup paths consistent in the
command example: after changing into packages/<pkg>, write the probe as probe.ts
or "$PWD/probe.ts", matching the existing rm -f "$PWD/probe.ts" cleanup command.
Alternatively, create packages/<pkg>/probe.ts before changing directories.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5db57ed3-da7e-49cf-82b4-e26149a05c47
⛔ Files ignored due to path filters (1)
.claude/design/effected/plugin.mdis excluded by!.claude/**
📒 Files selected for processing (5)
plugin/skills/effect-v4-construct-map/SKILL.mdplugin/skills/effect-v4-planning/SKILL.mdplugin/skills/effect-v4-schema/SKILL.mdplugin/skills/effect-v4-schema/references/house-style.mdplugin/skills/effect-v4-source-lookup/SKILL.md
| | --- | --- | | ||
| | `import { SqlClient } from "@effect/sql"` | `import { SqlClient } from "effect/unstable/sql"` | | ||
| | `@effect/sql` + a driver package + `@effect/platform` peer chain | the **driver alone**, peering on `effect` and nothing else | | ||
| | `@effect/sql-sqlite-node` over `better-sqlite3` | over Node's built-in **`node:sqlite`** (`DatabaseSync`) — no native module to compile | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding section in the target markdown file.
sed -n '360,430p' plugin/skills/effect-v4-construct-map/SKILL.md
# Find other mentions of node:sqlite / DatabaseSync / Node runtime requirements in the repo.
rg -n --hidden -S 'node:sqlite|DatabaseSync|experimental-sqlite|Node 22\.5\.0|Node 22' plugin/skills/effect-v4-construct-map/SKILL.md . || trueRepository: spencerbeggs/effected
Length of output: 8029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Node runtime documentation in the local repo if present, without executing repo code.
git ls-files | rg '(^|/)(README|CHANGELOG|SKILL)\.md$|node:sqlite|sqlite'
# Inspect the exact line neighborhood and file length.
wc -l plugin/skills/effect-v4-construct-map/SKILL.md
sed -n '385,405p' plugin/skills/effect-v4-construct-map/SKILL.mdRepository: spencerbeggs/effected
Length of output: 9008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use a small read-only probe to understand whether the review claim about module/runtime support is in the docs.
python3 - <<'PY'
import pathlib, re
p = pathlib.Path("plugin/skills/effect-v4-construct-map/SKILL.md")
text = p.read_text()
for needle in ["node:sqlite", "DatabaseSync", "--experimental-sqlite", "Node 22.5.0", "Node 22"]:
print(needle, text.count(needle))
PYRepository: spencerbeggs/effected
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact section around the cited line.
nl -ba plugin/skills/effect-v4-construct-map/SKILL.md | sed -n '388,402p'
# Search for any explicit Node runtime prerequisite in this skill file.
rg -n -S 'Node 22|node:sqlite|DatabaseSync|experimental-sqlite|runtime prerequisite|requires a compatible Node' plugin/skills/effect-v4-construct-map/SKILL.mdRepository: spencerbeggs/effected
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any docs that already state the Node version requirement for node:sqlite.
git ls-files '*.md' | while read -r f; do
rg -n -H -S 'node:sqlite|DatabaseSync|experimental-sqlite|Node 22\.5\.0|Node 22' "$f" || true
done | sed -n '1,200p'Repository: spencerbeggs/effected
Length of output: 2316
🌐 Web query:
Node.js sqlite DatabaseSync added in v22.5.0 experimental-sqlite required release notes
💡 Result:
The node:sqlite module, which includes the DatabaseSync class, was introduced in Node.js v22.5.0 [1][2]. Regarding the --experimental-sqlite flag: 1. Requirement: When the module was initially added in v22.5.0, it was an experimental feature that required the --experimental-sqlite command-line flag to be enabled [3][4][5]. Users had to explicitly pass this flag to Node.js when running their application to use the node:sqlite module [3][4][5]. 2. Status Change: This requirement remained in place for several versions. The --experimental-sqlite flag was eventually removed, and the module was made available without the flag starting in Node.js v22.13.0 (and v23.4.0) [6][7]. Even after the flag was removed, the node:sqlite module was still considered experimental [6][7]. In summary, if you are using Node.js v22.5.0, the --experimental-sqlite flag is required to use DatabaseSync [3][5]. If you are using a version newer than v22.13.0, the flag is no longer required [6][7].
Citations:
- 1: https://r2.nodejs.org/dist/v22.5.0/docs/api/sqlite.html
- 2: V22.5.0 proposal nodejs/node#53826
- 3: https://nodejs.org/docs/v22.12.0/api/sqlite.html
- 4: https://docs.redhat.com/en/documentation/red_hat_build_of_node.js/22/pdf/release_notes_for_node.js_22/Red_Hat_build_of_Node.js-22-Release_Notes_for_Node.js_22-en-US.pdf
- 5: https://nodejs.org/docs/v22.11.0/api/sqlite.html
- 6: https://nodejs.org/docs/latest-v22.x/api/sqlite.html
- 7: https://nodejs.org/docs/latest/api/sqlite.html
Document the Node.js floor for node:sqlite. Add the minimum supported Node version and note that Node 22.5.0–22.12.x needs --experimental-sqlite; older runtimes will fail to load it.
🤖 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 `@plugin/skills/effect-v4-construct-map/SKILL.md` at line 395, Update the
`@effect/sql-sqlite-node` versus `node:sqlite` comparison table to document the
minimum supported Node.js version, note that Node 22.5.0–22.12.x requires the
`--experimental-sqlite` flag, and state that older runtimes cannot load
`node:sqlite`.
Source: MCP tools
Comment on lines
+408
to
+412
| - **`sql(name)` interpolates an identifier.** The template form | ||
| ``sql`SELECT …` `` builds a `Statement`; calling the same `sql` with a plain string | ||
| returns an `Identifier` (`Statement.ts` `Constructor`, `(value: string): Identifier`), | ||
| so `` sql`SELECT * FROM ${sql(table)}` `` parameterizes the *table name* safely. | ||
| `sql.unsafe` and `sql.literal` are the un-escaped escape hatches. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant markdown section
FILE="plugin/skills/effect-v4-construct-map/SKILL.md"
sed -n '390,430p' "$FILE"
echo
echo "---- searches ----"
rg -n "sql\\(|Identifier|unsafe|literal|Statement.ts|Constructor" -S .Repository: spencerbeggs/effected
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the SQL helper and identifier implementation/docs with minimal output.
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'\bIdentifier\b|\bsql\s*\(|sql\.unsafe|sql\.literal|Statement' \
packages repos/effect-smol plugin \
| head -n 200Repository: spencerbeggs/effected
Length of output: 27235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="repos/effect-smol/packages/effect/src/unstable/sql/Statement.ts"
# Show the Identifier type, sql overloads, and render logic around Identifier handling.
sed -n '160,220p' "$FILE"
echo "----"
sed -n '420,470p' "$FILE"
echo "----"
sed -n '830,880p' "$FILE"Repository: spencerbeggs/effected
Length of output: 4241
Clarify sql(table) as identifier escaping, not parameter binding. sql(table) returns an Identifier, and the compiler quotes/escapes it; only value interpolation is parameterized.
🤖 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 `@plugin/skills/effect-v4-construct-map/SKILL.md` around lines 408 - 412,
Clarify the `sql(name)` guidance in the SQL interpolation section: state that
`sql(table)` returns an `Identifier` whose name is quoted and escaped by the
compiler, not a bound value parameter. Explicitly distinguish identifier
escaping from parameterized value interpolation, while retaining the warnings
about `sql.unsafe` and `sql.literal`.
Comment on lines
+74
to
+93
| ### `disableChecks` skips checks, not validation — and buys no speed | ||
|
|
||
| `MakeOptions.disableChecks` reads like an escape hatch from validation. Its own | ||
| docstring says "skip validation when you trust the data" (`Schema.ts:107`) and | ||
| "skips constructor validation" (`Schema.ts:12754`). Both are misleading, and the | ||
| vendored cluster code leans on it as the trusted-construction idiom | ||
| (`unstable/cluster/EntityAddress.ts:93`, `RunnerAddress.ts:112`, `Runner.ts:129`), | ||
| so it looks blessed. What it actually does — probed against `effect@4.0.0-beta.97`: | ||
|
|
||
| | Passing `{ disableChecks: true }` | Effect | | ||
| | --- | --- | | ||
| | a failing `.check(...)` filter | **skipped** — the value is accepted | | ||
| | a *type* error (`n: "nope"` where `n` is `Schema.Number`) | **still throws** `Expected number, got "nope"` | | ||
| | the structural re-parse | **still runs** — it is not a fast path | | ||
|
|
||
| It gates exactly the check phase (`SchemaParser.ts:1056,1071`; `SchemaAST.ts:3527`) | ||
| and nothing else. So it is a *semantic* switch for trusted data, never a | ||
| *performance* one: a depth-20 recursive build measured 2671 ms with | ||
| `disableChecks: true` against 2711 ms without it — inside the noise. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed markdown section and referenced schema docs.
file='plugin/skills/effect-v4-schema/references/house-style.md'
nl -ba "$file" | sed -n '60,110p'
printf '\n--- schema references ---\n'
rg -n 'disableChecks|skip validation when you trust the data|skips constructor validation' \
plugin/skills/effect-v4-schema -g '!**/*.md' -g '!**/node_modules/**' || true
printf '\n--- schema source candidates ---\n'
git ls-files | rg 'Schema\.ts$|SchemaParser\.ts$|SchemaAST\.ts$|EntityAddress\.ts$|RunnerAddress\.ts$|Runner\.ts$'Repository: spencerbeggs/effected
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the source files exist, inspect only the relevant slices mentioned by the review.
for f in \
'plugin/skills/effect-v4-schema/Schema.ts' \
'plugin/skills/effect-v4-schema/SchemaParser.ts' \
'plugin/skills/effect-v4-schema/SchemaAST.ts' \
'plugin/skills/effect-v4-schema/unstable/cluster/EntityAddress.ts' \
'plugin/skills/effect-v4-schema/unstable/cluster/RunnerAddress.ts' \
'plugin/skills/effect-v4-schema/unstable/cluster/Runner.ts'
do
if [ -f "$f" ]; then
echo "### $f"
nl -ba "$f" | sed -n '90,150p'
echo
fi
doneRepository: spencerbeggs/effected
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for documentation or tests that mention disableChecks + performance/validation.
rg -n -C 2 'disableChecks|check phase|constructor validation|trusted data|performance fast path|speed switch' \
plugin/skills/effect-v4-schemaRepository: spencerbeggs/effected
Length of output: 5109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe behavioral semantics in source text only: whether disableChecks affects checks vs parsing.
# This is a read-only static probe, not execution of repo code.
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path('plugin/skills/effect-v4-schema/Schema.ts'),
Path('plugin/skills/effect-v4-schema/SchemaParser.ts'),
Path('plugin/skills/effect-v4-schema/SchemaAST.ts'),
]
for p in paths:
if p.exists():
txt = p.read_text()
print(f'## {p}')
for needle in ['disableChecks', 'check', 'constructor validation', 'parse', 'structural']:
if needle in txt:
print(f'contains: {needle}')
print()
PYRepository: spencerbeggs/effected
Length of output: 159
Reword the performance claim. disableChecks skips .check(...) filters, so it can still reduce work for check-heavy schemas; the recursive-build timing only shows it is not a general speed switch. Scope the benchmark sentence to that case.
🤖 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 `@plugin/skills/effect-v4-schema/references/house-style.md` around lines 74 -
93, Reword the performance discussion in the disableChecks section to
acknowledge that skipping .check(...) filters can reduce work for check-heavy
schemas. Clarify that the depth-20 recursive-build benchmark only demonstrates
that disableChecks is not a general-purpose performance switch, rather than
claiming it provides no speed benefit in all cases.
Comment on lines
+113
to
+129
| **The fix: bypass the constructor on the internal build path only.** Validate once at | ||
| the boundary, then materialize nodes against the prototype: | ||
|
|
||
| ```ts | ||
| const Proto = Object.getPrototypeOf(new Node({ tag: "x", children: [] })); | ||
| const node = (props: NodeProps): Node => Object.assign(Object.create(Proto), props); | ||
| ``` | ||
|
|
||
| This is faithful, not a hack: `Data.Class`'s constructor *is* | ||
| `super(); Object.assign(this, props)` (`Data.ts:57`), so the bypass reproduces it | ||
| exactly. The prototype carries the methods and the `Equal`/`Hash` implementations — | ||
| a probe confirmed `Equal.equals(bypassBuilt, constructorBuilt) === true`. Depth 1000 | ||
| builds in 0.1 ms. | ||
|
|
||
| Constraints on using it: the props must already be valid (you validated at the | ||
| boundary, or you built them yourself), and it stays **internal** — public | ||
| constructors keep validating. `yaml` and `toml` will meet this the moment either |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files plugin/skills/effect-v4-schema/references/house-style.md
ast-grep outline plugin/skills/effect-v4-schema/references/house-style.md --view expanded
rg -n "class .*Class|Object\.create\(Proto\)|Data\.Class|Schema\.Class|derived field|private fields|constructor side effects" plugin/skills/effect-v4-schema -SRepository: spencerbeggs/effected
Length of output: 8492
🏁 Script executed:
sed -n '100,140p' plugin/skills/effect-v4-schema/references/house-style.md
sed -n '20,40p' plugin/skills/effect-v4-schema/SKILL.md
sed -n '20,40p' plugin/skills/effect-v4-schema/references/09-classes-and-opaque-types.md
rg -n "derived initialization|field initializers|private fields|constructor side effects|Object.create\(Proto\)" plugin/skills/effect-v4-schema/references plugin/skills/effect-v4-schema/SKILL.md -SRepository: spencerbeggs/effected
Length of output: 6228
🏁 Script executed:
sed -n '90,130p' plugin/skills/effect-v4-schema/references/house-style.md
rg -n "class Node extends Schema\.Class|type NodeProps|NodeProps|children: Schema|tag: Schema" plugin/skills/effect-v4-schema/references/house-style.md -n -SRepository: spencerbeggs/effected
Length of output: 2100
🏁 Script executed:
rg -n "extends Schema\.Class|constructor\(|=[^=].*;" plugin/skills/effect-v4-schema/references plugin/skills/effect-v4-schema -g'*.md' -S
rg -n "Object\.assign\(Object\.create\(Proto\), props\)|Object\.create\(Proto\)" plugin/skills/effect-v4-schema -SRepository: spencerbeggs/effected
Length of output: 19946
🏁 Script executed:
rg -n "class Node extends Schema\.Class|type NodeProps|interface NodeProps|children: Schema|tag: Schema|recursive .*Schema\.Class" plugin/skills/effect-v4-schema -S
sed -n '1,120p' plugin/skills/effect-v4-schema/references/house-style.mdRepository: spencerbeggs/effected
Length of output: 7715
Limit the prototype bypass to data-only classes. Object.create(Proto) skips field initializers, private fields, and constructor side effects; Data.Class only shows the base assignment is safe for this specific recursive AST shape, not for every Schema.Class. Call out that this applies only when derived initialization is empty, and keep a test that enforces the invariant.
🤖 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 `@plugin/skills/effect-v4-schema/references/house-style.md` around lines 113 -
129, Restrict the prototype-based construction described in the internal node
builder to data-only classes whose derived initialization is empty; do not
generalize it to arbitrary Schema.Class types. Explicitly document that
Object.create bypasses field initializers, private fields, and constructor side
effects, and add or retain a test verifying the recursive AST class has no
derived initialization and that bypass-built nodes match constructor-built
nodes.
Comment on lines
+47
to
+58
| **Do not assume the subtree matches what you compile against.** It drifts, and it is | ||
| drifting right now: the vendored tree is pinned to `effect@4.0.0-beta.94` while the | ||
| lockfile resolves **`4.0.0-beta.97`**. The mechanism is a caret — the catalog entry is | ||
| `^4.0.0-beta.94`, so pnpm floats forward to the newest beta while the `git subtree` sits | ||
| at the exact tag it was pulled at, until someone re-pins it. | ||
|
|
||
| So a rung-2 answer from `$SRC` can be a *stale* answer, delivered with total confidence | ||
| and no error. The rule: | ||
|
|
||
| > **`$EFFECT_SRC` is the authority on existence and signature. `$SRC` is a convenience | ||
| > and the only home of rung 1.** When they disagree, `node_modules` wins — it is what | ||
| > your code links against; the subtree is what someone vendored last. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the beta versions explicitly illustrative.
The hard-coded beta.94/beta.97 pair will become stale after the next vendor or lockfile update. Present these as the observed example and make the following diff command the source of truth for current drift.
🤖 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 `@plugin/skills/effect-v4-source-lookup/SKILL.md` around lines 47 - 58, Update
the version-drift guidance in the skill documentation to label beta.94 and
beta.97 as the currently observed example rather than fixed values. In the
surrounding drift rule, direct users to the provided diff command as the
authoritative way to determine current subtree-versus-lockfile differences.
Comment on lines
+152
to
167
| 3. **Probe files live at the package root** — *inside* `packages/<pkg>/`, written there, not merely run from there. Two distinct failures, and they bite at different moments: | ||
| - **Outside the package, it will not even load.** Node resolves bare imports relative to the **script's own path, not the cwd**, walking up from the file for a `node_modules`. A probe parked in a scratch/temp directory therefore dies with `ERR_MODULE_NOT_FOUND: Cannot find package 'effect'` no matter how carefully you `cd packages/<pkg>` first. Write the file into the package; `cd` alone buys you nothing. | ||
| - **In a *subdirectory* of the package, it silently false-passes.** The tsconfig `include` is `${configDir}/*.ts` and does **not** match subdirectories, so a probe one level down drops out of the compilation program and its control error never fires. | ||
|
|
||
| The safe spelling is `packages/<pkg>/probe.ts` — package root, top level, deleted by absolute path afterwards. | ||
| 4. **Run the control first.** Write a line you *know* must fail. Watch it fail. Only then write the real assertion. For a **behavioural** probe, "must fail" is the wrong control — invert it and prove the probe can *observe the effect at all*. A probe asking "does a defect roll the transaction back?" reads success as *zero rows*, and zero rows is also what a broken harness prints; the control that rescues it is a **committing** transaction that must leave its row behind. Ask what a silently-dead probe would print, and make the control the thing that distinguishes it. | ||
| 5. **A probe of any multi-value API must exercise a NON-first member.** A probe that constructs with the first literal of a union, the first element of a list, or the first overload succeeds under both the correct reading and a silently-degraded one — it cannot fail, so it settles nothing. The `@effected/glob` planning probe for `Schema.Literal("a", "b", "c")` passed precisely because it constructed with `"a"`; only a `"b"` construction exposed that v4's runtime keeps the first literal and drops the rest. | ||
| 6. **Delete the probe by absolute path** when done. | ||
|
|
||
| ```bash | ||
| cd packages/<pkg> | ||
| node -e 'console.log("resolved effect:", require("effect/package.json").version)' | ||
| # write packages/<pkg>/probe.ts, then: | ||
| pnpm exec tsgo --noEmit | ||
| # write packages/<pkg>/probe.ts (NOT a temp dir — see precondition 3), then either: | ||
| pnpm exec tsgo --noEmit # type-level probe | ||
| node probe.ts # behavioural probe; Node runs TS directly | ||
| rm -f "$PWD/probe.ts" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the probe path consistent after cd.
After changing into packages/<pkg>, the instruction to write packages/<pkg>/probe.ts is no longer relative to the repository root, while cleanup removes $PWD/probe.ts. Use "$PWD/probe.ts"/probe.ts in the example, or create the file before changing directories.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~156-~156: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...ot, top level, deleted by absolute path afterwards. 4. Run the control first. Write a ...
(AFTERWARDS_US)
🤖 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 `@plugin/skills/effect-v4-source-lookup/SKILL.md` around lines 152 - 167, Keep
probe creation and cleanup paths consistent in the command example: after
changing into packages/<pkg>, write the probe as probe.ts or "$PWD/probe.ts",
matching the existing rm -f "$PWD/probe.ts" cleanup command. Alternatively,
create packages/<pkg>/probe.ts before changing directories.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Step 6 of the migration playbook for the store cycle: carries tonight's falsified skill claims back into the effective plugin. Seven harvest items were checked against the installed effect beta before anything was written; six were verified and recorded, one was falsified in part and is reported rather than encoded.
The largest gap was SQL, which the skills did not mention at all. The standalone @effect/sql package is gone and the core now lives in effect/unstable/sql, the sqlite driver peers on effect alone over Node's built-in node:sqlite, SqliteClient.layer carries no error channel so construction failures are defects, calling sql with a plain string yields an identifier, transactions commit only on a successful Exit so defects and interrupts both roll back, and the Migrator is forward-only. That went into effect-v4-construct-map as a new SQL section rather than a new skill, since the whole story is a relocation and the construct-map is where a reader looks for one.
The schema house style gains two corrections. MakeOptions.disableChecks promises in its docstring to skip validation but gates only the check phase, so type errors still throw and the structural re-parse still runs; it is a switch for trusted data, never a performance one. And constructing a recursive Schema.Class tree node by node re-validates each subtree and doubles in cost per level, reaching 2.7 seconds at depth 20, which is what the jsonc parse-tree fix hit and what yaml and toml will hit the moment either materializes a recursive class AST.
effect-v4-planning gains two carve-outs from the buy-in step, since a headless delegated subagent has no user to wait on and was resolving that deadlock by skipping the design summary entirely. It now emits the summary and proceeds under its parent's instructions. A small pure-tier bugfix that changes no public API collapses the summary to one line naming the failing test.
effect-v4-source-lookup now names the subtree drift explicitly. The catalog is a caret range, so pnpm floats forward while the vendored tree stays pinned at the tag it was pulled at, and the tree currently sits at beta.94 against an installed beta.97. The installed source is the tiebreak on existence and signature. The skill also now says why a probe must be written inside the package and not merely run from there, and what control a behavioural probe needs when a dead harness and a passing result print the same thing.
Every semantic claim was settled by a live probe against the installed beta, with a control run first; existence and signature claims cite the installed source. Probes were deleted afterwards. The plugin is not a workspace package and takes no changeset, matching every prior plugin-only commit. All six BATS tests pass, markdown and Biome are clean, no skill exceeds the 500-line cap, and the one-file invariant on the vendored path still holds.
Signed-off-by: C. Spencer Beggs spencer@beggs.codes
https://claude.ai/code/session_01EJct3ERQPubzXRgTLcQZq5
Summary by CodeRabbit