Variable contracts: see the value that crosses an activity boundary undeclared - #502
Merged
Conversation
The derived contract narrowed both reads and writes to the workflow's declared namespace, and that namespace is assembled from the declarations being checked. A variable nothing declares was therefore invisible on both sides at once: its production dropped out of the derived writes, its consultation dropped out of the derived reads, and the pair of silences was indistinguishable from a name no activity uses. The contract now carries what a step produces and what a step consults before that narrowing, and the guard reports a name produced in one activity and consulted in another that no contract in the workflow declares. Only that shape: a production nothing else consults is a utility operation's confirmation value and dies with its step, and one whose technique declares an artifact has a file for a destination.
The guard on this branch reports a value crossing an activity boundary with no contract; the corpus at this pointer is the one it leaves clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An activity declares the session variables it reads and the variables it writes, and a guard holds
the corpus to those declarations. It could not see a variable that no declaration mentioned at all.
The guard derives what an activity reads and writes, then narrows both sets to the workflow's
declared namespace — and that namespace is assembled from the very declarations being checked. So a
name nobody declared dropped out of both sides at once: its production vanished from the derived
writes, its consultation vanished from the derived reads, and the pair of silences was
indistinguishable from a name no activity uses. The check could police a name someone had already
declared and nothing else.
Stacked on #501 so the two can be reviewed apart and land together. Base is
feat/496-activity-exits; the diff here is the guard and the contracts, not the routing.How it surfaced
A layer-3a smoke run of
work-package— a real worker, two activities — failed its session-contractassertion: the run wrote
prior_feedback_triageandrating_capwhile exitingstart-work-package,which declared neither. The corpus guard was green at the time.
start-work-packagebindsreview-existing-feedback, whose declared outputs are those two names.submit-for-reviewbindsreview-summary, whose prose interpolates both. Nothing declared either.So the producing side and the consuming side were both invisible, for the same circular reason, and
the run was the only thing that could see it.
The change
The derived contract now carries two sets before the narrowing: what a step produces (a bound
operation's declared output, a remap target, a checkpoint's
setVariablekey, asettarget, aloop's item variable) and what a step mentions. A new finding family reads them:
undeclared-crossing— a name produced in one activity, consulted in another, and declared by nocontract in the workflow. The value crosses an activity boundary with nothing accounting for it at
either end.
Only that shape. Two wider readings were tried and rejected, because the namespace filter turns out
to be doing real work:
body,title,file_countare consumed by a later step and never become session state.written_artifactat 15 sites — which legitimately dies with its step.A production nothing else consults is not reported, and neither is one whose technique declares an
#### artifact: its destination is a file. The exemption means a declaration is not requiredthere, not that it is wrong —
prior_feedback_triageis an artifact output that a worker alsorelays into the bag, so it is declared, and the two checks now agree about it.
What the corpus owed
79 findings, 55 values, 7 workflows. All of them are now declared: the write on every producing
activity, carrying the description the producing technique already gives the output, and the read on
every activity the guard finds consulting it.
Three things came out of the fixing that are worth stating.
A read can be reached before its producer runs. 54 of the declarations landed on activities that
consult a value earlier in the order than the activity producing it, or on a loop-back. Those writes
carry a default, so the reader finds the empty value rather than nothing.
requirements(29 sites),changed_files(10) andreview_summary(5) account for most of them.A borrowed activity is bound by the workflow running it.
remediate-vulnopens on its ownstartrather thanwork-package'sstart-work-package, so the two review-mode values that openerproduces have no producer there. Declaring the write upstream left the borrowing workflow with a
reader and no writer — the #491 borrowed-scope class, reported exactly as designed.
remediate-vulnnow owns both as the empty constants they are in a workflow that never reviews a pull request.
A description does not travel with its links. 13 of the descriptions copied from technique files
carried relative markdown links that resolve from
techniques/and not fromactivities/; one brokecheck:resource-anchors. The declarations keep the words and drop the links.Verification
npm run check:all— 30 guards, all pass. The new family is at hard zero, like the other five.npm run test:ci— 1,097 pass.Re-running
check-session-contractover the savedsmoke-20260824083443/session.json:23 write(s) measured … OK — every write the run recorded is one the activity that made it declares.That is the original defect confirmed fixed on the original evidence, without a second paid run.
Non-goals
The guard reports a crossing, not a design. Where a value ought not to cross an activity boundary at
all, this declares the crossing that exists rather than redesigning the flow.
Two near-duplicate names in
substrate-node-security-audit—dispatch_resultsanddispatched_results— are both declared here because both are produced and consulted. That theycoexist is a smell worth its own look; it is not this change's business.
🤖 Generated with Claude Code