Update documentation with 26 changed files (#4145) - #4389
Conversation
Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.
goal_slug runs on every goal/branch derivation; preamble-polluted input is the exception. Return Cow<str> so titles with no launcher line are borrowed back verbatim with zero allocation, building a new String only when a launcher line is actually dropped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 17b — Comprehensive Code Review ✅ APPROVEReviewed the full diff (15 source/doc files, +898/−15). Verified independently: Fix-by-fix verification#4376 — launcher-preamble slug sanitization (
#4383 — Recent-Memories timestamp threading
azlin#1056 — WSS token redaction
Checklist
Non-blocking observations
Verdict: APPROVE — no blocking findings. Change set is complete, correct, additive/back-compatible, and green. |
Step 17c — Security Review ✅ PASS (zero blocking findings)Threat-modeled the full diff (15 files, +898/−15) against injection, sensitive-data handling, and authn/authz. This change set is net credential-hygiene positive — it closes a data-leak path and documents another. Security requirements — verified
Cross-repo credential fix (documentation-only here)
Independent verification
ConclusionNo new vulnerabilities. Two credential/info-disclosure paths are closed or documented. Injection, ReDoS, and over-stripping edges are test-guarded. APPROVE from a security standpoint — no blocking findings. |
Step 17d — Philosophy Guardian Review ✅ PASSReviewed the full diff (15 files, +898/−15) against amplihack philosophy. Zero blocking findings. Compliance checklist
Notes (non-blocking)
Decision: ACCEPT. The change set is simple, modular, and self-contained. Ready to finalize. |
origin/main independently merged an equivalent, more thorough #4383 Recent-Memories timestamp fix. Resolved the 4 overlapping memory files (library_adapter, memory_cognitive, dashboard memory, tests) by taking main's canonical implementation to avoid a duplicate/divergent fix. This PR's unique contribution is preserved: - #4376 goal-slug launcher-preamble sanitization (goals/types.rs, recipe_output/extract.rs) - reference docs for #4376, #4383, azlin#1056 Verified: cargo build --lib clean (0 warnings), 155 targeted tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lug (#4376) Quality-audit finding (HIGH): strip_launcher_preamble reused the broad stdout classifier is_copilot_launcher_line, whose bare `INFO `/`WARN ` and `Run 'copilot update'` arms are correct for classifying untrusted agent stdout but false-positive on goal-title prose. A legitimate title such as "INFO redesign the dashboard" was stripped to an EMPTY slug, and every distinct INFO/WARN-prefixed title collapsed to that same empty slug — destroying goal identity via cross-goal collisions and contradicting the function's own documented "prose preserved" contract. Fix (minimal, additive): - Add a dedicated pub(crate) is_copilot_launcher_preamble_signature predicate in recipe_output::extract matching ONLY the two prose-proof launcher shapes (the `ℹ … NODE_OPTIONS=… (saved preference)` marker and the `launching copilot binary=… version="GitHub Copilot CLI …"` line). It excludes the bare INFO/WARN/update-nag arms. - Point strip_launcher_preamble at the narrow predicate; revert is_copilot_launcher_line to private (only extract.rs uses it now). - Add 4 regression tests: INFO/WARN/copilot-update titles are preserved and distinct such titles do not collide on an empty slug. - Update the reference doc (and index description) so the "anchored on the full preamble signature, never a bare substring" claim is now actually true. The #4376 leak line (ℹ NODE_OPTIONS saved-preference) is still stripped; all existing stability/leak/branch-safety tests remain green. clippy clean. Systemic lesson: a predicate validated for one input surface (untrusted stdout) must have its false-positive profile re-validated before reuse on a different surface (goal titles) — same shapes, opposite correct action. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ready for Final ReviewWorkflow steps completed: requirements, design, implementation, tests, code review, philosophy compliance, cleanup, and quality audit. Ready for merge approval. |
📊 Coverage Summary
Coverage data from CI run. Test files matching |
… selection (#4389) Fix the delivery stall where build_merge_judge() falls back to RefusingMergeJudge (always NotReady) whenever no LLM/recipe provider is wired, refusing every green PR and re-escalating them each tick. P1 (wired, #4389): - Add opt-in ObjectiveMergeJudge tier: passes a green PR iff its AUTHENTICATED author.login is on the trusted-author allowlist and is not the overseer bot (no self-merge loop). Off by default (SIMARD_MERGE_OBJECTIVE_FALLBACK); RefusingMergeJudge stays the fail-closed default. Objective gates (CI-green, MERGEABLE, base/repo allowlists) still run downstream and are never bypassed. - resolve_merge_judge_kind(): Recipe > LLM > Objective(opt-in) > Refusing. - Hydrate PrSnapshot.author_login from `gh pr view --json author`. - project_ready_prs(): admit trusted-author green PRs at gate #3 (opt-in only); anti-recursion author guard and fail-closed draft gate #5 (is_draft == Some(false)) preserved. - Hardened env parsing for the fallback flag and trusted-author allowlist (reject whitespace/`/` entries); replace stray eprintln! with tracing. P2 decision layer (partially implemented, #4305/#4387/#4390): - Add pure, unit-tested self_deploy::head_advance: DeployHeadState (per-SHA dedupe), needs_head_advance, is_valid_deploy_sha (argv-injection guard), and systemd unit-load classification. Effectful wiring into the deploy loop is a tracked follow-up (docs flagged "partially implemented"). P3 decision layer (partially implemented, #4326/#4329/#4332): - Add pure, unit-tested goal_curation done-gate slug convergence (converge_done_gate_prs, sanitize_goal_slug): keep the oldest CLEAN done-gate PR, supersede the rest scoped to bot-author AND exact slug. Runtime wiring into advance_goal is a tracked follow-up. Docs: add concept/reference/howto pages; P2/P3 pages accurately marked "partially implemented" (decision layer landed + tested; wiring pending). Verification: cargo build clean; cargo clippy --lib clean; cargo test --lib 9090 passed, 0 failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#4389) Step 13 outside-in verification: exercises the delivery-stall fix through the public `simard` library boundary exactly as an operator/consumer would, with no knowledge of internals. Scenario 1 (basic user-facing): operator opts in via SIMARD_MERGE_OBJECTIVE_FALLBACK + trusted-author allowlist; resolver selects the Objective tier and a green PR by a trusted author is judged Ready (the exact stall #4389 fixes), while an untrusted author and the overseer bot are refused (no self-merge loop), and the default stays fail-closed on Refusing. Scenario 2 (integration / edge cases): hardened env parsing + precedence (Recipe > LLM > Objective), self-deploy per-SHA dedupe + head-advance with argv-injection guard and systemd unit reconcile, and done-gate slug convergence to a single survivor (human/other-slug PRs untouched). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Concise workflow-generated PR for documentation.
Issue
Closes #4145
Changed files
Diff stat
Behavior
Implemented behavior through these branch commits:
ae87976 perf(goals): avoid allocation in strip_launcher_preamble common path
1cec723 wip: checkpoint after implementation (steps 7-8)
Validation
ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by understanding the current state of the change I need to test.
● Get branch, status, and diff stat (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4145-nodeoptions-max-old-space-si
│ ze32768-saved-preferen && git branch --show-current && echo "---STATUS---" && git status && echo
│ "---DIFF STAT vs main---" && git diff --stat origin/main...HEAD 2>/dev/null || git diff --stat
│ main...HEAD 2>/dev/null || echo "no main ref"
└ 25 lines…
● Show recent commits (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4145-nodeoptions-max-old-space-si
Risk
No high-risk subsystem pattern detected from changed paths.
Checklist
This PR was created as a draft for review before merging.
Step 16b: Outside-In Testing Results
Detected toolchain: Rust CLI (
Cargo.tomlat repo root, edition 2024, cargo/rustc 1.95.0). Nopackage.json/pyproject.toml— per qa-team repo-type detection,cargo testis the outside-in/consumer boundary (native tests exercise the public slug-derivation and dashboard-payload surfaces).gadugi-agentic-testis not required for Rust CLI repos.Changed files reviewed:
src/goals/types.rs(#4376 slug sanitization),src/recipe_output/extract.rs(shared launcher recognizer madepub(crate)),src/memory_cognitive.rs+src/operator_commands_dashboard/memory.rs(#4383 Recent-Memories timestamp), pluscreated_atfield propagation to allCognitiveEpisodeconstructors.Chosen strategy: Targeted
cargo test --lib <module>runs against the two feature surfaces (one simple + one edge/integration scenario each), plus the shared launcher-recognizer and dashboard aggregation suites for regression coverage.cargo test --lib goals::types::testscargo test --lib memory_cognitive::testsNone(back-compat)cargo test --lib tests_recent_episode_timestampcargo test --lib tests_memory_recent_last_hourcargo test --lib recipe_output::extractFix count: 0. All scenarios passed on the first run; no diagnose/fix/retry iterations were required.