Fix runtime-mask parsing and preserve safe-output schemas

[pelikhan]

Summary

Originally split the secret-parsing/redaction portion from #69235 into a standalone PR against main, transferring the five reviewed files from b681ac5f07cb0e8f3a6648fc3904c69a250681f7 in the initial split commit. Subsequent user requests added parser fuzz tests and the safe-output schema fix investigated on this PR. Backend ownership/path changes remain in #69235; this PR excludes gateway ownership, tools.md, and the ownership changeset.

• Use an iterative quoted-token scanner for multi-megabyte JSON/mixed records; resume after rejected spans instead of quadratic rescanning. Protect original numeric lexemes before JSON parsing can round them.
• Recover encoded masks behind malformed quotes by replacing only matching original source spans. Ambiguous records retain their JSON-string representation; their decoded contents preserve unrelated Windows paths and escape sequences. Records without additional matches remain unchanged.
• Resolve configured, built-in, and runtime credentials together before replacement/encoding, coalescing overlaps so a short mask cannot expose a longer secret's suffix. Preserve JSON whitespace, untouched escapes, and numeric lexemes in ordinary artifacts.
• Preserve executable safe-output public keys, aliases, and enums from bundled tool schemas, including collector configuration/validation vocabulary. Free-form payloads and arbitrary nested keys remain redactable; ordinary artifacts have no protocol exemptions.
• Fail closed on malformed executable records, unsafe custom routing changes, or control-data conflicts rather than publishing corrupted messages. Existing cleanup/upload gates, backend ownership behavior, independent timeouts, and step-summary limits remain unchanged.

Exact scope

• actions/setup/js/add_mask_redaction.cjs
• actions/setup/js/add_mask_redaction.test.cjs
• actions/setup/js/redact_secrets.cjs
• actions/setup/js/redact_secrets.test.cjs
• actions/setup/js/safe_output_redaction.cjs
• actions/setup/js/safe_output_redaction.test.cjs
• actions/setup/js/runtime_mask_publication.test.cjs
• docs/src/content/docs/reference/imports.md
• .changeset/patch-large-record-mask-redaction.md

Markdown overview

The imports reference documents stack-safe large-record parsing, bounded malformed-token recovery, source-span-preserving escape recovery, combined credential matching, schema-aware safe outputs, and fail-closed conflicts. The patch changeset covers parser reliability, overlapping/encoded credential redaction, and preservation of safe-output schemas/collector configuration. GitHub's step-summary limit remains unchanged.

Validation

Latest review-fix commit: 5ff7b3848c7e879766194b56defb518a0d763ca5.

• 731 tests passed across 11 suites covering parser/redaction, schema-safe outputs, publication, collector, review handlers, config redaction, HTML sessions, evaluator redaction, and unchanged gateway behavior.
• make fmt-cjs, make lint-cjs, and final make agent-report-progress passed. Final gate includes build, scoped lint, JS typecheck, schema freshness, and 441 impacted JavaScript tests; no changed Go files required Go tests. Full lint retains existing warnings, with no errors.
• Bounded inspection regression: 1,024/2,048 malformed escaped quotes now require 1,031/2,055 character inspections rather than approximately 526,000/2.1 million. Includes multi-megabyte malformed records, path/escape preservation, decoded configured-secret assertions, crossing overlaps, and fail-closed decoding errors.
• Retains 7,000 reproducible parser cases across five seeds, exhaustive truncation/boundary cases, original numeric lexemes, 256 escape combinations, 4,096-token records, and private 0600-file coverage.
• Schema/publication coverage includes all 62 shipped tools / 338 declared properties, aliases/enums, short/public-word masks, custom-routing/control conflicts, and malformed source removal. Synthetic end-to-end coverage executes redaction, the actual collector, and both real review handlers against mocked GitHub; the existing publication expectations remain unchanged.
• Local synthetic baseline comparison: original main helper throws RangeError for 10 MiB JSON and mixed records; the revised helper succeeds. A 1 MiB mixed record succeeds with both.

Run investigation and evidence limits

Ponytail Reviewer run 38855075860 completed redaction successfully but failed safe-output publication. Published artifact schema metadata showed renamed body/pull_request_number fields and an altered review-submission type, explaining missing-body/no-handler failures. Synthetic masks reproduced that structural corruption on both main and the earlier PR revision; the source and legitimacy of the historical mask remain unverified.

The separate historical run 38804538442 reported redact/unknown, not EACCES; its removed source artifact and absent exception details prevent definitive attribution to the local stack-failure reproduction. No Docker startup, live workflow validation, workflow dispatch/reruns, secret lookup, or raw-log inspection was performed. Local integration uses temporary files and mocked GitHub, not hosted end-to-end revalidation.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

Design Decision Gate 🏗️

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Design Decision Gate 🏗️ · opus50 · 15.1 AIC · ⌖ 40.6 AIC · ◇ 26.8 AIC · ⊞ 1.9K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Test Quality Sentinel · haiku45 · 30.6 AIC · ⌖ 6.83 AIC · ◇ 3.17 AIC · ⊞ 9.2K · ◷

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

❌ Ponytail Reviewer failed to deliver outputs. Please review the logs for details.

Generated by Ponytail Reviewer for #69244

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 88/100 — Excellent

Analyzed 117 test cases across 2 JavaScript test suites: strong behavioral contracts, comprehensive edge case coverage, and deterministic fuzz testing.

📊 Metrics (117 tests)

Metric	Value
Test files	2 (add_mask_redaction.test.cjs, redact_secrets.test.cjs)
Total tests	117 (39 + 78)
Expect assertions	412 (118 + 294)
✅ Design/behavioral	78 (67%)
⚠️ Implementation-detail	28 (24%)
🎯 Edge/error coverage	11 (9%)
Table-driven (it.each)	41 (35%)
Seeded fuzz iterations	2,500 (5 seeds × 500 iterations)
External I/O mocks	22 (vi.spyOn(fs/JSON/env)) — legitimate
🚨 Violations	0

✨ Highlights

add_mask_redaction.test.cjs (39 tests, 118 assertions)

Behavioral Contracts:

• transformJSONStrings: Token/span callback separation with truncation boundaries (7 tests, 15 assertions)
• Seeded parser fuzz: 5 deterministic seeds validate:
  • Random insertions/deletions/truncations match bounded grammar (500 iterations/seed)
  • Nested JSON/JSONL redaction with escaped masks (300 iterations/seed)
  • Malformed quote recovery without rewriting unmatched records (300 iterations/seed)
  • Numeric lexeme preservation during masking (300 iterations/seed)
• redactArtifactMaskedValues boundaries: 17 test cases for JSON string scanner edge cases
  • Control character recovery (32 cases)
  • Escaped character recovery (15 cases)
  • Malformed Unicode recovery (12 cases)
  • Conservative quote pairing after terminated malformed tokens
  • Byte-for-byte preservation for unmatched/truncated input

Error Coverage:

• Callback failures propagate without truncation
• Truncated input never partially transforms
• Regex metacharacter escaping
• Large mask handling (200,000 characters)
• Overlapping mask priority (leftmost/supplied order)
• No cross-replacement scanning
• Performance: 500-mask redaction with 2M+ intermediate space stays under search budget

redact_secrets.test.cjs (78 tests, 294 assertions)

Behavioral Contracts:

• OTLP credential extraction and redaction (3 variants: env, headers, endpoints)
• Built-in pattern detection: GitHub, Azure, Google/GCP, AWS, OpenAI, Anthropic, Linear tokens
• File handling: large nested file scans (150K+ entries) without stack overflow
• Artifact retention: multi-megabyte files preserved with redaction instead of fail-closed
• Step-summary and summary content redaction
• 256 deterministic escape-combination tests
• 4,096-token record handling

Error Coverage:

• File permission errors (EACCES, EPERM, ENOSPC) — secret-safe diagnostics
• Parse failures with error boundary isolation
• Malformed JSON with conservative recovery
• ReDoS protection patterns tested
• AWF_RESOLVED_CONFIG environment variable isolation
• Write boundary failures without secret leakage

Test Infrastructure:

• Deterministic temp directory setup/cleanup
• Mock filesystem isolation (fs.existsSync, fs.readFileSync, fs.writeFileSync, fs.readdirSync, fs.renameSync, fs.rmSync)
• Environment variable stubbing/restoration
• JSON.parse mocking for failure injection
• Core action mocks (debug, info, notice, warning, error, setFailed, summary)

Verdict

✅ APPROVED. Test Quality Score 88/100 (Excellent).

• Implementation-detail ratio: 24% (threshold: 30%) ✓
• No violations (build tags, mock libraries) ✓
• Deterministic fuzz testing (2,500 iterations) validates parser invariants ✓
• Behavioral contracts cover masking, escaping, error recovery, and performance ✓
• Edge case boundaries exhaustively tested (truncation, malformed JSON, Unicode, permissions) ✓

🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 30.6 AIC · ⌖ 6.83 AIC · ⊞ 9.2K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 88/100 (Excellent). 24% implementation tests (threshold: 30%). Comprehensive seeded fuzz testing (2,500 iterations), boundary condition coverage, and error recovery paths validated. No violations.

🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 30.6 AIC · ⌖ 6.83 AIC · ⊞ 9.2K
_{Comment /review to run again}

[github-actions]

Verdict

No blocking correctness issues stood up in the changed scanner/redaction path or the new failure-mode coverage.

Highlights

Reviewed the new transformJSONStrings scanner, the conservative malformed-quote fallback, large-record handling, and the added parser/file-handling tests. I did not find a changed-line bug or regression strong enough to block merge.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.7 AIC · ⌖ 8.36 AIC · ⊞ 20.2K
_{Comment /review to run again}

[github-actions]

Skills-based review summary

Applied /diagnosing-bugs (bug-fix classification via pr-triage) and /tdd to the core parser change in add_mask_redaction.cjs.

Found one correctness issue worth fixing before merge: the conservative-escape-decode fallback (used when a malformed quote makes pairing ambiguous) unescapes \t \n \r \b \f \\ \" \uXXXX across the whole line, not just the span needed to reveal a hidden mask. This can corrupt unrelated plain text containing backslash sequences (e.g. Windows-style paths) whenever the conservative path is triggered elsewhere on the same line. See inline comment for a concrete repro; none of the new fuzz/boundary tests combine a conservative-re-encode trigger with unrelated backslashes in surrounding text, so this slipped past the otherwise very strong (444-test) coverage.

Test coverage itself is excellent — the new scanner-boundary and seeded-fuzz suites are a textbook /tdd application (oracle comparison, truncation boundaries, 7000+ generated cases). The throw-on-malformed-token change in the mixed-log path is correctly exercised by a dedicated processFile fail-closed test.

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 158.7 AIC · ⌖ 15.1 AIC · ⊞ 11.7K · ◷
_{Comment /matt to run again}

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd (via pr-triage: change_type=bug_fix) to the iterative JSON-token scanner and conservative escape-decoding logic in add_mask_redaction.cjs.

Requesting changes on one confirmed correctness bug in the conservative re-encode path; everything else — the scanner itself, numeric-token matching, and the new test suite — is solid.

📋 Key Themes & Highlights

Key Themes

• Scope bug in conservative unescape: when a malformed quote forces the fallback 'unescape once, then re-mask' path, the unescape regex applies to the entire line, not just the region needed to reveal the hidden mask. Unrelated backslash sequences in plain text (e.g. \t, \f, \n inside a Windows path) get silently decoded into control bytes, which then survive the JSON.stringify re-encode as real control characters — a data-corruption regression, not just a redaction miss.
• No fuzz/boundary test currently combines a conservative-re-encode trigger with unrelated backslash sequences elsewhere on the line, so the otherwise excellent coverage (444 tests, 7 boundary cases, 20 seeded fuzz runs across 7000+ cases) didn't catch it.

Positive Highlights

• ✅ transformJSONStrings cleanly replaces the regex-stack-exhausting alternation with a linear scanner; the boundary/truncation test matrix (only transforms a complete token at every possible truncation boundary) is a model /tdd red-green-refactor artifact.
• ✅ The new throw on malformed mixed-log tokens correctly triggers the existing fail-closed processFile cleanup path, and that behavior change is directly tested (removes the artifact if decoding...fails).
• ✅ Docs (imports.md) and the changeset accurately describe the byte-preservation guarantee — which makes the one found gap more important to close before merge, since it contradicts that documented guarantee for a narrow case.

See inline comment for a concrete repro and suggested fix (scope the unescape to only the matched mask span, or verify re-encoded output round-trips before accepting it).

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 158.7 AIC · ⌖ 15.1 AIC · ⊞ 11.7K
_{Comment /matt to run again}

[Copilot]

🟡 Changes recommended

Confirmed secret-redaction correctness and malformed-record performance issues remain unresolved.

2 open findings

• Whole-record serialization bypasses configured-secret redaction · New
• Restarting scans at start + 1 causes quadratic rescanning · New

What changed in this PR

Updates runtime-mask redaction for large and malformed artifacts, separately from the backend-ownership fix in #69235.

Changes:

• Replaces quoted-string regex scanning with an iterative scanner.
• Adds conservative escape decoding and boundary, fuzz, and artifact-handling tests.
• Documents the behavior and adds a patch changeset.

File	Description
docs/​src/​content/​docs/​reference/​imports.md	Documents malformed-record handling and byte preservation.
actions/​setup/​js/​redact_secrets.test.cjs	Adds artifact and fail-closed regression tests.
actions/​setup/​js/​add_mask_redaction.test.cjs	Adds scanner boundaries, seeded fuzzing, and large-record coverage.
actions/​setup/​js/​add_mask_redaction.cjs	Implements iterative scanning and conservative decoding.
.changeset/​patch-large-record-mask-redaction.md	Records the redaction patch.

🧠 Review effort: Max

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[pelikhan]

In reply to this status report: acknowledged as informational; no actionable ADR finding is stated, so no separate ADR change is warranted. The reported no-op is not treated as proof of completion or publication.

[pelikhan]

In reply to this status report: acknowledged; the reported no-op does not establish completion or publication. The updated redaction behavior is validated separately with local regression tests.

[pelikhan]

In reply to this completion announcement: no separate code change is needed for the status message. The substantive skills-review findings are addressed with the parser fixes and regression tests.

[pelikhan]

In reply to this failed-delivery report: investigation located the failure in Process Safe Outputs after successful agent/redaction steps, and commit 116a128 adds schema-safe redaction with synthetic collector/review-handler coverage. No hosted rerun was performed, and the source of the historical runtime mask remains unverified.

[pelikhan]

In reply to this completion announcement: acknowledged as informational; the corresponding review reported no blocking correctness findings. No additional code change is requested by this status message.

[pelikhan]

In reply to this test-quality assessment: the positive assessment applies to an earlier revision, not the latest changes. Existing behavioral/fuzz coverage is retained, with added path-preservation, bounded-scanning, configured-secret encoding and overlap regressions validated locally.

[pelikhan]

In reply to this skills-review summary: accepted and fixed conservative decoding to replace only matching source spans, preserving unrelated Windows paths and escapes in the decoded record. Added focused and seeded regressions while retaining fail-closed decoding behavior.

[gh-aw-bot]

@copilot address the following outstanding work in one pass:

1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
2. Fix failing check JS Tests (shard 4/4) (FAILURE): https://github.com/github/gh-aw/actions/runs/38860848383/job/116842820603.

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: 7106608
Sous-chef work: 31f7e812d45f0792093269b7b90445aa9803225a204ec130694b9ceb0227d7cc
Sous-chef state: d2a65d5b21558856597b163d1f3100d2c6f4eee83b41adc3447194cd174ede60

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 5.04 AIC · ⌖ 6.97 AIC · ⊞ 1.1K · ◷
_{Comment /souschef to run again}

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: the completion announcement requires no additional code change.

[pelikhan]

Accept: Regarding this acknowledgment, the earlier score and 463-test run remain revision-specific evidence. They do not prove complete current deployment closure.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: matching source spans are replaced without rewriting unrelated paths or escapes.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed; no additional ADR work is requested.

[pelikhan]

Accept: Regarding this acknowledgment, the recorded local test result remains historical. Complete current dependency closure is not inferred from it.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: the parser finding is already fixed and no additional change is requested.

[pelikhan]

Accept: Regarding this acknowledgment, agreed: local schema-safe regressions verify the synthetic behavior, not historical mask provenance or hosted publication.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed; no new code change is requested.

[pelikhan]

Accept: Regarding this acknowledgment, the earlier 463-test run is useful historical coverage. It is not complete current deployment closure or hosted validation.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: the source-span path-preservation fix is already covered.

[pelikhan]

Partially accept: Regarding this refactor acknowledgment, the module split and direct/runtime coverage remain implemented. The historical bundle checks did not establish complete dependency closure; the newly found JSON omission is handled separately.

[pelikhan]

Accept: Regarding this acknowledgment, the requested module refactor is already present. Historical deployment-list tests are not proof of complete current closure.

[pelikhan]

Accept: Regarding this acknowledgment, the earlier JavaScript bundle omission and main update were already addressed. The later diagnostic-test failure was independently reproduced on main in the prior investigation.

[pelikhan]

Partially accept: Regarding this acknowledgment, the c350350 fix and subsequent shard 4 success remain valid historical evidence. The earlier closure claim was narrower than complete current deployment coverage, and the new JSON omission is handled separately.

[pelikhan]

Accept: Regarding this acknowledgment, agreed: matching fail-closed boundaries do not prove identical causes or current recovery.

[pelikhan]

Accept: Regarding this acknowledgment, the historical MCP startup failure remains cause-unverified. No additional defect or current recovery follows from that startup notice.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: Copilot Auto succeeded on c350350, not necessarily on later revisions.

[pelikhan]

Accept: Regarding this acknowledgment, the Pi conclusion reported incomplete work. That historical status does not establish a parser defect.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: Gemini exit code 41 alone does not establish a redaction or parser defect.

[pelikhan]

Accept: Regarding this acknowledgment, agreed: the Codex CLI failure and unknown redaction failure remain distinct historical observations. Neither proves current failure or recovery.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: the captured container failure was fail-closed redaction, not demonstrated missing tools. Its root cause and current recovery remain unverified.

[pelikhan]

Accept: Regarding this acknowledgment, the converter fixture correction and historical main update are already addressed. Those focused checks are not a blanket current deployment guarantee.

[pelikhan]

Accept: Regarding this acknowledgment, confirmed: the poem requests no code change and the associated smoke success remains historical.

[pelikhan]

Accept: Regarding this acknowledgment, the converter fixture issue is already corrected. The recorded later diagnostic regression was independently reproduced on main, not newly investigated here.

[pelikhan]

Accept: Regarding this request, invalid ordinary JSON now uses whole-artifact text-mode matching, including raw and encoded credentials across malformed lines. Valid JSON/JSONL keeps structured handling, while malformed executable safe-output records remain fail-closed.

[pelikhan]

Partially accept: Regarding this implementation summary, the requested text fallback is present, but 621023d needed subsequent hardening for raw and multiline encoded credentials. ddec951 supplies that coverage while retaining fail-closed executable records.

[pelikhan]

Accept: Regarding this request, 6446862 uses combined credential matching for safe-output payloads with decodedText mode, preserving literal decoded strings while coalescing overlaps. The main update is already recorded in 176d567; no repeat merge is needed.

[pelikhan]

Accept: Regarding this completion summary, verified that the combined decoded-text matcher and 176d567 main merge are present. The saved local overlap gate passed 457 tests; this is not a hosted pass or proof of complete deployment closure.