Skip to content

OCTO-919: nested MULTI_FILE_RENDER output paths (ADR-0003, v0.8.0)#23

Merged
jebentier merged 6 commits into
mainfrom
OCTO-919-nested-multi-file-output
Jul 23, 2026
Merged

OCTO-919: nested MULTI_FILE_RENDER output paths (ADR-0003, v0.8.0)#23
jebentier merged 6 commits into
mainfrom
OCTO-919-nested-multi-file-output

Conversation

@jebentier

@jebentier jebentier commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Spike for nested output directories from MULTI_FILE_RENDER keys containing / (OCTO-919 / ST-11).

Decision: Outcome A (implemented). All four "small" bar conditions passed — change confined to Resource#write_template + Resource#output_path, full suite green with no pre-existing expectation changes, containment guard fits at the write site, and prune/reconcile/SPP need no code changes.

  • ADR docs/adrs/0003-nested-multi-file-output.md records investigation findings, ENOENT evidence, and the decision.
  • Resource calls FileUtils.mkdir_p before multi-file writes and enforces containment in output_path (lexical .. check + filesystem walk rejecting symlinks and verifying File.realpath of existing ancestors).
  • Specs cover nested write, escape guard, symlink bypass regression, Jsonnet producer passthrough, and a ResourceSet integration case.
  • Version bumped to 0.8.0 with CHANGELOG entry.

Rework (review attempt 1)

  • Blocking fix: output_path now walks existing path components with File.realpath and rejects symlinks inside @output_directory, closing the lexical-only start_with? bypass (committed symlink + nested key could write outside the sandbox).
  • Nit 1: ADR path-safety section updated for symlink-aware containment and per-key (not batch) atomicity.
  • Nit 2: Added resource_set_spec.rb integration test exercising full ResourceSetResource pipeline with nested-key fixture under us-east-1/staging/orange/.

Test plan

  • Symlink regression (red before fix, green after): bundle exec rspec spec/kubernetes_template_rendering/resource_spec.rb -e "traverses a symlink"
  • bundle exec rspec152 examples, 0 failures (147 pre-existing + 3 reviewer probing tests + 2 new)
  • End-to-end demo: render_templates writes pr-1/app/service.yaml and pr-1/mysql/database.yaml under us-east-1/staging/orange/
  • ADR grep checks: Context/Decision Outcome headers present; mkdocs.yml nav entry added
  • bundle exec rubocop lib spec — local crash loading inherited Invoca style-guide config (pre-existing/environmental); CI authoritative

Human-gated ACs (not executed by implementer)

  • [human-gated: rubygems credentials] Post-merge: tag + publish via bundle exec rake release; verify gem list -r -e kubernetes_template_rendering ≥ 0.8.0.
  • ST-01 Gemfile pin bump from 0.6.2 — separate library PR after gem publish; ST-05 must not rely on nested keys until both land.

Ticket

https://invoca.atlassian.net/browse/OCTO-919

Add mkdir_p before multi-file writes and a containment guard in
output_path so keys containing `/` render into nested directories.
Documents the decision in ADR-0003; bumps gem to 0.8.0.

Co-authored-by: Cursor <cursoragent@cursor.com>

@unblocked unblocked Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No issues found

About Unblocked

Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.

📖 Documentation — Learn more in our docs.

💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.

👍 Give feedback — React to comments with 👍 or 👎 to help us improve.

⚙️ Customize — Adjust settings in your preferences.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Enables nested output directories when MULTI_FILE_RENDER keys (or MULTI_FILE_RENDER_NAME values) contain /, by creating intermediate directories before writing and adding an output-directory containment guard to prevent .. path escapes. This supports producers emitting final pr-N/{app,mysql}/... layouts directly without post-render shuffling.

Changes:

  • Create intermediate directories (FileUtils.mkdir_p) before writing multi-file outputs.
  • Add an output_path containment guard that raises ArgumentError if a resolved path escapes the output directory.
  • Add specs/fixtures for nested-path multi-file keys, plus version + changelog + ADR + mkdocs nav updates.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spec/kubernetes_template_rendering/template_spec.rb Adds coverage asserting Jsonnet multi-file keys with / pass through unchanged.
spec/kubernetes_template_rendering/resource_spec.rb Adds filesystem-backed spec for nested directory creation and escape rejection.
spec/fixtures/jsonnet/nested_path_multi_file_example.jsonnet New Jsonnet fixture producing nested multi-file keys and names.
mkdocs.yml Adds ADR to documentation navigation.
lib/kubernetes_template_rendering/version.rb Bumps gem version to 0.8.0.
lib/kubernetes_template_rendering/resource.rb Creates intermediate dirs before writes; adds output path containment guard.
Gemfile.lock Updates local gem version entry to 0.8.0.
docs/adrs/0003-nested-multi-file-output.md Records the decision and rationale for enabling nested multi-file outputs.
CHANGELOG.md Documents the 0.8.0 change and behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/adrs/0003-nested-multi-file-output.md Outdated
jebentier and others added 3 commits July 23, 2026 16:14
Cross-model review (resource_spec.rb): pin correct behavior for a
literal ".." key, an absolute-looking key (neutralized by File.join,
not an escape), and a deeply nested key - to guard against regressions
in the new containment guard/mkdir_p logic.

Co-authored-by: Cursor <cursoragent@cursor.com>
Walk existing path components with File.realpath and reject symlinks
before multi-file writes. Add symlink regression spec and ResourceSet
integration test; document per-key atomicity in ADR-0003.

Co-authored-by: Cursor <cursoragent@cursor.com>
Cross-model review round 2: cover a symlink several levels deep (not
just the immediate child), a dangling symlink, a symlink chain
(symlink -> symlink -> outside), and confirm output_directory itself
being reached via a symlink still resolves and writes correctly. All
pass against the current validate_resolved_containment! fix.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread lib/kubernetes_template_rendering/resource.rb Outdated
output_path validated File.expand_path but returned the raw File.join
result, letting .. through a symlink segment escape at write time.
Return expanded so the write target matches what was validated; add
regression specs for ..+symlink ordering.

Co-authored-by: Cursor <cursoragent@cursor.com>
@jebentier

Copy link
Copy Markdown
Contributor Author

Addresses unblocked[bot] review on output_path: the method now returns expanded (the lexically-resolved path that passed containment checks) instead of the raw File.join result. Regression specs cover a/../b/foo.yaml through a symlink and link1/../link2/pwn.yaml between symlink segments. ADR path-safety section updated.

…edge case

Cross-model review round 3: confirms .. correctly collapses through two
distinct symlinked segments without ever touching either symlink target
(both stay empty), and that a key whose .. segments resolve exactly to
the output directory itself raises rather than silently succeeding.

Co-authored-by: Cursor <cursoragent@cursor.com>
@jebentier
jebentier merged commit d24cff1 into main Jul 23, 2026
7 checks passed
@jebentier
jebentier deleted the OCTO-919-nested-multi-file-output branch July 23, 2026 15:17
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.

2 participants