add lint workflow and fix lint issue by leslie-qiwa · Pull Request #212 · ChristopherHX/github-act-runner

leslie-qiwa · 2025-08-13T17:50:19Z

total 394 lint issues discovered by the tool. 95% fix is done by copilot , including several bugs found by lint

Run method in actionsrunner/runner.go

actionsrunner/runner.go:147:3: deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop (gocritic)
defer cancelJobListening()

io.Read/Write err not check: ignore most by AI but manually add logging at several places
3 files were DOS/Windows format.

protocol/results/contracts.go: ASCII text, with CRLF line terminators
protocol/results/service.go:   ASCII text, with CRLF line terminators
runnersurvey.go:               ASCII text, with CRLF line terminators

leslie-qiwa · 2025-08-13T17:52:22Z

@ChristopherHX I really like the project, and hope to use it in our project. Understand this is a big PR, but I think it is needed to guard each PR.

ChristopherHX

Hi,

The most dangerous part is changing the protocol structure names / saved json configuration fields.

if you add json tags
```
 `json:"myUrl"
```
means myURL is no longer accepted by the json parser
not adding this means
unexpected naming convention used in writing, causes problems with my other software interacting with this project

unresolved problem

ChristopherHX · 2025-08-13T18:56:42Z

+	TarballURL            string
 	Ref                   string
-	ZipballUrl            string
+	ZipballURL            string


Dangerous JSON Api public, potential problem when creating json from golang

Used by me in https://github.com/ChristopherHX/gitea-actions-runner/blob/343a633c06f22890e8133b8c999c567083ef1384/actions/server/server.go#L220

Impact seems to be minor, since this is legacy protocol and c# is case insensitive

Got you, didn't know the relationship. I can have another PR to fix that repo too if needed. :-)

ChristopherHX · 2025-08-13T19:01:20Z

During reading I understood almost every rename is fine, e.g. have a json tag to preserve binary compat

ChristopherHX · 2025-08-13T19:08:04Z

+	_, xmlErr := ToXMLString(&ret.PKey.PublicKey)
+	if xmlErr != nil {
+		fmt.Printf("convert xml string: %v", xmlErr)
+	}


this was dead code experimental code from myself

we may do another cleanup next time.

ChristopherHX · 2025-08-13T19:23:49Z

 }

-func (logger *JobLogger) Append(record protocol.TimelineRecord) *protocol.TimelineRecord {
+func (logger *JobLogger) Append(record *protocol.TimelineRecord) *protocol.TimelineRecord {


Breaking change api

yes, this is one I forgot the mention in description. It is also discovered by lint tool with below warning

protocol/logger/job_logger.go:415:33: hugeParam: record is heavy (352 bytes); consider passing it by pointer (gocritic) func (logger *JobLogger) Append(record protocol.TimelineRecord) *protocol.TimelineRecord {

thus, I made the change. Didn't realize it is also used by other repos. Let me know if you want to keep my current change. If yes, I can change other repo too if needed, otherwise, I can revert the change and add some nolint comment. But in my opinion, seems original copy may not be efficient in long live production environment.

ChristopherHX · 2025-08-13T19:23:58Z

 }

-func (logger *JobLogger) Insert(record protocol.TimelineRecord) *protocol.TimelineRecord {
+func (logger *JobLogger) Insert(record *protocol.TimelineRecord) *protocol.TimelineRecord {


Breaking change api

same as above

ChristopherHX · 2025-08-13T19:37:41Z

Generally yes, I think this can be merged even if it is big. The change rate of this project has declined so, we could make this short and painless

@leslie-qiwa In what way do you want to use this project?

As a...

runner application
runner go library
something else

Normally I would decide that potential breaking changes should move to 1.0.0 milestone
Now my milestone moved months after months due to still being in a hybrid actions protocol mode.

leslie-qiwa · 2025-08-14T05:48:28Z

@ChristopherHX Thanks for quick review. I would use it as a runner application, this is why I'm having this PR as I may create more PRs to improve stability, debug / logging, etc. We have a relative large golang based CI system, now need a self host runner to integrate with it and communicate with our internal resources. Thus your project is perfectly matching our need. :-)

leslie-qiwa · 2025-08-14T05:51:35Z

During reading I understood almost every rename is fine, e.g. have a json tag to preserve binary compat

yes, that is why I'm making the change as json marshall/unmarshal should remain same, only public API need adapt the change.

leslie-qiwa · 2025-08-14T06:06:03Z

Normally I would decide that potential breaking changes should move to 1.0.0 milestone Now my milestone moved months after months due to still being in a hybrid actions protocol mode.

The later the CI has static analysis, the more issues people need fix and deal with. :-) So better sooner than later imo.

ChristopherHX · 2025-08-14T15:58:44Z

Some change from your PR causes my runner CI to fail, stricter permissions? This is a mandatory CI check.

This readme change pr shows https://github.com/ChristopherHX/github-act-runner/actions/runs/16970040883/job/48104403780?pr=213 that CI passes today and this PR always failed due to permission denied

ChristopherHX · 2025-08-14T16:00:31Z

this is your CI result: https://github.com/ChristopherHX/github-act-runner/actions/runs/16945165477/job/48033615340?pr=212#step:6:1277

##[warning]github-act-runner is be unable to access "/home/runner/.cache/act". You might want set one of the following environment variables XDG_CACHE_HOME, HOME to a user read and writeable location. Details: mkdir /home/runner/.cache/act: permission denied
| Starting nektos/act
| ##[error]mkdir /home/runner/.cache/act: permission denied

Running a non dockerized job after a docker job, or whatever the reason is.

ChristopherHX · 2025-08-14T16:03:05Z

I would merge this PR as soon as CI / run-tests (pull_request) is satisfied.

All other things are minor

ChristopherHX · 2025-08-14T16:13:38Z

+	maxRedirects = 10
+	jobTimeout   = 5 * time.Minute
+	// File permissions
+	directoryPermissions = 0664


BUG Copilot revealed the fatal error 0775 is expected

Suggested change

directoryPermissions = 0664

directoryPermissions = 0777

It is not known if such a change will impact the runner

Suggested change

directoryPermissions = 0664

directoryPermissions = 0775

However folders need execute rights for enumerating it's content

The cause of the new error is the change in access rights when creating the cache directory:

Before the PR, os.MkdirAll(cacheDir, 0777) was probably used (full rights for everyone).
Now, os.MkdirAll(cacheDir, 0664) is used (no execute permission for directories, and no write permission for “others”).

This is a bug:
Directories require the execute (x) permission, otherwise no one (not even the owner) can enter the directory or create files in it.
0664 makes sense for files, but for directories you MUST use at least 0775 or 0777.

Conclusion:
The error was introduced in the PR by changing os.MkdirAll(cacheDir, 0777) to os.MkdirAll(cacheDir, 0664).
This is the cause of the new “permission denied” when creating ~/.cache/act.

you are right. I just pushed new change, and will see the result.

total 394 lint issues discovered by the tool. 95% fix is done by copilot , including several bugs found by lint 1. Run method in actionsrunner/runner.go actionsrunner/runner.go:147:3: deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop (gocritic) defer cancelJobListening() 2. io.Read/Write err not check: ignore most by AI but manually add logging at several places Signed-off-by: Leslie Qi Wang <qiwa@pensando.io>

leslie-qiwa · 2025-08-14T16:58:19Z

my installed golangci-lint tool version seems not new as the one used in action, which identified more issues. Let me upgrade and fix.

ChristopherHX · 2025-08-14T17:09:00Z

As far I know golang ci lint is at v2.x already https://github.com/golangci/golangci-lint/releases

Resolved

leslie-qiwa · 2025-08-14T19:08:15Z

As far I know golang ci lint is at v2.x already https://github.com/golangci/golangci-lint/releases

yes, it need use v8.0 of golangci/golangci-lint-action and may discover more lint issues. Maybe merge this PR, and I can have another one to fix new issues identified. One complexity tool gocyclo is very useful. It is not included in current PR. I'm planning to add in next one with the latest golangci-lint tool

ChristopherHX · 2025-08-14T19:18:21Z

well now you broke almost all 32bit targets

ChristopherHX · 2025-08-14T22:30:33Z

You know that len(x) of type int at least for 32bit?

casting max uint to int is an overflow or some other complaint

Do this in a bash shell to get the errors locally
GOOS=linux GOARCH=386 go build

Signed-off-by: Leslie Qi Wang <qiwa@pensando.io>

leslie-qiwa · 2025-08-15T17:30:48Z

solved some errors introduced by AI auto fix, but seems more. Just pushed a new change, I used GOARCH=386 to verify and build is success. We can see if some other compilation error.

ChristopherHX · 2025-08-15T18:24:14Z

CI is passing now, ready for merge?

leslie-qiwa · 2025-08-15T19:11:01Z

CI is passing now, ready for merge?

yes, thanks.

leslie-qiwa · 2025-08-15T19:11:59Z

I'll have another PR to upgrade golangci-lint, and enable more lint.

ChristopherHX reviewed Aug 13, 2025

View reviewed changes

ChristopherHX previously requested changes Aug 14, 2025

View reviewed changes

leslie-qiwa force-pushed the lint branch from d5ea73e to 81419ac Compare August 14, 2025 16:41

leslie-qiwa force-pushed the lint branch from 81419ac to 62fc68f Compare August 14, 2025 16:43

leslie-qiwa force-pushed the lint branch from 475f365 to 41e9f60 Compare August 14, 2025 22:13

leslie-qiwa force-pushed the lint branch from 41e9f60 to c6efee1 Compare August 14, 2025 23:39

fix another 39 errors by upgrading golangci-lint

d4fc13a

Signed-off-by: Leslie Qi Wang <qiwa@pensando.io>

leslie-qiwa force-pushed the lint branch from c6efee1 to d4fc13a Compare August 15, 2025 17:24

ChristopherHX approved these changes Aug 15, 2025

View reviewed changes

ChristopherHX merged commit b291e34 into ChristopherHX:main Aug 15, 2025
46 checks passed

Uh oh!

Conversation

leslie-qiwa commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leslie-qiwa commented Aug 13, 2025

Uh oh!

ChristopherHX left a comment

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherHX commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leslie-qiwa Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherHX commented Aug 13, 2025

Uh oh!

leslie-qiwa commented Aug 14, 2025

Uh oh!

leslie-qiwa commented Aug 14, 2025

Uh oh!

leslie-qiwa commented Aug 14, 2025

Uh oh!

ChristopherHX commented Aug 14, 2025

Uh oh!

ChristopherHX commented Aug 14, 2025

Uh oh!

ChristopherHX commented Aug 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leslie-qiwa commented Aug 14, 2025

Uh oh!

ChristopherHX commented Aug 14, 2025

Uh oh!

leslie-qiwa commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristopherHX commented Aug 14, 2025

Uh oh!

ChristopherHX commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leslie-qiwa commented Aug 15, 2025

Uh oh!

ChristopherHX commented Aug 15, 2025

Uh oh!

leslie-qiwa commented Aug 15, 2025

Uh oh!

leslie-qiwa commented Aug 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leslie-qiwa commented Aug 13, 2025 •

edited

Loading

ChristopherHX Aug 13, 2025 •

edited

Loading

leslie-qiwa Aug 14, 2025 •

edited

Loading

leslie-qiwa commented Aug 14, 2025 •

edited

Loading

ChristopherHX commented Aug 14, 2025 •

edited

Loading