Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions pkg/cli/logs_rate_limit.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ import (
var logsRateLimitLog = logger.New("cli:logs_rate_limit")
var fetchRateLimitFunc = fetchRateLimit

func contextCause(ctx context.Context) error {
if ctx == nil {
return nil
}
return context.Cause(ctx)
}

// rateLimitResponse models the JSON returned by `gh api rate_limit`.
// Only the "core" resource bucket is used because log downloads and
// workflow-run listing both draw from the core quota.
Expand Down Expand Up @@ -75,14 +82,18 @@ func fetchRateLimit() (rateLimitResource, error) {

// sleepWithContext pauses for duration d and returns nil when the timer fires.
// If ctx is cancelled before the timer expires, it stops the timer and returns
// ctx.Err() so callers can propagate cancellation immediately.
// context.Cause(ctx) so callers can propagate cancellation (and any wrapped
// cause) immediately.
func sleepWithContext(ctx context.Context, d time.Duration) error {
if ctx == nil {
ctx = context.Background()
var done <-chan struct{}
if ctx != nil {
done = ctx.Done()
}
// When ctx is nil, done remains nil. A nil channel is never selected, which
// intentionally makes cancellation checks a no-op and preserves prior behavior.
select {
case <-ctx.Done():
return ctx.Err()
case <-done:
return contextCause(ctx)
Comment thread
pelikhan marked this conversation as resolved.
Comment thread
pelikhan marked this conversation as resolved.
default:
}

Expand All @@ -91,8 +102,8 @@ func sleepWithContext(ctx context.Context, d time.Duration) error {
select {
case <-timer.C:
return nil
case <-ctx.Done():
return ctx.Err()
case <-done:
return contextCause(ctx)
}
}

Expand Down
41 changes: 28 additions & 13 deletions pkg/parser/remote_download_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,15 @@ func resolveRemoteSymlinks(ctx context.Context, client *api.RESTClient, owner, r

for i := 1; i < len(parts); i++ {
dirPath := strings.Join(parts[:i], "/")
resolvedPath, found, err := resolveRemoteSymlinkComponent(ctx, client, owner, repo, filePath, ref, parts, i, dirPath)
resolvedPath, found, err := resolveRemoteSymlinkComponent(ctx, client, remoteSymlinkComponentParams{
owner: owner,
repo: repo,
filePath: filePath,
ref: ref,
parts: parts,
index: i,
dirPath: dirPath,
})
if err != nil {
return "", err
}
Expand All @@ -223,39 +231,46 @@ func resolveRemoteSymlinks(ctx context.Context, client *api.RESTClient, owner, r
return "", fmt.Errorf("no symlinks found in path: %s", filePath)
}

type remoteSymlinkComponentParams struct {
owner string
repo string
filePath string
ref string
parts []string
index int
dirPath string
}

func resolveRemoteSymlinkComponent(
ctx context.Context,
client *api.RESTClient,
owner, repo, filePath, ref string,
parts []string,
index int,
dirPath string,
params remoteSymlinkComponentParams,
Comment thread
pelikhan marked this conversation as resolved.
) (string, bool, error) {
target, isSymlink, err := checkRemoteSymlink(ctx, client, owner, repo, dirPath, ref)
target, isSymlink, err := checkRemoteSymlink(ctx, client, params.owner, params.repo, params.dirPath, params.ref)
if err != nil {
if errorutil.IsNotFoundError(err) {
remoteLog.Printf("Path component %s returned 404, skipping", dirPath)
remoteLog.Printf("Path component %s returned 404, skipping", params.dirPath)
return "", false, nil
}
return "", false, fmt.Errorf("failed to check path component %s for symlinks: %w", dirPath, err)
return "", false, fmt.Errorf("failed to check path component %s for symlinks: %w", params.dirPath, err)
}
if !isSymlink {
return "", false, nil
}
parentDir := ""
if index > 1 {
parentDir = strings.Join(parts[:index-1], "/")
if params.index > 1 {
parentDir = strings.Join(params.parts[:params.index-1], "/")
}
resolvedBase, err := resolveAndValidateRemoteSymlinkBase(parentDir, target, dirPath)
resolvedBase, err := resolveAndValidateRemoteSymlinkBase(parentDir, target, params.dirPath)
if err != nil {
return "", false, err
}
remaining := strings.Join(parts[index:], "/")
remaining := strings.Join(params.parts[params.index:], "/")
resolvedPath := pathpkg.Clean(pathpkg.Join(resolvedBase, remaining))
if resolvedPath == "" || resolvedPath == "." || pathpkg.IsAbs(resolvedPath) || strings.HasPrefix(resolvedPath, "..") {
return "", false, fmt.Errorf("resolved symlink path escapes repository root: %s", resolvedPath)
}
remoteLog.Printf("Resolved symlink in remote path: %s -> %s (full: %s -> %s)", dirPath, target, filePath, resolvedPath)
remoteLog.Printf("Resolved symlink in remote path: %s -> %s (full: %s -> %s)", params.dirPath, target, params.filePath, resolvedPath)
return resolvedPath, true, nil
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/workflow/action_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func (c *ActionCache) marshalSorted() ([]byte, error) {

// Manually construct JSON with sorted keys
var result []byte
result = append(result, []byte("{\n \"entries\": {\n")...)
result = append(result, "{\n \"entries\": {\n"...)

for i, key := range keys {
entry := c.Entries[key]
Expand All @@ -284,7 +284,7 @@ func (c *ActionCache) marshalSorted() ([]byte, error) {
}

// Add the key and entry
result = append(result, []byte(" \""+key+"\": ")...)
result = append(result, " \""+key+"\": "...)
result = append(result, entryJSON...)

// Add comma if not the last entry
Expand All @@ -294,27 +294,27 @@ func (c *ActionCache) marshalSorted() ([]byte, error) {
result = append(result, '\n')
}

result = append(result, []byte(" }")...)
result = append(result, " }"...)

// Add containers section if non-empty
if len(c.ContainerPins) > 0 {
pinKeys := sliceutil.SortedKeys(c.ContainerPins)

result = append(result, []byte(",\n \"containers\": {\n")...)
result = append(result, ",\n \"containers\": {\n"...)
for i, k := range pinKeys {
pin := c.ContainerPins[k]
pinJSON, err := json.MarshalIndent(pin, " ", " ")
if err != nil {
return nil, err
}
result = append(result, []byte(" \""+k+"\": ")...)
result = append(result, " \""+k+"\": "...)
result = append(result, pinJSON...)
if i < len(pinKeys)-1 {
result = append(result, ',')
}
result = append(result, '\n')
}
result = append(result, []byte(" }")...)
result = append(result, " }"...)
}

result = append(result, '\n', '}')
Expand Down
12 changes: 10 additions & 2 deletions pkg/workflow/compiler_orchestrator_frontmatter.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ type frontmatterParseResult struct {
redirectTarget string
}

type frontmatterReadError struct {
message string
}

func (e frontmatterReadError) Error() string {
return e.message
}

func (c *Compiler) validateEngineBeforeSchema(
cleanPath string,
content []byte,
Expand Down Expand Up @@ -84,8 +92,8 @@ func (c *Compiler) parseFrontmatterSection(markdownPath string) (*frontmatterPar
content, err := os.ReadFile(cleanPath)
if err != nil {
orchestratorFrontmatterLog.Printf("Failed to read file: %s, error: %v", cleanPath, err)
// Intentionally not wrapping to avoid exposing internal path details
return nil, fmt.Errorf("failed to read file: %v", err) //nolint:errorlint // intentionally not wrapping to avoid exposing os.PathError
// Keep the user-facing message while avoiding exposure of os.PathError internals.
return nil, fmt.Errorf("failed to read file: %w", frontmatterReadError{message: err.Error()})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/codebase-design] `frontmatterReadError` is a value type (not a pointer), has no exported fields, and is not accessible outside this package — so callers can never do `errors.As(err, &frontmatterReadError{})` to discriminate read failures from parse failures. The wrapping via `%w` satisfies the linter, but the wrapped sentinel adds no practical information over the raw error string.

💡 Alternative to consider

If the goal is lint compliance without an intermediate sentinel, a simple fmt.Errorf around the message string already breaks the os.PathError chain:

return nil, fmt.Errorf("failed to read file: %s", err.Error())

If frontmatterReadError is intended to be testable via errors.As, it needs to be exported and the test in TestParseFrontmatterSection_FileReadError should assert on it. As-is, the type is unreachable by callers.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The %w wrapping of frontmatterReadError means callers can now do errors.As(err, &frontmatterReadError{}) — but frontmatterReadError doesn't implement Unwrap(), so errors.Is/errors.As traversal stops at the outer fmt.Errorf wrapper and won't reach the underlying *os.PathError.

That's the intended behaviour (hiding os.PathError), but it's subtle. Consider adding an explicit // Unwrap is intentionally not implemented comment inside frontmatterReadError, or at minimum a comment on this line noting that the chain stops here. Without it a future maintainer may add Unwrap() thinking it's an oversight.

@copilot please address this.

}
contentString := string(content)

Expand Down
Loading