Repository navigation
Add advisory review and risk checks for the shadow week - #94
Conversation
|
@greptileai review |
…ets. Closes gabe4coding/puredori#12 Co-authored-by: Gabe <gpavanello@thefork.com>
The tier job runs on pull_request_target with the base workflow, rules, and classifier. The PR head is fetched as data and Python runs isolated. A Greptile Review check on the current head can raise the tier and cannot lower it. Reviewer config files are T2-hard. Co-authored-by: Gabe <gpavanello@thefork.com>
Only a completed Greptile Review check from greptile-apps with conclusion success is a pass. Every other case, including a missing, pending, neutral, or stale check, raises the deterministic tier one step and stops at T2-hard. Co-authored-by: Gabe <gpavanello@thefork.com>
[Critical impact] Adds automated code review and risk classification to CI. Fix the workflow-move classification before merging; a PR can disable CI without receiving the required hard tier.
|
| if f.status == "D": | ||
| return Reason("T2-hard", "workflow-weakened", f"deletes workflow `{f.path}`") |
There was a problem hiding this comment.
Removed workflows get lower tiers
Moving .github/workflows/test.yml outside .github/workflows/ disables it. Git reports an unchanged move as R with no added or removed lines, but workflow_reason only treats D as deletion. The move therefore receives T2-soft instead of T2-hard, hiding the removal of a running CI check. Treat moves out of the workflow directory as workflow deletions.
| m = re.match(r"diff --git a/(.*) b/(.*)$", line) | ||
| cur = FileChange(path=m.group(2) if m else line.split(" b/", 1)[-1]) |
There was a problem hiding this comment.
Quoted filenames miss risk rules
parse_diff does not decode Git's quoted filenames. For a non-ASCII path such as src/café.ts, Git can quote both header paths. Neither the regex nor its fallback extracts the filename, so cur.path becomes the whole header. A changed line containing process.env then misses the src/** rule and receives T1 instead of T2-soft. Read filenames from NUL-separated Git output or decode the quoted paths.
| lost = sum(1 for f in tests for l in f.removed if ASSERTION.search(l)) | ||
| gained = sum(1 for f in tests for l in f.added if ASSERTION.search(l)) | ||
| deleted_tests = [f.path for f in tests if f.status == "D"] | ||
| if deleted_tests: | ||
| reasons.append(Reason("T1", "test-deletion", f"deletes {', '.join(deleted_tests[:3])}")) | ||
| elif lost > gained: |
There was a problem hiding this comment.
New assertions hide deleted checks
Comparing total removed and added assertion counts lets unrelated new assertions cancel deleted checks. Removing assert.match(warn.warnings[0], /env var is not set/) and adding assert.ok(true) elsewhere leaves lost == gained, so the PR becomes T0 despite losing coverage. The configured T0 rule says no assertion may be removed. Check removals conservatively rather than treating any new assertion as a replacement.
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
Description reviews stay stale
The general pass reviews the PR title and body, but this trigger omits edited. If an author corrects a body-versus-diff finding without pushing another commit, the review comment remains stale. Adding unsupported claims after a clean review also goes unchecked. Include title and body edits in the review trigger.
| types: [opened, synchronize, reopened, ready_for_review] | |
| types: [opened, synchronize, reopened, ready_for_review, edited] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
A successful Greptile Review check is no longer enough. The latest greptile-apps[bot] summary must review the current head at 4/5 or 5/5, and that bot must have no P0 or P1 inline comment on that SHA. The claude-code-mode#47 review (success check, confidence 0/5, three P1s) stays a non-pass and raises the tier one step. Co-authored-by: Gabe <gpavanello@thefork.com>
Intent
Start the shadow week of the validation loop on this repo, the same setup as gabe4coding/puredori#10 and gabe4coding/retroagent#58. Two new checks run on every PR. Both are advisory: they are not required, they never fail a PR, and nothing here merges, approves or changes repo settings.
review(.github/workflows/review.yml): two passes in one job.ci/github/shall-review.yml..github/scripts/general_review.py): one tool-less, isolatedclaude -pcall that checks for bugs, missing or weaker tests, silent behavior changes, PR body vs diff drift and irreversible changes. The PR text is passed as untrusted data. It also records the risk-judgment JSON from the design, which does not change the tier yet.::noticethat names the secret.risk(.github/workflows/risk.yml,.github/scripts/risk_tier.py,.github/risk.yml): the deterministic tiers HOLD / T2-hard / T2-soft / T1 / T0.risk tiercheck run (success for T0/T1, neutral for T2/HOLD, never failure) and as arisk:*label.AUTOMERGEisoff, every PR becomes T2-hard. It is read only.versionchange inpackage.json(merging it publishes to npm and tagsv<version>), pluspublish.yml.bin/**, the CLIs and their flags (src/cli.ts,src/computer/cli.ts,src/mobile/cli.ts,src/suite/native-cli.ts,src/suite/options.ts), env loading (src/core/env.ts), the Jev key and model pin (src/jev/provider.ts), the gate floors (scripts/gates.test.mjs), the source rules (scripts/source-rules/**), the agent PR gate (scripts/pr-gate.mjs,scripts/validation-stamp.mjs,.claude/settings.json,.codex/hooks.json) and the publicaction.yml.src/**that matchessecret|redact|mask|password|process.env|api_key. This catchesmaskPasswordsinsrc/browser/page.tsand the secret checks insrc/core/spec.ts.plugins/**is generated, so it doesn't count toward the size limits, but it is not docs.scripts/gates.test.mjsis a gate, not a test.liveskips on this PR because no runtime files change..github/scripts/test_risk_tier.pyandtest_general_review.py, run by therisk (classifier tests)job.Secrets to add (Settings > Secrets and variables > Actions):
TYPESAFE_API_KEY: needed for shall's Jev rule selection.ANTHROPIC_API_KEYorCLAUDE_CODE_OAUTH_TOKEN: needed forclaude -pin both passes.TYPESAFE_API_KEYis probably already set, becauselive.ymluses it. If so, only the Claude secret is missing.This PR is T2-hard by its own rules, because it adds
.github/risk.ymland the review and risk workflows. Gabe reviews and merges it.Evidence
uv run --no-project --python 3.12 --with pyyaml==6.0.2 --with pytest -- python -m pytest -q .github/scriptspasses. actionlint is clean on both workflows.check:docs,check:agent-notes,check:sourceandcheck:examplespass.npm testandcheck:depsneed Node 22+ (this box has Node 20), sotestin CI is the proof.scripts/validate.mjs' live steps don't apply: no runtime files change.T2-hard(hard paths:.github/risk.yml, the scripts and both workflows).scripts/gates.test.mjs; sensitive-path: touchesscripts/source-rules/casts.mjsscripts/source-rules/env.mjs; sensitive-code:src/core/aria-changes.tschanges a line withprocess.envscripts/source-rules/baseline.json; sensitive-path: touchesscripts/source-rules/casts.mjsscripts/source-rules/stdout.mjsscripts/gates.test.mjssrc/core/spec.tschanges a line withsecret; sensitive-code:src/suite/validate.tschanges a line withsecretsrc/cli.ts; sensitive-path: touchessrc/computer/cli.tsscripts/source-rules/baseline.jsonTotals: T0 6, T1 6, T2-soft 8.
Things to compare by hand this week: whether each T2-soft really needed you, and whether any T1 should have been T2.
Intent
Shadow week, updated. The hosted reviewer is Greptile. This removes the
claude -preview job (it ran PR-controlled code with API secrets) and hardens the risk tier so a PR cannot set or lower its own tier..github/workflows/review.yml). Nothing in apull_request_targetjob readssecrets.*.general_review.pyis still in the tree and still tested; no workflow runs it.risk(.github/workflows/risk.yml,.github/scripts/risk_tier.py,.github/risk.yml): tiers HOLD / T2-hard / T2-soft / T1 / T0.pull_request_target. The workflow, rules, and classifier are the base branch's. The PR head is fetched as data (git diff/git show) and no PR file is executed. Python runs with-I, uv with--no-config.pull_requestwithcontents: readonly and no stored git credentials.git diff --name-status -z. The diff uses--text --no-textconvand lines split on\nonly. A test-only PR that changes an assertion or adds skip/only/todo is T1. If the file list and the diff disagree, the tier is T2-hard.greptile-appson the PR's current head SHA. Labels and comments are not read.successandneutralcount as a pass and never lower the tier. A completed failure, stale, cancelled, timed_out, action_required, or startup_failure raises the tier one step and stops at T2-hard (it does not create HOLD). A missing or pending check is not a pass and does not change the tier. When Greptile's check completes, the tier job runs again.risk tiercheck run (success for T0/T1, neutral for T2/HOLD, never failure) and as arisk:*label.AUTOMERGE=offmakes every PR T2-hard.greptile.json,.greptile/**,.coderabbit.yaml,.coderabbit.yml.versionchange inpackage.json(merging it publishes to npm) andpublish.yml.Until this workflow is on the default branch,
pull_request_targetwill not run the tier job for this PR. The classifier tests run from the PR.Evidence
uv run --no-config --no-project --python 3.12 --with pyyaml==6.0.2 --with pytest -- python -m pytest -q .github/scripts: 95 passed, 7 skipped..github/workflows.secrets.reference remains inreview.yml(the file is gone) or in any job that runs onpull_request_target.Totals: T0 6, T1 6, T2-soft 8.