Close SPDD spec-completeness gaps across ET, Forecast, Frontmatter Hash, MCP Scripts, and Fuzzy Schedule#42805
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates multiple 2026-07-01 SPDD specs to close structural completeness gaps by adding missing top-level normative sections, clarifying terminology and schemas, and making deprecation and safeguard requirements explicit.
Changes:
- Adds/expands top-level Safeguards/Norms sections and compliance mappings (notably for MCP Scripts and Forecast).
- Clarifies specification contracts via added terminology, merge behavior, and field-level output schemas (Frontmatter Hash, Fuzzy Schedule).
- Documents a deprecation lifecycle and deprecation-era conformance constraints for Effective Tokens.
Show a summary per file
| File | Description |
|---|---|
| docs/src/content/docs/specs/mcp-scripts-specification.md | Adds top-level Safeguards/Norms sections, new compliance test entry, and updates TOC/Sync Notes mapping. |
| docs/src/content/docs/specs/fuzzy-schedule-specification.md | Renumbers safeguards to avoid ID collision, adds calendar field-level schema, and adds a parse-time rejection norm for unsupported period types. |
| docs/src/content/docs/specs/frontmatter-hash-specification.md | Adds a Terminology section and completes the field-selection/merge strategy table (including inputs). |
| docs/src/content/docs/specs/forecast-specification.md | Introduces a top-level Norms section and fills in concrete Sync Notes follow-up tasks with updated section numbering. |
| docs/src/content/docs/specs/effective-tokens-specification.md | Adds deprecation-era conformance guidance and a new Deprecation Lifecycle section with explicit D-ET requirements. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Low
| - **N-SN-ROT-01**: Workflows that use long-lived secrets SHOULD define an explicit rotation policy. | ||
| - **N-SN-ROT-02**: Implementations SHOULD NOT cache secret values across tool invocations beyond a | ||
| single workflow run, and any in-process cache MUST be cleared at run end. | ||
| - **N-SN-ROT-03**: When secret rotation is detected, implementations MUST NOT reuse stale cached | ||
| secret values from before the rotation event. |
| - **N-SN-SCOPE-01**: Secrets declared in workflow `env:` MUST be scoped to the minimum required | ||
| access level. | ||
| - **N-SN-SCOPE-02**: Implementations MUST NOT propagate a tool's declared secrets to child | ||
| processes or containers outside that tool's execution scope. |
| - **N-SN-SCOPE-03**: When the same secret is declared for multiple tools, each tool MUST receive | ||
| only its own environment-variable binding and MUST NOT gain access to sibling tool bindings. | ||
| - **N-SN-SCOPE-04**: Secrets MUST NOT be included in tool result objects, logs, or output returned | ||
| to MCP clients. |
| | Error code | Trigger condition | Normative requirement | | ||
| |---|---|---| | ||
| | `R-SAFE-003` | Hash input material is empty (e.g., missing workflow identifier) | §8 R-SAFE-003: MUST fail with a descriptive error; MUST NOT fall back to random scattering | | ||
| | `R-SAFE-004` | Non-unique hash input causes repeated collision across workflows | §8 R-SAFE-004: MUST log a diagnostic; MAY apply deterministic offset to resolve | | ||
| | `R-SAFE-005` | Non-unique hash input causes repeated collision across workflows | §8 R-SAFE-005: MUST log a diagnostic; MAY apply deterministic offset to resolve | | ||
|
|
| ### 8.6 Deprecation-Era Conformance | ||
|
|
||
| While this specification remains available as Deprecated documentation, ET output in `gh aw logs` | ||
| and `gh aw audit` MUST be treated as a backward-compatibility alias for AIC-based reporting. | ||
| Implementations MUST NOT introduce new ET-only output fields, ET-only APIs, or ET-only extension |
| ## Deprecation Lifecycle | ||
|
|
||
| This specification is deprecated in favor of AI Credits (AIC). The deprecation lifecycle defines | ||
| sunset criteria and compatibility obligations while ET remains available for legacy consumers. | ||
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42805 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. All changes are documentation-only (spec markdown files). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — Three blocking issues across three spec files
This batch adds valuable normative structure but ships three correctness problems that will produce downstream damage if merged as-is.
Blocking issues (new, in addition to 6 existing review comments):
-
Fuzzy Schedule — R-SAFE-004 defined after R-SAFE-005 and missing from the mandatory error code table (§13.2 explicitly requires all codes to appear there; R-SAFE-004 is absent).
-
MCP Scripts — T-MCP-051 listed as Level 2 "Standard" conformance but the test doesn't exist (Sync Notes use the word "Add", and no test file references the ID). Merging a compliance checklist row that claims coverage that isn't there is actively misleading.
-
Forecast — N-FC-003 applies RFC 2119 MUST to external callers, not implementers — untestable, unenforced, and likely to mislead spec readers about what is actually required.
Issues already flagged by prior review (for completeness)
- MCP Scripts §11 introduces duplicate IDs (N-SN-ROT-01..03 / N-SN-SCOPE-01..04) alongside existing §7.6 IDs (SN-ROT-01..03 / SN-SCOPE-01..04); compliance test T-MCP-051 references the old form (SN-SCOPE-01..04).
- Effective Tokens §8.6 heading level causes S-6 (####) to be inadvertently nested under §8.6 (###).
- Effective Tokens TOC is not updated to include the new top-level Deprecation Lifecycle section.
🔎 Code quality review by PR Code Quality Reviewer · 194.9 AIC · ⌖ 12.3 AIC · ⊞ 1.6K
Comment /review to run again
|
|
||
| ### 13.4 Unsupported Period-Type Rejection | ||
|
|
||
| **R-SAFE-004**: Implementations MUST reject unsupported period types such as `monthly` and `yearly` |
There was a problem hiding this comment.
R-SAFE-004 is defined out-of-sequence and absent from the mandatory error code table, making it invisible to implementers who check §13.2 for the authoritative list.
💡 Details
The PR renames the old R-SAFE-004 (hash collision) to R-SAFE-005, then introduces a new R-SAFE-004 (parse-time rejection) here in §13.4 — after R-SAFE-005 in document order. Two concrete problems result:
-
Out-of-order numbering: R-SAFE-005 is defined in §8 (line 871) and listed in the §13.2 error code table, but R-SAFE-004 appears after it in §13.4. Spec-compliance tooling that indexes requirements by ID will encounter R-SAFE-005 before R-SAFE-004.
-
Missing from the normative error table: §13.2 states: "Implementations MUST use these exact codes in error messages or structured error objects." R-SAFE-004 is not in that table — so an implementation emitting the correct error code has no normative anchor, and a conformance check against §13.2 alone will silently miss this requirement.
Fix: Add R-SAFE-004 to the §13.2 operator-visible error table alongside R-SAFE-003 and R-SAFE-005.
| | Secret isolation | T-SEC-001, T-SEC-002 | 1 | Required | | ||
| | Process isolation | T-SEC-003 | 2 | Standard | | ||
| | Go sandbox network isolation | T-MCP-050 | 3 | Complete | | ||
| | Secret scope norms (SN-SCOPE-01–04) | T-MCP-051 | 2 | Standard | |
There was a problem hiding this comment.
T-MCP-051 is listed as Level 2 "Standard" conformance but the test does not exist, so this compliance row is a false claim of coverage today.
💡 Details
The Sync Notes added in the same commit explicitly say:
"Add/maintain compliance coverage for T-MCP-051 in
pkg/workflow/mcp_scripts_renderer_test.gofor SN-SCOPE-01 through SN-SCOPE-04"
The verb "Add" is unambiguous: the test doesn't exist yet. Grep of pkg/workflow/ confirms no file references T-MCP-051 or the SN-SCOPE IDs.
The compliance checklist designates T-MCP-051 as Level 2 "Standard", which per §12.1 means it is required for standard conformance. Publishing a spec that marks an unimplemented test as required will cause any conformance validation pipeline that reads this table to report a false pass (if the test harness skips missing tests) or a hard failure (if it enforces the list).
Fix: Either implement T-MCP-051 before merging, or mark the checklist row as "Planned" / "Pending" with an explicit tracking reference, and gate the Level 2 designation until the test exists.
| - **N-FC-002 (Projection immutability)**: Once a projection result is computed for a given input set | ||
| and random seed policy, the emitted P10/P50/P90 values MUST be immutable within that command | ||
| invocation and MUST be reused consistently across console and JSON output paths. | ||
| - **N-FC-003 (Caller percentile obligations)**: Callers consuming forecast output MUST treat P10, P50, |
There was a problem hiding this comment.
N-FC-003 places an RFC 2119 MUST on external callers, which is unenforceable and untestable by this spec's own compliance suite.
💡 Details
"Callers consuming forecast output MUST treat P10, P50, and P90 as a set. Callers MUST NOT present P50 alone as a complete risk estimate"
RFC 2119 MUST/MUST NOT is meaningful only when the subject is an implementer of this specification who can be validated by the compliance tests in §12. "Callers" are external consumers — they are not tested by any entry in §12 (Compliance Testing), and there is no conformance mechanism that can verify or enforce this obligation.
Publishing an untestable MUST on an external actor creates two problems:
- Spec readers may interpret this as a hard conformance requirement (it is not — there is no test for it).
- If a caller violates this, there is no defined failure mode in the spec (no error code, no rejection behavior).
Fix: Downgrade to SHOULD/SHOULD NOT (advisory recommendation) or add a "Caller Guidance" / "Non-normative" callout to make clear this is best-practice prose, not a testable requirement. If it is meant to be normative, add a corresponding compliance test or audit check to §12.
| This section elevates the MCP Scripts threat model and mitigation requirements for direct normative | ||
| reference. | ||
|
|
||
| ### Threat Model |
There was a problem hiding this comment.
§10 Safeguards subsections are unnumbered, creating anchor ambiguity with Appendix D which reuses the exact same heading text at a different level.
💡 Details
The new §10 uses plain-text subsection headings:
### Threat Model### Required Mitigations### Residual Risk
Every other section in this spec uses numbered headings (### 9.1 ..., ### 7.1 ...). More concretely, Appendix D (pre-existing) uses:
#### D.1 Threat Model#### D.2 Required Mitigations#### D.3 Residual Risk
This creates two problems:
- Static site generators (Astro/Docusaurus) that auto-generate anchors from heading text will produce colliding fragment IDs (
#threat-modelappears twice), causing cross-reference links to land on the wrong section. - The §13 Sync Notes row
§10 | Safeguards | ...cannot link to specific sub-requirements without numbered anchors.
Fix: Number the §10 subsections (### 10.1 Threat Model, ### 10.2 Required Mitigations, ### 10.3 Residual Risk) to match the document convention and disambiguate anchors from Appendix D.
Review Summary — SPDD Spec-Completeness Gaps 🧠This is strong structural work — five specs all receive missing normative sections in a well-organized batch. The review found 6 actionable issues across consistency, enforceability, and coverage: 📋 Issues by file
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs, /codebase-design, and /tdd — requesting changes on normative consistency and one compliance-coverage gap.
📋 Key Themes & Findings
Key Themes
- Normative drift (MCP Scripts §11): The new
N-SN-SCOPE-*/N-SN-ROT-*norms in §11 restate §7.6 with subtly different wording, creating two canonical texts that can diverge. No precedence rule is declared. - ID namespace collision (MCP Scripts):
T-MCP-051and the compliance checklist referenceSN-SCOPE-01–04(§7.6 identifiers) while §11 names the same conceptsN-SN-SCOPE-01–04— two different labels, one concept. - Observation ≠ requirement (MCP Scripts §10):
S-MCP-001/002/003are threat observations lacking RFC 2119 verbs, yet share the normativeS-MCP-*ID scheme withS-MCP-004/005which are real MUST requirements. - Unenforceable MUST NOT (Forecast N-FC-003):
MUST NOTis placed on undefined "callers" with no testable criterion — should beSHOULD NOTor moved to informative guidance. - Silent SHOULD→MUST upgrade (Forecast N-FC-001): Cross-references §7.6 but silently promotes the obligation level from SHOULD (R-MC-030) to MUST without annotation.
- Open test vs. Standard status (MCP Scripts T-MCP-051): Compliance checklist marks T-MCP-051 as
Standard(implying implemented), while the sync note leaves coverage as an[Open]follow-up item.
Positive Highlights
- ✅ Excellent structural work: five specs all receive missing normative sections in one coherent batch.
- ✅ The
## Deprecation Lifecyclesection for ET (D-ET-001–004) is well-structured with concrete sunset criteria. - ✅ The Fuzzy Schedule §12.4 field-level schema is a useful addition with clear required/optional fields.
- ✅ The Frontmatter Hash Terminology section fills an important gap for BFS traversal and diamond dependency.
- ✅ The Forecast sync follow-up tasks are concrete and actionable (AIC/ET migration, promotion-gate evidence, CI evidence window).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 147.1 AIC · ⌖ 8.58 AIC · ⊞ 6.6K
Comment /matt to run again
| processes or containers outside that tool's execution scope. | ||
| - **N-SN-SCOPE-03**: When the same secret is declared for multiple tools, each tool MUST receive | ||
| only its own environment-variable binding and MUST NOT gain access to sibling tool bindings. | ||
| - **N-SN-SCOPE-04**: Secrets MUST NOT be included in tool result objects, logs, or output returned |
There was a problem hiding this comment.
[/grill-with-docs] N-SN-SCOPE-01–04 in §11 are near-duplicates of SN-SCOPE-01–04 in §7.6.2 with subtly different wording — two canonical texts now exist and they can diverge.
💡 Details and suggested fix
For example, §7.6.2 SN-SCOPE-03 cross-references §7.1 and §7.2 for execution scope; §11 N-SN-SCOPE-03 drops those references. §7.6.2 SN-SCOPE-04 says "log lines, or any output forwarded to the MCP client"; §11 says "logs, or output returned to MCP clients" — a meaningful narrowing.
Suggested fix: Add a preamble to §11:
This section reproduces §7.6 norms verbatim for top-level reference.
§7.6 remains authoritative; in any conflict, §7.6 takes precedence.
Or replace §11 entries with explicit cross-references rather than re-stating norms inline.
@copilot please address this.
| - **T-SEC-008**: GitHub Actions global objects access control | ||
| - **T-MCP-050**: Go sandbox network isolation (no unrestricted outbound access without explicit | ||
| `network.allowed` entries) | ||
| - **T-MCP-051**: Secret-scope isolation for SN-SCOPE-01 through SN-SCOPE-04 (per-tool binding only, |
There was a problem hiding this comment.
[/grill-with-docs] T-MCP-051 description and the compliance checklist both reference SN-SCOPE-01–04 (§7.6 identifiers), but the elevating §11 Norms section names them N-SN-SCOPE-01–04. There are now two distinct ID namespaces for the same constraints.
💡 Details
Line 1142: SN-SCOPE-01 through SN-SCOPE-04
Line 1187 (checklist): SN-SCOPE-01–04
But §11 uses: N-SN-SCOPE-01, N-SN-SCOPE-02, etc.
Pick one identifier form and use it consistently throughout: the compliance test, the checklist, the sync follow-up note, and the §11 Norms headers should all reference the same label.
@copilot please address this.
| installer logs, and exception stack traces. | ||
| - **S-MCP-002**: Container escape attempts can originate from shell/python/go tools via privilege | ||
| escalation paths such as host mounts, kernel interfaces, or unrestricted network egress. | ||
| - **S-MCP-003**: Cross-tool contamination risk exists when one invocation attempts to read another |
There was a problem hiding this comment.
[/codebase-design] S-MCP-001, S-MCP-002, and S-MCP-003 are labeled with the normative S-MCP-* identifier scheme but describe threat observations, not requirements — they lack any RFC 2119 verb (MUST/SHOULD/MAY). This makes them look like normative requirements but they have no testable obligation.
💡 Suggested fix
Either:
- Re-label them as informative threat items with a non-normative prefix (e.g.,
THREAT-001) to distinguish them fromS-MCP-004/S-MCP-005which do carry MUST requirements, or - Add explicit MUST/SHALL language — e.g.,
S-MCP-001: Implementations MUST treat tool stdout/stderr ... as a potential secret leakage vector and MUST sanitize accordingly.
Mixing observation items and normative requirements under the same ID scheme erodes reader confidence in which entries bind implementations.
@copilot please address this.
|
|
||
| **R-SAFE-004**: Implementations MUST reject unsupported period types such as `monthly` and `yearly` | ||
| at parse time. The parser MUST return a deterministic validation error and MUST NOT silently coerce | ||
| unsupported period types into supported schedules. |
There was a problem hiding this comment.
[/grill-with-docs] R-SAFE-004 (parse-time rejection of monthly/yearly) substantially overlaps with the existing E-01 error norm in §3.7, which already requires: "Implementation MUST reject with a descriptive error naming the unrecognized keyword" for monthly. Consider consolidating or cross-referencing to avoid two normative anchors for the same behavior.
💡 Details
§3.7 (pre-existing, unchanged):
E-01: Unknown schedule keyword (not one of
daily,weekly,hourly,bi-weekly,tri-weekly,every) —monthly— Implementation MUST reject with a descriptive error. Error code:UNKNOWN_KEYWORD.
R-SAFE-004 (new, §13.4):
Implementations MUST reject unsupported period types such as
monthlyandyearlyat parse time. The parser MUST return a deterministic validation error and MUST NOT silently coerce.
R-SAFE-004 adds the "MUST NOT silently coerce" and "deterministic" nuances, which are valuable. Rather than a standalone rule that duplicates scope, add a cross-reference: "(extends E-01 in §3.7 with determinism and no-coerce obligations)".
@copilot please address this.
| This specification is deprecated in favor of AI Credits (AIC). The deprecation lifecycle defines | ||
| sunset criteria and compatibility obligations while ET remains available for legacy consumers. | ||
|
|
||
| - **D-ET-001 (Sunset date and criteria)**: Beginning **2026-06-05** (deprecation date), ET MUST be |
There was a problem hiding this comment.
[/grill-with-docs] D-ET-001 states the deprecation began 2026-06-05 (26 days ago) but this spec is still marked as the active documentation. The section lacks a current status statement that would tell readers what state the spec is in right now — "active", "deprecated-and-maintained", or "sunset-imminent".
💡 Suggested addition
Add a status note at the top of the Deprecation Lifecycle section, e.g.:
Current status (as of publication): Deprecated — actively maintained for backward-compatible ET consumers. Removal is planned after the one-release-cycle warning period described in D-ET-002.
This removes ambiguity for readers who land on this spec and need to decide whether to build new ET-facing integrations.
@copilot please address this.
| invocation and MUST be reused consistently across console and JSON output paths. | ||
| - **N-FC-003 (Caller percentile obligations)**: Callers consuming forecast output MUST treat P10, P50, | ||
| and P90 as a set. Callers MUST NOT present P50 alone as a complete risk estimate and SHOULD include | ||
| percentile spread when using forecast data for budgeting or promotion decisions. |
There was a problem hiding this comment.
[/grill-with-docs] N-FC-003 places a MUST NOT obligation on callers, but this spec doesn't define what constitutes a "caller" or how compliance can be verified — making it an unenforceable normative requirement.
💡 Details
RFC 2119-style MUST/MUST NOT only work when there's a clear subject (implementation) and a testable criterion. "Callers MUST NOT present P50 alone" is a UX policy, not a testable property of the forecast engine itself.
Options:
- Downgrade to
SHOULD NOTwith a rationale note ("to avoid misleading risk estimates"). - Add a concrete test obligation: e.g., the implementation MUST include all three percentiles when producing machine-readable output (JSON), so callers can't present P50-only without actively discarding fields.
- Move caller guidance to an informative note rather than a normative requirement.
@copilot please address this.
| This section defines cross-cutting normative requirements that apply across sampling, projection, and | ||
| consumer-facing output behavior. | ||
|
|
||
| - **N-FC-001 (Minimum sample-size floor)**: Implementations MUST apply the minimum sample-size floor |
There was a problem hiding this comment.
[/grill-with-docs] N-FC-001 refers to the sample-size floor as being defined in "§7.6", but §7.6 uses SHOULD (R-MC-030) while N-FC-001 upgrades this to MUST. This unannounced strengthening of the requirement level should be called out explicitly to avoid implementation surprises.
💡 Details
R-MC-030 (§7.6): "Implementations SHOULD require a minimum of 10 ET observations..."
N-FC-001: "Implementations MUST apply the minimum sample-size floor in §7.6..."
If this upgrade from SHOULD to MUST is intentional, add a note: "N-FC-001 strengthens R-MC-030 from SHOULD to MUST for conforming implementations." If unintentional, align to SHOULD.
@copilot please address this.
|
|
||
| Normative requirements: **R-SAFE-009**, **R-SAFE-010** | ||
|
|
||
| ### 8.6 Deprecation-Era Conformance |
There was a problem hiding this comment.
[/grill-with-docs] §8.6 (Deprecation-Era Conformance) appears inside the §8 Safeguards section — between §8.5 and a safeguard definition — rather than near the new ## Deprecation Lifecycle section at the bottom of the file. Splitting the deprecation guidance between §8.6 and the end-of-file lifecycle section makes the deprecation story harder to follow.
💡 Suggested fix
Consider moving §8.6 content to become §2 of the Deprecation Lifecycle section at the end (or consolidate into a single ## Deprecation top-level section containing both the lifecycle rules and the era-conformance clause). Keeping both in one place avoids readers needing to jump between §8 and the end section.
@copilot please address this.
| | `steps` | Yes | Append | | ||
| | `jobs` | Yes | Append | | ||
| | `imports` | Yes | Track (ordered list) | | ||
| | `inputs` | Yes | Deep merge by input key | |
There was a problem hiding this comment.
[/codebase-design] The new field-selection merge table at §2.1 is a valuable addition, but it's placed under §2 (Field Selection) without the table being linked from the TOC, and the section heading #### 2.1 Field Selection and Merge Strategy Table uses heading level 4 (####) while its parent ### 2. Field Selection is level 3 — the numbering hierarchy is off.
💡 Details
The table heading should be ### 2.1 Field Selection and Merge Strategy Table (level 3, ###) to be consistent with other numbered subsections in this spec. As-is, the rendered heading is nested one level too deep. Also consider adding 2.1 Field Selection and Merge Strategy Table to the TOC at the top of the document so it's discoverable.
@copilot please address this.
|
|
||
| Sync follow-up tasks: | ||
|
|
||
| - Add/maintain compliance coverage for **T-MCP-051** in |
There was a problem hiding this comment.
[/tdd] The sync follow-up for T-MCP-051 says to add/maintain coverage in pkg/workflow/mcp_scripts_renderer_test.go, but T-MCP-051 was added to the compliance checklist with status Standard — implying it should already be tested. Leaving an [Open] task for a newly-declared compliance test creates a gap between the normative spec and the implementation at merge time.
💡 Suggested fix
Either:
- Change the compliance checklist entry for
T-MCP-051to statusPlanneduntil the test is implemented, so the checklist accurately reflects current state. - Or, add the actual test(s) for
SN-SCOPE-01–04in this PR before merging, so the new normative requirement lands with coverage.
A Standard checklist entry with an open sync task signals a regression risk — future authors may not realize the test is missing.
@copilot please address this.
There was a problem hiding this comment.
Review: SPDD spec-completeness gaps (ET, Forecast, Frontmatter Hash, MCP Scripts, Fuzzy Schedule)
The content additions are substantive and well-reasoned — the deprecation lifecycle terms, forecast norms, terminology glossary, calendar schema, and MCP security elevation are all good additions. However, there are four cross-cutting correctness issues that need fixing before merge:
Blocking issues
-
ET spec —
### 8.6interrupts### 8.5's subsections (effective-tokens-specification.md)
### 8.6 Deprecation-Era Conformanceis placed between### 8.5 Safeguardsand#### S-6:, which is a child of §8.5. This breaks heading hierarchy and orphans the last safeguard from its parent section. -
ET spec —
## Deprecation Lifecyclemissing from Table of Contents (effective-tokens-specification.md)
The new top-level section at line 989 is not listed in the ToC, making it undiscoverable by navigation. -
Fuzzy Schedule — non-contiguous
R-SAFE-*numbering in §8 (fuzzy-schedule-specification.md)
Renumbering old R-SAFE-004 to R-SAFE-005 leaves a gap in §8 (R-SAFE-001/002/003/005 with no 004). The new R-SAFE-004 appears in §13.4, not §8, creating inconsistency with the §13.2 operator-visible error-code table that attributes codes to §8. -
MCP Scripts —
SN-SCOPE-*vsN-SN-SCOPE-*ID conflict (mcp-scripts-specification.md)
§7.6.2 defines the rules asSN-SCOPE-01..04; the new §11 redefines them asN-SN-SCOPE-01..04. T-MCP-051, the compliance checklist, and Sync Notes all still reference the §7.6.2 IDs, making the §11 IDs orphaned and the cross-references inconsistent.
Non-blocking notes
- Frontmatter Hash —
inputsmerge strategy mismatch (frontmatter-hash-specification.md): §2.1 table says "Deep merge by input key" but §3.1 listsinputsunder Track (same asimports). Not blocking but should be aligned. - Forecast norms (§13) look correct and complete. N-FC-001/002/003 are well-scoped.
- Frontmatter Hash Terminology additions are clear and useful.
- Fuzzy Schedule §12.4 calendar schema is a solid addition; the field types and required/optional distinctions are appropriate.
Inline comments placed
- ET spec line 449: §8.5/§8.6 heading hierarchy
- ET spec line 989: ToC missing Deprecation Lifecycle
- Fuzzy Schedule line 1450: non-contiguous R-SAFE-* numbering
- Frontmatter Hash line 145: inputs merge strategy mismatch
- MCP Scripts line 1080: SN-SCOPE vs N-SN-SCOPE ID conflict
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 143.2 AIC · ⌖ 7.31 AIC · ⊞ 4.9K
| Implementations MUST NOT introduce new ET-only output fields, ET-only APIs, or ET-only extension | ||
| points. | ||
|
|
||
| #### S-6: Output NaN/Inf Guards, Large-Count Overflow, and Zero-Invocation Edge Case |
There was a problem hiding this comment.
Structural issue: ### 8.6 interrupts ### 8.5's subsections
### 8.6 Deprecation-Era Conformance (line 442) is inserted between ### 8.5 Safeguards and #### S-6: (this line 449). Since #### S-6 is a child of ### 8.5 Safeguards, placing ### 8.6 between them breaks the heading hierarchy — a reader following the document linearly will encounter §8.6 before seeing the last safeguard (S-6) that belongs to §8.5.
Consider moving ### 8.6 Deprecation-Era Conformance after all #### S-* blocks, so §8.5 is complete before §8.6 begins.
@copilot please address this.
|
|
||
| --- | ||
|
|
||
| ## Deprecation Lifecycle |
There was a problem hiding this comment.
## Deprecation Lifecycle not listed in Table of Contents
The new top-level section ## Deprecation Lifecycle (added at line 989) is not referenced in the ToC (§Table of Contents). The ToC ends at item 16 (Change Log) with no mention of Deprecation Lifecycle. This omission means readers cannot navigate to the new section from the ToC.
Add a ToC entry such as:
17. [Deprecation Lifecycle](#deprecation-lifecycle)
(and renumber the existing References/Change Log entries accordingly, or insert it before them.)
@copilot please address this.
|
|
||
| ### 13.4 Unsupported Period-Type Rejection | ||
|
|
||
| **R-SAFE-004**: Implementations MUST reject unsupported period types such as `monthly` and `yearly` |
There was a problem hiding this comment.
R-SAFE-004 gap in §8 Safeguards — numbering is non-contiguous
This PR renumbers the old §8 R-SAFE-004 (collision warning) to R-SAFE-005, but does not assign any rule to R-SAFE-004 within §8. The new R-SAFE-004 (unsupported period-type rejection) is introduced here in §13.4, leaving §8's safeguard list as R-SAFE-001, R-SAFE-002, R-SAFE-003, R-SAFE-005 — with no R-SAFE-004 in the canonical safeguards section.
This is confusing because:
- The §13.2 error-code table lists
R-SAFE-003andR-SAFE-005(from §8) but now also hasR-SAFE-004(from §13.4), making the table's "§8 ..." normative references inconsistent. - Readers expect all
R-SAFE-*IDs to originate in §8 Safeguards.
Recommendation: move R-SAFE-004 into §8 alongside the other safeguards, or use a distinct ID prefix (e.g., R-PARSE-001) for parse-time safety norms so §8 stays contiguous.
@copilot please address this.
| | `steps` | Yes | Append | | ||
| | `jobs` | Yes | Append | | ||
| | `imports` | Yes | Track (ordered list) | | ||
| | `inputs` | Yes | Deep merge by input key | |
There was a problem hiding this comment.
Inconsistency: inputs merge strategy differs between §2.1 table and §3.1
The new §2.1 field-selection table shows inputs with merge strategy "Deep merge by input key". However, §3.1 Merge Strategy (line ~160) lists inputs under the Track category alongside imports — not under Deep merge.
These two descriptions of the same field contradict each other. Please align them: either update §3.1 to say inputs uses deep merge (not just track), or revise the §2.1 table to say "Track" consistent with §3.1.
@copilot please address this.
|
|
||
| ### Secret Scope Norms | ||
|
|
||
| - **N-SN-SCOPE-01**: Secrets declared in workflow `env:` MUST be scoped to the minimum required |
There was a problem hiding this comment.
ID inconsistency: §7.6.2 uses SN-SCOPE-*, §11 uses N-SN-SCOPE-*
§7.6.2 (pre-existing) defines these rules as SN-SCOPE-01–SN-SCOPE-04, but the new §11 Norms section redefines them as N-SN-SCOPE-01–N-SN-SCOPE-04 — a different ID format. This creates two conflicting canonical names for the same four requirements.
Downstream references are already inconsistent:
- T-MCP-051 (line 1142) references
SN-SCOPE-01 through SN-SCOPE-04(§7.6.2 IDs). - The §12.2 compliance checklist (line 1187) uses
SN-SCOPE-01–04(§7.6.2 IDs). - §13 Sync Notes (line 1645) uses
SN-SCOPE-01 through SN-SCOPE-04(§7.6.2 IDs).
All three downstream references point to the old IDs, not the new N-SN-SCOPE-* IDs from §11.
Please either: (a) align §11 to use the same SN-SCOPE-* identifiers as §7.6.2, or (b) update §7.6.2 and all downstream references (T-MCP-051, the checklist, and Sync Notes) to the new N-SN-SCOPE-* IDs.
@copilot please address this.
🤖 PR Triage
Score breakdown: Impact 15 + Urgency 12 + Quality 10 Rationale: Closes spec-completeness gaps across 5 spec docs. All files are markdown. Multiple changes_requested from automated reviewer with 25 review comments — batch alongside other docs-spec PRs. Address reviewer feedback before merging.
|
|
@copilot merge main and recompile |
…ec-work-plan-2026-07-01 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main and recompiled all 258 workflows successfully (commit |
This updates the 2026-07-01 SPDD batch to resolve missing structural sections and normative coverage across five specs. It addresses deprecation lifecycle gaps, missing Norms/Terminology content, buried safeguards, incomplete sync follow-ups, and missing schema-level detail.
Effective Tokens (Deprecated lifecycle + conformance guardrails)
## Deprecation Lifecyclewith explicit sunset criteria, removal procedure, and backward-compat obligations (D-ET-001..004).Forecast (top-level norms + sync backlog)
## 13. NormswithN-FC-001..003covering sample-size floor, projection immutability, and required caller use of P10/P50/P90.Frontmatter Hash (terminology + field-selection completeness)
## Terminologyfor frontmatter, BFS traversal, diamond dependency, and canonical JSON.inputsto the field-selection merge table with explicit merge behavior.MCP Scripts (elevated safeguards/norms + compliance mapping)
## 10. Safeguardsand## 11. Normswith labeledS-*andN-*requirements.T-MCP-051for SN-SCOPE-01..04 in §12 and mapped follow-up linkage in Sync Notes topkg/workflow/mcp_scripts_renderer_test.go.Fuzzy Schedule (calendar schema depth + parse-time safety norm)
R-SAFE-004requiring parse-time errors for unsupported period types (monthly,yearly).R-SAFE-005.