refactor(contract): a wrapper prologue is data, not two hand-written lists - #502
Merged
Conversation
…lists #500 added a fourth parity comparison because a wrapper prologue could diverge. This removes the ability to diverge: neither backend writes one any more. modules/src/contract/wrappers.json declares each generated wrapper's environment once — the export, and the reason it exists — and both renderers render that. The contract mechanism is issue #451's, one level over. It already carries a UNIT's bindings as data that the module and bin/agentbox both read; a wrapper belongs to no unit, which is exactly why it fell through: the webhook receiver and the settings daemon both run agent-box-webhook-spawn, so what its payload gets is the block above the `exec`. Two entry kinds: bin names a PROGRAM, resolved by each backend the way it already resolves a unit contract's programs — contractPrograms here, self.bin/<program> natively. jq and agent-box-envstore join that table; nothing else was needed. config names a per-box VALUE by key. The two backends may legitimately resolve one to different text, which is the point of naming a key: a diagnostic that says where the fleet-wide default LIVES has to name a NixOS option on one backend and config.yaml on the other. omitWhenEmpty drops the line when the value is empty, so "nothing set" stays distinguishable from "set to nothing". The prose moves too. Every export carried a paragraph explaining itself, written once in Nix and once in Python — the same reasoning maintained twice, which is how they drift. It now rides on the manifest entry and is emitted into both wrappers. Two divergences died on the way, both invisible to the box-wide comparison because each name was supplied SOMEWHERE by both backends: - the module's agent-box-webhook wrapper never exported AGENT_BOX_HOOK_ARGS_OPTION_NAME, native's did. Now both do, and the WRAPPER_VARS_BY_DESIGN entry #500 added for it is deleted — a fixed gap must leave the table in the same change, and the staleness check enforces that. - native's agent-box-webhook wrapper exported AGENT_BOX_HOOK_SESSION_ARGS, which nothing reads: webhook-cli.sh does not use it and never invokes the spawn payload, it delegates to webhook.py. The module never had it there. Now neither does, and the apostrophe test asserts the CLI wrapper does NOT carry it rather than looping over both wrappers looking for it. Scope: the two webhook wrappers, which is where the divergence actually bit. The remaining hand-written prologues (session, profile, password, attach) are a follow-up; #500's comparisons stay as the backstop until they migrate, and the per-wrapper section is now empty of divergences rather than declaring one. NIX_BUILD_RC=0 over all 26 aarch64 checks, 85 native tests OK, PARITY_RC=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rt3Sp5XwGZy1Uzto8qP4is
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughWebhook wrappers now use a shared manifest to generate environment exports. The renderer resolves binaries and configuration values, shell-quotes values, omits empty optional values, and keeps native and Nix-generated wrappers aligned. ChangesWrapper contract generation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change centralizes webhook wrapper environment declarations across both backends, with the supplied builds, tests, and parity checks passing. No actionable merge-blocking risk remains. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NixModule
participant WrapperEnvRenderer
participant ProgramRegistry
participant BoxConfiguration
participant WebhookWrapper
NixModule->>WrapperEnvRenderer: render wrapper environment
WrapperEnvRenderer->>ProgramRegistry: resolve pinned binaries
WrapperEnvRenderer->>BoxConfiguration: resolve configured values
WrapperEnvRenderer-->>WebhookWrapper: emit shell export prologue
WebhookWrapper-->>WebhookWrapper: run with generated environment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (8 skipped: 8 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
1 participant
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.
#500 added a fourth parity comparison because a wrapper prologue could diverge. This removes the ability to diverge: neither backend writes one any more.
modules/src/contract/wrappers.jsondeclares each generated wrapper's environment once — the export, and the reason it exists — and both renderers render that.Why a wrapper, specifically
The contract mechanism is issue #451's, one level over. It already carries a unit's bindings as data both
modules/agent-box.nix.inandbin/agentboxread. A wrapper belongs to no unit, which is exactly why it fell through: the webhook receiver and the settings daemon both runagent-box-webhook-spawn, so what its payload gets is the block above theexec, not any unit'sEnvironment=.The schema
bincontractProgramshere,self.bin/<program>natively — the same resolution a unit contract already usesconfigconfignames a key rather than a value because the two backends may legitimately resolve one to different text: a diagnostic that says where the fleet-wide default LIVES has to name a NixOS option on one andconfig.yamlon the other.omitWhenEmptydrops the line when the value is empty, so "nothing set" stays distinguishable from "set to nothing".jqandagent-box-envstorejoincontractPrograms; nothing else was needed.The prose moves too
Every export carried a paragraph explaining itself — written once in Nix and once in Python. That is the same reasoning maintained in two places, which is how they drift in the first place. It now rides on the manifest entry and is emitted into both backends' wrappers.
Two divergences died on the way
Both were invisible to the box-wide comparison, because each name was supplied somewhere by both backends:
agent-box-webhookwrapper never exportedAGENT_BOX_HOOK_ARGS_OPTION_NAME; native's did. Now both do — and theWRAPPER_VARS_BY_DESIGNentry fix(native): the spawn wrapper never had the env its payload demands #500 added for it is deleted. A fixed gap must leave the table in the same change, and the staleness check enforces exactly that.agent-box-webhookwrapper exportedAGENT_BOX_HOOK_SESSION_ARGS, which nothing reads:webhook-cli.shdoes not use it and never invokes the spawn payload — it delegates towebhook.py. The module never had it there. Now neither does, and the apostrophe test asserts the CLI wrapper does not carry it rather than looping over both wrappers looking for it.The per-wrapper parity section is now empty of divergences rather than declaring one, which is the result to look for: the check is still there, but it has nothing left to report on these two.
Scope
The two webhook wrappers, which is where the divergence actually bit. The remaining hand-written prologues (
session,profile,password,attach) are a follow-up; #500's comparisons stay as the backstop until they migrate.Verification
NIX_BUILD_RC=0over all 26 aarch64 checks, 85 native tests OK,PARITY_RC=0.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rt3Sp5XwGZy1Uzto8qP4is