Repository navigation
docs(consumer): ship an operable caller workflow and onboarding guide - #83
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
PR Summary by Qododocs(consumer): Add dispatchable caller template and ordered onboarding guide
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by QodoContext used✅ Compliance rules (platform):
15 rules 1.
|
PR approved by QodoAll merge criteria satisfied — approved by default policy |
Qodo FixerNo findings are available for this PR yet. Findings appear here once Qodo has reviewed the PR. |
b7a9b37 to
8358b25
Compare
|
Code review by qodo was updated up to the latest commit 8358b25 |
A supervised consumer pilot could not obtain signed evidence, and every blocker was an undocumented prerequisite rather than a code defect. ThreadLoop documented only a caller workflow with hardcoded inputs. No operator can use that shape, because session id, proof-plan digest, and pull request number are known only at dispatch time. So both pilot attempts wrote their own caller, and both carried the same defect: `workflow_dispatch` delivers every input as a string, including one declared `type: number`, and passing that string into the review sensor's numeric input fails `workflow_call` validation before any job is created. The failure presents as a job that never appears, with no step and no log. Add `examples/threadloop-caller-workflow.yml` as a complete dispatch-driven caller for both sensors, casting the numeric input with `fromJSON`. Add `docs/consumer-onboarding.md` for the ordered prerequisites the pilot surfaced: - the caller workflow must be on the consumer's default branch before the first governed session, because GitHub resolves `workflow_dispatch` targets from the default branch only, so a consumer cannot produce signed evidence for its first governed feature branch; - the caller must be correct before starting, because it lives in the governed repository and editing it moves HEAD, which stales every HEAD-bound receipt and recorded pre-PR review, so an in-flight session cannot absorb the fix; - a declared gate must provision its own toolchain, because the sensor runs exactly the declared command with only Node set up while a typical verify target assumes CI already provisioned the environment; - CLI location is environment rather than a fifth wake input; and - `session next` outranks any planned step order. Also record that `init` has no `--json` by design: it is an operator action, and the runner contract forbids a wake from invoking it. `tests/unit/caller-template.test.ts` cross-checks the template against the sensors' own declared `workflow_call` inputs, so drift fails locally instead of as an invisible missing job. Mutation-tested: removing the `fromJSON` cast and dropping a required input each fail a distinct assertion. Refs #79, #80
Qodo review, Action required, Bug/Correctness: the template used `OWNER/threadloop` while the template header and onboarding guide both told consumers to replace only `@FULL_COMMIT_SHA`. Copying it as directed left the owner placeholder intact and dispatch failed against a repository that does not exist. The attestation docs already used the real slug, so the template was the only thing out of step -- the same "documented but unusable" defect this PR set out to remove. Use the real slug in both `uses:` pins, so "replace only the SHA" is true. The slug names the repository hosting the sensors, not the consumer's own, so consumers leave it alone. Say that in the template and the onboarding guide, along with what to change when consuming a fork. Declare `repository` in package.json, which was missing, and derive the expected slug from it in the test rather than hardcoding an owner. Hardcoding would bake this repository's ownership into an assertion, so a fork would ship a template pointing at upstream with a test enforcing it. Deriving gives the slug one source of truth: a fork updates `repository` and the test then requires the template to match. Verified by mutation: the original `OWNER` placeholder fails, a fork that updates package.json but forgets the template fails, and a fork that updates both passes.
8358b25 to
686ebdd
Compare
Qodo Fix Summary — Round 1Fixed (1)1. Unreplaced repo placeholder 🐞 Bug · ≡ Correctness — The template used an owner placeholder while the template header and onboarding guide both said to replace only Applied Qodo's option 1: use the real slug in both Two additions beyond the finding, from reviewer feedback on the first attempt at this fix:
Verified by mutation, three ways: the original placeholder fails; a fork that updates Validation
Rebased onto current Generated by Qodo PR Resolver skill |
|
Code review by qodo was updated up to the latest commit 686ebdd |
Summary
A supervised consumer pilot could not obtain signed evidence, and every blocker turned out to be an undocumented prerequisite rather than a code defect. This ships the missing template and the ordered prerequisites.
ThreadLoop documented only a caller workflow with hardcoded inputs. No operator can use that shape — session id, proof-plan digest, and pull-request number are known only at dispatch time. Both pilot attempts therefore wrote their own caller, and both carried the same defect.
Related issue
Refs #79, #80
Does not close them: #79 also asks that the review path be exercised end to end against a live sensor, which is still blocked, and #80 carries the
init --jsonversioning decision (resolved here as documentation, see below).Changes
examples/threadloop-caller-workflow.yml— complete dispatch-driven caller for both sensors, withpull_request_number: ${{ fromJSON(inputs.pull_request_number) }}.docs/consumer-onboarding.md— ordered prerequisites: default-branch requirement, get-the-caller-right-first, self-provisioning gates, CLI location, projection authority, and theinitrationale.docs/attestations/receipt-v1.md,docs/attestations/review-v1.md— point at the operable template rather than leaving the literal snippet as the only guidance.tests/unit/caller-template.test.ts— 5 tests.README.md— docs index entry.Impact
No
src/change. The new test covers an example file and the existing sensor workflows.Validation
npm run checkfromJSONcast fails "casts the numeric review input"; dropping a required input fails "passes exactly the inputs each sensor declares". Both assertions are load-bearing.Why the test cross-checks the sensors
tests/unit/caller-template.test.tsreads the sensors' ownworkflow_call.inputsand asserts the template passes exactly the required ones. That guards the specific failure mode that cost the pilot the most time: an input mismatch failsworkflow_callvalidation before any job is created, so it surfaces as a job that never appears, with no step and no log to read. This turns a silent, near-undiagnosable CI failure into a local test failure.The
init --jsondecision#80 asked whether
initshould accept--json. Resolved here as documentation rather than code. Adding the flag changes the published protocol contract forinit, andprotocol: 4is pinned in six places including the runner skill, which requires it exactly. Bumping to 5 across the skill, tests, docs, and every pinned consumer is disproportionate for a cosmetic gain, especially since the runner is already forbidden from invokinginit. If you would rather have the flag and the version bump, say so and I will do it separately.Risk and recovery
Low risk, documentation only. The one judgement worth checking is whether the guidance in section 3 is the behaviour you want to commit to, since it tells adopters to restructure their verify target. #78 tracks whether ThreadLoop should instead let the proof plan declare setup steps — if that lands, section 3 needs rewriting.
Reviewer guidance
examples/threadloop-caller-workflow.yml— whether the permissions split is right:id-token: writeon both sensor calls,pull-requests: readonly on review.docs/consumer-onboarding.mdsection 2 — the claim that an in-flight session cannot absorb a caller-workflow fix. That follows from HEAD-bound evidence, and it is the most consequential statement in the document.examples/is the right home for a YAML template, given it currently holds only Markdown artifact examples.