Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions .github/skills/code-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Code Review Skill

This skill enables Copilot to perform context-aware code reviews on pull requests in this repository.

## Skill Configuration

- **Skill type**: `code-review`
- **Trigger**: Automatically on pull request open/synchronize, or on `@copilot review` comment

## Review Focus Areas

When reviewing pull requests in this repository, Copilot will check:

1. **C++ Safety & Correctness**
- RAII and resource management (no raw `new`/`delete` without justification)
- Exception safety and error propagation
- Undefined behaviour, type-limits, sign-compare issues
- Thread safety and lock ordering

2. **Compiler Diagnostic Hygiene**
- No new `-Wunused-*`, `-Wmissing-field-initializers`, `-Wsign-compare`, `-Wswitch`, `-Wformat-truncation` warnings
- No suppression pragmas without accompanying justification comment

3. **Security**
- Input validation on trust-boundary crossings (T3/T4/T5)
- No new raw pointer arithmetic or unchecked casts
- Strict-aliasing compliance (`std::bit_cast` over `reinterpret_cast`)

4. **Code Style**
- Designated initialisers for aggregate types with ≥3 fields
- Dead code removal preferred over `[[maybe_unused]]` annotation
- Modern C++20 features where applicable

5. **Documentation**
- Doxygen `@brief`/`@param`/`@return`/`@throws` for all new public APIs
- Intent and constraint comments ("why"), not implementation paraphrase ("what")

## References

- `.github/instructions/cpp-best-practices.instructions.md`
- `.github/instructions/documentation-enforcement.instructions.md`
- `ai_context/developer_llm_wiki/MODULES_AND_APIS.md`
9 changes: 6 additions & 3 deletions include/cdc/cdc_metrics.h
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,13 @@ class ScopedTimer {
};

/**
* @brief Helper macro for easy latency recording
* @brief Helper macro for easy latency recording.
* Uses __LINE__ to generate a unique variable name per call site and
* avoid variable-shadowing warnings (MSVC C4456, etc.).
*/
#define CDC_MEASURE_LATENCY(histogram) \
ScopedTimer _timer(histogram)
#define CDC_MEASURE_LATENCY_IMPL(histogram, line) \
ScopedTimer CDC_timer_##line(histogram)
#define CDC_MEASURE_LATENCY(histogram) CDC_MEASURE_LATENCY_IMPL(histogram, __LINE__)

} // namespace cdc
} // namespace themis
2 changes: 0 additions & 2 deletions src/analytics/distributed_analytics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,6 @@ DistributedAnalyticsSharding::executeDistributed(const OLAPQuery &query) {
double max_shard_ms = 0.0;
std::string slow_shard;
bool any_timeout = false;
bool any_circuit_open = false;
size_t failed_count = 0;
std::vector<std::string> open_cb_shards;

Expand All @@ -1044,7 +1043,6 @@ DistributedAnalyticsSharding::executeDistributed(const OLAPQuery &query) {
}
}
if (si.circuit_state == CircuitBreakerState::OPEN) {
any_circuit_open = true;
open_cb_shards.push_back(si.shard_id);
}
if (si.execution_time_ms > max_shard_ms) {
Expand Down
42 changes: 2 additions & 40 deletions src/analytics/process_mining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ ProcessMining::discoverProcessFromCollection(std::string_view collection, const

// ===== Mining Algorithms =====

DiscoveredProcess ProcessMining::runAlphaMiner(const EventLog &log, [[maybe_unused]] const MiningConfig &config) {
DiscoveredProcess ProcessMining::runAlphaMiner(const EventLog &log, const MiningConfig &) {
DiscoveredProcess process;
process.name = "Alpha Miner Result";

Expand Down Expand Up @@ -1323,22 +1323,6 @@ DiscoveredProcess ProcessMining::runHeuristicMiner(const EventLog &log, const Mi

namespace {

// Helper: build activity-to-id mapping for a sub-log
std::unordered_map<std::string, int> buildActivityIds(const std::vector<ProcessTrace> &traces) {
std::unordered_map<std::string, int> ids;
int next = 0;
for (const auto &t : traces) {
for (const auto &e : t.events) {
// NOTE: ids.find() on unordered_map is O(1) average case, not O(n).
// Scanner reports O(n²) false positive due to not recognizing unordered_map complexity.
if (ids.find(e.activity) == ids.end()) {
ids[e.activity] = next++;
}
}
}
return ids;
}

// DFG for a sub-log (activity names → pair of frequency maps)
struct SubDFG {
std::set<std::string> activities;
Expand Down Expand Up @@ -1379,28 +1363,6 @@ SubDFG buildSubDFG(const std::vector<ProcessTrace> &traces, double noise_thresho
return dfg;
}

// Check reachability in DFG (BFS)
bool dfgReachable(const SubDFG &dfg, const std::string &from, const std::string &to) {
std::queue<std::string> q;
std::unordered_set<std::string> visited;
q.push(from);
visited.insert(from);
while (!q.empty()) {
auto cur = q.front();
q.pop();
if (cur == to) {
return true;
}
for (const auto &[k, _] : dfg.freq) {
if (k.first == cur && !visited.count(k.second)) {
visited.insert(k.second);
q.push(k.second);
}
}
}
return false;
}

// Find weakly connected components of the DFG (undirected)
std::vector<std::set<std::string>> findComponents(const SubDFG &dfg) {
std::unordered_map<std::string, std::string> parent;
Expand Down Expand Up @@ -2871,7 +2833,7 @@ ProcessMining::findSimilarPatterns(const std::vector<std::string> &pattern, cons
}

std::pair<ProcessMining::Status, std::vector<ProcessMining::GeoProcessCluster>>
ProcessMining::discoverGeoVariants(const EventLog &log, [[maybe_unused]] double cluster_radius_km) {
ProcessMining::discoverGeoVariants(const EventLog &log, double) {
std::vector<GeoProcessCluster> clusters;
std::set<std::string> processed_variants;

Expand Down
55 changes: 0 additions & 55 deletions src/analytics/streaming_join.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,21 +71,6 @@ size_t findColumnIndex(const ColumnBatch &batch, const std::string &name) {
return SIZE_MAX;
}

/// Build a list of column indices from a list of names.
/// Throws std::invalid_argument if any name is missing.
std::vector<size_t> resolveColumns(const ColumnBatch &batch, const std::vector<std::string> &names) {
std::vector<size_t> idxs;
idxs.reserve(names.size());
for (const auto &n : names) {
size_t idx = findColumnIndex(batch, n);
if (idx == SIZE_MAX) {
throw std::invalid_argument("StreamingJoin: column not found: " + n);
}
idxs.push_back(idx);
}
return idxs;
}

/// Make a composite key from given columns at a specific row.
std::string makeCompositeKey(const std::vector<std::shared_ptr<Column>> &cols, const std::vector<size_t> &key_indices,
size_t row) {
Expand All @@ -98,36 +83,6 @@ std::string makeCompositeKey(const std::vector<std::shared_ptr<Column>> &cols, c
return key;
}

/// Project columns from `src` batch by name list (empty = all columns).
/// Returns a list of (new Column, original name) pairs for building result.
std::vector<std::shared_ptr<Column>> projectColumns(const ColumnBatch &src,
const std::vector<std::string> &select_names) {
std::vector<std::shared_ptr<Column>> result;
if (select_names.empty()) {
result.reserve(src.columnCount());
for (size_t i = 0; i < src.columnCount(); ++i) {
result.push_back(src.getColumnAt(i));
}
} else {
result.reserve(select_names.size());
for (const auto &n : select_names) {
auto col = src.getColumn(n);
if (!col) {
throw std::invalid_argument("StreamingJoin::project: column not found: " + n);
}
result.push_back(col);
}
}
return result;
}

/// Append a single null row to every column in `cols`.
void appendNullRow(std::vector<std::shared_ptr<Column>> &cols) {
for (auto &c : cols) {
c->appendNull();
}
}

/// Append row `src_row` from `src_col` to `dst_col`.
void appendRow(Column &dst, const Column &src, size_t src_row) {
if (src.isNull(src_row)) {
Expand All @@ -153,16 +108,6 @@ void appendRow(Column &dst, const Column &src, size_t src_row) {
}
}

/// Create empty output columns matching the schema of `src_cols`.
std::vector<std::shared_ptr<Column>> makeEmptyOutputCols(const std::vector<std::shared_ptr<Column>> &src_cols) {
std::vector<std::shared_ptr<Column>> result;
result.reserve(src_cols.size());
for (const auto &c : src_cols) {
result.push_back(std::make_shared<Column>(c->name(), c->type()));
}
return result;
}

} // anonymous namespace

// ============================================================================
Expand Down
2 changes: 1 addition & 1 deletion src/api/graphql.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ themis::Result<Operation> Parser::parseOperation() {
skipWhitespace();

// Optional operation name
if (op.type != OperationType::Query || !peek('{') && !peek('(')) {
if (op.type != OperationType::Query || (!peek('{') && !peek('('))) {
auto nameResult = parseName();
if (nameResult) {
op.name = *nameResult;
Expand Down
10 changes: 0 additions & 10 deletions src/auth/jwks_security.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,6 @@ std::string base64Encode(const unsigned char* data, size_t len) {
}

// Read file content
std::string readFile(const std::string& path) {
std::ifstream file(path, std::ios::binary);
if (!file) {
throw std::runtime_error("Failed to open file: " + path);
}

std::stringstream buffer;
buffer << file.rdbuf();
return buffer.str();
}

// Check file exists
bool fileExists(const std::string& path) {
Expand Down
1 change: 1 addition & 0 deletions src/auth/jwt_validator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ JWTValidator::JWTValidator(const std::string &jwks_url)
.expected_audience = std::nullopt,
.cache_ttl = std::chrono::seconds(600),
.clock_skew = std::chrono::seconds(60),
.revoked_kids = {},
.require_issuer_validation = false,
.require_audience_validation = false,
}},
Expand Down
2 changes: 0 additions & 2 deletions src/auth/totp_secret_encryption.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,6 @@ TOTPSecretRotationManager::getActiveSecrets(const std::string &user_id) {
return active_secrets;
}

auto now = std::chrono::system_clock::now();

for (const auto &secret : it->second) {
if (isSecretValid(secret)) {
active_secrets.push_back(secret);
Expand Down
2 changes: 0 additions & 2 deletions src/cdc/changefeed_buffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ Changefeed::ChangeEvent ChangefeedBuffer::recordEvent(Changefeed::ChangeEvent ev
if (config_.compress_payloads && event.value.has_value()) {
size_t payload_size = event.value->size();
if (payload_size > config_.compression_threshold_bytes) {
#pragma warning(suppress: 4456)
CDC_MEASURE_LATENCY(metrics_.compression_latency);
try {
auto compressed = utils::zstd_compress(*event.value, 3);
Expand Down Expand Up @@ -263,7 +262,6 @@ size_t ChangefeedBuffer::flushBuffer(Changefeed::ChangeEventType event_type, Eve
if (event.metadata.contains("_compressed") && event.metadata["_compressed"] == true) {
if (event.value.has_value()) {
{
#pragma warning(suppress: 4456)
CDC_MEASURE_LATENCY(metrics_.decompression_latency);
try {
std::vector<uint8_t> compressed_data(event.value->begin(), event.value->end());
Expand Down
4 changes: 2 additions & 2 deletions src/config/config_file_watcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,12 @@ void ConfigFileWatcher::stop() {
#if defined(__linux__)
if (pipe_write_fd_ != -1) {
char dummy = 1;
(void)write(pipe_write_fd_, &dummy, 1);
[[maybe_unused]] ssize_t rc = write(pipe_write_fd_, &dummy, 1);
}
#elif defined(__APPLE__)
if (pipe_write_fd_ != -1) {
char dummy = 1;
(void)write(pipe_write_fd_, &dummy, 1);
[[maybe_unused]] ssize_t rc = write(pipe_write_fd_, &dummy, 1);
}
#elif defined(_WIN32)
if (stop_event_ != nullptr) {
Expand Down
8 changes: 0 additions & 8 deletions src/exporters/huggingface_hub_client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -413,8 +413,6 @@ HubUploadResult HuggingFaceHubClient::uploadDataset(const std::string &dataset_d
long timeout_seconds;
int max_retries;
int retry_delay_ms;
bool create_repo;
bool private_repo;
std::shared_ptr<themis::utils::AuditLogger> audit_log;
std::shared_ptr<ExporterMetrics> metrics;
std::string requesting_user;
Expand All @@ -427,8 +425,6 @@ HubUploadResult HuggingFaceHubClient::uploadDataset(const std::string &dataset_d
timeout_seconds = config_.timeout_seconds;
max_retries = config_.max_retries;
retry_delay_ms = config_.retry_delay_ms;
create_repo = config_.create_repo;
private_repo = config_.private_repo;
audit_log = config_.audit_log;
metrics = config_.metrics;
requesting_user = config_.requesting_user;
Expand Down Expand Up @@ -634,8 +630,6 @@ HubUploadResult HuggingFaceHubClient::uploadShards(const std::vector<MemoryShard
long timeout_seconds;
int max_retries;
int retry_delay_ms;
bool create_repo;
bool private_repo;
std::shared_ptr<themis::utils::AuditLogger> audit_log;
std::shared_ptr<ExporterMetrics> metrics;
std::string requesting_user;
Expand All @@ -648,8 +642,6 @@ HubUploadResult HuggingFaceHubClient::uploadShards(const std::vector<MemoryShard
timeout_seconds = config_.timeout_seconds;
max_retries = config_.max_retries;
retry_delay_ms = config_.retry_delay_ms;
create_repo = config_.create_repo;
private_repo = config_.private_repo;
audit_log = config_.audit_log;
metrics = config_.metrics;
requesting_user = config_.requesting_user;
Expand Down
3 changes: 3 additions & 0 deletions src/gpu/p2p_transfer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,16 @@ bool devicesValid(int src, int dst, const std::vector<DeviceInfo> &devs) {

// Determine the preferred interconnect for the device pair (best-effort).
// Uses GPUClusterTopology::detect() when no explicit topology is provided.
// Only needed in GPU builds — the CPU simulation path does not track interconnect types.
#if defined(THEMIS_ENABLE_CUDA) || defined(THEMIS_ENABLE_HIP)
InterconnectType detectInterconnect(int src, int dst, const std::vector<DeviceInfo> &devs) {
if (!devicesValid(src, dst, devs)) {
return InterconnectType::CPU;
}
auto topo = GPUClusterTopology::detect(devs);
return topo.preferredInterconnect(src, dst);
}
#endif

} // namespace

Expand Down
2 changes: 1 addition & 1 deletion src/graph/scheduled_edge_refresh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ void ScheduledGraphEdgeRefreshEngine::schedulerLoop() {

{
std::lock_guard<std::mutex> lock(cycle_mutex_);
auto stats = runRefreshCycle();
runRefreshCycle();
// logged inside runRefreshCycle
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/importers/debezium_cdc_importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,6 @@ ImportStats DebeziumCDCImporter::importData(

// Delegate to streamEvents() with a ThemisDB-write sink.
ImportStats stats{};
const auto start_time = std::chrono::steady_clock::now();

// Delegate to streamEvents() which returns detailed stats including
// structured_errors for the no-build-flag guard path. Return the
Expand Down Expand Up @@ -268,7 +267,6 @@ ImportStats DebeziumCDCImporter::importData(
ImportStats DebeziumCDCImporter::streamEvents(const ImportOptions& options,
CDCEventCallback callback) {
ImportStats stats{};
const auto start_time = std::chrono::steady_clock::now();

const auto deadline = (options.deadline_ms > 0)
? std::optional<std::chrono::steady_clock::time_point>(
Expand Down
1 change: 0 additions & 1 deletion src/index/ann_index.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,6 @@ std::vector<AnnSearchResult> ScaNN::search(const float* query, [[maybe_unused]]

// ---- Step 2: AH scan within selected leaves ----
size_t reorder_n = std::max(static_cast<size_t>(k), cfg_.reorder_num_neighbors);
using Candidate = std::pair<float, size_t>; // (dist, global_idx_in_leaf)
struct FullCandidate { float dist; const Leaf* leaf; size_t idx; };
std::vector<FullCandidate> candidates;
candidates.reserve(reorder_n * 2);
Expand Down
2 changes: 0 additions & 2 deletions src/index/approximate_radius_search.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ ApproximateRadiusSearch::searchWithTargetCount(
// Binary search on radius to find the right value that gives ~target_count results
float min_radius = MIN_SEARCH_RADIUS;
float max_radius = config.radius * RADIUS_MULTIPLIER;
float best_radius = config.radius;
SearchResult best_result;
size_t best_count_diff = std::numeric_limits<size_t>::max();

Expand All @@ -214,7 +213,6 @@ ApproximateRadiusSearch::searchWithTargetCount(
// Track best result
if (count_diff < best_count_diff) {
best_count_diff = count_diff;
best_radius = test_radius;
best_result = std::move(result.value());
}

Expand Down
Loading
Loading