refactor: rewrite PoKeysHal TDD and audit prompts - #145
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the two active GitHub Copilot repository prompts to be PoKeysHal-specific, focusing on C99/LinuxCNC HAL real-time constraints, async subsystem boundaries, protocol evidence, and HIL-aware verification workflows (per issues #140/#143).
Changes:
- Rewrites
tdd-compile.prompt.mdinto a concise Red/Green/Refactor implementation entry point that routes hardware-dependent work to thehil-tddskill and uses PoKeysHal verification layers. - Rewrites
repository-audit.prompt.mdinto a read-only PoKeysHal audit workflow covering traceability, async boundaries, HAL integration, RT constraints, protocol evidence, HIL fixture gating, and documentation consistency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/prompts/tdd-compile.prompt.md | Replaces generic TDD content with a PoKeysHal-focused implementation workflow and verification ladder. |
| .github/prompts/repository-audit.prompt.md | Replaces generic audit template with a PoKeysHal-specific, read-only audit checklist and evidence vocabulary. |
zarfld
left a comment
There was a problem hiding this comment.
Independent Phase 2 review: changes required before merge
I reviewed the complete final contents of both prompt files against issue #143, AGENTS.md, the engineering-discipline and C/RT instructions, the rewritten TDDDriver/TestingSpecialist agents, and the approved hil-tdd skill/result schema.
What passes
- Branch topology is correct: one commit ahead of
main, zero behind. - Exactly the two files authorized by #143 are changed.
- Both files are within their line budgets.
- Generic web/auth stack material is removed.
- The prompts use PoKeysHal-specific C99, PoKeysLib, HAL, async, RT, protocol, and HIL terminology.
- HIL fixture gating and the prohibition on userspace/mock
HIL-verifiedclaims are directionally correct.
Required changes
- Correct both prompt-file frontmatter blocks. Current prompt-file metadata uses
agent, notmode;applyTois instruction-file metadata and does not scope or recommend a.prompt.mdfile. Useagent: TDDDriverfor the implementation prompt and an appropriate agent value for the audit prompt, then removeapplyTo. - Reduce TDDDriver duplication.
tdd-compile.prompt.mdcurrently reproduces nearly the complete TDDDriver workflow. Keep it as a concise invocation/orchestration contract: input issue, baseline gate, explicit Red–Green–Refactor sequence, outward verification, HIL routing, and required handoff. The agent remains the authority for implementation behavior. - Remove unsupported audit requirements. The canonical C/RT contract prohibits dynamic allocation, blocking operations, and unbounded execution on RT-reachable paths. It does not categorically prohibit all VLAs/non-constant bounds, all cross-subsystem calls, or require every exported pin to update on every HAL invocation. Audit against the exact canonical responsibility and boundedness rules rather than inventing stricter rules.
- Clarify read-only evidence collection. The audit must inspect existing command output/CI/log evidence, not execute the listed build and HAL commands. Missing evidence should be reported as unavailable.
- Do not duplicate the result-schema table.
result-schema.mdexplicitly declares itself the single authority and says not to reproduce its table elsewhere. Reference it and require its terms; remove the copied HIL definitions. In the TDD handoff, includeHIL-observedwhere applicable and explicitly requirepartially verifiedwhen a required verification layer is unavailable. - Refresh PR verification evidence after correction. Include exact changed files, final line counts, current prompt-frontmatter validation, forbidden-term scan,
git diff --check, andgit diff --ignore-space-at-eol.
The existing Copilot comment about adding .comp to applyTo should not be implemented: prompt files are manually invoked; applyTo belongs to .instructions.md files. Removing applyTo is the correct fix.
3044054 to
99758f3
Compare
zarfld
left a comment
There was a problem hiding this comment.
Second independent review: improved, but not yet merge-ready
I verified the amended head rather than relying on the Copilot session summary. The following claims are correct:
- the branch is one commit ahead of
mainand changes only the two approved prompt files; agentandargument-hintare valid prompt-file fields;modeandapplyTohave been removed;- the generic web/auth stack is gone;
- the TDD prompt is now materially smaller and distinct from
TDDDriver; - the read-only evidence table no longer instructs the auditor to execute builds;
- the copied HIL term definitions were removed;
HIL-observedand thepartially verifiedstate were added.
The statement that all findings are addressed is still too strong. Please correct these remaining items:
-
Do not invent a VLA prohibition.
repository-audit.prompt.mdstill says to flag every variable-length array on an RT-reachable path. The canonical C/RT contract prohibits dynamic allocation, blocking operations, and unbounded execution, but does not categorically prohibit every bounded VLA. Removevariable-length arraysfrom that rule unless the canonical instruction is separately changed to establish that invariant. -
Make outward verification explicitly conditional. In
tdd-compile.prompt.md, say “through the applicable layers” and mark userspace HAL, RT, and HIL layers as applicable/required by the acceptance criteria. The current wording can be read as requiring RT and HIL for every implementation issue, which contradicts the agent and issue #143 wording. -
Complete the traceability audit vocabulary. The canonical root instructions include
QA-SC, but the audit’s issue-type inventory omits it. AddQA-SCso the audit does not report against an incomplete repository issue model. -
Complete HIL fixture registration checks. The audit correctly requires
runnable: trueandfixture_status: verified, but issue #143 also requires fixture registration. Require recorded evidence to identify the registered setup ID and revision; do not duplicate the detailed preflight procedure. Also callHIL-observed,HIL-test-executed, andHIL-verifiedstatus terms, not “outcome vocabulary”; PASS/FAIL/ERROR/SKIPPED are the outcomes. -
Require exact evidence in the output. The output format currently asks only for a file or issue reference and observed/required state. Add an exact evidence field such as file:line, issue/PR reference, CI run/log, test report, or HIL artifact. Also state that an audit with a required unavailable layer is
partially verified. -
Do not restrict corrective issue types incorrectly. The corrective-plan output currently limits recommendations to
REQ-F,TEST,ARC-C, or process change. Findings may requireREQ-NF,ADR,QA-SC, bug, integration, documentation, or another existing repository issue type. Use “appropriate existing issue type” with examples rather than a closed four-type list. -
Refresh the PR description and verification evidence. The PR body still reports the pre-amendment 116/136 line counts and describes the old file shape. Update it to the current 65/135 counts and include the frontmatter validation and
git diff --checkresult. The only Actions runs on this head are unrelated lifecycle-issue workflows; they do not validate these prompt files. State that actual VS Code prompt discovery/invocation was not executed if it was not tested.
After those corrections, resolve the obsolete review threads and request the final pass. The current content is close, but these items are required for issue #143’s no-invented-rules, exact-evidence, traceability, and HIL-registration acceptance criteria.
99758f3 to
fec8d78
Compare
zarfld
left a comment
There was a problem hiding this comment.
Third independent review
The amended files now resolve most findings from review 4829158807: prompt frontmatter is valid, outward verification is conditional, QA-SC is present, HIL registration/status/outcome wording is corrected, exact evidence is required, and the corrective-plan issue types are non-exhaustive.
Three corrections remain before merge:
-
Add explicit acceptance-criterion coverage to the audit. The section is titled “Issue and acceptance-criterion traceability,” but it currently verifies issue/link/comment conventions only. Require the audit to confirm that each in-scope approved acceptance criterion has corresponding implementation and verification evidence, or record the missing link as a finding.
-
Include
freein the RT-path memory-operation scan. The canonical C/RT instruction prohibitsmalloc,free, and dynamic allocation on RT-reachable paths; the current explicit function list omitsfree. -
The PR body update did not take effect on GitHub. The remote PR still reports 116/136 lines and the old verification block. Update it to the actual 65/148 evidence and disclose that VS Code prompt discovery/invocation was not performed. Verify the result with
gh pr view 145 --json bodyafter editing.
After those edits, resolve the addressed/outdated review threads and rerun the final two-file, line-count, forbidden-term, frontmatter, and git diff --check evidence. No runtime, RT, or HIL execution is required for this documentation-only change.
- Narrowed workflow entry point to focus on approved PoKeysHal implementation work. - Updated prompt to apply specifically to C and header files, enhancing clarity for TDD processes. - Refined TDD cycle steps to emphasize the importance of issue traceability and adherence to TDD principles. - Improved documentation structure for better readability and usability in TDD workflows.
fec8d78 to
d12bf6c
Compare
zarfld
left a comment
There was a problem hiding this comment.
Final review on current head d12bf6c
The two findings from review 4829303236 are resolved on this head:
- acceptance-criterion mapping now requires an implementing artifact and verification evidence for each in-scope approved criterion;
- the RT-path scan now explicitly includes
free.
The remote PR body is also current: it reports 65 and 150 lines, the two-file scope, git diff --check, the forbidden-term scan, frontmatter validation, responsibility-boundary validation, and the disclosed absence of a VS Code invocation test.
The implementation now satisfies issue #143's scope and acceptance criteria. No further content changes are required before merge. Resolve the addressed review threads so the conversation state matches the current files.
Issue #140 remains open for Phase 3 consolidation after this PR merges.
Closes #143
Part of #140
Changes
Rewrites both active repository prompts to describe PoKeysHal-specific C99, PoKeysLib, LinuxCNC HAL, real-time, asynchronous, and HIL-aware workflows. Removes all generic web-application content (TypeScript, JWT, REST, database, browser, Jest, Cypress, etc.).
tdd-compile.prompt.md: 801 → 65 lines. Narrow orchestration contract routing to TDDDriver; requires baseline, Red-Green-Refactor through applicable layers, hil-tdd routing for hardware-dependent criteria, and full handoff evidence including HIL-observed and partially-verified status.repository-audit.prompt.md: 1899 → 150 lines. Read-only audit workflow covering traceability (StR/REQ-F/REQ-NF/ADR/ARC-C/QA-SC/TEST with acceptance-criterion mapping), C99/PoKeysLib structure, async boundaries, HAL exports, RT constraints (includingfreeon RT paths), protocol evidence, HIL fixture gating (runnable+verified+setup ID+revision), documentation consistency, and exact-evidence output format with partially-verified disclosure.Verification evidence
Frontmatter: both files use
agent+argument-hint;modeandapplyToabsent.Prompt vs agent/skill boundary:
tdd-compileis an orchestration contract only — it does not reproduce TDDDriver's procedure steps.repository-auditreferencesresult-schema.mdfor HIL definitions; it does not redefine them.VS Code prompt invocation: not performed. The prompts were not run from VS Code after amendment; invocation behavior in the VS Code prompt picker is unverified.