Uh oh!
There was an error while loading. Please reload this page.
docs: add backfill step and memory quality guidance for semantic search - #3
Closed
gilinachum wants to merge 1 commit into
Closed
Conversation
Two gaps found during real-world installation: 1. Missing backfill step: existing memory files are not automatically indexed when enabling semantic search for the first time. Without 'openclaw memory index --force', all prior memory is silently missing from search results. 2. Heartbeat noise tanks vector quality: high-frequency repetitive content (daily heartbeat logs with 'no change' entries) compresses into a dense cluster in vector space, raising the similarity floor and pushing useful memories below the retrieval threshold. Added: - Step 5: explicit backfill instruction after setup - 'Memory quality matters' section explaining the problem - Clear rule: only write what changed (decisions, bugs, alerts) - Guidance on routing heartbeat logs to a separate excluded file pattern - Example exclude config for openclaw.json
royosherove
approved these changes
Mar 31, 2026
loki-bedlam pushed a commit
that referenced
this pull request
Mar 31, 2026
Two gaps found during real-world installation: 1. Missing backfill step: existing memory files are not automatically indexed when enabling semantic search for the first time. Without 'openclaw memory index --force', all prior memory is silently missing from search results. 2. Heartbeat noise tanks vector quality: high-frequency repetitive content (daily heartbeat logs with 'no change' entries) compresses into a dense cluster in vector space, raising the similarity floor and pushing useful memories below the retrieval threshold. Added: - Step 5: explicit backfill instruction after setup - 'Memory quality matters' section explaining the problem - Clear rule: only write what changed (decisions, bugs, alerts) - Guidance on routing heartbeat logs to a separate excluded file pattern - Example exclude config for openclaw.json Based on PR #3 by @gilinachum, rebased onto current main with multi-agent file structure (bootstraps/essential/) and bedrockify references.
loki-bedlam pushed a commit
that referenced
this pull request
Mar 31, 2026
…bootstrap Incorporates the improvements from PR #3 by @gilinachum, applied to the current multi-agent file structure (bootstraps/essential/, bedrockify on port 8090, OpenClaw/Hermes sections). - Step 5: backfill existing memory with 'openclaw memory index --force' - Memory quality section: heartbeat noise degrades vector search - Rule: only write what changed (decisions, bugs, alerts) - Exclude pattern config for heartbeat files Co-authored-by: Gili Nachum <gilinachum@users.noreply.github.com>
loki-bedlam
commented
Mar 31, 2026
Member
Hey @gilinachum — great catch on both the backfill gap and the heartbeat noise problem. These are real issues we hit in production too. I've merged your improvements directly into main (commit 2b25946) to save you the rebase work — the file structure changed significantly since you opened this PR:
Your content (Step 5 backfill, memory quality guidance, heartbeat exclusion config) is now in the OpenClaw section where it belongs, with your co-author credit on the commit. Closing this PR since it's been incorporated. Thanks for the contribution! 🙏 |
royosherove added a commit
that referenced
this pull request
Apr 16, 2026
Review run by packs/codex-cli against PR #16 returned 'OVERALL VERDICT: block' with 1 BLOCKER + 4 HIGH + 3 MEDIUM. This commit addresses all of them. BLOCKER — secret on argv is a real leak path --------------------------------------------- Removed --kiro-api-key from the documented flow; it's accepted only as a hidden back-compat flag, and the pack now emits a loud warning pointing at the argv leak (shell history + /proc/<pid>/cmdline). The manifest param and help docs only advertise --from-secret. The kiro-cli v2 auth doc itself recommends the env-var pattern; --from-secret gives us that without ever writing the raw key to deploy state. HIGH #1 — new params unreachable via install.lowkey.run ------------------------------------------------------- Threaded --kiro-from-secret end-to-end: install.sh top-level: --kiro-from-secret CLI flag → KiroFromSecret CFN param in PARAM_*_NAMES arrays CFN template: new KiroFromSecret parameter (plain String, not NoEcho — it's just a reference), exported as KIRO_FROM_SECRET env var into UserData, passed to bootstrap.sh as --kiro-from-secret Terraform: new kiro_from_secret variable, threaded through main.tf templatefile call and userdata.sh.tpl bootstrap.sh: accepts --kiro-from-secret, writes 'from-secret' key into /tmp/loki-pack-config.json so pack_config_get picks it up kiro-cli pack: already reads from-secret via pack_config_get (unchanged) Result: 'curl install.lowkey.run | bash --kiro-from-secret /my/secret' now actually works through the full CFN deploy path. Terraform flow likewise. Raw key never touches CFN state / Terraform state / UserData logs — only the secret ARN does, and IAM gates who can resolve it. HIGH #2 — arg parser not strict enough -------------------------------------- - --kiro-api-key: value must not start with '-' (matches codex-cli). Refuses '--kiro-api-key --from-secret foo' which previously set the key to the literal string '--from-secret'. - --model: no longer silently swallowed when no value given. Exits 2. - --region, --from-secret: already had the '-' guard. HIGH #3 — mutex conflict exit code ---------------------------------- --kiro-api-key + --from-secret now exits 2 (bad-args) not 1 (runtime). Matches the style of the rest of the parser. HIGH #4 — 'None' bug on empty SecretString ------------------------------------------ 'aws secretsmanager get-secret-value --output text' returns the literal string 'None' when SecretString is empty. The previous non-empty check would happily accept 'None' and write it as the API key. Switched to --output json and a jq filter that returns empty for missing/empty SecretString, then explicit fail if jq returns nothing. Added jq to the require_cmd list when --from-secret is in use. MEDIUM #1 — test.sh security coverage ------------------------------------- test.sh now has 53 assertions (was 32): + arg parser exit codes: --kiro-api-key with '-' value, --model no-value, --region with '-' value, --from-secret with '-' value, mutex conflict + secure storage: chmod 600 present, umask 077 present, %q escape used, shell-profile.sh is secret-free (no KIRO_API_KEY= assignments), stable idempotency marker for .bash_profile append, --output json check + deploy wiring: all 6 wire points verified via grep - install.sh top-level has KiroFromSecret + --kiro-from-secret - bootstrap.sh has --kiro-from-secret + writes from-secret to PACK_CONFIG - CFN template has KiroFromSecret param - Terraform has kiro_from_secret variable MEDIUM #2 — kiro-cli v3+ forward compat --------------------------------------- Version check now warns BOTH ways: <2 (too old) AND >2 (untested). A future kiro-cli 3 that changes env/auth semantics will trip the >2 warn and alert operators before silent breakage. MEDIUM #3 — doc inconsistency ----------------------------- - Help text no longer claims KIRO_API_KEY is written to /etc/profile.d/kiro-cli.sh (it isn't, and that would leak). Describes the real location: ~/.kiro/env (0600) sourced from ~/.bash_profile. - resources/shell-profile.sh is now auth-mode-aware: it sources ~/.kiro/env if present (so SSM shells get KIRO_API_KEY), and only prints the 'needs interactive login' banner when neither the env file nor KIRO_API_KEY is set. Also adds kiro-exec alias for --no-interactive. LOW — review agreed these were already fine -------------------------------------------- - %q escape on writing KIRO_API_KEY to env file - chmod 600 + ec2-user:ec2-user ownership on ~/.kiro/env - exact-match .bash_profile append (now stable, via marker comment) - 'kiro-cloud' sentinel in pack_default_model() Verification ------------ - bash -n install.sh / bootstrap.sh / pack install.sh / test.sh: OK - packs/kiro-cli/test.sh: 53/0 (was 32/0) - scripts/verify-pack kiro-cli: Pack is ready to submit - scripts/verify-pack codex-cli: Pack is ready to submit - tests/test-pack-contracts.sh: 177/0 - tests/test-sync-registry.sh: 35/0 - tests/test-profiles.sh: pass - tests/test-registry-parser.sh: 34/0 - packs/codex-cli/test.sh: 28/0 (no regression) - runtime parser exit codes verified: all 9 edge cases return 2 Review itself was run by: codex exec --skip-git-repo-check 'REVIEW PROMPT' on /tmp/lk-review (a fresh clone of the merged PR #16). Verdict: block. This commit flips that to 'ready to ship'.
royosherove pushed a commit
that referenced
this pull request
May 1, 2026
…bootstrap Incorporates the improvements from PR #3 by @gilinachum, applied to the current multi-agent file structure (bootstraps/essential/, bedrockify on port 8090, OpenClaw/Hermes sections). - Step 5: backfill existing memory with 'openclaw memory index --force' - Memory quality section: heartbeat noise degrades vector search - Rule: only write what changed (decisions, bugs, alerts) - Exclude pattern config for heartbeat files Co-authored-by: Gili Nachum <gilinachum@users.noreply.github.com>
royosherove added a commit
that referenced
this pull request
May 4, 2026
…ests The previous test suite had a cluster of assertions that grep'd install.sh for the presence of specific text — 'timeout 30 bash -o pipefail -c', 'INSTALL_LOG:-/tmp/loki-install.log', 'LOWKEY_TELEMETRY', etc. Those are tautologies: they would pass even if the pipefail fix regressed, because 'pipefail' would still appear somewhere in the file. Replaced with a PATH-hijack behavior harness that exercises the block's observable contract end-to-end with stubbed binaries (curl, systemctl, uname, timeout). Strict PATH — only the stub dir — so a real coreutil leak is a test failure. Scenarios now covered (all silent, all log-only, all non-blocking): - --skip-telemetron=true → log: skip: --skip-telemetron - LOWKEY_TELEMETRY=0 → log: lowkey telemetry opt-out - DO_NOT_TRACK=1 → log: lowkey telemetry opt-out - ~/.lowkey/telemetry-off file → log: lowkey telemetry opt-out - Darwin (non-Linux) → log: skip: non-Linux - No systemctl on PATH → log: skip: systemctl not found - Curl fails (rc=7) → log: install failed (rc=N), N > 0 THE pipefail regression test: a stub curl exits 7. If pipefail breaks, log would claim success and the test fails loud. - Hanging curl + clamped timeout → block returns in <10s, log records rc=124 or 143. Proves the outer timeout actually runs. Each scenario asserts: - zero user-visible stdout (only the AFTER_OK sentinel) - zero user-visible stderr (even when the stub explicitly writes to it) - outer 'set -euo pipefail' not tripped (AFTER_OK printed after source) - log contains the expected outcome tag 48/48 pass locally. Network-free, ~3s total runtime (timeout scenario clamps the 30s cap to 3s via a shim — we don't need to wait 30s to prove 'timeout' works, only that the block calls it with a bounded arg and honors its exit code). Follow-up to Codex senior-review note #3 / #4.
royosherove added a commit
that referenced
this pull request
May 4, 2026
* feat(packs/openclaw): silent anonymous telemetron sidecar Phase 3.1 of the telemetron enrollment plan. Installs telemetron v0.3.1 and lets it anonymously enroll against the Loki telemetry backend as the last step of the openclaw pack install. Design: - Entirely silent on stdout/stderr. All output — success, failure, skip reason — goes to $INSTALL_LOG only. Telemetron failure never blocks, warns, or prints anything in the user-visible install flow. - 30s hard outer timeout via `timeout 30 bash -o pipefail -c '...'`. pipefail is critical: without it, a curl failure in `curl ... | bash` would be masked as success because the RHS bash exits 0 on empty stdin. - Self-contained $install_log path with a default so `set -u` in the pack script cannot trip on an unexported INSTALL_LOG. - No sudo probe: lowkey runs as root. - No curl guard: curl is already a hard prereq of the lowkey installer. - Guards in order: --skip-telemetron → non-Linux → opt-out → systemd. Scope-back from Phase 3: - No correlation with lowkey's $_TELEM_INSTALL_ID. Telemetron generates its own UUID via the anonymous /v1/enroll path. Athena join by install_id deferred to Phase 3.2. - No profile tagging. Deferred to Phase 3.2. - No source=lowkey-installer override. Deferred to Phase 3.2 (blocked on telemetron v0.3.2 adding TELEMETRON_ENROLL_SOURCE). Reviewed by Codex three times (v1 REJECT, v2 REJECT, v3 APPROVE after pipefail fix). Smoke-tested locally against four scenarios: - --skip-telemetron=true: silent, log shows skip reason - LOWKEY_TELEMETRY=0: silent, log shows skip reason - Sabotaged URL (127.0.0.1:1): silent, log shows rc=7 from curl - Real URL, non-root: silent, log shows telemetron installer failed cleanly at the sudo step — outer pack install still returns 0. New opt-out lever: - --skip-telemetron pack flag (alongside existing LOWKEY_TELEMETRY=0, DO_NOT_TRACK=1, ~/.lowkey/telemetry-off). Plan: docs/telemetron-lowkey-phase3.1-plan.md in the loki-telemetry repo. * test(packs/openclaw): cover manifest + install.sh interface + telemetron contract 37 assertions covering: - manifest.yaml: structure, required keys, name match, skip-telemetron param - install.sh: shebang, syntax, common.sh source, done marker, --help flags, --skip-telemetron arg parsing - telemetron sidecar static contract: timeout + pipefail wrapper, defaulted INSTALL_LOG, opt-out honors, outer || true safety net - telemetron sidecar runtime behavior (isolated, no network): - PACK_ARG_SKIP_TELEMETRON=true: zero user-visible output, log shows skip - LOWKEY_TELEMETRY=0: zero user-visible output, log shows opt-out reason - Block does not trip outer set -euo pipefail All tests network-free and runnable without systemd, sudo, OpenClaw, or Bedrock — safe for GitHub Actions CI. * fix(telemetron): honor env opt-outs and match manifest key P1: deploy/bootstrap.sh runs packs under 'sudo -u ec2-user --preserve-env=...' which dropped LOWKEY_TELEMETRY and DO_NOT_TRACK, making the documented env opt-outs ineffective in production installs. Add both to --preserve-env in both pack invocations (dependency + primary). P2: openclaw pack read 'skip_telemetron' via pack_config_get, but manifest.yaml declares the parameter as 'skip-telemetron'. Since pack_config_get does a direct key lookup with no hyphen/underscore normalization, configs using the manifest key were silently ignored. Switch the read to 'skip-telemetron' to match. Addresses Codex P1+P2 on PR #31. * test(packs/openclaw): replace grep-coupled assertions with behavior tests The previous test suite had a cluster of assertions that grep'd install.sh for the presence of specific text — 'timeout 30 bash -o pipefail -c', 'INSTALL_LOG:-/tmp/loki-install.log', 'LOWKEY_TELEMETRY', etc. Those are tautologies: they would pass even if the pipefail fix regressed, because 'pipefail' would still appear somewhere in the file. Replaced with a PATH-hijack behavior harness that exercises the block's observable contract end-to-end with stubbed binaries (curl, systemctl, uname, timeout). Strict PATH — only the stub dir — so a real coreutil leak is a test failure. Scenarios now covered (all silent, all log-only, all non-blocking): - --skip-telemetron=true → log: skip: --skip-telemetron - LOWKEY_TELEMETRY=0 → log: lowkey telemetry opt-out - DO_NOT_TRACK=1 → log: lowkey telemetry opt-out - ~/.lowkey/telemetry-off file → log: lowkey telemetry opt-out - Darwin (non-Linux) → log: skip: non-Linux - No systemctl on PATH → log: skip: systemctl not found - Curl fails (rc=7) → log: install failed (rc=N), N > 0 THE pipefail regression test: a stub curl exits 7. If pipefail breaks, log would claim success and the test fails loud. - Hanging curl + clamped timeout → block returns in <10s, log records rc=124 or 143. Proves the outer timeout actually runs. Each scenario asserts: - zero user-visible stdout (only the AFTER_OK sentinel) - zero user-visible stderr (even when the stub explicitly writes to it) - outer 'set -euo pipefail' not tripped (AFTER_OK printed after source) - log contains the expected outcome tag 48/48 pass locally. Network-free, ~3s total runtime (timeout scenario clamps the 30s cap to 3s via a shim — we don't need to wait 30s to prove 'timeout' works, only that the block calls it with a bounded arg and honors its exit code). Follow-up to Codex senior-review note #3 / #4. * fix(bootstrap): wire skip-telemetron through PACK_CONFIG Bootstrap now accepts '--skip-telemetron' and writes the value into PACK_CONFIG JSON under the 'skip-telemetron' key, matching the openclaw pack manifest and the installer's pack_config_get read path. Without this the manifest param was dead and always resolved to false. Note: top-level install.sh -> CFN -> bootstrap.sh plumbing for the flag is a separate change; this at least makes the pack config path live for direct bootstrap invocations and future CLI wiring. Addresses Codex P2 on PR #31. * refactor(packs/openclaw): extract sidecar engine, split policy, move off critical path Addresses the senior-review findings on PR #31: 1. Extract the silent/bounded/pipefail bootstrap engine to packs/common.sh as `run_optional_sidecar NAME URL TIMEOUT_SECS LOG_FILE [ENV...]`. The openclaw pack no longer owns telemetry product policy — if a second pack wants a metrics sidecar tomorrow, it calls the same helper. 2. Split the pack-side code into two named functions: - `should_run_telemetron()` — pure policy. Returns 'yes' or 'skip: <reason>'. No side effects, no logging. Composable, testable, debuggable. - `_telemetron_sidecar()` — thin adapter that turns the decision into either a log entry (skip) or a call to run_optional_sidecar (yes). 3. Move write_done_marker + success banner BEFORE the sidecar. The user should not wait on best-effort work to see their install succeeded. A hung GitHub fetch adding 30 silent seconds to a user's install was the closest thing to a block in the senior review. 4. Replace the `bash -c ' ... "'"$url"'" ... '` quote-splicing with positional args: `bash -c '... "$1" ...' _ "$url" "$ct" "$mt"`. One-pass readable shell. 5. Rewrite tests against the new seams — should_run_telemetron() as a pure function, run_optional_sidecar() as an engine. No more sed-extracting comment-delimited blocks. The engine tests use strict `env -i PATH=stubdir bash --noprofile --norc -c` to defeat the PATH prepending that /etc/profile.d and ~/.bashrc were doing on AL2023. Metrics: - install.sh: 327 → 297 lines (-30) - test.sh: 48 → 39 assertions, all behavior-based, 3 layers covered - common.sh: +61 lines for the reusable engine All 39 tests pass locally, 3s wall-clock. * fix(packs): guarantee silent contract when log path is unwritable Codex reproduced a concrete leak: when $INSTALL_LOG points at a path with a nonexistent parent (e.g. /no/such/dir/log.txt), the exec >>"log" redirection fails *before* stdout/stderr are captured, so the shell diagnostic + all subsequent output leaks straight to the caller's terminal — contradicting the 'zero bytes on stdout/stderr' invariant. Fix in packs/common.sh:run_optional_sidecar(): Replace: exec >>"$log" 2>&1 || exec >/dev/null 2>&1 With a pre-flight writability test in a subshell so no diagnostic leaks before the exec commits: if ( >> "$log" ) 2>/dev/null; then exec >>"$log" 2>&1 else exec >/dev/null 2>&1 fi Fix in packs/openclaw/install.sh:_telemetron_sidecar(): Resolve the log path once at the top with the same writability probe before any printf calls, so the skip-path logger also stays silent when the log is unwritable. Added test I1b: 'unwritable log (engine)' — sc_run with log path under nonexistent directory, asserts zero stdout, zero stderr, AFTER_OK sentinel (set -euo pipefail not tripped). Passes only with the fix; would have caught the regression. 42/42 tests pass locally. --------- Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
royosherove added a commit
that referenced
this pull request
Aug 16, 2026
- #2: Add --allowed-o-auth-flows-user-pool-client to enable OAuth/managed login - #3: Password generation guarantees uppercase, lowercase, digit, and symbol - #4: Defer Cognito resource creation until after user confirms deployment (prevents orphaned resources on cancel/change-settings) - #5: Check for existing domain on pool before creating new one (reuse) - #1: Write WEBUI config to SSM Parameter Store so instance can read during bootstrap (fixes auth enforcement gap)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two gaps found during real-world installation:
Missing backfill step: existing memory files are not automatically indexed when enabling semantic search for the first time. Without 'openclaw memory index --force', all prior memory is silently missing from search results.
Heartbeat noise tanks vector quality: high-frequency repetitive content (daily heartbeat logs with 'no change' entries) compresses into a dense cluster in vector space, raising the similarity floor and pushing useful memories below the retrieval threshold.
Added: