Fix #18: workflow-gate and emit-shape regressions in v1.2.0#19
Merged
Conversation
The v1.2 retest surfaced that pharaoh-mece returns a brief executive summary instead of the full MECE Analysis Report. Root cause: the session-state update was the last instruction in the skill, becoming the LLM's focal point. Reordered so the report is the final visible output. Added a top-level Output Invariant making the full report mandatory and the session-state update internal.
The v1.2 retest surfaced that pharaoh-change returns only the acknowledgment prompt; the change document never reaches the non-interactive caller. Removed Section 6's ask-for-ack pattern. The skill now writes session state with acknowledged: false, emits the full document, and ends the turn. Downstream enforcing-mode skills check the session-state flag on their own; advisory-mode flows ignore it.
Rules 3 and 6 used to hardcode :source_doc: and :verification: as required emit. Projects whose tailoring does not declare those fields/links saw their sphinx-build -W gate fail on undeclared options. Both rules are now tailoring-driven: emit only when the catalog (for fields) or ubproject.toml extra_links (for verification link) declares the slot for the type. Added tailoring_path as a required input.
The interactive 1/2/3/4 file-save prompt at Step 7b traps non-interactive callers. Replaced with a save_artifacts input flag. Step 7d session-state update moved to the start of the process so the release report is the last visible turn content. Tag suggestion collapsed into the report footer.
Adds an Output Invariant that requires the visible turn output to start with 'Decision <ID> written to <path>'. Without this line the LLM was prone to collapsing visible output to just the next-step suggestion, leaving non-interactive callers without an audit trail of which decision was recorded. Session-state update reordered to run before visible output.
There was a problem hiding this comment.
Pull request overview
This PR updates several Pharaoh skill definitions to address v1.2.0 regressions where visible artifacts were being collapsed behind workflow bookkeeping or interactive prompts. It mainly tightens output-shape rules for user-facing skills and makes pharaoh-req-from-code more tailoring-aware.
Changes:
- Added explicit output invariants so
pharaoh-change,pharaoh-mece,pharaoh-release, andpharaoh-decideemit their primary artifact in the same turn and self-terminate. - Made
pharaoh-req-from-codeconditionally emit:source_doc:and:verification:based on project tailoring, and addedtailoring_pathto its documented inputs. - Reworked
pharaoh-releaseto use asave_artifactsinput flag instead of the interactive save prompt.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
skills/pharaoh-req-from-code/SKILL.md |
Tailoring-driven rules for source_doc/verification emission and updated input/output contract. |
skills/pharaoh-release/SKILL.md |
Adds release-report output invariant and replaces interactive save flow with save_artifacts. |
skills/pharaoh-mece/SKILL.md |
Moves MECE session bookkeeping ahead of output and requires full report emission. |
skills/pharaoh-decide/SKILL.md |
Adds mandatory write-confirmation output rules for standalone decision creation. |
skills/pharaoh-change/SKILL.md |
Removes interactive acknowledgment turn and makes the change document the terminal output. |
|
|
||
| - (a) Indivisible — one file in → N reqs out. No I/O beyond file read + optional Papyrus query/write + req emit. Emits in exactly one representation per call (`rst` OR `codelinks_comment`). | ||
| - (b) Input: `{file_path, target_level, shared_context_path?, papyrus_workspace?, reporter_id, parent_feat_ids?, emit_override?, codelinks_project_name?, on_missing_config?, allowed_ids?, split_strategy?}`. Output: single JSON object `{"reqs": [{"id", "title", "type", "body", "source_doc", "satisfies", "verification", "raw_rst"}, ...]}` for `emit=rst`, or `{"codelinks": [str, ...]}` for `emit=codelinks_comment`. On missing tailoring with `on_missing_config=prompt`: single JSON object `{status: "needs_confirmation", proposal}`. | ||
| - (b) Input: `{file_path, target_level, tailoring_path, shared_context_path?, papyrus_workspace?, reporter_id, parent_feat_ids?, emit_override?, codelinks_project_name?, on_missing_config?, allowed_ids?, split_strategy?}`. Output: single JSON object `{"reqs": [{"id", "title", "type", "body", "source_doc", "satisfies", "verification", "raw_rst"}, ...]}` for `emit=rst`, or `{"codelinks": [str, ...]}` for `emit=codelinks_comment`. On missing tailoring with `on_missing_config=prompt`: single JSON object `{status: "needs_confirmation", proposal}`. |
Comment on lines
+49
to
52
| When the project declares `source_doc` and the CREQ behavior spans multiple source files, pick the file that owns the primary observable (usually the converter module, not a CLI dispatcher). `pharaoh-req-code-grounding-check` axis #8 (`source_doc_resolves`) fails if the cited file is the spec RST or missing entirely. | ||
|
|
||
| When the project does NOT declare `source_doc`, code grounding moves to a backref comment in the source file via `pharaoh-req-codelink-annotate` (mode: backref). The CREQ stays clean of paths. | ||
|
|
| 2. **As a field** in `<tailoring_path>/artefact-catalog.yaml` under `optional_fields` or `required_fields` for the directive's type. When declared, emit `:verification: <value>`. | ||
|
|
||
| Every emitted CREQ carries `:verification:` with at minimum the placeholder `tc__TBD`. Absence is a schema failure. If the project uses a different link name for the req→test relation (`verifies`, `covered_by`), declare it in `[[needs.extra_links]]`; the default placeholder stays required. | ||
| If neither slot declares `verification` for the directive's type, do NOT emit `:verification:`. The req-to-test relation is then declared elsewhere. For example, `pharaoh-vplan-draft` backlinks via `:verifies:` from the test side. |
Comment on lines
+323
to
+326
| Acknowledgment is a separate concern handled by downstream authoring skills: | ||
|
|
||
| ### If the user ignores the acknowledgment prompt | ||
| - In **advisory** strictness, no skill checks `acknowledged`. The user proceeds freely. | ||
| - In **enforcing** strictness, downstream authoring skills check `.pharaoh/session.json[changes][<id>].acknowledged` and FAIL with a message naming the file path and the field to flip. The user edits the session-state file directly to acknowledge. |
Comment on lines
+227
to
+231
| Substitute `<DEC_ID>` with the generated ID from Step 3 and `<file_path>` with the absolute or repo-relative path to the file written in Step 4 / Step 5. The confirmation line MUST appear even if the follow-up suggestion is suppressed for any reason. | ||
|
|
||
| #### Called by `pharaoh:spec` | ||
|
|
||
| Return the decision ID silently. Do not print follow-up suggestions. The calling skill manages the workflow. | ||
| Return the decision ID silently. Do not print follow-up suggestions. Do not print the written-confirmation line. The calling skill manages the workflow. |
Comment on lines
+314
to
+327
| ### Step 8: Update session state (internal) | ||
|
|
||
| This step is internal bookkeeping. Perform it silently before emitting the | ||
| report in Step 9 -- do not narrate it in the visible output. | ||
|
|
||
| Update the session state file (`.pharaoh/session.json`) as described in | ||
| `skills/shared/strictness.md`: | ||
|
|
||
| 1. Read the current `.pharaoh/session.json` (or create the default structure | ||
| if it does not exist). | ||
| 2. Set `global.mece_checked` to `true`. | ||
| 3. Set `global.mece_timestamp` to the current ISO 8601 timestamp. | ||
| 4. Set `updated` to the current ISO 8601 timestamp. | ||
| 5. Write the file back. |
Comment on lines
+462
to
+469
| #### 7.1. Update session state (internal) | ||
|
|
||
| #### 7b. Offer to write to file | ||
| Run this BEFORE emitting any visible output. Update `.pharaoh/session.json`: | ||
|
|
||
| After presenting the output, ask the user: | ||
| 1. Read the current session state (or create the initial structure). | ||
| 2. Set `global.last_release` to the current ISO 8601 timestamp. | ||
| 3. Set `updated` to the current ISO 8601 timestamp. | ||
| 4. Write the updated JSON back to `.pharaoh/session.json`. |
The B2 fix in 435d150 added an Output Invariant requiring the full Change Document as visible output, but Sonnet still collapsed to a brief summary. Diagnosis: the document template was at Section 4 in the middle of the skill, with state-update and end-turn sections following it. Sonnet read the trailing instructions as the focal point and lost the template. Reorder: state update is now Section 4 (internal), document emission is Section 5 (final visible action) and the LAST major section before Strictness Behavior. The end-turn instruction is absorbed into Section 5's closing line. Sonnet now emits the full document end-to-end (verified 6110 chars vs 643 pre-refactor).
patdhlk
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the five workflow-gate and emit-shape regressions surfaced by retesting v1.2.0 user-facing slash skills against a project conforming to the canonical schemas.
What this PR fixes
acknowledged: false, emits the change document, and ends the turn. Downstream enforcing-mode authoring skills check the session-state flag on their own.source_doc) and 6 (verification) are now tailoring-driven. Skill emits only fields the project'sartefact-catalog.yamldeclares (or[[needs.extra_links]]for the verification link). Closes thesphinx-build -Wfailure on projects that do not use those fields.1/2/3/4file-save prompt with asave_artifactsinput flag (none/changelog/release_notes/both). Session-state update runs first. Skill is self-terminating.Decision <ID> written to <path>confirmation line before any next-step suggestion. Closes the audit-trail regression for non-interactive callers.Diagnosis
All five share a common root cause. When the skill's last visible instruction is a session-state update, an interactive prompt, or a next-step suggestion, the LLM treats that as the focal point and collapses the actual artefact (report, document, decision confirmation) out of the turn output. Fix shape: hoist a top-level Output Invariant, move session-state and side-effect work before visible output, end the turn after the artefact.
Out of scope
Issue #18 Section 1 (canonical schemas at install path) and Section 9a (parametric arch types) shipped in v1.2.0 already and need no further work. Section 9c (separate hazard / safety_goal / FSR drafting skills) is a design call. `pharaoh-req-draft` already supports `target_level` in `{hazard, safety_goal, fsr, tsr}`. A separate-skill split is deferred to a follow-up issue pending a decision on whether the embedded ASIL lookup tables justify their own atomic skills.
A residual `[yes/no]` interactive prompt in `pharaoh-release` Step 3b (baseline-resolution path) was identified during review but is outside this PR's reproduction scope. Filed for a follow-up.