NOJIRA: process overhead, merge develop -> main - #100
Closed
brendanobra wants to merge 111 commits into
Closed
brendanobra wants to merge 111 commits into
brendanobra wants to merge 111 commits into
Conversation
…nto feature/openspec
This reverts commit 6fe772c.
…t into openspec-guidelines
…port into openspec-guidelines
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The include reordering in the previous commit left gateway.cpp with a clang-format violation (include ordering). Reformat to comply.
canonicalPath was declared const, preventing NRVO (Named Return Value Optimisation) and implicit move on return. Coverity COPY_INSTEAD_OF_MOVE flags this as an unnecessary copy. Remove const so the compiler can apply NRVO/move on the return statement. All 129 tests passing. Clang-format compliant.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The test passes malformedFormat ('%') directly as the format string to
exercise the vsnprintf() error path. This triggers -Wformat-security
('format not a string literal and no format arguments') under -Wall.
Fix: pass a trailing 0 int argument. -Wformat-security only fires when
there are NO format arguments; with a dummy argument the warning is
suppressed. The test still exercises the malformed '%' format path.
All 129 unit tests passing. Clang-format compliant.
1. **gateway.cpp**: eliminate string allocation under queue_mtx in checkPromises()
- std::string construction under the lock could throw std::bad_alloc after
queue.erase(), leaving the caller's promise unfulfilled (blocked forever).
- Now stores only the std::shared_ptr<Caller> in timedOut; the log message is
built with format args (id=%u method='%s') after releasing the lock.
2. **transport.cpp**: fix '@' detection in URL redaction
- Previous code used find('@') anywhere in the URL, which mis-redacts valid
URLs with '@' in the path (e.g. wss://host/path@segment).
- Now finds '@' only within the authority section (between '://' and the first
'/', '?', or '#'), matching RFC-3986 userinfo semantics.
3. **logger.cpp**: cache validated log-file path per-thread to reduce overhead
- resolveLogFilePathFromEnvironment() previously called realpath(), open(),
and multiple syscalls on every log line when LOG_FILE is set.
- Now uses thread_local cache: realpath() is skipped when the env var value
is unchanged since the last call in this thread. Changes to the env var are
picked up on the next log call in each thread.
All 129 unit tests passing. Clang-format compliant.
1. **logger.cpp L177**: avoid double-slash when canonicalParent is '/'
- 'string(canonicalParent) + "/" + filename' produced "//filename" when
the parent resolved to root.
- Now appends '/' only when canonicalParent does not already end with it.
2. **logger.cpp L249**: make tryWriteToConfiguredLogFile() exception-safe
- std::string construction can throw std::bad_alloc; an uncaught exception
would propagate through Logger::log() and potentially terminate the process.
- Logging must be best-effort: wrap string construction in try/catch and
return false (caller falls back to stderr) on any exception.
3. **logger.h L65**: narrow misleading macro comment
- 'Arguments are only evaluated if log level is enabled' implied the level
argument itself is skipped, which is wrong (level is needed for the check).
- Changed to 'Variadic format arguments are only evaluated...' for clarity.
All 129 unit tests passing. Clang-format compliant.
1. **logger.h**: capture 'level' into a local variable in FIREBOLT_LOG macro
- 'level' was evaluated twice: once in isLogLevelEnabled(level) and again
in Logger::log(level, ...). If a caller ever passes an expression with
side effects as the level argument, this would cause double evaluation.
- Introduce '_fb_level = (level)' inside the do{} block and reuse it.
2. **logger.cpp**: add O_NOFOLLOW to openat() in tryWriteToConfiguredLogFile
- Without O_NOFOLLOW, a symlink at the log filename could silently redirect
writes to an arbitrary file (security risk for privileged processes).
- O_NOFOLLOW causes openat() to return ELOPP if the target is a symlink;
the write is then skipped and falls back to stderr.
3. **gateway.cpp**: use map key 'id' in cancelAll() log instead of 'caller->id'
- The structured binding 'auto& [id, caller]' introduced 'id' (map key),
but the log used 'caller->id' instead, leaving 'id' unused.
- This triggers -Wunused-variable under the project's -Wall -Wextra build.
- Changed to use 'id' (the same value, but uses the bound variable).
All 129 unit tests passing. Clang-format compliant.
snprintf returns the *would-have-written* length even when the output is truncated. Blindly adding this to 'len' can push len beyond sizeof(formattedMsg), making the next (formattedMsg + len) out-of-bounds. Negative returns (-1 on error) convert to a huge size_t, with the same result. Fix: introduce a snAppend lambda that advances len by the actual number of bytes written — min(ret, remaining - 1) — and ignores negative returns. This ensures len never exceeds sizeof(formattedMsg) - 1 regardless of how many format segments are appended or how large the individual strings are. All 129 unit tests passing. Clang-format compliant.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1. **gateway.cpp**: wrap watchdog checkPromises() in try/catch
- An exception from checkPromises() (vector growth, promise::set_value(),
logging allocation) would escape the thread function and call std::terminate.
- Added try/catch(std::exception) and catch(...) inside the watchdog loop;
exceptions are logged and the watchdog continues rather than crashing the
process.
2. **logger.cpp**: eliminate redundant string comparison in cache fast path
- The previous fast path did two full string comparisons:
if (raw == cachedEnvValue || std::strcmp(raw, cachedEnvValue.c_str()) == 0)
Both sides compare all characters of the env var string.
- Reduced to a single comparison:
if (cachedEnvValue == raw)
std::string::operator==(const char*) is one pass; no second comparison
needed.
All 129 unit tests passing. Clang-format compliant.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Two of the four accepted suggestions introduced regressions: 1. **gateway.cpp (c158b2c)**: The autofix removed 'timedOut.push_back(it->second)' from checkPromises(), leaving only 'it = queue.erase(it)'. This erased timed-out callers from the queue WITHOUT fulfilling their promises, so waiting threads blocked indefinitely instead of receiving Timedout errors. Broke: GatewayUTest.RequestTimeout, GatewayUTest.WaitTimeConfiguration. Fix: restore timedOut.push_back() before the erase. 2. **logger.cpp (6455a0f)**: Changed 'return ret == size' to 'return ret > 0', treating partial writes as success. A partial write leaves a truncated line in the log file; the original semantics (fail on partial) are correct so the caller falls back to stderr with the full line intact. Fix: restore 'return ret == static_cast<ssize_t>(line.size())'. The other two suggestions (strlen overflow guards in parseEnvLogLevel and isEnvLogDisabled, and EINTR retry in write()) are kept as they are correct. All 129 unit tests passing. Clang-format compliant.
When firebolt-cpp-transport is used as a git submodule, its .git entry is a file pointer to ../.git/modules/firebolt-cpp-transport, which does not exist inside the Docker container (only the submodule directory is mounted). 'git ls-files' therefore fails with 'not a git repository'. Replace 'git ls-files -- *.cpp *.h' with a 'find' over the explicit source directories (src/, include/, test/) to avoid requiring git inside the Docker context. Behaviour is equivalent for this repo's layout.
1. **gateway.cpp checkPromises()**: reserve timedOut + move debug log outside lock
- timedOut.push_back() could reallocate under queue_mtx on vector growth;
reserve(queue.size()) inside the lock guarantees no reallocation during
the erase loop. If reserve() itself throws, no entries have been erased.
- '[watchdog] pending queue size' debug log moved outside the lock scope
so it doesn't block I/O under the mutex.
2. **gateway.cpp request()**: capture queue size then log outside lock
- '[request] queued ... pending=%zu' was logged while holding queue_mtx.
Captured queue.size() into a local before releasing the lock, then log
after, consistent with the pattern used in checkPromises/cancelAll.
3. **gateway.cpp connect()**: redact URL before logging at NOTICE level
- 'Connecting to url = ...' logged the full URL which may contain
credentials in userinfo or tokens in query/fragment. Applied the same
authority-scoped redaction used by transport.cpp.
4. **logger.cpp**: copy getenv() result to buffer before strlen/strcmp
- strlen() and strcmp() were called on the raw getenv() pointer before
the buffer copy, which the comment claimed to avoid. Now strncpy runs
first; all subsequent operations use the buffer. Truncation is detected
by checking raw[sizeof(buffer)-1] != '\0'.
All 129 unit tests passing. Clang-format compliant.
If the gateway sends a malformed JSON-RPC error payload (missing 'code' or 'message' keys, or wrong types), nlohmann::json's operator[] throws. Without a catch, the exception escapes the calling thread and terminates the process. Added defensive extraction with explicit contains()/is_number()/is_string() checks inside a try/catch. Falls back to Firebolt::Error::General / 'unknown error' when fields are absent or malformed, then fulfills the promise with that safe fallback so the waiting caller unblocks rather than hanging. All 129 unit tests passing. Clang-format compliant.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
raw[sizeof(buffer)-1] is undefined behavior when the environment string is shorter than PATH_MAX: it reads past the string's null terminator. Fix: use strlen(raw) to check the length first (reads only up to the null terminator — always safe), then memcpy with the exact length + 1 for the null terminator. This avoids both the UB probe and strncpy's silent truncation-without-detection. All 129 unit tests passing. Clang-format compliant.
The unit_tests CI job was failing during coverage report generation: (ERROR) Exiting because of parse errors. Use --gcov-ignore-parse-errors=negative_hits.warn gcovr was crashing on negative hit counts in gcov output, which is a known compatibility issue between newer gcovr versions and coverage data from recent GCC/gcov. The tests themselves all passed (129/129). Add the flag to suppress the parse error and allow the coverage report to complete.
…oging Nojira/runtime configurable logging
Contributor
There was a problem hiding this comment.
Pull request overview
This PR brings Firebolt C++ Transport’s main branch up to date with develop, adding WebSocket header support, richer runtime logging controls (env overrides + optional file sink), improved disconnect semantics (cancel pending requests), and tooling/docs around OpenSpec and coverage gating.
Changes:
- Add custom request-header injection and response-header retrieval (
getResponseHeader) throughTransport→IGateway, plus related tests and docs. - Enhance logging: env-controlled log level (including “off”), safer formatting behavior, optional file sink with path hardening, and updated log prefix/banner.
- Extend CI/dev tooling: coverage gate + baseline updater, optional local coverage in
test.sh, formatting script updates, version/banner metadata generation, and OpenSpec artifacts/presentations.
Reviewed changes
Copilot reviewed 56 out of 58 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/transportTest.cpp | Adds an integration test for header injection and response header retrieval. |
| test/unit/loggerTest.cpp | Expands logger tests for env overrides, “off” behavior, file sink, and formatting robustness. |
| test/unit/helperTest.cpp | Updates MockGateway to include the new getResponseHeader method. |
| test/unit/gatewayTest.cpp | Adds regression test ensuring disconnect() cancels pending requests (no hanging futures). |
| test/CMakeLists.txt | Defines FIREBOLT_TRANSPORT_TESTING consistently for library + unit test target to avoid ODR issues. |
| test.sh | Adds optional gcovr coverage summary output when ENABLE_COVERAGE=1. |
| src/transport.h | Extends Transport::connect() to accept headers; adds getResponseHeader() and test hooks. |
| src/transport.cpp | Implements header injection, response header caching, and additional lifecycle logging. |
| src/logger.cpp | Adds env-based log level resolution, “off” suppression, safer formatting, and hardened file sink support. |
| src/gateway.cpp | Passes headers into transport connect, adds response-header passthrough, cancels pending requests on disconnect, and expands diagnostics. |
| spec_coverage.md | Adds an OpenSpec coverage report output file. |
| README.md | Documents header support and runtime logging overrides. |
| presentations/transport-slides/slides.md | Adds a Slidev deck for the transport project overview. |
| presentations/transport-slides/package.json | Adds Slidev dependencies/scripts for the transport slide deck. |
| presentations/openspec-with-copliot-experience/slides.md | Adds a Slidev deck describing an OpenSpec + Copilot workflow experience. |
| presentations/openspec-with-copliot-experience/package.json | Adds Slidev dependencies/scripts for the OpenSpec experience deck. |
| package.json | Adds repo-level dev dependency (Playwright Chromium) for Slidev export workflows. |
| openspec/specs/transport_recommendations_spec.md | Adds/updates transport recommendations spec content. |
| openspec/specs/transport_layer_spec.md | Adds/updates transport-layer spec content, including header retrieval mention. |
| openspec/specs/json_rpc_handling_spec.md | Adds JSON-RPC handling spec content. |
| openspec/specs/header_interfaces_spec.md | Adds header-interfaces spec content. |
| openspec/specs/cpp_specifics_spec.md | Adds C++ specifics spec content. |
| openspec/config.yaml | Adds OpenSpec configuration for template/coverage rules and repository context. |
| openspec/changes/archive/2026-03-16-add-header-support/tasks.md | Archives tasks for the header-support change. |
| openspec/changes/archive/2026-03-16-add-header-support/specs/header_support_spec.md | Archives the header-support spec. |
| openspec/changes/archive/2026-03-16-add-header-support/design.md | Archives header-support design notes. |
| openspec/changes/archive/2026-03-16-add-header-support/add_header_support_proposal.md | Archives the header-support proposal. |
| NOTICE | Adds attribution notice for OpenSpec material. |
| LICENSE | Appends MIT license text section for OpenSpec-related material. |
| include/firebolt/version.h.in | Adds git ref, build time, component name, and a banner string for runtime reporting. |
| include/firebolt/logger.h | Adds env log level resolver, atomic state, and macro guard to avoid evaluating disabled log args. |
| include/firebolt/gateway.h | Adds IGateway::getResponseHeader() to expose response header retrieval. |
| include/firebolt/config.h.in | Adds Config.headers for handshake header injection. |
| fmt.sh | Switches formatting file discovery to find over src/include/test. |
| cmake/version.cmake | Adds git describe/SHA + reproducible build timestamp generation for version header substitution. |
| .gitignore | Ignores pnpm-lock.yaml files. |
| .github/workflows/ci.yml | Produces lcov artifact, adds a coverage gate job, and updates baseline on develop. |
| .github/skills/openspec-templater/spec_template.md | Adds the canonical OpenSpec spec template. |
| .github/skills/openspec-templater/SKILL.md | Adds the OpenSpec templater skill documentation. |
| .github/skills/openspec-propose/SKILL.md | Adds the OpenSpec propose skill documentation. |
| .github/skills/openspec-explore/SKILL.md | Adds the OpenSpec explore skill documentation. |
| .github/skills/openspec-coverage/SKILL.md | Adds the OpenSpec coverage skill documentation. |
| .github/skills/openspec-coverage/openspec_coverage.py | Adds a script to compute spec coverage scoring/report. |
| .github/skills/openspec-archive-change/SKILL.md | Adds the OpenSpec archive-change skill documentation. |
| .github/skills/openspec-apply-change/SKILL.md | Adds the OpenSpec apply-change skill documentation. |
| .github/skills/init-slidedev-project/SKILL.md | Adds a skill for scaffolding Slidev projects. |
| .github/skills/export-presentation/SKILL.md | Adds a skill for exporting Slidev presentations. |
| .github/skills/add-slide/SKILL.md | Adds a skill for inserting new Slidev slides. |
| .github/skills/add-diagram/SKILL.md | Adds a skill for inserting Mermaid diagrams into Slidev decks. |
| .github/scripts/compare_coverage.py | Adds a Python script to compare coverage against a stored baseline and output a report. |
| .github/release/package-lock.json | Updates lockfile metadata name for the release tooling package. |
| .github/prompts/opsx-propose.prompt.md | Adds prompt content for the propose workflow. |
| .github/prompts/opsx-explore.prompt.md | Adds prompt content for explore mode. |
| .github/prompts/opsx-archive.prompt.md | Adds prompt content for archiving workflow. |
| .github/prompts/opsx-apply.prompt.md | Adds prompt content for apply workflow. |
| project_template/slides.md | Adds a Slidev project template starter slides file. |
| project_template/package.json | Adds a Slidev project template package.json. |
Files not reviewed (1)
- .github/release/package-lock.json: Generated file
Comment on lines
331
to
335
| client_ = std::make_unique<client>(); | ||
| connectionStatus_ = TransportState::NotStarted; | ||
| FIREBOLT_LOG_DEBUG("Transport", "[disconnect] transport reset complete, state=%d", | ||
| static_cast<int>(connectionStatus_.load())); | ||
| return Firebolt::Error::None; |
Comment on lines
+303
to
+305
| auto customHeader = transport.getResponseHeader("X-Test-Header"); | ||
| // Custom header may not be echoed by server, but should not crash | ||
| EXPECT_TRUE(customHeader == std::nullopt || customHeader.has_value()); |
Comment on lines
+206
to
+210
| MIT License | ||
|
|
||
| Copyright (c) YEAR COPYRIGHT HOLDER | ||
|
|
||
| Permission is hereby granted, free of charge, to any person obtaining a copy |
Comment on lines
50
to
+53
| Firebolt::Error connect(std::string url, MessageCallback onMessage, ConnectionCallback onConnectionChange, | ||
| std::optional<unsigned> transportLoggingInclude = std::nullopt, | ||
| std::optional<unsigned> transportLoggingExclude = std::nullopt); | ||
| std::optional<unsigned> transportLoggingExclude = std::nullopt, | ||
| const std::map<std::string, std::string>& headers = {}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.