Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Bosh::Director::Blobstore::UuidValidation module and enforces UUID-format validation for blobstore object IDs in agent client logic, LogsFetcher, LocalClient, and package compilation. Introduces two new error codes for invalid blobstore IDs. Replaces several tar extraction calls to use Open3.capture3. Normalizes manifest 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rb (1)
490-514:⚠️ Potential issue | 🟠 MajorKeep the parsed and raw manifest consistent in
/diff.This rewrites
manifest_hash['name'], butManifest.load_from_hashon Lines 507-514 still receives the original request body. The loader is now given two different versions of the same manifest whenever the submitted YAML names another deployment. Rebuildmanifest_textafter the rewrite so the diff path operates on a single consistent manifest.💡 Proposed fix
begin manifest_text = request.body.read manifest_hash = validate_manifest_yml(manifest_text) - manifest_hash['name'] = deployment.name if deployment + if deployment + manifest_hash['name'] = deployment.name + manifest_text = YAML.dump(manifest_hash) + end if deployment🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rb` around lines 490 - 514, The manifest name is mutated by setting manifest_hash['name'] but manifest_text still contains the original request body, causing inconsistent input to Manifest.load_from_hash and the diff; after validating and mutating manifest_hash (in this controller block where validate_manifest_yml, manifest_hash, and deployment are used), regenerate manifest_text from the updated manifest_hash (e.g. serialize the updated hash back to YAML/UTF-8) before calling Manifest.load_from_hash so Manifest.load_from_hash and the subsequent diff operate on the same, updated manifest data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb`:
- Around line 164-170: The method validate_compiled_package_blobstore_id! should
defensively handle non-Hash or missing task_result values before indexing:
ensure task_result is a Hash (or use safe lookup) and that a 'blobstore_id'
exists, then call Blobstore::UuidValidation.valid_uuid?; if task_result is nil,
not a Hash, or the id is missing/invalid, raise
PackageCompilationInvalidTaskBlobstoreId so the validation path is consistent.
Update validate_compiled_package_blobstore_id! to explicitly check
task_result.is_a?(Hash) (or use task_result&.[] / dig safely) and treat any
other case as invalid, referencing the method name
validate_compiled_package_blobstore_id!, the exception
PackageCompilationInvalidTaskBlobstoreId, and
Blobstore::UuidValidation.valid_uuid? to locate the change.
In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb`:
- Around line 75-81: The log message in the error path after Open3.capture3
contains an extra closing parenthesis; update the logger.error call (the string
built using release_path, release_dir, status.exitstatus, and combined) to
remove the stray ')' at the end of the message so the string punctuation is
correct (check the logger.error invocation near Open3.capture3 and the variables
release_path, release_dir, status, combined).
In
`@src/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rb`:
- Around line 2738-2742: The test currently only checks for a 302 redirect but
must also assert that the queued manifest uses the URL deployment name; modify
the spec around the put '/owned_deployment/jobs/dea?state=recreate' call to set
an expectation on the DeploymentManager create method (e.g.
expect(DeploymentManager).to
receive(:create_deployment).with(hash_including('name' => 'owned_deployment'))
or the equivalent instance method spy used in this suite) and then perform the
request so the spec verifies the manifest argument's 'name' was rewritten to
'owned_deployment' rather than left as 'other_deployment'.
In `@src/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rb`:
- Line 44: The timestamp arithmetic in the spec uses ambiguous operator
precedence (e.g. Time.now - 12*24*60*60) which triggers
Lint/AmbiguousOperatorPrecedence; update each occurrence (the FactoryBot.create
calls that set variables like old_log_bundle and the other two timestamped
factories) to parenthesize the multiplication part (e.g. Time.now -
(12*24*60*60)) so the subtraction is unambiguous.
---
Outside diff comments:
In
`@src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rb`:
- Around line 490-514: The manifest name is mutated by setting
manifest_hash['name'] but manifest_text still contains the original request
body, causing inconsistent input to Manifest.load_from_hash and the diff; after
validating and mutating manifest_hash (in this controller block where
validate_manifest_yml, manifest_hash, and deployment are used), regenerate
manifest_text from the updated manifest_hash (e.g. serialize the updated hash
back to YAML/UTF-8) before calling Manifest.load_from_hash so
Manifest.load_from_hash and the subsequent diff operate on the same, updated
manifest data.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94ccf45b-c9cf-4864-9184-4f6d54694ee3
📒 Files selected for processing (25)
.gitignoresrc/bosh-director/lib/bosh/director/agent_client.rbsrc/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rbsrc/bosh-director/lib/bosh/director/blobstore/local_client.rbsrc/bosh-director/lib/bosh/director/blobstore/uuid_validation.rbsrc/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rbsrc/bosh-director/lib/bosh/director/errors.rbsrc/bosh-director/lib/bosh/director/jobs/release/release_job.rbsrc/bosh-director/lib/bosh/director/jobs/update_release.rbsrc/bosh-director/lib/bosh/director/jobs/update_release/package_persister.rbsrc/bosh-director/lib/bosh/director/jobs/update_stemcell.rbsrc/bosh-director/lib/bosh/director/logs_fetcher.rbsrc/bosh-director/spec/gemspec_spec.rbsrc/bosh-director/spec/unit/bosh/director/agent_client_spec.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/blobstore/local_client_spec.rbsrc/bosh-director/spec/unit/bosh/director/blobstore/uuid_validation_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/release/release_job_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_release/package_persister_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_release_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/logs_fetcher_spec.rbsrc/spec/integration/release/upload_release_spec.rb
github-project-automation
bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rb`:
- Around line 69-71: Extract the validate → overwrite name → dump sequence into
a single helper (e.g., a private controller method
normalize_manifest_name(manifest_text, deployment_name)) that calls
validate_manifest_yml, sets manifest_hash['name'] = deployment_name, and returns
YAML.dump(manifest_hash); then replace the inline sequences (the current calls
using validate_manifest_yml, manifest_hash['name'] = deployment.name,
YAML.dump(...)) in the job-update endpoints and the diff endpoint with calls to
this new helper (use validate_manifest_yml, deployment.name, and YAML.dump only
inside the helper to centralize name normalization).
In
`@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb`:
- Around line 135-136: The code accesses agent_task['result'] as if it were a
Hash before calling validate_compiled_package_blobstore_id!, which allows a
non-Hash/malformed signed-URL task result to raise NoMethodError earlier; change
the task-result handling so you first assert the type/structure of
agent_task['result'] (e.g., ensure it is a Hash or respond_to?(:[])) before any
key access, and/or call validate_compiled_package_blobstore_id! before
dereferencing agent_task['result'] keys; update the logic around
agent_task['result'] and the validate_compiled_package_blobstore_id! invocation
to raise the intended validation error for malformed results instead of letting
NoMethodError bubble up.
In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb`:
- Around line 75-80: The extraction call using Open3.capture3('tar', '-C',
release_dir, '-xf', release_path) must be wrapped in begin/rescue/ensure to
handle Errno::ENOENT and ensure release_dir is always removed; catch
Errno::ENOENT (and any other StandardError if desired), log the error via
logger.error including the exception message and the release_path/release_dir,
raise ReleaseInvalidArchive, and move FileUtils.rm_rf(release_dir) into an
ensure block so the temp dir is cleaned whether capture3 fails, returns nonzero
status, or raises.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50c29c5e-032d-4e1e-aad5-8f7e58d0b719
📒 Files selected for processing (5)
src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rbsrc/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rbsrc/bosh-director/lib/bosh/director/jobs/update_release.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rb
aramprice
force-pushed
the
blobstore-id-handling
branch
from
ae20e2f to
96e307a
Compare
mkocher
previously approved these changes
Apr 15, 2026
aramprice
force-pushed
the
blobstore-id-handling
branch
from
96e307a to
3fb4a91
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb (1)
119-121:⚠️ Potential issue | 🟠 MajorDefensively validate
task_resultshape before any key access.Line 121 and Line 135 still assume
agent_task['result']is aHash; malformed agent responses can still raiseNoMethodErrorbeforePackageCompilationInvalidTaskBlobstoreIdis raised.Proposed fix
@@ - task_result = agent_task['result'] - task_result['blobstore_id'] = compiled_package_blobstore_id + task_result = agent_task['result'] + unless task_result.is_a?(Hash) + raise PackageCompilationInvalidTaskBlobstoreId, + 'Compilation task result contained an invalid blobstore object id' + end + task_result['blobstore_id'] = compiled_package_blobstore_id @@ - task_result = agent_task['result'] + task_result = agent_task['result'] + unless task_result.is_a?(Hash) + raise PackageCompilationInvalidTaskBlobstoreId, + 'Compilation task result contained an invalid blobstore object id' + end @@ - validate_compiled_package_blobstore_id!(task_result['blobstore_id']) + validate_compiled_package_blobstore_id!(task_result['blobstore_id'])Also applies to: 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb` around lines 119 - 121, The code assumes agent_task['result'] is a Hash and directly accesses task_result[...] which can raise NoMethodError for malformed responses; update the compile flow around agent_task = instance.agent_client.compile_package_with_signed_url(request) and the subsequent task_result usage to defensively validate the shape (e.g., confirm agent_task is a Hash and agent_task['result'] is a Hash) before setting task_result['blobstore_id'] or reading keys later, and raise or convert to PackageCompilationInvalidTaskBlobstoreId with a clear message when the shape is invalid so downstream key access is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rb`:
- Around line 820-823: The test is passing a Hash to the validator but the
method validate_compiled_package_blobstore_id! expects a blobstore ID string;
change the spec invocation (currently calling
compiler.send(:validate_compiled_package_blobstore_id!, { 'blobstore_id' =>
'bogus-id' })) to pass the string 'bogus-id' directly so the test exercises UUID
validation properly and still expects PackageCompilationInvalidTaskBlobstoreId
to be raised.
In `@src/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rb`:
- Around line 143-157: The test uses a hardcoded 'common-blobstore-id' for the
shared bundle while per-instance temp bundles use UUIDs; update the mock for
blobstore.create in the fetch_logs spec to return a UUID-formatted string (e.g.
'cccccccc-cccc-4ccc-8ccc-cccccccc0000') instead of 'common-blobstore-id' so the
test aligns with UUID standardization—modify the setup where blobstore.create is
stubbed for the fetch_logs example (referencing fetch_logs.perform and
Models::LogBundle checks) to return the UUID string.
---
Duplicate comments:
In
`@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb`:
- Around line 119-121: The code assumes agent_task['result'] is a Hash and
directly accesses task_result[...] which can raise NoMethodError for malformed
responses; update the compile flow around agent_task =
instance.agent_client.compile_package_with_signed_url(request) and the
subsequent task_result usage to defensively validate the shape (e.g., confirm
agent_task is a Hash and agent_task['result'] is a Hash) before setting
task_result['blobstore_id'] or reading keys later, and raise or convert to
PackageCompilationInvalidTaskBlobstoreId with a clear message when the shape is
invalid so downstream key access is safe.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed228c78-8013-41fd-99a4-1f181e7a8005
📒 Files selected for processing (6)
src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rbsrc/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rbsrc/bosh-director/lib/bosh/director/jobs/update_release.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rb
Run tar via argv lists instead of shell strings for release, package, job, and stemcell extraction paths. Validate release manifest identifiers against existing model rules during normalization. For deployments, authorize updates against the deployment record when the name already exists, and set manifest name from the URL deployment on PUT and diff so the requested deployment matches the payload. - update integrtion spec to check manifest validation error for malformed upload release since the task output no longer contains the Sequel validation Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb (1)
120-121:⚠️ Potential issue | 🟠 MajorGuard malformed
agent_task['result']before hash indexing.Line 121 and Line 135 can still raise
NoMethodErrorwhenagent_task['result']is nil/non-Hash, bypassingPackageCompilationInvalidTaskBlobstoreId. Route malformed results through the typed validation path.Proposed fix
- validate_compiled_package_blobstore_id!(task_result['blobstore_id']) + validate_compiled_package_blobstore_id!(task_result) @@ - def validate_compiled_package_blobstore_id!(blobstore_id) + def validate_compiled_package_blobstore_id!(task_result) + blobstore_id = task_result.is_a?(Hash) ? task_result['blobstore_id'] : nil return if Blobstore::UuidValidation.valid_uuid?(blobstore_id) raise PackageCompilationInvalidTaskBlobstoreId, 'Compilation task result contained an invalid blobstore object id' endAlso applies to: 135-135, 164-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb` around lines 120 - 121, agent_task['result'] may be nil or not a Hash which causes NoMethodError when indexing; before doing task_result['blobstore_id'] = compiled_package_blobstore_id (and any other accesses to task_result), validate that agent_task['result'] is a Hash (e.g. unless agent_task['result'].is_a?(Hash) then call the existing typed validation path or raise PackageCompilationInvalidTaskBlobstoreId) and only then assign/read keys; apply the same guard to all other spots that index task_result (references: agent_task, task_result, compiled_package_blobstore_id, and the PackageCompilationInvalidTaskBlobstoreId validation path).src/bosh-director/lib/bosh/director/jobs/update_release.rb (1)
75-80:⚠️ Potential issue | 🟠 MajorHandle
capture3exceptions so failed extractions do not leak temp dirs.If
tarcannot be spawned,Open3.capture3raises beforerelease_diris returned, so theperformensure block never sees that temp dir to clean it up. That also exposes a raw system exception instead ofReleaseInvalidArchive.🔧 Minimal fix sketch
def extract_release release_dir = Dir.mktmpdir + extracted = false - out, err, status = Open3.capture3('tar', '-C', release_dir, '-xf', release_path) - combined = [out, err].map(&:to_s).join - if status.exitstatus != 0 - logger.error("Failed to extract release archive '#{release_path}' into dir '#{release_dir}', tar returned #{status.exitstatus}, output: #{combined}") - FileUtils.rm_rf(release_dir) - raise ReleaseInvalidArchive, 'Extracting release archive failed. Check task debug log for details.' - end + begin + out, err, status = Open3.capture3('tar', '-C', release_dir, '-xf', release_path) + combined = [out, err].map(&:to_s).join + if status.exitstatus != 0 + logger.error("Failed to extract release archive '#{release_path}' into dir '#{release_dir}', tar returned #{status.exitstatus}, output: #{combined}") + raise ReleaseInvalidArchive, 'Extracting release archive failed. Check task debug log for details.' + end + extracted = true + rescue Errno::ENOENT => e + logger.error("Failed to extract release archive '#{release_path}' into dir '#{release_dir}', tar execution error: #{e.message}") + raise ReleaseInvalidArchive, 'Extracting release archive failed. Check task debug log for details.' + ensure + FileUtils.rm_rf(release_dir) unless extracted + end release_dir end#!/bin/bash set -euo pipefail echo "Inspect the current extraction path and cleanup handling:" rg -n -C4 "def extract_release|Open3\.capture3|FileUtils\.rm_rf\(release_dir\)|ReleaseInvalidArchive" src/bosh-director/lib/bosh/director/jobs/update_release.rb echo echo "Confirm there is no local rescue/ensure around capture3:" rg -n -C2 "rescue Errno::ENOENT|ensure|Open3\.capture3" src/bosh-director/lib/bosh/director/jobs/update_release.rb🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb` around lines 75 - 80, Open3.capture3 can raise if tar cannot be spawned, causing the temp release_dir to leak and a raw exception to surface; wrap the capture3 call in a begin...rescue that catches spawn/system errors (e.g., StandardError/Errno::*) around the Open3.capture3(...) invocation inside the extract_release (or the method containing that code), ensure FileUtils.rm_rf(release_dir) is called in the rescue (or an inner ensure) so the temp dir is removed on failure, and then raise ReleaseInvalidArchive with a descriptive message including the original error (preserve error details) instead of letting the raw exception propagate. Ensure existing status-check path still logs and raises ReleaseInvalidArchive as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb`:
- Around line 120-121: agent_task['result'] may be nil or not a Hash which
causes NoMethodError when indexing; before doing task_result['blobstore_id'] =
compiled_package_blobstore_id (and any other accesses to task_result), validate
that agent_task['result'] is a Hash (e.g. unless
agent_task['result'].is_a?(Hash) then call the existing typed validation path or
raise PackageCompilationInvalidTaskBlobstoreId) and only then assign/read keys;
apply the same guard to all other spots that index task_result (references:
agent_task, task_result, compiled_package_blobstore_id, and the
PackageCompilationInvalidTaskBlobstoreId validation path).
In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb`:
- Around line 75-80: Open3.capture3 can raise if tar cannot be spawned, causing
the temp release_dir to leak and a raw exception to surface; wrap the capture3
call in a begin...rescue that catches spawn/system errors (e.g.,
StandardError/Errno::*) around the Open3.capture3(...) invocation inside the
extract_release (or the method containing that code), ensure
FileUtils.rm_rf(release_dir) is called in the rescue (or an inner ensure) so the
temp dir is removed on failure, and then raise ReleaseInvalidArchive with a
descriptive message including the original error (preserve error details)
instead of letting the raw exception propagate. Ensure existing status-check
path still logs and raises ReleaseInvalidArchive as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c01f308d-de61-4676-8bee-cdab9ed467ce
📒 Files selected for processing (6)
src/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rbsrc/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rbsrc/bosh-director/lib/bosh/director/jobs/update_release.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rb
github-project-automation
bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
Add shared UuidValidation helper matching SecureRandom.uuid format. AgentClient only fetches ephemeral blobs when the id is a well-formed UUID; invalid ids are logged and skipped. LocalClient restricts filesystem paths to UUID object names. Reject non-UUID ids returned when fetching logs without signed URLs and when persisting compiled package task results without signed URLs. Introduce AgentTaskInvalidBlobstoreId for log fetch responses and PackageCompilationInvalidTaskBlobstoreId for compile task results. Update unit tests for AgentClient, LocalClient, and the new helper. Made-with: Cursor
aramprice
force-pushed
the
blobstore-id-handling
branch
from
3fb4a91 to
e5719a2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/bosh-director/lib/bosh/director/jobs/update_release.rb (1)
75-80:⚠️ Potential issue | 🟠 MajorExtraction path still misses exception-safe cleanup/error mapping.
If
Open3.capture3raises (e.g., tar not executable), this bypassesReleaseInvalidArchivemapping and may leaverelease_dirbehind.#!/bin/bash set -euo pipefail # Verify extract_release error-handling structure around Open3.capture3 rg -n -C5 "def extract_release|Open3\.capture3|ReleaseInvalidArchive|FileUtils\.rm_rf\(release_dir\)|rescue|ensure" src/bosh-director/lib/bosh/director/jobs/update_release.rbExpected verification:
Open3.capture3should be wrapped with rescue/ensure sorelease_dircleanup andReleaseInvalidArchivemapping happen for raised exceptions too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb` around lines 75 - 80, The extract_release code calls Open3.capture3 directly so if capture3 raises (e.g., tar not found) the FileUtils.rm_rf(release_dir) and the ReleaseInvalidArchive mapping are skipped; wrap the capture3 call in a begin/rescue/ensure inside the extract_release method so any exception is rescued and re-raised (or wrapped) as ReleaseInvalidArchive (including original error details in the log), and ensure FileUtils.rm_rf(release_dir) runs in ensure to always clean up the release_dir; reference Open3.capture3, logger.error, FileUtils.rm_rf(release_dir), and the ReleaseInvalidArchive class when making the change.src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb (1)
119-122:⚠️ Potential issue | 🟠 Major
task_resultcan still crash before typed blobstore-id validation.If
agent_task['result']is nil/non-Hash, the hash dereference path throwsNoMethodErrorand bypassesPackageCompilationInvalidTaskBlobstoreId.Suggested defensive fix
agent_task = instance.agent_client.compile_package_with_signed_url(request) { Config.job_cancelled? } task_result = agent_task['result'] + unless task_result.is_a?(Hash) + raise PackageCompilationInvalidTaskBlobstoreId, + 'Compilation task result contained an invalid blobstore object id' + end task_result['blobstore_id'] = compiled_package_blobstore_id else agent_task = instance.agent_client.compile_package( package.blobstore_id, @@ ) { Config.job_cancelled? } task_result = agent_task['result'] end end + unless task_result.is_a?(Hash) + raise PackageCompilationInvalidTaskBlobstoreId, + 'Compilation task result contained an invalid blobstore object id' + end validate_compiled_package_blobstore_id!(task_result['blobstore_id'])Also applies to: 135-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb` around lines 119 - 122, The code assumes agent_task['result'] is a Hash and directly writes task_result['blobstore_id'], which will raise NoMethodError if agent_task['result'] is nil or not a Hash; in the package_compile_stage (around agent_task = instance.agent_client.compile_package_with_signed_url(...)) validate that agent_task and agent_task['result'] are present and are a Hash (e.g., check task_result = agent_task['result'].is_a?(Hash) ? agent_task['result'] : nil) before assigning task_result['blobstore_id']; if validation fails, raise PackageCompilationInvalidTaskBlobstoreId with a clear message instead of dereferencing, and apply the same guard logic to the other occurrence at the later block (lines ~135-136).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rb`:
- Around line 119-122: The code assumes agent_task['result'] is a Hash and
directly writes task_result['blobstore_id'], which will raise NoMethodError if
agent_task['result'] is nil or not a Hash; in the package_compile_stage (around
agent_task = instance.agent_client.compile_package_with_signed_url(...))
validate that agent_task and agent_task['result'] are present and are a Hash
(e.g., check task_result = agent_task['result'].is_a?(Hash) ?
agent_task['result'] : nil) before assigning task_result['blobstore_id']; if
validation fails, raise PackageCompilationInvalidTaskBlobstoreId with a clear
message instead of dereferencing, and apply the same guard logic to the other
occurrence at the later block (lines ~135-136).
In `@src/bosh-director/lib/bosh/director/jobs/update_release.rb`:
- Around line 75-80: The extract_release code calls Open3.capture3 directly so
if capture3 raises (e.g., tar not found) the FileUtils.rm_rf(release_dir) and
the ReleaseInvalidArchive mapping are skipped; wrap the capture3 call in a
begin/rescue/ensure inside the extract_release method so any exception is
rescued and re-raised (or wrapped) as ReleaseInvalidArchive (including original
error details in the log), and ensure FileUtils.rm_rf(release_dir) runs in
ensure to always clean up the release_dir; reference Open3.capture3,
logger.error, FileUtils.rm_rf(release_dir), and the ReleaseInvalidArchive class
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f65ea6e-d571-4353-9b76-2f36ca21d3b3
📒 Files selected for processing (24)
src/bosh-director/lib/bosh/director/agent_client.rbsrc/bosh-director/lib/bosh/director/api/controllers/deployments_controller.rbsrc/bosh-director/lib/bosh/director/blobstore/local_client.rbsrc/bosh-director/lib/bosh/director/blobstore/uuid_validation.rbsrc/bosh-director/lib/bosh/director/deployment_plan/stages/package_compile_stage.rbsrc/bosh-director/lib/bosh/director/errors.rbsrc/bosh-director/lib/bosh/director/jobs/release/release_job.rbsrc/bosh-director/lib/bosh/director/jobs/update_release.rbsrc/bosh-director/lib/bosh/director/jobs/update_release/package_persister.rbsrc/bosh-director/lib/bosh/director/jobs/update_stemcell.rbsrc/bosh-director/lib/bosh/director/logs_fetcher.rbsrc/bosh-director/spec/gemspec_spec.rbsrc/bosh-director/spec/unit/bosh/director/agent_client_spec.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/deployments_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/blobstore/local_client_spec.rbsrc/bosh-director/spec/unit/bosh/director/blobstore/uuid_validation_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/fetch_logs_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/release/release_job_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_release/package_persister_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_release_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/logs_fetcher_spec.rbsrc/spec/integration/release/upload_release_spec.rb
github-project-automation
bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
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
3 participants
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.
What is this change about?
Makes bosh's handling of UUIDs more consistent