Add footers to pull requests created

[atoomic]

the goal is to track the model/provider used when submitting a PR
so we can know what the options were during implementation

[atoomic]

failures from CI

=========================== short test summary info ============================
FAILED tests/test_silent_exceptions.py::TestNoSilentBroadExceptions::test_no_silent_broad_catches_in_app - Failed: Silent broad exception handler(s) detected.
Every `except Exception` must have diagnostic output
(print to stderr, log(), logging.*, or raise).

Violations:
  app/pr_footer.py:96 — except Exception:
  app/pr_footer.py:105 — except Exception:

Fix: add `print(f'[module] error: {e}', file=sys.stderr)` or use logging/log().
If the catch is intentionally silent, add ("filename", "function_name") to ALLOWLIST in test_silent_exceptions.py with a comment.
============ 1 failed, 14002 passed, 1 warning in 82.67s (0:01:22) =============

[BabyKoan]

@atoomic: failures from CI

Fix required per project exception handling standards. Add diagnostic output to both except Exception: blocks in resolve_footer_provider_model():

except Exception as e:
    print(f'[pr_footer] provider lookup failed: {e}', file=sys.stderr)
    provider_name = ''

Same pattern for line 105 (model config). This matches the project's run_log.py patterns for non-critical fallback paths.

[BabyKoan]

Previous review — superseded by a newer review below.

[BabyKoan]

Blocking issues found — see the review comment above.

[BabyKoan]

Rebase with requested adjustments

Branch footers was rebased onto main and review feedback was applied.

Changes applied

• Changes address all review feedback about silent exception swallowing in PR footer generation.
• koan/app/pr_footer.py: Added logging import and _logger. Wrapped bare except blocks in resolve_footer_provider_model(), _short_head(), and _elapsed_label() with _logger.debug(...) calls so provider/model/HEAD/duration lookup failures leave a diagnostic trace instead of silently returning empty strings.
• koan/app/claude_step.py: Added _logger and replaced the bare print(..., file=sys.stderr) in the cross-link comment handler with _logger.warning(...) that includes full_repo and pr_number context.
• koan/app/pr_submit.py: Added logger.warning(...) when KOAN_MISSION_STARTED_AT holds an invalid value, making timestamp propagation issues visible in logs.

Stats

17 files changed, 331 insertions(+), 13 deletions(-)

Actions performed

• Already-solved check: skipped (Claude call failed)
• Rebased footers onto upstream/main
• Applied review feedback
• Pre-push CI check: previous run #27181387739 failed
• Pre-push CI fix: no changes needed or Claude found nothing to fix
• Force-pushed footers to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Kōan

[atoomic]

view failures from CI and fix

=========================== short test summary info ============================
FAILED tests/test_silent_exceptions.py::TestNoSilentBroadExceptions::test_no_silent_broad_catches_in_app - Failed: Silent broad exception handler(s) detected.
Every `except Exception` must have diagnostic output
(print to stderr, log(), logging.*, or raise).

Violations:
  app/pr_footer.py:96 — except Exception:
  app/pr_footer.py:105 — except Exception:

Fix: add `print(f'[module] error: {e}', file=sys.stderr)` or use logging/log().
If the catch is intentionally silent, add ("filename", "function_name") to ALLOWLIST in test_silent_exceptions.py with a comment.
============ 1 failed, 14002 passed, 1 warning in 82.67s (0:01:22) =============

[atoomic]

Created follow-up issue #1849 to track the architectural re-design: moving generic mission PR creation from the LLM session into Python (unifying with the skill-dispatch path). This will fix the missing rich footer on all future generic mission PRs.

[BabyKoan]

@atoomic: view failures from CI and fix

Fix required per project exception handling standards. Add diagnostic output to both except Exception: blocks in resolve_footer_provider_model():

except Exception as e:
    print(f'[pr_footer] provider lookup failed: {e}', file=sys.stderr)
    provider_name = ''

Same pattern for line 105 (model config). This matches the project's run_log.py patterns for non-critical fallback paths.

[BabyKoan]

PR Review — Add footers to pull requests created

Feature implementation blocked by silent exception handling.

• Critical: 2 bare except Exception: blocks in pr_footer.py (lines 96, 105) - CI already flagged this
• Warning: Git subprocess and elapsed time error paths silently discard failures
• Suggestion: Hardcoded skill list will drift - consider SKILL.md frontmatter flag or central registry

Fix the silent exception handlers before merging. The footer feature itself is solid - provider/model/HEAD/duration tracking adds valuable observability.

---

🟡 Important

1. Cross-link comment failure only logged to stderr (`koan/app/claude_step.py`, L1514-1522)

The print(..., file=sys.stderr) pattern works but is inconsistent with the project's run_log.py logging patterns. Consider using the shared logger for better diagnostics in production logs.

except Exception as e:
    print(f"Failed to cross-link {full_repo}#{pr_number}: {e}", file=sys.stderr)

🟢 Suggestions

1. Hardcoded skill list will drift (`koan/app/run.py`, L2033-2037)

The hardcoded set {"fix", "implement", "incident", "rebase", "recreate"} will become stale as new skills are added. Consider:

1. Adding a footer_enabled: true flag to SKILL.md frontmatter
2. Central registry in config.py
3. SKILL.md parsing to discover which skills create PRs

This prevents future skills from missing footer attribution.

_mission_model_key = (
    "mission"
    if _mission_command in {"fix", "implement", "incident", "rebase", "recreate"}
    else ""
)

---

Checklist

• No bare except Exception without diagnostic output — critical ＃1, critical ＃2
• Error paths have diagnostic logging — warning ＃3, warning ＃4
• No hardcoded secrets or credentials
• Input validation at boundaries
• Resource cleanup in error paths
• Test coverage for new code paths
• No SQL/command injection risks

---

To rebase specific severity levels, use: /rebase <url> critical (fixes 🔴 only), /rebase <url> important (fixes 🔴 + 🟡), or just /rebase <url> for all.

---

Silent Failure Analysis

🟡 **MEDIUM** — silent exception suppression with empty string return (`koan/app/pr_footer.py:95-108`)

Risk: Broad except Exception catches all errors (including unexpected ones like ImportError or AttributeError) and silently returns empty string, making debugging impossible.

try:
    from app.provider import get_provider_name
    provider_name = get_provider_name()
except Exception:
    provider_name = ""

Fix: Catch specific expected exceptions (e.g., Exception is acceptable here per the allowlist, but should log at debug level for diagnosability).

🟡 **MEDIUM** — silent exception suppression with empty string return (`koan/app/pr_footer.py:109-117`)

Risk: Same pattern as above — any exception during config lookup is silently discarded, leaving no trace of what went wrong.

try:
    from app.config import get_model_config
    models = get_model_config(project_name)
    if isinstance(models, dict):
        model = str(models.get(model_key, "") or "")
except Exception:
    model = ""

Fix: Add debug-level logging before returning empty string to preserve observability while maintaining best-effort semantics.

🟡 **MEDIUM** — silent subprocess failure with empty string return (`koan/app/pr_footer.py:124-138`)

Risk: Git command failures (including timeout, missing git, or non-git directory) return empty string with no logging, making it impossible to diagnose why HEAD metadata is missing.

try:
    result = subprocess.run(
        ["git", "rev-parse", "--short", "HEAD"],
        cwd=project_path,
        ...
    )
except (OSError, subprocess.SubprocessError):
    return ""

Fix: Log the exception at debug level with the project_path context before returning empty string.

🟡 **MEDIUM** — silent type/value error with empty string return (`koan/app/pr_footer.py:140-152`)

Risk: Invalid timestamp formats or non-numeric started_at values fail silently, losing the ability to detect clock skew or data corruption issues.

try:
    elapsed = max(0, int(time.time() - float(started_at)))
except (TypeError, ValueError):
    return ""

Fix: Log the raw started_at value at debug level when conversion fails to aid debugging of timestamp pipeline issues.

🟡 **MEDIUM** — silent exception suppression in cross-link comment (`koan/app/claude_step.py:1514-1524`)

Risk: GitHub comment creation failures are caught but the exception handling is not visible in this diff snippet — if it only logs or passes, PR cross-linking failures become invisible.

try:
    crosslink = append_koan_footer(...)
    run_gh("pr", "comment", pr_number, ...)
    actions.append("Cross-linked original PR")
except Exception as e:
    # [silently continues]

Fix: Ensure the except block logs the exception with the PR number and error details; consider adding a warning action to the result.

🟡 **MEDIUM** — environment variable parsing without validation (`koan/app/run.py:2030-2045`)

Risk: If mission_title parsing fails or returns unexpected values, _mission_command becomes empty string and _mission_model_key silently defaults to empty, causing inconsistent model tracking.

_mission_command = mission_command_name(mission_title)
_mission_model_key = (
    "mission"
    if _mission_command in {"fix", "implement", "incident", "rebase", "recreate"}
    else ""
)

Fix: Validate mission_command_name output against expected commands and log unexpected patterns for monitoring.

---

Automated review by Kōan (Ollama-launch · model qwen3.5:cloud) HEAD=cdab09d 1 min 37s

[BabyKoan]

Blocking issues found — see the review comment above.