Skip to content

fix(tfpolicy-author): Allow core::getdatasource() inside resource_policy - #92

Open
jweigand wants to merge 2 commits into
hashicorp:mainfrom
jweigand:tfpolicy-getdatasource
Open

fix(tfpolicy-author): Allow core::getdatasource() inside resource_policy#92
jweigand wants to merge 2 commits into
hashicorp:mainfrom
jweigand:tfpolicy-getdatasource

Conversation

@jweigand

Copy link
Copy Markdown
Contributor

The tfpolicy-author skill unconditionally instructed agents to avoid core::getdatasource() inside resource_policy blocks ("it calls provider APIs"). It blocked legitimate, necessary uses: validations where the datasource filter is derived from the current resource's own attributes (e.g., confirming attrs.ami is owned by an approved account, or attrs.kms_key_id has a required key policy) genuinely can't be pre-computed in a top-level locals block, since attrs is only available inside resource_policy.

This mirrors the existing, more nuanced guidance the skill already has for core::getresources(): cache at the top level when the filter is a static/literal value, but call inline within resource_policy when the filter depends on the resource being evaluated

Changes:

  • references/tfpolicy-author.md: replaced the blanket "avoid" rule (Best Practices item 9) with the literal-vs-attrs.*-dependent decision guide, and added item 17 with a worked example (AMI-owner validation) showing the correct inline pattern, scoped with filter and wrapped in core::try().
  • references/verified-syntax.md: reframed "Mistake 14" to specifically call out static-filter misuse, and added an "✅ Exception" section demonstrating the legitimate inline case, cross-referencing the equivalent core::getresources() exception.

Why: Without this, the skill refuses to write policies that need live, resource-specific provider validation which is a valid use case per the getdatasource() docs.

…icy when filter depends on attrs.*

The skill previously banned core::getdatasource() inside resource_policy
outright, which blocked legitimate per-resource external validations
(e.g. verifying an AMI's owner, a KMS key's policy) whose filter is
necessarily derived from the resource's own attrs.* and can't be
hoisted to a top-level locals cache. Replaces the blanket rule with the
same literal-vs-attrs.*-dependent decision guide already used for
core::getresources(), plus a worked example and updated "Mistake 14"
writeup in verified-syntax.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jweigand
jweigand requested a review from a team as a code owner July 31, 2026 00:50

@vpaul97 vpaul97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@viveksinghgit95

Copy link
Copy Markdown

Direction makes sense — the blanket ban did block legitimate per-resource checks, and mirroring the getresources() literal-vs-attrs.* rule keeps it to one mental model. A few suggestions before merge:

  1. Keep a code example. The deleted Mistake 14 block was the only place in the skill showing core::getdatasource() syntax; after this PR every mention is prose. Since agents pattern-match on code blocks, it would help to keep a small wrong/right pair for the static-filter case and add the AMI-owner validation as actual HCL under item 17 (the PR description mentions a worked example, but it's currently prose only). An example would also document the return shape (single object vs list, what a no-match looks like).

  2. Say what the core::try() fallback should be. Wrapping in core::try() prevents a crash, but if the fallback value makes the condition pass, a failed API call silently skips the check — the worst failure mode for a compliance policy. One sentence like "choose a fallback that causes the condition to fail (deny), not pass" would close that gap, and ties into existing best-practice item 14 about not masking values with try() defaults.

  3. Plan-time unknowns. The inline getresources() pattern mandates the "will not resolve during plan time" comment. Does the same caveat apply when the getdatasource() filter uses an attribute that's unknown at plan time (e.g. attrs.kms_key_id referencing a not-yet-created key)? Worth stating either way.

Nit: item 9 says wrap the result in core::try(), item 17 says wrap the call — worth picking one phrasing since agents read these files as spec.

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.

3 participants