-
Notifications
You must be signed in to change notification settings - Fork 131
feat: greenfield spec-test synthesis for dbt-pr-review (P0–P2) #991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
anandgupta42
wants to merge
9
commits into
main
Choose a base branch
from
feat/greenfield-spec-test-execution
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+4,264
−11
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
850a9c5
feat: greenfield spec-test synthesis for dbt-pr-review (P0: propose-o…
anandgupta42 41c586f
feat: [P1] verdict-layer enforcement for spec-test blocking
anandgupta42 649183b
feat: [P1] track-A execution — verify declared-but-unenforced dbt con…
anandgupta42 d5055d1
fix: [dbt-pr-review] address bot review findings + sandboxed track-B …
anandgupta42 57e3a6d
feat: [dbt-pr-review] fetch track-B prompt from compiled core with in…
anandgupta42 416bab7
feat: [dbt-pr-review] P2 corrective app-memory (read-path)
anandgupta42 0086cb3
test: register review_spec_test_prompt in dispatcher completeness test
anandgupta42 122b316
fix: [dbt-pr-review] address #991 review findings (security + correct…
anandgupta42 73e93fb
fix: [dbt-pr-review] address #991 round-2 re-review findings
anandgupta42 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # Core change: move the track-B generation prompt into the compiled binary | ||
|
|
||
| Status: pending apply in `altimate-core-internal` · 2026-07-07 | ||
|
|
||
| The greenfield spec-test **track-B** generation prompt should ship compiled into | ||
| the core binary (IP-in-binary), mirroring the AI reviewer | ||
| (`review_ai_system_prompt`). The altimate-code side is **already wired**: | ||
|
|
||
| - `native/types.ts` declares `altimate_core.review_spec_test_prompt`. | ||
| - `native/altimate-core.ts` registers a handler that calls | ||
| `core.reviewSpecTestSystemPrompt()` **defensively** — if the running addon | ||
| predates the method, the handler fails and `spec-test-gen.ts` | ||
| (`resolveSystemPrompt`) falls back to the inline `buildSystemPrompt()`. So there | ||
| is **no runtime regression** before the core ships; the move activates | ||
| automatically once the addon is rebuilt. | ||
|
|
||
| ## Why this wasn't pushed to the core repo automatically | ||
|
|
||
| At implementation time `altimate-core-internal` had an unclean working tree | ||
| (unrelated WIP in `.claude/settings.json`, `.codex/hooks.json`, | ||
| `.github/workflows/ci.yml` + a stash) on a stale working branch 549 commits | ||
| behind `origin/main`. Applying below on a clean branch off `origin/main`. | ||
|
|
||
| ## Apply on a clean branch off `origin/main` | ||
|
|
||
| In `crates/altimate-core-node/src/review.rs`, inside the existing | ||
| `// altimate_change start … end` block, add: | ||
|
|
||
| ```rust | ||
| /// System prompt for the greenfield spec-test generation lane (track B). | ||
| /// | ||
| /// Ships compiled in the binary so the TS transport carries no prompt IP, matching | ||
| /// `review_ai_system_prompt`. Keep in sync with the TS fallback in | ||
| /// `spec-test-gen.ts` (`buildSystemPrompt`) until that fallback is removed. | ||
| const SPEC_TEST_GEN_SYSTEM_PROMPT: &str = "\ | ||
| You propose dbt generic tests for a newly added dbt model.\n\ | ||
| Rules:\n\ | ||
| - Use ONLY the provided specSources. Do not infer expected values from current output or observed data.\n\ | ||
| - Propose dbt generic tests only. Fill dbtTest; do not use assertionSql.\n\ | ||
| - Every proposal must copy one derivedFrom object from specSources exactly, including derivedFrom.ref.\n\ | ||
| - The derivedFrom.ref must be one of the provided refs. Never invent refs.\n\ | ||
| - Allowed kind values: not_null, unique, accepted_values, relationships, range.\n\ | ||
| - Return ONLY a JSON array of GeneratedTest objects. Return [] when there is no grounded proposal."; | ||
|
|
||
| /// Return the greenfield spec-test generation system prompt string. | ||
| #[napi] | ||
| pub fn review_spec_test_system_prompt() -> String { | ||
| SPEC_TEST_GEN_SYSTEM_PROMPT.to_owned() | ||
| } | ||
| ``` | ||
|
|
||
| Then rebuild the napi addon (release/CI) so `reviewSpecTestSystemPrompt` is | ||
| exported. After it ships and is confirmed live, the inline `buildSystemPrompt` | ||
| fallback in `spec-test-gen.ts` may be deleted. | ||
|
|
||
| ## Verification once the addon is rebuilt | ||
|
|
||
| - `spec-test-gen.ts resolveSystemPrompt()` returns the core prompt (not the | ||
| fallback); confirm via a debug log or by diffing the two strings. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| # Spec: Corrective, attribute-keyed app-memory | ||
|
|
||
| Status: draft · Owner: anand · 2026-07-07 | ||
| Scope: shared substrate; first consumers = `dbt-pr-review` test-acceptance + | ||
| reviewer finding-acceptance. Companion to `greenfield-spec-test-synthesis.md` (§P2). | ||
|
|
||
| ## 1. Problem | ||
|
|
||
| Two memory needs in the product, both currently unserved or served badly: | ||
|
|
||
| 1. **dbt-pr-review learning.** The spec-test-synthesis agent generates tests per | ||
| repo. Some a team keeps, some it dismisses as noise. Today that signal | ||
| evaporates — the next PR regenerates the same dismissed tests. | ||
| 2. **altimate-code's own reviewer/skill memory.** Stored today as raw markdown | ||
| traces (`MEMORY.md` is already ~34KB and self-warning about size). | ||
|
|
||
| Both are the failure the BAIR "Intelligence is Free" piece names: **file-based | ||
| raw-trace memory doesn't scale, and raw traces induce repeated mistakes.** Its | ||
| prescription — the thing to build — is *structured, corrective* memory: organize | ||
| across attributes, each `*` (universal) or a specific value, and store the | ||
| **correction**, never the trace. | ||
|
|
||
| The move that makes this defensible (and not just a cache): store author- | ||
| *committed* corrections keyed by the attributes of the situation, retrieve by | ||
| most-specific match, and **never let a learned entry silently escalate a block.** | ||
|
|
||
| ## 2. Data model | ||
|
|
||
| A memory entry is `(scope, correction, provenance)`. | ||
|
|
||
| ```ts | ||
| interface MemoryScope { | ||
| project: string // tenant / repo — always specific (isolation boundary) | ||
| // Each below: a specific value OR "*" (universal). Retrieval matches specificity. | ||
| modelLayer?: string | "*" // staging | intermediate | marts | ... | ||
| table?: string | "*" | ||
| column?: string | "*" | ||
| operation?: string | "*" // date | dedup | join | pii | ... | ||
| warehouse?: string | "*" | ||
| category?: string | "*" // ReviewCategory, or a domain tag | ||
| derivedFromKind?: string | "*" // for test-acceptance: contract|grain|range|referential|not_null | ||
| } | ||
|
|
||
| interface MemoryEntry { | ||
| id: string // fingerprint of the canonicalized scope + directive | ||
| scope: MemoryScope | ||
| directive: string // the correction: "prefer X" / "do NOT flag Y" / "suppress test kind Z here" | ||
| polarity: "prefer" | "suppress" // reinforce vs. suppress the behavior | ||
| provenance: { | ||
| source: "accepted_pr" | "explicit_dismiss" | "human_rule" | ||
| committed: boolean // true only when a human/PR action ratified it | ||
| supportCount: number // times observed; merges increment, they don't append | ||
| lastSeen: string // ISO; drives decay/expiry | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Non-negotiables that keep this from becoming raw traces: | ||
|
|
||
| - **Store the directive, not the transcript.** An entry is a single correction, | ||
| not a session log. No PR bodies, no diffs, no model output. | ||
| - **Merge, don't append.** Two observations with the same canonical scope+directive | ||
| update one entry (`supportCount++`, refresh `lastSeen`) — the store does not grow | ||
| per event. This is what bounds size where `MEMORY.md` doesn't. | ||
| - **Corrective, not descriptive.** "when deduping `int_*` models, dismiss | ||
| `referential` tests on unenforced sources" — a rule that changes future | ||
| behavior, not a note about the past. | ||
|
|
||
| ## 3. Retrieval | ||
|
|
||
| Given a situation described by the same attributes, return the merged set of | ||
| applicable entries, **most-specific match wins** on conflict: | ||
|
|
||
| - Score each matching entry by count of non-`*` scope fields that match. | ||
| - Higher specificity overrides lower (a `column`-specific `suppress` beats a | ||
| `layer`-wide `prefer`). | ||
| - Ties → higher `supportCount`, then more recent `lastSeen`. | ||
| - Expired/low-support entries (`supportCount < N` past a decay window) are not | ||
| returned — a single fluke never becomes a standing rule. | ||
|
|
||
| Retrieval is a pure function of `(scope query, store)` → deterministic, testable. | ||
|
|
||
| ## 4. Consumers | ||
|
|
||
| ### 4.1 dbt-pr-review test acceptance (the P2 loop) | ||
|
|
||
| - **Write:** when a generated test's proposed patch is merged → `accepted_pr` | ||
| `prefer` entry keyed by `(project, modelLayer, derivedFromKind, category)`. | ||
| When a proposed test is explicitly dismissed → `explicit_dismiss` `suppress` | ||
| entry, same key. | ||
| - **Read:** `SpecTestGenInput` gains a `priors: MemoryEntry[]` field. The | ||
| generator is told: prefer kinds this repo keeps, avoid kinds it dismisses. | ||
| Generation gets sharper per-repo each PR **without** retraining anything. | ||
|
|
||
| ### 4.2 Reviewer finding acceptance (generalizes static rubric exclusions) | ||
|
|
||
| - **Write:** a dismissed/muted finding → `suppress` entry keyed by the finding's | ||
| `(category, model/column, ruleKey)`. | ||
| - **Read:** the orchestrator's post-filter (orchestrate.ts:1336, currently | ||
| `exclusionReason` over the static rubric) additionally consults learned | ||
| `suppress` entries — turning the hand-written exclusion list into a learned one. | ||
|
|
||
| ### 4.3 altimate-code reviewer/skill memory | ||
|
|
||
| Same store replaces the raw-`.md` reviewer memory: corrections keyed by | ||
| `(operation, table, column, warehouse)` instead of prose files. The Spider2 | ||
| `builder.txt` improvements are already de-facto corrective memory — this promotes | ||
| that pattern into the layer itself. | ||
|
|
||
| ## 5. Safety: learned memory must not silently escalate | ||
|
|
||
| Mirrors the test-gen honesty rule. A learned entry may: | ||
|
|
||
| - **bias generation** (which tests to propose), and | ||
| - **suppress/downgrade** advisory findings (raise precision). | ||
|
|
||
| A learned entry may **NOT**: | ||
|
|
||
| - escalate any finding to blocking, or manufacture a new blocking finding. | ||
|
|
||
| Rationale: `suppress`/`prefer` only ever *reduce* noise or *reweight* proposals — | ||
| both fail safe. Escalation off a learned pattern would let accumulated bias | ||
| force `REQUEST_CHANGES`, exactly the "livelock / drift" risk. Blocking stays | ||
| gated on deterministic engine proof (equivalence, executed author-declared | ||
| constraints), never on memory. `provenance.committed` is recorded so a future | ||
| governance tier *could* allow human-ratified escalation, but P0 does not. | ||
|
|
||
| ## 6. Storage & governance | ||
|
|
||
| - **Per-project, tenant-scoped** (the isolation boundary; `project` is never `*`). | ||
| - Backing store: append-and-compact JSONL or a small SQLite table — but the API | ||
| is `get(scopeQuery)` / `record(entry)` with merge semantics, so the backend is | ||
| swappable. No embeddings needed (structured attribute match, not semantic | ||
| search — the article's specific critique of the KG/embedding approach). | ||
| - **Reviewable & exportable:** entries are inspectable; `.altimate/review.yml` | ||
| P0 exposes only `memory.entries`. FUTURE governance can add `pin` | ||
| (force-keep) or `forbid` (never-learn) scopes. Memory is a config surface, | ||
| not a black box. | ||
| - **Decay:** entries below support threshold expire; the store self-prunes. | ||
|
|
||
| ## 7. Build phases | ||
|
|
||
| - **P0 — read-path with seeded rules.** The `MemoryEntry` schema + deterministic | ||
| `get(scopeQuery)` retrieval + `.altimate/review.yml` hand-authored entries. | ||
| Wire into `SpecTestGenInput.priors` and the reviewer post-filter. No learning | ||
| yet — proves the retrieval + safety model with static data. | ||
| - **P1 — write-path.** Record `accepted_pr` / `explicit_dismiss` entries from PR | ||
| merge/dismiss events with merge semantics + decay. | ||
| - **P2 — reviewer/skill memory migration.** Move altimate-code's raw-`.md` | ||
| reviewer memory onto the same store. | ||
|
|
||
| ## 8. Interfaces (summary) | ||
|
|
||
| | File | Change | | ||
| |------|--------| | ||
| | `corrective-memory.ts` (new, shared) | `MemoryScope`, `MemoryEntry`, `get()`, `record()` with merge/decay | | ||
| | `spec-test-gen.ts` | `SpecTestGenInput.priors`; generator prompt consumes them | | ||
| | `orchestrate.ts` | post-filter consults learned `suppress` entries | | ||
| | `config.ts` | P0 `memory.entries`; FUTURE `pin` / `forbid` scopes | | ||
|
|
||
| ## 9. Non-goals | ||
|
|
||
| - Not a semantic/embedding memory — deliberately structured attribute match. | ||
| - Not cross-tenant — `project` is the hard isolation boundary. | ||
| - Not a path to new blocking decisions (§5). | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.