Skip to content

fix(pe3): keep credential actions visible in skill-card fields - #895

Open
yashrajp22 wants to merge 79 commits into
mainfrom
yashraj/fix-credential-label-actions
Open

yashrajp22 wants to merge 79 commits into
mainfrom
yashraj/fix-credential-label-actions

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #827. A skill-card field such as Credential Type(s): [Read Perforce access credentials] could hide a real credential-access finding. Keep the original fix for harmless credential disclosures, while retaining findings for credential reads, acquisition, storage and transfer instructions inside the field.

The new guard stays within the existing 4,096-character field limit and leaves the shared OAuth checks unchanged. Exact adjective compounds such as read-only and open-source remain harmless before or after credential, secret and token nouns; actual actions and sensitive paths still remain visible.

Validation:

  • 991 targeted and adjacent regression tests pass from source and again from a fresh installed wheel.
  • 32 CLI scans across eight focused skill cases reproduce the baseline problems and verify the fix in source and wheel modes.
  • 48 scans across 12 complete sample skills preserve source/wheel results and unrelated findings.
  • Fresh wheel/sdist builds, package-resource checks, Ruff and both independent secondary reviews pass.

All scan checks used offline static analysis; no live-provider behavior is changed or claimed.

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Value and readiness

Ready from code review. The bounded label check now vetoes credential acquisition, storage and transfer actions across the whole field and preserves harmless credential-type disclosures.

Material findings

No required changes found.

PIC tradeoffs

The guard intentionally treats action words conservatively throughout a field; this may retain some descriptive findings. Exact adjective compounds are masked with equal-length spaces so boundaries and sensitive paths remain visible.

Verification and gaps

Reviewed the complete diff, description, linked issue material, all previous reviews/comments/replies and pinned-head surrounding source at 30a4fd26d1dd0cc719d66a1cf564d0211aee4bc8. Inspected the field fullmatch/4096-character bound, noun-only eligibility, shared action/source guards and the expanded positive, negative, boundary and graph-level tests. No GitHub status checks are reported for this head; author-reported test runs are not independently reproduced. No contributor code, tests, imports, build steps or scanner invocations were executed during this review.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Value and readiness

Approval holds at the synchronized head. Both PR-owned production and regression files are unchanged.

Material findings

No new required changes found. All head movement is merged main work and automatic synchronization; the bounded field/action checks and positive/negative regression matrix are byte-unchanged. No authored changes were introduced after the prior approval.

PIC tradeoffs

The existing conservative field-wide action veto and equal-length adjective masking remain the reviewed precision tradeoff.

Verification and gaps

Reviewed exact head 5d18076f2830568cad73cabfd111e07787026bc3 against previously assessed 30a4fd26d1dd0cc719d66a1cf564d0211aee4bc8. Combined the earlier complete assessment with the full commit/file comparison and current pinned source. The main updates reviewed in this run add no changes to the two PR-owned files. No hosted checks are attached to this head; fresh CI remains a merge prerequisite. No contributor code, tests, builds, imports or scanner invocations were executed.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Decision: Approved at 7ef93e8f7662b91c56f63b285555163f581d364e.

Value and readiness

The bounded field-wide action veto repairs the credential-access gap introduced by #827 while preserving harmless credential disclosures. The complete current production and regression files are byte-identical to persisted reviewed head bf09e09cd0168ad5d65f9518608d003db45bb2ac. Ready from code review; exact-head CI remains a merge gate.

Material findings

No required change found. Eligibility still requires a determiner-free access-credential/secret/token noun inside a complete Markdown credential field no longer than 4096 characters. Action and sensitive-source guards inspect the whole field. Equal-length masking applies only to exact read-only/read-write/open-source/open-access compounds, preserves distances, and does not erase a recognized sensitive path. Tests retain findings for prefix/suffix actions, longer-distance actions, inflections, multiple occurrences, malformed compound lookalikes and sensitive sources, with boundary, runner and graph assertions. The shared OAuth guards are unchanged. Both prior marked approvals were checked; neither left an unresolved required code finding.

PIC tradeoffs

A conservative action word anywhere in a field can retain a descriptive finding; that is the existing reviewed precision tradeoff. No new product decision identified.

Verification and gaps

Read the complete two-file diff, exact-head field/action/source filters and finding call path, all regression assertions, #827's original disclosure rationale, both own marked reviews, and empty discussion/comment/thread records. Compared exact file contents with the persisted reviewed head rather than trusting the review's remapped commit association. Fresh metadata still matches the inventory head, open and non-draft. No hosted checks attach to this head; author source/wheel/offline scan claims were not independently reproduced. No contributor code, tests, imports, builds, setup, hooks, scanner invocations or regex probes were executed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants