Update documentation with 33 changed files (#4575)

[rysweet]

Summary

Concise workflow-generated PR for documentation.

Issue

Closes #4575

Changed files

• docs/design/agentic-observe-orient-merge-queue.md
• docs/design/ecosystem-observe.md
• docs/ecosystem-map.md
• docs/howto/configure-agentic-merge-queue-reasoning.md
• docs/reference/agentic-merge-queue-reasoning-api.md
• docs/reference/ci-health-sweep.md
• docs/reference/ecosystem-roster-resolution.md
• docs/reference/engineer-worktree-isolation.md
• docs/reference/engineer-worktree-presence-guard.md
• docs/testing/deflaking-known-flaky-tests.md
• prompt_assets/simard/ecosystem_repos.toml
• prompt_assets/simard/engineer_system.md
• prompt_assets/simard/identity/stewarded_repos.seed.toml
• src/ci_health/mod.rs
• src/ci_health/tests.rs
• src/engineer_worktree/mod.rs
• src/engineer_worktree/tests_presence_guard.rs
• src/identity_curated_state.rs
• src/install/paths.rs
• src/lib.rs
• src/ooda_actions/advance_goal/mod.rs
• src/ooda_actions/advance_goal/subordinate.rs
• src/ooda_actions/advance_goal/tests_presence_guard.rs
• src/ooda_actions/advance_goal/typed_goal_session.rs
• src/ooda_loop/cycle.rs
• src/overseer/ecosystem_observe.rs
• src/overseer/mod.rs
• src/overseer/wiring.rs
• src/stewardship/gh_client.rs
• tests/adaptive_scaling.rs
• tests/ecosystem_observe_assets.rs
• tests/ooda_daemon.rs
• tests/qa-scenarios/identity-curated-stewarded-roster.yaml

Diff stat

 src/ci_health/tests.rs                             |  58 +--
 src/engineer_worktree/mod.rs                       |  25 +-
 src/engineer_worktree/tests_presence_guard.rs      | 184 ++++++++
 src/identity_curated_state.rs                      | 482 ---------------------
 src/install/paths.rs                               |  26 +-
 src/lib.rs                                         |   1 -
 src/ooda_actions/advance_goal/mod.rs               |   2 +
 src/ooda_actions/advance_goal/subordinate.rs       |  28 +-
 .../advance_goal/tests_presence_guard.rs           | 229 ++++++++++
 .../advance_goal/typed_goal_session.rs             | 104 +++--
 src/ooda_loop/cycle.rs                             |  10 +-
 src/overseer/ecosystem_observe.rs                  | 459 +++++++++-----------
 src/overseer/mod.rs                                |  15 +-
 src/overseer/wiring.rs                             |  89 ++--
 src/stewardship/gh_client.rs                       | 198 +--------
 tests/adaptive_scaling.rs                          |  27 +-
 tests/ecosystem_observe_assets.rs                  |  19 +-
 tests/ooda_daemon.rs                               |   5 -
 .../identity-curated-stewarded-roster.yaml         |  65 ---
 33 files changed, 1572 insertions(+), 1883 deletions(-)

Behavior

Implemented behavior through these branch commits:
7685b42 fix(engineer-worktree): guard reuse against reaped worktrees (#4578)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by understanding the current state: branch, changed files, and toolchain.

● Get branch, status, and diff stat (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4575-nodeoptions-max-old-space-si
│ ze32768-saved-preferen && git branch --show-current && echo "---REMOTE---" && git remote -v |
│ head -2 && echo "---STATUS---" && git status --short && echo "---DIFF STAT vs main---" && git
│ --no-pager diff --stat main...HEAD 2>/dev/null | tail -30
└ 37 lines…

This is a Rust (Cargo) project. Let me see the full diff stat and recent commits to understand the actual change.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 1 commit(s) ahead of main
• Code review completed
• Philosophy check passed

---

This PR was created as a draft for review before merging.

---

Step 16b: Outside-In Testing Results

Tested the presence-guard fix (#4578) from the consumer boundary as a user of the worktree/goal-session lifecycle.

Detected toolchains: Rust CLI (Cargo.toml at root, cargo 1.95.0) — primary; Node.js CLI wrapper (package.json, bin.js) — unchanged by this PR. All changed files are Rust (src/engineer_worktree/, src/ooda_actions/advance_goal/, src/ooda_loop/cycle.rs) plus docs.

Chosen strategy: Per qa-team repo-type detection, Rust CLI repos use native cargo test (not gadugi-agentic-test). Exercised the new EngineerWorktree::is_present() presence seam and its stored-map/discovery consumers through the documented release test gate: cargo test --release --lib -- --test-threads=8 presence_guard. Also ran cargo fmt --all -- --check.

Scenarios

#	Type	Scenario	Command	Result	Key output
1	Simple (happy path)	Live worktree reports present so consumers reuse it (is_present_true_after_allocate, stored_map_worktree_reports_present_while_live, discovery_finds_live_worktree_while_present)	cargo test --release --lib -- presence_guard	✅ PASS	discovery returns the live path; stored map reports present
2	Edge / integration	Reaped-out-of-band worktree no longer counts as a live worker — the #4578 fault (is_present_false_when_dir_reaped_out_of_band, discovery_returns_none_after_worktree_reaped, stored_map_worktree_reports_absent_after_reap, is_present_false_when_claim_sentinel_removed, is_present_false_after_cleanup, is_present_is_side_effect_free_and_idempotent, attached_worktree_lives_under_managed_root)	cargo test --release --lib -- presence_guard	✅ PASS	after remove_dir_all, is_present() → false and discovery → None; no phantom live worker

Aggregate: test result: ok. 10 passed; 0 failed; 0 ignored; 9286 filtered out; finished in 0.32s. cargo fmt --all -- --check exit 0.

Fix count: 0 — all outside-in scenarios passed on the first run; no diagnose/fix/push iterations were required.

---

⚠️ Behavior change (reviewer note — #4578)

This PR changes the assigned-engineer reuse outcome in advance_goal's live typed spawn. Previously, an already-assigned goal whose worktree had been reaped out of band returned a permanent Err (missing-workspace fault), which crash-looped the next cycle. It now:

• Present checkout → idempotent Succeeded reusing the existing engineer (unchanged happy path).
• Absent checkout → fail closed: emit a structured engineer_worktree.reaped_before_reuse warning, clear the stale assigned_to + map entry, and fall through to a clean re-provision instead of failing permanently.

This is intentional and is the mechanism that breaks the #4578 crashloop. Downstream reviewers/mergers should be aware that a previously terminal Err path is now a recoverable Succeeded/re-provision.

Follow-up commit (Step 18b — review feedback S3)

Normalized the reuse-branch evidence session_id for observability: the assigned short-circuit now derives session_id from the worktree checkout basename (with an existing-engineer fallback), matching the disk-discovery reuse branch instead of the previous synthetic engineer-{goal_id}. Check+mutate semantics are unchanged (probe still runs under the same lock_state guard). Commit 305df99fe.

[rysweet]

🔍 Comprehensive Code Review — Step 17b

Scope: #4578 engineer-worktree presence guard (TOCTOU fix). 9 files, +887/−33.
Verdict: ✅ Approve — no blocking issues. High-quality, well-tested, additive fix. Notes below are non-blocking.

Review checklist

• Code quality & standards — idiomatic Rust; structured tracing (no print!/println!); consistent event = "engineer_worktree.reaped_before_reuse" telemetry across all sites.
• Test coverage — real fault injection (allocate() + out-of-band remove_dir_all), not mocks. Covers happy path, own-cleanup, out-of-band reap, sentinel-only removal, idempotency, discovery-vs-stored-map, and managed-root layout anchor.
• No TODOs / stubs / swallowed exceptions — none introduced; presence probe fails closed.
• No unimplemented functions — is_present() fully implemented.
• Logic correctness — see analysis below; fix is sound.
• Edge cases — dir reaped, sentinel gone/corrupted, partial reap all handled → not-present → re-provision.

Strengths

1. Right-sized seam. EngineerWorktree::is_present() = one fail-closed claim read. read_engineer_claim_full already fails closed when the dir is gone, so no separate lstat needed — correctly justified in the doc-comment.
2. All five sites covered consistently. Discovery reuse (typed_goal_session, via PathBuf), assigned-goal reuse, stored-map reuse (subordinate.rs), and the live-worker signal (cycle.rs) each re-verify presence before trusting path().
3. Honest type handling. Discovery holds only a PathBuf, so it re-reads the claim directly rather than forcing an EngineerWorktree — avoids a false abstraction.
4. Recovery, not just detection. Stale entries are dropped + re-provisioned instead of returning a stale Succeeded that crashes the next cycle.

Non-blocking observations

1. Semantic change worth flagging (typed_goal_session.rs L960-964 removed). The already_assigned branch previously returned a permanent Err("goal already has an assigned engineer"). It now returns idempotent Succeeded when the worktree is present, or clears the assignment and re-provisions when reaped. This is intentional and is precisely what breaks the crashloop (an assigned-but-reaped goal could never recover before), but it is a behavior change beyond a pure null-check — reviewers should be aware the old permanent-error path is gone.
2. Residual TOCTOU window (inherent). A gap still exists between the presence read and the checkout's actual use next cycle. This is unavoidable for filesystem TOCTOU; the fail-closed → re-provision design is the correct mitigation and the doc acknowledges it. No action needed.
3. Per-cycle I/O in gather_per_goal_cycle_ctx (cycle.rs). worker_present changed from an O(1) HashMap::contains_key to is_present() (a claim-file read) per goal per cycle. Negligible at cycle cadence, but it does add a syscall on a hot path — worth a mental note if goal counts grow large.
4. Mutex held across FS I/O (typed_goal_session.rs). lock_state is now mut and spans the is_present() read plus the assignment/map mutation. Justified — the check+mutate must be atomic against concurrent state access — but note the lock now covers a syscall.
5. Minor evidence inconsistency. The assigned-present branch synthesizes session_id: format!("engineer-{goal_id}") while the discovery branch derives it from path.file_name(). Cosmetic (evidence label only); could derive from the stored worktree path for consistency.

Verification performed

• CI: 18/18 checks pass (incl. pre-commit 12m, coverage 7m50s, install-real, cargo-audit/deny/vet).
• mergeable: MERGEABLE; clean re-export path for read_engineer_claim_full (pub use in mod.rs).
• Confirmed scope is limited to the #4578 fix; coverage-gate (#4523/#4495) and amplihack-rs #983 correctly deferred to separate PRs.

No changes requested. Ready to merge once required approvals are in.

[rysweet]

🔒 Security Review — Step 17c

Scope: #4578 engineer-worktree presence guard (TOCTOU fix). 9 files, +887/−33. Reviewed against the actual diff (src/engineer_worktree/mod.rs, subordinate.rs, typed_goal_session.rs, cycle.rs, claim.rs read path).

Verdict: ✅ Approve — no exploitable security vulnerabilities found. The change is an additive, fail-closed robustness fix within a single trust domain (the supervisor daemon). No changes requested.

Security checklist

Item	Result
New vulnerabilities introduced	✅ None
Injection (shell/SQL/command/log)	✅ None — no command construction; tracing::warn! uses structured fields, values (goal_id, path) are daemon-internal
Sensitive data handling	✅ Clean — claim sentinel holds only PID + starttime; logs emit path/goal_id, no secrets. claim_key semantics unchanged
AuthN / AuthZ	✅ N/A — no auth surface touched; presence guard is a safety check, not a security boundary
Path traversal	✅ None — is_present() → read_engineer_claim_full(&self.path) joins a constant (ENGINEER_CLAIM_FILE) onto a daemon-owned PathBuf; no untrusted input reaches the path
Memory safety	✅ unsafe: 0 in diff. Parse is bounded and fail-closed (.ok()? on i32/u64) — malformed/oversized sentinels return None, no panic
Dependencies	✅ No new crates, no network, no deserialization of untrusted data

Non-blocking observations (defense-in-depth notes, no action required)

1. Symlink-following read (informational). read_engineer_claim_full uses fs::read_to_string, which follows symlinks. A local writer inside the worktree dir could plant a .simard-engineer-claim symlink so is_present() returns true. Not a real vuln: the worktree lives under the supervisor-owned state root (same trust domain as the daemon), the probe is read-only (it never writes or deletes based on the resolved target), and liveness is still owned separately by find_live_engineer_for_goal. An lstat/symlink_metadata probe would buy nothing here — correctly rejected by the author.

2. Residual TOCTOU window (by design). is_present() → use is inherently racy; a reaper can still remove the checkout after the probe. This is a safety/availability property, not a security one — it fails closed to re-provision, so there is no privilege escalation or stale-privilege reuse. Correct design.

3. Availability: FS syscall under lock (minor). In typed_goal_session.rs the is_present() blocking FS read now runs while holding lock_state, and cycle.rs/subordinate.rs add a per-cycle read to hot paths. On a wedged filesystem (e.g., stalled NFS) this could stall the OODA cycle / hold the state lock. Low likelihood on local worktrees; acceptable for this fix. Flagged only for future hardening (e.g., timeout/bounded probe), not for this PR.

Conclusion: The fail-closed → re-provision design is the security-correct choice — it eliminates the crashloop without opening any injection, traversal, escalation, or data-exposure vector. No secrets, no unsafe, no untrusted input on the new code paths. Approved from a security standpoint.

[rysweet]

🧭 Philosophy Guardian Review — Step 17d

PR #4612 — #4578 engineer-worktree presence guard (TOCTOU fix). 9 files, +887/−33 (core code +~130 lines; remainder tests + docs).
Verdict: ✅ PASS — fully compliant. Zero required changes.

Reviewed against the actual diff, not the transcript.

Compliance checklist

• Ruthless simplicity achieved — The entire fix hangs off one primitive, EngineerWorktree::is_present(), implemented as a single line: read_engineer_claim_full(&self.path).is_some(). No new abstraction layers, no config, no state machine. The doc-comment explicitly records the decision not to add a redundant lstat/symlink_metadata probe because the claim read "already fails closed when the directory is gone, so a separate lstat buys nothing." That is the simplest sufficient seam.
• Bricks & studs pattern followed — is_present() is a clean public stud on the EngineerWorktree brick. Presence (worktree on disk) and liveness (engineer PID) stay separate bricks: the comment notes discovery/find_live_engineer_for_goal owns liveness. The re-export change (pub use ...read_engineer_claim_full) is the honest minimal contract for the one call site (discovery) that holds a bare PathBuf rather than an EngineerWorktree — a real type boundary, not a shortcut.
• Zero-BS implementation — No stubs, no faked APIs, no swallowed exceptions. is_present() is fail-closed by design: any read error (dir reaped, sentinel removed/corrupted) becomes false, which drives re-provision — the errors are acted on, not hidden. Tests use real fault injection (actual allocate() + out-of-band remove_dir_all), not mocks.
• No over-engineering — Deferred robustness ideas (bounded-retry counter, forcing is_present() onto the PathBuf discovery path) were correctly rejected/deferred rather than speculatively built. Each of the four reuse sites calls the same primitive inline; no premature helper extraction for the warn! blocks (indirection > value here).
• Clean module boundaries — Change is additive and localized: presence probe lives in engineer_worktree, the four consumers (subordinate.rs, typed_goal_session.rs ×2, cycle.rs) each consult it at the point of use. No cross-module leakage; state mutation (remove/assigned_to = None) is confined under the existing lock_state guard.

Observations (non-blocking, no action required)

1. Behavior change is principled, not accidental — already_assigned shifts from a permanent Err to an idempotent Succeeded/re-provision. This is the correct fix for the crashloop and is well-justified in-comment; it aligns with "idempotent, fail-closed" over "brittle, fail-permanent."
2. The presence primitive appears in two forms — worktree.is_present() (typed) and read_engineer_claim_full(&path).is_some() (raw PathBuf on the discovery path). This is a justified consequence of the type boundary, is documented, and does not warrant a forced unification that would fight the types.
3. Residual TOCTOU window is inherent and correctly mitigated — check→use can never be atomic against an out-of-band reaper; the design fails closed to re-provision, which is the right disposition.

Conclusion: This is a model example of ruthless simplicity — the smallest change that fully closes the TOCTOU gap, honest about its residual window, no over-engineering. Approved from a philosophy standpoint. Ready to proceed.

[github-actions]

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module	Lines	Covered	Coverage
Total	197193	165819	84.1%

_{Coverage data from CI run. Test files matching tests?/ are excluded from line counts.}