diff --git a/core/docs/pkg-requests-m1.md b/core/docs/pkg-requests-m1.md new file mode 100644 index 0000000..c20dd4f --- /dev/null +++ b/core/docs/pkg-requests-m1.md @@ -0,0 +1,44 @@ +# CORE → PKG/QA — requests + +Status: **open**. Raised by lane CORE. PKG/QA owns root build files, `.github/`, and the +toolchain survey. + +## P1. Enable the `core` subdirectory — one line + +`CMakeLists.txt` line 38 is the commented `add_subdirectory(core)`. CORE has landed its +first target (`libveloxcore` + `veloxcore_tests`), so please uncomment it: + +```cmake +add_subdirectory(core) # lane CORE → libveloxcore +``` + +`core/CMakeLists.txt` is self-contained: it defines the `veloxcore` library and, under +`if(VELOX_BUILD_TESTS)`, the test target registered with CTest. It pulls in no external +package yet (stage 1 = `util/` only). `net/` (stage 2) will add `find_package(CURL 8.0)` +and `meta/` (stage 5) will add `find_package(OpenSSL)` — separate one-line requests when +those land. + +## P2. Test framework decision + +No test framework is chosen in the root `CMakeLists.txt` or the docs. CORE is currently +using a ~90-line header-only harness at `core/tests/support/vtest.hpp` so the lane isn't +blocked. This is **provisional**. Please pick one (GoogleTest / Catch2 / doctest) and say +how it's provided (system package vs `FetchContent` vs vendored) — CORE will swap the +harness for it. The harness API is deliberately tiny (`VT_TEST`, `VT_CHECK`, `VT_REQUIRE`, +`VT_CHECK_EQ`) so the migration is mechanical. + +## P3. `VELOX_BUILD_FUZZ` needs a clang guard + +This machine has no `clang++`; libFuzzer is clang-only. `core/CMakeLists.txt` guards the +fuzz targets behind `if(VELOX_BUILD_FUZZ AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")` so a +GCC configure with `-DVELOX_BUILD_FUZZ=ON` (the `ci` preset sets it) doesn't hard-fail. +The `ci` preset in `CMakePresets.json` should either run with clang or expect fuzz targets +to be skipped on GCC. Flagging so CI doesn't go red on the first CORE push. + +## P4. `dev`/`tsan` presets overwrite `CMAKE_CXX_FLAGS` + +`CMakePresets.json` sets `CMAKE_CXX_FLAGS` wholesale (`-fsanitize=... -g`), which replaces +rather than appends. `core/CMakeLists.txt` therefore sets its own `-Wall -Wextra -Werror` +via `target_compile_options` (not the cache var) so warnings-as-errors survive a +sanitizer preset. No action needed unless PKG would rather centralize warning flags — just +noting why CORE sets them at target scope. diff --git a/core/docs/proto-requests-m1.md b/core/docs/proto-requests-m1.md new file mode 100644 index 0000000..5c85902 --- /dev/null +++ b/core/docs/proto-requests-m1.md @@ -0,0 +1,157 @@ +# CORE → PROTO — requests before the `VERSION` freeze + +Status: **open**. Raised by lane CORE against `contracts/` at `1.0.0-draft`. +This is a request list, not a decision record. PROTO owns every item here. Once PROTO +answers D1, that one becomes a three-way ADR (`docs/adr/`) per `contracts/README.md` rule 4. + +Ranking is by `contracts/README.md` rule 4, not by intuition: +new method or new optional field = **minor**, can land anytime; +rename / remove / retype / default change = **major** + ADR. + +--- + +## Freeze-blockers — get these into `1.0.0` (major if we guess wrong) + +### B1. A frozen wire enum for `TaskSummary.error.code` + +`TaskSummary.error.code` is currently a bare `integer`. CORE cannot import a protocol +header (layering rule — `core/` never sees JSON/RPC), so CORE has its own failure +taxonomy **regardless** of what PROTO picks; DAEMON owns the projection from one to the +other. The ask is not "who owns the enum" — it is: **freeze a wire enum with enough +cardinality that DAEMON's mapping from the CORE taxonomy below is lossless.** + +If the wire enum is coarser than this table, DAEMON has to collapse distinct engine +failures into one code and the GUI can't tell "server file changed" from "checksum +mismatch". Retyping `error.code` later is a major bump. + +CORE failure taxonomy (this *is* `vdm::Error` in `core/include/vdm/util/error.hpp`): + +| CORE `Error` | Meaning | `retryable` | carries `httpStatus` | +|---|---|---|---| +| `canceled` | user/daemon cancelled | no | no | +| `resolve_failed` | DNS resolution failed | yes | no | +| `connect_failed` | TCP connect failed | yes | no | +| `tls_failed` | TLS handshake / cert rejected | no | no | +| `connection_reset` | peer reset mid-transfer | yes | no | +| `timeout` | connect or idle/stall timeout | yes | no | +| `too_many_redirects` | redirect limit hit | no | no | +| `http_client_error` | 4xx not otherwise classified | no | yes | +| `http_server_error` | 5xx | yes | yes | +| `auth_required` | 401 / 407 — task pauses, not terminal | no | yes | +| `forbidden` | 403 after the referrer-retry (`docs/04` §7) | no | yes | +| `not_found` | 404 | no | yes | +| `range_not_satisfiable` | 416 — triggers re-probe/re-split | yes | yes | +| `gone` | 410 / expired signed URL, no mirror left | no | yes | +| `server_file_changed` | `200` where `206` expected, or `If-Range` / ETag drift (`docs/04` §5) | no | yes | +| `content_length_mismatch` | bytes received ≠ declared length | yes | no | +| `checksum_mismatch` | post-download hash ≠ expected | no | no | +| `disk_full` | ENOSPC | no | no | +| `io_error` | `pwrite` / `fallocate` / `fdatasync` failed | no | no | +| `path_rejected` | outside allowed roots or not writable | no | no | +| `permission_denied` | EACCES on the destination | no | no | +| `meta_corrupt` | `.veloxpart.meta` failed CRC / bad magic | no | no | +| `meta_version_unsupported` | meta file from a newer engine | no | no | +| `probe_failed` | probe could not determine file info | maybe | yes | +| `unsupported_url_scheme` | not http/https | no | no | +| `max_retries_exhausted` | backoff loop gave up (`data` names the last underlying `Error`) | no | maybe | +| `internal` | invariant violation / bug | no | no | + +Concretely: a **string enum** on `error.code` mirroring these names is simplest to keep +lossless; if it stays an `integer`, please reserve grouped ranges (e.g. 1xx network, +2xx http, 3xx local-io, 4xx metadata, 5xx internal) with room to grow inside each. + +### B2. Freeze the meaning of `TaskSummary.segments` — *major, retype-of-meaning* + +`docs/04` §4/§3 require the *effective* value (after the per-host cap and the +non-resumable→1 demotion) to reach the UI. `TaskSummary.segments` currently has no stated +meaning. Please freeze it as **"effective segment count in use right now"**, not the +requested value. This is the only genuinely freeze-blocking half of the buffer/segment +story — pinning a field's meaning after `1.0.0` is a major bump under rule 4. It stands +alone; do not defer it together with B2a below. + +### B2a. A readable effective buffer size — *minor, but note the trap* + +Split out from B2 on purpose so PROTO can't use the cheap half to defer the major half. + +`bufferBytes` is **write-only**: accepted by `download.add` and `download.update.patch`, +readable on **no** type. `docs/04` §4 says the clamped / "silently reduced to fit" value +is "reported back to the UI", so CORE will expose an effective value and PROTO needs a +frozen field to carry it. Adding an optional field is minor under rule 4 — **except** +that `TaskSummary` is `additionalProperties: false`, so a daemon cannot tack it on as an +extension; it has to be in the schema or it's dropped by every conformance replay. Name +and placement are PROTO's call (`effectiveBufferBytes` on `TaskSummary` / `TaskDetail`, +or a dedicated readback). Land it whenever — it just can't be improvised later. + +### B3. Freeze `Segment.schema.json` field names + +`Segment` is already on PROTO's type list (`contracts/README.md`) and `TaskDetail` +"includes `segments[]`", so this is mostly field-name matching — but do it **before** +freeze or each fix is a rename = major. + +CORE will emit per segment: + +| CORE field | Type | Note | +|---|---|---| +| `index` | int ≥ 0 | **`event.task.progress` currently says `i`.** Pick one name for both. | +| `start` | int ≥ 0 | absolute byte offset, inclusive | +| `end` | int ≥ 0 | absolute byte offset, **exclusive** — range is `[start, end)` | +| `completed` | int ≥ 0 | bytes written in this range so far | +| `speedBps` | int ≥ 0 | current per-segment rate | +| `state` | enum | `connecting` \| `downloading` \| `stalled` \| `complete` \| `failed` | + +Asks: (a) reconcile `i` vs `index` — one spelling in both the `Segment` type and the +`event.task.progress` payload; (b) confirm the half-open `[start, end)` convention in the +schema `description` so DAEMON and GUI don't off-by-one the last byte; (c) confirm the +segment `state` enum values. + +--- + +## Do not hold the freeze for these + +All minor under rule 4 (new methods / new events / a pattern on an existing optional +string). `contracts/README.md` rule 5 wants contract changes "cheap and frequent" — raise +these now, land them as small follow-up PRs, but **do not gate `1.0.0` on them.** Each is +needed *within* M1. + +### F1. A "needs user decision" carrier — *new event + method, minor* + +`docs/04` §5 mandates a "File on server has changed — restart download?" prompt, and §7 +has 416/stale-metadata. The event surface has only `event.notify` + task states. Either: +- confirm `state: paused` + an `event.notify {level:"warn", …}` is the intended carrier + (CORE reports `server_file_changed`, DAEMON pauses + notifies, GUI offers restart), **or** +- add `event.task.decision {taskId, kind, options[]}` + `download.decide {taskId, choice}`. + +CORE needs to know which, but a string comparison on `error.code` covers the engine side +either way, so this doesn't block freeze. + +### F2. A credential return path for 401/407 — *new method, minor, M1-needed* + +`event.auth.required` exists; there is **no method to hand credentials back to a paused +task**. `session logins` / Secret Service is M3, but "401 handled" is in CORE's M1 DoD. +Requesting `download.provideAuth {taskId, username, password, save?}` → +`{ok}`. Secrets go to the Secret Service, never SQLite/logs (`CLAUDE.md` §4) — `save` +just tells DAEMON whether to persist. + +### F3. `checksum` string format — *pattern on an existing optional field, borderline* + +`download.add {checksum?}` has no shape. CORE's verifier will accept `":"` +with `algo ∈ {md5, sha1, sha256}` (appendix lists MD5/SHA-256). Adding a `pattern` to an +existing optional string is close to free; PROTO's call whether it's worth a fixture now +or a doc note. + +--- + +## Three-way ADR material (after PROTO answers) + +### D1. State-machine ownership split — CORE / DAEMON / PROTO + +`docs/04` §1 hands CORE the lifecycle, but `queued` and every scheduler transition are +DAEMON's. Proposed split, to be recorded as an ADR once all three agree: + +- **CORE owns:** `probing → connecting → downloading ⇄ paused → retry_wait → assembling + → verifying → complete | failed`, plus `cancelled` from any state. +- **DAEMON owns:** `new`, `queued`, and pause-for-schedule (enters CORE's `paused`). +- **Shared:** `paused` is reachable from either side; both must be idempotent about it. + +This needs an ADR regardless of the answer (`contracts/README.md` rule 4: the lifecycle +enum is a frozen type and any change to who drives it is a documented decision).