fix: remaining review findings — path traversal, dead field, env caching - #126
Merged
Conversation
- simard_gym_bridge.py: reject scenario_id containing '/', '\', or '..' before using it in os.path.join (path traversal guard) - remote_azlin.rs: remove unused ssh_key_path field from AzlinConfig - identity_composition.rs: cache max_subordinate_depth() with OnceLock instead of reading env on every call; clamp to absolute max of 10 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rysweet
added a commit
that referenced
this pull request
Jul 8, 2026
…min to seconds (#40) The OODA daemon's per-cycle prepare-context step (`preparation_memory_operations_with_active_slugs_phased`) spun for ~11 min at ~300% CPU every cycle with no child processes and no log output — the substance behind the operator's "the memory graph never loads" complaint. The graph loads; the read was pathologically slow, so each cycle took ~15–18 min. Root cause was engine-side: ranked declarative/episodic recall computed the graph-proximity term with a per-node `query_neighbors` BFS — an N+1 fan-out of ~3N lock-serialized Cypher scans per recall (~23k queries at ~7,590 facts). Fixed in amplihack-memory-lib PR #126 (bulk graph-adjacency index: one bulk edge scan per type, byte-identical ranking). This PR consumes that fix: - Bump the pinned `amplihack-memory` git rev to the PR #126 squash-merge commit (72c5ea1) and rebuild `Cargo.lock`. Pure performance refactor upstream; no store-format change (still v41), same relevant facts/triggers/procedures/ episodes surface with recency/phase weighting intact. - Add per-sub-step `tracing` spans (counts/timings only — never memory content) around the four prepare-context sub-ops (ranked declarative recall, check_triggers, procedure recall, ranked episodic recall) plus the whole call, so per-sub-step wall-clock is attributable from a live log without a profiler. The engine emits `graph_path` (indexed|legacy); Simard emits the timings. - Preserve the operator-facing "Prepared context: N facts, …" working-memory summary verbatim. Regression guard: `memory_consolidation::tests_prepare_context_perf` pins the orchestration invariant that prepare-context issues a small, CONSTANT number of expensive recall calls — O(objective fragments), never O(fact-store size) — via deterministic call-count assertions (no wall-clock timeout), and that the "Prepared context: …" summary keeps reaching working memory. This fails the moment a per-fact fan-out is reintroduced at the orchestration layer, one layer above the engine fix. Engine profiling (persistent/lbug, 1,500 facts, 1 hop): legacy 3,000 per-node Cypher scans / 14,688 ms -> indexed 0 per-node scans, 2 bulk scans / 286 ms (51.3x); round-trips now constant in fact count. Docs: add the bulk graph-adjacency index reference and mark the ranked-recall graph signal as shipped. Closes #40 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
added a commit
that referenced
this pull request
Jul 8, 2026
…min to seconds (#40) (#3046) The OODA daemon's per-cycle prepare-context step (`preparation_memory_operations_with_active_slugs_phased`) spun for ~11 min at ~300% CPU every cycle with no child processes and no log output — the substance behind the operator's "the memory graph never loads" complaint. The graph loads; the read was pathologically slow, so each cycle took ~15–18 min. Root cause was engine-side: ranked declarative/episodic recall computed the graph-proximity term with a per-node `query_neighbors` BFS — an N+1 fan-out of ~3N lock-serialized Cypher scans per recall (~23k queries at ~7,590 facts). Fixed in amplihack-memory-lib PR #126 (bulk graph-adjacency index: one bulk edge scan per type, byte-identical ranking). This PR consumes that fix: - Bump the pinned `amplihack-memory` git rev to the PR #126 squash-merge commit (72c5ea1) and rebuild `Cargo.lock`. Pure performance refactor upstream; no store-format change (still v41), same relevant facts/triggers/procedures/ episodes surface with recency/phase weighting intact. - Add per-sub-step `tracing` spans (counts/timings only — never memory content) around the four prepare-context sub-ops (ranked declarative recall, check_triggers, procedure recall, ranked episodic recall) plus the whole call, so per-sub-step wall-clock is attributable from a live log without a profiler. The engine emits `graph_path` (indexed|legacy); Simard emits the timings. - Preserve the operator-facing "Prepared context: N facts, …" working-memory summary verbatim. Regression guard: `memory_consolidation::tests_prepare_context_perf` pins the orchestration invariant that prepare-context issues a small, CONSTANT number of expensive recall calls — O(objective fragments), never O(fact-store size) — via deterministic call-count assertions (no wall-clock timeout), and that the "Prepared context: …" summary keeps reaching working memory. This fails the moment a per-fact fan-out is reintroduced at the orchestration layer, one layer above the engine fix. Engine profiling (persistent/lbug, 1,500 facts, 1 hop): legacy 3,000 per-node Cypher scans / 14,688 ms -> indexed 0 per-node scans, 2 bulk scans / 286 ms (51.3x); round-trips now constant in fact count. Docs: add the bulk graph-adjacency index reference and mark the ranked-recall graph signal as shipped. Closes #40 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
added a commit
that referenced
this pull request
Jul 8, 2026
…ork git dep The pin-bump broke three guards in issue_2626_amplihack_pin_bump.rs: - MEMORY_TARGET_REV was the old 72c5ea1b (#126); bump it to 5807d056 (#129, the lbug portability fix) and move MEMORY_STALE_REV to 72c5ea1b as the new anti-regression sentinel. - direct_lbug_pin_matches_the_single_locked_version assumed lbug is a crates.io `= "=X"` version pin. lbug is now a git dep on the rysweet/ladybug-rust fork, so the guard now asserts the direct lbug is a git dep on the fork remote at the expected full-SHA rev AND that Cargo.lock resolves exactly one lbug from that fork rev (single engine / one store format — the invariant is unchanged). All 9 guards pass locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
added a commit
that referenced
this pull request
Jul 8, 2026
…ug portability fix #3119) (#3171) * chore(deps): bump amplihack-memory to #129 + repoint lbug to the fork (lbug portability fix, #3119) Consumes amplihack-memory-lib #129 (rev 5807d056): the lbug portability fix. Published crates.io lbug 0.17.1 SIGSEGVs in std::vformat during Database::initBufferManager on hosts whose libstdc++ std::format ABI differs from the prebuilt (Ubuntu 26.04), and its build.rs double-links the bundled third-party archives in source mode (duplicate symbols). The fix pins lbug to the rysweet/ladybug-rust fork (link_bundled_deps=false, links only the self-contained liblbug.a): from-source cures the ABI SIGSEGV and links cleanly with no unsafe --allow-multiple-definition. Simard's direct `lbug` dep (used by simard-tui's read-only goal-board reader) is repointed at the SAME fork rev so cargo unifies to exactly one lbug crate / one engine / one storage format. Verified: Cargo.lock has a single lbug (0.17.0, fork); `cargo build --bin simard --bin simard-tui` succeeds. The fork engine writes on-disk format v42 (forward-only from v41). This unblocks running Simard-family agents (Crocutus, the platform installer) from source on newer-libstdc++ hosts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(deny): allow the rysweet/ladybug-rust git source (lbug fork, #3119) The lbug portability fix repoints lbug at the rysweet/ladybug-rust fork; add it to the [sources] allow-git allowlist so cargo-deny's unknown-git=deny policy passes. Verified locally: `cargo deny check sources` -> sources ok. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(pins): update #2626 pin guards for the #129 memory bump + lbug fork git dep The pin-bump broke three guards in issue_2626_amplihack_pin_bump.rs: - MEMORY_TARGET_REV was the old 72c5ea1b (#126); bump it to 5807d056 (#129, the lbug portability fix) and move MEMORY_STALE_REV to 72c5ea1b as the new anti-regression sentinel. - direct_lbug_pin_matches_the_single_locked_version assumed lbug is a crates.io `= "=X"` version pin. lbug is now a git dep on the rysweet/ladybug-rust fork, so the guard now asserts the direct lbug is a git dep on the fork remote at the expected full-SHA rev AND that Cargo.lock resolves exactly one lbug from that fork rev (single engine / one store format — the invariant is unchanged). All 9 guards pass locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fix the last 3 review findings from #110 and #114:
Resolves #110, resolves #114
🤖 Generated with Claude Code