From f84a8c2eec832921652ade529e7ebc53ec2bfb6b Mon Sep 17 00:00:00 2001 From: Oz Date: Sun, 26 Apr 2026 18:43:08 +0000 Subject: [PATCH 1/4] spec: add configurable response template specs Co-Authored-By: Safia Abdalla --- specs/GH347/product.md | 159 +++++++++++++++++++++++++++++++ specs/GH347/tech.md | 211 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 370 insertions(+) create mode 100644 specs/GH347/product.md create mode 100644 specs/GH347/tech.md diff --git a/specs/GH347/product.md b/specs/GH347/product.md new file mode 100644 index 0000000..70f1119 --- /dev/null +++ b/specs/GH347/product.md @@ -0,0 +1,159 @@ +# Issue #347: Support configurable template formats for Oz responses + +## Product Spec + +### Summary + +Repositories using Oz workflows should be able to customize deterministic Oz-authored workflow comment text through `.github/oz/config.yml`, using YAML multi-line string templates keyed by workflow and response surface. This should build directly on the shared workflow config introduced in #338, preserve current behavior when no templates are configured, and let maintainers change wording without forking the Python workflow code. + +### Problem + +Oz workflow comments are still assembled from hardcoded strings spread across shared helpers and workflow entrypoints. That works for this repository, but it forces OSS adopters to edit Python if they want to adjust wording, branding, or repository-specific instructions in Oz-authored comments. It also means small response-copy changes require code changes rather than a reviewed config update in source control. + +Issue #338 already established `.github/oz/config.yml` as the reviewable workflow config path. Response templating should extend that existing format rather than introducing another config file or reusing unrelated triage config. + +### Goals + +- Let repositories configure workflow-owned Oz response templates in `.github/oz/config.yml`. +- Store templates as YAML multi-line strings so full markdown fragments can be reviewed in pull requests. +- Reuse the existing consumer-repo-first config lookup model introduced by #338. +- Keep today’s comment behavior unchanged when the template section is absent. +- Support partial overrides so a repository can customize one or two templates without copying the full default set. +- Expose a stable, documented set of template keys and placeholder variables. +- Fail fast when configured templates are invalid instead of silently posting broken comments. + +### Non-goals + +- Templating arbitrary model-authored prose such as triage analyses, PR review summaries, or inline analytical replies. +- Adding loops, conditionals, or a general-purpose templating language. +- Introducing a second workflow config file or cross-file merging. +- Changing workflow decisions, labels, assignee behavior, or PR/review state transitions. +- Retrofitting localization, theme packs, or per-run environment-variable overrides for comment copy. + +### Figma / design references + +Figma: none provided. This is a workflow-config and comment-rendering feature. + +### User experience + +#### Config location and structure + +Response templates live in `.github/oz/config.yml`, the same versioned file introduced for shared workflow settings in #338. The existing lookup order remains unchanged: + +1. `.github/oz/config.yml` in the consuming repository +2. `.github/oz/config.yml` in the bundled `oz-for-oss` checkout + +Only the first discovered file is used. The workflow does not merge consumer and bundled config files. + +Within that file, response templates live under a dedicated top-level section. The initial shape should be: + +```yaml path=null start=null +version: 1 +self_improvement: + base_branch: auto +workflow_comments: + shared: + powered_by_suffix: | + _Powered by [Oz](https://oz.warp.dev)_ + create-spec-from-issue: + start_new: | + I'm drafting product and tech specs for this issue. + complete_created: | + I created a new [spec PR](${pr_url}) for this issue. +``` + +`workflow_comments` is optional. If it is absent, Oz keeps using its built-in default copy. + +#### Supported surfaces in the first release + +The first release should cover workflow-owned, deterministic response text that Oz constructs in Python before posting to GitHub. That includes: + +- progress-comment start, session-link, completion, and error copy for the core workflows +- helper-generated sections such as `Next steps`, spec-preview text, and the shared suffix appended to Oz-managed comments +- the triage wrapper text around agent-produced findings, follow-up questions, duplicate lists, maintainer-details framing, and the triage disclaimer +- one-off workflow-owned notices that are still hardcoded directly in entrypoint scripts, such as the unready-assignment message and PR-enforcement close comment + +This first release should not template the freeform prose generated by the model itself. For example, these remain model-authored outputs rather than config-authored templates: + +- `triage_result.json.summary` +- `triage_result.json.issue_body` +- `triage_result.json.statements` +- `triage_result.json.follow_up_questions` +- `issue_response.json.analysis_comment` +- `review.json.summary` and inline review comments + +Those model-authored payloads may still be inserted into configured templates through placeholders, but their underlying text is not managed in `.github/oz/config.yml`. + +#### Template behavior + +Each supported response surface has: + +- a stable template key +- a documented set of named placeholders +- a built-in default template that matches current behavior + +A repository may override any subset of keys. Missing keys fall back to the built-in default template for that surface. + +Template behavior rules: + +- Templates are markdown strings and may span multiple lines. +- Templates use named placeholders with `${name}` syntax. +- Only documented placeholders are allowed for a given template key. +- Template rendering happens only when Oz is about to post or update a workflow-owned comment. +- Changing `.github/oz/config.yml` affects future workflow runs only; existing comments are not retroactively rewritten. +- The HTML metadata marker that Oz uses to find/update its own comments remains workflow-owned and is not configurable. +- Template strings must be non-empty. Omitting a template key is the supported way to keep the default behavior. + +Representative placeholder examples include: + +- `${pr_url}` for created or updated PR links +- `${session_link_markdown}` for the formatted Warp session/conversation link +- `${reporter_mention}` for a preformatted `@user` mention when available +- `${questions_markdown}` or `${duplicate_list_markdown}` for dynamic markdown blocks already assembled by the workflow +- `${required_label}` or `${change_kind}` for enforcement messages + +#### Validation and failure behavior + +Configured templates should be validated before Oz posts any user-visible comment. Invalid config should fail the workflow with an error that points at `.github/oz/config.yml` and the offending template key. + +Validation should catch: + +- unsupported template keys +- wrong value types, such as non-string entries +- unknown placeholder names +- missing required placeholder values for a rendered template +- malformed placeholder syntax +- empty template strings + +A partially configured file should still work when the configured entries are valid: unconfigured template keys keep their built-in defaults. + +#### Documentation + +README documentation should explain: + +- where response templates live in `.github/oz/config.yml` +- that the config file itself still follows the consumer-first, bundled-fallback lookup order from #338 +- the `workflow_comments` section and key naming scheme +- the `${name}` placeholder syntax with concrete examples +- which response surfaces are supported in the first release and which are intentionally out of scope + +### Success criteria + +- A repository can customize at least one Oz-authored workflow comment surface by editing `.github/oz/config.yml` without modifying Python code. +- A repository can override only a small subset of templates and keep built-in defaults for the rest. +- Repositories with no `workflow_comments` section see no change in posted comments. +- Invalid configured templates fail fast with actionable errors instead of silently posting malformed comments. +- The first supported surface set covers the current hardcoded workflow-owned comment copy across shared helpers and the remaining one-off workflow scripts identified for this issue. +- README documentation is specific enough that a maintainer can successfully customize response text without reading the implementation. + +### Validation + +- Unit tests cover config parsing, template lookup, placeholder validation, partial overrides, and default fallback behavior. +- Unit tests cover representative rendered comments for progress-comment flows, triage comment wrappers, and one-off workflow notices. +- Regression tests confirm that existing default comment bodies remain unchanged when no template overrides are present. +- Manual verification in a fixture repository confirms that a YAML block-scalar override in `.github/oz/config.yml` changes the next Oz workflow comment on that surface. +- Manual verification confirms that an invalid placeholder name or template key fails the workflow before any user-visible comment is posted. + +### Open questions + +None for the first release. Model-authored prose remains intentionally out of scope unless a later issue expands this config surface. diff --git a/specs/GH347/tech.md b/specs/GH347/tech.md new file mode 100644 index 0000000..565950e --- /dev/null +++ b/specs/GH347/tech.md @@ -0,0 +1,211 @@ +# Issue #347: Support configurable template formats for Oz responses + +## Tech Spec + +### Problem + +The repository now has a shared workflow config file at `.github/oz/config.yml`, but response text is still hardcoded across helper functions and workflow entrypoints. As a result, repositories cannot customize Oz-authored workflow comments without editing Python, and the config format introduced in #338 stops short of one of the most visible workflow behaviors: the messages Oz posts back to issues and pull requests. + +The implementation needs to extend the existing config contract rather than inventing a second config path, while keeping current default comment text stable for repositories that do not opt into overrides. + +### Relevant code + +- `.github/scripts/oz_workflows/workflow_config.py:24` — resolves `.github/oz/config.yml` using the consumer-repo-first, bundled-fallback lookup model introduced in #338. +- `.github/scripts/oz_workflows/workflow_config.py:110` — parses and validates the current `self_improvement` section from that YAML file. +- `.github/scripts/oz_workflows/helpers.py:350` — current hardcoded triage start/session/spec/implementation/review comment-format helpers begin here. +- `.github/scripts/oz_workflows/helpers.py:576` — `build_comment_body()` appends the shared suffix and metadata marker to Oz-managed comments. +- `.github/scripts/oz_workflows/helpers.py:664` — `WorkflowProgressComment` owns create/update/replace behavior for progress comments. +- `.github/scripts/oz_workflows/helpers.py:1153` — `build_spec_preview_section()` returns user-visible helper text used in spec workflow comments. +- `.github/scripts/oz_workflows/helpers.py:1223` — `build_next_steps_section()` assembles shared follow-up copy. +- `.github/scripts/triage_new_issues.py:181` — `process_issue()` assembles the triage progress comment body. +- `.github/scripts/triage_new_issues.py:337` — `build_triage_prompt()` is unrelated to GitHub comment rendering and helps define what must remain out of scope. +- `.github/scripts/triage_new_issues.py:633` — `_record_triage_session_link()` swaps the Stage 2 triage message. +- `.github/scripts/triage_new_issues.py:700` — `build_statements_section()` renders reporter-facing triage findings. +- `.github/scripts/triage_new_issues.py:713` — `build_follow_up_section()` renders reporter follow-up-question framing. +- `.github/scripts/triage_new_issues.py:737` — `build_duplicate_section()` renders duplicate-detection framing. +- `.github/scripts/comment_on_unready_assigned_issue.py:25` — still posts hardcoded workflow-owned comment text directly. +- `.github/scripts/enforce_pr_issue_state.py:72` — builds workflow-owned close-comment text for unmatched or unready implementation PRs. +- `README.md:72-88` — documents the existing `.github/oz/config.yml` contract and is the right place to extend docs for `workflow_comments`. +- `.github/scripts/tests/test_workflow_config.py`, `.github/scripts/tests/test_comment_updates.py`, and `.github/scripts/tests/test_triage.py` — current test coverage for config loading and comment text assembly. + +### Current state + +Issue #338 added `.github/oz/config.yml` and a typed `SelfImprovementConfig`, but `workflow_config.py` still only exposes that single section. There is no generic typed loader for other workflow config domains yet. + +User-visible response strings currently live in two places: + +- shared helper functions in `oz_workflows/helpers.py` +- one-off literals inside workflow entrypoints such as `triage_new_issues.py`, `comment_on_unready_assigned_issue.py`, and `enforce_pr_issue_state.py` + +This means comment wording is only configurable by code changes. It also means there is no stable registry of response surfaces or placeholder names, so an implementer would currently have to audit literals by hand to find every supported comment string. + +Triage is the trickiest path because its final issue comment mixes deterministic workflow framing with model-authored content. The framing is hardcoded in Python today, while the substantive findings come from agent output. Any template system has to preserve that boundary rather than turning agent-authored prose into config-managed text. + +### Proposed changes + +#### 1. Refactor workflow config loading to expose reusable typed sections + +Extend `.github/scripts/oz_workflows/workflow_config.py` so it can parse the YAML file once and expose multiple typed sections from the same resolved config file. + +Proposed additions: + +```python path=null start=null +@dataclass(frozen=True) +class WorkflowConfigDocument: + path: Path + data: dict[str, Any] + +@dataclass(frozen=True) +class WorkflowCommentTemplateConfig: + overrides: dict[str, dict[str, str]] + +def load_workflow_config_document(workspace_root: Path) -> WorkflowConfigDocument: ... +def load_workflow_comment_templates(workspace_root: Path) -> WorkflowCommentTemplateConfig: ... +``` + +Implementation notes: + +- Keep `resolve_repo_config_path()` and `load_self_improvement_config()` as stable public APIs, but implement them on top of the new raw-document loader so YAML parsing and version checks stay centralized. +- Continue using the existing single-file resolution behavior from #338: the consuming repo config wins if present, otherwise the bundled fallback is used, and the two files are never merged. +- Validate `workflow_comments` as a mapping of namespace → template-key → non-empty string. +- Unknown top-level sections outside the loaders that care about them can continue to exist, but unknown workflow namespaces or template keys inside `workflow_comments` should fail fast so typos do not silently fall through. +- Cache the resolved/parsed config per process (for example with `functools.lru_cache`) so repeated helper renders in one workflow run do not reread YAML. + +#### 2. Add a centralized template registry and renderer + +Add a new shared module, for example `.github/scripts/oz_workflows/comment_templates.py`, that owns: + +- the stable catalog of supported template IDs +- the built-in default text for each template +- the allowed placeholders for each template +- rendering logic for defaults or configured overrides + +Proposed shape: + +```python path=null start=null +@dataclass(frozen=True) +class TemplateDefinition: + namespace: str + key: str + default_template: str + allowed_placeholders: frozenset[str] + +def render_comment_template( + workspace_root: Path, + *, + namespace: str, + key: str, + context: Mapping[str, str], +) -> str: ... +``` + +Design choices: + +- Use `${name}` placeholders backed by Python’s `string.Template`. This keeps template syntax simple in YAML block scalars and avoids the brace-escaping burden that `str.format()` would impose on markdown-heavy comment text. +- Validate configured overrides before rendering: + - the namespace/key pair must exist in the registry + - every placeholder referenced in the configured template must be in that template’s allowlist + - every placeholder referenced by the template must be present in the supplied context +- Treat template strings as pure substitution, not code. There is no expression language, no conditionals, and no evaluation beyond named placeholder replacement. +- Store defaults in the registry using the exact current strings so repositories without overrides see no behavior change. + +#### 3. Route helper-owned comment text through the registry + +Update the formatting helpers in `.github/scripts/oz_workflows/helpers.py` so they no longer return hardcoded literals directly. Instead, each helper becomes a thin wrapper over `render_comment_template(...)`. + +Examples: + +- `format_triage_start_line()` selects `workflow_comments.triage-new-issues.start_new` or `...start_retriage` +- `format_triage_session_line()` renders `...session` with `${session_link_markdown}` +- `format_spec_start_line()` and `format_spec_complete_line()` map new-vs-update variants to separate template keys +- implementation/review/respond/enforce start-line helpers map each current variant to a dedicated key instead of requiring conditional syntax inside templates +- `build_next_steps_section()` renders a shared heading template plus the already-built bullet list +- `build_spec_preview_section()` renders its intro text via a shared template while keeping generated links in placeholders + +Move `POWERED_BY_SUFFIX` from a hardcoded constant to a `workflow_comments.shared.powered_by_suffix` template. The metadata marker itself must remain hardcoded and non-configurable. + +#### 4. Convert workflow-specific inline comment builders + +Some user-visible text still lives directly in workflow entrypoints and must be pulled onto the new registry so the feature covers the actual hardcoded surfaces called out in issue triage. + +Target files: + +- `triage_new_issues.py` + - keep the current control flow and section ordering in Python + - replace hardcoded wrapper text in `build_statements_section()`, `build_follow_up_section()`, `build_duplicate_section()`, the maintainer-details wrapper, and the triage disclaimer with template renders + - pass already-rendered markdown blocks such as the numbered question list or duplicate bullet list through placeholders like `${questions_markdown}` and `${duplicate_list_markdown}` +- `comment_on_unready_assigned_issue.py` + - render the readiness-check start and completion comments through the template registry rather than inline literals +- `enforce_pr_issue_state.py` + - render the explicit-associated-issue close comment and the unmatched-ready-issue close comment through templates while keeping issue matching and PR closing logic unchanged + +Any additional workflow-owned GitHub comment literals found during implementation should be moved onto the same registry rather than left as one-off strings. + +#### 5. Keep model-authored content out of the registry + +Do not move agent-generated payloads such as: + +- `triage_result.json.issue_body` +- `triage_result.json.statements` +- `triage_result.json.follow_up_questions[*].question` +- `issue_response.json.analysis_comment` +- `review.json.summary` + +Those values remain model-authored data that the workflow may wrap with configured template framing. This keeps the config surface deterministic and avoids coupling repository config to the shape or style of LLM-authored prose. + +#### 6. Update README and examples + +Extend `README.md` near the existing `.github/oz/config.yml` section with: + +- the `workflow_comments` schema +- a small example override using YAML block scalars +- a note that missing template keys fall back to built-in defaults +- a note that template validation is strict and will fail on unknown keys or placeholders +- a short statement that model-authored prose is intentionally out of scope + +### End-to-end flow + +1. A workflow entrypoint calls a helper or local section builder to produce user-visible comment text. +2. That helper requests a stable template ID from the registry with a context dict. +3. The renderer resolves `.github/oz/config.yml` using the existing consumer-first, bundled-fallback logic from `workflow_config.py`. +4. The renderer loads `workflow_comments` from that single resolved file, validates any override for the requested namespace/key, and renders it with `${name}` substitution. +5. If no override exists, the renderer uses the built-in default template from the registry. +6. The caller assembles the final GitHub comment body, and `WorkflowProgressComment` posts or updates it with the fixed metadata marker and other non-configurable workflow bookkeeping. + +This preserves the current single-source config model from #338 while still allowing partial template overrides within the chosen config file. + +### Risks and mitigations + +- Template sprawl could make the registry hard to maintain. + - Mitigation: key template IDs to existing workflow names and helper surfaces instead of inventing ad hoc names, and document a naming convention in the new module. +- Invalid placeholders or malformed markdown could break user-visible comments. + - Mitigation: strict validation before posting, with errors that include the config path and the failing namespace/key. +- Rendering every fragment by reparsing YAML could add unnecessary overhead. + - Mitigation: cache the parsed workflow config and template overrides for the lifetime of each script process. +- Triage mixes deterministic framing and model-authored content. + - Mitigation: keep flow control and content assembly in Python, and pass only already-rendered dynamic markdown blocks into configured wrapper templates. +- Regressions could be subtle because many workflows share the same helper layer. + - Mitigation: preserve existing default strings in the registry and add representative exact-output tests for default-rendered comments. + +### Testing and validation + +- Extend `.github/scripts/tests/test_workflow_config.py` with cases for: + - valid `workflow_comments` parsing + - rejection of unknown namespace/key pairs + - rejection of empty template strings + - rejection of unknown placeholders + - default fallback when `workflow_comments` is absent +- Extend `.github/scripts/tests/test_comment_updates.py` to cover: + - default-rendered progress-comment strings + - configured overrides for representative helper-owned templates + - shared suffix rendering if moved into the registry +- Extend `.github/scripts/tests/test_triage.py` to cover overridden statements/follow-up/duplicate/disclaimer wrappers without changing the underlying model-authored content +- Add or extend tests for: + - `comment_on_unready_assigned_issue.py` + - `enforce_pr_issue_state.py` +- Run `env PYTHONPATH=.github/scripts python -m unittest discover -s .github/scripts/tests`. + +### Follow-ups + +- Consider reusing the same registry for other deterministic markdown surfaces, such as generated PR descriptions, only if a later issue shows real demand. +- If maintainers want broader branding or localization controls later, add them as explicit follow-up config keys rather than generalizing this feature into a full template engine. From 0940583eef07189d1603269d2597511ccdb9fed8 Mon Sep 17 00:00:00 2001 From: Oz Date: Sun, 26 Apr 2026 19:20:25 +0000 Subject: [PATCH 2/4] spec: refine configurable comment template specs Co-Authored-By: Safia Abdalla --- specs/GH347/product.md | 17 ++++++--- specs/GH347/tech.md | 79 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/specs/GH347/product.md b/specs/GH347/product.md index 70f1119..fdc7cc7 100644 --- a/specs/GH347/product.md +++ b/specs/GH347/product.md @@ -28,6 +28,7 @@ Issue #338 already established `.github/oz/config.yml` as the reviewable workflo - Adding loops, conditionals, or a general-purpose templating language. - Introducing a second workflow config file or cross-file merging. - Changing workflow decisions, labels, assignee behavior, or PR/review state transitions. +- Making the shared `_Powered by [Oz](https://oz.warp.dev)_` suffix configurable. - Retrofitting localization, theme packs, or per-run environment-variable overrides for comment copy. ### Figma / design references @@ -52,15 +53,19 @@ version: 1 self_improvement: base_branch: auto workflow_comments: - shared: - powered_by_suffix: | - _Powered by [Oz](https://oz.warp.dev)_ create-spec-from-issue: start_new: | I'm drafting product and tech specs for this issue. complete_created: | I created a new [spec PR](${pr_url}) for this issue. + ${spec_preview_markdown} + ${next_steps_markdown} + triage-new-issues: + follow_up_with_reporter: | + ${reporter_mention} — I have a few follow-up questions before I can narrow this down: + ${questions_markdown} ``` +This example shows how configured markdown can interpolate workflow-provided placeholders such as `${pr_url}`, `${spec_preview_markdown}`, `${next_steps_markdown}`, `${reporter_mention}`, and `${questions_markdown}` directly inside the YAML block scalar. `workflow_comments` is optional. If it is absent, Oz keeps using its built-in default copy. @@ -69,10 +74,12 @@ workflow_comments: The first release should cover workflow-owned, deterministic response text that Oz constructs in Python before posting to GitHub. That includes: - progress-comment start, session-link, completion, and error copy for the core workflows -- helper-generated sections such as `Next steps`, spec-preview text, and the shared suffix appended to Oz-managed comments +- helper-generated sections such as full `Next steps` blocks and spec-preview text - the triage wrapper text around agent-produced findings, follow-up questions, duplicate lists, maintainer-details framing, and the triage disclaimer - one-off workflow-owned notices that are still hardcoded directly in entrypoint scripts, such as the unready-assignment message and PR-enforcement close comment +The shared `_Powered by [Oz](https://oz.warp.dev)_` suffix and the HTML metadata marker remain workflow-owned constants in the first release and are not configurable through `.github/oz/config.yml`. + This first release should not template the freeform prose generated by the model itself. For example, these remain model-authored outputs rather than config-authored templates: - `triage_result.json.summary` @@ -109,6 +116,8 @@ Representative placeholder examples include: - `${pr_url}` for created or updated PR links - `${session_link_markdown}` for the formatted Warp session/conversation link - `${reporter_mention}` for a preformatted `@user` mention when available +- `${spec_preview_markdown}` for the generated spec-preview block +- `${next_steps_markdown}` for the full rendered next-steps block - `${questions_markdown}` or `${duplicate_list_markdown}` for dynamic markdown blocks already assembled by the workflow - `${required_label}` or `${change_kind}` for enforcement messages diff --git a/specs/GH347/tech.md b/specs/GH347/tech.md index 565950e..77e5218 100644 --- a/specs/GH347/tech.md +++ b/specs/GH347/tech.md @@ -109,6 +109,73 @@ Design choices: - Treat template strings as pure substitution, not code. There is no expression language, no conditionals, and no evaluation beyond named placeholder replacement. - Store defaults in the registry using the exact current strings so repositories without overrides see no behavior change. +#### 2a. Enumerate the first-release template registry + +The first release should explicitly register every deterministic user-visible string that is currently hardcoded in helpers or workflow entrypoints, rather than leaving implementers to rediscover them during coding. The initial namespace/key set should be documented in the new module and reflected in README examples. + +Proposed namespace and key coverage: + +- `shared` + - `progress_session_session` + - `progress_session_conversation` + - `spec_preview` + - `next_steps_section` +- `triage-new-issues` + - `start_new` + - `start_retriage` + - `session` + - `complete_without_user_facing_content_with_session` + - `complete_without_user_facing_content_without_session` + - `session_link_only` + - `statements_with_reporter` + - `statements_without_reporter` + - `follow_up_with_reporter` + - `follow_up_without_reporter` + - `duplicate_with_reporter` + - `duplicate_without_reporter` + - `maintainer_details` + - `disclaimer` +- `respond-to-triaged-issue` + - `start` +- `create-spec-from-issue` + - `start_new` + - `start_update` + - `complete_created` + - `complete_updated` +- `create-implementation-from-issue` + - `start_blocked_unapproved_specs` + - `start_from_approved_spec_new_pr` + - `start_from_approved_spec_update_pr` + - `start_from_directory_specs_new_pr` + - `start_from_directory_specs_update_pr` + - `start_without_spec_context_new_pr` + - `start_without_spec_context_update_pr` + - `complete_updated_spec_pr` + - `complete_updated_existing_draft_pr` + - `complete_created_new_draft_pr` +- `review-pull-request` + - `start_first_review_code` + - `start_first_review_spec` + - `start_rereview_code` + - `start_rereview_spec` +- `respond-to-pr-comment` + - `start_review_reply_with_spec_context` + - `start_review_reply_without_spec_context` + - `start_review_body_with_spec_context` + - `start_review_body_without_spec_context` + - `start_conversation_comment_with_spec_context` + - `start_conversation_comment_without_spec_context` +- `comment-on-unready-assigned-issue` + - `start` + - `complete` +- `enforce-pr-issue-state` + - `start_explicit_issue` + - `start_matching_ready_issue` + - `close_explicit_issue_not_ready` + - `close_no_matching_ready_issue` + +The corresponding placeholder set should be called out alongside those definitions. Important examples include `${session_link_markdown}`, `${pr_url}`, `${reporter_mention}`, `${statements_markdown}`, `${questions_markdown}`, `${duplicate_list_markdown}`, `${maintainer_details_markdown}`, `${next_steps_markdown}`, `${required_label}`, `${change_kind}`, `${contribution_docs_url}`, and `${association_rationale}`. + #### 3. Route helper-owned comment text through the registry Update the formatting helpers in `.github/scripts/oz_workflows/helpers.py` so they no longer return hardcoded literals directly. Instead, each helper becomes a thin wrapper over `render_comment_template(...)`. @@ -119,10 +186,10 @@ Examples: - `format_triage_session_line()` renders `...session` with `${session_link_markdown}` - `format_spec_start_line()` and `format_spec_complete_line()` map new-vs-update variants to separate template keys - implementation/review/respond/enforce start-line helpers map each current variant to a dedicated key instead of requiring conditional syntax inside templates -- `build_next_steps_section()` renders a shared heading template plus the already-built bullet list +- `build_next_steps_section()` renders a whole-section template such as `shared.next_steps_section`, with `${next_steps_markdown}` containing the rendered bullet list so the heading, surrounding copy, and list placement are all editable - `build_spec_preview_section()` renders its intro text via a shared template while keeping generated links in placeholders -Move `POWERED_BY_SUFFIX` from a hardcoded constant to a `workflow_comments.shared.powered_by_suffix` template. The metadata marker itself must remain hardcoded and non-configurable. +Leave `POWERED_BY_SUFFIX` as a hardcoded constant in `helpers.py` and `build_pr_body()`. The HTML metadata marker also remains hardcoded and non-configurable. #### 4. Convert workflow-specific inline comment builders @@ -137,7 +204,8 @@ Target files: - `comment_on_unready_assigned_issue.py` - render the readiness-check start and completion comments through the template registry rather than inline literals - `enforce_pr_issue_state.py` - - render the explicit-associated-issue close comment and the unmatched-ready-issue close comment through templates while keeping issue matching and PR closing logic unchanged + - render the explicit-associated-issue close comment through the template registry + - stop asking the association agent to author the no-match close comment body; instead, keep the agent focused on structured match/no-match output plus rationale and let Python render the unmatched-ready-issue close comment through a template with placeholders like `${change_kind}`, `${required_label}`, `${contribution_docs_url}`, and `${association_rationale}` Any additional workflow-owned GitHub comment literals found during implementation should be moved onto the same registry rather than left as one-off strings. @@ -150,6 +218,7 @@ Do not move agent-generated payloads such as: - `triage_result.json.follow_up_questions[*].question` - `issue_response.json.analysis_comment` - `review.json.summary` +- `issue_association.json.rationale` Those values remain model-authored data that the workflow may wrap with configured template framing. This keeps the config surface deterministic and avoids coupling repository config to the shape or style of LLM-authored prose. @@ -198,11 +267,11 @@ This preserves the current single-source config model from #338 while still allo - Extend `.github/scripts/tests/test_comment_updates.py` to cover: - default-rendered progress-comment strings - configured overrides for representative helper-owned templates - - shared suffix rendering if moved into the registry + - the hardcoded suffix continuing to remain unchanged outside the registry - Extend `.github/scripts/tests/test_triage.py` to cover overridden statements/follow-up/duplicate/disclaimer wrappers without changing the underlying model-authored content - Add or extend tests for: - `comment_on_unready_assigned_issue.py` - - `enforce_pr_issue_state.py` + - `enforce_pr_issue_state.py`, including the templated no-match close comment rendered from agent-supplied rationale - Run `env PYTHONPATH=.github/scripts python -m unittest discover -s .github/scripts/tests`. ### Follow-ups From 0ef5f57346c4ef413ff18915fb099c80d9701a32 Mon Sep 17 00:00:00 2001 From: Oz Date: Sun, 26 Apr 2026 21:22:39 +0000 Subject: [PATCH 3/4] feat: add configurable workflow comment templates Co-Authored-By: Safia Abdalla --- .../comment_on_unready_assigned_issue.py | 13 +- .github/scripts/enforce_pr_issue_state.py | 39 +- .../scripts/oz_workflows/comment_templates.py | 557 ++++++++++++++++++ .github/scripts/oz_workflows/helpers.py | 250 +++++--- .../scripts/oz_workflows/workflow_config.py | 38 +- .../test_comment_on_unready_assigned_issue.py | 62 ++ .../scripts/tests/test_comment_templates.py | 211 +++++++ .../tests/test_enforce_pr_issue_state.py | 138 +++++ .github/scripts/tests/test_triage.py | 53 ++ .github/scripts/triage_new_issues.py | 115 ++-- README.md | 13 +- 11 files changed, 1334 insertions(+), 155 deletions(-) create mode 100644 .github/scripts/oz_workflows/comment_templates.py create mode 100644 .github/scripts/tests/test_comment_templates.py diff --git a/.github/scripts/comment_on_unready_assigned_issue.py b/.github/scripts/comment_on_unready_assigned_issue.py index 42ca28a..47e7cb8 100644 --- a/.github/scripts/comment_on_unready_assigned_issue.py +++ b/.github/scripts/comment_on_unready_assigned_issue.py @@ -3,6 +3,7 @@ from typing import Any, Mapping from github import Auth, Github +from oz_workflows.comment_templates import render_comment_template from oz_workflows.env import load_event, repo_parts, repo_slug, require_env from oz_workflows.helpers import WorkflowProgressComment @@ -38,9 +39,17 @@ def main() -> None: workflow="comment-on-unready-assigned-issue", event_payload=event, ) - progress.start("I'm checking whether this assignment is ready for work.") + progress.start( + render_comment_template( + namespace="comment-on-unready-assigned-issue", + key="start", + ) + ) progress.complete( - "This issue is assigned to me, but it is not labeled `ready-to-spec` or `ready-to-implement`, so there is no work to do yet.", + render_comment_template( + namespace="comment-on-unready-assigned-issue", + key="complete", + ), ) issue.remove_from_assignees(assignee_login) diff --git a/.github/scripts/enforce_pr_issue_state.py b/.github/scripts/enforce_pr_issue_state.py index 1d679c3..93f433f 100644 --- a/.github/scripts/enforce_pr_issue_state.py +++ b/.github/scripts/enforce_pr_issue_state.py @@ -6,6 +6,7 @@ from github import Auth, Github from oz_workflows.actions import set_output +from oz_workflows.comment_templates import render_comment_template from oz_workflows.env import optional_env, repo_parts, repo_slug, require_env, workspace from oz_workflows.helpers import ( format_enforce_start_line, @@ -60,8 +61,8 @@ def build_issue_association_prompt( Output requirements: - Decide whether there is a clear match. - Produce JSON with exactly this shape: - {{"matched": boolean, "issue_number": number | null, "rationale": string, "close_comment": string}} - - If there is no clear match, set `close_comment` to a concise PR comment explaining that this {change_kind} PR could not be matched to an issue marked `{required_label}` and include this contribution docs link: {contribution_docs_url} + {{"matched": boolean, "issue_number": number | null, "rationale": string}} + - When there is no clear match, explain why in `rationale` using concrete evidence from the PR and candidate issues. - Do not close the PR yourself. - Validate the JSON with `jq`. - After validating the JSON, upload it as an artifact via `oz artifact upload issue_association.json` (or `oz-preview artifact upload issue_association.json` if the `oz` CLI is not available). Either CLI is acceptable — use whichever one is installed in the environment. The subcommand is `artifact` (singular) on both CLIs; do not use `artifacts`. @@ -138,12 +139,17 @@ def main() -> None: ) issue_refs = ", ".join(f"#{n}" for n in associated_issue_numbers) association_noun = "issue" if len(associated_issue_numbers) == 1 else "issues" - close_comment = ( - f"The PR that you've opened seems to contain {change_kind} changes and is associated with issue " - f"{issue_refs}, but none of those associated {association_noun} are marked as " - f"`{required_label}`. This PR will be " - f"automatically closed. Please see our [contribution docs]({contribution_docs_url}) for guidance " - "on when changes are accepted for issues." + close_comment = render_comment_template( + workspace(), + namespace="enforce-pr-issue-state", + key="close_explicit_issue_not_ready", + context={ + "change_kind": change_kind, + "issue_refs": issue_refs, + "associated_issue_noun": association_noun, + "required_label": required_label, + "contribution_docs_url": contribution_docs_url, + }, ) progress.complete(close_comment) pr.edit(state="closed") @@ -204,9 +210,20 @@ def main() -> None: progress.cleanup() set_output("allow_review", "true") return - close_comment = str(result.get("close_comment") or "").strip() - if not close_comment: - raise RuntimeError("Oz returned no issue match without a close_comment") + association_rationale = str(result.get("rationale") or "").strip() + if not association_rationale: + raise RuntimeError("Oz returned no issue match without a rationale") + close_comment = render_comment_template( + workspace(), + namespace="enforce-pr-issue-state", + key="close_no_matching_ready_issue", + context={ + "change_kind": change_kind, + "required_label": required_label, + "contribution_docs_url": contribution_docs_url, + "association_rationale": association_rationale, + }, + ) final_sections = [close_comment] if session_links: final_sections.append(f"Session: [view on Warp]({session_links[-1]})") diff --git a/.github/scripts/oz_workflows/comment_templates.py b/.github/scripts/oz_workflows/comment_templates.py new file mode 100644 index 0000000..8aa7195 --- /dev/null +++ b/.github/scripts/oz_workflows/comment_templates.py @@ -0,0 +1,557 @@ +from __future__ import annotations + +import re +from dataclasses import dataclass +from functools import lru_cache +from pathlib import Path +from string import Template +from typing import Mapping + +from .env import workspace +from .workflow_config import load_workflow_config_document + + +_PLACEHOLDER_PATTERN = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}") + + +@dataclass(frozen=True) +class TemplateDefinition: + namespace: str + key: str + default_template: str + allowed_placeholders: frozenset[str] + + +@dataclass(frozen=True) +class WorkflowCommentTemplateConfig: + overrides: dict[str, dict[str, str]] + + +def _definition( + namespace: str, + key: str, + default_template: str, + *, + allowed_placeholders: tuple[str, ...] = (), +) -> TemplateDefinition: + return TemplateDefinition( + namespace=namespace, + key=key, + default_template=default_template, + allowed_placeholders=frozenset(allowed_placeholders), + ) + + +_TEMPLATE_DEFINITIONS: tuple[TemplateDefinition, ...] = ( + _definition( + "shared", + "progress_session_session", + "You can follow along in ${session_link_markdown}.", + allowed_placeholders=("session_link_markdown",), + ), + _definition( + "shared", + "progress_session_conversation", + "You can view ${session_link_markdown}.", + allowed_placeholders=("session_link_markdown",), + ), + _definition( + "shared", + "spec_preview", + "Preview generated specs:\n" + "- Product spec: [${product_path}](${product_url})\n" + "- Tech spec: [${tech_path}](${tech_url})", + allowed_placeholders=("product_path", "product_url", "tech_path", "tech_url"), + ), + _definition( + "shared", + "next_steps_section", + "Next steps:\n${next_steps_markdown}", + allowed_placeholders=("next_steps_markdown",), + ), + _definition( + "shared", + "error_with_run_link", + "I ran into an unexpected error while working on this. " + "You can view [the workflow run](${workflow_run_url}) for more details.", + allowed_placeholders=("workflow_run_url",), + ), + _definition( + "shared", + "error_without_run_link", + "I ran into an unexpected error while working on this.", + ), + _definition( + "triage-new-issues", + "start_new", + "I'm starting to work on triaging this issue.", + ), + _definition( + "triage-new-issues", + "start_retriage", + "I'm re-triaging this issue based on new information.", + ), + _definition( + "triage-new-issues", + "session", + "I'm ${triage_verb} this issue. You can follow ${session_link_markdown}.", + allowed_placeholders=("session_link_markdown", "triage_verb"), + ), + _definition( + "triage-new-issues", + "complete_without_user_facing_content_with_session", + "I've finished triaging this issue. " + "A maintainer will verify the details shortly. " + "You can view ${session_link_markdown}.", + allowed_placeholders=("session_link_markdown",), + ), + _definition( + "triage-new-issues", + "complete_without_user_facing_content_without_session", + "I've completed the triage of this issue.", + ), + _definition( + "triage-new-issues", + "session_link_only", + "You can view ${session_link_markdown}.", + allowed_placeholders=("session_link_markdown",), + ), + _definition( + "triage-new-issues", + "statements_with_reporter", + "${reporter_mention} — here's what I found while triaging this issue:\n\n${statements_markdown}", + allowed_placeholders=("reporter_mention", "statements_markdown"), + ), + _definition( + "triage-new-issues", + "statements_without_reporter", + "Here's what I found while triaging this issue:\n\n${statements_markdown}", + allowed_placeholders=("statements_markdown",), + ), + _definition( + "triage-new-issues", + "follow_up_with_reporter", + "${reporter_mention} — I have a few follow-up questions before I can narrow this down:\n\n" + "${questions_markdown}\n\n" + "Reply in-thread with those details and the triage workflow will " + "automatically re-evaluate the issue and update the diagnosis, " + "labels, and next steps.", + allowed_placeholders=("reporter_mention", "questions_markdown"), + ), + _definition( + "triage-new-issues", + "follow_up_without_reporter", + "I have a few follow-up questions before I can narrow this down:\n\n" + "${questions_markdown}\n\n" + "Reply in-thread with those details and the triage workflow will " + "automatically re-evaluate the issue and update the diagnosis, " + "labels, and next steps.", + allowed_placeholders=("questions_markdown",), + ), + _definition( + "triage-new-issues", + "duplicate_with_reporter", + "${reporter_mention} — this issue appears to overlap with existing issues:\n\n" + "${duplicate_list_markdown}\n\n" + "If this report is meaningfully different, please comment with the " + "additional context or distinguishing behavior so a maintainer can " + "review it. Otherwise, a maintainer may close it as a duplicate after review.", + allowed_placeholders=("reporter_mention", "duplicate_list_markdown"), + ), + _definition( + "triage-new-issues", + "duplicate_without_reporter", + "This issue appears to overlap with existing issues:\n\n" + "${duplicate_list_markdown}\n\n" + "If this report is meaningfully different, please comment with the " + "additional context or distinguishing behavior so a maintainer can " + "review it. Otherwise, a maintainer may close it as a duplicate after review.", + allowed_placeholders=("duplicate_list_markdown",), + ), + _definition( + "triage-new-issues", + "maintainer_details", + "
\nMaintainer details\n\n${maintainer_details_markdown}\n\n
", + allowed_placeholders=("maintainer_details_markdown",), + ), + _definition( + "triage-new-issues", + "disclaimer", + "*This is my automated analysis and may be incorrect. A maintainer will verify the details.*", + ), + _definition( + "respond-to-triaged-issue", + "start", + "I'm drafting an inline response to this comment. " + "This issue is already triaged, so I'll reply without changing labels, " + "the issue body, or assignees.", + ), + _definition( + "create-spec-from-issue", + "start_new", + "I'm starting work on product and tech specs for this issue.", + ), + _definition( + "create-spec-from-issue", + "start_update", + "I'm updating the existing spec PR for this issue.", + ), + _definition( + "create-spec-from-issue", + "complete_created", + "I created a new [spec PR](${pr_url}) for this issue.", + allowed_placeholders=("pr_url",), + ), + _definition( + "create-spec-from-issue", + "complete_updated", + "I updated the existing [spec PR](${pr_url}) for this issue.", + allowed_placeholders=("pr_url",), + ), + _definition( + "create-implementation-from-issue", + "start_blocked_unapproved_specs", + "I'm not starting implementation because the linked spec PR(s) " + "have not been marked `plan-approved`${linked_spec_prs_suffix}.", + allowed_placeholders=("linked_spec_prs_suffix",), + ), + _definition( + "create-implementation-from-issue", + "start_from_approved_spec_new_pr", + "I'm implementing this issue on top of the approved spec PR's branch.", + ), + _definition( + "create-implementation-from-issue", + "start_from_approved_spec_update_pr", + "I'm implementing this issue on top of the approved spec PR's branch (updating the existing draft PR).", + ), + _definition( + "create-implementation-from-issue", + "start_from_directory_specs_new_pr", + "I'm implementing this issue using the repository's directory specs.", + ), + _definition( + "create-implementation-from-issue", + "start_from_directory_specs_update_pr", + "I'm implementing this issue using the repository's directory specs (updating the existing draft PR).", + ), + _definition( + "create-implementation-from-issue", + "start_without_spec_context_new_pr", + "I'm implementing this issue with no spec context.", + ), + _definition( + "create-implementation-from-issue", + "start_without_spec_context_update_pr", + "I'm implementing this issue with no spec context (updating the existing draft PR).", + ), + _definition( + "create-implementation-from-issue", + "complete_updated_spec_pr", + "I pushed implementation updates to the linked approved [spec PR](${pr_url}).", + allowed_placeholders=("pr_url",), + ), + _definition( + "create-implementation-from-issue", + "complete_updated_existing_draft_pr", + "I updated the existing draft [implementation PR](${pr_url}) for this issue.", + allowed_placeholders=("pr_url",), + ), + _definition( + "create-implementation-from-issue", + "complete_created_new_draft_pr", + "I created a new draft [implementation PR](${pr_url}) for this issue.", + allowed_placeholders=("pr_url",), + ), + _definition( + "review-pull-request", + "start_first_review_code", + "I'm starting a first review of this pull request.${focus_suffix}", + allowed_placeholders=("focus_suffix",), + ), + _definition( + "review-pull-request", + "start_first_review_spec", + "I'm starting a first review of this spec-only pull request.${focus_suffix}", + allowed_placeholders=("focus_suffix",), + ), + _definition( + "review-pull-request", + "start_rereview_code", + "I'm re-reviewing this pull request in response to a review request.${focus_suffix}", + allowed_placeholders=("focus_suffix",), + ), + _definition( + "review-pull-request", + "start_rereview_spec", + "I'm re-reviewing this spec-only pull request in response to a review request.${focus_suffix}", + allowed_placeholders=("focus_suffix",), + ), + _definition( + "respond-to-pr-comment", + "start_review_reply_with_spec_context", + "I'm working on changes requested in this PR (responding to an inline review-thread comment). " + "Spec context was found and will be used to ground the change.", + ), + _definition( + "respond-to-pr-comment", + "start_review_reply_without_spec_context", + "I'm working on changes requested in this PR (responding to an inline review-thread comment).", + ), + _definition( + "respond-to-pr-comment", + "start_review_body_with_spec_context", + "I'm working on changes requested in this PR (responding to a PR review body). " + "Spec context was found and will be used to ground the change.", + ), + _definition( + "respond-to-pr-comment", + "start_review_body_without_spec_context", + "I'm working on changes requested in this PR (responding to a PR review body).", + ), + _definition( + "respond-to-pr-comment", + "start_conversation_comment_with_spec_context", + "I'm working on changes requested in this PR (responding to a PR conversation comment). " + "Spec context was found and will be used to ground the change.", + ), + _definition( + "respond-to-pr-comment", + "start_conversation_comment_without_spec_context", + "I'm working on changes requested in this PR (responding to a PR conversation comment).", + ), + _definition( + "comment-on-unready-assigned-issue", + "start", + "I'm checking whether this assignment is ready for work.", + ), + _definition( + "comment-on-unready-assigned-issue", + "complete", + "This issue is assigned to me, but it is not labeled `ready-to-spec` or `ready-to-implement`, so there is no work to do yet.", + ), + _definition( + "enforce-pr-issue-state", + "start_explicit_issue", + "I'm checking this ${change_kind} PR for association with an explicitly linked issue.", + allowed_placeholders=("change_kind",), + ), + _definition( + "enforce-pr-issue-state", + "start_matching_ready_issue", + "I'm checking this ${change_kind} PR for association with a likely matching ready issue.", + allowed_placeholders=("change_kind",), + ), + _definition( + "enforce-pr-issue-state", + "close_explicit_issue_not_ready", + "The PR that you've opened seems to contain ${change_kind} changes and is associated with issue " + "${issue_refs}, but none of those associated ${associated_issue_noun} are marked as " + "`${required_label}`. This PR will be automatically closed. Please see our " + "[contribution docs](${contribution_docs_url}) for guidance on when changes are accepted for issues.", + allowed_placeholders=( + "change_kind", + "issue_refs", + "associated_issue_noun", + "required_label", + "contribution_docs_url", + ), + ), + _definition( + "enforce-pr-issue-state", + "close_no_matching_ready_issue", + "I couldn't confidently match this ${change_kind} PR to an issue marked `${required_label}`, " + "so this PR will be automatically closed.\n\n" + "Rationale: ${association_rationale}\n\n" + "Please see our [contribution docs](${contribution_docs_url}) for guidance on when changes are accepted for issues.", + allowed_placeholders=( + "change_kind", + "required_label", + "association_rationale", + "contribution_docs_url", + ), + ), +) + +_TEMPLATE_REGISTRY: dict[tuple[str, str], TemplateDefinition] = { + (definition.namespace, definition.key): definition + for definition in _TEMPLATE_DEFINITIONS +} +_KNOWN_NAMESPACES = {definition.namespace for definition in _TEMPLATE_DEFINITIONS} + + +def _fail(config_path: Path, message: str) -> RuntimeError: + return RuntimeError(f"{config_path}: {message}") + + +def _template_source(namespace: str, key: str) -> str: + return f"workflow_comments.{namespace}.{key}" + + +def _extract_placeholders( + template_text: str, + *, + config_path: Path | None = None, + namespace: str = "", + key: str = "", +) -> set[str]: + placeholders: set[str] = set() + index = 0 + while index < len(template_text): + if template_text[index] != "$": + index += 1 + continue + if index + 1 < len(template_text) and template_text[index + 1] == "$": + index += 2 + continue + match = _PLACEHOLDER_PATTERN.match(template_text, index) + if match is not None: + placeholders.add(match.group(1)) + index = match.end() + continue + if config_path is not None: + raise _fail( + config_path, + f"Invalid placeholder syntax in {_template_source(namespace, key)}. " + "Use ${name} placeholders.", + ) + raise RuntimeError( + f"Invalid placeholder syntax in default workflow comment template " + f"{namespace}.{key}. Use ${name} placeholders." + ) + return placeholders + + +def _validate_template_definition(definition: TemplateDefinition) -> None: + placeholders = _extract_placeholders( + definition.default_template, + namespace=definition.namespace, + key=definition.key, + ) + unknown_placeholders = sorted(placeholders - definition.allowed_placeholders) + if unknown_placeholders: + joined = ", ".join(unknown_placeholders) + raise RuntimeError( + f"Default workflow comment template {definition.namespace}.{definition.key} " + f"references unsupported placeholders: {joined}." + ) + + +for _definition_item in _TEMPLATE_DEFINITIONS: + _validate_template_definition(_definition_item) + + +def get_template_definition(namespace: str, key: str) -> TemplateDefinition: + definition = _TEMPLATE_REGISTRY.get((namespace, key)) + if definition is None: + raise RuntimeError(f"Unknown workflow comment template: {namespace}.{key}") + return definition + + +@lru_cache(maxsize=None) +def _load_workflow_comment_template_config_cached( + workspace_root: Path, +) -> WorkflowCommentTemplateConfig: + document = load_workflow_config_document(workspace_root) + config_path = document.path + raw_section = document.data.get("workflow_comments") + if raw_section is None: + return WorkflowCommentTemplateConfig(overrides={}) + if not isinstance(raw_section, dict): + raise _fail(config_path, "workflow_comments must be a YAML mapping.") + + overrides: dict[str, dict[str, str]] = {} + for raw_namespace, raw_templates in raw_section.items(): + if not isinstance(raw_namespace, str) or not raw_namespace.strip(): + raise _fail(config_path, "workflow_comments keys must be non-empty strings.") + namespace = raw_namespace.strip() + if not isinstance(raw_templates, dict): + raise _fail( + config_path, + f"workflow_comments.{namespace} must be a YAML mapping of template keys to strings.", + ) + parsed_templates: dict[str, str] = {} + for raw_key, raw_template in raw_templates.items(): + if not isinstance(raw_key, str) or not raw_key.strip(): + raise _fail( + config_path, + f"workflow_comments.{namespace} keys must be non-empty strings.", + ) + key = raw_key.strip() + definition = _TEMPLATE_REGISTRY.get((namespace, key)) + if definition is None: + if namespace not in _KNOWN_NAMESPACES: + raise _fail( + config_path, + f"Unknown workflow_comments namespace: {namespace}.", + ) + raise _fail( + config_path, + f"Unknown workflow comment template key: {_template_source(namespace, key)}.", + ) + if not isinstance(raw_template, str): + raise _fail( + config_path, + f"{_template_source(namespace, key)} must be a string.", + ) + if not raw_template.strip(): + raise _fail( + config_path, + f"{_template_source(namespace, key)} must not be blank.", + ) + placeholders = _extract_placeholders( + raw_template, + config_path=config_path, + namespace=namespace, + key=key, + ) + unknown_placeholders = sorted( + placeholders - definition.allowed_placeholders + ) + if unknown_placeholders: + raise _fail( + config_path, + f"Unknown placeholders in {_template_source(namespace, key)}: " + + ", ".join(unknown_placeholders), + ) + parsed_templates[key] = raw_template + overrides[namespace] = parsed_templates + return WorkflowCommentTemplateConfig(overrides=overrides) + + +def load_workflow_comment_template_config( + workspace_root: Path | None = None, +) -> WorkflowCommentTemplateConfig: + return _load_workflow_comment_template_config_cached( + Path(workspace_root or workspace()).resolve() + ) + + +def render_comment_template( + workspace_root: Path | None = None, + *, + namespace: str, + key: str, + context: Mapping[str, str] | None = None, +) -> str: + definition = get_template_definition(namespace, key) + config = load_workflow_comment_template_config(workspace_root) + template_text = ( + config.overrides.get(namespace, {}).get(key) or definition.default_template + ) + placeholders = _extract_placeholders( + template_text, + namespace=namespace, + key=key, + ) + normalized_context = { + name: str(value) + for name, value in (context or {}).items() + } + missing = sorted(name for name in placeholders if name not in normalized_context) + if missing: + raise RuntimeError( + f"Missing placeholder values for workflow comment template " + f"{namespace}.{key}: {', '.join(missing)}." + ) + return Template(template_text).substitute(normalized_context) diff --git a/.github/scripts/oz_workflows/helpers.py b/.github/scripts/oz_workflows/helpers.py index 308deed..bed7938 100644 --- a/.github/scripts/oz_workflows/helpers.py +++ b/.github/scripts/oz_workflows/helpers.py @@ -19,7 +19,8 @@ from . import actions from .artifacts import ResolvedReviewComment -from .env import optional_env +from .comment_templates import render_comment_template +from .env import optional_env, workspace logger = logging.getLogger(__name__) @@ -362,10 +363,7 @@ def build_comment_body(content: str, metadata: str) -> str: return f"{content}\n\n{metadata}" return content -_PROGRESS_LINK_PREFIXES = ( - "You can follow along in [the session on Warp]", - "You can view [the conversation on Warp]", -) +_PROGRESS_LINK_MARKERS = ("/session/", "/conversation/") # Labels that signal that a prior triage pass has already been performed on @@ -399,42 +397,54 @@ def issue_has_prior_triage(labels: list[Any]) -> bool: def format_triage_start_line(*, is_retriage: bool) -> str: """State-aware opening line for the triage workflow.""" - if is_retriage: - return ( - "I'm re-triaging this issue based on new information." - ) - return "I'm starting to work on triaging this issue." + return render_comment_template( + workspace(), + namespace="triage-new-issues", + key="start_retriage" if is_retriage else "start_new", + ) def format_triage_session_line( *, is_retriage: bool, session_link_markdown: str ) -> str: """Mid-run status line used once the Oz session link is known.""" - verb = "re-triaging" if is_retriage else "triaging" - return f"I'm {verb} this issue. You can follow {session_link_markdown}." + return render_comment_template( + workspace(), + namespace="triage-new-issues", + key="session", + context={ + "session_link_markdown": session_link_markdown, + "triage_verb": "re-triaging" if is_retriage else "triaging", + }, + ) def format_respond_to_triaged_start_line() -> str: """State-aware opening line for the inline triaged-issue response workflow.""" - return ( - "I'm drafting an inline response to this comment. " - "This issue is already triaged, so I'll reply without changing labels, " - "the issue body, or assignees." + return render_comment_template( + workspace(), + namespace="respond-to-triaged-issue", + key="start", ) def format_spec_start_line(*, is_update: bool) -> str: """State-aware opening line for the create-spec-from-issue workflow.""" - if is_update: - return "I'm updating the existing spec PR for this issue." - return "I'm starting work on product and tech specs for this issue." + return render_comment_template( + workspace(), + namespace="create-spec-from-issue", + key="start_update" if is_update else "start_new", + ) def format_spec_complete_line(*, is_update: bool, pr_url: str) -> str: """State-aware completion line for the create-spec-from-issue workflow.""" - if is_update: - return f"I updated the existing [spec PR]({pr_url}) for this issue." - return f"I created a new [spec PR]({pr_url}) for this issue." + return render_comment_template( + workspace(), + namespace="create-spec-from-issue", + key="complete_updated" if is_update else "complete_created", + context={"pr_url": pr_url}, + ) def format_implementation_start_line( @@ -456,29 +466,38 @@ def format_implementation_start_line( """ if should_noop: numbers = ", ".join(f"#{n}" for n in (unapproved_spec_pr_numbers or [])) - suffix = f" Linked spec PR(s): {numbers}." if numbers else "" - return ( - "I'm not starting implementation because the linked spec PR(s) " - "have not been marked `plan-approved`." - + suffix + return render_comment_template( + workspace(), + namespace="create-implementation-from-issue", + key="start_blocked_unapproved_specs", + context={ + "linked_spec_prs_suffix": ( + f". Linked spec PR(s): {numbers}" if numbers else "" + ) + }, ) - updating = " (updating the existing draft PR)" if existing_implementation_pr else "" if spec_context_source == "approved-pr": - return ( - "I'm implementing this issue on top of the approved spec PR's branch" - + updating - + "." + key = ( + "start_from_approved_spec_update_pr" + if existing_implementation_pr + else "start_from_approved_spec_new_pr" ) - if spec_context_source == "directory": - return ( - "I'm implementing this issue using the repository's directory specs" - + updating - + "." + elif spec_context_source == "directory": + key = ( + "start_from_directory_specs_update_pr" + if existing_implementation_pr + else "start_from_directory_specs_new_pr" ) - return ( - "I'm implementing this issue with no spec context" - + updating - + "." + else: + key = ( + "start_without_spec_context_update_pr" + if existing_implementation_pr + else "start_without_spec_context_new_pr" + ) + return render_comment_template( + workspace(), + namespace="create-implementation-from-issue", + key=key, ) @@ -490,29 +509,40 @@ def format_implementation_complete_line( ) -> str: """State-aware completion line for the implementation workflow.""" if updated_spec_pr: - return ( - f"I pushed implementation updates to the linked approved [spec PR]({pr_url})." - ) - if existing_implementation_pr: - return ( - f"I updated the existing draft [implementation PR]({pr_url}) for this issue." - ) - return f"I created a new draft [implementation PR]({pr_url}) for this issue." + key = "complete_updated_spec_pr" + elif existing_implementation_pr: + key = "complete_updated_existing_draft_pr" + else: + key = "complete_created_new_draft_pr" + return render_comment_template( + workspace(), + namespace="create-implementation-from-issue", + key=key, + context={"pr_url": pr_url}, + ) def format_review_start_line( *, spec_only: bool, is_rereview: bool, focus: str = "" ) -> str: """State-aware opening line for the review-pull-request workflow.""" - kind = "spec-only pull request" if spec_only else "pull request" - if is_rereview: - base = f"I'm re-reviewing this {kind} in response to a review request." + if spec_only and is_rereview: + key = "start_rereview_spec" + elif spec_only: + key = "start_first_review_spec" + elif is_rereview: + key = "start_rereview_code" else: - base = f"I'm starting a first review of this {kind}." + key = "start_first_review_code" focus_text = (focus or "").strip() - if focus_text: - return f"{base} Focus: {focus_text}" - return base + return render_comment_template( + workspace(), + namespace="review-pull-request", + key=key, + context={ + "focus_suffix": f" Focus: {focus_text}" if focus_text else "", + }, + ) def format_pr_comment_start_line( @@ -520,19 +550,27 @@ def format_pr_comment_start_line( ) -> str: """State-aware opening line for the respond-to-pr-comment workflow.""" if is_review_reply: - source = "an inline review-thread comment" + key = ( + "start_review_reply_with_spec_context" + if has_spec_context + else "start_review_reply_without_spec_context" + ) elif is_review_body: - source = "a PR review body" + key = ( + "start_review_body_with_spec_context" + if has_spec_context + else "start_review_body_without_spec_context" + ) else: - source = "a PR conversation comment" - spec_clause = ( - " Spec context was found and will be used to ground the change." - if has_spec_context - else "" - ) - return ( - f"I'm working on changes requested in this PR (responding to {source})." - + spec_clause + key = ( + "start_conversation_comment_with_spec_context" + if has_spec_context + else "start_conversation_comment_without_spec_context" + ) + return render_comment_template( + workspace(), + namespace="respond-to-pr-comment", + key=key, ) @@ -540,13 +578,11 @@ def format_enforce_start_line( *, explicit_issue: bool, change_kind: str ) -> str: """State-aware opening line for the enforce-pr-issue-state workflow.""" - association = ( - "an explicitly linked issue" - if explicit_issue - else "a likely matching ready issue" - ) - return ( - f"I'm checking this {change_kind} PR for association with {association}." + return render_comment_template( + workspace(), + namespace="enforce-pr-issue-state", + key="start_explicit_issue" if explicit_issue else "start_matching_ready_issue", + context={"change_kind": change_kind}, ) @@ -563,8 +599,24 @@ def _workflow_run_url() -> str: def _format_progress_link_section(session_link: str) -> str: normalized_link = session_link.strip() if "/conversation/" in normalized_link: - return f"You can view [the conversation on Warp]({normalized_link})." - return f"You can follow along in [the session on Warp]({normalized_link})." + return render_comment_template( + workspace(), + namespace="shared", + key="progress_session_conversation", + context={ + "session_link_markdown": ( + f"[the conversation on Warp]({normalized_link})" + ) + }, + ) + return render_comment_template( + workspace(), + namespace="shared", + key="progress_session_session", + context={ + "session_link_markdown": f"[the session on Warp]({normalized_link})" + }, + ) def _format_triage_session_link(session_link: str) -> str: @@ -583,11 +635,14 @@ def append_comment_sections(existing_body: str, metadata: str, sections: list[st # re-add it as the last section after new sections are appended. updated_sections = [s for s in updated_sections if s != POWERED_BY_SUFFIX] for section in normalized_sections: - if section.startswith(_PROGRESS_LINK_PREFIXES): + if any(marker in section for marker in _PROGRESS_LINK_MARKERS): updated_sections = [ existing_section for existing_section in updated_sections - if not existing_section.startswith(_PROGRESS_LINK_PREFIXES) + if not any( + marker in existing_section + for marker in _PROGRESS_LINK_MARKERS + ) ] updated_sections.append(section) continue @@ -773,12 +828,18 @@ def report_error(self) -> None: """ run_url = _workflow_run_url() if run_url: - message = ( - "I ran into an unexpected error while working on this. " - f"You can view [the workflow run]({run_url}) for more details." + message = render_comment_template( + workspace(), + namespace="shared", + key="error_with_run_link", + context={"workflow_run_url": run_url}, ) else: - message = "I ran into an unexpected error while working on this." + message = render_comment_template( + workspace(), + namespace="shared", + key="error_without_run_link", + ) sections: list[str] = [] normalized_requester = self.requester_login.strip().removeprefix("@") if normalized_requester: @@ -1156,10 +1217,16 @@ def build_spec_preview_section(owner: str, repo: str, branch_name: str, issue_nu tech_path = f"{spec_dir}/tech.md" product_url = f"https://github.com/{owner}/{repo}/blob/{branch_name}/{product_path}" tech_url = f"https://github.com/{owner}/{repo}/blob/{branch_name}/{tech_path}" - return ( - f"Preview generated specs:\n" - f"- Product spec: [{product_path}]({product_url})\n" - f"- Tech spec: [{tech_path}]({tech_url})" + return render_comment_template( + workspace(), + namespace="shared", + key="spec_preview", + context={ + "product_path": product_path, + "product_url": product_url, + "tech_path": tech_path, + "tech_url": tech_url, + }, ) @@ -1224,7 +1291,16 @@ def build_next_steps_section(steps: list[str]) -> str: normalized_steps = [step.strip() for step in steps if step and step.strip()] if not normalized_steps: return "" - return "Next steps:\n" + "\n".join(f"- {step}" for step in normalized_steps) + return render_comment_template( + workspace(), + namespace="shared", + key="next_steps_section", + context={ + "next_steps_markdown": "\n".join( + f"- {step}" for step in normalized_steps + ) + }, + ) def branch_exists(github: Repository, owner: str, repo: str, branch: str) -> bool: diff --git a/.github/scripts/oz_workflows/workflow_config.py b/.github/scripts/oz_workflows/workflow_config.py index 0452b3e..276619e 100644 --- a/.github/scripts/oz_workflows/workflow_config.py +++ b/.github/scripts/oz_workflows/workflow_config.py @@ -3,6 +3,7 @@ import os import re from dataclasses import dataclass +from functools import lru_cache from pathlib import Path from typing import Any @@ -21,6 +22,12 @@ class SelfImprovementConfig: base_branch: str | None +@dataclass(frozen=True) +class WorkflowConfigDocument: + path: Path + data: dict[str, Any] + + def resolve_repo_config_path(workspace_root: Path) -> Path | None: """Resolve the first available workflow config path for *workspace_root*.""" for root in preferred_repo_roots(workspace_root): @@ -107,15 +114,8 @@ def _parse_env_base_branch(config_path: Path) -> str | None: ) -def load_self_improvement_config(workspace_root: Path) -> SelfImprovementConfig: - """Load and validate the resolved self-improvement workflow config.""" - config_path = resolve_repo_config_path(workspace_root) - if config_path is None: - raise RuntimeError( - "Unable to locate .github/oz/config.yml in either the consuming " - "repository workspace or the checked-out workflow code." - ) - +@lru_cache(maxsize=None) +def _load_workflow_config_document_from_path(config_path: Path) -> WorkflowConfigDocument: try: raw_data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except yaml.YAMLError as exc: @@ -132,6 +132,26 @@ def load_self_improvement_config(workspace_root: Path) -> SelfImprovementConfig: if version != 1: raise _fail(config_path, "Unsupported config version; expected version: 1.") + return WorkflowConfigDocument(path=config_path, data=raw_data) + + +def load_workflow_config_document(workspace_root: Path) -> WorkflowConfigDocument: + """Load and validate the resolved workflow config document once.""" + config_path = resolve_repo_config_path(workspace_root) + if config_path is None: + raise RuntimeError( + "Unable to locate .github/oz/config.yml in either the consuming " + "repository workspace or the checked-out workflow code." + ) + return _load_workflow_config_document_from_path(config_path) + + +def load_self_improvement_config(workspace_root: Path) -> SelfImprovementConfig: + """Load and validate the resolved self-improvement workflow config.""" + document = load_workflow_config_document(workspace_root) + config_path = document.path + raw_data = document.data + self_improvement = raw_data.get("self_improvement") if self_improvement is None: self_improvement = {} diff --git a/.github/scripts/tests/test_comment_on_unready_assigned_issue.py b/.github/scripts/tests/test_comment_on_unready_assigned_issue.py index 81f1ecd..423520c 100644 --- a/.github/scripts/tests/test_comment_on_unready_assigned_issue.py +++ b/.github/scripts/tests/test_comment_on_unready_assigned_issue.py @@ -1,13 +1,25 @@ from __future__ import annotations +import os import unittest +from pathlib import Path +from tempfile import TemporaryDirectory +from unittest.mock import MagicMock, patch from comment_on_unready_assigned_issue import ( DEFAULT_ASSIGNEE_LOGIN, + main, resolve_assignee_login, ) +def _write_config(repo_root: Path, text: str) -> Path: + path = repo_root / ".github" / "oz" / "config.yml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + class ResolveAssigneeLoginTest(unittest.TestCase): """Tests for resolve_assignee_login, which must tolerate null/missing values.""" @@ -32,5 +44,55 @@ def test_assignee_shapes(self) -> None: self.assertEqual(resolve_assignee_login(event), expected) +class MainTest(unittest.TestCase): + def test_main_uses_configured_comment_templates(self) -> None: + with TemporaryDirectory() as tempdir: + _write_config( + Path(tempdir), + ( + "version: 1\n" + "workflow_comments:\n" + " comment-on-unready-assigned-issue:\n" + " start: Custom assignment check.\n" + " complete: Custom unready assignment notice.\n" + ), + ) + + client = MagicMock() + client.close = MagicMock() + github = MagicMock() + issue = MagicMock() + client.get_repo.return_value = github + github.get_issue.return_value = issue + progress_instance = MagicMock() + + with ( + patch.dict(os.environ, {"GITHUB_WORKSPACE": tempdir}, clear=False), + patch( + "comment_on_unready_assigned_issue.load_event", + return_value={ + "issue": {"number": 42}, + "assignee": {"login": "alice"}, + }, + ), + patch("comment_on_unready_assigned_issue.repo_parts", return_value=("owner", "repo")), + patch("comment_on_unready_assigned_issue.repo_slug", return_value="owner/repo"), + patch("comment_on_unready_assigned_issue.require_env", return_value="token"), + patch("comment_on_unready_assigned_issue.Auth.Token", return_value="token"), + patch("comment_on_unready_assigned_issue.Github", return_value=client), + patch( + "comment_on_unready_assigned_issue.WorkflowProgressComment", + return_value=progress_instance, + ), + ): + main() + + progress_instance.start.assert_called_once_with("Custom assignment check.") + progress_instance.complete.assert_called_once_with( + "Custom unready assignment notice." + ) + issue.remove_from_assignees.assert_called_once_with("alice") + + if __name__ == "__main__": unittest.main() diff --git a/.github/scripts/tests/test_comment_templates.py b/.github/scripts/tests/test_comment_templates.py new file mode 100644 index 0000000..e4e7018 --- /dev/null +++ b/.github/scripts/tests/test_comment_templates.py @@ -0,0 +1,211 @@ +from __future__ import annotations + +import os +import unittest +from pathlib import Path +from tempfile import TemporaryDirectory +from unittest.mock import patch + +from oz_workflows.comment_templates import ( + load_workflow_comment_template_config, + render_comment_template, +) +from oz_workflows.helpers import build_next_steps_section, format_triage_start_line + + +def _write_config(repo_root: Path, text: str) -> Path: + path = repo_root / ".github" / "oz" / "config.yml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +class WorkflowCommentTemplateConfigTest(unittest.TestCase): + def test_defaults_apply_when_workflow_comments_absent(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + _write_config(workspace_root, "version: 1\n") + + self.assertEqual( + render_comment_template( + workspace_root, + namespace="shared", + key="next_steps_section", + context={"next_steps_markdown": "- validate\n- ship"}, + ), + "Next steps:\n- validate\n- ship", + ) + + def test_render_accepts_string_workspace_root(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + _write_config(workspace_root, "version: 1\n") + + self.assertEqual( + render_comment_template( + str(workspace_root), + namespace="triage-new-issues", + key="start_new", + ), + "I'm starting to work on triaging this issue.", + ) + + def test_helper_functions_use_configured_overrides(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + _write_config( + workspace_root, + ( + "version: 1\n" + "workflow_comments:\n" + " shared:\n" + " next_steps_section: |-\n" + " Do this next:\n" + " ${next_steps_markdown}\n" + " triage-new-issues:\n" + " start_new: Custom triage start.\n" + ), + ) + + with patch("oz_workflows.helpers.workspace", return_value=str(workspace_root)): + self.assertEqual( + build_next_steps_section(["collect logs", "retry"]), + "Do this next:\n- collect logs\n- retry", + ) + self.assertEqual( + format_triage_start_line(is_retriage=False), + "Custom triage start.", + ) + + def test_falls_back_to_workflow_repo_config_for_templates(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + workflow_root = workspace_root / "__oz_shared" + _write_config( + workflow_root, + ( + "version: 1\n" + "workflow_comments:\n" + " shared:\n" + " next_steps_section: |-\n" + " Workflow fallback:\n" + " ${next_steps_markdown}\n" + ), + ) + + with patch.dict( + os.environ, + { + "GITHUB_WORKSPACE": str(workspace_root), + "WORKFLOW_CODE_PATH": "__oz_shared", + }, + clear=False, + ): + self.assertEqual( + render_comment_template( + workspace_root, + namespace="shared", + key="next_steps_section", + context={"next_steps_markdown": "- from fallback"}, + ), + "Workflow fallback:\n- from fallback", + ) + + def test_invalid_workflow_comment_config_cases(self) -> None: + cases = [ + ( + "unknown_namespace", + ( + "version: 1\n" + "workflow_comments:\n" + " made-up:\n" + " start: nope\n" + ), + "Unknown workflow_comments namespace", + ), + ( + "unknown_key", + ( + "version: 1\n" + "workflow_comments:\n" + " triage-new-issues:\n" + " made_up: nope\n" + ), + "Unknown workflow comment template key", + ), + ( + "blank_value", + ( + "version: 1\n" + "workflow_comments:\n" + " triage-new-issues:\n" + " start_new: \" \"\n" + ), + "must not be blank", + ), + ( + "non_string_value", + ( + "version: 1\n" + "workflow_comments:\n" + " triage-new-issues:\n" + " start_new: 42\n" + ), + "must be a string", + ), + ( + "invalid_placeholder_syntax", + ( + "version: 1\n" + "workflow_comments:\n" + " shared:\n" + " next_steps_section: \"Next: $next_steps_markdown\"\n" + ), + "Invalid placeholder syntax", + ), + ( + "unknown_placeholder", + ( + "version: 1\n" + "workflow_comments:\n" + " shared:\n" + " next_steps_section: \"Next: ${unknown_value}\"\n" + ), + "Unknown placeholders", + ), + ] + for label, contents, expected in cases: + with self.subTest(label=label): + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + config_path = _write_config(workspace_root, contents) + + with self.assertRaises(RuntimeError) as ctx: + load_workflow_comment_template_config(workspace_root) + + self.assertIn(str(config_path), str(ctx.exception)) + self.assertIn(expected, str(ctx.exception)) + + def test_render_requires_all_placeholder_values(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + _write_config(workspace_root, "version: 1\n") + + with self.assertRaises(RuntimeError) as ctx: + render_comment_template( + workspace_root, + namespace="shared", + key="spec_preview", + context={ + "product_path": "specs/GH1/product.md", + "product_url": "https://example.test/product", + }, + ) + + self.assertIn("Missing placeholder values", str(ctx.exception)) + self.assertIn("tech_path", str(ctx.exception)) + self.assertIn("tech_url", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/scripts/tests/test_enforce_pr_issue_state.py b/.github/scripts/tests/test_enforce_pr_issue_state.py index 3db2232..b196850 100644 --- a/.github/scripts/tests/test_enforce_pr_issue_state.py +++ b/.github/scripts/tests/test_enforce_pr_issue_state.py @@ -1,6 +1,9 @@ from __future__ import annotations +import os import unittest +from pathlib import Path +from tempfile import TemporaryDirectory from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -11,6 +14,13 @@ ) +def _write_config(repo_root: Path, text: str) -> Path: + path = repo_root / ".github" / "oz" / "config.yml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + class IsPrAuthorOrgMemberTest(unittest.TestCase): def test_association_table(self) -> None: cases = [ @@ -220,6 +230,62 @@ def test_associated_unready_issue_close_path_posts_start_and_complete(self) -> N pr.edit.assert_called_once_with(state="closed") mock_set_output.assert_called_once_with("allow_review", "false") + def test_associated_unready_issue_uses_configured_close_template(self) -> None: + with TemporaryDirectory() as tempdir: + _write_config( + Path(tempdir), + ( + "version: 1\n" + "workflow_comments:\n" + " enforce-pr-issue-state:\n" + " close_explicit_issue_not_ready: |-\n" + " Custom close for ${issue_refs} requiring ${required_label}.\n" + ), + ) + + client = MagicMock() + client.close = MagicMock() + github = MagicMock() + client.get_repo.return_value = github + + pr = self._build_basic_pr(filename="src/app.py", body="Closes #42") + github.get_pull.return_value = pr + github.get_issue.return_value = SimpleNamespace( + pull_request=None, + number=42, + title="Not ready", + body="", + html_url="https://github.com/owner/repo/issues/42", + labels=[SimpleNamespace(name="bug")], + ) + + progress_instance = MagicMock() + + with ( + patch.dict(os.environ, {"GITHUB_WORKSPACE": tempdir}, clear=False), + patch("enforce_pr_issue_state.require_env", side_effect=["7", "token"]), + patch("enforce_pr_issue_state.optional_env", return_value=None), + patch("enforce_pr_issue_state.repo_parts", return_value=("owner", "repo")), + patch("enforce_pr_issue_state.repo_slug", return_value="owner/repo"), + patch("enforce_pr_issue_state.workspace", return_value=tempdir), + patch("enforce_pr_issue_state.Auth.Token", return_value="token"), + patch("enforce_pr_issue_state.Github", return_value=client), + patch( + "enforce_pr_issue_state.WorkflowProgressComment", + return_value=progress_instance, + ), + patch( + "enforce_pr_issue_state.resolve_pr_association", + return_value={"same_repo_issue_numbers": [42]}, + ), + patch("enforce_pr_issue_state.set_output"), + ): + main() + + progress_instance.complete.assert_called_once_with( + "Custom close for #42 requiring ready-to-implement." + ) + def test_any_ready_associated_issue_allows_pr(self) -> None: client = MagicMock() client.close = MagicMock() @@ -329,6 +395,78 @@ def test_agent_matched_allow_path_posts_start_before_agent_run(self) -> None: progress_instance.complete.assert_not_called() mock_set_output.assert_called_once_with("allow_review", "true") + def test_agent_no_match_uses_configured_close_template(self) -> None: + with TemporaryDirectory() as tempdir: + _write_config( + Path(tempdir), + ( + "version: 1\n" + "workflow_comments:\n" + " enforce-pr-issue-state:\n" + " close_no_matching_ready_issue: |-\n" + " No ready issue matched this ${change_kind} PR.\n" + " Why: ${association_rationale}\n" + ), + ) + + client = MagicMock() + client.close = MagicMock() + github = MagicMock() + client.get_repo.return_value = github + + pr = self._build_basic_pr(filename="src/app.py") + github.get_pull.return_value = pr + github.get_issues.return_value = [ + SimpleNamespace( + pull_request=None, + number=99, + title="Ready impl issue", + body="", + html_url="https://github.com/owner/repo/issues/99", + labels=[SimpleNamespace(name="ready-to-implement")], + ) + ] + + progress_instance = MagicMock() + + with ( + patch.dict(os.environ, {"GITHUB_WORKSPACE": tempdir}, clear=False), + patch("enforce_pr_issue_state.require_env", side_effect=["7", "token"]), + patch("enforce_pr_issue_state.optional_env", return_value=None), + patch("enforce_pr_issue_state.repo_parts", return_value=("owner", "repo")), + patch("enforce_pr_issue_state.repo_slug", return_value="owner/repo"), + patch("enforce_pr_issue_state.workspace", return_value=tempdir), + patch("enforce_pr_issue_state.Auth.Token", return_value="token"), + patch("enforce_pr_issue_state.Github", return_value=client), + patch( + "enforce_pr_issue_state.WorkflowProgressComment", + return_value=progress_instance, + ), + patch( + "enforce_pr_issue_state.resolve_pr_association", + return_value={"same_repo_issue_numbers": []}, + ), + patch("enforce_pr_issue_state.build_agent_config", return_value=MagicMock()), + patch( + "enforce_pr_issue_state.run_agent", + return_value=SimpleNamespace(run_id="run-123"), + ), + patch( + "enforce_pr_issue_state.poll_for_artifact", + return_value={ + "matched": False, + "issue_number": None, + "rationale": "Changed files do not line up with any ready issue.", + }, + ), + patch("enforce_pr_issue_state.set_output"), + ): + main() + + progress_instance.complete.assert_called_once_with( + "No ready issue matched this implementation PR.\nWhy: Changed files do not line up with any ready issue." + ) + class BuildIssueAssociationPromptTest(unittest.TestCase): def test_includes_security_rules_for_untrusted_pr_and_issue_content(self) -> None: diff --git a/.github/scripts/tests/test_triage.py b/.github/scripts/tests/test_triage.py index 76879a8..3a4b9d0 100644 --- a/.github/scripts/tests/test_triage.py +++ b/.github/scripts/tests/test_triage.py @@ -4,6 +4,7 @@ from datetime import datetime, timezone from pathlib import Path from tempfile import TemporaryDirectory +from unittest.mock import patch from triage_new_issues import ( TRIAGE_DISCLAIMER, _container_companion_path, @@ -49,6 +50,13 @@ ) +def _write_repo_config(repo_root: Path, text: str) -> Path: + path = repo_root / ".github" / "oz" / "config.yml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + class LoadTriageConfigTest(unittest.TestCase): def test_config_parsing_table(self) -> None: """``load_triage_config`` accepts label-only configs and rejects @@ -1190,6 +1198,51 @@ def test_fallback_summary_stays_lowercase(self) -> None: self.assertEqual(sentence, "The triage concluded that triage completed.") +class TriageWorkflowCommentTemplateOverrideTest(unittest.TestCase): + def test_reporter_facing_sections_use_configured_overrides(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + _write_repo_config( + workspace_root, + ( + "version: 1\n" + "workflow_comments:\n" + " triage-new-issues:\n" + " statements_with_reporter: |-\n" + " ${reporter_mention} custom triage summary:\n" + " ${statements_markdown}\n" + " follow_up_without_reporter: |-\n" + " Need more data:\n" + " ${questions_markdown}\n" + " duplicate_with_reporter: |-\n" + " ${reporter_mention} possible duplicates:\n" + " ${duplicate_list_markdown}\n" + ), + ) + issue_with_reporter = {"user": {"login": "alice"}} + issue_without_reporter = {"user": {}} + + with patch("triage_new_issues.workspace", return_value=str(workspace_root)): + self.assertEqual( + build_statements_section(issue_with_reporter, "- Check logs"), + "@alice custom triage summary:\n- Check logs", + ) + self.assertEqual( + build_follow_up_section( + issue_without_reporter, + [{"question": "What version?", "reasoning": ""}], + ), + "Need more data:\n1. What version?", + ) + self.assertEqual( + build_duplicate_section( + issue_with_reporter, + [{"issue_number": 10, "title": "Original report"}], + ), + "@alice possible duplicates:\n- #10 — Original report", + ) + + class TriageHeuristicsPromptTest(unittest.TestCase): def test_prompt_is_generic_regardless_of_repo(self) -> None: # Repo-specific heuristics have been moved to the diff --git a/.github/scripts/triage_new_issues.py b/.github/scripts/triage_new_issues.py index 7fe3be1..1db7d0b 100644 --- a/.github/scripts/triage_new_issues.py +++ b/.github/scripts/triage_new_issues.py @@ -11,6 +11,7 @@ from github.Repository import Repository from oz_workflows.actions import append_summary, warning +from oz_workflows.comment_templates import render_comment_template from oz_workflows.docker_agent import ( REPO_MOUNT, resolve_triage_image, @@ -53,6 +54,20 @@ TRIAGE_DISCLAIMER = "*This is my automated analysis and may be incorrect. A maintainer will verify the details.*" +def _triage_template(key: str, **context: str) -> str: + return render_comment_template( + workspace(), + namespace=WORKFLOW_NAME, + key=key, + context=context, + ) + + +def _reporter_mention(issue: Any) -> str: + reporter_login = get_login(get_field(issue, "user")).strip() + return f"@{reporter_login}" if reporter_login else "" + + def _lowercase_first(text: str) -> str: """Lowercase the first character of *text* so it reads naturally mid-sentence. @@ -283,17 +298,27 @@ def process_issue( if session_link: link_text = _format_triage_session_link(session_link) parts.append( - "I've finished triaging this issue. " - "A maintainer will verify the details shortly. " - f"You can view {link_text}." + _triage_template( + "complete_without_user_facing_content_with_session", + session_link_markdown=link_text, + ) ) else: - parts.append("I've completed the triage of this issue.") + parts.append( + _triage_template( + "complete_without_user_facing_content_without_session" + ) + ) elif session_link: # Follow-up questions or duplicates are present; show session link # on its own line before the user-facing content. link_text = _format_triage_session_link(session_link) - parts.append(f"You can view {link_text}.") + parts.append( + _triage_template( + "session_link_only", + session_link_markdown=link_text, + ) + ) # User-facing content above the fold: follow-up questions or duplicate info. # Follow-up questions and duplicates are mutually exclusive. @@ -328,13 +353,13 @@ def process_issue( details_body = "\n\n".join(maintainer_parts) parts.append( - "
\n" - "Maintainer details\n\n" - f"{details_body}\n\n" - "
" + _triage_template( + "maintainer_details", + maintainer_details_markdown=details_body, + ) ) - parts.append(TRIAGE_DISCLAIMER) + parts.append(_triage_template("disclaimer")) progress.replace_body("\n\n".join(parts)) append_summary(f"- Issue #{issue_number}: {summary} Labels: {labels_text}.\n") except Exception: @@ -699,15 +724,17 @@ def build_question_reasoning_section(questions: list[dict[str, str]]) -> str: def build_statements_section(issue: Any, statements: str) -> str: """Build the reporter-facing statements section for the progress comment.""" - reporter_login = get_login(get_field(issue, "user")).strip() - lines: list[str] = [] - if reporter_login: - lines.append(f"@{reporter_login} — here's what I found while triaging this issue:") - else: - lines.append("Here's what I found while triaging this issue:") - lines.append("") - lines.append(statements) - return "\n".join(lines) + reporter_mention = _reporter_mention(issue) + if reporter_mention: + return _triage_template( + "statements_with_reporter", + reporter_mention=reporter_mention, + statements_markdown=statements, + ) + return _triage_template( + "statements_without_reporter", + statements_markdown=statements, + ) def build_follow_up_section(issue: Any, questions: list[dict[str, str]]) -> str: @@ -717,32 +744,26 @@ def build_follow_up_section(issue: Any, questions: list[dict[str, str]]) -> str: Only the question text is rendered here; reasoning is handled separately by ``build_question_reasoning_section`` for the maintainer section. """ - reporter_login = get_login(get_field(issue, "user")).strip() - lines: list[str] = [] - if reporter_login: - lines.append(f"@{reporter_login} — I have a few follow-up questions before I can narrow this down:") - else: - lines.append("I have a few follow-up questions before I can narrow this down:") - lines.append("") - lines.extend(f"{i}. {q['question']}" for i, q in enumerate(questions, start=1)) - lines.append("") - lines.append( - "Reply in-thread with those details and the triage workflow will " - "automatically re-evaluate the issue and update the diagnosis, " - "labels, and next steps." + reporter_mention = _reporter_mention(issue) + questions_markdown = "\n".join( + f"{i}. {q['question']}" for i, q in enumerate(questions, start=1) + ) + if reporter_mention: + return _triage_template( + "follow_up_with_reporter", + reporter_mention=reporter_mention, + questions_markdown=questions_markdown, + ) + return _triage_template( + "follow_up_without_reporter", + questions_markdown=questions_markdown, ) - return "\n".join(lines) def build_duplicate_section(issue: Any, duplicates: list[dict[str, Any]]) -> str: """Build the duplicate detection section for embedding in the progress comment.""" - reporter_login = get_login(get_field(issue, "user")).strip() + reporter_mention = _reporter_mention(issue) lines: list[str] = [] - if reporter_login: - lines.append(f"@{reporter_login} — this issue appears to overlap with existing issues:") - else: - lines.append("This issue appears to overlap with existing issues:") - lines.append("") for dup in duplicates: num = dup["issue_number"] title = dup.get("title") or "" @@ -750,13 +771,17 @@ def build_duplicate_section(issue: Any, duplicates: list[dict[str, Any]]) -> str if title: line += f" — {title}" lines.append(line) - lines.append("") - lines.append( - "If this report is meaningfully different, please comment with the " - "additional context or distinguishing behavior so a maintainer can " - "review it. Otherwise, a maintainer may close it as a duplicate after review." + duplicate_list_markdown = "\n".join(lines) + if reporter_mention: + return _triage_template( + "duplicate_with_reporter", + reporter_mention=reporter_mention, + duplicate_list_markdown=duplicate_list_markdown, + ) + return _triage_template( + "duplicate_without_reporter", + duplicate_list_markdown=duplicate_list_markdown, ) - return "\n".join(lines) def _triage_summary_comment_metadata(issue_number: int) -> str: diff --git a/README.md b/README.md index 7ff9385..bfa3fdd 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ Each adapter is deliberately thin — it defines the GitHub event triggers and c Repositories can commit `.github/oz/config.yml` to make workflow-level defaults visible and reviewable in source control. Oz resolves that file from the consuming repository first; if it is absent there, the workflows fall back to the bundled `.github/oz/config.yml` shipped with `oz-for-oss`. Discovery stops at the first existing file — the two locations are not merged. -The initial supported settings live under `self_improvement`: +Supported settings currently live under `self_improvement` and `workflow_comments`: ```yaml version: 1 @@ -80,12 +80,23 @@ self_improvement: - octocat - repo-maintainer base_branch: auto +workflow_comments: + shared: + next_steps_section: | + Next: + ${next_steps_markdown} + triage-new-issues: + session: "I'm ${triage_verb} this issue. Track it in ${session_link_markdown}." ``` - `self_improvement.reviewers` — optional list of GitHub handles. Set `[]` to disable automatic reviewer requests. - `self_improvement.base_branch` — optional branch name, or `auto` to detect the repository default branch from git metadata. - `SELF_IMPROVEMENT_REVIEWERS` and `SELF_IMPROVEMENT_BASE_BRANCH` remain high-precedence overrides for one-off runs. - Provide reviewer handles without the `@` prefix in both `.github/oz/config.yml` and `SELF_IMPROVEMENT_REVIEWERS`. +- `workflow_comments` — optional mapping of deterministic workflow-owned comment text. Templates are keyed by workflow namespace (for example `shared`, `triage-new-issues`, or `enforce-pr-issue-state`) and then by a supported template key within that namespace. +- Template placeholders must use `${name}` syntax. Every override is validated strictly at load time: unknown namespaces, unknown template keys, non-string or blank values, invalid placeholder syntax, and unknown placeholders all fail the workflow with an error that points back to `.github/oz/config.yml`. +- If `workflow_comments` is omitted, the built-in `oz-for-oss` wording is used unchanged. +- Only deterministic workflow-owned strings are configurable this way. Free-form agent-authored prose, such as the substantive content of a triage diagnosis or PR review, is still generated by the agent rather than templated in YAML. The bundled fallback config is intentionally neutral: it does not ship a Warp-specific reviewer list and defaults the base branch to `auto`. From c59acccde49dd30112df68cf9869ad3aaab5084d Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Sun, 26 Apr 2026 23:21:03 +0000 Subject: [PATCH 4/4] fix: route all remaining hardcoded workflow comments through template registry Add missing completion/no-op template keys to the registry and route all previously hardcoded entrypoint strings through render_comment_template: - create-spec-from-issue.complete_no_diff - create-implementation-from-issue.complete_no_diff - respond-to-pr-comment.complete_no_diff - review-pull-request.complete_no_feedback / complete_approved_with_reviewers / complete_approved_no_reviewers / complete_changes_requested / complete_commented Also include the resolved config path in the render-time missing-placeholder error when the template came from a workflow_comments override, and update the tech spec to enumerate all first-release keys including the completion paths. Co-Authored-By: Oz --- .../create_implementation_from_issue.py | 3 +- .github/scripts/create_spec_from_issue.py | 3 +- .../scripts/oz_workflows/comment_templates.py | 58 +++++++++++++++++-- .github/scripts/respond_to_pr_comment.py | 3 +- .github/scripts/review_pr.py | 34 +++++++---- .../scripts/tests/test_comment_templates.py | 25 ++++++++ specs/GH347/tech.md | 10 +++- 7 files changed, 116 insertions(+), 20 deletions(-) diff --git a/.github/scripts/create_implementation_from_issue.py b/.github/scripts/create_implementation_from_issue.py index 51e20f5..6a7db49 100644 --- a/.github/scripts/create_implementation_from_issue.py +++ b/.github/scripts/create_implementation_from_issue.py @@ -16,6 +16,7 @@ workspace, require_env, ) +from oz_workflows.comment_templates import render_comment_template from oz_workflows.helpers import ( branch_updated_since, build_next_steps_section, @@ -242,7 +243,7 @@ def main() -> None: target_branch, created_after=run.created_at - timedelta(minutes=1), ): - progress.complete("I analyzed this issue but did not produce an implementation diff.") + progress.complete(render_comment_template(workspace(), namespace="create-implementation-from-issue", key="complete_no_diff")) return if not pr_body: diff --git a/.github/scripts/create_spec_from_issue.py b/.github/scripts/create_spec_from_issue.py index 37e35b5..38fc12c 100644 --- a/.github/scripts/create_spec_from_issue.py +++ b/.github/scripts/create_spec_from_issue.py @@ -13,6 +13,7 @@ workspace, require_env, ) +from oz_workflows.comment_templates import render_comment_template from oz_workflows.helpers import ( branch_updated_since, build_next_steps_section, @@ -198,7 +199,7 @@ def main() -> None: branch_name, created_after=run.created_at - timedelta(minutes=1), ): - progress.complete("I analyzed this issue but did not produce a spec diff.") + progress.complete(render_comment_template(workspace(), namespace="create-spec-from-issue", key="complete_no_diff")) return existing_prs = list(github.get_pulls(state="open", head=f"{owner}:{branch_name}")) metadata = load_pr_metadata_artifact(run.run_id) diff --git a/.github/scripts/oz_workflows/comment_templates.py b/.github/scripts/oz_workflows/comment_templates.py index 8aa7195..09ffb20 100644 --- a/.github/scripts/oz_workflows/comment_templates.py +++ b/.github/scripts/oz_workflows/comment_templates.py @@ -25,6 +25,7 @@ class TemplateDefinition: @dataclass(frozen=True) class WorkflowCommentTemplateConfig: overrides: dict[str, dict[str, str]] + config_path: Path def _definition( @@ -208,6 +209,11 @@ def _definition( "I updated the existing [spec PR](${pr_url}) for this issue.", allowed_placeholders=("pr_url",), ), + _definition( + "create-spec-from-issue", + "complete_no_diff", + "I analyzed this issue but did not produce a spec diff.", + ), _definition( "create-implementation-from-issue", "start_blocked_unapproved_specs", @@ -263,6 +269,11 @@ def _definition( "I created a new draft [implementation PR](${pr_url}) for this issue.", allowed_placeholders=("pr_url",), ), + _definition( + "create-implementation-from-issue", + "complete_no_diff", + "I analyzed this issue but did not produce an implementation diff.", + ), _definition( "review-pull-request", "start_first_review_code", @@ -287,6 +298,33 @@ def _definition( "I'm re-reviewing this spec-only pull request in response to a review request.${focus_suffix}", allowed_placeholders=("focus_suffix",), ), + _definition( + "review-pull-request", + "complete_no_feedback", + "I completed the review and did not identify any actionable feedback for this pull request.", + ), + _definition( + "review-pull-request", + "complete_approved_with_reviewers", + "I approved this pull request and requested human review from: ${reviewer_mentions}.", + allowed_placeholders=("reviewer_mentions",), + ), + _definition( + "review-pull-request", + "complete_approved_no_reviewers", + "I approved this pull request. No matching stakeholder was found " + "for the changed files, so no human reviewers were requested.", + ), + _definition( + "review-pull-request", + "complete_changes_requested", + "I requested changes on this pull request and posted feedback.", + ), + _definition( + "review-pull-request", + "complete_commented", + "I completed the review and posted feedback on this pull request.", + ), _definition( "respond-to-pr-comment", "start_review_reply_with_spec_context", @@ -320,6 +358,11 @@ def _definition( "start_conversation_comment_without_spec_context", "I'm working on changes requested in this PR (responding to a PR conversation comment).", ), + _definition( + "respond-to-pr-comment", + "complete_no_diff", + "I analyzed the request but did not produce any changes.", + ), _definition( "comment-on-unready-assigned-issue", "start", @@ -456,7 +499,7 @@ def _load_workflow_comment_template_config_cached( config_path = document.path raw_section = document.data.get("workflow_comments") if raw_section is None: - return WorkflowCommentTemplateConfig(overrides={}) + return WorkflowCommentTemplateConfig(overrides={}, config_path=config_path) if not isinstance(raw_section, dict): raise _fail(config_path, "workflow_comments must be a YAML mapping.") @@ -516,7 +559,7 @@ def _load_workflow_comment_template_config_cached( ) parsed_templates[key] = raw_template overrides[namespace] = parsed_templates - return WorkflowCommentTemplateConfig(overrides=overrides) + return WorkflowCommentTemplateConfig(overrides=overrides, config_path=config_path) def load_workflow_comment_template_config( @@ -536,9 +579,8 @@ def render_comment_template( ) -> str: definition = get_template_definition(namespace, key) config = load_workflow_comment_template_config(workspace_root) - template_text = ( - config.overrides.get(namespace, {}).get(key) or definition.default_template - ) + override_text = config.overrides.get(namespace, {}).get(key) + template_text = override_text or definition.default_template placeholders = _extract_placeholders( template_text, namespace=namespace, @@ -550,6 +592,12 @@ def render_comment_template( } missing = sorted(name for name in placeholders if name not in normalized_context) if missing: + if override_text: + raise _fail( + config.config_path, + f"Missing placeholder values for workflow comment template " + f"{namespace}.{key}: {', '.join(missing)}.", + ) raise RuntimeError( f"Missing placeholder values for workflow comment template " f"{namespace}.{key}: {', '.join(missing)}." diff --git a/.github/scripts/respond_to_pr_comment.py b/.github/scripts/respond_to_pr_comment.py index 8a22cad..b2286c7 100644 --- a/.github/scripts/respond_to_pr_comment.py +++ b/.github/scripts/respond_to_pr_comment.py @@ -12,6 +12,7 @@ try_load_pr_metadata_artifact, try_load_resolved_review_comments_artifact, ) +from oz_workflows.comment_templates import render_comment_template from oz_workflows.env import load_event, optional_env, repo_parts, repo_slug, require_env, workspace from oz_workflows.helpers import ( branch_updated_since, @@ -306,7 +307,7 @@ def _run_implementation( head_branch, created_after=run.created_at - timedelta(minutes=1), ): - progress.complete("I analyzed the request but did not produce any changes.") + progress.complete(render_comment_template(workspace(), namespace="respond-to-pr-comment", key="complete_no_diff")) return # Refresh the PR title/body when the agent's changes materially diff --git a/.github/scripts/review_pr.py b/.github/scripts/review_pr.py index 366e8ba..0390155 100644 --- a/.github/scripts/review_pr.py +++ b/.github/scripts/review_pr.py @@ -11,6 +11,7 @@ from github.File import File from github.GithubException import GithubException +from oz_workflows.comment_templates import render_comment_template from oz_workflows.env import optional_env, repo_parts, repo_slug, require_env, workspace from oz_workflows.helpers import ( format_review_start_line, @@ -430,24 +431,35 @@ def _normalize_review_payload( def _format_review_completion_message( + workspace_root: str, event: str, recommended_reviewers: list[str], ) -> str: - """Build the progress-comment completion message for a posted review.""" if event == "APPROVE": if recommended_reviewers: mentions = ", ".join(f"@{login}" for login in recommended_reviewers) - return ( - "I approved this pull request and requested human review from: " - f"{mentions}." + return render_comment_template( + workspace_root, + namespace="review-pull-request", + key="complete_approved_with_reviewers", + context={"reviewer_mentions": mentions}, ) - return ( - "I approved this pull request. No matching stakeholder was found " - "for the changed files, so no human reviewers were requested." + return render_comment_template( + workspace_root, + namespace="review-pull-request", + key="complete_approved_no_reviewers", ) if event == "REQUEST_CHANGES": - return "I requested changes on this pull request and posted feedback." - return "I completed the review and posted feedback on this pull request." + return render_comment_template( + workspace_root, + namespace="review-pull-request", + key="complete_changes_requested", + ) + return render_comment_template( + workspace_root, + namespace="review-pull-request", + key="complete_commented", + ) def build_review_prompt( *, @@ -735,7 +747,7 @@ def main() -> None: # agent had nothing to say, skip posting an empty review. # Non-member PRs always post so the verdict lands on the # PR even when the agent has no inline comments. - progress.complete("I completed the review and did not identify any actionable feedback for this pull request.") + progress.complete(render_comment_template(workspace(), namespace="review-pull-request", key="complete_no_feedback")) return review_body = f"{summary or 'Automated review'}\n\n{POWERED_BY_SUFFIX}" if comments: @@ -757,7 +769,7 @@ def main() -> None: owner, repo, ) - progress.complete(_format_review_completion_message(event, recommended_reviewers)) + progress.complete(_format_review_completion_message(workspace(), event, recommended_reviewers)) except Exception: progress.report_error() raise diff --git a/.github/scripts/tests/test_comment_templates.py b/.github/scripts/tests/test_comment_templates.py index e4e7018..419a1db 100644 --- a/.github/scripts/tests/test_comment_templates.py +++ b/.github/scripts/tests/test_comment_templates.py @@ -206,6 +206,31 @@ def test_render_requires_all_placeholder_values(self) -> None: self.assertIn("tech_path", str(ctx.exception)) self.assertIn("tech_url", str(ctx.exception)) + def test_render_includes_config_path_for_missing_override_placeholder(self) -> None: + with TemporaryDirectory() as tempdir: + workspace_root = Path(tempdir) + config_path = _write_config( + workspace_root, + ( + "version: 1\n" + "workflow_comments:\n" + " shared:\n" + " spec_preview: \"Preview: ${product_path}\"\n" + ), + ) + + with self.assertRaises(RuntimeError) as ctx: + render_comment_template( + workspace_root, + namespace="shared", + key="spec_preview", + context={}, + ) + + self.assertIn(str(config_path), str(ctx.exception)) + self.assertIn("Missing placeholder values", str(ctx.exception)) + self.assertIn("product_path", str(ctx.exception)) + if __name__ == "__main__": unittest.main() diff --git a/specs/GH347/tech.md b/specs/GH347/tech.md index 77e5218..8b4b581 100644 --- a/specs/GH347/tech.md +++ b/specs/GH347/tech.md @@ -142,6 +142,7 @@ Proposed namespace and key coverage: - `start_update` - `complete_created` - `complete_updated` + - `complete_no_diff` — no-op completion when the agent produced no spec changes - `create-implementation-from-issue` - `start_blocked_unapproved_specs` - `start_from_approved_spec_new_pr` @@ -153,11 +154,17 @@ Proposed namespace and key coverage: - `complete_updated_spec_pr` - `complete_updated_existing_draft_pr` - `complete_created_new_draft_pr` + - `complete_no_diff` — no-op completion when the agent produced no implementation changes - `review-pull-request` - `start_first_review_code` - `start_first_review_spec` - `start_rereview_code` - `start_rereview_spec` + - `complete_no_feedback` — no-op completion when the agent had no actionable feedback + - `complete_approved_with_reviewers` — completion when the PR was approved and reviewers were requested; uses `${reviewer_mentions}` + - `complete_approved_no_reviewers` — completion when the PR was approved but no stakeholders matched + - `complete_changes_requested` — completion when the agent requested changes + - `complete_commented` — completion when the agent posted feedback without a formal vote - `respond-to-pr-comment` - `start_review_reply_with_spec_context` - `start_review_reply_without_spec_context` @@ -165,6 +172,7 @@ Proposed namespace and key coverage: - `start_review_body_without_spec_context` - `start_conversation_comment_with_spec_context` - `start_conversation_comment_without_spec_context` + - `complete_no_diff` — no-op completion when the agent produced no changes - `comment-on-unready-assigned-issue` - `start` - `complete` @@ -174,7 +182,7 @@ Proposed namespace and key coverage: - `close_explicit_issue_not_ready` - `close_no_matching_ready_issue` -The corresponding placeholder set should be called out alongside those definitions. Important examples include `${session_link_markdown}`, `${pr_url}`, `${reporter_mention}`, `${statements_markdown}`, `${questions_markdown}`, `${duplicate_list_markdown}`, `${maintainer_details_markdown}`, `${next_steps_markdown}`, `${required_label}`, `${change_kind}`, `${contribution_docs_url}`, and `${association_rationale}`. +The corresponding placeholder set should be called out alongside those definitions. Important examples include `${session_link_markdown}`, `${pr_url}`, `${reporter_mention}`, `${statements_markdown}`, `${questions_markdown}`, `${duplicate_list_markdown}`, `${maintainer_details_markdown}`, `${next_steps_markdown}`, `${required_label}`, `${change_kind}`, `${contribution_docs_url}`, `${association_rationale}`, and `${reviewer_mentions}`. #### 3. Route helper-owned comment text through the registry