PoC: add context files by Michal-Leszczynski · Pull Request #4798 · scylladb/scylla-manager

Michal-Leszczynski · 2026-03-24T17:50:23Z

No description provided.

This commit adds 2 context files used by ai agents: - AGENTS.md - general context file containing project overview - integration-test-review/SKILL.md - context file containing instructions for reviewing changes made to integration tests AGENTS.md file placed at repo root is recognized by both opencode and copilot tools. By default, SKILL.md files live under different paths in opencode and copilot tools, but they both support claude SKILL.md paths, so that's why it was placed there. This is the first iteration of trying to use context files in SM repo and their contents will be adjusted after gathering more info on how they do.

Michal-Leszczynski · 2026-03-24T17:51:05Z

@copilot review changes in this PR

Copilot

Pull request overview

This PR introduces agent/context documentation files and changes default backup keyspace selection to exclude temporary keyspaces, with an integration test validating tmp* keyspaces are omitted from backup targets.

Changes:

Add default exclusion for tmp* keyspaces in backup task properties.
Add an integration test asserting tmp* keyspaces are not included in backup targets even when requested.
Add repository context/documentation files for agents and integration test review.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
pkg/service/backup/model.go	Changes default backup keyspace filters to exclude `tmp*`.
pkg/service/backup/service_backup_integration_test.go	Adds integration coverage around excluding `tmp*` keyspaces from backup targets.
AGENTS.md	Adds high-level repository/module and development workflow documentation.
.claude/skills/integration-test-review/SKILL.md	Adds guidelines for reviewing integration test changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T17:57:43Z

 	}
 }
+
+func TestTmpKsIntegration(t *testing.T) {


This integration test name does not follow the documented convention in this repo (service-prefixed and ...Integration suffix, e.g. TestBackup...Integration). Also, the test covers multiple scenarios (3 different keyspace inputs) but is not table-driven; converting to table-driven subtests would make failures clearer and align with the guidance added in .claude/skills/.../SKILL.md.

Suggested change

func TestTmpKsIntegration(t *testing.T) {

func TestBackupTmpKsIntegration(t *testing.T) {

I wonder if ai saw what is the purpose of this PR and crafted response to satisfy it:)

Copilot · 2026-03-24T17:57:43Z

 		Transfers: scyllaclient.TransfersFromConfig,
 		Method:    MethodRclone,
 		Continue:  true,
+		Keyspace:  []string{"*", "!tmp*"},


This changes the default keyspace selection behavior, but the added integration test only exercises cases where keyspace is explicitly provided. Please add a coverage case where keyspace is omitted (i.e., rely on defaults) and assert that tmp* keyspaces are excluded, to directly validate this default-property change.

good comment

Copilot · 2026-03-24T17:57:43Z

+
+### SM server architecture
+
+SM server is build around its scheduler responsible for scheduling and triggering tasks and services responsible


Correct grammar: 'build' → 'built'.

Suggested change

SM server is build around its scheduler responsible for scheduling and triggering tasks and services responsible

SM server is built around its scheduler responsible for scheduling and triggering tasks and services responsible

Copilot · 2026-03-24T17:57:44Z

+
+### SM agent architecture
+
+SM agent is a small server running on every Scylla node. It servers a proxy for SM server to Scylla HTTP REST API,


Correct grammar: 'servers' → 'serves'.

Suggested change

SM agent is a small server running on every Scylla node. It servers a proxy for SM server to Scylla HTTP REST API,

SM agent is a small server running on every Scylla node. It serves as a proxy for SM server to Scylla HTTP REST API,

Copilot · 2026-03-24T17:57:44Z

+- utils module (./v3/pkg/utils) containing generic legacy helpers (new helpers are added to ./pkg/util2)
+
+In case changes need to be made across multiple modules, the need to be merged in separate per module PRs.
+Modules in go.mod can only reference commits from master branch.


This repo appears to use main (diffs are index main..branch), but the doc says 'master branch'. Please update this to the correct branch name to avoid misleading contributors.

Suggested change

Modules in go.mod can only reference commits from master branch.

Modules in go.mod can only reference commits from main branch.

Copilot · 2026-03-24T17:57:44Z

+- Test name should start with `Test` followed by service name (if applicable) and end with `Integration` (e.g., `TestBackupSmokeIntegration`)
+- They should start with a comment explaining which features are tested
+- They should follow the table-driven approach whenever multiple scenarios are tested. Table entry should contain at least the test name and description
+- Each tested feature should be covered by a separate test of subtest, unless it's too small to justify such separation


Correct wording: 'of subtest' → 'or subtest'.

Suggested change

- Each tested feature should be covered by a separate test of subtest, unless it's too small to justify such separation

- Each tested feature should be covered by a separate test or subtest, unless it's too small to justify such separation

Copilot · 2026-03-24T18:00:25Z

@copilot review changes in this PR

Here's the review of all changes in this PR:

1. `AGENTS.md` — ✅ Looks good

Well-structured context file. No issues found.

2. `.claude/skills/integration-test-review/SKILL.md` — Minor

Missing newline at end of file (standard convention).

3. `pkg/service/backup/model.go` — Code change

Adding Keyspace: []string{"*", "!tmp*"} to defaultTaskProperties() means that when a user does NOT specify any keyspace filter, backups will exclude tmp* keyspaces by default. When a user explicitly provides a keyspace list, json.Unmarshal fully replaces the default, so !tmp* has no effect.

4. `pkg/service/backup/service_backup_integration_test.go` — Multiple issues

Naming: TestTmpKsIntegration should follow the convention TestBackup<Feature>Integration, e.g. TestBackupTmpKeyspaceExclusionIntegration.

Missing comment: The test should start with a comment explaining the tested feature.

Not table-driven: Three scenarios are tested inline — this should use table-driven approach per the integration test requirements.

Fundamental logic bug in scenarios 1 and 3: All three scenarios explicitly set props["keyspace"], which overwrites the default ["*", "!tmp*"] via json.Unmarshal. So:

Scenario 1 (keyspace: ["ks", "tmpks"]): The !tmp* exclusion is NOT active. tmpks WILL be included in target. The assertion if slices.ContainsFunc(target.Units, containsTmpKs) → t.Fatal("Expected no tmp ks") would fail.
Scenario 3 (keyspace: ["tmpks"]): Same issue — tmpks WILL be in target. The assertion expecting no tmpks would fail.

Wrong error message: Line 3181 checks len(target.Units) != 1 but the error message says "Expected 2 units" — should be "Expected 1 unit".

Missing key scenario: The test never validates the actual behavior introduced by the code change — i.e., when NO explicit keyspace is provided and the default ["*", "!tmp*"] filter applies. That's the primary scenario to test.

Setup/validation separation: Could be improved with helper functions for property marshaling.

Summary: The context files (AGENTS.md, SKILL.md) look good. The backup code change is reasonable. However, the integration test has fundamental correctness issues — the test scenarios don't actually test the behavior introduced by the model.go change (default !tmp* exclusion), and scenarios 1 and 3 would fail because explicit keyspace lists override the default filter. These must be addressed before merging.

Michal-Leszczynski added 2 commits March 24, 2026 18:49

tmp(backup): add some feature and integration test

1ed497c

Michal-Leszczynski requested a review from Copilot March 24, 2026 17:50

Copilot started work on behalf of Michal-Leszczynski March 24, 2026 17:51 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Copilot started reviewing on behalf of Michal-Leszczynski March 24, 2026 18:01 View session

Copilot finished work on behalf of Michal-Leszczynski March 24, 2026 18:01

Michal-Leszczynski mentioned this pull request Mar 25, 2026

Add agents context files #4800

Merged

Michal-Leszczynski closed this Mar 26, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PoC: add context files#4798

PoC: add context files#4798
Michal-Leszczynski wants to merge 2 commits into
masterfrom
ml/add-ai-files

Michal-Leszczynski commented Mar 24, 2026

Uh oh!

Michal-Leszczynski commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Michal-Leszczynski Mar 25, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Michal-Leszczynski Mar 25, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	func TestTmpKsIntegration(t *testing.T) {
	func TestBackupTmpKsIntegration(t *testing.T) {


		### SM server architecture

		SM server is build around its scheduler responsible for scheduling and triggering tasks and services responsible

	SM server is build around its scheduler responsible for scheduling and triggering tasks and services responsible
	SM server is built around its scheduler responsible for scheduling and triggering tasks and services responsible


		### SM agent architecture

		SM agent is a small server running on every Scylla node. It servers a proxy for SM server to Scylla HTTP REST API,

	Modules in go.mod can only reference commits from master branch.
	Modules in go.mod can only reference commits from main branch.

	- Each tested feature should be covered by a separate test of subtest, unless it's too small to justify such separation
	- Each tested feature should be covered by a separate test or subtest, unless it's too small to justify such separation

Uh oh!

Conversation

Michal-Leszczynski commented Mar 24, 2026

Uh oh!

Michal-Leszczynski commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Michal-Leszczynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Michal-Leszczynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Mar 24, 2026

1. AGENTS.md — ✅ Looks good

2. .claude/skills/integration-test-review/SKILL.md — Minor

3. pkg/service/backup/model.go — Code change

4. pkg/service/backup/service_backup_integration_test.go — Multiple issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. `AGENTS.md` — ✅ Looks good

2. `.claude/skills/integration-test-review/SKILL.md` — Minor

3. `pkg/service/backup/model.go` — Code change

4. `pkg/service/backup/service_backup_integration_test.go` — Multiple issues