Skip to content

Evaluate docs updates#687

Merged
tooky merged 5 commits intomainfrom
evaluate-docs-updates
Mar 9, 2026
Merged

Evaluate docs updates#687
tooky merged 5 commits intomainfrom
evaluate-docs-updates

Conversation

@tooky
Copy link
Contributor

@tooky tooky commented Mar 6, 2026

Address the documentation suggestions from #671 cc @dangrondahl

@tooky tooky requested a review from dangrondahl March 6, 2026 17:38
@tooky tooky enabled auto-merge (squash) March 6, 2026 17:39
Copy link
Contributor

@AlexKantor87 AlexKantor87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR — clean, well-scoped, and the docs feedback from #671 looks fully addressed. The long descriptions are a real improvement: the policy contract explanation (types, exit codes, data.policy.* query mechanism) is exactly what a new user would need. The snyk example in Step 4 is a better choice than the near-duplicate pull-request example — it shows a materially different use case and the ALLOWED result is less likely to confuse readers.

Three minor comments inline, none of them blockers. Happy to approve once you've had a look.

Copy link
Contributor

@AlexKantor87 AlexKantor87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opus 4.6 review (compared to the Sonnet 4.6 review above)

Overall: This is a solid, well-scoped PR. The three buckets of work — Long descriptions/Example blocks for evaluate commands, expanded policy contract hint, and snyk example replacement — are all improvements. Clean commits, addresses #671 feedback fully.

Where I agree with Sonnet 4.6

  • --attestations format (tutorial line 143): Strongest point in the Sonnet review. The artifact-name.attestation-type format needs a brief inline explanation — this is the single most valuable change the PR could still make.
  • my-policy.rego rename (tutorial Step 6): Agree it's a mild discontinuity, though I'd frame it as a mode shift (worked example → reusable template) rather than just a filename change. A transitional sentence would help.

Where I disagree with Sonnet 4.6

  • Generic placeholders in --help examples (evaluateTrails.go): The suggestion to use main-branch-trail staging-branch-trail would bake in one mental model. CLI help examples should stay neutral — the tutorial is the right place for concrete use cases.

What I'd add beyond the Sonnet review

  • String concatenation pattern (evaluate.go): The backtick-escaped inline code convention works but is hard to read. A brief explanatory comment would help maintainability as this pattern spreads.
  • Policy contract hint expansion (tutorial): Best change in the PR. Distinguishing Kosli conventions from OPA built-ins, with type expectations, is exactly right.
  • Snyk example replacement (tutorial Step 4): Much better pedagogical choice — shows a genuinely different use case and data traversal pattern.
  • draft: true removal: Worth a final sanity check that the tutorial is ready for public consumption.

Bottom line: I'd approve with one suggested change (explain the --attestations format) and one optional improvement (transitional sentence before Step 6). See inline comments for details.

tooky added a commit that referenced this pull request Mar 9, 2026
…ceholder note, comment backtick pattern

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tooky and others added 5 commits March 9, 2026 13:25
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ual-command example with snyk trail example

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ceholder note, comment backtick pattern

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@tooky tooky force-pushed the evaluate-docs-updates branch from 70b9174 to 3ad33c0 Compare March 9, 2026 13:25
Copy link
Contributor

@AlexKantor87 AlexKantor87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All feedback addressed cleanly. The --attestations format explanation, the backtick convention comment, and the my-policy.rego transition sentence all land well. Good to merge.

@tooky tooky merged commit 425fa22 into main Mar 9, 2026
10 checks passed
@tooky tooky deleted the evaluate-docs-updates branch March 9, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants