Pular para conteúdo

AUR-576: one review session serves --base and --pr

Measurement (before the change, base 1569d875)

  • baseReview (review_base.go, ~45 fields) and prReview (review_pr.go, ~55 fields) held the same data under different names (repoCfg/cfg, codebaseContextText/codebaseText, contextBlockDigest/contextBlockDig, ruleCatalogDigest/ruleCatalogDig, rawIssues/rawIssuesSnapshot).
  • Two phase lists (runReview: resolveInputs, analyze, decideGate, publish; runPRReview: validate, resolveInputs, analyze, runGate, publish), each with its own analyze sub-list.
  • Two inconclusiveReason (main.go and review_pr_gate.go): base provider_failure/quality_skipped, PR the constant and model_parse_failure; the rest of the ranking was copied.
  • Two exit ladders: base (quality failed > gate > artifacts > fail-on) and PR (comment failures > --exigir-qualidade degraded > status failure > gate > artifacts > fail-on > --check status).
  • runSAST copied (only PR had sastReasonUnverifiedCheckout); writeComplianceArtifacts assembled twice, with run.IssuesForGate() on base and result.Issues on PR.
  • Security findings: base kept them in run.Extra and applied the rule config separately, and its raw verdict snapshot did NOT contain them; PR merged them into result.Issues before the snapshot.
  • os.Getenv("GITHUB_SHA") read in both gate phases (and AURUMCODE_BASE_SHA, GITHUB_REPOSITORY, LLM_MODEL, AURUMCODE_OUTPUT_FILE, AURUMCODE_PR_PERMISSION_MODE across the phases).
  • Package-variable seams: gatePipelineObserver, evaluateGate, newGatePipeline, diffContentDigest (review_gate.go), resolveWithFilesHook (verified_checkout.go), newCacheDigestBuilder (review_cache.go); Now: time.Now fixed in both gate phases.
  • runPRReview took 9 positional parameters.

What changed

  • internal/gate/reason.go: ModelOutcome (reviewed, skipped, provider failed, parse failed), typed Reason constants and RankReason, the one ranking (provider failure > skipped > model parse failure > degraded parse > SAST reason > partial coverage). Tokens are unchanged. NotReviewedRules maps an outcome to "not reviewed" (never, when quality is required, always); the requirement is a typed QualityRequirement (optional, required).
  • internal/gate/exit.go: ExitPolicy(ExitInputs) ExitDecision, one ladder of rungs with typed inputs and a named cause; ExitClean/ExitBehavioral/ ExitFindings. cmd's exitFindings, exitQualityNotReviewed, exitArtifactNotWritten now alias these values.
  • internal/review/session: Phase, Order (resolve, model, evidence, gate, publish), Run, and Source data: LocalDiff and PullRequest with their NotReviewedRules, where the security findings live and the notice printed when "not reviewed" decides. AUR-579 changes the model/evidence order by editing Order only.
  • cmd/aurumcode/review_session.go: reviewState (the shared fields, one name each), reviewDeps (clock, Semgrep runner, gate observer, codebase resolver, prompt digest builder, environment snapshot; zero fields take the production default), reviewEnv (read once by readReviewEnv), reviewIO.
  • cmd/aurumcode/review_evidence.go: the shared evidence steps: runSecurityPass, joinSecurityFindings (the one place that decides where security findings live), snapshotAndApplyRules (snapshot = issues + the kept-apart security findings, on both sources), runSAST(root, blocked), findingsAtThreshold, writeArtifacts (one assembly from the gate run: run.IssuesForGate()).
  • cmd/aurumcode/review_gate.go: the shared gate phase (runGate, gatePipelineInputs, executeGate with the injected observer), inconclusiveReason over gate.RankReason, decideExit over gate.ExitPolicy. The seams became plain calls or injected dependencies.
  • baseReview and prReview embed reviewState, implement session.Phases.Step, and keep only their own fields. review_base_gate.go and review_pr_gate.go are gone; failOnExit, exitCode and the two writeArtifacts are gone.
  • runPRReview(reviewIO, prReviewOptions): the PR number, repository, --publicar, --na-linha and --check joined the options struct. runReviewWith(reviewIO, args) is the entry that takes dependencies.
  • Tests that substituted package seams inject them through reviewDeps (aur524, aur536, aur543, review_gate_test via runReviewWith, runPR499With, runAUR536ReviewWith). No t.Setenv("PATH") was added (6 before, 6 after).

Per-source mapping (data, not code)

Outcome --base --pr
provider failed always not reviewed not reviewed only with --exigir-qualidade
parse failed always not reviewed (not reachable today: base classifies every model error as provider failed) not reviewed only with --exigir-qualidade
skipped gate decides; --exigir-qualidade classifies it as provider failed (AUR-458) not reachable

Unifying these divergences is a product decision for a later card; no e2e exit code changes here.

Exit precedence (one ladder)

comment publish failure (1) > not reviewed (1) > status publish failure (1) > gate breach (3) > gate block (1) > artifact not written (1) > --fail-on (3)

--check status code. It is the union of the two old ladders: base had no publication rungs, and on every input both old ladders already agreed with it, so no fail-closed choice changed an exit. The notice printed when "not reviewed" decides is source data (only --pr prints the --exigir-qualidade line, as before).

Snapshot change (AC-003)

The --base raw verdict snapshot now includes the security pass's findings, as --pr's always did. The verdict-reuse key is unchanged; a verdict stored by an older binary without security findings can only be re-evaluated with fewer findings, and the gate's own security contributor still counts the current run's security findings.

How AC-002 is proven

  • TestAUR576SameInputsSameExitThroughTheSession (cmd/aurumcode): for each row it builds a reviewState per source with the same config, diff, model outcome, findings and injected dependencies (fixed clock, a Semgrep runner that is not installed, an empty environment), then runs the real evidence steps, the real gate pipeline (runGate) and the real exit decision (decideExit), and compares the exit codes and the published policy gate: lines. Rows: provider failure (block), parse failure (block), scanner missing, artifact not written over a --fail-on finding, a finding above the gate threshold, a finding above --fail-on, clean; a row whose gate must speak fails if it published no line. Declared divergent rows: provider failure under warn (--base 1, --pr 0) and provider failure with a breach (--base 1, --pr 3).
  • TestAUR576SameInputsSameExitOnBothSources (internal/review/session): the same table over RankReason, the sources' NotReviewedRules and ExitPolicy.
  • TestExitPolicyPrecedence, TestRankReasonOrder (internal/gate).
  • MUT-001 is three edits, each RED: a second exit ladder in the --base publisher (AC-001's AST test), a reordered shared ladder (the ladder and table tests), and a --base-only rung putting --fail-on above a missing artifact in decideExit (the behavioral table above).

Environment still read outside the snapshot

The review path no longer reads GITHUB_SHA, AURUMCODE_BASE_SHA, GITHUB_REPOSITORY, LLM_MODEL, AURUMCODE_OUTPUT_FILE or AURUMCODE_PR_PERMISSION_MODE outside readReviewEnv. Still read where they were: provider selection (LLM_API_KEY, LLM_BASE_URL, LLM_MODEL, AURUMCODE_LLM_FIXTURE, AURUMCODE_PROMPT_CAPTURE in main.go and the model cache key), the GitHub client (GITHUB_TOKEN, the API URL), the CI context file and AURUMCODE_POLICY (flag resolution). Moving the provider and client construction behind the session is left for the card that splits them (AUR-584).

Re-anchored done acceptances (same semantics, same RED)

Script Was Now
AUR-519 MUT-001 main.go gateInconclusiveReason = "provider_failure" internal/gate/reason.go return ReasonProviderFailure -> return ReasonNone
AUR-557 AC-002-MUT-001 review_pr_gate.go executeGate("--pr", ...) with newGatePipeline review_gate.go res, ok := s.executeGate(pipeline, reason), prefixed with a --pr-only gate.NewPipeline(...[:len-1]...)
AUR-450 MUT-001 review_base_analysis.go printSecurityCoverage(b.stderr, ...) review_evidence.go printSecurityCoverage(s.stderr, ...)
AUR-504 MUT-001 replacement os.Getenv("GITHUB_SHA") replacement p.env().githubSHA (the same snapshot)
AUR-505 MUT-001 pr.go p.qualityDegraded = true pr.go p.model = modelParseFailed

AUR-519's re-anchored mutation now drops provider_failure on both paths, because the ranking is shared; it turns RED through the same test (TestAUR519GateInconclusiveProviderFailureBlocks).

AUR-519's all also listed EvaluateGateFailOnWithoutInconclusiveNeverApproves, renamed by AUR-575 to ...Blocks; the list follows the rename (it failed on main before this card). Anchors that survived unchanged: AUR-537 (both), AUR-449, AUR-473 (both), AUR-542, AUR-448, AUR-522, AUR-543 (N1 included). Already stale before this card (not touched): AUR-451 (result.Issues = combined in pr.go), AUR-490, AUR-513 (main.go), AUR-515 (pr.go), AUR-458 (if qualityFailed { in main.go).

Proof

All commands run in the shared container (.board/bin/go-shared exec -w <worktree> bash -c '...'), in series.

Command Last line
bash tests/acceptance/AUR-576.sh all (after the behavioral AC-002 test) AUR-576/all/MUT-001/rejected, AUR-576/all/MUT-002/rejected, AUR-576/all/pass
AUR-576.sh AC-001 / AC-002 / AC-003 / AC-004 AUR-576/AC-00N/pass each
AUR-519.sh all, AUR-519.sh AC-003-MUT-001 AUR-519/all/pass, AUR-519/AC-003-MUT-001/pass (mutation produced RED)
AUR-557.sh AC-002-MUT-001 AUR-557/AC-002-MUT-001/pass
AUR-450.sh AC-001, AC-001-MUT-001 AUR-450/AC-001/ok, AUR-450/AC-001/MUT-001/rejected
AUR-504.sh all, MUT-001 rc 0, AUR-504/AC-001/MUT-001/rejected
AUR-505.sh all, MUT-001, MUT-002 AUR-505/all/ok, .../MUT-001/rejected, .../MUT-002/rejected
AUR-537.sh all AUR-537/all/pass
AUR-449.sh AC-001, AC-001-MUT-001 AUR-449/AC-001/ok, AUR-449/AC-001/MUT-001/rejected
AUR-473.sh all, MUT-001, MUT-002 rc 0, .../MUT-001/rejected, .../MUT-002/rejected
AUR-542.sh all AUR-542/all/pass
AUR-522.sh all AUR-522/all/pass
AUR-543.sh all, AC-001-MUT-001, AC-001-MUT-002, N1-MUT-001 AUR-543/all/pass, each mutation pass (mutation produced RED)
AUR-524.sh all, AUR-538.sh all, AUR-548.sh all, AUR-556.sh all .../all/pass each

Staged copies without the repository roots (card v3): AUR-575's TestAUR575ExistingConfigsStayValid (internal/config/strict_test.go) failed with "no shipped config.yml was found to check" whenever a staged copy had none of demo, tests, docs, .aurumcode (AUR-550's and, nested, AUR-557's). It now skips with "raízes do repositório ausentes nesta cópia" when no root exists, and still requires at least one checked file and the strict parse when any root exists (AUR-575.sh stages them and requires the PASS).

Measured after the change, in the container, in series: go test -p 1 ./internal/config/ -run TestAUR575 -> ok (checked 64 shipped config.yml files, --- PASS: TestAUR575ExistingConfigsStayValid); AUR-550.sh all -> AUR-550/all/pass; timeout 900 bash tests/acceptance/AUR-557.sh all -> AUR-557/all/pass (every nested script nested-green).

Tutorials (card v4). After AUR-586, run.sh --check refuses an out/ recorded by an image of another tree, and the tree identity hashes every production file of cmd, internal and pkg, so this card's change made every tutorial out/ stale. All 15 tutorials were re-recorded in series with AURUMCODE_TUT_IMAGE=aurum-tutoriais:aur576 bash demo/tutoriais/<nome>/run.sh all (AURUMCODE_TUT_REBUILD=1 on the first) and each run.sh --check passed (all=0 check=0 for all 15). No expected/ file and no docs/tutorials block changed. In out/, 22 logs changed and .imagem moved to the new tree; comparing each old and new log as a sorted multiset of lines, after normalizing volatile values (temporary paths, timestamps, release tags, digests), all 22 are identical: what moved is only the interleaving of stderr lines with stdout lines in the combined log (which depends on when each stream is flushed; this run did not isolate whether the change or scheduling moved it) and the volatile values. No terminal, audit or SARIF content changed. Then, in the container, in series: AUR-575.sh all -> AUR-575/all/pass; AUR-561.sh..AUR-564.sh all -> rc 0 each (last line AC-002-MUT-001/ok); AUR-567.sh all -> AUR-567/all/pass; AUR-569.sh all -> AUR-569/all/pass. docker rmi aurum-tutoriais:aur576 afterwards.

Red before this card, red the same way on base d4a1ee4f (measured in a worktree of that commit), not caused by it:

  • Pending (not fixed here): AUR-448.sh AC-001 -> AUR-448/AC-001/s3-grounded-finding-missing, and therefore AC-001-MUT-001 -> not-rejected (the script itself records that the scope/evidence gate of AUR-547 discards its fixture's finding).
  • AUR-550.sh all -> package-proof-exit:1: its staged copy has no shipped config.yml, so AUR-575's TestAUR575ExistingConfigsStayValid fails (no shipped config.yml was found to check). AUR-550.sh is not in this card's paths. AUR-557 all (which nests 519, 537, 538, 543, 524, 548, 550, 556, 522, 533) exits 1: every other nested script is green when run on its own (table above; AUR-533 printed nested-green), so the red one was AUR-550 (green after the card v3 change below); that was established by elimination, not by capturing its nested line. For the same reason the AUR-557 MUT-001 check "AUR-550 must go RED" proves nothing while AUR-550 is already red on base; its own AC-002 RED (TestAUR557PathsShareOnePipeline) is what holds.

Suite: go vet ./... && go test -p 1 ./... exit 0 (35 packages ok, no FAIL). Required acceptances, all: AUR-575 pass, AUR-578 pass, AUR-567 pass, AUR-569 pass, AUR-568 ok, AUR-538 pass, AUR-556 pass, AUR-533 pass, AUR-497 pass. bash scripts/docs/build.sh (strict) built without a warning.