feat: 006 stale-scan denial 및 bounded retry 구현 - #279
Conversation
📝 WalkthroughWalkthroughThis change implements T040 SAST freshness and comparability validation, durable retry decisions, Prisma persistence, runtime admission checks, preflight-bound attestations, module wiring, and contract, behavioral, and design validation. ChangesSAST freshness and retry
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ScannerRuntime
participant FreshnessService
participant TargetAuthority
participant FreshnessStore
participant RuntimeStore
ScannerRuntime->>FreshnessService: authorize retry request
FreshnessService->>FreshnessStore: load retry context
FreshnessService->>TargetAuthority: observe target scope
TargetAuthority-->>FreshnessService: verified target observation
FreshnessService->>FreshnessStore: persist retry decision
FreshnessStore-->>FreshnessService: durable decision
FreshnessService-->>ScannerRuntime: authorized or rejected
ScannerRuntime->>RuntimeStore: begin attempt two
RuntimeStore-->>ScannerRuntime: persisted attempt with retry linkage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45983129e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
apps/api/src/scan-plane/sast-scan-freshness.service.ts (2)
327-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInject the time source into
targetStillCurrent.
evaluateaccepts aclockparameter, buttargetStillCurrentcallsDate.now()directly at Line 347. This makes the skew boundary untestable and inconsistent with the rest of the service. Pass a clock or a reference time throughverifyinto this method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/sast-scan-freshness.service.ts` around lines 327 - 347, Inject the existing clock or reference time used by evaluate into the verify flow and pass it to targetStillCurrent. Replace the direct Date.now() call in targetStillCurrent with that injected time while preserving the existing five-second future-skew boundary.
258-264: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGate
readTargetObservationon explicitVERIFIEDstatus.
SastLatestTargetObservationResultis currentlyVERIFIED | UNAVAILABLE, butreadTargetObservationreturnsVERIFIEDfor anything exceptUNAVAILABLEwhiletargetStillCurrentrejects everything exceptVERIFIED. Add aresult.status !== 'VERIFIED'guard at the same point to prevent future statuses from leakingauthority: 'VERIFIED'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/sast-scan-freshness.service.ts` around lines 258 - 264, Update readTargetObservation to explicitly return the unavailable result whenever result.status is not VERIFIED, rather than only when it equals UNAVAILABLE. Preserve the existing VERIFIED path, and ensure future statuses cannot produce authority: 'VERIFIED'.apps/api/src/scan-plane/scan-plane.module.ts (1)
172-181: 🩺 Stability & Availability | 🔵 TrivialTrack the replacement of the unavailable authorities.
SastLatestTargetAuthoritymaps toUnavailableSastLatestTargetAuthority, which always returns{ status: 'UNAVAILABLE' }.SastRetryRuntimeAuthoritymaps toUnavailableSastRetryRuntimeAuthority, which always reportsscannerSetAvailable: falseandkillSwitchStatus: 'UNAVAILABLE'.With this wiring, every freshness evaluation resolves to
UNAVAILABLEauthority and every retry decision resolves toretryAllowed: false. That matches the fail-closed T040 rollout. Add an alert or a startup log so an operator can see that the placeholder authorities are active, and so the swap to real implementations is not missed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/scan-plane.module.ts` around lines 172 - 181, Add a startup log or alert in the module initialization path that clearly reports when UnavailableSastLatestTargetAuthority and UnavailableSastRetryRuntimeAuthority are wired for SastLatestTargetAuthority and SastRetryRuntimeAuthority. Keep the existing fail-closed provider mappings unchanged and ensure the signal is emitted once during application startup.apps/api/src/scan-plane/sandbox-runtime-attestation.service.ts (1)
405-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated preflight predicate into
packages/shared.The validation at Lines 414-427 duplicates the preflight portion of
isSastScannerWrapperExecutionRequestValidinpackages/shared/src/types/sast-wrapper.ts: the 8192-byteattestationRefbound, theinventoryDigestequality againstplan.repositoryState.inventoryDigest, theACCEPT/RESTRICTED_ESCALATIONplusRESTRICTEDisolation rule, and the attempt-one / attempt-twoattestationRefrule.Attestation issuance and request validation must agree on these rules. Two copies can diverge. Export one predicate from
packages/sharedand call it from both places.As per coding guidelines: "Place shared API contracts in
packages/shared".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/sandbox-runtime-attestation.service.ts` around lines 405 - 431, Extract the shared preflight validation predicate from effectivePreflight and isSastScannerWrapperExecutionRequestValid into packages/shared, exporting it from the shared SAST wrapper types. Preserve the 8192-byte attestationRef bound, inventoryDigest match, decision/isolation rules, pathPolicyVersion bound, and attempt-based attestationRef rule, then replace both local implementations with calls to the shared predicate.Source: Coding guidelines
apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts (3)
241-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the predecessor attempt explicitly.
sastScanAttemptsorders byattemptNumberdescending and takes one row. The code then treats that row as the predecessor ofrequest.attemptNumber. The query does not state that relationship.If an attempt row with
attemptNumber >= request.attemptNumberalready exists,previousresolves to that row instead of the true predecessor. The flow still fails closed, becauseevaluateSastScanRetrythen reportsSANDBOX_IDENTITY_REUSEDandRETRY_ATTEMPT_LIMIT_EXCEEDED. An explicit filter removes the ambiguity and makes the intent readable.♻️ Proposed refactor
sastScanAttempts: { - orderBy: { attemptNumber: 'desc' }, + where: { attemptNumber: request.attemptNumber - 1 }, take: 1,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts` around lines 241 - 266, Update the sastScanAttempts query used before assigning previous to filter attempts by attemptNumber less than request.attemptNumber, while retaining the descending order and take: 1 behavior. This ensures previous is explicitly the immediate predecessor rather than any existing attempt at or after the requested attempt.
673-693: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd backoff between serializable retries.
runSerializableretries a serialization failure immediately. UnderSerializableisolation, the competing transaction is often still running, so an immediate retry tends to conflict again and consumes the retry budget without progress.Add a short jittered delay before each retry.
♻️ Proposed refactor
} catch (error) { lastError = error; if (!isRetryableTransactionError(error) || attempt === SERIALIZABLE_ATTEMPTS) { throw error; } + await new Promise((resolve) => + setTimeout(resolve, attempt * 25 + Math.floor(Math.random() * 25)) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts` around lines 673 - 693, Add a short jittered delay in runSerializable before each retryable transaction attempt, after confirming the error is retryable and another attempt remains. Keep the final-attempt throw behavior unchanged, and use the existing retry constants or established delay utility if available.
1103-1113: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShare one canonical serializer instead of duplicating
stableJson.This
stableJsonduplicates the privatestableJsoninpackages/shared/src/types/sast-scan-freshness.tslines 931-943. Both feed digest and equality comparisons that must agree exactly across the two packages.The two implementations currently produce identical output. The shared version sorts with an explicit
compareStringscomparator, and this version uses the defaultArray.prototype.sort(); both yield UTF-16 code-unit order for string keys. A future change to either one would silently break digest agreement, and the failure would surface asCONTEXT_DRIFTrather than as a serialization bug.Export the canonical serializer from
packages/sharedand import it here.As per coding guidelines: "Place shared API contracts in
packages/shared".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts` around lines 1103 - 1113, Remove the local stableJson implementation and reuse the canonical serializer from packages/shared. Export the existing stableJson from the shared SAST scan freshness module, then import and use that symbol wherever this file computes digest or equality serialization, preserving identical output across packages.Source: Coding guidelines
packages/shared/src/types/sast-scan-freshness.ts (1)
884-887: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider precompiling the contract-id patterns.
isContractIdbuilds a newRegExpon every call. This function runs insideisSastScanFreshnessDecisionShapeValidandisSastScanRetryDecisionShapeValid, which the Prisma store calls on every load and persist. A small module-level cache removes the repeated compilation.The ast-grep ReDoS hint is not applicable here. All callers pass literal prefixes, and the pattern contains no nested quantifiers.
♻️ Proposed refactor
+const CONTRACT_ID_PATTERNS = new Map<string, RegExp>(); + function isContractId(value: unknown, prefix: string): value is string { - return typeof value === 'string' && - new RegExp(`^${prefix}:\\/\\/[a-f0-9]{64}$`, 'u').test(value); + if (typeof value !== 'string') return false; + let pattern = CONTRACT_ID_PATTERNS.get(prefix); + if (!pattern) { + pattern = new RegExp(`^${prefix}:\\/\\/[a-f0-9]{64}$`, 'u'); + CONTRACT_ID_PATTERNS.set(prefix, pattern); + } + return pattern.test(value); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/sast-scan-freshness.ts` around lines 884 - 887, Precompile and reuse contract-ID regular expressions instead of constructing one on every call to isContractId. Add a module-level cache keyed by the literal prefix, have isContractId retrieve or create the corresponding pattern, and preserve the existing validation behavior for both freshness decision validators.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/prisma/migrations/20260810030000_sast_scan_freshness_retry/migration.sql`:
- Around line 1-19: Adjust
apps/api/prisma/migrations/20260810030000_sast_scan_freshness_retry/migration.sql
at lines 1-19 to leave the NOT VALID t039_source_check in the initial
transaction and move its VALIDATE CONSTRAINT statement to a follow-up migration.
At lines 234-235 and 267-268, move the
SastScanCoverageDecision_comparison_scope_key and
SastScanAttempt_retryDecisionId_key builds into that separate non-transactional
migration using CREATE UNIQUE INDEX CONCURRENTLY. At lines 335-338, add
SastScanAttempt_retryDecisionId_fkey as NOT VALID in the initial migration and
validate it in the follow-up; indexes and checks on tables created by this
migration require no change.
In `@apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts`:
- Around line 409-462: The persistence behavior in persistRetryDecision
permanently consumes the requested-attempt slot for retryAllowed: false
decisions; confirm the intended policy. If denials are permanent, add a concise
comment at the decision-row creation explaining that the durable unique keys
intentionally prevent later authorization. If denials must be retriable, change
persistence to store only authorized decisions or key rows by decision inputs so
later evaluations can create a new decision, while preserving replay handling
for persisted outcomes.
- Around line 533-554: Update the previousRow lookup to exclude all coverage
decisions from the current scan request, not only the current decision id, so it
selects the newest genuinely comparable predecessor. Preserve the existing
tenant, repository, target, state, and timestamp filters, and decide separately
whether profile-family filtering belongs in the query or remains the evaluator’s
fail-closed PROFILE_FAMILY_INCOMPATIBLE outcome.
In `@apps/api/src/scan-plane/sandbox-runtime-attestation.service.ts`:
- Around line 49-57: Update issue so isSastScanPlanValid(binding.plan) is
evaluated before calling effectivePreflight(binding). Keep the existing
invalid-binding condition and Error('Sandbox runtime attestation binding is
invalid.') path intact, then resolve preflight only after plan validation
succeeds; leave verify unchanged.
In `@apps/api/test/scan-plane/sast-scan-freshness.e2e-spec.ts`:
- Around line 172-187: Rename the test around lifecycleDecision and
SastScanFreshnessService to describe sequence rollback rejection, since the
authority sequence is 1 while the decision sequence is 2. Do not call this a
same-sequence replay; add a separate same-sequence case only if rejection is
intended by the contract.
In `@packages/shared/src/types/sast-wrapper.ts`:
- Around line 446-451: Update the validation around
request.preflight.attestationRef and SCANNER_EXECUTION_REQUEST_INVALID so
attempt-two requests require evidence of a newly executed repository preflight,
not only an attestationRef different from
request.plan.repositoryState.attestationRef. Reuse the existing
repository-preflight state or validation symbol available in this flow, and
preserve the attempt-one equality and existing bounded-identifier checks.
In `@packages/shared/test/sast-scan-freshness.test.mjs`:
- Line 49: Fix the unused destructured binding in the decision-to-core
construction by removing decisionDigest without assigning it to _digest, while
preserving core as the decision object excluding decisionDigest.
---
Nitpick comments:
In `@apps/api/src/scan-plane/prisma-sast-scan-freshness.store.ts`:
- Around line 241-266: Update the sastScanAttempts query used before assigning
previous to filter attempts by attemptNumber less than request.attemptNumber,
while retaining the descending order and take: 1 behavior. This ensures previous
is explicitly the immediate predecessor rather than any existing attempt at or
after the requested attempt.
- Around line 673-693: Add a short jittered delay in runSerializable before each
retryable transaction attempt, after confirming the error is retryable and
another attempt remains. Keep the final-attempt throw behavior unchanged, and
use the existing retry constants or established delay utility if available.
- Around line 1103-1113: Remove the local stableJson implementation and reuse
the canonical serializer from packages/shared. Export the existing stableJson
from the shared SAST scan freshness module, then import and use that symbol
wherever this file computes digest or equality serialization, preserving
identical output across packages.
In `@apps/api/src/scan-plane/sandbox-runtime-attestation.service.ts`:
- Around line 405-431: Extract the shared preflight validation predicate from
effectivePreflight and isSastScannerWrapperExecutionRequestValid into
packages/shared, exporting it from the shared SAST wrapper types. Preserve the
8192-byte attestationRef bound, inventoryDigest match, decision/isolation rules,
pathPolicyVersion bound, and attempt-based attestationRef rule, then replace
both local implementations with calls to the shared predicate.
In `@apps/api/src/scan-plane/sast-scan-freshness.service.ts`:
- Around line 327-347: Inject the existing clock or reference time used by
evaluate into the verify flow and pass it to targetStillCurrent. Replace the
direct Date.now() call in targetStillCurrent with that injected time while
preserving the existing five-second future-skew boundary.
- Around line 258-264: Update readTargetObservation to explicitly return the
unavailable result whenever result.status is not VERIFIED, rather than only when
it equals UNAVAILABLE. Preserve the existing VERIFIED path, and ensure future
statuses cannot produce authority: 'VERIFIED'.
In `@apps/api/src/scan-plane/scan-plane.module.ts`:
- Around line 172-181: Add a startup log or alert in the module initialization
path that clearly reports when UnavailableSastLatestTargetAuthority and
UnavailableSastRetryRuntimeAuthority are wired for SastLatestTargetAuthority and
SastRetryRuntimeAuthority. Keep the existing fail-closed provider mappings
unchanged and ensure the signal is emitted once during application startup.
In `@packages/shared/src/types/sast-scan-freshness.ts`:
- Around line 884-887: Precompile and reuse contract-ID regular expressions
instead of constructing one on every call to isContractId. Add a module-level
cache keyed by the literal prefix, have isContractId retrieve or create the
corresponding pattern, and preserve the existing validation behavior for both
freshness decision validators.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e89dbcce-6200-47fe-837d-77feedb75d04
📒 Files selected for processing (34)
apps/api/prisma/migrations/20260810030000_sast_scan_freshness_retry/migration.sqlapps/api/prisma/schema.prismaapps/api/src/scan-plane/prisma-sast-scan-freshness.store.tsapps/api/src/scan-plane/prisma-sast-scanner-runtime.store.tsapps/api/src/scan-plane/sandbox-runtime-attestation.service.tsapps/api/src/scan-plane/sast-latest-target-authority.tsapps/api/src/scan-plane/sast-retry-admission.gate.tsapps/api/src/scan-plane/sast-retry-runtime-authority.tsapps/api/src/scan-plane/sast-scan-freshness.service.tsapps/api/src/scan-plane/sast-scan-freshness.store.tsapps/api/src/scan-plane/sast-scanner-runtime.service.tsapps/api/src/scan-plane/scan-plane.module.tsapps/api/src/scan-plane/scanner-workspace-manifest.service.tsapps/api/test/scan-plane/sast-finding-correlation-persistence.e2e-spec.tsapps/api/test/scan-plane/sast-finding-lineage-persistence.e2e-spec.tsapps/api/test/scan-plane/sast-scan-coverage-persistence.e2e-spec.tsapps/api/test/scan-plane/sast-scan-freshness-persistence.e2e-spec.tsapps/api/test/scan-plane/sast-scan-freshness.e2e-spec.tsapps/api/test/scan-plane/sast-scanner-runtime.e2e-spec.tspackages/shared/src/index.tspackages/shared/src/types/sast-scan-freshness.tspackages/shared/src/types/sast-wrapper.tspackages/shared/test/sast-scan-freshness.test.mjsspecs/006-production-sast-runtime-design/contracts/sast-runtime.mdspecs/006-production-sast-runtime-design/data-model.mdspecs/006-production-sast-runtime-design/plan.mdspecs/006-production-sast-runtime-design/quality-gates.mdspecs/006-production-sast-runtime-design/quickstart.mdspecs/006-production-sast-runtime-design/research.mdspecs/006-production-sast-runtime-design/spec.mdspecs/006-production-sast-runtime-design/tasks.mdspecs/006-production-sast-runtime-design/threat-model.mdtest/github-actions/active-feature.test.mjstest/github-actions/ontology.test.mjs
1 participant
🎋 작업 중인 브랜치 및 이슈
feat/278-006-stale-scan-retry🔎 주요 변경 사항
sast-scan-freshness-v1shared 계약에 latest-target observation, freshness/comparability 판정, bounded retry 계획·시도·결과와 canonical digest 및 exact-shape validator를 추가했습니다.ScanPlaneModule은 다음 단계에 T040 service만 export하며 SCM writer, route, AI payload 또는 외부 publication 권한은 열지 않습니다.✅ 컨벤션 확인
type/issue-number-short-feature형식을 따르나요?<type>: <description>형식을 따르나요?Check List
검증
corepack pnpm lintcorepack pnpm typecheckcorepack pnpm buildcorepack pnpm --filter @aegisai/api prisma:validatenode --test test/runtime/*.test.mjs(1/1)Verify Workspace성공git diff --check006 진행 상태
Closes #278
Summary by CodeRabbit
New Features
Documentation
Tests