Skip to content

chore(skills): add deps triage skill - #9

Merged
mortik merged 6 commits into
mainfrom
chore/deps-triage-skill
Aug 17, 2026
Merged

chore(skills): add deps triage skill#9
mortik merged 6 commits into
mainfrom
chore/deps-triage-skill

Conversation

@mortik

@mortik mortik commented Aug 14, 2026

Copy link
Copy Markdown
Member

Adds a /deps skill that works through the open Dependabot queue. Companion to the same skill in the other repos, but the gating here is different in kind, not just in detail.

Merging in this repo applies infrastructure. A merge to main runs Main, and a successful Main triggers Deploy, whose stage job runs terraform initterraform plan -out=tfplanterraform apply tfplan. The Stage environment has no protection rules, so that apply is unattended — a merged provider bump changes real infrastructure within minutes. (Live is safe: live-plan and live-apply only run on workflow_dispatch with target: live.)

So a green check is not sufficient evidence. CI runs terraform init -backend=false, validate and test; it never plans against real state. A provider bump that changes a default, deprecates an attribute, or alters how an existing resource is read passes validate and then shows up as a change — or a replacement — in the Stage apply.

The skill therefore makes the plan the primary gate rather than CI: check out the PR, terraform workspace select stage, terraform plan, and merge only on No changes. Any must be replaced / -/+ stops the merge and gets reported with the resource addresses. If credentials for the S3 backend or the 1Password provider are unavailable, the PR is reported as unverified rather than merged on CI alone, and the report must say whether a plan actually ran.

Other repo-specific notes it encodes:

  • Providers in use are hetznercloud/hcloud, hashicorp/aws, hashicorp/cloudinit and 1Password/onepassword — the last is why local plans need OP_SERVICE_ACCOUNT_TOKEN.
  • Constraints in versions.tf are floors (>= 1.60), so bumps normally touch only .terraform.lock.hcl. A versions.tf change is worth reading.
  • Read the version from the diff, not the title — Dependabot rewrites the branch as new releases land while the title lags behind.
  • Every open PR edits .terraform.lock.hcl, so each merge conflicts the rest — merge one at a time, watch the Stage apply, then @dependabot recreate the remainder.
  • main is protected by a ruleset (terraform_test + merge queue); the classic protection API returns 404, which reads as "unprotected" if you do not know to check /rulesets.

The queue is empty right now, so this lands before it is needed rather than during a backlog.

Docs only — no Terraform changes. 🤖

Walks the open Dependabot queue: classifies each bump, gates on CI and on
intentional pins, merges the safe ones and reports what needs a decision.
Tailored to this repo's ecosystems, protection rules and merge behaviour.
@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds a dependency-triage skill that classifies Dependabot updates, requires CI and a no-op Stage plan, binds merges to the planned commit, and waits for the resulting deployment. The plan procedure still needs to ensure that local Terraform inputs cannot differ from the printed commit.

  • Adds provider-update classification and repository-specific safety gates.
  • Requires commit-bound no-op plans before merge.
  • Serializes merges and waits for commit-specific Main and Deploy completion.
  • Defines reporting and failure-handling procedures.

Confidence Score: 4/5

The PR is not yet safe to merge because Gate C can authorize a dependency update using a plan influenced by local Terraform files outside the recorded commit.

The branch and workspace checks bind the plan to the intended PR context, but no clean-worktree check prevents surviving local Terraform inputs from changing the plan relative to the commit later merged and applied.

Files Needing Attention: .claude/skills/deps/SKILL.md

Important Files Changed

Filename Overview
.claude/skills/deps/SKILL.md Adds the complete dependency-triage workflow, but its plan gate does not prove that the planned working tree matches the recorded commit.

Fix all with Greploop

Fix All in Claude Code

Reviews (7): Last reviewed commit: "chore(skills): pass the planned SHA lite..." | Re-trigger Greptile

Comment thread .claude/skills/deps/SKILL.md Outdated
Comment thread .claude/skills/deps/SKILL.md Outdated
…ps triage

Gate C allowed patch bumps and in-place plan changes through without a
confirmed no-op plan, and step 4's 'watch the Stage apply' was a one-shot
run listing that neither identified the run for the merged commit nor
waited on it. Both let an unreviewed change reach the unattended Stage
apply, and let the next merge start before the previous one landed.
@mortik

mortik commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Both findings were valid — thanks. Fixed in db41ddd.

Gate C permits unchecked changes — the gate said "before merging anything beyond a trivial patch", which contradicted the file's own premise that terraform_test never runs a plan. A patch release is precisely where a silently changed provider default shows up. The plan is now mandatory for every bump, and the pass condition is narrowed to No changes. only: any +, ~, -, or -/+ fails the gate and can only be cleared by the user explicitly approving that PR. The old "new optional attributes defaulting in → usually safe" escape hatch is gone, since a re-read attribute and a destructive rewrite are indistinguishable at that level of summary.

Deployment monitoring does not wait — replaced the one-shot gh run list --limit 3 with a blocking wait across all three hops: poll until the merge queue closes the PR, capture mergeCommit.oid, then gh run watch --exit-status on the Main and Deploy runs filtered to that SHA (-c $SHA). Deploy is workflow_run-triggered so it doesn't exist until Main finishes — the loop polls for it rather than assuming. A non-zero --exit-status halts the whole triage instead of merging the next PR, and a queue ejection (CLOSED rather than MERGED) is reported instead of waiting on a deploy that will never start.

The report template now distinguishes "merged, Stage apply succeeded" from merely enqueued. 🤖

Comment thread .claude/skills/deps/SKILL.md Outdated
`|| break` exited only the per-workflow loop and discarded the non-zero
status, so a failed Stage apply left no signal and the triage could go on
to merge the next bump. Fail loudly with the run log and a non-zero exit.
@mortik

mortik commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Valid again — fixed in 5dee706.

|| break exited only the for wf in Main Deploy loop and threw away the non-zero status, so a failed Stage apply left no signal at all for the caller. Worse, breaking on a failed Main also skipped the Deploy check entirely, which reads as "nothing left to wait for" rather than "this halted early".

Now the failure branch prints a HALT: line, dumps --log-failed, and exit 1s, and the success path ends with an explicit OK: Stage apply succeeded marker. The prose states that a non-zero exit ends the whole triage rather than just the wait, and warns against reading a failed Main as proof that nothing was applied. Absent the OK: line the bump counts as unverified and the next merge stays blocked. 🤖

Comment thread .claude/skills/deps/SKILL.md
The Gate C snippet ran four unchained commands, so a failed checkout or
workspace select fell through to a plan against the previous branch or
workspace — a false `No changes.` that reads as a passing gate. Abort on
setup failure and assert branch and workspace before planning.
@mortik

mortik commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Valid, and the sharpest of the four — fixed in cf5ff8c.

The Gate C snippet was four unchained commands, so a failed gh pr checkout (dirty tree) or a failed terraform workspace select stage fell straight through to terraform plan. That plan then describes the previously active branch and workspace, and a No changes. from the wrong workspace is indistinguishable from a genuinely clean bump — the exact false pass this gate exists to prevent. If the leftover workspace happened to be live, the gate would be reading production while claiming to have checked Stage.

The snippet now opens with set -euo pipefail so a failed init or workspace select aborts instead of falling through, and asserts both invariants before planning: git rev-parse --abbrev-ref HEAD against the PR's headRefName, and terraform workspace show equal to stage. Either assertion failing prints HALT: and exits non-zero, and the prose classifies that outcome as unverified rather than clean. Also noted that gh pr checkout fails on a dirty tree so it gets committed or stashed rather than worked around. 🤖

Comment thread .claude/skills/deps/SKILL.md
A clean Gate C plan is evidence about one commit, but the merge targeted
whatever the PR head was by then. Dependabot force-pushes the branch when a
newer release lands, so the plan and the merged code could differ. Record
the planned SHA and pass it as --match-head-commit.
Comment thread .claude/skills/deps/SKILL.md
@mortik

mortik commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Valid — fixed in 765457c.

A clean plan is evidence about one commit, not about the PR, and nothing tied the two together. Dependabot force-pushes these branches whenever a newer release lands — the same behavior step 2 already warns about for stale titles — so the head could move between Gate C and gh pr merge, and the Stage apply would then run a provider revision that was never planned.

Gate C now records planned_sha=$(git rev-parse HEAD) immediately before the plan, and step 4 merges with --match-head-commit "$planned_sha", so a moved head fails the merge instead of silently enqueueing an unplanned commit. On rejection the instruction is to re-run Gate C against the new head and merge only if that plan is also clean, with an explicit warning never to drop the flag to get past the rejection.

One structural note while this is open: every finding in this review has been a variant of "an unplanned change reaches the unattended Stage apply", and prose in a skill file can't enforce what a required check can. The durable fixes are on the repo side — a protection rule on the Stage environment, or a workflow that runs plan against real state on Dependabot PRs and posts the diff. Worth doing separately from this PR. 🤖

Each command runs in a fresh shell, so the $planned_sha set in Gate C
expanded to empty at merge time, and --match-head-commit "" disables the
guard silently. Print a PLANNED SHA to copy and paste literally instead.
Comment on lines +116 to +118

#### Gate D — mergeable state

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Plan includes uncommitted inputs

When an ignored untracked Terraform override or another surviving local Terraform change is present, the branch and workspace assertions still pass and terraform plan evaluates content not represented by PLANNED SHA, causing a clean result to authorize a provider update whose committed configuration can produce changes during the unattended Stage apply.

Fix in Claude Code

@mortik
mortik added this pull request to the merge queue Aug 17, 2026
Merged via the queue into main with commit ee7d512 Aug 17, 2026
3 checks passed
@mortik

mortik commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Fixed in a03fe06 — and this one was my bug, not a reviewer artifact.

--match-head-commit "$planned_sha" could never have worked: each command runs in a fresh shell, so the planned_sha set during Gate C expands to empty at merge time, and --match-head-commit "" disables the guard without erroring. Gate C now prints PLANNED SHA: <oid> and step 4 shows the SHA pasted literally on the command line, with a note that losing the SHA means you no longer have a valid plan and must re-run Gate C.

Separately, merging this PR turned up a repo fact the skill gets wrong: gh pr merge --squash fails here with Auto merge is not allowed for this repository. The "Main branch protection" ruleset requires a merge queue, and gh pr merge enqueues through the auto-merge mutation, which is disabled at repo level (allow_auto_merge: false). The command that actually works is the queue-specific GraphQL mutation, which takes an expectedHeadOid and so keeps the commit binding:

gh api graphql -f query='mutation($pr:ID!,$oid:GitObjectID!){enqueuePullRequest(input:{pullRequestId:$pr, expectedHeadOid:$oid}){mergeQueueEntry{position state}}}' \
  -f pr="$(gh api repos/reckoning/infrastructure/pulls/<number> --jq .node_id)" -f oid=<PLANNED SHA>

That correction needs its own PR since this branch is already enqueued. Follow-ups I'm opening: the merge-command fix, and the enforcement side — a protection rule on the Stage environment or a workflow that plans against real state on Dependabot PRs, since none of the guidance in this file is enforced by anything. 🤖

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.

1 participant