fix(assessments): review degerlendirmesi inherent skorunu residualin altina indirebiliyordu - #2
Closed
ersinkoc wants to merge 1 commit into
Closed
Conversation
…altina indirebiliyordu Kok neden: "residual skor inherent skoru asamaz" invarianti risk olusturma/duzenleme formlarinda (risks/_validate.php) ve 'residual' tipi degerlendirmede (assessments/_validate.php) dogrulaniyordu, ancak 'review' tipi icin koruma yoktu. Review, likelihood/impact (ve uretilen inherent_score) kolonlarini degistirdigi icin mevcut residual skorun ALTINA dusen bir inherent kabul ediliyordu: ornek 5x5=25 inherent, 4x4=16 residual olan riskte 2x3=6 review kabul edilir, tabloda residual_score(16) > inherent_score(6) kalirdi. Etki: tum "etkin skor" hesaplari (COALESCE(residual_score, inherent_score) - risks/index.php filtreleri, reports/_reports.php, api/dashboard_charts.php, reports/executive_summary.php) riskin kendisinden buyuk deger gosterir; register ve matris celisir. Cozum: assessment_collect_input() icinde review tarafi icin tek bir dogrulama dalı eklendi - riskte residual varsa yeni skor residual skordan kucuk olamaz; esit/uzerinde/residualsiz review'ler etkilenmez. Kanit: round-owned proof-script GERCEK dogrulayiciyi stub db() koza semaiyle calistirdi: oncesinde FAIL (hata donmuyordu), sonrasi PASS (8/8 kontrol). Dayanikli regresyon testi tools/assessment_validation_test.php eklendi (11 kontrol, DB gerektirmez, uretim dogrulayicisini gercekten calistirir). Not: bu dal main uzerinden acilmistir; PR #1/#2'ye bagimliligi yoktur.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="assessments/_validate.php">
<violation number="1" location="assessments/_validate.php:96">
P1: When a residual assessment commits after this check reads a risk without a residual, the review can still lower inherent below the newly committed residual because validation runs outside the transaction and is not repeated. Lock the risk row and re-check this invariant inside the same transaction that inserts the assessment and updates `risks`.</violation>
</file>
<file name="tools/assessment_validation_test.php">
<violation number="1" location="tools/assessment_validation_test.php:106">
P3: The new regression assertions check `$e !== []` / `$e === []` against the full error map, so the review-lowering rejection can pass whenever any unrelated validation error is present, and the acceptance cases fail if an unrelated error is added to the same path. Pin the assertions to the invariant error (key `impact` and expected message) so the tests specifically guard the new residual/inherent rule rather than error presence in general.</violation>
<violation number="2" location="tools/assessment_validation_test.php:171">
P3: When this script starts immediately before midnight, the future-date case can become today's date before validation and fail spuriously. Generate a date at least two days ahead or freeze the clock for this assertion.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| indiremez: kalıcı tabloda residual > inherent kalır ve tüm etkin | ||
| skor hesapları (COALESCE(residual, inherent)) riskin kendisinden | ||
| büyük çıkar. Önce residual güncellenmelidir. */ | ||
| if ($type === 'review' && $risk !== null && $likelihood !== null && $impact !== null |
There was a problem hiding this comment.
P1: When a residual assessment commits after this check reads a risk without a residual, the review can still lower inherent below the newly committed residual because validation runs outside the transaction and is not repeated. Lock the risk row and re-check this invariant inside the same transaction that inserts the assessment and updates risks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At assessments/_validate.php, line 96:
<comment>When a residual assessment commits after this check reads a risk without a residual, the review can still lower inherent below the newly committed residual because validation runs outside the transaction and is not repeated. Lock the risk row and re-check this invariant inside the same transaction that inserts the assessment and updates `risks`.</comment>
<file context>
@@ -89,6 +89,21 @@ function assessment_collect_input(): array
+ indiremez: kalıcı tabloda residual > inherent kalır ve tüm etkin
+ skor hesapları (COALESCE(residual, inherent)) riskin kendisinden
+ büyük çıkar. Önce residual güncellenmelidir. */
+ if ($type === 'review' && $risk !== null && $likelihood !== null && $impact !== null
+ && $risk['residual_likelihood'] !== null && $risk['residual_impact'] !== null
+ && ($likelihood * $impact) < ((int)$risk['residual_likelihood'] * (int)$risk['residual_impact'])) {
</file context>
| [, $e] = assessment_test_collect([ | ||
| 'risk_id' => '7', 'assessment_type' => 'review', | ||
| 'likelihood' => '2', 'impact' => '3', | ||
| 'assessed_at' => date('Y-m-d', strtotime('+1 day')), |
There was a problem hiding this comment.
P3: When this script starts immediately before midnight, the future-date case can become today's date before validation and fail spuriously. Generate a date at least two days ahead or freeze the clock for this assertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/assessment_validation_test.php, line 171:
<comment>When this script starts immediately before midnight, the future-date case can become today's date before validation and fail spuriously. Generate a date at least two days ahead or freeze the clock for this assertion.</comment>
<file context>
@@ -0,0 +1,190 @@
+[, $e] = assessment_test_collect([
+ 'risk_id' => '7', 'assessment_type' => 'review',
+ 'likelihood' => '2', 'impact' => '3',
+ 'assessed_at' => date('Y-m-d', strtotime('+1 day')),
+]);
+check('gelecek tarihli assessed_at reddedilir', isset($e['assessed_at']));
</file context>
Suggested change
| 'assessed_at' => date('Y-m-d', strtotime('+1 day')), | |
| 'assessed_at' => date('Y-m-d', strtotime('+2 days')), |
| 'risk_id' => '7', 'assessment_type' => 'review', | ||
| 'likelihood' => '2', 'impact' => '3', | ||
| ]); | ||
| check('review 2x3=6 vs residual 16 reddedilir', $e !== [], |
There was a problem hiding this comment.
P3: The new regression assertions check $e !== [] / $e === [] against the full error map, so the review-lowering rejection can pass whenever any unrelated validation error is present, and the acceptance cases fail if an unrelated error is added to the same path. Pin the assertions to the invariant error (key impact and expected message) so the tests specifically guard the new residual/inherent rule rather than error presence in general.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/assessment_validation_test.php, line 106:
<comment>The new regression assertions check `$e !== []` / `$e === []` against the full error map, so the review-lowering rejection can pass whenever any unrelated validation error is present, and the acceptance cases fail if an unrelated error is added to the same path. Pin the assertions to the invariant error (key `impact` and expected message) so the tests specifically guard the new residual/inherent rule rather than error presence in general.</comment>
<file context>
@@ -0,0 +1,190 @@
+ 'risk_id' => '7', 'assessment_type' => 'review',
+ 'likelihood' => '2', 'impact' => '3',
+]);
+check('review 2x3=6 vs residual 16 reddedilir', $e !== [],
+ $e === [] ? 'hata donmedi; residual > inherent kalici olurdu' : 'reddedildi');
+
</file context>
ersinkoc
changed the title
Owner
|
Teşekkürler. Dört bulguyu da doğruladım, dördü de gerçekti. Özellikle schema.sql'deki eksik users tablosu ciddiydi: belgelenmiş Düzeltmeleri main'e bağımsız olarak uyguladım (2530ae1). PR'ları merge
cubic'in son admin korumasına yaptığı P1 eleştirisini de hesaba kattım: Eklenen regresyon testleri:
#7 açık kalıyor - orada karşılığı olmayan bir test var, kapsamı Tekrar teşekkürler, başka bulgunuz olursa memnuniyetle bakarım. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hata
'Gözden geçirme' (review) değerlendirmesi, bir riskin inherent skorunu mevcut residual skorunun ALTINA indirebiliyordu — ve bu durum doğrulamadan geçip kalıcı hâle geliyordu.
Örnek: inherent 5×5=25, residual 4×4=16 olan geçerli bir riske
likelihood=2, impact=3(skor 6) girilen bir review kabul ediliyor; sonuçta tablodaresidual_score (16) > inherent_score (6)kalıyordu.Kök neden
"Residual skor inherent skoru aşamaz — kontroller riski artırmaz" invariantı iki yazma yolunda zaten korunuyordu:
risks/_validate.php(residual > inherent reddedilir),'residual'tipi değerlendirme →assessments/_validate.php(mevcut inherent ile karşılaştırılır).Ancak
'review'tipi için koruma yoktu: review,likelihood/impact(ve üretileninherent_score) kolonlarını değiştirdiği için stored residual ile karşılaştırılmadan yazılıyordu.Etki: kalan tüm "etkin skor" hesapları —
COALESCE(residual_score, inherent_score);risks/index.phpfiltreleri,reports/_reports.php,api/dashboard_charts.php,reports/executive_summary.php— riskin kendisinden büyük değer gösterir; register ve matris çelişir.Çözüm
assessment_collect_input()içine tek bir doğrulama dalı eklendi: riskte residual skor varsa, review'ın yeni inherent skoru residual skordan küçük olamaz. Eşit/üzerinde olan review'ler ve residual'ı olmayan riskler etkilenmez. Kullanıcıya residual'ı önce güncellemesi söylenir.Kanıt ve doğrulama
db()yerine gerçek MySQL'in vereceği risk satırını döndüren koza) →review 2x3=6 vs residual 16 is rejected — no error returned(exit 1).tools/assessment_validation_test.php— 11 kontrol, DB gerektirmez, üretim doğrulayıcısını gerçekten çalıştırır:php tools/assessment_validation_test.php.php -ltemiz. Tam kapı testi (tools/smoke_test.php) bu Windows kutusunda çevresel nedenlerle çalışmıyor; hatanın bu değişiklikle ilişkisi yok.Kapsam çiti
Bu PR yalnızca
assessments/_validate.phpvetools/assessment_validation_test.phpdosyalarını değiştirir. Dal doğrudanmainüzerinden açılmıştır; PR #1 ve PR #2'ye bağımlılığı yoktur, herhangi bir sırayla birleştirilebilir.Summary by CodeRabbit
Bug Fixes
Tests
Not (upstream): Bu PR fork'tan
CodeByPinar/riskops:main'e açılmıştır; gövdedeki "PR #1 / PR #2" referansları fork'taki yerel numaralardır. Bu PRmainüzerindedir ve diğer PR'lere bağımlılığı yoktur.