Skip to content

refactor(capture): reuse credential header classification - #140855

Merged
steipete merged 1 commit into
mainfrom
steipete/dedup-infra-header-policy-0906
Sep 7, 2026
Merged

refactor(capture): reuse credential header classification#140855
steipete merged 1 commit into
mainfrom
steipete/dedup-infra-header-policy-0906

Conversation

@steipete

@steipete steipete commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Debug proxy capture independently repeats the credential-header names and fragments used by the secrets audit. Keeping both lists makes future credential-name updates prone to drift.

Why This Change Was Made

Capture delegates credential-name classification to the secrets-owned isLikelySensitiveModelProviderHeaderName and retains its cookie, set-cookie, and session-name extension. Caller-declared sensitive names, array flattening, output keys, and registered-secret value masking stay unchanged.

The matched header-name set is identical: the secrets owner's extra exact secret-key names were already matched by capture's secret fragment.

User Impact

None intended. Request and response captures retain the same redacted output. No configuration, dependency, schema, persistence, or public-export change.

Evidence

  • node scripts/run-vitest.mjs src/proxy-capture: 99 tests passed across nine files, covering header forms and runtime request/response persistence and streams.
  • The supplied src/secrets/model-provider-header-policy filter has no standalone test file; the wrapper rejected that target before running tests. The existing capture suites exercise the shared classifier through the changed boundary.
  • Independent review with unchanged classifier and normalization dependencies supplied: P0-P2 scoped-clean, zero findings.
  • node scripts/check-changed.mjs --base 5e550555920fe6ccc0c0e8924b6d11435ed971fc: passed.
  • Production +7/-30, net -23 lines; no test changes. Exact-head CI and native landing gates remain required.

Required fresh-main rebase onto 038acb2695d298ac671c3b886647025abdbfd22c preserved the patch exactly. The unchanged policy and normalization dependencies also have no diff. Current head b48208bd72f5c7012a793f65f04f6e15ef44a52d passed CI 34087090712.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added size: XS maintainer Maintainer-authored PR labels Sep 7, 2026
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 1:38 AM ET / 05:38 UTC.

ClawSweeper review

What this changes

Debug proxy capture reuses the shared credential-header classifier while retaining cookie and session redaction.

Merge readiness

✅ Ready for maintainer review

The refactor preserves header-redaction behavior and removes duplication still present on current main. No actionable correctness or security defect was found; this maintainer-authored PR remains a useful landing candidate.

Priority: P3
Reviewed head: b48208bd72f5c7012a793f65f04f6e15ef44a52d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving simplification with relevant existing coverage and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored refactor is exempt from the external-contributor proof gate; it preserves the sanitizer used by both capture writers, with reported capture-suite results as supplemental validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored refactor is exempt from the external-contributor proof gate; it preserves the sanitizer used by both capture writers, with reported capture-suite results as supplemental validation.
Evidence reviewed 7 items Introduced patch: The pinned base-to-head diff only replaces duplicated credential-name classification; additional sensitive names, header enumeration, flattening, output keys, and secret-value masking remain unchanged.
Classifier equivalence: The shared classifier contains the same credential fragments and exact names. Its additional secret-key names already matched capture's secret fragment; capture explicitly retains cookie, set-cookie, and session matching.
Normalization contract: The imported workspace normalization helper trims and lowercases strings, returning an empty string for blank input. Applying it to capture's already-normalized names preserves classification. This dependency is owned in the target repository.
Findings None None.
Security None None.

How this fits together

Debug proxy capture records HTTP requests and responses from the runtime and standalone proxy. Both writers sanitize headers before storing diagnostic events.

flowchart LR
  A[Runtime HTTP capture] --> C[Header redaction]
  B[Standalone proxy capture] --> C
  D[Shared credential classification] --> C
  E[Cookie and session rules] --> C
  C --> F[Registered secret masking]
  F --> G[Stored diagnostic events]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -23. Total -23 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 7 30 -23
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 7 30 -23

Review metrics

None.

Technical review

Best possible solution:

Keep credential-name classification in its existing shared owner and preserve capture-specific cookie, session, and value-redaction protections.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a behavior-preserving refactor, and source comparison establishes equivalent classification.

Is this the best way to solve the issue?

Yes. Reusing the existing leaf classifier removes duplication without changing capture's public contract or storage behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b7848ca6b197.

Labels

Label justifications:

  • P3: This is bounded internal cleanup with equivalent redaction behavior and no new user-facing capability.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored refactor is exempt from the external-contributor proof gate; it preserves the sanitizer used by both capture writers, with reported capture-suite results as supplemental validation.

Evidence

What I checked:

  • Introduced patch: The pinned base-to-head diff only replaces duplicated credential-name classification; additional sensitive names, header enumeration, flattening, output keys, and secret-value masking remain unchanged. (src/proxy-capture/header-redaction.ts:16, b48208bd72f5)
  • Classifier equivalence: The shared classifier contains the same credential fragments and exact names. Its additional secret-key names already matched capture's secret fragment; capture explicitly retains cookie, set-cookie, and session matching. (src/secrets/model-provider-header-policy.ts:34, b48208bd72f5)
  • Normalization contract: The imported workspace normalization helper trims and lowercases strings, returning an empty string for blank input. Applying it to capture's already-normalized names preserves classification. This dependency is owned in the target repository. (packages/normalization-core/src/string-coerce.ts:66, b48208bd72f5)
  • Production boundary and existing coverage: Runtime request/response capture and the standalone proxy invoke the same sanitizer before recording headers. Existing tests cover case-insensitive credentials, cookies, session fragments, caller-declared sensitive names, registered secret values, array headers, and global/Undici Headers. Tests were inspected, not executed. (src/proxy-capture/header-redaction.test.ts:12, b48208bd72f5)
  • Still necessary on main: Fetched main retains the separate capture credential lists. Inspection of v2026.9.2 likewise found the duplicated implementation; neither inspected revision contains this consolidation. (src/proxy-capture/header-redaction.ts:15, b7848ca6b197)
  • Area history and routing limits: Main-path history lists Peter Steinberger's recent header-retention work and Ayaan Zaidi's capture-redaction work. Missing historical objects prevented exact source-line parent verification, so these are routing candidates without introduction attribution. (src/proxy-capture/header-redaction.ts, b7848ca6b197)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Ayaan Zaidi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-07T05:24:05.682Z sha 12ab1aa163b1fb769d2a298af86057020a33e0ac :: needs maintainer review before merge. :: none

@steipete
refactor(capture): reuse credential header classification
b48208b 
Keep capture's cookie/session extension, caller-sensitive names, and exact-value masking while delegating credential names to the secrets policy.

Proof: node scripts/run-vitest.mjs src/proxy-capture passed 99 tests. Independent review with unchanged owner context found no actionable P0-P2 findings.
@steipete
steipete force-pushed the steipete/dedup-infra-header-policy-0906 branch from 12ab1aa to b48208b Compare September 7, 2026 05:28
@steipete
steipete marked this pull request as ready for review September 7, 2026 05:30
@steipete
steipete merged commit 6ca2ef5 into main Sep 7, 2026
249 checks passed
@steipete
steipete deleted the steipete/dedup-infra-header-policy-0906 branch September 7, 2026 05:42
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 8, 2026
@steipete
refactor(capture): reuse credential header classification (openclaw#1…
0c28841 
…40855)

Keep capture's cookie/session extension, caller-sensitive names, and exact-value masking while delegating credential names to the secrets policy.

Proof: node scripts/run-vitest.mjs src/proxy-capture passed 99 tests. Independent review with unchanged owner context found no actionable P0-P2 findings.
@jason-allen-oneal jason-allen-oneal mentioned this pull request Sep 13, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete