fix(ooda): ensure ooda-stuck label before filing tracking issues (#4472)

[rysweet]

Summary

Concise workflow-generated PR for documentation.

Issue

Closes #4472

Changed files

• docs/index.md
• docs/reference/ooda-tracking-issue-label-resilience.md
• src/ooda_actions/advance_goal/spawn.rs
• src/ooda_actions/gh_label.rs
• src/ooda_actions/mod.rs
• src/ooda_loop/no_progress.rs

Diff stat

 docs/index.md                                      |   1 +
 .../ooda-tracking-issue-label-resilience.md        | 269 +++++++++++++++++++++
 src/ooda_actions/advance_goal/spawn.rs             |  73 ++++--
 src/ooda_actions/gh_label.rs                       | 239 ++++++++++++++++++
 src/ooda_actions/mod.rs                            |   4 +
 src/ooda_loop/no_progress.rs                       |  36 ++-
 6 files changed, 584 insertions(+), 38 deletions(-)

Behavior

Implemented behavior through these branch commits:
eb55d0c docs(ooda): align tracking-issue label doc with actual LabelEnsure API
94d71a2 refactor(ooda): centralize gh issue-create arg building in LabelEnsure
fa4eb37 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 checking the branch and the changes that need testing.

● Get branch name and git status (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4472-ooda-no-progress-breaker-can
│ not-file-its-operator && git branch --show-current && echo "---STATUS---" && git --no-pager
│ status && echo "---DIFF STAT---" && git --no-pager diff --stat HEAD
└ 9 lines…

● Show recent commits and diff stat vs main (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4472-ooda-no-progress-breaker-can
│ not-file-its-operator && git --no-pager log --oneline -5 && echo "---DIFF vs origin/main---" &&

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 3 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

Detected toolchain: Rust CLI (Cargo.toml/Cargo.lock at root, cargo 1.95.0, rustc 1.95.0). Per the qa-team skill's repo-type detection, native Rust tests (cargo test) are used — the gadugi-agentic-test framework is not required.

Chosen strategy: Exercise the changed tracking-issue escalation path from the outside-in consumer boundary via the crate's native test suite, then run the exact CI gates (cargo fmt --check, cargo clippy -D warnings) that guard merge. The three gh issue create sites (deterministic safeguard, EngineerLifecycleDecision::OpenTrackingIssue, and the no-progress GhIssueFiler) all now route through the shared LabelEnsure::issue_create_args decision, so validating that decision + its classification covers the user-observable behavior (issue filed with label when present, still filed unlabeled when the label can't be ensured — never silently dropped).

#	Scenario	Command	Result	Key output
1 (simple)	Label-ensure classification: success→Created, already exists→AlreadyExists (idempotent, case-insensitive), genuine failure→Unavailable with preserved non-empty cause; labeled vs degraded-unlabeled argv building	cargo test --lib gh_label --locked	✅ PASS	test result: ok. 8 passed; 0 failed
2 (edge/integration)	No-progress breaker suite — escalation, re-escalation idempotency, tracking-issue linking, reasoner/reinvestigation edge paths	cargo test --lib no_progress --locked	✅ PASS	test result: ok. 112 passed; 0 failed
3 (CI gate)	Formatting gate	cargo fmt --all -- --check	✅ PASS	exit 0, no diff
4 (CI gate)	Lint gate (deny warnings, all targets/features)	cargo clippy --all-targets --all-features --locked -- -D warnings	✅ PASS	Finished — exit 0, zero warnings

Non-breaking / safety checks: New code emits structured tracing::info!/tracing::warn! (targets simard::ooda_brain, simard::ooda) only — no new print!/println!/eprintln! and no new panics (the pre-existing success eprintln! in the deterministic safeguard is unchanged). The degraded path files the issue without --label rather than silently dropping it, and the Unavailable cause is always non-empty so failures stay operator-visible.

Fix count: 0 — all scenarios and CI gates passed on the first run; no code changes were required during outside-in testing.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	194447	163303	84.0%

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

[rysweet]

🔎 Comprehensive Code Review (Step 17b)

Verdict: ✅ Merge-ready. Reviewed all 6 changed files; built and re-verified locally on eb55d0c8.

What was reviewed

Fix for #4472 — the three gh issue create --label ooda-stuck sites failed every escalation cycle (could not add label: 'ooda-stuck' not found), silently recording escalated=1 while no issue was ever filed. The fix centralizes a label-ensure prelude in a new src/ooda_actions/gh_label.rs and routes all three sites through it.

Local verification

Gate	Result
cargo build --lib	✅ Finished, clean
cargo clippy --lib -- -D warnings	✅ Zero warnings
cargo test --lib gh_label	✅ 8/8 passed

Checklist

• Code quality & standards — gh_label.rs is a small, single-responsibility module. Pure classify_label_create is cleanly separated from the gh-shelling ensure_gh_label, making the decision logic unit-testable. issue_create_args is the single source of truth for the labeled/unlabeled argv, eliminating the cross-site duplication that caused OODA no-progress breaker cannot file its operator-facing tracking issue. Both gh issue create sites in src/ooda_actions/advance_goal/spawn.rs (the deterministic safeguard around lines 370-380 and th #4472.
• Test coverage adequate — 8 focused unit tests cover Created / AlreadyExists (case-insensitive) / Unavailable (cause preserved, non-empty even with empty stderr) and both labeled & degraded argv shapes. no_progress suite (112) stays green.
• No TODOs, stubs, or swallowed exceptions — every degraded/error branch emits structured tracing; Unavailable always carries a non-empty operator-legible cause. No silent swallow.
• No unimplemented functions — all paths complete.
• Logic correctness — the prelude is applied identically at all three sites; each site's existing success/Ok(out)-non-success/Err failure branches and its target: (simard::ooda_brain × 2, simard::ooda) are preserved unchanged. Degraded path files the issue without --label rather than dropping it.
• Edge case handling — case-insensitive "already exists"; spawn (io::Error) vs non-zero exit distinguished; empty-stderr failure still yields a non-empty reason.

Security

• ✅ Label is the compile-time OODA_STUCK_LABEL: &'static str (enforced by ensure_gh_label(label: &'static str)), so no title/body/branch string can flow into --label (argument-injection safe).
• ✅ Argv-only invocation via Command::args — never sh -c / shell interpolation.

Non-blocking observations (no change required)

1. Untested shell-out boundary — ensure_gh_label and GhIssueFiler::file_issue themselves aren't unit-tested (they spawn gh); coverage stops at the pure classify_label_create/issue_create_args. This is the correct, pragmatic boundary and is called out in the module docs. GhIssueFiler remains mockable via the NoProgressIssueFiler trait.
2. Extra subprocess per escalation — each filing now runs an idempotent gh label create before gh issue create. Escalation is a cold path, so the cost is negligible; noting for completeness.
3. Degraded path under auth failure — if the label can't be ensured due to auth (e.g. HTTP 401), gh issue create will fail for the same reason; the fix still correctly attempts to file and logs the failure via the preserved error branch. Working as intended.

Docs

docs/reference/ooda-tracking-issue-label-resilience.md (new) + docs/index.md link accurately describe the LabelEnsure API (Created/AlreadyExists/Unavailable(String), #[derive(Debug)], &'static str signature) — the earlier doc/code drift was already corrected in eb55d0c8.

No blocking issues found. Approve for merge.

[rysweet]

🔐 Security Review (Step 17c) — ✅ PASS, no blocking findings

Reviewed the full change set (gh_label.rs + 3 filing sites in spawn.rs, no_progress.rs, mod.rs) against the injection, authn/authz, and sensitive-data checklist. No new vulnerabilities introduced; the change is net-positive for security posture.

Checklist

Check	Result	Evidence
Injection (command/argument)	✅ Safe	Label is compile-time const OODA_STUCK_LABEL: &'static str = "ooda-stuck" — never runtime-derived. All invocation is argv-only via Command::new("gh").args(&gh_args); no sh -c, no string interpolation, no format!-into-command anywhere on the path.
Argument-injection via title/body	✅ Safe	issue_create_args places title/body positionally as the values of --title/--body in a fixed argv vector. A hostile value (e.g. --label, ; rm) is consumed as a flag value, not re-parsed as a flag or shell token. No reordering allows a runtime string to reach --label.
New attack surface	✅ Minimal	Adds one idempotent gh label create ooda-stuck subprocess per escalation. Label is constant; no new externally-influenced inputs.
Authn/Authz	✅ Sound	No new auth logic. Relies on ambient gh auth (unchanged). Auth failure (HTTP 401) is classified Unavailable(reason) and degrades to filing the issue without --label rather than dropping the escalation — fixes the prior silent-failure hole (#4472).
Sensitive-data handling	✅ Acceptable (1 note)	gh label create stderr is captured into Unavailable(reason) and surfaced via tracing::warn!. gh does not echo tokens to stderr, so real-world leak risk is low. Note: this logs whatever gh writes to stderr verbatim; if a future gh/network layer ever emitted a credential-bearing error, it would land in operator logs. Non-blocking; flagged for awareness.
Error surfacing (no silent swallow)	✅ Good	classify_label_create guarantees a non-empty reason for every failure (including empty-stderr case), and the original brain-failure is never swallowed by a filing error.
DoS / resource	✅ None	Single extra short-lived subprocess on an already cold escalation path; idempotent, no loops/retries added.

Findings

• No High/Medium findings.
• Low / informational (non-blocking): degraded-path reason is logged verbatim from gh stderr. Current risk is negligible (gh does not print secrets), but if stricter log-hygiene is desired later, consider truncating/redacting the reason string. No change required for merge.

Verdict

Security-approved for merge. The PR removes a real operational-security weakness (silent broken escalation that hid stuck goals from operators) and introduces no injection, auth, or data-exposure regressions.

[rysweet]

🧭 Philosophy Guardian Review (Step 17d)

Verdict: ✅ PASS — merge-ready. The change embodies ruthless simplicity and the bricks & studs pattern with zero over-engineering.

Compliance checklist

• Ruthless simplicity achieved — One small module (gh_label.rs): a 3-variant enum, one pure classifier, one thin shell-out wrapper, one argv builder. No layers, traits, or indirection beyond what the problem needs.
• Bricks & studs pattern followed — gh_label is a self-contained brick. Its studs (public contract) are exactly OODA_STUCK_LABEL, LabelEnsure, ensure_gh_label(), and issue_create_args(). All three filing sites (2× spawn.rs, 1× no_progress.rs) plug into the same contract; issue_create_args is the single source of truth for the labeled/unlabeled decision that OODA no-progress breaker cannot file its operator-facing tracking issue. Both gh issue create sites in src/ooda_actions/advance_goal/spawn.rs (the deterministic safeguard around lines 370-380 and th #4472 had duplicated and fixed inconsistently.
• Zero-BS implementation — No stubs, no faked APIs, no swallowed exceptions. Unavailable(String) always carries a non-empty cause; every failure is surfaced via warn! at the call site, and the degraded path still files the issue (unlabeled) rather than dropping it. ensure_gh_label is honestly documented as not-unit-tested (it shells out); the pure classify_label_create it delegates to is tested (8/8).
• No over-engineering — Minimal derive (#[derive(Debug)] only — no speculative Clone/PartialEq/Eq). Compile-time &'static str label, no config knobs, no premature generality.
• Clean module boundaries — Correct pub(crate) scoping; label_present and classify_label_create stay private. No internals leak; the module emits no tracing itself, leaving each caller its own static target:.

Non-blocking observation

Each of the three sites repeats the match label { Created / AlreadyExists / Unavailable } logging block with its own tracing target:. This is a deliberate and correct DRY boundary: the invariant (labeled-vs-unlabeled argv) is centralized in issue_create_args, while the varying part (per-site tracing target and surrounding failure branch) stays local. Centralizing the logging too would couple the sites and obscure their distinct failure contexts. No change recommended.

Philosophy compliance confirmed. No changes required.