diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 36ac396e7a3..37282e0fcf8 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,178 +1,77 @@ # CCF Repository Copilot Instructions -- This document provides guidance for AI coding and review agents working in the CCF (Confidential Consortium Framework) repository -- **CCF** is an open-source framework for building secure, highly available, and performant applications focused on multi-party compute and data. It's designed for confidential, distributed systems running on secure hardware. -- When prompted to work from a GitHub Issue, make sure the resulting PR description links to that Issue so reviewers can see the context. Use `Closes #123` (with the real Issue number) so that the Issue is automatically closed when the PR is merged. +CCF (Confidential Consortium Framework) is a replicated state machine for confidential, distributed applications. -## Architecture +## Task boundaries -CCF is a replicated state machine where application state lives in an in-memory **key-value store** (`src/kv/`). Writes are serialised to an append-only **ledger** and replicated across nodes via the **AFT consensus protocol** (a Raft variant in `src/consensus/aft/`). The node lifecycle — startup, join, recovery, reconfiguration — is managed by the **node state machine** (`src/node/node_state.h`). +- Answer questions and planning requests without editing files unless requested. +- Inspect the worktree before editing. Preserve existing user changes; ask before overwriting conflicting edits. +- Limit edits to the requested task and necessary tests/documentation. Do not fix unrelated failures, reformat unrelated files, or expand into adjacent refactors. +- Use only ASCII characters in code you add or modify for commit, including comments, docstrings, and string literals, and in agent instruction files. Lean source files (`*.lean`) are exempt and may use Unicode. Elsewhere, use ASCII escape sequences when Unicode test data or runtime output is needed; preserve its meaning. Existing grandfathered Unicode lines may remain unchanged, but must not be expanded. +- When a PR resolves an issue, include `Closes #123` with the actual issue number in its description. Use a non-closing reference for partial work. -Applications are either **C++ endpoint registries** (subclassing `ccf::UserEndpointRegistry`) or **JavaScript/TypeScript bundles** executed by an embedded QuickJS runtime (`src/js/`). Both register HTTP endpoints that read/write the KV store through transaction objects (`ccf::Tx`). +## Repository map -Governance is handled by a built-in **member-driven constitution** system — proposals are submitted as JavaScript and executed against the KV. The crypto subsystem (`src/crypto/`, `include/ccf/crypto/`) wraps OpenSSL for TLS, x.509, COSE signatures, and Merkle tree operations. +- `src/kv/`, `src/consensus/aft/`: transactional in-memory state, ledger replication, and the AFT consensus protocol. +- `src/node/node_state.h`, `src/service/`: node lifecycle and governance tables; member constitutions execute JavaScript against the KV. +- `src/endpoints/`, `src/js/`, `samples/`: C++ endpoint registries and embedded QuickJS applications. +- `src/crypto/`, `src/tls/`, `src/http/`: cryptography, TLS, and HTTP transport. +- `include/ccf/`: public C++ API; `src/ds/`: internal utilities. +- `tests/`, `tests/infra/`: Python e2e tests and network infrastructure; C++ unit tests live alongside implementation code. +- `python/`: Python SDK; `doc/`: Sphinx/RST documentation; `tla/`: formal specifications; `cmake/`: build helpers. -**Key directories**: +## Task-specific guidance -- `src/` — Core C++ implementation, including unit tests in subdirectories - - `consensus/aft/` — AFT (Raft variant) consensus protocol - - `kv/` — Replicated key-value store and transaction machinery - - `node/` — Node state machine, governance, historical queries, snapshots - - `crypto/` — Cryptographic primitives (OpenSSL wrappers, COSE, Merkle) - - `endpoints/` — HTTP endpoint registration and dispatch - - `js/` — QuickJS-based JavaScript runtime for JS applications - - `http/` — HTTP/1.1 and HTTP/2 parser and session management - - `tls/` — TLS session handling - - `ds/` — Data structures and utilities (logging, serialisation helpers) - - `service/` — Internal service tables and governance tables -- `include/ccf/` — Public C++ API headers (the stable interface for app developers) -- `tests/` — Python-based end-to-end test suite and infrastructure (`tests/infra/`) -- `python/` — CCF Python SDK (ledger parsing, COSE signing, receipts) -- `doc/` — Sphinx-based RST documentation -- `samples/` — Example C++ and JS applications -- `tla/` — TLA+ formal specifications for consensus and disaster recovery -- `cmake/` — CMake build helpers (`common.cmake`, `ccf_app.cmake`) +- For C/C++ changes and security-sensitive reviews in any language, read [security/safety review guidance and C/C++ conventions](/.github/instructions/reviewing.instructions.md). +- Before selecting, running, or writing tests, load the [testing skill](/.github/skills/testing/SKILL.md). +- Before formatting or linting, load the [formatting-and-linting skill](/.github/skills/formatting-and-linting/SKILL.md). +- For user-facing API or behaviour changes, update existing documentation and follow the [changelog instructions](/.github/instructions/changelog.instructions.md). Link to existing documentation rather than duplicating it. -## Build, test, and lint +## Validation -### Building +- Run checks relevant to the changed files before pushing. For C++ changes, build affected targets and run relevant tests locally. Behaviour changes need regression tests, including e2e coverage for user-visible behaviour. +- For changes that may affect older releases, use the compatibility procedure in the testing skill. +- Run `scripts/ci-checks.sh` without auto-fix for full local validation when prerequisites are available. Targeted checks do not constitute a full-suite pass. +- Required CI checks, including applicable tests in `.github/workflows/ci.yml`, must pass before merge; local validation does not replace them. +- If a check is blocked by missing tools, network access, privileges, or resources, report the exact command, blocker, and checks still needed. Do not claim unrun checks passed or weaken checks to obtain a pass. -```bash -mkdir build && cd build -cmake -GNinja .. # RelWithDebInfo by default -cmake -GNinja -DCMAKE_BUILD_TYPE=Debug .. # Debug with clang-tidy: add -DCLANG_TIDY=ON -ninja # Build all targets -``` +### Build prerequisites and commands -### Testing +Use [development setup](/doc/contribute/build_setup.rst) and [building CCF](/doc/contribute/build_ccf.rst) for supported environments and dependencies. The Copilot setup workflow installs formatting/lint prerequisites only; it does not provision a full C++ build/test environment. The full checks include `test-buckets-checks.sh`, which requires a successful CMake configure. -Before selecting, running, or writing tests, load the [testing skill](/.github/skills/testing/SKILL.md) for unit, e2e, SDK, test-label, coverage, and e2e test-pattern guidance. +From the repository root, after installing build prerequisites: -### Linting and formatting +```bash +cmake -S . -B build -GNinja +cmake --build build +``` -Before formatting or linting changes, load the [formatting-and-linting skill](/.github/skills/formatting-and-linting/SKILL.md) to choose the checks for each file type and identify which support auto-fix. +The default configuration is `RelWithDebInfo`. For a separate Debug build, select a different build directory and `-DCMAKE_BUILD_TYPE=Debug`; add `-DCLANG_TIDY=ON` only when clang-tidy is installed. Reuse existing build configuration intentionally rather than overwriting it. ### Documentation +For RST changes, build Sphinx from the repository root in a Python virtual environment with the documentation dependencies: + ```bash uv pip install -r doc/requirements.txt -r doc/historical_ccf_requirements.txt sphinx-build --fail-on-warning -b html doc doc/html ``` -## Code changes - -- `ci-checks.sh` must run successfully before any commit is pushed. -- All tests in `ci.yml` must pass before a PR can be merged. Consider which are likely to be affected by your changes and run those locally before pushing. -- Take particular care with any changes that may affect compatibility with older releases, and ensure these are tested, via the `lts_compatibility` test with `LONG_TESTS=1` enabled. -- Take particular care with changes to the consensus and crypto code, as these are critical for security and correctness. Ensure you have a thorough understanding of the existing code and the implications of your changes before proceeding. -- Any changes to user-facing APIs or behaviour must be documented in `CHANGELOG.md` in Keep a Changelog format, under a concrete version and in an `Added`, `Changed`, `Fixed`, `Removed`, or similarly named section. Follow `.github/instructions/changelog.instructions.md` to select the correct release section and keep `python/pyproject.toml` in sync. - -### C++ - -- C++ changes must be built and tested locally before creating a PR. -- Most changes should be accompanied by new or updated tests. End-to-end tests are required for any changes that affect the user-visible behaviour. - -#### Naming conventions - -- **Classes/structs**: `PascalCase` (`EndpointRegistry`, `TypedMap`, `NodeState`) -- **Methods/functions**: `snake_case` (`make_endpoint`, `set_auto_schema`, `get_path_param`) -- **Member variables**: `snake_case`, no prefix (`uri_path`, `forwarding_required`) -- **Constants**: `UPPER_SNAKE_CASE` (`PRIVATE_RECORDS`, `JOIN_TIMEOUT`) -- **Namespaces**: `snake_case` (`ccf::kv`, `ccf::endpoints`, `ccf::crypto`) -- **Files**: `snake_case` (`node_state.h`, `endpoint_registry.cpp`) -- **Header guards**: Always `#pragma once`, never `#ifndef` - -#### JSON serialisation - -Use the `DECLARE_JSON_*` macros from `ccf/ds/json.h` for struct serialisation: - -```cpp -DECLARE_JSON_TYPE(MyStruct); -DECLARE_JSON_REQUIRED_FIELDS(MyStruct, field_a, field_b); - -DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(MyConfig); -DECLARE_JSON_REQUIRED_FIELDS(MyConfig, name); -DECLARE_JSON_OPTIONAL_FIELDS(MyConfig, description, timeout); - -DECLARE_JSON_TYPE_WITH_BASE(DerivedType, BaseType); -DECLARE_JSON_REQUIRED_FIELDS(DerivedType, extra_field); -``` - -#### Endpoint registration - -Endpoints are registered in `init_handlers()` using a fluent builder pattern: - -```cpp -make_endpoint("/records/{key}", HTTP_PUT, handler, {ccf::user_cert_auth_policy}) - .set_auto_schema() - .set_forwarding_required(ccf::endpoints::ForwardingRequired::Never) - .install(); - -make_read_only_endpoint("/records/{key}", HTTP_GET, ro_handler, {ccf::user_cert_auth_policy}) - .install(); -``` - -Use `make_endpoint` for read-write, `make_read_only_endpoint` for read-only, and `make_command_endpoint` for operations that don't access the KV store. - -#### Logging - -Use the macro-based logging system. For application code: - -```cpp -CCF_APP_INFO("Processing request for key {}", key); // INFO level, "app" tag -CCF_APP_FAIL("Failed to process: {}", error_msg); // FAIL level -CCF_APP_TRACE("Debug detail: {}", detail); // TRACE level -``` - -For framework-internal code, use `LOG_INFO_FMT`, `LOG_DEBUG_FMT`, `LOG_FAIL_FMT`, `LOG_FATAL_FMT` (from `src/ds/internal_logger.h`). Log levels in order of decreasing verbosity: `TRACE`, `DEBUG`, `INFO`, `FAIL`, `FATAL`. - -#### KV store - -Maps are typed with key/value serialisers and accessed through transaction handles: - -```cpp -using MyMap = ccf::kv::Map>; -auto* handle = ctx.tx.template rw("my_map"); // Read-write handle -handle->put(key, value); -auto val = handle->get(key); // Returns std::optional -``` - ### Python -- There are 2 kinds of Python code in the repository: the end-to-end tests (and supporting infra) in `tests/`, and the Python SDK in `python/`. -- Pay attention to existing helpers and utilities in the test suite when writing new tests, and avoid duplicating code. If you find yourself copying and pasting code, consider refactoring it into a shared helper function or class. -- All code in the SDK should include type annotations and docstrings. - -### Documentation - -- Any RST changes must be built with Sphinx to ensure they render correctly. -- Check for existing documentation on the topic before creating new docs, and provide thorough crosslinks where appropriate. Avoid duplicating information that already exists in the docs. -- For any user-facing changes, ensure that the documentation is updated to reflect the new behaviour. +- Reuse existing e2e helpers from `tests/infra/`; only extract new shared helpers when needed for the task. +- Add type annotations and docstrings to new or changed SDK interfaces in `python/`; do not retrofit unrelated code. -### Security posture +## Security and correctness -- **No secrets in code**: Avoid committing API keys, passwords, or other secrets. Some certificates and keys are included in the repository for testing purposes, but if adding more ensure these are freshly created and properly documented as test-only artifacts. -- **Input validation**: Always validate and sanitize external inputs -- **Cryptographic operations**: Use CCF's crypto library (`include/ccf/crypto/`) — don't roll your own -- **Memory safety**: Use RAII, smart pointers, and avoid manual memory management +- Never commit credentials or production keys. New certificate/key fixtures must be freshly generated and clearly test-only. +- Use CCF's crypto APIs in `include/ccf/crypto/` rather than implementing cryptographic primitives. +- For consensus, KV, and crypto changes, trace affected commit/rollback, ownership, and failure paths before editing; cover the relevant invariants with regression tests. ## Reviews -- Never comment on code formatting when performing code reviews. -- Flag any non-ASCII characters introduced in source code, comments, docstrings, or string literals (for example em/en dashes U+2014/U+2013, smart quotes U+201C/U+201D and U+2018/U+2019, arrow U+2192, or ligatures such as U+FB01). These frequently slip in via copy-paste or AI-generated text. Ask the author to replace them with the plain ASCII equivalent (for example U+2014 -> `-`, U+2192 -> `->`, U+201C -> `"`). Genuinely intentional non-ASCII (such as box-drawing or block glyphs used to render terminal charts/visualisations, or characters that are the subject of a test) is acceptable; everything else should be ASCII. -- When shell scripts or bash scripts are created or modified, if they contain any use of the pipe (|) operator, they must also set the pipefail option (set -o pipefail). Remind the PR author if they have missed that. -- When writing a pull request review summary, include a "Custom instructions used" section that lists every repository instruction file (from `.github/copilot-instructions.md` and `.github/instructions/`) that was loaded and applied during the review. This lets the PR author verify the correct instruction files were active. - -### Code Review Security Focus - -When reviewing code, pay special attention to: - -- Authentication and authorization logic -- Cryptographic operations -- Input parsing and validation -- Memory management -- Error handling in security-critical paths - -### Third-party library return values and error handling - -See [reviewing instructions](/.github/instructions/reviewing.instructions.md) for detailed guidance on checking return values from OpenSSL, libcurl, llhttp, nghttp2, QuickJS, and other third-party C libraries. When a diff adds or modifies calls to any of these libraries, verify that every call that can fail has its return value checked, the correct check macro is used, and error handling is consistent within each function. +- Security and safety are the highest review priority: protect confidentiality, authorization, integrity, consensus safety, and availability before considering performance or convenience. Apply the security and safety review approaches in the scoped guidance where relevant. +- Report actionable issues introduced by the diff, with a code location, triggering condition, and consequence. Separate demonstrated security impact from correctness risks and unverified hypotheses; do not call a finding exploitable without a supported path. +- Leave mechanical formatting to existing checks; do not repeat their findings as inline review comments. Run `scripts/ascii-checks.sh` for its covered files. Apply the character policy above when reviewing changed code and agent instructions outside its coverage; exclusions other than Lean do not permit new non-ASCII code. Uncovered violations are an explicit exception to the no-formatting-comments rule. +- Bash scripts with pipelines must enable `set -o pipefail`. For other shells, check support before recommending Bash-specific options. +- Include a "Custom instructions used" section in PR review summaries listing the repository instruction files actually loaded and applied. Cite the scoped error-handling instructions when reporting a violation of that policy. diff --git a/.github/instructions/changelog.instructions.md b/.github/instructions/changelog.instructions.md index 2424fcc2b4c..8bb711e0c86 100644 --- a/.github/instructions/changelog.instructions.md +++ b/.github/instructions/changelog.instructions.md @@ -1,30 +1,33 @@ --- -applyTo: - - "CHANGELOG.md" +applyTo: "CHANGELOG.md,python/pyproject.toml" --- # CHANGELOG entries -These instructions apply both when writing and when reviewing changes to `CHANGELOG.md`. +Use [Keep a Changelog 1.0.0](https://keepachangelog.com/en/1.0.0/) format, with the CCF-specific rules below. + +These instructions apply when writing or reviewing changelog entries and SDK release-version changes. Unrelated changes to `python/pyproject.toml` do not require a release bump. ## Selecting the release section -Before adding an entry, determine the latest published CCF release from the git `ccf-` tags or github.com/microsoft/CCF releases. Do not infer release status from the contents of `CHANGELOG.md` alone. +Before adding an entry, identify the target branch's release line and whether it is a stable maintenance line or a prerelease/development line. Use the branch context and published `ccf-` releases/tags; do not select the repository-wide newest release across unrelated release lines, infer publication from `CHANGELOG.md` alone, or rely on an incomplete local tag list. Confirm ambiguous publication status against GitHub releases. -- Every entry must be placed under a concrete Semantic Versioning release section. -- If the first release section in `CHANGELOG.md` is newer than the latest published release, treat it as the next release and add the entry to the appropriate existing subsection. -- If the first release section has already been published, create a new section above it by incrementing the patch component of the latest published release. Add the matching link definition using the existing `https://github.com/microsoft/CCF/releases/tag/ccf-` convention. +- Use concrete Semantic Versioning release sections, not `Unreleased`. +- If the first section is an unpublished next release for the target line, use it. +- On a stable maintenance line, if the first section is already published, create the next patch section above it using the latest published stable release on that line. Add the matching `https://github.com/microsoft/CCF/releases/tag/ccf-` link definition. +- For prerelease/development lines, follow an explicit release target rather than inventing a patch, minor, major, or prerelease increment. If the target line, next version, or publication status cannot be established, ask for clarification before editing release metadata. - Whenever a new release section is created, update `project.version` in `python/pyproject.toml` to the same version. The first version in `CHANGELOG.md` and `project.version` must always match. -- When reviewing a changelog addition, verify the release status, section selection, and version synchronisation above. +- When reviewing release metadata, verify section selection and version synchronisation. Report unavailable publication evidence as a validation limitation, not a guessed release status. ## Pull request references -Every new or modified entry must include a reference to the pull request that introduced the change and to the relevant issues(s) that the PR closes, in the form `(#1234)` at the end of the entry, matching the existing convention. +Each new or modified entry must reference the introducing PR using `(#1234)`. Preserve original PR references when correcting an existing entry; include the current PR when it introduces an additional change. Issue references are optional in changelog entries; closing references belong in the PR description. -When reviewing, flag any added or modified bullet under an `Added`, `Changed`, `Fixed`, `Removed`, or similarly named section that does not include such a `(#)` reference, and ask the author to add the corresponding PR number. This applies to entries directly under top-level version sections and in nested subsections such as `Developer API` / `C++` / `Added`. +- Before a PR number exists, omit the reference temporarily and report that it must be added once the PR exists, before merge. Never invent a number or add a fake numeric placeholder. +- When reviewing a PR, flag touched entries missing the relevant PR reference, including nested entries. A reference to an issue alone does not satisfy the PR-reference requirement. Do not flag: - Section headings, version headings, or release-highlights blockquotes. - Pre-existing entries that the diff does not touch. -- Entries that already cite at least one PR number, even if they reference additional issues or commits as well. +- Entries that already cite the relevant introducing PR, whether or not they also reference issues or commits. diff --git a/.github/instructions/reviewing.instructions.md b/.github/instructions/reviewing.instructions.md index df8060af3a7..bd89bd835a5 100644 --- a/.github/instructions/reviewing.instructions.md +++ b/.github/instructions/reviewing.instructions.md @@ -7,118 +7,105 @@ applyTo: - "**/*.c" --- -# Code review – third-party library error handling +# Security/safety review guidance and C/C++ conventions -When flagging a third-party error-handling issue during review, cite this file (`.github/instructions/reviewing.instructions.md`) so the author can look up the full guidelines. +The security/safety guidance applies to security-sensitive reviews in any language, as linked from the global instructions. The `applyTo` patterns additionally load this file for C/C++ changes; the C++ conventions and library-specific sections apply only where relevant. -When reviewing any C++ change that adds or modifies calls to OpenSSL (or another third-party C library), apply the checks below. The goal is to catch unchecked return values and inconsistent error-handling patterns before they reach production. +## ASCII-only authoring and review -## General principles +- Apply the ASCII policy and Lean exception in the [repository instructions](/.github/copilot-instructions.md#task-boundaries). +- For files subject to the ASCII policy, check additions and modified text before committing or approving, including files outside `scripts/ascii-checks.sh` coverage. Report uncovered violations; do not duplicate findings already reported by the automated check. +- Keep fixes scoped to the current change. Do not rewrite unrelated existing Unicode fixtures, vendored files, or prose documentation. -1. **Every call that can fail must be checked.** If a C function documents a failure return (error code, null pointer, negative value, …), the caller must test for it. Silently discarding the result is always a defect in this codebase. -2. **Use the project's own helpers.** CCF already provides wrapper macros and RAII types for the most common libraries (see tables below). Prefer those over ad-hoc `if` checks so that error messages stay consistent and nothing is accidentally skipped. -3. **Consistent style within a function.** If the first half of a function uses `CHECK1()` for every OpenSSL call but the second half silently ignores a return value, flag the inconsistency even if the ignored call "usually succeeds." -4. **Clean up on every error path.** When RAII wrappers are not used, verify that every early-return or throw after a partial allocation frees the already-acquired resources. +## Security and safety first -## OpenSSL +CCF's primary review concern is preserving its security guarantees and distributed-system safety. Review confidentiality, authentication/authorization, data and ledger integrity, consensus safety, and resistance to denial of service before performance or convenience. A safety violation matters even without a demonstrated attacker. -CCF wraps OpenSSL with helpers defined in `include/ccf/crypto/openssl/openssl_wrappers.h`. +- Identify the trust boundary and who controls each input: unauthenticated clients, users, members, peer nodes, or the untrusted host. Do not assume authenticated input is well-formed or authorized for every operation. +- Trace validation through the protected operation, including forwarding, asynchronous callbacks, retries, recovery, and rollback. A check is insufficient if the checked identity/state can change before use or if another path bypasses it. +- Check that failures leave state consistent and do not expose secrets, grant access, or continue using partially validated data. Distinguish rejected input from retryable outcomes and internal invariant failures; do not turn attacker-controlled errors into process-wide termination. +- Require regression coverage appropriate to the changed invariant: malformed and boundary inputs, unauthorized callers, and relevant lifetime or commit/rollback interleavings. Follow the testing skill rather than adding unrelated tests or tools. +- For each finding, identify the changed location, controllable input or triggering interleaving, missing protection, and concrete consequence. State uncertainty and prerequisites; matching a risk pattern alone is not evidence of a vulnerability. -### Available check macros +### Security and safety review approaches -| Macro | Use when the OpenSSL function … | -| ---------------------------- | ----------------------------------------------------------------- | -| `CHECK1(rc)` | returns **1** on success (most `EVP_*`, `BN_*`, `X509_*` setters) | -| `CHECKNULL(ptr)` | returns a **pointer** that is null on failure | -| `CHECKPOSITIVE(val)` | returns a **positive int** on success (e.g. `EVP_PKEY_CTX_set_*`) | -| `CHECKEQUAL(expect, actual)` | must return an **exact value** | +Apply these approaches to the paths affected by the diff, including called libraries and shared helpers, rather than limiting review to the edited lines. -### Available RAII wrappers (`Unique_*`) +- **Prioritize processing reachable before authentication.** Trace all potentially unauthenticated data processing, including parsing performed to verify credentials or signatures. Check for stack overflow from unbounded recursion or nesting, out-of-bounds access, use-after-free, and attacker-controlled allocations. Authentication later in the pipeline does not protect earlier processing. +- **Bound resource consumption end to end.** Verify limits on input size, nesting depth, element counts, allocation growth, CPU work, and concurrent or queued requests. A byte-size limit alone does not bound stack depth or computational complexity. Enforce limits before expensive processing and reject excess work without terminating the node. +- **Bind claims and permissions to the authenticated identity.** Verify that identity claims satisfy trusted issuer, tenant, audience, and validity constraints required by the policy. Check authorization for the actual operation and resource; a valid signature or successful key lookup is not sufficient. +- **Validate cryptographic material at admission and use.** Check structural validity, unambiguous identifiers, permitted algorithms and key uses, key strength, and trust-anchor constraints. Authorized configuration changes still require validation; stored material must not acquire broader trust when consumed by another component. +- **Establish trust before using claims.** Verify the complete certificate/signature/attestation policy, including required metadata and peer identity, before returning trusted outputs or authorizing actions. Keep necessary pre-verification parsing bounded. Preserve intended trust anchors across library changes and fallback paths. +- **Validate lengths and arithmetic before acting on them.** Check declared lengths against available bytes, overflow-safe offsets, representable ranges, narrowing conversions, and intermediate unit/time arithmetic before allocation, access, or state mutation. Reconcile limits across protocol layers while preserving documented compatibility requirements. +- **Preserve consensus and transactional invariants across transitions.** Trace all consumers when changing state or index semantics. Distinguish signed, committed, speculative, and term-local state; check atomicity of validation and mutation across concurrency, retries, term changes, rollback, and recovery. Rejected work must not leave state or metadata inconsistent. +- **Protect data throughout its lifetime.** Trace sensitive data through memory ownership, logging, responses, and persistent files. Use restrictive access at file creation, preserve exclusive-create guarantees where required, and release resources on partial failure. Host-side access controls do not make the host trusted or replace cryptographic confidentiality protections. -`Unique_EVP_PKEY_CTX`, `Unique_BIO`, `Unique_PKEY`, `Unique_X509`, `Unique_X509_REQ`, `Unique_X509_CRL`, `Unique_SSL_CTX`, `Unique_SSL`, `Unique_BIGNUM`, `Unique_X509_TIME`, and others. These call the correct `*_free()` destructor automatically. +## C++ conventions -### What to look for +- Use `PascalCase` for classes/structs; `snake_case` for functions, members (no prefix), namespaces, and filenames; `UPPER_SNAKE_CASE` for constants. Use `#pragma once` for headers. +- Use `DECLARE_JSON_*` macros from `include/ccf/ds/json.h` for struct serialisation, selecting required, optional, and base fields to match the data contract. +- Register endpoints in `init_handlers()`: `make_endpoint` for read-write transactions, `make_read_only_endpoint` for read-only transactions, and `make_command_endpoint` for no KV access. Select authentication and forwarding policies explicitly for the endpoint's semantics; follow nearby handlers rather than copying an arbitrary policy. +- Access typed KV maps through transaction handles (`tx.rw` or `tx.ro`), not directly. Handle absent values returned by `get`. +- Use `CCF_APP_*` logging macros in applications and `LOG_*_FMT` macros from `src/ds/internal_logger.h` internally. Levels, in decreasing verbosity: `TRACE`, `DEBUG`, `INFO`, `FAIL`, `FATAL`. +- Prefer existing RAII wrappers and smart pointers. Follow surrounding comment density; explain invariants or non-obvious behaviour, not the history of an edit. -- **Allocations without `CHECKNULL`:** Any direct call to `EVP_PKEY_new()`, `BIO_new()`, `X509_new()`, `EVP_MD_CTX_new()`, `BN_new()`, `SSL_CTX_new()`, `SSL_new()`, or similar that stores the result without passing it through `CHECKNULL()` or an equivalent null check. -- **`CHECK1` vs `CHECKPOSITIVE` mix-ups:** Some OpenSSL functions (notably `EVP_PKEY_CTX_set_*`) return a positive value on success, not exactly 1. Using `CHECK1` on those calls will incorrectly treat valid return codes > 1 as failures and trigger false-positive error handling. Conversely, `CHECKPOSITIVE` is wrong for functions that return exactly 1 on success. -- **`BIO_get_mem_ptr` / `BIO_read` ignored:** These return an int indicating success. Verify the return is tested before dereferencing the output pointer. -- **Missing `ERR_get_error` drain on error paths:** When an OpenSSL failure is caught but the error queue is not drained (or vice-versa), later calls may see stale errors. -- **Raw `new`/`free` instead of RAII wrappers:** If a `Unique_*` type exists for the object, the review should suggest using it rather than manual `*_free()` calls. -- **Partial checks:** A sequence of OpenSSL calls where some are wrapped in a check macro and others are not is a red flag. All calls in the sequence should be checked. +## Error-handling review method -### Consult the documentation +1. Establish the specific API's return-value and ownership contract using the version shipped by the repository, its headers, wrappers, and matching documentation. Do not infer a contract from a function-name prefix. +2. Check whether failure is handled locally, by a wrapper, or by propagation to the caller. Distinguish failures from normal outcomes such as verification mismatch, EOF, retry, or absent properties. +3. Prefer existing check helpers when their success predicate and throwing behaviour fit the call site. Explicit checks are valid for recoverable errors, partial results, callbacks, and non-throwing cleanup. +4. Trace resource ownership through partial allocation, early return, exceptions, and ownership transfer. Do not add duplicate checks or frees when a wrapper already handles them. +5. Flag ignored failures when they cause incorrect behaviour or lose necessary diagnostics. Best-effort cleanup may intentionally ignore a result when it cannot affect correctness; verify that justification rather than treating every discarded return as a defect. +6. Explain the concrete failure path in a review finding. A different check style, manual ownership that is demonstrably correct, or a missing preferred error string alone is not a correctness defect. -OpenSSL documents return values on its man pages (). When reviewing a call you are unfamiliar with, look up the specific function to confirm: +## OpenSSL -- What value indicates success (1, 0, positive, non-null, …). -- Whether the function sets the OpenSSL error queue on failure. -- Whether the caller must free the returned object. +CCF wraps OpenSSL with helpers defined in `include/ccf/crypto/openssl/openssl_wrappers.h`. -Use this to verify that the correct check macro is used and that the error path is appropriate. +### Check helpers -## libcurl +| Helper | Success predicate | +| ---------------------------- | ------------------ | +| `CHECK1(rc)` | `rc == 1` | +| `CHECKNULL(ptr)` | `ptr != nullptr` | +| `CHECKPOSITIVE(val)` | `val > 0` | +| `CHECKEQUAL(expect, actual)` | `actual == expect` | -CCF wraps libcurl in `src/http/curl.h`. +Choose the predicate from the individual API contract. `CHECK1` rejects valid values above 1 for APIs allowing any positive success result. `CHECKPOSITIVE` is also correct for an API whose only success is 1 and whose failures are all non-positive; do not flag that equivalence as a bug. These helpers throw, and `CHECKNULL` validates rather than returns the pointer. -| Macro | Use when … | -| -------------------------------------------- | ----------------------------------- | -| `CHECK_CURL_EASY(fn, ...)` | calling any `curl_easy_*` function | -| `CHECK_CURL_EASY_SETOPT(handle, opt, arg)` | calling `curl_easy_setopt` | -| `CHECK_CURL_EASY_GETINFO(handle, info, arg)` | calling `curl_easy_getinfo` | -| `CHECK_CURL_MULTI(fn, ...)` | calling any `curl_multi_*` function | +- Existing `Unique_*` wrappers cover objects such as BIOs, keys, certificates, and SSL contexts. Verify the chosen constructor's allocation/null check and ownership semantics before adding another check. +- `BIO_get_mem_ptr` returns a control result and writes an output pointer; validate success before using that pointer. `BIO_read` returns a byte count, not a Boolean: handle short reads and the BIO's EOF/retry/error semantics. +- Inspect error-queue ownership at the recovery boundary. Preserve errors needed by the caller (notably before `SSL_get_error`); drain or clear stale errors only where the API contract and recovery flow require it. One `ERR_get_error()` removes one entry, not the entire queue. +- For unfamiliar APIs, consult the matching-version [OpenSSL documentation](https://docs.openssl.org/) for success values, error-queue behaviour, and whether returned objects are owned or borrowed. -### What to look for +## libcurl -- Direct calls to `curl_easy_setopt`, `curl_easy_perform`, or `curl_multi_*` that do not use the above macros. -- `curl_easy_init()` or `curl_multi_init()` returns not checked for null. -- `curl_slist_append()` return not checked for null (it returns null on allocation failure). +CCF wraps libcurl in `src/http/curl.h`. -## llhttp (HTTP/1.x parser) +| Macro | Applicable return contract | +| -------------------------------------------- | ------------------------------- | +| `CHECK_CURL_EASY(fn, ...)` | `CURLcode`, success `CURLE_OK` | +| `CHECK_CURL_EASY_SETOPT(handle, opt, arg)` | `curl_easy_setopt` | +| `CHECK_CURL_EASY_GETINFO(handle, info, arg)` | `curl_easy_getinfo` | +| `CHECK_CURL_MULTI(fn, ...)` | `CURLMcode`, success `CURLM_OK` | -Used in `src/http/http_parser.h`. The parser entry point is `llhttp_execute()`; its return must be compared against `HPE_OK` (and, where relevant, `HPE_PAUSED_UPGRADE`). +These macros throw; use explicit handling for recoverable transfer errors. They do not apply to pointer- or void-returning APIs. `curl_easy_init()` and `curl_multi_init()` need null checks, already provided by CCF's `UniqueCURL`/`UniqueCURLM` constructors. On `curl_slist_append()` failure, preserve ownership of the original list rather than overwriting its only pointer with null. -### What to look for +## llhttp (HTTP/1.x parser) -- Calls to `llhttp_execute()` whose return value is not tested. -- Missing use of `llhttp_errno_name()` / `llhttp_get_error_reason()` in the error message (makes debugging harder). -- Callback return values: llhttp callbacks (e.g. `on_message_complete`) that return non-zero indicate a parse error to the library. Ensure these are intentional. +Used in `src/http/http_parser.h`. Check `llhttp_execute()` against `HPE_OK`, handling supported pause/upgrade outcomes explicitly. Callback return contracts differ; distinguish intentional pause/upgrade from parse errors using the shipped API. For parse failures, prefer diagnostics from `llhttp_errno_name()` / `llhttp_get_error_reason()`. ## nghttp2 (HTTP/2) -Used in `src/http/http2_callbacks.h` and `src/http/http2_session.h`. Most `nghttp2_*` functions return 0 on success or a negative error code. - -### What to look for - -- Calls to `nghttp2_session_*`, `nghttp2_submit_*`, or `nghttp2_hd_*` where the return value is silently discarded. -- Error messages that print only the raw integer code instead of `nghttp2_strerror(rc)`. -- `nghttp2_session_send()` / `nghttp2_session_mem_recv()` return values not checked. +Used in `src/http/http2_callbacks.h` and `src/http/http2_session.h`. Many APIs return 0 on success and negative error codes, but others return counts or identifiers. In particular, `nghttp2_session_mem_recv()` returns consumed bytes on success; account for partial consumption. Check `nghttp2_session_send()` failures and prefer `nghttp2_strerror(rc)` in error diagnostics. ## QuickJS -Used in `src/js/`. `JS_*` functions return `JSValue`; errors are indicated by `JS_IsException()`. - -### What to look for - -- `JS_Call`, `JS_Eval`, `JS_GetPropertyStr`, `JS_NewObject`, etc. whose return value is not passed through `JS_IsException()` (or an equivalent check) before use. -- Missing `JS_FreeValue()` on values that are no longer needed (leaks in the JS runtime). - -## Other third-party libraries - -For any other C library call added in a change (e.g. `uv_*` from libuv, zlib, or platform APIs), apply the same discipline: - -1. Look up the function's documented return-value contract. -2. Confirm the call site checks for the failure case. -3. Confirm the error message includes enough context (function name, error code or string) to be debuggable. -4. Confirm resources are released on the error path. - -## Checklist for reviewers +Used in `src/js/`, with declarations in `3rdparty/exported/quickjs/quickjs.h`. -Use this as a mental checklist when reviewing a diff that touches third-party library calls: +- Fallible `JSValue` producers such as `JS_Call`, `JS_Eval`, and `JS_GetPropertyStr` indicate exceptions via `JS_IsException()`. Check or propagate exceptions before treating the value as a successful result. +- Integer-returning APIs such as `JS_ToInt32` and `JS_SetPropertyStr` use their documented integer failure convention, not `JS_IsException()`. +- Pointer-returning constructors such as `JS_NewRuntime` need null checks; void-returning functions cannot be return-checked. +- Track owned, borrowed, duplicated, and consumed values. Free owned values no longer needed (or use existing RAII wrappers), but do not free values after a consuming API has taken ownership. -- [ ] Every function that can fail has its return value checked. -- [ ] The correct check macro/pattern is used (e.g. `CHECK1` vs `CHECKPOSITIVE` for OpenSSL). -- [ ] All allocations are null-checked, ideally via RAII wrappers. -- [ ] Error handling is consistent within each function — no unchecked calls mixed with checked ones. -- [ ] Error messages include the library's own error string (e.g. `error_string(ec)`, `nghttp2_strerror(rc)`). -- [ ] Resources allocated before the failing call are freed on the error path. -- [ ] No OpenSSL error-queue state is leaked across unrelated operations. +Apply the same contract-first review method to other libraries, including libuv, zlib, and platform APIs. diff --git a/.github/skills/formatting-and-linting/SKILL.md b/.github/skills/formatting-and-linting/SKILL.md index 5f5e029015f..163c1d272f0 100644 --- a/.github/skills/formatting-and-linting/SKILL.md +++ b/.github/skills/formatting-and-linting/SKILL.md @@ -6,49 +6,42 @@ description: "Format and lint CCF changes. Use when choosing or running checks f # Formatting and linting -`scripts/ci-checks.sh` orchestrates all formatting and linting checks by running individual scripts concurrently. You can run all checks at once, or run only the scripts relevant to the files you changed. - -To run **all** checks with auto-fix: `scripts/ci-checks.sh -f` - -To run **only the checks you need**, use the individual scripts below based on the file types you modified. When a script supports `-f`, you **must** use it to auto-fix issues. When `-f` is not available, run the script and read its error output to determine what changes are needed. - -## Scripts with auto-fix (`-f`) - -These scripts accept a `-f` flag that automatically corrects issues. Always run them with `-f`: - -| Script | Run with | File types | Tool | -| --------------------------------- | ------------------------------------ | ----------------------------------------------------------------------------- | ------------------------ | -| `scripts/cpp-format-checks.sh` | `scripts/cpp-format-checks.sh -f` | `.h`, `.hpp`, `.c`, `.cpp`, `.cc` in `include/`, `src/`, `samples/` | clang-format | -| `scripts/python-format-checks.sh` | `scripts/python-format-checks.sh -f` | `.py` in `tests/`, `python/`, `scripts/`, `tla/` | black | -| `scripts/python-lint-checks.sh` | `scripts/python-lint-checks.sh -f` | `.py` in `python/`, `tests/` | ruff | -| `scripts/prettier-checks.sh` | `scripts/prettier-checks.sh -f` | `.ts`, `.js`, `.md`, `.yaml`, `.yml`, `.json` (excludes `tests/sandbox/`) | prettier | -| `scripts/cmake-format-checks.sh` | `scripts/cmake-format-checks.sh -f` | `CMakeLists.txt` and `.cmake` files in `cmake/`, `samples/`, `src/`, `tests/` | gersemi | -| `scripts/release-notes-checks.sh` | `scripts/release-notes-checks.sh -f` | Release notes in `CHANGELOG.md` | extract-release-notes.py | - -## Scripts without auto-fix - -These scripts only report problems. Run them and read their error output to determine what manual changes are needed: - -| Script | Run with | File types | What to look for in the output | -| -------------------------------- | -------------------------------- | ---------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | -| `scripts/shellcheck-checks.sh` | `scripts/shellcheck-checks.sh` | `.sh` files (excludes `3rdparty/`) | shellcheck warnings and errors with line numbers and fix suggestions | -| `scripts/python-types-checks.sh` | `scripts/python-types-checks.sh` | `.py` in `python/` | mypy type errors with file, line number, and expected types | -| `scripts/includes-checks.sh` | `scripts/includes-checks.sh` | Public headers under `include/ccf/` (`.h`, `.hpp`) | Public/private include violations in files under `include/ccf/`, missing `namespace ccf` in public headers, or unused exported headers | -| `scripts/copyright-checks.sh` | `scripts/copyright-checks.sh` | All source files | Files missing or with incorrect copyright notice headers | -| `scripts/openapi-checks.sh` | `scripts/openapi-checks.sh` | `.json` in `doc/schemas/` | OpenAPI schema validation errors from swagger-cli | -| `scripts/todo-checks.sh` | `scripts/todo-checks.sh` | All tracked files | Unacceptable comments that must be removed or resolved | -| `scripts/ascii-checks.sh` | `scripts/ascii-checks.sh` | Source files (excludes `3rdparty/`, prose docs, and intentionally non-ASCII files) | Non-ASCII characters that must be replaced with their plain ASCII equivalents | - -## Which scripts to run for each file type - -| If you modified | Run these scripts | -| ----------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| C/C++ source or headers (`.h`, `.hpp`, `.c`, `.cpp`, `.cc`) | `cpp-format-checks.sh -f`, `includes-checks.sh`, `copyright-checks.sh`, `ascii-checks.sh` | -| Python files (`.py`) | `python-format-checks.sh -f`, `python-lint-checks.sh -f`, `python-types-checks.sh`, `copyright-checks.sh`, `ascii-checks.sh` | -| TypeScript/JavaScript (`.ts`, `.js`) | `prettier-checks.sh -f`, `copyright-checks.sh`, `ascii-checks.sh` | -| Markdown (`.md`) | `prettier-checks.sh -f` | -| YAML (`.yaml`, `.yml`) | `prettier-checks.sh -f`, `ascii-checks.sh` | -| JSON (`.json`) | `prettier-checks.sh -f`, `openapi-checks.sh` (if in `doc/schemas/`), `ascii-checks.sh` | -| CMake files (`CMakeLists.txt`, `.cmake`) | `cmake-format-checks.sh -f`, `ascii-checks.sh` | -| Shell scripts (`.sh`) | `shellcheck-checks.sh`, `copyright-checks.sh`, `ascii-checks.sh` | -| Release notes (`CHANGELOG.md`) | `release-notes-checks.sh -f`, `prettier-checks.sh -f` | +## Scope and prerequisites + +Run commands from the repository root. `scripts/ci-checks.sh` runs the individual checks concurrently, including a build-configuration check. Use the global instructions' validation policy to choose targeted local checks or the full suite. + +The Copilot setup workflow runs `scripts/setup-ubuntu-ci-checks.sh` for Ubuntu formatting/lint prerequisites. Some checks use `uvx` or npm to obtain tools at runtime and need network access. The test-bucket check additionally requires the full CMake configure prerequisites; the setup workflow does not install those. + +## Check first, fix only task-owned changes + +- Run scripts without `-f` initially. Success is exit status 0; inspect failure output to distinguish changed-file issues, unrelated failures, and environment blockers. +- The scripts generally scan whole directories or tracked files, not just the diff. Selecting a script by file type does not restrict which files it may rewrite. +- For auto-fix, use the existing underlying formatter/linter with explicit changed-file paths and the same version/configuration used by the script. Only fix files or hunks owned by the task; preserve existing user edits. +- Use a script's `-f` mode only after verifying its complete write scope is intended. Do not run repository-wide auto-fix as a default. +- Inspect the resulting diff and rerun the applicable check. Do not remove unrelated edits to make checks pass. Report blockers and unrelated failures under the global validation policy. + +## Check inventory + +Each command below is under `scripts/`. This table is a routing guide; the scripts own exact file coverage, exclusions, tool versions, and options. When changing that coverage, update this guide too. Include cross-cutting checks (copyright, disallowed comments, ASCII) when applicable. + +| Script | Relevant changes | Tool/check | Supports auto-fix | +| ------------------------- | ------------------------------------------------------------ | -------------------------------------------------- | ----------------- | +| `cpp-format-checks.sh` | C/C++ in `include/`, `src/`, `samples/` | clang-format | `-f` | +| `python-format-checks.sh` | Python in `tests/`, `python/`, `scripts/`, `tla/` | black | `-f` | +| `python-lint-checks.sh` | Python in `python/`, `tests/` | ruff | `-f` | +| `python-types-checks.sh` | Python SDK | mypy | No | +| `prettier-checks.sh` | TS, JS, Markdown, YAML, JSON (excluding `tests/sandbox/`) | prettier | `-f` | +| `cmake-format-checks.sh` | CMake files | gersemi | `-f` | +| `release-notes-checks.sh` | `CHANGELOG.md` (also run prettier) | extract-release-notes.py | `-f` | +| `shellcheck-checks.sh` | Shell scripts outside `3rdparty/` | shellcheck | No | +| `includes-checks.sh` | Public C++ headers and their uses | Public/private include and exported-header checks | No | +| `copyright-checks.sh` | Source files | Copyright notices | No | +| `openapi-checks.sh` | JSON under `doc/schemas/` | openapi-spec-validator | No | +| `todo-checks.sh` | Tracked files | Disallowed comments | No | +| `ascii-checks.sh` | Source/config files and agent-instruction Markdown | ASCII policy and grandfathered Unicode lines | No | +| `ascii-policy-tests.sh` | ASCII policy/checker changes | ASCII policy regression tests | No | +| `test-buckets-checks.sh` | CMake test registration, defaults, or `tests/ci-buckets.txt` | Fresh configure and CI bucket inventory comparison | No | + +Some report-only scripts accept `-f` for interface compatibility without changing files. For Rust or other file types not covered by a formatter above, consult their existing build/CI configuration rather than introducing a new tool. + +The ASCII check includes Rust and TLA+, but exempts Lean source files (`*.lean`). Existing Unicode is grandfathered by exact line hashes, not file-wide exemptions. Do not extend the grandfathered hashes to accept new Unicode. diff --git a/.github/skills/testing/SKILL.md b/.github/skills/testing/SKILL.md index bf906670949..c891945da26 100644 --- a/.github/skills/testing/SKILL.md +++ b/.github/skills/testing/SKILL.md @@ -1,75 +1,77 @@ --- name: testing user-invocable: false -description: "Run and write CCF tests. Use when selecting or executing unit, end-to-end, partition, compatibility, coverage, or Python SDK tests, or when adding an e2e test. Covers test labels, the tests.sh wrapper, e2e infrastructure, and test patterns. Never call ctest without reading this first." +description: "Select, run, and write CCF unit, end-to-end, partition, compatibility, coverage, and Python SDK tests. Covers prerequisites, the tests.sh wrapper, test labels, and e2e registration." --- # Testing ## Running tests -Tests must be run via the `tests.sh` wrapper (in the build directory), which sets up a Python venv, installs the SDK and test dependencies, then invokes `ctest`: +Follow the global validation policy for choosing relevant tests and reporting blockers. Before running tests, configure and build the affected targets using the documented development environment. The Copilot formatting/lint setup alone is not a full test environment. + +From the repository root, enter the configured build directory (shown here as `build`). Use its generated `tests.sh` wrapper for e2e tests: it creates/activates a Python venv, installs the SDK and test dependencies, then invokes `ctest`. First-time setup requires Python venv support and network access; subsequent runs still invoke dependency installation. ```bash cd build -./tests.sh # Run all tests -./tests.sh -VV # Verbose output -./tests.sh -R # Run tests matching a name regex -./tests.sh -L unit # Run only unit tests -./tests.sh -L e2e # Run only end-to-end tests -./tests.sh -L partitions # Run partition tests (requires NET_ADMIN) -./tests.sh --timeout 360 -R recovery_test # Single e2e test with timeout +./tests.sh -N # Discover registered tests; does not run them +./tests.sh -L '^unit$' --no-tests=error +./tests.sh -L '^e2e$' --no-tests=error +./tests.sh -L '^partitions$' --no-tests=error # Requires NET_ADMIN +./tests.sh -VV --timeout 360 -R '^recovery_test$' --no-tests=error ``` -Test labels: `unit`, `e2e`, `partitions`, `perf`, `benchmark`, `raft_scenario`, `suite`, `lts_compatibility`, `snp`. +Use `-R` for name regexes and `-L` for labels. `./tests.sh` without a filter runs all registered tests; do not make this the default for a small change. Pure C++ unit tests and test discovery may use `ctest` directly from the build directory without Python setup. Use `--no-tests=error` for execution so an empty selection is not mistaken for passing tests. + +Labels include `unit`, `e2e`, `partitions`, `perf`, `benchmark`, `raft_scenario`, `suite`, `lts_compatibility`, `snp`, and CI routing labels `bucket_a`, `bucket_b`, `bucket_c`. Registration depends on build options; inspect the configured inventory rather than assuming a named test exists. -Python SDK tests (separate from e2e): +### Compatibility + +For changes affecting older releases, configure the intended build directory with `-DLONG_TESTS=ON` (a CMake option, not just a shell variable), rebuild affected targets, then run from that directory: ```bash -cd python && pytest +./tests.sh -R '^lts_compatibility$' --no-tests=error ``` +`LONG_TESTS` enables additional ledger compatibility coverage. This test is not registered with `SAN=ON`; use a suitable separate build rather than silently skipping it. It also needs access to older releases; report unavailable downloads as blockers. + +### Python SDK + +SDK tests are separate from e2e. From the repository root, in an activated Python venv meeting `python/pyproject.toml`'s Python requirement, install the SDK and pytest as the SDK CI job does: + +```bash +uv pip install -e ./python pytest +cd python +pytest +``` + +Exit status 0 indicates success; report the selected tests and their actual result, not merely a successful setup step. + ## Code coverage -Build with `-DCOVERAGE=ON`, run tests, then: +Configure with `-DCOVERAGE=ON`, build instrumented targets, and run the selected tests. From that build directory, with `llvm-profdata` and `llvm-cov` available: ```bash -scripts/coverage.sh # Print summary -scripts/coverage.sh --html report/ # Generate HTML report +../scripts/coverage.sh # Print summary +../scripts/coverage.sh --html report/ # Generate HTML report ``` +These paths assume `build` is immediately under the repository root. For another layout, use the actual path to `scripts/coverage.sh` while retaining the build working directory. The script consumes `.profraw` files and the generated `coverage_binaries.txt`; building alone does not produce coverage. + ## End-to-end test infrastructure E2e tests use the infrastructure in `tests/infra/`. The key classes are: -- `infra.network.Network` — manages a multi-node CCF network (start, stop, find primary/backup, add/remove nodes) -- `infra.node.Node` — represents a single CCF node process -- `infra.consortium.Consortium` — member governance operations (proposals, votes) -- `infra.runner.ConcurrentRunner` — runs multiple test functions against separate networks in parallel +- `infra.network.Network`: manages a multi-node network; use the existing `infra.network.network(...)` context-manager pattern for lifecycle cleanup. +- `infra.node.Node`: represents one node process. +- `infra.consortium.Consortium`: member governance operations. +- `infra.runner.ConcurrentRunner`: schedules `run_*(args)` functions, each owning its network. ## Writing e2e tests -Test functions take `(network, args)` parameters and are decorated with requirement annotations: - -```python -@reqs.description("Write/Read messages on primary") -@reqs.supports_methods("/app/log/private") -@reqs.at_least_n_nodes(2) -def test_example(network, args): - primary, _ = network.find_primary() - with primary.client("user0") as c: - r = c.post("/app/log/private", body={"id": 42, "msg": "hello"}) - assert r.status_code == http.HTTPStatus.OK - return network -``` - -Tests are assembled in `ConcurrentRunner` at the bottom of test files: - -```python -if __name__ == "__main__": - cr = ConcurrentRunner() - cr.add("test_name", test_function, package="samples/apps/logging/logging", nodes=...) - cr.run() -``` +Follow a nearby test for the same application. In `tests/e2e_logging.py`, individual test cases accept `(network, args)`, while `run(args)` creates/opens the network and calls those cases. Requirement decorators from `suite.test_requirements` express the case's prerequisites. -When a test needs its own network configuration, deep-copy `const_args`, set a distinct `args.label`, and create a standalone `Network` in a separate `run_*` function. +- Register the network-owning `run_*(args)` function with `ConcurrentRunner.add`, not a `(network, args)` case. The runner invokes its target with one argument. +- `ConcurrentRunner.add(prefix, target, **args_overrides)` already deep-copies arguments and assigns a distinct label. Prefer its overrides for separate configurations rather than duplicating that work. Deep-copy arguments and set a distinct label yourself only when manually creating an additional independent configuration outside that mechanism. +- Reuse network/client/governance helpers; assert observable behaviour and clean up through existing context managers. +- Ensure the case is called by a runner. For a new e2e executable entry point, follow existing CMake `add_e2e_test` registration, including its CI bucket. Run `scripts/test-buckets-checks.sh` from the repository root when changing inventory, and update `tests/ci-buckets.txt` only for intentional registration changes. diff --git a/scripts/ascii-checks.sh b/scripts/ascii-checks.sh index ffe34bfad4d..4996d49f463 100755 --- a/scripts/ascii-checks.sh +++ b/scripts/ascii-checks.sh @@ -2,7 +2,8 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the Apache 2.0 License. -# Checks that source files contain only ASCII characters. +# Checks that source and agent instruction files contain only ASCII characters, +# except for Lean source and grandfathered Unicode lines. # Non-ASCII punctuation/ligatures (em/en dashes, smart quotes, arrows, the fi # ligature, ...) frequently slip into comments, docstrings and string literals # via copy-paste or AI-generated text. This is a deterministic counterpart to @@ -44,55 +45,77 @@ EXTENSIONS=( ) # # Deliberately excluded suffixes (and why): +# - Lean source (lean): Unicode syntax and identifiers are idiomatic. # - Prose documentation (md, rst, txt): human-authored prose where non-ASCII -# (em dashes, accented author names, mathematical symbols) is legitimate. +# is legitimate. Agent instruction Markdown is included explicitly below. # - Binary / generated / vendored data (committed, cose, pem, png, pdf, ico, # lock, csv, numbered raft scenario fixtures, everything under 3rdparty/): # not human-edited source, so an ASCII check is meaningless or harmful. -# Files that intentionally contain non-ASCII characters (for example box-drawing -# or block glyphs used to render terminal charts/visualisations, or symbolic -# state labels). These are excluded from the check. -ALLOWLIST=( - "python/src/ccf/ledger_viz.py" # overline glyph in rendered ledger output - "js/ccf-app/doc/theme/partials/analytics.hbs" # decorative arrow in doc link - "tla/consensus/MCAliases.tla" # symbolic state glyphs used as model values +# SHA-256 hashes of existing Unicode lines, excluding their line endings. +# Consume each hash once so neither edited lines nor extra copies are exempt. +# Do not expand this baseline to allow new Unicode outside Lean. +declare -A LEGACY_NON_ASCII_LINES=( + ["python/src/ccf/ledger_viz.py"]="da452fa6d2ee3717bf92ca53b9225a3aa53ba66e838e39decba2d50edc539855" + ["js/ccf-app/doc/theme/partials/analytics.hbs"]="d22a72116e0f20860074017113d763917f4516d95a566d9ab90464f60ec7f144" + ["tla/consensus/MCAliases.tla"]=$'b9acbf868d048a06807d3d7bd7d321610a271281285476e24d6c646fe88ce637\n57f4a8bc014081bb7465cb339edf54e506ced1cfabf26e468dc96b1dedab87af' ) -is_allowlisted() { - local file="$1" - for allowed in "${ALLOWLIST[@]}"; do - if [ "$file" == "$allowed" ]; then - return 0 - fi - done - return 1 -} - -# Build the git ls-files glob arguments from the extension list. -globs=() +globs=( + "CMakeLists.txt" "*/CMakeLists.txt" + ".github/copilot-instructions.md" + ".github/instructions/*.md" + ".github/skills/*.md" + ".github/agents/*.md" + "AGENTS.md" "*/AGENTS.md" + "CLAUDE.md" "*/CLAUDE.md" + "GEMINI.md" "*/GEMINI.md" +) for ext in "${EXTENSIONS[@]}"; do globs+=("*.$ext") done -failed=0 -while IFS= read -r file; do - if is_allowlisted "$file"; then - continue - fi - # Report each offending line with its line number. - matches=$(LC_ALL=C grep -nP '[^\x00-\x7F]' "$file" 2>/dev/null) || continue - if [ -n "$matches" ]; then - failed=1 - echo "Non-ASCII characters found in $file:" - echo "$matches" - fi -done < <(git ls-files "${globs[@]}" | grep -v -e '^3rdparty/') +if git ls-files -z "${globs[@]}" | { + failed=0 + while IFS= read -r -d '' file; do + # Missing tracked files may have been deleted in the working tree. + if [[ "$file" == 3rdparty/* || ! -e "$file" ]]; then + continue + fi -if [ "$failed" -ne 0 ]; then - echo "Replace non-ASCII characters with their plain ASCII equivalents." - echo "If the non-ASCII content is intentional, add the file to ALLOWLIST in scripts/ascii-checks.sh." + if matches=$(LC_ALL=C grep -nP '[^\x00-\x7F]' "$file"); then + remaining_legacy=$'\n'"${LEGACY_NON_ASCII_LINES[$file]:-}"$'\n' + reported=0 + while IFS= read -r match; do + if [[ -n "${LEGACY_NON_ASCII_LINES[$file]:-}" ]]; then + line=${match#*:} + if ! digest=$(printf '%s' "${line%$'\r'}" | sha256sum); then + echo "Could not check grandfathered Unicode in $file" >&2 + failed=1 + continue + fi + legacy_hash=$'\n'"${digest%% *}"$'\n' + if [[ "$remaining_legacy" == *"$legacy_hash"* ]]; then + remaining_legacy=${remaining_legacy/"$legacy_hash"/$'\n'} + continue + fi + fi + if [ "$reported" -eq 0 ]; then + echo "Non-ASCII characters found in $file:" + reported=1 + fi + echo "$match" + failed=1 + done <<< "$matches" + elif [ "$?" -ne 1 ]; then + failed=1 + fi + done + exit "$failed" +}; then + echo "All checked files satisfy the ASCII policy." +else + echo "Outside Lean source (*.lean), use plain ASCII or language-appropriate ASCII escapes for required Unicode data." + echo "Do not add file-wide exemptions or expand the grandfathered Unicode baseline." exit 1 fi - -echo "All checked files contain only ASCII characters!" diff --git a/scripts/ascii-policy-tests.sh b/scripts/ascii-policy-tests.sh new file mode 100755 index 00000000000..38c51104641 --- /dev/null +++ b/scripts/ascii-policy-tests.sh @@ -0,0 +1,100 @@ +#!/bin/bash +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the Apache 2.0 License. + +set -euo pipefail + +SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" +ROOT_DIR=$( dirname "$SCRIPT_DIR" ) +TEST_ROOT=$(mktemp -d) +trap 'rm -rf "$TEST_ROOT"' EXIT + +unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_CONFIG_COUNT +export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 +git -c init.defaultBranch=ascii-policy-tests init -q "$TEST_ROOT" +mkdir -p "$TEST_ROOT/scripts" +cp "$SCRIPT_DIR/ascii-checks.sh" "$TEST_ROOT/scripts/ascii-checks.sh" + +legacy_files=( + "python/src/ccf/ledger_viz.py" + "js/ccf-app/doc/theme/partials/analytics.hbs" + "tla/consensus/MCAliases.tla" +) +for file in "${legacy_files[@]}"; do + mkdir -p "$TEST_ROOT/$(dirname "$file")" + cp "$ROOT_DIR/$file" "$TEST_ROOT/$file" +done +git -C "$TEST_ROOT" add . + +check() { + local expected=$1 + local description=$2 + local actual=0 + local output + output=$(cd "$TEST_ROOT" && bash scripts/ascii-checks.sh 2>&1) || actual=$? + if [ "$actual" -ne "$expected" ]; then + printf '%s: expected exit %s, got %s\n%s\n' "$description" "$expected" "$actual" "$output" >&2 + exit 1 + fi +} + +check 0 "Existing Unicode remains accepted" + +ascii_files=( + "src/example.py" + "src/example.rs" + "CMakeLists.txt" + "src/CMakeLists.txt" + ".github/copilot-instructions.md" + ".github/instructions/nested/example.instructions.md" + ".github/skills/example/SKILL.md" + ".github/skills/example/references/guide.md" + ".github/agents/reviewer.md" + "AGENTS.md" + "src/AGENTS.md" + "CLAUDE.md" + "GEMINI.md" +) +for file in "${ascii_files[@]}"; do + mkdir -p "$TEST_ROOT/$(dirname "$file")" + printf '\342\206\222\n' > "$TEST_ROOT/$file" + git -C "$TEST_ROOT" add -- "$file" + check 1 "Reject Unicode in $file" + printf '%s\n' 'ASCII text with an escape: "\u2192"' > "$TEST_ROOT/$file" + check 0 "Accept ASCII escapes in $file" +done + +for file in proof.lean nested/proof.lean doc/guide.md 3rdparty/example.py; do + mkdir -p "$TEST_ROOT/$(dirname "$file")" + printf '\342\210\200 n : Nat, n = n\n' > "$TEST_ROOT/$file" + git -C "$TEST_ROOT" add -- "$file" +done +check 0 "Lean, prose and vendored files remain exempt" + +for file in "${legacy_files[@]}"; do + printf '\342\206\222\n' >> "$TEST_ROOT/$file" + check 1 "Reject additional Unicode in $file" + cp "$ROOT_DIR/$file" "$TEST_ROOT/$file" + + match=$(LC_ALL=C grep -nPm1 '[^\x00-\x7F]' "$ROOT_DIR/$file") + printf '%s\n' "${match#*:}" >> "$TEST_ROOT/$file" + check 1 "Reject duplicate grandfathered lines in $file" + + awk -v target="${match%%:*}" 'NR == target { $0 = $0 " changed" } { print }' \ + "$ROOT_DIR/$file" > "$TEST_ROOT/$file" + check 1 "Reject edits to grandfathered lines in $file" + cp "$ROOT_DIR/$file" "$TEST_ROOT/$file" + + printf '\n# ASCII addition\n' >> "$TEST_ROOT/$file" + check 0 "Accept ASCII-only additions in $file" + cp "$ROOT_DIR/$file" "$TEST_ROOT/$file" +done + +rm "$TEST_ROOT/src/example.py" +check 0 "Deleted tracked files do not fail" +mkdir "$TEST_ROOT/src/example.py" +check 1 "Read errors must fail the check" +rmdir "$TEST_ROOT/src/example.py" +GIT_DIR="$TEST_ROOT/missing.git" check 1 "Git errors must fail the check" + +echo "ASCII policy regression tests passed" diff --git a/scripts/ci-checks.sh b/scripts/ci-checks.sh index f3036923655..10c90c27e6f 100755 --- a/scripts/ci-checks.sh +++ b/scripts/ci-checks.sh @@ -41,6 +41,7 @@ CHECKS=( "Includes:includes-checks.sh" "Release notes:release-notes-checks.sh" "Non-ASCII characters:ascii-checks.sh" + "ASCII policy regression tests:ascii-policy-tests.sh" "C/C++ format:cpp-format-checks.sh" "TypeScript, JavaScript, Markdown, TypeSpec, YAML and JSON format:prettier-checks.sh" "OpenAPI:openapi-checks.sh"