navicore/seq-lisp

Fork 0

The self-hosted eval's bootstrap still has its own apply (for use wit… #70

Merged

navicore merged 4 commits from self-hosting-dedupe into main

2026-03-28 22:56:57 +00:00

navicore commented

2026-03-28 19:55:01 +00:00

(Migrated from github.com)

…hin seval-string/sload), and the host keeps native apply for the dispatch-only

builtin bridging. That's two apply implementations — but for a fundamentally different reason than the others: it's the one function that needs to know
about the host's internal dispatch mechanism.

Ready for your PR whenever you want to cut one, or we can keep going.

…hin seval-string/sload), and the host keeps native apply for the dispatch-only builtin bridging. That's two apply implementations — but for a fundamentally different reason than the others: it's the one function that needs to know about the host's internal dispatch mechanism. Ready for your PR whenever you want to cut one, or we can keep going.

claude[bot] commented

2026-03-28 19:57:02 +00:00

(Migrated from github.com)

Code Review

Nice step forward on the self-hosting dedupe work. The direction is right — removing the native Seq implementations of map/filter/fold/not and routing through the bootstrap closures is exactly what the design doc calls for. A few issues worth addressing before merging:

🐛 `eval-print` still uses `env-empty` (likely a bug)

src/eval.seq:5361

: eval-print ( String -- )
  parse env-empty eval-with-env

This word was not updated to use bootstrap-env. Any caller that goes through eval-print (test harness, perhaps?) won't have access to map, filter, fold, or not. Worth checking whether this path is exercised by tests.

⚠️ `bootstrap-env` is not actually cached

src/eval.seq:5409-5413

# Produce the bootstrap environment (cached at first call site)
: bootstrap-env ( -- Env )
  bootstrap-source tokenize parse-all
  env-empty bootstrap-eval-loop
;

The comment says "cached at first call site" but there is no caching — every call parses and evaluates the bootstrap source from scratch. It's called 5 times in repl.seq alone (lines 131, 285, 312, 369, 384) plus the eval.seq internal calls. In the REPL this means the bootstrap is re-evaluated on every file load and every piped-stdin invocation.

If caching was intentional (or even just desirable for performance), the bootstrap-env word needs to store the result somewhere global. If the current repeated-evaluation is acceptable, the comment is misleading and should be removed.

🔧 `lib/bootstrap.slisp` and the embedded string can drift

The readable file lib/bootstrap.slisp is never actually loaded by the interpreter — the embedded string in bootstrap-source is what's evaluated. They are currently in sync but there's nothing preventing them from diverging. This is exactly the open question noted in docs/design/self-hosting-phase2.md:

Should the bootstrap source move from a string literal in eval.slisp to a separate .slisp file loaded at init?

Two options:

Load the file at init — bootstrap-env reads lib/bootstrap.slisp at startup. Eliminates the duplicate. Adds a filesystem dependency at startup.
Delete lib/bootstrap.slisp — If the file is only documentation/reference and isn't going to be loaded, state that clearly in a comment and consider putting it in docs/ rather than lib/.

As-is, lib/bootstrap.slisp has a misleading header comment: "These functions are the single implementation — there are no native Seq versions. They are loaded at host startup..." — they are not loaded from this file. That comment will confuse future contributors.

📋 Step ordering vs. design doc

docs/design/self-hosting-phase2.md specifies:

Step 1 — TCO via trampoline ... Step 2 — Deduplicate ... "No new bootstrap functions until the self-hosted eval can handle deep tail recursion."

This PR implements Step 2 before Step 1. The map/filter/fold implementations in the bootstrap are all straightforwardly recursive. On a large list (e.g. (map f (range 100000))) they will stack overflow — the same regression the design doc flags. Is this intentional (i.e., TCO is tracked separately)? If so, noting it explicitly in the PR description or a code comment would help.

Minor: bootstrap error handling continues on failure

src/eval.seq:5396-5404

dup eval-err? if
  eval-err-message "Bootstrap error: " swap string.concat io.write-line
  scdr swap
  bootstrap-eval-loop   # continues despite error

If a bootstrap definition fails to parse or evaluate, the loop prints a message and continues. This means you can end up with an environment where map or not is silently missing. For a bootstrap where all definitions are statically known, it may be safer to halt (or at minimum exit non-zero) so the failure is visible during development.

Summary

Issue	Severity
`eval-print` not updated to `bootstrap-env`	Medium (likely bug)
`bootstrap-env` comment claims caching, no caching exists	Medium (misleading + perf)
`lib/bootstrap.slisp` comment says "loaded at startup" but it isn't	Medium (misleading)
Step 2 before Step 1 (TCO) — recursive bootstrap on large lists	Low/known risk
Bootstrap error continues instead of halting	Low

## Code Review Nice step forward on the self-hosting dedupe work. The direction is right — removing the native Seq implementations of `map`/`filter`/`fold`/`not` and routing through the bootstrap closures is exactly what the design doc calls for. A few issues worth addressing before merging: --- ### 🐛 `eval-print` still uses `env-empty` (likely a bug) **`src/eval.seq:5361`** ``` : eval-print ( String -- ) parse env-empty eval-with-env ``` This word was not updated to use `bootstrap-env`. Any caller that goes through `eval-print` (test harness, perhaps?) won't have access to `map`, `filter`, `fold`, or `not`. Worth checking whether this path is exercised by tests. --- ### ⚠️ `bootstrap-env` is not actually cached **`src/eval.seq:5409-5413`** ``` # Produce the bootstrap environment (cached at first call site) : bootstrap-env ( -- Env ) bootstrap-source tokenize parse-all env-empty bootstrap-eval-loop ; ``` The comment says "cached at first call site" but there is no caching — every call parses and evaluates the bootstrap source from scratch. It's called 5 times in `repl.seq` alone (lines 131, 285, 312, 369, 384) plus the `eval.seq` internal calls. In the REPL this means the bootstrap is re-evaluated on every file load and every piped-stdin invocation. If caching was intentional (or even just desirable for performance), the `bootstrap-env` word needs to store the result somewhere global. If the current repeated-evaluation is acceptable, the comment is misleading and should be removed. --- ### 🔧 `lib/bootstrap.slisp` and the embedded string can drift The readable file `lib/bootstrap.slisp` is never actually loaded by the interpreter — the embedded string in `bootstrap-source` is what's evaluated. They are currently in sync but there's nothing preventing them from diverging. This is exactly the open question noted in `docs/design/self-hosting-phase2.md`: > Should the bootstrap source move from a string literal in eval.slisp to a separate .slisp file loaded at init? Two options: 1. **Load the file at init** — `bootstrap-env` reads `lib/bootstrap.slisp` at startup. Eliminates the duplicate. Adds a filesystem dependency at startup. 2. **Delete `lib/bootstrap.slisp`** — If the file is only documentation/reference and isn't going to be loaded, state that clearly in a comment and consider putting it in `docs/` rather than `lib/`. As-is, `lib/bootstrap.slisp` has a misleading header comment: _"These functions are the single implementation — there are no native Seq versions. They are loaded at host startup..."_ — they are **not** loaded from this file. That comment will confuse future contributors. --- ### 📋 Step ordering vs. design doc `docs/design/self-hosting-phase2.md` specifies: > **Step 1 — TCO via trampoline** ... **Step 2 — Deduplicate** ... _"No new bootstrap functions until the self-hosted eval can handle deep tail recursion."_ This PR implements Step 2 before Step 1. The `map`/`filter`/`fold` implementations in the bootstrap are all straightforwardly recursive. On a large list (e.g. `(map f (range 100000))`) they will stack overflow — the same regression the design doc flags. Is this intentional (i.e., TCO is tracked separately)? If so, noting it explicitly in the PR description or a code comment would help. --- ### Minor: bootstrap error handling continues on failure **`src/eval.seq:5396-5404`** ``` dup eval-err? if eval-err-message "Bootstrap error: " swap string.concat io.write-line scdr swap bootstrap-eval-loop # continues despite error ``` If a bootstrap definition fails to parse or evaluate, the loop prints a message and continues. This means you can end up with an environment where `map` or `not` is silently missing. For a bootstrap where all definitions are statically known, it may be safer to halt (or at minimum exit non-zero) so the failure is visible during development. --- ### Summary | Issue | Severity | |---|---| | `eval-print` not updated to `bootstrap-env` | Medium (likely bug) | | `bootstrap-env` comment claims caching, no caching exists | Medium (misleading + perf) | | `lib/bootstrap.slisp` comment says "loaded at startup" but it isn't | Medium (misleading) | | Step 2 before Step 1 (TCO) — recursive bootstrap on large lists | Low/known risk | | Bootstrap error continues instead of halting | Low |

claude[bot] commented

2026-03-28 21:07:15 +00:00

(Migrated from github.com)

test comment - ignore

claude[bot] commented

2026-03-28 21:07:24 +00:00

(Migrated from github.com)

DISREGARD - this is a test comment that will be deleted

claude[bot] commented

2026-03-28 21:12:53 +00:00

(Migrated from github.com)

Code Review

Note: A prior automated review comment on this PR contained two factual errors — eval-print was updated (the diff confirms it), and the bootstrap-env comment correctly documents the re-evaluation behaviour. This review supersedes that one.

What this PR does well

Clean removal of native Seq implementations for map/filter/fold — the dispatch chain is simpler and the then count drops correctly from 15 to 12.
bootstrap-eval-loop calls os.exit on a fatal bootstrap error rather than silently continuing. That's the right call for a startup-time invariant.
bootstrap-env is threaded into all external entry points consistently (repl-loop, run-file, eval-all-print, eval-all-quiet, piped stdin).
The comment on apply staying native is clear and accurate.

Issues

1. Step ordering violates the design doc constraint (Medium)

docs/design/self-hosting-phase2.md is explicit:

TCO before expansion. No new bootstrap functions until the self-hosted eval can handle deep tail recursion.

The doc specifies Step 1 (TCO via trampoline) before Step 2 (deduplication). This PR implements Step 2 first. The bootstrap implementations of map, filter, and fold are all naively recursive. On a long list they will stack overflow — the exact regression the constraint was written to prevent.

Is this intentional (TCO tracked separately, regression accepted temporarily)? If so, a comment in lib/bootstrap.slisp and a note in the PR description would help future reviewers understand the tradeoff.

2. Design doc specifies renaming host `apply` to `host-apply` (Medium)

self-hosting-phase2.md, Step 2:

The host's internal apply stays in Seq but is renamed to __host-apply__ — an ugly internal name that is obviously not part of the language. The user-facing apply is the self-hosted version in the bootstrap.

This PR keeps the host apply in the dispatch chain under the user-facing name apply, and the bootstrap does not define a self-hosted apply. That's a conscious deviation from the spec — but the PR description acknowledges it. Worth a note in the design doc recording this decision, or an update to the spec if the approach has changed.

3. `lib/bootstrap.slisp` header comment is misleading (Low)

The file header says the functions "are loaded at host startup" but they are NOT loaded from this file. The embedded string in eval.seq:bootstrap-source is what actually runs. A future contributor will search for a file.slurp call and not find one.

Suggested fix: update the comment to say this file is documentation/reference only, and that the canonical source is the embedded string in eval.seq.

4. Silent drop of non-define results in `bootstrap-eval-loop` (Low)

If a bootstrap expression evaluates to a non-define, non-error value, the result is silently dropped. For a statically known bootstrap this is fine, but a short comment clarifying intent would help: "top-level non-define results are discarded — all bootstrap forms should be defines".

Summary

Issue	Severity
Step 2 before Step 1 (TCO) — recursive bootstrap risks stack overflow on large lists	Medium
Host `apply` not renamed to `__host-apply__` per design doc	Medium
`lib/bootstrap.slisp` says "loaded at startup" but it is not	Low
Non-define results in `bootstrap-eval-loop` silently dropped without comment	Low

The core mechanism is sound. The biggest risk before merging is clarifying the TCO ordering decision — either document that the regression is accepted, or gate this behind Step 1.

<h2>Code Review</h2> *Note: A prior automated review comment on this PR contained two factual errors — `eval-print` was updated (the diff confirms it), and the `bootstrap-env` comment correctly documents the re-evaluation behaviour. This review supersedes that one.* <h3>What this PR does well</h3> - Clean removal of native Seq implementations for `map`/`filter`/`fold` — the dispatch chain is simpler and the `then` count drops correctly from 15 to 12. - `bootstrap-eval-loop` calls `os.exit` on a fatal bootstrap error rather than silently continuing. That's the right call for a startup-time invariant. - `bootstrap-env` is threaded into all external entry points consistently (`repl-loop`, `run-file`, `eval-all-print`, `eval-all-quiet`, piped stdin). - The comment on `apply` staying native is clear and accurate. <h3>Issues</h3> <h4>1. Step ordering violates the design doc constraint (Medium)</h4> `docs/design/self-hosting-phase2.md` is explicit: > **TCO before expansion.** No new bootstrap functions until the self-hosted eval can handle deep tail recursion. The doc specifies Step 1 (TCO via trampoline) before Step 2 (deduplication). This PR implements Step 2 first. The bootstrap implementations of `map`, `filter`, and `fold` are all naively recursive. On a long list they will stack overflow — the exact regression the constraint was written to prevent. Is this intentional (TCO tracked separately, regression accepted temporarily)? If so, a comment in `lib/bootstrap.slisp` and a note in the PR description would help future reviewers understand the tradeoff. <h4>2. Design doc specifies renaming host `apply` to `__host-apply__` (Medium)</h4> `self-hosting-phase2.md`, Step 2: > The host's internal `apply` stays in Seq but is **renamed to `__host-apply__`** — an ugly internal name that is obviously not part of the language. The user-facing `apply` is the self-hosted version in the bootstrap. This PR keeps the host `apply` in the dispatch chain under the user-facing name `apply`, and the bootstrap does not define a self-hosted `apply`. That's a conscious deviation from the spec — but the PR description acknowledges it. Worth a note in the design doc recording this decision, or an update to the spec if the approach has changed. <h4>3. `lib/bootstrap.slisp` header comment is misleading (Low)</h4> The file header says the functions "are loaded at host startup" but they are NOT loaded from this file. The embedded string in `eval.seq:bootstrap-source` is what actually runs. A future contributor will search for a `file.slurp` call and not find one. Suggested fix: update the comment to say this file is documentation/reference only, and that the canonical source is the embedded string in `eval.seq`. <h4>4. Silent drop of non-define results in `bootstrap-eval-loop` (Low)</h4> If a bootstrap expression evaluates to a non-define, non-error value, the result is silently dropped. For a statically known bootstrap this is fine, but a short comment clarifying intent would help: "top-level non-define results are discarded — all bootstrap forms should be defines". <h3>Summary</h3> | Issue | Severity | |---|---| | Step 2 before Step 1 (TCO) — recursive bootstrap risks stack overflow on large lists | Medium | | Host `apply` not renamed to `__host-apply__` per design doc | Medium | | `lib/bootstrap.slisp` says "loaded at startup" but it is not | Low | | Non-define results in `bootstrap-eval-loop` silently dropped without comment | Low | The core mechanism is sound. The biggest risk before merging is clarifying the TCO ordering decision — either document that the regression is accepted, or gate this behind Step 1.

claude[bot] commented

2026-03-28 21:20:59 +00:00

(Migrated from github.com)

PR Review: Self-hosted bootstrap for map/filter/fold/not

Overall this is a clean, well-motivated change. The architecture is sound — removing native dispatch for the four self-hostable functions and bootstrapping them from embedded SeqLisp source is exactly the right direction. A few issues to address:

Bug: Inaccurate comment in `repl.seq`

# bootstrap-env is defined in eval.seq — it embeds the bootstrap source
# and produces an environment with map, filter, fold, apply, not.

apply is not in the bootstrap environment — it stays in the native dispatch chain. The comment should read map, filter, fold, not (no apply). Small thing, but it directly contradicts the design intent documented everywhere else.

Performance: `bootstrap-env` called on every `eval-print`

eval-print in eval.seq now calls bootstrap-env on every invocation:

: eval-print ( String -- )
  parse bootstrap-env eval-with-env
  ...

bootstrap-env tokenizes, parses, and evaluates four define forms each time. For the REPL and run-file this is a one-shot startup cost, but eval-print is called per-expression in test_eval.seq and anywhere that uses it as a convenience helper. This will add measurable overhead to the test suite.

A simple fix would be to not use bootstrap-env inside eval-print — instead, callers that want the bootstrap environment should pass one in. Or bootstrap-env could be memoized/cached at a higher level. The current architecture where it's re-created per call is the main rough edge here.

Maintenance risk: `lib/bootstrap.slisp` divergence

The file header says it's a reference copy and not loaded by the interpreter. This is a known fragile pattern — the embedded string in eval.seq and lib/bootstrap.slisp will inevitably drift. A few options ranked by robustness:

Best: Make lib/bootstrap.slisp the canonical source and have the build system embed it as a string (a just recipe that generates the string literal).
Good: Add a CI test that verifies the embedded string matches lib/bootstrap.slisp (strip comments, normalize whitespace, compare).
Acceptable (current): Keep it as-is, but add a comment to the justfile or a CI step reminding maintainers to sync both.

Option 1 eliminates the problem entirely and makes the bootstrap source as readable as the .slisp file.

Dead code after `os.exit` — comment clarity

The dead code in bootstrap-eval-loop after os.exit is correctly identified in the comment, but it might confuse future readers who wonder if the loop tail is actually reachable:

1 os.exit
# Dead code — stack shaping for compiler after os.exit
scdr swap bootstrap-eval-loop

Consider phrasing it as: # Unreachable — compiler requires consistent stack depth after os.exit. That's the conventional way this pattern is documented elsewhere in the codebase (e.g. # Never reaches here - stack shaping for compiler in repl.seq).

Test coverage

The existing tests/lisp/functions/higher_order.slisp covers map/filter/fold/apply behavior, which is good. But there's no test for the bootstrap mechanism itself — specifically:

That not works post-bootstrap (it's tested implicitly via if but not directly as (not #t) → #f)
That a malformed bootstrap source triggers os.exit 1 gracefully (hard to test, but worth noting the gap)

A minimal addition to higher_order.slisp or a new bootstrap.slisp test file with (assert-eq (not #t) #f) and (assert-eq (not #f) #t) would close this gap.

Minor: `check-candidate` for `not`

Good catch adding "not" check-candidate to the dispatch validator — this ensures the duplicate-implementation check covers not now that it's self-hosted. ✓

Summary

Severity	Issue
Bug	Comment in `repl.seq` lists `apply` as part of bootstrap output
Medium	`bootstrap-env` re-evaluated on every `eval-print` call
Medium	`lib/bootstrap.slisp` / embedded string will drift without tooling
Minor	Dead-code comment phrasing inconsistent with rest of codebase
Minor	No direct test for `not` via bootstrap

The core design is solid. Fix the comment bug and consider the eval-print performance issue before merging — the rest can be follow-on work.

## PR Review: Self-hosted bootstrap for map/filter/fold/not Overall this is a clean, well-motivated change. The architecture is sound — removing native dispatch for the four self-hostable functions and bootstrapping them from embedded SeqLisp source is exactly the right direction. A few issues to address: --- ### Bug: Inaccurate comment in `repl.seq` ```seq # bootstrap-env is defined in eval.seq — it embeds the bootstrap source # and produces an environment with map, filter, fold, apply, not. ``` `apply` is **not** in the bootstrap environment — it stays in the native dispatch chain. The comment should read `map, filter, fold, not` (no `apply`). Small thing, but it directly contradicts the design intent documented everywhere else. --- ### Performance: `bootstrap-env` called on every `eval-print` `eval-print` in `eval.seq` now calls `bootstrap-env` on every invocation: ```seq : eval-print ( String -- ) parse bootstrap-env eval-with-env ... ``` `bootstrap-env` tokenizes, parses, and evaluates four `define` forms each time. For the REPL and `run-file` this is a one-shot startup cost, but `eval-print` is called per-expression in `test_eval.seq` and anywhere that uses it as a convenience helper. This will add measurable overhead to the test suite. A simple fix would be to not use `bootstrap-env` inside `eval-print` — instead, callers that want the bootstrap environment should pass one in. Or `bootstrap-env` could be memoized/cached at a higher level. The current architecture where it's re-created per call is the main rough edge here. --- ### Maintenance risk: `lib/bootstrap.slisp` divergence The file header says it's a reference copy and not loaded by the interpreter. This is a known fragile pattern — the embedded string in `eval.seq` and `lib/bootstrap.slisp` will inevitably drift. A few options ranked by robustness: 1. **Best**: Make `lib/bootstrap.slisp` the canonical source and have the build system embed it as a string (a `just` recipe that generates the string literal). 2. **Good**: Add a CI test that verifies the embedded string matches `lib/bootstrap.slisp` (strip comments, normalize whitespace, compare). 3. **Acceptable** (current): Keep it as-is, but add a comment to the `justfile` or a CI step reminding maintainers to sync both. Option 1 eliminates the problem entirely and makes the bootstrap source as readable as the `.slisp` file. --- ### Dead code after `os.exit` — comment clarity The dead code in `bootstrap-eval-loop` after `os.exit` is correctly identified in the comment, but it might confuse future readers who wonder if the loop tail is actually reachable: ```seq 1 os.exit # Dead code — stack shaping for compiler after os.exit scdr swap bootstrap-eval-loop ``` Consider phrasing it as: `# Unreachable — compiler requires consistent stack depth after os.exit`. That's the conventional way this pattern is documented elsewhere in the codebase (e.g. `# Never reaches here - stack shaping for compiler` in `repl.seq`). --- ### Test coverage The existing `tests/lisp/functions/higher_order.slisp` covers `map`/`filter`/`fold`/`apply` behavior, which is good. But there's no test for the bootstrap mechanism itself — specifically: - That `not` works post-bootstrap (it's tested implicitly via `if` but not directly as `(not #t)` → `#f`) - That a malformed bootstrap source triggers `os.exit 1` gracefully (hard to test, but worth noting the gap) A minimal addition to `higher_order.slisp` or a new `bootstrap.slisp` test file with `(assert-eq (not #t) #f)` and `(assert-eq (not #f) #t)` would close this gap. --- ### Minor: `check-candidate` for `not` Good catch adding `"not" check-candidate` to the dispatch validator — this ensures the duplicate-implementation check covers `not` now that it's self-hosted. ✓ --- ### Summary | Severity | Issue | |----------|-------| | Bug | Comment in `repl.seq` lists `apply` as part of bootstrap output | | Medium | `bootstrap-env` re-evaluated on every `eval-print` call | | Medium | `lib/bootstrap.slisp` / embedded string will drift without tooling | | Minor | Dead-code comment phrasing inconsistent with rest of codebase | | Minor | No direct test for `not` via bootstrap | The core design is solid. Fix the comment bug and consider the `eval-print` performance issue before merging — the rest can be follow-on work.

claude[bot] commented

2026-03-28 21:33:31 +00:00

(Migrated from github.com)

PR Review: Self-Hosting Phase 2 — map/filter/fold/not Bootstrap

Overall this is clean, well-reasoned work. The self-hosting architecture is sound and the rationale for keeping apply native is well-documented. A few items worth considering:

Issues

1. eval-print calls bootstrap-env on every invocation

eval-print in src/eval.seq:5361 re-parses and re-evaluates the entire bootstrap source string every call. In production this is fine (startup only), but eval-print is used for every test case in tests/seq/test_eval.seq, so the test suite pays the bootstrap cost on each line. The commit message says "not called in hot loops" but the test harness contradicts that. Not a blocker, but worth considering whether eval-print should use env-empty (test assertions do not need map/filter/fold/not), or whether a cached bootstrap env would be worth adding later.

2. lib/bootstrap.slisp will drift from the embedded string

The new reference file duplicates the canonical source from src/eval.seq:5377-5383. The comment warns about this clearly, but there is no automated enforcement. Even a simple CI check (extract the embedded string and diff against the file) would prevent silent drift. A future editor who only reads the .slisp file may be confused when their edits have no effect.

3. eval (the compat wrapper) now silently uses bootstrap-env

src/eval.seq:680 — the word previously documented as "for backward compatibility" now uses bootstrap-env instead of env-empty. This is probably correct, but it is a subtle behavior change for any caller that expected a pristine environment. Worth adding a comment noting the intention ("callers now get map/filter/fold/not in scope").

Minor / Nits

bootstrap-eval-loop unreachable code comment: the post-os.exit path is a good pattern for satisfying the compiler stack-depth checker. The existing comment says "unreachable" but could also note that the stack shape intentionally matches the else branch so the compiler sees consistent depth on all paths.
not in check-candidate with no native dispatch: correct, since not is new to the language here. But a future reader might wonder if there is supposed to be a native not handler that got lost. A short comment next to that check-candidate entry ("self-hosted in bootstrap, no native dispatch") would help.
then count: 12 is correct after removing 3 branches (map/filter/fold). No issue.

What is good

Architecture is clean: exactly one implementation of each function, clearly located
The apply decision is well-reasoned and documented in both the design doc and the code
os.exit 1 on bootstrap failure is the right call — a silent partial bootstrap would cause mysterious downstream errors
TCO (PR 69) landing before this makes the recursive bootstrap-eval-loop safe for long lists
Existing test coverage in tests/lisp/functions/higher_order.slisp exercises all four self-hosted functions plus apply

Verdict: Ready to merge with the nits above as optional follow-ups. The lib/bootstrap.slisp drift risk is the one item worth revisiting before the file gets stale — perhaps a just check-bootstrap-sync target in the justfile.

## PR Review: Self-Hosting Phase 2 — map/filter/fold/not Bootstrap Overall this is clean, well-reasoned work. The self-hosting architecture is sound and the rationale for keeping `apply` native is well-documented. A few items worth considering: ### Issues **1. `eval-print` calls `bootstrap-env` on every invocation** `eval-print` in `src/eval.seq:5361` re-parses and re-evaluates the entire bootstrap source string every call. In production this is fine (startup only), but `eval-print` is used for every test case in `tests/seq/test_eval.seq`, so the test suite pays the bootstrap cost on each line. The commit message says "not called in hot loops" but the test harness contradicts that. Not a blocker, but worth considering whether `eval-print` should use `env-empty` (test assertions do not need `map`/`filter`/`fold`/`not`), or whether a cached bootstrap env would be worth adding later. **2. `lib/bootstrap.slisp` will drift from the embedded string** The new reference file duplicates the canonical source from `src/eval.seq:5377-5383`. The comment warns about this clearly, but there is no automated enforcement. Even a simple CI check (extract the embedded string and diff against the file) would prevent silent drift. A future editor who only reads the `.slisp` file may be confused when their edits have no effect. **3. `eval` (the compat wrapper) now silently uses `bootstrap-env`** `src/eval.seq:680` — the word previously documented as "for backward compatibility" now uses `bootstrap-env` instead of `env-empty`. This is probably correct, but it is a subtle behavior change for any caller that expected a pristine environment. Worth adding a comment noting the intention ("callers now get map/filter/fold/not in scope"). ### Minor / Nits - `bootstrap-eval-loop` unreachable code comment: the post-`os.exit` path is a good pattern for satisfying the compiler stack-depth checker. The existing comment says "unreachable" but could also note that the stack shape intentionally matches the `else` branch so the compiler sees consistent depth on all paths. - `not` in `check-candidate` with no native dispatch: correct, since `not` is new to the language here. But a future reader might wonder if there is supposed to be a native `not` handler that got lost. A short comment next to that `check-candidate` entry ("self-hosted in bootstrap, no native dispatch") would help. - `then` count: 12 is correct after removing 3 branches (map/filter/fold). No issue. ### What is good - Architecture is clean: exactly one implementation of each function, clearly located - The `apply` decision is well-reasoned and documented in both the design doc and the code - `os.exit 1` on bootstrap failure is the right call — a silent partial bootstrap would cause mysterious downstream errors - TCO (PR 69) landing before this makes the recursive `bootstrap-eval-loop` safe for long lists - Existing test coverage in `tests/lisp/functions/higher_order.slisp` exercises all four self-hosted functions plus `apply` **Verdict**: Ready to merge with the nits above as optional follow-ups. The `lib/bootstrap.slisp` drift risk is the one item worth revisiting before the file gets stale — perhaps a `just check-bootstrap-sync` target in the justfile.