Skip to content

fix: agentkit test hygiene — dead branch, misleading name, fragmented drift detectio #70

Description

@yvonnedevlinrh

Follow-up from PR #59 review. Four non-blocking test quality findings in internal/agentkit/agentkit_test.go:

F1/F10 — Dead negative test in TestForgeGlobal_StructuralHardening

Assertion (6) checks for a line starting with "3." that also contains "ttl_seconds" and "auto-release". The current step 3 is "3. FINALLY, coordinator can emergency release..." which doesn't contain "ttl_seconds", so the branch can never fire. The regression it guards against (standalone TTL step reintroduced) would not be caught.

Suggested fix: match any line containing both "ttl_seconds" and "auto-release" that is NOT part of the comms_reserve call, regardless of step number.

F4 — TestSkillFiles_DriftDetection name implies generic scope

The name says "SkillFiles" but the implementation covers only always-on-guidance and forge-global (2 of 7 skills). Either scope the name (TestHardenedSkillFiles_DriftDetection) or extend to all 7 skills.

F2 — Fragmented drift detection

Drift detection is split across two tests covering disjoint subsets: TestSkillFiles_DriftDetection (2 skills) and TestForgeCoordinationSkill_StructuralHardening assertion (5) (1 skill). Four skills have no drift detection. Consider consolidating into a single parameterized test covering all 7.

F3 — t.Fatalf in loop masks failures

TestSkillFiles_DriftDetection uses t.Fatalf for ReadFile errors inside the skill loop. If both skills fail to read, only the first is reported. Use t.Errorf + continue instead.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions