fix(stewardship): dedup search must use a bare signature, not a qualifier (#4962)

[rysweet]

Problem (Observed Problem #2 — stewardship issue-churn)

The Overseer re-filed identical [stewardship] recurring_goal_reblock in simard::overseer issues every tick — a churn storm of 30+ open duplicates (#4962, #4956, #4951, #4945, #4942, plus [stewardship] workstream_gap:issue #4957), all sharing the same stewardship-signature: cfa5358a3b59894c. So the dedup key was stable; the existence check was broken.

Root cause (confirmed live)

src/stewardship/gh_client.rs::issue_list_args built the dedup search as:

gh issue list --search "stewardship-signature:<sig> in:body"

GitHub issue search parses a leading <word>: token as a search qualifier, so stewardship-signature:<sig> is treated as an unknown qualifier and matches nothing. Verified against the live repo:

• --search "stewardship-signature:cfa5358a3b59894c in:body" → []
• --search "cfa5358a3b59894c in:body" → 30+ existing duplicates

With 654 open issues, the strongly-consistent RecentOpen(100) fallback also can't see the older tracking issue, so every tick re-files.

Fix

Search for the bare 16-hex signature as a full-text term ("<sig> in:body") — the form already used correctly in supply_chain_steward::gh. A 64-bit hex signature is specific enough to match only the intended tracking issue.

Additive / non-breaking: query semantics only. Issue-body marker format and the RecentOpen fallback are unchanged.

Tests

• Updated issue_list_args_uses_search_for_signature_query to the corrected argv.
• Added regression signature_search_is_bare_fulltext_not_a_qualifier pinning that the search term never re-introduces the stewardship-signature: qualifier.
• cargo test --lib stewardship::gh_client → 22 passed. cargo fmt + clippy -D warnings clean (pre-commit).

The pre-existing duplicate issues are being closed/deduped as cleanup.

Refs #4962 #4956 #4951 #4945 #4942 #4957

---

Step 16b: Outside-In Testing Results

Detected toolchain: Rust (Cargo.toml/Cargo.lock at root, package simard, edition 2024, cargo 1.95.0). Changed file: src/stewardship/gh_client.rs. Per the qa-team skill, Rust CLI repos use cargo test as the outside-in boundary.

Chosen strategy: Exercise the module tests (cargo test) for the fast/regression boundary, plus a real end-to-end gh CLI comparison against rysweet/Simard to prove the search-term change behaves correctly at the true consumer boundary a user/Overseer hits.

#	Scenario	Command	Result	Key output
1 (simple)	Signature-query arg builder + regression test	cargo test --lib stewardship::gh_client	✅ PASS	22 passed; 0 failed — includes new signature_search_is_bare_fulltext_not_a_qualifier
2 (integration)	Full stewardship module suite (resolve/dedup/fallback paths)	cargo test --lib stewardship::	✅ PASS	169 passed; 0 failed
3 (edge, real consumer boundary)	Old buggy qualifier form vs. new bare full-text form against live GitHub search	gh issue list -R rysweet/Simard --state open --search "stewardship-signature:cfa5358a3b59894c in:body" vs. ... --search "cfa5358a3b59894c in:body"	✅ PASS	OLD form → 0 matches (reproduces the dedup bug); NEW form → 1 match (#4581, the original tracking issue). Confirms dedup now resolves the prior issue instead of re-filing.

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

CI: 18/18 required checks passing; mergeable = MERGEABLE.

[rysweet]

Step 17b — Comprehensive Code Review: APPROVE (posted as comment; author cannot self-approve) ✅

Reviewed the full diff of src/stewardship/gh_client.rs (the sole changed file), the surrounding dedup pipeline, and the referenced supply_chain_steward::gh pattern. This is a correct, minimal, non-breaking fix.

Root-cause & fix — correct

gh issue list --search "stewardship-signature:<sig> in:body" was parsed by GitHub as an unknown search qualifier, matching nothing, so the fast-search dedup path always returned empty and every tick re-filed a duplicate. Switching to the bare "<sig> in:body" full-text term (mirroring the already-correct supply_chain_steward/gh.rs:165) resolves it. The qualifier-parsing root cause is consistent with GitHub issue-search semantics.

Logic correctness — verified

The bare full-text search is safely backstopped: resolve_dedup_candidates (L328–337) hands search hits to find_existing (dedup.rs:152), which matches the exact marker stewardship-signature: <sig> in the body. So even though the search term is now a bare 16-hex token, the actual dedup decision stays precise — no false-positive risk from an issue that merely mentions the hex elsewhere. If the search index lags, the strongly-consistent RecentOpen fallback still covers it. Fail-loud error propagation is preserved (no swallowed errors).

Review checklist

• ✅ Code quality / standards — clear, well-scoped comment explaining the qualifier pitfall; consistent with the sibling module.
• ✅ Test coverage — updated issue_list_args_uses_search_for_signature_query to the corrected argv, plus new regression signature_search_is_bare_fulltext_not_a_qualifier pinning that the term never re-introduces the stewardship-signature: qualifier. cargo test --lib stewardship::gh_client → 22 passed / 0 failed (reproduced locally).
• ✅ No TODOs / stubs / swallowed exceptions — none; errors propagate fail-loud.
• ✅ No unimplemented functions — n/a.
• ✅ Edge cases — index-lag handled via RecentOpen; exact-marker post-filter guards against bare-hex false positives.

CI

All 18 required checks green; mergeable = MERGEABLE, state CLEAN.

No blocking issues. LGTM — recommend merge. The regression test locks in the contract so this specific bug cannot silently return.

[rysweet]

Step 17c — Security Review: PASS ✅ (no vulnerabilities)

Security-focused review of PR #4980 (src/stewardship/gh_client.rs, +37/−2). The change swaps the dedup search term from the broken qualifier form stewardship-signature:<sig> in:body to the bare full-text form <sig> in:body.

Checklist

• Command / shell injection — NONE. The search term is passed via Command::new("gh").args(&args) (argv, line ~343), not a shell string. There is no shell to interpolate into; --search "<term>" is a single argv element, so metacharacters cannot break out.
• Untrusted input into argv — NOT POSSIBLE. The signature is sha256(kind || "\n" || normalized_message) truncated to the first 8 bytes and rendered {b:02x} (dedup.rs:137), i.e. strictly [0-9a-f]{16}. Even fully attacker-controlled error text can only influence which hex digest is produced — never inject characters into the argv.
• Injection vulnerabilities (search-qualifier / false-positive DoS) — MITIGATED. Broadening to a bare full-text term widens the candidate set (an attacker could file an issue merely mentioning a 16-hex string), but the authoritative "does a tracking issue already exist?" decision is gated by find_existing (mod.rs:92, gh_client.rs:333), which requires the exact literal marker stewardship-signature: <sig> in the issue body. A bare mention therefore cannot suppress alerting or de-dup a real failure away → no alert-suppression DoS introduced.
• Sensitive data handling — UNCHANGED / OK. Only issue number,url,title,body are fetched; nothing new is logged. Signature normalization already redacts tokens/UUIDs (redact_token), so secrets are not embedded in the searched signature.
• AuthN / AuthZ — UNCHANGED. Relies on the ambient gh credential; this PR does not alter auth scope or add new privileged calls.
• Error handling — fail-loud preserved. Non-zero gh exit and parse errors propagate as StewardshipGhCommandFailed (lines ~344, ~349); no silent swallow that could mask a degraded/spoofed search.

Verdict

No new vulnerabilities. No injection, no secret exposure, no auth changes. The broadened search is safely backstopped by the exact-marker dedup gate. Approved from a security standpoint.

[rysweet]

Step 17d — Philosophy Guardian Review: PASS ✅

Evaluated the sole changed file src/stewardship/gh_client.rs (+37/−2) against the amplihack philosophy (ruthless simplicity, bricks & studs, zero-BS).

Compliance Checklist

• Ruthless simplicity achieved — The fix is a one-line behavioral change (stewardship-signature:{sig} in:body → {sig} in:body). No new abstractions, types, or indirection introduced. The bulk of the diff is an explanatory comment + a regression test, not new machinery.
• Bricks & studs pattern followed — Change is fully contained within the issue_list_args "brick"; the IssueListQuery::Signature contract (the "stud") is unchanged, so no ripple to callers (find_existing, resolve_dedup_candidates).
• Zero-BS implementation — No stubs, no faked APIs, no swallowed exceptions. Fail-loud error handling preserved. The new test asserts real behavior, not a placeholder.
• No over-engineering — Resisted the temptation to build a query-escaping DSL; picked the minimal correct fix that mirrors the proven supply_chain_steward::gh pattern.
• Clean module boundaries — Bare full-text search is safely backstopped by find_existing's exact stewardship-signature: <sig> marker match, so correctness is layered without leaking concerns across modules.

Notes

• The inline comment documents the why (GitHub parses a leading <word>: as a qualifier → matched nothing → duplicate-issue storm) precisely — this is the kind of high-value comment the philosophy endorses.
• Regression test signature_search_is_bare_fulltext_not_a_qualifier locks the invariant against future reversion.

Verdict: APPROVE — fully philosophy-compliant. No simplification or de-risking required.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	214946	181708	84.5%

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