Skip to content

ci: extend Windows secrets ACL timeout#2654

Open
YOMXXX wants to merge 3 commits into
tinyhumansai:mainfrom
YOMXXX:fix/windows-secrets-acl-timeout
Open

ci: extend Windows secrets ACL timeout#2654
YOMXXX wants to merge 3 commits into
tinyhumansai:mainfrom
YOMXXX:fix/windows-secrets-acl-timeout

Conversation

@YOMXXX
Copy link
Copy Markdown
Contributor

@YOMXXX YOMXXX commented May 26, 2026

Summary

  • Raise the reusable Windows secrets ACL job timeout from 20 to 30 minutes.
  • Stabilize the tool-memory unit test helper by replacing random UUID-like tool names with deterministic non-PII names.
  • Derive vault memory namespaces from an alphabet-only digest instead of embedding raw UUIDs, preventing random UUID substrings from tripping the memory PII namespace/key guard during vault sync.
  • Add vault namespace safety coverage and improve vault sync E2E assertion diagnostics.

Problem

#2551 proved the Windows-specific secrets test step passed, but the job was cancelled during Post Cache Rust build artifacts after the current 20-minute job timeout. While validating this CI fix, Rust coverage also exposed two existing flaky memory/vault tests: random UUID-derived identifiers can occasionally resemble strict personal-identifier formats and get rejected by the memory safety guard.

Solution

Only the Windows secrets ACL job timeout is increased, keeping the rest of the reusable workflow unchanged. Test helpers now avoid random PII-like names, and vault creation now stores documents under a stable alphabet-only namespace derived from the vault id instead of the raw UUID.

Testing

  • git diff --check
  • cargo fmt --all --check
  • ruby -e 'require "yaml"; YAML.load_file(".github/workflows/test-reusable.yml"); puts "yaml ok"'\n- [x] GGML_NATIVE=OFF cargo test -p openhuman tool_rules_for_prompt_sorts_by_priority_and_tool_name -- --nocapture\n- [x] GGML_NATIVE=OFF cargo test -p openhuman memory_namespace -- --nocapture\n- [x] GGML_NATIVE=OFF cargo test -p openhuman vault_namespace_derivation -- --nocapture\n- [x] GGML_NATIVE=OFF cargo test -p openhuman --test vault_sync_e2e -- --nocapture\n\nBlocked local checks:\n- [ ] pnpm exec prettier --check .github/workflows/test-reusable.yml — local root workspace does not provide prettier on pnpm exec (Command "prettier" not found).\n- [ ] pnpm --filter openhuman-app exec prettier --check ../.github/workflows/test-reusable.yml — local app workspace reports Node v22.14.0 below its required >=24, then Command "prettier" not found.\n\nNotes:\n- Initial local cargo test -p openhuman tool_rules_for_prompt_sorts_by_priority_and_tool_name -- --nocapture without GGML_NATIVE=OFF hit the documented macOS Tahoe / Apple Silicon whisper-rs -mcpu=native issue; rerun with GGML_NATIVE=OFF passed.\n- Latest pushed commit: 4af6666a.\n

Summary by CodeRabbit

  • Chores

    • Vault memory namespaces are now generated in a PII-safe way (no raw IDs embedded).
    • CI Windows security test timeout increased for improved reliability.
  • Tests

    • Added tests ensuring vault namespaces don’t expose IDs or PII.
    • Made tool-related memory tests more deterministic.
  • Documentation

    • Clarified vault ingestion/namespace wording to reflect vault-derived namespaces.

Review Change Stack

@YOMXXX YOMXXX requested a review from a team May 26, 2026 00:10
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cf132e21-fb5e-4cf2-ba00-657fb89638ee

📥 Commits

Reviewing files that changed from the base of the PR and between 4af6666 and 137aab2.

📒 Files selected for processing (2)
  • .github/workflows/test-reusable.yml
  • src/openhuman/memory/ops/tool_memory.rs
💤 Files with no reviewable changes (1)
  • src/openhuman/memory/ops/tool_memory.rs

📝 Walkthrough

Walkthrough

Increases the Windows test job timeout from 20 to 30 minutes, replaces a UUID-based test helper with a static atomic counter for deterministic tool names, and implements alphabet-only SHA-256-derived vault namespaces with docs and tests verifying they don't embed PII/secret material.

Changes

Windows Test Job Timeout

Layer / File(s) Summary
Windows test job timeout increase
.github/workflows/test-reusable.yml
The rust-core-tests-windows job timeout-minutes was increased from 20 to 30 minutes.

Tool-memory test helper

Layer / File(s) Summary
Atomic counter-based unique tool name
src/openhuman/memory/ops/tool_memory.rs
Added AtomicUsize and Ordering imports and replaced a truncated-UUID approach with a static AtomicUsize counter in unique_tool_name() to produce deterministic, incrementing test tool names.

Vault namespace derivation and tests

Layer / File(s) Summary
Namespace derivation implementation
src/openhuman/vault/ops.rs
Adds sha2::{Digest, Sha256} and pub(crate) fn vault_namespace_for_id(id: &str) -> String that computes a SHA-256 digest and converts it into a 24-character a-z suffix, returned as vault-{suffix}.
Usage and doc updates
src/openhuman/vault/ops.rs, src/openhuman/vault/mod.rs, src/openhuman/vault/sync.rs
vault_create now sets Vault.namespace via vault_namespace_for_id(&id). Module- and field-level doc comments were reworded to describe a vault-derived memory namespace.
Namespace safety tests and e2e assertions
src/openhuman/vault/tests.rs, tests/vault_sync_e2e.rs
Adds unit tests asserting vault--prefixed namespaces do not embed ids and are not flagged as likely secret/PII, and expands e2e sync assertions to include diagnostic error output (first.errors, second.errors).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

memory, rust-core, working

Suggested reviewers

  • graycyrus
  • M3gA-Mind

Poem

🐰 I hopped through tests with coffee hot,
Tick the counter — numbers plot.
Vaults get hashed, letters bloom,
No secrets shown, no PII in room.
Runners wait — timeouts grow a spot.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ci: extend Windows secrets ACL timeout' is partially related to the changeset—it accurately describes the main CI workflow change, but the PR also includes significant code changes to vault namespace derivation, tool naming determinism, and test coverage that are not reflected in the title.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 26, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 26, 2026
@coderabbitai coderabbitai Bot added rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. working A PR that is being worked on by the team. labels May 26, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 26, 2026
@YOMXXX
Copy link
Copy Markdown
Contributor Author

YOMXXX commented May 26, 2026

Latest head 4af6666a is green now. Windows secrets ACL passed under the new 30-minute timeout, Rust Core Coverage/Coverage Gate passed after the vault namespace stabilization, Linux Appium, Rust/TS/Tauri, Docker core smoke, install smoke, and CodeRabbit are all passing.

No unresolved review threads remain from my latest check. @graycyrus @M3gA-Mind this should be ready for review/merge; once it lands I can sync #2551 to clear the same Windows cancellation there.

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walkthrough: CI timeout bump (20→30 min) for Windows secrets ACL job, plus a fix for vault memory namespaces that could trip the PII safety guard when raw UUIDs resembled personal identifiers. Test helpers now use deterministic names instead of random UUIDs, and two new tests cover the namespace derivation. E2E assertion diagnostics improved.

Area Files Verdict
CI test-reusable.yml Timeout bump is justified by #2551 evidence
Rust core (memory) tool_memory.rs AtomicUsize counter — clean, no ordering issues
Rust core (vault) ops.rs, mod.rs, sync.rs SHA256→alphabet digest is sound; no migration needed since namespace is set at creation time
Tests tests.rs, vault_sync_e2e.rs Good coverage of the new behavior, better assertion messages

Solid work — the PII guard bypass was a real flakiness source and this is the right fix. No existing vaults are affected since the namespace is persisted at creation time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants