From b02bcc74ef5a0621fe677136f98ebc72b08bbe94 Mon Sep 17 00:00:00 2001 From: Ajay Krishnan Date: Mon, 20 Jul 2026 14:26:22 -0700 Subject: [PATCH] Harden alpha8 publication safety --- .github/workflows/release.yml | 2 + CHANGELOG.md | 12 ++ Gemfile | 2 +- README.md | 44 ++++--- examples/rails_integration.rb | 109 ++++++++-------- lib/opencode-rails.rb | 6 + lib/opencode/message_artifacts.rb | 4 + lib/opencode/sandbox_file.rb | 50 ++++++-- lib/opencode/transform.rb | 7 +- lib/opencode/uploaded_files_prompt.rb | 77 ++++++++++-- opencode-rails.gemspec | 1 + test/example_test.rb | 44 +++++++ test/opencode/loading_test.rb | 26 +++- test/opencode/message_artifacts_test.rb | 30 +++++ test/opencode/sandbox_file_test.rb | 47 +++++++ test/opencode/transform_test.rb | 10 +- test/opencode/uploaded_files_prompt_test.rb | 133 ++++++++++++++++++++ test/readme_test.rb | 19 +++ test/release_workflow_test.rb | 11 ++ 19 files changed, 532 insertions(+), 102 deletions(-) create mode 100644 test/example_test.rb diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1cd2158..7c01849 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -29,4 +29,6 @@ jobs: 'expected = ENV.fetch("RELEASE_TAG").delete_prefix("v"); abort "tag #{expected.inspect} does not match gem #{Opencode::RAILS_VERSION.inspect}" unless Opencode::RAILS_VERSION == expected' + - name: Run tests + run: bundle exec rake test - uses: rubygems/release-gem@052cc82692552de3ef2b81fd670e41d13cba8092 # v1.4.0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 018365d..697561d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ hardened SSE framing parser while retaining the alpha7 subscribe-before- prompt and at-most-once reconnect contract. +### Fixed + +- Keep transform destination filenames out of the agent-authored identity + attachment path and require transforms to verify trust explicitly. +- Anchor uploads to an opened sandbox directory across path swaps, replace + destination symlinks without following them, and validate bounded reads from + the opened sandbox file. Upload copying now requires a traversable + `/proc/self/fd` or `/dev/fd` descriptor filesystem and fails closed without it. +- Load every runtime standard library explicitly and align the shipped + permissions, observer, Turn, prompt, and instrumentation examples with the + actual APIs. + ### Changed - Test the supported runtime surface on Ruby 3.2, 3.3, 3.4, and 4.0. diff --git a/Gemfile b/Gemfile index fb26b46..fc53998 100644 --- a/Gemfile +++ b/Gemfile @@ -2,6 +2,6 @@ source "https://rubygems.org" gem "opencode-ruby", git: "https://github.com/ajaynomics/opencode-ruby.git", - ref: "9277646a4bb2cf25a8384ffc140b154f49ea5766" + ref: "49a161632e6631d3605af5170de00c4688cfcedb" gemspec diff --git a/README.md b/README.md index 9bca90b..9e00434 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ exact `opencode-ruby` source it was tested with: # Gemfile gem "opencode-ruby", git: "https://github.com/ajaynomics/opencode-ruby.git", - ref: "9277646a4bb2cf25a8384ffc140b154f49ea5766" + ref: "49a161632e6631d3605af5170de00c4688cfcedb" gem "opencode-rails", path: "../opencode-rails" @@ -42,7 +42,13 @@ gem "opencode-rails", bundle install ``` -Runtime deps: `activerecord`, `activestorage`, `activesupport` (>= 7.1). Depends on `opencode-ruby` for the underlying HTTP/SSE primitives. +Runtime deps: `activerecord`, `activestorage`, `activesupport` (>= 7.1), and +`marcel`. Depends on `opencode-ruby` for the underlying HTTP/SSE primitives. + +`UploadedFilesPrompt` anchors upload writes through `/proc/self/fd` or +`/dev/fd` to prevent sandbox-root path swaps. Hosts using upload copying must +provide one of those traversable descriptor filesystems; unsupported platforms +fail closed before writing. During the alpha series both gems are pinned in lockstep. Version 0.0.1.alpha8 retains the subscribe-ready-before-prompt transport contract and reconnects an @@ -86,9 +92,11 @@ end ```ruby # app/jobs/generate_response_job.rb class GenerateResponseJob < ApplicationJob - def perform(assistant_message) + def perform(assistant_message, user_message) conversation = assistant_message.conversation - user_message = conversation.messages.where(role: :user).last + unless user_message.conversation_id == conversation.id + raise ArgumentError, "User and assistant messages must belong to the same conversation" + end client = Opencode::Client.new(base_url: ENV.fetch("OPENCODE_URL")) session = Opencode::Session.new( @@ -121,7 +129,14 @@ class GenerateResponseJob < ApplicationJob def permission_rules_for(conversation) [ - { type: "edit", action: "allow", path: "data/sandbox/#{conversation.id}/" } + { permission: "bash", pattern: "*", action: "deny" }, + { permission: "external_directory", pattern: "*", action: "deny" }, + { permission: "edit", pattern: "*", action: "deny" }, + { + permission: "edit", + pattern: "data/sandbox/#{conversation.id}/**", + action: "allow" + } ] end end @@ -159,17 +174,18 @@ Plus everything from `opencode-ruby`: `Client`, `Reply`, `ReplyObserver`, `Trace ## Instrumentation + error reporting -The gem emits events through `Opencode::Instrumentation.instrument(name, payload, &blk)` and reports swallowed errors through `Opencode::ErrorReporter.report(error, **opts)`. Both are no-ops by default. Wire your host's emitter / reporter in an initializer (see Quickstart above). +The wire client emits `opencode.request` and artifact extraction emits +`opencode.apply_patch.artifacts_dropped` through `Opencode::Instrumentation`. +Turn events flow through the injected tracer; with the Quickstart's +`assistant.` prefix they include: -Events emitted (non-exhaustive): +- `assistant.response.started`, `assistant.turn.finished` +- `assistant.stream.completed`, `assistant.stream.interrupted` +- `assistant.session.created`, `assistant.session.recreated_with_resend` +- `assistant.response.upstream_error` -- `opencode.turn.started`, `opencode.turn.finished` -- `opencode.stream.completed`, `opencode.stream.interrupted` -- `opencode.session.created`, `opencode.session.recreated` -- `opencode.apply_patch.artifacts_dropped` -- `opencode.response.upstream_error` - -Subscribe via `ActiveSupport::Notifications.subscribe("opencode.*")` once you've wired the adapter. +Swallowed errors flow through `Opencode::ErrorReporter.report(error, **opts)`. +Instrumentation and error reporting are no-ops until the host wires adapters. ## Position diff --git a/examples/rails_integration.rb b/examples/rails_integration.rb index 075d9f1..66b981d 100644 --- a/examples/rails_integration.rb +++ b/examples/rails_integration.rb @@ -27,8 +27,7 @@ # 3. ReplyObserver: bridge Reply state to Turbo Stream broadcasts. # 4. Permissions builder: per-product session permission rules. # -# Tested patterns. Every line below has been exercised in production. -# Copy what you need; adapt to your domain. +# Contract-tested reference. Copy what you need and adapt it to your domain. # ---------------------------------------------------------------------- # 1. config/initializers/opencode.rb @@ -39,10 +38,8 @@ # events to its observability stack. Rails.application.config.to_prepare do - # opencode-ruby: every HTTP request, SSE event lifecycle, recovery - # path flows through this adapter as an ActiveSupport::Notifications - # event. Subscribers in this app pick it up via - # `ActiveSupport::Notifications.subscribe("opencode.*", ...)`. + # opencode-ruby HTTP requests flow through this adapter as + # `opencode.request` ActiveSupport::Notifications events. Opencode::Instrumentation.adapter = ->(name, payload, &block) { ActiveSupport::Notifications.instrument(name, payload, &block) } @@ -70,13 +67,15 @@ class GenerateResponseJob < ApplicationJob # SolidQueue concurrency_key — only one turn per conversation in # flight at a time. Without this, a user sending two messages back- # to-back can race two turns through the same OpenCode session. - limits_concurrency to: 1, key: ->(message) { "GenerateResponseJob/#{message.conversation_id}" } + limits_concurrency to: 1, key: ->(message, _user_message) { "GenerateResponseJob/#{message.conversation_id}" } - def perform(assistant_message) + def perform(assistant_message, user_message) return if assistant_message.terminal? # idempotent conversation = assistant_message.conversation - user_message = conversation.messages.where(role: :user).order(:created_at).last + unless user_message.conversation_id == conversation.id + raise ArgumentError, "User and assistant messages must belong to the same conversation" + end client = Opencode::Client.new(base_url: ENV.fetch("OPENCODE_URL")) # Session: AR-coupled, row-locked, idempotent. ensure! creates the @@ -96,11 +95,13 @@ class GenerateResponseJob < ApplicationJob subject: conversation, query_text: user_message.content, client: client, - session_for: ->(*) { session }, + session_for: session, observer_factory: ->(message) { ReplyStream.new(message: message) }, - system_context: build_system_context(conversation), - agent_name: "default", - tracer: Opencode::Tracer.new(prefix: "opencode."), + system_context: ->(record) { build_system_context(record) }, + agent_name: ->(_record) { "build" }, + tracer: ->(name, **payload) { + ActiveSupport::Notifications.instrument("assistant.#{name}", payload) + }, on_turn_finished: ->(result) { Rails.logger.info("turn finished status=#{result.status} cost=#{result.cost}") } @@ -117,19 +118,25 @@ class GenerateResponseJob < ApplicationJob # Per-product permissions. The shape mirrors what # opencode-ruby's Client#create_session expects in `permissions:`. [ - { type: "edit", action: "allow", path: "data/sandbox/#{conversation.id}/" }, - { type: "edit", action: "deny", path: "*" } # default deny outside sandbox + { permission: "bash", pattern: "*", action: "deny" }, + { permission: "external_directory", pattern: "*", action: "deny" }, + { permission: "edit", pattern: "*", action: "deny" }, + { + permission: "edit", + pattern: "data/sandbox/#{conversation.id}/**", + action: "allow" + } ] end def build_system_context(conversation) # System prompt context the agent gets. Your app probably already # has helpers for this; the gem doesn't impose a shape. - { - user_name: conversation.user.name, - conversation_id: conversation.id, - sandbox_path: "/sandbox/#{conversation.id}" - } + <<~CONTEXT + User: #{conversation.user.name} + Conversation: #{conversation.id} + Sandbox: /sandbox/#{conversation.id} + CONTEXT end end @@ -138,7 +145,7 @@ end # ---------------------------------------------------------------------- # # An Opencode::ReplyObserver implementation that bridges the gem's -# state-machine callbacks (part_appended, part_updated, finalized) to +# state-machine callbacks (part_added, part_changed, part_finalized) to # Turbo Stream broadcasts. The gem ships the protocol; the host owns # the rendering. # @@ -147,51 +154,47 @@ end # build_system_context above. class ReplyStream + include Opencode::ReplyObserver + def initialize(message:) @message = message @parts_dom_id = "parts_message_#{message.id}" end - # Called every time a new part shows up in the reply. - def on_part_appended(part) + def watch(reply) + reply.add_observer(self) + self + end + + def part_added(part:, index:) Turbo::StreamsChannel.broadcast_append_to( @message.conversation, target: @parts_dom_id, partial: "messages/part", - locals: { part: part, message: @message } + locals: { part: part, index: index, message: @message } ) end - # Called when an existing part's content grows (text/reasoning - # deltas, tool state changes). - def on_part_updated(part, _index) + def part_changed(part:, index:, delta:) + broadcast_update(part, index) + end + + def part_finalized(part:, index:) + broadcast_update(part, index) + end + + def tool_progressed(part:, index:, status:, raw:) + broadcast_update(part, index) + end + + private + + def broadcast_update(part, index) Turbo::StreamsChannel.broadcast_update_to( @message.conversation, - target: "part_#{part['id']}_message_#{@message.id}", + target: "part_#{index}_message_#{@message.id}", partial: "messages/part", - locals: { part: part, message: @message } - ) - end - - # Called when the turn finalizes. Use this to swap "Thinking…" - # placeholders, update message status indicators, etc. - def on_finalized(reply_result) - Turbo::StreamsChannel.broadcast_update_to( - @message.conversation, - target: "message_#{@message.id}_status", - partial: "messages/status", - locals: { message: @message, result: reply_result } - ) - end - - # Optional: called when the gem catches an exception mid-stream and - # has done its own recovery (recreated session, retried, etc.). Use - # this for a brief transient banner in the UI. - def on_error(message, severity:) - Turbo::StreamsChannel.broadcast_replace_to( - @message.conversation, - target: "message_#{@message.id}_status", - html: %() + locals: { part: part, index: index, message: @message } ) end end @@ -201,9 +204,7 @@ end # - Idempotent session create/resolve with row-level locking # - SSE stream consumption + reconnection on transport hiccups # - SessionNotFoundError / StaleSessionError recovery (recreate + retry) -# - Mid-stream parts_json snapshotting via update_columns (bypasses -# after_save callbacks; your row-level Turbo broadcasts fire on -# YOUR cadence, not every SSE event) +# - ReplyObserver callbacks for host-owned live broadcasts or snapshots # - CAS-safe finalize: message reloaded under row lock, transitions # :pending -> :completed only if a concurrent cancel hasn't already # moved it out of :pending. diff --git a/lib/opencode-rails.rb b/lib/opencode-rails.rb index 9b7b93a..181d53d 100644 --- a/lib/opencode-rails.rb +++ b/lib/opencode-rails.rb @@ -9,6 +9,12 @@ # rename work. require "opencode-ruby" +require "fileutils" +require "marcel" +require "pathname" +require "set" +require "stringio" +require "tempfile" require "active_support/core_ext/object/blank" # blank?, present?, presence require "active_support/core_ext/object/try" diff --git a/lib/opencode/message_artifacts.rb b/lib/opencode/message_artifacts.rb index 5d77b15..6166e36 100644 --- a/lib/opencode/message_artifacts.rb +++ b/lib/opencode/message_artifacts.rb @@ -80,6 +80,10 @@ module Opencode if (transform = transforms.find { |t| t.applies_to?(file) }) attached += 1 if apply_transform(transform, file) + elsif transform_owned_filenames.include?(file.basename) + # Destination names belong exclusively to their host transform. + # Agent-authored bytes must never fall through as trusted output. + next elsif default_attach == :all # Default identity path: every safe sandbox file that no # transform claims attaches as-is. Callers that want the diff --git a/lib/opencode/sandbox_file.rb b/lib/opencode/sandbox_file.rb index 4cabb21..717f8f6 100644 --- a/lib/opencode/sandbox_file.rb +++ b/lib/opencode/sandbox_file.rb @@ -15,6 +15,8 @@ module Opencode # not here — the file doesn't know which turn opened "after." That's # a property of the scan, not a property of the file. class SandboxFile + UnsafeFileError = Class.new(Opencode::Error) + attr_reader :path, :sandbox_prefix def initialize(path:, sandbox_prefix:, max_bytes:) @@ -36,7 +38,13 @@ module Opencode end def content - File.read(path) + with_safe_file do |file| + content = file.read(@max_bytes + 1) || "".b + if content.bytesize > @max_bytes + raise UnsafeFileError, "Sandbox file exceeds size limit while reading: #{basename}" + end + content + end end def content_type @@ -52,17 +60,11 @@ module Opencode # - Reject anything over the size cap (default # Opencode::ResponseParser::MAX_ARTIFACT_SIZE = 10 MB). # - # The Sandbox scan filters non-files (directories, FIFOs) before - # yielding, so we don't re-check #file? here. + # Revalidates the opened file descriptor so a path swap between the + # sandbox scan and the read cannot redirect content outside the sandbox. def safe? - return false if File.symlink?(path) - return false unless Pathname.new(path).realpath.to_s.start_with?(sandbox_prefix) - return false if size > @max_bytes - - true - rescue Errno::ENOENT - # Concurrent deletion between scan-yield and safety-check — treat - # as unsafe so the orchestrator skips rather than crashing. + with_safe_file { true } + rescue UnsafeFileError false end @@ -77,5 +79,31 @@ module Opencode content_type: content_type ) end + + private + + def with_safe_file + before = File.lstat(path) + raise UnsafeFileError, "Unsafe sandbox file: #{basename}" unless before.file? && !before.symlink? + + resolved = Pathname.new(path).realpath.to_s + unless resolved.start_with?(sandbox_prefix) + raise UnsafeFileError, "Sandbox file escapes its root: #{basename}" + end + + File.open(path, "rb") do |file| + opened = file.stat + unless opened.file? && opened.dev == before.dev && opened.ino == before.ino + raise UnsafeFileError, "Sandbox file changed while opening: #{basename}" + end + if opened.size > @max_bytes + raise UnsafeFileError, "Sandbox file exceeds size limit: #{basename}" + end + + yield file + end + rescue Errno::ENOENT, Errno::ELOOP => e + raise UnsafeFileError, "Unsafe sandbox file #{basename}: #{e.message}" + end end end diff --git a/lib/opencode/transform.rb b/lib/opencode/transform.rb index cf414b6..11a088e 100644 --- a/lib/opencode/transform.rb +++ b/lib/opencode/transform.rb @@ -31,7 +31,8 @@ module Opencode # trusted?(attachment) — true if the attachment was produced by # this transform (used by Impostor.for and # by view code that decides inline-render - # vs download). Default: filename match. + # vs download). Default: false; subclasses + # must verify host-authored metadata. # purge_impostors? — if true, before attaching the substrate # deletes any existing attachment whose # filename matches destination_filename @@ -60,8 +61,8 @@ module Opencode raise NotImplementedError, "#{self.class.name} must implement #render" end - def trusted?(attachment) - attachment.filename.to_s == destination_filename + def trusted?(_attachment) + false end def purge_impostors? diff --git a/lib/opencode/uploaded_files_prompt.rb b/lib/opencode/uploaded_files_prompt.rb index 2eb80f7..74561e2 100644 --- a/lib/opencode/uploaded_files_prompt.rb +++ b/lib/opencode/uploaded_files_prompt.rb @@ -24,8 +24,10 @@ module Opencode # Side effect, unchanged from the concern: file bytes are copied from # ActiveStorage into the per-user OpenCode sandbox directory so the # agent can read them with the `read` tool. The copy is path-escape - # guarded (the cleanpath of the destination must start with the - # sandbox dir prefix, no symlink trickery). + # guarded: generated names must be basenames, and all writes stay anchored + # to an opened directory handle even if the sandbox path is replaced. + # A temporary file is renamed into place so destination symlinks are + # replaced rather than followed. Retried jobs can refresh existing copies. class UploadedFilesPrompt attr_reader :text, :sandbox_file_names @@ -52,24 +54,77 @@ module Opencode [ raw, "", - "The user uploaded #{file_instructions.size} file(s). Read each file thoroughly, then consult your reference materials and verify any legal claims before responding:", + "The user uploaded #{file_instructions.size} file(s). Read each file thoroughly before responding:", *file_instructions ].join("\n").strip end def copy_to_sandbox(file) - FileUtils.mkdir_p(@sandbox_path) + sandbox_path = Pathname.new(@sandbox_path).expand_path + FileUtils.mkdir_p(sandbox_path) + sandbox_stat = File.lstat(sandbox_path) + unless sandbox_stat.directory? && !sandbox_stat.symlink? + raise ArgumentError, "Sandbox root must be a directory, not a symlink: #{sandbox_path}" + end - sandbox_name = @sandbox_name_for.call(file) - dest = File.join(@sandbox_path, sandbox_name) - - resolved = Pathname.new(dest).cleanpath.to_s - unless resolved.start_with?(@sandbox_path) + sandbox_name = @sandbox_name_for.call(file).to_s + unless sandbox_name == File.basename(sandbox_name) && !%w[. ..].include?(sandbox_name) raise ArgumentError, "Filename escapes sandbox: #{sandbox_name}" end - File.open(dest, "wb") { |f| f.write(file.download) } - Placement.new(sandbox_name, dest) + File.open(sandbox_path, File::RDONLY) do |directory| + opened_stat = directory.stat + unless opened_stat.directory? && same_file?(sandbox_stat, opened_stat) + raise ArgumentError, "Sandbox root changed while opening: #{sandbox_path}" + end + + directory_path = directory_handle_path(directory) + dest = File.join(directory_path, sandbox_name) + mode = destination_mode(dest) + + Tempfile.create([ ".opencode-upload-", ".tmp" ], directory_path) do |temp| + temp.binmode + temp.write(file.download) + temp.flush + temp.chmod(mode) + File.rename(temp.path, dest) + + unless same_file_at_path?(sandbox_path, opened_stat) + File.unlink(dest) + raise ArgumentError, "Sandbox root changed during upload: #{sandbox_path}" + end + end + end + Placement.new(sandbox_name, sandbox_path.join(sandbox_name).to_s) + end + + def directory_handle_path(directory) + stat = directory.stat + [ "/proc/self/fd/#{directory.fileno}", "/dev/fd/#{directory.fileno}" ].find do |path| + same_file?(File.stat(path), stat) + rescue Errno::ENOENT + false + end || raise(ArgumentError, "Platform cannot anchor writes to the opened sandbox directory") + end + + def destination_mode(path) + stat = File.lstat(path) + return stat.mode & 0o777 if stat.file? && !stat.symlink? + + 0o666 & ~File.umask + rescue Errno::ENOENT + 0o666 & ~File.umask + end + + def same_file_at_path?(path, expected) + current = File.lstat(path) + current.directory? == expected.directory? && !current.symlink? && same_file?(current, expected) + rescue Errno::ENOENT + false + end + + def same_file?(left, right) + left.dev == right.dev && left.ino == right.ino end # Tiny value pair returned by copy_to_sandbox: the canonical filename diff --git a/opencode-rails.gemspec b/opencode-rails.gemspec index 8d15e98..0bf035c 100644 --- a/opencode-rails.gemspec +++ b/opencode-rails.gemspec @@ -38,6 +38,7 @@ Gem::Specification.new do |spec| # exactly (= not ~>) so that consumers always pick the version this gem # was tested against. spec.add_runtime_dependency "opencode-ruby", "= 0.0.1.alpha8" + spec.add_runtime_dependency "marcel", "~> 1.0" # Rails sub-libraries used at runtime. Depending on these individually # (instead of the `rails` umbrella) avoids forcing host apps to load diff --git a/test/example_test.rb b/test/example_test.rb new file mode 100644 index 0000000..3efdbdc --- /dev/null +++ b/test/example_test.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require "test_helper" +require "ripper" + +class ExampleTest < Minitest::Test + PATH = File.expand_path("../examples/rails_integration.rb", __dir__) + SOURCE = File.read(PATH) + + def test_example_parses + assert Ripper.sexp(SOURCE) + end + + def test_example_uses_turn_collaborator_contracts + assert_includes SOURCE, "def perform(assistant_message, user_message)" + assert_includes SOURCE, "session_for: session" + assert_includes SOURCE, "system_context: ->(record)" + assert_includes SOURCE, "agent_name: ->(_record)" + assert_includes SOURCE, '{ "build" }' + assert_includes SOURCE, "def watch(reply)" + assert_includes SOURCE, "include Opencode::ReplyObserver" + assert_includes SOURCE, 'ActiveSupport::Notifications.instrument("assistant.#{name}", payload)' + refute_includes SOURCE, 'Opencode::Tracer.new(prefix: "opencode.")' + refute_includes SOURCE, ".order(:created_at).last" + end + + def test_example_uses_reply_indexes_for_dom_identity + assert_includes SOURCE, 'target: "part_#{index}_message_#{@message.id}"' + assert_includes SOURCE, "locals: { part: part, index: index, message: @message }" + refute_includes SOURCE, "part['id']" + end + + def test_example_does_not_claim_unimplemented_mid_stream_persistence + refute_includes SOURCE, "Mid-stream parts_json snapshotting" + refute_includes SOURCE, "update_columns" + end + + def test_example_uses_current_fail_closed_permission_rules + refute_match(/\{\s*type:/, SOURCE) + assert_includes SOURCE, '{ permission: "bash", pattern: "*", action: "deny" }' + assert_includes SOURCE, '{ permission: "external_directory", pattern: "*", action: "deny" }' + assert_includes SOURCE, '{ permission: "edit", pattern: "*", action: "deny" }' + end +end diff --git a/test/opencode/loading_test.rb b/test/opencode/loading_test.rb index a6c2869..de43b71 100644 --- a/test/opencode/loading_test.rb +++ b/test/opencode/loading_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "test_helper" +require "open3" # Smoke test: every constant the gem promises is defined and points at # the right kind of object. If require "opencode-rails" loads cleanly, @@ -46,8 +47,9 @@ class Opencode::LoadingTest < Minitest::Test def test_session_constant_points_at_this_gem location = Opencode::Session.instance_method(:initialize).source_location.first - assert_match GEM_SOURCE_PATTERN.call("opencode-rails", "session"), location, - "Expected Opencode::Session to be loaded from opencode-rails, got: #{location}" + expected = File.expand_path("../../lib/opencode/session.rb", __dir__) + assert_equal expected, File.expand_path(location), + "Expected Opencode::Session to be loaded from this opencode-rails checkout" end def test_client_constant_points_at_opencode_ruby @@ -71,4 +73,24 @@ class Opencode::LoadingTest < Minitest::Test refute Opencode.const_defined?(:Rails), "Opencode::Rails must not be defined — it would shadow ::Rails inside the Opencode namespace" end + + def test_umbrella_require_loads_runtime_standard_libraries + root = File.expand_path("../..", __dir__) + script = <<~'RUBY' + require "opencode-rails" + abort "StringIO missing" unless defined?(StringIO) + abort "Marcel missing" unless defined?(Marcel::MimeType) + abort "FileUtils missing" unless defined?(FileUtils) + abort "Tempfile missing" unless defined?(Tempfile) + RUBY + + _stdout, stderr, status = Open3.capture3( + Gem.ruby, + "-I#{File.join(root, "lib")}", + "-e", + script + ) + + assert status.success?, stderr + end end diff --git a/test/opencode/message_artifacts_test.rb b/test/opencode/message_artifacts_test.rb index ad8bd6c..4f61561 100644 --- a/test/opencode/message_artifacts_test.rb +++ b/test/opencode/message_artifacts_test.rb @@ -7,6 +7,23 @@ require "test_helper" # coverage — including ActiveStorage attachment, Transform application, # error reporting via Opencode::ErrorReporter — lives in the host app. class Opencode::MessageArtifactsTest < Minitest::Test + Message = Struct.new(:id, keyword_init: true) + Sandbox = Struct.new(:entries, keyword_init: true) do + def exists? = true + def files(after: nil) = entries + end + SandboxEntry = Struct.new(:basename, :attached, keyword_init: true) do + def as_artifact + self.attached = true + raise "destination must not use the identity attachment path" + end + end + + class Transform < Opencode::Transform + def source_filename = "source.json" + def destination_filename = "rendered.html" + end + def test_initialize_takes_message_feature_and_optional_transforms params = Opencode::MessageArtifacts.instance_method(:initialize).parameters by_kind = params.group_by(&:first).transform_values { |list| list.map(&:last) } @@ -23,4 +40,17 @@ class Opencode::MessageArtifactsTest < Minitest::Test assert_equal [ :attach_from ], Opencode::MessageArtifacts.instance_methods(false), "MessageArtifacts's only public verb is #attach_from" end + + def test_agent_authored_transform_destination_never_uses_default_attachment + entry = SandboxEntry.new(basename: "rendered.html", attached: false) + artifacts = Opencode::MessageArtifacts.new( + message: Message.new(id: 1), + feature: "test", + transforms: [ Transform.new ] + ) + + artifacts.attach_from(sandbox: Sandbox.new(entries: [ entry ])) + + refute entry.attached + end end diff --git a/test/opencode/sandbox_file_test.rb b/test/opencode/sandbox_file_test.rb index 78e26e9..554ab60 100644 --- a/test/opencode/sandbox_file_test.rb +++ b/test/opencode/sandbox_file_test.rb @@ -11,6 +11,7 @@ require "marcel" # SandboxFile#content_type uses Marcel::MimeType.for class Opencode::SandboxFileTest < Minitest::Test def setup @tmpdir = Dir.mktmpdir("opencode-rails-sandbox-file-test-") + @outside_dir = Dir.mktmpdir("opencode-rails-sandbox-file-outside-") @path = File.join(@tmpdir, "notes.md") File.write(@path, "# hello\nworld\n") # SandboxFile uses `start_with?` against this prefix to detect path @@ -21,6 +22,7 @@ class Opencode::SandboxFileTest < Minitest::Test def teardown FileUtils.remove_entry(@tmpdir) if @tmpdir && File.exist?(@tmpdir) + FileUtils.remove_entry(@outside_dir) if @outside_dir && File.exist?(@outside_dir) end def test_basic_readers @@ -59,4 +61,49 @@ class Opencode::SandboxFileTest < Minitest::Test assert_equal "notes.md", artifact.filename assert_equal "# hello\nworld\n", artifact.content end + + def test_content_rejects_a_file_replaced_by_an_outside_symlink + file = Opencode::SandboxFile.new( + path: @path, sandbox_prefix: @sandbox_prefix, max_bytes: 10_000 + ) + outside = File.join(@outside_dir, "private.txt") + File.write(outside, "private") + assert file.safe? + + File.unlink(@path) + File.symlink(outside, @path) + + assert_raises(Opencode::SandboxFile::UnsafeFileError) { file.content } + end + + def test_content_rejects_an_inode_that_grows_past_the_cap_while_reading + file = Opencode::SandboxFile.new( + path: @path, sandbox_prefix: @sandbox_prefix, max_bytes: 20 + ) + real_open = File.method(:open) + grow_on_read = lambda do |path, *args, **kwargs, &block| + real_open.call(path, *args, **kwargs) do |opened| + proxy = Object.new + proxy.define_singleton_method(:stat) { opened.stat } + proxy.define_singleton_method(:read) do |length = nil| + real_open.call(path, "ab") { |writer| writer.write("x" * 100) } + opened.read(length) + end + block.call(proxy) + end + end + + File.stub(:open, grow_on_read) do + assert_raises(Opencode::SandboxFile::UnsafeFileError) { file.content } + end + end + + def test_content_returns_binary_empty_string_for_a_zero_byte_file + File.write(@path, "") + file = Opencode::SandboxFile.new( + path: @path, sandbox_prefix: @sandbox_prefix, max_bytes: 20 + ) + + assert_equal "".b, file.content + end end diff --git a/test/opencode/transform_test.rb b/test/opencode/transform_test.rb index 00e91b8..06806c8 100644 --- a/test/opencode/transform_test.rb +++ b/test/opencode/transform_test.rb @@ -29,7 +29,7 @@ class Opencode::TransformTest < Minitest::Test end # A trivial concrete subclass exercises the defaults that DO exist - # (#applies_to?, #trusted?, #owned_filenames all delegate to the + # (#applies_to? and #owned_filenames delegate to the # two abstract filename methods). class FakeTransform < Opencode::Transform def source_filename = "agent-output.json" @@ -48,13 +48,11 @@ class Opencode::TransformTest < Minitest::Test refute transform.applies_to?(other) end - def test_trusted_matches_destination_filename_by_default + def test_trusted_fails_closed_by_default transform = FakeTransform.new - trusted = Attachment.new(filename: "rendered.html") - untrusted = Attachment.new(filename: "agent-output.json") + destination = Attachment.new(filename: "rendered.html") - assert transform.trusted?(trusted) - refute transform.trusted?(untrusted) + refute transform.trusted?(destination) end def test_owned_filenames_is_source_and_destination diff --git a/test/opencode/uploaded_files_prompt_test.rb b/test/opencode/uploaded_files_prompt_test.rb index d6777c1..c0f63ff 100644 --- a/test/opencode/uploaded_files_prompt_test.rb +++ b/test/opencode/uploaded_files_prompt_test.rb @@ -17,6 +17,13 @@ class Opencode::UploadedFilesPromptTest < Minitest::Test EmptyFiles = Struct.new(:attached) do def attached? = attached end + AttachedFiles = Class.new(Array) do + def attached? = true + end + FakeUpload = Struct.new(:filename, :content_type, :content, keyword_init: true) do + def byte_size = content.bytesize + def download = content + end def setup @tmpdir = Dir.mktmpdir("opencode-rails-uploaded-prompt-test-") @@ -52,4 +59,130 @@ class Opencode::UploadedFilesPromptTest < Minitest::Test assert_equal %i[sandbox_file_names text].sort, Opencode::UploadedFilesPrompt.instance_methods(false).sort end + + def test_copies_an_upload_without_domain_specific_instructions + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "hello") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + prompt = Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: @tmpdir, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + + assert_equal "hello", File.binread(File.join(@tmpdir, "upload.txt")) + assert_includes prompt.text, "Read each file thoroughly before responding:" + refute_includes prompt.text, "legal claims" + refute_includes prompt.text, "reference materials" + end + + def test_rejects_a_sibling_prefix_escape + sandbox = File.join(@tmpdir, "foo") + sibling = File.join(@tmpdir, "foobar") + FileUtils.mkdir_p(sibling) + target = File.join(sibling, "target.txt") + File.write(target, "safe") + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "overwritten") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + + assert_raises(ArgumentError) do + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: sandbox, + sandbox_name_for: ->(_file) { "../foobar/target.txt" } + ) + end + assert_equal "safe", File.read(target) + end + + def test_atomically_replaces_a_destination_symlink_without_following_it + outside = File.join(@tmpdir, "outside.txt") + sandbox = File.join(@tmpdir, "sandbox") + FileUtils.mkdir_p(sandbox) + File.write(outside, "safe") + File.symlink(outside, File.join(sandbox, "upload.txt")) + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "overwritten") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: sandbox, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + + assert_equal "safe", File.read(outside) + assert_equal "overwritten", File.read(File.join(sandbox, "upload.txt")) + refute File.symlink?(File.join(sandbox, "upload.txt")) + end + + def test_retried_copy_refreshes_an_existing_sandbox_file + sandbox = File.join(@tmpdir, "sandbox") + FileUtils.mkdir_p(sandbox) + File.write(File.join(sandbox, "upload.txt"), "stale") + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "fresh") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: sandbox, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + + assert_equal "fresh", File.read(File.join(sandbox, "upload.txt")) + end + + def test_rejects_a_sandbox_root_replaced_during_the_copy + sandbox = File.join(@tmpdir, "sandbox") + moved_sandbox = File.join(@tmpdir, "moved-sandbox") + FileUtils.mkdir_p(sandbox) + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "private") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + create = Tempfile.method(:create) + replace_root = lambda do |*args, **kwargs, &block| + File.rename(sandbox, moved_sandbox) + FileUtils.mkdir_p(sandbox) + create.call(*args, **kwargs, &block) + end + + Tempfile.stub(:create, replace_root) do + assert_raises(ArgumentError) do + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: sandbox, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + end + end + + refute File.exist?(File.join(sandbox, "upload.txt")) + refute File.exist?(File.join(moved_sandbox, "upload.txt")) + end + + def test_new_upload_uses_the_normal_umask_file_mode + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "hello") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: @tmpdir, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + + assert_equal 0o666 & ~File.umask, File.stat(File.join(@tmpdir, "upload.txt")).mode & 0o777 + end + + def test_retried_copy_preserves_an_existing_file_mode + existing = File.join(@tmpdir, "upload.txt") + File.write(existing, "stale") + File.chmod(0o640, existing) + upload = FakeUpload.new(filename: "notes.txt", content_type: "text/plain", content: "fresh") + message = FakeMessage.new(content: "summarize", files: AttachedFiles.new([ upload ])) + + Opencode::UploadedFilesPrompt.new( + user_message: message, + sandbox_path: @tmpdir, + sandbox_name_for: ->(_file) { "upload.txt" } + ) + + assert_equal 0o640, File.stat(existing).mode & 0o777 + end end diff --git a/test/readme_test.rb b/test/readme_test.rb index a5bfea6..70ebe7a 100644 --- a/test/readme_test.rb +++ b/test/readme_test.rb @@ -50,4 +50,23 @@ class ReadmeTest < Minitest::Test "`opencode-rails` #{Opencode::RAILS_VERSION} is a release candidate" assert_includes @readme, "pushing a `v*` tag does not guarantee publication" end + + def test_quickstart_uses_current_fail_closed_permission_rules + refute_match(/\{\s*type:/, @quickstart) + assert_includes @quickstart, '{ permission: "bash", pattern: "*", action: "deny" }' + assert_includes @quickstart, '{ permission: "external_directory", pattern: "*", action: "deny" }' + assert_includes @quickstart, '{ permission: "edit", pattern: "*", action: "deny" }' + assert_includes @quickstart, 'permission: "edit",' + assert_includes @quickstart, 'action: "allow"' + end + + def test_quickstart_uses_the_enqueued_user_message + assert_includes @quickstart, "def perform(assistant_message, user_message)" + refute_includes @quickstart, ".where(role: :user).last" + end + + def test_instrumentation_docs_match_the_configured_tracer_prefix + assert_includes @readme, "`assistant.response.started`" + assert_includes @readme, "Turn events flow through the injected tracer" + end end diff --git a/test/release_workflow_test.rb b/test/release_workflow_test.rb index 98c2466..86616f6 100644 --- a/test/release_workflow_test.rb +++ b/test/release_workflow_test.rb @@ -50,4 +50,15 @@ class ReleaseWorkflowTest < Minitest::Test assert_equal "${{ github.ref_name }}", preflight.dig("env", "RELEASE_TAG") assert_includes preflight.fetch("run"), "unless Opencode::RAILS_VERSION == expected" end + + def test_release_runs_the_test_suite_before_publish + steps = push_job.fetch("steps") + test_index = steps.index { |step| step["name"] == "Run tests" } + publish_index = steps.index { |step| step["uses"] == RELEASE_GEM_ACTION } + + refute_nil test_index + refute_nil publish_index + assert_operator test_index, :<, publish_index + assert_equal "bundle exec rake test", steps.fetch(test_index).fetch("run") + end end