Feat/harness wave0

[mastwet]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces significant enhancements to the agent-diva runtime, including a new end-to-end testing framework (agent-diva-e2e), a four-state user presence state machine, structured runtime and audit logging, atomic file writes, configuration migration and hot-reloading, and a 3-layer prompt injection and PII redaction system. The review feedback highlights several critical issues: a cross-platform bug where atomic writes fail on Windows if the destination file exists; a security vulnerability in path traversal detection due to an incomplete nested URL-encoding blocklist; a regex bug in JSON secret redaction when handling escaped quotes; a startup crash in read-only environments during config migration; an inconsistent state vulnerability on hot-reload failures; and a redundant presence event storm on the message bus during inbound message processing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

On Windows, tempfile::NamedTempFile::persist will fail if the destination file already exists. Since atomic_write is used to repeatedly save session and configuration files, this will cause subsequent write operations to fail on Windows. To ensure cross-platform compatibility, the existing file should be removed first on Windows before persisting.

    #[cfg(windows)]\n    if path.exists() {\n        let _ = std::fs::remove_file(path);\n    }\n    temp_file.persist(path).map_err(|error| error.error)?;

[gemini-code-assist]

The contains_url_encoded_traversal function attempts to block nested URL-encoded path traversal by checking for specific hardcoded patterns (single, double, and triple encoding). This blocklist approach is incomplete and can be bypassed using higher levels of nested encoding (e.g., quadruple encoding %2525252f) or other encoding schemes like UTF-8 overlong encoding. A more robust security practice is to recursively decode the path until no percent-encodings remain, and then validate the fully decoded path.

[gemini-code-assist]

The JSON_FIELD_RE regex uses \"[^\"]*\" to match JSON string values. If a secret value contains an escaped quote (e.g., \"api_key\": \"some\\\"secret\"), this regex will prematurely stop matching at the escaped quote, leaving the rest of the secret unredacted in the logs and producing malformed JSON. To ensure complete redaction and maintain valid JSON structure, use a regex that correctly handles escaped characters.

static JSON_FIELD_RE: Lazy<Regex> = Lazy::new(|| {\n    Regex::new(\n        r#"(?ix)\n        (?P<key>"(?:api_key|token|secret|password|authorization)")\n        \s*:\s*\n        (?P<value>"(?:[^"\\\\]|\\\\.)*"|null)\n    "#,\n    )\n    .expect("valid json field regex")\n});

[gemini-code-assist]

If a configuration migration is triggered on startup, load() attempts to save the migrated configuration back to disk via self.save(&persisted_config). In read-only environments (such as Docker/Kubernetes with read-only ConfigMaps or restricted user permissions), this write operation will fail with a PermissionDenied or ReadOnlyFilesystem error, causing the entire application to crash on startup. Consider handling the save error gracefully (e.g., logging a warning) so that the application can still run using the successfully migrated in-memory configuration.

        if let Some(base_value) = persist_migrated_base {\n            let persisted_config: Config = serde_json::from_value(base_value)?;\n            if let Err(e) = validate_config(&persisted_config) {\n                tracing::error!("Migrated config failed validation: {}", e);\n            } else if let Err(e) = self.save(&persisted_config) {\n                tracing::warn!("Failed to persist migrated config to disk: {}", e);\n            }\n        }

[gemini-code-assist]

In the hot-reload loop, if any module fails to reload (returning an Err), all_succeeded is set to false, and the global current_config is not updated. However, the modules that were processed before the failure have already applied the new configuration, and there is no rollback mechanism to revert them. This leaves the system in an inconsistent state where some modules are running with the new configuration while others are running with the old one, but the global state still reports the old configuration. Consider implementing a two-phase commit or at least logging a critical warning about the inconsistent state.

[gemini-code-assist]

Calling refresh_presence() immediately followed by note_user_activity() when publishing an inbound message causes a redundant double-transition event storm. For example, if the user has been inactive, refresh_presence() will transition the state from Active to Distracted (emitting an event), and then note_user_activity() will immediately transition it back to Active (emitting another event). These two transitions happen within microseconds, creating unnecessary noise on the message bus. Consider checking the elapsed state silently before resetting, or combining these operations.