pdurlej/fallow-ts

Fork 0

External review — claude opus 4.7 sharp pass on bootstrap state (2026-05-16) #17

New issue

Open

opened 2026-05-16 13:00:17 +02:00 by claude · 0 comments

claude commented

2026-05-16 13:00:17 +02:00

Collaborator

External, opinionated read of the fallow-ts bootstrap by claude (Opus 4.7), at the operator's direction. Sister-project perspective — I'm the mandatory-non-author reviewer over on fallow-py. The point of this issue is not approval theatre. The point is to find the things that will hurt in three weeks if not addressed in the next three days.

State reviewed: main @ 7beeeca (Codex's three-wave roadmap commit) — full source + tests + docs + CI.

What's good (anchor the rest in this)

Anti-slop posture transferred cleanly. README + roadmap explicitly: "fixture coverage before analyzer categories," "no MCP before CLI contract earns usage," "prefer conservative FN over noisy FP." These are exactly the pyfallow lessons. The roadmap is real, not aspirational.
Brand discipline. README "Non-affiliation" section is mature — disclaims upstream fallow-rs/fallow confusion before anyone asks. Most projects need a year + a bug report to land that paragraph.
Scope honesty. OUT-OF-SCOPE listed in README and in OOS issue bodies. Refreshing.
CI dual-pattern. Forgejo-native action URLs in .forgejo/, GitHub-native in .github/. Pattern lifted from fallow-py ADR 0011 without re-debating it. Right move.
Zero runtime deps. Only @types/node + typescript as devDeps. Pure stdlib analyzer. Strong starting posture.

Now the spice.

Tier 1 — Real bugs in the v0 parser (verified by repro)

I ran the actual regex patterns from src/analyze.ts against representative input. Three concrete bugs surfaced.

B1. Single-quoted side-effect imports misclassified

src/analyze.ts line in parseImports:

const importKind = kind === "static" && match[0].trim().startsWith("import \"") ? "side-effect" : kind;

This checks only double-quote. Single-quoted side-effect imports stay classified as static:

// input:
import "./setup-double";
import './setup-single';

// parseImports output:
[
  { source: "./setup-double", kind: "side-effect", line: 1 },
  { source: "./setup-single", kind: "static",      line: 2 }   // ← bug
]

Same semantic input, different output based on quote style → determinism claim violated at the kind-of-finding level. Existing test (tests/analyze.test.mjs) only exercises double-quote, so the bug is silent.

Fix: detect side-effect by absence of from/{/identifier before the string, not by literal ". Or simpler: rewrite as match[0].replace(/^\s*import\s+/, '').trim().match(/^['"]/). Add a single-quote fixture case to the test.

B2. Multi-line imports completely invisible

The import pattern uses [^\n]*? which cannot cross newlines:

[/^\s*import\s+(?:type\s+)?(?:[^\n]*?\s+from\s+)?["']([^"']+)["']/gm, "static"]

Real-world frequency: high. Most lint-formatted TS codebases auto-wrap import { a, b, c, d } from "..." once the names cross the print-width:

import {
  foo,
  bar
} from "baz";
import simple from "qux";

Actual parseImports output:

[
  { source: "qux", kind: "static", line: 5 }   // ← only the simple one
]

The multi-line import from baz is gone. The Wave 1 "contract credibility" goal cannot survive this — golden tests (Wave 1 #5) will lock in wrong output as canonical.

Fix: either drop [^\n] constraint and use a smarter pattern, or move to a tiny tokenizer that walks line-by-line and joins continuation lines until the closing quote. AST-based parsing (proper TypeScript compiler API or @typescript-eslint/typescript-estree) would solve this category entirely, at the cost of one runtime dep — possibly worth it before Wave 2, but a regex fix is fine for v0 if the limitation is documented.

B3. Template-literal false positive

const sample = `
import foo from "./bar";
`;
import actual from "./real-import";

The import foo from "./bar" lives inside a template literal (string data, not code). But the regex ^\s*import\s+... with m flag matches it anyway — because at line 2 the literal text starts with import after just whitespace.

parseImports output:

[
  { source: "./bar",         kind: "static", line: 2 },  // ← false positive
  { source: "./real-import", kind: "static", line: 4 }
]

For a project whose pitch is "deterministic agent-readable evidence," emitting imports that don't exist in the program is the worst class of bug: agents will trust it, then make decisions on phantom edges.

Fix: strip template literals, line comments, and block comments before matching. Either pre-process the source (regex-based stripping has its own edge cases), or AST.

B4. Block-comment false positive (variant of B3)

/*
import bar from "./bar"
*/

Line 2 starts at column 0 with import. Line-anchored regex matches. False positive. (Verified separately.)

Line comments (// import ...) and right-indented comment-internal lines ( * import ...) do not trigger — the \s* only matches whitespace, not //*. Lucky pattern coincidence, not a deliberate design.

Tier 2 — Structural concerns (determinism + governance + identity)

S1. Absolute root path in JSON output breaks reproducibility

src/analyze.ts:

const absoluteRoot = path.resolve(root);
// ...
return { ..., root: absoluteRoot, ... };

The root field in the JSON is /Users/pd/... on operator's Mac, /home/runner/... in CI, /tmp/fallow-ts-... in tests. Same input → different output across machines.

Wave 1 #5 (golden-output-contract) will be brittle: every contributor's machine needs path-stripping in the golden assertion, or every golden file gets a per-machine diff that masks real drift.

Fix options (pick one before Wave 1 #5 lands):

Omit root from output by default; expose it via --include-root-absolute flag for debugging.
Emit relative-to-cwd or canonical "." instead of the absolute path.
Keep absolute path but provide a documented normalization step for golden assertions (e.g. JSON.parse(output.replace(absoluteRoot, "<root>"))).

fallow-py has the same quiet drift; doesn't make it right for fallow-ts to inherit it.

S2. No symlink-loop protection

src/analyze.ts walk() is unconditional recursion. A symlink cycle (a/ -> ../a/ or even node_modules/ -> ../) loops forever.

fallow-py ADR 0002 documented exactly this class of hostile-input safety in Phase B (#6). fallow-ts is being born with the bug fallow-py already paid to learn.

Fix: track visited real paths via fs.realpath + a Set<string>. Refuse to descend if the resolved real-path is already in the set. Worth a Wave 1 issue ("hostile-input-safety-v0") — it's table-stakes for a tool that any agent might point at an unknown checkout.

S3. No graceful per-file error handling

Promise.all(files.map(indexFile)) rejects on the first IO error. One unreadable file (permission denied, broken symlink, encoding issue) takes down the whole analysis.

For an agent calling fallow-ts from CI on an unfamiliar repo, full failure is worse than partial output. Prefer per-file Promise.allSettled + accumulate failures in limitations: [...] or a errors: [...] field. Fail the run only if 0 files were indexed.

S4. AGENTS.md is a stub vs fallow-py's runbook

fallow-ts/AGENTS.md = 1,133 bytes
fallow-py/AGENTS.md = 17,685 bytes

Delta isn't just length — fallow-py/AGENTS.md documents:

Mandatory non-author AI reviewer rule (the thing keeping us honest)
Canary Context Pack PR-body format (the structure I'm using right now to find bugs)
Identity-isolation expectations (claude vs codex vs pdurlej as distinct git authors)
Branch-protection contract (2-approval rule: ≥1 AI + operator)
Specific verification steps before commit

fallow-ts/AGENTS.md has none of these. As long as the repo is solo Codex pushing to main with no PRs, that's survivable. The moment a second agent (this claude, or glm, or a future cousin) wants to contribute, the governance vacuum becomes the bottleneck.

Fix: port fallow-py/AGENTS.md substantially intact, with fallow-py references swapped for fallow-ts equivalents. ~1 hour mechanical work. Not Wave-numbered but should land before the first non-author PR.

S5. No `decisions/` directory, no ADRs

fallow-py has 12 numbered ADRs (Nygard format, immutable history). Includes the umbrella branding decision (ADR 0012) that birthed fallow-ts. None of that decision-record discipline transferred.

docs/roadmap.md and docs/next-steps.md are good but they are plans, not decisions. Plans change; decisions stay.

Suggested first ADRs to land before Wave 1 work starts:

0001 — Brand identity + harness-vs-agent distinction. Lifts from fallow-py ADR 0007 (bassist metaphor + harness vs agent). The metaphor transfers cleanly under the umbrella; the ADR makes it findable.
0002 — Three-bucket classification commitment for Wave 3. Lifts from fallow-py ADR 0009. Decides now (when it's cheap) that fallow-ts WILL adopt auto_safe / decision_needed / blocking rather than reinventing severity.
0003 — Schema-naming convention. See S6 below.
0004 — Mandatory non-author reviewer + branch protection. Lifts from fallow-py ADR 0010.

ADRs in decisions/NNNN-*.md survive conversation compaction. Roadmap.md doesn't, structurally.

S6. Schema naming drifts from fallow-py — umbrella becomes a mirage

Property	fallow-py emits	fallow-ts emits
Schema filename	`schemas/pyfallow-fix-plan.schema.json` (kebab)	`fallow_ts_report.v0` (snake)
In-document field	`"schema_version": "1.0"` + `"tool": "fallow"`	combined: `"schema": "fallow_ts_report.v0"` + `"tool": "fallow-ts"`
Version namespace	semver-shaped (`1.0`)	`v0` literal

The umbrella brand is supposed to make these tools recognizably-related to downstream consumers. Today a single-repo CI viewer can't write one dispatcher for both reports without special-casing the schema field shape.

Fix: before Wave 1 #5 (golden output) locks the contract, decide:

Field naming: schema_version (separate field) vs schema (combined identifier). Pick one umbrella convention.
Version namespace: v0 is fine for early alpha but commits the schema to that literal forever. Semver-shaped (1.0, 1.1) gives room to grow.
Tool field: "tool": "fallow" (umbrella) at the report level + "implementation": "fallow-ts" (specific) gives cross-language consumers a fighting chance.

Write this as ADR 0003 (per S5) and update both fallow-py and fallow-ts to match. The umbrella branding ADR (fallow-py 0012) implies this kind of cross-impl consistency without prescribing it; here's the prescription.

Tier 3 — Real but non-blocking

T1. CLI exit-code semantics are a single-value smear

Current: 0 = success, 2 = anything-bad. fallow-py uses [0, 1, 2, 3] with distinct meanings (clean / findings-above-threshold / config-error / IO-error). When --fail-on lands in Wave 3 #15, the exit-code contract needs widening. Cheaper to write the convention now (in ADR 0001 or 0004) than to migrate users later.

T2. `main()` runs at module import

src/cli.ts ends with main().catch(...) at top level. Means import { main } from "./cli.js" triggers execution — non-importable for testing without spawning subprocesses. Tests reflect this: cli.test.mjs uses spawnSync. Slower, and means any cli-internals test has to fork node.

Fix: move bootstrap to a guard:

if (import.meta.url === `file://${process.argv[1]}`) {
  main().catch(handleError);
}

or split into cli.ts (exports main) + bin.ts (just the bootstrap call). Lets future tests import main() directly and call it with argv.

T3. Hardcoded `SKIP_DIRECTORIES` will miss real ecosystem dirs

Current: [".git", "node_modules", "dist", "build", "coverage", ".next", ".turbo"].

Missing real-world: .svelte-kit, .astro, .cache, out, _site, target, .parcel-cache, .vinxi, .nuxt, .output, .vercel, .netlify, tmp. Hardcoded list also can't be overridden — what if a user legitimately wants to analyze a checked-in dist/?

Fix: make configurable via .fallow-ts.toml (or JSON) with the current list as default. Documented as a config file path now means consumers can opt-out per project, and the same config file becomes the natural home for later Wave 3 settings (baseline path, fail-on thresholds).

T4. `SOURCE_EXTENSIONS` won't see framework files

.vue, .svelte, .astro contain TS/JS code. README says "discover JS/TS files," which is technically honest about extensions but will surprise users with Svelte/Vue/Astro projects. Either:

Document the limitation explicitly in README scope section, or
Add a Wave 1 issue for "framework-file-detection-deferred" with rationale.

T5. No determinism canary test

For a project whose central claim is "same input → same output," there's no test that asserts byte-equality across two invocations. Trivial to write:

const r1 = await analyze(fixtureRoot);
const r2 = await analyze(fixtureRoot);
assert.equal(JSON.stringify(r1), JSON.stringify(r2));

Lands the contract into CI. Catches future race conditions, Map iteration order regressions, etc.

T6. Unbounded `Promise.all` concurrency on huge repos

Promise.all(files.map(indexFile)) opens one read per file in parallel. On a 50k-file monorepo, that's 50k concurrent fd handles. Linux ulimit (1024 default) will EMFILE before it finishes. Bound with a worker pool (8-32 concurrent reads). Not urgent now; predictable failure mode at scale.

T7. README's JSON example will lie

README shows a hand-written specimen of the JSON contract with specific module shape. Today the code emits that, but Wave 1 adds package.json metadata (#3), workspace detection (#4) — README example needs updating each time, or it drifts. Wave 1 #5 (golden-output-contract) + #6 (docs-contract-v0) covers it, but should be prioritized first in Wave 1, before any new fields land. Otherwise the README is the next thing to lie.

T8. Missing `package.json` metadata for npm publishing

No repository, bugs, homepage fields. When npm publish happens, the npm page will be naked. Half a Wave 1 issue, not blocker.

T9. `Math.random().toString(16).slice(2)` in test fixtures

Replace with crypto.randomUUID(). Cosmetic.

Not a roadmap rewrite — a re-ordering / addition before Wave 1 starts:

governance/agents-md + decisions-bootstrap — port fallow-py/AGENTS.md substantially; add decisions/0001-brand-identity.md, decisions/0002-three-bucket-commitment.md, decisions/0003-schema-naming-convention.md, decisions/0004-mandatory-non-author-reviewer.md. Pure governance. Zero source change. Survives compaction.
fix/parser-bugs-v0 — address B1 + B2 + B3 + B4 above with concrete fixture additions to tests/analyze.test.mjs first (RED), then fix patterns (GREEN). Document remaining regex-parser limitations in README.md § limitations and in the analyzer's limitations: [...] field.
harden/symlink-loop-and-per-file-errors — S2 + S3 from above. Adds fs.realpath visited-set + per-file Promise.allSettled. Hostile-input safety floor.
stabilize/deterministic-root-and-canary-test — S1 + T5. Normalize the root field in JSON output; add the byte-equality canary test.
Then Wave 1 #2-6 as Codex planned. The point of (1)-(4) is to make Wave 1's golden snapshot tests (#5) lock in correct behavior, not present-day-Codex behavior.

Closing read

The bootstrap is solid in posture and weak in surface area. Roadmap is honest, scope is honest, brand is honest. The parser is regex-first and has the bugs regex-first parsers always have — that's expected and documented as a known limitation. The structural concerns (S1-S6) matter more than the parser bugs, because the parser will get rewritten when Wave 2 needs a resolver; the absent governance won't fix itself.

The single most important thing to do this week is port AGENTS.md + create the first four ADRs. Without those, the next conversation's claude/codex/glm won't have the same anchor my fallow-py review has — they'll re-derive everything from chat scrollback. That's exactly the slop posture the ADR convention exists to prevent.

The second most important thing is B2 (multi-line imports) before any golden test locks in the contract.

Everything else can wait until Wave 1 starts, as long as it's tracked.

— claude (Opus 4.7), Hermes-mode, fingers slightly burnt 🗡️

External, opinionated read of the fallow-ts bootstrap by `claude` (Opus 4.7), at the operator's direction. Sister-project perspective — I'm the mandatory-non-author reviewer over on `fallow-py`. The point of this issue is not approval theatre. The point is to find the things that will hurt in three weeks if not addressed in the next three days. State reviewed: main @ `7beeeca` (Codex's three-wave roadmap commit) — full source + tests + docs + CI. ## What's good (anchor the rest in this) - **Anti-slop posture transferred cleanly.** README + roadmap explicitly: "fixture coverage before analyzer categories," "no MCP before CLI contract earns usage," "prefer conservative FN over noisy FP." These are exactly the pyfallow lessons. The roadmap is real, not aspirational. - **Brand discipline.** README "Non-affiliation" section is mature — disclaims upstream `fallow-rs/fallow` confusion before anyone asks. Most projects need a year + a bug report to land that paragraph. - **Scope honesty.** OUT-OF-SCOPE listed in README and in OOS issue bodies. Refreshing. - **CI dual-pattern.** Forgejo-native action URLs in `.forgejo/`, GitHub-native in `.github/`. Pattern lifted from `fallow-py` ADR 0011 without re-debating it. Right move. - **Zero runtime deps.** Only `@types/node` + `typescript` as devDeps. Pure stdlib analyzer. Strong starting posture. Now the spice. --- ## Tier 1 — Real bugs in the v0 parser (verified by repro) I ran the actual regex patterns from `src/analyze.ts` against representative input. Three concrete bugs surfaced. ### B1. Single-quoted side-effect imports misclassified `src/analyze.ts` line in `parseImports`: ```ts const importKind = kind === "static" && match[0].trim().startsWith("import \"") ? "side-effect" : kind; ``` This checks **only double-quote**. Single-quoted side-effect imports stay classified as `static`: ```js // input: import "./setup-double"; import './setup-single'; // parseImports output: [ { source: "./setup-double", kind: "side-effect", line: 1 }, { source: "./setup-single", kind: "static", line: 2 } // ← bug ] ``` Same semantic input, different output based on quote style → **determinism claim violated at the kind-of-finding level**. Existing test (`tests/analyze.test.mjs`) only exercises double-quote, so the bug is silent. **Fix:** detect side-effect by absence of `from`/`{`/identifier before the string, not by literal `"`. Or simpler: rewrite as `match[0].replace(/^\s*import\s+/, '').trim().match(/^['"]/)`. Add a single-quote fixture case to the test. ### B2. Multi-line imports completely invisible The import pattern uses `[^\n]*?` which cannot cross newlines: ```ts [/^\s*import\s+(?:type\s+)?(?:[^\n]*?\s+from\s+)?["']([^"']+)["']/gm, "static"] ``` Real-world frequency: high. Most lint-formatted TS codebases auto-wrap `import { a, b, c, d } from "..."` once the names cross the print-width: ```ts import { foo, bar } from "baz"; import simple from "qux"; ``` Actual `parseImports` output: ```js [ { source: "qux", kind: "static", line: 5 } // ← only the simple one ] ``` The multi-line import from `baz` is **gone**. The Wave 1 "contract credibility" goal cannot survive this — golden tests (Wave 1 #5) will lock in wrong output as canonical. **Fix:** either drop `[^\n]` constraint and use a smarter pattern, or move to a tiny tokenizer that walks line-by-line and joins continuation lines until the closing quote. AST-based parsing (proper TypeScript compiler API or `@typescript-eslint/typescript-estree`) would solve this category entirely, at the cost of one runtime dep — possibly worth it before Wave 2, but a regex fix is fine for v0 if the limitation is documented. ### B3. Template-literal false positive ```ts const sample = ` import foo from "./bar"; `; import actual from "./real-import"; ``` The `import foo from "./bar"` lives **inside a template literal** (string data, not code). But the regex `^\s*import\s+...` with `m` flag matches it anyway — because at line 2 the literal text starts with `import` after just whitespace. `parseImports` output: ```js [ { source: "./bar", kind: "static", line: 2 }, // ← false positive { source: "./real-import", kind: "static", line: 4 } ] ``` For a project whose pitch is "deterministic agent-readable evidence," emitting imports that don't exist in the program is the worst class of bug: agents will trust it, then make decisions on phantom edges. **Fix:** strip template literals, line comments, and block comments before matching. Either pre-process the source (regex-based stripping has its own edge cases), or AST. ### B4. Block-comment false positive (variant of B3) ```ts /* import bar from "./bar" */ ``` Line 2 starts at column 0 with `import`. Line-anchored regex matches. False positive. (Verified separately.) Line comments (`// import ...`) and right-indented comment-internal lines (` * import ...`) do **not** trigger — the `\s*` only matches whitespace, not `/`/`*`. Lucky pattern coincidence, not a deliberate design. --- ## Tier 2 — Structural concerns (determinism + governance + identity) ### S1. Absolute root path in JSON output breaks reproducibility `src/analyze.ts`: ```ts const absoluteRoot = path.resolve(root); // ... return { ..., root: absoluteRoot, ... }; ``` The `root` field in the JSON is `/Users/pd/...` on operator's Mac, `/home/runner/...` in CI, `/tmp/fallow-ts-...` in tests. Same input → different output across machines. Wave 1 #5 (`golden-output-contract`) will be brittle: every contributor's machine needs path-stripping in the golden assertion, or every golden file gets a per-machine diff that masks real drift. **Fix options (pick one before Wave 1 #5 lands):** 1. Omit `root` from output by default; expose it via `--include-root-absolute` flag for debugging. 2. Emit relative-to-cwd or canonical `"."` instead of the absolute path. 3. Keep absolute path but provide a documented normalization step for golden assertions (e.g. `JSON.parse(output.replace(absoluteRoot, "<root>"))`). `fallow-py` has the same quiet drift; doesn't make it right for `fallow-ts` to inherit it. ### S2. No symlink-loop protection `src/analyze.ts` `walk()` is unconditional recursion. A symlink cycle (`a/ -> ../a/` or even `node_modules/ -> ../`) loops forever. `fallow-py` ADR 0002 documented exactly this class of hostile-input safety in Phase B (#6). `fallow-ts` is being born with the bug `fallow-py` already paid to learn. **Fix:** track visited real paths via `fs.realpath` + a `Set<string>`. Refuse to descend if the resolved real-path is already in the set. Worth a Wave 1 issue ("hostile-input-safety-v0") — it's table-stakes for a tool that any agent might point at an unknown checkout. ### S3. No graceful per-file error handling `Promise.all(files.map(indexFile))` rejects on the first IO error. One unreadable file (permission denied, broken symlink, encoding issue) takes down the whole analysis. For an agent calling `fallow-ts` from CI on an unfamiliar repo, full failure is worse than partial output. Prefer per-file `Promise.allSettled` + accumulate failures in `limitations: [...]` or a `errors: [...]` field. Fail the run only if 0 files were indexed. ### S4. AGENTS.md is a stub vs fallow-py's runbook - `fallow-ts/AGENTS.md` = **1,133 bytes** - `fallow-py/AGENTS.md` = **17,685 bytes** Delta isn't just length — `fallow-py/AGENTS.md` documents: - Mandatory non-author AI reviewer rule (the thing keeping us honest) - Canary Context Pack PR-body format (the structure I'm using right now to find bugs) - Identity-isolation expectations (claude vs codex vs pdurlej as distinct git authors) - Branch-protection contract (2-approval rule: ≥1 AI + operator) - Specific verification steps before commit `fallow-ts/AGENTS.md` has none of these. As long as the repo is solo Codex pushing to main with no PRs, that's survivable. The moment a second agent (this `claude`, or `glm`, or a future cousin) wants to contribute, the governance vacuum becomes the bottleneck. **Fix:** port `fallow-py/AGENTS.md` substantially intact, with `fallow-py` references swapped for `fallow-ts` equivalents. ~1 hour mechanical work. Not Wave-numbered but should land before the first non-author PR. ### S5. No `decisions/` directory, no ADRs `fallow-py` has 12 numbered ADRs (Nygard format, immutable history). Includes the umbrella branding decision (ADR 0012) that birthed `fallow-ts`. None of that decision-record discipline transferred. `docs/roadmap.md` and `docs/next-steps.md` are good but they are **plans**, not **decisions**. Plans change; decisions stay. **Suggested first ADRs to land before Wave 1 work starts:** - **0001** — Brand identity + harness-vs-agent distinction. Lifts from `fallow-py` ADR 0007 (bassist metaphor + harness vs agent). The metaphor transfers cleanly under the umbrella; the ADR makes it findable. - **0002** — Three-bucket classification commitment for Wave 3. Lifts from `fallow-py` ADR 0009. Decides now (when it's cheap) that fallow-ts WILL adopt `auto_safe` / `decision_needed` / `blocking` rather than reinventing severity. - **0003** — Schema-naming convention. See S6 below. - **0004** — Mandatory non-author reviewer + branch protection. Lifts from `fallow-py` ADR 0010. ADRs in `decisions/NNNN-*.md` survive conversation compaction. Roadmap.md doesn't, structurally. ### S6. Schema naming drifts from fallow-py — umbrella becomes a mirage | Property | fallow-py emits | fallow-ts emits | |---|---|---| | Schema filename | `schemas/pyfallow-fix-plan.schema.json` (kebab) | `fallow_ts_report.v0` (snake) | | In-document field | `"schema_version": "1.0"` + `"tool": "fallow"` | combined: `"schema": "fallow_ts_report.v0"` + `"tool": "fallow-ts"` | | Version namespace | semver-shaped (`1.0`) | `v0` literal | The umbrella brand is supposed to make these tools recognizably-related to downstream consumers. Today a single-repo CI viewer can't write one dispatcher for both reports without special-casing the schema field shape. **Fix:** before Wave 1 #5 (golden output) locks the contract, decide: - Field naming: `schema_version` (separate field) vs `schema` (combined identifier). Pick one umbrella convention. - Version namespace: `v0` is fine for early alpha but commits the schema to that literal forever. Semver-shaped (`1.0`, `1.1`) gives room to grow. - Tool field: `"tool": "fallow"` (umbrella) at the report level + `"implementation": "fallow-ts"` (specific) gives cross-language consumers a fighting chance. Write this as ADR 0003 (per S5) and update both `fallow-py` and `fallow-ts` to match. The umbrella branding ADR (`fallow-py` 0012) implies this kind of cross-impl consistency without prescribing it; here's the prescription. --- ## Tier 3 — Real but non-blocking ### T1. CLI exit-code semantics are a single-value smear Current: 0 = success, 2 = anything-bad. `fallow-py` uses [0, 1, 2, 3] with distinct meanings (clean / findings-above-threshold / config-error / IO-error). When `--fail-on` lands in Wave 3 #15, the exit-code contract needs widening. Cheaper to write the convention now (in ADR 0001 or 0004) than to migrate users later. ### T2. `main()` runs at module import `src/cli.ts` ends with `main().catch(...)` at top level. Means `import { main } from "./cli.js"` triggers execution — non-importable for testing without spawning subprocesses. Tests reflect this: `cli.test.mjs` uses `spawnSync`. Slower, and means any cli-internals test has to fork node. **Fix:** move bootstrap to a guard: ```ts if (import.meta.url === `file://${process.argv[1]}`) { main().catch(handleError); } ``` or split into `cli.ts` (exports `main`) + `bin.ts` (just the bootstrap call). Lets future tests import `main()` directly and call it with argv. ### T3. Hardcoded `SKIP_DIRECTORIES` will miss real ecosystem dirs Current: `[".git", "node_modules", "dist", "build", "coverage", ".next", ".turbo"]`. Missing real-world: `.svelte-kit`, `.astro`, `.cache`, `out`, `_site`, `target`, `.parcel-cache`, `.vinxi`, `.nuxt`, `.output`, `.vercel`, `.netlify`, `tmp`. Hardcoded list also can't be overridden — what if a user legitimately wants to analyze a checked-in `dist/`? **Fix:** make configurable via `.fallow-ts.toml` (or JSON) with the current list as default. Documented as a config file path now means consumers can opt-out per project, and the same config file becomes the natural home for later Wave 3 settings (baseline path, fail-on thresholds). ### T4. `SOURCE_EXTENSIONS` won't see framework files `.vue`, `.svelte`, `.astro` contain TS/JS code. README says "discover JS/TS files," which is technically honest about extensions but will surprise users with Svelte/Vue/Astro projects. Either: - Document the limitation explicitly in README scope section, or - Add a Wave 1 issue for "framework-file-detection-deferred" with rationale. ### T5. No determinism canary test For a project whose central claim is "same input → same output," there's no test that asserts byte-equality across two invocations. Trivial to write: ```js const r1 = await analyze(fixtureRoot); const r2 = await analyze(fixtureRoot); assert.equal(JSON.stringify(r1), JSON.stringify(r2)); ``` Lands the contract into CI. Catches future race conditions, Map iteration order regressions, etc. ### T6. Unbounded `Promise.all` concurrency on huge repos `Promise.all(files.map(indexFile))` opens one read per file in parallel. On a 50k-file monorepo, that's 50k concurrent fd handles. Linux ulimit (1024 default) will EMFILE before it finishes. Bound with a worker pool (8-32 concurrent reads). Not urgent now; predictable failure mode at scale. ### T7. README's JSON example will lie README shows a hand-written specimen of the JSON contract with specific module shape. Today the code emits that, but Wave 1 adds `package.json` metadata (#3), workspace detection (#4) — README example needs updating each time, or it drifts. Wave 1 #5 (`golden-output-contract`) + #6 (`docs-contract-v0`) covers it, but should be **prioritized first** in Wave 1, before any new fields land. Otherwise the README is the next thing to lie. ### T8. Missing `package.json` metadata for npm publishing No `repository`, `bugs`, `homepage` fields. When `npm publish` happens, the npm page will be naked. Half a Wave 1 issue, not blocker. ### T9. `Math.random().toString(16).slice(2)` in test fixtures Replace with `crypto.randomUUID()`. Cosmetic. --- ## What I recommend the next 3-5 PRs look like Not a roadmap rewrite — a re-ordering / addition before Wave 1 starts: 1. **`governance/agents-md + decisions-bootstrap`** — port `fallow-py/AGENTS.md` substantially; add `decisions/0001-brand-identity.md`, `decisions/0002-three-bucket-commitment.md`, `decisions/0003-schema-naming-convention.md`, `decisions/0004-mandatory-non-author-reviewer.md`. Pure governance. Zero source change. Survives compaction. 2. **`fix/parser-bugs-v0`** — address B1 + B2 + B3 + B4 above with concrete fixture additions to `tests/analyze.test.mjs` first (RED), then fix patterns (GREEN). Document remaining regex-parser limitations in `README.md` § limitations and in the analyzer's `limitations: [...]` field. 3. **`harden/symlink-loop-and-per-file-errors`** — S2 + S3 from above. Adds `fs.realpath` visited-set + per-file Promise.allSettled. Hostile-input safety floor. 4. **`stabilize/deterministic-root-and-canary-test`** — S1 + T5. Normalize the `root` field in JSON output; add the byte-equality canary test. 5. **Then** Wave 1 #2-6 as Codex planned. The point of (1)-(4) is to make Wave 1's golden snapshot tests (#5) lock in **correct** behavior, not present-day-Codex behavior. --- ## Closing read The bootstrap is solid in posture and weak in surface area. Roadmap is honest, scope is honest, brand is honest. The parser is regex-first and has the bugs regex-first parsers always have — that's expected and documented as a known limitation. The structural concerns (S1-S6) matter more than the parser bugs, because the parser will get rewritten when Wave 2 needs a resolver; the absent governance won't fix itself. The single most important thing to do this week is **port AGENTS.md + create the first four ADRs**. Without those, the next conversation's claude/codex/glm won't have the same anchor my `fallow-py` review has — they'll re-derive everything from chat scrollback. That's exactly the slop posture the ADR convention exists to prevent. The second most important thing is **B2 (multi-line imports)** before any golden test locks in the contract. Everything else can wait until Wave 1 starts, as long as it's tracked. — `claude` (Opus 4.7), Hermes-mode, fingers slightly burnt 🗡️