Skip to content

Close SPDD conformance gaps for ADRs 27325/27327/27387/27476/27479#47637

Open
pelikhan with Copilot wants to merge 3 commits into
mainfrom
copilot/spdd-daily-spec-work-plan-2026-07-23
Open

Close SPDD conformance gaps for ADRs 27325/27327/27387/27476/27479#47637
pelikhan with Copilot wants to merge 3 commits into
mainfrom
copilot/spdd-daily-spec-work-plan-2026-07-23

Conversation

Copilot AI commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This PR addresses the SPDD queue gaps for five draft ADRs by adding missing conformance guards/tests and aligning comment-memory/safe-outputs specs with implemented behavior. It also promotes the covered ADRs to Accepted where implementation and verification now match normative requirements.

  • Compile-all frontmatter error-handling conformance (ADR-27387)

    • Added coverage for frontmatter-filter I/O failures to ensure read errors are surfaced as compile-time failures rather than silently skipped.
    • File: pkg/cli/commands_file_watching_test.go
  • Durable helper-organization guard (ADR-27325)

    • Added a regression test that enforces engine API target helpers stay out of awf_helpers.go and remain in engine_api_targets.go.
    • File: pkg/workflow/engine_api_targets_file_organization_test.go
  • Comment-memory prompt-size safeguards (ADR-27479)

    • Added hard caps during pre-agent materialization:
      • per-file cap: 16 KiB
      • aggregate directory cap: 48 KiB
    • Oversized memory now fails setup safely instead of inflating prompt/context size.
    • Files:
      • actions/setup/js/comment_memory_helpers.cjs
      • actions/setup/js/setup_comment_memory_files.cjs
      • actions/setup/js/setup_comment_memory_files.test.cjs
  • Safe-outputs needs/spec sync (ADR-27476)

    • Confirmed and documented schema alignment for safe-outputs.needs and added norms/safeguard language for cycle protection + schema/IDE sync expectations.
    • Files:
      • docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md
      • docs/src/content/docs/reference/safe-outputs.md
      • pkg/parser/schemas/main_workflow_schema.json (already aligned; documented explicitly)
  • ADR status and REASONS completion

    • Promoted ADRs 27325, 27327, 27387, 27476, 27479 from Draft → Accepted.
    • Added missing safeguards/norms and removed stale “draft” footer text in those ADR files.
const COMMENT_MEMORY_MAX_FILE_BYTES = 16 * 1024;
const COMMENT_MEMORY_MAX_TOTAL_BYTES = 48 * 1024;

const contentBytes = Buffer.byteLength(content, "utf8");
if (contentBytes > COMMENT_MEMORY_MAX_FILE_BYTES) {
  throw new Error(`...exceeding max ${COMMENT_MEMORY_MAX_FILE_BYTES} bytes`);
}
totalBytes += contentBytes;
if (totalBytes > COMMENT_MEMORY_MAX_TOTAL_BYTES) {
  throw new Error(`...exceeds max ${COMMENT_MEMORY_MAX_TOTAL_BYTES} bytes`);
}

Run: https://github.com/github/gh-aw/actions/runs/30042541462

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.1 AIC · ⌖ 9.05 AIC · ⊞ 7K ·
Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/30050062713

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.9 AIC · ⌖ 9.61 AIC · ⊞ 7K ·
Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/30057263862

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 32.9 AIC · ⌖ 8.04 AIC · ⊞ 7K ·
Comment /souschef to run again

Copilot AI linked an issue Jul 23, 2026 that may be closed by this pull request
11 tasks
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Review and implement gaps in daily spec work plan Close SPDD conformance gaps for ADRs 27325/27327/27387/27476/27479 Jul 23, 2026
Copilot AI requested a review from pelikhan July 23, 2026 16:48
@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Triage

Field Value
Category docs
Risk 🟡 Medium
Priority Score 52/100
Recommended Action batch_review

Score breakdown: Impact 22 + Urgency 15 + Quality 15

Rationale: Draft PR touching ADR docs, conformance specs, JS helpers, and tests for safe-outputs/comment-memory. Moderate scope across multiple subsystems. Groups well with #47633 as a docs/spec batch. No CI data available.

Generated by 🔧 PR Triage Agent · sonnet46 · 38.1 AIC · ⌖ 6.75 AIC · ⊞ 5.7K ·

@pelikhan
pelikhan marked this pull request as ready for review July 23, 2026 20:31
Copilot AI review requested due to automatic review settings July 23, 2026 20:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Closes SPDD conformance gaps through regression guards, bounded comment-memory materialization, and synchronized ADR/spec documentation.

Changes:

  • Adds Go conformance tests for compile-all errors and helper organization.
  • Enforces and tests comment-memory size limits.
  • Updates specifications and accepts five ADRs.
Show a summary per file
File Description
pkg/workflow/engine_api_targets_file_organization_test.go Guards helper placement.
pkg/cli/commands_file_watching_test.go Tests frontmatter read failures.
actions/setup/js/comment_memory_helpers.cjs Defines memory-size limits.
actions/setup/js/setup_comment_memory_files.cjs Enforces memory limits.
actions/setup/js/setup_comment_memory_files.test.cjs Tests oversized-memory handling.
docs/src/content/docs/specs/safe-outputs-specification.md Specifies memory safeguards.
docs/src/content/docs/reference/safe-outputs.md Documents schema integration.
docs/adr/27325-organize-workflow-helpers-by-semantic-responsibility.md Accepts helper-organization ADR.
docs/adr/27327-parser-utility-test-strategy.md Accepts parser-testing ADR.
docs/adr/27387-filter-non-frontmatter-markdown-during-compile-all.md Accepts compile-all filtering ADR.
docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md Adds safeguards and accepts ADR.
docs/adr/27479-comment-memory-file-based-agent-memory-with-github-persistence.md Adds size norms and accepts ADR.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md:80

  • This normative statement names a validator that cannot inspect safe-outputs.needs: detectSafeJobCycles only traverses custom jobs under safe-outputs.jobs, and it runs before job assembly. Require the full JobManager.ValidateDependencies pass instead, which is the existing gate that can detect cycles involving the consolidated job.
1. Safe-output custom job cycle detection **MUST** continue to run after `safe-outputs.needs` merge/validation so that new dependency edges cannot introduce cycles.
  • Files reviewed: 12/12 changed files
  • Comments generated: 5
  • Review effort level: Medium

}
let totalBytes = 0;
for (const [memoryId, content] of memoryMap.entries()) {
const contentBytes = Buffer.byteLength(content, "utf8");
When `tools.comment-memory` is enabled, implementations MUST support this additional data-flow path:

1. **GitHub comment → local files (pre-agent setup)**: A setup step reads the managed comment body from the target issue or pull request, extracts content between `<gh-aw-comment-memory id="...">` and `</gh-aw-comment-memory>`, and writes one file per memory entry under `/tmp/gh-aw/comment-memory/<memory_id>.md`.
- Setup MUST enforce memory-size safeguards: each materialized file MUST be ≤16 KiB and the aggregate materialized directory MUST be ≤48 KiB. Exceeding either limit MUST stop setup with an error.
Comment thread pkg/cli/commands_file_watching_test.go Outdated
Comment on lines +239 to +246
brokenWorkflow := filepath.Join(workflowsDir, "broken.md")
err = os.WriteFile(brokenWorkflow, []byte("---\n"), 0o644)
require.NoError(t, err)
err = os.Chmod(brokenWorkflow, 0o000)
require.NoError(t, err)
t.Cleanup(func() {
_ = os.Chmod(brokenWorkflow, 0o644)
})
Comment on lines +17 to +23
forbiddenFunctionSignatures := []string{
"func extractAPITargetHost(",
"func extractAPIBasePath(",
"func GetCopilotAPITarget(",
"func GetGeminiAPITarget(",
"const DefaultGeminiAPITarget",
}

### Safeguards

- Dependency cycles introduced through `safe-outputs.needs` are prevented by keeping custom safe-job cycle detection (`detectSafeJobCycles`) as a required compile-time validation gate.
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, address unresolved review feedback, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.1 AIC · ⌖ 9.05 AIC · ⊞ 7K ·
Comment /souschef to run again

…, ADR citations

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, address unresolved review feedback, and rerun checks once the branch is up to date.

...

Addressed all five unresolved review threads in commit Address reviewer feedback: byte counting, spec sync, test portability, ADR citations:

  1. Byte counting off-by-one (setup_comment_memory_files.cjs:151): Fixed — validation now measures ${content}\n (the exact string written to disk). Added a new edge-case test that specifically exercises content of exactly 16 KiB, which passes the old check but triggers correctly under the fixed measurement.

  2. Spec/implementation mismatch (safe-outputs-specification.md:997): Updated the normative text from "MUST stop setup with an error" to "MUST emit a warning and skip writing all memory files", matching the core.warning + skip behavior that main() actually implements.

  3. chmod-000 portability (commands_file_watching_test.go:239-246): Replaced with a directory named broken.md. os.Open succeeds on a directory but any subsequent Read returns EISDIR, deterministically exercising the I/O error propagation path without needing privileges or permission-bit portability.

  4. Incomplete engine-target guard (engine_api_targets_file_organization_test.go:17-23): Expanded the forbidden-signature list to cover all declarations owned by engine_api_targets.go: extractAPITargetAuthHeader, extractLiteralEngineEnvHost, GetCopilotAllowlistTargets, GetAntigravityAPITarget, getEngineAPIHosts, and DefaultAntigravityAPITarget.

  5. Wrong cycle-detection safeguard (27476-extend-safe-outputs-dependencies-via-needs-field.md:46,80): Fixed both the Safeguards narrative (line 46) and the normative requirement (line 80) to cite JobManager.ValidateDependencies — which validates the fully assembled job graph including safe-outputs.needs edges — instead of detectSafeJobCycles, which only operates on data.SafeOutputs.Jobs.

Copilot AI requested a review from gh-aw-bot July 23, 2026 21:00
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, review the latest reply that says all five review threads are addressed, verify the current unresolved review comments are resolved, and rerun checks once the branch is up to date.

Run: https://github.com/github/gh-aw/actions/runs/30046537550

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 11.7 AIC · ⌖ 9.18 AIC · ⊞ 7K ·
Comment /souschef to run again

Copilot AI commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, review the latest reply that says all five review threads are addressed, verify the current unresolved review comments are resolved, and rerun checks once ...

Confirmed all five review threads are addressed in commit 8f4b7f2:

  1. Byte counting (setup_comment_memory_files.cjs): Validation now measures writtenContent = \${content}\n`` — the exact string written to disk.
  2. Spec/implementation sync (safe-outputs-specification.md:997): Updated to "MUST emit a warning and skip writing all memory files", matching the core.warning + skip behavior.
  3. chmod-000 portability (commands_file_watching_test.go): Replaced with a directory named broken.md; Read on a directory returns EISDIR deterministically without privilege or portability dependencies.
  4. Engine-target guard completeness (engine_api_targets_file_organization_test.go): Expanded forbidden-signature list to cover all helpers owned by engine_api_targets.go: extractAPITargetAuthHeader, extractLiteralEngineEnvHost, GetCopilotAllowlistTargets, GetAntigravityAPITarget, getEngineAPIHosts, DefaultAntigravityAPITarget, DefaultGeminiAPITarget.
  5. ADR cycle-detection citation (27476-…md:46,80): Both the Safeguards narrative and the normative requirement now cite JobManager.ValidateDependencies.

Local validation: make fmt ✅, make lint ✅ (0 errors), JS vitest (11/11) ✅, new Go tests ✅. Pre-existing failures in pkg/actionpins and container-pin tests in pkg/workflow are unrelated to these changes (they fail identically on main).

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify the latest reply resolved the remaining review feedback, and rerun checks once the branch is up to date.

Run: https://github.com/github/gh-aw/actions/runs/30050062713

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.9 AIC · ⌖ 9.61 AIC · ⊞ 7K ·
Comment /souschef to run again

Copilot AI commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, verify the latest reply resolved the remaining review feedback, and rerun checks once the branch is up to date.

Run: https://github.com/github/gh-aw/actio...

All five review threads are addressed in commit 8f4b7f2 — GitHub marks them as outdated (the diff lines they referenced no longer exist in current HEAD). Local validation is clean: make fmt ✅, make lint ✅ (0 errors), make recompile ✅, JS vitest 11/11 ✅, relevant Go tests ✅.

The CI run in your link (30050062713) is the PR Sous Chef workflow on main — it failed on a dismiss_pull_request_review step due to a reviewer-identity permission mismatch (github-actions[bot] vs pelikhan), which is unrelated to this PR's code changes.

Pre-existing test failures in pkg/actionpins and container-pin tests in pkg/workflow exist on main independently of this PR.

Ready for human review and CI re-trigger — no further code changes needed.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Triage — §30057272009

Field Value
Category docs
Risk 🟢 Low
Priority Score 72 / 100
Action auto_merge
CI ✅ Passing
Review ✅ Human-approved (pelikhan)
Batch spec-docs

Score breakdown: Impact 35 + Urgency 20 + Quality 17

Rationale: Docs-only conformance promotion. Human reviewer approved. All CI checks pass. Safe for auto-merge.

Next action: Auto-merge when ready.

Generated by 🔧 PR Triage Agent · sonnet46 · 35.7 AIC · ⌖ 5.3 AIC · ⊞ 5.7K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify the latest reply resolved the remaining review feedback, and rerun checks once the branch is up to date.

Unresolved review threads (newest first):

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 32.9 AIC · ⌖ 8.04 AIC · ⊞ 7K ·
Comment /souschef to run again

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, verify the latest reply resolved the remaining review feedback, and rerun checks once the branch is up to date.

Unresolved review threads (newest first):...

All five code issues are addressed in commit 8f4b7f2 — GitHub marks all threads as outdated (changes have landed). No further code changes are needed.

The CI failure (run 30057263862) is not from this PR's changes. The PR Sous Chef workflow is failing on resolve_pull_request_review_thread mutations: the threads were created by copilot-pull-request-reviewer and cannot be resolved by the workflow's token. This is a permissions mismatch in the PR Sous Chef workflow itself.

Action needed from a human maintainer: please manually resolve the five outdated review threads in the GitHub UI. Once resolved, the PR is ready to merge (local validation: make fmt ✅, make lint ✅, make recompile ✅, JS vitest 11/11 ✅, Go tests ✅).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[spdd] Daily spec work plan - 2026-07-23

4 participants