Add DataJunction deep code-review skill

[robinld]

Summary

Add a maintainer-focused review skill for DataJunction changes. It traces semantic-layer correctness, query grain, materialization, metadata, authorization, API/client compatibility, and lifecycle risks beyond changed lines.

The automated-review reference defines a structured handoff for an advisory bot, including exact-head and discussion verification, existing-thread links, explicit resolution of verified fixes, and one maintained summary. It does not authorize the skill user to publish comments. The v2 handoff does not ask the model to reply in threads or reopen them.

Review feedback

Greptile's initial summary reviewed 5ce0e561, not the current head. Its three findings were addressed in d46692f5: structured reviews require a trusted discussion snapshot, and the strict handoff requires verdict, minimum actions, and test gaps. 17ed7f63 adds focused invariant tracing.

The later DataJunction bot summary also predates the current head. 5bacc47f removed model-directed reply actions and their target fields; the publisher derives writes from validated findings and explicit bot-thread resolutions. 7d928fc4 rejects status: incomplete with an approving verdict. 723bf9c9 clarifies that an incomplete result fails the worker before PR publication, so operational warnings do not appear in PR comments or checks.

Verification

The skill validator passes. Agent and worker policy bundles match this skill, and the internal Newt suites pass (18 agent and 59 worker tests, plus Ruff). This PR changes no application runtime code.

[netlify]

✅ Deploy Preview for thriving-cassata-78ae72 canceled.

Name	Link
🔨 Latest commit	723bf9c
🔍 Latest deploy log	https://app.netlify.com/projects/thriving-cassata-78ae72/deploys/6ac297f6b571a800087752c0

[greptile-apps]

Confidence Score: 4/5

[Low risk] Adds documentation and schemas for a code review skill.

The PR should not merge until structured reviews without a trusted discussion snapshot have a valid handoff path.

Findings

1.  Digest Required Without Snapshot ▶
2.  Publishing Fields Not Required ▶
3.  Conflicting Thread Actions Accepted ▶

Summary

Adds a maintainer-oriented DataJunction review skill and an automated-review handoff with generation and publishing schemas. The review guidance is detailed, but the structured handoff needs a defined snapshot-less path and tighter agreement between its two schemas.

• Align digest requirements with the inputs available to an orchestrated review.
• Make required publishing fields and thread-action constraints enforceable.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Orchestrator inputs] --> B[Review skill]
  B --> C[Model-schema generation]
  C --> D[Handoff-schema validation]
  D --> E[Publisher]
  A -. Trusted discussion snapshot, if supplied .-> C

Loading

_{Reviews (1) · Last reviewed commit: "Add DataJunction adversarial review skil..."}

[datajunction-gh]

---

AI Review

Summary

The PR adds a DataJunction review skill and structured bot handoff. The earlier reported schema fixes are present, but the strict handoff still accepts an approving incomplete review and reply actions whose targets are not validated at the publishing boundary. Request changes before using it for automated publication.

Findings

• ⚠️ .agents/skills/datajunction-review/references/review-result.schema.json:58 The incomplete-status rule requires only a nonempty limitations array. A payload with status: "incomplete", verdict: "approve", no findings, and limitations: ["Timed out before reviewing SQL changes"] passes the strict schema. That contradicts the handoff's rule that an incomplete review is not clean and can carry an approval into the maintained summary or check decision for unreviewed code.

  Reject this combination at the handoff boundary and explicitly require the publisher to treat every incomplete review as non-passing. Add a validation test for the payload above.

• ⚠️ .agents/skills/datajunction-review/references/automated-review.md:80 The publishing checks verify that a targeted thread is bot-authored, but do not require verification that reply_to_comment_id identifies an external author's comment in that same thread. The strict schema also accepts a reply kind with no target or body, and accepts any positive comment ID regardless of thread. A schema-valid model result can therefore request a malformed reply or address the wrong discussion.

  Require coherent all-or-none reply fields and have the publisher check reply-target membership and authorship against its fresh snapshot before any write. Cover missing fields and a comment ID from another thread in focused tests.

Tests

• ⚠️ Test that a payload with incomplete status, an approving verdict, and a nonempty limitation is rejected or cannot produce a passing check.
• ⚠️ Test rejection of missing reply fields and reply targets outside the specified bot-owned thread.

Missing context / blind spots

• ⚠️ The internal pilot publisher and CI or skill-validator logs were not available in this checkout; runtime enforcement could not be verified.

Final Verdict

• Status: ⚠️ Request changes — do not approve
• Minimum required actions:
  • Reject an approving verdict when review status is incomplete, and require the publisher to treat incomplete reviews as non-passing.
  • Validate reply fields together and verify the reply target against the fresh discussion snapshot before publishing.

[datajunction-gh]

DataJunction deep review of 17ed7f631470af49b40ac5efa4d82ae0e5ce88eb. Maintained summary includes off-diff findings. Advisory only.

[datajunction-gh]

The incomplete-status rule requires only a nonempty limitations array. A payload with status: "incomplete", verdict: "approve", no findings, and limitations: ["Timed out before reviewing SQL changes"] passes the strict schema. That contradicts the handoff's rule that an incomplete review is not clean and can carry an approval into the maintained summary or check decision for unreviewed code.

Reject this combination at the handoff boundary and explicitly require the publisher to treat every incomplete review as non-passing. Add a validation test for the payload above.

[datajunction-gh]

The publishing checks verify that a targeted thread is bot-authored, but do not require verification that reply_to_comment_id identifies an external author's comment in that same thread. The strict schema also accepts a reply kind with no target or body, and accepts any positive comment ID regardless of thread. A schema-valid model result can therefore request a malformed reply or address the wrong discussion.

Require coherent all-or-none reply fields and have the publisher check reply-target membership and authorship against its fresh snapshot before any write. Cover missing fields and a comment ID from another thread in focused tests.