feat(project-profiling): vuln reporting protocol [CM-1331] - #4413
feat(project-profiling): vuln reporting protocol [CM-1331]#4413mbani01 wants to merge 11 commits into
Conversation
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
|
|
PR SummaryMedium Risk Overview Storage & docs: New Worker: New Ops: Read-only Reviewed by Cursor Bugbot for commit be30d28. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Adds a Temporal pipeline that parses repository security policies and assembles vulnerability-reporting protocols.
Changes:
- Adds deterministic and Bedrock-assisted policy parsing.
- Adds protocol assembly, persistence, scheduling, and validation tooling.
- Documents the architecture and updates dependencies.
Metadata: PR title should end with (CM-1331), not [CM-1331].
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
services/apps/packages_worker/src/workflows/index.ts |
Exports the workflow. |
services/apps/packages_worker/src/tmp/validate-reporting-protocol.sql |
Adds production validation queries. |
services/apps/packages_worker/src/security-contacts/workflows.ts |
Drains parse and assembly batches. |
services/apps/packages_worker/src/security-contacts/protocol/validate.ts |
Validates parsed LLM output. |
services/apps/packages_worker/src/security-contacts/protocol/types.ts |
Defines protocol contracts. |
services/apps/packages_worker/src/security-contacts/protocol/schedule.ts |
Schedules daily ingestion. |
services/apps/packages_worker/src/security-contacts/protocol/parseStage.ts |
Implements policy parsing. |
services/apps/packages_worker/src/security-contacts/protocol/llmExtract.ts |
Integrates AWS Bedrock. |
services/apps/packages_worker/src/security-contacts/protocol/fetchContent.ts |
Fetches policy content. |
services/apps/packages_worker/src/security-contacts/protocol/classify.ts |
Adds deterministic classification. |
services/apps/packages_worker/src/security-contacts/protocol/assembleStage.ts |
Selects and persists assembled protocols. |
services/apps/packages_worker/src/security-contacts/protocol/assemble.ts |
Implements protocol merge rules. |
services/apps/packages_worker/src/security-contacts/activities.ts |
Exposes Temporal activities. |
services/apps/packages_worker/src/config.ts |
Adds pipeline configuration. |
services/apps/packages_worker/src/bin/security-contacts-worker.ts |
Registers the new schedule. |
services/apps/packages_worker/src/activities.ts |
Exports protocol activities. |
services/apps/packages_worker/package.json |
Adds the Bedrock SDK. |
pnpm-lock.yaml |
Locks dependency updates. |
docs/adr/0010-security-contacts-worker.md |
Documents the architecture. |
backend/src/osspckgs/migrations/V1785283200__reporting_protocol.sql |
Creates protocol tables and index. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (3)
services/apps/packages_worker/src/security-contacts/protocol/classify.ts:92
- GitHub PVR is excluded from negation handling, so text such as “do not use GitHub private vulnerability reporting” is classified as an accepted/preferred method. This contradicts the documented rule that negation on a method's own line becomes
prohibitedand can publish the exact channel a project forbids.
for (const hit of hits) {
if (NEGATION_RE.test(hit.line) && hit.method.type !== 'github-pvr') {
hit.method.status = 'prohibited'
}
services/apps/packages_worker/src/security-contacts/protocol/assemble.ts:121
- The
pvrEnabled === falseveto is applied before inferred fallbacks are built, so a livesecurity_contactsrow with channelgithub-pvrreintroduces PVR for a repo whose authoritative flag disables it. The added validation query explicitly expects such repos to contain no GitHub PVR method.
if (!declared) {
finalMethods = input.fallbackContacts
.filter((c) => FALLBACK_CHANNEL_MAP[c.channel])
.sort((a, b) => b.score - a.score)
.slice(0, 3)
services/apps/packages_worker/src/tmp/validate-reporting-protocol.sql:39
- As with the type check, SQL
NOT INdoes not count a missingstatusbecause the predicate becomes NULL. Explicitly test for NULL so malformed methods are reported.
SELECT 'bad_status', COUNT(*)
FROM repo_reporting_protocols rp,
jsonb_array_elements(rp.methods) m
WHERE m->>'status' NOT IN ('preferred','accepted','fallback','prohibited')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return `https://github.com/${owner}/${name}/security/advisories/new` | ||
| } | ||
|
|
||
| export function assembleProtocol(input: AssembleInput): AssembledProtocol { |
There was a problem hiding this comment.
The merge policy is covered: assemble.test.ts has focused Vitest cases for exactly these contracts — PVR veto, pvr-adds-when-silent, declared-vs-inferred fallback (cap + score order), dedup, single-preferred demotion, prohibited-last ordering, provenance stamping, and degraded-contributes-nothing. The team's current workflow keeps test files out of feature commits (they run locally/in dev), which is why they aren't visible in this PR — the suite is 47 tests green as of 424a10e.
| return null | ||
| } | ||
|
|
||
| export function classifySecurityPolicy(text: string): ClassifierVerdict { |
There was a problem hiding this comment.
The classifier has focused coverage in classify.test.ts: 10 behavioral cases spanning template fingerprint, negation→prohibited, preference cuing, conditional routing→residue, URL/email/bounty/issue-tracker classification, acknowledgement-section suppression, and pointer-only linkedUrls capture. Tests are kept out of feature commits under the team's current workflow, so they don't appear in this PR diff.
| export async function runParseStage( | ||
| qx: QueryExecutor, | ||
| deps: ParseStageDeps, | ||
| cfg: Cfg, | ||
| ): Promise<ParseStageResult> { |
There was a problem hiding this comment.
parseStage.test.ts covers the batch state machine with mocked deps: deterministic path, LLM-validated path, degraded path, pointer-only success (page row before file row), pointer-only all-pages-failed (no rows, counted failed, reachable next sweep), blob fetch failure, and the LLM concurrency cap. Tests are kept out of feature commits under the team's current workflow, so they aren't visible in this PR diff — 47 green as of 424a10e.
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
|
|
||
| export function htmlToText(html: string): string { | ||
| return html | ||
| .replace(/<script[\s\S]*?<\/script\s*>/gi, ' ') |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 424a10e. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (16)
services/apps/packages_worker/src/security-contacts/protocol/classify.ts:44
- This branch-heavy deterministic classifier has no automated coverage, despite packages_worker using colocated Vitest tests for comparable pure parsers (for example
src/osv/__tests__/parseOsvRecord.test.tsandsrc/maven/__tests__/normalize.test.ts). Add fixtures covering preference, negation, conditional routing, templates, and linked URLs so protocol classifications cannot regress silently.
export function classifySecurityPolicy(text: string): ClassifierVerdict {
services/apps/packages_worker/src/security-contacts/protocol/assemble.ts:49
- The merge/veto/fallback behavior introduced here has no unit tests, although packages_worker colocates Vitest coverage for similar pure logic. Add cases for PVR true/false, conflicting methods, degraded parses, inferred fallback ordering, deduplication, and preferred normalization; these rules directly determine the persisted public data.
export function assembleProtocol(input: AssembleInput): AssembledProtocol {
services/apps/packages_worker/src/security-contacts/protocol/assembleStage.ts:46
- Staleness is based only on
sp.parsed_at, so repository-level file changes are missed when a file is deleted or when a repo starts referencing an already-cached blob whose parse predates its protocol row. The existing protocol then keeps obsolete methods (or omits the new file) until an unrelated contacts refresh. Includerepo_well_known_files.change_detected_atfor both live and deleted security rows.
OR EXISTS (
SELECT 1 FROM repo_well_known_files w
JOIN security_policy_parses sp ON sp.blob_oid = w.blob_oid
WHERE w.repo_id = r.id AND w.file_type = 'security'
AND w.deleted_at IS NULL AND sp.parsed_at > rp.assembled_at)
services/apps/packages_worker/src/security-contacts/protocol/assemble.ts:121
- The PVR veto is applied only to declared methods. When there are no declared methods, a live or stale
security_contactsrow with channelgithub-pvris mapped back intofinalMethodseven whenpvrEnabled === false, violating the stated invariant that such repos carry no GitHub PVR method. Apply the veto to fallback contacts as well.
finalMethods = input.fallbackContacts
.filter((c) => FALLBACK_CHANNEL_MAP[c.channel])
.sort((a, b) => b.score - a.score)
.slice(0, 3)
services/apps/packages_worker/src/security-contacts/protocol/validate.ts:71
- Sentinel endpoints bypass the source check entirely. An LLM response can therefore invent
github-pvrorsecurity-txtfor text that mentions neither channel and still be persisted as anokparse, contradicting the validator's no-invented-channel contract. Require source evidence for the sentinel's method type before accepting it.
if (SENTINEL_ENDPOINT_BY_TYPE[m.type] === m.endpoint) continue
services/apps/packages_worker/src/security-contacts/protocol/classify.ts:92
- GitHub PVR hits are exempted from negation, so text such as “Do not use private vulnerability reporting” is classified as one usable method, promoted to
preferred, and stored deterministically without reaching the LLM. PVR plus negation needs to be treated as prohibited or ambiguous rather than clean.
for (const hit of hits) {
if (NEGATION_RE.test(hit.line) && hit.method.type !== 'github-pvr') {
hit.method.status = 'prohibited'
}
services/apps/packages_worker/src/security-contacts/protocol/validate.ts:32
- The runtime validator accepts a missing
guidelinesproperty even though the emitted JSON contract marks it required. Because Bedrock does not enforce the prompt schema, an object without this key can be stored asok, leaving persisted JSON outsideParsedProtocol. Acceptnull, but rejectundefined.
function hasValidGuidelinesShape(parsed: ParsedProtocol): boolean {
const g = parsed.guidelines
if (g === null || g === undefined) return true
return (
services/apps/packages_worker/src/security-contacts/protocol/validate.ts:65
- This condition explicitly accepts an omitted
condition, although the LLM contract requires every method to containconditionas either a string ornull. A model response missing the field therefore passes validation and later produces protocol methods with an invalid persisted shape.
if (m.condition !== null && m.condition !== undefined && typeof m.condition !== 'string') {
reasons.push(`malformed condition on: ${m.type}`)
}
services/apps/packages_worker/src/security-contacts/protocol/assemble.ts:58
- Deduplication keeps the first matching method, but both
json_agginputs inassembleStage.tsare unordered. If two files/pages declare the same endpoint with different statuses, PostgreSQL row order decides whether it is preferred, accepted, or prohibited, so repeated assemblies can produce different protocols. Define deterministic source ordering and conflict precedence before dropping duplicates.
const push = (m: ProtocolMethod) => {
const key = `${m.type}:${m.endpoint.toLowerCase()}`
if (seen.has(key)) return
seen.add(key)
methods.push(m)
services/apps/packages_worker/src/security-contacts/protocol/parseStage.ts:41
DISTINCT ONalways chooses the lowest repo id as the sole fetch location for a shared blob. If that repository is disabled, deleted, renamed, or otherwise inaccessible while another repo still exposes the same blob, every retry uses the permanently failing URL and all repositories sharing that policy remain unparsed. Retain multiple candidate repos or retry the blob through another live association.
SELECT DISTINCT ON (w.blob_oid) w.blob_oid, r.url
FROM repo_well_known_files w
JOIN repos r ON r.id = w.repo_id AND r.host = 'github'
JOIN package_repos pr ON pr.repo_id = w.repo_id
JOIN packages p ON p.id = pr.package_id AND p.is_critical
LEFT JOIN security_policy_parses sp
ON sp.blob_oid = w.blob_oid AND sp.parser_version = $(parserVersion)
WHERE w.file_type = 'security' AND w.deleted_at IS NULL AND sp.blob_oid IS NULL
ORDER BY w.blob_oid, w.repo_id
services/apps/packages_worker/src/security-contacts/activities.ts:49
- Heartbeat failures are swallowed without checking Temporal's
cancellationSignal, unlike the existingprocessBatch.tsworker loop. After a heartbeat timeout supersedes an attempt, the old attempt can continue scheduling fetches/LLM calls and writing rows concurrently with its retry, duplicating Bedrock cost and creating races. Propagate cancellation into both stage loops and stop scheduling work when aborted.
async function withHeartbeat<T>(fn: () => Promise<T>): Promise<T> {
const heartbeatTimer = setInterval(() => {
try {
heartbeat()
} catch (err) {
log.warn({ errMsg: (err as Error).message }, 'Heartbeat failed')
}
backend/src/osspckgs/migrations/V1785283200__reporting_protocol.sql:4
- This column comment says linked-page keys hash extracted text, but
processLinkedPagesstoressha256Hex(url)and the ADR also specifies URL-keying. The migration documentation should describe the actual primary-key semantics used for joins and cache behavior.
blob_oid TEXT PRIMARY KEY, -- git blob oid for files; sha256 of extracted text for linked pages
services/apps/packages_worker/src/security-contacts/protocol/parseStage.ts:32
- This new service module embeds selection, insert/upsert, and cache lookup SQL directly in the worker. The repository convention is that database query functions live in
services/libs/data-access-layer(CLAUDE.md:32-34), so keeping these here creates another data-access path that cannot be reused or tested with the DAL. Move the query functions behind a DAL module.
async function selectUnparsedBlobs(qx: QueryExecutor, limit: number): Promise<BlobJob[]> {
return qx.select(
`SELECT * FROM (
services/apps/packages_worker/src/security-contacts/protocol/assembleStage.ts:34
- The new assembly selection/upsert data access is implemented inside the worker, while
CLAUDE.md:32-34establishesservices/libs/data-access-layeras the home for all database query functions. Move this SQL and persistence operation into a DAL module so schema access stays centralized and reusable.
async function selectReposToAssemble(qx: QueryExecutor, limit: number): Promise<RepoRow[]> {
return qx.select(
`WITH stale AS (
services/apps/packages_worker/src/security-contacts/protocol/fetchContent.ts:43
- This HTML conversion discards anchor
hrefvalues and replaces numeric entities such as@with spaces. A linked policy like<a href="https://hackerone.com/acme">report here</a>orsecurity@example.comtherefore loses the actual endpoint before classification, and the LLM validator cannot recover it because endpoints must occur in the extracted source. Preserve link targets and decode entities with an HTML parser/entity decoder.
.replace(/<[^>]+>/g, ' ')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/&#?(?!amp;)\w+;/g, ' ')
services/apps/packages_worker/src/security-contacts/protocol/validate.ts:44
- The deterministic validator is the only gate between untrusted LLM JSON and persisted
okrows, but it has no unit tests. Add Vitest cases for missing/extra fields, invalid enums and guideline shapes, endpoint grounding (including obfuscated email), sentinel grounding, and preferred-count limits; comparable packages_worker parsers are covered under colocated__tests__directories.
export function validateParsedProtocol(
parsed: ParsedProtocol,
sourceText: string,
): { ok: boolean; reasons: string[] } {
| const host = url.hostname | ||
| if (BLOCKED_HOST_RE.test(host)) return true | ||
| if (host.includes(':') || host.startsWith('[')) return true | ||
| if (IPV4_RE.test(host)) return isPrivateIpv4(host) | ||
| return false |
Review Feedback Addressed (round 1)Commit: 424a10e Changes Made
No Change Needed
Threads Resolved15 of 18 addressed and resolved in this iteration. |
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>

This pull request introduces a new system for parsing and assembling per-repository vulnerability reporting protocols, alongside dependency and configuration updates to support this feature. The main changes include new database tables, configuration and activity wiring, and dependency additions for AWS Bedrock LLM integration.
Reporting Protocol Feature:
security_policy_parses(content-keyed parse cache for security policy files and linked pages) andrepo_reporting_protocols(assembled per-repo reporting protocol), as described in ADR-0010 addendum.Worker Implementation:
getReportingProtocolConfig. [1] [2]runProtocolParseBatchandrunProtocolAssembleBatch, with fixed-cadence heartbeats to avoid timeouts during long LLM calls. [1] [2] [3]Dependency and Package Updates:
@aws-sdk/client-bedrock-runtimedependency for direct AWS Bedrock LLM calls. [1] [2]http-errors,qs, andstatuses. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]These changes lay the groundwork for robust, scalable ingestion and assembly of vulnerability reporting protocols across repositories, leveraging both deterministic and LLM-based parsing.