fix: harden forge-global and always-on-guidance skills for compression resilience - #59
Conversation
sonupreetam
left a comment
sonupreetam
left a comment
There was a problem hiding this comment.
Review Council
Ran the review council (6 Divisor agents). The restructuring patterns (decision table, temporal markers, priority positioning) are sound but needs structural tests and drift detection.
Cross-PR note: The Curator filed a blog opportunity issue for the prompt hardening methodology: unbound-force/website#198.
6abb1d5 to
0d2d9a5
Compare
sonupreetam
left a comment
sonupreetam
left a comment
There was a problem hiding this comment.
Review Council (6 Divisor agents)
Verdict: REQUEST CHANGES — mechanical blockers only, no content findings.
Content restructuring is well-done: decision table for forge-global, Critical Safety section for always-on-guidance, drift detection tests. No documentation gaps (blog issue unbound-force/website#198 covers the series).
Findings
CRITICAL — findRepoRoot conflict with PR #56: PR #56 defines findRepoRoot(t *testing.T) string and this PR defines findRepoRoot() (string, error) — both in agentkit_test.go. Guaranteed compile error when both merge. Must rebase after #56 merges and adopt its signature.
LOW — Missing Closes #51: PR description should include Closes #51 for auto-closing the tracked issue.
LOW — Drift detection covers 2 of 7 skills: Consider extending TestSkillFiles_DriftDetection to all 7 skills in a follow-up.
Mechanical blockers
- Needs rebase onto main (PR #54 merged, test file has shifted)
- Must merge AFTER PR #56 to avoid
findRepoRootconflict
Merge order recommendation
Merge last (#54 already merged → #55 → #57 → #56 → #59). Rebase after #56 merges.
Addressing Review Council Findings (2nd review)Thanks for the thorough second review council pass. CRITICAL —
|
5c6b9c7 to
8909a6e
Compare
…n resilience Restructure two embedded skill files to improve survival under DCP context compression, addressing six specific vulnerabilities (unbound-force#51): - Replace parallel do/don't lists with decision table in forge-global - Inline TTL parameter into reservation step, add temporal ordering - Add Critical Safety section in always-on-guidance for safety rules - Move hivemind_find from last to first position in Tool Usage - Split Code Quality, Testing, Error Handling into sub-headed groups - Sync .opencode/skills/ copies with canonical embedded source No semantic changes to any rules -- structural reorganization only. Closes unbound-force#51 Assisted-by: claude-opus Generated with AI assistance (claude-opus)
- always-on-guidance: Never -> NEVER force push to main (DR-002) - forge-global: add (5-minute auto-release) parenthetical to TTL Co-authored-by: claude-opus-4-6 <noreply@anthropic.com>
- TestAlwaysOnGuidance_StructuralHardening: verifies Critical Safety section positioning, RFC 2119 keywords, hivemind_find priority, and sub-header splits - TestForgeGlobal_StructuralHardening: verifies decision table format, temporal markers, inlined TTL, and no standalone TTL step - TestSkillFiles_DriftDetection: TC-007 drift detection comparing embedded files against .opencode/ scaffolded copies Co-authored-by: claude-opus-4-6 <noreply@anthropic.com>
8909a6e to
0074d7c
Compare
yvonnedevlinrh
left a comment
yvonnedevlinrh
left a comment
There was a problem hiding this comment.
Review Summary
APPROVE. All 6 issue #51 requirements are covered. Production changes are markdown-only with no semantic differences. CI passes.
Four findings noted — none blocking. Tracked in #70.
| ID | Severity | Finding |
|---|---|---|
| F1/F10 | MEDIUM | Assertion (6) in TestForgeGlobal_StructuralHardening is a dead negative test — the triple-AND condition can never fire. Mitigated by assertion (4) already covering the TTL-inlined invariant positively. |
| F4 | MEDIUM | TestSkillFiles_DriftDetection name implies generic skill file coverage, implementation covers 2/7. |
| F2 | LOW | Drift detection split across two tests covering disjoint skill subsets; 4 skills uncovered. |
| F3 | LOW | t.Fatalf in loop body masks second-skill failures. |
| for _, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| // Old format was a standalone step like "3. Set `ttl_seconds` to auto-release..." | ||
| if strings.HasPrefix(trimmed, "3.") && strings.Contains(trimmed, "ttl_seconds") && strings.Contains(trimmed, "auto-release") { |
There was a problem hiding this comment.
MEDIUM
The test iterates all lines looking for one that starts with "3." AND contains "ttl_seconds" AND contains "auto-release". The current forge-global SKILL.md step 3 reads:
3. FINALLY, coordinator can emergency release if workers fail: comms_release_all()
This line starts with "3." but does not contain "ttl_seconds", so the triple-AND condition can never be true. The test passes trivially — it's testing for the absence of something that's already structurally impossible given how the file was rewritten. It would still pass if someone reintroduced a 4. Set ttl_seconds... step (the old format used step 3, but a future author might use step 4). The assertion is coupled to the old numbering, not to the actual invariant ("no standalone TTL step exists").
A more robust check:
for _, line := range lines {
trimmed := strings.TrimSpace(line)
if strings.Contains(trimmed, "ttl_seconds") && strings.Contains(trimmed, "auto-release") && !strings.Contains(trimmed, "comms_reserve") {
t.Error("forge-global: standalone TTL step should be removed (inlined into step 1)")
}
}
This fires regardless of step number and regardless of whether the line is a numbered step at all.
Also
MEDIUM
The conditional at agentkit_test.go:1023:
if strings.HasPrefix(trimmed, "3.") && strings.Contains(trimmed, "ttl_seconds") && strings.Contains(trimmed, "auto-release") {
Tracing the input: text comes from content.ReadFile("content/skills/forge-global/SKILL.md"). The file's line starting with "3." is "3. FINALLY, coordinator can emergency release if workers fail: comms_release_all()". This line does not contain "ttl_seconds", so the branch body (the t.Error call) is dead code. The test would need a future regression that coincidentally uses "3." as the step number AND reintroduces both "ttl_seconds" and "auto-release" in the same line to ever trigger.
| } | ||
| } | ||
|
|
||
| func TestSkillFiles_DriftDetection(t *testing.T) { |
There was a problem hiding this comment.
MEDIUM
The function name TestSkillFiles_DriftDetection uses the plural generic "SkillFiles" (not "TwoSkillFiles" or "HardenedSkillFiles"). A new contributor reading this name would expect it to cover all embedded skill files. The skills slice at line 1037-1040 hardcodes only ["always-on-guidance", "forge-global"]. Meanwhile TestForgeCoordinationSkill_StructuralHardening separately covers forge-coordination. The remaining 4 skills (replicator-cli, testing-patterns, system-design, learning-systems) have no drift detection.
The doc comment (line 1030-1031) says "Verify embedded skill files match the .opencode/ scaffolded copies" — again generic, not qualified to a subset.
Either the name should be scoped (TestHardenedSkillFiles_DriftDetection) or the implementation should cover all skills.
Also
LOW
agentkit_test.go:894-904 already verifies byte-for-byte identity of the forge-coordination embedded vs .opencode/ copies. The new TestSkillFiles_DriftDetection at line 1029 covers always-on-guidance and forge-global but does not include forge-coordination — so the repo now has two independent drift-detection mechanisms that cover disjoint skill subsets. Neither covers all 7 skills. This isn't a bug, but the name TestSkillFiles_DriftDetection implies comprehensive drift detection for skill files generically. Anyone extending it to all 7 skills would introduce a duplicate assertion for forge-coordination.
| embeddedPath := filepath.Join("content", "skills", skill, "SKILL.md") | ||
| embedded, err := content.ReadFile(embeddedPath) | ||
| if err != nil { | ||
| t.Fatalf("read embedded %s: %v", embeddedPath, err) |
There was a problem hiding this comment.
agentkit_test.go:1046 and 1052: Both ReadFile error handlers use t.Fatalf, which stops the entire test on the first skill that fails to read. If both skills have issues, only the first is reported. For a 2-element loop this is marginal, but t.Errorf + continue would be more informative. The drift comparison itself at line 1055 correctly uses t.Errorf.
4 participants
Summary
Restructures two embedded skill files to improve constraint survival under DCP context compression, addressing six specific vulnerabilities identified in #51.
forge-global: Replaces parallel "do/don't" lists with a decision table (Signal/Forge/Skip columns), inlines TTL parameter into the reservation step, and adds temporal ordering markers (FIRST/THEN/FINALLY) to protocol steps.
always-on-guidance: Adds a dedicated
## Critical Safetysection for safety-critical rules, moveshivemind_findfrom last to first position in Tool Usage, and splits Code Quality/Testing/Error Handling lists into sub-headed groups of 2-3 items each.All changes are structural reorganization -- no rules were added, removed, or weakened.
Closes #51
How to Test
Acceptance criteria from specs:
How to Demo
Inspect the restructured skill files:
Compare the decision table format in forge-global and the sub-headed groups in always-on-guidance against the original flat list structure.
Key Files Changed
internal/agentkit/content/skills/forge-global/SKILL.mdinternal/agentkit/content/skills/always-on-guidance/SKILL.md.opencode/skills/forge-global/SKILL.md.opencode/skills/always-on-guidance/SKILL.mdopenspec/changes/harden-skill-compression/*This PR was generated by /uf.finale (AI-assisted).