From b79779e3bf20f7287fa91bb4d1adbc070258ad47 Mon Sep 17 00:00:00 2001 From: Greg Slepak Date: Mon, 2 Mar 2026 12:09:47 -0800 Subject: [PATCH 1/3] Add test suite, CI workflow, and technical documentation This commit introduces a comprehensive testing infrastructure for the Lwt branch, including: - Unit tests for parsers (JS, HTML, Pug, .strings) using ppx_inline_test - Integration tests for CLI extraction against fixture files - A GitHub Actions workflow for automated CI on Linux - AGENTS.md and ARCHITECTURE.md for system design and agent guidance - Build fixes for Linux (link flags and libgomp/libomp path search) - Testing section added to DEVELOPMENT.md Co-authored-by: Claude Opus 4.6 (via Crush) --- .github/workflows/test.yml | 48 +++++++++++++++++ .gitignore | 2 + AGENTS.md | 88 +++++++++++++++++++++++++++++++ ARCHITECTURE.md | 80 ++++++++++++++++++++++++++++ DEVELOPMENT.md | 11 ++++ dune | 2 +- src/cli/link_flags.linux.dev.dune | 1 + src/quickjs/dune | 22 +++++++- tests/dune | 54 +++++++++++++++++++ tests/fixtures/demo.html | 1 + tests/fixtures/demo.js | 1 + tests/fixtures/demo.pug | 1 + tests/fixtures/demo.vue | 13 +++++ tests/test_runner.ml | 71 +++++++++++++++++++++++++ 14 files changed, 392 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/test.yml create mode 100644 AGENTS.md create mode 100644 ARCHITECTURE.md create mode 100644 src/cli/link_flags.linux.dev.dune create mode 100644 tests/dune create mode 100644 tests/fixtures/demo.html create mode 100644 tests/fixtures/demo.js create mode 100644 tests/fixtures/demo.pug create mode 100644 tests/fixtures/demo.vue create mode 100644 tests/test_runner.ml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..8c2ef9f --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,48 @@ +name: Tests + +on: + push: + branches: [ lwt, test-suite ] + pull_request: + branches: [ lwt ] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up OCaml + uses: ocaml/setup-ocaml@v3 + with: + ocaml-compiler: 5.1.1 + dune-cache: true + opam-repositories: | + default: https://github.com/ocaml/opam-repository.git + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y npm xz-utils libomp-dev llvm-dev + opam install . --deps-only --update-invariant + npm install --no-save typescript browserify pug-lexer pug-parser pug-walk + + - name: Install QuickJS + run: | + curl https://bellard.org/quickjs/quickjs-2021-03-27.tar.xz > quickjs.tar.xz + tar xvf quickjs.tar.xz && rm quickjs.tar.xz + mv quickjs-2021-03-27 quickjs + cd quickjs && make + + - name: Install Flow + run: | + git clone --branch v0.183.1 --depth 1 https://github.com/facebook/flow.git flow + ln -s "$(pwd)/flow/src/parser" src/flow_parser + ln -s "$(pwd)/flow/src/third-party/sedlex" src/sedlex + ln -s "$(pwd)/flow/src/hack_forked/utils/collections" src/collections + + - name: Run tests + run: | + mkdir -p strings + opam exec -- dune runtest tests/ diff --git a/.gitignore b/.gitignore index c7b7725..f6881ea 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,5 @@ bad/ src/flow_parser src/sedlex src/collections + +tests/integration_test_run/ diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..5228f22 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,88 @@ +# Agent Information - String Extractor + +This repository contains an OCaml-based internationalization (i18n) string extraction tool. It parses source files (JS, TS, Vue, Pug, HTML) and extracts strings for translation management. + +## Documentation + +- **[ARCHITECTURE.md](ARCHITECTURE.md)**: Contains a deep-dive into the codebase layout, directory structure, and a comprehensive API reference. **Read this file first** when: + - Starting a new task to understand which files are relevant. + - Investigating the impact of changes across the system. + - Looking for specific functionality or function definitions before searching. + +- **[DEVELOPMENT.md](DEVELOPMENT.md)**: Contains instructions for environment setup, build processes for various platforms, and release workflows. **Read this file first** when: + - Setting up the development environment or installing dependencies (OCaml, JS, QuickJS). + - Building the project for development or release. + - Executing the tool for manual verification or testing. + - Managing version numbers or release artifacts. + +## Project Overview + +- **Language**: OCaml (5.1.1) with some C++ (QuickJS bridge) and JavaScript (parsers via Browserify). +- **Architecture**: + - `src/cli/`: Main entry point, command-line interface, and output generation logic. + - `src/parsing/`: OCaml parsers using `Angstrom` for custom formats and `Flow_parser` for JS. + - `src/quickjs/`: Bridge to QuickJS to run JavaScript-based parsers (TypeScript/Pug) from OCaml. + - `src/utils/`: Common utilities for collection, timing, and I/O. +- **Key Libraries**: `Core`, `Lwt` (concurrency), `Angstrom` (parsing), `Yojson`, `Ppx_jane`. + +## Essential Commands + +### Build +- **Development build**: `dune build src/cli/strings.exe` +- **Watch mode**: `dune build src/cli/strings.exe -w` +- **Release build (MacOS)**: `DUNE_PROFILE=release dune build src/cli/strings.exe` +- **Full release cycle**: See `DEVELOPMENT.md` for `cp`, `strip`, and Docker commands. + +### Run +- After building: `./_build/default/src/cli/strings.exe [directory-to-extract-from]` +- The CLI expects to be run from the root of a project containing a `strings/` directory (or it will create one if a `.git` folder is present). + +### Installation (Dev Setup) +Refer to `DEVELOPMENT.md` for specific `opam` and `npm` setup steps, as the project has several external dependencies (Flow, QuickJS, pug-lexer, etc.). + +## Code Conventions & Patterns + +### Parsing Strategy +1. **Direct Parsers**: Simple formats like `.strings`, `HTML`, and basic `Vue` tags are parsed using `Angstrom` in `src/parsing/`. +2. **JS/TS Parsing**: + - Javascript uses `Flow_parser` and a custom AST walker in `src/parsing/js_ast.ml`. + - TypeScript uses the official TS parser running inside QuickJS (`src/quickjs/`). +3. **Pug Parsing**: Has a "fast" OCaml implementation (`src/parsing/pug.ml`) and a "slow" official Pug implementation via QuickJS (`src/quickjs/`). + +### Extraction Pattern +- Content is extracted into a `Utils.Collector.t`. +- The collector tracks found strings, potential scripts (to be further parsed), and file errors. +- **Convention**: Strings found inside `L("...")` calls are treated as translations in JS/TS. + +### Concurrency +- Uses `Lwt` for cooperative concurrency. +- Parallel traversal of directories is handled in `src/cli/strings.ml` via `Lwt_list` and `Lwt_pool`. +- JS workers (QuickJS) are managed via `Lwt_pool` and `Lwt_preemptive` in `src/quickjs/quickjs.ml`. + +## Important Gotchas + +- **QuickJS Dependency**: Requires a compiled `quickjs` directory at the project root for building. `dune` rules in `src/quickjs/dune` copy headers and libraries from there. +- **Generated Headers**: `src/quickjs/runtime.h` is generated from `src/quickjs/parsers.js` using `browserify` and `qjsc`. +- **Linking**: MacOS builds use specific link flags (e.g., `ld64.lld`) defined in `src/cli/link_flags.*`. +- **OCamlFormat**: `.ocamlformat` is present; ensure you format OCaml code before submitting. +- **Memory Safety**: Be cautious with C++ FFI code in `src/quickjs/quickjs.cpp`, particularly regarding OCaml's GC interaction (`CAMLparam`, `CAMLreturn`, `caml_release_runtime_system`). + +## Testing Approach + +- **Inline Tests**: The project uses `ppx_inline_test`. Parsers in `src/parsing/` can be tested directly within the OCaml files or in the `tests/` directory. +- **Test Suite**: A standard test suite is located in `tests/test_runner.ml`. It covers JS, HTML, Pug, and `.strings` file parsing. +- **Integration Tests**: Verification can be performed by running the built binary against fixtures in `tests/fixtures/` and checking the generated output in the `strings/` directory. +- **Debug Flags**: Use `--show-debugging` or `--debug-pug` / `--debug-html` flags in the CLI to inspect internal parsing results. + +## Troubleshooting + +### "File modified since last read" +If you receive an error stating that a file has been **"modified since it was last read"**, it usually indicates a discrepancy between the file's filesystem timestamp and the internal state tracking. + +**Example Error:** +> `Edit failed: The file '/path/to/file' was modified since it was last read. Please read the file again before trying to edit it.` + +**Recommended Fix:** +1. Execute `touch filename` to reset the file's modification time to the current system time. +2. Re-read the file using the `view` tool. +3. Attempt the edit again. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 0000000..e42c901 --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,80 @@ +# Architecture Documentation - String Extractor + +This document provides a high-level overview of the String Extractor's architecture, directory structure, and internal APIs. + +## Project Entry Point + +The main entry point of the application is **`src/cli/strings.ml`**. It handles command-line argument parsing using `Core.Command`, sets up the `Lwt` runtime, and initiates the file traversal process. + +## Directory Structure + +```text +/ +├── src/ +│ ├── cli/ # Main CLI application logic +│ │ ├── strings.ml # CLI entry point, traversal coordination +│ │ ├── vue.ml # Vue-specific parsing and extraction logic +│ │ └── generate.ml # Localization file generation (.strings, .json) +│ ├── parsing/ # Core parsers using Angstrom and Flow +│ │ ├── basic.ml # Common parsing utilities and combinators +│ │ ├── js_ast.ml # Flow AST walker for string extraction +│ │ ├── js.ml # JavaScript string extraction entry point +│ │ ├── pug.ml # Native Pug template parsing +│ │ ├── html.ml # HTML template parsing +│ │ ├── strings.ml # .strings file parsing logic +│ │ └── ... # Other specialized parsers (vue blocks, styles) +│ ├── quickjs/ # Interface to QuickJS for JS/TS/Pug parsing +│ │ ├── quickjs.ml # OCaml FFI to QuickJS +│ │ ├── quickjs.cpp # C++ implementation of the bridge +│ │ └── parsers.js # JS-based parsers running in QuickJS +│ └── utils/ # Shared utility modules +│ ├── collector.ml # State container for collected strings/errors +│ ├── io.ml # I/O helpers +│ ├── timing.ml # Performance measurement +│ └── exception.ml # Exception handling +├── strings/ # Directory where .strings files are managed +├── dune-project # Dune build system configuration +└── README.md # Project overview and usage instructions +``` + +## Core API Reference + +### `src/cli/` +- **`Strings.main`**: Coordinates the entire run, including directory traversal and result generation. +- **`Vue.parse`**: Splits a `.vue` file into its constituent parts (template, script, style). +- **`Generate.write_english`**: Creates `english.strings` and `english.json` from the collected strings. +- **`Generate.write_other`**: Updates existing translations for other languages. + +### `src/parsing/` +- **`Parsing.Basic`**: Provides foundational Angstrom parsers for whitespace, strings, and standard error handling. +- **`Parsing.Js.extract_to_collector`**: Entry point for scanning JavaScript source code. +- **`Parsing.Js_ast.extract`**: A comprehensive walker for the Flow AST that identifies and extracts strings from `L("...")` calls. +- **`Parsing.Pug.collect`**: Traverses the native Pug AST to extract strings. +- **`Parsing.Strings.parse`**: Parses existing `.strings` files into a lookup table. Takes a `Lwt_io.input_channel` and returns a `string Core.String.Table.t Lwt.t`. + +### `src/quickjs/` +- **`Quickjs.extract_to_collector`**: Offloads extraction to QuickJS for TypeScript and advanced Pug templates. + +### `src/utils/` +- **`Utils.Collector.create`**: Initializes a new string collection state for a specific file. (type `t = { path: string; strings: string Queue.t; ... }`) +- **`Utils.Collector.blit_transfer`**: Merges results from one collector into another. + +## Control Flow +1. **Initiation**: `strings.exe` starts, parses CLI flags, and identifies the target directory. +2. **Traversal**: Uses `Lwt` to cooperatively walk the directory tree via `Lwt_list` and `Lwt_pool`. +3. **Dispatch**: For each supported file extension, the corresponding parser in `src/parsing` is invoked. +4. **Collection**: Parsers find strings (usually inside `L()`) and add them to a `Collector.t`. +5. **Generation**: `Generate.ml` aggregates strings from all collectors and updates the `strings/` directory. + +## Testing Setup + +The project implements a multi-layered testing strategy: + +1. **Inline Tests**: Using `ppx_inline_test`, logic can be tested directly within the source files. This is primarily used for parser validation in `src/parsing/`. +2. **Standard Test Suite**: Located in `tests/test_runner.ml`, this suite uses `ppx_expect` and `ppx_assert` to verify: + - JavaScript string extraction via `Flow_parser`. + - HTML and Pug extraction via `SZXX` and `Angstrom`. + - Apple-style `.strings` file parsing (via `Lwt_main.run` and `Lwt_io`). +3. **Integration Testing**: The `tests/fixtures/` directory contains sample files of all supported types. The CLI can be run against these fixtures to verify end-to-end extraction and output generation (`.strings` and `.json` files). + +The `tests/dune` file configures the test library and enables inline tests for the module. diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index e80341f..9c88817 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -68,3 +68,14 @@ docker run -it --rm \ ## apt-get update && apt-get install musl ## /app/strings.linux frontend ``` + +### Testing + +To run the automated tests and generate the translation files, first create the `strings/` directory at the project root, then run the tests. Ensure your opam environment is initialized: + +```sh +eval $(opam env) +mkdir -p strings && opam exec -- dune runtest tests/ +``` + +This command builds the project, executes the test suite, and populates the `strings/` directory with `english.strings` (extracted from fixtures) and merged `french.strings`. diff --git a/dune b/dune index 7d80e55..89f3c7b 100644 --- a/dune +++ b/dune @@ -1,4 +1,4 @@ -(data_only_dirs node_modules quickjs) +(data_only_dirs node_modules quickjs flow) (env (dev (flags (:standard -warn-error -A)) diff --git a/src/cli/link_flags.linux.dev.dune b/src/cli/link_flags.linux.dev.dune new file mode 100644 index 0000000..dd626a0 --- /dev/null +++ b/src/cli/link_flags.linux.dev.dune @@ -0,0 +1 @@ +() \ No newline at end of file diff --git a/src/quickjs/dune b/src/quickjs/dune index 32f8425..8940b59 100644 --- a/src/quickjs/dune +++ b/src/quickjs/dune @@ -55,8 +55,26 @@ (rule (targets libomp.a) (action (bash " - cp /opt/homebrew/Cellar/libomp/20.1.6/lib/libomp.a . &> /dev/null \ - || cp /usr/lib/libgomp.a libomp.a + OUT=\"libomp.a\" + PAWTHS=\" + /usr/local/Cellar/libomp/17.0.6/lib/libomp.a + /opt/homebrew/Cellar/libomp/20.1.6/lib/libomp.a + /usr/lib/x86_64-linux-gnu/libomp.a + /usr/lib/x86_64-linux-gnu/libgomp.a + /usr/lib/libgomp.a + /usr/lib/gcc/x86_64-linux-gnu/*/libgomp.a + /usr/lib/gcc/aarch64-redhat-linux/*/libgomp.a + \" + for p in $PAWTHS; do + for matched_path in $p; do + if [ -f \"$matched_path\" ]; then + cp \"$matched_path\" \"$OUT\" + exit 0 + fi + done + done + echo \"Error: Could not find libomp.a or libgomp.a\" >&2 + exit 1 ")) (mode standard) ) diff --git a/tests/dune b/tests/dune new file mode 100644 index 0000000..673c7e5 --- /dev/null +++ b/tests/dune @@ -0,0 +1,54 @@ +(library + (name parsing_tests) + (inline_tests) + (libraries parsing utils core lwt lwt.unix angstrom-lwt-unix) + (preprocess (pps ppx_jane ppx_inline_test)) +) + +(rule + (alias runtest) + (deps + ../src/cli/strings.exe + (source_tree fixtures)) + (action + (bash " + TMP_DIR=\"integration_test_run\" + rm -rf $TMP_DIR + mkdir -p $TMP_DIR/strings + mkdir -p $TMP_DIR/.git + printf '\"Hello from HTML\" = \"Bonjour de HTML\";\n' > $TMP_DIR/strings/french.strings + cp -r fixtures $TMP_DIR/ + cd $TMP_DIR + ../../src/cli/strings.exe fixtures --output strings &> /dev/null + cd .. + + if ! grep -q \"Bonjour de HTML\" $TMP_DIR/strings/french.strings; then + echo \"Error: French translation lost in .strings\" + exit 1 + fi + if ! grep -q \"Bonjour de HTML\" $TMP_DIR/strings/french.json; then + echo \"Error: French translation lost in .json\" + exit 1 + fi + if ! grep -q \"MISSING TRANSLATION - demo.pug\" $TMP_DIR/strings/french.strings; then + echo \"Error: Missing translation marker not found in .strings\" + exit 1 + fi + + echo \"✅ French integration test passed\" + rm -rf $TMP_DIR + + # Help user populate root strings/ if it exists + # We use absolute paths to ensure we hit the real source directory + # even if sandboxed in _build/default/tests + # The dune-project is used as a landmark for the root + # Traverse up to find the root of the source tree + # In dune, we are at _build/default/tests + ROOT_SRC=\"$(cd ../../.. && pwd)\" + if [ -d \"$ROOT_SRC/strings\" ]; then + # Extraction generates 5 strings. + # We pre-populate 3 translations. + printf '\"Hello from HTML\" = \"Bonjour de HTML\";\n\"Hello from JS\" = \"Bonjour de JS\";\n\"Hello from Pug\" = \"Bonjour de Pug\";\n' > \"$ROOT_SRC/strings/french.strings\" + ./../src/cli/strings.exe \"$ROOT_SRC/tests/fixtures\" --output \"$ROOT_SRC/strings\" + fi + "))) diff --git a/tests/fixtures/demo.html b/tests/fixtures/demo.html new file mode 100644 index 0000000..6c9bdfd --- /dev/null +++ b/tests/fixtures/demo.html @@ -0,0 +1 @@ +Hello from HTML diff --git a/tests/fixtures/demo.js b/tests/fixtures/demo.js new file mode 100644 index 0000000..6a9852c --- /dev/null +++ b/tests/fixtures/demo.js @@ -0,0 +1 @@ +L('Hello from JS'); diff --git a/tests/fixtures/demo.pug b/tests/fixtures/demo.pug new file mode 100644 index 0000000..e4b3a99 --- /dev/null +++ b/tests/fixtures/demo.pug @@ -0,0 +1 @@ +i18n Hello from Pug diff --git a/tests/fixtures/demo.vue b/tests/fixtures/demo.vue new file mode 100644 index 0000000..7f61388 --- /dev/null +++ b/tests/fixtures/demo.vue @@ -0,0 +1,13 @@ + + + diff --git a/tests/test_runner.ml b/tests/test_runner.ml new file mode 100644 index 0000000..f03d8d8 --- /dev/null +++ b/tests/test_runner.ml @@ -0,0 +1,71 @@ +open! Core +open Parsing + +let%test_unit "js_extraction_basic" = + let collector = Utils.Collector.create ~path:"test.js" in + let source = "L('Hello World'); L('Foo Bar');" in + Js.extract_to_collector collector source; + let strings = Queue.to_list collector.strings in + [%test_eq: string list] (List.sort strings ~compare:String.compare) (List.sort ["Hello World"; "Foo Bar"] ~compare:String.compare) + +let%test_unit "js_extraction_nested" = + let collector = Utils.Collector.create ~path:"test.js" in + let source = "function test() { if (true) { return L('Nested'); } }" in + Js.extract_to_collector collector source; + let strings = Queue.to_list collector.strings in + [%test_eq: string list] strings ["Nested"] + +let%test_unit "js_extraction_no_match" = + let collector = Utils.Collector.create ~path:"test.js" in + let source = "console.log('Hello');" in + Js.extract_to_collector collector source; + let strings = Queue.to_list collector.strings in + [%test_eq: string list] strings [] + +let%test_unit "strings_parsing" = + Lwt_main.run @@ ( + let path = "test.strings" in + let content = {| +/* Comment */ +"Hello" = "Bonjour"; +"World" = "Monde"; +|} in + let ic = Lwt_io.of_bytes ~mode:Lwt_io.input @@ Lwt_bytes.of_string content in + let open Lwt.Syntax in + let+ table = Strings.parse ~path ic in + [%test_eq: string option] (Hashtbl.find table "Hello") (Some "Bonjour"); + [%test_eq: string option] (Hashtbl.find table "World") (Some "Monde"); + [%test_eq: string option] (Hashtbl.find table "Missing") None) + +let%test_unit "french_strings_parsing" = + Lwt_main.run @@ ( + let path = "french.strings" in + let content = {| +/* Accented characters */ +"Logout" = "Déconnexion"; +"You and {count} others" = "Vous et {count} autres"; +"Settings" = "Paramètres"; +|} in + let ic = Lwt_io.of_bytes ~mode:Lwt_io.input @@ Lwt_bytes.of_string content in + let open Lwt.Syntax in + let+ table = Strings.parse ~path ic in + [%test_eq: string option] (Hashtbl.find table "Logout") (Some "Déconnexion"); + [%test_eq: string option] (Hashtbl.find table "You and {count} others") (Some "Vous et {count} autres"); + [%test_eq: string option] (Hashtbl.find table "Settings") (Some "Paramètres")) + +let%test_unit "html_extraction" = + let collector = Utils.Collector.create ~path:"test.html" in + let source = "Hello HTML" in + let on_ok parsed = Parsing.Html.collect collector parsed in + Parsing.Basic.exec_parser ~on_ok Parsing.Html.parser ~path:"test.html" ~language_name:"HTML" source; + let strings = Queue.to_list collector.strings in + [%test_eq: string list] strings ["Hello HTML"] + +let%test_unit "pug_extraction" = + let collector = Utils.Collector.create ~path:"test.pug" in + let source = "i18n Hello Pug" in + let string_parsers = Parsing.Basic.make_string_parsers () in + let on_ok parsed = Parsing.Pug.collect collector parsed in + Parsing.Basic.exec_parser ~on_ok (Parsing.Pug.parser string_parsers) ~path:"test.pug" ~language_name:"Pug" source; + let strings = Queue.to_list collector.strings in + [%test_eq: string list] strings ["Hello Pug"] From a1e1aff32490b66dbd101a3d2ac72e905b11f5da Mon Sep 17 00:00:00 2001 From: Greg Slepak Date: Mon, 2 Mar 2026 12:29:15 -0800 Subject: [PATCH 2/3] Address Copilot review comments on PR #7 - Rename PAWTHS to PATHS in src/quickjs/dune for readability - Use curl -fsSL in CI workflow for fail-fast downloads - Remove side-effecting block from tests/dune that clobbered strings/french.strings in the repo root on every test run - Fix DEVELOPMENT.md to accurately describe hermetic test behavior - Fix ARCHITECTURE.md test tooling description (ppx_inline_test, not ppx_expect; separate SZXX/Angstrom for HTML vs Pug) Co-authored-by: Claude Opus 4.6 (via Crush) --- .agents/tasks/feedback/KNOWLEDGE.md | 17 +++++++++ .agents/tasks/feedback/PLAN.md | 23 +++++++++++ .agents/tasks/feedback/STEP-1.md | 24 ++++++++++++ .agents/tasks/feedback/STEP-10.md | 26 +++++++++++++ .agents/tasks/feedback/STEP-2.md | 19 ++++++++++ .agents/tasks/feedback/STEP-3.md | 11 ++++++ .agents/tasks/feedback/STEP-4.md | 13 +++++++ .agents/tasks/feedback/STEP-5.md | 17 +++++++++ .agents/tasks/feedback/STEP-6.md | 12 ++++++ .agents/tasks/feedback/STEP-7.md | 13 +++++++ .agents/tasks/feedback/STEP-8.md | 12 ++++++ .agents/tasks/feedback/STEP-9.md | 59 +++++++++++++++++++++++++++++ .agents/tasks/feedback/TODOs.md | 10 +++++ .github/workflows/test.yml | 2 +- ARCHITECTURE.md | 6 +-- DEVELOPMENT.md | 6 +-- src/quickjs/dune | 4 +- tests/dune | 14 ------- 18 files changed, 265 insertions(+), 23 deletions(-) create mode 100644 .agents/tasks/feedback/KNOWLEDGE.md create mode 100644 .agents/tasks/feedback/PLAN.md create mode 100644 .agents/tasks/feedback/STEP-1.md create mode 100644 .agents/tasks/feedback/STEP-10.md create mode 100644 .agents/tasks/feedback/STEP-2.md create mode 100644 .agents/tasks/feedback/STEP-3.md create mode 100644 .agents/tasks/feedback/STEP-4.md create mode 100644 .agents/tasks/feedback/STEP-5.md create mode 100644 .agents/tasks/feedback/STEP-6.md create mode 100644 .agents/tasks/feedback/STEP-7.md create mode 100644 .agents/tasks/feedback/STEP-8.md create mode 100644 .agents/tasks/feedback/STEP-9.md create mode 100644 .agents/tasks/feedback/TODOs.md diff --git a/.agents/tasks/feedback/KNOWLEDGE.md b/.agents/tasks/feedback/KNOWLEDGE.md new file mode 100644 index 0000000..d34281e --- /dev/null +++ b/.agents/tasks/feedback/KNOWLEDGE.md @@ -0,0 +1,17 @@ +# Project Knowledge - Feedback Task + +## Key Gotchas + +- The `lwt` branch uses `Angstrom_lwt_unix.parse` which takes `Lwt_io.input_channel`, not strings or Eio flows. +- `Strings.parse` on lwt returns `string Core.String.Table.t Lwt.t` (wrapped in Lwt). +- The lwt `exec_parser_lwt` error handler signature is `~unparsed:string -> 'a option -> 'b Lwt.t` (different from Eio's). +- `exec_parser` (non-Lwt, non-Eio) works on raw strings and is synchronous — the JS/HTML/Pug tests use this and should work without changes on both branches. +- The test-suite branch was 6 commits ahead of master (Eio). Rebasing onto lwt was impractical due to heavy divergence — instead we reset the branch to `origin/lwt` and recreated all PR changes from scratch, adapted for Lwt. +- `ppx_inline_test` tests using `%test_unit` work fine with Lwt as long as we use `Lwt_main.run` inside the test body. +- `Lwt_io.of_bytes ~mode:Lwt_io.Input` can create an in-memory input channel from `Lwt_bytes.t` for testing `Strings.parse` without touching the filesystem. +- The lwt quickjs/dune keeps `lwt` and `lwt.unix` in library deps (the Eio port removed them). +- The lwt `src/cli/dune` uses `angstrom-lwt-unix`, `lwt`, `lwt.unix` (not `eio_main`, `angstrom-eio`). +- CI workflow branches should reference `lwt` (not `main` or `master`) since that's the active build branch. +- There was no `link_flags.linux.dev.dune` on the lwt branch — we created it with `()` (empty flags, same as the Eio PR did). +- The `default_error_handler` signature differs between branches: lwt uses `~unparsed -> 'a`, Eio uses `?unparsed ~msg -> unit -> 'a`. +- The `DEVELOPMENT.md` already exists on lwt and references lwt-compatible setup — no changes needed. diff --git a/.agents/tasks/feedback/PLAN.md b/.agents/tasks/feedback/PLAN.md new file mode 100644 index 0000000..4911597 --- /dev/null +++ b/.agents/tasks/feedback/PLAN.md @@ -0,0 +1,23 @@ +# Plan - Feedback Task + +## Goal + +Retarget PR #7 from `master` (Eio) to `lwt` branch, rewriting all Eio-specific code to use Lwt, and updating documentation accordingly. + +## Approach + +1. **Rebase the `test-suite` branch onto `origin/lwt`** instead of `origin/master`. This will bring the lwt source code as the base, and our PR additions (tests, CI, docs) on top. Conflicts are expected in files we modified that differ between Eio and Lwt — we'll resolve them to match lwt. + +2. **Rewrite `tests/test_runner.ml` for Lwt**: The `.strings` parsing tests use `Eio_posix.run` and `Eio.Flow.string_source`. These need to use `Lwt_main.run` and `Lwt_io` instead. JS/HTML/Pug tests that don't use Eio should be unaffected. + +3. **Update `tests/dune`**: Replace `eio_main` with `lwt`, `lwt.unix`, and `angstrom-lwt-unix` in the test library dependencies. + +4. **Update `AGENTS.md`**: Replace all Eio references with Lwt equivalents. + +5. **Update `ARCHITECTURE.md`**: Replace all Eio references with Lwt equivalents. + +6. **Update CI workflow** if needed: Branch triggers should target `lwt` instead of `main`/`master`. + +7. **Change PR base branch** from `master` to `lwt` using `gh pr edit`. + +8. **Verify**: Ensure tests pass or at least the code compiles correctly for the lwt branch. diff --git a/.agents/tasks/feedback/STEP-1.md b/.agents/tasks/feedback/STEP-1.md new file mode 100644 index 0000000..edc15db --- /dev/null +++ b/.agents/tasks/feedback/STEP-1.md @@ -0,0 +1,24 @@ +# Rebase test-suite onto origin/lwt and recreate PR changes + +Status: COMPLETED + +## Sub tasks + +1. [x] Reset `test-suite` branch to `origin/lwt` base +2. [x] Recreate all PR file additions adapted for Lwt + +## NOTES + +Instead of rebasing (which would produce many conflicts in files we didn't change due to heavy divergence between master/Eio and lwt), we reset the branch to `origin/lwt` and recreated all PR changes from scratch: + +- `dune` — added `flow` to `data_only_dirs` +- `.gitignore` — added `tests/integration_test_run/` +- `src/cli/link_flags.linux.dev.dune` — created with `()` (empty flags) +- `src/quickjs/dune` — replaced fragile libomp.a cp chain with robust path search loop +- `tests/fixtures/` — created demo.html, demo.js, demo.pug, demo.vue +- `tests/dune` — created with lwt/lwt.unix/angstrom-lwt-unix deps (instead of eio_main) +- `tests/test_runner.ml` — rewritten for Lwt (see STEP-2) +- `.github/workflows/test.yml` — targets `lwt` branch instead of `main` +- `AGENTS.md` — created, references Lwt (not Eio) +- `ARCHITECTURE.md` — created, references Lwt (not Eio) +- `DEVELOPMENT.md` — added Testing section diff --git a/.agents/tasks/feedback/STEP-10.md b/.agents/tasks/feedback/STEP-10.md new file mode 100644 index 0000000..0827d42 --- /dev/null +++ b/.agents/tasks/feedback/STEP-10.md @@ -0,0 +1,26 @@ +# Form a list of the good comments and resolve them + +Status: COMPLETED + +## Valid Comments Resolved + +| # | File | Issue | Resolution | +|---|------|-------|------------| +| 1 | `src/quickjs/dune:59` | `PAWTHS` typo | Renamed to `PATHS` | +| 2 | `.github/workflows/test.yml:33` | `curl` without fail-fast flags | Changed to `curl -fsSL ... -o quickjs.tar.xz` | +| 3 | `tests/dune:41-53` | Side-effecting block clobbers `strings/french.strings` in repo root | Removed the entire "populate root strings/" block | +| 4 | `DEVELOPMENT.md:72-81` | Claims `dune runtest` populates `strings/` dir | Rewritten to accurately describe hermetic test behavior | +| 5 | `ARCHITECTURE.md:73-76` | Says `ppx_expect`; conflates SZXX+Angstrom for Pug | Fixed to `ppx_inline_test` + `ppx_assert`; separated SZXX (HTML) from Angstrom (Pug) | + +## Sub tasks + +1. [x] Implement fix 1: Rename `PAWTHS` → `PATHS` in `src/quickjs/dune` +2. [x] Implement fix 2: Use `curl -fsSL` in `.github/workflows/test.yml` +3. [x] Implement fix 3: Remove side-effecting block from `tests/dune` +4. [x] Implement fix 4: Update `DEVELOPMENT.md` testing section +5. [x] Implement fix 5: Fix `ARCHITECTURE.md` test tooling description +6. [x] Run tests — all pass + +## NOTES + +All 5 Copilot comments were valid and resolved. Tests pass after changes. The most substantive fix was removing the side-effecting block from `tests/dune` that would clobber `strings/french.strings` on every test run. diff --git a/.agents/tasks/feedback/STEP-2.md b/.agents/tasks/feedback/STEP-2.md new file mode 100644 index 0000000..7e0757c --- /dev/null +++ b/.agents/tasks/feedback/STEP-2.md @@ -0,0 +1,19 @@ +# Rewrite test_runner.ml for Lwt + +Status: COMPLETED + +## Sub tasks + +1. [x] Replace `Eio_posix.run` with `Lwt_main.run` +2. [x] Replace `Eio.Flow.string_source` with `Lwt_io.of_bytes ~mode:Lwt_io.input` +3. [x] Use `Lwt.Syntax` `let+` for Lwt promise handling +4. [x] Keep JS/HTML/Pug tests unchanged (they use synchronous `exec_parser`) + +## NOTES + +Key changes in test_runner.ml: +- `strings_parsing` and `french_strings_parsing` tests now use `Lwt_main.run` instead of `Eio_posix.run` +- Created `Lwt_io` input channels from in-memory byte strings using `Lwt_io.of_bytes ~mode:Lwt_io.input @@ Lwt_bytes.of_string content` +- Used `let+` from `Lwt.Syntax` since `Strings.parse` returns `Lwt.t` +- JS, HTML, and Pug tests are unchanged as they use the synchronous `Basic.exec_parser` +- Initially wrote `Lwt_io.Input` (uppercase) — corrected to `Lwt_io.input` (lowercase value) after verifying against Lwt's own test suite via Sourcegraph diff --git a/.agents/tasks/feedback/STEP-3.md b/.agents/tasks/feedback/STEP-3.md new file mode 100644 index 0000000..8b64abd --- /dev/null +++ b/.agents/tasks/feedback/STEP-3.md @@ -0,0 +1,11 @@ +# Update tests/dune dependencies (eio_main → lwt) + +Status: COMPLETED + +## Sub tasks + +1. [x] Replace `eio_main` with `lwt lwt.unix angstrom-lwt-unix` in library deps + +## NOTES + +The test library in `tests/dune` was created with Lwt dependencies from the start (since we recreated all files from scratch on the lwt base in STEP-1). Final deps: `parsing utils core lwt lwt.unix angstrom-lwt-unix`. diff --git a/.agents/tasks/feedback/STEP-4.md b/.agents/tasks/feedback/STEP-4.md new file mode 100644 index 0000000..8e297cd --- /dev/null +++ b/.agents/tasks/feedback/STEP-4.md @@ -0,0 +1,13 @@ +# Update AGENTS.md to reference Lwt instead of Eio + +Status: COMPLETED + +## Sub tasks + +1. [x] Replace "Eio (concurrency)" with "Lwt (concurrency)" in Key Libraries +2. [x] Replace Eio concurrency description with Lwt (`Lwt_list`, `Lwt_pool`, `Lwt_preemptive`) +3. [x] Remove `Fiber.List.iter` / `Eio.Executor_pool` references + +## NOTES + +Created AGENTS.md from scratch on the lwt base. All references use Lwt terminology: `Lwt_pool`, `Lwt_list`, `Lwt_preemptive`. diff --git a/.agents/tasks/feedback/STEP-5.md b/.agents/tasks/feedback/STEP-5.md new file mode 100644 index 0000000..d2d888a --- /dev/null +++ b/.agents/tasks/feedback/STEP-5.md @@ -0,0 +1,17 @@ +# Update ARCHITECTURE.md to reference Lwt instead of Eio + +Status: COMPLETED + +## Sub tasks + +1. [x] Replace "Eio runtime" with "Lwt runtime" in Project Entry Point +2. [x] Update Control Flow section (Eio → Lwt, `Lwt_list` and `Lwt_pool`) +3. [x] Update `Parsing.Strings.parse` API description to mention `Lwt_io.input_channel` +4. [x] Update Testing Setup section to reference `Lwt_main.run` and `Lwt_io` + +## NOTES + +Created ARCHITECTURE.md from scratch on the lwt base. Key differences from Eio version: +- Control flow references `Lwt` cooperative concurrency via `Lwt_list` and `Lwt_pool` +- `Strings.parse` documented as taking `Lwt_io.input_channel` returning `Lwt.t` +- Testing section notes `.strings` tests use `Lwt_main.run` diff --git a/.agents/tasks/feedback/STEP-6.md b/.agents/tasks/feedback/STEP-6.md new file mode 100644 index 0000000..ef8e2a8 --- /dev/null +++ b/.agents/tasks/feedback/STEP-6.md @@ -0,0 +1,12 @@ +# Update CI workflow for lwt dependencies + +Status: COMPLETED + +## Sub tasks + +1. [x] Change branch triggers from `main`/`master` to `lwt` +2. [x] Keep opam deps the same (lwt branch opam already has correct deps) + +## NOTES + +CI workflow `.github/workflows/test.yml` created with `lwt` and `test-suite` as push branches, and `lwt` as PR branch. The opam install step uses `--update-invariant` for flambda compatibility. diff --git a/.agents/tasks/feedback/STEP-7.md b/.agents/tasks/feedback/STEP-7.md new file mode 100644 index 0000000..0d2dbbb --- /dev/null +++ b/.agents/tasks/feedback/STEP-7.md @@ -0,0 +1,13 @@ +# Change PR #7 base branch from master to lwt + +Status: COMPLETED + +## Sub tasks + +1. [x] Run `gh pr edit 7 --base lwt` +2. [x] Update PR body to reference Lwt instead of Eio + +## NOTES + +- Changed base from `master` to `lwt` using `gh pr edit 7 --base lwt` +- Updated PR description to reference Lwt-specific testing (Lwt_main.run, Lwt_io) diff --git a/.agents/tasks/feedback/STEP-8.md b/.agents/tasks/feedback/STEP-8.md new file mode 100644 index 0000000..5afdd5e --- /dev/null +++ b/.agents/tasks/feedback/STEP-8.md @@ -0,0 +1,12 @@ +# Verify build/tests + +Status: COMPLETED + +## Sub tasks + +1. [x] Initialize opam environment locally +2. [x] Run `dune runtest tests/` to verify tests pass + +## NOTES + +User ran `opam install . --deps-only -t --update-invariant` and then `dune runtest tests/` — all tests passed locally. diff --git a/.agents/tasks/feedback/STEP-9.md b/.agents/tasks/feedback/STEP-9.md new file mode 100644 index 0000000..e64a181 --- /dev/null +++ b/.agents/tasks/feedback/STEP-9.md @@ -0,0 +1,59 @@ +# Review latest Copilot comments on PR #7 and evaluate each one + +Status: COMPLETED + +## Sub tasks + +1. [x] Fetch all review comments from PR #7 (Copilot + Devin) +2. [x] Evaluate each Copilot comment individually +3. [x] Evaluate Devin comments for completeness + +## Copilot Review Comments (5 comments from review 3878489572) + +### Comment 1 — `src/quickjs/dune`: Rename `PAWTHS` to `PATHS` +**Verdict: VALID (minor)** +The variable `PAWTHS` is indeed a typo/odd spelling of `PATHS`. Renaming improves readability. Trivial fix. + +### Comment 2 — `.github/workflows/test.yml`: Use `curl -fsSL` for QuickJS download +**Verdict: VALID (good practice)** +`curl URL > file` can silently succeed with an error page (e.g. 404 HTML body) and only fail during `tar`. Using `curl -fsSL ... -o quickjs.tar.xz` is more robust. Worth fixing. + +### Comment 3 — `tests/dune`: Remove the "populate root strings/" block +**Verdict: VALID (substantive)** +This block tries to mutate the working tree from a test rule. Copilot claims `../../..` from `_build/default/tests` resolves to `_build` not repo root — but actually `_build/default/tests/../../../` = repo root. However, the broader point is correct: **tests should not mutate the working tree**. The block clobbers `strings/french.strings` on every `dune runtest` if a `strings/` dir exists. This is a real side-effect concern. Devin also flagged this (and then self-resolved as false positive on the path computation, but the data-loss concern stands). We should remove this block and update DEVELOPMENT.md accordingly. + +### Comment 4 — `DEVELOPMENT.md`: Documentation mismatch about `strings/` population +**Verdict: VALID (follows from Comment 3)** +DEVELOPMENT.md line 81 claims `dune runtest tests/` "populates the `strings/` directory with `english.strings` ... and merged `french.strings`." If we remove the side-effecting block per Comment 3, this documentation is wrong. Needs updating either way — the current docs describe behavior that is a test side-effect we're removing. + +### Comment 5 — `ARCHITECTURE.md`: Inaccurate test tooling description +**Verdict: VALID (documentation accuracy)** +Line 74 says the test suite uses `ppx_expect` — but it doesn't. It uses `ppx_inline_test` + `ppx_assert`. Also "HTML and Pug extraction via `SZXX` and `Angstrom`" conflates SZXX (HTML) with Pug (Angstrom only). The suggested text is accurate. + +## Devin Review Comments (evaluated for completeness) + +### Devin Comment (review 1) — `src/quickjs/dune`: libomp.a fallback chain missing Ubuntu x86_64 +**Verdict: ALREADY RESOLVED** — The current code (as of latest commits) already includes `/usr/lib/x86_64-linux-gnu/libgomp.a` and glob patterns. The fallback chain was reworked. + +### Devin Comment (review 2) — `src/quickjs/dune`: `%{project_root}/../../` paths +**Verdict: FALSE POSITIVE (self-resolved by Devin)** — `%{project_root}` in dune resolves to `_build/default`, not repo root. So `../../` correctly navigates to repo root. Standard dune pattern. + +### Devin Comment (review 3) — `tests/dune`: Data loss from writing to repo root +**Verdict: VALID (same as Copilot Comment 3)** — Already addressed above. + +### Devin Comment (review 3, part 2) — `src/quickjs/dune`: Wildcard glob can match multiple files +**Verdict: ALREADY RESOLVED** — Current code uses a nested loop with `for matched_path in $p` and picks the first match with `exit 0`, handling glob expansion correctly. + +## Summary of Valid Comments Worth Resolving + +| # | File | Issue | Severity | +|---|------|-------|----------| +| 1 | `src/quickjs/dune` | Rename `PAWTHS` → `PATHS` | Minor | +| 2 | `.github/workflows/test.yml` | Use `curl -fsSL ... -o` | Minor | +| 3 | `tests/dune` | Remove side-effecting "populate root strings/" block | Substantive | +| 4 | `DEVELOPMENT.md` | Fix docs about `strings/` population | Documentation | +| 5 | `ARCHITECTURE.md` | Fix test tooling description (ppx_expect → ppx_inline_test, SZXX/Pug) | Documentation | + +## NOTES + +All 5 Copilot comments are valid and worth resolving. Comments 3 and 4 are related (removing side-effect + updating docs). None of the Devin comments need new action — they were either already resolved in prior commits or are false positives. diff --git a/.agents/tasks/feedback/TODOs.md b/.agents/tasks/feedback/TODOs.md new file mode 100644 index 0000000..a79a1bf --- /dev/null +++ b/.agents/tasks/feedback/TODOs.md @@ -0,0 +1,10 @@ +1. [x] Rebase `test-suite` branch onto `origin/lwt` and resolve conflicts +2. [x] Rewrite `tests/test_runner.ml` for Lwt +3. [x] Update `tests/dune` dependencies (eio_main → lwt) +4. [x] Update `AGENTS.md` to reference Lwt instead of Eio +5. [x] Update `ARCHITECTURE.md` to reference Lwt instead of Eio +6. [x] Update CI workflow (`.github/workflows/test.yml`) for lwt dependencies +7. [x] Change PR #7 base branch from `master` to `lwt` +8. [x] Verify build/tests +9. [x] Review latest Copilot comments on PR #7 and evaluate each one +10. [x] Form a list of the good comments and resolve them diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8c2ef9f..5b6932c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -30,7 +30,7 @@ jobs: - name: Install QuickJS run: | - curl https://bellard.org/quickjs/quickjs-2021-03-27.tar.xz > quickjs.tar.xz + curl -fsSL https://bellard.org/quickjs/quickjs-2021-03-27.tar.xz -o quickjs.tar.xz tar xvf quickjs.tar.xz && rm quickjs.tar.xz mv quickjs-2021-03-27 quickjs cd quickjs && make diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index e42c901..a7f95da 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -70,10 +70,10 @@ The main entry point of the application is **`src/cli/strings.ml`**. It handles The project implements a multi-layered testing strategy: -1. **Inline Tests**: Using `ppx_inline_test`, logic can be tested directly within the source files. This is primarily used for parser validation in `src/parsing/`. -2. **Standard Test Suite**: Located in `tests/test_runner.ml`, this suite uses `ppx_expect` and `ppx_assert` to verify: +1. **Inline Tests**: Using `ppx_inline_test` (e.g. `let%test_unit`) together with `ppx_assert` (e.g. `[%test_eq]`), logic can be tested directly within the source files. This is primarily used for parser validation in `src/parsing/`. +2. **Standard Test Suite**: Located in `tests/test_runner.ml`, this suite runs the inline tests via `ppx_inline_test` and uses `ppx_assert` to verify: - JavaScript string extraction via `Flow_parser`. - - HTML and Pug extraction via `SZXX` and `Angstrom`. + - HTML extraction via `SZXX` and Pug extraction via `Angstrom`. - Apple-style `.strings` file parsing (via `Lwt_main.run` and `Lwt_io`). 3. **Integration Testing**: The `tests/fixtures/` directory contains sample files of all supported types. The CLI can be run against these fixtures to verify end-to-end extraction and output generation (`.strings` and `.json` files). diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 9c88817..bca2660 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -71,11 +71,11 @@ docker run -it --rm \ ### Testing -To run the automated tests and generate the translation files, first create the `strings/` directory at the project root, then run the tests. Ensure your opam environment is initialized: +To run the automated tests, ensure your opam environment is initialized: ```sh eval $(opam env) -mkdir -p strings && opam exec -- dune runtest tests/ +dune runtest tests/ ``` -This command builds the project, executes the test suite, and populates the `strings/` directory with `english.strings` (extracted from fixtures) and merged `french.strings`. +This builds the project, runs the inline unit tests, and executes the integration test (which verifies extraction and translation preservation in an isolated temporary directory). diff --git a/src/quickjs/dune b/src/quickjs/dune index 8940b59..228298b 100644 --- a/src/quickjs/dune +++ b/src/quickjs/dune @@ -56,7 +56,7 @@ (targets libomp.a) (action (bash " OUT=\"libomp.a\" - PAWTHS=\" + PATHS=\" /usr/local/Cellar/libomp/17.0.6/lib/libomp.a /opt/homebrew/Cellar/libomp/20.1.6/lib/libomp.a /usr/lib/x86_64-linux-gnu/libomp.a @@ -65,7 +65,7 @@ /usr/lib/gcc/x86_64-linux-gnu/*/libgomp.a /usr/lib/gcc/aarch64-redhat-linux/*/libgomp.a \" - for p in $PAWTHS; do + for p in $PATHS; do for matched_path in $p; do if [ -f \"$matched_path\" ]; then cp \"$matched_path\" \"$OUT\" diff --git a/tests/dune b/tests/dune index 673c7e5..acdb36a 100644 --- a/tests/dune +++ b/tests/dune @@ -37,18 +37,4 @@ echo \"✅ French integration test passed\" rm -rf $TMP_DIR - - # Help user populate root strings/ if it exists - # We use absolute paths to ensure we hit the real source directory - # even if sandboxed in _build/default/tests - # The dune-project is used as a landmark for the root - # Traverse up to find the root of the source tree - # In dune, we are at _build/default/tests - ROOT_SRC=\"$(cd ../../.. && pwd)\" - if [ -d \"$ROOT_SRC/strings\" ]; then - # Extraction generates 5 strings. - # We pre-populate 3 translations. - printf '\"Hello from HTML\" = \"Bonjour de HTML\";\n\"Hello from JS\" = \"Bonjour de JS\";\n\"Hello from Pug\" = \"Bonjour de Pug\";\n' > \"$ROOT_SRC/strings/french.strings\" - ./../src/cli/strings.exe \"$ROOT_SRC/tests/fixtures\" --output \"$ROOT_SRC/strings\" - fi "))) From ef298bdc181ab8ffe44f18d7bc3e92ba5a3cc6be Mon Sep 17 00:00:00 2001 From: Greg Slepak Date: Mon, 2 Mar 2026 12:32:44 -0800 Subject: [PATCH 3/3] Remove .agents from tracking and add .agents/, ignored/ to .gitignore Co-authored-by: Claude Opus 4.6 (via Crush) --- .agents/tasks/feedback/KNOWLEDGE.md | 17 --------- .agents/tasks/feedback/PLAN.md | 23 ----------- .agents/tasks/feedback/STEP-1.md | 24 ------------ .agents/tasks/feedback/STEP-10.md | 26 ------------- .agents/tasks/feedback/STEP-2.md | 19 ---------- .agents/tasks/feedback/STEP-3.md | 11 ------ .agents/tasks/feedback/STEP-4.md | 13 ------- .agents/tasks/feedback/STEP-5.md | 17 --------- .agents/tasks/feedback/STEP-6.md | 12 ------ .agents/tasks/feedback/STEP-7.md | 13 ------- .agents/tasks/feedback/STEP-8.md | 12 ------ .agents/tasks/feedback/STEP-9.md | 59 ----------------------------- .agents/tasks/feedback/TODOs.md | 10 ----- .gitignore | 2 + 14 files changed, 2 insertions(+), 256 deletions(-) delete mode 100644 .agents/tasks/feedback/KNOWLEDGE.md delete mode 100644 .agents/tasks/feedback/PLAN.md delete mode 100644 .agents/tasks/feedback/STEP-1.md delete mode 100644 .agents/tasks/feedback/STEP-10.md delete mode 100644 .agents/tasks/feedback/STEP-2.md delete mode 100644 .agents/tasks/feedback/STEP-3.md delete mode 100644 .agents/tasks/feedback/STEP-4.md delete mode 100644 .agents/tasks/feedback/STEP-5.md delete mode 100644 .agents/tasks/feedback/STEP-6.md delete mode 100644 .agents/tasks/feedback/STEP-7.md delete mode 100644 .agents/tasks/feedback/STEP-8.md delete mode 100644 .agents/tasks/feedback/STEP-9.md delete mode 100644 .agents/tasks/feedback/TODOs.md diff --git a/.agents/tasks/feedback/KNOWLEDGE.md b/.agents/tasks/feedback/KNOWLEDGE.md deleted file mode 100644 index d34281e..0000000 --- a/.agents/tasks/feedback/KNOWLEDGE.md +++ /dev/null @@ -1,17 +0,0 @@ -# Project Knowledge - Feedback Task - -## Key Gotchas - -- The `lwt` branch uses `Angstrom_lwt_unix.parse` which takes `Lwt_io.input_channel`, not strings or Eio flows. -- `Strings.parse` on lwt returns `string Core.String.Table.t Lwt.t` (wrapped in Lwt). -- The lwt `exec_parser_lwt` error handler signature is `~unparsed:string -> 'a option -> 'b Lwt.t` (different from Eio's). -- `exec_parser` (non-Lwt, non-Eio) works on raw strings and is synchronous — the JS/HTML/Pug tests use this and should work without changes on both branches. -- The test-suite branch was 6 commits ahead of master (Eio). Rebasing onto lwt was impractical due to heavy divergence — instead we reset the branch to `origin/lwt` and recreated all PR changes from scratch, adapted for Lwt. -- `ppx_inline_test` tests using `%test_unit` work fine with Lwt as long as we use `Lwt_main.run` inside the test body. -- `Lwt_io.of_bytes ~mode:Lwt_io.Input` can create an in-memory input channel from `Lwt_bytes.t` for testing `Strings.parse` without touching the filesystem. -- The lwt quickjs/dune keeps `lwt` and `lwt.unix` in library deps (the Eio port removed them). -- The lwt `src/cli/dune` uses `angstrom-lwt-unix`, `lwt`, `lwt.unix` (not `eio_main`, `angstrom-eio`). -- CI workflow branches should reference `lwt` (not `main` or `master`) since that's the active build branch. -- There was no `link_flags.linux.dev.dune` on the lwt branch — we created it with `()` (empty flags, same as the Eio PR did). -- The `default_error_handler` signature differs between branches: lwt uses `~unparsed -> 'a`, Eio uses `?unparsed ~msg -> unit -> 'a`. -- The `DEVELOPMENT.md` already exists on lwt and references lwt-compatible setup — no changes needed. diff --git a/.agents/tasks/feedback/PLAN.md b/.agents/tasks/feedback/PLAN.md deleted file mode 100644 index 4911597..0000000 --- a/.agents/tasks/feedback/PLAN.md +++ /dev/null @@ -1,23 +0,0 @@ -# Plan - Feedback Task - -## Goal - -Retarget PR #7 from `master` (Eio) to `lwt` branch, rewriting all Eio-specific code to use Lwt, and updating documentation accordingly. - -## Approach - -1. **Rebase the `test-suite` branch onto `origin/lwt`** instead of `origin/master`. This will bring the lwt source code as the base, and our PR additions (tests, CI, docs) on top. Conflicts are expected in files we modified that differ between Eio and Lwt — we'll resolve them to match lwt. - -2. **Rewrite `tests/test_runner.ml` for Lwt**: The `.strings` parsing tests use `Eio_posix.run` and `Eio.Flow.string_source`. These need to use `Lwt_main.run` and `Lwt_io` instead. JS/HTML/Pug tests that don't use Eio should be unaffected. - -3. **Update `tests/dune`**: Replace `eio_main` with `lwt`, `lwt.unix`, and `angstrom-lwt-unix` in the test library dependencies. - -4. **Update `AGENTS.md`**: Replace all Eio references with Lwt equivalents. - -5. **Update `ARCHITECTURE.md`**: Replace all Eio references with Lwt equivalents. - -6. **Update CI workflow** if needed: Branch triggers should target `lwt` instead of `main`/`master`. - -7. **Change PR base branch** from `master` to `lwt` using `gh pr edit`. - -8. **Verify**: Ensure tests pass or at least the code compiles correctly for the lwt branch. diff --git a/.agents/tasks/feedback/STEP-1.md b/.agents/tasks/feedback/STEP-1.md deleted file mode 100644 index edc15db..0000000 --- a/.agents/tasks/feedback/STEP-1.md +++ /dev/null @@ -1,24 +0,0 @@ -# Rebase test-suite onto origin/lwt and recreate PR changes - -Status: COMPLETED - -## Sub tasks - -1. [x] Reset `test-suite` branch to `origin/lwt` base -2. [x] Recreate all PR file additions adapted for Lwt - -## NOTES - -Instead of rebasing (which would produce many conflicts in files we didn't change due to heavy divergence between master/Eio and lwt), we reset the branch to `origin/lwt` and recreated all PR changes from scratch: - -- `dune` — added `flow` to `data_only_dirs` -- `.gitignore` — added `tests/integration_test_run/` -- `src/cli/link_flags.linux.dev.dune` — created with `()` (empty flags) -- `src/quickjs/dune` — replaced fragile libomp.a cp chain with robust path search loop -- `tests/fixtures/` — created demo.html, demo.js, demo.pug, demo.vue -- `tests/dune` — created with lwt/lwt.unix/angstrom-lwt-unix deps (instead of eio_main) -- `tests/test_runner.ml` — rewritten for Lwt (see STEP-2) -- `.github/workflows/test.yml` — targets `lwt` branch instead of `main` -- `AGENTS.md` — created, references Lwt (not Eio) -- `ARCHITECTURE.md` — created, references Lwt (not Eio) -- `DEVELOPMENT.md` — added Testing section diff --git a/.agents/tasks/feedback/STEP-10.md b/.agents/tasks/feedback/STEP-10.md deleted file mode 100644 index 0827d42..0000000 --- a/.agents/tasks/feedback/STEP-10.md +++ /dev/null @@ -1,26 +0,0 @@ -# Form a list of the good comments and resolve them - -Status: COMPLETED - -## Valid Comments Resolved - -| # | File | Issue | Resolution | -|---|------|-------|------------| -| 1 | `src/quickjs/dune:59` | `PAWTHS` typo | Renamed to `PATHS` | -| 2 | `.github/workflows/test.yml:33` | `curl` without fail-fast flags | Changed to `curl -fsSL ... -o quickjs.tar.xz` | -| 3 | `tests/dune:41-53` | Side-effecting block clobbers `strings/french.strings` in repo root | Removed the entire "populate root strings/" block | -| 4 | `DEVELOPMENT.md:72-81` | Claims `dune runtest` populates `strings/` dir | Rewritten to accurately describe hermetic test behavior | -| 5 | `ARCHITECTURE.md:73-76` | Says `ppx_expect`; conflates SZXX+Angstrom for Pug | Fixed to `ppx_inline_test` + `ppx_assert`; separated SZXX (HTML) from Angstrom (Pug) | - -## Sub tasks - -1. [x] Implement fix 1: Rename `PAWTHS` → `PATHS` in `src/quickjs/dune` -2. [x] Implement fix 2: Use `curl -fsSL` in `.github/workflows/test.yml` -3. [x] Implement fix 3: Remove side-effecting block from `tests/dune` -4. [x] Implement fix 4: Update `DEVELOPMENT.md` testing section -5. [x] Implement fix 5: Fix `ARCHITECTURE.md` test tooling description -6. [x] Run tests — all pass - -## NOTES - -All 5 Copilot comments were valid and resolved. Tests pass after changes. The most substantive fix was removing the side-effecting block from `tests/dune` that would clobber `strings/french.strings` on every test run. diff --git a/.agents/tasks/feedback/STEP-2.md b/.agents/tasks/feedback/STEP-2.md deleted file mode 100644 index 7e0757c..0000000 --- a/.agents/tasks/feedback/STEP-2.md +++ /dev/null @@ -1,19 +0,0 @@ -# Rewrite test_runner.ml for Lwt - -Status: COMPLETED - -## Sub tasks - -1. [x] Replace `Eio_posix.run` with `Lwt_main.run` -2. [x] Replace `Eio.Flow.string_source` with `Lwt_io.of_bytes ~mode:Lwt_io.input` -3. [x] Use `Lwt.Syntax` `let+` for Lwt promise handling -4. [x] Keep JS/HTML/Pug tests unchanged (they use synchronous `exec_parser`) - -## NOTES - -Key changes in test_runner.ml: -- `strings_parsing` and `french_strings_parsing` tests now use `Lwt_main.run` instead of `Eio_posix.run` -- Created `Lwt_io` input channels from in-memory byte strings using `Lwt_io.of_bytes ~mode:Lwt_io.input @@ Lwt_bytes.of_string content` -- Used `let+` from `Lwt.Syntax` since `Strings.parse` returns `Lwt.t` -- JS, HTML, and Pug tests are unchanged as they use the synchronous `Basic.exec_parser` -- Initially wrote `Lwt_io.Input` (uppercase) — corrected to `Lwt_io.input` (lowercase value) after verifying against Lwt's own test suite via Sourcegraph diff --git a/.agents/tasks/feedback/STEP-3.md b/.agents/tasks/feedback/STEP-3.md deleted file mode 100644 index 8b64abd..0000000 --- a/.agents/tasks/feedback/STEP-3.md +++ /dev/null @@ -1,11 +0,0 @@ -# Update tests/dune dependencies (eio_main → lwt) - -Status: COMPLETED - -## Sub tasks - -1. [x] Replace `eio_main` with `lwt lwt.unix angstrom-lwt-unix` in library deps - -## NOTES - -The test library in `tests/dune` was created with Lwt dependencies from the start (since we recreated all files from scratch on the lwt base in STEP-1). Final deps: `parsing utils core lwt lwt.unix angstrom-lwt-unix`. diff --git a/.agents/tasks/feedback/STEP-4.md b/.agents/tasks/feedback/STEP-4.md deleted file mode 100644 index 8e297cd..0000000 --- a/.agents/tasks/feedback/STEP-4.md +++ /dev/null @@ -1,13 +0,0 @@ -# Update AGENTS.md to reference Lwt instead of Eio - -Status: COMPLETED - -## Sub tasks - -1. [x] Replace "Eio (concurrency)" with "Lwt (concurrency)" in Key Libraries -2. [x] Replace Eio concurrency description with Lwt (`Lwt_list`, `Lwt_pool`, `Lwt_preemptive`) -3. [x] Remove `Fiber.List.iter` / `Eio.Executor_pool` references - -## NOTES - -Created AGENTS.md from scratch on the lwt base. All references use Lwt terminology: `Lwt_pool`, `Lwt_list`, `Lwt_preemptive`. diff --git a/.agents/tasks/feedback/STEP-5.md b/.agents/tasks/feedback/STEP-5.md deleted file mode 100644 index d2d888a..0000000 --- a/.agents/tasks/feedback/STEP-5.md +++ /dev/null @@ -1,17 +0,0 @@ -# Update ARCHITECTURE.md to reference Lwt instead of Eio - -Status: COMPLETED - -## Sub tasks - -1. [x] Replace "Eio runtime" with "Lwt runtime" in Project Entry Point -2. [x] Update Control Flow section (Eio → Lwt, `Lwt_list` and `Lwt_pool`) -3. [x] Update `Parsing.Strings.parse` API description to mention `Lwt_io.input_channel` -4. [x] Update Testing Setup section to reference `Lwt_main.run` and `Lwt_io` - -## NOTES - -Created ARCHITECTURE.md from scratch on the lwt base. Key differences from Eio version: -- Control flow references `Lwt` cooperative concurrency via `Lwt_list` and `Lwt_pool` -- `Strings.parse` documented as taking `Lwt_io.input_channel` returning `Lwt.t` -- Testing section notes `.strings` tests use `Lwt_main.run` diff --git a/.agents/tasks/feedback/STEP-6.md b/.agents/tasks/feedback/STEP-6.md deleted file mode 100644 index ef8e2a8..0000000 --- a/.agents/tasks/feedback/STEP-6.md +++ /dev/null @@ -1,12 +0,0 @@ -# Update CI workflow for lwt dependencies - -Status: COMPLETED - -## Sub tasks - -1. [x] Change branch triggers from `main`/`master` to `lwt` -2. [x] Keep opam deps the same (lwt branch opam already has correct deps) - -## NOTES - -CI workflow `.github/workflows/test.yml` created with `lwt` and `test-suite` as push branches, and `lwt` as PR branch. The opam install step uses `--update-invariant` for flambda compatibility. diff --git a/.agents/tasks/feedback/STEP-7.md b/.agents/tasks/feedback/STEP-7.md deleted file mode 100644 index 0d2dbbb..0000000 --- a/.agents/tasks/feedback/STEP-7.md +++ /dev/null @@ -1,13 +0,0 @@ -# Change PR #7 base branch from master to lwt - -Status: COMPLETED - -## Sub tasks - -1. [x] Run `gh pr edit 7 --base lwt` -2. [x] Update PR body to reference Lwt instead of Eio - -## NOTES - -- Changed base from `master` to `lwt` using `gh pr edit 7 --base lwt` -- Updated PR description to reference Lwt-specific testing (Lwt_main.run, Lwt_io) diff --git a/.agents/tasks/feedback/STEP-8.md b/.agents/tasks/feedback/STEP-8.md deleted file mode 100644 index 5afdd5e..0000000 --- a/.agents/tasks/feedback/STEP-8.md +++ /dev/null @@ -1,12 +0,0 @@ -# Verify build/tests - -Status: COMPLETED - -## Sub tasks - -1. [x] Initialize opam environment locally -2. [x] Run `dune runtest tests/` to verify tests pass - -## NOTES - -User ran `opam install . --deps-only -t --update-invariant` and then `dune runtest tests/` — all tests passed locally. diff --git a/.agents/tasks/feedback/STEP-9.md b/.agents/tasks/feedback/STEP-9.md deleted file mode 100644 index e64a181..0000000 --- a/.agents/tasks/feedback/STEP-9.md +++ /dev/null @@ -1,59 +0,0 @@ -# Review latest Copilot comments on PR #7 and evaluate each one - -Status: COMPLETED - -## Sub tasks - -1. [x] Fetch all review comments from PR #7 (Copilot + Devin) -2. [x] Evaluate each Copilot comment individually -3. [x] Evaluate Devin comments for completeness - -## Copilot Review Comments (5 comments from review 3878489572) - -### Comment 1 — `src/quickjs/dune`: Rename `PAWTHS` to `PATHS` -**Verdict: VALID (minor)** -The variable `PAWTHS` is indeed a typo/odd spelling of `PATHS`. Renaming improves readability. Trivial fix. - -### Comment 2 — `.github/workflows/test.yml`: Use `curl -fsSL` for QuickJS download -**Verdict: VALID (good practice)** -`curl URL > file` can silently succeed with an error page (e.g. 404 HTML body) and only fail during `tar`. Using `curl -fsSL ... -o quickjs.tar.xz` is more robust. Worth fixing. - -### Comment 3 — `tests/dune`: Remove the "populate root strings/" block -**Verdict: VALID (substantive)** -This block tries to mutate the working tree from a test rule. Copilot claims `../../..` from `_build/default/tests` resolves to `_build` not repo root — but actually `_build/default/tests/../../../` = repo root. However, the broader point is correct: **tests should not mutate the working tree**. The block clobbers `strings/french.strings` on every `dune runtest` if a `strings/` dir exists. This is a real side-effect concern. Devin also flagged this (and then self-resolved as false positive on the path computation, but the data-loss concern stands). We should remove this block and update DEVELOPMENT.md accordingly. - -### Comment 4 — `DEVELOPMENT.md`: Documentation mismatch about `strings/` population -**Verdict: VALID (follows from Comment 3)** -DEVELOPMENT.md line 81 claims `dune runtest tests/` "populates the `strings/` directory with `english.strings` ... and merged `french.strings`." If we remove the side-effecting block per Comment 3, this documentation is wrong. Needs updating either way — the current docs describe behavior that is a test side-effect we're removing. - -### Comment 5 — `ARCHITECTURE.md`: Inaccurate test tooling description -**Verdict: VALID (documentation accuracy)** -Line 74 says the test suite uses `ppx_expect` — but it doesn't. It uses `ppx_inline_test` + `ppx_assert`. Also "HTML and Pug extraction via `SZXX` and `Angstrom`" conflates SZXX (HTML) with Pug (Angstrom only). The suggested text is accurate. - -## Devin Review Comments (evaluated for completeness) - -### Devin Comment (review 1) — `src/quickjs/dune`: libomp.a fallback chain missing Ubuntu x86_64 -**Verdict: ALREADY RESOLVED** — The current code (as of latest commits) already includes `/usr/lib/x86_64-linux-gnu/libgomp.a` and glob patterns. The fallback chain was reworked. - -### Devin Comment (review 2) — `src/quickjs/dune`: `%{project_root}/../../` paths -**Verdict: FALSE POSITIVE (self-resolved by Devin)** — `%{project_root}` in dune resolves to `_build/default`, not repo root. So `../../` correctly navigates to repo root. Standard dune pattern. - -### Devin Comment (review 3) — `tests/dune`: Data loss from writing to repo root -**Verdict: VALID (same as Copilot Comment 3)** — Already addressed above. - -### Devin Comment (review 3, part 2) — `src/quickjs/dune`: Wildcard glob can match multiple files -**Verdict: ALREADY RESOLVED** — Current code uses a nested loop with `for matched_path in $p` and picks the first match with `exit 0`, handling glob expansion correctly. - -## Summary of Valid Comments Worth Resolving - -| # | File | Issue | Severity | -|---|------|-------|----------| -| 1 | `src/quickjs/dune` | Rename `PAWTHS` → `PATHS` | Minor | -| 2 | `.github/workflows/test.yml` | Use `curl -fsSL ... -o` | Minor | -| 3 | `tests/dune` | Remove side-effecting "populate root strings/" block | Substantive | -| 4 | `DEVELOPMENT.md` | Fix docs about `strings/` population | Documentation | -| 5 | `ARCHITECTURE.md` | Fix test tooling description (ppx_expect → ppx_inline_test, SZXX/Pug) | Documentation | - -## NOTES - -All 5 Copilot comments are valid and worth resolving. Comments 3 and 4 are related (removing side-effect + updating docs). None of the Devin comments need new action — they were either already resolved in prior commits or are false positives. diff --git a/.agents/tasks/feedback/TODOs.md b/.agents/tasks/feedback/TODOs.md deleted file mode 100644 index a79a1bf..0000000 --- a/.agents/tasks/feedback/TODOs.md +++ /dev/null @@ -1,10 +0,0 @@ -1. [x] Rebase `test-suite` branch onto `origin/lwt` and resolve conflicts -2. [x] Rewrite `tests/test_runner.ml` for Lwt -3. [x] Update `tests/dune` dependencies (eio_main → lwt) -4. [x] Update `AGENTS.md` to reference Lwt instead of Eio -5. [x] Update `ARCHITECTURE.md` to reference Lwt instead of Eio -6. [x] Update CI workflow (`.github/workflows/test.yml`) for lwt dependencies -7. [x] Change PR #7 base branch from `master` to `lwt` -8. [x] Verify build/tests -9. [x] Review latest Copilot comments on PR #7 and evaluate each one -10. [x] Form a list of the good comments and resolve them diff --git a/.gitignore b/.gitignore index f6881ea..88f23aa 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,5 @@ src/sedlex src/collections tests/integration_test_run/ +.agents/ +ignored/