Repository navigation
Feat: add Vim grammar token model - #119
8 commits merged into
Conversation
Summary by CodeRabbit
WalkthroughThis PR closes WSC evidence slices 127–136 by implementing deterministic history listing, replay proof verification, and export tooling, while establishing comprehensive design roadmaps for Vim power-moves parity (WF-0105) and Emacs-inspired causal features (WF-0106), backed by parity matrices, usability fixtures, and snapshot-driven grammar tests. ChangesWSC History, Replay Proof, Vim Grammar Parity, and Design Roadmaps
Sequence Diagram(s)No sequence diagrams generated: the changes comprise design documentation, test fixtures, port contracts, and implementation of deterministic history listing/replay proof/grammar tokenization—primarily data modeling and offline verification logic without multi-component request/response flows or user interaction sequences that would benefit from visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: Substantial heterogeneous changes across deterministic history/replay logic, Vim grammar tokenization with multiple specialized readers, comprehensive test suites, exhaustive parity matrix JSON (744 lines), detailed design documents (1236 + 306 lines), and cascading documentation updates. Logic density is moderate (sorting, digest computation, mismatch detection, envelope parsing), files are diverse (TypeScript, JSON, specs, markdown), and proof-of-correctness assertions are numerous but repetitive in pattern. Requires close attention to semantic identity field ordering, obstruction code paths, envelope schema dispatch, token reader precedence/lookahead, and alignment of parity matrix/fixture/test expectations. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d692199e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| artifact: undefined, | ||
| materialize(envelope) { | ||
| const payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING)); |
There was a problem hiding this comment.
When an agent exports a retained basis whose envelope digest is valid but whose bytes are malformed JSON, this JSON.parse throws out of the materializer instead of returning JEDIT_WSC_CURRENT_HISTORY_EXPORT_OBSTRUCTED. That makes scripts/jedit-wsc-history.mjs export --json ... terminate with a stack trace/non-JSON output for a basis that the new history listing path otherwise represents as malformed evidence, so callers cannot handle the documented typed obstruction path.
Useful? React with 👍 / 👎.
| closeoutStage('submission', (record) => record.submissionId != null), | ||
| closeoutStage('admission', (record) => record.admissionId != null), | ||
| closeoutStage('ticket', (record) => record.ticketId != null), | ||
| closeoutStage('execution', (record) => record.executionId != null), | ||
| closeoutStage('receipt', (record) => record.receiptId != null), |
There was a problem hiding this comment.
For closeout on real WSC history, these stage predicates require submissionId, admissionId, ticketId, executionId, and checkpointId, but the actual settlement envelope writer only persists filePath, bufferId, commandKind, range, submittedAtMs, receiptId, reading, and aperture in workspace-text-wsc-settlement.ts. As a result, jeditWscReplayCloseout(listJeditWscHistory(realStore)) always obstructs at the first missing stage even when applied and rejected WSC envelopes are retained; the spec passes only because it uses fake payload fields that production does not write.
Useful? React with 👍 / 👎.
| function readTextObject(keys: readonly string[], index: number): TokenRead | undefined { | ||
| const scope = textObjectScope(keys[index]); | ||
| const target = TEXT_OBJECTS.get(keys[index + 1] ?? ''); | ||
| if (scope == null || target == null) { | ||
| return undefined; | ||
| } | ||
| return singleToken(textObjectToken(scope, target, keys.slice(index, index + 2), index), index + 2); |
There was a problem hiding this comment.
When tokenizing a normal-mode sequence like ['i', 'w'] or ['a', 'p'], this reader runs before mode-switch parsing and consumes the pair as an inner word/around paragraph text object even though text objects are only valid after an operator or in visual/operator-pending context. That loses the i/a mode-switch token and will mis-model ordinary insert/append commands followed by typed keys; the text-object reader needs operator/visual context or should not greedily match at top level.
Useful? React with 👍 / 👎.
| export const TEXT_OBJECT_INNER_SCOPE: VimTextObjectScope = 'inner'; | ||
|
|
||
| export const REGISTER_NAMES = new Set('"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_-+*='.split('')); | ||
| export const LINEWISE_OPERATOR_KEYS = new Set(['c', 'd', 'y']); |
There was a problem hiding this comment.
When tokenizing Vim linewise operator commands such as >>, <<, ==, or !!, this allow-list only recognizes c, d, and y, so the tokenizer emits two operator tokens instead of an operator plus the lineCurrent motion. Downstream operator compilation will not be able to distinguish these common linewise commands from incomplete operator chains, so the repeated linewise operators need to be included here or handled with an equivalent multi-key rule.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/BEARING.md`:
- Around line 74-81: The main progress ledger still shows slices 127-136 as
unchecked while the narrative and sub-checklists mark them complete; update the
top-level progress ledger entries for slices 127-136 to checked ([x]) so the
authoritative progress surface matches the detailed slice closeouts, and commit
that change alongside the slice closeout to satisfy the guideline about checking
off items just before committing.
In `@scripts/jedit-wsc-history.mjs`:
- Around line 86-106: The materialize function currently calls
Buffer.from(envelope.bytes).toString(UTF8_ENCODING) and JSON.parse(...) without
protection; wrap the entire parsing and payload validation (the
Buffer.from(envelope.bytes), .toString, and JSON.parse steps that produce
`payload`) in a try-catch, and on any error return the same obstruction shape
used elsewhere (status:
ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED and obstruction.code:
ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED with a descriptive
message) instead of throwing; keep downstream logic (setting this.artifact and
returning JEDIT_WSC_CURRENT_HISTORY_MATERIALIZED) unchanged when parsing
succeeds.
In `@spec/vim-power-grammar.spec.mjs`:
- Around line 19-29: The test 'Vim grammar token kind constants are stable for
agent witnesses' only asserts 7 of 13 VimGrammarTokenKinds; add assertions for
the missing discriminants (CommandLineText, ModeSwitch, Prefix, Unknown,
VisualPrefix) inside that same test so all token kinds are verified for
stability, referencing grammar.VimGrammarTokenKinds.CommandLineText,
.ModeSwitch, .Prefix, .Unknown and .VisualPrefix and asserting they equal their
expected string discriminants.
In `@spec/vim-power-parity-matrix.spec.mjs`:
- Around line 125-133: The check in assertTargetSlices uses magic numbers 2 and
30 for the WF-0105 allowed slice range; define named constants (e.g.,
WF_0105_MIN_SLICE = 2 and WF_0105_MAX_SLICE = 30) at the top of the spec file
and replace the hardcoded values in assertTargetSlices with those constants
(update the assertion slice >= WF_0105_MIN_SLICE && slice <= WF_0105_MAX_SLICE)
so the constraint is explicit and maintainable.
- Around line 6-8: Replace the locally defined REPO_ROOT in
spec/vim-power-parity-matrix.spec.mjs with the shared constant exported by
spec/dist-helpers.mjs: remove the line "const REPO_ROOT = process.cwd();" and
instead import { REPO_ROOT } from the dist-helpers.mjs module at the top of the
file, keeping MATRIX_PATH and DESIGN_PATH usage unchanged so all specs use the
single shared REPO_ROOT constant.
In `@spec/vim-power-target-usability-fixtures.spec.mjs`:
- Around line 6-10: Replace the local REPO_ROOT declaration with an import from
the shared helper: remove the line that defines REPO_ROOT = process.cwd() and
instead import the exported REPO_ROOT from spec/dist-helpers.mjs at the top of
spec/vim-power-target-usability-fixtures.spec.mjs; keep the existing
FIXTURES_PATH, MATRIX_PATH, DESIGN_PATH and MATRIX_REPO_PATH variables but
construct them using the imported REPO_ROOT so all path constants
(FIXTURES_PATH, MATRIX_PATH) reference the shared REPO_ROOT symbol rather than a
duplicated local value.
- Around line 59-78: The test uses magic numbers for validation thresholds (40,
4, 5); replace them with named constants to make intent clear: define
MIN_HUMAN_TASK_LENGTH, MIN_WITNESS_STEPS, and MIN_REQUIRED_FACTS at the top of
the test (or file) and use them in the assertions that check
workflow.humanTask.length, workflow.witness.steps.length, and
workflow.witness.requiredFacts.length respectively; update the test named "Vim
power target usability fixtures separate human task prose from witness data" to
reference those constants when asserting minimums so the policy is explicit and
maintainable.
In `@src/app/jedit-wsc-history-listing.ts`:
- Around line 155-238: The literal 'rejected' used as a fallback in
rejectedRecord should be extracted to a module-level constant; add a constant
like DEFAULT_REJECTION_REASON = 'rejected' near the top of the file and replace
the inline fallback in rejectedRecord (the expression
String(payload.rejectionReason ?? payload.reason ?? 'rejected')) with
String(payload.rejectionReason ?? payload.reason ?? DEFAULT_REJECTION_REASON) so
the magic string is centralized and reusable.
In `@src/app/jedit-wsc-replay-proof.ts`:
- Around line 75-103: The function jeditWscReplayCloseout embeds literal
obstruction messages; extract those magic strings into named constants (e.g.,
CLOSEOUT_OBSTRUCTION_MISSING_STAGE_MESSAGE and
CLOSEOUT_OBSTRUCTION_MISSING_NON_APPLIED_MESSAGE) and use the constants in the
calls to closeoutObstructed instead of inline strings; update
jeditWscReplayCloseout to pass the appropriate constant (or a templated constant
+ missingStage.name if dynamic text is required) and add the new constant
declarations near the other module-level constants so all diagnostic messages
follow the "no magic strings" guideline.
In `@src/app/workspace/vim-grammar.ts`:
- Around line 253-259: Replace the ad-hoc lexicographic checks in isDigitKey and
isCountStartKey with explicit Set membership tests: add a DIGIT_KEYS Set
(containing '0'..'9') and a COUNT_START_KEYS Set (containing '1'..'9') to
vim-grammar-vocabulary.ts, import those constants into vim-grammar.ts, and
change isDigitKey(key: string) and isCountStartKey(key: string) to return
membership (DIGIT_KEYS.has(key) and COUNT_START_KEYS.has(key)) so the logic
follows the project's Set-based key-validation pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 996098f3-858f-442f-8783-a06ca65263f0
📒 Files selected for processing (25)
docs/BEARING.mddocs/design/0105-vim-power-moves-causal-parity.mddocs/design/0105-vim-power-moves-parity-matrix.jsondocs/design/0105-vim-power-target-usability-fixtures.jsondocs/design/0106-emacs-ideas-to-steal-causally.mddocs/echo-application-hosting-guide.mdscripts/jedit-echo-release-gate.mjsscripts/jedit-wsc-history.mjsspec/echo-application-hosting-guide.spec.mjsspec/fixtures/vim-power-grammar-snapshots.jsonspec/jedit-wsc-agent-history-cli.spec.mjsspec/jedit-wsc-current-history-export.spec.mjsspec/jedit-wsc-history-listing.spec.mjsspec/jedit-wsc-replay-proof.spec.mjsspec/release-gate-script.spec.mjsspec/vim-power-grammar.spec.mjsspec/vim-power-parity-matrix.spec.mjsspec/vim-power-target-usability-fixtures.spec.mjssrc/app/jedit-wsc-current-history-export.tssrc/app/jedit-wsc-history-listing.tssrc/app/jedit-wsc-replay-proof.tssrc/app/workspace/vim-grammar-vocabulary.tssrc/app/workspace/vim-grammar.tssrc/ports/jedit-wsc-history-listing.tssrc/ports/jedit-wsc-replay-proof.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test / cycle-proofs
- GitHub Check: test / echo-authority
- GitHub Check: test / contract-api
- GitHub Check: release-gate / jedit-echo
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # jedit Agent ContractGit Safety
- Never amend commits. Make a new commit.
- Never rebase unless the user explicitly approves it after a concrete explanation.
- Never force git operations.
- Draft pull requests are allowed only for design-cycle kickoff or active cycle
work. Convert them to ready before merge. If draft PRs are unavailable, use a
normal PR with thework-in-progresslabel.Quality Doctrine
These are hard repo rules, not suggestions:
- No
any.- No
unknown.- Hexagonal architecture, strict.
- Encoding and decoding happen only at the boundary, inside a port/adapter.
- One file equals one runtime truth.
- Runtime truth beats compile-time theater.
- No ad hoc string comparison in core logic. Prefer
instanceof, numeric tags, symbols, or explicit runtime objects.- No magic strings.
- No magic numbers.
- SOLID.
- DRY.
- DI.
- Red to green.
- Tests are spec.
- Agent-first.
- No file over 500 lines of code.
Structure
ARCHITECTURE.mdis the canonical repo architecture doctrine.docs/BEARING.mdrecords the current execution gravity.docs/method/process.mdis the canonical cycle workflow.docs/method/roadmap-planning.mddefines release-gate, roadmap, slice, and
proof policy for multi-cycle planning.docs/design/TEMPLATE.mdis the required template for full cycle designs.docs/design/0034-design-cycle-template-and-lifecycle.mdrecords the policy
decision that made executable design docs official.docs/design/0024-jedit-powered-by-echo-release-gate.mdis the completed
baseline plan for the first thirty slices proving jedit has an installed
package/evidence path while Echo remains generic.docs/design/0025-echo-application-hosting-pattern.mdis the completed
post-release-pressure ten-slice plan for proving the reusable Echo
application-hosting pattern before applying it to Graft, Think, or other apps.docs/design/0026-echo-hosting-hardening-first-twenty.mdis th...
Files:
spec/release-gate-script.spec.mjsspec/vim-power-grammar.spec.mjsspec/fixtures/vim-power-grammar-snapshots.jsonspec/echo-application-hosting-guide.spec.mjsscripts/jedit-echo-release-gate.mjsspec/vim-power-parity-matrix.spec.mjssrc/ports/jedit-wsc-history-listing.tssrc/ports/jedit-wsc-replay-proof.tsdocs/echo-application-hosting-guide.mdspec/jedit-wsc-replay-proof.spec.mjssrc/app/jedit-wsc-replay-proof.tsspec/jedit-wsc-current-history-export.spec.mjsdocs/design/0105-vim-power-target-usability-fixtures.jsonspec/jedit-wsc-agent-history-cli.spec.mjsspec/jedit-wsc-history-listing.spec.mjsscripts/jedit-wsc-history.mjsspec/vim-power-target-usability-fixtures.spec.mjsdocs/design/0106-emacs-ideas-to-steal-causally.mddocs/design/0105-vim-power-moves-parity-matrix.jsonsrc/app/workspace/vim-grammar-vocabulary.tssrc/app/jedit-wsc-history-listing.tssrc/app/workspace/vim-grammar.tsdocs/BEARING.mddocs/design/0105-vim-power-moves-causal-parity.mdsrc/app/jedit-wsc-current-history-export.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/ports/jedit-wsc-history-listing.tssrc/ports/jedit-wsc-replay-proof.tssrc/app/jedit-wsc-replay-proof.tssrc/app/workspace/vim-grammar-vocabulary.tssrc/app/jedit-wsc-history-listing.tssrc/app/workspace/vim-grammar.tssrc/app/jedit-wsc-current-history-export.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/ports/jedit-wsc-history-listing.tssrc/ports/jedit-wsc-replay-proof.tssrc/app/jedit-wsc-replay-proof.tssrc/app/workspace/vim-grammar-vocabulary.tssrc/app/jedit-wsc-history-listing.tssrc/app/workspace/vim-grammar.tssrc/app/jedit-wsc-current-history-export.ts
docs/design/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the template from
docs/design/TEMPLATE.mdfor all full cycle designs
Files:
docs/design/0106-emacs-ideas-to-steal-causally.mddocs/design/0105-vim-power-moves-causal-parity.md
docs/BEARING.md
📄 CodeRabbit inference engine (AGENTS.md)
Record current execution gravity in
docs/BEARING.md
Files:
docs/BEARING.md
🪛 LanguageTool
docs/design/0106-emacs-ideas-to-steal-causally.md
[style] ~106-~106: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t is the jedit-owned semantic model? 3. What Echo evidence, receipt, retained materi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~281-~281: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nal command transcripts with anchors. - Define debug traces with honest replay levels ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/design/0105-vim-power-moves-causal-parity.md
[style] ~105-~105: Consider using “incomplete” to avoid wordiness.
Context: ...rtant current caveats: - Vim parity is not complete. - Motions are mostly imperative UI cur...
(NOT_ABLE_PREMIUM)
[grammar] ~241-~241: Use a hyphen to join words.
Context: ...structed; - a command preview when an ex operation spans many edits; - an optiona...
(QB_NEW_EN_HYPHEN)
[grammar] ~555-~555: Use a hyphen to join words.
Context: ...or Parity Operators should compile over resolved targets: | Operator | Vim pari...
(QB_NEW_EN_HYPHEN)
[style] ~1205-~1205: Consider using “incomplete” to avoid wordiness.
Context: ...etrospective Roadmap implementation is not complete. Slices 2, 3, and 4 now have inspectabl...
(NOT_ABLE_PREMIUM)
🔇 Additional comments (38)
src/app/workspace/vim-grammar-vocabulary.ts (1)
1-291: LGTM!src/app/workspace/vim-grammar.ts (1)
1-252: LGTM!Also applies to: 260-325
spec/fixtures/vim-power-grammar-snapshots.json (1)
1-111: LGTM!spec/vim-power-grammar.spec.mjs (1)
11-17: LGTM!docs/echo-application-hosting-guide.md (2)
234-293: LGTM!
304-306: LGTM!docs/BEARING.md (2)
100-130: LGTM!
1735-1738: LGTM!Also applies to: 1763-1766, 1791-1794, 1818-1821, 1845-1848, 1872-1875, 1900-1903, 1928-1931, 1957-1960, 1984-1987
spec/echo-application-hosting-guide.spec.mjs (1)
18-23: LGTM!scripts/jedit-echo-release-gate.mjs (1)
43-47: LGTM!spec/release-gate-script.spec.mjs (1)
77-81: LGTM!src/ports/jedit-wsc-history-listing.ts (3)
1-11: LGTM!
13-36: LGTM!
38-66: LGTM!src/ports/jedit-wsc-replay-proof.ts (4)
1-11: LGTM!
13-47: LGTM!
49-77: LGTM!
79-92: LGTM!src/app/jedit-wsc-history-listing.ts (7)
1-42: LGTM!
44-106: LGTM!
108-127: LGTM!
129-153: LGTM!
240-266: LGTM!
268-322: LGTM!
324-369: LGTM!src/app/jedit-wsc-replay-proof.ts (4)
1-49: LGTM!
51-73: LGTM!
105-153: LGTM!
155-193: LGTM!src/app/jedit-wsc-current-history-export.ts (3)
44-47: LGTM!
187-196: LGTM!
200-210: LGTM!spec/jedit-wsc-agent-history-cli.spec.mjs (1)
1-123: LGTM!spec/jedit-wsc-history-listing.spec.mjs (1)
1-160: LGTM!spec/jedit-wsc-replay-proof.spec.mjs (1)
1-134: LGTM!spec/jedit-wsc-current-history-export.spec.mjs (1)
101-123: LGTM!Also applies to: 428-434
scripts/jedit-wsc-history.mjs (2)
30-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse strict equality.
Loose equality
==works here but violates modern JavaScript best practices. Strict equality makes null/undefined handling explicit.♻️ Prefer strict equality
- if (parsed.command == null || parsed.command.startsWith('--')) { + if (parsed.command === undefined || parsed.command.startsWith('--')) { throwUsage('missing command'); }As per coding guidelines: "No ad hoc string comparison in core logic; prefer instanceof, numeric tags, symbols, or explicit runtime objects" and general JavaScript best practices favor strict equality.
> Likely an incorrect or invalid review comment.Source: Coding guidelines
110-118: Confirmresult.filePathis part of the export contract.exportedBodyis only used whenresult.status === JEDIT_WSC_CURRENT_HISTORY_EXPORTED, and the correspondingJeditWscCurrentHistoryExportedtype requiresfilePath: string(populated asfilePath: artifact.filePathinexportJeditWscHistoryBasis), sopath: result.filePathcan’t come from a missing/renamed field.
| The current branch closes slices 127-136: WSC history listing now exposes | ||
| app-safe submissions, outcomes, receipts, readings, checkpoints, rejection | ||
| reasons, export refs, and typed missing-evidence posture; WSC replay compares | ||
| semantic evidence while excluding host timing; current export ignores retained | ||
| rejections for applied-text export; an agent JSON CLI lists and exports | ||
| historical bases; the release gate includes the WSC history/export/replay | ||
| witnesses; and the reusable Echo hosting guide carries the portability template | ||
| and Graft/Think readiness checklist. |
There was a problem hiding this comment.
Critical inconsistency: narrative claims completion but main ledger disagrees.
Lines 74-81 state "The current branch closes slices 127-136" with detailed accomplishments listed. However, the main progress ledger at lines 626-636 still shows these slices as unchecked [ ]. Only the sub-checklists within each detailed slice description (lines 1735-1987) were marked complete.
The coding guidelines require: "Keep checking off slice items just before committing the slice that closes them." The main ledger at lines 626-636 is the authoritative progress surface and must be updated to [x] for slices 127-136 to match this closeout narrative.
🔧 Proposed fix
Update the main progress ledger to mark slices 127-136 as complete:
- [x] 125 - Current History Export.
- [x] 126 - Point-In-Time Export.
-- [ ] 127 - History Listing And Evidence View.
-- [ ] 128 - Replay Same Edits Same Evidence.
-- [ ] 129 - Host Timing Permutation Proof.
-- [ ] 130 - Conflict And Rejection Retention.
-- [ ] 131 - Multi-File History Recovery.
-- [ ] 132 - Agent Historical Export.
-- [ ] 133 - WSC Release Gate Integration.
-- [ ] 134 - DIND Replay Closeout.
-- [ ] 135 - Portability Template Update.
-- [ ] 136 - Graft/Think Readiness Checklist.
+- [x] 127 - History Listing And Evidence View.
+- [x] 128 - Replay Same Edits Same Evidence.
+- [x] 129 - Host Timing Permutation Proof.
+- [x] 130 - Conflict And Rejection Retention.
+- [x] 131 - Multi-File History Recovery.
+- [x] 132 - Agent Historical Export.
+- [x] 133 - WSC Release Gate Integration.
+- [x] 134 - DIND Replay Closeout.
+- [x] 135 - Portability Template Update.
+- [x] 136 - Graft/Think Readiness Checklist.
- [ ] 137 - End-To-End Guide Refresh.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The current branch closes slices 127-136: WSC history listing now exposes | |
| app-safe submissions, outcomes, receipts, readings, checkpoints, rejection | |
| reasons, export refs, and typed missing-evidence posture; WSC replay compares | |
| semantic evidence while excluding host timing; current export ignores retained | |
| rejections for applied-text export; an agent JSON CLI lists and exports | |
| historical bases; the release gate includes the WSC history/export/replay | |
| witnesses; and the reusable Echo hosting guide carries the portability template | |
| and Graft/Think readiness checklist. | |
| - [x] 125 - Current History Export. | |
| - [x] 126 - Point-In-Time Export. | |
| - [x] 127 - History Listing And Evidence View. | |
| - [x] 128 - Replay Same Edits Same Evidence. | |
| - [x] 129 - Host Timing Permutation Proof. | |
| - [x] 130 - Conflict And Rejection Retention. | |
| - [x] 131 - Multi-File History Recovery. | |
| - [x] 132 - Agent Historical Export. | |
| - [x] 133 - WSC Release Gate Integration. | |
| - [x] 134 - DIND Replay Closeout. | |
| - [x] 135 - Portability Template Update. | |
| - [x] 136 - Graft/Think Readiness Checklist. | |
| - [ ] 137 - End-To-End Guide Refresh. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/BEARING.md` around lines 74 - 81, The main progress ledger still shows
slices 127-136 as unchecked while the narrative and sub-checklists mark them
complete; update the top-level progress ledger entries for slices 127-136 to
checked ([x]) so the authoritative progress surface matches the detailed slice
closeouts, and commit that change alongside the slice closeout to satisfy the
guideline about checking off items just before committing.
Source: Coding guidelines
| materialize(envelope) { | ||
| const payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING)); | ||
| if (payload?.schemaVersion !== SETTLEMENT_SCHEMA_VERSION || !Array.isArray(payload.reading?.lines)) { | ||
| return { | ||
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED, | ||
| obstruction: { | ||
| code: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED, | ||
| message: 'basis is not an applied WSC settlement envelope', | ||
| }, | ||
| }; | ||
| } | ||
| this.artifact = { | ||
| filePath: path.resolve(outputPath), | ||
| lines: payload.reading.lines, | ||
| readingId: String(payload.reading.readingId ?? ''), | ||
| }; | ||
| return { | ||
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZED, | ||
| artifact: this.artifact, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Unguarded JSON parsing will crash the CLI on malformed envelopes.
Line 87 chains three operations that can throw:
Buffer.from(envelope.bytes)if bytes are invalid.toString(UTF8_ENCODING)on malformed UTF-8JSON.parse()on invalid JSON
When an envelope is corrupted or tampered with, the materialize function will propagate an unhandled exception, killing the CLI process without a typed obstruction response.
🛡️ Wrap parsing in try-catch and return obstruction
materialize(envelope) {
+ let payload;
+ try {
+ payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING));
+ } catch (error) {
+ return {
+ status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED,
+ obstruction: {
+ code: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED,
+ message: 'basis envelope is malformed or unreadable',
+ },
+ };
+ }
- const payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING));
if (payload?.schemaVersion !== SETTLEMENT_SCHEMA_VERSION || !Array.isArray(payload.reading?.lines)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| materialize(envelope) { | |
| const payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING)); | |
| if (payload?.schemaVersion !== SETTLEMENT_SCHEMA_VERSION || !Array.isArray(payload.reading?.lines)) { | |
| return { | |
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED, | |
| obstruction: { | |
| code: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED, | |
| message: 'basis is not an applied WSC settlement envelope', | |
| }, | |
| }; | |
| } | |
| this.artifact = { | |
| filePath: path.resolve(outputPath), | |
| lines: payload.reading.lines, | |
| readingId: String(payload.reading.readingId ?? ''), | |
| }; | |
| return { | |
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZED, | |
| artifact: this.artifact, | |
| }; | |
| }, | |
| materialize(envelope) { | |
| let payload; | |
| try { | |
| payload = JSON.parse(Buffer.from(envelope.bytes).toString(UTF8_ENCODING)); | |
| } catch (error) { | |
| return { | |
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED, | |
| obstruction: { | |
| code: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED, | |
| message: 'basis envelope is malformed or unreadable', | |
| }, | |
| }; | |
| } | |
| if (payload?.schemaVersion !== SETTLEMENT_SCHEMA_VERSION || !Array.isArray(payload.reading?.lines)) { | |
| return { | |
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED, | |
| obstruction: { | |
| code: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED, | |
| message: 'basis is not an applied WSC settlement envelope', | |
| }, | |
| }; | |
| } | |
| this.artifact = { | |
| filePath: path.resolve(outputPath), | |
| lines: payload.reading.lines, | |
| readingId: String(payload.reading.readingId ?? ''), | |
| }; | |
| return { | |
| status: ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZED, | |
| artifact: this.artifact, | |
| }; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/jedit-wsc-history.mjs` around lines 86 - 106, The materialize
function currently calls Buffer.from(envelope.bytes).toString(UTF8_ENCODING) and
JSON.parse(...) without protection; wrap the entire parsing and payload
validation (the Buffer.from(envelope.bytes), .toString, and JSON.parse steps
that produce `payload`) in a try-catch, and on any error return the same
obstruction shape used elsewhere (status:
ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_OBSTRUCTED and obstruction.code:
ports.JEDIT_WSC_CURRENT_HISTORY_MATERIALIZATION_FAILED with a descriptive
message) instead of throwing; keep downstream logic (setting this.artifact and
returning JEDIT_WSC_CURRENT_HISTORY_MATERIALIZED) unchanged when parsing
succeeds.
| test('Vim grammar token kind constants are stable for agent witnesses', async () => { | ||
| const grammar = await importDist('app', 'workspace', 'vim-grammar.js'); | ||
|
|
||
| assert.equal(grammar.VimGrammarTokenKinds.Count, 'count'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.Register, 'register'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.Operator, 'operator'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.Motion, 'motion'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.TextObject, 'textObject'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.CommandLineInvocation, 'commandLineInvocation'); | ||
| assert.equal(grammar.VimGrammarTokenKinds.MacroControl, 'macroControl'); | ||
| }); |
There was a problem hiding this comment.
Test incomplete: 6 of 13 token kinds not verified for stability.
The test title claims to verify token kind constants are "stable for agent witnesses," but only checks 7 of the 13 VimGrammarTokenKind discriminants defined in the vocabulary. Missing assertions for:
CommandLineTextModeSwitchPrefixUnknownVisualPrefix
If agent witness stability matters, all token kinds should be tested. Otherwise, a refactor could silently change these discriminant values and break agents.
🧪 Add missing stability assertions
assert.equal(grammar.VimGrammarTokenKinds.TextObject, 'textObject');
assert.equal(grammar.VimGrammarTokenKinds.CommandLineInvocation, 'commandLineInvocation');
+ assert.equal(grammar.VimGrammarTokenKinds.CommandLineText, 'commandLineText');
assert.equal(grammar.VimGrammarTokenKinds.MacroControl, 'macroControl');
+ assert.equal(grammar.VimGrammarTokenKinds.ModeSwitch, 'modeSwitch');
+ assert.equal(grammar.VimGrammarTokenKinds.Prefix, 'prefix');
+ assert.equal(grammar.VimGrammarTokenKinds.Unknown, 'unknown');
+ assert.equal(grammar.VimGrammarTokenKinds.VisualPrefix, 'visualPrefix');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('Vim grammar token kind constants are stable for agent witnesses', async () => { | |
| const grammar = await importDist('app', 'workspace', 'vim-grammar.js'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Count, 'count'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Register, 'register'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Operator, 'operator'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Motion, 'motion'); | |
| assert.equal(grammar.VimGrammarTokenKinds.TextObject, 'textObject'); | |
| assert.equal(grammar.VimGrammarTokenKinds.CommandLineInvocation, 'commandLineInvocation'); | |
| assert.equal(grammar.VimGrammarTokenKinds.MacroControl, 'macroControl'); | |
| }); | |
| test('Vim grammar token kind constants are stable for agent witnesses', async () => { | |
| const grammar = await importDist('app', 'workspace', 'vim-grammar.js'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Count, 'count'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Register, 'register'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Operator, 'operator'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Motion, 'motion'); | |
| assert.equal(grammar.VimGrammarTokenKinds.TextObject, 'textObject'); | |
| assert.equal(grammar.VimGrammarTokenKinds.CommandLineInvocation, 'commandLineInvocation'); | |
| assert.equal(grammar.VimGrammarTokenKinds.CommandLineText, 'commandLineText'); | |
| assert.equal(grammar.VimGrammarTokenKinds.MacroControl, 'macroControl'); | |
| assert.equal(grammar.VimGrammarTokenKinds.ModeSwitch, 'modeSwitch'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Prefix, 'prefix'); | |
| assert.equal(grammar.VimGrammarTokenKinds.Unknown, 'unknown'); | |
| assert.equal(grammar.VimGrammarTokenKinds.VisualPrefix, 'visualPrefix'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/vim-power-grammar.spec.mjs` around lines 19 - 29, The test 'Vim grammar
token kind constants are stable for agent witnesses' only asserts 7 of 13
VimGrammarTokenKinds; add assertions for the missing discriminants
(CommandLineText, ModeSwitch, Prefix, Unknown, VisualPrefix) inside that same
test so all token kinds are verified for stability, referencing
grammar.VimGrammarTokenKinds.CommandLineText, .ModeSwitch, .Prefix, .Unknown and
.VisualPrefix and asserting they equal their expected string discriminants.
| const REPO_ROOT = process.cwd(); | ||
| const MATRIX_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-moves-parity-matrix.json'); | ||
| const DESIGN_PATH = 'docs/design/0105-vim-power-moves-causal-parity.md'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extract REPO_ROOT from the existing dist-helpers.mjs module to follow DRY.
The constant REPO_ROOT is defined here as process.cwd(), but the relevant code snippet shows that spec/dist-helpers.mjs already exports this exact constant. Importing it from the shared module eliminates duplication and ensures consistent path resolution across all specs.
♻️ Refactor to import shared constant
import assert from 'node:assert/strict';
import { readFileSync } from 'node:fs';
import path from 'node:path';
import test from 'node:test';
+import { REPO_ROOT } from './dist-helpers.mjs';
-const REPO_ROOT = process.cwd();
const MATRIX_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-moves-parity-matrix.json');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/vim-power-parity-matrix.spec.mjs` around lines 6 - 8, Replace the
locally defined REPO_ROOT in spec/vim-power-parity-matrix.spec.mjs with the
shared constant exported by spec/dist-helpers.mjs: remove the line "const
REPO_ROOT = process.cwd();" and instead import { REPO_ROOT } from the
dist-helpers.mjs module at the top of the file, keeping MATRIX_PATH and
DESIGN_PATH usage unchanged so all specs use the single shared REPO_ROOT
constant.
| function assertTargetSlices(item) { | ||
| assert.ok(Array.isArray(item.targetSlices), `${item.id} targetSlices must be an array`); | ||
| assert.ok(item.targetSlices.length > 0, `${item.id} targetSlices must not be empty`); | ||
|
|
||
| for (const slice of item.targetSlices) { | ||
| assert.equal(Number.isInteger(slice), true, `${item.id} target slice must be an integer`); | ||
| assert.ok(slice >= 2 && slice <= 30, `${item.id} target slice must fit WF-0105`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace magic numbers with named constants for WF-0105 slice range boundaries.
The hardcoded values 2 and 30 on line 131 represent the valid slice range for the WF-0105 design cycle. These are magic numbers. Extract them as named constants at the top of the file (e.g., WF_0105_MIN_SLICE = 2, WF_0105_MAX_SLICE = 30) to make the constraint explicit and maintainable. As per coding guidelines, no magic numbers should appear in code.
♻️ Extract slice range constants
+const WF_0105_MIN_SLICE = 2;
+const WF_0105_MAX_SLICE = 30;
+
const STATUS_VOCABULARY = Object.freeze([
'supported',
'partial',
'unsupported',
'causally-enhanced',
]);
// ... later in assertTargetSlices:
function assertTargetSlices(item) {
assert.ok(Array.isArray(item.targetSlices), `${item.id} targetSlices must be an array`);
assert.ok(item.targetSlices.length > 0, `${item.id} targetSlices must not be empty`);
for (const slice of item.targetSlices) {
assert.equal(Number.isInteger(slice), true, `${item.id} target slice must be an integer`);
- assert.ok(slice >= 2 && slice <= 30, `${item.id} target slice must fit WF-0105`);
+ assert.ok(slice >= WF_0105_MIN_SLICE && slice <= WF_0105_MAX_SLICE, `${item.id} target slice must fit WF-0105 range [${WF_0105_MIN_SLICE}, ${WF_0105_MAX_SLICE}]`);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/vim-power-parity-matrix.spec.mjs` around lines 125 - 133, The check in
assertTargetSlices uses magic numbers 2 and 30 for the WF-0105 allowed slice
range; define named constants (e.g., WF_0105_MIN_SLICE = 2 and WF_0105_MAX_SLICE
= 30) at the top of the spec file and replace the hardcoded values in
assertTargetSlices with those constants (update the assertion slice >=
WF_0105_MIN_SLICE && slice <= WF_0105_MAX_SLICE) so the constraint is explicit
and maintainable.
Source: Coding guidelines
| const REPO_ROOT = process.cwd(); | ||
| const FIXTURES_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-target-usability-fixtures.json'); | ||
| const MATRIX_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-moves-parity-matrix.json'); | ||
| const DESIGN_PATH = 'docs/design/0105-vim-power-moves-causal-parity.md'; | ||
| const MATRIX_REPO_PATH = 'docs/design/0105-vim-power-moves-parity-matrix.json'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Import REPO_ROOT from the shared dist-helpers.mjs module to eliminate duplication.
This file redefines REPO_ROOT as process.cwd(), duplicating the constant already exported by spec/dist-helpers.mjs (per the relevant code snippet). Import the shared constant to follow DRY principles and ensure consistent path resolution across the test suite.
♻️ Import shared REPO_ROOT
import assert from 'node:assert/strict';
import { readFileSync } from 'node:fs';
import path from 'node:path';
import test from 'node:test';
+import { REPO_ROOT } from './dist-helpers.mjs';
-const REPO_ROOT = process.cwd();
const FIXTURES_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-target-usability-fixtures.json');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const REPO_ROOT = process.cwd(); | |
| const FIXTURES_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-target-usability-fixtures.json'); | |
| const MATRIX_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-moves-parity-matrix.json'); | |
| const DESIGN_PATH = 'docs/design/0105-vim-power-moves-causal-parity.md'; | |
| const MATRIX_REPO_PATH = 'docs/design/0105-vim-power-moves-parity-matrix.json'; | |
| import assert from 'node:assert/strict'; | |
| import { readFileSync } from 'node:fs'; | |
| import path from 'node:path'; | |
| import test from 'node:test'; | |
| import { REPO_ROOT } from './dist-helpers.mjs'; | |
| const FIXTURES_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-target-usability-fixtures.json'); | |
| const MATRIX_PATH = path.join(REPO_ROOT, 'docs', 'design', '0105-vim-power-moves-parity-matrix.json'); | |
| const DESIGN_PATH = 'docs/design/0105-vim-power-moves-causal-parity.md'; | |
| const MATRIX_REPO_PATH = 'docs/design/0105-vim-power-moves-parity-matrix.json'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/vim-power-target-usability-fixtures.spec.mjs` around lines 6 - 10,
Replace the local REPO_ROOT declaration with an import from the shared helper:
remove the line that defines REPO_ROOT = process.cwd() and instead import the
exported REPO_ROOT from spec/dist-helpers.mjs at the top of
spec/vim-power-target-usability-fixtures.spec.mjs; keep the existing
FIXTURES_PATH, MATRIX_PATH, DESIGN_PATH and MATRIX_REPO_PATH variables but
construct them using the imported REPO_ROOT so all path constants
(FIXTURES_PATH, MATRIX_PATH) reference the shared REPO_ROOT symbol rather than a
duplicated local value.
| test('Vim power target usability fixtures separate human task prose from witness data', () => { | ||
| const fixtures = readJson(FIXTURES_PATH); | ||
|
|
||
| for (const workflow of fixtures.workflows) { | ||
| assert.equal(typeof workflow.humanTask, 'string', `${workflow.id} humanTask missing`); | ||
| assert.ok(workflow.humanTask.length > 40, `${workflow.id} humanTask should be descriptive`); | ||
| assert.equal(typeof workflow.witness.command, 'string', `${workflow.id} witness command missing`); | ||
| assert.match(workflow.witness.command, /jedit-vim-power-witness\.mjs --json/); | ||
| assert.equal(typeof workflow.witness.workspace.cwd, 'string', `${workflow.id} workspace cwd missing`); | ||
| assert.ok(workflow.witness.workspace.files.length > 0, `${workflow.id} needs seed files`); | ||
| assert.ok(workflow.witness.steps.length >= 4, `${workflow.id} needs enough witness steps`); | ||
| assert.ok(workflow.witness.requiredFacts.length >= 5, `${workflow.id} needs required facts`); | ||
|
|
||
| for (const step of workflow.witness.steps) { | ||
| assert.equal(typeof step.id, 'string', `${workflow.id} step id missing`); | ||
| assert.equal(typeof step.input, 'string', `${workflow.id}:${step.id} input missing`); | ||
| assert.equal(typeof step.expect, 'string', `${workflow.id}:${step.id} expectation missing`); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract magic numbers for workflow validation thresholds into named constants.
Lines 64, 69, and 70 contain magic numbers (40, 4, 5) representing minimum quality thresholds for workflow fixtures. These should be named constants (e.g., MIN_HUMAN_TASK_LENGTH, MIN_WITNESS_STEPS, MIN_REQUIRED_FACTS) to make the validation policy explicit and maintainable. As per coding guidelines, magic numbers should not appear in code.
♻️ Extract validation threshold constants
+const MIN_HUMAN_TASK_LENGTH = 40;
+const MIN_WITNESS_STEPS = 4;
+const MIN_REQUIRED_FACTS = 5;
+
const REQUIRED_WORKFLOWS = Object.freeze([
'open-edit-save-reopen',
'text-object-surgery',
// ... later in the test:
test('Vim power target usability fixtures separate human task prose from witness data', () => {
const fixtures = readJson(FIXTURES_PATH);
for (const workflow of fixtures.workflows) {
assert.equal(typeof workflow.humanTask, 'string', `${workflow.id} humanTask missing`);
- assert.ok(workflow.humanTask.length > 40, `${workflow.id} humanTask should be descriptive`);
+ assert.ok(workflow.humanTask.length > MIN_HUMAN_TASK_LENGTH, `${workflow.id} humanTask should be descriptive (min ${MIN_HUMAN_TASK_LENGTH} chars)`);
assert.equal(typeof workflow.witness.command, 'string', `${workflow.id} witness command missing`);
assert.match(workflow.witness.command, /jedit-vim-power-witness\.mjs --json/);
assert.equal(typeof workflow.witness.workspace.cwd, 'string', `${workflow.id} workspace cwd missing`);
assert.ok(workflow.witness.workspace.files.length > 0, `${workflow.id} needs seed files`);
- assert.ok(workflow.witness.steps.length >= 4, `${workflow.id} needs enough witness steps`);
- assert.ok(workflow.witness.requiredFacts.length >= 5, `${workflow.id} needs required facts`);
+ assert.ok(workflow.witness.steps.length >= MIN_WITNESS_STEPS, `${workflow.id} needs at least ${MIN_WITNESS_STEPS} witness steps`);
+ assert.ok(workflow.witness.requiredFacts.length >= MIN_REQUIRED_FACTS, `${workflow.id} needs at least ${MIN_REQUIRED_FACTS} required facts`);
for (const step of workflow.witness.steps) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/vim-power-target-usability-fixtures.spec.mjs` around lines 59 - 78, The
test uses magic numbers for validation thresholds (40, 4, 5); replace them with
named constants to make intent clear: define MIN_HUMAN_TASK_LENGTH,
MIN_WITNESS_STEPS, and MIN_REQUIRED_FACTS at the top of the test (or file) and
use them in the assertions that check workflow.humanTask.length,
workflow.witness.steps.length, and workflow.witness.requiredFacts.length
respectively; update the test named "Vim power target usability fixtures
separate human task prose from witness data" to reference those constants when
asserting minimums so the policy is explicit and maintainable.
Source: Coding guidelines
| function appliedRecord( | ||
| envelopeId: string, | ||
| payload: JeditWscHistoryEnvelopePayload, | ||
| ): JeditWscHistoryRecordDraft { | ||
| const readingId = payload.reading?.readingId; | ||
| return { | ||
| basisId: envelopeId, | ||
| envelopeId, | ||
| outcomeStatus: JEDIT_WSC_HISTORY_APPLIED, | ||
| evidencePosture: JEDIT_WSC_HISTORY_SETTLEMENT_EVIDENCE, | ||
| ...commonPayloadFields(payload), | ||
| readingId, | ||
| readingLineCount: payload.reading?.lineCount, | ||
| readingTextDigest: readingDigest(payload.reading?.lines), | ||
| exportEvidenceId: readingId != null | ||
| ? `${JEDIT_WSC_CURRENT_HISTORY_EXPORT_EVIDENCE_PREFIX}${envelopeId}:${readingId}` | ||
| : undefined, | ||
| }; | ||
| } | ||
|
|
||
| function rejectedRecord( | ||
| envelopeId: string, | ||
| payload: JeditWscHistoryEnvelopePayload, | ||
| ): JeditWscHistoryRecordDraft { | ||
| return { | ||
| basisId: envelopeId, | ||
| envelopeId, | ||
| outcomeStatus: JEDIT_WSC_HISTORY_REJECTED, | ||
| evidencePosture: JEDIT_WSC_HISTORY_REJECTION_EVIDENCE, | ||
| ...commonPayloadFields(payload), | ||
| rejectionReason: String(payload.rejectionReason ?? payload.reason ?? 'rejected'), | ||
| }; | ||
| } | ||
|
|
||
| function commonPayloadFields(payload: JeditWscHistoryEnvelopePayload): JeditWscHistoryCommonFields { | ||
| return { | ||
| filePath: payload.filePath, | ||
| bufferId: payload.bufferId, | ||
| commandKind: payload.commandKind, | ||
| submittedAtMs: payload.submittedAtMs, | ||
| receiptId: payload.receiptId, | ||
| checkpointId: payload.checkpointId, | ||
| submissionId: payload.submissionId, | ||
| admissionId: payload.admissionId, | ||
| ticketId: payload.ticketId, | ||
| executionId: payload.executionId, | ||
| }; | ||
| } | ||
|
|
||
| function missingEvidenceRecord( | ||
| envelopeId: string, | ||
| obstruction: JeditWscWorkspaceStoreObstruction, | ||
| ): JeditWscHistoryRecordDraft { | ||
| return { | ||
| basisId: envelopeId, | ||
| envelopeId, | ||
| outcomeStatus: JEDIT_WSC_HISTORY_MISSING_EVIDENCE, | ||
| evidencePosture: JEDIT_WSC_HISTORY_READ_OBSTRUCTED_EVIDENCE, | ||
| obstructionCode: obstruction.code, | ||
| obstructionMessage: obstruction.message, | ||
| }; | ||
| } | ||
|
|
||
| function unsupportedEvidenceRecord(envelopeId: string): JeditWscHistoryRecordDraft { | ||
| return { | ||
| basisId: envelopeId, | ||
| envelopeId, | ||
| outcomeStatus: JEDIT_WSC_HISTORY_MISSING_EVIDENCE, | ||
| evidencePosture: JEDIT_WSC_HISTORY_UNSUPPORTED_EVIDENCE, | ||
| obstructionCode: JEDIT_WSC_HISTORY_UNSUPPORTED_EVIDENCE, | ||
| obstructionMessage: `unsupported WSC envelope schema: ${envelopeId}`, | ||
| }; | ||
| } | ||
|
|
||
| function malformedEvidenceRecord(envelopeId: string, message: string): JeditWscHistoryRecordDraft { | ||
| return { | ||
| basisId: envelopeId, | ||
| envelopeId, | ||
| outcomeStatus: JEDIT_WSC_HISTORY_MISSING_EVIDENCE, | ||
| evidencePosture: JEDIT_WSC_HISTORY_MALFORMED_EVIDENCE, | ||
| obstructionCode: JEDIT_WSC_HISTORY_MALFORMED_EVIDENCE, | ||
| obstructionMessage: message, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider extracting the default rejection reason literal.
Line 185: The fallback string 'rejected' is used when both rejectionReason and reason fields are missing. Per coding guidelines, this is a magic string in application logic. Extract it to a named constant like DEFAULT_REJECTION_REASON = 'rejected' at the module level.
♻️ Proposed fix
Add constant near the top of the file (after line 42):
+const DEFAULT_REJECTION_REASON = 'rejected';Then update line 185:
- rejectionReason: String(payload.rejectionReason ?? payload.reason ?? 'rejected'),
+ rejectionReason: String(payload.rejectionReason ?? payload.reason ?? DEFAULT_REJECTION_REASON),As per coding guidelines: "No magic strings in code" applies to **/*.{ts,tsx,js} files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/jedit-wsc-history-listing.ts` around lines 155 - 238, The literal
'rejected' used as a fallback in rejectedRecord should be extracted to a
module-level constant; add a constant like DEFAULT_REJECTION_REASON = 'rejected'
near the top of the file and replace the inline fallback in rejectedRecord (the
expression String(payload.rejectionReason ?? payload.reason ?? 'rejected')) with
String(payload.rejectionReason ?? payload.reason ?? DEFAULT_REJECTION_REASON) so
the magic string is centralized and reusable.
Source: Coding guidelines
| export function jeditWscReplayCloseout( | ||
| history: JeditWscHistoryListed, | ||
| ): JeditWscReplayCloseout { | ||
| const evidenceCoordinates = history.records.map(evidenceCoordinate); | ||
| const coveredStages = CLOSEOUT_STAGES | ||
| .filter((stage) => history.records.some(stage.coveredBy)) | ||
| .map((stage) => stage.name); | ||
| const missingStage = CLOSEOUT_STAGES.find((stage) => !coveredStages.includes(stage.name)); | ||
| if (missingStage != null) { | ||
| return closeoutObstructed(JEDIT_WSC_REPLAY_MISSING_STAGE_COVERAGE, missingStage.name, evidenceCoordinates); | ||
| } | ||
| const nonAppliedOutcomeCount = history.records | ||
| .filter((record) => record.outcomeStatus !== JEDIT_WSC_HISTORY_APPLIED) | ||
| .length; | ||
| if (nonAppliedOutcomeCount === 0) { | ||
| return closeoutObstructed( | ||
| JEDIT_WSC_REPLAY_MISSING_NON_APPLIED_OUTCOME, | ||
| 'replay closeout requires at least one retained non-applied outcome', | ||
| evidenceCoordinates, | ||
| ); | ||
| } | ||
| return { | ||
| status: JEDIT_WSC_REPLAY_CLOSEOUT_READY, | ||
| evidenceCoordinates, | ||
| coveredStages, | ||
| nonAppliedOutcomeCount, | ||
| deterministicOnCleanCheckout: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider extracting obstruction messages to constants.
Lines 84 and 92: The obstruction message strings are embedded directly in the closeout logic. While these are diagnostic messages rather than protocol discriminators, the coding guidelines specify "No magic strings in code" for all **/*.{ts,tsx} files. Extract these to named constants like CLOSEOUT_OBSTRUCTION_MISSING_STAGE_MESSAGE and CLOSEOUT_OBSTRUCTION_MISSING_NON_APPLIED_MESSAGE for consistency and maintainability.
♻️ Proposed fix
Add constants after line 21:
const WALL_CLOCK_CADENCE_IS_SEMANTIC = false;
+const CLOSEOUT_OBSTRUCTION_MISSING_NON_APPLIED_MESSAGE = 'replay closeout requires at least one retained non-applied outcome';Then update lines 84 and 92:
if (missingStage != null) {
- return closeoutObstructed(JEDIT_WSC_REPLAY_MISSING_STAGE_COVERAGE, missingStage.name, evidenceCoordinates);
+ return closeoutObstructed(JEDIT_WSC_REPLAY_MISSING_STAGE_COVERAGE, `missing stage: ${missingStage.name}`, evidenceCoordinates);
}
const nonAppliedOutcomeCount = history.records
.filter((record) => record.outcomeStatus !== JEDIT_WSC_HISTORY_APPLIED)
.length;
if (nonAppliedOutcomeCount === 0) {
return closeoutObstructed(
JEDIT_WSC_REPLAY_MISSING_NON_APPLIED_OUTCOME,
- 'replay closeout requires at least one retained non-applied outcome',
+ CLOSEOUT_OBSTRUCTION_MISSING_NON_APPLIED_MESSAGE,
evidenceCoordinates,
);
}Note: Line 84's current behavior passes missingStage.name as the message, which is just the stage name. If this is intentional, consider a template constant or leaving as-is with a comment explaining the dynamic message generation.
As per coding guidelines: "No magic strings in code" applies to **/*.{ts,tsx} files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/jedit-wsc-replay-proof.ts` around lines 75 - 103, The function
jeditWscReplayCloseout embeds literal obstruction messages; extract those magic
strings into named constants (e.g., CLOSEOUT_OBSTRUCTION_MISSING_STAGE_MESSAGE
and CLOSEOUT_OBSTRUCTION_MISSING_NON_APPLIED_MESSAGE) and use the constants in
the calls to closeoutObstructed instead of inline strings; update
jeditWscReplayCloseout to pass the appropriate constant (or a templated constant
+ missingStage.name if dynamic text is required) and add the new constant
declarations near the other module-level constants so all diagnostic messages
follow the "no magic strings" guideline.
Source: Coding guidelines
| function isDigitKey(key: string): boolean { | ||
| return key >= ZERO_KEY && key <= COUNT_MAX; | ||
| } | ||
|
|
||
| function isCountStartKey(key: string): boolean { | ||
| return key >= COUNT_MIN && key <= COUNT_MAX; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace ad hoc string range comparison with Set membership checks.
These functions use lexicographic string comparison (key >= '0' && key <= '9') to validate digit characters. This violates the Quality Doctrine guideline: "No ad hoc string comparison in core logic." The rest of the codebase consistently uses Sets for key membership checks (e.g., REGISTER_NAMES, INCOMPLETE_PREFIX_KEYS). As per coding guidelines, "No ad hoc string comparison in core logic; prefer instanceof, numeric tags, symbols, or explicit runtime objects."
♻️ Refactor to use Set-based membership checks
Add these constants to vim-grammar-vocabulary.ts:
export const REGISTER_NAMES = new Set('"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_-+*='.split(''));
+export const DIGIT_KEYS = new Set(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']);
+export const COUNT_START_KEYS = new Set(['1', '2', '3', '4', '5', '6', '7', '8', '9']);
export const LINEWISE_OPERATOR_KEYS = new Set(['c', 'd', 'y']);Update imports in vim-grammar.ts:
import {
COMMAND_LINE_PREFIX,
- COUNT_MAX,
- COUNT_MIN,
+ COUNT_START_KEYS,
+ DIGIT_KEYS,
DOUBLE_MOTIONS,Apply this refactor:
function isDigitKey(key: string): boolean {
- return key >= ZERO_KEY && key <= COUNT_MAX;
+ return DIGIT_KEYS.has(key);
}
function isCountStartKey(key: string): boolean {
- return key >= COUNT_MIN && key <= COUNT_MAX;
+ return COUNT_START_KEYS.has(key);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/workspace/vim-grammar.ts` around lines 253 - 259, Replace the ad-hoc
lexicographic checks in isDigitKey and isCountStartKey with explicit Set
membership tests: add a DIGIT_KEYS Set (containing '0'..'9') and a
COUNT_START_KEYS Set (containing '1'..'9') to vim-grammar-vocabulary.ts, import
those constants into vim-grammar.ts, and change isDigitKey(key: string) and
isCountStartKey(key: string) to return membership (DIGIT_KEYS.has(key) and
COUNT_START_KEYS.has(key)) so the logic follows the project's Set-based
key-validation pattern.
Source: Coding guidelines
1 participant
Summary
Validation