SPDD 2026-07-25: strengthen normative specs across ADRs 27479, 27523, 27626, 27639, 27707 - #48041
Merged
Merged
Conversation
11 tasks
…utputs.md Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Review daily SPDD spec work plan for ADR specifications
SPDD 2026-07-25: strengthen normative specs across ADRs 27479, 27523, 27626, 27639, 27707
Jul 25, 2026
pelikhan
marked this pull request as ready for review
July 25, 2026 16:44
Contributor
There was a problem hiding this comment.
Pull request overview
Strengthens five ADR specifications, promotes them to Accepted, and documents allowed-domains dual sanitization scope.
Changes:
- Adds normative safeguards, synchronization, sequencing, migration, and tie-break requirements.
- Documents formal GitHub access-mode entities and invariants.
- Clarifies input and output domain sanitization behavior.
Show a summary per file
| File | Description |
|---|---|
docs/adr/27479-...md |
Adds comment-memory safeguards and synchronization norms. |
docs/adr/27523-...md |
Adds warning-initialization operation requirements. |
docs/adr/27626-...md |
Specifies firewall migration and numeric normalization. |
docs/adr/27639-...md |
Marks allowed-domain parity ADR Accepted. |
docs/adr/27707-...md |
Defines mode/type precedence and entities. |
docs/src/content/docs/reference/safe-outputs.md |
Documents dual sanitization scope. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Medium
Comment on lines
+62
to
+65
| ### Operations | ||
|
|
||
| 1. `initWarnings(ctx)` **MUST** be called before any write to `ctx.Warnings` within every code path in `ResolveActionPin`, including early-return branches. | ||
| 2. Every code path in `ResolveActionPin` **MUST NOT** write to `ctx.Warnings` without a prior call to `initWarnings`; inline `if ctx.Warnings == nil` guards **MUST NOT** be used as a substitute. |
|
|
||
| 1. The `normalizeFirewallVersion` helper **MUST** handle all numeric YAML types that a YAML parser may produce for a bare numeric version value, including at minimum: `int` (and its sized variants `int8`, `int16`, `int32`, `int64`), `uint` (and its sized variants `uint8`, `uint16`, `uint32`, `uint64`), `float32`, and `float64`. | ||
| 2. For each of these numeric types, the helper **MUST** convert the value to its canonical decimal string representation before writing it as `sandbox.agent.version`. | ||
| 3. Any future addition of YAML numeric sub-types (e.g., `complex128`) **MUST** be handled explicitly rather than falling through to a default that could silently lose precision or produce a malformed version string. |
| |---|---|---| | ||
| | `tools.github.mode` | `"gh-proxy"` \| `"local"` \| `"remote"` | `"gh-proxy"` activates CLI-proxy guidance and suppresses MCP server registration. `"local"` and `"remote"` select MCP transport (superseded by `tools.github.type` when both are set). Absent: fall back to `features.cli-proxy`. | | ||
| | `tools.github.type` | `"local"` \| `"remote"` | When present, exclusively determines MCP transport type, overriding the transport-selector meaning of `tools.github.mode`. | | ||
| | `features.cli-proxy` | `true` \| `false` (boolean) | Legacy access-mode flag. Evaluated only when `tools.github.mode` is absent. `true` is semantically equivalent to `tools.github.mode: gh-proxy`. | |
|
|
||
| **Domain Filtering** (`allowed-domains`): Controls which domains are allowed in URLs. URLs from other domains are replaced with `(redacted)`. Accepts specific domain strings or [ecosystem identifiers](/gh-aw/reference/network/#ecosystem-identifiers): | ||
|
|
||
| > **Note:** `safe-outputs.allowed-domains` applies to **both** output sanitization (the domains the agent may reference in its outputs) and input sanitization (the `sanitized` activation step that redacts URLs in incoming issue/PR text before passing it to the agent). Domains listed here are therefore permitted in both directions. |
Contributor
|
🎉 This pull request is included in a new release. Release: |
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.
Five Draft ADRs had weak or missing normative sections in Part 2, and
safe-outputs.mddidn't document the dual scope ofallowed-domains. This batch addresses the P0/P1 spec gaps and promotes all five to Accepted.ADR-27479 — Comment Memory
### Safeguardsto Part 2: MUST constraints for the 16 KiB per-file and 48 KiB total caps, with source pinned toactions/setup/js/comment_memory_helpers.cjs### Normsto Part 2: constants, spec, andsetup_comment_memory_files.test.cjsMUST stay in syncADR-27523 — actionpins helpers
### Operationsto Part 2:initWarnings(ctx)MUST be called before anyctx.Warningswrite in allResolveActionPincode paths; inline nil-guards MUST NOT substituteADR-27626 — sandbox.agent.version
versionkey emitted when migratingfalse/"disable"forms### Norms: MUST handle all YAML numeric types (int,int8–int64,uint,uint8–uint64,float32,float64) with string normalization before writingsandbox.agent.versionADR-27639 — allowed-domains parity
docs/src/content/docs/reference/safe-outputs.md:safe-outputs.allowed-domainsapplies to both output sanitization and thesanitizedinput activation step — previously undocumented dual scopeTestComputeTextStepIncludesAllowedDomainsEnvconfirmed inpkg/workflow/compute_text_lazy_test.go:434, verifyingGH_AW_ALLOWED_DOMAINSin the compiledsanitizedstepADR-27707 — tools.github.mode unification
### Mode × Type Tie-Break: whentools.github.modeislocal/remoteandtools.github.typeis also set,tools.github.typeMUST win for transport selection### Entitiestable enumeratingtools.github.mode,tools.github.type, andfeatures.cli-proxywith their value domains and invariants