Repository navigation
fix(pe3): keep credential actions visible in skill-card fields - #895
yashrajp22 wants to merge 79 commits into
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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.
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-onlyandopen-sourceremain harmless before or after credential, secret and token nouns; actual actions and sensitive paths still remain visible.Validation:
All scan checks used offline static analysis; no live-provider behavior is changed or claimed.