Refactor log file creation

[selzoc]

No description provided.

[coderabbitai]

Warning

Rate limit exceeded

@selzoc has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 44 minutes and 19 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 159454f2-a574-44f6-8c90-75aa27142aca

📥 Commits

Reviewing files that changed from the base of the PR and between d9c03e6 and bd33553.

📒 Files selected for processing (7)

• src/bpm/commands/root.go
• src/bpm/integration/start_test.go
• src/bpm/runc/adapter/adapter.go
• src/bpm/runc/adapter/adapter_test.go
• src/bpm/safeio/safeio.go
• src/bpm/safeio/safeio_suite_test.go
• src/bpm/safeio/safeio_test.go

Walkthrough

This pull request introduces a new safeio package containing the OpenAppendChown function that safely performs privileged file operations on paths that may be controlled by less-privileged users. The function opens files in append mode with the O_NOFOLLOW flag to prevent symlink traversal, detects symlinks via ELOOP errors, and applies ownership changes using the file descriptor rather than the path. Existing code in setupBpmLogs and adapter.go is refactored to use this new function. Integration and unit tests are added to verify symlink detection and proper error handling across multiple code paths.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No description was provided, making it impossible to assess whether it relates to the changeset.	Add a pull request description explaining the purpose of the refactoring, security benefits, and testing approach for symlink handling.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: refactoring log file creation across multiple files to use a centralized safeio.OpenAppendChown function.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch symlink-updates

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 19 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bpm/runc/adapter/adapter.go (1)

188-199: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Close already-opened log files on later failure.

If OpenAppendChown succeeds for the first path and fails for the second, the first descriptor is leaked and the caller never sees it. Please clean up the earlier handle before returning.

♻️ Suggested fix

 func createLogFiles(bpmCfg *config.BPMConfig, user specs.User) (*os.File, *os.File, error) {
 	files := make([]*os.File, 2)
 	paths := []string{bpmCfg.Stdout().External(), bpmCfg.Stderr().External()}
 	for i, path := range paths {
 		f, err := safeio.OpenAppendChown(path, int(user.UID), int(user.GID), 0600)
 		if err != nil {
+			for _, opened := range files[:i] {
+				_ = opened.Close() //nolint:errcheck
+			}
 			return nil, nil, err
 		}
 		files[i] = f
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bpm/runc/adapter/adapter.go` around lines 188 - 199, In createLogFiles,
if safeio.OpenAppendChown succeeds for the first path but fails for the second,
the first file descriptor (files[0]) is leaked; update createLogFiles to close
any previously opened files before returning an error (i.e., after a failed
OpenAppendChown for paths[i], close any files[j] that were set earlier),
ensuring you still return nil, nil, err on failure and only return the two files
on complete success.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/bpm/runc/adapter/adapter.go`:
- Around line 188-199: In createLogFiles, if safeio.OpenAppendChown succeeds for
the first path but fails for the second, the first file descriptor (files[0]) is
leaked; update createLogFiles to close any previously opened files before
returning an error (i.e., after a failed OpenAppendChown for paths[i], close any
files[j] that were set earlier), ensuring you still return nil, nil, err on
failure and only return the two files on complete success.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ae0bb6f2-6578-409e-97b3-3f3e9d4b0abc

📥 Commits

Reviewing files that changed from the base of the PR and between e52220b and d9c03e6.

📒 Files selected for processing (7)

• src/bpm/commands/root.go
• src/bpm/integration/start_test.go
• src/bpm/runc/adapter/adapter.go
• src/bpm/runc/adapter/adapter_test.go
• src/bpm/safeio/safeio.go
• src/bpm/safeio/safeio_suite_test.go
• src/bpm/safeio/safeio_test.go

[Alphasite]

i think this looks ok?