ci: require adversarial high-risk regression gates - #420
Closed
leehack wants to merge 41 commits into
Closed
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical gate-bypass and classification issues, along with evidence-validation gaps, remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an exact-head high-risk regression gate with path classification, adversarial evidence validation, documentation, and durable tests.
Changes:
- Adds GitHub workflow and high-risk contract validation.
- Updates testing policy, contributor guidance, PR template, and ownership.
- Adds regression coverage for gate behavior and test-matrix output.
File summaries
| File | Findings |
|---|---|
tool/testing/test_matrix.dart |
Nit (4): invalid-tier help omits the advertised high-risk tier. |
tool/testing/check_high_risk_pr_contract.dart |
Critical (4): structured-output and streaming paths can bypass classification. Critical (2): test-only chat_template paths are misclassified.Moderate (2): release-sensitive files are missed. Moderate (4, 3, 3, 2): evidence checks accept fabricated or insufficient references. |
test/unit/tooling/test_matrix_test.dart |
No final comments. |
test/unit/tooling/high_risk_pr_contract_test.dart |
No final comments. |
doc/testing_matrix.md |
No final comments. |
AGENTS.md |
No final comments. |
.github/workflows/high_risk_regression_gate.yml |
Critical (3): validator runs from the mutable PR checkout. Critical (1): rename handling can bypass classification. Moderate (3): review-thread changes do not trigger revalidation. |
.github/pull_request_template.md |
No final comments. |
.github/CODEOWNERS |
No final comments. |
Review details
Suppressed comments (10)
tool/testing/check_high_risk_pr_contract.dart:218
isStructuredOutputis also set for grammar-only production changes, but this check accepts only template test directories. Existing production grammar coverage is undertest/unit/core/grammarandtest/integration/core/grammar; a valid grammar change with tests in those locations will be rejected as missing durable tests. Include the grammar/engine production test paths or key the requirement to the changed surface.
'High-risk production or policy changes require a durable *_test.dart '
'change on the same PR.',
);
}
tool/testing/check_high_risk_pr_contract.dart:345
- When affected weights are unavailable, this condition accepts either
upstreamorfixture. Thus anN/A:claim can omit one half of the required proof even though the policy requires primary upstream emissions plus durable fixtures. Require both terms here, as the exact-format validator already does.
final lower = affected.toLowerCase();
if (lower.startsWith('pass:') && affected.length < 20) {
errors.add(
'Affected-family real model/artifact PASS evidence must identify the '
'model/artifact and result.',
tool/testing/check_high_risk_pr_contract.dart:104
- The gate's own durable contract test is not part of the policy classification. A PR that changes only
test/unit/tooling/high_risk_pr_contract_test.dart(including deleting adversarial assertions) is treated as standard even though the documented high-risk surface includes changes to this gate. Add the test path to the regression-policy allowlist.
path == 'tool/testing/test_matrix.dart' ||
path == 'tool/testing/check_high_risk_pr_contract.dart') {
tool/testing/check_high_risk_pr_contract.dart:84
- The artifact allowlist omits existing runtime consumers
lib/src/hook/native_bundle_config.dartandexample/chat_app/web/index.html: the former controls code-assets bundle selection and the latter bootstraps the deployed bridge. Changes to either can alter shipped artifacts/runtime wiring while being classified as standard and skipping artifact evidence. Add these concrete consumer paths (or classify the owning directories).
if (path == 'hook/build.dart' ||
path.startsWith('tool/native/') ||
tool/testing/check_high_risk_pr_contract.dart:345
- The N/A branch does not verify that the text names even one exact unavailable family, let alone every affected family: any 35-character string containing
unavailableandupstreamorfixturepasses. This contradicts the field's required exact-family audit and allows the runtime limitation to be omitted. Parse family identifiers or require them in a verifiable evidence record.
final lower = affected.toLowerCase();
if (lower.startsWith('pass:') && affected.length < 20) {
errors.add(
'Affected-family real model/artifact PASS evidence must identify the '
'model/artifact and result.',
tool/testing/check_high_risk_pr_contract.dart:81
ModelCachePlatform.supportsImplicitSharedModelCacheis a public capability probe inlib/src/core/models/download/model_download_manager_base.dart, but a change to that file matches none of the classifier branches. Such a PR would be treated as standard and could skip the exact-head QA required by the policy's capability-probe scope; add capability-surface matching and a test for this path.
if (path.startsWith('lib/src/backends/') ||
path.startsWith('lib/src/core/engine/') ||
path.startsWith('lib/src/core/speech/') ||
path == 'lib/src/core/models/chat/content_part.dart') {
surfaces.add(HighRiskSurface.backendRuntime);
tool/testing/check_high_risk_pr_contract.dart:246
- The rejection contract documented in
doc/testing_matrix.mdrequires unknown, missing, mismatched, wrong-type, and malformed structures, but this required-term list omitsmismatched. A PR body can therefore claim all validated categories while omitting mismatch cases and still pass the gate; include the missing term and an adversarial validator test.
_requirePassEvidence(
fields,
errors,
'Compiled grammar valid upstream emissions',
const ['compiled', 'upstream'],
tool/testing/check_high_risk_pr_contract.dart:252
- The contributor matrix explicitly requires empty containers and zero-argument calls, but this check only looks for generic type words plus
empty. Evidence that mentions those words without exercising zero-argument behavior will pass, so the #408 failure class is not represented by the contract; require an explicit zero-argument/empty-container evidence marker and test it.
_requirePassEvidence(
fields,
errors,
'Compiled grammar rejection matrix',
const ['compiled', 'unknown', 'missing', 'wrong-type', 'malformed'],
tool/testing/check_high_risk_pr_contract.dart:308
- The task fields are described as concrete identifiers, but this helper accepts any non-N/A value of five or more characters. Values such as
abcdeandfghijare accepted as the implementation and independent QA tasks, so the gate does not enforce stable task references or meaningful independence; validate the expected identifier/reference syntax.
String? _requireDetail(
Map<String, String> fields,
List<String> errors,
String label,
tool/testing/test_matrix.dart:102
- The new matrix row presents
tool/testing/run_template_parity_suites.shas a command, but a repository-relative executable must be invoked as./tool/testing/run_template_parity_suites.shin a normal shell. As written, copying the documented command fails withcommand not foundunless the caller has modifiedPATH.
'tool/testing/run_template_parity_suites.sh; run issue-specific VM '
- Files reviewed: 9/9 changed files
- Comments generated: 11
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical CI-gate classification gap and both moderate policy coverage gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (22)
Previously missed (2) — in code that hasn't changed since the last review.
.github/CODEOWNERS:19
test_matrix.dartis explicitly classified as aregressionPolicysurface by the new checker, but it is not protected by CODEOWNERS. A later PR could remove or weaken the high-risk rows without requesting the maintainer owner, undermining this policy. Add the matrix file to the protected paths.
tool/testing/check_high_risk_pr_contract.dart @leehack
test/unit/tooling/high_risk_pr_contract_test.dart @leehack
tool/testing/check_high_risk_pr_contract.dart:252
- The rejection matrix omits
mismatched, althoughdoc/testing_matrix.mdexplicitly requires rejection of unknown, missing, mismatched, wrong-type, and malformed structures. Evidence that mentions every currently checked term can therefore omit the mismatched case. Add this term and update the corresponding template and complete-body fixture together.
This issue also appears in the following locations of the same file:
- line 257
- line 263
'Compiled grammar rejection matrix',
const ['compiled', 'unknown', 'missing', 'wrong-type', 'malformed'],
.github/pull_request_template.md:54
- The generated template leaves this field as an HTML comment, but
_parseEvidenceFieldstreats the comment text as the value and line 133 rejects anything other thanstandardfor a non-high-risk PR. An ordinary PR that leaves the template untouched therefore fails this required check; default the field tostandard(or strip HTML comments in the parser).
- **High-risk classification:** <!-- `standard` or `high-risk` -->
.github/workflows/high_risk_regression_gate.yml:6
- This trigger list does not rerun or invalidate the check when review activity changes or when
mainadvances. A new unresolved thread, or a new base commit while the PR head is unchanged, can leave the old zero-thread/base-distance result green. Add review/review-comment triggers and a base-branch re-evaluation or invalidation path before relying on these live-state checks.
types: [opened, reopened, synchronize, edited, ready_for_review]
.github/workflows/high_risk_regression_gate.yml:108
- The job checks out the exact PR head and then executes the checker from that checkout. A PR can therefore modify
check_high_risk_pr_contract.dart(or the workflow) to return success, making the required gate bypassable. Run a checker sourced from a trusted base/default revision and pass the exact-head state to it, with an explicit trusted bootstrap path for this initial addition.
dart tool/testing/check_high_risk_pr_contract.dart \
tool/testing/check_high_risk_pr_contract.dart:74
- The structured-output matcher only covers template/grammar/engine paths. Public structured-output and tool-schema behavior is also implemented under
lib/src/core/models/inference/structured_output.dartandlib/src/core/models/tools/, so a PR changing those files can be classified as standard and skip all adversarial evidence. Include these public schema/tool paths (and the relevant tool-choice/generation paths) in the high-risk classification.
if (path.startsWith('lib/src/core/template/') ||
path.startsWith('lib/src/core/grammar/') ||
path == 'lib/src/core/engine/chat_completion_stream_parser.dart' ||
path == 'lib/src/core/engine/chat_template_renderer.dart' ||
(path.startsWith('lib/src/') && path.contains('/chat_template'))) {
tool/testing/check_high_risk_pr_contract.dart:224
structuredOutputincludeslib/src/core/grammar/, but this predicate accepts only template test directories. The repository's grammar tests live undertest/unit/core/grammarandtest/integration/core/grammar, so a grammar-only change with the appropriate durable tests will be rejected by this new gate. Include those production-path test directories in the predicate.
final hasTemplateTests = assessment.changedFiles.any(
(path) =>
(path.startsWith('test/unit/core/template/') ||
path.startsWith('test/integration/core/template/')) &&
path.endsWith('_test.dart'),
tool/testing/check_high_risk_pr_contract.dart:351
- The N/A fallback passes when either
upstreamorfixtureis present. The documented fallback requires both primary upstream emissions and durable fixtures, so an unavailable-family claim with only one kind of evidence is accepted for non-structured high-risk changes. Require both terms here, as the exact-format check already does.
} else if (!lower.startsWith('pass:') &&
(!lower.startsWith('n/a:') ||
affected.length < 35 ||
!(lower.contains('unavailable') &&
(lower.contains('upstream') || lower.contains('fixture'))))) {
tool/testing/check_high_risk_pr_contract.dart:330
- These checks treat evidence as keyword-only prose: any
PASS:value containing the required substrings is accepted, without verifying the cited task, test, artifact, or result. For example,PASS: production adversarialsatisfies the negative-evidence check and two arbitrary distinct task strings satisfy the independence check. Bind the contract to verifiable task/check/artifact output or an explicit required human status instead of accepting self-attested keywords.
final value = fields[label] ?? '';
final lower = value.toLowerCase();
if (!lower.startsWith('pass:')) {
errors.add('$label must start with "PASS:" and cite exact evidence.');
return;
tool/testing/check_high_risk_pr_contract.dart:246
- The valid-emission requirement only checks for the words
compiledandupstream, soPASS: compiled upstreampasses without showing that a compiled grammar accepted an actual emitted shape. Require acceptance and emitted-shape evidence as stated by the field contract.
'Compiled grammar valid upstream emissions',
const ['compiled', 'upstream'],
tool/testing/check_high_risk_pr_contract.dart:258
- The schema evidence terms include
emptybut not zero-argument calls, even though the contributor matrix requires coverage of empty containers and zero-argument calls. A passing field can therefore omit the #408 zero-argument regression entirely. Require explicit zero-argument evidence and update the template and fixture.
'Schema-directed types and empty values',
const ['string', 'number', 'boolean', 'null', 'object', 'array', 'empty'],
tool/testing/check_high_risk_pr_contract.dart:264
- This accepts
PASS: partial final rollbackwithout requiring malformed-final content preservation or suppression of incomplete protocol markup, both of which the template and matrix require. Add terms for the missing behaviors so the evidence row cannot be satisfied by a generic rollback claim.
'Partial/final streaming and rollback',
const ['partial', 'final', 'rollback'],
tool/testing/check_high_risk_pr_contract.dart:270
- The tool-choice check requires
thinkingbut notprefix, so a field such asPASS: auto required none thinkingpasses without evidence that reasoning/thinking prefixes were exercised. Require the prefix behavior explicitly, as the template and matrix do.
'Tool choice and thinking prefixes',
const ['auto', 'required', 'none', 'thinking'],
tool/testing/check_high_risk_pr_contract.dart:205
- The deletion/bypass/miswire row only checks those three words, so
PASS: delete bypass miswirepasses without showing that mutation tests fail. Require explicit failing-test evidence or a verifiable mutation-test artifact for this sensitivity claim.
_requirePassEvidence(
fields,
errors,
'Deletion/bypass/miswire sensitivity',
const ['delete', 'bypass', 'miswire'],
tool/testing/check_high_risk_pr_contract.dart:301
_expectExactinterpolates the submitted PR-body value into the workflow summary/log on mismatch. A contributor who accidentally pastes a token, signed URL, or other sensitive text into one of these fields would have it persisted in Actions output. Report only the field name and mismatch (or a redacted, length-limited value), never raw body text.
final actual = fields[label];
if (actual != expected) {
errors.add('$label must be exactly "$expected"; found "${actual ?? ''}".');
tool/testing/check_high_risk_pr_contract.dart:118
docs_version_cut.ymlis a release workflow: it cuts versioned docs from release tags and pushes the result tomain, but its filename matches none ofpublish,release,deploy, orsync. A PR changing this workflow therefore avoidsreleaseAutomationevidence. Add an explicit path or use a release-workflow classification that covers this workflow.
bool _isReleaseWorkflow(String path) {
if (!path.startsWith('.github/workflows/')) return false;
final name = path.split('/').last;
return name.contains('publish') ||
name.contains('release') ||
name.contains('deploy') ||
name.contains('sync');
tool/testing/check_high_risk_pr_contract.dart:346
- For a
PASS:runtime claim, this code checks only that the string is at least 20 characters long; it never verifies the model/artifact identifier or result despite the diagnostic saying those are required. For example, filler such asPASS: xxxxxxxxxxxxxxxxxis accepted as affected-family proof. Validate a concrete evidence reference/identifier instead of length alone.
void _requireRuntimeEvidence(Map<String, String> fields, List<String> errors) {
final affected = fields['Affected-family real model/artifact evidence'] ?? '';
final lower = affected.toLowerCase();
if (lower.startsWith('pass:') && affected.length < 20) {
errors.add(
'Affected-family real model/artifact PASS evidence must identify the '
'model/artifact and result.',
);
tool/testing/check_high_risk_pr_contract.dart:383
- The
PASS:branch for exact affected-format evidence has the same length-only loophole: any 20-character filler is accepted without naming the affected format or result. This allows the structured-output gate to pass without the exact-format proof required by the template and issue. Require a concrete format/evidence reference, not just a minimum length.
final value = fields['Exact affected-format evidence'] ?? '';
final lower = value.toLowerCase();
if (lower.startsWith('pass:')) {
if (value.length < 20) {
errors.add(
'Exact affected-format PASS evidence must identify the format and '
'result.',
);
}
return;
tool/testing/check_high_risk_pr_contract.dart:351
- The
N/A:branch also never checks that an unavailable family is named; it only checks length plus the wordsunavailableandupstream/fixture. A claim such asN/A: unavailable family; upstream fixture evidencecan therefore satisfy the gate without identifying the exact family required by the template. Validate a concrete family/evidence reference.
} else if (!lower.startsWith('pass:') &&
(!lower.startsWith('n/a:') ||
affected.length < 35 ||
!(lower.contains('unavailable') &&
(lower.contains('upstream') || lower.contains('fixture'))))) {
tool/testing/check_high_risk_pr_contract.dart:389
- The
N/A:branch for exact affected-format evidence checks only length and the wordsunavailable,upstream, andfixture; it does not require any format/family name. This permits an N/A claim with no exact unavailable family, contrary to the documented evidence contract. Validate a concrete affected-format reference.
if (!lower.startsWith('n/a:') ||
value.length < 35 ||
!(lower.contains('unavailable') &&
lower.contains('upstream') &&
lower.contains('fixture'))) {
tool/testing/check_high_risk_pr_contract.dart:314
_requireDetailis meant to reject missing task identifiers, but it accepts the template's own HTML placeholder (<!-- Stable task/review identifier. -->) because it only checks length andn/a. Both default task placeholders are long and different, so a high-risk PR can leave them untouched and still pass the independence check. Reject HTML-comment placeholders and add a regression test.
final value = fields[label]?.trim();
if (value == null ||
value.length < 5 ||
value.toLowerCase().startsWith('n/a')) {
errors.add('$label requires a concrete identifier or evidence reference.');
tool/testing/test_matrix.dart:789
- The updated
--tierhelp listshigh-risk, but the invalid-tier diagnostic in_filterRowsstill lists onlyall, essential, targeted, platform, or release. An invalid tier now produces a misleading error; update that diagnostic alongside the new tier.
--tier <tier> Filter by all, essential, targeted, high-risk, platform, or release.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Multiple unresolved critical enforcement gaps remain in freshness, evidence binding, classifier coverage, rename handling, and diagnostics safety.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
tool/testing/enforce_high_risk_pr_contract.dart:85
- The artifact-consumer allowlist includes only
model_download_manager_base.dart, while the actual conditional export and file-backed implementation arelib/src/core/models/download/model_download_manager.dartandlib/src/platform/io/model_download_manager_io.dart(with the controller/stub in the same module). Changes to download/cache behavior can therefore escape this high-risk gate. Match the whole download module and its platform implementation instead of only the interface.
path ==
'lib/src/core/models/download/model_download_manager_base.dart') {
tool/testing/enforce_high_risk_pr_contract.dart:84
- This backend/runtime classification omits the exported runtime configuration surfaces under
lib/src/core/models/config/andlib/src/core/models/inference/model_params.dart(includingpreferredBackendand LiteRT settings). A PR changing backend selection or capability/device routing there will be treated as standard and bypass the required gate. Include these paths and add classifier coverage.
if (path.startsWith('lib/src/backends/') ||
path.startsWith('lib/src/core/engine/') ||
path.startsWith('lib/src/core/speech/') ||
path == 'lib/src/core/models/chat/content_part.dart' ||
path ==
tool/testing/enforce_high_risk_pr_contract.dart:14
- The new public contract declarations (
HighRiskSurface,HighRiskAssessment,HighRiskPrState, andHighRiskContractResult) have no Dartdoc, and the public validation functions below are likewise undocumented. This utility is imported by tests and defines the evidence contract used by the trusted workflow; document the classifications and validation semantics so this boundary remains maintainable.
enum HighRiskSurface {
structuredOutput,
backendRuntime,
artifactConsumer,
releaseAutomation,
regressionPolicy,
}
class HighRiskAssessment {
- Files reviewed: 10/10 changed files
- Comments generated: 11
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical workflow integrity, path-handling, and high-risk classification issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
Previously missed (4) — in code that hasn't changed since the last review.
tool/testing/enforce_high_risk_pr_contract.dart:117
- The classifier does not include
tool/testing/run_template_parity_suites.sh, even though structured manifests are required to use that command. A PR can replace this required evidence command with a no-op and remain standard, avoiding exact-head high-risk validation while weakening the gate. Include the parity command (and its pinned-source/ref inputs) in the regression-policy surface.
}
if (path == 'pubspec.yaml' ||
path == 'pubspec.lock' ||
path.startsWith('hook/') ||
path.startsWith('lib/src/hook/') ||
tool/testing/enforce_high_risk_pr_contract.dart:432
- The manifest is PR-controlled untrusted data, but these diagnostics interpolate its test path into stdout and the GitHub step summary. A malformed path can expose secret-bearing text or inject log/control sequences, contrary to the repository's logging rule. Report fixed, non-sensitive diagnostics (or safely escape/redact the path) instead of echoing
path.
}
final coverage = _stringSet(
structured['requiredCoverage'],
'structuredOutput.requiredCoverage',
errors,
tool/testing/enforce_high_risk_pr_contract.dart:303
familycomes directly from the PR's evidence JSON and is interpolated into the trusted check's output on invalid evidence. This permits a crafted affected-family name to leak arbitrary secret-bearing text or inject log/control sequences. Keep this error generic or escape the untrusted family name before writing it to stdout/step summary.
}
if (evidence['independentQaTask'] != qaTask) {
errors.add('Manifest independentQaTask must match the PR body.');
}
final surfaceNames = assessment.surfaces
.map((surface) => surface.name)
tool/testing/enforce_high_risk_pr_contract.dart:313
- This only requires a non-empty list of strings that look like tags or SHAs; a structured-output manifest containing just
b10549passes even though the documented contract requires both pinned and current upstream references. Distinguish those roles in the manifest and validate both concrete refs, with a missing-current regression case.
final changed = assessment.changedFiles.toSet();
final production = evidence['productionEvidence'];
if (production is! Map<String, dynamic>) {
errors.add('Manifest productionEvidence must be an object.');
.github/CODEOWNERS:20
- The new classifier explicitly treats
tool/testing/test_matrix.dartasregressionPolicy, but this CODEOWNERS block protects only the validator and trusted test. A contributor can therefore rewrite the canonical high-risk rows and their documented command without the maintainer review this policy relies on. Add the matrix script to the protected paths.
tool/testing/verify_release_docs_versions.dart @leehack
.github/workflows/trusted_high_risk_regression_gate.yml:54
head_shaandbase_shaare sampled here, then several live API calls run before enforcement without a final consistency check. A force-push or base advance can make the later file list/evidence describe a different state than the SHAs passed to the checker;cancel-in-progressis asynchronous and does not make this snapshot atomic. Re-fetch both refs immediately before invocation and fail closed if either changed.
[[ "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]]
gh api "repos/$REPOSITORY/pulls/$PR_NUMBER" \
> "$RUNNER_TEMP/pull-request.json"
base_ref="$(jq -r '.base.ref' "$RUNNER_TEMP/pull-request.json")"
head_sha="$(jq -r '.head.sha' "$RUNNER_TEMP/pull-request.json")"
test/unit/tooling/trusted_high_risk_contract_test.dart:375
- This contract assertion uses the REST API status
removed, which GitHub never emits for deleted pull-request files, so the test passes while deletion tracking is broken. Assert thedeletedstatus and cover the renamed-away source path as well.
expect(errors, contains('Exact head SHA does not match'));
tool/testing/enforce_high_risk_pr_contract.dart:356
upstreamParityCommandis accepted only when it namesrun_template_parity_suites.sh, but that script currently buildschat_appand runs Qwen multimodal CPU/WebGPU smokes; it does not run template/parser parity or compare pinned/current upstream refs. A structured-output manifest can therefore satisfy this field without the claimed parity evidence. Require an actual parity suite here, or fix and protect the referenced script.
'upstream+fixture; representative models are not family proof.',
);
tool/testing/enforce_high_risk_pr_contract.dart:169
Independent QA verdictis accepted as the literalPASSfrom the PR body, while the two task fields are validated only as different-looking strings. The workflow never resolves either URI or consumes a trusted QA result, so an author can provide two arbitrary valid-lookingcodex://references and self-certify the independent blocking pass. Bind the verdict to a trusted task/artifact result or make this explicitly human-enforced rather than an enforceable contract.
}
bool _isGateScript(String path) {
- Files reviewed: 10/10 changed files
- Comments generated: 8
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved critical findings affect status freshness, review revocation, and high-risk classification coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
.github/workflows/trusted_high_risk_regression_gate.yml:36
- This privileged
pull_request_targetjob is the gate's trust root, but its external action references are mutable major tags. A moved or compromised tag could execute withstatuses: writeand publish an accepted status without running the trusted checker. Pin the third-party actions in this workflow, including checkout and setup-dart, to immutable full commit SHAs.
if: github.event.pull_request.base.ref == 'main'
runs-on: ubuntu-latest
timeout-minutes: 10
.github/workflows/trusted_high_risk_regression_gate.yml:84
- The Pull Files API filenames are serialized with
jq -rinto newline-delimited files, but Git paths may contain newline characters. A PR can create a filename such as.github/high-risk-evidence/419.json\nother;grep,mapfile, and the DartreadAsLines()call then treat the injected first line as a changed manifest, undermining the exactly-one-changed-manifest check (and the same issue applies to deletion paths). Keep these records as JSON/NUL-delimited data end-to-end, or reject control characters before writing them as lines.
[[ "$head_repo" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]
tool/testing/enforce_high_risk_pr_contract.dart:90
- The structured-output classifier only marks the parser and renderer filenames here, but
chat_completion_request_planner.dart,engine.dart, andchat_session.dartare the public tool/response-format/streaming paths, andlib/llamadart.dartre-exports those APIs. A PR changing one of those files is high-risk only asbackendRuntimeand can therefore omit the compiled-grammar, schema-type, streaming, and tool-choice evidence required for structured changes. Classify the complete chat engine/public barrel (or inspect changed symbols) and add table-driven coverage.
path == 'lib/src/core/engine/chat_completion_stream_parser.dart' ||
path == 'lib/src/core/engine/chat_template_renderer.dart' ||
tool/testing/enforce_high_risk_pr_contract.dart:437
- The testing matrix explicitly requires coverage of
wrong-typerejection, but this set only requiresmismatched-type. A manifest can therefore omit the documented wrong-type axis and still pass the trusted contract.
'mismatched-type',
'malformed-output',
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect status publication, final revalidation, and CI-run selection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (12)
Previously missed (3) — in code that hasn't changed since the last review.
.github/workflows/trusted_high_risk_regression_gate.yml:814
- This bare
[[ ... ]]runs underset -e, so a head change exits the final publisher before the failure POST below. The same happens for the bare base assertion at line 826 and for any failing final revalidation API call; because the snapshot exists, the earlier fail-closed step is skipped and the required status can remain pending instead of being revoked. Handle these failures as explicitfinal_revalidation_failedstate and always reach the owner-checked failure publication.
This issue also appears on line 823 of the same file.
shell: bash
.github/workflows/trusted_high_risk_regression_gate.yml:858
- This condition accepts any object-shaped
upstreamRefs, including{}or an object missingpinned/current; the followingjq -rcalls then producenullandgh api .../commits/nullexits underset -ebefore the failure status is posted. This is reachable for a rejected non-structured high-risk manifest because those fields are not validated, so the gate can remain pending. Restrict the final lookup to the same validated structured-output shape as the initial resolver and convert lookup failures into a failure result rather than aborting.
> "$RUNNER_TEMP/final-live-pull-request.json"
final_head="$(jq -r '.head.sha' "$RUNNER_TEMP/final-live-pull-request.json")"
[[ "$final_head" == "$HEAD_SHA" ]]
.github/workflows/trusted_high_risk_regression_gate.yml:883
- An upstream tag or ref can change between the initial and final resolution. This
exit 1runs before the final status-owner check and POST, so the required context is left at the earlier pending status rather than receiving the promised failure. Record the mismatch as a failed final revalidation and continue to publish failure.
-f owner="$owner" -f name="$name" -F number="$PR_NUMBER" \
-f query="$GRAPHQL_QUERY" --jq "$THREADS_JQ"
)"
gh api --paginate --slurp \
"repos/$REPOSITORY/pulls/$PR_NUMBER/reviews?per_page=100" \
.github/high-risk-evidence/419.json:41
- The PR description says the policy-only bootstrap manifest has
structuredOutput: nulland parity is N/A, but this manifest supplies a structured-output object. The new CI job gates its pinned/current parity execution on this field, so this branch will run parity instead of the documented N/A path. Align the manifest with the stated bootstrap boundary or update the description and validation evidence.
"structuredOutput": {
"compiledAcceptanceTests": [
"test/integration/core/grammar/generated_tool_schema_grammar_test.dart"
],
"compiledRejectionTests": [
.github/workflows/trusted_high_risk_regression_gate.yml:92
- Every evaluation unconditionally overwrites this context with its own pending URL. If an older run reaches this POST after a newer evaluation has posted pending, it can steal the owner; the newer run then exits at its owner check and the stale run may publish the final result (or leave pending) for the context.
cancel-in-progressdoes not make an in-flight API call atomic, so guard pending publication with a monotonic owner/lease that is also enforced at final publication.
gh api --method POST "repos/$REPOSITORY/statuses/$EVENT_HEAD_SHA" \
-f state=pending \
-f context='High-Risk Regression Gate / Trusted exact-head adversarial evidence' \
-f description='Trusted high-risk evidence is being evaluated' \
-f target_url="$RUN_URL" \
> /dev/null
.github/workflows/trusted_high_risk_regression_gate.yml:942
- The owner read and status POST are not atomic. A newer evaluation can post its pending status after this check but before this POST; this run will then overwrite the newer owner's pending status with a stale result, and the newer run will later see the wrong owner and exit. The final publication needs a serialization/lease mechanism that cannot be invalidated between the ownership check and write.
filename, previous_filename, status, sha
}] | sort_by(.filename, .previous_filename, .status, .sha)' \
"$RUNNER_TEMP/final-live-pull-files-pages.json"
printf '%s\n' "$final_threads"
jq -cS . "$RUNNER_TEMP/final-live-high-risk-reviews.json"
.github/workflows/trusted_high_risk_regression_gate.yml:826
- Because this step runs with
set -e, a base advance makes the[[ ... ]]assertion terminate the publisher before the final status POST. The existing required context is therefore left pending instead of being changed to failure, contrary to the final revalidation contract. Handle the mismatch by settingstate=failure/final_revalidation_failed=trueand continue to the status publication.
EVIDENCE_PATH: ${{ steps.evidence.outputs.path }}
PARITY_FILE: ${{ steps.ci.outputs.parity_file }}
TRUSTED_PARITY_FILE: ${{ steps.trusted_parity.outputs.file }}
VERIFIED_UPSTREAM_COMMITS: ${{ steps.upstream.outputs.file }}
.github/workflows/trusted_high_risk_regression_gate.yml:76
- For a
workflow_runfromCI,workflow_run.pull_requestscan also be empty, not just for the review-signal workflow. This fallback is gated onHigh-Risk Review Signal, so a fork PR's completed CI run exits before it can identify the PR/head and re-evaluate the gate; the initial result can never be replaced by a successful exact-head result. Recover the candidate PR/head from the trusted run metadata for every producer (then verify it through the live Pull Request API), or otherwise add an equivalent CI association path.
if [[ -z "$EVENT_PR_NUMBER" || -z "$EVENT_HEAD_SHA" ]]; then
[[ "$EVENT_WORKFLOW_NAME" == "High-Risk Review Signal" ]]
[[ "$EVENT_SIGNAL_TITLE" =~ ^high-risk-review-signal-pr-([1-9][0-9]*)-head-([0-9a-f]{40})-base-main$ ]]
EVENT_PR_NUMBER="${BASH_REMATCH[1]}"
EVENT_HEAD_SHA="${BASH_REMATCH[2]}"
fi
.github/workflows/trusted_high_risk_regression_gate.yml:19
- Resolving a review thread emits none of the configured triggers (the documentation explicitly notes there is no thread-resolution Actions event), but
unresolvedThreads != 0is a hard validator failure. If the gate runs while a thread is open, resolving it cannot rerun the gate, so the required status stays failed until an unrelated PR event. Add a reliable resolution recheck, or avoid making this transient count a required-status failure and rely on the repository conversation rule.
workflow_run:
workflows: [CI, High-Risk Review Signal]
types: [in_progress, completed]
.github/workflows/trusted_high_risk_regression_gate.yml:936
- The final owner check assumes that the newest status with this context is either this run or a trusted newer run. Commit statuses can also be written by repository collaborators, so a user can post a success status with this context and make
latest_ownerdiffer from the current pending URL; the workflow then exits successfully without publishing its result. Verify the status creator and bind it to a trusted run before honoring this early exit, or use a publisher-only check run.
{
jq -cS '{
body, changed_files, user: .user.login,
head: .head.sha, head_repo: .head.repo.full_name, base: .base.sha
}' "$RUNNER_TEMP/final-live-pull-request.json"
.github/workflows/trusted_high_risk_regression_gate.yml:33
- When
workflow_run.pull_requestsis omitted for the fork-safe review signal, this expression falls through toworkflow_run.id; the step only recovers the PR number later fromdisplay_title(lines 71-75). Multiple review signals for the same PR can therefore run concurrently, and an older evaluation can post success after a newer signal posted pending, making stale review state authoritative. Use the signal title (or another recovered PR/head key) in the concurrency group before falling back to the run ID.
high-risk-regression-${{
github.event.pull_request.number ||
github.event.workflow_run.pull_requests[0].number ||
github.event.workflow_run.id ||
github.run_id
tool/testing/enforce_high_risk_pr_contract.dart:182
- Every evidence manifest path is classified only as
regressionPolicyhere. A later PR can therefore rewrite an existing structured-output manifest, setstructuredOutputto null, and pass the policy-only validation; the trusted path collector will then stop importing that manifest's parity/compiled-test references, weakening protection for future PRs. Evidence manifests should be append-only (require the changed manifest to be newly added/current to the PR) or their trusted-base structured claims must be preserved and revalidated.
path == 'scripts/build_chat_app_web.sh' ||
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Lite
Copilot stopped reviewing on behalf of
leehack due to an error
August 23, 2026 01:23
22 tasks
…ion-gate-reconcile
…ion-gate-reconcile
leehack
added a commit
that referenced
this pull request
Aug 24, 2026
The shell change ships without assertions; #420 carries them but also asserts workflow files this PR does not touch.
Owner
Author
|
Parking this as a draft. The executable parts are now in #445; what remains here cannot merge in its current form. Findings, all verified against The approval requirement cannot be satisfied by anyone. It currently rejects itself. The body records The published check never appears. The job binds Three requirements are enforced only nominally. Classification is too broad to live with. 9 of the last 10 merged PRs would trip it, including all three open Dependabot PRs — an Also: the concurrency group at None of this says the underlying problem isn't real — #391 shipped regressions that adversarial QA found within minutes. It says this apparatus doesn't catch them. #419's acceptance criteria need rewriting before this is reviewed again, or it keeps being measured against criteria it cannot meet. |
leehack
added a commit
that referenced
this pull request
Aug 24, 2026
…ds (#445) * test: execute the compiled tool-grammar path and bound chat-test builds Issue #419 required the generated tool grammar to survive llama.cpp's own grammar compiler, but nothing asserted it: the existing coverage stopped at the generated GBNF string. Add an integration test that pushes the generated grammar through llama_sampler_init_grammar and through LlamaEngine's production tools/tool-choice path, so a grammar that only looks right but does not compile fails here instead of at inference time. The adversarial case mutates the generated root rule and asserts the native parser rejects it. Bound the llama.cpp chat-test build. An unbounded --parallel spawns one compile job per core, which starves or OOMs constrained machines and CI runners; LLAMA_CPP_CHAT_TEST_BUILD_JOBS defaults to 2 and rejects non-positive-integer values up front rather than passing them to cmake. The e2e wrapper made its environment explicit so the child clearly inherits the parent PATH it needs to configure and build, and the template fixture count is now asserted so a registered fixture disappearing fails loudly instead of silently shrinking the parity matrix. * test: cover the bounded chat-test build parallelism The shell change ships without assertions; #420 carries them but also asserts workflow files this PR does not touch. * test: skip Bash validation on Windows * test: harden compiled grammar native cleanup
Owner
Author
|
Supersession recommendation: use draft #455 for the current high-risk classification/checklist/test-matrix bootstrap and do not merge this historical branch. #455 is based directly on current main, omits the compiled grammar/parity scope already merged in #445, and removes the settings-dependent privileged publisher from this reviewable policy slice. Keep #419 open for the separate GitHub App/protected-environment/status-enforcement phase. This PR remains open and draft until the maintainer chooses the final retirement action after #455 settles. |
Owner
Author
|
Superseded by the smaller, independently qualified policy/tooling slice in #455, now merged. The overlapping compiled-grammar work already landed in #445. Issue #419 remains open for the separate protected status-publisher and settings-enforcement phase; the privileged publisher machinery from this PR was intentionally not merged. |
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
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.
Summary
Part of #419. This bootstrap is not its completion.
Bootstrap boundary
This PR deliberately leaves #419 open. The trusted workflow cannot protect its own bootstrap PR because it is not yet on
main. Live configuration is also incomplete:high-risk-status-publisherreturns HTTP 404, classic branch protection returns HTTP 404, and the only active ruleset is the Copilot review ruleset.After this slice lands, #419 remains open until an administrator creates the narrowly scoped App and protected environment, binds the exact required check to that App ID, enables conversation resolution and strict up-to-date enforcement, and proves the configuration on a follow-up high-risk PR. No settings/environment mutation is part of this PR.
Exclusive publisher authority
GITHUB_TOKEN; it no longer writes commit statuses.actions/create-github-app-tokeninvocation receives only an environment-scoped client ID/private key and requests onlychecks:writeforleehack/llamadart.Exact enforcement
mainPR with the exact live head SHA and repository. Review signals use a deterministic exact PR/head title from the base repository, while CI remains bound to the PR head repository..github/high-risk-evidence/<issue>.jsonfile must change. Its production test paths must be changed, non-deleted durable Dart tests, and trusted-base evidence paths cannot be removed.Validation
mainatea2fe4e05041cd01fae7c745039b22e932a780e0, preserving merged fix: make workspace quality gates deterministic #417 workspace logic, Fix structured-output parser and grammar regressions #421 structured-output recovery, and Fix collision-prone tool grammar rule names #427 collision-free grammar coverageactionlintfor the trusted workflow andgit diff --check: PASSRequired external administrator setup
leehack/llamadart, with Checks read/write, no other write permission, and no webhook.high-risk-status-publisher, restrict it to selected branchmain, disallow bypass, and add environment-scopedHIGH_RISK_STATUS_PUBLISHER_CLIENT_ID/HIGH_RISK_STATUS_PUBLISHER_PRIVATE_KEYsecrets.High-Risk Regression Gate / Trusted exact-head adversarial evidenceonmainand bind it to the App as the expected source. Also require CODEOWNER approval for publisher workflow/settings changes, conversation resolution, and strict up-to-date-with-main.Until these controls exist, the new publisher job fails closed and this PR is not operational completion of #419.
Zero-regression audit
High-risk regression gate