From 358d6e76ab749a7808ce02089b1b5b8d6431fc82 Mon Sep 17 00:00:00 2001 From: Pxx500 Date: Thu, 20 Aug 2026 16:29:19 +0200 Subject: [PATCH 01/10] fix(mcp): recover unsent requests after daemon loss Signed-off-by: Pxx500 --- src/daemon/application.c | 27 +-- src/daemon/application.h | 5 +- src/daemon/frontend.c | 183 +++++++++++++++----- src/daemon/frontend.h | 22 ++- src/daemon/runtime.c | 14 +- src/daemon/runtime.h | 5 +- src/main.c | 60 +++++-- tests/test_daemon_frontend.c | 322 +++++++++++++++++++++++++++++++---- tests/test_daemon_runtime.c | 27 ++- 9 files changed, 547 insertions(+), 118 deletions(-) diff --git a/src/daemon/application.c b/src/daemon/application.c index 69ff52ec7..58fb6bb31 100644 --- a/src/daemon/application.c +++ b/src/daemon/application.c @@ -3116,7 +3116,7 @@ cbm_daemon_runtime_application_callbacks_t cbm_daemon_application_runtime_callba static cbm_daemon_runtime_application_status_t application_client_exchange_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, uint8_t *request, uint32_t request_length, uint8_t **response_out, - uint32_t *response_length_out, uint32_t timeout_ms) { + uint32_t *response_length_out, bool *request_sent_out, uint32_t timeout_ms) { uint8_t *response = NULL; uint32_t response_length = 0; if (response_out) { @@ -3125,13 +3125,16 @@ static cbm_daemon_runtime_application_status_t application_client_exchange_tagge if (response_length_out) { *response_length_out = 0; } + if (request_sent_out) { + *request_sent_out = false; + } cbm_daemon_runtime_application_status_t status = request_token == CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID ? cbm_daemon_runtime_client_application_request(client, request, request_length, &response, &response_length, timeout_ms) - : cbm_daemon_runtime_client_application_request_tagged(client, request_token, request, - request_length, &response, - &response_length, timeout_ms); + : cbm_daemon_runtime_client_application_request_tagged( + client, request_token, request, request_length, &response, &response_length, + request_sent_out, timeout_ms); free(request); if (status != CBM_DAEMON_RUNTIME_APPLICATION_OK) { free(response); @@ -3162,7 +3165,7 @@ static cbm_daemon_runtime_application_status_t application_client_exchange( uint8_t **response_out, uint32_t *response_length_out, uint32_t timeout_ms) { return application_client_exchange_tagged(client, CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID, request, request_length, response_out, - response_length_out, timeout_ms); + response_length_out, NULL, timeout_ms); } cbm_daemon_runtime_application_status_t cbm_daemon_application_client_set_context( @@ -3290,7 +3293,7 @@ cbm_daemon_runtime_application_status_t cbm_daemon_application_client_ui_readine static cbm_daemon_runtime_application_status_t application_client_text_request_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, cbm_daemon_application_request_kind_t kind, const char *text, uint8_t **response_out, - uint32_t *response_length_out, uint32_t timeout_ms) { + uint32_t *response_length_out, bool *request_sent_out, uint32_t timeout_ms) { if (!client || !text || !text[0]) { return CBM_DAEMON_RUNTIME_APPLICATION_REJECTED; } @@ -3306,7 +3309,7 @@ static cbm_daemon_runtime_application_status_t application_client_text_request_t memcpy(request + 1, text, text_length); return application_client_exchange_tagged(client, request_token, request, (uint32_t)text_length + 1U, response_out, - response_length_out, timeout_ms); + response_length_out, request_sent_out, timeout_ms); } static cbm_daemon_runtime_application_status_t application_client_text_request( @@ -3314,7 +3317,7 @@ static cbm_daemon_runtime_application_status_t application_client_text_request( const char *text, uint8_t **response_out, uint32_t *response_length_out, uint32_t timeout_ms) { return application_client_text_request_tagged( client, CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID, kind, text, response_out, - response_length_out, timeout_ms); + response_length_out, NULL, timeout_ms); } cbm_daemon_runtime_application_status_t cbm_daemon_application_client_mcp( @@ -3327,10 +3330,10 @@ cbm_daemon_runtime_application_status_t cbm_daemon_application_client_mcp( cbm_daemon_runtime_application_status_t cbm_daemon_application_client_mcp_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, const char *message, uint8_t **response_out, uint32_t *response_length_out, - uint32_t timeout_ms) { - return application_client_text_request_tagged(client, request_token, - CBM_DAEMON_APPLICATION_REQUEST_MCP, message, - response_out, response_length_out, timeout_ms); + bool *request_sent_out, uint32_t timeout_ms) { + return application_client_text_request_tagged( + client, request_token, CBM_DAEMON_APPLICATION_REQUEST_MCP, message, response_out, + response_length_out, request_sent_out, timeout_ms); } cbm_daemon_runtime_application_status_t cbm_daemon_application_client_tool( diff --git a/src/daemon/application.h b/src/daemon/application.h index 933186aa7..b071ea03c 100644 --- a/src/daemon/application.h +++ b/src/daemon/application.h @@ -149,11 +149,12 @@ cbm_daemon_runtime_application_status_t cbm_daemon_application_client_mcp( cbm_daemon_runtime_client_t *client, const char *message, uint8_t **response_out, uint32_t *response_length_out, uint32_t timeout_ms); -/* Cancellable frontend variant using a token reserved on the runtime client. */ +/* Cancellable frontend variant using a token reserved on the runtime client. + * request_sent_out preserves the runtime's application-frame delivery fact. */ cbm_daemon_runtime_application_status_t cbm_daemon_application_client_mcp_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, const char *message, uint8_t **response_out, uint32_t *response_length_out, - uint32_t timeout_ms); + bool *request_sent_out, uint32_t timeout_ms); cbm_daemon_runtime_application_status_t cbm_daemon_application_client_tool( cbm_daemon_runtime_client_t *client, const char *tool_name, const char *args_json, diff --git a/src/daemon/frontend.c b/src/daemon/frontend.c index 2dd986901..92b032686 100644 --- a/src/daemon/frontend.c +++ b/src/daemon/frontend.c @@ -76,6 +76,7 @@ typedef struct { cbm_mutex_t mutex; cbm_daemon_runtime_client_t *client; cbm_version_cohort_manager_t *cohort_manager; + const cbm_daemon_frontend_session_config_t *session; FILE *out; frontend_item_t queue[FRONTEND_QUEUE_CAPACITY]; size_t head; @@ -89,6 +90,7 @@ typedef struct { int64_t active_id; const char *active_id_str; cbm_daemon_runtime_application_token_t active_request_token; + bool active_cancelled; bool failed; /* Monotonic count of fully processed queue items (responses written or * cancellations acknowledged). The EOF drain below watches it to tell a @@ -352,6 +354,7 @@ static bool frontend_pop_begin(frontend_state_t *state, frontend_item_t *item) { state->active_id_str = item->id_str; item->request_token = request_token; state->active_request_token = request_token; + state->active_cancelled = false; popped = true; } cbm_mutex_unlock(&state->mutex); @@ -365,6 +368,7 @@ static void frontend_end_request(frontend_state_t *state, bool failed) { state->active_id = 0; state->active_id_str = NULL; state->active_request_token = CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; + state->active_cancelled = false; state->failed = state->failed || failed; cbm_mutex_unlock(&state->mutex); } @@ -446,12 +450,8 @@ typedef enum { * request routes its exact runtime token without closing the authenticated * session. A queued request is marked and receives a cancellation error * without ever reaching the daemon. Stale/invalid targets are ignored. */ -static frontend_cancellation_route_t frontend_route_cancellation( - frontend_state_t *state, const char *message, - cbm_daemon_runtime_application_token_t *request_token_out) { - if (request_token_out) { - *request_token_out = CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; - } +static frontend_cancellation_route_t frontend_route_cancellation(frontend_state_t *state, + const char *message) { cbm_jsonrpc_request_t request = {0}; if (!frontend_parse_cancellation(message, &request)) { return FRONTEND_CANCELLATION_NONE; @@ -460,12 +460,9 @@ static frontend_cancellation_route_t frontend_route_cancellation( frontend_cancellation_route_t route = FRONTEND_CANCELLATION_STALE; cbm_mutex_lock(&state->mutex); if (state->in_request && state->active_has_id && - state->active_request_token != CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID && cbm_mcp_cancel_request_matches(request.params_raw, state->active_id, state->active_id_str)) { - if (request_token_out) { - *request_token_out = state->active_request_token; - } + state->active_cancelled = true; route = FRONTEND_CANCELLATION_ACTIVE; } else { for (size_t offset = 0; offset < state->count; offset++) { @@ -484,6 +481,89 @@ static frontend_cancellation_route_t frontend_route_cancellation( return route; } +static cbm_daemon_runtime_cancel_result_t frontend_cancel_active(frontend_state_t *state) { + cbm_mutex_lock(&state->mutex); + cbm_daemon_runtime_cancel_result_t result = CBM_DAEMON_RUNTIME_CANCEL_STALE; + if (state->in_request && state->active_cancelled && state->client && + state->active_request_token != CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID) { + result = cbm_daemon_runtime_client_application_cancel(state->client, + state->active_request_token); + } else if (state->in_request && state->active_cancelled) { + /* A reconnect has no daemon request to cancel yet. The worker observes + * active_cancelled before retrying the frame. */ + result = CBM_DAEMON_RUNTIME_CANCEL_ACCEPTED; + } + cbm_mutex_unlock(&state->mutex); + return result; +} + +static bool frontend_configure_client(cbm_daemon_runtime_client_t *client, + const cbm_daemon_frontend_session_config_t *session) { + uint32_t timeout_ms = session->bootstrap.connect_timeout_ms; + if (cbm_daemon_application_client_set_context( + client, session->session_root, session->allowed_root, session->tool_profile, NULL, NULL, + timeout_ms) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { + return false; + } + return session->ui_update_mask == 0 || + cbm_daemon_application_client_set_ui_config( + client, session->ui_update_mask, session->ui_enabled, session->ui_port, + timeout_ms) == CBM_DAEMON_RUNTIME_APPLICATION_OK; +} + +static bool frontend_recover_client(frontend_state_t *state, frontend_item_t *item, + bool *cancelled_out) { + if (cancelled_out) { + *cancelled_out = false; + } + + cbm_mutex_lock(&state->mutex); + if (state->stopping || !state->client) { + cbm_mutex_unlock(&state->mutex); + return false; + } + cbm_daemon_runtime_client_t *previous = state->client; + state->client = NULL; + state->active_request_token = CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; + cbm_mutex_unlock(&state->mutex); + if (previous) { + (void)cbm_daemon_runtime_client_close(previous, FRONTEND_CLOSE_TIMEOUT_MS); + } + + cbm_daemon_bootstrap_result_t bootstrap = {0}; + cbm_daemon_bootstrap_status_t bootstrap_status = + cbm_daemon_bootstrap_execute(&state->session->bootstrap, &bootstrap); + cbm_daemon_runtime_client_t *replacement = + bootstrap_status == CBM_DAEMON_BOOTSTRAP_CONNECTED ? bootstrap.client : NULL; + bool configured = replacement && frontend_configure_client(replacement, state->session); + cbm_daemon_runtime_application_token_t replacement_token = + CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; + + cbm_mutex_lock(&state->mutex); + bool cancelled = state->active_cancelled; + bool usable = configured && !state->stopping && !state->failed; + if (usable && !cancelled) { + usable = + cbm_daemon_runtime_client_application_token_reserve(replacement, &replacement_token); + } + if (usable) { + state->client = replacement; + if (!cancelled) { + state->active_request_token = replacement_token; + item->request_token = replacement_token; + } + if (cancelled_out) { + *cancelled_out = cancelled; + } + } + cbm_mutex_unlock(&state->mutex); + + if (!usable && replacement) { + (void)cbm_daemon_runtime_client_close(replacement, FRONTEND_CLOSE_TIMEOUT_MS); + } + return usable; +} + static void *frontend_worker(void *opaque) { frontend_state_t *state = opaque; for (;;) { @@ -504,10 +584,27 @@ static void *frontend_worker(void *opaque) { if (item.cancelled) { failed = !frontend_write_cancelled_response(state->out, &item); } else { + bool request_sent = false; cbm_daemon_runtime_application_status_t status = - cbm_daemon_application_client_mcp_tagged(state->client, item.request_token, - item.message, &response, &response_length, - FRONTEND_REQUEST_TIMEOUT_MS); + cbm_daemon_application_client_mcp_tagged( + state->client, item.request_token, item.message, &response, &response_length, + &request_sent, FRONTEND_REQUEST_TIMEOUT_MS); + if (status == CBM_DAEMON_RUNTIME_APPLICATION_TRANSPORT_ERROR && !request_sent) { + free(response); + response = NULL; + response_length = 0; + bool cancelled = false; + if (frontend_recover_client(state, &item, &cancelled)) { + if (cancelled) { + status = CBM_DAEMON_RUNTIME_APPLICATION_CANCELLED; + } else { + bool retry_sent = false; + status = cbm_daemon_application_client_mcp_tagged( + state->client, item.request_token, item.message, &response, + &response_length, &retry_sent, FRONTEND_REQUEST_TIMEOUT_MS); + } + } + } if (status == CBM_DAEMON_RUNTIME_APPLICATION_CANCELLED) { failed = !frontend_write_cancelled_response(state->out, &item); } else { @@ -526,12 +623,11 @@ static void *frontend_worker(void *opaque) { atomic_fetch_add_explicit(&state->completed_items, 1, memory_order_release); if (failed) { if (!expected_stop) { - /* A failed daemon transport cannot wake a thread blocked in - * stdio portably. This frontend owns no state: immediate - * process exit closes the kernel IPC handle, which cancels - * daemon session ownership without a detached reader or an - * unsafe cross-thread fclose. Logging or flushing here could - * itself block on agent-owned stdout/stderr. */ + /* Recovery was unsafe or the one safe retry failed. Immediate + * process exit wakes an agent blocked on stdio and closes the + * kernel IPC handle without a detached reader or cross-thread + * fclose. Logging or flushing here could itself block on + * agent-owned stdout/stderr. */ _Exit(EXIT_FAILURE); } break; @@ -614,11 +710,11 @@ static bool frontend_enqueue(frontend_state_t *state, char *message, bool conten static bool frontend_stop_begin(frontend_state_t *state) { cbm_mutex_lock(&state->mutex); state->stopping = true; + cbm_daemon_runtime_client_t *client = state->client; + bool stop_begun = !client || cbm_daemon_runtime_client_close_begin(client); cbm_mutex_unlock(&state->mutex); - /* Retain the client allocation until the worker is joined. This covers the - * boundary where the worker has claimed an item but has not yet entered the - * runtime exchange: a late call observes closing instead of freed memory. */ - return cbm_daemon_runtime_client_close_begin(state->client); + /* NULL means recovery owns cleanup. Otherwise retain the client until join. */ + return stop_begun; } static bool frontend_cancel_for_maintenance(void *opaque) { @@ -628,24 +724,33 @@ static bool frontend_cancel_for_maintenance(void *opaque) { cbm_mutex_lock(&state->mutex); state->stopping = true; if (state->in_request) { + state->active_cancelled = true; request_token = state->active_request_token; } - cbm_mutex_unlock(&state->mutex); if (request_token == CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID) { + cbm_mutex_unlock(&state->mutex); return false; } - return cbm_daemon_runtime_client_application_cancel(state->client, request_token) == - CBM_DAEMON_RUNTIME_CANCEL_ACCEPTED; + bool cancelled = state->client && + cbm_daemon_runtime_client_application_cancel(state->client, request_token) == + CBM_DAEMON_RUNTIME_CANCEL_ACCEPTED; + cbm_mutex_unlock(&state->mutex); + return cancelled; } int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, - cbm_version_cohort_manager_t *cohort_manager, FILE *in, FILE *out) { - if (!client || !cohort_manager || !in || !out) { + cbm_version_cohort_manager_t *cohort_manager, + const cbm_daemon_frontend_session_config_t *session, FILE *in, + FILE *out) { + if (!client || !cohort_manager || !session || !session->bootstrap.endpoint || + !session->bootstrap.identity || !session->session_root || !session->session_root[0] || + !in || !out) { return -1; } frontend_state_t state = { .client = client, .cohort_manager = cohort_manager, + .session = session, .out = out, }; cbm_mutex_init(&state.mutex); @@ -669,7 +774,7 @@ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, } int result = 0; - bool close_begun = false; + bool stop_begun = false; bool clean_eof = false; for (;;) { char *message = NULL; @@ -681,18 +786,14 @@ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, free(message); break; } - cbm_daemon_runtime_application_token_t cancel_token = - CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; - frontend_cancellation_route_t cancellation = - frontend_route_cancellation(&state, message, &cancel_token); + frontend_cancellation_route_t cancellation = frontend_route_cancellation(&state, message); if (cancellation != FRONTEND_CANCELLATION_NONE) { free(message); if (cancellation == FRONTEND_CANCELLATION_ACTIVE) { - cbm_daemon_runtime_cancel_result_t cancelled = - cbm_daemon_runtime_client_application_cancel(state.client, cancel_token); + cbm_daemon_runtime_cancel_result_t cancelled = frontend_cancel_active(&state); if (cancelled == CBM_DAEMON_RUNTIME_CANCEL_ERROR) { result = -1; - close_begun = frontend_stop_begin(&state); + stop_begun = frontend_stop_begin(&state); break; } } @@ -707,7 +808,7 @@ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, } } - if (clean_eof && !close_begun) { + if (clean_eof && !stop_begun) { frontend_input_closed(&state); /* EOF ends INPUT, not accepted work: as long as queued items keep * completing, every already-enqueued request still receives its @@ -733,8 +834,8 @@ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, } } } - if (!close_begun) { - close_begun = frontend_stop_begin(&state); + if (!stop_begun) { + stop_begun = frontend_stop_begin(&state); } frontend_join_watchdog_t watchdog; cbm_thread_t watchdog_thread; @@ -759,9 +860,9 @@ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, if (!cbm_daemon_maintenance_monitor_stop(&maintenance_monitor)) { _Exit(EXIT_FAILURE); } - if (close_begun) { + if (stop_begun && state.client) { (void)cbm_daemon_runtime_client_close_finish(state.client, FRONTEND_CLOSE_TIMEOUT_MS); - } else { + } else if (!stop_begun) { result = -1; } for (size_t i = 0; i < FRONTEND_QUEUE_CAPACITY; i++) { diff --git a/src/daemon/frontend.h b/src/daemon/frontend.h index f23395774..10255687f 100644 --- a/src/daemon/frontend.h +++ b/src/daemon/frontend.h @@ -4,14 +4,29 @@ #ifndef CBM_DAEMON_FRONTEND_H #define CBM_DAEMON_FRONTEND_H +#include "daemon/bootstrap.h" #include "daemon/runtime.h" #include "daemon/version_cohort.h" +#include "mcp/mcp.h" #include #include typedef struct cbm_daemon_maintenance_monitor cbm_daemon_maintenance_monitor_t; +/* Everything the stdio frontend must replay when its authenticated daemon + * connection is replaced. Pointer fields are borrowed for the complete + * frontend call. */ +typedef struct { + cbm_daemon_bootstrap_config_t bootstrap; + const char *session_root; + const char *allowed_root; + cbm_mcp_tool_profile_t tool_profile; + uint8_t ui_update_mask; + bool ui_enabled; + int ui_port; +} cbm_daemon_frontend_session_config_t; + /* Called once when install/update/uninstall requests an active local command * to stop cooperatively. Returning false does not authorize the command to * outlive the bounded grace period. */ @@ -60,9 +75,12 @@ uint64_t cbm_daemon_frontend_test_worker_idle_cycles(void); * either thread is blocked in stdio, requests cooperative cancellation for the * exact active request, and then bounds process exit. Kernel IPC close cancels * only this session's daemon work. EOF/parse failure closes the authenticated - * session. An unexpected daemon transport failure likewise terminates the + * session. A transport failure before the application frame is sent replaces + * the daemon client and retries once. Sent or repeated failures terminate the * process so an agent waiting with stdin still open observes server EOF. */ int cbm_daemon_frontend_mcp_run(cbm_daemon_runtime_client_t *client, - cbm_version_cohort_manager_t *cohort_manager, FILE *in, FILE *out); + cbm_version_cohort_manager_t *cohort_manager, + const cbm_daemon_frontend_session_config_t *session, FILE *in, + FILE *out); #endif /* CBM_DAEMON_FRONTEND_H */ diff --git a/src/daemon/runtime.c b/src/daemon/runtime.c index 7db0f0225..c9eaeda49 100644 --- a/src/daemon/runtime.c +++ b/src/daemon/runtime.c @@ -3019,13 +3019,16 @@ cbm_daemon_runtime_cancel_result_t cbm_daemon_runtime_client_application_cancel( cbm_daemon_runtime_application_status_t cbm_daemon_runtime_client_application_request_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, const void *request, uint32_t request_length, uint8_t **response_out, - uint32_t *response_length_out, uint32_t timeout_ms) { + uint32_t *response_length_out, bool *request_sent_out, uint32_t timeout_ms) { if (response_out) { *response_out = NULL; } if (response_length_out) { *response_length_out = 0; } + if (request_sent_out) { + *request_sent_out = false; + } if (!client || !response_out || !response_length_out || request_token == CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID || timeout_ms == CBM_DAEMON_IPC_WAIT_FOREVER || @@ -3081,6 +3084,9 @@ cbm_daemon_runtime_application_status_t cbm_daemon_runtime_client_application_re request_can_send && cbm_daemon_ipc_send_frame(connection, CBM_DAEMON_FRAME_REQUEST, CBM_DAEMON_RUNTIME_OP_APPLICATION_REQUEST, wire, (uint32_t)wire_length); + if (request_sent_out) { + *request_sent_out = sent; + } bool send_cancel = false; cbm_mutex_lock(&client->state_mutex); client->application_request_sent = sent; @@ -3179,9 +3185,9 @@ cbm_daemon_runtime_application_status_t cbm_daemon_runtime_client_application_re if (!cbm_daemon_runtime_client_application_token_reserve(client, &request_token)) { return CBM_DAEMON_RUNTIME_APPLICATION_TRANSPORT_ERROR; } - return cbm_daemon_runtime_client_application_request_tagged(client, request_token, request, - request_length, response_out, - response_length_out, timeout_ms); + return cbm_daemon_runtime_client_application_request_tagged( + client, request_token, request, request_length, response_out, response_length_out, NULL, + timeout_ms); } bool cbm_daemon_runtime_client_close_begin(cbm_daemon_runtime_client_t *client) { diff --git a/src/daemon/runtime.h b/src/daemon/runtime.h index 4d53da9a4..15521ac83 100644 --- a/src/daemon/runtime.h +++ b/src/daemon/runtime.h @@ -399,11 +399,12 @@ cbm_daemon_runtime_application_status_t cbm_daemon_runtime_client_application_re /* Execute using the sole outstanding token returned by token_reserve(). This * is the cancellable frontend path; the legacy helper above reserves - * internally. */ + * internally. request_sent_out reports whether the application frame crossed + * the local transport, including failures while awaiting its response. */ cbm_daemon_runtime_application_status_t cbm_daemon_runtime_client_application_request_tagged( cbm_daemon_runtime_client_t *client, cbm_daemon_runtime_application_token_t request_token, const void *request, uint32_t request_length, uint8_t **response_out, - uint32_t *response_length_out, uint32_t timeout_ms); + uint32_t *response_length_out, bool *request_sent_out, uint32_t timeout_ms); /* Begin a two-phase close without freeing the client. This atomically rejects * future exchanges and interrupts an exchange already in flight. Exactly one diff --git a/src/main.c b/src/main.c index 8a575c44f..98db00561 100644 --- a/src/main.c +++ b/src/main.c @@ -3073,38 +3073,54 @@ int main(int argc, char **argv) { cbm_daemon_ipc_endpoint_free(endpoint); if (bootstrap_status != CBM_DAEMON_BOOTSTRAP_CONNECTED || !bootstrap_result.client) { main_report_client_bootstrap_failure(role, &bootstrap_result); + cbm_daemon_ipc_endpoint_free(endpoint); (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); return EXIT_FAILURE; } g_daemon_client = bootstrap_result.client; - if (role == CBM_DAEMON_PROCESS_MCP_CLIENT && - !main_set_client_context(g_daemon_client, NULL, tool_profile, NULL, NULL, - MAIN_CONNECT_TIMEOUT_MS)) { - (void)fprintf(stderr, "codebase-memory-mcp: daemon session context was rejected\n"); - (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); - g_daemon_client = NULL; - (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); - return EXIT_FAILURE; + char mcp_session_root[MAIN_PATH_CAP] = {0}; + char mcp_allowed_root[MAIN_PATH_CAP] = {0}; + const char *mcp_allowed_root_ptr = NULL; + if (role == CBM_DAEMON_PROCESS_MCP_CLIENT) { + bool context_resolved = + main_session_context(NULL, mcp_session_root, mcp_allowed_root, &mcp_allowed_root_ptr); + cbm_daemon_runtime_application_status_t context_status = + context_resolved + ? cbm_daemon_application_client_set_context(g_daemon_client, mcp_session_root, + mcp_allowed_root_ptr, tool_profile, + NULL, NULL, MAIN_CONNECT_TIMEOUT_MS) + : CBM_DAEMON_RUNTIME_APPLICATION_REJECTED; + if (context_status != CBM_DAEMON_RUNTIME_APPLICATION_OK) { + (void)fprintf(stderr, "codebase-memory-mcp: daemon session context was rejected\n"); + (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); + g_daemon_client = NULL; + cbm_daemon_ipc_endpoint_free(endpoint); + (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); + return EXIT_FAILURE; + } } /* Persist UI mutations only after the exact-build HELLO succeeds. A * conflicting binary must be observationally read-only: applying its * flags before bootstrap could reconfigure the already-running daemon * even though that client was then rejected. */ + bool mcp_ui_enabled = false; + int mcp_ui_port = 0; + uint8_t mcp_ui_update_mask = 0; if (role == CBM_DAEMON_PROCESS_MCP_CLIENT && cbm_mcp_tool_profile_allows_http(tool_profile)) { - bool ui_enabled = false; - int ui_port = 0; bool explicitly_enabled = false; - uint8_t update_mask = - parse_ui_flags(argc, argv, &ui_enabled, &ui_port, &explicitly_enabled); - if (update_mask != 0 && cbm_daemon_application_client_set_ui_config( - g_daemon_client, update_mask, ui_enabled, ui_port, - MAIN_CONNECT_TIMEOUT_MS) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { + mcp_ui_update_mask = + parse_ui_flags(argc, argv, &mcp_ui_enabled, &mcp_ui_port, &explicitly_enabled); + if (mcp_ui_update_mask != 0 && + cbm_daemon_application_client_set_ui_config( + g_daemon_client, mcp_ui_update_mask, mcp_ui_enabled, mcp_ui_port, + MAIN_CONNECT_TIMEOUT_MS) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { (void)fprintf(stderr, "codebase-memory-mcp: daemon UI configuration update failed\n"); (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); g_daemon_client = NULL; + cbm_daemon_ipc_endpoint_free(endpoint); (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); return EXIT_FAILURE; } @@ -3119,14 +3135,26 @@ int main(int argc, char **argv) { (void)fprintf(stderr, "codebase-memory-mcp: parent-death watchdog could not start\n"); (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); g_daemon_client = NULL; + cbm_daemon_ipc_endpoint_free(endpoint); (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); return EXIT_FAILURE; } #endif setup_signal_handlers(); - int result = cbm_daemon_frontend_mcp_run(g_daemon_client, client_cohort_manager, stdin, stdout); + cbm_daemon_frontend_session_config_t frontend_session = { + .bootstrap = bootstrap_config, + .session_root = mcp_session_root, + .allowed_root = mcp_allowed_root_ptr, + .tool_profile = tool_profile, + .ui_update_mask = mcp_ui_update_mask, + .ui_enabled = mcp_ui_enabled, + .ui_port = mcp_ui_port, + }; + int result = cbm_daemon_frontend_mcp_run(g_daemon_client, client_cohort_manager, + &frontend_session, stdin, stdout); g_daemon_client = NULL; /* frontend consumed the handle */ + cbm_daemon_ipc_endpoint_free(endpoint); bool client_cohort_cleanup = main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); atomic_store(&g_shutdown, 1); diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index ed0e868e9..7df0e3e9f 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -4,6 +4,7 @@ #include "test_framework.h" #include "test_helpers.h" +#include "daemon/application.h" #include "daemon/frontend.h" #include "daemon/ipc.h" #include "daemon/service.h" @@ -239,8 +240,18 @@ enum { * regression black-box: it must remain valid if the exact capacity changes * while still proving that overload cannot hide an already-pending EOF. */ FRONTEND_EOF_TEST_OVERFLOW_MESSAGES = 32, + /* Hold replacement context past the production frontend's 15 second + * no-progress EOF + drain so shutdown deterministically overlaps recovery. */ + FRONTEND_RECOVERY_EOF_RELEASE_MS = 16000, }; +typedef enum { + FRONTEND_RECOVERY_RETRY = 0, + FRONTEND_RECOVERY_CANCEL = 1, + FRONTEND_RECOVERY_CLEAN_EOF = 2, +} frontend_recovery_mode_t; + typedef struct { atomic_int requests; atomic_int session_cancels; @@ -268,6 +279,7 @@ typedef struct { typedef struct { int fd; bool overflow; + frontend_recovery_mode_t recovery_mode; frontend_eof_application_context_t *application; atomic_bool finished; atomic_bool succeeded; @@ -275,7 +287,9 @@ typedef struct { typedef struct { char conflict_log[FRONTEND_TEST_PATH_CAP]; + char build_fingerprint[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; cbm_daemon_ipc_endpoint_t *endpoint; + cbm_daemon_build_identity_t identity; cbm_version_cohort_manager_t *manager; cbm_daemon_runtime_service_t *service; cbm_daemon_runtime_client_t *client; @@ -337,16 +351,22 @@ static cbm_daemon_runtime_application_status_t frontend_eof_application_request( } /* Released: fall through and answer normally like every later item. */ } - if (request_length == 0) { + if (request_length == 0 || request[0] == CBM_DAEMON_APPLICATION_REQUEST_SET_CONTEXT) { return CBM_DAEMON_RUNTIME_APPLICATION_OK; } - uint8_t *response = malloc(request_length); + const uint8_t *response_source = request; + uint32_t response_length = request_length; + if (request[0] == CBM_DAEMON_APPLICATION_REQUEST_MCP) { + response_source++; + response_length--; + } + uint8_t *response = malloc(response_length); if (!response) { return CBM_DAEMON_RUNTIME_APPLICATION_HANDLER_ERROR; } - memcpy(response, request, request_length); + memcpy(response, response_source, response_length); *response_out = response; - *response_length_out = request_length; + *response_length_out = response_length; if (context->request_observed_fd >= 0) { const char marker = 'Q'; (void)write(context->request_observed_fd, &marker, 1); @@ -454,6 +474,86 @@ static void *frontend_eof_writer(void *opaque) { return NULL; } +static const char frontend_recovery_request[] = + "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/list\",\"params\":{}}\n"; +static const char frontend_recovery_cancel[] = + "{\"jsonrpc\":\"2.0\",\"method\":\"notifications/cancelled\"," + "\"params\":{\"requestId\":1}}\n"; +static const char frontend_recovery_followup[] = + "{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"tools/list\",\"params\":{}}\n"; + +static void *frontend_recovery_writer(void *opaque) { + frontend_eof_writer_t *writer = opaque; + bool ok = frontend_eof_write_all(writer->fd, frontend_recovery_request, + sizeof(frontend_recovery_request) - 1U); + uint64_t deadline = cbm_now_ms() + FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS; + if (writer->recovery_mode != FRONTEND_RECOVERY_RETRY) { + while (ok && + !atomic_load_explicit(&writer->application->first_request_started, + memory_order_acquire) && + cbm_now_ms() < deadline) { + cbm_usleep(1000); + } + ok = ok && atomic_load_explicit(&writer->application->first_request_started, + memory_order_acquire); + } + if (writer->recovery_mode == FRONTEND_RECOVERY_CANCEL) { + ok = ok && frontend_eof_write_all(writer->fd, frontend_recovery_cancel, + sizeof(frontend_recovery_cancel) - 1U); + /* Keep context blocked until the reader routes the cancellation. */ + cbm_usleep(100000); + atomic_store_explicit(&writer->application->release_first_request, true, + memory_order_release); + ok = ok && frontend_eof_write_all(writer->fd, frontend_recovery_followup, + sizeof(frontend_recovery_followup) - 1U); + } else if (writer->recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF) { + ok = close(writer->fd) == 0 && ok; + writer->fd = -1; + cbm_usleep(FRONTEND_RECOVERY_EOF_RELEASE_MS * 1000U); + atomic_store_explicit(&writer->application->release_first_request, true, + memory_order_release); + } + int expected_requests = writer->recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF ? 1 : 2; + while (ok && + atomic_load_explicit(&writer->application->requests, memory_order_acquire) < + expected_requests && + cbm_now_ms() < deadline) { + cbm_usleep(1000); + } + ok = ok && atomic_load_explicit(&writer->application->requests, memory_order_acquire) == + expected_requests; + if (writer->fd >= 0) { + ok = close(writer->fd) == 0 && ok; + writer->fd = -1; + } + atomic_store_explicit(&writer->succeeded, ok, memory_order_release); + atomic_store_explicit(&writer->finished, true, memory_order_release); + return NULL; +} + +static cbm_daemon_runtime_service_t *frontend_eof_service_start(frontend_eof_fixture_t *fixture) { + cbm_daemon_runtime_application_callbacks_t callbacks = { + .context = &fixture->application, + .session_open = frontend_eof_application_session_open, + .request = frontend_eof_application_request, + .request_cancel = frontend_eof_application_request_cancel, + .session_cancel = frontend_eof_application_session_cancel, + .session_close = frontend_eof_application_session_close, + }; + cbm_daemon_runtime_service_config_t config = { + .endpoint = fixture->endpoint, + .identity = fixture->identity, + .conflict_log_path = fixture->conflict_log, + .conflict_log_cap_bytes = 64U * 1024U, + .max_clients = 4, + .lease_timeout_ms = 5000, + .request_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .shutdown_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .application = callbacks, + }; + return cbm_daemon_runtime_service_start(&config); +} + static bool frontend_eof_fixture_start(frontend_eof_fixture_t *fixture, const char *parent) { memset(fixture, 0, sizeof(*fixture)); atomic_init(&fixture->application.requests, 0); @@ -467,53 +567,42 @@ static bool frontend_eof_fixture_start(frontend_eof_fixture_t *fixture, const ch fixture->application.request_observed_fd = -1; fixture->application.session_cancel_fd = -1; char key[CBM_DAEMON_KEY_SIZE]; - char build[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; int log_written = snprintf(fixture->conflict_log, sizeof(fixture->conflict_log), "%s/conflicts.ndjson", parent); if (log_written <= 0 || log_written >= (int)sizeof(fixture->conflict_log) || !cbm_daemon_rendezvous_key(key) || - !cbm_daemon_runtime_process_build_fingerprint((uint64_t)getpid(), build)) { + !cbm_daemon_runtime_process_build_fingerprint((uint64_t)getpid(), + fixture->build_fingerprint)) { return false; } fixture->endpoint = cbm_daemon_ipc_endpoint_new(key, parent); fixture->manager = fixture->endpoint ? cbm_version_cohort_manager_new(fixture->endpoint) : NULL; - cbm_daemon_build_identity_t identity = { + fixture->identity = (cbm_daemon_build_identity_t){ .semantic_version = "2.4.0", - .build_fingerprint = build, + .build_fingerprint = fixture->build_fingerprint, .cache_fingerprint = FRONTEND_TEST_CACHE, .protocol_abi = 3, .store_abi = 11, .feature_abi = 7, }; - cbm_daemon_runtime_application_callbacks_t callbacks = { - .context = &fixture->application, - .session_open = frontend_eof_application_session_open, - .request = frontend_eof_application_request, - .request_cancel = frontend_eof_application_request_cancel, - .session_cancel = frontend_eof_application_session_cancel, - .session_close = frontend_eof_application_session_close, - }; - cbm_daemon_runtime_service_config_t config = { - .endpoint = fixture->endpoint, - .identity = identity, - .conflict_log_path = fixture->conflict_log, - .conflict_log_cap_bytes = 64U * 1024U, - .max_clients = 4, - .lease_timeout_ms = 5000, - .request_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, - .shutdown_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, - .application = callbacks, - }; - fixture->service = fixture->manager ? cbm_daemon_runtime_service_start(&config) : NULL; + fixture->service = fixture->manager ? frontend_eof_service_start(fixture) : NULL; cbm_daemon_runtime_connect_result_t connect_result = {0}; fixture->client = fixture->service - ? cbm_daemon_runtime_client_connect(fixture->endpoint, &identity, + ? cbm_daemon_runtime_client_connect(fixture->endpoint, &fixture->identity, FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, &connect_result) : NULL; return fixture->client && connect_result.status == CBM_DAEMON_RUNTIME_CONNECT_ACCEPTED; } +static bool frontend_eof_fixture_restart_service(frontend_eof_fixture_t *fixture) { + bool stopped = fixture->service && cbm_daemon_runtime_service_stop( + fixture->service, FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS); + bool freed = stopped && cbm_daemon_runtime_service_free(fixture->service); + fixture->service = freed ? frontend_eof_service_start(fixture) : NULL; + return stopped && freed && fixture->service; +} + static bool frontend_eof_fixture_finish(frontend_eof_fixture_t *fixture) { bool ok = true; if (fixture->client) { @@ -579,7 +668,21 @@ static int frontend_eof_child_run(const char *parent, bool overflow) { cbm_daemon_runtime_client_t *frontend_client = fixture.client; fixture.client = NULL; /* cbm_daemon_frontend_mcp_run consumes it. */ - int result = cbm_daemon_frontend_mcp_run(frontend_client, fixture.manager, input, output); + cbm_daemon_frontend_session_config_t session = { + .bootstrap = + { + .role = CBM_DAEMON_PROCESS_MCP_CLIENT, + .endpoint = fixture.endpoint, + .identity = &fixture.identity, + .executable_path = "unused", + .connect_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .startup_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + }, + .session_root = parent, + .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, + }; + int result = + cbm_daemon_frontend_mcp_run(frontend_client, fixture.manager, &session, input, output); bool joined = cbm_thread_join(&writer_thread) == 0; bool writer_ok = atomic_load_explicit(&writer.finished, memory_order_acquire) && atomic_load_explicit(&writer.succeeded, memory_order_acquire); @@ -660,6 +763,111 @@ static bool frontend_eof_run_isolated(const char *tag, bool overflow) { return child_ok && cleaned; } +static int frontend_recovery_child_run(const char *parent, frontend_recovery_mode_t recovery_mode) { + (void)alarm(FRONTEND_EOF_TEST_CATASTROPHIC_TIMEOUT_S); + frontend_eof_fixture_t fixture; + if (!frontend_eof_fixture_start(&fixture, parent)) { + (void)frontend_eof_fixture_finish(&fixture); + return 73; + } + atomic_store_explicit(&fixture.application.block_first_request, + recovery_mode != FRONTEND_RECOVERY_RETRY, memory_order_release); + cbm_daemon_runtime_client_t *stale_client = fixture.client; + fixture.client = NULL; + bool replacement_ready = frontend_eof_fixture_restart_service(&fixture); + int input_pipe[2] = {-1, -1}; + bool pipe_ready = replacement_ready && pipe(input_pipe) == 0; + FILE *input = pipe_ready ? fdopen(input_pipe[0], "rb") : NULL; + FILE *output = input ? tmpfile() : NULL; + frontend_eof_writer_t writer = { + .fd = input_pipe[1], + .recovery_mode = recovery_mode, + .application = &fixture.application, + }; + atomic_init(&writer.finished, false); + atomic_init(&writer.succeeded, false); + cbm_thread_t writer_thread; + bool writer_started = + output && cbm_thread_create(&writer_thread, 0, frontend_recovery_writer, &writer) == 0; + if (!writer_started && input_pipe[1] >= 0) { + (void)close(input_pipe[1]); + input_pipe[1] = -1; + } + bool request_ready = writer_started; + cbm_daemon_frontend_session_config_t session = { + .bootstrap = + { + .role = CBM_DAEMON_PROCESS_MCP_CLIENT, + .endpoint = fixture.endpoint, + .identity = &fixture.identity, + .executable_path = "unused", + .connect_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .startup_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + }, + .session_root = parent, + .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, + }; + int result = request_ready ? cbm_daemon_frontend_mcp_run(stale_client, fixture.manager, + &session, input, output) + : -1; + stale_client = NULL; /* the frontend consumes the stale or replacement handle */ + bool writer_joined = writer_started && cbm_thread_join(&writer_thread) == 0; + bool writer_ok = writer_joined && + atomic_load_explicit(&writer.finished, memory_order_acquire) && + atomic_load_explicit(&writer.succeeded, memory_order_acquire); + char response[512]; + memset(response, 0, sizeof(response)); + bool response_ready = result == 0 && fflush(output) == 0 && fseek(output, 0, SEEK_SET) == 0; + size_t response_length = response_ready ? fread(response, 1, sizeof(response) - 1U, output) : 0; + bool response_exact = false; + if (recovery_mode == FRONTEND_RECOVERY_CANCEL) { + response_exact = strstr(response, "\"id\":1") && strstr(response, "\"code\":-32800") && + strstr(response, frontend_recovery_followup); + } else if (recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF) { + response_exact = response_length == 0; + } else { + response_exact = response_length == sizeof(frontend_recovery_request) - 1U && + memcmp(response, frontend_recovery_request, + sizeof(frontend_recovery_request) - 1U) == 0; + } + int requests = atomic_load_explicit(&fixture.application.requests, memory_order_acquire); + int expected_requests = recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF ? 1 : 2; + bool input_closed = input && fclose(input) == 0; + bool output_closed = output && fclose(output) == 0; + bool fixture_closed = frontend_eof_fixture_finish(&fixture); + return replacement_ready && request_ready && result == 0 && response_exact && + requests == expected_requests && writer_ok && input_closed && output_closed && + fixture_closed + ? 0 + : 74; +} + +static bool frontend_recovery_run_isolated(frontend_recovery_mode_t recovery_mode) { + char parent[FRONTEND_TEST_PATH_CAP]; + int written = snprintf(parent, sizeof(parent), "%s/cbm-frontend-recovery-XXXXXX", cbm_tmpdir()); + if (written <= 0 || written >= (int)sizeof(parent) || !cbm_mkdtemp(parent)) { + return false; + } + pid_t child = fork(); + if (child == 0) { + (void)signal(SIGALRM, SIG_DFL); + _exit(frontend_recovery_child_run(parent, recovery_mode)); + } + int status = 0; + pid_t waited; + do { + waited = child > 0 ? waitpid(child, &status, 0) : -1; + } while (waited < 0 && errno == EINTR); + bool cleaned = th_rmtree(parent) == 0; + bool child_ok = child > 0 && waited == child && WIFEXITED(status) && WEXITSTATUS(status) == 0; + if (!child_ok && child > 0 && waited == child) { + (void)fprintf(stderr, "frontend recovery fixture failed: status=0x%x exit=%d signal=%d\n", + status, WIFEXITED(status) ? WEXITSTATUS(status) : -1, + WIFSIGNALED(status) ? WTERMSIG(status) : 0); + } + return child_ok && cleaned; +} + static void frontend_test_release_lease(cbm_version_cohort_lease_t **lease) { while (lease && *lease && cbm_version_cohort_lease_release(lease) != CBM_PRIVATE_FILE_LOCK_OK) { cbm_usleep(1000); @@ -920,7 +1128,20 @@ static int frontend_backpressure_frontend_run(const char *parent, int input_fd, return 81; } - int result = cbm_daemon_frontend_mcp_run(client, manager, input, output); + cbm_daemon_frontend_session_config_t session = { + .bootstrap = + { + .role = CBM_DAEMON_PROCESS_MCP_CLIENT, + .endpoint = endpoint, + .identity = &identity, + .executable_path = "unused", + .connect_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .startup_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + }, + .session_root = parent, + .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, + }; + int result = cbm_daemon_frontend_mcp_run(client, manager, &session, input, output); bool joined = cbm_thread_join(&writer_thread) == 0; bool writer_ok = atomic_load_explicit(&writer.finished, memory_order_acquire) && atomic_load_explicit(&writer.succeeded, memory_order_acquire); @@ -1397,8 +1618,21 @@ TEST(daemon_frontend_maintenance_exits_while_stdio_reader_is_blocked) { if (!announced) { _exit(70); } + cbm_daemon_frontend_session_config_t session = { + .bootstrap = + { + .role = CBM_DAEMON_PROCESS_MCP_CLIENT, + .endpoint = endpoint, + .identity = &identity, + .executable_path = "unused", + .connect_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .startup_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + }, + .session_root = fixture.parent, + .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, + }; int result = cbm_daemon_frontend_mcp_run((cbm_daemon_runtime_client_t *)(uintptr_t)1, - manager, input, output); + manager, &session, input, output); (void)result; _exit(71); } @@ -1612,6 +1846,27 @@ TEST(daemon_frontend_over_capacity_input_backpressures_without_loss) { PASS(); } +/* A request rejected before its application frame reaches the daemon is safe + * to replay. The frontend must replace its stale authenticated client, restore + * the session, reserve a token from the new connection, and return the one + * response without closing Codex's stdio transport. */ +TEST(daemon_frontend_recovers_one_unsent_request_after_daemon_replacement) { + ASSERT_TRUE(frontend_recovery_run_isolated(FRONTEND_RECOVERY_RETRY)); + PASS(); +} + +/* Context-replay cancellation must not strand a token or break the next request. */ +TEST(daemon_frontend_recovery_cancellation_preserves_next_request) { + ASSERT_TRUE(frontend_recovery_run_isolated(FRONTEND_RECOVERY_CANCEL)); + PASS(); +} + +/* Clean EOF during context replay must preserve worker-owned client cleanup. */ +TEST(daemon_frontend_clean_eof_during_recovery_preserves_client_ownership) { + ASSERT_TRUE(frontend_recovery_run_isolated(FRONTEND_RECOVERY_CLEAN_EOF)); + PASS(); +} + /* Clean EOF is the normal MCP-session ownership boundary. Accepted work gets a * bounded drain opportunity; work still active at the deadline is cancelled * with that exact session, while the frontend itself reports a clean close. */ @@ -1669,6 +1924,9 @@ SUITE(daemon_frontend) { RUN_TEST(daemon_local_participant_monitor_cancels_then_bounds_active_operation); RUN_TEST(daemon_local_participant_monitor_allows_supervisor_containment_window); RUN_TEST(daemon_frontend_over_capacity_input_backpressures_without_loss); + RUN_TEST(daemon_frontend_recovers_one_unsent_request_after_daemon_replacement); + RUN_TEST(daemon_frontend_recovery_cancellation_preserves_next_request); + RUN_TEST(daemon_frontend_clean_eof_during_recovery_preserves_client_ownership); RUN_TEST(daemon_frontend_eof_drain_timeout_cancels_and_returns_success); RUN_TEST(daemon_frontend_stdout_backpressure_eof_fail_stops_and_cancels_session); RUN_TEST(daemon_frontend_stdout_backpressure_maintenance_stops_and_cancels_session); diff --git a/tests/test_daemon_runtime.c b/tests/test_daemon_runtime.c index 991506a74..b79d6c463 100644 --- a/tests/test_daemon_runtime.c +++ b/tests/test_daemon_runtime.c @@ -907,7 +907,8 @@ static void *runtime_application_client_request_thread(void *opaque) { call->status = call->tagged ? cbm_daemon_runtime_client_application_request_tagged( call->client, call->request_token, call->request, call->request_length, - &call->response, &call->response_length, RUNTIME_TEST_TIMEOUT_MS) + &call->response, &call->response_length, NULL, + RUNTIME_TEST_TIMEOUT_MS) : cbm_daemon_runtime_client_application_request( call->client, call->request, call->request_length, &call->response, &call->response_length, RUNTIME_TEST_TIMEOUT_MS); @@ -3161,7 +3162,7 @@ TEST(daemon_runtime_presend_request_cancel_is_sticky_and_nonterminal) { cancel = cbm_daemon_runtime_client_application_cancel(client, request_token); cancelled_status = cbm_daemon_runtime_client_application_request_tagged( client, request_token, blocking_request, (uint32_t)sizeof(blocking_request), - &cancelled_response, &cancelled_response_length, RUNTIME_TEST_TIMEOUT_MS); + &cancelled_response, &cancelled_response_length, NULL, RUNTIME_TEST_TIMEOUT_MS); } if (cancelled_status == CBM_DAEMON_RUNTIME_APPLICATION_CANCELLED) { next_status = cbm_daemon_runtime_client_application_request( @@ -3218,8 +3219,10 @@ TEST(daemon_runtime_allows_only_one_unstarted_application_token) { bool first_reserved = false; bool duplicate_reservation_rejected = false; bool first_exact = false; + bool first_sent = false; bool second_reserved = false; bool second_exact = false; + bool second_sent = false; bool closed = false; bool exited = false; @@ -3239,7 +3242,7 @@ TEST(daemon_runtime_allows_only_one_unstarted_application_token) { cbm_daemon_runtime_application_status_t status = cbm_daemon_runtime_client_application_request_tagged( client, first_token, first_request, (uint32_t)sizeof(first_request), &response, - &response_length, RUNTIME_TEST_TIMEOUT_MS); + &response_length, &first_sent, RUNTIME_TEST_TIMEOUT_MS); first_exact = status == CBM_DAEMON_RUNTIME_APPLICATION_OK && response_length == sizeof(first_request) && response && memcmp(response, first_request, sizeof(first_request)) == 0; @@ -3255,7 +3258,7 @@ TEST(daemon_runtime_allows_only_one_unstarted_application_token) { cbm_daemon_runtime_application_status_t status = cbm_daemon_runtime_client_application_request_tagged( client, second_token, second_request, (uint32_t)sizeof(second_request), &response, - &response_length, RUNTIME_TEST_TIMEOUT_MS); + &response_length, &second_sent, RUNTIME_TEST_TIMEOUT_MS); second_exact = status == CBM_DAEMON_RUNTIME_APPLICATION_OK && response_length == sizeof(second_request) && response && memcmp(response, second_request, sizeof(second_request)) == 0; @@ -3274,9 +3277,11 @@ TEST(daemon_runtime_allows_only_one_unstarted_application_token) { ASSERT_TRUE(first_reserved); ASSERT_TRUE(duplicate_reservation_rejected); ASSERT_TRUE(first_exact); + ASSERT_TRUE(first_sent); ASSERT_TRUE(second_reserved); ASSERT_EQ(second_token, first_token + 1U); ASSERT_TRUE(second_exact); + ASSERT_TRUE(second_sent); ASSERT_TRUE(closed); ASSERT_TRUE(exited); ASSERT_EQ(atomic_load(&context.requests), 2); @@ -3376,6 +3381,10 @@ TEST(daemon_runtime_close_begin_retains_storage_and_rejects_late_exchange) { uint8_t *response = NULL; uint32_t response_length = 0; cbm_daemon_runtime_application_status_t status = CBM_DAEMON_RUNTIME_APPLICATION_OK; + cbm_daemon_runtime_application_token_t request_token = + CBM_DAEMON_RUNTIME_APPLICATION_TOKEN_INVALID; + bool token_reserved = false; + bool request_sent = true; bool close_begun = false; bool duplicate_begin_rejected = false; bool close_acknowledged = false; @@ -3386,6 +3395,8 @@ TEST(daemon_runtime_close_begin_retains_storage_and_rejects_late_exchange) { RUNTIME_TEST_TIMEOUT_MS, &result); } if (client) { + token_reserved = + cbm_daemon_runtime_client_application_token_reserve(client, &request_token); close_begun = cbm_daemon_runtime_client_close_begin(client); duplicate_begin_rejected = close_begun && !cbm_daemon_runtime_client_close_begin(client); } @@ -3393,9 +3404,9 @@ TEST(daemon_runtime_close_begin_retains_storage_and_rejects_late_exchange) { /* Deterministically models the frontend boundary where close begins * after a worker claims an item but before it enters the runtime API. * The retained allocation must reject the call without touching IPC. */ - status = cbm_daemon_runtime_client_application_request( - client, request, (uint32_t)sizeof(request), &response, &response_length, - RUNTIME_TEST_TIMEOUT_MS); + status = cbm_daemon_runtime_client_application_request_tagged( + client, request_token, request, (uint32_t)sizeof(request), &response, &response_length, + &request_sent, RUNTIME_TEST_TIMEOUT_MS); close_acknowledged = cbm_daemon_runtime_client_close_finish(client, RUNTIME_TEST_TIMEOUT_MS); client = NULL; @@ -3413,11 +3424,13 @@ TEST(daemon_runtime_close_begin_retains_storage_and_rejects_late_exchange) { runtime_test_fixture_finish(&fixture); ASSERT_TRUE(started); + ASSERT_TRUE(token_reserved); ASSERT_TRUE(close_begun); ASSERT_TRUE(duplicate_begin_rejected); ASSERT_EQ(status, CBM_DAEMON_RUNTIME_APPLICATION_TRANSPORT_ERROR); ASSERT_NULL(response); ASSERT_EQ(response_length, 0); + ASSERT_FALSE(request_sent); ASSERT_TRUE(close_acknowledged); ASSERT_TRUE(exited); ASSERT_EQ(atomic_load(&context.opened), 1); From b9da6dca03126ef0c8cacccb3321dd5d69ccf34d Mon Sep 17 00:00:00 2001 From: Pxx500 Date: Fri, 21 Aug 2026 18:44:15 +0200 Subject: [PATCH 02/10] test(frontend): widen recovery hang guard for MSan Signed-off-by: Pxx500 --- src/main.c | 1 - tests/test_daemon_frontend.c | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main.c b/src/main.c index 98db00561..5547d79b1 100644 --- a/src/main.c +++ b/src/main.c @@ -3070,7 +3070,6 @@ int main(int argc, char **argv) { cbm_daemon_bootstrap_result_t bootstrap_result; cbm_daemon_bootstrap_status_t bootstrap_status = main_client_bootstrap_with_upgrade(&bootstrap_config, &bootstrap_result, true); - cbm_daemon_ipc_endpoint_free(endpoint); if (bootstrap_status != CBM_DAEMON_BOOTSTRAP_CONNECTED || !bootstrap_result.client) { main_report_client_bootstrap_failure(role, &bootstrap_result); cbm_daemon_ipc_endpoint_free(endpoint); diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index 7df0e3e9f..96889ad33 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -231,6 +231,9 @@ enum { * dev machine (see the index-supervisor calibration). */ FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS = 30000, FRONTEND_EOF_TEST_CATASTROPHIC_TIMEOUT_S = 90, + /* Recovery can spend most of the generic guard restarting an instrumented + * daemon before the clean-EOF case enters its deliberate 15s drain. */ + FRONTEND_RECOVERY_TEST_CATASTROPHIC_TIMEOUT_S = 120, FRONTEND_BACKPRESSURE_MESSAGE_BYTES = 2 * 1024 * 1024, FRONTEND_BACKPRESSURE_FRONTEND_TIMEOUT_S = 90, FRONTEND_BACKPRESSURE_DAEMON_TIMEOUT_S = 90, @@ -764,7 +767,7 @@ static bool frontend_eof_run_isolated(const char *tag, bool overflow) { } static int frontend_recovery_child_run(const char *parent, frontend_recovery_mode_t recovery_mode) { - (void)alarm(FRONTEND_EOF_TEST_CATASTROPHIC_TIMEOUT_S); + (void)alarm(FRONTEND_RECOVERY_TEST_CATASTROPHIC_TIMEOUT_S); frontend_eof_fixture_t fixture; if (!frontend_eof_fixture_start(&fixture, parent)) { (void)frontend_eof_fixture_finish(&fixture); From 85482e3c599d454d76ccef5e862a5dc6c38a898e Mon Sep 17 00:00:00 2001 From: Pxx500 Date: Mon, 7 Sep 2026 17:30:13 +0200 Subject: [PATCH 03/10] test(frontend): address recovery review feedback Signed-off-by: Pxx500 --- src/daemon/frontend.c | 16 ++++++++ src/daemon/frontend.h | 12 ++++-- tests/test_daemon_frontend.c | 72 +++++++++++++++++++++++++----------- 3 files changed, 75 insertions(+), 25 deletions(-) diff --git a/src/daemon/frontend.c b/src/daemon/frontend.c index 92b032686..d65284bd7 100644 --- a/src/daemon/frontend.c +++ b/src/daemon/frontend.c @@ -119,6 +119,7 @@ static atomic_bool frontend_test_monitor_is_waiting = ATOMIC_VAR_INIT(false); static atomic_uint_fast64_t frontend_test_monitor_observation_count = ATOMIC_VAR_INIT(0); static atomic_uint_fast64_t frontend_test_worker_observation_count = ATOMIC_VAR_INIT(0); static atomic_uint_fast64_t frontend_test_worker_idle_count = ATOMIC_VAR_INIT(0); +static atomic_uint_fast64_t frontend_test_routed_cancellation_count = ATOMIC_VAR_INIT(0); void cbm_daemon_frontend_test_observer_reset(bool hold_monitor) { atomic_store_explicit(&frontend_test_monitor_observation_count, 0, memory_order_release); @@ -147,6 +148,10 @@ uint64_t cbm_daemon_frontend_test_worker_observations(void) { uint64_t cbm_daemon_frontend_test_worker_idle_cycles(void) { return atomic_load_explicit(&frontend_test_worker_idle_count, memory_order_acquire); } + +uint64_t cbm_daemon_frontend_test_routed_cancellations(void) { + return atomic_load_explicit(&frontend_test_routed_cancellation_count, memory_order_acquire); +} #endif static cbm_version_cohort_maintenance_presence_t frontend_observe_maintenance( @@ -478,6 +483,12 @@ static frontend_cancellation_route_t frontend_route_cancellation(frontend_state_ } cbm_mutex_unlock(&state->mutex); cbm_jsonrpc_request_free(&request); +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS + if (route == FRONTEND_CANCELLATION_ACTIVE || route == FRONTEND_CANCELLATION_QUEUED) { + atomic_fetch_add_explicit(&frontend_test_routed_cancellation_count, 1, + memory_order_release); + } +#endif return route; } @@ -531,6 +542,11 @@ static bool frontend_recover_client(frontend_state_t *state, frontend_item_t *it } cbm_daemon_bootstrap_result_t bootstrap = {0}; + /* Bootstrap can report launch failures on stderr from this worker. + * Keep that diagnostic + * to explain failed recovery. It never writes MCP + * stdout, but blocked stderr can delay + * exit. Do not add more logging here. */ cbm_daemon_bootstrap_status_t bootstrap_status = cbm_daemon_bootstrap_execute(&state->session->bootstrap, &bootstrap); cbm_daemon_runtime_client_t *replacement = diff --git a/src/daemon/frontend.h b/src/daemon/frontend.h index 10255687f..9ee193be8 100644 --- a/src/daemon/frontend.h +++ b/src/daemon/frontend.h @@ -14,9 +14,11 @@ typedef struct cbm_daemon_maintenance_monitor cbm_daemon_maintenance_monitor_t; -/* Everything the stdio frontend must replay when its authenticated daemon - * connection is replaced. Pointer fields are borrowed for the complete - * frontend call. */ +/* Connection settings replayed when the authenticated daemon is replaced. + * Background activation + * from initialize is not restored here. + * Pointer fields are borrowed for the complete frontend + * call. */ typedef struct { cbm_daemon_bootstrap_config_t bootstrap; const char *session_root; @@ -67,6 +69,10 @@ bool cbm_daemon_frontend_test_monitor_waiting(void); uint64_t cbm_daemon_frontend_test_monitor_observations(void); uint64_t cbm_daemon_frontend_test_worker_observations(void); uint64_t cbm_daemon_frontend_test_worker_idle_cycles(void); +/* Counts matching cancellations marked under the frontend mutex. + * Tests snapshot this monotonic + * count before sending a notification. */ +uint64_t cbm_daemon_frontend_test_routed_cancellations(void); #endif /* Takes ownership of client and borrows cohort_manager for the complete call. diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index 96889ad33..acdaf0512 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -39,6 +39,8 @@ typedef struct { typedef struct { frontend_maintenance_fixture_t maintenance; char conflict_log[FRONTEND_TEST_PATH_CAP]; + char build_fingerprint[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; + cbm_daemon_build_identity_t identity; cbm_daemon_runtime_service_t *service; cbm_daemon_runtime_client_t *client; } frontend_idle_fixture_t; @@ -46,6 +48,7 @@ typedef struct { typedef struct { cbm_daemon_runtime_client_t *client; cbm_version_cohort_manager_t *manager; + const cbm_daemon_frontend_session_config_t *session; FILE *input; FILE *output; int result; @@ -99,22 +102,22 @@ static cbm_daemon_runtime_application_status_t frontend_idle_request( return CBM_DAEMON_RUNTIME_APPLICATION_OK; } -static void frontend_idle_request_cancel( - void *context, cbm_daemon_runtime_application_session_t *session, - cbm_daemon_runtime_application_token_t request_token) { +static void frontend_idle_request_cancel(void *context, + cbm_daemon_runtime_application_session_t *session, + cbm_daemon_runtime_application_token_t request_token) { (void)context; (void)session; (void)request_token; } -static void frontend_idle_session_cancel( - void *context, cbm_daemon_runtime_application_session_t *session) { +static void frontend_idle_session_cancel(void *context, + cbm_daemon_runtime_application_session_t *session) { (void)context; (void)session; } -static void frontend_idle_session_close( - void *context, cbm_daemon_runtime_application_session_t *session) { +static void frontend_idle_session_close(void *context, + cbm_daemon_runtime_application_session_t *session) { (void)context; (void)session; } @@ -128,22 +131,21 @@ static uint64_t frontend_test_process_id(void) { } static bool frontend_idle_fixture_start(frontend_idle_fixture_t *fixture) { - static const char cache[] = - "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"; + static const char cache[] = "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"; memset(fixture, 0, sizeof(*fixture)); if (!frontend_maintenance_fixture_start(&fixture->maintenance, "idle-observer")) { return false; } - char build[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; int log_written = snprintf(fixture->conflict_log, sizeof(fixture->conflict_log), "%s/conflicts.ndjson", fixture->maintenance.parent); if (log_written <= 0 || log_written >= (int)sizeof(fixture->conflict_log) || - !cbm_daemon_runtime_process_build_fingerprint(frontend_test_process_id(), build)) { + !cbm_daemon_runtime_process_build_fingerprint(frontend_test_process_id(), + fixture->build_fingerprint)) { return false; } - cbm_daemon_build_identity_t identity = { + fixture->identity = (cbm_daemon_build_identity_t){ .semantic_version = "2.4.0", - .build_fingerprint = build, + .build_fingerprint = fixture->build_fingerprint, .cache_fingerprint = cache, .protocol_abi = 3, .store_abi = 11, @@ -159,7 +161,7 @@ static bool frontend_idle_fixture_start(frontend_idle_fixture_t *fixture) { }; cbm_daemon_runtime_service_config_t config = { .endpoint = fixture->maintenance.endpoint, - .identity = identity, + .identity = fixture->identity, .conflict_log_path = fixture->conflict_log, .conflict_log_cap_bytes = 64U * 1024U, .max_clients = 2, @@ -170,10 +172,11 @@ static bool frontend_idle_fixture_start(frontend_idle_fixture_t *fixture) { }; fixture->service = cbm_daemon_runtime_service_start(&config); cbm_daemon_runtime_connect_result_t connect_result = {0}; - fixture->client = fixture->service - ? cbm_daemon_runtime_client_connect(fixture->maintenance.endpoint, &identity, - 30000, &connect_result) - : NULL; + fixture->client = + fixture->service + ? cbm_daemon_runtime_client_connect(fixture->maintenance.endpoint, &fixture->identity, + 30000, &connect_result) + : NULL; return fixture->client && connect_result.status == CBM_DAEMON_RUNTIME_CONNECT_ACCEPTED; } @@ -213,8 +216,8 @@ static int frontend_test_close_fd(int fd) { static void *frontend_idle_run(void *opaque) { frontend_idle_run_t *run = opaque; - run->result = - cbm_daemon_frontend_mcp_run(run->client, run->manager, run->input, run->output); + run->result = cbm_daemon_frontend_mcp_run(run->client, run->manager, run->session, run->input, + run->output); return NULL; } #endif @@ -479,11 +482,13 @@ static void *frontend_eof_writer(void *opaque) { static const char frontend_recovery_request[] = "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/list\",\"params\":{}}\n"; +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS static const char frontend_recovery_cancel[] = "{\"jsonrpc\":\"2.0\",\"method\":\"notifications/cancelled\"," "\"params\":{\"requestId\":1}}\n"; static const char frontend_recovery_followup[] = "{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"tools/list\",\"params\":{}}\n"; +#endif static void *frontend_recovery_writer(void *opaque) { frontend_eof_writer_t *writer = opaque; @@ -500,16 +505,24 @@ static void *frontend_recovery_writer(void *opaque) { ok = ok && atomic_load_explicit(&writer->application->first_request_started, memory_order_acquire); } +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS if (writer->recovery_mode == FRONTEND_RECOVERY_CANCEL) { + uint64_t cancellations = cbm_daemon_frontend_test_routed_cancellations(); ok = ok && frontend_eof_write_all(writer->fd, frontend_recovery_cancel, sizeof(frontend_recovery_cancel) - 1U); /* Keep context blocked until the reader routes the cancellation. */ - cbm_usleep(100000); + while (ok && cbm_daemon_frontend_test_routed_cancellations() == cancellations && + cbm_now_ms() < deadline) { + cbm_usleep(1000); + } + ok = ok && cbm_daemon_frontend_test_routed_cancellations() > cancellations; atomic_store_explicit(&writer->application->release_first_request, true, memory_order_release); ok = ok && frontend_eof_write_all(writer->fd, frontend_recovery_followup, sizeof(frontend_recovery_followup) - 1U); - } else if (writer->recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF) { + } +#endif + if (writer->recovery_mode == FRONTEND_RECOVERY_CLEAN_EOF) { ok = close(writer->fd) == 0 && ok; writer->fd = -1; cbm_usleep(FRONTEND_RECOVERY_EOF_RELEASE_MS * 1000U); @@ -1537,9 +1550,20 @@ TEST(daemon_frontend_idle_uses_one_maintenance_observer) { bool pipe_ready = fixture_ready && cbm_pipe(input_pipe) == 0; FILE *input = pipe_ready ? frontend_test_fdopen_read(input_pipe[0]) : NULL; FILE *output = input ? tmpfile() : NULL; + cbm_daemon_frontend_session_config_t session = { + .bootstrap = {.role = CBM_DAEMON_PROCESS_MCP_CLIENT, + .endpoint = fixture.maintenance.endpoint, + .identity = &fixture.identity, + .executable_path = "unused", + .connect_timeout_ms = 30000, + .startup_timeout_ms = 30000}, + .session_root = fixture.maintenance.parent, + .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, + }; frontend_idle_run_t run = { .client = fixture.client, .manager = fixture.maintenance.manager, + .session = &session, .input = input, .output = output, .result = -1, @@ -1859,10 +1883,12 @@ TEST(daemon_frontend_recovers_one_unsent_request_after_daemon_replacement) { } /* Context-replay cancellation must not strand a token or break the next request. */ +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS TEST(daemon_frontend_recovery_cancellation_preserves_next_request) { ASSERT_TRUE(frontend_recovery_run_isolated(FRONTEND_RECOVERY_CANCEL)); PASS(); } +#endif /* Clean EOF during context replay must preserve worker-owned client cleanup. */ TEST(daemon_frontend_clean_eof_during_recovery_preserves_client_ownership) { @@ -1928,7 +1954,9 @@ SUITE(daemon_frontend) { RUN_TEST(daemon_local_participant_monitor_allows_supervisor_containment_window); RUN_TEST(daemon_frontend_over_capacity_input_backpressures_without_loss); RUN_TEST(daemon_frontend_recovers_one_unsent_request_after_daemon_replacement); +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS RUN_TEST(daemon_frontend_recovery_cancellation_preserves_next_request); +#endif RUN_TEST(daemon_frontend_clean_eof_during_recovery_preserves_client_ownership); RUN_TEST(daemon_frontend_eof_drain_timeout_cancels_and_returns_success); RUN_TEST(daemon_frontend_stdout_backpressure_eof_fail_stops_and_cancels_session); From 71ac6ecc15d3ab3ebcd31f0e467071a506006ca2 Mon Sep 17 00:00:00 2001 From: Pxx500 Date: Sat, 26 Sep 2026 23:01:11 +0200 Subject: [PATCH 04/10] fix(mcp): reuse response cleanup in frontend worker Signed-off-by: Pxx500 --- src/daemon/frontend.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/daemon/frontend.c b/src/daemon/frontend.c index d65284bd7..a4d4f8846 100644 --- a/src/daemon/frontend.c +++ b/src/daemon/frontend.c @@ -580,6 +580,12 @@ static bool frontend_recover_client(frontend_state_t *state, frontend_item_t *it return usable; } +static void frontend_response_reset(uint8_t **response, uint32_t *response_length) { + free(*response); + *response = NULL; + *response_length = 0; +} + static void *frontend_worker(void *opaque) { frontend_state_t *state = opaque; for (;;) { @@ -606,9 +612,7 @@ static void *frontend_worker(void *opaque) { state->client, item.request_token, item.message, &response, &response_length, &request_sent, FRONTEND_REQUEST_TIMEOUT_MS); if (status == CBM_DAEMON_RUNTIME_APPLICATION_TRANSPORT_ERROR && !request_sent) { - free(response); - response = NULL; - response_length = 0; + frontend_response_reset(&response, &response_length); bool cancelled = false; if (frontend_recover_client(state, &item, &cancelled)) { if (cancelled) { @@ -632,7 +636,7 @@ static void *frontend_worker(void *opaque) { item.content_length_framed); } } - free(response); + frontend_response_reset(&response, &response_length); bool expected_stop = failed && frontend_should_stop(state); frontend_end_request(state, failed && !expected_stop); frontend_item_free(&item); From 09dc36d26100236bc58969fda149f2c8b55bb1cb Mon Sep 17 00:00:00 2001 From: harshitaajoshi Date: Sun, 27 Sep 2026 18:16:16 -0500 Subject: [PATCH 05/10] fix(extract): parse PHP past inline HTML so symbols after ?> reach the graph (#2000) Signed-off-by: harshitaajoshi --- Makefile.cbm | 1 + internal/cbm/cbm.c | 14 ++ internal/cbm/cbm.h | 4 + internal/cbm/lsp/php_lsp.c | 4 + internal/cbm/php_inline_html.c | 317 +++++++++++++++++++++++++++++++++ tests/test_extraction.c | 128 +++++++++++++ tests/test_pipeline.c | 60 +++++++ 7 files changed, 528 insertions(+) create mode 100644 internal/cbm/php_inline_html.c diff --git a/Makefile.cbm b/Makefile.cbm index 6d19f430a..fec175f67 100644 --- a/Makefile.cbm +++ b/Makefile.cbm @@ -398,6 +398,7 @@ EXTRACTION_SRCS = \ $(CBM_DIR)/route_path.c \ $(CBM_DIR)/openapi.c \ $(CBM_DIR)/sql_values.c \ + $(CBM_DIR)/php_inline_html.c \ $(CBM_DIR)/helpers.c \ $(CBM_DIR)/cpp_branch_views.c \ $(CBM_DIR)/result_compact.c \ diff --git a/internal/cbm/cbm.c b/internal/cbm/cbm.c index 23b871dbc..4eb030ae0 100644 --- a/internal/cbm/cbm.c +++ b/internal/cbm/cbm.c @@ -3313,6 +3313,18 @@ CBMFileResult *cbm_extract_file(const char *source, int source_len, CBMLanguage return r; } +/* The bytes the grammar parses. PHP that leaves PHP mode (`?> markup ` (#2000). Same length and line breaks as the input; returns +// `source` itself when nothing needs rewriting. Defined in php_inline_html.c. +const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int source_len); void cbm_extract_imports(CBMExtractCtx *ctx); void cbm_extract_usages(CBMExtractCtx *ctx); void cbm_extract_semantic(CBMExtractCtx *ctx); diff --git a/internal/cbm/lsp/php_lsp.c b/internal/cbm/lsp/php_lsp.c index 7aae329e8..85118f671 100644 --- a/internal/cbm/lsp/php_lsp.c +++ b/internal/cbm/lsp/php_lsp.c @@ -20,6 +20,7 @@ #include "php_lsp.h" #include "lsp_node_iter.h" #include "lsp_work.h" +#include "../cbm.h" #include "../helpers.h" #include "../../../src/foundation/hash_table.h" #include @@ -4795,6 +4796,9 @@ void cbm_run_php_lsp_cross_with_bindings(CBMArena *arena, const char *source, in const CBMPHPImportBindings *bindings) { if (!arena || !source || source_len <= 0 || !out) return; + /* The retained source is the file on disk; read it the way the per-file + * extract did, so the cached tree and node text agree (#2000). */ + source = cbm_php_mask_inline_html(arena, source, source_len); TSParser *parser = NULL; TSTree *tree = cached_tree; diff --git a/internal/cbm/php_inline_html.c b/internal/cbm/php_inline_html.c new file mode 100644 index 000000000..df5652d54 --- /dev/null +++ b/internal/cbm/php_inline_html.c @@ -0,0 +1,317 @@ +// php_inline_html.c — let the php_only grammar read a file that leaves PHP mode. +// +// A PHP file starts in inline-HTML mode and may switch in and out of PHP any +// number of times: `?>
...
` or for the +// text that follows it, so every byte after the first close tag used to land in +// an ERROR region and the declarations there never reached the graph (#2000). +// +// Instead of parsing the markup, blank it. The rewritten buffer has the same +// length as the original and keeps every line break, so byte offsets, rows and +// columns are unchanged and node text inside PHP code is byte-identical: +// +// - markup before the first open tag and between tags becomes spaces +// - the first open tag stays, so the tree keeps its php_tag +// - each later `` becomes `; `, the statement terminator PHP itself treats it as +// +// Only code and `//` / `#` comments can close PHP mode. Strings, heredoc, +// nowdoc and block comments are skipped, `#[` is an attribute, and everything +// after __halt_compiler is left alone, as PHP stops reading there. + +#include "cbm.h" +#include "foundation/arena.h" +#include "foundation/constants.h" +#include +#include +#include + +enum { + PHP_TAG_OPEN_LEN = 5, /* = 0 && i <= len && (size_t)(len - i) >= n && memcmp(s + i, lit, n) == 0; +} + +static bool php_is_ident_char(unsigned char c) { + return c == '_' || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || + c >= PHP_FIRST_NON_ASCII; +} + +static bool php_is_ident_start(unsigned char c) { + return php_is_ident_char(c) && !(c >= '0' && c <= '9'); +} + +static bool php_is_space(unsigned char c) { + return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f' || c == '\v'; +} + +static bool php_is_blank(char c) { + return c == ' ' || c == '\t'; +} + +/* Case-insensitive match of the lowercase `word` at s[i]. */ +static bool php_ieq(const char *s, int len, int i, const char *word) { + int n = (int)strlen(word); + if (i + n > len) { + return false; + } + for (int k = 0; k < n; k++) { + char c = s[i + k]; + if (c >= 'A' && c <= 'Z') { + c = (char)(c - 'A' + 'a'); + } + if (c != word[k]) { + return false; + } + } + return true; +} + +/* Length of the open tag at i, or 0. `` prolog in markup stays markup. */ +static int php_open_tag_len(const char *s, int len, int i) { + if (!php_at(s, len, i, "")) { + return true; + } + } + return false; +} + +/* Blank [from, to) in place, keeping line breaks. */ +static void php_blank(char *out, int from, int to) { + for (int k = from; k < to; k++) { + if (out[k] != '\n' && out[k] != '\r') { + out[k] = ' '; + } + } +} + +/* Index just past the quoted string opened at i. */ +static int php_skip_quoted(const char *s, int len, int i) { + char quote = s[i]; + i += SKIP_ONE; + while (i < len && s[i] != quote) { + i += (s[i] == '\\') ? PAIR_LEN : SKIP_ONE; + } + return i < len ? i + SKIP_ONE : len; +} + +/* Index just past the block comment opened at i. */ +static int php_skip_block_comment(const char *s, int len, int i) { + for (int k = i + PAIR_LEN; k < len; k++) { + if (php_at(s, len, k, "*/")) { + return k + PAIR_LEN; + } + } + return len; +} + +/* Index of the line break or `?>` that ends the one-line comment at i. */ +static int php_skip_line_comment(const char *s, int len, int i) { + while (i < len && s[i] != '\n' && !php_at(s, len, i, "?>")) { + i++; + } + return i; +} + +/* Parse the heredoc/nowdoc opener `<<= len || s[p] != quote) { + return PHP_NOT_FOUND; + } + p++; + } + if (p < len && s[p] == '\r') { + p++; + } + return (p < len && s[p] == '\n') ? p : PHP_NOT_FOUND; +} + +/* True when the line starting at p closes the heredoc. PHP 7.3+ allows the + * closing label to be indented and followed by any non-identifier byte. */ +static bool php_heredoc_closes(const char *s, int len, int p, int label, int label_len, int *end) { + while (p < len && php_is_blank(s[p])) { + p++; + } + if (len - p < label_len || memcmp(s + p, s + label, (size_t)label_len) != 0) { + return false; + } + int after = p + label_len; + if (after < len && php_is_ident_char((unsigned char)s[after])) { + return false; + } + *end = after; + return true; +} + +/* Index just past the heredoc/nowdoc opened by the `<<<` at i. */ +static int php_skip_heredoc(const char *s, int len, int i) { + int label = 0; + int label_len = 0; + int nl = php_heredoc_opener(s, len, i, &label, &label_len); + if (nl < 0) { + return i + PHP_HEREDOC_LEN; + } + while (nl < len) { + int line = nl + SKIP_ONE; + int end = 0; + if (php_heredoc_closes(s, len, line, label, label_len, &end)) { + return end; + } + const char *next = memchr(s + line, '\n', (size_t)(len - line)); + if (!next) { + return len; + } + nl = (int)(next - s); + } + return len; +} + +/* Index just past the identifier at i; sets *halt when it is __halt_compiler + * (a keyword, so not a `$variable` of that name). */ +static int php_skip_ident(const char *s, int len, int i, bool *halt) { + int start = i; + while (i < len && php_is_ident_char((unsigned char)s[i])) { + i++; + } + bool is_var = start > 0 && s[start - SKIP_ONE] == '$'; + *halt = + !is_var && i - start == PHP_HALT_COMPILER_LEN && php_ieq(s, len, start, PHP_HALT_COMPILER); + return i; +} + +/* Scan PHP code from i to the next close tag. Returns the index of that `?>`, + * len when the file ends in PHP mode, or PHP_NOT_FOUND at __halt_compiler. */ +static int php_scan_code(const char *s, int len, int i) { + while (i < len) { + unsigned char c = (unsigned char)s[i]; + if (php_at(s, len, i, "?>")) { + return i; + } + if (c == '\'' || c == '"' || c == '`') { + i = php_skip_quoted(s, len, i); + } else if (php_at(s, len, i, "/*")) { + i = php_skip_block_comment(s, len, i); + } else if (php_at(s, len, i, "//") || (c == '#' && !php_at(s, len, i, "#["))) { + i = php_skip_line_comment(s, len, i); + } else if (php_at(s, len, i, "<<<")) { + i = php_skip_heredoc(s, len, i); + } else if (php_is_ident_start(c)) { + bool halt = false; + i = php_skip_ident(s, len, i, &halt); + if (halt) { + return PHP_NOT_FOUND; + } + } else { + i++; + } + } + return len; +} + +const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int source_len) { + if (!arena || !source || source_len <= 0) { + return source; + } + /* Fast path: a file that opens PHP at its first byte and never writes a + * close tag is exactly what the grammar already accepts. */ + if (php_open_tag_len(source, source_len, 0) > 0 && !php_has_close_tag(source, source_len)) { + return source; + } + + char *out = (char *)cbm_arena_alloc(arena, (size_t)source_len + SKIP_ONE); + if (!out) { + return source; + } + memcpy(out, source, (size_t)source_len); + out[source_len] = '\0'; + + bool changed = false; + bool seen_open = false; + int i = 0; + while (i < source_len) { + /* Inline-HTML mode: blank up to the next open tag. */ + int html_start = i; + int tag = 0; + while (i < source_len && (tag = php_open_tag_len(source, source_len, i)) == 0) { + i++; + } + if (i > html_start) { + php_blank(out, html_start, i); + changed = true; + } + if (i >= source_len) { + break; + } + if (seen_open) { + php_blank(out, i, i + tag); + changed = true; + } + seen_open = true; + + /* PHP mode, up to the close tag. */ + int close = php_scan_code(source, source_len, i + tag); + if (close < 0 || close >= source_len) { + break; + } + out[close] = ';'; + out[close + SKIP_ONE] = ' '; + changed = true; + i = close + PAIR_LEN; + } + return changed ? out : source; +} diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 7bfda63fc..c8d47ca52 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -833,6 +833,130 @@ TEST(php_function) { PASS(); } +/* #2000: inline HTML between `?>` and `` fell into an + * ERROR region and never reached the graph. */ +static const CBMCall *find_call_named(CBMFileResult *r, const char *callee) { + for (int i = 0; i < r->calls.count; i++) { + if (r->calls.items[i].callee_name && strcmp(r->calls.items[i].callee_name, callee) == 0) + return &r->calls.items[i]; + } + return NULL; +} + +static int php_def_line(CBMFileResult *r, const char *name) { + for (int i = 0; i < r->defs.count; i++) { + if (strcmp(r->defs.items[i].name, name) == 0) + return (int)r->defs.items[i].start_line; + } + return -1; +} + +TEST(php_inline_html_tail_reaches_graph_issue2000) { + CBMFileResult *r = extract("\n" + "
\n" + "

plain markup after the closing tag

\n" + "
\n" + "has_error); + ASSERT(has_def(r, "Function", "gamma")); + ASSERT(has_def(r, "Function", "delta")); + /* Lines are in original-file coordinates. */ + ASSERT_EQ(php_def_line(r, "gamma"), 2); + ASSERT_EQ(php_def_line(r, "delta"), 8); + const CBMCall *c = find_call_named(r, "gamma"); + ASSERT_NOT_NULL(c); + ASSERT_NOT_NULL(strstr(c->enclosing_func_qn, "delta")); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + +/* View-file shapes: echo tags keep their calls, and control structures that + * are opened in one PHP block and closed in a later one still parse. */ +TEST(php_inline_html_template_shapes_issue2000) { + CBMFileResult *r = extract("\n" + "<?= page_title($page) ?>\n" + "\n" + "\n" + "

Hello name?>

\n" + "\n" + " log in\n" + "\n" + "\n" + "
    • \n" + "\n" + "\n" + "\n", + CBM_LANG_PHP, "t", "view.php"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT(has_call(r, "page_title")); + ASSERT(has_call(r, "render_item")); + ASSERT(has_def(r, "Function", "footer_links")); + ASSERT_EQ(php_def_line(r, "footer_links"), 12); + ASSERT(has_call(r, "nav_links")); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + +/* `?>` only leaves PHP mode from code or a one-line comment. Inside strings, + * heredoc, nowdoc and block comments it is text; `#[` is an attribute, not a + * comment; ` quoted';\n" + "$b = \"double ?> quoted \\\" still\";\n" + "$c = << body\n" + " EOT;\n" + "$d = <<<'NOW'\n" + "nowdoc ?> body\n" + "NOW;\n" + "/* block ?> comment */\n" + "#[Pure]\n" + "function one() { return 1; }\n" + "// line comment ends here ?>\n" + "\n" + "has_error); + ASSERT(has_def(r, "Function", "one")); + ASSERT(has_def(r, "Function", "two")); + ASSERT_EQ(php_def_line(r, "two"), 16); + ASSERT(has_call(r, "one")); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + +/* A trailing `?>` (common at the end of older class files) is not a partial + * parse, and CRLF line endings keep their line numbers. */ +TEST(php_inline_html_trailing_close_tag_crlf_issue2000) { + CBMFileResult *r = extract("\r\n", + CBM_LANG_PHP, "t", "Repo.php"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT(has_def(r, "Class", "Repo")); + ASSERT(has_def(r, "Method", "find")); + ASSERT_EQ(php_def_line(r, "find"), 3); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + /* --- Ruby --- */ TEST(ruby_class) { CBMFileResult *r = extract("class Animal\n def initialize(name)\n @name = name\n end\n " @@ -13125,6 +13249,10 @@ SUITE(extraction) { RUN_TEST(python_class_base_extracted_bare); RUN_TEST(php_class); RUN_TEST(php_function); + RUN_TEST(php_inline_html_tail_reaches_graph_issue2000); + RUN_TEST(php_inline_html_template_shapes_issue2000); + RUN_TEST(php_inline_html_close_tag_in_literals_issue2000); + RUN_TEST(php_inline_html_trailing_close_tag_crlf_issue2000); RUN_TEST(ruby_class); RUN_TEST(ruby_module); RUN_TEST(csharp_class); diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index d9b327226..94a281a07 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -1376,6 +1376,65 @@ TEST(pipeline_nix_scoped_binding_calls_resolve) { PASS(); } +/* #2000, end to end: functions declared after an inline-HTML block reach the + * store with their CALLS edges, in the same file and across files. */ +TEST(pipeline_php_inline_html_calls_resolve_issue2000) { + if (setup_test_repo() != 0) { + FAIL("failed to create temp dir"); + } + + static const struct { + const char *name; + const char *body; + } files[] = { + {"inline_html.php", "\n" + "
    \n" + "

    plain markup after the closing tag

    \n" + "
    \n" + "\n" + "\n" + "

    \n" + " Date: Sun, 27 Sep 2026 18:55:55 -0500 Subject: [PATCH 06/10] fix(extract): address review on PHP inline-HTML masking (#2000) Signed-off-by: harshitaajoshi --- internal/cbm/php_inline_html.c | 65 +++++++++++++++++++++++++--------- tests/test_extraction.c | 52 +++++++++++++++++++++++++++ tests/test_php_lsp.c | 60 +++++++++++++++++++++++++++++++ tests/test_pipeline.c | 10 +----- 4 files changed, 161 insertions(+), 26 deletions(-) diff --git a/internal/cbm/php_inline_html.c b/internal/cbm/php_inline_html.c index df5652d54..e766a1fbd 100644 --- a/internal/cbm/php_inline_html.c +++ b/internal/cbm/php_inline_html.c @@ -263,6 +263,47 @@ static int php_scan_code(const char *s, int len, int i) { return len; } +/* The rewritten buffer. It is allocated and filled from the source on the + * first write, so a file whose only `?>` bytes sit in strings or comments + * costs no copy. */ +typedef struct { + CBMArena *arena; + const char *source; + int len; + char *out; + bool failed; +} PHPMask; + +static char *php_mask_writable(PHPMask *m) { + if (!m->out && !m->failed) { + m->out = (char *)cbm_arena_alloc(m->arena, (size_t)m->len + SKIP_ONE); + if (m->out) { + memcpy(m->out, m->source, (size_t)m->len); + m->out[m->len] = '\0'; + } else { + m->failed = true; + } + } + return m->out; +} + +/* Blank [from, to) of the rewritten buffer, keeping line breaks. */ +static void php_mask_blank(PHPMask *m, int from, int to) { + char *out = php_mask_writable(m); + if (out) { + php_blank(out, from, to); + } +} + +/* Turn the `?>` at i into `; `. */ +static void php_mask_close_tag(PHPMask *m, int i) { + char *out = php_mask_writable(m); + if (out) { + out[i] = ';'; + out[i + SKIP_ONE] = ' '; + } +} + const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int source_len) { if (!arena || !source || source_len <= 0) { return source; @@ -273,17 +314,10 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so return source; } - char *out = (char *)cbm_arena_alloc(arena, (size_t)source_len + SKIP_ONE); - if (!out) { - return source; - } - memcpy(out, source, (size_t)source_len); - out[source_len] = '\0'; - - bool changed = false; + PHPMask m = {.arena = arena, .source = source, .len = source_len}; bool seen_open = false; int i = 0; - while (i < source_len) { + while (i < source_len && !m.failed) { /* Inline-HTML mode: blank up to the next open tag. */ int html_start = i; int tag = 0; @@ -291,15 +325,13 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so i++; } if (i > html_start) { - php_blank(out, html_start, i); - changed = true; + php_mask_blank(&m, html_start, i); } if (i >= source_len) { break; } if (seen_open) { - php_blank(out, i, i + tag); - changed = true; + php_mask_blank(&m, i, i + tag); } seen_open = true; @@ -308,10 +340,9 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so if (close < 0 || close >= source_len) { break; } - out[close] = ';'; - out[close + SKIP_ONE] = ' '; - changed = true; + php_mask_close_tag(&m, close); i = close + PAIR_LEN; } - return changed ? out : source; + /* On allocation failure parse the file as it is: the old behaviour. */ + return (m.out && !m.failed) ? m.out : source; } diff --git a/tests/test_extraction.c b/tests/test_extraction.c index c8d47ca52..4d03423c1 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -938,6 +938,56 @@ TEST(php_inline_html_close_tag_in_literals_issue2000) { PASS(); } +/* Markup is blanked, never parsed: code-shaped text outside the PHP tags must + * not produce definitions or calls, wherever it sits. */ +TEST(php_inline_html_markup_never_reaches_graph_issue2000) { + CBMFileResult *r = extract("

    function before() { return leakBefore(); }

    \n" + "\n" + "

    function fake() { return leak(); }

    \n" + "\n" + "function xmlFake() { leakXml(); }\n" + "\n" + "

    class Tail { function tailFake() { leakTail(); } }

    \n", + CBM_LANG_PHP, "t", "markup.php"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT(has_def(r, "Function", "real")); + ASSERT(has_def(r, "Function", "after")); + ASSERT(has_call(r, "real")); + ASSERT_FALSE(has_def_any(r, "before")); + ASSERT_FALSE(has_def_any(r, "fake")); + ASSERT_FALSE(has_def_any(r, "jsFake")); + ASSERT_FALSE(has_def_any(r, "xmlFake")); + ASSERT_FALSE(has_def_any(r, "Tail")); + ASSERT_FALSE(has_def_any(r, "tailFake")); + ASSERT_FALSE(has_call(r, "leak")); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + +/* The mask hands back the caller's buffer, uncopied, when nothing needs + * rewriting, and a same-length rewrite otherwise. */ +TEST(php_inline_html_mask_copies_only_on_write_issue2000) { + CBMArena arena; + cbm_arena_init(&arena); + const char *plain = "';\n/* ?> */\n$h = <<\nEOT;\n"; + const char *mixed = "\nx\n"; + ASSERT(cbm_php_mask_inline_html(&arena, plain, (int)strlen(plain)) == plain); + ASSERT(cbm_php_mask_inline_html(&arena, quoted, (int)strlen(quoted)) == quoted); + const char *masked = cbm_php_mask_inline_html(&arena, mixed, (int)strlen(mixed)); + ASSERT(masked != mixed); + ASSERT_EQ((int)strlen(masked), (int)strlen(mixed)); + ASSERT_STR_EQ(masked, "` (common at the end of older class files) is not a partial * parse, and CRLF line endings keep their line numbers. */ TEST(php_inline_html_trailing_close_tag_crlf_issue2000) { @@ -13252,6 +13302,8 @@ SUITE(extraction) { RUN_TEST(php_inline_html_tail_reaches_graph_issue2000); RUN_TEST(php_inline_html_template_shapes_issue2000); RUN_TEST(php_inline_html_close_tag_in_literals_issue2000); + RUN_TEST(php_inline_html_markup_never_reaches_graph_issue2000); + RUN_TEST(php_inline_html_mask_copies_only_on_write_issue2000); RUN_TEST(php_inline_html_trailing_close_tag_crlf_issue2000); RUN_TEST(ruby_class); RUN_TEST(ruby_module); diff --git a/tests/test_php_lsp.c b/tests/test_php_lsp.c index 04ff1001d..7c0314a27 100644 --- a/tests/test_php_lsp.c +++ b/tests/test_php_lsp.c @@ -110,6 +110,65 @@ TEST(phplsp_local_method_via_typed_param) { PASS(); } +/* #2000: with no cached tree (a result spilled under the memory budget comes + * back without one) the cross-file pass re-parses the on-disk source itself. + * It must blank inline HTML exactly as the per-file extract does, or a file's + * tail would lose cross-file resolution only when that file spilled. */ +static int find_resolved_arr(const CBMResolvedCallArray *arr, const char *callerSub, + const char *calleeSub) { + for (int i = 0; i < arr->count; i++) { + const CBMResolvedCall *rc = &arr->items[i]; + if (rc->caller_qn && strstr(rc->caller_qn, callerSub) && rc->callee_qn && + strstr(rc->callee_qn, calleeSub)) + return i; + } + return -1; +} + +TEST(phplsp_cross_reparse_reads_past_inline_html_issue2000) { + const char *src = "\n" + "
    \n" + "

    plain markup after the closing tag

    \n" + "
    \n" + "send(); }\n"; + CBMLSPDef defs[] = { + {.qualified_name = "test.main.gamma", + .short_name = "gamma", + .label = "Function", + .def_module_qn = "test.main"}, + {.qualified_name = "test.main.delta", + .short_name = "delta", + .label = "Function", + .def_module_qn = "test.main"}, + {.qualified_name = "test.mailer.Mailer", + .short_name = "Mailer", + .label = "Class", + .def_module_qn = "test.mailer"}, + {.qualified_name = "test.mailer.Mailer.send", + .short_name = "send", + .label = "Method", + .def_module_qn = "test.mailer", + .receiver_type = "test.mailer.Mailer"}, + }; + const char *imp_names[] = {"Mailer"}; + const char *imp_qns[] = {"test.mailer.Mailer"}; + + CBMArena arena; + cbm_arena_init(&arena); + CBMResolvedCallArray out = {0}; + + cbm_run_php_lsp_cross(&arena, src, (int)strlen(src), "test.main", defs, 4, imp_names, imp_qns, + 1, NULL, &out); + + ASSERT_GTE(find_resolved_arr(&out, "main.delta", "Mailer.send"), 0); + + cbm_arena_destroy(&arena); + PASS(); +} + /* ── 3. Arrow function with typed parameter ───────────────────── */ TEST(phplsp_arrow_function_typed_param) { @@ -5841,6 +5900,7 @@ SUITE(php_lsp) { /* Phase 1 baseline regressions */ RUN_TEST(phplsp_local_method_via_new_assignment); RUN_TEST(phplsp_local_method_via_typed_param); + RUN_TEST(phplsp_cross_reparse_reads_past_inline_html_issue2000); RUN_TEST(phplsp_arrow_function_typed_param); RUN_TEST(phplsp_static_call_resolved); RUN_TEST(phplsp_self_and_parent); diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 94a281a07..c8094efac 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -1405,15 +1405,7 @@ TEST(pipeline_php_inline_html_calls_resolve_issue2000) { "function renderCard($item) { return formatPrice($item['cents']); }\n"}, }; for (size_t i = 0; i < sizeof(files) / sizeof(files[0]); i++) { - char path[512]; - snprintf(path, sizeof(path), "%s/%s", g_tmpdir, files[i].name); - FILE *f = fopen(path, "w"); - if (!f) { - teardown_test_repo(); - FAIL("failed to write php fixture"); - } - fputs(files[i].body, f); - fclose(f); + write_temp_file(g_tmpdir, files[i].name, files[i].body); } char db[512]; From bbdeeb61a49c91fb13665a3512b7f36a4ed7e0dd Mon Sep 17 00:00:00 2001 From: harshitaajoshi Date: Mon, 28 Sep 2026 13:10:32 -0500 Subject: [PATCH 07/10] test(extract): pin PHP mask copy-on-write via arena allocation total (#2000) Signed-off-by: harshitaajoshi --- tests/test_extraction.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 4d03423c1..ad6ecd863 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -971,16 +971,28 @@ TEST(php_inline_html_markup_never_reaches_graph_issue2000) { } /* The mask hands back the caller's buffer, uncopied, when nothing needs - * rewriting, and a same-length rewrite otherwise. */ + * rewriting, and a same-length rewrite otherwise. The pointer alone cannot + * tell copy-on-write from an eager copy, so the arena's allocation total is + * sampled around each call: no bytes for `plain` and `quoted`, a copy for + * `mixed`. */ TEST(php_inline_html_mask_copies_only_on_write_issue2000) { CBMArena arena; cbm_arena_init(&arena); const char *plain = "';\n/* ?> */\n$h = <<\nEOT;\n"; const char *mixed = "\nx\n"; + + size_t before = cbm_arena_total(&arena); ASSERT(cbm_php_mask_inline_html(&arena, plain, (int)strlen(plain)) == plain); + ASSERT_EQ(cbm_arena_total(&arena), before); + + before = cbm_arena_total(&arena); ASSERT(cbm_php_mask_inline_html(&arena, quoted, (int)strlen(quoted)) == quoted); + ASSERT_EQ(cbm_arena_total(&arena), before); + + before = cbm_arena_total(&arena); const char *masked = cbm_php_mask_inline_html(&arena, mixed, (int)strlen(mixed)); + ASSERT(cbm_arena_total(&arena) > before); ASSERT(masked != mixed); ASSERT_EQ((int)strlen(masked), (int)strlen(mixed)); ASSERT_STR_EQ(masked, " Date: Sun, 1 Nov 2026 18:34:34 +0000 Subject: [PATCH 08/10] build(deps): bump actions/download-artifact from 8.0.1 to 8.0.2 Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 8.0.1 to 8.0.2. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](https://github.com/actions/download-artifact/compare/3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c...9000827ccba6bdab643e8b6fd33ac0654aef8333) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-version: 8.0.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/_build.yml | 2 +- .github/workflows/_smoke.yml | 6 +++--- .github/workflows/_soak.yml | 8 ++++---- .github/workflows/_test.yml | 4 ++-- .github/workflows/release.yml | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/.github/workflows/_build.yml b/.github/workflows/_build.yml index 152cca737..dac1dde67 100644 --- a/.github/workflows/_build.yml +++ b/.github/workflows/_build.yml @@ -317,7 +317,7 @@ jobs: persist-credentials: false - name: Download all native candidate pairs - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: pattern: release-candidates-* path: ${{ runner.temp }}/release-candidate-artifacts diff --git a/.github/workflows/_smoke.yml b/.github/workflows/_smoke.yml index ed1022b71..932ee858d 100644 --- a/.github/workflows/_smoke.yml +++ b/.github/workflows/_smoke.yml @@ -103,7 +103,7 @@ jobs: go-version-file: pkg/go/go.mod cache: false - - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-${{ matrix.goos }}-${{ matrix.goarch }} @@ -203,7 +203,7 @@ jobs: zip coreutils - - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-windows-${{ matrix.arch }} @@ -303,7 +303,7 @@ jobs: go-version-file: pkg/go/go.mod cache: false - - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-linux-${{ matrix.arch }}-portable diff --git a/.github/workflows/_soak.yml b/.github/workflows/_soak.yml index 39f9e76b1..4eb0ac44a 100644 --- a/.github/workflows/_soak.yml +++ b/.github/workflows/_soak.yml @@ -73,7 +73,7 @@ jobs: run: scripts/build.sh ${{ inputs.version && format('--version {0}', inputs.version) || '' }} CC=${{ matrix.cc }} CXX=${{ matrix.cxx }} - name: Download selected release container if: ${{ inputs.use_release_artifacts }} - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-${{ matrix.goos }}-${{ matrix.goarch }} path: ${{ runner.temp }}/cbm-selected-container @@ -147,7 +147,7 @@ jobs: run: scripts/build.sh ${{ inputs.version && format('--version {0}', inputs.version) || '' }} CC=clang CXX=clang++ - name: Download selected release container if: ${{ inputs.use_release_artifacts }} - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-windows-amd64 path: ${{ runner.temp }}/cbm-selected-container @@ -225,7 +225,7 @@ jobs: run: scripts/build.sh ${{ inputs.version && format('--version {0}', inputs.version) || '' }} CC=clang CXX=clang++ - name: Download selected release container if: ${{ inputs.use_release_artifacts }} - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-windows-arm64 path: ${{ runner.temp }}/cbm-selected-container @@ -287,7 +287,7 @@ jobs: timeout-minutes: 10 run: APT_RETRIES=3; [ -f /etc/apt/apt-mirrors.txt ] && APT_RETRIES=0; sudo apt-get -o Acquire::Retries=$APT_RETRIES -o Acquire::http::Timeout=30 -o Acquire::https::Timeout=30 --error-on=any update && sudo apt-get -o Acquire::Retries=$APT_RETRIES -o Acquire::http::Timeout=30 -o Acquire::https::Timeout=30 install -y python3 git - name: Download selected portable release container - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: binaries-linux-${{ matrix.arch }}-portable path: ${{ runner.temp }}/cbm-selected-container diff --git a/.github/workflows/_test.yml b/.github/workflows/_test.yml index 501a82b20..2f89e79e6 100644 --- a/.github/workflows/_test.yml +++ b/.github/workflows/_test.yml @@ -231,7 +231,7 @@ jobs: # producer, so arm64, macOS and Windows code under #ifdef has static # evidence only and those legs run every test. if: ${{ inputs.test_selection != '' && runner.os == 'Linux' && runner.arch == 'X64' }} - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: ${{ inputs.test_selection }} path: ${{ runner.temp }}/test-selection @@ -720,7 +720,7 @@ jobs: with: path: repo - - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: pattern: shard-manifest-* path: manifests diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e4926260d..c70330c7e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -185,7 +185,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Download only selected release containers - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: pattern: binaries-* merge-multiple: true @@ -194,7 +194,7 @@ jobs: # download above. Preserve only the three reviewable manifests that bind # candidate bytes, VT results, and the tuple-local decision. - name: Download candidate selection evidence separately - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: release-selection-evidence path: selection-evidence @@ -617,7 +617,7 @@ jobs: # the release-draft job as a same-run artifact (not a draft-release # download — that would need contents beyond read plus gh). - name: Fetch release checksums - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: release-checksums From bf30a1aaf01d01f750fdfa695baec34ed977dd53 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 1 Nov 2026 23:48:31 +0100 Subject: [PATCH 09/10] fix(daemon): replay the session within the startup budget The recovery path replays the session context and UI settings on the replacement daemon. It waited on the bootstrap connect budget, which only bounds a probe (1 s in production), while main.c gives the first set_context and set_ui_config the startup budget since eb294298e. A replacement daemon whose UI save runs longer than a probe would end an otherwise good recovery. frontend_configure_client now waits on startup_timeout_ms. A test seam records the budget the replay used, and the recovery fixture gives the bootstrap distinct connect and startup budgets and exits 75 when the replay waited on the wrong one. With the old budget exactly the three recovery tests fail with exit 75; with the fix daemon_frontend is 17/17. Signed-off-by: Martin Vogel --- src/daemon/frontend.c | 14 +++++++++++++- src/daemon/frontend.h | 4 ++++ tests/test_daemon_frontend.c | 15 ++++++++++++++- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/daemon/frontend.c b/src/daemon/frontend.c index 123ec00e4..e76c920b6 100644 --- a/src/daemon/frontend.c +++ b/src/daemon/frontend.c @@ -120,6 +120,7 @@ static atomic_uint_fast64_t frontend_test_monitor_observation_count = ATOMIC_VAR static atomic_uint_fast64_t frontend_test_worker_observation_count = ATOMIC_VAR_INIT(0); static atomic_uint_fast64_t frontend_test_worker_idle_count = ATOMIC_VAR_INIT(0); static atomic_uint_fast64_t frontend_test_routed_cancellation_count = ATOMIC_VAR_INIT(0); +static atomic_uint_fast32_t frontend_test_replay_budget_ms = ATOMIC_VAR_INIT(0); void cbm_daemon_frontend_test_observer_reset(bool hold_monitor) { atomic_store_explicit(&frontend_test_monitor_observation_count, 0, memory_order_release); @@ -152,6 +153,10 @@ uint64_t cbm_daemon_frontend_test_worker_idle_cycles(void) { uint64_t cbm_daemon_frontend_test_routed_cancellations(void) { return atomic_load_explicit(&frontend_test_routed_cancellation_count, memory_order_acquire); } + +uint32_t cbm_daemon_frontend_test_replay_timeout_ms(void) { + return (uint32_t)atomic_load_explicit(&frontend_test_replay_budget_ms, memory_order_acquire); +} #endif static cbm_version_cohort_maintenance_presence_t frontend_observe_maintenance( @@ -528,7 +533,14 @@ static cbm_daemon_runtime_cancel_result_t frontend_cancel_active(frontend_state_ static bool frontend_configure_client(cbm_daemon_runtime_client_t *client, const cbm_daemon_frontend_session_config_t *session) { - uint32_t timeout_ms = session->bootstrap.connect_timeout_ms; + /* The replay restores what startup set, so it waits within the startup + * budget like the first set_context and set_ui_config in main.c. The + * connect budget only bounds a probe (1 s in production): a replacement + * daemon whose UI save runs longer would end an otherwise good recovery. */ + uint32_t timeout_ms = session->bootstrap.startup_timeout_ms; +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS + atomic_store_explicit(&frontend_test_replay_budget_ms, timeout_ms, memory_order_release); +#endif if (cbm_daemon_application_client_set_context( client, session->session_root, session->allowed_root, session->tool_profile, NULL, NULL, timeout_ms) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { diff --git a/src/daemon/frontend.h b/src/daemon/frontend.h index 8f59ec28b..8ee33d276 100644 --- a/src/daemon/frontend.h +++ b/src/daemon/frontend.h @@ -79,6 +79,10 @@ uint64_t cbm_daemon_frontend_test_worker_idle_cycles(void); * Tests snapshot this monotonic * count before sending a notification. */ uint64_t cbm_daemon_frontend_test_routed_cancellations(void); +/* Budget in ms that the most recent session replay gave its context and UI + * settings requests. Recovery tests use distinct connect and startup budgets + * and compare. */ +uint32_t cbm_daemon_frontend_test_replay_timeout_ms(void); #endif /* Takes ownership of client and borrows cohort_manager for the complete call. diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index e4ed5e135..aa49bf4c7 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -250,6 +250,11 @@ enum { * no-progress EOF drain so shutdown deterministically overlaps recovery. */ FRONTEND_RECOVERY_EOF_RELEASE_MS = 16000, + /* Startup budget of the recovery bootstrap. It differs from the connect + * budget (FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS) so the recovery tests can + * tell which one the session replay waited on, and it is the larger one, + * as in production (1 s connect probe, 30 s startup). */ + FRONTEND_RECOVERY_TEST_STARTUP_TIMEOUT_MS = 45000, }; typedef enum { @@ -818,7 +823,7 @@ static int frontend_recovery_child_run(const char *parent, frontend_recovery_mod .identity = &fixture.identity, .executable_path = "unused", .connect_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, - .startup_timeout_ms = FRONTEND_EOF_TEST_REQUEST_TIMEOUT_MS, + .startup_timeout_ms = FRONTEND_RECOVERY_TEST_STARTUP_TIMEOUT_MS, }, .session_root = parent, .tool_profile = CBM_MCP_TOOL_PROFILE_ALL, @@ -851,6 +856,14 @@ static int frontend_recovery_child_run(const char *parent, frontend_recovery_mod bool input_closed = input && fclose(input) == 0; bool output_closed = output && fclose(output) == 0; bool fixture_closed = frontend_eof_fixture_finish(&fixture); +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS + /* The replay restores what startup set within the startup budget. On the + * connect budget (a probe bound) a replacement daemon that answers slower + * than a probe would end an otherwise good recovery. Every mode replays. */ + if (cbm_daemon_frontend_test_replay_timeout_ms() != FRONTEND_RECOVERY_TEST_STARTUP_TIMEOUT_MS) { + return 75; + } +#endif return replacement_ready && request_ready && result == 0 && response_exact && requests == expected_requests && writer_ok && input_closed && output_closed && fixture_closed From e8f0f4a2898b6d2b4468dd6cb984a39372e86ded Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 2 Nov 2026 18:23:18 +0100 Subject: [PATCH 10/10] fix(cli): accept the emptied hooks: {} mapping in the Hermes hook editor Removing the only Hermes pre_llm_call hook empties the hooks mapping, and the mapping editor writes an emptied section as hooks: {} - the same convention it uses for mcp_servers: {}. The hook sequence editor then refused that file for both upsert and removal, because it required every parent on the sequence path to be a block mapping header. On a config where our hook was the only entry under hooks, install -> uninstall -> install exited 1, and a repeated uninstall did too. An exact empty flow mapping on a parent of the sequence path is now read as that empty mapping, mirroring how the mapping-entry editor already treats section: {}. Removal is a byte-identical no-op, and upsert expands the header in place, keeping its comment, the line endings and every other byte, so a reinstall lands back on the first install byte for byte. The sequence key itself holding a mapping, any other flow value, and content nested under the inline mapping stay refused. Hooks written by the user and every foreign key are left untouched. Found while finishing #1862: its persisted hook opt-out runs this remover on every later install, so every Hermes user whose only hook was ours would have hit the refusal on each install after the first. Signed-off-by: Martin Vogel --- src/cli/config_yaml_edit.c | 87 +++++++++++++++++++-- tests/test_cli.c | 139 ++++++++++++++++++++++++++++++++++ tests/test_config_yaml_edit.c | 126 ++++++++++++++++++++++++++++++ 3 files changed, 346 insertions(+), 6 deletions(-) diff --git a/src/cli/config_yaml_edit.c b/src/cli/config_yaml_edit.c index a6f8ced10..09a079212 100644 --- a/src/cli/config_yaml_edit.c +++ b/src/cli/config_yaml_edit.c @@ -2589,6 +2589,10 @@ typedef struct { bool identity_found; size_t identity_start; size_t identity_end; + /* The missing rest of the path begins under a `parent: {}` header. */ + bool parent_inline_empty; + size_t parent_line; + size_t parent_colon; } yaml_mapping_sequence_target_t; static void yaml_sequence_key_vec_free(yaml_sequence_key_vec_t *keys) { @@ -2962,6 +2966,37 @@ static int yaml_sequence_parse_block(const yaml_doc_t *doc, size_t begin_line, s return 0; } +/* `section: {}` is how the mapping-entry editor writes a section it emptied + * (yaml_build_last_mapping_entry_removal), and its upsert accepts it back. On + * a PARENT of the sequence path it is that same empty mapping, so the rest of + * the path is missing beneath it: removal is a no-op and upsert expands the + * header in place. The Hermes remover leaves `hooks: {}` whenever our hook was + * the only entry. The sequence key itself holding `{}`, any other flow value, + * and content nested under the inline mapping stay refused. */ +static int yaml_sequence_target_empty_parent(const yaml_doc_t *doc, size_t key_line, size_t colon, + size_t child_end, size_t next_depth, + size_t sequence_path_len, + yaml_mapping_sequence_target_t *target) { + bool inline_empty = false; + if (next_depth >= sequence_path_len || + yaml_tail_is_explicit_empty_mapping(doc->data, colon, doc->lines[key_line].text_end, + &inline_empty) != 0 || + !inline_empty) { + return YAML_ERROR; + } + for (size_t i = key_line + YAML_UNIT; i < child_end; i++) { + if (!doc->lines[i].blank && !doc->lines[i].comment) { + return YAML_ERROR; + } + } + target->missing_index = next_depth; + target->insert_offset = yaml_sequence_line_offset(doc, child_end); + target->parent_inline_empty = true; + target->parent_line = key_line; + target->parent_colon = colon; + return 0; +} + static int yaml_sequence_analyze(const yaml_doc_t *doc, const char *const *sequence_path, const size_t *path_lengths, size_t sequence_path_len, const char *identity_key, const char *identity_value, @@ -2993,10 +3028,11 @@ static int yaml_sequence_analyze(const yaml_doc_t *doc, const char *const *seque return 0; } const yaml_line_t *header = &doc->lines[key_line]; - if (!yaml_tail_is_empty(doc->data, colon, header->text_end)) { - return YAML_ERROR; - } size_t child_end = yaml_sequence_nested_end(doc, key_line, parent_end); + if (!yaml_tail_is_empty(doc->data, colon, header->text_end)) { + return yaml_sequence_target_empty_parent(doc, key_line, colon, child_end, + depth + YAML_UNIT, sequence_path_len, target); + } if (depth + YAML_UNIT == sequence_path_len) { target->sequence_found = true; target->missing_index = sequence_path_len; @@ -3095,13 +3131,52 @@ static int yaml_sequence_validate_canonical(const char *canonical_item, size_t c return result; } +/* Copies the document up to insert_offset with the `parent: {}` header turned + * into a block header `parent:`, exactly as + * yaml_build_inline_empty_mapping_section_add does for a mapping section: a + * trailing comment and the line ending are kept. */ +static int yaml_sequence_append_expanded_parent(yaml_buf_t *out, const yaml_doc_t *doc, + const yaml_mapping_sequence_target_t *target) { + if (target->parent_line >= doc->line_count) { + return YAML_ERROR; + } + const yaml_line_t *header = &doc->lines[target->parent_line]; + size_t comment = 0U; + if (target->insert_offset < header->end || + yaml_find_comment(doc->data, target->parent_colon + YAML_UNIT, header->text_end, + &comment) != 0 || + yaml_buf_append(out, doc->data, target->parent_colon + YAML_UNIT) != 0) { + return YAML_ERROR; + } + size_t kept_tail = header->text_end; + if (comment < header->text_end) { + if (yaml_buf_append_char(out, ' ') != 0) { + return YAML_ERROR; + } + kept_tail = comment; + } + if (yaml_buf_append(out, doc->data + kept_tail, header->end - kept_tail) != 0 || + yaml_buf_append(out, doc->data + header->end, target->insert_offset - header->end) != 0 || + (out->len > 0U && out->data[out->len - YAML_UNIT] != '\n' && + yaml_buf_append(out, doc->eol, doc->eol_len) != 0)) { + return YAML_ERROR; + } + return 0; +} + static int yaml_sequence_build_insert(yaml_buf_t *out, const yaml_doc_t *doc, const yaml_mapping_sequence_target_t *target, const char *const *sequence_path, const size_t *path_lengths, size_t sequence_path_len, const yaml_buf_t *rendered_item) { - if (target->insert_offset > doc->len || - yaml_buf_append(out, doc->data, target->insert_offset) != 0 || - yaml_append_separator_if_needed(out, doc, target->insert_offset) != 0) { + if (target->insert_offset > doc->len) { + return YAML_ERROR; + } + if (target->parent_inline_empty) { + if (yaml_sequence_append_expanded_parent(out, doc, target) != 0) { + return YAML_ERROR; + } + } else if (yaml_buf_append(out, doc->data, target->insert_offset) != 0 || + yaml_append_separator_if_needed(out, doc, target->insert_offset) != 0) { return YAML_ERROR; } if (!target->sequence_found) { diff --git a/tests/test_cli.c b/tests/test_cli.c index f716418ad..c5417b4e2 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -9449,6 +9449,143 @@ TEST(cli_hermes_stable_shell_context_contract) { PASS(); } +/* A fake HOME holding only ~/.hermes/config.yaml, so install and uninstall + * reach exactly the Hermes editors. */ +typedef struct { + char tmpdir[256]; + char config_path[640]; + char binary_path[640]; + char *saved_home; + char *saved_path; + char *saved_hermes; +} cli_hermes_cycle_t; + +static bool cli_hermes_cycle_open(cli_hermes_cycle_t *cycle, const char *initial_config) { + memset(cycle, 0, sizeof(*cycle)); + snprintf(cycle->tmpdir, sizeof(cycle->tmpdir), "/tmp/cli-hermes-cycle-XXXXXX"); + if (!cbm_mkdtemp(cycle->tmpdir)) { + cycle->tmpdir[0] = '\0'; + return false; + } + cycle->saved_home = save_test_env("HOME"); + cycle->saved_path = save_test_env("PATH"); + cycle->saved_hermes = save_test_env("HERMES_HOME"); + cbm_setenv("HOME", cycle->tmpdir, 1); + cbm_setenv("PATH", cycle->tmpdir, 1); + cbm_unsetenv("HERMES_HOME"); + + char hermes_dir[512]; + snprintf(hermes_dir, sizeof(hermes_dir), "%s/.hermes", cycle->tmpdir); + snprintf(cycle->config_path, sizeof(cycle->config_path), "%s/config.yaml", hermes_dir); +#ifdef _WIN32 + snprintf(cycle->binary_path, sizeof(cycle->binary_path), + "%s/.local/bin/codebase-memory-mcp.exe", cycle->tmpdir); +#else + snprintf(cycle->binary_path, sizeof(cycle->binary_path), "%s/.local/bin/codebase-memory-mcp", + cycle->tmpdir); +#endif + test_mkdirp(hermes_dir); + return write_test_file(cycle->config_path, initial_config) == 0; +} + +static void cli_hermes_cycle_close(cli_hermes_cycle_t *cycle) { + if (!cycle->tmpdir[0]) { + return; + } + restore_test_env("HOME", cycle->saved_home); + restore_test_env("PATH", cycle->saved_path); + restore_test_env("HERMES_HOME", cycle->saved_hermes); + test_rmdir_r(cycle->tmpdir); + cycle->tmpdir[0] = '\0'; +} + +/* Found while finishing #1862: when our hook was the only entry under + * `hooks:`, uninstall empties that mapping and the mapping editor writes it as + * `hooks: {}` (the same convention as `mcp_servers: {}`). The hook sequence + * editor then refused the file, so install -> uninstall -> install exited 1, + * and so did a repeated uninstall. The explicit empty mapping must be accepted: + * a repeated uninstall changes nothing and the reinstall restores the first + * install byte for byte, with exactly one owned hook. */ +TEST(cli_hermes_hook_reinstalls_after_uninstall_empties_hooks) { + cli_hermes_cycle_t cycle; + if (!cli_hermes_cycle_open(&cycle, "theme: solarized\n")) { + cli_hermes_cycle_close(&cycle); + FAIL("Hermes fake-HOME setup failed"); + } + char *argv[] = {"uninstall", "--yes"}; + int install_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *installed = read_test_file_alloc(cycle.config_path); + int uninstall_rc = cli_test_cmd_uninstall(2, argv); + char *emptied = read_test_file_alloc(cycle.config_path); + int repeat_rc = cli_test_cmd_uninstall(2, argv); + char *repeated = read_test_file_alloc(cycle.config_path); + int reinstall_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *reinstalled = read_test_file_alloc(cycle.config_path); + cli_hermes_cycle_close(&cycle); + + bool installed_once = install_rc == 0 && installed && + test_count_substring(installed, "pre_llm_call:") == 1U && + test_count_substring(installed, "hook-augment") == 1U; + bool emptied_clean = uninstall_rc == 0 && emptied && strstr(emptied, "theme: solarized\n") && + test_count_substring(emptied, "hooks: {}\n") == 1U && + !strstr(emptied, "pre_llm_call") && !strstr(emptied, "hook-augment"); + bool repeat_unchanged = repeat_rc == 0 && emptied && repeated && strcmp(repeated, emptied) == 0; + bool reinstalled_once = + reinstall_rc == 0 && installed && reinstalled && strcmp(reinstalled, installed) == 0; + free(installed); + free(emptied); + free(repeated); + free(reinstalled); + if (!installed_once) + FAIL("Hermes install must write exactly one owned pre_llm_call hook"); + if (!emptied_clean) + FAIL("uninstall must remove the owned hook and leave only the empty hooks: {} mapping"); + if (!repeat_unchanged) + FAIL("a repeated uninstall must accept hooks: {} and leave the file unchanged"); + if (!reinstalled_once) + FAIL("reinstall must accept hooks: {} and restore exactly one owned hook"); + PASS(); +} + +/* The same cycle with a user's own hook in the shared pre_llm_call sequence: + * the foreign hook survives untouched and uninstall restores the original + * file byte for byte. */ +TEST(cli_hermes_hook_cycle_keeps_foreign_pre_llm_hook) { + static const char original[] = "theme: solarized\n" + "mcp_servers: {}\n" + "hooks:\n" + " pre_llm_call:\n" + " - command: \"/usr/bin/user-hermes-hook\"\n"; + cli_hermes_cycle_t cycle; + if (!cli_hermes_cycle_open(&cycle, original)) { + cli_hermes_cycle_close(&cycle); + FAIL("Hermes fake-HOME setup failed"); + } + char *argv[] = {"uninstall", "--yes"}; + int install_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *installed = read_test_file_alloc(cycle.config_path); + int uninstall_rc = cli_test_cmd_uninstall(2, argv); + char *restored = read_test_file_alloc(cycle.config_path); + int reinstall_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *reinstalled = read_test_file_alloc(cycle.config_path); + cli_hermes_cycle_close(&cycle); + + bool merged = install_rc == 0 && installed && + test_count_substring(installed, "/usr/bin/user-hermes-hook") == 1U && + test_count_substring(installed, "pre_llm_call:") == 1U && + test_count_substring(installed, "hook-augment") == 1U; + bool restored_exactly = uninstall_rc == 0 && restored && strcmp(restored, original) == 0; + bool reinstalled_once = + reinstall_rc == 0 && installed && reinstalled && strcmp(reinstalled, installed) == 0; + free(installed); + free(restored); + free(reinstalled); + if (!merged || !restored_exactly || !reinstalled_once) + FAIL("a user's own pre_llm_call hook must survive install, uninstall and reinstall, and " + "uninstall must restore the original file byte for byte"); + PASS(); +} + #ifndef _WIN32 TEST(cli_detected_agent_summary_includes_registry_clients) { char tmpdir[256]; @@ -20351,6 +20488,8 @@ SUITE(cli) { RUN_TEST(cli_junie_current_durable_context_contract); RUN_TEST(cli_rovo_installs_documented_global_memory); RUN_TEST(cli_hermes_stable_shell_context_contract); + RUN_TEST(cli_hermes_hook_reinstalls_after_uninstall_empties_hooks); + RUN_TEST(cli_hermes_hook_cycle_keeps_foreign_pre_llm_hook); #ifndef _WIN32 RUN_TEST(cli_detected_agent_summary_includes_registry_clients); #endif diff --git a/tests/test_config_yaml_edit.c b/tests/test_config_yaml_edit.c index 9efaa2e38..95bc89404 100644 --- a/tests/test_config_yaml_edit.c +++ b/tests/test_config_yaml_edit.c @@ -1998,6 +1998,129 @@ TEST(config_yaml_edit_nested_sequence_removes_only_exact_canonical_item) { PASS(); } +/* Found while finishing #1862: cbm_yaml_remove_mapping_entry writes a section + * it empties as `section: {}`, and the Hermes remover empties `hooks:` whenever + * our hook was its only entry. The sequence editor refused that explicit empty + * parent mapping, so the next install (and a repeated removal) failed. The + * primitives' own round trip must land back on the first insert. */ +TEST(config_yaml_edit_nested_sequence_reinserts_under_emptied_parent) { + const char *initial = "theme: x\n"; + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, initial), 0); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *first = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(first); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + ASSERT_EQ(cbm_yaml_remove_mapping_entry(fixture.path, "hooks", "pre_llm_call"), 0); + char *emptied = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(emptied); + ASSERT_STR_EQ(emptied, "theme: x\nhooks: {}\n"); + free(emptied); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *again = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(again); + ASSERT_STR_EQ(again, first); + free(again); + free(first); + th_cleanup(fixture.dir); + PASS(); +} + +/* An exact empty flow mapping on a PARENT of the sequence path is an empty + * mapping, the same `section: {}` the mapping-entry editor writes and accepts. + * Removal is a byte-identical no-op; upsert expands the header in place, + * keeping its comment, the line endings and every other byte. */ +TEST(config_yaml_edit_nested_sequence_expands_explicit_empty_parent_mapping) { + static const char item_lf[] = " pre_llm_call:\n" + " - id: \"codebase-memory-mcp\"\n" + " type: \"command\"\n" + " command: \"/opt/Codebase Memory/bin/cbm\"\n"; + static const char item_crlf[] = " pre_llm_call:\r\n" + " - id: \"codebase-memory-mcp\"\r\n" + " type: \"command\"\r\n" + " command: \"/opt/Codebase Memory/bin/cbm\"\r\n"; + struct { + const char *initial; + const char *header; + const char *item; + const char *tail; + } cases[] = { + {"theme: x\nhooks: {} # keep me\nafter: 1\n", "theme: x\nhooks: # keep me\n", item_lf, + "after: 1\n"}, + {"theme: x\r\nhooks: {}\r\nafter: 1\r\n", "theme: x\r\nhooks:\r\n", item_crlf, + "after: 1\r\n"}, + {"theme: x\nhooks: {}", "theme: x\nhooks:\n", item_lf, ""}, + {"hooks: {}\n# trailing note\n", "hooks:\n# trailing note\n", item_lf, ""}, + }; + for (size_t i = 0U; i < sizeof(cases) / sizeof(cases[0]); i++) { + char expected[1024]; + ASSERT(snprintf(expected, sizeof(expected), "%s%s%s", cases[i].header, cases[i].item, + cases[i].tail) > 0); + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, cases[i].initial), 0); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *unchanged = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(unchanged); + ASSERT_STR_EQ(unchanged, cases[i].initial); + free(unchanged); + for (int pass = 0; pass < 2; pass++) { + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, + 2U, "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *expanded = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(expanded); + ASSERT_STR_EQ(expanded, expected); + free(expanded); + } + th_cleanup(fixture.dir); + } + PASS(); +} + +/* Only an exact `{}` on a parent with nothing nested beneath it is accepted. + * The sequence key itself holding a mapping, a non-empty flow mapping, a flow + * sequence where a mapping belongs, and content indented under an inline `{}` + * stay refused byte-identically by both upsert and removal. */ +TEST(config_yaml_edit_nested_sequence_still_refuses_non_empty_parent_flow) { + const char *cases[] = { + "hooks:\n pre_llm_call: {}\n", + "hooks: {pre_llm_call: []}\n", + "hooks: []\n", + "hooks: {}\n pre_llm_call:\n - id: \"codebase-memory-mcp\"\n", + }; + for (size_t i = 0U; i < sizeof(cases) / sizeof(cases[0]); i++) { + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, cases[i]), 0); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_ERROR); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_ERROR); + char *after = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(after); + ASSERT_STR_EQ(after, cases[i]); + free(after); + th_cleanup(fixture.dir); + } + PASS(); +} + /* Uninstall must be idempotent against a config that was never written. Hermes * is detected by its binary on PATH, so a HOME without `.hermes/` reached the * YAML removers with an absent target — and they failed acquiring the lock @@ -2245,6 +2368,9 @@ SUITE(config_yaml_edit) { RUN_TEST(config_yaml_edit_nested_sequence_preserves_crlf); RUN_TEST(config_yaml_edit_nested_sequence_foreign_identity_is_preserved); RUN_TEST(config_yaml_edit_nested_sequence_removes_only_exact_canonical_item); + RUN_TEST(config_yaml_edit_nested_sequence_reinserts_under_emptied_parent); + RUN_TEST(config_yaml_edit_nested_sequence_expands_explicit_empty_parent_mapping); + RUN_TEST(config_yaml_edit_nested_sequence_still_refuses_non_empty_parent_flow); RUN_TEST(config_yaml_edit_nested_sequence_ignores_foreign_flow_and_block_scalars_issue2209); RUN_TEST(config_yaml_edit_nested_sequence_ambiguity_fails_byte_identically); RUN_TEST(config_yaml_edit_remove_on_absent_config_is_a_no_op);