feat: add Natural-Language Agent Harness (NLAH) contract system#28
feat: add Natural-Language Agent Harness (NLAH) contract system#28frankxai wants to merge 3 commits into
Conversation
Formalizes ACOS skill execution with opt-in contracts based on arXiv:2603.25723. Skills can now declare required outputs, budget limits, permissions, completion conditions, and artifact configuration in SKILL.md frontmatter. New files: - .claude/schemas/nlah-contract.schema.json — contract JSON schema - .claude/skills/nlah-contracts/SKILL.md — system documentation skill - .claude/RUNTIME_CHARTER.md — runtime interpretation spec (itself an NLAH) - .claude/hooks/contract-enforcer.js — PostToolUse budget tracker - .claude/hooks/artifact-manager.js — Stop hook for manifest sealing - .acos/.gitignore — isolate execution artifacts from version control Modified: - planning-with-files: reference contract block added as example - skill-rules.json: NLAH auto-activation rule Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01DxqEuLQdCgt6cbA8c1QCu5
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Natural-Language Agent Harness (NLAH) contract system for ACOS, including a runtime charter, a contract JSON schema, a new documentation skill, and Node.js hooks for contract enforcement and artifact management. The review feedback highlights several critical issues: incorrect property access in contract-enforcer.js that silences budget enforcement, potential race conditions and state corruption from using global state files for concurrent sessions or child agents, and a lack of glob pattern support in artifact-manager.js when verifying outputs. Additionally, the reviewer noted a potential crash if the index file is corrupted, along with minor opportunities to clean up an unused parameter and round elapsed time values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const contract = findActiveContract(); | ||
| if (!contract || !contract.budget) { | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
In contract-enforcer.js, the active contract is checked for contract.budget directly. However, according to the schema and the implementation in artifact-manager.js, the contract details (including budget) are nested under the contract property of the parsed object (i.e., contract.contract.budget). As written, contract.budget will always be undefined, causing the enforcer to silently exit and never enforce any budgets.
| const contract = findActiveContract(); | |
| if (!contract || !contract.budget) { | |
| process.exit(0); | |
| } | |
| const contract = findActiveContract(); | |
| if (!contract || !contract.contract?.budget) { | |
| process.exit(0); | |
| } |
|
|
||
| writeBudgetState(state); | ||
|
|
||
| const budget = contract.budget; |
There was a problem hiding this comment.
Following the nested structure of the contract object, budget should be accessed via contract.contract.budget instead of contract.budget.
| const budget = contract.budget; | |
| const budget = contract.contract.budget; |
| const ACOS_DIR = path.join(process.cwd(), '.acos', 'artifacts'); | ||
| const ACTIVE_FILE = path.join(ACOS_DIR, '_active-contract.json'); | ||
| const BUDGET_FILE = path.join(ACOS_DIR, '_budget-state.json'); |
There was a problem hiding this comment.
Using a single global file path for _active-contract.json and _budget-state.json under .acos/artifacts/ will cause race conditions and state corruption when multiple sessions run concurrently or when child agents are spawned (which is a core feature of the NLAH lifecycle defined in the charter). Since child agents run in the same project directory, they will overwrite the parent's active contract and budget state. When the child agent completes, it will delete these files, leaving the parent agent unable to track its budget or seal its manifest. Consider using a session-specific identifier (e.g., from an environment variable like ACOS_SESSION_ID or CLAUDE_SESSION_ID) to namespace these active state files.
| if (o.pattern) { | ||
| const resolved = o.pattern; | ||
| result.path = resolved; | ||
| try { | ||
| result.exists = fs.existsSync(path.join(process.cwd(), resolved)); | ||
| if (result.exists) { | ||
| const stat = fs.statSync(path.join(process.cwd(), resolved)); | ||
| result.size_bytes = stat.size; | ||
| } | ||
| } catch { | ||
| result.exists = false; | ||
| } |
There was a problem hiding this comment.
The schema and documentation define pattern as a glob pattern (e.g., docs/specs/SPEC-*.md). However, fs.existsSync does not support glob expansion and will treat wildcard characters (like *) as literal characters, causing verification to fail for any actual glob patterns. Consider using a glob matching library or implementing a simple directory-scanning helper to resolve wildcard patterns.
| } catch { return null; } | ||
| } | ||
|
|
||
| function readBudgetState(skillName) { |
There was a problem hiding this comment.
The parameter skillName is defined in readBudgetState but is never used within the function. Consider removing it to keep the signature clean.
| function readBudgetState(skillName) { | |
| function readBudgetState() { |
| ].filter(Boolean); | ||
|
|
||
| if (budget['timeout-minutes'] && state.started_at) { | ||
| const elapsed = (Date.now() - new Date(state.started_at).getTime()) / 60000; |
There was a problem hiding this comment.
The calculated elapsed time is a floating-point number with high precision. When printed in budget warnings, it will display many decimal places (e.g., 12.3456789/45). Consider rounding it to one decimal place for cleaner output.
| const elapsed = (Date.now() - new Date(state.started_at).getTime()) / 60000; | |
| const elapsed = Math.round(((Date.now() - new Date(state.started_at).getTime()) / 60000) * 10) / 10; |
| let index = []; | ||
| try { | ||
| index = JSON.parse(fs.readFileSync(indexPath, 'utf8')); | ||
| } catch {} |
There was a problem hiding this comment.
If _index.json exists but is corrupted or does not contain an array (e.g., it is an empty object {}), index.push will throw a TypeError and crash the script. It is safer to verify that the parsed data is indeed an array before using it.
let index = [];
try {
const data = JSON.parse(fs.readFileSync(indexPath, 'utf8'));
if (Array.isArray(data)) {
index = data;
}
} catch {}contract-enforcer.js: - Fix nested property access: contract.contract.budget (was contract.budget) - Remove unused skillName parameter from readBudgetState - Round elapsed timeout to 1 decimal place - Namespace state files by session ID to prevent concurrent corruption artifact-manager.js: - Namespace active-contract/budget-state files by session ID - Add glob pattern matching for output verification (was using fs.existsSync) - Add Array.isArray guard on index file parsing Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01DxqEuLQdCgt6cbA8c1QCu5
Queen-doctrine review — verdict: NEEDS-CHANGESClassification: Substrate (runtime execution contracts for every contracted skill). MERGEABLE/CLEAN, well-structured docs, but one load-bearing gap: The two hooks are never registered. Required before merge:
Once wired, this deserves a board pass before ready — it changes the execution model (budgets, permission intersection, child-agent lifecycle) for every skill that opts in, which is Substrate-level surface. The design itself is good: opt-in, additive, backward compatible, and |
Addresses queen-doctrine review:
1. Register contract-enforcer as PostToolUse hook and artifact-manager
as Stop hook in .claude/hooks.json — enforcement is now live
2. Harden matchSegment regex to escape +, ^, $, {, }, (, ), |, [, ], \
so patterns from skill frontmatter can't inject regex
3. Verified test plan: budget warnings fire at 70%/90%/100% thresholds,
manifest seals with correct output verification and budget usage,
active state cleans up after sealing
Co-Authored-By: claude-flow <ruv@ruv.net>
Claude-Session: https://claude.ai/code/session_01DxqEuLQdCgt6cbA8c1QCu5
2 participants
Summary
.claude/RUNTIME_CHARTER.md) defines how contracts are interpreted and enforced at runtime — itself written as a natural-language agent harness.artifact-manifest.json, updates the cross-skill index.planning-with-filesskill demonstrates the contract block pattern..acos/.gitignorekeeps execution artifacts out of version control.Files Added/Modified
.claude/schemas/nlah-contract.schema.json.claude/skills/nlah-contracts/SKILL.md.claude/RUNTIME_CHARTER.md.claude/hooks/contract-enforcer.js.claude/hooks/artifact-manager.js.claude/skill-rules.json.claude/skills/planning-with-files/SKILL.md.acos/.gitignoreTest plan
planning-with-filesskill still activates normally (contract is additive, not breaking).acos/artifacts/directory is created on contracted skill activation.acos/artifacts are excluded from git tracking🤖 Generated with claude-flow
https://claude.ai/code/session_01DxqEuLQdCgt6cbA8c1QCu5
Generated by Claude Code