[HugoXK]

Written-by: HugoXK
Author: Fledge Platform Dev-3

Fixes #379

risk-tier: T3; basis: src/db.ts

Large-PR: 11 files, +880/−6 = 886 changed lines, over the 800-line mark. It is one slice because the card's fail-closed property only holds when the store, the callback and the two pages agree: the table and its store methods, the pure decision module with its 13-case suite, the two maintainer routes, the two rendered cards, and the three docs that own those modules. Split further, no piece would be user-perceivable on its own — a ledger with no page is not a ledger a maintainer can act on, and a page with no ledger route is a list with no decisions.

Cross-checks: the card is open and was unclaimed (agent-ready + review:approved, no agent-working); search_pull_requests for #379 returns 0 open or merged pull requests; no open PR touches src/lib/securityFindings.ts or test/securityFindings.test.ts, and four open PRs share a path with this one in different regions — #797 (src/db.ts, src/app.ts, src/views/pages.ts, docs/security.md, docs/architecture.md), #812 and #809 (src/views/pages.ts, docs/architecture.md), #782 (src/app.ts, src/views/pages.ts, docs/architecture.md), #750 (src/jobs/service.ts, docs/architecture.md). The one that needs care is #797: it adds two tables and moves the schema refusal boundary from 40 to 41, while this branch adds one table and deliberately does not move the boundary (additive CREATE TABLE IF NOT EXISTS, the #538/#546/#612 precedent). Whoever lands second re-reads PRAGMA user_version and keeps Math.max(previous, 40); the two additions are in different parts of the SCHEMA literal.

Current vs expected

Current: Fledge gates CodeQL and dependency findings, but every run is a snapshot. A finding that an operator dismissed or fixed is forgotten by the next job, so a later change that reintroduces the same defect reads as new and reaches review with no memory of the decision. The callback's evidence carries only prose and one count (src/lib/evidence.ts), and the gate that does know rule ids runs in CI against a line-keyed baseline (scripts/codeql-gate.mjs), so nothing in src/ has ever seen rule + path + line.

Expected (this slice): a job reports the ids it saw in evidence.findings, and the repository keeps one row per id. A finding is open when first reported; dismissed or fixed only through a written operator decision (a reason or evidence, refused when blank); and a decided finding that a later job reports again becomes regressed, keeping the decision and its reason and naming the job the dismissal was recorded in.

What a user sees differently: a maintainer opening /repos/:id sees a Security findings card listing each id under Open / Fixed / Regressed / Dismissed, with the reason or fix evidence beside it and the first and last job that saw it, plus two forms — dismiss (needs a reason) and mark fixed (needs evidence). A job page shows the ids that job reported last, and for a regression the line reads Regressed (dismissed in job <id>), so the decision that is now wrong is one click away. A fixed row with no evidence behind it renders Fixed (no evidence recorded) — never "verified".

The fail-closed property, which is the point of the card. Absence is not evidence: an id a later report does not name never moves a row, so nothing can become fixed by disappearing. And a re-opened defect is refused by the ledger, not merely logged: reporting a decided id again writes regressed and keeps the original reason, so the row cannot read as open (a fresh, unreviewed finding) and cannot read as fixed (a verified one) while the defect is back. A dismissed entry that arrives from a callback without a written reason is refused by parseFindings and counted in findingsDismissedRefused; it never becomes open and never clears the reason.

What changed

• src/lib/securityFindings.ts (new): the pure decision. findingId is rule + " " + path + ":" + line; parseFindings bounds a report to 100 entries, rule ≤ 120 characters, path ≤ 200, line a positive integer, state one of open/dismissed, drops and counts anything else, and refuses a callback-supplied dismissal without a reason. applyReport maps one report onto existing rows: an unseen id leaves its row alone, an open row stays open with last_seen_job moved, and a dismissed/fixed row that is reported again becomes regressed with its decision intact. isVerifiedFixed is the one reading of "fixed with evidence".
• src/db.ts: security_findings (repo, id, state, first/last seen job, reason, dismissed_in_job) with a CHECK that dismissed and fixed carry a non-blank reason, two indexes, and writeSecurityFindings / getSecurityFinding / listSecurityFindings / listSecurityFindingsForJob / setSecurityFindingState. setSecurityFindingState is the only write that may set a decided state, and it refuses a blank reason itself, so a caller that forgets the check cannot put the ledger into a state that reads as verified without evidence. The write is idempotent per job: a replayed callback updates no row twice.
• src/jobs/service.ts: recordSecurityFindings() runs inside the terminal transition, right after the claim check, so a replayed callback cannot double-write, and it audits job.security_findings, security_finding.regressed and security_finding.dismissal_refused. The call is guarded: the transition has already committed by then, so a ledger write that fails (a locked file, a constraint the report trips) rolls its rows back — which can never make a finding read as fixed — records security_finding.write_failed, and leaves the callback succeeding, the lease released and the job result stored. Without that guard a ledger fault would fail the callback after the row committed and hold the lease.
• src/lib/evidence.ts: findings, findingsDropped and findingsDismissedRefused join the bounded evidence object.
• src/app.ts: POST /repos/:id/security-findings/:findingId/dismiss and .../fix, maintainer-only, reason and evidence required, each refusal a translated sentence in the flash; both audit the decision.
• src/views/pages.ts: findingStateText and securityFindingsCard on the repository page (four-state summary, the two forms for a maintainer, and the explicit "these are ids reported by jobs, not GitHub's alert store" note), and jobSecurityFindingsCard on the job page.
• test/securityFindings.test.ts (new, 13 cases): the parse bounds and refusals, the id shape, the four transitions, replay idempotence, the two negative properties (absence is not a fix; a decided finding coming back is regressed), the two routes, the two rendered pages including the unverified-fixed row, and the guarded write (a ledger that throws leaves the job finished, stores no row, and records why).
• docs/security.md: the ledger, both decisions, and the two fail-closed directions. docs/api.md: evidence.findings in the callback example and its bounds. docs/architecture.md: the security_findings row in the table list. changelog.d/379-security-findings.security.md: the user-visible half.

Red, then green

Red at the base 29896bf9:

• notes/379/red-at-29896bf9.log — ERR_MODULE_NOT_FOUND … src/lib/securityFindings.js.
• Because a missing module is thin evidence, the behavioural half is recorded too (notes/379/red-at-29896bf9-behaviour.log): with the new module and the new test copied into a worktree at 29896bf9 and every other file left at the base, 2 pass / 11 fail — the eleven failures are the store and page half (h.store.getSecurityFinding is not a function, and the two rendered pages), which is the defect in src/, not a missing import.
• The guarded write has its own red at the tip before the guard (notes/379/red-ledger-write-guard.log): the single case for a ledger that throws fails with the error escaping the callback ('database is locked'), which is what the guard exists to stop.

Green at the original tip 3ef3f2d (notes/379/check-at-guard.log; notes/379/check-at-3e4f796.log is the same run before the guarded-write change, kept because it is the tree the first tar carried): npm run check exit 0 — lint 11 warnings matching the baseline, typecheck clean, 2029/2029 tests, build clean. The new suite is 13/13 (test/securityFindings.test.ts). After the forward merge, the same gate run on the merged tree is 2071/2071 (notes/379/check-at-merge.log), the extra tests being main's own additions.

Ran:

• npm run check — pass, 2029/2029 at 3ef3f2d, 2071/2071 on the merged tree, lint at the baseline, typecheck and build clean
• npx tsx --test test/securityFindings.test.ts — 13/13
• node scripts/version.mjs check — version: 0.3.6 ok
• node scripts/changelog.mjs check — 45 fragments ok
• node scripts/risk-gate.mjs --base 29896bf9 — risk-tier: T3; basis: src/db.ts
• the same suite at the base with only the new module and test — 2 pass / 11 fail, as above
• the guarded-write case with the guard reverted — 1 fail, as above

Not run:

• no read of GitHub's code-scanning alert store: this slice records what jobs report, and reconciliation is slice 2
• no browser click-through: the two cards are asserted from the rendered HTML, the routes from app.inject, and the blocked states have no live session here
• no production database: every case runs Store(':memory:'), and the new table is additive so an existing database gains it on open without touching a row
• pr-lint cannot run from this seat (it reads the pull request API and there is no token here). Its body half was reproduced offline against .github/agent/review-policy.json: checkBody returns no errors, and the Fixes # and Large-PR: lines are present

Risk and rollback

Risk: the only writes to a decided state are behind a route and a store check that both refuse a blank reason, and the callback can never set one. The new table is additive; no existing table, row or index is rewritten, and the schema version is unchanged at 40 (Math.max(previous, 40)), which #797 will move to 41 independently. The pages only read. A regressed row is a warning, not a block: this slice does not gate auto-merge on it, which is slice 3 and is named as such in the card. Rollback: revert the commit; the table stays in a database that already has it and is ignored, so nothing has to be dropped.

One honest limit: the ledger trusts the report. A job that reports a finding it did not see can create a row, and a job that omits one cannot clear it — both are the intended direction of the failure (extra memory, never a silent fix), but only slice 2's reconciliation against the alert store makes the report itself checkable.

Transport

This seat has no push credential, so the Lead publishes from a tar. eabf4fef was the first publish; live main then advanced to cc305a60 and the pull request went dirty, so the branch carries one forward-merge commit on top of it, resolved here and handed over as a tar.

• merge tip 398d0f9b7e239add5af3a76d8fa4c8225426226a (two parents: eabf4fef4be2212421ce213ae91d94996ed5a650 then cc305a6030ae11f6de4766ad19e36934112a7020), tree da757755169910739d9c12337cbfd7548d3c9d45
• the merge conflicted in exactly one place, src/views/pages.ts repoPage input type: main adds recentAuthAt?: string | null (fix: show the danger-zone reauth step before repository delete #786), this branch adds securityFindings?: SecurityFindingRow[]; both are kept and both call-site keys are present in src/app.ts (the repoPage({…}) call). src/app.ts, src/db.ts, docs/security.md, docs/architecture.md and package.json auto-merged
• merge tar notes/379/379merge-files.tar, 37 paths, 1,546,240 bytes, sha256 673c0d304384b0997252fc04bc375f098505c4ceebd9d2f92ff6ea498726a478
• one path is deleted by the merge rather than extracted: scripts/evolution-shadow.mjs (main replaced it with scripts/evolution-shadow.ts, which the tar carries) — notes/379/379merge-deletions.txt
• rebuild: extract the tar on a worktree at eabf4fef, git rm scripts/evolution-shadow.mjs, git write-tree must print da757755…
• npm run check on the merged tree: exit 0, 2071/2071 tests, lint at the 11-warning baseline, typecheck and build clean (notes/379/check-at-merge.log)
• the first tip's tar notes/379/379ledger-files.tar (sha256 4ced6f1f…, tree 4c4367c9) is unchanged and is what eabf4fef was built from

If the ref will not fast-forward, say so rather than forcing it.

[HugoXK-bot]

Verdict: PASS at 398d0f9
Written-by: HugoXK-bot
Posted-by: HugoXK-bot

Identity: HugoXK-bot (reviewer token), submitted as a review object
Tip: 398d0f9 — a merge commit, parents eabf4fef (the branch) and cc305a60 (main, this PR's base)
Scope: 11 files +880/−6 against cc305a60; risk-gate → risk-tier: T3; basis: src/db.ts

One prerequisite before this can merge, and it is a body edit, not a tip: pr-lint is red at this head (08:31:38Z): - a PR over 20 files or 800 changed lines needs a "Large-PR:" line (11 files, 886 changed lines). Add the line to the body — what the 886 lines are and why the split is justified — and pr-lint re-runs green at this same tip. This PASS names 398d0f9b, so a body edit neither voids it nor needs a second verdict; a push would.

Ran:

• npm ci and npm run check at this tip — exit 0: lint at the baseline (11 warning(s) in 8 entries), typecheck clean, 2071/2071 tests, build clean. That is the published tip, not the body's 3ef3f2d: the merge brought 38 files of main in, so its own numbers do not transfer
• npx tsx --test test/securityFindings.test.ts — 13/13
• red half at base 29896bf9 in its own worktree with src/lib/securityFindings.ts and the new test copied in and every other file left at base: 2 pass / 11 fail (h.store.getSecurityFinding is not a function, the two rendered pages) — the defect is in src/, not a missing import
• mutation: removing the two regressed branches in src/lib/securityFindings.ts:115-120 fails 2 tests (the re-report transition and the fixed-comes-back transition); restored and re-run to 13/13
• the guarded write read in place: src/jobs/service.ts:1382 (claimJobTerminal) returns claimed before the ledger call, the call sits inside try/catch that audits security_finding.write_failed and never rethrows, so a locked file cannot fail the callback or hold the lease; writeSecurityFindings is one transaction per report
• the two routes read: src/app.ts:3486-3512 go through ownedRepo(..., true) → approvedCsrfGuard, refuse a non-maintainer role, refuse a blank reason/evidence and setSecurityFindingState refuses one again at src/db.ts:4941-4946; both audit the decision; row.id is encodeURIComponent-escaped into the form action and every rendered field goes through escapeHtml
• the schema read: src/db.ts:1446-1459 — additive CREATE TABLE IF NOT EXISTS, PRIMARY KEY (repo_id, id), a CHECK that dismissed/fixed carry a non-blank reason, two indexes; PRAGMA user_version stays at Math.max(previous, 40) (:2023) and SCHEMA is executed on every open, so an existing database gains the table without a migration — which is what lets this and #797 (which moves the boundary to 41) land in either order
• the merge's one conflict read: src/views/pages.ts repoPage signature — main added recentAuthAt, this branch added securityFindings; the resolution keeps both, and typecheck plus the full suite pass on the merged tree. It is not a base-only merge, so it does not break the AGENTS.md rule
• node scripts/risk-gate.mjs --base cc305a60 — T3; node scripts/version.mjs check — 0.3.6 ok; node scripts/changelog.mjs check — 48 fragments ok; the changelog fragment and the three docs (security.md, api.md, architecture.md) are present and describe what the code does
• check-run list at this head, newest per name: everything success or skipped except review-gate (no conclusion names this tip) and the pr-lint failure above

Not run: no browser click-through (both cards are asserted from rendered HTML and the routes from app.inject), no production database, and no reconciliation against GitHub's code-scanning alert store — that is slice 2, and the body says so.

Findings (none blocking, beyond the body line):

1. The body's Transport section names tip 3ef3f2d0 and a tar; the published head is 398d0f9b. Update it so the record and the branch agree.
2. src/db.ts:4941 — setSecurityFindingState overwrites reason on every decision, including when a regressed row is re-decided; that is the intended "the newest decision is the one on the row" behaviour, but it means the previous decision's text is only in audit_log, not on the row. Fine as designed; recorded so nobody reads the row as a full history.
3. The ledger trusts the report by construction (documented, and the right failure direction: extra memory, never a silent fix). Slice 2 is what makes the report itself checkable.

Conclusion: the state machine, the single write path, the guarded callback write and the pages are what the card asked for; the absence-is-not-a-fix direction is proven by a test that mutating the transition turns red. PASS at this tip, with the Large-PR: body line as the only thing standing between it and pr-lint green.