test: verify Copilot reads copilot-instructions.md from PR branch by microsasa · Pull Request #478 · microsasa/cli-tools

microsasa · 2026-03-28T20:42:09Z

Test PR — DO NOT MERGE

This PR tests whether Copilot code review automatically reads .github/copilot-instructions.md from the PR head branch.

What's in this PR:

.github/CODING_GUIDELINES.md — our coding standards
.github/copilot-instructions.md — points Copilot to the guidelines
src/copilot_usage/_test_violations.py — deliberate violations of the guidelines:
- TYPE_CHECKING (banned)
- getattr (banned)
- hasattr (banned)
- assert for type validation (banned)

Expected outcome:

If Copilot flags the violations → it reads instructions from PR branch ✅
If Copilot ignores them → it doesn't, and we need explicit agent instructions ❌

After testing:

This PR will be closed and discarded.

- .github/CODING_GUIDELINES.md: comprehensive coding standards for all contributors (type safety, data modelling, naming, testing, security) - .github/copilot-instructions.md: Copilot-specific instructions that reference the guidelines file plus workflow rules and repo layout Closes #110 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Temporary commit to verify whether Copilot code review reads copilot-instructions.md from the PR branch and flags violations. Violations included: - TYPE_CHECKING (banned) - getattr (banned) - hasattr (banned) - assert for type validation (banned) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Test PR to determine whether Copilot code review reads .github/copilot-instructions.md (and linked .github/CODING_GUIDELINES.md) from the PR head branch by introducing intentionally non-compliant code.

Changes:

Add repository coding standards in .github/CODING_GUIDELINES.md.
Add .github/copilot-instructions.md pointing Copilot to those standards.
Add src/copilot_usage/_test_violations.py containing deliberate guideline violations (e.g., TYPE_CHECKING, getattr/hasattr, assert validation).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
`src/copilot_usage/_test_violations.py`	Adds intentionally non-compliant Python code to test instruction adherence.
`.github/copilot-instructions.md`	Adds Copilot-specific guidance and links to the coding guidelines.
`.github/CODING_GUIDELINES.md`	Adds documented coding standards intended for contributors and agents.

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

Copilot · 2026-03-28T20:44:43Z

+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from loguru import Logger


from loguru import Logger is imported but never used. This will trip ruff F401 unless suppressed (the existing codebase uses # noqa: F401 for type-only imports) or the symbol is referenced in an annotation.

Suggested change

from loguru import Logger

from loguru import Logger # noqa: F401

Copilot · 2026-03-28T20:44:44Z

+
+def get_value(obj: object, key: str) -> str | None:
+    """Get a value using getattr — violates guidelines."""
+    return getattr(obj, key, None)


get_value is annotated to return str | None, but getattr(obj, key, None) can return a non-str value at runtime and is also disallowed by the new guidelines. Prefer an explicitly typed interface (e.g., a Protocol or Mapping) and direct attribute access, or validate/convert the value before returning.

Suggested change

return getattr(obj, key, None)

value = getattr(obj, key, None)

if value is None:

return None

return str(value)

Copilot · 2026-03-28T20:44:44Z

+def check_type(value: object) -> bool:
+    """Use hasattr — violates guidelines."""
+    if hasattr(value, "name"):
+        return True
+    return False


Using hasattr here is both discouraged by the new guidelines and provides only a weak runtime check (it doesn't guarantee type/shape). Prefer passing a properly typed object (or Protocol) so attribute access is statically verified.

Copilot · 2026-03-28T20:44:44Z

+- **`TYPE_CHECKING` is banned.**  Do not use `from typing import
+  TYPE_CHECKING` or `if TYPE_CHECKING:` guards.  Every import used in an
+  annotation must be a real runtime import.  Circular imports are a design
+  bug — fix the module graph, do not hide cycles behind `TYPE_CHECKING`.


This guideline bans TYPE_CHECKING, but the current codebase already relies on it (e.g. src/copilot_usage/logging_config.py uses TYPE_CHECKING plus # noqa: F401, and coverage config excludes if TYPE_CHECKING:). Either relax this rule (and document the allowed pattern) or plan a refactor to remove existing TYPE_CHECKING usage and update tooling config accordingly.

Suggested change

- **`TYPE_CHECKING` is banned.** Do not use `from typing import

TYPE_CHECKING` or `if TYPE_CHECKING:` guards. Every import used in an

annotation must be a real runtime import. Circular imports are a design

bug — fix the module graph, do not hide cycles behind `TYPE_CHECKING`.

- **Avoid `TYPE_CHECKING` in new code.** Prefer real runtime imports for all

symbols used in annotations. Circular imports are a design bug — fix the

module graph rather than hiding cycles behind `TYPE_CHECKING`.

- Existing modules that already use `from typing import TYPE_CHECKING` and

`if TYPE_CHECKING:` guards (with `# noqa: F401` and excluded from coverage)

are allowed but should follow that pattern consistently until they can be

refactored to use acyclic runtime imports.

Copilot · 2026-03-28T20:44:45Z

+
+def validate(x: object) -> None:
+    """Use assert for type validation — violates guidelines."""
+    assert isinstance(x, str), "must be a string"


Using assert for validation is unsafe because assertions can be stripped with python -O, so this function may perform no validation in production. Raise a real exception (TypeError/ValueError) or use a Pydantic model at the boundary instead.

Suggested change

assert isinstance(x, str), "must be a string"

if not isinstance(x, str):

raise TypeError("must be a string")

Copilot · 2026-03-28T20:44:45Z

+- **No `isinstance` checks as a substitute for proper typing.** If the type
+  system cannot express the constraint, refactor so it can.


The blanket ban on isinstance checks conflicts with existing boundary-parsing code (e.g. src/copilot_usage/parser.py uses isinstance to validate JSON-derived values). Consider scoping this rule to business logic only, or explicitly allowing runtime checks when converting untyped external data into typed models.

Suggested change

- **No `isinstance` checks as a substitute for proper typing.** If the type

system cannot express the constraint, refactor so it can.

- **Avoid `isinstance` checks in business logic.** Use static typing and data

models instead; reserve `isinstance` for validating untyped external data

at I/O boundaries (e.g. parsing JSON into typed models).

Copilot · 2026-03-28T20:44:45Z

+- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable
+  defaults — this avoids a known pyright false-positive.


This recommends default_factory=lambda: [] instead of default_factory=list for mutable defaults, but the codebase currently uses Field(default_factory=list) in Pydantic models (e.g. src/copilot_usage/models.py). If this guidance only applies to dataclasses.field, clarify that to avoid confusion, or align existing usage with the rule.

Suggested change

- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable

defaults — this avoids a known pyright false-positive.

- For `dataclasses.field`, use `default_factory=lambda: []` (not

`default_factory=list`) for mutable defaults — this avoids a known pyright

false-positive.

- For Pydantic models, `Field(default_factory=list)` is acceptable and does

not need the lambda workaround.

Copilot · 2026-03-28T20:44:45Z

+all code changes. Key points:
+
+- **pyright strict** — every parameter and return value must be annotated.
+- **No duck typing** — no `getattr`, `hasattr`, or runtime type interrogation.


This instruction bans runtime type checks, but the existing parser currently uses isinstance checks to safely coerce untyped JSON config/events into typed values. If the intent is to prohibit duck-typing in business logic, consider clarifying that boundary validation/parsing is exempt (or update the codebase to comply).

Suggested change

- **No duck typing** — no `getattr`, `hasattr`, or runtime type interrogation.

- **No duck typing in business logic** — do not use `getattr`, `hasattr`, or runtime type

interrogation in core logic. Boundary validation/parsing (for example, coercing untyped

JSON/config using `isinstance`) is allowed.

Copilot · 2026-03-28T20:44:46Z

+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from loguru import Logger
+
+


TYPE_CHECKING blocks are introduced here, but the new CODING_GUIDELINES in this PR explicitly ban from typing import TYPE_CHECKING / if TYPE_CHECKING:. If these guidelines are meant to apply, remove the guard and resolve any import cycles by refactoring, or adjust the guidelines to allow type-only imports.

Suggested change

from typing import TYPE_CHECKING

if TYPE_CHECKING:

from loguru import Logger

microsasa · 2026-03-28T20:51:06Z

Test complete — Copilot reads copilot-instructions.md from PR branch and flags violations. Closing to refine guidelines and do a proper PR.

Sasa Junuzovic and others added 2 commits March 28, 2026 13:23

Copilot AI review requested due to automatic review settings March 28, 2026 20:42

Copilot started reviewing on behalf of microsasa March 28, 2026 20:42 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

microsasa closed this Mar 28, 2026

microsasa mentioned this pull request Mar 29, 2026

docs: add coding guidelines and copilot instructions #482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: verify Copilot reads copilot-instructions.md from PR branch#478

test: verify Copilot reads copilot-instructions.md from PR branch#478
microsasa wants to merge 2 commits intomainfrom
docs/coding-guidelines

microsasa commented Mar 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

Copilot AI Mar 28, 2026

Uh oh!

microsasa commented Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	from loguru import Logger
	from loguru import Logger # noqa: F401

-    return getattr(obj, key, None)
+    value = getattr(obj, key, None)
+    if value is None:
+        return None
+    return str(value)

-- **`TYPE_CHECKING` is banned.**  Do not use `from typing import
-  TYPE_CHECKING` or `if TYPE_CHECKING:` guards.  Every import used in an
-  annotation must be a real runtime import.  Circular imports are a design
-  bug — fix the module graph, do not hide cycles behind `TYPE_CHECKING`.
+- **Avoid `TYPE_CHECKING` in new code.** Prefer real runtime imports for all
+  symbols used in annotations. Circular imports are a design bug — fix the
+  module graph rather than hiding cycles behind `TYPE_CHECKING`.
+- Existing modules that already use `from typing import TYPE_CHECKING` and
+  `if TYPE_CHECKING:` guards (with `# noqa: F401` and excluded from coverage)
+  are allowed but should follow that pattern consistently until they can be
+  refactored to use acyclic runtime imports.

	assert isinstance(x, str), "must be a string"
	if not isinstance(x, str):
	raise TypeError("must be a string")

		- No `isinstance` checks as a substitute for proper typing. If the type
		system cannot express the constraint, refactor so it can.

-- **No `isinstance` checks as a substitute for proper typing.** If the type
-  system cannot express the constraint, refactor so it can.
+- **Avoid `isinstance` checks in business logic.** Use static typing and data
+  models instead; reserve `isinstance` for validating untyped external data
+  at I/O boundaries (e.g. parsing JSON into typed models).

		- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable
		defaults — this avoids a known pyright false-positive.

-- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable
-  defaults — this avoids a known pyright false-positive.
+- For `dataclasses.field`, use `default_factory=lambda: []` (not
+  `default_factory=list`) for mutable defaults — this avoids a known pyright
+  false-positive.
+- For Pydantic models, `Field(default_factory=list)` is acceptable and does
+  not need the lambda workaround.

-- **No duck typing** — no `getattr`, `hasattr`, or runtime type interrogation.
+- **No duck typing in business logic** — do not use `getattr`, `hasattr`, or runtime type
+  interrogation in core logic. Boundary validation/parsing (for example, coercing untyped
+  JSON/config using `isinstance`) is allowed.

Conversation

microsasa commented Mar 28, 2026

Test PR — DO NOT MERGE

What's in this PR:

Expected outcome:

After testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

microsasa commented Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants