Repository navigation
feat(iac): render portable GraphForge deployment specs - #251
Merged
Merged
Conversation
WalkthroughThe PR adds a provider-neutral ChangesDeployment specification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Preview
participant DeploymentRenderer
participant PulumiDeploymentSpec
participant PreviewVerifier
Preview->>DeploymentRenderer: Load resolved configuration and artifact locator
DeploymentRenderer->>DeploymentRenderer: Validate target and immutable locator
DeploymentRenderer-->>PulumiDeploymentSpec: Render deployment specification and canonical JSON
PulumiDeploymentSpec->>Preview: Register state-only outputs
Preview->>PreviewVerifier: Export deployment specification
PreviewVerifier-->>PreviewVerifier: Verify allowed resource type and golden marker
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
iac/pulumi/python/src/graphforge_pulumi/deployment.py (3)
107-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCopy the ID lists into the projection.
The projection embeds the caller's list objects by reference. The TypeScript renderer copies them (
index.tslines 649-650). If a caller mutatesresolved_configafter rendering, the Python spec and the registered Pulumi output change too.♻️ Proposed refactor
"bindings": { - "secret_ids": target["secret_ids"], - "source_ids": target["source_ids"], + "secret_ids": list(target["secret_ids"]), + "source_ids": list(target["source_ids"]), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/python/src/graphforge_pulumi/deployment.py` around lines 107 - 110, Update the "bindings" projection in the deployment rendering flow to copy target["secret_ids"] and target["source_ids"] into new list objects rather than retaining caller-owned references. Preserve the existing values and output structure while ensuring later mutations to resolved_config cannot alter the generated spec or registered Pulumi output.
160-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare one canonical serializer.
Lines 161-169 repeat the serialization options of
render_deployment_spec_json. Two copies can drift, and drift breaks byte equivalence. Extract one private helper and call it from both places.♻️ Proposed refactor
+def _canonical_text(spec: JsonObject) -> str: + return ( + json.dumps(spec, ensure_ascii=False, separators=(",", ":"), sort_keys=True) + "\n" + ) + + def render_deployment_spec_json( resolved_config: Mapping[str, Any], target_id: str, artifact_locator: str, ) -> str: """Return canonical UTF-8 JSON text with a single trailing newline.""" - spec = render_deployment_spec(resolved_config, target_id, artifact_locator) - return ( - json.dumps( - spec, - ensure_ascii=False, - separators=(",", ":"), - sort_keys=True, - ) - + "\n" - ) + return _canonical_text(render_deployment_spec(resolved_config, target_id, artifact_locator))spec = render_deployment_spec(resolved_config, target_id, artifact_locator) - canonical_json = ( - json.dumps( - spec, - ensure_ascii=False, - separators=(",", ":"), - sort_keys=True, - ) - + "\n" - ) + canonical_json = _canonical_text(spec)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/python/src/graphforge_pulumi/deployment.py` around lines 160 - 169, Extract the shared canonical JSON serialization logic from render_deployment_spec and render_deployment_spec_json into one private helper, preserving ensure_ascii=False, compact separators, sorted keys, and the trailing newline. Update both callers to use that helper so their serialized output remains byte-equivalent.
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
_failasNoReturn.
_failalways raises, but the annotation says-> None. Type checkers therefore assume execution continues after each_failcall. Two places then look unsafe: line 46 usesmatch.group()after thematch is Nonebranch, and line 58 readsparsed, which is bound only inside thetryblock.NoReturnremoves both false positives and keeps runtime behavior identical.♻️ Proposed refactor
-from typing import Any +from typing import Any, NoReturn-def _fail(message: str) -> None: +def _fail(message: str) -> NoReturn: raise ValueError(f"invalid graphforge-deployment-spec/1: {message}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/python/src/graphforge_pulumi/deployment.py` around lines 24 - 25, The _fail function is annotated as -> None but always raises an exception, which causes type checkers to incorrectly assume execution continues after calls to _fail. Update the return type annotation of _fail from -> None to -> NoReturn (imported from the typing module) to correctly signal that the function never returns normally. This will eliminate false positives at line 46 and line 58 where type checkers incorrectly flag unreachable code.iac/pulumi/typescript/src/index.ts (1)
535-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract one deployment-spec failure helper.
The prefix
"invalid graphforge-deployment-spec/1: "is repeated in every throw site of this function. One helper removes the duplication and keeps the prefix stable, which matters because the tests assert on these messages.♻️ Proposed refactor
+function failDeployment(message: string): never { + throw new Error(`invalid graphforge-deployment-spec/1: ${message}`); +} + function validateArtifactLocator(value: unknown, kind: string, expectedSha256: string): string { if (typeof value !== "string" || value.length < 1 || value.length > 2048) { - throw new Error( - "invalid graphforge-deployment-spec/1: artifactLocator must be a bounded string", - ); + failDeployment("artifactLocator must be a bounded string"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/typescript/src/index.ts` around lines 535 - 559, In validateArtifactLocator, extract a local failure helper that prepends the stable "invalid graphforge-deployment-spec/1: " prefix to messages, then route each existing throw through it while preserving the current message text and validation behavior.iac/pulumi/typescript/test/deployment-spec.test.ts (1)
36-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSort canonical keys by code unit, not by locale.
canonicalorders keys withlocaleCompare. The renderer's canonical JSON sorts by code unit. The two orders agree for the lowercase snake_case keys in this fixture, but they diverge for keys that differ in case or that contain_next to letters. A future contract field could make this helper disagree with the renderer and produce a misleading failure.♻️ Proposed comparator change
return `{${Object.entries(value) - .sort(([left], [right]) => left.localeCompare(right)) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) .map(([key, item]) => `${JSON.stringify(key)}:${canonical(item)}`) .join(",")}}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/typescript/test/deployment-spec.test.ts` around lines 36 - 43, Update the key comparator in canonical to use JavaScript’s code-unit ordering rather than localeCompare, while preserving the existing recursive canonicalization and output formatting.tests/unit/test_graphforge_config_contract.py (1)
129-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection case and the capability status assertion for the deployment example.
The test proves the deployment example validates, but it never proves the deployment schema rejects anything. Lines 85-90 already apply that pattern to the config and resolved schemas. The deployment block also omits the
capability_compatibilitystatus assertion that line 135 makes forinfra.♻️ Proposed additions
assert deployment["readiness"] == {"status": "not_checked"} assert deployment["artifact"]["sha256"] in deployment["artifact"]["locator"] + assert deployment["capability_compatibility"]["status"] == "requirements_declared" + invalid_deployment = deepcopy(deployment) + invalid_deployment["infrastructure"]["status"] = "graphforge_owned" + assert not Draft202012Validator(deployment_schema, registry=registry).is_valid( + invalid_deployment + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_graphforge_config_contract.py` around lines 129 - 134, Add a deployment-schema rejection assertion following the existing config and resolved-schema pattern from lines 85-90, using the deployment example and an invalid mutation to verify validation fails. In the deployment contract assertions, add the expected capability_compatibility status assertion alongside infrastructure, connectivity, readiness, and artifact checks.iac/terraform/tests/deployment-spec-plan.sh (2)
109-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe Terraform negative suite cannot attribute a failure to a specific precondition. The loop treats any non-zero
planexit as success, and the fixture set covers five of the nine preconditions. Together these let a configuration error or a removed precondition keep the suite green.
iac/terraform/tests/deployment-spec-plan.sh#L109-L118: read an expected diagnostic per case and match it against the captured log before accepting the failure.iac/terraform/tests/deployment-spec-invalid.json#L2-L43: add anexpectfield to every case, and add cases for thecontractcheck, the single-match check,artifact_safe, andstable_source_ids && stable_secret_ids.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/terraform/tests/deployment-spec-plan.sh` around lines 109 - 118, The Terraform negative suite must verify each case fails for its intended precondition rather than accepting any non-zero plan. In iac/terraform/tests/deployment-spec-plan.sh lines 109-118, read each case’s expected diagnostic and require it to match the captured Terraform log before accepting the failure. In iac/terraform/tests/deployment-spec-invalid.json lines 2-43, add an expect field to every existing case and add coverage for the contract check, single-match check, artifact_safe, and stable_source_ids && stable_secret_ids preconditions.
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant byte-length constant and normalize the key-order dependency.
The assertion on line 43 duplicates the comparison on line 42: the byte length 1404 is already implied by the JSON equality check. Terraform
jsonencodesorts object keys lexicographically; jqtojsonpreserves the key order from the fixture file. The test passes only because the fixture is stored with sorted keys at every level. Document this coupling or normalize both sides to prevent the test from breaking if the fixture is reformatted. Removing the constant reduces maintenance burden.♻️ Proposed fix
"$JQ" -e --slurpfile expected \ "$terraform_root/../../docs/contracts/examples/graphforge-deployment-spec-production-v1.json" ' - (.planned_values.outputs.deployment_spec_json.value == (($expected[0] | tojson) + "\n")) and - ((.planned_values.outputs.deployment_spec_json.value | utf8bytelength) == 1404) + (.planned_values.outputs.deployment_spec_json.value + == (($expected[0] | to_entries | sort_by(.key) | from_entries | tojson) + "\n")) ' "$temporary_directory/plan.json" >/dev/null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/terraform/tests/deployment-spec-plan.sh` around lines 40 - 44, Update the deployment spec assertion in the jq expression to remove the redundant utf8bytelength comparison and its hard-coded 1404 constant. Normalize the Terraform output and expected fixture JSON before comparing, or explicitly document the existing key-order coupling if normalization is not supported, so fixture reformatting does not break the test.iac/pulumi/python/tests/test_deployment.py (1)
173-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the bounded locator error message.
Use
match="graphforge-deployment-spec/1"withpytest.raisesfor all non-OCI locator cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/pulumi/python/tests/test_deployment.py` around lines 173 - 185, Update test_non_oci_locator_rejects_mutable_or_sensitive_forms to pass match="graphforge-deployment-spec/1" to pytest.raises, asserting the bounded locator error message for every parameterized non-OCI locator case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iac/pulumi/typescript/src/index.ts`:
- Around line 669-675: Update canonicalJson’s key comparator to use
deterministic code-point ordering instead of localeCompare, ensuring TypeScript
serialization matches Python json.dumps(sort_keys=True) across runtimes.
Preserve the existing recursive canonicalization behavior, and add coverage
using keys that differ by punctuation or case to lock the expected ordering.
In `@iac/terraform/modules/deployment-spec/main.tf`:
- Around line 89-95: Update the OCI branch of local.locator_safe to validate
local.artifact_sha before interpolating it into the regex, using the existing
digest-validation logic or symbol. Ensure invalid digests short-circuit to false
so regex() is never evaluated with untrusted metacharacters, preserving the
bounded deployment_spec precondition diagnostic.
- Around line 45-54: Update the stable_secret_ids validation to limit
length(local.secret_ids) to 128 instead of 256, matching the shared contract;
leave the stable_source_ids validation unchanged.
- Line 137: Update the resolved_config_sha256 computation in the deployment
specification module to hash a canonical JSON representation whose escaping
matches the TypeScript and Python renderers, rather than relying on Terraform
jsonencode’s extra escaping of <, >, &, U+2028, and U+2029. Apply the same
canonical escaping rule in every renderer and add regression fixtures covering
these characters so all implementations produce identical hashes.
In `@iac/terraform/tests/deployment-spec-plan.sh`:
- Around line 74-75: Update the state-resource assertion in
deployment-spec-plan.sh to recursively traverse values.root_module and its
child_modules, counting resources at every module level. Ensure the assertion
fails when any root or child module creates a resource, while still treating
missing resources or child_modules as empty collections.
---
Nitpick comments:
In `@iac/pulumi/python/src/graphforge_pulumi/deployment.py`:
- Around line 107-110: Update the "bindings" projection in the deployment
rendering flow to copy target["secret_ids"] and target["source_ids"] into new
list objects rather than retaining caller-owned references. Preserve the
existing values and output structure while ensuring later mutations to
resolved_config cannot alter the generated spec or registered Pulumi output.
- Around line 160-169: Extract the shared canonical JSON serialization logic
from render_deployment_spec and render_deployment_spec_json into one private
helper, preserving ensure_ascii=False, compact separators, sorted keys, and the
trailing newline. Update both callers to use that helper so their serialized
output remains byte-equivalent.
- Around line 24-25: The _fail function is annotated as -> None but always
raises an exception, which causes type checkers to incorrectly assume execution
continues after calls to _fail. Update the return type annotation of _fail from
-> None to -> NoReturn (imported from the typing module) to correctly signal
that the function never returns normally. This will eliminate false positives at
line 46 and line 58 where type checkers incorrectly flag unreachable code.
In `@iac/pulumi/python/tests/test_deployment.py`:
- Around line 173-185: Update
test_non_oci_locator_rejects_mutable_or_sensitive_forms to pass
match="graphforge-deployment-spec/1" to pytest.raises, asserting the bounded
locator error message for every parameterized non-OCI locator case.
In `@iac/pulumi/typescript/src/index.ts`:
- Around line 535-559: In validateArtifactLocator, extract a local failure
helper that prepends the stable "invalid graphforge-deployment-spec/1: " prefix
to messages, then route each existing throw through it while preserving the
current message text and validation behavior.
In `@iac/pulumi/typescript/test/deployment-spec.test.ts`:
- Around line 36-43: Update the key comparator in canonical to use JavaScript’s
code-unit ordering rather than localeCompare, while preserving the existing
recursive canonicalization and output formatting.
In `@iac/terraform/tests/deployment-spec-plan.sh`:
- Around line 109-118: The Terraform negative suite must verify each case fails
for its intended precondition rather than accepting any non-zero plan. In
iac/terraform/tests/deployment-spec-plan.sh lines 109-118, read each case’s
expected diagnostic and require it to match the captured Terraform log before
accepting the failure. In iac/terraform/tests/deployment-spec-invalid.json lines
2-43, add an expect field to every existing case and add coverage for the
contract check, single-match check, artifact_safe, and stable_source_ids &&
stable_secret_ids preconditions.
- Around line 40-44: Update the deployment spec assertion in the jq expression
to remove the redundant utf8bytelength comparison and its hard-coded 1404
constant. Normalize the Terraform output and expected fixture JSON before
comparing, or explicitly document the existing key-order coupling if
normalization is not supported, so fixture reformatting does not break the test.
In `@tests/unit/test_graphforge_config_contract.py`:
- Around line 129-134: Add a deployment-schema rejection assertion following the
existing config and resolved-schema pattern from lines 85-90, using the
deployment example and an invalid mutation to verify validation fails. In the
deployment contract assertions, add the expected capability_compatibility status
assertion alongside infrastructure, connectivity, readiness, and artifact
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ceb0e5e5-b939-49ff-8bfa-654d1b585045
⛔ Files ignored due to path filters (13)
CHANGELOG.mdis excluded by!**/*.mddocs/adr/0016-repository-integration-and-deployment-configuration.mdis excluded by!**/*.md,!**/docs/**docs/contracts/examples/graphforge-deployment-spec-production-v1.jsonis excluded by!**/docs/**,!**/examples/**docs/contracts/graphforge-deployment-spec-v1.schema.jsonis excluded by!**/docs/**docs/guides/infrastructure-validation.mdis excluded by!**/*.md,!**/docs/**docs/reference/changelog.mdis excluded by!**/*.md,!**/docs/**iac/pulumi/README.mdis excluded by!**/*.mdiac/pulumi/python/README.mdis excluded by!**/*.mdiac/pulumi/typescript/README.mdis excluded by!**/*.mdiac/terraform/README.mdis excluded by!**/*.mdiac/terraform/examples/deployment-spec/main.tfis excluded by!**/examples/**iac/terraform/examples/deployment-spec/resolved.jsonis excluded by!**/examples/**iac/terraform/modules/deployment-spec/README.mdis excluded by!**/*.md
📒 Files selected for processing (20)
iac/pulumi/python/pyproject.tomliac/pulumi/python/src/graphforge_pulumi/__init__.pyiac/pulumi/python/src/graphforge_pulumi/deployment.pyiac/pulumi/python/tests/test_deployment.pyiac/pulumi/tests/preview/python/__main__.pyiac/pulumi/tests/preview/typescript/index.jsiac/pulumi/tests/verify_preview.pyiac/pulumi/typescript/package.jsoniac/pulumi/typescript/src/index.tsiac/pulumi/typescript/test/deployment-spec.test.tsiac/terraform/modules/deployment-spec/main.tfiac/terraform/modules/deployment-spec/outputs.tfiac/terraform/modules/deployment-spec/variables.tfiac/terraform/modules/deployment-spec/versions.tfiac/terraform/tests/deployment-spec-invalid.jsoniac/terraform/tests/deployment-spec-plan.shiac/terraform/tests/plan.shscripts/ci/classify-changes.shscripts/ci/test-classify-changes.shtests/unit/test_graphforge_config_contract.py
17 tasks done
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
graphforge-deployment-spec/1contract and frozen production fixtureEvidence
uv run --project iac/pulumi/python --extra test pytest iac/pulumi/python/tests(31 passed)uv run --project iac/pulumi/python --extra test ruff check iac/pulumi/pythonuv run --project iac/pulumi/python --extra test ruff format --check iac/pulumi/pythonnpm test && npm run format:checkiniac/pulumi/typescript(13 passed)iac/pulumi/tests/preview_acceptance.shTERRAFORM=<terraform-1.11.4> JQ=/usr/bin/jq iac/terraform/tests/deployment-spec-plan.sh(all six target topologies; 1,404-byte golden; 0 resources on apply/destroy)uv run pytest tests/unit/test_graphforge_config_contract.py(3 passed)scripts/ci/test-classify-changes.shmake docs-buildgit diff --checkCloses #227
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Add portable GraphForge deployment spec rendering for Pulumi and Terraform
graphforge-deployment-spec/1contract with a JSON Schema, a golden example, and an ADR entry describing a runtime- and provider-agnostic deployment specification.renderDeploymentSpec/DeploymentSpecin TypeScript and Python Pulumi SDKs and amodules/deployment-specTerraform module that produce a deterministic, secret-free projection from a resolved config, a target ID, and a validated artifact locator.repository@sha256:<64-hex>) with the digest matching the resolved config; non-OCI kinds must be credential-free HTTPS URLs; local paths, mutable tags, and inline credentials are rejected.graphforge:deployment:DeploymentSpec) registers no provider children and stores no secrets in state; outputs arespecandcanonical_json.canonicalJsonnow escapes<,>,&, U+2028, U+2029 for Terraform compatibility, changing output for inputs containing those characters.resolved_config_sha256computed in the Python validation module now uses the legacy-escaped canonical JSON, which can change the hash for configs containing<,>,&, U+2028, or U+2029.Macroscope summarized 8984fae.
Summary by CodeRabbit
New Features
Bug Fixes
Tests