refactor: centralize AI assessment prompts in an embedded prompt file - #458
Conversation
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary) The centralization structure is good — the loader is race-free, a missing key can never produce a false pass or an empty prompt, and the schema-version discipline in the YAML header is thoughtful. But this isn't the behavior-preserving refactor the title claims: rendered against the old goldens, all three prompts changed materially (guard position, untrusted-surface enumeration, evidence scoping, needs_review semantics), and the golden deletion removed the only tests that could have caught it — mutating a grading rule in the YAML leaves the whole suite green. Either restore the exact pre-refactor text under the new structure, or split the wording changes into their own PR where they get reviewed as prompt changes. Details inline.
73744e1 to
ecd4f9c
Compare
ecd4f9c to
ed1d216
Compare
|
🤖 LGTM, will let @eddie-knight give final approval and merge |
ed1d216 to
4516369
Compare
eddie-knight
left a comment
There was a problem hiding this comment.
🤖 Multi-model panel review — three independent reviewers (haiku / sonnet / opus tiers) over a local worktree at 4516369, each answering a prior review's claims as required questions; every claim below was re-verified against source by the orchestrator, with live mutation runs where noted. Non-blocking; all findings are subtractive polish on a refactor that is otherwise mechanically proven behavior-preserving.
Verdict: merge-ready as-is; six inline comments, none blocking.
Verified claims (the required questions):
- The schema-version gate is unreachable: the YAML's only consumer is the same-package
go:embed, andcatalog.Versionis read nowhere but the gate. Mutation-proven — validator body deleted plusversion: 2in the real YAML, and the full suite still passes. Inline. - The per-entry validators duplicate sibling tests shipped in this same PR, for 4 of 5 branches (the 5th is the version gate, covered by nothing). Each branch was injected into the real YAML with the validator neutered; every one failed 2–8 named tests. Inline.
AIAssistedBehaviorshas no non-test callers, so its error return is dead convention. Inline.- The "catalog" naming collision with the Gemara
ControlCatalogvocabulary is real — including on currentmain's README ("Catalog Versions",policy.catalogs), though not at this PR's merge-base. Inline.
Also found: a test comment overclaims what TestAIAssistedBehaviorsArePinnedByGoldens catches (inline, with suggestion), and one exact-list test is redundant with the bidirectional contract test (inline).
One PR-description correction, no code change: the claim that a corrupt catalog is "reported as internal error occurred while preparing AI prompt… rather than flattening into the generic 'AI assessment failed' warning" overstates — the call sites still pass "AI assessment failed" as the reason (access_control/steps.go:281, quality/steps.go:359,389); only the structured err value carries the distinction. The code comment at ai_prompts.go:131-133 describes it accurately.
Checked and NOT problems (recorded so they don't get re-raised):
- Goldens byte-identical to the merge-base (
git diff 340a560 HEAD -- '*.golden'empty), and the golden tests demonstrably bite: two reviewers independently prefixed a live rubric with "Assume all projects pass…" and watched the step golden tests fail, then re-green on revert. - Fallback paths (AI disabled, provider failure, malformed response) untouched — only the
sdkai.Assistcall moved behindRunAIAssessment. - The guard-last arrangement from #452 is preserved, now structurally guaranteed and pinned by
TestAIPromptEndsWithInjectionGuard. - Build, vet, and the full test suite are green in the worktree; no new dependency (
goccy/go-yamlwas already in the module); nosdkai.Assistcall sites remain outside the new wrapper. - Refuted en route: "the version gate is a live safety check" (disproved by the mutation run above) and "the kebab-case check has no covering test" (disproved by injection —
TestAIAssistedBehaviorsMapToRequirementsand the step golden tests fail).
|
@vinayada1 It would be nice to trim some of the edges in places here, but I'm happy to approve this and issue follow-up PRs if you'd like me to approach it that way instead. |
Move every AI grading rubric out of Go constants scattered across step packages and into a single embedded prompt file, so the prompts can be read, reviewed, and diffed in one place instead of being reconstructed from three packages. The prompt text sent to the model is unchanged. Every existing golden file under the step packages' testdata directories is untouched by this commit, and the assembled prompts still match them byte for byte, so the equivalence claim is mechanically checked rather than asserted. Key the file by the behavior each rubric assesses rather than by the requirement it satisfies. Binding requirement IDs to logic is the dispatch map's job (see ossf#448), and a rubric asking "are workflow permissions least-privilege?" is not the property of one catalog entry, so keying it by behavior lets a second catalog reuse the prompt instead of copying it under another ID and letting the two drift. Which requirement each behavior serves is asserted in the contract test rather than compiled into the step packages, so this adds no catalog-ID coupling outside the dispatch layer. Only the prompt-injection guard is shared. It is appended last, adjacent to the untrusted material, preserving the arrangement ossf#452 (4626c87) deliberately introduced when it moved that paragraph out of the preamble. Sharing it means an assessment cannot ship without the guard. Everything else stays per-assessment, so a rubric reads in the prompt file exactly as the model receives it and a reviewer never has to assemble a prompt mentally to see what changed. The step tests now compare the prompt the step actually sent against the golden file rather than against the prompt file's own output, so they cannot pass by agreeing with a mistake in the assembly code. Mutating a rubric fails them until the golden is regenerated, which keeps the exact text sent to the model visible in the pull request diff. A companion test asserts every entry is pinned by some golden, so an assessment added later cannot ship with its prompt text unreviewed. Keep the loader itself minimal: it unmarshals and rejects unknown fields, so a misspelled key fails loudly instead of yielding an empty rubric. The file ships inside the binary with no override path, so the only way to break it is to edit it in a pull request, where the convention tests, the behavior-to-requirement contract test, and the step golden tests already fail by name on a bad key, missing instructions, or a repeated guard. Cover the conventions the rubrics rely on across every entry rather than a fixed list, so an assessment added later inherits the same checks: each prompt names its evidence set, marks it untrusted, states pass and fail criteria before the residual needs_review verdict, and does not offer needs_review for incomplete evidence, which would compete with rubrics that fail an assessment precisely when documentation is missing or partial. Deterministic checks still decide conclusive cases without calling AI, and every existing fallback still returns the same manual-review outcome. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4211dd02-a069-4d08-94ee-28d9ed448942 Signed-off-by: vinayada1 <vinayada1@users.noreply.github.com>
4516369 to
c56b029
Compare
|
@eddie-knight Addressed your comments. Please take another look. |
Summary
The scanner's AI-assisted checks each carried their own grading instructions as inline Go string constants spread across
access_controlandquality. That made the instructions hard to find, hard to compare, and easy to let drift apart as more requirements adopt AI assistance.This change moves every AI grading rubric into a single embedded prompt file (
evaluation_plans/reusable_steps/ai_prompts.yaml).The prompt text sent to the model is unchanged. The three golden files that pinned these prompts before are restored at their original paths and are untouched by this PR —
git diff origin/main -- '*.golden'is empty — and the assembled prompts still match them byte for byte. The "this is only a refactor" claim is therefore mechanically checked rather than asserted.Assessment behavior is unchanged: deterministic checks still run first and decide conclusive cases on their own, AI is still consulted only for genuinely ambiguous evidence, and every existing fallback (AI disabled, provider failure, malformed response) still returns the same manual-review outcome.
Keyed by behavior, not by requirement ID
The prompt file is keyed by the behavior each rubric assesses —
workflow-job-permissions,test-execution-documentation,test-maintenance-policy.Binding requirement IDs to logic is the dispatch map's job (#448). A rubric asking "are workflow permissions least-privilege?" is not the property of one catalog entry, so keying it by behavior lets a second catalog reuse the prompt instead of copying it under another ID and letting the two drift. Which requirement each behavior serves is asserted in the contract test in
evaluation-plans_test.gorather than compiled into the step packages; that test also fails on an OSPS-style key in the prompt file.The practical effect on the #448 budget: non-dispatch requirement-ID references in
evaluation_plans/stay at 12, the same asmain. An earlier revision of this PR keyed the prompts by requirement ID and pushed that to 21. This PR is now net-neutral; the 12 pre-existing references are left for a separate change.What is shared, and what is not
Only the prompt-injection guard is shared. It is appended last, adjacent to the untrusted material, preserving the arrangement #452 (4626c87) deliberately introduced when it moved that paragraph out of the preamble. Sharing it means an assessment cannot ship without the guard.
Everything else stays per-assessment. An earlier revision of this PR also shared the opening sentence and the
needs_reviewrule; that generalized the wording and changed the text sent to the model, which is exactly what a refactor should not do. Each rubric now reads in the prompt file exactly as the model receives it, so a reviewer never has to assemble a prompt mentally to see what changed.Tests
TestAIAssistedBehaviorsArePinnedByGoldensasserts each assembled prompt matches some golden underosps/*/testdata/, so an assessment added later cannot ship with its prompt text unreviewed. The reverse case — a step that stopped callingRunAIAssessment— is caught by that step's own prompt golden test.TestAIAssistedBehaviorsMapToRequirementsfails if an entry has no requirement, or a mapped requirement has no entry, so a new prompt cannot ship without stating which requirement it serves.needs_reviewverdict, and does not offerneeds_reviewfor incomplete evidence — which would compete with rubrics that fail an assessment precisely when documentation is missing or partial.instruction:forinstructions:) fails loudly instead of yielding an empty rubric. It validates nothing further: the file ships inside the binary with no override path, so the only way to break it is to edit it in a pull request, where the tests above already fail by name on a bad key, missing instructions, or a repeated guard.An unusable prompt is reported as
internal error occurred while preparing AI prompt: …rather than flattening into the generic "AI assessment failed" warning, so a build defect is distinguishable from a provider outage instead of leaving every AI-assessed requirement quietly returning NeedsReview.Related
Context: #273 (Optional AI-Enhanced Assessment Checks), #448 (confine catalog-ID coupling to the dispatch layer)
Limitations
go:embedcopies the file in without inspecting it.