Repository navigation
Conversation
Split the secret-parsing and redaction changes from #69235 without backend ownership or path changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise 7,000 reproducible generated cases across token mutations, escaped nested and mixed records, malformed quotes, and high-precision numeric lexemes; check every truncation boundary for representative quoted tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ 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: 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Ponytail Reviewer failed to deliver outputs. Please review the logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (117 tests)
✨ Highlights
|
There was a problem hiding this comment.
✅ 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
There was a problem hiding this comment.
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
Skills-based review summaryApplied Found one correctness issue worth fixing before merge: the conservative-escape-decode fallback (used when a malformed quote makes pairing ambiguous) unescapes Test coverage itself is excellent — the new scanner-boundary and seeded-fuzz suites are a textbook @copilot please address the review comments above.
|
There was a problem hiding this comment.
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,\ninside a Windows path) get silently decoded into control bytes, which then survive theJSON.stringifyre-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
- ✅
transformJSONStringscleanly 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/tddred-green-refactor artifact. - ✅ The new
throwon malformed mixed-log tokens correctly triggers the existing fail-closedprocessFilecleanup 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
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed secret-redaction correctness and malformed-record performance issues remain unresolved.
2 open findings
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.
Keep bundled public field names and enum values intact while sanitizing payload data. Protect post-redaction collector configuration, fail closed on unsafe control changes, and cover the short-mask failure through collection and real review handlers with mocked GitHub. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve unrelated escapes, bound rejected-token scanning, and resolve configured, built-in, and runtime masks without encoding or overlap bypasses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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. |
|
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. |
|
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. |
|
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. |
|
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. |
|
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. |
|
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. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
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
|
|
In reply to comment 6209357085 (Accept): Confirmed, and the current implementation has now been checked independently at 67cb55b. All 463 tests in the focused parser, matcher, redaction, schema, runtime publication, and bundle-dependency suites passed. |
|
In reply to comment 6209357458 (Accept): Confirmed: the completion announcement needs no code change, and the substantive parser finding remains fixed. Fresh path-preservation, seeded, and runtime regression tests passed at 67cb55b. |
|
In reply to comment 6209357785 (Partially accept): The schema-safe fix from 116a128 remains present, and fresh tests confirm that short runtime masks preserve executable review messages through redaction, collection, and review handlers. This verifies the local regression coverage, not the source of the historical mask or successful hosted publication. |
|
In reply to comment 6209358097 (Accept): Confirmed: the completion announcement is informational and requests no additional code change. No new parser or redaction change is warranted for this acknowledgment. |
|
In reply to comment 6209358379 (Accept): Confirmed: the earlier score is not evidence for later revisions, so the current HEAD was validated separately. All 463 focused tests passed, including path preservation, bounded scanning, configured-secret encoding, overlap handling, runtime publication, and bundle dependencies. |
|
In reply to comment 6209358706 (Accept): Confirmed: conservative decoding still replaces only matching source spans and preserves unrelated paths and escapes. The corresponding focused, seeded, fail-closed, and runtime regressions passed again at 67cb55b. |
|
In reply to comment 6215986711 (Accept): Already implemented in 67cb55b: JSON string scanning and mask matching are separate modules, existing exports are preserved, and both deployment bundles include their dependencies. Fresh direct, redaction, schema, runtime publication, and bundle-dependency checks passed all 463 tests; no further refactor was needed. |
|
In reply to comment 6216278695 (Accept): Verified against 67cb55b: the module split, compatibility exports, deployment lists, and direct plus runtime regression coverage are already present. A fresh focused run passed 463 tests across seven suites, so no additional source change is needed for the requested refactor. |
|
In reply to comment 6209942697 (Accept): This earlier request was addressed by c350350, which included safe_output_redaction.cjs in both setup bundles, and the subsequent main merge ab02a1f. The newer shard 4 failure is a different diagnostic-test regression reproduced on unmodified main, not a recurrence of the missing-bundle defect. |
|
In reply to comment 6210320291 (Accept): Confirmed that c350350 added the missing dependency to both setup bundles, and the next CI run recorded shard 4 success. The current bundle-closure tests also pass; the later agent_execution diagnostic failures are separate. |
|
In reply to comment 6215569490 (Partially accept): Run 38901241131 failed at Redact secrets in logs on c350350, before the current parser refactor; its annotations report fail-closed artifact removal with an unknown redaction cause. Main run 38856386715 has the same failure boundary, but that does not prove a shared root cause or establish that current HEAD is fixed. |
|
In reply to comment 6215577000 (Accept): This is a startup notification, not an additional code-change request. The completed Claude run failed before agent execution at Start MCP Scripts HTTP Server on c350350; its later missing-session-artifact error is downstream, and the available annotations do not identify a concrete current-HEAD defect. |
|
In reply to comment 6215578357 (Accept): Confirmed that Smoke Copilot Auto run 38901157812 completed successfully on c350350. That is useful historical smoke coverage, not validation of the later commits or a reason to change code. |
|
In reply to comment 6215579961 (Accept): This is a startup notification and requires no implementation change. The completed Pi run had successful agent, detection, and safe-output jobs but failed its final Handle agent failure step because the agent reported incomplete work; this does not establish a parser regression. |
|
In reply to comment 6215580036 (Accept): This is a startup notification and requires no implementation change. The completed Gemini run failed Execute Gemini CLI with exit code 41 on c350350; the available runtime metadata does not establish a redaction or parser defect. |
|
In reply to comment 6215582248 (Partially accept): Run 38901157772 has two distinct failures on c350350: Execute Codex CLI exited with code 1, and Redact secrets in logs then failed closed with an unknown redaction cause. Its threat-detection warning does not identify either root cause, and no evidence here confirms the same failures on current HEAD. |
|
In reply to comment 6215582595 (Partially accept): The container run's recorded failure is Redact secrets in logs, not a failed tool-installation step, and annotations show fail-closed artifact removal with an unknown cause. This c350350 result predates current HEAD and shares a failure boundary with main, so it does not justify an unrelated tool or container change. |
|
In reply to comment 6215638287 (Accept): The requested main update and shard 1 fixture correction were supplied by ab02a1f, including GITHUB_ENV and run identity for the converter fixture. The affected fixture and bundle-closure tests pass now; the later shard 4 diagnostic regression is independently reproduced on main. |
|
In reply to comment 6215639164 (Accept): Thanks for the smoke-run output; it contains no actionable review request. Its associated Copilot Auto run passed on c350350, while smoke coverage for the subsequent HEAD remains unverified. |
|
In reply to comment 6215924119 (Accept): Confirmed that ab02a1f contains the main merge and the converter fixture now supplies GITHUB_ENV and run identity. The current focused tests pass, and the remaining later shard 4 failure is a separate main-branch diagnostic regression rather than this already-corrected fixture issue. |
|
@copilot secret redaction should switch to text mode if JSON is invalid |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
Match raw and decoded credential spans across entire invalid artifacts, including multiline secrets, and preserve fail-closed cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| text => { | ||
| const custom = redactSecrets(redactBuiltInPatterns(text).content, secretValues); | ||
| return applyAddMaskRedaction(custom.content, maskedValues); | ||
| } |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Originally split the secret-parsing/redaction portion from #69235 into a standalone PR against
main, transferring the five reviewed files fromb681ac5f07cb0e8f3a6648fc3904c69a250681f7in 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.Exact scope
actions/setup/js/add_mask_redaction.cjsactions/setup/js/add_mask_redaction.test.cjsactions/setup/js/redact_secrets.cjsactions/setup/js/redact_secrets.test.cjsactions/setup/js/safe_output_redaction.cjsactions/setup/js/safe_output_redaction.test.cjsactions/setup/js/runtime_mask_publication.test.cjsdocs/src/content/docs/reference/imports.md.changeset/patch-large-record-mask-redaction.mdMarkdown 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.make fmt-cjs,make lint-cjs, and finalmake agent-report-progresspassed. 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.RangeErrorfor 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_numberfields 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
38804538442reportedredact/unknown, notEACCES; 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.