next
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Jakub Zych
|
a9a7a328e6 |
refactor: hard-fork GSD -> MSD (Make Software Done)
Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted. |
||
|
sim
|
6f0e5ccf85 |
fix(#4636,#4653): close the symlink hole, revert a wrong collapse, fix six review findings
The RED checkpoint and two orthogonal reviews found eight defects. All fixed here.
THE COLLAPSE THAT WAS WRONG — installer-migrations. Routing ensureInsideConfig's
containment decision through the realpath-based canonical predicate broke four
tests, and the failure message says it plainly: "migration path escapes
configDir: extensions/gsd.cjs". That module's entire contract is that a
symlinked managed path is snapshotted, restored and backed up AS A LINK and
never dereferenced. The canonical predicate dereferences, then rejects the
result for escaping configDir — so it destroys exactly the thing the module
exists to preserve. Reverted to lexical, with the ruling recorded above the
function so it is not collapsed a third time. normalizeRelPath is the real
pre-gate there; it throws on absolute paths and '..' before this check runs.
That makes THREE deliberately-retained implementations, not two, and they share
one shape worth naming: a realpath-based predicate is the wrong tool wherever a
symlink must be PRESERVED rather than resolved. CONTEXT.md and
docs/explanation/security-model.md are corrected — both previously described
ensureInsideConfig as collapsed.
THE MISSED CONSUMER. tests/security-prompt-injection.security.test.cjs
destructures validatePath from the compiled lib; un-exporting it turned five
tests into TypeError. It appeared in my own earlier search output and I did not
follow it up. Translated under the same rule as the rest: assertions on the
rejection REASON go through assertWithinRoot, boolean-only through
tryWithinRoot.
VALIDATE-ONE-PATH-USE-ANOTHER, FOUND TWICE MORE. This is the fourth and fifth
occurrence in this epic of the exact defect it exists to prevent.
- scripts/check-glossary-refs.cjs decided containment on `token` and then
stat'd a separately re-joined path.join(ROOT, token). The ContainedPath is
now carried through to the probe, so the validated value is the probed one.
- src/init.cts computed skillPathContained and DISCARDED it, re-joining from
the raw input for the existsSync and read. The branded type exists to make
that a type error and here it was inert.
AND THE OVER-CORRECTION OF THAT FIX, caught before it shipped. The first attempt
also substituted the validated value into the EMITTED `ref` for a global skill.
That value is a display token, not a path anything reads through — the only fs
access in that branch runs on the lexical path beforehand — so substituting it
changed emitted output two ways: it is realpath-resolved, so a symlinked global
skills directory would have emitted its resolved target instead of the user's
own path, and it came from path.join, so Windows would have emitted a backslash
where the template has a literal '/'. Restored, with the distinction recorded:
the containment check there is a GATE, not a path producer.
A TEST THAT COULD NOT FAIL. The first symlink regression planted its symlink
from inside a hooked fs.readdirSync and never asserted the planting happened —
if the hook did not fire, the "nothing was written outside" assertion passed
trivially, green against vulnerable code. It now asserts the plant, matching its
sibling. The other two were re-checked: one already asserted its equivalent, the
other plants synchronously and cannot silently no-op.
THE SYMLINK FIX ITSELF, now that the tests are proven red on the matrix.
isPathConfined is lexical by design and structurally cannot see a symlink; three
callers relied on it with no defense of their own. install-engine.cts:1608 and
install-profiles.cts:880 refuse to mkdir/write through a link — mkdirSync with
recursive:true does NOT throw on an existing symlink-to-directory, so a planted
link redirected the SKILL.md write outside the install root.
install-profiles.cts:755 refuses to read through one — statSync FOLLOWS links,
so an outside file's contents were returned and installed as a skill body. Each
mirrors the guard retired-artifact-cleanup.cts:77 already uses.
Severity stated accurately rather than dramatically: only the read at :755 needs
no race. _removeGsdEntries sweeps a pre-planted link at :1608 before the write
loop, and :880's stageDir is a fresh mkdtemp, so both of those require winning a
window. They are fixed as defense-in-depth, not as live exploits.
ALSO: the Changed changeset claimed "every command's observable behavior [is]
unchanged". Three rejection messages are reworded. It now says so, and says that
none of them reveals a host path it previously hid. A stale comment in
verify.cts still named validatePath; an init.cts warning hardcoded "resolves
outside the project directory" for a check that also rejects absolute paths, NUL
bytes and empty strings; and the rationale deleted with check-glossary-refs'
retired helper is restored, noting honestly that a rejected token is now
realpath-resolved before rejection rather than rejected by string comparison.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
sim
|
cd58aaabf4 |
refactor(#4653): drain the containment duplicates and record the two rulings
Phase 3 of epic #4636, stage 3c. ADR-4650 decision 6: a wrapper may decide HOW to degrade, never WHETHER a path is contained. Four implementations are drained on that rule; two are retained, with the reasons recorded rather than assumed. DRAINED — the containment decision now comes from the canonical predicate: scripts/check-glossary-refs.cjs local isWithinRoot deleted outright. src/installer-migrations.cts ensureInsideConfig keeps its throw and its lexical fullPath; only the decision moves. src/planning-inspect.cts isPathContained keeps must-exist as its own condition; only the decision moves. Two of those are wrappers rather than deletions, and each is a wrapper for a reason that would have been a silent behavior change if collapsed naively: - `isPathContained` returns FALSE for a path that does not exist, because fs.realpathSync throws ENOENT and its catch swallows it. The canonical predicate does the opposite: for a missing target it walks up to the nearest existing ancestor and ACCEPTS a not-yet-created path under the root. Its callers at planning-inspect.cts:747 and :839 guard a phaseDir immediately before readdirSync, so under a naive swap a missing phaseDir would stop reporting scope UNREADABLE and start throwing ENOENT out of readdirSync. Existence is therefore kept as an explicit local requirement. - `ensureInsideConfig` returns a LEXICAL fullPath that both callers consume for existsSync and for journal entries. The canonical predicate realpath-resolves, so if configDir is itself a symlink the two differ. The decision is canonical; the returned value stays lexical. Its message is likewise preserved verbatim, which is why this uses tryWithinRoot plus an explicit throw rather than assertWithinRoot. `isWithinRoot` in planning-inspect is left in place and documented: it is a pure comparison over paths the CALLER has already resolved, which readDocument does inline specifically to keep a third degradation shape (exists-but-unreadable vs absent) that neither isPathContained nor the canonical predicate expresses. It is the comparison step of one implementation, not a second implementation. RETAINED, DELIBERATELY — gsd-core/bin/gsd-tools.cjs. My own design document said "collapse" and that was wrong. The file carries an explicit comment forbidding it, and the comment is correct: its three checks reject symlinks OUTRIGHT, which is strictly stricter than the canonical predicate, not a reimplementation of it. The canonical predicate accepts a link whose target lands inside the root — for a restore that is still wrong, because writing through the link overwrites whatever it points at instead of materializing a regular file. Collapsing would have reintroduced that hole. The comment is updated to name the current exported predicate, to record that this was reviewed under this phase and deliberately not collapsed, and to note that isInsideDir treats target === root as NOT contained — the one implementation in the repo that does. THE configHome RULING — retained lexical, and a false safety claim corrected. isPathConfined stays lexical because two of its callers must validate a destSubpath BEFORE the mkdirSync that creates it (install-engine.cts:1608, install-profiles.cts:880), where realpath cannot resolve and a realpath-based predicate would reject every legitimate install. Its docstring's justification, however, did not survive being checked. It cited capability-source.cts:491,577,675 as the upstream symlink rejection that made the lexical form safe. Read directly: :491 is a blank line before assertSafeId's JSDoc and :577 is an entry-count budget check. Neither is a symlink check. The real guards are :585-586 and :671-674. Worse than stale line numbers, the claim that this "keeps every caller of this function's callers symlink-safe" is false: that rejection lives in capability-source's staging path and covers only the capability-loader route to assertDescriptorConfined. Three other callers do not reach it, and only retired-artifact-cleanup.cts:69 carries its own defense (its lstatSync check at :77). The docstring now states what is actually true and cites the lines that actually exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4e60dba717 |
fix(#3604): make glossary ref visibility independent of backtick parity (#3680)
* test(#3604): pin parity-dependent ref visibility in the glossary gate * fix(#3604): make glossary ref visibility independent of backtick parity * chore(#3604): regenerate CONTEXT-INDEX for corrected predicates * chore(#3604): regenerate examples CONTEXT-INDEX for corrected predicates * fix(#3604): complete retired-family exemptions and pin the guard rails * chore(#3604): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e276cc7f00 |
enhance(#2778): make the size-ratchet failure name its own remedy (#2780)
* fix(#2778): exempt intentionally-absent paths from the glossary gate check-glossary-refs asserts that every backticked tests/ token in CONTEXT.md resolves on disk. tests/emitted-drift-ack.json (ADR-2719 section 3) is absent on a healthy next BY DESIGN — it appears only inside a PR that needs it, which is what makes touching it the alarm. It passed before only by accident of backtick pairing: CONTEXT.md's RULESET entries are themselves backtick-wrapped and contain backticks, so the token happened to fall outside a code span. Any edit that shifted the parity exposed it. A gate that passes by luck is not passing. The exemption is exact, not a prefix hole: a sibling missing tests/ path still fails, and a test locks that. * feat(#2778): make the size-ratchet failure name its own remedy The growth branch stated a requirement and withheld the means of satisfying it: no ack file named, no schema, no key format, and no do-not-regenerate line — so the likeliest guess was to hunt for a baseline that #2724 deleted. Observed live on #2543. All remediation now comes from one frozen REMEDIATION export whose example document is rendered from ACK_VERSION, so the taught schema cannot drift from the schema parseAck accepts. A round-trip test feeds the printed document back through parseAck. The report is now built as a typed IR (buildReport) that formatReport renders, so tests assert on structure rather than prose, per CONTRIBUTING.md's raw-text-matching rule. Two defects found and fixed inline while building: - diffEmitted's validation early-return omitted newFileCapExceeded while formatReport reads its length, so the branch that reports a failed git diff threw a TypeError instead of naming the problem. - Printing one complete ack document per failing branch made each read as the whole file, so pasting the second over the first silently lost an acknowledgment. One document now covers the whole report. Closes #2778 * chore(#2778): backfill changeset pr number to 2780 |
||
|
Tom Boucher
|
f2c077df38 |
chore(#2387): refactor CONTEXT.md legacy content + add glossary drift gate (#2391)
* chore(#2387): refactor CONTEXT.md legacy content + add glossary drift gate Apply the audit-and-enforce concept from the ADR index (#2356) to CONTEXT.md: correct stale facts, and add a CI gate so the machine-verifiable claims can't silently re-rot. CONTEXT.md was entirely hand-maintained with nothing checking its claims against the shipped tree, so it had rotted. An audit against live code (Memtrace + filesystem + gh), each finding adversarially re-verified, drove 38 factual corrections + 1 surfaced by the new gate: - Dead references: Package Identity named @opengsd/get-shit-done-redux (package is @opengsd/gsd-core); Shell Command Projection named run-git/run-npm/run-tool (real exports execGit/execNpm/execTool); a partial docs/adr/1606 ref; retired sdk/ framing. - Superseded facts: allRuntimes 15 -> 17 (pi #2102, zcode); "seven nested-loader runtimes" -> five (claude reverted flat #924, antigravity flat); stacked-PR examples rebasing onto main -> next; QUOTA_SENTINELS precedence corrected to match src/agent-command-router.cts. - Drifted CONTRIBUTING.md line citations refreshed. Per CONTRIBUTING.md:179, only stale FACTS were corrected -- no maintainer intent, lesson, or opinion was rewritten, and the append-only session log is untouched except one dated in-place superseding note. The three tests that assert on CONTEXT.md content (phase6-capstone-conformance, tracer-bullet, external-job-waiting) keep all their anchors. New scripts/check-glossary-refs.cjs (--check, wired into lint:generated-sync): - Check A: every backticked file reference under a TRACKED_PREFIXES allowlist resolves on disk. Generated gsd-core/bin/lib/*.cjs (77 refs, gitignored), ~/-paths, .planning/, and bare filenames are deliberately skipped so a clean CI checkout never false-fails. - Check B: the allRuntimes count + member set in the glossary prose match bin/install.js's allRuntimes literal (drifts on every runtime addition). tests/check-glossary-refs.test.cjs covers both, including the false-positive guard that a missing bin/lib/*.cjs ref does NOT trip the gate. Closes #2387 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2387): confine glossary-gate file refs to ROOT (no `..` traversal) Pre-PR security review finding (low): extractTrackedRefs fed tokens straight to fs.existsSync(path.join(ROOT, token)), and PATH_TOKEN_RE admits `.` in a segment, so a CONTEXT.md token like `src/../../../etc/passwd` passed the `src/` prefix check and normalized to an out-of-tree absolute path — turning the doc lint into a filesystem-existence oracle on the CI host (existsSync only; CONTEXT.md is a trusted committed file, hence low severity, but a defense-in-depth gap). Add isWithinRoot() confinement in extractTrackedRefs: a token is dropped unless path.resolve(ROOT, token) stays within ROOT. A CONTEXT.md reference is always a plain in-repo path, so a `..` escape is never legitimate. Regression test asserts a `..`-bearing token is skipped and never named in output. Refs #2387 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2387): drop legacy `get-shit-done` name from a CONTEXT.md defect entry CI lint-legacy-dir-name failed: the line-928 upstream-issue re-point I applied wrote the historical provenance as "gsd-build/get-shit-done#3545", and scripts/lint-legacy-dir-name.cjs forbids the legacy `get-shit-done` name. Reword to "moved from #3545 in the predecessor repo" — same provenance, no legacy name. Caught by `npm run lint:ci` (the CI lint chain), which I had not run locally — lint:generated-sync + eslint do not include lint-legacy-dir-name. Refs #2387 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |